diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 0fa80e19e87..b6d1da5bcf2 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -761,17 +761,24 @@ describe('composeReview — presubmit permission gates certification even when n }); describe('composeReviewCommand handler (the CLI glue)', () => { - it('reads --input and writes the result JSON to --out', () => { + it('reads --input, counts the drafted comments, and writes the result JSON to --out', () => { const dir = mkdtempSync(join(tmpdir(), 'compose-review-test-')); const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); const outPath = join(dir, 'nested', 'composed.json'); + writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8'); + // The count comes from the drafted comments, not from a number in the + // state JSON — one Suggestion drafted, one Suggestion composed. writeFileSync( - inputPath, - JSON.stringify({ suggestionsInline: 1, modelId: MODEL }), + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 3, body: '**[Suggestion]** prefer x over y' }, + ]), 'utf8', ); (composeReviewCommand.handler as (argv: unknown) => void)({ input: inputPath, + comments: commentsPath, out: outPath, }); const written = JSON.parse( @@ -782,6 +789,171 @@ describe('composeReviewCommand handler (the CLI glue)', () => { expect(written.body.endsWith(FOOTER)).toBe(true); }); + it('a drafted inline Critical reaches the verdict line — the report-only hole', () => { + // The dogfooded failure this boundary exists for: a report-only run (no + // submit, so nothing downstream recounts) moved its one Critical from + // `bodyCriticals` to an inline comment, dropped the count on the way, and + // the verdict line read Approve over a blocker the same report listed. + // With the counts derived from the drafted comments, that finding cannot + // fall out of the computation. + const dir = mkdtempSync(join(tmpdir(), 'compose-inline-crit-')); + try { + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + const outPath = join(dir, 'composed.json'); + writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8'); + writeFileSync( + commentsPath, + JSON.stringify([ + { + path: 'shellAstParser.ts', + line: 141, + body: '**[Critical]** the AST path omits %G[?GKFPST]', + }, + ]), + 'utf8', + ); + (composeReviewCommand.handler as (argv: unknown) => void)({ + input: inputPath, + comments: commentsPath, + out: outPath, + }); + const written = JSON.parse(readFileSync(outPath, 'utf8')) as { + event: string; + verdictLine: string; + }; + expect(written.event).toBe('REQUEST_CHANGES'); + expect(written.verdictLine).toContain('Request changes'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it('accepts the review-payload shape too — the same file submit takes', () => { + const dir = mkdtempSync(join(tmpdir(), 'compose-payload-shape-')); + try { + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'review.json'); + const outPath = join(dir, 'composed.json'); + writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8'); + writeFileSync( + commentsPath, + JSON.stringify({ + commit_id: 'abc', + comments: [{ path: 'a.ts', line: 1, body: '**[Critical]** boom' }], + }), + 'utf8', + ); + (composeReviewCommand.handler as (argv: unknown) => void)({ + input: inputPath, + comments: commentsPath, + out: outPath, + }); + expect( + (JSON.parse(readFileSync(outPath, 'utf8')) as { event: string }).event, + ).toBe('REQUEST_CHANGES'); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it.each([ + ['criticalsInline', { criticalsInline: 1 }], + ['suggestionsInline', { suggestionsInline: 2 }], + ])( + 'refuses a state JSON carrying %s — counts are counted, not typed', + (_, extra) => { + const dir = mkdtempSync(join(tmpdir(), 'compose-typed-count-')); + try { + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + writeFileSync( + inputPath, + JSON.stringify({ modelId: MODEL, ...extra }), + 'utf8', + ); + writeFileSync(commentsPath, '[]', 'utf8'); + expect(() => + (composeReviewCommand.handler as (argv: unknown) => void)({ + input: inputPath, + comments: commentsPath, + }), + ).toThrow(/counted from the --comments file/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, + ); + + it('refuses a drafted comment with no severity marker — it would weigh nothing', () => { + const dir = mkdtempSync(join(tmpdir(), 'compose-unmarked-')); + try { + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8'); + writeFileSync( + commentsPath, + JSON.stringify([ + { path: 'a.ts', line: 1, body: '**[Critical]** real one' }, + { path: 'b.ts', line: 2, body: 'this blocker forgot its marker' }, + ]), + 'utf8', + ); + expect(() => + (composeReviewCommand.handler as (argv: unknown) => void)({ + input: inputPath, + comments: commentsPath, + }), + ).toThrow(/comments\[1\].*neither/s); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + + it.each([ + ['missing --comments', undefined, /--comments is required/], + [ + 'a comments path that does not resolve', + '/nonexistent/c.json', + /cannot read the comments file/, + ], + ])( + 'refuses %s — omission is the failure mode, not a default', + (_, commentsPath, pattern) => { + const dir = mkdtempSync(join(tmpdir(), 'compose-no-comments-')); + try { + const inputPath = join(dir, 'compose.json'); + writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8'); + expect(() => + (composeReviewCommand.handler as (argv: unknown) => void)({ + input: inputPath, + comments: commentsPath, + }), + ).toThrow(pattern as RegExp); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }, + ); + + it('refuses a comments file that is not an array (nor a payload with one)', () => { + const dir = mkdtempSync(join(tmpdir(), 'compose-bad-comments-')); + try { + const inputPath = join(dir, 'compose.json'); + const commentsPath = join(dir, 'comments.json'); + writeFileSync(inputPath, JSON.stringify({ modelId: MODEL }), 'utf8'); + writeFileSync(commentsPath, JSON.stringify({ criticals: 3 }), 'utf8'); + expect(() => + (composeReviewCommand.handler as (argv: unknown) => void)({ + input: inputPath, + comments: commentsPath, + }), + ).toThrow(/must be a JSON array of comment objects/); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }); + it('strips a model-supplied `env` — it cannot redirect the transcript lookup', () => { // The input is a JSON the model wrote. `env` decides where the harness // transcripts are read from; if the handler honoured it, a model could point @@ -865,19 +1037,20 @@ describe('composeReviewCommand handler (the CLI glue)', () => { writeFileSync( inputPath, JSON.stringify({ - criticalsInline: 0, - suggestionsInline: 0, planPath, env: { QWEN_CODE_PROJECT_DIR: forged, QWEN_CODE_SESSION_ID: 'S1' }, modelId: MODEL, }), ); + const commentsPath = join(dir, 'comments.json'); + writeFileSync(commentsPath, '[]', 'utf8'); const outPath = join(dir, 'out.json'); const prevProj = process.env['QWEN_CODE_PROJECT_DIR']; delete process.env['QWEN_CODE_PROJECT_DIR']; // real env cannot find transcripts try { (composeReviewCommand.handler as (argv: unknown) => void)({ input: inputPath, + comments: commentsPath, out: outPath, }); } finally { @@ -1046,12 +1219,12 @@ describe('coverage is recomputed, never accepted', () => { writeFileSync( input, JSON.stringify({ - criticalsInline: 0, - suggestionsInline: 0, planPath: p, modelId: MODEL, }), ); + const commentsPath = join(dir, 'comments.json'); + writeFileSync(commentsPath, '[]', 'utf8'); const prevDir = process.env['QWEN_CODE_PROJECT_DIR']; const prevSession = process.env['QWEN_CODE_SESSION_ID']; @@ -1062,6 +1235,7 @@ describe('coverage is recomputed, never accepted', () => { vi.mocked(writeStdoutLine).mockClear(); (composeReviewCommand.handler as (a: Record) => void)({ input, + comments: commentsPath, }); const stderr = vi diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index 17fabf589d3..c28eeb74b38 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -14,7 +14,8 @@ // one downstream branch not updated when an upstream rule gained a new // state. This module is the single source of truth; the skill gathers the // state, calls it, and uses `{event, body}` verbatim. 422 recovery is the -// same call with updated counts. +// same call with the updated `--comments` file — the counts are counted +// from it, never updated by hand. // // The model stays responsible for judgment (what is a Critical, is it // real); this owns only the bookkeeping that follows from the counts. @@ -29,13 +30,31 @@ import { TranscriptsUnavailableError, } from './lib/coverage.js'; import { shellQuotePath } from './lib/shell-quote.js'; +import { + CRITICAL_PREFIX, + SUGGESTION_PREFIX, + countInlineFindings, + unmarkedComments, + type DraftedComment, +} from './lib/inline-counts.js'; export type ReviewEvent = 'APPROVE' | 'REQUEST_CHANGES' | 'COMMENT'; export interface ComposeReviewInput { - /** Critical findings anchored as inline `comments` entries. Omitted = 0. */ + /** + * Critical findings anchored as inline `comments` entries. + * + * A seam for the two CLI boundaries and the tests — NEVER a field of the + * model-written state JSON. Both boundaries derive it from the drafted + * comments (`compose-review --comments`, `submit`'s payload) and refuse it + * when the JSON carries it: a count handed over beside the thing it counts + * is a count that can disagree with it, and a dogfooded report-only run — + * where nothing downstream recounts — moved its one Critical from + * `bodyCriticals` to an inline comment, lost the count on the way, and this + * function printed `Verdict: Approve` over a Critical the report listed. + */ criticalsInline?: number; - /** Suggestion findings anchored as inline `comments` entries. Omitted = 0. */ + /** Suggestion findings anchored inline. Same seam, same refusal. */ suggestionsInline?: number; /** * Critical descriptions whose only copy lives in the review body — the @@ -116,10 +135,8 @@ export interface ComposeReviewResult { remediation: string[]; } -const CRITICAL_MARKER = '**[Critical]**'; - function withMarker(line: string): string { - return line.startsWith(CRITICAL_MARKER) ? line : `${CRITICAL_MARKER} ${line}`; + return line.startsWith(CRITICAL_PREFIX) ? line : `${CRITICAL_PREFIX} ${line}`; } // The input arrives as JSON a model wrote, and the skill tells it to omit @@ -628,25 +645,96 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { interface ComposeReviewCliArgs { input: string | undefined; + comments: string; out: string | undefined; } +/** + * The drafted inline comments, read from the file Step 6 is told to pass. + * + * Accepts the bare array or the full review-payload shape (`{comments: […]}`), + * so the same file Step 7 submits can be handed over unchanged. Every entry + * must open with a severity marker: `countInlineFindings` weighs an unmarked + * body as nothing, and for a verdict computation "nothing" means a blocker + * written without its marker approves the review it should have blocked. + * Step 6 is where the draft is still cheap to fix, so it refuses here. + */ +function readDraftedComments(path: string): DraftedComment[] { + let raw: string; + try { + raw = readFileSync(path, 'utf8'); + } catch (err) { + throw new Error( + `compose-review: cannot read the comments file ${path}: ` + + `${(err as Error).message}. Pass the drafted inline comments — the ` + + `same array the review payload will carry — or a file containing [] ` + + `when nothing anchors inline.`, + ); + } + let parsed: unknown; + try { + parsed = JSON.parse(raw); + } catch (err) { + throw new Error( + `compose-review: the comments file ${path} is not JSON: ${(err as Error).message}`, + ); + } + const comments = Array.isArray(parsed) + ? parsed + : (parsed as { comments?: unknown })?.comments; + if (!Array.isArray(comments)) { + throw new Error( + `compose-review: the comments file ${path} must be a JSON array of ` + + `comment objects, or a review payload with a \`comments\` array.`, + ); + } + const unmarked = unmarkedComments(comments as DraftedComment[]); + if (unmarked.length > 0) { + throw new Error( + `compose-review: comments[${unmarked.join(', ')}] in ${path} open with ` + + `neither ${CRITICAL_PREFIX} nor ${SUGGESTION_PREFIX}. Every inline ` + + `comment is a finding and carries its severity first — an unmarked ` + + `body would be counted as neither, and a blocker that weighs nothing ` + + `approves the review it should block. Fix the draft, not the counts.`, + ); + } + return comments as DraftedComment[]; +} + export const composeReviewCommand: CommandModule = { command: 'compose-review', describe: - 'Compute the review event and body from the finding counts and run states (the Step 7 invariant, as code); reads the state JSON from --input or stdin', + 'Compute the review event and body from the drafted comments and run states (the Step 7 invariant, as code); reads the state JSON from --input or stdin', builder: (yargs) => yargs .option('input', { type: 'string', describe: 'Path to the state JSON (omit to read stdin)', }) + .option('comments', { + type: 'string', + demandOption: true, + describe: + 'Path to the drafted inline comments JSON (the review payload, or ' + + 'its bare comments array). The inline counts are counted from it, ' + + 'never typed — pass a file containing [] when nothing anchors inline.', + }) .option('out', { type: 'string', describe: 'Also write the {event, body} JSON to this path', }), handler: (argv) => { - const { input, out } = argv as unknown as ComposeReviewCliArgs; + const { input, comments, out } = argv as unknown as ComposeReviewCliArgs; + // yargs enforces --comments on the real command line; this covers every + // other way in (tests, programmatic calls) with the same sentence instead + // of an ENOENT on `undefined`. + if (!comments) { + throw new Error( + 'compose-review: --comments is required — the inline counts are ' + + 'counted from the drafted comments file, never typed. Pass a file ' + + 'containing [] when nothing anchors inline.', + ); + } const raw = readFileSync(input ?? 0, 'utf8'); // The input is a JSON the model wrote. `env` decides where the harness // transcripts are read from, and it must NOT come from that JSON: a model @@ -656,7 +744,28 @@ export const composeReviewCommand: CommandModule = { // always resolves the transcripts from the environment the CLI exported. const parsed = JSON.parse(raw) as ComposeReviewInput; delete parsed.env; - const result = composeReview(parsed); + // The inline counts are counted, not accepted — `submit` has refused them + // since the count-beside-the-comments bug, and this boundary refusing them + // too is what makes the Step 6 line and the posted verdict the same + // computation on the same source. Silently overwriting instead would let a + // run keep believing the number it typed. + if ( + parsed.criticalsInline !== undefined || + parsed.suggestionsInline !== undefined + ) { + throw new Error( + 'compose-review: `criticalsInline` / `suggestionsInline` are counted ' + + 'from the --comments file, not taken from the state JSON. Remove ' + + 'them. (A dogfooded run moved its one Critical from `bodyCriticals` ' + + 'to an inline comment, dropped the count on the way, and the ' + + 'verdict line read Approve over a blocker.)', + ); + } + const drafted = readDraftedComments(comments); + const result = composeReview({ + ...parsed, + ...countInlineFindings(drafted), + }); // The exact terminal verdict, persisted beside the fields it is computed // from. `event` + `cappedBy` alone cannot reconstruct it — a presubmit // downgrade also depends on `downgraded`/`downgradedFrom` — and Step 8's diff --git a/packages/cli/src/commands/review/lib/inline-counts.ts b/packages/cli/src/commands/review/lib/inline-counts.ts new file mode 100644 index 00000000000..3ac4626e723 --- /dev/null +++ b/packages/cli/src/commands/review/lib/inline-counts.ts @@ -0,0 +1,80 @@ +/** + * @license + * Copyright 2026 Qwen Team + * SPDX-License-Identifier: Apache-2.0 + */ + +// The inline finding counts, derived from the drafted comments — never accepted +// as numbers. +// +// A count handed over beside the thing it counts is a count that can disagree +// with it, and both directions have now happened on real runs: `submit` once +// took `criticalsInline` as a number and a run posted "Suggestions are inline" +// beside an empty comments array; then `compose-review` kept taking the numbers +// after `submit` stopped, and a dogfooded report-only run — which never reaches +// `submit`'s recount — moved its one Critical from the body list to an inline +// comment, dropped the count on the way, and `compose-review` printed +// `Verdict: Approve` over a Critical the report itself listed. One counting +// function, fed by the comments array both callers already hold. + +/** The severity prefixes the skill mandates on every posted inline comment. */ +export const CRITICAL_PREFIX = '**[Critical]**'; +export const SUGGESTION_PREFIX = '**[Suggestion]**'; + +/** A drafted inline comment, as far as counting needs it. */ +export interface DraftedComment { + body?: unknown; +} + +/** + * Which severity marker a drafted comment opens with — or null for neither. + * + * The ONE statement of the predicate. The counter and the unmarked-scan each + * restated it at first, and drift between restatements is exactly the + * bug-class this file's header describes; every caller classifies through + * here so the two can never disagree about what "marked" means. + */ +export function severityOf( + c: DraftedComment, +): 'critical' | 'suggestion' | null { + const body = typeof c?.body === 'string' ? c.body.trimStart() : ''; + if (body.startsWith(CRITICAL_PREFIX)) return 'critical'; + if (body.startsWith(SUGGESTION_PREFIX)) return 'suggestion'; + return null; +} + +/** How many drafted comments open with each severity marker. */ +export function countInlineFindings(comments: readonly DraftedComment[]): { + criticalsInline: number; + suggestionsInline: number; +} { + let criticalsInline = 0; + let suggestionsInline = 0; + for (const c of comments) { + const severity = severityOf(c); + if (severity === 'critical') criticalsInline++; + else if (severity === 'suggestion') suggestionsInline++; + } + return { criticalsInline, suggestionsInline }; +} + +/** + * The indices of drafted comments that open with NEITHER severity marker. + * + * `countInlineFindings` counts such a comment as nothing at all — which for a + * verdict computation means a blocker written without its marker weighs zero. + * Both boundaries refuse these outright instead: `compose-review` because + * Step 6 is where the draft is still cheap to fix, and `submit` because the + * skill's own re-compose instruction expects the set to churn after Step 6 — + * a marker lost in that churn would otherwise reach the one boundary that + * actually posts, and weigh zero there. + */ +export function unmarkedComments( + comments: readonly DraftedComment[], +): number[] { + const out: number[] = []; + comments.forEach((c, i) => { + if (severityOf(c) === null) out.push(i); + }); + return out; +} diff --git a/packages/cli/src/commands/review/submit.test.ts b/packages/cli/src/commands/review/submit.test.ts index 5a9920bf4b8..c4146455f05 100644 --- a/packages/cli/src/commands/review/submit.test.ts +++ b/packages/cli/src/commands/review/submit.test.ts @@ -312,6 +312,27 @@ describe('payload consistency — refuse before GitHub sees it', () => { expect(ghMock).not.toHaveBeenCalled(); }); + it('refuses an inline comment with no severity marker — it would weigh nothing', () => { + // Step 6 refuses unmarked drafts, but the skill's re-compose instruction + // expects the comment set to churn after Step 6 — and a marker lost in + // that churn reaches exactly this boundary, the one that posts. The + // verdict is counted from the markers, so an unmarked blocker weighs + // zero: beside a clean state it composes an APPROVE that posts the very + // comment it never weighed. + const review = file('c3.json', { + ...REVIEW, + comments: [ + { path: 'a.ts', line: 12, body: '**[Critical]** boom' }, + { path: 'b.ts', line: 3, body: 'this blocker lost its marker' }, + ], + }); + + expect(() => runSubmit(authorized({ review }))).toThrow( + /comments\[1\] opens with neither/, + ); + expect(ghMock).not.toHaveBeenCalled(); + }); + it('writes the body as JSON, so a finding that quotes `\\n` survives intact', () => { // Finding text quotes code: `/\n/` in a regex, an escaped string in a snippet. // The body used to be built by the caller — sometimes with `-f body=`, which diff --git a/packages/cli/src/commands/review/submit.ts b/packages/cli/src/commands/review/submit.ts index d19d68785e1..2e2ea60c5c6 100644 --- a/packages/cli/src/commands/review/submit.ts +++ b/packages/cli/src/commands/review/submit.ts @@ -53,6 +53,12 @@ import { skillArgsPath, currentSessionId, } from '../../services/skill-args-file.js'; +import { + CRITICAL_PREFIX, + SUGGESTION_PREFIX, + countInlineFindings, + severityOf, +} from './lib/inline-counts.js'; /** * Where the CLI records a skill's invocation arguments, verbatim, before the @@ -118,19 +124,10 @@ interface ReviewPayload { body?: unknown; } -/** - * The prefixes the skill mandates on every posted comment, and the autofix - * workflow keys off. - * - * They are what makes the inline counts *derivable*. A caller used to hand - * `criticalsInline` over as a number beside the comments — and a number beside a - * thing is a number that can disagree with it. The breaching dogfood run posted a - * body reading "Suggestions are inline" next to an empty `comments` array and a - * summary claiming `0 Suggestion inline`; every count in it disagreed with every - * other. Count the comments. - */ -const CRITICAL_PREFIX = '**[Critical]**'; -const SUGGESTION_PREFIX = '**[Suggestion]**'; +// The severity prefixes and the counting live in `lib/inline-counts.ts`, +// shared with `compose-review`: the Step 6 verdict line and the Step 7 posted +// verdict must be the same computation on the same source, and two counting +// functions is how they were once allowed to disagree. /** * Was this run authorised to write to the pull request? @@ -269,12 +266,7 @@ function compose(payload: ReviewPayload): { } { const comments = payload.comments ?? []; const state = payload.state ?? ({} as ComposeReviewInput); - const criticalsInline = comments.filter((c) => - (c.body ?? '').trimStart().startsWith(CRITICAL_PREFIX), - ).length; - const suggestionsInline = comments.filter((c) => - (c.body ?? '').trimStart().startsWith(SUGGESTION_PREFIX), - ).length; + const { criticalsInline, suggestionsInline } = countInlineFindings(comments); // `env` decides where the harness transcripts are read from, and it must not // come from a JSON the caller wrote: a run that wanted an approval could point @@ -354,6 +346,20 @@ function inconsistencies(payload: ReviewPayload, event: string): string[] { if (!c.path) problems.push(`${at} has no \`path\``); if (!c.body) problems.push(`${at} has no \`body\` — an empty comment`); + // The verdict above was counted from these markers, so a body carrying + // neither weighed nothing in it. Step 6 already refuses unmarked drafts, + // but the skill's own re-compose instruction expects the comment set to + // churn after Step 6 — and a marker lost in that churn reaches exactly + // this boundary, the one that posts. A blocker that weighs nothing + // approves the review it should block. + if (c.body && severityOf(c) === null) { + problems.push( + `${at} opens with neither ${CRITICAL_PREFIX} nor ` + + `${SUGGESTION_PREFIX} — the verdict counts comments by their ` + + `severity marker, and an unmarked one weighs nothing in it`, + ); + } + if (!isDiffLine(c.line)) { problems.push( `${at} has no usable \`line\` (${JSON.stringify(c.line)}) — a line is a ` + diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 7950f5dfedd..f1d5916c208 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -301,7 +301,7 @@ Seven rounds of review-the-review on this PR converged on one diagnosis: the ski The resolution is the same one this document already records for presubmit and cleanup: judgment stays in the prompt, bookkeeping moves to tested subcommands that version together with the skill. - **`parse-args`** owns the grammar. Every previously-shipped parsing bug is a named row in its table-driven tests. The raw string travels **on stdin** (`--stdin` with a quoted heredoc), never as a positional: a flag-first raw string (`/review --effort low`) is consumed by the CLI's own strict parser before the handler runs, and a positional also breaks on quotes and shell metacharacters. Pure-function tests could not see that class — the documented invocation failed only when run against the built binary — so the suite includes yargs-level wiring tests alongside the table. -- **`compose-review`** owns event selection and body composition — the C/S table (counting body Criticals and discarded Suggestions), the event caps (cannot-tell existing Criticals, uncoverable chunks, unreviewed dimensions, context-unavailable), the downgrade carve-outs, and the clause composition. Its truth-table tests pin each shipped bug; writing them immediately caught one more instance of the class (all Suggestions discarded → S=0 → APPROVE). The input is validated at the boundary: the producer is a model writing JSON that omits inapplicable fields, so absent counts default to zero and malformed values throw typed errors — before that, an omitted count meant `undefined + 1 = NaN`, which fails every event comparison and would have returned APPROVE over a body-only blocker. 422 recovery stops being a hand-derived recomposition: it is the same call with updated counts, so the "recompute may never upgrade the verdict" guarantee holds by construction. +- **`compose-review`** owns event selection and body composition — the C/S table (counting body Criticals and discarded Suggestions), the event caps (cannot-tell existing Criticals, uncoverable chunks, unreviewed dimensions, context-unavailable), the downgrade carve-outs, and the clause composition. Its truth-table tests pin each shipped bug; writing them immediately caught one more instance of the class (all Suggestions discarded → S=0 → APPROVE). The input is validated at the boundary: the producer is a model writing JSON that omits inapplicable fields, so absent counts default to zero and malformed values throw typed errors — before that, an omitted count meant `undefined + 1 = NaN`, which fails every event comparison and would have returned APPROVE over a body-only blocker. 422 recovery stops being a hand-derived recomposition: it is the same call with the updated `--comments` file (the inline counts are counted from the drafted comments, never typed — a dogfooded report-only run dropped the typed count while moving its one Critical inline, and the verdict line read Approve over it), so the "recompute may never upgrade the verdict" guarantee holds by construction. - **`pr-context`** ends the fetch-prose chain at its root: review bodies **and every blocker-bearing body** render **in full** (a body-only blocker lives only there; a capped body names its review or comment id so the tail stays fetchable one object at a time, and reply snippets name their comment id when cut), and blocker-bearing threads are quarantined into a "Blockers to re-check" section instead of settling into "Already discussed" — a reply alone never retires a blocker. The `gh` wrapper's `maxBuffer` rises to 64 MiB, closing the ENOBUFS that killed two subcommands mid-review on a comment-heavy PR. What deliberately stays prose: everything judgment-shaped — what counts as a Critical, verification, the posting gate's authorization semantics, the angles. A truth table cannot decide whether a finding is real; it can guarantee that a real finding is never mislabeled, dropped by a downgrade, or approved past. @@ -384,7 +384,7 @@ No new `compose-review` input was needed: `cannot tell` already caps the event. Six concurrent real-PR runs (batch 3) produced three targeted changes, each fixing something the batch measured rather than predicted: -- **Overlap disposal is deterministic.** presubmit's overlap report used to end in "ask the user whether to proceed" — 2 of 6 runs stalled on an improvised interactive question (fatal for a headless run) while the other 4 proceeded, the signature of an under-specified decision point. An overlap is a duplicate by the Exclusion Criteria; the rule is now drop, note in the terminal, continue — and the counts handed to `compose-review` shrink accordingly, so a dropped finding can never flip the verdict. +- **Overlap disposal is deterministic.** presubmit's overlap report used to end in "ask the user whether to proceed" — 2 of 6 runs stalled on an improvised interactive question (fatal for a headless run) while the other 4 proceeded, the signature of an under-specified decision point. An overlap is a duplicate by the Exclusion Criteria; the rule is now drop, note in the terminal, continue — and the comments file `compose-review` counts from shrinks accordingly, so a dropped finding can never flip the verdict. - **Host routing is a flag, not prose.** The GH_HOST-by-prefix instruction survived exactly one review round before a reviewer noted the model must remember it per call. `--host` on `fetch-pr` / `pr-context` / `presubmit` routes every wrapped `gh` call in code (`lib/gh.ts` `setGhHost`/`ghEnv`), leaving the prose rule only for the handful of `gh` commands the orchestrating model runs directly. - **A fixed completion line.** Three different completion phrasings across one batch each needed their own detection regex in the batch driver. Step 9 now ends every run with `Review complete: `, greppable by `^Review complete: `. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index d6646f95fe1..2712dd1b182 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -627,10 +627,11 @@ Two failure modes this closes, both observed in this repo's own dogfood: reporti ```bash "${QWEN_CODE_CLI:-qwen}" review compose-review --input .qwen/tmp/qwen-review-{target}-compose.json \ + --comments .qwen/tmp/qwen-review-{target}-comments.json \ --out .qwen/tmp/qwen-review-{target}-composed.json ``` -It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every high-effort review, whether or not you are going to post. The state file is the same one Step 7 uses (see there for every field): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the presubmit flags, the model id. It does **not** take the coverage or the inline counts. It derives coverage from the harness's transcripts, and Step 7 derives the inline counts from the comments you actually attach. +It prints a `Verdict:` line to stderr. **That line is the verdict — print it, and nothing else.** It writes nothing, posts nothing, and needs no authorisation, so run it on every high-effort review, whether or not you are going to post. The state file is the same one Step 7 uses (see there for every field): your findings and the states you established — the body Criticals, the discarded suggestions, the `cannot tell` blockers, the unreviewed dimensions, the `planPath`, the presubmit flags, the model id. It does **not** take the coverage or the inline counts, and it **refuses** a state JSON carrying `criticalsInline`/`suggestionsInline`. It derives coverage from the harness's transcripts, and it **counts** the inline findings from `--comments`: write the drafted inline comments to that file first — the same `[{path, line, body, …}]` array the Step 7 payload will carry, each body opening with its `**[Critical]**`/`**[Suggestion]**` marker; a review with nothing anchored inline passes a file containing `[]`. Dogfooded, a report-only run — where no later step recounts — moved its one Critical from `bodyCriticals` to an inline comment, and the verdict line read Approve over a blocker the same report listed; counted from the draft, that finding cannot fall out of the computation. **If the comment set changes after composing** — an anchor fails to resolve, a finding relocates to the body, a comment is dropped — update the comments file (and the state), and run `compose-review` again: the verdict must be computed from the set you actually post, and Step 7's `submit` recounts from the payload to hold you to it. **It also proves Step 4 and Step 5 ran — the way `check-coverage` proves Step 3.** `check-coverage` runs at Step 3D, before verify and reverse audit exist, so its roster cannot reach them; and their count is not in the plan (verify shards on the finding count, the reverse audit loops until it goes dry), so there is no exact roster to check. What there is is a floor, and `compose-review` — which runs only at high effort, where both steps are part of the contract — checks it from the same transcripts: at least one **reverse auditor** ran and opened its brief (on every high-effort review), and at least one **verifier** did (whenever the review posts findings). A step skipped wholesale, or run with agents that never opened their brief, is named in `unreviewedDimensions` and caps the verdict, exactly like a dimension nobody reviewed. You do not pass a flag for this and cannot turn it off: the proof is the intersection of the prompt the CLI recorded building (`--role verify` / `--role reverse-audit`) and the harness's transcript of an agent that ran it. So a run cannot approve a diff by skipping the pass that looks for what Step 3 missed — the highest-value catch here is a clean, zero-finding review that never ran its reverse audit.