Harden codex-review.sh: review in an isolated temp worktree (Closes #9) by yihanzhu · Pull Request #11 · yihanzhu/ystack

yihanzhu · 2026-06-22T23:48:11Z

What & why

scripts/codex-review.sh previously checked the PR out into the operator's own checkout via gh pr checkout <PR#> --force. Even behind the clean-worktree guard, --force could silently discard unpushed local commits — a reviewer documented as read-only / comments-only must never be able to mutate operator state.

This rewrites the checkout/review section to run the review in an isolated, detached, throwaway git worktree at the PR head. The operator's branch, index, working tree, and unpushed commits are provably untouched, so the entire force-reset / clean-guard class is eliminated and "read-only" is literally true.

Closes #9

Changes

Fetch PR head fork-safely: git fetch origin pull/<PR#>/head <base> brings the head commit into the object store even for fork PRs (a plain git fetch origin would not) and refreshes origin/<base>. Resolve FETCH_HEAD and git worktree add --detach <tmpdir> <head>.
Review in isolation: codex exec -C <tmpdir> review -c sandbox_mode="read-only" --base origin/<base> -o <tmp>. (-C is a parent-codex exec flag, so it precedes the review subcommand — verified against codex-cli 0.129.0.)
Cleanup on every exit: trap ... EXIT runs git worktree remove --force <tmpdir> (rm -rf fallback) + rm -f <tmp>; git worktree prune first tolerates a stale worktree from a hard-killed run. Re-run safe.
Dropped the dirty-worktree guard — the script no longer touches the operator's tree, so it now works even with local uncommitted changes.
Invariants preserved: -c sandbox_mode="read-only" (never --dangerously-bypass-*); comments-only; verbatim gh pr comment with the cross-vendor header; unset GH_REPO + explicit cwd-derived --repo; -m <model> passthrough (no hardcoded model); current repo only.
Docs: updated the script header block + reviewer/codex-review.md to the temp-worktree mechanism; dropped force-checkout / clean-guard references. RESTORE.md and README.md need no change (confirmed no dangling references).

How tested

shellcheck scripts/codex-review.sh — clean; same find . -name '*.sh' | xargs shellcheck as CI — clean.
CI structure check (required files) — passes.
Static assertion: no gh pr checkout, no bare --force (only git worktree remove --force).
Arg-order verified: codex exec -C <dir> review parses; codex exec review -C does not.
Live functional run: evidence posted as a follow-up comment below (the rewritten script run against this very PR).

Run `codex exec review` inside a detached, throwaway git worktree checked out at the PR head instead of `gh pr checkout <PR#> --force` against the operator's own checkout. This eliminates the force-reset class entirely: the operator's branch, index, working tree, and unpushed commits are never touched, so the read-only reviewer can no longer mutate operator state. - Fetch the PR head fork-safely (`git fetch origin pull/<PR#>/head <base>`) so fork PRs work too; resolve FETCH_HEAD and add the worktree there. - Review against the qualified, freshly-fetched `origin/<base>`. - `trap ... EXIT` removes the temp worktree (`git worktree remove --force`) and temp file even on failure; `git worktree prune` tolerates a stale worktree from a hard-killed run, so re-runs are safe. - Drop the now-unneeded clean-worktree guard; the reviewer works even with local uncommitted changes. - Preserve all invariants: `-c sandbox_mode="read-only"`, comments-only, verbatim `gh pr comment` with the cross-vendor header, `unset GH_REPO` + cwd-derived `--repo`, `-m <model>` passthrough, current-repo only. - Update the script header + reviewer/codex-review.md to describe the temp-worktree approach; drop force-checkout / clean-guard references. Closes #9 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

yihanzhu · 2026-06-22T23:58:27Z

Codex reviewer (cross-vendor, read-only)

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

The patch mostly improves isolation, but the unconditional repo-wide worktree prune can affect unrelated operator worktrees and violates the stated isolation guarantee.

Review comment:

[P2] Avoid pruning unrelated worktrees — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.aWq4qYtA3s/scripts/codex-review.sh:103-103
In repositories that already have linked worktrees, this prunes the repo-wide worktree registry, not just leftovers from this script; if another worktree path is absent past gc.worktreePruneExpire (for example an unmounted portable or network worktree), running the reviewer will remove its metadata. That is an operator-state mutation unrelated to the temp worktree, so the script should avoid a global prune or only clean worktrees it created.

yihanzhu · 2026-06-23T00:03:52Z

Codex reviewer (cross-vendor, read-only)

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

The new isolated worktree flow can review against a stale base because the rewritten fetch no longer refreshes the remote-tracking base ref used by Codex.

Review comment:

[P1] Refresh the remote-tracking base ref — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.6TGR937Ldj/scripts/codex-review.sh:96-98
When the local origin/<base> is stale or missing, this explicit source-only fetch does not update refs/remotes/origin/<base>; it only writes the fetched branch to FETCH_HEAD. Since the review still runs with --base origin/${base}, reruns after the base branch advances can silently review against an old base, or fail in clones that do not already have that remote-tracking ref. Fetch the base into refs/remotes/origin/${base} or run a normal git fetch origin as well.

…ktree prune Address Codex review on PR #11 (issue #9): P1 — Stale/missing remote-tracking base ref. The previous source-only fetch (`git fetch origin pull/<PR#>/head <base>`) only updated FETCH_HEAD, leaving `refs/remotes/origin/<base>` stale or missing, so a re-run after the base advanced could review against an old base (or fail in a clone lacking that tracking ref). Now fetch with explicit destination refspecs: `git fetch --force origin pull/<PR#>/head:<tmpref> <base>:refs/remotes/origin/<base>`, so `--base origin/<base>` always diffs against a current base. The head goes to a private local ref (resolved, then deleted) to avoid FETCH_HEAD ambiguity. P2 — Repo-wide `git worktree prune` mutated operator state. Removed the global prune entirely (it could drop registry metadata for unrelated operator worktrees past gc.worktreePruneExpire). Cleanup now removes only this script's own temp worktree via the trap; re-run safety comes from each run using a fresh mktemp path. Updated the script header block and reviewer/codex-review.md to match (refspec fetch; no global prune). All invariants preserved: isolated detached temp worktree, no gh pr checkout, no bare --force on the operator's branch, -c sandbox_mode="read-only", comments-only verbatim gh pr comment with the cross-vendor header, unset GH_REPO + cwd-derived --repo, -m <model> passthrough. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

yihanzhu · 2026-06-23T00:32:53Z

Coder revision — round-1 (addresses both Codex findings)

Both findings fixed in 601d700. No push-back — both were valid.

[P1] Stale/missing remote-tracking base ref. The fetch was source-only
(git fetch origin pull/<PR#>/head <base>), which only wrote FETCH_HEAD and left
refs/remotes/origin/<base> stale or missing — so a re-run after the base advanced
could review against an old base, or fail in a clone lacking that tracking ref.
Now the fetch uses explicit destination refspecs:

git fetch --force origin \
  "pull/${pr}/head:refs/codex-review/pr-head" \
  "${base}:refs/remotes/origin/${base}"

so --base origin/<base> always diffs against a freshly-fetched base. The PR head is
written to a private local ref (resolved into $pr_head, then deleted) to avoid any
FETCH_HEAD ambiguity.

[P2] Repo-wide git worktree prune mutated operator state. Removed the global
prune entirely — it is repo-wide and could drop registry metadata for unrelated
operator worktrees (e.g. an unmounted portable/network worktree past
gc.worktreePruneExpire). Cleanup now removes only this script's own temp
worktree, by its explicit mktemp path, in the trap ... EXIT. Re-run safety comes
from each run using a fresh mktemp -d path, so a stale entry from a hard-killed run
never blocks git worktree add.

Docs in sync: updated the script's header comment block and reviewer/codex-review.md
step 1 to describe the refspec fetch and to note the global prune is deliberately avoided.

Invariants preserved: isolated detached temp worktree; no gh pr checkout; no bare
--force on the operator's branch (the only --force uses are git fetch and
git worktree remove on the script's own temp dir); -c sandbox_mode="read-only" (no
--dangerously-bypass-*); comments-only verbatim gh pr comment with the cross-vendor
header; unset GH_REPO + cwd-derived --repo; -m <model> passthrough;
#!/usr/bin/env bash, set -euo pipefail, executable bit.

Verified statically (did not run the harness against a live PR, to avoid posting a
duplicate review): shellcheck scripts/codex-review.sh clean; CI structure check and
CI-style find . -name '*.sh' | xargs shellcheck both pass; confirmed by reading the
code that refs/remotes/origin/<base> is refreshed before --base origin/<base> and
that cleanup removes only the script's own temp worktree with no global prune.

yihanzhu · 2026-06-23T00:38:13Z

Codex reviewer (cross-vendor, read-only)

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

The new isolated worktree flow is generally sound, but the explicit fetch uses an unqualified base ref that can break valid repositories with same-named branch/tag refs. That can prevent the reviewer from running on affected PRs.

Review comment:

[P2] Fully qualify the fetched base ref — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.AvPmiuNFo9/scripts/codex-review.sh:105-107
When the PR base branch name also exists as a tag on origin (for example a release branch/tag named v1.2.0), this unqualified fetch source can resolve ambiguously or fail before Codex runs. Since the review only needs the branch, fetch it as refs/heads/${base}:refs/remotes/origin/${base} (and consider fully qualifying the PR ref as refs/pull/...) so same-named tags cannot break the reviewer.

The base-ref fetch used unqualified refspec sources. If a base branch name also exists as a tag on origin (e.g. a release branch and tag both named v1.2.0), the unqualified source can resolve ambiguously or fail before Codex runs. Qualify both sources: pull/<PR#>/head -> refs/pull/<PR#>/head <base> -> refs/heads/<base> Fetch behavior is otherwise unchanged. Header comment block and reviewer/codex-review.md updated to quote the qualified refspecs. Addresses round-2 Codex [P2] finding on PR #11. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

yihanzhu · 2026-06-23T00:41:45Z

Coder revision — round-2 (addresses Codex [P2])

Fixed in 6c3e58b. No push-back — the finding was valid.

[P2] Unqualified fetch refspec sources. The base-ref git fetch used unqualified
sources, so if a base branch name also exists as a tag on origin (e.g. a release branch
and tag both named v1.2.0), the source could resolve ambiguously or fail before Codex
runs. Both sources are now fully qualified:

git fetch --force origin \
  "refs/pull/${pr}/head:${pr_head_ref}" \
  "refs/heads/${base}:refs/remotes/origin/${base}"

Fetch behavior is otherwise unchanged — same --force, same private head ref resolved
into $pr_head then deleted, same base landing in refs/remotes/origin/<base> for
--base origin/<base>.

Docs in sync: updated the script's header comment block (both the top isolation
summary and the fetch comment) and reviewer/codex-review.md step 1 to quote the
fully-qualified refspecs and explain why.

