diff --git a/packages/core/src/__tests__/node-override-guard.test.ts b/packages/core/src/__tests__/node-override-guard.test.ts index 06f18e4e60..6ce163af91 100644 --- a/packages/core/src/__tests__/node-override-guard.test.ts +++ b/packages/core/src/__tests__/node-override-guard.test.ts @@ -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)", () => { diff --git a/packages/core/src/node-override-guard.ts b/packages/core/src/node-override-guard.ts index 2911c387b2..f2f0353dd1 100644 --- a/packages/core/src/node-override-guard.ts +++ b/packages/core/src/node-override-guard.ts @@ -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[0], + taskId: string, +): Promise<{ wipColumns: Set; completeColumns: Set }> { + /* + 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(["in-progress"]), completeColumns: new Set(["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; + completeColumns?: ReadonlySet; /** * 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 }; diff --git a/packages/core/src/task-store/branch-and-pr-entities.ts b/packages/core/src/task-store/branch-and-pr-entities.ts index 272374ffac..144359ac6b 100644 --- a/packages/core/src/task-store/branch-and-pr-entities.ts +++ b/packages/core/src/task-store/branch-and-pr-entities.ts @@ -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); diff --git a/packages/core/src/task-store/task-update.ts b/packages/core/src/task-store/task-update.ts index 6b784fbe9d..a710922d19 100644 --- a/packages/core/src/task-store/task-update.ts +++ b/packages/core/src/task-store/task-update.ts @@ -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); } diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 38de605566..66e44a7221 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -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,