From 2c53a9260658c9a8f57b16bc6021d4a7111bfd99 Mon Sep 17 00:00:00 2001 From: verify Date: Sun, 26 Jul 2026 23:27:07 +0800 Subject: [PATCH] =?UTF-8?q?feat(review):=20give=20the=20verifier=20a=20pro?= =?UTF-8?q?be=20capability=20=E2=80=94=20run=20a=20runnable=20claim,=20don?= =?UTF-8?q?'t=20just=20read=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Measured on this repo: read-only verification is where the review misses its hardest bugs. Handed the exact low-confidence finding for a `!` command that executes twice, the strongest model's Step-3 agents traced the mechanism and called it correct (0/3 across the full roster). Validated the fix directly — when the same models (3.7-max and 3.8-max-preview) were allowed to WRITE AND RUN a probe, both confirmed the double-execute from observed behaviour (`sendShellCommand called twice with ["git push"]`), both did the probe-validity self-check unprompted, and a plausible-but-false negative control was correctly refuted with no fabrication. So the verifier's brief now says: when a finding's failure scenario is a runnable claim about a named unit and the repo has a fast unit harness (vitest/jest/pytest), write a minimal probe, run it, and let the observed behaviour settle the verdict. Two rules keep it evidence rather than theatre — a mandatory self-check that the probe flips between buggy and correct, and leaving the tree exactly as found. A finding a probe confirmed carries `Source: [probe]`, which compose-review treats as deterministic (a run produced it) like `[build]`/`[test]`. This is Phase 1: the agent-driven loop the validation exercised, no new command. The deterministic runner + artifact and the finding-generation side (emitting a probeable low-confidence finding rather than concluding "correct") are follow-ups. --- .../cli/src/commands/review/agent-prompt.test.ts | 15 +++++++++++++++ .../src/commands/review/compose-review.test.ts | 15 +++++++++++++++ .../cli/src/commands/review/compose-review.ts | 11 ++++++----- .../cli/src/commands/review/lib/agent-briefs.ts | 7 +++++++ packages/core/src/skills/bundled/review/SKILL.md | 2 +- 5 files changed, 44 insertions(+), 6 deletions(-) diff --git a/packages/cli/src/commands/review/agent-prompt.test.ts b/packages/cli/src/commands/review/agent-prompt.test.ts index 88bfb3423f2..b8c15293428 100644 --- a/packages/cli/src/commands/review/agent-prompt.test.ts +++ b/packages/cli/src/commands/review/agent-prompt.test.ts @@ -1820,6 +1820,21 @@ describe('buildRoleBrief — every agent, not just the territory ones', () => { ); }); + it('gives the verifier the probe capability — run a claim, self-check the probe, tag [probe]', () => { + // Measured: read-only verification traced a real double-execute and called it + // correct. The verifier may RUN a probe for a runnable claim; the self-check + // (make the probe flip) is what keeps it evidence, and `Source: [probe]` is + // what makes compose-review treat it as deterministic. + const p = buildRoleBrief(PLAN, 'verify'); + expect(p).toContain('do not just trace it — run it'); + expect(p).toContain('write a **probe**'); + expect(p).toContain('confirm the probe **flips**'); + expect(p).toContain('Source: [probe]'); + expect(p).toContain('Leave the tree as you found it'); + // The capability is the verifier's; it must not bleed into a dimension brief. + expect(buildRoleBrief(PLAN, '1a')).not.toContain('write a **probe**'); + }); + it('carries the command-aware subprocess-injection correction into Agent 2', () => { // The all-role test sees only that Agent 2 gets the diff and the format; it // cannot see whether the `--` correction reached it. If a revert restores the diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index a02ae56609c..43eb69e8192 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -1865,6 +1865,21 @@ describe('the Step 4/5 gate — verify and reverse audit must have run (high eff expect(r.cappedBy).not.toContain('criticals-unverified'); }); + it('a [probe] finding is deterministic too — a run confirmed it, so it needs no separate verifier', () => { + // The verifier confirmed this by RUNNING a probe against the code; its + // evidence is an observed behaviour, so it is pre-confirmed like [build]/[test] + // and must not be softened for a missing verification it never owed. + const r = composeReview({ + criticalsInline: 0, + bodyCriticals: ['[probe] sendShellCommand ran twice for one `!git push`'], + planPath: coveredPlan(['reverse-audit']), // verifier absent, none owed + env: ENV, + modelId: MODEL, + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).not.toContain('criticals-unverified'); + }); + it('a verified Request changes still blocks — the cap binds only when Step 4 is missing', () => { const r = composeReview({ criticalsInline: 1, diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index c9d3fb84d1f..66880e1f594 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -271,12 +271,13 @@ export function composeReview(input: ComposeReviewInput): ComposeReviewResult { let coveredChunks: number[] = []; // The Criticals a verifier must have ruled on before this review may post - // them as blockers. Deterministic `[build]`/`[test]` body findings are - // pre-confirmed and skip verification by design; every other Critical — - // anchored or body — is a claim, and a claim is confirmed by Step 4 or it - // is not confirmed at all. + // them as blockers. Deterministic `[build]`/`[test]`/`[probe]` body findings + // are pre-confirmed and skip verification by design — `[probe]` is a finding + // the verifier confirmed by *running* a probe against the code, so its evidence + // is an observed behaviour, not a re-reading; every other Critical — anchored or + // body — is a claim, and a claim is confirmed by Step 4 or it is not at all. const nonDeterministicBodyCriticals = bodyCriticals.filter( - (x) => !/\[(?:build|test)\]/i.test(x), + (x) => !/\[(?:build|test|probe)\]/i.test(x), ).length; const criticalsNeedingVerify = criticalsInline + nonDeterministicBodyCriticals; diff --git a/packages/cli/src/commands/review/lib/agent-briefs.ts b/packages/cli/src/commands/review/lib/agent-briefs.ts index 064c09ccf92..895370f5f2f 100644 --- a/packages/cli/src/commands/review/lib/agent-briefs.ts +++ b/packages/cli/src/commands/review/lib/agent-briefs.ts @@ -476,6 +476,13 @@ For each finding you were given: 4. **Check the finding against the diff's own documented intent** — especially anything framed as a "regression", "removed protection", or "now allows X". Read the comments, JSDoc and rationale **inside the diff** for the changed lines. A behaviour the diff deliberately changes *and documents* (a comment saying \`X is intentionally preserved\`, a rationale block, a test asserting the new behaviour on purpose) is a design decision, not a defect — engage that rationale. This changes what you must do, **not** what confidence you may reach: a traced, concrete harm that survives the rationale keeps full confidence (if the author documents "unauthenticated access is intentional" and the trace still shows real data exposure, that is \`confirmed (high confidence)\` with the rebuttal stated — documentation does not make a harm safe). Use \`confirmed (low confidence)\` when engaging the rationale makes the harm genuinely uncertain. **Reject only** a finding that re-describes the documented change as a regression without naming a harm the rationale fails to answer. (A real run auto-posted a Critical claiming a secret-sanitization PR "now leaks AWS/GitHub tokens"; the file's own comment three lines up said those credentials **must remain available** to shell/MCP tools and the old broad denylist was the bug being fixed. The verifier had not read the rationale.) 5. **Reject a false positive** — a finding that matches an item in the Exclusion Criteria below. +**When the claim is runnable, do not just trace it — run it.** Reading is where this review missed its hardest bugs: measured, the strongest model traced a real double-execute (\`!git push\` firing twice) and called it correct. When a finding's failure scenario is a **concrete behavioural claim about a named unit** — a function, a component, a route — **and the repo has a fast unit harness** (a \`vitest\`/\`jest\`/\`pytest\` setup, with existing tests whose scaffolding you can copy) — **and tracing by reading has not settled it**, write a **probe**: a minimal test that reproduces the scenario and **records what actually happens** (the call count, the arguments, the return, the external state), and run it in the worktree. Two rules make a probe evidence and not theatre: + +- **Show it distinguishes buggy from correct.** After the probe reports the suspected-wrong behaviour, apply the one-line fix the finding implies (or revert the change that introduced it), re-run, and confirm the probe **flips**; then restore. A probe you cannot make flip proves nothing — it is inconclusive, and the finding stays at low confidence. +- **The observation is the verdict, not your reading of it.** The probe *ran* the code, so its output is the confirmation a Critical needs — cite the observed values (\`sendShellCommand called twice with ["git push"]\`). A probe that shows the **correct** outcome is exactly the "quote the contradicting code" that lets you reject a Critical: the code demonstrably does not do what the finding claims. A probe that could not be run, or could not be shown to flip, confirms nothing — fall back to the reading-based verdict and its low-confidence floor. + +**Leave the tree as you found it** — delete any probe file and revert any fix you applied for the self-check, so nothing you wrote reaches the diff or the build. A finding you actually probed carries \`Source: [probe]\` with the observed evidence; never tag one you only reasoned about — that source means "a run produced this", and downstream treats it as deterministic. + Return, for each finding, one verdict: - **confirmed (high confidence)** — the trace works: you can restate the failure scenario against the real code, naming the triggering input/state and quoting the line(s) that produce the wrong outcome. Carry the severity (Critical | Suggestion | Nice to have). diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index cd90fa8aa97..d5a4e9063f7 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -507,7 +507,7 @@ Write this shard's findings to a file — each with its file, line, issue and fa **`--findings` is required for this role — the command refuses without it**, because a bare block is a block you would assemble by hand, and hand-assembly is the one step this skill measured drifting. **Paste what it prints verbatim — the whole block, findings and all. Do not prepend, append, reword, or add a shard number** (a repeat round passes `--round ` and the CLI bakes the label in). Dogfooded twice: the step that used to have you prepend the list by hand is where the prompt got paraphrased — a summary inserted, the "nothing replaces the brief" line truncated — and Step 6's check caught it and capped the verdict. The command records the exact block it prints — findings included, keyed per findings digest — so a launch that drops or rewrites the findings matches no record. In worktree mode the verifier's `working_dir` is the PR worktree (same rule as Step 3), so its reads and re-checks resolve against the PR's code. -The brief holds the method the orchestrator used to spell out here and that a paraphrase kept dropping: trace the failure scenario through the real code rather than voting on the finding's prose; engage the diff's own documented intent before calling a documented change a regression (the rule a run skipped when it auto-posted a false "leaks tokens" Critical); and the one-way, quote-the-contradiction bar on **rejecting a Critical**. Read the brief to know what a verdict means; do not re-derive it here. +The brief holds the method the orchestrator used to spell out here and that a paraphrase kept dropping: trace the failure scenario through the real code rather than voting on the finding's prose; engage the diff's own documented intent before calling a documented change a regression (the rule a run skipped when it auto-posted a false "leaks tokens" Critical); the one-way, quote-the-contradiction bar on **rejecting a Critical**; and — when a finding's claim is **runnable** and the repo has a fast unit harness (`vitest`/`jest`/`pytest`) — the option to **write and run a probe** and let the observed behaviour, not a re-reading, settle the verdict. That last one earns its place: measured on this repo, the strongest model traced a real double-execute (`!git push` firing twice) and called it correct; a probe that runs the path reports `sendShellCommand called twice` and the guessing stops. The brief makes the probe evidence rather than theatre with two hard rules — a mandatory self-check that the probe **flips** between buggy and correct, and leaving the tree exactly as found (no probe file, no fix edit, reaches the diff or build). A finding a probe confirmed carries `Source: [probe]`, which `compose-review` treats as deterministic (a run produced it), exactly like `[build]`/`[test]`. Read the brief to know what a verdict means; do not re-derive it here. **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.