Optimizing review flow

[fiddur]

Cheaper, faster preparation without a weaker review

The inbox prepares every review-requested pull request with one Claude Code run. Measured over the
22 runs that did real work (session logs under ~/.claude/projects/-home-fiddur--diffity-inbox-worktrees-*,
bundles and store under ~/.diffity/inbox):

Measure	Value
Wall time per prepare	avg 8.7 min, max 25
Tool calls per prepare	avg 37
Time split	80% model (thinking, writing), 20% tools
Output tokens per prepare	avg 27k (8k–61k)
Cache reads per prepare	avg 1.1M; ~76k context per turn; 27k on the first turn
Local pnpm typecheck / pnpm test	20 of 22 runs, 13 min in total, 191 s in one run
Findings in 18 bundles	P1 in 6, P1 or P2 in 12, P3-only or none in 6
Wasted attempts	3 PRs × 3 attempts ended on "You've hit your session limit · resets 2pm" and sit failed

Two of the six P1s came from tiny diffs (#14550 at +11/−11, a stale snapshot baseline; #13444 at +5/−1),
so nothing below picks a cheaper path by diff size.

The logs also show two things the daemon should not be doing:

• The agent gets the reviewer's MCP servers. In #14188 it called gcloud (read-only, harmless) with the
  reviewer's credentials inside a checkout the author controls; the prod datastore, Slack, Atlassian and
  Mixpanel servers were in its tool list. The daemon strips GitHub tokens from the environment, but MCP
  servers come from the reviewer's Claude settings.
• The agent runs the repository's toolchain (pnpm typecheck, pnpm test) in nearly every run. CI
  has already done that, and the result is one field away on a gh call the daemon already makes.

It also loads the reviewer's own ~/.claude/CLAUDE.md, memory index and skills into every run; in #14532
it wrote a memory file.

Verified on this machine before planning: claude -p --output-format json returns total_cost_usd,
duration_ms, num_turns, usage and per-model modelUsage; --setting-sources "" loads no user
settings, no MCP servers, no user skills and no user CLAUDE.md; --setting-sources user keeps MCP servers
(including the claude.ai connectors); a PreToolUse hook passed through --settings '<json>' with matcher
mcp__.* blocks a tool by exiting 2 even under --dangerously-skip-permissions (gcloud denied, Atlassian
allowed in the test); --append-system-prompt takes the 20 KB review skill body; --tools restricts
built-ins only and does not touch MCP; --strict-mcp-config removes the claude.ai connectors too, so it is
not the way to keep a few of them.

Wave 1

1. The daemon builds the agent command; agent replaces prepare

Config gains an agent block and refuses the old prepare key with a message naming the replacement
(prepare was the raw argv; it cannot carry the safety flags below, and a copied default would silently
keep the old behaviour):

"agent": {
  "model": null,        // --model for the drafting pass; null = Claude Code's default
  "effort": null,       // --effort low|medium|high|xhigh|max
  "mcpAllow": [],       // exact MCP tool names the agent may call; [] = no MCP servers at all
  "extraArgs": [],      // appended verbatim to the built argv
  "maxBudgetUsd": null  // --max-budget-usd per run
}

agent-argv.ts builds, for the drafting pass:

claude -p --dangerously-skip-permissions --output-format json
  --setting-sources ""                      # mcpAllow empty: no user settings, memory, skills or MCP
  --setting-sources user                    # mcpAllow set: MCP servers come from user settings
  --settings '{"hooks":{"PreToolUse":[{"matcher":"mcp__.*","hooks":[{"type":"command","command":"<node> <entry> inbox mcp-gate"}]}]}}'
                                            # mcpAllow set only; the gate reads DIFFITY_MCP_ALLOW from its env
  --append-system-prompt "<diffity-review SKILL.md body, frontmatter stripped, from <dist>/skills>"
  --disallowedTools 'Bash(gh pr review:*)' 'Bash(gh pr comment:*)' 'Bash(gh pr merge:*)' 'Bash(gh api:*)'
                    'Bash(pnpm:*)' 'Bash(npm:*)' 'Bash(npx:*)' 'Bash(yarn:*)' 'Bash(bun:*)' 'Bash(make:*)'
  [--model X] [--effort Y] [--max-budget-usd Z] [extraArgs...]

• inbox mcp-gate is a hidden subcommand: reads the hook JSON on stdin, exits 0 for a tool named in
  DIFFITY_MCP_ALLOW (comma-separated, set by agentEnv), otherwise prints "the review agent may only use
  " to stderr and exits 2. Unit-tested on stdin fixtures.
• Shipping the skill in the system prompt removes the dependency on the reviewer's ~/.claude/skills. The
  prompt's "follow the diffity-review skill" becomes "follow the review instructions in your system prompt".
• With mcpAllow set the prompt adds: "You may use these tools to read material the pull request refers to
  (a ticket, a document, a thread): . Nothing else outside this checkout." Reading is the intent;
  the allowlist is exact tool names, so a write tool is only there if the reviewer put it there.
  Suggested starting set for NC: mcp__claude_ai_Atlassian__getJiraIssue,
  mcp__claude_ai_Atlassian__getConfluencePage, mcp__claude_ai_Slack__slack_read_thread,
  mcp__claude_ai_Google_Drive__read_file_content.
• runAgent output goes through parseAgentOutput(stdout) → { text, stats | null }: an object with
  type: "result" yields text = result and stats from total_cost_usd, duration_ms, num_turns,
  usage, modelUsage, is_error, subtype; anything else is text as today. verdictOf runs on text.
  The log file keeps the raw stdout and appends the extracted text so it stays readable.
• subtype: error_max_budget_usd is a failed attempt with that reason.
• README: the agent block, what each built flag closes off, and the prepare migration note.
• Settings panel: model, effort, mcpAllow (one per line), maxBudgetUsd.

2. A log of every agent run, and a pause on the session limit

• Table inbox_runs: id, pr_id, head_sha, phase ('prepare' | 'validate' | 'answer'), model, started_at, ended_at, duration_ms, turns, cost_usd, input_tokens, output_tokens, cache_read_tokens, cache_write_tokens, outcome ('prepared' | 'skipped' | 'validated' | 'answered' | 'failed' | 'timeout' | 'rate-limited'), note. Same migration style as the inbox_prs columns. store.recordRun,
  store.runs({ since?, prId? }), store.runTotals(since).
• preparePr returns the runs it made (runs: RunRecord[] on every result kind); tick records them.
  Attendant answers are recorded through a recordRun callback handed to realAttendantDeps.
• diffity inbox runs [--json] [--since <days>]: a table of when, PR, phase, model, turns, minutes, cost,
  tokens, outcome, with totals; the log the reviewer keeps.
• Page: a prepared card's meta line gets "· 8 min · $1.20" (prepare plus validate; hover shows each
  phase, model, turns and tokens); the footer gets "agent runs today: 5 · 41 min · $6.30 · 7 days: …".
  /api/inbox carries per-row cost and the totals; inbox status --json too.
