next
17 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Jakub Zych
|
a9a7a328e6 |
refactor: hard-fork GSD -> MSD (Make Software Done)
Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD across contents and paths, upstream package/repo coordinates -> @golem15/msd-core and golem15com/msd-core. Deep links into upstream history, sibling upstream packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is. Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line, package/plugin identity, regenerated lockfile, install-tree fixtures, derived registries and benchmark baseline; migration checksum baseline re-locked (MSD keeps its own install state, so no install had applied the old sums); sort-order and regex-escaped expectations in tests adjusted. |
||
|
Tom Boucher
|
5e0a7b1b56 |
fix(#4433,#4569,#4126): consolidate the phase-identity seam at name-validity, allocation, and branch-slug (#4640)
* fix(#4433): apply the name-validity guard symmetrically to every milestone-name capture extractMilestoneHeadingName already refused a punctuation-only captured name (#4134), but its two sibling capture sites in getMilestoneInfo — the STATE.md-anchored 🚧-bullet match and the no-STATE.md in-progress 🚧-bullet fallback — skipped straight to a bare truthiness check, so a malformed bullet whose only content past the version was punctuation passed through as a real milestone name. Extracts the existing inline /[\p{L}\p{N}]/u check into a single shared hasNameableContent predicate and applies it at all three capture sites, so the guard is one owner rather than a copy that happened to land at only one of them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4433): pin the name-validity guard at all three milestone-name capture sites Failing-first coverage for the hasNameableContent extraction: a punctuation-only 🚧-bullet name must not surface as a real milestone name, either on the STATE.md-anchored path or the no-STATE.md in-progress fallback, while a real name (including a digits-only one) still resolves COMPLETE exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4569): consolidate decimal-phase-number allocation into one function cmdPhaseInsert allocated its next decimal sub-phase number by scanning only on-disk phases/ directories and ### Phase N.M: headings, never the roadmap summary checklist — so a decimal that existed only as a checklist bullet (no heading yet, no on-disk directory yet) was invisible, and phase insert could silently reallocate an already-used number. It also always nested one level deeper under afterPhase, with no way to request a sibling. cmdPhaseNextDecimal had its own separate, near-identical two-source scan (missing the checklist source too) — the exact "duplicate implementations kept in sync instead of deleted" pattern this issue exists to close. Extracts scanExistingDecimalPhaseNumbers (directories + headings + checklist bullets, in one place) and migrates both cmdPhaseInsert and cmdPhaseNextDecimal onto it — deleting cmdPhaseNextDecimal's own copy rather than patching it in parallel. Adds an allocation: 'nested' | 'sibling' argument to cmdPhaseInsert (default 'nested', matching every existing caller's behavior); a top-level phase with no existing decimal segment falls back to nested since there is no sibling level to join. No CLI flag wires 'sibling' yet — that is a separate, disclosed follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4569): pin decimal-allocation coverage across phase insert and next-decimal Failing-first coverage for scanExistingDecimalPhaseNumbers: a checklist-only decimal must not be reallocated by phase insert; a decimal present in heading, checklist, and on-disk directory simultaneously must count once; an unrelated phase family's checklist bullet must not cross-pollute; and phase next-decimal (migrated onto the same shared helper) must see a checklist-only decimal too, closing the same gap in a second command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): extend the phase-id drift guard for name-validity and shell arithmetic The epic's ratchet requirement: lint-phase-id-drift.cjs must cover the two new predicates this PR introduces, and must also scan shell inside gsd-core/workflows/**/*.md and gsd-core/references/**/*.md for integer-coercing phase-number arithmetic ($((10#...)) and friends), which neither the canonical TypeScript module nor a source-only lint can reach. Adds findNameValidityDrift (bans re-deriving /[\p{L}\p{N}]/u outside hasNameableContent's owner file) and findShellPhaseArithDrift + scanMarkdownShellArith (bans $((10#...)) in workflow/reference markdown, sanctioned via <!-- phase-id-owner: --> on the preceding line). scanRepo keeps its existing, narrower contract (src/**/*.cts only) so the already-passing "the live repo is clean" test is untouched; a new scanAll merges both for the CLI's full report. Running the guard directly against this tree correctly reports the 7 pre-existing #4619 shell sites (workflows/execute-phase.md x4, workflows/execute-phase/steps/completion-reconciliation.md x2, references/tdd.md x1) as violations — demonstrating the ratchet works, not fixing them. #4619 is a live regression tracked and fixed separately; this PR does not touch those markdown files. A characterization test pins the current count of 7 so a future change to that number is investigated rather than silently absorbed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4569): wire --sibling through phase insert's CLI so the argument is reachable cmdPhaseInsert's allocation parameter had no CLI path to 'sibling' — shipped, untested, unreachable code (code-review finding: a guaranteed surviving mutant). Adds --sibling to phase insert's argument parsing, threads it through, and documents the flag in docs/CLI-TOOLS.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4569): exercise --sibling end-to-end through the real CLI Confirms --sibling joins afterPhase's parent decimal level rather than nesting, and falls back to nested when afterPhase has no existing decimal segment (no sibling level to join). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4634): demonstrate the two new drift detectors end-to-end via a planted violation The epic asks for the guard to be "demonstrated by watching it go red" on a reintroduced copy. The two new detectors (name-validity, shell-arith) had only unit-level fixture tests; mirrors the existing bracket-rule's planted-violation-in-a-temp-tree test for both, proving they're actually wired into scanRepo/scanMarkdownShellArith end-to-end, not just correct in isolation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): consolidate the drift guard's own owner-sanction-check logic Standards review flagged the "walk to nearest preceding non-blank line, check for a phase-id-owner comment" logic as duplicated across all four detector functions in a PR whose whole point is eliminating exactly that pattern. Extracts isSanctionedByPrecedingComment, shared by all four; behavior-preserving (verified: identical output before/after, same 7 known #4619 violations, zero token/bracket/name-validity). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): add Fixed changeset for the name-validity guard and allocation consolidation pr:0 placeholder — backfilled once the real PR number exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4126): consolidate branch-name slug substitution into one shared renderer cmdCommit (commands.cts) and cmdInitExecutePhase (init.cts) each independently implemented branch-name template substitution, and both substituted the literal string 'phase' when phase_slug was empty or undeliverable — producing a non-identifying branch name (gsd/phase-08-phase) that contradicted the honestly-reported phase_slug: null in the same payload. Same structural defect as the other three gaps in this epic: two consumers reimplementing one concept independently instead of sharing an owner. Adds renderPhaseBranchName (src/phase-id.cts) as the sole owner: a real slug substitutes normally; an empty/undeliverable one drops the {slug} token plus one adjacent separator (collapsing/trimming the result) rather than substituting a placeholder word, for the shipped default template and any user-configured shape alike. Both call sites now delegate to it; the old inline duplicates are deleted, not kept in sync. {project} substitution stays a separate step in init.cts, unchanged, since it is a config-level field with its own fallback contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4126): pin renderPhaseBranchName and both migrated call sites Property-based coverage for the shared renderer's degrade-path invariant (output, when non-null, never contains {slug} and never starts/ends with a separator), plus example coverage for real-slug substitution, empty/null/ non-string slug, token position at either edge, a doubled-separator template, and the only-{slug} -> null case. One regression test each in commands.test.cjs and init.test.cjs confirms a phase with no derivable slug no longer produces a branch name ending in the literal '-phase'. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: route scanExistingDecimalPhaseNumbers through the canonical enumeration owner Caught by an actual gsd-test run, not a hypothesis: the new decimal-scan helper (fix(#4569)) enumerated phases/ directories via a raw fs.readdirSync, which the pre-existing phase-enumeration drift guard (#3185/#3882) correctly flags as an unsanctioned re-derivation outside its canonical owner (listAllPhaseDirs / isSentinelPhaseId). Ironic given this epic's own thesis, and exactly why the guard exists: consolidating one seam can reintroduce drift in an adjacent one if the new code doesn't route through what's already there. Migrates the enumeration to listAllPhaseDirs; identical decimal-detection output for every existing case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): extend the drift guard for branch-slug fallback; fix a real regex bug Adds the fourth detector the epic's ratchet section names ("both branch-name sites"): bans a `.replace('{slug}', ... || 'phase')` call outright, sanctioned via renderPhaseBranchName or a dedicated comment. Wired into scanRepo (no per-file exemption — this is a banned anti-pattern everywhere, not a grammar with one legitimate owner). Now that #4126's fix (prior commit) has landed, scanRepo reports zero violations across all four .cts-scanning rules, restoring the simple "the live repo is clean" assertion instead of a pinned-known-count characterization. Also fixes a real bug an actual gsd-test run caught: findNameValidityDrift's regex didn't tolerate the doubled-backslash template-string form its own test claimed to cover (0 !== 1) — widened to \{1,2} matching TOKEN_DRIFT_RE's existing tolerance for the same two forms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4126): document the {slug} degrade behavior; update changeset for the full seam Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: detectPhaseNumberFromFiles wrongly rejected bare, slug-less phase directories Caught by an actual gsd-test run on the #4126 regression test, not a hypothesis: a bare phase directory with no slug remainder (e.g. .planning/phases/01/) has extractPhaseToken correctly return "01" — which is simply identical to the directory name in that case, not its no-match fallback. A stale `token !== phaseDir` check treated that equality as "no numeric token found" and rejected it regardless, leaving phaseNum null and silently skipping cmdCommit's phase-branching block entirely (the commit proceeded on whatever branch was already checked out instead of the phase branch). phaseTokenShape.test(normalized) already excludes every genuine non-phase case on its own: extractPhaseToken's real no-match fallback only fires for a dirName that doesn't start with a digit or short letter+digit prefix, and normalizePhaseName's leading-\d+ requirement rejects those regardless. The equality check was redundant for real rejections and actively wrong for bare-numeric directories. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: backfill changeset PR number to 4640 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
59e7a677fe | fix(#3511): scope every phase-directory scan to the phase it belongs to (#3535) | ||
|
Tom Boucher
|
dc3c81e93d |
chore(#3212): src/pattern.cts is the sole owner of runtime-value regex construction — Phase 1 (#3416)
* test(#3412): failing-first suite for the pattern-construction seam Phase 1 of epic #3212 (ADR-3212 §1/§2/§7). Tests only — src/pattern.cts and eslint-rules/no-adhoc-regex-escape.cjs do not exist yet, so both suites fail with MODULE_NOT_FOUND, which is the intended RED. Locks the measured behavior rather than the assumed behavior: RegExp.escape hex-escapes the leading character of nearly every string ("abc" -> "\x61bc"), so the suite asserts match-equivalence against an inlined historical oracle (the implementation being deleted) rather than byte-equivalence of pattern text — 200 seeded fast-check runs plus a fixed corpus, 0 mismatches. Also locks the latent character-class range bug this phase fixes as a side effect: a hyphen-bearing value interpolated into [...] currently forms a real range and matches an unintended character; post-migration it must not. * chore(#3412): src/pattern.cts owns runtime-value regex construction Phase 1 of epic #3212 (ADR-3212 §1/§2/§6/§7). Adds the pattern seam delegating to the built-in RegExp.escape, deletes every hand-rolled copy, and raises the Node floor to the Active LTS line. The census was low, three times over. ADR-3212 counted 10 copies; a graph query found 12; the new lint rule — once live — found 27 more. The difference is that the census counted named helper FUNCTIONS while the rule counts the escape SHAPE, so inline .replace(<class>, '\$&') copies were never in scope. ADR §1's actual requirement is that no module outside the seam escapes a value for regex use, so all of them are, and CLAUDE.md's no-defer rule makes them this change's work. Fourth consecutive epic here whose copy count was low — the argument for ADR-3180 Amendment 3's "state N found by the guard" rule. Also corrected mid-implementation: the survey reported phase-id.cts's escapeRegex had 0 external importers. It had 8 production importers, making its removal a public-surface change to an ADR-2121-owned module and requiring an update to that ADR's locked-surface test. Blast radius revised Medium-High -> High. RegExp.escape is match-equivalent but NOT text-equivalent: it hex-escapes the leading char of nearly every string ("abc" -> "\x61bc"). Equivalence is proven by a seeded fast-check property test against the deleted implementation as oracle. It also fixes a latent bug: a hyphen-bearing value interpolated into a character class previously formed a real range and matched an unintended character. Node floor 22 -> 24 (RegExp.escape is Node 24+), across engines, .nvmrc, package-lock, 9 CI matrix entries, and 5 docs. The aggregate `required-tests` context is unchanged and no job was added or removed, so branch protection cannot be orphaned by the dropped lanes. Enforced by eslint-rules/no-adhoc-regex-escape.cjs (shape-matched, with structural provenance for reviewed pattern-fragment constants rather than a name heuristic) plus a whole-tree companion guard covering the directories ESLint's globs miss. * fix(#3412): close the _SOURCE guard evasion, correct two false claims Three findings from the orthogonal review pass, all fixed. 1. The ESLint rule's `_SOURCE` provenance fallback was pure identifier- name matching with no binding check, so `new RegExp(userInput_SOURCE)` — a function parameter — sailed past the guard. That is the same rename-evasion class issue #3410 documents, reopened by the very fallback meant to complement the structural check. Now bound to the identifier's actual binding kind: import, require-derived const, or module-scope const; parameters, `let`/`var`, and unresolvable bindings fail closed. Four RuleTester cases cover the evasion and prove the legitimate cross-module case still passes. 2. src/pattern.cts's own header carried the stale pre-correction counts (12 copies / 17 call sites) while CONTEXT.md and the design doc carried the corrected ones (~39 / ~44) — a self-contradiction inside the PR whose entire purpose is deleting divergent copies. Rewritten, preserving the durable lesson: a named-function census cannot see inline copies; only a shape-matching guard can. 3. The claim that all deleted copies threw TypeError on non-string was false. phase-id.cts's copy — the one with 8 external importers — did String(value).replace(...) and never threw. The seam's locked signature does not coerce, so this is a real, now-disclosed behavior change rather than the pure preservation the tests asserted. Audited all 32 invocations across the 8 importers and 6 in-file callers: every one is safe by construction (upstream truthy guard or a string-producing derivation), verified by runtime probe against the compiled modules rather than by TS compilation, which cannot see a runtime undefined. Corrected the false claim in both the test comment and the design doc, and added it to Known limits. * docs(#3412): add Changed changeset for the Node 24 floor The only user-visible break in this phase. The escape-behavior change is internal and match-equivalent, so it carries no user-facing note. * fix(#3412): resolve the seam's require graph in script fixtures and packaging Checkpoint 2 came back red with 90 failures on the node24 lane. Three distinct defects, all introduced by routing scripts/ through the new pattern seam, none reproducible by any local gate: 1. ~82 failures — tests/adr-index-gate.test.cjs and tests/removed-but-needed-lint.test.cjs copy a scripts/*.cjs into an mkdtemp fixture and spawn it there (necessary: those scripts resolve their scan root from __dirname/.., so running the real script would scan the real repo). Each harness hand-listed the dependencies to copy alongside. Adding require('../gsd-core/bin/lib/pattern.cjs') to gen-adr-index.cjs made both lists silently incomplete -> MODULE_NOT_FOUND, plus 17 downstream 'did not emit parseable JSON' failures from the same crash. Fixed as a class, not an instance: new tests/helpers/copy-script- fixture.cjs walks a script's transitive static relative-require graph and copies it, so dependencies are derived and never re-declared. It throws (naming the unbuilt artifact) instead of letting the child die with a bare MODULE_NOT_FOUND. Verified for all four seam-consuming scripts: gen-adr-index, lint-removed-but-needed, gen-loop-host- contract, sync-runtime-launcher. 2. 2 failures — scripts/ ships wholesale but eslint-rules/ does not, so the new scripts/lint-no-adhoc-regex-escape.cjs would be MODULE_NOT_FOUND in a published install (#2858 guard). Excluded from the tarball, matching the existing precedent for gen-emitted- baseline.cjs, which is excluded for the identical reason, and locked with a test modeled on that one. Confirmed against a real npm pack: 890 files, 0 from eslint-rules/, and gsd-core/bin/lib/pattern.cjs present (so the other four scripts' requires are legitimate). 3. 6 failures — tests/phase-id.test.cjs asserted the literal escaped source text ('0*29', 'PROJ-42'). RegExp.escape is match-equivalent to the retired hand-rolled escaper but NOT text-equivalent: it hex- escapes the leading character and all hyphens ('0*\x329', '\x50ROJ\x2d42'). Verified NOT a behavior change — 576 match decisions across all three real interpolation prefixes, zero divergence. Those tests now compile each source into the same heading regex src/roadmap.cts's searchPhaseInContent builds and assert what matches and what does not, including the 'i'-flag canonicalization the hex escape has to preserve. Re-pinning the new literals would have rebuilt the same brittleness one layer down. Adds a test for the property the escape exists for: a dot in '1.2' must not act as a wildcard. Also shares one definition of 'a require' between the packaging guard and the fixture copier, so the two cannot disagree about what they scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): refuse to copy a fixture dependency outside the fixture root copyScriptWithDeps resolved each relative require and joined the repo-relative result onto fixtureRoot. A require resolving OUTSIDE the repo yields a '../'-prefixed relative path, so path.join climbed out of the fixture and wrote into the surrounding temp dir (verified: repoRoot=/repo + depAbs=/etc/passwd wrote /tmp/etc/passwd). No script in the tree does this today, so this closes an available escape rather than an active one. Refuses via the existing unresolved- require path so the failure names the offending specifier. Covered by a negative proof that the guard fires and that nothing lands outside the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): parse requires instead of pattern-matching them; restore the foreign-prefix contract Applies all findings from the second orthogonal review round, re-run because real code changed after round 1. HIGH (security) — extractRequires stripped BLOCK comments before LINE comments, so a '//' comment containing '/*' opened a phantom block comment, and a '//' inside a string literal truncated the line. Both hid real requires: 'const u="http://x"; require("./real.cjs")' returned [], and four real requires in gsd-core/bin/gsd-tools.cjs were invisible. Replaced with a real AST parse via espree. This is ADR-3212's own Decision 4 — tokenizer-first for stateful grammars — applied to the case it describes; comment/string/regex nesting is exactly such a grammar, which is why the regex version was wrong. The function was moved byte-identical out of the #2858 packaging guard, so the bug PRE-DATES this branch and has been a live blind spot there: a shipped script could have required an unshipped path undetected. Fixing it makes that guard strictly stronger than on next. espree is promoted from a transitive eslint dependency to an explicit devDependency rather than relying on hoisting. The script parse attempt sets ecmaFeatures.globalReturn because Node wraps CommonJS bodies in a function, making a top-level return legal — scripts/check-coverage-gate .cjs relies on it, and without the flag the guard throws on a file it is supposed to scan. Verified 0 unparseable across all 324 .cjs/.js under scripts/, bin/, and gsd-core/bin/, and 0 new violations against a real npm pack, so the exact extractor does not newly fail the guard. MEDIUM (security) — the repo-containment check guarded dependencies but not the entry path. One escapesContainment predicate now guards both. LOW (security) — containment was lexical while fs follows symlinks, and a directory symlink could mint a fresh dedupe key per level. realpath now resolves both repoRoot and each dependency before the decision, and the realpath-derived path is the dedupe key. Destination layout still uses the original repo-relative path, so copied trees are unchanged. MAJOR (standards) — the round-1 behavioral rewrite of phase-id tests lost the foreign-prefix contract: every assertion was satisfied by an impl returning [A-Z]+\x2d42, i.e. ANY project code — the exact #3599 bug class the exact-source prevents. The literal assertions it replaced were catching this. Now asserts the compiled regex REJECTS a different prefix with the same number. MAJOR (standards) — the test hand-duplicated production's heading regex with no parity guard (CLAUDE.md's 'Generative Fix Divergence'). Removed the parallel surface instead of policing it: src/roadmap.cts exports buildPhaseHeadingRegex, searchPhaseInContent calls it, the test imports it. Byte-identical .source and .flags verified for both escaped forms. MINOR — '..foo' no longer false-flagged as an escape; the inverted spurious-vs-missing doc claim corrected; the dead allow-test-rule header removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3412): backfill changeset pr number to 3416 * fix(#3412): make the escape guard's own regex linear, reword an injection-scan collision Two CI failures on PR #3416, both in code this branch added. CodeQL js/redos (high) — REPLACE_CALL_RE's outer alternation let a bracket run be consumed EITHER by the character-class branch OR one character at a time by the trailing catch-all, so a failing match explored both parses of every pair. Measured on the real regex: n=26 -> 204ms, n=28 -> 791ms, n=30 -> 3475ms, a clean 2^n. This script scans repo source, so a file with a long bracket run after '.replace(/' would hang CI outright — a guard against undisciplined pattern construction was itself the worst pattern in the diff. Fixed the way ADR-3212 already prescribes: the catch-all branch now excludes '[' and ']' so a bracket can only be consumed by the class branch (this is what makes it linear), and every quantifier is bounded (the locked bounded-quantifiers decision) as a second line of defense. Now 0ms at n=2000. Disclosed coverage tradeoff, recorded at the constant: a regex literal with a BARE unescaped ']' outside a class is no longer matched by this backstop. No census shape has that form, and the AST rule remains the primary detector. Verified the guard did not go blind doing it: a real census-shape violation is still reported, and an allow-adhoc-regex-escape suppression comment is still honored. Regression test drives the exported findViolations on a 2000-repetition adversarial input and asserts the RESULT. It makes no wall-clock assertion — elapsed-time tests are forbidden — so a regression surfaces as a harness timeout, which is the correct signal. Prompt injection scan — 'must not act as a regex wildcard' in a test comment matched the scanner's jailbreak pattern act\s+as\s+(a|an|if| my). Reworded to 'behave as'. Deliberately NOT allowlisted: silencing a whole test file over one phrase would blunt the scanner permanently, and the comment has nothing to do with injection. Neither failure was reachable from the remote runner — CodeQL and the injection scan are not in that matrix, so the sha it passed was green and still wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
JusticeWay
|
77374cfc32 |
fix(#2528): resolve digit-leading phase directories by bare number (#2559)
* 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
|
||
|
Adnan
|
137f3fbb9c |
fix(#2562): scope workstream progress/status to the current milestone (#2588)
* fix(#2562): scope workstream progress/status to the current milestone `workstream progress` / `workstream status` / `workstream list` share one derivation that could report a workstream's CURRENT milestone as "milestone complete" / 100% while phases in that milestone were unstarted, in progress, or failing verification. Three coupled defects: 1. The shipped signal was project-lifetime, not milestone-scoped: workstreamMilestoneShipped() returned true if ANY *-ROADMAP.md snapshot existed or "SHIPPED" appeared anywhere in ROADMAP.md. Every prior shipped milestone leaves a permanent collapsed <summary>✅ … SHIPPED</summary> block, so any post-v1.0 workstream was pinned to "milestone complete" forever (over-correction from #1913). 2. The denominator dropped declared-but-unscaffolded phases, and completed PRIOR-milestone phase directories inflated the numerator, letting progress_percent round to 100 while real work remained. 3. Phase completeness ignored the VERIFICATION verdict — SUMMARY >= PLAN count alone marked a phase complete even with a human_needed verdict. Fix: derive both numerator and denominator from artifacts scoped to the current milestone. The current version comes from the workstream STATE.md `milestone:` field (ROADMAP in-progress markers can be stale); the ROADMAP `## Progress` table maps every phase — including dirless ones — to its milestone, and the matching set is both the denominator and the directory membership filter. The shipped signal now requires the CURRENT version's archived ROADMAP snapshot (REQUIREMENTS snapshots are not accepted; they can be written at milestone start) or the current milestone's own line marked shipped. Phases with an explicit failing verdict (gaps_found/human_needed) count as in_progress; missing/unknown/stale are left untouched so verifier-disabled projects do not regress to never-complete. Greenfield roadmaps with no versioned Progress table, and projects whose current version cannot be determined, keep the prior behaviour. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#2562): add changeset for workstream milestone-scoping fix Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(#2562): parse the Progress table via findTableWithColumns The ad-hoc pipe-table regex tripped the local/no-adhoc-markdown-parsing ESLint rule. Use the canonical markdown-table helper instead: the milestone-grouped RoadmapProgress variant is located by its required `Phase` + `Milestone` columns and cells are addressed by column NAME, so the parser tolerates column reordering and injected columns. The `flat` variant (no Milestone column) yields no attribution, which is the intended fallback to legacy counting. Behaviour is unchanged: verified against a real multi-workstream project (same status/percent/phase and plan counts before and after). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2562): count table-only phases in the unscoped denominator Addresses the reporter's repro detail: a phase declared as a `## Progress` table row with no `### Phase N` heading is missed by countRoadmapPhases EVEN WHEN other headings exist — the heading regex counts 1 for a "1 heading + 1 table-only" roadmap — not just in the zero-heading fallback path. Milestone scoping did not cover this, because a flat Progress table (no Milestone column) carries no per-phase attribution, so greenfield and single-milestone projects kept the old heading-only denominator and the declared phase silently vanished from it. When milestone scoping cannot engage, the denominator is now the union of the Progress table's declared phase numbers and the phase directories, so neither source can shrink it. Verified against the reporter's minimal fixture (phase 1: 1 PLAN + 1 SUMMARY + gaps_found; phase 2: table row only, no heading, no dir), which now reports 0/2 at 0% across all four table/STATE permutations instead of 1/1 at 100%. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2562): attribute dir-only sub-phases to their parent's milestone A sub-phase directory inserted mid-milestone (e.g. `30.1-…` under a table-declared phase 30) usually has no ROADMAP Progress-table row, so it had no milestone attribution and was scoped out of the rollup entirely — its completed work was invisible and it could never hold the percentage below 100. It now inherits its parent phase's milestone and joins BOTH sides of the calculation. Both sides is the load-bearing part: adding it to the numerator alone would let completed_phases exceed a denominator that never counted it, cap back to 100% via Math.min, and reintroduce exactly the defect this issue reports. A regression test pins that failure mode (all declared phases complete + an in-progress dir-only sub-phase → 75%, not 100%). Attribution is deliberately one-directional: a sub-phase counts only when its PARENT is in the current milestone, so a follow-up created in a later milestone under an older parent is excluded rather than misattributed — conservative (under-count) rather than falsely inflating. Verified on a real project: the reported workstream moves from 2/6 (33%) to 3/7 (43%), the 3/7 being the honest figure — a completed sub-phase that was previously invisible now counts, and so does its plan total. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(#2562): describe the denominator + sub-phase fixes in the changeset The fragment was written at the first commit and only covered the three original defects. Bring it up to date with what actually ships: the table-only-phase denominator union (heading-only counting dropped a declared phase even when other headings existed) and sub-phase milestone inheritance across both sides of the calculation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(#2562): promote the canonical phase-key surface to phase-id state.cts kept `phaseKeyFromToken`/`phaseKeyFromDir` private, so every other module that had to compare two independently-derived phase references — a ROADMAP table cell against a phase directory, say — wrote its own regex. That is the defect class #2562 reports: a padded `01` and an unpadded `1-slug` land in different key spaces and the comparison silently yields nothing. Move the pair to the phase-id owner module and add `phaseKeyFromProse` (for ROADMAP/STATE prose, markdown emphasis stripped) and `parentPhaseKey` (a sub-phase's parent). state.cts imports them; its call sites are unchanged. * fix(#2562): own milestone-shipped detection and accept a workstream scope Three changes to the module that owns milestone parsing, so its consumers stop reimplementing it: - `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `<summary>` lines only. A bullet that merely names the version (`- [x] 03-01: ship the v2.0 login endpoint`) is prose about a phase, not a milestone verdict. The version token is boundary-matched with `(?![\w.-])` — `\b` does not bound it, since `.` is a non-word character, so a shipped `v2.0.1` heading would otherwise close `v2.0`. - `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name and thread it to `planningDir(cwd, ws)`. A caller iterating workstreams cannot set `GSD_WORKSTREAM` per iteration, which is what the existing resolution falls back to. Omitted, resolution is unchanged. - `getMilestonePhaseFilter` exposes `versionScoped`, true only when the phase set really is one milestone's. On an unversioned roadmap `phaseCount` spans the project's lifetime and must not be read as a current-milestone denominator. The closed/active milestone-marker patterns were kept in three byte-identical copies; they are hoisted to module scope as one `isClosedMilestoneHeading`. * fix(#2562): derive membership and denominator from one phase-key space The milestone scoping added earlier in this PR derived the ROADMAP table key and the phase-directory key with two different regexes, and dropped rows it could not attribute. Each of those was another way to reproduce the symptom this issue reports — a rollup contradicting its own `phases[]` listing: - a padded `| 01. … |` row never matched a `1-slug` directory (and a bespoke `^0*(\d+…)` never matched `PROJ-05-…` at all), so phases fell out of the milestone entirely and the percentage collapsed or pinned; - a blank or malformed Milestone cell deleted the phase from BOTH sides, letting an unstarted phase vanish and the remainder round to 100%; - shipped detection scanned bullets, so any checkmarked line naming the version closed the milestone; - the numerator counted per-directory while the denominator counted distinct phases, so a stale same-numbered directory (Bug #2445's scenario) pushed `completed_phases` past the denominator, where `Math.min` capped it to 100% and hid the unstarted phase. Both sides now key off the phase-id owner module (`phaseKeyFromDir` / `phaseKeyFromProse`), directory membership additionally consults `getMilestonePhaseFilter` when that filter is genuinely version-scoped, and the denominator is the union of the roadmap's declarations with the member directories' keys — so `completed_phases <= denominator` holds by construction. The Builder asserts it and throws; the `Math.min` cap survives only on the legacy unscoped path, where the denominator is a heading count that cannot bound the numerator. An unattributable row degrades over-inclusively (kept, never dropped), matching the degrade direction roadmap-parser already commits to. * test(#2562): boundary coverage for each milestone-scoping reproduction One test per way the scoping could still report "milestone complete"/100% while phases are incomplete: zero-padded rows vs padded dirs (and the mirror), project-code-prefixed dirs, a blank/malformed Milestone cell, a checkmarked bullet naming the version, a shipped `v2.0.1` heading against a current `v2.0`, and a stale same-numbered directory. Plus the current milestone's own shipped heading (the signal must survive the boundary fix), the Builder's numerator-above-denominator throw, a parity check that every non-`passed` verifier status blocks completeness, and a guard that scoping reads the workstream's ROADMAP rather than the project root's. Reverting only `src/` reddens six of them. * docs(#2562): record the milestone-scoped semantics and its consumer impact CONTEXT.md: the Workstream Inventory Module's completion fields now describe the current milestone, not the workstream's lifetime; phase-id owns the canonical phase-key surface; roadmap-parser owns milestone shipped/active classification and takes an optional workstream scope. Changeset: name the behaviour change explicitly — `roadmap_phase_count`, `completed_phases` and `progress_percent` change meaning with no schema signal, and `getOtherActiveWorkstreamInventories` filters on the derived status, so consumers see real movement. * fix(#2562): collapse every zero-padding spelling to one phase key A property test over the key surface — table cell and directory decorated INDEPENDENTLY, which is the point — found a divergence neither review named: `padStart(2, '0')` is a no-op once the input is already ≥2 characters, so `5` normalised to `05` while `005` stayed `005`. A `| 5. … |` row and a `005-slug` directory therefore never compared equal, which is the same failure mode as the padded-vs-unpadded blocker, one level down. The strip belongs in `phaseKeyFromToken`, not in `normalizePhaseName`: applying it to the latter regressed multi-decimal leading-zero plan IDs (`001.10-PLAN.md` capture + wave assignment), which rely on its verbatim rendering. Confining it to the key surface fixes the comparison and leaves rendering untouched. Also tightens `isMilestoneShippedInRoadmap`'s patterns to anchored, complementary character classes so an untrusted ROADMAP cannot drive backtracking, and makes the project-code test discriminating — it previously passed pre-fix, because an unresolvable key collapsed scoping to the whole roadmap and happened to land on the same number. It now carries a prior-milestone directory that a collapse would wrongly admit. * fix(#2562): prefer the milestone-attributing Progress table; pin the seams Three gaps the earlier self-check missed: - Both RoadmapProgress variants carry a `Plans Complete` column, so probing it first picked a FLAT table appearing earlier in the document over the milestone-grouped one that actually carries the attribution. Every row came back unattributed, was treated as current-milestone, and silently re-admitted prior-milestone phases. The attributing shape is probed first; flipping the order reddens the new test. - `lint-phase-id-drift` exempts phase-id.cts by design, so it is silent on `phaseKeyFromToken`'s own segment strip by construction — not evidence. Its interaction with `stripProjectCodePrefix` (which runs AFTER) is pinned across project codes and hyphenated ids, including the pre-existing `M1-46-6` vs `M1-46-6-rs` asymmetry, which is `extractPhaseToken`'s #2043/#2232 slug-word rule and not something to "fix" by accident. - `listWorkstreamInventories` loops every workstream with no try/catch, so a REACHABLE Builder-invariant throw would take down `workstream list`/`status`/ `progress` for all of them. The invariant test only exercised the pure Builder with hand-built inputs. A test now drives `inspectWorkstream` over every adversarial shape at once (prior-milestone dirs, three colliding duplicates, a dirless declaration, an unattributed row, a project-code prefix, a dir-only sub-phase) and asserts it does not throw and the invariant holds — so the throw stays a contract assertion for external callers, not a runtime path. Also covers the active-marker-wins rule (`## v2.0 — 🚧 IN PROGRESS … ✅` must not mark shipped), which nothing exercised. * fix(#2562): scope a declared-but-empty current milestone instead of falling back to history The review's open MAJOR. `STATE.md`'s `milestone:` field updates the moment `/gsd-new-milestone` writes the heading, while the `## Progress` table and phase sections land later. In that window nothing attributes a phase to the current milestone, `scoped` went false, and the fallback counted the project's ENTIRE phase history as both numerator and denominator — a milestone with zero work done reported 100% off its predecessors'. That is #2562's own symptom reached by a different precondition, and none of the 16 tests covered it. Reproduced first, four ROADMAP shapes, at `inspectWorkstream` rather than the Builder — the Builder takes the scoping decision as an input, so a Builder-level test proves it honours a flag, not that the derivation sets it. Three of the four reported 2/2 100% with no phase of the current milestone begun. Which signal witnesses the state depends on the ROADMAP's shape, and no single one covers all three: - `## v3.0` exists but declares no phases. `getMilestonePhaseFilter` DOES locate the section and sets `versionScoped`, then the zero-phase pass-all degrade resets it to false — erasing the only evidence the milestone exists. Neither existing flag survives that path, so this adds `versionSectionFound`, set beside `versionScoped` and deliberately preserved through the degrade. - No section for this version at all, in a roadmap that versions its others — the existing `missingExplicitVersion`, already exposed and tested. - Unversioned headings, but a Progress table attributing every row elsewhere: neither filter flag fires and the table is the only witness. A ROADMAP that attributes NO versions anywhere matches none of them, which is the point. Its rows parse with `version: null`, land in `currentMilestoneKeys`, and never reach the new branch. `readCurrentMilestoneVersion` returns a non-null version for very nearly every project (`getMilestoneInfo` defaults to `v1.0`), so keying off `currentVersion` alone would have zeroed out every free-form legacy project — the condition looks fussy for that reason. A test pins it. Within an empty milestone, membership inverts: a directory belongs unless another milestone's row claims it. Excluding everything would have dropped a phase scaffolded before the roadmap caught up from BOTH sides of the rollup, and hiding real work is the same class of defect as inventing it — this codebase degrades over-inclusive, never under. Scoping is now stated by the caller (`milestoneScoped`) rather than inferred from `currentMilestonePhaseCount > 0`. That inference was the root cause: it cannot represent a milestone that is scoped AND legitimately zero-phase, so the Builder read "no phases yet" as "no scoping" and reopened the whole-history path. The count-derived value stays the default for callers that say nothing. A regression test also pins that a zero denominator does not trip the Builder's `completed_phases <= denominator` throw, since `listWorkstreamInventories` has no try/catch and a crash on every freshly-declared milestone would be worse than a wrong percentage. The changeset and CONTEXT.md no longer claim membership is derived in "ONE" / "a SINGLE" phase-key space. `getMilestonePhaseFilter` still runs its own `normalizePhaseIdSegments`; the signals are OR'd so a divergence can only widen membership, but two normalisers coexist and the docs now say so. * fix(#2562): cross-validate the shipped marker against the milestone's artifacts `status: "milestone complete"` was asserted from the shipped marker alone, so a single payload could report it beside `progress_percent: 67` — this issue's own symptom, reached through `status` rather than the percentage. The marker is now a claim checked against the milestone's own artifacts, and the two signals are checked at DIFFERENT strengths because one check cannot serve both. A `heading` marker (operator-typed, live ROADMAP) is refused on a short completion ratio, which also catches phases declared but never scaffolded. A `snapshot` marker is NOT ratio-gated: `milestone complete` moves the milestone's phase dirs into `milestones/<version>-phases/` (milestone.cts:755-762) while copying — never truncating — the live ROADMAP (:671-674), so a correctly archived milestone reads 0/N by construction and a ratio gate would strip `milestone complete` from every archived milestone. It is refused instead when an in-milestone phase dir is still live and unfinished, reachable because `milestone complete` does not advance STATE's `milestone:` field (state-transition.cts:1335 vs :1224). `legacy` stays ungated — only reachable when scoping is off. A refused marker does not fall through to a STATE field claiming the same thing; against contradicting artifacts neither source may report completion. The refusal surfaces as `milestone_shipped_unverified` rather than staying silent, distinct from `status_conflict` (derived-vs-field only). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * test(#2562): pin both marker strengths, the archived guard, and the owner modules workstream-inventory: four tests, all four red against the prior src and green with it. A live-ROADMAP SHIPPED heading over an incomplete milestone is refused; an archived snapshot SURVIVES its phase dirs being moved out (the regression the obvious single ratio-gate would cause — swapping the snapshot branch to that gate reddens this AND the pre-existing `CURRENT-version snapshot marks the milestone complete` at :321); an archived snapshot is refused once a phase is reopened under it; and a refused marker is not re-asserted by a STATE field claiming the same. roadmap-parser: `isMilestoneShippedInRoadmap` gets unit coverage at its owner module rather than only through the inventory that consumes it, plus two characterisation tests for `getMilestonePhaseFilter`'s legacy call surface — omitting the new trailing `ws` param is indistinguishable from `undefined`/`null`, and the `GSD_WORKSTREAM` env fallback still resolves. These characterise the call surface; they do not stand in for coverage of its individual callers. phase-id: the `phaseKeyFrom*` / `parentPhaseKey` one-key-space contract, incl. a property that padding a directory number never changes its key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * docs(#2562): record the two-strength shipped cross-check + milestone_shipped_unverified Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * fix(#2562): refuse an archived snapshot on a DIRTY archive, not just a live-unfinished dir The snapshot arm checked `liveIncompletePhases > 0`, which misses the shape @davesienkowski reproduced: a COMPLETE live dir beside a phase declared in the Progress table with no directory. Nothing is live-and-unfinished, the marker sails through, and `cmdWorkstreamProgress` returns `{"status":"milestone complete","progress_percent":50}` — the reported symptom verbatim, from one payload. Reproduced at 483a3ba30 before changing anything. His diagnosis is the right one and better than mine: an in-milestone directory outliving the archive means the archive is not CLEAN, and once that is true the completion ratio is meaningful again. So the check is the conjunction — any live in-milestone dir AND `completedPhases < effectivePhaseCount`. That strictly subsumes the old predicate (an incomplete member dir is in the denominator and not the numerator, so the ratio is always short when one exists) and leaves the clean-archive guard green, since a clean archive has no live dirs at all. Also corrects the module comment: the `scoped &&` prefix ungates all three signals, not just `legacy`. That is correct behaviour — unscoped, the denominator is the whole-roadmap count and membership is everything, so there is no current-milestone artifact set to check a current-milestone claim against — but the comment claimed otherwise. And the `milestone.cts` citations were ~28 lines stale after the rebase; they are now :700-702 (copy) and :783-790 (move), re-verified against this head. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * fix(#2562): project milestone_shipped_unverified from list, status and progress The inventory carried the field and every renderer dropped it — `workstream.cts` was not in this PR's diff at all — so at the CLI a refused marker looked exactly like no marker: a fallback `status` and nothing saying one was seen and rejected. That is the silent collapse this issue is about, reintroduced one layer up, and it made the changeset's "visible rather than silent" claim false at every surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * test(#2562): pin the dirty-archive shape and the CLI projection Five tests, all five red against the prior src and green with it. The reviewer's repro at the builder: an archived snapshot with a COMPLETE live dir beside a dirless declared phase must be refused, and status must not contradict the percentage. Four at the CLI via runGsdTools, the surface that was dropping the field rather than the builder that already had it: `workstream progress`/`status`/`list` each project `milestone_shipped_unverified: true` for that workstream, and a clean archive still reports `false` with `status: "milestone complete"`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd * docs(#2562): correct the snapshot check, the scoped-only caveat and the CLI claim Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PpFzuEHKTN1jaypSN44rzd --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
7e9ce08e2c |
test(#2736): property-test parsePhaseFromProse name precedence (#2878)
#2821 reworked parsePhaseFromProse's NAME precedence (dash-vs-paren choice, status-keyword tails, `Milestone:` tails, paren-stripped separator search, the lone-ALL-CAPS-tail rule) but shipped it pinned only by hand-picked examples. The two pre-existing property blocks cover phase-token ANCHORING (#2111) and "N of M" phase extraction; neither touches name precedence. This adds nine properties over the canonical parser. P1 and P9 are the delta guards: both fail against the pre-#2821 paren-first parser (proved with a standalone mutation harness), because each requires a genuine em-dash name to win over a co-present parenthetical. P9 additionally exercises the paren-stripped separator search, since the losing parenthetical itself contains an em-dash. P2-P4 are characterization tests for precedence contracts both parser versions satisfy; P5-P8 pin totality and phase-token extraction, which #2821 left alone. The section comment states which is which so a future reader does not mistake the characterization tests for delta guards. The generator's status-word exclusion filter is a test-local mirror of the private, unexported STATUSY_TAIL_RE. A divergence-guard test pins that mirror to observable parser behavior (not source text, which the no-source-grep rule forbids), so an implementation vocabulary change fails loudly instead of silently weakening every property that depends on the filter. Also clears six pre-existing no-unused-vars lint warnings surfaced by the lint run for this change (dead bindings in four unrelated test files, deleted rather than underscore-renamed); removing the never-called openCodeBlock helper cascaded to its now-unused fs/path/reviewPath consts in both fold scopes. Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
0xdhx
|
82ca13f5a5 |
fix(#2736): write current_phase_name from the transition intent, not the lossy prose round-trip (#2821)
* fix(#2736): intent-first current_phase_name on transitions; dash-first prose precedence Primary: completePhase (adapter) and beginPhase (via readModifyWriteStateMd options) pass the intent-held display name to syncStateFrontmatter as an authoritative override, applied after every derive/preserve/carry-forward step — so the lossy prose round-trip can never destroy a name the transition just resolved. Names containing a parenthetical (`Closer-ruling measurement (D1a)`) now land in frontmatter verbatim instead of collapsing to the parenthetical (`D1a`). Secondary (#1695 AC #3 residual): parsePhaseFromProse prefers the em-dash name when it is a genuine name (not a status keyword, not a `Milestone:` tail), else falls back to the parenthetical — satisfying both first-party writer shapes (`N — Name (aside)` and `N (Name) — EXECUTING`). Still lossy for paren-containing names, which is why the intent-first override is the primary fix. plannedPhase carries no name in its intent, so it is naturally out of scope. Fixes #2736 * docs(changeset): backfill PR number for #2736 fragment * fix(#2736): drop an unnecessary type assertion on result.data StateTransitionResult.data is already `Record<string, unknown> | undefined`, so the cast was a no-op and tripped @typescript-eslint/no-unnecessary-type- assertion (CI lint-tests red on the first push; every test lane was green). --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
BeeHiggs
|
bf9fe4630d |
feat(#2249): bracket phase-id core grammar — parse/render/toDir round-trip pair (epic #612 PR-1) (#2258)
* feat(#2249): bracket phase-id core grammar — parse/render/toDir + READING-B + guards PR-1 of epic #612 (ADR-612, in-tree at docs/adr/612-bracket-phase-id-convention.md). Adds the bracket-convention grammar INSIDE src/phase-id.cts — the ADR-2121 single canonical owner — as a pure, additive extension. The 17 locked exports and PHASE_NUMBER_TOKEN_SOURCE are untouched, and normalizePhaseName is byte-identical, so the PR-0 collision anchor (tests/adr-612-collision-characterization.test.cjs) stays green. New pure round-trippable model (ADR Decision 4): - PhaseId { project, milestone, phase, subphase?, plan? }. - parsePhaseId(input): accepts display `[GSD.02] 05.03-01`, dir/token `GSD.02-05.03-slug`, or bare `GSD.02-05`; rejects ambiguous non-bracket tokens (`02-04`, `05`) rather than guessing. The rejection lives ONLY in this new parser — normalizePhaseName and every legacy reader keep accepting those tokens unchanged (conservative default; no existing path gains a throw). - renderPhaseId(id) -> `[GSD.02] 05.03-01`; toDir(id, slug) -> `GSD.02-05.03-slug` with a slug guard that sanitizes path-traversal input. - getMilestoneFromPhaseId(phaseId, convention?): READING-B derives the milestone from the `[PROJECT.MM]` prefix, gated on convention === 'bracket' and returning the `vN.0` form (parity with READING-A). The optional parameter keeps the helper pure (no config read) and byte-compatible — every existing single-arg caller resolves to the unchanged READING-A body (ADR Decision 6). - extractPhaseToken(dirName, convention?): bracket dir branch GATED on convention === 'bracket'. A bracket dir `{CODE}.{MM}-{PP}` is string-indistinguishable from the legacy #2043/#1324 letter-prefixed-decimal family (`P0.3-2`, `P0.12-34`) whenever the code ends in a digit, so no string-only discriminator is complete — an ungated auto-detect silently reinterpreted legacy reads on this CRITICAL 6-caller helper. The explicit convention signal keeps every existing convention-less call site byte-identical (pinned by a #2043 numeric-tail characterization in tests/phase-id.test.cjs). - comparator: no new code — comparePhaseNum already orders the dot-decimal `PP[.SS]` tokens extractPhaseToken yields; milestone-qualified ordering is a PR-2 resolution concern (bracketQualifiedKey), not core grammar. - SENTINEL_RANGES / isSentinelPhaseId(phaseId, convention?): {0, 999} non-milestone guard; the bracket-prefix reading is gated the same way (an ungated read called `P0.0-foundation` a sentinel), legacy leading-int form unchanged. - BRACKET_PHASE_TOKEN_SOURCE (dot-or-dash `[.-]` sub-separator; deliberately more permissive than parsePhaseId — a read-tolerance source for PR-2, not the emit grammar) and PHASE_HEADING_PREFIX_SRC exported from the drift-guard-exempt owner so PR-2 builds every bracket read regex from the canonical source and check:phase-id-drift stays green stack-wide. The bracket project code follows the repo's config-validated `[A-Z][A-Z0-9_]*` grammar (not the ADR §1 illustration's `[A-Z]{1,6}`), so every project_code the config permits parses. parsePhaseId has no live callers in PR-1, so this grammar choice is forward-facing for PR-2 with zero PR-1 behavior impact. Tests: tests/adr-612-bracket-grammar.test.cjs (28) — ADR §3 example round-trips, full 5-tuple parse, READING-B (+ legacy-unchanged and sentinel cases), extractPhaseToken bracket ON/OFF, comparator ordering of extracted tokens, sentinel + slug guards, bare-token rejection, exported-source behavioral assertions, and two generative fast-check properties: render∘parse identity over well-formed displays, and the toDir/disk↔display bijection. Plus a #2043 numeric-tail characterization (single- AND multi-digit rows) in tests/phase-id.test.cjs pinning the convention-less reading byte-identical. The compiled gsd-core/bin/lib/phase-id.cjs is gitignored (ADR-457 build-at-publish) and rebuilt by CI, so it is intentionally not committed. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(#2249): changeset fragment for PR #2258 (docs-exempt: internal grammar behind flag) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2249): reject non-canonical phase-id input + harden toDir (review B1/M1-M3) PR-1 CHANGES_REQUESTED follow-up (epic #612, ADR-612 Decision 4). B1 (blocker): parsePhaseId accepted non-canonical input (unpadded numbers, over-padded numbers, multi-space separators, stray whitespace), so render(parse(x)) === x did not hold for every well-formed x as ADR-612 Decision 4 requires. Both branches now enforce canonicality by construction: parse permissively, rebuild the canonical string via the same emit path (renderPhaseId for display, a hand-rebuilt token for dir/token), and throw "parsePhaseId: not canonical" on any mismatch. The .trim() at the parser's entry is removed — the match anchors now reject leading/trailing whitespace outright, folding into the existing "not a bracket phase id" rejection. M1 (major): toDir only ever guarded the slug; project/milestone/phase/ subphase were interpolated unsanitized, so a hand-built PhaseId (a structural, not nominal, type) could smuggle a path-traversal segment onto disk. Every field is now validated against the exact shape parsePhaseId itself would produce before use. M2 (major): a slug that sanitized to empty (e.g. '!!!') left a dangling trailing hyphen in the emitted dir name. toDir now throws in that case. M3 (major): an all-digit slug (e.g. '2026') was string-indistinguishable from the dir-branch's plan tail, so it silently broke the disk<->identity bijection on read-back. toDir now rejects all-digit slugs. Nits: toDir now rejects a non-string slug instead of coercing it to the literal token 'undefined'/'null'; sentinel boundary tests added for milestones 1/998/1000 (SENTINEL_RANGES is the two discrete values {0, 999}, not an inclusive range — these were already correct, now locked by test). Test-first: every new assertion (concrete examples + fast-check mutation property for B1; concrete cases for M1-M3 and the nits) was written and confirmed red before the implementation changes, per repo TDD convention. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(#2249): reformat changeset body to house convention (review Mi2) The fragment added in ab26190a was a plain paragraph — no bold headline, no trailing issue reference. Reformat to the repo's `**Bold headline** — symptom/explanation. (#issue)` body shape (see e.g. .changeset/agile-pandas-dance.md, .changeset/fierce-pumas-gather.md). Uses (#2249), the issue every commit on this branch references, not the PR number already carried in frontmatter (`pr: 2258`) — the changelog serializer appends `(#{pr})` unconditionally, so a body also ending in `(#2258)` would double-render as `(#2258) (#2258)`. Verified the rendered bullet directly via parseFragment + serializeChangelog: it now reads `... (#2249) (#2258)`, matching the dominant convention across the other fragments (frontmatter pr = merged PR, body reference = originating issue). Also moved the docs-exempt marker back before the paragraph -> after it (matching the file's original order): the marker sits on its own line and is stripped before the body is used, but placing it first left a leading blank line in front of the bold headline once reformatted. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test(#2249): widen property generators — 3+-digit numerics + subphase-pad mutation (re-review Minor 1/2) PR-1 re-review follow-up (epic #612, ADR-612 Decision 4). Test-only: closes two property-generator coverage gaps the reviewer flagged; no source change (src/phase-id.cts and gsd-core/bin/lib/phase-id.cjs are byte-unchanged). Minor 1 (3+-digit numerics never exercised): numArb capped at 99, so no property fed a 3+-digit milestone/phase/subphase/plan through parse/render/ toDir despite CANONICAL_NUMERIC_RE's dedicated `[1-9]\d{2,}` branch. Widen numArb to 1–999 so the round-trip and disk↔display bijection properties both span 3-digit widths (pad2 passes ≥3-digit values through un-truncated with no leading zero, so canonicality still holds). Add a concrete regression pinning the reviewer's hand-traced example: '[GSD.100] 05' round-trips, renders, and toDirs to 'GSD.100-05-feature' without truncation. Minor 2 (no subphase-pad mutation): the B1 mutation-rejection property covered milestone/phase pad + whitespace mutations but never a subphase pad. Add unpad-subphase / overpad-subphase to the mutation set and a generated `includeSub` boolean that decides whether the canonical carries a `.SS` (forced in for the subphase mutations so there is always a `.SS` to mutate); non-subphase mutations keep their original no-subphase coverage. Non-vacuity verified against the compiled lib by temporarily probing each widened/new property and confirming it fails: round-trip counterexample ["A",100,1,…] and bijection counterexample ["A",1,100,…,"a"] prove 3-digit tokens are genuinely generated and reach the body; a no-op unpad-subphase mutation trips the mutated===canonical guard (counterexample ["A",1,1,1,false,"unpad-subphase"]), proving the subphase branch is reached with a subphase present. Probes reverted; numRuns unchanged. Gates: tests/adr-612-bracket-grammar.test.cjs 44 pass / 0 fail; `npm run test:unit` 1079 pass / 0 fail; `npm run lint:ci` exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2249): consume the #2232 continuation seam at the bracket token's slug-adjacent position (review Major) BRACKET_PHASE_TOKEN_SOURCE was a sixth continuation-recognition site that re-derived the grammar as an unbounded `\d+` literal instead of consuming PHASE_CONTINUATION_SEGMENT_SOURCE, re-opening the #2232 bug class on the bracket path: a PR-2 reader interpolating it over dir `PROJ.01-14-2026-photos-…` (a slug whose first word is a year) over-collected the token as `01-14-2026` instead of `01-14`. Interpolating the cap verbatim at every position was rejected on evidence: the bracket run is `MM-PP[.SS][-LL]` and only the LAST position is slug-adjacent. The exactly-2 cap at the others would under-collect ids toDir itself emits — `PROJ.02-105-slug` (3-digit phase) reads as `02`, `[GSD.02] 05.100` (3-digit sub-phase) as `05` — because CANONICAL_NUMERIC_RE admits `[1-9]\d{2,}` and `[GSD.100] 05` is a pinned regression. Those positions are delimiter- disambiguated (a required field separator; a dot a slug can never contain), not heuristically recognized, so they have no year collision to defend against. Upstream draws the same line for the same reason: core-utils/phase cap the paired PLAN component while the leading phase component stays unbounded. So the run is now positional rather than a free `(?:[.-]\d+)*` repetition, and each position takes the width its delimiter affords: leading unbounded, dash-1 and dot canonical, and the slug-adjacent dash-2 interpolating the single-owner seam. The accepted trade-off is #2232's policy verbatim: a PLAN ≥100 is out of the token grammar. Also derives CANONICAL_NUMERIC_RE from the new BRACKET_CANONICAL_NUMERIC_SOURCE instead of re-spelling it as a literal, so the emit-side gate and the read-side token source are one rule — the same single-owner discipline this fix is about. Behaviour-identical (the anchors make the source's `(?!\d)` guard redundant). Refs #2249 * test(#2249): pin the bracket/#2232 reconciliation — parity surface 6 + divergence gate + property (review Major) The comment block alone cannot hold the divergence: src/phase-id.cts is exempt from the #2128 drift guard by construction, so lint-phase-id-drift.cjs would not catch the bracket token source drifting from the seam. Per the Generative Fix Divergence rule, the divergence is pinned behaviorally instead. Surface 6 joins the existing #2232 parity gate rather than starting a rival one: the review named the bracket token source "a sixth continuation-recognition site", and continuation-grammar-parity.test.cjs is already the invariant-named home where the five #2043 sites agree with the owner on a shared width corpus. Surface 6 asserts the same contract at the bracket run's slug-adjacent position (`01-14-<seg>-photos-…`, mirroring surface 1 with the extra milestone level), so the bracket path now fails the same gate the other five do. A second block pins the DELIBERATE half — the wider canonical width at the delimiter-disambiguated positions, plus the accepted bound (a plan >=100 is out of the grammar). Without it, "unifying" bracket onto the exactly-2 cap would look like a cleanup rather than a regression. The generative property ties the READ side to the EMIT side metamorphically: for every id toDir can produce, BRACKET_PHASE_TOKEN_SOURCE must collect exactly that id's numeric run — no more, no less. It needed a new arbitrary: the existing slugArb generates one [a-z0-9] word and so can never produce the number-leading slug the collision requires. Probe-falsified, both directions (probes reverted): - reverting the source to the old unbounded `\d+` fails 8: the parity gate reports `"01-14-2026-photos-performance" collected "01-14-2026"` — the review's scenario verbatim — and the property shrinks to ["A",1,1,undefined,"100-a"]. - interpolating the seam at EVERY position (the rejected verbatim option) leaves the repro and parity green but fails the divergence gate `'02' !== '02-105'` and the property at ["A",1,1,100,"100-a"] (3-digit sub-phase), which is the evidence that a verbatim cap under-collects ids toDir emits. Width 2 stays green under both probes — the corpus agrees with the owner exactly where the old and new rules coincide, so the gate discriminates rather than merely mirroring the regex. Refs #2249 * docs(#2249): add the new phase-id exports to the CONTEXT.md glossary bullet (round-4 Major) * test(#2249): pin deterministic grammar boundary cases (re-review m1) PR-1 re-review follow-up (epic #612, ADR-612 Decision 4). Test-only: closes the m1 proof gap — the grammar's bounds were exercised only incidentally through the fast-check domain (1-999, [a-z0-9] slugs). No source change (src/phase-id.cts and gsd-core/bin/lib/phase-id.cjs byte-unchanged). Adds a deterministic boundary block (7 describe groups, +22 tests) pinning the compiled lib's CURRENT behavior — a proof gap, not a behavior gap: - m1.1 numeric-width 99/100/101 at milestone/phase/subphase/plan: parse (display + dir) -> render/toDir round-trip byte-equality. The plan position is identity-symmetric (parse/render accept 99/100/101) but toDir drops it (filename-surface dimension only). - m1.2 read-token width is POSITIONAL: BRACKET_PHASE_TOKEN_SOURCE absorbs 99/100/101 at milestone/phase/subphase (delimiter-disambiguated) but caps the slug-adjacent plan (dash-2) at exactly 2 digits — plan >=100 is out of the token grammar (#2232 seam). Pinned as asymmetry, NOT symmetry. - m1.3 leading-zero 007 -> not-canonical rejection at every position/form. - m1.4 slug abuse: parse DROPS a null-byte/control/unicode/emoji trailing slug (never stored, never mis-read as a plan) and rejects a line terminator; toDir's allow-list sanitizer collapses each to a safe [a-z0-9-] token or rejects sanitize-to-empty. - m1.5 absolute-path slug sanitizes (next to the ../../etc traversal test); an absolute-path project on a hand-built id is rejected by PROJECT_ID_RE; an abs-path string is not a bracket id; an abs-path dir slug is dropped to a clean tuple. - m1.6 whitespace-only -> not-a-bracket-phase-id. - m1.7 very-long input (10k) resolves promptly (ReDoS smoke, behavioral): garbage/partial-prefix throw; a 10k-char slug parses (dropped)/sanitizes. No accept-not-reject case is a src bug: parse never STORES an abusive slug (dropped from the identity tuple) and toDir independently re-sanitizes on emit, so the only slug reaching disk is allow-listed. Plan >=100 accepted by parse is the documented positional design (toDir drops the plan; the read-token caps it) — divergence pinned, not papered over. Probe-falsify: corrupted one assertion in each of the 7 groups (m1.4 both its parse-side and emit-side), ran -> 8 distinct named failures, reverted -> 66/66 green. Confirms every new group executes and can fail. Gates: tests/adr-612-bracket-grammar.test.cjs 66 pass / 0 fail; grammar + continuation-grammar-parity + collision-characterization + phase-id family 175 pass / 0 fail; `npm run lint:ci` exit 0. `npm run test:unit` is green except one pre-existing, unrelated env failure (npm-integrity-gate: a live npm-audit advisory in the production dep tree — reproduces with this change stashed; no package.json/lock change here). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Cody Anderson
|
612fcb00f7 |
fix(#2232): cap phase-token continuation segments at exactly 2 digits (all sites) (#2254)
* fix(#2232): cap phase-token continuation segments at exactly 2 digits (all sites) A phase whose slug's first word is a ≥2-digit number (dir 14-2026-photos-performance, roadmap phase "2026 Photos & Performance" → slug 2026-photos-…) had its phase token over-collected as "14-2026" instead of "14", so every phase-locating verb (init.plan-phase, init.execute-phase, phase-plan-index, state.planned-phase, roadmap.annotate-dependencies) resolved phase_dir=null / plan_count=0 while the directory existed. This is the residual case #2043 explicitly scoped out: its ≥2-digit continuation gate (\d{2,}) distinguishes single-digit slug words but not multi-digit ones (years, counts). The structural distinguisher: getPhaseDirFromPhaseId writes sub-phase and plan continuation segments zero-padded to EXACTLY 2 digits, so a genuine continuation's digit run is exactly 2 — \d{2}(?!\d). The (?!\d) guard caps the run without anchoring what follows, so each call site keeps its own trailing grammar (letter suffixes, dotted sub-phases, boundaries). Shared-source, not hand-synced: the grammar lives once in phase-id.cts as PHASE_CONTINUATION_SEGMENT_SOURCE / isPhaseContinuationSegment (the #2121 single-owner seam), consumed by all five #2043 sites: - phase-id.cts extractPhaseToken (the reported repro) - validate.cts PHASE_TOKEN_FROM_DIR_RE + canonicalPlanStem - roadmap-parser.cts isDirInMilestone numericRe (hyphenated mode) - core-utils.cts + phase.cts extractCanonicalPlanId (paired plan component only — the LEADING phase component keeps unbounded \d{2,}; phase numbers ≥100 are legitimate) Digit-width policy, resolved per triage and locked by boundary tests at 1/2/3/4-digit continuation widths across all sites: sub-phase/plan numbers ≥100 are out of the dir-token grammar. validate.cts phaseDirNameRe's leading \d{2,} is intentionally untouched — it encodes the write-side padding of the leading dir number, not the continuation heuristic, and has no year collision. Fixes #2232 Claude-Session: https://claude.ai/code/session_017KaYUJnfzV3JVVuQnhkcjg * chore(#2232): add changeset for PR #2254 Claude-Session: https://claude.ai/code/session_017KaYUJnfzV3JVVuQnhkcjg * test(#2232): parity gate + fast-check properties for the continuation cap Addresses trek-e's review on PR #2254 (M1, M2, B1). Test-only — the fix itself was verified as a true root-cause fix, so no source changes. M1 — drift/parity enforcement for the new shared constant. scripts/lint-phase-id-drift.cjs guards PHASE_NUMBER_TOKEN_SOURCE only; its TOKEN_DRIFT_RE cannot match a bare \d{2,} re-derivation, so a future edit reintroducing a raw digit-cap at a consuming site would pass lint + CI silently. Extending the lint was rejected: \d{2,} legitimately appears at the intentionally-unbounded LEADING-token sites (validate phaseDirNameRe, core-utils/phase tokenRe), so a textual guard would need sanctions on correct code and would flag by spelling rather than by behaviour. Instead, per the repo's *-parity.test.cjs precedent, added tests/phase-continuation-parity.test.cjs: a shared digit-width corpus (1/2/3/4/5) asserting every consuming surface's notion of "is this segment absorbed" equals isPhaseContinuationSegment(). Covers all five #2043 sites: extractPhaseToken, PHASE_TOKEN_FROM_DIR_RE, canonicalPlanStem, extractCanonicalPlanId (paired component), and roadmap isDirInMilestone (hyphenated mode, on a real ROADMAP fixture). The corpus states the policy independently of the regex, so it fails on divergence rather than mirroring whatever the code does. Failing-first verified: reverting PHASE_TOKEN_FROM_DIR_RE to \d{2,} fails 3 parity tests; reverting the owner constant itself fails 11 across parity + properties + examples. M2 — fast-check properties for the changed parser (4 added to phase-id.test.cjs, following its existing inline fc precedent): - biconditional: a segment is absorbed IFF its digit run is exactly 2 - the owner agrees with observable extraction for every digit run - metamorphic: a write-side getPhaseDirFromPhaseId dir round-trips to its own normalizePhaseName id — ties the cap to the zero-padding convention it mirrors, so a change to the write-side width fails loudly - metamorphic: the round-trip holds when the phase name leads with a year (the #2232 bug itself, generatively) Digit runs are generated as digit strings (not String(int)) so leading-zero forms like "02" — the whole point of the rule — are actually exercised. B1 — GitGuardian red. The session-trailer hypothesis is disproven: the same Claude-Session trailer rides 3 commits now merged to next via #2173, whose GitGuardian check PASSED. GitGuardian's own comment names tests/phase-id.test.cjs:260 — the synthetic dir literal 'M1-14-2026-photos' tripping the generic high-entropy detector. Composed it from parts; the assertion is unchanged, only the source spelling. Refs #2232 Claude-Session: https://claude.ai/code/session_019SkiJk38YWAbmxHrGxEmuU * test(#2232): name the parity gate after the invariant, not the phase module CI caught two failures from the new parity test, both one root cause: lint-test-file-count caps each production module at 2 test files (primary + one integration, per the #3740 consolidation). The file was named phase-continuation-parity.test.cjs, and the linter clusters a test to a production module by name prefix — "phase-*" bound it to src/phase.cts, whose cluster (phase.test.cjs + phase-dependency-levels.test.cjs) was already at the cap, making 3. That tripped the lint-tests job AND the ubuntu-24 unit lane, where tests/lint-test-file-count.test.cjs is a meta-test asserting the linter exits 0 against the real repo. Renamed to continuation-grammar-parity.test.cjs, matching the convention the repo's other cross-cutting parity gates already follow: they are named after the INVARIANT, not a module — capability-precedence-parity, agent-classification-parity, and runtime-launcher-parity all have no corresponding src/*.cts, so they cluster to nothing. The gate tests a grammar shared ACROSS phase-id/validate/core-utils/roadmap-parser rather than the phase module specifically, so the invariant-name is also the semantically correct home. Not allowlisted: a novel offender belongs under the cap, not ratcheted into the exemption list. Content unchanged — same 12 assertions across the same 5 surfaces. Refs #2232 Claude-Session: https://claude.ai/code/session_019SkiJk38YWAbmxHrGxEmuU --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
80923244b1 |
fix(#2124): harden parsePhaseFromProse per orthogonal review (ReDoS + coercion)
Orthogonal security review of the Phase 1 surface found two issues; both fixed
and regression-tested:
- MEDIUM ReDoS: the name-extraction regexes /\(([^)]+)\)/ and
/—\s*([^(\n]+?).../ backtrack O(n^2) on a crafted STATE.md field value with a
long unterminated "(" / "—" run (reviewer measured ~38s at 320k chars).
Length-bound both quantifiers to {1,200} -> linear (320k now ~100ms). A real
phase name is far shorter than the cap.
- LOW: parsePhaseFromProse threw on non-string truthy input, unlike its three
sibling #2121 functions. Coerce via String(value) up front.
The identical ReDoS regexes are copied verbatim from the pre-existing
state.cts:parseProsePhaseField; per the no-defer rule that surfaced defect is
fixed inline there too (Phase 2 / #2125 later supersedes that function by
delegating to the bounded phase-id.cts parser).
Adds a behavioral bound-guard regression test (a >200-char parenthetical is not
extracted) and a non-string-coercion test.
Refs #2124, #2121
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
bf25e0b435 |
chore(#2124): build canonical phase-id.cts surface (Phase 1 of #2121)
Add the ADR-2121-locked canonical functions to src/phase-id.cts. No consumer
behavior changes — Phases 2-4 migrate the divergent call sites against them.
- parsePhaseFromProse: anchored prose parser. A phase is returned only when the
STATE.md "Phase:" field VALUE begins with a phase token, so
"Milestone v0.5 complete" yields { phase: null } instead of "5" (the #2111
root cause: the old unanchored \b(\d+..)\b mined the minor-version digit).
Name extraction (parenthetical / em-dash tail, minus status words) unchanged.
- stripConfiguredProjectCodePrefix / isForeignPrefixedPhaseQuery: config-aware
prefix policy. A foreign prefix (MEM-01 when the configured code is LKML) is
preserved rather than collapsed to a bare numeric phase — the #2104 fix's
canonical home (consumed later, outside this epic's critical path).
- roadmapPhaseLookupSources: moved from roadmap-parser.cts so phase-id.cts is
the single owner of the exact -> numeric -> prefix-tolerant ordering.
roadmap-parser.cts now imports it (behavior-identical); its two now-unused
imports (phaseMarkdownRegexSourceExact, OPTIONAL_PROJECT_CODE_PREFIX_SOURCE)
are dropped.
Tests: subject-named suites in tests/phase-id.test.cjs covering the ADR
boundary set (v0.5, v1.0, MEM-01, AB-29, bare 29, zero-padded 029) plus two
fast-check properties: the #2111 "Milestone vX.Y complete never yields a phase"
invariant and a parse/normalize property.
Extend-never-mutate: the 12 pre-existing phase-id.cts exports are unchanged.
Closes #2124
Refs #2121
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
6efd2b4016 |
fix(#2043): reject single-digit slug word in phase-token extraction (all sites)
A phase whose slug's first word is a bare single digit (dir
`46-6-rs-pipeline-orchestrator`, roadmap phase "6 Rs Pipeline Orchestrator" →
slug `6-rs-…`) had its phase token over-collected as `46-6` instead of `46`, so
`gsd-tools` phase-by-number lookups (init.plan-phase, init.phase-op, and
downstream execute/verify/ship) resolved phase_dir=null / has_context=false.
Root cause: an over-broad "looks numeric" test (`/^\d/`, `\d+(?:-\d+)*`)
classified token segments and could not distinguish a legitimate zero-padded
sub-phase segment (`01`, `02`) from a single-digit slug word (`6`). Zero-padded
phase/sub-phase segments are always ≥2 digits, so requiring ≥2 digits is the
structural distinguisher. Applied consistently across every same-class
implementation the triage identified (fixing extractPhaseToken alone leaves the
health-check and milestone-filter subsystems exposed):
- src/phase-id.cts extractPhaseToken — a pure-numeric leading segment continues
only with ≥2-digit segments; a letter-prefixed milestone id (`M1`) still
admits its single-digit sub-phase (`M1-2`).
- src/validate.cts PHASE_TOKEN_FROM_DIR_RE (W005/W006/W007 health checks) and
canonicalPlanStem (I001 plan/summary pairing).
- src/roadmap-parser.cts isDirInMilestone numericRe (getMilestonePhaseFilter —
highest blast radius: a false negative silently excludes a phase from the
milestone).
- src/core-utils.cts + its verbatim duplicate src/phase.cts extractCanonicalPlanId.
Regression tests added across tests/{phase-id,health-validation,core-utils,
roadmap-parser}.test.cjs using the shared `46-6-rs-…` fixture, asserting the
token resolves to `46` (not `46-6`) while legit multi-segment tokens
(`01-02`, `02-03-04`, `M1-2`, `68-01`, `02-01`) are unchanged. The
roadmap-parser test exercises the real getMilestonePhaseFilter end-to-end.
The residual ≥2-digit-leading-slug case (e.g. `NN-2024-roadmap`) stays out of
scope — subsumed by the #612 / #565 phase-ID convention work, per the issue.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
2dedbdd11c |
fix(#1455): resolve project-code-prefixed roadmap headings
* fix: remove hardcoded phase project-code prefix cap
Centralize project-code prefix stripping/matching and replace fixed {1,6} caps so long codes (for example MANIFOLD-117) resolve across phase, roadmap parser, roadmap upgrade, and validate flows.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix: address review feedback on prefix parsing
- allow project_code prefixes with digits and underscores while preserving milestone parsing
- use shared optional project-code prefix source in phase dir parsing
- extend regression coverage for APP1/APP_1 prefixed phases
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* chore: resolve review nit in phase-id test comment
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix: resolve project-code-prefixed roadmap headings
Make getRoadmapPhaseInternal recover from drifted project-code-prefixed ROADMAP headings while preserving canonical bare-heading preference. Add init.phase-op and parser regressions for #1455 and guard gsd-roadmapper against emitting project_code in headings.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* chore: add changeset for project-code phase fix
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* chore: update roadmapper agent size baseline
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
* fix(#1455): tighten project-code prefix regex to [A-Z] start; add boundary test; document source order
Resolves blockers from review:
- Regex changed from [A-Z_][A-Z0-9_]* to [A-Z][A-Z0-9_]* so leading
underscores (_FOO-7, _-7) are never misread as project-code prefixes;
adds boundary test asserting both do NOT strip.
- Adds comment to roadmapPhaseLookupSources explaining why 3 sources are
needed (order-dependent canonical-heading preference).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Solvely-Colin <211764741+Solvely-Colin@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
c53fd1f654 | fix(#1324): resolve glued phase tokens (#1353) | ||
|
Tom Boucher
|
c76827afbc |
refactor(#1291): T6 — migrate test files off the core spine ahead of deletion (#1293)
The convergence lint only scanned src/ + gsd-core/bin, so ~35 test files still imported core.cjs. Repoint all 33 behaviour importers to the leaf modules directly (same symbol->leaf map as the src migration; leaves are the objects core re-exported by reference), delete the now-meaningless shim-identity describe blocks in the 8 leaf tests, and delete tests/core.test.cjs (forwarded-behaviour coverage now lives at the leaves; resolveWorktreeRoot test relocated to worktree-safety in T0) and tests/lint-core-spine-imports.test.cjs (the lint is removed in T-final). Dropped the stale core.test.cjs entries from the allow-test-rule-refs allowlist; eslint-rules RuleTester fixture path pointed at io.cjs. After T6: ZERO test imports core.cjs. core.cts still builds (now fully unused); T-final deletes it. No behaviour change. Closes #1291 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2988a21c46 |
refactor(#865): extract pure phase-id helpers from core.cts into phase-id.cts (#868)
ADR-857 rollout phase 2a — the first cut of the core.cts decomposition. Move the 9 pure phase-id parsing/matching helpers (escapeRegex, normalizePhaseName, comparePhaseNum, extractPhaseToken, phaseTokenMatches, phaseMarkdownRegexSource/Exact, getMilestoneFromPhaseId, getPhaseDirFromPhaseId) out of core.cts into a new leaf module src/phase-id.cts. core.cts re-exports them (behavior-preserving); its internal callers resolve the destructured bindings. Cycle-safe by design: phase-id depends on nothing in core, so re-export creates no circular require (the property that made phase-1 io.cts clean). This is the leaf-first ordering — it unblocks the roadmap-parser extraction (2b), which imports phaseMarkdownRegexSource. New-CLI-module checklist: .gitignore, eslint.config.mjs ignores, INVENTORY.md count 91->92 + row, INVENTORY-MANIFEST.json, ARCHITECTURE.md row, CONTEXT.md "Phase Id Module" glossary entry. Adds tests/phase-id.test.cjs (63 behavioral tests incl. shim-identity + adversarial inputs). Gates: lint, code-review, security-review, codex adversarial-review (all 0 findings), and gsd-test-both (14814 pass on Mac + Linux Docker, 0 fail). Closes #865 Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |