root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

U7: the replan rebound targets a column the workflow declares (R7) — re-landed on main (#2598)

Browse Source 
> 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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-30 00:31:03 -07:00
committed by GitHub
parent f8c053c3fa
commit 8393bba7dc
3 changed files with 143 additions and 25 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
.changeset/replan-target-r7.md Normal file
View File

@@ -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.

108
packages/engine/src/__tests__/replan-target.test.ts
View File

@@ -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<Record<string, unknown>>): 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 });
});
});

53
packages/engine/src/replan-target.ts
View File

@@ -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<string> {
export async function resolveReplanTargetColumn(store: TaskStore, taskId: string): Promise<string | undefined> {
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<Task, "id" | "column">,
target?: string,
): Promise<string> {
): Promise<string | undefined> {
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;
}