Invariants preserved: isolated detached temp worktree; no gh pr checkout; no bare
--force on the operator's branch (the only --force uses remain git fetch and
git worktree remove on the script's own temp dir); -c sandbox_mode="read-only";
comments-only verbatim gh pr comment with the cross-vendor header; unset GH_REPO +
cwd-derived --repo; -m <model> passthrough; set -euo pipefail; executable bit.

Verified statically (did not run the harness against a live PR): shellcheck scripts/codex-review.sh clean; CI structure check passes; CI-style
find . -name '*.sh' | xargs shellcheck clean.

yihanzhu · 2026-06-23T00:48:35Z

Codex reviewer (cross-vendor, read-only)

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

The patch can still mutate operator Git state by force-updating local tags during the fetch, which conflicts with the read-only invariant the change is meant to strengthen.

Review comment:

[P2] Scope forced fetches to the two refs — /private/var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.anmqrsOAra/scripts/codex-review.sh:108-108
When origin has a moved tag reachable from the fetched PR head or base, the global git fetch --force can also force-update auto-followed refs/tags/*. That violates the script's read-only/operator-state guarantee by rewriting local tags even though only the PR head and base refs need forced updates; use per-ref + refspecs and/or --no-tags instead of a global force.

yihanzhu · 2026-06-23T00:50:17Z

Faber — escalating to human (round-3 cap)

CI has been green every round. Reached the ~3-round cap without convergence, so flagging needs-human per the loop.

Round history (all findings valid, all accepted by the coder):

round-0 (initial PR): [P1] stale base ref · [P2] repo-wide git worktree prune touches operator's other worktrees.
round-1 (both fixed): [P2] base fetched with an unqualified ref source — ambiguous if a branch and tag share a name.
round-2 (fixed): [P2] global git fetch --force can force-update auto-followed local tags (refs/tags/*) — operator-state mutation.

Why escalating, not auto-fixing: each round resolves the prior finding but the reviewer surfaces a progressively more marginal operator-state edge case in the same git fetch line. None of these conditions exist in this control-plane repo (no name-colliding branch/tag, no moved origin tags). The current finding's fix is a one-liner (--no-tags / per-ref + refspecs), but per the round cap this is a human judgment call: do one final round, or merge as-is.

Decision left to the human (who holds the merge gate).

The PR head + base fetch used a global `git fetch --force`. Git auto-follows tags, so a global `--force` could also force-update local `refs/tags/*` if origin moved a tag reachable from the fetched commits — an operator-state mutation that violates the script's read-only guarantee. Stop forcing globally: force-update ONLY the two destination refs we own via `+`-prefixed refspecs, and pass `--no-tags` to disable tag auto-following. The fetch now touches nothing outside the PR-head temp ref and origin/<base>. Header comment block and reviewer/codex-review.md step 1 updated to match. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

yihanzhu · 2026-06-23T00:55:10Z

Coder revision — fetch no longer mutates operator tags (`5629604`)

Addresses the [P2] global---force tag finding. No push-back — valid.

The PR head + base fetch used a global git fetch --force. Because git auto-follows tags, that global --force could also force-update local refs/tags/* whenever origin had moved a tag reachable from the fetched commits — an operator-state mutation that breaks the script's read-only guarantee.

Fix (scripts/codex-review.sh): the fetch is now

git fetch --no-tags origin \
  "+refs/pull/${pr}/head:${pr_head_ref}" \
  "+refs/heads/${base}:refs/remotes/origin/${base}"

Dropped the global --force; force-update is now scoped to only the two destination refs we own, via the + prefix on each refspec.
Added --no-tags to disable tag auto-following, so the fetch can't touch refs/tags/*.
The PR-head destination ref (refs/codex-review/pr-head) and base-ref handling are otherwise unchanged.

Docs in sync: the script header comment block and reviewer/codex-review.md step 1 now describe the --no-tags / +-refspec fetch and why a global --force is avoided.

Invariants preserved: isolated detached temp worktree; no gh pr checkout; no --force on the operator's branch (the only remaining --force is git worktree remove --force on the script's own temp dir); -c sandbox_mode="read-only"; comments-only verbatim gh pr comment with the cross-vendor header; unset GH_REPO + cwd-derived --repo; -m passthrough; set -euo pipefail; executable bit.

Verified statically (did not run the harness against a live PR): shellcheck scripts/codex-review.sh clean; CI structure check passes; CI-style find . -name '*.sh' | xargs shellcheck clean.

yihanzhu · 2026-06-23T01:00:56Z

Codex reviewer (cross-vendor, read-only)

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

The temp-worktree approach is mostly sound, but the shared temporary ref creates a concrete race for concurrent invocations in the same repository and can lead to reviewing/commenting on the wrong PR head.

Review comment:

[P2] Use a per-run PR head ref — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.I1euJaGhZr/scripts/codex-review.sh:112-117
When two instances of this script run concurrently in the same repo, this fixed refs/codex-review/pr-head ref is shared: one fetch can rewrite or delete it between the other run's fetch and rev-parse/update-ref, causing a worktree to be created at the wrong PR head or the run to fail before posting. Since the new worktree flow otherwise makes parallel reviews plausible, use a unique ref per run (for example including the PR and PID/temp suffix) or avoid a persistent shared ref.

yihanzhu · 2026-06-23T01:02:05Z

Faber — PR #11 capped at final round (recommend MERGE)

Final fix (5629604) applied the --no-tags + per-ref + refspec change. CI green.

Codex's final re-review raised one further [P2]: the shared refs/codex-review/pr-head ref could race if two reviews run concurrently in the same repo. Valid in theory, but it cannot occur in the fabrica loop — Faber runs reviews strictly one at a time (one coder/reviewer at a time is an enforced invariant). Per the agreed round cap, stopping here — no round-4.

Recommendation: merge as-is. The substantive issue-#9 hardening is complete and CI is green:

review runs in an isolated, detached, throwaway temp worktree;
no gh pr checkout --force on the operator's branch;
no global git worktree prune;
base ref freshly fetched and fully qualified (refs/heads/<base>);
no tag mutation (--no-tags + +-scoped refspecs).

The findings Codex peeled off across rounds 1–final are progressively more theoretical edge cases (name-colliding branch/tag, moved origin tags, concurrent same-repo runs) — none of which exist or occur in this control-plane repo's sequential loop.

Optional future follow-up if you ever want the script safe for concurrent manual use: a per-run unique PR-head ref (PID/temp suffix). Not needed for the loop. Say the word and I'll open an issue.

yihanzhu added the round-0 Review-loop counter: initial PR label Jun 23, 2026

yihanzhu added round-1 Review-loop counter: revision 1 and removed round-0 Review-loop counter: initial PR labels Jun 23, 2026

yihanzhu added round-2 Review-loop counter: revision 2 and removed round-1 Review-loop counter: revision 1 labels Jun 23, 2026

yihanzhu added round-3 Review-loop counter: revision 3 (cap) needs-human Escalation: plan refresh, round cap, ambiguous spec, size, or failure and removed round-2 Review-loop counter: revision 2 labels Jun 23, 2026

yihanzhu merged commit eb0c433 into main Jun 23, 2026
1 check passed

yihanzhu deleted the issue-9-isolated-review-worktree branch June 23, 2026 12:33

Navigation Menu

Harden codex-review.sh: review in an isolated temp worktree (Closes #9) - #11

Harden codex-review.sh: review in an isolated temp worktree (Closes #9)#11
yihanzhu merged 4 commits into
mainfrom
issue-9-isolated-review-worktree

yihanzhu commented Jun 22, 2026

Uh oh!

yihanzhu commented Jun 22, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

yihanzhu commented Jun 23, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

yihanzhu commented Jun 22, 2026

What & why

Changes

How tested

Uh oh!

yihanzhu commented Jun 22, 2026

Codex reviewer (cross-vendor, read-only)

Uh oh!

yihanzhu commented Jun 23, 2026

Codex reviewer (cross-vendor, read-only)

Uh oh!

yihanzhu commented Jun 23, 2026

Coder revision — round-1 (addresses both Codex findings)

Uh oh!

yihanzhu commented Jun 23, 2026

Codex reviewer (cross-vendor, read-only)

Uh oh!

yihanzhu commented Jun 23, 2026

Coder revision — round-2 (addresses Codex [P2])

Uh oh!

yihanzhu commented Jun 23, 2026

Codex reviewer (cross-vendor, read-only)

Uh oh!

yihanzhu commented Jun 23, 2026

Faber — escalating to human (round-3 cap)

Uh oh!

yihanzhu commented Jun 23, 2026

Coder revision — fetch no longer mutates operator tags (5629604)

Uh oh!

yihanzhu commented Jun 23, 2026

Codex reviewer (cross-vendor, read-only)

Uh oh!

yihanzhu commented Jun 23, 2026

Faber — PR #11 capped at final round (recommend MERGE)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Coder revision — fetch no longer mutates operator tags (`5629604`)