Code Review

Code review

Review a diff on two axes, in parallel sub-agents, and report them side by side.

• Standards — does the code follow the rules this repo documents?
• Spec — does the code do what the originating issue or plan asked for?

The premise that makes this different from "find problems in this diff": a review round has to be able to finish. An open-ended hunt samples a diff, and every fresh round samples it differently, so three rounds turn up three disjoint sets of real defects and nobody can tell whether the fourth would turn up more. Each axis here enumerates a closed list from a source document first, then returns a verdict per item. Findings are the items that came back bad. The reader learns what was checked, not just what was noticed.

Two rules follow from that, and they are the ones to keep if everything else is forgotten:

1. Enumerate before you judge. The list of things to check comes from a document (the repo's standards, the issue's acceptance criteria), so it is finite, and the same on every run.
2. A finding carries a failure scenario. Concrete input or state, traced in the code, and the wrong thing that results. No scenario means it goes in unverified or is dropped. A plausible-sounding finding nobody traced is how a fix gets written for a defect that was never there.

Delegation authority

Invoking this skill authorizes every sub-agent dispatch that this procedure marks mandatory, including a mandatory nested review skill. Do not ask again solely because a session-level preference says "do not spawn agents"; apply that preference to discretionary delegation only. An explicit task-level refusal of this required review or revocation of delegation overrides this authorization: stop and state that the requested workflow cannot run without its required independent review.

Dependency and reviewer selection

At the standard skill root, when orchestrate is installed, read its references/review-routing.md and references/routing-table.md directly before dispatch. Use the four-row reviewer matrix, classify this skill's area as Code review, and apply the matrix's Claude-parent Codex presence/headroom gate to select the initial reviewer adapter and model.

When orchestrate or its review-routing.md is not installed, say so in one line and continue Claude-only: use Sonnet for routine review or Opus for load-bearing review, with no Codex attempt.

Modes

First review — the full process below.

Re-review (after fixes land): the diff under review is the fix, not the original change. Run the same two axes over it, with one extra question per fixed finding: does the fix do what the finding asked, and does it contradict anything the original code or its documents already said? This mode is not optional politeness. A fix written from the finding text rather than from the code is the single most reliable source of new defects, because the author has the finding in front of them and not the system. Scope the divergent-copies check to the fix: enumerate the copies of every fact the fix touched, and return a verdict per pair. A fix that edits one copy of a fact with four encodings has minted three new pairs that can disagree, and the next round will find one.

Hard cap: three rounds. If round 3 still returns new violated items on the same enumeration, the change has a structural problem, not undiscovered typos — almost always a fact with too many copies, or logic that exists only as prose. Stop reviewing. Name the structure problem and route it to the author as a design decision. A fourth fix round on the same enumeration buys new defects, not fewer.

Fixing findings

Rules for the change's author, not for the reviewing session — a review still only finds, and hands these to whoever holds the code. This is where rounds are manufactured.

1. Re-derive the fix from the code and its documents, never from the finding text alone. The finding names the symptom; the code owns the truth. Open the file, read what is actually there, and decide the fix from that. Patching the sentence or line a finding quotes, without re-reading what surrounds it, is how a round's worst defect ends up authored by the previous round's fix.
2. A fix that touches a fact with copies updates every copy, or collapses them to one authority — and says which it did. Half a fact updated is worse than none: it converts a redundancy into a contradiction.
3. When a finding cites a rule with a rationale, sweep the diff for every other site that rationale covers before declaring it fixed. One measured fix applied a case-insensitivity correction to 1 of the 3 regexes that shared its reason, and the other 2 cost a further round.
4. A fix that edits prose triggers a re-read of the surrounding section, for sentences the edit now contradicts. Prose has no compiler; the only check on a documentation fix is the paragraphs it sits inside.

Process

1. Pin the fixed point

Whatever the user named is the fixed point: a SHA, branch, tag, main, HEAD~5, a PR URL or number, or nothing (meaning the working tree against HEAD).

For a PR, fetch it rather than trusting the local checkout:

git fetch origin pull/<n>/head:pr-<n> && git fetch origin <base>

Then resolve and check the base is not stale:

git rev-parse <fixed-point> && git rev-parse origin/<fixed-point>

If a local branch and its remote differ, use the remote. A local main that is five merges behind silently pulls other people's commits into the diff and every finding against them is noise. Confirm the diff is non-empty before spawning anything; a bad ref should fail here, not inside two sub-agents.

Record once: the diff command (git diff <base>...<head>, three-dot) and the commit list (git log <base>..<head> --oneline).

2. Build the Standards enumeration

Collect what this repo documents about how code is written: CONTRIBUTING.md, CODING_STANDARDS.md, AGENTS.md / CLAUDE.md, README.md, any docs/ convention file, and the linter config. Read them at the head of the diff, not on the current checkout, so you judge against the version the change lands.

From those, write the enumeration: the specific documented rules this diff could plausibly breach. Ten to twenty is normal. A rule nothing in the diff could touch is left out, and the sub-agent is told how many were left out.

