16 Commits

Author	SHA1	Message	Date
Jakub Zych	6cfa0c55d2	refactor: drop 12 runtimes, keep Claude, Codex, OpenCode, Cursor, ZCode, Antigravity ... Removes kilo, kimi, kimi-code, copilot, windsurf, augment, trae, qwen, hermes, cline, codebuddy and pi end to end: capability descriptors, installer branches and converters (bin/install.js 14.9k -> 11.2k lines), TypeScript converters, hook surfaces and runtime homes, review lanes qwen/kimi-code, the two pi migrations, Kimi payload normalization in the hook guards, dead hostBehaviors vocabulary, launcher home probes, fixtures, runtime-specific tests and the prose that presented them as supported. Installer output for the six kept runtimes is byte-identical to before the prune. The Kimi tool-vocabulary tests in workflow-guard, read-guard and read-injection-scanner are left in place pending a decision.	2026-10-06 20:02:40 +02:00
Jakub Zych	a9a7a328e6	refactor: hard-fork GSD -> MSD (Make Software Done) ... Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD across contents and paths, upstream package/repo coordinates -> @golem15/msd-core and golem15com/msd-core. Deep links into upstream history, sibling upstream packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is. Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line, package/plugin identity, regenerated lockfile, install-tree fixtures, derived registries and benchmark baseline; migration checksum baseline re-locked (MSD keeps its own install state, so no install had applied the old sums); sort-order and regex-escaped expectations in tests adjusted.	2026-10-06 01:47:40 +02:00
Tom Boucher	5d4c98cde7	chore(#4729): guard the retired-runtime name, and finish the locale residue (#4753) ... * chore(#4729): guard the retired-runtime name, and finish the locale residue Phase 5 of 5 on epic #4709, and the phase that closes it. Two parts, one concern: make the tree clean, and keep it clean. The guard is inert until the tree is clean, and shipping the cleanup without the guard is the one-bug-at-a-time pattern this epic exists to end. WHY A GUARD, AND WHY LAST Nothing in CI answered "does any shipped surface still present a retired runtime as live?", and the two gates that look like they should cannot. checkReviewerDocsParity is one-directional: it asserts the PRESENCE of every declared reviewer flag and never the ABSENCE of a retired one, so in #4716 it reported 0 violations while all four locale mirrors still documented --gemini as a live reviewer flag, with usage examples. And tests/gemini-runtime-removed.test.cjs is scoped by construction - its own docblock limits it to the installer CLI contract and the runtime-name-policy exports; it never reads docs/**, gsd-core/workflows/**, commands/** or agents/**. Every extension to it during this epic was a hand-added assertion for a surface somebody had already noticed. A guard written earlier would have red-flagged the very references phases 1b-4b were removing, which is why it lands last. PART A - THE RESIDUE, INCLUDING WORK I SHIPPED INCOMPLETE Each site was judged against its ENGLISH counterpart, not on its own: README.{ja-JP,ko-KR,pt-BR,zh-CN}.md :9 :24 :46 English README.md has ZERO occurrences -> substituted "Antigravity CLI, Kimi CLI" how-to/execute-a-phase.md:88 x4 locales fixed in #4728 -> substitute how-to/verify-and-ship.md:89 x4 locales fixed in #4728 -> substitute FEATURES.md cross-AI CLI list :1419 no Gemini -> DELETE FEATURES.md REQ-MULTI-RT-01 :1709 -> substitute FEATURES.md REQ-SKILLS-03 :1952 -> rewrite FEATURES.md REQ-QUOTA-02 :3256 deleted upstream -> delete VERSIONING.md:133 stale manifest -> see below The twelve README occurrences were an adversarial reviewer's BLOCKER, and the reason they survived my own sweep is structural: root-level *.md was outside the guard's scan set, so the repo's most-read runtime-advertising surface was invisible to the guard meant to police it. :46 is a live installer-runtime claim - it tells the reader the installer will offer a runtime that no longer exists. Checked for the duplicate-name trap before substituting: neither Antigravity nor Kimi appears anywhere in those four files. Two of these are mine to own: I fixed the ENGLISH execute-a-phase.md and verify-and-ship.md in #4728 and left all four mirrors behind. Unfinished work, not a deferral. Two more show why "substitute Gemini -> Antigravity" is the wrong default: in the cross-AI list and REQ-QUOTA-02 English DELETES the name, because Antigravity was already in the list or the classifier had dropped it. Substituting would have duplicated a name - the identical trap ARCHITECTURE.md:24 set in #4728, where English holds Kimi CLI in that slot. VERSIONING.md:133 is a different and worse defect than translation lag. Under "Manifest Version Sync" it listed gemini-extension.json as a version-synced manifest. That file is ABSENT from the repo, and scripts/sync-manifest-versions.cjs says so in its own comment - "#1928: gemini-extension.json was removed with the gemini runtime ... it is no longer a registered manifest" - while VERSIONED_MANIFESTS holds plugin.json, marketplace.json and vscode/package.json. So the doc named a manifest that does not exist AND omitted the one that replaced it. Both fixed, verified against the owning code rather than inferred from the name. The replacement bullet cites #1942, the issue that actually registered vscode/package.json, matching the convention of its neighbours. pt-BR/FEATURES.md is a 77-line stub genuinely lacking two sites, and ko-KR has no REQ-QUOTA-02 line. Skipped and recorded, never invented. PART B - THE GUARD scripts/lint-retired-runtime-name.cjs, modelled on scripts/lint-legacy-dir-name.cjs - the repo's own precedent for this problem shape (forbid a retired token, allowlist frozen content, self-exempt via a split literal, a REPO_ROOT test seam, lib/cli-exit.cjs, exit 0/1). Case sensitivity IS the mechanism, not an accident. The naive guard - "the string gemini must not appear" - is WRONG, not merely noisy: that string is load-bearing across Antigravity's real on-disk contract. A case-sensitive, standalone, capitalised name works because every legitimate reference is spelled differently and therefore cannot match: lowercase config homes (~/.gemini/antigravity, ~/.gemini/config, #3738), lowercase hyphenated model ids (gemini-2.5-flash-lite), uppercase env vars (GEMINI_API_KEY), and GEMINI.md. Table-driven, so the next retired runtime costs one row. THE ALLOWLIST IS THE ENTIRE RISK SURFACE, so it is three tiers, not one. Two rounds of isolated adversarial review reshaped it; both are recorded in .gsd/bug/chore-4729-gemini-drift-guard/60-review.json. ROUND 2 FOUND ONE ROOT CAUSE BEHIND TWO SEPARATE HOLES, and it was mine: both Tier-1 rules treated the ABSENCE of a runtime word as a GRANT. A veto list can never be complete, so "no runtime word found" silently exempted every phrasing nobody had enumerated. Demonstrated: `The installer now offers Gemini 3.`, `Supported agents include Gemini 3, Kimi, and Cursor.` and three more exited 0, as did `Suportamos Gemini, no estilo padrao, como runtime de instalacao.` and `Gemini 兼容，并且是受支持的运行时之一。`, both of which literally contain `runtime` or `运行时`. The fix was to stop enumerating exceptions and invert the evidence direction: Tier 1(a) - the hook DIALECT Antigravity inherits. Position is language-dependent and MEASURED: en Gemini-style/-compatible, ja Gemini スタイル, ko Gemini 스타일/호환, zh Gemini 风格 / 与 Gemini 兼容的, pt "no estilo Gemini" / "compatível com Gemini" where the qualifier PRECEDES the name. The marker must now form an ADJACENT COMPOUND with the name, not merely sit in a +/-24-character window - that window let `| Antigravity | Gemini-style hooks | Gemini support is live |` exit 0, one legitimate reference licensing a fresh live claim 21 characters later. The runtime-word veto is now LINE-GLOBAL. Ten real lines legitimately pair a dialect compound with a runtime word (`~/.gemini/antigravity-cli` in a table cell, "runtime files" in the same sentence); each is an explicit pin rather than a reason to loosen the veto for everyone. Measured: widening it surfaced exactly those ten and no others. Tier 1(b) - the provider/model axis. A version optionally followed by a qualifier, including full-width digits and CJK punctuation, AND positive model-axis evidence on the line, AND no runtime word. The positive requirement is the part that matters: all eight real model-axis lines in the repo name a model explicitly, so requiring it costs nothing on the real tree while flagging every laundering attempt. It is also the honest resolution of the agent/target tension below - rather than guess at an exhaustive veto list, stop treating an empty veto as evidence. Tier 2 - PINNED OCCURRENCES, now SPAN-SCOPED. A pin excuses only a match falling INSIDE an occurrence of its own snippet. Line-level containment let `Known provider menu update: Gemini CLI is once again a selectable GSD runtime.` and `Install target: Google (Gemini) - choose Gemini CLI as your GSD runtime.` both exit 0, because a short snippet elsewhere on the line pre-approved a brand-new claim. Span scoping makes short snippets safe: `Google (Gemini)` can only ever excuse the match inside those 15 characters. A LOAD-TIME validator now requires every pin to contain a retired name, and it immediately caught five of MY OWN pins whose snippets sat BESIDE the name rather than covering it - each would have shipped permanently inert and permanently reported stale. All pins were then reconciled in one pass. A pin is also marked used by PRESENCE on the line now, rather than only on the Tier-2 branch. Previously a pinned line that a general rule also matched never marked its pin used, producing a provably FALSE "no line matches pinned snippet" whose printed remedy told the maintainer to delete a pin that was still needed. Tier 3 - blanket trust, and a new occurrence inside it IS invisible. CHANGELOG.md and `.changeset/` - the rendered changelog and its source, one surface - plus six append-only directories. All 21 `.changeset/` hits were measured to be fragments DESCRIBING the retirement or a fix to it, 464 of them under archived/; a fragment can only describe what already shipped and is deleted at release, so pinning them would be friction with no signal. The cost is stated in the guard's own header rather than hidden. THE SCAN SET IS NOW EVERY TRACKED *.md FILE (1165 read). The original prefix list left `.github/`, `.changeset/`, `capabilities/`, `playbooks/` and `references/` invisible - and `.changeset/*.md` renders into CHANGELOG.md, so a live claim introduced there was invisible at BOTH ends. The escape hatch must now carry a justification (`gsd-allow-retired-runtime-name: <reason>`). A bare marker is rejected: it is checked first, excuses the whole line, and the failure message advertises it, so an unexplained one is indistinguishable from a silenced defect. Plus an anti-vacuity floor counting files actually READ, not files listed - a candidate count stays healthy-looking even if every read failed. A FALSE NEGATIVE I INTRODUCED, AND CLOSED The model-display escape began as a blanket /^ \d/ - "space then a digit" - which also matched "Install for Gemini 2.5 CLI as a supported runtime.", laundering a genuine stale-runtime claim through an attached version number. That was the THIRD appearance of one failure shape in this epic: an exclusion added to suppress false positives creating a false negative. #4716's sweep excluded lines matching gemini-[0-9] to spare Google's model ids, and thereby hid a stale review.models.gemini row whose example value was "gemini-2.5-pro" ON THE SAME LINE. Round 2 then produced the FOURTH and FIFTH instances, which is why the fix this time was to invert the rule's evidence direction rather than to enumerate more exceptions. The veto is word-anchored for Latin terms - unanchored, case-insensitive "CLI" matched inside "client" and would have vetoed legitimate model lists - and raw for CJK terms, where \b is ASCII-word-based and would never fire beside an ideograph, so anchoring them would silently disable the veto in ja/ko/zh. "agent" and "target" were deliberately left OUT: both occur throughout ordinary prose ("AI coding agents (Claude Code, Codex, Gemini 2.5 Pro)"), so vetoing on them would red correct content instead of catching runtime claims. The reasoning is in the guard's comment, not just the omission - and Tier 1(b)'s positive-evidence requirement is what makes that omission safe, since the rule no longer depends on the veto list being complete. COVERAGE tests/lint-retired-runtime-name.test.cjs drives the guard through its GSD_LINT_RETIRED_RUNTIME_REPO_ROOT seam against fixture repos, mirroring tests/lint-legacy-dir-name.test.cjs. A guard never observed failing is not a guard, and this epic already shipped one that was vacuous for 2 of its 5 files, so properties are paired against BOTH failure modes - too broad silently absorbs a future defect, too narrow reds on legitimate content. Floor boundaries are covered at 149/150/151. The round-2 reviewer's sharpest point was about that claim, and it was right: the first matrix's pairing was "true of the properties chosen, not of the predicate's actual surface" - not one of its twenty properties could see the dialect adjacency hole, a non-adjacent runtime word, pin shadowing, or an over-broad pin colliding with a new line. Every one of those is now a committed regression using the reviewer's own attack line verbatim, and the local fixture harness went from 14 cases to 35 (PASS=35 FAIL=0). That harness earned a finding of its own. Its first run reported PASS=2 FAIL=12 with BOTH passes VACUOUS: `git add` has no -q flag on this build, so nothing staged, every fixture hit the empty-walk error path, and the two checks that assert an ABSENCE passed off that error path rather than off real guard logic. A staging failure is now fatal and every absence-asserting check first proves the walk ran and the expected violation was flagged. Later, one case failed because its fixture supplied only one of a pinned file's two approved lines, so the stale-pin check fired correctly - the expectation was wrong, not the guard. Telling those two apart is the whole value of running a matrix rather than reasoning about one. On the two orthogonal reviews: the isolated adversarial pass executed a great deal of code, across two rounds, against its own fixture repos. The security pass did NOT - it self-discloses that it verified by reading only, because node --test is hard-blocked here. Saying so plainly, because "two orthogonal reviews" without that caveat overstates what the second one established. It also raised, and I cleared by measurement, a concern that importing escapeRegex from a gitignored build artifact would break lint:ci on an unbuilt clone: six other tracked scripts already require that exact path, three of them already in lint:ci, and .github/workflows/test.yml:192-193 runs `npm run build:lib` immediately before it for exactly this reason. Part A has no new test deliberately - those edits are covered by the guard itself inside lint:ci, and a separate per-locale assertion would duplicate it and then drift from it. The one exception is the root README case, which IS pinned: that residue was invisible to the guard rather than merely unasserted, so the fix is a scan-set change and needs its own regression test. No mode-bit read-failure fixture was added on purpose: the benches run as root, where chmod-based IO injection is vacuous, so such a test would assert nothing. The test's fixture helpers write throwaway docs/ paths, which trips lint-docs-guard-registration's reader-name heuristic. Resolved the way that lint documents - a header `// docs-guard-exempt:` marker plus a baseline entry - because the fixtures only WRITE scratch data and never read shipped docs; the baseline was re-confirmed, not merely extended, each time locale and adversarial fixtures were added. scripts/lib/macos-conformance-tier.generated.cjs regenerated through its own --write path, since a new test file changes the count lint:generated-sync reads. Fixes #4729 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4729): backfill changeset PR number (#4753) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-14 20:50:26 -04:00
Tom Boucher	9b750dc00a	fix(#4505): resolve models through the active runtime and the tier table (#4726) ... * test(#4505): cover runtime-aware overrides and routing precedence Failing-first for both halves of the issue, plus the precedence layers a naive fix silently defeats. Every row drives the REAL CLI in a subprocess. That is load-bearing: the defect is WHICH function the shipped call sites reach, so a row calling the resolver in-process would pass while every real spawn stayed broken. It also makes GSD_RUNTIME hermetic -- it is ambient, and an in-process row would leak it into its neighbours. Two fixture mechanics are documented in the helper because each silently invalidates a row when got wrong, and both were found by measuring rather than by reading the loader: - the loader reads `process.env.GSD_HOME || os.homedir()`, so redirecting only HOME leaves a developer's real ~/.gsd/defaults.json in play; - the mere EXISTENCE of a .planning/ directory disables the shared-defaults layer, so a fixture that creates one stops exercising the "poisoned global" path #2297 acceptance #4 is about. Measured: .planning/ with config -> gpt-5.6-terra; .planning/ present but empty -> gpt-5.6-terra; no .planning/ at all -> "". Rows cover: both reported repros; the init payload a real spawn reads; the omit gate, the runtime tier map and model_overrides each outranking the tier table; model/tier coherence under dynamic routing; resolve-execution with the attempt absent; max_escalations at limit-1/limit/limit+1 plus a cap of 0; and four fail-safe rows pinning that only a value canonicalizing to a recognised non-Claude runtime may outrank an omit. Registers the docs-guard exemption path: the rows quote the documented first-spawn contract in comments. The file still never READS a docs/ path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4505): resolve models through the active runtime and the tier table Consolidates #4495 and #4493. One gap: the function every agent spawn goes through consulted neither mechanism that was supposed to make resolution runtime- and tier-aware. Half A -- the runtime was READ from config, not resolved. resolveActiveRuntime (GSD_RUNTIME -> config.runtime -> per-install marker -> claude) existed and worked, but was called exactly once in the file. Every other site read config['runtime'] raw, and that key is normally absent, so model_profile_overrides.<runtime>.<tier> was inert for any install that identifies its runtime through the environment or the marker. Same override both times, differing only in WHERE the runtime is declared: GSD_RUNTIME=opencode, no runtime key -> sonnet (ignored) runtime:"opencode" in the config -> TEST-OPENCODE (works) Half B -- nothing consulted dynamic_routing on the first spawn, though docs/features/dynamic-routing-with-failure-tier-escalation.md documents "the resolver picks tier_models[default_tier] for the FIRST spawn". The tier-table lookup is extracted into ONE helper both entry points call, so the first-spawn value and the escalated value cannot drift; resolveModelInternal calls it at attempt 0 and resolveModelForTier at the real attempt. Placement is the documented composition, not a convenience. The same doc says "model_overrides always wins; dynamic_routing.tier_models[<tier>] resolves above models.<phase_type> and model_profile" -- so the step sits BELOW model_overrides, the model_policy preset, the runtime tier map, the resolve_model_ids:"omit" gate and the claude tier override, and ABOVE the profile lookup. An earlier cut routed every call site through resolveModelForTier instead, which returns the tier model directly and therefore skipped three of those layers: with an omit and a non-Claude runtime it handed out a model id where the gate had returned "". Criterion 1 is applied in full, including the two value-policy reads #4192 had recorded as "NOT via resolveActiveRuntime". The tests decided it: switching them breaks nothing, so that reading was never enforced -- and the old behaviour defeated #4192's own principle that an explicit pin must not be silently unpinned (claude-opus-4-8 under GSD_RUNTIME=opencode collapsed to the Claude-only alias opus). #4192's comment is updated in place rather than left stale. The step-3 opt-in signal is CANONICALIZED. Comparing the raw config field against the literal 'claude' made runtime:"Claude", "claude-code" and even 5 count as non-Claude opt-ins and outrank an explicit omit -- failing OPEN in exactly the #2297 case the guard exists to protect. null now covers both "not a string" and "not a runtime we recognise", and both read as NOT an opt-in. Verified cell by cell against a pristine origin/next worktree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4505): backfill changeset PR number (#4726) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>	2026-09-14 13:40:17 -04:00
Tom Boucher	4d65c248e5	fix(#4641): make test-conformance the sole Windows selector and narrow the tier to 28.5% (#4643) ... * test(#4641): failing-first tests for the tier ceiling and a single Windows selector Tests only, committed ahead of the implementation so the RED run is real. - tests/platform-conformance-tier.test.cjs: tier-size ceiling asserted as a ratio against a live denominator (Windows 33%, macOS 25%); per-helper negative cases proving seam calls and path-call-plus-slash-literal are not platform signals; positive pins that genuine platform content, seam-bypassing spawns, chmod and symlink still classify in; macOS signal set and generated list unchanged. - tests/ci-full-lane-sharding.test.cjs: the test job has zero windows-latest rows and test-conformance still has 3 windows + 1 macOS. - tests/ci-test-scope.test.cjs: windows_tests is absent rather than empty, a non-tier test file no longer forces full_matrix, a RULE-pulled windows-hint test does, and resolveSelection rejects the retired windows scope. Refs #4589, #4591, #4592, #4593, #4603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): delete the second Windows selector and narrow the conformance tier Epic #4589's goal — the OS-agnostic bulk on Linux, a small explicitly-scoped conformance tier on real Windows/macOS — was not met. Measured on PR #4640 (run 34618834118): 7 non-Linux jobs, a 546/930 (58.7%) "tier", and 5 of 7 changed test files running on a real Windows runner twice. Two selectors, only one in the epic's scope. The test job's three scope:windows shards predate the epic (#494, sharded #3057) and gate on product_changed, not full_matrix, so they fire on every product PR whatever Phase 3's classifier decides. They are deleted; test-conformance becomes the sole Windows selector, as it already was for macOS. Non-Linux jobs 7 -> 4. Gating the lane instead was rejected as provably redundant: for a test file reachesConformanceTierOrSeam is literally CONFORMANCE_TIER_FILES.includes(file), and that same predicate sets full_matrix, which turns test-conformance on. Every file a gated lane would run is already covered in the same run. The lane's one non-redundant residue -- RULE-pulled tests matched by the isWindowsHint filename heuristic -- is ported into reachesConformanceTierOrSeam so it sets full_matrix instead of feeding a parallel lane. Two detectors matched the repo's own test idiom rather than any platform signal and carried 226 of the tier's sole-signal membership against 41 for the other eight: process-seam-subprocess (335 files, 118 unique) matches the tests/helpers.cjs entry points nearly every CLI test uses, and going through the seam is the opposite of a platform signal since shell-command-projection takes platform as an injected parameter; hardcoded-path-vs-path-call (328, 108) needs only a path call anywhere plus a slash literal anywhere, and that class is already enforced by ADR-1703's Linux-runnable ESLint rules. Both are removed. Tier 546 -> 254 (27.3%). src/ reachability is unchanged at 28 files, measured. Adds the size gate Phase 2 never had, as a ratio against a live denominator so it cannot stop binding as the suite grows. 292 files leave real-OS Windows execution. The drop-out set was audited: 14 have a platform-suggestive filename and all 14 are static source-text analyses or seam-mediated CLI tests. raw-child-process was investigated as a suspected false negative and left unchanged -- relaxing it adds 13 files, all false positives. macOS is untouched: MACOS_CATEGORIES is a separate array and the regenerated macos-conformance-tier.generated.cjs is byte-identical at 196 files. Fixes #4641 Refs #4589, #4591, #4592, #4593, #4603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): register the new ADR path in the docs-guard exempt baseline tests/ci-test-scope.test.cjs references docs/adr/4641-windows-selector-consolidation.md in a comment justifying the retired windows scope; lint-docs-guard-registration tracks that reference set, so the baseline needs the new path. Verified the exemption still holds: the path is prose, not a filesystem read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): make the escalation tier-backed and drop every hardcoded count Three follow-ups from measuring the first pass rather than trusting it. The windows-hint escalation now requires tier membership as well as the filename hint. Setting full_matrix runs test-conformance, which runs only the tier; escalating on a test that is NOT in the tier costs four jobs and still never runs that test on Windows. Measured over the 16 RULES entries the narrowed predicate fires on exactly the same rules today, so this is correct-by-construction rather than a behavior change. The broader variant -- escalate on any tier member a rule pulls in, ignoring the hint -- was measured at 14/16 rules and rejected as over-broad. Removes the hardcoded counts. A hardcoded macOS tier length of 196 broke as soon as the rebase pulled in one new test file from #4253, which is the whole argument against them: the ceilings are ratios against a live denominator, the committed lists are pinned by comparison against a fresh classification of the live tree, and the three named probe files now assert on their SIGNAL rather than on membership in a literal list -- asserting by filename is the exact error this PR fixes in the classifier. Regenerates both lists against the rebased tree. Same-tree figures are now 547 -> 255 of 931 eligible (58.8% -> 27.4%), 292 entries removed and none added; macOS is unchanged at 197 with a zero-line diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): restore real-shell-spawn coverage and repair assertions the narrowing broke An isolated adversarial review found a real false negative. Removing the blanket process-seam-subprocess detector also removed the only coverage for tests that spawn a REAL shell: tests/helpers/process-seam.cjs's runHook spawns options.interpreter via real spawnSync, so runHook('-c', [script], { interpreter: 'bash' }) runs a real bash binary executing a shell script extracted from workflow markdown. The seam argument holds for src/shell-command-projection.cts, which takes platform as an injected parameter; it does NOT hold for the test helpers, which spawn real binaries. Conflating the two is what made the blanket detector look purely noisy -- it was 99% noise wrapping a real signal. Adds a narrow shell-interpreter-spawn category keyed on a real interpreter option. Measured 2026-09-11: 33 files match, 9 were outside the tier and are added back, taking it 255 -> 264 of 931 (27.4% -> 28.4%), still under the 33% ceiling. All 9 confirmed by reading the matching source line, zero comment or fixture matches. runGit-alone and non-node-spawnSeam alternatives were measured and rejected -- each adds 9 files but misses the counterexample entirely. Fixes a real bug the suite caught: jobs.test is ubuntu-only now that its scope:windows rows are gone, so it must wire GSD_STRICT_LIVE_CONFIG_GUARD strictly rather than carrying the Windows report-only carve-out. The carve-out now lives solely on test-conformance, whose matrix does include windows. Repairs seven pre-existing assertions the category removal invalidated, preserving each case's purpose rather than deleting coverage, and converts the last hardcoded tier bounds to live-derived ratios -- including the macOS sanity range that was still a magic [100, 350]. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): keep the confinement test on a real OS via a documented allowlist A security review found tests/external-descriptor-confinement.test.cjs had dropped out of the Windows tier. It must stay in, and no content signal can express why: it exercises isPathConfined (src/external-descriptor-trust.cts), which uses the AMBIENT path module -- path.resolve(root, target) and path.sep -- with no injection. Its win32 semantics (drive letters, UNC, separator) are only reachable by actually running on Windows, and it is a security-relevant write-confinement gate. A content classifier cannot see 'this module reads the ambient path module', so no regex belongs here. Adds ALWAYS_REAL_OS, a Map of path -> recorded reason, unioned into the Windows tier only. A Map rather than a list so an entry without a reason is impossible by construction, and tests assert every entry names a file that exists on disk so a stale entry fails loudly instead of rotting. This is the centrally- enumerated single source of truth epic #4589 Phase 2 asked for and ADR-1703's portability-vocab.cjs already models -- deliberately not a heuristic. Windows tier 264 -> 265 of 931 (28.5%), still under the 33% ceiling. macOS is untouched and byte-identical: the win32 concern does not apply to a POSIX runner, and a test asserts the allowlist does not leak into that tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): inject the path impl into isPathConfined and correct the ADR count Two review findings, both fixed rather than dispositioned. A security review found tests/external-descriptor-confinement.test.cjs had left real-OS execution. The allowlist pinned it back, but that only restored INCIDENTAL coverage: isPathConfined used the ambient path module, and its test carried POSIX-only literals, so a win32 confinement escape was unverified on every platform including Windows. isPathConfined now takes an optional third parameter carrying the path implementation, defaulting to the ambient module. Blast radius is CRITICAL -- 53 affected symbols across 19 files -- so the change is purely additive and every existing two-argument caller is byte-identical. Tests now inject path.win32 and path.posix, covering a different drive letter, a cross-drive absolute, backslash and forward-slash traversal, UNC, and the startsWith prefix-boundary bug (.gsdEVIL against root .gsd) on both separators. Proved load-bearing: dropping the + p.sep from the prefix check fails exactly the two boundary cases and nothing else. Callers' suites 149/149. The spec review caught an off-by-one: the ADR narrated a 264-file tier while the committed list holds 265. The ADR now records the full chain 547 -> 255 -> 264 -> 265 (28.5%). Also corrects a stale comment in scripts/docs-guard-registry.cjs that narrated classify() as zeroing windows_tests, a key this change removes -- kept as historical narration but labelled as such. Refs #4641 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#131): make the unwritable-HOME test actually test something Found by sweeping for the root-bypass class after fixing commit-files-deletion. This one is the silent variant, and it was broken twice over. First, the condition: the test made a fake HOME unwritable with chmod 0o500. The gsd-test Docker bench runs as root, root bypasses mode bits, so HOME stayed writable and the hostile condition never existed. Replaced with a HOME whose PARENT is a regular file, so every write under it fails ENOTDIR at the VFS layer for every uid -- no permission check is involved at all. Second, and more fundamental: the probe was npm --version, which on npm 11.19.0 performs zero filesystem I/O against HOME. Proven rather than assumed -- neutralizing runNpm()'s isolation turned the sibling test red while this one stayed green, so its assertion could never detect the regression it guards, on any uid, with or without the condition fix. npm config get cache was tried next and proved vacuous the same way (it only string-resolves the path). The probe is now npm cache verify, which really does mkdir _cacache under HOME. Re-proved load-bearing after the change: with isolation neutralized the test now fails with ENOTDIR on <blocker>/home/.npm/_cacache. tests/helpers.cjs was restored and verified diff-clean; suite 13/13. Refs #4641 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): correct the net drop-out figure in ADR-4641 The Consequences section still said 292 files leave real-OS Windows execution. That was the count before the narrow shell-interpreter-spawn replacement restored 9 and ALWAYS_REAL_OS pinned 1. Net is 282. Also names both real-binary categories rather than only raw-child-process, and clarifies that the 14-file filename audit was against the 292 initially dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record the rejected concentration ceiling and its measurement Applying Goodhart's own question to the new ceiling -- how would you make this metric look good without improving what it represents -- surfaces a real weakness: a ratio can be satisfied by inflating the denominator, so adding OS-agnostic tests loosens it without narrowing the tier. The obvious companion gate was a sole-signal concentration ceiling, since the original defect was one detector carrying half the tier. Measured and rejected: peak concentration post-fix is raw-child-process at 53/265 = 20.0%, against the historic offenders at 21.6% and 19.8%. Any threshold above 20% misses the original defect; any threshold below it fails on a legitimate category. The discriminator is whether a signal is platform-meaningful, which no threshold encodes. Weakness disclosed rather than covered by a gate that does not bind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4641): add the changeset fragment for the confinement-check change changeset-lint failed on PR #4643: the PR touches user-facing paths and carried no fragment. The earlier no-changeset call matched #4604's CI-only precedent and was correct then; it was not revisited once the PR grew a src/ change, which is my miss. The fragment describes the real user-visible improvement: the external-descriptor write-confinement check's Windows semantics are now verified deterministically rather than only when the suite happened to run on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): correct the tier count in TESTING-SUITES.md Said the tier narrowed from 546 to 254. The final committed list is 265 of 931 eligible (58.8% -> 28.5%) after the shell-interpreter-spawn replacement restored 9 files and ALWAYS_REAL_OS pinned 1. Same error class the spec review caught in the ADR, in a live reference page rather than a dated record, so it states the current truth rather than carrying an amendment note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record the measured aggregate from real CI job lists Epic #4589's closeout asserted its reduction from a static count; #4641's acceptance criterion asks for a figure read off a real run. Recorded here: test.yml job count 21 -> 15 and non-Linux 7 -> 4, comparing PR #4640's run against this PR's own. Against the true pre-epic baseline of 9, that is 9 -> 4. Also states the caveat that a PR's total CHECK count is not a clean before/after comparison, since many gates are path-scoped and this change touches a broader path set -- the like-for-like figure is the test.yml job count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): compare job totals the same way on both sides The measured-aggregate table put #4640's COMPLETED run total (21) against this run's count at matrix-expansion time (15). Those are not the same measurement: the completed total includes the post-test Coverage gate and baseline-publisher jobs. Counted identically, it is 21 -> 17. The load-bearing figure, non-Linux jobs 7 -> 4, was correct and is unchanged. Called out in the table rather than silently corrected -- comparing two differently-derived numbers is exactly the error class this ADR is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record measured conformance wall-clock and date the stale counterfactual Adds the per-job durations from both runs. The honest read is that this is a correctness win more than a speed one: file count fell 52% but wall-clock only 9-29%, because what was removed were the cheap static tests and what remains is concentrated in expensive spawn-heavy work. Stated explicitly so nobody expects a future narrowing to buy time proportional to file count. The load-bearing figure is windows shard 3/3: 40m24s against a 45-minute cap on the 547-file tier -- 90% of the cliff #869 and #3057 were both filed about -- pulled back to 31m27s. macOS moved the wrong way (17m48s -> 21m02s) while its tier was UNCHANGED at 197 files, which fixes that as runner variance and is noted as a caution against reading a single duration as signal. Also dates the symlink-keyword counterfactual, which cited a 254-file tier from before the replacement category and allowlist took it to its final 265. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): re-measure against the rebased tree and disclose the allowlist's zero next gained #4644 mid-flight, so every absolute count shifted. Re-measured on the tree this actually ships against (932 eligible): 548 -> 257 by detector removal, 257 -> 266 once shell-interpreter-spawn restores 9. Net 282 removed, 9 restored. macOS 198, unchanged by this PR. The percentages did not move across three rebases (58.8% -> 28.5%), which is the whole argument for expressing the ceilings as ratios rather than counts -- noted in the ADR since it is now evidence rather than assertion. Also discloses that ALWAYS_REAL_OS now contributes ZERO files: this PR's own win32 test cases introduced the literal win32 into the pinned file, so it classifies in on content via win32-darwin-literal. The entry stays and the reason is written down, because the file's real-OS need is a property of the code under test (isPathConfined reads the ambient path module), not of the test's text -- the text that currently saves it is incidental and could be refactored away silently. 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-09-11 17:00:11 -04:00
Dennis Alexis Valin Dittrich	18c899def5	enhance(#4209): optional external source reviewer lanes for /gsd:code-review (#4323) ... * test(01-01): define reviewer-support trait contract Add failing coverage for step.supportsReviewerLanes (#4209 DISP-02): validator rejects non-boolean values with an exact field path, accepts missing/true/false, and the real code-review capability.json steps must declare supportsReviewerLanes: true. Add loop-resolver projection coverage proving the trait reaches activeHooks verbatim for a provider-neutral synthetic step (not code-review-specific), and that omitted/false values stay inert (no key on the active hook). All 8 new assertions fail today: the validator has no such field, and loop-resolver has nothing to project. RED before GREEN. * feat(01-01): declare reviewer-capable steps Add step.supportsReviewerLanes (#4209 DISP-02): a strict optional boolean opt-in trait, step-scoped (not capability-wide). Only a literal true validates and projects; false/omitted stay inert (no key on the projected active hook), and every non-boolean type fails capability-validator.cjs with an exact field-path error. Opt both existing code-review steps (execute:post, execute:wave:post) into the trait in capabilities/code-review/capability.json. Project the validated field through src/loop-resolver.cts into activeHooks so a provider-neutral generic interpreter can read it without any code-review-specific knowledge. Document the field in docs/reference/capability-manifest.md and regenerate gsd-core/bin/lib/capability-registry.cjs via the generator (never hand-edited). Makes all 8 RED assertions from the prior commit pass. * test(01-02): define shared reviewer dispatch - Add tests/reviewer-step-dispatch.test.cjs covering dispatchReviewerLanes: inert when the supportsReviewerLanes trait is off or nothing is selected, exactly-once plan/invoke per selected lane, duplicate-alias dedup, the bounded metadata-only source-review prompt (repo root, paths+baseSha, depth, four fixed prohibitions), and capability-neutral reuse via a second synthetic step context. - RED: module under test (src/reviewer-step-dispatch.cts) does not exist yet, so require() fails and every assertion is unreached. * feat(01-02): dispatch reviewers for opted-in steps - Add src/reviewer-step-dispatch.cts: dispatchReviewerLanes(input, deps), ONE interpreter for a step's supportsReviewerLanes trait. Reuses resolveReviewerSelection for selection and resolveLanePlan for planning (both already-existing, pure building blocks); invocation is the one required, caller-injected seam (deps.invoke) since runLane needs OS-aware spawn plumbing this module does not own. - trait !== true, or a selection resolving to zero lanes, dispatches nothing (zero plan/invoke calls). Each selected lane is planned and invoked exactly once, in the selector's deduped/sorted order. - buildSourceReviewPrompt assembles a metadata-only bounded prompt (repo root, canonical paths + base SHA, depth, four fixed prohibitions) — never file contents — written once per dispatch and shared across every invoked lane. - GREEN: tests/reviewer-step-dispatch.test.cjs now passes. * test(01-02): define reviewer dispatch failures - Extend tests/reviewer-step-dispatch.test.cjs with the fail-closed matrix: an explicitly requested lane the selector could not resolve still lets the OTHER resolved lane run, but the aggregate result must never read as a clean success (and 'every explicit lane unavailable' must be distinguishable from the plain no-flags-passed inert case); request-level validation (path traversal, absolute paths outside repoRoot, empty/non-string paths, missing depth/base SHA) halts the whole dispatch before any lane is planned or invoked; a per-lane prompt-budget overflow hard-fails only that lane before invoke while its sibling still runs. - RED: src/reviewer-step-dispatch.cts does not yet implement any of these guards, so 9 of the new assertions fail against the current (Task 1) implementation. * fix(01-02): fail closed in reviewer dispatch - src/reviewer-step-dispatch.cts: add the fail-closed guards the prior commit deliberately left out. An explicitly requested lane the selector could not resolve no longer lets the aggregate read as a clean success — lanes that DID resolve still run and keep their results (never narrow the requested set), but selection.errors now flips the aggregate ok to false, and 'every explicit lane unavailable' is now distinguishable (SELECTION_FAILED) from the plain no-flags-passed inert case (NO_LANES_SELECTED). - Add request-level validation (validatePaths, depth/baseSha presence) that halts the WHOLE dispatch before any lane is planned or invoked: path traversal, absolute paths outside repoRoot, empty/non-string paths, and missing provenance are all rejected up front. - Add per-lane prompt-budget enforcement (resolveBudget, mirroring gsd-tools.cjs's budgetFor convention including budget 0 = unbounded): a lane whose resolved budget the prompt exceeds hard-fails before invoke runs for it, without cancelling a sibling lane already planned. - Document the supportsReviewerLanes trait and its dispatch-step interpreter in gsd-core/references/loop-hook-dispatch.md. - GREEN: all 19 tests in tests/reviewer-step-dispatch.test.cjs pass; no regressions in the review-lane/reviewer-selection/prompt-budget suites (356 passing). * test(01-03): define optional source reviewer flow RED: assert code-review.md dispatches roster-derived reviewer-lane flags through a single review-lane dispatch-step call (DISP-01..05), that the no-flag path stays byte-for-behavior unchanged (COMP-01), and that external evidence reaching the internal reviewer prompt is marked unverified (CONS-02). Also covers the CLI contract directly: no-op with no explicit selection, and fail-closed on an explicit unknown lane (SAFE-07) via real gsd-tools.cjs subprocess calls. * feat(01-03): route optional source reviewers GREEN: code-review.md gains a dispatch_reviewer_lanes step that matches canonical reviewer-lane flags against the merged first-party + installed roster (never a hand-maintained list) and, only when at least one is present, calls the shared reviewer-step interpreter exactly once with the already-resolved repo root, file scope, depth, and base SHA. Its evidence paths are appended to the internal reviewer prompt via ${EXTERNAL_EVIDENCE_BLOCK}, explicitly marked unverified. No reviewer-lane flag leaves the internal-only dispatch byte-for-behavior unchanged (COMP-01). Deviation (Rule 3 — blocking issue): 01-02 documented `review-lane dispatch-step` (gsd-core/references/loop-hook-dispatch.md) as the CLI route `dispatchReviewerLanes` wires through, but never implemented the gsd-tools.cjs subcommand — the workflow's call had nothing to reach. Add it to the existing review-lane router, reusing the same effort-aware plan building and runner deps `plan`/`invoke` already use (factored into buildLaneRunnerDeps to avoid duplicating the spawn/http/fs seam). Guard the CLI's own `detected` set on whether an explicit flag was passed: resolveReviewerSelection's no-explicit-selection fallback is "select every detected reviewer" (the correct default for /gsd:review), and passing it an unconditionally non-empty detected set would silently invoke the whole roster on every no-flag code review, violating COMP-01. * test(01-03): define external finding consolidation RED: assert gsd-code-reviewer.md treats <external_reviewer_evidence> as untrusted input — independently re-verifies every claim against the actual current source, resists a prompt-injection attempt embedded in evidence text, and folds a verified claim into the existing Narrative Findings section with no second REVIEW.md schema (CONS-01..03). Also assert code-review.md's EXTERNAL_EVIDENCE_BLOCK restates the four fixed source-review prohibitions (SAFE-03..06) at the internal-reviewer handoff. * feat(01-03): consolidate external review evidence GREEN: gsd-code-reviewer.md's load_context parses <external_reviewer_evidence> as untrusted data, independently re-verifies every cited claim against the actual current source before it can appear in REVIEW.md, and explicitly resists prompt injection embedded in evidence text (never a command, no matter what it claims to be). A verified claim folds into the existing Narrative Findings section with (external: {slug}) provenance — one REVIEW.md schema only, no separate external-findings section. code-review.md's EXTERNAL_EVIDENCE_BLOCK now restates the four fixed source-review prohibitions (SAFE-03..06) at the internal-reviewer handoff. * fix(01-02): gitignore the reviewer-step-dispatch build artifact 01-02 added src/reviewer-step-dispatch.cts but never added its npm run build:lib output to .gitignore, unlike every sibling gsd-core/bin/lib/*.cjs generated file. Left it showing as untracked noise in git status. * docs(01-04): publish user and command contract for reviewer-lane source review - Document optional reviewer-lane flags on /gsd-code-review in USER-GUIDE.md and COMMANDS.md: opt-in, no source bodies in prompts, no fallback on failure, findings independently consolidated into the single REVIEW.md - Add the same contract to the docs/features/code-review-pipeline.md fragment and regenerate docs/FEATURES.md from it - Preserve /gsd-review as the plan-review command; cross-reference it rather than duplicating the reviewer roster - Pick up docs/INVENTORY-MANIFEST.json and skills/gsd-code-review/SKILL.md drift owned by source already shipped in Plans 01-01/01-03 but never regenerated (npm run regen:derived had not been run in this worktree) * docs(01-04): align architecture and agent ownership docs for reviewer-lane trait - ARCHITECTURE.md: trace the #4209 capability trait (supportsReviewerLanes) through the shared dispatchReviewerLanes interpreter to the existing review-lane plan/invoke machinery, ending at gsd-code-reviewer as the sole REVIEW.md consolidator - AGENTS.md: document gsd-code-reviewer's full-context verification scope and its treatment of external reviewer evidence as unverified input - No new diagram, abstraction, or config key; docs/CONFIGURATION.md is unchanged since the feature adds no setting or default * fix(01-02): eslint-ignore the reviewer-step-dispatch build artifact Same gap as the earlier .gitignore fix: 01-02 added src/reviewer-step-dispatch.cts but never added its generated gsd-core/bin/lib/reviewer-step-dispatch.cjs output to eslint.config.mjs's ignore list like every sibling generated file, so tsc's emitted __importDefault CommonJS-interop var tripped no-var. * fix(01-04): add the reviewer-step-dispatch.cjs roster row to docs/INVENTORY.md 01-04 regenerated docs/INVENTORY-MANIFEST.json (which now lists cli_modules/reviewer-step-dispatch.cjs) but the hand-written roster row in docs/INVENTORY.md — required by design, since a role sentence cannot be generated — was never added. * fix(01-01): update the code-review capability-step fixture for supportsReviewerLanes refactor-trigger-cli.test.cjs's preservesCodeReviewHookShapeAlongsideRefactorHook strict-deep-equals the code-review step's exact shape at execute:post; 01-01 added supportsReviewerLanes: true to that step and this fixture was not updated. * chore(01-03): acknowledge emitted-doc growth for code-review.md and gsd-code-reviewer.md Both files grew as a direct, intended consequence of wiring optional reviewer lanes into /gsd:code-review (the new dispatch_reviewer_lanes step and the untrusted-evidence consolidation contract) — not incidental drift. Emitted-Drift-Ack-Growth: code-review.md — new dispatch_reviewer_lanes step and EXTERNAL_EVIDENCE_BLOCK wiring for optional reviewer lanes (#4209) Emitted-Drift-Ack-Growth: gsd-code-reviewer.md — untrusted external-evidence consolidation contract for optional reviewer lanes (#4209) * test(01-05): define WR-01/WR-02 reliability contract for dispatchReviewerLanes From internal code review: dispatched must be false when zero lanes actually reached plan(), and a throwing plan()/invoke() for one lane must not discard results already collected for a sibling lane — matching the fail-closed pattern gsd-tools.cjs already uses for the same resolveLanePlan call (#2494/#2605/#1698/#1936/#2073/#2176/#2589/#2794). Refs: gsd-core-dks.16, gsd-core-dks.17 * fix(01-05): close WR-01/WR-02/IN-01/IN-02 from internal review - WR-01: dispatched now tracks whether any lane actually reached plan(), not results.length — an unresolvable selected slug no longer reports dispatched:true. - WR-02: plan()/writePromptFile()/invoke() wrapped per-lane so a throw for one lane can never discard results already collected for a sibling lane, matching the same guard gsd-tools.cjs already has around the identical resolveLanePlan call. - IN-01: documents the intentional budget===0-is-unbounded convention (#2797) the caller already relies on. - IN-02: review-lane dispatch-step no longer blocks indefinitely on an un-piped interactive TTY; fails closed to empty paths instead. Refs: gsd-core-dks.16, gsd-core-dks.17 * docs(01-05): add changeset fragment for PR #17 * fix(01-03): allowlist prompt-injection-scan false positive on the untrusted-evidence contract agents/gsd-code-reviewer.md's untrusted-evidence section and its pinning regression test both quote injection phrases as the exact attack they defend against/detect — same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class as the existing allowlist entries, not an actual injection vector. * test(01-05): extend WR-02 coverage to writePromptFile/invoke throws; DIFF_BASE-empty skip From CodeRabbit review: WR-02's earlier fix only wrapped plan() — writePromptFile()/deps.invoke() still ran unguarded, so a throw there still aborted every later selected lane. Also covers the dispatch_reviewer_lanes DIFF_BASE-empty-provenance gap (explicit lanes silently not running when no prior review and no phase-start commit exist). * fix(01-05): skip dispatch_reviewer_lanes with a clear warning when DIFF_BASE cannot be resolved Previously an explicit reviewer-lane request with no prior review and no resolvable phase-start commit reached dispatch-step with an empty --base-sha, which fails closed via missing_provenance — correct, but silent about why explicitly requested lanes didn't run. Now skip dispatch entirely in that case with a stderr warning naming the actual cause. * fix(01-05): wrap writePromptFile/invoke in the same per-lane try/catch as plan() WR-02's original fix only guarded plan() — a throw from writePromptFile() or deps.invoke() still aborted the whole dispatch, discarding results already collected for lanes processed earlier in the loop. CodeRabbit caught the gap; WR-02b/WR-02c pin it. * fix(01-05): WR-02b mock must throw only on the first writePromptFile() call The committed mock threw unconditionally, so codex's retry also threw and failed for the same reason as claude's — the test could not distinguish 'sibling still runs' from 'sibling also breaks'. Gate the throw to the first call, matching WR-02/WR-02c's single-failure intent. * fix(#4209): close review findings from adversarial + critical-code-reviewer pass Two independent reviews (agy adversarial review, Opus critical-code-reviewer + ponytail) found 6 Blocking and 7 Required issues in the reviewer-lane dispatch wiring around dispatchReviewerLanes. All 13 tracked in gsd-core-dks.18-30 and fixed here: - dispatch-step's reducer silently swallowed whole-dispatch rejections (invalid paths, missing provenance, etc); it now checks parsed.ok/reason. - spawn_reviewer recomputed its own stale DIFF_BASE, diverging from the LAST_REVIEW_COMMIT-aware value dispatch_reviewer_lanes uses on re-review; now shares the single compute_file_scope derivation. - the external reviewer prompt had no actual review request or citation requirement, only prohibitions; added both. - removed the supportsReviewerLanes trait plumbing (capability registry, validator, loop-resolver, docs, tests) — it was never consulted by the real dispatch path, which gates on explicit CLI flags instead. - flag-resolution require() was a fragile cwd-relative literal that failed silently on non-vendored installs; now resolves via GSD_TOOLS's own directory and warns instead of swallowing failure. - reducer didn't unwrap the @file: overflow protocol for large payloads. - deduplicated resolveBudget/budgetFor into one resolveLaneBudget. - lane artifacts now write to a mktemp run dir instead of $PHASE_DIR, so a second dispatch can't overwrite prior evidence. - validatePaths rejects control characters, closing a markdown-injection vector into the external prompt via crafted filenames. - reworded the one line that tripped prompt-injection-scan.sh instead of allowlisting the whole production prompt file. - fixed a stale docstring range and a dispatched-field ordering bug. - added 3 integration tests executing the actual reducer against synthetic dispatch-step JSON, replacing markdown-substring-only assertions. 771/771 tests pass across every touched suite; tsc --noEmit clean. * fix(#4209): wire supportsReviewerLanes as the maintainer's required reusable trait The maintainer's approval on issue #4209 explicitly redirected implementation shape: reviewer-lane dispatch must be a reusable capability/step-dispatch trait ("supportsReviewerLanes"), not code-review.md hand-wiring the call itself. My previous commit (e2558326) deleted that trait entirely after finding it declared-but-never-consulted, which was backwards — the fix was to wire it, not remove it. Restores the trait (capability.json, generated registry, validator, loop-resolver.cts, docs, tests) and wires it for real: dispatch_reviewer_lanes now resolves its own active hook via `gsd_run loop render-hooks` for the configured workflow.code_review_point and only proceeds to CLI-flag matching when supportsReviewerLanes reads true. Explicit flags no longer bypass the trait; a matching flag with the trait false resolves zero slugs (proven by a new integration test executing the real fence with both trait states). Emitted-Drift-Ack-Growth: gsd-core/workflows/code-review.md — the dispatch_reviewer_lanes step grows a trait-resolution fence (#4209 maintainer redirect requires the capability layer, not the workflow, own the opt-in decision). * fix(#4209): dispatch-step self-verifies the reviewer-lane trait via --cap-id/--point Both an agy adversarial review and an Opus critical-code-reviewer pass independently found the same gap in my previous commit (9b2c3773d): the trait check I wired into code-review.md only protected code-review's OWN invocation — gsd-tools.cjs's dispatch-step handler still hardcoded `trait: true` unconditionally, so a second capability declaring supportsReviewerLanes would get zero enforcement from the shared CLI unless it correctly re-implemented the ~15-line render-hooks scrape itself. That is exactly the "each workflow.md hand-wiring the call" the maintainer's redirect said to eliminate. Moves the trait check into dispatch-step itself: given --cap-id/--point, it self-invokes `loop render-hooks <point>` (relocating the one subprocess code-review.md used to spawn for this, not adding a new one) and derives the real trait from that capId's active hook, rather than trusting a caller-passed boolean. code-review.md now only passes --cap-id code-review --point "$CODE_REVIEW_POINT" and no longer resolves or gates on the trait itself — the ~20-line scrape it previously carried is gone. Any other capability opts into the identical enforcement by declaring the trait and passing the same two flags. Replaced the two tests that stipulated SUPPORTS_REVIEWER_LANES as an input variable (they proved a bash branch honors a variable, not that the variable reflects the real capability manifest) with three integration tests that invoke the real dispatch-step CLI against the real first-party capability registry: the real code-review trait resolves true, an unknown --cap-id resolves false (trait_not_enabled, fail-closed), and omitting --cap-id/--point entirely resolves false (no context means no opt-in). Also: reject \x7f/U+2028/U+2029 in validatePaths' control-character check (agy-F1 was incomplete), and delete the promptWritten per-lane coupling flag — the prompt write is idempotent, so writing it once per lane instead of gating on "did any lane write it yet" removes a latent bug where a deps.plan override that ever varies promptPath per lane would silently skip writing for a later lane. Emitted-Drift-Ack-Growth: gsd-core/workflows/code-review.md — net line count drops (the trait scrape moved into dispatch-step), but the file still grew this session across multiple commits; acknowledging per the growth-tracking convention. * fix(#4209): remove per-run token waste from the shipped prompts Runtime prompt content, not session tokens: two real, per-invocation token costs in the code that ships. 1. agents/gsd-code-reviewer.md's critical_rules restated nearly all of load_context step 5's ~180-word untrusted-evidence contract in ~90 more words, breaking this section's own established terse one-liner style (every other rule here is 1-2 sentences). This prompt loads fresh on every /gsd:code-review invocation. Shrunk to a one-line cross-reference, matching how write_review's own reference to step 5 already does it. 2. buildSourceReviewPrompt repeated the base SHA on every single file line even though it is identical for every file and already stated once at the top of the prompt — O(files) wasted tokens on every dispatched lane for a 50-file review, for zero information gain. File lines are now bare paths. * fix(#4209): resolve reviewer-lane trait in-process, fix CI failures found in review round 3 Opus critical-code-reviewer found a real Blocking defect in the --cap-id/ --point self-invocation added last commit: `dispatch-step` spawned `loop render-hooks <point> --raw` as a subprocess and bare-JSON.parse'd its stdout, but `io.cjs`'s output() redirects any payload over 50000 chars to `@file:<path>` instead of inline JSON -- the same overflow protocol this feature already unwraps for its OWN dispatch result 60 lines later in code-review.md. A large-enough activeHooks envelope (more installed capabilities/fragments) would throw, get silently swallowed by the bare catch, and misreport a real trait as trait_not_enabled with zero diagnostic. Fixed by extracting the config/registry/capability-state resolution `cmdLoopRenderHooks` already performs into an exported pure function, resolveActiveHooksForPoint (both `cmdLoopRenderHooks` and dispatch-step now share it), and calling it in-process from dispatch-step instead of spawning a subprocess at all. This eliminates the @file: exposure entirely (the dispatch-step path never touches the rendered-string envelope or its JSON-stringify/50000-char threshold), removes one subprocess spawn per code-review invocation, and gives a genuine diagnostic (stderr warning) on resolution failure instead of silent fail-closed. Corrected three doc/ docstring references to the now-removed subprocess self-invocation. Also fixes 2 real CI failures this round surfaced: - lint-tests: the agy-F1 control-char regex fix's `eslint-disable-next-line no-control-regex` comment was unused under this project's ESLint config (verified locally: the rule never actually flags \x00-\x1f in this repo's config) -- a mistake from an earlier commit this session, never actually lint-checked before push. Removed the disable comment. - security (prompt-injection-scan): the agy-F1 regression test's crafted fixture literally contains "Ignore all prior instructions." as test data proving validatePaths rejects it -- allowlisted the test file, same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class as existing entries. Also trimmed agents/gsd-code-reviewer.md's load_context step 5 (R2): one bullet stated "untrusted, never a command" three different ways in one paragraph, and a same-file duplicate of write_review's schema rule. Consolidated to state each rule once. Declined one suggestion from this round: shrinking code-review.md's EXTERNAL_EVIDENCE_BLOCK to a bare evidence list. Two tests (tests/code-review-pipeline-regression.test.cjs's CONS-01..03 block, tests/code-review.test.cjs's CONS-02 test) deliberately lock the four- prohibitions restatement and the untrusted-evidence prose into the INJECTED block itself, not just the consolidator's system prompt -- adjacency of the warning to the untrusted payload it's warning about is a recognized prompt-injection defense-in-depth pattern from this workstream's original TDD plan, not accidental duplication. * fix(#4209): correct stale per-file base-SHA prose in the external prompt Leftover from removing the per-file base SHA repetition earlier this session: the review-request sentence still said "relative to its base SHA" (singular per-file framing) when there's now exactly one base SHA, stated once above the file list. Reads "relative to the base SHA above" now. * fix(#4209): make getLane/configGet/plan required deps, delete dead defaults R3/R4 from the review round I'd deferred as low-priority test-churn: this file's one production caller (gsd-tools.cjs's dispatch-step handler) always supplies all three, so the fallbacks were dead in production -- but each was actively WRONG if ever reached: the default configGet always returned undefined, silently disabling resolveLaneBudget's overflow guard; the default getLane looked up only first-party REVIEWER_LANES, diverging from production's overlay-merged roster; the default plan skipped per-host effort resolution entirely. These defaults were introduced by this PR's own earlier work (this file did not exist before #4209 -- first commit a760bfcda, 01-02), not inherited from elsewhere, so there's no external caller depending on the lenient contract. Turned out free to fix: making the three deps required and deleting defaultGetLane/defaultPlan needed zero test changes -- every existing test that actually reaches the per-lane loop already supplies getLane/plan explicitly, and configGet's only real dependent (the budget-overflow tests) already supplies it too. 788/788 tests pass unchanged, tsc/lint clean. * fix(#4209): define depth semantics for the external reviewer lane Verified this was a real bug, not a match to existing convention as I'd claimed when declining the suggestion earlier this session: the internal gsd-code-reviewer agent's own system prompt carries a full <depth_levels> block defining what quick/standard/deep mean and do (agents/gsd-code- reviewer.md:68-99). The external reviewer lane has no access to that persona at all -- it only ever sees buildSourceReviewPrompt's bounded text, which sent the bare depth label with zero definition to a third-party CLI with no other source of truth for what "standard" means. Added depthMeaning(), condensed from the internal reviewer's own <depth_levels> definitions so the two stay consistent, and interpolated it into the review-request sentence. 150/150 tests pass, tsc/lint clean. * fix(#4209): merge dispatch_reviewer_lanes' split fences into one shell invocation CR-01 (Opus critical-code-reviewer, confirmed by direct execution): the roster-matching fence set EXPLICIT_JOINED/EXPLICIT_REVIEWER_SLUGS, and a SEPARATE later fence read them via ${#EXPLICIT_REVIEWER_SLUGS[@]} to decide whether to dispatch at all. This file's own documented rule (its depth-resolution guard, stated explicitly a few hundred lines earlier) is that a guard and the extraction it protects must run as one shell control-flow decision, because markdown-fenced blocks do not share shell state -- this step violated its own file's rule for the entire feature's gating condition. Merged the roster-resolution fence and the dispatch-decision fence into one continuous bash block, removing the intervening prose that split them. Fixed the stderr-based failure detection in the same edit (RQ-01: checking whether stderr is non-empty misfires on any benign Node warning; now checks the actual exit status of the roster-resolution command). Verified by extracting the merged fence and executing it standalone, driving both branches: --codex resolves EXPLICIT_JOINED=codex, SLUGS_COUNT=1, and a real dispatch-step call succeeds; no flags resolves EXPLICIT_JOINED empty, SLUGS_COUNT=0, dispatch-step never invoked (COMP-01). 141/141 workflow tests pass, tsc/lint clean. * fix(#4209): depthMeaning accuracy, injection defense on all embedded fields, hoisted prompt write Batch of Required/Suggestion fixes from the Opus critical-code-reviewer + writing-for-agents pass: - CR-02/CR-03: depthMeaning() dropped real categories from quick (empty catch blocks, commented-out code) and deep (error propagation, state mutation consistency, circular dependencies) relative to the real <depth_levels> block, and had zero test coverage. Restored full accuracy and added tests that read the real agents/gsd-code-reviewer.md file directly, so drift between the two can't recur silently. Unrecognised depth now normalizes to standard's definition, matching that agent's own documented rule, instead of rendering an undefined bare label. - RQ-04: depth/baseSha/repoRoot/runDir land in the same markdown prompt `paths` does, but weren't checked for control characters like paths were (agy-F1's original finding). Hoisted CONTROL_CHAR to module scope and applied it to all four fields at the same provenance-check boundary. runDir previously had zero validation at all. - S1: deleted the dead `identity` parameter on `invoke` -- the one production caller already ignores it, no test read it by name. - S2: hoisted the shared prompt write above the per-lane loop -- promptPath is derived from runDir alone (constant across lanes by construction), so writing it once is both correct and cheaper than the per-lane write R1 introduced earlier this session. Discovered and fixed a real regression from the naive version of this hoist: an unguarded throw would have escaped dispatchReviewerLanes as an uncaught exception instead of a clean per-lane failure. Added a new PROMPT_WRITE_FAILED whole-dispatch reason, matching the existing validatePaths/MISSING_PROVENANCE halt pattern, with a dedicated regression test. - S3: moved `planned = true` past the budget-overflow gate, so `dispatched` only reports true once a lane has cleared BOTH plan and budget checks. - S5: relayed gsd-code-reviewer.md's own "performance issues are out of scope unless also correctness issues" policy into the external-lane prompt, which previously had no such guidance and could return findings the internal reviewer's own contract excludes. - RQ-05 (partial): shrunk this file's own header docstring's restatement of the trait-reuse architecture to a pointer at gsd-core/references/loop-hook-dispatch.md, the canonical home. 234/234 tests pass across the full reviewer-lane test suite, tsc/lint clean. * fix(#4209): dedupe roster-merge logic, consolidate trait architecture prose, add step completion criterion RQ-02: added a `review-lane explicit-from-argv` subcommand that reuses the SAME merged-roster logic (`laneBySlug`) `dispatch-step`/`plan`/`invoke` already share. code-review.md's ~18-line inline `node -e` reimplementing `loadRegistry`+`mergeReviewerLanes` (a rename-only copy of the block in gsd-tools.cjs) is now a single call to this subcommand -- the exact violation code-review-flags.cjs's own header warns against ("this is the canonical flag-parsing surface -- do not replicate inline bash parsing"). RQ-03: an empty --cap-id XOR --point now warns distinctly from the legitimate no-context opt-out (both absent) -- a caller that named a capability without its point was silently indistinguishable from a correct opt-out. Also hardened the CODE_REVIEW_POINT config-get fallback: it only ever fires when the config-get COMMAND ITSELF fails (config-get already resolves the manifest's own schema default in the normal case), but that failure was previously silent. RQ-05/W-01/W-12/W-13: the "supportsReviewerLanes is a reusable trait resolved inside dispatch-step" explanation was restated in full in 5 places across this session's own review cycles. Consolidated to ONE canonical statement in gsd-core/references/loop-hook-dispatch.md; the other 4 (this file's own header, gsd-tools.cjs's comment, docs/ARCHITECTURE.md, code-review.md's step-opening comment) now point at it instead. W-05/W-06: loop-hook-dispatch.md described "false or non-boolean" as two inert cases when capability-validator.cjs already rejects non-boolean at load -- restated as the two cases that actually reach this code. Removed a "do not hand-roll trait resolution" prohibition whose target no longer exists once the positive description precedes it. W-04: deleted a no-op sentence in agents/gsd-code-reviewer.md ("missing block means proceed as normal") -- an absent optional block already means proceed as normal without being told. W-08/W-09: replaced longhand "zero selection/plan/invoke calls" and the made-up compound "byte-for-behavior [un]changed" with the token this session's own docs already coined for this concept (inert) and the word that means what byte-for-behavior was reaching for (unchanged). W-10: dispatch_reviewer_lanes had no completion criterion -- added one sentence naming the checkable end state (EXTERNAL_EVIDENCE_BLOCK is set, either populated or empty). This exact sentence would have caught the cross-fence bug fixed two commits ago at authoring time. Declined from this round, with reasoning: W-02/W-03 (trim the untrusted-evidence restatement in EXTERNAL_EVIDENCE_BLOCK/critical_rules) -- two tests deliberately lock this as intentional adjacency-based prompt-injection defense-in-depth, not accidental duplication (see this branch's own earlier commit). S4 (wrap LANE_RUN_DIR in a creation-site `trap ... EXIT`) -- would fire at the end of the CREATING fence, before spawn_reviewer's agent ever reads the evidence files, given this file's own documented fenced-block execution model; the existing named cross-reference between creation and cleanup already satisfies the co-location concern without introducing that regression. 853/853 tests pass across the full reviewer-lane test suite, tsc/lint clean. * fix(#4209): merge CODE_REVIEW_POINT into dispatch_reviewer_lanes' one fence, stop test from spawning real codex Round-5 review (agy) found the same cross-fence-split bug CR-01 already fixed for EXPLICIT_JOINED/EXPLICIT_REVIEWER_SLUGS: CODE_REVIEW_POINT's config-get fallback lived in an earlier, separate fence from the fence that consumes it via --point, split only by prose (not a guard, per this step's own documented rule). Merged into the single continuous fence and added a structural test asserting exactly one bash fence in the step. The new end-to-end regression test for this used --codex, which drives the fence's real `review-lane dispatch-step` call and, with the codex binary present on PATH, spawns the real external CLI — which then blocks on interactive auth with no stdin (BL-01). Stubbed gsd_run for `review-lane dispatch-step` only (captures argv instead of executing), keeping the real config-get/explicit-from-argv calls the test is actually about. * fix(#4209): split control-char vs missing provenance reason, realpath-check path escapes, stale comment Round-5 review (Opus) warning-tier findings: - WR-04: MISSING_PROVENANCE covered both "field absent" and "field present but a control-character injection attempt" — a caller distinguishing a config problem from a security event couldn't tell them apart. Split into MISSING_PROVENANCE (absent) and INVALID_PROVENANCE (present but invalid). - WR-05: validatePaths' containment check was lexical only (path.resolve), so a symlink whose own path sits inside repoRoot could still point outside it. Added an fs.realpathSync check (ENOENT-tolerant — a git-diff path can legitimately name a file already deleted in a stale worktree), realpathing repoRoot itself too so a symlinked repoRoot (e.g. /tmp on macOS) doesn't false-positive-reject its own real children. - WR-08: a comment in the per-lane loop still said a throwing writePromptFile() was caught there — stale since the prompt write was hoisted above the loop in an earlier round. WR-03 (validate depth against the quick/standard/deep enum) was considered and declined: this dispatcher is deliberately capability-neutral (see the existing "synthetic step context" test, which passes a non-code-review depth label on purpose to prove no code-review-specific special-casing exists). WR-01 (double registry load), WR-02 (trim-vs-hard-fail budget semantics), and WR-07 (reason omitted on the aggregate return) were verified against source and are not bugs — see review notes. * docs(#4209): document LANE_RUN_DIR's early-exit trade-off as accepted, not a gap Round-5 review (Opus, BL-03) flagged that an early exit between dispatch_reviewer_lanes and commit_review leaks the run-scoped temp dir. A trap-based cleanup was considered and rejected: if a step genuinely runs as a separate process, a trap set at creation time would fire at the end of that SAME fence, deleting the directory before spawn_reviewer/commit_review ever read it — worse than the leak it would fix. review.md's own gather_context/cleanup pair for the identical resource class (a run-scoped reviewer temp dir) already makes and documents this exact trade-off: cleanup runs only on a documented success path, and a leftover $TMPDIR entry is explicitly called cheaper than destroyed evidence. Recording that precedent here so this isn't re-raised as a live gap in a future review. * fix(#4209): register the WR-05 symlink-escape test's synthetic docs/ path reviewer-step-dispatch.test.cjs's "capability-neutral reuse" fixture passes paths: ['docs/spec.md'] as a synthetic, never-read path proving the dispatcher has no code-review-specific special-casing. lint-docs-guard- registration correctly flagged this as an unregistered docs/ path reference — add the docs-guard-exempt marker and its pinned baseline entry, the same pattern every other synthetic docs/ literal in this test suite already uses. * fix(#4209): backfill changeset pr: field with the real upstream PR number changeset-lint's fail_pr_field_drift caught the fragment still pointing at the fork PR (17) instead of the upstream one (open-gsd/gsd-core#4323) this branch is now also open against. * docs(#4209): amend ADR-2782 for the supportsReviewerLanes step-trait seam trek-e's review (2026-09-07, gsd-core#4323) found a real ADR gap: every decision in ADR-2782 (D1-D9) and every prior dated amendment governs the `role: "reviewer"` capability body and its one consumer, /gsd:review. This PR's actual new seam - a `supportsReviewerLanes: true` trait on an ordinary feature capability's `steps[]` entry, projected through loop-resolver.cts and resolved in-process via resolveActiveHooksForPoint - is a different capability axis (steps/gates/contributions) that the ADR's own scope note explicitly places out of reach. Per docs/contributor-standards.md's "Amending an accepted ADR", an in-place dated section is the established, lighter-weight path for an addition that stays within the ADR's existing decisions - used twice already in this same file - so this appends a third dated entry documenting the new seam, its consumer, and why it reuses the existing D1-D9-governed plan/invoke machinery rather than adding a second one. No decision is reversed; no new Amends/Amended-by pair is needed since the steps/gates/contributions axis already carries reciprocal links to ADR-857 and ADR-894. * fix(#4209): close two test-quality gaps trek-e's review found Minor 1: validatePaths (a path-shape parser guarding the prompt- injection/path-traversal trust boundary) had only example-based coverage, violating ADR-456's rule that parsers/budget limits carry at least one fast-check property test. Adds three: safe-segment paths are never rejected, a single leading "../" always escapes the one-segment repoRoot, and a control character anywhere is always rejected - one property per rejection reason validatePaths owns. Minor 2: the budget-overflow check (`estimatedTokens > budget`) was only ever exercised far below budget or at budget:0 (unbounded), never at the exact threshold crossing where a `>` vs `>=` off-by-one would hide. Adds three exact-boundary tests using the real estimateTokens/ buildSourceReviewPrompt the module calls internally, so the resolved token count is exact rather than approximated: budget == estimate (must pass), budget == estimate - 1 (must fail), budget == estimate + 1 (must pass). Also extracts okPlan()'s fixture timeoutMs into a named constant - local/no-adhoc-timeout-literal (#4446) landed on next after this branch was authored and flagged the pre-existing literal on rebase; it is fixture data for a synthetic plan object dispatchReviewerLanes never waits on, a distinct class from tests/helpers/timeouts.cjs's real subprocess norms. * fix(#4209): update docs-guard-registration baseline for the new ADR citation reviewer-step-dispatch.test.cjs's new fast-check property tests cite docs/adr/456-test-rigor-architecture.md in a justifying comment (never a real read). lint-docs-guard-registration fingerprints every docs/ path string an exempted test file mentions and fails on drift so a human re-confirms the exemption still holds - re-confirmed, and the baseline is updated to match. * fix(#4209): point changeset pr: field at the fork PR for CI validation changeset-lint's fail_pr_field_drift check compares the fragment's pr: field against the PR the CI run is actually attached to (GITHUB_EVENT_PATH), not a fixed target. Rehearsing this branch on fork PR davdittrich/gsd-core#17 needs pr: 17 to pass that check; the prior commit's pr: 4323 (the real open-gsd upstream PR number) is correct for that PR but fails here. Backfill to 4323 happens again, as the last commit, immediately before the approved push to open-gsd#4323 - never leaving pr: 17 on the branch that ships upstream. * fix(#4209): reject promptChannel:none lanes from source-review dispatch CodeRabbit found a real scope mismatch: coderabbit's lane declares promptChannel: 'none' and reviews the working tree on its own terms, fed nothing (review.md:367). Silently dispatching it through dispatchReviewerLanes would ignore the bounded paths/depth/baseSha scope buildSourceReviewPrompt promises and let the lane review whatever it independently sees fit, violating this interpreter's own scoped, metadata-only contract. Reject before plan()/invoke(), same as an unresolved slug. * fix(#4209): scope CONS-02 test to the evidence-block line, not the whole file CodeRabbit found the whole-file match on workflowContent would still pass if UNVERIFIED and re-open/reopen appeared in two unrelated parts of this 1000+-line workflow, proving nothing about the actual evidence block's contract. Line-filtered via splitLines (not a bare-\n regex spanning readFileSync content) so this stays CRLF-portable and passes local/no-unbounded-quantifier and local/no-crlf-fragile-split. * fix(#4209): guard DISPATCH_JSON substitution and capture its stderr CodeRabbit found the dispatch-step command substitution unguarded: a non-zero exit could leave DISPATCH_JSON empty (or halt the step under errexit with no warning), and the downstream reducer would only ever report the generic unparseable_dispatch_output reason, discarding the command's own diagnostic. Guarded like the existing CODE_REVIEW_POINT/ EXPLICIT_JOINED calls above it: capture stderr to a temp file, surface it in a warning on failure, and fall back to a parseable dispatch_ command_failed JSON stub so the reducer's existing reason-reporting path still fires. * docs(#4209): fix byte-for-behavior wording and missing colon, regenerate CodeRabbit found "byte-for-behavior" should read "byte-for-byte" (the established repo term for output-identical unchanged behavior) and a missing colon after the bold "Optional external reviewer lanes (#4209)" lead-in in docs/features/code-review-pipeline.md. Fixed in the two hand-authored sources (commands/gsd/code-review.md, docs/features/ code-review-pipeline.md) and regenerated the two derived projections (skills/gsd-code-review/SKILL.md via gen-plugin-skills.cjs, docs/ FEATURES.md via gen-features.cjs) so they stay in sync. * fix(#4209): drop the fabricated DISPATCH_JSON fallback stub (Windows CI) The prior fix's fallback `DISPATCH_JSON='{"ok":false,...}'` embeds double-quoted JSON keys inside a single-quoted shell literal. That extra quote density, inside an already quote-heavy ~8KB driver string, passed bash -n and the full local suite on Linux but broke Windows Git-Bash: `dispatch_reviewer_lanes computes CODE_REVIEW_POINT ... end to end (#4209 round 5)` failed on two Windows CI shards with `bash -c: unexpected EOF while looking for matching '''` — a Windows argv-to- command-line re-quoting edge case, reproducible on rerun, not a flake. Root-caused via gh api job logs plus a byte-identical local reconstruction of the test's own driver script. Fix: drop the fabricated stub. The downstream node -e reducer already falls back to reason `unparseable_dispatch_output` on any JSON.parse failure, so an empty/partial DISPATCH_JSON on command failure is still handled correctly, with zero new quoting risk. * revert(#4209): drop the DISPATCH_JSON stderr-guard nitpick (Windows CI) Two materially different mechanisms for the same CodeRabbit Nitpick ("Trivial | Quick win") both broke Windows Git-Bash reproducibly: a single-quoted JSON-literal fallback ("bash -c: unexpected EOF ... matching '''") and, after removing that, a plain `head -1 "$VAR"` inside a nested command substitution ("unexpected EOF ... matching '"'"). Both passed bash -n and the full local suite on Linux every time; both failed the SAME test deterministically on Windows CI. Two attempts at the same class of fix (nested-quote construction near this exact step) is the retry limit - reverting to the original, already-shipped, Windows-verified unguarded form rather than continuing to guess at a third quoting mechanism for a Trivial- severity nitpick. Logged as bug-221/bug-222 in .wolf/buglog.json for anyone attempting this again: the fix belongs outside this specific markdown-fence-driver test harness (e.g., a real .sh helper script) if it's worth doing at all. * fix(#4209): backfill changeset pr: field to the real upstream PR before push Fork validation (davdittrich/gsd-core#17) needed pr: 17 to satisfy changeset-lint's PR-number check while rehearsing there; this is the last commit before the approved push to the real upstream PR (open-gsd/gsd-core#4323), so the field points at that PR number again. --------- Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-07 22:52:33 -04:00
Tom Boucher	a0f8f956c4	enhance(#4139): Phase 3 — partition rules + the five checks (#4497) ... * enhance(#4139): Phase 3 — partition rules + the five checks ADR-4139 Decision 5, epic #4139 Phase 3. Issue #4403's own "Proposed behavior" section lists four checks; the ADR's Decision 5 and its own phase table ("partition rules + the five checks") list five — the same four plus "boundary moves are declared, ongoing". Same issue-vs-ADR drift Phase 2 hit on the detail.md vs detail/*.md layout: the ADR is the locked, reviewed document, so it wins. This PR implements all five. docs/PARTITION-RULES.md (new) is the partition-rules document: the partition rule itself, the protected-content list and <!-- gsd:protected --> sentinel syntax (relocated unchanged from gsd-core/references/compact-content-protected-content.md, now deleted — it was never referenced by any runtime workflow Read, only by the predecessor test as documentation, so nothing at runtime regresses, and removing it from gsd-core/references/ also drops it from all 19 installed-project shipped-content trees for a file nothing ever read), and the five checks explained for a human reader. Referenced from a new CONTRIBUTING.md subsection under "Editing shipped content". tests/helpers/compact-content-split.cjs (new) is the shared mechanics: split discovery (any gsd-core/workflows/<name>/detail/*.md paired with <name>.md — no registry file, a pair is registered by existing on disk), line normalization (carries forward Phase 2's bare-label-line isTrivial fix and the canonical gsd_run-launcher-preamble exclusion), sentinel extraction, and a Boundary-Move-Declared commit-trailer reader that is a direct structural port of tests/helpers/emitted-runtime.cjs's Emitted-Drift-Ack-Hash/-Growth trailer reader (ADR-3942) — same merge-base range, same fail-closed throw on an uncomputable range, same dedupe/conflict rules. tests/compact-content-partition-guard.test.cjs (new) is the actual guard, superseding tests/plan-phase-compact-split.test.cjs (deleted — its per-pair checks are now the general guard's job for plan-phase specifically). Checks 2 (disjointness) and 3 (registration + size cap) run unconditionally against every registered split. Checks 1 (completeness, fires once per split on the PR that introduces a new detail/ path), 4 (protected content — no trailer can ever excuse this one, unlike check 5) and 5 (boundary moves declared) are PR-diff-scoped against the resolved base ref and skip cleanly when there's nothing to compare (a fresh clone, no PR in flight) — a deliberate asymmetry from check 5's trailer reader, which must throw rather than silently pass when ITS range is uncomputable, since that function is answering "did this PR declare its moves" rather than "is there even a diff to look at". Each of the five checks carries a RED (deliberately broken fixture) / GREEN (fixed) test pair, built against synthetic temp files or real throwaway git repos, per this repo's rule that a guard nobody has seen go red is not yet a guard. Building the real fixtures caught and fixed one real bug before it shipped: check 4's line-presence test was using the trivial-line-filtered normalizer, so a byte-identical spine falsely reported its own protected code-fence line as "deleted" — fixed with a non-filtering membership check. Extending docs/INVENTORY.md's "Workflow Sub-Files" table for `detail` surfaced a pre-existing, unrelated gap in the SAME area: gsd-core/workflows/<name>/templates/*.md is a fourth workflow sub-file kind that already existed on disk and was already known to lint-response-language-coverage.cjs's FRAGMENT_DIRS, but was invisible to gen-inventory-manifest.cjs and undocumented in that table. Fixed alongside it, same pattern, same PR, rather than deferred. Also, mechanically required by the new fourth sub-file kind: - scripts/lint-response-language-coverage.cjs: `detail` added to FRAGMENT_DIRS alongside modes/steps/templates — a detail/<part>.md inherits its parent's response_language coverage through the same per-file proof, not a parallel one. - tests/workflow-size-budget.test.cjs: explicit regression test locking that detail/ files are governed solely by the hard, non-waivable NEW_FILE_CAP (tests/helpers/emitted-diff.cjs) and never by the XL/LARGE/DEFAULT spine tiers — true by construction (measureWorkflows/listWorkflowStems don't recurse), made explicit per the issue's own Done-when item rather than left true-by-omission. - scripts/gen-inventory-manifest.cjs: `workflow_detail` and `workflow_templates` NESTED_FAMILIES entries; docs/INVENTORY-MANIFEST.json regenerated (plan-phase/detail/elaboration.md, discuss-phase/templates/*.md now tracked); docs/INVENTORY.md's table updated to four kinds. Verified: `npm run lint:ci` clean with the eslint cache cleared. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4403): review findings + a real gsd-test failure in the new guard Two orthogonal review passes (Standards + Spec, isolated sub-agents) plus a separate security review ran against the prior commit. Fixed everything each surfaced: - Security (Low, path-traversal existence oracle): checkRegistration's dangling-reference check extracted detail-path-shaped substrings from spine PROSE via a regex that permits `.`/`/` freely, then joined them onto repoRoot and probed fs.existsSync with no containment check — a spine file containing `../../../etc/detail/passwd.md`-shaped text could make the guard test file existence outside the repo. Added a path.relative-based containment check before the fs.existsSync call; anything that resolves outside repoRoot is now reported as a dangling reference directly, never probed on disk. - Standards (Boundary Coverage): the size-cap fixtures covered NEW_FILE_CAP and NEW_FILE_CAP-1 but not NEW_FILE_CAP+1 — added the third boundary-point case CLAUDE.md's TEST RULES require (limit-1/limit/limit+1). - Standards (Property-Based Testing): extractProtectedBlocks (a sentinel parser) and the new parseBoundaryMoveTrailerValues (a declare/dedupe/conflict parser, bijective-shaped) had no fast-check property test. Added three: a render/parse bijectivity property for the trailer parser (mirroring the exact ADR-3942 sibling test's alphabet/idiom), a dedupe-is-idempotent property for the same parser, and a well-formed-sentinel-round-trips property for extractProtectedBlocks. Then dispatched gsd-test on the resulting commit. It found a real bug the reviews couldn't have caught (none of them can run inside gsd-test's sandbox): checks 4/5's real-repo assertion failed against plan-phase's own split, reporting DISK_PLANS/#3218-comment lines as "undeclared boundary moves" — content Phase 2 (#4402) legitimately moved into detail/elaboration.md months before this PR's Boundary-Move-Declared mechanism existed to require a trailer for it. Root cause: `resolveBase()`'s own doc comment already documents that no `origin/*` remote-tracking ref exists inside the gsd-test sandbox container, and its fallback candidate (a bare `next` branch) can resolve to a point in history that predates an already-merged, already-reviewed split — making that split look "newly introduced" from the sandbox's vantage point. Check 1 (completeness) already scopes itself correctly to only genuinely-new detail paths (git diff status 'A'); checks 4 and 5 did not share that scoping, so a stale base made them re-litigate a settled split retroactively. Fixed by having checks 4/5 skip any split name check 1 already counted as newly-split — their own premise ("did an EXISTING split shed/undeclare something") does not apply to a split that is, from the resolved base's vantage point, brand new; that is check 1's domain alone. Verified locally (25/25 tests pass via a direct `node -e` require, since `node --test` is blocked in this repo) and via re-reasoning through the exact real-repo scenario the gsd-test failure showed. Also regenerated all 19 tests/fixtures/install-tree/*.json goldens — the prior commit's deletion of gsd-core/references/compact-content-protected-content.md was never reflected there, which is what golden-install-tree.test.cjs's other 19 failures in the same gsd-test run were. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4403): backfill changeset pr number to 4497 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4403): isolate codex-config.test.cjs into its own chunk, root-causing the Windows CI failure PR #4497's "full test (windows-latest, 24, shard 2/3)" job failed: run-tests killed chunk 3/8 at the 600s per-chunk backstop, with codex-config.test.cjs (weight 17.87, by far the chunk's dominant cost) packed alongside 39 other files. Traced, not assumed: - scripts/run-tests.cjs's own timeout-headroom comment for the OUTER per-shard timeout documents that "adding one test file reshuffled 115 of 268 unit files between shards" — shard/chunk composition is architecturally known to be unstable to single-file additions, which is exactly what this PR's own new tests/compact-content-partition-guard.test.cjs is. - A second comment, dated 2026-09-06 (one day before this PR, PR #4428's own CI), already documents the SAME chunk hitting the SAME 600s backstop with the SAME file (codex-config.test.cjs, "a genuinely MEASURED weight of 17.87 — not a stale-table miss") dominating it — the fix then was cutting the Windows per-chunk budget from 60 to 40. That cut clearly was not enough: two documented incidents in two days, at two different budget settings, both centered on one file that alone consumes ~45% of even the reduced Windows budget. - tests/test-timings.json's own header confirms its source data (test-events-linux-node22/24.jsonl) is Linux-only, and run-tests.cjs's own chunk-timeout diagnostic already prints "real Windows cost runs ~2.2x the recorded figure" — the packer's weight-balancing is working off data that is both stale (table last regenerated 2026-08-07) and known to underestimate the platform where the failure occurs. Given codex-config.test.cjs is disproportionately heavy AND every companion sharing its chunk is decided by a packing algorithm already documented as reshuffling unpredictably on any new file, tuning the shared budget a third time only moves the marginal line to wherever the next new file happens to land — it does not remove the gamble. Isolating codex-config.test.cjs into its own dedicated single-file chunk, unconditionally and on every platform, removes it at the source: the file never enters the pool packChunks balances, so no other file's packing changes, and no future single-file addition (mine or anyone else's) can silently reintroduce this exact failure by landing in its chunk. Extracted as a small pure function, partitionIsolatedFiles (mirroring this file's existing pattern of pulling packing/analysis logic out of main() for in-process unit coverage — see computeSweepProtectSet, analyzeChunkEvents), with 6 new tests in tests/run-tests-harness.test.cjs covering basename matching across path separators, near-miss non-matches, the empty-list case, and the isolated-set contents. Root cause is now closed rather than papered over with a retry: this failure is a property of one specific heavy file's chunk placement, not something that recurs randomly. If codex-config.test.cjs itself is ever genuinely sped up, this isolation can be revisited — this is a packing-side mitigation for a known file's cost, not a claim the cost is irreducible. 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-07 16:44:53 -04:00
Tom Boucher	38e4ce5f62	fix(#4186): anchored status vocabulary, record-session arg guard, recount pin (#4381) ... * fix(#4186): anchored status vocabulary, record-session arg guard, recount pin Three defects from #4186: 1. normalizeStateStatus ran a first-match-wins SUBSTRING chain over the free-prose body Status field, so prose merely mentioning a status word was silently rewritten to a credible wrong token (a .planning/ path in Italian prose -> status: planning; verifica -> verifying; completezza -> completed). Recognition is now an ANCHORED whole-field match against a declared vocabulary (STATUS_EXACT_TOKENS + STATUS_ANCHORED_PATTERNS, state-document.cts) — case/whitespace-tolerant, branch-order artifacts preserved (Planning complete -> planning; Phase complete — ready for verification -> verifying). The recorded lenient fallback (#3873 row 26) stands: unrecognized prose passes through verbatim. Read-side consumers (W011, statusline) ride the same function. 2. The progress recount skew (stray *-SUMMARY.md inflating completed_plans) is already dead on next via #1988/PR #2016 (countMatchedSummaries pairs summaries to plans) — verified live and pinned with regression rows composed against the #4129/#4359 ratchet. 3. state record-session with no args executed and wrote STATE.md; it now errors like state update (stopped-at or resume-file required), handler- side so SDK callers are covered too. Four tests pinning the bare-call write are updated to the new contract. * fix(#4186): update status pins to the anchored vocabulary contract Bench round 1 follow-ups: - Legacy bare 'Milestone complete' kept as reader-side vocabulary (ADR-2207 removed the writers, not recognition of legacy files). - state.test pins updated: 'Paused at Plan 3' and round-trip 'Executing Plan 5' were pins of the substring guessing itself — the round-trip now uses the real handler form 'Executing Phase 5'. - record-session no-op/no-fields tests repurposed to the usage-error contract (CLI + SDK-level ExitError), byte-unchanged assertions kept. - statusline tests repinned: vocabulary values collapse to keywords; narratives render the documented first-word fallback instead of a guessed token. Hook doc comment updated to match. - docs-guard exempt baseline: state.test.cjs now cites docs/CLI-TOOLS.md. - docs/CLI-TOOLS.md: record-session signature notes the required flag. * fix(#4186): repair a dangling sentence in the schema docstring * test(#4186): bound the completed_plans scan regex (#2128 class) * chore(#4186): backfill changeset PR number --------- Co-authored-by: sim <sim@local>	2026-09-06 17:08:24 -04:00
BeeHiggs	1a358ce0fd	feat(#2761): bracket-tolerant read path — roadmap/validate/verify/state recognize bracket ids (epic #612 PR-2) (#2867) ... * feat(#2761): gated heading-intro selection + one bracket identity grammar Foundation. Two owner-level changes plus a federated convention resolver; no reader consumes them yet. 1. GATED SELECTION, not an ungated widening. Widening every heading matcher requires the claim "no legacy ROADMAP contains a `[CODE.MM]` bracket followed by a digit", and that is false: `### [RFC.2119] 5:`, `### [v1.0] 2024:`, `### [ADR.612] 3:` and `### [ISO.8601] 2026:` are ordinary headings, and a widened reader claims each as a phase — moving phase_count and total_phases and adding W006 on projects that never opted in. No narrowing rescues it: the premise is about documents we do not control. `phaseHeadingPrefixSrcFor(baseline, convention, capturing?)` selects the pattern SOURCE at construction time. A project whose resolved `phase_id_convention` is not exactly 'bracket' compiles the same source string it compiled before. `baseline` is explicit because whether a site spells the any-bracket prefix or a bare `Phase\s+` is a fact about that site's history: handing the wider grammar to a bare site retro-grants tolerance it never had, in both directions — warnings appear, and a warning that fires today vanishes. Both bracket forms CAPTURE. `[GSD.999] Phase 07:` previously matched through the base alternative, which captures nothing, so a reader saw no bracket, fell back to the legacy token rule, and counted a labeled icebox heading while excluding the label-less one beside it — two derivations of one ROADMAP disagreeing. 2. ONE bracket identity grammar, one width rule. The milestone width is reconciled with the emit validator: pad2 output, so two digits or 3+ with no leading zero. Earlier spellings diverged in both directions — admitting `002`, which the validator rejects, and a bare `0` pad2 never produces — and the section recognizers accepted `[GSD.2]`, which SCOPED a milestone no phase heading could then resolve into, recreating the on-disk-count fallback this epic removes. An unpadded bracket is now uniformly malformed: it scopes nothing, bounds nothing, sections nothing. W005 on its directories is the surfacing signal. The milestone field is boundary-anchored, so a malformed run cannot match by its prefix (`GSD.002-01` read as sentinel `00`). Recognition stays case-insensitive because readers compile `/i`, but identity helpers match `[A-Z]`, so a captured id is folded first — otherwise `### [gsd.999] 07:` failed every sentinel test. The qualified key shares the width, the `(?=-|$)` boundary and the single-sub-phase shape of the directory token, because phaseTokenMatches returns unconditionally on a qualified hit: a key matching a directory isPhaseDirName rejects would be a final wrong answer. 3. resolvePhaseIdConvention federates workstream -> root exactly as config-loader does — including that root is a fallback only when a WORKSTREAM is active, so a project-scoped directory stands alone. loadConfig cannot serve this: it merges against CONFIG_DEFAULTS and drops keys it does not know, and this key is not among them. It governs the bracket-selection reads ONLY. PHASE_HEADING_PREFIX_SRC is left byte-identical: PR-1 shipped it, nothing consumes it, and it is superseded rather than redefined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): roadmap.cts selects its heading grammar from the convention Six matchers build their intro through the gated selector, and cmdRoadmapAnalyze / cmdRoadmapGetPhase / getRoadmapPhaseWithFallback each resolve the convention ONCE per command and thread it down. Three sites take the any-bracket baseline (they already tolerated `[anything] Phase N`); three take label-only (they spelled a bare `Phase\s+`). Handing the wider grammar to a label-only site retro-grants tolerance it never had — and not only by adding matches: on a legacy repo an unchecked `- [ ] **[v1.0] Phase 05: Thing**` bullet would start SUPPRESSING the W006 that fires today. Sentinel handling under bracket ADDS a rule rather than replacing one: a bracketed heading is a sentinel when its bracket milestone is reserved (`### [GSD.999] 01:`) OR when its token is, so the engine-wide 0/999 backlog convention keeps applying to `### [GSD.02] 999:`. Replacing the token rule let a mid-migration ROADMAP — bracket headings plus a legacy backlog block, exactly the content this epic targets — add entries to the progress denominator. The captured id is folded before the identity test, so a lowercase `### [gsd.999] 07:` is excluded too. The DIRECTORY read is threaded too. `cmdRoadmapAnalyze` resolves the convention once and hands it to all four of its heading/checklist patterns, but the single `phaseTokenMatches` call that decides `disk_status`, `plan_count`, `summary_count`, `has_context` and `has_research` was left two-argument — so every canonical `{CODE}.{MM}-{PP}-slug` directory read as `no_directory` with zero counts, on the PR's own headline verb, while the SAME build resolved those same directories correctly in three other places on the same repo (W006/W007 via phaseTokenFromDir, `state json` via the milestone filter, and the W021 milestone-complete read through this very helper's three-argument form). It failed ONLY for the directory shape the convention exists to name: a mid-migration bracket repo carrying legacy `01-one` dirs resolved fine, which is why nothing caught it. Measured, bracket vs its flat-legacy twin: `[["01","no_directory",0,0],["02","no_directory",0,0]]` against `[["01","complete",1,1],["02","planned",1,0]]`. The oracle is the twin, computed in the same test run, plus exact literals — `grep disk_status tests/adr-612-*` was zero hits before this, so neither the fix nor a future regression had any gate at all. Disclosed: a ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted. Silent invisibility during the migration window is the deliberate trade against claiming phases on projects that never opted in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): validate.cts selects its grammar; gated directory recognition The W006/W007 feeders take the resolved convention as a threaded parameter. These sites carry the letter-tolerant `[\w][\w.-]*` capture, which makes them where an ungated widening does the most damage: `### [RFC.2119] 5:` enters roadmapPhases as a phantom and becomes a W007 "in ROADMAP.md but no directory on disk" on a project that never opted in. buildRoadmapPhaseVariants also surfaces the tokens borne ONLY by sentinel-bracket headings. Surfaced rather than filtered in place because roadmapPhases feeds both a membership check and a missing-directory warning, and only the latter should ignore an icebox item. That set is OCCURRENCE-AWARE, and the subtlety is load-bearing: roadmapPhases is a TOKEN set, so `[GSD.999] 01` and `[GSD.02] 01` collapse to one entry. Keying suppression on the token alone let an icebox heading silence a REAL phase that happens to share its number — a false negative strictly worse than the warning it removed. A token is suppressed only when no non-sentinel heading bears it. Directory recognition is added as gated FUNCTIONS beside the exported RegExp constants, which stay byte-identical: the `{CODE}.{MM}-` prefix is string-indistinguishable from the letter-prefixed-decimal family this repo documents as ambiguous, and folding a branch in changes those constants' answers on exactly that family. A RegExp constant has nowhere to attach a gate. The recognizer mirrors the emit grammar and delegates the token to the canonical owner, so recognizer and resolver agree on rejected input as well as accepted. Both functions throw on a non-string, matching the call pattern they replace. buildRoadmapPhaseVariants' CHECKLIST scan is capturing, like its heading twin and like the sibling checklist scan in roadmap.cts, and for the reason that one states: the bracket id has to ride along or the sentinel filter is blind to `- [ ] **[GSD.999] 01: Icebox**`. Left un-capturing, the scan called every checklist token REAL, and the occurrence-aware un-suppression loop then deleted the icebox token the HEADING scan had correctly marked sentinel — so `validate consistency` warned that a bracket ICEBOX phase had no directory, in the HOUSE ROADMAP shape where an icebox appears as both a bold bullet and a detail heading. `validate health` stayed silent on that same repo, so the two verbs disagreed — which is the disagreement `sentinelPhases` exists to close. Both directions are pinned, because the failure mode of a careless fix here is the opposite one: a real phase sharing a sentinel's token must still warn. It does, in all four shapes that attack it (sentinel heading + real bullet, lowercase sentinel, sentinel after the real heading, colon-less bullet). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): count bracket headings, and retire them, in both derivations Both `total_phases` derivations select their grammar from the resolved convention, in one commit — cmdStateSync already carries the comment that it mirrors buildStateFrontmatter "so both report consistent percents (#3242 Bug B)", so teaching one and not the other ships that divergence. The #1514 retirement filter widens WITH the counter it protects. The canonical gesture strikes the checklist BULLET and leaves the detail heading intact, so a bracket-form retirement went undetected and the phase stayed in the denominator forever. That is half a fix alone: the retired key is compared against phaseKeyFromDir, which called extractPhaseToken with no convention. Both halves land here. Under bracket the sentinel token rule composes as the full engine set {0, 999}, so this counter agrees with `roadmap analyze`, which has always excluded both — otherwise the two derivations report different numbers for one ROADMAP and the changeset's "excluded from every count" is false as written. The LEGACY path keeps its pre-existing 999-only rule: widening it there would move legacy totals, so the two stay split off the bracket path exactly as they are today. The sync-side assertion reads the PERCENT sync writes into the STATE.md body, not the frontmatter total_phases. Sync's own counter never reaches that field — the read derivation writes it — so asserting the frontmatter after a sync measures the read path twice and lets a mutation to the write-path guard survive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): verify.cts bracket-coherence W021 + selected milestone-complete read The shipped milestone-prefixed W021 gate keeps its ROOT-only config read, verbatim base semantics. Federating it silently moved a legacy convention's answer in BOTH directions on workstream repos — a W021 that fires at base vanishing, and one that is silent at base firing. resolvePhaseIdConvention governs the new bracket-selection reads only. B6, the milestone-complete check, keeps its ungated POSTURE (bug-557 pins it with an empty config) but selects its grammar from the convention. Inferring 'bracket' from the shape of a matched bracket ran a repo-failing check against a legacy ROADMAP that merely contained `### [RFC.2119] 5:`. Directory resolution widens with the heading read, so a bracket repo whose phases are on disk stays silent, and a bracket sentinel is not reported as unstarted. checkBracketCoherence is advisory and gated. Anchored to tokenizeHeadings so fenced examples cannot warn and heading level is structural. Its scope rules each close a way it silently did nothing or fired wrongly: only a genuine MILESTONE heading opens or closes a section (a `### Notes` used to reset scope and disable both sub-checks); a legacy `## v3.0` DOES close it; an M-NN or letter-suffixed phase heading raises missing-bracket and CONTINUES; a bare `#### 2026:` is not a phase; the full h2-h6 range is processed. Its section recognizer shares the one milestone width, so an unpadded `### [GSD.3] 05:` can no longer be a phase to the id grammar and a section to the section grammar at once, silently re-scoping every warning after it. validate consistency suppresses bracket sentinels in its missing-directory warning — the two verbs disagreed, health suppressing via notStartedPhases while consistency did not. The legacy reading is untouched, including its pre-existing wart that `### Phase 999:` still warns there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): scope the milestone by its bracket; select the disk-side filter Two roadmap-parser reads, both of which made a bracket project's totals track the disk instead of the ROADMAP. The ADR pins the bracket milestone heading as `## [GSD.02] Foundation` — a name, no version — but scoping matched STATE's `milestone: v2.0` STRING against a heading, so the canonical form matched nothing and total_phases fell back to the directory count. The rule was re-derived in THREE places: extractCurrentMilestone plus two `milestoneBounded` guards; fixing one left the others falling back regardless, so they are now one gated helper. It matches the CANONICAL padded spelling only — accepting `0*N` bounded a milestone whose phases were invisible, which un-suppressed a progress percent computed off an unscoped disk count. getMilestonePhaseFilter's heading scan becomes the 14th selected read. On a bracket ROADMAP it collected nothing, so the filter degraded to pass-all and buildStateFrontmatter counted every other milestone's directories — making the bracket convention strictly worse than the M-NN one it supersedes on the property that matters most: totals must track the ROADMAP, not the disk. The DIRECTORY side of that same filter is selected with it. Teaching only the heading scan was half a fix and a worse one: `milestonePhaseNums` became non-empty, so the pass-all degrade stopped firing, but no bracket directory could satisfy the three legacy dir checks (numericRe fails on `GSD.02-05-five`, the custom-id match captures the project code `GSD`, and stripProjectCodePrefix does not strip a dotted prefix). Every bracket directory was rejected, and completed_phases / total_plans / completed_plans / percent all collapsed to 0 while `state sync` went on writing a percent off the unfiltered disk — `state json` reporting 0% on the same repo, in the same second, that STATE.md's body called 67%. That is the #3242 Bug B divergence this PR exists to avoid, and total_phases could not show it: `Math.max(phaseDirs.length, roadmapPhaseCount)` floors it at the ROADMAP count no matter how many directories are rejected. The dir side matches on the milestone-QUALIFIED id, delegated to the owner's gated `phaseTokenMatches(dir, id, 'bracket')`, not on the bare token: READING-B puts the milestone in the bracket, so `GSD.01-01-old-one` and `GSD.02-01-one` share the token `01` and only the qualified key separates them. The qualified ids are kept in their own set — a hyphen in `milestonePhaseNums` would flip `roadmapUsesHyphenedIds` and silently move the LEGACY dir path on a bracket repo — and the branch is ADDITIVE: on a miss it falls through to the three legacy checks, so a bracket project carrying legacy-shaped directories reads unchanged. Both are resolved lazily and gated, so the legacy path pays neither a config read nor a second scan and cannot change answer. The scoping call is also GUARDED: resolvePhaseIdConvention reaches planningDir, which throws a plain Error for a GSD_PROJECT/GSD_WORKSTREAM segment carrying `/`, `\` or `..`. At base the only planningDir call in extractCurrentMilestone sits inside the STATE-read try, so the function returned normally on such an environment; an unguarded one here let that escape and broke the never-throws invariant that getRoadmapPhaseInternal and getMilestoneInfo three hundred lines below carry #2245 / ADR-227 notes about. Unreachable through the CLI — GSD_WORKSTREAM is rejected up front by the workstream-name policy and GSD_PROJECT throws identically at base — but reachable by any in-process embedder, which is precisely who that invariant is for. The filter's own resolve call was already inside its try and is unaffected. The milestone-qualified key is formed only for a token that is itself a bracket phase token. `${bracketId}-${token}` is a string SPLICE, so a mid-migration heading carrying an M-NN label — `### [GSD.02] Phase 02-01:` — spliced to `GSD.02-02-01`, which the qualified-key grammar reads as milestone 02 / phase 02: the `-01` truncated, both such headings collapsing to one key, and the heading claiming `GSD.02-02-two`, the directory it does NOT name, while rejecting `GSD.02-01-one`, the one it does. The guard drops those headings back to the unqualified legacy path, restoring the base ACCEPTANCE VECTOR exactly — pinned against the milestone-prefixed reading of the same ROADMAP, which is base-identical on this shape. Scoped precisely, because the fixture moves one number that the guard does not touch: `total_phases` on it reads 1 at base and 2 here. That is the bracket heading COUNT this PR exists to add, not the splice — measured identical with and without the guard, and identical to what the canonical `### [GSD.02] 01:` spelling does on the same fixture (both read 2 with zero directories on disk, where base reads 0). The claim is base-equivalent ACCEPTANCE, not a base-equivalent reading. One consequence is stated rather than fixed: a heading whose token carries a hyphen still puts that hyphen into milestonePhaseNums and so still flips `roadmapUsesHyphenedIds`. Base does the same for that spelling, so preserving it is what keeps the shape base-equivalent; excluding the token would have moved answers versus base on malformed input. The comment at the qualified-set declaration is corrected to claim only what is true — it keeps QUALIFIED IDS out of that flag's input, not hyphens in general. The oracles ship with it, and they are the five numbers, not the one: the parity gate now asserts total_phases, completed_phases, total_plans, completed_plans AND percent, on both derivations, on two fixture shapes (one milestone; two milestones with stale prior-milestone directories on disk). The oracle is the flat-legacy twin, built in the same test run and compared number for number, plus exact literals so a shared wrong answer cannot pass. The oracle SUBSTITUTION is itself pinned. The M-NN spelling of these shapes could not serve, because buildStateFrontmatter's #2445 de-dup key captures only a directory's leading integer and collapses `02-01-one` / `02-02-two` / `02-03-three` to one — measured [3,0,1,0,0] against the flat-legacy twin's [3,2,3,2,67], identically at base and before this fix, and structurally unreachable from the bracket key space. That reasoning is only sound while it stays true, so a characterization test holds the M-NN reading down on the two numbers that do not depend on which directory wins the mtime race. Widen the de-dup key and it fails, instead of quietly invalidating the changeset's disclosure. Also adds the call-site pin. The structural table pins transcription against the selector; it cannot see a call site whose BASELINE ARGUMENT is wrong. Flipping verify.cts's milestone-complete site to the wider baseline grants a fires-on-every-repo check tolerance it has never had, and every behavioural test still passed. The pin reads the shipped sources and asserts the mode at each of the 14 sites, count-exact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): pin the bracket read surfaces in the parity gate This gate exists because #2043 fixed one bug across five hand-edited copies of a rule and #2232 was the residual that survived, because a later reader could not tell the copies were one rule. PR-2 adds two consumers, so they belong here. Surface 7 — the heading read and the directory read must agree about WHICH phase a `MM-<seg>` pair names, across the shared width corpus, and the bracket and legacy spellings of one heading must yield the same token. Surface 8 — the two bracket directory readers, in BOTH directions. Agreement on ACCEPTED input was already pinned; agreement on REJECTED input is where they actually diverged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2761): changeset Disclosures for the PR body (deliberate, not defects): - phase_id_convention is not a CONFIG_DEFAULTS key, so loadConfig drops it and cannot serve as the convention resolver however the file is federated. This PR ships its own workstream->root resolver; adding the key and its value enum is later-slice work. - Convention matching is strictly === 'bracket'. A misspelled value reads as not-configured and the project keeps legacy behaviour silently. - An UNPADDED bracket milestone (`[GSD.2]`) is malformed: it scopes nothing, bounds nothing, sections nothing, and is not a phase id. W005 on its directories is the surfacing signal. - WIDTH UNIFICATION MOVED FOUR MERGED PR-1 EXPORT ANSWERS on non-canonical inputs, none of which toDir can emit and none of which had a bracket caller at base: isSentinelPhaseId('GSD.0-01', 'bracket') true -> false isSentinelPhaseId('GSD.0999-01', 'bracket') true -> false getMilestoneFromPhaseId('GSD.2-01', 'bracket') 'v2.0' -> null getMilestoneFromPhaseId('GSD.002-01', 'bracket') 'v2.0' -> null The canonical pad2 sentinel spelling `[GSD.00]` still tests true. - FLAG TO MAINTAINER: docs/adr/612:132 reads "Sentinel behavior (0.x / 999.x -> milestone null) is preserved". After the unification that holds for the canonical `00` spelling only, not for a bare `[GSD.0]`. ADR wording is yours; flagging the tension rather than editing it. - The bracket sentinel rule COMPOSES with the legacy one — a bracketed heading is a sentinel when its bracket milestone OR its token is reserved. Under bracket the state-side token rule is the full {0, 999} set so both derivations agree; the LEGACY path keeps its pre-existing 999-only rule, unchanged. - validate consistency's legacy reading is untouched, including the pre-existing wart that `### Phase 999:` warns there while validate health suppresses it. - find-phase still cannot resolve a bracket phase directory. phase-locator.cts is outside this PR's module set. Sibling PR #2559's matchPhaseDirs calls phaseTokenMatches without a convention, so whichever slice lands second must thread it through. - Four of the five bracket readers scan raw ROADMAP content, so a bracket heading inside a fenced code block is read as a phase. Pre-existing for the legacy spelling; parity, not a new class. - roadmapPhaseLookupSources gained no bracket source: nothing emits a milestone-qualified query into it yet. - roadmap validate remains a separate, unfederated convention reader. Pre-existing and base-identical, but two verbs can disagree about the active convention on one project. - _diskScanCache keys on cwd while the values it caches are now convention-dependent. Not reproducible through the CLI; pre-existing for the workstream dimension, widened here. Stated as inconclusive. - A ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted — the deliberate migration-window trade. - THE READ AND WRITE PERCENTS STILL DIVERGE ON A MULTI-MILESTONE REPO, and that divergence is MIRRORED under bracket rather than closed. buildStateFrontmatter applies the milestone filter; cmdStateSync does its own fs.readdirSync and never calls it, so on a repo carrying prior-milestone directories the read path reports the SCOPED percent and the sync body reports the WHOLE-DISK one. Measured on the true base build (d04592de), flat-legacy spelling, 3 in-scope phases with 1 complete plus 2 stale prior-milestone dirs: `state json` [3,1,3,1,33], sync body 60%. The bracket twin of that repo now reads the same two numbers — 33 and 60. Scoping the sync counter would move every legacy repo's percent, which a bracket read-path PR must not do. The gate pins both sides, so the mirror cannot silently become a one-sided fix. - THE PARITY ORACLE IS THE FLAT-LEGACY TWIN, NOT THE M-NN ONE, and that is a measurement finding rather than a preference. buildStateFrontmatter's #2445 de-dup key captures only a directory's LEADING integer, so the M-NN dirs `02-01-one` / `02-02-two` / `02-03-three` all key to `2` and two of the three are dropped before they are ever counted: base reads [3,0,1,0,0] where the flat-legacy twin of the same repo reads [3,2,3,2,67]. Present identically at base and at HEAD, untouched here, and structurally unreachable from the bracket key space — `GSD.02-01-one` does not match that pattern at all, so every bracket directory keys to its own name. The source line already carries a `phase-id-owner:` sanction recording the divergence. Mirroring it under bracket would mean manufacturing a collision that cannot occur, so the gate compares against the flat-legacy spelling, which is uncontaminated. This paragraph is itself pinned: a characterization test holds the M-NN reading on the two numbers that do not depend on which directory wins the mtime race, so widening the de-dup key in a later slice fails the suite rather than silently making this disclosure false. - THE `phaseTokenMatches` CALL-SITE CENSUS, stated so the remaining gaps are auditable rather than implied. 13 call sites outside the owner (phase-id.cts). THREE are three-argument: verify.cts:2229 (the W021 milestone-complete read, already was), roadmap.cts:436 (`roadmap analyze`'s directory lookup, threaded by this PR) and roadmap-parser.cts:792 (the disk-side milestone filter, added by this PR). The other TEN are two-argument and stay that way — phase.cts ×5 (220, 277, 444, 585, 1547), phase-locator.cts:62, smart-entry.cts:243, init.cts:1414, milestone.cts:551 and verify.cts:2467. All ten are untouched by this PR and base-identical. One of them sits in a file this PR DOES edit, so it is named rather than left to a reader's grep: verify.cts:2467, `verify schema-drift <phase>`. Measured on a bracket repo across base / pre-fix branch / this HEAD, all three agree on all three argument forms — `verify schema-drift GSD.02-01` and `… 01` both report "Phase directory not found" on every build, and `… GSD.02-01-one` resolves on every build through the exact-directory-name fallback. So the user-visible shape of what stays broken is: a bracket phase is addressable there by full directory name only, exactly as at base. Threading the convention into a function this PR never touched, in the last round before ship, is the wrong trade; it is where the same one-argument fix goes next, alongside milestone.cts:551 and init.cts:1414. - A BRACKET HEADING WHOSE TOKEN CARRIES A HYPHEN (`### [GSD.02] Phase 02-01:`, a mid-migration spelling) forms NO milestone-qualified key, and therefore scopes through the unqualified legacy path — base-equivalent ACCEPTANCE, which is the claim, and not a base-equivalent reading: `total_phases` on that shape moves 1 -> 2 for the same reason it moves on the canonical `### [GSD.02] 01:` spelling, because counting bracket headings is what this PR does. Such a token still flips `roadmapUsesHyphenedIds`, as it also does at base. The comment at the qualified-set declaration now claims only that narrower, true thing. The `pr:` field carries the sub-issue number as a placeholder — it must be updated to the real PR number when the PR is opened. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2761): point the changeset at PR #2867 * test(#2761): fast-check properties for the convention-selection layer CONTRIBUTING.md mandates a generative property test for parser/bijective-contract changes; PR-2 shipped six example-based files and none. This adds the missing layer, scoped to what PR-2 actually contracts — WHICH pattern each reader compiles, decided by the resolved `phase_id_convention` — rather than restating PR-1's grammar round-trip properties, which already live in tests/adr-612-bracket-grammar.test.cjs. Four properties: P1 an opted-in repo reads the ADR-canonical label-less bracket heading/dir and a non-opted-in repo is byte-blind to the identical input; P2 every non-bracket convention agrees with the hand-transcribed BASE source over generated content, including bracket-DOTTED legacy prose (`[RFC.2119] 5:`) that must never be claimed as a phase; P3 nine per-field mutations are rejected and the one case variation folds instead; P4 both sides of a phase comparison derive the same key under the same convention. Generators template every input from raw primitives — nothing is seeded through renderPhaseId/toDir, the p2() tautology that made #2258 round 1's property test structurally unable to find B1. Domain reaches past 99 into the 3+-digit branch (round 2's numArb-capped-at-99 miss), forces sub-phases in at weight, and pins both sentinel milestones. Falsified against the COMPILED lib, not the source: five deliberate mutants (gate never fires; gate always fires; milestone width widened to \d+; the #612 convention forwarding dropped from phaseKeyFromDir; extractPhaseToken's bracket branch ungated) each fail the specific property that should catch them — 16/2, 16/2, 15/3, 17/1, 17/1 pass/fail — and the lib restores byte-identical. An earlier draft of P2 held vacuously: its base regex omitted the markdown furniture the selected one carried, so every realistic `### Phase NN:` line matched neither side. The gate-always-fires mutant did not kill it. Both are now compiled through one function, and that mutant kills P2. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): adversarial malformed bracket tokens across the tolerant readers The existing boundary coverage stopped at shapes the emit grammar rejects (unpadded `[GSD.3]`, wrong-case, `12A`). It never exercised a STRUCTURALLY broken token — a non-numeric milestone, a bracket that never closes, a bracket nested in another — which is the input a tolerant reader is most likely to half-read, and the one the PR's own regex commentary is explicit about. read-tolerance (roadmap heading scan + validate's dir and variant builders): ten malformed headings, each asserted to be read as a phase by NO convention and to give the opted-in repo the same answer as the legacy one; the corpus driven through `roadmap analyze` end to end; malformed DIRECTORY names asserted unrecognized and non-throwing on all four conventions; and the two variant builders asserted to agree, since a widening that reaches only one splits `validate consistency` from `validate health` (the #3242 Bug B shape). coherence (verify.cts W021): the same six broken shapes asserted to raise no W021 of their own AND not to re-scope the W021 that follows them — the G2 failure mode reached from a different shape, where a heading that is not a phase but IS read as a section silently moves later warnings onto the wrong milestone. Both files gain a pathological-input time bound. Nested quantifiers over a long unclosed bracket are the classic ReDoS shape and two commits on next (#2828, #2944) were CodeQL-flagged for exactly that, so the bound is asserted rather than argued from reading the pattern. The probes themselves parse no regex. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2761): document "bracket" as a phase_id_convention value The row listed only `"milestone-prefixed"` and `null`, so after two shipped slices (#2258 grammar, this PR's read path) the convention had no documented enum value. CONFIGURATION.md is also a top-10 historical co-changer of both src/verify.cts and src/state.cts and was absent from this PR. The row states the boundary rather than the ambition: `"bracket"` changes the READ path only, there is no migrator and no emit yet, and a project on any other value compiles the patterns it compiled before. That keeps the docs honest for the two releases before PR-3 and PR-4 land, instead of describing a convention a user cannot yet migrate to. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2761): retype the changeset Added, drop the docs-exempt marker `Fixed` was wrong by CONTRIBUTING.md's own definition — a fix restores documented behavior, and bracket read tolerance is the second slice of a capability that did not exist before #2258. The type also carried a `docs-exempt` marker, and `Fixed`/`Security` are exempt from the docs-required lint, so the typing had the effect of routing around a gate this change should pass. It now passes it: `lint-docs-required` returns ok_docs_updated on the CONFIGURATION.md row added in the previous commit. Body gains one sentence pointing at that row and restating that `"bracket"` is a read-path opt-in until the migrator and write path land. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): pin the version-less bracket milestone scoping gap; narrow the claim The changeset asserted that milestone scoping "recognises the ADR-canonical `## [GSD.02] Foundation` heading and applies to the phase DIRECTORIES too." A CLI probe on that exact heading form falsifies the second half: with no `vN.N` in the milestone heading the directory side does not scope, and directories from BOTH the prior and the later milestone are admitted. Measured 4 dirs counted where the milestone declares 2. Every bracket fixture in the suite writes `## [GSD.02] v2.0: …`, so nothing covered the form the ADR actually specifies — and the state.cts doc comment calls that version-less form canonical. Mechanism, in extractCurrentMilestone: the bracket scope branch selects the right currentSection, but `preambleCutoff` keys off a pattern requiring a version or status emoji, so a version-less roadmap falls back to the current milestone's own offset and every PRIOR milestone lands in the preamble — whose phase-stripping regex only strips `Phase N:`-labelled headings, so bracket phase headings survive it. Independently, `computeSectionEnd` accepts a boundary only on a version/emoji heading, so the section runs to EOF and every LATER milestone is swept in. Two sites, bidirectional. Not fixed here: it changes milestone scoping, which is shared with the legacy path. Five characterization tests pin today's reading plus a versioned CONTROL proving the version string is the only difference, and the changeset sentence is narrowed to what the code does. The DEFECT assertions are written to be INVERTED by the fix, not deleted — that inversion is its regression proof. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2761): re-anchor the branch's own cross-file line citations after the rebase Three of this branch's code comments cite sibling call sites by line number, and the rebase onto 178ec000 moved two of the three targets: roadmap-parser.cts validate.cts:210 -> :218 (const g = capturing ? 1 : 0) state.cts:1715 -> :1752 (const bg = … 'bracket' ? 1 : 0) roadmap.cts verify.cts:2229 -> :2355 (phaseTokenMatches 3-arg form) `state.cts:1715` had drifted 37 lines and now lands on the retirement skip, not the capture-offset idiom the sentence is about — the citation read as evidence for a claim the cited line does not support. planning-workspace.cts's `config-loader.cts:618/:649` was checked and is still correct; left alone. Comment-only. Build, drift guard and the bracket suites re-run unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): scope the version-less bracket milestone heading too (B1) computeSectionEnd and the preambleCutoff scan in extractCurrentMilestone (roadmap-parser.cts) only recognized a milestone boundary heading that carried a vN.N token or a status emoji. The ADR-canonical bracket heading (## [GSD.02] Foundation) carries neither, so on that shape computeSectionEnd fell through to content.length (sweeping every LATER milestone into scope) and preambleCutoff fell back to the current milestone's own offset (leaking every PRIOR milestone's bracket phases into the preamble, whose Phase-N: strip regex never matches them). Under the bracket scope branch, both sites now also accept a `#{1,2}\s+\[CODE.MM\]` boundary, built from phase-id.cts's BRACKET_ID_SRC (single owner of the bracket-id grammar) rather than a re-typed literal. `#{1,2}` is the deliberate discriminator: a bracket PHASE heading is level 3 and shares the same `[CODE.MM]` prefix, so a `#{1,3}` boundary would swallow it too. Reachable only when bracketScopeConvention === 'bracket' was already resolved (i.e. the bracket scope branch actually fired), so version-bearing/emoji headings and non-bracket conventions take the exact pre-existing code path byte-identically — confirmed by the full adr-612 suite staying green. Inverts the four DEFECT assertions in the "#612 PR-2 CHARACTERIZATION: a version-less bracket milestone does not scope" describe block (tests/adr-612-bracket-phase-counting.test.cjs) into their regression-proof form, per the block's own doc comment, and reframes the describe title/comments accordingly. Corrects the .changeset/2761-bracket-read-tolerance.md fragment, which described the directory-side version-less gap as an open, un-closed bound. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): thread sentinelPhases into validate health's W006 loop (B2) cmdValidateHealth's W006 loop (src/verify.cts) destructured only roadmapPhases from buildRoadmapPhaseVariants, not sentinelPhases — unlike cmdValidateConsistency, which already skips sentinelPhases with the identical guard a few hundred lines up. A heading-only bracket icebox/pre-milestone entry ([GSD.999] / [GSD.00]) therefore gained a false W006 "no directory on disk" from validate health while validate consistency correctly stayed silent on the very same ROADMAP — the two validators contradicting each other. Threads sentinelPhases through and skips it before the existsOnDisk check, mirroring the consistency guard exactly. Gated the same way sentinelPhases already is (empty unless phase_id_convention is 'bracket'), so a legacy repo's W006 reading — including its own pre-existing wart where a legacy `### Phase 999:` still warns on both verbs — is untouched; confirmed by the existing "INHERITED WART, unchanged" test staying green. Adds the paired-agreement regression test (#612 PR-2 B2 describe block in tests/adr-612-bracket-read-tolerance.test.cjs): a sentinel-only bracket roadmap must produce no missing-directory warning from EITHER validator, plus a CONTROL proving a real phase with no directory still warns on both. Confirmed red (health false-W006) against the pre-fix code before applying the fix. Corrects the .changeset/2761-bracket-read-tolerance.md fragment, which described the asymmetry as already closed and in the wrong direction (it credited validate health with already staying silent, when health was the one falsely warning). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#2761): pin mixed-shape preamble cutoff and boundary heading levels Closes two self-flagged coverage gaps in the B1 fix (commit 08d5b0c4) ahead of adversarial review. No src change — all three new tests are green against the code as committed. 1. earliest-of-either preambleCutoff comparison: only exercised where the version/emoji match and the bracket match happen to land on the same heading. Adds the mid-migration mixed shape (version-bearing PRIOR + version-less CURRENT) and asserts scoping outcomes (accepts booleans + total_phases), not internals. 2. `h.level <= 2` conjunct in computeSectionEnd: provably redundant whenever the selected milestone heading is level 2 (every existing fixture), since `h.level > level` alone already implies it there — a mutant deleting the conjunct would have survived every prior test in this file. Adds a level-3 CURRENT-heading fixture (with a real PRIOR milestone so the preamble side-channel can't independently rescue the truncated phases) that makes the conjunct's deletion test-visible, confirmed by hand-mutating a throwaway copy of the compiled output (never touching tracked src or the real build) and observing the assertion flip. Also pins a level-1 companion case (#{1,2} tolerance, not just level 2). NOT included here: the other mixed-shape direction (version-less PRIOR + version-bearing CURRENT) turned out to be a genuine, currently-unfixed gap — reported separately rather than silently patched or weakened, per instruction not to touch src while a probe run is in flight. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): engage bracket boundaries when the current milestone heading is version-bearing (B3, self-caught) Found during round-2 self-verification of B1 (commit 08d5b0c4), while closing the mixed-heading-shape coverage gaps flagged in my own review notes. The B1 fix resolved `bracketScopeConvention` only inside the `if (headingMatches.length === 0)` gate that also drives SELECTION's own bracket fallback (which heading counts as "current"). That gate is correct for selection, but `bracketScopeConvention` also feeds computeSectionEnd's and preambleCutoff's boundary detection further down — which accidentally inherited selection's gate instead of having its own. Trigger shape: the CURRENT milestone heading is itself version-bearing (`## [GSD.02] v2.0: Current Milestone`), so the primary version-string match succeeds immediately — headingMatches.length !== 0 from the very first check — and the entire bracket-resolution branch was skipped. A sibling milestone (PRIOR or LATER) that is version-less then got neither the version/emoji boundary rule (it has none) nor the bracket boundary rule (never resolved), reproducing the original #612 defect (total_phases falling back to the whole-disk count) through a structural shape B1's own fixtures never exercised — every one of them is uniformly version-bearing or uniformly version-less across all three milestones, never mixed with CURRENT specifically being the version-bearing one. Fix: resolve `bracketScopeConvention` unconditionally, decoupled from `headingMatches.length`. SELECTION is deliberately left untouched — the `if (headingMatches.length === 0 && bracketScopeConvention === 'bracket')` fallback that picks which heading is "current" keeps its original gate byte-for-byte (confirmed by diff: that line is unmodified). Only the convention *resolution* moved out from behind it, so boundary detection can consult it regardless of which branch selected the heading. The extra `resolvePhaseIdConvention` call this now costs on every invocation (previously paid only when the version match found nothing) is the accepted cost: a non-bracket repo still resolves to something other than 'bracket' (or null on a poisoned env, caught exactly as before), so `bracketMilestoneHeadingRe` stays null and every downstream branch is byte-identical to today — confirmed by the full adr-612 + roadmap-parser + state + verify + health-validation suite staying green (1260/1260) and the all-version-bearing/legacy fixtures showing no behavior change. TDD: tests/adr-612-bracket-phase-counting.test.cjs describe block "#612 PR-2 B3: bracket boundaries engage even when CURRENT is version-bearing but a sibling is not" — 4 tests, confirmed red against pre-fix code (leak-in booleans true/true, total_phases 4) before this change, green after. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): reject same-milestone continuation headings as boundaries (B1) Gate-2 adversarial review Blocker 1: the B1/B3 boundary fired on ANY as the one currently selected — a version-less checklist/detail split (`## [GSD.02] Foundation (Phase Details)`, or an ad-hoc continuation heading) truncated the current milestone's own section instead of being recognised as a continuation of it. The `(Phase Details)` re-append only searches VERSION-STRING matches, so a version-less continuation heading was cut out and never re-appended — a confidently wrong, non-degraded phase count for a still-incomplete milestone (repro8 case 1: 1/1/100 instead of 2/1/50; repro5: same, on a fully version-less roadmap with no sibling milestones at all). Introduces one shared helper, isBracketMilestoneBoundary(headingText, level, selectedBracketId), used by both computeSectionEnd and the preambleCutoff bracket scan, replacing the ungated `h.level <= 2 && bracketMilestoneHeadingRe.test(...)` inline check. `selectedBracketId` (case-folded via phase-id.cts's foldBracketId, matching the branch's own fold-before-identity convention) is derived from `selected[0]`, which is the full matched heading line on BOTH selection paths (version-string and bracket-fallback), so one extraction covers both. Level cap stays at `level > 2` for now (temporary — ADR-612's content discriminator replaces it in the next commit); same-milestone rejection is the change this commit is scoped to. DEVIATION from the reviewed plan, caught empirically: applying the same-milestone rejection at the preambleCutoff site (as literally specified) regressed an existing pin ("boundary heading level: a level-1 CURRENT milestone heading also scopes correctly") and a fenced-heading case (repro10 A3) — because preambleCutoff's job is "where does the earliest milestone-shaped heading sit, scanning from the TOP of the document," and the selected heading's own occurrence is always a correct answer to that question regardless of same-id-ness; rejecting it let the earliest-of-either comparison fall through to a stray LATER heading instead. `selectedBracketId` is threaded through as `null` at the preambleCutoff call site for this reason — bracket-shaped (and, from the next commit, phase-tail) discrimination still applies uniformly at both sites; only the same-milestone component is call-site-specific, since it encodes a "keep scanning past this heading" instruction with no counterpart in a top-of-document search. Tests: new describe block "#612 PR-2 B1 round-2: a same-milestone continuation heading is not a boundary" — RED-turned-GREEN fixtures for repro8 case 1 and repro5, plus PINs for repro8 case 3 (trailing different-id icebox still terminates) and repro10 A1 (all-version- bearing + icebox + Phase Details stays exactly 2/1/50 — no double-count from the same-milestone exclusion interacting with the pre-existing detailsMatch re-append). syncedTotal()/syncedPercent() assertions omitted from the repro10 A1 pin: that fixture carries dirs outside the current milestone, which exposes the SEPARATE Major 1 defect (cmdStateSync's body percent from an unfiltered disk scan) — asserted once Major 1 is fixed, not here. Full suite green (796/796 across the targeted adr-612 + roadmap-parser + state files); node scripts/lint-phase-id-drift.cjs clean; eslint clean on both changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): bracket boundary discriminates by content, not heading level (B2) Gate-2 adversarial review Blocker 2: three sites disagreed about which heading levels are a bracket milestone. The selector (roadmap-parser.cts's bracket-fallback SELECTION branch, `^#{1,3}\s+\[CODE.MM\]`) and `isMilestoneBounded` (state.cts) both admit level 1-3, but isBracketMilestoneBoundary's level cap only admitted level 1-2 (`h.level <= 2`, from the B1 commit). A `###`-level bracket milestone heading was therefore SELECTED and BOUNDED but never TERMINATED: computeSectionEnd ran with level=3, a level-3 SIBLING milestone survived the pre-existing `h.level > level` (not-deeper) filter, failed the version/emoji test (version-less), then failed `h.level <= 2` — falling through to `return content.length` and sweeping the sibling milestone's own phases into the current one. Reproduces trek-e's original #612 defect verbatim ("a safe degrade became a confidently-wrong persisted number") on a heading level the selector and bounding predicate both already admit (repro2 case C: 4/75% instead of 2/100%; mechanism confirmed directly via repro7 — extractCurrentMilestone returned the whole 214-byte document). ADR-612 Decision 1 (docs/adr/612-bracket-phase-id-convention.md:56) specifies the discriminator as CONTENT, not level: "a phase heading is a bracket followed by a digit-then-colon ([GSD.02] 05:); a milestone heading is a bracket followed by a name." Replaces the `level > 2` rejection with BRACKET_PHASE_TAIL_RE — built by interpolating phase-id.cts's single-owner phaseHeadingPrefixSrcFor(ANY_BRACKET, 'bracket', false) plus the digit + optional-tag + colon tail every phase-heading counter in this file already spells, not a re-typed grammar — and widens the level check to a depth-sanity cap of 3 (mirroring the selector's own `#{1,3}` ceiling; NOT itself a phase/milestone discriminator). Covers the dotted sub-phase heading form (`[GSD.02] 05.03:`) via the same `[\w][\w.-]*` token, pinned by a new fixture — the shape where a regex slip in the tail grammar would hide. preambleCutoff's own raw-scan regex is widened from `^(#{1,2})` to `^(#{1,3})` in lockstep: the outer pattern's level ceiling must track the helper's cap, or a level-3 PRIOR milestone heading is invisible to that scan and its own phase heading leaks into the preamble un-stripped (a real double-count this widening closes, verified against repro2 case C directly). The existing "boundary heading level: a level-3 CURRENT milestone heading still scopes correctly" pin (3e562f12) now passes via a DIFFERENT mechanism than before — its own neighbours are version- bearing, so it previously passed via the version/emoji rule (the level cap was never actually exercised by that fixture, per the round-2 review's own finding); with the content discriminator, the SAME fixture's level-3 phase headings are now correctly excluded because they are phase-tail-shaped, not because they are too deep. A deliberate mechanism change, confirmed by re-running that test green after this commit. Also updates the "every selector call site declares the right baseline" governance pin (adr-612-bracket-heading-selection.test.cjs): BRACKET_PHASE_TAIL_RE is a new, legitimate ANY_BRACKET call site in roadmap-parser.cts (always passing the literal 'bracket' convention, since its only caller is already gated on bracketBoundaryActive) — EXPECTED count bumped 1->2, with a matching BASE_SITES transcription entry (identical src to every other ANY_BRACKET site, since the function is pure). Tests: new describe block "#612 PR-2 B2 round-2: the bracket boundary is a CONTENT discriminator, not a level cap" — RED-turned-GREEN for repro2 case C (exact total AND truthful percent, since isMilestoneBounded already returns true at #{1,3}) and repro7's mechanism, a PIN for the dotted sub-phase form, and a re-pin of repro8 case 3 (icebox) under the new mechanism. Full suite green (907/907 across the targeted adr-612 + roadmap-parser + state + phase-id files); node scripts/lint-phase-id-drift.cjs clean; eslint clean on all changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): fence-aware preamble cutoff on the bracket branch (Blocker 3) Gate-2 adversarial review Blocker 3: preambleCutoff's bracket scan used a raw content.match/matchAll — blind to fenced code blocks — while its sibling computeSectionEnd (a few lines above it) already consumed tokenizeHeadings(content), which strips fences. The two halves of one boundary semantic disagreed about what a heading is. A fenced markdown example in the preamble containing a bracket heading (ADR-612's own docs do exactly this) was textually the earliest `#{1,3} [CODE.MM]` match: preambleCutoff landed INSIDE the fence, `preamble = content.slice(0, preambleCutoff)` ended with an unclosed opener, and the unbalanced fence then blinded getMilestonePhaseFilter's own tokenizeHeadings(scope) call — every heading in the returned scope vanished, phaseCount degraded to 0, and the pass-all filter admitted every directory on disk (repro11's mechanism, confirmed directly: fence count 1/odd, tokenizeHeadings(scope) -> only "Roadmap"). Regression vs round-1, which had no bracket pattern to blind and so fell back to the correct heading (repro12 bracket row: 2/1/50 at round-1, 4/3/75 at HEAD). Fixed by hoisting one tokenizeHeadings(content) call (currentMilestoneHeadings) shared by computeSectionEnd and the preambleCutoff scan, which now iterates that same fence-aware token list instead of a raw regex. HeadingToken.text is already hash-stripped and trimmed, so isBracketMilestoneBoundary needs no `^#{1,3}\s+` re-derivation at this site (that spelling would not match h.text — a note the round-2 review called out explicitly, confirmed while porting). selectedBracketId stays `null` here, unchanged from the B1 commit's same-milestone-exclusion reasoning. DISCLOSED, not fixed (explicitly out of scope per the round-2 review's own minimal-fix note): the LEGACY (non-bracket) anyMilestonePattern raw-match path shares the identical fence-blindness hazard and stays byte-identical — a bracket repo whose preamble has a fenced VERSION-BEARING heading still has the legacy raw-match win the earliest-of-either min() (repro12's LEGACY control: 4/3/75, unchanged across base/round-1/HEAD/this commit). Pinned here so a future reviewer files this as a known, pre-existing gap rather than a new regression. Tests: new describe block "#612 PR-2 Blocker 3 round-2: preambleCutoff is fence-aware (bracket branch only)" — RED-turned-GREEN for repro12's bracket row and repro11's mechanism (fence balance + non-degraded phaseCount + correct per-directory admission), a PIN for repro12's LEGACY control (the disclosed gap, explicitly unchanged), and a PIN for repro10 A3 (a fenced heading INSIDE the current section must still not terminate it). Full suite green (1072/1072 across the targeted adr-612 + roadmap- parser + state + phase-id + markdown-sectionizer files); node scripts/lint-phase-id-drift.cjs clean; eslint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): scope cmdStateSync's disk scan by milestone under bracket (Major 1) Gate-2 adversarial review Major 1: `state sync` wrote a Progress PERCENT computed from an UNFILTERED whole-disk scan, beside the milestone-scoped total_phases/completed_phases it writes into the same STATE.md via the refreshed frontmatter (syncStateFrontmatter -> buildStateFrontmatter, which has always applied getMilestonePhaseFilter for the READ path). cmdStateSync's own `fs.readdirSync` chain (the WRITE-path scan) never called the milestone filter at all, unlike buildStateFrontmatter's identical-purpose scan. One command therefore wrote two contradictory numbers into one file: on the ADR-canonical version-less bracket fixture (4 dirs, 3 complete; asserted milestone = 2 phases, both complete), base wrote total_phases:2/completed_phases:2 (correct, from the READ derivation) alongside body Progress 75% (wrong — from the unfiltered WRITE derivation; repro3). Fixed by threading `getMilestonePhaseFilter(cwd)` through the same `.filter()` chain buildStateFrontmatter already applies, gated on `syncConvention === 'bracket'` (falling back to a pass-all predicate otherwise) — so totalDiskPlans/totalDiskSummaries/diskCompletedPhases/ syncTotalPhases become milestone-scoped under bracket, byte-identical under legacy. DEVIATION (approved, stated plainly): an earlier phrasing of this fix called for mirroring buildStateFrontmatter's filter UNCONDITIONALLY. Implemented GATED instead — an unconditional filter would ALSO move every LEGACY repo's persisted percent, since the milestone-scoping-vs- whole-disk divergence this closes is engine-wide, not bracket-specific. The gate keeps legacy byte-identical, which is the binding constraint: this is a bracket read-path PR, not a legacy behavior change. Nit 2 (informational, no code change): 10 calls to extractCurrentMilestone on a legacy repo cost 10 config.json existsSync + 10 readFileSync (0 before B3); accepted, unmemoized cost, unaffected by this commit. Also folds in two minors from the round-2 review: - Corrects .changeset/2761-bracket-read-tolerance.md: the sibling- exclusion sentence now states it holds at any heading level 1-3 and across a milestone split over two headings (true again now that Blockers 1 and 2 are fixed); the percent sentence states plainly that `state sync`'s body percent is now milestone-scoped under bracket, and unaffected under legacy. - Records the read/write scoping divergence at currentMilestoneRawRanges (src/roadmap-parser.cts) in a comment: it did not receive B1/B2's bracket boundary fixes, currently harmless (its only consumer falls back to whole-content mutation, and every mutation there is still Phase-labelled-only, not bracket-widened), but live the moment the write path is bracket-widened — flagged so a future PR closes it in lockstep with that work, not after. Tests: 6 pre-existing tests in tests/adr-612-bracket-phase-counting.test.cjs needed fixture updates, not logic changes — they used the default single directory (`GSD.02-01-setup`, phase "01"), which the SENTINEL/ retirement/mixed-heading fixtures in those tests never declare as a real phase (only 04/05/06/999/etc are declared). Before this fix, cmdStateSync's unfiltered scan counted that off-roadmap directory anyway; after this fix the milestone filter correctly excludes it, which for several of these fixtures made `state sync` a no-op (the computed 0% coincided with STATE.md's initial template default) and broke `syncedTotal()`/`syncedPercent()`'s ability to observe anything. Updated each to pass an EXPLICIT directory naming one of the fixture's REAL declared phases, preserving each test's original numerator/ denominator intent. One test — "shape 2 WRITE" — was substantively rewritten: it was a CHARACTERIZATION of the Major 1 bug itself ("the DISCLOSED legacy gap, mirrored — not closed"), and now correctly pins bracket closing to 33% (agrees with the read path) while legacy stays at the disclosed 60% (unchanged, deliberately, per the gating decision above). Full suite green: `npm test` 1449/1449 (0 fail, 0 skipped, 0 todo, single-shard "all" run — includes issue-2765-brace-expansion-lockfile passing); `npm run lint:ci` clean (0 errors; 2 pre-existing timing- assertion warnings in files this PR does not touch); node scripts/lint-phase-id-drift.cjs clean; node scripts/changeset/lint.cjs ok. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): preambleCutoff identity is offset- and child-aware (round-3 Blocker 1) Gate-2 round-3 re-verify Blocker 1 (NEW): the round-2 B1 deviation (39c42a89) threaded `selectedBracketId` as the real value at computeSectionEnd but as bare `null` at the preambleCutoff scan. The deviation's rationale — "the selected heading's own occurrence is always a correct earliest answer" — was right, but `null` disables the same-milestone check for EVERY candidate, not just the selected one. Any bracket-shaped heading earlier than the selected milestone was accepted as a boundary regardless of identity: a same-id checklist/ overview heading preceding the version-bearing selected heading (cases A, B — the version lands on the LATER half of a split, or a plain overview heading with no "(Phase Details)" spelling), or a DIFFERENT-id bracket-shaped PROSE heading with no children of its own sitting above the current milestone's content (case D — `## [ADR.612] Heading convention used by this roadmap`). In every case the region between that false boundary and the real sectionStart was silently dropped — a completed phase vanished and `state sync` persisted a confident 0% where base and round-1 both correctly wrote 50%. Regression vs base AND round-1 (not merely "under-fixed", per the round-3 review's own severity note). Fixed with two changes, both scoped to the preambleCutoff scan only (computeSectionEnd already threads the real `selectedBracketId` and is untouched): (a) `h.offset === sectionStart` now bypasses BOTH the same-milestone check inside isBracketMilestoneBoundary (passing the REAL `selectedBracketId` for every other candidate) and the new child rule below — the selected heading's own position is definitionally the correct answer, so neither discriminator should run against it (rejecting it would mean rejecting the heading against ITSELF). Closes cases A and B — verified by the reviewer's own one-liner, reproduced here. (b) New `bracketHeadingHasMatchingChild`: an otherwise-accepted candidate (bracket-shaped, not phase-tail-shaped, not the same id as the selected milestone) must ALSO have a next-strictly-deeper heading carrying its OWN bracket id to count as a boundary. This is what a genuine sibling milestone has (its own phase children share its bracket id — `## [GSD.01] Setup` / `### [GSD.01] 01: …`) and an unrelated bracket-shaped prose heading does not. A candidate with no such child at all (childless — e.g. an empty prior milestone, or one immediately followed by a same-or-shallower heading) degrades to NOT a boundary — over-inclusive, the safe direction: its own heading text stays in the preamble, contributing nothing to any phase count (not phase-shaped). Closes case D, which (a) alone does not — verified: without this rule, `[ADR.612]`'s prose heading is indistinguishable from a genuine prior sibling at this site. As a side effect, also neutralizes Nit 2 (a colon-less `[GSD.02] 05` heading spuriously terminating the preamble): a colon-less bracket heading is not phase-tail-shaped so isBracketMilestoneBoundary alone would accept it, but it is — precisely because it is malformed/ incomplete rather than a real milestone — childless, so the child rule rejects it too. Pinned. Known interaction with the fence-blind SELECTION path (disclosed by the reviewer, not introduced here, tracked for the next commit): when `sectionPattern` selects a FENCED version-bearing heading (an extremely pathological shape — a fenced example whose text happens to match STATE's asserted version), no token exists at `sectionStart`, so the `h.offset === sectionStart` bypass never fires and the loop falls through to the ordinary same-id / child-rule checks. This composes with the round-3 Major 1 fix (next commit) rather than introducing a new defect — SELECTION itself is untouched by any of this — but is worth stating plainly rather than rediscovering. Tests: new describe block "#612 PR-2 Blocker 1 round-3: preambleCutoff identity is offset- and child-aware" — RED-turned-GREEN for cases A, B (rv-attack1) and D (rv-attack1b) with syncedTotal()/syncedPercent() assertions (the persisted 0% is the point), a PIN for a genuine prior sibling with real children (still excluded), a PIN for a childless prior sibling (degrades to not-cutting, over-inclusive/safe), a PIN for the colon-less Nit 2 shape, and the reviewer's rv-mech1 mechanism re-run as a proper test (scope now equals the full input document, phaseCount 2, both dirs accepted). Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 937/937 pass (930 baseline + 7 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean on both changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): fence-aware version/emoji half of preambleCutoff on the bracket branch (round-3 Major 1) Gate-2 round-3 re-verify Major 1 (NEW): ff6bf0a8 (round-2 Blocker 3) made the BRACKET half of preambleCutoff's "earliest milestone-shaped heading" search fence-aware, but left the VERSION/emoji half a raw `content.match` even on the bracket branch. A fenced VERSION-BEARING example heading in a bracket repo's preamble (ADR-612's own docs illustrate the LEGACY heading shape exactly this way, inside a fenced authoring-guide block) was still textually the earliest match for that raw regex, winning the min() and un-suppressing a wrong persisted 75% that base correctly suppressed (rv-attack3c fixture C1: base suppressed the percent entirely — `isMilestoneBounded` false — HEAD wrote 75% where truth is 50%). Fixed by deriving the version/emoji half from the SAME fence-aware `currentMilestoneHeadings` token list as the bracket half, on the bracket branch only — the exact `/^Phase\s+\S/i` / `/v\d+\.\d+|✅|📋|🚧/i` pair `computeSectionEnd` already uses against `h.text`. The non-bracket (legacy) path is untouched: it keeps the raw `content.match`, byte- identical to before, including its own fence-blindness (repro12's LEGACY control, pinned unchanged in the round-2 Blocker-3 test block — not re-pinned here to avoid duplicating an already-covered assertion). Not rated Blocker (per the review) because it is not a regression vs round-1 and the fixture (a version-BEARING fenced example in a bracket repo) is rarer than the already-fixed bracket-heading case; still fixed now rather than disclosed, per this arc's own precedent (every prior "disclose instead of fix" call in this PR has been overturned on re-review). Tests: new describe block "#612 PR-2 Major 1 round-3: preambleCutoff's version/emoji half is fence-aware on the bracket branch" — RED-turned- GREEN for case C1 (readTotal + syncedPercent, so the persisted 75% is directly observed, not just the read-path total), PIN for case C2 (the already-fixed fenced-bracket-heading shape, unchanged). Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 939/939 pass (937 + 2 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean on both changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): correct changeset claims + mark runtime-gated BASE_SITES row (round-3 minors) Gate-2 round-3 re-verify Minor 2: three changeset sentences in .changeset/2761-bracket-read-tolerance.md were overstated in a new direction after round-2: - The split-milestone claim ("across a milestone split over two headings") was true only when the version-bearing heading came FIRST (repro8 case 1); false when it came LATER (round-3 Blocker 1 case A). Now restated to say plainly "with the version-bearing heading in EITHER position" — true again now that round-3's Blocker 1 fix lands earlier in this range. - The "counted from the phases... rather than from every directory on disk" claim was false on cases A/B/D (a strict subset of the milestone's own phases). Restated as "ALL of the phases... not a subset", and extended to state that an unrelated bracket-shaped heading with no phase children of its own (case D's `[ADR.612]` shape) does not truncate the milestone either — true now, not before. - "Each widened read is SELECTED by the project's phase_id_convention" was literally false for BRACKET_PHASE_TAIL_RE, which is RUNTIME-gated (via its only caller, isBracketMilestoneBoundary, itself only consulted when bracketBoundaryActive) rather than selector-gated. Restated behaviourally: "every widened read ENGAGES only when the project's resolved phase_id_convention is bracket" — true for both gating mechanisms, so it no longer implies a selector call this site does not make. Minor 1: the STRUCTURAL IDENTITY test's BASE_SITES row for BRACKET_PHASE_TAIL_RE (added in the B2 commit) asserts a property of `phaseHeadingPrefixSrcFor` — the function — not of the call site; it would pass unchanged even if the site were deleted. Safety at that specific site rests entirely on a runtime gate the test cannot see. Added `runtimeGated: true` to the row and threaded it into the generated test's own title (`… [runtime-gated, not selector-covered]`), so the gating mechanism is visible in test OUTPUT, not only in a source comment that could drift silently. No production code changed. Targeted suite green (48/48 in the affected file); `node scripts/changeset/lint.cjs` ok; eslint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): harden round-3 preamble cutoff — subtree child scan + level cap Team-lead review of f87bba0e found two edges in the round-3 preamble- cutoff code, both hardened here. AMENDMENT 1 — bracketHeadingHasMatchingChild (2e06aef5) checked only `headings[index + 1]`, the IMMEDIATE next heading, not the candidate's whole subtree. A genuine prior sibling milestone whose section opens with a non-bracket subsection before its first phase heading (`## [GSD.01] Setup` / `### Notes` / `### [GSD.01] 01: Old`) was therefore wrongly rejected as a boundary — its real phase heading sits TWO headings deep, not one — leaking its entire section into the preamble unstripped. CONFIRMED RED, not merely theoretical (built and ran the fixture against f87bba0e before touching the fix, per instruction): scope membership DOES drive the disk-side filter on this shape. `GSD.01-01-old`'s directory was wrongly admitted into the CURRENT milestone's filter via the leaked heading's qualified key (`GSD.01-01`) — 3/2/67% where truth is 2/1/50%. Fixed by scanning the candidate's full SUBTREE: continue past a non-matching deeper heading instead of returning false on the first one; only a same-or-shallower heading actually closes the subtree and yields "no match found". A candidate whose entire subtree closes with no same-id hit (including a genuinely childless one) still degrades to `false` — over-inclusive, safe, unchanged from before. AMENDMENT 2 — c483552a ported the version/emoji half of preambleCutoff to the token-based scan with no level cap; the raw `content.match(anyMilestonePattern)` it replaced was anchored `^#{1,3}\s+`. A level-4+ version-bearing heading in the preamble (`#### v2.0 notes`) therefore won the scan on the bracket branch where the raw pattern — and the legacy path, unaffected — ignores it outright. Fixed with `if (h.level > 3) continue;`, mirroring the depth-sanity cap isBracketMilestoneBoundary already applies to the bracket half of this same scan. Tests: new describe block "#612 PR-2 round-3 hardening: subtree child scan + level cap on preambleCutoff" — - RED-turned-GREEN for the Notes-intervening fixture: exact 2/1/50 (was 3/2/67), plus the disk-filter observable (`GSD.01-01-old` now correctly excluded). - PIN for the level-4 preamble heading: the scope now PRESERVES the heading's text (was silently dropped before this fix — harmless in this minimal fixture's total_phases specifically, since the dropped text carries no phase-shaped content, but a real correctness gap against the raw pattern's own ceiling) — asserted via scope content, not total_phases, since that number is invariant here either way. - PIN for the LEGACY control on the same level-4 shape — unchanged, confirming the raw content.match path is untouched. Re-verified the existing genuine-prior-sibling and childless-sibling pins (round-3 Blocker 1 commit) still pass under the subtree scan — both fixtures' outcomes are unchanged since their same-id hit (or its absence) was already at the first deeper heading. Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 942/942 pass (939 + 3 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean on both changed files. Full `npm test` + `npm run lint:ci` deferred to the team lead's own run per instruction. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): bracketHeadingHasMatchingChild requires a same-id PHASE child (round-4 Blocker 1) Gate-2 round-4 re-verify Blocker 1 (NEW): the round-3 hardening's subtree scan (fbfd0fca) proved SAME-ID-NESS but never asked whether the matching child was PHASE-shaped. Case F1 re-opens round-3's case D one heading later: `## [ADR.612] Heading convention` is followed by its OWN sub-heading `### [ADR.612] Examples` — same bracket id as the candidate, but MILESTONE-shaped (a name, no digit-then-colon), not a phase. Same-id-ness alone satisfied the subtree scan and re-cut the preamble at exactly the shape the round-3 hardening was written to close. Failing input: `## [ADR.612] Heading convention` / `### [ADR.612] Examples` (prose) / `### [GSD.02] 01: One` (the current milestone's own first phase, now unreachable) / `## [GSD.02] v2.0: Foundation` / `### [GSD.02] 02: Two`. Truth 2/1/50. HEAD read 1/0/0, and `state sync` reported "nothing to do" (exit 0, `{synced:true,changes:[]}`) because its wrong 0% happened to equal the STATE.md seed — a half-done milestone read as untouched with no write-path signal at all. Fixed with the reviewer's one-conjunct addition: a same-id child only counts if it is ALSO phase-tail-shaped (`BRACKET_PHASE_TAIL_RE`) — the same single-owner discriminator `isBracketMilestoneBoundary` already uses one level up for the identical distinction (phase vs milestone), reused here rather than re-derived. This is exactly what the changeset's own wording already claimed ("no phase children of its own") — the code now matches the sentence rather than the other way around. Docstring updated at the function itself: the rule is "same-id PHASE child", not "same-id child". Tests: new describe block "#612 PR-2 round-4 Blocker 1: the same-id child must be PHASE-shaped" — RED-turned-GREEN for F1 with syncedTotal()/syncedPercent() (the persisted 0% — and the report-nothing-to-do write-path silence — is the point), PINs for F11 (colon-less same-id child) and F11b (bullet-only phase list): both correctly stay excluded either way, and the leak the phase-shape requirement newly creates for these two shapes is INERT — a colon-less heading forms no qualified key (getMilestonePhaseFilter's own phase-heading pattern requires the colon too) and a bracket bullet never matches the legacy-only BULLET_PHASE_LINE_PATTERN — confirmed directly via getMilestonePhaseFilter, not merely inferred. Re-verified the four existing child-rule pins (F2 subtree-closure, F3 deep-nested same-id, F4 level-4 same-id, F5 childless-at-EOF) are unaffected by the phase-shape requirement, since every one of them already used a colon-bearing same-id child. Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 946/946 pass (942 + 4 new). Zero drift measured across the full F1-F12 corpus except F1 itself (F9/F10/F12 remain red, deferred to the separate Major 1 fix). node scripts/lint-phase-id-drift.cjs clean; eslint clean on both changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): fence-aware phase counting, milestone bounding, and bracket-fallback selection (round-4 Major 1) Gate-2 round-4 re-verify Major 1 (NEW): roadmapPhaseCount is a fence-blind raw `.exec()` over the scope string, duplicated in TWO independent copies (buildStateFrontmatter's read path, cmdStateSync's write path). With the bracket alternative now compiled into it (#612), a fenced EXAMPLE phase heading in the preamble inflates total_phases and persists a wrong percent that base got right. Two further fence-blind sites participate: isMilestoneBounded (a raw `.test(roadmapRaw)`) and the bracket-fallback SELECTOR inside extractCurrentMilestone (a raw `content.matchAll`, only reachable when version-string selection finds nothing). Failing inputs: - F10 (clean isolate, version-bearing selection): a fenced `### [GSD.02] 05: Example phase` in the preamble inflates total_phases 2->3, persisting 33% where truth is 50%. LEGACY control on the same shape is correct on every build — not a pre-existing hazard being inherited, bracket-only. - F9 (version-less selection): a fenced example carrying the project's OWN milestone id additionally confuses the bracket-fallback selector (the fenced heading gets SELECTED), compounding with the same fence-blind counter. base suppressed the percent; round-1 and HEAD both wrote 33%. - F12 (isMilestoneBounded isolate): the ONLY `[GSD.02]` heading in the document is inside a fence, and the asserted milestone genuinely has no section at all — HEAD persisted 67% where base correctly suppressed the percent (the milestone is absent from the roadmap). Fixed at the CONSUMER level, not the producer — extractCurrentMilestone's returned scope string is deliberately UNCHANGED, since every other consumer of that string needs its full content fidelity and legacy identity forbids touching the shared string (this branch's own precedent, ff6bf0a8/c483552a, was producer-level; here the ruling is consumer-level because the string is shared far more broadly than the two round-3 fixes' narrower producer edits): (a) New `countRoadmapPhaseHeadings` (src/state.cts, immediately above extractRetiredPhaseNumbers) — ONE shared implementation for both call sites, replacing two independently-maintained copies. BRACKET convention counts via `tokenizeHeadings(scope)` at levels 2-4, testing each heading's hash-stripped text directly — fence-aware by construction, since tokenizeHeadings never produces a token for a fenced line. LEGACY convention keeps the exact pre-existing raw `.exec()` loop, byte-for-byte. A pre-existing, deliberately PRESERVED asymmetry between the two original call sites — the read path always excluded a bare `/^999\b/` token, the write path never did — is threaded through as an explicit `includeUnconditional999Check` parameter per call site, so sharing the implementation does not silently unify (and thereby move) either total. (b) isMilestoneBounded's bracket branch now scans `tokenizeHeadings(roadmapRaw)` for a matching heading (level <= 3) instead of a raw regex test. Legacy version-string branch untouched. (c) The bracket-fallback SELECTOR now builds its candidate set from `tokenizeHeadings(content)` instead of `content.matchAll`, reconstructing a match-shaped array so every downstream consumer of `headingMatches` sees the identical shape the raw-regex path always produced. This is the ONE site in this entire arc where SELECTION itself changes — selection SEMANTICS are otherwise unchanged (same pattern, same first-match-wins by document order); only the candidate set is now fence-aware. Pinned that unfenced selection is byte-identical. Zero drift measured across the full historical corpus (repro2-13, rv-attack1/1b/3c, rv-mech1, rv2-amend1/2, and F1-F11b) except the three target fixtures. Tests: new describe block "#612 PR-2 round-4 Major 1: four fence-blind sites on the bracket path" — RED-turned-GREEN for F10 (with syncedTotal()/syncedPercent()) and F9 (both layers), PINs for F10's LEGACY control and F10c (non-phase-shaped fence, unaffected either way), RED-turned-GREEN for F12 (asserts the percent KEY is absent from `state json`'s output and that `state sync`'s body stays at its unmodified seed — the persisted-suppression signal, not merely a total_phases number), and an explicit PIN that unfenced bracket-fallback selection (first real milestone-shaped heading wins, no fences involved) is unaffected. Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 952/952 pass (946 + 6 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean on all three changed files. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): correct docstring overstatement + stale consumer-count sentence (round-4 minors) Gate-2 round-4 re-verify Minor 1 + Nit 1. No production code changed. Minor 1: bracketHeadingHasMatchingChild's own docstring said a rejected (no-same-id-PHASE-child) candidate's degrade "contributes nothing to any phase count" — true of the candidate's OWN heading text, but not of its SUBTREE, which is what actually stays in the preamble. F7 (`## [GSD.01] Setup` / `### [GSD.07] 01: Foreign`) shows a DIFFERENT-id bracket PHASE heading inside a rejected candidate's subtree DOES form a qualified key and CAN admit a foreign directory — 3/2/67%, stable across base, round-1 and HEAD (base via its own pass-all degrade). Not a regression, still the declared over-inclusive / never-under-inclusive safe direction — the comment now says that, with F7's numbers cited, at the call site that actually decides `isBoundary` (roadmap-parser.cts's preambleCutoff loop) rather than only at the helper's own definition. Minor 2 (changeset) — VERIFIED, no wording change needed: re-ran F1, F9, F10, F12 at this HEAD. The "no phase children of its own... does not truncate it either" sentence (naming the `[ADR.612]` shape directly) is now literally true — F1 reads 2/1/50. The "counted from ALL of the phases... not a subset" sentence is now true on every measured shape — F1/F9/F10 all read 2/1/50, F12 correctly suppresses the percent. No carve-out for F9/F10 is needed since round-4 Major 1 (3be5c412) closes both; per the fix-round instruction to "only carve out anything genuinely left," nothing is. Nit 1: tests/adr-612-bracket-heading-selection.test.cjs's runtimeGated row claimed `BRACKET_HEADING_INTRO_RE` has "no other consumers" — true when round-3's f87bba0e wrote it, stale since 2e06aef5 (round-3's own earlier commit) had already added two more uses inside bracketHeadingHasMatchingChild. Corrected to state the true count (three consumers) and re-confirm the conclusion is unaffected: all three are still nested inside the same bracketBoundaryActive runtime gate, and BRACKET_HEADING_INTRO_RE is built from BRACKET_ID_SRC, not phaseHeadingPrefixSrcFor, so it was never a selector site regardless of consumer count. Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 952/952 pass (comment-only changes, no count movement). eslint clean; node scripts/changeset/lint.cjs ok. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): restore the bracketId guard in countRoadmapPhaseHeadings (round-5 Blocker 1) Gate-2 round-5 re-verify Blocker 1 (NEW, introduced by 3be5c412): the merge that created the shared countRoadmapPhaseHeadings helper dropped the `bracketId && ` guard both original inline loops carried before calling isSentinelPhaseId. Every other isSentinelPhaseId call site in src/ (roadmap-parser.cts, roadmap.cts, validate.cts x2, verify.cts) keeps the guard; state.cts's shared counter was the only one of seven without it. When the phase-heading-intro grammar's LEGACY alternative matches (a `### Phase 00:` heading in a `phase_id_convention: "bracket"` repo — the mid-migration shape this PR exists for), the bracket capture group is `undefined`, so the unguarded call became `isSentinelPhaseId("undefined-00", 'bracket')` — measured TRUE, so phase 00 (and 000, 0a, 0.5, 999.1 — any token whose splice with the literal string "undefined" happens to fall in a sentinel range) was silently dropped from the denominator. `getMilestonePhaseFilter` (which still carries its own guard) counts the phase and admits its directory regardless, so the filter and the counter disagree — a half-done milestone reads as 100% complete, persisted. One-line fix, restoring the guard every sibling call site already has: if (bracketId && isSentinelPhaseId(`${bracketId}-${token}`, 'bracket')) continue; Line count: `git diff --stat src/state.cts` -> 1 file changed, 1 insertion(+), 1 deletion(-). Tests: new describe block "#612 PR-2 round-5 Blocker 1: countRoadmapPhaseHeadings restores the bracketId guard" — RED-turned- GREEN for G3 (3/2/67, was 2/2/100) and G3d (the mixed bracket+legacy mid-migration shape, same numbers) with syncedTotal()/syncedPercent(), PIN for G3's legacy control (unaffected), PIN for G3b (isolates the counter with no directory to admit), PIN for G3c (legacy 01/02 only, no sentinel-shaped token present). Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 957/957 pass (952 + 5 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): extractRetiredPhaseNumbers is fence-aware on the bracket path (round-5 Major 1) Gate-2 round-5 re-verify Major 1 (NEW) — a FIFTH fence-blind site on the bracket path, missed by 3be5c412's own enumeration of "four". extractRetiredPhaseNumbers' line scan (`scope.split(/\r?\n/)`) has no fence awareness. This PR compiles the bracket alternative into `introSrc` ("the retirement filter has to widen with the counter it protects" — the function's own pre-existing comment), so a FENCED authoring EXAMPLE showing the #1514 retirement gesture in bracket spelling is now indistinguishable from a real one: it retires a genuine phase, shrinking the denominator and persisting a confident 100% where base correctly read 50%. Fixed with the SAME consumer-level ruling this arc has used at every other fence-blind site, reusing markdown-sectionizer's existing exported `stripFencedCode` rather than hand-rolling a second fence parser (single-owner rule) — retirement lines are BULLETS, not headings, so `tokenizeHeadings` doesn't serve here; `stripFencedCode` is the general-purpose fence stripper the tokenizer itself is built on. Gated on `convention === 'bracket'`; the LEGACY line scan stays the raw `scope` string, byte-identical — its own fenced-example hazard is pre-existing (wrong at base too) and out of scope. Line count: `git diff --stat src/state.cts` -> 1 file changed, 9 insertions(+), 2 deletions(-) — one import added, four lines inside the function (a comment + the `scanScope` computation + the changed `.split()` call). Tests: new describe block "#612 PR-2 round-5 Major 1: extractRetiredPhaseNumbers is fence-aware on the bracket path" — RED-turned-GREEN for G2 (fenced example in the preamble) and G2b (the same example placed INSIDE the milestone section, ruling out a preamble-scoping artifact — the site itself was fence-blind wherever the fence sits), PIN for the LEGACY control (unchanged, pre-existing, out of scope — base is wrong on this shape too). Also folds in the "four fence-blind sites" correction: 3be5c412's commit message and any restatement of it should read FIVE going forward; this commit's own message states the count correctly. Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 960/960 pass (957 + 3 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): state sync's counter excludes the bracket 999 icebox token (round-5 Major 2) Gate-2 round-5 re-verify Major 2 (NEW) — `includeUnconditional999Check` left `state json` and `state sync` reporting different totals for one bracket repo. Under bracket, READING-B puts the sentinel in the bracket, so `isSentinelPhaseId("GSD.02-999", 'bracket')` is false — the `/^999\b/` TOKEN rule is the only thing excluding a `### [GSD.02] 999:` icebox heading, and it ran on the read path (buildStateFrontmatter, `true`) and on getMilestonePhaseFilter (unconditional), but not on cmdStateSync's own counter (`false`). One `state sync` call could leave a single STATE.md with its own frontmatter (percent 50, from the read-path re-sync inside writeStateMd) and body (percent 33, from the write-path counter that alone still counted the icebox heading) disagreeing — falsifying this PR's own stated invariant that sharing countRoadmapPhaseHeadings made "the two counters must see the same phases" structural. Functional change is one argument, exactly as specified: the write call site now passes `syncConvention === 'bracket'` instead of the literal `false`. `syncConvention === 'bracket'` is `false` for every non-bracket value, so the LEGACY path resolves to the exact same `false` it always did — this file's own pre-existing, deliberately- unchanged read/write divergence on that path is untouched. The READ site (`:1860`) is NOT touched — its historical behaviour applied `/^999\b/` to legacy and bracket alike, so changing it would move legacy READ totals, exactly the class of mistake this arc's own Blocker 1 (this round) was. Line count: the functional change is ONE argument (`false` -> `syncConvention === 'bracket'`); the surrounding comment was rewritten because the previous one asserted the now-superseded behaviour ("preserving this file's pre-existing... divergence... a bare 999 token is not excluded here") and leaving it would mislead the next reader — not a structural change. Tests: new describe block "#612 PR-2 round-5 Major 2: state sync excludes the bracket 999 icebox token like the read path" — RED-turned-GREEN for G1, asserting `state sync`'s body percent equals `state json`'s own percent (both 50, not 33 vs 50), PIN for G1's legacy control (33 vs 50 unchanged, deliberately). Targeted suite green: `node --test tests/adr-612-*.test.cjs tests/roadmap-parser.test.cjs tests/state.test.cjs tests/verify.test.cjs` -> 962/962 pass (960 + 2 new). node scripts/lint-phase-id-drift.cjs clean; eslint clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): restore indent parity at the two line-start-anchored bracket-heading reconstructions (round-5 Minor 1) `HeadingToken.offset` (tokenizeHeadings) is the LINE-START character offset, not necessarily the `#` character's own offset — a ≤3-space-indented ATX heading has both. Two round-4 reconstructions built on tokenizeHeadings inherited this gap and accepted indented headings their raw, line-start- anchored predecessors (`^#{1,3}\s+\[...`) never matched: 1. roadmap-parser.cts's bracket-fallback SELECTOR (extractCurrentMilestone, ~line 377) — an indented, version-less `[GSD.02]` milestone heading could be reconstructed into headingMatches, then mis-parsed downstream (selectedBracketId null, level fallback to 1), leaking a SIBLING milestone's phases into the counted scope (G6: HEAD read 3/2/67 instead of 2/1/50 — a real phase heading's own directory belonging to the NEXT milestone got counted). 2. state.cts's isMilestoneBounded (~line 1571) — the same gap let an indented-only `[GSD.02]`-shaped heading wrongly bound a milestone absent from the roadmap, un-suppressing a percent that should stay suppressed (mirrors round-4's F12 fenced-only case, but via indentation instead of a fence). Fix: one added conjunct per site — `content[h.offset] === '#'` (source-named `roadmapRaw` in state.cts) — filtering to tokens whose LINE-START offset IS the `#` character, i.e. exactly the set the raw line-start-anchored regex would ever have matched. Restores byte-for-byte raw parity; no other logic in either function changes. computeSectionEnd and the preamble version/ emoji-token scan are untouched, as instructed — they consumed tokenizeHeadings output before this arc and are out of scope here. Also corrects roadmap-parser.cts's now-provably-false docstring claim that `h.offset` is unconditionally "the same `#`-character coordinate space `content.match().index` used" — true only for the survivors of the new filter, not for every token tokenizeHeadings produces. Line count: the FUNCTIONAL change is exactly 2 lines (one added `&&` conjunct per call site — `git diff --stat` on the two source files shows 20 insertions/7 deletions, but only those 2 lines change behavior; the rest is docstring/comment rationale, per this round's "state the line count" ask). TDD: both fixtures verified RED at HEAD before this commit, GREEN after, via /tmp/pr612rev/rv5-attack.cjs G6 and a locally-authored isMilestoneBounded- isolating probe (G6 alone doesn't distinguish the two sites — its unindented phase headings already satisfy isMilestoneBounded's loose prefix regex either way, so a second, indentation-only fixture was needed to prove that site's fix is not a no-op; verified by temporarily reverting just that one conjunct, confirming 100%-wrongly-bounded RED, then restoring it, confirming suppressed-percent GREEN). New tests (adr-612-bracket-phase-counting.test.cjs): - RED (G6): indented version-less bracket milestone heading — 2/1/50, not the pinned-before-fix 3/2/67. - PIN (G6c unindented control): identical document, no indent — 2/1/50 unaffected on every build. - RED (isMilestoneBounded site, indented-ONLY): mirrors round-4's F12 shape (fenced-ONLY → indented-ONLY) — percent stays suppressed instead of the pinned-before-fix wrongly-bounded 100%. Full G1-G10 (rv5-attack.cjs) + G2b/G3c/G3d/G6c (rv5b.cjs) re-verified zero-drift against TRUTH after this change. Targeted suite (adr-612-*, roadmap-parser, state, verify): 962 -> 965 (+3), 0 fail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): changeset discloses counting-set narrowing + correct stale consumer-count sentence (round-5 minors) FIX 5 (Minor 2): .changeset/2761-bracket-read-tolerance.md did not disclose that bracket phase-heading counting is narrower than the raw-regex predecessor in two ways the review's G4/G5 fixtures surfaced: a level-5 heading (`##### [GSD.02] 05: ...`) is no longer counted (the counter's tokenizeHeadings scan caps at level 4, matching the selector/isMilestoneBounded ceiling), and a space-less heading (`###[GSD.02] 05: ...`) is no longer counted (CommonMark requires ≥1 space/tab after the hashes, which tokenizeHeadings correctly enforces and the old raw regex did not). Both G4 and G5 moved from round-1's wrong (inflated) values back to base's original values as an incidental side effect of routing through tokenizeHeadings — never a deliberate feature of this PR, and previously undocumented. One clause added to the existing run-on paragraph; no other wording in the changeset touched. FIX 6 (Nit 1): tests/adr-612-bracket-heading-selection.test.cjs:76 — `BRACKET_PHASE_TAIL_RE` has TWO consumers as of round-4's 65d257ce (isBracketMilestoneBoundary's own use, plus bracketHeadingHasMatchingChild's same-id-PHASE-child conjunct), not the "no other consumers" the comment claimed. Same correction pattern round-4 already applied to this row's BRACKET_HEADING_INTRO_RE neighbor (that sentence's own staleness was fixed in 4d7184b8): note the true consumer count, confirm both stay nested inside the same bracketBoundaryActive runtime gate (verified at src/roadmap-parser.cts:178 and :250, both reached only through the `if (bracketBoundaryActive)` block starting at :529), and record which commit and which fix introduced the drift. Comment-only; no assertion logic changed. Line count: 2 files, 8 insertions / 2 deletions total — one added clause in the changeset (1 line changed) and one comment block replacing the single stale line in the test file (6 comment lines replacing 1). Verified: targeted suite (adr-612-*, roadmap-parser, state, verify) unchanged at 965/965 pass (comment/prose-only diffs, no test count change). `node scripts/changeset/lint.cjs` and `npx eslint` on the touched files both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): restore the bracketId guard on the bracket-only /^0\b/ sibling rule (round-6 Blocker 1) Round-5's Blocker 1 was `isSentinelPhaseId("undefined-00")` — the merge that introduced the shared counter dropped the `bracketId &&` guard on the sentinel check. This is the identical failure one line further down, in the sibling rule this PR itself added: when the phase-heading grammar's LEGACY alternative matches (`### Phase 0:` in a `phase_id_convention: "bracket"` repo — the mid-migration shape this PR exists for), `bracketId` is `undefined`, and the unguarded `/^0\b/` fires on the bare token anyway. Neither the LEGACY branch of this same function nor `getMilestonePhaseFilter` has a `/^0\b/` rule at all, so the filter counts the phase and admits its completed directory while the counter refuses to count its heading — a milestone with an unstarted phase 02 persists as a confident 100%. `/^0\b/` matches `0` and `0.5` (word boundary before the `.`) but not `00` (no boundary between the two zeros), which is exactly why round-5's G3/G3d fixtures (`### Phase 00:`) never tripped this one — same defect class, different token spelling. Fix: one word, mirroring the guard round 5 restored two lines above — `if (/^0\b/.test(token)) continue;` -> `if (bracketId && /^0\b/.test(token)) continue;`. Expected and intentional side effect: `roadmap analyze` and `state json` now disagree again on this bracket-repo shape (analyze phase_count=2, json total_phases=3) — exactly as they already do under the legacy convention today (verified via /tmp/pr612rev/rv6c.cjs on both conventions). That is the counter regaining agreement with `getMilestonePhaseFilter` (the tighter constraint — it is what actually decides `completed_phases`), not a new break; the counter/filter disagreement is what was wrong. Out of scope, deliberately NOT fixed here (Minor 1, disclosed via a PIN test only): the bracket-SPELLED `### [GSD.02] 0:` shape has the same counter/filter disagreement, but reads 2/2/100 on base too — never closed by any build in this arc, so it is a pre-existing gap rather than a regression this commit could introduce. Line count: src/state.cts is exactly 1 insertion / 1 deletion (one word, `bracketId && ` prepended to the existing condition). TDD: T0 (`Phase 0:`) and T05 (`Phase 0.5:`) verified RED at HEAD before this commit (json 2/2/100, sync body 100%) via /tmp/pr612rev/rv6b.cjs, GREEN after (3/2/67 on both derivations, matching TRUTH). Zero-drift verified by diffing the FULL corpus (rv5-attack, rv5b, rv4-attack, rv-attack1, rv-attack1b, rv-attack3c, rv-mech1, rv2-amend1, rv2-amend2, rv6-attack H1-H19, rv6b) between a pre-fix and post-fix build: the only differing lines in the entire diff are T0, T05, and H14 — the three target fixtures. New tests (adr-612-bracket-phase-counting.test.cjs): - RED (T0) + PIN (T0L legacy control) - RED (T05) + PIN (T05L legacy control) - PIN (B0, bracket-spelled `0` — base-parity characterization, Minor 1, not fixed this round) Round-5's own G3 test (`### Phase 00:`) already serves as the T00 pin — `/^0\b/` never matched `00`, so it is unaffected and untouched. Targeted suite (adr-612-*, roadmap-parser, state, verify): 965 -> 970 (+5), 0 fail. `lint-phase-id-drift.cjs` and `npx eslint` on touched files clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): changeset re-attributes the phase-count level cap + discloses the bare-0 carve-out; correct two stale test comments (round-6 Minor 1 + Nits) FIX 2 (Minor 1 — disclosure only, NOT a code fix): the bracket-SPELLED `### [GSD.02] 0:` / `0.5:` shape has the same counter/filter disagreement round-6's Blocker fixed for the legacy spelling, but it reads 2/2/100 on BASE too — never closed by any build in this arc, so it is a pre-existing gap rather than a regression this round could introduce. Two changes: - Pinned as a base-parity characterization: new PIN test (B0) in 04b5a95f's describe block asserting the exact unchanged value with a comment stating why it's deliberately not touched. - .changeset/2761-bracket-read-tolerance.md: one qualifying clause added to the "counted from ALL of the phases … not a subset" sentence — a bare `0`/`0.x` phase token (however spelled) keeps `roadmap analyze`'s own pre-existing sentinel reading under bracket too, so it stays excluded from these counts. Carried over, not newly introduced by this PR — `roadmap analyze` has always read it this way. FIX 3(a) — same changeset paragraph misattributed the phase counter's 2-4 level cap to "the milestone-boundary machinery in this PR" (the selector, `isMilestoneBounded`, the preamble scan) — that machinery caps at `###` (level ≤3), not 2-4. The 2-4 cap belongs to `getMilestonePhaseFilter`'s own phase scan. Re-pointed the attribution; the paragraph's earlier, correct ≤3 claim (selector/isMilestoneBounded) is untouched. FIX 3(b) — tests/adr-612-bracket-heading-selection.test.cjs:76-82 (added by 1395bd89, round-5's own Nit-1 correction) claimed both `BRACKET_PHASE_TAIL_RE` consumers are reached "only through the `if (bracketBoundaryActive)` block starting at :529". Verified at HEAD: `bracketHeadingHasMatchingChild` is (only caller :593, inside that block). `isBracketMilestoneBoundary` is not — it has two callers, an inline `bracketBoundaryActive &&` conjunct at `:473` (inside `computeSectionEnd`) and the `:529` block at `:567` — the very fact this row's own earlier "exactly two callers" paragraph already stated correctly. Corrected the mechanism claim; the CONCLUSION (every consumer is still gated on the same flag) is unchanged, exactly as round-5's own Nit-1 fix left round-4's conclusion unchanged when it corrected the consumer count. FIX 3(c) — tests/adr-612-bracket-phase-counting.test.cjs:2311,2340 still said "four fence-blind sites" after round-5 (357ba671) added a fifth (the retirement scan) in its own block below. Reworded both the section comment and the `describe()` label to read as historical scoping of round-4's own fix ("the four sites known at round 4 … a fifth was found at round 5, see its own block below") rather than a live exhaustive claim. Line count: 3 files, changeset 1/1, heading-selection test 17/8, phase-counting test 6/2 (comment/prose-only; B0's own PIN test landed in 04b5a95f alongside T0/T05 since it was investigated as part of that same Blocker's defect class, not in this commit — noted here for the record). Verified: targeted suite (adr-612-*, roadmap-parser, state, verify) unchanged at 970/970 (comment/prose-only diffs, no test count change). `npx eslint` and `node scripts/changeset/lint.cjs` both clean. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): W021's bracket remediation hint stops pointing at a command that hard-errors (round-7 Minor 2) `checkBracketCoherence`'s W021 (added by 94abf5df) attached the fix hint `Run \`gsd-tools roadmap upgrade --convention bracket\` to migrate (dry-run by default)` to every bracket-convention W021 — both sub-checks (`missing-bracket` and `mismatch`) share the single `addIssue` call at src/verify.cts:2255-2256. But `roadmap-command-router.cts:204` throws unconditionally for any `--convention` value other than `milestone-prefixed`: $ gsd-tools roadmap upgrade --convention bracket Error: Only --convention milestone-prefixed is supported This contradicts the PR's own two disclosures: the changeset ("`bracket` is a READ-path opt-in until the migrator and write path land") and docs/CONFIGURATION.md:188 ("There is no bracket migrator and no bracket emit yet"). Reachability is the exact mid-migration repo this PR targets — any bracket project with one un-migrated heading gets an unfollowable instruction on every `validate health`. Fix: one string. Replaced the hint with what a user can actually do today — manually align the heading's bracket milestone to its section — and named the tracked future landing (#612 PR-3) instead of a command that errors. The milestone-prefixed sibling hint at src/verify.cts:2240 (a different, already-functional convention/command pair — verified against roadmap-command-router.cts:65) is untouched. Line count: src/verify.cts is exactly 1 insertion / 1 deletion (one string literal). No test in the suite previously asserted this string's content (`grep -rn "upgrade --convention bracket" tests/` was empty), so the unfollowable hint shipped unpinned — `lint-fix-has-regression-test.cjs` would not have caught a string-only change without a new test. Added one: asserts the fix string both (a) does not match `--convention bracket` (the specific pinned-before-this-fix hazard) and (b) equals the new string exactly, covering the invariant a future edit must not re-break: no unsupported `--convention` value named in remediation text users are expected to run verbatim. Verified end-to-end (not just unit-level) via /tmp/pr612rev/rv7f.cjs: the W021 issue's `fix` field now reads the new string; `gsd-tools roadmap upgrade --convention bracket` (and its --dry-run variant) still correctly hard-error — that command remains unsupported, only the hint text changed. Targeted suite (adr-612-*, roadmap-parser, state, verify): 970 -> 971 (+1), 0 fail. `lint-phase-id-drift.cjs` clean; `npx eslint` on touched files: 0 errors (1 pre-existing unrelated no-elapsed-assertion warning, same file, same line this arc's prior rounds already disclosed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): correct the bare-0 changeset disclosure + disclose W021's second sub-check (round-7 Minor 1 + Nit 1) FIX 1 (Minor 1) — round-6's bare-0 changeset clause (1395bd89) was wrong in three falsifiable ways: (a) Its own illustration (`### Phase 0:`) is exactly the LEGACY spelling 04b5a95f made COUNTED. The residual exclusion after that fix applies only to the BRACKET-spelled token (`### [GSD.02] 0:`) — the clause named the wrong shape as its example. (b) "excluded from these counts" over-scoped the carve-out. The phase-0 directory is admitted into `completed_phases`/`total_plans` in BOTH spellings (measured: B0 at HEAD has `completed_phases=2`, `accepts {"GSD.02-0-bootstrap":true}`). The exclusion that survives lives in `total_phases`/`phase_count` (heading counting) only. (c) The pre-existing "so `roadmap analyze` and `state json` report the same number" sentence (present since before this arc's bracket work) is now false for the bare-0 LEGACY-spelled shape: 04b5a95f's own commit message discloses this exact re-divergence as expected — `roadmap analyze` phase_count=2 vs `state json` total_phases=3 on T0 — matching the disagreement legacy already carries today. The changeset still asserted unconditional agreement. Rewrote both sentences: dropped the `### Phase 0:` example, scoped the carve-out to `total_phases`/`phase_count`, named the bracket-spelled token as the one that keeps analyze's sentinel reading, stated plainly that the legacy-spelled form in a bracket repo IS counted (the mid-migration guard), and qualified the "report the same number" claim to the `999` token, with the bare-0 legacy-spelled shape named as the one exception and why. FIX 3 (Nit 1) — `checkBracketCoherence` has always had two sub-checks (its own docstring: "Two sub-checks, both surfaced as W021") but both the changeset and docs/CONFIGURATION.md:188 described only the `mismatch` sub-check. The `missing-bracket` sub-check — which fires on every legacy-spelled heading in a bracket repo, the noisier of the two on a mid-migration project — was undisclosed in both places. One clause added to each. Line count: 2 files, 1 line changed each (both are single-paragraph/ single-row files; git diff --stat reports 1/1 per file though several distinct clauses were edited within that one line each). Verified: `node scripts/changeset/lint.cjs` clean. Targeted suite (adr-612-*, roadmap-parser, state, verify) unchanged at 971/971 (prose-only diffs, no test count change, no code touched). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): resolvePhaseIdConvention's docstring no longer claims loadConfig drops the key Upstream #2997 (aa7697fe, landed in `next` during this PR's final verification) added `phase_id_convention: get('phase_id_convention') ?? null` to `_baseConfig` in config-loader.cts — `loadConfig` now surfaces `phase_id_convention` in its resolved config. This function's docstring gave "loadConfig merges against CONFIG_DEFAULTS and drops keys it does not know, and `phase_id_convention` is not among them" as the reason for reading config.json directly instead of calling `loadConfig(cwd)`. That rationale is now stale against live `next`. Corrected the comment to state the two reasons that actually survive #2997: 1. The workstream->root federation this function performs is a standalone resolution run against a GIVEN cwd, not necessarily the same base a `loadConfig(cwd)` call elsewhere in the codebase would federate from. 2. Convention-ENUM validation is still #612 PR-4 work — this function returns the raw string unvalidated, exactly as the now-surfaced resolved key would. Noted that #2997 surfacing the key makes consuming it from resolved config (instead of re-reading config.json here) a natural PR-4 consolidation — not this PR's scope. The "cycles were never the obstacle" close and every other paragraph in the docstring (federation rationale, SCOPE note) are untouched; they still hold. Comment-only — no code behavior changed. This worktree's own history does not contain aa7697fe (git merge-base --is-ancestor confirms neither branch is an ancestor of the other; not rebasing per instruction), so this is a textual correction against a documented external fact, not a functional sync with upstream. git diff --stat: src/planning-workspace.cts | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) Verified: `npm run build:lib` clean, `node scripts/lint-phase-id-drift.cjs` clean, `npm run lint:ci` exit 0 (same 2 pre-existing unrelated eslint warnings as every prior round this arc, 0 errors; lint-fix-has-regression-test PASS). Full suite not re-run per instruction (comment-only diff). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#2761): resolve phase_id_convention against the caller's workstream `resolvePhaseIdConvention` took no workstream, so it resolved from `planningDir(cwd)` — which falls back to `GSD_WORKSTREAM` only when its `ws` argument is `undefined`. Every caller that passes a workstream by ARGUMENT (they cannot set the env var per iteration) therefore read the convention from the ROOT config while reading that workstream's ROADMAP. Two reproduced consequences: - A workstream that explicitly declares its own `phase_id_convention` had it ignored. Flipping ONLY the root config between bracket and milestone-prefixed changed which milestone that workstream extracted. - `--workstream foo` and `GSD_WORKSTREAM=foo` disagreed on the same repo: the arg form fell through to the root config, the env form did not. `resolvePhaseIdConvention(cwd, ws?)` now forwards `ws` to `planningDir`, and both roadmap-parser call sites pass theirs — `extractCurrentMilestoneScoped` (which already reads STATE from `planningDir(cwd, ws)`) and `getMilestonePhaseFilter`'s lazy branch. `undefined` keeps the env fallback, so convention-less call sites are byte-identical; `null` still means "explicitly no workstream". The `undefined` vs `null` discriminator on `phaseIdConvention` is untouched — only the base the `undefined` branch resolves FROM moves. workstream-inventory's two sites (`countRoadmapPhases`, `inspectWorkstream`) passed a literal `null` where they meant `undefined`, pinning every workstream to the legacy grammar. On a bracket workstream whose milestone declares 3 phases that returned phaseCount 0 and fell back to the on-disk directory count; it now returns 3. A non-bracket workstream is unchanged (legacy control pinned). The workstream -> root federation is preserved: a workstream that declares no convention still inherits the root, as config-loader does. That inheritance is pinned so the fix cannot be over-applied into isolation. * fix(#2761): classify missing phase details per occurrence, not per token Under READING-B a phase's sentinel status lives in the BRACKET, so `[GSD.999] 01` and `[GSD.02] 01` share a token and are not the same phase. Both sides of the missing-detail check were keyed by the bare token anyway, and both produced false negatives in `missing_phase_details`: - The checklist scan built a token -> bracket-id Map, FIRST-WINS. Of two entries sharing a token, whichever the author wrote first classified both. With `- [ ] **[GSD.999] 01: Icebox**` above `- [ ] **[GSD.02] 01: ...**` the real phase inherited the icebox's sentinel verdict and vanished from the report; swapping the two bullets — same document, same phases — reported it. Pinned with a test asserting BOTH orders. - The detail set was `new Set(phases.map(p => p.number))`, also token-keyed, so `[GSD.02] 01`'s heading marked token `01` present and satisfied `[GSD.03] 01`, which has no heading anywhere. Order-independent, same class. Both sides now key on an occurrence key: the owner's `bracketQualifiedKey` (fold- and padding-insensitive, so `[gsd.2] 01` and `[GSD.02] 01` are one phase), falling back to a fold-normalized composite for the two shapes that grammar refuses — a token carrying its own hyphen, which splices to an id whose trailing segment the qualified-key grammar truncates (the hazard `getMilestonePhaseFilter` guards with its own `!token.includes('-')`), and any id it does not accept. With no bracket id the key IS the bare token, so the legacy path keeps its exact keys and dedupe order. The emitted value is unchanged — `missing_phase_details` stays an array of bare tokens, matching `phases[].number`. Only the classification moved to the qualified key, so two brackets' `01` both missing report `01` once instead of one silently covering for the other. * test(#2761): replace the two wall-clock ReDoS assertions with algorithmic bounds Both guards asserted elapsed wall-clock time — `Date.now()` against a 20s ceiling in the coherence suite, `process.hrtime.bigint()` against 1s in the read-tolerance suite. Those measure the host machine rather than the SUT and flake on a loaded CI runner (RULESET.TESTS.no-timing-assertion). Per RULESET.TESTS.delete-bad-tests they are REPLACED, not skipped, and the behavioral property each one guarded is preserved. The property is "the widened bracket patterns do not backtrack catastrophically", which is a claim about growth, so it is now stated by scaling the input instead of by timing it: - validate health runs the pathological unclosed bracket at 4,000 and 16,000 characters and must return the same correct result (no W021) at both. - The four reader entry points run five ReDoS shapes at 5,000 and 20,000 and must name exactly the phases each shape should name at each size, with the variant cardinality unchanged across the two. Catastrophic backtracking is superlinear, so a regression cannot complete the 4x leg under any ceiling, while a bounded matcher is indifferent to the scaling. The one attack that is well-formed-but-oversized now pins its reading precisely (`['1'.repeat(n)]`) rather than being lumped in with the malformed ones. A positive control asserts the readers still extract a well-formed bracket heading, so "names no phase" cannot pass by the readers being inert. `{ timeout }` is a hang backstop, not an assertion: it turns a runaway into a deterministic failure instead of a suite that never returns. * fix(#2761): give the bracket grammar one owner and teach the drift guard to see it The bracket milestone-intro grammar was re-typed verbatim in three readers — roadmap-parser's bracket-fallback selector, state's `isMilestoneBounded` and verify's `checkBracketCoherence` — which is exactly what #2761's own gate forbids ("no token literal outside src/phase-id.cts"). `check:phase-id-drift` reported clean the whole time: its detector only ever knew the phase-NUMBER token grammar, so the bracket class `[A-Z][A-Z0-9_]*` was invisible to it. Ownership. `src/phase-id.cts` now exports the class as `BRACKET_PROJECT_CODE_SRC` and the intro in the two shapes its readers need: `bracketMilestoneIntroSrcFor(milestone)` (pinned to one milestone) and `BRACKET_MILESTONE_INTRO_CAPTURING_SRC` (milestone captured). The pinned builder owns the pad2 spelling rule too — "canonical spelling only, not `0*N`" was previously restated in prose beside each copy, a convention two files had to keep agreeing on by hand. `BRACKET_ID_SRC`, `BRACKET_ID_PREFIX_RE` and `BRACKET_QUALIFIED_KEY_RE` now compose the class rather than re-spelling it. All three call sites consume the owner; the regex sources are byte-identical to what they spelled, asserted against hand transcriptions of the pre-fix lines. Guard. `scripts/lint-phase-id-drift.cjs` gains a bracket rule (`findBracketGrammarDrift`), wired into `scanRepo` and tagged `kind`. It deliberately does NOT copy the token rule's `line.includes(CANON_REF)` escape: that escape is line-level, and verify's copy referenced the owner for the MILESTONE field on the same line as the re-typed PROJECT-CODE class — so a bracket rule with that escape would have kept passing on the very site under review. Partial ownership is the drift; only a dedicated `// phase-id-owner:` comment suppresses it. Proof, end to end: planting the shipped verify.cts literal back into src/ makes `npm run check:phase-id-drift` exit 1 naming `[bracket] src/verify.cts:1475`; restoring it returns the gate to ok. Tests. phase-id-drift-guard carries all three shipped literals as negative fixtures, the same-line-owner-reference case, the case-widened evasion variant, the sanction rules, and a temp-tree scan proving the rule is wired into scanRepo rather than merely exported. continuation-grammar-parity drives the pinned and capturing shapes over a 12-entry corpus and requires the same verdict from both plus the same captured milestone — widening either alone fails there. * test(#2761): drop the out-of-scope source-grep exemption from the selector pin The baseline-selector pin in tests/adr-612-bracket-heading-selection.test.cjs read `src/*.cts` with readFileSync and regex, claiming the no-source-grep escape with a source-text-is-the-product reason. CONTEXT.md's documented scope (RULESET.TESTS.no-source-grep.exemption) reserves that escape for tests whose subject is a runtime CONTRACT FILE — STATE.md, config.toml, hooks.json, agent .md — and `src/*.cts` is none of those. It was also broader than it looked: eslint-rules/no-source-grep.cjs matches the marker in ANY comment in the file, so one block's claim disarmed the rule for the whole ~700-line suite. The pin itself is worth keeping — the BASELINE ARGUMENT at each call site is a fact no behavioural test can recover (flipping verify's milestone-complete site from LABEL_ONLY to ANY_BRACKET grants a tolerance it has never had, and every behavioural test still passes), so pinning it does require reading the authored source. That reading moved to `scripts/lint-phase-id-drift.cjs` — the seam's own guard, where source scanning is sanctioned (`warn` scope) and already happens for the grammar rules — as `countSelectorBaselines` / `scanSelectorBaselines`, which return a structured census. The test asserts on the returned data and touches no file text. CONTEXT.md is unchanged: the exemption scope was not widened to fit the test. No marker remains in the suite, so the rule is live across all of it again, and eslint passes with the escape removed rather than relocated. A floor assertion pins that the census actually found the five consumers, so the "no other file consumes the selector unpinned" check cannot pass on an empty scan. * docs(#2761): rewrite the changeset as a lead + deltas instead of one paragraph The fragment was a single ~6,400-character paragraph that opened on internal mechanics, buried the user-visible change, and named an internal test path (tests/adr-612-bracket-phase-counting.test.cjs) that means nothing to a reader of the CHANGELOG. It now leads with what a user sees — bracket-style phase IDs are recognized on the read path across roadmap, validate, verify and state — followed by compact bullets for the behavioral deltas, the opt-in caveat, and the upstream consequences. Both merge-added disclosures are kept: the #3185 legacy-sentinel Phase-0 delta with the reason the bracket counter keeps the narrower rule, and the enumerator's convention-argument default flip with the four newly-scoped read surfaces and the note that archival and milestone-completion paths are unchanged. Two deltas from this review round are stated as their own bullets: per-occurrence classification in `missing_phase_details`, and workstream-scoped convention resolution including the workstream-rollup count change. The internal test path is gone and the body is down to ~3,850 characters. The bullets render as sub-bullets under the CHANGELOG entry and the `(#NNNN)` suffix still lands on a trailing paragraph rather than mid-list. `npm run lint:changeset` passes. * test(#2761): use helpers.cleanup in the drift-guard temp-tree test `local/no-raw-rmsync-in-tests` rejects a bare `fs.rmSync` in tests — the shared helper carries the Windows-EBUSY retry budget. The temp-tree scan added with M3's guard coverage test used the raw call. * docs(#2761): correct the occurrenceKey comment on qualified-key normalization The comment claimed `bracketQualifiedKey` is "fold- and padding-insensitive, so `[gsd.2] 01` and `[GSD.02] 01` are one phase". The fold half is right; the padding half is not. `BRACKET_MILESTONE_NUMERIC_SRC` is `(?:[1-9]\d{2,}|\d{2})`, so `bracketQualifiedKey('GSD.2-01', 'bracket')` returns null and that input takes the composite fallback instead. No behavior change — the two key spaces use different separators and cannot collide — but the claim was checkable and wrong. Restated: the owner case-folds, and padding-tolerance is not a property it has or needs, because each accepted milestone has exactly one canonical spelling and `[GSD.2]` is malformed rather than an alternate spelling of `[GSD.02]`. * ci(#2761): give the coverage-gate job the heap floor the shard jobs already have The gate's report steps parse ~2.5GB of merged raw V8 dumps in one process at the runner's implicit ~4GB ceiling, so pass/fail comes down to GC timing (run 31338081337 OOM'd; next passes the same volume at 28s). Deterministic locally: crashes at --max-old-space-size=4096, completes in 11s at 8192. PR shard data is within 0.15% of green next runs — the load is pre-existing, only the ceiling was missing. #2952 set 6144 on the shard-collection step; the gate job was missed. * Revert "ci(#2761): give the coverage-gate job the heap floor the shard jobs already have" This reverts commit 03c74a97c911e9ddc53675aded9c9bbb89f14cd1. * fix(#2761): thread the resolved convention into cmdStateValidate's phase-directory lookup #3208 replaced cmdStateValidate's `startsWith` prefix test with the canonical `phaseKeyFromDir(...) === selectedPhaseKey` comparison. That is the right surface, and it is why the lookup now needs the resolved `phase_id_convention`, which the rewrite does not pass. `phaseKeyFromDir` deliberately refuses to read a bracket directory without an explicit signal (ADR-2121: a bracket dir is string-indistinguishable from the legacy letter-prefixed-decimal family), so un-threaded it returns the whole dir name as the key — `GSD.02-05-real-work` -> `GSD.02-5-REAL-WORK` — while the STATE side is the bare `05` that `parsePhaseFromProse` yields. Both sides of one comparison derived under different conventions is #2562's defect class, and this file's other three `phaseKeyFromDir` call sites already thread against it. Observable: a bracket repo whose phase directory plainly exists reported `valid: false` and "no phase directory matches phase 05", and the drift scan (plan-count mismatch, verification status) never ran. The pre-#3208 `startsWith` missed the same directory but skipped silently, so this is a visible-failure regression on bracket repos, not a new miss. Non-bracket conventions are byte-identical by construction: `extractPhaseToken` branches only on `=== 'bracket'`, so null / 'milestone-prefixed' / unresolvable compile the same path as the un-threaded call. The flat-legacy twin assertion pins that. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2761): note the cmdStateValidate convention threading in the changeset fragment The fragment described the bracket read path as of the pre-merge branch. 504c64ff added a shipped behaviour change — `state validate` now resolves bracket phase directories — that the fragment did not mention, so the rendered changelog would have under-described what ships. Body text only; `type` and `pr` are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): invert the inherited-wart characterization — #3225 fixed it upstream Required by the merge of next @ 86101ee6; test-only, no src delta. This branch disclosed rather than fixed a pre-existing upstream wart: on a LEGACY repo, `### Phase 999:` warned from `validate consistency` while `validate health` suppressed it — the two verbs contradicting each other. The case was pinned as a characterization test ("INHERITED WART, unchanged") precisely so it would INVERT if upstream ever closed it, rather than rot silently. ae7dc529 (#3225) closed it, by adding the `isSentinelPhaseId` guard to this very loop. So the assertion inverted on the merge — as designed. Flipped to assert the FIXED behaviour rather than deleted: it is the negative-space proof that this branch's `sentinelPhases` guard never had to grow a legacy reading of its own, and it reds if a future conflict resolution keeps our guard while dropping upstream's. Added a scope control alongside it (a legacy NON-sentinel `### Phase 09:` with no directory still warns), so deleting the loop outright cannot pass. Union proven load-bearing in BOTH directions against the merged tree — neither guard subsumes the other: - drop `isSentinelPhaseId(p)` (ours only) -> 1 red, exactly this case. Note upstream's own #3225 tests stay GREEN there: they cover the disk-side loops (sentinel dir on disk) and the gap-numbering filter, not the ROADMAP-side loop. This case is that site's only coverage, which is the second reason to keep it rather than delete it. - drop `sentinelPhases.has(p)` (upstream only) -> 4 red bracket suites (icebox-not-missing, health/consistency agreement, occurrence-aware suppression, checklist-index suppression). tests/adr-612-bracket-read-tolerance.test.cjs 79 -> 80, all green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): pin the three re-homed W006/W007 reads the merge left unfalsifiable #3309/#3310 deleted the helpers this PR threaded (collectDiskPhases, collectDiskPhaseEntries, collectArchivedPhaseDirNames, forEachArchivedPhaseToken) and rebuilt their reads inside src/planning-snapshot.cts + src/health-diagnostic-rules/*.cts. The convention threading moved with them in the merge commit — but a mutation sweep over the re-homed sites found three where reverting the convention argument changed real CLI output and NOT ONE existing test went red. The pre-migration sites were covered indirectly, through helpers that no longer exist, so the coverage did not survive the relocation even though the behaviour did. Each case is pinned at the CLI with its flat-legacy twin as the byte-identity control, plus a non-vacuity control in the opposite direction: archivedPhaseTokens revert -> "Phase 05 in ROADMAP.md but no directory on (planning-snapshot.cts) disk" for a phase whose only directory is under .planning/milestones/v1.0-phases/ roadmapPhaseCheckboxes revert -> "Phase 09 in ROADMAP.md but no directory on (planning-snapshot.cts) disk" for an unstarted `- [ ]` phase W007's extractPhaseToken revert -> "Phase GSD.02-77-orphan exists on disk" (roadmap-disk-consistency) instead of "Phase 77 exists on disk" Red/green: each single-line revert reds exactly its own case and nothing else; every legacy control stays green in all three runs. NOT pinned, and disclosed as droppable rather than given an unfalsifiable test: buildValidPhaseSet's extractPhaseToken(dir, convention) in W002. That rule unions disk tokens with roadmapDeclaredPhases and archivedPhaseTokens, and any STATE.md reference a bracket disk token would rescue is already rescued by the ROADMAP half — probed directly, the argument makes no observable difference. It is threaded because it restores collectDiskPhases(planBase, convention)'s derivation exactly, not because a test needs it. Test-only; revertable independently of the merge commit. * test(#2761): pin the two reads the #3165 extraction left unfalsifiable DROPPABLE, offered as such. Test-only; the merge commit is correct without it. #3428's extraction gave `collectAnalyzePhases` TWO call sites — the scoped milestone window and the truncated-window recovery path. The merge threads `convention` into both and swaps `detailKeys` with `phases` across the recovery. Neither of those was falsifiable by the suite as it stood: - passing a NULL convention at the FALLBACK site, with the scoped site still threaded, is green across all 518 tests of the bracket, roadmap and milestone-window files. Every pre-existing bracket assertion reaches the enrichment through the scoped window, so none of them can observe the fallback's reading at all; - dropping `detailKeys = fallbackScan.detailKeys;` — the line this merge authored — is likewise green, because no fixture that reaches the recovery path carries a checklist bullet, so `missing_phase_details` is `null` either way. That is the same hole class lap 3 closed in 9914359c: an argument whose revert changes real CLI output with zero reds. Pinned at the CLI on the shape that reaches the fallback on a bracket repo — a MID-MIGRATION ROADMAP: bracketed ACTIVE milestone, its phase-detail sections and checklist bullets sitting after an intervening CLOSED legacy milestone (so the window closes over prose only), plus one legacy `### Phase N:` section of its own. Six tests: - a NON-VACUITY control that removes the phase directories, so #3428's own precondition fails and the result is the empty one the recovery exists to replace — this is what proves the rest read the FALLBACK's output rather than the scoped scan's; - the directory assertion (`disk_status`/`plan_count`/`summary_count` for canonical `{CODE}.{MM}-{PP}-slug` dirs) + a flat-legacy twin asserting byte-equal shape, and that the twin is the right answer rather than a shared wrong one; - `missing_phase_details: null` for phases the recovery just found, and its companion direction — a bullet with no heading anywhere is STILL reported, so a fix that merely suppressed the report does not pass; - a non-bracket repo taking the same path unaffected. Red/green, each mutation reverted afterwards: - fallback call site -> `null` convention: exactly 2 reds, both directory assertions in this block; 307 other tests green, including upstream's own #3428 tests in tests/milestone-window-single-owner.test.cjs. - `detailKeys` swap deleted: exactly 2 reds, both `missing_phase_details` assertions; 414 other tests green. - `matchPhaseDirs` 3rd argument dropped (the lap-2 site): 4 reds — the 2 already on record plus this block's 2, which is the point: one seam, now reached by two paths. DISCLOSED IN THE TEST BODY WITH MEASURED OUTPUT, not fixed: `hasPhaseEntries` (src/roadmap-parser.cts) is convention-blind, so on a PURE-bracket ROADMAP the window classifies COMPLETE and #3428's recovery is gated off entirely. That document returns `{"scope":"complete","phase_count":0,"next_phase":null, "phases":[]}` — the "genuinely empty milestone" answer, the indistinguishability #3184 introduced `scope` to remove — where the flat-legacy twin returns `{"scope":"truncated","phase_count":2,...}`. Widening it changes the value of an upstream-owned output field on bracket repos, so it is a behaviour slice (the PR-2.5/PR-4 convention-less-readers question), not a merge-round change. The mid-migration shape pinned here is the reachable half. * fix(#2761): mirror the bracket terminator in the milestone-scope write guard parser's terminator vocabulary — "a level 1-3 heading that is not a Phase heading and carries a milestone signal". On this branch that vocabulary is convention-SELECTED: `computeBracketSectionEnd` adds `isBracketMilestoneBoundary` as a terminator arm, and the ADR-canonical `## [GSD.09] Hidden` carries NO vN.N token, NO ✅/📋/🚧/🔄 marker, and not the word "Milestone". Left unmirrored, the guard accepted exactly the description it exists to reject. NOT a defect on clean next — a bracket heading terminates nothing there. The branch widens the terminator set, so the branch owns the mirror. MEASURED at the CLI seam before the fix (bracket fixture, one milestone, one phase): $ gsd-tools phase add $'Sneaky\n## [GSD.09] Hidden' -> exit 0, written $ gsd-tools phase add 'Innocent follow up' -> exit 0, written $ gsd-tools roadmap milestone-scope { "scope": "complete", "phases": ["01","1"], "phase_count": 2 } $ grep '^### Phase' .planning/ROADMAP.md ### Phase 1: Sneaky ### Phase 2: Innocent follow up <- in the document, out of the window After the fix the first add exits 1, ROADMAP.md is byte-unchanged and no phase directory is created. The legacy twin (same text, no `phase_id_convention`) still exits 0 — opt-in only, base behaviour preserved. SHAPE - `findMilestoneScopeHeadingLines(text, convention)` — REQUIRED, the same tripwire `scanMilestonePhaseIds` carries in the merge commit and for the sharper reason: a blind call here fails OPEN (the guard quietly ACCEPTS a window-narrowing description), so a future call site must fail to COMPILE. Census is one caller, which pays nothing for it. A non-bracket value takes the pre-existing path byte-identically. The bracket arm routes through `isBracketMilestoneBoundary`, the same single-owner phase-vs-milestone discriminator `computeBracketSectionEnd` consults; no second bracket-heading grammar is spelled here. - `selectedBracketId` is deliberately `null`, so the same-milestone CONTINUATION exemption never fires and a value naming the ACTIVE milestone is flagged too. That is this predicate's third stated conservatism and rests on its own existing argument: which milestone is active is a property of the document at write time, not of the text being validated, and over-rejecting is one-directional. - `assertDescriptionPreservesMilestoneScope` takes `cwd` and resolves through the branch's tolerant try/catch — this guard runs BEFORE `loadConfig` and before the ROADMAP existence check, so an unresolvable convention must degrade to the legacy vocabulary, never turn a rejection into a crash. - The error's marker list gains the bracket form only when the project is on the bracket convention. RED/GREEN - Revert the bracket arm -> 2 reds (phase add; insert + add-batch), the non-bracket control and both no-false-positive cases stay green. - Revert the probe threading in the merge commit -> 1 red (the probe case). - 6 new cases in the #3262 file, each with a non-bracket or no-false-positive control: fenced bracket milestone heading and bracket PHASE heading are both non-violations. Droppable: revert this commit and the merge stands on its own; the branch then ships the gap as a disclosure instead of a fix. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(changeset): narrow the enumerator claim to the directory set — per-entry rendering in progress/stats/init-manager is display-slice scope Round-7 review confirmed 3 of the 4 surfaces named by the closing claim parse each directory or heading with legacy-only patterns that live in files this PR does not touch (commands.cts:1766, commands.cts:2116, init.cts:2241). The claim now states exactly what this slice delivers: the scoped directory set. Their per-entry conversion is display work, deferred to the epic's display PR with the statusline/progress-card gates it belongs beside. * fix(#2761): de-accident the unmatched-milestone bracket fixture, re-pin to #3480's withhold contract tests/adr-612-bracket-phase-counting.test.cjs:515 ("a milestone that does NOT match STATE is not scoped in") asserted total_phases === 0. Since today's merge brought in 70b5c1a1 (#3354/#3480, already in `next`), buildStateFrontmatter withholds total_phases (omits the key, read back as null) for a milestone that is genuinely sectioned but matches no ROADMAP heading, instead of substituting a computed number — the fixture's `state json` read now returns null, failing the strictEqual(0) assertion. The original single-section fixture's 0 only ever survived by accident: hasMilestoneSectioning requires >=2 milestone-vocabulary headings to call a ROADMAP sectioned, and its isPhaseHeading helper recognizes only the legacy `Phase N:` text form — so the bracket phase heading `### [GSD.03] 09: Not this milestone` (title containing the word "milestone") miscounted as a second milestone heading, tipping hasMilestoneSectioning true and routing to the disk-count branch. With that miscount removed, the same one-section fixture reads 1 (the non-matching milestone's phase count leaking into v2.0's total) — proving the pinned 0 was never validating the scoping this test claims to exercise. Replaced the fixture with two genuine milestone sections (GSD.03, GSD.04), neither matching STATE's v2.0, with phase titles carrying no incidental vocabulary — the real #3354 shape. Re-pinned the assertion to null, matching upstream's own tested contract (tests/state- document.test.cjs, "#3354 with nothing stored, the key is omitted rather than written from the dir count"). Verified: file green 3/3 runs (104/104), full state-suite regression 771/771 green. The isPhaseHeading gap that made the old fixture accidental is a real, pre-existing, upstream-owned limitation (hasMilestoneSectioning only guards >=2-section conflation, not a single non-matching section leaking through) — not introduced by Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#2761): exempt two self-authored-fixture regexes from #3441's unbounded-quantifier rule Both sites parse the STATE.md the test itself just wrote to its own tmpdir — fixed-size fixture output, not adversarial or document-scale input. Exempted with the justification-comment pattern the tree's own tests use for this exact case (settings-integrations, copilot-install, research-agent-profiles). The sites predate the rule; #3441 landed on next this morning and this branch picked it up in the catch-up merge. * fix(#2761): fix forward two next-movement test regressions from the round-11 rebase origin/next's #3573 newly threads STATE's stored `milestone:` value into `state json`'s buildStateFrontmatter call (previously always `undefined` on that read surface). Two pre-existing PR-2 fixtures reach code paths that call never exercised before that merge: - RED (repro2 case C): a version-less, all-bracket-id ROADMAP now hits getMilestonePhaseFilter's pre-existing (unaffected by this branch) row-5 `versionResolved && !headingFound => SCOPE.UNSCOPED` rule, which withholds `progress.percent` even though the scoped total_phases/completed_phases are still correct. That row-5 rule is load-bearing for six other version-less-document pins in this same file; narrowing it broke seven of them in testing, so production code is untouched here. Reassert the test's real claim (scoping, via total_phases/completed_phases) and disclose the now-withheld percent instead of silently dropping it. - PIN (repro12 LEGACY control): a fenced-example-vs-real version heading fixture. #3573 routes this read through sliceMilestoneWindow (fence-aware) instead of the legacy anyMilestonePattern raw scan (fence-blind) this pin was disclosing as out of scope, so the fixture no longer exercises the blind path — total_phases moves from the whole-doc 4 to the correctly scoped 2. Re-pinned with an upstream-attributed comment, matching this file's existing house style for prior origin/next movements (see the sibling "PIN (G3 LEGACY control)" comment). Both are test-only fix-forwards: no production code changed. The remaining 12 failures in this file at 27363e9d0 (dropped seam commits' VERIFICATION.md naming + fixture updates) are pre-existing and deferred to the stacked follow-up per the task's own scope. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2761): thread phaseIdConvention explicitly at the two write-adjacent enumerator call sites (round-11 BLOCKER) listMilestonePhaseDirs' phaseIdConvention param lost its `= null` default (phase-locator.cts), so an omitted convention now means "resolve from config" instead of "explicitly not bracket" — a deliberate flip, but milestone.cts and state.cts were not in the PR-2 diff and both call the enumerator without threading it: - milestone.cts cmdMilestoneComplete (the single #3597 shared derivation feeding the stats loop, --dry-run preview, and the real archive/rename pass) now resolves phase_id_convention once and threads it explicitly, so a bracket project's `milestone complete` archives its real bracket-declared phase directories instead of silently inheriting whatever the enumerator's lazy default resolves to. - state.cts cmdStateUpdateProgress's own enumerator call threads the same resolved convention. Empirically this does not change the #3217 withhold gate (scope is assigned before headingConvention resolves in getMilestonePhaseFilter, so it's convention-independent either way) or the reported percent (already correctly threaded via computeUpdateProgressPreview -> buildStateFrontmatter); it closes a second, silently-resolved answer to the same question this file's own ONCE-and-THREAD rule (~:2300) already states as policy. - state.cts's other listMilestonePhaseDirs call site (the state-sync scope-only read, ~:4791) is left unthreaded on purpose, with an inline note explaining why: only `.scope` is consumed, and `.scope` is set before convention resolution in getMilestonePhaseFilter, so it cannot disagree with a threaded convention. Both enumerated sets are pinned by new tests (round-11 BLOCKER block in tests/adr-612-bracket-phase-counting.test.cjs), including a mutation-style regression check on the milestone.cts fix (forcing the un-threaded default back on makes the pinned test fail, catching a regression to the pass-all-degrade legacy reading). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2761): correct the changeset's false archival claim and amend ADR-612 for the round-11 M2(2) split scope The changeset (.changeset/2761-bracket-read-tolerance.md) asserted "The archival and milestone-completion paths are unchanged" — false: both paths reach the widened enumerator, and this PR now threads their convention explicitly (previous commit). Replaced the closing paragraph with an accurate description of what changes for a bracket project at those two call sites, and notes both enumerated sets are pinned by tests. ADR-612 (docs/adr/612-bracket-phase-id-convention.md) amended per M2(2): the 2026-08-03 PR-2/PR-4 boundary proposal is added in-body (PROPOSED, not stamped — mechanics per docs/contributor-standards.md:143 reserve ADR ratification to maintainers), rescoped to what actually ships in PR-2 (#2761) now that the round-11 M1 split moved the completion-seam threading (isPhaseArtifact/scopeToPhase, phase-id.cts:964-1090) out into a separate, stacked follow-up PR referenced generically via epic #612: - state.cts read-tolerance (both total_phases derivations, the #1514 retirement filter) moves into PR-2's row, alongside milestone.cts, reflecting the round-11 fix above. - the write-observability point is restated in terms of what actually ships: explicit threading at two named call sites, pinned by tests, rather than silent inheritance. - the completion-seam threading is named and explicitly excluded from PR-2's scope, with a new ratify item (5) and a Negative consequence bullet covering the sequencing cost. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2761): repair round-11 response gaps — disk-side sentinel bug, test-fixture bug, scanMilestonePhaseIds caller drift Four independently-verified gaps in the round-11 repair response: 1. isSentinelPhaseId (src/phase-id.cts) treated a bare, untagged phase directory under phase_id_convention: "bracket" (`0-bootstrap`, no `{CODE}.{MM}-` prefix) as sentinel milestone 0 by falling through to the legacy leading-int rule when the bracket-tag match failed. This silently dropped a real, on-disk, milestone-declared phase directory from `listMilestonePhaseDirs` (phase-locator.cts:424, the only unguarded call site) and undercounted completed_phases/percent. Mirrors the carve-out already present on both heading-side counters (state.cts's countRoadmapPhaseHeadings guards its bare-0 exclusion with `bracketId &&`; roadmap-parser.cts's scanMilestonePhaseIds composes the bare-token rule as 999-only) — under bracket convention, milestone 0 is expressed only via an explicit bracket tag, so an untagged leading 0 is a real phase token. 2. tests/adr-612-bracket-phase-counting.test.cjs's writeProject fixture hardcoded `01-VERIFICATION.md` for every "complete" phase dir regardless of the dir's real phase token. Under legacy convention this file already correctly failed #3511's strict isPhaseArtifact match for any dir other than phase 01 (the fixture never modeled what it claimed to); under bracket convention the pre-existing ambiguity fail-safe admitted it regardless. The two readings' disagreement was mistaken for a missing completion-seam threading (isPhaseArtifact/scopeToPhase convention awareness, correctly split out to the stacked #3644 per round-11's M1). Replaced the hardcoded name with verificationNameFor(dir), deriving the real per-phase filename from the production normalizePhaseName the same way cmdScaffold does — the 7 failing "flat-legacy-twin" / CHARACTERIZATION assertions pass on #2867's own code with no seam threading required, and the genuine seam-dependent cross-phase-stray-exclusion test the fixture fix would otherwise have hidden lives on the stacked branch instead. 3. tests/roadmap-parser.test.cjs's two #3577 tests still called scanMilestonePhaseIds with the old single-Set return shape; this PR changed it to `{ ids, qualifiedIds }` for every other caller but missed these two, which don't touch #2761/bracket code at all. TypeErrors at runtime, not silently-wrong assertions. Updated both call sites. 4. .changeset/2761-bracket-read-tolerance.md gains a paragraph disclosing fix 1 above, so the changeset stays accurate to what actually ships (the round-11 BLOCKER was exactly this changeset going stale once). Verified: tests/adr-612-bracket-phase-counting.test.cjs + tests/continuation-grammar-parity.test.cjs + tests/roadmap-parser.test.cjs = 354/354. adr-612-{coherence,grammar,heading-selection,read-tolerance, selection.property}.test.cjs + collision-characterization = 287/287 (CONFIRMED-CLEAN set, unaffected). Full unfiltered suite run separately. PR #3643 (round-11's M1 split, opened draft per that round's explicit requirement) was auto-closed by this repo's own draft-PR policy 11 seconds after opening — draft PRs are unconditionally closed here. Reopened as non-draft #3644 (same branch, same commits, DO NOT MERGE / stacked-on-#2867 marker in the body, enhancement template) since the repo's own bot confirms non-draft contributor PRs are tolerated even off-template. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2761): narrow the state-update-progress pin claim to what mutation testing actually proved, add the test that covers the rest Round-11 BLOCKER response gap (verified, not a guess): the changeset and ADR-612 amendment point 2 both claimed the round-11 tests pin `cmdStateUpdateProgress` so "a future change to the enumerator's default cannot silently move ... what state update-progress reports without failing a test." Mutation testing disproves this. Reverting BOTH the state.cts explicit `phaseIdConvention` thread AND simulating the phase-locator.cts pre-#612 hardcoded-null default (`phaseIdConvention: null` at that one call site) leaves the existing PIN test ("writes a real percent for a bracket-scoped milestone") green, because that percent comes from `computeUpdateProgressPreview` -> `buildStateFrontmatter`, a separately and already-correctly-threaded derivation the state.cts inline comment at ~:965 already candidly documents. What the reverted thread DOES change, empirically, is `phaseDirs`/`totalPlans` — the enumerated `.value` this call site feeds into the #3233 zero-plans no-op gate a few lines below. That is the one place a regression at this call site is observable in the command's output. Added a test that pins exactly that: a bracket milestone whose declared phases carry no plans on disk, alongside a decoy directory that plainly does not belong to the milestone (no bracket tag, no phase token) but does have a plan. Correctly scoped, the decoy is excluded and the #3233 no-op fires (`updated: false`). Degraded to the pass-all legacy reading, the decoy is swept in, `totalPlans` flips nonzero, and the no-op never fires (`updated: true`). Mutation-tested against both scenarios: - Reverting ONLY the state.cts explicit thread (falls back to `undefined`, which `getMilestonePhaseFilter` resolves via the identical `resolvePhaseIdConvention(cwd, undefined)` call the explicit thread also makes): new test stays green — confirms the single-hunk thread really is pure single-derivation hygiene, exactly as the existing inline comment claims, for this test too. - Forcing `phaseIdConvention: null` at that call site (the combined-revert scenario the round-11 mutation testing actually exercised): new test FAILS (`updated: true, percent: 0` instead of the expected `updated: false`). Restoring the real code makes it pass again. Narrowed the changeset and ADR-612 point 2 prose to match: `cmdMilestoneComplete`'s enumerated set is pinned against a pass-all-degrade regression (genuine, already verified in round 11); `cmdStateUpdateProgress`'s own call site is now pinned against the #3233 zero-plans no-op specifically, not against the reported percent, which the prose no longer claims. Added a one-line pointer to the new test in the state.cts inline comment. No production behavior changes — test and documentation only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * docs(#2761): reconcile ADR PR-2 module map * fix(#2761): restore #3639's dir-aware W007 sentinel exclusion lost in the rebase replay The rebase replayed this file's pre-#3639 patch over next, reverting the isSentinelPhaseId(token) -> isSentinelPhaseDir(dirName) fix: the extracted token is milestone-stripped, so a bracket sentinel (GSD.999-07-icebox, GSD.00-01-backlog) was invisible to the id predicate and false-fired W007. Restores upstream's call and comment verbatim; the branch's convention-aware token remains for the diagnostic message only. Caught by next's own #3639 regression tests (CI shard 2). Local: the file's 18/18, health-diagnostic-rules 165/165, full npm test exit 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MeAsZNfhiUQioFPA4ygEGS * docs(#2761): ratify scope and correct surface claims * test(#2761): strengthen legacy selection and drift claims * docs(#2761): correct the enumerator claim, scope list, and ratification receipt Three documentation corrections, none touching production code. The changeset claimed the shared phase-directory enumerator "now defaults its convention argument to 'not yet resolved' rather than 'resolved, and not bracket'". That is false: `phase-locator.cts:375` still destructures `phaseIdConvention = null`, and the lazy resolve-from-config fires only on `undefined` (`roadmap-parser.cts:941`, `:1928` — whose own comment records that "explicit null still means 'resolved and non-bracket'"). Measured: only 4 of 17 `listMilestonePhaseDirs` call sites thread a resolved convention (`milestone complete` and `state`'s three). The changeset went on to name `progress`, `stats`, `phase list` and the init manager view as now receiving a correctly scoped set — those are precisely callers that omit it. Replaced with what the code does, and the deferral stated plainly. The ADR's PR-2 row omitted `scripts/lint-phase-id-drift.cjs` (+141/-14) and `scripts/lint-phase-enumeration-drift.cjs`, both changed by this PR. An under-claim rather than an over-claim, but the row is the epic's scope-of-record. Ratify item 5 asserted maintainer ratification while citing only the review that raised the question. It now cites the review that granted it (#2867 review `5012940978`, 2026-08-24) and quotes its terms, so the claim carries its receipt. Found by an adversarial review pass over the round-12 diff. (#2761) * fix(#2761): drop the no-op --json that strict argv now rejects Surfaced by the rebase onto next, not by a change in this PR's subject. #3884 ("failure is a value — strict argv", e20744eac) made an unrecognized flag a hard error: `state validate --json` now exits 1 with `unknown flag "--json"; accepted: --strict` on stderr and EMPTY stdout, where the token was previously accepted and ignored. `state validate` never had a `--json` flag — JSON is its only output shape — so the argument was a silent no-op from the start. The helper parsed that empty stdout, so all five subtests in the "state validate resolves bracket phase DIRECTORIES" suite failed identically with `SyntaxError: Unexpected end of JSON input` at the JSON.parse, masking what they actually assert. Dropping the token restores the same envelope the helper already parsed. No assertion changes. Verified against the fixture the suite builds: exit 0, and the output carries the S005 plan-count warning the drift assertions require with no S004 phase-directory warning — i.e. the bracket directory resolves, which is the behaviour these tests exist to pin. 94/94 in the file. * fix(#2761): exempt the phase-counting suite from the docs-guard lane Surfaced by the rebase onto next. #3753 (107eb8c1d) added lint-docs-guard-registration, which requires every test file that reads a docs/ path to be either registered in the docs-guard lane or carry an explicit marker. It flags adr-612-bracket-phase-counting.test.cjs, which reads no docs/ path at all. The file's only docs/ occurrence is the ADR-612 Decision 1 citation in a line comment; every read call it makes targets a tmpdir .planning fixture. It trips Detector 3, whose DOCS_TEMPLATE_LITERAL_RE sees an odd prose backtick in the comment block above that citation as opening a template literal and reads the span between them — citation included — as a docs/ path expression. That detector documents this trade in its own header: it favours recall, and says a false positive costs one docs-guard-exempt marker with a reason. The baseline records the same class ("comment-only mentions") for 46 of its entries. Registration was the wrong side of the trade here: it would run this suite in the docs-guard lane on docs/ changes whose content it never reads. Three pieces, matching what the gate requires and what its 54 existing entries already do: - the marker in the file's header window, written without backticks so it cannot itself disturb the parity tracking findExemption does; - the basename in DOCS_GUARD_EXEMPT_BASELINE, since the ratchet fails a new marker until the baseline is deliberately updated — the reviewable diff is the point; - the FIX 3 per-file fingerprint, derived with the lint's own extractDocsPathReferences rather than retyped, so the exemption fails loudly if the set of docs/ paths this file mentions ever changes. Gates: lint-docs-guard-registration 0 violations, ci-docs-guard-registry 51/51. * docs(#2761): correct the bare-0 comment and state what opting into bracket costs Round-13 review items Minor 2 and Minor 3, both still live on the previous head. Minor 2 — src/phase-id.cts. The comment block claimed "Bare `0` is admitted alongside it because a 0.x sentinel is a legitimate identity that predates padding", which contradicted both the shipped constant and its own next paragraph. BRACKET_CANONICAL_NUMERIC_SOURCE is `(?:[1-9]\d{2,}|\d{2})`; measured against it, `0` is rejected while `00`, `05`, `99`, `100` and `999` are admitted. The paragraph four lines below already recorded the removal ("the earlier `(?:\d{2,}|0)` ... admitted ... a bare `0` that pad2 never produces"), so the block asserted and denied the same fact. The stale sentence is replaced with what ships: `00` is the backlog sentinel's canonical padded identity, `\d{2}` already covers it, and nothing needs the unpadded spelling. No behaviour change — the constant is untouched. Minor 3 — docs/CONFIGURATION.md. The phase_id_convention row described the read-path widening but never said what a repo GIVES UP by opting in. Added: on a bracket repo a heading whose bracket is followed directly by a digit is read as a phase heading, so `### [RFC.2119] 5:`, `### [v1.0] 2024:` and `### [ADR.612] 3:` — legal prose headings under any other convention — are claimed as phases and move phase_count, total_phases and W006. Those three shapes are the ones phase-id.cts's own selector comment names as the reason the widened read is selected at construction time from this value rather than applied everywhere; the row now carries that trade instead of only its upside. Gates: tsc --noEmit exit 0, npm run lint:ci exit 0, npm run test:unit 33890/33891 with the single failure being emitted-attribution's base drift against a next that moved after the rebase (133/133 against the rebase base; this push re-bases onto the current tip, which resolves it). --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-31 14:03:48 -04:00
Tom Boucher	80de48c319	enhance(#3914): every phase records a truthful guard ledger (#4018) ... * fix(#3914): retire n/no-process-exit where its successor governs Epic #3889 criterion 5 — no phase closes with a guard added and its predecessor left standing — is violated in the tree by the epic that wrote it. local/require-registered-exit was registered on gsd-core/bin/**/*.cjs and scripts/**/*.cjs, while n/no-process-exit stayed 'error' over a nine-glob block covering those same two. Only the hooks 'off' exemption ever came down; the predecessor's registration never did. Both rules have been enforcing the same property on the same surfaces since P6. Narrowed, not deleted. Seven of those nine globs have NO successor — eslint-rules/, bin/lib/, pi/, examples/, vscode/, .kilo/, .opencode/ — so deleting the rule outright would silently drop enforcement on all seven. That is the inversion this epic has already hit three times: removing a coarse guard because a narrower one exists somewhere it does not reach. Flat config is last-match-wins and both successor blocks come after the nine-glob block, so 'n/no-process-exit': 'off' in exactly those two retires the predecessor precisely where the successor governs and nowhere else. The successor is strictly more precise: it permits process.exit only inside terminateNow in cli-exit.cts, the single sanctioned terminator (ADR-3889 §3), where n/no-process-exit permits none and would flag terminateNow's own generated copy. Asserted at the consumer's altitude via ESLint.calculateConfigForFile on real paths, with the positive control that matters: n/no-process-exit is still 'error' on six of the seven successor-less globs, so a future edit that turns this into a blanket disable goes red. bin/lib/ has no file in this checkout and is reported as untested rather than given an invented path. Severity is normalized across the string/numeric/array forms the API can return, and the normalized value asserted — not truthiness. Verified by running calculateConfigForFile myself on both superseded globs and four controls before trusting the test. Found and fixed inline: the change made an eslint-disable directive at gsd-tools.cjs:257 partially unused, which --max-warnings 0 rejects; narrowed to the one rule still in force. Verification runs on the remote runner. Refs #3914 * docs(#3914): the epic added three guards, it did not remove one The audit reconciled the epic ledger against what actually landed. The net is +3, not -1: four lint:generated-sync --check arms (gen-scripts-cli-exit, gen-hooks-cli-exit, gen-exit-code-registry, gen-exit-code-docs) plus one rule, against two retirements. An epic whose thesis was consolidation ended with a larger guard surface than it started with. The additions are each defensible; the claim that the total fell was never true. Two of the three prior errors in this amendment are mine. It said "Net -1 by count" above terms reading -1 -1 +1 +1 +1, which sums to +1 — an arithmetic error in the paragraph directly below the sentence arguing that an ADR about honest accounting must not pad its own ledger. And the term list omitted two of the four --check arms, which is what turns that +1 into the real +3. Recorded rather than quietly rewritten. This ledger has now been wrong three times — the original -2, the -1 that replaced it, and #3914's own table, which states -1 above terms summing to 0 — and a written claim nobody checked against the thing it describes is the exact failure this epic exists to close. Refs #3914 * fix(#3914): make the successor actually supersede before retiring the predecessor An isolated security review found that the previous commit turned off a guard that was still doing work. Reproduced by executing both rules against a fixture, not inferred: const exit = 'exit'; process[exit](1); n/no-process-exit flags it; local/require-registered-exit did not, because it early-returned on callee.computed. So retiring the predecessor on gsd-core/bin/**/*.cjs and scripts/**/*.cjs un-guarded that shape on precisely the two globs this epic's exit contract cares most about. This is the third time in this epic I have removed a coarse guard on the claim that a narrower one covered it, without checking construct-level parity — after the allowlist key-to-prefix-to-exact-membership sequence and the band ranges-to-categories one. The rule is the same every time: a narrower guard supersedes a coarser one only where it demonstrably reaches at least as far, and "demonstrably" means executing both against the constructs, not reading either. The successor now resolves computed property access for the statically determinable cases — a string Literal, and an Identifier bound once to a string Literal, resolved through scope — and leaves genuinely dynamic properties alone so the rule does not over-fire. Measured after the fix: plain process.exit flagged, process['exit']() flagged, process[exit]() flagged, process[globalThis.k]() not flagged. That makes it a strict superset of the predecessor on these globs, since process['exit']() was caught by NEITHER rule before. The second finding is worse than the first, because it was reasoning rather than oversight. My justification comment claimed n/no-process-exit "would flag terminateNow's own generated copy here". It would not — that file is in the global ignore list, so neither rule ever lints it. There was no conflict to resolve; I wrote a rationale I had not checked, in a change whose entire subject is written claims nobody verified. Both comment blocks now state the real basis. The tests that should have caught this asserted only rule SEVERITY per glob and never construct REACH, which is exactly how a coverage hole passed. A parity matrix now pins all five shapes, including a RED/GREEN regression pin against an inlined reproduction of the pre-fix rule — inlined rather than loaded from HEAD, because HEAD resolves to the fixed commit under the remote runner and would silently stop testing anything. Verification runs on the remote runner. Refs #3914 * fix(#3914): the two exit rules are complementary — keep both Reverts this branch's retirement of n/no-process-exit. The premise was wrong twice, and the second review proved the change itself was wrong. I claimed local/require-registered-exit was a strict superset on gsd-core/bin/**/*.cjs and scripts/**/*.cjs. Measured, successor vs predecessor: function f(exit) { process[exit](1); } 0 vs 1 let exit='exit'; exit='exit'; process[exit]() 0 vs 1 const { exit } = ...; process[exit](1) 0 vs 1 plus for-of bindings, let-then-assign, var redeclaration, catch params, and an undeclared global named exit. The predecessor matches any identifier NAMED exit however it is bound; the successor resolves only a string literal or a single-write const. It never was a superset — I asserted the relationship after fixing one construct and did not re-check the rest. The justification was independently false: all three generated cli-exit copies are in the global ignore list, so n/no-process-exit was never flagging terminateNow. There was no conflict to resolve. I wrote a rationale I had not verified, in the phase whose subject is written claims nobody checked. So criterion 5 does not apply to this pair. They are not predecessor and successor — they are complementary, each catching constructs the other misses. The epic's criterion assumed a replacement relationship that does not exist here, and retiring either rule loses real coverage. The ADR ledger now says so with the measured shapes. What survives is the genuine improvement: the computed-property strengthening. local/require-registered-exit now catches process['exit'](1) and optional-chain terminators like process?.[k]?.(1), which NEITHER rule caught before, while correctly ignoring a genuinely dynamic property so it does not over-fire. The parity tests are rewritten to assert what is true rather than what I wanted to be true: a bidirectional matrix where each rule is shown catching shapes the other misses. The previous matrix tested only the four shapes where the successor wins, which is precisely why the regression shipped — a test set selected to confirm the thesis. Also corrected: a stale ADR sentence claiming a third wrong ledger version that does not exist (the table it described now reads +3 over terms summing to +3), and a changeset whose stated motivation was the false generated-copy conflict. Verification runs on the remote runner. Refs #3914 * fix(#3914): the exemption term was a no-op — the net is +4 Fourth correction to this ledger, and a fourth error of the same kind. Every version counted removing the n/no-process-exit 'off' entry from the hooks block as -1. Measured: calculateConfigForFile returns undefined for that rule on hooks/**. It was never registered there, and no broader block sets it globally, so the 'off' entry overrode nothing and removing it changed no enforcement at all. A no-op removal, not a guard removal — the same category error as counting baseline acknowledgement entries: a thing that is not a guard, in guard units. It is misattributed too; that block came down in d98b55562 (#3910), already on next before this branch existed. So the epic added FOUR guards, not three. This surfaced from a test of mine that overclaimed. I asserted n/no-process-exit was error on "all nine CommonJS/hook globs" — but hooks is not one of the nine, and the rule resolves to undefined there. Fixing the test to match reality is what exposed the ledger term, which is the argument for tests that assert identity rather than a comfortable shape. The hooks state is now pinned explicitly rather than glossed: n/no-process-exit unregistered, local/require-registered-exit error. It is mildly surprising and therefore worth a test. Also updates a pre-existing test that documented the old name-based-only boundary as intentional. The computed-property strengthening deliberately moves that boundary — process['exit'](0) was caught by NEITHER rule before — so the test now asserts the new contract and cites the ADR, rather than being left to fail or the rule weakened to satisfy it. A contract change should read as deliberate in the test that pins it. Verification runs on the remote runner. Refs #3914 * chore(#3914): backfill changeset pr number to 4018 --------- Co-authored-by: sim <sim@local>	2026-08-29 00:53:33 -04:00
Tom Boucher	ac7587287b	fix(#3812): document how Current Position actually resolves a duplicate field (#4017) ... * docs(#3812): say that Current Position is single-valued, and pin the behavior that makes it true #3812 shipped CLOSED with half its acceptance unmet. #3873 delivered cardinality for FRONTMATTER keys - current_phase/current_plan render as optional at docs/reference/state-md.md:89,91, covered by tests/gen-state-md-docs.test.cjs:374. The issue's actual ask was the ## Current Position BODY section, and that never landed. Surfaced by an /adr-phase-coverage audit of epic #3473; the issue was reopened rather than noted. The section now states three things: every field is single-valued, the section is overwritten rather than appended to, and a duplicate resolves to the FIRST occurrence with no warning - so a line appended in good faith is silently ignored rather than winning. Progress history belongs in ## Performance Metrics, two headings down, and the text now points there. The third claim is a behavioral promise about the reader, so it was VERIFIED BY EXECUTION before being written rather than inferred from the issue title: stateExtractField(<"Phase: 1 of 5 (First)" ... "Phase: 9 of 9 (Appended later)">, "Phase") -> "1 of 5 (First)" The mechanism is state-document.cjs:405 - the plain-line pattern ^<field>:[ \t]*(.+) carries flags im with NO g, so String.match returns the first hit. Writing "first wins" without running it would have repeated the exact error I had to retract twice in this epic already. A test pins the reader, not the prose. Three rows in tests/state.test.cjs: T1 (load-bearing) asserts the duplicated case resolves first; T2 asserts the ordinary single-field case still works, so a fix that only functions when duplicated cannot pass; T3 puts a Plan: line BETWEEN the two Phase: lines and asserts it resolves independently - negative space, because a reader returning the first line of the SECTION rather than the first matching FIELD would satisfy T1 alone. Proven to discriminate: a last-match variant returns "9 of 9 (Appended later)" and T1 reds. No assertion checks that the document contains a sentence. That is what local/no-source-grep exists to stop, and it would pin wording that is allowed to improve. The point of the test is that if that regex ever gains g and a last-match walk, the test fails - instead of the documentation quietly becoming a lie with nothing to notice. Prose only, no new heading. docs-state-md-locale-parity compares heading-level sequences by LCS rather than text, so added paragraphs cannot fail it while an added HEADING would fail all four locales. The constraint is structural, not stylistic - confirmed by running that comparison after the edit. The four locale copies are translated rather than left stale. They are not gate-enforced for prose, so "nothing fails" was available and is not the same as correct: leaving four documents asserting something the English one now contradicts is a correctness problem. Code spans and the anchor link stay untranslated - they name real tokens. The whole approach rests on one fact, checked first: ## Current Position at :196-208 sits OUTSIDE every generated marker region (:81-104, :138-151), so a hand edit survives --write. Re-confirmed after all five edits - gen-state-md-docs --check reports all 6 targets up to date. Had that been false the fix would have belonged in the generator, and a hand edit would have been silently reverted. One real gate failure fixed inline rather than reported: the new test's comments referenced docs/reference/state-md.md, which was not in that file's registered exempt-docs paths, and lint-docs-guard-registration failed lint:ci correctly. Registered. Known limit, named rather than folded in: gsd-tools validate/health still do NOT warn on a duplicated Phase:. #3812 records that as a "consider", not a requirement, and confirms none of the nine rules in src/health-diagnostic-rules/{state-consistency,phase-structure}.cts counts occurrences. Documenting the silent first-match is the delivered scope; making it loud is new scope and stays unclaimed. Closes #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3812): the rule I documented was false — replace it with the measured one An isolated review returned two blockers. Both mine, and the first is the worse kind: I wrote a falsifiable rule into a reference page and got it wrong. 1. "A duplicate resolves to the FIRST occurrence" is FALSE. stateExtractField (src/state-document.cts:401-419) tries BOLD `**F:**` across the whole input, THEN plain `^F:`, THEN a pipe-table row. Form precedence beats document order. Measured against the built reader, all intra-section: Phase: A (plain) / **Phase:** B (bold, later) -> B LATER WINS Phase: A (indented) / Phase: B (plain, later) -> B LATER WINS Phase: A (plain) / | Phase | T (table) | -> A first wins My original verification tested plain-versus-plain, saw first-wins, and generalized to all forms. Measuring one case and claiming the general rule is the same error I have had to retract twice already in this epic. It is also worse than silence. The sentence told authors an appended line is safely ignored; a bold line appended "for emphasis" silently overrides the original. Someone trusting the doc would have corrupted their own state file. And #3812 never asked for a resolution rule - it asked for single-valued, overwrite-not-append, and where history goes. The rule was my unrequested addition. Replaced with the measured truth: resolution is by FORM (bold anywhere, then plain at line-start, then table row), and only WITHIN the winning form does the first occurrence win. Both consequences stated plainly - a higher-ranked form wins regardless of position, and an indented `Phase:` is invisible to the plain form. All five claims in the new paragraph verified by execution before being written, including the two I had wrong. 2. The tests tested the wrong case and passed for the wrong reason. T1/T3 put the second `Phase:` under `## Somewhere else` - the INTER-section case, which #2956 already fixed by scoping. #3812 says verbatim that #2956 "fixed the inter-section case and never addressed intra-section duplication", so the case the new prose describes was untested, and the fixtures passed because of section scoping rather than field resolution. They also called bare stateExtractField rather than the production chain, T2 could not discriminate first from last at all, and no fixture mixed forms - which is precisely why the false claim survived to review. Rewritten as four rows, all intra-section, all through the real stateCurrentPositionSlice -> stateExtractField path: plain-then-plain (first wins within a form), plain-then-bold (the bold LATER value wins - the row whose absence let the false claim ship), indented-then-plain (indented invisible), and sibling-field independence. Each proven to fail against a reader that disagrees. 3. Two dead anchors. pt-BR and zh-CN linked `#performance-metrics` while their own headings are `### Métricas de Desempenho` and `### 性能指标`. Both fixed to the anchor their own heading generates. ja-JP/ko-KR kept the English heading, so theirs already resolved. 4. A ja/ko sentence inverted its own meaning. Both rendered "which is the section designed to grow" with a bare demonstrative whose nearest referent read as Current Position - saying the opposite of the point. Rewritten so the clause attaches unambiguously to `## Performance Metrics`. 5. Cross-locale drift, flagged by the implementing agent rather than by me: after fixing EN, the four locales still stated the OLD false rule. Four documents asserting something measured to be wrong is worse than four saying nothing. All four now carry a faithful translation of the corrected paragraph, with code spans, each file's own anchor, and the ja/ko referent fix preserved. Verified: all five claims executed against the built reader; every rewritten test row proven to discriminate; gen-state-md-docs --check reports all 6 targets up to date, so the edits stay outside the generated marker regions; locale heading parity unaffected (prose only, no headings added); build:lib, lint and lint:ci all exit 0. Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3812): second false rule on the same page — scope the ranking to the section A second isolated review found a second false falsifiable claim, and the failure mode is the same one twice in a row: attempt 1: verified plain-vs-plain, wrote a claim about ALL FORMS attempt 2: verified bare stateExtractField, wrote a claim about THE DOCUMENT Both times the claim covered a wider surface than what was actually executed. The fix each time was not a better sentence, it was executing the surface the sentence describes. BLOCKER — "bold `**Phase:**` anywhere in the DOCUMENT wins" is false. ## Current Position / Phase: 1 of 5 + ## Archive / **Phase:** 88 bare stateExtractField(whole doc) -> "88 (other section)" PRODUCTION (slice then extract) -> "1 of 5 (in section)" #2956's section slice means production never hands another section to the matcher; a bold line in `## Archive`, or in the YAML frontmatter, is simply not seen. The ranking is real but scoped: it applies WITHIN `## Current Position`. I verified against the bare function and wrote a claim about the system. Every existing test placed its bold line inside the section, which is exactly why nothing contradicted the claim. T5 now puts a bold `**Phase:**` in `## Archive` and asserts production returns the in-section plain value, with the unscoped reader asserted to DISAGREE so the row proves the scoping rather than assuming it. BLOCKER — the changeset still shipped the ORIGINAL retracted claim. I corrected the page and left the release note saying "resolves to the first occurrence ... a second entry added in good faith is silently ignored". The note contradicted the page it announces, and the release note is what most people actually read. Rewritten to the corrected rule. MEDIUM — the concession was inverted. It read "wins even if it comes FIRST in the file", which is the vacuous direction; the surprising case, and the one the very next clause illustrates with an APPENDED bold line, is "even if it comes LAST". All four locales reproduced the inversion faithfully, so it was an EN-source defect rather than translation drift. Two sharp edges now named, both measured: a bold `**Phase:**` followed only by trailing spaces resolves to an EMPTY STRING and does not fall through to a valid plain line below (T6 pins it); and `| **Phase:** | 3 of 4 |` short-circuits to the bold form and returns the literal `"| 3 of 4 |"`. A page that teaches form ranking has to say where the ranking bites. Also fixed: all five files labelled the link `## Performance Metrics` while the heading is `### Performance Metrics`. Anchors resolved correctly everywhere; only the label's level was wrong. Every clause in the final paragraph re-verified through the PRODUCTION chain (stateCurrentPositionSlice -> stateExtractField), clause by clause, before being written: bold in another section does not win; bold in frontmatter does not win; bold appended last does win; first wins within one form; trailing-space bold yields empty. All four locales carry the same corrected rule. gen-state-md-docs --check reports all 6 targets up to date; heading counts unchanged at 20/20 across all five files, so locale heading-parity is untouched; build:lib, lint and lint:ci all exit 0. Refs #3812 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3812): backfill changeset pr number Refs #3812 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-28 20:50:15 -04:00
Tom Boucher	dd4f179672	feat(#3970): per-task external-tracker content-resolution seam (#4000) ... * feat(#3970): per-task external-tracker content-resolution seam Implements ADR-3646 (Phase 1, #3970): a `<task tracker-id="...">` attribute plus a new optional `taskContentResolver` capability-manifest field let a capability resolve a task's action/verify/acceptance-criteria/read_first/done content from an external issue tracker instead of PLAN.md's inline body. - src/plan-document.cts: parses the `tracker-id` attribute into `PlanTask.trackerId` - src/task-content-resolution.cts: new leaf module — split/find/build/resolve, with a hard-halt (throw) contract on ambiguous/failed/timeout/malformed resolution, never a silent fallback to possibly-stale inline text - src/task-command-router.cts: new `task resolve-content --plan --task-id --raw` CLI verb wiring the module into a real process exit code - gsd-core/bin/lib/capability-validator.cjs: validates the new `taskContentResolver` manifest field (feature-role only, cross-capability trackerPrefix uniqueness) - gsd-core/workflows/execute-plan.md, gsd-core/references/loop-hook-dispatch.md, docs/reference/capability-manifest.md: wire the seam into the per-task loop and document it as a new `execute:task` point outside the existing contribution/step/gate vocabulary (unconditional in autonomous mode) Closes #3970 * fix(#3970): gate checkpoint tasks out of content resolution, close trackerPrefix grammar parity gap, cover path-traversal guard Standards/Spec code-review pass on the task-content-resolution seam (ADR-3646 Phase 1) found three defects: 1. execute-plan.md's task-content-resolution bullet fired on any tracker-id-bearing task with no check that it wasn't type="checkpoint:*", contradicting ADR-3646 Decision 1 (a checkpoint task must never enter resolve-content). plan-document.cts already parses trackerId: null unconditionally for checkpoint tasks; only the workflow prose needed the fix, so the bullet now explicitly excludes checkpoint tasks. 2. task-content-resolution.cts's parseResolverDeclaration accepted any non-empty trackerPrefix with no grammar check, while capability- validator.cjs's KEBAB_RE enforces kebab-case at install time — a Generative Fix Divergence gap. Added the same grammar (as a literal regex, documented as intentionally not shared across the .cts/.cjs build boundary) plus a parity test asserting the two surfaces agree across a valid/invalid trackerPrefix table. 3. task-command-router.cts's routeResolveContent path-traversal guard on --plan had zero test coverage. Added a test exercising a ../../../etc/passwit-shaped path and asserting the USAGE rejection names the offending path. * fix(#3970): sanitize resolver diagnostics and cap resolver timeoutMs Two findings caught by an isolated security-review pass on the task content resolution seam: - ResolverFailedError/ResolverMalformedOutputError embedded raw, unsanitized subprocess stderr/stdout (attacker/model-influenced via the tracker-id argv token) into .message. A hostile or buggy resolver could smuggle a newline plus a forged "Error: " line, or terminal escape sequences, into a diagnostic io.cjs's error() writes verbatim to stderr. Fixed at the constructor (task-content-resolution.cts) via io.cjs's existing formatDiagnosticToken(), so every caller of resolveTaskContent gets a safe .message by construction. - capability-validator.cjs's validateTaskContentResolverFields had no upper bound on taskContentResolver.invoke.timeoutMs, letting a manifest declare an effectively unbounded value and defeat the "bounded subprocess" design intent. Added a 120000ms ceiling specific to this field, without touching the shared isPositiveIntegerMs() helper (still used unbounded by the reviewer lane's timeoutFloorMs and probe timeoutMs). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3970): fix gsd-test failures — stale prose allowlist line and stderr-vs-message assertion gsd-test (remote dockerized matrix) came back red with 5 failures on this PR; all five are real defects, fixed here. - tests/no-bare-gsd-tools-command-position.test.cjs: PROSE_ALLOWLIST's execute-plan.md entry pointed at line 415, which ffc190df4's checkpoint-exclusion caveat (added near line 221) shifted down by one line. The actual "validated downstream by gsd-tools uat classify-coverage" descriptive mention now sits at line 416. Updated the allowlist entry's line number to match. - tests/task-command-router-resolve-content.test.cjs: the path-traversal test asserted the outside-project-scope diagnostic against the thrown ExitError's own .message. io.cts's error() (ADR-3889) writes its human-readable message to fd 2 via writeAllSync and then throws a bare `new ExitError(1)` with no message argument — by design, so the exception carries no duplicate text and the thrown ExitError's message defaults to "process exit 1" (cli-exit.cts's ExitError constructor). Root cause was the test, not the source: task-command-router.cjs's outside-project-scope rejection already calls error() correctly and the diagnostic text is genuinely emitted, just on fd 2, not on the exception. Fixed the test to capture fd-2 writes (mirroring tests/estimate-calibrate.test.cjs's runCalibrateExpectError and this same file's own captureStdout for fd 1) and assert against the captured stderr text instead of err.message. This was masked locally because a manual `node -e` sanity check that only inspects the caught exception's .message cannot see what the real node:test run actually failed on. Emitted-Drift-Ack-Growth: execute-plan.md — adds the ADR-3646 task-content-resolution bullet and checkpoint-exclusion caveat to the per-task execute loop; a real behavioral prose addition, not incidental bloat. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3970): backfill changeset PR number (pr:0 -> pr:4000) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>	2026-08-28 13:17:04 -04:00
Tom Boucher	c5f2b94b27	enhance(#3907): gates report no-input instead of a verdict they never reached (#3932) ... * feat(#3907): gates report no-input instead of asserting a verdict they never reached The three stdin-reading gates bound 2 to a stdin read error only, with no arm for stdin closed at zero bytes - so empty input flowed into the detector, found nothing, and exited 1, which each module's own comment defines as a negative verdict. An unset PHASE_SECTION made the UI gate assert the phase has no UI. Empty and whitespace-only input now exit NO_INPUT, and a read error exits UNAVAILABLE rather than a locally-invented 2, both resolved through the registry and delivered by terminateNow. The exit code was only half of it: under --json the same input emitted {detected:false}, byte-identical to the fabricated payload #3909 exists to fix, and the blocking coverage gate reads that payload. Empty input now emits the in-tree {skipped:true,reason} form with no detected key at all. teams-status is excluded: it never reads stdin and has no invented 2, so the four-module framing in the issue and ADR is wrong. The dead root bin/lib/ui-safety-gate.cjs is deleted - no installer reference, no workflow invocation, and the live fallback chains are for other modules. Its removal restores the unit tests to the module that actually ships; they had been asserting the stale copy's two-field shape, which is why it drifted unnoticed. * fix(#3907): drive gate tests through the process seam, and make removed-but-needed basename-precise CONTRIBUTING requires every subprocess go through tests/helpers/process-seam.cjs; two of the three gate suites hand-rolled spawnSync while the third, added in the same change, used runNode correctly for the identical injection case. Converted the blocks this change added, leaving pre-existing ones alone. Deleting one of two files sharing a basename made lint-removed-but-needed report 14 references that were all to the surviving canonical module - the false-positive class its own docstring names. It now matches on the deleted file's full path when a surviving file shares its basename, which is more precise rather than weaker: a genuine full-path reference still fails, and behaviour is unchanged when no basename collides. It immediately caught a docstring on this branch that spelled the deleted path. * test(#3907): update the one existing assertion that pinned the old empty-stdin verdict A pre-existing test asserted exit 1 on empty stdin - the defect this phase removes - and was missed because the change added new blocks without auditing existing ones pinning the old contract. Audited the rest: the other three status-1 assertions in that file all feed real input and are the genuine-negative controls that must keep returning 1, so exactly one was stale. The retired 2 is gone from the describe's contract comment too. * chore(#3907): backfill changeset pr number --------- Co-authored-by: sim <sim@local>	2026-08-27 11:37:31 -04:00
Tom Boucher	e20744eacb	enhance(#3884): failure is a value — strict argv, and --pick that signals absence (#3922) ... * test(#3884): failing-first coverage for strict argv and absence-signalling --pick ADR-3473 §8.4 says failure is a value. Three families currently encode failure as success, and this commit pins each one RED before the fix lands. Measured on this tree, 2026-08-26: gsd-tools generate-slug "test" --pick nonexistent -> empty stdout, exit 0 (#3365) gsd-tools audit-open --pick nonexistent_field -> dumps the entire human-readable audit report, exit 0 gsd-tools generate-slug "Hello World" --raw --pick bogus -> prints "hello-world", another field's value, exit 0 gsd-tools query state.planned-phase 3 (positional, no --phase) -> exit 0; STATE.md's "Phase: 2 of 5 (Widget Support)" is overwritten to "Phase: null - READY TO EXECUTE" and the frontmatter gains a corrupted current_phase_name (#3358) tests/pick-flag.test.cjs:27 previously asserted the #3365 defect as the contract ("returns empty string for missing field", success === true). That assertion is replaced by the required behavior rather than deleted. The new parseNamedArgs block calls the spec-object signature that does not exist yet, so it fails today by construction. The 11 existing behavior-lock tests are left untouched here; they are corrected in the implementation commit. C1/C4 assert at the consumer's output - STATE.md's bytes - per ADR-3180 Decision 4(b). A unit assertion on the parser would have passed throughout this defect's life. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3884): failure is a value — strict argv, and --pick that signals absence Implements ADR-3473 §8.4. Absence, emptiness and failure stop being interchangeable ways to say "I could not answer". parseNamedArgs (src/command-arg-projection.cts) Takes a spec object with a REQUIRED `positionals: number | 'rest'` and returns the hub's Result shape instead of a bare Record. Declaring the positional arity is what makes #3358's call site unrepresentable rather than merely detectable: an unrecognized flag or a token past the declared boundary is now InvalidArgs, naming the offending token and listing the accepted flags. The legacy positional-array call shape throws a TypeError — an internal invariant violation per ADR-3473 Decision 2, so a stale hand-written .cjs call site fails loudly instead of destructuring undefined off a Result. parseNamedArgsOrExit projects a failure onto the caller's error(); it is a projection over the one parser, not a second parser. Measured before, against a STATE.md with a populated phase-2 block: query state.planned-phase 3 (positional, no --phase) -> exit 0; "Phase: 2 of 5 (Widget Support)" overwritten to "Phase: null - READY TO EXECUTE", frontmatter gains a corrupted current_phase_name After: exit 1, `unexpected positional argument "3"`, STATE.md byte-identical. The flag form is unchanged and still updates STATE.md. --pick <field> (gsd-core/bin/gsd-tools.cjs) extractField returns {found,value}, and the pick block no longer shares one catch between "output was not JSON" and "field was absent". An absent field exits 1 with pick_field_absent, naming the field and the keys that do exist; non-JSON output exits 1 with pick_output_not_json instead of dumping the command's entire output. A field that is PRESENT with value null, '', 0 or false still prints at exit 0 — that is an answer, not a failure, and it is what keeps `--pick count` printing 0 on a fresh project. Measured before: `audit-open --pick nonexistent_field` printed the whole human-readable audit report at exit 0, and `generate-slug X --raw --pick bogus` printed "hello-world" — a different field's value, confidently, at exit 0. ADR-3409 Decision 7 explicitly deferred this contract fix to #3473; this is it. The sub-issue's "returns 0 when the count is zero OR absent" wording is superseded by the ADR rule it implements: zero prints 0, absence exits non-zero. Defaulting absence to 0 would demote "could not answer" to "the answer is zero" — the hazard docs/how-to/resolve-unreachable-guard-findings.md already warns against. Guard ledger (ADR-3473 Decision 6) scripts/lint-unreachable-guard-drift.cjs Detector A is RETIRED. Its premise — that a `--pick ... || echo` arm can never fire — is now false, so the shape it forbade is the correct idiom and keeping it would forbid the fix. Detector B (glob-consuming cat/ls, a nullglob mechanism this change does not touch) is retained in full, as are the shared scanner, the escape-marker parser and the baseline. Net: -1 detector, 0 added. The file is not deleted. Call-site audit 45 prompt-layer --pick invocations, every one a plain X=$(...) assignment — none in an if test, && chain, or a pipeline whose status is consumed, and no shell block in workflows/commands/agents/references sets -e. Of the 13 (command, field) pairs the prompt layer reads, 10 are always present; the 3 sometimes-absent ones each sit behind a prior found/existence check. No ADR-3409-class "field the command never produces" remains. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): escape untrusted tokens in diagnostics, and cover five unpinned rows Two review findings, both fixed here rather than recorded as limits. 1. A newline in an untrusted token forged a second stderr line. Before, plain-text mode: $ gsd-tools query state.planned-phase $'foo\nError: forged second line' Error: unexpected positional argument "foo Error: forged second line" After: Error: unexpected positional argument "foo\nError: forged second line" --json-errors mode was never affected — io.error runs that payload through JSON.stringify. Plain-text mode writes 'Error: ' + message verbatim, and the three new InvalidArgs reasons plus the two new --pick diagnostics all interpolate a token that comes straight from argv. Fixed with ONE shared helper, formatDiagnosticToken (src/io.cts), applied at every interpolation site — not a copy per site. It is deliberately NOT applied inside error() itself: several callers in this tree emit intentional multi-line diagnostics, and escaping newlines there would mangle them. The available-top-level-keys list needed the same treatment for a reason the review did not anticipate: `frontmatter get <file>` reads an ARBITRARY user document and echoes that document's own keys into the diagnostic. Verified reachable — a frontmatter key containing a newline reaches the key list — so formatKeyForDiagnosticList is guarding a live path, not a hypothetical one. Ordinary keys still render plain and unquoted; a fix that merely dropped the key would also have passed a "one line" assertion, so the test pins the escaped key's presence too. 2. Five behavior-table rows were implemented but nothing pinned them: B7 a dotted path that dies partway B9 bracket syntax on a non-array B10 a negative array index, in and out of range B14 a JSON root that is not an object B17 an @file: payload over 50KB B17 is the load-bearing one. output() writes @file:<path> instead of inline JSON past 50000 characters, and --pick resolves that BEFORE parsing; with no test, a future reordering of those two steps turns every large result into a false pick_output_not_json. The fixture seeds 1200 phase directories and measures the payload at 62474 characters, asserting the spill actually happened rather than assuming it. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): correct the strict-argv surface against a full verification run The first full run came back with 90 failures across 12 files, none in the new tests. They were the argv surface telling me what it actually is. Ten root causes; each classified before anything was changed. I over-implemented, and that is reverted. ADR-3473 §8.4 says parseNamedArgs rejects "unrecognized and positional tokens". It says nothing about a value flag whose value is missing. Making that an error was my design decision, not the rule, and it broke a deliberately recorded contract: `--prd` with no value resolving to null (tests/init.test.cjs emptyPrdValueIsFalsyAndTreatedAsAbsent, row B5; tests/section-manifest-init-facts.test.cjs "flag-shaped value"). The "requires a value" branch is deleted outright rather than kept behind an option — an unused strictness mode is speculative generality. Unknown-flag and unexpected-positional rejection, which is what §8.4 actually mandates, is unchanged. --wave needed a third flag kind the original design did not anticipate. `--wave N` is documented (commands/gsd/execute-phase.md:4,48) and the shipped workflow reconstructs and passes it (execute-phase.md:84), while #2932 records token-PRESENCE semantics: the CLI cares only that the flag appeared, and the value belongs to the workflow layer. That is neither a boolean flag nor a value flag, so `optionalValueFlags` now exists — presence-only in `data`, and the validation cursor consumes a following non-flag token so it is not reported as a stray positional. Every other declared boolean flag was checked against every argument-hint and prose usage in commands/, workflows/, agents/ and docs/; `--wave` is the only one of this shape. Five tests were pinning forms that never worked. tests/adr857-core-without-capabilities.test.cjs passed `init plan-phase --phase 01-stub`, but the documented form is positional (docs/CLI-TOOLS.md:776) and the handler reads args[2] — which for that form is the literal string "--phase". Measured on the pre-fix build against a real .planning/phases/01-stub/ directory: init plan-phase 01-stub -> phase_found=true init plan-phase --phase 01-stub -> phase_found=false The test asserted only exit 0 and key presence, so it had been green while proving nothing about phase resolution. Corrected to the documented form and strengthened to assert phase_found === true. Same class in state.test.cjs (`--plan-count`, a flag that does not exist; the real one is `--plans`), milestone-archive.test.cjs (`init new-milestone --json`, silently ignored), and concurrency-safety.test.cjs (a bare positional field name whose OR-assertion passed because a whole-document dump happens to contain the substring it looked for). Six handlers had no argv validation at all — the same #3358 shape this phase exists to close, found while fixing the rest: init verify-work / phase-op / review / todos / remove-workspace read args[2] with nothing checking the rest, and validate health read --repair/--backfill through a bare args.includes() scan that bypassed the parser entirely. All now go through the seam, so the flag has one owner. tests/init-debug.test.cjs rows C4/C5 asserted that an unrecognized flag must NOT fail. That is the behavior §8.4 removes, and Decision 8 says a caller's local expectation does not override §8, so they are inverted and renamed — a test still called "ignores an unrecognized flag" while asserting rejection would be its own defect. Row C6's point is its PWNED canary; that assertion is kept verbatim and only its exit-status expectation changed, because the hostile token is now rejected rather than absorbed. The blast-radius estimate in 40-design.md is corrected rather than quietly left wrong. get_impact reported MEDIUM / 8 symbols upstream, and that was accurate for what the graph can see — parseNamedArgs's callers. It cannot see that those callers' handlers accept argv shapes wider than the code reading args[2] suggests, which is where the real surface was. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): withdraw the validate-health tightening, finish the A2/A3 revert Second full run: 46 failures, down from 90. Four causes, two of them mine. Reverted `validate health` entirely — it was scope creep, and it broke a real flag. ~30 of the 46 read `unknown flag "--json"; accepted: --repair, --backfill`. The previous commit routed `validate health` through the parser on the reasoning that a flag should have one owner. That was wrong twice over: §8.4 names parseNamedArgs and count queries, and `validate health` was never a parseNamedArgs call site — it read its flags, just not through the parser, so it had no silent-drop defect to fix. Tightening it omitted `--json`, which the health-diagnostic suites use heavily. The handler is now byte-for-behaviour back to its pre-branch form. `validate context` stays converted: it genuinely was a call site, and its `--json` is now declared rather than read by a second `args.includes` scan. The five handlers that had NO validation at all — init verify-work / phase-op / review / todos / remove-workspace — stay fixed. Those read args[2] with nothing checking the rest, which is the #3358 shape this phase owns. Finished the A2/A3 revert. Three tests still encoded the deleted "a value flag with a missing value is an error" rule, including one added by the previous commit for that rule. All three now assert the reverted null contract, and the ones whose titles said "rejected" are renamed — a test named for a contract it no longer asserts is its own defect. `--wave=` and `--wave --weird` are correctly rejected. Neither is documented in commands/gsd/execute-phase.md, gsd-core/workflows/execute-phase.md or docs/, and neither is emitted by the shipped prompt layer, so both are unrecognized tokens that §8.4 mandates rejecting. `doesNotConsumeFollowingFlagAsWaveValue` keeps the property it exists for — asserted directly now, at the parser, that `--wave` does not swallow a following flag as its value — and only its exit-status expectation changed. A contradiction inside this branch, surfaced by the audit and resolved the safe way. Two pre-existing #3573 tests call `state begin-phase '2'` and `state planned-phase '2'` with a bare positional, relying on the old permissive parser to ignore it. This branch's own #3358 regression test requires that exact argv to be REJECTED. The two are mutually exclusive. Widening the router to accept a bare positional — mirroring complete-phase — would have silently re-opened #3358, and was verified to do exactly that: with the widened router, `query state.planned-phase 3` returned exit 0 and wrote current_phase_name again. It is reverted. docs/CLI-TOOLS.md:116 and docs/COMMANDS.md:2192 document only the `--phase N` form for both verbs, so the two #3573 tests move to it. Their assertions were never about the call shape — only that total_phases survives the resync — and both still pass. complete-phase is untouched: its bare positional IS documented, and it keeps the dynamic boundary and the negative-space note that record why. The audit that produced this is in the PR body: for every handler whose declaration changed, the flags it reads anywhere in its body, the flags the shipped surface documents, and the shapes the suite passes, compared. The `--json` miss was a pattern, not an accident — declaring a handler's flags from its parseNamedArgs call alone misses whatever it reads elsewhere. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3884): backfill the changeset PR number Refs #3884 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 00:12:13 -04:00
Tom Boucher	e40e9670f8	fix(#3705): consult model_policy in the install-time bake so agent frontmatter matches dispatch (#3863) ... * test(#3705): failing-first coverage for model_policy in the install-time bake * fix(#3705): consult model_policy in the install-time bake so frontmatter matches dispatch * fix(#3705): inject the effective runtime into the policy so runtime_tiers is reached * test(#3705): use assert.doesNotMatch, the assertion that exists * chore(#3705): backfill changeset PR number --------- Co-authored-by: sim <sim@local>	2026-08-25 12:11:15 -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