Recover stranded approved AI merge commits - #1997
Conversation
|
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 (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughChangesAdds recovery for approved stranded AI merge clean-room commits in self-healing and standalone AI merge flows. Commits are matched to approval evidence, validated, landed safely, and finalized with coverage for task state, metadata, logging, and merge-agent reuse. AI merge recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant landOneRepo
participant recoverApprovedPreexistingAiMergeWorktree
participant landSquash
participant mergeAgent
landOneRepo->>recoverApprovedPreexistingAiMergeWorktree: inspect approved clean-room worktrees
recoverApprovedPreexistingAiMergeWorktree->>landSquash: land validated stranded commit
landOneRepo->>mergeAgent: skip merge-agent execution after recovery
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
Greptile SummaryThis PR recovers approved AI merge clean-room commits that were left stranded. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "fix(FN-1997): bind stranded AI merge rec..." | Re-trigger Greptile |
There was a problem hiding this comment.
Pull request overview
This PR extends the engine’s self-healing logic to recover “stranded” approved AI-merge clean-room commits (e.g., when the merger crashes after producing/reviewing a clean-room commit but before landing it). Instead of re-emitting merge handoff and potentially losing the already-approved work, it attempts to safely fast-forward the integration branch to the stranded commit and then finalize the task.
Changes:
- Add a completion-handoff-limbo recovery path that searches AI merge worktree candidates, verifies task ownership of the commit, and lands it via fast-forward (directly or via
advanceIntegrationBranchRef). - Finalize the task using the recovered commit metadata (
commitSha,landedFiles,mergeConfirmed) once the integration branch is safely advanced. - Add a regression test covering stranded clean-room recovery behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| packages/engine/src/self-healing.ts | Adds stranded approved AI-merge clean-room commit recovery and integration-branch advancement during completion-handoff-limbo self-healing. |
| packages/engine/src/tests/self-healing.test.ts | Adds a regression test asserting recovery lands the stranded commit and finalizes the task without re-emitting merge handoff. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/engine/src/__tests__/self-healing.test.ts (1)
10693-10772: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend coverage beyond the single ff-only landing path.
This validates only the happy path where
HEADis already on the integration branch (thegit merge --ff-onlybranch). The higher-risk surfaces ofrecoverApprovedStrandedAiMergeCommitare untested: theadvanceIntegrationBranchRefbranch (currentBranch !== integrationBranch) and, importantly, the safety-rejection guards — foreign ownership (!ownership.owned) and the non-ancestor case (!alreadyAncestor && !tipIsAncestor) that must not finalize. A regression test for this recovery should assert those refusal invariants, not just the successful land.As per path instructions: "Bug regression tests must assert the general invariant across all known surfaces and reproduce the original symptom, rather than testing only the 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/engine/src/__tests__/self-healing.test.ts` around lines 10693 - 10772, Expand the “stranded AI merge clean-room recovery” tests around recoverApprovedStrandedAiMergeCommit to cover advanceIntegrationBranchRef when currentBranch differs from integrationBranch, plus refusal cases for !ownership.owned and !alreadyAncestor && !tipIsAncestor. Assert rejected recoveries do not finalize the task: no moveTask, no completion update, and no success log; retain the existing successful ff-only landing assertions.Source: Path instructions
packages/engine/src/self-healing.ts (1)
9010-9136: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an FNXC comment documenting this recovery's requirement/decision.
Per coding guidelines, important requirements and technical decisions must carry an FNXC comment.
recoverApprovedStrandedAiMergeCommitintroduces a significant, safety-sensitive behavior (advancing the integration ref / fast-forwarding and finalizing an approved detached clean-room commit), yet unlike its peers in this file it has no FNXC annotation explaining the ownership/ancestry gating and the ff-only-vs-advanceIntegrationBranchRefsplit.As per coding guidelines: "Add FNXC comments for important requirements and technical decisions. Each comment must begin with
FNXC:Area-of-product, include a timestamp inyyyy-MM-dd-hh:mmformat, explain the relevant behavior or requirement concisely".🤖 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/self-healing.ts` around lines 9010 - 9136, Add an FNXC comment immediately above recoverApprovedStrandedAiMergeCommit documenting its safety-sensitive recovery behavior, including ownership and ancestry checks, approved-completed-step gating, and the ff-only versus advanceIntegrationBranchRef path. Begin with the required FNXC:Area-of-product prefix and include the current timestamp in yyyy-MM-dd-hh:mm format.Source: Coding guidelines
🤖 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__/self-healing.test.ts`:
- Around line 10693-10772: Expand the “stranded AI merge clean-room recovery”
tests around recoverApprovedStrandedAiMergeCommit to cover
advanceIntegrationBranchRef when currentBranch differs from integrationBranch,
plus refusal cases for !ownership.owned and !alreadyAncestor && !tipIsAncestor.
Assert rejected recoveries do not finalize the task: no moveTask, no completion
update, and no success log; retain the existing successful ff-only landing
assertions.
In `@packages/engine/src/self-healing.ts`:
- Around line 9010-9136: Add an FNXC comment immediately above
recoverApprovedStrandedAiMergeCommit documenting its safety-sensitive recovery
behavior, including ownership and ancestry checks, approved-completed-step
gating, and the ff-only versus advanceIntegrationBranchRef path. Begin with the
required FNXC:Area-of-product prefix and include the current timestamp in
yyyy-MM-dd-hh:mm format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2dcc8f22-a858-445c-a673-b32f03e7d80b
📒 Files selected for processing (2)
packages/engine/src/__tests__/self-healing.test.tspackages/engine/src/self-healing.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
packages/engine/src/merger-ai.ts (1)
693-695: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve this pruning-order invariant with an FNXC comment.
This ordering is an important recovery requirement and should use the repository’s timestamped
FNXC:AIMergeconvention.As per coding guidelines, important requirements and technical decisions require an up-to-date
FNXC:<Area-of-product> yyyy-MM-dd-hh:mm:comment.🤖 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/merger-ai.ts` around lines 693 - 695, The recovery-critical pruning order near the merger finalization logic lacks the required FNXC annotation. Add an up-to-date `FNXC:AIMerge yyyy-MM-dd-hh:mm:` comment directly above the existing clean-room squash recovery comment, documenting that the approved commit must be landed before pre-merge pruning can remove its only reference.Source: Coding guidelines
🤖 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/__tests__/merger-ai.test.ts`:
- Around line 280-310: Expand the regression test around runAiMerge to exercise
all supported clean-room roots: the modern resolved root, the legacy
.fusion/ai-merge root, and a direct tmpdir() root. Parameterize setup and
stranded worktree creation per root while preserving the existing assertions
that the commit lands on main, the result reports merged with the stranded SHA,
recovery is logged, and mergeAgent is not called.
In `@packages/engine/src/merger-ai.ts`:
- Around line 173-180: Reverse the condition ordering in the candidate recovery
logic: evaluate alreadyLanded using merge-base before requiring squashSha to
descend from tipSha, and skip or recover already-landed candidates accordingly.
Update both occurrences near the checks following getCommitTaskOwnership,
ensuring the mutually exclusive ancestor checks no longer make recovery
unreachable.
- Around line 162-163: Honor cancellation in the early task-loading and recovery
path before any checkout, recovery, or landing work begins. In the logic
surrounding ctx, store.getTask, and stashResolveAgent, inspect ctx.signal and
immediately abort when cancellation is requested, preserving the existing
user-paused semantics and matching the later abort check near the integration
flow. Apply the same guard to the related 181–192 path.
- Around line 134-138: The task-level check in taskHasApprovedAiMergeReview must
be replaced with approval data bound to the exact squash commit SHA. Persist the
approved SHA alongside the review result, and update candidate recovery logic to
require that the candidate’s SHA matches the recorded approval; add a regression
covering two owned candidates where only one is approved.
---
Nitpick comments:
In `@packages/engine/src/merger-ai.ts`:
- Around line 693-695: The recovery-critical pruning order near the merger
finalization logic lacks the required FNXC annotation. Add an up-to-date
`FNXC:AIMerge yyyy-MM-dd-hh:mm:` comment directly above the existing clean-room
squash recovery comment, documenting that the approved commit must be landed
before pre-merge pruning can remove its only reference.
🪄 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: 4b966c2f-f58f-4a63-8a98-e0ef487d4dd7
📒 Files selected for processing (2)
packages/engine/src/__tests__/merger-ai.test.tspackages/engine/src/merger-ai.ts
Review feedback follow-up (nitpicks from review bodies)
Addressed in 39aa6ca:
|
Summary
Why
A merger can crash or stop after producing/reviewing a clean-room commit but before landing it, leaving the task stuck in review and the commit only reachable from a detached temporary worktree. The existing watchdog clears stale
mergingstate and re-emits handoff, but loses the chance to land the already-approved commit.Tests
pnpm --filter @fusion/engine exec tsc --noEmit --pretty falsepnpm --filter @fusion/engine exec vitest run --silent=passed-only --reporter=dot --project=engine-default src/__tests__/self-healing.test.ts -t "stranded AI merge clean-room recovery"Note: full
pnpm --filter @fusion/engine testcurrently aborts with Node unmanaged file descriptor warnings/SIGABRT in this worktree; the targeted engine-default regression test passes.Summary by CodeRabbit