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.