Repository navigation
fix(dev-lead): hold auto-merge while working a PR + push defensively (#452) - #453
Conversation
|
Warning Review limit reached
More reviews will be available in 59 minutes and 53 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (11)
📝 WalkthroughWalkthroughDev-lead agent workflow updated to require explicit per-thread replies describing specific code changes before resolving review threads, paired with a new auto-merge hold/restore system that prevents approval-triggered PR merges mid-run while safely handling defensive push and EXIT trap composition. ChangesReview thread reply-before-resolve workflow and auto-merge race prevention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a mechanism to hold GitHub auto-merge while dev-lead is running on a PR, preventing asynchronous merges from causing push failures. It adds a new auto-merge.sh library, integrates it into the CI and review fix scripts, and updates worktree checkout logic to chain EXIT traps rather than clobbering them. Additionally, concurrency tests and unit tests are updated or added. The review feedback highlights two valuable improvements: refining the trap extraction pattern in pr-worktree.sh to support environments where 0 is returned instead of EXIT, and making the auto-merge restoration check in auto-merge.sh more resilient to transient API failures.
donpetry-bot
left a comment
donpetry-bot
left a comment
There was a problem hiding this comment.
Automated review — APPROVED ✓
Risk: MEDIUM
Reviewed commit: 990cf9a46d1d46c2fdd9cffd2e6e128a0da5a57d
Review mode: triage-approved (single reviewer)
Summary
Fixes a real race where GitHub's async auto-merge can delete a PR's branch mid-run, failing dev-lead's push with cannot lock ref. The hold-then-restore design is defensively correct, with three independent guards in restore_auto_merge (a _AM_NEEDS_RESTORE flag that's only set after a successful disable, a PR-still-open check, and a current-auto-merge-state check) so it can never newly enable auto-merge on a PR that didn't have it. push_with_merge_guard correctly distinguishes the benign mid-run merge from genuine push failures by re-checking PR state.
Linked issue analysis
Fixes #452. The issue describes the exact ! [remote rejected] ... cannot lock ref failure mode observed in run 27078382804 (a review-changes run committed 1s after PR #449 auto-merged). The PR addresses this directly via three coordinated mechanisms — hold during work, restore on exit, and a push guard for the SIGKILL-escapes-traps edge case.
Findings
- scripts/lib/auto-merge.sh — Defensive design verified:
hold_auto_mergeonly sets_AM_NEEDS_RESTORE=1on a successful--disable-auto, sorestore_auto_mergecan never re-enable a PR that didn't have auto-merge to begin with. EXIT trap covers normal exit, error, and timeout; the existingenable-auto-merge-on-bot-approval intent is the documented backstop for SIGKILL. - scripts/lib/pr-worktree.sh — Trap-chaining captures any pre-existing EXIT trap via
trap -p EXIT | sed, then concatenates with;. The sed pattern only handles bare function names (no embedded single quotes), but the inline comment correctly notes this matches actual dev-lead usage (restore_auto_merge). Cleanup runs before the chained prior trap, which is the right order (worktree teardown before auto-merge restore). - dev-lead-fix-ci.sh / dev-lead-fix-reviews.sh —
trap restore_auto_merge EXITis registered beforehold_auto_merge, so even a failure insidehold_auto_mergewould still trip the trap (which then no-ops because_AM_NEEDS_RESTORE=0). Correct ordering. Both scripts routegit pushthroughpush_with_merge_guard || exit 1— notepush_with_merge_guardcallsexit 0(notreturn 0) on the benign-merge path, intentionally short-circuiting the rest ofmain()since there's nothing left to summarize once the PR is merged. - tests/dev-lead/unit/test_auto_merge.bats — 11 new test cases covering hold/restore/push-guard happy paths, dry-run, the no-op-when-already-off case, the don't-restore-on-closed-PR case, and the don't-double-restore case. Gh and git stubs are isolated via
STUB_BIN_DIRonPATH. - tests/dev-lead/unit/test_pr_worktree.bats — New trap-chaining test asserts both
cleanup_pr_worktreeand the priorecho PRIORsurvive in the final EXIT trap. Correctly locks the regression. - tests/dev-lead/integration/test_concurrency_config.sh — Was stale (asserted the pre-#450 repo-wide group, failing against main). Rewritten to assert per-PR/per-issue lanes +
cancel-in-progress: falseon bothdev-lead.ymlanddev-lead-reusable.yml. Good catch on the reusable workflow coverage.
No blocking findings. Suggestion (non-blocking, future): if more callers start registering EXIT traps with single quotes or complex syntax, the sed-based trap capture in pr-worktree.sh may need to be revisited — the comment already calls this out.
CI status
All 24 required checks passing: CodeQL, AgentShield, SonarCloud (Quality Gate passed: 0 new issues, 0 hotspots), gitleaks, dependency-audit, shellcheck, bats unit tests, validate-agent-profiles, gh-aw-compile, agent-security-scan. CodeRabbit was rate-limited (organizational quota) — not a finding.
Reviewed automatically by the PR-review agent (claude-opus-4-7). Reply if you need a human review.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
2b335ec
There was a problem hiding this comment.
Pull request overview
This PR addresses a failure mode where GitHub auto-merge can merge and delete a PR branch while dev-lead is mid-run, causing subsequent git push to fail. It introduces an auto-merge “hold + restore” mechanism and a defensive push guard so mid-run merges don’t produce red CI failures, and it updates tests to lock in the intended concurrency serialization model.
Changes:
- Add
scripts/lib/auto-merge.shto (1) disable auto-merge during PR-modifying runs, (2) restore it via anEXITtrap when appropriate, and (3) treat “PR merged/closed mid-run” push failures as a clean no-op. - Update
dev-lead-fix-reviews.shanddev-lead-fix-ci.shto register the restore trap, hold auto-merge before worktree checkout, and push via the merge-guard helper. - Update worktree trap handling and refresh unit/integration tests to cover trap chaining and the current per-PR/per-issue concurrency lanes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
tests/dev-lead/unit/test_pr_worktree.bats |
Adds coverage ensuring checkout_pr_in_worktree doesn’t clobber an existing EXIT trap (needed for auto-merge restore). |
tests/dev-lead/unit/test_auto_merge.bats |
New unit tests covering hold/restore behavior and the defensive push guard. |
tests/dev-lead/integration/test_concurrency_config.sh |
Updates the integration assertion to match the current per-PR/per-issue concurrency model across both inline and reusable workflows. |
scripts/lib/pr-worktree.sh |
Chains worktree cleanup onto any pre-existing EXIT trap instead of overwriting it. |
scripts/lib/auto-merge.sh |
New library implementing hold/restore auto-merge and push guarding for mid-run merges. |
scripts/dev-lead-fix-reviews.sh |
Hooks in auto-merge hold/restore and uses guarded push to avoid failing if the PR merges mid-run. |
scripts/dev-lead-fix-ci.sh |
Hooks in auto-merge hold/restore and uses guarded push to avoid failing if the PR merges mid-run. |
Dev-Lead — review-changes (applied)Changes committed and pushed. |
Dev-Lead — fix-bot-comment (no-changes)Agent reasoning |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c202e77659
ℹ️ 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".
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
…query too (#452) The fix-reviews OPEN_THREADS_JSON build was updated to select author.__typename (GraphQL omits the [bot] suffix from bot logins, so bots are identified by __typename == "Bot"), but the review-changes build still selected only author.login. Without __typename the review-changes engine cannot reliably tell bots from humans, so its "resolve only human threads, never bots" rule could misclassify coderabbitai / chatgpt-codex-connector / etc. as human. - Add __typename to the review-changes OPEN_THREADS_JSON query. - review-changes.md now identifies humans/bots by author.__typename (User/Bot), with an explicit note not to rely on a [bot] login suffix. - test_prompt_coverage.sh now asserts EVERY OPEN_THREADS_JSON build selects author.__typename, so a future build can't silently omit it. Full unit suite + prompt-coverage + concurrency green; shellcheck clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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: 78ad06457f
ℹ️ 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".
|
@don-petry assigned me as reviewer — starting a fresh review now. Results will appear in a few minutes. |
Dev-Lead — fix-reviews (applied)Changes committed and pushed. |
|
Note @don-petry I received your request but all AI engines are currently rate-limited. I'll retry automatically once the rate limit clears. |
Dev-Lead — rate-limited (intent: review-changes)PR: #453 |
|
Dev-Lead — review-changes (no-changes)No changes were needed for this PR. |
What
Fixes #452.
Holds GitHub auto-merge OFF while dev-lead is working a PR, and pushes defensively so a PR that merges mid-run doesn't fail the job. Also fixes the stale concurrency test so same-PR serialization is locked.
Why
dev-lead enables auto-merge (squash) by default (#447). GitHub's auto-merge is asynchronous and not gated by the workflow concurrency lane, so a review approval landing while the agent is mid-edit can satisfy branch protection and make GitHub merge + delete the branch out from under the run — which then fails on push:
Observed in run 27078382804: a
review-changesrun committed 1 second after PR #449 auto-merged. Per-PR serialization (#450) stops two dev-lead runs fighting, but it can't stop GitHub's async merge because the merge isn't a dev-lead run.How
scripts/lib/auto-merge.sh(new):hold_auto_merge— disables auto-merge at the start of a PR-modifying run (only if it was on), so a mid-flight approval can't merge the branch.restore_auto_merge— registered as anEXITtrap; re-enables auto-merge on any exit (success/error/timeout), but only if this run disabled it and the PR is still open. Never newly enables auto-merge on a PR that didn't have it.push_with_merge_guard— ifgit pushis rejected because the PR was merged/closed (branch deleted) mid-run, logs::notice::and exits 0 cleanly; a genuine push failure still aborts with exit 1.dev-lead-fix-reviews.sh/dev-lead-fix-ci.sh— register the restore trap +hold_auto_mergebefore checkout, and push viapush_with_merge_guard.scripts/lib/pr-worktree.sh—checkout_pr_in_worktreenow chains its cleanup onto any pre-existingEXITtrap instead of clobbering it, so the auto-merge restore survives.tests/dev-lead/integration/test_concurrency_config.sh— was stale: it still asserted the pre-#450 repo-widedev-leadgroup and failed againstmain. Rewritten to assert the per-PR/per-issue serialization lanes +cancel-in-progress: falseon both the inline and reusable workflows — this is the same-PR serialization regression lock.Why hold-then-restore (not just serialize)
Serializing dev-lead runs (#450) ensures two runs don't fight, but GitHub's auto-merge fires independently. Holding auto-merge off for the duration of a run is what actually prevents the mid-flight merge. The
EXITtrap restores it however the run ends, so we don't trade a loud failure for a silently-stuck PR. Backstop for the rare hard-kill (SIGKILL escapes traps): the existingenable-auto-merge-on-bot-approval intent re-enables on the next approval.Testing
bats tests/dev-lead/unit/— 284 pass (newtest_auto_merge.batscovering hold/restore/push-guard; trap-chaining case added totest_pr_worktree.bats).test_concurrency_config.shandtest_prompt_coverage.shintegration tests pass (concurrency test now green — it was red on main).shellcheck -S warningclean on all four scripts.🤖 Generated with Claude Code
Added in this PR — reply-with-specifics, then resolve (also #452)
Commit
a301873expands the scope to fix dev-lead's review-thread handling, which is what left a Copilot thread on this PR stuck open (blocking pr-review):fix-reviews.md,review-changes.md,fix-bot-comment.md) now tell the engine, for every thread it fixes, to post a reply naming the specific file/function changed and how it addresses the concern (addPullRequestReviewThreadReply, body passed as a GraphQL variable) — then resolve. No more silent resolves.fix-reviews.mdpreviously only resolved${TRIGGERING_REVIEWER}'s threads, so a bot thread fixed during another reviewer's run stayed open forever. It now resolves every bot thread it actually fixed (human threads remain gated to the trigger and are replied-to but left for the human).test_prompt_coverage.shnow asserts each review prompt instructs reply-with-specifics + resolve.Tests: 292 unit + prompt-coverage + concurrency all pass.
Summary by CodeRabbit
New Features
Bug Fixes
Tests