• Rate limit: parseAgentOutput flags is_error, or text matching
  /hit your (session|usage) limit/i, as rateLimited with a best-effort resetsAt parsed from
  "resets 2pm (Europe/Stockholm)" (fallback now + 30 min). The row goes back to queued with reason
  "waiting: Claude session limit until 14:00", no attempt is counted, the run is recorded as
  rate-limited, and runDaemon sets pausedUntil: polling and reconciling continue, preparing does not.
  DaemonStatus//api/inbox expose pausedUntil; the footer says "preparing paused until 14:00 — Claude
  session limit"; ↑ after the reset time clears it.

3. CI's verdict in the prompt, and an optional hold until it is green

• PrSnapshot gains checks: { name, status }[] with status normalised to success | failure | pending | skipped | neutral from statusCheckRollup (check runs: status + conclusion; status contexts:
  state), duplicates collapsed by name keeping the worst, and files: { path, additions, deletions }[]
  capped at 300 — both from the gh pr view --json call already made per PR (fields work on gh 2.46).
  ciState(checks) → passing | failing | running | none. Store persists ci_state.
• Prompt, after the size line:

  CI at this head: check-job SUCCESS · pr-ecosystem-test (admin3) SUCCESS · integration-test-job SKIPPED · 11 more skipped
  Do not install dependencies, build, typecheck, lint or run tests: CI has done that, and this checkout is
  the author's code. Reason from the source. If a check failed or is still running, say so in the summary.
  

  or "CI has not reported for this head." The deny list in (1) enforces it.
• waitForCi (default false; Settings panel checkbox): in reconcile, before the normal queueing and
  only when the row is not bumped — running → queued with reason "waiting: CI running (3 checks)";
  failing → skipped with reason "CI failed: check-job, pr-mgmt-job". Both are re-evaluated every tick
  from the fresh snapshot (a "CI failed:" skip returns to the queue when the checks pass, unlike a
  filter skip at the same head). none counts as passing. ↑ prepares regardless.
• alertPaths: string[] (globs via path.matchesGlob): any changed file matching → the row is alerted,
  OR'd with the agent's ALERT line. "changes to datastore models" becomes
  ["packages/shared/src/model/**", "**/dbref/**"] and cannot be misjudged. Settings panel textarea.
