feat: add scripts/codex-review.sh reviewer harness + document the in-session review loop

[yihanzhu]

Closes #7

What

• New scripts/codex-review.sh — the Codex cross-vendor reviewer harness. Usage scripts/codex-review.sh [-m <model>] <owner>/<repo> <PR#>. It derives the PR base, gh pr checkouts it, runs codex exec review --base <base> -o <tmpfile>, and posts Codex's review to the PR verbatim via gh pr comment, under a short header marking it the cross-vendor reviewer.
• Repurposed reviewer/codex-review.md — from a paste-able rubric into documentation of the real mechanism, the invariants, the synchronous in-session loop, and the alternatives (Codex GitHub integration = autonomous upgrade; codex-plugin-cc = interactive).
• README — Reviewer team-table row now reads "Codex via scripts/codex-review.sh (in-session) / GitHub integration (autonomous)"; added the script to the Layout block.
• RESTORE.md — reviewer recreate step now references scripts/codex-review.sh and the GitHub-integration option instead of "paste the reviewer prompt".

Why

The reviewer is Codex (cross-vendor), but posting its review was ad-hoc and the docs still implied a hand-written rubric was used. This encapsulates the real flow (Codex's built-in codex exec review, not a custom prompt — --base can't take one) and reconciles the docs.

How verified

• shellcheck scripts/codex-review.sh (v0.11.0) — clean (both tracked scripts clean).
• bash -n syntax check — OK.
• CI structure check run locally — structure ok (all required files present, including the kept reviewer/codex-review.md).
• Confirmed against codex-cli 0.129.0: codex exec review exposes --base, -o/--output-last-message, -m/--model; gh pr comment -F - reads from stdin.
• No dangling references; net +120 lines (well under the ~300–400 budget).

Invariants preserved

Cross-vendor (coder=Claude, reviewer=Codex), reviewer read-only (no --dangerously-bypass-*), comments-only (only side effect is one gh pr comment), and verbatim posting (no Claude session edits the review). Per CLAUDE.md: this is docs/tooling — no live-routine prompt changed.

🤖 Generated with Claude Code

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Posted verbatim by scripts/codex-review.sh (codex exec review --base main). Comments only — Codex never pushes, approves, or merges.

The new reviewer harness does not ensure Codex runs inside the repository being reviewed, so its advertised target-repo workflow can produce failed or incorrect reviews. This is a blocking correctness issue for the script's primary purpose.

Review comment:

• [P1] Run Codex from the target repo checkout — /Users/yihanzhu/git/fabrica/scripts/codex-review.sh:53-53
  When Faber invokes this control-plane script for a target repo other than the current checkout, the --repo flag only tells gh which PR to fetch; codex exec review still runs in whatever directory launched the script. That means the review either fails or compares the target PR against the Fabrica checkout/local main, then posts the wrong result to the PR. Clone/cd into the requested repo, or fail unless cwd matches it, before running Codex.

[yihanzhu]

Resolved the Codex P1 (round-1).

The footgun was real: codex exec review always ran in the launching dir, but the <owner>/<repo> arg implied the script worked for any repo — so running it from the wrong dir would review the wrong code and post to the wrong PR.

Fix — made the contract honest instead of just guarding it:

• Removed the <owner>/<repo> positional arg. New signature: scripts/codex-review.sh [-m <model>] <PR#>.
• Operates on the current repo: gh infers owner/repo from the cwd's remote; gh pr view / gh pr checkout / gh pr comment all run without --repo, against the same checkout codex exec review sees.
• Guard at the top: errors out unless the cwd is a git repo with a gh-recognized remote (gh repo view --json nameWithOwner), telling you to run it from within the target repo's clone.
• All invariants intact: set -euo pipefail, mktemp+trap cleanup, read-only (no bypass flag), verbatim post with the cross-vendor header, optional -m passthrough, shellcheck-clean + executable.
• Docs updated to the new signature + run-from-the-clone model: reviewer/codex-review.md (usage block + loop diagram), RESTORE.md (reviewer recreate step). README only names the script, no signature change.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Posted verbatim by scripts/codex-review.sh (codex exec review --base main). Comments only — Codex never pushes, approves, or merges.

The new review harness can operate on stale checkout refs in the intended repeated-review loop, does not enforce its stated read-only safety invariant, and the documented invocation path does not work from separate target repos. These issues should be addressed before considering the patch correct.

Full review comments:

• [P1] Refresh local refs before reviewing revisions — /Users/yihanzhu/git/fabrica/scripts/codex-review.sh:59-60
  When this loop is re-run after a coder pushes fixes to the same PR, the local PR branch may already exist from the previous run; gh pr checkout without --force does not reset an existing local branch to the latest PR head. Combined with passing the unqualified base branch name, Codex can review stale head/base refs, so it may repost findings for old code or miss new changes. Fetch/reset the PR head and base before invoking Codex.

• [P2] Force Codex into the read-only review sandbox — /Users/yihanzhu/git/fabrica/scripts/codex-review.sh:68-68
  If the operator's Codex CLI config/profile defaults to a writable sandbox, this call inherits that setting because the script never overrides it; not passing --dangerously-bypass-approvals-and-sandbox does not by itself enforce the non-negotiable read-only invariant. For PRs reviewed from a local checkout, force read-only/never or ignore user config before running the agent.

• [P2] Use an executable path that works from target repos — /Users/yihanzhu/git/fabrica/reviewer/codex-review.md:27-28
  The instructions say to run this from inside the target repository, but this relative command only exists in the Fabrica control-plane repo; a separate target clone will not have scripts/codex-review.sh unless it has been copied there, so the documented harness fails with No such file or directory. Document an absolute/path-in-PATH invocation or add an install/copy step while keeping the cwd set to the target repo.

