feat: branch-group single managed PR flow (planning + missions)#1357
Merged
Conversation
Planning and mission entry points discarded the BranchGroup returned by ensureBranchGroupForSource and stamped a synthetic planning:/mission: string that never resolved against getBranchGroup, breaking member enumeration. Capture and stamp the real BG- id; stop setTaskBranchGroup hardcoding assignmentMode; add a removable legacy read-side shim. Export TaskBranchContext.
Route and coordinator disagreed on landed/complete: the route required mergeConfirmed + matching mergeTargetBranch, the coordinator accepted bare column===done/in-review and never checked the branch. Extract canonical isBranchGroupMemberLanded/isBranchGroupComplete in @fusion/core (stricter route semantics win — load-bearing for merge-target safety) and consume from both sides. Tightens promotion gating to fire only when all members are merge-confirmed onto the group branch.
Audit of all shared-member merge + self-healing finalize paths: routing, merger finalize-success, and the 6 self-healing recovery paths were already group-branch-safe (FN-5846). Found a residual of the 2026-05-23 lost-work incident bug #2: already-merged-detector's ancestry strategy used bare git log --grep first-hit, and the ownership regex made the conventional scope optional (bare 'feat:' matched). Anchor attribution on trailers or task-scoped subject; scan candidates instead of accepting the first grep hit. Adds real-git characterization tests.
The dashboard promote route called engine.promoteBranchGroup(groupId) as a method that never existed — only a standalone coordinator function did — so the route was dead, masked by a vi.fn mock in the test. Add the real method on ProjectEngine delegating to the coordinator (resolving store/cwd/settings like attemptBranchGroupPromotion), and de-mock the test so it now fails if the method goes missing. No PR-creation behavior yet (U5).
…n (U5) Group promotion in PR mode previously flipped prState to 'open' without ever calling GitHub — prNumber/prUrl were never populated. Add an injected CreateGroupPrFn (mirrors the processPullRequestMerge seam, no engine→dashboard import): coordinator creates-or-reuses exactly one PR per group, persists prNumber/prUrl/prState, and leaves state untouched on GitHub failure so re-promotion retries. Idempotent via persisted prNumber + getBranchGroupByBranchName. Wired at all three CLI engine-construction sites (daemon/dashboard/serve).
…ycle (U6) Push the single group PR's body (member checklist, x/N landed) on each member landing via an injected SyncGroupPrFn — new updatePr/closePr GitHubClient helpers (gh CLI + API parity); refreshPrInBackground is task-scoped/wrong direction and intentionally not reused. Sync failures are non-fatal+retryable; out-of-band closed/merged PRs reconcile prState instead of erroring. New POST /branch-groups/:id/abandon closes the PR best-effort and marks the group abandoned. Also fixes the U5-introduced stub-context regression in the U4 dashboard bridge test (missing options).
Extend BranchGroupCard/GroupTaskModal with an Abandon action (open PRs) and terminal merged/closed badges; promote stays completion-gated. New fn branch-group list|show|promote (alias fn bg) reaching the same coordinator path with createGroupPrCallback wired — agent-native parity with the dashboard promote flow, same completion-gate rejection.
…(U8) Engine half: real-git E2E covering planning- and mission-sourced groups — members land on the group branch (never main/sibling), completion-gated single PR via injected callback, re-promote idempotency, sync on later landing, abandon→closed, and a self-healing finalize mid-flow staying group-anchored. Core half: real triageFeature stamps the BG- id, member enumeration, and the canonical completion gate flipping on landing.
Simplicity pass over the 8-unit diff: delete caller-less closeGroupPrCallback (CLI), dead dashboard createGroupPullRequest/syncGroupPullRequest (+ their builders/types/tests — production uses the CLI callbacks), and merge the two CLI PR-body builders into one parameterized function. ~140 LOC of parallel-but-unused code from isolated unit implementation.
…en-PR reuse only Code review (Tier 2) found two P1s: (1) the early no-op fast-path persisted mergeConfirmed/mergeTargetBranch without mergeTargetSource, so a shared-group member landing via it could never satisfy the strict completion predicate — promotion permanently blocked; thread mergeTarget.source through like the standard landing sites. (2) createGroupPrCallback's findPrForBranch used state:'all' and could reuse a closed/merged PR from a prior group, persisting a terminal prState onto a fresh promotion; create path now matches open PRs only.
…ped sync block Review residuals #3/#4/#6/#10: per-group in-process promotion lock (concurrent route+auto promotion could double-create PRs), finalized-but-PR-less groups can be repaired by re-promotion without re-merging, auto-promotion failures emit merge:branch-group-promotion-failed instead of silent swallow, exported reconcileBranchGroupPr for out-of-band merged reconciliation, and the merger sync block drops its (store as any) casts (TaskStore already carries the methods).
… residuals Review residuals #5/#7/#8/#11/#12 + #3 wiring: forward the configured GitHub token to the abandon route's client; guard abandon against finalized/merged groups; reconcile an open group PR's state from GitHub on single-group reads (merged out-of-band now flips prState); add fn branch-group abandon for agent-native parity; block branchName shell injection (execFile argv push + core-side branch-name validation at group creation); and collapse the branch-groups list N+1 to a single task fetch via a shared filterTasksByBranchGroup helper.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds canonical branch-group identity/completion, safe merge routing to the group branch, single managed PR creation/reuse, PR sync/reconcile and abandon, CLI/dashboard wiring and commands, hardened recovery attribution, and extensive tests and docs. ChangesBranch-group single managed PR flow
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Dashboard
participant ProjectEngine
participant Coordinator
participant GitHub
User->>CLI: fn branch-group promote <id>
CLI->>ProjectEngine: promoteBranchGroup(id)
alt complete group
ProjectEngine->>Coordinator: promote(group, createGroupPr)
Coordinator->>GitHub: create/reuse PR
GitHub-->>Coordinator: prNumber/prUrl/prState
Coordinator-->>ProjectEngine: promotion result
else incomplete
ProjectEngine-->>CLI: reject (incomplete)
end
Dashboard->>ProjectEngine: reconcile/abandon via routers
ProjectEngine->>GitHub: close/update PR (best-effort)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
Greptile SummaryThis PR completes the branch-group → single managed PR flow end-to-end, fixing several silent broken paths (synthetic groupId dead-wiring, divergent completion predicates, missing engine
Confidence Score: 5/5Safe to merge. All previously identified issues have been addressed and the new single-PR flow is correct end-to-end. The promotion lock pattern is correctly structured (chain-on-prior, re-read-inside-lock, conditional cleanup). The mergeTargetSource stamp added to tryEarlyEmptyOwnDiffFinalize closes a silent gap where no-op group members could never satisfy isBranchGroupMemberLanded. The commitOwnedByTask implementations in both already-merged-detector and self-healing are now consistent and in sync. The execFileAsync switch throughout git invocations is complete and correct. All entry points (planning routes + mission store) now stamp real BG-… group IDs. The DI seams for createGroupPr/syncGroupPr are correctly threaded through all three engine-construction sites. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant M as Merger (aiMergeTask)
participant E as ProjectEngine
participant C as group-merge-coordinator
participant GH as GitHubClient
participant S as TaskStore
Note over M,S: Member task lands on group branch
M->>S: updateTask(mergeDetails)
M->>S: recordBranchGroupMemberLanding(groupId, taskId)
M-->>M: fire-and-forget syncGroupPrOnLanding
M->>GH: getPrStatus (reconcile open/closed/merged)
GH-->>M: PrInfo
M->>GH: "updatePr(body=checklist+x/N)"
GH-->>M: PrInfo
M->>S: updateBranchGroup(prState if changed)
Note over E,S: Last member lands - auto-promotion
E->>C: promoteBranchGroup(groupId, createGroupPr)
Note over C: per-group promise-chain lock
C->>S: getBranchGroup (re-read inside lock)
C->>C: evaluateBranchGroupCompletion
C->>C: git checkout integrationBranch
C->>C: git merge --no-ff group.branchName
C->>GH: createGroupPr (push + gh pr create)
GH-->>C: prNumber, prUrl, prState
C->>S: updateBranchGroup(finalized, prNumber, prUrl, prState)
Note over E,S: GET /branch-groups/:id - out-of-band reconcile
E->>C: "reconcileBranchGroupPr(fetchMembers=false)"
C->>GH: getPrStatus
GH-->>C: PrInfo
C->>S: updateBranchGroup(prState) if changed
Reviews (9): Last reviewed commit: "test(FN-branch-group): update routes-tas..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
packages/dashboard/app/components/BranchGroupCard.tsx (1)
163-189:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAbandon control is unreachable if completion reverts while the PR is open.
The "Abandon group" button lives inside the
completeblock (Line 163). Per the cohort's hard-cancel semantics, a member can be moved back (in-progress → todo), which dropscompletion.landedand flipscompletion.completetofalse. If that happens whileprState === "open", this entire actions block is hidden and the user can no longer abandon (and close) an open PR from the card. Abandon's only real precondition is an open PR, so it shouldn't be coupled tocomplete.The same gating exists in
GroupTaskModal.tsx(Line 159 + 169).♻️ One option: render the abandon control independent of
complete- {!collapsed && complete && group.prState !== "merged" && group.prState !== "closed" && ( + {!collapsed && (complete || group.prState === "open") && group.prState !== "merged" && group.prState !== "closed" && ( <div className="branch-group-card-actions">Note: the "Open PR" / "Merge group into main" promote button should still be gated on
complete; only the abandon path needs to remain reachable for an open PR.🤖 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/dashboard/app/components/BranchGroupCard.tsx` around lines 163 - 189, The abandon button is currently nested under the complete gating so it disappears when completion flips false even if group.prState === "open"; move the Abandon control out of the complete conditional so it renders whenever group.prState === "open". Concretely, in BranchGroupCard.tsx (and mirror the same change in GroupTaskModal.tsx) keep the promote/Open PR/Merge button logic inside the existing complete && ... block but extract the {group.prState === "open" && <button ... onClick={() => void onAbandon()} ...>} block so it is rendered outside (and after) the complete check; ensure the button still uses abandoning and shows Loader2 when abandoning. This preserves promote gating while making onAbandon reachable whenever prState is "open".packages/cli/src/commands/task-lifecycle.ts (2)
462-486:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't relink terminal PRs in the legacy shared-group path.
Line 463 still looks up
state: "all"and then persists whatever PR it finds as the live group PR. That reintroduces the same closed/merged-PR reuse bug thatcreateGroupPrCallback()just fixed: if localprNumberis missing, this path will attach an old terminal PR instead of creating a fresh open one.Proposed fix
- groupPrInfo = await github.findPrForBranch({ head: branchGroup.branchName, state: "all" }); + groupPrInfo = await github.findPrForBranch({ head: branchGroup.branchName, state: "open" });🤖 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/cli/src/commands/task-lifecycle.ts` around lines 462 - 486, The code currently calls github.findPrForBranch({ head: branchGroup.branchName, state: "all" }) and will persist any found PR (groupPrInfo) even if it's closed/merged, causing terminal PRs to be relinked; change this so the path only accepts an open PR: either call github.findPrForBranch with state: "open" or, after fetching groupPrInfo, check groupPrInfo.state and treat non-open states as "no PR found" (proceed to push and call github.createPr). Ensure subsequent calls that persist the PR (e.g., store.updateBranchGroup / store.logEntry and the "Linked existing group PR" branch) only run when the PR is open so closed/merged PRs are not reattached.
76-98:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winFinish removing shell interpolation from the branch probes.
Lines 88-97 still run
git show-ref/git ls-remotethroughexecAsync(command)with${branch}and${localRef}embedded in a shell string. That means a crafted task/branch name can still trigger local command substitution before the laterexecFile("git", ["push", ...])hardening ever runs.Proposed fix
-async function gitCommandSucceeds(cwd: string, command: string, missingExitCode: number): Promise<boolean> { +async function gitCommandSucceeds( + cwd: string, + file: string, + args: string[], + missingExitCode: number, +): Promise<boolean> { try { - await execAsync(command, { cwd, timeout: 30_000 }); + await execFileAsync(file, args, { cwd, timeout: 30_000 }); return true; } catch (err: unknown) { if (commandExitCode(err) === missingExitCode) return false; throw err; } } async function pushTaskBranchToOrigin(cwd: string, branch: string): Promise<void> { const localRef = `refs/heads/${branch}`; const localBranchExists = await gitCommandSucceeds( cwd, - `git show-ref --verify --quiet "${localRef}"`, + "git", + ["show-ref", "--verify", "--quiet", localRef], 1, ); if (!localBranchExists) { const remoteBranchExists = await gitCommandSucceeds( cwd, - `git ls-remote --exit-code --heads origin "${branch}"`, + "git", + ["ls-remote", "--exit-code", "--heads", "origin", branch], 2, );Also applies to: 110-113
🤖 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/cli/src/commands/task-lifecycle.ts` around lines 76 - 98, The branch-probing calls in gitCommandSucceeds / pushTaskBranchToOrigin still interpolate branch names into shell command strings, allowing command injection; change these probes to call git without a shell by using execFile-style invocation (no string interpolation): invoke git with argument arrays for the show-ref check (use args ["show-ref","--verify","--quiet", localRef]) and for the ls-remote check (use args ["ls-remote","--exit-code","--heads","origin", branch]); update gitCommandSucceeds or add a sibling helper to accept execFile arguments and use that for the checks in pushTaskBranchToOrigin (and the other probe referenced at lines 110-113) so branch names are never passed through a shell.packages/engine/src/group-merge-coordinator.ts (1)
166-173:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace shell-command interpolation with argv-based git calls.
These
execAsync("git ...")calls are still shell-invoked, andJSON.stringify(...)is not shell-safe: branch names likefoo$(touch /tmp/pwned)will still execute inside double quotes. Here that affects settings-derivedintegrationBranch, repo-derivedcurrentBranch, and any branch name that reaches this helper, so the PR reintroduces command-injection risk despite the hardening goal.Suggested fix
-import { exec } from "node:child_process"; +import { exec, execFile } from "node:child_process"; import { promisify } from "node:util"; ... const execAsync = promisify(exec); +const execFileAsync = promisify(execFile); ... async function ensureGroupBranchExists(rootDir: string, branchName: string, startPoint: string): Promise<void> { - const quotedBranch = JSON.stringify(`refs/heads/${branchName}`); try { - await execAsync(`git show-ref --verify --quiet ${quotedBranch}`, { cwd: rootDir }); + await execFileAsync("git", ["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd: rootDir }); return; } catch { - await execAsync(`git branch ${JSON.stringify(branchName)} ${JSON.stringify(startPoint)}`, { cwd: rootDir }); + await execFileAsync("git", ["branch", branchName, startPoint], { cwd: rootDir }); } } ... - await execAsync(`git checkout ${JSON.stringify(integrationBranch)}`, { cwd: input.rootDir }); - await execAsync(`git merge --no-ff --no-edit ${JSON.stringify(group.branchName)}`, { cwd: input.rootDir }); + await execFileAsync("git", ["checkout", integrationBranch], { cwd: input.rootDir }); + await execFileAsync("git", ["merge", "--no-ff", "--no-edit", group.branchName], { cwd: input.rootDir }); } finally { - await execAsync(`git checkout ${JSON.stringify(currentBranch)}`, { cwd: input.rootDir }); + await execFileAsync("git", ["checkout", currentBranch], { cwd: input.rootDir }); }Also applies to: 344-349
🤖 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/engine/src/group-merge-coordinator.ts` around lines 166 - 173, The git commands in ensureGroupBranchExists (and the similar block at 344-349) currently build shell strings with JSON.stringify, allowing command injection; change them to argv-based invocations (e.g., use child_process.execFile or spawn with an args array) and pass arguments separately instead of interpolating into a shell string: call git with args ['show-ref','--verify','--quiet', `refs/heads/${branchName}`] to check existence and ['branch', branchName, startPoint] to create the branch, and keep the same try/catch flow and error handling around execFile/spawn to preserve behavior.packages/dashboard/src/__tests__/shared-branch-group-entry-points.test.ts (1)
251-357:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
## Surface Enumerationto this regression test spec.This bug-fix invariant test currently omits the mandatory enumeration of covered surfaces.
As per coding guidelines: “When fixing a bug, the regression test must assert the general invariant across ALL known surfaces … The spec must include a
## Surface Enumerationsection.”🤖 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/dashboard/src/__tests__/shared-branch-group-entry-points.test.ts` around lines 251 - 357, The test "shared branch-group entry-point invariants" is missing the required "## Surface Enumeration" section; add a clear enumeration of all surfaces covered by this regression spec (e.g., planning/subtasks, new-task shared-group, project-default, existing, custom-new, auto-new, per-task-derived) — place it at the top of the describe block as a comment or a named sub-test so it’s part of the spec metadata and visible in test output, and ensure the listed surfaces correspond to the behaviors asserted by the existing assertions (references: describe("shared branch-group entry-point invariants"), the two it(...) blocks and assertions that check store.createTask, ensureBranchGroupForSource, getBranchGroupByBranchName, setTaskBranchGroup, and updateTask calls).packages/core/src/__tests__/branch-group-completion.test.ts (1)
1-88:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd the required
## Surface Enumerationsection for this regression spec.This test validates a bug-fix invariant but does not include the mandatory surface-enumeration block required by repo test policy.
As per coding guidelines: “When fixing a bug, the regression test must assert the general invariant across ALL known surfaces … The spec must include a
## Surface Enumerationsection.”🤖 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-completion.test.ts` around lines 1 - 88, Add the missing mandatory "## Surface Enumeration" block to this regression test file: include a top-level comment section titled "## Surface Enumeration" that lists every known surface that should be covered by this invariant (e.g., the unit test surface invoking isBranchGroupMemberLanded and isBranchGroupComplete, plus any higher-level surfaces that rely on them such as the branch-group API handler, background worker that processes merges, and any CLI/automation entrypoints). Update the test file comment to enumerate these surfaces explicitly so the regression spec for the bug fix documents all surfaces exercising the invariants for isBranchGroupMemberLanded and isBranchGroupComplete.packages/core/src/mission-store.ts (1)
3815-3849:⚠️ Potential issue | 🟠 Major | ⚡ Quick winOnly stamp
groupIdfor actual shared-mode members.
missionGroupIdstill defaults tomission:${missionId}for every mission task.filterTasksByBranchGroup()now treats that synthetic value as the legacy shared-membership fallback, so auto-per-task mission tasks will get swept into a real shared branch group if one is ever created for the same mission. That can block promotion or inflate the managed PR with unrelated members.Suggested fix
- let missionGroupId = `mission:${missionId}`; + let missionGroupId: string | undefined; if (missionId && resolvedAssignmentMode === "shared") { const settings = await this.taskStore.getSettings(); const settingsDefaultBranch = typeof settings.defaultBranch === "string" && settings.defaultBranch.trim().length > 0 ? settings.defaultBranch @@ branchContext: { - groupId: missionGroupId, + ...(missionGroupId ? { groupId: missionGroupId } : {}), source: "mission" as const, assignmentMode: resolvedAssignmentMode, inheritedBaseBranch: resolvedBaseBranch, },Based on learnings: Shared-branch-group members (
branchContext.assignmentMode === "shared") still run the member→shared-branch local integration step while auto-merge is off. This exception is only for assemblingbranch_groups.branchName; shared-branch → default-branch promotion remains gated by group/global auto-merge (FN-5819).🤖 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/mission-store.ts` around lines 3815 - 3849, The code always assigns a synthetic missionGroupId (`mission:${missionId}`) into task branchContext even for non-shared members; change createTask's branchContext so that groupId is only included when resolvedAssignmentMode === "shared". Locate the logic that computes `missionGroupId` (uses `ensureBranchGroupForSource("mission", missionId, ...)`) and the `createTask` call that sets `branchContext`; remove or omit the `groupId: missionGroupId` entry unless `resolvedAssignmentMode === "shared"` (keep source: "mission" for context when appropriate), ensuring only true shared-mode tasks get the real group id.packages/core/src/store.ts (2)
4339-4365:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate branch-group renames too.
Line 4342 protects creates, but
updateBranchGroup()still persistspatch.branchNamewithout the same check. Renaming an existing group can still store an injection-shaped ref and send it into the downstream git/PR flows this change is trying to harden.Suggested fix
updateBranchGroup(id: string, patch: BranchGroupUpdate): BranchGroup { const current = this.getBranchGroup(id); if (!current) { throw new Error(`Branch group ${id} not found`); } + if (patch.branchName !== undefined) { + validateBranchGroupBranchName(patch.branchName); + } const nextStatus = patch.status ?? current.status; const now = Date.now();🤖 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 4339 - 4365, The createBranchGroup path validates branch names via validateBranchGroupBranchName but updateBranchGroup does not, allowing injection-shaped names on rename; update updateBranchGroup to call validateBranchGroupBranchName on any incoming patch.branchName (and reject/throw before persisting) so the same check that protects createBranchGroup is applied to renames—ensure you reference updateBranchGroup, validateBranchGroupBranchName, and the patch.branchName value when adding the validation.
4438-4461:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
assignmentModecan be persisted incorrectly here.Line 4460 falls back to
task.branchContext?.assignmentMode ?? "shared", but theBranchGrouprow itself does not carry the authoritative mode. That means first-time assignment into aper-task-derivedgroup silently stamps"shared"whenever a caller omitsoptions.assignmentMode, and reassigning between groups can carry the previous group's mode forward.Based on learnings: Shared-branch-group members (`branchContext.assignmentMode === "shared"`) still run the member→shared-branch local integration step while auto-merge is off.Minimal stopgap
if (branchGroupId) { const group = this.getBranchGroup(branchGroupId); if (!group) { throw new Error(`Branch group ${branchGroupId} not found`); } + const inheritedAssignmentMode = + task.branchContext?.groupId === group.id ? task.branchContext.assignmentMode : undefined; + const resolvedAssignmentMode = options?.assignmentMode ?? inheritedAssignmentMode; + if (!resolvedAssignmentMode) { + throw new Error(`assignmentMode is required when assigning ${taskId} to branch group ${group.id}`); + } branchContext = { groupId: group.id, source: group.sourceType, - assignmentMode: options?.assignmentMode ?? task.branchContext?.assignmentMode ?? "shared", + assignmentMode: resolvedAssignmentMode, }; }🤖 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 4438 - 4461, The code in setTaskBranchGroup currently falls back to task.branchContext?.assignmentMode, which can incorrectly carry a prior assignment mode or default new per-task-derived groups to "shared"; change the assignmentMode logic so it does not inherit from task.branchContext—use options.assignmentMode if provided, otherwise derive the mode from the BranchGroup itself (e.g., if group.sourceType === "per-task-derived" set assignmentMode to the per-task-derived value, else default to "shared"), updating the branchContext assignmentMode calculation in setTaskBranchGroup to prevent carrying previous modes when reassigning groups.
🧹 Nitpick comments (7)
packages/dashboard/app/components/BranchGroupCard.tsx (1)
184-188: 💤 Low valueConsider confirming the destructive abandon action.
onAbandoncloses the managed GitHub PR with no confirmation; a single misclick is irreversible from the UI. A lightweightwindow.confirm(or existing modal confirm) before invokingonAbandonwould prevent accidental closure.🤖 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/dashboard/app/components/BranchGroupCard.tsx` around lines 184 - 188, Wrap the destructive call to onAbandon in a user confirmation prompt in BranchGroupCard.tsx: update the button's onClick handler so it first runs a lightweight confirmation (e.g., window.confirm with a clear message) and only calls onAbandon() if the user confirms; ensure the existing abandoning state/disabled logic and Loader2 behavior remain unchanged so the button still shows the spinner and disables while abandoning.packages/cli/src/commands/__tests__/task-lifecycle.test.ts (1)
1392-1463: ⚡ Quick winAdd the same terminal-PR regression to
processPullRequestMergeTask().These cases lock the invariant on
createGroupPrCallback(), but the production code still has a second shared-group PR lookup path inprocessPullRequestMergeTask(). Please add a closed/merged-PR reuse case there too so both surfaces stay aligned. 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."🤖 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/cli/src/commands/__tests__/task-lifecycle.test.ts` around lines 1392 - 1463, processPullRequestMergeTask currently has a second lookup path that can resurrect closed/merged terminal PRs unlike createGroupPrCallback; update processPullRequestMergeTask to call github.findPrForBranch({ head: branchName, state: "open" }) and only reuse a returned PR when it exists and is open, otherwise call github.createPr(...) to create a fresh PR (use the same branching/args as createGroupPrCallback), and add a unit test mirroring the createGroupPrCallback closed/merged-PR case to assert it creates a new PR rather than reusing a terminal one; reference processPullRequestMergeTask, createGroupPrCallback, findPrForBranch, and createPr when making the changes.packages/engine/src/__tests__/project-engine.test.ts (1)
1785-1785: ⚡ Quick winUse FN-XXXX task ID format in test title.
The test title uses "(Fix
#4)" but the codebase convention uses FN-prefixed task IDs (e.g., FN-5627, FN-4084). Based on learnings, commit prefixes and task references should follow theFN-XXXXformat.♻️ Update test title to use task ID convention
- it("records an audit event (not silent) when auto-promotion of a branch-group member fails (Fix `#4`)", async () => { + it("records an audit event (not silent) when auto-promotion of a branch-group member fails (FN-XXXX)", async () => {Replace
FN-XXXXwith the actual task identifier if available.Based on learnings: Commit prefixes should use
feat(FN-XXX):,fix(FN-XXX):,test(FN-XXX):format with task ID prefix.🤖 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/engine/src/__tests__/project-engine.test.ts` at line 1785, Update the test title string for the "records an audit event (not silent) when auto-promotion of a branch-group member fails" test (the it(...) declaration) to use the repository task ID convention by replacing the "(Fix `#4`)" suffix with an FN-prefixed task ID in the form "(FN-XXXX)"; if you know the real task number use that exact FN-#### value, otherwise use a placeholder like "(FN-XXXX)" so the title follows the test(FN-XXX): convention.packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts (2)
98-101: ⚡ Quick winUse the real
listTasksByBranchGroupin this promote driver.Despite the comment above, this helper is reconstructing membership from a fixed
memberIdslist. Promotion would still look correct here if persisted branch-group membership drifted or if later members were added/removed, which weakens the end-to-end guarantee this suite is supposed to provide.🤖 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/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts` around lines 98 - 101, The promote-driver test is faking branch-group membership by reconstructing from the fixed memberIds array in the listTasksByBranchGroup helper; replace that stub with the real implementation (call the actual store/listTasksByBranchGroup function or the production helper used by the branch-group code instead of mapping memberIds -> store.getTask), ensuring you return the true live membership results (and keep the return type as Task[]/any as expected by the test). Locate the current helper named listTasksByBranchGroup and remove the memberIds-based reconstruction, invoking the real persistence/query method that the runtime uses so promotions reflect persisted membership drift and member changes.
17-37: ⚡ Quick winAdd the required
## Surface Enumerationsection.This suite is already spanning planning, mission, sync, abandon, and self-healing surfaces, so it should capture that inventory explicitly under the required heading instead of only in prose.
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 Enumerationsection."🤖 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/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts` around lines 17 - 37, Add a top-level "## Surface Enumeration" section to the file's header comment in branch-group-single-pr-e2e.test.ts that explicitly lists the surfaces this suite covers (e.g., planning, mission, sync, abandon, self-healing) and briefly ties each surface to the corresponding flow tested (planning→engine group creation, mission triage entry-point shape, sync behavior as members land, abandon closing PR, and self-healing/idempotent re-promotion). Ensure the new section is placed alongside the existing descriptive block so reviewers can see the required inventory for the regression test suite.packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts (1)
10-15: ⚡ Quick winAdd the required
## Surface Enumerationsection.This is a regression suite for the member-landing PR-sync fix, but the affected surfaces are only described informally. Please add the explicit
## Surface Enumerationblock so the covered states stay visible and future edits do not narrow the invariant unintentionally.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 Enumerationsection."🤖 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/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts` around lines 10 - 15, Add a formal "## Surface Enumeration" section to the branch-group-pr-sync.test.ts regression docblock that explicitly lists every surface the tests are intended to cover: e.g., the single managed group PR being kept in sync as members land, the aiMergeTask -> recordBranchGroupMemberLanding invocation path, the presence of a persisted open PR triggering syncGroupPr, different member states reflected in the sync payload (landing/succeeded/removed), and that sync failures are non-fatal; place this block near the existing file header comment so the invariant is clearly documented alongside the tests.packages/core/src/__tests__/mission-store.test.ts (1)
2239-2537: ⚡ Quick winConsider adding a
## Surface Enumerationsection for the group stamping fix.The updated tests verify the bug fix for "Corrected group stamping" across multiple surfaces (existing branch strategy, explicit overrides, shared-mode slice triage). As per coding guidelines, bug-fix tests should include an explicit
## Surface Enumerationsection documenting all tested surfaces.While the tests do cover the necessary surfaces, a formal enumeration would improve documentation and future maintainability. For example:
## Surface Enumeration Group stamping fix verified across: 1. Mission branchStrategy with existing branch mode (triageFeature) 2. Explicit branch options override (triageFeature) 3. Shared-mode slice triage creating multiple members (triageSlice) 4. Legacy synthetic groupId fallback for planning/mission sourcesThe test coverage itself is thorough and correct.
Based on learnings: "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 Enumerationsection."🤖 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__/mission-store.test.ts` around lines 2239 - 2537, Add a short "## Surface Enumeration" comment block to the mission-store.test.ts regression tests (near the new "Corrected group stamping" assertions) that explicitly enumerates all surfaces covered by the regression: triageFeature with mission branchStrategy existing mode, triageFeature with explicit branch overrides, triageSlice in shared-mode creating multiple members, and the legacy synthetic groupId fallback for planning/mission sources; update the test file around the triageFeature and triageSlice test groups (referencing the test names "explicit branch options override", "triageSlice respects explicit branch options", and "triageSlice shared mode creates distinct per-task branches with one shared merge target") to include this markdown-style enumeration so the regression is documented for future maintainers.
🤖 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/cli/src/commands/daemon.ts`:
- Around line 339-340: The group-PR callbacks are resolving repository context
globally and can attach metadata to the wrong repo in multi-project mode; update
createGroupPrCallback and syncGroupPrCallback so they are project-scoped by
passing the project's working directory or repo context into them and using that
when calling getCurrentRepo (e.g., call getCurrentRepo with the project's cwd or
inject a repoResolver into the callback), and ensure any place that registers
createGroupPrCallback/syncGroupPrCallback supplies the current project's cwd so
repo resolution and PR persistence happen per-project rather than globally.
In `@packages/cli/src/commands/task-lifecycle.ts`:
- Around line 258-269: syncGroupPrCallback currently calls getCurrentRepo()
inside the function which can pick the wrong repository; change
syncGroupPrCallback to accept the repo identity as an explicit parameter (for
example add a repo: { owner: string; repo: string } argument or capture it via
the caller) and use that repo when calling github.getPrStatus(...) and
github.updatePr(...), removing the getCurrentRepo() call; update all call sites
to pass the correct repo into syncGroupPrCallback so PR status updates for
group.prNumber target the intended repository.
In `@packages/core/src/__tests__/branch-group-entry-point-e2e.test.ts`:
- Around line 10-32: Add a "## Surface Enumeration" section to the
branch-group-entry-point-e2e.test.ts regression test (the U8 (R9) entry-point
test) that explicitly lists all external surfaces the invariant covers (e.g.,
TaskStore mission triage, branchContext.groupId propagation,
listTasksByBranchGroup(group.id), and any mission/member interaction paths) and
add assertions or comments tying each listed surface to the existing checks in
the test so the regression spec documents and verifies the invariant across ALL
known surfaces referenced by the test (use the top comment block and the main
test function names as anchors for placement).
In `@packages/core/src/branch-assignment.ts`:
- Around line 24-39: The branch-name validator is too permissive and the
metacharacter regex trips ESLint: update isValidBranchGroupBranchName to reject
any name containing empty path segments (disallow "//"), reject any path segment
that starts with "." (e.g., "/.tmp" or "foo/.tmp"), and reject any path segment
that ends with ".lock" or "." (e.g., "foo.lock/bar" or "foo/.tmp."), by
splitting name on "/" and validating each segment (no empty segments,
!segment.startsWith("."), !segment.endsWith(".") and
!segment.endsWith(".lock")); keep the existing global checks (length,
whitespace, reserved names, forbidden sequences like ".." and "@{", shell
metacharacters), and fix the ESLint no-useless-escape by removing the
unnecessary backslash before "[" in the metacharacter regex used in
isValidBranchGroupBranchName.
In `@packages/dashboard/src/github.ts`:
- Around line 3825-3905: The helpers reconcileGroupPullRequest and
closeGroupPullRequest currently call getCurrentRepoOrThrow() internally which
uses process.cwd(), causing repository mis-resolution for multi-project servers;
change both functions to accept owner and repo (or a single repo context) as
explicit parameters instead of calling getCurrentRepoOrThrow(), update their
uses (e.g., the integrated router callers) to pass the correct owner/repo from
the request context, and modify the closePr invocation in closeGroupPullRequest
to call github.closePr with the owner/repo plus number (instead of re-resolving
cwd inside closePr) so all GitHub API calls use the provided repo context.
In `@packages/engine/src/__tests__/already-merged-detector.real-git.test.ts`:
- Around line 81-117: Capture the SHA of the unrelated prose-mention commit
right after creating it (e.g. call git(repo, "git rev-parse HEAD") into a
variable like proseSha), then after calling findAlreadyMergedTaskCommit compute
returnedSha = result ? result.sha : null and add an assertion
expect(returnedSha).not.toBe(proseSha) so the test explicitly fails if the
detector ever returns the prose-mention commit regardless of result or strategy;
refer to findAlreadyMergedTaskCommit, result, and strategy to locate where to
insert this check.
In `@packages/engine/src/__tests__/group-merge-coordinator.test.ts`:
- Line 709: Replace the real sleep call await new Promise((resolve) =>
setTimeout(resolve, 25)) with a deterministic concurrency gate: either use fake
timers (jest.useFakeTimers(); trigger the timeout with
jest.advanceTimersByTime(25); then jest.useRealTimers()) or implement a simple
Deferred/gate (e.g., const gate = createDeferred(); await gate.promise; and
resolve the gate at the exact point you want to allow progress). Update the test
code around the existing await new Promise(...) to await the gate or advance
timers so the overlap is forced deterministically and no real wall-clock sleep
remains.
In
`@packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts`:
- Around line 308-326: The test currently hand-rolls the abandon behavior by
calling the mock closeGroupPr and directly calling
store.updateBranchGroup/getBranchGroup, which bypasses the real abandon
implementation; replace this block so the test calls the actual abandon flow
(e.g., the function under test that implements abandon, such as
abandonBranchGroup or the relevant handler) instead of invoking closeGroupPr or
store.updateBranchGroup directly, assert that the real flow calls the PR-close
interaction and persists prState="closed" and status="abandoned" (use
getBranchGroup to verify), and then invoke the same abandon function a second
time to assert idempotency (no extra PR-close call and no state changes) rather
than manually checking state.
- Around line 244-246: The test currently mutates the store directly with
store.updateBranchGroup(group.id, { status: "finalized", prState: "merged" })
which doesn't exercise the reconciliation path; instead, simulate the
out-of-band GitHub state change and call the production reconciliation function
(e.g., syncGroupPr(group.id) or the reconcile method used by the system) to have
the code read the external PR state and persist it to the store, then assert
store.getBranchGroup(group.id)?.prState === "merged"; alternatively, if no
reconciliation seam exists in tests, remove the wording about out-of-band
reconciliation and keep the direct mutation/assertion.
In `@packages/engine/src/already-merged-detector.ts`:
- Around line 37-39: The git-regex inputs (taskId/lineageId) used in the git log
--grep calls, trailer lookup, and ancestry candidate query are not escaped; use
the existing escapeRegex(value: string) helper to sanitize any ID before
interpolating into git grep/regex arguments (apply to the grep usages around the
ancestry candidate query and trailer lookup and where commitOwnedByTask() is
invoked) so that regex metacharacters in IDs cannot overmatch or miss commits;
update all occurrences (including the blocks referenced near the ancestry
candidate query and trailer/trailer-lookup logic) to call escapeRegex(taskId or
lineageId) before building the git command.
In `@packages/engine/src/group-merge-coordinator.ts`:
- Around line 362-375: The code currently reuses a PR number from any sibling
group without checking its state and then unconditionally sets prState = "open";
update the persistedPr logic so you only reuse a sibling's PR when
input.store.getBranchGroupByBranchName(group.branchName) returns an existing row
whose existing.prState === "open", and only then set prNumber/prUrl and prState
= "open"; if persistedPr comes from the current group (group.prNumber) preserve
the group's existing prState instead of forcing "open"; ensure the condition
that builds persistedPr and the subsequent assignment to prState reference
existing.prState and group.prNumber appropriately so closed/merged sibling PRs
are not reused and createGroupPr remains reachable.
In `@packages/engine/src/merger.ts`:
- Around line 7524-7568: The syncGroupPr call is currently awaited on the main
merge path (options.syncGroupPr inside the if block) and a failing async audit
can escape because recordRunAuditEvent isn't awaited; move this work off the
critical path by invoking options.syncGroupPr in a fire-and-forget background
task (e.g., wrap in Promise.resolve().then(...) so it doesn't block the merge)
or wrap the call with a local timeout to bound how long it can delay completion,
and ensure the fallback audit write uses await
Promise.resolve(store.recordRunAuditEvent(...)) (or otherwise await the promise)
inside its own try/catch so any rejection is contained. Reference
options.syncGroupPr, latestGroup, and store.recordRunAuditEvent when making the
changes.
---
Outside diff comments:
In `@packages/cli/src/commands/task-lifecycle.ts`:
- Around line 462-486: The code currently calls github.findPrForBranch({ head:
branchGroup.branchName, state: "all" }) and will persist any found PR
(groupPrInfo) even if it's closed/merged, causing terminal PRs to be relinked;
change this so the path only accepts an open PR: either call
github.findPrForBranch with state: "open" or, after fetching groupPrInfo, check
groupPrInfo.state and treat non-open states as "no PR found" (proceed to push
and call github.createPr). Ensure subsequent calls that persist the PR (e.g.,
store.updateBranchGroup / store.logEntry and the "Linked existing group PR"
branch) only run when the PR is open so closed/merged PRs are not reattached.
- Around line 76-98: The branch-probing calls in gitCommandSucceeds /
pushTaskBranchToOrigin still interpolate branch names into shell command
strings, allowing command injection; change these probes to call git without a
shell by using execFile-style invocation (no string interpolation): invoke git
with argument arrays for the show-ref check (use args
["show-ref","--verify","--quiet", localRef]) and for the ls-remote check (use
args ["ls-remote","--exit-code","--heads","origin", branch]); update
gitCommandSucceeds or add a sibling helper to accept execFile arguments and use
that for the checks in pushTaskBranchToOrigin (and the other probe referenced at
lines 110-113) so branch names are never passed through a shell.
In `@packages/core/src/__tests__/branch-group-completion.test.ts`:
- Around line 1-88: Add the missing mandatory "## Surface Enumeration" block to
this regression test file: include a top-level comment section titled "##
Surface Enumeration" that lists every known surface that should be covered by
this invariant (e.g., the unit test surface invoking isBranchGroupMemberLanded
and isBranchGroupComplete, plus any higher-level surfaces that rely on them such
as the branch-group API handler, background worker that processes merges, and
any CLI/automation entrypoints). Update the test file comment to enumerate these
surfaces explicitly so the regression spec for the bug fix documents all
surfaces exercising the invariants for isBranchGroupMemberLanded and
isBranchGroupComplete.
In `@packages/core/src/mission-store.ts`:
- Around line 3815-3849: The code always assigns a synthetic missionGroupId
(`mission:${missionId}`) into task branchContext even for non-shared members;
change createTask's branchContext so that groupId is only included when
resolvedAssignmentMode === "shared". Locate the logic that computes
`missionGroupId` (uses `ensureBranchGroupForSource("mission", missionId, ...)`)
and the `createTask` call that sets `branchContext`; remove or omit the
`groupId: missionGroupId` entry unless `resolvedAssignmentMode === "shared"`
(keep source: "mission" for context when appropriate), ensuring only true
shared-mode tasks get the real group id.
In `@packages/core/src/store.ts`:
- Around line 4339-4365: The createBranchGroup path validates branch names via
validateBranchGroupBranchName but updateBranchGroup does not, allowing
injection-shaped names on rename; update updateBranchGroup to call
validateBranchGroupBranchName on any incoming patch.branchName (and reject/throw
before persisting) so the same check that protects createBranchGroup is applied
to renames—ensure you reference updateBranchGroup,
validateBranchGroupBranchName, and the patch.branchName value when adding the
validation.
- Around line 4438-4461: The code in setTaskBranchGroup currently falls back to
task.branchContext?.assignmentMode, which can incorrectly carry a prior
assignment mode or default new per-task-derived groups to "shared"; change the
assignmentMode logic so it does not inherit from task.branchContext—use
options.assignmentMode if provided, otherwise derive the mode from the
BranchGroup itself (e.g., if group.sourceType === "per-task-derived" set
assignmentMode to the per-task-derived value, else default to "shared"),
updating the branchContext assignmentMode calculation in setTaskBranchGroup to
prevent carrying previous modes when reassigning groups.
In `@packages/dashboard/app/components/BranchGroupCard.tsx`:
- Around line 163-189: The abandon button is currently nested under the complete
gating so it disappears when completion flips false even if group.prState ===
"open"; move the Abandon control out of the complete conditional so it renders
whenever group.prState === "open". Concretely, in BranchGroupCard.tsx (and
mirror the same change in GroupTaskModal.tsx) keep the promote/Open PR/Merge
button logic inside the existing complete && ... block but extract the
{group.prState === "open" && <button ... onClick={() => void onAbandon()} ...>}
block so it is rendered outside (and after) the complete check; ensure the
button still uses abandoning and shows Loader2 when abandoning. This preserves
promote gating while making onAbandon reachable whenever prState is "open".
In `@packages/dashboard/src/__tests__/shared-branch-group-entry-points.test.ts`:
- Around line 251-357: The test "shared branch-group entry-point invariants" is
missing the required "## Surface Enumeration" section; add a clear enumeration
of all surfaces covered by this regression spec (e.g., planning/subtasks,
new-task shared-group, project-default, existing, custom-new, auto-new,
per-task-derived) — place it at the top of the describe block as a comment or a
named sub-test so it’s part of the spec metadata and visible in test output, and
ensure the listed surfaces correspond to the behaviors asserted by the existing
assertions (references: describe("shared branch-group entry-point invariants"),
the two it(...) blocks and assertions that check store.createTask,
ensureBranchGroupForSource, getBranchGroupByBranchName, setTaskBranchGroup, and
updateTask calls).
In `@packages/engine/src/group-merge-coordinator.ts`:
- Around line 166-173: The git commands in ensureGroupBranchExists (and the
similar block at 344-349) currently build shell strings with JSON.stringify,
allowing command injection; change them to argv-based invocations (e.g., use
child_process.execFile or spawn with an args array) and pass arguments
separately instead of interpolating into a shell string: call git with args
['show-ref','--verify','--quiet', `refs/heads/${branchName}`] to check existence
and ['branch', branchName, startPoint] to create the branch, and keep the same
try/catch flow and error handling around execFile/spawn to preserve behavior.
---
Nitpick comments:
In `@packages/cli/src/commands/__tests__/task-lifecycle.test.ts`:
- Around line 1392-1463: processPullRequestMergeTask currently has a second
lookup path that can resurrect closed/merged terminal PRs unlike
createGroupPrCallback; update processPullRequestMergeTask to call
github.findPrForBranch({ head: branchName, state: "open" }) and only reuse a
returned PR when it exists and is open, otherwise call github.createPr(...) to
create a fresh PR (use the same branching/args as createGroupPrCallback), and
add a unit test mirroring the createGroupPrCallback closed/merged-PR case to
assert it creates a new PR rather than reusing a terminal one; reference
processPullRequestMergeTask, createGroupPrCallback, findPrForBranch, and
createPr when making the changes.
In `@packages/core/src/__tests__/mission-store.test.ts`:
- Around line 2239-2537: Add a short "## Surface Enumeration" comment block to
the mission-store.test.ts regression tests (near the new "Corrected group
stamping" assertions) that explicitly enumerates all surfaces covered by the
regression: triageFeature with mission branchStrategy existing mode,
triageFeature with explicit branch overrides, triageSlice in shared-mode
creating multiple members, and the legacy synthetic groupId fallback for
planning/mission sources; update the test file around the triageFeature and
triageSlice test groups (referencing the test names "explicit branch options
override", "triageSlice respects explicit branch options", and "triageSlice
shared mode creates distinct per-task branches with one shared merge target") to
include this markdown-style enumeration so the regression is documented for
future maintainers.
In `@packages/dashboard/app/components/BranchGroupCard.tsx`:
- Around line 184-188: Wrap the destructive call to onAbandon in a user
confirmation prompt in BranchGroupCard.tsx: update the button's onClick handler
so it first runs a lightweight confirmation (e.g., window.confirm with a clear
message) and only calls onAbandon() if the user confirms; ensure the existing
abandoning state/disabled logic and Loader2 behavior remain unchanged so the
button still shows the spinner and disables while abandoning.
In `@packages/engine/src/__tests__/project-engine.test.ts`:
- Line 1785: Update the test title string for the "records an audit event (not
silent) when auto-promotion of a branch-group member fails" test (the it(...)
declaration) to use the repository task ID convention by replacing the "(Fix
`#4`)" suffix with an FN-prefixed task ID in the form "(FN-XXXX)"; if you know the
real task number use that exact FN-#### value, otherwise use a placeholder like
"(FN-XXXX)" so the title follows the test(FN-XXX): convention.
In
`@packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts`:
- Around line 10-15: Add a formal "## Surface Enumeration" section to the
branch-group-pr-sync.test.ts regression docblock that explicitly lists every
surface the tests are intended to cover: e.g., the single managed group PR being
kept in sync as members land, the aiMergeTask -> recordBranchGroupMemberLanding
invocation path, the presence of a persisted open PR triggering syncGroupPr,
different member states reflected in the sync payload
(landing/succeeded/removed), and that sync failures are non-fatal; place this
block near the existing file header comment so the invariant is clearly
documented alongside the tests.
In
`@packages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.ts`:
- Around line 98-101: The promote-driver test is faking branch-group membership
by reconstructing from the fixed memberIds array in the listTasksByBranchGroup
helper; replace that stub with the real implementation (call the actual
store/listTasksByBranchGroup function or the production helper used by the
branch-group code instead of mapping memberIds -> store.getTask), ensuring you
return the true live membership results (and keep the return type as Task[]/any
as expected by the test). Locate the current helper named listTasksByBranchGroup
and remove the memberIds-based reconstruction, invoking the real
persistence/query method that the runtime uses so promotions reflect persisted
membership drift and member changes.
- Around line 17-37: Add a top-level "## Surface Enumeration" section to the
file's header comment in branch-group-single-pr-e2e.test.ts that explicitly
lists the surfaces this suite covers (e.g., planning, mission, sync, abandon,
self-healing) and briefly ties each surface to the corresponding flow tested
(planning→engine group creation, mission triage entry-point shape, sync behavior
as members land, abandon closing PR, and self-healing/idempotent re-promotion).
Ensure the new section is placed alongside the existing descriptive block so
reviewers can see the required inventory for the regression test suite.
🪄 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: 1763ca1c-7527-488b-b991-8b339ca9744d
📒 Files selected for processing (54)
.changeset/fn-5846-shared-group-merge-routing.md.changeset/fn-branch-group-single-pr.mddocs/plans/2026-06-03-001-feat-branch-group-single-pr-flow-plan.mdpackages/cli/src/bin.tspackages/cli/src/commands/__tests__/branch-group.test.tspackages/cli/src/commands/__tests__/daemon.test.tspackages/cli/src/commands/__tests__/serve.test.tspackages/cli/src/commands/__tests__/task-lifecycle.test.tspackages/cli/src/commands/branch-group.tspackages/cli/src/commands/daemon.tspackages/cli/src/commands/dashboard.tspackages/cli/src/commands/serve.tspackages/cli/src/commands/task-lifecycle.tspackages/core/src/__tests__/branch-assignment.test.tspackages/core/src/__tests__/branch-group-completion.test.tspackages/core/src/__tests__/branch-group-entry-point-e2e.test.tspackages/core/src/__tests__/branch-group-store.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/branch-assignment.tspackages/core/src/branch-group-completion.tspackages/core/src/index.tspackages/core/src/mission-store.tspackages/core/src/store.tspackages/dashboard/app/api/legacy.tspackages/dashboard/app/components/BranchGroupCard.tsxpackages/dashboard/app/components/GroupTaskModal.tsxpackages/dashboard/app/components/__tests__/BranchGroupCard.test.tsxpackages/dashboard/app/components/__tests__/GroupTaskModal.test.tsxpackages/dashboard/src/__tests__/github-close-group-pr.test.tspackages/dashboard/src/__tests__/integrated-routers-group-pr-token.test.tspackages/dashboard/src/__tests__/routes-branch-groups.test.tspackages/dashboard/src/__tests__/routes-planning.test.tspackages/dashboard/src/__tests__/shared-branch-group-entry-points.test.tspackages/dashboard/src/github.tspackages/dashboard/src/index.tspackages/dashboard/src/routes/register-branch-groups-routes.tspackages/dashboard/src/routes/register-integrated-routers.tspackages/dashboard/src/routes/register-planning-subtask-routes.tspackages/engine/src/__tests__/already-merged-detector.real-git.test.tspackages/engine/src/__tests__/group-merge-coordinator.test.tspackages/engine/src/__tests__/merger-finalize-unproven.real-git.test.tspackages/engine/src/__tests__/project-engine.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-merge-routing.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.tspackages/engine/src/__tests__/reliability-interactions/shared-branch-group-lifecycle.test.tspackages/engine/src/__tests__/self-healing.test.tspackages/engine/src/already-merged-detector.tspackages/engine/src/group-merge-coordinator.tspackages/engine/src/index.tspackages/engine/src/merger.tspackages/engine/src/project-engine-manager.tspackages/engine/src/project-engine.tspackages/engine/src/self-healing.ts
- abandon route: guard already-abandoned groups (matches CLI)
- CLI branch-group list: single task fetch via filterTasksByBranchGroup (N+1)
- branch-name validator: git check-ref-format parity (//, dot-segments, .lock, @, @{, trailing /.)
- updateBranchGroup: validate renamed branchName too
- already-merged-detector: escape regex metachars in git log --grep; non-vacuous prose-mention assertion
- group PR callbacks: thread per-project cwd through SyncGroupPrFn/reconcile/github helpers (multi-project correctness)
- merger: group-PR sync is fire-and-forget (never blocks merge completion); deterministic test handle
- coordinator: sibling PR reuse only when open; reconcile skips member fetch on read-only path; argv-based git calls (no shell)
- task-lifecycle: legacy group-PR path links open PRs only; branch probes via execFile argv (injection hardening)
- mission/planning: branchContext.groupId only stamped for actual shared-mode members (groupId now optional)
- UI: Abandon reachable whenever PR is open (decoupled from completion); promote stays completion-gated
- tests: deterministic concurrency gate, real reconcile path in e2e, Surface Enumeration sections
Collaborator
Author
|
Re: CodeRabbit's outside diff range comments (9) — all addressed in the latest commits except one, with reasoning below:
🤖 Generated with Claude Code |
…d CONCEPTS.md First docs/solutions/ learning: synthetic groupIds, mock-masked engine wiring, and side-effect-free state flips (PR #1357 post-mortem). Seeds CONCEPTS.md with the branch-group domain vocabulary and surfaces both knowledge stores in AGENTS.md reference docs.
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 (2)
packages/core/src/store.ts (1)
202-229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCanonicalize
groupIdbefore re-emitting branch context.Lines 205-206 only use
trim()as a presence check, so" BG-123 "survives as" BG-123 ", and Line 229 writes that non-canonical id back out. That leaves the task with a branch-group id that looks valid here but won't match exact group-id comparisons later.Proposed fix
- const groupId = typeof candidate.groupId === "string" && candidate.groupId.trim() - ? candidate.groupId - : undefined; + const groupId = typeof candidate.groupId === "string" + ? candidate.groupId.trim() || undefined + : undefined; @@ - ...(branchContext.groupId ? { groupId: branchContext.groupId } : {}), + ...(branchContext.groupId?.trim() + ? { groupId: branchContext.groupId.trim() } + : {}),🤖 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 202 - 229, The branchContext.groupId is being passed through without canonicalization when re-emitting TASK_BRANCH_CONTEXT_METADATA_KEY in withTaskBranchContextInSourceMetadata; trim (and optionally normalize casing if your canonical form requires it) branchContext.groupId before writing it back so values like " BG-123 " become "BG-123". Locate withTaskBranchContextInSourceMetadata and replace uses of branchContext.groupId in the returned object with the canonicalized value (e.g., const groupId = typeof branchContext.groupId === "string" ? branchContext.groupId.trim() : undefined) and then emit groupId only if present.packages/core/src/__tests__/mission-store.test.ts (1)
2454-2472:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover the non-shared
triageSlicesurface too.This bug fix is only pinned for
triageFeature.triageSlice(... auto-per-task ...)is another mission entry-point for the same invariant, but this test still doesn't assertbranchContext.groupId === undefinedandgetBranchGroupBySource("mission", mission.id) === null, so a syntheticmission:<id>regression there would slip through.Suggested assertions
expect(triaged[0].id).toBe(f1.id); expect(task?.branchContext?.assignmentMode).toBe("per-task-derived"); + expect(task?.branchContext?.groupId).toBeUndefined(); + expect(ts.getBranchGroupBySource("mission", mission.id)).toBeNull();Based on learnings, "When fixing a bug, the regression test must assert the general invariant across ALL known surfaces — not only the single reported reproduction."
🤖 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__/mission-store.test.ts` around lines 2454 - 2472, The test for triageSlice is missing assertions that enforce the branch-group invariant; update the triageSlice test (the it block calling msWithTs.triageSlice and ts.getTask) to also assert that the resulting task.branchContext.groupId is undefined and that msWithTs.getBranchGroupBySource("mission", mission.id) returns null, mirroring the coverage added for triageFeature so the synthetic "mission:<id>" group regression is caught across the other mission entry-point.
🤖 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/engine/src/merger.ts`:
- Around line 7546-7564: The reconciliation reads latestGroup and then writes
after awaits, risking overwriting a newer PR state; fix by re-reading the
current group from the store (call store.getBranchGroup(groupId) again)
immediately before calling store.updateBranchGroup and only proceed with the
update if the re-read group still matches the same PR snapshot you reconciled
(compare the same unique identifiers: latestGroup.id/ prNumber/ prState or a
snapshot token), otherwise skip the update; apply this guard around the block
that calls store.updateBranchGroup after syncGroupPr so stale callbacks cannot
overwrite a newer open PR.
---
Outside diff comments:
In `@packages/core/src/__tests__/mission-store.test.ts`:
- Around line 2454-2472: The test for triageSlice is missing assertions that
enforce the branch-group invariant; update the triageSlice test (the it block
calling msWithTs.triageSlice and ts.getTask) to also assert that the resulting
task.branchContext.groupId is undefined and that
msWithTs.getBranchGroupBySource("mission", mission.id) returns null, mirroring
the coverage added for triageFeature so the synthetic "mission:<id>" group
regression is caught across the other mission entry-point.
In `@packages/core/src/store.ts`:
- Around line 202-229: The branchContext.groupId is being passed through without
canonicalization when re-emitting TASK_BRANCH_CONTEXT_METADATA_KEY in
withTaskBranchContextInSourceMetadata; trim (and optionally normalize casing if
your canonical form requires it) branchContext.groupId before writing it back so
values like " BG-123 " become "BG-123". Locate
withTaskBranchContextInSourceMetadata and replace uses of branchContext.groupId
in the returned object with the canonicalized value (e.g., const groupId =
typeof branchContext.groupId === "string" ? branchContext.groupId.trim() :
undefined) and then emit groupId only if present.
🪄 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: 59d59cb8-ba23-4042-97c1-59298577d978
📒 Files selected for processing (34)
AGENTS.mdCONCEPTS.mddocs/solutions/integration-issues/branch-group-single-pr-synthetic-id-dead-wiring.mdpackages/cli/src/commands/__tests__/branch-group.test.tspackages/cli/src/commands/__tests__/task-lifecycle.test.tspackages/cli/src/commands/branch-group.tspackages/cli/src/commands/task-lifecycle.tspackages/core/src/__tests__/branch-assignment.test.tspackages/core/src/__tests__/branch-group-completion.test.tspackages/core/src/__tests__/branch-group-entry-point-e2e.test.tspackages/core/src/__tests__/branch-group-store.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/branch-assignment.tspackages/core/src/mission-store.tspackages/core/src/store.tspackages/core/src/types.tspackages/dashboard/app/components/BranchGroupCard.tsxpackages/dashboard/app/components/GroupTaskModal.tsxpackages/dashboard/app/components/__tests__/BranchGroupCard.test.tsxpackages/dashboard/app/components/__tests__/GroupTaskModal.test.tsxpackages/dashboard/src/__tests__/routes-branch-groups.test.tspackages/dashboard/src/__tests__/shared-branch-group-entry-points.test.tspackages/dashboard/src/github.tspackages/dashboard/src/routes/register-branch-groups-routes.tspackages/dashboard/src/routes/register-integrated-routers.tspackages/dashboard/src/routes/register-planning-subtask-routes.tspackages/engine/src/__tests__/already-merged-detector.real-git.test.tspackages/engine/src/__tests__/group-merge-coordinator.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-single-pr-e2e.test.tspackages/engine/src/already-merged-detector.tspackages/engine/src/group-merge-coordinator.tspackages/engine/src/merger.tspackages/engine/src/project-engine.ts
✅ Files skipped from review due to trivial changes (2)
- AGENTS.md
- docs/solutions/integration-issues/branch-group-single-pr-synthetic-id-dead-wiring.md
🚧 Files skipped from review as they are similar to previous changes (24)
- packages/dashboard/app/components/tests/BranchGroupCard.test.tsx
- packages/core/src/mission-store.ts
- packages/dashboard/src/tests/shared-branch-group-entry-points.test.ts
- packages/core/src/tests/branch-group-completion.test.ts
- packages/core/src/tests/branch-assignment.test.ts
- packages/dashboard/src/routes/register-integrated-routers.ts
- packages/dashboard/app/components/GroupTaskModal.tsx
- packages/dashboard/src/tests/routes-branch-groups.test.ts
- packages/core/src/tests/branch-group-entry-point-e2e.test.ts
- packages/engine/src/tests/reliability-interactions/branch-group-pr-sync.test.ts
- packages/dashboard/app/components/tests/GroupTaskModal.test.tsx
- packages/engine/src/tests/already-merged-detector.real-git.test.ts
- packages/core/src/branch-assignment.ts
- packages/dashboard/src/github.ts
- packages/engine/src/project-engine.ts
- packages/cli/src/commands/tests/task-lifecycle.test.ts
- packages/engine/src/already-merged-detector.ts
- packages/dashboard/src/routes/register-branch-groups-routes.ts
- packages/cli/src/commands/task-lifecycle.ts
- packages/dashboard/app/components/BranchGroupCard.tsx
- packages/cli/src/commands/branch-group.ts
- packages/engine/src/group-merge-coordinator.ts
- packages/engine/src/tests/reliability-interactions/branch-group-single-pr-e2e.test.ts
- packages/engine/src/tests/group-merge-coordinator.test.ts
- syncGroupPrCallback forwards owner/repo to updatePr (multi-project daemons could 404 or edit an unrelated same-numbered PR via process-cwd fallback) - merger background reconcile re-reads the group before persisting and skips the write when the PR snapshot changed (stale-write race vs newer open PR) - branchContext.groupId trimmed on metadata emit/parse round-trip - triageSlice non-shared invariant assertions (no groupId, no group row)
Collaborator
Author
|
Re: CodeRabbit's second-round outside diff range comments (2) — both addressed:
🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts (1)
198-237: 🏗️ Heavy liftMove this stale-snapshot regression to a narrower seam.
This case is validating the post-
syncGroupPrreconciliation/write guard, but the new coverage adds anotherhasGit+45_000real-git test to get there. Please cover it closer to the merger/store seam so the race stays deterministic without expanding the slow reliability suite.As per coding guidelines, "Do not add slow tests (FN-5048). Prefer narrow seams, in-memory fakes, shared harnesses, and targeted assertions."
🤖 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/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts` around lines 198 - 237, The test is a slow, real-git reliability test exercising the stale-snapshot reconciliation (stale write guard) and should be moved to a much narrower seam that fakes Git and exercises only the merger/store interaction: copy or re-create this scenario as a fast unit test that does not use hasGit or real repo setup but instead uses the in-memory store and a mocked syncGroupPr to simulate the race; call stageMergeBranch (or skip it) and invoke aiMergeTask with the mocked syncGroupPr and onGroupPrSyncSettled to drive the code path, then assert on store.getBranchGroup(group.id).prNumber/prState to ensure the stale "merged" write is skipped—remove the hasGit guard and the 45_000 timeout from the new test and place it alongside other aiMergeTask/merger unit tests.
🤖 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.
Nitpick comments:
In
`@packages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.ts`:
- Around line 198-237: The test is a slow, real-git reliability test exercising
the stale-snapshot reconciliation (stale write guard) and should be moved to a
much narrower seam that fakes Git and exercises only the merger/store
interaction: copy or re-create this scenario as a fast unit test that does not
use hasGit or real repo setup but instead uses the in-memory store and a mocked
syncGroupPr to simulate the race; call stageMergeBranch (or skip it) and invoke
aiMergeTask with the mocked syncGroupPr and onGroupPrSyncSettled to drive the
code path, then assert on store.getBranchGroup(group.id).prNumber/prState to
ensure the stale "merged" write is skipped—remove the hasGit guard and the
45_000 timeout from the new test and place it alongside other aiMergeTask/merger
unit tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ff16e50-2e02-4a6c-ba48-43c8c6641747
📒 Files selected for processing (7)
packages/cli/src/commands/__tests__/task-lifecycle.test.tspackages/cli/src/commands/task-lifecycle.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/__tests__/store-persistence.test.tspackages/core/src/store.tspackages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.tspackages/engine/src/merger.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/core/src/store.ts
- packages/engine/src/merger.ts
- packages/cli/src/commands/task-lifecycle.ts
Both sides bootstrapped CONCEPTS.md — kept main's broader structure (Missions, Merge lifecycle, CE sessions) and folded the branch-group vocabulary into its Merge-lifecycle cluster (enriched Shared branch group + Self-healing sweep; added Branch assignment mode, Landed, Group promotion). Union-merged core index/store imports; took main's AGENTS.md discoverability wording. Also adds execFile to the two child_process test mocks that only stubbed exec (the coordinator's argv-based git calls now require it).
- abandon (route + CLI) preserves prState 'none' for groups that never had a PR instead of falsely persisting 'closed'; regression tests both sides - stale-snapshot write guard extracted to syncGroupPrOnLanding and covered by a fast in-memory unit test (FN-5048); the slow real-git duplicate removed
Collaborator
Author
|
Re: CodeRabbit's nitpick — "Move this stale-snapshot regression to a narrower seam" (branch-group-pr-sync.test.ts) — addressed: the sync-on-landing body (open-PR gate → sync → stale-snapshot write guard → reconciliation persist) is now extracted as 🤖 Generated with Claude Code |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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__/mission-store.test.ts`:
- Around line 1944-1947: The test's in-memory TaskStore created by
createStoreWithTaskStore() leaves task.missionId set so the lookup still uses
the normal task→mission path; update the test to explicitly clear the missionId
on the TaskStore row created by ts (e.g., load the created task row via
TaskStore APIs and set missionId to null/undefined or call the appropriate
ts.update/remove field) before proceeding with the assertion so the
fallback-through-feature-linkage path is exercised; reference
createStoreWithTaskStore, TaskStore, ts and the task.missionId field when
locating where to clear it.
In `@packages/dashboard/src/__tests__/routes-branch-groups.test.ts`:
- Around line 344-347: The test currently uses updateBranchGroup with
toHaveBeenCalledWith which can miss later overwrites; update the assertion to
use toHaveBeenLastCalledWith("BG-AB", expect.objectContaining({ status:
"abandoned", prState: "none" })) and also add an assertion against the response
body like expect(res.body.group.prState).toBe("none") to ensure the final
persisted state is "none" (reference updateBranchGroup and
res.body.group.prState in the test).
🪄 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: 1e4e93c3-5101-4351-b69f-8f11658d8acf
📒 Files selected for processing (23)
CONCEPTS.mdpackages/cli/src/commands/__tests__/branch-group.test.tspackages/cli/src/commands/branch-group.tspackages/core/src/__tests__/branch-group-store.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/index.tspackages/core/src/mission-store.tspackages/core/src/store.tspackages/dashboard/app/api/legacy.tspackages/dashboard/src/__tests__/routes-branch-groups.test.tspackages/dashboard/src/__tests__/routes-planning.test.tspackages/dashboard/src/index.tspackages/dashboard/src/routes/register-branch-groups-routes.tspackages/dashboard/src/routes/register-planning-subtask-routes.tspackages/engine/src/__tests__/group-pr-sync-on-landing.test.tspackages/engine/src/__tests__/project-engine.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.tspackages/engine/src/__tests__/reliability-interactions/completion-fanout-x-self-healing.test.tspackages/engine/src/__tests__/self-healing.test.tspackages/engine/src/index.tspackages/engine/src/merger.tspackages/engine/src/project-engine.tspackages/engine/src/self-healing.ts
💤 Files with no reviewable changes (1)
- packages/engine/src/tests/reliability-interactions/branch-group-pr-sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/core/src/index.ts
- packages/engine/src/index.ts
- packages/dashboard/app/api/legacy.ts
- packages/engine/src/self-healing.ts
- packages/dashboard/src/routes/register-planning-subtask-routes.ts
- packages/core/src/tests/branch-group-store.test.ts
- packages/dashboard/src/tests/routes-planning.test.ts
- packages/dashboard/src/index.ts
- packages/core/src/store.ts
- packages/engine/src/tests/project-engine.test.ts
- packages/dashboard/src/routes/register-branch-groups-routes.ts
- packages/cli/src/commands/branch-group.ts
- packages/engine/src/project-engine.ts
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
🤖 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__/mission-store.test.ts`:
- Around line 1944-1947: The test's in-memory TaskStore created by
createStoreWithTaskStore() leaves task.missionId set so the lookup still uses
the normal task→mission path; update the test to explicitly clear the missionId
on the TaskStore row created by ts (e.g., load the created task row via
TaskStore APIs and set missionId to null/undefined or call the appropriate
ts.update/remove field) before proceeding with the assertion so the
fallback-through-feature-linkage path is exercised; reference
createStoreWithTaskStore, TaskStore, ts and the task.missionId field when
locating where to clear it.
In `@packages/dashboard/src/__tests__/routes-branch-groups.test.ts`:
- Around line 344-347: The test currently uses updateBranchGroup with
toHaveBeenCalledWith which can miss later overwrites; update the assertion to
use toHaveBeenLastCalledWith("BG-AB", expect.objectContaining({ status:
"abandoned", prState: "none" })) and also add an assertion against the response
body like expect(res.body.group.prState).toBe("none") to ensure the final
persisted state is "none" (reference updateBranchGroup and
res.body.group.prState in the test).
🪄 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: 1e4e93c3-5101-4351-b69f-8f11658d8acf
📒 Files selected for processing (23)
CONCEPTS.mdpackages/cli/src/commands/__tests__/branch-group.test.tspackages/cli/src/commands/branch-group.tspackages/core/src/__tests__/branch-group-store.test.tspackages/core/src/__tests__/mission-store.test.tspackages/core/src/index.tspackages/core/src/mission-store.tspackages/core/src/store.tspackages/dashboard/app/api/legacy.tspackages/dashboard/src/__tests__/routes-branch-groups.test.tspackages/dashboard/src/__tests__/routes-planning.test.tspackages/dashboard/src/index.tspackages/dashboard/src/routes/register-branch-groups-routes.tspackages/dashboard/src/routes/register-planning-subtask-routes.tspackages/engine/src/__tests__/group-pr-sync-on-landing.test.tspackages/engine/src/__tests__/project-engine.test.tspackages/engine/src/__tests__/reliability-interactions/branch-group-pr-sync.test.tspackages/engine/src/__tests__/reliability-interactions/completion-fanout-x-self-healing.test.tspackages/engine/src/__tests__/self-healing.test.tspackages/engine/src/index.tspackages/engine/src/merger.tspackages/engine/src/project-engine.tspackages/engine/src/self-healing.ts
💤 Files with no reviewable changes (1)
- packages/engine/src/tests/reliability-interactions/branch-group-pr-sync.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/core/src/index.ts
- packages/engine/src/index.ts
- packages/dashboard/app/api/legacy.ts
- packages/engine/src/self-healing.ts
- packages/dashboard/src/routes/register-planning-subtask-routes.ts
- packages/core/src/tests/branch-group-store.test.ts
- packages/dashboard/src/tests/routes-planning.test.ts
- packages/dashboard/src/index.ts
- packages/core/src/store.ts
- packages/engine/src/tests/project-engine.test.ts
- packages/dashboard/src/routes/register-branch-groups-routes.ts
- packages/cli/src/commands/branch-group.ts
- packages/engine/src/project-engine.ts
🛑 Comments failed to post (2)
packages/core/src/__tests__/mission-store.test.ts (1)
1944-1947:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winThis test never clears
missionIdon theTaskStorerow it created.
createStoreWithTaskStore()builds a separate in-memoryTaskStoreDB, but Line 2021 updates the outerdbfrombeforeEach. That leavestask.missionIdintact in theTaskStore, so this case still passes through the normal task→mission lookup and never exercises the fallback-through-feature-linkage path the test is meant to pin.Also applies to: 2021-2024
🤖 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__/mission-store.test.ts` around lines 1944 - 1947, The test's in-memory TaskStore created by createStoreWithTaskStore() leaves task.missionId set so the lookup still uses the normal task→mission path; update the test to explicitly clear the missionId on the TaskStore row created by ts (e.g., load the created task row via TaskStore APIs and set missionId to null/undefined or call the appropriate ts.update/remove field) before proceeding with the assertion so the fallback-through-feature-linkage path is exercised; reference createStoreWithTaskStore, TaskStore, ts and the task.missionId field when locating where to clear it.packages/dashboard/src/__tests__/routes-branch-groups.test.ts (1)
344-347:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert the final persisted state, not just any matching call.
toHaveBeenCalledWithwill still pass if the route first writesprState: "none"and then later overwrites it to"closed", which is the regression this test is trying to block. Please switch this totoHaveBeenLastCalledWith(...)and/or assertres.body.group.prState === "none"so the test fails on a later bad write.🤖 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/dashboard/src/__tests__/routes-branch-groups.test.ts` around lines 344 - 347, The test currently uses updateBranchGroup with toHaveBeenCalledWith which can miss later overwrites; update the assertion to use toHaveBeenLastCalledWith("BG-AB", expect.objectContaining({ status: "abandoned", prState: "none" })) and also add an assertion against the response body like expect(res.body.group.prState).toBe("none") to ensure the final persisted state is "none" (reference updateBranchGroup and res.body.group.prState in the test).
- needsPrRepair no longer short-circuited by the open-state guard: legacy fallback rows (finalized + prState open + prNumber null) now repair by creating the real PR on re-promotion; regression test added - no-PR abandon route test asserts last persisted call + response body - goal-provenance fallback test clears missionId on its own in-memory store so the feature-linkage path is genuinely exercised
Collaborator
Author
|
Re: CodeRabbit's round-4 inline comments (failed to post inline; both addressed):
🤖 Generated with Claude Code |
…y, TaskCard narrowing, e2e memo race - resolve execFile lazily via namespace import in coordinator/merger/ task-lifecycle so the repo's exec-only child_process test mocks load again (10+ engine suites failed at import); dashboard.test.ts mock gains execFile so the argv-based git probes hit the mock instead of spawning real git - TaskCard: capture optional branchContext.groupId into a const (narrowing doesn't survive into the onClick closure; app tsconfig caught it in CI) - planning e2e: bounded poll past the 2.5s listTasks startup memo that served a pre-landing snapshot on fast CI runs
…al-groupId semantics Two stale assertions still encoded the synthetic planning:<sessionId> groupId: shared mode without a group-capable store now stamps no groupId, and per-task-derived members never carry one.
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.
Summary
End-to-end completion of the branch-group → single managed PR flow: tasks created by planning or missions land on one shared group branch, and a completion-gated promotion creates exactly one GitHub PR that is kept in sync as members land, with terminal merged/closed reconciliation.
Plan:
docs/plans/2026-06-03-001-feat-branch-group-single-pr-flow-plan.md(all units U1–U8 delivered; R1–R10 met).What was broken before
planning:/mission:string intobranchContext.groupIdthat resolved nowhere — member enumeration, gating, and promotion were silently broken (U1)@fusion/core)prState:"open"without ever calling GitHub — no PR existed (U5)git log --grephit (U3 — now trailer/subject ownership-anchored)What's new
CreateGroupPrFn/SyncGroupPrFnseam (mirrorsprocessPullRequestMerge; engine never imports dashboard), wired at all three CLI engine-construction sitesprNumber+ open-PR reuse only) under a per-group promotion lockfn branch-group list|show|promote|abandon(aliasfn bg) — full agent-native parity with the dashboard controlsexecFileargv (no shell)Testing
/tmp/compound-engineering/ce-code-review/20260603-110057-ac653281/).shared-branch-group-entry-points.test.ts(per-task-derived branch derivation + new-task shared-group block).Known Residuals (accepted, P2/advisory)
prNumbercopy can make two group rows reference one PR (group-merge-coordinator.tsreuse branch)--no-ffintegration merge is never pushed → local/remote integration-branch divergence after a squash-merged PRrecordBranchGroupMemberLandingside effects (audit + PR-body sync) — completion still correct via stamped mergeDetailsFN-branch-groupinstead of a numericFN-XXXXscope — fix in the squash-merge titlemergeDetailsnow read incomplete (intentional, no migration);syncGroupPrruns on the merge path (fire-and-forget refactor possible); multi-node promotion lease deferred (FN-4820); missingFusion-Task-Idtrailers; abandon response shape differs from promote.Post-Deploy Monitoring & Validation
merge:branch-group-routed(healthy routing),merge:branch-group-pr-sync-failed(sync degradation — retryable),merge:branch-group-promotion-failed(new; was silent before),task:shared-group-member-default-target-rerouted(defensive guard — should stay at zero)prNumber/prUrl; PR body completion reaches N/N;prStatereachesmerged/closedactiveafter all members land, any shared member withmergeTargetBranch≠ its group branch (lost-work class — investigate immediately)🤖 Generated with Claude Code
Summary by CodeRabbit