Review dispatcher — runbook

scripts/review-dispatcher.py (#3332). Turns a routing comment into an actual review session. Observe-only by default.

CLAUDE.md states the gap in as many words:

The dispatch IS the handoff. There is no queue. Nothing will pick a PR up later. If you do not dispatch, the PR sits unreviewed indefinitely.

That cost four days across five PRs in the milestone-#29 wave. This closes it.

---

1. What it can never do

Its entire write surface is one endpoint:

POST  /repos/{owner}/{repo}/issues/{n}/comments      (post an audit marker)
PATCH /repos/{owner}/{repo}/issues/comments/{id}     (refresh one)

There is no code path that merges, enqueues, approves, pushes, or relays. That is asserted behaviourally, not by inspection: LiveWriteSurfaceTests.test_the_only_mutating_endpoint_is_the_comment_endpoint records every HTTP call of a full live cycle and fails on any non-GET that is not a comment endpoint. (A grep for "merge" could not do this job — the source is full of prose about not merging.)

The reviewer it launches is bound by contract, not by the CLI. Be honest about this: --disallowedTools 'Bash(git push:*)' 'Bash(sudo:*)' 'Bash(gh pr merge:*)' stops those three literal command shapes and stops nothing clever — a reviewer that wanted to merge would call gh api -X PUT .../merge, which is a gh api invocation the deny-list does not match. So state what each control actually bounds, and claim no more than that:

1. the brief's explicit never-merge contract, restated in every dispatch — bounds a cooperative reviewer, which is the realistic case;
2. identity — the reviewer runs as its own bot, so a merge would be an attributable act by a named App in the audit log. That does not prevent the merge; it guarantees you can see who did it and revert it;
3. the deny-list — bounds git push, sudo and gh pr merge specifically, and nothing beyond them;
4. the live gate — DISPATCHER_LIVE=1, the caps and the kill switch bound how many reviewer sessions can exist at all and stop new ones instantly. This is the only one of the four that is unconditional.

What is not a control here: branch protection. A required-review rule stops an unreviewed merge; it does not bound a reviewer that approves and then merges its own approval. An earlier draft of this file claimed it did — it does not.

The honest summary is the one this was approved under: the automation adds no capability a human typing the same dispatch would not have — it only removes the requirement that a human be awake to type it (founder, 2026-09-09, "lets try").

---

2. The cycle

kill switch?  ─ yes ─▶ exit 0
      │ no
      ▼
take the global flock (LIVE only)  ─ held ─▶ exit 0 "another cycle holds it"
      │
      ▼
sweep review trees older than 6 h   (LIVE only; observe just reports them)
      │
      ▼
for each repo (core, client, admin):
    list open PRs
    for each PR:  fetch comments + reviews
                  parse the LATEST routing comment
                  decide, PER LANE
                  DISPATCH → post marker, THEN launch
                             (launch = fetch the pinned head, refuse it if it
                              moved, build a disposable worktree, run the
                              reviewer in it, trap-remove the tree)
                  NEEDS-HUMAN → post/refresh marker
                  SKIP → print the reason

The sweep is before any launch, and the whole cycle holds the lock, so a sweep can never race a review.

There are four triggers — A (initial), B (head moved after CHANGES_REQUESTED), C (dismissed lane, head settled), D (proved superseded COMMENT-only session) — and two review-state reads, below, shared by A/B/C/D.

The two review-state reads, and why there are two

CLAUDE.md pins the verdict read exactly — latest-per-reviewer, COMMENTED excluded; never .[-1].state, never bare counts. verdict_by_reviewer() is that and nothing else, and == [] under it is the "nobody was ever dispatched" sentinel.

It is the wrong question for "has this lane already been engaged". On core#3266 the QA lane was dispatched, read the PR, and left a substantive COMMENTED review; the architect went DISMISSED → APPROVED. Under the verdict read alone QA looks untouched and a per-lane trigger A would dispatch it again for work it had already done. So:

function	COMMENTED	answers	feeds
verdict_by_reviewer()	excluded	is this lane blocking?	trigger B, the CLAUDE.md == [] read
engaged_reviewers()	included	was this lane ever dispatched?	suppresses trigger A

Trigger A — initial

The lane has no review row of any kind → dispatch. For a single-lane routing this is exactly CLAUDE.md's reviews == [].

It is evaluated per lane, not per PR. A dual-lane routing where the architect approved and QA never answered still summons QA; the aggregate read alone would leave the second lane of every dual-lane routing unsummoned forever.

Trigger B — re-review

Verdict is CHANGES_REQUESTED AND head has moved off the commit that review was submitted against AND the cooldown has elapsed → dispatch again with a delta note listing the new SHAs.

"Pushing a fix does not re-request anything" (CLAUDE.md). This is the half of the loop that has repeatedly failed to close by hand.

Cooldown anchor = max(review submitted_at, head commit date, last marker ts); default 30 min. Anchoring on the head commit is what stops a re-dispatch landing in the middle of an author's push series. Anchoring on the marker is what stops a retry storm.

Trigger D — superseded pinned COMMENT-only session (#3532)

This repairs one specific gap: on core#3528 marker 5764450932 pinned QA to 063d7957481a123c62da226dc3508e222e7de645; review 5269678012 explicitly reported that examined pin while GitHub stamped its commit_id with the newer 684cf774e447987239e72da0c69b6ecf19a68154. A review row's commit_id alone therefore cannot establish the tree that a COMMENT-only session examined.

D applies only to an explicitly routed lane with no decisive review state. Its latest COMMENT must come from that lane's expected bot and contain exactly one standalone Reviewed at <40-lowercase-hex>. line, matching its latest non-shadow dispatcher marker. That marker must be one unambiguous dispatched marker posted by the dispatcher bot, with valid pin and timezone-aware dates; the review must postdate both the marker timestamp and its GitHub creation. A paired shadow makes the attribution ambiguous and holds for coordination. The pin must differ from the current head; that head must be open, non-draft, nonconflicting, stable for dismissed_stability_min, and past cooldown_min from the youngest head/dispatch/review timestamp. A missing head-date API read holds the spend and retries the read in a later cycle.

Missing, conflicting, malformed or untrusted provenance becomes an actionable NEEDS-HUMAN notice with trigger=D-hold; it NEVER patches a dispatched marker or an author's forged marker. Only a prior trusted D-hold notice can be refreshed. Ordinary substantive COMMENTs at an unchanged head remain engaged, with or without an explicit matching pin and even without a marker (hand-launched reviews). COMMENTED never becomes APPROVED. A decisive current-head review still wins. Optional shadow COMMENT lanes stay SKIP; they do not acquire a D-hold notice.

A push during a running review does not destroy its evidence. If the latest trusted dispatched marker is newer than the lane's latest COMMENT and younger than stale_min, D waits with SKIP superseded dispatch still running. At an old head, a stale dispatch with no subsequent report becomes one D-hold, never a speculative new-head retry. A later honest matching report is reevaluated against the latest dispatched marker, independently of the latest notice. Normal current-head stale retries still use the existing attempt ceiling.

Operational recovery: inspect the named dispatch marker, its pinned SHA, reviewer login/disclosure and corresponding session outcome. Preserve those records; do not rewrite a dispatch, mutate reviews, manufacture a pin or push a dummy commit. A genuine later COMMENT from the expected reviewer with the exact examined pin automatically clears only the provenance hold when the ordinary stability/cooldown/engine/cap gates permit; a genuine current-head decisive review is handled normally. If the session died without a report, or the record cannot establish an examined pin, the coordinator must arrange recovery through the authorized independent-review workflow and record its evidence. The hold itself authorizes no manual paid launch or policy bypass. Engine/auth/role/outage and other NEEDS-HUMAN markers keep their existing semantics; only an unambiguous D-hold written by the dispatcher bot is eligible for this reevaluation.

Activation impact: an open PR with an old marker, a pin-less legacy COMMENT and a moved head previously stayed silently SKIP. It now gets one visible D-hold per head (after the in-flight grace when applicable). Include these cases in the coordinator's scoped observe report before activating the reviewed release.

Completion of a D recovery requires its explicit examined pin, even if a pin-less COMMENT carries the current commit_id: this is the exact GitHub metadata mismatch D exists to repair. It becomes a non-destructive D-hold rather than a paid retry. Legacy unchanged-head plain comments keep their no-loop treatment.

Recovery uses the ordinary dispatch path: marker before launch, current-head lock, stale retry/attempt ceiling, PR debounce, global flock, cycle/daily caps, engine/account selection, outage policy, role identity and the launcher's fresh head fence all still apply. Old events do not authorize a second recovery at another head after a newer marker exists. Observe mode remains read-only and launches nothing. A completed COMMENT at the recovered pin also suppresses stale retries; a conflicting post-dispatch disclosure is a coordinator hold.

Source landing does not activate this behavior. The normal reviewed-release handoff and scoped observe/runtime evidence remain separate; no service, timer, configuration or state change is part of this source fix.

Trigger C — dismissal re-review (#3335)

Verdict is DISMISSED AND the PR is open, not draft, and not dirty AND no marker already covers this lane at this head AND the head has been stable for dismissed_stability_min (default 30 min) → dispatch again.

dismiss_stale_reviews: true is on main in all three repos, so every push to an approved PR turns its approvals into DISMISSED. That is neither CHANGES_REQUESTED (trigger B silent) nor an absent review (trigger A silent), so before #3335 the lane sat unserved forever and a human had to notice.

It was left out of #3332 deliberately — "the issue pins two triggers and a third deserves its own argument" — and the cost of the gap was then measured twice:

• core#3249 sat in this exact state through the #3332 acceptance run.
• On 2026-09-14 five Team Codex heads (#3387, #3358, #3362, #3364, #3365) were hand-carried to the coordinator with the diagnosis spelled out on #3335: "installed dispatcher release 6398984e explicitly excludes DISMISSED in _decide_lane." Five PRs, one afternoon, all coordinator time.

The argument, stated once. The API cannot tell "dismissed because stale" from "withdrawn on purpose" — both are the same row, and the dismissal event lives only in the timeline API, which does not say why either. #3335 rules that in this org nobody withdraws a review by hand, so a DISMISSED row means stale. If that stops being true the opt-out is a marker, not a new API read. The cost of being wrong is one review session; the cost of the gap is above.

Debounce anchor: the head commit's committer date. Never updated_at. pull_request.updated_at moves on any thread activity — including this script's own marker comment. It measures "time since anybody said anything", which is a different quantity from "time since the head moved", and using it would mean every dispatch resets its own stability window. So the cycle reads GET /repos/{repo}/commits/{head_sha} → commit.committer.date, paid only by a PR that actually has a dismissed lane.

Three edges of that anchor, all deliberate:

input	behaviour	why
date unknown (fetch failed, field absent)	HOLD	"could not establish that the head settled" is not "it settled". Fail-closed.
date in the future (clock skew)	HOLD	a negative age is < the window, so the safe branch is the one taken.
date in the past but lying (git commit --date)	dispatches early	committer dates are client-supplied. The marker caps the damage at one session.

Why 30 minutes, and why its own knob. A dismiss_stale_reviews push usually arrives in the middle of a push series — the author is mid-rebase — and the dismissal that matters is the last one. Thirty minutes of quiet is the cheapest available evidence that the series ended. It is DISPATCHER_DISMISSED_STABILITY_MIN, not DISPATCHER_COOLDOWN_MIN: the cooldown is a floor on how often one lane may be re-served after a block, this is a floor on how settled the tree must be before a dismissal is believed. Tuning one must not silently tune the other.

dirty is the only mergeable state that blocks. Never spend a review on a head that cannot land. unknown deliberately does not block: GitHub computes mergeability in a background job and answers unknown on a cold read, so "require known-clean" would gate the whole trigger on a race we do not control. The fail-open costs at most one session on a conflicting head; the fail-closed costs the trigger, intermittently and invisibly.

Marker-as-lock: one dismissal, one re-dispatch. The marker carries trigger=C and is head-pinned like every other, so a lane re-dismissed by a later push becomes eligible again only after that new head has been stable for the debounce.

Ordering — oldest-eligible-first. Both lanes of a dual-lane PR are dismissed by the same push, so both become eligible in the same cycle. With cycle_cap=1 exactly one is served, and serving them in routing-comment order would serve the same lane every time (the #3422 lesson: architect and QA must each be servable in successive cycles). Trigger-C dispatches are therefore ordered by when the pair became eligible — the dismissed review's submitted_at, which is a strict lower bound on the dismissal and monotone per lane, which is all an ordering key needs to be. Ties break on (repo, pr, lane) so the order is total.

Scope, stated because it is a real limit: this orders the lanes of one PR. Across PRs the cycle still serves in API list order — that is not starvation, because the per-PR head-churn debounce (§14) already bars a PR from being served twice inside debounce_min. Global ordering would mean collecting every repo's decisions before dispatching any, which is a restructure of run_cycle, not a trigger.

No bypass. Trigger C goes through the same kill switch, daily cap, cycle cap, global lock, marker and circuit breaker as A and B — asserted by running a dismissed PR into each gate (CycleTests.test_trigger_c_is_subject_to_the_*), not by inspection. Engine routing is untouched: a re-dispatched dismissal gets the engine, model and effort the lane would get today (test_trigger_c_uses_the_same_engine_resolution_as_any_other_trigger).

What still needs the coordinator. In this class: nothing. A dismissed lane now re-dispatches itself. The one case that still needs a human first is a conflicting head — the dispatcher holds with dismissed, but the head conflicts, and the author must rebase; no amount of coordinator attention substitutes for that. After the rebase the new head starts its own 30-minute stability window and the trigger fires on its own.

test_the_known_gap_is_closed_and_this_is_the_deliberate_update replaces the old test_dismissed_is_a_known_gap_and_does_not_dispatch, which said in as many words that a future change adding this trigger should update it deliberately.

---

3. Every review gets a disposable tree

The reviewer's cwd is a git worktree add --detach tree under /opt/upsquad/ops/review-trees, never a shared checkout.

The reason is not tidiness. The first version of this script ran cd /opt/upsquad/<repo> and leaned on the retry path to recover from a dead reviewer. That answers the dispatch consequence and not the filesystem one, and the filesystem one is worse:

A reviewer killed between mutating a file and restoring it leaves the shared checkout dirty; the marker goes stale; the next cycle re-dispatches into the same dirty tree — and the second reviewer reads someone's half-applied mutant as the PR's code.

That is core#852's class (parallel agents clobbering one checkout), now reachable on a 15-minute cron. Reviewers run mutation harnesses; being killed mid-mutation is not exotic on this box, it is what earlyoom does.

Cleanup is a trap, not a step the reviewer performs, because it must not depend on the reviewer surviving — and not surviving is precisely the case that fails. The launcher bash process survives an earlyoom kill of its claude child, so the trap covers that path. The one thing a trap cannot cover is the launcher itself being SIGKILLed; the sweeper in §9 is the belt for that.

Not scripts/agent-worktree.sh. Review trees never commit, so their HEAD never moves, and prune-agent-worktrees.sh never reclaims a worktree whose HEAD has not moved (core#2751 — that state is indistinguishable from "created seconds ago and in use"). Using it would leak one 84 MB tree per review forever. The box already carries 262 worktrees. test_tree_dir_is_not_the_pruned_worktree_base pins the location.

The moved-head refusal is structural. The launcher fetches the PR ref, compares it against the pinned SHA, and exits 75 before building a tree if they differ. An instruction the reviewer must remember to follow is weaker than a tree that cannot be built.

Two exit codes are meaningful rather than merely non-zero:

rc	meaning
75	head moved between the decision and the launch. Nothing ran; the next cycle re-evaluates against the new head.
78	the tree could not be prepared (token, fetch, or worktree add). Nothing ran.

4. The launcher — VERBATIM, smoke-tested

Smoke-tested 2026-09-09 on the devbox. This is the generated script verbatim (per-dispatch paths elided):

#!/usr/bin/env bash
# Generated by scripts/review-dispatcher.py (#3332). Ephemeral.
set -uo pipefail

MAIN=/opt/upsquad/upsquad-core
TREE=/opt/upsquad/ops/review-trees/upsquad-core-3333-principal-architect-<stamp>
REF=refs/dispatcher/upsquad-core-3333-<stamp>
PINNED=32a5bf66f7602a0c4d4d79402fbb52995a903fdc
LOG=/opt/upsquad/ops/review-dispatcher-logs/<...>.log
RAW=<the launch's ephemeral dir>/engine-stdout.json   # claude lane only (#3399)

cleanup() {
  git -C "$MAIN" worktree remove --force "$TREE" >/dev/null 2>&1 || rm -rf "$TREE"
  git -C "$MAIN" update-ref -d "$REF" >/dev/null 2>&1 || true
  git -C "$MAIN" worktree prune >/dev/null 2>&1 || true
}
trap cleanup EXIT INT TERM

unset GH_TOKEN
FETCH_TOKEN="$(python3 "$MAIN/scripts/gh-token.py" project-manager 2>>"$LOG")"
[ -n "$FETCH_TOKEN" ] || { echo "launcher: could not mint a fetch token" >>"$LOG"; exit 78; }
_hdr="AUTHORIZATION: Basic $(printf 'x-access-token:%s' "$FETCH_TOKEN" | base64 -w0)"
GIT_CONFIG_PARAMETERS="'http.https://github.com/.extraheader=${_hdr}'" \
  git -C "$MAIN" fetch --quiet --no-tags https://github.com/upsquad-ai/upsquad-core.git \
    "+refs/pull/3333/head:$REF" >>"$LOG" 2>&1
rc=$?; unset FETCH_TOKEN _hdr
[ "$rc" -eq 0 ] || { echo "launcher: fetch failed (rc=$rc)" >>"$LOG"; exit 78; }

FETCHED="$(git -C "$MAIN" rev-parse "$REF" 2>>"$LOG")"
if [ "$FETCHED" != "$PINNED" ]; then
  echo "launcher: head moved ($PINNED -> $FETCHED); NOT reviewing it" >>"$LOG"
  exit 75
fi

mkdir -p "$(dirname "$TREE")"
git -C "$MAIN" worktree add --detach --quiet "$TREE" "$REF" >>"$LOG" 2>&1 || exit 78
cd "$TREE" || exit 78

unset GH_TOKEN
claude -p \
  --setting-sources user \
  --agents "$(cat <ephemeral>/agents.json)" \
  --agent principal-architect \
  --model fable \
  --add-dir /opt/upsquad/upsquad-core/.claude/agents/principal-architect \
  --allowedTools Bash Read Grep Glob Write Edit WebFetch WebSearch TodoWrite \
  --disallowedTools 'Bash(git push:*)' 'Bash(sudo:*)' 'Bash(gh pr merge:*)' \
  --permission-prompts none \
  --max-budget-usd 8 \
  --output-format json \
  "$(cat <brief>.md)" \
  >"$RAW" 2>>"$LOG"
rc=$?
exit "$rc"

The redirection changed in #3399 and only half of it did. stdout is now a single JSON object (review text + usage + total_cost_usd), so it goes to $RAW and the parent writes .result into the log in its place, followed by one usage: line. stderr still goes straight to the log.

Corrected after review 5236668348. The first version of this section said stderr is where "a dying CLI says why". It is not. Probed on claude 2.1.273 (invalid model ⇒ a free 404): the error text arrives in the stdout JSON result with is_error:true, terminal_reason:"api_error", api_error_status:404, while stderr carries only a [claude-code:*] tag. So the quota breaker's input reaches the log through the parent writing .result back verbatim, at column 0 — engine_text_for_log, pinned by scenario_usage_logging_cannot_trip_the_breaker and attacked by mutants M3399-C/M3399-D, both of which survived the entire suite before that pin existed. stderr still belongs on $LOG for the wrapper's own output (timeout, shell errors, CLI tags) — a real reason, just not the one first claimed.

Note also: subtype is "success" on an API error. Never key off it. is_error / terminal_reason / api_error_status are the honest fields, and they are a better oracle than CLAUDE_QUOTA_SIGNATURE_RE (whose own docstring admits it is inferred from no corpus) — a follow-up, not #3399.

See §15.

invoked as:

sg upsquad-devs -c 'bash /tmp/rd-tree-smoke-XXXX/launch.sh'

Smoke transcripts

(a) The CLI and tool permissions, against the shared checkout before the disposable tree existed. Two runs, because "it replied" proves the CLI ran, not that Bash was granted:

$ printf 'Reply with the single word READY. Do not use any tool.\n' > brief.md
$ sg upsquad-devs -c 'bash /tmp/rd-smoke-86gztmpo/launch.sh'; echo RC=$?
RC=0
$ cat smoke.log
READY

$ printf 'Run exactly `echo READY-WITH-BASH` with the Bash tool ...\n' > brief.md
$ sg upsquad-devs -c 'bash /tmp/rd-smoke-86gztmpo/launch.sh'; echo RC=$?
RC=0
$ cat smoke.log
READY-WITH-BASH

(b) The disposable tree, end to end, pinned to a real PR head so the launcher's head assertion had to pass. The brief forced four Bash calls, one per thing that had to be true:

$ git -C /opt/upsquad/upsquad-core worktree list | wc -l
262
$ sg upsquad-devs -c 'bash /tmp/rd-tree-smoke-34x93wjh/launch.sh'; echo RC=$?
RC=0
$ cat smoke.log
/tmp/rd-tree-smoke-34x93wjh/trees/upsquad-core-3333-principal-architect-20260909T070419Z
CLAUDE_MD_OK
AGENT_DEF_OK
MEMORY_READABLE
$ ls -d /tmp/rd-tree-smoke-34x93wjh/trees/*
ls: cannot access ...: No such file or directory      # trap removed it
$ git -C /opt/upsquad/upsquad-core worktree list | wc -l
262                                                    # no leak
$ git -C /opt/upsquad/upsquad-core for-each-ref 'refs/dispatcher/**'
                                                       # empty: the ref went too

What that proves, line by line:

output	proves
/tmp/.../trees/upsquad-core-3333-...	the reviewer's cwd is the disposable tree, not the shared checkout
CLAUDE_MD_OK	CLAUDE.md is tracked, so the worktree carries it
AGENT_DEF_OK	.claude/agents/principal-architect.md is tracked, so --agent resolves from the tree root. Historical (2026-09-09): since #3489 the tree's copy is deliberately NOT loaded (--setting-sources user); the role comes from --agents (§6, §8)
MEMORY_READABLE	the --add-dir grant works — see below
worktree count unchanged, for-each-ref empty	the trap removed the tree and the fetch ref

MEMORY.md is untracked, so it needs an explicit grant

CLAUDE.md and .claude/agents/<role>.md are tracked files and a full worktree carries them. .claude/agents/<role>/MEMORY.md is NOT tracked — house rule is "never commit agent MEMORY.md" — so a fresh worktree does not have it. Verified, not assumed:

$ git ls-files .claude/agents/principal-architect/     # empty
$ ls   .claude/agents/principal-architect/             # MEMORY.md

So the launcher passes --add-dir <checkout>/.claude/agents/<role> — one directory, containing only that lane's own memory. Granting the whole shared checkout would re-open the exact mutation hole the disposable tree closes; test_grants_only_the_lanes_own_memory_dir_from_the_shared_checkout asserts the narrow form and the absence of the broad one.

Flags — why each one, and two that are deliberately absent

flag	why
-p	headless print mode.
--setting-sources user	every Claude launch (#3489). The cwd is the PR head; without this the CLI loads the PR's own .claude/settings.json (hooks, env, permission rules), its .mcp.json servers and its .claude/agents/. §6 has the measurement.
--agents '<json>'	every Claude launch (#3334, #3489): DEFINES the role --agent selects, from role_definitions — this release's core file for principal-architect/product-manager (every repo, core included), the target repo's main for local-team roles. Written to the launch's ephemeral dir and passed as "$(cat agents.json)". Required: with project settings off, no project agent loads, so this is the only place the role can come from. See §8.
--agent <role>	selects the --agents entry. Verified load-bearing: claude --agent no-such-agent-xyz exits 1 and lists the available agents, so a typo fails loudly rather than running a generic assistant; --setting-sources user --agent <role> with no --agents likewise exits not found.
--add-dir <memory>	restores the lane's untracked MEMORY.md, and nothing else.
--allowedTools ...	grants the tools without bypassing the permission system.
--permission-prompts none	nobody is there to answer a prompt; anything that would prompt is denied instead of hanging.
--model fable	house standard for agent work during development.
--max-budget-usd 8	bounds the cost of a runaway session.
--output-format json	text until #3399, when the log stopped being the only consumer: the single JSON object carries usage and total_cost_usd, which is the only way to price a review. The log still reads like a log because the parent writes .result back into it — and that write-back is also the route the quota breaker's input takes (§15).
--dangerously-skip-permissions / bypassPermissions	NOT used. Tried first; both unnecessary (the --allowedTools route grants everything a reviewer needs) and less safe. test_does_not_use_bypass_permissions pins its absence.
exec claude ...	NOT used. exec replaces the shell, and a replaced shell runs no trap — it would silently disarm the cleanup. test_no_exec_anywhere pins its absence line by line.

sg upsquad-devs -c 'bash <path>' — the -c string is POSIX-only because sg runs dash; all real logic lives in the generated .sh file. The group is needed because the App private keys at /opt/upsquad-keys are mode 2750, group upsquad-devs.

The fetch token goes through GIT_CONFIG_PARAMETERS (an env var) rather than git -c http...extraheader= (argv), so it never reaches .git/config, the reflog, or a world-readable ps. test_fetch_token_never_reaches_argv.

If you change any flag

LauncherTests.test_carries_the_smoke_tested_flags will go red. Re-smoke-test and update the transcript above — do not update the test from memory. The whole point of recording the invocation verbatim is that it was observed, not guessed.

---

5. The engine switch — which model reviews (#3337)

Review-always is unchanged. This changes which model reviews, never whether.

Founder-approved 2026-09-09 when the Claude plan moved to pay-as-you-go: routine review burn moves to Codex, Claude stays decisive where it must.

The policy

engine = f(author model, change class), with three overrides that configuration cannot relax:

#	rule	source
1	class in CLAUDE_ONLY_CLASSES (currently security-critical) → Claude	code
2	PR is Codex-authored → Claude (no-self-model)	code
3	routing comment asked for two lanes → Claude	code
4	otherwise the class default (Codex for every routine class)	.claude/review-briefs/engine-routing.toml

Rules 1–3 are evaluated first and in that order. Security pattern matches also win over routine pattern order and class rank, so a routine catch-all or negative rank cannot hide an existing security match.

What rule 2 does not cover, stated plainly: a Claude-authored security-critical PR is reviewed by Claude. That is Claude reviewing Claude. It is the "where avoidable" clause in the cross-model rule doing real work — the alternative is putting the most sensitive reviews on the lane with no OS confinement (§6), which is a worse trade than the self-model risk. Pinned by test_claude_reviews_claude_on_security_critical_and_that_is_stated.

Authorship detection — ANY commit, and the fail-safe

Any commit carrying Co-Authored-By: ... Codex ... makes the whole PR Codex-authored. Not the first, not the last, not a majority. A PR where four commits are Claude's and the third is Codex's still contains Codex's work, and that is the work cross-model review exists to keep away from Codex. scenario_trailer_in_a_middle_commit_counts, mutant M9.

The list-PR response does not carry authoritative commit/file counts. Read the PR detail at the same head, then page commits and files completely against those counts. An incomplete, malformed, repeated, over-limit or changed-head read routes to Claude: an unseen file may be security-critical and an unseen commit may carry Codex authorship. Guessing the other way would put a possibly-Codex-authored PR on Codex, which is the exact state rule 2 exists to prevent. Mutant M10. The same fail-safe applies when the API call fails outright.

unknown (no trailer at all) is not treated as Codex — a PR with no trailers is genuinely not Codex-authored, and the fail-safe belongs at the point where we could not see, not at the point where we looked and found nothing.

The config, and the one thing it cannot say

.claude/review-briefs/engine-routing.toml, checked in so every change to who reviews what is itself reviewed. It carries the models, the per-file classification rules, and the per-class engine + shadow flag.

Assigning any engine but claude to a CLAUDE_ONLY_CLASSES member makes the loader refuse the whole file — not correct the field. Correcting one value while trusting the rest of a file we have just proved we do not understand is how a half-understood policy ships. On refusal the dispatcher falls back to Claude everywhere and prints the reason on every cycle:

!! ENGINE ROUTING CONFIG REFUSED — falling back to Claude everywhere
!! .../engine-routing.toml: class 'security-critical' is assigned engine 'codex'. ...

The fallback is the safe engine precisely because the reason we are in it is that we could not establish what the policy is.

What this actually saves — measured, not hoped

Classification over the 40 most recently merged upsquad-core PRs (2026-09-09):

class	PRs	share
security-critical	22	55%
backend	11	28%
docs	4	10%
tests	3	8%

So 45% of merged core PRs would run on Codex, not "most routine work".

The reason is one pattern: 14 of those 22 security-critical PRs qualify ONLY because they touch .github/workflows/**, and on this repo that is overwhelmingly db.yml — whose named-guard roster a migration PR has to bump. A routine migration therefore drags itself into the Claude-only class.

.github/workflows/** is kept as security-critical anyway, because a workflow diff genuinely can add permissions:, a secret, or change runs-on, and a path match cannot tell that from a guard-count bump. Making the workflow rule content-aware would move the Codex share from 45% to roughly 80% — the single highest-leverage tightening available — and it is a real design decision rather than something to guess at inside this change. Filed as a follow-up.

Attribution

Every brief requires the reviewer to put this line in its review body:

Review engine: Codex (gpt-6-astra)

It mirrors the commit trailer that records which model wrote the code, so review quality is auditable per model the same way authorship already is. The marker carries the same facts (engine=, class, author model, and the reason the engine was chosen) for the machine.

Shadow mode

Per-class, off everywhere by default. When on for a class, the other engine is dispatched alongside the decisive one and its brief forbids APPROVE / REQUEST_CHANGES — two decisive verdicts from one lane is a contradiction the merge rules cannot resolve.

Shadow never puts Codex on a Codex-authored PR: it would be a back door into the exact pairing rule 2 forbids (test_shadow_never_puts_codex_on_a_codex_authored_pr).

Shadow markers are keyed on (lane, engine, shadow); decisive markers retain the lane lock across engine changes, so a policy rollout does not forget an in-flight review. Markers written before #3337 carry no engine= field and read as claude, so an old marker stays findable — an un-findable marker is a duplicate dispatch.

---

6. What the Codex sandbox does NOT enforce

Same standard as §1: say what each control actually bounds, and claim no more.

On this host the Codex engine runs with no OS confinement at all. Not a preference — a measured limitation:

$ codex exec --sandbox workspace-write ... 'run `echo READY-WITH-SHELL`'
bwrap: setting up uid map: Permission denied

$ unshare --user --map-root-user true
unshare: write failed /proc/self/uid_map: Operation not permitted

$ cat /proc/sys/kernel/apparmor_restrict_unprivileged_userns
1

Codex sandboxes with bubblewrap, bubblewrap needs an unprivileged user namespace, and this box refuses to create one. Every shell call the reviewer makes fails under workspace-write. --sandbox danger-full-access is the only mode in which the Codex engine can execute anything here.

Note codex doctor reports 20 ok · 0 warn · 0 fail on this box. It does not probe the sandbox. A green doctor is not evidence the sandbox works.

The comparison, honestly

	Claude lane	Codex lane (this host)
tool gating	--allowedTools — an in-process allow-list of tool names	none
command deny-list	--disallowedTools 'Bash(git push:*)' 'Bash(sudo:*)' 'Bash(gh pr merge:*)' — three literal prefixes	none
filesystem confinement	none (the allow-list is not an OS sandbox)	none on this host; workspace-write would confine writes to the workdir if bubblewrap worked
network confinement	none	none on this host; workspace-write blocks network unless sandbox_workspace_write.network_access = true
cost bound	--max-budget-usd 8	none — codex exec has no equivalent
wall bound	timeout in the launcher	timeout in the launcher

Neither lane is an OS sandbox today. The Claude lane is strictly stronger by exactly one thing: a command deny-list covering three literal prefixes. That deny-list does not stop gh api -X PUT .../merge either (§1), so the difference is real but narrow.

What remains on the Codex side: the disposable worktree (§3), the brief's never-merge contract, bot identity making every action attributable, and the live gate. That is why CLAUDE_ONLY_CLASSES is a code-level gate the config cannot relax, and why it is the first rule evaluated.

The PR head configures its own reviewer — closed for the Claude lane (#3489)

The launcher cds into the PR head and runs the engine there. For the Claude CLI that means the PR's own project configuration is loaded unless told otherwise, and the PR author controls it. Measured on claude 2.1.278 at $0 (invalid model), launched under sg upsquad-devs exactly as the launcher does, in a tree carrying a planted .claude/settings.json (a SessionStart hook, a UserPromptSubmit hook and an env block), a planted .mcp.json server, and an edited principal-architect.md (model: model-from-pr-head):

before #3489 (no --setting-sources, --agent principal-architect)
  api_error 404 cost=0 | markers: mcp.txt  sessionstart.txt=[upsquad-devs sudo users docker vb dev env=from-pr-head]  userprompt.txt
  (no CLI --model)     -> the 404 names model-from-pr-head: the PR's edited definition was the reviewer
#3489 (--setting-sources user --agents <release json> --agent principal-architect)
  api_error 404 cost=0 | markers: none
  (no CLI --model)     -> the 404 names model-from-release (the release JSON, its model set to that for the probe)
--setting-sources user alone, no --agents
  --agent 'principal-architect' not found. Available agents: claude, Explore, general-purpose, Plan, statusline-setup

The CLI's own init event names the rest of what the project source carries, and costs nothing to read (--output-format stream-json --verbose, invalid model). Same tree, only the flag differs:

init field	default sources	--setting-sources user
skills	planted-skill present	absent
slash_commands	pr-planted present	absent
mcp_servers	the planted project server listed	absent (only claudeai user connectors)

So before #3489, a PR could run arbitrary commands before the model read a single line — no reviewer judgement in the loop — with the upsquad-devs group that reads every bot key in /opt/upsquad-keys; could set the reviewer's environment (endpoints, model aliases); could start an MCP server; could add skills and slash commands; and could rewrite its own reviewer's contract. "Reviewers run PR code anyway" does not cover this: a reviewer runs PR code only after reading the diff and choosing to.

One thing the PR head did NOT control, stated exactly. permissions.allow in the planted settings was refused by the CLI itself — "Ignoring 1 permissions.allow entry from .claude/settings.json: this workspace has not been trusted." Hooks, env and .mcp.json were honoured; permission rules were not (review 5258304758, N5). The other three are damning enough without overstating the fourth.

Mitigation (every Claude launch): --setting-sources user — only the box owner's ~/.claude/settings.json loads; no project or local settings, no project .mcp.json, no project agents — plus --agents carrying the role from role_definitions (§8), which is the only place the role can then come from. Pinned by M3489-A (scenario and process level) and by RealCliProofTests against the real binary (opt-in: DISPATCHER_REAL_CLI=1).

The side effect: the reviewer also loses the repo's house rules. MEASURED, and an earlier version of this section claimed the opposite. Raised as R1 of review 5258304758 and re-measured here with the file tools BLOCKED, so the answer cannot come from the model reading the file itself (no permission_denials either — it did not try):

--setting-sources user         -> NONE                      (the CLAUDE.md sentinel is NOT in context)
--setting-sources user,project -> SENTINEL-CLAUDE-MD-7f3a

Note the trap in measuring this: with file tools ALLOWED, user also answers with the sentinel — because the model reads CLAUDE.md off disk. That reads like "it is loaded" and is not. Block the tools.

Security-wise this is a bonus: one more injection channel closed, since CLAUDE.md is where the routing table, the escalation list and the merge rules live, and a PR could otherwise edit the rules its own reviewer is judging it by. Operationally it is a real loss, so the rules are handed back over a trusted route: _shared-mechanics.md §2b tells every lane to read git show origin/main:CLAUDE.md — never the working tree's copy — and pins it with test_every_lane_brief_hands_the_house_rules_back_over_a_trusted_route. The same applies to .claude/skills/** and project slash commands: they stay unloaded, deliberately, and anything a lane genuinely needs comes through its brief (release-supplied) or git show origin/main:<path>.

What this does not cover.

• Codex — narrowed, not open. Probed at $0 on codex exec 0.154.0 with an invalid model, in a tree carrying a planted AGENTS.md and a planted .rules: nothing executed before the model call. Codex has no hook mechanism, and .rules only NARROWS an exec policy this lane already runs at danger-full-access. What remains is prompt-level (AGENTS.md is read as instructions) plus the user-level MCP servers Codex connects (review 5258304758, N4).
• User-level MCP connectors still connect in the Claude lane — the box owner's claude.ai Gmail / Slack / Drive / Calendar servers appear in the init event above. Their tools are not in --allowedTools, so the reviewer cannot call them; they are the part of the MCP surface --setting-sources user does not remove (N6).

Restoring the sandbox — the upgrade path

Needs root, so it is a founder action, not an agent one:

sudo sysctl -w kernel.apparmor_restrict_unprivileged_userns=0   # or install bwrap setuid
unshare --user --map-root-user true                             # must succeed

Then change _codex_engine_block from

--sandbox danger-full-access

to

--sandbox workspace-write -c sandbox_workspace_write.network_access=true

(network is required — the reviewer has to reach the GitHub API to post its verdict), re-run the §4 codex smoke, and update the transcript. Do not make this change on the strength of the sysctl alone; the smoke is the evidence.

Upgrading the model

gpt-6-astra at ultra is the highest-priority listed model and highest reasoning level observed in the installed CLI (0.153.4) on the 2026-09-09 Codex takeover. The original author measured xhigh; the available effort list now includes max and ultra, so the checked-in setting was updated and re-smoked:

python3 - <<'PY'
import json, pathlib
d = json.loads((pathlib.Path.home()/".codex/models_cache.json").read_text())
for m in sorted(d["models"], key=lambda x: x["priority"]):
    if m.get("visibility") == "list":
        print(m["priority"], m["slug"], m.get("default_reasoning_level"),
              [r["effort"] for r in m.get("supported_reasoning_levels", [])])
PY

Take the lowest priority with visibility == "list", re-run the smoke, and change the two lines in engine-routing.toml in a PR.

---

Takeover smoke and live-entrypoint evidence (2026-09-09)

Codex CLI 0.153.4 lists gpt-6-astra at priority 1 with efforts low, medium, high, xhigh, max, ultra. The generated launcher was re-run at ultra on a real disposable tree pinned to prerequisite PR #3333's head d69ae84384b6e83f2613acf1eefe9ef02607304c. The task's own devops-engineer identity was used for this read-only fetch smoke, rather than impersonating the deployed project-manager orchestrator. No review or GitHub comment was posted.

The current working engine invocation, re-smoked after the timeout correction (task paths elided):

timeout --foreground --signal=TERM --kill-after=60 120 \
  codex exec \
  --model gpt-6-astra \
  -c model_reasoning_effort=ultra \
  --sandbox danger-full-access \
  --ephemeral \
  -C "$TREE" \
  --add-dir /opt/upsquad/upsquad-core/.claude/agents/devops-engineer \
  "$(cat "$BRIEF")" \
  < /dev/null >>"$LOG" 2>&1

Observed shell output and cleanup:

/opt/upsquad-worktrees/upsquad-core/devops-engineer-3337-scratch/smoke-codex/tree
CLAUDE_MD_OK
AGENT_DEF_OK
d69ae84384b6e83f2613acf1eefe9ef02607304c
READY-WITH-SHELL
ENGINE codex RC 0 TREE_EXISTS False
REF_LEFT False

The unchanged Claude engine had passed its original disposable-tree smoke (§4); the takeover retry failed before any shell call with:

You've hit your monthly spend limit · raise it at claude.ai/settings/usage?from=cc_cli_limit_message · your weekly limit resets Sep 12, 7am (UTC)
ENGINE claude RC 1 TREE_EXISTS False
REF_LEFT False

This is an account-readiness blocker, not a successful smoke. The displayed weekly reset does not establish when the reported monthly spending block will clear. Claude-required lanes must not silently fall through to Codex.

The first actual flock probe also exposed an inherited entrypoint failure: placing -- after the lock filename runs an executable literally named -- (rc 69). It now precedes the filename. LockEntrypointTests.test_real_flock_runs_cycle_and_contention_does_not_run_another executes the real flock utility with a harmless child, proves the child ran once, then proves held-lock contention cannot run another. Mocked run_cycle tests alone did not reach that defect.

Independent-review corrections

File classification includes both filename and previous_filename for a rename. Moving internal/auth/token.go to a routine path still changes the security-sensitive surface. Raw API record counts are validated before expanding classification paths; a missing or malformed rename source refuses the input instead of treating it as routine.

The process supervisor now starts sg in its own session/process group and retains the global lock until that group is terminated. The inner timeout --foreground keeps ordinary descendants in the owned group. On expiry, the supervisor sends TERM, gives the EXIT trap up to five seconds, sends KILL to any remaining group even if sg exited first, reaps the immediate child, then performs bounded cleanup of the exact tree/ref if necessary. INT/TERM traps exit rather than returning into preparation. Temporary launcher files live beneath the configured log directory and are removed after execution.

This is process ownership, not OS confinement: a process deliberately escaping into another session is outside the guarantee. The generated model invocation does not do that. The existing no-confinement warning remains unchanged.

Real-process tests exercise normal completion for both engine command shapes, slow preparation that must never launch after lock release, and a TERM-ignoring engine plus descendant that must stop before return. M17 replaces whole-group termination with killing only sg; the exact orphan-launch assertion fails, with no import/process-setup error accepted as a kill. Full scripts lane: 384 tests pass; 17 mutants killed. The actual Codex launch() path was re-smoked successfully with gpt-6-astra/ultra, including tree/ref cleanup; the actual Claude path still reports its monthly spending limit and cleans up.

7. The marker — record and lock

Posted BEFORE the launch, never after. If the marker post fails, nothing is launched (test_a_failed_marker_post_means_no_launch). The state we refuse to have is launched but unrecorded, because the next cycle would launch again with nothing to suppress it.

<!-- review-dispatcher: lane=principal-architect head=<40-hex> attempt=1
     state=dispatched ts=2026-09-09T06:00:00Z trigger=A -->

The human-readable body above it repeats lane / head / timestamp / reason, so a person scrolling the thread sees what the next cycle parses.

Head-pinning is what makes it a lock and what makes it expire. A marker is authoritative only for the SHA it names; when head moves it goes stale by construction, which is the same mechanism that makes trigger B possible.

Decisive markers lock (PR, lane, head) across engine changes; changing policy cannot launch a second decisive review while the previous engine is running. Shadow markers remain separate and never satisfy the decisive lock. NEEDS-HUMAN PATCHes the same comment rather than posting a new one, so the history is the comment's edit history. A PR-level refusal (unparseable routing, which names no lane) uses the sentinel lane=none — without a sentinel to find, it would re-post an identical refusal every cycle forever.

States

state	meaning	next cycle
dispatched, age < 90 min	a session is running, or just ran	SKIP
dispatched, review exists at the same head	done	SKIP
dispatched, age ≥ 90 min, no review, attempt < 3	the session died	re-dispatch, attempt+1
dispatched, age ≥ 90 min, no review, attempt = 3	out of retries	→ needs-human
needs-human at the current head	already refused	SKIP

---

8. Refusals (NEEDS-HUMAN, never a dispatch)

refusal	why
author == reviewer	"Agents never self-approve." A lane whose bot login is the PR author is refused outright — and the check is evaluated before any trigger, so "we never got far enough to notice" cannot happen.
unparseable routing	a comment opening with "Routing to" that names no known @upsquad-<role>, including one naming an unknown role. Refusing beats guessing.
agent definition absent	the lane's definition does not resolve for this repo. See below — the portal half of this was live until #3334.
product-manager as sole lane on core	the PM bot has no write access on upsquad-core, so its APPROVE does not satisfy branch protection. Dispatching it alone produces something that reads like a green light and unblocks nothing. Paired with another lane, it is fine; on client/admin, it is fine.
review brief absent	no .claude/review-briefs/<role>.md resolves for the lane, by _load_template's own lookup (release → shared core → target). Only architect, QA and devops have briefs today. Refused at decision time: before #3487 _do_dispatch found out after posting a "Review dispatched" marker, spent a daily-cap slot, and retried into the same wall for ~4.5 h (#3486). Evaluated after the PM-on-core refusal so that one keeps its more specific reason. Observed on 2026-09-19: core#3481's paired PM lane was DISPATCH on main (held only by the day's cap) and is NEEDS-HUMAN review brief absent here.
retries exhausted	3 attempts, 90 min each, no review.

Cross-repo roles — one definition, supplied from core (#3334)

The gap, as it was (2026-09-09, and again at 11:30Z on 2026-09-19 for client#885/#887 and admin#22):

$ cd /opt/upsquad/upsquad-client && claude --agent principal-architect -p 'x'
--agent 'principal-architect' not found. Available agents: backend-sme, claude,
devops-engineer, Explore, frontend-sme, general-purpose, Plan, project-manager,
qa-engineer, statusline-setup

principal-architect and product-manager exist only in core. The portal repos carry their five local-team definitions and neither of these, while the routing table sends most portal PRs to the architect. Launching anyway would run a generic assistant that then posts a review as the architect bot, so the dispatcher refused.

The fix is not a copy. A vendored definition is a second reviewer that drifts, and a drifting definition is a drifting reviewer. Instead:

role	reviewing in core	reviewing in client/admin
local-team (5)	upsquad-core@main (contents API)	that repo's main (contents API)
principal-architect, product-manager	this release's core file (CORE_AGENTS_DIR)	this release's core file

Every one reaches the CLI as --agents '<json>', next to --setting-sources user (#3489, §6). Never the PR head — before #3489 core resolved from the tree under review — and never a shared checkout (any branch, dirty). A 404 on main is an answer (the lane is absent → NEEDS-HUMAN); any other read failure skips the repo for that cycle, because a transient 500 turned into a NEEDS-HUMAN marker would park the lane at that head.

• Source = the running release, not a shared checkout. Same root the brief templates are read from; a shared checkout can sit on any branch, dirty. No fallback. Pinned structurally by the_definition_source_is_this_release (M3334-K) — before it, pointing CORE_AGENTS_DIR at the shared checkout survived every test.

• --agents outranks a project file (documented subagent priority: managed > --agents > project > user > plugin). Probed at zero cost — a project copy and an --agents entry with the same name, each with a distinct invalid model; the 404 names the one that won:

  project copy only    -> There's an issue with the selected model (model-from-project-copy) ...  $0
  --agents + copy      -> There's an issue with the selected model (model-from-core-json) ...     $0

  So a portal copy, including one the PR under review adds, cannot stand in for or shadow the canonical definition — and since #3489 no project agent is loaded at all. The source is a function of the lane only (from_release), never of which files a checkout carries.

• Conversion is narrow and fails closed. claude_agents_json maps description, tools/disallowedTools (comma list → JSON list), model, and the body → prompt; name becomes the key and color is dropped. Any other frontmatter key is refused, not dropped — a key the file loader honours and the converter silently discarded would make the portal reviewer a different agent. Quoting, block scalars, continuation lines, duplicates and a >96 KiB result (one argv element caps at 128 KiB) are refused too. A refusal happens at decision time (role_definitions) — NEEDS-HUMAN, detail core's .claude/agents/<role>.md could not be supplied from this release — never after a marker claiming a dispatch.

• --model still governs. The definition carries model: fable; the dispatcher's --model overrides it exactly as it does for a file-loaded agent (probe: the exact JSON plus --model no-such-model-xyz 404s on no-such-model-xyz, $0).

• Memory is core's. One architect, one memory: --add-dir /opt/upsquad/upsquad-core/.claude/agents/<role> in every repo, not the empty portal directory.

• Codex has no --agents either; _do_dispatch inlines the same role_definitions text, carried from the decision rather than re-read.

Proof through the real generated launcher and the real CLI, an invalid model so the first API call 404s ($0), in a tree carrying the portal's tracked .claude/ + CLAUDE.md:

# origin/main dispatcher, upsquad-client tree — launcher log:
--agent 'principal-architect' not found. Available agents: backend-sme, claude, devops-engineer, Explore, frontend-sme, general-purpose, Plan, project-manager, qa-engineer, statusline-setup

# this change, upsquad-client tree (upsquad-admin identical) — launcher log:
[claude-code:unrecognized_model] {"model":"no-such-model-xyz","query_source":"sdk"}
dispatcher: engine reported is_error=true (terminal_reason=api_error api_error_status=404 subtype=success)
There's an issue with the selected model (no-such-model-xyz). It may not exist or you may not have access to it. Run --model to pick a different model.
usage: 0 in / 0 out / 0 cached / $0.0000 (0 total)

Getting as far as the model call is the proof: the CLI resolves --agent before it makes one, and on main it stops there.

What is and is not closed. (1) Core resolves exactly as before, so a core PR that edits .claude/agents/principal-architect.md is reviewed under its own edited copy. Closed by #3489: core's cross-repo roles come from the release too, and --setting-sources user stops the tree's copy loading at all (M3489-B; process test test_a_pr_head_edit_to_the_architect_in_core_is_not_the_definition_used). (2) Interactive sessions started inside a portal checkout still do not see the architect; pass the same --agents JSON, or launch from core. (3) product-manager now resolves on client/admin, but there is no .claude/review-briefs/product-manager.md, so a PM lane still cannot launch anywhere. It is refused honestly at decision time as review brief absent (the refusal above, added in #3487 review round 1); authoring the brief is #3486 (b).

---

9. Guards

guard	default	env	notes
kill switch	—	file /opt/upsquad/ops/dispatcher.off	checked before any API call. touch it to stop everything; rm to resume.
daily cap	12 (CLI); 0 (service)	DISPATCHER_DAILY_CAP	per UTC day; 0 means unlimited. State remains in /opt/upsquad/ops/review-dispatcher-state.json.
cycle cap	2	DISPATCHER_CYCLE_CAP	bounds how long one cycle can hold the global lock.
global lock	—	file /opt/upsquad/ops/review-dispatcher.lock	flock(1), -n. Held for the whole live cycle including the reviews it launches, so exactly one review runs at a time.
cooldown	30 min	DISPATCHER_COOLDOWN_MIN	triggers B and D.
dismissed-head stability	30 min	DISPATCHER_DISMISSED_STABILITY_MIN	triggers C and D. How long the head must have been quiet before a dismissed or proved superseded COMMENT-only lane is re-dispatched. Anchored on the head commit's committer date, never updated_at (§2).
stale	90 min	DISPATCHER_STALE_MIN	marker → retry threshold.
max attempts	3	DISPATCHER_MAX_ATTEMPTS	1 initial + 2 retries.
budget	$8	DISPATCHER_BUDGET_USD	per reviewer session.
timeout	3600 s	DISPATCHER_TIMEOUT_SECS	per reviewer session.
engine routing	.claude/review-briefs/engine-routing.toml	DISPATCHER_ROUTING	refused-not-corrected on any problem; falls back to Claude everywhere and says why (§5).
tree sweep	6 h	DISPATCHER_TREE_MAX_AGE_HOURS	review trees under DISPATCHER_TREE_DIR (default /opt/upsquad/ops/review-trees) older than this are removed at the top of each live cycle.

The daily cap limits quota consumption, not concurrent memory use. An absent setting retains the CLI default of 12; positive integers retain the daily limit. Explicit 0 removes only that daily hold. Negative or malformed values fail before the lock, filesystem changes, API calls or launches. Unlimited mode logs daily cap unlimited; N used today and continues recording dispatch usage; it does not reset or delete the state file. The cycle cap and global lock still bound each cycle and serialize reviewer sessions.

Why the lock is not optional — earlyoom

This devbox is memory-pressured and earlyoom prefers killing claude|node. Two concurrent reviewer sessions is how you lose both, and the second death looks like a flaky reviewer rather than an OOM. The global flock makes concurrency impossible rather than unlikely.

Observe mode does not take the lock — it writes nothing and must never be able to starve a live cycle.

There is no earlyoom exemption for reviewer sessions. That is a founder-run config change and was explicitly out of scope. Until it exists, a killed session shows up as a marker that goes stale with no review, and the retry path handles the dispatch half; the disposable tree (§3) and the sweeper below handle the filesystem half.

The tree sweeper — the belt to the trap's braces

The launcher's trap covers every exit path the launcher survives, including an earlyoom kill of its claude child. It cannot cover the launcher itself being SIGKILLed — earlyoom hitting the shell, a reboot, a kill -9. That is the only way a review tree outlives its review, and this sweep is the only thing that reclaims it.

Age is the directory's own mtime: not a manifest file (the same kill could have prevented writing it) and not the timestamp in its name (a rename breaks that). A tree with a future mtime — clock skew — is not stale; the comparison is one-sided on purpose.

Why 6 h and not 90 min. A directory's mtime moves only when an entry is created or deleted directly in it; a reviewer editing a file two levels down does not refresh it. So a running review does not reliably keep its own tree looking fresh, and the safety of the sweep rests on the threshold being far larger than the longest a review can last: timeout_secs 1 h vs 6 h. test_the_sweep_threshold_cannot_reach_a_running_review asserts at least 2x headroom, so raising the session timeout past it turns something red rather than silently deleting a live tree. (The sweep also runs at the top of a cycle, before any launch, and the global flock means no other cycle's review is in flight — so it cannot race a live reviewer even before the margin is considered.)

Observe mode reports stale trees and removes nothing — a dry run must not mutate the filesystem any more than it mutates GitHub. test_observe_mode_reports_stale_trees_but_removes_nothing.

Disk

The box runs at ~85–89% used, and agent-worktree.sh refuses at 95%. A review tree is ~84 MB (measured on upsquad-core; the git object store is shared, so this is working-tree files only).

Because the trap removes the tree on every exit path it survives, and the sweeper removes what is left after the paths it does not, the steady-state footprint of /opt/upsquad/ops/review-trees is one tree — the running review's — and the residue is one tree per SIGKILLed launcher until a later live cycle sweeps it after 6 hours. With an unlimited daily quota there is no daily-count-derived disk bound. The checked-in service still runs at most one review per cycle on the 15-minute timer; manual invocations can add cycles, and a stopped dispatcher cannot sweep. Monitor actual residue rather than relying on the former 12/day footprint estimate. The normal successful-cleanup footprint remains one tree.

Watch it with:

du -sh /opt/upsquad/ops/review-trees 2>/dev/null; ls /opt/upsquad/ops/review-trees
git -C /opt/upsquad/upsquad-core worktree list | wc -l   # should not grow per review

If that count grows monotonically across reviews, the trap is not firing — check for an exec in the generated launcher (see §4) before anything else. Logs under /opt/upsquad/ops/review-dispatcher-logs/ are plain text and small, but nothing rotates them; delete them by hand when they bother you.

---

10. Observe → live

Temporary engine outage exception — SUPERSEDED (#3340 → #3397)

The two environment variables this section used to document now REFUSE TO START the dispatcher. DISPATCHER_CLAUDE_UNAVAILABLE and DISPATCHER_OUTAGE_AUTHORIZATION are in RETIRED_ENV_VARS; exporting either, or putting either in a systemd drop-in, makes Config.from_env raise.

This section is kept rather than deleted because the drop-in it described was really installed, and someone will find that file or a shell history entry and need to know why it no longer works. The instructions below are history, not procedure. The live mechanism is §14 "Outage override — tracked, expiring, login-verified", which replaced a URL-shape regex with the authorizing comment's author login checked against a founder allowlist.

The 2026-09-09 founder ruling that authorized independent Codex reviewers to cover Claude lanes stands on its own terms; what changed is only how an override is expressed and verified. It never authorized self-review, omitting reviewer roles, merging without independent approval, or bypassing founder product, security, dependency or production gates.

Historical: how the retired mechanism was enabled (do not use)

# RETIRED — both of these now raise on startup.
export DISPATCHER_CLAUDE_UNAVAILABLE=1
export DISPATCHER_OUTAGE_AUTHORIZATION=https://github.com/.../issues/2991#issuecomment-5608005981

; RETIRED — ~/.config/systemd/user/review-dispatcher.service.d/claude-outage.conf
[Service]
Environment=DISPATCHER_CLAUDE_UNAVAILABLE=1
Environment=DISPATCHER_OUTAGE_AUTHORIZATION=https://github.com/.../issues/2991#issuecomment-5608005981

If that drop-in still exists on a host, delete it and run systemctl --user daemon-reload; otherwise the service will fail to start.

Return to normal: when Claude coordination resumes, stop future timer cycles and let an active review finish. Verify Claude's real CLI smoke, remove only this dedicated outage drop-in, unset both variables in the operator shell, and run systemctl --user daemon-reload. Verify neither setting remains in the loaded service environment, inspect an observe decision for a normally-Claude lane, record the handover/removal on #2991, and re-enable the timer. Merely reaching a quota-reset date is not evidence Claude is available. Do not edit the permanent routing TOML to implement or remove the exception.

Observe-only is the default. Without DISPATCHER_LIVE=1 the script makes read-only API calls, prints the decision transcript, and exits.

# observe (safe anywhere, any time)
python3 scripts/review-dispatcher.py --once

# observe one repo / one PR
python3 scripts/review-dispatcher.py --once --repo upsquad-ai/upsquad-core --pr 3332

# live
DISPATCHER_LIVE=1 python3 scripts/review-dispatcher.py --once

That the observe path writes nothing is enforced by tests, not by reading the code — on both surfaces:

• GitHub: the stubs in CycleTests raise on post_comment, patch_comment and launch, so a regression that starts writing fails in CI rather than on a live PR.
• the filesystem: test_observe_mode_reports_stale_trees_but_removes_nothing puts a 9-hour-old tree in front of an observe cycle and asserts it is still there afterwards.

Suggested rollout

1. Observe for a few cycles. Read the transcript; every DISPATCH line is a session you are about to authorise.
2. Live, scoped: --repo upsquad-ai/upsquad-core --pr <one PR> with DISPATCHER_CYCLE_CAP=1, watched. Check afterwards that /opt/upsquad/ops/review-trees is empty and git -C /opt/upsquad/upsquad-core worktree list | wc -l is unchanged — that is the trap working, and it is the thing most worth confirming on the first live run.
3. Live, core-wide with the default caps.
4. Client/admin: the agent-definition gap in §8 is closed (#3334). Widening the service beyond --repo upsquad-ai/upsquad-core is still its own step — observe the portal repos first, then one scoped live PR, exactly as 1–2 above.

touch /opt/upsquad/ops/dispatcher.off is the abort at every stage.

Scheduling — opt-in only after independent review and merge

The original #3332 intentionally omitted installation. The #3337 takeover includes user-service/timer files under scripts/systemd/ because the founder explicitly requested the switch be in effect. Checking in these files does not install or start them. Keep them inactive until the implementation is independently approved and merged, and either both engines are ready or the explicit founder-authorized outage procedure above is verified.

Use a separate detached release worktree at the approved merged SHA, under /opt/upsquad/ops/review-dispatcher-releases/<sha>, with /opt/upsquad/ops/review-dispatcher-current pointing to it. Do not switch, pull or build in shared /opt/upsquad/upsquad-core. The service explicitly selects the release's routing config, and template loading prefers the dispatcher script's own release tree. Target shared checkout paths remain only the repositories' object stores and role-memory sources.

Activation checklist (coordinator-operated under AGENTS.md):

1. Verify the PR is merged and the exact merged SHA; create that detached release tree and preserve any previous release link for rollback.
2. Verify both installed CLI engines can execute their shell smoke. During the explicit authorized outage exception above, verify Codex and record Claude's unavailability plus the ruling; otherwise a Claude quota refusal blocks readiness. Never silently change engine policy to evade a refusal.
3. Point the release link at that SHA. Run DISPATCHER_LIVE=0 DISPATCHER_ROUTING=<release>/.claude/review-briefs/engine-routing.toml python3 <release>/scripts/review-dispatcher.py --once --repo upsquad-ai/upsquad-core; inspect each engine decision and refuse any configuration fallback.
4. Run one authorized live PR with DISPATCHER_CYCLE_CAP=1, as in the scoped rollout above. Verify the marker's engine/head, the actual review body's Review engine: line, successful launcher exit, and disappearance of that dispatch's tree/ref. A printed DISPATCH line alone proves no reviewer ran.
5. Copy the approved review-dispatcher.service and .timer into ~/.config/systemd/user/, run systemd-analyze --user verify on them, then systemctl --user daemon-reload and systemctl --user enable --now review-dispatcher.timer.
6. Read systemctl --user status review-dispatcher.timer and journalctl --user -u review-dispatcher.service after the scheduled run. Record release SHA, loaded config, timer state, marker and completed review URL. Only this combination supports in effect.

The unit starts with core-only scope (the client/admin role-definition gap is closed by #3334, but widening --repo is a separate, observed step — §10 item 4), one launch per cycle and DISPATCHER_DAILY_CAP=0 (unlimited daily launches, #3342). The 15-minute timer and sequential lock are unchanged. It explicitly enables DISPATCHER_LIVE=1; direct CLI invocation remains observe-only by default. No unit is installed during tests. Stop future cycles with systemctl --user disable --now review-dispatcher.timer or the existing kill switch. For rollback, stop the timer, wait for a running service to finish, restore the previous release link, repeat observe verification, then re-enable it. Do not remove a release tree while its service is running. When rolling back to a release before #3342, restore its service configuration too: the old code treats daily cap 0 as exhausted, not unlimited. Preserve the accounting file through either direction.

An observe cycle uses API calls but launches no model sessions; routing candidates also require detail, commit and file-page reads.

---

11. Operating it

A PR did not get a reviewer. Run observe scoped to it — the transcript names the exact reason:

python3 scripts/review-dispatcher.py --once --repo upsquad-ai/upsquad-core --pr <n>

no routing comment is the most common answer, and it is correct: the dispatcher does not invent routing. Somebody has to say who should review. Post the routing comment.

A reviewer was dispatched but never posted. Check /opt/upsquad/ops/review-dispatcher-logs/<repo>-<pr>-<lane>-<ts>.log — that is the session's full output, and the launcher writes its own failures there too. Look for launcher: head moved ... (rc 75 — expected, not a fault) or launcher: fetch ... failed / could not create the review worktree (rc 78). The marker will go stale after 90 min and retry twice before flagging NEEDS-HUMAN.

Review trees are piling up. Each one is ~84 MB. Either the trap is not firing (check §4 for a stray exec) or launchers are being SIGKILLed. The next live cycle sweeps anything over 6 h; to see what would go without waiting, run observe: it prints would sweep stale review tree ... and removes nothing.

Stop everything: touch /opt/upsquad/ops/dispatcher.off.

A stuck lock (a cycle killed mid-review leaves nothing behind — flock releases on process exit — so a persistently-held lock means a live cycle really is running): fuser -v /opt/upsquad/ops/review-dispatcher.lock.

Remove a configured daily quota: set DISPATCHER_DAILY_CAP=0 on a release that supports unlimited mode, reload the user unit and verify the loaded setting and cycle log. Retain /opt/upsquad/ops/review-dispatcher-state.json; deleting it discards accounting rather than removing the quota. Restoring a positive cap uses the recorded count for that UTC day immediately. The kill switch, cycle cap and sequential lock continue to apply in either mode.

GitHub authentication during long reviews (#3535)

Before every GET, marker POST or marker PATCH, the dispatcher consults the absolute scripts/gh-token.py in the target checkout as its calling bot role. That helper alone owns token caching and its 300-second expiry buffer; the GitHub client keeps no credential across requests or reviewer sessions. Inherited GH_TOKEN, GITHUB_TOKEN and GH_DEBUG are discarded. Helper failures/empty output stop the request without logging helper output. During per-PR reads, including trigger/provenance enrichment, that PR is skipped for the cycle; later PRs and repositories are still attempted. It is never treated as absent provenance or a NEEDS-HUMAN decision. Initial repository reads retain their existing repository-skip behavior. The 60-second helper and 30-second HTTP bounds remain.

A 401 or uncertain write outcome remains an error, never an empty successful result. There is no automatic replay: a marker failure prevents its reviewer launch. Diagnose the failed request and inspect the actual comment receipt before any separately authorized recovery; an unknown outcome does not license a second mutation. Source verification does not activate a release: activation still follows the reviewed-release procedure in §10.

---

12. Tests

python3 -m unittest discover -s tests/scripts -t .

Runs in the existing Python unittest (scripts) required lane (a whole-tree unittest discover, no paths: filter), so no workflow change was needed.

Fixtures under tests/scripts/fixtures/review_dispatcher/ are recorded from the live API on 2026-09-09 and chosen for the shapes that break naive parsers:

fixture	the shape
core#3266	dual lane with two @-mentions; architect DISMISSED→APPROVED, QA COMMENTED only.
core#3328	routes to the architect while the body says "test files/fixtures → qa-engineer primary" as bare prose. A parser matching role names dispatches a QA lane nobody asked for — on a PR whose author is the QA bot, so the wrong lane is also a self-review.
core#3293	"migration + schema ⇒ dual review" with exactly one @-mention. This is why dual review is not a synonym for dual lane: the second reviewer there is explicitly "welcome", i.e. optional.
client#858, #863	the plain shape, ending "Coordinator: please dispatch the reviewer; this comment records routing only" — the gap this script exists to close, stated by the author.

FixtureIntegrityTests asserts each fixture still contains the trap it was recorded for, so the tests above cannot quietly become vacuous.

Mutation evidence

Every mutant killed, each by its named scenario. The count is not written down here — it was "twenty-one" while the roster had grown past it, which is the class of staleness this runbook keeps warning about elsewhere. Re-derive it:

python3 -c 'import importlib.util,sys
s=importlib.util.spec_from_file_location("t","tests/scripts/test_review_dispatcher.py")
m=importlib.util.module_from_spec(s);sys.modules["t"]=m;s.loader.exec_module(m)
print(len(m.MUTANTS), [x[0] for x in m.MUTANTS])'

The mutant is applied to a copy in a temp dir — the working tree is never modified, so a crashed run cannot leave a mutant behind and concurrent lanes are unaffected. Each mutant's target string must occur exactly once in the source: a substitution that no-ops turns a should-be-RED case into a meaningless GREEN.

The oracle is narrow: the mutated module must still import, and the scenario must raise AssertionError. A SyntaxError/AttributeError means the mutant broke the module rather than the guarantee, and proves nothing.

mutant	removes	killed by
MD1	restore permanent COMMENT-only skip	SupersededCommentTests.test_old_skip_mutant_is_killed_by_recorded_witness
MD2	remove expected reviewer-login filter	test_author_review_cannot_replace_pinless_lane_comment_causal
MD3	remove D stability while cooldown stays 30 min (stability 60 / head 45)	test_stability_is_independent_of_cooldown_causal
MD4	accept non-dispatched provenance as a dispatch	test_only_dispatched_provenance_is_evidence_causal
MD5	let a recovery completion omit its explicit pin	test_recovery_completion_requires_its_explicit_pin_causal
M1	the marker's head-pinning	double_dispatch_prevented
M2	the self-review refusal	author_is_reviewer_refused
M3	the cooldown comparison	cooldown_holds
M4	the COMMENTED exclusion (CLAUDE.md's read)	commented_excluded_from_verdict
M5	routing-line scoping of lane extraction	routing_line_scoped
M6	the sweeper's age comparison — flipped, it reclaims the running review's tree and keeps every leaked one	sweeper_reclaims_only_stale_trees
M7	the no-self-model rule — Codex-authored PRs fall through to the class default	codex_authored_is_never_reviewed_by_codex
M8	the CLAUDE_ONLY_CLASSES roster itself	security_critical_never_leaves_claude
M9	trailer detection reads only the last commit, not any commit	trailer_in_a_middle_commit_counts
M10	the truncation fail-safe — an unseen commit list reads as Claude-authored	truncated_commit_list_fails_safe
M11	complete engine input paging	complete_engine_inputs
M12	the decisive marker lock across engine changes	engine_switch_keeps_decisive_lock
M13	security class precedence over configured rank	security_class_precedence
M14	security pattern precedence over a routine catch-all	security_match_precedence
M15	renamed source path classification	renamed_security_path_keeps_claude
M16	missing rename source-path refusal	renamed_missing_previous_fails_closed
M17	process-group cancellation (kills only sg)	LauncherProcessTests.test_preparation_timeout_cannot_start_a_reviewer_after_lock_release
M3399-A	the usage: prefix — the summary line goes out as ERROR: usage limit reached …, so every completed review trips the global kill switch	usage_logging_cannot_trip_the_breaker
M3399-B	the usage path truncates the launcher log instead of appending — review record and breaker input destroyed at the moment the review completes	usage_logging_cannot_trip_the_breaker
M3399-C	the engine's own text is dropped when is_error is true — the quota signature, which arrives INSIDE the JSON result, never reaches the log	usage_logging_cannot_trip_the_breaker
M3399-D	the engine's text is written with a dispatcher: prefix on every line — present, readable, and permanently out of the anchor's reach	usage_logging_cannot_trip_the_breaker
M3399-E	the appended block no longer leads with a newline, gluing the first appended line off column 0	usage_logging_cannot_trip_the_breaker
M3399-F	the codex parser stops flagging a log with two different totals	usage_parsers_flag_what_they_cannot_vouch_for
M3399-G	a usage block whose counters do not add up to a total stops being flagged — a null total the coverage recipe reports as fine	usage_parsers_flag_what_they_cannot_vouch_for
M3399-H	the API Error: <status> prefix branch — the Claude breaker goes back to being unable to fire on a real wall	claude_signature_matches_the_cli_template_only
M3399-I	the column-0 anchor on the Claude pattern. It had no direct witness before #3443 (the sibling review's N-f); a wider prefix is the wrong moment to leave that so	claude_signature_matches_the_cli_template_only
N1 (rc unbound)	the rc initialization before launch's try — the interrupt is masked by UnboundLocalError and the ephemeral dir leaks	LauncherProcessTests.test_an_interrupt_during_wait_is_not_masked_and_leaks_nothing, via test_unbound_rc_mutant_is_killed
MC1	trigger C's DISMISSED conjunct — any lane with a verdict becomes eligible, so every approved PR on the board is re-reviewed every window	trigger_c_only_fires_for_a_dismissed_verdict
MC2	the open-PR conjunct — a closed PR is re-reviewed	trigger_c_refuses_a_draft_and_a_closed_pr
MC3	the draft conjunct. decide() returns SKIP/draft before the lane loop, so only a predicate-level witness can see this one	trigger_c_refuses_a_draft_and_a_closed_pr
MC4	the dirty conjunct — a review is spent on a head that cannot land	trigger_c_never_spends_on_a_conflicting_head
MC5	trigger C's marker-as-lock — one dismissal re-dispatches every cycle until the daily cap eats it. _decide_lane holds on the same marker first, so again only the predicate can witness it	trigger_c_is_one_dispatch_per_dismissal
MC6	the fail-closed on an unknown head date — "could not establish that the head settled" reads as "settled a day ago"	trigger_c_holds_when_the_head_age_is_unknown
MC7	the stability window, inverted — the dispatcher fires into a push series and waits once the head settles	trigger_c_waits_for_a_stable_head
MC8	eligible_since from the ordering key — service order becomes lane name, i.e. the same lane first every cycle, which is the starvation #3422 was about	trigger_c_serves_the_oldest_eligible_pair_first
MC9	latest_review_by_reviewer takes the FIRST row per reviewer. eligible_since becomes the round-1 block and the service order inverts — the starvation returns through the key's source rather than the sort. Survived all 614 before this row (QA 5248327433, their QM5)	eligible_since_is_the_dismissed_row_on_a_multi_round_lane
MC10	the COMMENTED exclusion in latest_review_by_reviewer — eligible_since jumps to the COMMENTED row's stamp (~10 h newer) and the lane sorts last. Every pre-existing COMMENTED pin is on the verdict twin, which is why the same mutation there already died and this one did not (QA QM11)	same
MC11	the stability window's boundary minute (< → <=). Survived the lane, reddening only through roster refusals — a roll-call RED that is really a target miss (QA QM3)	trigger_c_waits_for_a_stable_head
MC12	from_env stops reading dismissed_stability_min — a knob that is present, documented, and silently does nothing (QA QM12)	every_config_field_is_reachable_from_the_environment
M3446-A	the structured branch — the Claude path goes back to prose only, i.e. back to being structurally incapable of firing on the real 429 wall	the_structured_quota_oracle_fires_on_the_real_dead_log
M3446-B	the status conjunct, loosened from 429 to any three-digit status — an ordinary 404 or 500 writes the global kill switch	the_structured_quota_oracle_reads_only_our_line
M3446-C	the column-0 anchor on the structured pattern — a reviewer quoting or indenting the notice trips the breaker on a healthy run. Rostered because #3443 found the prose Claude pattern's anchor unwitnessed in 276 tests; adding a detector is the wrong moment to repeat that	same
M3334-A	from_release — nothing is read from the release; cross-repo roles fall through to each repo's main (absent on the portals, core's own copy in core)	every_role_resolves_from_a_source_the_pr_cannot_write
M3334-B	retired by #3489 — "core is supplied from the release too" is now the intended behaviour	—
M3334-C	retired by #3489 — no code path reads a definition from any checkout any more	—
M3334-D	the decision-time conversion check — an unconvertible main definition is admitted and fails after the marker	every_role_resolves_from_a_source_the_pr_cannot_write
M3334-E	the unmapped-key refusal — a frontmatter key is silently dropped	the_cli_gets_the_core_definition_unchanged
M3334-F	tools list conversion	same
M3334-G	--agents in the Claude block — with project settings off, every launch exits not found after the marker	every_claude_launch_is_defined_by_agents_not_the_tree
M3334-H	core's memory dir for a cross-repo role	same
M3334-I	the build-time guard against a Claude launch with no --agents file	same
M3334-J	the Codex brief's role contract	codex_inlines_the_resolved_definition
M3334-K	the release pin — CORE_AGENTS_DIR points at the shared core checkout (review 5257255250's Y1, which survived 291 tests)	the_definition_source_is_this_release
M3334-L	the decision-time review brief absent refusal — a portal PM lane gets a dead "dispatched" marker and spends a cap slot (#3486)	a_lane_without_a_brief_is_refused_before_any_marker
M3334-M	brief resolution claims every lane has one	same
M3334-N	the cycle never hands the brief set to decide() — refusal present, unreachable; only the live-cycle half sees it	same
M3334-O	the empty-description/body refusal (review N1, its Y8)	the_cli_gets_the_core_definition_unchanged
M3334-P	the parenthesised-tool-spec refusal (review N3)	same
M3489-A	--setting-sources user — the PR head's .claude/settings.json hooks run in its own reviewer's session, under upsquad-devs, before the model reads anything. Also witnessed at process level: test_the_setting_sources_mutant_lets_the_planted_hook_run	every_claude_launch_is_defined_by_agents_not_the_tree
M3489-B	core reads its cross-repo roles from its own main again — the self-supply hole	every_role_resolves_from_a_source_the_pr_cannot_write
M3489-C	read_main_blob's payload-shape check — a symlink or a >1 MB file decodes to nothing and reads as "role absent" instead of skipping the repo (review 5258304758 N3 found this unwitnessed; N1 noted the missing id)	a_definition_read_failure_skips_the_repo
M3489-D	any failure reading main is taken as "absent" — a transient 500 parks lanes with NEEDS-HUMAN markers	a_definition_read_failure_skips_the_repo
M3489-E	the cycle carries on after a read failure with no definitions — every lane in the repo refused	same
M3489-F	_do_dispatch posts a "dispatched" marker for a lane it carries no definition for	no_marker_without_a_definition

MC9/MC10 target a function that predates trigger C. They are rostered here because this PR is what makes latest_review_by_reviewer load-bearing: it is now the SOURCE of the fairness key that MC8 and four paragraphs of §2 defend, and a guard whose input is unpinned is a guard on half the property.

Name the invocation that COLLECTED the witness, not just the result. QA lost most of a round to this: running test_review_dispatcher_outage as a module reports Ran 14 tests … OK under mutants whose witnesses live in another module's ScenarioTests. A module-scoped green is not a lane-scoped green.

mutant	collected by
M3397-A … M3397-F	python3 -m unittest tests.scripts.test_review_dispatcher.MutationTests
M18-outage-* … M20-outage-*	python3 -m unittest tests.scripts.test_review_dispatcher_outage.OutageMutationTests
M17 (process group), M3489-A at process level	python3 -m unittest tests.scripts.test_review_dispatcher_processes
the real-CLI half of #3489 (opt-in, $0)	DISPATCHER_REAL_CLI=1 python3 -m unittest tests.scripts.test_review_dispatcher_processes.RealCliProofTests
MC1 … MC12 (trigger C + QA round 2), M3446-A … M3446-C, M3334-*, M3489-*	python3 -m unittest tests.scripts.test_review_dispatcher.MutationTests
everything, together	python3 -m unittest discover -s tests/scripts -t .

#3397's mutants are PR-scoped (M3397-*) rather than numbered: M17 already meant a process-group mutant in test_review_dispatcher_processes.py, and an id that means two things across one lane will mislead the next person to re-score it. | M18 | explicit outage override ignored | authorized_fallback | | M19 | outage override always on | default_keeps_claude | | M20 | self-review refusal removed during outage | outage_still_refuses_self_review | | M21 | old unconditional daily comparison blocks unlimited mode | unlimited_still_launches_and_accounts |

M8 is worth reading before you write your next mutant. The first version mutated the if klass in CLAUDE_ONLY_CLASSES branch inside select_engine and survived — EngineRouting.engine_for_class reads the same roster and returns claude anyway. The second version emptied the roster and also survived — the shipped config also says claude for that class. Three independent layers hold the property, and the scenario was measuring the shipped default rather than the guard.

The fix was to the SCENARIO, not the mutant: it now hand-builds a routing that assigns codex to security-critical, bypassing the loader, and asserts select_engine still says claude. That is what the roster uniquely does — make a config that says otherwise ineffective — and it dies correctly when the roster is emptied. test_security_critical_is_guarded_in_two_independent_places records the redundancy so a reader who sees a surviving single-guard mutant does not conclude the guard is untested.

A second test prints which other scenarios each mutant trips. That column is a diagnostic, not a gate — it is what makes a wrong witness visible on sight. M2 trips three scenarios because inverting the author check breaks everything downstream of it; that is reported rather than hidden.

---

13. Conventions

The Review engine: attribution line

Every review posted by a dispatched reviewer must carry, in its body:

Review engine: Codex (gpt-6-astra)

It mirrors the Co-Authored-By: commit trailer that records which model wrote the code, so review quality is auditable per model the same way authorship already is. When a review was routed by an outage exception it carries a second line naming the intended engine and the authorization.

This lives here rather than in CLAUDE.md deliberately: it is an operational convention of one tool, and CLAUDE.md is agent context that every session pays for.

It is an instruction, and instructions are not mechanisms — so since #3397 the dispatcher reads the posted review back and records the verdict on its own marker:

marker row	meaning
**Engine disclosure**: ok — disclosed as codex	the line is there and matches
**Engine disclosure**: NOT DISCLOSED — the review body carries no \Review engine:` line`	reviewer did not comply
... — no review from this lane after the dispatch	the reviewer never posted
... — review claims engine 'claude', dispatched on 'codex'	disclosure contradicts the dispatch

The motivation is measured, not theoretical. Of 118 bot reviews in the 2026-09-10/11 outage window, 59 carry an outage line and 59 do not, and the absence has three different explanations that the record cannot tell apart. On #3364 the DISMISSED review discloses and the APPROVE that actually cleared the PR does not.

---

14. The #3397 controls

Per-class model and effort

[classes.*] may set model and (codex only) reasoning; the engine table is the fallback. Effort tracks stakes:

class	engine	model	effort
security-critical	claude	fable	—
backend	codex	gpt-6-astra	ultra
frontend	codex	gpt-6-astra	high
tests / docs	codex	gpt-6-astra	medium
gen-only	codex	gpt-6-astra	low
(routine claude, e.g. under an outage)	claude	opus	—

Founder tiering, 2026-09-12: fable for architecture and adjudication, opus for execution. security-critical is where judgement is the work, so it overrides back to fable.

Unknown values make the loader refuse the whole file and fall back to Claude-everywhere, loudly — the same posture as the CLAUDE_ONLY_CLASSES gate, and for the same reason: by the time a bad --model reaches the CLI, a marker has been posted and a worktree built, so the failure looks like a dead reviewer rather than a typo. reasoning on the claude engine is refused outright rather than ignored, because that launcher has no such flag and the setting would silently never take effect.

Both value rosters (CODEX_REASONING_LEVELS, CLAUDE_MODELS) are snapshots and will go stale. That is the accepted cost of refusing unknowns, and it fails in the safe direction: a genuinely-new level is refused loudly. Re-derive the codex side from ~/.codex/models_cache.json — the command is in the source comment.

Head-churn debounce — at most one dispatch per PR per 60 min

#3396 took four dispatches in one hour — 00:15, 00:30, 00:45, 01:15 — all trigger=A, each at a different head, with cooldown_min=30 configured.

The tempting diagnosis is "a new head reset the cooldown". It is wrong, and the right one matters: trigger A never had a cooldown. cooldown_min is read in exactly one place, the trigger-B branch. Trigger A's only suppressor is the marker, and the marker is head-scoped on purpose — that is the mechanism that lets it expire so trigger B can fire at all. An author pushing every fifteen minutes therefore re-arms trigger A every fifteen minutes, and each dispatch is individually correct.

So the fix could not be "extend the cooldown" (does not apply to trigger A) or "make the marker head-blind" (breaks trigger B). It is a separate, head-blind, PR-level rate limit: DISPATCHER_DEBOUNCE_MIN, default 60, across all lanes and engines. Last-head-wins falls out for free — nothing records a head, so when the window expires the next cycle dispatches at whatever the head is then. Intermediate heads are skipped rather than queued, which is correct: nobody needs a review of a tree the author has already replaced.

scenario_head_churn_debounced replays #3396's real markers. Mutants M3397-A (debounce removed) and M3397-B (window comparison flipped) both die on it.

The limit is per-PR, not per-lane, and that needed its own witness: QA re-keyed _pr_debounce_holds per lane and all 514 tests stayed green, because scenario_head_churn_debounced ends in only(...) — one decision — and a one-lane scenario cannot separate the two. scenario_debounce_is_per_pr_not_per_lane puts two lanes on one PR inside the window; M3397-E re-keys per lane and dies on it. Two lanes each dispatching once is two launches, and launches are the quantity a memory-pressured box cannot afford.

Quota circuit breaker

A usage-limit signature in a launcher log → one NEEDS-HUMAN marker naming the quota and the reset date → touch /opt/upsquad/ops/dispatcher.off → cycle returns. The real line:

The CLI emits, at column 0 and as the last thing before it dies (shown here with the middle of the phrase elided on purpose — see the warning below):

ERROR: You<'>ve hit your usage li<m>it. Visit https://chatgpt.com/codex/settings/usage
to purchase more credits or try again at Sep 16th, 2026 7:56 PM.

Do not "fix" that elision. This file is corpus. A launcher log that captures runbook text — a reviewer reading its own documentation — would otherwise carry a line the detector could match, and the detector trips the global kill switch. QA caught exactly this: the first version of the detector matched the phrase as a substring anywhere in the log, and this runbook contained it twice while the merge base contained it zero times.

The detector is now structural (anchored ERROR: at line start, tail only), so the elision is belt to that braces rather than the only defence. Keep both.

Matched on the invariant half of the sentence — the URL and "purchase more credits" are marketing copy and will churn; the claim itself is the stable part. The reset date is captured verbatim rather than parsed into a datetime: a date we failed to parse would be dropped, and telling a human when is the entire value of the marker.

The lesson, in QA's words, is bigger than this detector: "Corpus scoring is evidence about the past; it is not evidence about a diff that adds new text to the corpus." Scoring the detector against 6 dead and 22 healthy logs said nothing about the markdown file shipping in the same commit.

The oracle is deliberately narrow. The breaker trips the global kill switch, so a false positive stops every review until a human clears it. "Non-zero exit" or "the word quota appeared" would do exactly that; scenario_ordinary_failures_do_not_trip_the_breaker pins four failures that must not trip it.

To recover: confirm the reset time has passed, then rm /opt/upsquad/ops/dispatcher.off. The file itself says so and names the date.

The Claude side could not fire at all until #3443

Everything above is the codex detector, which has a corpus of six real dead logs behind it. CLAUDE_QUOTA_SIGNATURE_RE never had one — #3397 said so in as many words — and the QA lane then measured what the CLI actually emits. Claude Code renders API errors, the plan-quota wall included, through its own template:

API Error: <3-digit status> <message, sometimes the provider's JSON>

The detector required ERROR: / Error: at column 0. End to end, against the real article: no match. The launcher delivered the signature into the log, the breaker read the log, and nothing could ever have tripped — a control that was present, tested, and structurally incapable of firing.

The prefix now accepts that template too. Two conjuncts, and only the first one changed:

conjunct	what it does
PREFIX	a form only the CLI emits, at column 0: ERROR:/Error:, or the API template followed by a three-digit status. The status proves the line is the template rather than prose about it; it is not restricted to 429, because the next conjunct is what decides.
PHRASE	the quota wording (usage limit …, out of credits/usage, exceeded your current quota) — untouched. This is what stops a 404 or a 500 from halting every review on this box.

The two prefix branches get different gap widths (40 characters after ERROR: , 200 after a status) because an API-error body is sometimes the provider's JSON, whose preamble alone exceeds 40. . never matches a newline, so even the wide branch cannot reach off its own line.

The elision rule extends to this form, and it now matters more. Widening a detector widens what "armed" means. The template above is written with <3-digit status> instead of a number on purpose: with real digits and quota wording on one line, this paragraph would itself be a line the breaker matches, and a reviewer reading this runbook would stop every review on the box. Sub-case (g) of scenario_claude_signature_matches_the_cli_template_only enforces that across the runbook, the script and both test modules — it goes red if anyone "fixes" the placeholder.

Still inferred, and stated as such: the template is measured, the wording after the status is not. A false negative costs one wasted launch; a false positive stops every review — which is why the phrase conjunct was left exactly as it was.

…and then a real wall arrived, and it missed anyway (#3446)

The paragraph above used to end "nobody has driven a real plan-quota wall through this box". On 2026-09-18 one did. The live dispatcher launched the architect lane on #3445 at 13:15:25Z; the engine died 19.5 s later on a provider 429; the breaker did not trip; the lane relaunched into the same wall. 3 dead dispatches, $0.857 booked, and the transcript read healthy throughout.

Both conjuncts missed at once, which is why the answer was not a fourth widening:

conjunct	why it missed
PREFIX	the wall message has no ERROR: / API Error: <status> prefix at all. It is plain prose the CLI prints as it dies.
PHRASE	it says "reached your <PLAN-NAME> limit" — a plan-named limit. None of usage limit reached, out of credits/usage, exceeded your current quota appears.

The provider's prose is not a contract. It is copy that changes without notice, and every round of chasing it enlarges the self-arming surface.

The fix is a structured oracle on a line we write ourselves — the direction the architect named in the #3443 review. record_launch_usage already wrote this into the log as a record; #3446 promotes it to the breaker's input:

dispatcher: engine reported is_error=true (terminal_reason=api_error api_error_status=<status> subtype=success)

<status> is a placeholder, and the elision rule is why. With 429 there instead, this paragraph would itself be a line the new detector matches at column 0, and a reviewer reading this runbook near the end of its own log would write the global kill switch on a healthy run. Sub-case (e) of scenario_the_structured_quota_oracle_reads_only_our_line sweeps the runbook, the script and all four dispatcher test modules, and goes red if anyone "fixes" the placeholder.

Four properties, each doing work:

property	what it buys
it is ours	IS_ERROR_NOTICE_HEAD is shared by is_error_notice() (the writer) and CLAUDE_STRUCTURED_QUOTA_RE (the reader), so "a detector matching a line nobody emits" is unreachable by construction rather than merely tested for.
it is deterministic	is_error and api_error_status are fields of the CLI's own JSON. They do not get reworded.
column 0 → end of line	a reviewer quoting the notice mid-sentence, indenting it, or continuing past it on the same line cannot trip the breaker.
exactly 429	a 404 or a 500 is an ordinary API error and must not stop every review on this box; 4291 is not 429.

Two things it deliberately does not do:

• It cannot tell a plan wall from a transient per-minute 429, and treats both as "stop". Either way the engine died on the provider refusing us, the launch is already paid for, and the alternative is relaunching into it — ~$0.85 per dead launch against one human rm of a file.
• It is not gated on the launcher's exit code. Tempting — #3446 raises it — and rejected: nobody has established that a CLI reporting is_error=true always exits non-zero, and an rc gate that is wrong goes blind in exactly the situation the breaker exists for. That is the same "present, tested, structurally incapable of firing" failure this change is fixing. The notice is written only when the engine reported an error at all, which is the gate that is known to hold.

The prose pattern is kept unchanged and tried second — it still covers the shapes it was measured against — and the codex path is untouched: its launcher writes no such notice, so a structured pattern there would be a detector for a line that engine never emits.

Reading the marker. A structured hit has no try again at … to parse, so the NEEDS-HUMAN marker says reset at unknown and carries the raw notice line under Signature:, which names the status and the terminal reason. That is the diagnostic; a reset date is not available on this route.

One masked guard, found and fixed in the same commit. Adding this oracle made M3399-C (drop the engine's text when is_error is true) survive: the breaker fired off the structured line whether or not the prose route worked — two guards where the new one masks the old one. The case that witnesses M3399-C now uses a non-429 is_error, which takes the structured oracle out of the picture and leaves the prose route as the only way that signature can reach the breaker. It is commented in place, because "change it back to 429" is a one-character edit that silently un-witnesses a guard.

Codex budget — what does NOT exist

codex exec 0.154.0 has no --max-budget-usd, no token cap, and no spend config key. Verified against codex exec --help. So the Codex lane has two bounds and neither of them bounds spend:

bound	what it actually bounds
timeout (DISPATCHER_TIMEOUT_SECS, 3600)	wall clock
log watchdog (DISPATCHER_CODEX_MAX_LOG_BYTES, 8 MB)	output volume, i.e. log bytes

The watchdog did not work until #3397. It was pkill -TERM -P $$ -x codex, and $$ inside the subshell is the launcher shell, whose child is timeout — codex is timeout's grandchild, so the pattern matched nothing and the cap could not fire. Measured:

-x codex    -> pkill rc=1   engine rc=0    after 8s (ran to natural exit)
-x timeout  -> pkill rc=0   engine rc=143  after 1s

It now backgrounds the engine and kills by PID, which cannot drift the way a process name can — -x timeout would be correct only until the wrapper changes, which is how the original bug arose. Pinned by two real-process tests (terminates over cap; leaves an under-cap run alone) and a mutant that restores -x codex and must let the engine finish.

Reasoning tokens never reach the log, and at ultra they are most of the cost, so a run can be expensive while staying small and quick. The real spend bound on this lane is the plan quota itself — which is why the circuit breaker above exists, and why hitting the wall now stops the dispatcher instead of being retried into. Do not read the watchdog as parity with the Claude lane's --max-budget-usd.

The watchdog is detached from the launcher's stdio (>/dev/null 2>&1 </dev/null) and killed from the trap. A backgrounded subshell that inherits the parent's pipes keeps them open after the engine exits, so anything reading the launcher's output blocks until the watchdog happens to die — found by the launcher process suite hanging for its full timeout.

Outage override — tracked, expiring, login-verified

.claude/review-briefs/outage-override.toml. The environment can no longer express an override at all: DISPATCHER_CLAUDE_UNAVAILABLE and DISPATCHER_OUTAGE_AUTHORIZATION are in RETIRED_ENV_VARS and refuse to start, rather than being silently ignored — an operator who exports one believes they have enabled something.

What changed and why:

before	after
two env vars, unreviewed	one tracked file; turning it on is a PR
authorization = a URL matching a regex	authorization = the author login of that comment, fetched from GitHub, checked against founder_logins
no end date	mandatory expires; past it the override is inert and says so every cycle
converted any Claude decision	cannot convert the fail-safe (below)

The regex checked that a URL was well-formed. It is not an authorization: the authorization used for the real outage on #3341 pointed at a comment authored by upsquad-project-manager[bot] — an agent relaying the founder's words in good faith. The regex passed it; login verification does not.

founder_logins ships empty, which is fail-closed: no comment can authorize an override until someone adds a login in a reviewed PR. It is empty because it could not be populated honestly — every human-authored artefact in this repo is currently written by a bot on the founder's behalf, so there is no founder login to read off the history, and guessing one would defeat the control on its first use.

The fail-safe is exempt. Architect on #3341: "The fail-safe degrades to precisely the engine it was written to avoid, in exactly the situation it was written for." The override converted any Claude decision, including the one that means "we could not determine what this PR touches, so use the stronger reviewer". The founder authorized routing known security-critical work to the other engine; a fail-safe firing means the dispatcher does not know what the work is. Under an active outage an unclassifiable PR is now NEEDS-HUMAN. Mutant M20.

Log rotation

Three rules, because they fail differently:

rule	env	default
keep newest N	DISPATCHER_LOG_KEEP	40
max age	DISPATCHER_LOG_MAX_AGE_DAYS	14
max total size	DISPATCHER_LOG_MAX_TOTAL_MB	100

The size rule is the one that actually bounds the directory. Measured 2026-09-12 it held 26 files / 28.5 MB, and neither the 40-file nor the 14-day rule would have removed a single byte:

budget 100 MB -> rotate  0 files, free  0.0 MB
budget  20 MB -> rotate  7 files, free  8.5 MB
budget  10 MB -> rotate 13 files, free 18.5 MB

A review log runs to 1.8 MB and the daily cap is 12, so the directory grows ~20 MB/day at full tilt: the 100 MB budget is about five days of logs. The newest log is never removed by the size rule even if it alone exceeds the budget — deleting the log of the review that just ran would destroy the only record of it. Rotation runs at the top of a live cycle; observe mode reports and removes nothing.

Daily cap — a stated number, not a product of two defaults

The service unit is back to DISPATCHER_DAILY_CAP=12. With 0 (unlimited) the real ceiling was, in the architect's words, "a product of two unrelated defaults rather than a stated policy":

timer     OnCalendar=*:0/15   -> 96 cycles/day
cycle cap DISPATCHER_CYCLE_CAP default 2
budget    --max-budget-usd default 8, per launch
          96 x 2 x $8  ~=  $1,536/day

versus $8 x 12 = $96/day under the finite cap. The code keeps the ability to express 0; nothing defaults to it.

Which control bounds how many CLAUDE-only-class PRs can be decided by the fallback engine in one day? Before this change the honest answer was "the cycle cap, incidentally". Now: the daily cap (12) bounds dispatches of every kind, and an outage override is additionally bounded by its mandatory expires window and cannot touch the fail-safe at all.

---

15. The usage ledger (#3399)

Every control in §14 bounds spend. None of them measured it. The only record of what a review cost was prose inside a launcher log that rotates at 40 files / 14 days / 100 MB — deleted days after the money was spent, and never machine-readable even while it existed.

One JSON object per review, appended to:

/opt/upsquad/ops/review-dispatcher-logs/usage.jsonl

DISPATCHER_USAGE_LEDGER overrides it; with only DISPATCHER_LOG_DIR set it follows the log dir. It is not rotated — rotatable_logs considers *.log only, which is the point of the .jsonl suffix rather than an accident of it.

Schema

Every key is present on every row, null where unknown. That uniformity is what lets select(.cost_usd != null) mean "cost unknown" rather than "key absent or cost unknown".

field	type	notes
ts	string	UTC YYYY-MM-DDTHH:MM:SSZ, the dispatch instant — the same one in the log filename
repo	string	owner/name
pr	int
role	string	the reviewing lane, e.g. principal-architect
engine	string	claude | codex
model	string	what the launcher ACTUALLY passed (effective_model_and_effort, one derivation)
effort	string | null	codex model_reasoning_effort; always null for claude, which has no such flag — see _validate_model_and_reasoning
input_tokens	int | null	claude only
output_tokens	int | null	claude only
cache_read_tokens	int | null	claude only
cache_creation_tokens	int | null	claude only
total_tokens	int | null	claude: the sum of the four above, all four or null; codex: the tokens used figure
cost_usd	float | null	claude total_cost_usd; always null for codex
duration_s	float	launcher wall clock — fetch and tree setup included
exit_code	int | null	the launcher's rc: 75/78/124/143, negative when the direct child died on a signal (Popen.wait returns -9 for SIGKILL), and null when the wait was interrupted before any code existed
parse_error	string | null	non-null when usage could not be recovered; the row is still written

What each engine can and cannot tell us

claude -p --output-format json returns one object (verified against 2.1.273). Two asymmetries, stated rather than smoothed over:

• usage is the main model's aggregate; total_cost_usd covers every model the session used. A probe showed usage at 2 in / 4 out for sonnet while a haiku title-generation call's $0.000942 sat inside the cost and outside the counters. Tokens slightly under-report; cost does not.
• codex exec prints only tokens used + a number, with no input/output split and no cost anywhere in that text output. Those columns are null for that engine because they are unknown, not because they are zero — and the two engines' totals are not comparable: codex's is one opaque figure, claude's is a derived sum including cache reads.

The codex total is recovered by regex, and the regex is honest about being ambiguous. tokens used is not at EOF — codex prints its closing message after it, and all three real logs on this box carry 3–5 lines at column 0 in that region. A reviewer quoting a totals block there (a fenced block does not indent) creates a second, later match: QA drove the real #3441 log plus one plausible closing message through the round-1 parser and got 1234567 with parse_error: null. There is no structural anchor that fixes this — the bait lives in the same tail as the real total — so: more than one distinct total ⇒ still take the last, and say so in parse_error. Silent-wrong becomes flagged-uncertain, which select(.parse_error != null) can see. Two matches with the same value are not flagged; a flag on every codex row would make the coverage recipe useless.

The structural fix is codex exec --json (the mode exists — --json for JSONL events, -o/--output-last-message), which would also likely supply the in/out split. Deliberately not done here: nobody has observed a successful turn.completed envelope (probing one costs quota), so its usage fields are unknown, and a format change to the engine that reviews with the weaker isolation is not a change to make against an unverified shape. Follow-up.

Claude counters are flagged on RECOVERY, not on presence — and the boundary is total_tokens. A usage block that is present but unreadable (the CLI camelCasing its keys across a version bump, counters as strings, a block that is only server_tool_use) used to yield all-null counters with parse_error: null; the worst shape has a cost and no tokens, so it reads as a legitimately cheap review. The band in between is the same defect one step over: with 3 of 4 counters recognised the total is null — correctly, a partial sum silently under-reports — and the row was still unflagged, so the coverage recipe below, which advertises "rows we could not price or count", could not see a row we could not count. One renamed key produces that, and it is at least as plausible as four renaming at once.

So the rule is exactly the one the ledger states: if total_tokens is null, the row carries a parse_error — naming the CLI's own spelling of the missing keys (usage block missing counters: cache_creation_input_tokens), because that is the string an operator will grep the CLI's output for.

The log still reads like a log

.result goes into the launcher log exactly where raw stdout used to, followed by one summary line at the tail:

usage: 15234 in / 2103 out / 1290334 cached / $0.4213 (1307671 total)
usage: - in / - out / - cached / $- (304094 total)        # codex: only a total

- rather than 0, because a run whose cost we cannot compute must not read as a free one.

The breaker and this path — both directions

Nothing the parent writes may start with ERROR:/Error:. The quota breaker anchors on that at column 0 within the last 25 lines, and the parent is a new writer of the same file. Every line it adds carries usage: or dispatcher: ; engine-supplied fragments (subtype=…, parse_error) sit mid-line where the anchor cannot reach.

And the engine's own text must stay verbatim and unprefixed, because under --output-format json that write-back is how an engine-reported usage limit reaches the log at all (§4's correction). engine_text_for_log is that contract, and it is the only place allowed to put unprefixed text in the log. Annotate on a separate dispatcher: line instead — as the is_error notice does.

Five mutants police this, all killed by scenario_usage_logging_cannot_trip_the_breaker:

mutant	what it does
M3399-A	emits the summary line as ERROR: usage limit reached … — every completed review trips the global kill switch
M3399-B	truncates the log instead of appending
M3399-C	drops the engine's text when is_error is true
M3399-D	prefixes every line of the engine's text — still present, still readable, permanently unmatchable
M3399-E	drops the leading newline, gluing the first appended line off column 0 when the log does not end in one

M3399-C/M3399-D survived the entire 266-test lane on this PR's first round; the case that kills them drives a signature through $RAW rather than seeding it into the log, which is the route that actually matters.

Residual, bounded (architect N4): the parent appends stdout after all of stderr, so an unparseable stdout dump of ≥ 24 lines could push a stderr-side signature out of the 25-line window. Unlikely in JSON mode — a partial result is one long line — and the miss costs the rest of one cycle under the daily cap, not a false global stop. The real-time interleaving that text mode gave for free is gone.

Recipes

All four were run on 2026-09-17 against a ledger built by this code, not written from memory — the first one is here in its corrected form because the obvious spelling of it is a jq syntax error (object values that use + need parentheses).

LEDGER=/opt/upsquad/ops/review-dispatcher-logs/usage.jsonl

# per day, per engine: reviews, known cost, tokens
jq -s 'group_by(.ts[:10] + " " + .engine)
       | map({day_engine: (.[0].ts[:10] + " " + .[0].engine),
              reviews: length,
              cost_usd: ((map(.cost_usd // 0) | add) * 10000 | round / 10000),
              tokens:   (map(.total_tokens // 0) | add),
              unpriced: (map(select(.cost_usd == null)) | length)})' "$LEDGER"

# what a single PR has cost across every review of it
jq -s --arg pr 3399 'map(select(.pr == ($pr|tonumber)))
       | {reviews: length, cost_usd: (map(.cost_usd // 0) | add)}' "$LEDGER"

# coverage check: rows we could not price or count, newest first
jq -c 'select(.parse_error != null) | {ts, repo, pr, role, engine, exit_code, parse_error}' "$LEDGER" | tail

# from a suspicious row back to the review that produced it
jq -r 'select(.cost_usd > 5)
       | "\(.repo|split("/")[1])-\(.pr)-\(.role)-\(.ts|gsub("[-:]";"")).log"' "$LEDGER"

unpriced in the first recipe is not decoration: every codex review lands in it, so a day whose cost_usd looks small should be read next to it.

It records attempts, not only completions

A launch that never reached an engine (75 head moved, 78 tree not prepared) gets a row too, with nulls and a parse_error. "No row" and "a row that cost nothing" are indistinguishable in a sum but completely different to anyone auditing whether the ledger saw everything — and the row that matters most, an hour of spend killed by the wall clock (124), is written from launch's finally for exactly that reason.

Being in the finally is also why rc is initialized before the try: a KeyboardInterrupt out of proc.wait used to hit an unbound rc there, which masked the interrupt behind an UnboundLocalError and skipped the rmtree, leaking a review-dispatch-* directory into log_dir — which rotation can never reclaim, because it only considers *.log files.

Rollout

Merging this does not change what runs: the service executes review-dispatcher-current → releases/<sha> (#3398), so the ledger starts on the next release cut. The first live row is the acceptance — check it with the recipes above, and with the fixture/repo check below.

If you are turning this on for the first time

Check the file is not already carrying fixture rows:

jq -r 'select(.repo == "fixture/repo") | .ts' "$LEDGER"   # must print nothing

The real-process test suite builds its own Config, outside the enumerating pin in test_review_dispatcher.py, and wrote eight fixture/repo#1 rows into the production ledger the first time this code ran. Both places are guarded now (test_no_config_path_in_this_suite_points_at_production_ops, which enumerates the helper as well as the instance, so the next path field cannot be redirected in one construction site and forgotten in the other); the check above is what tells you whether it happened before they were.

Both #3443 reviewers independently found the file already gone at review time (find /opt/upsquad/ops -name usage.jsonl → nothing), so the cleanup is done — keep the check anyway, it costs one line and it is the acceptance for the first live row.