• Card: a CI dot (green / red / grey) from ci_state.

Wave 2

4. validate: a second pass checks the P1 and P2 drafts

"validate": {
  "model": null,          // when set, a second pass with this model checks the P1/P2 findings
  "timeoutMinutes": 15,
  "maxBudgetUsd": null
}

• In preparePr, after PREPARED and before the bundle export: agent list --json on the session; keep
  open threads whose severityOf is P1 or P2. None → no validation (the "simple" case is decided by what
  was found, not by size). Otherwise run the validating agent — same argv builder with validate.model,
  same gate and deny list, no review skill in the system prompt — with composeValidatePrompt:

  A first pass drafted this review. Check its P1 and P2 findings against the code before the reviewer sees them.
  For each finding below, read the lines it points at and what they depend on (callers, callees, tests).
    - Correct and well put: leave it.
    - Correct but the text or severity is off: diffity --repo <wt> agent amend <comment-id> --body-file - <<'EOF' … EOF
    - Wrong: diffity --repo <wt> agent dismiss <thread-id> --reason "<why>"
  Add a finding only where checking one of these reveals another; this pass does not re-review the diff.
  If the general summary's verdict or counts no longer hold, amend it too.
  Nothing you do may reach GitHub. Print exactly one final line and stop: VALIDATED
  

  followed by the findings as data: thread id, comment id, file, lines, body (indented), one block each.
• VALIDATED → export the bundle, recompute the summary, record the run as validated. Timeout or no
  verdict → still export the draft, suffix the summary "· unchecked", record timeout/failed; the row is
  prepared either way.
• Live answers use validate.model when set, else agent.model: a question is about a finding, the
  stronger model's job.
• Summary hover shows "draft $x · check $y" from the run rows.

5. Measure before switching the default

• scripts/inbox-compare.ts <owner/repo#n> [--head <sha>] [--model <m>] [--effort <e>]: fetches
  refs/pull/n/head into the clone if needed, cuts a scratch worktree and data dir, runs preparePr with
  the candidate settings, and prints two columns of findings (severity, file, line range, first sentence)
  against the newest existing bundle for that head, matching on file plus overlapping lines, with each
  side's cost and time from the run log.
• Run it over the NCBackend3/admin3 bundles with --model opus, and with --effort medium on the
  default model. Paste the table here.
• Gate: a cheaper drafter is adopted as agent.model only if it reproduces every P1 in the baseline and at
  least four of five P2s. Otherwise the default model keeps drafting and validate stays off.

Delivery

Stacked PRs in this order, each with a patch version bump, a self-review on its own build, squash merge,
and a rebuild of the global:

1. (1) agent block, argv builder, skill in the system prompt, MCP gate, deny list, JSON output parsing.
2. (2) run log, inbox runs, cost on the page, rate-limit pause.
3. (3) checks/files on the snapshot, CI in the prompt, waitForCi, alertPaths, CI dot.
4. (4) validate pass.
5. (5) compare script, the comparison run, the table posted here.

