Retry capacity rejections on a designated backup model, and refuse a review whose repository moved

[patriyang]

Closes #96
Closes #98

#98 is the other half of the gap #97 half-closed, and builds on its request.target pinning. This branch was developed stacked on #97; #97 merged mid-flight, so it is now rebased directly onto main and the diff is only this work.

Both issues land together because they converge on one surface: a machine-readable failure class on the job record. #96 needs it to tell a transient capacity rejection from a real failure; #98 needs it to tell "the repo moved" from a review that genuinely found nothing.

#96 — capacity rejections were terminal, and controllers had to string-match

A capacity rejection is transient and content-independent, but it arrived as status: failed with the reason only in the prose summary. Every controller had to recognize "Selected model is at capacity" by hand and re-dispatch.

Three parts:

A failure class. New lib/failure-class.mjs classifies a turn's error message. runAppServerTurn / runAppServerReview now return failureClass and retryable; executeTaskRun / executeReviewRun put them on the payload; runTrackedJob persists them on both the job file and the index, on the completion path and the throw path. /codex:status --json and /codex:result --json expose them, and the human output carries one terse line.

A designated backup model — with nothing hardcoded. Model names and labelling conventions churn, so a pinned fallback would rot. Resolution is CODEX_COMPANION_FALLBACK_MODEL (set it to none to disable) → /codex:setup --fallback-model → live discovery from model/list, preferring the advertised default and skipping the model that was at capacity. Step 3 is what survives renames. The existing inline model/list walk was extracted into listAdvertisedModels and shared with effort validation rather than duplicated; it takes an optional stopping predicate so effort validation keeps its early exit.

A retry that cannot repeat work. Exactly one fallback attempt, on the same client and thread, changing only the model. It is gated on the turn having produced nothing.

That guard is the load-bearing part, and the first version of it was wrong. recordItem only records fileChanges / commandExecutions on lifecycle === "completed", so a command that had started and not finished looked like "produced nothing" — and a retry would have re-run it. The guard now counts every item notification regardless of lifecycle. This is not hypothetical: the Codex implementer working on this branch took a capacity rejection roughly five minutes into a run, well after it had started editing.

#98 — a queued review could silently review nothing

#97 pinned the resolved target's scope. It did not pin the repository state that scope resolves against. So: --scope auto on a dirty tree pins working-tree, you commit before the detached worker starts, and the worker reviews a now-clean tree.

Reproduced before fixing — the worker handed Codex a prompt whose entire repository context was:

## Git Status      (none)
## Staged Diff     (none)
## Unstaged Diff   (none)
## Untracked Files (none)

and the job completed. A clean review of a change nobody looked at, which is the worst direction to fail in, because a clean review gets trusted.

Chosen fix: fail loudly on drift (option 1 of the three the issue laid out). Smallest change, keeps the current inputs, and converts a silent false negative into a visible retryable error. Snapshotting the diff at enqueue is the only fully deterministic answer and stays open — nothing here forecloses it.

captureRepoStateIdentity records headOid, plus baseOid for branch targets and a working-tree digest for working-tree targets. executeReviewRun recomputes and refuses before anything can start a Codex turn, throwing an error carrying failureClass: "state-drift" and retryable: true. Only background reviews capture an identity; the foreground path's window is a few statements and a check there would only produce spurious failures. Jobs persisted by an older plugin version have no identity and skip the check.

Review rounds tightened the digest three times, each closing a real hole:

• The plan claimed git status --porcelain=v2 carries a blob OID for unstaged content. It does not — v2 gives HEAD and index OIDs only, so a further edit to an already-dirty tracked file was invisible. Dirty tracked paths now fold in git hash-object.
• A deleted base ref threw a raw Git error carrying none of the contract. Deleting the base branch mid-flight is drift; it now reports as such. An unresolvable ref at the command line still fails loudly as a user error.
• The untracked digest hashed a symlink's target string while formatUntrackedFile follows the link and reads the destination. It now mirrors what the review actually collects, bounds included.

Submodules get bounded, deliberately non-recursive handling: a dirty tracked directory folds in that submodule's own HEAD and status digest, one level, no nesting.

Deep review

Three rounds, the cap for this repo. Each round found real defects; all were fixed except one I pushed back on and then reversed.

Round 1 (4 findings, all accepted). The worst was that retryable: true was reported for a capacity failure on a turn that had already done work — the internal retry correctly refused, but the machine-readable flag told external controllers the opposite. retryable now means safe to repeat, judged against the turn actually returned, so it and the internal guard cannot drift apart. Also: /codex:review never used the backup model at all (the README promised it did), submodule diff content was missing from the fingerprint, and a successful fallback still attributed its output to the model that had rejected it.

Round 2 (2 findings, both accepted). A review is not always limited to a frozen prompt — native reviews and self-collect mode (large diffs) have Codex read the repository live during the run, so a single check at worker start cannot catch state that moves mid-run. Added a post-run revalidation, scoped to exactly those two modes; an inline-diff review keeps its findings, since discarding a completed multi-minute review whose prompt was already frozen would be a costly false positive.

Round 2 also re-raised the untracked-content hashing I had rejected in round 1. I had argued that if the rendered prompt is byte-identical then nothing reviewable changed. That was only true for inline-diff mode — in native and self-collect modes Codex reads raw bytes the digest never saw. Rejection withdrawn; untracked regular files are now hashed with git hash-object --no-filters, decoupled from display limits.

Round 3 (2 findings, both accepted, fixed directly at the review cap). The pinned-state check ran a few statements before collectReviewContext, so it validated a moment rather than the content actually frozen into the prompt; it now runs immediately after collection. And the classifier matched a bare try a different model, which the server also appends to unrelated model-compatibility errors — that pattern is gone, since the real capacity message carries at capacity anyway.

Verification

• npm test: 341 passing, 0 failing, run outside the Codex sandbox.
• npm run build (tsc with checkJs): clean. It was failing with three TS2339 errors on turnState.error?.message — caught locally, not by review; those now route through the existing extractErrorMessage.
• Per task: spec review SPEC_COMPLIANT and code-quality review APPROVED, each after one fix round. Task 3's second round came back with no issues at any severity.
• Both repros were landed failing first (387a9dd) and are unmodified.

Notes

Two reviewer findings were rejected rather than implemented: an "edited test files" flag that contradicted an explicit later instruction, and an "extra work" flag against 8e50b9a, which is this branch's own Task 2 follow-up that the review range happened to span.

Two known residuals, both deliberate:

• Enqueue hashes dirty paths one git hash-object process at a time, so a very large dirty tree makes --background enqueue slower than it needs to be. Batching would fix it; it is not correctness.
• The round-3 post-collection check is defense-in-depth for a window of a few statements, and has no isolated regression test. Reaching it deterministically would mean injecting a mutation hook into production code, which is worse than the gap. The surrounding behavior — drift refused before any turn starts, inline reviews unaffected by later changes — is covered.

#99 was left unclaimed for a separate agent — it only touches plugins/codex/commands/rescue.md and shares no files with this branch.

🤖 Generated with Claude Code