Files
fusion/scripts/check-capacity-pool-id.mjs
gsxdsm 7871b28766 fix(core): bind the in-transaction capacity gate — one shared pool-id convention (NOT user-visible yet — see R2) (#2488)
## The bug

`moves.ts` asked `countActiveInCapacitySlotAsync` for occupants of pool
`"builtin:coding"`, while the counter buckets selection-less rows under
`DEFAULT_WORKFLOW_POOL_ID` (`"__default-workflow__"`). Nothing ever
landed in the pool being asked about, so the count came back **0** and a
finite limit could never bind.

## Root fix, not a literal swap

A shared *constant* would not have prevented this:
**`DEFAULT_WORKFLOW_ID` was already imported in `moves.ts` and the code
still wrote a literal.** So both sides now call a shared **function**,
`resolveCapacityPoolId` — "which pool does a selection-less task belong
to" has exactly one answer and no call site is in a position to disagree
with it.

The one variable serving two masters is split: a capacity **pool key**
(a bucketing sentinel that must not collide with a workflow id) and a
**workflow id** (telemetry, must stay a real id). The emitted
`TaskTransitioned` payload is byte-identical.

## Checked, not assumed: no second copy

`scheduler.ts:2514` and `:2536` do carry `?? "builtin:coding"` — but as
an **IR resolution key** (`resolveWorkflowIrById`), where a real
workflow id is required and the pool sentinel would not resolve at all.
Same literal, different concept, correctly used. A blanket replace would
have broken it.

## Something did depend on the gate being dead — exactly one thing

`move-path-equivalence.pg.test.ts` → *"UNPROVEN: in-transaction column
capacity did NOT reject on EITHER path in this fixture"*. It left the
cause open —

> 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 … This
suite does not establish which.

— and predicted its own obsolescence (*"if a future change makes this
reject, that is the capacity gate coming alive"*). **Neither guess was
right; it was the pool id.** Updated to assert the divergence with the
answer recorded — **not weakened**. Its fixture also had to start each
phase from an empty wip column: once the gate binds, the inline phase's
leftovers trip the cap on the *holder* move before the contended move
under test runs.

`schema-applier.test.ts` failed only in the full-suite run and passes in
isolation both with and without the fix — cross-file contamination, not
mine.

## Before / after — measured, both directions

`maxConcurrent: 1`, real PG store, real `moveTask`:

| | flagOFF / no selection | flagOFF / selection | flagON / no selection
| flagON / selection |
|---|---|---|---|---|
| **before** | ADMITTED | ADMITTED | **ADMITTED** ← the bug | REJECTED |
| **after** | ADMITTED | ADMITTED | **REJECTED** | REJECTED |

The E2E acceptance row asserts **held at cap 1 and admitted at cap 2 on
the same fixture**, so it cannot pass by simply never admitting
anything. **With the fix reverted that row fails**; the `admitted` case
still passes, as it should. The Phase A3 ratchet's two flipped
assertions also fail with the fix reverted.

Ratchet flipped exactly as its author specified: `DEFECT (R1)` becomes a
rejection, and `it.fails` on the invariant becomes a plain `it`.

## ⚠️ This is NOT user-visible yet — please read before merging

The premise this was approved on ("once it binds, cards that currently
slip through will start being held") **does not hold for this change
alone.** The whole capacity block sits inside `if (useWorkflow &&
workflowIr && fromColumn !== toColumn)`, and `useWorkflow` is
`experimentalFeatures.workflowColumns === true` — absent from
`DEFAULT_GLOBAL_SETTINGS`, with **no writer anywhere outside tests**.
That is Phase A3's R2, still live and now retitled `DEFECT (R2, STILL
LIVE)` with the measured matrix recorded in it.

So on merge: nothing changes for any real project. Making it actually
bind means **also** removing the `useWorkflow` condition — a materially
larger, genuinely user-visible change that I have not made unilaterally.
Escalated for a decision; if that lands, the changeset here should be
re-categorised.


## Review follow-up (48e79ffd9): the convention was still duplicated —
swept and ratcheted

The first pass added the resolver and routed the transactional gate +
counters, but **hold-release still derived the pool independently**.
Swept the repo: six sites name the sentinel, **five derive the
convention** and now call `resolveCapacityPoolId`
(`hold-release.ts:116/118/442/576`, `task-store-helpers.ts:290`). The
sixth, `scheduler.ts:1558`, names the default pool as a literal in a
capacity *diagnostic* — no selection input, nothing to disagree with —
so it keeps the constant.

**Does this change hold-release behavior? No, and it was never releasing
against the wrong pool.** hold-release computed `x ??
DEFAULT_WORKFLOW_POOL_ID`, which is exactly what the counter buckets
under; `moves.ts` (`?? "builtin:coding"`) was the sole disagreeing site,
and the first commit moved *it* into agreement with hold-release, not
the reverse. `resolveCapacityPoolId(x)` **is** `x ??
DEFAULT_WORKFLOW_POOL_ID`, so every routed site computes an identical
value for every input. **No second user-visible change rides along with
this PR** — the only behavior delta remains the gate binding on the
flag-ON path, which per R2 is still not the path production takes.
Evidence: hold-release + capacity suites **43/43 identical before and
after**.

**The resolver is now the only way to compute a pool id, not merely the
newest way.** `scripts/check-capacity-pool-id.mjs` fails on any inline
`?? DEFAULT_WORKFLOW_POOL_ID` outside `workflow-capacity.ts`, wired into
**both `pretest` and the blocking `test:gate`**. A review note would not
have sufficed: the original defect landed in a file that *already
imported* the canonical constant. Verified both ways — clean run scans
1124 files and passes; reintroducing the old hold-release expression
exits 1 and names the line.


## Review follow-up (a5b675503): the ratchet was rebuilt because it
would not have caught the bug

The first ratchet matched one spelling (`?? DEFAULT_WORKFLOW_POOL_ID`)
and the real defect used another (`?? "builtin:coding"`). **Verified:
reintroducing the original defect and running the old checker exits 0.**
A guard that reports success without checking is worse than no guard —
it stops anyone looking.

Rebuilt on the TypeScript AST with two rules. **Rule 1 (sink):** a value
reaching a capacity counter's `workflowId` must come from
`resolveCapacityPoolId`, or a local initialized from it — so it fires on
the original defect regardless of which literal was used, on one line or
twenty. **Rule 2 (sentinel):** no `??` onto the sentinel at any
qualification depth or as its raw value; multiline is one AST node and
caught by construction. `?? "builtin:coding"` is deliberately *not*
banned outright — it is the legitimate default for a *workflow* id in ~8
places, and is only a bug when it reaches a capacity pool.

**Fails closed three ways** that previously reported success without
inspecting: unreadable file, unparseable file, and an empty file listing
(the old script would have printed a green tick off a broken glob).

**Acceptance was not "passes on main".** Each form was reintroduced into
the real source and confirmed to fail: the original defect in
`moves.ts`, a multiline fallback, and a deeply qualified sentinel. All
are pinned in `capacity-pool-id-check.test.ts` (12 cases: 7 must-catch
starting with the reduced actual pre-fix `moves.ts`, 4 must-not-flag, 1
fail-closed) so the guard cannot silently narrow again.

Also added to `pretest:full`, which had omitted it.


### Follow-up (0be8df6ea): a dead rule found by fixing a test title

Splitting the mislabelled fail-closed test surfaced more than a
mislabel: **`ts.createSourceFile` is error-tolerant and does not throw
on malformed syntax**, so the `try/catch` behind the `unparseable` rule
was unreachable and that rule could never fire. The earlier "fails
closed three ways" claim was overstated — the guard advertised a
capability it did not have. Detection now reads `sf.parseDiagnostics`; a
partial AST can silently lack the `??` nodes and sink calls the rules
look for, so "did not parse" must not read as "inspected and clean".
Mutation-verified: reverting the detection fails that case and only that
case.

Test-file exclusion also moved to the repo's `{test,spec}.{ts,tsx}`
guideline shape — a `.spec.ts` under `packages/<pkg>/src/` was being
scanned as production source. Verified both ways: the `.spec.ts` is
skipped, and the identical content in a non-test file is still caught,
so the exclusion is scoped rather than a hole.

## Verification

- engine + core `tsc --noEmit` clean
- `pnpm test:gate` green (299 + 10 + 71)
- E2E 20/20; capacity + move-path suites 14/14
- full core PG: **1037 passed / 3 failed** — all three reproduce with
the fix stashed (pre-existing)
- engine-default: **279 failed** vs **280 at baseline** with the fix
stashed — pre-existing red lane, no regression
- hold-release + capacity suites: **43/43 identical before and after**
the resolver routing
- `check-capacity-pool-id` ratchet: 14/14 regression cases; clean over
1124 files; exits 1 on the original defect, a multiline fallback, and a
deeply qualified sentinel reintroduced into real source

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit

* **Bug Fixes**
* Fixed capacity-limit accounting when workflow selection is missing by
consistently deriving the correct capacity pool id.
* Made capacity enforcement align across move and hold/release paths,
rejecting over-limit moves with `capacity-exhausted`.
* **Tests**
* Updated PostgreSQL and added an E2E scenario to verify the corrected
in-transaction gating behavior at `maxConcurrent` limits of 1 and 2.
* **Chores**
* Added an automated guard to detect inconsistent capacity pool id
fallback patterns in code.
* **Public API**
* Exposed `resolveCapacityPoolId` for consistent capacity pool id
derivation.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-27 21:09:51 -07:00

69 lines
2.9 KiB
JavaScript

#!/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/<pkg>/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)`);