fix(core): reuse same-name branch group instead of colliding on mission triage#1348
Merged
Conversation
branch_groups.branchName is globally UNIQUE, but ensureBranchGroupForSource only looked up an existing group by (sourceType, sourceId). When a second mission's shared-branch triage resolved to a base branch (e.g. "main") that another mission already owned a group for, createBranchGroup threw "UNIQUE constraint failed: branch_groups.branchName". That error escaped triageFeature and was swallowed by both callers (validation-failure auto-triage and the reconcile sweep), leaving the mission's "defined" features — including generated fix features — permanently un-triaged. ensureBranchGroupForSource now reuses an existing open group for the same branch name before attempting to create one, matching the established getBranchGroupByBranchName(...) ?? ensureBranchGroupForSource(...) idiom. Confirmed by reproducing against a snapshot of the affected mission DB: triageFeature threw the UNIQUE error before, succeeds after. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughensureBranchGroupForSource now checks for an existing open branch group by the resolved ChangesBranch Group Reuse Logic
🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/store.ts (1)
4388-4403:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake branch-group reuse/create atomic to avoid a remaining race.
This still has a check-then-create TOCTOU window: two concurrent callers can both miss
existingByBranchand one will still hitUNIQUE constraint failed: branch_groups.branchNameon create. That can reintroduce the same stalled-triage failure mode under concurrency.Suggested fix (retry on unique-conflict and reuse existing group)
ensureBranchGroupForSource( sourceType: BranchGroup["sourceType"], sourceId: string, init: Omit<BranchGroupCreateInput, "sourceType" | "sourceId">, ): BranchGroup { const existing = this.getBranchGroupBySource(sourceType, sourceId); if (existing) { return existing; } const existingByBranch = this.getBranchGroupByBranchName(init.branchName); if (existingByBranch) { return existingByBranch; } - return this.createBranchGroup({ - sourceType, - sourceId, - ...init, - }); + try { + return this.createBranchGroup({ + sourceType, + sourceId, + ...init, + }); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + if (/UNIQUE constraint failed:\s*branch_groups\.branchName/i.test(message)) { + const raced = this.getBranchGroupByBranchName(init.branchName); + if (raced) return raced; + } + throw error; + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/store.ts` around lines 4388 - 4403, The current get-or-create flow around getBranchGroupByBranchName and createBranchGroup has a TOCTOU race: make the operation atomic by catching a UNIQUE constraint error from createBranchGroup (the "UNIQUE constraint failed: branch_groups.branchName" case) and, on that specific error, re-query getBranchGroupByBranchName and return the existing group instead of failing; for other errors rethrow them. Ensure this change is implemented where you call createBranchGroup (the code that currently does: if (existingByBranch) return existingByBranch; return this.createBranchGroup({...})) so concurrent callers will tolerate a duplicate-create race and reuse the already-created branch group.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/__tests__/branch-group-store.test.ts`:
- Around line 70-96: Add a "## Surface Enumeration" comment block immediately
above the failing test case that lists all surfaces verified by the regression
(e.g., mission, new-task, planning) so the spec documents the tested surfaces;
reference the tested helper functions ensureBranchGroupForSource,
createBranchGroup, and listBranchGroups in the comment to make clear which code
paths are covered and that the invariant (reuse of an existing branch group for
branchName "main") is asserted across those surfaces.
---
Outside diff comments:
In `@packages/core/src/store.ts`:
- Around line 4388-4403: The current get-or-create flow around
getBranchGroupByBranchName and createBranchGroup has a TOCTOU race: make the
operation atomic by catching a UNIQUE constraint error from createBranchGroup
(the "UNIQUE constraint failed: branch_groups.branchName" case) and, on that
specific error, re-query getBranchGroupByBranchName and return the existing
group instead of failing; for other errors rethrow them. Ensure this change is
implemented where you call createBranchGroup (the code that currently does: if
(existingByBranch) return existingByBranch; return
this.createBranchGroup({...})) so concurrent callers will tolerate a
duplicate-create race and reuse the already-created branch group.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8dc18d6c-f6f9-4256-94a4-a6c1687a82c8
📒 Files selected for processing (3)
.changeset/fix-branch-group-name-collision-triage.mdpackages/core/src/__tests__/branch-group-store.test.tspackages/core/src/store.ts
Comment on lines
+70
to
+96
| it("reuses an existing open group with the same branchName across sources instead of throwing", () => { | ||
| // Regression: branch_groups.branchName is globally UNIQUE. When one mission | ||
| // already owns an open group for a shared base branch, a second source whose | ||
| // triage resolves to the same branch must reuse that group rather than crash | ||
| // on the UNIQUE constraint. (Mission triage discards the result and only needs | ||
| // it not to throw; a thrown error there silently strands "defined" features.) | ||
| const owner = store.createBranchGroup({ sourceType: "mission", sourceId: "M-OWNER", branchName: "main" }); | ||
|
|
||
| let reusedByMission!: ReturnType<typeof store.ensureBranchGroupForSource>; | ||
| expect(() => { | ||
| reusedByMission = store.ensureBranchGroupForSource("mission", "M-OTHER", { | ||
| branchName: "main", | ||
| autoMerge: true, | ||
| }); | ||
| }).not.toThrow(); | ||
| expect(reusedByMission.id).toBe(owner.id); | ||
|
|
||
| // Invariant holds across the other source types that share this helper. | ||
| const reusedByNewTask = store.ensureBranchGroupForSource("new-task", "shared/main", { branchName: "main" }); | ||
| expect(reusedByNewTask.id).toBe(owner.id); | ||
|
|
||
| const reusedByPlanning = store.ensureBranchGroupForSource("planning", "PS-main", { branchName: "main" }); | ||
| expect(reusedByPlanning.id).toBe(owner.id); | ||
|
|
||
| // No duplicate rows were created for the shared branch. | ||
| expect(store.listBranchGroups().filter((g) => g.branchName === "main")).toHaveLength(1); | ||
| }); |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a ## Surface Enumeration section to document tested surfaces.
The test correctly verifies the fix across all three source types (mission, new-task, planning), but the coding guidelines require regression tests to include a formal ## Surface Enumeration section documenting all known surfaces before filing or closing the fix.
📋 Suggested Surface Enumeration section
Add this comment block above the test case:
+ /**
+ * ## Surface Enumeration
+ *
+ * This regression test verifies `ensureBranchGroupForSource` reuses existing
+ * open branch groups across all source types that can trigger the collision:
+ *
+ * - **mission**: Primary failure path during `triageFeature` when multiple missions
+ * share a base branch (e.g., `main`). Tested at line 80-85.
+ * - **new-task**: Shared branch groups created from new task entry point.
+ * Tested at line 88-89.
+ * - **planning**: Planning-sourced branch groups. Tested at line 91-92.
+ *
+ * Out of scope: Closed/finalized groups with same branch name (PR explicitly
+ * notes these remain out of scope; `getBranchGroupByBranchName` ignores them).
+ */
it("reuses an existing open group with the same branchName across sources instead of throwing", () => {As per coding guidelines: "When fixing a bug, the regression test must assert the general invariant across ALL known surfaces — not only the single reported reproduction. The spec must include a ## Surface Enumeration section."
📝 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.
Suggested change
| it("reuses an existing open group with the same branchName across sources instead of throwing", () => { | |
| // Regression: branch_groups.branchName is globally UNIQUE. When one mission | |
| // already owns an open group for a shared base branch, a second source whose | |
| // triage resolves to the same branch must reuse that group rather than crash | |
| // on the UNIQUE constraint. (Mission triage discards the result and only needs | |
| // it not to throw; a thrown error there silently strands "defined" features.) | |
| const owner = store.createBranchGroup({ sourceType: "mission", sourceId: "M-OWNER", branchName: "main" }); | |
| let reusedByMission!: ReturnType<typeof store.ensureBranchGroupForSource>; | |
| expect(() => { | |
| reusedByMission = store.ensureBranchGroupForSource("mission", "M-OTHER", { | |
| branchName: "main", | |
| autoMerge: true, | |
| }); | |
| }).not.toThrow(); | |
| expect(reusedByMission.id).toBe(owner.id); | |
| // Invariant holds across the other source types that share this helper. | |
| const reusedByNewTask = store.ensureBranchGroupForSource("new-task", "shared/main", { branchName: "main" }); | |
| expect(reusedByNewTask.id).toBe(owner.id); | |
| const reusedByPlanning = store.ensureBranchGroupForSource("planning", "PS-main", { branchName: "main" }); | |
| expect(reusedByPlanning.id).toBe(owner.id); | |
| // No duplicate rows were created for the shared branch. | |
| expect(store.listBranchGroups().filter((g) => g.branchName === "main")).toHaveLength(1); | |
| }); | |
| /** | |
| * ## Surface Enumeration | |
| * | |
| * This regression test verifies `ensureBranchGroupForSource` reuses existing | |
| * open branch groups across all source types that can trigger the collision: | |
| * | |
| * - **mission**: Primary failure path during `triageFeature` when multiple missions | |
| * share a base branch (e.g., `main`). Tested at line 80-85. | |
| * - **new-task**: Shared branch groups created from new task entry point. | |
| * Tested at line 88-89. | |
| * - **planning**: Planning-sourced branch groups. Tested at line 91-92. | |
| * | |
| * Out of scope: Closed/finalized groups with same branch name (PR explicitly | |
| * notes these remain out of scope; `getBranchGroupByBranchName` ignores them). | |
| */ | |
| it("reuses an existing open group with the same branchName across sources instead of throwing", () => { | |
| // Regression: branch_groups.branchName is globally UNIQUE. When one mission | |
| // already owns an open group for a shared base branch, a second source whose | |
| // triage resolves to the same branch must reuse that group rather than crash | |
| // on the UNIQUE constraint. (Mission triage discards the result and only needs | |
| // it not to throw; a thrown error there silently strands "defined" features.) | |
| const owner = store.createBranchGroup({ sourceType: "mission", sourceId: "M-OWNER", branchName: "main" }); | |
| let reusedByMission!: ReturnType<typeof store.ensureBranchGroupForSource>; | |
| expect(() => { | |
| reusedByMission = store.ensureBranchGroupForSource("mission", "M-OTHER", { | |
| branchName: "main", | |
| autoMerge: true, | |
| }); | |
| }).not.toThrow(); | |
| expect(reusedByMission.id).toBe(owner.id); | |
| // Invariant holds across the other source types that share this helper. | |
| const reusedByNewTask = store.ensureBranchGroupForSource("new-task", "shared/main", { branchName: "main" }); | |
| expect(reusedByNewTask.id).toBe(owner.id); | |
| const reusedByPlanning = store.ensureBranchGroupForSource("planning", "PS-main", { branchName: "main" }); | |
| expect(reusedByPlanning.id).toBe(owner.id); | |
| // No duplicate rows were created for the shared branch. | |
| expect(store.listBranchGroups().filter((g) => g.branchName === "main")).toHaveLength(1); | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/__tests__/branch-group-store.test.ts` around lines 70 - 96,
Add a "## Surface Enumeration" comment block immediately above the failing test
case that lists all surfaces verified by the regression (e.g., mission,
new-task, planning) so the spec documents the tested surfaces; reference the
tested helper functions ensureBranchGroupForSource, createBranchGroup, and
listBranchGroups in the comment to make clear which code paths are covered and
that the invariant (reuse of an existing branch group for branchName "main") is
asserted across those surfaces.
Document the UNIQUE(branch_groups.branchName) collision that silently stranded mission triage, in docs/solutions/logic-errors/, cross-linked to the sibling PR #1345 mission-stall learning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
What
Fixes a mission stall where
definedfeatures — including auto-generated Fix features — are never triaged into tasks, so the mission can't progress. Distinct from #1345 (that fixed a slice-completion-gate wedge; this fixes a branch-group collision in triage).Why
branch_groups.branchNameis globally UNIQUE, butensureBranchGroupForSourceonly looked up an existing group by(sourceType, sourceId):When a mission has no
branchStrategy, triage defaults to shared assignment with the base branch falling through tosettings.defaultBranch(main). If another mission already owns a branch group formain,createBranchGroupthrowsUNIQUE constraint failed: branch_groups.branchName. That error escapestriageFeatureand is swallowed as a log line by both callers — the validation-failure auto-triage (mission-execution-loop.ts) and the startup/maintenance reconcile sweep (scheduler.ts) — so the feature silently staysdefinedforever.Observed live: the "Goals as First-Class Strategy" mission had 7 fix features + several plain
definedfeatures stuck across two slices, while a different mission already owned themainbranch group.How
ensureBranchGroupForSourcenow reuses an existing open group for the same branch name before creating one — matching the idiom already used inregister-task-workflow-routes.ts(getBranchGroupByBranchName(...) ?? ensureBranchGroupForSource(...)). SincebranchNameis unique, one open group per branch is the intended model. The low-levelcreateBranchGroupstill enforces uniqueness (unchanged).Verification
VACUUM INTOsnapshot of the affected mission DB:triageFeatureon a stuck fix feature threwUNIQUE constraint failedbefore the change and returnsstatus: triagedwith a new task after it.mission,new-task,planning) and that no duplicate group rows are created.branch-group-store+mission-store+dbsuites green (388 tests).Scope / follow-ups
branchStrategydefaulting to shared-on-mainis itself questionable (the affected mission's own tasks use per-taskfusion/fn-XXXXbranches). And the originals in that mission fail validation on bundled assertions and exhaust the retry budget (related to the FN-5902 effort) — neither is a code bug this PR addresses.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation