diff --git a/.changeset/empty-finalize-skipped-step-guard.md b/.changeset/empty-finalize-skipped-step-guard.md new file mode 100644 index 0000000000..01c512d955 --- /dev/null +++ b/.changeset/empty-finalize-skipped-step-guard.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Block empty-diff task finalizes that skipped verification steps so reverted work can't reach done. +category: fix +dev: Generalizes evaluateNoCommitsNoOpFinalize (packages/core) to block any zero-diff finalize when a step is skipped — verification/QA/review-named skips block unconditionally, other skips block unless every non-skipped step is done AND the task is noCommitsExpected. Applies at all finalize lanes (merger-ai empty lane, merger.ts, self-healing stranded-todo promoter + no-op review finalize). Closes the FN-8141 laundering path. diff --git a/packages/core/src/__tests__/no-commits-finalize-guard.test.ts b/packages/core/src/__tests__/no-commits-finalize-guard.test.ts index 74693b201e..971c5c46cb 100644 --- a/packages/core/src/__tests__/no-commits-finalize-guard.test.ts +++ b/packages/core/src/__tests__/no-commits-finalize-guard.test.ts @@ -5,6 +5,10 @@ function steps(statuses: Array): TaskStep[] { return statuses.map((status, index) => ({ name: `Step ${index}`, status })); } +function namedSteps(entries: Array<[string, TaskStep["status"]]>): TaskStep[] { + return entries.map(([name, status]) => ({ name, status })); +} + describe("evaluateNoCommitsNoOpFinalize", () => { it("blocks the FN-6455 skipped-release shape", () => { const result = evaluateNoCommitsNoOpFinalize({ @@ -13,7 +17,6 @@ describe("evaluateNoCommitsNoOpFinalize", () => { }); expect(result).toMatchObject({ blocked: true, doneCount: 1, incompleteCount: 5 }); - expect(result.reason).toContain("done=1, incomplete=5"); }); it("allows legitimate all-done no-op tasks", () => { @@ -23,10 +26,17 @@ describe("evaluateNoCommitsNoOpFinalize", () => { })).toEqual({ blocked: false, doneCount: 3, incompleteCount: 0 }); }); - it("allows mostly-done no-op tasks with only a minor skipped tail", () => { + it("allows mostly-done no-commits ops tasks with only a minor non-verification skipped tail", () => { expect(evaluateNoCommitsNoOpFinalize({ noCommitsExpected: true, - steps: steps(["done", "done", "done", "done", "done", "skipped"]), + steps: namedSteps([ + ["Plan", "done"], + ["Configure", "done"], + ["Apply", "done"], + ["Announce release", "done"], + ["Update dashboard", "done"], + ["Optional cleanup", "skipped"], + ]), })).toEqual({ blocked: false, doneCount: 5, incompleteCount: 1 }); }); @@ -44,13 +54,65 @@ describe("evaluateNoCommitsNoOpFinalize", () => { it("preserves zero-step behavior", () => { expect(evaluateNoCommitsNoOpFinalize({ noCommitsExpected: true, steps: [] })) .toEqual({ blocked: false, doneCount: 0, incompleteCount: 0 }); + expect(evaluateNoCommitsNoOpFinalize({ noCommitsExpected: false, steps: [] })) + .toEqual({ blocked: false, doneCount: 0, incompleteCount: 0 }); }); - it("does not block ordinary tasks", () => { + // FN-8141: the laundered shape — a commit-expected task whose branch is empty + // because the work was reverted, with a majority of steps done and the + // remainder skipped. Must block even though it is not `noCommitsExpected` and + // done (3) > skipped (2). + it("blocks the FN-8141 reverted commit-expected shape (3 done + 2 skipped)", () => { + const result = evaluateNoCommitsNoOpFinalize({ + noCommitsExpected: false, + steps: namedSteps([ + ["Update pi SDK", "done"], + ["Wire runtime", "done"], + ["Verify Kimi K3", "done"], + ["Testing & Verification", "skipped"], + ["Documentation & Delivery", "skipped"], + ]), + }); + + expect(result).toMatchObject({ blocked: true, doneCount: 3, incompleteCount: 2 }); + expect(result.reason).toContain("Testing & Verification"); + }); + + it("blocks a skipped verification step regardless of done/skip ratio or noCommitsExpected", () => { + // Majority done, only one skipped step, but it is verification-flavored. + for (const noCommitsExpected of [true, false]) { + const result = evaluateNoCommitsNoOpFinalize({ + noCommitsExpected, + steps: namedSteps([ + ["Implement", "done"], + ["Refactor", "done"], + ["Docs", "done"], + ["QA sign-off", "skipped"], + ]), + }); + expect(result).toMatchObject({ blocked: true }); + expect(result.reason).toContain("QA sign-off"); + } + }); + + it("blocks any non-verification skipped step on a commit-expected task", () => { + const result = evaluateNoCommitsNoOpFinalize({ + noCommitsExpected: false, + steps: namedSteps([ + ["Implement", "done"], + ["Deploy notes", "skipped"], + ]), + }); + expect(result).toMatchObject({ blocked: true, doneCount: 1, incompleteCount: 1 }); + expect(result.reason).toContain("Deploy notes"); + }); + + it("does not block skip-free ordinary tasks (all-done handled by lineage proof)", () => { expect(evaluateNoCommitsNoOpFinalize({ noCommitsExpected: false, - steps: steps(["done", "skipped", "skipped"]), - })).toEqual({ blocked: false, doneCount: 1, incompleteCount: 2 }); + steps: steps(["done", "done"]), + })).toEqual({ blocked: false, doneCount: 2, incompleteCount: 0 }); + // No skipped step and not noCommitsExpected → out of this guard's scope. expect(evaluateNoCommitsNoOpFinalize({ steps: steps(["pending"]), })).toEqual({ blocked: false, doneCount: 0, incompleteCount: 1 }); diff --git a/packages/core/src/no-commits-finalize-guard.ts b/packages/core/src/no-commits-finalize-guard.ts index 895737d861..b679fe64a6 100644 --- a/packages/core/src/no-commits-finalize-guard.ts +++ b/packages/core/src/no-commits-finalize-guard.ts @@ -11,16 +11,63 @@ export interface NoCommitsNoOpFinalizeEvaluation { * FNXC:Lifecycle 2026-06-14-19:54: * FN-6461/FN-6455 showed that release and ops tasks marked `noCommitsExpected` can be silently finalized as no-op after skipping substantive steps. * Zero-diff finalize lanes must only trust step evidence when completed work outweighs incomplete work; ties block because a todo requeue is recoverable while dropping operational work is not. + * + * FNXC:Lifecycle 2026-07-16-14:20: + * FN-8141 laundered a REVERTED (commit-expected) task to `done`: pi SDK bumps kept breaking verify, the work was reverted 5x, and the agent marked "Testing & Verification" + "Documentation & Delivery" skipped. The branch was empty vs main, so the AI empty-merge lane finalized it as a no-op with `mergeConfirmed:true` and no reviewer ever saw it. + * The FN-6461 rule missed it twice: it only fired for `noCommitsExpected === true` (FN-8141 was commit-expected), and even then only when incomplete >= done (FN-8141 had 3 done vs 2 skipped). + * New invariant: a zero-diff/no-op finalize is blocked whenever ANY step is `skipped` (empty diff + skipped step means work was never done or was reverted, so `done` is unsafe). A verification-flavored skipped step (name matching /test|verif|qa|review/i) blocks unconditionally; any other skipped step blocks unless every non-skipped step is `done` AND the task is the legacy `noCommitsExpected` ops shape. This is evaluated only at zero-diff finalize lanes, so the empty-diff condition is supplied by the caller. */ +const VERIFICATION_STEP_NAME = /test|verif|qa|review/i; + export function evaluateNoCommitsNoOpFinalize( task: Pick, ): NoCommitsNoOpFinalizeEvaluation { const steps = task.steps ?? []; const doneCount = steps.filter((step) => step.status === "done").length; const incompleteCount = steps.length - doneCount; + const noCommitsExpected = task.noCommitsExpected === true; + const skippedSteps = steps.filter((step) => step.status === "skipped"); + + // FN-8141: skipped step + empty diff. Applies to ALL tasks regardless of `noCommitsExpected`. + if (skippedSteps.length > 0) { + const verificationSkipped = skippedSteps.filter((step) => + VERIFICATION_STEP_NAME.test(step.name ?? ""), + ); + + // A skipped verification/QA/review step over an empty diff blocks unconditionally: + // there is no reviewer or test evidence, so `done` cannot be trusted. + if (verificationSkipped.length > 0) { + const names = verificationSkipped.map((step) => step.name).join(", "); + return { + blocked: true, + reason: `skipped verification step(s) with no net branch changes: ${names}`, + doneCount, + incompleteCount, + }; + } + + // Other skipped steps only pass for the legacy ops shape: every non-skipped step + // completed (`done`) AND the task explicitly expected no commits. Anything else + // (e.g. a reverted commit-expected task like FN-8141) blocks. + const everyNonSkippedDone = steps + .filter((step) => step.status !== "skipped") + .every((step) => step.status === "done"); + if (!(everyNonSkippedDone && noCommitsExpected)) { + const names = skippedSteps.map((step) => step.name).join(", "); + return { + blocked: true, + reason: `skipped step(s) with no net branch changes and no operator/reviewer sign-off: ${names}`, + doneCount, + incompleteCount, + }; + } + } + + // Legacy FN-6461 rule: no-commits ops tasks whose incomplete work (incl. pending/in-progress) + // ties or outweighs completed work must not finalize on step evidence alone. if ( - task.noCommitsExpected === true && + noCommitsExpected && steps.length > 0 && incompleteCount > 0 && // Equal counts still block: requeueing is recoverable, but silently dropping ops work is not. diff --git a/packages/engine/src/__tests__/merger-ai.test.ts b/packages/engine/src/__tests__/merger-ai.test.ts index 1f33e1ab03..134b6ea986 100644 --- a/packages/engine/src/__tests__/merger-ai.test.ts +++ b/packages/engine/src/__tests__/merger-ai.test.ts @@ -527,9 +527,11 @@ describe("runAiMerge", () => { expect(result.merged).toBe(false); expect(result.noOp).toBe(false); - expect(result.error).toContain("done=1, incomplete=5"); + // A skipped verification/QA step (here "Verify"/"Testing") blocks with a + // precise reason naming the skipped step(s). + expect(result.error).toContain("skipped verification step"); expect(task.column).toBe("todo"); - expect(task.error).toContain("done=1, incomplete=5"); + expect(task.error).toContain("skipped verification step"); expect(store.moveTask).toHaveBeenCalledWith("FN-1", "todo", expect.objectContaining({ preserveProgress: true, moveSource: "engine" })); expect(store.moveTask).not.toHaveBeenCalledWith("FN-1", "done"); expect(store.logEntry).toHaveBeenCalledWith( @@ -540,6 +542,41 @@ describe("runAiMerge", () => { expect(git(dir, "rev-parse main")).toBe(mainBefore); }); + // FNXC:Lifecycle 2026-07-16-14:20: + // FN-8141 was a COMMIT-expected task (noCommitsExpected falsy) whose branch was + // empty because the SDK-bump work was reverted; 3 steps done, "Testing & + // Verification" + "Documentation & Delivery" skipped. The FN-6461 guard skipped + // it (not noCommitsExpected, done>skip), so the AI empty-merge lane laundered it + // to done. The generalized guard must demote it to todo instead. + it("demotes the FN-8141 reverted commit-expected task instead of AI empty-merge finalizing done", async () => { + const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); + git(dir, "merge -q fusion/fn-1"); + const { store, task } = makeStore(dir, { + // Intentionally NOT noCommitsExpected — this is a normal feature task. + steps: [ + { name: "Update pi SDK", status: "done" }, + { name: "Wire runtime", status: "done" }, + { name: "Verify Kimi K3", status: "done" }, + { name: "Testing & Verification", status: "skipped" }, + { name: "Documentation & Delivery", status: "skipped" }, + ], + }); + const mainBefore = git(dir, "rev-parse main"); + + const result = await runAiMerge(store, dir, "FN-1", { manual: true }, { + mergeAgent: vi.fn(async () => { /* nothing to do */ }), + reviewAgent: vi.fn(async () => "REVIEW_VERDICT: approve"), + }); + + expect(result.merged).toBe(false); + expect(result.noOp).toBe(false); + expect(result.error).toContain("Testing & Verification"); + expect(task.column).toBe("todo"); + expect(store.moveTask).toHaveBeenCalledWith("FN-1", "todo", expect.objectContaining({ preserveProgress: true, moveSource: "engine" })); + expect(store.moveTask).not.toHaveBeenCalledWith("FN-1", "done"); + expect(git(dir, "rev-parse main")).toBe(mainBefore); + }); + it("still finalizes an all-done no-commits task on the AI empty-merge path", async () => { const { dir } = initRepoWithBranch({ branch: "fusion/fn-1" }); git(dir, "merge -q fusion/fn-1"); diff --git a/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts b/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts index 39ed357807..98d319f54b 100644 --- a/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts +++ b/packages/engine/src/__tests__/merger-finalize-unproven.real-git.test.ts @@ -250,8 +250,9 @@ describeIfGit("aiMergeTask finalize no-op unproven reproduction (real git)", () expect(result.merged).toBe(false); expect(result.noOp).toBe(false); - expect(result.error).toContain("done=1, incomplete=5"); - expect(store.updateTask).toHaveBeenCalledWith("FN-NO-COMMITS", expect.objectContaining({ error: expect.stringContaining("done=1, incomplete=5") })); + // "Verify"/"Testing" are skipped verification steps → precise reason naming them. + expect(result.error).toContain("skipped verification step"); + expect(store.updateTask).toHaveBeenCalledWith("FN-NO-COMMITS", expect.objectContaining({ error: expect.stringContaining("skipped verification step") })); expect(store.moveTask).toHaveBeenCalledWith("FN-NO-COMMITS", "todo", expect.objectContaining({ preserveProgress: true, moveSource: "engine" })); expect(store.moveTask).not.toHaveBeenCalledWith("FN-NO-COMMITS", "done"); expect(store.logEntry).toHaveBeenCalledWith( diff --git a/packages/engine/src/__tests__/self-healing.test.ts b/packages/engine/src/__tests__/self-healing.test.ts index 4db7de5129..77cd87faf0 100644 --- a/packages/engine/src/__tests__/self-healing.test.ts +++ b/packages/engine/src/__tests__/self-healing.test.ts @@ -2899,7 +2899,9 @@ describe("SelfHealingManager", () => { paused: false, error: null, reviewLevel: 2, - steps: [{ status: "done" }, { status: "skipped" }], + // FN-8141: a skipped step now blocks stranded-todo promotion, so a + // legitimately promotable task must be fully done (no skips). + steps: [{ status: "done" }, { status: "done" }], }, ]); @@ -4871,7 +4873,8 @@ describe("SelfHealingManager", () => { const result = await managerWithRecovery.finalizeNoOpReviewTasks(); expect(result).toBe(1); - expect(store.updateTask).toHaveBeenCalledWith("FN-6461", expect.objectContaining({ error: expect.stringContaining("done=1, incomplete=5") })); + // "Verify"/"Testing" are skipped verification steps → precise reason naming them. + expect(store.updateTask).toHaveBeenCalledWith("FN-6461", expect.objectContaining({ error: expect.stringContaining("skipped verification step") })); expect(store.moveTask).toHaveBeenCalledWith("FN-6461", "todo", expect.objectContaining({ preserveProgress: true, moveSource: "engine", recoveryRehome: true })); expect(store.moveTask).not.toHaveBeenCalledWith("FN-6461", "done"); expect(store.logEntry).toHaveBeenCalledWith( @@ -4954,6 +4957,45 @@ describe("SelfHealingManager", () => { managerWithRecovery.stop(); }); + // FNXC:Lifecycle 2026-07-16-14:20: + // FN-8141 was a commit-expected task (noCommitsExpected falsy) whose branch + // was empty (work reverted); 3 steps done, "Testing & Verification" + + // "Documentation & Delivery" skipped. The FN-6461 guard only covered + // noCommitsExpected tasks, so the stranded-todo promoter moved it to in-review + // and the merger then laundered it to done. The generalized guard must keep it + // parked in todo. + it("FN-8141: stranded todo recovery does not promote reverted commit-expected tasks with skipped steps", async () => { + const recoverCompletedTask = vi.fn().mockResolvedValue(true); + const managerWithRecovery = new SelfHealingManager(store, { + rootDir: "/tmp/test-project", + recoverCompletedTask, + }); + (store.listTasks as ReturnType).mockResolvedValue([ + { + id: "FN-8141", + column: "todo", + paused: false, + status: null, + // Intentionally NOT noCommitsExpected — normal feature task. + steps: [ + { name: "Update pi SDK", status: "done" }, + { name: "Wire runtime", status: "done" }, + { name: "Verify Kimi K3", status: "done" }, + { name: "Testing & Verification", status: "skipped" }, + { name: "Documentation & Delivery", status: "skipped" }, + ], + log: [], + }, + ]); + + const result = await managerWithRecovery.recoverStrandedCompletedTodoTasks(); + + expect(result).toBe(0); + expect(recoverCompletedTask).not.toHaveBeenCalled(); + + managerWithRecovery.stop(); + }); + it("blocks unproven no-op finalize candidates and emits audit", async () => { const managerWithRecovery = new SelfHealingManager(store, { rootDir: "/tmp/test-project",