From 8c2e373ca9002b611bacbe7a23a7996833535081 Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 07:27:32 -0400 Subject: [PATCH 1/3] fix(codex-review): fetch PR head/base from the gh-resolved repo, not literal origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In a fork workflow (origin = your fork, upstream = the canonical repo PRs target), `gh` resolves the PR on the canonical repo while the fetch used the literal `origin` remote — which lacks the PR ref (fetch fails) or resolves a same-numbered unrelated PR (wrong diff). Point the fetch at the canonical URL `https://github.com/$repo.git`, where `$repo` is the nameWithOwner `gh` resolved, so the source is provably the repo the review is bound to. Both PR head and base now land in private, review-owned refs under `refs/codex-review/` instead of a remote-tracking ref tied to a remote name: keeps destinations independent of which remote `origin` is and avoids clobbering the operator's `origin/`. The base ref is dropped in the cleanup trap once `--base` has read it (the head ref is deleted immediately after its SHA is captured, as before). Preserves: read-only sandbox, comments-only, verbatim posting, the real Reviewed-head:/Reviewed-base: SHAs, --no-tags, and the narrow destination refspecs. Doc note added to reviewer/codex-review.md. Closes #56 Co-Authored-By: Claude Opus 4.8 --- reviewer/codex-review.md | 30 +++++++++++------- scripts/codex-review.sh | 67 ++++++++++++++++++++++++---------------- 2 files changed, 59 insertions(+), 38 deletions(-) diff --git a/reviewer/codex-review.md b/reviewer/codex-review.md index ce0fb33..5555bf0 100644 --- a/reviewer/codex-review.md +++ b/reviewer/codex-review.md @@ -19,15 +19,23 @@ every `gh` call, so a `GH_REPO` in the environment can't redirect the comment to *different* repo's PR. Then: 1. Derives the PR's base branch (`gh pr view --json baseRefName`) and **fetches the PR - head fork-safely** with explicit, **fully-qualified** refspecs — `git fetch --no-tags origin - +refs/pull//head: +refs/heads/:refs/remotes/origin/`. Both sources are - qualified so a same-named tag on origin (e.g. branch and tag both named `v1.2.0`) can't make - the fetch resolve ambiguously or fail before Codex runs. The `refs/pull//head` source - brings the PR head commit into the object store even for **fork** PRs (a plain `git fetch - origin` would not), and the base is fetched straight into `refs/remotes/origin/` so the - `--base origin/` review is always **current** regardless of the clone's configured fetch - refspecs (a bare `git fetch origin ` would only set `FETCH_HEAD` and could leave - `origin/` stale or missing). The `+` prefixes force-update **only** these two + head fork-safely** with explicit, **fully-qualified** refspecs — `git fetch --no-tags + https://github.com//.git +refs/pull//head: + +refs/heads/:`. The fetch source is the **canonical repo `gh` resolved** + (`/` from `gh repo view --json nameWithOwner`), **not the literal `origin` + remote** — in a fork workflow (`origin` = your fork, `upstream` = the canonical repo PRs + target) `gh` reports the PR on the canonical repo, so fetching from `origin` would fail (the + PR ref doesn't exist on the fork) or silently grab a same-numbered, unrelated PR and review + the wrong diff. Pointing the fetch at `https://github.com//.git` makes the source + **provably** the repo `gh` resolved. Both sources are qualified so a same-named tag (e.g. + branch and tag both named `v1.2.0`) can't make the fetch resolve ambiguously or fail before + Codex runs. The `refs/pull//head` source brings the PR head commit into the object store + even for **fork** PRs (a plain `git fetch` of a branch would not). Both destinations are + private refs we own under `refs/codex-review/` (not a `refs/remotes/origin/*` tracking ref) — + that keeps them independent of which remote `origin` is and avoids clobbering the operator's + `origin/` with a commit fetched from a different URL — and the `--base` review runs + against the freshly-fetched base ref, always **current** regardless of the clone's configured + fetch refspecs. The `+` prefixes force-update **only** these two destination refs we own — never a global `git fetch --force`, which combined with git's tag auto-following could force-update local `refs/tags/*` and mutate operator state; `--no-tags` disables that auto-following so the fetch touches nothing outside the two named refs (the @@ -40,8 +48,8 @@ every `gh` call, so a `GH_REPO` in the environment can't redirect the comment to adds its worktree at a fresh `mktemp` path, a stale entry from a hard-killed previous run never blocks a re-run. (It deliberately avoids a global `git worktree prune`, which is repo-wide and would touch unrelated operator worktrees.) -2. Runs **`codex exec -C review -c sandbox_mode="read-only" --base origin/ -o `** — - Codex's built-in review of the PR head diff vs. its **current** (qualified, remote) base, +2. Runs **`codex exec -C review -c sandbox_mode="read-only" --base refs/codex-review/base -o `** — + Codex's built-in review of the PR head diff vs. its **current** (qualified, freshly-fetched) base, inside the temp worktree (`-C` is a flag on the parent `codex exec`, so it precedes the `review` subcommand). The `-c sandbox_mode="read-only"` override **forces** the read-only sandbox so the review can't inherit a writable default from the operator's Codex config diff --git a/scripts/codex-review.sh b/scripts/codex-review.sh index f8e69a0..284bac0 100755 --- a/scripts/codex-review.sh +++ b/scripts/codex-review.sh @@ -27,13 +27,14 @@ set -euo pipefail # Isolated review — the operator's checkout is never touched. Instead of checking the # PR out into the operator's own working tree (which, even with a clean guard, risks # discarding unpushed commits via a force reset), the script fetches the PR head -# fork-safely (`git fetch origin refs/pull//head`, which brings the head commit into -# the object store even for fork PRs) and adds a DETACHED, throwaway git worktree at -# that exact commit. `codex exec review` runs inside that temp worktree against the -# qualified, freshly-fetched `origin/`, so it always sees the latest head vs. a -# current base. The operator's branch, index, working tree, and unpushed commits are -# provably untouched — "read-only" is literally true — so there is no clean-worktree -# guard, and the reviewer works even when the operator has local uncommitted work. +# fork-safely (`git fetch https://github.com//.git refs/pull//head`, +# which brings the head commit into the object store even for fork PRs) and adds a +# DETACHED, throwaway git worktree at that exact commit. `codex exec review` runs inside +# that temp worktree against the qualified, freshly-fetched base, so it always sees the +# latest head vs. a current base. The operator's branch, index, working tree, and +# unpushed commits are provably untouched — "read-only" is literally true — so there is +# no clean-worktree guard, and the reviewer works even when the operator has local +# uncommitted work. # # Re-run safe: a `trap ... EXIT` removes the temp worktree (`git worktree remove # --force`) and the temp output file even on failure, so this script never leaves a @@ -118,33 +119,42 @@ fi base="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" # Fetch the PR head fork-safely AND refresh the base, into THIS repo's object store, -# using EXPLICIT, FULLY-QUALIFIED refspecs so both land at a known ref regardless of the +# from the CANONICAL repo `gh` resolved (`$repo`), NOT the literal `origin` remote. In a +# fork workflow (`origin` = your fork, `upstream` = the canonical repo PRs target), `gh` +# reports the PR on the canonical repo; fetching from `origin` would fail (the PR ref +# doesn't exist on the fork) or silently grab a same-numbered, unrelated PR — reviewing +# the wrong diff or nothing. Pointing the fetch at `https://github.com/$repo.git` makes +# the source PROVABLY the repo `gh` resolved (`$repo` is its `nameWithOwner`), so the head +# and base always come from the repo the review is bound to. +# +# We use EXPLICIT, FULLY-QUALIFIED refspecs so both land at a known ref regardless of the # clone's configured fetch refspecs. Both sources are qualified (`refs/pull//head` -# and `refs/heads/`) so a same-named tag on origin (e.g. a release branch and tag +# and `refs/heads/`) so a same-named tag on the repo (e.g. a release branch and tag # both named `v1.2.0`) can't make the fetch source resolve ambiguously or fail before # Codex runs. The `refs/pull//head` source brings the PR head commit in even when -# the PR comes from a fork (a plain `git fetch origin` would not); we write it to a -# private local ref we control so its resolution can't be ambiguous. The base is fetched -# straight into its remote-tracking ref (`refs/remotes/origin/`) so that -# `--base origin/` below is always CURRENT — a bare `git fetch origin ` -# would only set FETCH_HEAD and, in a clone without the default `origin/*` mapping, -# could leave origin/ stale or missing and review against an old base. -# Read-only stays literally true: we force-update (the `+` prefix) ONLY these two refs we -# own, never a global `git fetch --force`. A global `--force` plus git's tag -# auto-following could force-update local `refs/tags/*` if origin moved a tag reachable +# the PR comes from a fork (a plain `git fetch` of a branch would not). We write BOTH into +# private local refs we control (under `refs/codex-review/`), never a remote-tracking ref +# tied to a remote name — that keeps the destinations independent of which remote `origin` +# happens to be, and avoids clobbering the operator's `origin/` tracking ref with a +# commit fetched from a different URL. Read-only stays literally true: we force-update +# (the `+` prefix) ONLY these two refs we own and delete both before exit (the head right +# after its SHA is captured, the base in the cleanup trap once `--base` has read it); +# never a global `git fetch --force`. A global `--force` plus git's tag +# auto-following could force-update local `refs/tags/*` if the repo moved a tag reachable # from the fetched commits — an operator-state mutation. `--no-tags` disables that # auto-following, so this fetch touches nothing outside the two named destination refs. pr_head_ref="refs/codex-review/pr-head" -git fetch --no-tags origin \ +base_dest_ref="refs/codex-review/base" +git fetch --no-tags "https://github.com/${repo}.git" \ "+refs/pull/${pr}/head:${pr_head_ref}" \ - "+refs/heads/${base}:refs/remotes/origin/${base}" + "+refs/heads/${base}:${base_dest_ref}" pr_head="$(git rev-parse "$pr_head_ref")" git update-ref -d "$pr_head_ref" -base_ref="origin/${base}" +base_ref="$base_dest_ref" # Record the EXACT base commit Codex reviews against. The review runs `--base -# origin/`, so the effective diff is `pr_head` vs. THIS commit. Capturing it lets a -# later actor (scripts/merge-pr.sh) refuse if the base advanced after the review — a moved -# base changes the merged integration even when the head is unchanged. +# refs/codex-review/base`, so the effective diff is `pr_head` vs. THIS commit. Capturing it +# lets a later actor (scripts/merge-pr.sh) refuse if the base advanced after the review — a +# moved base changes the merged integration even when the head is unchanged. base_head="$(git rev-parse "$base_ref")" # Allocate temp paths in the system temp dir (never inside the repo, so nothing here @@ -155,11 +165,14 @@ base_head="$(git rev-parse "$base_ref")" worktree="$(mktemp -d)" tmp="$(mktemp)" -# Clean up on EVERY exit (success or failure): remove the temp worktree and the temp -# output file. `git worktree remove --force` drops the worktree even though it is at a +# Clean up on EVERY exit (success or failure): remove the temp worktree, the temp output +# file, and the private base ref we own (the head ref was already deleted above once its +# SHA was captured; the base ref must live until `codex exec review --base` reads it, so it +# is dropped here). `git worktree remove --force` drops the worktree even though it is at a # detached head; the rm -rf fallback covers the case where it was never added. cleanup() { git worktree remove --force "$worktree" 2>/dev/null || rm -rf "$worktree" + git update-ref -d "$base_dest_ref" 2>/dev/null || true rm -f "$tmp" } trap cleanup EXIT @@ -175,7 +188,7 @@ git worktree add --detach "$worktree" "$pr_head" # --ignore-user-config so the operator's model/effort defaults still apply. `-C` is a # flag on the parent `codex exec` (not on the `review` subcommand), so it must come # before `review`; it points codex at the temp worktree to review the PR head diff -# against the qualified remote base origin/. +# against the qualified, freshly-fetched base ref (refs/codex-review/base). review_cmd=(codex exec -C "$worktree" review -c sandbox_mode="read-only" --base "$base_ref" -o "$tmp") if [ -n "$model" ]; then review_cmd+=(-m "$model") From 9ece0d0861bd141116520f281eb970433bb87bff Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 07:40:53 -0400 Subject: [PATCH 2/3] fix(codex-review): use gh-resolved clone URL (GHE-safe) + per-run-unique fetch refs Co-Authored-By: Claude Opus 4.8 --- reviewer/codex-review.md | 38 +++++++++++++--------- scripts/codex-review.sh | 70 ++++++++++++++++++++++++---------------- 2 files changed, 65 insertions(+), 43 deletions(-) diff --git a/reviewer/codex-review.md b/reviewer/codex-review.md index 5555bf0..d5ea2a3 100644 --- a/reviewer/codex-review.md +++ b/reviewer/codex-review.md @@ -20,21 +20,27 @@ every `gh` call, so a `GH_REPO` in the environment can't redirect the comment to 1. Derives the PR's base branch (`gh pr view --json baseRefName`) and **fetches the PR head fork-safely** with explicit, **fully-qualified** refspecs — `git fetch --no-tags - https://github.com//.git +refs/pull//head: - +refs/heads/:`. The fetch source is the **canonical repo `gh` resolved** - (`/` from `gh repo view --json nameWithOwner`), **not the literal `origin` - remote** — in a fork workflow (`origin` = your fork, `upstream` = the canonical repo PRs - target) `gh` reports the PR on the canonical repo, so fetching from `origin` would fail (the - PR ref doesn't exist on the fork) or silently grab a same-numbered, unrelated PR and review - the wrong diff. Pointing the fetch at `https://github.com//.git` makes the source - **provably** the repo `gh` resolved. Both sources are qualified so a same-named tag (e.g. - branch and tag both named `v1.2.0`) can't make the fetch resolve ambiguously or fail before - Codex runs. The `refs/pull//head` source brings the PR head commit into the object store - even for **fork** PRs (a plain `git fetch` of a branch would not). Both destinations are - private refs we own under `refs/codex-review/` (not a `refs/remotes/origin/*` tracking ref) — - that keeps them independent of which remote `origin` is and avoids clobbering the operator's - `origin/` with a commit fetched from a different URL — and the `--base` review runs - against the freshly-fetched base ref, always **current** regardless of the clone's configured + +refs/pull//head: +refs/heads/:`. The + fetch source is the **clone URL `gh` returns for the resolved repo** — `gh repo view + --json url` (with `` passed **explicitly** so `gh` views that repo, never a PR-followed + parent), then `.git` appended — **not the literal `origin` remote** and **not a synthesized + `https://github.com/...` URL**. Two reasons: (a) in a fork workflow (`origin` = your fork, + `upstream` = the canonical repo PRs target) `gh` reports the PR on the canonical repo, so + fetching from `origin` would fail (the PR ref doesn't exist on the fork) or silently grab a + same-numbered, unrelated PR and review the wrong diff; (b) using gh's own clone URL keeps the + fetch **host-correct on GitHub Enterprise / non-github.com hosts**, where `nameWithOwner` + resolves the same but a synthesized github.com URL would hit the wrong host (fail, or worse, + an unrelated same-named repo). That makes the source **provably** the repo `gh` resolved. Both + sources are qualified so a same-named tag (e.g. branch and tag both named `v1.2.0`) can't make + the fetch resolve ambiguously or fail before Codex runs. The `refs/pull//head` source + brings the PR head commit into the object store even for **fork** PRs (a plain `git fetch` of + a branch would not). Both destinations are **private, per-run-unique refs we own** under + `refs/codex-review/-/` (not a `refs/remotes/origin/*` tracking ref) — that keeps them + independent of which remote `origin` is, avoids clobbering the operator's `origin/` with + a commit fetched from a different URL, **and** keeps two reviews launched from the same checkout + from colliding (a shared ref name would let a later run's fetch/cleanup force-update or delete + the ref while an earlier run is still resolving `--base`). The `--base` review runs against the + freshly-fetched per-run base ref, always **current** regardless of the clone's configured fetch refspecs. The `+` prefixes force-update **only** these two destination refs we own — never a global `git fetch --force`, which combined with git's tag auto-following could force-update local `refs/tags/*` and mutate operator state; `--no-tags` @@ -48,7 +54,7 @@ every `gh` call, so a `GH_REPO` in the environment can't redirect the comment to adds its worktree at a fresh `mktemp` path, a stale entry from a hard-killed previous run never blocks a re-run. (It deliberately avoids a global `git worktree prune`, which is repo-wide and would touch unrelated operator worktrees.) -2. Runs **`codex exec -C review -c sandbox_mode="read-only" --base refs/codex-review/base -o `** — +2. Runs **`codex exec -C review -c sandbox_mode="read-only" --base refs/codex-review/-/base -o `** — Codex's built-in review of the PR head diff vs. its **current** (qualified, freshly-fetched) base, inside the temp worktree (`-C` is a flag on the parent `codex exec`, so it precedes the `review` subcommand). The `-c sandbox_mode="read-only"` override **forces** the read-only diff --git a/scripts/codex-review.sh b/scripts/codex-review.sh index 284bac0..68d4b05 100755 --- a/scripts/codex-review.sh +++ b/scripts/codex-review.sh @@ -27,8 +27,9 @@ set -euo pipefail # Isolated review — the operator's checkout is never touched. Instead of checking the # PR out into the operator's own working tree (which, even with a clean guard, risks # discarding unpushed commits via a force reset), the script fetches the PR head -# fork-safely (`git fetch https://github.com//.git refs/pull//head`, -# which brings the head commit into the object store even for fork PRs) and adds a +# fork-safely (`git fetch refs/pull//head`, from the clone URL +# gh returns for the resolved repo — host-correct on GitHub Enterprise too — which brings +# the head commit into the object store even for fork PRs) and adds a # DETACHED, throwaway git worktree at that exact commit. `codex exec review` runs inside # that temp worktree against the qualified, freshly-fetched base, so it always sees the # latest head vs. a current base. The operator's branch, index, working tree, and @@ -118,14 +119,23 @@ fi # / from the cwd). base="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" +# Resolve the CLONE URL `gh` returns for the repo it bound the review to, on the CORRECT +# host. We pass `$repo` EXPLICITLY to `gh repo view` so it reports THAT repo's URL — never a +# PR-followed parent/upstream. Using gh's own URL (not a synthesized `https://github.com/...`) +# keeps the fetch host-correct on GitHub Enterprise / non-github.com hosts, where +# `nameWithOwner` resolves the same but the canonical host is NOT github.com — a synthesized +# github.com URL there would fail or, worse, hit an unrelated same-named repo. `gh repo view` +# returns the web URL (no `.git` suffix); we append `.git` for the fetch. +repo_url="$(gh repo view "$repo" --json url -q .url)" + # Fetch the PR head fork-safely AND refresh the base, into THIS repo's object store, -# from the CANONICAL repo `gh` resolved (`$repo`), NOT the literal `origin` remote. In a -# fork workflow (`origin` = your fork, `upstream` = the canonical repo PRs target), `gh` -# reports the PR on the canonical repo; fetching from `origin` would fail (the PR ref -# doesn't exist on the fork) or silently grab a same-numbered, unrelated PR — reviewing -# the wrong diff or nothing. Pointing the fetch at `https://github.com/$repo.git` makes -# the source PROVABLY the repo `gh` resolved (`$repo` is its `nameWithOwner`), so the head -# and base always come from the repo the review is bound to. +# from the CANONICAL repo `gh` resolved (`$repo` / `$repo_url`), NOT the literal `origin` +# remote. In a fork workflow (`origin` = your fork, `upstream` = the canonical repo PRs +# target), `gh` reports the PR on the canonical repo; fetching from `origin` would fail (the +# PR ref doesn't exist on the fork) or silently grab a same-numbered, unrelated PR — +# reviewing the wrong diff or nothing. Pointing the fetch at gh's own clone URL makes the +# source PROVABLY the repo `gh` resolved, so the head and base always come from the repo the +# review is bound to. # # We use EXPLICIT, FULLY-QUALIFIED refspecs so both land at a known ref regardless of the # clone's configured fetch refspecs. Both sources are qualified (`refs/pull//head` @@ -133,28 +143,33 @@ base="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" # both named `v1.2.0`) can't make the fetch source resolve ambiguously or fail before # Codex runs. The `refs/pull//head` source brings the PR head commit in even when # the PR comes from a fork (a plain `git fetch` of a branch would not). We write BOTH into -# private local refs we control (under `refs/codex-review/`), never a remote-tracking ref -# tied to a remote name — that keeps the destinations independent of which remote `origin` -# happens to be, and avoids clobbering the operator's `origin/` tracking ref with a -# commit fetched from a different URL. Read-only stays literally true: we force-update -# (the `+` prefix) ONLY these two refs we own and delete both before exit (the head right -# after its SHA is captured, the base in the cleanup trap once `--base` has read it); -# never a global `git fetch --force`. A global `--force` plus git's tag +# private, PER-RUN-UNIQUE local refs we control (under `refs/codex-review/-/`), +# never a remote-tracking ref tied to a remote name — that keeps the destinations +# independent of which remote `origin` happens to be, avoids clobbering the operator's +# `origin/` tracking ref with a commit fetched from a different URL, AND keeps two +# reviews launched from the SAME checkout from colliding (a shared ref name would let a later +# run's fetch force-update — or its cleanup delete — the ref while an earlier run is still +# resolving `--base`, reviewing the wrong base or failing spuriously; the temp worktree is +# already per-run via mktemp, so the refs now match). Read-only stays literally true: we +# force-update (the `+` prefix) ONLY these two refs we own and delete both before exit (the +# head right after its SHA is captured, the base in the cleanup trap once `--base` has read +# it); never a global `git fetch --force`. A global `--force` plus git's tag # auto-following could force-update local `refs/tags/*` if the repo moved a tag reachable # from the fetched commits — an operator-state mutation. `--no-tags` disables that # auto-following, so this fetch touches nothing outside the two named destination refs. -pr_head_ref="refs/codex-review/pr-head" -base_dest_ref="refs/codex-review/base" -git fetch --no-tags "https://github.com/${repo}.git" \ +run_ref_ns="refs/codex-review/${pr}-$$" +pr_head_ref="${run_ref_ns}/head" +base_dest_ref="${run_ref_ns}/base" +git fetch --no-tags "${repo_url}.git" \ "+refs/pull/${pr}/head:${pr_head_ref}" \ "+refs/heads/${base}:${base_dest_ref}" pr_head="$(git rev-parse "$pr_head_ref")" git update-ref -d "$pr_head_ref" base_ref="$base_dest_ref" # Record the EXACT base commit Codex reviews against. The review runs `--base -# refs/codex-review/base`, so the effective diff is `pr_head` vs. THIS commit. Capturing it -# lets a later actor (scripts/merge-pr.sh) refuse if the base advanced after the review — a -# moved base changes the merged integration even when the head is unchanged. +# "$base_ref"` (the per-run base ref), so the effective diff is `pr_head` vs. THIS commit. +# Capturing it lets a later actor (scripts/merge-pr.sh) refuse if the base advanced after the +# review — a moved base changes the merged integration even when the head is unchanged. base_head="$(git rev-parse "$base_ref")" # Allocate temp paths in the system temp dir (never inside the repo, so nothing here @@ -166,10 +181,11 @@ worktree="$(mktemp -d)" tmp="$(mktemp)" # Clean up on EVERY exit (success or failure): remove the temp worktree, the temp output -# file, and the private base ref we own (the head ref was already deleted above once its -# SHA was captured; the base ref must live until `codex exec review --base` reads it, so it -# is dropped here). `git worktree remove --force` drops the worktree even though it is at a -# detached head; the rm -rf fallback covers the case where it was never added. +# file, and the private per-run base ref we own (the head ref was already deleted above once +# its SHA was captured; the base ref must live until `codex exec review --base` reads it, so +# it is dropped here). Deleting only this run's own `refs/codex-review/-/base` never +# disturbs a concurrent run's refs. `git worktree remove --force` drops the worktree even +# though it is at a detached head; the rm -rf fallback covers the case where it was never added. cleanup() { git worktree remove --force "$worktree" 2>/dev/null || rm -rf "$worktree" git update-ref -d "$base_dest_ref" 2>/dev/null || true @@ -188,7 +204,7 @@ git worktree add --detach "$worktree" "$pr_head" # --ignore-user-config so the operator's model/effort defaults still apply. `-C` is a # flag on the parent `codex exec` (not on the `review` subcommand), so it must come # before `review`; it points codex at the temp worktree to review the PR head diff -# against the qualified, freshly-fetched base ref (refs/codex-review/base). +# against the qualified, freshly-fetched per-run base ref (refs/codex-review/-/base). review_cmd=(codex exec -C "$worktree" review -c sandbox_mode="read-only" --base "$base_ref" -o "$tmp") if [ -n "$model" ]; then review_cmd+=(-m "$model") From ee75ac8865bf8453350d5d79b2409451b8821021 Mon Sep 17 00:00:00 2001 From: Yihan Zhu <48186361+yihanzhu@users.noreply.github.com> Date: Sat, 27 Jun 2026 07:50:38 -0400 Subject: [PATCH 3/3] fix: respect configured remote transport for PR fetch (round-2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex round-1 [P2]: the fetch synthesized gh's HTTPS web URL (`git fetch "${repo_url}.git"`), which carries no credentials — so on private repos or SSH-only-authenticated checkouts the fetch failed even though `gh auth status` passed and `origin` worked, aborting the review before Codex ran. Replace the synthesized-URL fetch with remote-matching: resolve gh's canonical host + owner/repo, normalize each configured git remote's URL (scp-style SSH, ssh://, https; trailing `.git` stripped; case-insensitive) and fetch from whichever remote name matches (preferring `origin`). This uses the operator's own authenticated transport (fork-safe + host-correct on GHE + auth-correct). If no configured remote matches the gh-resolved repo, refuse with an actionable error and non-zero exit — never fall back to an unauthenticated synthesized URL. Per-run-unique refs, cleanup trap, `--no-tags`, fully-qualified refspecs, forced read-only sandbox, comments-only, and the Reviewed-head/base markers are all preserved. Doc updated to match. Live behavior unchanged until the operator re-runs the script (no install.sh sync needed for the reviewer harness). Co-Authored-By: Claude Opus 4.8 --- reviewer/codex-review.md | 43 ++++++++++----- scripts/codex-review.sh | 115 ++++++++++++++++++++++++++++++++------- 2 files changed, 122 insertions(+), 36 deletions(-) diff --git a/reviewer/codex-review.md b/reviewer/codex-review.md index d5ea2a3..0f6c752 100644 --- a/reviewer/codex-review.md +++ b/reviewer/codex-review.md @@ -20,24 +20,37 @@ every `gh` call, so a `GH_REPO` in the environment can't redirect the comment to 1. Derives the PR's base branch (`gh pr view --json baseRefName`) and **fetches the PR head fork-safely** with explicit, **fully-qualified** refspecs — `git fetch --no-tags - +refs/pull//head: +refs/heads/:`. The - fetch source is the **clone URL `gh` returns for the resolved repo** — `gh repo view - --json url` (with `` passed **explicitly** so `gh` views that repo, never a PR-followed - parent), then `.git` appended — **not the literal `origin` remote** and **not a synthesized - `https://github.com/...` URL**. Two reasons: (a) in a fork workflow (`origin` = your fork, - `upstream` = the canonical repo PRs target) `gh` reports the PR on the canonical repo, so - fetching from `origin` would fail (the PR ref doesn't exist on the fork) or silently grab a - same-numbered, unrelated PR and review the wrong diff; (b) using gh's own clone URL keeps the - fetch **host-correct on GitHub Enterprise / non-github.com hosts**, where `nameWithOwner` - resolves the same but a synthesized github.com URL would hit the wrong host (fail, or worse, - an unrelated same-named repo). That makes the source **provably** the repo `gh` resolved. Both - sources are qualified so a same-named tag (e.g. branch and tag both named `v1.2.0`) can't make + +refs/pull//head: +refs/heads/:`. The fetch source is + the **configured git remote whose URL resolves to the repo `gh` bound the review to** — *not* + the literal `origin`, *not* a synthesized `https://github.com/...` URL. The script resolves + gh's canonical identity (`gh repo view --json url` for the **host**, with `` passed + **explicitly** so `gh` views that repo, never a PR-followed parent; `` itself is the + `owner/repo`), then iterates `git remote`, **normalizes** each remote's URL to `host` + + `owner/repo` (handling scp-style `git@host:owner/repo(.git)`, `ssh://git@host/owner/repo(.git)`, + and `https://host/owner/repo(.git)` — trailing `.git` stripped, compared case-insensitively), + and **selects the remote that matches** gh's host + `owner/repo` (preferring `origin` when it is + itself the match). It then fetches from that **remote name**. Three reasons this is right: + (a) **auth-correct** — using the operator's configured remote means the fetch uses the + operator's own transport and credentials (SSH key, gh's git credential helper, etc.). A + synthesized HTTPS *web* URL carries no credentials, so on **private repos or + SSH-only-authenticated checkouts** `git fetch ` would fail even though `gh auth status` + passes and `origin` works — aborting the review before Codex runs. (b) **fork-safe** — we match + on the gh-resolved identity, not blind `origin`: in a fork workflow (`origin` = your fork, + `upstream` = the canonical repo PRs target) `gh` reports the PR on the canonical repo, so we + select `upstream`; fetching from `origin` would fail (the PR ref doesn't exist on the fork) or + silently grab a same-numbered, unrelated PR and review the wrong diff. (c) **host-correct** — + it's the operator's real remote URL, so GitHub Enterprise / non-github.com hosts work, where a + synthesized github.com URL would hit the wrong host. That makes the source **provably** the repo + `gh` resolved. If **no** configured remote matches the gh-resolved repo, the script **refuses** + with an actionable error (`add it (e.g. 'git remote add upstream ') and re-run`) and a + non-zero exit — it does **not** fall back to an unauthenticated synthesized URL. Both sources + are qualified so a same-named tag (e.g. branch and tag both named `v1.2.0`) can't make the fetch resolve ambiguously or fail before Codex runs. The `refs/pull//head` source brings the PR head commit into the object store even for **fork** PRs (a plain `git fetch` of a branch would not). Both destinations are **private, per-run-unique refs we own** under - `refs/codex-review/-/` (not a `refs/remotes/origin/*` tracking ref) — that keeps them - independent of which remote `origin` is, avoids clobbering the operator's `origin/` with - a commit fetched from a different URL, **and** keeps two reviews launched from the same checkout + `refs/codex-review/-/` (not a `refs/remotes//*` tracking ref) — that keeps them + independent of which remote we selected, avoids clobbering the operator's `/` with + a commit fetched into our own ref, **and** keeps two reviews launched from the same checkout from colliding (a shared ref name would let a later run's fetch/cleanup force-update or delete the ref while an earlier run is still resolving `--base`). The `--base` review runs against the freshly-fetched per-run base ref, always **current** regardless of the clone's configured diff --git a/scripts/codex-review.sh b/scripts/codex-review.sh index 68d4b05..906bbba 100755 --- a/scripts/codex-review.sh +++ b/scripts/codex-review.sh @@ -27,9 +27,11 @@ set -euo pipefail # Isolated review — the operator's checkout is never touched. Instead of checking the # PR out into the operator's own working tree (which, even with a clean guard, risks # discarding unpushed commits via a force reset), the script fetches the PR head -# fork-safely (`git fetch refs/pull//head`, from the clone URL -# gh returns for the resolved repo — host-correct on GitHub Enterprise too — which brings -# the head commit into the object store even for fork PRs) and adds a +# fork-safely (`git fetch refs/pull//head`, from the configured git remote +# whose URL matches the repo gh resolved — so the operator's own authenticated transport is +# used, which works on private repos and SSH-only checkouts where a synthesized web URL would +# fail; fork-safe + host-correct on GitHub Enterprise too — which brings the head commit into +# the object store even for fork PRs) and adds a # DETACHED, throwaway git worktree at that exact commit. `codex exec review` runs inside # that temp worktree against the qualified, freshly-fetched base, so it always sees the # latest head vs. a current base. The operator's branch, index, working tree, and @@ -119,23 +121,94 @@ fi # / from the cwd). base="$(gh pr view "$pr" --repo "$repo" --json baseRefName -q .baseRefName)" -# Resolve the CLONE URL `gh` returns for the repo it bound the review to, on the CORRECT -# host. We pass `$repo` EXPLICITLY to `gh repo view` so it reports THAT repo's URL — never a -# PR-followed parent/upstream. Using gh's own URL (not a synthesized `https://github.com/...`) -# keeps the fetch host-correct on GitHub Enterprise / non-github.com hosts, where -# `nameWithOwner` resolves the same but the canonical host is NOT github.com — a synthesized -# github.com URL there would fail or, worse, hit an unrelated same-named repo. `gh repo view` -# returns the web URL (no `.git` suffix); we append `.git` for the fetch. +# Resolve gh's canonical repo identity — the HOST + `owner/repo` of the repo it bound the +# review to. `$repo` is already the `owner/repo` (`nameWithOwner`); we pass it EXPLICITLY to +# `gh repo view` so the URL reports THAT repo — never a PR-followed parent/upstream — and we +# read the HOST off that URL so the match below is host-correct on GitHub Enterprise / +# non-github.com hosts (where `nameWithOwner` resolves the same but the canonical host is NOT +# github.com). `gh repo view` returns the web URL (e.g. `https://HOST/owner/repo`). repo_url="$(gh repo view "$repo" --json url -q .url)" -# Fetch the PR head fork-safely AND refresh the base, into THIS repo's object store, -# from the CANONICAL repo `gh` resolved (`$repo` / `$repo_url`), NOT the literal `origin` -# remote. In a fork workflow (`origin` = your fork, `upstream` = the canonical repo PRs -# target), `gh` reports the PR on the canonical repo; fetching from `origin` would fail (the -# PR ref doesn't exist on the fork) or silently grab a same-numbered, unrelated PR — -# reviewing the wrong diff or nothing. Pointing the fetch at gh's own clone URL makes the -# source PROVABLY the repo `gh` resolved, so the head and base always come from the repo the -# review is bound to. +# Normalize a git remote URL (or gh web URL) to "//", lowercased, with any +# trailing `.git` stripped. Handles the three common transports without fragile regex (pure +# parameter expansion + `case`): +# git@host:owner/repo(.git) (scp-style SSH) +# ssh://git@host/owner/repo(.git) (ssh:// URL, optional user@) +# https://host/owner/repo(.git) (https, optional user@host) +# Prints the normalized id, or nothing if the URL doesn't parse to host + owner/repo. +normalize_repo_id() { + local url="$1" host rest + case "$url" in + *://*) + # scheme://[user@]host/owner/repo... — drop scheme, then any leading userinfo, then split host/path. + rest="${url#*://}" + rest="${rest#*@}" + host="${rest%%/*}" + rest="${rest#*/}" + ;; + *@*:*) + # scp-style git@host:owner/repo — drop userinfo, split on the FIRST colon. + rest="${url#*@}" + host="${rest%%:*}" + rest="${rest#*:}" + ;; + *) + # Unrecognized form — can't match. + return 0 + ;; + esac + # `rest` is now the path "owner/repo[/...]"; keep exactly the first two segments. + local owner="${rest%%/*}" + rest="${rest#*/}" + local name="${rest%%/*}" + name="${name%.git}" + [ -n "$host" ] && [ -n "$owner" ] && [ -n "$name" ] || return 0 + printf '%s/%s/%s' "$host" "$owner" "$name" | tr '[:upper:]' '[:lower:]' +} + +# Compute gh's normalized identity to match against: host (from `$repo_url`) + `owner/repo` +# (`$repo`). We synthesize a normalizable string from the gh-resolved host and `$repo`. +gh_host="$(normalize_repo_id "$repo_url")"; gh_host="${gh_host%%/*}" +gh_repo_id="$(normalize_repo_id "https://${gh_host}/${repo}")" + +# Select the configured git remote whose URL resolves to the SAME host + owner/repo that gh +# bound the review to, and fetch from THAT REMOTE NAME — so the operator's own configured +# transport AND credentials are used. This is what makes the fetch work on private repos and +# SSH-only-authenticated checkouts: a synthesized HTTPS web URL carries no credentials, so +# `git fetch ` would fail there even though `gh auth status` passes and `origin` works. +# We do NOT blindly use `origin`: in a fork workflow `origin` = your fork while the PR lives +# on `upstream`, so we match on identity (fork-safe). `origin` is only PREFERRED when it is +# itself the match. If NO configured remote resolves to gh's repo, we REFUSE (below) rather +# than fall back to an unauthenticated synthesized URL. +selected_remote="" +while IFS= read -r remote_name; do + [ -n "$remote_name" ] || continue + remote_url="$(git remote get-url "$remote_name" 2>/dev/null)" || continue + [ -n "$remote_url" ] || continue + if [ "$(normalize_repo_id "$remote_url")" = "$gh_repo_id" ]; then + if [ "$remote_name" = "origin" ]; then + selected_remote="origin" + break + fi + [ -n "$selected_remote" ] || selected_remote="$remote_name" + fi +done < <(git remote) + +if [ -z "$selected_remote" ]; then + echo "error: the repo gh resolved (${repo}) is not reachable via any configured git remote;" >&2 + echo " add it (e.g. 'git remote add upstream ') and re-run" >&2 + exit 1 +fi + +# Fetch the PR head fork-safely AND refresh the base, into THIS repo's object store, from the +# SELECTED REMOTE NAME (not a synthesized URL) so the operator's configured transport + +# credentials are used. The remote was chosen by matching gh's canonical host + owner/repo, so +# the source is PROVABLY the repo `gh` resolved — fork-safe (we match identity, not blindly +# `origin`), host-correct (it's the operator's real remote URL, GHE included), and auth-correct +# (the operator's transport: SSH key, gh credential helper, etc.). In a fork workflow (`origin` +# = your fork, `upstream` = the canonical repo PRs target) we'd select `upstream`; fetching from +# `origin` there would fail (the PR ref doesn't exist on the fork) or grab a same-numbered, +# unrelated PR. # # We use EXPLICIT, FULLY-QUALIFIED refspecs so both land at a known ref regardless of the # clone's configured fetch refspecs. Both sources are qualified (`refs/pull//head` @@ -145,8 +218,8 @@ repo_url="$(gh repo view "$repo" --json url -q .url)" # the PR comes from a fork (a plain `git fetch` of a branch would not). We write BOTH into # private, PER-RUN-UNIQUE local refs we control (under `refs/codex-review/-/`), # never a remote-tracking ref tied to a remote name — that keeps the destinations -# independent of which remote `origin` happens to be, avoids clobbering the operator's -# `origin/` tracking ref with a commit fetched from a different URL, AND keeps two +# independent of which remote we selected, avoids clobbering the operator's +# `/` tracking ref with a commit fetched into our own ref, AND keeps two # reviews launched from the SAME checkout from colliding (a shared ref name would let a later # run's fetch force-update — or its cleanup delete — the ref while an earlier run is still # resolving `--base`, reviewing the wrong base or failing spuriously; the temp worktree is @@ -160,7 +233,7 @@ repo_url="$(gh repo view "$repo" --json url -q .url)" run_ref_ns="refs/codex-review/${pr}-$$" pr_head_ref="${run_ref_ns}/head" base_dest_ref="${run_ref_ns}/base" -git fetch --no-tags "${repo_url}.git" \ +git fetch --no-tags "$selected_remote" \ "+refs/pull/${pr}/head:${pr_head_ref}" \ "+refs/heads/${base}:${base_dest_ref}" pr_head="$(git rev-parse "$pr_head_ref")"