From 19eb1794730be7565b98c1f8e8d6a16aac3383a1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 16 Jul 2026 19:36:02 -0700 Subject: [PATCH] =?UTF-8?q?fix(core):=20block=20empty-diff=20finalize=20of?= =?UTF-8?q?=20tasks=20with=20skipped=20steps=20=E2=80=94=20generalized=20F?= =?UTF-8?q?N-6461=20guard=20(#2254)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## What & why FN-8141 (\"Update pi SDK to latest and verify Kimi K3 end to end\") was **laundered into `done` despite producing zero net changes**. The pi SDK bump kept breaking `verify:fast`, the work was reverted 5×, and the agent used the sanctioned skip affordance to mark **Testing & Verification** and **Documentation & Delivery** `skipped`. `isTaskComplete()` counts `skipped` as complete, so: 1. self-healing `recoverStrandedCompletedTodoTasks` promoted the todo task to in-review (all steps done/skipped), 2. the AI merger saw an empty diff vs main → \"finalizing as no-op\" → `done` with `mergeConfirmed:true`, 3. no reviewer ever saw it (skipped steps request no review; the merge-review pass reviews an empty diff). The only existing guard, `evaluateNoCommitsNoOpFinalize` (FN-6461), missed it **twice**: it only fired when `noCommitsExpected === true` (FN-8141 was commit-expected — the branch was empty because work was *reverted*), and even then only blocked when `incomplete >= done` (FN-8141 had 3 done vs 2 skipped). ## The fix Generalize the guard (same exported name/shape — every finalize lane keeps working) so a **zero-diff/no-op finalize is blocked whenever ANY step is `skipped`**: - 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; - the legacy FN-6461 ratio rule (`noCommitsExpected` + `incomplete >= done`) is preserved for pending/in-progress incomplete work; - blocked evaluations return a precise `reason` naming the skipped steps. Legitimate shapes still pass: all-done no-skip empty diffs (left to the lineage-proof work), zero-step tasks, and `noCommitsExpected` ops tasks with a minor non-verification skipped tail. ## Surface enumeration The guard is the single chokepoint used at every zero-diff finalize lane; all already honor `.blocked`/`.reason`, so the core change fixes each surface: - `packages/engine/src/merger-ai.ts` ~1116 — AI empty-merge lane - `packages/engine/src/merger.ts` ~6261 / ~7354 / ~7658 — merger empty-own-diff + no-op lanes - `packages/engine/src/self-healing.ts` ~2851 — stranded-todo promoter pre-check; ~6335 — no-op review finalize Behavior on block is unchanged (error set, durable log entry, `task:no-commits-finalize-blocked-incomplete-steps` run-audit event, move back to todo with progress preserved). ## Test evidence - **Core** `pnpm --filter @fusion/core exec vitest run src/__tests__/no-commits-finalize-guard.test.ts` → **9 passed**. Covers FN-8141 shape (3 done + 2 skipped, not noCommitsExpected → blocked), verification-skip blocks regardless of ratio/`noCommitsExpected`, legacy `noCommitsExpected` shapes, all-done no-skip → not blocked, zero steps → not blocked. - **Engine lanes** — one test per finalize-lane family, all green: - `merger-ai.test.ts` (AI empty lane, incl. new FN-8141 reverted-commit-expected case) → **36 passed** - `merger-finalize-unproven.real-git.test.ts` (merger lanes) → passing - `self-healing.test.ts` (stranded-todo promoter + no-op review finalize, incl. new FN-8141 promoter case) → **394 passed** ### `pnpm verify:fast` — pre-existing engine build breakage (not this PR) `verify:fast` fails at the workspace-dist bootstrap because `@fusion/engine` does **not** typecheck on `main`: `src/auth-storage.ts`, `src/pi.ts`, `src/provider-registration.ts` reference `ModelRuntime` / `AuthInteraction` / `CredentialInfo` / private `ModelRegistry` members removed by pi 0.80.9/0.80.10 (the FN-8142 migration that motivated this incident; upstream fix is FN-8145). Verified this failure reproduces with my changes **stashed** (13 identical tsc errors at clean HEAD). This PR touches only `@fusion/core` (builds clean, `tsc` exit 0) and engine **test** files — no engine source — so it neither causes nor can resolve that breakage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Bug Fixes** * Prevented empty or no-op finalization when verification, QA, review, or other required steps are skipped. * Ensured tasks with skipped work are not incorrectly marked complete, merged, or promoted during recovery. * Improved error messages to identify skipped verification steps blocking completion. * **Tests** * Added regression coverage across finalization, merge, and self-healing workflows. Co-authored-by: Claude Opus --- .../empty-finalize-skipped-step-guard.md | 7 ++ .../no-commits-finalize-guard.test.ts | 74 +++++++++++++++++-- .../core/src/no-commits-finalize-guard.ts | 49 +++++++++++- .../engine/src/__tests__/merger-ai.test.ts | 41 +++++++++- .../merger-finalize-unproven.real-git.test.ts | 5 +- .../engine/src/__tests__/self-healing.test.ts | 46 +++++++++++- 6 files changed, 209 insertions(+), 13 deletions(-) create mode 100644 .changeset/empty-finalize-skipped-step-guard.md 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",