fix(codex-review): fetch PR head/base from the gh-resolved repo, not literal origin

[yihanzhu]

Closes #56

What changed

scripts/codex-review.sh now fetches the PR head and base from the repo gh resolved ($repo) instead of the literal origin remote.

• Fetch source is now https://github.com/$repo.git, where $repo is the nameWithOwner returned by gh repo view. This makes the fetch source provably the repo gh bound the review to.
• Both the PR head and the base now land in private, review-owned refs under refs/codex-review/ (refs/codex-review/pr-head, refs/codex-review/base) rather than a refs/remotes/origin/* tracking ref. That keeps the destinations independent of which remote origin happens to be, and avoids clobbering the operator's origin/<base> tracking ref with a commit fetched from a different URL.
• The base ref is deleted in the existing cleanup trap once codex exec review --base has read it (the head ref is still deleted immediately after its SHA is captured, as before).
• reviewer/codex-review.md step 1/2 updated to document the canonical-URL fetch source and the fork-workflow rationale.

Why

In a fork workflow (origin = your fork, upstream = the canonical repo PRs target), gh reports the PR on the canonical repo while git fetch origin +refs/pull/<PR#>/head:... hit the fork — where the PR ref doesn't exist (fetch fails) or a same-numbered unrelated PR silently resolves (wrong diff reviewed). Fetching from the canonical URL gh resolved closes that gap.

Chosen mechanism: canonical URL (not remote-match)

AC #1 offered two options. I chose fetch directly from https://github.com/$repo.git over parsing git remote -v for a URL match because:

• It is provably the repo gh resolved — $repo is the same nameWithOwner string, no remote-table parsing/normalization (SSH vs HTTPS, .git suffix, trailing slash) to get wrong.
• It needs no fallback-to-origin branch, keeping the existing narrow-refspec / --no-tags / temp-ref-cleanup behavior the simplest (AC Add RESTORE.md — runbook to rebuild the whole team from this repo #1's stated preference).
• It does not depend on the operator having configured an upstream remote at all.

Preserved (AC #2)

Read-only forced sandbox, comments-only single gh pr comment, verbatim posting, the real Reviewed-head: / Reviewed-base: SHA markers, --no-tags, and the narrow destination refspecs that touch nothing else. The fetch still force-updates only the two refs it owns; no global git fetch --force.

How verified

• shellcheck -S style scripts/codex-review.sh — clean (same flag CI uses).
• Full CI structure check (ci/required-files.txt manifest) — green locally; script remains #!/usr/bin/env bash, set -euo pipefail, executable.
• bash -n syntax check — OK.
• Reasoned through the fork-clone case: head + base both come from the canonical repo URL regardless of what origin points at; private refs avoid mutating operator tracking refs; cleanup drops the base ref on every exit path.

Self-modification note

This is the reviewer-harness script (scripts/codex-review.sh). It takes live effect on merge — scripts run from disk, so no /faber re-sync is needed. (faber-command.md is unchanged; a re-run of scripts/install.sh is only required when that file changes.)

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 8c2e373
Reviewed-base: f70362f

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 introduces a synthesized github.com fetch URL that breaks gh-recognized repositories on other hosts, and the new shared temporary base ref is unsafe for concurrent reviews. These issues can prevent or invalidate the review workflow in supported scenarios.

Full review comments:

• [P2] Fetch from gh's resolved clone URL — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.QEnQ5X356I/scripts/codex-review.sh:148-148
  When the current repo is a GitHub Enterprise repo or otherwise not hosted at github.com, gh repo view can still resolve nameWithOwner, but this fetch now goes to https://github.com/<owner>/<repo>.git instead of the host/URL that gh resolved. Those reviews will fail before Codex runs, or could hit an unrelated github.com repo with the same owner/name; use a clone URL returned by gh rather than synthesizing a github.com URL.

• [P2] Make the base ref unique per review — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.QEnQ5X356I/scripts/codex-review.sh:147-147
  If two reviews are launched from the same checkout before the first codex exec review finishes, both processes share refs/codex-review/base; the later fetch can force-update it, or its cleanup can delete it, while the earlier review is still resolving --base. That can make the earlier run review the wrong base or fail despite recording a different Reviewed-base, so the temporary ref should include a per-run component such as the PR number plus PID/temp name.

[yihanzhu]

Round-1 review fixes applied (head 9ece0d0):

1. gh-resolved, host-correct clone URL (GHE-safe). The fork-safe fetch now sources from the clone URL gh returns for the resolved repo — repo_url="$(gh repo view "$repo" --json url -q .url)", with .git appended — instead of a synthesized https://github.com/<owner>/<repo>.git. $repo is passed explicitly so gh reports that repo (never a PR-followed parent/upstream). On GitHub Enterprise / non-github.com hosts nameWithOwner resolves the same but the canonical host is not github.com, so a synthesized github.com URL would fail or — worse — hit an unrelated same-named repo. Using gh’s own URL makes the fetch source provably the repo the review is bound to, on the correct host.

2. Per-run-unique fetch refs (no concurrent-review collisions). The fetch destinations are now namespaced per run: run_ref_ns="refs/codex-review/${pr}-$$" → ${run_ref_ns}/head and ${run_ref_ns}/base, replacing the shared refs/codex-review/pr-head / refs/codex-review/base. The cleanup trap deletes only this run’s own refs/codex-review/<PR#>-<PID>/base, so two reviews launched from the same checkout can no longer force-update or delete each other’s refs while the other is still resolving --base. This matches the worktree, which was already per-run via mktemp.

CI green locally: shellcheck -S style scripts/codex-review.sh (CI’s exact flag) clean, bash -n parses, structure check (ci/required-files.txt) ok and the script stays +x. reviewer/codex-review.md updated in sync. No behavior change to the read-only / comments-only / verbatim contract.

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: 9ece0d0
Reviewed-base: f70362f

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 updated fetch path can break the reviewer for common private-repo or SSH-authenticated setups despite valid gh authentication. That makes the patch unsafe as-is.

Review comment:

• [P2] Use authenticated transport for fetches — /var/folders/rm/bkx4f4b91vqfn79vk02pcd140000gn/T/tmp.GYGEJMGVjS/scripts/codex-review.sh:163-165
  For private repositories or checkouts that authenticate Git over SSH only, this now fetches from the HTTPS URL built from gh repo view --json url, which is just the web URL with .git appended; gh authentication does not automatically supply credentials to git fetch unless the user also configured gh's git credential helper. Those users can pass the preflight gh auth status and have a working origin, but the review aborts before Codex runs. Use an authenticated clone URL/credential helper, or respect the configured remote transport, for this fetch.

[yihanzhu]

Coder — round-1 fix (now round-2), head ee75ac8

Addressed Codex's round-1 [P2] (authenticated transport for the PR fetch).

What changed — scripts/codex-review.sh no longer fetches from a synthesized HTTPS web URL (git fetch "${repo_url}.git"), which carried no credentials and so failed on private repos / SSH-only-authenticated checkouts (review aborted before Codex ran, even with gh auth status passing). It now respects the configured remote transport:

1. Resolves gh's canonical identity — host from gh repo view <repo> --json url (repo passed explicitly so it's never a PR-followed parent) + owner/repo from $repo.
2. Iterates git remote, normalizes each remote URL to host + owner/repo via pure parameter-expansion/case (no fragile regex), handling scp-style git@host:owner/repo(.git), ssh://git@host/owner/repo(.git), and https://host/owner/repo(.git); strips trailing .git; compares case-insensitively.
3. Selects the remote whose normalized identity matches gh's (preferring origin when it is itself the match) and fetches from that remote name — so the operator's own transport + credentials are used (fork-safe + host-correct on GHE + auth-correct).
4. If no configured remote matches the gh-resolved repo → refuses with an actionable error to stderr and non-zero exit (add it (e.g. 'git remote add upstream <url>') and re-run). No fallback to an unauthenticated synthesized URL.

Preserved: per-run-unique refs (refs/codex-review/<PR#>-<PID>/{head,base}), cleanup trap (deletes the per-run base ref), --no-tags, fully-qualified refspecs, forced read-only sandbox, comments-only, verbatim posting, Reviewed-head:/Reviewed-base: markers, set -euo pipefail, executable bit. In-script comments and reviewer/codex-review.md updated to describe the remote-match approach.

Live behavior note: the reviewer harness takes effect when the operator re-runs the script; no install.sh sync is needed for this file.

Local checks all green: shellcheck -S style scripts/codex-review.sh, bash -n scripts/codex-review.sh, and the CI structure check (run under bash).

[yihanzhu]

Codex reviewer (cross-vendor, read-only)

Reviewed-head: ee75ac8
Reviewed-base: f70362f

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 changes appear consistent with the stated goal of selecting an authenticated matching remote and using per-run refs for the reviewed base. I did not identify any discrete regression that would clearly break existing behavior.