fix(development): restore phase-strict gating silently disabled by an option rename - #81
Conversation
…ubcommand error Two pre-existing failures on main. Both have been red since June and neither is related to any open PR. PHASE-STRICT GATING WAS SILENTLY DISABLED. Commit 7b73b7a renamed selectNextTasks' option from `prFormat` to `stacked` in phase-tracker, but the call site in complete-beads-planner.js still passed `prFormat`. That made `options.stacked === true` never true, so selectNextTasks took the unfiltered branch and the phase boundary stopped being enforced -- a later-phase task could jump ahead of an incomplete earlier phase, which is the precise bug the gate was written to prevent. Nothing surfaced it because the planner still returned a plausible-looking selection; it was just the wrong one. The planner keeps `prFormat` as its own public option name (it comes from the CLI's --pr-format). Only the call INTO phase-tracker moves to the new key. Mutation-verified: reverting that one word fails 2 tests. MISSING-SUBCOMMAND ERROR WAS INDISTINGUISHABLE FROM HELP. trd-cli emitted only a usage string when invoked with no subcommand, so a caller matching on the cause could not tell an invocation error from a help request. Now leads with "Missing subcommand." and keeps the usage after it. CI=true npm test: 0 failing suites, 722 passed in the affected workspace (was 719 passed / 3 failed). npm run generate is clean -- no artifact drift.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 17 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Comment |
ReviewVerified this against the source, not just the PR description — the claims hold up. Root-cause fix is correct. Tests actually cover the regression, not just assert current behavior — Naming is a little confusing but intentional and explained. The planner's public option is still CLI error fix ( Minor, non-blocking observation: Scope discipline: exactly the two one-line changes described, both with comments explaining the why (per this repo's comment convention — non-obvious constraint/history, not restating the diff). No unrelated cleanup bundled in. Overall: a well-targeted, well-verified fix for a nasty silent-failure bug, with tests that actually pin the regression down. LGTM. |
…er it
Review caught that restoring the gate would ship a regression, and it is the
same silent-failure class the original fix removes -- just relocated.
MISSING PHASE MAP BECAME A TOTAL STALL. complete-beads-cli turns an absent
phase file into `{}` (`phaseTaskIdsJson || {}`). With the gate dead that was
harmless. With it live, currentPhase() returns null and EVERY ready bead is
deferred as 'phase-gate' -- reported to the operator as "waiting on an earlier
phase", exit 0, nothing scheduled. Reproduced before the fix: empty map gave
selected=[] deferred=[t1,t2]; populated map gave selected=[t1].
Step 7's own call-site comment already said "if TRD phase metadata present" and
nothing enforced it. Now it does: no phase metadata means no boundaries to
enforce, so the ids pass through -- and it warns on stderr, because a TRD that
should have phases and silently lost its map is otherwise indistinguishable
from one that never had phases, and that difference decides whether the run is
correct.
UNPARSEABLE TITLES WERE MISLABELLED. extractTaskId falls back to the bead id,
which by construction never appears in phaseTaskIds, so any bead whose title
lacks its [trd:<slug>:task:<id>] marker can never be selected while the gate is
live. Discarding it is deliberate -- phase-tracker documents that an id with no
phase mapping cannot be proven to belong to the current phase -- but reporting
'phase-gate' sends the operator hunting an earlier phase when the real cause is
a malformed title. Now deferred as 'unparseable-task-id'.
Both guards are mutation-verified: removing either fails exactly one test.
CI=true npm test: 0 failing suites. npm run generate: clean.
ReviewVerified against the code (not just the PR description) — both fixes check out. Core fix is real and correctly scoped
The two bundled defensive fixes (not mentioned in the "Why" summary title, but present in the diff)
Both are scope creep relative to the stated "one word" fix, but they're small, directly adjacent to the code being touched, and each has a dedicated regression test with a clear comment explaining the failure mode it guards against. I'd call this acceptable, not something to push back on. Minor / non-blocking
Test coverageThe two new tests in Security / performanceNo concerns — this is pure selection logic with no I/O, network, or shell calls, consistent with the module's documented "NO side effects" contract. Overall: Well-diagnosed, well-tested fix for a real silent-failure bug. The two bundled hardening changes are justified by the same failure mode being reintroduced by re-arming the gate, and are tested. Nothing blocking. |
…closed
Pass 2 caught that my own guard failed open on the one condition it was written
to detect, and that it counted the wrong thing. Both confirmed by execution.
WRONG DIRECTION. trd-parser sets prFormat=true only when the TRD has `PR N:`
headings (`if (sawPR) { prFormat = true; ... }`), and a PR heading always yields
at least one phase. So prFormat with no phase task ids is NOT "a TRD without
phases" -- it can only mean the phase map failed to load, which is exactly how
complete-beads-cli's `phaseTaskIdsJson || {}` delivers a missing or unreadable
--phase-task-ids file. Passing the ids through with a warning silently
dispatched later-phase work across a real boundary: the failure this entire fix
exists to remove, reintroduced by its own guard. Now throws, which is how
planDispatch already signals unusable input -- and which removes the
library-writes-to-stderr side effect the warning introduced.
WRONG COUNT. The guard counted phase KEYS. buildPhaseTaskIds keeps empty phases
on the parse path, so {"1":[],"2":[]} has two keys and zero task ids -- it
skipped the guard, currentPhase() returned null, and every ready bead was
deferred 'phase-gate'. The identical total stall, still reachable through a
shape the guard was supposed to cover. Now counts task ids.
Also documents 'unparseable-task-id' in the deferral-reason list in
skills/complete-beads/SKILL.md and its generated Pi copy, which still read
(phase-gate, file-claim-conflict, slot-cap).
Mutation-verified both ways: counting keys instead of task ids fails 1 test;
passing through instead of throwing fails 3. A no-prFormat case asserts the
gate still does not fire when phase strictness was never requested.
36/36 in that file. CI=true npm test: 0 failing suites. npm run generate: clean.
ReviewReviewed the diff ( Bug: dead/unreachable code in
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
…phase keys
Cleanup of my own previous commit, all three found by pass 3.
DEAD CODE THAT ENCODED THE OPPOSITE POLICY. Replacing the fail-open guard with
a throw left the old `phaseCount === 0` warn-and-pass-through block sitting
directly below it, unreachable (any counted task id implies at least one key).
It still said "gating does not apply and all ready beads pass through" — the
exact behaviour this PR removes — waiting for a future edit to re-enable it, and
it kept the library-level process.stderr.write I had claimed to remove. Deleted;
zero stderr writes remain in the file. It had also stranded the "Extract task
IDs from all closed beads" comment above the wrong code; the comment is back
with the closedSet lines it describes.
GUARD COUNTED IDS phase-tracker WOULD DISCARD. sortedPhaseNumbers drops
non-finite keys, so `{a: ['TRD-001']}` counted one id, passed the guard, and
then stalled every ready bead as 'phase-gate' — the same total stall, through
the one shape left uncovered. Now counts ids only under numeric keys.
buildPhaseTaskIds and reconstructPhaseTaskIds only emit numeric keys, so this
reaches only a hand-edited or corrupted map, which is exactly when a loud
failure beats a silent one.
Mutation-verified: counting all keys again fails the new non-numeric case.
HALT CONDITIONS. skills/complete-beads/SKILL.md documented the sibling planner
throw but not this one, so an operator hitting exit 1 had no documented mapping
to complete|blocked|failed. Added there and in the generated Pi copy.
37/37. CI=true npm test: 0 failing suites. npm run generate: clean.
|
@coderabbitai review |
✅ Action performedReview finished.
|
ReviewVerified the core claim directly: Strengths
Notes (non-blocking)
VerdictSolid, well-tested fix for a real silent-failure bug, with an unusually transparent commit trail showing the review process. Only asks: refresh the PR description to match the final diff, and optionally reconsider the guard's error message for the zero-tasks-but-not-actually-broken edge case. |
…date Two reviewers independently found the same hole: the guard hardcoded packages/ and never read the root manifest or the workspaces globs, then printed an unconditional all-clear. A stale range declared at the root, or in a workspace added outside packages/*, drifted unchecked behind a green checkmark — the failure shape this PR exists to remove. - Drive the scan off rootPkg.workspaces and include the root manifest itself. 28 manifests now scanned, up from 27. - Exit non-zero on a workspaces pattern the expander does not understand, rather than silently covering fewer workspaces than the config declares. - Skip file:/link:/workspace:/portal: ranges. semver.satisfies returns false for those, so they were reported as failures with advice to replace a range that already resolves from disk. - Stop suggestRange throwing a bare TypeError on a manifest with no version. Corrected the date in the docstring. The masking did not start in June 2026: 0f44318 added --legacy-peer-deps on 2025-12-14, the same day 218daed moved packages/development to 5.0.0 and broke every ^4.0.0 range. The flag went in as the fix for that breakage and held for nearly eight months. It survived because it never looked like a regression, it looked like CI being repaired. Merged now-green main (26ef93f, #81), which clears the 3 inherited test failures this branch was carrying. Negative-tested: a stale range in the root package.json, an unsupported workspaces glob, and a workspace:* protocol range each behave correctly (fail, fail, pass respectively). npm ci exits 0 with no flags, npm run validate exits 0, and the full suite is now 0 failed across all 8 projects. Filed #85 for what this pass could not fix in scope: the guard step runs after npm ci, so on real drift the install 404s first and the guard never executes. Making it dependency-free enough to run pre-install is new capability, not a fix to this diff. Refs #83, #85
Why
mainhas been red since June with 3 failing tests. They are unrelated to any open PR — including #80, which inherits them and adds zero new failures of its own. This clears them so #80 (and anything else) can merge against a green baseline.The real bug
7b73b7arenamedselectNextTasks' option fromprFormattostackedinphase-tracker.js. The call site incomplete-beads-planner.js:338was missed and still passedprFormat.The result:
options.stacked === trueis never true, soselectNextTaskstakes the unfiltered branch and phase-strict gating does nothing. A later-phase task can jump ahead of an incomplete earlier phase — exactly what the gate exists to prevent.This is silent by construction. The planner still returns a plausible selection; it is just the wrong one.
implement-trd-beadshas been running without phase enforcement since that rename.The planner keeps
prFormatas its own public option name (it comes from the CLI's--pr-format). Only the call intophase-trackermoves to the new key.Mutation-verified: reverting that single word fails 2 tests.
Second fix
trd-cliinvoked with no subcommand emitted only a usage string, so a caller matching on the cause could not distinguish an invocation error from a help request. Now leads withMissing subcommand.and keeps the usage after it.Verification
Install note for anyone reproducing:
npm cialone fails because five workspace packages pin@fortium/ensemble-development@^4.0.0against a workspace at 5.8.0. CI usesnpm ci --legacy-peer-deps, which works. That mismatch is left alone here — separate concern, separate PR.Files
Two, both one-line changes plus explanatory comments.