[yihanzhu]

Resolved all three round-1 Codex findings (round-2). Commit 9ebe7e5.

[P1] Stale refs on re-run (scripts/codex-review.sh)

• Added git fetch origin before checkout, switched to gh pr checkout "$pr" --force (resets an existing local PR branch to the latest head instead of leaving a stale leftover), and now review against the qualified remote base origin/<base> rather than an unqualified local branch name. A re-run after a coder pushes fixes always reviews the latest head against a current base.

[P2] Enforce read-only (scripts/codex-review.sh)

• The review now forces the sandbox via -c sandbox_mode="read-only" on the codex exec review call, so it can't inherit a writable default from the operator's Codex config. Verified the exact key empirically: codex exec review has no -s/--sandbox flag (only the parent codex exec does), and an invalid value yields unknown variant ... expected one of read-only, workspace-write, danger-full-access in sandbox_mode``, confirming the key is sandbox_mode with value `read-only`. Deliberately did not use `--ignore-user-config` (that would also drop the operator's model/effort defaults); approval is already `never` for review.

[P2] Script path fails from a target repo (docs)

• Documented invoking the harness by absolute path from within the target repo's clone — "$HOME/git/fabrica/scripts/codex-review.sh" <PR#> — plus a one-line "add <fabrica>/scripts to your PATH" alias option. No copy-into-each-repo step. Updated reviewer/codex-review.md (usage block, read-only invariant, loop diagram) and RESTORE.md (in-session harness step). README only names the script (no runnable invocation), so it didn't need a change.

Invariants intact: cross-vendor, read-only (now forced), comments-only, verbatim post + cross-vendor header, mktemp+trap cleanup, cwd guard, set -euo pipefail. shellcheck clean; structure check unchanged.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Posted verbatim by codex-review.sh (codex exec review --base origin/main, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The new review harness can target the wrong GitHub repository when gh is configured via GH_REPO, and it can destructively reset local PR branches despite being presented as read-only/comments-only. These should be addressed before relying on the script.

Full review comments:

• [P2] Pin gh commands to the checkout repository — /Users/yihanzhu/git/fabrica/scripts/codex-review.sh:59-59
  When GH_REPO is set, gh repo view and the later gh pr view/checkout/comment calls target that environment-selected repository instead of the git remote for the cwd. Codex still reviews the cwd checkout, so in that environment the script can post a review of one repo to a PR in another repo. Unset/override GH_REPO or derive and pass the repo from the current checkout explicitly before making the gh calls.

• [P2] Guard before force-resetting the PR branch — /Users/yihanzhu/git/fabrica/scripts/codex-review.sh:75-75
  When a local branch for this PR already exists, gh pr checkout --force resets it to the PR head; if the operator has local-only commits or dirty work on that branch, this can discard that work before the read-only review runs. Since the harness is documented as comments-only/read-only, add a clean-worktree check or use a temporary worktree/clone instead of force-resetting the user's checkout.

[yihanzhu]

Resolved both round-2 Codex P2 findings (round-3). Commit f0225b5.

[P2] Pin gh to the checkout's repo (GH_REPO) (scripts/codex-review.sh)

• unset GH_REPO near the top, then derive repo="$(gh repo view --json nameWithOwner -q .nameWithOwner)" from the cwd's remote, and pass an explicit --repo "$repo" to every subsequent gh call (gh pr view, gh pr checkout, gh pr comment) — belt-and-suspenders. A GH_REPO in the environment can no longer redirect the review comment to a different repo's PR. The cwd guard now also captures repo (and errors out if empty/unresolved). codex exec review still runs against the cwd checkout, so it reviews the correct diff.

[P2] Don't clobber local work on force-checkout (scripts/codex-review.sh)

• Before gh pr checkout --force, added a clean-worktree guard: if git status --porcelain is non-empty, abort with a clear stderr error (worktree has uncommitted changes; commit or stash before running the reviewer) and exit non-zero. The force checkout (kept for the re-run/stale-ref fix) now never silently discards the operator's local-only commits or dirty changes — consistent with the documented read-only / comments-only contract. Went with the clean-worktree guard over a temp worktree/clone: lean, and it never destroys work.

Docs — reviewer/codex-review.md: documented the GH_REPO unset + explicit --repo pinning and the clean-worktree guard (in the "how it runs" step list and the Invariants section).

Invariants intact: cross-vendor, read-only forced via -c sandbox_mode="read-only", comments-only single gh pr comment, verbatim post + cross-vendor header, mktemp+trap cleanup, cwd guard, qualified base origin/<base>, set -euo pipefail. shellcheck clean; structure check passes.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Posted verbatim by codex-review.sh (codex exec review --base origin/main, sandbox forced read-only). Comments only — Codex never pushes, approves, or merges.

The new harness can discard unpublished local commits in a documented read-only workflow. That data-loss risk should be fixed before considering the patch correct.

Review comment:

• [P1] Avoid force-resetting unpublished local PR commits — /Users/yihanzhu/git/fabrica/scripts/codex-review.sh:97-97
  When an existing local branch for this PR is clean but contains commits that have not been pushed (for example, fixes made locally before re-running the reviewer), gh pr checkout --force resets that branch to the PR head. The git status --porcelain guard only catches uncommitted changes, so this can discard local commits despite the script’s read-only/no-destroy guarantee; check for ahead/diverged commits or review from a detached/fresh worktree before forcing.