b77b7f8e56863321202eb728cd3360861ad2d521
749 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
0624c5da6f |
chore(#3212): src/text-lines.cts is the sole owner of line-terminator handling — Phase 2 (#3420)
* test(#3413): failing-first suite for the line-terminator seam Phase 2 of epic #3212 (ADR-3212 §3/§6/§7). Tests only — src/text-lines.cts does not exist yet, so tests/text-lines.test.cjs fails with MODULE_NOT_FOUND at its require line, which is the intended RED. The frontmatter.test.cjs additions drive #3360 (confirmed-bug) fail-first: parseMustHavesBlock currently returns [] for every must_haves block on a CRLF-authored plan file, because \r is its own LineTerminator in ECMAScript and two /m-anchored \s* patterns can absorb it, inflating a captured indent by one character and tripping the "not nested under must_haves" guard. Verified locally against the current (unfixed) compiled module: both the direct repro and the silent-exit "blank line before must_haves:" variant return [] today. A parity property test (crlf vs lf must deep-equal for every block name) matches a pattern this maintainer has required repeatedly for prior CRLF fixes in this codebase (Cortex-recorded, verify_intent=held). The no-crlf-fragile-split.rule.test.cjs additions lock the eslint rule's future fix-hint text (pointing at splitLines()) and its self-reference non-violation (the seam's own correct \r?\n split must never flag itself). Design: .gsd/phase/chore-3413-text-lines-seam/40-design.md Test matrix: .gsd/phase/chore-3413-text-lines-seam/50-test-matrix.md * chore(#3413): src/text-lines.cts owns line-terminator handling Phase 2 of epic #3212 (ADR-3212 §3/§6/§7). Adds splitLines/normalizeEol/ detectEol/joinLines and migrates frontmatter.cts onto it. parseMustHavesBlock (#3360, confirmed-bug) returned [] for every must_haves block on a CRLF plan file. Root cause: \r is its own LineTerminator in ECMAScript, so under /m two \s*-anchored indentation lookups could match at the position INSIDE a \r\n pair and absorb the terminator, inflating the captured indent by one character and tripping the "not nested under must_haves" guard. Two silent exits, one with a diagnostic and one without (a blank line before must_haves: hits the silent path). Fixed by converting both lookups from a whole-string /m match to split-then-scan — splitLines first, then a per-line, non-/m match — the same structural pattern parseYamlRegion (30 lines away in the same file) already used safely. Nothing downstream of the two lookups changed; blockLines is now sliced from the already-split array instead of re-splitting a substring, but its contents are unchanged for LF input, and the per-line dash/kv parsing loop is untouched. A parity property test (CRLF and LF plans parse to identical must_haves for every block name) matches a pattern this maintainer has required repeatedly for prior CRLF fixes in this file's neighborhood (Cortex: 7 recorded decisions, verify_intent -> held). frontmatter.cts's other .split(/\r?\n/) call sites (parseYamlRegion, isFrontmatterShaped, sliceTopLevelFrontmatterSegments, spliceFrontmatter) are rerouted onto splitLines — a literal 1:1 substitution, zero behavior change, since splitLines IS that same regex plus a type guard. The 4 scripts/normalizeLineEndings copies (gen-registry, gen-loop-host- contract, gen-capability-registry, gen-context-index) are deleted and rerouted onto normalizeEol, which strips a bare unpaired \r exactly like the deleted copies did (not just \r\n pairs) -- verified against each script's own --check mode against its real generated output. local/no-crlf-fragile-split widens from tests/ to src/**/*.cts, with its fix-hint message now naming splitLines() instead of the raw regex -- the prohibition finally has a primitive to point at. Detection logic unchanged in this phase (deliberate scope limit, see design doc Known limits: the rule doesn't yet recognize safeReadFile/platformReadSync as a content source, and has no detector for the \s-adjacent-to-anchor shape that is #3360's actual mechanism -- the CLASS is converged by the direct fix + regression test regardless). joinLines/detectEol are NOT wired into frontmatter.cts's own write path (cmdFrontmatterSet/Merge -> platformWriteSync) -- verified that platformWriteSync already, unconditionally converts CRLF->LF on every .md write today as a pre-existing policy owned by a different module, and ADR-3212's backward-compatibility clause rules out a file-format change in any phase. Stated explicitly in Known limits rather than left for a reader to discover. Six-gate ripple: .gitignore, eslint.config.mjs (src/**/*.cts block), docs/INVENTORY.md + INVENTORY-MANIFEST.json (regenerated), CONTEXT.md glossary (Text Lines Module, mirroring Phase 1's Pattern Module entry). Design: .gsd/phase/chore-3413-text-lines-seam/40-design.md Test matrix: .gsd/phase/chore-3413-text-lines-seam/50-test-matrix.md * fix(#3413): fix 13 pre-existing CRLF-fragile splits the widened rule found Widening local/no-crlf-fragile-split from tests/ to src/**/*.cts (the previous commit) immediately surfaced 13 real, pre-existing violations across 10 files -- undetected until now because the rule never scanned src/. This is the exact defect class ADR-3212 exists to close, playing out again one phase after Phase 1 hit the same shape ("the new lint rule -- once live -- found 27 more"). Per CLAUDE.md's no-defer rule, fixed inline rather than deferred or suppressed; there is no established suppression convention for this rule in src/ and inventing one now would undermine the point of widening it. audit.cts, broken-windows.cts, core-utils.cts, init.cts, milestone.cts, phase.cts (x3), profile-output.cts, roadmap.cts (x2): bare-\n splits or regex character classes widened to \r?\n / [^\r\n], each following the same pattern already established migrating frontmatter.cts. phase-estimation.cts: `\r?(?:\n|$)` restructured to `(?:\r?\n|\r?$)` -- already semantically CRLF-safe, but the rule's lexical scanner doesn't recognize \r? guarding a group (only \r? immediately before a literal \n). Verified the two forms are equivalent across all four EOL/EOF cases before restructuring, not assumed. roadmap-upgrade.cts needed two coupled sites, not the one flagged line: computeMigrationPlan and applyMigration must agree on line representation for the lines[edit.lineIndex] === edit.from equality check to hold, and the write-back needed joinLines + detectEol -- a plain lines.join('\n') was silently flattening a CRLF ROADMAP.md to LF wholesale on every migration. This is the first real production consumer of joinLines/detectEol in this epic (frontmatter.cts's own write path doesn't use them -- see the previous commit's Known limits). Fixing the 13 flagged sites surfaced 4 more adjacent same-shape sites the rule doesn't track (.search() and new RegExp(dynamicString) aren't in its tracked call/construction set). Investigated each empirically -- hand-tracing this exact bug class already produced one wrong conclusion earlier in this phase (a detectEol design-doc arithmetic error), so these were verified with real CRLF fixtures rather than reasoned about on paper: - audit.cts (scanTodos): REAL bug, fixed. `bodyMatch.trim().split ('\n')[0]` leaked a trailing \r into a user-visible todo summary on CRLF input -- .trim() only strips the string's outer edges, not a \r sitting mid-string before the first bare \n. Now splitLines(...) [0]. - phase.cts (cmdPhaseInsert, bullet-style branch): REAL bug, fixed. [^\n]* in targetBulletPattern swallowed a line's trailing \r on CRLF input, shifting the computed insert position to land INSIDE the \r\n pair; combined with a hardcoded '\n' bullet separator, a CRLF ROADMAP.md ended up with a mixed CRLF/LF result after an insert. Fixed with two coupled changes (either alone still corrupts, verified both ways): [^\r\n]* in the pattern, and the new bullet's leading terminator now comes from detectEol(rawContent). - roadmap.cts (cmdRoadmapAnnotateDependencies phase-boundary scan): investigated, genuinely safe, left untouched. The .search(/\n#{2,4} .../) boundary-finder and the [^\n]*-based heading match were empirically verified on a 3-phase CRLF fixture -- the only stray \r ends up at the tail of an intermediate phaseSection string that is only ever used for .test()-based idempotency checks, never for an exact-match comparison or written back to disk. No corruption on round-trip. Every fix re-verified: npm run build:lib clean, npx eslint 'src/**/*.cts' --no-cache reports 0 problems (was 13), and each fixed function's existing LF-input tests were spot-checked unchanged. * fix(#3413): apply orthogonal review findings Two isolated review engines (correctness + security) ran against the full diff and found three majors, one real security issue, and several disclosure-worthy minors. All fixed or explicitly disclosed with evidence; nothing deferred. MAJOR — detectEol's tie-break contradicted its own documented contract. Code returned '\n' on a 1:1 crlf/bare-LF tie; every doc (design doc, CONTEXT.md, the function's own comment) says ties resolve to '\r\n'. The existing test masked this by reusing the same tie fixture the buggy code happened to satisfy, rather than a genuine LF-majority case. Root cause: an Edit attempted earlier in this phase to fix this exact arithmetic error was blocked by the tier guard, and a later dispatch was incorrectly told it had already landed. Fixed: condition is now crlfCount >= bareLfCount; the test fixture corrected to a genuine 2:1 majority, with a new explicit tie-case test. MAJOR — phase.cts's cmdPhaseInsert built an EOL-aware bulletEntry via detectEol(rawContent), justified by a comment claiming a hardcoded '\n' corrupts a CRLF ROADMAP.md. False: this write goes through platformWriteSync, whose normalizeContent/_normalizeMd unconditionally converts CRLF->LF for any .md target — the templating was inert dead code, erased before the file is ever written. Reverted to hardcoded '\n', comment corrected to state the true reasoning. The separate [^\n]* -> [^\r\n]* widening one function up (a real splice-position fix, independent of final EOL) was kept. MAJOR — roadmap-upgrade.cts's stated rationale for switching onto splitLines/joinLines was wrong (both functions always agreed on line representation, before and after — the claimed equality-check risk never existed), and the change it justified introduced a real regression: forcing every line onto one dominant terminator silently rewrites untouched lines' EOL on a mixed-CRLF/LF ROADMAP.md. This write path uses raw fs.writeFileSync, not platformWriteSync, so unlike the phase.cts case above the regression is genuinely live. Fixing this took two attempts. The first attempt (revert to split('\n')/join('\n') plus a suppression comment) was correctly blocked by an agent that discovered local/no-crlf-fragile-split is a PROTECTED_RULES entry in tests/portability-rule-disable-ban.test.cjs — a hard, out-of-band, ADR-1703-governed guardrail banning any eslint-disable of this rule anywhere in src/**/*.cts. That agent also detected and correctly disregarded an injected instruction that appeared in tool output during a git operation, per this session's untrusted-content policy. The actual fix: computeMigrationPlan reverted to roadmapContent.split('\n') (confirmed lint-clean — the rule's data-flow tracking only follows a variable's initializer, and this one is declared empty then reassigned in a try block). applyMigration's write-back now splices edits against the ORIGINAL content string via indexOf('\n', pos) boundary-walking instead of a full split/rejoin, so every untouched character — including every line's own terminator — is copied byte-for-byte. A capture-group split (/(\r\n|\n)/, preserving terminators inline) was tried first and empirically confirmed to still trip the rule before this approach was chosen instead. MINOR (security) — roadmap.cts's cmdRoadmapAnnotateDependencies used the STRING form of String#replace, so $&, $`, $', $1-$9 inside must_haves.truths content (author-controlled) were interpreted as replacement directives, splicing unrelated ROADMAP.md text into the result. Fixed with the function-replacement form, which is never pattern-interpreted. Verified before/after with the reviewer's exact repro. Also disclosed rather than silently left: test matrix row 31 (four planned CRLF-materialized regression tests) was never implemented as separate files — corrected to record the actual verification (a manual --check run plus incidental existing coverage via each script's normalizeLineEndings: normalizeEol alias). parseMustHavesBlock's LF behavior was claimed byte-for-byte unchanged but the old yaml.indexOf(blockMatch[0]) substring search could match an unrelated earlier occurrence of the header text (e.g. inside a quoted value) — the split-then-scan fix incidentally also closes this, a strict improvement now recorded in the design doc rather than left implicit. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3413): checkpoint 2 red — missing eslint ignore entry, RuleTester config error Checkpoint 2 came back red with 5 failures on the reviewed sha, both gaps genuinely undetectable by any local gate. eslint.config.mjs was missing the 'gsd-core/bin/lib/text-lines.cjs' ignores-list entry (ADR-457: generated .cjs artifacts are excluded from direct type-aware linting). Phase 1's sibling entry (pattern.cjs) sits two lines above it and was the exact precedent read while researching the six-gate ripple for this module -- missed anyway. Caught by tests/repo-invariants.test.cjs's bin/lib coverage-tracking test, which only runs on the remote suite. tests/no-crlf-fragile-split.rule.test.cjs's row-32 case specified both `messageId` and `message` on the same RuleTester error assertion -- ESLint's RuleTester rejects that combination outright. This existed since the test was first authored and was never caught locally: `npx eslint` only lints the file's syntax, it does not execute RuleTester, and local `node --test` is hard-blocked in this repo -- the assertion had never actually RUN before this checkpoint. It was even present in checkpoint 1's failure list, listed there as one of the "expected RED" tests; I matched it against my expected-failures list by test NAME only and never inspected the actual failure detail closely enough to notice it was failing for the wrong reason (a RuleTester config error, not the intended message-text mismatch). Fixed by keeping `message` (the exact-text assertion the test exists to make) and dropping `messageId`. Verified the crlfFragileSplit message string in eslint-rules/no-crlf-fragile-split.cjs matches this assertion character-for-character, and swept every other invalid case in the file for the same double-specification bug (none found -- all pre-existing cases use messageId alone). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3413): add Fixed changeset for the #3360 CRLF parsing fix The sole user-visible effect of this phase. No breaking-change label or Changed fragment needed — ADR-3212's Backward Compatibility section names the Node floor (Phase 1, already shipped) as the epic's only breaking change; Phase 2 has none. * chore(#3413): backfill changeset pr number to 3420 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
dc3c81e93d |
chore(#3212): src/pattern.cts is the sole owner of runtime-value regex construction — Phase 1 (#3416)
* test(#3412): failing-first suite for the pattern-construction seam Phase 1 of epic #3212 (ADR-3212 §1/§2/§7). Tests only — src/pattern.cts and eslint-rules/no-adhoc-regex-escape.cjs do not exist yet, so both suites fail with MODULE_NOT_FOUND, which is the intended RED. Locks the measured behavior rather than the assumed behavior: RegExp.escape hex-escapes the leading character of nearly every string ("abc" -> "\x61bc"), so the suite asserts match-equivalence against an inlined historical oracle (the implementation being deleted) rather than byte-equivalence of pattern text — 200 seeded fast-check runs plus a fixed corpus, 0 mismatches. Also locks the latent character-class range bug this phase fixes as a side effect: a hyphen-bearing value interpolated into [...] currently forms a real range and matches an unintended character; post-migration it must not. * chore(#3412): src/pattern.cts owns runtime-value regex construction Phase 1 of epic #3212 (ADR-3212 §1/§2/§6/§7). Adds the pattern seam delegating to the built-in RegExp.escape, deletes every hand-rolled copy, and raises the Node floor to the Active LTS line. The census was low, three times over. ADR-3212 counted 10 copies; a graph query found 12; the new lint rule — once live — found 27 more. The difference is that the census counted named helper FUNCTIONS while the rule counts the escape SHAPE, so inline .replace(<class>, '\$&') copies were never in scope. ADR §1's actual requirement is that no module outside the seam escapes a value for regex use, so all of them are, and CLAUDE.md's no-defer rule makes them this change's work. Fourth consecutive epic here whose copy count was low — the argument for ADR-3180 Amendment 3's "state N found by the guard" rule. Also corrected mid-implementation: the survey reported phase-id.cts's escapeRegex had 0 external importers. It had 8 production importers, making its removal a public-surface change to an ADR-2121-owned module and requiring an update to that ADR's locked-surface test. Blast radius revised Medium-High -> High. RegExp.escape is match-equivalent but NOT text-equivalent: it hex-escapes the leading char of nearly every string ("abc" -> "\x61bc"). Equivalence is proven by a seeded fast-check property test against the deleted implementation as oracle. It also fixes a latent bug: a hyphen-bearing value interpolated into a character class previously formed a real range and matched an unintended character. Node floor 22 -> 24 (RegExp.escape is Node 24+), across engines, .nvmrc, package-lock, 9 CI matrix entries, and 5 docs. The aggregate `required-tests` context is unchanged and no job was added or removed, so branch protection cannot be orphaned by the dropped lanes. Enforced by eslint-rules/no-adhoc-regex-escape.cjs (shape-matched, with structural provenance for reviewed pattern-fragment constants rather than a name heuristic) plus a whole-tree companion guard covering the directories ESLint's globs miss. * fix(#3412): close the _SOURCE guard evasion, correct two false claims Three findings from the orthogonal review pass, all fixed. 1. The ESLint rule's `_SOURCE` provenance fallback was pure identifier- name matching with no binding check, so `new RegExp(userInput_SOURCE)` — a function parameter — sailed past the guard. That is the same rename-evasion class issue #3410 documents, reopened by the very fallback meant to complement the structural check. Now bound to the identifier's actual binding kind: import, require-derived const, or module-scope const; parameters, `let`/`var`, and unresolvable bindings fail closed. Four RuleTester cases cover the evasion and prove the legitimate cross-module case still passes. 2. src/pattern.cts's own header carried the stale pre-correction counts (12 copies / 17 call sites) while CONTEXT.md and the design doc carried the corrected ones (~39 / ~44) — a self-contradiction inside the PR whose entire purpose is deleting divergent copies. Rewritten, preserving the durable lesson: a named-function census cannot see inline copies; only a shape-matching guard can. 3. The claim that all deleted copies threw TypeError on non-string was false. phase-id.cts's copy — the one with 8 external importers — did String(value).replace(...) and never threw. The seam's locked signature does not coerce, so this is a real, now-disclosed behavior change rather than the pure preservation the tests asserted. Audited all 32 invocations across the 8 importers and 6 in-file callers: every one is safe by construction (upstream truthy guard or a string-producing derivation), verified by runtime probe against the compiled modules rather than by TS compilation, which cannot see a runtime undefined. Corrected the false claim in both the test comment and the design doc, and added it to Known limits. * docs(#3412): add Changed changeset for the Node 24 floor The only user-visible break in this phase. The escape-behavior change is internal and match-equivalent, so it carries no user-facing note. * fix(#3412): resolve the seam's require graph in script fixtures and packaging Checkpoint 2 came back red with 90 failures on the node24 lane. Three distinct defects, all introduced by routing scripts/ through the new pattern seam, none reproducible by any local gate: 1. ~82 failures — tests/adr-index-gate.test.cjs and tests/removed-but-needed-lint.test.cjs copy a scripts/*.cjs into an mkdtemp fixture and spawn it there (necessary: those scripts resolve their scan root from __dirname/.., so running the real script would scan the real repo). Each harness hand-listed the dependencies to copy alongside. Adding require('../gsd-core/bin/lib/pattern.cjs') to gen-adr-index.cjs made both lists silently incomplete -> MODULE_NOT_FOUND, plus 17 downstream 'did not emit parseable JSON' failures from the same crash. Fixed as a class, not an instance: new tests/helpers/copy-script- fixture.cjs walks a script's transitive static relative-require graph and copies it, so dependencies are derived and never re-declared. It throws (naming the unbuilt artifact) instead of letting the child die with a bare MODULE_NOT_FOUND. Verified for all four seam-consuming scripts: gen-adr-index, lint-removed-but-needed, gen-loop-host- contract, sync-runtime-launcher. 2. 2 failures — scripts/ ships wholesale but eslint-rules/ does not, so the new scripts/lint-no-adhoc-regex-escape.cjs would be MODULE_NOT_FOUND in a published install (#2858 guard). Excluded from the tarball, matching the existing precedent for gen-emitted- baseline.cjs, which is excluded for the identical reason, and locked with a test modeled on that one. Confirmed against a real npm pack: 890 files, 0 from eslint-rules/, and gsd-core/bin/lib/pattern.cjs present (so the other four scripts' requires are legitimate). 3. 6 failures — tests/phase-id.test.cjs asserted the literal escaped source text ('0*29', 'PROJ-42'). RegExp.escape is match-equivalent to the retired hand-rolled escaper but NOT text-equivalent: it hex- escapes the leading character and all hyphens ('0*\x329', '\x50ROJ\x2d42'). Verified NOT a behavior change — 576 match decisions across all three real interpolation prefixes, zero divergence. Those tests now compile each source into the same heading regex src/roadmap.cts's searchPhaseInContent builds and assert what matches and what does not, including the 'i'-flag canonicalization the hex escape has to preserve. Re-pinning the new literals would have rebuilt the same brittleness one layer down. Adds a test for the property the escape exists for: a dot in '1.2' must not act as a wildcard. Also shares one definition of 'a require' between the packaging guard and the fixture copier, so the two cannot disagree about what they scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): refuse to copy a fixture dependency outside the fixture root copyScriptWithDeps resolved each relative require and joined the repo-relative result onto fixtureRoot. A require resolving OUTSIDE the repo yields a '../'-prefixed relative path, so path.join climbed out of the fixture and wrote into the surrounding temp dir (verified: repoRoot=/repo + depAbs=/etc/passwd wrote /tmp/etc/passwd). No script in the tree does this today, so this closes an available escape rather than an active one. Refuses via the existing unresolved- require path so the failure names the offending specifier. Covered by a negative proof that the guard fires and that nothing lands outside the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): parse requires instead of pattern-matching them; restore the foreign-prefix contract Applies all findings from the second orthogonal review round, re-run because real code changed after round 1. HIGH (security) — extractRequires stripped BLOCK comments before LINE comments, so a '//' comment containing '/*' opened a phantom block comment, and a '//' inside a string literal truncated the line. Both hid real requires: 'const u="http://x"; require("./real.cjs")' returned [], and four real requires in gsd-core/bin/gsd-tools.cjs were invisible. Replaced with a real AST parse via espree. This is ADR-3212's own Decision 4 — tokenizer-first for stateful grammars — applied to the case it describes; comment/string/regex nesting is exactly such a grammar, which is why the regex version was wrong. The function was moved byte-identical out of the #2858 packaging guard, so the bug PRE-DATES this branch and has been a live blind spot there: a shipped script could have required an unshipped path undetected. Fixing it makes that guard strictly stronger than on next. espree is promoted from a transitive eslint dependency to an explicit devDependency rather than relying on hoisting. The script parse attempt sets ecmaFeatures.globalReturn because Node wraps CommonJS bodies in a function, making a top-level return legal — scripts/check-coverage-gate .cjs relies on it, and without the flag the guard throws on a file it is supposed to scan. Verified 0 unparseable across all 324 .cjs/.js under scripts/, bin/, and gsd-core/bin/, and 0 new violations against a real npm pack, so the exact extractor does not newly fail the guard. MEDIUM (security) — the repo-containment check guarded dependencies but not the entry path. One escapesContainment predicate now guards both. LOW (security) — containment was lexical while fs follows symlinks, and a directory symlink could mint a fresh dedupe key per level. realpath now resolves both repoRoot and each dependency before the decision, and the realpath-derived path is the dedupe key. Destination layout still uses the original repo-relative path, so copied trees are unchanged. MAJOR (standards) — the round-1 behavioral rewrite of phase-id tests lost the foreign-prefix contract: every assertion was satisfied by an impl returning [A-Z]+\x2d42, i.e. ANY project code — the exact #3599 bug class the exact-source prevents. The literal assertions it replaced were catching this. Now asserts the compiled regex REJECTS a different prefix with the same number. MAJOR (standards) — the test hand-duplicated production's heading regex with no parity guard (CLAUDE.md's 'Generative Fix Divergence'). Removed the parallel surface instead of policing it: src/roadmap.cts exports buildPhaseHeadingRegex, searchPhaseInContent calls it, the test imports it. Byte-identical .source and .flags verified for both escaped forms. MINOR — '..foo' no longer false-flagged as an escape; the inverted spurious-vs-missing doc claim corrected; the dead allow-test-rule header removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3412): backfill changeset pr number to 3416 * fix(#3412): make the escape guard's own regex linear, reword an injection-scan collision Two CI failures on PR #3416, both in code this branch added. CodeQL js/redos (high) — REPLACE_CALL_RE's outer alternation let a bracket run be consumed EITHER by the character-class branch OR one character at a time by the trailing catch-all, so a failing match explored both parses of every pair. Measured on the real regex: n=26 -> 204ms, n=28 -> 791ms, n=30 -> 3475ms, a clean 2^n. This script scans repo source, so a file with a long bracket run after '.replace(/' would hang CI outright — a guard against undisciplined pattern construction was itself the worst pattern in the diff. Fixed the way ADR-3212 already prescribes: the catch-all branch now excludes '[' and ']' so a bracket can only be consumed by the class branch (this is what makes it linear), and every quantifier is bounded (the locked bounded-quantifiers decision) as a second line of defense. Now 0ms at n=2000. Disclosed coverage tradeoff, recorded at the constant: a regex literal with a BARE unescaped ']' outside a class is no longer matched by this backstop. No census shape has that form, and the AST rule remains the primary detector. Verified the guard did not go blind doing it: a real census-shape violation is still reported, and an allow-adhoc-regex-escape suppression comment is still honored. Regression test drives the exported findViolations on a 2000-repetition adversarial input and asserts the RESULT. It makes no wall-clock assertion — elapsed-time tests are forbidden — so a regression surfaces as a harness timeout, which is the correct signal. Prompt injection scan — 'must not act as a regex wildcard' in a test comment matched the scanner's jailbreak pattern act\s+as\s+(a|an|if| my). Reworded to 'behave as'. Deliberately NOT allowlisted: silencing a whole test file over one phrase would blunt the scanner permanently, and the comment has nothing to do with injection. Neither failure was reachable from the remote runner — CodeQL and the injection scan are not in that matrix, so the sha it passed was green and still wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
6dbc124018 | enhance(#3180): the sibling validators share one envelope and one owner — Phase 12 (#3407) | ||
|
sim
|
eae2b52e4a |
fix(#3309): W020 fires on any degraded worktree scan, not just real failures
gsd-test found buildWorktreeHealthField collapsed every inspectWorktreeHealth failure reason (git_timed_out, git_list_failed, not_a_git_repo) into one UNREADABLE scope, discarding which one. The migrated checkW020 then warned unconditionally on any UNREADABLE scope — but the original (verify.cts:2202-2217) only warned on git_timed_out/git_list_failed, staying silent on not_a_git_repo (a .planning/-only fixture with no git repo at all is not a degraded scan, just the absence of one). This spuriously degraded every test fixture that isn't a real git repo. planning-snapshot.cts's worktreeHealth field now carries `reason` through instead of discarding it; checkW020 branches on it exactly like the pre-migration code did. |
||
|
sim
|
96c7ea9b35 |
fix(#3309): W023's message drops each colliding directory's status
gsd-test found the migrated W023 dropped a piece of information the original message included: each colliding phase directory's overall status (e.g. "Complete"), not just its raw plan/summary/verification counts. Adds derivePhaseStatusLabel, reconstructing the status label from already-exposed PhaseSnapshot fields (planCount/summaryCount/ complete/verificationStatus) — no new ambient I/O, no new snapshot field. Also fixes a non-conforming test fixture found while verifying: tests/health-validation.test.cjs's "05-real" fixture used a bare VERIFICATION.md, which readVerificationStatus never matches (the real convention, and every other fixture in this repo, use the *-VERIFICATION.md suffix) — the status was always reading as "missing" regardless of message formatting. Renamed to 05-real-VERIFICATION.md. |
||
|
sim
|
a0c82f2bd8 |
fix(#3309): W002/W011/W026 state-consistency regressions
gsd-test found three real regressions in the migrated STATE.md checks: - W002 didn't exempt phase refs whose only directory lives in an archived milestone (#3652) — now consults planning-snapshot.cts's archivedPhaseTokens field (added alongside this fix, shared with W006's identical need). - W011 (STATE/ROADMAP cross-validation) never fired: currentPhaseLabel only read the current template's bare "Phase:" field, silently missing legacy STATE.md fixtures that use the older bold "**Current Phase:**" field (mirrors state.cts's own resolveStatePhase fallback ladder, which the migration didn't carry over). - W026 (STATE milestone-complete vs. unstarted ROADMAP phases) had two independent defects: roadmapDeclaredPhases's milestone attribution can't see <details>/<summary>-shaped ROADMAP sections, and current- milestone resolution could go null — both silently emptied the "unstarted" set every time. Fixed by scoping ROADMAP.md to the current milestone via the same <details>-tolerant extractCurrentMilestone every other milestone-aware consumer uses, in a new dedicated planning-snapshot.cts field (currentMilestoneRoadmapPhaseIds) rather than reusing roadmapDeclaredPhases, which exists for a narrower, <details>-blind derivation (W021's own original logic) and would have regressed it if repurposed. Also fixes an unrelated drift-guard violation this same rule file introduced: its own phase-token regex was independently re-derived instead of built from the canonical PHASE_NUMBER_TOKEN_SOURCE. |
||
|
sim
|
ce57e60098 |
fix(#3309): W006/W007 miss archived phases and phase-id variant normalization
gsd-test found two real regressions in the migrated W006/W007: 1. A phase whose directory lives under an archived milestone (.planning/milestones/v*-phases/<phase>/) instead of the active phases/ dir read as "in ROADMAP but no directory on disk" — the original forEachArchivedPhaseToken(planBase, ...) fed archived tokens into the same existence check (verify.cts:2038); the migrated rule's allPhaseDirNames never included them. 2. Comparing a ROADMAP-declared phase id against a disk directory name dropped phaseVariants() normalization the original ran as a second, independent check (verify.cts:2071-2073/2092-2093) — a ROADMAP "01A" and a disk "1A-..." read as mismatched instead of the same phase, since matchPhaseDirs's own token comparison never unifies that padding/letter-suffix difference. Adds planning-snapshot.cts's archivedPhaseTokens field (mirrors forEachArchivedPhaseToken/listMilestoneArchiveDirs exactly, no new regex derivation) and a phaseVariants()-based fallback in dirsForPhase when matchPhaseDirs finds nothing. |
||
|
sim
|
041414c4ad |
feat(#3309): generate health.md's error-code and repair-action tables
Closes the issue's explicit acceptance criterion: "health.md's tables are generated rather than hand-maintained, closing the 16-vs-30+ documentation gap structurally." The published roster listed 16 codes against 30+ actually emitted; W010-W017 and W020-W023 had never been documented. Adds description/repairable as static fields on Rule (health-diagnostic-types.cts) — generation needs a fixed, human-readable summary per code, distinct from the dynamic per-instance Diagnostic.message a rule's check() produces. repairable is true only when --repair will actually apply the remedy: false for ADVISE-only rules AND for DESTRUCTIVE-risk rules (regenerateState/resetConfig), which are described but never auto-applied — matches verify.cts's diagnosticToIssueEntry semantics exactly, after fixing E004/E005's static field to agree with it (both were wrongly true, an inconsistency caught during this same commit's own review, not left for later). New scripts/gen-health-docs.cjs (--write/--check, wired into lint:generated-sync) regenerates the two tagged table regions in gsd-core/workflows/health.md from RULES (31 rules) plus the 3 pre-checks that stay outside the rule table by design (E001, E010, I010) plus a small static Effect/Risk lookup for the 6 real repair actions — including addAiIntegrationPhaseKey, live in code since an earlier phase but never documented until now. 34 error-code rows, 6 repair-action rows. The table's old "grep verify.cts for the next free number" footnote is rewritten to point at the rule table and its lint guard instead. |
||
|
sim
|
8e1ff76204 |
refactor(#3309): consolidate adviseRemedy() into the shared types leaf
adviseRemedy() was defined identically in two of the eight rule-group
files (config-validation.cts, agent-install.cts) while the other six
repeated the same {action: ADVISE, risk: NONE, args: {command}} object
literal inline ~20+ times. Found by the Standards-axis orthogonal
review (Duplicated Code smell).
Moves the one-line helper into health-diagnostic-types.cts, the leaf
module every rule-group file already imports for its enums/types, and
uses it consistently across all 8 files. Pure mechanical refactor — no
ADVISE remedy's command text, code, or action changed.
|
||
|
sim
|
03258a07a2 |
fix(#3309): applyRepairs must not count a failed repair as applied
applyRepairs pushed a diagnostic's code onto applied unconditionally after the try/catch around runRepairAction, even when the handler threw (caught, recorded in details with success:false) or otherwise failed — making applied mean "attempted" rather than "succeeded," with no test exercising the failure path. Found by the Spec-axis orthogonal review. applied now only receives a code when the repair actually succeeded; a failed attempt is still fully recorded in details (success:false, the error message) but no longer misreported as applied. Adds a regression test forcing addNyquistKey to throw (ENOENT on a config.json that doesn't exist) and asserts it lands in details, not applied. |
||
|
sim
|
1255f960db |
fix(#3309): restore W027's active-worktree exclusion
The migrated checkW027 (stale worktree) dropped the pre-migration exclusion of the CLI's own current worktree, since a Rule.check(snapshot) has no cwd access (§8.1 rule 1 forbids ambient I/O) — flagged as a disclosed regression during this phase's own design work, then confirmed as a real, fixable gap by the Spec-axis orthogonal review rather than an inherent limitation. Fixes it properly instead of accepting the regression: buildPlanningSnapshot(cwd) already receives cwd as its own input, so exposing it as snapshot.cwd is not new ambient I/O, just surfacing an existing parameter — fully consistent with §8.1 rule 2's "parsed value" allowance. checkW027 now excludes the entry matching snapshot.cwd before flagging, matching the original verify.cts:2233-2242 behavior exactly. |
||
|
sim
|
9f8bcfa3a6 |
docs(#3309): disclose hardcoded-slash-command fidelity reduction consistently
root-existence.cts (E002, E003) and phase-structure.cts (W009) hardcode a canonical hyphen-form slash command in their ADVISE remedy, same as config-validation.cts's already-disclosed W016 — forced by §8.1 rule 1 (a Rule's check(snapshot) cannot call the runtime-resolved slash() formatter, which needs cwd). Only config-validation.cts's own header disclosed this tradeoff; the other two sites had it happen without recording it in their own file. Adds the same disclosure to both, matching the established convention. No behavior change. |
||
|
sim
|
d1760e3c31 |
refactor(#3309): migrate cmdValidateHealth onto the rule table
Replaces cmdValidateHealth's hand-rolled addIssue/switch accumulation
(961 lines) with buildPlanningSnapshot -> evaluateRules -> map to the
legacy {code, message, fix, repairable} shape, bucketed by severity.
Two pre-checks (home-dir E010/I010, .planning/-root-missing E001) stay
outside the rule table entirely, per ADR-3180 §8.2 rule 4 ("no
precedence system") — building "some rules suppress others" into the
table would itself be the forbidden precedence system.
W024 (STATE.md commit-age freshness) also stays outside the table:
its committed rule is a documented permanent no-op (readStateHeadFreshness's
git-log shell-out is ambient I/O a Rule.check may never perform, and no
PlanningSnapshot field carries a commits-behind count). Migrating onto
the rule table as designed would have silently regressed 7 passing
tests in tests/health-validation.test.cjs — found while wiring this
function, kept as a real check in the wrapper instead (same I/O
license applyRepairs already relies on), fixed inline per this repo's
no-defer policy rather than accepted as a silent loss.
Ports the real repair-handler bodies (createConfig/resetConfig,
regenerateState, addNyquistKey/addAiIntegrationPhaseKey,
backfillMilestones) into health-diagnostic.cts's applyRepairs,
replacing the skeleton's stub. DESTRUCTIVE-risk remedies
(resetConfig/regenerateState) are refused by --repair — a disclosed
breaking change; repairable now means "an automatic repair will
actually run," not merely "a remedy exists to describe," so E004/E005
now report repairable:false. --backfill alone now actually triggers
backfillMilestones, fixing a latent bug where its gate was unreachable
without --repair also being set (verify.cts:2504, confirmed dead code
pre-migration).
Test updates distinguish the two explicitly-authorized behavior
changes (DESTRUCTIVE refusal, backfill-alone fix, W021->W026 split)
from preservation — every changed assertion is commented with why, and
new regression tests were added for both changes plus W021/W026
mutual independence. Drift-guard bookkeeping (bypass-baseline shrunk
to the one disclosed W024 exception, milestone-window and
phase-enumeration exemptions, test-file-count allowlist) updated for
the relocated/new functions this migration introduces.
|
||
|
sim
|
e0021a2fed |
refactor(#3309): extract health-diagnostic-types leaf module, wire RULES
Wiring all 8 rule-group files into health-diagnostic.cts's RULES array created a genuine CJS circular dependency: each group file required health-diagnostic.cjs back for the shared enums, and health-diagnostic.cjs now required the group files forward, so the enums were undefined mid-load (destructuring health-diagnostic.cjs's still-unassigned exports). Fixes it by splitting the enums/types (SEVERITY, REMEDY_ACTION, REMEDY_RISK, Remedy, Diagnostic, Rule) into a dependency-free leaf module, health-diagnostic-types.cts, that both sides import instead of each other. health-diagnostic.cts re-exports the enums for existing consumers. RULES is now the real concatenation of all 8 groups (31 codes — E001 intentionally stays a pre-check outside the table). |
||
|
sim
|
c469aeeacd |
refactor(#3309): add milestone-archive-hygiene health-diagnostic rules
W018, W019 — MILESTONES.md archive completeness and unrecognized root .md file checks, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
a56679a4ca |
refactor(#3309): add worktree-health health-diagnostic rules
W020 (3 conditions), W017, W027 — git worktree list degradation, orphan and stale worktree checks, migrated onto the frozen rule table per ADR-3180 §8.2. W027 has a documented fidelity reduction: rules have no cwd access, so it can no longer exclude the active worktree. |
||
|
sim
|
9e74f00ba0 |
refactor(#3309): add roadmap-disk-consistency health-diagnostic rules
W006, W007 — ROADMAP entries with no matching disk dir, and disk dirs with no ROADMAP entry, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
f51f00ef0e |
refactor(#3309): add agent-install health-diagnostic rules
W010 — agent install completeness across its 4 internal conditions, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
1b09fb13d3 |
refactor(#3309): add phase-structure health-diagnostic rules
W005, W023, I001, W009 — phase directory naming, duplicate phase keys, plan-summary presence, and validation-architecture checks, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
80484249df |
refactor(#3309): add config-validation health-diagnostic rules
W003, E005, W004, W008, W016, W012, W013, W014, W015, W022 — config.json existence, parseability, and field-validity checks, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
8a7ef78906 |
refactor(#3309): add state-consistency health-diagnostic rules
W024 (deliberately inert, no snapshot field yet for stale state_head), W002, W011, W021, W026 — STATE.md cross-checks against ROADMAP/config, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
7195308384 |
refactor(#3309): add root-existence health-diagnostic rules
E002, E003, E004, W001 — PROJECT.md/ROADMAP.md/STATE.md existence and PROJECT.md section-completeness checks, migrated onto the frozen rule table per ADR-3180 §8.2. |
||
|
sim
|
8c9ca3f7c6 |
refactor(#3309): add allPhaseDirNames field to planning-snapshot
W007 (orphan disk dir with no ROADMAP entry) cannot be sourced from phaseDirs, which is windowed to ROADMAP-declared phases only — an orphan dir can never appear in an already-ROADMAP-filtered set. Adds an unwindowed allPhaseDirNames field so the rule can actually fire. |
||
|
sim
|
c5543e533c |
refactor(#3309): extend planning-snapshot.cts with 8 more parsed fields
Phase 11 of epic #3180 (ADR-3180 §8.1 rule 2). PlanningSnapshot grows from 7 fields to 15: projectSections, statePhaseTokens, stateStatus, roadmapDeclaredPhases, roadmapPhaseCheckboxes, researchValidationStatus, milestoneArchiveStatus, planningRootFiles. Every field is a reused owner (buildRoadmapPhaseVariants/ buildNotStartedPhaseVariants from src/validate.cts, stateFieldValue) or a small relocation of already-working verify.cts logic (PHASE_NUMBER_TOKEN_SOURCE scanning, the checkMilestonePrefixMismatches sectionRx walk, W009/W018's file-existence checks) — never a new algorithm, and never raw document text: §8.1 rule 2 forbids exposing raw text, not exposing a parsed list or boolean derived from it once by the snapshot builder. roadmapPhaseCheckboxes deliberately reads the same ROADMAP checkbox isPhaseComplete (§7.4, disk-strict) refuses to consult — that owner decides completion and must not read it; this field only exposes what the checkbox says, for a diagnostic (W011) whose whole purpose is flagging disagreement. Not a re-derivation of §7.4, recorded explicitly to prevent that reading. Adds PROJECT_UNREADABLE to UNUSABLE_REASON (ninth #1879 site), closing a gap the implementing agent correctly flagged rather than silently leaving absent-vs-corrupt collapsed for PROJECT.md, matching the STATE_UNREADABLE/ CONFIG_UNREADABLE precedent from this same effort's prior commits. currentPhaseLabel/statePhaseTokens/stateStatus share one STATE.md read (buildStateFields) rather than three independent reads. Additive only — all prior fields and worstScope/buildPhaseSnapshot unchanged. |
||
|
sim
|
ef10bba707 |
refactor(#3309): add health-diagnostic.cts skeleton (types + evaluator)
Phase 11 of epic #3180 (ADR-3180 §8.2/§8.3/§8.5). New src/health-diagnostic.cts: SEVERITY/REMEDY_ACTION (7 members: 6 real repair actions + ADVISE)/REMEDY_RISK (NONE/DESTRUCTIVE) frozen enums, Diagnostic/Remedy/Rule types, an empty RULES table (rules land in the next commits), evaluateRules (with a duplicate-code defense-in-depth check ahead of the lint guard), and applyRepairs (the DESTRUCTIVE-risk-refusal dispatcher — §8.3 rule 3 — with stub handlers; real repair bodies port in the migration step). Six-gate .cts ripple: .gitignore, eslint.config.mjs, docs/INVENTORY.md + manifest, CONTEXT.md glossary entry. |
||
|
sim
|
6aa378b261 |
refactor(#3309): extend planning-snapshot.cts with config/agentInstall/worktreeHealth
Phase 11 of epic #3180 (ADR-3180 §8.2/§8.3/§8.5) foundation. Extends the already-merged Phase-10 PlanningSnapshot additively with three fields the upcoming health-diagnostic rule table needs and Phase 10 never required: - config: {value, scope, exists} — parsed .planning/config.json. `exists` distinguishes absent (no diagnostic, non-answer) from present-but-invalid (CONFIG_UNREADABLE diagnostic, corruption) — both collapse to scope UNREADABLE, so a rule needs the extra bit to tell "not configured yet" apart from "config.json is broken." - agentInstall / worktreeHealth — not .planning/-sourced, wrap the existing checkAgentsInstalled/inspectWorktreeHealth owners with the same arguments cmdValidateHealth already passes them, so a later migration step reads these fields instead of calling the owners itself. Adds CONFIG_UNREADABLE to src/unusable-input.cts's UNUSABLE_REASON (eighth #1879 site), mirroring STATE_UNREADABLE's exact shape from Phase 10. Additive only — the four Phase-10 fields and worstScope/buildPhaseSnapshot are unchanged; existing tests for them are untouched. |
||
|
Tom Boucher
|
3c4df10a50 | Merge pull request #3402 from open-gsd/refactor/3308-planning-snapshot-parsed-projection | ||
|
Tom Boucher
|
2283168612 |
fix(#3170): anchor milestone one-liner extraction to a summary-shaped heading (#3401)
* test(#3170): extractOneLinerFromBody must anchor to a summary-shaped heading Regression for #3170: the function matched the first heading's first bold run, so an incidental first heading (rule list, deviation notes) contributed its bold text as the milestone accomplishment. Rows 1/2 fail RED on next; rows 3/4 guard Overview recognition and the #2660 Summary-heading form. * fix(#3170): anchor milestone one-liner extraction to a summary-shaped heading extractOneLinerFromBody matched the first heading's first bold run regardless of section, so an incidental first heading (rule list, deviation notes) contributed its bold text as the milestone accomplishment written into MILESTONES.md. Iterate headings and extract from the first Summary/Overview/ Accomplishments one with a bold run, falling back to null when none exists. The #2660 Summary-heading forms and the frontmatter one-liner precedence are preserved. * docs(#3170): add changeset * test(#3170): align extractOneLinerFromBody unit fixtures with summary-heading contract The core-utils unit fixtures used generic # Title headings encoding the old 'any first heading' contract; the #3170 fix anchors to a Summary/Overview/ Accomplishments heading (the function is summary-specific). Update the heading text to Summary-shaped; the extraction assertions (bold, frontmatter strip, colon-label, CRLF, unicode) are unchanged. * docs(#3170): backfill changeset PR number (3401) --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
2538fd6344 |
refactor(#3308): add planning-snapshot.cts parsed projection per ADR-3180 §8.1
Phase 10 of epic #3180. src/planning-snapshot.cts is a new parsed projection of .planning/, composed exclusively from the already- consolidated §7 owners (getMilestoneInfo, listMilestonePhaseDirs, isPhaseComplete, scanPhasePlans, stateFieldValue, planningPaths) plus the frozen SCOPE enum. No new semantic derivation is introduced beyond worstScope, a pure combinator folding several independently-scoped owner answers into one composite signal. Adds STATE_UNREADABLE to src/unusable-input.cts's UNUSABLE_REASON (seventh #1879 site) for STATE.md exists-but-unreadable, distinct from absent. Adds scripts/lint-planning-snapshot-bypass-drift.cjs, a ratcheted drift guard (ADR-3180 Decision 4(e)) scoped to DIAGNOSTIC_RULE_FUNCTIONS (currently cmdValidateHealth in src/verify.cts only) preventing new raw .planning/ reads from bypassing the snapshot, while acknowledging cmdValidateHealth's existing 15 raw-read sites as debt owned by Phase 11 (#3309). Six-gate .cts ripple: .gitignore, eslint.config.mjs, docs/INVENTORY.md + manifest regen, CONTEXT.md glossary entry. Breaking changes: none. This phase adds the subject only; Phase 11 migrates cmdValidateHealth onto it. |
||
|
Tom Boucher
|
b8cb031ce2 |
fix(#3163): scope phase.add insertion to the current milestone (#3400)
* test(#3163): phase add must insert in the active milestone, not the trailing archive Regression for #3163: cmdPhaseAdd/cmdPhaseAddBatch pick the insertion point via rawContent.lastIndexOf('\n---'), the file's last horizontal rule — which on a roadmap with shipped/history material after the active phase list sits deep in archive. Rows 1/2/4 fail RED on next (entry lands after the archive heading); row 3 guards the no-milestone legacy fallback. * fix(#3163): scope phase.add insertion to the current milestone window cmdPhaseAdd and cmdPhaseAddBatch picked the insertion point via rawContent.lastIndexOf('\n---') — the file's last horizontal rule, which on a roadmap with shipped/history material after the active phase list sits deep in archive. Extract phaseEntryInsertOffset(rawContent, cwd): scope the search to currentMilestoneRawRanges' primary window so the entry lands at the end of the active phase list. Fall back to the legacy whole-file heuristic when no current milestone resolves, preserving simple no-milestone roadmaps. Applies to both cmdPhaseAdd and cmdPhaseAddBatch (identical expression); the decimal insert path was already header-anchored and is untouched. * docs(#3163): add changeset * docs(#3163): backfill changeset PR number (3400) --------- Co-authored-by: sim <sim@local> |
||
|
Rezolv
|
3ff9a7ffcd |
fix(#2570): parse leading date from last_activity so stale_activity fires with a description suffix (#2571)
* fix(#2570): parse leading date from last_activity so stale_activity fires with a description suffix templates/state.md prescribes `Last activity: [YYYY-MM-DD] — [What happened]`, and gsd-core's own STATE.md mirrors that suffix into frontmatter. Date.parse on the whole string returned NaN, and because staleActivity treats null as "not stale" (fails open), the only idle/staleness detector never fired on any project whose last_activity kept its description. parseActivityTimestamp now reads the leading ISO date/time token when a whole-string parse fails, validating the calendar date (ADR-227: reject an impossible date rather than let Date.parse roll it forward) and preferring the whole-string parse when it succeeds so a trailing zone name is not dropped. Composes with #3099 (LAST_ACTIVITY_UNPARSEABLE diagnostic), which merged to next after this branch: both key off parseActivityTimestamp === null, so a value whose leading date now parses takes the stale path and does NOT emit the diagnostic. A regression test in tests/smart-entry.unit.test.cjs asserts exactly that (stale true, emission count 0), guarding against two staleness signals on one field. Rebased onto next (flattened): resolved the add/add test conflict by keeping both the #2570 and #3099 describe blocks. Tests: unit + property, 80 pass. * fix(#2570): fail open when a named zone can't be reconstructed from the token (#2571 B1) The 2026-08-08 flatten dropped the zone handling earlier rounds built, so the fallback path -- reached only when a description suffix makes the whole-string parse fail, the #2570 case -- reconstructed `${date}${time}` WITHOUT any named zone. ISO_LEADING_RE's offset group captures only Z / +-HH:MM, so " GMT"/" EST" land in the un-captured suffix; Date.parse then read the reconstruction as LOCAL time, shifting the instant by the host's UTC offset -- a wrong, host-dependent value the diff's own comment warned against but guarded only on the other branch. Fix (the simpler of the two offered in review): when the remainder after the matched token begins with a letter (a named zone we cannot preserve), return null -- fail open to not-stale, matching the base's honest behaviour and ADR-227's "never propagate a wrong instant". The #2570 template suffix (" -- description") starts with a separator, so it still reconstructs and reads stale as intended. Tests (both fail-first, verified RED on the pre-fix head): - smart-entry.unit: a named-zone + description suffix (54 days old) enters the fallback and must read not-stale, not a still-old local instant. Host- independent by construction. - smart-entry.property (f): named-zone + suffix over 1-week..1-year ages and 8 zones stays total and fails open. Discloses the removal M2 flagged: TRAILING_ZONE_RE / UTC_ZONE_NAMES / timeCarriesOffset were dropped by the flatten; this restores the SAFETY (no wrong instant) via the simpler null contract rather than the allowlist. * fix(#2570): narrow the stale_activity fallback guard to a zone-designator shape The round-9 fail-open guard `/^\s*[A-Za-z]/` treated any letter-led remainder as an unpreservable named zone, so a leading real date followed by a bare space/tab/colon and an ordinary description (a hand-edited STATE.md that omits the template em dash) returned null and re-opened #2570 for exactly those shapes. Narrow the guard to ZONE_DESIGNATOR_RE -- a standalone short all-caps run -- and consult it ONLY when the leading token captured a time-of-day: a zone qualifies a clock time, so a bare date carries no zone hazard and always reconstructs to its UTC midnight. A plain description (including one that opens with a tech acronym like "CI green") reconstructs; a real named zone on a timed value (GMT/EST/...) still fails open (ADR-227: never propagate a wrong, host-dependent instant). Widen the property generator to the non-em-dash separators (space/tab/colon), the arm that structurally could not reach the fallback before, and add unit cases for whitespace/tab/colon-separated and bare-date+acronym descriptions. All fail-first on the prior guard; green across UTC/LA/Tokyo/Kiritimati. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
JusticeWay
|
77374cfc32 |
fix(#2528): resolve digit-leading phase directories by bare number (#2559)
* fix(#2528): resolve digit-slug phase dirs by bare number — tokenizer rewind, shared bare-integer fallback, resolution-path parity gate
extractPhaseToken welded 2-digit slug words onto the phase token (phase 10
named "24/7 Autonomy" -> dir 10-24-7 -> token 10-24), making digit-prefixed
phase names unresolvable by bare number across every phase verb.
- phase-id: continuation segments must be the PURE 2-digit zero-padded form
the write side emits; a 1-digit terminator rewinds the absorbed run
(10-24-7 -> 10) while >=2-digit terminators keep the locked #2232
round-trip (14-06-2026-photos -> 14-06).
- phase-id: new matchPhaseDirs owner — primary exact-token match plus a
bare-integer leading-digit-run fallback for shapes the tokenizer cannot
rewind (05-80-20-cleanup); collisions stay #2237-loud.
- locator/find-phase/phase-plan-index all delegate selection to the owner;
plan-index gains the previously missing multi-match guard.
- tests: #2528 unit + fast-check metamorphic blocks; new 9-scenario
resolution-path parity gate across all three paths.
Fixes #2528
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* chore(#2528): add changeset for PR #2559
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix(#2528): align validation token grammar
* fix: address phase token review
* fix: restore phase grammar parity for numeric slugs
* docs: document digit-leading phase resolution
* docs: clarify ambiguous phase resolution behavior
* docs: register canonical phase directory selectors
* fix: align prefixed deep phase token parsing
* fix(#2528): route the fourth resolution site through matchPhaseDirs
Review BLOCKER. smart-entry.cts::detectVerifyFailed resolved the current
phase's directory with its own `.find(phaseTokenMatches)` and never
reached the shared selection, so the bare-integer-fallback family the
issue names — `05-80-20-cleanup`, `30-12-factor-refactor` — resolved
nowhere. The miss is silent by construction: an unresolved phase reports
"not failed", which is byte-identical to a healthy one, so a failed
verification simply never surfaced in /gsd or /gsd:progress.
`entries` is already sorted and matchPhaseDirs filters without
reordering, so matches[0] reproduces the previous selection exactly
wherever the old code resolved at all.
Wiring it into phase-resolution-parity.test.cjs as a fourth path then
exposed a second, older defect in the same function: phaseTokenFromDirName
shape-probed the UNSTRIPPED token, so a project-code-prefixed directory
(`MEM-05-…`, tokenizing to `MEM-05-80-20`) failed the leading-digit test
and was dropped before any resolution ran — every phase in a
project-coded plan was invisible to this check. The probe now runs on the
stripped token; the returned value is unchanged, so the comparePhaseNum
sort is untouched.
Path 4 has no JSON surface to compare, so the gate observes selection
indirectly: plant the failing artifact in exactly one directory and a
passing one everywhere else, then read the boolean. Reverting either fix
turns 5 of the 10 corpus scenarios red.
* refactor(#2528): collapse the duplicated extractCanonicalPlanId
Review MAJOR. The function existed as two independent, byte-identical
copies — src/core-utils.cts and src/phase.cts — and this PR had to patch
BOTH with the same single-digit-slug rewind rule. That is the generative
fix divergence CLAUDE.md names, and only the core-utils copy was under
test, so a future one-sided patch would have silently split plan-id
canonicalization between the plan listing and everything else.
Removed rather than parity-tested: core-utils was already the leaf owner
and already exported it, and phase.cts already imported that module, so
there is no second surface left for a parity test to police.
* test(#2528): pin matchPhaseDirs at the digit-width boundaries
Review MAJOR. The bare-integer fallback's correctness rests entirely on
capturing each directory's whole leading digit run before the zero-strip
compare; a regex that stopped short would turn every query into a prefix
match, and "1" would claim 10, 100, and 12 alike. The existing coverage
was example-based and never touched that boundary.
Adds the explicit 9/10 and 1/10/100 cases — including the forms where
only the wider directories exist, so an exact-width neighbour cannot
satisfy the assertion — plus a fast-check property over arbitrary
distinct leading runs. The property is stated as an invariant on the
result (every returned directory's leading run IS the query) rather than
an expected list, so it covers primary and fallback matches alike and
cannot be satisfied by reimplementing the selection in the test.
Both fail when the fallback regex is degraded to a prefix match.
* fix(#2528): route the remaining eight consumers through matchPhaseDirs
phaseTokenMatches had eight consumers left that each rebuilt the directory
selection around it by hand: phases-list, next-decimal, phase-remove, the
W021 milestone-consistency check, schema-drift, the init-manager overview,
milestone-complete's disk check, and roadmap analyze. Every one of them
reproduced the reported symptom in full after the tokenizer was fixed.
None of them derives a displayed phase number from the matched directory,
so none needs phaseNumberForMatch; the change at each site is the
selection and nothing else. matchPhaseDirs filters without reordering, so
matches[0] reproduces the prior .find() choice wherever the old code
resolved at all.
phaseTokenMatches now has no call sites outside phase-id.cts. It stays
exported as the primitive matchPhaseDirs is built from and as a pinned
canonical surface, but no consumer reaches past the owner to it.
* test(#2528): extend the parity gate to the migrated consumers
Each of the eight is observed through the surface a user sees, not
through the matcher, with a no-directory control so the assertions cannot
be satisfied by a consumer that resolves unconditionally. init-manager
and roadmap-analyze are additionally asserted to agree with each other.
* refactor(#2528): own the case-flexible phase grammar and the leading-digit-run fragment
validate.cts derived its case-flexible regex sources by running
`replaceAll('A-Z', 'A-Za-z')` over two constants exported by phase-id.cts.
That passes lint-phase-id-drift.cjs — there is no literal copy of the
grammar — but it depends on the owner rendering that exact substring. The
day phase-id.cts expresses the same class any other way the replaceAll
silently no-ops and validate.cts narrows to uppercase-only. The failure
mode is a NON-match, so nothing throws and no uppercase-only fixture
notices. Both variants are now derived once, beside the sources they
widen, and imported.
Also names the leading digit run the bare-integer fallback selects on.
It was spelled `/^(\d+)(?:-|$)/` where the fallback filters and `/^\d+/`
where phaseNumberForMatch reads the number back off the winner; selecting
on one run and displaying another would resolve a directory and then label
it with a number that never matched it.
* fix(#2528): refuse to remove a phase when two directories claim its number
cmdPhaseRemove was the only migrated site taking matches[0] with no
multi-match guard. Every sibling resolution path returns ambiguous_matches
and refuses to choose; this one is the DESTRUCTIVE path, so choosing
silently is strictly worse than anywhere else. With 05-80-20-a and
05-90-till-late on disk, `phase remove 5 --force` deleted one of them and
renumbered every phase after it — where the base resolved nothing, deleted
nothing, and the corpus in tests/phase-resolution-parity.test.cjs already
declared that exact input ambiguous.
The refusal is emitted before any file is touched and carries both
candidates. CONSUMER_SCENARIOS could not express the case — every row is
binary, resolving to one directory or to none — so the gate gains a
dedicated ambiguous test. It asserts on the filesystem, not only on the
reported directory_deleted: a null printed after an rmSync would satisfy
every other check.
* fix(#2528): pair digit-leading phase directories with their roadmap phase in validate health
W006/W007 are the ninth site of this bug class and the one a
`phaseTokenMatches` grep could never surface: they resolve roadmap↔disk by
intersecting TOKEN SETS, which is a dir→token labelling rather than the
query→dir selection matchPhaseDirs owns. On the canonical fixture the
label is wrong in both directions at once, so `validate health` reported
"Phase 5 in ROADMAP.md but no directory on disk" AND "Phase 05-80-20
exists on disk but not in ROADMAP.md" for the same directory.
collectDiskPhases now keeps the directory names behind each token, so
W006 can ask the canonical matcher whether a roadmap phase resolves to a
real directory, and W007 — which iterates directories and therefore has no
query to resolve — gets the inverse mapping it never had: a directory is
claimed when some roadmap phase resolves to it.
Both checks are additive: the token intersection still decides every shape
it already decided, and the resolution can only REMOVE a warning. The
regression test carries controls in the other direction — a roadmap phase
with no directory must still raise W006, an unclaimed directory must still
raise W007 — so it cannot be satisfied by a check that stopped reporting.
* docs(#2528): state and pin the directory-side scope of the bare-integer fallback
The matchPhaseDirs docblock claimed deep-decomposition lookups were
untouched. That is true of the QUERY side only — no non-bare query enters
the fallback — but the DIRECTORY side is what changed classification: a
bare `5` now reaches a lone `05-01-auth` and resolves it (phase_number
"05", phase_name "01-auth") where the base found nothing.
The widening is irreducible from directory names alone. `05-01-auth`
(sub-phase 5.1) and `30-12-factor-refactor` (phase 30 named "12-Factor
Refactor") are the same `NN-NN-<slug>` shape, and the discriminator that
would separate them — "is the second segment a valid decimal sub-phase" —
accepts `5.1` and `30.12` equally. Any rule strong enough to exclude the
first excludes the second, which is the defect #2528 exists to fix. So the
tie is broken in favour of resolving, the docblock now says so, and the
consequence is bounded where it matters: two such directories are two
matches, and every caller (including phase remove) refuses to choose.
Pins both directions, since nothing observed the directory side before.
* fix(#2528): count surviving phases by identity in phase remove's STATE resync
#2640 landed on `next` while this branch was open. Its STATE.md phase-count
resync re-derives "which directory was removed" from the query with
`phaseTokenMatches`, which is the tenth site of this issue's defect: the
bare-integer fallback resolves `05-80-20-cleanup` for query `5`, but the
token predicate does not, so the just-deleted directory is counted as still
present and the written `Total Phases` is one too high.
`targetDir` already IS the directory that was removed, and the block is gated
on it being non-null, so identity answers the question exactly — which is also
what the comment above the filter already claimed it did. This keeps
`phaseTokenMatches` out of `phase.cts` rather than re-importing it to satisfy
one call site: the module's public surface should not grow for a question that
does not need re-derivation.
Pinned in the parity gate with a control on a directory the tokenizer reads
correctly, so the assertion is about the digit-leading shape and not about the
counting rule changing for everything.
* fix(#2528): let the resolution layer own the digit-leading slug family alone
The tokenizer rewind this fix carried — pop the last absorbed continuation when
the segment that stopped the scan is a bare single digit — reads
"10-24-7-autonomy" (phase 10 named "24/7 Autonomy") correctly and silently
re-reads "10-24-7-zip" (sub-phase 10.24 named "7-Zip Integration") from "10-24"
to "10". The two names are string-identical in shape, so no local signal
separates them; the rule traded the reported ambiguity for the symmetric one a
level down, on a 15-caller chokepoint whose output also feeds query-less
derivations (STATE.md phase counts, W007, the #2562 key surface). A well-formed
sub-phase directory became unresolvable by its own id — the very symptom #2528
was filed about.
It also bought nothing. The bare-integer fallback in matchPhaseDirs already
resolves "10-24-7-autonomy" for query "10" whatever the token is: no primary
match, bare query, leading digit run "10". The reported case was covered twice,
by two rules, and the two disagreed about the case nobody reported.
So the rewind is removed rather than narrowed, in the tokenizer and in the five
surfaces kept in lockstep with it (BRACKET_PHASE_TOKEN_SOURCE,
PHASE_TOKEN_FROM_DIR_RE, canonicalPlanStem's pair grammar and its collision
branch, roadmap-parser's numericRe, extractCanonicalPlanId), together with the
SINGLE_DIGIT_RUN_SEGMENT_SOURCE owner constant they shared. Disambiguation now
lives only where a QUERY exists to disambiguate against, which is the same
bounded mechanism the "05-80-20-cleanup" shape already used.
Measured, not argued: over 29800 generated directory names, extractPhaseToken is
byte-identical to `next` on every input except the lowercase-continuation class
("01-20a", "05-80-20-25abc") — a rule about the segment itself, not a guess about
its neighbour.
Both readings now stay reachable by their own ids:
matchPhaseDirs(['10-24-7-autonomy'], '10') -> the dir (fallback)
matchPhaseDirs(['10-24-7-zip'], '10') -> the dir (fallback)
matchPhaseDirs(['10-24-7-zip'], '10-24') -> the dir (primary)
* test(#2528): pin the one-continuation boundary the rewind had no coverage for
The regressing shape was invisible to the suite by construction, not by luck:
the deep-rewind property built its cases from `continuationArb` with
`minLength: 2`, so it never exercised the single-continuation case — exactly one
genuine sub-phase level before a digit-leading slug — and every hand-written
fixture used the ambiguous shape only where "phase-plus-slug" was the intended
reading.
`continuationArb` is now `minLength: 1` and the property states the invariant
instead of the old rule: for any prefix, phase, 1-5 continuations and any
one-digit terminator, the token equals the FULL continuation run on both the
imperative and the regex surface, and `matchPhaseDirs([dir], token)` returns that
dir. That third assertion is the one that catches the class on its own — the old
behaviour made a well-formed directory unresolvable by its own id, which is a
property, not a fixture.
Around it: "10-24-7-zip" and "10-24-3d-printer" now sit beside
"10-24-7-autonomy" everywhere the family is pinned, so the two readings can never
diverge again; the 24/7 metamorphic property asserts the RESOLUTION result rather
than the token (the token is precisely the part no surface may decide); the
end-to-end parity corpus gains "a sub-phase with a digit-leading slug resolves by
its full id" across all four resolution paths; and the milestone-scoping residual
is pinned in three directions rather than left to prose.
Mutation: re-inserting the rewind and rebuilding turns 9 tests red, the
`minLength: 1` property first, and nothing else. Build success checked separately
(build:lib reports 0 `error TS`), so the mutation reached the artifact under test.
* test(#2528): pin the #2946 guard against digit-leading phase directories
The #2946 fix makes the milestone-complete unstarted-phase guard run
unconditionally, so whether it fires now rides entirely on the
directory-resolution owner this PR replaces. Two cases, both with STATE.md
carrying no `milestone:` field so the #2946 path is the one exercised:
- ROADMAP Phase 5, disk `05-80-20-cleanup` → guard must stay silent.
RED on next (fail-closed: the guard blocks a legitimate one-way-door
operation because phaseTokenMatches resolves neither 05 nor 80 for
that directory).
- ROADMAP Phase 80, same directory → guard must still fire. Green on
both sides; it pins the fail-open direction against a future widening
of the matcher.
* fix(#3175): stop the injection scanner reading RegExp.exec as code execution
The apostrophe fix in
|
||
|
0xdhx
|
c516c39b33 |
fix(#3203): stop npm-global installs validating bundled agents against themselves (#3229)
* fix(#3203): stop npm-global installs validating bundled agents against themselves getAgentsDir's claude branch derived the agents directory from __dirname, which is correct for repo runs and runtime-config-dir installs (where <root>/../agents IS the user's agents dir) but on an npm-global install resolves to the package's own bundled agents/, so checkAgentsInstalled validated the package against itself and agents_installed could never be false. The new-project and new-milestone halt/warn gates were silently dead for npm-global users. Keep the install-relative path for the shapes where it is correct, but when it lies inside a node_modules tree — the provably self-validating case — resolve getGlobalConfigDir('claude')/agents like every other runtime, honouring CLAUDE_CONFIG_DIR. GSD_AGENTS_DIR stays priority 1. Repair the doc comment that asserted the __dirname form was correct for both install shapes. Regression test mirrors the published npm-global layout (package under node_modules with a complete bundled agents/) and pins the resolved directory plus the issue's negative control (one agent missing from the config dir → agents_installed:false). Verified red against pre-fix code, green post-fix; the repo-layout W010 health test stays green. * chore(#3203): set changeset fragment pr to 3229 * docs(#3203): describe the node_modules guard as lexical, in CONTEXT.md and at the call site The Agent Install Check Module glossary entry asserted that Claude resolves the agents directory `__dirname`-relative unconditionally. That is the premise this PR falsified: on an npm-global install the install-relative path resolves to the package's own bundled `agents/`, so the check validated the package against itself and `agents_installed` could never be false. The inline doc comment above `getAgentsDir` was repaired with the fix; this external predicate was left behind and has been false since. CONTRIBUTING.md's `Fixed`-fragment docs exemption names this case explicitly — "Edit the docs anyway if a fix corrects something the docs got wrong." Both surfaces now describe the guard as what it is: an exact, case-sensitive path-segment test that TARGETS those layouts rather than detecting them, so neither claims more certainty than the predicate has. The call-site comment carried the same conflation the glossary did. A path merely carrying a directory of that name resolves the same way — the edge already disclosed on this PR — and a non-empty GSD_AGENTS_DIR overrides it. Comment-only in `src/`; no behaviour change. `CONFIG.LOCATION.SEAM.two-families` needs no change: `GSD_AGENTS_DIR -> getAgentsDir priority 1` is still accurate. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
86101ee612 |
fix(#3133): keep global Claude @-references on tilde, not $HOME (#3393)
* test(#3133): global Claude @-references must resolve on tilde, not $HOME Regression for #3133: on a global Claude install, _applyRuntimeRewrites rewrites @~/.claude/... -> @$HOME/.claude/..., but Claude Code does not expand $HOME in @-file references, so the include silently resolves to nothing and the skill loads an empty execution_context. Rows 1/2/6/9 fail RED on next; rows 3-5/7-8 guard #1284 (shell $HOME), local redirect, #3503 (no homedir leak), and non-Claude isolation. * fix(#3133): keep global Claude @-references on tilde, not $HOME On a global Claude install, _applyRuntimeRewrites rewrote every @~/.claude/ include to @$HOME/.claude/ because computePathPrefix returns the $HOME form for shell-context correctness (#1284: ~ does not expand inside double quotes). Claude Code does not expand $HOME in @-file references, so the rewritten include silently resolved to nothing and skills loaded an empty execution_context. Add a normalization pass in the claude case: when the prefix is the $HOME (global) form, restore @$HOME/.claude/ -> @~/.claude/ (the form Claude expands and the shipped tarball uses). Shell contexts keep $HOME; local installs (absolute prefix) are unaffected; computePathPrefix is untouched (all its tests stay green). * test(#3133): fix row 9 assertion — drop end-of-string anchor Row 9's regex used $ (end-of-string) but a realistic @-ref line ends with a newline, so the anchor could not match. The fix under test produces the correct @~/.claude/gsd-core/references/ui-brand.md tail; only the assertion was wrong. Drop the anchor and assert the tail + no @$HOME. * docs(#3133): add changeset * docs(#3133): backfill changeset PR number (3393) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2b20b7e2cd |
fix(#3257): preserve full-line frontmatter comments through the parse→reconstruct pair + syncStateFrontmatter (#3387)
* test(#3257: full-line frontmatter comments survive the parse→reconstruct pair AND a mutating state verb parseYamlRegion dropped column-0 # comments and reconstructFrontmatter rebuilt from Object.entries alone, so full-line comments were silently destroyed on every mutating STATE verb. Add failing-first regressions: 3 unit tests for the public pair (comment between keys, leading+trailing, consecutive) and an e2e test running a state verb (state update) on a commented STATE.md — the e2e exercises syncStateFrontmatter's fresh-derivedFm rebuild path, which is the actual loss site the issue is filed against. RED — fails on next; fix follows. * fix(#3257: preserve full-line frontmatter comments through parse→reconstruct AND syncStateFrontmatter Carry column-0 # comments through the frontmatter pair via a Symbol-keyed channel (FULL_LINE_COMMENTS): parseYamlRegion captures ^# lines and attaches them to the next top-level key (leading) or a trailing slot; reconstructFrontmatter re-emits them in place. The Symbol is invisible to Object.entries/keys/JSON, so every existing reader is unchanged; the channel is created only when a comment is seen, so comment-less frontmatter is byte-identical. CRITICAL (isolated review): syncStateFrontmatter rebuilds its target via buildStateFrontmatter (fresh object) + an Object.keys carry-forward, both of which skip the Symbol — so the pair-preserving channel was lost on the very STATE verbs the issue names. Export propagateCommentChannel(source, target) from frontmatter.cts and call it in syncStateFrontmatter before reconstruct, copying the channel onto derivedFm (leading filtered to keys still present so a deleted key's annotation drops with it, trailing preserved). Decision A. * chore(#3257: add changeset fragment * chore(#3257: backfill changeset PR number (#3387) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4a0b5e26b4 |
test(#3338): fold the verify/validate & workflow-text issue-* cluster — Wave 6 (#3383)
* test(#3338): fold the verify/validate & workflow-text issue-* cluster — Wave 6 Folds 10 legacy issue-*.test.cjs regression files (75 test() blocks) into their module's main suite, per H3 (#3315) of the test-hygiene epic (#3053). Third of 4 issue-* waves. - issue-2701-nul-corrupted-validators.test.cjs (9) + issue-429-comment- text-gate.test.cjs (31, incl. fast-check property tests): both target verify.cjs/validate.cjs via different calling styles (CLI vs direct- require) — merged jointly into verify.test.cjs, 0 dropped. - issue-2762-plan-reviews-chunked.test.cjs (3) merged into plan-phase-drift-guard.test.cjs. - issue-2771-advisor-subagent-type.test.cjs (1) + issue-2772-discuss- phase-text-inconsistencies.test.cjs (6) merged jointly into discuss-phase-power.test.cjs. - issue-498-update-backup-runtime-dir.test.cjs (3, rename basis) + issue-815-update-next-channel.test.cjs (7, merged in): both concern update.md workflow-text contracts, now update-workflow.test.cjs. - issue-498-update-context.test.cjs (13): pure rename to update-context.test.cjs, sole comprehensive suite for its module. - issue-2765-brace-expansion-lockfile.test.cjs (1, rename basis) + issue-3238-js-yaml-lockfile.test.cjs (1, merged in): two distinct CVE regression pins against package-lock.json, now lockfile-cve-audit.test.cjs, shared ROOT/npmLs helpers deduped instead of double-declared. Ratchet upkeep to keep this wave's own gates green: pruned 3 stale allow-test-rule allowlist entries, cited 2 previously-uncited comments that surfaced in the folded content (#3338), tightened the exemption-file ceiling 305 -> 297. Fixed one stale filename reference in production code (src/init.cts) plus one in docs/reference/workflow-fragments.md. Zero net test-coverage loss. No production code BEHAVIOR changed. * test(#3338): fix orthogonal-review findings — Wave 6 fold Standards-axis review found a real structural defect in two files, both the same root cause and both fixed here: - tests/update-workflow.test.cjs: the folded:issue-815-update-next-channel wrapper's closing brace was placed after the file's pre-existing tail instead of before it, making the already-established folded:bug-2470 and folded:bug-3130 wrappers CHILDREN of issue-815's block in the test hierarchy instead of independent siblings — confirmed via an actual node --test run showing the mislabeled TAP nesting. Moved the closing brace to the correct position; all three fold wrappers are now top-level siblings again (verified via node --test, TAP hierarchy correct, 10/10 tests, 5 suites, identical count before and after). - tests/plan-phase-drift-guard.test.cjs: same mistake in the other direction — folded:issue-2762-plan-reviews-chunked was spliced inside the pre-existing folded:bug-2492-context-coverage-gate wrapper instead of after it. Fixed the same way (227/227 tests, 38 suites, identical count before and after). - Reverted unnecessary 815-suffixed local renames (assert815/fs815/etc.) introduced by the fold — the wrapper is genuinely block-scoped once correctly closed, so no collision existed (same class as Wave 4's __foldSetNested finding). No test() count changed in either file. No production code touched. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
864f76b46f |
fix(#3255): updateTableCell scans for the table carrying the requested column (#3377)
* test(#3255): updateTableCell reaches a later table when the first lacks the column A ## Traceability section holding a summary table (no Status) above the requirement table (with Status) made updateTableCell bind to the first table and return 'unknown column: Status', so requirements.mark-complete left the row at Pending. Add a failing-first regression at the primitive. RED — fails on next; fix follows. * fix(#3255): updateTableCell scans for the table carrying the requested column updateTableCell bound to the FIRST GFM table in the scoped text and returned 'unknown column' if that table lacked the column — so a ## Traceability section holding a phase-summary table above the requirement table never reached the requirement table, and requirements.mark-complete left the row at Pending while reporting table_unmatched (the #2140 silent-divergence class one level deeper, flagged but not closed by the #2245 cross-section scoping fix). Scan candidate table headers and pick the first VALID table (delimiter + matching column count) whose columns include the requested column. Single-table behaviour is byte-identical (the one table carries the column). Error semantics preserved: no table -> 'no table found'; valid table without the column -> 'unknown column'; lone malformed table keeps its specific reason. Also fixes the derived hasRow/doneTable reasoning, which probes via the same call. * chore(#3255): add changeset fragment * chore(#3255): backfill changeset PR number (#3377) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ad07f76a31 |
test(#3336): fold the installer & runtime surface issue-* cluster — Wave 4 (#3376)
* test(#3336): fold the installer & runtime surface issue-* cluster — Wave 4 Folds 10 legacy issue-*.test.cjs regression files (79 test() blocks) into their module's main suite, per H3 (#3315) of the test-hygiene epic (#3053). First of 4 issue-* waves (following the 3 fix-* waves, all merged). - 1 file with no prior target coverage: renamed (git mv) into legacy-cleanup.test.cjs (sole comprehensive suite for that module). - 9 files merged into 6 pre-existing suites: golden-parity-single-source, runtime-artifact-layout-surface, codex-config (4 sources merged jointly in one pass per the issue's own instruction, to catch overlap between the 4 sources themselves, not just against the pre-existing target — zero overlap found, all 20 blocks additive), runtime-config-adapter-registry (1 of 10 source blocks dropped as a proven subset of existing coverage), cline-install, install.test.cjs. Incidental fixes required to keep this wave's own ratchets green: - Fixed a stale ADR doc reference (docs/adr/1235) to a folded-away filename. - scripts/lint-allow-test-rule-refs: pruned 4 stale allowlist entries for renamed/merged-away files, cited 2 previously-uncited allow-test-rule comments that surfaced as "new" only because their file path changed, added 1 fresh allowlist entry for a pre-existing uncited comment that predates this PR, and tightened the exemption-file ceiling 309 -> 305 to match the real post-fold high-water mark. Zero net test-coverage loss. No production code changed. * test(#3336): fix orthogonal-review findings — Wave 4 fold Standards-axis review + Memtrace graph pass found real issues in the just-folded suites, all fixed here: - Standardized the fold-wrapper convention (block-scoped __foldDescribe) across golden-parity-single-source.test.cjs, runtime-artifact-layout- surface.test.cjs, runtime-config-adapter-registry.test.cjs, and cline-install.test.cjs to match the pattern already used by codex-config.test.cjs and install.test.cjs in this same wave (and by earlier folds elsewhere in the epic) — repeats the exact inconsistency Wave 3 (#3335) already fixed once in this epic. - Fixed a stale allowlist entry's alphabetical position (cosmetic, not tool-gated, caught by review anyway). - Fixed two stale test-filename references in PRODUCTION code comments (src/capability-writer.cts, src/runtime-config-adapter-registry.cts) caught by lint-removed-but-needed — a class of stale reference this wave's fold agents didn't check for, since they were scoped to docs/ and gsd-core/references/ only, not src/. First fix attempt wrongly edited the gitignored gsd-core/bin/lib/*.cjs BUILD OUTPUT instead of the tracked .cts source; caught and corrected before commit. - Fixed one remaining stale doc reference in docs/adr/1235 (a prior partial fix in this same wave missed it). No test() count changed in any file. No production code BEHAVIOR changed — comment-only fixes in src/. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
23e6d49929 |
fix(#3233): no-op state update-progress when the milestone scan finds zero plans (#3375)
* test(#3233): zero plans (0/0) is a no-op; plans-but-none-done still writes 0% cmdStateUpdateProgress mapped 0/0 through clampPercent to 0% and rewrote the shipped Progress record after milestone close. Replace the stale 'handles zero plans gracefully' test (which asserted the buggy percent:0) with a #3233 no-op regression (100% record preserved, updated:false), and add a negative-space guard: plans exist but none done must still write a legitimate 0%. RED — fails on next; fix follows. * fix(#3233): no-op state update-progress when the milestone scan finds zero plans cmdStateUpdateProgress mapped 0/0 through clampPercent to 0% and unconditionally rewrote the body Progress line, so after /gsd-complete-milestone archived the phases (.planning/phases/ empty, scope COMPLETE) a routine update-progress run destroyed the shipped record ([██████████] 100% → [░░░░░░░░░░] 0%). Add an early-return no-op when totalPlans === 0 — mirroring the established scope-withholding no-op (stderr WARNING + {updated:false, reason}) and computeProgressPercent's null-for-empty contract ('nothing to measure' ≠ '0% done'). The legitimate 0% case (plans exist, none summarized) is unaffected: totalPlans > 0 reaches clampPercent(0, N>0) = 0 and writes 0% as before. * test(#3233): unshadow 'Progress field missing' — clear the zero-plans guard The new totalPlans===0 no-op guard fires before the 'Progress field not found' branch, so the existing 'returns error when Progress field missing' test (no phase dirs → 0 plans) was passing for the wrong reason and that branch lost coverage. Give that test a phase dir + PLAN so totalPlans > 0 clears the guard and it reaches the branch it is named for. (Isolated review finding.) * chore(#3233): add changeset fragment * chore(#3233): backfill changeset PR number (#3375) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ae7dc52972 |
fix(#3225): guard W006/W007 + consistency loops with isSentinelPhaseId (#3371)
* test(#3225): sentinel phase dirs no longer trigger W007 / consistency warnings / gaps The W006/W007 (validate health) and the parallel consistency disk↔roadmap and gap-numbering loops never got the isSentinelPhaseId guard that phase.cts has (#2786/#2949), so every sentinel phase dir (999.x/0.x — never-on-roadmap by convention) produced a spurious W007 and a spurious 'Gap in phase numbering: N → 999'. Add failing-first regressions for both surfaces + the gap check, each with a non-sentinel orphan negative-space guard. RED — fails on next; fix follows. * fix(#3225): guard W006/W007 + consistency + gap loops with isSentinelPhaseId cmdValidateHealth's W006/W007 loops, cmdValidateConsistency's parallel disk↔ roadmap loops, AND its gap-numbering check never got the isSentinelPhaseId guard that phase.cts has at 10+ sites (#2786/#2949). So any repo using the sentinel-id convention (999.x backlog/interim, 0.x drafts) got a permanent spurious W007 and a spurious 'Gap in phase numbering: N → 999', with advice to add-to-roadmap (violates the convention) or delete (destroys archived work). Add isSentinelPhaseId to the phaseIdMod destructure and skip sentinel ids in: W006 + W007 (cmdValidateHealth); the two plain-warning disk↔roadmap loops and the gap-numbering integerPhases filter (cmdValidateConsistency — same bug family, folded in inline per no-silent-defer). Additive only: non-sentinel orphans and real numbering gaps still warn. The gap-numbering guard was surfaced by the isolated review (a 999-interim dir would otherwise create a false 'N → 999' gap). Same family as #3167 (since fixed). * chore(#3225): add changeset fragment * chore(#3225): backfill changeset PR number (#3371) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
62f5f3b39e |
fix(#3224): register WINDOWS.md as a canonical .planning/ artifact (#3369)
* test(#3224): assert WINDOWS.md is a canonical .planning/ artifact The broken-windows ledger (.planning/WINDOWS.md, written by gsd-core's own windows command) was absent from CANONICAL_EXACT, so validate health flagged it W019 'Unrecognized' with advice to delete a file that can gate /gsd-ship. Add WINDOWS.md to the expected-canonical list + a dedicated predicate test. RED — fails on next; fix follows. * fix(#3224): register WINDOWS.md as a canonical .planning/ artifact CANONICAL_EXACT (src/artifacts.cts) was never updated when the broken-windows capability (#1950/#2441) started writing .planning/WINDOWS.md. validate health therefore flagged the ledger W019 'Unrecognized' with fix advice to archive or delete it — a file gsd-core itself produces (src/broken-windows.cts, LEDGER_FILE_NAME) and that can gate /gsd-ship under workflow.windows_enforce. Add WINDOWS.md to CANONICAL_EXACT, per the registry header's own maintenance mandate ('Add entries here whenever a new workflow produces a .planning/ root file'). isCanonicalPlanningFile now returns true for it, suppressing the false W019. Existing W019 behavior for genuinely-unrecognized files is unchanged. * chore(#3224): add changeset fragment * chore(#3224): backfill changeset PR number (#3369) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bfd749cb9a |
fix(#3213): segment-boundary membership for letter-named phase dirs (#3368)
* test(#3213): add letter-named phase regression for getMilestonePhaseFilter The custom-ID branch's greedy capture excluded every letter-named phase directory (Phase A:..Phase L:) from the milestone, fabricating counts. Add two failing-first regression tests: a single letter phase + numeric control, and the full A..L + 00 tree from the issue's reproduction. RED — fails on next; fix follows in a separate fix: commit. * fix(#3213): segment-boundary membership for letter-named phase dirs The custom-ID branch of isDirInMilestone (getMilestonePhaseFilter) used a greedy capture ^([A-Za-z][A-Za-z0-9]*(?:-[A-Za-z0-9]+)*) that swallowed the whole hyphenated directory name (A-tool-output-contract was captured as 'A-tool-output-contract', not 'A'). The set lookup then failed and every letter-named phase directory (Phase A:..Phase L: — GSD's own convention, ADR-612 first-class non-numeric IDs) fell out of the milestone, silently fabricating progress/plan counts over whatever numeric dir survived. Replace the capture-then-lookup with a segment-boundary membership test: a directory belongs if its lowercased name EQUALS a declared phase ID or BEGINS with that ID followed by '-' (so 'A-tool-output-contract' matches ID 'a'; 'PROJ-42-description' matches ID 'proj-42'; 'AB-combined' does NOT match 'a'). IDs are sorted longest-first so a hyphenated id (proj-42) is tested before a prefix of it (proj). Additive only — numericRe still handles every leading-digit dir first, and this can only ADMIT a dir the greedy capture wrongly excluded, never exclude one already matched. * chore(#3213): add changeset fragment * chore(#3213): backfill changeset PR number (#3368) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e7993d77bf |
fix(#3207): create-and-switch on the first phase/milestone commit (#3363)
* test(#3207): invert fresh-create branching tests to expect create+switch The four fresh-create branching tests (#3079) locked in create-without- switch behavior that #3207 identifies as the regression: a fresh phase/ milestone branch is created but HEAD never moves onto it, so the first phase/milestone-scoped commit lands on the base branch. Invert those assertions to expect create+switch, and add two new tests: - fresh-create is non-silent (logs the create+switch) (#3207 AC3) - a second phase commit does not re-warn once HEAD is on the phase branch (#3207 AC5) The existing-branch path (#2539) is deliberately byte-for-byte unchanged. RED — fails on next; the fix follows in a separate fix: commit. * fix(#3207): create-and-switch on the first phase/milestone commit The #3079 fix (PR #3141) replaced git checkout -b with git branch (create-only) unconditionally, including the case where the strategy branch does not yet exist. That regressed #1278: the first phase- or milestone-scoped commit no longer landed on the strategy branch — it stayed on the base branch, and the strategy branch was left as an empty marker pointing at the pre-phase tip. Every subsequent commit then warned 'already exists; committing on the current branch instead of switching', wording that misleads because the tool itself created the branch moments before. Re-separate the two cases #3079 collapsed: - branch does NOT exist -> create AND switch (git checkout -b). The #3079 resurrection hazard cannot apply: the branch was just verified absent, so there is no merged-and-deleted ref to resurrect and no silent move onto an existing unrelated branch (#2539 AC2 is honored by the existing-branch arm, which is unchanged). The create is logged so the first phase-scoped commit is not silent (#3207 AC3). - branch ALREADY exists -> unchanged: no switch, commit on current branch, non-silent warning (#2539/#3079). Once the first commit switches HEAD onto the strategy branch, the currentBranch !== branchName guard skips the block on subsequent commits, so the misleading 'already exists' warning no longer recurs. * chore(#3207): add changeset fragment * chore(#3207): backfill changeset PR number (#3363) --------- Co-authored-by: sim <sim@local> |
||
|
0xdhx
|
0396d9cab1 |
enhance(#2483): stop the claude reviewer lane from inheriting CLAUDE.md + auto-memory (#2493)
* enhance(#2483): env-guard the claude reviewer leg against CLAUDE.md injection
The claude reviewer in workflows/review.md was a bare headless `claude -p`
spawn run from the project cwd, so it inherited the invoking user's global
CLAUDE.md, the project CLAUDE.md, and Claude Code auto-memory.
That made it the only reviewer leg seeing anything beyond the prompt file.
gather_context assembles PROJECT.md, the roadmap section, every PLAN file,
CONTEXT.md, RESEARCH.md and REQUIREMENTS.md into the prompt before any
reviewer runs; the gemini leg receives only that prompt and the codex leg
runs --ephemeral. Beyond the measured ~4k tokens/spawn, the asymmetry cuts
at the workflow's own premise: "independent review" meant something
different for the claude leg than for the other two.
Guard both dispatch lines with a per-invocation
`env CLAUDE_CODE_DISABLE_CLAUDE_MDS=1`. `env`, never `export` — the flag
must not leak into the orchestrating session (which may itself be Claude
Code on the SELF_CLI="auto" path) or into any later spawn.
review.md is the only claude -p call site in the installed tree, so this is
two lines on one surface. The self-skip logic is untouched.
* enhance(#2483): fix CRLF-fragile split and regenerate workflow baselines
Two CI failures from the first push, both mine:
1. lint-tests: the new regression test split readFileSync content on a
literal "\n". On a Windows git-autocrlf checkout that leaves a trailing
"\r" on every line (local/no-crlf-fragile-split). Use .split(/\r?\n/).
2. golden-install-parity / workflow-size-budget / workflow-compat: editing
gsd-core/workflows/review.md changes its content hash and byte size, and
both are pinned in committed baselines. Regenerated via the repo's own
generators (npm run size:baseline, npm run gen:golden).
The regenerated diffs are review.md-only: exactly one hash line per
golden-install-parity fixture and one size entry in workflow-size-baseline
— no unrelated drift swept in.
Full suite now green locally: 2113 pass, 0 fail, 3 skipped (run with HOME
and CLAUDE_CONFIG_DIR overridden to throwaway dirs; live profile verified
untouched afterward).
* enhance(#2483): adapt guard-test matcher to the effort-args dispatch reshape
The effortSurface wiring (#2481) reshaped the bare-model dispatch to
`claude $CLAUDE_EFFORT_ARGS -p -`; the invocation matcher's dash-first
form could no longer see it, and the count assertion failed exactly as
designed. The matcher now tolerates variable expansions between `claude`
and its first literal flag. Negative-controlled both ways: a stripped
guard and a deleted dispatch line each still fail.
* enhance(#2483): also guard the claude leg against auto-memory injection
CLAUDE_CODE_DISABLE_CLAUDE_MDS suppresses CLAUDE.md file loading;
auto-memory is an independently-toggled mechanism with its own flag.
Add CLAUDE_CODE_DISABLE_AUTO_MEMORY=1 to both dispatch lines, correct
the docs/COMMANDS.md and changeset claims that credited the first flag
with covering auto-memory, and extend the regression test to require
both flags on every claude invocation (negative-controlled: 2/4
assertions fail with the new flag removed).
* enhance(#2483): match the claude binary in command position, not argument position
The line-oriented invocation matcher counted any line where the token
`claude` was followed by a flag. #2589 (landed on next as
|
||
|
Dennis Kim
|
5e951540af |
fix(#3162): resolve active state phase before drift scan (#3208)
* test(02-01): reproduce template state validation drift - derive command fixtures from the shipped STATE template - pair passed-verification drift with a clean opposite-result control * test(02-01): cover state phase resolution boundaries - exercise precedence conflicts fallbacks and fail-closed directory handling - prove canonical equality and reject outside-root verification evidence * docs: add changeset for PR #3208 * Address review feedback * fix(#3162): preserve phase validation after state refactor * test(#3162): align validation scope cases |
||
|
Behruz Nassre Esfahani
|
2076d450d7 |
fix(#2652): gate quick/diagnose dispatch on dispatch.isolation, not the runtime name (#2728)
* fix(#2652): gate quick/diagnose dispatch on dispatch.isolation, not runtime name quick.md and diagnose-issues.md kept the pre-#2584 `RUNTIME != "claude"` worktree gate, so every non-Claude runtime failed closed regardless of the capability it negotiated — including Codex, which declares orchestrator-worktree. Route both through the negotiated dispatch.isolation seam via a new shared reference, and migrate the two execute-phase reference fragments that carried the same runtime-name gate. - new gsd-core/references/dispatch-isolation-gate.md: canonical ISOLATION resolution, harness-flag resolution, single-agent degrade rule - quick.md / diagnose-issues.md read the gate; dispatch uses the {harnessFlag} placeholder rather than a hardcoded isolation="worktree" - execute-phase-wave-guard.md / execute-phase-between-wave-reset.md: migrate [ "$RUNTIME" = "claude" ] -> [ "$ISOLATION" = "harness-worktree" ] - every degrade site now clears BOTH USE_WORKTREES and ISOLATION; clearing one dispatched an isolated agent with no base guard and no manifest - parity guard in host-integration.test.cjs scans workflows AND references and matches six reintroduction shapes - migrate four tests that pinned the pre-#2584 runtime-name contract Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2652): use the /gsd:<cmd> namespace in the isolation degrade messages The degrade warnings cited /gsd-execute-phase, the retired hyphen form that slash-command-namespace.test.cjs rejects in Claude-facing source. Same length, so the quick.md size budget is unaffected. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2652): add changeset for PR #2728 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2652): normalize dispatch-site paths to forward slashes for Windows path.relative() returns backslash-separated paths on Windows, so the #2652 dispatch-site parity test compared "gsd-core\workflows\quick.md" against the hardcoded forward-slash literal "gsd-core/workflows/quick.md" and failed on every windows-latest CI lane. Normalize with .replace(/\\/g, '/'), matching the existing convention used elsewhere in this suite (e.g. tests/branch-no-track-guard.test.cjs:37). * test(#2652): restore the size-growth acknowledgment The rebase dropped tests/emitted-drift-ack.json. #2757/#2758 fixed the ATTRIBUTION axis, but the SIZE-GROWTH axis is independent: diagnose-issues.md (+2086) and quick.md (+230) still need an ack naming them and saying why. Verified: 65/66 without it (both files named), 66/66 with it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2652): convert execute-plan.md Pattern A onto the dispatch-isolation gate Pattern A hardcoded `isolation="worktree"` — Claude Code's own literal — gated only on `workflow.use_worktrees`, with no capability negotiation at all. It is the same defect #2652 fixes at the other four sites, just a different shape: the file contains no RUNTIME variable, so the new detector correctly does not flag it. Concrete break: a Codex user who follows this PR's own newly-documented pattern and sets `workflow.use_worktrees: true` to get isolated dispatch via /gsd:quick then runs a plan through /gsd-execute-plan Pattern A, and hits an unconverted path — either an Agent() call erroring on an unrecognized parameter or silent unisolated execution, depending on host tolerance. Pattern A is a single-agent dispatch site through the host's own subagent tool, so it takes the same treatment as quick.md and diagnose-issues.md: resolve ISOLATION/HARNESS_FLAG through the canonical reference, degrade to sequential on orchestrator-worktree hosts, and substitute the host's declared {harnessFlag} instead of Claude Code's literal. while the area was open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#2652): add the INVENTORY row for dispatch-isolation-gate.md, refresh CONTEXT Two bookkeeping gaps flagged in review: INVENTORY.md had no row for the new gsd-core/references/dispatch-isolation-gate.md. INVENTORY-MANIFEST.json was regenerated correctly and its --check only diffs a live directory scan against the committed manifest, so CI passed regardless — but gen-inventory-manifest.cjs's own stderr guidance says to add the matching INVENTORY.md row. This is the repo's named "Inventory Drift" pattern. Placed with the dispatch/isolation cluster (worktree-branch-check, runtime-aware-dispatch) rather than alphabetically, matching how that table is grouped. CONTEXT.md's Host-Integration Interface entry still described dispatch.isolation as "declared and negotiated but not yet consumed by any scheduler — Phase 1 of #2584". That was already stale before this PR (execute-phase graduated in Phase 3) and more so now with three single-agent dispatch sites consuming it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2652): detect reversed-operand runtime gates; add a permutation property All five reintroduction regexes assumed $RUNTIME on the LEFT of the comparison, so `[ "claude" != "$RUNTIME" ]` — the same gate written backwards — evaded every one of them. Verified against the old patterns before fixing: all four reversed shapes (single bracket, double bracket, test builtin, JS template) scored EVADED. Each comparison shape is now generated in both operand orders from a single template, so a shape cannot be added in one order and forgotten in the other. The mutation table gains the four reversed cases. Also adds the fast-check property review suggested in place of the hand-rolled cases: it generates the cross product of the axes an author actually varies — bracket form, operator, operand order, quoting, spacing, runtime id — so a permutation the hand-written patterns miss surfaces here rather than in production. The 11 explicit cases stay as named regression anchors. execute-plan.md joins the scan's required-identities list now that it is a converted dispatch site. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2652): acknowledge the execute-plan.md size growth The Pattern A conversion adds 811 bytes to an emitted workflow. Per #2719 the size axis needs its own acknowledgment, independent of attribution. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(#2652): repin the execute-plan.md PROSE_ALLOWLIST line after the rebase The #2751 command-position gate pins its prose exemptions by line number. This branch inserts the dispatch-isolation resolution above the `validated downstream by gsd-tools uat classify-coverage` sentence, moving it from execute-plan.md:387 to :397 — which fired the gate twice for one displacement (an un-allowlisted mention at 397, a stale entry at 387). The prose itself is unchanged from next; only the pin moves. Fixes #2652 * fix(#2652): gate the #2649 base-check on ISOLATION in diagnose-issues.md The rebase onto next merged #2649's pre-dispatch base-check textually, but its degrade flipped USE_WORKTREES after ISOLATION was already resolved, so the degrade never reached the dispatch decision. Gate the block on ISOLATION = "harness-worktree" and degrade ISOLATION itself, the same pairing quick.md already uses. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2652): key quick.md post-dispatch bookkeeping on ISOLATION, not the Claude literal Review Blocker: the manifest append (l.822), worktree merge-back (l.825), and its skip clause (l.839) all conditioned on the literal isolation="worktree" — Claude Code's own rendering of {harnessFlag}. Cursor renders --worktree, so a newly-unblocked isolated Cursor run created a worktree whose committed work was never merged back and never cleaned up, silently. All three now key on ISOLATION = "harness-worktree" at dispatch. The existing parity detector cannot catch this class (its ISOLATION_TOKEN treats the literal as a legitimate marker), so this adds a dedicated literal-condition detector with a discrimination proof against both pre-fix sentences, a benign-mention control, and a positive pin on all three re-keyed conditions. Verified fail-first against the pre-fix quick.md. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2652): scope the use_worktrees=false install stamp to isolation=none runtimes `_stampNonClaudeRuntimeDefaults` rewrote every non-Claude runtime's `workflow.use_worktrees` read to `--default false`. That default resolved before `gsd_run query dispatch-isolation` was ever consulted, so the five runtimes that declare worktree support — cursor (harness-worktree) and codex/opencode/kimi/kimi-code (orchestrator-worktree) — got ISOLATION=none regardless of what they negotiated. The gate this PR migrates dispatch onto was therefore still deciding isolation by runtime name, one layer down. The stamp's #1521 premise was that worktree isolation *was* Claude Code's isolation="worktree" spawn parameter, which no other host honored. #2584 replaced that premise with the negotiated capability. The stamp is now scoped to runtimes whose negotiated isolation really is `none`, where the default it writes is the outcome the resolver reaches anyway. `_negotiatedDispatchIsolation` mirrors routeDispatchIsolation's resolution against the same registry — closed vocabulary, a harness-worktree host must declare its flag, an orchestrator-worktree host must carry a descriptor that resolves — and fails closed to `none` on anything else, so an undeclared or unknown runtime keeps today's behavior. Two #1515 tests pinned the superseded premise for codex and are re-pointed at the new contract rather than deleted: the safety property they protect is now held by the isolation gate's fail-closed resolution, not by a name-scoped install-time default. Verified fail-first — all five assertions red against the pre-fix source, green after. * test(#2652): acknowledge the emitted ripple and re-point the end-to-end stamp proof Scoping the use_worktrees stamp changes emitted output, and two gates caught it. `gsd-core/workflows/execute-phase.md` now differs at emit time for the five hosts that declare worktree support (cursor harness-worktree; codex, opencode, kimi, kimi-code orchestrator-worktree) — the source file is byte-identical, only the stamp is gone. Acknowledged in this PR's fragment. `tests/install.test.cjs`'s real-install assertion pinned the superseded premise end-to-end, asserting codex receives `--default false`. Re-pointed rather than deleted, matching the two unit tests: it now proves codex keeps the unstamped `true` read. A second arm installs windsurf — which declares isolation `none` — and asserts the false stamp is still applied there, so the change cannot silently degrade into "never stamp" without a test noticing. The ack entry collides with `2658-trae-instruction-file-path.json`, which is fully spent (merged via #2925, so all 25 of its entries are present at base and gate nothing) and is pruned for the same reason and by the same rule as the spent `2649-*` fragment this PR already removed. #2566 prunes the same file for the same collision on `new-project.md`; a delete/delete merges cleanly either way, and the base-side cleanup would make both unnecessary. * fix(#2652): re-record the sentinel when a dispatch site degrades isolation Review Blocker B1/B2/B3. Every isolation degrade in a dispatch site is decided in shell, where routeDispatchIsolation cannot see it. That resolver persists whatever it resolved to the run-scoped sentinel as an unconditional side effect (#3045), so a degrade that only reassigns $ISOLATION leaves the sentinel asserting harness-worktree while the dispatch correctly omits the harness flag. The shipped PreToolUse guard reads the sentinel at the instant of the Agent() call and denies that mismatch with exit 2 — the work does not run unisolated, it does not run at all. Latent on this branch and lands on rebase, since |
||
|
Rezolv
|
e87fb409ee |
enhance(#2573): stamp STATE.md with its commit and surface a freshness hint (#2622)
* enhance(#2573): stamp STATE.md with its commit and surface a commit-age freshness hint Adds a `state_head` stamp to STATE.md and derives a tri-state commit-age freshness proxy (state_commits_behind / state_commit_stale) through state.cjs's readStateHeadFreshness, surfaced on smart-entry signals and as health W024. The proxy is advisory: classify() deliberately does NOT consume it (ADR-1787 locks the classification/routing boundary — a signal, not a route). Composes with #3099 and #1882 (both merged to next after this branch): the commit-age proxy reads `state_head` while the LAST_ACTIVITY_UNPARSEABLE diagnostic reads `last_activity` — two different fields, not "two staleness signals on one field." A new regression test asserts a STATE.md carrying both an unparseable last_activity AND a valid state_head resolves each independently (diagnostic fires once; freshness reads state_head, commits_behind 0). Rebased onto next (flattened): resolved the add/add conflicts in src/smart-entry.cts (kept both the #2573 freshness import/derivation and the #3099 diagnostic import/call) and tests/smart-entry.unit.test.cjs (kept both describe blocks). Drift-ack for health.md's W024 row is unchanged (12348 B). Tests: smart-entry 62, state/state-transition/health/verify 639, all pass. * chore(#2573): allowlist health-validation test in the prompt-injection scan The scanner's `exec('` code-execution pattern matches the benign `re.exec('<phase-id>')` RegExp method calls in the phase-ID grammar tests (pre-existing: 16 such calls on next, this PR adds none). The file entered the diff-mode scan's changed-file set only because #2573's W024 state_head assertions touch it. Allowlist it alongside the other test files that carry pattern-matching content as data (same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class). Scanner self-test 38/0; diff scan 14 files, 0 findings. |
||
|
Tom Boucher
|
7a7bf19fc1 |
enhance(#2872): record scope and runtime in the install manifest (#3323)
* enhance(#2872): record scope and runtime in the install manifest gsd-file-manifest.json gains manifestVersion, runtime and scope, and a new read-only Installed Surface Resolver Module reads both install scopes for a runtime in one call -- the first code path in the repo that does. Phase 3 of epic #2866 (ADR-2866). Blocks Phase 4 (#2873), which resolves #2218: the resolver's shadowedBy field is that defect expressed as a value for the first time. It ships computed-and-unread here. Installed-ness is decided by manifest PRESENCE, never by the new fields, so a manifest written by an older GSD stays fully functional and no user needs to reinstall. Recorded runtime/scope are corroboration: a disagreement with the probed config dir is reported as declaredScopeMatchesProbe: false, never silently corrected. readInstallManifest is widened additively -- version/timestamp/mode/files keep their exact names, types and meanings for all four existing callers. manifestVersion is a new field rather than a reinterpretation of version, which holds the package version and is read by the golden-parity fixtures. Stems are derived from the installed manifest's own file keys, the inverse of Phase 2's filename composition, guarded by a fast-check round-trip property plus a kebab-case charset check so a crafted manifest key cannot put a traversal segment, control character or ANSI escape into a trigger that Phase 4 renders back to the user. Also fixes two defects found while working: - bin/install.js hardcoded manifestVersion: 2 while the reader owned MANIFEST_SCHEMA_VERSION = 2. Now single-sourced, with a parity test. - docs/installer-migrations.md documented an install-state schema of five snake_case fields that have never been written; InstallState has only ever been { schemaVersion, appliedMigrations }. Corrected with a dated note. Verification runs on the remote runner. * fix(#2872): fold review findings from three independent engines Standards axis: - convert the manifest-schema suite from a hybrid setup(t) closure to beforeEach/afterEach (CONTRIBUTING.md:319-354 Pattern 1). The hybrid was neither approved pattern and a new test forgetting the call got no warning. - SCOPE_ORDER was declared twice with no parity test -- this repo's recorded generative-fix-divergence class. Give the ordering one owner: install-scope exports it frozen, the layout module and the resolver both import it, and a test locks it against scopeRank so the constant and the ranks cannot drift. - drop the defaultReadManifest passthrough (Middle Man). Spec axis: - add the VOLATILE_FILES exclusion test and source comment the acceptance table promised and did not deliver. gsd-file-manifest.json stays excluded: the new fields are deterministic, but timestamp -- the original reason -- is unchanged. Security axis: - bound the reported manifest runtime at 64 chars, matching the truncatePostureValue convention already used in this subsystem. It reached declaredRuntime unbounded while the adjacent stems were gated by SAFE_STEM; an inconsistent posture on the same attacker-influenceable document. The charset stays ungated on purpose -- declaredRuntimeMatchesProbe needs to see the real value -- so Phase 4 must sanitize before rendering, recorded in the design's Known limits. Both new parity tests were verified to FAIL when the two sides are made to disagree, then pass again on revert. Verification runs on the remote runner. * chore(#2872): backfill changeset pr number to 3323 * fix(#2872): give git fixture construction its own timeout class PR #3323's full test (windows-latest, 22, shard 2/3) failed with gitOrThrow: 'git init' failed -- outcome=timed_out exitCode=null gitOrThrow: 'git commit --allow-empty' failed -- outcome=timed_out from drift-detection.test.cjs's beforeEach, a file this branch never touched. Every other lane passed the same commit, including windows-latest node 24 on all three shards, and next is green. Root cause is a bound sized for the wrong class. DEFAULT_GIT_TIMEOUT_MS is 15000 and its own comment scopes it to plumbing READS -- rev-parse, branch, log -- against an existing repo. createFixture uses it for six sequential repo-CONSTRUCTION spawns: init, three config writes, add -A, commit. init and commit each write dozens of files, and on Windows every spawn is Defender-scanned. Sibling tests in the failing block took 15.6-22.0s against a 15000ms bound. This repo already diagnosed this exact shape once: timeouts.cjs's HOOK_FANOUT_TIMEOUT_MS records PR #3285 failing in the SAME job with the SAME outcome=timed_out exitCode=null signature at the SAME bound while every other lane passed, and concludes 'a bound sized for the wrong class, not a slow machine'. It was fixed by splitting out a heavier class-norm at 60000. Same remedy here: GIT_FIXTURE_TIMEOUT_MS = 60000, 4x the bound that failed and half INSTALL_TIMEOUT_MS. DEFAULT_GIT_TIMEOUT_MS deliberately stays at 15000 -- a blanket raise would stop a genuinely hung plumbing read from surfacing fast. Verified the value reaches the spawn rather than being an ignored option: spawnSync was monkeypatched before requiring the fixture module, and all six git construction calls were captured carrying timeout: 60000. This branch's two new test files shift shard composition, which is how a pre-existing fragility landed in the heaviest shard on the slowest lane. Fixed here rather than deferred, per the no-defer rule. Verification runs on the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
80734a9694 |
chore(#3053): clock-seam ADR-456 amendment + backfill — H2 (#3332)
* test(#3314): backfill deterministic clock-seam coverage (failing-first) Replaces loose regex/range assertions with exact-value and boundary tests for the CLI-subprocess and in-process clock-touching call sites identified by H2's audit (epic #3053): cmdCurrentTimestamp, _wsParseRetryAfter (commands.cts), cmdInitManager's is_active gate and cmdInitQuick's quick_id generation (init.cts), and reapStaleTempFiles (io.cts). The CLI-subprocess-pinned tests are expected RED until a follow-up commit routes those call sites through realClock so GSD_TEST_MODE+GSD_NOW_MS can reach them. * refactor(#3314): route CLI-subprocess clock reads through realClock cmdCurrentTimestamp, cmdInitManager's is_active gate, and cmdInitQuick's quick_id generation read Date directly, which the GSD_TEST_MODE+GSD_NOW_MS subprocess pin cannot reach (it only fires inside realClock.now()). Behavior-preserving: realClock.now() falls through to Date.now() whenever GSD_TEST_MODE is unset, which is every real invocation. * docs(#3314): amend ADR-456 with reachability-based clock-control rule ADR-456 §(a) documented one mechanism (injected {clock=Date} + t.mock.timers). Adds the two this repo already relies on: t.mock.timers for in-process direct-Date reads, and the GSD_TEST_MODE+GSD_NOW_MS subprocess pin (routed through realClock) for CLI-spawned code. Updates TESTING-STANDARDS.md's matching passages, which already referenced this issue by number as the no-elapsed-assertion promotion precondition. * fix(#3314): address orthogonal review findings Spec-axis findings: file and link the no-elapsed-assertion promotion follow-up (#3331) instead of leaving TESTING-STANDARDS.md pointing at a dead #1885, and ship the module-by-module audit table in the ADR itself rather than only in a gitignored phase artifact. Standards-axis finding: pin the "hour-old file = not active" test via GSD_TEST_MODE+GSD_NOW_MS for consistency with the reachability rule this PR's own ADR amendment now documents. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8b4545f3c0 |
feat(#3218): the prompt layer asks the CLI for plan counts (#3327)
* feat(#3218): the prompt layer asks the CLI for plan counts Seven sites across four workflows counted plans with ls and wc -l instead of asking the CLI. A shell glob is not scanPhasePlans, so every fix that landed on the owner missed all seven: they counted superseded plans as live, reported zero for the nested plans layout, and missed loosely-named files. The 1762 figure of 30 plans and 24 summaries came from here. phase find is extended rather than a verb added - 3218 is an enhancement whose own checklist says it adds no new command, and CONTRIBUTING makes a new verb a feature needing approved-feature. It gains plan_count and summary_count for the live set and plan_count_all for the physical one, additively; the existing arrays are untouched. Both sets are exposed because the sites need different ones. Amendment 1 names two cases; three of these sites ask a third - did the planner write files to disk - and take the physical set, because a superseded plan is still a file the planner wrote. The progress.md dead route is fixed and was worse than the issue said. It read .plans and .summaries arrays that roadmap.analyze has never emitted, so the fallback always fired, both counts were always zero, and Route 0's resume-incomplete-phase check had never fired at all. The ratchet baseline is empty. Its own stale-entry check makes that self-enforcing. Verified on the remote runner. * test(#3218): acknowledge the workflow growth and update the stale guard The emitted-attribution gate named its own remedy, so it was followed rather than pre-guessed: four workflow files grew between 200 and 770 bytes because each replaced a shell glob with a find-phase call plus its jq extraction. plan-phase grew most - two sites, and it takes the physical count for its did-the-planner- write-files question. progress also carries the Route 0 dead-path fix. plan-phase-drift-guard asserted the literal old ls shape. Updated rather than deleted: what it protects is that a filesystem fallback exists and is reachable, and that is intact. It is not a regression - gsd_run is already load-bearing throughout plan-phase.md long before step 9, so the 9a and 11a fallback never existed to survive gsd_run being unavailable; it guards against the planner subagent's return hanging. Three ack sources collided with the new fragment, which the gate treats as a hard error rather than last-wins. Only the three colliding keys were removed, not the 421 spent entries, and two fragments left entryless were deleted per the convention that an empty fragment signals nothing. Verified on the remote runner. * docs(#3218): document the live and physical plan counts docs/CLI-TOOLS.md gains a find-phase counts section covering plan_count and summary_count for the live set against plan_count_all for the physical one, plus the null-not-zero not-found behavior. The live-versus-physical distinction is spelled out because a caller picking the wrong one gets a plausible number, which is the trap Amendment 1 records. Changeset leads with what a user sees: progress and execute-plan stop counting superseded plans as outstanding, a nested plans layout stops reporting zero, and Route 0 resume routing starts working after never having worked. No how-to. Nothing is enabled and nothing is sequenced - the user runs the same command and the number is simply correct. The one new distinction is field semantics, which is what a reference entry is for. * chore(#3218): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3327 exists. --------- Co-authored-by: sim <sim@local> |