From 8393bba7dca063e7a16849f430538a1c985de931 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 00:31:03 -0700 Subject: [PATCH] =?UTF-8?q?U7:=20the=20replan=20rebound=20targets=20a=20co?= =?UTF-8?q?lumn=20the=20workflow=20declares=20(R7)=20=E2=80=94=20re-landed?= =?UTF-8?q?=20on=20main=20(#2598)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > Based on `main`, no dependencies. Re-landed after closing the stacked chain (#2517, and #2551 below) that never reached main. ## What main already has, and what it lacks Main independently converted the planner-lane **parameters** in this file — and **better than I had**: it splits `plannerColumn` from `roles.mergedPlanningColumn`, because a merged lane joins the FN-8596 arrival-order rescue but *not* the "planner column is never advanced" shortcut. That work is main's and untouched here. What main still lacks is the **R7 fix**: `resolveReplanTargetColumn` returns `"triage"` **by fiat** for any workflow declaring neither legacy id — `builtin:marketing` (ideation/backlog/drafting/…) and every fully renamed set. A Plan Review REVISE therefore moves the card into a column its workflow **does not declare**, for `reconcileUndeclaredTaskColumns` to clean up after. A move the engine makes on purpose, not drift. | Workflow | Target | Changed? | |---|---|---| | `builtin:coding` / stepwise | `todo` | no | | Coding (Ideas) | `todo` | no | | `builtin:marketing` | `backlog` (its own hold) | **yes** — was `triage`, undeclared | | declares no planning lane | `undefined` → park | **yes** — was `triage` by fiat | ## The ordering the existing suite taught me Legacy ids stay preferred **first**, and the trait resolution prefers **hold over intake**. That is not arbitrary: Coding (Ideas) declares `ideas` as its intake, and `ideas` is **manual capture with no AI** (plan R10) — a rejected plan sent there stops being replanned at all. The old code got Ideas right **by accident**: it never recognised `ideas` as intake and fell through to `todo`. An "intake first" trait rule would have shipped that regression dressed as a cleanup, and three existing Ideas tests were the only thing between me and doing it. ## An inverted comment, corrected The function's own U11 note read: *"the second lookup asks for `todo`, which U11 deletes… the first lookup still matches `triage` (which U11 keeps)"*. **That is backwards.** #2515 keeps `todo` and deletes `triage`, so the consequence is the opposite of what was written — the `todo` branch is what saves builtin coding. Fixed rather than left, because a comment that inverts a merge's direction sends the next reader to the wrong branch. ## Fail-closed callers `undefined` means "nowhere to replan" (plan U5: *skipped with a log rather than moved arbitrarily*). All four call sites park **visibly** rather than log a move they did not make. The scheduler's rebound still writes `needs-replan` — deliberately, since that is what blocks dispatch and the branch has already decided the card must not be released — with only the *log* made conditional. ## The superseded test is deleted, not skipped A skipped test is a guard that cannot fire. Its replacement asserts the new contract **and** the R7 invariant directly — *"a column this workflow declares"*, not just an id — plus a new case for a workflow with no planning lane at all. ## Verification | Check | Result | |---|---| | replan-target | 41/41 | | with scheduler-trait-dispatch + pre-release-plan-review | 55/55 | | `tsc --noEmit` (engine) | clean | | `pnpm lint` | clean | | `pnpm test:gate` | green (482 + 10 + 71) | | `pnpm check:changesets` | clean | `triage.test.ts` still shows main's **8 pre-existing #2515 failures** — unchanged by this, fixed by **#2576**. ## Closing #2551 Its parameter work is superseded by main's better version; this PR carries the only part main lacked. Same story as #2517: a stacked PR outlived the surface it was converting. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/replan-target-r7.md | 7 ++ .../src/__tests__/replan-target.test.ts | 108 +++++++++++++++--- packages/engine/src/replan-target.ts | 53 +++++++-- 3 files changed, 143 insertions(+), 25 deletions(-) create mode 100644 .changeset/replan-target-r7.md diff --git a/.changeset/replan-target-r7.md b/.changeset/replan-target-r7.md new file mode 100644 index 0000000000..2c3c128838 --- /dev/null +++ b/.changeset/replan-target-r7.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: A rejected plan on a custom workflow goes back to that workflow's own planning column, not one it does not have. +category: fix +dev: U7 / R7, re-landed on main after the stacked chain was closed. `resolveReplanTargetColumn` returned `triage` by fiat for any workflow declaring neither legacy id (builtin:marketing, every renamed set), moving the card into an undeclared column for `reconcileUndeclaredTaskColumns` to clean up. Legacy ids stay preferred first so both coding built-ins keep their exact target; only a workflow declaring neither reaches the trait fallback, which prefers HOLD over intake (Coding (Ideas)' intake is manual-capture with no AI, so a rejected plan sent there stops being replanned). No declared lane returns undefined and all callers park visibly. Also corrects an inverted U11 note in that function: #2515 keeps `todo` and deletes `triage`, not the reverse. diff --git a/packages/engine/src/__tests__/replan-target.test.ts b/packages/engine/src/__tests__/replan-target.test.ts index e1b5cf4aa4..5176460e67 100644 --- a/packages/engine/src/__tests__/replan-target.test.ts +++ b/packages/engine/src/__tests__/replan-target.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, vi } from "vitest"; import type { Task, TaskStep, TaskStore } from "@fusion/core"; -import { resolveWorkflowIrForTask, workflowHasColumn } from "@fusion/core"; +import { resolveWorkflowIrForTask } from "@fusion/core"; import { hasAdvancedPastPlanning, isTaskStillInPlanningStage, moveTaskToReplanColumn, resolveReplanTargetColumn } from "../replan-target.js"; /* @@ -277,27 +277,53 @@ Updated to the post-merge truth, not loosened: each still pins one exact column. await expect(resolveReplanTargetColumn(store, "FN-1")).resolves.toBe("todo"); }); - it("targets its OWN rebound lane for a workflow declaring neither triage nor todo", async () => { + it("targets a workflow's OWN hold column when it declares neither legacy id", async () => { /* - FNXC:WorkflowReplan 2026-07-31-13:30 (U11 — the flagged fallback, now converted): - builtin:marketing declares ideation/backlog/drafting/... — no `triage`, no `todo`. - The old fallback handed it the literal `triage`, a column that lineage does not - declare AND that the default lineage no longer declares either since #2515. So the - replan move targeted a nonexistent column: the card either failed to move or landed - somewhere no sweep owns. + FNXC:ReplanTargetR7 2026-07-29-23:50: + CONTRACT CHANGED — deliberately, and this expectation edit IS the change rather + than churn around it. This asserted `"triage"` for builtin:marketing, which + declares ideation/backlog/drafting/... and NO triage column: the engine moved the + card into a column the workflow does not declare, which is the R7 violation + `reconcileUndeclaredTaskColumns` then cleaned up after. - Resolved through `resolveReboundTarget` (KTD-10: hold -> intake -> first declared), - which is the same helper every other rebound path uses. The card now lands in a - column its own workflow actually declares. + The old note gave two reasons for preferring a wrong-but-legacy column, and both + are now obsolete: "triage only scans triage and todo" (discovery resolves the + task's own lanes via `resolvePlannerLanes`) and "the legacy move path throws on + custom targets" (a move out of a non-legacy source column resolves targets from + the task's own workflow adjacency, FN-7591). + + Asserted as "a column this workflow declares" as well as by id, because the id is + incidental and the INVARIANT is what matters. */ const store = storeWithSelection("builtin:marketing"); const target = await resolveReplanTargetColumn(store, "FN-1"); - expect(target).not.toBe("triage"); + + expect(target).toBe("backlog"); const ir = await resolveWorkflowIrForTask(store as never, "FN-1"); - expect(workflowHasColumn(ir, target)).toBe(true); + expect((ir as unknown as { columns: Array<{ id: string }> }).columns.map((c) => c.id)) + .toContain(target); }); - /* Resolution failure no longer reaches the `triage` catch: the resolver swallows + it("returns UNDEFINED for a workflow that declares no planning lane at all", async () => { + // Plan U5: "skipped with a log rather than moved arbitrarily". Inventing a column + // is what this change removes. + const store = { + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "custom:wip-only", stepIds: [] })), + getWorkflowDefinition: vi.fn(async () => ({ + ir: { + version: "v2", id: "custom:wip-only", name: "wip-only", nodes: [], edges: [], + columns: [ + { id: "building", name: "Wip", traits: [{ trait: "wip" }] }, + { id: "done", name: "Done", traits: [{ trait: "complete" }] }, + ], + }, + })), + } as unknown as TaskStore; + + await expect(resolveReplanTargetColumn(store, "FN-1")).resolves.toBeUndefined(); + }); + + /* Resolution failure no longer reaches the `triage` catch: the resolver swallows the error and hands back the default IR, whose planner lane is now `todo`. The two literal `return "triage"` fallbacks survive only for workflows that declare neither column (see the marketing case above) — a pre-existing wart, since that @@ -384,3 +410,57 @@ describe("replan bounces preserve the task worktree (FN-8603)", () => { ); }); }); + +/* +FNXC:ReplanTargetR7 2026-07-30-01:20 (PR #2598 review — greptile P1): +`resolveReplanTargetColumn` returning `undefined` is only half a contract — what the +CALLERS do with it is the other half, and a silent return there is worse than the R7 +move it replaced: `requestPreMergeOptionalStepFix` returns `true` unconditionally, so +an unchanged row is reported to the graph as "remediation scheduled", the retry budget +is never consumed, and the same failure recurs with nothing to stop it. + +These pin the contract at the seam rather than at each caller, because it is the seam +every caller shares. +*/ +describe("the no-declared-lane contract", () => { + function storeFor(columns: Array>): TaskStore { + return { + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "custom:x", stepIds: [] })), + getWorkflowDefinition: vi.fn(async () => ({ + ir: { version: "v2", id: "custom:x", name: "x", nodes: [], edges: [], columns }, + })), + moveTask: vi.fn(async () => undefined), + } as unknown as TaskStore; + } + + const WIP_ONLY = [ + { id: "building", name: "Wip", traits: [{ trait: "wip" }] }, + { id: "done", name: "Done", traits: [{ trait: "complete" }] }, + ]; + + it("resolves to undefined rather than inventing a column", async () => { + await expect(resolveReplanTargetColumn(storeFor(WIP_ONLY), "FN-1")).resolves.toBeUndefined(); + }); + + it("does not move the card, and reports that it did not", async () => { + // The return value is what callers use as "where the card now is", so a silent + // no-op would be a lie they cannot detect. + const store = storeFor(WIP_ONLY); + const moved = await moveTaskToReplanColumn(store, { id: "FN-1", column: "building" }); + + expect(moved).toBeUndefined(); + expect(store.moveTask).not.toHaveBeenCalled(); + }); + + it("still moves and reports the column when a lane IS declared", async () => { + // The other side, so "never moves" cannot pass for "correctly refuses". + const store = storeFor([ + { id: "drafting", name: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + ...WIP_ONLY, + ]); + const moved = await moveTaskToReplanColumn(store, { id: "FN-1", column: "building" }); + + expect(moved).toBe("drafting"); + expect(store.moveTask).toHaveBeenCalledWith("FN-1", "drafting", { preserveWorktree: true }); + }); +}); diff --git a/packages/engine/src/replan-target.ts b/packages/engine/src/replan-target.ts index 1dc1c07e26..eaa22630b2 100644 --- a/packages/engine/src/replan-target.ts +++ b/packages/engine/src/replan-target.ts @@ -1,6 +1,7 @@ import type { Task, TaskStore } from "@fusion/core"; -import { resolveLifecycleColumns, resolveReboundTarget, resolveWorkflowIrForTask, workflowHasColumn } from "@fusion/core"; +import { resolveLifecycleColumns, resolveWorkflowIrForTask, workflowHasColumn } from "@fusion/core"; import type { WorkflowIr } from "@fusion/core"; +import { schedulerLog } from "./logger.js"; /* FNXC:WorkflowReplan 2026-07-12-23:15: @@ -315,7 +316,7 @@ export function isTaskStillInPlanningStage( return !hasAdvancedPastPlanning(task, plannerColumn, roles); } -export async function resolveReplanTargetColumn(store: TaskStore, taskId: string): Promise { +export async function resolveReplanTargetColumn(store: TaskStore, taskId: string): Promise { try { const ir = await resolveWorkflowIrForTask(store, taskId); /* @@ -360,16 +361,33 @@ export async function resolveReplanTargetColumn(store: TaskStore, taskId: string (hold -> intake -> first declared), so the card lands in a column its own workflow declares and the replan lanes stay consistent with the rebound lanes. */ - return resolveReboundTarget(ir) ?? "triage"; + /* + FNXC:ReplanTargetR7 2026-07-31-15:30 (CORRECTS my own #2659, superseded by #2598): + PREFER HOLD, THEN INTAKE, THEN NOTHING — never an arbitrary column. + + #2659 (mine) used `resolveReboundTarget`, whose third fallback is "first declared + column". For a wip-only workflow that returns the WIP column, so a needs-replan + card would be moved INTO an execution lane where the scheduler can pick it up + against the spec that was just rejected. That is worse than the literal it + replaced, and #2598's review surfaced it. + + Hold before intake is deliberate: Coding (Ideas) declares `ideas` as its intake + and `ideas` is manual capture with NO AI, so a rejected plan sent there stops + being replanned at all. The old literal got Ideas right by accident — it never + recognised `ideas` as intake and fell through to `todo`. + + `undefined` means "this workflow declares nowhere to replan"; callers park the + card visibly rather than reporting a move that did not happen. + */ + const roles = resolveLifecycleColumns(ir); + return roles?.hold ?? roles?.intake; } catch { /* - NO IR MEANS NOTHING TO RESOLVE, so this keeps the legacy literal deliberately - rather than guessing. It is reached only when resolution THROWS — not when it - falls back to the default IR, which returns a real workflow and takes the `todo` - branch above. Changing it to another literal would trade one arbitrary column for - another without evidence about the workflow. + Unreachable in practice: `resolveWorkflowIrForTask` is TOTAL — every failure path + returns the default coding IR rather than throwing. Kept as belt-and-braces and + documented so nobody writes a test for a state that cannot occur. */ - return "triage"; + return undefined; } } @@ -403,10 +421,23 @@ export async function moveTaskToReplanColumn( store: TaskStore, task: Pick, target?: string, -): Promise { +): Promise { const replanColumn = target ?? await resolveReplanTargetColumn(store, task.id); + /* + FNXC:ReplanTargetR7 2026-07-31-15:35 (PR #2598): + NO DECLARED REPLAN COLUMN: do NOT move, and say so. Every caller treats the return + value as "where the card now is", so a silent no-op would be a lie — returning + `undefined` forces the caller to state the outcome instead of assuming one. + */ + if (!replanColumn) { + schedulerLog.warn( + `${task.id}: replan rebound skipped — the task's workflow declares no intake or hold ` + + `column to replan in; card left in ${task.column}`, + ); + return undefined; + } if (task.column !== replanColumn) { - await store.moveTask(task.id, replanColumn, { preserveWorktree: true }); + await store.moveTask(task.id, replanColumn as Task["column"], { preserveWorktree: true }); } return replanColumn; }