Codex Code Review

Codex Code Review

<!-- Security note: Bash(bash:*) is broader than ideal; cannot scope to specific
     script paths until Claude Code #9354 resolves ${CLAUDE_PLUGIN_ROOT} in
     command frontmatter. Only invoke bash for project scripts (scripts/*). -->

Trigger

• Keywords: review, PR, code review, second opinion, audit, check

When NOT to Use

• Document review (use doc-review)
• Security-specific review (use security-review)
• Test coverage review (use test-review)
• Just want to understand code (use code-explore)

Variants

Shared Workflow

Resolve adapter → [Pre-checks if Full] → Collect changes & freeze baseline → Codex Review → Gate: derive sentinel × gate_reason (Step 4.5)
  → Ready × NONE → next gate | Blocked × IN_SCOPE_BLOCKING × untriggered → fix loop | other Blocked outcomes → E1/E2

Dual dispatch adds a second reviewer, and is opt-in:

--dual:  … → Codex + Task in parallel → Merge findings in conversation (field-level)
  → Gate: derive sentinel × gate_reason (Step 4.5)
  → Ready × NONE → next gate | Blocked × IN_SCOPE_BLOCKING × untriggered → fix loop | other Blocked outcomes → E1/E2

Step 0: Reviewer Mode

Default: Codex alone. Do not launch a secondary reviewer. One reviewer, one verdict, noted in Step 4.5 — there is no mode field, no aggregate plane and no state machine behind this choice: which reviewers ran is a fact of the conversation, not of a store (hook-lightweighting § 3.3).

--dual (Branch variant only): adds a second reviewer in parallel; on the Codex-healthy path the merge is yours to perform in conversation (Step 4). A second opinion for releases, security-sensitive changes and public API surfaces — nothing persists it, nothing blocks on it, and the next invocation starts single again unless the flag is passed again. When Codex is out, there is no merge: the validated fallback report carries the gate alone (Step 3.5 Codex-failure path).

See @rules/auto-loop.md § Review Dispatch for why single is the default.

Step 0.5: Resolve the adapter locator (before any snapshot)

Resolve the adapter through references/codex-transport.md § Locator now, and let any auto-install that section prescribes happen here — before Step 1 freezes anything.

The ordering is the transport contract's, not a preference: in a consuming repository whose first review predates the installed adapter, § Locator's second step writes it into the tree. Resolve it at dispatch time instead and that write lands after Step 1 froze the changed-file set and the scope baseline, so the new untracked file is a tree change no baseline contains and no reviewer prompt lists — a review whose own snapshot went stale while it ran.

A setup-required locator outcome (no adapter at any step) stops here and is surfaced to the operator. It is not codex_fail: nothing was dispatched, so no fallback reviewer runs and no verdict is noted.

Step 0.7: Pre-checks (Full variant only)

{LINT_FIX_COMMAND}
{BUILD_COMMAND}

These placeholders are resolved from the host project's CLAUDE.md or package.json scripts. Record results as LOCAL_CHECKS.

It is numbered before Step 1 for the same reason Step 0.5 is. {LINT_FIX_COMMAND} writes — a project-wide lint fix edits files, and a build can regenerate them. Run it after Step 1 and those edits land outside the frozen changed-file set and scope baseline: delivered changes every reviewer dispatch then misses or misclassifies as out-of-scope. Settle the tree first — adapter, then pre-checks — and freeze once, over the tree that will actually be reviewed. It was numbered Step 2 until 2026-09-04, which put it after the freeze.

If anything writes to the tree after Step 1 has run, Step 1 is redone in full. The baseline is frozen once per review session, and a baseline computed over a tree that has since changed is not the one this review is judging.

Step 1: Collect Change Metadata

Collect metadata only — Codex reads the actual diffs and file contents itself via sandbox access.

TASK_DESCRIPTION is the original task in one or two sentences, captured here and frozen for the whole review session — every first, fallback, and rotated dispatch carries the same value, and it is never rewritten from review findings (rules/codex-invocation.md, full contract in skills/codex-code-review/references/codex-invocation-contract.md — read it before the first dispatch; the three-part dispatch shape).

CHANGED_FILES is the frozen baseline set itself, not a narrower query — the two are computed from the same expression below, because a manifest that is a subset of the baseline hands the reviewer a shorter change than the one it is told to judge. The change that added this paragraph proves it: the transport adapter, its reference and their tests were all untracked, so git diff --name-only HEAD alone omitted every one of them.

Codex reads the diffs and file contents itself, and which command shows them depends on the variant and on whether the file is tracked — one blanket git diff HEAD -- <file> is wrong for two of the three cases:

The variant's prompt template carries the same instruction; this row exists so the metadata step and the prompt cannot drift apart.

Scope baseline (frozen here). Compute the baseline file set once, now, and freeze it for the whole review session (skills/codex-code-review/references/scope-contract.md § Scope Baseline):

Resolving the Branch base

${BASE_BRANCH} is resolved to an object id here, once, and only that id travels onward. A ref name is not safe to render into shell source: git accepts ;, backticks and parentheses in a valid ref, rev-parse --verify accepts such a ref, and placeholders are bound textually before the command runs — so double quotes around a rendered ref do not help, since the metacharacters are already in the source when the shell parses it. Only never rendering the ref does.

1. Run the resolution with the ref as a shell-single-quoted literal, which suppresses expansion: git merge-base -- 'the/resolved/ref' HEAD (an embedded apostrophe is written '\'').
2. Verify the result is 40 hex characters before using it. Anything else is a parameter error: abort and ask for an explicit base.
3. Bind that id as MERGE_BASE and use it everywhere above and in every prompt. No later step recomputes it — one baseline, one id, which is also what the frozen-baseline contract requires.

${BASE_BRANCH} itself still travels to the reviewer as metadata (a name in the prompt's Task and Scope sections); what it must never do is appear inside a command the reviewer will run.

${BASE_BRANCH} resolution (Branch variant): explicit argument first (e.g. /codex-review-branch origin/develop); else git symbolic-ref --short refs/remotes/origin/HEAD; else origin/main — verify each candidate with git rev-parse --verify before use. All candidates failing → abort as a parameter error and ask for an explicit base; never continue on an empty baseline (an empty baseline would misread every unmodified file as out-of-scope), and the abort is not a human exit. Record the resolved base and the frozen file list in the review report metadata, and inject the list into every reviewer prompt as SCOPE_BASELINE.

The frozen baseline is task-scoped and immutable: the initial reviewer, the inline secondary, --continue, and every same-task re-dispatch reuse the same list — no path recomputes it. The only growth is the user-named monotonic union of skills/codex-code-review/references/scope-contract.md § Scope Baseline; ordinary fix edits during a round never write back into it.

Step 1.1: Resolve the tier (required before dispatch)

The gate is tier-derived, and the reviewer has to be told which severities block — otherwise it emits ✅ Ready / ⛔ Blocked against its own assumption, and that is the verdict you note in Step 4.5. Resolve the tier first, then bind TIER and BLOCKING into the prompt:

The Branch variant is thorough by definition, so BLOCKING = P0/P1/P2 there regardless of project config — a P2 blocks a branch review. Escalation for a security or data-integrity change applies to every variant, and you say that you escalated.

Step 1.5: Feature Context & AC Detection (Spec-Driven Review)

Execute: bash scripts/resolve-feature.sh → parse JSON output.

If has_requests=true AND confidence in (high, medium):

1. Glob ${docs_path}/requests/*.md, sort descending, take latest
2. Read latest request doc
3. Extract ## Acceptance Criteria section (parse - [ ] / - [x] items)
4. Filter out quality-gate ACs matching: /codex-review-fast, /codex-review-doc, /codex-review, /precommit, /precommit-fast, /pr-review
5. Cap: max 20 ACs (truncate with "... and N more" note)
6. Build SPEC_CHECKLIST variable, set REQUEST_DOC_PATH

Graceful degradation: resolve-feature fails / no requests / no AC section / parse error → SPEC_CHECKLIST = null (skip silently).

Step 3: Dispatch

Bind every placeholder before writing prompt.md — both cases below. The templates are body-only now, so no expression in them is evaluated by anything: a ${X || 'default'} is copied into the prompt literally and shipped to Codex as text (it was, until a doc review caught it). Two have no natural empty form and the dispatcher supplies it — ${LOCAL_CHECKS} becomes Skipped when no local checks ran, and ${DISPOSITIONS} becomes None when there are none. This sits above the Case A / Case B split deliberately: ${DISPOSITIONS} is consumed by Case B, so a --continue dispatcher that skipped Case A would otherwise never have read its binding rule.

Case A: First review (no --continue)

Dispatch Codex. Launch the secondary reviewer only when --dual was passed:

1. Codex (primary): dispatch per references/codex-transport.md § Start with the variant-specific prompt:

   Variant	Prompt Template
   Fast	references/codex-prompt-fast.md
   Full	references/codex-prompt-full.md
   Branch	references/codex-prompt-branch.md

   Save the returned threadId.

2. Secondary reviewer — --dual only, skip entirely otherwise: Use Task tool with reviewer selection cascade:

   Priority	Reviewer	subagent_type	Condition
   1	pr-review-toolkit:code-reviewer	pr-review-toolkit:code-reviewer	Default choice
   2	strict-reviewer	strict-reviewer	Priority 1 fails/times out
   3	Codex-only (degraded)	—	Both unavailable

   Selection: Try priority 1 first. If Task fails or times out (30s), try priority 2. If both unavailable, fall back to Codex-only (degraded mode — proceed with Codex results only, apply degradation matrix from references/review-common.md).

   Task prompt (provide changed file list + diff stats, request P0/P1/P2/Nit findings in standard output format):

   Review the code changes for correctness, security, performance, and maintainability issues.
   
   ## Changed Files
   <git diff --name-only output>
   
   ## Diff Stats
   <git diff --stat output>
   
   ## Scope Baseline (frozen)
   <SCOPE_BASELINE — the frozen file list from Step 1; do NOT recompute it>
   
   Read the actual diffs and file contents yourself to perform the review.
   
   Before reporting findings, independently verify each one:
   1. Evidence check: what specific code proves it's real? (file:line)
   2. Context check: did you read enough surrounding code?
   3. False positive check: could it be intentional design?
   4. Severity check: could it be more severe than initially assessed?
   5. Gap check: what related issues might you have overlooked?
   Only report findings that survive all 5 checks.
   
   Classify every finding against the frozen baseline (contract:
   references/review-common.md § Scope Fields): origin=<in-diff|pre-existing|uncertain>,
   scope_reason=<diff-file|one-hop|branch-introduced|pre-existing-outside|uncertain>,
   scope=<in-scope|out-of-scope> (derived: out-of-scope ⇔ pre-existing ∧
   pre-existing-outside), change_relation=<affected|independent|uncertain> (does the
   primary diff change this defect's inputs, reachability, contract, error behaviour,
   state, or operational impact? adjacency is not effect — a cited one-hop call site
   proves the defect is nearby, not that this change reaches it),
   evidence=<file:line call site, or a blame/log -L citation;
   pre-existing-outside requires the complete negative case; change_relation=independent
   on an in-scope finding requires the primary hunk(s) as file:@@-a,b+c,d>. One hop only — no
   transitive expansion; no citable evidence → uncertain.
   
   Output findings in this format:
   - [P0/P1/P2/Nit] file:line issue description → fix recommendation | origin=... scope_reason=... scope=... change_relation=... evidence=...
   
   Group by severity. Include a final gate: ✅ Ready or ⛔ Blocked, with one line
   gate_reason=<NONE|IN_SCOPE_BLOCKING|OUT_OF_SCOPE_CRITICAL|BOTH> — Blocked ⇔ an
   in-scope (incl. uncertain) finding at or above ${BLOCKING}, or an out-of-scope
   P0/security/data-integrity finding (valid [USER_SKIPPED] records, if any, are
   applied orchestration-side after your report); NONE pairs only with Ready.

Case B: Loop review (has --continue)

• Rotation check first: before each reply, apply references/review-common.md § Review Loop — Thread Rotation (central contract): at the R-a threshold (3 replies on this thread; @rules/auto-loop-project.md ## Review Thread Rotation overrides, 2–6) or on R-b judged context overrun, do not reply — dispatch Case A's first-review template on a new thread (frozen baseline only; old findings and dispositions reconciled orchestration-side after the fresh report) and record [THREAD_ROTATED].
• Codex: otherwise dispatch per references/codex-transport.md § Resume with the re-review template from references/review-common.md
• Under fallback (sticky carrier for this change): agents are stateless — every re-review is a fresh Step 3.5-style dispatch to the same carrier, so rotation is automatically satisfied; validate each report the same way before noting. Reconcile it like a rotated report (references/review-common.md § Thread Rotation step 3): an old unclosed finding the fresh report omits is closed if its fix is in the diff, and otherwise re-enters this round with its identity and severity and change_relation=uncertain — a fresh carrier's silence never retires an unfixed owed finding.
• Secondary (--dual only): re-dispatch in parallel, fresh context. Cycle resets on any code edit.

Step 3.5: Await Results

Single reviewer (default dispatch): await Codex. Its verdict is the gate for this dispatch. Go to Step 4. A dispatch that runs in the background is observed, not polled: launch it with nothing redirected (stderr on the task panel is the live 60 s view), arm the persistent self-terminating Monitor recipe of references/codex-transport.md § Progress on the adapter-owned progress.json (state changes only — started, five-minute marks, the stall advisory, the terminal status — so the operator can keep talking between them), treat the task's completion notification as the end of the run, and cleanup — which ends the Monitor by itself, since the recipe exits once progress.json has been unreadable for three polls, a full 60 s; TaskStop only silences it sooner. This skill and its three entry points run in the parent session and grant Monitor for exactly this step. A progress line, whatever it reports, is never a verdict and notes nothing (INV-005).

If the transport reports codex_fail — adapter exit 1 only (references/codex-transport.md § Completion state machine): quota, network, an unreachable CLI, a malformed stream. A pending or unknown completion keeps the gate open and dispatches nothing; exit 2 is a configuration error to fix, not a Codex failure; an alloc/cleanup failure is a lifecycle error surfaced to the operator. On codex_fail the gate does not stop: a contract-aware fallback carries it (@rules/auto-loop.md § Review Dispatch). Named steps, in order:

1. Decide — call scripts/lib/review-dispatch.js (node -e "console.log(JSON.stringify(require('./scripts/lib/review-dispatch.js').decide({contract:'code',probe:'codex_fail',sticky:'none'})))" shape) for the next action. Record [REVIEWER_FALLBACK] plane=code_review from=codex to=<agent> reason=<quota|timeout|error> | <ISO8601>; the selection is sticky for this change — re-reviews do not re-probe, the next change probes Codex afresh.

2. Dispatch the carrier via Task with this variant's own prompt template and the frozen SCOPE_BASELINE (the same template Codex would have received — the template is the contract):

   Priority	Carrier	Depth guarantee
   2	strict-reviewer	Repo-owned agent; frontmatter pinned by test/agents/frontmatter.test.js
   3	pr-review-toolkit:code-reviewer	Plugin agent — the pin cannot reach it, so the call-site MUST explicitly request model: opus, effort: high (best-effort)

3. Validate fail-closed — pipe the carrier's raw report to node scripts/validate-family-sentinel.js code. Exit 0 (exactly one of ✅ Ready / ⛔ Blocked, no foreign family terminal) → the report is the gate verdict with gate_source=fallback:<agent>; note it as usual. Exit 1 → this carrier's dispatch failed; move to the next priority. A terminal is never translated across contracts.

4. Priority 4 — both carriers exhausted: no validated verdict exists — carriers may have run, but no report survived the family contract. Emit no gate sentinel, surface behaviour-layer ⚠️ Need Human, and note nothing.

--dual (Codex-healthy path): Codex is the blocking reviewer — await its result for the initial gate. Secondary runs in background (run_in_background: true) and is non-blocking:

--dual (Codex-failure path): when the probe fails under --dual, the same fallback chain above carries the gate alone — the validated fallback report is the gate verdict, no aggregation is waited on or built, and no Codex thread exists to continue. The healthy-path table above does not apply, and the secondary's report — whether it completed before Codex failed, before precommit, or after — is never merged into the fallback's gate derivation and never carries the gate itself. Handle it under the Codex-down secondary policy: normalize it fail-closed on arrival, derive each finding's fix_obligation exactly as the gate carrier's report gets it (references/scope-contract.md § Opportunistic Envelope), then act on its owed blocking findings only, conservatively — a secondary in-scope mandatory blocking finding, or an admitted one at any severity, re-opens the fix loop; a secondary finding deriving deferred is recorded ([OPPORTUNISTIC_DEFERRED] at blocking severity, [NIT_DEFERRED] below it) and re-opens nothing; a secondary out-of-scope critical finding is E1 — while a secondary ✅ Ready is advisory and notes nothing: it never substitutes for a validated fallback verdict, and Step 4.5's ✅ Ready × NONE row is indexed only by the gate carrier's own report, never by a secondary's. In particular, at Priority 4 (every fallback carrier exhausted — no validated verdict exists) the gate stays open with behaviour-layer ⚠️ Need Human whatever the secondary reported. Nothing is silently merged or dropped. The pre-precommit checkpoint below is likewise Codex-healthy-only — on this path there is no aggregate to reconcile and no Codex gate to proceed with.