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>
This commit is contained in:
gsxdsm
7
.changeset/planning-handoff-outcome.md
Normal file
7
.changeset/planning-handoff-outcome.md
Normal file
@@ -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`.
|
||||
@@ -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();
|
||||
});
|
||||
});
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user