fix(core): bind the in-transaction capacity gate — one shared pool-id convention (NOT user-visible yet — see R2) - #2488
Merged
Conversation
… convention
`moves.ts` asked the counter for occupants of pool "builtin:coding" while
`countActiveInCapacitySlotAsync` buckets selection-less rows under
DEFAULT_WORKFLOW_POOL_ID. 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. Both sides now call `resolveCapacityPoolId`, so "which pool does a
selection-less task belong to" has one answer and no call site is in a position
to disagree. The one variable that served two masters is split: a capacity POOL
key (bucketing sentinel) and a WORKFLOW id (telemetry, must stay a real id). The
emitted event payload is unchanged.
CHECKED, NOT ASSUMED — no other site carries the same bug. `scheduler.ts:2514`
and `:2536` do use `?? "builtin:coding"`, but as an IR RESOLUTION key
(resolveWorkflowIrById), where a real workflow id is required and the pool
sentinel would not resolve. Same literal, different concept, correctly used.
ONE TEST DEPENDED ON THE GATE BEING DEAD: move-path-equivalence's "UNPROVEN:
in-transaction column capacity did NOT reject on EITHER path". It left the cause
open ("`resolveColumnCapacity`'s limit resolution, or what the counter counts as
an occupant — this suite does not establish which") and predicted its own
obsolescence. Neither guess was right; it was the pool id. Updated to assert the
divergence with the answer recorded — not weakened.
Also flips the Phase A3 ratchet as its author specified: the R1 DEFECT
expectation becomes a rejection and `it.fails` on the invariant becomes a plain
`it`. R2 stays and is retitled STILL LIVE.
MEASURED, both directions (maxConcurrent=1, real PG, real moveTask):
before flagOFF/no-sel ADMIT flagOFF/sel ADMIT flagON/no-sel ADMIT flagON/sel REJECT
after flagOFF/no-sel ADMIT flagOFF/sel ADMIT flagON/no-sel REJECT flagON/sel REJECT
E2E acceptance row asserts held-at-cap AND admitted-at-cap+1 on the same fixture,
so it cannot pass by never admitting. With the fix reverted that row fails; the
admitted case still passes, as it should.
NOT USER-VISIBLE YET: the capacity block is still inside `if (useWorkflow && …)`
and `useWorkflow` reads `experimentalFeatures.workflowColumns`, absent from
defaults with no production writer. Escalated separately.
Verified: engine + core tsc clean; test:gate green (299+10+71); E2E 20/20; full
core PG 1037 passed / 3 failed, all three reproducing with the fix stashed;
engine-default 279 failed vs 280 at baseline (pre-existing red lane, no
regression).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThe change centralizes capacity-pool ID resolution, updates move, counting, helper, and hold-release logic to use the sentinel for selection-less tasks, and adds regression, lifecycle, and source-validation coverage. ChangesCapacity pool sentinel correction
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThe PR centralizes capacity-pool ID derivation and restores enforcement for selection-less tasks on the workflow-enabled path.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously reported duplicated capacity-pool derivation has been routed through the canonical resolver across the relevant hold-release and store paths.
|
| Filename | Overview |
|---|---|
| packages/core/src/workflow-capacity.ts | Adds the canonical capacity-pool resolver while preserving the distinct default workflow identity. |
| packages/core/src/task-store/moves.ts | Uses the canonical pool key for transactional occupancy checks while retaining a real workflow ID for telemetry. |
| packages/engine/src/hold-release.ts | Routes selection, fallback, counting, and release pool derivations through the shared resolver. |
| scripts/lib/capacity-pool-id-check.mjs | Adds AST checks for underived capacity-counter inputs and duplicated sentinel fallback expressions. |
| packages/engine/src/tests/capacity-pool-id-check.test.ts | Covers the original regression, alternate fallback syntax, valid workflow defaults, and fail-closed parsing behavior. |
| packages/core/src/tests/postgres/workflow-capacity-invariant.pg.test.ts | Updates the capacity invariant to verify selection-less tasks are rejected on the workflow-enabled path. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
Selection[Task workflow selection] --> Resolver[resolveCapacityPoolId]
Resolver --> MoveGate[Transactional move capacity gate]
Resolver --> Counter[Active capacity counter]
Resolver --> HoldRelease[Hold-release sweep]
MoveGate --> Decision{Pool below capacity?}
Counter --> Decision
Decision -->|Yes| Admit[Admit transition]
Decision -->|No| Reject[Reject with capacity-exhausted]
Reviews (4): Last reviewed commit: "fix(scripts): make the `unparseable` rul..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts (1)
374-397: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCondense the FNXC test narratives to the current behavioral contract.
Keep only the observed capacity behavior and reason needed to maintain each assertion; remove historical investigation and operator-planning prose.
packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts#L374-L397: reduce this to the hooks rejection / inline ungated contract.packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts#L115-L132: reduce this to the production-inline admission contract.As per coding guidelines, “Add dated
FNXC:Area-of-productcomments …; keep them concise and current.” Based on learnings, test-source FNXC context should remain concise and behavioral.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts` around lines 374 - 397, Condense the FNXC comments at packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts lines 374-397 and packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts lines 115-132 to concise, current behavioral context: document that the hooks path rejects at capacity while the inline path remains ungated, and separately state the production-inline admission contract. Remove historical investigation, root-cause narrative, and operator-planning discussion while preserving the dated FNXC format.Sources: Coding guidelines, Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts`:
- Around line 374-397: Condense the FNXC comments at
packages/core/src/__tests__/postgres/move-path-equivalence.pg.test.ts lines
374-397 and
packages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.ts
lines 115-132 to concise, current behavioral context: document that the hooks
path rejects at capacity while the inline path remains ungated, and separately
state the production-inline admission contract. Remove historical investigation,
root-cause narrative, and operator-planning discussion while preserving the
dated FNXC format.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 53e0be30-c446-48ce-b50f-0aca0010dabd
📒 Files selected for processing (7)
.changeset/capacity-pool-sentinel.mdpackages/core/src/__tests__/postgres/move-path-equivalence.pg.test.tspackages/core/src/__tests__/postgres/workflow-capacity-invariant.pg.test.tspackages/core/src/task-store/moves.tspackages/core/src/task-store/project-store-ops.tspackages/core/src/workflow-capacity.tspackages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts
…solver, and ratchet it PR #2488 review: the resolver was added and the transactional gate + counters used it, but the HOLD-RELEASE capacity path still applied `workflowId ?? DEFAULT_WORKFLOW_POOL_ID` independently — so the convention was still duplicated across enforcement surfaces and could drift again at the other site. Correct, and the same failure mode the fix was meant to close. Swept the whole repo. SIX sites named the sentinel; FIVE derived the convention and now call `resolveCapacityPoolId`: - hold-release.ts:116 selection fallback - hold-release.ts:118 catch fallback (an unreadable selection is indistinguishable from none for pooling) - hold-release.ts:442 map-miss fallback in the occupant counter - hold-release.ts:576 map-miss fallback at the release decision - task-store-helpers.ts:290 resolveEffectiveWorkflowIdSyncImpl The sixth, scheduler.ts:1558, is NOT a derivation: it names the default pool as a literal field in a capacity DIAGNOSTIC, with no selection input and therefore no way to disagree with anything. Left as the constant. NO BEHAVIOR CHANGE, and specifically NOT a second user-visible change. hold-release was already computing `?? DEFAULT_WORKFLOW_POOL_ID` — the SAME value the counter buckets under — so it was never releasing against the wrong pool. The gate in moves.ts was the sole disagreeing site (`?? "builtin:coding"`), and the previous commit moved IT into agreement with hold-release, not the reverse. `resolveCapacityPoolId(x)` IS `x ?? DEFAULT_WORKFLOW_POOL_ID`, so every routed call site computes an identical value for every input. RATCHET so the resolver is 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 been enough — the original defect landed in a file that already imported the canonical constant, so availability of the right thing demonstrably does not prevent the wrong thing. Verified in both directions: passes clean (1124 files), and reintroducing the old hold-release expression makes it exit 1. Verified: hold-release + capacity suites 43/43 IDENTICAL before and after the routing; test:gate green (299+10+71) with the new check running in it; core + engine tsc clean; full core PG 1037 passed / 3 failed, the same three that reproduce with the fix stashed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Around line 17-18: Update the pretest:full script to include
check-capacity-pool-id.mjs in its chained checker commands, matching the
existing pretest and test:gate coverage while preserving the current command
order and behavior.
In `@scripts/check-capacity-pool-id.mjs`:
- Around line 27-28: Update the BANNED check in the capacity-pool validation
script to enforce resolver-only pool derivation rather than matching only
same-line DEFAULT_WORKFLOW_POOL_ID expressions. Use AST/source-aware analysis,
or equivalent detection, that rejects literal fallbacks such as
"builtin:coding", multiline nullish fallbacks, and arbitrarily deep qualified
DEFAULT_WORKFLOW_POOL_ID references; add regression coverage for each form.
- Around line 42-47: Update the read failure handling in the file-scanning loop
around readFileSync so an unreadable tracked file records a violation or causes
the check to exit nonzero. Remove the silent continue behavior, while preserving
normal processing for files that are read successfully.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 4f0cf9af-392c-4e55-b684-07665652a05a
📒 Files selected for processing (6)
package.jsonpackages/core/src/index.gate.tspackages/core/src/index.tspackages/core/src/task-store/task-store-helpers.tspackages/engine/src/hold-release.tsscripts/check-capacity-pool-id.mjs
… regex missed the defect it guards PR #2488 review, and the finding is the serious kind: the ratchet I added would have passed green on the exact bug it exists to prevent. Verified, not assumed — reintroducing the original `?? "builtin:coding"` into moves.ts and running the old checker exits 0. It matched one spelling of the fallback (`?? DEFAULT_WORKFLOW_POOL_ID`) and the real defect used another. A guard that reports success without checking is worse than no guard, because it stops anyone looking. Rebuilt on the TypeScript AST around two complementary rules: RULE 1 (SINK) — the one that catches the original defect. A value reaching a capacity counter's `workflowId` must come from `resolveCapacityPoolId`, or a local initialized from it. The defect passed a local derived from a `??` literal, so this fires regardless of WHICH literal, on one line or twenty. The banned thing is not a spelling; it is an underived value reaching the counter. RULE 2 (SENTINEL) — no `??` onto the pool sentinel, at any qualification depth (`A.B.C.DEFAULT_WORKFLOW_POOL_ID`) or as its raw value `"__default-workflow__"`. Being AST-based, a fallback split across lines is one node and is caught. `?? "builtin:coding"` is deliberately NOT banned outright: it 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, which is what Rule 1 encodes. Banning the spelling would be suppressed until it rotted. FAILS CLOSED, three ways that previously reported success without inspecting: - an unreadable file is a violation, not a skip (was a silent `continue`); - an unparseable file is a violation; - an empty file listing is a violation rather than "0 files, all clean". Also wired into `pretest:full`, which omitted it — so the claim that both pretest chains run it is now true rather than merely stated. REGRESSION SUITE (capacity-pool-id-check.test.ts, 12 cases) pins every form the guard must catch — starting with the reduced ACTUAL pre-fix moves.ts — plus the negatives it must not flag, so the pattern cannot silently narrow again. The acceptance test is not "passes on main": each form was also reintroduced into the REAL source and confirmed to fail (original defect, multiline fallback, deeply qualified sentinel). Verified: 12/12 regression cases; checker clean over 1124 files and exit 1 on each reintroduced form; test:gate green (299+10+71) with the check inside it; core + engine tsc clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/__tests__/capacity-pool-id-check.test.ts`:
- Around line 130-144: Correct the test description in the
“check-capacity-pool-id ratchet — it fails CLOSED” suite to describe the
simulated read failure as unreadable, matching the existing “unreadable”
assertion. Add a separate regression test that supplies malformed TypeScript to
findViolations and verifies the unparseable rule from the ts.createSourceFile
parsing failure.
In `@scripts/check-capacity-pool-id.mjs`:
- Around line 18-25: Update the file discovery command in the capacity-pool scan
to include both .ts and .tsx sources, and update its exclusion filter to omit
recognized .test.ts, .test.tsx, .spec.ts, and .spec.tsx files in addition to
__tests__ paths. Keep scanning the existing package source directories and
preserve the non-empty path filtering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 093638a0-8ab0-4954-b3c8-35a7017949f4
📒 Files selected for processing (4)
package.jsonpackages/engine/src/__tests__/capacity-pool-id-check.test.tsscripts/check-capacity-pool-id.mjsscripts/lib/capacity-pool-id-check.mjs
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
…lude .spec.ts too PR #2488 review, two threads. The first one turned out to be more than a test-title fix. 1) THE MISLABELLED TEST WAS HIDING A DEAD RULE. The case titled "unparseable" simulated a read() throw and asserted "unreadable" — it never reached the parse path. Splitting it as asked surfaced why that mattered: `ts.createSourceFile` is error-TOLERANT and does NOT throw on malformed syntax (verified: `function )( {` returns a source file with 4 parseDiagnostics), so the try/catch it was meant to cover was UNREACHABLE and the `unparseable` rule could never fire. My "fails closed three ways" claim was therefore overstated — the guard advertised a capability it did not have, which is this PR's own failure mode in miniature. Detection now reads `sf.parseDiagnostics`. A file that did not parse yields a partial AST whose `??` nodes and sink calls may simply be absent, so reporting it clean would report "not inspected" as "inspected". The suite now has three fail-closed cases, each exercising the path it names: unreadable, genuinely-malformed-source, and a negative that well-formed source is not flagged (a diagnostics check that fired on valid syntax would be noise, and noise gets suppressed). Mutation-verified: reverting the parseDiagnostics detection fails the new case and only that case. 2) TEST-FILE EXCLUSION now uses the repo's guideline shape (the `{test,spec}.{ts,tsx}` family) instead of a `.test.ts`-only suffix. A `.spec.ts` directly under `packages/<pkg>/src/` was being scanned as production source and would have flagged its fixtures as real violations. Verified BOTH directions: a `.spec.ts` carrying a violation is skipped, and the identical content in a non-test file is still caught — so the exclusion is scoped, not a hole. Verified: 14/14 regression cases; checker clean over 1124 files; test:gate green (299+10+71); core + engine tsc clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The bug
moves.tsaskedcountActiveInCapacitySlotAsyncfor occupants of pool"builtin:coding", while the counter buckets selection-less rows underDEFAULT_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_IDwas already imported inmoves.tsand 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
TaskTransitionedpayload is byte-identical.Checked, not assumed: no second copy
scheduler.ts:2514and:2536do 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 —— 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.tsfailed 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, realmoveTask: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
admittedcase 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, andit.failson the invariant becomes a plainit.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), anduseWorkflowisexperimentalFeatures.workflowColumns === true— absent fromDEFAULT_GLOBAL_SETTINGS, with no writer anywhere outside tests. That is Phase A3's R2, still live and now retitledDEFECT (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
useWorkflowcondition — 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 (48e79ff): 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)isx ?? 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.mjsfails on any inline?? DEFAULT_WORKFLOW_POOL_IDoutsideworkflow-capacity.ts, wired into bothpretestand the blockingtest: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 (a5b6755): 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
workflowIdmust come fromresolveCapacityPoolId, 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 incapacity-pool-id-check.test.ts(12 cases: 7 must-catch starting with the reduced actual pre-fixmoves.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 (0be8df6): a dead rule found by fixing a test title
Splitting the mislabelled fail-closed test surfaced more than a mislabel:
ts.createSourceFileis error-tolerant and does not throw on malformed syntax, so thetry/catchbehind theunparseablerule 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 readssf.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.tsunderpackages/<pkg>/src/was being scanned as production source. Verified both ways: the.spec.tsis skipped, and the identical content in a non-test file is still caught, so the exclusion is scoped rather than a hole.Verification
tsc --noEmitcleanpnpm test:gategreen (299 + 10 + 71)check-capacity-pool-idratchet: 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
Summary by CodeRabbit
capacity-exhausted.maxConcurrentlimits of 1 and 2.resolveCapacityPoolIdfor consistent capacity pool id derivation.