## 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>
236 lines
9.1 KiB
JavaScript
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;
|
|
}
|