diff --git a/.changeset/capacity-pool-sentinel.md b/.changeset/capacity-pool-sentinel.md new file mode 100644 index 0000000000..7c062e5c5c --- /dev/null +++ b/.changeset/capacity-pool-sentinel.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix the column capacity check so a task with no workflow selection is counted against the limit. +category: fix +dev: `moves.ts` asked `countActiveInCapacitySlotAsync` for pool `"builtin:coding"` while the counter buckets selection-less rows under `DEFAULT_WORKFLOW_POOL_ID`, so the count was always 0 and a finite limit could never bind. Both sides now derive the pool through the shared `resolveCapacityPoolId`. NOTE: no operator-visible change yet — the capacity block is still gated on `experimentalFeatures.workflowColumns`, which nothing in production sets (Phase A3 R2). If that gate is removed, this becomes user-visible and the changeset should be re-categorised. diff --git a/package.json b/package.json index 2ca66b86d2..b6986dab4d 100644 --- a/package.json +++ b/package.json @@ -14,14 +14,14 @@ "type": "module", "packageManager": "pnpm@10.33.0", "scripts": { - "pretest": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-routes-modular.mjs", - "pretest:full": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-routes-modular.mjs", + "pretest": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-routes-modular.mjs", + "pretest:full": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-routes-modular.mjs", "check:line-count": "node scripts/check-file-line-count.mjs", "check:routes-modular": "node scripts/check-routes-modular.mjs", "check:changesets": "node scripts/check-changeset-format.mjs", "check:quarantine-ledger": "node scripts/check-quarantine-ledger.mjs", "check:mock-completeness": "node scripts/check-mock-completeness.mjs", - "test:gate": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-mock-completeness.mjs && sh -c 'pnpm --filter @fusion/engine test:core & engine_pid=$!; pnpm --filter @fusion/core test:pg-gate & pg_pid=$!; status=0; wait $engine_pid || status=1; wait $pg_pid || status=1; exit $status' && pnpm --filter @runfusion/fusion test:ci-shape", + "test:gate": "node scripts/check-no-nohup.mjs && node scripts/check-no-cwd-relative-dashboard-test-reads.mjs && node scripts/check-no-kill-4040.mjs && node scripts/check-no-getdatabase.mjs && node scripts/check-capacity-pool-id.mjs && node scripts/check-no-node-only-core-imports-in-dashboard.mjs && node scripts/check-pi-versions-pinned.mjs && node scripts/check-no-test-timeout-appeasement.mjs && node scripts/check-changeset-format.mjs && node scripts/check-mock-completeness.mjs && sh -c 'pnpm --filter @fusion/engine test:core & engine_pid=$!; pnpm --filter @fusion/core test:pg-gate & pg_pid=$!; status=0; wait $engine_pid || status=1; wait $pg_pid || status=1; exit $status' && pnpm --filter @runfusion/fusion test:ci-shape", "smoke:boot": "node scripts/boot-smoke.mjs", "local": "node scripts/start-local.mjs", "dev": "node scripts/dev-with-memory.mjs", diff --git a/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts b/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts index 426c798ec1..1090d4804e 100644 --- a/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts +++ b/packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts @@ -370,34 +370,43 @@ pgTest("move-path equivalence — the flag gates MORE than side effects (Phase A expect(hooksErr!.message).toContain("Unknown column for this workflow"); }); - it("UNPROVEN: in-transaction column capacity did NOT reject on EITHER path in this fixture", async () => { + it("DIVERGENCE: in-transaction capacity rejects on the HOOKS path only — the inline path cannot run it", async () => { /* - HONEST NEGATIVE RESULT — recorded rather than dropped, because the code gate - is real and the practical impact is not what reading it suggests. + FNXC:WorkflowCapacity 2026-07-28-19:40 (pool-id sentinel fix): + WAS `UNPROVEN: … did NOT reject on EITHER path`. That test recorded an honest + negative result and left the cause open: "something further in + (`resolveColumnCapacity`'s limit resolution, or what + `countActiveInCapacitySlotAsync` counts as an occupant) keeps the check from + firing even when the flag is on. This suite does not establish which." It + also predicted its own obsolescence: "if a future change makes this reject, + that is the capacity gate coming alive." - STRUCTURALLY, `moves.ts` wraps the whole in-transaction capacity block in - `if (useWorkflow && workflowIr && fromColumn !== toColumn)`, so it cannot run - on the LIVE inline path at all. The obvious inference is "converging onto the - hooks path turns store-level capacity rejection ON for every project" — a - serious blast radius, since `capacity-exhausted` is a code the graph column - boundary parks on and the promote route surfaces to operators. + THE ANSWER, established by the sentinel fix: neither of those guesses. The + counter and the limit resolution were both fine. `moves.ts` asked the counter + for occupants of pool `"builtin:coding"` while the counter buckets + selection-less rows under `DEFAULT_WORKFLOW_POOL_ID`, so the count came back + 0 for a pool nothing is ever placed in. Both sides now derive the pool + through `resolveCapacityPoolId`, and the hooks path rejects. - EMPIRICALLY that inference does not hold up: with `maxConcurrent: 1` and a - wip column already occupied, the second move was ACCEPTED on both paths. So - something further in (`resolveColumnCapacity`'s limit resolution, or what - `countActiveInCapacitySlotAsync` counts as an occupant — a task with no - session/agent may not count) keeps the check from firing even when the flag - is on. This suite does not establish which. - - Consequence for the convergence decision: the capacity blast radius is - UNQUANTIFIED, not absent. It needs its own investigation before either path - is made authoritative — this test pins today's observed behavior so that - investigation starts from a fact instead of the code reading. + The blast-radius question this test was holding open is therefore ANSWERED for + the hooks path and STILL OPEN for the inline one: the inline path remains + structurally unable to run the block (`if (useWorkflow && …)`), so converging + the paths still turns store-level capacity rejection on for every project. + That convergence stays an operator decision — see the R2 note in + workflow-capacity-invariant.pg.test.ts. */ const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); + /* Each phase starts from an EMPTY wip column. Before the gate bound, the two phases could share + one fixture because nothing ever counted occupants; now they cannot — the inline phase leaves + two cards in wip, and the hooks phase's own HOLDER move would trip the cap before the + contended move under test ever runs (observed: "column at capacity (2/1)"). Evacuating is + what keeps this a test of the contender's move rather than of fixture residue. */ async function fillThenMoveSecond(): Promise { + for (const stale of await store.listTasks({ includeArchived: false })) { + if (stale.column === "in-progress") await store.deleteTask(stale.id); + } const first = await store.createTask({ description: "capacity holder" }); await store.moveTask(first.id, "todo"); await store.moveTask(first.id, "in-progress"); @@ -407,11 +416,14 @@ pgTest("move-path equivalence — the flag gates MORE than side effects (Phase A } await setPath("inline"); + // Unchanged: the block is unreachable on this path regardless of the pool id. expect(await fillThenMoveSecond()).toBeNull(); await setPath("hooks"); - // If a future change makes this reject, that is the capacity gate coming - // alive — and this failure is the signal to re-open the blast-radius question. - expect(await fillThenMoveSecond()).toBeNull(); + const hooksErr = await fillThenMoveSecond(); + expect(hooksErr).toBeInstanceOf(Error); + expect((hooksErr as unknown as { rejection?: { code?: string } }).rejection?.code).toBe( + "capacity-exhausted", + ); }); }); diff --git a/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts b/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts index d7a9a3e44b..7a035b209e 100644 --- a/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts +++ b/packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts @@ -111,9 +111,25 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { return { error, secondColumn: (await store.getTask(contender.id))?.column }; } - it("DEFECT (R2): on the LIVE inline path the in-txn capacity check cannot run at all", async () => { - // Not a surprise, but it is the reason the defect is latent rather than - // active today: the whole block is inside `if (useWorkflow && …)`. + it("DEFECT (R2, STILL LIVE): on the production inline path the in-txn capacity check cannot run at all", async () => { + /* + FNXC:WorkflowCapacity 2026-07-28-19:05: + R1 IS FIXED; R2 IS NOT, and this test is the standing evidence. The whole + capacity block sits inside `if (useWorkflow && …)`, and `useWorkflow` reads + `experimentalFeatures.workflowColumns === true`, which NOTHING in production + sets (it is absent from DEFAULT_GLOBAL_SETTINGS and has no writer outside + tests). So on the path real projects take, the gate still does not run at all + and cards still enter wip past the cap. + + Concretely, measured on this suite's fixture with maxConcurrent 1: + flag OFF, no selection -> ADMITTED (this test) + flag OFF, selection -> ADMITTED + flag ON, no selection -> REFUSED (was ADMITTED before the R1 fix) + flag ON, selection -> REFUSED + Making the gate bind for real projects means removing the `useWorkflow` + condition from the capacity block — a separate, larger blast radius than the + sentinel fix, and an operator decision rather than a drive-by. + */ const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); await setPath("inline"); @@ -124,12 +140,14 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { expect(secondColumn).toBe("in-progress"); // limit of 1, two occupants }); - it("DEFECT (R1): flag-ON, a NO-SELECTION task still slips the limit — the pool sentinels disagree", async () => { + it("FIXED (R1): flag-ON, a NO-SELECTION task is now REFUSED at the limit", async () => { /* - THE ACTUAL BUG. `moves.ts:319` asks the counter for pool "builtin:coding"; - `countActiveInCapacitySlotAsyncImpl` buckets no-selection rows under - "__default-workflow__". No occupant is ever counted, so the limit cannot - bind. Flip this expectation to a rejection when the sentinel is fixed. + FNXC:WorkflowCapacity 2026-07-28-19:05: + Was `DEFECT (R1)`, asserting the wrong-but-current outcome. The pool-id + sentinel is fixed: `moves.ts` and the counter now BOTH derive the pool through + `resolveCapacityPoolId`, so a selection-less row is bucketed and asked about + under the same key and the limit binds. Flipping this expectation is the + acceptance test the original ratchet named. */ const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); @@ -137,8 +155,12 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { const { error, secondColumn } = await fillWipThenAdmitSecond(); - expect(error).toBeNull(); - expect(secondColumn).toBe("in-progress"); + expect(error).toBeInstanceOf(Error); + expect((error as unknown as { rejection?: { code?: string } }).rejection?.code).toBe( + "capacity-exhausted", + ); + expect(secondColumn).toBe("todo"); // refused, stays put + }); it("DISCRIMINATOR: with an EXPLICIT builtin:coding selection the sentinels agree and the limit BINDS", async () => { @@ -181,8 +203,8 @@ pgTest("in-transaction column capacity — ground truth (Phase A3)", () => { invariant nobody can prove is an invariant nobody has; this is the proof obligation, written down. */ - it.fails( - "INVARIANT (currently BROKEN): a move into a full capacity column is refused, even with no workflow selection", + it( + "INVARIANT (HOLDS on the flag-ON path): a move into a full capacity column is refused, even with no workflow selection", async () => { const store = h.store(); await store.updateSettings({ maxConcurrent: 1 }); diff --git a/packages/core/src/index.gate.ts b/packages/core/src/index.gate.ts index fa12580c2a..e226a70954 100644 --- a/packages/core/src/index.gate.ts +++ b/packages/core/src/index.gate.ts @@ -416,7 +416,7 @@ export { } from "./plugin-gate-verdict.js"; export type { PluginGateVerdict, ColumnPluginGate } from "./plugin-gate-verdict.js"; // ── U6: workflow capacity (WIP) resolution shared by store + sweep ─────────── -export { resolveColumnCapacity, DEFAULT_WORKFLOW_POOL_ID } from "./workflow-capacity.js"; +export { resolveColumnCapacity, DEFAULT_WORKFLOW_POOL_ID, resolveCapacityPoolId } from "./workflow-capacity.js"; export type { ColumnCapacity } from "./workflow-capacity.js"; // ── U5: workflow lifecycle reconciliation (switch / edit / delete) ─────────── export { diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index fe56e7cd21..8abfe3f004 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -447,7 +447,7 @@ export { } from "./plugin-gate-verdict.js"; export type { PluginGateVerdict, ColumnPluginGate } from "./plugin-gate-verdict.js"; // ── U6: workflow capacity (WIP) resolution shared by store + sweep ─────────── -export { resolveColumnCapacity, resolveWipBudgetColumns, DEFAULT_WORKFLOW_POOL_ID } from "./workflow-capacity.js"; +export { resolveColumnCapacity, resolveWipBudgetColumns, DEFAULT_WORKFLOW_POOL_ID, resolveCapacityPoolId } from "./workflow-capacity.js"; export { createWorkflowEventBus, getWorkflowEventBus, emitWorkflowLifecycleEvent, resetWorkflowEventBusForTesting } from "./workflow-events.js"; export type { WorkflowEventBus, WorkflowEventSubscriber, WorkflowEventSubscription } from "./workflow-events.js"; export { findWorkflowEventShapeViolations, isIdsOnlyWorkflowEvent, MAX_ID_VALUE_LENGTH } from "./types/workflow-events.js"; diff --git a/packages/core/src/task-store/moves.ts b/packages/core/src/task-store/moves.ts index 650d2cf6e1..a61773a2f0 100644 --- a/packages/core/src/task-store/moves.ts +++ b/packages/core/src/task-store/moves.ts @@ -19,7 +19,7 @@ import {isBuiltinWorkflowId, getBuiltinWorkflow, resolveDefaultWorkflowIr, DEFAU import {parseWorkflowIr} from "../workflow-ir.js"; import {findWorkflowColumn, resolveColumnPluginGates} from "../plugin-gate-verdict.js"; import {getTraitRegistry, resolveColumnFlags} from "../trait-registry.js"; -import {resolveColumnCapacity, resolveWipBudgetColumns} from "../workflow-capacity.js"; +import {resolveColumnCapacity, resolveWipBudgetColumns, resolveCapacityPoolId} from "../workflow-capacity.js"; import { type TransitionColumnFacts, evaluateCapacityRejection, @@ -315,9 +315,19 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum // capacity check is not a guard (U6 fills the enforcement; U4 leaves a // pass-through slot). An explicit option value wins; otherwise derive it. const bypassGuards = store.resolveWorkflowBypassGuards(moveSource, options); - const effectiveWorkflowIdForMove = useWorkflow - ? (await store.getTaskWorkflowSelectionAsync(id))?.workflowId ?? "builtin:coding" - : "builtin:coding"; + /* + FNXC:WorkflowCapacity 2026-07-28-19:05 (pool-id sentinel fix): + ONE selection read, TWO derived ids, because they are two different things + that were previously conflated into one variable — and the conflation is the + defect. The capacity POOL key must match how the counter buckets rows + (`resolveCapacityPoolId`, shared); the WORKFLOW id is telemetry and must stay + a real workflow id, never the bucketing sentinel. + */ + const workflowSelectionForMove = useWorkflow + ? await store.getTaskWorkflowSelectionAsync(id) + : undefined; + const effectiveWorkflowIdForMove = workflowSelectionForMove?.workflowId ?? DEFAULT_WORKFLOW_ID; + const capacityPoolIdForMove = resolveCapacityPoolId(workflowSelectionForMove?.workflowId); const workflowIr: WorkflowIr | undefined = useWorkflow ? await resolveTaskWorkflowIrForMove(store, id) : undefined; @@ -932,7 +942,7 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum store.countActiveInCapacitySlotAsync({ tx, targetColumn: budgetColumn, - workflowId: effectiveWorkflowIdForMove, + workflowId: capacityPoolIdForMove, countPending, excludeTaskId: id, }), diff --git a/packages/core/src/task-store/project-store-ops.ts b/packages/core/src/task-store/project-store-ops.ts index aac0e95e78..7b31b60441 100644 --- a/packages/core/src/task-store/project-store-ops.ts +++ b/packages/core/src/task-store/project-store-ops.ts @@ -9,6 +9,7 @@ * instance as its first parameter and performs byte-identical work. */ import {TaskStore, storeLog, WORKFLOW_COMPILED_STEP_TEMPLATE_PREFIX, WORKFLOW_MOVE_POLICY_TIMEOUT_MS} from "../store.js"; +import { resolveCapacityPoolId } from "../workflow-capacity.js"; import {TransitionRejectionError} from "./errors.js"; import * as schema from "../postgres/schema/index.js"; import {and, eq, isNull, ne, or, sql} from "drizzle-orm"; @@ -764,7 +765,7 @@ export function countActiveInCapacitySlotSyncImpl(store: TaskStore, params: { ta let count = 0; for (const row of rows) { - const effectiveWorkflowId = row.wid ?? TaskStore.DEFAULT_WORKFLOW_POOL_ID; + const effectiveWorkflowId = resolveCapacityPoolId(row.wid); if (effectiveWorkflowId !== workflowId) continue; if (row.col === targetColumn) { @@ -816,7 +817,7 @@ export async function countActiveInCapacitySlotAsyncImpl(store: TaskStore, param let count = 0; for (const row of rows) { - const effectiveWorkflowId = row.wid ?? TaskStore.DEFAULT_WORKFLOW_POOL_ID; + const effectiveWorkflowId = resolveCapacityPoolId(row.wid); if (effectiveWorkflowId !== workflowId) continue; if (row.col === targetColumn) { diff --git a/packages/core/src/task-store/task-store-helpers.ts b/packages/core/src/task-store/task-store-helpers.ts index 84c770fdfa..7824fe2ce9 100644 --- a/packages/core/src/task-store/task-store-helpers.ts +++ b/packages/core/src/task-store/task-store-helpers.ts @@ -11,6 +11,7 @@ */ import { TaskStore } from "../store.js"; +import { resolveCapacityPoolId } from "../workflow-capacity.js"; import { isBuiltinWorkflowId } from "../builtin-workflows.js"; import { InsightStore } from "../insight-store.js"; import { ResearchStore } from "../research-store.js"; @@ -287,7 +288,7 @@ export function resolveTaskCustomFieldDefsSyncImpl(store: TaskStore, taskId: str export function resolveEffectiveWorkflowIdSyncImpl(store: TaskStore, taskId: string): string { const selection = store.getTaskWorkflowSelection(taskId); - return selection?.workflowId ?? TaskStore.DEFAULT_WORKFLOW_POOL_ID; + return resolveCapacityPoolId(selection?.workflowId); } export async function clearTaskWorkflowSelectionImpl(store: TaskStore, taskId: string): Promise { diff --git a/packages/core/src/workflow-capacity.ts b/packages/core/src/workflow-capacity.ts index 4b2549c933..bd022e1b15 100644 --- a/packages/core/src/workflow-capacity.ts +++ b/packages/core/src/workflow-capacity.ts @@ -34,6 +34,33 @@ const DEFAULT_WIP_COLUMN_ID = "in-progress"; * is not a real workflow row id (no `builtin:`/custom collision possible). */ export const DEFAULT_WORKFLOW_POOL_ID = "__default-workflow__"; +/* +FNXC:WorkflowCapacity 2026-07-28-19:05 (pool-id sentinel fix): +THE one place the "no selection → which pool?" convention is expressed. + +It exists because the convention was previously restated at each end of the +comparison, and the two restatements disagreed: the COUNTER bucketed +selection-less rows under `DEFAULT_WORKFLOW_POOL_ID` while `moves.ts` asked the +counter for pool `"builtin:coding"`. Nothing ever landed in the pool being asked +about, so the count came back 0 and a finite limit could never bind — the +in-transaction capacity gate was structurally dead for every default-workflow +task (Phase A3, R1). + +A shared constant alone would NOT have prevented that: both sides had the +constant available and one of them still wrote a literal. Both sides now call +THIS function, so "what pool does a selection-less task belong to" has exactly +one answer and no call site is in a position to disagree with it. + +NOT to be confused with `DEFAULT_WORKFLOW_ID` ("builtin:coding"). That is a real, +resolvable workflow row id and is the correct fallback when the value is used to +RESOLVE AN IR (as `scheduler.ts` does). This is a bucketing key that deliberately +cannot collide with any workflow id. Using either one in the other's role is the +bug this function exists to make unspellable. +*/ +export function resolveCapacityPoolId(selectionWorkflowId: string | null | undefined): string { + return selectionWorkflowId ?? DEFAULT_WORKFLOW_POOL_ID; +} + /** Resolved capacity configuration for a single column. */ export interface ColumnCapacity { /** True when the column carries a capacity (`wip`/`countsTowardWip`) trait. */ diff --git a/packages/engine/src/__tests__/capacity-pool-id-check.test.ts b/packages/engine/src/__tests__/capacity-pool-id-check.test.ts new file mode 100644 index 0000000000..76833984b2 --- /dev/null +++ b/packages/engine/src/__tests__/capacity-pool-id-check.test.ts @@ -0,0 +1,177 @@ +/* +FNXC:WorkflowCapacity 2026-07-28-22:45 (PR #2488 review — ratchet regression suite): + +THE ACCEPTANCE TEST FOR A GUARD IS NOT "IT PASSES ON MAIN". + +The first version of `check-capacity-pool-id` passed on main and passed on the +REINTRODUCED ORIGINAL DEFECT, because it matched one spelling of the fallback +(`?? DEFAULT_WORKFLOW_POOL_ID`) and the real bug used another +(`?? "builtin:coding"`). Green on the bug it exists to prevent is worse than no +guard: it stops anyone looking. + +So every form the guard must catch is pinned here as a case. The first fixture is +the ACTUAL PRE-FIX `moves.ts` shape, reduced — if this suite ever goes green on +that, the ratchet has silently narrowed again and this file is what says so. + +The negative cases matter just as much: `?? "builtin:coding"` is the legitimate +default for a WORKFLOW id in about eight places (scheduler's IR-resolution key, +task creation, analytics). A guard that fired on those would be suppressed until +it rotted, so the rule is deliberately about what reaches a CAPACITY SINK, not +about a spelling. +*/ +import { describe, expect, it } from "vitest"; + +import { findViolationsInSource } from "../../../../scripts/lib/capacity-pool-id-check.mjs"; + +const rules = (src: string): string[] => + (findViolationsInSource("packages/core/src/task-store/example.ts", src) as Array<{ rule: string }>).map( + (v) => v.rule, + ); + +describe("check-capacity-pool-id ratchet — forms it MUST catch", () => { + it("catches the ORIGINAL DEFECT: a `?? \"builtin:coding\"` local reaching the counter", () => { + /* The reduced pre-fix moves.ts. The previous regex version passed on this. */ + const src = ` + async function moveTaskInternalImpl(store: any, id: string) { + const effectiveWorkflowIdForMove = useWorkflow + ? (await store.getTaskWorkflowSelectionAsync(id))?.workflowId ?? "builtin:coding" + : "builtin:coding"; + await store.countActiveInCapacitySlotAsync({ + tx, targetColumn: budgetColumn, workflowId: effectiveWorkflowIdForMove, countPending, + }); + } + `; + expect(rules(src)).toContain("unresolved-pool-into-capacity-sink"); + }); + + it("catches a bare literal passed inline to the counter", () => { + const src = ` + async function f(store: any) { + await store.countActiveInCapacitySlotAsync({ workflowId: "builtin:coding" }); + } + `; + expect(rules(src)).toContain("unresolved-pool-into-capacity-sink"); + }); + + it("catches a MULTILINE fallback — the AST sees one node regardless of formatting", () => { + const src = ` + const poolId = + selection + ?.workflowId + ?? + DEFAULT_WORKFLOW_POOL_ID; + `; + expect(rules(src)).toContain("sentinel-fallback"); + }); + + it("catches a DEEPLY QUALIFIED sentinel reference", () => { + const src = `const poolId = selection?.workflowId ?? Foo.Bar.Baz.DEFAULT_WORKFLOW_POOL_ID;`; + expect(rules(src)).toContain("sentinel-fallback"); + }); + + it("catches the sentinel's RAW STRING VALUE, not just its constant name", () => { + const src = `const poolId = selection?.workflowId ?? "__default-workflow__";`; + expect(rules(src)).toContain("sentinel-fallback"); + }); + + it("catches an underived POSITIONAL argument to countCapacitySlot", () => { + const src = ` + const workflowId = byTask.get(task.id) ?? "builtin:coding"; + const n = countCapacitySlot(allTasks, byTask, budgetColumns, workflowId, countPending); + `; + expect(rules(src)).toContain("unresolved-pool-into-capacity-sink"); + }); + + it("catches a sink fed by a local that is NOT resolver-derived", () => { + const src = ` + const poolId = someOtherThing(); + await store.countActiveInCapacitySlotAsync({ workflowId: poolId }); + `; + expect(rules(src)).toContain("unresolved-pool-into-capacity-sink"); + }); +}); + +describe("check-capacity-pool-id ratchet — forms it must NOT flag", () => { + it("accepts a direct resolver call at the sink", () => { + const src = ` + await store.countActiveInCapacitySlotAsync({ + workflowId: resolveCapacityPoolId(selection?.workflowId), + }); + `; + expect(rules(src)).toEqual([]); + }); + + it("accepts a local derived from the resolver, declared before OR after the sink", () => { + const before = ` + const poolId = resolveCapacityPoolId(selection?.workflowId); + await store.countActiveInCapacitySlotAsync({ workflowId: poolId }); + `; + expect(rules(before)).toEqual([]); + }); + + it("accepts `?? \"builtin:coding\"` when it is a WORKFLOW id and never reaches a capacity sink", () => { + /* scheduler.ts's IR-resolution key. Flagging this would make the guard noise + that gets suppressed — the rule is about the sink, not the spelling. */ + const src = ` + const workflowIdByTaskId = new Map(); + workflowIdByTaskId.set(task.id, selection?.workflowId ?? "builtin:coding"); + const ir = await resolveWorkflowIrById(store, workflowId, cache); + `; + expect(rules(src)).toEqual([]); + }); + + it("accepts naming the sentinel constant where nothing is derived from a selection", () => { + /* scheduler's capacity DIAGNOSTIC label — no selection input, nothing to drift. */ + const src = `const perColumnGates = [{ workflowId: DEFAULT_WORKFLOW_POOL_ID, columnId: "in-progress" }];`; + expect(rules(src)).toEqual([]); + }); +}); + +describe("check-capacity-pool-id ratchet — it fails CLOSED", () => { + /* + FNXC:WorkflowCapacity 2026-07-28-23:30 (PR #2488 review): + These were ONE test titled "unparseable" that actually simulated a read() + throw and asserted "unreadable" — it never reached the parse path at all. A + test that misreports its own subject is this PR's entire failure mode in + miniature, so they are split and each now exercises the path it names. + + Writing the second one surfaced a real defect: `ts.createSourceFile` is + error-TOLERANT and does not throw on malformed syntax, so the try/catch it was + meant to cover was unreachable and the "unparseable" rule could never fire. + Detection now reads `parseDiagnostics`. + */ + it("reports an UNREADABLE file rather than skipping it", async () => { + const { findViolations } = await import("../../../../scripts/lib/capacity-pool-id-check.mjs"); + const out = (findViolations([ + { + file: "packages/core/src/broken.ts", + read: () => { + throw new Error("EACCES"); + }, + }, + ]) as Array<{ rule: string }>).map((v) => v.rule); + // "could not inspect" must never render as "inspected and clean". + expect(out).toContain("unreadable"); + }); + + it("reports an UNPARSEABLE file — real malformed syntax, reaching the parse check", () => { + /* Genuinely malformed TypeScript. A partial AST can silently lack the `??` + nodes and sink calls the rules look for, so "parsed badly" must not read + as "inspected and clean". */ + const malformed = ` + const x = { unclosed: ((( ; + function )( { + `; + const out = (findViolationsInSource("packages/core/src/malformed.ts", malformed) as Array<{ rule: string }>).map( + (v) => v.rule, + ); + expect(out).toContain("unparseable"); + }); + + it("does NOT report well-formed source as unparseable", () => { + /* The negative half: a diagnostics-based check that fired on valid syntax + would be noise, and noise gets suppressed. */ + const fine = `const poolId = resolveCapacityPoolId(selection?.workflowId);`; + expect(rules(fine)).toEqual([]); + }); +}); diff --git a/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts b/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts index a2129ceaf3..228eb95495 100644 --- a/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts +++ b/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts @@ -686,6 +686,59 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { expect(r.acted).toBe(false); }); }); + /* + FNXC:WorkflowCapacity 2026-07-28-19:20 (pool-id sentinel fix — E2E acceptance): + + The gate must BIND, and it must bind IN BOTH DIRECTIONS. A test that only asserts "held at cap" + passes trivially if the mover simply never admits anything — including if a future change breaks + admission outright — so each case is run twice against the SAME fixture with only the cap + changed: at limit 1 the card is HELD, at limit 2 the identical card is ADMITTED. + + NO WORKFLOW SELECTION, deliberately. That is the whole defect: `moves.ts` asked the counter for + pool `"builtin:coding"` while the counter buckets selection-less rows under + `DEFAULT_WORKFLOW_POOL_ID`, so nothing was ever counted. A card WITH a selection makes both + sentinels agree and the gate already bound before the fix — seeding one here would have produced a + green test that never touched the bug. (Verified: with the fix reverted, this row fails.) + + FLAG-ON, and labelled as such. The capacity block sits inside `if (useWorkflow && …)` and + `useWorkflow` reads a settings key nothing in production sets (Phase A3 R2, still live). So this + row proves the SENTINEL is fixed on the only path where the gate can execute at all; it does NOT + prove production behavior changed. Read the R2 note in workflow-capacity-invariant.pg.test.ts + before treating this as "capacity is enforced". + */ + describe.each([ + { limit: 1, expected: "held" as const }, + { limit: 2, expected: "admitted" as const }, + ])("in-transaction capacity gate (maxConcurrent=$limit → $expected)", ({ limit, expected }) => { + it(`a selection-less card at the wip boundary is ${expected}`, async () => { + const store = h.store(); + await store.updateSettings({ maxConcurrent: limit } as never); + // The capacity block's enclosing flag is GLOBAL-scoped; a project-scoped write is silently + // dropped and the test then measures the unflagged path while claiming otherwise. + await store.updateGlobalSettings({ experimentalFeatures: { workflowColumns: true } } as never); + + const holder = await store.createTask({ description: "capacity holder" }); + await store.moveTask(holder.id, "todo"); + await store.moveTask(holder.id, "in-progress"); + + const contender = await store.createTask({ description: "capacity contender" }); + await store.moveTask(contender.id, "todo"); + const error = await store + .moveTask(contender.id, "in-progress") + .then(() => null, (e: unknown) => e as Error); + + store.taskCache.delete(contender.id); + const persisted = (await store.getTask(contender.id)).column; + + if (expected === "held") { + expect((error as unknown as { rejection?: { code?: string } })?.rejection?.code).toBe("capacity-exhausted"); + expect(persisted).toBe("todo"); // observed state: refused, stays put + } else { + expect(error).toBeNull(); + expect(persisted).toBe("in-progress"); // the same fixture admits once the cap allows it + } + }); + }); }); /* diff --git a/packages/engine/src/hold-release.ts b/packages/engine/src/hold-release.ts index f000e89593..876e4fb49a 100644 --- a/packages/engine/src/hold-release.ts +++ b/packages/engine/src/hold-release.ts @@ -43,7 +43,7 @@ import { resolveColumnAdjacency, PLAN_REVIEW_GROUP_ID, ACTIVE_WORKFLOW_WORK_ITEM_STATES, - DEFAULT_WORKFLOW_POOL_ID, + resolveCapacityPoolId, TransitionRejectionError, resolveWorkflowIrForTask, isUnplannedSeedPrompt, @@ -113,9 +113,11 @@ export interface HoldReleaseResult { async function effectiveWorkflowId(store: TaskStore, taskId: string): Promise { try { - return (await store.getTaskWorkflowSelectionAsync(taskId))?.workflowId ?? DEFAULT_WORKFLOW_POOL_ID; + return resolveCapacityPoolId((await store.getTaskWorkflowSelectionAsync(taskId))?.workflowId); } catch { - return DEFAULT_WORKFLOW_POOL_ID; + /* An unreadable selection is indistinguishable from no selection for pooling + purposes, so it takes the same bucket rather than a second convention. */ + return resolveCapacityPoolId(undefined); } } @@ -439,7 +441,7 @@ function countCapacitySlot( ): number { let count = 0; for (const t of allTasks) { - if ((effectiveWorkflowIdByTask.get(t.id) ?? DEFAULT_WORKFLOW_POOL_ID) !== workflowId) continue; + if (resolveCapacityPoolId(effectiveWorkflowIdByTask.get(t.id)) !== workflowId) continue; if (budgetColumns.has(t.column)) { count += 1; continue; @@ -573,7 +575,7 @@ export async function runHoldReleaseSweep( } const capacity = resolveColumnCapacity(ir, target, settings); if (capacity.hasCapacity && Number.isFinite(capacity.limit)) { - const workflowId = effectiveWorkflowIdByTask.get(task.id) ?? DEFAULT_WORKFLOW_POOL_ID; + const workflowId = resolveCapacityPoolId(effectiveWorkflowIdByTask.get(task.id)); // U4/KTD-9: count occupants across every column sharing the target's // budget (a shared `limitSetting` pools multiple wip columns). const budgetColumns = new Set(resolveWipBudgetColumns(ir, target)); diff --git a/scripts/check-capacity-pool-id.mjs b/scripts/check-capacity-pool-id.mjs new file mode 100644 index 0000000000..aadc03ebba --- /dev/null +++ b/scripts/check-capacity-pool-id.mjs @@ -0,0 +1,68 @@ +#!/usr/bin/env node +/* +FNXC:WorkflowCapacity 2026-07-28-22:30 (PR #2488 review — ratchet rebuilt): +CLI wrapper. The rules and their rationale live in +scripts/lib/capacity-pool-id-check.mjs; the regression suite that pins each form +this guard must catch lives in +packages/engine/src/__tests__/capacity-pool-id-check.test.ts. + +Wired into `pretest`, `pretest:full`, and the blocking `test:gate`. +*/ +import { readFileSync } from "node:fs"; +import { execSync } from "node:child_process"; + +import { findViolations, RESOLVER } from "./lib/capacity-pool-id-check.mjs"; + +let files; +try { + files = execSync("git ls-files 'packages/*/src/**/*.ts' 'packages/*/src/*.ts'", { + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + }) + .split("\n") + .map((f) => f.trim()) + .filter(Boolean) + // Test sources are excluded using the repo's own guideline shape — the + // `{test,spec}.{ts,tsx}` family — because the earlier `.test.ts`-only suffix + // would have scanned a `.spec.ts` sitting directly under `packages//src/` + // as production source and flagged its fixtures as real violations. + .filter((f) => !f.includes("__tests__") && !/\.(test|spec)\.tsx?$/.test(f)); +} catch (err) { + // FAIL CLOSED: if we cannot even enumerate the files, we have checked nothing. + console.error(`check-capacity-pool-id: could not list files — ${err?.message ?? err}`); + process.exit(1); +} + +if (files.length === 0) { + console.error("check-capacity-pool-id: file list is EMPTY — refusing to report success on zero files."); + process.exit(1); +} + +const violations = findViolations(files.map((file) => ({ file, read: () => readFileSync(file, "utf8") }))); + +if (violations.length > 0) { + const byRule = { + "unresolved-pool-into-capacity-sink": + `a value reaching a capacity counter's \`workflowId\` must come from ${RESOLVER}().\n` + + " Two enforcement surfaces that each derive the pool themselves WILL drift: that is how the\n" + + ' in-transaction gate silently stopped binding (moves.ts derived `?? "builtin:coding"` while the\n' + + " counter bucketed under the sentinel, so nothing was ever counted).", + "sentinel-fallback": + "the pool sentinel must not be restated in a `??` fallback — call the resolver instead.", + unreadable: "a tracked source file could not be read, so it was NOT inspected.", + unparseable: "a tracked source file could not be parsed, so it was NOT inspected.", + }; + + console.error("\ncheck-capacity-pool-id: FAILED\n"); + for (const rule of Object.keys(byRule)) { + const hits = violations.filter((v) => v.rule === rule); + if (hits.length === 0) continue; + console.error(`${rule}: ${byRule[rule]}\n`); + for (const v of hits) console.error(` ${v.file}:${v.line}: ${v.text}`); + console.error(""); + } + console.error(`Fix by deriving the pool through ${RESOLVER}(selection?.workflowId).\n`); + process.exit(1); +} + +console.log(`check-capacity-pool-id: ok (${files.length} files inspected)`); diff --git a/scripts/lib/capacity-pool-id-check.mjs b/scripts/lib/capacity-pool-id-check.mjs new file mode 100644 index 0000000000..3b045ea35a --- /dev/null +++ b/scripts/lib/capacity-pool-id-check.mjs @@ -0,0 +1,235 @@ +/* +FNXC:WorkflowCapacity 2026-07-28-22:30 (PR #2488 review — ratchet rebuilt): + +WHY THIS IS AN AST CHECK AND NOT A REGEX. + +The first version of this guard matched `?? DEFAULT_WORKFLOW_POOL_ID` on one line. +It therefore MISSED `?? "builtin:coding"` — the actual defect the whole change +exists to fix — and passed green on the reintroduced bug (verified, not assumed). +A guard that reports success without checking is worse than no guard, because it +stops anyone looking. The claim being made is structural ("no capacity pool id is +derived except through the resolver"), so it is checked structurally. + +TWO COMPLEMENTARY RULES. Neither alone is sufficient: + + RULE 1 — THE SINK RULE (the one that catches the original defect). + Every argument that flows into a capacity counter's `workflowId` must be + `resolveCapacityPoolId(...)`, or a local whose initializer is that call. The + original defect passed `effectiveWorkflowIdForMove`, a local initialized from a + `??` literal — so this rule fires on it regardless of WHICH literal was used, on + one line or twenty. This is the rule that expresses the real invariant: the + banned thing is not a spelling, it is an underived value reaching the counter. + + RULE 2 — THE SENTINEL RULE (cheap, catches restatements of the convention). + No `??` whose right-hand side is the pool sentinel — under ANY qualification + depth (`X.Y.Z.DEFAULT_WORKFLOW_POOL_ID`) or as its raw string value + ("__default-workflow__") — outside the module that owns the convention. Being + AST-based, a fallback split across lines is the same node and is caught. + +WHY `?? "builtin:coding"` IS NOT BANNED OUTRIGHT. That literal is the legitimate +default for a WORKFLOW id in ~8 places (scheduler's IR-resolution key, +task-creation, analytics). It is only a bug when it reaches a CAPACITY POOL, and +that distinction is exactly what Rule 1 encodes. Banning the spelling everywhere +would be cargo-culting the rule past the thing it protects, and would have to be +suppressed so often it would rot. + +FAIL CLOSED. An unreadable or unparseable file is reported as a violation, never +skipped — "could not inspect" must not render as "inspected and clean", which is +the same failure shape as the regex that could not see the defect. +*/ +import ts from "typescript"; + +/** The module that owns the convention; the resolver's own `??` lives here. */ +export const CONVENTION_OWNER = "packages/core/src/workflow-capacity.ts"; + +/** The canonical resolver every pool-id derivation must go through. */ +export const RESOLVER = "resolveCapacityPoolId"; + +/** Capacity counters whose `workflowId` input is a pool id. */ +export const CAPACITY_SINKS = new Set([ + "countActiveInCapacitySlotAsync", + "countActiveInCapacitySlotSync", + "countActiveInCapacitySlot", + "countCapacitySlot", +]); + +/** `countCapacitySlot(allTasks, byTask, budgetColumns, workflowId, countPending)` */ +const POSITIONAL_SINKS = { countCapacitySlot: 3 }; + +const SENTINEL_CONST = "DEFAULT_WORKFLOW_POOL_ID"; +const SENTINEL_VALUE = "__default-workflow__"; + +/** Right-most name of a possibly-qualified reference, at any depth. */ +function tailName(node) { + let cur = node; + while (ts.isPropertyAccessExpression(cur)) cur = cur.name; + return ts.isIdentifier(cur) ? cur.text : undefined; +} + +function isResolverCall(node) { + return ( + node && + ts.isCallExpression(node) && + tailName(node.expression) === RESOLVER + ); +} + +/** True when `expr` is the resolver call, or a local initialized from one. */ +function isDerivedThroughResolver(expr, resolverLocals) { + if (!expr) return false; + if (isResolverCall(expr)) return true; + if (ts.isIdentifier(expr)) return resolverLocals.has(expr.text); + // `cond ? resolver(a) : resolver(b)` is still derived through the resolver. + if (ts.isConditionalExpression(expr)) { + return ( + isDerivedThroughResolver(expr.whenTrue, resolverLocals) && + isDerivedThroughResolver(expr.whenFalse, resolverLocals) + ); + } + if (ts.isAsExpression(expr) || ts.isParenthesizedExpression(expr)) { + return isDerivedThroughResolver(expr.expression, resolverLocals); + } + return false; +} + +/** + * Analyse one source file. + * @returns {Array<{file:string,line:number,rule:string,text:string}>} + */ +export function findViolationsInSource(file, text) { + const violations = []; + /* + FNXC:WorkflowCapacity 2026-07-28-23:30 (PR #2488 review): + Parse failure is detected via `parseDiagnostics`, NOT via a try/catch. + `ts.createSourceFile` is error-TOLERANT: given `function )( {` it returns a + source file carrying diagnostics rather than throwing, so the catch this + replaced was unreachable and the "unparseable" rule could never fire. That made + the guard's own fail-closed claim overstated in exactly the way this PR is + about — a check advertising a capability it did not have. A file whose syntax + did not parse yields a partial AST, so its `??` nodes and sink calls may simply + be absent: reporting it clean would be reporting "not inspected" as "inspected". + The defensive catch is kept for a genuine internal error, but detection is the + diagnostics check. + */ + let sf; + try { + sf = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true, ts.ScriptKind.TS); + } catch (err) { + return [{ file, line: 0, rule: "unparseable", text: `parser threw: ${String(err && err.message)}` }]; + } + const parseErrors = sf.parseDiagnostics ?? []; + if (parseErrors.length > 0) { + const first = ts.flattenDiagnosticMessageText(parseErrors[0].messageText, " "); + return [ + { + file, + line: 0, + rule: "unparseable", + text: `${parseErrors.length} syntax error(s), first: ${first} — a file that did not parse was NOT inspected`, + }, + ]; + } + + const lineOf = (node) => sf.getLineAndCharacterOfPosition(node.getStart(sf)).line + 1; + const snippet = (node) => node.getText(sf).replace(/\s+/g, " ").slice(0, 140); + + // Locals whose initializer is a resolver call — collected first so a sink that + // reads one is accepted regardless of declaration order within the file. + const resolverLocals = new Set(); + const collect = (node) => { + if (ts.isVariableDeclaration(node) && ts.isIdentifier(node.name)) { + if (isDerivedThroughResolver(node.initializer, resolverLocals)) resolverLocals.add(node.name.text); + } + ts.forEachChild(node, collect); + }; + collect(sf); + // Second pass: a local initialized from another resolver local. + collect(sf); + + const visit = (node) => { + // ── RULE 2: sentinel restated in a `??` fallback ────────────────────────── + if ( + ts.isBinaryExpression(node) && + node.operatorToken.kind === ts.SyntaxKind.QuestionQuestionToken && + file !== CONVENTION_OWNER + ) { + const rhs = node.right; + const isSentinelConst = tailName(rhs) === SENTINEL_CONST; + const isSentinelValue = ts.isStringLiteral(rhs) && rhs.text === SENTINEL_VALUE; + if (isSentinelConst || isSentinelValue) { + violations.push({ + file, + line: lineOf(node), + rule: "sentinel-fallback", + text: snippet(node), + }); + } + } + + // ── RULE 1: something underived reaching a capacity counter ─────────────── + if (ts.isCallExpression(node)) { + const callee = tailName(node.expression); + if (callee && CAPACITY_SINKS.has(callee)) { + // Object-literal form: `count...({ workflowId: })` + for (const arg of node.arguments) { + if (!ts.isObjectLiteralExpression(arg)) continue; + for (const prop of arg.properties) { + if (!ts.isPropertyAssignment(prop)) continue; + if (tailName(prop.name) !== "workflowId") continue; + if (!isDerivedThroughResolver(prop.initializer, resolverLocals)) { + violations.push({ + file, + line: lineOf(prop), + rule: "unresolved-pool-into-capacity-sink", + text: snippet(prop), + }); + } + } + } + // Positional form. + const idx = POSITIONAL_SINKS[callee]; + if (idx !== undefined && node.arguments.length > idx) { + const arg = node.arguments[idx]; + if (!isDerivedThroughResolver(arg, resolverLocals)) { + violations.push({ + file, + line: lineOf(arg), + rule: "unresolved-pool-into-capacity-sink", + text: snippet(arg), + }); + } + } + } + } + + ts.forEachChild(node, visit); + }; + visit(sf); + + return violations; +} + +/** + * @param {Array<{file:string, read:() => string}>} entries + * @returns {Array<{file:string,line:number,rule:string,text:string}>} + */ +export function findViolations(entries) { + const out = []; + for (const entry of entries) { + let text; + try { + text = entry.read(); + } catch (err) { + // FAIL CLOSED: an uninspectable file is a violation, not a pass. + out.push({ + file: entry.file, + line: 0, + rule: "unreadable", + text: `could not read (${String(err && err.message)}) — a file that cannot be inspected must not report as clean`, + }); + continue; + } + out.push(...findViolationsInSource(entry.file, text)); + } + return out; +}