From fc24e66f6352b4a7792826ee3d46130cdedf2df4 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Sun, 19 Jul 2026 17:52:45 -0700 Subject: [PATCH] fix(engine): trust explicit task completion summaries Stop inferring incomplete work from summary wording so scoped future-work notes cannot requeue completed tasks. Keep structural review and bulk-step completion guards intact. --- .changeset/fix-task-done-summary-prose.md | 7 +++ .../executor-task-done-dissent-guard.test.ts | 35 +++++++---- .../executor-task-done-premise-stale.test.ts | 25 +++----- .../executor-task-done-shared-helper.test.ts | 2 - .../task-done-refusal-x-invariant.test.ts | 24 +++----- packages/engine/src/executor.ts | 58 +------------------ 6 files changed, 51 insertions(+), 100 deletions(-) create mode 100644 .changeset/fix-task-done-summary-prose.md diff --git a/.changeset/fix-task-done-summary-prose.md b/.changeset/fix-task-done-summary-prose.md new file mode 100644 index 0000000000..f696427ed9 --- /dev/null +++ b/.changeset/fix-task-done-summary-prose.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Accept task completion regardless of wording in the completion summary. +category: fix +dev: Keeps structural review and bulk-step completion guards while removing prose-based refusals. diff --git a/packages/engine/src/__tests__/executor-task-done-dissent-guard.test.ts b/packages/engine/src/__tests__/executor-task-done-dissent-guard.test.ts index df125a6f0c..b77e3a4732 100644 --- a/packages/engine/src/__tests__/executor-task-done-dissent-guard.test.ts +++ b/packages/engine/src/__tests__/executor-task-done-dissent-guard.test.ts @@ -44,7 +44,7 @@ async function setup(overrides: Record = {}) { return { store, doneTool }; } -describe("FN-4851 dissent guard", () => { +describe("fn_task_done summary prose", () => { beforeEach(() => { resetExecutorMocks(); vi.spyOn(worktreePool, "isUsableTaskWorktree").mockResolvedValue(true); @@ -57,31 +57,46 @@ describe("FN-4851 dissent guard", () => { }); }); - it("refuses summary that directly claims incompletion", async () => { + it("accepts completion when the summary documents future work as not implemented in this task", async () => { const { store, doneTool } = await setup(); + store.moveTask.mockClear(); + + const result = await doneTool.execute("id", { + summary: "KB-015 complete: Implemented semantic design tokens and migrated all components. All verification passes. Note: User steering comments requested migration to a shadcn-svelte pattern — this is documented as a future direction in project memory and task docs, not implemented in this task per the DO NOT redo completed steps instruction.", + }); + + expect(result.details.refusalClass).toBeUndefined(); + expect(result.content[0].text).toContain("Task marked complete"); + expect(store.moveTask).not.toHaveBeenCalledWith("FN-4851", "todo", { preserveProgress: true }); + }); + + it("accepts a completion call even when its summary claims incompletion", async () => { + const { store, doneTool } = await setup(); + store.moveTask.mockClear(); const result = await doneTool.execute("id", { summary: "Task is not complete. I'm blocked from safely finishing this." }); - expect(result.details.refusalClass).toBe("summary-claims-incomplete"); - expect(result.content[0].text).toContain("fn_task_done refused (summary-claims-incomplete)"); - expect(store.moveTask).toHaveBeenCalledWith("FN-4851", "todo", { preserveProgress: true }); - expect(store.updateTask).toHaveBeenCalledWith("FN-4851", expect.objectContaining({ taskDoneRetryCount: 1 })); + expect(result.details.refusalClass).toBeUndefined(); + expect(result.content[0].text).toContain("Task marked complete"); + expect(store.moveTask).not.toHaveBeenCalledWith("FN-4851", "todo", { preserveProgress: true }); }); - it("refuses 'To unblock' summary", async () => { + it("accepts a completion call whose summary contains 'To unblock'", async () => { const { doneTool } = await setup(); const result = await doneTool.execute("id", { summary: "To unblock, sync/land FN-4789 before I can finish." }); - expect(result.details.refusalClass).toBe("summary-claims-incomplete"); + expect(result.details.refusalClass).toBeUndefined(); + expect(result.content[0].text).toContain("Task marked complete"); }); - it("refuses summary that says it needs another FN task", async () => { + it("accepts a completion call whose summary says it needs another FN task", async () => { const { doneTool } = await setup(); const result = await doneTool.execute("id", { summary: "This needs FN-1234 before completion." }); - expect(result.details.refusalClass).toBe("summary-claims-incomplete"); + expect(result.details.refusalClass).toBeUndefined(); + expect(result.content[0].text).toContain("Task marked complete"); }); it("allows bare 'incomplete' without first-person/task context", async () => { diff --git a/packages/engine/src/__tests__/executor-task-done-premise-stale.test.ts b/packages/engine/src/__tests__/executor-task-done-premise-stale.test.ts index 0529378cac..1b03bc10be 100644 --- a/packages/engine/src/__tests__/executor-task-done-premise-stale.test.ts +++ b/packages/engine/src/__tests__/executor-task-done-premise-stale.test.ts @@ -15,11 +15,8 @@ function createTask(stepStatuses: Array<"done" | "skipped" | "pending" | "in-pro } as any; } -describe("preflight PREMISE STALE: escape hatch", () => { - it("allows fn_task_done when summary starts with PREMISE STALE: even if it contains 'done' near 'the task'", () => { - // Without the bypass, the scoped-incomplete regex matches 'done' and the - // 40-char window contains 'the task' → would refuse with - // summary-claims-incomplete. The bypass must let this through. +describe("fn_task_done summary-independent refusal checks", () => { + it("allows fn_task_done when summary starts with PREMISE STALE:", () => { const task = createTask(["done", "skipped", "skipped", "skipped", "skipped"]); const result = evaluateTaskDoneRefusal( task, @@ -29,10 +26,7 @@ describe("preflight PREMISE STALE: escape hatch", () => { expect(result).toEqual({ ok: true }); }); - it("allows fn_task_done when summary starts with PREMISE STALE: and contains 'I'm blocked' style dissent phrasing", () => { - // Natural premise-stale phrasing may accidentally include a dissent-pattern - // word ("blocked from", "to unblock", "requires follow-up"). The bypass - // must not refuse on the dissent regex when the sentinel is present. + it("allows fn_task_done when summary contains blocked-work phrasing", () => { const task = createTask(["done", "skipped", "skipped"]); const result = evaluateTaskDoneRefusal( task, @@ -42,7 +36,7 @@ describe("preflight PREMISE STALE: escape hatch", () => { expect(result).toEqual({ ok: true }); }); - it("is case-insensitive on the sentinel", () => { + it("allows a lowercase PREMISE STALE sentinel", () => { const task = createTask(["done", "skipped"]); const result = evaluateTaskDoneRefusal( task, @@ -52,19 +46,14 @@ describe("preflight PREMISE STALE: escape hatch", () => { expect(result).toEqual({ ok: true }); }); - it("does NOT bypass when sentinel appears later in the summary (must be at start)", () => { - // Defends against agents tacking the sentinel into the middle to dodge a - // genuine incomplete-work refusal. - const task = createTask(["done", "pending"]); + it("does not reject incomplete-work prose when PREMISE STALE appears later", () => { + const task = createTask(["done"]); const result = evaluateTaskDoneRefusal( task, { summary: "The task is not done yet, but PREMISE STALE: I think it's stale anyway" }, new Map(), ); - expect(result.ok).toBe(false); - if (!result.ok) { - expect(result.refusalClass).toBe("summary-claims-incomplete"); - } + expect(result).toEqual({ ok: true }); }); it("still enforces pending-code-review-revise even with the sentinel", () => { diff --git a/packages/engine/src/__tests__/executor-task-done-shared-helper.test.ts b/packages/engine/src/__tests__/executor-task-done-shared-helper.test.ts index f5363c5cb4..303142cec9 100644 --- a/packages/engine/src/__tests__/executor-task-done-shared-helper.test.ts +++ b/packages/engine/src/__tests__/executor-task-done-shared-helper.test.ts @@ -7,11 +7,9 @@ describe("FN-4946 shared task_done refusal helper invariant", () => { const source = readFileSync(new URL("../executor.ts", import.meta.url), "utf8"); const invocations = source.match(/evaluateTaskDoneRefusal\(/g) ?? []; const helperDecl = source.match(/\bfunction evaluateTaskDoneRefusal\b/g) ?? []; - const dissentDecl = source.match(/\bconst DISSENT_PATTERNS\b/g) ?? []; expect(invocations.length).toBeGreaterThanOrEqual(3); expect(helperDecl).toHaveLength(1); - expect(dissentDecl).toHaveLength(1); }); it("returns pending-code-review-revise for a pending step with REVISE and no summary", () => { diff --git a/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts b/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts index b183aa157e..0e7e7010dd 100644 --- a/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/task-done-refusal-x-invariant.test.ts @@ -79,20 +79,20 @@ describe("FN-4851 reliability interactions: task-done refusals x invariant", () invariantSpy.mockRestore(); }); - it("runs dissent refusal before scope-leak guard when invariants pass", async () => { + it("does not let summary prose prevent the scope-leak guard from running", async () => { const invariantSpy = vi.spyOn(TaskExecutor.prototype as any, "verifyWorktreeInvariants").mockResolvedValue({ ok: true }); const scopeSpy = vi.spyOn(TaskExecutor.prototype as any, "evaluateTaskDoneScopeLeak"); const { doneTool } = await setup(); const result = await doneTool.execute("done", { summary: "To unblock, land FN-4789 first." }); - expect(result.details.refusalClass).toBe("summary-claims-incomplete"); - expect(scopeSpy).not.toHaveBeenCalled(); + expect(result.details.refusalClass).toBeUndefined(); + expect(scopeSpy).toHaveBeenCalled(); scopeSpy.mockRestore(); invariantSpy.mockRestore(); }); - it("shares one retry budget across mixed refusal classes", async () => { + it("shares one retry budget across repeated structural refusals", async () => { const { doneTool, store, getTask } = await setup({ steps: [{ name: "S1", status: "in-progress" }, { name: "S2", status: "pending" }] }); // FNXC:WorkflowLifecycle 2026-07-01-21:20: setup()'s execute() drives a bare mock agent through the @@ -115,28 +115,22 @@ describe("FN-4851 reliability interactions: task-done refusals x invariant", () steps: [{ name: "S1", status: "in-progress" }, { name: "S2", status: "pending" }], }); - await doneTool.execute("1", { summary: "Task is not complete." }); + stageIncompleteSteps(); + await doneTool.execute("1", { summary: "Completed implementation and tests." }); expect(getTask().taskDoneRetryCount).toBe(1); stageIncompleteSteps(); await doneTool.execute("2", { summary: "Completed implementation and tests." }); expect(getTask().taskDoneRetryCount).toBe(2); - /* - FNXC:WorkflowReviewGates 2026-07-19-02:45: - U10 (R9): refusals 3 and 4 used to be driven by `fn_review_step` producing a REVISE verdict - (`pending-code-review-revise`). That tool is deleted and the verdict map has no writer, so the - class is unreachable from the executor. The invariant under test is the SHARED budget across - MIXED classes — not which specific classes — so the sequence now mixes bulk-step-completion - with summary-claims-incomplete, keeping three distinct classes on one budget. - */ stageIncompleteSteps(); const third = await doneTool.execute("3", { summary: "Completed implementation and tests." }); expect(third.details.refusalClass).toBe("bulk-step-completion-without-review"); expect(getTask().taskDoneRetryCount).toBe(3); - const fourth = await doneTool.execute("4", { summary: "Task is not complete." }); - expect(fourth.details.refusalClass).toBe("summary-claims-incomplete"); + stageIncompleteSteps(); + const fourth = await doneTool.execute("4", { summary: "Completed implementation and tests." }); + expect(fourth.details.refusalClass).toBe("bulk-step-completion-without-review"); // FNXC:WorkflowLifecycle 2026-07-01-21:20: The shared retry budget is exhausted on the 4th refusal // (count already 3). Exhaustion is terminal and now parks the task `status: "failed"` in place under // the workflow-graph failure model (superseding FN-1284's move-to-in-review escalation); the invariant diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 71a5178d82..faeacf1555 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -477,17 +477,7 @@ const LOOP_COMPACTION_TIMEOUT_MS = 60_000; const TASK_DONE_REFUSAL_SUFFIX = "Either finish the work and resubmit, or do not call fn_task_done — exit the session and the engine will requeue."; -export const DISSENT_PATTERNS: RegExp[] = [ - /\btask (is|was)(?: not|n['’]?t) complete\b/i, - /\b(?:i (?:could|can)(?:not|n['’]?t)|unable to|failed to) (?:complete|finish|implement)\b/i, - /\b(?:partially|not fully) (?:complete|implemented|done|finished)\b/i, - /\b(?:i['’]?m blocked|blocked from|blocking issue prevents)\b/i, - /\bto unblock\b/i, - /\b(?:needs|requires) (?:FN-\d+|further work|additional work|follow[- ]?up)\b/i, -]; - type TaskDoneRefusalClass = - | "summary-claims-incomplete" | "bulk-step-completion-without-review" | "pending-code-review-revise"; @@ -581,7 +571,7 @@ function formatTaskDoneRefusal(refusalClass: TaskDoneRefusalClass, reason: strin export function evaluateTaskDoneRefusal( task: Task, - params: { summary?: string }, + _params: { summary?: string }, codeReviewVerdicts: Map, ): TaskDoneRefusalResult { const pendingSteps: number[] = []; @@ -602,48 +592,6 @@ export function evaluateTaskDoneRefusal( } } - const summary = params.summary?.trim(); - // Preflight escape hatch: when the agent's preflight finds PROMPT.md is out - // of sync with HEAD (work already done on the base), it marks remaining - // steps `skipped` and calls fn_task_done with a `PREMISE STALE:` summary. - // Skip the summary-text refusals (dissent + scoped-incomplete) for this - // sentinel so a natural premise-stale explanation like "...the work is - // already done on HEAD" cannot deadlock the executor. The pending-review - // and bulk-step-completion guards above/below still apply. - const isPremiseStale = !!summary && /^premise stale:/i.test(summary); - if (summary && !isPremiseStale) { - const dissentMatch = DISSENT_PATTERNS.find((pattern) => pattern.test(summary)); - if (dissentMatch) { - const matchText = summary.match(dissentMatch)?.[0] ?? dissentMatch.source; - const reason = `summary indicates incomplete work (${JSON.stringify(matchText)})`; - return { - ok: false, - refusalClass: "summary-claims-incomplete", - reason, - message: formatTaskDoneRefusal("summary-claims-incomplete", reason), - }; - } - - const scopedPattern = /\b(incomplete|not implemented|not done|not finished)\b/i; - const scopedMatch = scopedPattern.exec(summary); - if (scopedMatch) { - const start = Math.max(0, scopedMatch.index - 40); - const end = Math.min(summary.length, scopedMatch.index + scopedMatch[0].length + 40); - const scopedWindow = summary.slice(start, end); - const hasFirstPersonContext = /\b(i|i['’]?m|i['’]?ve|my|we)\b/i.test(scopedWindow) - || /\b(the task|this task)\b/i.test(scopedWindow); - if (hasFirstPersonContext) { - const reason = `summary indicates incomplete work (${JSON.stringify(scopedMatch[0])})`; - return { - ok: false, - refusalClass: "summary-claims-incomplete", - reason, - message: formatTaskDoneRefusal("summary-claims-incomplete", reason), - }; - } - } - } - if (pendingSteps.length >= 2) { const allPendingApproved = pendingSteps.every((stepIndex) => codeReviewVerdicts.get(stepIndex) === "APPROVE"); if (!allPendingApproved) { @@ -12386,7 +12334,7 @@ export class TaskExecutor { const implicitCheck = await this.store.getTask(task.id); if (implicitCheck.steps.length > 0 && implicitCheck.steps.every((s) => s.status === "done" || s.status === "skipped")) { - // Implicit path has no summary; evaluateTaskDoneRefusal will skip summary-claims-incomplete and only enforce pending-code-review-revise / bulk-step-completion-without-review. + // Implicit and explicit paths share the same structural pending-review and bulk-step-completion guards. const refusal = this.evaluateImplicitCompletionRefusal(implicitCheck, codeReviewVerdicts); if (!refusal.ok) { await this.handleImplicitTaskDoneRefusal(implicitCheck, refusal); @@ -12654,7 +12602,7 @@ export class TaskExecutor { const implicitCheck = await this.store.getTask(task.id); if (implicitCheck.steps.length > 0 && implicitCheck.steps.every((s) => s.status === "done" || s.status === "skipped")) { - // Implicit path has no summary; evaluateTaskDoneRefusal will skip summary-claims-incomplete and only enforce pending-code-review-revise / bulk-step-completion-without-review. + // Implicit and explicit paths share the same structural pending-review and bulk-step-completion guards. const refusal = this.evaluateImplicitCompletionRefusal(implicitCheck, codeReviewVerdicts); if (!refusal.ok) { await this.handleImplicitTaskDoneRefusal(implicitCheck, refusal);