ci: split Opus review into discovery and validation passes

[CrysisDeu]

Summary

The Opus review lane almost never says anything: 2.7% blocking rate vs GPT's 18% across 73 PRs, and 63 of 73 posted comments contained literally **No findings.**.

A controlled experiment on this repo found the cause is neither the model nor the call architecture — it is three clauses in the prompt. Each one, added on its own to an otherwise bar-free prompt, silenced a real defect that the same model reports 3/3 times without it.

This PR moves precision enforcement downstream of discovery, which is what Anthropic's own code-review plugin does (parallel discovery agents → a per-candidate validation agent with a confidence floor).

What the experiment found

Ground truth: PR #2169 deleted three except BaseException: temp-file cleanup handlers and replaced them with a helper that only unwinds on except Exception, so a Ctrl-C during fsync orphans a full copy of the deploy store. GPT blocked on it. Opus said nothing.

arm	prompt	result
A	current production prompt	0/3 found it
B	same prompt, split into two independent calls	0/3 — discovery emitted zero candidates
C	identical model / diff / runner, contract removed	3/3 found it, matching GPT's file:line set exactly
ADD-PLUMBING	C + the inert plumbing blocks (inert control)	3/3 ✅ harness valid

Single-clause ablation, n=3 each, isolated the three suppressors: the closed-list reading of the residual defect classes, the certainty threshold, and "drop the finding if the fix touches untouched code". No single clause is necessary — removing any one from the full prompt left it silent, so the suppression is over-determined.

Arm B is the important negative result: splitting the call is not the fix. Keeping those clauses in the discovery half produced nothing for the filter to keep.

Industry evidence points the same way — arXiv:2603.18740 measures a 16–93pp recall loss from "bug-free" framing with only a 0.8pp FP increase, Greptile could not reduce nits by prompt without also losing critical comments, and Cursor moved Bugbot to aggressive discovery prompts with category filtering and FP validation as post-generation steps.

What changes

• Stage 1 (discovery) — generous recall, no precision gates. Output is never posted and gates nothing.
• Stage 2 (validation) — an independent call. Re-derives input / call path / observable outcome for every candidate from code it opens itself, verifies the candidate's quoted evidence actually appears, keeps only what it scores ≥ 80, and only then applies the closed blocking list. It may not add findings of its own. Keeps id: review, so the existing transcript capture, comment upsert and fail-closed gate are unchanged.
• Prompts move out of the YAML into .github/review-prompts/ and are now shared by the same-repo and fork lanes, which previously carried near-duplicate copies (that duplication is why a single-file fix would have left fork PRs on the suppressive prompt). They are materialised from the BASE commit, so a PR can neither weaken the rules that govern it nor rewrite the prompt that reviews it. A missing prompt fails the job rather than degrading into an unspecified review that could look clean.
• Candidates cross the stage boundary as a workspace file, never string interpolation — model output must not reach YAML or a shell argument, and a file has no arg-length ceiling.
• The fix-scope rule changes behaviour instead of being deleted: a finding whose only remedy lies outside the changed lines is reported as advisory rather than dropped — the author cannot land the remedy here, but the signal is real. A regression the diff itself introduces still blocks, since reverting the hunk is an in-diff fix.

Job name (Opus 5 Review), required check, tool surface and the [OPUS-REVIEWED] / [BLOCK-MERGE] marker contract are unchanged. The fork lane keeps its no-shell posture.

Verification

Ran these exact prompt files, both stages, against the same corpus.

Negative controls — PRs where both production lanes reviewed the same SHA and both emitted zero findings. This is the question that decides whether the lane can ship: does loosening discovery make the gate noisy?

candidates in (8 runs, 4 PRs × n=2)	33
BLOCKING out	0
advisory out	7
removed by the validator	79%

No run would have turned the required check red on a PR two reviewers already passed.

Positive controls — a GPT blocking the old Opus lane missed:

PR	GPT	this pipeline
#2109	blocking	BLOCKING, same guard GPT named (tailnet_serve.py:396), 2/2
#2169	blocking	reported as advisory, 4/4 — where the old lane said nothing
#2152	blocking	GPT's finding was generated by discovery but dropped by the validator

