From a3a7f16977cb8a516ab97b140a6bb76024bab0b1 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 20:31:02 -0700 Subject: [PATCH] =?UTF-8?q?Drift=204/4:=20reset=20reported=20a=20successfu?= =?UTF-8?q?l=20reset=20as=20a=20409=20"limbo"=20conflict=20=E2=80=94=20plu?= =?UTF-8?q?s=20the=20audit=20verdict=20for=20every=20other=20site=20in=20t?= =?UTF-8?q?he=20file=20(#2582)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Drift 4/4 — a second live bug in the routes file, plus the audit for the rest of it **Stacks on #2571** (the P0). Merge that first. ### The bug `POST /tasks/:id/reset` resolves its destination through `resolveReboundColumnForTask` — the task's own workflow rebound column — and then verified the outcome against the literal `todo`. **Twice.** 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 `409` "limbo state" conflict. The mover and its own verification disagreed about where the card was supposed to land. Both checks now compare against `resetColumn`, which is already in scope two lines above the first one. ### Revert-proof, after I caught my own vacuous test My first version of this test **passed with the fix reverted**. The reset route demands `{ confirm: true }` and was 400ing before it ever reached the column check, so `expect(status).not.toBe(409)` was trivially true. That is the third time in this program a route/DOM assertion has looked like coverage while checking nothing, and the second time I have caught it in my own test. With the confirmation sent, the reverted form fails: `expected 409 not to be 409` — a correctly-reset card reported as limbo. ### Audit of the remaining sites in this file | site | fires after #2515? | verdict | |---|---|---| | 2597/2607 manual retry | yes | **SAFE, by design.** Falls back to a `todo` branch gated on the workflow declaring no `triage` column — exactly the merged shape. Written for Coding (Ideas); the merge made the default match it. | | 4584 respecify | yes | **SAFE.** Already `column === "triage" \|\| column === respecifyTarget`, and `respecifyTarget` resolves the intake column. | | 2887/2917 reset verification | **no** | **FIXED here** — false 409 on a successful reset. | | 1121 awaiting-planning enrichment | partially | **BROKEN, deliberately not converted** — see below. | ### The one I chose not to convert, and why `1121` filters on `column === "todo"`, so a workflow whose waiting lane is named otherwise gets no enrichment and silently falls back to the heuristic. I converted it and **reverted**. Resolving each task's hold column needs a per-task workflow read, and this is the board-load path whose own comment 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. Left with the cost written at the site rather than quietly skipped, and flagged here so it is tracked rather than forgotten. ### Verification `pnpm test:gate` (414 + 10 + 71), `pnpm lint`, dashboard typecheck green. Route suites: 9 passed, including `stranded-refinements-routes.test.ts` unchanged at 5. ### My drift set, final | file | before | after | PR | |---|---|---|---| | `TaskCard.tsx` | 8 | 3 | #2558 | | `ListView.tsx` | 5 | 3 | #2566 | | `taskActivity.ts` (found underneath) | 1 | 1 | #2566 | | `TaskDetailModal.tsx` | 4 | 3 | #2577 | | `register-task-workflow-routes.ts` | 10 | 11 → 9 | #2571 + this | Routes went 10 → 11 in #2571 (guards widened to accept resolved-intake **or** `triage`, so a P0 fix could not reject anything previously allowed) and back to 9 here. Every other survivor is the documented no-metadata fallback: flags are absent during the pre-load window and for a card stranded in a vanished lane, and a bare trait read would drop the affordance in exactly those states. They retire with the load window, not with a rename. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../plan-approval-intake-column.test.ts | 112 ++++++++++++++++++ .../routes/register-task-workflow-routes.ts | 52 +++++++- 2 files changed, 159 insertions(+), 5 deletions(-) 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"})`, );