fixes #31 Multi-conflict chains leave orphaned merge-forward branches#32
Conversation
When a merge chain hit conflicts at multiple points, each conflict resolution PR created merge-forward branches under its own PR number. Cleanup only searched for the latest PR's branches, leaving earlier ones orphaned and branch-here un-advanced. Added extractOriginalPRNumber to branch-name-utils.js so both AutoMerger and BranchMaintainer trace back to the original PR number. createMergeForwardBranchName and createMergeConflictsBranchName now use this, keeping the entire chain discoverable under one PR number. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai full review |
|
Hmm, Coderabbit doesn't seem to be working. I tried to go configure it, but the config screen looks messed up. I'm guessing it's an issue on their end so I'll just merge this in. |
✅ Actions performedFull review triggered. |
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
|
It looks like CodeRabbit was just slower to respond than normal |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/automerger.js`:
- Around line 118-124: The getter originalPRNumber uses
this.pullRequest.head?.ref which still throws if this.pullRequest is undefined;
update the reference to this.pullRequest?.head?.ref to fully guard access (in
the get originalPRNumber method that calls extractOriginalPRNumber) so that
extractOriginalPRNumber receives undefined safely when pullRequest is omitted
from the constructor.
In `@test/branch-name-utils-test.js`:
- Around line 93-101: The test "prefers base over head when both present"
currently uses baseRef and headRef that both resolve to PR 100 so it doesn't
verify priority; update the headRef (or baseRef) so the two refs encode
different PR numbers (e.g., make headRef resolve to 200 while leaving baseRef as
100) and assert extractOriginalPRNumber(...) returns the baseRef's PR ('100') to
prove baseRef wins; ensure the test still calls extractOriginalPRNumber with the
same keys (baseRef, headRef, prNumber) so you only change the ref string
content.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/automerger.jssrc/branch-maintainer.jssrc/branch-name-utils.jstest/auto-merge-action-test.jstest/branch-name-utils-test.js
| get originalPRNumber() { | ||
| return extractOriginalPRNumber({ | ||
| baseRef: this.baseBranch, | ||
| headRef: this.pullRequest.head?.ref, | ||
| prNumber: this.prNumber | ||
| }) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check TestAutoMerger and pullRequest initialization
rg -n "class TestAutoMerger" test/ -A 20 | head -50Repository: SpiderStrategies/merge-bot
Length of output: 2781
🏁 Script executed:
# Find where pullRequest is initialized in test helpers
rg -n "pullRequest" test/test-helpers.js -B 3 -A 3Repository: SpiderStrategies/merge-bot
Length of output: 581
🏁 Script executed:
# Check tests calling originalPRNumber
rg -n "originalPRNumber" test/ -B 5 -A 2Repository: SpiderStrategies/merge-bot
Length of output: 1720
🏁 Script executed:
# Inspect the actual constructor in src/automerger.js
head -120 src/automerger.js | tail -60Repository: SpiderStrategies/merge-bot
Length of output: 1844
🏁 Script executed:
# Get complete TestAutoMerger constructor
sed -n '167,210p' test/test-helpers.jsRepository: SpiderStrategies/merge-bot
Length of output: 1377
🏁 Script executed:
# Get AutoMerger constructor to see how pullRequest is assigned
sed -n '1,100p' src/automerger.js | grep -A 40 "constructor"Repository: SpiderStrategies/merge-bot
Length of output: 960
🏁 Script executed:
# Check if AutoMerger is ever instantiated directly in production code without pullRequest
rg -n "new AutoMerger" src/ --type jsRepository: SpiderStrategies/merge-bot
Length of output: 126
🏁 Script executed:
# Check how AutoMerger is instantiated in the action
rg -n "new AutoMerger\|extends AutoMerger" src/ test/ --type jsRepository: SpiderStrategies/merge-bot
Length of output: 52
🏁 Script executed:
# Check how AutoMerger is instantiated in merge-bot.js
sed -n '20,40p' src/merge-bot.jsRepository: SpiderStrategies/merge-bot
Length of output: 456
🏁 Script executed:
# Verify what object is passed to AutoMerger constructor
sed -n '25,50p' src/merge-bot.jsRepository: SpiderStrategies/merge-bot
Length of output: 566
this.pullRequest.head?.ref lacks protection if pullRequest is undefined.
The optional chaining on .head?.ref is insufficient; if this.pullRequest itself is undefined (e.g., when pullRequest is omitted from the constructor), it throws a TypeError before the chain evaluates. Use this.pullRequest?.head?.ref for complete protection.
Proposed fix
get originalPRNumber() {
return extractOriginalPRNumber({
baseRef: this.baseBranch,
- headRef: this.pullRequest.head?.ref,
+ headRef: this.pullRequest?.head?.ref,
prNumber: this.prNumber
})
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| get originalPRNumber() { | |
| return extractOriginalPRNumber({ | |
| baseRef: this.baseBranch, | |
| headRef: this.pullRequest.head?.ref, | |
| prNumber: this.prNumber | |
| }) | |
| } | |
| get originalPRNumber() { | |
| return extractOriginalPRNumber({ | |
| baseRef: this.baseBranch, | |
| headRef: this.pullRequest?.head?.ref, | |
| prNumber: this.prNumber | |
| }) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/automerger.js` around lines 118 - 124, The getter originalPRNumber uses
this.pullRequest.head?.ref which still throws if this.pullRequest is undefined;
update the reference to this.pullRequest?.head?.ref to fully guard access (in
the get originalPRNumber method that calls extractOriginalPRNumber) so that
extractOriginalPRNumber receives undefined safely when pullRequest is omitted
from the constructor.
| t.test('prefers base over head when both present', | ||
| async t => { | ||
| t.equal(extractOriginalPRNumber({ | ||
| baseRef: 'merge-forward-pr-100-main', | ||
| headRef: 'merge-conflicts-200-pr-100' + | ||
| '-release-5.8.0-to-main', | ||
| prNumber: 999 | ||
| }), '100') | ||
| }) |
There was a problem hiding this comment.
"prefers base over head" test doesn't actually verify priority.
Both baseRef (merge-forward-pr-100-main) and headRef (merge-conflicts-200-pr-100-release-5.8.0-to-main) resolve to the same PR number ('100'), so the assertion passes regardless of evaluation order. To truly verify that baseRef wins when both are present, the two refs must encode different PR numbers.
♻️ Proposed fix
t.test('prefers base over head when both present',
async t => {
t.equal(extractOriginalPRNumber({
baseRef: 'merge-forward-pr-100-main',
- headRef: 'merge-conflicts-200-pr-100' +
- '-release-5.8.0-to-main',
+ headRef: 'merge-conflicts-200-pr-999' +
+ '-release-5.8.0-to-main',
prNumber: 999
- }), '100')
+ }), '100',
+ 'baseRef-derived number (100) should win over headRef-derived number (999)')
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| t.test('prefers base over head when both present', | |
| async t => { | |
| t.equal(extractOriginalPRNumber({ | |
| baseRef: 'merge-forward-pr-100-main', | |
| headRef: 'merge-conflicts-200-pr-100' + | |
| '-release-5.8.0-to-main', | |
| prNumber: 999 | |
| }), '100') | |
| }) | |
| t.test('prefers base over head when both present', | |
| async t => { | |
| t.equal(extractOriginalPRNumber({ | |
| baseRef: 'merge-forward-pr-100-main', | |
| headRef: 'merge-conflicts-200-pr-999' + | |
| '-release-5.8.0-to-main', | |
| prNumber: 999 | |
| }), '100', | |
| 'baseRef-derived number (100) should win over headRef-derived number (999)') | |
| }) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/branch-name-utils-test.js` around lines 93 - 101, The test "prefers base
over head when both present" currently uses baseRef and headRef that both
resolve to PR 100 so it doesn't verify priority; update the headRef (or baseRef)
so the two refs encode different PR numbers (e.g., make headRef resolve to 200
while leaving baseRef as 100) and assert extractOriginalPRNumber(...) returns
the baseRef's PR ('100') to prove baseRef wins; ensure the test still calls
extractOriginalPRNumber with the same keys (baseRef, headRef, prNumber) so you
only change the ref string content.
Add optional chaining on this.pullRequest in originalPRNumber getter to guard against undefined. Fix "prefers base over head" test to use different PR numbers so it actually verifies evaluation priority. Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Closes #31
When a merge chain hit conflicts at multiple points (e.g., both
release-5.8.0andmain), each conflict resolution PR created merge-forward branches under its own PR number. Cleanup only searched for the latest PR's branches, leaving earlier ones orphaned andbranch-hereun-advanced.Changes
extractOriginalPRNumbertobranch-name-utils.js— shared function that traces back to the original PR number from merge-forward base refs or merge-conflicts head refsAutoMerger.originalPRNumbergetter delegates to the shared functioncreateMergeForwardBranchNameandcreateMergeConflictsBranchNameuseoriginalPRNumber, keeping the entire chain under one PR numberBranchMaintainer.determineOriginalPRNumber()(DRY)updateTargetBranchessimplified to use the same getterMade with Cursor