From d438cd1d13bd04a314c12fcf9d95fda719ca9172 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 21:34:31 -0700 Subject: [PATCH] =?UTF-8?q?U12=20drift:=20register-task-workflow-routes.ts?= =?UTF-8?q?=20=E2=80=94=20resolve=20the=20intake=20column=20(7=20->=201)?= =?UTF-8?q?=20(#2614)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **File claimed: `packages/dashboard/src/routes/register-task-workflow-routes.ts`.** Per-file lifecycle-column guard count: **7 → 1**, and the 1 is comment prose (line 3758), so this file is done for completion-bar item 1. ## The bug this fixes `retrySpecification` decided "this Retry is a re-plan, not a generic retry" with `task.column === "triage"`. `status: "planning"` is retryable **only** through that flag — it is not in the generic `failed`/`stuck-killed` set. So on any lineage whose intake column is not literally named `triage`, a card visibly sitting in planning got `400 Task is not in a retryable state`. The operator's Retry button did nothing, with no error to explain why. Post-#2515 that includes the **default** workflow: `columnsWithFlag(resolveDefaultWorkflowIr(), "intake")` is `["todo"]` and the default's columns are `[todo, in-progress, in-review, done, archived]` — `triage` is not declared at all. The pre-existing `todo` fallback below it papered over the default case (it fires when the workflow has no `triage`), which is why this did not show up as a total outage; custom and renamed lineages had no such cover. Now: `const retryIntakeColumn = await resolveIntakeColumnForTask(scopedStore, task.id)`. ## Red-green, measured `packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts` — new case, custom lineage with intake `backlog`, card in `backlog` with `status: "planning"`: - with the change: `200` - with `task.column === "triage"` restored: **`AssertionError: expected 400 to be 200`** The fixture uses `planning` deliberately. A `failed` fixture would pass either way through the generic retryable set and prove nothing. ## What is NOT tested, and why not This PR also removes four `&& task.column !== "triage"` disjuncts I added earlier while widening the P0 approve/reject guard. **Those are untestable by construction** and I am not claiming coverage for them: removing an extra acceptance only shrinks what the guard accepts, and no case can feed these routes a `triage` card now that no shipped lineage declares one. I re-widened one guard and confirmed the suite stays green — i.e. nothing depends on the disjunct in either direction. That is the honest result, not a passing test. ## Three fixtures updated, not guards re-widened `stranded-refinements-routes.test.ts` failed with three `expected 400 to be 200` — the same failures that made me widen in the first place. This time I probed instead: `BASE_TASK` had `column: "triage"`, a column the default workflow no longer declares, so a 400 is **correct** and the fixtures were pre-merge artifacts describing a board shape the product stopped shipping. Changed to `column: "todo"` with the resolver output recorded in the file. ## Observation, deliberately not fixed here The `todo` fallback at ~2642 (`retrySpecification = !workflowHasColumn(workflowIr, "triage")`) is now near-dead: for the merged default the first branch already fires. It survives only for a lineage that has a `todo` column, no `triage`, and some *other* intake column — where treating a `todo` card as planning is arguably wrong. Deleting it is a behaviour change with its own blast radius, so it does not ride along in a conversion commit. ## Verification `pnpm lint` clean. `pnpm test:gate` green (10 / 482 / 71). Target suites: `plan-approval-intake-column.test.ts` 8/8, `stranded-refinements-routes.test.ts` 12/12. --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/u12-retry-intake-column.md | 7 +++ .../plan-approval-intake-column.test.ts | 60 +++++++++++++++++++ .../stranded-refinements-routes.test.ts | 19 +++++- .../routes/register-task-workflow-routes.ts | 42 ++++++++++--- 4 files changed, 117 insertions(+), 11 deletions(-) create mode 100644 .changeset/u12-retry-intake-column.md diff --git a/.changeset/u12-retry-intake-column.md b/.changeset/u12-retry-intake-column.md new file mode 100644 index 0000000000..6eb0958188 --- /dev/null +++ b/.changeset/u12-retry-intake-column.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Retry now works for planning cards on boards whose first column isn't named "triage". +category: fix +dev: register-task-workflow-routes.ts resolves the intake column via columnsWithFlag(ir,"intake") instead of comparing task.column to the literal "triage"; 7 lifecycle-column comparisons in the file drop to 1 (comment text). diff --git a/packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts b/packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts index adcc277121..9ed3757793 100644 --- a/packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts +++ b/packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts @@ -239,3 +239,63 @@ describe("reset verification uses the resolved rebound column", () => { expect((correctionCall![1] as { column: string }).column).toBe("backlog"); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): +A SPEC RETRY must be recognised in the workflow's intake column, whatever its id. + +`retrySpecification` decided "this is a re-plan, not a generic retry" with +`task.column === "triage"`, plus a `todo` fallback that only applied when the workflow +had no `triage` column at all. Both are id checks, so on a lineage whose intake column is +named anything else the flag stayed false — and because `status: "planning"` is retryable +ONLY through that flag, the route answered 400 "not in a retryable state" for a card that +was plainly sitting in planning. The operator's Retry button did nothing. + +This is the one conversion in this PR with an observable behaviour change, so it is the +one that gets a test. Narrowing the four `&& task.column !== "triage"` disjuncts I had +added while widening the P0 guard cannot be tested by construction: removing an extra +acceptance only shrinks what is accepted, and no case feeds these routes a `triage` card +now that no shipped lineage declares one. + +REVERT CHECK: restore `task.column === "triage"` and this fails with 400, because the +card is in `backlog` — measured, not assumed. +*/ +describe("spec retry resolves the workflow's intake column", () => { + const CUSTOM_IR = { + version: "v2", + name: "custom", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + }; + + it("accepts a retry for a planning card in a custom intake column", async () => { + /* + `status: "planning"` is deliberate: it is NOT in the generic retryable set + (`failed` / `stuck-killed`), so the request survives the retryable-state check only if + `retrySpecification` resolved true. A `failed` fixture would pass either way and prove + nothing. + */ + const planningTask = { + ...PLANNING_TASK, + id: "FN-400", + column: "backlog", + status: "planning", + } as unknown as TaskDetail; + + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(planningTask), + updateTask: vi.fn().mockResolvedValue(planningTask), + moveTask: vi.fn().mockResolvedValue(planningTask), + getTaskWorkflowSelectionAsync: vi.fn().mockResolvedValue({ workflowId: "wf-custom" }), + getWorkflowDefinition: vi.fn().mockResolvedValue({ id: "wf-custom", name: "Custom", ir: CUSTOM_IR }), + }); + + const res = await performRequest(createApp(store), "POST", "/api/tasks/FN-400/retry"); + expect(res.status).toBe(200); + }); +}); diff --git a/packages/dashboard/src/__tests__/stranded-refinements-routes.test.ts b/packages/dashboard/src/__tests__/stranded-refinements-routes.test.ts index 0f5df22d88..1be748d132 100644 --- a/packages/dashboard/src/__tests__/stranded-refinements-routes.test.ts +++ b/packages/dashboard/src/__tests__/stranded-refinements-routes.test.ts @@ -43,11 +43,25 @@ async function REQUEST(app: express.Express, method: string, path: string) { return performRequest(app, method, path); } +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): +`todo`, not `triage`. #2515 merged the default lineage's pre-implementation columns into +one whose id is `todo` (displayed "Planning") and REMOVED `triage` — so the default +workflow no longer declares `triage` at all, and these refine routes now correctly reject +a card sitting there. The fixture was a pre-merge artifact: it described a board shape the +product no longer ships. + +Verified rather than assumed: `columnsWithFlag(resolveDefaultWorkflowIr(), "intake")` is +`["todo"]`, and the default's columns are `[todo, in-progress, in-review, done, archived]`. +That is why narrowing the routes' intake guards to the resolved column (dropping the +legacy `|| === "triage"` disjunct) surfaced these three, and why updating the fixture is +the correct resolution rather than re-widening the guard. +*/ const BASE_TASK: TaskDetail = { id: "FN-100", title: "refine", description: "refine", - column: "triage", + column: "todo", sourceType: "task_refine", dependencies: [], steps: [], @@ -95,7 +109,8 @@ describe("stranded refinement routes", () => { const res = await REQUEST(createApp(store), "POST", "/api/tasks/FN-100/expedite-refinement"); expect(res.status).toBe(200); expect(res.body.expedited).toBe(true); - expect(res.body.task.column).toBe("triage"); + // Echoes the fixture's column, which is now the merged planning column. + expect(res.body.task.column).toBe("todo"); }); it("GET /tasks/:id/stranded-refinement returns detail", async () => { diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index 2db5fb8165..1de53375ec 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -2619,7 +2619,17 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork task.status === "planning" || task.status === "needs-replan" || (task.stuckKillCount ?? 0) > 0; - let retrySpecification = task.column === "triage" && retrySpecificationStatus; + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + The INTAKE column, resolved from the task's workflow. `=== "triage"` stopped matching + for default-workflow cards once the merged lineage dropped that id, so a spec retry on + a planning card fell through to the generic-retry path below. That path still catches + it for the merged shape (it keys on `todo` when the workflow declares no `triage`), so + this was not a stall — but it worked by accident of the two conditions overlapping, + not because either was right. + */ + const retryIntakeColumn = await resolveIntakeColumnForTask(scopedStore, task.id); + let retrySpecification = task.column === retryIntakeColumn && retrySpecificationStatus; /* FNXC:ManualRetry 2026-07-13-12:20: Plan-in-place workflows (Coding (Ideas): no "triage" column) keep planning/replanning @@ -3745,16 +3755,24 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — P0, post-#2515): Resolve the workflow's INTAKE column; do not name `triage`. #2515 removed `triage` from the default lineage — the single pre-implementation column is now id `todo` - displayed as "Planning" — so `task.column !== "triage"` became TRUE for every + displayed as "Planning" — so comparing the card's column against the legacy + `triage` id became TRUE for every default-workflow card and this route rejected all of them. A card parked `awaiting-approval` could not be approved OR rejected (same guard below), i.e. it was STUCK with no operator action able to release it. The guard did not stop firing; it started firing on everything. */ const approveIntakeColumn = await resolveIntakeColumnForTask(scopedStore, task.id); - // WIDEN, never narrow: accept the resolved intake column OR the legacy id, so this - // P0 fix cannot reject a card the route previously allowed. - if (task.column !== approveIntakeColumn && task.column !== "triage") { + /* + The resolved column ONLY — the legacy-`triage` disjunct this comment + used to justify is gone (PR #2614 review — greptile: the comment outlived the code). + It was a belt-and-braces widening added with the P0 fix, on the theory that a card + might still be sitting in `triage`. Nothing shipped declares that column since + #2515, so the disjunct only widened what the guard accepts, and re-adding it changed + no test in either direction. A guard that accepts a column no workflow declares is + not caution, it is an unreachable branch that reads like a requirement. + */ + if (task.column !== approveIntakeColumn) { throw badRequest(`Task must be in the '${approveIntakeColumn}' column to approve plan`); } if (task.status !== "awaiting-approval") { @@ -3817,7 +3835,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork // Same P0 as approve-plan above: resolve the intake column rather than naming // `triage`, which #2515 removed from the default lineage. const rejectIntakeColumn = await resolveIntakeColumnForTask(scopedStore, task.id); - if (task.column !== rejectIntakeColumn && task.column !== "triage") { + if (task.column !== rejectIntakeColumn) { throw badRequest(`Task must be in the '${rejectIntakeColumn}' column to reject plan`); } if (task.status !== "awaiting-approval") { @@ -3877,7 +3895,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork // Intake column, resolved from the task's workflow (#2515 removed `triage` from // the default lineage, so the literal rejected every default-workflow card). const refineIntakeColumn = await resolveIntakeColumnForTask(scopedStore, task.id); - if (task.column !== refineIntakeColumn && task.column !== "triage") { + if (task.column !== refineIntakeColumn) { throw badRequest(`Task must be in the '${refineIntakeColumn}' column`); } @@ -3931,7 +3949,7 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork // Intake column, resolved from the task's workflow (#2515 removed `triage` from // the default lineage, so the literal rejected every default-workflow card). const refineIntakeColumn = await resolveIntakeColumnForTask(scopedStore, task.id); - if (task.column !== refineIntakeColumn && task.column !== "triage") { + if (task.column !== refineIntakeColumn) { throw badRequest(`Task must be in the '${refineIntakeColumn}' column`); } if (task.paused) { @@ -4643,7 +4661,13 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork // If task is already at its workflow's intake column, skip the transition // check and moveTask. Just reset for replanning in place. - if (task.column === "triage" || task.column === respecifyTarget) { + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + `respecifyTarget` IS the resolved intake column (`resolveIntakeColumnForTask`), so the + `=== "triage"` disjunct only ever fired for a workflow whose intake is literally + triage — which that same call already returns. Redundant before the merge, dead after. + */ + if (task.column === respecifyTarget) { // Log the revision request await scopedStore.logEntry(task.id, "AI spec revision requested", feedback);