One post-fix #2152 run blocked instead on a different, rule-backed defect that neither production lane caught at that SHA: the new PTY integration tests spawn the operator's real login shell in their real $HOME and leave printf 'AKIAIOSFODNN7EXAMPLE\n' in their real shell history — no-test-side-effects (blocking: true, test/**/*.py).

Tests

Three tests asserted on prompt text that now lives in the prompt files. They follow the prompt to its new home rather than being deleted. TestOpusTwoStageArchitecture (7 tests) locks the split in place — most usefully a cross-check that the marker the gate greps equals the marker the validation prompt is told to emit, because a typo on either side fails every PR closed and silently. Prose assertions run through a whitespace-collapsing helper so re-wrapping a paragraph cannot break them.

40 passed; flake8 / isort / mypy clean.

Known limits — please read before approving

• Latency is untested. The two stages are sequential, so end-to-end roughly doubles under the same 90-minute runaway backstop. This PR's own CI run is the first real measurement.
• Advisory volume goes up, from ~0 to ~1 per PR on the negative controls.
• The 7 surviving advisories are unadjudicated. The production lanes' silence is not ground truth, so each is either a real defect both lanes missed or a false positive; only reading them decides which. Two of them reproduced across independent replicates (sessionWatch.ts:256, DevFleetPage.tsx:415), which argues against hallucination but does not settle it.
• #2169 lands advisory, not blocking, in 4/4 runs. The validator's reading is defensible — the guard was removed but a narrower compensating replacement exists, so it is not the "removed guard with no compensating replacement" class — but blocking parity with GPT is not achieved for that case.

Experiment artifacts (23 ablation variants, 14 pipeline runs, prompts, per-run outputs) are on the dev desk under opus-lane-exp/, not in this repo.

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of d2691a906bab39006298b97b7ad393844ba12bf9 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d2691a9

False positive or not applicable? A repository writer can comment:
/ai-review override gpt d2691a906bab39006298b97b7ad393844ba12bf9: <one-sentence reason>

[CrysisDeu]

Blocked on #2333 — and the block is the design working

First CI run failed in 21s, exactly where it should:

##[error].github/review-prompts/opus-discovery.md is missing or empty on the
base commit (300d244b). Refusing to review against an unspecified contract.
Opus 5 Review — failed in 21s

The extraction step reads the prompts from the base commit so a PR cannot rewrite the prompt that reviews it. This PR introduces those prompts, so on its own base they do not exist — the step refuses, and the fail-closed gate turns the check red rather than reviewing against nothing. Both halves behaved correctly; the lane simply cannot bootstrap itself.

I did not add a fallback to the PR-head copy. That would mean any time these files were absent from main, a pull request could supply the prompt used to review it — the exact hole the base-ref read closes. A permanent security property is not worth one merge's convenience.

So this splits in two:

• ci: add the Opus review prompt files (bootstrap half) #2333 — the two prompt files alone, dormant, nothing reads them. Merge first.
• this PR — the workflow rewrite plus its tests. Rebase onto main after ci: add the Opus review prompt files (bootstrap half) #2333 lands, and the review runs.

Converting to draft until #2333 is in.

Also pushed 558f2dfa, which documents the operational consequence of the base-ref read in the workflow header: a PR that edits a prompt is still reviewed by the old prompt. The edit takes effect only once it is on the default branch. Inherent to the property, but it will bite whoever tunes a prompt next if it is not written down.

The latency measurement this PR was opened to get is therefore still outstanding — it needs a run that actually reaches both stages.

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of d2691a906bab39006298b97b7ad393844ba12bf9 — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound, evidence-driven split; the risk left is operational — a doubled worst-case runtime under an unchanged fail-closed 90-minute ceiling.

Watch

• The backstop comment still claims a healthy review "self-terminates at --max-turns 120 per stage … long before" the 90-minute timeout, but that headroom was calibrated for one stage. Two sequential 120-turn Opus calls on a large diff can plausibly exceed 90 minutes → the runaway ceiling kills the job → the required check goes red on a healthy PR with no remedy except splitting it. The description concedes "latency is untested" yet ships the old ceiling; raise it (or cap stage-1 turns lower) rather than letting this PR's own CI run be the calibration.
• The 200KB candidate cap plus an explicitly uncapped, generous-recall discovery means a large-but-clean mechanical PR can fail the required check on discovery verbosity alone. Fail-over-truncate is the right call, but watch the first big refactor PR through this lane.

Suggestions

• The motivating defect (refactor(core): migrate five inline atomic writes onto the shared helper #2169) still doesn't block because the validator reads any replacement as "compensating." One line in opus-validate.md's blocking class 2 — a strictly narrower replacement is not compensating — would close the exact loophole the experiment documented.

[DESIGN-REVIEWED] d2691a9

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision d2691a906bab39006298b97b7ad393844ba12bf9 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[CrysisDeu]

Rebased onto main now that #2333 has landed (53553eed), so the prompts exist on the base commit and the extraction step can find them. The prompt files collapsed to a no-op in this PR's diff — #2333 owns them — leaving only the two workflows and the tests.

Out of draft. This run is the first one that should actually reach both stages, so it is also the first real measurement of the end-to-end latency question this PR was opened to answer.

[CrysisDeu]

Prior reviewed SHA: 7a8a740d. Rebased onto edba4af5 and the blocking finding is fixed.

Candidate truncation made the gate fail open: fixed. This was the third fail-open of the same shape in this pipeline, after the empty-candidate list and the symlinked scratch paths. A candidate emitted past 24,000 bytes was dropped with only a ::warning::, validation could not report what it never received, and the gate passed on a clean [OPUS-REVIEWED] verdict.

Fixed in two parts rather than only failing at the old cap:

• Cap raised to 200,000 bytes. The 24,000 figure was inherited from the GPT lane, where candidates ride in a command-line argument and MAX_ARG_STRLEN is the binding constraint. Here they cross the stage boundary as a file, so the validator's context is the only real limit — 24 KB left most of it unused while making the cliff easy to hit. The GPT lanes keep their own cap; theirs is load-bearing.
• Fail closed when even that is exceeded. A diff producing 200 KB of candidates is not something one validation pass can judge honestly, so the honest outcome is a red check saying so, not a verdict computed from a prefix.

test_an_oversized_candidate_list_fails_closed locks it: no truncation path, ::error:: plus a nonzero exit on the over-cap branch, and the cap value itself.

Also absorbed from main: #2339 switched this lane's model to opus-4.8 "for faster reviews" and #2342 renamed the check to Opus 4.8 Review. Those touched the same lines as the two-stage split, so the rebase had four conflict hunks whose upstream side was purely the rename. Resolved by keeping the architecture and adopting main's naming and model everywhere — the check name is what branch protection keys on, and the model choice is the repo's current call, not mine to revert in a rebase.

One consequence a reviewer should weigh, because it is not covered by this PR's evidence: every measurement in the description was taken on opus-5. The zero-blocking-on-negative-controls result, the 79% candidate filtering, and the ground-truth retention were all measured before #2339 landed. On opus-4.8 none of it is measured, and a weaker model plausibly moves it in the direction this reform exists to fix. The harness that produced those numbers is still on the dev desk, so re-running the corpus against 4.8 is cheap if that matters before merge.

[CrysisDeu]

Prior reviewed SHA: 4567118aa. Blocking finding fixed in e61f0b8b5.

Discovery could poison the validation workspace: fixed. The grant was Bash(gh pr diff:*), and that permission matches by command prefix — so gh pr diff <n> > .review-prompts/opus-validate.md sits inside it. The diff under review is PR-authored content, so a directive embedded in it that induces a redirect lets discovery overwrite the validation contract, or the candidate file, in the shared workspace before stage 2 reads either. Stage 2 would then render its verdict against attacker-supplied instructions.

Reading the prompts from the base ref closes the "a PR rewrites its own reviewer" path through git. This closes the same path through the shell — and it was an asymmetry this PR introduced, since the fork lane has always prefetched its diff and run with no Bash at all.

Fixed exactly as suggested, matching the fork lane:

• A new Prefetch the reviewable diff (data only) step runs git diff --no-color "$BASE_SHA...$HEAD_SHA" before either agentic step, writing to ${{ runner.temp }}/pr.diff — outside the workspace, so nothing the PR tracks can shadow the path, and the tree is never modified. An empty result exits nonzero rather than reviewing nothing.
• Both stages drop to --allowedTools "Read,Grep,Glob". No shell in the reviewer at all.
• Both shims point at the prefetched file instead of naming a command.

Two existing tests asserted the old posture; they follow it rather than being deleted. test_reviewer_is_code_only_and_cannot_fetch_pr_prose now asserts Bash is absent from both --allowedTools lines, and the renamed test_the_diff_is_prefetched_not_fetched_by_the_agent asserts the prefetch step exists, precedes discovery, writes outside the workspace, and fails closed on an empty diff. 43 tests pass; flake8 / isort / mypy clean.

For the record, since it is a pattern rather than one bug: this is the fourth finding on this PR and the fourth of the same shape — a way for the two-stage lane to emit a clean verdict without having actually reviewed. The earlier three were an empty candidate list, symlinked scratch paths, and silent candidate truncation. Splitting a reviewer into two stages creates a handoff, and every artifact crossing that handoff is attack surface the single-call design did not have. Worth weighing against the recall gain when reviewing this PR.

[CrysisDeu]

Rebased onto 7304bb0e, and this now overlaps #2379 — here is the measurement I used to resolve it

#2379 landed while this PR was open and rewrote the same lane in the opposite
direction: the review contract moved back inline into the workflow YAML, the two
stages collapsed into one call with internal PHASE A discover / PHASE B falsify
phases, and the reviewer gained the PR title/description as an input. Six conflict
hunks, all the same collision.

Rather than resolve it by preference, I measured the three candidate designs against
the same corpus, model, worktrees and runner — only the contract varied.

arm	design	named the known defect	labelled it BLOCKING	false-positive BLOCKING	advisory
A	#2379 as merged (one call, inline contract, PR intent)	2/6	0	0/8	5
B	this PR (two calls, prompt files from base ref)	4/5	2	0/8	8
C	hybrid: #2379's contract split into two independent calls	4/6	0	0/8	19

Corpus: 3 PRs carrying a defect the GPT lane caught and the old Opus lane missed
(recall), 4 PRs where both production lanes emitted zero findings at that SHA
(false positives). n=2 per cell, 69/70 cells completed.

Arm B is the only design that produces a blocking verdict. One example, verbatim:

**BLOCKING — src/kiro_crew/dashboard/handlers/terminal.py:309** — a JSON
null/0/"" in redact_output coerces to False, silently turning off live PTY
credential redaction — the falsy-value fail-open class backend-security-controls
(blocking) names.

Two findings worth stating because they cut against my own prior claims:

• The effective variable is the contract, not the call structure. B and C both run
  two independent calls; only B blocks. Splitting the call is not what recovers a
  blocking verdict — the validation contract's wording is.
• "Demote instead of drop" backfired. Arm C is my own idea (an out-of-scope fix
  demotes to advisory rather than being dropped) and it became a pressure valve: 19
  advisories, zero blocking. Reporting went up, gating went to nothing.

I also retract something I asserted earlier in this PR's history: I claimed the
opus-5 → opus-4.8 switch in #2339 invalidated this PR's original evidence. Arm B
still blocks, so that claim was unfounded.

What this resolution keeps from #2379, and what it supersedes

Kept — #2379's five contract guarantees are all still enforced, retargeted at the
prompt files where the contract now lives (TestClaudeReviewQualityDimensions):
consequence-chain bar, advisory-only quality dimensions, the authoritative
blocking: true flag, terse output, and the mechanical/semantic division of labour.
Four already existed in the prompts under different wording; the fifth was the
BLOCKING cap, and I adopted #2379's 5 over this branch's 2 — a strict relaxation
that cannot change any measured outcome, since arm B emitted at most 1 per review.

Superseded — the Fetch PR intent step and its two tests. Arm B was measured
without author prose reaching the reviewer, so carrying the intent step here would
ship an untested combination, which is precisely what arm C was. Re-introducing PR
intent (and the sage quality dimensions) on top of arm B is a follow-up that deserves
its own measurement, not a freebie folded into a conflict resolution.

What I am not deciding

Whether the contract belongs in files or inline is a maintainer call, and #2379 is the
later, human-reviewed decision. I have brought data and a working diff; I am not
claiming authority to reverse that. If the intent is that the lane stays inline, say so
and I will close this and re-propose the two prompt-file guarantees separately.

Caveats, so the table is not read as stronger than it is

• n=2 per cell, 5–6 observations per arm. 2 versus 0 is suggestive, not statistically
  significant.
• The model was not pinned — spawn run exposes no --model and no model field is
  recorded — so this compares the designs under one model, not production's
  opus-4.8 specifically.
• One cell (2152 arm B rep 1) never completed after three attempts, so arm B is n=5
  against the others' n=6.
• PR titles/descriptions were fetched today and may have been edited post-merge. That
  favours arms A and C, which are the arms that lost.

Harness and all 69 outputs are reproducible from gen_compare.py / grade_compare.py.

Also worth flagging independently of this PR: .github/review-prompts/*.md are
currently dead files on main — nothing references them since #2379 inlined the
contract. Whichever direction wins, someone should either delete them or wire them
back up.

[CrysisDeu]

Round on 9ea15ea5 → pushed d2691a90

Opus 4.8 Review passed. Four checks were red; two were mine and are fixed, two are not
caused by this diff and I have not touched them.

Fixed — Backend Tests (3.10, 3) and (3.12, 3)

Both failures were in test/test_prepare_pr_profiles.py, a file this branch does not
otherwise change. That test keeps the prepare-pr skill's local reviewer in sync with
what CI actually runs, and it reads two facts straight out of claude-review.yml — the
model and the finding budgets. Correct while the whole contract lived inline there; this
branch moves the contract into .github/review-prompts/*.md and splits the lane into two
calls, so both reads landed in the wrong place.

• AssertionError: expected one --model arg, got ['us.anthropic.claude-opus-4-8', 'us.anthropic.claude-opus-4-8'] — two stages, so two --model flags. The guarantee is
  "the local reviewer mirrors CI's model", so the assertion now requires every stage to
  agree and compares that one value against the profile. That is strictly stronger than
  before: it also catches the two stages drifting apart from each other, which the old
  single-value assertion could not express.
• AssertionError: no BUDGET line in claude-review.yml — the budgets moved into
  opus-validate.md, which is the contract that actually applies them. _budget() now
  reads the Opus caps from there and the GPT cap from codex-review.yml, matching either
  wording, since the two lanes own their own contracts and phrase the cap differently.

While doing that I found this branch had carried only one of the two caps #2379
introduced. Added the advisory one, so both survive: At most 5 BLOCKING per review +
At most 6 advisory FINDINGs per review. A cap only ever trims output once exceeded, and
the measured runs peaked at 1 BLOCKING and 5 advisories, so neither can change a measured
outcome — this is about not silently dropping a guarantee, not about behaviour.

68 tests pass locally (test_prepare_pr_profiles.py + test_ai_review_workflows.py),
flake8 / isort / mypy clean.

Not caused by this diff — Frontend Tests

src/i18n/unitLiterals.test.ts > holds at most 74 un-migrated number+unit literal(s).
That ratchet walks website/src only, and this branch's website/src tree is
byte-identical to its base — git rev-parse HEAD:website/src and
7304bb0e:website/src are both a7b3313cbfaa8b7ebeb6580b50b11a0dfbb22e9b, and
git diff --name-only 7304bb0e..HEAD -- website/ is empty. A gate whose entire input is
unchanged cannot have been pushed over its baseline by this diff. The same job passed on
7304bb0e itself 24 minutes earlier, and the log carries a wall of
ECONNREFUSED 127.0.0.1:3000 and JavaScript file loading is disabled noise, so this
reads as environmental. Left alone rather than edited: raising a shared upward-only
baseline to quiet a red on an unrelated PR is exactly what that constant's own comment
forbids.

Not caused by this diff — Backend Tests (Windows) (3)

Already failing on 7304bb0e, this branch's base, in run
31368012454 where it
was the only failing job. Inherited debt. It will likely stay red after this push even
with my two shard-3 fixes in, and that residue is main's, not this PR's.

Coverage Gate is a cascade — it fails closed when an upstream coverage job did not
succeed — so it should clear once the backend shards are green.