Files
fusion/scripts/lib/capacity-pool-id-check.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

236 lines
9.1 KiB
JavaScript

/*
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: <expr> })`
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;
}