Then add these three, which apply even to a repo that documents nothing:

• Divergent copies. Which facts does this change write down in more than one place, and do the copies agree? Count every encoding: code, config, workflow conditions, generated output, and prose in documents. This is the highest-yield check in the set. A fact with N copies has N(N-1)/2 pairs that can disagree, none of them checked by anything, and each review round finds a different pair. Where the copies are already many, the finding is the redundancy itself, not the one pair that happens to disagree today.
• Claims nothing can falsify. Does the change add prose asserting what the code does, with no test that fails when the two disagree? Name the sentence.
• Guards for unreachable states, and their inverse: a trust boundary (external input, cross-process data, durable state) crossed with no guard.

Skip anything the linter or formatter already enforces. Reporting what CI already fails on wastes the round.

3. Build the Spec enumeration

Find the originating spec, in order:

1. Issue or ticket references in the commit messages or PR body (#123, PROJ-4567, Closes #45) — fetch it. If the repo documents its tracker, follow that; otherwise infer from the git remotes.
2. A path the user passed.
3. A plan under openspec/, docs/, specs/, or .scratch/ matching the branch or feature. An archived plan added by the diff itself still counts.

The enumeration is that document's acceptance criteria, one line each, verbatim. If it has a tasks list, that is the enumeration.

If the change wires into another component or skill, the integration contract joins the enumeration: who calls whom, and at which point. A change reviewed only in isolation passes clean and then spends the next round on nothing but seam defects.

Before declaring the Spec axis unavailable, extract the admitted risk contract from the spec, then look in the matching scope ledger if necessary. Do not substitute the PR description written by the same author as the code.

If neither a spec nor a risk contract exists, skip the Spec sub-agent and say so. A ledger-only contract still bounds the review: run the Spec sub-agent against it and include an unmet item stating that admission never promoted it into an authoritative artifact. If a bounded spec exists but failure handling, recovery, or evidence scope is material and its contract is missing, add that omission as an unmet Spec item; otherwise do not manufacture one.

Append Must prevent, Must recover, and Evidence owed entries to the Spec enumeration. Carry Accepted failure and Unsupported entries as explicit bounds for both sub-agents. A reviewer may challenge a bound only with evidence that changes its assumed likelihood, consequence, or recoverability; label that result reopen scope, not a code finding.

4. Run both axes in parallel

Before composing either positional prompt, run:

python3 <reviewer-memory-skill-directory>/scripts/memory.py pointer <repo>

Obey the reviewer-memory failure rule, including its not-installed carve-out. Keep the pointer and store content inside worker positional prompts only; never copy them into the review report, a tracker comment, work order, pull request body, or target-repository file.

The coordinator supplies three explicit seam inputs: the adapter appropriate to the coordinator's existing parent policy, reviewer model, and reviewer effort. Pass model and effort through unchanged to the selected helper. This process does not choose a model or effort, consume a routing table, classify work, apply headroom rules, or add defaults.

Dispatch only through skills/drivers/orchestrate/scripts/codex-worker.py or skills/drivers/orchestrate/scripts/claude-worker.py. For every admitted axis, use the selected helper's start command with --sandbox read-only, the explicit coordinator-supplied --model and --effort, --cwd set to the reviewed checkout, and one state file. Give the shared brief, the axis brief, the diff command, commit list, enumeration, risk contract when one exists, and the store index path printed by python3 <reviewer-memory-skill-directory>/scripts/memory.py pointer <repo> as the helper's positional prompt. The prompt must say that the reviewer must not modify, patch, or stash. Codex receives the positional prompt with inherited stdin closed; Claude adapter receives the positional prompt and delivers it to the child on stdin.

When a verdict depends on rendered evidence, include every required image in each relevant Codex axis invocation with the repeated --image option required by orchestrate's reviewer-routing contract. Do not substitute prompt text containing image paths for actual attachments.

Create one coordinator-owned <review-state-dir> and use deterministic state paths <review-state-dir>/standards.json and <review-state-dir>/spec.json. Capture each axis's launcher stdout and stderr separately in standards.stdout, standards.stderr, spec.stdout, and spec.stderr within that directory. State files carry lifecycle metadata only and never the reviewer answer.

When Spec is admitted, start all admitted helper invocations as background processes before waiting for readiness. Retain each axis-specific launcher PID and start the second admitted axis while the first remains active. Launcher PIDs are wait handles only; cleanup authority remains with adapter state and scoped stop / verify. On the portable path run_portable blocks and writes state only after worker exit, so the second admitted axis starts while the first remains active: never wait for state readiness before launching another admitted axis. When Spec is unavailable, launch Standards only, do not launch Spec, and report Spec unavailable.

Join every launched helper by waiting on its retained PID individually. Reject every nonzero exit. For every successful helper, parse final_message from its stdout artifact. Retry an axis with the selected helper's resume command against the same axis state file; a retry does not start a replacement state.

If a later launch fails after another worker launched, wait for the surviving helper to reach valid readable state or exit. A valid process-family state is recoverable; a portable terminal state records an already-exited worker and is not stop/verify-capable, so wait for the portable helper to exit instead of treating the state file's existence as readiness. Only when recoverable state exists, run that launched worker's scoped stop --state ... --cwd ..., then scoped verify --state ... --cwd .... Then join that worker's retained PID. Do not discover, stop, verify, or join an unlaunched worker.

Shared brief — give this to both:

Reviewer memory index: the path printed by python3 <reviewer-memory-skill-directory>/scripts/memory.py pointer <repo>. Read that bundle before reviewing. Trace every claim to the code before reporting it. For each finding state a failure scenario: the concrete input or state, and the wrong output, crash, or false statement that results. If you cannot construct one, put the item under unverified instead of findings. Do not report anything the linter enforces. Do not report style preferences the repo has not written down. Any executable literal in the diff or its documents — a regex, a shell fragment, a workflow expression, a query — is verified by executing it against a real input, never by reading it. Behavior matching an Accepted failure or Unsupported entry is accepted risk, not a finding. A missing test is a finding only when the enumeration or risk contract says evidence is owed; test count and an uncovered branch alone prove nothing. Under 400 words.

Standards brief:

Here is the enumeration of documented rules this diff could breach, plus the three general checks. Return a verdict for every item: holds / violated / not applicable, with the file and line for anything violated, and the rule you are citing. Then, and only if any would change whether this merges, up to three additional observations not on the list. State how many items you checked.

Spec brief:

Here is the enumeration of acceptance criteria, verbatim. Return a verdict for every one: met / partial / unmet / not verifiable from the diff, quoting the criterion and naming the code or test that settles it. Then report behaviour in the diff that no criterion asked for (scope creep). Do not re-litigate decisions the plan already settled, and do not report unticked checkboxes as findings — an unchecked box for work the plan defers is not a defect. Return met / partial / unmet / not verifiable for every must-prevent, must-recover, and evidence-owed entry. Report accepted-risk bounds only when the implementation contradicts them or added unasked-for machinery to handle them. State how many criteria and risk entries you checked.

5. Aggregate and report

Present the two verdict sets under ## Standards and ## Spec, verbatim or lightly cleaned. Do not merge or rerank across axes — see Why two axes.

Lead each axis with its coverage: "18 rules checked, 2 violated". Coverage is what tells the reader the space was enumerated rather than sampled, and it is the number that makes a clean round mean something.

List unverified items separately and briefly, if any. They are not findings. They are the honest residue, and naming them stops a later round from re-deriving the same suspicion as if it were new.

List any reopen scope items separately from findings. They return a risk decision to the user because new evidence invalidated its premise; they are not instructions to harden the implementation. Do not list accepted risks merely to make the report look complete.

End with one line: findings per axis, and the worst issue within each axis. Never pick a single winner across axes.

6. Say whether it is done

Close with the termination state, explicitly:

• Converged — every enumerated item holds or is met, and any prior round's findings were re-reviewed and confirmed fixed.
• Not converged — name what is outstanding.
• Capped — round 3 returned new violated items on the same enumeration. Name the structural problem behind them and hand it to the author as a design decision. This state ends the review; it is not an invitation to round 4.

A re-review that returns no new violated items on the same enumeration completes the review helper. Return that result to the caller, which decides and performs its next authorized step.

After stating the round's termination state, send one raw JSON object on standard input to:

python3 <reviewer-memory-skill-directory>/scripts/memory.py append-review <repo>

The object contains the ticket, round, verdict, findings summary, and reviewer model. Obey the reviewer-memory failure rule, including its not-installed carve-out.

Reporting rules

• Anchor every finding to file:line.
• Quote the rule or criterion. A finding that cannot cite one is an opinion, and belongs in the capped observations, not in the verdicts.
• Say what breaks, not what is inelegant. "Returns None and the caller never sees the failure" beats "swallows the error".
• A fix suggestion is one line. The review's job is to find, not to rewrite.

The smell checklist

Consult while judging; report only what passes the gate in the Standards brief (would it change whether this merges?), capped at three. These are heuristics, never hard violations, and a documented repo standard always overrides one.

Mysterious Name (name doesn't reveal what it does) · Duplicated Code (same shape twice) · Feature Envy (method reaching into another object's data) · Data Clumps (same fields travelling together) · Primitive Obsession (string standing in for a domain concept) · Repeated Switches (same cascade on the same type) · Shotgun Surgery (one change, scattered edits) · Divergent Change (one file edited for unrelated reasons) · Speculative Generality (abstraction for needs the spec doesn't have) · Message Chains (a.b().c().d()) · Middle Man (mostly delegates) · Refused Bequest (implementer ignoring most of what it inherits).

Why two axes

A change can pass one and fail the other:

• Follows every standard, implements the wrong thing → Standards pass, Spec fail.
• Does exactly what the issue asked, breaks the project's conventions → Spec pass, Standards fail.

Reporting them separately stops one axis from masking the other. It also stops the aggregation step from quietly dropping the axis with fewer findings, which is what reranking into a single list does.