* fix(#2528): resolve digit-slug phase dirs by bare number — tokenizer rewind, shared bare-integer fallback, resolution-path parity gate
extractPhaseToken welded 2-digit slug words onto the phase token (phase 10
named "24/7 Autonomy" -> dir 10-24-7 -> token 10-24), making digit-prefixed
phase names unresolvable by bare number across every phase verb.
- phase-id: continuation segments must be the PURE 2-digit zero-padded form
the write side emits; a 1-digit terminator rewinds the absorbed run
(10-24-7 -> 10) while >=2-digit terminators keep the locked #2232
round-trip (14-06-2026-photos -> 14-06).
- phase-id: new matchPhaseDirs owner — primary exact-token match plus a
bare-integer leading-digit-run fallback for shapes the tokenizer cannot
rewind (05-80-20-cleanup); collisions stay #2237-loud.
- locator/find-phase/phase-plan-index all delegate selection to the owner;
plan-index gains the previously missing multi-match guard.
- tests: #2528 unit + fast-check metamorphic blocks; new 9-scenario
resolution-path parity gate across all three paths.
Fixes #2528
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* chore(#2528): add changeset for PR #2559
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(#2528): align validation token grammar
* fix: address phase token review
* fix: restore phase grammar parity for numeric slugs
* docs: document digit-leading phase resolution
* docs: clarify ambiguous phase resolution behavior
* docs: register canonical phase directory selectors
* fix: align prefixed deep phase token parsing
* fix(#2528): route the fourth resolution site through matchPhaseDirs
Review BLOCKER. smart-entry.cts::detectVerifyFailed resolved the current
phase's directory with its own `.find(phaseTokenMatches)` and never
reached the shared selection, so the bare-integer-fallback family the
issue names — `05-80-20-cleanup`, `30-12-factor-refactor` — resolved
nowhere. The miss is silent by construction: an unresolved phase reports
"not failed", which is byte-identical to a healthy one, so a failed
verification simply never surfaced in /gsd or /gsd:progress.
`entries` is already sorted and matchPhaseDirs filters without
reordering, so matches[0] reproduces the previous selection exactly
wherever the old code resolved at all.
Wiring it into phase-resolution-parity.test.cjs as a fourth path then
exposed a second, older defect in the same function: phaseTokenFromDirName
shape-probed the UNSTRIPPED token, so a project-code-prefixed directory
(`MEM-05-…`, tokenizing to `MEM-05-80-20`) failed the leading-digit test
and was dropped before any resolution ran — every phase in a
project-coded plan was invisible to this check. The probe now runs on the
stripped token; the returned value is unchanged, so the comparePhaseNum
sort is untouched.
Path 4 has no JSON surface to compare, so the gate observes selection
indirectly: plant the failing artifact in exactly one directory and a
passing one everywhere else, then read the boolean. Reverting either fix
turns 5 of the 10 corpus scenarios red.
* refactor(#2528): collapse the duplicated extractCanonicalPlanId
Review MAJOR. The function existed as two independent, byte-identical
copies — src/core-utils.cts and src/phase.cts — and this PR had to patch
BOTH with the same single-digit-slug rewind rule. That is the generative
fix divergence CLAUDE.md names, and only the core-utils copy was under
test, so a future one-sided patch would have silently split plan-id
canonicalization between the plan listing and everything else.
Removed rather than parity-tested: core-utils was already the leaf owner
and already exported it, and phase.cts already imported that module, so
there is no second surface left for a parity test to police.
* test(#2528): pin matchPhaseDirs at the digit-width boundaries
Review MAJOR. The bare-integer fallback's correctness rests entirely on
capturing each directory's whole leading digit run before the zero-strip
compare; a regex that stopped short would turn every query into a prefix
match, and "1" would claim 10, 100, and 12 alike. The existing coverage
was example-based and never touched that boundary.
Adds the explicit 9/10 and 1/10/100 cases — including the forms where
only the wider directories exist, so an exact-width neighbour cannot
satisfy the assertion — plus a fast-check property over arbitrary
distinct leading runs. The property is stated as an invariant on the
result (every returned directory's leading run IS the query) rather than
an expected list, so it covers primary and fallback matches alike and
cannot be satisfied by reimplementing the selection in the test.
Both fail when the fallback regex is degraded to a prefix match.
* fix(#2528): route the remaining eight consumers through matchPhaseDirs
phaseTokenMatches had eight consumers left that each rebuilt the directory
selection around it by hand: phases-list, next-decimal, phase-remove, the
W021 milestone-consistency check, schema-drift, the init-manager overview,
milestone-complete's disk check, and roadmap analyze. Every one of them
reproduced the reported symptom in full after the tokenizer was fixed.
None of them derives a displayed phase number from the matched directory,
so none needs phaseNumberForMatch; the change at each site is the
selection and nothing else. matchPhaseDirs filters without reordering, so
matches[0] reproduces the prior .find() choice wherever the old code
resolved at all.
phaseTokenMatches now has no call sites outside phase-id.cts. It stays
exported as the primitive matchPhaseDirs is built from and as a pinned
canonical surface, but no consumer reaches past the owner to it.
* test(#2528): extend the parity gate to the migrated consumers
Each of the eight is observed through the surface a user sees, not
through the matcher, with a no-directory control so the assertions cannot
be satisfied by a consumer that resolves unconditionally. init-manager
and roadmap-analyze are additionally asserted to agree with each other.
* refactor(#2528): own the case-flexible phase grammar and the leading-digit-run fragment
validate.cts derived its case-flexible regex sources by running
`replaceAll('A-Z', 'A-Za-z')` over two constants exported by phase-id.cts.
That passes lint-phase-id-drift.cjs — there is no literal copy of the
grammar — but it depends on the owner rendering that exact substring. The
day phase-id.cts expresses the same class any other way the replaceAll
silently no-ops and validate.cts narrows to uppercase-only. The failure
mode is a NON-match, so nothing throws and no uppercase-only fixture
notices. Both variants are now derived once, beside the sources they
widen, and imported.
Also names the leading digit run the bare-integer fallback selects on.
It was spelled `/^(\d+)(?:-|$)/` where the fallback filters and `/^\d+/`
where phaseNumberForMatch reads the number back off the winner; selecting
on one run and displaying another would resolve a directory and then label
it with a number that never matched it.
* fix(#2528): refuse to remove a phase when two directories claim its number
cmdPhaseRemove was the only migrated site taking matches[0] with no
multi-match guard. Every sibling resolution path returns ambiguous_matches
and refuses to choose; this one is the DESTRUCTIVE path, so choosing
silently is strictly worse than anywhere else. With 05-80-20-a and
05-90-till-late on disk, `phase remove 5 --force` deleted one of them and
renumbered every phase after it — where the base resolved nothing, deleted
nothing, and the corpus in tests/phase-resolution-parity.test.cjs already
declared that exact input ambiguous.
The refusal is emitted before any file is touched and carries both
candidates. CONSUMER_SCENARIOS could not express the case — every row is
binary, resolving to one directory or to none — so the gate gains a
dedicated ambiguous test. It asserts on the filesystem, not only on the
reported directory_deleted: a null printed after an rmSync would satisfy
every other check.
* fix(#2528): pair digit-leading phase directories with their roadmap phase in validate health
W006/W007 are the ninth site of this bug class and the one a
`phaseTokenMatches` grep could never surface: they resolve roadmap↔disk by
intersecting TOKEN SETS, which is a dir→token labelling rather than the
query→dir selection matchPhaseDirs owns. On the canonical fixture the
label is wrong in both directions at once, so `validate health` reported
"Phase 5 in ROADMAP.md but no directory on disk" AND "Phase 05-80-20
exists on disk but not in ROADMAP.md" for the same directory.
collectDiskPhases now keeps the directory names behind each token, so
W006 can ask the canonical matcher whether a roadmap phase resolves to a
real directory, and W007 — which iterates directories and therefore has no
query to resolve — gets the inverse mapping it never had: a directory is
claimed when some roadmap phase resolves to it.
Both checks are additive: the token intersection still decides every shape
it already decided, and the resolution can only REMOVE a warning. The
regression test carries controls in the other direction — a roadmap phase
with no directory must still raise W006, an unclaimed directory must still
raise W007 — so it cannot be satisfied by a check that stopped reporting.
* docs(#2528): state and pin the directory-side scope of the bare-integer fallback
The matchPhaseDirs docblock claimed deep-decomposition lookups were
untouched. That is true of the QUERY side only — no non-bare query enters
the fallback — but the DIRECTORY side is what changed classification: a
bare `5` now reaches a lone `05-01-auth` and resolves it (phase_number
"05", phase_name "01-auth") where the base found nothing.
The widening is irreducible from directory names alone. `05-01-auth`
(sub-phase 5.1) and `30-12-factor-refactor` (phase 30 named "12-Factor
Refactor") are the same `NN-NN-<slug>` shape, and the discriminator that
would separate them — "is the second segment a valid decimal sub-phase" —
accepts `5.1` and `30.12` equally. Any rule strong enough to exclude the
first excludes the second, which is the defect #2528 exists to fix. So the
tie is broken in favour of resolving, the docblock now says so, and the
consequence is bounded where it matters: two such directories are two
matches, and every caller (including phase remove) refuses to choose.
Pins both directions, since nothing observed the directory side before.
* fix(#2528): count surviving phases by identity in phase remove's STATE resync
#2640 landed on `next` while this branch was open. Its STATE.md phase-count
resync re-derives "which directory was removed" from the query with
`phaseTokenMatches`, which is the tenth site of this issue's defect: the
bare-integer fallback resolves `05-80-20-cleanup` for query `5`, but the
token predicate does not, so the just-deleted directory is counted as still
present and the written `Total Phases` is one too high.
`targetDir` already IS the directory that was removed, and the block is gated
on it being non-null, so identity answers the question exactly — which is also
what the comment above the filter already claimed it did. This keeps
`phaseTokenMatches` out of `phase.cts` rather than re-importing it to satisfy
one call site: the module's public surface should not grow for a question that
does not need re-derivation.
Pinned in the parity gate with a control on a directory the tokenizer reads
correctly, so the assertion is about the digit-leading shape and not about the
counting rule changing for everything.
* fix(#2528): let the resolution layer own the digit-leading slug family alone
The tokenizer rewind this fix carried — pop the last absorbed continuation when
the segment that stopped the scan is a bare single digit — reads
"10-24-7-autonomy" (phase 10 named "24/7 Autonomy") correctly and silently
re-reads "10-24-7-zip" (sub-phase 10.24 named "7-Zip Integration") from "10-24"
to "10". The two names are string-identical in shape, so no local signal
separates them; the rule traded the reported ambiguity for the symmetric one a
level down, on a 15-caller chokepoint whose output also feeds query-less
derivations (STATE.md phase counts, W007, the #2562 key surface). A well-formed
sub-phase directory became unresolvable by its own id — the very symptom #2528
was filed about.
It also bought nothing. The bare-integer fallback in matchPhaseDirs already
resolves "10-24-7-autonomy" for query "10" whatever the token is: no primary
match, bare query, leading digit run "10". The reported case was covered twice,
by two rules, and the two disagreed about the case nobody reported.
So the rewind is removed rather than narrowed, in the tokenizer and in the five
surfaces kept in lockstep with it (BRACKET_PHASE_TOKEN_SOURCE,
PHASE_TOKEN_FROM_DIR_RE, canonicalPlanStem's pair grammar and its collision
branch, roadmap-parser's numericRe, extractCanonicalPlanId), together with the
SINGLE_DIGIT_RUN_SEGMENT_SOURCE owner constant they shared. Disambiguation now
lives only where a QUERY exists to disambiguate against, which is the same
bounded mechanism the "05-80-20-cleanup" shape already used.
Measured, not argued: over 29800 generated directory names, extractPhaseToken is
byte-identical to `next` on every input except the lowercase-continuation class
("01-20a", "05-80-20-25abc") — a rule about the segment itself, not a guess about
its neighbour.
Both readings now stay reachable by their own ids:
matchPhaseDirs(['10-24-7-autonomy'], '10') -> the dir (fallback)
matchPhaseDirs(['10-24-7-zip'], '10') -> the dir (fallback)
matchPhaseDirs(['10-24-7-zip'], '10-24') -> the dir (primary)
* test(#2528): pin the one-continuation boundary the rewind had no coverage for
The regressing shape was invisible to the suite by construction, not by luck:
the deep-rewind property built its cases from `continuationArb` with
`minLength: 2`, so it never exercised the single-continuation case — exactly one
genuine sub-phase level before a digit-leading slug — and every hand-written
fixture used the ambiguous shape only where "phase-plus-slug" was the intended
reading.
`continuationArb` is now `minLength: 1` and the property states the invariant
instead of the old rule: for any prefix, phase, 1-5 continuations and any
one-digit terminator, the token equals the FULL continuation run on both the
imperative and the regex surface, and `matchPhaseDirs([dir], token)` returns that
dir. That third assertion is the one that catches the class on its own — the old
behaviour made a well-formed directory unresolvable by its own id, which is a
property, not a fixture.
Around it: "10-24-7-zip" and "10-24-3d-printer" now sit beside
"10-24-7-autonomy" everywhere the family is pinned, so the two readings can never
diverge again; the 24/7 metamorphic property asserts the RESOLUTION result rather
than the token (the token is precisely the part no surface may decide); the
end-to-end parity corpus gains "a sub-phase with a digit-leading slug resolves by
its full id" across all four resolution paths; and the milestone-scoping residual
is pinned in three directions rather than left to prose.
Mutation: re-inserting the rewind and rebuilding turns 9 tests red, the
`minLength: 1` property first, and nothing else. Build success checked separately
(build:lib reports 0 `error TS`), so the mutation reached the artifact under test.
* test(#2528): pin the #2946 guard against digit-leading phase directories
The #2946 fix makes the milestone-complete unstarted-phase guard run
unconditionally, so whether it fires now rides entirely on the
directory-resolution owner this PR replaces. Two cases, both with STATE.md
carrying no `milestone:` field so the #2946 path is the one exercised:
- ROADMAP Phase 5, disk `05-80-20-cleanup` → guard must stay silent.
RED on next (fail-closed: the guard blocks a legitimate one-way-door
operation because phaseTokenMatches resolves neither 05 nor 80 for
that directory).
- ROADMAP Phase 80, same directory → guard must still fire. Green on
both sides; it pins the fail-open direction against a future widening
of the matcher.
* fix(#3175): stop the injection scanner reading RegExp.exec as code execution
The apostrophe fix in 27aa40f6 replaced ["\x27] with a real ["'] class.
The old class never contained an apostrophe at all (POSIX bracket
expressions do not honour backslash escapes, so it was the set ", \, x,
2, 7), so only exec(" matched. Single-quoted method calls now match for
the first time, and RegExp.prototype.exec takes a subject string, not
code: any PR touching a file that tests a regex goes red. On next, six
files match the scanner's own pattern across 16 method calls.
A plain [^[:alnum:]] boundary cannot separate the two forms because . is
not alnum, so exec gets [^[:alnum:].] and the command-execution vector
moves to a dedicated member-call pattern. Bare exec('rm -rf /'),
cp.exec(...) and child_process.exec(...) all still fire.
Mutation: reverting the boundary reds 1 test and only it; removing the
member-call pattern reds the 2 non-weakening tests and only them.
Co-Authored-By: Claude <noreply@anthropic.com>
* fix(#2528): keep exec( detection receiver-blind, allowlist the two grammar suites
The left boundary [^[:alnum:].] added in 58a7b560 excluded a preceding dot,
which dropped every member-position .exec('…') from the scanner. The follow-up
receiver pattern only restored three literal spellings (child_process,
childProcess, cp), so require('child_process').exec('…') — the most common Node
spelling of the vector this pattern exists to catch — became invisible, along
with any opaque receiver (conn.exec, shelljs.exec).
Revert the pattern to its receiver-blind form and handle the RegExp.prototype
.exec false positive where the script already handles this class: per-file
ALLOWLIST entries for the two phase-token grammar suites. Mutation-checked —
removing the two entries reds exactly those two files and nothing else.
The four assertions written around the old patterns are replaced by a
table-driven set covering all six spellings, including the three the narrowed
pattern silently lost.
Co-Authored-By: Claude <noreply@anthropic.com>
* docs(#2528): pin the three undeclared grammar edges, correct the ambiguity claim
Review round 10 asked for declaration, not behavior change, on four items. All
four have zero production consumers or preserve their caller's prior rule, so
each is pinned as a test or corrected in prose rather than reverted.
- BRACKET_PHASE_TOKEN_SOURCE: the (?=-|$) terminator is what keeps the bracket
read path in step with the other surfaces, and it costs the display shapes
(`05.03: Title`, `12A: X`, `05.03]` no longer tokenize). Pinned so widening
the terminator class is a deliberate act rather than a lookahead deletion.
- canonicalPlanStem: uppercase plan suffixes still strip, lowercase and dotted
sub-plans now fall through. Dead export; pinned as a decision on record.
- getMilestonePhaseFilter: `12A-01-foo` now yields `12A-01`, matching what
`12-01-foo` has always yielded. The letter suffix was the only reason a
sub-phase directory folded into its parent phase's milestone window; the two
shapes now agree. Not named in the review — found auditing the same commit.
- matchPhaseDirs docblock claimed every caller refuses on multi-match. Four do;
five take matches[0]. Replaced the claim with the actual two-tier policy and
the honest caveat that the bare fallback makes multi-match newly reachable
for queries that previously found nothing.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Co-authored-by: Tom Boucher <trekkie@nomorestars.com>
This commit is contained in:
JusticeWay
@@ -47,6 +47,7 @@
|
||||
"files": [
|
||||
"phase-completion-single-owner.test.cjs",
|
||||
"phase-dependency-levels.test.cjs",
|
||||
"phase-resolution-parity.test.cjs",
|
||||
"phase.test.cjs"
|
||||
],
|
||||
"issue": "3186"
|
||||
|
||||
@@ -72,6 +72,15 @@ PATTERNS=(
|
||||
# `eval('...')` (single-quoted) silently went undetected on macOS while
|
||||
# passing on GNU-grep CI runners. Found auditing #3175; fixed here since it
|
||||
# is the same unanchored/portability defect class as the boundary fix.
|
||||
#
|
||||
# `exec` stays receiver-blind on purpose. A left boundary that excludes a
|
||||
# preceding `.` would drop every member-position `.exec('…')` — including
|
||||
# `require('child_process').exec('…')`, the single most common Node spelling
|
||||
# of the vector this pattern exists to catch — and a receiver allowlist
|
||||
# cannot restore it, because the literal `child_process` is not adjacent to
|
||||
# `.exec`. The cost is that `RegExp.prototype.exec`, which takes a subject
|
||||
# string rather than code, also matches; files that legitimately call it are
|
||||
# handled by ALLOWLIST below, never by narrowing the pattern.
|
||||
'(^|[^[:alnum:]])eval[[:space:]]*\([[:space:]]*["'"'"']'
|
||||
'exec[[:space:]]*\([[:space:]]*["'"'"']'
|
||||
'(^|[^[:alnum:]])Function[[:space:]]*\([[:space:]]*["'"'"'].*return'
|
||||
@@ -137,6 +146,14 @@ ALLOWLIST=(
|
||||
# here only because #2573's W024 `state_head` assertions make the file appear in
|
||||
# the changed-file set the diff-mode scan walks.
|
||||
'tests/health-validation.test.cjs'
|
||||
# #2528 — same collision, same disposition: the continuation-grammar suite
|
||||
# drives the tokenizer regexes directly via `re.exec('05-80-20')`, so the
|
||||
# argument is the subject string, not a command. Exempted per file rather
|
||||
# than by narrowing the `exec(` pattern: a left boundary excluding a preceding
|
||||
# `.` would drop `require('child_process').exec('…')`, and a receiver
|
||||
# allowlist cannot reach it either, because the literal `child_process` is
|
||||
# not adjacent to `.exec`. See the note at the pattern itself.
|
||||
'tests/continuation-grammar-parity.test.cjs'
|
||||
)
|
||||
|
||||
is_allowlisted() {
|
||||
|
||||
Reference in New Issue
Block a user