fix(core): block empty-diff finalize of tasks with skipped steps — generalized FN-6461 guard (#2254)
## 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) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Claude Opus <noreply@anthropic.com>
This commit is contained in:
gsxdsm
7
.changeset/empty-finalize-skipped-step-guard.md
Normal file
7
.changeset/empty-finalize-skipped-step-guard.md
Normal file
@@ -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.
|
||||
@@ -5,6 +5,10 @@ function steps(statuses: Array<TaskStep["status"]>): 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 });
|
||||
|
||||
@@ -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<Task, "noCommitsExpected" | "steps">,
|
||||
): 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.
|
||||
|
||||
@@ -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");
|
||||
|
||||
@@ -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(
|
||||
|
||||
@@ -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<typeof vi.fn>).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",
|
||||
|
||||
Reference in New Issue
Block a user