U7 PR2: finalize reports what it did with the card — a refused planning handoff is retried, not counted as recovered (#2498)

## The bug

`finalizeApprovedTask` has ~25 exit points and returned `void`, so no
caller could tell *"the card was handed off"* from *"finalize gave up"*.
Both callers assumed success.

`recoverApprovedTask` returned `true` **unconditionally** after
finalize, and `handleStuckAbortRequeue` treats `true` as "recovery done,
stop here". So when the release move was **refused by the planning-stage
guard** (FN-8361), or the store could not perform the move at all,
recovery reported success and the card's stuck-retry budget was skipped
— nothing re-planned it, nothing escalated it, and it sat in the planner
column holding a finished spec.

The refusal was already logged loudly by FN-8596's visibility work. The
return value was the part still lying.

## Three states, not a boolean

This is the load-bearing decision in the PR:

| Outcome | Meaning | Retry? |
|---|---|---|
| `released` | crossed into the hold column, or already resting there
(plan-in-place) | n/a — handed off |
| `parked` | deliberate, terminal-for-now: awaiting manual plan
approval, duplicate decision, operator pause, deleted duplicate | **no**
— a human owns it |
| `withheld` | finalize could not complete the handoff, nothing waiting
on a human | **yes** — caller's budget owns it |

`recoverApprovedTask` returns `outcome !== "withheld"`, so **`parked`
still returns `true`**. Narrowing to `=== "released"` is the tempting
simplification and it is wrong: it would send the stuck handler down its
draft path and stamp `needs-replan` over a plan a human is mid-review on
— a worse bug than the one being fixed. That is asserted, and the
assertion fails under exactly that narrowing.

## Why a mutable report, not a return at each exit

Threading a return through 25 exits is 25 chances to mis-classify a
branch, and mis-classifying turns a truthfulness fix into a lifecycle
bug. The report defaults to `parked`, which is equivalent to today's
observable behavior at every exit — so the plumbing is **inert
everywhere except the three sites explicitly classified**. Adding a
state to an exit is then a deliberate, reviewable act rather than a
diff-wide judgement call.

Only **two** exits are marked `withheld`, both in the release block,
both already warning loudly. Deliberately *not* marked:

- the `updatePlanningStateIfStillCurrent` guard — FN-8024 says a normal
scheduler advance legitimately lands there; the card has moved on, so a
retry would be wrong.
- `recoverMissingPromptBeforeRelease` — it owns its own recovery budget;
retrying would double up.

## Revert proofs (measured)

| Reverted | Result |
|---|---|
| `recoverApprovedTask` back to unconditional `true` | `Tests 2 failed
\| 3 passed (5)` |
| narrowed to `outcome === "released"` | `Tests 1 failed \| 4 passed
(5)` — the approval-park control |

The second row is the point: the park case is load-bearing, not
decoration.

## A fixture note that nearly produced a false green

A `vi.fn()` stub for `updateTaskAtomic` that ignores its callback makes
**every** finalize report "no longer in the planning stage" and return
before the release — silently collapsing every case into the same
uninteresting early exit. My first run was 3 failures for that reason,
not the reason I expected. The fake now applies the patch, and the
control asserts `moveTaskIf` was actually reached. Same class as the
`moveTaskIf` fake caught on #2491; recording it so the next person
recognises the shape.

## Scope

The other caller — `specifyTask`'s unconditional `onSpecifyComplete` —
is **not** gated here. Reaching it needs a live planning session, so
gating it without first extracting the reaction would be a change I
cannot prove, which is exactly the finding review caught on #2491's
deferral. That lands next, on this plumbing.

## Verification

| Check | Result |
|---|---|
| new suite | 5/5 |
| 11 triage/planning suites (triage, finalize-duplicate-lineage,
stuck-requeue-preserve-draft, explicit-duplicate-marker, preflight,
plan-artifact-writeback, refinement-routing, planning-wake,
planning-evacuation, …) | 326/326 |
| `tsc --noEmit` (engine) | clean |
| `pnpm lint` | clean |
| `pnpm test:gate` | green (299 + 10 + 71) |
| `pnpm check:changesets` | clean |

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

.changeset/planning-handoff-outcome.md

@@ -0,0 +1,7 @@
+---
+"@runfusion/fusion": patch
+---
+
+summary: A task whose planning handoff was refused is retried instead of being silently reported as recovered.
+category: fix
+dev: U7. `finalizeApprovedTask` now reports a three-state `PlanningHandoffOutcome` (released / parked / withheld) through a mutable report threaded into its ~25 exits; the default is `parked`, so the plumbing is inert except at the two sites explicitly classified as `withheld` (store lacks `moveTaskIf`; the planning-stage guard refuses the release move, FN-8361) and the one that sets `released`. `recoverApprovedTask` returns `outcome !== "withheld"` instead of an unconditional `true`, so `handleStuckAbortRequeue` stops treating a failed handoff as a completed recovery and skipping the stuck-retry budget. `parked` deliberately still returns true — an awaiting-approval park is a successful recovery and must not be overwritten with `needs-replan`.

packages/engine/src/__tests__/triage-planning-handoff-outcome.test.ts

@@ -0,0 +1,207 @@
+/*
+FNXC:PlanningHandoffOutcome 2026-07-28-09:20 (U7 / R4, R12 — workflow-owned lifecycle):
+
+THE INVARIANT: a finalize pass reports what it actually did with the card, and
+"could not hand it off" is distinguishable from "handed it to a human".
+
+Finalize has ~25 exit points and returned `void`. Both callers therefore assumed
+success. This suite covers the caller that is reachable through a public method —
+`recoverApprovedTask`, which returned `true` UNCONDITIONALLY after finalize:
+
+  - `handleStuckAbortRequeue` treats a `true` recovery as "done, stop here".
+  - So a card whose release move was REFUSED by the planning-stage guard (FN-8361),
+    or whose store could not perform the move at all, was reported as recovered.
+  - Its stuck-retry budget was then skipped: nothing re-planned it, nothing
+    escalated it, and it sat in the planner column holding a finished spec.
+
+WHY THREE STATES AND NOT A BOOLEAN. `parked` must keep returning `true`. An
+awaiting-approval park is a SUCCESSFUL recovery outcome — the card is exactly where
+the operator's pending decision put it. Returning `false` there would send the
+stuck handler down its draft path and stamp `needs-replan` over a plan a human is
+in the middle of reviewing, which is a worse bug than the one being fixed. The
+approval case below is therefore a load-bearing control, not a courtesy test.
+
+SCOPE. The other caller — `specifyTask`'s unconditional `onSpecifyComplete` — is
+not exercised here: reaching it requires a live planning session. Its gating lands
+with the runtime-reaction extraction, for the same reason the continuation drain
+had to be extracted in PR #2491 before its wiring could be proven. The plumbing
+this suite pins is what that change will consume.
+
+Every test below fails with `report.outcome`/`recoverApprovedTask`'s return
+reverted — see the PR body for the measured revert run.
+*/
+import { mkdtemp, mkdir, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+import type { Settings, Task, TaskStore } from "@fusion/core";
+
+import { TriageProcessor } from "../triage.js";
+import { planLog } from "../logger.js";
+
+/** A spec that clears deterministic validation and the step-headings requirement. */
+const REAL_SPEC = [
+  "# Task: FN-001 - Real spec",
+  "",
+  "## Mission",
+  "",
+  "Do the thing.",
+  "",
+  "## Steps",
+  "",
+  "### Step 0: Implement",
+  "- [ ] do the work",
+  "",
+].join("\n");
+
+function createTask(overrides: Partial<Task> = {}): Task {
+  return {
+    id: "FN-001",
+    title: "Task",
+    description: "desc",
+    column: "triage",
+    status: "planning",
+    dependencies: [],
+    steps: [],
+    currentStep: 0,
+    log: [],
+    createdAt: new Date().toISOString(),
+    updatedAt: new Date().toISOString(),
+    ...overrides,
+  } as Task;
+}
+
+/**
+ * `moveTaskIfResult` is the seam under test: it models what the store's
+ * planning-stage-guarded release move decided. `undefined` removes the method
+ * entirely, which is the "store cannot perform the move" branch.
+ */
+function createStore(opts: {
+  task: Task;
+  settings?: Partial<Settings>;
+  moveTaskIfResult?: "moved" | "refused" | "absent";
+} ): TaskStore {
+  const { task } = opts;
+  const store: Record<string, unknown> = {
+    listTasks: vi.fn().mockResolvedValue([]),
+    getTask: vi.fn(async (id: string) => (id === task.id ? task : undefined)),
+    getSettings: vi.fn().mockResolvedValue({ requirePlanApproval: false, ...opts.settings } as Settings),
+    parseDependenciesFromPrompt: vi.fn().mockResolvedValue([]),
+    parseStepsFromPrompt: vi.fn().mockResolvedValue([]),
+    parseFileScopeFromPrompt: vi.fn().mockResolvedValue([]),
+    updateTask: vi.fn(),
+    /*
+    Must actually invoke its callback and apply the patch. `updatePlanningStateIfStillCurrent`
+    reports success from whether the callback ran, so a `vi.fn()` that ignores it makes EVERY
+    finalize report "no longer in the planning stage" and return before the release — which
+    silently turns every case below into the same uninteresting early exit.
+    */
+    updateTaskAtomic: vi.fn(async (_id: string, patch: unknown) => {
+      const next = typeof patch === "function"
+        ? (patch as (t: Task) => Partial<Task> | null)(task)
+        : (patch as Partial<Task> | null);
+      if (next) Object.assign(task, next);
+      return task;
+    }),
+    moveTask: vi.fn(),
+    withTaskLock: vi.fn(async (_id: string, fn: () => Promise<unknown>) => fn()),
+    readTaskForMove: vi.fn(async (id: string) => (id === task.id ? task : undefined)),
+    logEntry: vi.fn(),
+    recordActivity: vi.fn().mockResolvedValue(undefined),
+    getTaskWorkflowSelection: vi.fn().mockReturnValue({ workflowId: "builtin:coding", stepIds: [] }),
+    on: vi.fn(),
+    off: vi.fn(),
+  };
+  if (opts.moveTaskIfResult !== "absent") {
+    store.moveTaskIf = vi.fn(async (_id: string, column: string) => {
+      if (opts.moveTaskIfResult === "refused") {
+        // The planning-stage guard rejected the move; the card stays put (FN-8361).
+        return { moved: false, task };
+      }
+      return { moved: true, task: { ...task, column, status: null } };
+    });
+  }
+  return store as unknown as TaskStore;
+}
+
+describe("planning handoff outcome — recoverApprovedTask reports what finalize did", () => {
+  let rootDir = "";
+
+  beforeEach(async () => {
+    rootDir = await mkdtemp(join(tmpdir(), "fusion-handoff-outcome-"));
+    await mkdir(join(rootDir, ".fusion", "tasks", "FN-001"), { recursive: true });
+    await writeFile(join(rootDir, ".fusion", "tasks", "FN-001", "PROMPT.md"), REAL_SPEC);
+    vi.spyOn(planLog, "log").mockImplementation(() => {});
+    vi.spyOn(planLog, "warn").mockImplementation(() => {});
+  });
+
+  afterEach(async () => {
+    vi.restoreAllMocks();
+    await rm(rootDir, { recursive: true, force: true });
+  });
+
+  it("reports recovered when the card is actually released (the control)", async () => {
+    const task = createTask();
+    const store = createStore({ task, moveTaskIfResult: "moved" });
+
+    const recovered = await new TriageProcessor(store, rootDir).recoverApprovedTask(task);
+
+    expect(recovered).toBe(true);
+    expect(store.moveTaskIf).toHaveBeenCalledWith("FN-001", "todo", expect.any(Function));
+  });
+
+  it("reports NOT recovered when the planning-stage guard refuses the release move (FN-8361)", async () => {
+    // The symptom: `true` here makes handleStuckAbortRequeue stop, so the card
+    // keeps a finished spec in the planner column with its retry budget skipped.
+    const task = createTask();
+    const store = createStore({ task, moveTaskIfResult: "refused" });
+
+    const recovered = await new TriageProcessor(store, rootDir).recoverApprovedTask(task);
+
+    expect(recovered).toBe(false);
+  });
+
+  it("reports NOT recovered when the store cannot perform the release move at all", async () => {
+    const task = createTask();
+    const store = createStore({ task, moveTaskIfResult: "absent" });
+
+    const recovered = await new TriageProcessor(store, rootDir).recoverApprovedTask(task);
+
+    expect(recovered).toBe(false);
+  });
+
+  it("STILL reports recovered when finalize parks the card for manual plan approval", async () => {
+    /*
+    Load-bearing control, not a courtesy case. A park is a successful recovery
+    outcome: the card is where the operator's pending decision put it. If this
+    returned false, handleStuckAbortRequeue would take its draft path and stamp
+    `needs-replan` over a plan a human is mid-review on — a worse bug than the one
+    the other two cases fix. Whoever narrows this to `outcome === "released"` will
+    fail here.
+    */
+    const task = createTask();
+    const store = createStore({
+      task,
+      settings: { requirePlanApproval: true, planApprovalMode: "workflow" } as Partial<Settings>,
+      moveTaskIfResult: "moved",
+    });
+
+    const recovered = await new TriageProcessor(store, rootDir).recoverApprovedTask(task);
+
+    expect(recovered).toBe(true);
+    // Parked, so the release move must NOT have been attempted.
+    expect(store.moveTaskIf).not.toHaveBeenCalled();
+  });
+
+  it("leaves the pre-existing withheld-before-finalize paths reporting false", async () => {
+    // Recovery's own guards run BEFORE finalize and already returned false; the
+    // outcome plumbing must not have changed them.
+    const seedTask = createTask({ status: "needs-replan" });
+    const store = createStore({ task: seedTask, moveTaskIfResult: "moved" });
+
+    await expect(
+      new TriageProcessor(store, rootDir).recoverApprovedTask(seedTask),
+    ).resolves.toBe(false);
+    expect(store.moveTaskIf).not.toHaveBeenCalled();
+  });
+});

packages/engine/src/triage.ts

@@ -244,6 +244,35 @@ export interface TriageProcessorOptions {
  * transparently restarted, so dashboard setting changes take effect without
  * an engine restart.
  */
+/**
+ * FNXC:PlanningHandoffOutcome 2026-07-28-09:20 (U7 / R4 — workflow-owned lifecycle):
+ * What a finalize pass actually did with the card. Three states, because "did it
+ * work?" is not a yes/no question here and collapsing it to one is what produced
+ * the bugs this type exists to remove:
+ *
+ *   released — the card crossed into the hold column, or was already resting there
+ *              (plan-in-place). It is the graph's now. This is the ONLY state that
+ *              means "a specification handoff happened".
+ *   parked   — finalize reached a deliberate disposition that is terminal for now:
+ *              awaiting manual plan approval, a duplicate decision, an operator
+ *              pause, a deleted duplicate. A human or a later event owns the card;
+ *              an automated retry would fight that decision.
+ *   withheld — finalize could not complete the handoff. The card still holds a
+ *              finished spec in the planner column and nothing is waiting on a
+ *              human, so the CALLER'S retry budget is the correct owner.
+ *
+ * The distinction that matters: `parked` and `withheld` both mean "not released",
+ * but only `withheld` should be retried. Treating them alike either strands a card
+ * that needed a retry or overwrites an operator's park with `needs-replan`.
+ */
+export type PlanningHandoffOutcome = "released" | "parked" | "withheld";
+
+/** Mutable report threaded through finalize's many exits. See the rationale on
+ *  `finalizeApprovedTask` for why this is a report object and not a return value. */
+export interface PlanningHandoffReport {
+  outcome: PlanningHandoffOutcome;
+}
+
 export class TriageProcessor {
   private running = false;
   private polling = false;
@@ -1114,13 +1143,29 @@ export class TriageProcessor {
       }
     }
 
-    await this.finalizeApprovedTask(task, written, settings, {
+    const report = await this.finalizeApprovedTask(task, written, settings, {
       recoveryLogAction: approvalRequired
         ? "Auto-recovered specified task stuck in planning — awaiting manual approval"
         : "Auto-recovered specified task stuck in planning — moved to todo",
     });
 
-    return true;
+    /*
+    FNXC:PlanningHandoffOutcome 2026-07-28-09:20 (U7 / R4):
+    Report what finalize ACTUALLY did. This used to `return true` unconditionally,
+    which meant a finalize that could not hand the card off still reported recovery
+    as successful — and `handleStuckAbortRequeue` treats `true` as "done, stop here".
+    So a card whose release move was refused by the planning-stage guard (FN-8361),
+    or whose store could not perform the move at all, was left holding a finished
+    spec in the planner column with its stuck-retry budget silently skipped: nothing
+    re-planned it and nothing escalated it.
+
+    `parked` still returns TRUE, and that is the whole reason this is three states
+    rather than a boolean. An awaiting-approval park is a successful outcome of
+    recovery — the card is exactly where the operator's pending decision put it.
+    Returning false there would send the stuck handler down its draft path and stamp
+    `needs-replan` over a plan a human is in the middle of reviewing.
+    */
+    return report.outcome !== "withheld";
   }
 
   private async readNonEmptyPromptDraft(taskId: string, context: string): Promise<string | undefined> {
@@ -3199,18 +3244,34 @@ export class TriageProcessor {
       recoveryLogAction?: string;
       preservePromptContent?: boolean;
     } = {},
-  ): Promise<void> {
+  ): Promise<PlanningHandoffReport> {
     /*
     FNXC:TriageStuckKill 2026-07-18-21:05:
     Mark the card finalizing for the whole Plan Review → column handoff so stuck-kill
     eviction and poll rediscovery cannot start a concurrent planner (FN-1312).
     */
     this.finalizing.add(task.id);
+    /*
+    FNXC:PlanningHandoffOutcome 2026-07-28-09:20 (U7 / R4 — workflow-owned lifecycle):
+    Finalize has ~25 exit points and previously returned `void`, so no caller could
+    tell "the card was handed off" from "finalize gave up". Both callers then assumed
+    success: `specifyTask` announced completion unconditionally, and
+    `recoverApprovedTask` returned `true` unconditionally.
+
+    A mutable report rather than a return value at each exit, deliberately: threading
+    a return through every one of those exits is 25 chances to mis-classify a branch,
+    and mis-classifying is what turns a truthfulness fix into a lifecycle bug. The
+    default is `parked`, which is exactly today's observable behavior at every exit —
+    so this plumbing is inert everywhere except the two sites explicitly marked
+    below. Adding a state to an exit is then a deliberate, reviewable act.
+    */
+    const report: PlanningHandoffReport = { outcome: "parked" };
     try {
-      await this.finalizeApprovedTaskBody(task, writtenInput, settings, options);
+      await this.finalizeApprovedTaskBody(task, writtenInput, settings, options, report);
     } finally {
       this.finalizing.delete(task.id);
     }
+    return report;
   }
 
   /*
@@ -3285,6 +3346,7 @@ export class TriageProcessor {
       recoveryLogAction?: string;
       preservePromptContent?: boolean;
     } = {},
+    report: PlanningHandoffReport = { outcome: "parked" },
   ): Promise<void> {
     let written = writtenInput;
     // FNXC:WorkflowArtifacts 2026-07-21-17:00: Confirm the authoritative plan
@@ -3840,6 +3902,10 @@ export class TriageProcessor {
         // cannot even be attempted the card stays in the planner column with a finished spec, so
         // never let that be silent.
         planLog.warn(`${task.id}: planning handoff skipped — store does not expose moveTaskIf; card left in ${task.column}`);
+        // FNXC:PlanningHandoffOutcome 2026-07-28-09:20: WITHHELD, not parked — the card
+        // holds a finished spec in the planner column and nothing is waiting on a human,
+        // so a caller's retry budget is the correct owner of what happens next.
+        report.outcome = "withheld";
         return;
       }
       const release = await moveTaskIf.call(this.store, task.id, "todo", isTaskStillInPlanningStage);
@@ -3848,10 +3914,21 @@ export class TriageProcessor {
           `${task.id}: planning handoff to todo REFUSED by the planning-stage guard `
           + `(column=${release.task?.column ?? "unknown"}, status=${release.task?.status ?? "null"}). Card left in ${task.column}.`,
         );
+        // FNXC:PlanningHandoffOutcome 2026-07-28-09:20: same class as above (FN-8361).
+        report.outcome = "withheld";
         return;
       }
     }
 
+    /*
+    FNXC:PlanningHandoffOutcome 2026-07-28-09:20:
+    The handoff is complete: the card either crossed into the hold column or was
+    already resting there (plan-in-place). Set BEFORE the terminal status clear and
+    the log lines, because the release is what makes the card the graph's — a failure
+    in the bookkeeping that follows does not un-hand-off a card that has already moved.
+    */
+    report.outcome = "released";
+
     /*
     FNXC:TriageStuckKill 2026-07-18-21:05:
     Re-assert status:null after the release move. finalize clears status early (before Plan