diff --git a/README.md b/README.md index 5ddb26d..986123e 100644 --- a/README.md +++ b/README.md @@ -624,6 +624,12 @@ enforced rather than hoped for: - **It never touches your working tree.** Local builds use a throwaway git worktree; hosted builds a shallow clone in a scratch directory. The branch survives; the workspace does not. +- **It reviews its own diff before asking you to.** Between verify and the PR, + a read-only pass reads the change against the spec and puts its findings in + the PR body — advisory, never blocking, and an empty list is a real answer. +- **Effort is per station.** Builds and reviews run at `xhigh`, spec and + grounding at `high`, indexing at `low`; one project setting moves all of them + ([`knowledge/decisions/0021-…`](knowledge/decisions/0021-effort-is-per-station-and-builds-review-themselves.md)). - **Every run starts from a clean branch.** `spec/-` is cut from the default branch each time, so a rebuild replaces the last attempt rather than stacking on it. The PR is titled `[] - `. diff --git a/apps/api/src/agents/anthropic.service.ts b/apps/api/src/agents/anthropic.service.ts index fdab5a0..8c4cac5 100644 --- a/apps/api/src/agents/anthropic.service.ts +++ b/apps/api/src/agents/anthropic.service.ts @@ -1,6 +1,6 @@ import Anthropic from '@anthropic-ai/sdk'; import { Injectable } from '@nestjs/common'; -import { MODELS, type ModelId, type TokenUsage } from '@specd/shared'; +import { MODELS, type ModelId, type TokenUsage, STATION_EFFORT, type Effort } from '@specd/shared'; import { AiNotConfigured } from '../common/errors.js'; export interface ModelCallOptions { @@ -11,7 +11,7 @@ export interface ModelCallOptions { /** JSON Schema. When set, the reply is guaranteed to match it. */ schema?: Record<string, unknown>; maxTokens?: number; - effort?: 'low' | 'medium' | 'high' | 'xhigh' | 'max'; + effort?: Effort; /** Called with every text delta, so a run log can stream while it thinks. */ onDelta?: (text: string) => void; } @@ -71,7 +71,7 @@ export class AnthropicService { // `effort` sets the ceiling on total spend. thinking: { type: 'adaptive' }, output_config: { - effort: opts.effort ?? 'high', + effort: opts.effort ?? STATION_EFFORT.spec, ...(opts.schema ? { format: { type: 'json_schema' as const, schema: opts.schema } } : {}), diff --git a/apps/api/src/agents/build.agent.test.ts b/apps/api/src/agents/build.agent.test.ts index 328d670..bb22991 100644 --- a/apps/api/src/agents/build.agent.test.ts +++ b/apps/api/src/agents/build.agent.test.ts @@ -1,4 +1,5 @@ import { describe, expect, it } from 'vitest'; +import type { SpecView } from '@specd/shared'; import { buildPrBody, looksUnrunnable } from './build.agent.js'; /** @@ -99,3 +100,71 @@ describe('the PR body when evidence moved', () => { expect(body).toContain('did not'); }); }); + + +describe('the review section of a pull request body', () => { + const spec = { + ticketKey: 'E-101', + title: 'Add CSV export', + version: 2, + approvedBy: 'jpark', + approvedAt: '2026-08-18T00:00:00.000Z', + content: { requirements: [], design: [], tasks: [] }, + } as unknown as SpecView; + + const base = { commits: 3, verifyPassed: true, verifyCommand: 'pnpm test', asBuilt: 'k/s/E-101.md' }; + + it('says nothing at all when the pass could not run', () => { + // An absent review must read as absent. A "no findings" banner for a pass + // that never happened is the same lie as a green verify for tests that + // never ran. + expect(buildPrBody(spec, base)).not.toMatch(/Review pass/); + expect(buildPrBody(spec, { ...base, review: null })).not.toMatch(/Review pass/); + expect( + buildPrBody(spec, { ...base, review: { verdict: 'unreviewed', findings: [] } }), + ).not.toMatch(/Review pass/); + }); + + it('reports a clean pass, because silence would be indistinguishable from not running', () => { + const body = buildPrBody(spec, { + ...base, + review: { verdict: 'clean', summary: 'Does what the spec approved.', findings: [] }, + }); + expect(body).toMatch(/found nothing to raise/); + expect(body).toContain('Does what the spec approved.'); + }); + + it('leads with blocking findings and says they did not block', () => { + const body = buildPrBody(spec, { + ...base, + review: { + verdict: 'findings', + summary: 'Exports, but drops a column.', + findings: [ + { where: 'src/a.ts:12', severity: 'nit', what: 'Stray import.' }, + { where: 'src/b.ts:40', severity: 'blocking', what: 'Header row omitted.', against: 'SHALL include a header' }, + { where: 'src/c.ts:7', severity: 'consider', what: 'Could stream instead.' }, + ], + }, + }); + + // Ordered by severity: a reviewer scanning the top of the list must hit + // the blocking one first, whatever order the model emitted them in. + expect(body.indexOf('src/b.ts:40')).toBeLessThan(body.indexOf('src/c.ts:7')); + expect(body.indexOf('src/c.ts:7')).toBeLessThan(body.indexOf('src/a.ts:12')); + + expect(body).toMatch(/3 finding\(s\), 1 of them blocking/); + expect(body).toMatch(/advisory — they did not stop the build/); + expect(body).toContain('SHALL include a header'); + }); + + it('keeps the acceptance criteria below the findings', () => { + // The findings are the new reading; the criteria are what the reader + // checks them against. Reversing that buries the part nobody has seen. + const body = buildPrBody(spec, { + ...base, + review: { verdict: 'findings', summary: 's', findings: [{ where: 'a:1', severity: 'nit', what: 'x' }] }, + }); + expect(body.indexOf('### Review pass')).toBeLessThan(body.indexOf('### Acceptance criteria')); + }); +}); diff --git a/apps/api/src/agents/build.agent.ts b/apps/api/src/agents/build.agent.ts index 109ca7c..db2da3d 100644 --- a/apps/api/src/agents/build.agent.ts +++ b/apps/api/src/agents/build.agent.ts @@ -12,10 +12,12 @@ import { type CitationDrift, type ModelId, type SpecTask, - type SpecView, + type SpecView, STATION_EFFORT, + type Effort, } from '@specd/shared'; import type { DetectedStack } from '@specd/templates'; import { ClaudeCodeProvider } from './claude-code.provider.js'; +import { ReviewAgent, type ReviewFindings } from './review.agent.js'; import { WorkspaceService } from '../vcs/workspace.js'; import type { RunHandle } from '../runs/runs.service.js'; @@ -29,6 +31,8 @@ export interface BuildResult { asBuiltPath: string; /** Where to review it. A PR on hosted providers; null in local mode. */ reviewUrl: string | null; + /** What the review pass found, or null when it could not run. */ + review: ReviewFindings | null; } /** One task, with its prompt already rendered so a runner needs no spec knowledge. */ @@ -49,6 +53,10 @@ export interface PreparedBuildTask { export interface PreparedBuild { system: string; model: ModelId; + /** Travels to a runner so a dispatched build works as hard as a local one. */ + effort: Effort; + /** Rendered here, executed wherever the workspace is. */ + review: { system: string; brief: string; schema: Record<string, unknown> }; branch: string; asBuiltPath: string; asBuiltCommitMessage: string; @@ -74,6 +82,13 @@ export interface BuildRunnerReport { commits: number; verifyPassed: boolean | null; verifyOutput: string | null; + /** + * The review the runner ran on its own machine, if it did. It has to happen + * there: the workspace is on the runner's disk and gone by the time this + * report arrives, so the alternative is that dispatched builds silently ship + * unreviewed while local ones do not. + */ + review?: ReviewFindings | null; } /** @@ -101,6 +116,7 @@ export class BuildAgent { constructor( private readonly claudeCode: ClaudeCodeProvider, private readonly workspaces: WorkspaceService, + private readonly reviewer: ReviewAgent, ) {} /** @@ -117,6 +133,8 @@ export class BuildAgent { projectName: string; knowledgeExcerpts: string; model: ModelId; + /** Defaults to the build station's level. */ + effort?: Effort; drifted?: CitationDrift[]; }): Promise<PreparedBuild> { const { repo, spec, model } = input; @@ -136,6 +154,8 @@ export class BuildAgent { return { system: BUILD_SYSTEM_PROMPT, model, + effort: input.effort ?? STATION_EFFORT.build, + review: this.reviewer.prepare(spec), branch, asBuiltPath: asBuilt, drifted: input.drifted ?? [], @@ -195,15 +215,71 @@ export class BuildAgent { verifyOutput: report.verifyOutput, asBuiltPath: prepared.asBuiltPath, reviewUrl: published.url, + // The runner built on its own machine and its workspace is gone by the + // time this runs, so there is nothing here to read. `runner-review` + // covers that path instead of this one. + review: report.review ?? null, }; } + /** + * Read the branch back and say what is wrong with it. + * + * Best-effort throughout: the commits exist and verify has already spoken, + * so a review that cannot run costs an opinion, not the build. That is the + * same posture the review *surface* takes one step later — nothing after the + * work is allowed to lose the work. + */ + private async reviewOwnWork(input: { + spec: SpecView; + model: ModelId; + effort: Effort; + workspace: { dir: string; baseBranch: string }; + run: RunHandle; + }): Promise<ReviewFindings | null> { + const { spec, model, effort, workspace, run } = input; + try { + const diff = await this.workspaces.diff(workspace.dir, workspace.baseBranch); + if (!diff.trim()) return null; + + await run.log(`reviewing the diff at effort ${effort}`); + const findings = await this.reviewer.run({ + spec, + model, + effort, + workspaceDir: workspace.dir, + diff, + }); + + if (findings.verdict === 'unreviewed') { + await run.log('the review pass did not answer — opening the PR without it', 'warn'); + return null; + } + + const blocking = findings.findings.filter((f) => f.severity === 'blocking').length; + await run.log( + findings.findings.length === 0 + ? 'review found nothing to raise' + : `review raised ${findings.findings.length} finding(s), ${blocking} blocking — advisory, in the PR body`, + blocking > 0 ? 'warn' : 'info', + ); + return findings; + } catch (err) { + await run.log( + `the review pass failed (${err instanceof Error ? err.message : String(err)}) — opening the PR without it`, + 'warn', + ); + return null; + } + } + async run(input: { repo: Repository; spec: SpecView; projectName: string; knowledgeExcerpts: string; model: ModelId; + effort?: Effort; run: RunHandle; }): Promise<BuildResult> { const { repo, spec, run, model } = input; @@ -228,6 +304,7 @@ export class BuildAgent { const result = await this.claudeCode.code({ model, + effort: plan.effort, workspaceDir: workspace.dir, system: plan.system, user: task.prompt, @@ -288,18 +365,31 @@ export class BuildAgent { const commits = await this.workspaces.commitCount(workspace.dir, workspace.baseBranch); + // Station 05b. After verify, because a review of code whose tests do not + // run is mostly noise about the tests; before publish, because the point + // is for the findings to reach the pull request rather than a run log + // somebody would have to go and find. + const review = await this.reviewOwnWork({ + spec, + model, + effort: plan.effort, + workspace, + run, + }); + // Hand the branch to wherever this team reviews. On a hosted provider the // workspace is a temporary clone about to be deleted, so a branch that is // never pushed is a build that produced nothing. const published = await workspace.publish({ title: specPrTitle(spec.ticketKey, spec.title), body: buildPrBody(spec, { - commits, - verifyPassed, - verifyCommand, - asBuilt, - drifted: plan.drifted ?? [], - }), + commits, + verifyPassed, + verifyCommand, + asBuilt, + drifted: plan.drifted ?? [], + review, + }), }); await run.log(`branch ${branch} ready · ${commits} commit(s) · ${published.reviewHint}`); @@ -313,6 +403,7 @@ export class BuildAgent { verifyOutput, asBuiltPath: asBuilt, reviewUrl: published.url, + review, }; } finally { // The worktree goes; the branch it produced stays. @@ -417,6 +508,43 @@ ${ Implement it in the current directory.`; } +/** + * The review section of the pull request body. + * + * Advisory, and says so in the text: these findings did not block the build and + * are not a second gate. What they are is the reading a human would otherwise + * do cold — so they sit above the acceptance criteria, where that reading + * starts, rather than in a run log nobody opens. + */ +function renderReview(review: ReviewFindings | null | undefined): string[] { + if (!review || review.verdict === 'unreviewed') return []; + + if (review.findings.length === 0) { + return ['', `> **Review pass found nothing to raise.** ${review.summary ?? ''}`.trimEnd()]; + } + + const order = { blocking: 0, consider: 1, nit: 2 } as const; + const sorted = [...review.findings].sort((a, b) => order[a.severity] - order[b.severity]); + const blocking = sorted.filter((f) => f.severity === 'blocking').length; + + return [ + '', + '### Review pass', + '', + review.summary ?? '', + '', + `${sorted.length} finding(s)${blocking ? `, ${blocking} of them blocking` : ''}. ` + + 'These are advisory — they did not stop the build, and nothing here has been ' + + 'acted on. A human decides which are real.', + '', + ...sorted.map( + (f) => + `- **${f.severity}** \`${f.where}\` — ${f.what}` + + (f.against ? `\n _against:_ ${f.against}` : ''), + ), + ]; +} + /** Runs the repo's own verify command. Never a model-supplied string. */ function runShell( command: string, @@ -469,6 +597,7 @@ export function buildPrBody( verifyCommand: string | null; asBuilt: string; drifted?: CitationDrift[]; + review?: ReviewFindings | null; }, ): string { const verify = @@ -501,6 +630,8 @@ export function buildPrBody( ] : []; + const review = renderReview(meta.review); + return [ `Built from **${spec.ticketKey} v${spec.version}** — ${spec.title}`, '', @@ -510,6 +641,7 @@ export function buildPrBody( `- As-built spec filed at \`${meta.asBuilt}\``, `- ${verify}`, ...drift, + ...review, '', '---', '', diff --git a/apps/api/src/agents/claude-code.provider.ts b/apps/api/src/agents/claude-code.provider.ts index eff1904..9d5b09e 100644 --- a/apps/api/src/agents/claude-code.provider.ts +++ b/apps/api/src/agents/claude-code.provider.ts @@ -10,7 +10,16 @@ import { type ClaudeCodeEnvelope, type ModelId, type TokenUsage, + STATION_EFFORT, + type Effort, } from '@specd/shared'; + +const EMPTY_USAGE: TokenUsage = { + inputTokens: 0, + outputTokens: 0, + cacheReadInputTokens: 0, + cacheCreationInputTokens: 0, +}; import { AiNotConfigured } from '../common/errors.js'; import type { ModelCallOptions, ModelCallResult } from './anthropic.service.js'; @@ -131,6 +140,8 @@ export class ClaudeCodeProvider { system: string; user: string; workspaceDir: string; + /** Defaults to the build station's level — the highest of any station. */ + effort?: Effort; timeoutMs?: number; }): Promise<{ text: string; usage: TokenUsage; model: ModelId | null }> { if (!(await this.isAvailable())) { @@ -145,6 +156,11 @@ export class ClaudeCodeProvider { 'json', '--model', input.model, + // The CLI takes the same five levels the API does. Omitting it left + // subscription-mode builds running at whatever the CLI defaulted to, + // regardless of what specd had decided for the station. + '--effort', + input.effort ?? STATION_EFFORT.build, '--append-system-prompt', input.system, '--permission-mode', @@ -207,6 +223,96 @@ export class ClaudeCodeProvider { }; } + /** + * A read-only pass over a workspace, answering in a schema. + * + * Between `call()` (no tools — a text transform) and `code()` (editing tools + * — it changes the repository), because a reviewer needs the third thing: + * it must open files the diff only alludes to, and it must not be able to + * "fix" what it finds. `Write` and `Edit` are denied rather than merely + * unused, so an over-helpful pass cannot quietly amend the branch it was + * asked to assess. + */ + async review<T>(input: { + model: ModelId; + effort?: Effort; + system: string; + user: string; + schema: Record<string, unknown>; + workspaceDir: string; + timeoutMs?: number; + }): Promise<{ parsed: T | null; usage: TokenUsage; model: ModelId | null }> { + if (!(await this.isAvailable())) { + throw new AiNotConfigured( + 'Reviewing a build drives the Claude Code CLI, and `claude` is not on PATH on this machine.', + ); + } + + const args = [ + '--print', + '--output-format', + 'json', + '--model', + input.model, + '--effort', + input.effort ?? STATION_EFFORT.review, + '--append-system-prompt', + `${input.system}\n${schemaInstruction(input.schema)}`, + '--permission-mode', + 'plan', + '--allowedTools', + 'Read', + 'Glob', + 'Grep', + '--disallowed-tools', + 'Write', + 'Edit', + 'Bash', + 'WebFetch', + 'WebSearch', + 'Task', + 'NotebookEdit', + ]; + + const { stdout, stderr, code, timedOut } = await this.exec( + args, + input.user, + input.timeoutMs ?? 600_000, + input.workspaceDir, + ); + + // Every failure here is soft. The branch is built and verified; a review + // that could not run is a missing opinion, not a broken build. + if (timedOut || code !== 0) { + this.logger.warn( + `review pass did not complete: ${timedOut ? 'timed out' : `exit ${code}`} ${(stderr || '').slice(0, 200)}`, + ); + return { parsed: null, usage: EMPTY_USAGE, model: null }; + } + + let envelope: ClaudeCodeEnvelope; + try { + envelope = JSON.parse(stdout) as ClaudeCodeEnvelope; + } catch { + this.logger.warn('review pass returned no readable envelope'); + return { parsed: null, usage: EMPTY_USAGE, model: null }; + } + + const usage: TokenUsage = { + inputTokens: envelope.usage?.input_tokens ?? 0, + outputTokens: envelope.usage?.output_tokens ?? 0, + cacheReadInputTokens: envelope.usage?.cache_read_input_tokens ?? 0, + cacheCreationInputTokens: envelope.usage?.cache_creation_input_tokens ?? 0, + }; + + try { + return { parsed: parseAgainstSchema<T>(envelope.result ?? '', input.schema), usage, model: null }; + } catch { + this.logger.warn('review pass did not answer in the requested shape'); + return { parsed: null, usage, model: null }; + } + } + private async invoke( opts: ModelCallOptions, system: string, @@ -229,6 +335,8 @@ export class ClaudeCodeProvider { 'json', '--model', opts.model, + '--effort', + opts.effort ?? STATION_EFFORT.spec, '--system-prompt', system, // Drop the environment/cwd preamble — this is a text transform. @@ -371,6 +479,16 @@ export class ClaudeCodeProvider { // Large prompts go over stdin — argv would hit the platform limit once a // spec carries a dozen knowledge excerpts. + // A child that exited — or never started — leaves a stdin pipe with + // nobody on the other end, and Node reports the write as an `error` on + // the stream rather than on the child. Unhandled, that is an uncaught + // exception for a prompt the callee was never going to read. The runner + // hit this first (`apps/runner/src/claude.ts`) and this is the same + // guard: the exit code and stderr below already describe the outcome. + child.stdin.on('error', (err: NodeJS.ErrnoException) => { + if (err.code === 'EPIPE' || err.code === 'ERR_STREAM_DESTROYED') return; + stderr += `\n[specd] could not write to claude's stdin: ${err.message}`; + }); child.stdin.write(stdin); child.stdin.end(); }); diff --git a/apps/api/src/agents/onboarding.agent.ts b/apps/api/src/agents/onboarding.agent.ts index ed01fb1..8a20689 100644 --- a/apps/api/src/agents/onboarding.agent.ts +++ b/apps/api/src/agents/onboarding.agent.ts @@ -19,6 +19,7 @@ import { VcsService } from '../vcs/vcs.service.js'; import type { RepoFile, RepoSnapshot, RepoTarget, VcsAdapter } from '../vcs/vcs.types.js'; import { ModelRouter } from './model.router.js'; import type { RunHandle } from '../runs/runs.service.js'; +import { STATION_EFFORT, type Effort } from '@specd/shared'; /** * What we ask the model for. @@ -285,6 +286,8 @@ export class OnboardingAgent { apiKey: string | null; model: ModelId; mode: AiMode; + /** Defaults to the ground station's level. */ + effort?: Effort; run: RunHandle; }): Promise<{ branch: string; url: string | null; reviewHint: string; fileCount: number }> { const { repo, projectName, apiKey, model, mode, run } = input; @@ -301,7 +304,7 @@ export class OnboardingAgent { apiKey: apiKey ?? '', model, maxTokens: 32_000, - effort: 'high', + effort: input.effort ?? STATION_EFFORT.ground, system: prepared.system, user: prepared.user, schema: prepared.schema, diff --git a/apps/api/src/agents/pipeline.service.ts b/apps/api/src/agents/pipeline.service.ts index e4ba965..d64fc08 100644 --- a/apps/api/src/agents/pipeline.service.ts +++ b/apps/api/src/agents/pipeline.service.ts @@ -7,8 +7,7 @@ import { specBranchName, type CitationDrift, type ModelId, - type SpecView, -} from '@specd/shared'; + type SpecView, effortFor, type Effort } from '@specd/shared'; import { SpecNotApproved } from '../common/errors.js'; import { ProjectsService } from '../projects/projects.service.js'; import { RepositoriesService } from '../projects/repositories.service.js'; @@ -160,6 +159,7 @@ export class PipelineService { const repo = await this.repositories.get(input.projectId, input.repositoryId); const ai = await this.connections.resolveAi(project.id, project.defaultModel); const model = this.resolveModel(ai.model, project.defaultModel); + const effort = effortFor('ground', project.effort as Effort | null); // The row was queued before anything knew which model it would use. await this.runs.setModel(input.runId, model); @@ -191,6 +191,7 @@ export class PipelineService { apiKey: ai.apiKey, model, mode: ai.mode, + effort, run, }); await this.runs.finish(input.runId, { @@ -278,7 +279,7 @@ export class PipelineService { schema: prepared.schema, model, maxTokens: 32_000, - effort: 'high', + effort: effortFor('spec', project.effort as Effort | null), chunks: prepared.chunks, coverage: prepared.coverage, slug: prepared.slug, @@ -311,6 +312,7 @@ export class PipelineService { apiKey: ai.apiKey, model, mode: ai.mode, + effort: effortFor('spec', project.effort as Effort | null), run, revisionNotes, previousContent, @@ -368,6 +370,9 @@ export class PipelineService { const ai = await this.connections.resolveAi(project.id, project.defaultModel); const model = this.resolveModel(ai.model, project.defaultModel); + // The highest of any station by default: this one writes code a human is + // about to review and merge. + const effort = effortFor('build', project.effort as Effort | null); // Ground the build in the same knowledge the spec was drafted against. const { chunks } = await this.knowledge.retrieve( @@ -481,6 +486,7 @@ export class PipelineService { projectName: project.name, knowledgeExcerpts, model, + effort, drifted, }); @@ -489,6 +495,8 @@ export class PipelineService { kind: 'build', model, system: prepared.system, + effort: prepared.effort, + review: prepared.review, branch: prepared.branch, asBuiltPath: prepared.asBuiltPath, asBuiltCommitMessage: prepared.asBuiltCommitMessage, @@ -536,6 +544,7 @@ export class PipelineService { projectName: project.name, knowledgeExcerpts, model, + effort, drifted, run, }); @@ -557,6 +566,7 @@ export class PipelineService { projectName: string; knowledgeExcerpts: string; model: ModelId; + effort: Effort; run: Awaited<ReturnType<RunsService['start']>>; }): Promise<void> { const { run, spec } = input; diff --git a/apps/api/src/agents/review.agent.ts b/apps/api/src/agents/review.agent.ts new file mode 100644 index 0000000..46d0d02 --- /dev/null +++ b/apps/api/src/agents/review.agent.ts @@ -0,0 +1,168 @@ +import { Injectable } from '@nestjs/common'; +import { STATION_EFFORT, type Effort, type ModelId, type SpecView } from '@specd/shared'; +import { ClaudeCodeProvider } from './claude-code.provider.js'; + +/** + * Station 05b — Review. + * + * specd ran the repository's verify command and stopped. That answers "do the + * tests pass", which is the question a test suite already answers; it does not + * answer "is this the change we approved, and is it any good". The build agent + * cannot answer that about its own work in the same turn it does the work — + * so this is a separate pass, over the diff, with the spec beside it. + * + * Advisory by construction. It reports into the pull request a human is about + * to read; it does not fail a build. Making a finding refusable is a per-project + * policy decision (2.4), the same line citation drift sits on — an unrelated + * opinion should never be able to stop an approved spec from shipping. + */ +@Injectable() +export class ReviewAgent { + constructor(private readonly claudeCode: ClaudeCodeProvider) {} + + /** The prompt half, rendered without side effects so a runner can execute it. */ + prepare(spec: SpecView): { system: string; schema: Record<string, unknown>; brief: string } { + return { system: REVIEW_SYSTEM_PROMPT, schema: REVIEW_SCHEMA, brief: specBrief(spec) }; + } + + async run(input: { + spec: SpecView; + model: ModelId; + effort?: Effort; + workspaceDir: string; + diff: string; + }): Promise<ReviewFindings> { + const { system, schema, brief } = this.prepare(input.spec); + + const result = await this.claudeCode.review<ReviewFindings>({ + model: input.model, + effort: input.effort ?? STATION_EFFORT.review, + workspaceDir: input.workspaceDir, + system, + schema, + user: reviewPrompt(brief, input.diff), + }); + + return result.parsed ?? { findings: [], verdict: 'unreviewed' }; + } +} + +export interface ReviewFinding { + /** `path:line`, so it is clickable where the PR renders it. */ + where: string; + severity: 'blocking' | 'consider' | 'nit'; + what: string; + /** Which acceptance criterion or design claim it bears on, if any. */ + against?: string; +} + +export interface ReviewFindings { + findings: ReviewFinding[]; + verdict: 'clean' | 'findings' | 'unreviewed'; + /** One line a reviewer reads before the list. */ + summary?: string; +} + +const REVIEW_SCHEMA = { + type: 'object', + properties: { + verdict: { + type: 'string', + enum: ['clean', 'findings'], + description: '"clean" only if you would approve this as written.', + }, + summary: { + type: 'string', + description: + 'One sentence: what this change does, and whether it does what the spec approved. No preamble.', + }, + findings: { + type: 'array', + description: + 'What a reviewer should look at. Empty is a valid and useful answer — do not pad it.', + items: { + type: 'object', + properties: { + where: { type: 'string', description: 'path:line, from the diff.' }, + severity: { + type: 'string', + enum: ['blocking', 'consider', 'nit'], + description: + 'blocking: wrong, unsafe, or not what the spec approved. consider: a real improvement. nit: style.', + }, + what: { + type: 'string', + description: + 'The problem and why it is one, in one or two sentences. Name the failing input where you can.', + }, + against: { + type: 'string', + description: + 'The acceptance criterion or design claim this bears on, quoted. Omit if none.', + }, + }, + required: ['where', 'severity', 'what'], + additionalProperties: false, + }, + }, + }, + required: ['verdict', 'summary', 'findings'], + additionalProperties: false, +} as const; + +const REVIEW_SYSTEM_PROMPT = `You are reviewing a diff an agent wrote from an approved spec, for a +human who is about to decide whether to merge it. + +You have read-only tools. Use them: the diff is what changed, not what the code +around it does, and a finding you could not have made without opening the file +is worth more than one you could have made from the diff alone. + +The question is not "is this clever". It is: + +1. Does it do what the spec approved — no less, and no more? Work the spec did + not ask for is a finding, not a bonus. +2. Is it correct? Name the input or state that breaks it. A concern you cannot + turn into a failing case is a nit at most. +3. Does it match this codebase? Read the surrounding code and the knowledge + base before calling something unidiomatic. + +Rules: + +- Report what you find, at the severity you actually believe. Do not filter for + importance — a human ranks these, and a finding you suppressed because it + seemed minor is one they never got to weigh. +- An empty findings list is a real answer. Do not manufacture one to look + thorough; padding here is what makes the whole section ignorable. +- Every finding names a location from the diff and, where it bears on one, the + acceptance criterion or design claim it is measured against. +- You are advisory. You are not blocking the merge, and you are not rewriting + the code — say what is wrong, not what you would have written instead.`; + +function specBrief(spec: SpecView): string { + const criteria = spec.content.requirements + .flatMap((r) => r.criteria) + .map((c) => `- ${c.keyword} ${c.trigger} THE SYSTEM SHALL ${c.response}`) + .join('\n'); + + const design = spec.content.design + .map((d) => (d.citation ? `- ${d.text} [per ${d.citation}]` : `- ${d.text} [UNVERIFIED]`)) + .join('\n'); + + return `Spec ${spec.ticketKey} v${spec.version} — ${spec.title} +Approved by ${spec.approvedBy ?? 'unknown'} + +=== ACCEPTANCE CRITERIA (what the change must satisfy) === +${criteria || '(none recorded)'} + +=== DESIGN (what was agreed; a deviation is a finding) === +${design || '(none recorded)'}`; +} + +function reviewPrompt(brief: string, diff: string): string { + return `${brief} + +=== THE DIFF === +${diff} + +Review it. Open the files you need.`; +} diff --git a/apps/api/src/agents/spec.agent.ts b/apps/api/src/agents/spec.agent.ts index 8ce40ac..783b6c2 100644 --- a/apps/api/src/agents/spec.agent.ts +++ b/apps/api/src/agents/spec.agent.ts @@ -13,8 +13,7 @@ import { type SpecDraftResult, type EarsCriterion, type CitationCoverage, - type Precedent, -} from '@specd/shared'; + type Precedent, STATION_EFFORT, type Effort } from '@specd/shared'; import { KnowledgeService } from '../knowledge/knowledge.service.js'; import { ModelRouter } from './model.router.js'; import type { RunHandle } from '../runs/runs.service.js'; @@ -246,6 +245,8 @@ export class SpecAgent { apiKey: string | null; model: ModelId; mode: AiMode; + /** Defaults to the spec station's level. */ + effort?: Effort; run: RunHandle; /** Review discussion, when re-drafting. v2 consumes the threads (§8). */ revisionNotes?: string[]; @@ -266,7 +267,7 @@ export class SpecAgent { apiKey: input.apiKey ?? '', model: input.model, maxTokens: 32_000, - effort: 'high', + effort: input.effort ?? STATION_EFFORT.spec, system: prepared.system, user: prepared.user, schema: prepared.schema, diff --git a/apps/api/src/app.module.ts b/apps/api/src/app.module.ts index 15f0f79..497c6e7 100644 --- a/apps/api/src/app.module.ts +++ b/apps/api/src/app.module.ts @@ -40,6 +40,7 @@ import { ModelRouter } from './agents/model.router.js'; import { OnboardingAgent } from './agents/onboarding.agent.js'; import { SpecAgent } from './agents/spec.agent.js'; import { BuildAgent } from './agents/build.agent.js'; +import { ReviewAgent } from './agents/review.agent.js'; import { WorkspaceService } from './vcs/workspace.js'; import { PipelineService } from './agents/pipeline.service.js'; import { IndexQueueService } from './agents/index-queue.service.js'; @@ -96,6 +97,7 @@ import { RunnersController } from './runners/runners.controller.js'; OnboardingAgent, SpecAgent, BuildAgent, + ReviewAgent, WorkspaceService, PipelineService, IndexQueueService, diff --git a/apps/api/src/projects/projects.controller.ts b/apps/api/src/projects/projects.controller.ts index cceafd9..c29b35d 100644 --- a/apps/api/src/projects/projects.controller.ts +++ b/apps/api/src/projects/projects.controller.ts @@ -21,7 +21,7 @@ import { Min, MinLength, } from 'class-validator'; -import { MODEL_IDS, MODELS } from '@specd/shared'; +import { EFFORT_LEVELS, MODEL_IDS, MODELS } from '@specd/shared'; import { CurrentUser } from '../auth/current-user.decorator.js'; import type { TokenClaims } from '../auth/auth.service.js'; import { ProjectsService } from './projects.service.js'; @@ -51,6 +51,12 @@ class UpdateProjectDto { @IsOptional() @IsString() @MaxLength(500) description?: string; @IsOptional() @IsInt() @Min(0) spendCapCents?: number; @IsOptional() @IsIn(MODEL_IDS) defaultModel?: string; + /** + * `null` clears the override and puts every station back on its own + * default — which is why this is nullable rather than merely optional: an + * omitted field means "unchanged", and there has to be a way to say "none". + */ + @IsOptional() @IsIn([...EFFORT_LEVELS, null]) effort?: string | null; @IsOptional() @IsBoolean() agentsPaused?: boolean; /** One-way: true marks setup finished; false is ignored. */ @IsOptional() @IsBoolean() setupComplete?: boolean; diff --git a/apps/api/src/projects/projects.service.ts b/apps/api/src/projects/projects.service.ts index 8e77c19..541ad96 100644 --- a/apps/api/src/projects/projects.service.ts +++ b/apps/api/src/projects/projects.service.ts @@ -10,7 +10,7 @@ import { specs, type Db, } from '@specd/db'; -import { isModelId, slugify, type ProjectSummary, type VcsProvider } from '@specd/shared'; +import { isModelId, slugify, type ProjectSummary, type VcsProvider, isEffort } from '@specd/shared'; import { RunsInFlight } from '../common/errors.js'; import { DB } from '../db/db.module.js'; @@ -103,6 +103,8 @@ export class ProjectsService { description?: string | null; spendCapCents?: number; defaultModel?: string; + /** null clears the override; undefined leaves it alone. */ + effort?: string | null; agentsPaused?: boolean; setupComplete?: boolean; }, @@ -120,6 +122,11 @@ export class ProjectsService { if (patch.defaultModel !== undefined && isModelId(patch.defaultModel)) { values.defaultModel = patch.defaultModel; } + // `null` is a value here, not an absence: it is how a project goes back to + // per-station defaults after having had an override. + if (patch.effort !== undefined && (patch.effort === null || isEffort(patch.effort))) { + values.effort = patch.effort; + } if (Object.keys(values).length === 0) return this.byId(projectId); const [row] = await this.db diff --git a/apps/api/src/runners/runner-jobs.service.ts b/apps/api/src/runners/runner-jobs.service.ts index cfdb2e5..76eb160 100644 --- a/apps/api/src/runners/runner-jobs.service.ts +++ b/apps/api/src/runners/runner-jobs.service.ts @@ -4,6 +4,7 @@ import { agentRuns, type DbHandle, type Repository, type Runner } from '@specd/d import type { DetectedStack } from '@specd/templates'; import type { CitationCoverage, + Effort, ModelId, RetrievedChunk, SpecContent, @@ -41,7 +42,7 @@ export interface SpecJobPayload { schema: Record<string, unknown>; model: ModelId; maxTokens: number; - effort: 'low' | 'medium' | 'high' | 'xhigh' | 'max'; + effort: Effort; // Carried through to finalize() once the runner reports a result — the // runner itself never needs to understand any of this, only echo the // model's reply back. @@ -82,12 +83,23 @@ export interface OnboardJobPayload { export interface BuildJobPayload { kind: 'build'; model: ModelId; + /** So a dispatched build works as hard as one that ran here. */ + effort: Effort; system: string; branch: string; asBuiltPath: string; asBuiltCommitMessage: string; verifyCommand: string | null; tasks: PreparedBuildTask[]; + /** + * The review pass, rendered here and executed there. + * + * The prompt is authored on this side for the same reason every other prompt + * is (`prepare()` has no side effects, so both paths ask the model the same + * thing), but it can only *run* on the runner: the diff it reviews lives in + * the runner's workspace, which is gone by the time the report arrives. + */ + review: { system: string; brief: string; schema: Record<string, unknown> }; remote: { cloneUrl: string; baseBranch: string }; ticketKey: string; // Carried through to finalize(), which opens the review surface once the diff --git a/apps/api/src/vcs/local-review.test.ts b/apps/api/src/vcs/local-review.test.ts index 21f3886..373b60a 100644 --- a/apps/api/src/vcs/local-review.test.ts +++ b/apps/api/src/vcs/local-review.test.ts @@ -6,6 +6,7 @@ import { simpleGit } from 'simple-git'; import { afterAll, afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { describeForPushOption, + run, detectHost, instanceUrlFromRemote, openLocalReview, @@ -373,3 +374,54 @@ describe('the push-option route against a remote that does not take them', () => expect(result.note).toMatch(/did not report a merge request from the push/); }); }); + +describe('a child that exits before it reads stdin', () => { + /** + * The CI failure this pins. `openLocalReview` shells out to `gh`, `glab` and + * `git`, and in the common failure cases — no such binary, a rejected push — + * the child is gone before the prompt is written. Node reports that write as + * an `error` on the stdin stream, and unhandled it is an *uncaught + * exception*: every test in the run passed and the run still failed. + * + * Made deterministic by writing far more than any pipe buffer to a command + * that exits at once, so the write always outlives the process rather than + * usually winning the race. + */ + const withoutUncaught = async (fn: () => Promise<unknown>) => { + const uncaught: unknown[] = []; + const onUncaught = (err: unknown) => uncaught.push(err); + process.on('uncaughtException', onUncaught); + try { + await fn(); + await new Promise((r) => setTimeout(r, 50)); // let an async EPIPE surface + return uncaught; + } finally { + process.off('uncaughtException', onUncaught); + } + }; + + it('answers instead of raising, when the pipe is already closed', async () => { + let result: { code: number | null } | undefined; + const uncaught = await withoutUncaught(async () => { + result = await run('git', ['--version'], { cwd: process.cwd(), stdin: 'x'.repeat(8_000_000) }); + }); + + expect(uncaught).toEqual([]); + // The call still reports the child's outcome — a pipe nobody read is not + // a failure of the command. + expect(result?.code).toBe(0); + }); + + it('does not raise when the binary does not exist at all', async () => { + // The CI shape: no `gh` on the runner, so the spawn fails and there is no + // process to receive the write. + const uncaught = await withoutUncaught(async () => { + const out = await run('specd-no-such-binary', ['--version'], { + cwd: process.cwd(), + stdin: 'x'.repeat(8_000_000), + }); + expect(out.code).toBeNull(); + }); + expect(uncaught).toEqual([]); + }); +}); diff --git a/apps/api/src/vcs/local-review.ts b/apps/api/src/vcs/local-review.ts index 986b74c..3d3348f 100644 --- a/apps/api/src/vcs/local-review.ts +++ b/apps/api/src/vcs/local-review.ts @@ -527,8 +527,12 @@ function short(err: unknown): string { * No shell on purpose: `title` and `body` are repository- and project-derived * strings, and the moment they reach a shell they are code. As argv entries * they are data. + * + * Exported for one test: the stdin race below is a property of this function + * and not of any caller, and reproducing it through `openLocalReview` means + * depending on which binaries the machine happens to have. */ -function run( +export function run( bin: string, args: string[], opts: { cwd: string; stdin?: string; timeoutMs?: number }, @@ -575,6 +579,16 @@ function run( child.on('error', () => done(null)); child.on('close', (code) => done(code)); + // `gh` and `git` both exit before reading stdin in the common failure + // cases — a missing binary, a rejected push — leaving the pipe with nobody + // on the other end. Node reports that write as an `error` on the stream, + // and unhandled it is an uncaught exception rather than a failed call. + // Same guard the runner arrived at (`apps/runner/src/claude.ts`); the exit + // code and stderr already carry the outcome. + child.stdin.on('error', (err: NodeJS.ErrnoException) => { + if (err.code === 'EPIPE' || err.code === 'ERR_STREAM_DESTROYED') return; + stderr += `\n[specd] could not write to ${bin}'s stdin: ${err.message}`; + }); child.stdin.end(opts.stdin ?? ''); }); } diff --git a/apps/api/src/vcs/workspace.ts b/apps/api/src/vcs/workspace.ts index dcd0d81..4c8a1c3 100644 --- a/apps/api/src/vcs/workspace.ts +++ b/apps/api/src/vcs/workspace.ts @@ -446,6 +446,23 @@ export class WorkspaceService { return (await git.revparse(['HEAD'])).trim(); } + /** + * The change as a reviewer will see it, capped. + * + * Capped because a build that touched a lockfile or a generated bundle would + * otherwise spend the whole review budget on diff nobody reads. The cap is + * announced in the text rather than silently applied — a review of half a + * change that claims to be a review of the change is worse than no review. + */ + async diff(dir: string, baseBranch: string, limit = 120_000): Promise<string> { + const git = simpleGit({ baseDir: dir }); + const out = await git + .raw(['diff', `${baseBranch}...HEAD`, '--', '.', ':(exclude)*.lock', ':(exclude)*-lock.json']) + .catch(() => ''); + if (out.length <= limit) return out; + return `${out.slice(0, limit)}\n\n[diff truncated at ${limit} characters — ${out.length - limit} more not shown]`; + } + async commitCount(dir: string, baseBranch: string): Promise<number> { const git = simpleGit({ baseDir: dir }); try { diff --git a/apps/runner/src/build.ts b/apps/runner/src/build.ts index 1bc489a..64f6e4d 100644 --- a/apps/runner/src/build.ts +++ b/apps/runner/src/build.ts @@ -2,8 +2,8 @@ import { spawn } from 'node:child_process'; import { mkdir, mkdtemp, rm, writeFile } from 'node:fs/promises'; import { tmpdir } from 'node:os'; import { dirname, join } from 'node:path'; -import { renderAsBuiltMarkdown, type ModelId, type SpecContent, type TokenUsage } from '@specd/shared'; -import { callClaudeCode } from './claude.js'; +import { renderAsBuiltMarkdown, type ModelId, type SpecContent, type TokenUsage, type Effort } from '@specd/shared'; +import { callClaudeCode, reviewWithClaude } from './claude.js'; import { canReachRemote, changedFiles, @@ -13,6 +13,7 @@ import { pushBranch, shallowClone, startBranch, + diffAgainst, } from './git.js'; /** @@ -34,10 +35,13 @@ export interface BuildJob { model: ModelId; system: string; branch: string; + effort?: Effort; asBuiltPath: string; asBuiltCommitMessage: string; verifyCommand: string | null; tasks: { id: string; title: string; prompt: string; commitMessage: string }[]; + /** Rendered by the API; run here, because the diff is here. */ + review?: { system: string; brief: string; schema: Record<string, unknown> }; remote: { cloneUrl: string; baseBranch: string }; ticketKey: string; ctx: { @@ -61,6 +65,8 @@ export interface BuildOutcome { commits: number; verifyPassed: boolean | null; verifyOutput: string | null; + /** Findings from the read-only pass, or null when it could not run. */ + review: unknown; }; usage: TokenUsage; model: ModelId; @@ -110,6 +116,7 @@ export async function runBuildJob(job: BuildJob, narrate: Narrate): Promise<Buil const result = await callClaudeCode({ model: job.model, + effort: job.effort, system: job.system, user: task.prompt, workspaceDir: dir, @@ -189,8 +196,31 @@ export async function runBuildJob(job: BuildJob, narrate: Narrate): Promise<Buil await narrate(`pushing ${job.branch} as this machine's git user`); await pushBranch(dir, job.remote.cloneUrl, job.branch); + // Station 05b, on this machine — see `reviewWithClaude`. Advisory: a + // review that cannot run leaves the report without one rather than + // failing a build whose commits already exist. + let review: unknown = null; + if (job.review) { + const diff = await diffAgainst(dir, job.remote.baseBranch); + if (diff.trim()) { + await narrate('reviewing the diff'); + const outcome = await reviewWithClaude<unknown>({ + model: job.model, + effort: job.effort, + system: job.review.system, + user: `${job.review.brief}\n\n=== THE DIFF ===\n${diff}\n\nReview it. Open the files you need.`, + schema: job.review.schema, + workspaceDir: dir, + }); + review = outcome.parsed; + usage.inputTokens += outcome.usage.inputTokens; + usage.outputTokens += outcome.usage.outputTokens; + await narrate(review ? 'review complete' : 'the review pass did not answer — reporting without it'); + } + } + return { - report: { tasksAttempted, tasksCommitted, commits, verifyPassed, verifyOutput }, + report: { tasksAttempted, tasksCommitted, commits, verifyPassed, verifyOutput, review }, usage, model: observedModel ?? job.model, }; diff --git a/apps/runner/src/claude.ts b/apps/runner/src/claude.ts index 844368c..56c9304 100644 --- a/apps/runner/src/claude.ts +++ b/apps/runner/src/claude.ts @@ -9,6 +9,8 @@ import { type ClaudeCodeEnvelope, type ModelId, type TokenUsage, + STATION_EFFORT, + type Effort, } from '@specd/shared'; /** @@ -20,6 +22,8 @@ import { */ export interface ClaudeCallOptions { + /** Sent by the API with the job; falls back to the station default. */ + effort?: Effort; model: ModelId; system: string; user: string; @@ -84,6 +88,8 @@ export async function callClaudeCode(opts: { system: string; user: string; workspaceDir: string; + /** Sent by the API with the job, so a dispatched build is not quietly cheaper. */ + effort?: Effort; timeoutMs?: number; }): Promise<{ text: string; usage: TokenUsage; model: ModelId | null }> { const args = [ @@ -92,6 +98,8 @@ export async function callClaudeCode(opts: { 'json', '--model', opts.model, + '--effort', + opts.effort ?? STATION_EFFORT.build, '--append-system-prompt', opts.system, '--permission-mode', @@ -127,6 +135,8 @@ async function invoke( 'json', '--model', opts.model, + '--effort', + opts.effort ?? STATION_EFFORT.spec, '--system-prompt', system, '--exclude-dynamic-system-prompt-sections', @@ -292,3 +302,71 @@ function stripClaudeSessionVars(env: NodeJS.ProcessEnv): NodeJS.ProcessEnv { } return copy; } + + +/** + * The read-only review pass, mirroring `ClaudeCodeProvider.review()`. + * + * It exists here because the diff it reads is on this machine — a dispatched + * build whose review ran on the API host would be reviewing a workspace that + * does not exist there. Best-effort: a review that cannot run costs an + * opinion, never the build. + */ +export async function reviewWithClaude<T>(opts: { + model: ModelId; + effort?: Effort; + system: string; + user: string; + schema: Record<string, unknown>; + workspaceDir: string; + timeoutMs?: number; +}): Promise<{ parsed: T | null; usage: TokenUsage }> { + const args = [ + '--print', + '--output-format', + 'json', + '--model', + opts.model, + '--effort', + opts.effort ?? STATION_EFFORT.review, + '--append-system-prompt', + `${opts.system}\n${schemaInstruction(opts.schema)}`, + '--permission-mode', + 'plan', + '--allowedTools', + 'Read', + 'Glob', + 'Grep', + '--disallowed-tools', + 'Write', + 'Edit', + 'Bash', + 'WebFetch', + 'WebSearch', + 'Task', + 'NotebookEdit', + ]; + + const empty: TokenUsage = { + inputTokens: 0, + outputTokens: 0, + cacheReadInputTokens: 0, + cacheCreationInputTokens: 0, + }; + + try { + const result = await exec(args, opts.user, opts.timeoutMs ?? 600_000, opts.workspaceDir); + if (result.timedOut || result.code !== 0) return { parsed: null, usage: empty }; + + const envelope = JSON.parse(result.stdout) as ClaudeCodeEnvelope; + const usage: TokenUsage = { + inputTokens: envelope.usage?.input_tokens ?? 0, + outputTokens: envelope.usage?.output_tokens ?? 0, + cacheReadInputTokens: envelope.usage?.cache_read_input_tokens ?? 0, + cacheCreationInputTokens: envelope.usage?.cache_creation_input_tokens ?? 0, + }; + return { parsed: parseAgainstSchema<T>(envelope.result ?? '', opts.schema), usage }; + } catch { + return { parsed: null, usage: empty }; + } +} diff --git a/apps/runner/src/git.ts b/apps/runner/src/git.ts index 3171815..1a62179 100644 --- a/apps/runner/src/git.ts +++ b/apps/runner/src/git.ts @@ -171,3 +171,20 @@ export async function pushBranch(dir: string, cloneUrl: string, branch: string): 600_000, ); } + + +/** + * The change as a reviewer will see it, capped — mirrors + * `WorkspaceService.diff()` on the API side, for the same reason and with the + * same cap: a generated bundle should not eat the review budget, and a review + * of half a change must not claim to be a review of the change. + */ +export async function diffAgainst(dir: string, baseBranch: string, limit = 120_000): Promise<string> { + const out = await gitOrThrow( + ['diff', `${baseBranch}...HEAD`, '--', '.', ':(exclude)*.lock', ':(exclude)*-lock.json'], + dir, + 'Could not read the diff', + ).catch(() => ''); + if (out.length <= limit) return out; + return `${out.slice(0, limit)}\n\n[diff truncated at ${limit} characters — ${out.length - limit} more not shown]`; +} diff --git a/apps/web/components/SettingsView.tsx b/apps/web/components/SettingsView.tsx index 09f04b7..556bff3 100644 --- a/apps/web/components/SettingsView.tsx +++ b/apps/web/components/SettingsView.tsx @@ -42,6 +42,7 @@ export function SettingsView({ const [description, setDescription] = useState(project.description ?? ''); const [cap, setCap] = useState((project.spendCapCents / 100).toFixed(0)); const [model, setModel] = useState(project.defaultModel); + const [effort, setEffort] = useState<string>(project.effort ?? ''); // Seeded from the server, and only flipped once the server confirms — a // kill switch whose label can disagree with reality is worse than none. const [paused, setPaused] = useState(project.agentsPaused); @@ -329,6 +330,35 @@ export function SettingsView({ </div> </div> + <div className="card"> + <h5>Effort</h5> + <p className="sub"> + How hard the model works per call — the intelligence, latency and cost dial. Each + station has its own default: builds and reviews run at <code>xhigh</code>, spec and + grounding at <code>high</code>, indexing at <code>low</code>. Set this only to move the + whole project off those. + </p> + <div className="inline"> + <select + value={effort} + disabled={saveBusy === 'effort'} + onChange={(e) => { + setEffort(e.target.value); + save('effort', { effort: e.target.value === '' ? null : e.target.value }); + }} + > + <option value="">Per station (recommended)</option> + <option value="low">low · cheapest, for routine work</option> + <option value="medium">medium</option> + <option value="high">high</option> + <option value="xhigh">xhigh · what coding agents want</option> + <option value="max">max · correctness over cost</option> + </select> + {saveBusy === 'effort' && <span className="spinner" />} + {saved === 'effort' && <span className="ok">saved ✓</span>} + </div> + </div> + <div className="card"> <h5>Spend cap</h5> <p className="sub"> diff --git a/apps/web/lib/docs/guides.ts b/apps/web/lib/docs/guides.ts index c19b115..8ee55f1 100644 --- a/apps/web/lib/docs/guides.ts +++ b/apps/web/lib/docs/guides.ts @@ -395,6 +395,7 @@ specd spec status CRM-131 # exit 0 approved · exit 3 not approved`, 'A branch named `spec/<ID>-<slug>`, cut fresh from your default branch every run — which is also how the merge webhook matches the delivery back to the spec.', 'One commit per task, in the order the spec listed them.', 'A pull request (or merge request) titled `[<ID>] - <Title>`, for you to review and merge. specd never merges.', + 'A **review pass** over the diff, in the PR body: what it does, and what a reviewer should look at, each finding tied to the acceptance criterion or design claim it bears on. Advisory — it never blocks the build, and nothing it raises has been acted on.', 'The as-built spec, filed into `knowledge/specs/` by the last task — and appended by specd itself if the model omitted it.', ], }, diff --git a/apps/web/lib/types.ts b/apps/web/lib/types.ts index 11a66e2..9fbb88a 100644 --- a/apps/web/lib/types.ts +++ b/apps/web/lib/types.ts @@ -12,6 +12,8 @@ export interface ProjectSummary { spendCapCents: number; knowledgeHealth: number; defaultModel: string; + /** null = every station keeps its own default. */ + effort: string | null; agentsPaused: boolean; setupComplete: boolean; } diff --git a/knowledge/README.md b/knowledge/README.md index 1663c58..8a688bb 100644 --- a/knowledge/README.md +++ b/knowledge/README.md @@ -46,6 +46,7 @@ everything else is a consequence of it. - [0018 — the working agreements ship as a plugin](decisions/0018-working-agreements-ship-as-a-plugin.md) - [0019 — the documentation is data, rendered twice](decisions/0019-the-docs-are-data-rendered-twice.md) - [0020 — local mode borrows the machine's host CLI to open a real review](decisions/0020-local-mode-borrows-the-host-cli.md) +- [0021 — effort is per-station, and builds review themselves](decisions/0021-effort-is-per-station-and-builds-review-themselves.md) New decisions join this list in the PR that adds them — the map rides the change like everything else. diff --git a/knowledge/conventions.md b/knowledge/conventions.md index ca98299..d6c25d8 100644 --- a/knowledge/conventions.md +++ b/knowledge/conventions.md @@ -68,6 +68,25 @@ cross-package imports go through the `@specd/*` workspace names, which is why headless loop (`apps/api/src/e2e-loop.ts`). - Postgres is on **5433** on the host (non-default, to avoid clashing with local installs). It is the only runtime dependency — Redis was removed with the unused queue (`decisions/0008-remove-unused-queue.md`). +## Asking a model to do something + +Every model call names its station's effort — never a literal level at the call +site. `effortFor(station, project.effort)` resolves it; `STATION_EFFORT` holds +the defaults. Both paths carry it: `output_config.effort` on the API and +`--effort` on the Claude Code CLI, including in the runner, so a dispatched job +is not quietly cheaper than a local one. + +The three CLI modes are distinct on purpose and should stay that way: + +| Mode | Tools | For | +| --- | --- | --- | +| `call()` | none | a text transform that answers in a schema | +| `code()` | Read/Write/Edit/Glob/Grep | the build station, which changes the repository | +| `review()` | Read/Glob/Grep, `--permission-mode plan` | reading a diff without being able to "fix" it | + +A reviewer that can edit is not a reviewer. `Write` and `Edit` are denied +rather than merely unused. + ## Talking to somebody else's server Every outbound HTTP call goes through `apps/api/src/common/http-failures.ts`. diff --git a/knowledge/decisions/0021-effort-is-per-station-and-builds-review-themselves.md b/knowledge/decisions/0021-effort-is-per-station-and-builds-review-themselves.md new file mode 100644 index 0000000..dee4dee --- /dev/null +++ b/knowledge/decisions/0021-effort-is-per-station-and-builds-review-themselves.md @@ -0,0 +1,78 @@ +# 0021 — Effort is a per-station setting, and a build reviews itself before it asks you to + +- **Status:** accepted +- **Date:** 2026-08-18 +- **Project:** specd + +## Context + +Two gaps, found by reading the agent layer rather than by a failure. + +**Effort was typed and then thrown away.** `ModelCallOptions.effort` accepted +all five levels the API takes and defaulted to `high`; three call sites passed +the literal `'high'`; nothing configured it. Worse, the *Claude Code* path +passed no effort at all — so in subscription mode, the setting specd thought it +had chosen did not reach the model. `claude --effort` exists and takes the same +five levels; specd simply never sent it. + +One value for every station is wrong in both directions at once. Anthropic's +guidance is that `xhigh` is the setting for coding and agentic work — it is +what Claude Code itself runs at — while cheap mechanical passes are where `low` +belongs. specd ran a build that writes code a human is about to merge at the +same level as an index run that summarises text. + +**Nothing read the diff.** The build station runs the repository's verify +command, which answers "do the tests pass" — a question the test suite already +answers. Nothing answered "is this the change we approved, and is it any good". +That reading was left entirely to the human opening the PR, cold. + +## Decision + +**Effort is per station, with a project override.** `STATION_EFFORT` in +`@specd/shared` sets `build`/`review` to `xhigh`, `spec`/`ground` to `high`, +and `index` to `low`. `effortFor(station, override)` resolves it. A project may +set `projects.effort` to move every station at once; NULL — the default — means +"no preference", not `low`. The value now travels to *both* model paths, +including `claude --effort`, and into the runner job payload so a dispatched +build is not quietly cheaper than a local one. + +**A review pass runs between verify and publish.** `ReviewAgent` reads the diff +with read-only tools and answers in a schema: a verdict, a one-line summary, +and findings carrying `path:line`, a severity, and the acceptance criterion or +design claim each bears on. The findings render into the pull-request body, +above the acceptance criteria — the new reading sits above what it is measured +against. + +Four properties this is built around: + +1. **Advisory, and it says so in the text.** A finding does not fail a build. + This is the line citation drift already sits on: an unrelated opinion must + not be able to stop an approved spec from shipping. Making findings + refusable is a house-rule decision (2.4), not a default. +2. **Read-only by denial, not by omission.** `Write` and `Edit` are in + `--disallowed-tools` and the pass runs under `--permission-mode plan`, so an + over-helpful reviewer cannot quietly amend the branch it was asked to + assess. +3. **An empty findings list is a real answer**, and a clean pass is reported + rather than omitted — silence would be indistinguishable from a pass that + never ran, which is the same failure as reporting a green verify for tests + that did not execute. +4. **It runs where the workspace is.** The prompt is authored on the API side + (`prepare()` stays side-effect-free, so both paths ask the same thing) but a + dispatched build reviews on the runner, because the diff is on the runner's + disk and gone by the time the report arrives. + +## Consequences + +- Builds cost more per run and should be better for it; indexing costs less. + Both were previously wrong in the same direction because they shared a dial. +- A pull request now carries a reading of its own diff. The risk is that a + padded findings list trains reviewers to skip the section, which is why the + prompt says an empty list is a real answer and the rendering states plainly + that nothing was acted on. +- The review adds a model call to every build. It is skipped when the diff is + empty, and every failure in it is soft — the commits exist and verify has + already spoken, so a review that cannot run costs an opinion, not the build. +- `effort` is one dial for the whole project rather than per station. Someone + who wants grounding cheap and builds expensive cannot express that yet; + `effortFor` is the seam where that would go. diff --git a/knowledge/glossary.md b/knowledge/glossary.md index 09faee1..8743b18 100644 --- a/knowledge/glossary.md +++ b/knowledge/glossary.md @@ -21,6 +21,8 @@ Domain terms mined from code and docs. | **approver / actor** | The named human recorded on an approval; a DB CHECK constraint rejects an approved row without one. | | **EARS** | Easy Approach to Requirements Syntax — acceptance criteria rendered as "WHEN «trigger» THE SYSTEM SHALL «response»" with keywords `WHEN / WHILE / IF / WHERE` (`EARS_KEYWORDS`, `packages/shared/src/spec.ts`). | | **rate card** | Per-model token pricing table used to meter run cost in integer EUR cents. | +| **effort** | How hard the model works per call (`output_config.effort` on the API, `--effort` on the CLI): `low`…`max`. Per station by default — builds and reviews at `xhigh`, spec and grounding at `high`, indexing at `low` (`STATION_EFFORT` in `@specd/shared`); `projects.effort` moves all of them, and NULL means "no preference", not `low`. | +| **review pass** | Station 05b: a read-only pass over the build's own diff, between verify and publish, whose findings land in the PR body. Advisory — it never fails a build (`knowledge/decisions/0021-…`). | | **cap** | Spend limit checked before a run starts rather than after it overspends. | | **subscription_runner** | AI-connection mode (`connections.settings.mode`) that drives a locally logged-in Claude Code CLI instead of an API key. Executed on a paired self-hosted runner (`apps/runner`), so hosted specd can use it too — the credential never reaches the platform. The `SPECD_AI_MODE` env var sets it only for the headless loop. | | **loop (`pnpm --filter @specd/api loop`)** | Headless end-to-end exercise of every station over the real HTTP API, reporting pass/skip per station. | diff --git a/packages/db/migrations/0019_project_effort.sql b/packages/db/migrations/0019_project_effort.sql new file mode 100644 index 0000000..ec869a3 --- /dev/null +++ b/packages/db/migrations/0019_project_effort.sql @@ -0,0 +1,11 @@ +-- A project's effort override. +-- +-- NULL is "no preference", never "low": every station keeps its own default +-- (see STATION_EFFORT in @specd/shared) unless a project says otherwise, and a +-- project that never expressed one must not be quietly moved off those. +ALTER TABLE projects + ADD COLUMN effort text; + +ALTER TABLE projects + ADD CONSTRAINT projects_effort_is_a_level + CHECK (effort IS NULL OR effort IN ('low', 'medium', 'high', 'xhigh', 'max')); diff --git a/packages/db/src/schema.ts b/packages/db/src/schema.ts index fab0d9f..2acef54 100644 --- a/packages/db/src/schema.ts +++ b/packages/db/src/schema.ts @@ -56,6 +56,13 @@ export const projects = pgTable( /** Spend caps are default-on (§12). Enforced pre-run, per project, per month. */ spendCapCents: integer('spend_cap_cents').notNull().default(10_000), defaultModel: text('default_model').notNull().default('claude-opus-5'), + /** + * Effort override for every station, or NULL for "no preference" — which + * is not the same as `low`. Stations have different defaults for good + * reasons (`STATION_EFFORT` in `@specd/shared`); this dial says the whole + * project is cost-sensitive or worth the spend, and moves all of them. + */ + effort: text('effort'), /** Kill switch per project (§12). */ agentsPaused: boolean('agents_paused').notNull().default(false), /** diff --git a/packages/shared/src/effort.test.ts b/packages/shared/src/effort.test.ts new file mode 100644 index 0000000..1fa551d --- /dev/null +++ b/packages/shared/src/effort.test.ts @@ -0,0 +1,55 @@ +import { describe, expect, it } from 'vitest'; +import { EFFORT_LEVELS, STATION_EFFORT, effortFor, isEffort } from './effort.js'; + +describe('station defaults', () => { + it('does not give every station the same level', () => { + // The bug this replaced: `effort: 'high'` hardcoded at every call site, + // which is simultaneously too little for a build and too much for an + // index run. If these ever collapse to one value, that is back. + expect(new Set(Object.values(STATION_EFFORT)).size).toBeGreaterThan(1); + }); + + it('spends most where code is written and least where text is summarized', () => { + const rank = (level: string) => EFFORT_LEVELS.indexOf(level as never); + expect(rank(STATION_EFFORT.build)).toBeGreaterThan(rank(STATION_EFFORT.spec)); + expect(rank(STATION_EFFORT.index)).toBeLessThan(rank(STATION_EFFORT.spec)); + }); + + it('reviews as hard as it builds', () => { + // Reading a diff for what is wrong with it is not the cheap half of the + // job — a review run cheaply is a review nobody should act on. + expect(STATION_EFFORT.review).toBe(STATION_EFFORT.build); + }); +}); + +describe('effortFor', () => { + it('uses the station default when a project has no preference', () => { + expect(effortFor('build')).toBe(STATION_EFFORT.build); + expect(effortFor('index', null)).toBe(STATION_EFFORT.index); + }); + + it('lets an override move every station', () => { + expect(effortFor('build', 'low')).toBe('low'); + expect(effortFor('index', 'max')).toBe('max'); + }); + + it('treats null as absent, not as a level', () => { + // The column is nullable and NULL means "no preference". Reading it as a + // falsy level would silently move a project that never chose one. + expect(effortFor('spec', null)).toBe(STATION_EFFORT.spec); + }); +}); + +describe('isEffort', () => { + it('accepts every level the API and the CLI both take', () => { + for (const level of EFFORT_LEVELS) expect(isEffort(level)).toBe(true); + expect(EFFORT_LEVELS).toEqual(['low', 'medium', 'high', 'xhigh', 'max']); + }); + + it('rejects everything else, including the empty string the form sends', () => { + expect(isEffort('')).toBe(false); + expect(isEffort('highest')).toBe(false); + expect(isEffort(null)).toBe(false); + expect(isEffort(3)).toBe(false); + }); +}); diff --git a/packages/shared/src/effort.ts b/packages/shared/src/effort.ts new file mode 100644 index 0000000..004c438 --- /dev/null +++ b/packages/shared/src/effort.ts @@ -0,0 +1,52 @@ +/** + * How hard a model works on one call. + * + * `effort` is the intelligence ↔ latency ↔ cost dial (`output_config.effort` + * on the API, `--effort` on the Claude Code CLI). It was typed here from the + * start and then hardcoded to `high` at every call site, which left the two + * stations that want opposite settings sharing one — a build wants the model + * thinking hard about code it is about to commit, and an index run wants the + * cheapest pass that still parses. + */ +export const EFFORT_LEVELS = ['low', 'medium', 'high', 'xhigh', 'max'] as const; +export type Effort = (typeof EFFORT_LEVELS)[number]; + +export function isEffort(value: unknown): value is Effort { + return typeof value === 'string' && (EFFORT_LEVELS as readonly string[]).includes(value); +} + +/** The stations that call a model. Indexing is here because it is the cheap one. */ +export type EffortStation = 'ground' | 'spec' | 'build' | 'review' | 'index'; + +/** + * What each station gets when a project has expressed no preference. + * + * These are not one value repeated. Anthropic's own guidance is that `xhigh` + * is the setting for coding and agentic work — it is what Claude Code itself + * runs at — while cheap mechanical passes are the place to spend `low`. specd + * ran every station at `high`, which was simultaneously too little for the + * build station and too much for indexing. + */ +export const STATION_EFFORT: Record<EffortStation, Effort> = { + // Writes code that a human is about to review and merge. + build: 'xhigh', + // Reads a diff it did not write, looking for what is wrong with it. + review: 'xhigh', + // Judgement over retrieved evidence, but a single structured answer. + spec: 'high', + ground: 'high', + // Summarising and embedding. Cheapest pass that still parses. + index: 'low', +}; + +/** + * The effort one station runs at, given a project's override. + * + * A project override applies to every station rather than replacing the + * per-station shape: someone who sets it is expressing "this project is + * cost-sensitive" or "this project is worth the spend", not re-deciding the + * relative cost of grounding versus building. + */ +export function effortFor(station: EffortStation, override?: Effort | null): Effort { + return override ?? STATION_EFFORT[station]; +} diff --git a/packages/shared/src/index.ts b/packages/shared/src/index.ts index 31fe9f0..b4a57ba 100644 --- a/packages/shared/src/index.ts +++ b/packages/shared/src/index.ts @@ -1,4 +1,5 @@ export * from './citation.js'; +export * from './effort.js'; export * from './claude-code-parse.js'; export * from './lifecycle.js'; export * from './models.js';