diff --git a/.changeset/heartbeat-parked-link-renamed-lanes.md b/.changeset/heartbeat-parked-link-renamed-lanes.md new file mode 100644 index 0000000000..0e4da51b91 --- /dev/null +++ b/.changeset/heartbeat-parked-link-renamed-lanes.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Agents no longer appear to be running a parked task on boards with renamed columns. +category: fix +dev: Both `isParkedTaskColumn` call sites in `agent-heartbeat.ts` omitted the resolved `parkedColumns` argument and took the legacy `todo`/`triage` default, so the stale-link clear never fired on a renamed board. Both now pass the task's resolved `hold`/`intake` lanes. diff --git a/.changeset/merge-blocker-renamed-review-lane.md b/.changeset/merge-blocker-renamed-review-lane.md new file mode 100644 index 0000000000..6e9fee1c1b --- /dev/null +++ b/.changeset/merge-blocker-renamed-review-lane.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Tasks can be merged and completed on boards whose review column is renamed. +category: fix +dev: Two `getTaskMergeBlocker` callers omitted the optional resolved `reviewColumns`, so the identity check fell back to the literal `in-review` and refused a card sitting in its own board's review lane — `mergeTaskImpl` threw "Cannot merge …" and the completion move threw "Cannot move … to done". Both now pass `resolveReviewColumns` from the task's workflow, unioned with the legacy id. diff --git a/docs/solutions/architecture-patterns/resolved-seams-nobody-wired.md b/docs/solutions/architecture-patterns/resolved-seams-nobody-wired.md new file mode 100644 index 0000000000..4b12a8efc8 --- /dev/null +++ b/docs/solutions/architecture-patterns/resolved-seams-nobody-wired.md @@ -0,0 +1,251 @@ +--- +category: architecture-patterns +module: workflow-resolved-columns +date: 2026-07-30 +problem_type: systemic_gap +component: core +severity: high +applies_when: + - "Converting a guard by adding an optional resolved-lane parameter" + - "Reviewing a conversion that added a parameter" + - "Auditing what a renamed board still breaks after the census reaches zero" +tags: + - workflow-resolved-columns + - column-census + - unwired-parameter + - census-invisible +--- + +# A resolved seam nobody wired is indistinguishable from no seam at all + +## The shape + +The standard conversion in this program adds an optional resolved parameter with a legacy default: + +```ts +export function isParkedTaskColumn( + task: Pick, + parkedColumns: readonly string[] = LEGACY_PARKED_COLUMNS, +): boolean +``` + +The helper is now correct, its own test passes, and the census entry is gone — the literal moved into a +documented default. **But every caller that does not pass the argument still gets the legacy behaviour**, +and nothing in the codebase records that. + +This is worse than an unconverted literal in one specific way: the literal is *visible* to the census and +to grep. A converted helper with unwired callers looks finished from every angle except the call site. + +## Why the seam test cannot catch it + +The seam test supplies the parameter — that is what it is testing. It proves the helper honours a resolved +set. It says nothing about whether anyone supplies one. So the suite is green, the census is clean, and +the guard is inert in production. + +Same root as the optional-flags blind spot (`optional-flags-seam-hides-unconverted-column-guards.md`), one +level up: there the *test* omits the parameter, here the *caller* does. + +## The audit, and its result + +Method — cheap and repeatable: + +1. Find helpers with an optional resolved-lane parameter: + + ```bash + grep -rn "ReadonlySet\|ColumnRoleFlags\|LifecycleColumns" packages/*/src --include=*.ts \ + | grep -v __tests__ + ``` + then keep the signatures where that type appears on an **optional** parameter (`name?: ReadonlySet`). + +2. For each, grep every call site and check whether the argument is actually passed. + +Result across `core`, `engine`, `dashboard`, `cli` — **13 such helpers**: + +| disposition | count | notes | +| --- | ---: | --- | +| callers wired | 7 | `isStaleBlockedByBlocker`, `areAllDependenciesDone`, `enqueue`/`dequeueMergeQueueInTransaction`, the three `restart-recovery-coordinator` predicates, `isTerminalTaskStatus` | +| **unwired — fixed** | **5** | see below | +| left deliberately | 1 | `selectActionablePlanningContinuations` — **no production caller**; wiring a parameter into a function nothing calls is the unwired-parameter anti-pattern the caller audit (#2803) removed five of | +| not lifecycle | 1 | `isBuiltinWorkflowEnabled` | + +**SCOPE OF THAT TABLE, corrected.** It enumerates the HELPERS and the call sites reached while fixing +them. It does NOT claim every caller of every helper was audited — and for at least one helper that +distinction matters a lot. `getTaskMergeBlocker` alone has **13 call sites** across core and engine; +two are fixed here, two were already wired (`moves.ts:821` passes `moveLifecycle?.review`; +`moves.ts:647` deliberately passes `skipColumnIdentityCheck` because the trait was already proven), four +sit inside self-healing sweeps that are query-gated and never run on a renamed board anyway, and the +**remaining five are genuinely unwired**: + +```text + packages/core/src/default-workflow-hooks.ts:72 + packages/core/src/task-merge.ts:355, 377 + packages/core/src/in-review-stall.ts:237 + packages/engine/src/merger.ts:6645 + packages/engine/src/merger-ai.ts:1173 + packages/engine/src/executor.ts:2404 +``` + +Run step 2 of the method per helper before believing any "audit complete" claim, including this one. + +Two of those five are now fixed (`default-workflow-hooks:72`, `executor.ts:2404` — both had the resolved +lane already in scope). The remaining three are each blocked on something other than effort, and the +reason matters more than the count: + +- **`task-merge.ts:377` (`isTaskReadyForMerge`) — dead in production.** Exported and referenced only by + the index barrels and its own test. Wiring a parameter into a function nothing calls is the + unwired-parameter anti-pattern itself; it is left, like `selectActionablePlanningContinuations`. +- **`task-merge.ts:355` (`getTaskHardMergeBlocker`) — mostly query-gated.** Three of its four callers are + self-healing sweeps behind hardcoded `listTasks({ column: "in-review" })`, so they never run on a + renamed board regardless (see the self-healing doc). Only `project-engine.ts:3537` is live. +- **`in-review-stall.ts:237` (`getInReviewStallReason`) — needs a batch prefetch, not a per-task resolve.** + Its four callers are in `reads.ts`, which decorates EVERY task on every list read. A per-task + `resolveWorkflowIrForTask` there costs one IR resolution per row on a hot path. The correct shape is the + prefetched per-workflow map used by the converted self-healing sweeps — resolve once per workflow for + the batch, then index by task. That is a performance-shaped change, not a one-argument wiring, which is + why it is recorded here rather than done in passing. + +The operator-visible cost of the last one is the in-review stall badge: on a renamed board the stall +reason is computed against the legacy lane, so the badge can be wrong for every card in review. + +## The recurring shape: outer question resolved, inner one not + +The sharpest instances are not "a caller forgot an argument" but "the same function resolved the lane and +then re-asked with the literal", a few lines apart: + +- `task-artifacts-ops` resolves `completeColumn` from the workflow, then calls the blocker with the literal. +- `default-workflow-hooks:72` gates on `lifecycleColumns?.review` / `?.complete`, then calls it with the literal. +- `executor.ts:2404` compares against `(await this.resolveResumeLanes(taskId)).review`, then calls it with the literal. +- `resolvePlanningContinuationCandidate` applies the caller's resolved terminal set, then delegates without it. + +A conversion that stops at the outer question looks complete at the call site and is not. Grep for the +helper, not for the literal — the literal is one function away, where it is correctly annotated as a +fallback. + +The five that were unwired at the sites reached here, and what each cost on a renamed board: + +- **`isParkedTaskColumn` ×2** (`agent-heartbeat`) — the stale-link clear never fired, so a durable agent + kept claiming a parked card and Reports Health Check rendered it **RUNNING**. +- **`getTaskMergeBlocker` ×2** (`mergeTaskImpl`, the completion move) — a card that had passed review + **could not be merged or completed at all**: `Cannot merge …: task is in 'checking', must be in + 'in-review'`. +- **`isPlanningContinuationTaskDispatchable`** (`in-process-runtime`) — partially threaded: the enclosing + function applied the caller's resolved set to its own check, then delegated *without* it. + +Note the second entry: `task-artifacts-ops` **already resolved the completion lane four lines above** the +call that re-asked with the literal. The outer question was resolved and the inner one was not, inside one +function. + +## The arity trap: first-per-role where membership was meant + +Six occurrences in this program, and the sixth was found only because a reviewer caught the fifth. It +deserves naming separately because it survives every check the others fail: + +```ts +const parked = [lifecycle.hold, lifecycle.intake].filter(isString); // FIRST per role +… +if (parked.includes(task.column)) … // MEMBERSHIP test +``` + +`resolveLifecycleColumns` / `resolveTaskLifecycleColumns` answer **"which column is THE hold lane?"**. +A `.includes()` / `.has()` test asks **"is this column ANY hold lane?"**. On a workflow declaring two +columns with the same trait, the first answer silently covers one of them. + +**Why nothing catches it:** + +- The census sees no literal — the lanes are resolved. +- The types are identical: `string | undefined` either way. +- A default-vs-renamed differential passes, because **the default board declares one column per role and + therefore cannot express the failing shape**. It needs a *structurally* different fixture, not a + differently-named one. + +Measured across production code — collections built from two or more first-per-role reads: **12 sites**. +Two are fixed here (`agent-heartbeat` ×2 call sites, `task-agent-sync.resolveLinkSyncColumnRoles`). The +rest are recorded for per-site classification, since some are legitimately ordered tuples rather than +membership sets: + +| site | disposition | +| --- | --- | +| `engine/src/agent-heartbeat.ts` ×2 | **fixed** | +| `engine/src/task-agent-sync.ts:62-63` | **fixed** | +| `engine/src/executor.ts:12535` | **fixed** — a card in a second wip lane read as INACTIVE and its prompt file as reclaimable | +| `engine/src/executor.ts:3041` | **blocked** — reads `resolvePlannerLanes`, i.e. the sync IR reader, which returns the default workflow for every task in production. Converting the arity here changes nothing until that is fixed. | +| `engine/src/scheduler.ts:1593` | **blocked** — same, via `resolveTaskParkedColumnsSync` | +| `core/src/workflow-lifecycle-traits.ts:231` | **not a membership use** — a returned 2-tuple | +| `engine/src/planner-lane-resolution.ts:70, 84` | **ordering-sensitive** — a precedence list, not a set; converting would change which lane wins | +| `core/src/default-workflow-hooks.ts:274, 280` | **genuine, but a contract change** — `planningColumnsOf`/`liveWorkColumnsOf` take a `LifecycleColumns`, which is first-per-role *by construction*. Fixing the arity means `DefaultWorkflowMoveContext` carrying the IR (or trait sets) instead, which is a shared hook contract touching every caller. Not a local edit. | +| `engine/src/triage.ts:833` | **blocked twice over** — built from `resolvePlannerLanes` (the sync reader) *and* used to build `listTasks({ column })` queries, so it is in the query-filter class too | +| `dashboard/src/routes/register-task-workflow-routes.ts:304` | **FALSE POSITIVE of the scan** — already correct: `columnsWithFlag` for both roles, unioned, legacy only when the resolved set is empty | + +So of twelve: **four fixed, three blocked (two on the sync reader, one also query-shaped), one needs a +contract change, three are not defects, one was my scan misfiring.** + +Two things that spread is worth saying out loud: + +1. A sweep converting all twelve would have **broken** the ordering-sensitive pair, delivered **nothing** + at the sync-blocked ones, and "fixed" a site that was already right. +2. **The scan itself has false positives**, because it classifies by syntax — two role-shaped reads and a + bracket on one line. That is the same failure this program has documented about the census: an + instrument that counts syntax and is read as counting meaning. Treat the twelve as candidates, never + as a backlog. + +Re-measure with: for each file importing a lifecycle resolver, flag lines containing **two or more** +`.hold`/`.intake`/`.review`/`.wip`/`.complete`/`.archived` reads **and** an array or `new Set(`. + +The fix is always `columnsWithFlag(ir, trait)`, which returns every column carrying it. + +## Two traps when fixing these + +**1. The legacy id is a FALLBACK, not a member.** The tempting shape is wrong: + +```ts +const reviewColumns = new Set(["in-review"]); // WRONG +for (const c of resolveReviewColumns(ir)) reviewColumns.add(c); +``` + +That admits a board which *declares* `in-review` as its WIP column — a card mid-implementation passes the +merge check. The legacy id is only correct when the board tells us nothing: + +```ts +let reviewColumns: ReadonlySet = new Set(["in-review"]); +const resolved = ir ? resolveReviewColumns(ir) : []; +if (resolved.length > 0) reviewColumns = new Set(resolved); // a real answer REPLACES the default +``` + +**2. Two guards, one assertion.** When two call sites can refuse the same operation, a loose assertion +passes with either fixed. Measured: `expect(message).toContain("must be in")` passed with `mergeTaskImpl` +reverted, because the completion guard caught the card instead. Assert something that names the site — +here the message prefix (`Cannot merge` vs `Cannot move … to done`) — so the sites fail independently. + +## When the conservative verdict needs reporting — and when it does not + +`resolveWorkflowIrForTask` **substitutes** the built-in IR rather than failing, so `resolved.length > 0` +reads as *"this board answered"* when nobody did. Both self-healing sweeps in #2838 hit this: a card whose +workflow could not be resolved was measured against the built-in lane, rejected, and rejected again on +every pass **with nothing recorded**. They now use `...WithProvenance` — not to change the verdict +(measured: identical in every state, because the built-in lane already *is* the legacy id) but to make the +unrepaired card **reportable**. + +The merge-blocker sites in this PR sit on the same shape and deliberately do **not** get that treatment. +Measured the verdict first — also identical — then checked visibility, which is where they differ: + +```ts +throw new Error(`Cannot merge ${id}: ${mergeBlocker}`); +// → "Cannot merge FN-1: task is in 'checking', must be in 'in-review'" +``` + +The refusal is already surfaced to the operator, and the message **names the lane it expected**, which is +the diagnostic a report would have added. A sweep that skips silently needs the report; a call that throws +with the expected lane in the string does not. + +**The rule, so this is not applied by reflex:** provenance buys *visibility*, never a different answer. +Add it where the conservative verdict would otherwise be invisible. Adding it where the failure already +surfaces is ceremony — and, since it changes no behaviour, ceremony that reads like a fix. + +## Related + +- `docs/solutions/architecture-patterns/hardcoded-movetask-destinations-are-census-invisible.md` — the + destination half of a conversion, also invisible to the census. +- `docs/solutions/architecture-patterns/self-healing-sweeps-are-blind-on-a-renamed-board.md` — the query + half; a guard behind a hardcoded `listTasks({ column })` never runs at all. +- `docs/solutions/test-failures/optional-flags-seam-hides-unconverted-column-guards.md` — the same blind + spot one level down, in the tests rather than the callers. diff --git a/packages/core/src/__tests__/merge-blocker-renamed-review-lane.test.ts b/packages/core/src/__tests__/merge-blocker-renamed-review-lane.test.ts new file mode 100644 index 0000000000..2025bfcf48 --- /dev/null +++ b/packages/core/src/__tests__/merge-blocker-renamed-review-lane.test.ts @@ -0,0 +1,185 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-00:45 (the unwired-parameter class, cf. #2803): + +`getTaskMergeBlocker(task, { reviewColumns })` has taken a RESOLVED lane set since its own conversion. +`mergeTaskImpl` — the merge path itself — omitted it, so the identity check fell back to the literal +`in-review` and produced: + + Cannot merge FN-X: task is not in 'in-review' + +for a card sitting correctly in ITS OWN board's review lane. A hard, operator-visible merge failure on +every renamed board. A resolved seam nobody wired is indistinguishable from no seam at all. + +WHY THIS ASSERTS AN ABSENCE. The blocker check runs BEFORE any git or worktree work, so proving the fix +does not require a merge to succeed — it requires that this particular refusal is gone. The card has no +worktree here, so the call still fails; the assertion is that it no longer fails for the WRONG reason. +Asserting success instead would drag a git fixture into a test about a lane set. + +REVERT CHECK, measured: dropping the `{ reviewColumns }` argument restores the +"task is not in 'in-review'" refusal on the renamed board. +*/ +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { pgDescribe, createTaskStoreForTest, type PgTestHarness } from "../__test-utils__/pg-test-harness.js"; +import type { TaskStore, WorkflowIr } from "../types.js"; + +/** A board whose review lane is not the legacy id, and which declares a merge-class node for it. */ +const RENAMED_IR = { + version: "v2", + id: "merge-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" }] }, + ], + /* + A real lifecycle spine, not a stub. Column ADJACENCY is derived from the graph + (`resolveAllowedColumns`), so an IR with no node in a column cannot be moved into it — my first + version's two-node graph made every setup move illegal and the failure looked like the subject. + */ + 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; + +pgDescribe("mergeTask resolves the review lane from the task's own workflow", () => { + let harness: PgTestHarness; + let store: TaskStore; + + beforeEach(async () => { + harness = await createTaskStoreForTest({ prefix: "fusion_merge_blocker_lane" }); + store = harness.store; + }); + + afterEach(async () => { + await harness?.teardown(); + }); + + it("does not refuse a card sitting in a RENAMED review lane", async () => { + /* + The REAL API. My first version called `saveWorkflowDefinition?.()` and `setTaskWorkflowSelection?.()` + — neither exists on TaskStore, and the optional-call `?.` swallowed both silently, so the task kept + resolving the BUILTIN workflow and every assertion was about the wrong board. That is what made the + first version pass with the fix reverted. + */ + const created = await store.createWorkflowDefinition({ name: "renamed merge lanes", ir: RENAMED_IR as never }); + const task = await store.createTask({ description: "renamed review lane" }); + await store.selectTaskWorkflow(task.id, created.id); + /* + moveTask, NOT updateTask. My first version used `updateTask({ column })`, which does not move a + card — so it sat in `todo` and the whole case was vacuous: the refusal it asserted about never + concerned the renamed lane at all. The premise is asserted below rather than assumed, which is the + rule `live-move-path-undeclared-target.test.ts` exists to enforce. + */ + /* The card is created in the builtin intake, so it enters the custom board through `backlog`. */ + for (const lane of ["backlog", "building", "checking"]) { + await store.moveTask(task.id, lane as never, { moveSource: "user" } as never); + } + expect((await store.getTask(task.id)).column).toBe("checking"); + + let message = ""; + try { + await store.mergeTask(task.id); + } catch (err) { + message = err instanceof Error ? err.message : String(err); + } + + // It still fails — there is no worktree — but NOT because of the lane. + expect(message).not.toContain("must be in 'in-review'"); + }); + + it("still refuses a card that is in no review lane at all on the RENAMED board", async () => { + /* + Non-vacuous companion: without it, a merge path that had simply stopped checking lane identity would + satisfy the case above. Same board, same workflow — only the card's column changes. + */ + const created = await store.createWorkflowDefinition({ name: "renamed merge lanes", ir: RENAMED_IR as never }); + const task = await store.createTask({ description: "not in review" }); + await store.selectTaskWorkflow(task.id, created.id); + for (const lane of ["backlog", "building"]) { + await store.moveTask(task.id, lane as never, { moveSource: "user" } as never); + } + expect((await store.getTask(task.id)).column).toBe("building"); + + let message = ""; + try { + await store.mergeTask(task.id); + } catch (err) { + message = err instanceof Error ? err.message : String(err); + } + + expect(message).toContain("must be in"); + }); + + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:10 (#2820 review — coderabbit, Major): + THE REPURPOSED-COLUMN DIRECTION. My first version pre-seeded `in-review` into the resolved set, which + admits a board that declares `in-review` as its WIP lane — a card mid-implementation would pass the + merge-identity check and merge prematurely. + + This is the converse direction the optional-flags doc names: not "the lane was renamed" but "the lane + still CARRIES a lifecycle name while its traits say otherwise", which is what a project gets by + repurposing a default column. A rename-only test cannot see it. + + REVERT CHECK, measured: pre-seeding the legacy id back into the set makes this fail — the card merges + from a WIP lane. + */ + it("still refuses a card in a column NAMED in-review that its workflow declares as WIP", async () => { + const repurposed = { + ...RENAMED_IR, + columns: [ + { id: "backlog", name: "Planning", traits: [{ trait: "intake" }, { trait: "hold", config: { release: "capacity" } }] }, + /* The trap: legacy NAME, wip TRAITS. */ + { id: "in-review", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "signoff", 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: "in-review", config: { seam: "execute" } }, + { id: "merge-gate", kind: "merge-gate", column: "signoff", 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; + + const created = await store.createWorkflowDefinition({ name: "repurposed in-review", ir: repurposed as never }); + const task = await store.createTask({ description: "mid-implementation" }); + await store.selectTaskWorkflow(task.id, created.id); + for (const lane of ["backlog", "in-review"]) { + await store.moveTask(task.id, lane as never, { moveSource: "user" } as never); + } + expect((await store.getTask(task.id)).column).toBe("in-review"); + + let message = ""; + try { + await store.mergeTask(task.id); + } catch (err) { + message = err instanceof Error ? err.message : String(err); + } + + // The board's review lane is `signoff`; a card in the WIP lane must not merge. + /* + NAMES THE SITE, deliberately. A looser `toContain("must be in")` passes when EITHER guard refuses — + and it did: with merge-queue-ops reverted, the completion guard in task-artifacts-ops caught the card + instead and the assertion still held. `Cannot merge` is merge-queue-ops' wording; `Cannot move … to + done` is the other. Asserting the prefix is what makes the two sites independently provable. + */ + expect(message).toContain("Cannot merge"); + expect(message).toContain("must be in"); + expect(message).toContain("signoff"); + }); +}); diff --git a/packages/core/src/task-store/merge-queue-ops.ts b/packages/core/src/task-store/merge-queue-ops.ts index 48bea43411..b65542ad1c 100644 --- a/packages/core/src/task-store/merge-queue-ops.ts +++ b/packages/core/src/task-store/merge-queue-ops.ts @@ -12,6 +12,8 @@ import type {Task, MergeResult, MergeQueueEntry, MergeQueueAcquireOptions} from import {assertNotWorkspaceTaskMerge} from "../types.js"; import "../builtin-traits.js"; import {getTaskMergeBlocker, resolveTaskMergeTarget} from "../task-merge.js"; +import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; +import {resolveReviewColumns} from "../workflow-lifecycle-traits.js"; import {__setTaskActivityLogLimitsForTesting} from "../task-store/comments.js"; import {assertSafeGitBranchName, assertSafeAbsolutePath} from "../task-store/shell-safety.js"; import {acquireMergeQueueLease as acquireMergeQueueLeaseAsync} from "../task-store/async-merge-coordination.js"; @@ -388,7 +390,36 @@ export async function mergeTaskImpl(store: TaskStore, id: string): Promise: task is not in 'in-review'` for a card sitting correctly in ITS OWN board's + review lane. A hard, operator-visible merge failure on every renamed board. + + A resolved seam nobody wired is indistinguishable from no seam at all. + + MEMBERSHIP, not first-per-role: `resolveReviewColumns` unions mergeOrchestration, mergeBlocker and + humanReview, so a workflow splitting those across columns has all of them accepted. Unioned with + the legacy id because `resolveWorkflowIrForTask` degrades to the BUILT-IN IR rather than throwing. + */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:10 (#2820 review — coderabbit, Major): + THE LEGACY ID IS A FALLBACK, NOT A MEMBER. My first version pre-seeded `in-review` into the set and + unioned the resolved lanes on top. That admits a board which declares `in-review` as its WIP column: + a card mid-implementation would pass the merge-identity check and merge prematurely. + + The legacy id is only correct when the board tells us NOTHING — an empty resolved set, or a + resolution that threw. A non-empty resolved answer replaces it outright; that is the same + "unscoped legacy acceptance" the glasses plugin's own review caught, and I reintroduced it here. + */ + let reviewColumns: ReadonlySet = new Set(["in-review"]); + try { + const ir = await resolveWorkflowIrForTask(store, id); + const resolved = ir ? resolveReviewColumns(ir) : []; + if (resolved.length > 0) reviewColumns = new Set(resolved); + } catch { /* degraded: the board told us nothing, so the legacy id stands */ } + const mergeBlocker = getTaskMergeBlocker(task, { reviewColumns }); if (mergeBlocker) { throw new Error(`Cannot merge ${id}: ${mergeBlocker}`); } diff --git a/packages/core/src/task-store/task-artifacts-ops.ts b/packages/core/src/task-store/task-artifacts-ops.ts index 067750e445..42c689e547 100644 --- a/packages/core/src/task-store/task-artifacts-ops.ts +++ b/packages/core/src/task-store/task-artifacts-ops.ts @@ -496,7 +496,38 @@ export async function moveToDoneImpl(store: TaskStore, task: Task, dir: string): } const fromColumn = task.column; - const mergeBlocker = getTaskMergeBlocker(task); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-01:10 (unwired-parameter class, cf. #2803): + THE OUTER QUESTION WAS RESOLVED AND THE INNER ONE WAS NOT — in the same function, four lines apart. + `completeColumn` above comes from the task's workflow, then this call re-asked with the literal and + refused: on a renamed board the completion move threw + + Cannot move FN-1 to done: task is in 'checking', must be in 'in-review' + + for a card sitting correctly in its own review lane. `getTaskMergeBlocker`'s own note calls out this + exact half-conversion shape in `moves.ts`; this is the same shape in a second site it did not cover. + + MEMBERSHIP via `resolveReviewColumns` (mergeOrchestration ∪ mergeBlocker ∪ humanReview), so a board + splitting those across columns has all of them accepted, unioned with the legacy id because + `resolveWorkflowIrForTask` degrades to the BUILT-IN IR rather than throwing. + */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:10 (#2820 review — coderabbit, Major): + THE LEGACY ID IS A FALLBACK, NOT A MEMBER. My first version pre-seeded `in-review` into the set and + unioned the resolved lanes on top. That admits a board which declares `in-review` as its WIP column: + a card mid-implementation would pass the merge-identity check and merge prematurely. + + The legacy id is only correct when the board tells us NOTHING — an empty resolved set, or a + resolution that threw. A non-empty resolved answer replaces it outright; that is the same + "unscoped legacy acceptance" the glasses plugin's own review caught, and I reintroduced it here. + */ + let reviewColumns: ReadonlySet = new Set(["in-review"]); + try { + const ir = await resolveWorkflowIrForTask(store, task.id); + const resolved = ir ? resolveReviewColumns(ir) : []; + if (resolved.length > 0) reviewColumns = new Set(resolved); + } catch { /* degraded: the board told us nothing, so the legacy id stands */ } + const mergeBlocker = getTaskMergeBlocker(task, { reviewColumns }); if (mergeBlocker) { throw new Error(`Cannot move ${task.id} to done: ${mergeBlocker}`); } diff --git a/packages/engine/src/__tests__/agent-heartbeat-parked-link-renamed-lanes.test.ts b/packages/engine/src/__tests__/agent-heartbeat-parked-link-renamed-lanes.test.ts new file mode 100644 index 0000000000..536e2b42ab --- /dev/null +++ b/packages/engine/src/__tests__/agent-heartbeat-parked-link-renamed-lanes.test.ts @@ -0,0 +1,236 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-23:50 (the unwired-parameter class, cf. #2803): + +`isParkedTaskColumn(task, parkedColumns?)` has taken a RESOLVED lane set since its own conversion, and +`task-agent-sync-renamed-columns.test.ts` proves the seam works when the set is supplied. But BOTH call +sites in `agent-heartbeat.ts` passed nothing and silently took the legacy `todo`/`triage` default, so on +a board whose hold and intake lanes are renamed the check returned false for every card. + +A resolved seam nobody wired is indistinguishable from no seam at all — which is why the seam test alone +could not catch this, and why the caller audit (#2803) found five more of the same shape. + +CONSEQUENCE. `reconcileOrphanedRunningAgents` clears a durable agent's task link when the card is parked +with no live execution proof. With the check inert, the link is kept: the agent goes on claiming a card +nobody is working, and Reports Health Check renders it as RUNNING. + +Reached through the private method for the same reason as `executor-worktree-owner-renamed-lanes.test.ts` +— the public route is the heartbeat poll loop, and standing that up would make this a test about polling +rather than about the lane set. + +REVERT CHECK, measured: dropping the resolved `parkedColumns` argument (back to `isParkedTaskColumn( +linkedTask)`) fails the RENAMED case — the stale link is not cleared. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Task, TaskStore, WorkflowIr } from "@fusion/core"; +import { HeartbeatMonitor } from "../agent-heartbeat.js"; +import { DEFAULT_VOCAB, RENAMED_VOCAB, lifecycleIr, type Vocabulary } from "./_workflow-vocabulary-fixture.js"; + +function parkedCard(vocab: Vocabulary): Task { + return { + id: "FN-PARKED", + title: "parked, nobody working it", + description: "", + /* The HOLD lane — parked by definition, and renamed on the custom board. */ + column: vocab.hold, + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-07-30T00:00:00.000Z", + updatedAt: "2026-07-30T00:00:00.000Z", + } as Task; +} + +function harness(vocab: Vocabulary) { + const ir: WorkflowIr = lifecycleIr(vocab, "heartbeat-parked"); + const agent = { id: "a1", name: "A", role: "executor", state: "running", taskId: "FN-PARKED" }; + const store = { + listAgents: vi.fn().mockResolvedValue([agent]), + getAgent: vi.fn().mockResolvedValue(agent), + getCachedAgent: vi.fn().mockReturnValue(null), + getActiveHeartbeatRun: vi.fn().mockResolvedValue(null), + updateAgent: vi.fn(), + updateAgentState: vi.fn(), + assignTask: vi.fn(), + recordHeartbeat: vi.fn(), + getAgentsByReportsTo: vi.fn().mockResolvedValue([]), + /* + Load-bearing: the clear path calls this, and `reconcileOrphanedRunningAgents` CATCHES its own + errors and only warns. Omit it and the sweep silently does nothing — the first version of this + test "passed" its negative case that way, which is the incomplete-fake defect this program has + documented. + */ + syncExecutionTaskLink: vi.fn(), + endHeartbeatRun: vi.fn(), + }; + const taskStore = { + getSettings: vi.fn().mockResolvedValue({}), + getTask: vi.fn().mockResolvedValue(parkedCard(vocab)), + listTasks: vi.fn().mockResolvedValue([]), + updateTask: vi.fn(), + moveTask: vi.fn(), + logEntry: vi.fn(), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "heartbeat-parked", stepIds: [] })), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "heartbeat-parked", stepIds: [] })), + getWorkflowDefinition: vi.fn(async (id: string) => (id === "heartbeat-parked" ? { ir } : undefined)), + } as unknown as TaskStore; + + const monitor = new HeartbeatMonitor({ store: store as never, taskStore, rootDir: "/repo" }); + return { monitor, store, taskStore }; +} + +/** The private sweep under test; see the header for why this is reached directly. */ +function reconcile(monitor: HeartbeatMonitor): Promise { + return (monitor as unknown as { reconcileOrphanedRunningAgents: () => Promise }) + .reconcileOrphanedRunningAgents(); +} + +describe("the parked-link sweep resolves its lanes by ROLE, not by the legacy default", () => { + for (const [label, vocab] of [["DEFAULT", DEFAULT_VOCAB], ["RENAMED", RENAMED_VOCAB]] as const) { + it(`clears a stale link to a card parked in a ${label} hold lane (${vocab.hold})`, async () => { + const { monitor, store } = harness(vocab); + + await reconcile(monitor); + + // The link is dropped: the agent stops claiming a card nobody is working. + expect(store.syncExecutionTaskLink).toHaveBeenCalledWith("a1", undefined); + }); + } + + it("leaves the link alone when the card is NOT in a parked lane on a RENAMED board", async () => { + /* + Non-vacuous companion: without it, a sweep that cleared every link would satisfy both cases above. + Same renamed board, same agent — only the card's lane changes, to the one lane where work is live. + */ + const { monitor, store, taskStore } = harness(RENAMED_VOCAB); + (taskStore.getTask as ReturnType).mockResolvedValue({ + ...parkedCard(RENAMED_VOCAB), + column: RENAMED_VOCAB.wip, + }); + + await reconcile(monitor); + + expect(store.syncExecutionTaskLink).not.toHaveBeenCalled(); + }); +}); + +/* +FNXC:WorkflowResolvedColumns 2026-07-30-00:20 (#2820 review — greptile P2): +THE SECOND CALL SITE. The commit converted BOTH `isParkedTaskColumn` callers but the tests drove only +`reconcileOrphanedRunningAgents`. `buildReportsHealthSection` resolves its lanes independently and +rewrites the rendered report, so it is a separate surface and needed its own case — converting two +copies and testing one is the Surface Enumeration failure this program keeps paying for. + +What it does on the parked path: renders the state as `active` rather than `running` and annotates the +task as "queued/no live run", which is the operator-visible half of the same defect. With the lanes +unresolved on a renamed board the report kept saying RUNNING. + +REVERT CHECK, measured: dropping the resolved argument here fails the RENAMED case — the section still +reports `running`. +*/ +function buildHealth(monitor: HeartbeatMonitor, agentStore: unknown): Promise { + return (monitor as unknown as { + buildReportsHealthSection: (agentId: string, agentStore: unknown) => Promise; + }).buildReportsHealthSection("boss", agentStore); +} + +describe("the reports health section resolves its parked lanes by ROLE", () => { + for (const [label, vocab] of [["DEFAULT", DEFAULT_VOCAB], ["RENAMED", RENAMED_VOCAB]] as const) { + it(`renders a parked report as queued on a ${label} hold lane (${vocab.hold})`, async () => { + const { monitor, store } = harness(vocab); + /* The direct report is the running agent linked to the parked card. */ + store.getAgentsByReportsTo.mockResolvedValue([ + { id: "a1", name: "A", role: "executor", state: "running", taskId: "FN-PARKED", lastHeartbeatAt: new Date().toISOString() }, + ]); + + const section = await buildHealth(monitor, store); + + expect(section).toContain("queued/no live run"); + }); + } + + it("still reports a live report as running on a RENAMED board", async () => { + /* + Non-vacuous companion: without it, a section that annotated every report would satisfy the cases + above. Same renamed board, same agent — only the card's lane changes to the wip one. + */ + const { monitor, store, taskStore } = harness(RENAMED_VOCAB); + (taskStore.getTask as ReturnType).mockResolvedValue({ + ...parkedCard(RENAMED_VOCAB), + column: RENAMED_VOCAB.wip, + }); + store.getAgentsByReportsTo.mockResolvedValue([ + { id: "a1", name: "A", role: "executor", state: "running", taskId: "FN-PARKED", lastHeartbeatAt: new Date().toISOString() }, + ]); + + const section = await buildHealth(monitor, store); + + expect(section).not.toContain("queued/no live run"); + }); +}); + +/* +FNXC:WorkflowResolvedColumns 2026-07-30-15:10 (#2820 review — greptile P1): +THE ARITY TRAP, fifth occurrence in this program. My first version read the parked lanes through +`resolveTaskLifecycleColumns`, which returns the FIRST column carrying each trait. A workflow declaring +TWO hold lanes had only one recognised, so a card parked in the SECOND one still read as live and its +stale link was never cleared — the very defect the fix exists to close, one degree narrower. + +`resolveLifecycleColumns` answers "which column is THE hold lane?"; this code needs "is this card in ANY +parked lane?". Those are different questions and nothing in the types distinguishes them, which is why +this keeps recurring. + +The default board cannot express this shape — it has one hold lane — so only a multi-lane fixture can +catch it. + +REVERT CHECK, measured: reading the lanes through `resolveTaskLifecycleColumns` again fails this case, +because `secondary-hold` is not the first hold column. +*/ +const TWO_HOLD_IR = { + version: "v2", + id: "heartbeat-parked", + name: "two holds", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold", config: { release: "capacity" } }] }, + { id: "secondary-hold", name: "Blocked", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", name: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }], + edges: [], +} as unknown as WorkflowIr; + +describe("every parked lane counts, not just the first one the resolver returns", () => { + it("clears a stale link to a card parked in the SECOND hold lane", async () => { + const tasksById = new Map([["FN-PARKED", { ...parkedCard(DEFAULT_VOCAB), column: "secondary-hold" } as Task]]); + const agent = { id: "a1", name: "A", role: "executor", state: "running", taskId: "FN-PARKED" }; + const store = { + listAgents: vi.fn().mockResolvedValue([agent]), + getAgent: vi.fn().mockResolvedValue(agent), + getCachedAgent: vi.fn().mockReturnValue(null), + getActiveHeartbeatRun: vi.fn().mockResolvedValue(null), + updateAgent: vi.fn(), + updateAgentState: vi.fn(), + assignTask: vi.fn(), + recordHeartbeat: vi.fn(), + getAgentsByReportsTo: vi.fn().mockResolvedValue([]), + syncExecutionTaskLink: vi.fn(), + endHeartbeatRun: vi.fn(), + }; + const taskStore = { + getSettings: vi.fn().mockResolvedValue({}), + getTask: vi.fn(async (id: string) => tasksById.get(id)), + listTasks: vi.fn().mockResolvedValue([]), + updateTask: vi.fn(), + moveTask: vi.fn(), + logEntry: vi.fn(), + getTaskWorkflowSelectionAsync: vi.fn(async () => ({ workflowId: "heartbeat-parked", stepIds: [] })), + getTaskWorkflowSelection: vi.fn(() => ({ workflowId: "heartbeat-parked", stepIds: [] })), + getWorkflowDefinition: vi.fn(async (id: string) => (id === "heartbeat-parked" ? { ir: TWO_HOLD_IR } : undefined)), + } as unknown as TaskStore; + + const monitor = new HeartbeatMonitor({ store: store as never, taskStore, rootDir: "/repo" }); + await reconcile(monitor); + + expect(store.syncExecutionTaskLink).toHaveBeenCalledWith("a1", undefined); + }); +}); diff --git a/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts b/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts index d6e8bb41af..6ae602fe79 100644 --- a/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts +++ b/packages/engine/src/__tests__/task-agent-sync-renamed-columns.test.ts @@ -193,3 +193,61 @@ describe("task-agent-sync under a renamed column vocabulary", () => { }); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-30-15:40 (the arity trap, sixth site in this program): +`resolveLinkSyncColumnRoles` built its `parked` and `terminal` sets from `resolveTaskLifecycleColumns`, +which returns the FIRST column carrying each trait — then handed them to `.includes()` membership tests. +A workflow declaring TWO hold lanes had link hygiene applied to only one of them: a card moved into the +second stayed linked to its agent, which is the state this whole module exists to clean up. + +The DEFAULT board cannot express this — it declares one column per role — so no rename differential +catches it. It needs a structurally different board, not a differently-named one. + +REVERT CHECK, measured: rebuilding the sets from `resolveTaskLifecycleColumns` fails this case, because +`overflow-hold` is not the first hold column. +*/ +describe("every lane carrying a role counts, not just the first", () => { + function twoHoldIr(): WorkflowIr { + return { + version: "v2", + id: WF, + nodes: [], + edges: [], + columns: [ + { id: "inbox", label: "Intake", traits: [{ trait: "intake" }] }, + { id: "backlog", label: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "overflow-hold", label: "Second Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] }, + { id: "building", label: "Wip", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", label: "Complete", traits: [{ trait: "complete" }] }, + ], + } as unknown as WorkflowIr; + } + + it("treats a SECOND hold lane as parked when clearing the agent link", async () => { + const selection = { workflowId: WF, stepIds: [] }; + const syncCalls: Array = []; + let handler: ((e: { task: { id: string }; from: string; to: string }) => Promise) | undefined; + + const store = { + on: vi.fn((_evt: string, h: typeof handler) => { handler = h; }), + off: vi.fn(), + getTaskWorkflowSelection: vi.fn(() => selection), + getTaskWorkflowSelectionAsync: vi.fn(async () => selection), + getWorkflowDefinition: vi.fn(async () => ({ ir: twoHoldIr() })), + getTask: vi.fn(async () => ({ id: "FN-1", column: "overflow-hold" }) as Task), + } as unknown as TaskStore; + + const agentStore = { + listAgents: vi.fn(async () => [{ id: "A1", taskId: "FN-1", state: "running" } as Agent]), + getActiveHeartbeatRun: vi.fn(async () => null), + updateAgentState: vi.fn(async () => {}), + syncExecutionTaskLink: vi.fn(async (_id: string, taskId: string | undefined) => { syncCalls.push(taskId); }), + } as unknown as AgentStore; + + attachAgentLinkSync({ store, agentStore, logger: { log: () => {}, warn: () => {} } }); + await handler?.({ task: { id: "FN-1" }, from: "building", to: "overflow-hold" }); + + expect(syncCalls).toContain(undefined); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-continuation-selection.test.ts b/packages/engine/src/__tests__/workflow-continuation-selection.test.ts index 7f4ada85e1..443a15dbcc 100644 --- a/packages/engine/src/__tests__/workflow-continuation-selection.test.ts +++ b/packages/engine/src/__tests__/workflow-continuation-selection.test.ts @@ -117,3 +117,63 @@ describe("selectActionablePlanningContinuations", () => { ]); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-30-01:40 (closing the partially-threaded half of the gap that +workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts documented): + +`resolvePlanningContinuationCandidate` applied the caller's resolved terminal set to its OWN check and +then delegated to `isPlanningContinuationTaskDispatchable(task)` WITHOUT it, so the inner predicate +re-tested against the legacy `done`/`archived` pair. + +THE REACHABLE CASE is a board that declares `done` as a NON-terminal column id — legal, and the shape a +project gets by repurposing a default column rather than renaming one. The outer check passes (the +resolved set says not terminal), the inner one calls it terminal per the legacy pair, and the card is +skipped as "paused": stalled by a lane name. + +REVERT CHECK, measured: dropping the threaded set makes this fail — the candidate resolves `skip` +instead of `actionable`. +*/ +describe("the inner dispatchable predicate uses the caller's resolved terminal set", () => { + it("does not treat a NON-terminal `done` column as terminal", () => { + const item = { taskId: "FN-1", waitReason: "planning" } as never; + const task = { + id: "FN-1", + column: "done", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-07-30T00:00:00.000Z", + updatedAt: "2026-07-30T00:00:00.000Z", + } as never; + + /* This board declares `done` as an ordinary lane; its terminal lane is `shipped`. */ + const resolved = resolvePlanningContinuationCandidate(item, task, { + terminalColumns: new Set(["shipped", "boxed"]), + }); + + expect(resolved.kind).toBe("actionable"); + }); + + it("still treats the board's OWN terminal lane as terminal", () => { + /* Non-vacuous companion: without it, a predicate that never classified anything terminal passes. */ + const item = { taskId: "FN-2", waitReason: "planning" } as never; + const task = { + id: "FN-2", + column: "shipped", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-07-30T00:00:00.000Z", + updatedAt: "2026-07-30T00:00:00.000Z", + } as never; + + const resolved = resolvePlanningContinuationCandidate(item, task, { + terminalColumns: new Set(["shipped", "boxed"]), + }); + + expect(resolved.kind).toBe("orphan"); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts b/packages/engine/src/__tests__/workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts index 22cd123151..677edabcef 100644 --- a/packages/engine/src/__tests__/workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts +++ b/packages/engine/src/__tests__/workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts @@ -1,5 +1,5 @@ /* -FNXC:WorkflowScheduling 2026-07-31-07:10 (E2E evidence — the optional-role-parameter class, third instance): +FNXC:WorkflowScheduling 2026-07-30-07:10 (E2E evidence — the optional-role-parameter class, third instance): Third measured instance of the pattern from #2795 and #2798, and the sharpest form of it: the converted and unconverted call sites are in the SAME FILE, and one is nested inside the other's call @@ -35,6 +35,26 @@ runtime module's SYNTAX (parsed, not string-matched — see the note on that cas drain needs the runtime's full dependency set, which I did not build; the audit case says so rather than dressing it up. +FNXC:WorkflowResolvedColumns 2026-07-30-01:40 (follow-up — one severity correction and one site closed): + +TWO REFINEMENTS to the above, both measured rather than argued. + +1. `selectActionablePlanningContinuations` has NO PRODUCTION CALLER. Grepped across the repo excluding + node_modules and dist: the only references are this file and + `workflow-continuation-selection.test.ts`. The live drain is `drainDuePlanningContinuations:386`, + which IS converted. So the stated consequence — a completed card re-entering plan-review "silently, + on every custom board" — is NOT reachable today. The finding is real but LATENT: the helper is + exported, so the first production caller inherits the bug. Worth keeping, worth not over-stating. + +2. The THIRD site is closed. `resolvePlanningContinuationCandidate` now threads its own resolved + `terminal` into `isPlanningContinuationTaskDispatchable`. Its reachable case is a board that declares + `done` as a NON-terminal column id: the outer check passes, the inner one calls it terminal per the + legacy pair, and the card is skipped as "paused" — stalled by a lane name. Pinned by two cases in + `workflow-continuation-selection.test.ts`, revert-measured. + +The first site is deliberately left: wiring a parameter into a function nothing calls would be an +unwired parameter, which is the anti-pattern the caller audit (#2803) removed five of. + LANE. `.pg.test.ts`, skipped via `pgDescribe` when no PostgreSQL is reachable, so the merge gate is unaffected. Throwaway per-file database; never port 4040. */ @@ -157,7 +177,7 @@ pgDescribe("planning-continuation terminal columns, measured on a live store", ( NOT driven: reaching the drain needs the runtime's full dependency set. Asserted against the module's SYNTAX and labelled as such. - FNXC:WorkflowScheduling 2026-07-31-10:15 (PR #2799 review — greptile P2): + FNXC:WorkflowScheduling 2026-07-30-10:15 (PR #2799 review — greptile P2): AST, not string splitting. The first version found call sites by splitting on the callee name and then reasoning about the text that followed — whitespace, the next `;`, whether the slice started with a parameter name. A formatting-only change to the runtime could fail this suite or, worse, diff --git a/packages/engine/src/agent-heartbeat.ts b/packages/engine/src/agent-heartbeat.ts index 2104c5cda2..bab81221b4 100644 --- a/packages/engine/src/agent-heartbeat.ts +++ b/packages/engine/src/agent-heartbeat.ts @@ -1232,7 +1232,33 @@ export class HeartbeatMonitor { FNXC:AgentTaskStateDrift 2026-06-23-09:02: Reports Health Check must not render a durable direct report as running a parked todo/triage task unless a fresh heartbeat run or tracked executor signal proves live execution. Clearing Agent.taskId here preserves overlapBlockedBy on the task row; the file-scope lease remains the scheduler's source of truth. */ - if (isParkedTaskColumn(linkedTask) && !parkedProof.shouldPreserveParkedLink) { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-23:50 (unwired-parameter class, cf. #2803): + `isParkedTaskColumn` has taken a resolved `parkedColumns` since its own conversion, but BOTH + call sites here passed nothing and silently took the legacy `todo`/`triage` default. On a board + whose hold and intake lanes are renamed the check returned false for every card, so this clear + never fired: a durable agent kept its task link to a parked card with no live execution proof, + and Reports Health Check went on rendering it as RUNNING. + + A resolved seam nobody wired is indistinguishable from no seam at all — which is exactly what + the caller audit found five of. + */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-15:10 (#2820 review — greptile P1): + MEMBERSHIP, not first-per-role. `resolveTaskLifecycleColumns` returns the FIRST column carrying + each trait, so a workflow declaring TWO hold lanes (or a hold plus a second intake) had only one + of them recognised as parked — a card in the secondary lane still read as live, and the stale + link was never cleared for it. Same defect this fix exists to close, one degree narrower. + + `columnsWithFlag` returns EVERY column carrying the trait, so both halves are unions. This is the + fifth time this program has hit first-per-role where it wanted membership; the two are not + interchangeable and the compiler cannot tell them apart. + */ + const parkedIr = await resolveWorkflowIrForTask(this.taskStore!, linkedTask.id).catch(() => undefined); + const parkedColumns = parkedIr + ? [...new Set([...columnsWithFlag(parkedIr, "hold"), ...columnsWithFlag(parkedIr, "intake")])] + : []; + if (isParkedTaskColumn(linkedTask, parkedColumns.length > 0 ? parkedColumns : undefined) && !parkedProof.shouldPreserveParkedLink) { reason = `parked ${linkedTask.column} task ${agent.taskId} without live execution proof`; clearTaskLink = true; taskIdToClear = agent.taskId; @@ -3100,7 +3126,7 @@ export class HeartbeatMonitor { try { const assignedOpen = await this.taskStore.getTasksByAssignedAgent(agentId, { excludeArchived: true }); /* - FNXC:WorkflowLifecycleColumns 2026-07-31-13:40: + FNXC:WorkflowLifecycleColumns 2026-07-30-13:40: Pass the resolved lane flags so the ranking's terminal filter is not the literal pair. `rankAssignedTasksForWakeDelta` gained `flagsByColumnId` and this, its only production @@ -3712,7 +3738,15 @@ export class HeartbeatMonitor { if (report.state === "running" && !isEphemeralAgent(report) && report.taskId && this.taskStore) { try { const linkedTask = await this.taskStore.getTask(report.taskId); - if (isParkedTaskColumn(linkedTask)) { + /* FNXC:WorkflowResolvedColumns 2026-07-30-23:50: same unwired parameter as above — the health + report rendered a parked card as running on any board with renamed hold/intake lanes. */ + /* FNXC:WorkflowResolvedColumns 2026-07-30-15:10 (#2820 review — greptile P1): membership, not + first-per-role — see the note on the sweep above. */ + const reportParkedIr = await resolveWorkflowIrForTask(this.taskStore, report.taskId).catch(() => undefined); + const reportParkedColumns = reportParkedIr + ? [...new Set([...columnsWithFlag(reportParkedIr, "hold"), ...columnsWithFlag(reportParkedIr, "intake")])] + : []; + if (isParkedTaskColumn(linkedTask, reportParkedColumns.length > 0 ? reportParkedColumns : undefined)) { const activeRun = await agentStore.getActiveHeartbeatRun(report.id); const proof = evaluateParkedAgentTaskLink({ agent: report, diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index 1728f5352b..5b7ccc69c2 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -2401,7 +2401,16 @@ export class TaskExecutor { return "missing"; } - const blocker = getTaskMergeBlocker(latestTask); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:40 (outer question resolved, inner one not): + The guard directly above compares against `(await this.resolveResumeLanes(taskId)).review`, then this + call re-asked with the literal — so a card that just PASSED the resolved lane check was refused by the + unresolved blocker on any renamed board. + */ + const resumeReviewLane = (await this.resolveResumeLanes(taskId)).review; + const blocker = getTaskMergeBlocker(latestTask, { + reviewColumns: new Set([resumeReviewLane ?? "in-review"]), + }); if (blocker) { await this.store.logEntry(taskId, "Task already in-review; merge deferred", blocker, this.getRunContextFor(taskId)); return "blocked"; @@ -12548,10 +12557,23 @@ export class TaskExecutor { throwing when a definition is missing or corrupt, so a degraded resolution must not NARROW this set — narrowing it re-opens the interruption this fixes. */ - const activeLifecycle = resolveLifecycleColumns(await resolveWorkflowIrForTask(this.store, task.id)); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-16:10 (the arity trap, seventh site): + MEMBERSHIP, not first-per-role. `activeColumns` is a `.has()` test, but was filled from + `resolveLifecycleColumns`, which returns the FIRST column carrying each trait — so a workflow with two + wip lanes, or a review lane plus a second merge-blocking one, had only one of each recognised as + active. A card in the second read as INACTIVE and its prompt file was treated as reclaimable. + + The IR is already in hand one line up; `columnsWithFlag` returns every column carrying the trait. + The legacy trio stays unioned in — this predicate is about liveness, and under-reporting active is + the destructive direction. + */ + const activeIr = await resolveWorkflowIrForTask(this.store, task.id); const activeColumns = new Set(["in-progress", "in-review", "done"]); - for (const lane of [activeLifecycle?.wip, activeLifecycle?.review, activeLifecycle?.complete]) { - if (lane !== undefined) activeColumns.add(lane); + if (activeIr) { + for (const flag of ["countsTowardWip", "mergeOrchestration", "mergeBlocker", "humanReview", "complete"] as const) { + for (const lane of columnsWithFlag(activeIr, flag)) activeColumns.add(lane); + } } const activeMergeStatuses = new Set(["merging", "merging-pr", "merging-fix"]); const isActiveTask = activeColumns.has(task.column) || activeMergeStatuses.has(task.status ?? ""); diff --git a/packages/engine/src/runtimes/in-process-runtime.ts b/packages/engine/src/runtimes/in-process-runtime.ts index a02fae2a4c..e3eae66796 100644 --- a/packages/engine/src/runtimes/in-process-runtime.ts +++ b/packages/engine/src/runtimes/in-process-runtime.ts @@ -196,7 +196,21 @@ export function resolvePlanningContinuationCandidate( if (task.paused === true || task.userPaused === true) { return { kind: "skip", item, reason: "paused" }; } - if (!isPlanningContinuationTaskDispatchable(task)) { + /* + FNXC:WorkflowResolvedColumns 2026-07-30-01:40 (the partially-threaded conversion named by + workflow-planning-continuation-terminal-gap-live-e2e.pg.test.ts): + THREAD THE SET THIS FUNCTION ALREADY RESOLVED. The terminal test at the top of this function uses the + caller's `terminal`; this delegation then re-tested against `LEGACY_TERMINAL_PAIR`, so the conversion + was whole at the call site and not whole inside it. + + The reachable case is narrow but real: a board that DECLARES `done` as a non-terminal column id. The + outer check passes (not terminal per the resolved set), then the inner predicate calls it terminal per + the legacy pair and the continuation is skipped as "paused" — a card stalled by a lane name. + + A partially threaded conversion is indistinguishable from a complete one at every call site that looks + converted, which is why this is worth closing even though the outer check dominates the common case. + */ + if (!isPlanningContinuationTaskDispatchable(task, terminal)) { return { kind: "skip", item, reason: "paused" }; } return { kind: "actionable", item, task }; diff --git a/packages/engine/src/task-agent-sync.ts b/packages/engine/src/task-agent-sync.ts index 111ef8c08c..2159ac78d3 100644 --- a/packages/engine/src/task-agent-sync.ts +++ b/packages/engine/src/task-agent-sync.ts @@ -1,4 +1,4 @@ -import { resolveTaskLifecycleColumns } from "@fusion/core"; +import { resolveWorkflowIrForTask, columnsWithFlag } from "@fusion/core"; import type { Agent, AgentHeartbeatRun, AgentStore, Task, TaskStore, WorkflowIr } from "@fusion/core"; export const PARKED_AGENT_LINK_FRESH_RUN_MS = 5 * 60_000; @@ -56,11 +56,22 @@ async function resolveLinkSyncColumnRoles( taskId: string, cache?: Map, ): Promise { - const lifecycle = await resolveTaskLifecycleColumns(store, taskId, cache); - if (!lifecycle) return LEGACY_COLUMN_ROLES; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-15:40 (the arity trap, sixth site): + MEMBERSHIP, not first-per-role. These two sets are consumed by `roles.parked.includes(to)` and + `roles.clear.includes(to)` — membership tests — but were built from `resolveTaskLifecycleColumns`, + which returns the FIRST column carrying each trait. A workflow declaring two hold lanes, or an + archive lane plus a second terminal one, had link hygiene applied to only one of them. - const parked = [lifecycle.hold, lifecycle.intake].filter((c): c is string => typeof c === "string"); - const terminal = [lifecycle.complete, lifecycle.archived].filter((c): c is string => typeof c === "string"); + `resolveLifecycleColumns` answers "which column is THE hold lane?"; a membership test asks "is this + column ANY hold lane". Nothing in the types distinguishes them, which is why this program has now hit + it six times. `columnsWithFlag` returns every column carrying the trait. + */ + const ir = await resolveWorkflowIrForTask(store, taskId, cache).catch(() => undefined); + if (!ir) return LEGACY_COLUMN_ROLES; + + const parked = [...new Set([...columnsWithFlag(ir, "hold"), ...columnsWithFlag(ir, "intake")])]; + const terminal = [...new Set([...columnsWithFlag(ir, "complete"), ...columnsWithFlag(ir, "archived")])]; const clear = [...terminal, ...parked]; // A v2 workflow declaring none of the four roles yields an empty clear set,