From 1d6a0449ceb2f57f516a3ee406a41d385b261c5e Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 15 Jul 2026 22:36:23 -0700 Subject: [PATCH] fix(engine): keep replan cards plannable when they carry prior steps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Tasks piled up in the Planning column and never moved. hasAdvancedPastPlanning counted steps.length > 0 as proof a card had advanced past planning, but a replan card legitimately retains the steps its previous planning pass materialized. The still-in-planning guard therefore failed for every card Plan Review sent back, so triage's specifyTask claim silently skipped its status:"planning" write and re-claimed the same cards every poll — never planning them, and starving healthy cards out of the maxTriageConcurrent slots they held. Steps are no longer advancement evidence while a card sits in a planner lane: the "triage" column, and the merged "todo" planner lane used by plan-in-place workflows when the card carries a planning status. Worktrees and execution/terminal columns remain durable advancement evidence, preserving FN-7977's protection against a recovery write clobbering a card that raced ahead into execution. The primary claim path now warns instead of returning silently; recovery-write skips stay silent by design. The silence is why this stalled the planner for hours undiagnosed. Regression coverage asserts the invariant across both planner surfaces rather than the reported repro alone: triage cards with and without an explicit needs-replan status, plan-in-place todo replans, every parked-for-planning status, and the advancement signals that must still fire. Co-Authored-By: Claude Opus 4.8 (1M context) --- .changeset/fix-triage-replan-steps-wedge.md | 7 ++ .../src/__tests__/replan-target.test.ts | 91 ++++++++++++++++--- packages/engine/src/replan-target.ts | 49 ++++++++-- packages/engine/src/triage.ts | 13 +++ 4 files changed, 140 insertions(+), 20 deletions(-) create mode 100644 .changeset/fix-triage-replan-steps-wedge.md diff --git a/.changeset/fix-triage-replan-steps-wedge.md b/.changeset/fix-triage-replan-steps-wedge.md new file mode 100644 index 0000000000..858e313500 --- /dev/null +++ b/.changeset/fix-triage-replan-steps-wedge.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix tasks getting stuck in Planning forever after a plan review asks for revisions. +category: fix +dev: `hasAdvancedPastPlanning` (replan-target.ts) counted `steps.length > 0` as proof a task had advanced past planning. Replan cards legitimately retain the steps their previous planning pass materialized, so the guard at triage's `specifyTask` claim silently skipped its `status:"planning"` write, re-claimed the card every poll, and starved healthy cards out of `maxTriageConcurrent`. Steps are no longer advancement evidence in a planner lane ("triage", or "todo" for plan-in-place workflows carrying a planning status); worktrees and execution/terminal columns still are, preserving FN-7977. The primary claim path now warns instead of skipping silently. diff --git a/packages/engine/src/__tests__/replan-target.test.ts b/packages/engine/src/__tests__/replan-target.test.ts index 4e0146ed26..1bfe555183 100644 --- a/packages/engine/src/__tests__/replan-target.test.ts +++ b/packages/engine/src/__tests__/replan-target.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it, vi } from "vitest"; -import type { TaskStore } from "@fusion/core"; +import type { Task, TaskStep, TaskStore } from "@fusion/core"; import { hasAdvancedPastPlanning, isTaskStillInPlanningStage, moveTaskToReplanColumn, resolveReplanTargetColumn } from "../replan-target.js"; /* @@ -18,18 +18,85 @@ function storeWithSelection(workflowId: string | undefined): TaskStore { } as unknown as TaskStore; } +/* +FNXC:WorkflowReplan 2026-07-16-05:35: +Regression surfaces for the steps>0 planner wedge. A replan card retains the steps its +previous planning pass materialized, so steps must never imply "advanced" while the card is +parked in a planner lane. Enumerated surfaces: the "triage" column (with and without an +explicit needs-replan status), the plan-in-place "todo" planner lane used by Coding (Ideas), +every parked-for-planning status, and the advancement signals that must still fire +(worktree, execution/terminal columns, planned-and-queued todo cards). +*/ +type PlanningGuardCase = { + label: string; + task: Pick; + stillPlanning: boolean; +}; + +const planStep = (name: string): TaskStep => ({ name, status: "pending" }); + +const planningGuardCases: PlanningGuardCase[] = [ + { label: "empty triage task", task: { column: "triage", steps: [] }, stillPlanning: true }, + { label: "unplanned todo seed", task: { column: "todo", steps: [] }, stillPlanning: true }, + { label: "todo task with a worktree", task: { column: "todo", worktree: "/tmp/FN-1", steps: [] }, stillPlanning: false }, + { + label: "planned-and-queued todo task with materialized steps", + task: { column: "todo", steps: [planStep("step-1")] }, + stillPlanning: false, + }, + { label: "in-progress task", task: { column: "in-progress", steps: [] }, stillPlanning: false }, + { label: "in-review task", task: { column: "in-review", steps: [] }, stillPlanning: false }, + { label: "completed task", task: { column: "done", steps: [] }, stillPlanning: false }, + { label: "archived task", task: { column: "archived", steps: [] }, stillPlanning: false }, + + // A triage card sits in the planner column by definition — nothing executes out of triage, + // so steps materialized by its previous planning pass must never read as advancement. + { + label: "triage replan card carrying steps from its previous planning pass", + task: { column: "triage", steps: [planStep("step-1")], status: "needs-replan" }, + stillPlanning: true, + }, + { + label: "triage card carrying steps with no explicit status", + task: { column: "triage", steps: [planStep("step-1"), planStep("step-2")] }, + stillPlanning: true, + }, + { + label: "triage card parked by a reviewer outage", + task: { column: "triage", steps: [planStep("step-1")], status: "plan-review-unavailable" }, + stillPlanning: true, + }, + + // Plan-in-place workflows (Coding (Ideas)) park replans in the merged "todo" planner lane, + // carrying a real spec — the planning status is what separates them from queued work. + { + label: "plan-in-place todo replan card carrying steps", + task: { column: "todo", steps: [planStep("step-1")], status: "needs-replan" }, + stillPlanning: true, + }, + { + label: "plan-in-place todo card parked by a reviewer outage", + task: { column: "todo", steps: [planStep("step-1")], status: "plan-review-unavailable" }, + stillPlanning: true, + }, + + // FN-7977's protections must survive: real advancement still outranks a planning status. + { + label: "triage card an executor already claimed a worktree for", + task: { column: "triage", worktree: "/tmp/FN-1", steps: [planStep("step-1")], status: "needs-replan" }, + stillPlanning: false, + }, + { + label: "card that reached execution while a planning recovery was in flight", + task: { column: "in-progress", steps: [planStep("step-1")], status: "needs-replan" }, + stillPlanning: false, + }, +]; + describe("planning-stage guard", () => { - it.each([ - [{ column: "triage", worktree: null, steps: [] }, true, "empty triage task"], - [{ column: "todo", worktree: null, steps: [] }, true, "unplanned todo seed"], - [{ column: "todo", worktree: "/tmp/FN-1", steps: [] }, false, "todo task with a worktree"], - [{ column: "todo", worktree: null, steps: [{ id: "step-1" }] }, false, "todo task with materialized steps"], - [{ column: "in-progress", worktree: null, steps: [] }, false, "in-progress task"], - [{ column: "in-review", worktree: null, steps: [] }, false, "in-review task"], - [{ column: "done", worktree: null, steps: [] }, false, "completed task"], - ] as const)("recognizes %s", (task, expected) => { - expect(isTaskStillInPlanningStage(task)).toBe(expected); - expect(hasAdvancedPastPlanning(task)).toBe(!expected); + it.each(planningGuardCases)("recognizes $label", ({ task, stillPlanning }) => { + expect(isTaskStillInPlanningStage(task)).toBe(stillPlanning); + expect(hasAdvancedPastPlanning(task)).toBe(!stillPlanning); }); }); diff --git a/packages/engine/src/replan-target.ts b/packages/engine/src/replan-target.ts index 1eb7439d2e..665e7e7482 100644 --- a/packages/engine/src/replan-target.ts +++ b/packages/engine/src/replan-target.ts @@ -24,21 +24,54 @@ legal from every legacy column and eligibleTriageTasks re-specifies unconditiona * FNXC:WorkflowReplan 2026-07-15-13:15: * FN-7977: a planning/provider recovery may finish after another engine lane has * started execution. Recovery callers must prove the live row is still planning - * before writing planning state; worktrees, materialized steps, and execution or - * terminal columns are durable evidence that the task has advanced. + * before writing planning state; worktrees and execution or terminal columns are + * durable evidence that the task has advanced. + * + * FNXC:WorkflowReplan 2026-07-16-05:35: + * Materialized steps are NOT advancement evidence for a card still parked in a planner + * lane. Triage materializes steps when it finalizes a spec, so every replan (Plan Review + * REVISE -> needs-replan) legitimately carries the steps of its previous planning pass. + * Counting steps>0 as "advanced" made the primary triage claim in specifyTask() skip its + * status:"planning" write on every poll: the card was re-claimed forever, never planned, + * and — because wedged cards keep occupying maxTriageConcurrent slots — starved every + * healthy card queued behind them. Both planner surfaces must stay plannable: the "triage" + * column, and plan-in-place workflows (Coding (Ideas)) that park needs-replan cards in + * "todo" carrying a real spec. A planned-and-queued "todo" card with no planning status is + * still genuinely advanced, so steps remain the deciding signal there. */ -export function hasAdvancedPastPlanning(task: Pick): boolean { - return ( + +/** Statuses that explicitly park a card for (re)planning, whichever column holds it. */ +const PLANNING_STAGE_STATUSES = new Set(["planning", "needs-replan", "plan-review-unavailable"]); + +export function hasAdvancedPastPlanning( + task: Pick, +): boolean { + if ( task.column === "in-progress" || task.column === "in-review" || task.column === "done" || task.column === "archived" - || task.worktree != null - || (task.steps?.length ?? 0) > 0 - ); + ) { + return true; + } + // A worktree proves an executor claimed the card, even while it still sits in a planner lane. + if (task.worktree != null) { + return true; + } + // The planner column itself is never "advanced" — nothing executes out of triage. + if (task.column === "triage") { + return false; + } + // Plan-in-place planner lane ("todo"): a card explicitly parked for planning has not advanced. + if (task.status != null && PLANNING_STAGE_STATUSES.has(task.status)) { + return false; + } + return (task.steps?.length ?? 0) > 0; } -export function isTaskStillInPlanningStage(task: Pick): boolean { +export function isTaskStillInPlanningStage( + task: Pick, +): boolean { return !hasAdvancedPastPlanning(task); } diff --git a/packages/engine/src/triage.ts b/packages/engine/src/triage.ts index 129f46eff3..02ced2b0f5 100644 --- a/packages/engine/src/triage.ts +++ b/packages/engine/src/triage.ts @@ -1012,7 +1012,20 @@ export class TriageProcessor { const agentWork = async () => { // Set status only after the semaphore slot has been acquired, so // tasks waiting in the queue don't appear as "planning". + /* + FNXC:Triage 2026-07-16-05:35: + A skip on this PRIMARY claim path is an anomaly, not a benign scheduler race: poll() + already proved the card is an eligible planner candidate, so failing the guard here + means it is re-claimed every poll, never planned, and holds a maxTriageConcurrent slot + against healthy cards. Recovery-write skips stay silent by design (see + updatePlanningStateIfStillCurrent); this one must be visible — the FN-7977 steps>0 + wedge stalled the whole planner for hours precisely because it logged nothing. + */ if (!await this.updatePlanningStateIfStillCurrent(task, { status: "planning" })) { + planLog.warn( + `${task.id}: planning claim skipped — live row is no longer in the planning stage; ` + + "it will be re-claimed on the next poll", + ); return; }