test(cli): pin the node-override error contract on a renamed board, which is what #3019 actually changed (#3024)
## What #3019 actually changed, pinned — and a correction to my own claim I described #3019 as closing a hole where an operator could re-route a running task on a renamed board. **That was wrong.** `TaskStore.updateTask` runs the same guard with its own resolved lanes (`resolveNodeOverrideLanes`) and throws, so the change was refused either way. This test is how I found out: I wrote it to cover #3019's wiring and it passed against a tree with that wiring removed. A test that passes with the change reverted is not a test, so I went looking for what was really refusing — and it was the store. ## But the two paths *are* distinguishable, which my correction then got wrong in the other direction In correcting myself on #3019 I said the paths were externally indistinguishable and no test could separate them. Also wrong. Measured both ways: | | `details.error` | | --- | --- | | pre-check fires (wired) | `"task-in-progress"` — machine-readable reason code | | pre-check misses (unwired) | `"Cannot change node override for KB-001 while it is in progress…"` — the store's thrown prose | So on a **legacy** board a caller could branch on `task-in-progress`; on a **renamed** board it silently got a sentence instead. That is a real API inconsistency, visible only to whoever was parsing it — the kind of thing nobody notices until it breaks. That is what these cases pin, and it is the honest description of #3019's value: an error-contract fix, not a security fix. ## Revert proof With #3019's wiring removed: ``` Expected: "task-in-progress" Received: "Cannot change node override for KB-001 while it is in progress. …" Tests 1 failed | 1 passed (2) ``` Verified by actually reverting, not by reading the source — which is the discipline that caught both of my wrong claims above. The paired case ("still allows the override once the card leaves that wip lane") passes both ways by design; it guards against over-refusal, so I am not counting it as coverage of the contract. ## Also closes the gap I named in #3019 That PR shipped with `check-lane-wiring` as its only regression proof, and I said a behavioural test was owed. The two are complementary and fail for different reasons: **the ratchet** fails if the argument stops being passed; **this** fails if it is passed and the contract still degrades. ## Verification (measured) - **2 passed / 0 failed** - `tsc --noEmit` clean; `eslint` clean (one pre-existing warning, no errors) - `check-fnxc-future-dates`, `lifecycle-column-census --strict`, `check-lane-wiring` — green Tests only; no product file touched. No changeset. ## Note on the harness, for whoever writes the next one of these Seeding a card into a renamed lane has two traps, both inherited from `merge-blocker-renamed-review-lane.test.ts` and both recorded in this file's header: the real API is `createWorkflowDefinition` + `selectTaskWorkflow` (the plausible `saveWorkflowDefinition?.()` does not exist and the optional call swallows it silently), and moving a card takes `moveTask`, not `updateTask({ column })`. Both are guarded here by asserting the card really is in `building` before the subject runs.
This commit is contained in:
gsxdsm
@@ -0,0 +1,132 @@
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-31-14:10:
|
||||
THE INVARIANT: a card in the board's OWN wip lane is refused a mid-flight node override, BY THE CLI's
|
||||
own guard, with the structured reason code rather than a prose message from deeper down.
|
||||
|
||||
Two guards sit on this path and both refuse, so "is the change blocked?" cannot tell them apart:
|
||||
`fn_task_update` pre-checks with `validateNodeOverrideChange`, and `TaskStore.updateTask` checks again
|
||||
with its own resolved lanes (`resolveNodeOverrideLanes`) and throws. What separates them is the ERROR
|
||||
CONTRACT, and that is what these cases pin:
|
||||
|
||||
pre-check fires -> details.error === "task-in-progress" (machine-readable reason)
|
||||
pre-check misses -> details.error === "Cannot change node ..." (the store's thrown prose)
|
||||
|
||||
Measured both ways, not reasoned about. With #3019's wiring removed the first case fails exactly
|
||||
there. That difference is the real cost of the unwired pre-check: on a legacy board a caller could
|
||||
branch on `task-in-progress`, and on a renamed board it silently got a sentence instead — an API
|
||||
inconsistency visible only to whoever was parsing it.
|
||||
|
||||
WHAT THIS DOES NOT SHOW, since I claimed the opposite twice before measuring: the operator was never
|
||||
able to make the change. The store refuses either way. This is an error-contract regression test, not
|
||||
proof of a bypassable guard.
|
||||
|
||||
Two traps inherited from `merge-blocker-renamed-review-lane.test.ts`, whose own header records paying
|
||||
for both:
|
||||
- The real API is `createWorkflowDefinition` + `selectTaskWorkflow`. The plausible-looking
|
||||
`saveWorkflowDefinition?.()` / `setTaskWorkflowSelection?.()` do not exist on TaskStore, and the
|
||||
optional call swallows that silently — the task then resolves the BUILTIN workflow and every
|
||||
assertion is about the wrong board.
|
||||
- Moving a card takes `moveTask`, not `updateTask({ column })`. The latter does not move it, so the
|
||||
card sits in intake and the case is vacuous.
|
||||
Both are guarded below by asserting the premise (the card really is in `building`) before the subject.
|
||||
*/
|
||||
import { afterAll, afterEach, beforeAll, beforeEach, expect, it } from "vitest";
|
||||
import type { WorkflowIr } from "@fusion/core";
|
||||
import {
|
||||
createPgExtensionHarness,
|
||||
createMockApi,
|
||||
registerExtension,
|
||||
requireTool,
|
||||
pgDescribe,
|
||||
} from "./pg-extension-harness.js";
|
||||
|
||||
const h = createPgExtensionHarness("fn-node-override-wip");
|
||||
|
||||
/* A real lifecycle spine: column adjacency is derived from the graph, so an IR whose nodes do not
|
||||
cover a column cannot be moved into it and the setup moves would fail as if they were the subject. */
|
||||
const RENAMED_IR = {
|
||||
version: "v2",
|
||||
id: "node-override-lifecycle",
|
||||
name: "renamed",
|
||||
columns: [
|
||||
{ id: "backlog", name: "Planning", traits: [{ trait: "intake" }, { trait: "hold", config: { release: "capacity" } }] },
|
||||
{ id: "building", name: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] },
|
||||
{ id: "checking", name: "Review", traits: [{ trait: "merge-blocker" }, { trait: "human-review" }, { trait: "merge" }] },
|
||||
{ id: "shipped", name: "Done", traits: [{ trait: "complete" }] },
|
||||
],
|
||||
nodes: [
|
||||
{ id: "start", kind: "start", column: "backlog" },
|
||||
{ id: "exec", kind: "prompt", column: "building", config: { seam: "execute" } },
|
||||
{ id: "merge-gate", kind: "merge-gate", column: "checking", config: { gate: "auto-merge" } },
|
||||
{ id: "end", kind: "end", column: "shipped" },
|
||||
],
|
||||
edges: [
|
||||
{ from: "start", to: "exec" },
|
||||
{ from: "exec", to: "merge-gate", condition: "success" },
|
||||
{ from: "merge-gate", to: "end", condition: "success" },
|
||||
],
|
||||
} as unknown as WorkflowIr;
|
||||
|
||||
async function seedCardInRenamedWip(description: string): Promise<string> {
|
||||
const store = h.store();
|
||||
const definition = await store.createWorkflowDefinition({ name: "renamed node override", ir: RENAMED_IR as never });
|
||||
const task = await store.createTask({ description });
|
||||
await store.selectTaskWorkflow(task.id, definition.id);
|
||||
/* Created in the builtin intake, so it enters the custom board through `backlog`. */
|
||||
for (const lane of ["backlog", "building"]) {
|
||||
await store.moveTask(task.id, lane as never, { moveSource: "user" } as never);
|
||||
}
|
||||
/* The premise, asserted rather than assumed — see the header. */
|
||||
expect((await store.getTask(task.id)).column).toBe("building");
|
||||
return task.id;
|
||||
}
|
||||
|
||||
pgDescribe("fn_task_update node override respects the board's own wip lane", () => {
|
||||
beforeAll(h.beforeAll);
|
||||
beforeEach(h.beforeEach);
|
||||
afterEach(h.afterEach);
|
||||
afterAll(h.afterAll);
|
||||
|
||||
it("refuses a mid-flight node override for a card in a RENAMED wip lane", async () => {
|
||||
const api = createMockApi();
|
||||
registerExtension(api);
|
||||
const taskId = await seedCardInRenamedWip("renamed wip node override");
|
||||
|
||||
const updateTool = requireTool(api, "fn_task_update");
|
||||
const result = await updateTool.execute(
|
||||
"u1",
|
||||
{ id: taskId, nodeId: "merge-gate" },
|
||||
undefined,
|
||||
undefined,
|
||||
{ cwd: h.rootDir() },
|
||||
);
|
||||
|
||||
expect(result.isError).toBe(true);
|
||||
/* The reason CODE, not the message: this is the assertion that separates the CLI pre-check from
|
||||
the store's throw. See the header — with the pre-check unwired this line receives prose. */
|
||||
expect(result.details?.error).toBe("task-in-progress");
|
||||
expect(String(result.content?.[0]?.text ?? "")).toContain("routing cannot be changed mid-flight");
|
||||
|
||||
/* And it really did not write: the override must be absent, not merely reported as refused. */
|
||||
expect((await h.store().getTask(taskId)).nodeId ?? null).toBeNull();
|
||||
});
|
||||
|
||||
it("still allows the override once the card leaves that wip lane", async () => {
|
||||
const api = createMockApi();
|
||||
registerExtension(api);
|
||||
const taskId = await seedCardInRenamedWip("renamed wip node override, moved on");
|
||||
await h.store().moveTask(taskId, "checking" as never, { moveSource: "user" } as never);
|
||||
|
||||
const updateTool = requireTool(api, "fn_task_update");
|
||||
const result = await updateTool.execute(
|
||||
"u2",
|
||||
{ id: taskId, nodeId: "merge-gate" },
|
||||
undefined,
|
||||
undefined,
|
||||
{ cwd: h.rootDir() },
|
||||
);
|
||||
|
||||
expect(result.isError).toBeFalsy();
|
||||
expect((await h.store().getTask(taskId)).nodeId).toBe("merge-gate");
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user