review-workflow

name

review-workflow

description

Independently validate a pull request from another AI session before a human merge. Enforce CI/head-SHA triage, an isolated review worktree, repository gates, negative-path proof for guards, reproduced findings, a three-tier verdict, cold fix handoff, delta review, and an explicit merge-or-hold outcome. Use for delegated builder PR review, fix delta review, or agent-work validation. Do not use for your own uncommitted diff or a plain human-authored PR without delegation context.

Reviewing another AI session's PR

A builder cannot see what it cannot see about itself. Its suite is green because it wrote the tests; the paths it forgot to test are the paths its tests don't cover. Your job is not to re-read its reasoning — it is to independently produce evidence.

Evidence here is literal: each phase writes to a state file, and review.sh check refuses to pass until the required evidence exists. A hook blocks gh pr comment … VERDICT: when check fails, and blocks gh pr merge unconditionally. Skipping a phase makes the review fail, not merely violate a paragraph.

Driver: scripts/review.sh (all commands below). State: ~/.codex/state/review-workflow/pr-<N>.json.

Phases and their gates

#	Phase	Command	Gate (checked by review.sh check)
1	Triage	review.sh triage <N>	CI is pass; CI's SHA == head SHA. Red CI aborts here.
2	Isolate	review.sh worktree <N>	A worktree exists at the PR head.
3	Classify	review.sh classify <N>	Sets diff_class + arms the negative-path gate if a guard is touched.
4	Gates	review.sh gate <N> -- '<cmd>'	≥1 gate command ran; all exited 0.
5	Verify	review.sh negative <N> / finding <N> …	If a guard was touched, negative-path evidence is required.
6	Verdict	review.sh verdict <N> approve|hold	approve is impossible with a CONFIRMED finding; hold requires ≥1.
7	Handoff	review.sh handoff <N> < prompt.md	Only on hold. Rejected unless the prompt is cold-start-complete.
8	Post	gh pr comment <N> --body-file …	Hook runs check; a failing gate blocks the post.
9	Cleanup	review.sh cleanup <N>	Not gated — post-verdict hygiene; check runs at post time, before cleanup can exist. Run it anyway: leftover worktrees are residue.

Run review.sh show <N> at any point to see the accumulated evidence.

1 · Triage

Prints CI state, the SHA CI actually ran on, the head SHA, and the commit sequence. A review of a commit that is no longer the head reviews nothing — the gate enforces that.

Read the commit sequence: if the repo mandates TDD, implementation-then-tests is a finding — but only where a unit-test surface exists. A workflow YAML or a docs page has none; there, "no unit test" is a deviation you rule ACCEPT, not a finding.

"No surface" can also be declared (slice_start --no-surface <regex>; gate evidence prints "extra no-test-surface patterns in force"). The tooling already refuses patterns absent from the armed plan document, so verify only what a gate cannot know: the plan is the genuine build prompt (not an edited copy), and the exempted paths truly have no test runner. Either failing is a finding — the builder muted its own gate.

2 · Isolate

Worktree at ../<repo>-review<N>, diffed as origin/main...HEAD so a stale branch doesn't smear unrelated commits into the diff. It exists so you can write throwaway repro tests without touching the builder's branch or your own checkout.

3 · Classify

Sets diff_class (app / ci / docs) and guard_touched. A guard is anything whose job is to say no: a check, validator, permission test, fallback, gate. CI steps are always guards. When armed, phase 5's negative-path evidence becomes mandatory — see below for why.

4 · Gates

Resolve the repo's own commands (CONTRIBUTING.md, CLAUDE.md, .github/workflows/*) — never assume them, never carry them from another project. Run each through review.sh gate so exit codes land in evidence:

review.sh gate 8 -- 'dotnet build src/App.csproj -c Release'
review.sh gate 8 -- 'dotnet test src/App.Tests/App.Tests.csproj'
review.sh gate 8 -- 'git diff --check origin/main...HEAD'

Then exercise the change where "production" actually is:

Diff class	Production is	Drive it by
app	the running app	headless/server + curl, a CLI call, browser automation
ci	the runner	reproduce the step's script locally and read the step outcome of this PR's own run
infra	the target env	a dry-run/plan; else say plainly it wasn't exercised

5 · Verify — your suspicion is also a claim

Three states, nothing in between:

• CONFIRMED — reproduced, or proven from source with file:line and a concrete failure scenario.
• REFUTED — disproving evidence found. It still goes in the verdict (phase 6).
• PLAUSIBLE — neither. Say so; never dress it as confirmed.

The technique that earns the review its keep: when you suspect a defect, write the failing test. Reading a diff and reasoning about a race produces a story. Running a test produces a fact. (references/worked-example.md walks a real instance: a green suite, a suspicion, a repro test, a confirmed crash.) Then delete your test — you commit nothing to their branch — and quote it verbatim in the punch list so the builder adopts it as their red. Where the diff has no unit-test surface (YAML, a shell step), the repro is a transcript: the commands you ran, the output you saw.

When guard_touched is armed, prove the negative case. A guard's failure mode is a silent pass, so a green signal proves nothing. Feed it bad input and watch it fail: a fixed CI check must still reject dirty input; a validator must still reject; a fallback must show the primary path was not attempted; a permission check must still refuse. Then:

review.sh negative 8 <<'EOF'
$ printf 'bad   \n' >> f.txt && git diff --check
f.txt:2: trailing whitespace.
exit=2   ← the gate fires
EOF

The gate exists because this is the check most likely to be skipped and most likely to matter. It was skipped in this skill's own trial run.

6 · Verdict

Render from references/verdict-template.md. Two sections carry information the builder can't get elsewhere:

• Wins — the good moves, named. Ratifying a correct choice tells the next session to keep making it.
• Verified non-issues (REFUTED) — what you checked and cleared, with evidence. This is a fence: without it, a builder receiving a punch list defensively "improves" adjacent code and grows your next diff.

Rule every deviation from the spec explicitly: ACCEPT (record an amendment) or REJECT (fix required). An unruled deviation becomes ambiguity in the next prompt.

7 · Handoff — the reviewer never fixes

A reviewer who patches the code they just reviewed leaves that patch unreviewed, and every later verdict reads as self-certification. So a hold ends by producing a prompt a different, cold session can execute. check refuses a hold without one.

Write it from references/fix-handoff-template.md and post it as a second PR comment, separate from the verdict — the verdict is analysis, the handoff is instructions.

The test it must pass: if the implementer's context were cleared after finding F2 of four, could it resume from the PR alone? That forces the handoff to carry absolute repo path, branch, PR number, the command to re-read itself, governing docs by path, the exact gate commands, each finding's verbatim repro and its observed failure output, a checkpoint checklist the implementer ticks in the PR as it goes, the fence (verified non-issues + out-of-scope), and the stop condition.

The handoff has a consumer: the fix-workflow skill. A cold implementer executes it by running /fix-workflow <N>, whose gates (fix.py, guard.py) read the structure you write — the C0–C7 spine, F<n> finding ids, AC-n criterion ids, and any force-push licence. Write it from references/fix-handoff-template.md and only there — that file is the canonical spine; every hand-restated copy of it has drifted from the enforcing script before. review.sh handoff rejects a handoff that never names fix-workflow or lacks the C7 spine, for the same reason it rejects one with no STOP.

Your chat message to the human is a courier. The PR comment is the record.

review.sh handoff 9 < handoff.md    # rejected unless cold-start-complete

8 · Post, and close the loop

Punch list goes as a PR comment, three tiers — Required (block merge) / Optional cleanups / Verified non-issues. If the caller asked for a different channel (a file, a dry run), honor it: the delivery channel is theirs, the rigor isn't.

Optional cleanups go to a durable candidates list (project memory, a candidates doc). Whoever writes the next task register starts from it: every item lands, is deferred with a destination, or is dropped with a reason.

Delta re-review when the fix arrives: re-run triage (it re-checks head==CI SHA), then audit only the fix range — git show <fix-sha> --stat, git diff --name-only <old-head>..<fix-sha>. Never re-read the implementer's checklist as evidence. If you offered two fix shapes and they picked one with a reason, evaluate the reason — ratify or correct it.

9 · Cleanup

review.sh cleanup <N> removes the worktree. Leftover worktrees are residue on a shared environment.

Report to the human

One line, their words: "merge it" or "hold: fixes requested" (with the fix prompt). "Looks mostly good" hands back the decision you were asked to make.

On a hold, hand them the /fix-workflow <N> chat message from references/fix-handoff-template.md (its "chat message to the human" block — the one canonical copy). Never a bare "go fix it": without the skill named, the implementer runs unguarded.

Bad news gets the same evidence discipline as good news. Before reporting something broken, verify it independently — a false alarm costs their trust in every later verdict.

Calibration

Never skip a phase; vary the depth. A review that finds nothing is the product — zero-finding approvals are what licence a human to merge without reading 600 lines. One confirmed crash caught across several reviews pays for all of them.

• docs — phase 5 usually finds nothing to reproduce; the guard gate stays disarmed.
• ci — phase 3 arms the guard gate automatically. Watch for event data (github.event.*) interpolated into a run: block instead of passed via env:.
• app — full weight on phases 4–5.
• concurrency / auth / data-loss — expect more than one repro test; assume your first read missed the race.

Setup

The hook is what makes the invariants real. Register it once (see references/setup.md):

{ "hooks": { "PreToolUse": [ { "matcher": "Bash",
  "hooks": [ { "type": "command",
    "command": "$HOME/.codex/skills/review-workflow/scripts/verdict_hook.sh" } ] } ] } }