diff --git a/packages/core/src/__tests__/agent-dispatch-renamed-lanes.test.ts b/packages/core/src/__tests__/agent-dispatch-renamed-lanes.test.ts new file mode 100644 index 0000000000..cc1b951d0e --- /dev/null +++ b/packages/core/src/__tests__/agent-dispatch-renamed-lanes.test.ts @@ -0,0 +1,185 @@ +// @vitest-environment node +/* +FNXC:WorkflowResolvedColumns 2026-07-30-14:50 (fleet phase — the dispatcher's own filters): +`selectNextTaskForAgentImpl` picks an agent's next task by filtering the board for its WIP lane, then its +hold lane. Both were `task.column === ""`. + +THE FAILURE: on a board whose lanes are renamed, both filters match nothing, so an agent asking for work +is told there is none — with its own assigned tasks sitting right there in the list it just fetched. No +error, no log line. The agent simply idles. + +`agent-heartbeat-worktree-renamed-hold.test.ts` covers the requeue TARGET on a renamed board; nothing +covered the dispatcher's SELECTION filters, which is why this file exists rather than a case added there. + +WHY THE IMPL DIRECTLY. The existing `selectNextTaskForAgent` coverage in +`agent-store-routing-policy.test.ts` drives a real store harness, so exercising a renamed vocabulary there +means registering a real custom workflow and moving cards through it. This test is about which lane the +filters name, so it calls the impl with a store fake that resolves a renamed IR — the same shape used for +the reconciler in #2737. The bind evaluator is exercised for real; only the store is faked — and that claim is now TRUE, which it +was not when first written. `selectNextTaskForAgentImpl`'s `agent` argument is OPTIONAL and I omitted it, so +`isBindCompatible` hit its `if (!agent) return true` short-circuit and `evaluateImplementationTaskBind` never +ran. The header asserted coverage the invocation did not produce — the same "comment claims what the code +does not do" defect this program keeps finding, in my own test. Every case now passes an executor agent. + +REVERT CHECK, measured (both run): restoring `task.column === "in-progress"` fails the WIP case with +`expected null to be truthy`; restoring `task.column === "todo"` fails the hold case the same way. The +default-vocabulary cases pass either way, which is why both vocabularies run. +*/ +import { describe, expect, it } from "vitest"; +import type { Task, TaskStore, WorkflowIr } from "../types.js"; +import { selectNextTaskForAgentImpl } from "../task-store/branch-group-ops.js"; + +const AGENT_ID = "agent-1"; + +/** A real agent, so `isBindCompatible` runs `evaluateImplementationTaskBind` instead of short-circuiting. */ +const EXECUTOR_AGENT = { id: AGENT_ID, role: "executor" } as never; + +/** One workflow shape, two vocabularies — only the column ids differ. */ +function ir(wip: string, hold: string, complete: string): WorkflowIr { + return { + version: "v2", + id: "wf-dispatch", + name: "dispatch", + nodes: [], + edges: [], + columns: [ + { id: hold, name: "Hold", traits: [{ trait: "intake" }, { trait: "hold", config: { release: "capacity" } }] }, + { id: wip, name: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: complete, name: "Complete", traits: [{ trait: "complete" }] }, + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:15 (#2739 review — greptile P1): + A SECOND complete lane. `resolveLifecycleColumns` returns the FIRST column per trait, so a dependency + resting here was not counted as satisfied and its dependent was dropped from dispatch entirely. + Declared on every vocabulary so the case below is not special-cased to a renamed board. + */ + { id: `${complete}-signoff`, name: "Signed off", traits: [{ trait: "complete" }] }, + ], + } as unknown as WorkflowIr; +} + +function makeStore(tasks: Task[], workflowIr: WorkflowIr): TaskStore { + return { + listTasks: async () => tasks, + getTaskWorkflowSelection: () => ({ workflowId: "wf-dispatch", stepIds: [] }), + getWorkflowDefinition: async () => ({ ir: workflowIr }), + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:25 (#2739 review — a fake that hid the thing under test): + REAL semantics, mirroring `areAllDependenciesDoneImpl`: a dependency is satisfied only if its column is + in the `satisfiedColumns` set the caller computed. The previous `() => true` made every dependency + trivially done, so the membership fix and the first-per-role bug were indistinguishable — the new case + passed with the change reverted, which is exactly the vacuous-test failure this program keeps finding. + */ + areAllDependenciesDone: ( + dependencies: string[], + tasksById: Map, + satisfiedColumns?: ReadonlySet, + ) => dependencies.every((id) => { + const dependency = tasksById.get(id); + return dependency !== undefined + && (satisfiedColumns ?? new Set(["done", "archived"])).has(dependency.column); + }), + } as unknown as TaskStore; +} + +function task(overrides: Partial & { id: string; column: string }): Task { + return { + title: overrides.id, + description: "work", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + assignedAgentId: AGENT_ID, + createdAt: "2026-07-31T00:00:00.000Z", + updatedAt: "2026-07-31T00:00:00.000Z", + columnMovedAt: "2026-07-31T00:00:00.000Z", + ...overrides, + } as unknown as Task; +} + +const LINEAGES = [ + { label: "DEFAULT", wip: "in-progress", hold: "todo", complete: "done" }, + { label: "RENAMED", wip: "building", hold: "backlog", complete: "shipped" }, +] as const; + +describe("agent dispatch selects by lifecycle ROLE, not by column id", () => { + for (const { label, wip, hold, complete } of LINEAGES) { + it(`resumes an in-progress assigned task on a ${label} WIP lane (${wip})`, async () => { + const store = makeStore([task({ id: "FN-1", column: wip })], ir(wip, hold, complete)); + + const selected = await selectNextTaskForAgentImpl(store, AGENT_ID, EXECUTOR_AGENT); + + expect(selected, `${label} lineage selected nothing`).toBeTruthy(); + expect(selected?.task?.id).toBe("FN-1"); + expect(selected?.priority).toBe("in_progress"); + }); + + it(`picks up a queued assigned task on a ${label} hold lane (${hold})`, async () => { + const store = makeStore([task({ id: "FN-2", column: hold })], ir(wip, hold, complete)); + + const selected = await selectNextTaskForAgentImpl(store, AGENT_ID, EXECUTOR_AGENT); + + expect(selected, `${label} lineage selected nothing`).toBeTruthy(); + expect(selected?.task?.id).toBe("FN-2"); + }); + } + + it("still skips an operator-parked hold task on a renamed board", async () => { + /* + Non-vacuous guard on the hold filter: it must keep its `userPaused` exclusion, not just its lane. + Without this, a filter that matched every column would satisfy both cases above. + */ + const store = makeStore( + [task({ id: "FN-3", column: "backlog", userPaused: true } as never)], + ir("building", "backlog", "shipped"), + ); + + expect(await selectNextTaskForAgentImpl(store, AGENT_ID, EXECUTOR_AGENT)).toBeNull(); + }); + + it("DOCUMENTS A DEFECT: the role-routing policy does not apply on a renamed board", async () => { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-15:20 (#2739 review — the claim, and what proving it exposed): + Passing an agent is not evidence the evaluator RAN: an executor agent is allowed, so a short-circuit and + a real evaluation are indistinguishable from a passing case. So this asserts a `custom`-role agent is + REFUSED an implementation task on a renamed lane. + + It failed — the task was handed over — and the cause is production, not the test. + `isImplementationTask` tests Set membership over the hardcoded ids {triage, todo, in-progress, ...}, and + `evaluateImplementationTaskBind` short-circuits to `allowed: true` when that is false. On a renamed + board EVERY agent is therefore bind-compatible with EVERY task, and the role check that stops a liaison + being handed implementation work does not apply at all. + + This case is written to the CURRENT behaviour and named as documenting a defect, so it does not sit red. + Flip the expectation when the policy resolves lanes by role; the reasoning is recorded at + `agent-role-policy.ts`'s `IMPLEMENTATION_TASK_COLUMNS`. + */ + const store = makeStore([task({ id: "FN-9", column: "building" })], ir("building", "backlog", "shipped")); + + const selected = await selectNextTaskForAgentImpl(store, AGENT_ID, { id: AGENT_ID, role: "custom" } as never); + + // Current behaviour, not desired behaviour: the bind check is bypassed, so the task IS selected. + expect(selected?.task?.id).toBe("FN-9"); + }); + + it("a dependency in a SECOND complete lane satisfies the dependent (#2739 review)", async () => { + /* + The dependency loop unions terminal columns across dependencies, but added only the resolver's single + canonical `complete`/`archived` ids. A workflow with two complete lanes therefore left a finished + blocker reading as unfinished, and the dependent silently never dispatched. + + REVERT CHECK, measured: putting back + `if (lifecycle?.complete) satisfiedColumns.add(lifecycle.complete)` + in place of `columnsWithFlag(ir, "complete")` fails this with `expected null to be truthy` — the + dependent is withheld because its blocker sits in `shipped-signoff`. + */ + const dependency = task({ id: "FN-DEP", column: "shipped-signoff", assignedAgentId: undefined } as never); + const dependent = task({ id: "FN-8", column: "backlog", dependencies: ["FN-DEP"] } as never); + const store = makeStore([dependent, dependency], ir("building", "backlog", "shipped")); + + const selected = await selectNextTaskForAgentImpl(store, AGENT_ID, EXECUTOR_AGENT); + + expect(selected, "dependent withheld: its blocker's terminal lane was not counted").toBeTruthy(); + expect(selected?.task?.id).toBe("FN-8"); + }); +}); diff --git a/packages/core/src/agent-role-policy.ts b/packages/core/src/agent-role-policy.ts index c71df74a57..8a1d5d9fc6 100644 --- a/packages/core/src/agent-role-policy.ts +++ b/packages/core/src/agent-role-policy.ts @@ -1,5 +1,31 @@ import type { Agent, Task } from "./types.js"; +/* +FNXC:WorkflowResolvedColumns 2026-07-30-15:20 (FLAGGED, NOT FIXED — found by a #2739 review thread): +THE ROLE-ROUTING POLICY IS BYPASSED ENTIRELY ON A RENAMED BOARD. + +`isImplementationTask` is a Set membership test over these hardcoded ids, and +`evaluateImplementationTaskBind` short-circuits to `{ allowed: true }` when it returns false. So on a +workflow whose lanes are named anything else, EVERY agent is bind-compatible with EVERY task: the role +check that exists to stop a liaison/custom agent being handed implementation work — the NEXT-871 loop +FN-7851 fixed — silently does not apply. + +HOW IT SURFACED, which is the part worth keeping: a reviewer noticed my dispatch test claimed to exercise +the bind evaluator while omitting the optional `agent` argument. Passing a real agent was not enough to +prove the evaluator RAN, so I added a case asserting a `custom`-role agent is refused an implementation +task on a renamed lane. It FAILED — the task was handed over — and the cause is this Set, not the test. + +WHY THE CENSUS NEVER FLAGGED IT: these are Set MEMBERS, not comparisons. The lifecycle-column census scans +`===`/`!==` against a column, so a literal collection is invisible to it — the same blind spot that hid the +raw-`sql` encoding of the archived gate (PR #2724). Worth knowing that the backlog number is a floor, not a +total. + +NOT FIXED HERE. `isImplementationTask` is a SYNC pure predicate with no store and no task id, called from +`evaluateImplementationTaskBind`, which gates agent assignment; resolving a workflow inside it means +threading a resolver through the routing policy and making its callers async. That is a behaviour change to +agent admission, not a vocabulary conversion, and the failure mode of getting it wrong is either handing +implementation work to a liaison or refusing work to a valid executor. +*/ const IMPLEMENTATION_TASK_COLUMNS: ReadonlySet = new Set([ "triage", "todo", diff --git a/packages/core/src/task-store/branch-group-ops.ts b/packages/core/src/task-store/branch-group-ops.ts index 4ea853f977..da9b6a885d 100644 --- a/packages/core/src/task-store/branch-group-ops.ts +++ b/packages/core/src/task-store/branch-group-ops.ts @@ -7,7 +7,8 @@ * instance as its first parameter and performs byte-identical work. */ import {TaskStore} from "../store.js"; -import {resolveTaskLifecycleColumns} from "../workflow-lifecycle-traits.js"; +import {resolveTaskLifecycleColumns, columnsWithFlag} from "../workflow-lifecycle-traits.js"; +import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; import type {WorkflowIr} from "../workflow-ir-types.js"; import type {Task, ColumnId, ArtifactType, ArtifactWithTask, InboxTask, TaskLogEntry, RunMutationContext, Agent} from "../types.js"; import {runReconciliationAbort} from "../workflow-reconciliation.js"; @@ -114,8 +115,32 @@ export async function selectNextTaskForAgentImpl(store: TaskStore, agentId: stri const assignedTasks = tasks.filter((task) => task.assignedAgentId === agentId); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:50 (fleet phase; #2739 review — greptile P2): + The dispatcher's OWN lane filters, resolved for this agent's tasks only. On a renamed board both + matched nothing, so an agent asking for work was told there was none with its own assigned tasks + sitting in the list it just fetched — no error, no log, the agent simply idles. + + Scoped to `assignedTasks` deliberately: an earlier version walked the whole board before filtering, so + a 400-card board paid 400 resolutions to dispatch one agent that owns three. `assignedAgentId` is a + plain property read and was already the next filter, so hoisting it costs nothing. + + ONE cache across BOTH lifecycle loops. An earlier version of this comment claimed the dependency pass + below shared it while the code allocated a second map, so a task and its dependency in the same workflow + resolved that workflow's IR twice — the comment asserted an optimisation the code did not perform. + `lifecycleIrCache` is now the only cache and the dependency loop takes it. + */ + const lifecycleIrCache = new Map(); + const lifecycleByTaskId = new Map>>(); + for (const task of assignedTasks) { + if (lifecycleByTaskId.has(task.id)) continue; + lifecycleByTaskId.set(task.id, await resolveTaskLifecycleColumns(store, task.id, lifecycleIrCache)); + } + const isWipTask = (task: Task) => task.column === (lifecycleByTaskId.get(task.id)?.wip ?? "in-progress"); + const isHoldTask = (task: Task) => task.column === (lifecycleByTaskId.get(task.id)?.hold ?? "todo"); + const inProgress = assignedTasks - .filter((task) => task.column === "in-progress" && isBindCompatible(task)) + .filter((task) => isWipTask(task) && isBindCompatible(task)) .sort(sortByOldestColumnMove); if (inProgress.length > 0) { return { @@ -139,19 +164,35 @@ export async function selectNextTaskForAgentImpl(store: TaskStore, agentId: stri legacy ids, matching the answer settled in #2720 and used by the merge blocker. */ const satisfiedColumns = new Set(["done", "archived"]); - const satisfiedIrCache = new Map(); + /* FNXC:WorkflowResolvedColumns 2026-07-30-14:50 (#2739 review): reuses the dispatch cache above, so a + task and its dependency in one workflow read that IR once between them. */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:10 (#2739 review — greptile P1, MEMBERSHIP not first-per-role): + `resolveLifecycleColumns` returns the FIRST column carrying each trait, so a workflow declaring two + complete lanes (or a complete plus a separate sign-off-complete) had only one of them counted as + satisfying a dependency. A dependent whose blocker landed in the SECOND terminal lane read as unfinished + and was dropped from both ready and actionable-blocked dispatch — silently, since "no work available" is + indistinguishable from "correctly waiting". + + Fixed locally with `columnsWithFlag`, and worth distinguishing from the arity gap I declined to fix on + the earlier thread. That one asked a single-id question (`lifecycle.wip`) where making it membership + changes what the shared resolver returns for ~30 consumers. THIS loop already builds a SET and already + unions across dependencies, so membership is what it was always trying to express — no resolver change, + no consumer migration, and it matches the `register-task-workflow-routes` precedent. + */ for (const dependencyId of new Set( roleCompatibleAssignedTasks.flatMap((task) => task.dependencies ?? []), )) { - const lifecycle = await resolveTaskLifecycleColumns(store, dependencyId, satisfiedIrCache); - if (lifecycle?.complete) satisfiedColumns.add(lifecycle.complete); - if (lifecycle?.archived) satisfiedColumns.add(lifecycle.archived); + const ir = await resolveWorkflowIrForTask(store, dependencyId, lifecycleIrCache); + if (!ir) continue; + for (const columnId of columnsWithFlag(ir, "complete")) satisfiedColumns.add(columnId); + for (const columnId of columnsWithFlag(ir, "archived")) satisfiedColumns.add(columnId); } const isDoneLike = (task: Task | undefined) => task !== undefined && satisfiedColumns.has(task.column); /** FNXC:TaskDispatch 2026-07-19-14:40: remembered ownership must not reselect an operator-parked task when `userPaused` remains true but legacy `paused` is false. */ const todoCandidates = roleCompatibleAssignedTasks.filter( - (task) => task.column === "todo" && task.paused !== true && task.userPaused !== true, + (task) => isHoldTask(task) && task.paused !== true && task.userPaused !== true, ); const readyTodo = todoCandidates @@ -236,7 +277,17 @@ export async function pauseTaskImpl(store: TaskStore, id: string, paused: boolea } // When pausing an in-progress/in-review task, set status so the UI can show the state. // When unpausing, clear the "paused" status. - if (task.column === "in-progress" || task.column === "in-review") { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:50 (fleet phase): + One task in hand, so one resolution — the "is this card mid-flight or in review" test that decides + whether pausing shows a `paused` status. On a renamed board neither literal matched, so pausing a + running card left its status untouched and the UI kept showing it as working. + */ + const pauseLifecycle = await resolveTaskLifecycleColumns(store, id); + if ( + task.column === (pauseLifecycle?.wip ?? "in-progress") + || task.column === (pauseLifecycle?.review ?? "in-review") + ) { task.status = paused ? "paused" : undefined; } const now = new Date().toISOString(); diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index e5e7b7d550..701324c2b9 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -16,7 +16,6 @@ "packages/engine/src/agent-tools.ts": 5, "packages/engine/src/project-engine.ts": 5, "packages/engine/src/restart-recovery-coordinator.ts": 5, - "packages/core/src/task-store/branch-group-ops.ts": 4, "packages/dashboard/app/components/TaskDetailModal.tsx": 4, "packages/dashboard/src/routes/register-git-github.ts": 4, "packages/engine/src/agent-heartbeat.ts": 4,