From 2934cccad81819e4d908d660834a04dbf0694c10 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Tue, 28 Jul 2026 11:36:20 -0700 Subject: [PATCH] =?UTF-8?q?U7=20PR2:=20finalize=20reports=20what=20it=20di?= =?UTF-8?q?d=20with=20the=20card=20=E2=80=94=20a=20refused=20planning=20ha?= =?UTF-8?q?ndoff=20is=20retried,=20not=20counted=20as=20recovered=20(#2498?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .changeset/planning-handoff-outcome.md | 7 + .../triage-planning-handoff-outcome.test.ts | 207 ++++++++++++++++++ packages/engine/src/triage.ts | 85 ++++++- 3 files changed, 295 insertions(+), 4 deletions(-) create mode 100644 .changeset/planning-handoff-outcome.md create mode 100644 packages/engine/src/__tests__/triage-planning-handoff-outcome.test.ts diff --git a/.changeset/planning-handoff-outcome.md b/.changeset/planning-handoff-outcome.md new file mode 100644 index 0000000000..a0424169b8 --- /dev/null +++ b/.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`. diff --git a/packages/engine/src/__tests__/triage-planning-handoff-outcome.test.ts b/packages/engine/src/__tests__/triage-planning-handoff-outcome.test.ts new file mode 100644 index 0000000000..b23b88f336 --- /dev/null +++ b/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 { + 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; + moveTaskIfResult?: "moved" | "refused" | "absent"; +} ): TaskStore { + const { task } = opts; + const store: Record = { + 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 | null)(task) + : (patch as Partial | null); + if (next) Object.assign(task, next); + return task; + }), + moveTask: vi.fn(), + withTaskLock: vi.fn(async (_id: string, fn: () => Promise) => 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, + 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(); + }); +}); diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index b8b6f84495..93c0981830 100644 --- a/packages/engine/src/triage.ts +++ b/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 { @@ -3199,18 +3244,34 @@ export class TriageProcessor { recoveryLogAction?: string; preservePromptContent?: boolean; } = {}, - ): Promise { + ): Promise { /* 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 { 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