Skip to content

Harden scripts/codex-review.sh: review in an isolated temp worktree (eliminate force-checkout of the operator's branch) #9

Description

@yihanzhu

Goal / problem

scripts/codex-review.sh checks the PR out into the operator's own checkout with gh pr checkout <PR#> --force. Even with the clean-worktree guard, --force can still silently discard unpushed local commits on that branch — the residual round-3 P1 Codex kept flagging. A reviewer documented as read-only / comments-only must never be able to mutate the operator's working state at all.

Root fix (what Codex pointed at): run the review in an isolated throwaway git worktree checked out at the PR head, so the operator's branch, index, working tree, and unpushed commits are never touched. This eliminates the entire force-reset class and makes "read-only" literally true.

Severity / context: this risk does not occur in the automated loop (the coder always pushes before review), so it is hardening for manual/operator use, not a loop blocker. Worth doing for the reusable-product + truly-read-only guarantee.

Acceptance criteria

  1. Isolated review, no operator-checkout mutation. Run codex exec review --base origin/<base> inside a temporary detached git worktree checked out at the PR head — not the operator's checkout. Remove gh pr checkout, --force, and any branch reset; the operator's current branch / index / working tree / unpushed commits must be provably untouched. (A throwaway git clone is an acceptable fallback if a worktree misbehaves with codex, but a worktree is preferred — it shares the object store and is cheaper.)
  2. Fetch PR refs directly (fork-safe). Get base via gh pr view <PR#> --json baseRefName. Fetch the PR head with git fetch origin pull/<PR#>/head — this works for fork PRs too, where a plain git fetch origin would not bring the head commit into the object store — and add the worktree at that fetched commit. Refresh origin/<base> and review against the qualified origin/<base> so the base is current. (Same-repo PRs — every PR in the automated loop — work either way; the pull/N/head form is what makes it correct for the reusable product.)
  3. Cleanup on every exit. A trap ... EXIT removes the temp worktree (git worktree remove --force) and the temp output file even on failure; tolerate/prune a stale worktree left by a hard-killed previous run (git worktree prune). No leftover worktrees, branches, or temp files. Re-run safe.
  4. Drop the now-unneeded dirty-worktree guard. Since the script no longer touches the operator's tree, remove the clean-worktree abort so the reviewer works even when the operator has local uncommitted work.
  5. All existing invariants preserved: read-only sandbox forced via -c sandbox_mode="read-only" (never --dangerously-bypass-*); comments-only; review posted verbatim via gh pr comment with the cross-vendor header; unset GH_REPO + explicit cwd-derived --repo on every gh call; -m <model> passthrough with no hardcoded model; operates on the current repo's remote.
  6. #!/usr/bin/env bash, set -euo pipefail, shellcheck-clean, executable.
  7. Docs in sync: update the script's header comment block + reviewer/codex-review.md to describe the temp-worktree approach; drop references to force-checkout and the clean-worktree guard. (RESTORE.md describes the mechanism only at a high level — "runs codex exec review, read-only" — and needs no change; just confirm no dangling references remain.)

Likely files

  • scripts/codex-review.sh (rewrite the checkout/review section)
  • reviewer/codex-review.md (mechanism description)

Test expectations

  • CI green: shellcheck-clean, structure check intact, no dangling references. A cheap static assertion that the script no longer references gh pr checkout or --force is encouraged.
  • CI cannot validate this change functionally (no Codex auth / real PR), and shellcheck passes on a script that silently reviews the wrong or empty diff. So: the coder must do a real run against an actual PR and paste the resulting Codex review (or a transcript showing it reviewed the correct diff against the correct base) into the PR description. Required acceptance check, not optional.

Out of scope

  • Changing what Codex checks or the verbatim-posting contract.
  • The Codex GitHub-integration autonomous path (mention only).
  • Any change to the coder routine or Faber.
  • One concern: make the reviewer harness operate in an isolated worktree.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    readyCleared and unclaimed; manager must claim before coder spawn

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions