## 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>
69 lines
2.9 KiB
JavaScript
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)`);
|