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`).
|
||||
- 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`.
|
||||
- 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
|
||||
6. **archived** — preserved history, optionally cleaned from filesystem
|
||||
|
||||
|
||||
@@ -35,6 +35,7 @@ type MockTask = {
|
||||
mergeRetries: number;
|
||||
status: string | null;
|
||||
error: string | null;
|
||||
mergeDetails?: { mergeConfirmed?: boolean } | null;
|
||||
verificationFailureCount?: number;
|
||||
mergeConflictBounceCount?: number;
|
||||
branch?: string;
|
||||
@@ -583,6 +584,35 @@ describe("ProjectEngine merge error recovery", () => {
|
||||
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 () => {
|
||||
const store = makeStore({
|
||||
updateTask: vi.fn(async () => {
|
||||
|
||||
@@ -2244,6 +2244,42 @@ describe("SelfHealingManager", () => {
|
||||
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 () => {
|
||||
const managerWithRecovery = new SelfHealingManager(store, {
|
||||
rootDir: "/tmp/test-project",
|
||||
|
||||
@@ -80,6 +80,11 @@ function formatErrorDetails(error: unknown): { message: string; detail: string }
|
||||
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 {
|
||||
status: "not-initialized" | "initializing" | "ready" | "degraded";
|
||||
message: string;
|
||||
@@ -1265,7 +1270,24 @@ export class ProjectEngine {
|
||||
"Merge already confirmed; completing task (recovered from post-merge state inconsistency)",
|
||||
);
|
||||
await store.updateTask(taskId, { status: null });
|
||||
await store.moveTask(taskId, "done");
|
||||
try {
|
||||
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;
|
||||
}
|
||||
|
||||
|
||||
@@ -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 {
|
||||
// ── Auto-unpause state ──────────────────────────────────────────────
|
||||
private unpauseTimer: ReturnType<typeof setTimeout> | null = null;
|
||||
@@ -1181,6 +1186,7 @@ export class SelfHealingManager {
|
||||
t.status !== "merging-pr" &&
|
||||
Boolean(t.worktree) &&
|
||||
t.mergeDetails?.mergeConfirmed !== true &&
|
||||
!hasTerminalInvalidDoneTransition(t) &&
|
||||
// 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
|
||||
|
||||
Reference in New Issue
Block a user