fix: classify harmless merge worktree cleanup failures - #1663
Merged
gsxdsm merged 7 commits intoJun 13, 2026
Conversation
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughClassifies certain temporary post-merge ChangesHarmless Merge Worktree Cleanup Classification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds a “harmless” classification path for temporary merge/post-merge worktree removal failures by pruning + inspecting Git worktree porcelain output, so merger cleanup can proceed when no registered worktree leak remains.
Changes:
- Introduce
WorktreeRemoveOutcomeand a classifier that runsgit worktree prune+git worktree list --porcelainto distinguish harmless temp cleanup failures from real leaks. - Update merger cleanup callers to consume the new outcome and log a targeted warning for harmless residue.
- Extend audit event types, add unit/reliability tests, and document the new diagnostic behavior.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/engine/src/worktree-backend.ts | Adds outcome type + harmless failure classifier; updates removeWorktree to return outcomes and audit new classifications. |
| packages/engine/src/run-audit.ts | Extends GitMutationType with new audit event strings for classified outcomes. |
| packages/engine/src/merger.ts | Consumes removeWorktree outcome to warn on harmless classified failures and set worktreeRemoved accurately. |
| packages/engine/src/tests/worktree-backend.test.ts | Adds unit tests covering harmless classification, non-temp behavior, and still-registered leak behavior. |
| packages/engine/src/tests/reliability-interactions/worktrunk-worktree-removal.test.ts | Adds integration-style tests verifying merger logging + command sequence around prune/porcelain classification. |
| docs/diagnostics.md | Documents the new merge temp worktree cleanup classification and operator verification steps. |
| .changeset/fn-343-merge-worktree-cleanup.md | Declares the change as an @fusion/engine patch release. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+98
to
+112
| await execAsync("git worktree prune", { | ||
| cwd: input.rootDir, | ||
| encoding: "utf-8", | ||
| timeout: NATIVE_TIMEOUT_MS, | ||
| maxBuffer: MAX_BUFFER, | ||
| }); | ||
|
|
||
| const listResult = await execAsync("git worktree list --porcelain", { | ||
| cwd: input.rootDir, | ||
| encoding: "utf-8", | ||
| timeout: 10_000, | ||
| maxBuffer: MAX_BUFFER, | ||
| }); | ||
| const stdout = typeof listResult === "string" ? listResult : String(listResult.stdout ?? ""); | ||
| const registeredAfterPrune = porcelainContainsWorktree(stdout, input.worktreePath); |
| processActiveProbe?: ProcessActiveProbe; | ||
| reconcileMinIdleMs?: number; | ||
| }): Promise<void> { | ||
| }): Promise<void | WorktreeRemoveOutcome> { |
Comment on lines
+7325
to
+7327
| if (outcome && "harmless" in outcome && outcome.harmless) { | ||
| mergerLog.warn(`${taskId}: post-merge worktree cleanup remove failed, but no registered worktree remains after prune for ${postMergeWorktree}: ${outcome.message}`); | ||
| } |
Contributor
Greptile SummaryThis PR introduces a
Confidence Score: 5/5Safe to merge — the previously flagged gaps (missing audit parameter in the post-merge path, stale task-store worktree pointer, void return type) are all resolved, and the new classification logic is well-guarded by both reason and path-prefix checks. The harmless classification is limited to two specific path prefixes and two merger-only removal reasons; any probe failure or still-registered worktree causes the original error to be rethrown, preserving the pre-existing failure behaviour. The dashboard round-trip fix is narrow and fully covered by the new test. No new code paths introduce data loss or incorrect state transitions. packages/engine/src/worktree-backend.ts carries a pre-existing dead branch in porcelainContainsWorktree (the privateTarget comparison) that was identified in an earlier review cycle but not yet removed. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[git worktree remove --force path] -->|success| B[return removed:true]
A -->|error| C{isMergeTempCleanupCandidate?<br/>reason=MergerCleanup/PostMerge<br/>AND path starts with fusion-ai-merge- or post-merge-<br/>AND error matches patterns}
C -->|no| D{WorktrunkOperationError<br/>+ fallback-native configured?}
D -->|no| E[throw original error]
D -->|yes| F[native.remove]
F -->|success| G[return removed:true]
F -->|error| H{classifyHarmless<br/>for native error}
H -->|harmless| I[return removed:false harmless:true]
H -->|not harmless| J[throw native error]
C -->|yes| K[git worktree prune]
K -->|prune fails| L[emit probe-failed audit<br/>return null → throw original]
K -->|success| M[git worktree list --porcelain]
M --> N{path still registered?}
N -->|yes| O[emit leaked-registered-worktree audit<br/>return null → throw original]
N -->|no| P[emit classified-harmless audit<br/>return removed:false harmless:true]
Reviews (5): Last reviewed commit: "fix(FN-343): address follow-up review fi..." | 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: 1
🧹 Nitpick comments (1)
packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts (1)
62-77: ⚡ Quick winConsider extracting the shared mock setup to reduce duplication.
The mock implementations in both new tests are nearly identical (16 lines each), differing only in the
git worktree list --porcelainoutput. Extracting a helper function would improve maintainability and make it easier to add similar tests in the future.♻️ Example refactor to extract shared setup
function mockWorktreeRemoveFailure(postMergePath: string, porcelainOutput: string) { execSpy.mockImplementation((cmd: string, _opts: unknown, cb: (err: any, stdout: string, stderr: string) => void) => { if (cmd.includes("git worktree remove")) { const stderr = `fatal: validation failed, cannot remove working tree: '${postMergePath}/.git' is not a .git file, error code 2`; cb(Object.assign(new Error(stderr), { stderr, status: 2 }), "", stderr); return; } if (cmd === "git worktree prune") { cb(null, "", ""); return; } if (cmd === "git worktree list --porcelain") { cb(null, porcelainOutput, ""); return; } cb(null, "", ""); }); }Then use it in tests:
- execSpy.mockImplementation((cmd: string, _opts: unknown, cb: (err: any, stdout: string, stderr: string) => void) => { - if (cmd.includes("git worktree remove")) { - const stderr = `fatal: validation failed, cannot remove working tree: '${postMergePath}/.git' is not a .git file, error code 2`; - cb(Object.assign(new Error(stderr), { stderr, status: 2 }), "", stderr); - return; - } - if (cmd === "git worktree prune") { - cb(null, "", ""); - return; - } - if (cmd === "git worktree list --porcelain") { - cb(null, "worktree /repo\nbranch refs/heads/main\n", ""); - return; - } - cb(null, "", ""); - }); + mockWorktreeRemoveFailure(postMergePath, "worktree /repo\nbranch refs/heads/main\n");Also applies to: 94-109
🤖 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/worktrunk-worktree-removal.test.ts` around lines 62 - 77, Extract the duplicated execSpy.mockImplementation into a helper (e.g., mockWorktreeRemoveFailure) that takes postMergePath and porcelainOutput parameters and installs the shared mock behavior for "git worktree remove", "git worktree prune", and "git worktree list --porcelain"; then replace the duplicated mockImplementation blocks in the tests with calls to this helper (pass the file's postMergePath variable and the differing porcelainOutput), keeping execSpy and existing error/stderr/status behavior unchanged.
🤖 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/worktree-backend.ts`:
- Around line 98-110: The current probe commands ("git worktree prune" and "git
worktree list --porcelain") can throw inside classifyHarmlessMergeRemoveFailure
and overwrite the original git worktree remove failure; change the logic around
classifyHarmlessMergeRemoveFailure in worktree-backend.ts so any errors from
prune/list are caught and used only to annotate/log/return a diagnostic but do
not replace the original remove error—i.e., call
classifyHarmlessMergeRemoveFailure with the probe results inside a try/catch,
capture its classification or warning text, attach it to or log it alongside the
original git worktree remove error (or rethrow the original error with the
classification appended) instead of throwing the probe error itself; reference
classifyHarmlessMergeRemoveFailure, the probe calls for "git worktree prune" and
"git worktree list --porcelain", and follow the downstream pattern used in
merger.ts where callers surface the caught error/warning text.
---
Nitpick comments:
In
`@packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts`:
- Around line 62-77: Extract the duplicated execSpy.mockImplementation into a
helper (e.g., mockWorktreeRemoveFailure) that takes postMergePath and
porcelainOutput parameters and installs the shared mock behavior for "git
worktree remove", "git worktree prune", and "git worktree list --porcelain";
then replace the duplicated mockImplementation blocks in the tests with calls to
this helper (pass the file's postMergePath variable and the differing
porcelainOutput), keeping execSpy and existing error/stderr/status behavior
unchanged.
🪄 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: 30f24112-28e4-425e-8838-186e85b765e1
📒 Files selected for processing (7)
.changeset/fn-343-merge-worktree-cleanup.mddocs/diagnostics.mdpackages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.tspackages/engine/src/__tests__/worktree-backend.test.tspackages/engine/src/merger.tspackages/engine/src/run-audit.tspackages/engine/src/worktree-backend.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/dashboard/app/components/workflow-flow-mapping.ts`:
- Around line 139-149: The aliasing in workflow-flow-mapping.ts collapses
distinct IR node kinds (e.g., "merge-gate", "merge-attempt",
"branch-group-member-integration", "branch-group-promotion", "recovery-router",
"manual-merge-hold", "retry-backoff") into editor render kinds
("merge"/"hold"/"loop"), but flowToIr and toIrNode later serialize from
data.kind and will lose the original IR kind; to fix, preserve the original IR
kind on the flow node data when you map to editor render kinds (e.g., add a
stable property like irKind or originalKind to the node data in the mapping
function that returns "merge"/"hold"/"loop"), and update toIrNode (and/or
flowToIr) to prefer that irKind/originalKind when reconstructing IR nodes so the
persisted workflow retains the original semantic kinds instead of the editor
aliases.
🪄 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: deb2400a-b159-47ae-b020-0e4b6c2e7de7
📒 Files selected for processing (1)
packages/dashboard/app/components/workflow-flow-mapping.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dashboard/app/components/workflow-flow-mapping.ts (1)
149-149:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
retry-backofftemplates lost on round-trip due to mismatched expand/reassemble logic.The
retry-backoff→"loop"editor mapping (line 149) causes template data loss:
loopConfigOf(line 199) only recognizeskind === "loop", soretry-backofftemplates aren't expanded to child nodes inirToFlow- In
toIrNode, thedata.kind === "loop"check (line 408) reassembles templates from (empty) children- Line 424 overwrites the original template with the empty reassembled one
A
retry-backoffnode withconfig: { template: { nodes: [A,B], edges: [...] } }round-trips back withtemplate: { nodes: [], edges: [] }.🛠️ Suggested fix: skip template reassembly for aliased kinds
if (data.kind === "foreach" || data.kind === "loop") { + // Aliased kinds (e.g. retry-backoff) weren't expanded, so don't reassemble their templates + const shouldReassembleTemplate = !originalKind || originalKind === "foreach" || originalKind === "loop"; + if (!shouldReassembleTemplate) { + return { + id: localId, + kind: originalKind, + config: config && Object.keys(config).length ? config : undefined, + }; + } // Reassemble the template from this group's children. const children = childrenByGroup.get(node.id) ?? [];Also applies to: 408-425
🤖 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/workflow-flow-mapping.ts` at line 149, The mapping that aliases node.kind "retry-backoff" to "loop" causes template loss because loopConfigOf and toIrNode only treat literal "loop"; fix by preserving the original kind when aliasing and skipping template reassembly for aliased kinds: when you map "retry-backoff" -> "loop" in the editor mapping (the code that returns "loop" for node.kind === "retry-backoff"), attach a small marker (e.g., originalKind or __aliasedKind) to the node or its data so downstream functions can see it, then update toIrNode (the branch that runs when data.kind === "loop" and reassembles template from children) to check that marker and, if originalKind exists and is not "loop" (i.e., it was an alias like "retry-backoff"), skip reconstructing/overwriting the template (or alternatively have loopConfigOf accept the aliased kind); this preserves the original template payload across irToFlow -> toIrNode round-trips.
🧹 Nitpick comments (1)
packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts (1)
201-228: ⚡ Quick winConsider adding config preservation assertion for comprehensive coverage.
The test correctly verifies kind round-trip, but doesn't assert that node configs (especially the
retry-backofftemplate) are preserved. Given the aliasing complexity, adding config checks would catch data loss regressions.💡 Suggested enhancement
expect(byId.retry.kind).toBe("retry-backoff"); + // Verify configs survive round-trip + expect(byId.gate.config?.name).toBe("Gate"); + expect(byId.hold.config?.release).toBe("manual"); + expect(byId.retry.config).toMatchObject({ maxIterations: 2, template: { nodes: [], edges: [] } }); });For stronger coverage, consider a separate test with a non-empty
retry-backofftemplate to ensure template content isn't lost.🤖 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/__tests__/workflow-flow-mapping.test.ts` around lines 201 - 228, The test currently verifies kind round-tripping but misses asserting that node configs (especially the retry-backoff template) are preserved; update this test (or add a new one) to include a non-empty template in the `retry` node and assert after round-trip (via irToFlow and flowToIr) that the output IR `byId.retry.config` (and specifically `byId.retry.config.template`) matches the original template content; reference the existing test flow using irToFlow, flowToIr, and the byId lookup to add these config/template assertions for nodes with kind "retry-backoff" (and any other nodes with config like "manual-merge-hold") so we catch config loss during aliasing conversions.
🤖 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.
Outside diff comments:
In `@packages/dashboard/app/components/workflow-flow-mapping.ts`:
- Line 149: The mapping that aliases node.kind "retry-backoff" to "loop" causes
template loss because loopConfigOf and toIrNode only treat literal "loop"; fix
by preserving the original kind when aliasing and skipping template reassembly
for aliased kinds: when you map "retry-backoff" -> "loop" in the editor mapping
(the code that returns "loop" for node.kind === "retry-backoff"), attach a small
marker (e.g., originalKind or __aliasedKind) to the node or its data so
downstream functions can see it, then update toIrNode (the branch that runs when
data.kind === "loop" and reassembles template from children) to check that
marker and, if originalKind exists and is not "loop" (i.e., it was an alias like
"retry-backoff"), skip reconstructing/overwriting the template (or alternatively
have loopConfigOf accept the aliased kind); this preserves the original template
payload across irToFlow -> toIrNode round-trips.
---
Nitpick comments:
In `@packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.ts`:
- Around line 201-228: The test currently verifies kind round-tripping but
misses asserting that node configs (especially the retry-backoff template) are
preserved; update this test (or add a new one) to include a non-empty template
in the `retry` node and assert after round-trip (via irToFlow and flowToIr) that
the output IR `byId.retry.config` (and specifically
`byId.retry.config.template`) matches the original template content; reference
the existing test flow using irToFlow, flowToIr, and the byId lookup to add
these config/template assertions for nodes with kind "retry-backoff" (and any
other nodes with config like "manual-merge-hold") so we catch config loss during
aliasing conversions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7f1f3329-7472-473e-9e7b-2af1b9b6b49a
📒 Files selected for processing (6)
packages/dashboard/app/components/__tests__/workflow-flow-mapping.test.tspackages/dashboard/app/components/workflow-flow-mapping.tspackages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.tspackages/engine/src/__tests__/worktree-backend.test.tspackages/engine/src/run-audit.tspackages/engine/src/worktree-backend.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/engine/src/run-audit.ts
- packages/engine/src/tests/worktree-backend.test.ts
- packages/engine/src/worktree-backend.ts
Fusion-Task-Id: FN-343 Co-authored-by: Fusion <noreply@runfusion.ai>
Fusion-Task-Id: FN-343 Co-authored-by: Fusion <noreply@runfusion.ai>
Fusion-Task-Id: FN-343 Co-authored-by: Fusion <noreply@runfusion.ai>
…geset Fusion-Task-Id: FN-343 Co-authored-by: Fusion <noreply@runfusion.ai>
plarson
force-pushed
the
fix/fn-343-merge-worktree-cleanup
branch
from
June 12, 2026 22:51
e62889d to
02a8581
Compare
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
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
git worktree pruneplusgit worktree list --porcelainproves the path is no longer registeredmerge-gate,merge-attempt, etc.)Test Plan
corepack pnpm --filter @fusion/engine exec vitest run src/__tests__/worktree-backend.test.ts src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts --silent=passed-only --reporter=dot— 2 files / 60 tests passedcorepack pnpm --filter @fusion/engine typecheck— passedcorepack pnpm --filter @fusion/engine build— passedcorepack pnpm lint -- packages/engine/src/worktree-backend.ts packages/engine/src/merger.ts packages/engine/src/run-audit.ts packages/engine/src/__tests__/worktree-backend.test.ts packages/engine/src/__tests__/reliability-interactions/worktrunk-worktree-removal.test.ts— passed with existing test-file ignored warningscorepack pnpm typecheck— passed after adding the workflow-node type mapping fixcorepack pnpm build— passedCI
Summary by CodeRabbit
Documentation
Bug Fixes
Tests