0075 — Upgrade the AI code-review pipeline: models, rendered evidence, merge discipline, and measurement

Status: Accepted
Date: 2026-07-01
Deciders: Fede

Context

The goal driving this ADR: increase shipped quality — fewer bugs (especially UI bugs), more component reuse and consistency, on-brand customer-facing copy, and testing depth (unit tests are never sufficient; prefer the E2E harnesses). The trigger question was "should we bump the review bot's model?", which turned into a full audit of the review pipeline (2026-07-01): CI/CD inspection, a 12-case study of bugs that shipped past review, a reviewer-prompt-vs-standards gap analysis, Trello bug-corpus mining (109 bug cards in 60 days), team-meeting transcript mining, and research on current claude-code-action capabilities.

Findings (each verified during the audit)

The bot was effectively Sonnet-only and a generation behind. The size-tier table pinned claude-sonnet-4-6 for tiny/medium/large and claude-opus-4-7 for huge/mega — but every one of the last 30 PR reviews ran on Sonnet 4.6 (no PR reached an Opus tier). Turn budgets had large headroom (large tier: ~18–30 of 100 turns used).
Most shipped bugs are invisible to a diff-only reviewer. Of 12 traced shipped bugs (fix-up PR → origin PR → what the bot said), 9 required a rendered page, a click-through, or realistic prod-shaped data to detect (stale statuses, blank sections, nav traps, wizard spinners, mislabeled channels). The dominant Trello/UI classes match: stale state after backend events, rendering regressions with real data, jargon/label wrongness leaking to customers, hidden/broken controls.
The bot catches real bugs that get merged anyway. In 2 of the 12 cases the bot named the exact bug, file, line, and fix (PR #2572's ← Back to ProspectInquiry label; PR #2793's stale urgent flag) — and the PR merged with the finding open. Root cause: the review check was not in main's required status checks (verified: only Build / Type Check / Unit Tests / Vercel; the active ruleset requires 0 approvals). The entire verdict-encoder/override machinery was advisory.
Lint was enforced nowhere. No CI lint job existed; the review bot's "mandatory lint verification" runs in a checkout without node_modules (env-only failure, waved through). Running lint for real surfaced 1,164 pre-existing errors and one genuine syntax error in scripts/ (outside tsconfig, so tsc never saw it).
Verdict-marker drift. None of the last 30 review summaries carried the  marker (the bot rides the action's track_progress comment) — the verdict encoder has been living on its fragile last-##-comment fallback, the exact failure class of the #962/#967 incident.
Reviewer prompt gaps vs. our own standards (from the gap analysis): stale cross-references ("universal lens 6" → actual Lens 8; arsenal registry path pointing at a nonexistent file), Lens 26 requiring true regression tests only for classification bugs, e2e/** matching no lens-router row (the harness-first lens never fired on the E2E dir), only 11 of 89 ADRs cited anywhere in the corpus (ADR-0047/0052/0071 missing despite incident-grade importance), no DetailPageShell enforcement, and severity inconsistency (the same raw-<select> violation was 🔴 in one PR and a nit in another).
Platform facts (verified against docs, 2026-07-01): the bot runs on a dedicated Claude subscription (OAuth token + fallback token); Opus 4.8 is plan-included and selectable in CI; Sonnet 5 exists (claude-sonnet-5, the API's current sonnet alias); --effort {low..max} flows through claude_args; Fable 5 switches from plan-quota to paid usage credits after 2026-07-07 and runs cyber-domain safety classifiers that can refuse security-adjacent review content.

Decision

Seven decisions, first three already landed as PRs on 2026-07-01:

D1 — Model bump (PR #2832). Tier table: tiny → claude-sonnet-5; medium → claude-opus-4-8 (effort high); large/huge/mega → claude-opus-4-8 (effort xhigh). claude.yml (@claude re-review/work): claude-opus-4-8, effort xhigh. Turn budgets unchanged (headroom verified). Rationale: Opus 4.8 is specifically improved at code review / real-bug finding; the dedicated subscription makes per-token cost a non-issue.

D2 — Blocking lint via ratchet (PR #2836). New Lint job in ci.yml wired into the gate job (enforced through the existing required Build & Test check). Pre-existing violations captured in an ESLint 9 native bulk-suppressions baseline (eslint-suppressions.json); only NEW errors block. .next.backup/** added to eslint ignores (local OOM fix).

D3 — Reviewer-prompt hardening (PR #2837). Rendered-evidence policy (§J in the UI lens: UI-touching PRs must include screenshots/recording incl. one unhappy state; reviewer judges the pixels against brand/copy/loading rules); Lens 26 strengthened (every bugfix ships a regression test that fails pre-fix/passes post-fix; rendered-only failures may use a Playwright probe or documented preview repro); stale-reference fixes; e2e/** router row; ADR-0047/0052/0071 enforcement; DetailPageShell as canonical detail layout; marker-drift fix; environment-honesty rule for the bot's tsc/lint claims.

D4 — Make the bot's verdict a real merge gate. Add review to main's required status checks. The belt-and-suspenders infrastructure (fallback token, override workflow, admin merge) already mitigates bot-outage risk. This closes the "caught it, merged anyway" class — the single most fixable gap found.

D5 — Preview-based UI verification (follow-up workstream). The structural fix for finding #2: every PR already has a Vercel preview deploy; the bot reads diffs. Rollout: (a) deterministic screenshot step for UI-touching PRs — resolve preview URL (the await-preview job already does), sign in as the smoke user, screenshot routes mapped from changed files, capture console/hydration errors, post to the PR; (b) feed screenshots to the reviewer + allowlist a preview-shot script so it can adaptively capture suspicious states; (c) Playwright interaction probes against the preview for the top surfaces (renewal detail, prospect conversation, tours, work orders, turnovers) — grown by turning every shipped UI bug into a preview probe (the regression-test rule, extended to UI). Author-attached screenshots (D3's §J) are the interim layer, not the end state: self-reported evidence is happy-path-biased.

D6 — Measure review quality with a replay eval, not vibes. Before/after evidence for this (and any future) reviewer change:

Golden-set replay: the audit's 12 origin-PRs-with-known-bugs (plus Trello-mined additions) become a benchmark corpus. Re-run the reviewer prompt offline (claude -p with reviewer-core.md + the historical PR diff) under old vs. new config, ≥3 trials each (reviews are stochastic); measure recall on the known bugs and precision via judge + human spot-check of all findings. A reviewer config change counts as an improvement only if golden recall rises without a noise regression.
Standing scorecard (lagging indicators): fix-up-PR rate per 100 merged PRs, Trello bug-card inflow/week (baseline: 109/60d), % of fix-up PRs whose bug the bot had already flagged (merge-discipline metric — target ~0 after D4), marker-presence rate in summaries.
Corpus and harness live under evals/review-bot/ (follow-up task).

D7 — ADR-sync cadence. The lens corpus drifts from the ADR corpus (~20 ADRs shipped in June alone; 11/89 cited). Monthly: list ADRs newer than the reviewer corpus's last touch and fold the enforceable ones into lenses (candidate for a small CI reminder script).

Consequences

Watch the subscription's Opus-specific usage sub-limit for the first week of D1; the fallback token covers limit-hit runs, and rollback is reverting model strings.
D2's suppressions baseline is a ratchet: touching a file with suppressed violations can surface them (fix-on-touch); --prune-suppressions shrinks the baseline over time. The baseline file must not grow — a PR adding suppressions is a red flag.
D4 makes bot outages merge-blocking; mitigations exist (fallback token, claude-review-check-override.yml, admin merge). Also finally makes PR #1008's encoder-fix worth landing.
D5 adds ~2–4 min to UI-touching PRs and requires smoke-user creds (plus a Vercel protection-bypass token if previews are protected) in the screenshot job — scoped, not org-wide, secrets.
The reviewer prompt grows (~1.5 KB from D3); at ~54 KB the core doc is already heavy — the next prompt change should consolidate (fold Lens 30 into the numbered sequence) rather than append.
Fable 5 is deliberately NOT used for reviews (cost model changes 7/7; cyber-classifier refusals on security-adjacent diffs would fall back mid-review). Revisit if either constraint changes.

Alternatives considered

Fable 5 for huge/mega reviews. Rejected for now: post-2026-07-07 it bills usage credits rather than plan quota, and its safety classifiers can refuse security-adjacent review content mid-run (falling back to Opus anyway). Opus 4.8 at xhigh captures most of the gain at zero marginal cost.
Anthropic's managed "Code Review" product (claude.ai-configured, multi-agent + verification pass, REVIEW.md customization). Rejected: requires Team/Enterprise plan (we run Max subscriptions), bills $15–25/review via usage credits, and our 54 KB lens corpus with repo-specific incident memory is the moat the managed product can't ingest. Revisit on a plan change.
Author-attached screenshots as the whole answer. Rejected as sole mechanism (self-reported, happy-path-biased, static) — kept as the D3 interim layer under D5.
Full-repo lint cleanup before gating. Rejected: 1,164 errors is a multi-day cleanup that blocks the win; the suppressions ratchet gets the gate today and cleans incrementally.
Re-review on every push (synchronize trigger). ~~Deferred~~ — ADOPTED 2026-08-21. The original deferral read: "doubles review volume for mostly-unchanged diffs; @claude review covers the re-review path, and D4 prevents merging over stale findings. Reconsider if post-review pushes regularly introduce regressions (the PR #2777 rebase-clobber case) — a cheap middle ground would be re-review only when the prior verdict wasn't 🟢."

The reconsideration trigger fired, though not in the shape it predicted. The problem was not regressions slipping through — it was that @claude review did not, in fact, cover the re-review path, because it required a human to remember. Measured 2026-08-18..2026-08-21: review-verdict-watchdog.yml paged #alerts 48 times over 35 PRs, and 45 of those were a fix-push that never got a run on its new head (36 outright; 9 more on branches later rebased, 8 of which also had zero runs at that SHA). Meanwhile auto-merge-all.yml disarms on every synchronize and re-arms only on a verdict pinned to the new head (2026-07-28, #4749/#4751), so those PRs could not merge unattended either. The stall was 45–78 minutes, ended by a hand re-fire that ran the review anyway — 146 workflow_dispatch re-fires since 2026-08-14.

"Doubles review volume for mostly-unchanged diffs" was also answered in the interim by the incremental re-review detector (2026-08-08), which reviews only last-pinned-sha..HEAD and sizes the tier on that delta; its own comment anticipated "a synchronize trigger if one is ever added". Cost is bounded by a ~90s settle wait gated on synchronize, sitting inside the existing per-PR cancel-in-progress concurrency group, so a push burst cancels its own older runs mid-sleep (≈1 runner-minute each, zero LLM tokens) and only the surviving head is reviewed.

This sentence is history, not current mechanism (superseded 2026-09-13). The cancellation half was removed: it held only for bursts tighter than the 90s sleep, and at the cadence agent lanes actually push at it cancelled reviews 2–7 minutes in, leaving the PR with the oldest verdict. The group is now cancel-in-progress: false plus a supersede check. The authoritative description is the header comment in .github/workflows/claude-code-review.yml; the correction is in docs/planning/ci-review-recheck.md §A.

The suggested middle ground — re-review only when the prior verdict wasn't 🟢 — was evaluated and rejected: a gated run still enters the concurrency group, still cancels an in-flight round-1 review, then skips, leaving the PR with no verdict at all. (That specific objection was an artefact of cancel-in-progress: true and no longer holds; the refinement is still not wanted.) Full detail and the classification table: docs/planning/ci-review-recheck.md §A. Pinned by src/__tests__/review-synchronize-trigger.drift.test.ts.
agent-approval-check (new official action requiring N human approvals on agent-authored PRs). Not adopted: contradicts the auto-merge-on-green workflow that the team explicitly runs. Noted for the future if human-in-the-loop review returns.

References

PRs: #2832 (D1), #2836 (D2), #2837 (D3)
Audit evidence: 12-case miss study (fix-up→origin tracing, 2026-07-01 session), reviewer-prompt gap analysis, Trello bug corpus (109 cards / 60 days), claude-code-action capability research (code.claude.com docs + repo, 2026-07-01)
Related ADRs: ADR-0047, ADR-0052, ADR-0060, ADR-0071