From e84e9d7f60a7c469367c84748378646ac870828c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 12:02:53 -0700 Subject: [PATCH] =?UTF-8?q?fix:=20the=20caller=20audit=20=E2=80=94=20five?= =?UTF-8?q?=20unwired=20parameters,=20five=20defects=20in=20their=20caller?= =?UTF-8?q?s=20(#2803)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Seven fixes that were sitting on separate handoff branches with no owner while `main` moved. Consolidated, rebased onto current `main`, and verified **together** rather than only per-branch. The individual branches remain if a subset is preferred. This is the same consolidation that got `batch-core` and #2787 adopted. **Close it if it breaks queue policy** — the branch keeps the work safe either way. ## Where these came from #2787's review found an optional parameter whose production caller never passed it. That is a class, so I ran it against everything I had landed and found five more. **All five turned out to have their real defect in the CALLER, not the parameter** — in four of them the parameter was unreachable: | unwired parameter | what was actually wrong | |---|---| | `blocker-fanout.escalationColumns` | the hold default made the count zero — **no bottleneck warning was emitted at all** | | analytics `columnFlagsByName` | routes never built a map — **0 in-progress / 0 in-review beside correct cost totals** | | `isLegacyAutoMergeStampCandidate` | the read **queried a column a renamed board does not have**, so the backfill iterated nothing | | `rankAssignedTasksForWakeDelta` | `getTasksByAssignedAgent`'s `excludeArchived` used the literal — **archived cards returned as open work** | | `duplicate-intake.columnFlagsByColumnId` | intake could **archive or soft-delete a newly created task** as a duplicate of finished work | The heuristic worth keeping: **an optional parameter no production caller fills is a marker pointing at an unexamined caller.** The census cannot see any of these five — every gate is a `Set`/array literal or a query filter, i.e. a definition rather than a comparison. ## Also included - **`executor.ts`** — the stale-spec guard did the exact thing its own comment forbids: on a renamed board it ran on a LIVE task and pulled it out of execution into replan. `activeMergeStatuses` protected merging cards *by accident*, which is why the symptom looked arbitrary. - **`register-project-routes.ts`** — project health reported **0 active tasks**; its list also still contained `triage`, dead since U11. - **`dashboard/app/utils/taskTiming.ts`** — a **second copy** of `getTotalAgentActiveMs`. Core's was converted; the card chip imports this one, so the census counted the site as done while the rendered number stayed keyed on `"in-progress"`. ## Verification Verified as a set: `pnpm test:gate` **161 / 13 / 487 / 71** · core suites **15 passed** · engine **7** · dashboard **12** · four `tsc` targets clean · lint clean · census `--strict` exits 0. Each fix is revert-proven individually; the specific case that fails is named in each test header. ## Two honesty notes **Three guards here are structural, not behavioural, and say so in their headers.** `sanitizeAgentTaskLinks` is a closure inside `createApiRoutes`; the analytics aggregators need a live `AsyncDataLayer`; the stale-spec guard sits deep inside `execute()`. Each ratchet fails on revert — verified — but none is an end-to-end proof, and the headers state which half they cover. **One of my behavioural test sets would have lied.** The intake-dedup cases drive `findSameAgentDuplicates` directly; I removed the wiring to measure the revert and **they stayed green**, because they pin the predicate and not the caller. That is the exact illusion this audit was chasing, reproduced in my own file. The forward now has its own structural check. ## Deliberately not included `worktree-pool.ts:1205` — it **fails safe** (a missed match protects a branch from cleanup rather than deleting it) and sits in the merger's branch-reaping path where the opposite error destroys work. That deserves its owner's judgement, not a drive-by conversion. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../assigned-agent-archived-lanes.test.ts | 96 +++++++++++++ .../core/src/__tests__/blocker-fanout.test.ts | 53 +++++++ .../intake-duplicate-terminal-lanes.test.ts | 107 ++++++++++++++ ...gacy-auto-merge-stamp-review-lanes.test.ts | 104 ++++++++++++++ packages/core/src/blocker-fanout.ts | 21 ++- .../src/task-store/branch-and-pr-entities.ts | 30 +++- packages/core/src/task-store/task-creation.ts | 47 +++++- .../core/src/task-store/task-mutation-ops.ts | 11 +- .../core/src/task-store/task-store-helpers.ts | 57 +++++++- .../core/src/task-store/workflow-integrity.ts | 11 +- .../dashboard/app/components/ListView.tsx | 8 +- .../dashboard/app/components/TaskCard.tsx | 24 +++- .../app/utils/__tests__/taskProgress.test.ts | 56 ++++++++ .../app/utils/__tests__/taskTiming.test.ts | 52 +++++++ packages/dashboard/app/utils/taskProgress.ts | 18 ++- packages/dashboard/app/utils/taskTiming.ts | 25 +++- ...mand-center-analytics-column-flags.test.ts | 62 ++++++++ .../routes/register-command-center-routes.ts | 84 +++++++++++ .../src/routes/register-project-routes.ts | 32 ++++- .../census-baseline-corruption-guard.test.ts | 98 +++++++++++++ .../executor-stale-spec-active-lanes.test.ts | 77 ++++++++++ .../scheduler-fanout-escalation-lanes.test.ts | 134 ++++++++++++++++++ .../unwired-lane-parameter-guard.test.ts | 113 +++++++++++++++ packages/engine/src/agent-heartbeat.ts | 21 +++ packages/engine/src/executor.ts | 27 +++- packages/engine/src/scheduler.ts | 89 +++++++++++- .../lib/lifecycle-column-census-baseline.json | 9 +- scripts/lib/unwired-lane-parameter.mjs | 126 ++++++++++++++++ scripts/lifecycle-column-census.mjs | 36 ++++- 29 files changed, 1598 insertions(+), 30 deletions(-) create mode 100644 packages/core/src/__tests__/assigned-agent-archived-lanes.test.ts create mode 100644 packages/core/src/__tests__/intake-duplicate-terminal-lanes.test.ts create mode 100644 packages/core/src/__tests__/legacy-auto-merge-stamp-review-lanes.test.ts create mode 100644 packages/dashboard/src/__tests__/command-center-analytics-column-flags.test.ts create mode 100644 packages/engine/src/__tests__/census-baseline-corruption-guard.test.ts create mode 100644 packages/engine/src/__tests__/executor-stale-spec-active-lanes.test.ts create mode 100644 packages/engine/src/__tests__/scheduler-fanout-escalation-lanes.test.ts create mode 100644 packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts create mode 100644 scripts/lib/unwired-lane-parameter.mjs diff --git a/packages/core/src/__tests__/assigned-agent-archived-lanes.test.ts b/packages/core/src/__tests__/assigned-agent-archived-lanes.test.ts new file mode 100644 index 0000000000..4639c7d4e4 --- /dev/null +++ b/packages/core/src/__tests__/assigned-agent-archived-lanes.test.ts @@ -0,0 +1,96 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-13:40: + +THE INVARIANT: `excludeArchived` excludes the cards the board's OWN workflow calls archived. + +FOUND BY AUDITING AN UNWIRED PARAMETER ONE LEVEL UP. `rankAssignedTasksForWakeDelta` gained a +resolved terminal answer that no production caller passed. Reading that caller showed the real gap +was HERE: `getTasksByAssignedAgent`'s `excludeArchived` filtered on `column === "archived"`, so on a +renamed board archived cards came back as OPEN assigned work and the Wake Delta inventory asked a +coordinator to unblock or reassign tasks that had already been archived. + +That is the fourth unwired parameter in this sweep whose CALLER held the larger defect — after +`blocker-fanout` (no warning emitted at all), the analytics routes (silent zero), and the legacy +stamp backfill (queried a column the board does not have). The parameter is the visible end; the +defect lives one level up every time. + +COST NOTE: resolution runs only over rows that already matched `agentId` — a handful — not the whole +board, and shares one IR cache. Asserted below, because a per-card IR read over `listTasks()` would +be a real regression on a large board. + +REVERT PROOF, measured: restore `task.column === "archived"` and the renamed-archived case fails. +*/ +import { describe, expect, it, vi } from "vitest"; +import { getTasksByAssignedAgentImpl } from "../task-store/branch-and-pr-entities.js"; +import type { TaskStore } from "../store.js"; + +const RENAMED_IR = { + version: "v2", id: "wf-renamed", name: "renamed", nodes: [], edges: [], + columns: [ + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + { id: "vault", name: "Vault", traits: [{ trait: "archived" }] }, + ], +}; + +function harness(tasks: Array>, ir: unknown) { + const selection = { workflowId: "wf-renamed", stepIds: [] as string[] }; + let irReads = 0; + const store = { + listTasks: vi.fn(async () => tasks), + getTaskWorkflowSelection: () => (ir ? selection : undefined), + getTaskWorkflowSelectionAsync: async () => (ir ? selection : undefined), + getWorkflowDefinition: async () => { irReads += 1; return ir ? { ir } : undefined; }, + } as unknown as TaskStore; + return { store, irReads: () => irReads }; +} + +const card = (id: string, column: string, agent = "AG-1") => ({ id, column, assignedAgentId: agent }); + +describe("getTasksByAssignedAgent excludes the board's own archived lane", () => { + it("drops a card in a RENAMED archived lane", async () => { + // Pre-fix: `vault` !== "archived", so an archived card was returned as open assigned work. + const { store } = harness([card("FN-LIVE", "building"), card("FN-VAULTED", "vault")], RENAMED_IR); + + const result = await getTasksByAssignedAgentImpl(store, "AG-1", { excludeArchived: true }); + + expect(result.map((t) => t.id)).toEqual(["FN-LIVE"]); + }); + + it("keeps a completed-but-not-archived card, since complete is not archived", async () => { + // The two roles are separable; excludeArchived must not quietly become excludeTerminal. + const { store } = harness([card("FN-SHIPPED", "shipped")], RENAMED_IR); + + const result = await getTasksByAssignedAgentImpl(store, "AG-1", { excludeArchived: true }); + + expect(result.map((t) => t.id)).toEqual(["FN-SHIPPED"]); + }); + + it("resolves nothing when excludeArchived is not requested", async () => { + // The common call must not pay for resolution it did not ask for. + const { store, irReads } = harness([card("FN-LIVE", "building")], RENAMED_IR); + + await getTasksByAssignedAgentImpl(store, "AG-1", {}); + + expect(irReads()).toBe(0); + }); + + it("resolves only the agent's own rows, sharing one IR read", async () => { + const mine = [card("FN-1", "building"), card("FN-2", "building"), card("FN-3", "building")]; + const theirs = [card("FN-X", "building", "AG-OTHER"), card("FN-Y", "building", "AG-OTHER")]; + const { store, irReads } = harness([...mine, ...theirs], RENAMED_IR); + + const result = await getTasksByAssignedAgentImpl(store, "AG-1", { excludeArchived: true }); + + expect(result).toHaveLength(3); + expect(irReads()).toBe(1); + }); + + it("keeps the legacy id when the workflow cannot be resolved", async () => { + const { store } = harness([card("FN-LIVE", "in-progress"), card("FN-OLD", "archived")], undefined); + + const result = await getTasksByAssignedAgentImpl(store, "AG-1", { excludeArchived: true }); + + expect(result.map((t) => t.id)).toEqual(["FN-LIVE"]); + }); +}); diff --git a/packages/core/src/__tests__/blocker-fanout.test.ts b/packages/core/src/__tests__/blocker-fanout.test.ts index 854ce0ece1..b1985bc796 100644 --- a/packages/core/src/__tests__/blocker-fanout.test.ts +++ b/packages/core/src/__tests__/blocker-fanout.test.ts @@ -186,3 +186,56 @@ describe("escalation resolves the board's own active lanes", () => { expect(entry?.escalation).toBeUndefined(); }); }); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-18:00: + +THE INVARIANT: escalation asks the BLOCKER's own workflow, not a board-wide union. + +Added by applying a rule I had written for myself one review earlier: *if a caveat describes a wrong +answer the code can actually produce, it is a bug with good documentation, not a documented +trade-off.* I shipped `escalationColumns` as a flat board-wide union with a note admitting it +over-approximates on a multi-workflow board — and that over-approximation is a wrong answer, just a +cheap one: a blocker gets labelled `long-lived` because ANOTHER workflow calls its column active. + +`escalationClassify` mirrors the per-task `classify` this module already documents as the only +correct option for a board spanning workflows. The flat set stays for single-vocabulary callers. + +REVERT PROOF, measured: drop the per-task branch and the cross-workflow case below labels the blocker +long-lived on the strength of a different workflow's vocabulary. +*/ +describe("escalation prefers the per-task answer over the board-wide set", () => { + it("does NOT escalate when only ANOTHER workflow calls this column active", () => { + const nowMs = Date.parse("2026-01-01T06:00:00.000Z"); + const blocker = createTask("B", "shipping" as Task["column"], { columnMovedAt: "2026-01-01T00:00:00.000Z" }); + const dependents = [1, 2, 3, 4, 5].map((n) => createTask(`D${n}`, "backlog" as Task["column"], { blockedBy: "B" })); + + const entry = computeBlockerFanoutMap([blocker, ...dependents], MAX_AUTO_MERGE_RETRIES, { + nowMs, + staleHighFanoutAgeThresholdMs: 60 * 60 * 1000, + holdColumn: "backlog", + // The union says `shipping` is active because some other workflow's wip lane is named that… + escalationColumns: new Set(["building", "shipping"]), + // …but THIS blocker's own workflow does not. + escalationClassify: () => false, + }).get("B"); + + expect(entry?.escalation).toBeUndefined(); + }); + + it("escalates when the blocker's own workflow says the lane is active", () => { + const nowMs = Date.parse("2026-01-01T06:00:00.000Z"); + const blocker = createTask("B", "building" as Task["column"], { columnMovedAt: "2026-01-01T00:00:00.000Z" }); + const dependents = [1, 2, 3, 4, 5].map((n) => createTask(`D${n}`, "backlog" as Task["column"], { blockedBy: "B" })); + + const entry = computeBlockerFanoutMap([blocker, ...dependents], MAX_AUTO_MERGE_RETRIES, { + nowMs, + staleHighFanoutAgeThresholdMs: 60 * 60 * 1000, + holdColumn: "backlog", + escalationColumns: new Set(), + escalationClassify: (task) => task.column === "building", + }).get("B"); + + expect(entry?.escalation?.blockerId).toBe("B"); + }); +}); diff --git a/packages/core/src/__tests__/intake-duplicate-terminal-lanes.test.ts b/packages/core/src/__tests__/intake-duplicate-terminal-lanes.test.ts new file mode 100644 index 0000000000..a49d1f57bc --- /dev/null +++ b/packages/core/src/__tests__/intake-duplicate-terminal-lanes.test.ts @@ -0,0 +1,107 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-14:20: + +THE INVARIANT: the intake duplicate guard never reuses a FINISHED sibling as the canonical. + +THE FIFTH AND LAST inert conversion from the #2787 audit — and the one with the worst blast radius. +`findSameAgentDuplicates` gained `columnFlagsByColumnId` and no caller passed it. On the two +agent-tools paths the cost of a bad match is a bad suggestion. On THIS path a match either +auto-archives the newly created task or, on the tombstoned branch, **soft-deletes it and removes its +directory**. So on a renamed board a new task could be archived or deleted as a duplicate of work +that had already finished. + +Four of the five audited conversions turned out to have their real defect in the CALLER rather than +in the parameter; this is the fifth, and it holds. The generalisation stands: an optional parameter +no production caller fills is a marker pointing at an unexamined caller. + +SECOND FIX IN THE SAME FUNCTION: the auto-archive branch mirrored the result into the in-memory row +with `task.column = "archived"`. On a renamed board that returned an object claiming a column its +workflow does not declare — the same shape as the `"triage"` write fixed earlier in this program. + +WHAT THE BEHAVIOURAL CASES DO AND DO NOT COVER, measured rather than assumed. They drive +`findSameAgentDuplicates` directly, so removing the WIRING in `task-creation.ts` leaves them green — +I checked, and they stayed green. They pin the predicate; they cannot pin the caller. + +The wiring therefore gets its own structural check at the bottom. That split is deliberate: driving +`resolveSameAgentDuplicateIntake` end to end means an async layer, run-audit writes and filesystem +removal, which is a large harness around a one-line forward. Saying so beats letting three green +behavioural cases imply the wiring is covered — the exact illusion this whole audit was chasing. + +REVERT PROOF, measured: restoring the literal fallback fails the renamed-complete case; removing the +`columnFlagsByColumnId:` argument fails the structural case. +*/ +import { describe, expect, it } from "vitest"; +import { findSameAgentDuplicates } from "../duplicate-intake.js"; +import type { ColumnRoleTraitFlags } from "../column-roles.js"; + +const COMPLETE_FLAGS = { complete: true } as unknown as ColumnRoleTraitFlags; +const WIP_FLAGS = { countsTowardWip: true } as unknown as ColumnRoleTraitFlags; + +const NOW = Date.parse("2026-07-31T12:00:00Z"); + +const sibling = (id: string, column: string) => ({ + id, + column, + title: "add screenshot upload", + description: "add screenshot upload to the composer", + sourceAgentId: "AG-1", + sourceParentTaskId: "FN-PARENT", + createdAt: NOW - 60_000, +}); + +const input = { + title: "add screenshot upload", + description: "add screenshot upload to the composer", + sourceParentTaskId: "FN-PARENT", +}; + +const run = (cands: ReturnType[], flags?: ReadonlyMap) => + findSameAgentDuplicates(input as never, cands as never, { + nowMs: NOW, + sourceAgentId: "AG-1", + ...(flags ? { columnFlagsByColumnId: flags } : {}), + }); + +describe("intake dedup excludes finished siblings on a renamed board", () => { + it("does not match a sibling in a RENAMED complete lane", async () => { + // Pre-fix this matched, and the intake path then archived or soft-deleted the NEW task. + const matches = run([sibling("FN-DONE", "shipped")], new Map([["shipped", COMPLETE_FLAGS]])); + + expect(matches.map((m) => m.id)).toEqual([]); + }); + + it("still matches a live sibling — the guard must keep working", async () => { + // The positive case is what makes the negative one evidence rather than an empty result. + const matches = run([sibling("FN-LIVE", "building")], new Map([["building", WIP_FLAGS]])); + + expect(matches.map((m) => m.id)).toEqual(["FN-LIVE"]); + }); + + it("keeps the legacy ids when no flags are supplied", async () => { + expect(run([sibling("FN-DONE", "done")]).map((m) => m.id)).toEqual([]); + expect(run([sibling("FN-LIVE", "in-progress")]).map((m) => m.id)).toEqual(["FN-LIVE"]); + }); +}); + +describe("the intake path actually forwards the resolved flags", () => { + it("passes columnFlagsByColumnId into findSameAgentDuplicates", () => { + /* + The behavioural cases above cannot see this — they call the predicate directly. An unforwarded + option is precisely the class this audit found five times, so the forward gets its own guard + rather than being assumed from a green predicate test. + */ + const source = readFileSync(new URL("../task-store/task-creation.ts", import.meta.url), "utf8"); + + expect(source).toContain("columnFlagsByColumnId: await resolveIntakeDuplicateColumnFlags(store, allCandidates)"); + // Scoped to distinct columns, not one IR read per candidate row. + expect(source).toContain("if (seenColumns.has(candidate.column)) continue;"); + }); + + it("mirrors the auto-archive into the row using the resolved archived lane", () => { + const source = readFileSync(new URL("../task-store/task-creation.ts", import.meta.url), "utf8"); + + expect(source).toContain('task.column = archivedLane ?? "archived";'); + }); +}); + +import { readFileSync } from "node:fs"; diff --git a/packages/core/src/__tests__/legacy-auto-merge-stamp-review-lanes.test.ts b/packages/core/src/__tests__/legacy-auto-merge-stamp-review-lanes.test.ts new file mode 100644 index 0000000000..c5105d3f17 --- /dev/null +++ b/packages/core/src/__tests__/legacy-auto-merge-stamp-review-lanes.test.ts @@ -0,0 +1,104 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-13:00: + +THE INVARIANT: the legacy auto-merge stamp backfill reads from the board's OWN review lanes. + +THE QUERY WAS THE DEFECT, not the predicate. `isLegacyAutoMergeStampCandidate` gained an optional +resolved `reviewColumns` and no caller passed it — but wiring that parameter alone would have changed +NOTHING, because the read above it asked `listTasks({ column: "in-review" })`. On a renamed board that +query returns zero rows, so the backfill iterated an empty list and reported success over nothing. +The predicate was never reached. + +That makes three unwired parameters in this sweep whose CALLER held the larger defect +(`blocker-fanout` emitted no warning at all; the analytics routes reported a silent zero; this one +queried a column that does not exist). An optional parameter nobody fills is worth reading as a +symptom of an unexamined caller, not as a cosmetic gap. + +ONE RESOLUTION, THREE USES. `resolveLegacyStampReviewColumns` is exported so the candidate query and +both re-checks share a single answer. My first draft derived the re-check set from the candidates' +own columns, which is subtly wrong: a row that moved between two VALID review lanes would have been +rejected because no candidate happened to sit in the second. Deriving the same fact twice is how a +read and its re-check disagree. + +REVERT PROOF, measured: restore `listTasks({ column: "in-review" })` and the renamed-board case +returns no candidates. +*/ +import { describe, expect, it, vi } from "vitest"; +import { listLegacyAutoMergeStampCandidatesImpl, resolveLegacyStampReviewColumns } from "../task-store/task-store-helpers.js"; +import type { TaskStore } from "../store.js"; + +const RENAMED_IR = { + version: "v2", id: "wf-renamed", name: "renamed", nodes: [], edges: [], + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "signoff", name: "Sign-off", traits: [{ trait: "merge" }] }, + { id: "waiting", name: "Waiting", traits: [{ trait: "human-review" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], +}; + +function harness(tasksByColumn: Record>>, definitions: unknown[]) { + const queried: string[] = []; + const store = { + listWorkflowDefinitions: vi.fn(async () => definitions), + listTasks: vi.fn(async ({ column }: { column: string }) => { + queried.push(column); + return tasksByColumn[column] ?? []; + }), + isLegacyAutoMergeStampCandidate: (task: { column: string; autoMerge?: boolean; autoMergeProvenance?: string }, reviewColumns?: ReadonlySet) => + (reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review") + && task.autoMerge === true && task.autoMergeProvenance !== "user", + } as unknown as TaskStore; + return { store, queried }; +} + +const stampable = (id: string, column: string) => ({ id, column, autoMerge: true }); + +describe("the legacy stamp backfill reads the board's own review lanes", () => { + it("finds candidates in a RENAMED merge lane", async () => { + // Pre-fix: the query asked for "in-review", got nothing, and the backfill reported success. + const { store } = harness({ signoff: [stampable("FN-1", "signoff")] }, [{ ir: RENAMED_IR }]); + + const candidates = await listLegacyAutoMergeStampCandidatesImpl(store); + + expect(candidates.map((c) => c.id)).toEqual(["FN-1"]); + }); + + it("also covers a human-review-only lane — the union, not one id", async () => { + const { store } = harness({ waiting: [stampable("FN-2", "waiting")] }, [{ ir: RENAMED_IR }]); + + const candidates = await listLegacyAutoMergeStampCandidatesImpl(store); + + expect(candidates.map((c) => c.id)).toEqual(["FN-2"]); + }); + + it("still queries the legacy id, for a board mid-rename", async () => { + // Rows stored under the old id must not be skipped while a rename is in flight. + const { store, queried } = harness({ "in-review": [stampable("FN-3", "in-review")] }, [{ ir: RENAMED_IR }]); + + const candidates = await listLegacyAutoMergeStampCandidatesImpl(store); + + expect(queried).toContain("in-review"); + expect(candidates.map((c) => c.id)).toEqual(["FN-3"]); + }); + + it("does not return a card outside the review lanes", async () => { + // The predicate must still filter — widening the query is not widening the answer. + const { store } = harness({ signoff: [], backlog: [stampable("FN-4", "backlog")] }, [{ ir: RENAMED_IR }]); + + const candidates = await listLegacyAutoMergeStampCandidatesImpl(store); + + expect(candidates).toEqual([]); + }); + + it("falls back to the legacy id alone when definitions cannot be read", async () => { + const store = { + listWorkflowDefinitions: vi.fn(async () => { throw new Error("unreadable"); }), + listTasks: vi.fn(async ({ column }: { column: string }) => (column === "in-review" ? [stampable("FN-5", "in-review")] : [])), + isLegacyAutoMergeStampCandidate: (t: { column: string; autoMerge?: boolean }) => t.column === "in-review" && t.autoMerge === true, + } as unknown as TaskStore; + + expect(await resolveLegacyStampReviewColumns(store)).toEqual(new Set(["in-review"])); + expect((await listLegacyAutoMergeStampCandidatesImpl(store)).map((c) => c.id)).toEqual(["FN-5"]); + }); +}); diff --git a/packages/core/src/blocker-fanout.ts b/packages/core/src/blocker-fanout.ts index 766095a0ec..3026b20eb5 100644 --- a/packages/core/src/blocker-fanout.ts +++ b/packages/core/src/blocker-fanout.ts @@ -92,6 +92,21 @@ export interface ComputeBlockerFanoutOptions { Defaults to `BLOCKER_ESCALATION_COLUMNS` so unconverted callers are byte-identical. */ escalationColumns?: ReadonlySet; + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-18:00: + PER-TASK escalation, and it takes precedence over the flat set above. + + Added by applying a rule I had just written for myself on another review: *if a caveat describes a + wrong answer the code can actually produce, it is a bug with good documentation, not a documented + trade-off.* I shipped `escalationColumns` as a board-wide union with a note saying it + over-approximates on a multi-workflow board — which is a wrong answer, just a cheap one: a blocker + can be labelled `long-lived` because ANOTHER workflow calls its column active. + + Same reasoning and same shape as `classify` above, which this module already documents as the only + correct option when a board spans workflows. The flat set stays for single-vocabulary callers and + as the legacy default. + */ + escalationClassify?: (task: Task) => boolean; } export const BLOCKER_ESCALATION_COLUMNS = new Set(["in-progress", "in-review"]); @@ -162,6 +177,10 @@ export function computeBlockerFanoutMap( const terminalColumns = options.terminalColumns ?? LEGACY_TERMINAL_COLUMNS; /* DELIBERATE-LITERAL — the unconverted-caller default, reviewed 2026-07-31-05:00. */ const escalationColumns = options.escalationColumns ?? BLOCKER_ESCALATION_COLUMNS; + /* Per-task wins over the board-wide set, which wins over the legacy default. */ + const isEscalationLane = (task: Task | undefined): boolean => + task !== undefined + && (options.escalationClassify ? options.escalationClassify(task) : escalationColumns.has(task.column)); const holdColumn = options.holdColumn ?? "todo"; const reviewColumns = options.reviewColumns ?? LEGACY_REVIEW_COLUMNS; @@ -256,7 +275,7 @@ export function computeBlockerFanoutMap( const shouldEscalate = blockerColumn !== undefined && isHighFanout && - escalationColumns.has(blockerColumn) && + isEscalationLane(blocker) && blockingAgeMs >= staleHighFanoutAgeThresholdMs; result.set(blockerId, { diff --git a/packages/core/src/task-store/branch-and-pr-entities.ts b/packages/core/src/task-store/branch-and-pr-entities.ts index 18b1c499dd..272374ffac 100644 --- a/packages/core/src/task-store/branch-and-pr-entities.ts +++ b/packages/core/src/task-store/branch-and-pr-entities.ts @@ -22,6 +22,8 @@ import { BranchGroup, BranchGroupCreateInput, ColumnId, MergeRequestRecord, Merg import { validateNodeOverrideChange } from "../node-override-guard.js"; import { WorkflowMovePolicyInput } from "../workflow-extension-types.js"; import { resolveWorkflowIrById } from "../workflow-ir-resolver.js"; +import { resolveTaskLifecycleColumns } from "../workflow-lifecycle-traits.js"; +import type { WorkflowIr } from "../workflow-ir-types.js"; import { WorkflowSettingDefinition } from "../workflow-ir-types.js"; import { and, asc, eq, inArray, isNull, ne, sql } from "drizzle-orm"; import { existsSync } from "node:fs"; @@ -493,12 +495,36 @@ export async function getTasksByAssignedAgentImpl(store: TaskStore, * In backend mode, use listTasks and filter in-memory instead of raw SQL. */ const allTasks = await store.listTasks(); - return allTasks.filter((task) => { + const assigned = allTasks.filter((task) => { if (task.assignedAgentId !== agentId) return false; if (options?.pausedOnly && !task.paused) return false; - if (options?.excludeArchived && task.column === "archived") return false; return true; }); + if (options?.excludeArchived !== true) return assigned; + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-13:40: + `excludeArchived` asks each card's OWN workflow, not the literal id. + + Found by auditing an unwired optional parameter one level up: `rankAssignedTasksForWakeDelta` + gained a resolved terminal answer that no caller passed, and reading the caller showed the real + gap was HERE — on a renamed board `column === "archived"` matched nothing, so archived cards were + returned as open assigned work and the Wake Delta inventory asked a coordinator to unblock or + reassign tasks that had already been archived. + + That is the fourth unwired parameter in this sweep whose CALLER held the larger defect. + + Resolution runs only over the rows that already matched `agentId` — a handful — not the whole + board, and shares one IR cache. A card whose workflow will not resolve keeps the literal. + */ + const archivedIrCache = new Map(); + const live: Task[] = []; + for (const task of assigned) { + const lanes = await resolveTaskLifecycleColumns(store, task.id, archivedIrCache).catch(() => undefined); + /* DELIBERATE-LITERAL — the unresolvable-workflow default, reviewed 2026-07-31-13:40. */ + const isArchived = lanes === undefined ? task.column === "archived" : task.column === lanes.archived; + if (!isArchived) live.push(task); + } + return live; } export function resolveWorkflowMoveActorImpl(store: TaskStore, diff --git a/packages/core/src/task-store/task-creation.ts b/packages/core/src/task-store/task-creation.ts index 697cda0f46..f27f2dae22 100644 --- a/packages/core/src/task-store/task-creation.ts +++ b/packages/core/src/task-store/task-creation.ts @@ -25,6 +25,8 @@ import {generateTaskLineageId} from "../task-lineage.js"; import {archiveAsSameAgentDuplicate, findSameAgentDuplicates, flagSameAgentDuplicate, type SameAgentDuplicateCandidate} from "../duplicate-intake.js"; import {buildBootstrapPrompt} from "../mesh-task-replication.js"; import {resolveWorkflowIrById} from "../workflow-ir-resolver.js"; +import {resolveTaskLifecycleColumns} from "../workflow-lifecycle-traits.js"; +import type {WorkflowIr} from "../workflow-ir-types.js"; import {DEFAULT_WORKFLOW_ID} from "../builtin-workflows.js"; import {columnsWithFlag} from "../workflow-lifecycle-traits.js"; import {validateFileScopeInPromptContent} from "../task-store/file-scope.js"; @@ -1074,6 +1076,38 @@ listTasks(includeDeleted, includeArchived), so FN-5233 sticky near-duplicate blo includes soft-deletes whose delete lifecycle puts them in `archived` on both persistence backends without a synchronous SQLite dependency. */ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-14:20: +Column trait flags for the intake duplicate guard, resolved from the candidates' OWN workflows. + +WHY THIS PATH MATTERS MORE THAN THE OTHER TWO. `findSameAgentDuplicates` gained +`columnFlagsByColumnId` so a FINISHED sibling cannot be reused as the canonical for new work, and no +caller passed it. On the agent-tools paths the cost is a bad suggestion. Here it is DESTRUCTIVE: a +match either auto-archives the newly created task or, on the tombstoned branch, soft-deletes it and +removes its directory. So on a renamed board a new task could be archived or deleted as a duplicate +of work that had already finished. + +Resolution is scoped to the columns the candidate set actually occupies — a handful of distinct ids, +not one read per card — and shares one IR cache. +*/ +async function resolveIntakeDuplicateColumnFlags( + store: TaskStore, + candidates: ReadonlyArray<{ id: string; column: string }>, +): Promise> { + const byColumn = new Map(); + const irCache = new Map(); + const seenColumns = new Set(); + for (const candidate of candidates) { + if (seenColumns.has(candidate.column)) continue; + seenColumns.add(candidate.column); + const lanes = await resolveTaskLifecycleColumns(store, candidate.id, irCache).catch(() => undefined); + if (!lanes) continue; + if (lanes.complete !== undefined) byColumn.set(lanes.complete, { ...byColumn.get(lanes.complete), complete: true }); + if (lanes.archived !== undefined) byColumn.set(lanes.archived, { ...byColumn.get(lanes.archived), archived: true }); + } + return byColumn; +} + export async function resolveSameAgentDuplicateIntake(store: TaskStore, task: Task, input: TaskCreateInput): Promise { const sourceAgentId = task.sourceAgentId ?? null; const sourceParentTaskId = task.sourceParentTaskId ?? null; @@ -1113,7 +1147,7 @@ export async function resolveSameAgentDuplicateIntake(store: TaskStore, task: Ta sourceParentTaskId: candidate.sourceParentTaskId ?? null, tombstoned: false, }]; }), - { nowMs, sourceAgentId }, + { nowMs, sourceAgentId, columnFlagsByColumnId: await resolveIntakeDuplicateColumnFlags(store, allCandidates) }, ); if (matches.length === 0) return; @@ -1140,7 +1174,16 @@ export async function resolveSameAgentDuplicateIntake(store: TaskStore, task: Ta const scores = Object.fromEntries(matches.filter((match) => !match.tombstoned).map((match) => [match.id, match.score])); if (settings.autoArchiveDuplicateTasksEnabled === true) { await archiveAsSameAgentDuplicate(store, task.id, siblingTaskIds, scores); - task.column = "archived"; + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-14:20: + Mirror the archive into the in-memory row using the board's OWN archived lane. Writing the + literal here made the returned object disagree with what the archive actually did on a renamed + board — the caller then saw a task claiming a column its workflow does not declare, the same + shape as the `"triage"` write fixed earlier in this program. + */ + const archivedLane = (await resolveTaskLifecycleColumns(store, task.id).catch(() => undefined))?.archived; + /* DELIBERATE-LITERAL — the unresolvable-workflow default, reviewed 2026-07-31-14:20. */ + task.column = archivedLane ?? "archived"; } else { const appliedPatch = await flagSameAgentDuplicate(store, task.id, siblingTaskIds, scores); if (appliedPatch) task.sourceMetadata = { ...(task.sourceMetadata ?? {}), ...appliedPatch }; diff --git a/packages/core/src/task-store/task-mutation-ops.ts b/packages/core/src/task-store/task-mutation-ops.ts index 3a255e2a1d..d0e9ce71a6 100644 --- a/packages/core/src/task-store/task-mutation-ops.ts +++ b/packages/core/src/task-store/task-mutation-ops.ts @@ -1,4 +1,5 @@ import { createLogger } from "../logger.js"; +import { resolveLegacyStampReviewColumns } from "./task-store-helpers.js"; const severityAuditLog = createLogger("core-task-mutation-ops"); /** @@ -650,6 +651,7 @@ export async function setCompletionHandoffAcceptedMarkerImpl(store: TaskStore, t export async function reconcileLegacyAutoMergeStampsImpl(store: TaskStore, options?: { apply?: boolean }): Promise { const candidates = await store.listLegacyAutoMergeStampCandidates(); + const stampReviewColumns = await resolveLegacyStampReviewColumns(store); const results: LegacyAutoMergeStampReconcileResult[] = []; if (options?.apply !== true) { @@ -658,7 +660,14 @@ export async function reconcileLegacyAutoMergeStampsImpl(store: TaskStore, optio for (const candidate of candidates) { const current = await store.getTask(candidate.id); - if (!current || !store.isLegacyAutoMergeStampCandidate(current)) { + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-13:00: + Re-check the freshly read row against the SAME resolved review vocabulary the candidate list + used. Left on the literal, this second check discarded every candidate the widened query had + just found on a renamed board — a half-converted pair where the read is resolved and the + re-check is not, which is the shape that makes a fix look applied and behave as before. + */ + if (!current || !store.isLegacyAutoMergeStampCandidate(current, stampReviewColumns)) { continue; } diff --git a/packages/core/src/task-store/task-store-helpers.ts b/packages/core/src/task-store/task-store-helpers.ts index 757099b5e4..7b1625b7f5 100644 --- a/packages/core/src/task-store/task-store-helpers.ts +++ b/packages/core/src/task-store/task-store-helpers.ts @@ -12,6 +12,8 @@ import { TaskStore } from "../store.js"; import { isBuiltinWorkflowId } from "../builtin-workflows.js"; +import { parseWorkflowIr } from "../workflow-ir.js"; +import { columnsWithFlag } from "../workflow-lifecycle-traits.js"; import { InsightStore } from "../insight-store.js"; import { ResearchStore } from "../research-store.js"; import { type TaskRow } from "./persistence.js"; @@ -225,9 +227,60 @@ export function getWorkflowWorkItemByIdentityImpl(store: TaskStore, return row ? store.rowToWorkflowWorkItem(row) : null; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-13:00: +THE QUERY WAS THE DEFECT, not the predicate below it. + +`isLegacyAutoMergeStampCandidate` gained an optional resolved `reviewColumns` and no caller passed +it. Wiring that parameter here would have changed NOTHING, because the read above it asked +`listTasks({ column: "in-review" })` — a QUERY filter with the literal. On a renamed board that query +returns zero rows, so the backfill iterated an empty list and reported success over nothing; the +predicate was never reached. + +This is the third unwired parameter in this sweep whose caller held the larger defect +(`blocker-fanout` emitted no warning at all; the analytics routes reported a silent zero). An +optional parameter nobody fills is worth reading as a SYMPTOM of an unexamined caller rather than as +a cosmetic gap. + +WHY A UNION ACROSS DEFINITIONS: there is no task to resolve from before the read, which is what makes +the query class hard. The project's declared workflows are the only lane vocabulary available at this +point, so every review-bearing column any of them declares is queried, unioned with the legacy id so +a board mid-rename (rows still stored under the old id) is not skipped. Over-inclusion costs one +extra query and is filtered by the predicate; under-inclusion silently backfills nothing, which is +the failure being fixed. +*/ +/** + * The review-lane vocabulary for the legacy auto-merge stamp backfill, resolved from the PROJECT's + * declared workflows because there is no task to resolve from before the read. + * + * Exported so the candidate query and the two re-checks that follow it share ONE answer. Deriving it + * separately per site is how a read and its re-check end up disagreeing. + */ +export async function resolveLegacyStampReviewColumns(store: TaskStore): Promise> { + /* DELIBERATE-LITERAL — unioned, not replaced: a board mid-rename still has rows stored under the + old id, and skipping them is the failure this fixes. Reviewed 2026-07-31-13:00. */ + const reviewColumns = new Set(["in-review"]); + try { + for (const definition of await store.listWorkflowDefinitions()) { + const ir = typeof definition.ir === "string" ? parseWorkflowIr(definition.ir) : definition.ir; + if (!ir) continue; + for (const id of columnsWithFlag(ir, "mergeOrchestration")) reviewColumns.add(id); + for (const id of columnsWithFlag(ir, "mergeBlocker")) reviewColumns.add(id); + for (const id of columnsWithFlag(ir, "humanReview")) reviewColumns.add(id); + } + } catch { + /* Unreadable definitions leave the legacy id alone — exactly the previous behaviour. */ + } + return reviewColumns; +} + export async function listLegacyAutoMergeStampCandidatesImpl(store: TaskStore): Promise { - const inReview = await store.listTasks({ column: "in-review" }); - return inReview.filter((task) => store.isLegacyAutoMergeStampCandidate(task)); + const reviewColumns = await resolveLegacyStampReviewColumns(store); + const byId = new Map(); + for (const column of reviewColumns) { + for (const task of await store.listTasks({ column })) byId.set(task.id, task); + } + return [...byId.values()].filter((task) => store.isLegacyAutoMergeStampCandidate(task, reviewColumns)); } export function deleteTaskByIdImpl(store: TaskStore, taskId: string): void { diff --git a/packages/core/src/task-store/workflow-integrity.ts b/packages/core/src/task-store/workflow-integrity.ts index bca024b513..32bfb7cadf 100644 --- a/packages/core/src/task-store/workflow-integrity.ts +++ b/packages/core/src/task-store/workflow-integrity.ts @@ -1,4 +1,5 @@ import { createLogger } from "../logger.js"; +import { resolveLegacyStampReviewColumns } from "./task-store-helpers.js"; const severityAuditLog = createLogger("core-workflow-integrity"); /** @@ -31,10 +32,18 @@ export async function markLegacyAutoMergeStampsOnceImpl(store: TaskStore): Promi } const candidates = await store.listLegacyAutoMergeStampCandidates(); + const stampReviewColumns = await resolveLegacyStampReviewColumns(store); const markedTaskIds: string[] = []; for (const candidate of candidates) { const current = await store.getTask(candidate.id); - if (!current || !store.isLegacyAutoMergeStampCandidate(current)) { + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-13:00: + Re-check the freshly read row against the SAME resolved review vocabulary the candidate list + used. Left on the literal, this second check discarded every candidate the widened query had + just found on a renamed board — a half-converted pair where the read is resolved and the + re-check is not, which is the shape that makes a fix look applied and behave as before. + */ + if (!current || !store.isLegacyAutoMergeStampCandidate(current, stampReviewColumns)) { continue; } current.autoMergeProvenance = "legacy-stamp"; diff --git a/packages/dashboard/app/components/ListView.tsx b/packages/dashboard/app/components/ListView.tsx index d0cbc50060..4deb3a9723 100644 --- a/packages/dashboard/app/components/ListView.tsx +++ b/packages/dashboard/app/components/ListView.tsx @@ -2970,7 +2970,9 @@ export function ListView({ const hasStatus = (hasTaskStatusBadge(visualStatus) && visualStatus !== "queued") || isTransientPlannerActive; const isReviewBudgetExhausted = isReviewBudgetExhaustedApproval(task); - const optionalGateBadge = getRunningOptionalGateBadge(task); + /* FNXC:WorkflowLifecycleColumns 2026-07-31-15:50: pass the already-resolved flags so the badge's + review-lane gate is not the literal — this list already owns `columnFlagsById`. */ + const optionalGateBadge = getRunningOptionalGateBadge(task, columnFlagsById.get(task.column)); const showOptionalGateBadge = Boolean(optionalGateBadge) && isAgentActive; /* FNXC:TaskStatusBadge 2026-07-26-14:05: @@ -3232,7 +3234,9 @@ export function ListView({ && isAgentActive; const showStatusBadge = (hasTaskStatusBadge(visualStatus) && visualStatus !== "queued") || isTransientPlannerActive; - const optionalGateBadge = getRunningOptionalGateBadge(task); + /* FNXC:WorkflowLifecycleColumns 2026-07-31-15:50: pass the already-resolved flags so the badge's + review-lane gate is not the literal — this list already owns `columnFlagsById`. */ + const optionalGateBadge = getRunningOptionalGateBadge(task, columnFlagsById.get(task.column)); const showOptionalGateBadge = Boolean(optionalGateBadge) && isAgentActive; // FNXC:TaskStatusBadge 2026-07-26-14:05: the step-name override yields to the // gate badge — see the grouped-card render path above. diff --git a/packages/dashboard/app/components/TaskCard.tsx b/packages/dashboard/app/components/TaskCard.tsx index d516476aef..1b41636595 100644 --- a/packages/dashboard/app/components/TaskCard.tsx +++ b/packages/dashboard/app/components/TaskCard.tsx @@ -373,11 +373,25 @@ function getInProgressElapsedMs(task: Task, nowMs: number): number | null { // timer reflects how long the task actually took, not just the time spent // inside instrumented code paths. Returns null on legacy tasks that completed // before `executionStartedAt` was tracked, so callers can fall back. -function getTaskEndToEndDurationMs(task: Task, nowMs: number): number | null { +function getTaskEndToEndDurationMs( + task: Task, + nowMs: number, + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-10:10: + THREADED SO THE CONVERSION IS NOT INERT. `getTotalAgentActiveMs` gained an optional `columnFlags` + so the LIVE execution segment is counted from the card's own wip lane. This is one of its two + production callers, and it passed nothing — so the resolved path existed and never ran, and the + card chip under-reported the in-flight run on a renamed board by exactly its elapsed time. + + An optional parameter no production caller supplies is a conversion that reads as done and behaves + as the literal: the census drops and nothing changes. Threading it here is what makes it real. + */ + columnFlags?: TaskContextMenuColumnFlags, +): number | null { // FNXC:TaskTiming 2026-07-20-12:00: planning-only tasks have no execution // accumulator, but their active AI duration still belongs on the card chip. // Use the legacy execution window only when neither active-time source exists. - const totalActiveMs = getTotalAgentActiveMs(task, nowMs); + const totalActiveMs = getTotalAgentActiveMs(task, nowMs, columnFlags); return totalActiveMs ?? getEndToEndDurationMs(task.executionStartedAt, task.executionCompletedAt, nowMs); } @@ -401,8 +415,8 @@ function getMergeElapsedMs(task: Task, nowMs: number): number | null { return Math.max(0, nowMs - mergeStartedMs); } -function getActiveMergeTotalMs(task: Task, nowMs: number): number | null { - const endToEndMs = getTaskEndToEndDurationMs(task, nowMs); +function getActiveMergeTotalMs(task: Task, nowMs: number, columnFlags?: TaskContextMenuColumnFlags): number | null { + const endToEndMs = getTaskEndToEndDurationMs(task, nowMs, columnFlags); if (endToEndMs != null) { return endToEndMs; } @@ -1687,7 +1701,7 @@ function TaskCardComponent({ const nowMs = Date.now(); if (isWipColumn) { - const endToEndMs = getTaskEndToEndDurationMs(task, nowMs); + const endToEndMs = getTaskEndToEndDurationMs(task, nowMs, taskColumnFlags); const elapsedMs = getInProgressElapsedMs(task, nowMs); const instrumentedMs = getInstrumentedDurationMs(task, nowMs); if (endToEndMs == null && elapsedMs == null && instrumentedMs == null) { diff --git a/packages/dashboard/app/utils/__tests__/taskProgress.test.ts b/packages/dashboard/app/utils/__tests__/taskProgress.test.ts index 6b1a28f6ab..4834a4cdf1 100644 --- a/packages/dashboard/app/utils/__tests__/taskProgress.test.ts +++ b/packages/dashboard/app/utils/__tests__/taskProgress.test.ts @@ -412,3 +412,59 @@ describe("getUnifiedTaskProgress", () => { expect(progress.completed).toBe(2); }); }); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-15:50: + +THE INVARIANT: the running-gate badge appears in the board's OWN review lane. + +CENSUS-INVISIBLE, in a HALF-CONVERTED file. `REVIEW_LANE_COLUMNS` is a `Set` literal — a definition, +not a comparison — sitting under a Plan Review badge whose own column restriction had already been +removed. One converted badge and one unconverted one, deciding sibling questions about the same card. + +Keyed on the literal, the code-review / browser-verification / post-merge badges never appeared on a +renamed board: the gate WAS running and the card showed nothing, which reads as an idle card rather +than a missing badge. Cosmetic in consequence, but it is the surface an operator watches to know a +review is in flight. + +The caller is wired in the same change — `ListView` already owns `columnFlagsById`, so the resolved +answer was one lookup away. An unwired parameter is the class this program's caller audit found five +times; adding a sixth would have been careless. + +REVERT PROOF, measured: restore the literal gate and the renamed-lane case returns undefined. +*/ +describe("the running-gate badge resolves the review lane", () => { + const REVIEW_FLAGS = { mergeBlocker: true } as never; + /* `makeTask` is file-scoped but `runningCodeReview` is not, so the shape is restated here. My + first draft hand-rolled `status: "in-progress"` and produced no RUNNING item at all — a gate + counts as running only when it is `pending` WITH a `startedAt` — so the renamed case failed for + the wrong reason. The fixture has to match the branch under test. */ + const codeReviewRunning = (column: string) => ({ + ...makeTask({ + enabledWorkflowSteps: ["code-review"], + workflowStepResults: [{ + workflowStepId: "code-review", + workflowStepName: "Code Review", + status: "pending" as const, + startedAt: "2026-07-11T12:00:00.000Z", + }], + }), + column, + }) as never; + + it("badges a card in a RENAMED review lane", () => { + /* testId is `code-review`, not the plan-review badge's `reviewing` — I asserted the latter first + and the failure was mine, not the product's. */ + expect(getRunningOptionalGateBadge(codeReviewRunning("signoff"), REVIEW_FLAGS)?.testId).toBe("code-review"); + }); + + it("still does not badge a card outside the review lane", () => { + // The gate must stay a gate — badging everywhere is its own bug. + expect(getRunningOptionalGateBadge(codeReviewRunning("building"), { countsTowardWip: true } as never)).toBeUndefined(); + }); + + it("keeps the legacy id when no flags are supplied", () => { + expect(getRunningOptionalGateBadge(codeReviewRunning("in-review"))?.testId).toBe("code-review"); + expect(getRunningOptionalGateBadge(codeReviewRunning("signoff"))).toBeUndefined(); + }); +}); diff --git a/packages/dashboard/app/utils/__tests__/taskTiming.test.ts b/packages/dashboard/app/utils/__tests__/taskTiming.test.ts index b2d678c987..83c078814e 100644 --- a/packages/dashboard/app/utils/__tests__/taskTiming.test.ts +++ b/packages/dashboard/app/utils/__tests__/taskTiming.test.ts @@ -64,3 +64,55 @@ describe("taskTiming helpers", () => { expect(wallClock).toBe(16_500_000); }); }); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-10:10: + +THE INVARIANT: the card's active-time chip counts the live run from the card's OWN wip lane. + +THE FINDING THAT MATTERS MORE THAN THE FIX: `@fusion/core` exports its own `getTotalAgentActiveMs`, +and it was already converted onto `isWipColumnRole`. The card chip imports THIS module instead — a +second implementation of the same calculation in a different package — so that conversion never +reached the surface an operator looks at. The census counted core's site as done while the rendered +number stayed wrong. Two implementations of one rule, one converted and one not, is exactly the drift +`column-roles.ts` exists to end. + +Keyed on the literal, the live execution segment was dropped on a renamed board: the chip +under-reported the run in flight by exactly its elapsed time, then healed itself the moment the card +moved on and the segment was persisted into `cumulativeActiveMs`. A number that is wrong only while +you are watching it. + +REVERT PROOF, measured: restore `task.column === "in-progress"` in `getActiveRuntimeMs` and the +renamed-lane cases below fail. +*/ +describe("active-time resolves the card's own wip lane", () => { + const WIP_FLAGS = { countsTowardWip: true } as never; + const NOW = Date.parse("2026-07-31T12:00:00Z"); + const STARTED = "2026-07-31T11:00:00Z"; + const HOUR = 60 * 60 * 1000; + + it("counts the in-flight run for a RENAMED wip lane", () => { + expect(getActiveRuntimeMs( + { column: "building", cumulativeActiveMs: 0, executionStartedAt: STARTED } as never, NOW, WIP_FLAGS, + )).toBe(HOUR); + }); + + it("includes it in the rendered total", () => { + expect(getTotalAgentActiveMs( + { column: "building", cumulativeActiveMs: 0, executionStartedAt: STARTED } as never, NOW, WIP_FLAGS, + )).toBe(HOUR); + }); + + it("does NOT count a live segment outside the wip lane", () => { + // A stale executionStartedAt on a review card is not active time. + expect(getActiveRuntimeMs( + { column: "signoff", cumulativeActiveMs: 0, executionStartedAt: STARTED } as never, NOW, { mergeBlocker: true } as never, + )).toBe(0); + }); + + it("keeps the legacy id when no flags are supplied", () => { + expect(getActiveRuntimeMs( + { column: "in-progress", cumulativeActiveMs: 0, executionStartedAt: STARTED } as never, NOW, + )).toBe(HOUR); + }); +}); diff --git a/packages/dashboard/app/utils/taskProgress.ts b/packages/dashboard/app/utils/taskProgress.ts index b81b44adfa..4e174ba345 100644 --- a/packages/dashboard/app/utils/taskProgress.ts +++ b/packages/dashboard/app/utils/taskProgress.ts @@ -1,3 +1,4 @@ +import { isReviewColumnRole, type ColumnRoleFlags } from "./columnRoles"; import type { Task, WorkflowStepResult, WorkflowStepPhase, StepStatus } from "@fusion/core"; /* @@ -207,6 +208,19 @@ Lane-owned optional gates are header badges, not progress bullet-list rows. Code FNXC:TaskCardOptionalGateBadge 2026-07-27-06:10: Plan Review's planning-lane restriction is GONE (see getRunningOptionalGateBadge) — it badges wherever it runs. The badge is keyed on the RUNNING gate rather than the card's column, so it stays correct across every workflow regardless of where that workflow places the node. */ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-15:50: +DELIBERATE-LITERAL — the unresolved-flags default, reviewed 2026-07-31-15:50. + +Census-invisible: a `Set` literal is a definition, not a comparison, so nothing in the lifecycle +backlog pointed at this gate — in a file whose Plan Review badge directly above was already converted +off its column restriction. One converted badge and one unconverted one, deciding sibling questions. + +Keyed on the literal, the code-review / browser-verification / post-merge badges never appeared on a +renamed board: the gate WAS running and the card simply showed nothing, which reads as an idle card +rather than a missing badge. Cosmetic in consequence, but it is the surface an operator watches to +know a review is in flight. +*/ const REVIEW_LANE_COLUMNS = new Set(["in-review"]); export interface RunningOptionalGateBadge { @@ -225,6 +239,8 @@ function workflowStepIdFromProgressItemId(itemId: string): string { export function getRunningOptionalGateBadge( task: Pick, + /** Resolved trait flags for the card's column; omitted keeps the legacy id. */ + columnFlags?: ColumnRoleFlags, ): RunningOptionalGateBadge | undefined { const running = getUnifiedTaskProgress(task).items.find( (item) => item.source === "workflow" && item.status === "running", @@ -259,7 +275,7 @@ export function getRunningOptionalGateBadge( }; } - if (!REVIEW_LANE_COLUMNS.has(task.column)) return undefined; + if (!(columnFlags ? isReviewColumnRole(columnFlags, task.column) : REVIEW_LANE_COLUMNS.has(task.column))) return undefined; if ( workflowStepId !== "code-review" && workflowStepId !== "browser-verification" diff --git a/packages/dashboard/app/utils/taskTiming.ts b/packages/dashboard/app/utils/taskTiming.ts index c69c28eedb..d1ea596caa 100644 --- a/packages/dashboard/app/utils/taskTiming.ts +++ b/packages/dashboard/app/utils/taskTiming.ts @@ -1,3 +1,4 @@ +import { isWipColumnRole, type ColumnRoleFlags } from "./columnRoles"; import type { Task, TaskLogEntry, WorkflowStepResult } from "@fusion/core"; export interface TimingEvent { @@ -96,11 +97,27 @@ export function getEndToEndDurationMs( export function getActiveRuntimeMs( task: Pick, nowMs: number, + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-10:10: + THE DASHBOARD HAS ITS OWN COPY OF THIS FUNCTION, and converting core's did not touch it. + + `@fusion/core`'s `task-timing.ts` exports a `getTotalAgentActiveMs` that was converted onto + `isWipColumnRole` — but the card chip imports THIS module instead, so that conversion never + reached the surface an operator actually looks at. Two implementations of one calculation, one + converted and one not, is the same drift `column-roles.ts` was created to end. + + Keyed on the literal, the LIVE execution segment was dropped on a renamed board, so the card + under-reported the run in flight by exactly its elapsed time — and healed itself the moment the + card moved on and the segment was persisted. + + Omitted flags keep the legacy id via `isWipColumnRole`'s own degraded mode. + */ + columnFlags?: ColumnRoleFlags, ): number | null { const persisted = task.cumulativeActiveMs; const base = persisted ?? 0; - if (task.column === "in-progress") { + if (isWipColumnRole(columnFlags, task.column)) { const startedMs = parseTimestampToMs(task.executionStartedAt); if (startedMs != null) { return base + Math.max(0, nowMs - startedMs); @@ -119,11 +136,13 @@ export function getActiveRuntimeMs( export function getTotalAgentActiveMs( task: Pick, nowMs: number, + /** Resolved trait flags for the card's column; omitted keeps the legacy id. */ + columnFlags?: ColumnRoleFlags, ): number | null { - const execution = getActiveRuntimeMs(task, nowMs) ?? 0; + const execution = getActiveRuntimeMs(task as never, nowMs, columnFlags) ?? 0; const planningStart = parseTimestampToMs(task.planningStartedAt); const planning = Math.max(0, task.cumulativePlanningMs ?? 0) + (planningStart != null ? Math.max(0, nowMs - planningStart) : 0); - return task.cumulativeActiveMs != null || task.cumulativePlanningMs != null || (task.column === "in-progress" && parseTimestampToMs(task.executionStartedAt) != null) || planningStart != null + return task.cumulativeActiveMs != null || task.cumulativePlanningMs != null || (isWipColumnRole(columnFlags, task.column) && parseTimestampToMs(task.executionStartedAt) != null) || planningStart != null ? execution + planning : null; } diff --git a/packages/dashboard/src/__tests__/command-center-analytics-column-flags.test.ts b/packages/dashboard/src/__tests__/command-center-analytics-column-flags.test.ts new file mode 100644 index 0000000000..0c035c54a2 --- /dev/null +++ b/packages/dashboard/src/__tests__/command-center-analytics-column-flags.test.ts @@ -0,0 +1,62 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-12:20: + +THE INVARIANT: the Command Center analytics routes supply the column trait flags the aggregators +need, so the wip/review tallies come from ROLES rather than ids. + +WIRING AN OPTION NOTHING FILLED — the second of the five inert conversions I audited after #2787's +review. `columnFlagsByName` existed on both aggregators and neither route passed it, so both surfaces +reported **0 in-progress and 0 in-review** on a renamed board beside token and cost totals that were +entirely correct. A zero next to a populated neighbour reads as "nobody is working", not as "this +metric is broken". + +STRUCTURAL, AND THE REASON IS THE SEAM: the aggregators take a live `AsyncDataLayer` and run SQL, so +driving them here means standing up PostgreSQL to re-assert that an option is forwarded. The value +being forwarded is already covered behaviourally by `analytics-timing-roles-resolved.test.ts` in core. +What was missing was the WIRING, and that is what this pins. Labelled rather than dressed up as a +behavioural test. + +REVERT PROOF, measured: drop either `columnFlagsByName:` line from the routes and the matching case +below fails. +*/ +import { readFileSync } from "node:fs"; +import { describe, expect, it } from "vitest"; + +const source = readFileSync(new URL("../routes/register-command-center-routes.ts", import.meta.url), "utf8"); + +describe("Command Center analytics routes pass resolved column flags", () => { + it("builds the flag map from the project's own workflow definitions", () => { + expect(source).toContain("async function resolveColumnFlagsByName("); + expect(source).toContain("await store.listWorkflowDefinitions()"); + expect(source).toContain("resolveColumnFlags(column)"); + }); + + it("forwards it to BOTH aggregators", () => { + // Two routes, two call sites: wiring one and not the other is the half-converted-pair shape + // this program keeps finding. + const forwarded = source.split("columnFlagsByName: await resolveColumnFlagsByName(store)").length - 1; + expect(forwarded).toBe(2); + }); + + it("DROPS a column id two workflows disagree about, rather than merging their flags", () => { + /* + #2803 review (greptile P1). Merging flags across workflows made one id carry both roles, so the + aggregators counted the same rows as in-progress AND in-review — a double count, worse than the + silent zero the wiring set out to fix. I had documented the flat-map ambiguity in the PR body and + shipped it anyway; documenting a defect is not resolving it. + + Dropping the ambiguous id leaves those rows on the documented legacy behaviour: still wrong for a + renamed board, but wrong in ONE direction and never double counted. + */ + expect(source).toContain("if (isConflictingColumnFlags(existing, flags)) conflicting.add(column.id);"); + expect(source).toContain("for (const id of conflicting) byColumn.delete(id);"); + // The comparison covers exactly the roles the tallies read. + expect(source).toContain("(a.countsTowardWip === true) !== (b.countsTowardWip === true)"); + }); + + it("degrades to an empty map rather than throwing when definitions are unreadable", () => { + // An empty map means both aggregators keep their documented legacy ids — the analytics page must + // not 500 because a workflow row is corrupt. + expect(source).toContain("Unreadable definitions leave the map empty"); + }); +}); diff --git a/packages/dashboard/src/routes/register-command-center-routes.ts b/packages/dashboard/src/routes/register-command-center-routes.ts index 61ededab5d..ca60f76039 100644 --- a/packages/dashboard/src/routes/register-command-center-routes.ts +++ b/packages/dashboard/src/routes/register-command-center-routes.ts @@ -13,6 +13,9 @@ import { composeLiveSnapshot, LITELLM_PRICING_SOURCE_URL, parseLiteLLMPricing, + resolveColumnFlags, + parseWorkflowIr, + type TaskStore, type TokenGroupBy, type TokenTimeGranularity, } from "@fusion/core"; @@ -388,6 +391,85 @@ export const registerCommandCenterRoutes: ApiRouteRegistrar = (ctx) => { * FNXC:CommandCenter 2026-06-18-16:57: * The Team endpoint must inherit Command Center auth and resolve getScopedStore(req) before aggregation so project-A callers cannot read project-B agent rows or task metrics. It intentionally omits GitHub issue stats; FN-6653 owns that overlay. */ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-12:20: +Board-wide column trait flags for the analytics tallies, keyed by column ID. + +WIRING AN OPTION NOTHING FILLED. `columnFlagsByName` was added to both aggregators so +`tasksInProgress`/`tasksInReview` come from column ROLES; neither route passed it, so both surfaces +kept reporting **0 in-progress and 0 in-review** on a renamed board beside token and cost totals that +were entirely correct. A zero next to a populated neighbour reads as "nobody is working". + +THE LIMIT, STATED RATHER THAN GLOSSED. This map is FLAT — one entry per column id, unioned across +every workflow the project declares. On a board where two workflows use the SAME id for different +roles, the merged flags make that column count for both roles. That is a real ambiguity and it cannot +be resolved here for `team` analytics: its rows are already aggregated to `{agentId, columnName}` by +SQL, so the workflow identity is gone before this code sees the data. Fixing that properly means +grouping by workflow in the query, which is a change to the aggregation, not to this wiring. + +`workflow` analytics is different — its rows DO carry `workflowId`, so a per-workflow map is possible +there and would be exact. I have not taken it in this change because it needs a second option shape +on the aggregator; it is the obvious follow-up and is called out on the PR rather than left implied. +*/ +/** Do two workflows disagree about what this column id MEANS? Only the roles the analytics tallies + * read are compared — a difference in an unrelated trait is not a conflict for this purpose. */ +function isConflictingColumnFlags( + a: ReturnType, + b: ReturnType, +): boolean { + return (a.countsTowardWip === true) !== (b.countsTowardWip === true) + || (a.mergeBlocker === true) !== (b.mergeBlocker === true) + || (a.humanReview === true) !== (b.humanReview === true); +} + +async function resolveColumnFlagsByName( + store: Pick, +): Promise>> { + const byColumn = new Map>(); + const conflicting = new Set(); + try { + const definitions = await store.listWorkflowDefinitions(); + for (const definition of definitions) { + const ir = typeof definition.ir === "string" ? parseWorkflowIr(definition.ir) : definition.ir; + const columns = ir && "columns" in ir ? ir.columns : undefined; + for (const column of columns ?? []) { + const flags = resolveColumnFlags(column); + const existing = byColumn.get(column.id); + if (existing === undefined) { + byColumn.set(column.id, flags); + continue; + } + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-17:20 (#2803 review — greptile P1): + A CONFLICTING id is DROPPED, not merged. + + My first version merged flags across workflows with `{ ...existing, ...flags }`. Where two + workflows reuse one column id for DIFFERENT roles, that produced an entry carrying both, and + the aggregators then counted the same rows as in-progress AND in-review — a double count, + which is worse than the silent zero this wiring set out to fix. I had documented the flat-map + ambiguity in the PR body and shipped it anyway; documenting a defect is not resolving it. + + Dropping the id leaves those rows on the aggregators' documented legacy behaviour: still + wrong for a renamed board, but wrong in ONE direction and never double counted. Unambiguous + ids — the overwhelming majority, and the whole board on a single-workflow project — keep the + resolved answer. + + Exact per-workflow classification is possible for `workflow` analytics (its rows carry + `workflowId`) and impossible for `team` analytics at this seam (its rows are aggregated to + `{agentId, columnName}` by SQL before this code sees them). That asymmetry needs a second + option shape on the aggregator and a change to the team query — a separate change, named here + rather than approximated. + */ + if (isConflictingColumnFlags(existing, flags)) conflicting.add(column.id); + } + } + } catch { + /* Unreadable definitions leave the map empty, so both aggregators keep their legacy ids. */ + } + for (const id of conflicting) byColumn.delete(id); + return byColumn; +} + router.get("/command-center/team", async (req, res) => { try { const store = await getScopedStore(req); @@ -400,6 +482,7 @@ export const registerCommandCenterRoutes: ApiRouteRegistrar = (ctx) => { to: range.to, now: Date.now(), pricingOverrides: settings.modelPricingOverrides, + columnFlagsByName: await resolveColumnFlagsByName(store), }); res.json(result); } catch (err: unknown) { @@ -427,6 +510,7 @@ export const registerCommandCenterRoutes: ApiRouteRegistrar = (ctx) => { now: Date.now(), pricingOverrides: settings.modelPricingOverrides, defaultWorkflowId, + columnFlagsByName: await resolveColumnFlagsByName(store), }); if (wantsCsv(req.query)) { sendCsv(res, "command-center-workflows.csv", workflowAnalyticsToTable(result)); diff --git a/packages/dashboard/src/routes/register-project-routes.ts b/packages/dashboard/src/routes/register-project-routes.ts index 02720fc900..8083a49661 100644 --- a/packages/dashboard/src/routes/register-project-routes.ts +++ b/packages/dashboard/src/routes/register-project-routes.ts @@ -12,6 +12,8 @@ import { writeProjectIdentity, resolveWorkflowIrForTask, columnsWithFlag, + resolveTaskLifecycleColumns, + isTerminalColumnRole, } from "@fusion/core"; import type { CentralCore as CentralCoreApi, WorkflowIr } from "@fusion/core"; import { ApiError, badRequest, notFound } from "../api-error.js"; @@ -920,8 +922,34 @@ export const registerProjectRoutes: ApiRouteRegistrar = (ctx) => { // Compute live task counts from the project-specific store const tasks = await projectStore.listTasks({ slim: true }); - const activeCols = new Set(["triage", "todo", "in-progress", "in-review"]); - const activeTaskCount = tasks.filter((t) => activeCols.has(t.column)).length; + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-08:10: + Project-health "active tasks" is the negation of each card's OWN terminal lanes. + + CENSUS-INVISIBLE: a `Set` literal is a definition, not a comparison. On a renamed board it + matched nothing, so project health reported **0 active tasks** beside an in-flight agent + count that was still correct — the same silent-zero shape as the analytics tallies fixed in + #2780. A zero next to a populated neighbour reads as "this project is idle", not as "this + number is broken". + + Note the set also listed `triage`, a column U11 deleted, so one of its four entries had been + dead on every board since that cutover. + + Resolved per card through one shared IR cache; a card whose workflow will not resolve keeps + the legacy answer via `isTerminalColumnRole`'s own degraded mode. + */ + const activeIrCache = new Map(); + const terminalByTaskId = new Map(); + for (const t of tasks) { + const lanes = await resolveTaskLifecycleColumns(projectStore, t.id, activeIrCache as never).catch(() => undefined); + terminalByTaskId.set( + t.id, + lanes === undefined + ? isTerminalColumnRole(undefined, t.column) + : t.column === lanes.complete || t.column === lanes.archived, + ); + } + const activeTaskCount = tasks.filter((t) => !terminalByTaskId.get(t.id)).length; /* * FNXC:GlobalConcurrencyControls 2026-06-26-23:46: * Project health In-Flight Agents is a live read-layer count, not persisted slot bookkeeping. diff --git a/packages/engine/src/__tests__/census-baseline-corruption-guard.test.ts b/packages/engine/src/__tests__/census-baseline-corruption-guard.test.ts new file mode 100644 index 0000000000..b04624655b --- /dev/null +++ b/packages/engine/src/__tests__/census-baseline-corruption-guard.test.ts @@ -0,0 +1,98 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-15:10: + +THE INVARIANT: a corrupt census baseline fails with a DIAGNOSIS, and `--update-baseline` refuses to +regenerate on top of one. + +MEASURED CAUSE, TWICE IN ONE PROGRAM — including once by me after I had already documented it. A +rebase or cherry-pick leaves CONFLICT MARKERS in the derived baseline; the operator runs +`--update-baseline` to fix it; the strict comparison's `JSON.parse` throws FIRST, the run dies before +writing anything, and the still-conflicted file gets staged. CI then fails with a raw `SyntaxError` +naming a byte offset and nothing about what to do — I lost two rounds to exactly that. + +Both halves are covered here because fixing only the message would have left the trap intact: the +`--update-baseline` path is the one an operator reaches for, and it was the path that silently did +nothing. + +This is a guard on the guard. `--strict` is the lifecycle ratchet the whole program leans on; a +failure mode that turns it into an unreadable stack trace is worth pinning, since the response to an +inscrutable ratchet failure is to disable it. +*/ +import { execFileSync } from "node:child_process"; +import { mkdtempSync, writeFileSync, rmSync, copyFileSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { describe, expect, it, afterEach } from "vitest"; + +const REPO_ROOT = resolve(import.meta.dirname, "../../../.."); +const SCRIPT = join(REPO_ROOT, "scripts/lifecycle-column-census.mjs"); +const REAL_BASELINE = join(REPO_ROOT, "scripts/lib/lifecycle-column-census-baseline.json"); + +const CONFLICTED = `{ +<<<<<<< HEAD + "byFile": { "a.ts": 1 } +======= + "byFile": { "a.ts": 2 } +>>>>>>> other +} +`; + +let scratch: string | undefined; + +afterEach(() => { + if (scratch) rmSync(scratch, { recursive: true, force: true }); + scratch = undefined; +}); + +function runCensus(baselineContents: string, extraArgs: string[]): { status: number; stderr: string } { + /* A scratch baseline via FUSION_CENSUS_BASELINE_PATH, so the repo's real one is never touched. */ + scratch = mkdtempSync(join(tmpdir(), "fusion-census-guard-")); + const baselinePath = join(scratch, "baseline.json"); + writeFileSync(baselinePath, baselineContents); + try { + const stdout = execFileSync("node", [SCRIPT, "--strict", ...extraArgs], { + cwd: REPO_ROOT, + env: { ...process.env, FUSION_CENSUS_BASELINE_PATH: baselinePath }, + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }); + return { status: 0, stderr: stdout }; + } catch (err) { + const e = err as { status?: number; stderr?: string }; + return { status: e.status ?? 1, stderr: e.stderr ?? "" }; + } +} + +describe("the census fails readably on a corrupt baseline", () => { + it("--strict names the conflict markers and the recovery command", () => { + const { status, stderr } = runCensus(CONFLICTED, []); + + expect(status).toBe(1); + expect(stderr).toContain("is not valid JSON"); + expect(stderr).toContain("MERGE CONFLICT MARKERS"); + expect(stderr).toContain("--update-baseline"); + }); + + it("--update-baseline REFUSES rather than dying midway", () => { + // This is the path an operator reaches for, and the one that used to leave the corruption staged. + const { status, stderr } = runCensus(CONFLICTED, ["--update-baseline"]); + + expect(status).toBe(1); + expect(stderr).toContain("is not valid JSON"); + }); + + it("still succeeds against the repo's real baseline", () => { + // Guards against the diagnosis firing on a healthy file — a guard that always fails is no guard. + scratch = mkdtempSync(join(tmpdir(), "fusion-census-guard-")); + const baselinePath = join(scratch, "baseline.json"); + copyFileSync(REAL_BASELINE, baselinePath); + + const result = execFileSync("node", [SCRIPT, "--strict"], { + cwd: REPO_ROOT, + env: { ...process.env, FUSION_CENSUS_BASELINE_PATH: baselinePath }, + encoding: "utf8", + }); + + expect(result).toContain("every file matches its baseline exactly"); + }); +}); diff --git a/packages/engine/src/__tests__/executor-stale-spec-active-lanes.test.ts b/packages/engine/src/__tests__/executor-stale-spec-active-lanes.test.ts new file mode 100644 index 0000000000..4c4ece5b82 --- /dev/null +++ b/packages/engine/src/__tests__/executor-stale-spec-active-lanes.test.ts @@ -0,0 +1,77 @@ +// @vitest-environment node +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-08:10: + +THE INVARIANT: the stale-spec guard skips cards the board's OWN workflow calls active. + +THIS GUARD DID THE EXACT THING ITS OWN COMMENT SAYS IT MUST NOT. The comment above the code reads: +"Skip for tasks that are already in-progress, in-review, merging, or done — these should not be +interrupted and sent back to triage for re-planning." Keyed on a hard-coded `Set`, a renamed board +matched NOTHING, so `isActiveTask` was false for a card in a renamed wip/review/complete lane, the +guard ran on a LIVE task, and `moveTaskToReplanColumn` + `status: "needs-replan"` pulled it out of +execution mid-flight. + +`activeMergeStatuses` still covered the merging states, so a merging card was protected BY ACCIDENT +while a plain in-progress card was not — which is why the failure looks arbitrary from outside. + +CENSUS-INVISIBLE: a `Set` literal is a definition, not a comparison, so nothing in the lifecycle +backlog pointed at this site. Found by grepping for lane-shaped list literals. + +--- + +WHY THIS FILE IS PURELY STRUCTURAL, AND A MISTAKE I MADE TWICE. + +My first draft added four "behavioural" cases that built the union themselves from +`resolveLifecycleColumns` and asserted membership. Measured against a revert, **only the structural +case failed** — the other four passed with the fix removed, because they exercised a MIRROR of the +guard rather than the guard. They were really a test of core's `resolveLifecycleColumns`, which has +its own coverage. + +That is the same mirrored-implementation trap I caught and deleted in +`analytics-timing-roles-resolved.test.ts` earlier in this program. Twice is a pattern, so the rule is +written down here: **if a test constructs the value the product is supposed to construct, reverting +the product cannot fail it.** Deleted rather than shipped. + +The guard sits deep inside `execute()`, behind worktree and session setup a unit test has no business +standing up, so what remains is a source ratchet in the shape this repo already uses for +`engine-no-blocking-shellout`. It fails on revert — verified — and it is not a behavioural proof. +Whoever next touches `execute()`'s test scaffolding should add the end-to-end case. +*/ +import { describe, expect, it } from "vitest"; +import { readFileSync } from "node:fs"; + +const source = readFileSync(new URL("../executor.ts", import.meta.url), "utf8"); + +describe("the stale-spec skip resolves the board's own active lanes", () => { + it("resolves the task's lifecycle columns before deciding the skip", () => { + expect(source).toContain( + "const activeLifecycle = resolveLifecycleColumns(await resolveWorkflowIrForTask(this.store, task.id));", + ); + }); + + it("adds the wip, review and complete lanes to the active set", () => { + expect(source).toContain( + "for (const lane of [activeLifecycle?.wip, activeLifecycle?.review, activeLifecycle?.complete]) {", + ); + expect(source).toContain("if (lane !== undefined) activeColumns.add(lane);"); + }); + + it("UNIONS rather than replaces, so a degraded IR cannot narrow the set", () => { + /* + `resolveWorkflowIrForTask` hands back the BUILT-IN IR on a missing or corrupt definition instead + of throwing. If this replaced the legacy trio with the resolved lanes, that degraded case would + silently drop `in-progress` on a board still using it — re-opening the interruption this fixes in + the one situation hardest to notice. The legacy trio must remain the seed. + */ + expect(source).toContain('const activeColumns = new Set(["in-progress", "in-review", "done"]);'); + }); + + it("keeps the merge-status escape hatch alongside the column check", () => { + // A merging card was protected by accident before; that protection must survive the conversion + // rather than be replaced by it. + expect(source).toContain('const activeMergeStatuses = new Set(["merging", "merging-pr", "merging-fix"]);'); + expect(source).toContain( + 'const isActiveTask = activeColumns.has(task.column) || activeMergeStatuses.has(task.status ?? "");', + ); + }); +}); diff --git a/packages/engine/src/__tests__/scheduler-fanout-escalation-lanes.test.ts b/packages/engine/src/__tests__/scheduler-fanout-escalation-lanes.test.ts new file mode 100644 index 0000000000..92b2a0c83f --- /dev/null +++ b/packages/engine/src/__tests__/scheduler-fanout-escalation-lanes.test.ts @@ -0,0 +1,134 @@ +// @vitest-environment node +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-11:10: + +THE INVARIANT: the overlap-bottleneck warning ages a blocker using the board's OWN active lanes. + +WIRING AN OPTION NOTHING FILLED. `computeBlockerFanoutMap`'s `escalationColumns` was added as an +optional resolved answer and its only production caller — `emitHighOverlapFanoutWarnings` — passed +nothing. On a renamed board `shouldEscalate` was false for every blocker, so a long-standing +bottleneck was reported as `temporary` forever. + +The fan-out COUNT was correct throughout. That is what makes this quiet: the message names a real +problem and mis-states its age, so it reads as a fresh contention spike rather than a stuck card. + +This is the class I audited my own merged work for after #2787's review — five conversions whose +production callers passed nothing — and this is the first of them wired. The test drives +`emitHighOverlapFanoutWarnings` through the real prototype rather than re-asserting +`computeBlockerFanoutMap`, because the defect was never in the map; it was in the caller. + +REVERT PROOF, measured: stop passing `escalationColumns` and the renamed case below logs +`(temporary)` instead of `(long-lived)`. +*/ +import { describe, expect, it, vi } from "vitest"; +import type { Task, TaskStore } from "@fusion/core"; + +import { Scheduler } from "../scheduler.js"; + +const RENAMED_IR = { + version: "v2", id: "wf-renamed", name: "renamed", nodes: [], edges: [], + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "signoff", name: "Sign-off", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], +}; + +const SIX_HOURS_AGO = new Date(Date.now() - 6 * 60 * 60 * 1000).toISOString(); + +function card(id: string, column: string, extra: Record = {}): Task { + return { + id, column, dependencies: [], steps: [], log: [], status: null, + createdAt: SIX_HOURS_AGO, updatedAt: SIX_HOURS_AGO, columnMovedAt: SIX_HOURS_AGO, ...extra, + } as unknown as Task; +} + +function harness(blockerColumn: string, ir: unknown, holdColumn = "backlog") { + const selection = { workflowId: "wf-renamed", stepIds: [] as string[] }; + const logged: string[] = []; + const store = { + logEntry: vi.fn(async (_id: string, message: string) => { logged.push(message); }), + getTaskWorkflowSelection: () => (ir ? selection : undefined), + getTaskWorkflowSelectionAsync: async () => (ir ? selection : undefined), + getWorkflowDefinition: async () => (ir ? { ir } : undefined), + } as unknown as TaskStore; + + const tasks: Task[] = [ + card("B", blockerColumn), + ...[1, 2, 3, 4, 5].map((n) => card(`D${n}`, holdColumn, { blockedBy: "B" })), + ]; + + const self = { store, lastHighOverlapFanoutWarningKey: new Map() }; + const run = () => + (Scheduler.prototype as unknown as { + emitHighOverlapFanoutWarnings: (this: unknown, t: Task[]) => Promise; + }).emitHighOverlapFanoutWarnings.call(self, tasks); + + return { run, logged }; +} + +describe("the overlap-bottleneck warning ages blockers by resolved lane", () => { + it("reports a stale blocker in a RENAMED wip lane as long-lived", async () => { + // Pre-fix: `building` was in no literal set, so escalation never fired and this said "temporary". + const { run, logged } = harness("building", RENAMED_IR); + + await run(); + + expect(logged.join("\n")).toContain("(long-lived)"); + }); + + it("still reports a blocker outside the active lanes as temporary", async () => { + // The escalation must stay conditional — labelling everything long-lived is its own bug. + const { run, logged } = harness("backlog", RENAMED_IR); + + await run(); + + expect(logged.join("\n")).toContain("(temporary)"); + }); + + it("keeps the legacy behaviour when no workflow resolves", async () => { + /* + The dependents must sit in the LEGACY hold column here. My first draft left them in `backlog`, + so `overlapBlockedTodoCount` was zero, the threshold check skipped the blocker, and the case + asserted a missing message rather than a legacy one — green for the wrong reason had the + expectation been negative. Same shape as every other harness mistake in this sweep: the fixture + has to match the branch under test. + */ + const { run, logged } = harness("in-progress", undefined, "todo"); + + await run(); + + expect(logged.join("\n")).toContain("(long-lived)"); + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-18:40: +The REVIEW half of the same call, wired after the escalation half — and found only by auditing the +flat-set class by hand, not by any guard. + +`computeBlockerFanoutMap` feeds `reviewColumns` to `isStaleBlockedByBlocker`, which decides whether a +paused or retry-exhausted review blocker still counts as blocking its dependents. This call site +already passed `classify`, `escalationClassify` and `escalationColumns`; it did not pass this one, so +that half ran on the legacy `{in-review}` beside three resolved neighbours — the half-converted-pair +shape inside a call site I had converted twice already. + +My own unwired-parameter guard is blind to it: `reviewColumns` is mentioned in `task-priority.ts`, so +the name reads as used. Recorded here and in the guard's header rather than quietly patched. + +STRUCTURAL, and labelled: the consequence lives inside `isStaleBlockedByBlocker`'s classification of +a paused blocker, which this suite's harness does not construct. What is pinned is the WIRING — the +gap was that the argument was absent, not that the predicate was wrong. +*/ +describe("the fan-out call forwards the resolved review lanes too", () => { + it("passes reviewColumns alongside the escalation and classify answers", () => { + const source = readFileSync(new URL("../scheduler.ts", import.meta.url), "utf8"); + + expect(source).toContain("...(blockerReviewColumns.size > 0 ? { reviewColumns: blockerReviewColumns } : {}),"); + // Built from the same IR loop, so the two halves cannot resolve from different reads. + expect(source).toContain('for (const id of columnsWithFlag(ir, "humanReview")) blockerReviewColumns.add(id);'); + }); +}); + +import { readFileSync } from "node:fs"; diff --git a/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts b/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts new file mode 100644 index 0000000000..127bd3868d --- /dev/null +++ b/packages/engine/src/__tests__/unwired-lane-parameter-guard.test.ts @@ -0,0 +1,113 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-16:40: + +THE INVARIANT: a lane-resolution parameter that no production caller supplies is a build failure. + +WHY THIS GUARD EXISTS, measured rather than asserted. This program repeatedly shipped conversions of +the shape "optional resolved answer, documented literal fallback" and then never passed the argument +from the production caller. **Five such parameters were live on `main` at once.** Auditing them found +that in FOUR the parameter was unreachable because the CALLER held a larger defect: + + - a bottleneck warning whose count was always zero, so it never printed at all; + - analytics routes that never built the map, reporting 0 active beside correct cost totals; + - a backfill that queried a column a renamed board does not have; + - a store read returning archived cards as open assigned work. + +None of that is visible to the lifecycle census: the literal sits behind a documented fallback, so +the site counts as converted. None of it is visible to the unit tests either, because they inject the +value by hand — one of my own test files stayed green when I removed the wiring, which is what +prompted this. + +Two workers found the class independently (#2787's review and #2799). That is the argument for +detecting it mechanically rather than by sweep. + +DELIBERATELY CONSERVATIVE, because a false positive here is worse than a miss: the response to a +noisy guard is to disable it. It reports only exported declarations, only optional parameters, only +names in the lane vocabulary, and treats a MENTION anywhere else as wired. +*/ +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join, resolve } from "node:path"; +import { describe, expect, it } from "vitest"; + +import { findUnwiredLaneParameters, LANE_PARAMETER_NAMES } from "../../../../scripts/lib/unwired-lane-parameter.mjs"; + +const REPO_ROOT = resolve(import.meta.dirname, "../../../.."); +const SCANNED_PACKAGES = ["packages/core/src", "packages/engine/src", "packages/dashboard/src", "packages/dashboard/app", "packages/cli/src"]; + +function sourceFiles(dir: string, acc: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + /* Tests are excluded on purpose: a test passing the argument is exactly the false signal. */ + if (entry === "__tests__" || entry === "node_modules" || entry === "dist") continue; + sourceFiles(full, acc); + continue; + } + if (/\.(ts|tsx)$/.test(entry) && !/\.(test|spec)\.tsx?$/.test(entry)) acc.push(full); + } + return acc; +} + +describe("no lane-resolution parameter is left unwired", () => { + it("every optional lane parameter is supplied by at least one other file", () => { + const files = SCANNED_PACKAGES.flatMap((pkg) => sourceFiles(join(REPO_ROOT, pkg))); + const unwired = findUnwiredLaneParameters(files, (f) => readFileSync(f, "utf8")); + + const described = unwired.map((d) => `${d.file.replace(`${REPO_ROOT}/`, "")}:${d.line} ${d.owner}(${d.parameter})`); + + expect(described, [ + "These declarations take a resolved lane answer that NO production file supplies.", + "That is not a loose end — in four of five audited cases the caller held a larger defect.", + "Either wire the caller, or make the parameter required so the compiler finds the call sites.", + ].join("\n")).toEqual([]); + }); + + it("fires on the shape it exists to catch", () => { + // A guard nobody has proven can fail is a number, not a check. + const decl = "/repo/packages/core/src/thing.ts"; + const files = { + [decl]: 'export function isThing(task: Task, reviewColumns?: ReadonlySet) { return reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review"; }', + "/repo/packages/core/src/caller.ts": 'import { isThing } from "./thing.js"; isThing(task);', + } as Record; + + const unwired = findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!); + + expect(unwired.map((d) => `${d.owner}(${d.parameter})`)).toEqual(["isThing(reviewColumns)"]); + }); + + it("stays quiet once a caller supplies the argument", () => { + const decl = "/repo/packages/core/src/thing.ts"; + const files = { + [decl]: 'export function isThing(task: Task, reviewColumns?: ReadonlySet) { return reviewColumns?.has(task.column) ?? false; }', + "/repo/packages/core/src/caller.ts": "isThing(task, reviewColumns);", + } as Record; + + expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!)).toEqual([]); + }); + + it("ignores a REQUIRED lane parameter — the compiler already finds those call sites", () => { + const decl = "/repo/packages/core/src/thing.ts"; + const files = { + [decl]: "export function isThing(task: Task, reviewColumns: ReadonlySet) { return reviewColumns.has(task.column); }", + "/repo/packages/core/src/caller.ts": "isThing(task);", + } as Record; + + expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!)).toEqual([]); + }); + + it("covers the options-object shape, not just positional parameters", () => { + // Half the real cases arrived as an optional property on an exported options interface. + const decl = "/repo/packages/core/src/thing.ts"; + const files = { + [decl]: "export interface ThingOptions { escalationColumns?: ReadonlySet; }", + "/repo/packages/core/src/caller.ts": "computeThing(tasks, {});", + } as Record; + + expect(findUnwiredLaneParameters(Object.keys(files), (f) => files[f]!).map((d) => d.parameter)).toEqual(["escalationColumns"]); + }); + + it("keeps the vocabulary list explicit, so a new convention opts in deliberately", () => { + expect(LANE_PARAMETER_NAMES).toContain("reviewColumns"); + expect(LANE_PARAMETER_NAMES).toContain("columnFlags"); + }); +}); diff --git a/packages/engine/src/agent-heartbeat.ts b/packages/engine/src/agent-heartbeat.ts index 02fcd7e449..2104c5cda2 100644 --- a/packages/engine/src/agent-heartbeat.ts +++ b/packages/engine/src/agent-heartbeat.ts @@ -36,6 +36,7 @@ import { resolveEffectivePlannerHeartbeatPatrolEnabled, resolveReboundTarget, resolveWorkflowIrForTask, + columnsWithFlag, resolveTaskLifecycleColumns, } from "@fusion/core"; import type { ToolDefinition } from "@earendil-works/pi-coding-agent"; @@ -3098,9 +3099,29 @@ export class HeartbeatMonitor { if (!isAgentEphemeral && this.taskStore && typeof this.taskStore.getTasksByAssignedAgent === "function") { try { const assignedOpen = await this.taskStore.getTasksByAssignedAgent(agentId, { excludeArchived: true }); + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-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 + caller, passed nothing — so the conversion was inert here. Auditing it also surfaced the + larger defect one level down in `getTasksByAssignedAgent`, whose `excludeArchived` + filtered on the literal id and therefore returned archived cards as open assigned work. + Both halves are needed: the store read stops handing back archived rows, and this stops + the ranking counting a finished card as open. + */ + const wakeLaneFlags = new Map(); + const wakeIrCache = new Map>>(); + for (const assignedTask of assignedOpen) { + const ir = await resolveWorkflowIrForTask(this.taskStore, assignedTask.id, wakeIrCache).catch(() => undefined); + if (!ir) continue; + for (const id of columnsWithFlag(ir, "complete")) wakeLaneFlags.set(id, { ...wakeLaneFlags.get(id), complete: true }); + for (const id of columnsWithFlag(ir, "archived")) wakeLaneFlags.set(id, { ...wakeLaneFlags.get(id), archived: true }); + } const ranked = rankAssignedTasksForWakeDelta(assignedOpen, { agentId, boundTaskId: isNoTaskRun ? null : taskId, + ...(wakeLaneFlags.size > 0 ? { flagsByColumnId: wakeLaneFlags as never } : {}), }); const section = formatAssignedTasksWakeDeltaSection(ranked, { boundTaskId: isNoTaskRun ? null : taskId, diff --git a/packages/engine/src/executor.ts b/packages/engine/src/executor.ts index ec640d97b0..6f173ae4be 100644 --- a/packages/engine/src/executor.ts +++ b/packages/engine/src/executor.ts @@ -12498,7 +12498,32 @@ export class TaskExecutor { // so existing filesystem validation paths remain authoritative. // Skip for tasks that are already in-progress, in-review, merging, or done — // these should not be interrupted and sent back to triage for re-planning. - const activeColumns = new Set(["in-progress", "in-review", "done"]); + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-08:10: + THIS GUARD DID THE EXACT THING ITS OWN COMMENT SAYS IT MUST NOT. + + The comment directly above is explicit: skip for tasks already in-progress, in-review, merging or + done, because "these should not be interrupted and sent back to triage for re-planning". Keyed on + a hard-coded `Set`, a renamed board matched NOTHING, so `isActiveTask` was false for a card in a + renamed wip/review/complete lane — the stale-spec guard then ran on a LIVE task and + `moveTaskToReplanColumn` + `status: "needs-replan"` yanked it out of execution mid-flight. + + `activeMergeStatuses` still covers the merging states, so a merging card was protected by + accident; a plain in-progress card was not. + + CENSUS-INVISIBLE: a `Set` literal is a definition, not a comparison, so nothing in the lifecycle + backlog pointed here. Found by grepping for lane-shaped list literals. + + Resolved from the task's OWN workflow, unioned with the legacy trio for the reason documented on + `resolveTerminalColumnsFor`: `resolveWorkflowIrForTask` returns the BUILT-IN IR rather than + 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)); + 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); + } const activeMergeStatuses = new Set(["merging", "merging-pr", "merging-fix"]); const isActiveTask = activeColumns.has(task.column) || activeMergeStatuses.has(task.status ?? ""); if (!isActiveTask) { diff --git a/packages/engine/src/scheduler.ts b/packages/engine/src/scheduler.ts index 9a2c38c628..4ed8fde6ae 100644 --- a/packages/engine/src/scheduler.ts +++ b/packages/engine/src/scheduler.ts @@ -1445,7 +1445,94 @@ export class Scheduler { } private async emitHighOverlapFanoutWarnings(tasks: Task[]): Promise { - const fanoutMap = computeBlockerFanoutMap(tasks, 3); + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-11:10: + WIRING THE ESCALATION LANES — the option existed and nothing in production filled it. + + `computeBlockerFanoutMap`'s `escalationColumns` was added as an optional resolved answer and this, + its only production caller, passed nothing. So on a renamed board `shouldEscalate` was false for + every blocker and the warning below reported a long-standing bottleneck as `temporary` forever. + The fan-out COUNT was correct throughout, which is what makes it quiet: the message names a real + problem and mis-states its age. + + FLAT SET, DELIBERATELY, and the limit is worth naming: `escalationColumns` is board-wide, so on a + board spanning workflows this unions every workflow's active lanes. An id one workflow calls wip + and another calls something else is therefore treated as active for both. That over-approximates + in the CHEAP direction here — the cost is a bottleneck warning reading `long-lived` slightly too + eagerly, never a missed one — and it matches how `terminalColumns`/`reviewColumns` are already + passed in this file. A per-task answer would need the `classify`-shaped option this module + documents for the cases where the distinction actually bites. + + One IR cache for the sweep, per the caller-owned-cache contract. + */ + const escalationIrCache = new Map>>(); + /* Per-task, keyed by id — see the `escalationClassify` note in blocker-fanout.ts. The flat set is + still built alongside it as the legacy fallback for tasks whose workflow will not resolve. */ + const escalationByTaskId = new Map(); + const escalationColumns = new Set(); + const holdByTaskId = new Map(); + const terminalByTaskId = new Map(); + /* + FNXC:WorkflowLifecycleColumns 2026-07-31-18:40: + The REVIEW half was still unwired after the escalation fix, and I only found it by auditing the + rest of the flat-set class rather than by any guard. + + `computeBlockerFanoutMap` feeds `reviewColumns` to `isStaleBlockedByBlocker`, which decides + whether a paused or retry-exhausted review blocker still counts as blocking its dependents. This + call passed `classify`, `escalationClassify` and `escalationColumns` and NOT this one, so that + half ran on the legacy `{in-review}` while everything beside it was resolved — the + half-converted-pair shape, inside a call site I had already converted twice. + + Notably my own unwired-parameter guard cannot see it: `reviewColumns` is mentioned in + `task-priority.ts`, so the name reads as used. That blind spot is documented in the guard's header. + */ + const blockerReviewColumns = new Set(); + for (const task of tasks) { + const ir = await resolveWorkflowIrForTask(this.store, task.id, escalationIrCache).catch(() => undefined); + if (!ir) continue; + for (const id of columnsWithFlag(ir, "countsTowardWip")) escalationColumns.add(id); + for (const id of columnsWithFlag(ir, "mergeOrchestration")) escalationColumns.add(id); + for (const id of columnsWithFlag(ir, "mergeBlocker")) escalationColumns.add(id); + for (const id of columnsWithFlag(ir, "humanReview")) escalationColumns.add(id); + escalationByTaskId.set(task.id, [ + ...columnsWithFlag(ir, "countsTowardWip"), + ...columnsWithFlag(ir, "mergeOrchestration"), + ...columnsWithFlag(ir, "mergeBlocker"), + ...columnsWithFlag(ir, "humanReview"), + ].includes(task.column)); + for (const id of columnsWithFlag(ir, "mergeOrchestration")) blockerReviewColumns.add(id); + for (const id of columnsWithFlag(ir, "mergeBlocker")) blockerReviewColumns.add(id); + for (const id of columnsWithFlag(ir, "humanReview")) blockerReviewColumns.add(id); + holdByTaskId.set(task.id, columnsWithFlag(ir, "hold").includes(task.column)); + terminalByTaskId.set( + task.id, + columnsWithFlag(ir, "complete").includes(task.column) || columnsWithFlag(ir, "archived").includes(task.column), + ); + } + const fanoutMap = computeBlockerFanoutMap(tasks, 3, { + ...(escalationColumns.size > 0 ? { escalationColumns } : {}), + ...(blockerReviewColumns.size > 0 ? { reviewColumns: blockerReviewColumns } : {}), + /* Per-task answer where the workflow resolved; the flat set above covers the rest. */ + escalationClassify: (task: Task) => escalationByTaskId.get(task.id) ?? escalationColumns.has(task.column), + /* + `classify` is PER TASK, which this module documents as the only correct option on a board + spanning workflows — and it is required here for a reason the escalation half alone would have + hidden: `overlapBlockedTodoCount` counts dependents in the HOLD lane, whose flat default is + `"todo"`. On a renamed board that count was ZERO, so the threshold check below skipped every + blocker and no warning was emitted AT ALL. Wiring only `escalationColumns` would have fixed the + long-lived/temporary label on a message that never printed. + + A task whose workflow did not resolve is absent from both maps and falls back to the legacy + answer, matching the documented default. + */ + /* DELIBERATE-LITERAL — the unresolvable-workflow default for both halves, reviewed + 2026-07-31-11:10. A task absent from the maps above had no readable workflow, so it keeps + the answer `computeBlockerFanoutMap` would have given it with no options at all. */ + classify: (task: Task) => ({ + isHold: holdByTaskId.get(task.id) ?? task.column === "todo", + isTerminal: terminalByTaskId.get(task.id) ?? (task.column === "done" || task.column === "archived"), + }), + }); const seenBlockers = new Set(); for (const [blockerId, fanout] of fanoutMap) { diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index f0e831c5a3..7737bc9948 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -42,7 +42,6 @@ "packages/dashboard/app/components/effective-model-resolution.ts": 2, "packages/dashboard/app/components/WorkflowResultsTab.tsx": 2, "packages/dashboard/app/utils/taskRevert.ts": 2, - "packages/dashboard/app/utils/taskTiming.ts": 2, "packages/dashboard/src/github-tracking-state.ts": 2, "packages/dashboard/src/server.ts": 2, "packages/engine/src/auto-merge-finalization.ts": 2, @@ -52,7 +51,6 @@ "packages/core/src/mission-store.ts": 1, "packages/core/src/plugin-store.ts": 1, "packages/core/src/stalled-review-detector.ts": 1, - "packages/core/src/task-store/branch-and-pr-entities.ts": 1, "packages/core/src/task-store/lifecycle-ops.ts": 1, "packages/core/src/task-store/merge-queue-ops-2.ts": 1, "packages/core/src/task-store/merge-queue-ops.ts": 1, @@ -101,6 +99,8 @@ "packages/dashboard/app/components/TaskCard.tsx\u0000triage": 2, "packages/dashboard/app/components/TaskDetailModal.tsx\u0000triage": 2, "packages/engine/src/cli-agent/state-machine.ts\u0000done": 2, + "packages/engine/src/scheduler.ts\u0000archived": 2, + "packages/engine/src/scheduler.ts\u0000done": 2, "packages/engine/src/scheduler.ts\u0000in-progress": 2, "packages/engine/src/scheduler.ts\u0000in-review": 2, "packages/engine/src/usage-limit-detector.ts\u0000archived": 2, @@ -116,6 +116,7 @@ "packages/core/src/store.ts\u0000done": 1, "packages/core/src/store.ts\u0000in-progress": 1, "packages/core/src/store.ts\u0000todo": 1, + "packages/core/src/task-store/branch-and-pr-entities.ts\u0000archived": 1, "packages/core/src/task-store/task-store-helpers.ts\u0000in-progress": 1, "packages/core/src/task-store/task-store-helpers.ts\u0000todo": 1, "packages/core/src/task-store/task-update.ts\u0000in-progress": 1, @@ -146,8 +147,7 @@ "packages/engine/src/hold-release.ts\u0000done": 1, "packages/engine/src/hold-release.ts\u0000in-review": 1, "packages/engine/src/project-engine.ts\u0000in-review": 1, - "packages/engine/src/scheduler.ts\u0000archived": 1, - "packages/engine/src/scheduler.ts\u0000done": 1, + "packages/engine/src/scheduler.ts\u0000todo": 1, "packages/engine/src/triage.ts\u0000triage": 1, "plugins/fusion-plugin-even-cards/src/cards/board-cards.ts\u0000archived": 1, "plugins/fusion-plugin-even-cards/src/cards/board-cards.ts\u0000done": 1, @@ -171,7 +171,6 @@ "packages/core/src/task-store/async-archive-lineage.ts": 1, "packages/core/src/task-store/async-self-healing.ts": 1, "packages/core/src/task-store/task-artifacts-ops.ts": 1, - "packages/core/src/task-store/task-store-helpers.ts": 1, "packages/dashboard/src/routes/register-gitlab.ts": 1, "packages/engine/src/agent-tools.ts": 1, "packages/engine/src/auto-merge-finalization.ts": 1, diff --git a/scripts/lib/unwired-lane-parameter.mjs b/scripts/lib/unwired-lane-parameter.mjs new file mode 100644 index 0000000000..dbb54bd8e9 --- /dev/null +++ b/scripts/lib/unwired-lane-parameter.mjs @@ -0,0 +1,126 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-16:40: +Find lane-resolution parameters that NO production caller supplies. + +WHY THIS EXISTS. The lifecycle-column program repeatedly shipped a conversion shaped like this: + + export function isSomething(task, reviewColumns?: ReadonlySet) { + return reviewColumns ? reviewColumns.has(task.column) : task.column === "in-review"; + } + +…and then never passed `reviewColumns` from the production caller. The census counts the site as +converted (the literal is behind a documented fallback), every test passes (they inject the value by +hand), and production keeps the legacy behaviour. Measured: FIVE such parameters were live on `main` +at once, and auditing them found that in FOUR the parameter was unreachable because the CALLER held a +larger defect — a query for a column the board does not have, a count that was always zero, a store +read returning archived rows as open work. + +Two workers found this class independently (#2787's review and #2799), which is the argument for +detecting it mechanically instead of by sweep. Unlike the census, the shape IS statically decidable: +an exported declaration has an optional parameter whose name is lane-shaped, and no file anywhere +mentions that name as an argument. + +DELIBERATELY CONSERVATIVE. It only reports a parameter when: + - the declaration is exported (an internal helper's callers are all in-file and easy to see); + - the parameter is optional (a required one cannot be silently skipped); + - the name matches the lane vocabulary this program actually uses; + - and NO file in the scanned set mentions that name outside the declaring file. + +The last condition is deliberately loose — a mention is enough. A guard that argues about how a value +reaches a call site would produce false positives, and a false positive here costs more than a miss: +it teaches people to disable the check. +*/ + +import { createRequire } from "node:module"; +import { readFileSync } from "node:fs"; + +const require = createRequire(import.meta.url); +const ts = require("typescript"); + +/** + * Parameter names this program uses for a resolved lane answer. + * + * A NAME list rather than a type check on purpose: the same fact is spelled `ReadonlySet`, + * `ColumnRoleFlags`, `boolean` and `(task) => boolean` across the packages, so the type tells you + * less than the name does. Adding a name here is how a new convention opts into the guard. + */ +export const LANE_PARAMETER_NAMES = [ + "activeColumns", + "columnFlags", + "columnFlagsByColumnId", + "columnFlagsByName", + "completeColumnsByTaskId", + "escalationColumns", + "flagsByColumnId", + "holdColumn", + "isReviewColumn", + "isWipColumn", + "reviewColumns", + "satisfactionColumnsByTaskId", + "terminalColumns", + "terminalColumnsByTaskId", +]; + +const LANE_PARAMETER_SET = new Set(LANE_PARAMETER_NAMES); + +function isExported(node) { + const modifiers = node.modifiers ?? []; + return modifiers.some((m) => m.kind === ts.SyntaxKind.ExportKeyword); +} + +/** Declarations whose parameters are worth checking: exported functions and exported interfaces. */ +function collectLaneParameters(filePath, source) { + const sourceFile = ts.createSourceFile(filePath, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX); + const found = []; + + const recordParam = (param, ownerName) => { + if (!param.name || !ts.isIdentifier(param.name)) return; + if (!LANE_PARAMETER_SET.has(param.name.text)) return; + /* Only OPTIONAL parameters can be silently skipped; a required one fails to compile. */ + if (!param.questionToken && !param.initializer) return; + const { line } = sourceFile.getLineAndCharacterOfPosition(param.getStart(sourceFile)); + found.push({ file: filePath, line: line + 1, parameter: param.name.text, owner: ownerName }); + }; + + const visit = (node) => { + if (ts.isFunctionDeclaration(node) && isExported(node) && node.name) { + for (const param of node.parameters) recordParam(param, node.name.text); + } + /* An options-object property is the same fact wearing a different shape. */ + if (ts.isInterfaceDeclaration(node) && isExported(node)) { + for (const member of node.members) { + if (!ts.isPropertySignature(member) || !member.name || !ts.isIdentifier(member.name)) continue; + if (!LANE_PARAMETER_SET.has(member.name.text)) continue; + if (!member.questionToken) continue; + const { line } = sourceFile.getLineAndCharacterOfPosition(member.getStart(sourceFile)); + found.push({ file: filePath, line: line + 1, parameter: member.name.text, owner: node.name.text }); + } + } + ts.forEachChild(node, visit); + }; + + visit(sourceFile); + return found; +} + +/** + * @param files absolute paths to scan (production sources; callers exclude tests) + * @param readFile injected for testability + * @returns declarations whose lane parameter is mentioned in no other file + */ +export function findUnwiredLaneParameters(files, readFile = (f) => readFileSync(f, "utf8")) { + const sources = new Map(); + for (const file of files) sources.set(file, readFile(file)); + + const declarations = []; + for (const [file, source] of sources) declarations.push(...collectLaneParameters(file, source)); + + return declarations.filter((declaration) => { + for (const [file, source] of sources) { + if (file === declaration.file) continue; + /* A mention anywhere else counts as wired. Deliberately loose — see the header. */ + if (source.includes(declaration.parameter)) return false; + } + return true; + }); +} diff --git a/scripts/lifecycle-column-census.mjs b/scripts/lifecycle-column-census.mjs index 3d203c38e7..814344f96a 100644 --- a/scripts/lifecycle-column-census.mjs +++ b/scripts/lifecycle-column-census.mjs @@ -262,7 +262,38 @@ if (!existsSync(BASELINE_PATH)) { process.exit(1); } -const baseline = JSON.parse(readFileSync(BASELINE_PATH, "utf8")); +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-15:10: +FAIL WITH A DIAGNOSIS, not a stack trace, when the baseline is not valid JSON. + +Measured cause, twice in one program: a rebase or cherry-pick leaves CONFLICT MARKERS in the baseline, +the operator runs `--update-baseline` to "fix" it, THIS parse throws first, the run dies before +writing, and the still-conflicted file gets staged. `--strict` then fails in CI with a raw +`SyntaxError` that names a byte offset and nothing about what to do. + +Both halves are covered: `--strict` explains the real cause and the fix, and `--update-baseline` +REFUSES to run against an unparseable baseline rather than reading it and dying midway. Regenerating +is safe (the file is derived), but it must be a deliberate act with the corruption named, not a side +effect of a command that appears to have worked. +*/ +function readBaselineOrExplain() { + const raw = readFileSync(BASELINE_PATH, "utf8"); + try { + return JSON.parse(raw); + } catch (err) { + const conflicted = /^(<{7}|={7}|>{7})/m.test(raw); + console.error(`lifecycle-column-census: ${BASELINE_PATH} is not valid JSON (${err.message}).`); + if (conflicted) { + console.error(" It still contains MERGE CONFLICT MARKERS — a rebase or cherry-pick left them behind."); + } + console.error(" The baseline is derived, so regenerate it from the target branch rather than hand-editing:"); + console.error(" git show origin/main:scripts/lib/lifecycle-column-census-baseline.json > scripts/lib/lifecycle-column-census-baseline.json"); + console.error(" node scripts/lifecycle-column-census.mjs --strict --update-baseline"); + process.exit(1); + } +} + +const baseline = readBaselineOrExplain(); const baselineByFile = new Map(Object.entries(baseline.byFile ?? {})); const currentByFile = new Map(summary.byFile); /* @@ -425,6 +456,9 @@ function writeBaseline() { } if (updateBaseline) { + /* Refuse to regenerate ON TOP of a corrupt file: the run would die inside the strict comparison + below and leave the corruption staged, which is exactly how it reached CI twice. */ + if (existsSync(BASELINE_PATH)) readBaselineOrExplain(); writeBaseline(); if (regressions.length > 0) { console.log("\n ACCEPTED RISES (a merge or a conversion added guards here — convert them or they stay in the bar):");