feat(FN-3951): fix merge recovery re-enqueue guard to prevent auto-merge lo
Hardens merge recovery logic (FN-3951) with guards against invalid state transitions during terminal merge errors, backed by new tests in `merge-error-recovery.test.ts` and `self-healing.test.ts`, and documented in the task management docs. Fusion-Task-Id: FN-3951
This commit is contained in:
12
.changeset/fn-3951-auto-merge-loop.md
Normal file
12
.changeset/fn-3951-auto-merge-loop.md
Normal file
@@ -0,0 +1,12 @@
|
|||||||
|
---
|
||||||
|
"@runfusion/fusion": patch
|
||||||
|
---
|
||||||
|
|
||||||
|
Prevent auto-merge loops on terminal invalid done-transition failures during merge recovery.
|
||||||
|
|
||||||
|
When merge finalization encounters a non-recoverable state-machine error like
|
||||||
|
`Invalid transition: 'todo' → 'done'`, auto-recovery now keeps that task parked
|
||||||
|
in a stable failed review state instead of repeatedly re-enqueuing it for merge.
|
||||||
|
|
||||||
|
The merge-confirmed fast path also now re-checks task ownership and skips
|
||||||
|
finalization if the task has already left `in-review`.
|
||||||
@@ -94,6 +94,7 @@ Fusion task columns:
|
|||||||
- 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`).
|
- 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`).
|
||||||
- Retry behavior splits by step completion: `in-review` tasks with incomplete steps (`pending`/`in-progress`) are treated as execution failures and retried back to `todo` with `preserveProgress: true`; `in-review` tasks with all steps `done` are treated as merge/finalization failures and stay in `in-review` with merge retry state reset.
|
- Retry behavior splits by step completion: `in-review` tasks with incomplete steps (`pending`/`in-progress`) are treated as execution failures and retried back to `todo` with `preserveProgress: true`; `in-review` tasks with all steps `done` are treated as merge/finalization failures and stay in `in-review` with merge retry state reset.
|
||||||
- Self-healing can still auto-finalize retry-exhausted failed review tasks when it can prove their branch content already landed on the merge target, so already-merged work does not deadlock in `in-review`.
|
- Self-healing can still auto-finalize retry-exhausted failed review tasks when it can prove their branch content already landed on the merge target, so already-merged work does not deadlock in `in-review`.
|
||||||
|
- Non-recoverable state-machine errors during finalization (for example `Invalid transition: 'todo' → 'done'`) are treated as terminal review failures: recovery must not re-enqueue these tasks for merge unless task state changes prove they are recoverable.
|
||||||
5. **done** — merged/finalized
|
5. **done** — merged/finalized
|
||||||
6. **archived** — preserved history, optionally cleaned from filesystem
|
6. **archived** — preserved history, optionally cleaned from filesystem
|
||||||
|
|
||||||
|
|||||||
@@ -35,6 +35,7 @@ type MockTask = {
|
|||||||
mergeRetries: number;
|
mergeRetries: number;
|
||||||
status: string | null;
|
status: string | null;
|
||||||
error: string | null;
|
error: string | null;
|
||||||
|
mergeDetails?: { mergeConfirmed?: boolean } | null;
|
||||||
verificationFailureCount?: number;
|
verificationFailureCount?: number;
|
||||||
mergeConflictBounceCount?: number;
|
mergeConflictBounceCount?: number;
|
||||||
branch?: string;
|
branch?: string;
|
||||||
@@ -583,6 +584,35 @@ describe("ProjectEngine merge error recovery", () => {
|
|||||||
expect(hasErrorLog(errorSpy, "after non-conflict error")).toBe(false);
|
expect(hasErrorLog(errorSpy, "after non-conflict error")).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not park merge-confirmed tasks as failed when finalize loses in-review ownership", async () => {
|
||||||
|
const store = makeStore({
|
||||||
|
tasks: [
|
||||||
|
makeTask({
|
||||||
|
mergeDetails: { mergeConfirmed: true },
|
||||||
|
}),
|
||||||
|
makeTask({ column: "todo" }),
|
||||||
|
],
|
||||||
|
});
|
||||||
|
store.moveTask.mockRejectedValueOnce(
|
||||||
|
new Error("Invalid transition: 'todo' → 'done'. Valid targets: in-progress, triage"),
|
||||||
|
);
|
||||||
|
|
||||||
|
const engine = createEngine(store);
|
||||||
|
await runMergeCycle(engine);
|
||||||
|
|
||||||
|
expect(store.moveTask).toHaveBeenCalledWith(TASK_ID, "done");
|
||||||
|
expect(store.updateTask).toHaveBeenCalledWith(TASK_ID, { status: null });
|
||||||
|
expect(store.updateTask).not.toHaveBeenCalledWith(TASK_ID, {
|
||||||
|
status: "failed",
|
||||||
|
mergeRetries: 3,
|
||||||
|
error: expect.stringContaining("Invalid transition"),
|
||||||
|
});
|
||||||
|
expect(store.logEntry).toHaveBeenCalledWith(
|
||||||
|
TASK_ID,
|
||||||
|
expect.stringContaining("finalize skipped"),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("logs when non-conflict direct merge error recovery update fails", async () => {
|
it("logs when non-conflict direct merge error recovery update fails", async () => {
|
||||||
const store = makeStore({
|
const store = makeStore({
|
||||||
updateTask: vi.fn(async () => {
|
updateTask: vi.fn(async () => {
|
||||||
|
|||||||
@@ -2244,6 +2244,42 @@ describe("SelfHealingManager", () => {
|
|||||||
managerWithRecovery.stop();
|
managerWithRecovery.stop();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not re-enqueue review tasks carrying terminal invalid done-transition errors", async () => {
|
||||||
|
const enqueueMerge = vi.fn();
|
||||||
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
|
rootDir: "/tmp/test-project",
|
||||||
|
enqueueMerge,
|
||||||
|
});
|
||||||
|
(store.getSettings as ReturnType<typeof vi.fn>).mockResolvedValue({
|
||||||
|
autoMerge: true,
|
||||||
|
globalPause: false,
|
||||||
|
enginePaused: false,
|
||||||
|
});
|
||||||
|
|
||||||
|
(store.listTasks as ReturnType<typeof vi.fn>).mockResolvedValue([
|
||||||
|
{
|
||||||
|
id: "FN-3946",
|
||||||
|
column: "in-review",
|
||||||
|
paused: false,
|
||||||
|
status: null,
|
||||||
|
error: "Invalid transition: 'todo' → 'done'. Valid targets: in-progress, triage",
|
||||||
|
mergeRetries: 0,
|
||||||
|
worktree: "/tmp/test-project/.worktrees/fn-3946",
|
||||||
|
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.mergeTask).not.toHaveBeenCalled();
|
||||||
|
managerWithRecovery.stop();
|
||||||
|
});
|
||||||
|
|
||||||
it("moves stale in-review tasks with incomplete steps back to todo for retry", async () => {
|
it("moves stale in-review tasks with incomplete steps back to todo for retry", async () => {
|
||||||
const managerWithRecovery = new SelfHealingManager(store, {
|
const managerWithRecovery = new SelfHealingManager(store, {
|
||||||
rootDir: "/tmp/test-project",
|
rootDir: "/tmp/test-project",
|
||||||
|
|||||||
@@ -80,6 +80,11 @@ function formatErrorDetails(error: unknown): { message: string; detail: string }
|
|||||||
return { message: detail, detail };
|
return { message: detail, detail };
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function isInvalidDoneTransitionError(error: unknown): boolean {
|
||||||
|
const message = error instanceof Error ? error.message : String(error);
|
||||||
|
return message.includes("Invalid transition:") && message.includes("→ 'done'");
|
||||||
|
}
|
||||||
|
|
||||||
export interface AutomationSubsystemHealth {
|
export interface AutomationSubsystemHealth {
|
||||||
status: "not-initialized" | "initializing" | "ready" | "degraded";
|
status: "not-initialized" | "initializing" | "ready" | "degraded";
|
||||||
message: string;
|
message: string;
|
||||||
@@ -1265,7 +1270,24 @@ export class ProjectEngine {
|
|||||||
"Merge already confirmed; completing task (recovered from post-merge state inconsistency)",
|
"Merge already confirmed; completing task (recovered from post-merge state inconsistency)",
|
||||||
);
|
);
|
||||||
await store.updateTask(taskId, { status: null });
|
await store.updateTask(taskId, { status: null });
|
||||||
|
try {
|
||||||
await store.moveTask(taskId, "done");
|
await store.moveTask(taskId, "done");
|
||||||
|
} catch (error) {
|
||||||
|
if (isInvalidDoneTransitionError(error)) {
|
||||||
|
const latest = await store.getTask(taskId).catch(() => null);
|
||||||
|
if (latest && latest.column !== "in-review") {
|
||||||
|
runtimeLog.warn(
|
||||||
|
`Auto-merge: ${taskId} merge-confirmed finalize skipped — task moved to ${latest.column} before done transition`,
|
||||||
|
);
|
||||||
|
await store.logEntry(
|
||||||
|
taskId,
|
||||||
|
`Merge confirmed finalize skipped: task moved to '${latest.column}' before in-review → done transition`,
|
||||||
|
);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
throw error;
|
||||||
|
}
|
||||||
continue;
|
continue;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -168,6 +168,11 @@ function parseShortstat(output: string): Pick<LandedTaskCommit, "filesChanged" |
|
|||||||
};
|
};
|
||||||
}
|
}
|
||||||
|
|
||||||
|
function hasTerminalInvalidDoneTransition(task: Pick<Task, "error">): boolean {
|
||||||
|
const error = task.error ?? "";
|
||||||
|
return error.includes("Invalid transition:") && error.includes("→ 'done'");
|
||||||
|
}
|
||||||
|
|
||||||
export class SelfHealingManager {
|
export class SelfHealingManager {
|
||||||
// ── Auto-unpause state ──────────────────────────────────────────────
|
// ── Auto-unpause state ──────────────────────────────────────────────
|
||||||
private unpauseTimer: ReturnType<typeof setTimeout> | null = null;
|
private unpauseTimer: ReturnType<typeof setTimeout> | null = null;
|
||||||
@@ -1181,6 +1186,7 @@ export class SelfHealingManager {
|
|||||||
t.status !== "merging-pr" &&
|
t.status !== "merging-pr" &&
|
||||||
Boolean(t.worktree) &&
|
Boolean(t.worktree) &&
|
||||||
t.mergeDetails?.mergeConfirmed !== true &&
|
t.mergeDetails?.mergeConfirmed !== true &&
|
||||||
|
!hasTerminalInvalidDoneTransition(t) &&
|
||||||
// Mirror ProjectEngine.canMergeTask retry gate. If retries are already
|
// Mirror ProjectEngine.canMergeTask retry gate. If retries are already
|
||||||
// exhausted, re-enqueueing here is a no-op and each recovery log write
|
// exhausted, re-enqueueing here is a no-op and each recovery log write
|
||||||
// refreshes updatedAt, preventing cooldown-based retries from ever
|
// refreshes updatedAt, preventing cooldown-based retries from ever
|
||||||
|
|||||||
Reference in New Issue
Block a user