--- name: Codex Code Review slug: codex-code-review category: Quality description: Codex Code Review runs Codex-based review on pull requests and code changes, then returns severity-grouped findings and a merge gate. Use it for PR review, code audits, or a second opinion on changes. github: "https://github.com/sd0xdev/sd0x-harness/tree/main/skills/codex-code-review" language: JavaScript stars: 188 forks: 24 install: "npx degit https://github.com/sd0xdev/sd0x-harness/tree/main/skills/codex-code-review ~/.claude/skills/codex-code-review" installs_to: ~/.claude/skills/codex-code-review source_path: skills/codex-code-review/SKILL.md collection_size: 25 category_size: 1745 collection_url: "https://dirskills.com/collections/sd0xdev/sd0x-dev-flow" added: 2026-09-06T05:19:57.959Z last_synced: 2026-09-06T05:19:57.959Z canonical_url: "https://dirskills.com/skills/codex-code-review" --- # Codex Code Review Codex Code Review runs Codex-based review on pull requests and code changes, then returns severity-grouped findings and a merge gate. Use it for PR review, code audits, or a second opinion on changes. **Install:** ```bash npx degit https://github.com/sd0xdev/sd0x-harness/tree/main/skills/codex-code-review ~/.claude/skills/codex-code-review ``` ## README # Codex Code Review ## 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 | Variant | Command | Scope | Pre-checks | |---------|---------|-------|------------| | Fast | `/codex-review-fast` | Diff only | None | | Full | `/codex-review` | Diff + local checks | lint:fix + build | | Branch | `/codex-review-branch` | Full branch | None | ## 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). | Variant | `--dual` accepted? | |---------|--------------------| | Fast (`/codex-review-fast`) | No — single only | | Full (`/codex-review`) | No — single only | | Branch (`/codex-review-branch`) | Yes, off unless passed | 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) ```bash {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`, 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. | Variant | Collection Method | |---------|-------------------| | Fast | `CHANGED_FILES`: `git diff --name-only HEAD` ∪ `git ls-files --others --exclude-standard` + `DIFF_STAT`: `git diff --stat HEAD`, plus a line count for each untracked file (`wc -l`), which no diff stat covers | | Full | Same as Fast | | Branch | Resolve `MERGE_BASE` **once**, per § Resolving the Branch base below, then use only that object id: `CHANGED_FILES`: `git diff --name-only $MERGE_BASE` ∪ the same uncommitted and untracked sets + `DIFF_STAT`: `git diff --stat $MERGE_BASE` + `CURRENT_BRANCH` + `BASE_BRANCH` + `COMMIT_COUNT` | 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 -- ` is wrong for two of the three cases: | What | How Codex reads it | |------|--------------------| | Fast / Full, tracked file | `git diff HEAD -- ` | | Branch, tracked file | `git diff $MERGE_BASE -- ` for the committed part, plus `git diff HEAD -- ` for what is uncommitted on top — the id resolved once below, never a fresh `git merge-base` here | | Untracked file, any variant | `cat ` — git has no diff for a file it does not track, so the whole file is the change | 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): | Variant | Baseline set | |---------|-------------| | Fast / Full | `git diff --name-only HEAD` ∪ untracked (`git ls-files --others --exclude-standard`) | | Branch (incl. `--dual`) | `git diff --name-only $MERGE_BASE` ∪ the same uncommitted + untracked set — the same single id, not a second computation | #### 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: | Tier | `BLOCKING` | Source | |------|-----------|--------| | `fast` | `P0` | `@rules/auto-loop-project.md ## Tier` | | `standard` (default) | `P0/P1` | unset, unrecognized, or explicit | | `thorough` | `P0/P1/P2` | explicit, **or** the Branch variant, **or** a security / data-integrity change | 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. | Field | Use | |-------|-----| | `has_requests` | Gate: only proceed if true | | `docs_path` | Glob for request docs | | `confidence` | Require >= medium | 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 ## Diff Stats ## Scope Baseline (frozen) 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=, scope_reason=, scope= (derived: out-of-scope ⇔ pre-existing ∧ pre-existing-outside), change_relation= (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=. 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= — 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= reason= | `; 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:`; 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