Repository navigation
fix(dev-lead): release stale PR-branch worktrees before checkout (#470) - #494
Conversation
`checkout_pr_in_worktree` did `git worktree add --detach` then `gh pr checkout`, which aborts with fatal: '<branch>' is already used by worktree at '<stale path>' failed to run git: exit status 128 when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree (issue #470; run 27107230249 broke review-changes on PR #383, which then showed ci-failing and could never be reviewed). Before checking the PR branch out, the function now: 1. `git worktree prune` — drop registry entries whose dirs are gone; 2. look up the PR head branch and release whoever holds it — detach the main agent checkout in place (it can't be removed), force-remove any other worktree — then prune again. Adds 3 bats tests covering all three collision shapes (stale sibling worktree on disk, stale registry entry with dir deleted, agent checkout itself on the branch). Full dev-lead unit suite: 372/372; shellcheck clean. Note: prior dev-lead runs reported this "complete" but committed to the already-merged branch fix/dev-lead-pr-worktree-checkout and never opened a PR, so the fix never reached main. Implemented manually on a fresh branch. Closes #470 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request introduces recovery logic to clear stale Git worktree states left by crashed prior runs, preventing checkout collisions when checking out a PR branch. It also adds comprehensive unit tests to verify recovery under various scenarios. A review comment suggests normalizing the worktree and agent directory paths using git rev-parse --show-toplevel before comparing them, ensuring robust matching on systems with symlinks.
…view) Gemini review on PR #494: git worktree list emits resolved paths but PR_WORKTREE_AGENT_DIR comes from pwd and may contain symlinks (macOS, some CI runners). Resolve both with `pwd -P` before deciding detach-vs-remove, so the agent's own checkout isn't mis-routed to force-remove (a no-op that would leave the branch unreleased). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR addresses dev-lead failures caused by leftover/stale git worktrees on the same runner, where gh pr checkout can abort with “branch is already used by worktree …” (issue #470), preventing automated review/fix runs from proceeding.
Changes:
- Add pre-checkout cleanup logic to prune stale worktrees and release the PR head branch from any existing worktree holder (including detaching the agent checkout when necessary).
- Extend unit coverage with new Bats cases for the main collision shapes (stale sibling worktree, stale registry entry, agent checkout holding the branch).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| scripts/lib/pr-worktree.sh | Adds collision-recovery logic before creating/checking out the PR worktree. |
| tests/dev-lead/unit/test_pr_worktree.bats | Adds regression tests covering worktree collision recovery scenarios. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
… dirs Copilot review on PR #494: - PR_WORKTREE_AGENT_DIR is pwd, which can be a SUBDIRECTORY of the repo, while `git worktree list` reports the worktree root — so the equality check could miss the agent checkout (collision unfixed) or target the main worktree. Resolve via `git rev-parse --show-toplevel` (+ pwd -P) before comparing. Adds a subdirectory regression test. - The two new collision tests leaked their mktemp parent dirs; capture and rm -rf them after the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 4af1196c3982623a8fea18d528e475bb7ef98483
Review mode: triage-approved (single reviewer)
Summary
Targeted dev-lead fix for issue #470: clears stale worktree state before gh pr checkout so a crashed prior run on the same runner can't permanently break review-changes for a PR. Logic is well-scoped (prune → release branch holder → prune again), distinguishes the agent's own checkout (detach in place) from sibling worktrees (force-remove), and ships with focused regression tests for all four collision shapes (sibling, missing dir, agent self-hold, agent self-hold from subdir cwd).
Linked issue analysis
Closes #470 (checkout_pr_in_worktree aborts with fatal: '<branch>' is already used by worktree at '<stale path>'). The diff directly fixes the failure mode described in the issue — prior runs leaving the PR head branch claimed by a defunct worktree — and the new bats cases reproduce each crash shape before exercising the fix. PR-body context about the previously-merged-but-orphaned #449 attempt is consistent with what's on main (fix/dev-lead-pr-worktree-checkout not present in current source).
Findings
Prior bot findings — all resolved in head commit 4af1196:
- Gemini (high): direct string-equality against
PR_WORKTREE_AGENT_DIRwould misroute on symlinked runners → fixed: both sides now canonicalized viapwd -Pbefore comparison. - Copilot:
PR_WORKTREE_AGENT_DIRfrompwdmay be a subdir, breaking the equality check → fixed:git rev-parse --show-toplevelresolves to the repo root first; a dedicated subdirectory regression test was added. - Copilot (×2): two new tests leaked
mktemp -dparent dirs → fixed: both nowrm -rfthe captured parent at end.
Independent observations (non-blocking):
gh pr view ... 2>/dev/null || truesilently falls through to a no-op if the API call fails (rate limit, transient network); on collision the nextgh pr checkoutwill then surface the originalexit 128— acceptable degradation, matches today's behavior.- The cleanup runs under the EXIT trap chain established two lines earlier, so failures inside the loop won't leak the PR worktree.
- No secrets, no migrations, no GHA workflow changes, no new dependencies.
CI status
All required checks green: shellcheck, bats, unit-tests, CodeQL (actions+python), Agent Security Scan, AgentShield, Secret scan (gitleaks), SonarCloud (Quality Gate passed, 0 new issues), validate-agent-profiles, gh-aw-compile. Dependency-audit ecosystem matrices skipped as expected (no ecosystem files changed). PR is BEHIND main but MERGEABLE — base ref update only.
Reviewed automatically by the PR-review agent (single-reviewer mode: opus 4.7). Reply if you need a human review.
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a699132666
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # detaching releases the branch so the new worktree can claim it. | ||
| git -C "$_wt_path" checkout --detach --quiet 2>/dev/null || true | ||
| else | ||
| git worktree remove --force "$_wt_path" 2>/dev/null || true |
There was a problem hiding this comment.
Avoid deleting worktrees by branch name alone
On a persistent runner, if this repo has any other registered worktree checked out to a local branch whose name matches the PR's headRefName (common for fork PRs using names like main or patch-1), this removes that worktree even though it may be unrelated to a stale dev-lead temp checkout. I checked git worktree remove -h; --force forces removal even when the worktree is dirty or locked, so this can discard active/uncommitted work instead of only releasing stale /tmp/dev-lead-wt-* worktrees.
Useful? React with 👍 / 👎.
… (#494) * fix(dev-lead): release stale PR-branch worktrees before checkout (#470) `checkout_pr_in_worktree` did `git worktree add --detach` then `gh pr checkout`, which aborts with fatal: '<branch>' is already used by worktree at '<stale path>' failed to run git: exit status 128 when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree (issue #470; run 27107230249 broke review-changes on PR #383, which then showed ci-failing and could never be reviewed). Before checking the PR branch out, the function now: 1. `git worktree prune` — drop registry entries whose dirs are gone; 2. look up the PR head branch and release whoever holds it — detach the main agent checkout in place (it can't be removed), force-remove any other worktree — then prune again. Adds 3 bats tests covering all three collision shapes (stale sibling worktree on disk, stale registry entry with dir deleted, agent checkout itself on the branch). Full dev-lead unit suite: 372/372; shellcheck clean. Note: prior dev-lead runs reported this "complete" but committed to the already-merged branch fix/dev-lead-pr-worktree-checkout and never opened a PR, so the fix never reached main. Implemented manually on a fresh branch. Closes #470 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): canonicalize worktree paths before comparison (#470 review) Gemini review on PR #494: git worktree list emits resolved paths but PR_WORKTREE_AGENT_DIR comes from pwd and may contain symlinks (macOS, some CI runners). Resolve both with `pwd -P` before deciding detach-vs-remove, so the agent's own checkout isn't mis-routed to force-remove (a no-op that would leave the branch unreleased). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): compare worktree by repo top-level; clean up test temp dirs Copilot review on PR #494: - PR_WORKTREE_AGENT_DIR is pwd, which can be a SUBDIRECTORY of the repo, while `git worktree list` reports the worktree root — so the equality check could miss the agent checkout (collision unfixed) or target the main worktree. Resolve via `git rev-parse --show-toplevel` (+ pwd -P) before comparing. Adds a subdirectory regression test. - The two new collision tests leaked their mktemp parent dirs; capture and rm -rf them after the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (#494) * fix(dev-lead): release stale PR-branch worktrees before checkout (#470) `checkout_pr_in_worktree` did `git worktree add --detach` then `gh pr checkout`, which aborts with fatal: '<branch>' is already used by worktree at '<stale path>' failed to run git: exit status 128 when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree (issue #470; run 27107230249 broke review-changes on PR #383, which then showed ci-failing and could never be reviewed). Before checking the PR branch out, the function now: 1. `git worktree prune` — drop registry entries whose dirs are gone; 2. look up the PR head branch and release whoever holds it — detach the main agent checkout in place (it can't be removed), force-remove any other worktree — then prune again. Adds 3 bats tests covering all three collision shapes (stale sibling worktree on disk, stale registry entry with dir deleted, agent checkout itself on the branch). Full dev-lead unit suite: 372/372; shellcheck clean. Note: prior dev-lead runs reported this "complete" but committed to the already-merged branch fix/dev-lead-pr-worktree-checkout and never opened a PR, so the fix never reached main. Implemented manually on a fresh branch. Closes #470 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): canonicalize worktree paths before comparison (#470 review) Gemini review on PR #494: git worktree list emits resolved paths but PR_WORKTREE_AGENT_DIR comes from pwd and may contain symlinks (macOS, some CI runners). Resolve both with `pwd -P` before deciding detach-vs-remove, so the agent's own checkout isn't mis-routed to force-remove (a no-op that would leave the branch unreleased). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): compare worktree by repo top-level; clean up test temp dirs Copilot review on PR #494: - PR_WORKTREE_AGENT_DIR is pwd, which can be a SUBDIRECTORY of the repo, while `git worktree list` reports the worktree root — so the equality check could miss the agent checkout (collision unfixed) or target the main worktree. Resolve via `git rev-parse --show-toplevel` (+ pwd -P) before comparing. Adds a subdirectory regression test. - The two new collision tests leaked their mktemp parent dirs; capture and rm -rf them after the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (#494) * fix(dev-lead): release stale PR-branch worktrees before checkout (#470) `checkout_pr_in_worktree` did `git worktree add --detach` then `gh pr checkout`, which aborts with fatal: '<branch>' is already used by worktree at '<stale path>' failed to run git: exit status 128 when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree (issue #470; run 27107230249 broke review-changes on PR #383, which then showed ci-failing and could never be reviewed). Before checking the PR branch out, the function now: 1. `git worktree prune` — drop registry entries whose dirs are gone; 2. look up the PR head branch and release whoever holds it — detach the main agent checkout in place (it can't be removed), force-remove any other worktree — then prune again. Adds 3 bats tests covering all three collision shapes (stale sibling worktree on disk, stale registry entry with dir deleted, agent checkout itself on the branch). Full dev-lead unit suite: 372/372; shellcheck clean. Note: prior dev-lead runs reported this "complete" but committed to the already-merged branch fix/dev-lead-pr-worktree-checkout and never opened a PR, so the fix never reached main. Implemented manually on a fresh branch. Closes #470 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): canonicalize worktree paths before comparison (#470 review) Gemini review on PR #494: git worktree list emits resolved paths but PR_WORKTREE_AGENT_DIR comes from pwd and may contain symlinks (macOS, some CI runners). Resolve both with `pwd -P` before deciding detach-vs-remove, so the agent's own checkout isn't mis-routed to force-remove (a no-op that would leave the branch unreleased). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): compare worktree by repo top-level; clean up test temp dirs Copilot review on PR #494: - PR_WORKTREE_AGENT_DIR is pwd, which can be a SUBDIRECTORY of the repo, while `git worktree list` reports the worktree root — so the equality check could miss the agent checkout (collision unfixed) or target the main worktree. Resolve via `git rev-parse --show-toplevel` (+ pwd -P) before comparing. Adds a subdirectory regression test. - The two new collision tests leaked their mktemp parent dirs; capture and rm -rf them after the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (#494) * fix(dev-lead): release stale PR-branch worktrees before checkout (#470) `checkout_pr_in_worktree` did `git worktree add --detach` then `gh pr checkout`, which aborts with fatal: '<branch>' is already used by worktree at '<stale path>' failed to run git: exit status 128 when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree (issue #470; run 27107230249 broke review-changes on PR #383, which then showed ci-failing and could never be reviewed). Before checking the PR branch out, the function now: 1. `git worktree prune` — drop registry entries whose dirs are gone; 2. look up the PR head branch and release whoever holds it — detach the main agent checkout in place (it can't be removed), force-remove any other worktree — then prune again. Adds 3 bats tests covering all three collision shapes (stale sibling worktree on disk, stale registry entry with dir deleted, agent checkout itself on the branch). Full dev-lead unit suite: 372/372; shellcheck clean. Note: prior dev-lead runs reported this "complete" but committed to the already-merged branch fix/dev-lead-pr-worktree-checkout and never opened a PR, so the fix never reached main. Implemented manually on a fresh branch. Closes #470 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): canonicalize worktree paths before comparison (#470 review) Gemini review on PR #494: git worktree list emits resolved paths but PR_WORKTREE_AGENT_DIR comes from pwd and may contain symlinks (macOS, some CI runners). Resolve both with `pwd -P` before deciding detach-vs-remove, so the agent's own checkout isn't mis-routed to force-remove (a no-op that would leave the branch unreleased). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): compare worktree by repo top-level; clean up test temp dirs Copilot review on PR #494: - PR_WORKTREE_AGENT_DIR is pwd, which can be a SUBDIRECTORY of the repo, while `git worktree list` reports the worktree root — so the equality check could miss the agent checkout (collision unfixed) or target the main worktree. Resolve via `git rev-parse --show-toplevel` (+ pwd -P) before comparing. Adds a subdirectory regression test. - The two new collision tests leaked their mktemp parent dirs; capture and rm -rf them after the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (#494) * fix(dev-lead): release stale PR-branch worktrees before checkout (#470) `checkout_pr_in_worktree` did `git worktree add --detach` then `gh pr checkout`, which aborts with fatal: '<branch>' is already used by worktree at '<stale path>' failed to run git: exit status 128 when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree (issue #470; run 27107230249 broke review-changes on PR #383, which then showed ci-failing and could never be reviewed). Before checking the PR branch out, the function now: 1. `git worktree prune` — drop registry entries whose dirs are gone; 2. look up the PR head branch and release whoever holds it — detach the main agent checkout in place (it can't be removed), force-remove any other worktree — then prune again. Adds 3 bats tests covering all three collision shapes (stale sibling worktree on disk, stale registry entry with dir deleted, agent checkout itself on the branch). Full dev-lead unit suite: 372/372; shellcheck clean. Note: prior dev-lead runs reported this "complete" but committed to the already-merged branch fix/dev-lead-pr-worktree-checkout and never opened a PR, so the fix never reached main. Implemented manually on a fresh branch. Closes #470 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): canonicalize worktree paths before comparison (#470 review) Gemini review on PR #494: git worktree list emits resolved paths but PR_WORKTREE_AGENT_DIR comes from pwd and may contain symlinks (macOS, some CI runners). Resolve both with `pwd -P` before deciding detach-vs-remove, so the agent's own checkout isn't mis-routed to force-remove (a no-op that would leave the branch unreleased). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(dev-lead): compare worktree by repo top-level; clean up test temp dirs Copilot review on PR #494: - PR_WORKTREE_AGENT_DIR is pwd, which can be a SUBDIRECTORY of the repo, while `git worktree list` reports the worktree root — so the equality check could miss the agent checkout (collision unfixed) or target the main worktree. Resolve via `git rev-parse --show-toplevel` (+ pwd -P) before comparing. Adds a subdirectory regression test. - The two new collision tests leaked their mktemp parent dirs; capture and rm -rf them after the test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: donpetry-bot <{}+donpetry-bot@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>



Summary
Fixes #470 —
checkout_pr_in_worktreeaborts withfatal: '<branch>' is already used by worktree at '<stale path>'(exit status 128) when a crashed prior run on the same runner left the PR's head branch checked out in a now-defunct worktree. This brokereview-changeson #383 (run 27107230249), which then reportedci-failingand could never be reviewed by the cascade.Fix
Before
gh pr checkout,checkout_pr_in_worktreenow clears stale state:git worktree prune— drops registry entries whose directories are gone.git checkout --detachin place (it can't be removed);git worktree remove --force;git worktree pruneagain.Tests
tests/dev-lead/unit/test_pr_worktree.bats— 3 new cases for every collision shape:Local: full dev-lead unit suite 372/372,
shellcheck --severity=warning -xclean.Note
Prior autonomous dev-lead runs reported this issue "Implementation Complete" but committed to the already-merged branch
fix/dev-lead-pr-worktree-checkout(PR #449, closed) and never opened a PR — so the fix never reachedmain, and retriggers dedup-skipped on the staledev-lead/issue-470-*branches. Implemented manually on a fresh branch offmain.🤖 Generated with Claude Code