From 86a2b48968740f2e70f484b90e141ae5e8fe2eab Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 08:09:36 -0700 Subject: [PATCH] =?UTF-8?q?fleet:=20branch-group-ops=206=20=E2=86=92=200?= =?UTF-8?q?=20=E2=80=94=20an=20agent=20asking=20for=20its=20next=20task=20?= =?UTF-8?q?was=20told=20there=20was=20none=20(#2739)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second application of the sync-filter pattern decided in #2737. `branch-group-ops.ts` 6 → **0**. ## The failure `selectNextTaskForAgentImpl` picks an agent's next task by filtering the board for its WIP lane, then its hold lane — both `task.column === ""`. On a renamed board **both filters match nothing**, so an agent asking for work is told there is none, with its own assigned tasks sitting in the list it just fetched. No error, no log line. The agent idles. `pauseTaskImpl` had the same shape: pausing a running card on a renamed board left its `status` untouched, so the UI kept showing it as working. ## Consumer, not a gate — checked rather than assumed Applying the #2724 test to this file, since it sits closer to the persistence layer than the reconciler did: its **only** SQL predicate is `eq(table.projectId, ...)`. Nothing here compares a column to a literal in SQL, so there is no second encoding of these questions to diverge from. The list arrives from `store.listTasks` and the filters select among rows already in hand. Async predicates were the alternative and would have turned these filter chains into sequential awaits inside the dispatch path. One prefetch, one IR read per distinct workflow, filters stay synchronous. `pauseTaskImpl` resolves for the single task it holds rather than joining a map — different entry point, one id in scope, and a map would have exactly one entry. ## Revert proof | reverted | result | |---|---| | the wip literal | renamed WIP case fails: `expected null to be truthy` | | the hold literal | renamed hold case fails identically | The new test calls the impl **directly** with a store fake resolving a renamed IR. The existing `selectNextTaskForAgent` coverage drives a real store harness, so exercising a renamed vocabulary there means registering a real custom workflow and moving cards through it — heavier than the question, which is only which lane the filters name. The bind evaluator runs for real; only the store is faked. A third case pins that the hold filter keeps its `userPaused` exclusion, so a filter matching every column would not satisfy the other two. **Related coverage checked before writing a new file:** `agent-heartbeat-worktree-renamed-hold.test.ts` covers the requeue **target** on a renamed board, not the dispatcher's **selection** filters — different branch of the same subsystem, so a case added there would have read as duplicate coverage of the wrong thing. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **20 passed** across the routing-policy and new dispatch suites · core `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit - **Bug Fixes** - Improved agent task selection when workflow lane names or IDs have been renamed. - Agents now correctly resume assigned in-progress or queued tasks across customized workflows. - Prevented agents from selecting tasks paused by users, including on boards with renamed lanes. - Updated task pausing behavior to correctly reflect lifecycle stages beyond default lane names. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../agent-dispatch-renamed-lanes.test.ts | 185 ++++++++++++++++++ packages/core/src/agent-role-policy.ts | 26 +++ .../core/src/task-store/branch-group-ops.ts | 67 ++++++- .../lib/lifecycle-column-census-baseline.json | 1 - 4 files changed, 270 insertions(+), 9 deletions(-) create mode 100644 packages/core/src/__tests__/agent-dispatch-renamed-lanes.test.ts 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,