Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions packages/cli/src/commands/review/agent-prompt.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
15 changes: 15 additions & 0 deletions packages/cli/src/commands/review/compose-review.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
11 changes: 6 additions & 5 deletions packages/cli/src/commands/review/compose-review.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
7 changes: 7 additions & 0 deletions packages/cli/src/commands/review/lib/agent-briefs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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).
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/skills/bundled/review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <k>` 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.

Expand Down
Loading