Drift 4/4: reset reported a successful reset as a 409 "limbo" conflict — plus the audit verdict for every other site in the file (#2582)

## 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) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-29 20:31:02 -07:00
committed by GitHub
parent 131feb243c
commit a3a7f16977
2 changed files with 159 additions and 5 deletions

View File

@@ -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");
});
});

View File

@@ -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"})`,
);