From f1b528f4c5bc5af76c14716b529eac5d25442521 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 15 Jul 2026 16:21:03 -0700 Subject: [PATCH] fix(FN-7965): honor terminal fn_task_done park instead of resurrecting the session MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The in-session `fn_task_done` handler parks a task terminally (status=failed, worktree/branch/sessionFile cleared) once its refusal/invariant retry budget is exhausted. That write happens inside the live agent session, so the executor's no-fn_task_done retry loop never observed it and spawned a fresh session anyway. The retry completed, marked the task done, and dragged a worktree-less row into the pre-merge graph, where the first write-capable node failed on `no-worktree-for-write-node` — surfacing as a misleading "Workflow graph terminated with failure at node 'code-review-remediation'" instead of the real refusal. Observed on FN-7965 and again live on FN-7981. Re-read state at the top of the retry loop and honor the park. The status probe covers all three park sites (invariant-check, explicit refusal, implicit refusal) rather than the single reported repro. Deliberately not routed through the FN-4806 reclaim branch: its silent todo requeue would clear the park and, with the budget already spent, re-park on the next pickup in a todo->execute->park loop. The pre-existing reclaim probes could not catch this — they test `worktree === null`, but the store maps a cleared column to `undefined` (`task-store/serialization.ts`: `row.worktree || undefined`), so the existing test only passed because its mock returned a value production never emits. Tightening that probe regressed 7 fixtures and is left as separate work. Verified: new tests fail with the guard disabled; engine reliability surfaces show zero regressions vs baseline (17 pre-existing failures unchanged, 495->499 passing); engine-core gate suite 294/294. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/fn-7965-honor-task-done-park.md | 7 ++ ...r-no-task-done-vs-worktree-reclaim.test.ts | 68 +++++++++++++++++++ packages/engine/src/executor.ts | 24 +++++++ 3 files changed, 99 insertions(+) create mode 100644 .changeset/fn-7965-honor-task-done-park.md diff --git a/.changeset/fn-7965-honor-task-done-park.md b/.changeset/fn-7965-honor-task-done-park.md new file mode 100644 index 0000000000..2e69c2a4bb --- /dev/null +++ b/.changeset/fn-7965-honor-task-done-park.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Tasks parked by a refused fn_task_done no longer resurrect and strand at code review. +category: fix +dev: FN-7965. `fn_task_done`'s in-session refusal handler parks the row terminally (status=failed, worktree/branch/sessionFile cleared) once `MAX_TASK_DONE_REQUEUE_RETRIES` is exhausted, but the executor's no-fn_task_done retry loop never observed that park and spawned a fresh session. That session completed and marked the task done against a worktree-less row, so the pre-merge graph failed on the first write-capable node (`no-worktree-for-write-node`) and surfaced as a misleading "Workflow graph terminated with failure at node 'code-review-remediation'". The loop now re-reads state and honors the park (`terminallyParked`), stopping without requeue or review handoff — deliberately not routed through the FN-4806 reclaim branch, whose silent todo requeue would clear the park and re-park on next pickup in a todo→execute→park loop. The pre-existing reclaim probes could not catch this: they test `worktree === null`, but the store maps a cleared column to `undefined` (`task-store/serialization.ts` — `row.worktree || undefined`); tightening that probe is left as separate work. diff --git a/packages/engine/src/__tests__/reliability-interactions/executor-no-task-done-vs-worktree-reclaim.test.ts b/packages/engine/src/__tests__/reliability-interactions/executor-no-task-done-vs-worktree-reclaim.test.ts index 6d1f64c908..6582656a70 100644 --- a/packages/engine/src/__tests__/reliability-interactions/executor-no-task-done-vs-worktree-reclaim.test.ts +++ b/packages/engine/src/__tests__/reliability-interactions/executor-no-task-done-vs-worktree-reclaim.test.ts @@ -164,4 +164,72 @@ describe("reliability interactions: executor no-fn_task_done vs worktree reclaim expect.objectContaining({ status: "failed" }), ); }); + + /* + FNXC:ExecutorTaskDonePark 2026-07-15-16:10: + FN-7965 symptom verification. The in-session `fn_task_done` refusal handler parks the row terminally + (status=failed + cleared worktree/branch/sessionFile) once the refusal budget is exhausted. The retry + loop never observed that park and spawned a fresh session, which completed and marked the task done + against a worktree-less row — stranding the pre-merge graph on `no-worktree-for-write-node`. + Surface enumeration: the park must be honored whichever way the cleared binding surfaces (the real + store maps a cleared column to `undefined` via `row.worktree || undefined`, never `null`), and the + park must NOT be laundered into the FN-4806 silent requeue, which would clear the failure and + re-park on the next pickup in a todo→execute→park loop. + */ + it.each([ + ["binding cleared to undefined (real store contract)", { worktree: undefined, branch: undefined }], + ["binding cleared to null (mirror/legacy shape)", { worktree: null, branch: null }], + ["binding still present", {}], + ])("honors a terminal fn_task_done park instead of retrying — %s", async (_label, cleared) => { + const store = createMockStore(); + const initial = makeTask({ taskDoneRetryCount: 3 }); + let getTaskCalls = 0; + // Mutate only what the STORE returns, never the object passed to execute(): in production the + // in-session tool handler writes the park while this loop still holds its original `task`. + store.getTask.mockImplementation(async () => { + getTaskCalls++; + return getTaskCalls >= 2 + ? { ...initial, status: "failed", error: "fn_task_done refused (bulk-step-completion-without-review)", ...cleared } + : { ...initial }; + }); + + mockedCreateFnAgent.mockResolvedValue({ session: makeSession() } as any); + + const executor = new TaskExecutor(store as any, "/tmp/test"); + await executor.execute(makeTask({ taskDoneRetryCount: 3 })); + + // No resurrection: the park must abort before a second session spawns. + expect(mockedCreateFnAgent).toHaveBeenCalledTimes(1); + expect(store.logEntry).toHaveBeenCalledWith( + "FN-4601", + expect.stringContaining("honoring park, not retrying"), + undefined, + expect.any(Object), + ); + // The park must survive: never laundered into the FN-4806 silent requeue... + expect(store.moveTask).not.toHaveBeenCalledWith("FN-4601", "todo", { preserveProgress: true }); + // ...and never handed off to review, which is what dragged the worktree-less row into the graph. + expect(store.moveTask).not.toHaveBeenCalledWith("FN-4601", "in-review"); + }); + + it("still retries when the task has not been parked", async () => { + // Negative control: the park check must not abort a healthy retry, or every no-fn_task_done + // session would degrade into a premature stop. + const store = createMockStore(); + const state = makeTask(); + store.getTask.mockImplementation(async () => ({ ...state })); + + mockedCreateFnAgent.mockResolvedValue({ session: makeSession() } as any); + + const executor = new TaskExecutor(store as any, "/tmp/test"); + await executor.execute(state); + + expect(mockedCreateFnAgent.mock.calls.length).toBeGreaterThan(1); + expect(store.logEntry).not.toHaveBeenCalledWith( + "FN-4601", + expect.stringContaining("honoring park, not retrying"), + undefined, + expect.any(Object), + ); + }); }); diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 601304f614..4a6ee63426 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -11518,8 +11518,26 @@ export class TaskExecutor { let retryAbortedDueToReclaim = false; let refusalHandled = false; let pendingReviewParked = false; + /* FNXC:ExecutorTaskDonePark 2026-07-15-16:10: FN-7965 — set when the row was terminally parked (status=failed) by the in-session fn_task_done refusal handler; suppresses both the retry and every post-loop completion/requeue branch so the park survives. */ + let terminallyParked = false; while (!taskDone && taskDoneSessionRetries < MAX_TASK_DONE_SESSION_RETRIES) { const liveTask = await this.store.getTask(task.id); + /* + FNXC:ExecutorTaskDonePark 2026-07-15-16:10: + FN-7965: the explicit `fn_task_done` tool handler parks the task terminally (status=failed, worktree/branch/sessionFile cleared) once the refusal retry budget is exhausted — but it runs INSIDE the agent session, so this loop never learned the row had been parked and spawned a retry session anyway. That session completed and marked the task done against a row with no worktree, so the pre-merge graph died on the first write-capable node with `no-worktree-for-write-node` and surfaced as a bogus "terminated at code-review-remediation" instead of the real refusal. Re-read state and honor the park. + This deliberately does NOT reuse the FN-4806 reclaim branch below: that silently requeues to `todo`, which would clear the park and — with the refusal budget already exhausted — re-park on the next pickup, looping todo→execute→park. A terminal park is the agent's own failure and must stay parked for a human. + Note the reclaim probes below cannot cover this: they test `liveTask.worktree === null`, but the store maps a cleared column to `undefined`, never `null` (`task-store/serialization.ts` — `row.worktree || undefined`). Tightening that probe is a separate change with real blast radius, so the park is detected by status here instead. + */ + if (liveTask.status === "failed") { + const parkMessage = `${task.id}: task parked failed during no-fn_task_done retry — honoring park, not retrying`; + executorLog.log(parkMessage); + await this.store.logEntry(task.id, parkMessage, undefined, this.getRunContextFor(task.id)); + this.deleteActiveSession(task.id); + this.tokenUsageBaselines.delete(task.id); + session.dispose(); + terminallyParked = true; + break; + } const hasExplicitWorktreeBinding = typeof liveTask.worktree === "string" || liveTask.worktree === null; const hasExplicitBranchBinding = typeof liveTask.branch === "string" || liveTask.branch === null; const worktreeContractIntact = liveTask.column === "in-progress" @@ -11798,6 +11816,12 @@ export class TaskExecutor { this.clearCompletedTaskWatchdog(task.id); executorLog.log(`✓ ${task.id} completed on retry → in-review`); this.signalTaskComplete(task); + } else if (terminallyParked) { + // FN-7965: the in-session refusal handler already wrote the terminal failure and cleared + // the binding. Nothing further to do — requeueing or handing off to review here is exactly + // the resurrection that stranded the pre-merge graph. + await this.persistTokenUsage(task.id); + return; } else if (retryAbortedDueToReclaim) { // FN-4806: Worktree/branch was reclaimed mid-retry by an engine-side housekeeping path // (e.g. FN-4546 stale-active-branch reclaim, FN-4742 self-healing removals). This is NOT