After (1) lands: restart diffity inbox, delete the prepare key from ~/.diffity/inbox/config.json
(the built default applies), set agent.mcpAllow if wanted, and ↑ the three rows that failed on the
session limit (#14378, #13713, admin3 #605).

Later, if the run log says the prompt is where the tokens go: a per-file digest marking generated and
lockfile paths, and CI annotations on failing checks so lint and type errors are not re-found.

[fiddur]

Comparison run 1: Opus drafts vs the Fable bundles

scripts/inbox-compare.ts … --model opus over the 17 pull requests the inbox has bundles for, run 2026-09-07 21:19–22:58, one at a time. Baseline = the Fable bundle at the same head (old pipeline: reviewer settings loaded, toolchain runs allowed). Candidate = Opus through the new pipeline (no user settings, no MCP, CI in the prompt, deny list). "Found" is judged by reading the findings, not only by the script's line matching — a finding anchored on another line of the same problem counts, a different problem on the same line does not.

PR	baseline P1 / P2 / P3	Opus found P1	Opus found P2	new by Opus (P2 / P3)	$	min	out tokens
admin3#678	1 / 0 / 1	1/1 (as P1, anchored on the import, reasoned from red CI)	–	1 / 2	2.28	5.4	18k
NCBackend3#13444	1 / 0 / 2	1/1	–	0 / 0 (two P3s raised to P2)	1.38	3.2	12k
NCBackend3#14440	0 / 1 / 0	–	1/1	0 / 0	0.64	1.7	6k
NCBackend3#14473	1 / 1 / 2	1/1	1/1	1 / 4	1.84	4.9	19k
NCBackend3#14475	0 / 1 / 3	–	0/1 (ownership guard misses gifted B1s)	0 / 1	3.67	6.8	26k
NCBackend3#14493	0 / 0 / 4	–	–	1 / 3	4.85	9.1	37k
NCBackend3#14502	0 / 1 / 2	–	1/1 (as P1)	0 / 1	2.01	5.0	21k
NCBackend3#14508	0 / 2 / 1	–	1/2 (missed: non-canonical utm_* dropped from events)	1 / 2	3.53	7.7	29k
NCBackend3#14513	0 / 3 / 3	–	1/3 (missed: includesLHTest derivation, Airtable rows)	0 / 2	3.13	6.3	24k
NCBackend3#14516	0 / 1 / 4	–	0/1 (docs contradict the job)	0 / 6	4.30	8.9	35k
NCBackend3#14521	0 / 0 / 1	–	–	0 / 0	0.98	2.1	8k
NCBackend3#14530	1 / 1 / 2	1/1 (as P2)	0/1 (snapshots assert the opposite of the test names)	2 / 0	3.64	10.5	46k
NCBackend3#14532	0 / 0 / 1	–	–	1 / 1	3.01	7.8	30k
NCBackend3#14539	1 / 0 / 1	1/1 (as P2)	–	2 / 1	1.92	4.6	18k
NCBackend3#14547	0 / 0 / 0	–	–	0 / 0	0.57	1.3	5k
NCBackend3#14550	1 / 1 / 1	0/1 (missed: the snapshot baseline generated with the wrong threshold; Opus's P3 on the same line is about string literals)	1/1 (as P1)	1 / 0	2.88	6.9	28k
NCBackend3#14559	0 / 0 / 1	–	–	0 / 1	1.73	4.4	18k
total	6 / 12 / 29	5/6 (3 kept at P1, 2 lowered to P2)	6/12 (2 raised to P1)	~9 / ~24	42.36	96	381k

Per run: avg $2.49, 5.7 min, 40 turns, 22k output tokens. The Fable baseline runs (session logs, old pipeline) averaged 8.7 min, 37 tool calls and 27k output tokens; their cost was not recorded — the run log did not exist yet.

Against the gate

The plan's gate was: adopt a cheaper drafter only if it reproduces every P1 and at least four of five P2s.

• P1: 5 of 6 by content, 3 of 6 at P1. The miss is #14550 — the +11/−11 change whose P1 needed the pinned snapshot replayed against the committed threshold. Two of the found ones came in as P2.
• P2: 6 of 12. The misses are substantive, not questions: an ownership guard with a gap (#14475), a behaviour change that drops non-canonical utm_* params (#14508), a derivation that no longer holds for the new product keys (#14513), snapshots asserting the opposite of their test names (#14530).
• New findings: about nine P2s and two dozen P3s Fable did not raise. Several read as real (the postpartum cohort gate in #14493, unhandled Stripe calls in #14539, the °F edit lost to °C rounding in #14530); none has been verified, which is what the validate pass is for.

Verdict: do not make Opus the default drafter. The routing design lets the second pass remove or reword drafted findings but never add what the drafter missed, so the four missed P2s and the missed P1 would reach the reviewer as absences. A P1 drafted as a P3 (#14550) would not even be validated.

Caveats

• The columns differ in pipeline as well as model: the Opus runs could not run the toolchain and had CI's verdict in the prompt; the Fable runs could and did not. Comparison run 2 (--effort medium on the default model, same 17 PRs) is running now and separates the two.
• "Found" is my reading of 47 baseline findings against 68 candidate ones; the script's automatic file-and-line matching counted 32 of 47 reproduced.
• One run each; a model's findings vary between runs.

What the wins are, then

The token and time savings in this issue come from Wave 1 — no toolchain runs, no user context, CI in the prompt, the session-limit pause — with Fable drafting. validate stays available for a reviewer who wants a second opinion on the P1/P2 drafts, and inbox runs now records what every run costs, so the next comparison has a priced baseline.

Raw output per PR (markdown and JSON, plus each candidate's own diffity session) is under /tmp/diffity-compare/opus/ on valhall.

[fiddur]

Comparison run 2: Fable at --effort medium vs the Fable bundles

Same 17 pull requests, same baselines, scripts/inbox-compare.ts … --model fable --effort medium (the model passed explicitly: with agent.model unset the daemon runs Claude Code's default, which on this machine is now opus[1m] from ~/.claude/settings.json). Run 2026-09-07 23:04–23:24, then 08:23–08:50 after the Claude session limit stopped it for the night — the daemon's own rate-limit detection reported "waiting: Claude session limit until 00:40" on each affected run. "Found" is judged by reading the findings, as in run 1.

PR	baseline P1 / P2 / P3	medium found P1	medium found P2	new (P2 / P3)	$	min	out tokens
admin3#678	1 / 0 / 1	1/1 (as P1, anchored on the import; names the resolved @naturalcycles/shared version)	–	0 / 0	1.75	4.2	9k
NCBackend3#13444	1 / 0 / 2	1/1	–	0 / 0	1.19	2.7	6k
NCBackend3#14440	0 / 1 / 0	–	1/1	0 / 0	0.47	0.7	2k
NCBackend3#14473	1 / 1 / 2	1/1	0/1	0 / 1	0.79	1.4	4k
NCBackend3#14475	0 / 1 / 3	–	0/1	0 / 1	1.75	3.9	11k
NCBackend3#14493	0 / 0 / 4	–	–	1 / 1	1.65	3.7	12k
NCBackend3#14502	0 / 1 / 2	–	0/1 (the P3 on the same lines is the config-drift point, not the stale document)	0 / 0	0.74	1.1	3k
NCBackend3#14508	0 / 2 / 1	–	1/2	1 / 2	3.10	4.4	17k
NCBackend3#14513	0 / 3 / 3	–	1/3 (the includesLHTest derivation, as P3 on another line; productMaxItemsMap and the Airtable rows missed)	0 / 0	1.59	3.4	9k
NCBackend3#14516	0 / 1 / 4	–	0/1	0 / 2	1.95	4.2	13k
NCBackend3#14521	0 / 0 / 1	–	–	0 / 0	0.59	1.0	2k
NCBackend3#14530	1 / 1 / 2	0/1 (missed: the Oura flag gate on the account's hwId instead of the entry's)	1/1	0 / 1	1.61	3.9	12k
NCBackend3#14532	0 / 0 / 1	–	–	0 / 0	1.07	2.2	6k
NCBackend3#14539	1 / 0 / 1	1/1 (as P2)	–	0 / 0	0.94	1.8	5k
NCBackend3#14547	0 / 0 / 0	–	–	0 / 0	0.44	0.7	1k
NCBackend3#14550	1 / 1 / 1	0/1 (missed: the snapshot baseline generated with the wrong threshold)	1/1	0 / 0	1.31	3.1	7k
NCBackend3#14559	0 / 0 / 1	–	–	0 / 0	0.61	1.1	3k
total	6 / 12 / 29	4/6 (3 at P1, 1 as P2)	5/12 (1 as P3)	2 / 8	21.54	43	123k

Per run: avg $1.27, 2.6 min, 10 turns, 7k output tokens.

The two candidates side by side

	Fable, full effort (baseline, old pipeline)	Opus (run 1)	Fable, medium effort (run 2)
P1 found (of 6)	6	5 (3 at P1)	4 (3 at P1)
P2 found (of 12)	12	6	5
P3 found (of 29)	29	~20	~11
new P2 / P3 raised	–	~9 / ~24	2 / 8
avg cost per run	not recorded	$2.49	$1.27
avg time	8.7 min	5.7 min	2.6 min
avg output tokens	27k	22k	7k
avg turns	37 tool calls	40	10

Against the gate

Neither candidate passes. Medium effort is the cheapest by far — a quarter of the baseline's output tokens and a third of its time — but it reads a fraction as much code (10 turns against 37) and it shows: two P1s missed outright (#14530's account-versus-entry hwId gate, #14550's stale snapshot), one lowered to P2, seven of twelve P2s missed, most P3s gone, and almost nothing new. Where it did find the finding it was well put (the admin3 P1 names the exact resolved package version), so the loss is coverage, not judgement.

Verdict for #93: Fable at full effort stays the drafter. The savings that hold up are the Wave 1 ones already merged: no toolchain runs, no personal context or MCP unless allowlisted, CI's verdict in the prompt, the session-limit pause, and a run log that prices every run from now on. The comparison script stays for the next candidate model.

Notes

• A partial third data point exists: two PRs ran on opus[1m] at medium effort by accident before the model was made explicit (/tmp/diffity-compare/effort-medium/); admin3#678 found its P1 there too, at $1.52.
• Because agent.model unset now means Opus on this machine, anyone wanting Fable reviews from the inbox should set agent.model to fable in Settings. The run log's model column shows what actually ran.
• Raw output per PR is under /tmp/diffity-compare/fable-medium/ on valhall.