Repository navigation
Fix reviewer checkout after merged PR branch deletion - #71210
Conversation
Fetch the retained pull-request head ref for same-repository PR events instead of relying on a source branch that can disappear while queued. Reset the local branch from the fetched head, preserve lifecycle/error handling, and cover deleted refs with isolated real Git regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/matt |
|
/review |
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
Record the runtime RS-05a ref-isolation boundary and dated regression evidence without adding a compiler threat rule or changing checkout provenance policy. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design (via pr-triage: bug_fix). The root-cause fix is sound — switching from git fetch origin <branchName> to the retained refs/pull/<number>/head ref correctly eliminates the race where a merged PR's source branch is deleted before the agent checks it out, and the fix is backed by a strong new real-Git integration test covering open/closed payload states and shallow/full clones.
📋 Key Themes & Highlights
Key Themes
- Doc placement: the new paragraph explaining retained-ref checkout behavior sits under "Disabling the PR Branch Checkout", which is about an unrelated opt-out flag.
- Minor duplication: the
origin/pr-headcheckout target literal isn't derived from the existingPR_HEAD_BASE_REFconstant, so the two could drift.
Positive Highlights
- ✅
/diagnosing-bugs: root cause (branch-name fetch racing against branch deletion) is fixed at the source, not papered over — the retained PR ref survives deletion. - ✅
/tdd: the newit.eachintegration test exercises real git repos (not just mocks) across open/closed × shallow/full, directly reproducing the reported failure mode (PR #71195 run). - ✅ Error classification between "missing-ref invalidation" and genuine auth/permission/network failures is preserved and still tested.
- ✅ Existing tests were updated consistently rather than left stale.
Both findings above are minor (non-blocking) and left as inline suggestions; approving since there are no correctness or test-coverage gaps.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~82 AIC · ⌖ ~17 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /codebase-design — no blocking issues. Only observations.
📋 Key Themes & Highlights
Key Themes
- Root-cause fix: switching to the retained
refs/pull/<n>/headref (instead of the branch name) directly addresses the race condition rather than masking it, and the fix is applied symmetrically in both the fast-path and fallback branches. - Minor duplication (
/codebase-design): after this change, the fast-path (isFork === false & !isClosed) and fallback branches incheckout_pr_branch.cjs(around lines 469-540) both now dofetchPRHead([...PR_HEAD_BASE_REF]) → git checkout -B branchName origin/pr-head → exportPRHeadBaseline(...). The only remaining differences are howbranchName/commitCountare derived and the log strings. This could be extracted into a shared helper (e.g.checkoutRetainedPRHead({ prNumber, branchName, commitCount })) to reduce the chance the two paths drift again in a future fix — not blocking, just a deepening opportunity since the two paths are converging. - Excellent regression coverage (
/tdd//diagnosing-bugs): the newit.eachreal-Git test (open/closed × shallow/full) actually deletes the remote branch and asserts the exact failure that was observed in production (https://github.com/github/gh-aw/actions/runs/39829351695), then verifies the fix resolves it end-to-end rather than only at the mock level. This is a strong example of reproducing before fixing. - Docs updated appropriately:
checkout.mdnow explains that the local branch name is kept but the remote branch is not recreated, which matches the implementation.
Positive Highlights
- ✅ Real-Git regression test directly reproduces the reported production failure mode (deleted branch + stale local ref) rather than relying only on mocks.
- ✅ Shallow/full-history behavior, trusted-actor checks, and lifecycle-invalidation semantics are explicitly preserved and covered by both an old and a new regression test.
- ✅ Symmetry: the same-repo fast path now matches the retained-ref strategy already used by the fork/
pull_request_targetpath, removing a special case that was the actual source of the bug.
No actionable blocking issues found; the minor duplication note above is optional follow-up, not a requirement for merge.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · ~77 AIC · ⌖ ~16 AIC · ⊞ 10.2K
Comment /matt to run again
There was a problem hiding this comment.
Verdict
No actionable blocking issues found in the changed lines. The retained PR-head ref fetch and forced local branch reset address the reported deleted-source-branch race while preserving checkout failure handling and baseline export behavior. Review was static; repository tests were not run.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt50 · ~7 AIC · ⌖ ~1 AIC · ⊞ 1.6K
Comment /review to run again
|
|
||
| core.info(`Checking out branch: ${branchName}`); | ||
| await exec.exec("git", ["checkout", branchName]); | ||
| await exec.exec("git", ["checkout", "-B", branchName, "origin/pr-head"]); |
There was a problem hiding this comment.
This checkout can create an unintended upstream for the local branch because origin/pr-head is a remote-tracking start point, so a later plain git push may target a stray pr-head branch rather than the PR source branch.
💡 Prevent accidental pushes to the synthetic ref
Usegit checkout -B --no-track <branchName> origin/pr-head (and apply the same option in the fallback path) so the synthetic retained ref is only a checkout source and never becomes the branch's push target.Separate checkout lifecycle behavior from the opt-out reference and use PR_HEAD_BASE_REF as the checkout source in both paths. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Regarding #71210 (comment): The requested Matt review completed, and both actionable inline suggestions have been accepted and addressed. No additional code change is needed for the command itself, and I did not dispatch or rerun a workflow. |
|
Regarding #71210 (comment): The review feedback has been evaluated and the documentation-placement and shared-ref suggestions have been addressed. No code change is needed for the command itself, and this batch does not dispatch or rerun a workflow. |
|
Regarding #71210 (comment): Accepted both inline suggestions from this completed review: the checkout behavior documentation now has its own section and both checkout paths use the shared ref constant. The completion notification itself requires no code change. |
|
Regarding #71210 (comment): This status comment has since changed to review completion and requires no code change. I addressed the concrete shared-ref duplication, but deferred the optional larger checkout-helper extraction to keep this race fix narrowly scoped. |
There was a problem hiding this comment.
🟡 Changes recommended
A verified ref-name collision can still make checkout fail after a successful fetch.
1 open issue
What changed in this PR
Updates the shared PR checkout helper to use retained PR-head refs, addressing queued reviewer failures after merged source branches are deleted.
Changes:
- Fetches the retained PR head for same-repository events and resets the local source-named branch.
- Adds real-Git regressions covering deleted branches, stale refs and payload SHAs, and shallow/full clones.
- Documents checkout behavior and its separation from safe-output lifecycle checks.
| File | Description |
|---|---|
specs/compiler-threat-detection-spec.md |
Maps retained-head checkout to runtime ref-isolation requirements. |
specs/compiler-threat-detection-changelog.md |
Records version 1.0.67 and the checkout mapping audit. |
docs/src/content/docs/reference/checkout.md |
Explains checkout after source-branch deletion. |
actions/setup/js/checkout_pr_branch.test.cjs |
Updates command assertions and adds deleted-branch regressions. |
actions/setup/js/checkout_pr_branch.cjs |
Switches same-repository checkout to the retained PR ref. |
Review effort: Max
|
🎉 This pull request is included in a new release. Release: |
Summary
Fix the shared PR checkout race behind failures in Test Quality Sentinel, PR Code Quality Reviewer, Design Decision Gate, Ponytail Reviewer, and Matt Pocock Skills Reviewer. The same-repository
pull_requestfast path fetched the source branch by name; merging the PR and deleting that branch before the agent started made checkout fail.+refs/pull/<number>/head:refs/remotes/origin/pr-headfor same-repository PR events, matching the retained-head checkout already used for forks and other PR events. Reset the local source-named branch to the fetched PR head so stale local branches cannot select the wrong commit.The latest main already contained #70141's lifecycle classification, but that deliberately failed the checkout step and did not remove the branch-name fetch race. No duplicated reviewer workflow patches or generated lock changes are needed.
Validation
npm exec --no -- vitest run checkout_pr_branch.test.cjs checkout_pr_branch_z3.test.cjs: 201 passed; optional Z3 execution skipped because Z3 is unavailable.make fmt-cjsandmake lint-cjs: passed.PATH="/opt/homebrew/bin:$PATH" make agent-report-progress: passed, including build, TypeScript checks, JavaScript lint, and all 200 checkout tests. No Go files changed; impacted Go tests were correctly skipped. Two existing catch-property lint warnings remain unchanged.Representative observed failure: https://github.com/github/gh-aw/actions/runs/39829351695 (PR #71195 source branch deleted after merge).