diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 155d8f0a349..f2a6075d3cb 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -493,6 +493,13 @@ describe('agent-prompt (command boundary)', () => { // The verdict branch: Exclusion Criteria yes, finding format no. expect(briefText).toContain('What is NOT a finding'); expect(briefText).not.toContain('**Anchor:**'); + // The witness rule: a confirmed Critical returns its executed evidence + // or the one-line reason, and the sweep is a named witness form. These + // demands are what the orchestrator's low-confidence demotion sorts on, + // so a brief that drops them silently demotes every trace-only Critical. + expect(briefText).toContain('A confirmed Critical returns its witness.'); + expect(briefText).toContain('witness: not run —'); + expect(briefText).toContain('sweep the real population'); } finally { rmSync(dir, { recursive: true, force: true }); } diff --git a/packages/cli/src/commands/review/findings.test.ts b/packages/cli/src/commands/review/findings.test.ts index 2dcbb6ca502..28a07ef9086 100644 --- a/packages/cli/src/commands/review/findings.test.ts +++ b/packages/cli/src/commands/review/findings.test.ts @@ -24,6 +24,7 @@ import { type Finding, type FindingsReport, holdCriticalsFailingOnBase, + holdUnwitnessedCriticals, sharedFailingFilesOf, } from './findings.js'; @@ -511,6 +512,30 @@ describe('findings (command boundary)', () => { return out; } + it('demotes an unwitnessed Critical through the whole handler, and says so on stderr', () => { + // The unit tests pin holdUnwitnessedCriticals in isolation; this pins the + // WIRING — the call sits in the handler before buildReport, so removing + // it, or moving it after the report is built, fails here, not silently. + const input = join(dir, 'in.json'); + const out = join(dir, 'findings.json'); + writeFileSync( + input, + JSON.stringify([ + { ...base, id: 'w1' }, + { ...base, id: 'w2', witness: 'probe flipped: 2 calls → 1' }, + ]), + ); + const stderr = runCapturingStderr({ input, out, print: false }); + const report = JSON.parse(readFileSync(out, 'utf8')) as FindingsReport; + const byId = new Map(report.findings.map((f) => [f.id, f])); + expect(byId.get('w1')?.confidence).toBe('low'); + expect(byId.get('w1')?.failureScenario).toContain('witness rule'); + expect(byId.get('w2')?.confidence).toBe('high'); + expect(stderr).toContain('w1 filed at low confidence'); + expect(stderr).not.toContain('w2 filed at low confidence'); + expect(report.counts.byConfidence['low']).toBe(1); + }); + it('announces every hold, naming the finding and the measured file', () => { // A severity this command lowered is a change to what the review says. Left // unannounced it reads as the reviewer's own judgement, which is the one @@ -790,6 +815,68 @@ describe('findings (command boundary)', () => { }); }); +describe('holdUnwitnessedCriticals — the witness rule has a machine half', () => { + const critical = { + id: 'w1', + severity: 'Critical' as const, + confidence: 'high' as const, + source: 'review' as const, + summary: 'double-executes the shell command', + shortSummary: 'double execute', + failureScenario: 'run !git push → sendShellCommand fires twice', + locations: [{ file: 'src/pay.ts', line: 42 }], + }; + + it('files an unwitnessed high-confidence review Critical at low confidence, and says why', () => { + // The demotion the SKILL promises as mechanical: without this, the sort + // exists only as Step 4 prose, and an omitted `confidence` even defaults + // to `high` — the fail-open direction (dogfood review of the witness PR). + const { findings, unwitnessed } = holdUnwitnessedCriticals([critical]); + expect(findings[0].confidence).toBe('low'); + expect(findings[0].severity).toBe('Critical'); + expect(findings[0].failureScenario).toContain('witness rule'); + // The original evidence survives — the rule is appended, not substituted. + expect(findings[0].failureScenario).toContain('fires twice'); + expect(unwitnessed).toEqual(['w1']); + }); + + it('leaves a witnessed Critical alone — either form of the field counts', () => { + for (const witness of [ + 'BASE: 2 calls / PR: 1 call — probe flipped', + 'not run — needs a live OAuth endpoint this harness lacks', + ]) { + const { findings, unwitnessed } = holdUnwitnessedCriticals([ + { ...critical, witness }, + ]); + expect(findings[0].confidence).toBe('high'); + expect(unwitnessed).toEqual([]); + } + }); + + it('exempts deterministic sources — their witness is constitutive', () => { + // A [build]/[test]/[probe] finding IS a run's output; demanding a second + // witness would demote findings the pipeline treats as pre-confirmed. + for (const source of ['build', 'test', 'probe', 'lint'] as const) { + const { unwitnessed } = holdUnwitnessedCriticals([ + { ...critical, source }, + ]); + expect(unwitnessed).toEqual([]); + } + }); + + it('is idempotent — a demoted finding re-fed is not touched again', () => { + const once = holdUnwitnessedCriticals([critical]).findings[0]; + const twice = holdUnwitnessedCriticals([once]).findings[0]; + expect(twice).toEqual(once); + // Suggestions are never judged: the rule targets the severity that posts + // as a blocker. + expect( + holdUnwitnessedCriticals([{ ...critical, severity: 'Suggestion' }]) + .unwitnessed, + ).toEqual([]); + }); +}); + describe('holdCriticalsFailingOnBase', () => { // The shape test-delta writes: workspace-relative paths, while a finding // names the repo-relative one. @@ -1214,4 +1301,15 @@ describe('validateFindings — the canonical artifact round-trips', () => { expect(f.outcome).toBeUndefined(); expect(f.outcomeNote).toBeUndefined(); }); + + it('keeps witness, so the executed evidence survives being fed back', () => { + // The Step 4 witness rule attaches the evidence once; the report and the + // comment bodies read it back out of the artifact. Dropped here, every + // downstream quote becomes a fresh transcription. + const [f] = validateFindings([ + { ...base, witness: 'BASE: 2 calls / PR: 1 call — probe flipped' }, + ]); + expect(f.witness).toBe('BASE: 2 calls / PR: 1 call — probe flipped'); + expect(validateFindings([{ ...base }])[0].witness).toBeUndefined(); + }); }); diff --git a/packages/cli/src/commands/review/findings.ts b/packages/cli/src/commands/review/findings.ts index 068b4d846d5..49e325f4b8d 100644 --- a/packages/cli/src/commands/review/findings.ts +++ b/packages/cli/src/commands/review/findings.ts @@ -83,6 +83,13 @@ export interface Finding { shortSummary: string; /** The concrete trigger and wrong outcome — the finding's evidence. */ failureScenario: string; + /** + * The executed evidence that settled the verdict (a probe's two sides, an + * A/B's quoted pair, a sweep count) — or the verifier's + * `not run — ` line. Carried as data so the report and the comment + * bodies quote one recorded string instead of transcribing it twice more. + */ + witness?: string; suggestedFix?: string; /** Free-form kebab-case tag (`correctness`, `security`, `test-coverage`, …). */ category?: string; @@ -351,6 +358,11 @@ export function validateFindings(raw: unknown): Finding[] { const shortSummary = asString(o, 'shortSummary') ?? asString(o, 'short_summary'); + // `witness` round-trips for the same reason `outcomeNote` does: the Step 4 + // witness rule attaches it once, and the report and the comment bodies read + // it back out of the artifact instead of transcribing the evidence again. + const witness = asString(o, 'witness'); + return { id, severity, @@ -361,6 +373,7 @@ export function validateFindings(raw: unknown): Finding[] { ? compressSummary(shortSummary) : compressSummary(summary), failureScenario, + ...(witness ? { witness } : {}), ...(asString(o, 'suggestedFix') || asString(o, 'suggested_fix') ? { suggestedFix: (asString(o, 'suggestedFix') ?? @@ -507,6 +520,45 @@ export function holdCriticalsFailingOnBase( return { findings: out, held, readjudicated }; } +/** + * The witness rule's machine half. Step 4 demands that a confirmed Critical + * carry its executed evidence — the `witness` field, holding either the + * observed output or the verifier's `not run — ` line — and promises + * the demotion is mechanical. This is the mechanism, in the same place the + * test-delta holdback lives: a high-confidence Critical from the one + * non-deterministic source that arrives with no witness is filed at low + * confidence — terminal-only, never posted. Only `source: 'review'` is + * judged: a `[build]`/`[test]`/`[lint]`/`[probe]` finding IS a run's output, + * so its witness is constitutive, not an attachment. Nothing is deleted and + * nothing is raised; the appended sentence names the rule that moved it and + * the way back (attach the witness, or say why none could run). Idempotent by + * construction — a demoted finding re-fed through `--input` is already low + * confidence and is not touched again. + */ +export function holdUnwitnessedCriticals(findings: readonly Finding[]): { + findings: Finding[]; + unwitnessed: string[]; +} { + const unwitnessed: string[] = []; + const out = findings.map((f) => { + if ( + f.severity !== 'Critical' || + f.confidence !== 'high' || + f.source !== 'review' || + f.witness !== undefined + ) { + return f; + } + unwitnessed.push(f.id); + return { + ...f, + confidence: 'low' as Confidence, + failureScenario: `${f.failureScenario}\n\nFiled at low confidence by the witness rule: this confirmed Critical arrived with neither a witness (the executed evidence that settled the verdict) nor a \`not run — \` line. Attach either and it stands at high confidence again.`, + }; + }); + return { findings: out, unwitnessed }; +} + const WORKSPACE_IN_COMMAND_RE = /--workspace="([^"]+)"/; /** @@ -888,6 +940,8 @@ export const findingsCommand: CommandModule = { shared, )); } + const witnessHold = holdUnwitnessedCriticals(findings); + findings = witnessHold.findings; const report = buildReport(findings); const target = resolve(out); @@ -909,6 +963,14 @@ export const findingsCommand: CommandModule = { `findings: ${h.id} held back from Critical — test-delta measured ${h.file} as failing on the merge base too`, ); } + // The witness rule's demotions get the same disclosure: a confidence this + // command lowered must name the finding and the rule, or the demotion + // reads as the reviewer's own judgement. + for (const id of witnessHold.unwitnessed) { + writeStderrLine( + `findings: ${id} filed at low confidence — a confirmed Critical carried neither a witness nor a 'not run' reason (Step 4's witness rule)`, + ); + } // A hold that was weighed and reversed is a decision, and a decision this // command declined to overrule is exactly as reportable as one it made. for (const r of readjudicated) { diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 53e304a0bef..f9b45bd90d9 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -636,6 +636,8 @@ For each finding you were given: **When the fix IS a threshold, measure the threshold.** A guard built on a ratio or length cutoff makes the fix's coverage an empirical number, not a reading: hold every other variable fixed, vary the guarded quantity, and binary-search the boundary where behaviour flips. Then put that number next to what the linked issue actually reports — a live verification of a prose-ratio guard measured the minimum recovering payload at ~473 chars with the issue's own preamble held fixed, which proved the fix covered the issue's \`edit\`/\`write_file\` half and silently declined its \`run_shell_command\` half. "Fix is narrower than its claim, here is the boundary, here is the half it misses" is a finding no amount of code-reading produces. +**When the defect is mechanically enumerable, sweep the real population — the count is the verdict.** For a claim about a pattern, a predicate, or a parser ("this misclassifies X", "this mishandles shape Y"), do not stop at the one reported instance: run the check over every real instance this repo holds (every workflow step body, every call site, every input the code will actually see) and report the count. "195 of 434 real \`run:\` bodies reach this path" confirms the finding, sizes its severity, and hands the author a number they can re-run rather than argue with — and a count of **zero** is the quoted contradiction that rejects it. Two rules keep a sweep evidence rather than theatre: its oracle must be an **external authority** — the real parser, the real tool, \`bash -n\` — never your own reimplementation of the logic under test, because a mirror shares the blind spots of what it mirrors and mirrored sweeps have manufactured false findings out of their own bugs; and spot-check one hit by reading it before you quote a nonzero count. + **A suggested fix you did not run is a hypothesis; say which one you are giving.** When a finding's fix is cheap to apply, patch it in, re-run the same probe/harness to show it works, then revert — and state that every other number in your report comes from the unmodified PR (the contamination line is what lets a reader trust the rest). A fix too costly to verify is still worth proposing, labeled untested. **A probabilistic failure gets a RATE, not an anecdote.** For a timing/race claim, run N repetitions per arm and report the rates as the verdict; amplify with full CPU load to force the window open (a live case went from 4/11 idle to 5/5 loaded). And attribute honestly: a lower idle rate with no structural change is luck, not a fix. Fake-timer tests hardcode one ordering by construction — they cannot discriminate a race, so a green fake-timer suite is non-evidence here. @@ -696,6 +698,8 @@ Return, for each finding, one verdict: - **confirmed (low confidence)** — the mechanism is real but the trigger is uncertain (timing, environment, configuration). Say what would confirm it. Carry the severity. - **rejected** — the code does not do what the finding claims (**quote the contradicting code**), or it matches an Exclusion Criterion (one-line reason). +**A confirmed Critical returns its witness.** Alongside the verdict, include a \`witness:\` line quoting the observed output that settled it — the probe's two sides, the A/B's \`BASE:\`/\`PR:\` pair, the extracted step's run, the sweep count — trimmed to the deciding lines. When every run-capability above is genuinely inapplicable and the confirmation rests on the trace alone, write the one line \`witness: not run — \` instead; writing that line is also the moment you notice when the claim was runnable after all. This is mechanical downstream — enforced in code at the findings canonicalization, not merely by the orchestrator's read of its rules: a confirmed Critical returning neither the witness nor the reason line is filed at **low confidence** — terminal-only, never posted — whatever your prose argued, because the evidence a run produced is the one part of a Critical its author can act on without re-deriving the bug. + **Rejecting a Critical carries a higher bar than anything else, and it is one-way.** A rejected Critical is gone — no later stage revisits it, it vanishes from both the pull request and the terminal. To reject one you must **quote the specific code that contradicts the claim**. A passing test, a plausible-looking guard, or "I could not reproduce the reasoning" is not enough — when you cannot quote the contradiction, the floor is \`confirmed (low confidence)\`, never rejection. Downgrading is reversible; a human still sees a low-confidence finding under "Needs Human Review". Rejection is not. **For anything non-Critical, when uncertain, downgrade to low confidence rather than rejecting.** Reserve outright rejection for a finding that clearly does not match the code (it describes behaviour the code does not have) or matches an Exclusion Criterion. Low confidence is for "likely real, needs human judgement", not for "I have no idea" — a vague suspicion with no concrete evidence in the code can still be rejected. diff --git a/packages/core/src/skills/bundled/review/DESIGN.md b/packages/core/src/skills/bundled/review/DESIGN.md index 0b75dfa9317..c4885d51bfa 100644 --- a/packages/core/src/skills/bundled/review/DESIGN.md +++ b/packages/core/src/skills/bundled/review/DESIGN.md @@ -511,6 +511,14 @@ Four things this gets right by construction, three of them borrowed from the mut The gating mistake worth recording, because it inverted the feature while every test stayed green: the hunk loop first lived **inside** the mutant branch, so it ran only when the diff already had a safety-verb candidate. The one class of diff per-hunk probing exists for — no mutants at all — got nothing. Selection now happens beside the mutants' and the phase runs whenever **either** kind has candidates. +## Why a confirmed Critical carries a witness + +The verify brief accumulated three execution capabilities — the probe, the A/B, extract-step — and every one of them was **optional**, spent at the verifier's discretion. Mining the maintainer's dogfood corpus (356 review sessions and 182 real-environment verification sessions, 2026-06 through 2026-08) measured what discretion produces: in the review rounds that held up, evidence-gathering was ~80% of all tool calls and **every posted hard finding quoted executed output** — a probe's two sides, a repo-wide count, a failing test's text. The counterexample is the rule's origin: the one round-1 claim written from a reading alone was retracted publicly in round 2 when its first measurement returned zero (see the measured entry). The witness rule inverts the default for the findings that post at the highest severity: the executed evidence **is** the confirmation; a reading is a reason to go get one, or to say in one line why none can run. + +The enforcement shape is borrowed from the `— [unverified]` tag rather than from the brief, and the borrowing includes the tag's machine half: "no witness and no reason ⇒ low confidence" is first a sort the orchestrator performs on observable state, and then a demotion `qwen review findings` applies in code at canonicalization — a high-confidence `[review]`-source Critical with no `witness` field is filed at low confidence, each named on stderr. The rule shipped without the code half first, and the PR's own dogfood review caught it: the precedent being cited (compose-review machine-reads the surviving tags) HAS code, `validateFindings` defaults an omitted confidence to `high` — the fail-open direction — and the same command already demotes Criticals mechanically for test-delta, so the pattern was local. Deterministic sources are exempt: a `[build]`/`[test]`/`[probe]` finding is itself a run's output, and demanding a second witness of it would demote findings the pipeline treats as pre-confirmed. A verifier that traced a genuinely unrunnable claim still posts it — the one-line reason is cheap, and writing it is exactly the moment the verifier notices when the claim was runnable after all. The field rides the findings artifact (`witness`, optional) for the same reason `failureScenario` does: the report and the comment bodies quote one recorded string instead of transcribing the evidence twice more, and transcription is this skill's best-documented failure mode. + +The impact sweep earned its place as a named witness form because it does two jobs no single-instance confirmation can: it converts severity from adjective to measurement ("195 of 434 real step bodies"), and it retracts as mechanically as it confirms — a sweep that returns zero is a false Critical caught before posting instead of after. Its external-authority guard exists because both of its measured failures were the same failure: an oracle that reimplemented the logic under test inherited its blind spots and manufactured findings out of its own bugs. + ## Why "fixed by this diff" is the verdict that needed a bar The re-check has three verdicts, and until PR #6486 only two of them cost anything: @@ -964,3 +972,11 @@ The fix is two briefs, not a new agent — the lens goes where an agent already The question also needs a stop rule that respects it. The reverse audit converges on "two consecutive dry rounds" — no auditor found a new gap — which is sound evidence about the layer the auditors walked and silent about every layer they did not. On this guard the token layer filled every round while the state layer went unexamined, and a dry round on the token layer said nothing about it, so the loop could converge with a whole class untouched. The auditor brief now asks, for a modeled executable system, that each defect layer be walked and **receipted on its own line** — `Layer walked: ` — the `Budget gap:` discipline, a line the tooling reads rather than prose it guesses at. `audit-layers.ts` parses those receipts into per-layer coverage; a run that receipts `lexing` and `expansion` but never `scope-propagation` has named its own blind spot. Empirically the receipt requirement changes the model's behaviour on the target model: on a synthetic evaluator with a planted state-propagation defect, the same qwen3.8-max auditor emitted **zero** layer receipts under the old brief and a full **six** under the new one, walking (and filing findings against) the `resolution-order`, `inheritance`, and `toctou` layers a flat "find gaps" pass left untouched. Coverage feeds a cap, not the stop rule — deliberately, and this is where the change stops for now. `layerAuditGate` (in `compose-review`, model out of the loop like `scriptLintGate`) reads the reverse-audit returns and emits one `unreviewedDimensions` entry per unwalked layer, which caps a would-be Approve to Comment and discloses the gap. It is **opt-in and inert by default**: it fires only when a `.qwen/review-context.json` matching rule — read from the trusted base branch — sets the `modeled-executable-system` domain on the diff, so no ordinary review is touched, and it rides an existing manifest field rather than the strict context schema. And it moves in one direction only: it can **withhold** an Approve, never end the audit loop early, never block a Request changes, never retire a chunk. Making an unwalked layer actually _extend_ the loop — turning "two dry rounds" into "two dry rounds AND every declared layer walked" — is the larger, riskier half, and it is staged behind an A/B on real modeled-system PRs rather than shipped on this reasoning: a stop rule that never converges is a worse failure than one that stops a layer early, and the measured evidence that discriminates them does not exist yet. The cap is the safe increment that makes the coverage visible and consequential while that evidence is gathered. + +### The read-only claim retracted in round 2 (PR #8225) + +A maintainer-dogfood round-1 finding asserted head-comment leakage against the text the tool actually scans, written from a reading of the code. Round 2 swept all 434 real `run:` step bodies in the repo through the tool and measured that claim's coverage at **zero** — true of workflow files, false of the extracted text the tool operates on — and the round-2 review had to open with a public correction: "I asserted it without verifying." The same sweep, in the same pass, gave the surviving findings their scale (195 of 434 real step bodies affected): one measurement both retracted the unexecuted claim and armed the executed ones. + +### The mirrored oracle's false positives (PR #8225) + +Two sweeps in the same dogfood series manufactured findings out of their own bugs: a round-2 sweep unconditionally filtered `set -e` lines and reported four leaks that were not there, and a round-7 differential oracle misread a shell continuation line as a command position and reported one miss that was not one. Both oracles were reimplementations of the logic under test — a mirror of the implementation shares its blind spots. The round-7 fix handed adjudication to bash itself, and the false red vanished; that is why a sweep's oracle must be an external authority, and why a nonzero count is spot-checked by reading one hit before it is quoted. diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 4d24364ea74..6955a1a6e57 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -592,7 +592,11 @@ The brief also carries the **A/B capability**, which is the probe's counterpart The brief also carries **`extract-step`**, which is the A/B's counterpart for a claim about a **workflow**. A `run:` script is a shell program that happens to live inside YAML, and reviewing one in place fails in a way reading normal code does not: the body is indented inside a block scalar, the `env:` that decides its behaviour is spread over three levels — workflow, job, step, nearest wins, and two of them sit nowhere near the step — and every `${{ … }}` is a hole the reader silently fills in. `qwen review extract-step` lifts the script out **verbatim** as an executable and reports what the runner would have supplied around it: the merged three-level `env:` with each key's level named, every `${{ … }}` site listed unevaluated (the stub list — the command refuses to invent values), the resolved `shell` and `working-directory`, and a heuristic list of invoked commands. What to stub and what to feed it stays with the verifier, which is the judgment half; with `base-tree`, the two arms of a workflow A/B become two invocations. A `uses:` step has no `run:` and is refused rather than simulated. -**After verification:** remove all rejected findings. Separate confirmed findings into two groups: high-confidence and low-confidence. Low-confidence findings appear **only in terminal output** (under "Needs Human Review") and are **never posted as PR inline comments** — this preserves the "Silence is better than noise" principle for PR interactions. +**The witness rule.** The capabilities above exist so a verdict can be something a run produced instead of something a reading concluded, and for a **Critical** that difference is the verdict: a confirmed Critical carries a **witness** — the observed output that settled it, quoted and trimmed to the deciding lines — or one line saying why none could run (`witness: not run — `: the claim needs infrastructure the harness lacks, a timing window no probe can pin, state only production holds). The forms a witness takes are exactly the capabilities' outputs: the probe's flip (both sides), the A/B's two quoted outputs, an extract-step run, the failing build/test text a `[build]`/`[test]` finding already carries, the render read-back, and the **impact sweep** below. A confirmed Critical carrying neither the witness nor the one-line reason is not confirmed at the bar this pipeline posts at: sort it **low confidence** — terminal-only, "Needs Human Review" — whatever the verifier's prose says. The demotion is deliberately mechanical, the same shape as the `— [unverified]` tag — and like that tag it has a machine half, not just this rule: `qwen review findings` (Step 6) demotes any high-confidence `[review]`-source Critical that arrives without the `witness` field and names each demotion on stderr, so a sort you miss here is caught at canonicalization rather than posted. Deterministic sources are exempt there by construction — a `[build]`/`[test]`/`[probe]` finding IS a run's output. This is the double-execute lesson made the default instead of the option (measured; DESIGN.md — The double-execute the probe caught), and it is what maintainer dogfooding measured at scale from the other side: in the review rounds that held up, every posted hard finding quoted executed output, and the one claim written from a reading alone was retracted publicly a round later when its first measurement came back zero (measured; DESIGN.md — The read-only claim retracted in round 2 (PR #8225)). + +**The impact sweep** is the witness form for a defect that is mechanically enumerable — a pattern misused, a predicate that misclassifies, a parser that mishandles a shape. Instead of confirming the one reported instance, run the check over the repo's **real population** (every workflow step body, every call site, every input the predicate will actually see) and quote the count. "195 of 434 real `run:` bodies reach this path" is at once the confirmation, the severity evidence, and a number the author can re-run rather than argue with — and "0 of 434" is the retraction that keeps a false Critical off the PR. Two guards keep a sweep evidence rather than theatre: its oracle must be an **external authority** — the real parser, the real tool, `bash -n` — never a reimplementation of the logic under test, because a mirror of the implementation shares its blind spots and mirrored sweeps have manufactured false findings twice (measured; DESIGN.md — The mirrored oracle's false positives (PR #8225)); and a nonzero count is spot-checked by reading one hit before it is quoted. + +**After verification:** remove all rejected findings. Separate confirmed findings into two groups: high-confidence and low-confidence, applying the witness rule as you sort — a Critical whose confirmation carries neither witness nor the one-line reason lands in the low-confidence group. The witness rides the finding from here on — into the findings artifact (`witness`, Step 6), the terminal report, and, on a posting run, the inline comment body (Step 7) — because the evidence that settled the verdict is the one part of a finding the author can act on without re-deriving the bug. Low-confidence findings appear **only in terminal output** (under "Needs Human Review") and are **never posted as PR inline comments** — this preserves the "Silence is better than noise" principle for PR interactions. ### Pattern aggregation @@ -606,6 +610,7 @@ After verification, identify **confirmed** findings that describe the **same typ - **Occurrences:** N locations - **Example:** - **Failure scenario:** + - **Witness:** ` line; the witness rule reads an aggregate exactly as it reads a standalone finding> - **Suggested fix:** - **Severity:** @@ -716,9 +721,10 @@ For each **individual** finding, include: 2. **Source tag** — `[build]`, `[test]`, or `[review]` 3. **What's wrong** — Clear description of the issue 4. **Failure scenario** — the concrete trigger and wrong outcome (for quality findings, the concrete cost or the quoted rule) -5. **Suggested fix** — Concrete code suggestion when possible +5. **Witness** — for a Critical: the observed output that settled the verdict, trimmed to the deciding lines — the probe's two sides, the A/B's quote pair, the sweep count over the real population, the failing test text — or the verifier's `not run — ` line (Step 4's witness rule). A Suggestion carries one when a run produced it; it is not owed one. +6. **Suggested fix** — Concrete code suggestion when possible -For **pattern-aggregated** findings, use the aggregated format from Step 4 (Pattern, Occurrences, Example, Failure scenario, Suggested fix, Severity) with the source tag added. +For **pattern-aggregated** findings, use the aggregated format from Step 4 (Pattern, Occurrences, Example, Failure scenario, Witness, Suggested fix, Severity) with the source tag added. Group high-confidence findings first. Then add a separate section: @@ -819,7 +825,7 @@ Write every confirmed finding — high and low confidence alike — as a JSON ar **One finding, one name.** A high-effort PR review also writes the incremental cache's cross-round `findings` ledger (Step 8), whose ids are `R-` — use those same ids here: a finding that will enter the ledger gets its `R-` as the artifact `id`, and a carried-forward finding keeps the id it already has. Two id schemes for one finding is how "R1-2" in next round's report and "f7" in this round's outcome ledger turn out to be the same defect that nobody can join. -Each entry carries `id` (unique — outcomes and resolved anchors both join on it), `severity`, `confidence`, `source`, `summary`, `failureScenario`, and either `file`/`line`/`anchor` or, for a pattern aggregate, a `locations[]` array with **one entry per location** (`suggestedFix`, `category` and `shortSummary` are optional; `shortSummary` is derived from `summary` when absent). The command validates the shape, refuses a duplicate id, refuses a finding with no failure scenario, sorts by severity → confidence → file → line → id, and writes counts nobody then recomputes by hand. Read the artifact for the numbers you quote in the Summary. This is a **canonicalization**, not a gate: it does not decide the verdict — `compose-review` does that, from the same findings — and it does not run at low effort, where the pass is unverified and emits no verdict. +Each entry carries `id` (unique — outcomes and resolved anchors both join on it), `severity`, `confidence`, `source`, `summary`, `failureScenario`, and either `file`/`line`/`anchor` or, for a pattern aggregate, a `locations[]` array with **one entry per location** (`suggestedFix`, `category`, `shortSummary` and `witness` are optional; `shortSummary` is derived from `summary` when absent; `witness` is the Step 4 witness — the executed evidence, or its `not run — ` line — carried as data so the report and the comment bodies quote one recorded string instead of transcribing it twice more). The command validates the shape, refuses a duplicate id, refuses a finding with no failure scenario, sorts by severity → confidence → file → line → id, and writes counts nobody then recomputes by hand. Read the artifact for the numbers you quote in the Summary. This is a **canonicalization**, not a gate: it does not decide the verdict — `compose-review` does that, from the same findings — and it does not run at low effort, where the pass is unverified and emits no verdict. **The severities in this artifact are the canonical ones — draft the inline markers and the compose state FROM it, not from the list you typed by hand.** Ordering alone does not close the loop: `compose-review` reads `comments.json` and `compose.json`, both hand-written, so a hold that lowered a severity here still ships as `**[Critical]**` in the payload if the marker was copied from the draft instead of the artifact. Read `severity` out of `findings.json` for every marker and for the body Criticals. @@ -925,6 +931,8 @@ Also skip this step (independently of the gate above) if the review target is no **Use the "Create Review" API to submit verdict + inline comments in a single call** (like Copilot Code Review). This eliminates separate summary comments — the inline comments ARE the review. +**A Critical's comment body carries its witness.** After the failure scenario, quote the observed output that settled the verdict — fenced, trimmed to the deciding lines — or the verifier's `witness: not run — ` line (Step 4's witness rule). The witness is the difference between a comment the author can act on and a claim they have to re-derive before they can trust; the findings artifact already holds the string (`witness`), so this is a copy from data, not a fresh transcription. + **Resolve every anchor before you submit — do not post the line numbers the agents reported.** GitHub rejects the whole review with a 422 if any comment's `(path, line)` falls outside every hunk of that file, and it does so all-or-nothing: one miscounted anchor takes every Critical in the review down with it. The line is therefore computed from the diff, not carried over from an agent. Write every Critical and Suggestion headed for the `comments` array — using each finding's **Anchor** snippet — and run the resolver: ```bash @@ -1306,6 +1314,7 @@ These criteria apply to both Step 3 (review agents) and Step 4 (verification age - Focus on the diff, not pre-existing issues in unchanged code. - Keep the review concise. Don't repeat the same point for every occurrence — use pattern aggregation. - When suggesting a fix, show the actual code change. +- A Critical you post carries its witness — the observed output that proved it — or says in one line why none could run (Step 4's witness rule). - Flag any exposed secrets, credentials, API keys, or tokens in the diff as **Critical**. - Silence is better than noise. If you have nothing important to say, say nothing. - **Do NOT use `#N` notation** (e.g., `#1`, `#2`) in PR comments or summaries — GitHub auto-links these to issues/PRs. Use `(1)`, `[1]`, or descriptive references instead. diff --git a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx index e7b0039d421..65adbb98e3c 100644 --- a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx +++ b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.test.tsx @@ -34,6 +34,7 @@ const reviewDocument = { shortSummary: 'Cross-workspace overwrite', failureScenario: 'Open workspace B and submit a request from workspace A.', + witness: 'BASE: overwrote workspace B / PR: refused — probe flipped', suggestedFix: 'Resolve the runtime before writing.', category: 'security', locations: [{ file: 'src/write.ts', line: 42, anchor: 'writeFile()' }], @@ -136,6 +137,13 @@ describe('CodeReviewArtifactDetail', () => { expect(container.textContent).toContain('coverage'); expect(container.textContent).toContain('Source: test'); expect(container.textContent).toContain('Failure scenario'); + // The executed evidence is the field the witness rule exists to deliver + // to the author; parsing it and dropping it from display was the measured + // gap (PR 9065 review R1-3). + expect(container.textContent).toContain('Witness'); + expect(container.textContent).toContain( + 'BASE: overwrote workspace B / PR: refused — probe flipped', + ); expect(container.textContent).toContain('Suggested fix'); expect(container.textContent).toContain( 'skipped — Outside the reviewed diff.', diff --git a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx index b5babe6c6c8..1b89768a833 100644 --- a/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx +++ b/packages/web-shell/client/components/artifacts/CodeReviewArtifactDetail.tsx @@ -38,6 +38,8 @@ interface ReviewFinding { summary: string; shortSummary: string; failureScenario: string; + /** The executed evidence that settled the verdict, or its `not run` line. */ + witness?: string; suggestedFix?: string; category?: string; locations: FindingLocation[]; @@ -208,6 +210,9 @@ function parseFinding(value: unknown, index: number): ReviewFinding { source['failureScenario'], `${label}.failureScenario`, ), + ...(optionalString(source['witness'], `${label}.witness`) + ? { witness: source['witness'] as string } + : {}), ...(optionalString(source['suggestedFix'], `${label}.suggestedFix`) ? { suggestedFix: source['suggestedFix'] as string } : {}), @@ -588,6 +593,12 @@ export function CodeReviewArtifactDetail({ label={t('codeReview.failureScenario')} value={finding.failureScenario} /> + {finding.witness && ( + + )} {finding.suggestedFix && ( `Source: ${v?.value ?? ''}`, 'codeReview.failureScenario': 'Failure scenario', + 'codeReview.witness': 'Witness', 'codeReview.suggestedFix': 'Suggested fix', 'codeReview.outcome': 'Outcome', 'codeReview.locations': 'Locations', @@ -4058,6 +4059,7 @@ const ZH: Messages = { 'codeReview.noMatches': '没有符合当前筛选条件的发现。', 'codeReview.source': (v) => `来源：${v?.value ?? ''}`, 'codeReview.failureScenario': '失败场景', + 'codeReview.witness': '实测证据', 'codeReview.suggestedFix': '建议修复', 'codeReview.outcome': '处理结果', 'codeReview.locations': '位置',