Use refs/pull/N/head for AI review checkout (unblock /ai-review on merged PRs)#417
Merged
Conversation
|
Overall Assessment ✅ Looks good — no unmitigated P0/P1 findings. Executive Summary
Methodology No findings. Affected method(s): none. Code Quality No findings. Performance No findings. Maintainability No findings. Tech Debt No new findings beyond the already-tracked workflow test-coverage gap noted below. Security No findings. Documentation/Tests
|
The merge ref is garbage-collected on closed/merged PRs, which breaks `/ai-review` reruns on merged PRs at the actions/checkout step with `fatal: couldn't find remote ref refs/pull/<N>/merge`. The head ref persists indefinitely on both open and merged PRs, so swapping to it unblocks post-merge re-reviews without affecting the open-PR path. The diff that the reviewer sees is unchanged: it's computed from the frozen BASE_SHA/HEAD_SHA on the PR object, not from the checked-out filesystem. For open PRs the only visible difference is that Codex now reads files at the PR branch tip rather than at GitHub's test-merge state - the canonical "what the developer pushed" view for code review. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Address the rerun-blocker more robustly: switch the checkout target from `refs/pull/<N>/head` to the immutable head SHA from `pulls.get`. The PR ref is documented as racy right after API-created PR creation (.claude/commands/submit-pr.md:327-345), so unconditionally depending on it leaves `pull_request: opened` reviews vulnerable to checkout failures on a known edge case. The head SHA is frozen on the PR object the moment the PR is created and is fetchable via GitHub's `uploadpack.allowReachableSHA1InWant`, so checkout-by-SHA works in both branches the prior commit was trying to cover: merged-PR reruns (no merge ref) and freshly-opened API PRs (no head ref yet). Also drop the second `refs/pull/<N>/head` dependency in the prefetch step. After checkout-by-SHA, only the base side needs to be fetched to enable `git diff BASE_SHA HEAD_SHA`; the prefetch now pulls the base SHA directly so it survives base-branch movement after PR open. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The previous comment said the head SHA was "frozen the moment the PR is created," which misreads `pulls.get`: it returns the PR's current head SHA at API-call time, which can change as the PR receives more commits. The accurate framing is that the metadata step resolves the head SHA for *this* workflow run, and the checkout step uses that already-resolved value. Same wording fix applied to the base-side race description so it does not over-promise either. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
For owner-created PRs the head repo equals github.repository, so this is a no-op. For fork PRs it lets the workflow fetch head_sha directly from the fork, instead of relying on `refs/pull/<N>/head` having been mirrored to the base repo. The mirror is documented as racy on API-created PRs (.claude/commands/submit-pr.md:327-345), so depending on it could leave fresh fork PR reviews failing at checkout. base_sha still lives on the base repo regardless of where head lives, so the prefetch now adds an explicit `base` remote pointing at github.repository rather than reusing `origin` (which equals the head repo after checkout). `git diff BASE_SHA HEAD_SHA` continues to find both trees locally. head.repo can be null on fork PRs whose fork was deleted; fall back to the base repo's full name in that case so the checkout step still has a sensible target. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three rounds of review feedback have surfaced overlapping checkout robustness concerns. Picking one approach and unifying behind it. Closed/merged PR path (the audit-campaign use case): Use `refs/pull/<N>/head` from the base repo. GitHub keeps this mirror durably regardless of whether the original branch is deleted, the fork is removed, or the merge was rebase/squash. Replaces the prior `refs/pull/<N>/merge` checkout, which is garbage-collected on close. Open PR path: Use `head_sha` from `head_repo_full_name`. For owner PRs the head repo equals github.repository so this is identical to a base-repo checkout. For fork PRs it avoids the documented race where the base repo has not yet mirrored a freshly API-created fork PR's head (.claude/commands/submit-pr.md:327-345). base_sha lives on github.repository regardless of which checkout path runs, so the prefetch step continues to add a `base` remote explicitly. `git diff BASE_SHA HEAD_SHA` therefore finds both trees locally in every covered scenario. The PR state is sourced from `pulls.get` in the existing metadata step (works for both `pull_request` and `issue_comment` events). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
igerber
force-pushed
the
fix-audit-checkout-head
branch
from
def89e1 to
fb1c468
Compare
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment
Executive Summary
Methodology
Code Quality No findings. Performance No findings. Maintainability No additional findings beyond the source-material alignment issue above. Tech Debt No new untracked tech-debt findings. Security No direct secret-handling or permission regression found in the changed lines. Documentation/Tests
Path to Approval
|
The prompt defines HOW the reviewer reviews. Sourcing it from the PR head allowed a PR to silently change its own review rules. Read it from base_sha via `git show` instead; the prefetch step has already fetched that commit's tree. Scope-limited intentionally: docs/methodology/REGISTRY.md and TODO.md remain sourced from the PR head. The prompt itself instructs the reviewer to recognize PR-added Note/Deviation labels in REGISTRY.md and new entries in TODO.md as mitigations (`.github/codex/prompts/ pr_review.md:4,9,62,97`), so those files must reflect the PR's edits to behave correctly. Only the review-rules file is moved. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
🔁 AI review rerun (requested by @igerber) Head SHA: Overall Assessment ✅ Looks good — the prior P1 on trusted review-rule sourcing is resolved, and I did not find any unmitigated P0/P1 issues in this diff. The only remaining item is a P3 workflow-test gap that is already tracked in Executive Summary
Methodology No findings. This is a workflow-only change, not an estimator/maths/inference change. The prior review-control mismatch is resolved because the prompt now comes from Code Quality No findings. Performance No findings. Maintainability No findings. Tech Debt No new untracked tech-debt findings. Security No findings. Documentation/Tests
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
fatal: couldn't find remote ref refs/pull/<N>/merge: GitHub GCs themergeref shortly after PR close.refs/pull/<N>/mergetorefs/pull/<N>/head. Theheadref persists indefinitely on both open and merged PRs.BASE_SHA/HEAD_SHAon the PR object, not from the checked-out filesystem. For open PRs the only visible difference is that Codex now reads file context at the PR branch tip rather than GitHub's test-merge state - the canonical "what the developer pushed" view for code review.git ls-remote https://github.com/igerber/diff-diff.git refs/pull/{401,402,413}/{head,merge}returns SHAs only forheadon all three merged PRs.Motivation
Re-review of merged PRs (e.g. to compare reviewer behavior across CI configurations) is currently impossible without this fix.
Test plan
/ai-reviewcomment on any previously-merged PR should successfully fetchrefs/pull/<N>/head, build the diff, and post a rerun review comment.🤖 Generated with Claude Code