From 1747d6fc4365755e3f568165b30db5a77f8b6640 Mon Sep 17 00:00:00 2001 From: Fusion Date: Fri, 1 May 2026 02:05:59 -0700 Subject: [PATCH] feat(FN-3050): preserve failed review tasks during recovery to prevent loop The merge strengthens task recovery logic to prevent in-review tasks from entering merge-recovery loops and preserves failed review tasks during retry sweeps instead of incorrectly resetting them. It adds targeted test coverage for these edge cases in the self-healing and project engine modules. Fusion-Task-Id: FN-3050 --- docs/architecture.md | 2 + docs/task-management.md | 1 + .../src/__tests__/project-engine.test.ts | 29 +++++++++++ .../engine/src/__tests__/self-healing.test.ts | 52 ++++++++++++++++--- packages/engine/src/self-healing.ts | 7 +++ 5 files changed, 83 insertions(+), 8 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 285aa92e3..88feffa7b 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -329,6 +329,8 @@ See [Memory Plugin Contract](./memory-plugin-contract.md) for the full plan. - `GridlockDetector` (`gridlock-detector.ts`) — detects all-blocked todo pipelines and emits notification events (plus explicit clear signals when gridlock resolves) - `TransientErrorDetector` (`transient-error-detector.ts`) — retriable error classification - `SelfHealingManager` (`self-healing.ts`) — auto-unpause/maintenance recovery actions + - `recoverGhostReviewTasks()` is a fallback only for idle, non-terminal `in-review` states. Terminal/actionable states (notably `status: "failed"`) are preserved and **not** auto-kicked back to `todo`. + - `recoverMergeableReviewTasks()` only re-enqueues truly eligible tasks; retry-exhausted review tasks are skipped to avoid re-enqueue/no-op loops that keep refreshing `updatedAt`. - `UsageLimitPauser` (`usage-limit-detector.ts`) and `withRateLimitRetry` (`rate-limit-retry.ts`) ### Worktree and naming helpers diff --git a/docs/task-management.md b/docs/task-management.md index 9f9222f7e..bfe9726be 100644 --- a/docs/task-management.md +++ b/docs/task-management.md @@ -59,6 +59,7 @@ Fusion task columns: 2. **todo** — ready for scheduling 3. **in-progress** — executor active in isolated worktree 4. **in-review** — implementation complete; awaiting finalization + - If merge/finalization hits a terminal error, tasks can remain in `in-review` with `status: "failed"` for explicit follow-up. This state is intentionally preserved by recovery (not auto-bounced to `todo`). 5. **done** — merged/finalized 6. **archived** — preserved history, optionally cleaned from filesystem diff --git a/packages/engine/src/__tests__/project-engine.test.ts b/packages/engine/src/__tests__/project-engine.test.ts index fab679495..6e6564263 100644 --- a/packages/engine/src/__tests__/project-engine.test.ts +++ b/packages/engine/src/__tests__/project-engine.test.ts @@ -1205,6 +1205,35 @@ describe("ProjectEngine paused in-review auto-merge behavior", () => { await engine.stop(); }); + it("periodic merge sweep does not re-enqueue failed in-review tasks", async () => { + vi.useFakeTimers(); + const mockStore = createMockStore({ ...baseSettings, autoMerge: true, pollIntervalMs: 15_000 }); + mocks.currentStore = mockStore.store; + const engine = createEngine(); + const privateEngine = engine as unknown as { internalEnqueueMerge: (taskId: string) => void }; + const enqueueSpy = vi.spyOn(privateEngine, "internalEnqueueMerge"); + + await engine.start(); + enqueueSpy.mockClear(); + + mockStore.store.listTasks.mockResolvedValueOnce([ + // Retry exhausted + failed (FN-2997 observed state after merge error) + { id: "FN-failed", column: "in-review", paused: false, mergeRetries: 3, status: "failed", updatedAt: new Date(Date.now() - 60 * 60_000).toISOString() }, + // Failed status must block even when retries are below the cap. + { id: "FN-failed-low-retries", column: "in-review", paused: false, mergeRetries: 0, status: "failed", updatedAt: new Date().toISOString() }, + { id: "FN-ready", column: "in-review", paused: false, mergeRetries: 0, status: null, updatedAt: new Date().toISOString() }, + ]); + + await vi.advanceTimersByTimeAsync(15_000); + + expect(enqueueSpy).toHaveBeenCalledWith("FN-ready"); + expect(enqueueSpy).not.toHaveBeenCalledWith("FN-failed"); + expect(enqueueSpy).not.toHaveBeenCalledWith("FN-failed-low-retries"); + + await engine.stop(); + vi.useRealTimers(); + }); + it("engine unpause sweep does not enqueue paused in-review tasks", async () => { const mockStore = createMockStore({ ...baseSettings, autoMerge: true }); mocks.currentStore = mockStore.store; diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index 6b52fa982..57ba01b28 100644 --- a/packages/engine/src/__tests__/self-healing.test.ts +++ b/packages/engine/src/__tests__/self-healing.test.ts @@ -1694,6 +1694,46 @@ describe("SelfHealingManager", () => { managerWithRecovery.stop(); }); + it("does not re-enqueue retry-exhausted review tasks", async () => { + const enqueueMerge = vi.fn(); + const managerWithRecovery = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + enqueueMerge, + }); + (store.getSettings as ReturnType).mockResolvedValue({ + autoMerge: true, + globalPause: false, + enginePaused: false, + }); + + (store.listTasks as ReturnType).mockResolvedValue([ + { + id: "FN-2997", + column: "in-review", + paused: false, + status: null, + error: null, + mergeRetries: 3, + worktree: "/tmp/test-project/.worktrees/fn-2997", + steps: [{ name: "Ship it", status: "done" }], + workflowStepResults: [{ id: "ws-1", status: "passed", phase: "pre-merge" }], + mergeDetails: undefined, + log: [], + }, + ]); + + const result = await managerWithRecovery.recoverMergeableReviewTasks(); + + expect(result).toBe(0); + expect(enqueueMerge).not.toHaveBeenCalled(); + expect(store.logEntry).not.toHaveBeenCalledWith( + "FN-2997", + expect.stringContaining("re-enqueued for merge"), + ); + + managerWithRecovery.stop(); + }); + it("moves stale in-review tasks with incomplete steps back to todo for retry", async () => { const managerWithRecovery = new SelfHealingManager(store, { rootDir: "/tmp/test-project", @@ -2051,7 +2091,7 @@ describe("SelfHealingManager", () => { }); describe("recoverGhostReviewTasks", () => { - it("kicks idle in-review tasks back to todo regardless of status or worktree", async () => { + it("preserves failed in-review tasks so actionable merge failures are not ghost-retried", async () => { const managerWithRecovery = new SelfHealingManager(store, { rootDir: "/tmp/test-project", }); @@ -2075,13 +2115,9 @@ describe("SelfHealingManager", () => { const result = await managerWithRecovery.recoverGhostReviewTasks(); - expect(result).toBe(1); - expect(store.updateTask).toHaveBeenCalledWith("FN-9001", { status: null, error: null }); - expect(store.moveTask).toHaveBeenCalledWith("FN-9001", "todo"); - expect(store.logEntry).toHaveBeenCalledWith( - "FN-9001", - expect.stringContaining("idle past stuck-task timeout"), - ); + expect(result).toBe(0); + expect(store.updateTask).not.toHaveBeenCalled(); + expect(store.moveTask).not.toHaveBeenCalled(); managerWithRecovery.stop(); }); diff --git a/packages/engine/src/self-healing.ts b/packages/engine/src/self-healing.ts index 9b5a66c63..366308c41 100644 --- a/packages/engine/src/self-healing.ts +++ b/packages/engine/src/self-healing.ts @@ -84,6 +84,7 @@ const NON_TERMINAL_STEP_STATUSES = new Set(["pending", "in-progress"]); /** Statuses that represent an explicit human-handoff or active merge — * the ghost-review fallback must not disturb tasks parked in these states. */ const GHOST_REVIEW_PRESERVED_STATUSES = new Set([ + "failed", "awaiting-user-review", "awaiting-approval", "merging", @@ -103,6 +104,7 @@ const ORPHANED_WITH_WORKTREE_GRACE_MS = 300_000; * forever; when exhausted the task stays in `in-review` for human inspection. */ const MAX_TASK_DONE_RETRIES = 3; +const MAX_AUTO_MERGE_RETRIES = 3; interface LandedTaskCommit { sha: string; @@ -803,6 +805,11 @@ export class SelfHealingManager { !t.paused && Boolean(t.worktree) && t.mergeDetails?.mergeConfirmed !== true && + // Mirror ProjectEngine.canMergeTask retry gate. If retries are already + // exhausted, re-enqueueing here is a no-op and each recovery log write + // refreshes updatedAt, preventing cooldown-based retries from ever + // becoming eligible. + (t.mergeRetries ?? 0) < MAX_AUTO_MERGE_RETRIES && getTaskMergeBlocker(t) === undefined, );