batch-core: node-override guards let a running task be re-routed on a renamed board (45 → 43) (#2821)
## The defect Two guards in `node-override-guard.ts` answered a **role** question with a **column name**: - **`task.column === "in-progress"`** refuses changing a task's node override mid-flight. On a renamed board it never matched, so an operator could re-route a **running** task — precisely what the guard exists to prevent, and the failure is silent because the guard simply returns `allowed: true`. - **`task.column !== "done"`** gates overriding *to* the terminal node. On a renamed board it never matched either, so the override was refused for exactly the tasks that had legitimately reached the end node. Both fail in the direction that looks like normal behaviour rather than an error. ## Why the lanes are injected rather than resolved in place `validateNodeOverrideChange` is **synchronous by design**, and its existing `isTerminalNodeId` option already establishes the pattern: callers with cheap IR access inject, callers without keep a documented literal fallback. **Both production callers now supply the lanes** — `branch-and-pr-entities.ts:594` (which already injected `isTerminalNodeId`) and `task-update.ts:53`. That was the deciding factor: an optional parameter that only tests fill is the inert-injection shape this program keeps finding, where a guard reads as converted, its test passes because the test injects the value, and production keeps the literal. I checked both call sites had a store in scope *before* adding the option. `resolveNodeOverrideLanes` lives beside the guard rather than in the callers, so the two cannot drift about what "executing" and "completed" mean. ## Fallbacks A workflow expressing **no trait on any column** is a v1 upgrade — `synthesizeDefaultColumns` emits `traits: []` everywhere — not a board without these roles, so it keeps the legacy ids. Same for an unresolvable workflow. Both preserve exactly the behaviour the literals already had. ## Verification - **Mutation-verified per guard:** restoring `task.column === "in-progress"` fails a case; restoring `task.column !== "done"` fails a different one. - The suite also pins the paired negative — resolving lanes must not turn the guard into a blanket refusal for a task outside every WIP lane. - `node-override-guard.test.ts` → 27 passed - `pnpm test:gate` → 161 + 487 + 13 + 71 - `--strict` → exit 0; `tsc --noEmit` and `pnpm lint` → 0 errors Census: batch-core scope **45 → 43**; repo total **255**. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Node overrides now correctly recognize workflow-defined in-progress and completed lanes, including renamed columns. - Override validation falls back safely for legacy or unresolved workflows. - Prevented validation from using stale task-column information during updates. - **Tests** - Added coverage for workflow lane resolution, legacy fallbacks, renamed lanes, and override eligibility. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
@@ -1,6 +1,123 @@
|
||||
import { describe, expect, it } from "vitest";
|
||||
|
||||
import { validateNodeOverrideChange } from "../node-override-guard.js";
|
||||
import { validateNodeOverrideChange, resolveNodeOverrideLanes } from "../node-override-guard.js";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-22:35 (batch-core):
|
||||
|
||||
BOTH GUARDS ANSWERED A ROLE QUESTION WITH A COLUMN NAME.
|
||||
|
||||
- "is this task executing right now?" refused a mid-flight override. Keyed on `in-progress`, a
|
||||
renamed board let an operator re-route a RUNNING task — precisely what the guard exists to stop.
|
||||
- the terminal-node gate asks whether the task has COMPLETED. Keyed on `done`, a renamed board
|
||||
refused the override for exactly the tasks that had legitimately reached the end node.
|
||||
|
||||
The guard is synchronous by design, so the lanes are injected — and both production callers
|
||||
(`branch-and-pr-entities.ts` and `task-update.ts`) now resolve and pass them, which is what keeps
|
||||
this from being an option only tests supply.
|
||||
*/
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-23:30 (#2821 review — greptile):
|
||||
|
||||
THE RESOLVER'S OWN CONTRACT, which the guard-level cases above cannot reach.
|
||||
|
||||
Those pass the sets in by hand, so they pin what `validateNodeOverrideChange` does with a set and say
|
||||
nothing about how the set is BUILT. The floor bug lived in the builder: seeding the legacy ids and
|
||||
adding resolved lanes on top meant a v2 board that declares `in-progress` as an ordinary untraited
|
||||
column still had it counted as WIP. Mutating the resolver back to a floor left every guard-level case
|
||||
green — which is exactly why this suite needs a resolver-level one.
|
||||
*/
|
||||
describe("resolveNodeOverrideLanes builds the set from traits, with legacy as an ELSE", () => {
|
||||
const storeFor = (ir: unknown) => {
|
||||
const selection = { workflowId: "wf", stepIds: [] as string[] };
|
||||
return {
|
||||
getTaskWorkflowSelection: () => selection,
|
||||
getTaskWorkflowSelectionAsync: async () => selection,
|
||||
getWorkflowDefinition: async () => (ir === undefined ? undefined : { id: "wf", ir }),
|
||||
} as never;
|
||||
};
|
||||
|
||||
it("EXCLUDES a legacy-named column the board declares without the trait", async () => {
|
||||
const ir = {
|
||||
version: "v2", id: "wf", name: "wf", nodes: [], edges: [],
|
||||
columns: [
|
||||
{ id: "in-progress", name: "Not actually wip", traits: [] },
|
||||
{ id: "building", name: "Building", traits: [{ trait: "wip" }] },
|
||||
{ id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] },
|
||||
],
|
||||
};
|
||||
const lanes = await resolveNodeOverrideLanes(storeFor(ir), "FN-1");
|
||||
|
||||
expect([...lanes.wipColumns]).toEqual(["building"]);
|
||||
expect(lanes.wipColumns.has("in-progress")).toBe(false);
|
||||
expect([...lanes.completeColumns]).toEqual(["shipped"]);
|
||||
});
|
||||
|
||||
it("falls back to the legacy ids for a V1-UPGRADED board that traits nothing", async () => {
|
||||
const v1 = {
|
||||
version: "v2", id: "wf", name: "wf", nodes: [], edges: [],
|
||||
columns: ["todo", "in-progress", "done"].map((id) => ({ id, name: id, traits: [] })),
|
||||
};
|
||||
const lanes = await resolveNodeOverrideLanes(storeFor(v1), "FN-1");
|
||||
|
||||
expect([...lanes.wipColumns]).toEqual(["in-progress"]);
|
||||
expect([...lanes.completeColumns]).toEqual(["done"]);
|
||||
});
|
||||
|
||||
it("falls back to the legacy ids when the workflow cannot be resolved", async () => {
|
||||
const lanes = await resolveNodeOverrideLanes(storeFor(undefined), "FN-1");
|
||||
expect([...lanes.wipColumns]).toEqual(["in-progress"]);
|
||||
expect([...lanes.completeColumns]).toEqual(["done"]);
|
||||
});
|
||||
});
|
||||
|
||||
describe("node override lanes are resolved, not named", () => {
|
||||
const RENAMED = { wipColumns: new Set(["building"]), completeColumns: new Set(["shipped"]) };
|
||||
|
||||
it("refuses a mid-flight override for a task in a RENAMED wip lane", () => {
|
||||
const result = validateNodeOverrideChange(
|
||||
{ id: "FN-1", column: "building" } as never, "some-node", RENAMED,
|
||||
);
|
||||
expect(result.allowed).toBe(false);
|
||||
expect(result.reason).toBe("task-in-progress");
|
||||
});
|
||||
|
||||
it("still ALLOWS an override for a task outside every wip lane", () => {
|
||||
/* The paired negative: resolving lanes must not turn the guard into a blanket refusal. */
|
||||
const result = validateNodeOverrideChange(
|
||||
{ id: "FN-2", column: "backlog" } as never, "some-node", RENAMED,
|
||||
);
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-23:10 (#2821 review — greptile):
|
||||
A LEGACY NAME THE BOARD DOES NOT TRAIT IS NOT THAT ROLE.
|
||||
|
||||
The first version SEEDED the legacy ids and added the resolved lanes on top, so a v2 board that
|
||||
declares `in-progress` as an ordinary untraited column still had it treated as WIP — blocking a
|
||||
mid-flight override that the board's own traits say is fine. The fallback has to be an ELSE, not a
|
||||
floor.
|
||||
|
||||
This drives the resolved sets directly (the guard is synchronous and takes them), so it pins the
|
||||
contract the resolver must honour.
|
||||
*/
|
||||
it("ALLOWS an override for a legacy-named column the board does not trait as wip", () => {
|
||||
const result = validateNodeOverrideChange(
|
||||
{ id: "FN-4", column: "in-progress" } as never,
|
||||
"some-node",
|
||||
{ wipColumns: new Set(["building"]), completeColumns: new Set(["shipped"]) },
|
||||
);
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
|
||||
it("permits a terminal-node override for a task finished in a RENAMED complete lane", () => {
|
||||
const result = validateNodeOverrideChange(
|
||||
{ id: "FN-3", column: "shipped" } as never, "end", RENAMED,
|
||||
);
|
||||
expect(result.allowed).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
describe("validateNodeOverrideChange", () => {
|
||||
it("allows when newNodeId is undefined (not being changed)", () => {
|
||||
|
||||
@@ -1,3 +1,45 @@
|
||||
import { columnsWithFlag, declaresAnyLifecycleTrait } from "./workflow-lifecycle-traits.js";
|
||||
import { resolveWorkflowIrForTask } from "./workflow-ir-resolver.js";
|
||||
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-22:30 (batch-core):
|
||||
The async companion to `validateNodeOverrideChange`, which is deliberately synchronous. Lives beside
|
||||
the guard so the two cannot drift: a caller that resolves lanes some other way would eventually
|
||||
disagree with what the guard means by "executing" or "completed".
|
||||
|
||||
A workflow expressing no trait at all is a v1 upgrade rather than a board without these roles, so it
|
||||
keeps the legacy ids — as does an unresolvable one. Both are the behaviour the literals already had.
|
||||
*/
|
||||
export async function resolveNodeOverrideLanes(
|
||||
store: Parameters<typeof resolveWorkflowIrForTask>[0],
|
||||
taskId: string,
|
||||
): Promise<{ wipColumns: Set<string>; completeColumns: Set<string> }> {
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-23:05 (#2821 review — greptile):
|
||||
THE LEGACY IDS ARE A FALLBACK, NOT A FLOOR. My first version SEEDED them and then added the
|
||||
resolved lanes, so a v2 board that declares `in-progress` or `done` as an ORDINARY untraited column
|
||||
still had them treated as wip/complete — blocking a valid mid-flight override in the first and
|
||||
refusing a terminal override in the second. A conversion that widens a guard onto columns the board
|
||||
says are not those roles is a regression, not a fallback.
|
||||
|
||||
Three states, the same split this program settled on elsewhere:
|
||||
resolved + traits expressed -> trust the resolved lanes ALONE.
|
||||
resolved + no trait anywhere -> a v1 upgrade (`synthesizeDefaultColumns` emits `traits: []`), so
|
||||
the legacy ids are the only vocabulary that exists.
|
||||
unresolvable -> legacy ids; today's behaviour.
|
||||
*/
|
||||
const legacy = { wipColumns: new Set<string>(["in-progress"]), completeColumns: new Set<string>(["done"]) };
|
||||
try {
|
||||
const ir = await resolveWorkflowIrForTask(store, taskId);
|
||||
if (!ir || !declaresAnyLifecycleTrait(ir)) return legacy;
|
||||
return {
|
||||
wipColumns: new Set(columnsWithFlag(ir, "countsTowardWip")),
|
||||
completeColumns: new Set(columnsWithFlag(ir, "complete")),
|
||||
};
|
||||
} catch {
|
||||
return legacy;
|
||||
}
|
||||
}
|
||||
export type NodeOverrideBlockReason = "task-in-progress" | "terminal-without-merge-proof";
|
||||
|
||||
export interface NodeOverrideValidationResult {
|
||||
@@ -27,6 +69,17 @@ export interface NodeOverrideTaskInput {
|
||||
}
|
||||
|
||||
export interface NodeOverrideValidationOptions {
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-22:20 (batch-core):
|
||||
The task's resolved WIP and COMPLETE lanes. This guard is SYNCHRONOUS and both its production
|
||||
callers already await a store before reaching it, so the lanes come in rather than being resolved
|
||||
here — the same shape `isTerminalNodeId` already uses for the same reason.
|
||||
|
||||
Both callers supply them. An omitted set keeps the legacy id, which is what a caller without cheap
|
||||
IR access (a CLI tool, a route with only a task row) still gets.
|
||||
*/
|
||||
wipColumns?: ReadonlySet<string>;
|
||||
completeColumns?: ReadonlySet<string>;
|
||||
/**
|
||||
* Resolve whether `nodeId` is the task workflow's terminal `end` node.
|
||||
* Callers with access to the task's resolved workflow IR (e.g.
|
||||
@@ -50,7 +103,12 @@ export function validateNodeOverrideChange(
|
||||
return { allowed: true };
|
||||
}
|
||||
|
||||
if (task.column === "in-progress") {
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-22:20 (batch-core):
|
||||
"Is this task executing right now?" — keyed on the literal, a renamed board let an operator change
|
||||
the node override MID-FLIGHT on a running task, which is exactly what this guard exists to refuse.
|
||||
*/
|
||||
if ((options?.wipColumns ?? new Set(["in-progress"])).has(task.column)) {
|
||||
return {
|
||||
allowed: false,
|
||||
reason: "task-in-progress",
|
||||
@@ -72,7 +130,13 @@ export function validateNodeOverrideChange(
|
||||
const isTerminal =
|
||||
newNodeId !== null &&
|
||||
(options?.isTerminalNodeId ? options.isTerminalNodeId(newNodeId) : defaultIsTerminalNodeId(newNodeId));
|
||||
if (isTerminal && task.column !== "done") {
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-22:20 (batch-core):
|
||||
The terminal-node gate asks whether the task has already COMPLETED. On a renamed board a finished
|
||||
task never matched, so overriding to the terminal node was refused for exactly the tasks that had
|
||||
legitimately reached it.
|
||||
*/
|
||||
if (isTerminal && !(options?.completeColumns ?? new Set(["done"])).has(task.column)) {
|
||||
const mergeConfirmed = task.mergeDetails?.mergeConfirmed === true;
|
||||
if (mergeConfirmed) {
|
||||
return { allowed: true, requiresFinalize: true };
|
||||
|
||||
@@ -19,7 +19,7 @@ import { ensureBranchGroupForSource as ensureBranchGroupForSourceAsync, ensurePr
|
||||
import { getWorkflowWorkItem as getWorkflowWorkItemAsync } from "./async-workflow-workitems.js";
|
||||
import { MergeRequestRow, PrEntityRow, WorkflowWorkItemRow } from "./row-types.js";
|
||||
import { BranchGroup, BranchGroupCreateInput, ColumnId, MergeRequestRecord, MergeRequestState, PrEntity, PrEntityCreateInput, PrThreadOutcome, PrThreadState, RunMutationContext, Task, TaskLogEntry, TaskPriority, TaskVerificationRequest, TaskVerificationResultSummary, TaskVerificationStatus, WorkflowWorkItem, WorkflowWorkItemKind, WorkflowWorkItemState, WorkflowWorkItemTransitionPatch } from "../types.js";
|
||||
import { validateNodeOverrideChange } from "../node-override-guard.js";
|
||||
import { validateNodeOverrideChange, resolveNodeOverrideLanes } from "../node-override-guard.js";
|
||||
import { WorkflowMovePolicyInput } from "../workflow-extension-types.js";
|
||||
import { resolveWorkflowIrById } from "../workflow-ir-resolver.js";
|
||||
import { resolveTaskLifecycleColumns } from "../workflow-lifecycle-traits.js";
|
||||
@@ -589,10 +589,27 @@ export async function updateTaskImpl(store: TaskStore,
|
||||
explicit error instead of letting updateTaskUnlocked write a no-op nodeId field.
|
||||
*/
|
||||
if (updates.nodeId !== undefined) {
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-30-23:20 (#2821 review — greptile):
|
||||
THE COLUMN IS READ AFTER THE AWAIT, NOT BEFORE IT.
|
||||
|
||||
My first version read the task, then awaited lane resolution, then validated — so a move landing
|
||||
in that window was judged with a STALE column against freshly resolved lanes. The dangerous
|
||||
direction is the obvious one: a task that entered a WIP lane during the gap still carried its
|
||||
pre-move column, and the mid-flight guard passed for a task that had started running.
|
||||
|
||||
Resolving the lanes FIRST closes it. `resolveNodeOverrideLanes` needs only the task id, so the
|
||||
order is free, and the column then comes from the latest read before validation. This does not
|
||||
make the check atomic — `updateTaskUnlocked` runs outside the per-task lock by design, as the
|
||||
note above explains — but it removes the window this change introduced rather than leaving a
|
||||
new one behind a resolved-lane improvement.
|
||||
*/
|
||||
const overrideLanes = await resolveNodeOverrideLanes(store, id);
|
||||
const currentTask = await store.getTask(id).catch(() => null);
|
||||
if (currentTask) {
|
||||
const validation = validateNodeOverrideChange(currentTask, updates.nodeId ?? null, {
|
||||
isTerminalNodeId: (nodeId) => isTaskTerminalNodeIdImpl(store, id, nodeId),
|
||||
...overrideLanes,
|
||||
});
|
||||
if (!validation.allowed) {
|
||||
throw new Error(validation.message);
|
||||
|
||||
@@ -16,7 +16,7 @@ import type {Task, Column, TaskLogEntry, RunMutationContext} from "../types.js";
|
||||
import {validateCustomFieldPatch, CustomFieldRejectionError} from "../task-fields.js";
|
||||
import "../builtin-traits.js";
|
||||
import {normalizeTaskPriority} from "../task-priority.js";
|
||||
import {validateNodeOverrideChange} from "../node-override-guard.js";
|
||||
import {validateNodeOverrideChange, resolveNodeOverrideLanes} from "../node-override-guard.js";
|
||||
import {extractTaskIdTokens, normalizeTitleForTaskId} from "../task-title-id-drift.js";
|
||||
import {buildBootstrapPrompt} from "../mesh-task-replication.js";
|
||||
import {validateFileScopeInPromptContent} from "../task-store/file-scope.js";
|
||||
@@ -50,7 +50,36 @@ export async function updateTaskUnlockedImpl(store: TaskStore, id: string, updat
|
||||
const preUpdateDescription = task.description;
|
||||
|
||||
if (updates.nodeId !== undefined) {
|
||||
const validation = validateNodeOverrideChange(task, updates.nodeId ?? null);
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-31-00:40 (#2821 review — greptile, second call site):
|
||||
THE COLUMN IS RE-READ AFTER THE AWAIT.
|
||||
|
||||
`task` was loaded above, and awaiting lane resolution here opened a window: another process
|
||||
moving the card into a resolved WIP lane during that await left the guard judging a STALE
|
||||
non-WIP column, so the mid-flight refusal passed for a task that had started running.
|
||||
|
||||
I fixed exactly this at the sibling call site in `branch-and-pr-entities.ts` by resolving lanes
|
||||
BEFORE the task read, and missed it here — the same half-conversion this program keeps
|
||||
finding, in my own fix. Hoisting is not available at this site because `task` is the working
|
||||
copy the whole function mutates, so the column is re-read instead and only for the guard.
|
||||
|
||||
The re-read is best-effort: if it fails, the already-loaded copy is used, which is strictly no
|
||||
worse than before this change.
|
||||
*/
|
||||
const overrideLanes = await resolveNodeOverrideLanes(store, id);
|
||||
/*
|
||||
FNXC:WorkflowResolvedColumns 2026-07-31-01:50 (#2821 review — greptile, and it caught a DEADLOCK I shipped):
|
||||
RE-READ WITHOUT THE LOCK. My previous version used `store.getTask(id)`, which acquires the
|
||||
per-task lock. This function is `updateTaskUnlockedImpl` — the caller ALREADY HOLDS that lock,
|
||||
and it is non-reentrant, so the inner read waited on the outer update forever. A stale-column
|
||||
race is a narrow window; a deadlock is every `nodeId` update.
|
||||
|
||||
`readTaskJson` is the lock-free read this function already uses for its own working copy, so
|
||||
the column is refreshed after the await without touching the lock. Falls back to the copy
|
||||
loaded above if the re-read fails, which is no worse than before.
|
||||
*/
|
||||
const freshForGuard = await store.readTaskJson(dir).catch(() => null);
|
||||
const validation = validateNodeOverrideChange(freshForGuard ?? task, updates.nodeId ?? null, overrideLanes);
|
||||
if (!validation.allowed) {
|
||||
throw new Error(validation.message);
|
||||
}
|
||||
|
||||
@@ -13,7 +13,6 @@
|
||||
"packages/dashboard/src/routes/register-task-workflow-routes.ts": 3,
|
||||
"packages/engine/src/planner-overseer.ts": 3,
|
||||
"packages/core/src/agent-store.ts": 2,
|
||||
"packages/core/src/node-override-guard.ts": 2,
|
||||
"packages/core/src/task-store/audit-ops.ts": 2,
|
||||
"packages/core/src/task-store/moves.ts": 2,
|
||||
"packages/core/src/task-store/project-store-ops.ts": 2,
|
||||
|
||||
Reference in New Issue
Block a user