P0: plan approve/reject rejects EVERY card on a merged planning column — operator-visible stall, cannot approve or reject (#2571)

## P0 — plan approve/reject is dead for cards on a merged planning
column

**This is the "card stuck with nothing to rescue it" case you asked to
hear about immediately.** Found auditing my files after #2515.

### What happens

#2515 removed `triage` from the merged default lineage — one
pre-implementation column, id `todo`, displayed "Planning". Four routes
guard with:

```ts
if (task.column !== "triage") throw badRequest("Task must be in 'triage' column ...")
```

On a workflow with no `triage` column that condition is **true for every
card**, so the routes reject all of them:

| route | effect on a merged-lineage card |
|---|---|
| `POST /tasks/:id/approve-plan` | 400 — **cannot approve** |
| `POST /tasks/:id/reject-plan` | 400 — **cannot reject** |
| `task_refine` route (×2) | 400 — refine blocked |

A card parked `awaiting-approval` can be **neither approved nor
rejected**. It is stuck, the operator is being asked for a decision they
have no way to give, and nothing throws to reveal it.

### Why it is the worst variant of this drift

Everything we have chased so far is a guard that silently **stops**
firing. This is a guard that silently starts firing on **everything** —
same root cause, opposite symptom, and worse, because the failure is
visible to the operator as a task that demands an answer and refuses
every one.

### The fix, and a deliberate choice

The guards resolve the workflow's own intake column through the existing
`resolveIntakeColumnForTask`, and they **widen rather than replace**: a
card is accepted if it is in the resolved intake column **or** in
`triage`.

That is on purpose for a P0. The fix cannot reject anything the route
previously allowed, so it carries no regression risk of its own.
Narrowing to the resolved column alone is a follow-up once the legacy id
is gone everywhere — not something to do under time pressure on a route
that gates operator decisions.

I found the value of that when a strict replacement broke 3 pre-existing
tests in `stranded-refinements-routes.test.ts`. The widened form passes
all of them **and** the new P0 cases.

### The convergence number goes UP, and I am not hiding it

Live-code `column === / !== "todo" | "triage"` in
`register-task-workflow-routes.ts`: **10 → 11**.

Each converted guard keeps the legacy id as an explicit second
condition, so a widened guard has two literals where it had one. The
metric counts id literals; it does not know the guard is now strictly
more correct. Reporting the direction that is true rather than the one
that looks better — and flagging that this file's number will only fall
once the widening can be removed.

### Revert-proof

Restore either bare literal and the matching case fails with **400 where
200 is expected**, on a `todo` card with `awaiting-approval`. A third
case pins that the guard still **narrows** — an `in-progress` card is
still rejected — so this cannot be mistaken for deleting the check.

### Verification

`pnpm test:gate` (414 + 10 + 71), `pnpm lint`, dashboard typecheck
green. New suite 3 passed; `stranded-refinements-routes.test.ts` back to
5 passed (it was 3 failed under the strict form).

### Still auditing

`TaskDetailModal.tsx` conversion is in flight on a separate branch.
`TaskCard.tsx` (#2558) and `ListView.tsx` + `taskActivity.ts` (#2566)
are already open — and note #2566 covers `isTaskAgentActive`, whose
planner-lane clause has the *silent* version of this same bug: planning
cards read as idle everywhere at once.

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-07-29 11:11:50 -07:00
committed by GitHub
parent 3c46ecca14
commit a56253f426
2 changed files with 159 additions and 10 deletions

View File

@@ -0,0 +1,129 @@
// @vitest-environment node
/*
FNXC:WorkflowResolvedColumns 2026-07-29-00:00 (U12 — P0, post-#2515):
Plan approve/reject must resolve the workflow's INTAKE column, not the id `triage`.
THE STALL THIS PINS. #2515 removed `triage` from the default lineage: there is now one
pre-implementation column, id `todo`, displayed as "Planning". The routes guarded with
`if (task.column !== "triage") throw badRequest(...)`, so after that merge the condition
was TRUE for every default-workflow card and BOTH routes rejected all of them. A card
parked `awaiting-approval` could be neither approved nor rejected — stuck, with no
operator action able to release it, and nothing crashing to reveal it.
That is the inverse of the usual drift: the guard did not stop firing, it started firing
on everything.
REVERT CHECK: restore either `task.column !== "triage"` literal and the matching case
fails with 400 instead of succeeding, because these cards are in `todo`.
*/
import { describe, it, expect, vi } from "vitest";
import express from "express";
import { mkdtempSync } from "node:fs";
import { join } from "node:path";
import { tmpdir } from "node:os";
import type { TaskStore, TaskDetail } from "@fusion/core";
import { createApiRoutes } from "../routes.js";
import { request as performRequest } from "../test-request.js";
/** The post-#2515 default lineage: ONE pre-implementation column, id `todo`. */
const MERGED_CODING_IR = {
version: "v2",
name: "builtin-stepwise-coding",
columns: [
{ id: "todo", name: "Planning", traits: [{ trait: "intake" }, { trait: "hold" }] },
{ id: "in-progress", name: "In progress", traits: [{ trait: "wip" }] },
{ id: "done", name: "Done", traits: [{ trait: "complete" }] },
],
nodes: [{ id: "start", kind: "start", column: "todo" }, { id: "end", kind: "end", column: "done" }],
edges: [{ from: "start", to: "end" }],
};
/** A card parked awaiting approval on the merged planning column. */
const PLANNING_TASK: TaskDetail = {
id: "FN-200",
title: "awaiting approval",
description: "",
column: "todo",
status: "awaiting-approval",
sourceType: "task_refine",
dependencies: [],
steps: [],
currentStep: 0,
log: [],
createdAt: new Date().toISOString(),
updatedAt: new Date().toISOString(),
prompt: "# Plan",
} as unknown as TaskDetail;
function createMockStore(overrides: Partial<TaskStore> = {}): TaskStore {
return {
getSettings: vi.fn().mockResolvedValue({}),
getRootDir: vi.fn().mockReturnValue(mkdtempSync(join(tmpdir(), "kb-plan-approval-"))),
getTask: vi.fn().mockResolvedValue(PLANNING_TASK),
updateTask: vi.fn().mockResolvedValue(PLANNING_TASK),
moveTask: vi.fn().mockResolvedValue(PLANNING_TASK),
logEntry: vi.fn().mockResolvedValue(undefined),
// Resolve the merged workflow so the routes see its real intake column.
getTaskWorkflowSelectionAsync: vi.fn().mockResolvedValue({ workflowId: "builtin:stepwise-coding" }),
getWorkflowDefinition: vi.fn().mockResolvedValue({ id: "builtin:stepwise-coding", name: "Coding", ir: MERGED_CODING_IR }),
listWorkflowDefinitions: vi.fn().mockResolvedValue([]),
on: vi.fn(),
off: vi.fn(),
getProjectScopedPluginMcpServers: vi.fn().mockResolvedValue([]),
...overrides,
} as unknown as TaskStore;
}
function createApp(store: TaskStore) {
const app = express();
app.use(express.json());
app.use("/api", createApiRoutes(store));
return app;
}
describe("plan approval on the merged planning column (post-#2515)", () => {
it("does NOT reject approve-plan for a card in the merged intake column", async () => {
const res = await performRequest(createApp(createMockStore()), "POST", "/api/tasks/FN-200/approve-plan");
/*
Assert the SUCCESS status, not merely "not 400" (PR #2571 review — greptile). A
not-400 assertion also passes on a 404 or a 500, so it would keep this case green
while the route was broken in a different way — a guard that reports success without
checking, which is the class this whole audit exists to remove.
*/
expect(res.status).toBe(200);
});
it("does NOT reject reject-plan for a card in the merged intake column", async () => {
const res = await performRequest(createApp(createMockStore()), "POST", "/api/tasks/FN-200/reject-plan");
expect(res.status).toBe(200);
});
/*
The two `task_refine` routes share the same converted guard, so they share the same
failure mode (PR #2571 review — greptile): before the fix they rejected every card on a
lineage without `triage`, which is how a stranded refinement became unrecoverable from
the UI. Covering only approve/reject would have left that guard unprotected.
*/
it("does NOT reject the stranded-refinement read for a merged-lineage card", async () => {
// The read path also consults the stranded-refinement list; stub it so a 500 from
// missing store surface cannot masquerade as the guard passing.
const store = createMockStore({ listStrandedRefinements: vi.fn().mockResolvedValue([]) });
const res = await performRequest(createApp(store), "GET", "/api/tasks/FN-200/stranded-refinement");
expect(res.status).toBe(200);
});
it("does NOT reject expedite-refinement for a merged-lineage card", async () => {
const store = createMockStore({ listStrandedRefinements: vi.fn().mockResolvedValue([]) });
const res = await performRequest(createApp(store), "POST", "/api/tasks/FN-200/expedite-refinement");
expect(res.status).toBe(200);
});
it("still rejects a card that is NOT in its workflow's intake column", async () => {
// The guard must narrow, not disappear: an in-progress card is still not approvable.
const store = createMockStore({
getTask: vi.fn().mockResolvedValue({ ...PLANNING_TASK, column: "in-progress" }),
});
const res = await performRequest(createApp(store), "POST", "/api/tasks/FN-200/approve-plan");
expect(res.status).toBe(400);
});
});

View File

@@ -3699,9 +3699,21 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork
const { store: scopedStore } = await getProjectContext(req);
const task = await scopedStore.getTask(req.params.id);
// Verify task is in triage column with awaiting-approval status
if (task.column !== "triage") {
throw badRequest("Task must be in 'triage' column to approve plan");
/*
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
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") {
throw badRequest(`Task must be in the '${approveIntakeColumn}' column to approve plan`);
}
if (task.status !== "awaiting-approval") {
throw badRequest("Task must have status 'awaiting-approval' to approve plan");
@@ -3760,9 +3772,11 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork
const { store: scopedStore } = await getProjectContext(req);
const task = await scopedStore.getTask(req.params.id);
// Verify task is in triage column with awaiting-approval status
if (task.column !== "triage") {
throw badRequest("Task must be in 'triage' column to reject plan");
// 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") {
throw badRequest(`Task must be in the '${rejectIntakeColumn}' column to reject plan`);
}
if (task.status !== "awaiting-approval") {
throw badRequest("Task must have status 'awaiting-approval' to reject plan");
@@ -3818,8 +3832,11 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork
if (task.sourceType !== "task_refine") {
throw badRequest("Task must have sourceType 'task_refine'");
}
if (task.column !== "triage") {
throw badRequest("Task must be in 'triage' column");
// 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") {
throw badRequest(`Task must be in the '${refineIntakeColumn}' column`);
}
const stranded = await scopedStore.listStrandedRefinements();
@@ -3869,8 +3886,11 @@ export function registerTaskWorkflowRoutes(ctx: ApiRoutesContext, deps: TaskWork
if (task.sourceType !== "task_refine") {
throw badRequest("Task must have sourceType 'task_refine'");
}
if (task.column !== "triage") {
throw badRequest("Task must be in 'triage' column");
// 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") {
throw badRequest(`Task must be in the '${refineIntakeColumn}' column`);
}
if (task.paused) {
throw badRequest("Paused refinements cannot be expedited");