103 Commits

Author	SHA1	Message	Date
Michel Moreira	26e650f312	fix(#4462): normalize whitespace workstream environment values (#4509) ... * test(#4462): expose whitespace workstream scope * fix(#4462): normalize the workstream environment scope * docs(#4462): add changeset for #4509 * fix(#4462): normalize sibling planning scope readers * test(#4462): cover workstream normalization properties * fix(#4462): share normalized workstream resolution --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-15 00:40:15 -04:00
Tom Boucher	c99d7bb2be	test(#4522): migrate core CLI/domain state batch to named timeout constants (#4662) ... Batch 11 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/state-document.test.cjs, tests/phase.test.cjs, tests/commands.test.cjs, tests/pattern.test.cjs, tests/adr-612-bracket-coherence.test.cjs, tests/adr-612-bracket-read-tolerance.test.cjs, tests/milestone-lock.test.cjs, tests/init.test.cjs, tests/state-todos-render.test.cjs, tests/quick-batch.test.cjs, tests/graphify.test.cjs, and tests/effort-surface-axis.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 12 files from the rule's allowlist. Ground truth via eslint found 25 sites, not the issue's stated 24 (phase.test.cjs has 5, not 4) -- disclosed in the PR body. Reuses PROBE_TIMEOUT_MS, GIT_TIMEOUT_MS, and LOOP_HOOK_POINT_CLI_TIMEOUT_MS across 8 files. Adds two new shared constants to tests/helpers/timeouts.cjs (each independently arrived at by 2 files in this batch, crossing the promotion bar): PATHOLOGICAL_INPUT_TEST_TIMEOUT_MS (node:test's own per-test timeout option, not a subprocess bound) and GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS (a single gsd-tools.cjs CLI subcommand spawn, distinct tier from PROBE_TIMEOUT_MS/LOOP_HOOK_POINT_CLI_TIMEOUT_MS). Adds 3 file-local constants for values used by only 1 file in this batch. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-09-12 09:12:31 -04:00
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>	2026-09-11 12:42:04 -04:00
Tom Boucher	8c8eda46b0	fix(#4225): scope the sibling-worktree phase-number horizon to the active workstream (#4450) ... * test(#4225): failing-first matrix for phase.add --ws workstream-scoped numbering Nine rows driven through the real CLI: the issue's verbatim topology (root roadmap @39 committed, workstream @2, sibling git worktree carrying the root roadmap), same-workstream sibling boundary, empty-workstream first phase, coincidental root-maximum, no---ws control (the #3849 global horizon, byte-for-byte), cross-workstream isolation, sibling lacking the workstream (fail open), add-batch parity, and a next-decimal control. Rows 1/2/3/6/7/8 are RED on next @38e4ce5f62 (numbering computed from the sibling ROOT roadmaps: 40 instead of 3). * fix(#4225): scope the #3849 sibling-worktree widening horizon to the active workstream collectSiblingWorktreePhaseNums scanned each sibling git worktree's ROOT .planning/ (phases/ dirs + ROADMAP.md headers) unconditionally. Under --ws (GSD_WORKSTREAM), every local number source flows through planningDir(cwd) and lands in the workstream scope, but the widening horizon still merged the siblings' ROOT-roadmap numbers into it — so phase.add --ws in a workstream at Phase 2 inside a project whose root roadmap sits at Phase 39 minted Phase 40 (directory 40-<slug>, and a Depends on: Phase 39 that does not exist in the workstream's numbering universe). The horizon now resolves each sibling's planning dir through the SAME canonical resolver, planningDir(wt, ws), with the env workstream read once via planningDir's own discriminator: a workstream-scoped allocation scans the sibling's copy of the SAME workstream (a number taken by that workstream on another branch is still taken — the #3849 widening survives, scoped), and never the sibling's root roadmap or another workstream's. No workstream active: ws is null and the root-scope horizon is byte-for-byte the #3849 behavior. A sibling lacking the workstream directory contributes nothing (fail open, unchanged). phase.add and phase.add-batch share the helper; both scopes of both verbs are covered by the matrix in the previous commit. Output shape and the publishStateContract boundary are untouched — only the number changes. * fix(#4225): rename siblingPlanning -> siblingPlanningDir (review nit) * chore(#4225): changeset fragment (pr number to backfill) * chore(#4225): backfill PR number in changeset --------- Co-authored-by: sim <sim@local>	2026-09-06 22:24:07 -04:00
Tom Boucher	e6d047decc	fix(#4129): derive completed_phases from the ROADMAP authority; honor the progress-ratchet on every state write (#4359) ... * test(#4129): failing-first regressions — completed_phases clobber on resyncing writes and phase-complete failure to increment * fix(#4129): completed_phases derives from the ROADMAP authority and the write path honors the progress-ratchet Three coordinated prongs (diagnosis in .gsd/bug/fix-4129-completed-phases-recompute/): P1 — buildStateFrontmatter's disk scan floors the completed-phases numerator at the milestone-scoped ROADMAP Complete-row count (deriveProgressFromRoadmap, the one owner), gated inside the same safeToUseRoadmapCount / not-withheld branch that owns the denominator. A completed phase whose verification routes stale (#2348 clean-commit-time drift) or is missing no longer under-counts forever. P2 — applyPreserveAlways's resync arm merges instead of wholesale-replacing on a measured scan: totals derived both directions (#2440), completed counters up-only (#2969 — the schema-declared progress-ratchet, now enforced on the write path like the read path always has), percent recomputed from the merged counters. The #3756 unmeasured guard and the #3242 explicit-progress contract are unchanged. P3 — phase complete's atomic 3-file commit passes the post-completion ROADMAP-derived counters through the #2736 authoritativeFm seam (new object direction for the progress key; completedOnlyRaise at the post-preservation re-assert), because the transaction's disk scan reads the pre-completion ROADMAP and failed to increment on the completing phase's own write. * fix(#4129): adversarial-review hardening — intent is a floor at BOTH authoritativeFm sites The pre-preservation merge could lower a correctly-higher disk-derived counter (a verification-passed phase whose ROADMAP table row drifted behind the disk signal). completedOnlyRaise now governs both application sites: the intent and the derivation agree on direction (up), never on subtraction. * fix(#4129): the ratchet merge keeps derived values verbatim when numerically equal The re-parsed derived block carries string scalars ("2") while the curated snapshot carries numbers (2); substituting the curated spelling over an equal derived one was a no-op in substance but a shape churn the ADR-3473 §8.7 reporting loop surfaced as a phantom preserved-over-disagreeing-derived warning on phase complete (ADR-3408 §8.5 Matrix B). Only a strictly-greater curated counter replaces the derived value now; percent gets the same verbatim rule. * changeset(#4129): backfill PR 4359 --------- Co-authored-by: sim <sim@local>	2026-09-05 23:02:07 -04:00
Tom Boucher	02ad0b91f3	fix(#4078): phase.complete next-phase cascade reads dash-grammar checkbox rows (#4301) ... * test(#4078): phase.complete mixed-grammar roadmap picks lowest outstanding phase, not positional-last * fix(#4078): accept dash-grammar checkbox rows in phase.complete next-phase cascade Stage 2 (roadmap identity scan) and stage 3 (#2028 lowest-outstanding override) required a colon separator after the phase number, while the canonical phase lookup has accepted the bullet-house dash grammar (- [ ] **Phase N — Name**, #2199) for years. On a mixed-grammar roadmap the only parseable row above N was a later phase.add-ingested colon-form phase - positionally last - and it won the numeric-minimum vote it should never have been alone in: completing Phase 1 of 18 selected Phase 18 and skipped phases 2-17 (#4078). The checkbox branches now accept the #2199 separator class (em/en-dash, hyphen, colon); heading branches stay colon-only, mirroring findRoadmapPhaseInContent exactly. * test(#4078): align regression fixtures with slug name + checked-box semantics * fix(#4078): drop unnecessary type assertion flagged by eslint * chore(#4078): add changeset fragment * chore(#4078): backfill PR number in changeset --------- Co-authored-by: sim <sim@local>	2026-09-04 22:03:43 -04:00
Tom Boucher	383c2f6b34	fix(#3982): strip closed-milestone details from the current window (#4177) ... * test(#3982): archived details must not leak into the current-milestone window Parser-level regression (newest-first layout, two archived details blocks) plus the issue's end-to-end phase.complete fixture: completing phase 20 must advance to 21, never backwards into the archived range. * fix(#3982): strip closed-milestone details from the current window The heading-located window stripped <details> archives from the preamble but not from currentSection; on newest-first roadmaps the archived titles sit in summary tags rather than headings, so the section walk reached end-of-document and the window swallowed every collapsed archive below the active milestone. The strip is gated on isClosedMilestoneHeading over each block's summary — the issue's prescribed narrow form — so the active milestone's own collapsed blocks (#1341) survive instead of trading this bug for the phase_count: 0 class (#557/#2947). * chore(#3982): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-09-02 02:33:54 -04:00
0xdhx	7c116b1c17	fix(#3697): warn when the phase-complete Requirements-line tokenizer under-selects REQ-IDs (#3744) ... * fix(#3697): warn when the Requirements line under-selects REQ-IDs `cmdPhaseComplete` tokenizes ROADMAP's `**Requirements**:` line by splitting on `[,\s]+` and keeping tokens matching the anchored REQ-ID shape. That is correct for the canonical comma list the template ships, and silently wrong for every other form: `RANGE-01 … RANGE-05` -> the two ENDPOINTS only; the interior IDs are never considered, yet `requirements_updated` reports true with zero warnings `RANGE-01…05` -> ZERO IDs; the whole line is inert The silence is structural: the only cross-check, `ghostReqIds`, is itself `citedReqIds.filter(...)`, so an ID the tokenizer dropped is invisible to it by construction — and to `traceabilityWriteMisses` and `requirements_updated` with it. Warn on both paths. This does not add range support: the selected set is unchanged, so no existing ledger write changes. The trigger is ID-SHAPED EVIDENCE only — an ID-shaped substring the tokenizer did not select, or a range operator joining two IDs — with parenthetical citations and HTML comments stripped before the scan, so the #2334/#2339 over-warning on `None`, on the shipped `<!-- brackets optional -->` template comment, and on annotated lines cannot return. Regression tests extend the #2316/#2334 fixture family in tests/phase.test.cjs (10 cases: 4 defect, 2 canonical controls, 4 negative-space controls). Fixes #3697 * fix(#3697): rework under-selection detection onto tokens, not a free-text scan Round 2, driven by the P4.6 cross-AI review (codex, gpt-5.6-sol) of b3ce71cb. That review refuted 5 of 9 claims; three were false-positive classes in exactly the category #2334/#2339 had to REMOVE: `RANGE-01, RANGE-02 - 3 points` the bare-hyphen alternative read `RANGE-02 - 3` as a range `REQ-01, REQ-02 — locked per ADR-7.` the trailing period kept `ADR-7.` out of the anchored filter, so the unanchored substring scan reported it as unparsed `REQ-01, REQ-02 (see (ADR-7), then ADR-8)` nested parens left `ADR-8)` behind Replaces the free-text substring scan + loose range regex with three narrow, token-based rules (R1 range-shaped token, R2 pure range operator flanked by two selected IDs, R3 zero-selection with ID-shaped text). Also fixes the review's CLAIM 9: the warning said IDs were "marked complete" when a ghost range marks nothing — it now says "selected". Side effect: the two false NEGATIVES the same review found are now covered — `RANGE-01 through RANGE-05` and a parenthesised `(plus RANGE-02..RANGE-05)`. NOT YET DONE (see the handoff prompt): regression tests for the four false positives, the two new true positives, and the #3697-4 tightening the review's CLAIM 8 asked for (it currently filters on the warning's phrasing rather than asserting silence). Verified so far: tsc clean, the 10 existing #3697 tests green, and a 20-case standalone harness covering every case above. * test(#3697): pin the v2 token-detector boundary end-to-end Six new cases + two hardenings for the review findings against v1: - #3697-1 gains the worded spaced range (`RANGE-01 through RANGE-05`) — the operator set's `to|thru|through` arm was previously untested. - #3697-5 (new): a tight range hidden inside balanced parentheses (`RANGE-01 (plus RANGE-02..RANGE-05)`) warns, names the range token, and ticks exactly RANGE-01 — the paren shave must not hide it. - #3697-4 gains the four false-positive classes a free-text detector produced: numeric estimate (`- 3 points`), date annotation, em-dash citation with trailing period (`— locked per ADR-7.`), and nested parenthetical citations. - #3697-3 and #3697-4 now assert the ENTIRE warnings channel is empty, not that one phrase is absent — a re-worded over-warning cannot pass. Negative control: against the merge-base with its lib rebuilt, all 6 defect tests fail and all 10 controls pass. * fix(#3697): close round-2 review findings — annotation false positives Round 2 of the adversarial review (against 822a72a04) refuted five claims; this closes the false-positive class and the cheap misses: - R1's bare-hyphen arm now demands a full ID on BOTH sides (`REQ-01-REQ-05`): `LETTERS-\d+-\d+` is also a date-like annotation (`FY-2026-08`) and a sub-numbered ID, and warning on those is the expensive class. Tight hyphen shorthand with a live selection is the disclosed false negative; at zero selection R3 still catches it. - R2 requires the endpoint pair to imply an INTERIOR (same prefix, gap > 1): `REQ-02 - REQ-03` selects both endpoints and can drop nothing, so an annotation hyphen between adjacent IDs stays silent. - R3 skips placeholder-led lines: `None (per ADR-7)` is a declared-empty line citing its rationale, not unparsed residue. - Token shave: quotes/backticks now shaved from alphanumeric tokens (`` `RANGE-02..RANGE-05` `` warns); punctuation-only tokens get a bracket-only shave so `(..)` surfaces its operator. - 256-char token cap bounds the quadratic unanchored substring test. - Warning text mentions range expansion only when a range rule fired. Tests: 6 new cases (22 total). Negative control against the merge-base: 8 defect tests fail, 14 controls pass. * fix(#3697): close round-3 review findings — half-spaced ranges, cross-prefix annotations, markdown wrappers Round 3 of the adversarial review (against 2eb92dd0e) refuted four claims; this closes them: - Half-spaced ranges (`REQ-01 -REQ-05`, `REQ-01- REQ-05`) split at the tokenizer before R1's `\s*` can see them and under-selected silently. A glued-fragment rule warns when an operator is glued to a full ID with an ID-shaped neighbour on the open side and the endpoint pair implies an interior. - Cross-prefix pairs around a separator no longer read as ranges: `REQ-02 - (ADR-7)` and `REQ-02 (...) (ADR-7)` are annotations, and real ranges are same-prefix by nature. `impliesInterior` now returns false on prefix mismatch and computes the gap with BigInt (parseInt lost precision past 2^53). - The token shave now removes markdown emphasis markers and curly quotes, so `**None** (per ADR-7)` reaches the placeholder gate and `**RANGE-02..RANGE-05**` reaches R1. - The unanchored-substring cap rises to 2048 (a markdown-link range with a long URL cleared 256); the anchored range regexes scan linearly and drop their cap. Tests: 6 new cases (28 total; 377/377 file-wide). Negative control against the merge-base: 11 defect tests fail, 17 controls pass. * fix(#3697): round-4 review finding — word operators excluded from glued-fragment rule `TOREQ-05` is a valid prefix-agnostic REQ-ID, and the glued-fragment rule read it as `to` + `REQ-05`, warning on the canonical two-ID list `REQ-01, TOREQ-05`. Glued fragments are now SYMBOL-operator-only (`..`+, ellipsis, dashes): a word operator glued to an ID is an ID, not a range spelling. Tests: word-operator-prefixed ID control (misparse channel silent; the fixture's ghost-ID warning legitimately fires, so the whole-channel assertion stays with the registered controls) and an underscore-wrapped tight-range defect case. 30 targeted cases; 379/379 file-wide; negative control: 12 defect tests fail on the merge-base, 18 controls pass. * fix(#3697): round-5 review findings — trailing word-op glue, dot shave, honest wording - The glued-fragment TRAILING arm takes the word operators back: an ID must end in digits, so `REQ-01through` can never be an ID — the round-4 TOREQ collision was leading-arm-only, and symbol-only on both arms lost the `REQ-01through REQ-05` typo class. - A trailing run of 2+ dots survives the punctuation shave: `REQ-01..` is a glued range operator, not sentence punctuation, and the shave was silently eating the `REQ-01.. REQ-05` form. - The warning now says the line "could not be parsed as" a comma-separated REQ-ID list: `**REQ-01**, **REQ-05**` IS such a list — the selector just cannot parse decorated tokens — and a warning that misstates the input teaches readers to distrust it. Tests: two new trailing-glue defect cases (32 targeted; 381/381 file-wide). Negative control: 14 defect tests fail on the merge-base, 18 controls pass. * test(#3697): use t.after for cleanup per CONTRIBUTING test ruleset CONTRIBUTING bans try/finally inside test bodies (it masks failures); the approved shape is `t.after(() => cleanup(tmpDir))`. All seven converted tests are this PR's own additions; the file's pre-existing instances are untouched. * chore(#3697): add changeset fragment for the Requirements-line under-selection warning changeset-lint fails on this PR (fail_missing_fragment): src/phase.cts is a user-facing surface and the branch carried no .changeset/*.md. Adds the Fixed fragment via `npm run changeset -- --type Fixed --pr 3744`, symptom-led per the house format, with the (#3697) backlink. * refactor(#3697): extract the Requirements-line detector to a testable surface Round-3 review Blocker 1 requires a fast-check property test over this detector (`RULESET.TESTS.property-based-testing`: modules implementing parsing contracts must include at least one), and Blocker 2 requires limit-1/limit/limit+1 fixtures on its 2048-char token cap (`RULESET.TESTS.boundary-coverage.fixtures`). Neither is expressible while the logic is a closure inside `cmdPhaseComplete`: every existing #3697 test reaches it by spawning the CLI, and a property test cannot pay a subprocess per generated case. So the selector and the three detection rules move to module scope as `analyzeRequirementsLine` (pure, exported) plus `formatRequirementsLineWarning`, and `cmdPhaseComplete` calls them. This commit changes NO behaviour: `tests/phase.test.cjs` is untouched here, and the pre-round suite passes against it unmodified (403/403). Two things the move makes explicit rather than incidental. The selector and the detector tokenize the SAME line DIFFERENTLY — the selector strips only `[` and `]`, the detector also shaves quotes, emphasis and trailing sentence punctuation — and that gap is deliberate: it is why `ADR-7)` is not selected while `ADR-7` is still nameable in a warning. They now sit adjacent with the reason written down, so they cannot drift apart silently. And the stale citations in the moved comment are corrected. It pointed at src/phase.cts:833,920,1078 for the `**Requirements**: TBD` seeds, which had drifted to 1132/1237/1413, and at `templates/roadmap.md:32`, which is `gsd-core/templates/roadmap.md:32`. Both are now anchored by content. * fix(#3697): stop the warning claiming a misparse that did not happen Round-3 review Major 3 and Minor 4. Both are the same defect: the warning asserted more than the evidence supported. MAJOR 3 — a correct comma list such as `RANGE-01, RANGE-02 — RANGE-05 deferred` warned "could not be parsed ... Range forms are not expanded; rewrite the line". Reproduced: it selects RANGE-01, RANGE-02 AND RANGE-05, i.e. every ID written on the line. Nothing was dropped, and the pinned control only stayed silent because its pair was ADJACENT (gap == 1), so the control was passing by accident of the fixture rather than by the rule. The obvious fix — go silent — is not available. `RANGE-02 — RANGE-05` as a range and as an annotation separator are textually identical, and no token-level rule separates them; staying quiet re-opens the exact silent under-selection #3697 is about. Deciding the ambiguity by assertion in either direction is wrong. So it is DISCLOSED: the warning now has two channels, chosen by whether any ID-shaped token was actually left unselected (`droppedIdShaped`). * something was dropped (tight range, glued fragment, inert residue) -> "could not be parsed as a comma-separated REQ-ID list", as before. * nothing was dropped (only the spaced-operator rule fired) -> "contains what reads as a range between two cited REQ-IDs", stating both readings and saying explicitly that an annotation separator means the line is already correct. This retires the "could not be parsed" wording for the four #3697-1 spaced cases too, and that is a deliberate expectation change rather than a fix counted twice: those lines never failed to parse either. They still warn, still name the selected IDs, and still assert the endpoint-only marking is unchanged; #3697-1 now also asserts the misparse channel stays SILENT. MINOR 4 — `Deferred (see ADR-7)` reported `Unparsed text: ADR-7`, naming a citation as requirement content it had failed to read. The trigger is correct and stays: #3697's acceptance criterion asks for a warning "when it selects zero IDs from a line that is non-empty and is not the `TBD` placeholder", and inferring placeholder-ness from arbitrary prose is the free-text heuristic this detector exists to avoid. What was wrong is the wording, so the non-range arm now says "ID-shaped text that was not selected" and names the escape the author actually has (`TBD` / `None`). Tests: #3697-9 (three spaced forms — must warn, must NOT claim a misparse, must offer both readings) and #3697-10 (`Deferred (see ADR-7)`, `N/A (tracked in ADR-12)` — must warn, must not say "Unparsed text", must not diagnose a range, must name the placeholder escape). Reversion control: reverting the ambiguous channel fails #3697-9 (3 named tests); reverting the R3 wording fails #3697-10 (2 named tests). * fix(#3697): cap every token predicate, complete the dash set, cover the boundary Round-3 review Blocker 2 and Nit 6, plus one self-found finding. All three are about the detector's own predicates, so they land together. BLOCKER 2 — the 2048-char budget had no boundary coverage. `RULESET.TESTS.boundary-coverage.fixtures` requires limit-1 / limit / limit+1 for any budget parameter. #3697-B1 and #3697-B2 now exercise 2047 / 2048 / 2049 against BOTH predicate families the cap guards, and each asserts its fixture's exact length before asserting behaviour, so a mis-built fixture fails loudly rather than passing at the wrong size. Clause (d) of that rule — an input pushed within reserve-distance of the limit — has no referent here: this is a hard cap with no reserve constant beside it, and the test comment says so rather than leaving the omission to be re-derived. NIT 6 — the cap guarded only the unanchored ID-substring regex. The anchored range regexes were left uncapped, justified by a comment asserting they scan linearly. The finding is right that this is informational (they are anchored; the input is a local ROADMAP.md), but an asserted property is cheaper to enforce than to defend, so all three predicates now share one `short()` guard. #3697-B2 is what pins it: at 2049 the anchored scan must now decline to classify. SELF-FOUND (RV4 guard-shape census) — the range-operator set is a list this code fixes at author time over a domain that grows without it, so the round owes a census of what the enumeration reaches. reached: `..`+, U+2026, U+2013, U+2014, ASCII `-`, to/thru/through NOT reached: U+2010 hyphen, U+2011 non-breaking hyphen, U+2012 figure dash, U+2015 horizontal bar, U+2212 minus sign consequence: a range spelled with any of those is SILENTLY under-selected — #3697's own defect, in the code that exists to fix it Those five close. They are the same operator at a different codepoint and carry none of the ASCII hyphen's collision risk, because they are not the REQ-ID separator: `FY-2026-08` is date-shaped only with ASCII hyphens, so a U+2010 never reaches the ID shape. They therefore join the NOHYPHEN arm beside `—` and `–`; the strict full-ID-both-sides shape the bare hyphen is held to is untouched, and #3697-12 pins that. Still NOT reached, declined with reason rather than left unstated: `→`, `~`, `..=`, `..<`, `until`, and `up to` (two tokens, so never one operator token). Each is a symbol or word with an independent non-range use between two REQ-IDs — the over-warning class #2334 cost three rounds. Reversion control: reverting the uniform cap fails #3697-B2 (limit+1); reverting the dash set fails #3697-11 (5 named tests). * test(#3697): add the fast-check property coverage the parser rule requires Round-3 review Blocker 1. `RULESET.TESTS.property-based-testing` (CONTEXT.md) requires modules implementing parsing contracts to carry at least one fast-check property test asserting a domain invariant, and the round-2 diff had zero occurrences of `fc.` across its +311 test lines. Five properties, 1,900 generated cases: P1 soundness of silence (boundary containment) — for ANY canonical comma list of well-formed REQ-IDs, the selected set EQUALS the written set and nothing warns. This is the #2334 over-warning invariant and the #3697 under-warning invariant asserted as one statement, over generated IDs rather than hand-picked ones. It generalises #3697-4b: a prefix beginning with a word operator (`TORANGE-05`) is an ID, and P1 covers that class rather than the single example. P2 completeness — a same-prefix pair with an interior between them, separated by any of the nine spaced operators, ALWAYS warns. P3 the #2334 invariant — an ADJACENT pair around a separator can drop nothing, so it stays silent however it is annotated. P4 totality + idempotency — total over arbitrary strings, deterministic, and the formatter agrees with the analysis on whether there is anything to say (a warn with no text, or text with no warn, is a channel that can go silent or noisy on its own). P5 containment — every selected ID is ID-shaped and appears verbatim in the input. Honest scoping, since a property test is easy to overclaim: P1, P3, P4 and P5 hold against the round-2 code as well as this one — they are regression guards, not bug-finders, and their value is that the invariants are now stated and generatively checked rather than implied by examples. P2 is the one that would have failed before the dash enumeration was completed. fast-check v4 removed `fc.stringOf`, so the ID-prefix tail is built from `fc.array(...).map(join)` with the alphabet pinned to the selector's own `[A-Z0-9]` class. These live in tests/phase.test.cjs rather than a new `phase.property.test.cjs`: `lint-test-file-count` caps a production module at 2 test files and phase.cts is already at its allowlisted entry, so a new file would trade one gate for another. * docs(#3697): document the ROADMAP Requirements-line grammar Round-3 review Minor 5 — the change adds net-new user-visible warning output for a grammar constraint documented nowhere under docs/. `type: Fixed` is docs-exempt so this does not block, but a warning about a rule the reader cannot look up is not actionable, and that is worth fixing whether or not a gate demands it. Added as a subsection of `phase complete` in docs/CLI-TOOLS.md, beside the existing SUMMARY artifact-check advisory it is a sibling of: the supported comma-list form, why ranges are deliberately not expanded, that `TBD` and `None` are the entire placeholder vocabulary, and what each of the two warning voices means — including that the range/annotation one may be reporting a line that is already correct. Existing file rather than a new one, deliberately: docs/ carries generated indexes and zh-CN / ja-JP trees, and a new top-level page invites a parity or index gate this change has no reason to touch. * fix(#3697): rule-scope the warning-channel discriminator Self-found at the round's pre-push review, against the Major 3 fix two commits back. That fix chose the channel from a LINE-GLOBAL question — "was any ID-shaped token left unselected?" — while the rules that produce the warning are not line-global. The two disagree as soon as the line carries an ID-shaped token no rule fired on: `RANGE-01, RANGE-02 — RANGE-05 deferred per (ADR-7)` `(ADR-7)` survives the selector's bracket strip, so the global test called it a drop and sent the line to the assertive channel — putting the false "could not be parsed ... rewrite the line" claim back on a correct line. That is review finding Major 3 returning through a side door, and it directly contradicts #3697-4, which pins a parenthetical citation as NOT unparsed residue. The discriminator is now rule-scoped: R2 is the only ambiguous rule, so the ambiguous channel requires that R2 fired, that no other rule did, and that every endpoint R2 fired on was actually selected. The last conjunct is not redundant — the detector shaves brackets and the selector does not, so R2 can fire on a `(RANGE-02)` that was never selected, and that IS a drop: `RANGE-01 (RANGE-02) — RANGE-05` -> assertive, correctly `droppedIdShaped` is replaced by `spacedRangePairs` (R2's hits, so the channel can ask about the endpoints the rule fired on) and the `rangeReadingOnly` verdict. Reversion control: against the line-global rule, #3697-9b fails. #3697-9c passes under both rules — there the dropped token IS the R2 endpoint, so the two agree; it is a regression guard, not a bug-finder, and is recorded as such rather than counted as a second control. * fix(#3697): hold every dash to the strict range shape, not just ASCII Self-found at the round's pre-push review, and it CORRECTS a claim made two commits back. That commit widened the range-operator set by five Unicode dashes and asserted they "carry none of the ASCII hyphen's collision risk, because they are not the REQ-ID separator". That reasoning was wrong. The collision is a property of the SHAPE — `PREFIX-\d+ <dash> \d+` is also a date (`FY-2026-08`) and a sub-numbered ID (`API-2-01`) — and the shape does not care which dash sits in the operator slot, because the ID's own separator is still ASCII either side of it. Measured: RANGE-01 (target FY-2026-08) silent <- pinned by #3697-4 RANGE-01 (target FY-2026‐08) WARNED <- same line, U+2010 So the widening reintroduced the #2334 over-warning class on a date annotation. It also exposed that the inconsistency PREDATES this PR: U+2013 and U+2014 were already in the loose arm at ce71dd399, so the en- and em-dash forms of that same date annotation warned before round 3 ever ran. One rule for every dash: a tight range spelled with any of the eight must carry a FULL ID on both sides, exactly as the bare hyphen already had to. `..`, `…` and the word operators stay loose — no date or sub-number reading exists between two numbers, so the strict shape would cost them coverage for nothing. The cost is a false negative, and it is one the design already accepts: `RANGE-01, RANGE-02-05` is silent today, deliberately, and now `RANGE-01, RANGE-02–05` is too. That removes an inconsistency rather than opening a gap, and a bare `RANGE-02–05` still warns — it selects nothing, so R3 catches it. Tests: #3697-13 (date annotation AND sub-numbered ID silent for all eight dashes), #3697-13b (full-ID tight range still warns for all eight), #3697-13c (loose operators keep their numeric endpoint), #3697-13d (the accepted false negative is symmetric, and the bare zero-selection line still warns). * fix(#3697): close the round's own pre-push review findings An adversarial cross-AI review of this round refuted 4 of its 10 claims. All four were real. Every fix below is to code THIS round introduced. 1. THE SOFT VOICE CLAIMED TOO MUCH (refuted CLAIM 1). `REQ-01, (REQ-02), REQ-03 — REQ-05` took the range-reading voice and told the author "the line is already correct and nothing needs to change" — while `(REQ-02)` had been dropped by the selector, which does not strip parentheses. The channel choice is still right, and deliberately so: `(ADR-7)` and `(REQ-02)` are the SAME shape, so routing on "was anything unselected?" puts the false "could not be parsed" claim back on a line carrying a citation — the misroute fixed two commits ago. No rule can adjudicate this; the author can. So the voice stops asserting the line is correct (it now speaks about the SEPARATOR, which is all it has evidence about), and BOTH voices gained a factual clause naming ID-shaped text the selector skipped, with the reason (brackets are not stripped) and no verdict attached. 2. THE CAP SILENCED A LINE THAT USED TO WARN (refuted CLAIM 2). A 2049-char range token warned before this round and went silent after it: the "uniform cap" commit bounded the predicate and, with it, the warning. That is #3697's own defect, introduced by the fix for a nit. The cap bounds the WORK, not the warning. An over-cap token carrying `-` is now recorded as unclassified (a linear `includes`, never the unanchored regex the cap exists to keep off it) and gets its own voice: "could not be checked ... the REQ-ID selection on this line is unverified". Unclassified is reported, never treated as clean. 3. THE CAP WAS NOT UNIFORM (review MISSED finding). R2 capped the operator token but not its neighbours, so `<2049-char ID> .. <2049-char ID>` still ran REQ_ID_SHAPE_RE and BigInt over both endpoints unbounded. The glued rule had the same hole. Every participant is capped now. 4. PROPERTY P5 WAS VACUOUS (refuted CLAIM 5). It drew from a bare `fc.string()`, which over 500 samples produced max length 10 and ZERO inputs containing a REQ-ID — the loop body never executed an assertion. A containment property that never contains anything is a green test measuring nothing. The generator now interleaves real IDs with noise and the property ASSERTS it saw them (>50/500), so it can never silently go vacuous again. The free-form coverage it was actually providing survives, honestly labelled, as #3697-P6. The same finding refuted this round's claim that P2 distinguishes pre-round behaviour: every operator P2 uses was already in the pre-round operator set. P2 is a regression guard, and its comment now says so. Also: docs/CLI-TOOLS.md repeated the broken channel claim verbatim (review MISSED finding) and is corrected with the code. Tests: #3697-9d (soft voice names the skipped ID, never claims the line is correct), #3697-9e (over-cap token reported as unclassified, still warns), #3697-9f (R2 and the glued rule cap their neighbours). #3697-B1/B2 now key the boundary on the PREDICATE's verdict with `warn` asserted true at every length — asserting `warn === false` at limit+1 was itself finding 2. * docs(#3697): describe the third voice and the dash rule Follow-on to the review-findings commit: that commit corrected the docs' claim about the soft voice but left two things the code now does undescribed. - There are THREE voices, not two. The over-cap voice ("could not be checked ... unverified") arrived with the fix for the review's CLAIM 2 and had no entry. - Dash spellings require a full ID on both sides, and `..` / `…` / the word operators do not. That asymmetry is deliberate and load-bearing — `PREFIX-<digits><dash><digits>` is date- and sub-number-shaped — so a reader hitting `REQ-01-05` and getting silence has no way to find out why. The accepted cost (`REQ-01, REQ-02-05` unreported, bare `REQ-02-05` still reported) is stated rather than left to be discovered. Documentation only; no behaviour change. * fix(#3697): close the continuation review's findings A continuation of the same adversarial reviewer, run against the reworked round, refuted 6 of 7 claims. Four were real defects in this round's own work and are fixed here; the other two are answered rather than changed, below. 1. THE SKIPPED-TEXT CLAUSE WAS ON ONE VOICE, NOT BOTH (refuted CLAIM A). The previous commit's message said both voices gained it. Only the soft return appended it. The assertive voice now carries it too — and, because that voice already names range tokens and inert residue under its own clauses, the note is filtered to what those did not already name. A warning that says the same token twice is one readers learn to skim. 2. THE CLAUSE'S WORDING WAS FALSE (also CLAIM A). It read "brackets and parentheses are not stripped". Square brackets ARE stripped by the selector — `[REQ-01, REQ-02]` is the documented form — so only parentheses qualify. Corrected in the message and in docs/CLI-TOOLS.md, which had inherited the same error. 3. THE OVER-CAP RULE STILL SILENCED A LINE (refuted CLAIM B). `oversizedTokens` filtered on `includes('-')`, which misses an over-cap OPERATOR: `REQ-01 <2049 dots> REQ-05` warned before this round, R2 declined to classify it once capped, and nothing reported it. That is the exact regression the field was added to close, one input over. Any token past the cap now counts — what it contains is irrelevant when we could not read it. 4. AND THEN OVER-REPORTED ONE (review MISSED finding). With (3) in place, a 2049-character CANONICAL REQ-ID was selected by the uncapped, fully-anchored selector AND flagged "REQ-ID selection on this line is unverified" — a contradiction inside one warning. A token the selector took was examined end to end, so it is excluded. Two findings are answered, not changed: CLAIM C — the selector's own `REQ_ID_SHAPE_RE.test` is uncapped. True, and deliberate: this round does not touch what gets MARKED, and the pattern is anchored at both ends with no nested quantifier, so it is linear. The claim that "all predicate paths are capped" was too broad; the DETECTOR's are. CLAIM E — `REQ-01, REQ-02<dash>05` is silent for every dash. That is the documented, deliberate cost of holding dashes to the strict shape, and it is symmetric with ASCII, which behaved that way before this PR. The reviewer is right that "without losing a range spelling that should be detected" was too strong; a bare `REQ-02<dash>05` still warns. Tests: #3697-9g (clause on the assertive voice, no repetition, bracket claim true), #3697-9h (over-cap operator does not silence the line), #3697-9i (a selected over-cap ID is never called unverified). #3697-9f is rebuilt — its first version used the SAME id twice, so R2 could not have fired even uncapped and it proved nothing; it now uses endpoints with a gap and fails when the neighbour cap is removed. Reversion control: all four fixes fail a named test when reverted in isolation (#3697-9h, #3697-9i, #3697-9g, #3697-9f). * fix(#3697): scope the over-cap exemption to what could actually pair A second continuation of the same reviewer, against the reworked round, confirmed the two claims that matter most and refuted three. This closes the one real defect; the other two are answered below. CLAIM J / CLAIM K (one defect, found from both directions). The previous commit exempted EVERY selector-accepted token from `oversizedTokens`, on the reasoning that the selector is uncapped and anchored so it examined the whole token. True of that token's SELECTION — and not the same as "no rule was suppressed by it". Two over-cap valid IDs either side of `..` are both selected, so both were exempted, and R2 is capped: a line that warned before this round went silent. That is the third appearance of one class in this round — the cap suppresses a check, and the suppression is not reported. Each fix for it over-corrected in the opposite direction, which is why the rule is now stated in terms of what was actually suppressed rather than in terms of the token: an over-cap token is exempt only when it was selected AND nothing beside it could have paired with it into a range (no range operator, no glued fragment, no second over-cap token). Everything else is unexaminable and says so. Two findings are answered, not changed: CLAIM M — the reviewer demonstrated, with driven evidence, a contextual rule that catches `REQ-01, REQ-02-05` while leaving `FY-2026-08` and `API-2-01` silent: recognise `PREFIX-a<dash>b` only when another SELECTED id on the line shares that prefix. That refutes this round's claim that the strict-dash trade was FORCED, and the claim is withdrawn — it is a design choice. The choice stands for this PR: the conservative rule is what ASCII already did before #3697, adopting a new contextual heuristic unreviewed at the end of a round is how the last three defects in this round were made, and #3697 asks for a warning rather than better range inference. Named here so the alternative is on the record rather than lost. Docs MISSED — CLI-TOOLS said every token over 2,048 characters "is not classified at all" and warns. Selection is not bounded; only range detection is. Corrected. Confirmed by the same pass, and worth recording because they are the PR's load-bearing promises: a 20,000-input comparison of the pre-extraction selector against HEAD found `mismatches=0` (nothing about which REQ-IDs are MARKED has changed), and the uncapped selector regex was measured linear from 100k to 800k characters. Tests: #3697-9f now asserts the range case is reported rather than silent, and #3697-9j pins the exemption's scope in both directions. Reversion control: restoring the blanket exemption fails both. * fix(#3697): warn on zero selection, as the acceptance criterion asks `Deferred`, `N/A`, `Pending`, `TBA` and `-` selected no REQ-IDs and stayed SILENT, while three shipped artifacts said they warned: `docs/CLI-TOOLS.md`, the `placeholderLed` census comment, and the advice string the command emits to the user. The asymmetry was the tell — `Deferred (see ADR-7)` warned, because the citation supplied the ID-shaped residue R3 required, while bare `Deferred` did not. The claim was written into three places and never executed once. This is also #3697's AC-1b/AC-4 verbatim: "warn when `citedReqIds.length === 0` while the raw capture is non-empty and not `TBD`". R3b keys on the SELECTION being empty, never on what the prose means, so it adds no free-text heuristic. It is deliberately not gated on ID-shaped residue the way R3 is, and the negative space is what settles that: all fifteen #2334/#2339 fixtures are held silent by non-zero selection or by `placeholderLed`, and not one of them by the ID-shape gate — measured, not argued. The gate was buying no negative space while costing the acceptance criterion. `tokens.length > 0` keeps an empty line and a comment-only line silent: the tokenizer strips `<!-- ... -->` before splitting, so the shipped template's own comment cannot reach the rule. Selection behavior is unchanged. This warns; it never invents an ID. Also extracts `warn` to a named const (round 3 review Minor 3) — this commit adds a disjunct to exactly that predicate, and in the return literal a later reordering would be a TDZ ReferenceError rather than a reader-visible error. Tests: #3697-14 (six zero-selection lines warn and tick nothing, and the warning names the TBD/None escape), #3697-14b (five placeholder spellings stay whole-channel silent), #3697-14c (comment-only line stays silent). Fail-first controls: all six #3697-14 cases fail against the pre-fix tree; -14b and -14c pass at both ends, which is correct — they pin silence the widening must preserve. * fix(#3697): name the REQ-ID a glued delimiter dropped `RANGE-01; RANGE-02` selects only RANGE-02 and marks only RANGE-02, with `requirements_updated: true` — #3697's own half-success failure mode, reached by one wrong delimiter, and silent before this rule. It is the issue's AC-1a ("a warning whenever the line contains ID-shaped content that the tokenizer did NOT select") at the shape most likely to be typed by accident. Round 4 review rated this Major rather than Blocker on the ground that the case is indistinguishable from a parenthesised citation, since `(ADR-7)` also shaves down to a bare ID. At the RAW token level it is distinguishable, and that is what makes the rule shippable: `REQ-01;` is shaved of a trailing DELIMITER, `ADR-7)` of a citation wrapper. R4 keys on that shave class and requires the token to sit outside any parenthetical. Measured before implementing: 0 false positives and 0 false negatives across 21 probes, including all fifteen #2334/#2339 negative-space fixtures. A first cut without the parenthetical test scored 3 false positives — every one of them a colon inside a citation (`(see ADR-7: section 3)`) — which is why that test is the rule's boundary rather than an optimisation. Adds the delimiter census the module did not have. The range-operator domain was already censused; the comma-substitute domain was not. Swept 26 spellings: exactly two produce a silent under-selection, `; ` and `: `. Every other spelling either selects both IDs or selects none and already warns. The review hand-listed the semicolon; the colon is the sibling that sweep found, and it fails identically. `rangeReadingOnly` now excludes an R4 hit — the ambiguous voice claims nothing was dropped, and must not speak for a line where something demonstrably was. Tests: #3697-15 (four delimiter shapes warn, name EVERY dropped ID, and tick exactly the unchanged selection), #3697-15b (three citation forms stay whole-channel silent). Fail-first control: all four #3697-15 cases fail against the previous commit's tree; -15b passes at both ends, pinning the boundary the widening must not cross. * fix(#3697): give the Requirements-line warning a stable machine kind The warning's kind existed only in the prose of its message, so every consumer and every test had to regex an English sentence — and rewording a message silently un-asserted the tests that pinned it. Round 4 review Major 3. The repo already had the settled seam for exactly these semantics. `CONTEXT.md` records `diffLiveConfig` emitting `kind:'unverified'` for a truncated scan, which is precisely this module's third voice; and `WAVE_CLEANUP_WARNING` in `src/worktree-safety.cts` carries codes for the same reason. ADR-3473 Decision 3 ("failure is a value") points the same way. `formatRequirementsLineWarning` now returns `{ code, message }` instead of a bare string, which also settles round 4 Nit 3 — `null` still means CLEAN, a legitimate value, but the success arm is no longer a naked string one field away from the shape the ADR standardises on. The kind is carried ALONGSIDE the prose, never instead of it. `warnings[]` is a documented `string[]` in `phase complete`'s JSON output, rendered by execute-phase.md's "If has_warnings is true" step, so re-typing its elements would be a breaking output-contract change for a shipped command. The code is emitted as its own additive `requirements_line_warning` field, absent entirely when the line is clean. Vocabulary, exported so tests key on it rather than on string literals: `req-line-misparse`, `req-line-range-reading`, `req-line-unverified`. Tests: channel ROUTING in #3697-9/-9b/-9c/-9d/-9e/-9g/-10 now asserts the code; message-content assertions stay where the user-visible wording is itself under test. #3697-16 pins the code end-to-end through the CLI's JSON for four line shapes and asserts warnings[] is still a string[]; #3697-16b pins that a clean line emits no kind at all, because a field present on every run carries no information. #3697-P4 holds kind-and-message-appear-together and kind-is-in-the-declared-vocabulary over arbitrary input, so a channel added later cannot ship without one. * test(#3697): pin the divergence against the second parser of the same line CLAUDE.md, KNOWN DEFECTS & ANTI-PATTERNS: "Generative Fix Divergence: when sharing constants/arrays/parsers between parallel surfaces, add a parity assertion test that fails if they diverge." Round 4 review Major 2. `normalizePhaseReqIds` (src/gap-checker.cts) parses the SAME ROADMAP `**Requirements:**` value — its own docblock says callers "may pass the roadmap value through verbatim" — and diverges on four axes. Measured, not inferred: line phase complete gap-checker RANGE-01..RANGE-05 [] 5 IDs None (per ADR-7) [] ["ADR-7"] (REQ-02) [] ["REQ-02"] REQ-01a [] ["REQ-01a"] REQ-01, REQ-02 both both This pins the divergence rather than removing it, which is the review's second option and the correct one here: unifying the two would change what `phase complete` MARKS, and "the ledger-writing set is byte-identical to base" is the one invariant this PR holds fixed. Every axis is now asserted in BOTH directions, so drift on either side fails here instead of widening silently. The range axis is a DELIBERATE disagreement and is labelled as such — #3697 declines range expansion in terms ("I am not asking for range syntax to be supported") while gap analysis adopted it under #1269. The placeholder axis is the one worth reading twice: `None (per ADR-7)` is a declared-empty line to `phase complete`, which reads the lead token, and a one-requirement line to gap-checker, which strips parentheses first so the citation survives its ID-shape filter. That is a citation being reported as a requirement. #3697-17b states the cost concretely: one line, five requirements in scope to gap analysis and zero to phase complete. This PR is what makes that contradiction visible, by finally giving the silent side a voice. * fix(#3697): stop the skipped-text rider reporting a date, and close the 4b channel gap Two round 4 review minors, both about a warning saying something it cannot support. MINOR 2 — false rider content. `REQ_ID_SUBSTRING_RE` is unanchored, so `FY-2026-08` matches as `FY-2026` and lands in `unselectedIdShaped`. `REQ_RANGE_TOKEN_RE`'s entire strict-dash arm exists to keep that shape silent, and #3697-4 pins `RANGE-01 (target FY-2026-08)` as producing no warning at all — but whenever some OTHER rule fired on a line that also carried a date annotation, the rider told the author to "check whether any of it is a requirement" about a date. Not a false warning, since the line was warning anyway; false CONTENT, in the #2334 voice, through the side door. Filtered at the MESSAGE rather than in the analysis: `unselectedIdShaped` stays a faithful record of what the selector skipped — it is documented as a fact that never routes — while the user-facing clause declines to assert requirement-ness about a shape the design already ruled unadjudicable. #3697-18b is the other half, so the filter cannot become a silencer: a genuinely dropped REQ-ID is still named. MINOR 1 — `#3697-4b` asserted only that the ASSERTIVE channel stayed silent, so a regression routing `RANGE-01, TORANGE-05` into the AMBIGUOUS channel would have passed. Whole-channel silence is not available on that fixture (the pre-existing ghost-ID warning legitimately fires on the unregistered `TORANGE-05`), so the precise assertion is that no Requirements-line warning of ANY kind was emitted. The machine code added earlier in this round is what makes that statable; before it, "both channels" could only have meant a second prose regex. * docs(#3697): record the Requirements-line seam in CONTEXT.md CLAUDE.md names the CONTEXT.md glossary as a PR gate, and `get_cochange_context(src/phase.cts, 45d)` ranks CONTEXT.md 4th at 25 co-changes — above src/init.cts and src/roadmap.cts. This PR introduced a named seam, three warning kinds, a bound, a rule taxonomy and a deliberate cross-parser divergence, and recorded none of it. Round 4 review Major 4. The precedent is explicit rather than inferred: the directly analogous seam is already there as `LIVE-CONFIG.GUARD.SEAM.truncation`, including its bound and its boundary obligation — and that entry is the one this module's third voice was modelled on. Eight predicates, in the machine-oriented section beside it: .module the two exported functions and the code vocabulary .selector-identity citedReqIds is byte-identical to base and is the only thing reaching the ledger — a change to what phase.complete MARKS is outside this contract .rules R1 / R2 / R2' / R3 / R3b / R4 / over-cap .kinds the three codes, and why they ride beside warnings[] rather than inside it .cap 2048, neighbours included, and the boundary rule .placeholder the gate that actually holds the negative space .census-domains both open domains with their NOT-reached members .gap-checker-divergence the four axes, pinned not unified The changeset type is `Fixed`, which exempts this PR from the docs/ co-change requirement — but the glossary gate is separate from that exemption, and the 2048 cap in particular is a machine-canon-shaped fact that until now existed only inside a source comment. `docs/CONTEXT-INDEX.json` regenerated (269 predicates); lint:generated-sync confirms all six targets in sync. * docs(#3697): document what the command now does, in one changeset sentence DOCS. The grammar section predated this round's two new rules, so it under-described the behaviour it exists to make lookup-able: - The placeholder paragraph enumerated three words; the rule is a DEFAULT. Any wording that selects no REQ-IDs warns, and the placeholders are matched as the LEAD token, so `None (per ADR-7)` and `**None**` are declared-empty too. The comment-only line is called out, because "any other wording" would otherwise read as covering the shipped template's own `<!-- ... -->`. - The comma rule was implicit. `REQ-01; REQ-02` marks only REQ-02, and it is the quietest way to lose a requirement on this line — `requirements_updated` reads `true` either way — so it gets its own paragraph, with the parenthetical exemption stated beside it. - The machine kind is documented where a consumer would look for it, with the instruction to key on the kind rather than the wording. - The skipped-text note no longer implies it reports date shapes; it deliberately does not, and silently omitting that left the doc promising the behaviour this round removed. CHANGESET (round 4 review Minor 4). CONTRIBUTING.md's format is `**<Bold user-visible change>** — <symptom-led explanation>.` and both canonical examples are one sentence; this fragment ran three. Now one, and covering what the round actually delivers rather than only the range shape it started from. * test(#3697): keep phase.test.cjs off the docs-guard exemption fingerprint A comment added earlier in this round named `docs/CLI-TOOLS.md` by path. The docs-guard exemption ratchet (#3753 FIX 3) fingerprints literal `docs/` references in exempt test files and fails when a new one appears, so that comment turned four green gates red — `ci-docs-guard-registry` and the registration lint — for a file that reads no documentation at all. Caught by diffing the full suite's failing-name set against the same suite run at `upstream/next` in a probe worktree: 33 of 37 failures reproduce at base (install / config-home / shadowing tests under the sandbox HOME), and exactly these 4 did not. Rephrased rather than baselined. Adding the path to DOCS_GUARD_EXEMPT_DOCS_PATHS is the sanctioned response when a test genuinely starts READING a new docs path — the violation text asks the author to re-confirm the exemption still holds. Nothing here reads documentation; the guard matched prose. Baselining would have recorded a coupling that does not exist and made the next reader wonder what phase.test.cjs does with CLI-TOOLS.md. The comment still names where the contract is written, just without planting a path string. * fix(#3697): close four defects this round's own pre-push review drove An adversarial cross-AI review of this round, run before the push, returned 6 CONFIRMED and 4 REFUTED. Every refutation was driven against the built tree, and every one was a shape the author had not probed — the rules were correct across the probe set and wrong just outside it. (1) R4 FALSE POSITIVE, and it is the #2334 over-warning class arriving through the rule added to close a different hole. `REQ-01, see ADR-7: section 3` fired: `ADR-7:` is the same shave class as `REQ-01;`, and the parenthetical test does not reach a BARE citation. The FP probe that scored this rule 0/0 only ever tested the parenthesised form. Fixed by requiring the dropped id's prefix to agree with a SELECTED id — the module's own idiom, not a new heuristic: `reqEndpointsImplyInterior` already demands an agreeing prefix for the same reason. Cost, stated in the census: a dropped id whose prefix is on no selected id (`REQ-01, FOO-02: x`) stays silent. Same trade the strict-dash rule takes — under-report a rare shape rather than over-report a common one. Pinned as a declared blind spot by (2) R4 FALSE NEGATIVE, on the DOCUMENTED form. `[REQ-01; REQ-02]` dropped REQ-01 silently: the selector strips square brackets and R4's raw scanner did not. The bracket spelling the shipped template recommends was the one shape the rule could not see. (3) The rider filter suppressed a REGISTERED requirement. `API-2-01` is a legal requirement id — gap-checker's `parseRequirements` accepts it from REQUIREMENTS.md — so a `\d+-\d+` filter hid a genuinely dropped requirement behind a rule meant only to hide dates. Narrowed to a four-digit year segment. The earlier #3697-18 case asserting `API-2-01` should be suppressed is REMOVED, and the removal is recorded in place: its premise was refuted, it was not inconvenient. (4) An INVISIBLE line warned. A lone U+200B carried a token to the parser while reading as empty to the author, so R3b fired with nothing on screen to explain it. Zero-width and format characters are now stripped — stripped rather than treated as delimiters, because splitting on one would fabricate two fragments out of one ID. Also corrects the documentation the same review found overstated: the line is split on commas AND whitespace, and the ID shape is matched case-insensitively, so `REQ-01 REQ-02` and `req-01, req-02` both select and neither warns. That was pre-existing selector behaviour; this round is the one that asserted the docs were true of it. Tests: #3697-19 (four invisible-only shapes), -19b (embedded zero-width is stripped, not split on), -19c (three citation forms), -19d (both bracket spellings), -19e (both halves of the rider boundary), -19f (the declared blind spot), -19g (the two documented tolerances). 500 tests in phase.test.cjs, 0 failures; lint:ci clean. * fix(#3697): generalise the drop rule, and stop the invisible fix hiding a drop The pre-push review's continuation refuted six of seven follow-up claims. The first one is the one that mattered: the invisible-character fix committed in c7dce173a INTRODUCED #3697's own defect. Stripping zero-width characters from the detector wholesale made `REQ-01<ZWSP>, REQ-02` go SILENT — the selector really does drop REQ-01, and the strip removed the only evidence of it. The test written alongside asserted the tokens and the empty R4 result and never asserted `warn`, so it DOCUMENTED the bug rather than catching it; that omission was the reviewer's own MISSED finding. An invisible is two different questions about one character, and the fix is to stop conflating them: absence-of-content for the empty test, DECORATION on a token for the drop rule. Neither is a reason to delete it from the line. R4 is generalised accordingly, because the continuation drove four more shapes a trailing-delimiter-only regex could not see — `REQ-01 ;REQ-02`, `REQ-01 :REQ-02`, `**REQ-01;** REQ-02`, the backticked form — plus `**REQ-01**, REQ-02`, where emphasis alone defeats the selector. These are one class: decoration on a token the selector then cannot take. One rule, not four patches; patching them individually is how a list stays short and wrong. PARENTHESES ARE NOT DECORATION, and the suite caught me learning that: shaving them made `REQ-01, (REQ-02), REQ-03 — REQ-05` report a glued delimiter that was never there and broke #3697-9d's channel routing with it. A parenthesis is this rule's citation marker. The rider stops adjudicating an undecidable shape. `API-2-01` is a legal requirement id and `API-2026-08` is too, while `FY-26-08` and `FY-2026-08-15` are dates — no regex separates them, and both filters this round tried scored a miss in each direction. It now NAMES the token and states the ambiguity, which is the same thing the two warning voices already do about a range separator. Filtering hides a real dropped requirement; reporting it bare asks the author whether a date is a requirement; saying "this may equally be a date" does neither. The census and the docs are corrected to what the code does, including the part that is NOT complete: the prefix gate does not stop a citation that SHARES a selected prefix (`ADR-01, see ADR-7: sec 3` fires), and nothing at token level separates that from a real drop. A prose heuristic on "see" is the free-text detector this module exists to avoid, so the honest move is to say so. CLAIM 17 — the invariant that actually matters — came back CONFIRMED on a 20,000-run fast-check property over arbitrary Unicode: `citedReqIds` is identical to upstream/next's for every input, and marking is untouched. 513 tests in phase.test.cjs, 0 failures; lint:ci clean. * fix(#3697): gate the drop rule on evidence, and stop an unmatched paren swallowing the line Third pass of the round's own pre-push review, scoped to regression-hunting rather than further polish. Three findings, all driven, all mine. R4 OVER-WARNED on markdown styling. `REQ-01, see **REQ-7** for context` claimed a dropped requirement: the previous cut treated any shaved decoration as evidence, and emphasis is not evidence. Nothing separates that line from `**REQ-01**, REQ-02` meaning to list one, so the rule now requires a positive signal — a glued `;`/`:` (a list separator was INTENDED) or an invisible (the token is CORRUPTED; nobody types one on purpose). Emphasis alone falls back to the skipped-text rider, which names the id without asserting a drop, exactly as `(REQ-02)` is handled. That is the #2334 class caught one cut before shipping. R4 UNDER-WARNED on `**REQ-01**; REQ-02` — one shave pass cannot reach a wrapper sitting behind a delimiter. Shaves to a stable point now. The range OPERATOR lost its invisibles handling. `REQ-01 <ZWSP>..<ZWSP> REQ-05` went silent, because the previous commit removed the invisible strip from BOTH the tokenizer and R4 when only R4's was wrong. An invisible is two questions about one character: for the classification rules it is noise and is stripped from the token; for the drop rule it is the evidence and must survive on the raw line. Stripping in both places hid a dropped id; stripping in neither hid a range. The reviewer's MISSED finding named the missing control — regression tests covered invisibles inside ids and not beside operators — and #3697-19i is that control. UNBALANCED PARENTHESES swallowed the line. `REQ-01, (note REQ-02; REQ-03` reported nothing: a running-depth counter left the unclosed `(` open through end-of-line, so every genuine drop after it inherited citation immunity. A parenthesis confers that immunity only as part of a MATCHED span now — an unmatched one is a typo, not a citation. CLAIM 22 re-confirmed on a fresh 20,000-run property over arbitrary Unicode: `citedReqIds` identical to upstream/next, marking untouched, warnings appended. 522 tests in phase.test.cjs, 0 failures; lint:ci rc=0; full suite carries zero head-only failures against a probe worktree at upstream/next. * fix(#3697): make delimiter ADJACENCY the rule, and delete matched citations outright Fourth and final pass of the round's own pre-push review. Three findings, and they shared one root cause, so this is a narrower rule rather than a longer list of shapes. TOKEN-WIDE PAREN IMMUNITY LEAKED. `REQ-01, REQ-02;(note) REQ-03` is a single whitespace token, so a matched parenthetical inside it conferred immunity on the `REQ-02;` sitting OUTSIDE the parens, and the drop went silent. Matched spans are now deleted from the line outright — which states what is actually meant, that for this rule a citation is not on the line — and an UNMATCHED paren is a typo that confers nothing. That also retires the running-depth counter whose previous bug was the mirror image: an unclosed `(` swallowing the rest of the line. DECORATION WAS TESTED TOKEN-WIDE, so `REQ-01, see **REQ-7**; next topic` was reported as a dropped requirement. It is a citation with sentence punctuation. The rule is now ADJACENCY: styling is stripped, then the `;`/`:` must be touching the id. `REQ-01;`, `;REQ-02` and `**REQ-01;**` qualify; `**REQ-01**;` does not, because outside the styling that character is punctuation. An invisible needs no adjacency test — nobody types one on purpose, so anywhere in the token it is corruption rather than intent. `**REQ-01**; REQ-02` therefore goes silent, and the test row asserting otherwise is inverted rather than deleted quietly: it was added one commit ago on the reasoning this pass refuted, and nothing distinguishes it from `see **REQ-7**; next topic`. Worth recording plainly: three successive cuts of this rule fired on a citation, and each fix was a narrower definition of EVIDENCE, never a longer list of shapes. The list-lengthening instinct is what produced the bug each time. The review's last MISSED finding named the missing control — the paren tests all surrounded matched spans with whitespace, so none covered a span sharing a token with an id outside it. #3697-19j carries both directions now. 526 tests in phase.test.cjs, 0 failures; lint:ci rc=0; full suite zero head-only failures against a probe at upstream/next; the invariant that `citedReqIds` is identical to upstream re-confirmed on 20,000 arbitrary Unicode inputs. * fix(#3697): state R4's real boundary, and stop the over-cap voice masking a drop Two defects, both found by this round's own pre-publication body claim-audit. 1. A DEMONSTRATED drop was discarded by the unverified voice. On `REQ-01, REQ-02: <2049 chars>` the analyzer names REQ-02 in delimiterDroppedIds and the formatter then reported `req-line-unverified`, whose message never mentions it — the one actionable finding masked by the token beside it. The over-cap channel now excludes a line carrying an R4 hit, exactly as rangeReadingOnly already did and for the same reason: that voice's whole claim is that nothing could be checked, and R4 has already checked something. The assertive channel still carries the over-cap rider, so nothing about the cap is traded away. Pinned by #3697-19l, which fails against the pre-fix build and nothing else does. 2. Three shipped artifacts asserted behaviour the code does not have — the same class as this PR's round-4 blocker, re-committed. CONTEXT.md's rules predicate, the CLI tools reference, and the warning's own advice string all listed markdown emphasis as an R4 trigger. It is not: styling is shaved BEFORE the test and tolerated around an id, never a trigger on its own, so `**REQ-01**, REQ-02` and `**REQ-01**; REQ-02` are both silent. The trigger is exactly a glued `;`/`:` or an embedded invisible. The census predicate was wrong in a second way. Its 26-spelling separator sweep found only `;` and `:` because the sweep was SYMMETRIC-ONLY and therefore biased: one-sided attachment drops silently for every punctuation outside the set — `/ | & + . > \` and the full-width and non-ASCII forms `； ， ؛` all measured silent. The domain is wide open and R4 covers two characters of it. Said plainly in all three places rather than widened here: every previous widening of this rule first fired on a citation, so it is not done blind at the end of a round. Both blind spots are now PINNED as tests (#3697-19m styling-only, #3697-19n one-sided separators) so the documents and the code cannot drift apart again — which is what the round-4 blocker asked for. tests/phase.test.cjs: 542 tests, 542 pass, 0 fail, 0 skip. lint:ci rc=0. Both CONTEXT-INDEX consumers regenerated. * fix(#3697): re-sweep the separator census properly, and say what it really found The round-4 census in src/phase.cts concluded "exactly two — `; ` and `: `" from a 26-spelling sweep. That conclusion was forced by how the sweep was built, not by the code: it swept the ONE-SIDED form (`REQ-01; REQ-02`) for the semicolon and colon, and only the BARE and SYMMETRIC forms (`|`, ` | `) for every other separator. Different members of the domain were tested in different shapes, so no other answer was reachable. Caught by this round's pre-publication claim-audit of the response comment, reading the census comment against its own swept list. Re-swept fully crossed and driven through the built artifact: 21 separators x {bare, trailing-space, leading-space, both-spaces} = 84 combinations. 26 select both ids, 24 under-select and already warn, and 34 UNDER-SELECT SILENTLY. All 34 are one shape — a separator glued to exactly one of the two ids, e.g. `REQ-01/ REQ-02` or `REQ-01 /REQ-02` — for every punctuation except `,` and the `;`/`:` that R4 covers. So R4 covers TWO CHARACTERS of a wide-open domain. That is now what the census comment, the CONTEXT.md census-domains predicate and the CLI tools reference all say. The set is deliberately not widened here: three successive cuts of this rule fired on a citation, and a fourth at the end of a round with no adversarial pass is how each of those got in. Second false passage in the same block: styling-only decoration was described as "left to the skipped-text rider, which names the id". A rider only exists inside a message, and a message only exists once some rule sets `warn` — so on a line where nothing else fires, `REQ-01, **REQ-02**` is wholly silent. Describing it as handled reads as coverage. #3697-19m already pins the silence. so the test matches the documented claim. tests/phase.test.cjs: 552 tests, 552 pass, 0 fail, 0 skip. lint:ci rc=0. Both CONTEXT-INDEX consumers regenerated. * chore(#3697): regenerate both CONTEXT-INDEX.json after rebasing onto next Rebased onto next @ f4fefb0be. The two generated indexes conflicted on the replay and were resolved by regenerating, not by hand-merging: `npm run gen:context-index` for docs/CONTEXT-INDEX.json and `node examples/dynamic-context-management/gen-context-index.cjs --write` for the example's copy. Against next, each now differs only in the eight PHASE.REQ-LINE.SEAM.* predicates this PR adds (plus the PHASE class and the count); every base-side change (the SEAM.* predicates, the ADR-3942 value rewrites) is carried. `lint-example-parser-parity` and `gen-context-index --check` both pass. * fix(#3697): an over-cap token outranks the ambiguous range voice Round 7 review, Minor 1. `rangeReadingOnly`'s guard conjunction checked `hasGluedRangeFragment`, `inertIdShaped` and `delimiterDroppedIds` but not `oversizedTokens`, so a line carrying a clean, fully-selected spaced range *and* an unrelated token past the 2048-char scan cap was coded `req-line-range-reading` — a code CONTEXT.md's PHASE.REQ-LINE.SEAM.kinds predicate documents as "nothing was dropped" — over a token no rule (R1-R4 all skip over-cap tokens) had ever examined. Since the PR tells machine consumers to branch on `.code` rather than parse prose, that is a false "nothing to verify further" signal. The review's suggested fix was to add `oversizedTokens.length === 0` to the conjunction. That clause is right and is here, but on its own it routes the line to the ASSERTIVE channel: `req-line-misparse`, whose message says the line "could not be parsed as a comma-separated REQ-ID list" on a line where every ID present was in fact selected. That is the #2334 over-warning class this module's own channel-selection docblock exists to prevent — a false-clean code traded for a false-assertion one. So the correct destination is `req-line-unverified`: the line was not CHECKED, which is not the same as clean, and nothing on it demonstrably failed to parse either. Both non-assertive voices were carrying their own inline copy of the same "nothing was demonstrably dropped" conjunction, and that duplication is what let them drift: `rangeReadingOnly` omitted the cap, while the over-cap channel excluded a spaced range wholesale via `!hasSpacedRange`. Extract it once as `nothingDemonstrablyDropped` and have both read it. The new predicate is a strict superset of the old `!hasSpacedRange` guard — `spacedRangePairs.every()` is vacuously true when no spaced range fired — so the over-cap channel's behaviour on every line without a spaced range is unchanged, and R2 firing on an endpoint the selector did not take still routes to the assertive channel. Exactly one input class changes routing: a clean fully-selected spaced range beside an unexamined over-cap token, which moves from `req-line-range-reading` to `req-line-unverified`. Pinned by `#3697-19n` (the twin of `#3697-19l` on the other side of the boundary — there a demonstrated drop outranks the unverified voice, here the cap outranks the ambiguous one), with `#3697-19o` as the negative control asserting a clean range with nothing over the cap is still a range reading. * docs(#3697): state the warning-code precedence, and what the cap condition actually is Two surfaces, one point. The round 7 finding cited CONTEXT.md's PHASE.REQ-LINE.SEAM.kinds predicate as the documentation of what `req-line-range-reading` claims, and it was right to: that predicate said "R2 alone fired on selected endpoints; nothing was dropped" with no mention of the scan cap, describing a line the code could not distinguish from one carrying a token it never examined. The range reading is the weakest of the three claims and yields to the other two: a demonstrated drop makes the line a misparse, and a token the cap left unclassified makes it unverified. `docs/CLI-TOOLS.md` gains that sentence; the CONTEXT.md predicate gains it plus two precision points that this round's own pre-push adversarial review extracted over three passes, each with a driven counterexample I reproduced before acting on it: * "nothing was dropped" overstates the rule. The discriminator is RULE-scoped by design (round 3, Major 3), so that a parenthesised ID-shaped token does not re-open the #2334 over-warning class — `(REQ-02)` is indistinguishable from `(ADR-7)` at token level and is carried by the skipped-text rider, never by this code. `REQ-01, (REQ-02), REQ-03 - REQ-05` selects three, names REQ-02 as skipped, and is still a range reading. The predicate now says "no rule named a dropped ID", which is what the code tests. * The deferral condition is `oversizedTokens` being non-empty, NOT the presence of a token past the cap. Those differ: a long token the selector itself took can be exempt, because selection is uncapped and anchored and such a token was therefore examined. `RANGE-01 - RANGE-05, R-<2047 sevens>` yields `oversizedTokens=[]` and stays a range reading. Two earlier attempts to characterise WHEN the exemption applies were both refuted — the neighbour test admits any non-short neighbour, not just a range operator — so the predicate now states the condition and defers the exemption's own rule to SEAM.cap rather than paraphrasing it a third time. Verified after the edit: deferral holds if and only if the cap left a token unclassified, across all three counterexamples plus controls, and an exempt selector-taken long token is exhibited. No behaviour change — predicate text, one CLI-reference paragraph, and the two regenerated indexes. Predicate count unchanged at 285 across 22 classes, 0 duplicate ids; lint-example-parser-parity and both `--check` generators pass. * test(#3697): rename the round 7 regression pair — 19n was already taken Self-found immediately after the push, before anything was published to the review thread. The two tests added this round were named `#3697-19n` and `#3697-19o`, and `#3697-19n` was already in use: it is the declared-blind-spot case for one-sided separators in both attachment directions, generated inside a loop with a computed label, which is why a grep for a literal `test('#3697-19n'` did not find it. The PR body's own *Declared blind spots* list already refers to `#3697-19n` with that meaning. Nothing failed. Duplicate test names do not error, and the suite stayed green at 567/567 — which is the argument for fixing it rather than against. Two concrete costs: any TAP name-set differential collapses same-named tests under `sort -u`, so one of the two becomes invisible to exactly the did-not-run and pass->fail checks that name-set comparison exists to perform; and a reviewer reading "#3697-19n" in the body now gets a different test than the one the body means. Renamed to `#3697-19p` (over-cap beside a clean range) and `#3697-19q` (its negative control), the next free ids in the series; the pre-existing `#3697-19n` is untouched at its original 4 occurrences. Cross-references inside the renamed block were updated with them. Negative control re-run under the new ids: 19p still fails against pre-fix source and passes after, 19q passes at both ends. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-01 14:36:57 -04:00
Tom Boucher	0bf778c352	fix(#3849): phase allocation counts numbers held by sibling git worktrees (#4042) ... * test(#3849): phase allocation must skip numbers held by sibling worktrees (failing first) * fix(#3849): phase allocation counts numbers held by sibling git worktrees Both allocators (cmdPhaseAdd, cmdPhaseAddBatch) chose max+1 over numbers gathered from ONE checkout — headers, bullets (add only), on-disk dirs. Every sibling git worktree carries its own .planning/ on its own branch, so a phase minted there was invisible and the same number was allocated twice (the reported incident: two Phase 441s, one with six written plans, surfaced a day late by human memory). New shared horizon collectSiblingWorktreePhaseNums: one git worktree list --porcelain, then per sibling — phase-dir names (the cheap scan that would have caught the incident) and the WHOLE sibling ROADMAP.md headers (a row can predate its dir; milestone-scoping would be wrong — a number used under any milestone on another branch is taken). Widen, never refuse: unreadable sibling / no .planning / not a git repo / git unavailable each contribute nothing and allocation is unchanged. Reuses isSentinelPhaseId and the allocators' own patterns. Secondary (#1229 never reached batch): cmdPhaseAddBatch now also scans roadmap bullets — a bullet-only 'Phase N' row was invisible to batch allocation, exactly the condition #1229 was filed for. Also fixes the two new tests' result-key access (output.phases, not output.results). * fix(#3849): review fold-ins — subprocess band, bounded test git, linked-worktree fixture - execFileSync options now match the repo's git band (10s window, windowsHide, 4MiB maxBuffer) — a spurious 4s timeout silently reverted to the pre-fix collision. - test git helper bounded (15s) per local/no-unbounded-spawn. - new fixture: allocation FROM a linked worktree counts the main checkout — the incident's actual topology direction. - fixture-setup rmSync carries the sanctioned lint-disable (setup, not teardown; cleanup() still owns directory removal). * fix(#3849): exempt the sibling-worktree scan from the enumeration-drift guard collectSiblingWorktreePhaseNums reads a SIBLING checkout's phases dir — a different question from the cwd-scoped listMilestonePhaseDirs the guard routes everything to (which cannot see another worktree's .planning at all). Function-scoped, per ADR-3180 Decision 4(a): any other re-derivation in phase.cts is still caught. The GREEN bench caught the omission. * chore(#3849): changeset fragment (pr number backfilled after PR creation) * chore(#3849): backfill changeset PR number (4042) --------- Co-authored-by: sim <sim@local>	2026-08-29 11:35:19 -04:00
Tom Boucher	15af0f5536	enhance(#3951): B6+B7 — widen two unreachable lint rules and make the guard ledger true (#3965) ... * fix(#3951): two lint rules that could not reach the code they govern B6 names two widenings. Measuring them first turned up a defect the criterion did not know about, and refuted the reason it gave for one of them. 1. no-adhoc-markdown-parsing self-gates on its own filename. Lines 107-110 short-circuit create() to {} unless the path matches /(?:^|\/)src\/[^/]+\.cts$/. B6 says to widen the files: glob in eslint.config.mjs - but doing only that ships an INERT rule, because the gate still returns {} for every new path. Both halves have to change, and the gate is the load-bearing one. That same regex hides a live hole: [^/]+ is FLAT-ONLY, so it requires the file to sit directly in src/. The registered glob is src/**/*.cts, which includes subdirectories. 28 .cts files - health-diagnostic-rules/ (10), installer-migrations/ (11), observability/ (3), host-integration-adapters/ (2), vendor/ (2) - are inside the registered glob and silently skipped. Measured with the gate neutralized: 0 violations there today. The hole is hiding nothing right now, and is fixed anyway, because "no violations today" is not a property that keeps holding. The fix is not invented: require-subprocess-timeout.cjs:196 already carries the correct form of this guard, /(?:^|\/)src\/.*\.cts$/ with .*, one directory over. Checked the other 21 rules for the same bug - no-adhoc-regex-escape and no-private-binary-resolution short-circuit only to exempt their own seam file, which is the right shape, and no-crlf-fragile-split has no filename gate at all. This bug is unique to the one rule. 2. no-adhoc-regex-escape could not see the shape that actually occurs. Line 396 gated the whole UNSAFE-NEW-REGEXP arm on arg.type === 'Identifier'. Every check below it - the _SOURCE provenance check, the isSoleReturnOfOwnParameter shape - lives inside that branch, so new RegExp(obj['key']) and new RegExp(cfg.pattern) were never examined at all. Runtime data arrives as a property access far more often than as a bare identifier, which is exactly why this rule never fired on the #3477 ReDoS. Widened to MemberExpression, measured by AST walk across all five registered blocks rather than by grep. 27 sites, zero TSAsExpression: 18 safe new RegExp(X.source, flags) -> exempted, keyed strictly on the PROPERTY being `source`, never on the object. Keying on the object would wave through X.anything and buy nothing. B6 estimated ~10; that was an undercount. 3 _SOURCE-suffixed constants reached through a required module namespace (phaseId.BRACKET_PHASE_TOKEN_SOURCE) -> the same provenance-exempt class the rule already recognizes for bare identifiers, extended to reach them. Without this the widening produces 3 false flags. 6 real findings -> marked, each a test extracting a pattern from a shipped file at test time, where the runtime contract IS the product. Deliberately the NARROW MemberExpression form. The rule's own isSoleReturnOfOwnParameter doc comment records that an earlier broad "any non-literal identifier" heuristic produced ~25 false positives and was rejected; a re-run of the census after this change flags exactly the 6 above and nothing else. Verified by execution, not by reading: the gate now accepts src/<subdir>/x.cts, still accepts flat src/x.cts, and still exempts paths outside src/ - each pinned by a test proven to fail against the old regex. build:lib, lint and lint:ci all exit 0. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3951): give no-adhoc-markdown-parsing its reach, and fix the 80 parses it finds The rule self-gates on filename AND is registered on one glob, so widening either half alone is inert. Both move here: the gate now accepts tests/**/*.cjs and scripts/**/*.cjs alongside src/**/*.cts, and eslint.config.mjs registers it on the same two. A test pins that the gate and the registration AGREE, in both directions. The original defect was a gate narrower than its registration; the failure mode of this fix is a gate wider than its registration. Both are silent, so the test asserts the pair rather than either half. 80 violations across 43 files, all in tests/, zero in scripts/. 70 are routed through the existing seams - scanFencedBlocks, collectSection, stripFencedCode, tokenizeHeadings from markdown-sectionizer; splitTableRow, parseMarkdownTable, findTableWithColumns from markdown-table. Headerless STATE.md tables use splitTableRow per line, because parseMarkdownTable needs a real delimiter row. 10 are suppressed, 12.5%, well under the third that would have meant the rule is mis-scoped for tests/ rather than the tests carrying debt. Each names its reason: three regression guards (#3873 / bug-#21) are deliberately independent of the generator's own fence handling, and routing them through the seam would have them test the generator against itself; one is a negative-text probe that extracts nothing; six are a shell-pipe-to-jq detector whose regex coincidentally matches the table fingerprint and is not markdown parsing at all. All ten sit in tests whose subject is .md content, which is normally a reason to prefer the seam. The marker used is allow-adhoc-markdown, distinct from no-source-grep's allow-test-rule, and lint:ci's lint-allow-test-rule-refs reports the same 280/280 unverified count as before - checked rather than assumed, because those two markers are easy to conflate. The widening earned its keep immediately: it found a test that passed for the wrong reason. tests/config-field-docs.test.cjs asserted notEqual(<cell>, '600') against the TYPE column instead of the DEFAULT column. notEqual('number', '600') is true forever, so the guard against workflow.subagent_timeout regressing to the old seconds default could never fire. docs/CONFIGURATION.md:434 is `| workflow.subagent_timeout | number | 300000 | ... |`, so the default is cell index 2; the assertion is now row-scoped through splitTableRow and reads 300000. That is the argument for the widening in one case: the violation was invisible to lint, the suite was green, and the assertion was vacuous. A rule that cannot reach a file cannot tell you the file is lying. Not fixed here, and recorded rather than assumed: #3426/#3239 are NOT reachable by this widening. tests/package-legitimacy-gate.test.cjs yields zero violations even with the gate bypassed - its hand-rolled scans are real, but built from line filters and split('|') rather than the regex-literal fingerprints this rule detects. They need new detectors. The epic assumed a wider glob would catch them. build:lib, lint and lint:ci all exit 0; the post-fix census across tests/** and scripts/** is 0 violations. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3951): B7 — and #3356's defects were still live in the code B7 asks that each closed child be driven fail-first with a behavioral identity test at the CONSUMER's output. Four of eleven children had no test citing their issue number. Auditing them by BEHAVIOR rather than by number-grep changed the answer for three of the four. #3364 and #2540 — traceability only. Both were implemented by #3941 and their consumer-output tests exist and were shown failing-first; neither cited its originating issue, so an audit that greps for the number reports them uncovered. Tagged the specific asserting test in each file, following the citation form those files already use. #3372 — covered, but only at helper level, and the triage narrowed it. Of the four commands the issue names, only estimate-cli's collectCalibrationSamples actually enumerates phase dirs from disk; smart-entry, audit and roadmap-upgrade derive from ROADMAP/body text and never reach the sentinel path, so they are benign by construction and were left alone rather than "fixed" into churn. The existing #3882 rows asserted the helper's return value. Added a consumer-output test driving `query estimate-calibrate` and asserting sample_count and the persisted document. RED proof: reverted collectCalibrationSamples to a raw readdirSync and ran the real CLI - sample_count 3, sentinel leaked; restored - sample_count 2. #3356 — NOT covered, and BOTH halves of the defect were still live in source. The issue is closed; the bug was not fixed. Fixed here rather than writing tests that document a bug as correct. Defect 1, the contradicted row. quick.md:627 claimed `quick-tasks-append` performs "the equivalent write" to the Step 7c row. It did not: the `#` cell was a positional ordinal and `Directory` read `—`, because the route had no way to receive a quick id or task directory. Added OPTIONAL `--quick-id` / `--slug` / `--directory`. A caller with neither - fast.md, the original #2133 caller - omits them and gets the byte-identical prior row, so nothing existing changes. A caller that HAS a real id and directory now gets the canonical row quick.md:632 renders. The false-equivalence sentence itself is corrected rather than left to mislead the next reader. Defect 2, the forced re-derive. The route called readModifyWriteStateMd with no options, so a body-only append to the Quick Tasks table triggered a full re-derive of the disk-derived progress.* frontmatter. Every other body-only writer passes { resync: false } - src/state.cts's own docstring prescribes it - and this route was the lone outlier. RED proof: reverted the option, seeded a project with 2 real phase dirs and a curated total_phases of 25, ran quick-tasks-append; total_phases collapsed to 2. Restored; it stayed 25. That second one is the shape this epic exists to close: a silent write that replaces curated state with a re-derivation nobody asked for, exit 0 throughout. build:lib, lint and lint:ci all exit 0. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3951): amend B6's ledger to what was measured, and document the new flags The ADR gains a ledger amendment in its own correction style - the sixth wrong premise it records, found the same way as the other five, by measuring before building. B6 says the net guard count must fall. It rose: 62 -> 69, +7, measured from the epic's filing commit to origin/next. The attribution is the point, though. Five of the seven came from PRs unrelated to this epic, one was added by a phase of it, and the epic did retire something sub-file - #3884 removed a detector with an explicit "net: -1 detector, 0 added" ledger. Every named casualty is load-bearing, two already carry retractions in this same document, and a sweep of all 22 rules plus every scripts/lint-* found no provably dead guard. There is no honest way to make the count fall; forcing it would trade coverage for a number, which is the Goodhart outcome Decision 6 exists to prevent. The amendment also records that B6's own prescribed fix for one widening was inert. no-adhoc-markdown-parsing self-gates on its filename, so widening only the files: glob - which is what the criterion says to do - ships a rule that still returns {} for every new path. And #3426/#3239 are not reachable by that widening at all; their scans use line filters and split('|'), not the regex fingerprints the rule detects. The roster row tracked them against the wrong mechanism. Three roster rows updated from aspiration to fact: the two widenings are DONE with their measured counts, and lint-phase-enumeration-drift is marked RETAINED rather than "expected casualty - verify before retiring", because Phase 5 verified it and kept it. The rule Decision 6 should carry forward is stated plainly: a guard ledger is a claim about COVERAGE, not about COUNT. "Net count must fall" is measurable and wrong. "Every guard is reachable, and each retirement names what makes its defect unrepresentable" is the property that was actually wanted. CLI-TOOLS.md documents the optional --quick-id/--slug/--directory flags and says plainly that omitting them keeps the pre-#3356 row byte-identical, plus that the append no longer re-derives progress frontmatter. New features fragment (id 3951); FEATURES.md regenerated rather than hand-edited. Changeset is Changed, pr:0 pending backfill. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): correct four rows that pinned the lint rule's old narrow reach The remote suite came back RED with 5 failures, all in tests/eslint-rules.test.cjs. They are stale tests, not a regression: four rows assert that no-adhoc-markdown-parsing is inert outside src/*.cts, which is exactly the contract this deliverable changes. Confirmed by reading rather than inferred from the names - the row at :1981 used filename: 'tests/some.test.cjs' and filename: 'scripts/helper.cjs', the two roots the rule now covers on purpose. Worth recording WHY local gates missed this. npm run lint and lint:ci were green, and the touched test files passed standalone. Lint only reports violations in real files; these rows assert the rule's REACH using synthetic RuleTester filenames, so nothing but the full suite could see them. Local green on a rule change says nothing about the rule's own tests. Each row is rewritten with BOTH halves rather than flipped from valid to invalid: - the same fingerprint under tests/ or scripts/ is now flagged, with the right messageId - the negative space is preserved - the same fingerprint under a path outside all three roots (gsd-core/bin/lib/foo.cjs) is still NOT flagged The second half is the one that matters. Without it the rule has no boundary and nothing would catch an over-wide gate later, which is the mirror image of the bug this deliverable just fixed. Each row is renamed to state the current contract; the old names said "non-src/*.cts ... is not flagged" and would have been actively misleading once the bodies changed. Proven to test the widening rather than restate it: every flagged half was run against HEAD~2's pre-widening rule and does NOT fire there, then against the current rule and does. 12/12 on that probe; the full file is 178/178. Swept for the same staleness elsewhere and found none. require-subprocess-timeout's own "inert outside src/*.cts" row is untouched - that rule's gate was not widened here - and no-adhoc-regex-escape's test file already carries correctly-targeted rows. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): acknowledge the quick.md growth the attribution guard reported The full suite came back RED with one failure, and it is mine: 1 file(s) grew without an acknowledgment: quick.md grew 364 bytes gsd-core/workflows/quick.md is runtime-loaded emitted content, so correcting its false 'performs the equivalent write' claim trips emitted-attribution by construction. This is the acknowledgment, not a workaround - there is nothing to regenerate. The fragment names ONE path, which is the only one the guard reported. The four spent acknowledgments it also listed (audit-uat, plan-phase, progress, review) belong to other fragments whose ripple the base already absorbs; they are inert, not failures, and are deliberately NOT copied here - naming paths I did not change would make this record false in the other direction. Byte figure corrected before committing: the guard reported 37220 -> 37584 (+364), but origin/next has since moved and quick.md is 37232 there now, so the measured delta is +352. The reason text says so and names the base as a moving figure rather than pinning a number that is already stale. Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3951): move the quick.md growth ack to a trailer, delete the obsolete fragment The acknowledgment mechanism changed under this branch. Merging next brought in the redesign - it also deleted .github/workflows/ack-fragment-sweep.yml, which was in the merge status and which I did not register at the time - and the guard now says so directly: Add a trailer to a commit in this PR (never a new file). Emitted-Drift-Ack-Growth: quick.md - <why this growth is deliberate> So tests/emitted-drift-acks/3951-quick-append-equivalence.json is obsolete on arrival. A fragment file is no longer read by anything, and leaving it would be a dead record that looks like an active one. It is deleted here rather than kept "just in case". The byte figure moved again with the merge: 37232 -> 37596, +364. The earlier fragment said +352, measured before the merge auto-merged quick.md itself. The trailer carries no number, which is the better design - the figure was stale twice in two attempts. Refs #3951 Emitted-Drift-Ack-Growth: quick.md — #3356/#3951 replaces a false claim with an accurate one. Line 627 said the `quick-tasks-append` shortcut "performs the equivalent write" to the Step 7c row rendered above it; it did not, and that was the documented half of #3356 — with no quick id or task directory the route emitted a positional ordinal in `#` and an em-dash in `Directory`, a visibly different row. The corrected sentence has to carry three facts the original elided: what the shortcut actually writes when it has neither input, that this is honest behavior for its real caller (`fast.md`, which has neither), and how a caller with both now gets the byte-identical canonical row via the new optional `--quick-id`/`--slug`/`--directory` flags. Prose is the product here — an executing agent reads this line to decide whether the shortcut is safe for its case, and a shorter correction would either drop the flags (leaving the reader unable to act on the fix) or drop the limitation (recreating the false claim in gentler words). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3951): backfill changeset pr number Refs #3951 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-27 23:10:49 -04:00
Tom Boucher	9410f7e6e6	enhance(#3897): ADR-3473 §8.3 rungs 2-4 — runtime marker, derived Codex sandbox, short-form depends_on (#3941) ... * test(#3897): failing-first coverage for §8.3 rungs 2-4 ADR-3473 §8.3 has four rungs; #3883/PR #3896 shipped the first. This pins the other three RED before any fix. Rung 2 — the install marker has four readers and resolveRuntime is not one. resolveRuntime resolves GSD_RUNTIME > config.runtime > 'claude' and reads no marker at all, while bin/install.js writes one (#2297) and FOUR hand-rolled readInstallRuntimeMarker copies exist: src/model-resolver.cts:65 (cached, with test seams), hooks/gsd-agent-isolation-guard.js:112, and TWICE in hooks/gsd-cursor-subagent-start.js at :346 and :355. Four copies of one rule. Fixtures and seam names mined from PR #3382 rather than re-derived; it implemented this rung and was closed "not on the merits". Rung 3 — the sandbox map, and the fallback that was the real defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: - all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements — the map carries nothing the contract does not - 24 roles fall through `|| 'read-only'`, of which 16 declare Write or Edit So the map is redundant and the silent fallback is the defect. The maintainer chose to derive but hold those 16 at read-only pending the question of whether Codex enforces sandbox_mode or merely advises; HALT.md records it. T20 asserts the emitted sandbox_mode PER ROLE against a captured baseline, not in aggregate — an aggregate passes while one role silently widens, which is the proxy-instead-of-identity shape this repo names. T24 and T25 fail on a stale hold, so the hold list cannot rot into the subset map being deleted. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: `git log -S shortFormToId` returns only documentation commits. That was the wrong instrument. Direct inspection of sdk/src/query/phase.ts at 11918dcc3^ shows five occurrences, and the tests match that code rather than a guess at its semantics — including first-write-wins on a duplicate short form. T43 asserts at the consumer's output: the emitted `waves` map from the real CLI, which pre-fix collapses to {"1":[...]} because every short-form edge is dropped. A unit assertion on resolveDependencyId would have passed throughout this defect's life. Observed RED, this tree: rung 2 11/11 fail — no marker rung, no seams rung 3 T23,T24,T25,T26,T30 fail; T28 fails (validate agents passes a TOML whose sandbox_mode disagrees — it checks presence only) rung 4 T42,T44 fail; T43,T49 fail with waves collapsed to a single wave 1 Green and staying green: T20/T21/T22/T27 as captured baselines, #3885's unresolvable-token warning and wave-verdict suppression, and #3785's display-mapping passthrough. If the third tier over-reaches, those go red — that is their job. Disclosed weakness: T45 (a canonical id with no dash is not short-form indexed) cannot be isolated behaviorally, because planMap always masks it. It is a non-crash boundary pin, weaker than the other rows, and is recorded as such rather than presented as equivalent. Design: .gsd/phase/feat-3897-adr3473-83-rungs/40-design.md Test matrix: .gsd/phase/feat-3897-adr3473-83-rungs/50-test-matrix.md Decision: .gsd/phase/feat-3897-adr3473-83-rungs/HALT.md Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3897): §8.3 rungs 2-4 — one marker reader, a derived sandbox, the third depends_on tier ADR-3473 §8.3 has four rungs. #3883/PR #3896 shipped the first. These are the other three. Rung 2 — the install marker had four readers, and resolveRuntime was not one. resolveRuntime resolved GSD_RUNTIME > config.runtime > 'claude' and read no marker, while bin/install.js writes one (#2297) and four hand-rolled readInstallRuntimeMarker copies existed: src/model-resolver.cts (cached, with seams), hooks/gsd-agent-isolation-guard.js, and twice in hooks/gsd-cursor-subagent-start.js. model-resolver's was already the house idiom, so it was promoted rather than replaced: src/runtime-slash.cts now owns it, and model-resolver plus both hooks delegate. The hooks reach it through ensureRuntimeBuild(), the seam lint-hooks-runtime-build-seam enforces. No import cycle existed - checked both directions before moving anything. The marker is the THIRD rung: env > project config > marker > 'claude'. N1 was checked rather than assumed, and my first reading of it was wrong. A marker holding an unknown name comes back essentially verbatim, which looked like a validation gap. Measured against the env rung with the same inputs - including "../../etc/passwd" and "claude;rm -rf /" - the two are identical, because they share resolveRuntimeNameFromCandidates. N1 asks for exactly that, and it is met. The residual (the shared normalizer normalizes shape, it does not validate against the known-runtime set) is pre-existing on the env rung and plausibly deliberate, since a new runtime should not need a code change. The marker also does not widen the trust boundary in any real sense: it lives inside the install tree beside the code, so anyone who can write it can write runtime-slash.cjs itself. Rung 3 — the map was redundant; the silent fallback was the defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements. The map carried nothing the contract did not already have, so it is DELETED rather than reconciled. What was actually broken is `|| 'read-only'`, which silently under-granted 24 of 35 roles. 16 of those 24 declare Write or Edit and would widen under derivation. Per the maintainer's decision (HALT.md), they are held at read-only pending the question of whether Codex enforces sandbox_mode or merely advises. Emitted TOML is therefore byte-identical for all 35 roles - asserted per role, not in aggregate, because an aggregate passes while one role silently widens. The hold list self-invalidates. A hold whose role no longer derives broader fails, and so does a hold naming a role with no agents/<name>.md. Without that it would rot into exactly the hand-maintained subset map being deleted, and this commit's own ledger claim would become false over time. Both cases were proved by injecting them and watching them throw. Two committed tests asserted the deleted map's existence and contents. They were pinning the thing being removed, so the tests moved rather than the production code: the 11 role-value pairs survive as a test-local PRE_3897_CODEX_AGENT_SANDBOX baseline, and the assertions now drive the real derivation against real agents/*.md. The coverage is preserved; only its source moved out of production code. validate agents gains checkCodexSandboxPosture, mirroring the existing checkCodexModelPosture: each installed TOML's sandbox_mode must equal the role's expected value, failing with role, expected and found. It previously checked file presence and manifest completeness only, so a TOML whose sandbox_mode disagreed passed. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: git log -S returns only documentation commits. Wrong instrument. sdk/src/query/phase.ts at 11918dcc3^ carries five occurrences, and the implementation here matches that code rather than a guess at its semantics - including first-write-wins on a duplicate short form, deterministic from the sorted plan order. It resolves the bare plan number: depends_on: ["01"] now reaches 26-01-auth-hardening. That is a control-flow change, not a diagnostic one - plans that silently collapsed into a single wave 1 now execute in their declared waves, and execute-phase.md consumes those wave values. In-phase only, by construction: the map is built from this phase's rawPlans, so a same-named short form in another phase does not resolve. #3785's display-mapping passthrough and #3885's unresolvable-token warning and wave-verdict suppression are untouched and stay green. If the third tier had over-reached, those are what would have caught it. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open I introduced, and wire the posture check to its command Two blockers from review. Both are mine, and one is a security regression my own change created. 1. A held role could escape its hold by editing its own frontmatter. The Codex install loop set the sandbox identity from the agent's frontmatter `name:` field rather than from its filename, so the hold lookup keyed off a value the file itself declares: deriveCodexSandboxMode('gsd-doc-writer', <real file>) -> read-only deriveCodexSandboxMode('gsd-doc-writer-x', <same file, name: edited>) -> workspace-write deriveCodexSandboxMode('GSD-Doc-Writer', <same file, name: recased>) -> workspace-write What makes this a blocker rather than a nit is the DIRECTION. The deleted CODEX_AGENT_SANDBOX map had the identical lookup-key quirk, but it was an allowlist: an unmatched key fell back to read-only, which is safe. The new scheme derives workspace-write from the tool contract and uses the hold as a subtraction, so the same mismatch fails OPEN. I converted a fail-closed quirk into a fail-open one and did not notice; the isolated reviewer proved it by execution. Neither safety net caught it. validateCodexSandboxHolds only checks that <key>.md exists, never that a file's derived identity matches its key. checkCodexSandboxPosture looks the canonical source up by the installed TOML's filename, finds nothing for a renamed agent, and treats it as a custom non-roster agent — silently no violation. The identity is now the FILENAME STEM, which is what validateCodexSandboxHolds already validates and what an attacker editing frontmatter cannot change without renaming the file — at which point the existing validator catches it. The lookup is case-insensitive so a recase does not slip past either. The frontmatter name still drives the TOML body and filename, unchanged; only the sandbox identity moved. All 35 roster files were checked: name matches filename stem everywhere, so a stricter "they must agree or throw" invariant would have been safe against real content. It is deliberately NOT added — it would abort an install on a tampered file where emitting a correctly-derived read-only TOML is the safer outcome. Recorded as a fork rather than decided silently. 2. checkCodexSandboxPosture was exported and never called. cmdValidateAgents (src/verify.cts) called checkAgentsInstalled and checkCodexModelPosture only; grep for the sandbox check in that file returned nothing. So criterion 3 — "validate agents fails on semantic drift, not only on missing files" — was unmet, and `validate agents` behaved exactly as before. That is ADR-3473 Decision 2's named shape: a declared policy with no executor. It also meant the T28 test asserted at the helper's return value while the COMMAND stayed broken — the ADR-3180 Decision 4(b) failure this epic exists to close, committed by me while enforcing it elsewhere in the same epic. Now wired as an additive `sandbox_posture` field beside `codex_posture`, following the sibling precedent exactly. Drift is report-only, not a non-zero exit, because that is what checkCodexModelPosture does — two sibling posture checks disagreeing about whether a violation is fatal would be its own defect. The choice is recorded in a comment rather than left implicit. A consumer-output test now drives the real CLI and asserts on the emitted JSON, and was shown failing before the wiring and passing after. Also corrected a stale artifact: the design's Known limit L1 still claimed rung 3 was not in this deliverable, written while it was halted and false once the maintainer unblocked it. Verified after both fixes: the three bypass probes all return read-only, the per-role table is 35/35 byte-identical, and both hold self-invalidation cases still throw. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the marker rung, the derived sandbox, and the bare plan-number depends_on Reference: the runtime precedence ladder in docs/CLI-TOOLS.md gains the install marker rung; docs/COMMANDS.md documents validate agents' new sandbox_posture field; docs/reference/plan-md.md documents that depends_on accepts the bare plan number. Explanation: a docs/features fragment keyed id 3897, so it cannot collide with a concurrent PR hand-allocating a section number, regenerated into FEATURES.md. ADR-3473 §8.3 gains an ANSWER blockquote in the document's own correction style, recording what was measured and built against the section's 2026-08-26 correction - including the qualification that checkAgentsInstalled itself still checks presence only, and the semantic assertion lives in a sibling wired into validate agents rather than folded into it. No how-to. Both user-visible changes are zero-step: a non-Claude install resolving its own runtime, and plans executing in their declared waves, both happen without the user doing anything. docs/how-to/control-the-reported-host-runtime.md covers a DIFFERENT ladder (resolveReportedRuntime / agent_runtime) that this change does not touch, and was deliberately left alone rather than edited by association. No tutorial - nothing multi-step to walk through. docs/AGENTS.md unchanged: it documents Claude-side tools frontmatter, never Codex sandbox_mode, and the emitted tools contract did not change. The prompt layer documents depends_on only by example, not by schema, so nothing there needed editing - and few-shot-examples/plan-checker.md already showed depends_on: ['01'], which now actually resolves. Translated copies of plan-md.md are untouched; the project treats translations as community-maintained. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): move the sandbox derivation out of the installer, off the install path, and off a third parser The full suite came back with 26 failures across four files. Three distinct causes, mapped individually rather than assuming the first explained the rest. A. Requiring bin/install.js printed the GSD banner to stdout and corrupted `validate agents` JSON. Unexpected token '', "[36m ██"... is not valid JSON checkCodexSandboxPosture reached deriveCodexSandboxMode by lazily requiring bin/install.js, whose module load prints the ASCII banner. So the command emitted banner bytes before its JSON and every JSON consumer broke, including ten tests that predate this branch. src/ reaching into bin/ was backwards layering that happened to also be loud. The derivation now lives in src/codex-agent-toml.cts - the existing Codex TOML domain module, no new module and no six-gate ripple - and both bin/install.js and src/agent-install-check.cts import it. One owner, which is §8.3's rule applied to the fix for §8.3. B. The stale-hold throw fired on a legitimate partial source dir, and masked a security assertion. validateCodexSandboxHolds treated "this hold's .md is absent from the install SOURCE dir" as a stale hold and threw. A test fixture, or any partial install source, legitimately contains a couple of agents. Worse, it threw BEFORE the path-escape check, so a test asserting that a `../../evil` frontmatter name is rejected got my unrelated error instead of the traversal rejection it was written for. A fail-closed check of mine was hiding a real security check. The "no stale holds, shrink-only" invariant is a property of the repo's canonical agents/ roster, not of whatever directory an install happens to read. It is off the runtime path and enforced where it belongs, in the tests that already existed for it. A partial source dir now installs cleanly, and the evil-name case throws with its own escapes-configHome message again. C. T8 depended on ambient process.env state. The marker/env parity assertion round-tripped through live process.env. It now compares against resolveExplicitRuntime's already-exported dependency-injection parameter - deterministic and hermetic, same claim. Proven still falsifiable rather than assumed: with the marker rung's normalization temporarily bypassed the two rungs diverge ("codex\n../../etc/passwd" vs "codex-../../etc/passwd") and the assertion fails, then passes again once reverted. One correction folded in along the way. The first version of the move added private _extractFrontmatterAndBody/_extractFrontmatterField helpers to codex-agent-toml.cts - a THIRD copy of frontmatter extraction, where the graph already shows two (bin/install.js:2348, runtime-artifact-conversion.cts:893). Adding a third inside the epic whose thesis is one implementation per rule is not defensible. deriveCodexSandboxMode no longer parses anything: it takes (identity, toolsValue) and each caller supplies the tools value using the extractor it already has. Both helpers are deleted. The identity argument is still the filename stem, so the fail-open fix is untouched. Verified after all three: `validate agents --raw` emits parseable JSON with no banner and both posture fields; the four hold-bypass probes still return read-only; the per-role table is 35/35 byte-identical at 26 read-only / 9 workspace-write; the hold list is still 16. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): drop a dev-only transitive dep, make the derivation total, retire a stale fallback test Suite down to 7 failures from 26. Three more causes, mapped individually. A. My extractor import dragged in a script that does not exist in an installed tree. Cannot find module '../../../scripts/fix-slash-commands.cjs' Chain: src/agent-install-check.cts imported runtime-artifact-conversion.cjs, which requires command-roster.cjs, whose line 36 requires ../../../scripts/fix-slash-commands.cjs. That path exists in the repo and not in an install, so every test exercising a synthetic install dir died at module load. I picked that extractor for convenience without checking what it pulls in - the same mistake that produced the banner bug, one layer further out. agent-install-check now uses a single-purpose extractToolsLine on codex-agent-toml.cts. That is deliberately NOT a general frontmatter parser: we deleted those helpers a commit ago for good reason, and this reads one line. Verified from outside the repo root that requiring either module prints nothing and does not throw. B. A test pinned the deleted name-based fallback. 'defaults unknown agents to read-only' called generateCodexAgentToml with a fixture declaring tools: Read, Write, Edit. Under derivation an unknown agent with a writing contract correctly derives workspace-write - design row S6, a new writing role gets the contract, not the pin. The behavior it asserted was the silent fallback this rung deleted; identity no longer decides the sandbox. Replaced with two rows rather than a flipped string: no tools declared -> read-only (absence is not a grant), and Write/Edit declared -> workspace-write. Strictly more coverage than the row it replaces. C. The stale-hold check still threw per derivation call. Last commit took the roster-existence check off the install path, but deriveCodexSandboxMode itself still threw when a hold's role did not derive broader FOR THE CONTENT IT WAS HANDED - so it fired on any synthetic fixture for a held role. The throw is gone, and it cost nothing: if a held role's content does not derive broader, the hold pins read-only and derivation returns read-only anyway, so the hold is a no-op and there is nothing to fail about. The staleness invariant is a property of the real agents/ roster, and validateCodexSandboxHolds still enforces it there - confirmed against the real roster after the change, not assumed. deriveCodexSandboxMode is now total: every (identity, toolsValue) including undefined and null returns read-only or workspace-write, never throws. Verified: validate agents emits parseable JSON; the four hold-bypass probes return read-only; the per-role table is 35/35 at 26 read-only / 9 workspace-write. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): put the rung-3 decision in the shipped docs instead of pointing at an ignored path The ADR entry and the feature fragment both ended their rung-3 explanation with "see .gsd/phase/feat-3897-adr3473-83-rungs/45-decision-rung3-sandbox.md". That directory is gitignored (.gitignore:55), so the rationale for holding 16 roles at read-only was reachable only from the machine that produced it. A reader of the ADR got a pointer to nothing. Both now carry the reasoning inline: the criterion asks both that the sandbox derive from the declared tool contract and that no role gain a broader sandbox, and those cannot both hold, because a faithful derivation widens 16 roles the deleted map never listed and that fell through its silent read-only default. The resolution is derive-and-hold - the derivation owns the rule now, each hold is released as its enforcement question is answered, and a hold is reversible where a widened sandbox that turns out to be enforced is not. Checked before assuming this was a defect class: CONTEXT.md cites .gsd/phase/<slug>/40-design.md as its standard Design: provenance line in eight module entries, and four other shipped docs do the same. Citing a phase artifact is an established convention here, so those are left alone. What was wrong was specific to these two: they put load-bearing rationale behind the pointer instead of provenance. docs/FEATURES.md regenerated from the fragment via scripts/gen-features.cjs rather than hand-edited. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open, stop a silent mis-resolution, and read a declaration as a declaration Two orthogonal reviews on the shipped sha. Three of the findings are the same failure class this epic exists to close, committed inside it. 1. BLOCKER - the sandbox was decided for one identity and applied to another. bin/install.js derived sandbox_mode for the filename stem and then wrote the result to `${name}.toml`, where name comes from the file's own frontmatter. Make the two disagree and a HELD role's artifact goes wide: rename gsd-doc-writer.md -> gsd-doc-writer-v2.md, keep name: gsd-doc-writer -> stem is unheld, derives workspace-write, lands on gsd-doc-writer.toml add any gsd-*.md whose frontmatter name: is a held role -> clobbers that role's toml with workspace-write Both emit read-only on origin/next, because the deleted map was an allowlist and a miss fell back safe. This is a regression my change introduced. The previous review round moved the HOLD KEY off frontmatter to the filename stem and left the OUTPUT PATH on frontmatter; my own comment at install.js:6985 calls that value attacker-editable, four lines above the line that uses it as the filename. The decision is now made over BOTH candidate identities, most-restrictive wins: if either the stem or the emitted name is held, the mode is read-only. 2. MAJOR - hold matching was toLowerCase() only, so confusables escaped. Turkish dotted/dotless i, fullwidth, NFD, trailing space/NBSP/dot/newline, ./ and ../agents/ all slipped the hold and emitted workspace-write. Identities are now basenamed, trimmed of NBSP/zero-width/control characters, NFKC-normalized and lowercased - and anything still carrying a character outside [a-z0-9._-] is treated as suspicious and derives read-only. We do not enumerate confusables; every shipped roster file is ASCII, so refusing to widen on an identity we cannot recognize is fail-closed with no false positives on real content. 3. MAJOR - the short-form depends_on tier mis-resolved SILENTLY. shortFormToId keyed on the last dash-segment of any canonical id with no constraint that it is a plan number, so a phase holding 09-FIX-auth-PLAN.md made depends_on: ["auth"] bind at wave 2 with zero warnings. This is the worst shape in the epic: the unresolvable-token warning fires on a DROPPED token, so a MIS-RESOLVED one is invisible and the tool reports a confident wave assignment built from a wrong edge. A wrong edge is worse than a missing one. The segment must now match /^\d+$/, which is exactly the contract docs/reference/plan-md.md already documents. This tier was recovered verbatim from the retired SDK lineage, which carried the same defect; we are deliberately NOT preserving it bug-for-bug, and the comment says so, so the next reader does not "restore" it. 4. MAJOR - the derivation was reading a declaration as an absence. extractToolsLine read one line, so a YAML list-form tools: block returned only its first item. Two roster files use list form, and gsd-nyquist-auditor declares Write and Edit there - parsed as "- Read", found no write tool, and emitted read-only. Rung 3's headline claim is that sandbox_mode derives from the declared tool contract; that claim was false for 2 of 35 roles and materially wrong for 1. Reading a declaration as an absence is the silent-drop class this epic exists to close. Renamed extractToolsValue and taught it both shapes. gsd-nyquist-auditor now derives workspace-write and joins CODEX_SANDBOX_HOLDS as its 17th entry, per the standing derive-and-hold decision - so emitted TOML stays byte-identical at 26 read-only / 9 workspace-write while the hold list finally records every role that would widen. A previous pass declined this fix because it moved the count; that inverts the priority. Byte-identity is preserved THROUGH the hold, not by leaving a parser broken. Divergence check, because this is where that bug hides: both paths feeding sandbox derivation - install.js's emitter and checkCodexSandboxPosture - now route through the one extractor. The tools readers in runtime-artifact-conversion and install.js's other frontmatter call sites serve Claude-side emission and do not feed sandbox derivation. Also fixed, each real: the posture check's `found` used a naive whole-file regex where its own sibling uses the block-aware scanner, so prose inside developer_instructions produced a false violation; `found` skipped truncatePostureValue and leaked a 300-char value into validate agents output; deriveCodexSandboxMode's absolute never-throws claim was false for an object with a throwing toString; T49 could not falsify cross-phase leakage (its target phase had its own 01, so a globally-scoped map passed too); T20/N6 iterated a hardcoded table and pinned the FIXTURE size, so a 36th agent would be silently unchecked; three tests reimplemented the code they were testing instead of importing it; and T2-T4 deleted GSD_RUNTIME without restoring it. Verified: hold list 17, gsd-nyquist-auditor derives workspace-write unheld and emits read-only held, roster 35/35 at 26/9, depends_on ["auth"] no longer resolves while ["01"] still does, both identity-bypass cases and every confusable vector emit read-only. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the hold list is 17, and the reason the 17th was missing The count read 16 because the derivation could not read the declaration it claimed to derive from: the tools reader was single-line, so a YAML list-form tools: block returned only its first item and gsd-nyquist-auditor's declared Write and Edit were read as an absence. Both the ADR entry and the feature fragment now carry the corrected count and the reason for it, rather than a silently updated number. Deriving from a declaration you cannot parse is not deriving, and a flattering count is worse than a wrong one because it looks settled. docs/FEATURES.md regenerated from the fragment. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3897): backfill changeset pr number Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-27 15:19:01 -04:00
Tom Boucher	929e02cb2c	enhance(#3885): no silent swallow, and no verdict manufactured from dropped data (#3925) ... * test(#3885): failing-first coverage for the depth bound and the manufactured wave verdict ADR-3473 §8.5 says a swallowed failure may not become an authoritative-looking answer. Three families do exactly that today; this commit pins each one RED. Measured on this tree, 2026-08-27: intel query, .planning/intel/file-roles.json nested 12000 deep -> exit 1, "Error: Maximum call stack size exceeded" searchJsonEntries / matchesInValue carry no depth parameter at all. The MAX_JSON_SEARCH_DEPTH = 48 bound existed in the retired SDK lineage (sdk/src/query/intel.ts at 11918dcc3^) and the surviving .cts lineage never received it. same fixture nested 48 and 49 deep -> both return total=1 at exit 0, truncated=undefined Nothing distinguishes "searched to the bottom" from "stopped looking". query phase-plan-index, a plan whose depends_on names an unresolvable token -> warnings: ["Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"] The token is never mentioned. computeDependencyLevels drops the edge with `if (!resolvedDep) continue;`, every plan becomes a root, and the tool then reports the author's correct wave: as the thing that is wrong. countPhasePlansAndSummaries with fs.readdirSync throwing EACCES -> hasContext:false, indistinguishable from a phase that simply has no CONTEXT.md. context_read_error is undefined. The shapes these tests assert against, chosen here so the implementation has a target rather than inventing one later: `truncated: boolean` on the intel query result, `unresolved: Array<{plan, token}>` from computeDependencyLevels, and `context_read_error: string | null` per analyzed phase. Deliberately green, and they must stay that way — each stops the fix from over-firing: depth 48 is found and NOT flagged truncated (the ceiling is inclusive) a shallow miss reports no truncation (noise control, N1) 10,000 siblings at depth 2 are unaffected (the bound is DEPTH, N2) a genuine wave: mismatch on a fully-resolved DAG still warns (N3) a genuinely missing directory is absent, not an error the emitted depends_on display mapping still passes an unresolved token through verbatim — already pinned by the existing #3785 test, so no duplicate was added T31 asserts at the consumer's output per ADR-3180 Decision 4(b): it runs the real CLI and reads the emitted JSON, because a unit assertion on computeDependencyLevels would have passed throughout #3427's life. Design: .gsd/phase/feat-3885-no-silent-swallow/40-design.md Test matrix: .gsd/phase/feat-3885-no-silent-swallow/50-test-matrix.md Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3885): no silent swallow, and no verdict manufactured from dropped data Implements ADR-3473 §8.5. A failure or a gap in the input stops being absorbed into an output that reads as authoritative. The recursion bound, restored but NOT verbatim (src/intel.cts) MAX_JSON_SEARCH_DEPTH = 48 is threaded through searchJsonEntries and matchesInValue, which carried no depth parameter at all. The bound existed in the retired SDK lineage (sdk/src/query/intel.ts at 11918dcc3^) and the surviving .cts lineage never received it — §8.3's "a consolidation may not delete an invariant along with the surface that held it", demonstrated. Measured before: a .planning/intel file nested 12000 deep exits 1 with "Error: Maximum call stack size exceeded". Reachable from a project document. The original returned a bare `false` at the ceiling. Restoring that verbatim would trade a crash for a silent "no match" when the truth is "I stopped looking" — the same class this epic exists to close, and ADR-3473 Decision 4 forbids it. So the bound carries a truncation signal: nesting 47 -> found, truncated false nesting 48 -> found, truncated false (the ceiling is inclusive) nesting 49 -> not found, truncated TRUE nesting 12000 -> exit 0, truncated TRUE, no RangeError A shallow document that simply has no match reports truncated FALSE — the flag means "I stopped early", never "I found nothing", or it would be noise. The bound is on DEPTH: 10,000 siblings at depth 2 are unaffected. The dropped edge is named, and stops being blamed on the author (src/phase.cts) computeDependencyLevels dropped every unresolvable depends_on token with a bare `continue`. Each drop makes a plan a root, so the whole phase collapses to wave 1 — and cmdPhasePlanIndex then reported the author's CORRECT wave: as the thing that was wrong. Before: warnings: ["Plan 03-02: declared wave: 2 but depends_on DAG places it in wave 1"] After: warnings: ["Plan 03-02: depends_on token \"nonexistent-token-3427\" does not resolve to any plan in this phase — edge dropped, wave placement for this plan may be unreliable"] The suppression is PER PLAN, never blanket: a plan with a fully-resolved DAG and a genuinely wrong wave: still gets the mismatch warning. resolveDependencyId stays two-tier — the shortFormToId third tier is §8.3/Phase 6's rule and is deliberately not built here. The emitted depends_on display mapping still passes an unresolved token through verbatim (#3785). No artifact from failed inputs (gsd-core/workflows/review.md, #3352) A failed lane leaves no result file, so "every lane failed" is exactly "the aggregate JSONL has zero lines" — the gate condition already existed as a byproduct. REVIEWS.md is no longer written in that case, and the commit step is skipped with it. A budget-SKIPPED lane also leaves no file and is NOT counted as a failure. Per-lane output and non-empty .err are preserved to .review-diagnostics/ before `rm -rf "{run_dir}"` destroys the only record that the lanes failed at all; the commit step names one file, never a glob, so the diagnostics are not swept in. Unreadable is not absent (roadmap.cts, gap-checker.cts, init.cts x2) Four callers collapsed an EACCES on a phase directory into [] and reported hasContext:false — byte-identical to a phase that simply has no CONTEXT.md. Each now names the directory it could not read. A genuinely missing directory stays absent rather than becoming an error, which is what keeps the fix from over-firing. Fatal errno folded into a retry set: audited, no defect found Reported as a verified negative rather than padded with a change. withPlanningLock was fixed by #1884/PR #3472; acquireStateLock by #3776; atomicRenameWithRetry and estimate-cli's renameWithRetry are correct by construction — bounded set {EPERM,EBUSY,EACCES}, bounded attempts, and they return or rethrow the final error rather than swallowing it. estimate-cli's sole caller surfaces that rethrow as write_error in its JSON output. Manufacturing a diff to make the checkbox look worked-on is the Goodhart outcome Decision 6 exists to prevent. Disclosed: R46 (the commit step names one file, never a glob) is a real regression guard but is NOT independently failing-first — the commit fence is byte-identical pre- and post-fix, so it only fails pre-fix through its shared extraction dependency. Recorded rather than claimed as fail-first. Design: .gsd/phase/feat-3885-no-silent-swallow/40-design.md Test matrix: .gsd/phase/feat-3885-no-silent-swallow/50-test-matrix.md Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3885): escape untrusted tokens, and stop cleanup destroying unpreserved evidence Two review findings, both real, both in my own change. An isolated adversarial review found the evidence-preservation block never checked mkdir/cp exit status while `rm -rf "{run_dir}"` ran unconditionally in a SEPARATE fenced block. A disk-full or unwritable phase directory therefore still destroyed the only copy of the failed lanes' output — reintroducing the exact #3352 data loss this item exists to stop, inside the fix for it. Preservation and cleanup are now one block, because each fenced block is a separate execution and a shell variable cannot carry between them. mkdir -p and each cp are exit-checked; cleanup runs only when preservation succeeded, and a failure warns naming the intact run directory. "Nothing to preserve" is not a failure and still cleans up. Driven three ways: success removes run_dir, failure leaves it intact with the warning, nothing-to-preserve removes it. The failure is induced by a file-vs-directory conflict rather than chmod 0o000, which root bypasses. The new unresolved-depends_on warning embedded a user-authored token verbatim: warnings: ["Plan 03-02: depends_on token \"evil Plan 03-01: FORGED WARNING\" does not resolve ..."] The JSON wire form is safe, and the security reviewer judged it non-exploitable for that reason. It is escaped anyway through formatDiagnosticToken — the helper #3884 added one phase earlier for exactly this class. warnings[] is an array a consumer naturally prints line by line, and not reusing the sibling fix is the generative-fix-divergence shape this epic exists to close. The same treatment is applied to context_read_error / phase_dir_read_error, which embed a phase directory path a repository can choose, and to the fs error message, which echoes the raw path itself. Known limit L5 recorded: the bound is on DEPTH only. A 300,000-element shallow array yields a 14.5MB reply with truncated:false. Correct per §8.5 and per negative space N2, disclosed rather than left to be discovered. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3885): unreadable is not absent in intel.cts either, and a corrupt snapshot is not "no snapshot" Blocker from the round-2 isolated review, and it is my own inconsistency: this phase applied "unreadable is not absent" to phase directories and left it broken in the file it was already editing. chmod 000 .planning/intel/file-roles.json gsd-tools intel query <term> -> {"matches":[],"total":0,"truncated":false} exit 0 safeReadJson swallowed every read failure and returned null, so an EACCES was byte-indistinguishable from an absent file AND from a genuine no-match. Now it separates three states: ENOENT stays silently absent, because not every project has every intel file and intelQuery loops over all of them expecting misses; EACCES/EIO and malformed JSON are both surfaced naming the file. A corrupt intel file previously read as "no matches" too — same defect, same fix. Threading that outcome through the other three callers found something worse than the reported case. intelDiff returned no_baseline:true for a corrupt or unreadable snapshot — not a silent failure but an actively FALSE verdict, telling the caller they never took a snapshot when they did. That is §8.5's headline case, so it is fixed and tested rather than noted. intelStatus and intelApiSurface collapsed the same way; intelApiSurface additionally printed a "not yet populated" banner that was simply untrue. Every row is failing-first, including the absent-file ones — the field is new, so it does not exist pre-fix at all. Those rows are not pre-fix pins; they pin that the fix does not OVER-fire on the ordinary absent case, which is what would turn this into noise on every project lacking an intel file. IO failure is injected by monkeypatching fs and restoring in finally, never chmod 0o000 — root bypasses mode bits, so the reviewer's manual chmod repro is not reproducible as a test. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): build the pathological intel fixture as text, not by stringifying a nested object The remote runner came back red on Linux with two failures, both T4: deeplyNestedIntelDoesNotOverflowTheStack, while the same test passed on macOS. The product was never at fault. writeNestedFixture(12000) built a 12,000-deep JavaScript OBJECT and then JSON.stringify'd it. JSON.stringify recurses once per level, so it overflowed the TEST PROCESS's stack — the error was thrown before the CLI was ever spawned. Linux's container stack is smaller than macOS's, which is the whole of the platform difference. Measured, with the same document built as JSON TEXT so nothing in the building process recurses: depth=100 rc=0 truncated=true depth=5000 rc=0 truncated=true depth=12000 rc=0 truncated=true depth=60000 rc=0 truncated=true V8 parses this shape iteratively; only stringify recurses. The bound works at every depth tried. The fixture is now built by string concatenation. That is also the more faithful input — a real deeply nested JSON document on disk is exactly what the bound guards, where a stringified object was only ever a way to produce one. The depth stays 12000. Lowering it would have made the test pass by weakening it to accommodate a fixture bug, and 12000 is a legitimate pathological input the product handles. T4 remains a genuine fail-first: rebuilt against the parent of the commit that added the bound, the string-built depth-12000 fixture still drives the CLI to rc=1 with "Error: Maximum call stack size exceeded". A comment records why the fixture is text, so it is not "simplified" back into a macOS-green / Linux-red test. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3885): backfill the changeset PR number Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): normalize path separators before splicing into the workflow's bash CI red on one lane — test (windows-latest, 24, shard 3/3). macOS, Linux and the remote runner were all green. AssertionError: commit must name the single REVIEWS.md file; got: --files C:UsersRUNNER~1AppDataLocalTempgsd-3352-phasedir-mOKmuy/03-REVIEWS.md Every backslash in C:\Users\RUNNER~1\AppData\Local\Temp\... was eaten. The harness spliced an OS-native temp path into the extracted bash, and bash consumes \U, \A, \L and \T as escapes on an unquoted expansion. The same loss broke RUN_DIR, so "rm -rf" targeted a path that never existed and the run directory survived — which is the other two assertions. This is a fixture defect, not a product one, and that was checked rather than assumed. In production the phase directory is toPosixPath-normalized at every call site that serializes it (bin/lib/init.cjs:951, 1381, 1461, 1529, 1595), and the run directory is created by "mktemp -d" running inside the bash block itself (gsd-core/workflows/review.md:163), which emits POSIX-style output even under Git-Bash on Windows. Neither ever carries a backslash where the workflow reads it. The file's pre-existing #3034 harness splices raw native paths too, but only ever inside double-quoted assignments, so it never tripped this — my new harness followed that convention faithfully into the one place where it does not hold. Both now splice through toPosixPath from shell-command-projection, the established seam, which is a no-op on POSIX and mirrors what production does. No assertion was weakened. "commit must name the single REVIEWS.md file" and "the run dir must still be destroyed" still assert exactly that; only how the fixture supplies its path changed. Nothing is skipped on Windows — a t.skip() here would have hidden the question of whether the exposure was real, which is the question that mattered. Driven both ways: a synthetic C:\Users\RUNNER~1\... input reproduces the exact CI string when unfixed and yields C:/Users/RUNNER~1/... when fixed; a POSIX input produces a byte-identical shape, proving the normalization is idempotent. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3885): stop the harness making the deleted run dir its own cwd Windows shard 3/3 stayed red after the separator fix, on two assertions the separator fix never touched: AssertionError: the run dir must still be destroyed AssertionError: nothing to preserve is not a failure — run dir must still be removed The separators were a real bug and fixing them fixed the --files assertion. They were not this bug, and two CI cycles went into the wrong axis before I stopped converting path forms and looked at what the harness actually does. runWriteReviewsFlow passed cwd: runDir to runHook, so the child bash process's working directory WAS the directory the block under test then removes with rm -rf "$RUN_DIR". POSIX allows a process to delete its own cwd — verified locally, cd "$d"; rm -rf "$d" removes it cleanly — and Windows does not: a live process's working directory cannot be removed. So on Windows the directory survived and both assertions failed, on macOS and Linux it vanished and they passed. Nothing to do with slashes. Harness-only. Production never cd's into the run directory; every reference is by absolute path, and RUN_DIR is created by mktemp -d inside the bash block itself (gsd-core/workflows/review.md:165) rather than injected. review.md is unchanged. Fix: the child now runs with its cwd in an unrelated temp directory that the block under test never deletes. Neither assertion was weakened, and nothing is skipped on Windows — the tests in this file carry no platform guard and run there unconditionally, which is how this surfaced at all. Honest limit: the Windows failure mode cannot be reproduced on macOS, because POSIX permits the very thing Windows refuses. The diagnosis is grounded in that documented divergence and in the fact that only the Windows lane failed, but the green outcome on windows-latest is unverified until CI runs it. Refs #3885 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-27 04:12:47 -04:00
Tom Boucher	aa6c332b5a	fix(#3701): resolve next_phase from the roadmap, selecting the numerically lowest successor (#3852) ... * test(#3701): failing-first coverage for roadmap-order next_phase resolution * fix(#3701): resolve next_phase from roadmap order, keeping the disk scan for spelling and fallback * fix(#3701): select the numerically lowest successor in both scans, not the first row encountered * chore(#3701): backfill changeset PR number --------- Co-authored-by: sim <sim@local>	2026-08-25 08:58:10 -04:00
Tom Boucher	aaf47c5fc2	fix(#3691): let every reviewer lane take a prompt cap, and make the documented global resolve (#3832) ... * test(#3691): failing-first coverage for the reviewer prompt budget No prompt cap can reach any CLI reviewer lane, by any configuration. Two independent defects compound: all nine `transport: spawn` lanes declare `promptBudgetKey: null`, so `budgetFor` returns on its first line; and the documented global `review.max_prompt_tokens` is advertised in the schema manifest but declared nowhere, so the resolver never materializes it and `budgetFor`'s fallback is dead code. Adds to tests/reviewer-config-federation.test.cjs, which already owns the per-reviewer budget config-set/config-get idiom: - a CLI lane inherits the global cap (RED: reports null) - an http lane with the -1 sentinel inherits the global cap (RED: reports null) - the resolved review surface carries max_prompt_tokens at all (RED: absent) - per-lane overrides the global on a CLI lane - the sentinel boundary: -1 inherits, 0 means do-not-trim and must NOT read as unset, 1 is the smallest real budget — the regression budgetFor's own comment warns about - anti-tightening pins that must stay green: an empty config leaves every lane null, the three existing budgeted lanes are unchanged, and config-set still rejects a per-reviewer key naming something that is not a declared lane - a fast-check property over the resolution contract itself, with -1, 0 and non-finite inputs generated explicitly rather than left to chance Every row was reproduced by hand against the real CLI before being written, so the RED/GREEN split is observed rather than predicted. Refs #3691 * fix(#3691): let every reviewer lane take a prompt cap, and make the global resolve No prompt cap could reach any CLI reviewer lane, by any configuration. Two independent defects compounded. The nine spawn-transport lanes — claude, coderabbit, antigravity, cursor, gemini, codex, kimi-code, opencode, qwen — declared `promptBudgetKey: null`, so `budgetFor` returned on its first line and `review-lane plan` reported `promptBudget: null` no matter what was configured. Each now declares `review.max_prompt_tokens_per_reviewer.<slug>` with the same `-1`-is-unset sentinel the three local-server lanes already use. Separately, the central `review.max_prompt_tokens` was listed in the schema manifest's validKeys and documented as a supported setting, but declared nowhere — the resolved surface is built from capability declarations plus the defaults manifest, and neither carried it. `configGet` returned undefined and `budgetFor`'s documented fallback was dead code. It is now declared with a `null` default, exactly as docs/CONFIGURATION.md already specified, so the default behavior is unchanged: nothing configured means nothing trims. Two things the diagnosis had not predicted, found and fixed while implementing: - `REVIEWER_LANES` in src/review-lane-descriptor.cts is a second, hardcoded registration site that `mergeReviewerLanes` prefers over the capability registry on a slug collision. Editing only the capability files left every CLI lane still null. Both sites now agree. - The generated `gsd-core/bin/lib/capability-registry.cjs` was stale and masked the capability edits; regenerated with `npm run gen:capability-registry` rather than hand-edited. docs/CONFIGURATION.md said "Only lanes that declare a budget key accept one — today ollama, lm_studio and llama_cpp". That is false as of this change and is corrected rather than left to rot. The trim-versus-refuse question the issue raises is deliberately not taken up here: the refusal path already exists for the case that matters — a reviewer whose minimum set exceeds its budget is skipped rather than sent a misleading prompt — and trimming above that floor is the documented, shipped design of the feature. Changing it would alter behavior for the three lanes that already work, which is not what the issue asks for. Fixes #3691 * fix(#3691): document the new global and narrow an invariant this change obsoleted The full suite surfaced two consequences of giving every CLI lane a budget key. `review.max_prompt_tokens` entered CONFIG_DEFAULTS without a matching entry in the planning-config reference, which config-field-docs guards. Documented, including the sentinel semantics a reader needs: a per-lane value overrides the global, `-1` means unset and inherits it, and `0` means "do not trim that lane" and is not unset. The #2797 federation guard asserted that "a lane with no model flag and no host owns no config keys". That held only because budget keys existed solely on the three local-server lanes, all of which have hosts. A lane can now legitimately own a config key for a third reason, so qwen tripped it. The assertion is narrowed rather than weakened: such a lane must still own no model key and no host key, and may own at most its own `review.max_prompt_tokens_per_reviewer.<slug>` — never another lane's. That is strictly more specific in the dimensions that still matter. Proven to still bite: hypothetically giving qwen a `review.models.qwen` key fails it with `model/host: review.models.qwen`. The name and comment cite #3691 for why the premise changed, so a reader sees a deliberate narrowing, not erosion. Checked the sibling assertions in that describe block; the other three do not rest on the obsolete premise and are untouched. Refs #3691 * fix(#3685): port the write-flag content-change contract to its three sibling sites #3685 fixed `phase complete`'s `roadmap_updated` / `state_updated`, which reported `fs.existsSync(path)` rather than whether the transaction wrote anything. Three sibling sites carried the identical defect and are ported here. - `cmdPhaseRemove` reported `roadmap_updated: true`, hardcoded. `updateRoadmapAfterPhaseRemoval` now returns whether the content changed and the flag reports it. #2640/#2974 already fixed `state_updated` at this same call site and left this one behind, so the correct shape was adjacent. - `cmdMilestoneComplete` reported `state_updated: fs.existsSync(statePath)` — byte-identical to #3685's bug in a different command. - `cmdMilestoneComplete` reported `milestones_updated: true`, hardcoded, never consulting the MILESTONES.md write. `gsd-core/workflows/remove-phase.md:100` extracts `roadmap_updated` for display and never branches on it, so the flip from always-true to content-based changes no workflow behavior. Verified by reading the step, not assumed. One trap found while implementing: the obvious in-memory `finalContent !== originalStateContent` comparison — copying `cmdPhaseComplete`'s shipped shape verbatim — gives a FALSE POSITIVE for milestone completion. `platformWriteSync` normalizes Markdown at write time, and the milestone-closure transform regenerates `## Current Position` fresh on every call, so its pre-normalize output always differs from the already-normalized file on disk even when the persisted bytes are identical. The comparison is therefore made against the post-write on-disk content. `cmdPhaseComplete`'s own comparisons are left untouched — their repeat-no-op tests pass, so they are not exposed to this artifact. `milestones_updated` has no reachable no-op: the MILESTONES.md write unconditionally appends an entry every call. Only the true direction is pinned, documented inline rather than faked with a passing test. Refs #3685 * fix(#3685): compare write-flag content through the writer's own normalizer An independent reviewer disproved a claim made while porting #3685's contract to its sibling sites: that `cmdPhaseComplete`'s comparisons were not exposed to the Markdown-normalization artifact already diagnosed in `cmdMilestoneComplete`. `platformWriteSync` normalizes on write — CRLF stripped, blank-line runs collapsed, a blank line inserted after a heading, a single trailing newline enforced. Every flag that compares the PRE-normalization in-memory string against the on-disk pre-image can therefore report a change when the persisted bytes are identical. `cmdMilestoneComplete` had been worked around by re-reading the file after the write; the other sites compared raw strings. All of them now go through one exported seam, `contentChangedAfterNormalize(filePath, before, after)`, which normalizes both sides exactly as the writer does. That removes the extra disk read the milestone workaround needed, and makes the sites agree by construction rather than by four independent implementations of one rule — the divergence the repo names as an anti-pattern. Reachability, stated precisely rather than uniformly: the seam is load-bearing at `cmdPhaseComplete`'s `roadmapUpdated`, `requirementsUpdated` and `stateUpdated`, where section-rewrite logic genuinely regenerates content into a different-but-normalization-equivalent shape. At `updateRoadmapAfterPhaseRemoval` it is defense-in-depth: the no-match branch never reassigns `content`, so the raw comparison was already correct there. The first analysis claimed the reverse; this is the corrected finding. Also fixes an unsound test premise the remote suite caught. The byte-identity precondition in `roadmap_updated is false when ROADMAP.md comes out byte-identical` asserted against a hand-authored, un-normalized fixture — so the very first write reformatted it and the file could not come back identical. The fixture is now written already-normalized, so the assertion compares a normalized pre-image against a normalized post-image and still fails if the flag regresses to a hardcoded `true`. Not platform-specific; it reproduces on macOS too, and the earlier local check simply never exercised it. The sibling true-direction and milestone tests were checked for the same premise and do not share it — they assert `notEqual`, or compare two post-write states produced through the same normalizing seam. Refs #3685 * chore(changeset): backfill PR number for #3691 fragment --------- Co-authored-by: sim <sim@local>	2026-08-24 19:39:51 -04:00
Tom Boucher	7a41248c4f	fix(#3685): report phase-complete write flags from the transaction, not the filesystem (#3826) ... * test(#3685): failing-first regression coverage for phase-complete write flags `phase complete` reports `roadmap_updated`/`state_updated` from `fs.existsSync(path)`, so both read `true` whenever the file merely exists — including when the transaction wrote nothing. Add the regression tests that prove it, plus the negative-space and true-direction pins, before the fix. New in tests/phase.test.cjs: - roadmap_updated is false when the transaction rewrites nothing (FAILS today) - state_updated is false when the transaction rewrites nothing, and stays false on a third consecutive run (FAILS today) - both flags are true when the transaction genuinely rewrites (pins the true direction so the fix cannot be tightened into always-false) - each flag stays false when its file is absent The STATE.md cases pin the clock via GSD_TEST_MODE + GSD_NOW_MS (src/clock.cts:43-70) because syncStateFrontmatter stamps a millisecond-resolution `last_updated:` on every write, which would otherwise make the no-op unobservable. Also strengthens four pre-existing `=== true` assertions on these fields that passed vacuously: each now pairs the flag assertion with a content-changed assertion against a pre-call snapshot, so the `true` is earned. Refs #3685 * fix(#3685): report phase-complete write flags from the transaction, not the filesystem `phase complete` computed `roadmap_updated` and `state_updated` as `fs.existsSync(path)`, so both read `true` for any project that had the file at all — including a run that rewrote nothing. The flags are the only signal a caller has that the rollup landed, so a no-op was indistinguishable from a successful write and a stale ROADMAP went unnoticed until something downstream read wrong numbers. Both flags now reflect whether that file's content actually changed in the transaction, computed at the existing `writes.push({filePath, before, after})` sites — the same contract `requirements_updated` has honored since #2316-3, and the same correction #2640/#2974 already applied to `phase remove`. Nothing about what gets written changes; only what gets reported. Fixes #3685 * chore(changeset): backfill PR number for #3685 fragment --------- Co-authored-by: sim <sim@local>	2026-08-24 18:05:35 -04:00
Tom Boucher	107eb8c1d9	feat(#3753): run docs guards on the PR that changes the docs they read (#3787) ... A PR whose diff is entirely under docs/ runs zero tests, so a guard whose INPUT is shipped prose cannot protect the PR lane of the diffs it exists to check. Its only firing opportunity is after merge, on the shared branch -- which is how next went red on dacae9273 while the PR that caused it (#3746) was green on every check. The docs-lint job in .github/workflows/docs-required.yml -- an ALREADY-REQUIRED context -- now selects and runs the docs guards that read the specific docs files the PR changed. scripts/docs-guard-registry.cjs test file -> the docs paths it reads (63) scripts/select-docs-guards.cjs pure (changedPaths, registry) -> test files scripts/lint-docs-guard-registration.cjs drift guard, wired into lint:ci scripts/ci-test-scope.cjs is NOT touched -- `git diff origin/next --` on it is empty -- so #764's saving stands and its 21 pinning tests are untouched. Selection: exact path; trailing-slash directory prefix (boundary-checked -- docs/adrenaline.md does NOT match docs/adr/, which a naive startsWith gets wrong); and '*' for the 6 entries that walk docs/ generally or read a computed path. Unknown maps to '*' -- guessing narrow is how a guard silently stops running. Measured: a typo fix selects 6 of 63; docs/AGENTS.md selects 12; docs/COMMANDS.md selects 18. Four things this got wrong first, each found by an independent reviewer or by probe, and each having been asserted safe in a comment: 1. The registry started as a RULE in ci-test-scope.cjs's RULES, on the theory that classify()'s !codeChanged normalization made it inert. True for docs-ONLY diffs; false for MIXED docs+code diffs, where codeChanged is true and the normalization never runs: node scripts/ci-test-scope.cjs --files "docs/a.md src/semver.cts" with the RULE: 25 targeted_tests origin/next: 3 targeted_tests Category error: RULES is the scoped lane's input; a docs-guard registry is a lane manifest for a consumer that never calls classify(). Extracted; pinned by value. 2. The second attempt was a dedicated workflow with paths: [docs/**]. Such a workflow never reports on a non-docs PR, so it can never be a required context without hanging every non-docs PR -- and a non-required check does not block a merge, so the guard would have been advisory and #3753 unfixed. docs-required.yml already has no paths: filter, already supplies the required docs-lint context, already computes docs_changed, and already ran one docs guard gated on it. Generalizing that step needs no ruleset edit at all. 3. The registry and the drift lint were built from ONE path-segment heuristic, so both were blind identically -- and blind at the guard that motivated the issue. The reader-call regex required a character BEFORE its keyword, so a callee named exactly read( / load( / parse( / doc( / file( / content( could never match; and only an INLINE path.join(ROOT,'docs','X.md') argument was caught, missing the two-step-via-variable form -- the MAJORITY spelling -- plus template literals and concatenation. Detector 1 fired on 14 of ~450 files, so 35 genuine guards sat unregistered while the lint reported 0 violations, including cursor-reviewer (reads docs/COMMANDS.md, asserts .includes('--cursor')) and inventory-headings-countfree. The "accepted blind spot" this shipped with was the common case, not a fringe. 4. With detection fixed the true population is 115 files: 63 genuine guards, 52 incidental. Running all 63 in a REQUIRED check on a one-line typo fix is the cost #764 exists to avoid -- install.test.cjs is 7840 lines and reads exactly one docs file, docs/AGENTS.md, for its frontmatter. Dropping it reproduces the bug; running it for a typo elsewhere is waste. Hence the map. Then a second review round found six more, all fixed here: - fragment-single-edit-propagation.install.test.cjs was EXEMPTED as "overlay fixture only". False: it reads the real docs/registries/eos.json and asserts on a registry entry name, and reads the real ADR-0001 and asserts its H1. A docs-only PR touching either would have gone green and red next -- #3753 shipping again, from inside the fix for it. Now registered against both paths, and all 52 remaining exemptions were re-audited one by one. - The SUITES-collision guard compared RAW registry keys, but run-tests.cjs strips a leading `tests/` BEFORE its suite check. So it caught 'all' and missed 'tests/all' -- the only spelling that can actually occur, since every key carries the prefix. One typo would have run all 824 test files inside the required job. Now normalized the same way run-tests.cjs normalizes. - The lint failed OPEN on an unreadable tests dir or candidate file: 0 violations, ok:true. A guard that cannot read its input must never report success. - The exemption ratchet gated identity only, so a baselined file that later STARTED asserting on shipped docs stayed exempt silently -- 52 permanently blind files. The baseline now fingerprints the docs paths each exempted file references and fails when that set changes, naming what changed. - The exemption marker was still honored inside a multi-line template literal in the header window. The scanner now tracks template-literal and block-comment state. - `git diff --name-only | grep '^docs/'` silently dropped C-quoted non-ASCII docs paths, making docs_changed=false a green zero-guard check. Both call sites now pass -c core.quotepath=false. - The run step was gated on hashFiles(), which a force-committed .docs-guard-tests.txt would satisfy. The step now rm -f's both scratch files first and gates on an output it sets itself. Three empty states, deliberately distinct, because conflating them rebuilds #3753: an empty or malformed registry HARD-FAILS; docs changed with no guard covering them logs and skips; no docs change is already gated. The middle state must never be expressed as an empty --files-from, which prints `no tests in suite "all"` and exits 0 -- a green check that guarded nothing. With the current registry that state is unreachable, because the six '*' entries always match; the branch is kept as defensive handling for a future registry and says so. timeout-minutes: 15 bounds the required job against a hanging fork-supplied test; it had none. npm ci was added because the job never installed dependencies -- the previous single-file step got away without it, the registry does not. docs/contributing/docs-guard-registration.md documents the rule, following its sibling cross-platform-portability-rules.md, and CONTRIBUTING.md's CI Test Quality Checks table links to it. It is also load-bearing: without a docs/ file in the diff this PR would not have triggered its own lane, shipping an unexercised change to a required check. One unrelated fix, included because this PR surfaced it and CLAUDE.md forbids deferring a defect found while working. On this branch's first CI run, `full test (windows-latest, 24, shard 3/3)` was CANCELLED at exactly 30 minutes; tests were still passing 0.8s before the cancel, so it is a wall-clock timeout, not a hang, and a cancelled job reddens `Required tests`. The cause is not this PR's test file, which costs ~60ms. Shard composition is unstable: adding ONE file to the unit suite reshuffled 115 of 268 files between shards, and shard 3 drew a heavier mix. Underneath that is a real pre-existing defect. tests/ci-test-job-timeout-budget.test.cjs requires every lane's budget to be >= 1.5x its MEASURED cost -- "a lane that got slower must be re-budgeted, not excused" -- and its test-full entry recorded 19m from a windows-22 shard. That is stale. Measured on `next` with none of this PR's changes present: 26m18s (run 32614439702, windows-latest/24 shard 3/3), 23m36s and 23m17s on shard 2/3. So the lane costs ~26m and the 30-minute cap carried 1.14x headroom, not 1.5x. The gate had been out of compliance with its own rule; this PR was merely the file addition that reshuffled shard 3 past the cliff. Fixed as that file prescribes: measuredMinutes 19 -> 27 with fresh evidence, and test-full timeout-minutes 30 -> 45. The rule's minimum for 27m is 41; 45 is deliberately above it because the reshuffle means per-shard worst case moves run to run, and a budget pinned to the exact minimum would be re-breached by the next test file anyone adds. Only that one job's timeout changed; test.yml's scope, matrix and steps are untouched, so #764's saving is unaffected. Raising that cap let the Windows shard finish (28m45s, inside 45) and uncovered a real failure the 30-minute cancel had been masking: `new quick-task branch branches off origin/main (#2916)` died with `outcome=timed_out exitCode=null`, SIGTERM, at the 15000ms bound. tests/quick-branching.test.cjs:149 `runStep` runs a `#!/usr/bin/env bash` script executing MULTIPLE git commands, but was bound to GIT_TIMEOUT_MS (15000) -- the norm for a SINGLE git plumbing call. tests/helpers/timeouts.cjs already documents this exact failure and exists to fix it: HOOK_FANOUT_TIMEOUT_MS was created after PR #3285 recorded "outcome=timed_out exitCode=null at exactly the 15000ms probe bound while every other lane passed the same commit", and calls that "a bound sized for the wrong class, not a slow machine". Our failure is that case verbatim, so both sites move to the class norm rather than to a bigger number. The same class also failed on `next` itself 21 hours earlier -- run 32608945654, windows-latest/24 shard 1/3, `plan touching only src/ in a submodule project keeps worktree isolation ENABLED` -- where tests/worktree-safety.test.cjs:5845 `runGate` fans out to `git config --file .gitmodules` under a hardcoded 30000. Fixed too, since it is a defect in the tree regardless of which branch surfaced it. A survey of the whole tests/ tree found the same class-mismatch at further bash fan-out sites bound under 60000ms, and the maintainer approved sweeping them rather than leaving them latent to surface the same way one at a time. 16 fan-out sites across 16 files now use the class norm. The sweep is class-correctness, not raising numbers until things pass. Sites were moved ONLY where the bash body demonstrably spawns something (git, node, npm, a CLI); self-contained shell snippets were left where they are, and are listed as deliberately unchanged: pure if/printf bodies (copilot-install), pure array/case builtins (code-review-pipeline-regression:638), a documented pure-shell gsd_run stub (host-integration), single-process hook calls (workflow-guard:222/271/302), and a deliberately tight 5000ms fast-check hook (gsd-write-guard.property). Nothing was lowered. process-seam.test.cjs:513 (literal 300) is untouched on purpose -- it tests timeout BEHAVIOR, so raising it would destroy what it asserts. Shared file-level constants were the trap here, and were handled per file rather than by redefinition: GIT_TIMEOUT_MS has ~15 users in git-base-branch and only 1 is a fan-out; WORKTREE_TIMEOUT_MS has 16 users in worktree.test.cjs and 3 are; PROBE_TIMEOUT_MS has several in three more files. In each the CALL SITE was changed and the constant left alone, so no single-plumbing-call site silently inherited a 60s bound. The one exception is hooks-opt-in.test.cjs, where HOOK_TIMEOUT_MS has exactly one consumer -- spawnHook, the fan-out itself -- so redefining it is identical in effect and reads better. Only two of these sites have actually been observed failing. The rest cite that shared class and those two run ids rather than inventing evidence of their own. Co-authored-by: sim <sim@local>	2026-08-23 21:21:21 -04:00
Tom Boucher	0f417aa6d0	fix(#3584): the verb owns the count token and nothing else (#3635) ... * test(3584): failing-first coverage for Plans-line trailing text roadmap update-plan-progress preserves trailing text only when the line begins with a canonical count token. Every other phrasing — including the TBD value the shipped template itself suggests — is replaced to end-of-line, and a sentence wrapping onto a second line has its first line deleted, leaving the continuation standing alone so the roadmap asserts something nobody wrote. Exit 0, updated:true, and the diff reads as a routine count bump. These tests fail on that, and pin the arms that must keep working: the template placeholder is still replaced, the #2853 token-plus-annotation path is unchanged, CRLF is neither stranded nor duplicated, and a run that leaves the line alone still updates the Progress table and checkboxes rather than becoming a no-op. * fix(3584): the verb owns the count token and nothing else RED proven at 105bdf7c: 7 failures — the preserving cases (freeform prose, wrapped continuation, TBD, CRLF) failed while the template-placeholder and #2853 token arms passed on base. The trailing-text guard fired only when the line began with a canonical count token: dropped the rest of the line whenever the regex's count group did not match. #2853 fixed end-of-line truncation on that one path only. The in-code comment justified the rest as 'the fresh-template bracketed placeholder or other freeform guidance, not user prose' — a heuristic that misreads ordinary human phrasing and destroys even TBD, the value the shipped template itself suggests at templates/roadmap.md:37. The sharper failure was the wrapped sentence: only the first line is inside the match, so the verb deleted line one and left line two standing alone, leaving the roadmap asserting something nobody wrote — at exit 0, updated:true, in a diff that reads as a routine count bump. Inverted the default into three arms. A real count token is rewritten with its annotation preserved (unchanged, #2853). A bracketed placeholder is detected POSITIVELY and replaced. Everything else — freeform prose, TBD, a wrapped sentence's first line, an empty value — returns the match untouched. That last arm resolves the wrapped case by construction: an untouched first line cannot orphan its continuation. Positive detection is the load-bearing part. Implemented as 'not a count token, therefore disposable', rows 1-3 come straight back; the detector instead asks whether the value IS a bracketed placeholder. Leaving the line alone does not make the verb a no-op: the phase checkbox, the Progress-table cells and the plan-checklist row still update in the same run, and that is asserted. CRLF is unaffected in every arm — the pattern's [^\r\n]* never consumes the \r, so it sits outside the match regardless of which arm runs. Fixes #3584 * fix(3584): detect the template placeholder by its text, not by its brackets Two defects in the arm-2 detector shipped in fc23e49e, both found in review. Finding A: isBracketedPlaceholder asked only whether the trimmed value was wrapped in [...]. Brackets are ordinary prose punctuation in a roadmap, so any hand-written bracketed note — '[Deferred pending re-scope]', '[blocked on #1234]' — was classified as the fresh-template placeholder and destroyed. That is the very defect #3584 is about, reintroduced one arm over. The detector now matches the placeholder's TEXT (/^\[\s*Number of plans\b[\s\S]*\]$/i), so it recognizes the shipped template value and its short form and nothing else. Finding B: the count group matched '\\d+\\s+plans' only. The plural is not the template's own output shape — templates/roadmap.md:62 ships '1 plan' — so a single-plan phase fell through every arm and its line froze permanently, never updating again. Widened to 'plans?'. This one was introduced by the arm-3 default: before it, the singular fell through to the old replace-everything path and at least stayed current. Cases 11-14 cover both: a bracketed human note preserved, the short placeholder still replaced, '1 plan' rewritten, and '1 plan (annotation)' rewritten with the annotation intact. Verified against the live binary, not just re-read. Also converted all 15 cases in this block from try/finally to t.after(), per CONTRIBUTING.md:356-370 which bans try/finally in test bodies. The existing #2853 block above is untouched — it is not in this change's scope and its conversion is not this fix's concern. Refs #3584 * chore(3584): add changeset fragment Fixed-type fragment for the roadmap Plans-line trailing-text fix. pr:0 placeholder, backfilled once the PR number exists. Refs #3584 * chore(3584): backfill changeset PR number (#3635) --------- Co-authored-by: sim <sim@local>	2026-08-18 16:39:17 -04:00
Tom Boucher	0c00ef4a6d	fix(#3572): keep phase remove's STATE.md write single-block and drop the removed heading (#3594) ... * test(#3572): pin single-frontmatter contract for phase remove STATE.md writes Failing-first regression for #3572: when the removed phase has a directory and the body lacks Total Phases/of-N, cmdPhaseRemove's no-op-guard bypass prepended the count field to the WHOLE file — before the opening fence — corrupting STATE.md into two frontmatter blocks. Rows also strengthen the #2640 coverage (whose first-match assertions pass even on a corrupted file) and pin the issue's ROADMAP-only control. * fix(#3572): keep phase remove's STATE.md write single-block and drop the removed heading from ROADMAP Two defects in the decimal-phase removal path: (1) the #2640 no-op-guard bypass prepended 'Total Phases: N' to the WHOLE file content, landing it before the opening fence and corrupting STATE.md into two frontmatter blocks; the field now inserts at the top of the BODY, after the closing fence (EOL-aware, frontmatter-less files unchanged in behavior). (2) updateRoadmapAfterPhaseRemoval matched the raw query token ('1.1') against the normalized zero-padded heading ('Phase 01.1:'), so the removed phase stayed in ROADMAP and the resync counted it; the heading, checklist, and progress-row matchers are now zero-pad tolerant, which also covers unpadded integer headings. * fix(#3572): clamp phase-count decrements at zero; harden EOL detection; pin controls Review findings: a stale 'Total Phases: 0' could decrement to -1 on the next removal (both the field and the 'of N' phrase now clamp at 0); insertStateBodyFieldAtTop detects EOL from the first line ending so an LF-dominant file with a stray CRLF cannot fall through to the raw prepend; the issue's insert-alone control is pinned; row 1 pins the body-field value (dir-count provenance) alongside the roadmap-derived frontmatter count. * fix(#3572): keep CRLF endings intact in the body-field insertion Green-run failure root cause: splitting on '\n' but re-joining on a detected '\r\n' doubled every carriage return in CRLF files. Split and join uniformly on '\n' so each '\r' stays attached to the line it terminated. * chore(#3572): add changeset fragment * chore(#3572): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-17 12:58:25 -04:00
Tom Boucher	c5b83cb050	chore(#3560): delete two unreachable workflows, gate workflow reachability in lint (#3564) ... * chore(#3560): delete two unreachable workflows, gate reachability in lint discovery-phase.md and plan-milestone-gaps.md shipped to all 19 runtime install trees with no command, agent, or skill referencing them. plan-milestone-gaps' command was deleted by #2790 and the workflow was left behind; discovery-phase's own header claimed a caller in plan-phase.md's mandatory_discovery step, and that step does not exist — plan-phase.md contains zero occurrences of "discovery". docs/INVENTORY.md asserted discovery-phase.md was an alternate entry for /gsd-new-project. new-project.md never referenced it. The row and the matching note sentence are removed across all five locales rather than corrected. Adds rule 6 to lint-command-contract: every shipped workflow must be reachable from a loader, walking the transitive closure over the three reference shapes this repo uses. The closure seeds ONLY from commands/agents/skills, so a workflow that references only itself and a pair that reference only each other are both correctly reported rather than satisfying themselves; a visited set makes reference cycles terminate. The measure is a mention in a LOADER — docs/ and install-tree fixtures deliberately do not count, because scan.md proved a file can be documented and shipped while entirely unreached. Ships blocking, not report-only: #3561 is in this branch's base, so the tree reports 0 unreachable from the start. Closes #3560 * test(#3560): drive rule 6 end-to-end, sweep a stale allowlist, update ADR-0002 Review findings. Rule 6 had no end-to-end coverage: the tests exercised the pure closure with in-memory data, so the wiring — file collection, exit code, diagnostic — was unproven, and #3560's acceptance list explicitly wants a fixture showing the rule FAILS on a planted orphan. Adds an optional --root to lint-command-contract (default behavior unchanged) and four tests driving the real CLI through the process seam against a temp fixture: clean=0, planted orphan=1, orphan referenced only from docs/=1, orphan reachable transitively=0. The docs/ case is what pins the Goodhart defense — a mention outside a loader must not confer reachability. Deletes two tests that were byte-identical to a third and could not assert anything loader-specific, since the closure is source-agnostic by design; that distinction lives in the lint script's file collection and is now covered above. Removes a stale ALLOWLIST entry for discovery-phase.md in planner-language-regression — the exact sweep-miss class rule 6 exists to catch, found in the PR that adds the rule. ADR-0002 described five per-file frontmatter checks; rule 6 is a repo-level reachability graph, so the Decision section now says so. Refs #3560 * test(#3560): cut the bug-3298 test pin on the deleted plan-milestone-gaps workflow The remote runner went red with four failures: tests/phase.test.cjs asserted the plan-milestone-gaps workflow exists and checked its mkdir patterns, so deleting the file broke the test that pinned it. This is the fence the epic describes — the content-sync test IS what keeps an unreachable file alive — and cutting the coupling is what makes the deletion safe. Removes only that arm. The bug-3298 block guards three workflows against phase-dir prefix drift; the import and add-backlog arms and both shared mkdir-pattern helpers are untouched. Worth recording where the sweep failed: my reachability walk covered commands, agents, skills, gsd-core and docs, and lint-removed-but-needed covers .github/workflows, gsd-core, docs and package.json. Neither looks at tests/, so a test-pinned deletion is invisible to both and surfaces only on the remote runner. The how-to added by this PR names that gap explicitly so the next deletion searches tests/ by hand. Refs #3560 * docs(#3560): add a how-to for resolving unreachable-workflow findings * chore(#3560): backfill changeset pr number to 3564 --------- Co-authored-by: sim <sim@local>	2026-08-15 23:29:30 -04:00
Tom Boucher	59e7a677fe	fix(#3511): scope every phase-directory scan to the phase it belongs to (#3535)	2026-08-15 07:02:33 -04:00
Tom Boucher	411196bc3a	refactor(#3471): one enforcement point for the empty case, and reports that match the disk (#3519) ... * refactor(#3471): one enforcement point for the empty case, and reports that match the disk Implements ADR-3408 section 8.5 and section 8.4's residue (folded in when Phase 3 closed as subsumed). Four items, and two findings the design did not predict. FINDING 1 — the guards could not simply be deleted, as the design instructed. state sync and REGENERATE_STATE never run applyStatePreservation at all, so those six conditions were their ONLY empty-field fallback. A baseline probe on the unedited tree confirmed unconditional deletion drops current_phase, current_phase_name, current_plan, stopped_at and paused_at from a blank-body STATE.md on state sync — breaking the byte-identical requirement section 8.3 grants those two sanctioned-permanent exceptions. They are now GATED, not deleted: on for the exceptions, off for the write seam, where an empty derived value finally reaches the executor unmolested. FINDING 2, the more serious one — there was a FOURTH encoding of this policy. The pre-existing #2202 unknown-key carry-forward loop independently restored the same six fields whenever derivedFm lacked the key, completely neutralizing the fix. It is named nowhere in the ADR, the design, or three prior phases. It was found only because a probe that should have passed did not: the first attempt reported divergedFields: [] and silently restored both fields, reproducing the exact bug this phase exists to close. That is worth stating plainly. This epic's thesis is 'policy declared in one table, enforcement hand-rolled per call site.' The final phase found one more call site than anyone had counted — which is the fourth consecutive time a copy count in this epic proved to be a lower bound. Also: divergedFields could only observe fields the executor actively RESTORED, by diffing postFm. A discard-to-empty is absent both before and after, so it was invisible. A second pass now reports it, which is what makes section 8.5's 'preservation is visible' true for the delete-the-body-line case rather than aspirational. cmdPhaseComplete now reports what it preserved — #3374 was filed against that command and its complaint was warnings: [], silence. cmdStateJson's private third copy of the guards is routed onto the executor's preserve-when-unchanged rule. A read is definitionally not a write, so the #1230 delta is 'unchanged' and curated wins over a stale annotation. shouldPreserveExistingProgress is a different rule and is untouched. Report reconciliation is ONE shared helper across seven commands, not five copies of fix(#3351)'s block. Five copies of a reconciliation is precisely the shape this epic removes, and introducing it in the final phase would have been a poor joke. Both untraced commands were traced rather than assumed: cmdStatePlannedPhase matched cmdStateBeginPhase exactly; cmdStateCompletePhase turned out to be a different legacy hand-rolled path reporting a mix of field names AND a section name, where the naive helper would have dropped 'Current Position' as a false negative every time. * test(#3471): characterization coverage for one enforcement point and reconciled reports Matrix sections A-E, asserted at the consumer's output per ADR-3180 Decision 4(b)/(c) — this phase owes Decision 5's outcome metric, the one the drift guard's zero may never be reported without. Three walls matter more than the new coverage: A2 is SIX separately named tests, one per gated guard, not one parameterised assertion over a list. A list is trivially shortened later; six named tests are not, and six guards is exactly where a field gets silently dropped. A6 pins what Phases 1-3 already fixed — non-empty stale body, delta unchanged, losing to fresher curated frontmatter, with the divergence reported. If A6 reddens, this phase broke the thing the epic was for. D1/D2 pin state sync byte-identical. The implementation had to GATE the six guards rather than delete them precisely because state sync has no executor, and a baseline probe showed unconditional deletion drops five fields. Nothing else in the suite would notice that regression. E6 covers #3345's direction — a field preservation restored that the intent never named IS reported. Nothing has ever tested that direction. Assertions were empirically verified against the compiled lib and the real CLI before being written, since the suite cannot be executed locally. That caught two type bugs in the draft: fm.current_phase after a quoted-YAML round-trip is the string '5', not the number 5. E5 is recorded as structurally unreachable rather than weakened or faked. Those four commands report body Title-Case labels, which cannot string-collide with a frontmatter snake_case key the way cmdStatePatch's arbitrary field names can — which is why fix(#3351) targeted only cmdStatePatch. Testing it directly would need reconcileReportedFields exported from private scope; the helper is exercised through E6 and all seven commands instead. * docs(#3471): amend ADR-3408 section 8.5 — a fourth enforcement point, and guards that could not be deleted Amendment 3. The contract held; two of section 8.5's own statements did not. It said the six empty-only guards are DELETED. They cannot be. writeStateMd is the sole path for both section 8.3 sanctioned-permanent exceptions and never runs applyStatePreservation, so those guards were their only empty-field fallback. A baseline probe on the unedited tree confirmed unconditional deletion drops five fields from a blank-body STATE.md on state sync, breaking the byte-identical guarantee section 8.3 grants it. They are gated instead. It also mis-located cmdStateJson's guards, describing them as living in syncStateFrontmatter. They were a separate private copy on the read path with no delta check at all, so a stale body annotation always beat fresher curated frontmatter in state.json — #3395's shape entirely outside the write seam. THE FINDING: a fourth enforcement point nobody had counted. The pre-existing #2202 unknown-key carry-forward loop independently restored the same six fields, silently neutralizing the fix. It is named nowhere in this ADR, in the phase design, or in three prior phases, and was found only because a probe that should have passed did not. Fourth consecutive time a copy count in this epic proved a lower bound: 2 write-seam bypasses became 4, three preservation encodings became four, and the estimate was wrong every time. ADR-3180's standing rule has earned itself in every phase — read the code, not the write-up. Records the Row 2 decision (a discard-to-empty wins per the delta rule and is reported, not silent — the sharpest Hyrum exposure in the epic), section 8.4's residue landing as ONE shared reconcileReportedFields across seven commands rather than five copies, and the parity assertion added because FRONTMATTER_KEY_TO_BODY_LABEL was itself a second table that failed silently — this epic's shape in miniature, in its final phase. * fix(#3471): repair four regressions the checkpoint caught Checkpoint returned 16 failures of 34389: six real regressions in pre-existing tests, plus seven of my own test bugs. My hypothesis was wrong and is recorded as such. I predicted the #2202 carry-forward skip was the cause, reasoning it had removed a load-bearing fallback the way the six guards nearly were. It was not implicated in any of the six. Three unrelated causes: #2111 — current_phase came back undefined from milestone complete, which is the epic's own defect class reintroduced by its final phase. Root cause is Row 2 working exactly as designed: milestoneCompleteCore rewrites the body Phase: line to a closure message, so current_phase's #1230 delta reads CHANGED and the new rule correctly discards the curated value. The transition never declared any intent to touch that field. Fixed by re-asserting current_phase and current_phase_name through authoritativeFm — the existing #2736 mechanism beginPhaseCore and completePhaseCore already use — rather than by weakening Row 2, which A5 pins. That interaction is worth naming: a rule that keys on 'did this write change the body source' will fire on a transition that moves the body line for an entirely unrelated reason. The design did not anticipate it. #1264 / #3242 / the state.patch progress report — reconcileReportedFields folded EVERY divergedFields entry into updated, including preserve-always progress restores no caller asked about. Now scoped to preserve-when-unchanged rows only. #1162 / case-insensitive table fields — valueOf checked frontmatter before body, so a lowercase table field name exact-matched the lowercase frontmatter key sync always derives, comparing stale pre-sync body text against a post-sync frontmatter enum. Flipped to body-first. That last one is the SAME lesson as Phase 2's patchCore, recurring in a different function two phases later: in this model the body is authoritative and frontmatter is the projection, so a name that could mean either resolves body-first. Twice now. Test bugs: a stray unused parameter shifted every argument at six call sites, so body arrived undefined; and A4 compared nested progress scalars against numbers when extractFrontmatter returns raw YAML strings. The string-vs-number YAML round-trip has now been caught three times in this phase alone. * test(#3471): one helper for the progress coercion that bit four times A2f failed on the string-vs-number YAML round-trip: extractFrontmatter returns nested progress scalars as raw YAML strings, so a comparison against numeric literals can never pass. This is the FOURTH time this exact class has been caught in this phase — twice during test authoring, once as A4 in the previous checkpoint, now as A2f. Patching it a fourth time by hand would guarantee a fifth. Added numericProgress() with a comment saying why it exists, and routed every progress-reading assertion in the #3471 block through it. Swept the block: C3 needed no change, because cmdStateJson's output already runs through normalizeProgressNumbers. Deliberately NOT shared with frontmatter.test.cjs's readPersistedProgress: that one is path-based and re-reads from disk, while these assert on an in-memory string that is never written. Sharing would have meant either a disk round-trip these tests do not do, or duplicating half the helper — so the coercion pattern is mirrored locally and the reason recorded, rather than manufacturing a dependency to satisfy the letter of consolidation. * chore(#3471): backfill pr number in changeset fragment --------- Co-authored-by: sim <sim@local>	2026-08-14 21:06:01 -04:00
Tom Boucher	e2f4c16d9e	refactor(#3469): one composition for the STATE.md write seam (#3501) ... * docs(#3469): amend ADR-3408 section 8.3 — the pipeline has sanctioned exceptions Section 8.3 read 'Every STATE.md write applies the pipeline.' That is false by design for two commands, and acting on it would have inverted a shipped feature. Preservation makes curated frontmatter win over a re-derived body value. state sync exists to do the opposite — #905's 'body annotation beats existing frontmatter when both are present'; it re-derives frontmatter FROM the body. REGENERATE_STATE is a factory reset that rebuilds STATE.md from scratch. Applying the pipeline to either would re-lock exactly what the command was invoked to replace. This issue's own scope line, inherited from the epic, said to route the direct writeStateMd callers through the pipeline. For cmdStateSync that would have shipped silently, with every gate green, because no test asserts that sync LETS the body win. Caught by reading the helper's docstring and then verifying the claim against the code — a stale comment had already misdirected this epic once. Both commands are now named in a closed exception list and are permanent ratchet entries. Consequence recorded rather than left to bite Phase 4: the 'drive the ratchet to 0 and delete the file' target in this ADR and in #3471 is wrong. Two entries are permanent, so the correct end state is 2, and the honest report is '0 removable bypasses, 2 sanctioned'. A guard reaching 0 here would only do so by having stopped looking at two real writers. * refactor(#3469): one composition for the write seam, not one per caller Implements ADR-3408 section 8.3 as amended. syncAndPreserveStateMd is now the single composition of syncStateFrontmatter and applyPostSyncPreservation. readModifyWriteStateMd and cmdPhaseComplete both CALL it instead of each assembling the two steps themselves. cmdPhaseComplete keeps its own writePlanningFileSet envelope — the composition returns content, it does not take over the write, so STATE.md still commits atomically with ROADMAP and REQUIREMENTS. Assembling the stages at a call site is a re-derivation even when every step calls an owner. Upstream's fix(#3374) routed cmdPhaseComplete through applyPostSyncPreservation but left it calling syncStateFrontmatter directly first, so the composition was duplicated and free to diverge with both guards green. That is ADR-3180 Amendment 2's finding repeating on the write side. cmdMilestoneComplete gains preservation. It wrote through writeStateMd, so it got sync and no preservation — the identical shape #3374 reported for phase.complete, and flagged upstream as a follow-up in the helper's own docstring. This is that follow-up. Divergence is now visible: preservation_warnings names each field restored over a disagreeing derived value. Deliberately NOT named warnings — cmdPhaseComplete already exposes warnings as a prose string array, and two sibling commands carrying that name with different element types is Generative Fix Divergence, the class this epic exists to remove. patchCore stops running stateReplaceField over the whole document. One observable consequence, intended per design row 9: a frontmatter-shaped patch key with no body counterpart now reports failed instead of silently succeeding, because the old whole-document match was literally hitting the YAML line case-insensitively. The guard closes Phase 1's DECLARED KNOWN GAP as promised rather than re-deferring it: section 8.3(b) detection is tractable now the composition exists. Scoped by two factors to avoid Phase 1's measured 29-to-1 false positive rate — a variable field-name argument AND a content argument whose nearest preceding assignment is not stripFrontmatter. Verified 0 findings and 0 false positives across all 33 call sites, plus 5 synthetic shapes. It also detects the re-assembly shape above. Ratchet: 4 entries to 2, both sanctioned-permanent. cmdStateSync's owner changes from #3471 to sanctioned-permanent per Amendment 2 — routing it through preservation would invert the #905 contract. Also fixed inline rather than deferred: cmdMilestoneComplete's STATE.md read now happens inside withStateLock. It previously read outside any lock before writeStateMd took its own, leaving a TOCTOU window under concurrent writers. * test(#3469): characterization coverage for the single write seam Matrix sections A-E. Criterion 6 was amended by maintainer decision — all five instances closed by point fixes while Phase 1 was in flight — so these are characterization tests at the consumer's output per ADR-3180 Decision 4(b)/(c), paired with the drift guard's count, never either alone. Section C is the one that earns its keep. cmdStateSync is a sanctioned permanent exception: state sync exists to re-derive frontmatter FROM the body, so preservation there re-locks exactly what the command was invoked to replace. C1 pins that the body wins; C4 pins that this phase left the command byte-identical. Nothing else in the suite would notice if a future change made sync start preserving, and the natural reading of 'one write seam' is to make precisely that change. Section E pins the guard's false-positive scoping. E4 (updateCore's strip-then-replace) and E5 (sectionBody-scoped calls) must NOT be reported — the naive detector measured 29 false positives to 1 true positive in Phase 1. E7 is the inverse: a sanctioned-permanent entry disappearing must FAIL, because a guard reaching zero here would only do so by having stopped looking at two real writers. Also corrects a stale test that asserted patchCore's old whole-document behavior, which this phase deliberately changes. One honest limitation, flagged rather than papered over: A1's 'byte-identical to pre-refactor' cannot be diffed against real pre-refactor bytes from inside the suite. It is implemented as the seeded fast-check property that cmdPhaseComplete's composed output equals readModifyWriteStateMd's for the same inputs — the strongest available proxy, not the literal claim. * docs(#3469): refresh the seam glossary entry and add the changeset Two spec-review gaps, both real. CONTEXT.md's STATE.md Transition Module entry named three direct writeStateMd callers including cmdMilestoneComplete. This phase routed that one through the composition, so the line was false the moment the refactor landed. Worth recording plainly: I wrote that sentence in Phase 0, correcting an older stale pointer in it, and my own Phase 2 change invalidated it again within the same epic. That is the exact drift this epic exists to remove, demonstrated on the epic's own documentation — and it is why the entry now ends by saying the whole-repo drift guard, not this line, is the authoritative count. The entry now records the composition (syncAndPreserveStateMd) and states that exactly two direct callers remain, both SANCTIONED PERMANENT rather than debt. Changeset: type Changed, because milestone complete's observable output moves. Tier-2 per ADR-3180 Decision 3 — a stale body line no longer wins over fresher frontmatter, and the command gains preservation_warnings. Docs requirement is met by the ADR amendment already in this diff. * test(#3469): register property-test temp-dir cleanup at creation time Standards review, minor but real: the new fast-check property cleaned up its temp dirs in a loop AFTER fc.assert returned. A genuine property failure throws, so that line never ran and every dir from the failing run — including all of fast-check's shrinking iterations — leaked. The failure path is exactly when a littered machine hurts most, and a failing property test is the case the test exists for. Cleanup is now registered with t.after() at dir-creation time, so teardown happens however the test exits. Not try/finally — CONTRIBUTING.md:356 bans it inside test bodies, which is why the after-the-assertion shape existed in the first place. Swept the rest of the branch's test diff for the same shape; phase.test.cjs already uses registered teardown and nothing else matched. * fix(#3469): patchCore routes frontmatter writes instead of dropping them Checkpoint returned 10 failures of 33880. One implementation defect, three test defects, one stale test — all fixed, and the implementation defect is the one that matters. patchCore stripped frontmatter and then reconstructed it VERBATIM, applying no patches to it. An arbitrary custom frontmatter key with no body counterpart and no FIELD_CLASSIFICATION row — risk_level in the upstream fix(#3351) test — therefore always reported failed and silently never wrote. It worked before, via the old whole-document match on the raw YAML line. That is a regression against this phase's own design row 9, which requires frontmatter changes to ROUTE THROUGH the seam — still work, policy-governed — not to stop working. Removing a capability is not routing it. An upstream test caught it, which is the argument for running the checkpoint before believing the refactor. patchCore now partitions by frontmatter shape, decided structurally from the parsed frontmatter's own keys rather than a naming heuristic: - classified keys still report failed — policy owns them and a raw patch may not bypass it; - unclassified keys apply to the frontmatter object and report updated — Phase 1's behavior-table row 19, a field with no row is not this contract's business; - body-shaped keys are unchanged. The property 'failure' was my own test breaking the repo's Clock Seams rule. The two paths agree byte-for-byte; the only difference was last_updated, stamped from the wall clock on two invocations milliseconds apart, so it could never pass. Time is now frozen with mock.timers across both — not by excluding last_updated from the comparison, which would have silently stopped comparing a field the composition writes. B4's fixture could not discriminate: normalizeStateStatus maps any text containing 'complete' to 'completed', and milestone complete's own new body value derives to exactly that — which was also the fixture's stale value. The stale value is now 'executing' so the assertion can tell 'body correctly won' from 'stale survived'. B5's fixture tripped a pre-existing unstarted-phase guard before reaching any write-seam code; it now has the matching phase directory. D9 asserted the old exempt set. readModifyWriteStateMd now calls one symbol rather than assembling two, so it needs no exemption; syncAndPreserveStateMd is the sole legitimate composition site. * fix(#3469): patchCore resolves body-first, so the body wins a name collision Re-verification returned 2 failures of 33880, both D4 — the hostile row for a key that exists as BOTH a frontmatter key and a body field. The partition checked frontmatter first, so 'status' — classified in FIELD_CLASSIFICATION and also present as a body 'Status:' line — routed to the frontmatter branch, was rejected as classified, and reported failed. Wrong order. Patching 'status' means the body field, and upstream fix(#3351) says so in its own comment: 'the legitimate working case for state.patch is display-cased BODY fields — Status, Current Plan, Phase.' The body is authoritative in this model; frontmatter is the projection. D4 asserted exactly that and was right. Resolution order is now body, then frontmatter: 1. resolves to a body field -> apply to body, updated 2. else an own key of the frontmatter: classified -> failed (policy owns it) unclassified -> apply to frontmatter, updated 3. else -> failed Verified by probe against the compiled lib for all four cases rather than asserted: risk_level (frontmatter-only, unclassified) still lands; current_phase still fails; display-cased Status unchanged; D4's lower-cased status now lands via the body with the frontmatter untouched. The current_phase case was the one that could have regressed silently, so its fixture was read rather than assumed — D1's body carries 'Phase: 3 (alpha)' and no 'Current Phase:' line, so body-first cannot reach it. * chore(#3469): backfill pr number in changeset fragment --------- Co-authored-by: sim <sim@local>	2026-08-14 16:04:09 -04:00
Tom Boucher	c90ae479f9	fix(#3350): prefer lowest outstanding phase over positional next in phase complete (#3482) ... * fix(#3350): prefer lowest outstanding phase over positional next in phase complete * chore(#3350): add changeset fragment * chore(#3350): backfill changeset pr field --------- Co-authored-by: sim <sim@local>	2026-08-14 11:30:39 -04:00
Tom Boucher	69e7afd0c7	chore(#3212): bounded quantifiers over document content — prohibition with teeth — Phase 4 (#3441) ... * feat(#3415): ship local/no-unbounded-quantifier, burn down ReDoS class Phase 4 of epic #3212 (ADR-3212 §5/§7, the final phase). New rule flags an unbounded */+/{n,} quantifier over a broad character class ([\s\S], dotAll ., or a 1-2-unit negated class like [^\n]/[^)\n] — the exact #2128-fixed shape) applied to a regex whose match target is data-flow-traced to readFileSync content. eslint-rules/lib/readfilesync-trace.cjs extracts the data-flow tracer shared with no-crlf-fragile-split (Phase 2) rather than a second copy — no-crlf-fragile-split refactored onto it with zero behavior change, parity-tested. Real triage, not 798 mechanical edits: the ADR's census (2026-08-08) screened every unbounded quantifier in the tree unscoped. Correctly scoped to readFileSync-derived content (matching Phase 2's own G2/G3 scoping), the rule found 162 real hits across two detection waves — the second wave (93) surfaced only after a genuine off-by-one bug in this rule's own first draft was caught while writing its RuleTester tests and fixed (the bug silently missed every directly-quantified [\s\S]* with no gap before the quantifier — exactly the class this rule exists to catch). 3 hits landed in production src/ (commands.cts, milestone.cts, roadmap.cts) and were each empirically timed against adversarial input (matching #2128's own measured-not-assumed precedent) — all confirmed linear-time/benign, left unbounded with a measured-evidence comment rather than mechanically bounded. The remaining 159 are test-file fixture parsing (test-author-controlled, fixed-size content, not adversarial input) — each suppressed with a specific, non-generic reason. Zero functional behavior changed anywhere in this diff. tests/no-pending-3212-markers.test.cjs locks the epic's own closing invariant (ADR §7: "assert zero pending #3212 markers remain") — ground truth confirmed trivially true today (no phase left any such marker behind), now regression-locked going forward. Design: .gsd/phase/chore-3415-prohibition-with-teeth/40-design.md Test matrix: .gsd/phase/chore-3415-prohibition-with-teeth/50-test-matrix.md Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): correct rule category mislabel, add CI test-scope entry An orthogonal Standards-axis review found eslint-rules/no-unbounded-quantifier.cjs mistakenly carried meta.docs.category: 'Portability', copied from a sibling rule without realizing what that implied: docs/contributing/cross-platform- portability-rules.md governs an ADR-1703 rule family under a hard "zero escape hatches" contract (tests/portability-rule-disable-ban.test.cjs's PROTECTED_RULES bans eslint-disable for those rules entirely). This rule is not part of that family — it's ADR-3212 (ReDoS/CWE-1333), a different epic — and its eslint-disable-next-line suppressions (159 of them, added earlier this same phase after empirical benign-verification) are an intentional, correct design, not a bypass. Corrected to category: 'Best Practices', matching the actual precedent (no-adhoc-regex-escape.cjs, Phase 1 of the same epic, which is also correctly outside PROTECTED_RULES), and the rule's own docstring now states this explicitly so a future reader doesn't have to re-derive it. Also registers a new scripts/ci-test-scope.cjs bucket so editing this rule or the shared eslint-rules/lib/readfilesync-trace.cjs helper re-runs their own test suites under targeted CI selection — was previously unregistered and invisible to that fast-path (this PR's own gsd-test checkpoint runs the full suite regardless, so this only affects future narrowly-scoped PRs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): bound no-unbounded-quantifier's own scanner (CWE-1333, ironic) Security review found the rule meant to catch algorithmic-complexity bugs had one of its own: hasUnboundedBroadQuantifier's negated-class inner scan walked from each `[^` occurrence to the next `]` (or EOF) with no bound, while the outer loop only ever advanced by one character — O(n²) total work on a pattern with many unclosed `[^` runs. Runs unconditionally inside checkPattern on any `new RegExp('literal string')` argument in any linted file, before the (cheap) readFileSync data-flow gate — so a single crafted string literal, no valid regex syntax required, could make `npm run lint` / CI hang. Empirically confirmed both the bug and the fix: pre-fix, n=4000/8000/ 16000/32000 chars took 30.8/115.6/463.8/1874.3ms (~4x work per 2x n, quadratic); extrapolated, the 300000-char repro from the finding would run ~165s. Post-fix (bail the inner scan once units exceeds the rule's own 1-2-unit scope, rather than continuing to hunt for a closing `]`), the same 300000-char input runs in 8.7ms via the real rule module, independently reconfirmed at 18ms via a fresh Linter.verify() call. New regression row in tests/no-unbounded-quantifier.rule.test.cjs asserts the RuleTester run on a 50000-char adversarial pattern completes and returns a defined result — no wall-clock assertion (CLAUDE.md Clock Seams / local/no-elapsed-assertion). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3415): triage 3 new sites, re-raise ceiling after upstream batch next merged 12 more PRs during this PR's review. Two consequences: - tests/edit-phase.test.cjs (fix #3262, unrelated) added 3 new content.match(/<tag>([\s\S]*?)<\/tag>/) reads of this repo's own workflow .md content — the same Class A pattern as the ~159 sites already triaged elsewhere in this PR. Suppressed with the same established reason. - lint-allow-test-rule-refs' ratchet ceiling needed re-raising again (301 -> 303) for the same reason as the two prior bumps: organic growth from unrelated, already-reviewed PRs landing concurrently, not a defect in this branch's own diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-08-14 10:02:28 -04:00
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>	2026-08-13 16:19:57 -04:00
Tom Boucher	b8cb031ce2	fix(#3163): scope phase.add insertion to the current milestone (#3400) ... * test(#3163): phase add must insert in the active milestone, not the trailing archive Regression for #3163: cmdPhaseAdd/cmdPhaseAddBatch pick the insertion point via rawContent.lastIndexOf('\n---'), the file's last horizontal rule — which on a roadmap with shipped/history material after the active phase list sits deep in archive. Rows 1/2/4 fail RED on next (entry lands after the archive heading); row 3 guards the no-milestone legacy fallback. * fix(#3163): scope phase.add insertion to the current milestone window cmdPhaseAdd and cmdPhaseAddBatch picked the insertion point via rawContent.lastIndexOf('\n---') — the file's last horizontal rule, which on a roadmap with shipped/history material after the active phase list sits deep in archive. Extract phaseEntryInsertOffset(rawContent, cwd): scope the search to currentMilestoneRawRanges' primary window so the entry lands at the end of the active phase list. Fall back to the legacy whole-file heuristic when no current milestone resolves, preserving simple no-milestone roadmaps. Applies to both cmdPhaseAdd and cmdPhaseAddBatch (identical expression); the decimal insert path was already header-anchored and is untouched. * docs(#3163): add changeset * docs(#3163): backfill changeset PR number (3400) --------- Co-authored-by: sim <sim@local>	2026-08-12 21:01:07 -04:00
sim	4036470ca0	fix(#3339): consolidated helper silently overwrote an unrelated module-scope function ... The runVerifiedPhaseComplete(args, tmpDir) consolidation in the prior commit hoisted a `function` declaration into a bare (sloppy-mode) block. Annex B legacy hoisting semantics mean a block-scoped function declaration in sloppy mode also reassigns any enclosing var of the same name the instant the block executes -- and this file already had an unrelated module-scope runVerifiedPhaseComplete(args, tmpDir, env) at line 54, used by ~50 other call sites throughout the file. The block ran before any test() body did, so every one of those 50 call sites was silently pointed at the consolidated helper's different phase-matching logic (parseInt-based, loses dotted decimal sub-phase segments) instead of the real one (normalizePhaseToken/phaseTokenFromDirName-based), breaking multi-level decimal phases like 03.2.1. Fixed by wrapping the consolidated helper in a strict-mode IIFE, which disables Annex B hoisting and restores real block scoping. Caught by a genuine gsd-test failure (2 unique failures, both throw- class, both in phase-completion verification-gate behavior) -- root- caused via diff against the pre-consolidation commit and a scoped local node --test run confirming the fix, not asserted from a hunch. No test() count changed. No production code touched.	2026-08-12 08:25:14 -04:00
sim	c444051bef	test(#3339): fix orthogonal-review findings — Wave 7 fold ... Standards/Spec-axis review + Memtrace graph pass found real issues in the just-folded suites, all fixed here: - tests/phase.test.cjs: the issue explicitly asked to dedupe overlapping fixtures between issue-2945/issue-2949 — the fold preserved both test sets correctly (genuinely distinct code paths, 0 tests dropped, already verified correct) but left each fold block with its own near-identical copy of a runVerifiedPhaseComplete(args, tmpDir) helper, the fixture- level dedup the issue actually asked for. Consolidated into one shared definition without disturbing the file's other, unrelated same-named helper at module scope (would have collided if hoisted directly). Removed two now-unused local runGsdTools destructurings left behind by the consolidation (both blocks already close over the module-scope import at line 26). - tests/review-lane-descriptor.test.cjs: two .find() results dereferenced without a presence guard (same defect class Wave 3/#3335 already found and fixed once in this epic) — added assert.ok() guards matching the repo's established style. - tests/model-resolver.test.cjs: documented the 1-of-80 dropped duplicate test with an inline comment, matching this same wave's host-integration fold's convention of citing drops by exact reference instead of leaving a reviewer to reconstruct the justification via git archaeology. No test() count changed in any file. No production code touched.	2026-08-12 08:25:14 -04:00
sim	1bc7f7e6b0	test(#3339): fold the state/phase/dispatch & model-profile issue-* cluster — Wave 7 ... Folds 9 legacy issue-*.test.cjs regression files (140 test() blocks) into their module's main suite, per H3 (#3315) of the test-hygiene epic (#3053). LAST of 4 issue-* waves — closes out the 74-file fix-*/issue-* backlog (pending BUG_FILE_RE extension, held for a follow-up commit until Wave 6 is confirmed merged, per the epic's own zero-backlog precondition). - issue-2828-flat-roadmap-total-phases.test.cjs (1) + issue-3204-state- writer-phase-count.test.cjs (21): both target state-document.cjs buildStateFrontmatter via different CLI entrypoints — merged jointly into state-document.test.cjs, 0 dropped. - issue-2945-phase-complete-checkbox-rollback.test.cjs (4) + issue-2949- phase-complete-stage3-sentinel.test.cjs (4): both target phase.cts cmdPhaseComplete; issue explicitly warned of overlap — verified disjoint fixtures/assertions, 0 dropped, merged into phase.test.cjs. - issue-2927-reviewer-lane-overlay-invocation.test.cjs (10) merged into review-lane-descriptor.test.cjs. - issue-2939-dispatch-flatten-maxdepth.test.cjs (9) merged into host-integration.test.cjs, 2 dropped as verified exact duplicates. - issue-2977-frontmatter-bom.test.cjs (5) merged into frontmatter.test.cjs. - issue-2045-third-party-skills-surface.test.cjs (6) merged into capability-loader.test.cjs. - issue-2517-runtime-aware-profiles.test.cjs (80, the largest single fold in the epic) merged into model-resolver.test.cjs, 1 dropped as a verified true duplicate (checked against src/model-resolver.cts logic, not just title similarity). Fixed a genuine eslint irregular-whitespace finding: a literal BOM character embedded in a doc comment (pre-existing content from the original #2977 source, illustrating what a BOM looks like) — replaced with a readable U+FEFF notation. 3 stale doc references found and fixed (docs/adr/2313, 3180, 443). Zero net test-coverage loss. No production code changed.	2026-08-12 08:25:14 -04:00
Tom Boucher	8b4545f3c0	feat(#3218): the prompt layer asks the CLI for plan counts (#3327) ... * feat(#3218): the prompt layer asks the CLI for plan counts Seven sites across four workflows counted plans with ls and wc -l instead of asking the CLI. A shell glob is not scanPhasePlans, so every fix that landed on the owner missed all seven: they counted superseded plans as live, reported zero for the nested plans layout, and missed loosely-named files. The 1762 figure of 30 plans and 24 summaries came from here. phase find is extended rather than a verb added - 3218 is an enhancement whose own checklist says it adds no new command, and CONTRIBUTING makes a new verb a feature needing approved-feature. It gains plan_count and summary_count for the live set and plan_count_all for the physical one, additively; the existing arrays are untouched. Both sets are exposed because the sites need different ones. Amendment 1 names two cases; three of these sites ask a third - did the planner write files to disk - and take the physical set, because a superseded plan is still a file the planner wrote. The progress.md dead route is fixed and was worse than the issue said. It read .plans and .summaries arrays that roadmap.analyze has never emitted, so the fallback always fired, both counts were always zero, and Route 0's resume-incomplete-phase check had never fired at all. The ratchet baseline is empty. Its own stale-entry check makes that self-enforcing. Verified on the remote runner. * test(#3218): acknowledge the workflow growth and update the stale guard The emitted-attribution gate named its own remedy, so it was followed rather than pre-guessed: four workflow files grew between 200 and 770 bytes because each replaced a shell glob with a find-phase call plus its jq extraction. plan-phase grew most - two sites, and it takes the physical count for its did-the-planner- write-files question. progress also carries the Route 0 dead-path fix. plan-phase-drift-guard asserted the literal old ls shape. Updated rather than deleted: what it protects is that a filesystem fallback exists and is reachable, and that is intact. It is not a regression - gsd_run is already load-bearing throughout plan-phase.md long before step 9, so the 9a and 11a fallback never existed to survive gsd_run being unavailable; it guards against the planner subagent's return hanging. Three ack sources collided with the new fragment, which the gate treats as a hard error rather than last-wins. Only the three colliding keys were removed, not the 421 spent entries, and two fragments left entryless were deleted per the convention that an empty fragment signals nothing. Verified on the remote runner. * docs(#3218): document the live and physical plan counts docs/CLI-TOOLS.md gains a find-phase counts section covering plan_count and summary_count for the live set against plan_count_all for the physical one, plus the null-not-zero not-found behavior. The live-versus-physical distinction is spelled out because a caller picking the wrong one gets a plausible number, which is the trap Amendment 1 records. Changeset leads with what a user sees: progress and execute-plan stop counting superseded plans as outstanding, a nested plans layout stops reporting zero, and Route 0 resume routing starts working after never having worked. No how-to. Nothing is enabled and nothing is sequenced - the user runs the same command and the number is simply correct. The one new distinction is field semantics, which is what a reference entry is for. * chore(#3218): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3327 exists. --------- Co-authored-by: sim <sim@local>	2026-08-10 12:56:38 -04:00
Tom Boucher	e201cde73c	refactor(#3186): one shared phase-completion predicate, disk-strict (#3306) ... * docs(#3186): record the disk-strict completion decision in ADR-3180 7.4 The maintainer decided #2957 on 2026-08-08: disk state is authoritative and a ROADMAP checkbox is a human annotation with no machine authority. Section 7.4 still carried the OPEN QUESTION and was marked blocked, so the contract said one thing and the tracker another. Recorded per section 7's own rule - a behavior not stated there is not decided, and amending a rule is an ADR amendment rather than a code change with a comment. The decision comment names Phase 4's PR as the carrier of this edit and makes it an acceptance criterion that the text be in the tree before implementation begins, so this lands first, alone, ahead of any code. Also clears the stale blocked-on-2957 row in the guard roster. * refactor(#3186): one shared phase-completion predicate, disk-strict isPhaseComplete in verification.cts becomes the single owner. It calls readVerificationStatus UNCONDITIONALLY - plan count is not a precondition - so a zero-plan phase with a passing VERIFICATION.md is complete. That is #3168: init gated the read on a plan count and synthesized a not_required sentinel, so phase.complete succeeded while init.manager reported incomplete for the same phase. The guard, built and run before scope was fixed per Amendment 3, found 9 re-derivations where the ADR named 3. Four were unnamed, including one in the prompt layer: mvp-phase.md ORed a ticked checkbox with disk status, which under disk-strict is the divergence itself. Per the #2957 decision, a ticked ROADMAP checkbox is a human annotation with no machine authority. The overrides in roadmap analyze and init manager are deleted rather than generalized; the user's checkbox stays in ROADMAP.md, only its authority goes. scanPhasePlans.completed and buildWorkstreamInventory are deliberately NOT folded - they answer 'are all plans summarized', which is a different question, and folding them would either over-report completion or invert the dependency direction between Phase 1's owner and this one. Verified on the remote runner. * fix(#3186): close seven review findings and record the missing-verdict rule The isolated review reproduced a write-path regression I introduced: migrating cmdRoadmapUpdatePlanProgress dropped its summaryCount>=planCount gate, so a phase with a fresh passing verification plus a newly-added unsummarized plan reported complete AND wrote a checkbox into ROADMAP.md while phase complete refused. The owner stays right per 7.4 - plan count is not a completion precondition - so the gate is restored at the write site as an explicit composition, mirroring the separate 2648 unexecuted-plan gate cmdPhaseComplete already carries. The spec axis was right that my 0.x-split reasoning was too permissive. The 2957 decision names buildStateFrontmatter as one of the three that must converge, and buildWorkstreamInventory combined a summaries-met local with verification data to decide the same verdict - Decision 4(c)'s named bypass, and it reproduced 3168 in a third surface. Both now route through the owner. The raw scanPhasePlans helper stays: it answers are-plans-summarized, which genuinely is a different question. Maintainer decision recorded in 7.4: a missing verdict is not a passing one, so an absent VERIFICATION.md means not complete everywhere. That retires 2645's verifier-disabled tolerance and inverts its Goodhart incentive - deleting the evidence now lowers completion instead of raising it. Guard hardened: block-form count gates and algebraic restatements are caught, and the header now discloses its remaining limits instead of overclaiming. Verified on the remote runner. * fix(#3186): route state sync through the owner and catch bare completed reads The matrix found 52 failures. 51 were fixtures asserting the old semantics: a phase with plans and summaries but no VERIFICATION.md used to count complete and correctly no longer does. Each fixture now carries a passing verification where that is what the test was actually about, rather than having its assertion weakened. The 52nd was a real 10th re-derivation the guard could not see. cmdStateSync destructured scanPhasePlans().completed directly - a bare field read, not a comparison - and used it as a completion verdict, so state sync and state json disagreed on completed_phases for identical disk state. Routed through the owner. Guard gains shape (d): any read of .completed off a scanPhasePlans() result outside plan-scan.cts, in chained, destructured and indirect forms, function scoped with no line window. It cannot tell a summaries-met read from a completion read - that is data flow - so it flags every one and requires a written-reason exemption, which is the same discipline shapes a-c already use. The blind spot is disclosed in the header rather than overclaimed. The emitted-attribution failure was also mine, not pre-existing: the mvp-phase.md checkbox-OR removal moves emitted bytes, acknowledged in tests/emitted-drift-acks. Verified on the remote runner. * test(#3186): give the nested-plans sync fixture a passing verification Last 3 matrix failures were one failure echoing up two describe levels. Phase 01-alpha had plans and summaries but no VERIFICATION.md, so under disk-strict completed stayed 0 and no Progress change was emitted - correct new behavior, not a regression. Added the passing verification rather than dropping the Progress expectation, so the test still covers what #3257 is about: that a nested plans/ layout is counted and not undercounted. Probe against the built lib confirms Progress: 0% -> 50% alongside Total Plans in Phase: 0 -> 3. * chore(#3186): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3306 exists. --------- Co-authored-by: sim <sim@local>	2026-08-10 10:18:29 -04:00
Tom Boucher	9faacc0c15	test(#3148): bound the long tail and delete the unbounded-spawn allowlist (#3192) ... * test(#3148): bound the long tail and delete the allowlist Migrates the final 170 unbounded sync spawn sites across 49 files, then removes the allowlist entirely. local/no-unbounded-spawn now runs with no exemption surface across tests/**: there is no file to add a name to. drift-detection's throw-native git() helper routes to gitOrThrow -- bare runGit would have taken 16 call sites quiet on failure. commands.test.cjs has two independently-scoped runGsdTools/runCli helpers, one already bounded and one not; they are kept distinct rather than unified, the same trap as the two same-named git() helpers in Wave 1. runNpm's bound was erasable. Its options spread callerOptions after the defaults, so an explicit timeout:undefined silently dropped the 180000ms bound -- the rule flagged it and was right; it was not a false positive. Fixed by destructuring with a default, with a test that fails when the default is removed. Two sites stay on a raw spawn with an explicit timeout because the seam cannot express them: one needs shell:true for npm.cmd on Windows, one redirects stdout to a real fd. Both are the rule's own documented second option, not an escape from it. Closure verified rather than asserted: the derivation scan reports 0 unbounded spawn helpers and 0 unbounded direct git call sites, and a temporary file carrying an unbounded spawn still errors with the allowlist gone. Closes #3064. * test(#3148): close a hole in the guard's own eslint-disable ban The ban listed only the top level of tests/, so it was blind to 37 .cjs files under tests/helpers, qa, observability, fixtures and dispatch. With the allowlist deleted this test is the sole remaining way to detect someone silencing the rule inline, so the gap was load-bearing: a nested file could carry an unbounded spawn plus an eslint-disable and pass everything. Proven before and after. A probe planted under tests/helpers with both was invisible to the guard and clean under eslint; after making the listing recursive the guard fails on it. The scanned set goes from 771 files to 808. Pre-existing since the guard shipped, but this wave is what promoted it to sole defense, so it is fixed here rather than filed. Also converts the last hand-rolled throw check to throwIfFailed and the last re-derived legacy shape to compose toLegacyResult, which makes the epic's none-remain claim true rather than nearly true. toLegacyResult itself is not widened -- eight callers depend on its shape and one consumer does not justify changing a shared contract. * fix(#3148): correct seam incoherence at the bound and a slow review-lane error path Two real failures from the remote runner, both fixed at the cause. The seam could return outcome TIMED_OUT together with exitCode 0. At the exact bound spawnSync reports ETIMEDOUT while the child has already exited with a real status, and toSeamResult classified on the error code while passing status straight through -- an incoherent pair its own boundary test was written to catch, and did. A status that is not null is direct evidence the child exited on its own, so it now decides the outcome before the error-code branches run. process-seam.cjs was deliberately untouched by every earlier wave; this is a defect in the module itself, kept surgical, with a unit test that fails against the old logic. review-lane with an unknown subcommand fell through to its usage error only after loading the capability registry and building a per-lane plan, which spawns one child process per lane -- up to twelve. The error path took ~1288ms instead of ~119ms, and under bench load it outran a caller's spawn timeout and was killed before writing anything, which is the empty stdout and stderr CI saw. It now fails fast before any of that work begins. This is the epic's first production change. It is user-facing, so it carries a changeset rather than a no-changelog label. * test(#3148): replace a real-race timeout test with a deterministic one E9 raced git rev-parse against a 1ms bound and assumed git always lost. On a warm container git finishes first, spawnSync returns status 0 with no error at all, the seam correctly classifies EXITED, and gitOrThrow correctly does not throw -- so the test failed on both lanes. A probe confirms a genuine timeout always carries status null, so this was never the seam misbehaving. Raising the bound would only lengthen the odds, which is the same defect with better luck. The test now drives gitOrThrow against a stubbed runGit that returns a synthetic TIMED_OUT result, so it asserts exactly what it always meant to -- that a timeout propagates as a throw -- with no timing dependence. Five consecutive runs are identical where the old one varied. I wrote this test in Wave 0; it is a real-race test by construction and CLAUDE.md says to replace those rather than re-run them. * chore(#3148): backfill changeset PR number 3192 --------- Co-authored-by: sim <sim@local>	2026-08-07 21:03:50 -04:00
sim	7dd9e59f6b	test(#3090): stop exempting violations under categories that do not fit ... An allow-test-rule annotation citing a category that does not apply is worse than no annotation, because it reads as reviewed. Eight were confirmed by reading the assertions each one covered, and auditing the rest found five more plus one refutation — a converter test whose wording described the wrong mechanism while the covered assertion genuinely was deployed-text. The instructive one used the CANONICAL string for the same mistake: STATE.md command output labelled as a deployed artifact. A canonical string is not evidence the category fits, which is why normalising strings alone would have laundered the problem rather than fixed it. Every mapping the audit had inferred rather than code-verified was spot-checked before rewriting, and the ones that turned out not to fit were re-annotated rather than relabelled. Fourteen STATE.md assertions had a typed extractor available all along and now use it; their annotations came out because nothing needs exempting. Eight assertions genuinely need a production change first — CLI stdout and stderr with no structured mode — and are tagged pending-migration-to-typed-ir citing #3090, which is what that category is for. It had zero real uses before this, while one file carried a real citation to migration issue #2974 under a non-canonical tag. Six annotations covered assertions that do no text matching at all. An exemption for a violation that does not exist is noise that makes the real ones harder to audit; those are removed. atomic-write-coverage gains the annotation it always warranted — its own docstring describes a structural-regression-guard while the file carried none. Fifty-nine non-canonical strings across roughly thirty files are normalised, and the allow-test-rule allowlist is regenerated to match. 472 annotations became 463: every one now uses a canonical category, and the two remaining non-canonical strings are ESLint RuleTester fixtures, not annotations. Refs #3057 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>	2026-08-05 17:20:56 -04:00
Tom Boucher	53ea8e0664	fix(#3057): make a guard's failure distinguishable from its benign result — Wave 1 (#3088) ... * fix(#3057): refuse the write when the duplicate scan cannot complete writeManifest documents itself as a fail-closed duplicate guard: if any existing manifest shares plan_id with a different, non-terminal job_id it must refuse, because dispatching again would duplicate the external job. It could not honour that. The scan reads every sibling manifest looking for the duplicate, and an unreadable or unparseable sibling was `continue`d past. If the corrupt file was the one holding the live duplicate, the scan found nothing and a duplicate external job dispatched. The asymmetry is what gives it away: a malformed TARGET refused with malformed_existing because clobbering is unacceptable, while a malformed SIBLING was skipped — yet siblings are the only thing the duplicate check reads. Adds a scan_incomplete verdict that refuses and names the offending file, so an operator can quarantine or repair it. Fail-closed alone would let one stale corrupt manifest wedge every dispatch for that planning dir permanently; naming the file is what makes refusing survivable. malformed_existing is untouched, so the target/sibling distinction stays visible. The docstring is updated — it previously stated a rule the function did not keep. memFs() gains an optional failReads map so these branches are reachable at all; they had zero coverage because the fake could not express a per-file read fault. The signature is additive and every existing caller is unchanged. The regression is proved by a pair, not a single test. A control writes a readable sibling holding a genuine non-terminal duplicate and asserts duplicate_plan_id, establishing the scenario is real; the regression then makes that same path unreadable and asserts scan_incomplete. A first draft of this test used a corrupt-JSON fixture containing no plan_id at all while its comment claimed otherwise — it duplicated the unparseable-sibling case and proved nothing, which is the defect class this phase exists to remove. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): make a guard's failure distinguishable from its benign result Wave 1 of the negative-space backfill: the branches where a guard that could not verify something reported the same value it reports when everything is fine. That indistinguishability is the defect; every fix here makes the two states tellable apart, and every test proves it with a pair — one for the failure, one for the benign case. A single test cannot establish that two states are distinguishable, which is the whole property being fixed. state.cts phaseInventoryProvider returned null for both a real disk-scan failure and a genuinely empty phases dir, so `state rebuild` could report success while phase-table reconciliation never ran. It now returns a discriminated result and the CLI surfaces phase_inventory_scan_failed plus a reason. The reason field turned out never to have been wired into the emitted JSON at all — it existed only as an internal variable — so a test could only assert on the operator-facing note. It is a real field now. state.cts treated an unreadable lock body the same as an empty one, applying the 1-second stealable floor. A lock we cannot read is not a lock we know is stale; an unreadable body is now held to the deadman ceiling like a live holder. verification.cts findStaleVerificationSummary returned null on any fs, scan or clock failure — meaning "not stale". It now returns a discriminated StaleCheckResult and the caller records that the check was indeterminate. git-base-branch resolveBaseBranch returned 'main' both when no candidate branch existed and when every git tier timed out. A diagnostics variant now reports whether the answer was verified, and the CLI writes an unverified-fallback note to stderr. The stdout contract five workflows parse is untouched. worktree-safety snapshotWorktreeInventory left exists:true when statSync threw, so a guard that could not check reported the worktree present; exists is now tri-state and a stat failure surfaces as an 'unverified' finding. planWorktreePrune reported 'no_worktrees' for a parse failure, which is not the same as an empty list — and it drives a prune. It now reports 'parse_failed'. Fixing the inventory change exposed a second fail-open in verify.cts: the validate-health consumer silently dropped findings whose kind it did not recognise, so the new kind would have vanished. That is closed too — worth noting that the survey enumerated producers of degraded verdicts, not consumers that discard them. worktree-base-ref and state-transition gain the distinguishing signal without changing what they do: headAbsenceVerified, and a phase-inventory scan meta. Whether those guards should ACT differently is a product question this change does not answer, and both are flagged rather than quietly settled. rescueSummaryArtifacts is left alone: rescuing on an uncertain cat-file is deliberate per #2556. It now has tests proving it, and a recorded negative finding — git cat-file -e returns 128 for both "absent from HEAD" and a fatal error, so "uncertain" and "certain-and-fine" are not separable at the git level. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): assert typed values, not rendered text Ten assertions in the rebuild CLI suite matched substrings of produced output — STATE.md body fields, a markdown table row, an audit-log heading, and JSON keys read as text. CONTRIBUTING prohibits that: if the code under test produces text, the test asserts on its structured surface instead. No production surface had to be built. Every one already existed and was already compiled into bin/lib: stateExtractField for body fields, parseMarkdownTable for the phase table, collectSection for the audit-log section, and result.data.log — already a typed RebuildLogEntry[]. The tests were matching rendered text sitting next to the structured data. One of those assertions was passing for the wrong reason. `stdout.includes ('rebuilt')` matched the JSON KEY name, not a value: the dry-run path emits `mutated` and the real path emits `rebuilt`, so it would have passed whether the value was true or false. It now asserts the value. external-job's refusal already had to name the offending file — that naming is why the fail-closed variant is survivable rather than a permanent wedge — but the tests proved it by substring of a prose message. The failure result now carries offendingPath as its own field and the tests assert it by value. The human message is unchanged; operators read it. Array membership is left alone. `phaseIds.includes('99')` and `result.updated.includes('Completed Phases')` are membership checks on real arrays, not text matching, and converting them would weaken nothing and clarify nothing. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): execute acquireStateLock instead of grepping its source The non-EEXIST lock test asserted on the TEXT of the built .cjs and never called acquireStateLock. It carried an allow-test-rule: architectural-invariant exemption to permit that. A source grep proves a literal is present in a file, not that the behaviour works — it is weaker than a liveness test, which at least runs the code, and it was the only coverage the fatal-errno path had. Replaced with tests that inject the errno through fs and assert what actually happens: a fatal EACCES propagates out of acquireStateLock with zero backoff sleeps, while EAGAIN/EINTR/EINVAL/EIO/ENOENT/ESTALE/EPERM/EBUSY retry once and succeed. The exemption is removed and its allowlist entry with it. One old assertion is deliberately not carried over: it checked the retryable errnos were expressed as a Set rather than an inline literal. That is a shape check with no runtime signature; the behavioural tests fail if the code reverts to the old inline check, which is the regression it was really guarding. The #3057 lock-body tests move into that same file rather than a new one, which is what lint-test-file-count asks for and puts every acquireStateLock test in one place. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): surface an indeterminate staleness check to its callers An isolated review caught an inconsistency inside this wave. Two of the three "add the distinguishing signal" fixes wire through to something a user sees: git base-branch writes an unverified-fallback diagnostic to stderr, and an unverifiable worktree surfaces as a W020 finding. The third set staleCheckIndeterminate on readVerificationStatus's result and nothing read it. A signal nobody consumes leaves the fail-open exactly as silent as before: the staleness check could fail and the operator saw precisely what they would see if the answer were genuinely "not stale". That is the defect this issue exists to remove, so it is not defensible as scaffolding when its two siblings in the same change already wire through. All five callers now surface it, each through the channel it already had rather than a mechanism imposed uniformly: phase complete adds it to its existing warnings array and, on the blocked path, as an additive note on the error text; init and roadmap carry it as a field on output they already emit; the UAT report carries it without ever gating passed/blockers; workstream inventory takes an injectable writeDiagnostic mirroring the git base-branch idiom, because its return shape had nowhere to hang a per-phase field without rippling the builder's types. The routing decision is unchanged everywhere. What changes is only that a caller and an operator can now tell a failed check from a completed one. That diagnostic carries structured meta rather than being asserted by regex — the default still writes only the human message to stderr, but tests assert phaseDir and reason by value. Two earlier assertions in this branch were converted the same way; this was the last raw-text assertion left. Also records a scope correction: the completePhaseCore guards now compare stateReplaceField's result to the body instead of testing truthiness, so a field whose substitution produced identical text no longer reports as updated. That is a real behaviour fix, not the signal-only change this file was described as carrying, and its tests cover both the changed and unchanged cases. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): bound two heavy subprocesses for a loaded bench, not an idle one The remote matrix surfaced three failures unrelated to this branch's changes. All were bad tests, and a re-run would have hidden every one of them. The reviewer-flags parse block bounded bash -> node -> a full gsd-tools cold start at 5 seconds. On a bench running thirty thousand tests in parallel that is not a hang, it is a busy machine. Raised to 30s, matching the convention sibling suites already use for script invocations, with a comment saying what the budget covers so nobody tightens it back. Two further copies of the same 5-second spawn in the same file had the identical defect and are raised too — they were not in the failure report, but they will be next time. The fragment-propagation test bounded npm run regen:derived — a full build plus eight generators, the heaviest subprocess in the suite — at five minutes, and node22 was killed near the end. The captured output proves it: every generator had written its files and gen:install-tree had emitted all fifteen runtimes before the kill. Raised to fifteen minutes. That failure read as `null !== 0`, which says nothing. status null means killed, not a non-zero exit, and the two want different responses: one is a timeout to size correctly, the other is a real build break. The assertion now distinguishes them and names the signal. Neither test's assertions were weakened and no retry was added. A retry here would suppress exactly the signal the timeout exists to produce. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture fd 1 through the mock tracker, not a raw reassignment The phase suite reported zero test results on both lanes while running for five and a half minutes and exiting 1. No assertion text, no stderr, four events for the whole file: enqueue, start, dequeue, complete. That shape is not a failing assertion — it is the runner being unable to read the child at all, because it parses its event stream from the child's stdout. The cause was the capture helper reassigning fs.writeSync directly. Proven rather than assumed: a standalone probe patched fs.writeSync and called process.stdout.write, and the interception fired only when fd 1 resolved to a FILE, not when it was a pipe. The remote runner captures the event stream to a file, so a helper that was invisible against a pipe swallowed the reporter's own output on the bench. That is also why the two sibling suites wired the same way in this change pass cleanly — they use the mock tracker, the seam io.test.cjs established for this exact function. The helper now uses t.mock.method with an explicit restore after each call, so teardown belongs to node:test rather than a second hand-rolled implementation, and the interception cannot outlive the one synchronous call it wraps even if that call throws. Ten call sites thread the test context through; three test callbacks gained the parameter they lacked. The three B3 tests are untouched — same assertions, same fault injection. Only how the context reaches the helper changed. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3057): capture phase-complete output from a subprocess, not fd 1 Two attempts to make in-process fd-1 interception safe both failed on the bench. The suite reported zero test results on either lane while exiting 1 — four events for the whole file — because the runner parses its event stream from the child's stdout, and process.stdout.write routes through fs.writeSync whenever fd 1 resolves to a file, which is how the runner captures. Patching that seam anywhere in a file can therefore destroy the file's own reporting, and tightening the window only moved the runtime from 326s to 125s without recovering a single event. So the interception is gone rather than tuned. The helper now spawns gsd-tools as a real subprocess and reads stdout the way the OS already gives it to us, which is what the rest of the suite does. It asserts the command succeeded before parsing, so a genuine failure can no longer present as a JSON parse error. The two fault-injecting tests could not survive that move as written: a subprocess cannot see a mock installed in the parent. Instead of reinstating the interception they now produce the fault on disk — the summary artifact is created as a dangling symlink, so the staleness check's real statSync throws inside the child. That is a more honest fixture than a mock in any case, since it is a condition a user's tree can actually be in. Skipped on Windows, matching the existing symlink precedent in the write-guard suite. Three further call sites turned out to depend on parent-process writeFileSync mocks the subprocess could not see. Those call the CJS function directly, which is what they always wanted — they never needed stdout at all. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3057): one name for one signal, one encoding for one distinction Standards review found four things this branch introduced, all of them inconsistencies with itself rather than with the repo. One upstream bit reached its consumers under three names — verification_stale_check_indeterminate in two modules, the same value with "stale" dropped in a third, and stderr only in the fourth. Standardised on the long name wherever it is a field. The workstream inventory keeps its stderr channel, since its return shape has nowhere to hang a per-phase field without rippling the builder's types, but it now says the same word for the same thing. worktree-safety encoded one three-way distinction two ways in a single file: a named union for a finding's kind, and boolean|null for an inventory entry's existence. The second is now a named union too. Two assertions matched human prose because the blocked and non-blocked completion paths carried no typed field for the signal. Both now assert typed values. The first round of this fix added the field but left the regex beside it, which is the banned pattern sitting next to its own replacement; the second removed it and added an assertion on the reason enum so nothing was lost. The remaining two were reasoned away before being fixed, and both reasons were bad. "No typed surface exists" is the condition CONTRIBUTING says to fix by adding one — it took three lines. "The file already does this dozens of times" is not licence to add instance number thirty-one; a convention that violates a documented rule is debt, not precedent. Vocabulary differing across DIFFERENT modules is left alone: CONTEXT.md rejects a single shared result envelope, so per-module shapes are precedented, and a baseline smell does not outrank a documented standard. A census of every line this branch adds to a test file now finds no regex or substring assertion on produced prose: 87 strictEqual, 25 ok (all non-empty or shape guards), 12 equal, 3 throws (all typed err.code predicates), 3 deepStrictEqual, 2 notStrictEqual. Refs #3051 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3057): backfill changeset pr number to 3088 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-08-05 16:00:52 -04:00
Tom Boucher	f0bb0787c9	fix(#2640): report truthful state_updated + keep progress frontmatter in sync after phase remove (#2974) ... * test(#2640): add regression for state_updated false positive + stale progress Three cases: (1) state_updated reflects actual content change, not just file existence; (2) progress.total_phases resync'd even when the body lacks 'Total Phases:' (the no-op guard was skipping syncStateFrontmatter); (3) state_updated is false when STATE.md doesn't exist. * fix(#2640): report truthful state_updated + force frontmatter resync Two defects in cmdPhaseRemove: 1. state_updated was fs.existsSync(statePath) — trivially true, since the file existed before and readModifyWriteStateMd never deletes it. Now captures the boolean return from readModifyWriteStateMd (changed from void to boolean: true when content was written, false on no-op). 2. progress.* frontmatter stayed stale when the body lacked 'Total Phases:' or 'of N' — readModifyWriteStateMd's no-op guard (#948) skipped syncStateFrontmatter when the body transform was unchanged. Now the transform forces a body diff when a phase was actually removed, so the guard passes and syncStateFrontmatter rebuilds progress.* from the post-deletion disk/ROADMAP state. * fix(#2640): address review — gate forced-diff on targetDir, strengthen assertions Two MAJOR findings from isolated adversarial review: 1. Forced-diff injected a spurious 'Total Phases:' line even when no directory was removed (targetDir === null). Now gated on targetDir !== null. 2. Test #2 asserted 'not 3' instead of '2' — would pass for any wrong count. Now asserts exact value. Test #1 strengthened to assert body Total Phases and frontmatter total_phases both equal 1. * chore(#2640): add changeset fragment * chore(#2640): backfill changeset PR number 2974 --------- Co-authored-by: sim <sim@local>	2026-08-01 11:56:34 -04:00
Tom Boucher	388837219d	fix(#2648): phase.complete refuses when non-retired plans lack summaries (fail-closed coverage gate) (#2953) ... * test(#2648): failing-first — phase complete must refuse when plans lack summaries Adds findUnsummarizedPlans to core-utils (mirrors countMatchedSummaries but returns the unmatched plan files) and a PHASE_PLAN_COVERAGE_INCOMPLETE error reason, plus a 3-case regression block in phase.test.cjs. The gate itself is NOT yet wired into cmdPhaseComplete (reverted for the RED run), so the 'blocks completion' and 'superseded does not block' cases must FAIL (the pre-fix code completes silently). * fix(#2648): phase.complete refuses when non-retired plans lack summaries cmdPhaseComplete gated only on a single *-VERIFICATION.md status, so a phase could close complete while an arbitrary number of its plans had no completion record (confirmed incident: 6/30 plans unexecuted incl. the phase's entire final UI scope, every signal green). Add a fail-closed plan-coverage gate that refuses completion when any plan lacks a matching *-SUMMARY.md, naming the missing plans, UNLESS the plan is retired via machine-readable status: superseded frontmatter (#2349) — closing the Goodhart hole (delete a SUMMARY to raise the %) without regressing the lock/recovery pattern. Uses scanPhasePlans (superseded-AWARE) + new findUnsummarizedPlans helper so the gate, the count, and the named list can never disagree. Evaluated before the verification-gate transaction so a refusal fails fast without mutating ROADMAP/STATE. milestone.complete's parallel gap is explicitly out of scope (separate seam, separate PR). * fix(#2648): test fixtures — give #1752 phase plans summaries + STATE.md in coverage fixture The plan-coverage gate (#2648) correctly blocks phase completion when a plan lacks a SUMMARY. Two test fixtures needed updating to reflect the new contract: - #1752 (total_phases-decrement cascade): its 8 phase dirs each had a PLAN.md with no SUMMARY. The test's concern is the total_phases cascade, not plan coverage, so add a matching SUMMARY to each to keep the phase fully-covered and isolate the #1752 behavior. - the #2648 coverage-gate fixture: write STATE.md (createTempProject scaffolds .planning/phases but not STATE.md) so the 'ROADMAP/STATE unchanged on refusal' assertions have a file to read. * fix(#2648): security — fail closed on unreadable plan dir + sanitize msg + surface superseded Address the security-review blocker (B1) and hardening (M1/m2): - B1 (blocker): the gate failed OPEN when scanPhasePlans could not read the phase dir (it swallows readdirSync errors → empty plan set → gate sees zero unsummarized plans → passes). A coverage gate that passes when it cannot read the plans re-opens the #2648 hole under any I/O failure. Now readdirSync the dir explicitly and fail closed (PHASE_PLAN_COVERAGE_INCOMPLETE) on a throw; a readable empty dir still passes (legitimately complete empty phase). - m2: sanitize plan filenames (strip C0 controls / DEL) before interpolating into the error message — they come raw from readdirSync and could spoof the terminal in plain-error mode. - M1: surface the count of plans excluded as status: superseded so a reviewer can audit which work was declared retired (the marker is a committable, review-time-trusted bypass; keep it visible). - Add a 4th regression case: unreadable plan dir (ENOTDIR via a file, not chmod 0o000 which root bypasses) must fail closed. * test(#2648): drop unreachable B1 case — no root-safe unreadable-dir repro The B1 fail-closed-on-unreadable-dir defense stays in src/phase.cts (cheap + correct), but it cannot be unit-tested cross-platform: any condition that makes the phase dir unreadable to the gate's readdirSync ALSO fails findPhaseInternal upstream ('Phase N not found') before the gate runs, and chmod 0o000 is forbidden (root bypasses it in root CI). Document the gap in the test file; remove the case that asserted a reason the upstream error pre-empts. * style(#2648): drop unnecessary type assertions flagged by lint:ci scanPhasePlans returns typed string[] arrays, so the `as string[]` casts on coverageScan.planFiles/summaryFiles were redundant (@typescript-eslint/ no-unnecessary-type-assertion). Compute supersededCount from typed lengths; only the phaseInfo['plans'] cast remains (it is genuinely unknown). * changeset(#2648): phase.complete refuses when plans lack summaries * changeset(#2648): backfill PR number 2953 --------- Co-authored-by: sim <sim@users.noreply.github.com>	2026-07-31 18:11:40 -04:00
Tom Boucher	d49a7d0c4d	fix(#2853): roadmap.update-plan-progress preserves hand-written annotations (#2916) ... * test(#2853): add failing-first regression for plan-progress annotation preservation The count-bump regex's trailing [^\n]+ swallowed the whole Plans line and the replacement wrote back only the regenerated count, deleting any hand-written annotation after it. 8-row matrix covers bold/plain forms, bare template form, executed path, CRLF, and idempotency. * fix(#2853): preserve hand-written annotations in roadmap plan-progress bump The count-bump regex's trailing [^\n]+ swallowed the entire Plans line and the replacement wrote back only the regenerated count, deleting any hand-written prose after the count (e.g. a gap-closure annotation). The verb owns the count token only. Capture the existing count token ($2) and the trailing line text ($3), and rebuild the line as <label><new count><surviving text>. Trailing text is preserved ONLY when a real count token preceded it, so the fresh-template bracketed placeholder (`[Number of plans…]`) is still replaced cleanly rather than glued after the count (pre-#2853 behaviour on the template path preserved). CRLF \r is preserved via [^\r\n]. Widens replaceInCurrentMilestone to accept a replacement callback (needed to branch on whether the count group matched). The bare Plans: checklist header is still skipped — the lazy match lands on the summary line first and a count-less bare header yields no count to anchor preservation to. * changeset(#2853): backfill PR number 2916 --------- Co-authored-by: Test <test@example.com> Co-authored-by: sim <sim@local>	2026-07-31 09:54:55 -04:00
Rezolv	7e8f6a6d7d	enhance(#2572): run the verify-summary artifact check against phase SUMMARYs (#2685) ... * enhance(#2572): run the verify-summary artifact check against phase SUMMARYs (W025) The artifact<->git check has existed since the beginning but was only ever pointed at .planning/research/SUMMARY.md (new-project.md:1145, new-milestone.md:425). Phase summaries -- the ones that actually claim "I created these files" -- were never checked. - extract verifySummaryCore from cmdVerifySummary: same checks, lifted out of the output() wrapper so callers consume {passed, checks, errors} directly instead of shelling out and re-parsing JSON; cmdVerifySummary is now a thin adapter over it - validate.health gains advisory W025 per phase SUMMARY with missing files Advisory only: appends to warnings[], never touches status escalation beyond the channel's own warning semantics, the repair set, or readVerificationStatus. Resolves both open questions from triage: (a) commits_exist is deliberately NOT surfaced -- its hash pattern matches any hex-shaped token in prose, too loose to show a user; (b) a phase carries N per-plan summaries plus a legacy bare SUMMARY.md, so all of them are checked via the repo-wide filter. * chore(#2572): add changeset fragment * feat(#2572): move the SUMMARY artifact check to phase completion Responds to the #2685 review. Three substantive changes. Seam (Blocker 2). The check now runs in cmdPhaseComplete, the seam the issue body cited (src/phase.cts:~1745), not validate.health. That channel does exist: cmdPhaseComplete declares warnings[], populates it from the UAT/VERIFICATION pre-scan, and emits it. The cycle objection raised against the earlier deviation holds for state.cts only -- verify.cts has no transitive import path to phase.cts, so phase.cts -> verify.cjs adds no cycle (verified over every src/*.cts). Moving it also retires the retroactive firing across all historical phases: this fires once, at completion, for the phase being completed. Extraction (Blocker 1). Pattern 2 now excludes [ and ] from its path class. The SUMMARY templates prescribe a YAML flow sequence (key-files.created: [a.ts, b.ts]) and the label matches case-insensitively, so the class previously captured the literal [ and produced a candidate that can never exist on disk -- firing on healthy projects built from the templates GSD itself ships. Stripping frontmatter was the other offered remedy; measured across all three shipped templates it is a no-op on top of the exclusion, so it is not carried. Consequence named in-code: the key-files block still is not read, which needs a real frontmatter parse. Also narrowed to the noise classes confirmed in review -- globs, bare hostnames, and paths resolving outside the project are skipped rather than reported, and the containment guard the old comment claimed now actually exists. Budget (Majors 1 and 3). verifySummaryCore takes a checkCommits option; phase completion passes false, so the discarded git cat-file probes are not spawned at all. It also passes Infinity, so every referenced file is reported instead of the first two -- a summary listing twelve files of which nine are missing now says nine, not zero. The verb keeps its historical 2-file default. Tests (Major 2). The vacuous fixtures are gone with the health block. The replacements use /-bearing paths that genuinely extract, and each fix was mutation-checked: un-anchoring pattern 2, dropping the glob, hostname or containment filter, forcing commit checking on, and re-capping at 2 each fail at least one test. * docs(#2572): describe the phase-completion SUMMARY artifact check The W025 text under /gsd-health is withdrawn with the health seam; the check is documented where it now runs, under `phase complete` in docs/CLI-TOOLS.md. Both the docs and the changeset previously overclaimed: they said a referenced file not on disk is warned about, while at most two candidates per SUMMARY were ever examined. The cap is gone at this seam, so the claim now holds -- and the text states the limits that remain, rather than leaving them to be discovered: the key-files frontmatter block is not read, commit hashes are not resolved, and globs, URLs, bare hostnames and out-of-project paths are skipped rather than reported. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux>	2026-07-28 18:31:33 -04:00
Tom Boucher	27c2279a39	fix(#2617): project verification next_command onto the runtime's command surface (#2700) ... * fix(#2617): project verification next_command onto the runtime's command surface `src/verification.cts` stored and synthesized hard-coded `/gsd:…` command strings with no runtime context, and `phase complete` relayed that raw field straight into its verification-blocked error. On a Codex project the suggested next step was `/gsd:execute-phase`, a surface Codex does not install — it installs `$gsd-execute-phase`. The colon form is wrong twice over: `runtime-slash.cts` documents that "the colon form is never emitted", so EVERY runtime — not just Codex — was being handed a deprecated shape. Fixed at the one routing seam rather than per caller: - The routing table now stores BARE command names (`execute-phase`), never a prefixed literal. A prefixed literal in the table is what leaked. - A single `projectNextCommand(bare, runtime, tail)` helper runs every return path through `formatGsdSlash`, preserving the argument tail (`01 --gaps`) untouched. An empty command stays empty, so "no next step" never becomes a bare prefix. - `readVerificationStatus` accepts `opts.runtime`; `cmdVerificationStatus` and `phase complete` pass `resolveRuntime(cwd)`. The default is `claude`, which yields the canonical `/gsd-` hyphen form. All four routed states are covered: missing, unknown, gaps_found, stale. `init.cts` keeps its own projector deliberately. It already formats correctly, and its command CONTENT differs from the router's on purpose (it appends the phase number to `execute-phase`, and routes `human_needed` to `verify-work`). Consolidating them would silently change `init`'s user-visible output, which this issue did not ask for — so the divergence is left intact and the new tests instead pin the property that matters on both surfaces: no raw colon form escapes. Failing-first record: `origin/next:src/verification.cts` carried the four `/gsd:` literals (lines 101, 108, 382, 392), and 11 existing assertions in tests/verification-status.test.cjs asserted the colon form. Those 11 are corrected in this commit — they passed before the fix and fail after it, which is precisely the regression this closes. Tests are folded into the module's primary suite rather than added as a third file (`lint-test-file-count` caps the `verification` module at two, and consolidating is its documented remedy — growing the allowlist is not). The `phase complete` assertion reads `res.error`, not `res.stderr`: `runGsdTools` exposes a clean non-zero exit's stderr as `error`, and reading the wrong field yields '' and makes the whole check vacuous — which is how this user-visible path stayed untested. Closes #2617 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2617): scope the new hooks to their describes; cover gaps_found through the CLI Two findings from the orthogonal review of the first commit, both in the tests this change added. 1. The folded block's `beforeEach`/`afterEach` were declared at MODULE scope. node:test applies module-scope hooks to every test in the file, so hooks added for the #2617 suites also wrapped the ~40 pre-existing tests in verification-status.test.cjs — making an unrelated block a single point of failure for them (currently benign, but a throwing hook would have failed suites it has nothing to do with). They now install inside their own describes via a small `useProjectionPhaseDir()` helper, with a comment recording why. 2. The live-CLI `phase complete` test exercised only the `missing` state, so a regression in any other routed branch would have shown up in the router's return object but not in the text a user actually reads. Added a `gaps_found` case per runtime, asserting the projected `plan-phase <N> --gaps` reaches the blocked-completion error. Whole file verified green: 48 tests, 48 pass — the ~40 pre-existing ones included. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * test(#2617): correct the last colon-form assertion in phase.test.cjs The remote run surfaced one more stale assertion outside tests/verification-status.test.cjs: the `phase complete` canonical-gate suite matched the blocked-completion message against `/\/gsd:verify-work 0?1/`. That project fixture configures no runtime, so it takes the `claude` default, which now yields the canonical `/gsd-verify-work 01` hyphen form. The colon form this asserted is exactly the deprecated shape #2617 removes — `runtime-slash.cts` documents that "the colon form is never emitted". Like the eleven corrected in the first commit, this assertion passed before the fix and fails after it, which is the regression record rather than a test being loosened: the surrounding assertions (failure reason, `stale` wording, and that neither ROADMAP.md nor STATE.md was mutated) are untouched. Verified against the real CLI: the emitted message is now "Phase 1 verification is incomplete: Verification is stale. Re-run verify-work before transition. Next: /gsd-verify-work 01". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * fix(#2617): collapse the two verification projectors into one seam The orthogonal review found that `init.cts` carried a second, independently maintained `verificationNextCommand()` that had drifted from the router's table in CONTENT, not just formatting: state router (before) init.cts missing execute-phase execute-phase <N> unknown execute-phase execute-phase <N> human_needed "" (no command) verify-work <N> The `human_needed` row is the sharp one: two GSD surfaces disagreed about whether a next command existed at all, and the router's own next_action told the user to "re-run the verify step until status is passed" while naming no command to run. init's answers were the useful ones, so the router adopts them and init now delegates to it — satisfying the issue's "keep one verification-routing seam" direction. `verificationNextCommand()` is deleted. Appending the phase number surfaced a trap the old bare commands hid. `extractPhaseToken` also returns project-code forms (`PROJ-07`), which are indistinguishable by shape from an ordinary directory name — `gsd-651-parent` yields `gsd-651` — so deriving the argument blindly emits `execute-phase gsd-651`. The number is therefore appended only when it is unambiguously numeric, or when the caller supplies it explicitly. `init` does supply it: its `phaseDir` is unresolved in several branches, where the router could not derive one at all. dir `01-example` -> $gsd-execute-phase 01, $gsd-verify-work 01 dir `gsd-651-parent` -> $gsd-execute-phase, $gsd-verify-work Suites verified green against the built lib: verification-status 50/50, phase 268/268, init 143/143, init-manager 40/40. `npm run lint:ci` clean. User-visible change beyond the reported bug, as agreed: `query verification.status` and `phase complete` now append the phase number for missing/unknown, and emit `verify-work <N>` for human_needed where they previously emitted nothing. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * chore(#2617): backfill changeset PR number (#2700) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>	2026-07-27 10:18:10 -04:00
Tom Boucher	a7d83dc234	fix(#2390): warn on goal-shaped phase.add titles, correct auto-detect docs (#2425) ... * fix(#2390): phase.add title warning + auto-detect doc fix phase.add now returns a `warning` field when a description reads as goal-shaped (>80 chars and/or multi-sentence) rather than title-shaped, instead of silently writing the whole paragraph verbatim as the `### Phase N:` header. The CLI still creates the phase as-is (the strict two-layer slash-vs-CLI interface is unchanged); the warning just surfaces the gap. Also clarifies six doc sites (command argument hints, workflow detection steps, and how-to/reference docs) that described the phase-number argument as "auto-detecting" the next unplanned phase -- that detection is an orchestrating-workflow/LLM step reading ROADMAP.md (concretely: `query roadmap.analyze`'s `next_phase` field), not a `gsd-tools.cjs` CLI feature. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#2390): regenerate fixtures + lint gate-prep * fix(#2390): repair failing tests after gate verification * chore(#2390): add changeset (#2425) --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-19 07:53:04 -04:00
Tom Boucher	19c1f54a2a	fix(#2316): stop phase complete silently dropping ghost requirement IDs (#2339) ... * test(#2316): fail-first tests for ghost REQ-IDs, v-heading over-match, all-orphan gap check Red phase: 4 of 10 fail against current source (#2316-1 ghost-ID warning, -3 requirements_updated honesty, -4a v1-heading suppression, -6b all-orphan gap rows). The other 6 are controls/boundaries that must keep passing — including the #1159 deferred-heading guard and the literal "TBD" placeholder boundary. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * fix(#2316): stop phase complete silently dropping ghost requirement IDs phase complete parses a phase's `**Requirements**:` line from ROADMAP.md and reconciles it into REQUIREMENTS.md. When a cited ID was registered nowhere, every branch degraded to a no-op and the report was indistinguishable from a run that applied every update: `requirements_updated: true, warnings: [], has_warnings: false`, file byte-for-byte unchanged. Four defects on that path, all long-standing (traced to 2bc295b32d, v1.2.0): 1. The only cross-check compared REQUIREMENTS.md's own body against its own Traceability table, so an ID cited by ROADMAP but defined in neither was invisible to it. `citedReqIds` is now hoisted out of the `if (reqMatch)` block and cross-checked; ghost IDs raise a warning through the existing warnings/has_warnings surface that execute-phase.md already prints. 2. `if (reqUpdate.ok)` had no `else`, so a Traceability-row write matching nothing was discarded silently. Misses are now recorded. 3. `requirements_updated` was set unconditionally inside `if (existsSync)`, reporting "the file was in the transaction" rather than "a write landed". It now reflects whether the content actually changed. 4. DEFERRED_HEADING_RE's bare `v\d+` alternative treated an active `## v1 Requirements` heading as deferred, zeroing the body scan for that section. Dropped; `deferred|backlog|future` still match, so #1159's suite still holds. Also fixes the same class in gap-checker: the `items.length === 0` early return fired before ghost rows were folded in, so an all-unregistered phase reported LESS than a partially-unregistered one. It is now gated on ghostReqIds too. Follows the #2140 precedent in milestone.cts, which hardened this exact class for `requirements mark-complete`; adapted to this function's single `warnings` array rather than adding fields execute-phase.md never reads. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * fix(#2316): address review — milestone-aware deferred headings, probe-based ghost check Independent review found the first pass green but wrong — the tests were built to miss the shape that breaks. BLOCKER: dropping the `v\d+` alternative from DEFERRED_HEADING_RE regressed #1159 against GSD's OWN shipped template. gsd-core/templates/requirements.md:35 ships `## v2 Requirements` with the deferred-ness in the BODY PROSE ("Deferred to future release"), not the heading — so `v\d+` was the only alternative matching it. Deleting it made every scaffolded project emit a false traceability warning on every phase complete, forever. The real problem is that `v\d+` matched BOTH the active v1 and the deferred v2: deleting it swings from suppress-everything to suppress-nothing. The heading check is now milestone-aware — a `## v<N>` heading is deferred only when <N> is not the current milestone's major, resolved through the existing state.cjs seam. Unresolvable milestone fails safe to the old always-deferred behavior, since suppressing a warning beats spamming every project. HIGH: the ghost check re-derived membership from two lossy index sets that disagreed with the case-insensitive write paths, so it warned "not registered anywhere" about an ID whose checkbox it had just ticked, and about a case-mismatched ID whose write landed. It now probes the same two write surfaces the writes use, mirroring milestone.cts's #2140 precedent (probe the write, don't re-derive it) — which is what the original brief asked for and the first pass didn't do. HIGH: the cited-ID tokenizer split the rest of the line on whitespace, so every trailing word became a "cited REQ-ID". The shipped roadmap.md template puts an inline HTML comment on that exact line, so a fully correct run told users to register `<!--` and `-->` as requirements. Cited IDs are now filtered to the REQ-ID shape, matching what bodyReqIds/tableReqIds already require. HIGH: gap-checker had the identical early-return defect 34 lines above the one fixed — the could-not-parse branch gated ghost rows behind the same unguarded items.length. One malformed <decisions> line made all ghost rows vanish. Tests: the #2316-5 guard only covered `## Deferred`/`## Backlog`/`## Future` — headings that trivially still match — omitting `## v2 Requirements`, the only shape the change altered. Added the shipped-template fixture plus guards for each finding above. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA * docs(#2316): backfill PR number 2339 into changeset Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SLufH5sDuqA1AiEGu45cuA --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>	2026-07-17 06:49:58 -04:00
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>	2026-07-15 15:33:58 -04:00
Tom Boucher	8b70db343b	fix(#2204): phase-completion writes 'All phases complete' per ADR-2207 (#2259) ... * fix(#2204): phase-completion writes 'All phases complete' per ADR-2207 completePhaseCore was writing the overloaded bare 'Milestone complete' on the last phase — the same string space the milestone-close verb owns for terminal state. Per ADR-2207, phase-completion now writes the existing intermediate value 'All phases complete' (already used in gsd2-import.cts). Milestone termination ('<version> milestone complete' / 'Awaiting next milestone') remains solely with milestoneCompleteCore. Status lifecycle: Ready to plan → All phases complete → <version> milestone complete → Awaiting next milestone. Changes: - src/state-transition.cts: completePhaseCore status value - src/phase.cts: #2028 guard comment - tests/state-transition.test.cjs: assertion + test name - tests/phase.test.cjs: 8 assertion updates (positive + negative) - tests/state.test.cjs: normalizeStateStatus test case + reset regex - tests/workstream.test.cjs: fixture status to terminal value - gsd-core/workflows/progress.md: Route D label - gsd-core/workflows/transition.md: Route B label - CONTEXT.md: Status lifecycle glossary entry (ADR-2207) - .changeset/brave-geese-jump.md * test(#2204): regenerate golden-install-parity fixtures + workflow-size baseline Workflow file edits (progress.md, transition.md) changed install payload hashes and pushed past the committed workflow-size baseline. Regenerated all 17 golden-install-parity fixtures + claude-local via the standalone gen script (which now also covers the local-scope claude layout). Updated workflow-size-baseline.json and agent-size-baseline.json via size:baseline. * fix(#2204): correct claude-local golden hashes + document gen-script limitation The gen-script's claude-local generation produces macOS-specific hashes incompatible with Linux CI (local-scope install embeds platform-varying node-runner paths). Reverted to manual update using Linux FAILURES.md +actual hashes for the 2 changed workflow files. Added explanatory comment in the gen script. * test(#2204): add isCompletedInventory coverage + clarify CONTEXT.md glossary Addresses orthogonal code-review findings (Medium #1 + #2): - Add isCompletedInventory test cases for ADR-2207 status lifecycle (terminal 'milestone complete' → true; intermediate 'All phases complete' → false; archived → true; active statuses → false) - Clarify CONTEXT.md glossary: note that isCompletedInventory intentionally excludes the intermediate value * docs: backfill changeset PR number (#2259) * docs(#2204): add Status lifecycle table to state-md reference (ADR-2207)	2026-07-14 14:47:03 -04:00
Tom Boucher	c1885df9e5	chore(#2143): prohibition-with-teeth + migrate remaining ad-hoc table sites — Phase 4 (final) (#2253) ... * chore(#2143): prohibition-with-teeth + migrate remaining table sites — Phase 4 Phase 4 of epic #2143 (ADR-2143 §7). Completes the markdown table/mutation consolidation by (a) giving the ad-hoc-parsing prohibition teeth and (b) migrating the last ad-hoc table sites onto the shared seam. - src/markdown-table.cts: new formatting-preserving `updateTableCell` primitive (self-contained, ragged-row-tolerant header/delimiter/cell-range scan; splices only the target cell's raw span, preserving all other bytes incl. padding/CRLF; no-op-preserves-padding when a transformer returns the current value). Exports splitTableRow/isDelimiterRow/findTableStartOffset for tolerant reuse. - eslint-rules/no-adhoc-markdown-parsing.cjs: TABLE-REGEX detector extended to `new RegExp(<literal|static-template>)`; new `.replace()`-mutation detector for roadmap/state/content receivers with a table/section-shaped pattern. - scripts/lint-table-schema-drift.cjs (wired into lint:ci): fails if a TABLE_SCHEMA header drifts from its authored table; tests import its logic (single source). - Migrated onto the seam (behaviour-preserving vs pre-Phase-4 HEAD, verified byte-diff old-vs-new): roadmap.cts cmdRoadmapUpdatePlanProgress, phase.cts cmdPhaseComplete + traceability, milestone.cts cmdRequirementsMarkComplete, uat.cts read path, state.cts metrics/decisions/By-Phase. - Incidental correctness gains from the migration: a decoy table can no longer swallow a phase-progress update (## Progress scoping); a ragged neighbouring row no longer silently aborts an edit; completing integer phase N no longer touches a decimal sub-phase N.x row; record-metric no longer drops trailing section content or duplicates the ## Performance Metrics section. - Kept justified allow-adhoc-markdown markers only where genuinely not a table (security.cts <|role|> token) or a loose non-GFM section (uat human-verify). Two orthogonal isolated reviews (correctness/adversarial + security) passed; correctness found 4 behaviour regressions in the first migration pass, all fixed and re-verified byte-identical-or-better vs OLD. Surfaced for maintainer (pre-existing, ambiguous domain logic, NOT changed here): templates/state.md places a By-Phase table under ## Performance Metrics while cmdStateRecordMetric assumes a Plan|Duration|Tasks|Files table. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): match traceability row by first-cell value, not Requirement header Phase 4's migration matched the REQUIREMENTS.md traceability row by a column literally named `Requirement` (`row['Requirement']`), but real tables head that column `REQ-ID`. The by-name lookup found nothing, so `phase complete` and `requirements mark-complete` left the Status cell `Pending` (regressed #2769 / #2203, caught by gsd-test — 8 failures, both node 22/24). - src/phase.cts, src/milestone.cts: match the row by its FIRST cell's value (the requirement-ID column) regardless of that column's HEADER name, via `Object.values(row)[0]` (updateTableCell builds the record in header order). This mirrors OLD's first-cell `\|\s*<id>\s*\|` anchor, restoring header-name independence while keeping the seam. - src/milestone.cts hasTable: broadened from `Requirement`-only to also recognize `Requirement ID` / `REQ-ID` / `REQ ID` headers, kept in sync with the now-positional rowMatch/hasRow so a REQ-ID-headed table participates in the ADR-2143 §6 write-set and the #2140 table_unmatched drift check (it was silently omitted before — a checkbox-only partial reconcile against a REQ-ID table could report as fully reconciled). The `Requirement`-headed path is byte-identical to OLD. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2143): replace stale structural milestone guards with behavioural suite The `milestone.cjs regex global state fix` block was a source-structure guard (allow-test-rule: structural-regression-guard) — it readFileSync'd the compiled milestone.cjs and asserted removed regex idioms (`tablePattern.test`, `afterTable !== reqContent`, `doneTable = new RegExp(...)`). Phase 4's migration deleted those regexes (table update is now updateTableCell), making the assertions obsolete. Per the Test Cleanup rule, replace them in-PR with a behavioural suite driving the compiled CLI: - multi-ID mark-complete flips all IDs (guards the lastIndex/global-state class), - Pending->Complete flip under both `REQ-ID` and `Requirement` headers (#2769), - idempotent already_complete detection with no corruption, - REQ-ID-headed table participates in write_set (traceability entry, applied), - REQ-ID-headed table trips #2140 table_unmatched drift on a missing row. Pruned the now-nonexistent structural-regression-guard entry from the lint-allow-test-rule-refs allowlist (the source-text-is-the-product entry for the same file remains valid). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(changeset): backfill PR number 2253 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): record-metric targets its own metrics table, not By-Phase velocity `state record-metric` appended its per-plan row (`| Phase 1 P1 | 5min | 3 tasks | 4 files |`) into the FIRST table under `## Performance Metrics` — which on a real template-derived STATE.md is the By-Phase velocity table `| Phase | Plans | Total | Avg/Plan |`, polluting it on EVERY plan completion (execute-plan.md:414 is a per-plan call). The command's own metrics table is `| Plan | Duration | Tasks | Files |`, which the template does not ship, so the row never reached it; the scaffold branch also emitted a wrong `| Phase | Plan | Duration | Notes |` header matching neither the row nor the canonical table. Pre-existing (predates Phase 4); surfaced while migrating this site and fixed here per no-defer, on the user's explicit go-ahead. - src/state.cts cmdStateRecordMetric: locate the metrics table by its own header shape (`Plan|Duration|Tasks|Files`, via splitTableRow/isDelimiterRow) rather than "first table in the section". When the section exists but has no metrics table (only the By-Phase table), self-heal by appending a fresh **Per-Plan Metrics:** table to the END of the section body — By-Phase table, Recent Trend and footer preserved verbatim, no duplicate `## Performance Metrics` heading, created stays false. Absent-section scaffold header corrected to the canonical `| Plan | Duration | Tasks | Files |`. Ragged-tolerance + None-yet preserved. - Not touching templates/state.md (golden-install-parity hashed) — record-metric self-creates the table on first use instead. Failing-first regression test (tests/state.test.cjs) demonstrates the By-Phase pollution on the pre-fix build, then green after. Verified: no pollution, self- heal idempotency, both-tables isolation, content/heading preservation, flags, None-yet, corrected scaffold header (23-check adversarial harness + all existing record-metric scenarios). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#2143): deleteSection seam primitive (level-bounded whole-section removal) ADR-2143 §4 shipped withSection/collectSection (replace a section BODY) but no way to DELETE a section (heading + body). Phase 4 suppressed the phase-remove section delete instead of building it. deleteSection(content, predicate, opts) locates the section via the collectSection machinery and splices out from the heading's start offset to the next same-or-higher-level heading — so a level-3 `### Phase N` delete stops at a following level-2 `## Progress`, never past it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): phase remove no longer deletes ## Progress on last-phase removal updateRoadmapAfterPhaseRemoval deleted a `### Phase N` detail section with a greedy raw regex whose lazy scan, on the LAST phase, ran to EOF and destroyed the following `## Progress` heading and its entire tracking table — silent data loss, uncovered by tests (removal tests only exercised a middle phase). Migrated onto the new deleteSection seam (level-bounded, stops at `## Progress`); dropped the allow-adhoc-markdown SECTION-DELETION suppression. Failing-first regression (tests/phase.test.cjs) removes the LAST phase and asserts the ## Progress heading + table survive; middle-phase removal is byte-identical. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#2143): deleteTableRow seam primitive (row removal, ragged-tolerant) Sibling of updateTableCell: locates the first GFM table, matches a DATA row by predicate (ragged-tolerant record build, header order), and splices out that row's whole line preserving every other byte. Returns {ok:false,reason} on no table / no match. Enables migrating the phase-remove Progress-table row delete off its ad-hoc regex (ADR-2143 §7 — the "future row-delete seam" Phase 4 punted). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): phase remove deletes the Progress row via deleteTableRow The Progress-table row delete used a whole-document regex with two defects: (a) `\.?\s` required whitespace after the phase number, so a COMPACT row `|2|Beta|` was never deleted (stale row left behind); (b) unscoped — it could strike a row in a different table (e.g. an earlier `| Phase | Requirements |` table). Migrated onto deleteTableRow, scoped to the `## Progress` section (mirrors deriveProgressFromRoadmap), matching the row by first-cell phase number (integer zero-pad-insensitive; decimal exact; removing `2` never touches `2.5`). Both allow-adhoc-markdown suppressions removed. New behavioural tests: compact unpadded row deleted; padded byte-parity on the surviving rows (their ordinal correctly renumbers via the pre-existing renumber block). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): deleteTableRow leaves no dangling newline on last EOL-less row Deleting the final row of a table with no trailing EOL sliced from the row's start to end-of-string, stranding the newline that terminated the previous line. Back rowStart over the preceding \r?\n in that branch so the table ends cleanly. (Caught by the primitive's own unit test on gsd-test; local scenario checks missed the no-trailing-EOL edge.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): migrate read-only section-collects onto collectSection Six hand-rolled `## Section` read-extract regexes replaced by the collectSection seam (behaviour-preserving; extracted bodies feed the same downstream parsers): state.cts matchSessionSection (## Session / ## Session Continuity) + ## Blockers, smart-entry.cts ## Blockers, audit.cts ## Current Focus + ## Open Questions. Removes 6 allow-adhoc-markdown "pending #1372" suppressions. Incidental fix: the old Session regex `## Session[ \t]*\n` silently failed on a CRLF `## Session\r\n` heading (Windows STATE.md), nulling all session fields; collectSection is CRLF-safe, so session state now resolves on Windows. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): fence-safe state-transition section writes + dedup stripFrontmatter - milestoneCompleteCore's `## Current Position` and `## Operator Next Steps` section resets used fence-blind raw regexes that a fenced `##` inside the body could truncate/mis-target (#2130/#2067/#2080 class). Migrated onto a fence-aware tokenizeHeadings-based helper (resetSectionVerbatim) that is byte-identical to the old output on the canonical path (9/9 fixtures) and correctly ignores a fenced fake heading (proven robustness gain). - mutateCurrentPositionFirstTime: hand-rolled locate+splice → collectSection + replaceSection (byte-parity). - stripFrontmatter was inlined byte-identically in state.cts AND state-transition.cts; hoisted the single canonical copy into frontmatter.cts (both call sites now import it) + unit tests — eliminates the divergence risk per CLAUDE.md "Generative Fix Divergence". Removes 3 allow-adhoc-markdown / #1372 markers. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): name-address By-Phase sum + uat parse, eslint recall hole, catches - state.cts By-Phase "Total plans completed" sum: positional 2nd-cell regex → name-addressed splitTableRow read (correct on a reordered header, where the old code silently summed the wrong column). Marker removed. - uat.cts parseVerificationItems: loose pipe regex → splitTableRow within the existing table/numbered/bullet union scan (item list byte-identical; does NOT reintroduce the reverted strict-parseMarkdownTable item-drop). Marker removed. - eslint no-adhoc-markdown-parsing: close the `new RegExp(identifier)` recall hole — resolve a const-declared table-shaped regex identifier (mirrors the .replace() detector) + RuleTester cases; param/call args stay out (boundary). - commands.cts: delete a lying comment that claimed the scaffold date "stays on raw UTC / deferred" — #2136 already moved it to realClock.localToday(). - Empty catches (classified, not blind-swept): removed 4 dead try/catch; fixed 3 error-hiding (phase-insert decimal-dir I/O collision now fails loud; phase-remove rename partial-failure surfaced; milestone-archive true count via finally); left best-effort swallows with justification comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): extractFencedBlock seam + migrate api-coverage named fence parseCoverageMatrix extracted its ```coverage fenced block with an ad-hoc regex (the last real allow-adhoc-markdown suppression). Added extractFencedBlock to the markdown-sectionizer seam (reuses stripFencedCode's CommonMark fence engine — info-string match, ~~~/backtick, nesting, indent) and migrated onto it; byte- parity on the parsed CoverageMatrix across 8 fixtures. Only security.cts:367 (a genuine `<|role|>` protocol-token false-positive, not a GFM table) remains marked in src/ — the "prohibition with teeth" goal (nothing grandfathered but a true FP) is met. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): By-Phase row insert is name-addressed (insertTableRow seam) updatePerformanceMetricsSection's INSERT-new-row branch located the By-Phase table with a canonical-column-order-only regex + a hardcoded positional row literal, so on a reordered header it silently inserted nothing — inconsistent with the now name-addressed UPDATE and SUM halves of the same function. Added insertTableRow (markdown-table seam sibling of updateTableCell/deleteTableRow: name-addressed, header-order-agnostic, EOL-preserving) and migrated the branch onto it, mapping By-Phase values by column NAME. Canonical-order output is byte-identical; a reordered header now inserts a correctly-mapped row; a pre-existing CRLF mixed-EOL splice glitch is incidentally fixed. Retired the now-dead byPhaseTablePattern const. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): phase-list checkbox flip via updateBullet seam Added updateBullet (markdown-sectionizer): a fence-aware, offset-tracked single-bullet write primitive (GFM 1–4-space marker tolerance) — the write counterpart to read-only iterateBullets. Migrated mutateMilestonePhase's phase-list checkbox flip (`- [ ] Phase N …` → `- [x] … (completed <date>)`) off its whole-slice regex onto it, same milestone-slice scope + clock seam. Byte-identical across simple / idempotent / metachar-title / double-space / CRLF scenarios. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): scope the Progress-ordinal renumber to ## Progress via seam phase remove's integer-renumber decremented Progress-table phase ordinals with a whole-document `content.replace(/(\|\s*)(\d+)(\.\s)/g, …)` — unscoped, so it also rewrote any `| N. …` cell in an unrelated/decoy table (same class as the batch-2 row-delete scoping bug). Migrated onto updateTableCell, scoped to the ## Progress section, decrementing each affected row's leading phase ordinal by column name. Byte-identical on canonical Progress tables + multi-row + decimal-sibling cases; a decoy `| 3. … |` row before ## Progress is now correctly left untouched. The sibling heading / checkbox-bullet / PLAN.md-filename / Depends-on-prose renumbers are not GFM-table mutations (outside ADR-2143's table/section mandate) — left as-is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2143): review fixes — scope traceability write, restore Current Position H3-stop Adversarial review of the remediation (BLOCK verdict) — all 9 findings fixed: - F1 (BLOCKER): requirements mark-complete / phase complete flipped the checkbox but NOT the traceability row on the shipped template, because updateTableCell bound to the FIRST table (## Out of Scope, no Status column) instead of the ## Traceability table — the #2140 silent-divergence class, re-introduced by the seam migration and missed by tests (fixtures had Traceability first). Scoped the write + hasRow probe to the ## Traceability section slice (updateTraceability Cell helper) in milestone.cts + phase.cts. Failing-first tests on the Out-of-Scope-before-Traceability layout; the #2769 first-cell match preserved. - F2 (MAJOR): mutateCurrentPositionFirstTime restored to locateCurrentPosition (STOP_H2_PLUS) — collectSection's default H2-stop swallowed a level-3 subsection and the field regexes clobbered it (#2130 class). - F3/F8: Progress-ordinal renumber re-escapes via escapeCell + keys padding recovery by row index (was de-escaping `\|` and losing padding on dup values). - F4: insertTableRow escapes cell values internally. - F5: updateBullet accepts a tab after the marker (`[ \t]{1,4}`). - F7: resetSectionVerbatim consumes CRLF blank lines (byte-parity on CRLF). - F6/F9: corrected two misleading comments. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(changeset): data-loss + CRLF-session user-facing fixes (#2253) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2143): de-flake the G10 windsurf ReDoS-guard wall-clock assertion The G10 test asserted `elapsedMs < 1000` for a 200k-char payload — a wall-clock assertion (CLAUDE.md: never assert on wall-clock time) that flaked on a loaded node24 bench at ~1.1s. It was redundant: runHook's spawnSync `timeout: 10000` already SIGKILLs a catastrophic-backtracking hook, so the exit-0 assertion is the real ReDoS guard. Removed the timing assertion; kept exit-0 + documented the subprocess-timeout mechanism. Surfaced (not caused) by this branch's gsd-test runs loading the bench; unrelated to the markdown-parsing changes but fixed in place per the no-flaky-tests rule. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-14 14:25:44 -04:00
Tom Boucher	b321bc04f4	fix(#2128): bound the sibling bracket-prefix clause — complete the ReDoS fix ... Review caught that the prior commit bounded only the paren tag clause and left the SIBLING bracket-prefix `(?:\[[^\]]+\]\s*)?` (same host regexes, before Phase) UNBOUNDED — the identical quadratic reachable via a `[...]` run (measured ~16s at 1.7MB). Bound `[^\]]+`/`[^\]]*` -> {1,200}/{0,200} across all 19 phase/milestone heading prefixes. Comprehensive re-measurement now shows EVERY vector linear (bracket/paren/id/name/milestone all ~2-44ms at 2.45MB; bracket scaling 2k->2ms, 4k->5ms, 8k->10ms). Also: update the #1729 literal-mirror parity test off its stale unbounded constant, and add limit-1 (199) boundary coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-10 09:44:49 -04:00
Tom Boucher	c1cd43a39f	fix(#2128): bound the phase-tag clause to {0,200} — kill quadratic ReDoS ... The canonical OPTIONAL_PHASE_TAG_SOURCE tag clause `(?:\s*\([^)\n]*\))?` (and its inlined literal mirrors across 11 modules) had an UNBOUNDED body, making the optional-group + /g header scan quadratic on adversarial ROADMAP.md/STATE.md — a long run of `(` after a header ran ~18.8s at 1.7MB. Bound the body to {0,200} in the constant AND every mirror in lockstep (the #1729 "both forms change together" contract), so the scan is linear: the same 1.7MB input now resolves in ~9ms (measured), while real tags (a handful of chars) still match and a 201-char tag is rejected. Added a #2128 boundary regression to the #1729 suite. Pre-existing (byte-identical before/after the Phase 4 migrations); folded in at maintainer direction rather than deferred. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-10 09:31:17 -04:00
Tom Boucher	597ee3248a	test: add failing regression for #2067 phase-complete checkbox regex ... The checkbox regex in cmdPhaseComplete uses a greedy .* between ] and 'Phase N'. Completing an already-checked phase (idempotent re-run) wrongly matches a later phase whose description mentions the target phase. These tests encode the exact repro from #2067; they fail on next @ origin/next.	2026-07-08 10:52:55 -04:00
Tom Boucher	51dfa683d4	fix(#2028): phase.complete milestone-end out-of-order + workstream root-fallback guard ... Two code-confirmed defects in `gsd-tools phase complete` (re-verified against next; the three severe corruption paths the issue filed are superseded by the ADR-1769 Transition Module migration + #2012, so this is the confirmed remainder). 1. Milestone-end mislabel (isLastPhase). The milestone-end determination only cleared isLastPhase when a HIGHER-numbered phase existed, so completing the numerically-highest phase out of order (e.g. Phase 10 before Phase 9) stamped STATE.md `Status: Milestone complete` while a lower phase was still outstanding. Added a lower-phase check: after the existing higher-phase scans, if any earlier phase in the current milestone has an unchecked roadmap checkbox (`[ ]`), isLastPhase becomes false AND next_phase/next_phase_name point at the LOWEST outstanding lower phase — so STATE.md advances to the real gap instead of parking on the just-completed phase. A completed phase always has `[x]` (phase.complete sets it), so all-lower-complete still reports milestone-end; heading-only roadmaps (no checkboxes) retain prior behavior. The checkbox regex mirrors the sibling phasePattern's anchoring (whitespace/bold + required `:`) so unrelated checklist lines mentioning "Phase N" don't match. 2. Workstream root-fallback (no guard). cmdPhaseComplete resolves every path via planningDir(cwd); with a `workstreams/` dir present but no active workstream and no --ws, that returns root `.planning`, so phase.complete wrote STATE.md/ ROADMAP.md (and the mislabel) into the shared root other workstreams read. Added the same #1912 fail-safe guard init.progress got: refuse (asking for `--ws`/active workstream) instead of silently writing root. Resolution itself was already wired globally (resolveActiveWorkstream: --ws > GSD_WORKSTREAM > pointer, set in bin/gsd-tools.cjs), so only the refusal guard was missing. The workstream-mode detection (`listAvailableWorkstreams`) is extracted into planning-workspace.cts as the single source of truth and consumed by BOTH init.progress and phase.complete, so the two fail-safe paths cannot drift. Tests (tests/phase.test.cjs, new #2028 describe): out-of-order completion becomes `Ready to plan` with is_last_phase=false, next_phase pointing at the outstanding phase and Current Phase advancing to it (not the completed phase); all-lower- complete still reports milestone-end; workstream-mode-no-active refuses with an `--ws` hint; `--ws` completes in the workstream leaving root untouched; flat mode unaffected. Fail-first verified locally via direct gsd-tools invocation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-07 14:40:44 -04:00
Tom Boucher	f77179248d	fix(#2012): scope phase.complete Progress-row regex to ## Progress section (#2032) ... * fix(#2012): scope Progress-row regex to ## Progress section (was binding to earlier table) The Progress-row writer used a non-global regex that matched ANY table row starting with the phase number. When an earlier table (e.g. Requirements coverage | Phase | Requirements | Count |) preceded ## Progress, the regex bound to the wrong row (3-column), no-op'd, and never reached the real Progress row. roadmap_updated stayed true (it's existsSync), masking the failure. - src/phase.cts: scope the tableRowPattern regex to the ## Progress section (indexOf + slice) so it only matches Progress-table rows. - tests/phase.test.cjs: regression test — ROADMAP with a phase-numbered Requirements table before ## Progress → Progress row updated, Requirements row untouched. Closes #2012 * docs(#2012): backfill changeset pr 2032	2026-07-05 17:39:11 -04:00
Tom Boucher	2c32e8d890	test(#1973): consolidate 48 workflow regression tests into workflow-aspect suites ... Fold 48 issue-named workflow-markdown regression files into the canonical test that owns each workflow aspect (execute-phase-*, plan-phase-*, quick-*, discuss-*, worktree-cleanup, secure-phase, verify, update, settings, etc.), across 32 existing suites. Verbatim block-scoped describe wrappers; 281 subtests conserved 1:1. No new test files. Host-env pre-check: only gsd-settings-advanced spawns CLI and it sets no GSD_WORKSTREAM/GSD_PROJECT value — no leak risk. Regenerates regression-name allowlist (222->181), ratchets file-count allowlist (verify 11->10), makes 30 relocated allow-test-rule exemptions issue-ref-compliant (ADR-456; prunes 30 stale ids). lint:ci green. Part of epic #1969. Closes #1973. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>	2026-07-03 09:17:32 -04:00