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 813063c3fe..adcc277121 100644 --- a/packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts +++ b/packages/dashboard/src/__tests__/plan-approval-intake-column.test.ts @@ -127,3 +127,115 @@ describe("plan approval on the merged planning column (post-#2515)", () => { expect(res.status).toBe(400); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): +Reset must verify against the column it actually TARGETED. + +`resolveReboundColumnForTask` picks the rebound column from the task's workflow, but both +post-reset checks compared against the literal `todo`. On any workflow whose rebound +column is not `todo` — Coding (Ideas), any custom or renamed lineage — a reset that +SUCCEEDED was reported as a "limbo state" conflict: the mover and its own verification +disagreed about where the card was supposed to land. + +REVERT CHECK: restore either `updated.column !== "todo"` and this fails with a 409, +because the card lands in `backlog`, which is where its workflow says a reset belongs. +*/ +describe("reset verification uses the resolved rebound column", () => { + const REBOUND_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("does not report a limbo-state conflict when the card lands in its own rebound column", async () => { + const resetTask = { + ...PLANNING_TASK, + id: "FN-300", + column: "backlog", + status: undefined, + worktree: null, + branch: null, + checkedOutBy: null, + } as unknown as TaskDetail; + + const store = createMockStore({ + getTask: vi.fn().mockResolvedValue(resetTask), + moveTask: vi.fn().mockResolvedValue(resetTask), + updateTask: vi.fn().mockResolvedValue(resetTask), + getTaskWorkflowSelectionAsync: vi.fn().mockResolvedValue({ workflowId: "wf-custom" }), + getWorkflowDefinition: vi.fn().mockResolvedValue({ id: "wf-custom", name: "Custom", ir: REBOUND_IR }), + }); + + // The route is destructive and demands explicit confirmation; without it the request + // 400s before ever reaching the column check, which would make this case vacuous. + const res = await performRequest( + createApp(store), + "POST", + "/api/tasks/FN-300/reset", + JSON.stringify({ confirm: true }), + { "content-type": "application/json" }, + ); + /* + Assert SUCCESS, not "not 409" (PR #2582 review — greptile). A negative assertion also + passes on a 404 or 500, so it would stay green while the route failed some other way. + */ + expect(res.status).toBe(200); + }); + + it("routes drift correction to the resolved rebound column, not `todo`", async () => { + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (PR #2582 review — greptile): + The drift-correction path is where fixing the CHECK without fixing the WRITER just + moved the bug: `RESET_DRIFT_CORRECTION_FIELDS` hardcoded `column: "todo"`, so a card + with stale reset metadata was forced to `todo` and the final check — now comparing + against `resetColumn` — raised the very 409 this change removes. + + REVERT CHECK: restore `column: "todo"` in the constant and this fails, because the + correction writes `todo` while the workflow's rebound column is `backlog`. + */ + const driftedTask = { + ...PLANNING_TASK, + id: "FN-301", + column: "backlog", + // Stale binding: this is what triggers drift correction. + worktree: "/tmp/stale", + branch: null, + checkedOutBy: null, + } as unknown as TaskDetail; + const corrected = { ...driftedTask, worktree: null } as unknown as TaskDetail; + + const updateTask = vi.fn().mockResolvedValue(corrected); + let reads = 0; + const store = createMockStore({ + /* + The route reads the task before the move AND after it; both must still show the + stale worktree for drift correction to trigger. Only reads after the correction + writes see the cleaned task. + */ + getTask: vi.fn().mockImplementation(async () => (reads++ < 2 ? driftedTask : corrected)), + moveTask: vi.fn().mockResolvedValue(driftedTask), + updateTask, + getTaskWorkflowSelectionAsync: vi.fn().mockResolvedValue({ workflowId: "wf-custom" }), + getWorkflowDefinition: vi.fn().mockResolvedValue({ id: "wf-custom", name: "Custom", ir: REBOUND_IR }), + }); + + await performRequest( + createApp(store), + "POST", + "/api/tasks/FN-301/reset", + JSON.stringify({ confirm: true }), + { "content-type": "application/json" }, + ); + + const correctionCall = updateTask.mock.calls.find(([, patch]) => patch && "column" in patch); + expect(correctionCall).toBeDefined(); + expect((correctionCall![1] as { column: string }).column).toBe("backlog"); + }); +}); diff --git a/packages/dashboard/src/routes/register-task-workflow-routes.ts b/packages/dashboard/src/routes/register-task-workflow-routes.ts index bddd220038..2db5fb8165 100644 --- a/packages/dashboard/src/routes/register-task-workflow-routes.ts +++ b/packages/dashboard/src/routes/register-task-workflow-routes.ts @@ -536,8 +536,18 @@ const RESET_TASK_FIELDS = { sessionFile: null, } as const; +/* +FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — PR #2582 review, greptile): +COLUMN REMOVED from the shared constant. It hardcoded `todo`, so drift correction forced +the card there regardless of the workflow's actual rebound column — and then the final +verification (which now compares against `resetColumn`) saw the mismatch and raised the +very 409 "limbo" conflict this change exists to remove. Fixing the check without fixing +the writer just moved the bug. + +The column is supplied per call from the resolved rebound column; everything else here is +genuinely column-independent cleanup. +*/ const RESET_DRIFT_CORRECTION_FIELDS = { - column: "todo" as const, worktree: null, branch: null, status: null, @@ -1118,6 +1128,21 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork the heuristic instead of turning a board load into thousands of reads. */ try { + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8, DELIBERATELY NOT CONVERTED): + This filter names `todo`, so a workflow whose waiting lane is called something else + gets no enrichment and silently falls back to the heuristic. I converted it and then + REVERTED: resolving each task's hold column needs a per-task workflow read, and this + is the board-load path whose own comment above exists because unbounded reads here + "turn a board load into thousands of reads". My version did those reads for every + task BEFORE the enrich limit applied — trading a silent degradation for a load-time + regression on every board. + + Converting it properly needs the hold column resolved per WORKFLOW from data the + board payload already carries, not per task from the store. That is a real change + with a measurable cost, not a rename, so it is left for one — with the cost stated + rather than the conversion quietly skipped. + */ const todoRows = tasks.filter((task) => task.column === "todo"); const enrichable = todoRows.slice(0, AWAITING_PLANNING_ENRICH_LIMIT); if (todoRows.length > enrichable.length) { @@ -2884,7 +2909,16 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork throw notFound(`Task ${req.params.id} not found after reset`); } - const needsDriftCorrection = updated.column !== "todo" + /* + FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — R8 drift conversion): + Verify against the column the reset actually TARGETED. The mover two lines up already + resolves `resetColumn` from the task's workflow, but both post-reset checks compared + against the literal `todo` — so on any workflow whose rebound column is not `todo` + (Coding (Ideas), any custom or renamed lineage) a reset that SUCCEEDED was reported + as a "limbo state" conflict. The mover and its own verification disagreed about + where the card was supposed to land. + */ + const needsDriftCorrection = updated.column !== resetColumn || (updated.worktree ?? null) !== null || (updated.branch ?? null) !== null || (updated.checkedOutBy ?? null) !== null @@ -2901,10 +2935,17 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork worktreeSessionRetryCount: updated.worktreeSessionRetryCount ?? null, sessionFile: updated.sessionFile ?? null, }; - await scopedStore.updateTask(req.params.id, RESET_DRIFT_CORRECTION_FIELDS); + /* + Built as a named const, not an inline literal: `updateTask`'s patch type does not + declare `column`, and the original code only compiled because a variable reference + skips excess-property checking. Keeping that shape preserves the existing runtime + behaviour exactly while making the column follow the resolved rebound target. + */ + const driftCorrection = { ...RESET_DRIFT_CORRECTION_FIELDS, column: resetColumn }; + await scopedStore.updateTask(req.params.id, driftCorrection); await scopedStore.logEntry( req.params.id, - "Auto-corrected reset drift after moveTask — normalized task back to todo with cleared worktree/branch bindings", + `Auto-corrected reset drift after moveTask — normalized task back to ${resetColumn} with cleared worktree/branch bindings`, JSON.stringify(offendingSnapshot), ); await emitResetDriftAudit(scopedStore, req.params.id, offendingSnapshot); @@ -2914,7 +2955,8 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork } } - if (updated.column !== "todo" || (updated.worktree ?? null) !== null || (updated.branch ?? null) !== null) { + // Same target as the drift check above: the resolved rebound column, not `todo`. + if (updated.column !== resetColumn || (updated.worktree ?? null) !== null || (updated.branch ?? null) !== null) { throw conflict( `Reset refused to return task ${req.params.id} in limbo state (${updated.column}, branch=${updated.branch ?? "null"}, worktree=${updated.worktree ?? "null"})`, );