23884a6448dd6123349d4bc2eba1e4f35079e00a
221 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
sim
|
97876a77a4 |
docs(#3308): ADR-3180 §8.1 Enforced, Amendment 9 — Phase 10 validation
Updates docs/adr/3180-planning-semantic-model-single-owner.md to reflect Phase 10 shipping: §8.1 status Required -> Enforced (Phase 10, #3308), guard-roster row contract only -> enforced, phase-index table issue/status backfilled, and a new Amendment 9 recording the guard's real baseline (15 distinct raw-read sites, 21 total acknowledged occurrences in cmdValidateHealth) against the issue's own vaguer estimate, per Amendment 4a's standing "N found by the guard, never per the epic" rule. Also records the intended reading of an absent STATE.md as UNREADABLE-without-diagnostic, symmetric with every other §7 owner's absence-vs-corruption distinction. |
||
|
sim
|
1bc7f7e6b0 |
test(#3339): fold the state/phase/dispatch & model-profile issue-* cluster — Wave 7
Folds 9 legacy issue-*.test.cjs regression files (140 test() blocks) into their module's main suite, per H3 (#3315) of the test-hygiene epic (#3053). LAST of 4 issue-* waves — closes out the 74-file fix-*/issue-* backlog (pending BUG_FILE_RE extension, held for a follow-up commit until Wave 6 is confirmed merged, per the epic's own zero-backlog precondition). - issue-2828-flat-roadmap-total-phases.test.cjs (1) + issue-3204-state- writer-phase-count.test.cjs (21): both target state-document.cjs buildStateFrontmatter via different CLI entrypoints — merged jointly into state-document.test.cjs, 0 dropped. - issue-2945-phase-complete-checkbox-rollback.test.cjs (4) + issue-2949- phase-complete-stage3-sentinel.test.cjs (4): both target phase.cts cmdPhaseComplete; issue explicitly warned of overlap — verified disjoint fixtures/assertions, 0 dropped, merged into phase.test.cjs. - issue-2927-reviewer-lane-overlay-invocation.test.cjs (10) merged into review-lane-descriptor.test.cjs. - issue-2939-dispatch-flatten-maxdepth.test.cjs (9) merged into host-integration.test.cjs, 2 dropped as verified exact duplicates. - issue-2977-frontmatter-bom.test.cjs (5) merged into frontmatter.test.cjs. - issue-2045-third-party-skills-surface.test.cjs (6) merged into capability-loader.test.cjs. - issue-2517-runtime-aware-profiles.test.cjs (80, the largest single fold in the epic) merged into model-resolver.test.cjs, 1 dropped as a verified true duplicate (checked against src/model-resolver.cts logic, not just title similarity). Fixed a genuine eslint irregular-whitespace finding: a literal BOM character embedded in a doc comment (pre-existing content from the original #2977 source, illustrating what a BOM looks like) — replaced with a readable U+FEFF notation. 3 stale doc references found and fixed (docs/adr/2313, 3180, 443). Zero net test-coverage loss. No production code changed. |
||
|
Tom Boucher
|
aee83c8e9e |
test(#3337): fold the manifest & package-identity issue-* cluster — Wave 5 (#3378)
* test(#3337): fold the manifest & package-identity issue-* cluster — Wave 5 Folds 6 legacy issue-*.test.cjs regression files (120 test() blocks) into their module's main suite, per H3 (#3315) of the test-hygiene epic (#3053). Second of 4 issue-* waves. - issue-766-plugin-manifest.test.cjs (50 tests): pure rename (git mv) into plugin-manifest.test.cjs, sole comprehensive suite for its module. - issue-844-manifest-version-sync.test.cjs (14 tests, rename basis) + issue-1855-marketplace-manifest.test.cjs (17 tests, merged in): both target scripts/sync-manifest-versions.cjs via non-overlapping describe blocks (generic engine vs. marketplace.json-specific), now manifest-version-sync.test.cjs, 31 tests, 0 dropped. - issue-498-identity-drift-lint.test.cjs (8 tests, rename basis) + issue-498-package-identity.test.cjs (17 tests, merged in): both target package-identity.cjs's surface from different angles (lint-side drift detection vs. derive/slugify), now package-identity.test.cjs, 25 tests, 0 dropped. - issue-607-cache-lineage.test.cjs (14 tests) merged into the existing gsd-statusline.test.cjs, matching its already-established fold-wrapper convention from prior waves. Learned from Wave 4: fold agents checked src/** (not just docs/ and gsd-core/references/) for stale filename references — found and fixed 5 across 3 ADR docs (766, 2121, 457), zero in src/ this time. Zero net test-coverage loss. No production code changed. * test(#3337): fix orthogonal-review findings — Wave 5 fold Standards-axis review found real issues in the just-folded manifest-version-sync.test.cjs, both fixed here: - The folded:issue-1855-marketplace-manifest block omitted its own test/describe/assert/fs/path/os requires, silently closing over the #844 basis section's outer-scope bindings instead of declaring its own dependencies — inconsistent with every other fold block in this wave. Restored the local requires the original issue-1855 file declared. - Merging two independently-lettered legacy files (each A-F) left duplicate top-level describe() labels (two "A:", two "B:", two "C:"). Renamed the folded-in #1855 labels (A2/B4/C2) to make every top-level label in the file unique. No test() count changed (31). No production code touched. --------- 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> |
||
|
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
|
||
|
Tom Boucher
|
9341d8b8d3 |
test(#3334): fold the workflow-dispatch & review-lane fix-* cluster — Wave 2 (#3342)
Folds 15 tests/fix-*.test.cjs regression files (191 test() blocks) into their module's main suite, per the wave decomposition of #3315 (H3 of epic #3053). 187 blocks land in 8 existing suites (4 exact-duplicate cases dropped, documented inline); 4 blocks move via git mv into 2 new suite files with no prior coverage to merge into. Zero production behavior change. Also tightens two H1 (#3313) ratchets that the fold's own file-count reduction moved past their grace window, per the ratchets' documented dual failure mode (a stale/too-loose baseline fails exactly like a novel violation): - lint-test-file-count.allowlist.json: removes the stale "audit" entry (folding fix-2766 into tests/uat.test.cjs drops that module back to its 2-file cap). - lint-allow-test-rule-refs.ceiling.json: lowers maxFiles 314 -> 309, the real post-fold high-water mark (gsd-test's own repo-baseline test caught this — CI, not a human, found it). Two orthogonal review passes (Standards+Spec code-review, isolated security-review) found and this commit fixes two issues before push: a genuinely-distinct #2287 test case (file-absent vs. file-present- resolved) that a prior fold pass had wrongly dropped as a duplicate — restored verbatim into tests/uat.test.cjs; and a missing same-line allow-test-rule citation on the #2196 block in tests/debug-session-management.test.cjs, added for consistency with its sibling #2257 block. lint-removed-but-needed also caught two stale doc references to the now-folded-away fix-2285-claude-orchestration-wiring.test.cjs filename (docs/adr/1143-claude-orchestration-capability.md, gsd-core/references/execute-phase-response-language.md) — updated both to point at tests/claude-orchestration.test.cjs, its new home. 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
|
aceea3ce4a |
refactor(#3217): withhold a percentage when its scope is not complete (#3318)
* wip(#3217): rule-4 scope withholding — parked, two open findings Implemented but NOT shippable. An isolated review found buildStateFrontmatter still hardcodes SCOPE.COMPLETE, so state json reports percent 0 where roadmap analyze, stats and query progress all correctly report null on the same disk state - rule 4 reintroduced at a site this phase claims to close. Also: roadmap analyze emits scope complete beside progress_percent null with nothing explaining it. Parked to build Phase 4 (#3186) first, which is unblocked. Findings recorded in .gsd/phase/refactor-3217-completion-ratio-scoping/60-review.json. * fix(#3217): withhold the sync percentage on a non-complete scope The parked blocker is fixed - buildStateFrontmatter no longer hardcodes SCOPE.COMPLETE, and the prose Progress fallback is gated too, which was a second leak found while tracing the first. roadmap analyze exposes progress_scope so a consumer can tell WHY a percentage is absent from the JSON alone. Then a residual gap was reproduced rather than assumed. cmdStateSync carried the same hardcode behind a written reason claiming it did not reproduce. It did: on a TRUNCATED window and on UNSCOPED row 4, state sync wrote Progress 0 percent to 100 percent while state json, roadmap analyze, stats and query progress all withheld - and it persisted a self-contradictory file, body claiming 100 percent while its own frontmatter correctly omitted percent. The excuse was also wrong. syncRoadmapRaw is already parsed in that function and is exactly what produces a real scope, so there was a scope to pass. Threaded through listMilestonePhaseDirs; a non-complete scope now skips the write with a reason in changes. milestoneBounded stays as the orthogonal 1761 guard for row 5. Second time this epic a does-not-reproduce claim was too generous. Recorded in ADR Amendment 8 as a correction rather than a quiet rewrite. Verified on the remote runner. * test(#3217): give the withholding fixtures a resolvable scope 40 matrix failures, all fixture drift - no code regression. My own hypothesis that this was over-withholding was wrong and is recorded as such: the worry case, a plain ROADMAP with Phase entries and no version heading, resolves to complete exactly as ADR 7.1 says it should. The real causes were two fixture shapes. Most had no ROADMAP.md at all, which is unreadable via a pre-existing graceful path, and asserted a numeric percent. The five vscode, pi-extension, mcp-server and shell-projection failures were that shape - bare temp dirs using progress json as a reachability proxy while asserting typeof percent is number, which under rule 4 is now null. The rest had a version token in a title or heading with no STATE.md milestone pointer to resolve it, which is classification row 4, versioned but unresolved, so withholding is correct per the contract. Verified on the remote runner. * test(#3217): make the LM-tools reachability tests dispatch against their fixture The gsd_progress reachability test was never testing its fixture. invoke() resolves cwd from vscode.workspace.workspaceFolders by design (the real LanguageModelToolInvocationOptions has no cwd field, per the 2103 fix in extension.js), the mock had no workspace at all, and the test passed a cwd option nothing reads - so it dispatched against the repo working directory. Writing a ROADMAP into the temp dir had no effect. Rule 4 only made it visible. Fixed by mocking workspaceFolders. The two siblings in the same file carried the identical dead cwd and were dispatching against the repo too; they were not failing only because their assertions did not touch scope-dependent output. Both now use their own fixture with assertions unchanged - the no-planning fallback paths already satisfy them honestly. Re-scanned the other five reachability files: no further instances. They thread cwd into parameters that genuinely read it, not through an options shape that ignores it. Verified on the remote runner. * chore(#3217): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3318 exists. * ci(#3217): give the coverage merge enough heap for the merged shards The coverage gate OOMed at exit 134. c8 report merges three shard artifacts, roughly 358MB of V8 dumps in coverage/tmp, and died holding their per-file position maps at the ~4GB default heap. Verified as this branch's delta rather than pre-existing: the same job succeeded on next at 14:18, after phases 4 and 5 merged. Both coverage-gate steps get the bump because both re-slice the same merged data. 8192 doubles what failed and leaves headroom on a 16GB ubuntu runner, matching the idiom the shard step already uses at 6144. This is a memory bound, not a change to what is measured. No threshold was touched. The test file was checked for gratuitous subprocess spawning and is already reasonable at 43 spawns, each a distinct fixture-by-surface pairing. Verified on the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
e201cde73c |
refactor(#3186): one shared phase-completion predicate, disk-strict (#3306)
* docs(#3186): record the disk-strict completion decision in ADR-3180 7.4 The maintainer decided #2957 on 2026-08-08: disk state is authoritative and a ROADMAP checkbox is a human annotation with no machine authority. Section 7.4 still carried the OPEN QUESTION and was marked blocked, so the contract said one thing and the tracker another. Recorded per section 7's own rule - a behavior not stated there is not decided, and amending a rule is an ADR amendment rather than a code change with a comment. The decision comment names Phase 4's PR as the carrier of this edit and makes it an acceptance criterion that the text be in the tree before implementation begins, so this lands first, alone, ahead of any code. Also clears the stale blocked-on-2957 row in the guard roster. * refactor(#3186): one shared phase-completion predicate, disk-strict isPhaseComplete in verification.cts becomes the single owner. It calls readVerificationStatus UNCONDITIONALLY - plan count is not a precondition - so a zero-plan phase with a passing VERIFICATION.md is complete. That is #3168: init gated the read on a plan count and synthesized a not_required sentinel, so phase.complete succeeded while init.manager reported incomplete for the same phase. The guard, built and run before scope was fixed per Amendment 3, found 9 re-derivations where the ADR named 3. Four were unnamed, including one in the prompt layer: mvp-phase.md ORed a ticked checkbox with disk status, which under disk-strict is the divergence itself. Per the #2957 decision, a ticked ROADMAP checkbox is a human annotation with no machine authority. The overrides in roadmap analyze and init manager are deleted rather than generalized; the user's checkbox stays in ROADMAP.md, only its authority goes. scanPhasePlans.completed and buildWorkstreamInventory are deliberately NOT folded - they answer 'are all plans summarized', which is a different question, and folding them would either over-report completion or invert the dependency direction between Phase 1's owner and this one. Verified on the remote runner. * fix(#3186): close seven review findings and record the missing-verdict rule The isolated review reproduced a write-path regression I introduced: migrating cmdRoadmapUpdatePlanProgress dropped its summaryCount>=planCount gate, so a phase with a fresh passing verification plus a newly-added unsummarized plan reported complete AND wrote a checkbox into ROADMAP.md while phase complete refused. The owner stays right per 7.4 - plan count is not a completion precondition - so the gate is restored at the write site as an explicit composition, mirroring the separate 2648 unexecuted-plan gate cmdPhaseComplete already carries. The spec axis was right that my 0.x-split reasoning was too permissive. The 2957 decision names buildStateFrontmatter as one of the three that must converge, and buildWorkstreamInventory combined a summaries-met local with verification data to decide the same verdict - Decision 4(c)'s named bypass, and it reproduced 3168 in a third surface. Both now route through the owner. The raw scanPhasePlans helper stays: it answers are-plans-summarized, which genuinely is a different question. Maintainer decision recorded in 7.4: a missing verdict is not a passing one, so an absent VERIFICATION.md means not complete everywhere. That retires 2645's verifier-disabled tolerance and inverts its Goodhart incentive - deleting the evidence now lowers completion instead of raising it. Guard hardened: block-form count gates and algebraic restatements are caught, and the header now discloses its remaining limits instead of overclaiming. Verified on the remote runner. * fix(#3186): route state sync through the owner and catch bare completed reads The matrix found 52 failures. 51 were fixtures asserting the old semantics: a phase with plans and summaries but no VERIFICATION.md used to count complete and correctly no longer does. Each fixture now carries a passing verification where that is what the test was actually about, rather than having its assertion weakened. The 52nd was a real 10th re-derivation the guard could not see. cmdStateSync destructured scanPhasePlans().completed directly - a bare field read, not a comparison - and used it as a completion verdict, so state sync and state json disagreed on completed_phases for identical disk state. Routed through the owner. Guard gains shape (d): any read of .completed off a scanPhasePlans() result outside plan-scan.cts, in chained, destructured and indirect forms, function scoped with no line window. It cannot tell a summaries-met read from a completion read - that is data flow - so it flags every one and requires a written-reason exemption, which is the same discipline shapes a-c already use. The blind spot is disclosed in the header rather than overclaimed. The emitted-attribution failure was also mine, not pre-existing: the mvp-phase.md checkbox-OR removal moves emitted bytes, acknowledged in tests/emitted-drift-acks. Verified on the remote runner. * test(#3186): give the nested-plans sync fixture a passing verification Last 3 matrix failures were one failure echoing up two describe levels. Phase 01-alpha had plans and summaries but no VERIFICATION.md, so under disk-strict completed stayed 0 and no Progress change was emitted - correct new behavior, not a regression. Added the passing verification rather than dropping the Progress expectation, so the test still covers what #3257 is about: that a nested plans/ layout is counted and not undercounted. Probe against the built lib confirms Progress: 0% -> 50% alongside Total Plans in Phase: 0 -> 3. * chore(#3186): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3306 exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
95d0da9060 |
docs(#3287): add ADR-3180 decision 8, the diagnostic contract (#3292)
Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
693f12ad56 |
refactor(#3187): give state field extraction one canonical owner (#3283)
* refactor(#3187): give state field extraction one canonical owner stateFieldValue in state-document.cts becomes the single owner of the #1760 frontmatter-then-body fallback chain. The new whole-repo guard found 14 independent re-derivations where the epic scoped 5, all now routed through it: cmdStateSnapshot (11), cmdStatePrune (2) and smart-entry fmScalar (1). state validate was a gate that could not fail. Every warning it could emit sat behind a phase resolved without the frontmatter tier, so a STATE.md whose phase lives only in frontmatter skipped the drift scan entirely and returned valid:true. It also read unstripped content, letting a frontmatter status: key shadow the body field (#1255 class). Both fixed; output gains a scope field so could-not-look stops being output-identical to looked-and-clean. Verified on the remote runner. * docs(#3187): document the state validate scope field and its reason codes Adds docs/how-to/interpret-state-validate-results.md so a reader can tell nothing-to-report from could-not-look, updates the COMMANDS.md and USER-GUIDE.md entries, corrects the CONTEXT.md glossary overstatement about Current Position sole ownership, and drops the changeset fragment. * fix(#3187): close three drift-guard evasion shapes and test the refuse path The isolated adversarial review found the ladder detector was evadable by ordinary reformatting, not just deliberately: a member or computed operand (fm.key / fm[key]) missed the bare-identifier backreference, a swapped tier order missed a hardcoded number-then-boolean sequence, and a ladder wrapped across lines missed single-line detection. All three now caught, each with its own test plus a proven boundary control. The frontmatter-parse refuse path on the destructive complete-phase route was unreachable and therefore untested. It is now driven by an injected parse failure and asserts STATE.md is byte-identical after the refusal, rather than shipping untested defensive code on a path that rewrites user state. Verified on the remote runner. * fix(#3187): widen the drift guard to the prompt layer and disclose tier-2 changes The code-review spec axis found the guard's scan surface was src/ only, which is Decision 4(d)'s forbidden allowlist one directory wide - and it had a live miss: gsd-core/workflows/smart-entry.md tells an agent to read status from frontmatter or the body, a prose expression of this same chain. The surface now covers the prompt layer. That one site carries a permanent written exemption rather than a ratchet: it is the gsd-tools-is-down fallback, so it cannot call the owner by construction, and a ratchet would imply removable debt that does not exist. Two tier-2 output changes shipped undisclosed and are now named in the changeset and docs: complete-phase's idempotency guard consulting frontmatter, and the workstream inventory resolving frontmatter-only fields. docs/COMMANDS.md gains a state complete-phase entry, which it never had. Also records Amendment 5 on ADR-3180, extracts the duplicated frontmatter-parse block the epic's own thesis forbids, and re-points two assertions from free-form warning prose onto the structured drift object. Verified on the remote runner. * chore(#3187): backfill changeset PR number pr:0 placeholder replaced with the real PR number now that #3283 exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cf6de5e1c0 |
feat(#2871): resolve triggers and host precedence, not just placement (#3291)
* test(#2871): failing-first suite for trigger-surface resolution 23 tests over the 50-test-matrix rows. RED by construction: resolveTriggerSurface and DEFAULT_TRIGGER_PRECEDENCE do not exist yet, and the validator silently ignores triggerPrecedence today. Written in the per-runtime describe idiom the other four runtime-artifact-layout suites use, not a table. The rows that carry the weight: windsurf must NOT report a shadow it does not have, since its global scope emits only agents and agents are not trigger-bearing; agents and kimi-agents must be absent from the output for every runtime; and reordering a runtime's triggerPrecedence must flip the winner, which is the only assertion that proves the axis is read rather than decorative. Stems are injected, never scanned, so the surface is assertable with no filesystem. * feat(#2871): resolve triggers and host precedence, not just placement resolveTriggerSurface(runtime, scopes) returns every /gsd-<name> trigger a runtime emits, with the scope and kind that produced it, whether the host registers it directly or only through a router, and which artifact shadows it. resolveRuntimeArtifactLayout is untouched -- its 7 callers need placement only and the issue requires them unchanged. AGENTS ARE NOT TRIGGER-BEARING, and ADR-2866 said they were. The host-integration matrix models command and dispatch as separate interface points: an agent is invoked through the Agent tool's subagent_type, not by typing a slash trigger, and _copyStaged never applies the kind prefix to an agents entry. So agents and kimi-agents are excluded from the surface entirely, and this commit amends ADR-2866 with a dated correction. #2218's conclusion is unchanged -- the collision is strictly commands-vs-skills, and claude's local /gsd-* trigger surface is still fully shadowed -- but the ADR implied the local agents surface was lost too, and it is not. That correction is what makes windsurf come out right. Its global scope emits only agents, so it has no global trigger and its local commands are unshadowed. Model agents as trigger-bearing and windsurf falsely reports a full shadow. The triggerPrecedence axis lands on all 19 descriptors as an ordered kind list, one value with one owner, rather than a numeric rank spread across N kind entries with nothing keeping them consistent. Validation uses a required-with-default shape that has no precedent in this validator -- every existing axis is hard-required -- so a third-party capability.json omitting the field still validates, which is what ADR-894's additive-only contract promises. Winner resolution reads Phase 1's scope rank first, then the kind ordering. A test reorders the axis and asserts the winner flips, since an axis that is added, validated and never consulted would pass every other assertion. shadowedBy ships unread. Phase 4 (#2873) is its first consumer, per this issue's out-of-scope note. Verified via the remote runner. * fix(#2871): single-source namespacedByDir and close two test gaps Four findings from the isolated adversarial review. The namespacedByDir rule had reached three copies -- install-engine, surface, and the new trigger resolver -- one of which carried a hand-written keep-in-sync comment and no assertion. That is this repo's generative-fix-divergence class. Extracted to one exported predicate all three now call. Verified by diverging one copy deliberately: the existing #816 parity test failed, and passes again on revert. The omission test was vacuous. Row 16 asserted that a descriptor without triggerPrecedence still validates, but built its fixture from claude's shipped descriptor -- which this PR had just added the axis to. It now clones and deletes the key, following the shippedDescriptorWithout pattern, and asserts both that validation passes and that the resolver still picks the right winner from the default. The second half is what makes it prove anything. resolveTriggerSurface silently dropped an unrecognized scope while every sibling in this epic throws. Two phases of one epic should not disagree about whether an invalid scope is an error, so it now rejects through the same shared validator; an empty scope list still returns empty rather than throwing. The ADR amendment had been spliced into the middle of the References list, orphaning its last bullet. Moved to the top, after the header block, which is where ADR-3660 and ADR-1016 both put dated amendments. No lint checks markdown structure, so this was green while malformed. * fix(#2871): single-source the command filename composition too The earlier fix shared the namespacedByDir boolean but left the filename composition around it written twice -- once in _copyStaged as what actually gets written, once in resolveTriggerSurface as what gets predicted. The predictor could go stale silently. One exported helper now composes it for both. The entry.name asymmetry that looked like it would block extraction does not: entry.name is filtered to end in .md and stem is entry.name minus those three characters, so the two branches are the same string by construction. Divergence proven to fail: injecting a marker into the helper broke the trigger-surface suite; reverting restored 25/25. The four sibling layout suites hold at 227 unchanged. * docs(#2871): correct the ADR timing notes that this phase makes stale The Amended by back-links on ADR-3660 and ADR-1016 were written in Phase 0, when the widenings they describe had not shipped. Each carried a forward-looking clause -- "the module changes at Phase 2, not before, until then this module resolves placement only" -- which becomes false the moment this PR merges. ADR-2866's own Amends header and its reciprocal-notes section carried the same tense. All four now describe what shipped. This is a tense and status correction on Accepted ADRs, not a change to any decision. Worth stating because it is the failure mode this epic keeps meeting: gen-adr-index.cjs tracks only Supersedes and Subsumes, so nothing in CI would have caught either the missing back-link in Phase 0 or these stale clauses now. They stay correct only because someone checks. * chore(#2871): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b901d1e06f |
feat(#1953): complexity-triggered refactor extension point (execute:post) (#3261)
* test(#1953): failing-first suite for the complexity-triggered refactor hook 60 behavioral cases against src/complexity-trigger.cts, which does not exist yet: decision-point counting, the comment/literal stripping leak surface, threshold and jump-delta boundaries at limit-1/limit/limit+1, stable-anchor baseline semantics, and fs fault injection via mock.method. Two fast-check properties assert that stripping never manufactures a decision point and that comments and string literals are score-neutral. Also registers the refactor-trigger capability manifest (inert until refactor.trigger_enabled) and regenerates the capability registry and matrix. Verified RED on the remote runner before any implementation exists. * feat(#1953): complexity-triggered refactor extension point Adds the opt-in refactor-trigger capability. After a phase executes, an execute:post step measures per-function complexity for the files the phase touched and writes a scoped refactor proposal when a function crosses the configured threshold or drifts past its recorded anchor. Design notes worth carrying: - The signal is computed in-core (decision-point counting over comment- and literal-stripped source, Node builtins only) rather than via Memtrace or a shelled-out analyzer. The hook fires as a deterministic CLI, not an agent with MCP tools, and core takes no external dependencies — this is the only option a behavioral test can bind to. The metric sits behind a seam. - The baseline is a stable anchor, not a rolling value: set on first observation, moved only on disposition. A rolling baseline makes the delta the single-phase change, so a function creeping +2 per phase never trips a delta of 5 and the jump check adds nothing over the absolute threshold. - Strict mode records an open deviation window in the broken-windows ledger rather than declaring its own ship:pre gate. ship.md has no generic ship:pre gate dispatch — only two hardcoded branches — so a third gate of any kind would be declared and never evaluated. - The gate clears on the proposal being dispositioned, never on the score improving. A blocking complexity number is one an executor can satisfy by splitting a coherent function in two. execute-phase.md gains a generic execute:post step-dispatch contract; it previously matched only ref.skill == "code-review", so any other step registered there was declared and never run. The code-review branch is unchanged. Full rationale in ADR-1953. Closes #1953 * fix(#1953): close git option injection and symlink escape in the refactor hook Three findings from the isolated security review, all fixed inline. HIGH — changedFilesSince interpolated the --since value into a revision token placed before the -- separator. A -- only stops PATHSPEC parsing of arguments after it; git still option-parses what comes before. So --since '--output=/tmp/x' became --output=/tmp/x..HEAD, which git accepts as --output=<file> and uses to redirect diff output — an arbitrary write. Fixed with --end-of-options before the revision range plus a conservative ref validator. The validator deliberately permits ~ ^ @ { } because those are legitimate git REVISION syntax (HEAD~1, main@{yesterday}) as distinct from ref-NAME syntax; --end-of-options is the actual barrier. The doc comment asserting the trailing -- was sufficient was wrong and is corrected. MEDIUM — resolveConfinedPath confined by string prefix only, so a symlink committed inside the repo passed the check (its own path is under cwd) and readFileSync then followed it outside the root. Now lstat-checks for a regular file and skips anything else with REFACTOR_FILE_UNREADABLE, so one bad path skips one file and the run continues. LOW — the new execute:post dispatch contract showed the gsd_run example before the rule requiring ref.command be validated first. That prose is executed by an agent, so textual order is execution order. Reordered. Refs #1953 * fix(#1953): make the analyzer able to see TypeScript at all Found by running the shipped analyzer over its own source: it reported functions=1 for a 940-line module with 24 function forms. A return-type annotation or a generic parameter list made a function invisible — `function f(a): number {}` and `function f<T>(a: T): T {}` both detected as zero. Since gsd-core is written in .cts and the capability declares .ts/.cts/.mts analyzable, the feature silently found nothing in this repo's own primary language while reporting success. A safety net that reports "all clear" because it cannot see is worse than no safety net. All 98 tests passed over this, because every fixture was plain JS — the exact failure the test matrix's own "assert against the shape production uses" warning describes. Adds a TypeScript-shapes suite covering return types (including unions, generics, object literals and type predicates), generic parameter lists (constrained and defaulted), export/async/generator combinations, annotated arrows, class-method modifiers, and optional/ default/rest params — plus the two traps: an overload signature has no body and must not count, and `a < b && c > d` is a comparison, not a generic. Detection now reports 24/37/21 functions for the three source files, which matches a hand count exactly. Also from review: - The strict-mode ledger dedup identified entries by parsing a prose description string. That is banned by CONTRIBUTING's raw-text-matching rule and was a real bug: the "exactly one window per untriaged proposal" guarantee rested on prose matching, so rewording a description or editing WINDOWS.md by hand silently produced duplicates. Now matches structurally on kind + phase + file + line. - A property test asserted on the stripper's output text. Reframed to assert the same invariant through analyzeSource's score. - nextBaseline's `candidates` parameter has been dead since the anchor change; removed from the signature and all call sites. - Extracted the duplicated require-or-degrade and capability-check boilerplate. - ADR-1953's Implementation bullet still named a `refactor.ship-gate` in check-command-router.cts — a leftover from the design cut D6 rejects. That file is untouched and no such gate exists. Removed. Refs #1953 * fix(#1953): keep execute-phase.md under its byte ceiling; un-vacuum the large-file test Five of the seven remote-runner failures were one cause: the execute:post dispatch contract, written out inline, grew execute-phase.md 1876 bytes (93,400 -> 95,276) against a frozen PRE_PHASE6 ceiling of 93,600. A drift-ack does not clear that — tests/phase6-capstone-conformance.test.cjs and tests/fix-2285-claude-orchestration-wiring.test.cjs assert the file is literally under the cap. The contract now lives in gsd-core/references/loop-hook-dispatch.md, which already claimed to be the point-agnostic dispatch reference and already documented ref.skill and ref.agent. It gains the ref.command shape, its in-context validation rule, the advisory-by-construction statement, and a note that a point whose workflow hand-rolls one kind is not implementing this contract. execute-phase.md now defers to it in one line: 145 bytes of growth, 55 B of headroom under the cap. Better placement than the first cut — the reference was overstating its coverage, and this makes the claim true rather than duplicating prose next to it. Acknowledged by appending to tests/emitted-drift-acks/2930-*.json rather than a new 1953-*.json: two ack sources may never name the same path, and that fragment is already the accumulating ack for this file. Sixth and seventh failures: analyzesLargeFileWithinBounds tripped its own vacuity guard — the fixture generated ~480 KB against a `> 500000` assert, so the guard fired and the three assertions after it never ran. The test has been vacuous since it was written. The matrix row specifies ~1 MB, so N goes 8000 -> 20000 (1.17 MB, 17% margin) and the guard to > 1_000_000. Verified by reproducing the exact body against the compiled module: 1168888 bytes, 118 ms, all four assertions hold. Refs #1953 * fix(#1953): fold the execute:post step deferral into the existing resolve line The remaining two failures were one test: execute-phase.md carries a SECOND, tighter assertion than the 93,600 ceiling — `<=93400`, which is exactly its current size. The file cannot grow by a single byte. My previous fix got it under 93,600 but not under 93,400, so it still failed. ("H." in the report is just the parent describe of that same test, not a separate defect.) Rather than add a paragraph, the deferral now REPLACES the existing hook resolution line. It read: Resolve active step hooks from `EXECUTE_POST_HOOKS_JSON` where `kind == "step"` and `ref.skill == "code-review"`. which is the bug itself written down — only code-review was ever dispatched. It now reads: Dispatch each `kind == "step"` hook per @gsd-core/references/loop-hook-dispatch.md. For `code-review`: The following prose already begins "If no active code-review step hook exists", so it reads correctly and the code-review handling is untouched. Net effect on the file is -11 bytes: 93,400 -> 93,389, under the margin assertion rather than merely under the ceiling. That also removes the need for a drift-ack: the file shrank, so there is no growth to acknowledge, and the append to the shared 2930-*.json fragment is reverted. Leaving it would have shipped a claim of "145 bytes of growth" that is no longer true, on a file six other issues share. The test's own comment states the principle this ended up honoring: "the host loop must stay small — optional-feature detail belongs in the capability fragment, not the host workflow." Putting the dispatch contract in the reference rather than inline is that rule, applied. Refs #1953 * fix(#1953): keep the code-review hook literal the workflow test requires tests/code-review.test.cjs extracts the <step name="code_review_gate"> block and asserts it contains `ref.skill == "code-review"` verbatim. The previous commit replaced the line carrying that literal, so the token vanished and the test went red — a fair assertion: code-review IS the bespoke branch there and the workflow should still name it. Restored inside the same one-line deferral, which now reads: Dispatch `kind == "step"` hooks per @gsd-core/references/loop-hook-dispatch.md. `ref.skill == "code-review"`: 93,396 bytes — still under the `<=93400` margin assertion and 4 bytes below the base, so the file continues to shrink rather than grow. Because three consecutive runs were each reddened by a different assertion on this one file, this change was verified by sweeping ALL of them at once rather than one run at a time: every test under tests/ that reads execute-phase.md or references/loop-hook-dispatch.md was located by resolving its path constants, and each content/size assertion was evaluated directly against the working tree — 22 assertions, plus two real executions (gen-section-manifest --check, and emitted-attribution's full real-tree differential). All pass. That sweep also confirms the earlier judgement call: the net change to execute-phase.md is a SHRINK, and the size ratchet only gates growth, so reverting the append to the shared 2930-*.json ack fragment was correct — an ack would have been both unnecessary and factually wrong. Refs #1953 * chore(#1953): backfill changeset pr number to 3261 * docs(#1953): add the missing how-to for acting on a refactor proposal Reference and explanation shipped (COMMANDS.md, CONFIGURATION.md, FEATURES.md 159, ADR-1953) but the Diataxis how-to quadrant did not, and that is the one a user reaches for. CONTRIBUTING's required-docs table is 'new command -> COMMANDS.md + FEATURES.md', so CI was green on a gap. Enabling this feature is genuinely multi-step and no single page walked it: turn it on, tune the threshold, understand advisory vs strict, discover that strict needs a SECOND toggle on a DIFFERENT capability, and know what to do when a proposal appears. The two-toggle subtlety in particular was a footnote in a config table; here it is a section with both commands. Follows the shape of its closest siblings, resolve-edge-coverage-findings and resolve-prohibition-findings — both 'the loop surfaced a finding, here is what to do with it'. Includes a reason-code table for the silent cases, since the analyzer is deliberately quiet in six situations and a user who expected a proposal needs to tell 'nothing to report' from 'could not look'. Indexed from docs/README.md beside the other loop how-tos. Docs-only: exempt from the push gate, no re-verification, pass marker on 2af188b4 untouched. Refs #1953 * feat(#1953): warn when strict mode is on but nothing will actually block Closes acceptance criterion 5, which I had wrongly marked satisfied. refactor.trigger_strict records an untriaged proposal as an open deviation window, but a ship only STOPS if workflow.windows_enforce is also on — a toggle owned by the broken-windows capability that this feature neither sets nor requires. So a user could enable strict, believe ship was gated, and find out otherwise at ship time. The split itself stays: requires:["broken-windows"] would force-install the ledger on advisory users who never enable strict, and a ship:pre gate of our own would never fire because ship.md has no generic ship:pre gate dispatch. What was missing was discoverability, so that is what this fixes. `refactor evaluate` now emits a typed REFACTOR_STRICT_NOT_ENFORCING warning, naming the exact remediation command, whenever strict is on and either workflow.windows_enforce is off or broken-windows is unavailable. It fires only on a run that produced a candidate — with nothing to block on there is nothing to warn about, and warning every run would be noise. Reads workflow.windows_enforce through the same resolveConfigKey walk the router already uses for its own keys rather than a second config reader. Four tests cover the matrix: strict+enforce-off warns, strict+enforce-on does not, strict+ledger-absent warns, strict-off never warns. Also corrects a user-facing message in this same file that told the user to run `gsd-tools config-set` — the wrong form. docs/CONFIGURATION.md and the broken-windows capability both use `gsd config-set`, and gsd-tools is invoked as `node gsd-tools.cjs`, so the bare form may not resolve. The two adjacent messages in this file now agree. Refs #1953 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
653f95e39f |
chore(#2801): remove the hostBehaviors.reviewerCli deprecated alias (#3272)
* test(#2801): failing-first suite for the hostBehaviors.reviewerCli alias removal Inverts the Phase 5a rows that assert the derived legacy alias still contributes a reviewer slug, and adds the removal-warning coverage the alias's exit needs (ADR-2782 D9). RED against unmodified production code, by design: the six shipped manifests still declare the key and collectReviewerWarnings emits nothing for hostBehaviors. Refs #2801 * chore(#2801): remove the hostBehaviors.reviewerCli deprecated alias ADR-2782 D9, Phase 7 — the final phase of epic #2782. The derived legacy alias survived one release (Phase 5a shipped in 1.9.0; 1.9.1 and 1.10.0 have since gone out), so it goes. A declared reviewer body is now the only route onto the reviewer roster. - deriveReviewerSlugs no longer reads runtime.hostBehaviors.reviewerCli - the key is stripped from the six manifests that carried it; each already declares a reviewer body whose slug equals its capability id, so the derived roster is unchanged at the same twelve slugs - collectReviewerWarnings emits a presence-based, non-fatal removal notice for any manifest still declaring the key, reaching both the build-time registry generation and the third-party overlay load path. The check runs before the reviewer-body early-return, because the manifest it exists for is the alias-only one that has no body. - hostBehaviors stays an open, unvalidated bag for its other 59 keys; this adds one keyed removal notice, not general validation Refs #2801 * refactor(#2801): give the reviewer-warning channel a typed IR Review finding: the new tests asserted with String#includes() on the warning prose, which CONTRIBUTING.md's 'Prohibited: Raw Text Matching on Test Outputs' bans in favor of a typed intermediate representation. Adds the IR beside the renderer rather than replacing it, which is the shape that section prescribes and bin/verify-reapply-patches.cjs already models: - REVIEWER_WARNING, a frozen code enum - REMOVED_REVIEWER_CLI_FIELD, so the emitting site and its test share one symbol instead of duplicating a literal - collectReviewerWarningRecords(cap), returning typed records collectReviewerWarnings(cap) keeps its exact string[] contract as a thin map over the records, so both production consumers are untouched. Every section-K row now asserts on record.code/field/capId and none on the rendered message. Locks the code surface, asserts the renderer stays one-to-one with the records, and migrates the pre-existing Phase 2 test on the same channel off prose matching. Refs #2801 * test(#2801): invert the section F alias fall-through regression row Caught by the remote runner: 2 unique failures on both Node lanes out of 31,692. tests/reviewer-lane-declarations.test.cjs section F — Phase 5a's isolated-security-review regressions — asserted that a blank reviewer.slug falls through to the hostBehaviors.reviewerCli alias rather than dropping the lane. That is the direct inverse of this phase's contract. The original rationale held only while the alias existed. With it gone there is nothing to fall through to: a blank body is not a declaration, and a declaration is the only route onto the roster. Inverted rather than deleted — the row carries the adversarial-review provenance for the slug trim, and removing a security regression guard to make a change pass is backwards. The duplicate row added earlier in section C is dropped instead; section F is its canonical home. Also corrects two count strings Phase 5b left at eleven while asserting twelve, which would misreport on failure. Refs #2801 * docs(#2801): give the removed reviewerCli flag a migration path The Reference edit alone satisfied CI — a file under docs/ moved, so lint-docs-required.cjs was green — while the task-oriented quadrant said nothing about the removal. A maintainer whose lane had just gone silent would have found the field documented as removed and no page telling them what to do about it. Adds a migration section to the how-to: the symptom, the verbatim warning they will see, the before/after manifest, and the note to keep the reviewer slug equal to the capability id so existing review.default_reviewers entries and --<slug> flags survive. Refs #2801 * chore(#2801): backfill changeset pr number to 3272 * feat(#2801): close the runtime.hostBehaviors vocabulary ADR-1016 closes twelve descriptor axes and rejects an open escape hatch in the descriptor. It never mentioned runtime.hostBehaviors, and that silence was read as permission: 59 keys across 18 manifests, 39 of them set by a single capability, validated by nothing. The reference docs went further and attributed the open seam to ADR-1016, which does not mention the field at all. KNOWN_HOST_BEHAVIORS enumerates the vocabulary. An undeclared key yields a non-fatal UNKNOWN_HOST_BEHAVIOR record on the same D4.3 channel as the alias removal notice, reaching both build-time generation and overlay install. Warning, never error, for the reason this phase exists: an error would hard-break an out-of-tree descriptor carrying a bespoke key with no deprecation window, which is what reviewerCli was given a release to avoid. Escalation is a separate decision. reviewerCli is excluded from the unknown-key sweep so it keeps its own notice with the migration pointer rather than drawing two records. A parity test binds the vocabulary to the shipped manifests in both directions, and a second asserts no shipped capability draws a notice, so the closure is provably inert in-tree. Records the decision and the miscitation as an ADR-1016 amendment. Refs #2801 * fix(#2801): bound and sanitize the unknown-key diagnostics Two findings from an isolated adversarial review of the closure commit, both proven by execution rather than asserted. MAJOR, introduced by the closure: the new Object.keys(hostBehaviors) sweep had no ceiling. An installed third-party manifest is bounded only by MANIFEST_MAX_BYTES, and an 8.69MB manifest with 800,000 keys produced 800,000 records and ~139MB of message text, retained for the registry's lifetime in OverlayMeta.diagnostics. Now capped at ten records plus a summary carrying omittedCount, mirroring capability-loader's existing slice(0,3) idiom. The same manifest now yields 11 records and 1748 chars. MINOR, newly reachable: manifest-supplied key names were interpolated raw. Unlike cap.id, which validateCapability gates on KEBAB_RE before these diagnostics run, hostBehaviors keys have no grammar check anywhere, so ANSI escapes and CRLF reached stderr and OverlayMeta.warnings intact. New describeKey replaces C0/C1 controls and clips at 80 chars. The file already had describeValue for this and applied it only to values. Both fixes land on the pre-existing reviewer.* sweep too — it carried the identical pair, and fixing only the new copy would leave the same defect one screen from its own fix. Refs #2801 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
58d73dd220 |
enhance(#3241): omit the codex per-agent model by default (#3276)
* test(#3241): failing-first suite for the codex passive model posture Locks ADR-2313's D1-D5 before any production code exists, so the tests bind to the behavior rather than to whatever the implementation happens to do. Red-first (fail against the current tree): - the resolver path emits no `model` and no `model_reasoning_effort` - a whitespace-only model_overrides value yields no pin - isAnthropicFlavoredModel / CLAUDE_AGENT_ALIASES on model-catalog - the one-time install notice, and its once-per-install dedupe Regression guards (pass today, must keep passing): resolver-null via `inherit` and via absent runtime; a resolver that resolves to nothing; empty-string and non-string overrides; and the light-tier service_tier/model_verbosity fields, which are NOT coupled to the model pin and would silently regress if the implementation coupled them. Classifying each test as red-first or regression guard is deliberate. A test that passes on both sides of the change proves nothing, and this epic has already shipped two such rows before catching them. The whitespace case is a live defect, not a quirk: `' '` is truthy, survives the type guard, is not Anthropic-flavored, and is embedded verbatim as `model = " "` — the same class the #2310 guard exists to stop. Same function, same path, fixed in this phase per CLAUDE.md §3. Two matrix rows were dropped as vacuous rather than shipped green: a 64-char truncation case (the pinned notice interpolates no user-controlled value, so it cannot exhibit truncation) and a newline hazard that the input surface cannot reach. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3241): omit the codex per-agent model by default Implements ADR-2313 D1-D5. generateCodexAgentToml no longer embeds the runtime resolver's per-tier Codex model, so an agent inherits the always-available session model instead of a pin a ChatGPT-account Codex may not expose. model_reasoning_effort disappears with it via the existing hasPinnedModel coupling (#838) — no logic change needed there. Supersedes #2517's embedding on the default path only. An explicit real-Codex model_overrides pin is still embedded verbatim, and the #2310 Anthropic-flavored guard is retained: the model_overrides route to it is still live even though the resolver route is now unreachable. The shared rule moves down a layer. CLAUDE_AGENT_ALIASES leaves model-resolver for model-catalog — a genuine leaf importing only node:path and its own JSON — with isAnthropicFlavoredModel defined beside it, and is re-exported from model-resolver so every existing importer is untouched. This is what lets Phase 2's install-check and Phase 3's sync consume the rule without taking the config-loader dependency model-resolver would have dragged into a module documented as pure read/verify with 33 dependents. A parity test fails if the two ever fork. Also fixes a live defect surfaced while writing the tests: a whitespace-only model_overrides value was truthy, survived the type guard, was not Anthropic-flavored, and so was embedded verbatim as `model = " "` — the same class the #2310 guard exists to stop, reached by a different route. Trimmed before the truthiness test. It is deliberately not routed through _warnCodexModelOverrideDropped, whose text would misdescribe a blank field as a mis-typed model. Adds the one-time install notice (maintainer direction, recorded as an ADR-2313 amendment): one stderr line naming model_overrides and the session model, deduped per install rather than per agent, and emitted only for the population that actually loses a pin. service_tier and model_verbosity stay decoupled from the model (#774); a regression guard asserts they still emit with nothing pinned. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3241): amend ADR-2313, add the model-catalog glossary entry ADR-2313 gains two dated amendments rather than edits to its merged text, since ADRs here are append-only. The first records that a deprecation notice IS offered, reversing the Migration section's "no deprecation window" position, and states why that position was wrong rather than just superseding it: the ADR identified the API-key population as losing something real and then declined to warn it, in the same document. Hyrum's guidance was applied to the recourse and not to the notice. The second records the whitespace-only model_overrides defect and notes that D2 always implied the fix — the implementation simply never enforced it and no test covered the case. CONTEXT.md gains a Model Catalog Module entry. The module had none, which is why the glossary gate passed without one: check-glossary-refs verifies that references resolve, not that modules are documented. The entry records why the Anthropic-flavored rule lives there rather than in model-resolver, so a later reader does not "helpfully" move it back. The Model Resolver entry is updated to point at its new home and note the back-compat re-export. docs/CONFIGURATION.md carried a claim that is now false: that the resolved tier ID is embedded into agent frontmatter at install time on codex and opencode. Corrected to name codex as the exception, with the 400 symptom and the model_overrides recourse. Changeset leads with the user-visible change and the migration line rather than the implementation, per the ADR's Hyrum's-Law analysis. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3241): only notice a lost pin when one was actually embeddable Review finding from an isolated reviewer. The deprecation notice gated on whether the runtime resolver would have returned *any* model, but the question that matters is whether that model would have been *embedded*. Those differ. The #2310 safety gate already rejected an Anthropic- flavored model arriving from the resolver path before Phase 1 — so for a mixed-runtime config resolving to a claude-* id against a Codex install target, the user never had that pin. The notice told them they lost something they never got, and pointed them at model_overrides for no reason. The existing #2310 test drives exactly that path but asserts only the emitted `model` line, never stderr, which is why it slipped through. Now covered. Deliberately unchanged: an Anthropic-flavored model_overrides value plus a legal resolver model fires BOTH the override warning and the notice. That is correct — pre-Phase-1 the guard dropped the override, execution fell through to the resolver, and the resolver's model was embedded, so that user did lose a pin. Two messages, two distinct true facts, and the prefixes differ (`gsd: warning — ` vs `gsd: notice — `) so the one-notice-per-install contract holds. A regression test now pins that behavior so it does not get "simplified" away. Of the three tests added, only the first is red-first; the other two pass on both sides by design and are labelled as guards — one against over-correcting the fix into silence, one against removing the intentional double message. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3241): reset the notice dedupe via a seam, not a require.cache bust The remote runner caught a regression I introduced: the #2760 post-write-validation test began failing with the validator override no longer intercepting. Cause, confirmed by trace rather than guessed: the new #3241 review tests deleted require.cache for bin/install.js and re-required it mid suite, to clear the notice's module-level dedupe flag. But runCodexInstall destructures `install` at file load, closing over the ORIGINAL module's exports. After the cache bust a second instance existed, so the test's `installModule.__codexSchemaValidator = ...` mutated the new object while the code under test still called the old one. The override silently stopped intercepting, the real validator ran and passed on GSD-emitted output, and the abort-and-restore path was never exercised. Cache-busting a module mid-suite breaks every later test that assumes a single instance, which every other test in the file is entitled to. So the fix is a seam, not a workaround: bin/install.js exports _resetCodexNoticeDedupeForTests(), and the three tests call it directly instead of reloading the module. The flag is module-level by design — the dedupe is per-install and install() already resets it — so a unit test driving generateCodexAgentToml directly needs an explicit way to reset it. That is now what it has. Swept the rest of the #3241 diff for the same hazard; this flag was the only shared module-level state introduced. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3241): reset both codex dedupe stores, not just the notice flag Second incomplete fix, same class one layer down. bin/install.js keeps TWO module-level dedupe stores and the require.cache bust I removed had been papering over both; my replacement seam cleared only one. _codexModelOverrideDroppedWarned is a Set keyed `${agent}::${value}`. tests/codex-config.test.cjs:558 already emits for `gsd-executor::sonnet`, so by the time the review test using the same agent and value ran, _warnCodexModelOverrideDropped was a silent no-op and the expected warning never appeared. The seam now clears both stores and is renamed to say so. Its comment records that per-install dedupe lives in module scope deliberately and that this is the single sanctioned way for a unit test to clear it. Swept bin/install.js for every other module-scope mutable a test could latch. Two are inert (capability registries assigned once at require time; selectedRuntimes computed once from argv). One is a genuine latent hazard and is deliberately NOT folded in: attributionCache (:1654) memoizes getCommitAttribution by runtime name for process lifetime, so two in-process installs of one runtime with differing attribution config would collide. It is unreachable from any current test and is a different concern from Codex warning dedupe, so it stays out of this PR rather than widening it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3241): correct the codex tier-routing how-to The docs gate forced the task-oriented quadrant and found the worst defect in this change's documentation surface. docs/how-to/configure-model-profiles.md carried a section titled "If you want tiered models on Codex" telling users to set runtime:codex + model_profile:balanced, promising "GSD resolves each tier alias to the Codex-native model and reasoning effort defined in the runtime tier map." That is exactly the behavior this PR removes — a how-to page confidently instructing users to do something that no longer works, which is worse than a missing page because it fails at the moment of use. Rewritten to state that Codex does no tier routing, give the model_overrides pin as the supported alternative, and name the two constraints on what may be pinned: it must be a real Codex model id, and the account must actually expose it — GSD cannot verify the second, so the honest advice when unsure is to omit the pin. Carries an upgrade note for both account types, since the change is a no-op for ChatGPT accounts and a real loss for API-key ones. Also tightened the same page's claim that Codex "embeds the resolved model" at install time — now true only of an explicit override. The re-install instruction it supports is still correct and still needed, so only the premise moved. Both the required-docs set (COMMANDS.md + FEATURES.md) and lint-docs-required.cjs would have passed before this commit, since CONFIGURATION.md and the ADR had already moved. Neither checks the quadrant a user in trouble actually opens. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3241): backfill changeset pr number (#3276) --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2e2b8ba4a7 |
enhance(#2704): resolve documentation links and compare H1 status brackets in the ADR gate (#3266)
* test(#2704): failing-first coverage for ADR link resolution and H1 status brackets Binds the gate to two assertions it does not yet make: every relative markdown link under docs/adr/ must resolve, and an H1 trailing status bracket must agree with the Status: field instead of being silently stripped. Covers all 51 rows of the phase test matrix across two altitudes - the pure extractLinks/maskCode IR for fence and inline-code-span boundaries, hostile input and the fast-check totality properties, and the real CLI verdict for the end-to-end classes. Includes the DEFECT.GENERATIVE-FIX parity test that iterates the exported STATUSES array so a sixth status is covered the day it is added. * feat(#2704): resolve ADR documentation links and compare H1 status brackets The ADR gate validated naming, relation symmetry and index freshness but never resolved a link target, and it stripped an ADR's trailing H1 status bracket for display rather than comparing it against that ADR's own Status: field. Both classes were structurally invisible: #2691 found five dangling references by manual audit roughly a year after they were introduced, one of which reached the published npm payload, while CI reported green throughout. Both are now assertions on the same --check path, using only node:fs and node:path - no dependency and no subprocess. Fenced blocks and inline code spans are masked before scanning, because markdown does not render a link inside code. That is not a policy choice: the corpus contains exactly two such sequences today and both are ordinary JavaScript. Masking preserves length and column positions so findings still name a real line. Resolution is case-exact on every platform - a link that resolves only through macOS or Windows case-folding still 404s on github.com and still fails the Linux lane - and a destination resolving outside the repository is reported before any filesystem call is made. Also single-sources two duplicated surfaces this change would otherwise have extended: the H1 bracket vocabulary (a second hand-written copy of STATUSES with nothing asserting agreement, a DEFECT.GENERATIVE-FIX instance) and the docs/adr directory traversal. Two tests added by #2691 that reimplemented link resolution and bracket comparison inside the test file are removed for the same reason; the corpus assertion is now made by running the real gate against the real corpus. * fix(#2704): reject symlinks that leave the repository and linearize code masking Four defects from the isolated adversarial security review, plus one it noted. BLOCKER - a symlink defeated path containment. path.relative(ROOT, abs) is purely lexical, but the case-exact walk then calls readdirSync, which follows symlinks at the OS level: a contributor-committed docs/adr/x -> /etc together with a link through it passed containment and listed the real external directory, and a wrong-case probe echoed a real external filename through the "Did you mean" hint into publicly-readable fork-PR logs. Every segment is now lstat'd before descent; a symlink is realpathed and re-checked against realpath(ROOT) - realpath on both sides, so a root under /var does not produce false escapes - and an escape emits no hint and reads nothing further. The same rule now governs which FILES are read: an ADR entry that is a symlink out of the repository is excluded and reported rather than parsed, closing the vector this change had widened by newly reading README.md, naming-violation files, and full bodies rather than only header fields. MAJOR - inline-span masking rescanned the line remainder per backtick run, roughly O(n^1.6) on adversarial input: 1.76s for an 800KB line. Rewritten as a single linear pass pairing runs through forward-only per-length cursors. Same input now takes 3.31ms, with behavior unchanged. MINOR - an unreadable or broken entry threw, and the generic handler wrote a raw stack trace carrying absolute CI paths to stderr. The scan is now fault-tolerant and reports excluded entries as ordinary violations. The status vocabulary is escaped before being interpolated into a dynamic RegExp - defence-in-depth, not a live bug. The containment predicate had reached three hand-written copies while fixing this; it is now the single escapesRoot() helper used by all four call sites. * feat(#2704): add a --json report so the gate's tests assert on typed values Maintainer-directed addition. CONTRIBUTING.md's "Prohibited: Raw Text Matching on Test Outputs" requires that a system under test producing text also expose a structured intermediate representation, and that tests assert on that IR rather than on rendered prose. This gate had no such surface, so its verdict tests matched on stderr. --json runs exactly the same validation as --check and writes a report to stdout with the same exit code, following the frozen-REASON-enum pattern already used by verify-reapply-patches.cjs. Every violation carries a stable reason code plus the fields a consumer needs, so nothing has to pattern-match an error message. Adding a reason stays three coordinated changes - the enum, the emitting site, and the test locking Object.keys(REASON).sort(). The human output is unchanged, deliberately: a large pre-existing suite asserts on it and migrating that is not this PR's concern. Verified by running the pre-change and post-change scripts against an identical violating corpus and diffing their stderr - character-for-character identical. This PR's own verdict tests now assert on parsed --json. Absence checks improve the most: "no bracket violation" is now a reason-code predicate rather than a negative regex over prose, which could pass for the wrong reason. The security assertions were strengthened rather than translated - no leaked filename may appear in ANY field of the serialized report. Unknown flags are now rejected instead of silently falling through to printing the index. * test(#2704): fix the status-parity fixture and guard hooks/dist before overlay builds Two failures from the matrix run of 79b29909. The status-parity fixture was mine. It built, per status token, an ADR whose H1 bracket and Status field both carried that token - but Superseded carries an obligation beyond the bracket: it must name its successor as a file link and be symmetric with it. The fixture declared a bare Superseded, tripped that unrelated invariant, and the test reported a bracket-parity failure for a reason that had nothing to do with bracket parity. The fixture now satisfies each token's own obligations in both the agreeing and contradicting corpora, derived from the status actually declared rather than special-cased on one name, so a future token carrying obligations is handled rather than silently skipped. The second failure was not mine but is fixed here rather than deferred. mcp-catalog-parity.install.test.cjs hardlinks hooks/dist/* while building its overlay, but hooks/dist is a gitignored build artifact produced only by build:hooks. The suite had no guard, so it passed only when some other suite happened to build it first - an execution-order dependency, which is why it failed on node22 and passed on node24 for identical code. install.test.cjs already documents this exact hazard and guards it. Six behaviorally identical copies of that guard existed across three files. Rather than add a seventh, they are now one canonical tests/helpers/hooks-dist.cjs - idempotent and bounded by the shared BUILD_TIMEOUT_MS class norm - which is the same single-sourcing this PR applies to the ADR gate itself. * docs(#2704): add a how-to for contributors the ADR gate rejects The reference and explanation quadrants were covered by Lifecycle rules 5 and 6, but the task-oriented one was thin: a contributor meets this gate because it failed on their PR, under pressure, and the rules told them what is checked without telling them what to do about it. Adds the command to reproduce the CI failure locally and a message-to-remedy table covering every reason code that can be hit - unresolved target, wrong case with the did-you-mean hint, repository escape, symlinked ADR file, bracket contradiction - plus the backtick escape hatch for illustrative links and the caveat that indented code blocks are not skipped. The table is itself written in backticked inline code, so the gate skips it: the escape hatch demonstrated on the page that documents it. * chore(#2704): backfill changeset PR number pr:0 placeholder replaced with the real PR number now that #3266 exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2ac21c7fdb |
docs(#2980): ratify the payload-carried error idiom as a degraded result (#3270)
* docs(#2980): ratify the payload-carried error idiom as a degraded result Records ADR-2980: an `error` key in a gsd-tools result payload on stdout with exit 0 is a ratified contract meaning the command ran to completion and is reporting a condition, not a process failure. Faults keep stderr + exit 1 + --json-errors. Normalizing the 42 output({error}) sites to exit 1 was declined on measured blast radius (get_impact rates cmdStateSnapshot CRITICAL; output has 170 direct callers) — a Hyrum's Law break with no versioning escape hatch for an exit code. Adds the "Degraded results vs faults" section to docs/json-errors.md with a correct-caller recipe, indexes that page from docs/README.md, and records the two-channel contract in the CONTEXT.md I/O Module entry. No code change. Closes #2980 * docs(#2980): correct the site count and cross-refs after review The isolated review found the population figure was the answer to a regex, not to the question. `output\(\{\s*error:` only matches literals whose FIRST key is `error`; re-deriving it by brace-matching output()'s first argument gives 60 sites across 9 modules (42 error-first + 18 error-not-first), adding workstream.cts, phase.cts and gsd2-import.cts. roadmap.cts:260 is the case in point — it carries `error` alongside `found:false` and is the site that actually produces the documented `roadmap get-phase` output. Also from review: correct the --raw claim (11 sites pass a rawValue, not 2), reconcile the missing-required-argument count to the 7 verified sites, link the bare ADR-2966 references per the ADR lifecycle rule, and fix 'licence' to American spelling. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c07297cd50 |
docs(#2869): record ADR-2866 install-surface resolution (#3265)
Phase 0 of epic #2866. Records the decision that the install pipeline resolves surface identity — (runtime × scope × trigger) — as a value instead of implying it from destination paths. The ADR makes four things explicit: - Amends ADR-3660 (placement -> placement + trigger resolution) and says why placement-only stopped paying: the /gsd-<name> trigger two artifacts collide on is not a value anywhere, so #2218 cannot be stated by any module or test. - Adds one axis to ADR-1016's deliberately-closed descriptor vocabulary (host trigger precedence), required-with-default so ADR-894's additive-only stability contract holds. - Records the @-include constraint (expands ~, does NOT expand env vars, no conditional syntax) as the reason #2218 triage option 1 is REFUTED rather than merely deprioritized. - Notes the non-conflicts: completes ADR-58 rather than revising it, and preserves ADR-1508's dependency direction. ADR-3660 and ADR-1016 each gain the reciprocal Amended by back-link, matching the corpus convention ADR-2782 already set on ADR-1016. Each states that the decision is recorded now while the modules change at Phase 2 (#2871), so no reader is told a widening has already shipped. Also corrects CONTEXT.md's Installer Module entry: bin/install.js is hand-authored, not generated. ADR-1508 states this verbatim and no build step emits it; the stale annotation invites contributors to look for a generator that does not exist. Docs-only. Verified via the remote runner. Closes #2869 Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b183317abd |
docs(#3256): ratify ADR-2363 to Accepted (#3260)
Both phases of epic #2363 have landed and the epic is closed as completed, so ADR-2363's own stated ratification bar is met: #3248 merged, the consent summary renders instruction surfaces, and a passing test pins the D4 signature behavior. Adds the dated Ratification section the corpus requires, naming the files, symbols and tests for each of D1-D5, and restores the reciprocal back-link on ADR-1244 - owed only on ratification, which is why the premature flip in 4d26887e correctly withdrew it. Records the judgment call the ratification rests on rather than burying it: D3 classifies instruction surfaces as skills and agents, and the mechanism discloses skills only. Third-party agents are never staged into the instruction context, so disclosing them would have named a surface that does not exist. Everything actually staged is disclosed, which is what the decision requires; whether agents should be staged is recorded in D5 as an open maintainer question. Docs-only. No behavior change. Closes #3256 Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9f57fa43ed |
docs(#3240): record the codex passive/session-only model posture (#3251)
* docs(#3240): record the codex passive/session-only model posture ADR-2313 locks the install-time contract for epic #2313: omit the per-agent model from generated ~/.codex/agents/<agent>.toml by default so the agent inherits the always-available Codex session model, embed one only for an explicit real-Codex model_overrides pin, and keep model_reasoning_effort coupled to a pinned model (#838). Supersedes #2517's per-tier embedding on the default path only. Also records the reader/writer boundary the downstream phases need (strict writer, liberal-but-visible readers, never partially rewrite an unparseable .toml), the migration path for API-key Codex users, and the Phase 5 the coverage gate found unowned. Amends ADR-1239 with a dated section: its effortSurface amendment described this ADR as "not yet written", and the install-time vs invocation-time boundary is now stated from both sides. Docs-only. The posture is not real until Phase 1 (#3241) merges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3240): remove the ADR index count cells that race between PRs The generated region of docs/adr/README.md carried three numeric cells — a per-group `### <heading> (N)` and a `_N ADRs._` footer — that every ADR-adding PR must rewrite. Two PRs adding different ADRs merge their table rows cleanly, since those are distinct lines, but both rewrite the same count lines, so whichever lands second gets a green local `gen-adr-index.cjs --check` and a red CI one: CI evaluates the PR merged with next, where the count reflects both ADRs. That is not hypothetical. It reddened this PR: ADR-2313 regenerated the index at 75 while #3249 landed ADR-3247 concurrently, making the merged tree 76. The counts carry no verification value — --check regenerates and diffs the whole region regardless — and are derivable by reading the table, so they are removed rather than tolerated. Loosening --check to ignore them would have let genuine staleness through. This is the shared-mutable-cell problem CHANGELOG.md and the drift acks already solved with per-PR fragment files; here removing the cell is enough. The regression test locks the invariant rather than the symptom: adding an ADR only INSERTS lines, so render(N) is a line-subsequence of render(N+1). That is the property that makes concurrent PRs merge, and unlike asserting the absence of one count format it fails for a count reintroduced in any shape. Covered at append, lowest-id, middle-id, empty-corpus, new-status-group, and hazardous-title positions; each names the pre-fix line that would have failed it. 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
|
f96cb44f85 |
enhance(#3248): disclose capability skills as an instruction surface (#3253)
* test(#3248): failing-first suite for instruction-surface disclosure 28 matrix rows from 50-test-matrix.md. Rows requiring the new Disclosure.instructionSurfaces field fail today; rows 18-20/23-25 (the ADR-2363 D4 signature invariants) pass today by construction because the current code never reads skills/agents at all, and stand as regression guards for the implementation commit. Refs #3248 * feat(#3248): disclose capability skills and agents as an instruction surface ADR-2363 D5. A capability whose only contribution was skills disclosed nothing at install: summarizeDisclosure early-returned "ships no executable surfaces (declarative only)" because hasExecutable was false, while each SKILL.md body landed verbatim in the agent's instruction context. discloseExecutableSurfaces gains a fifth, NON-executable class, instructionSurfaces, collecting declared skills/agents stems through the same safeCollect wrapper as the four existing collectors, so a hostile value degrades only this class and the function stays total for any manifest shape. Nothing existing is edited: the collectors, hasExecutable, disclosureSignature and missingArtifacts are untouched. get_impact rates the symbol CRITICAL at 196 affected, which is why the design is strictly additive. D4 is implemented by omission and pinned rather than left incidental: adding, changing or removing skills/agents leaves disclosureSignature byte-identical, so no stored consent record is perturbed and no spurious re-consent fires. ADR-2782's conditional-append trick is deliberately NOT reused - it worked because no manifest could declare a reviewer body before that class existed, whereas skills predate this one, so a conditional append would re-sign every already-consented skill-bearing capability. The renderer is extracted as summarizeInstructionSurfaces and called from BOTH branches of summarizeDisclosure. Appending only at the end would never render for skill-only capabilities - the ones that need it - since those take the early return. That branch's "declarative only" claim is now conditional on there being no instruction surface either. The renderer iterates rather than spreading into push, so an unbounded stem count cannot throw RangeError, and tolerates the bare {} the CLI edge passes via `res.disclosure || {}`. Scope note: #3248's prose says "skill stems"; ADR-2363 D3 classifies instruction surfaces as "skills, agents". Shipping skills alone would leave an ADR deliverable owned by no phase, and the epic has no Phase 2. Agents are the same shape at no extra cost. Narrowing back is a two-line change. Ratifies ADR-2363 (Proposed -> Accepted) and adds the owed ADR-1244 back-link. Closes #3248 * fix(#3248): escape consent-prompt values and narrow disclosure to skills Two review findings, both of which made the previous commit wrong. BLOCKER (isolated adversarial review). Every manifest-supplied value interpolated into a consent-prompt line was rendered unescaped. Those lines are joined with \n and written RAW to stderr on the needs-consent path (capability-command-router -> cli-exit runMain), so a stem carrying a newline forged additional lines indistinguishable from genuine GSD disclosure text, and an ANSI escape could clear or rewrite lines already printed. That defeats the informed-consent guarantee this change exists to provide, and is a prompt-injection vector against any agent that reads the stderr text to decide whether to retry with --yes. The hole was not unique to the new class - hook event/script, command family/module/router, every MCP field, and every reviewer-lane field were equally unescaped. Fixing only the new one would have created the generative-fix divergence this repo tracks, so renderValueForPrompt is applied to all five classes through one helper, guarded by a parity test that fails if a future class skips it. Escaping is identity for ordinary names, so no well-formed manifest's output changes. The disclosure OBJECT stays verbatim - only the rendered LINE is escaped - because the signature and every consumer reasoning about identity depend on the declared value. NARROWED to skills only. The previous commit also collected agents, arguing ADR-2363 D3 classifies instruction surfaces as "skills, agents". Verified against staging: stageSkillsForRuntimeAsSkills takes a registry and unions third-party skills in via readInstalledCapabilitySkill, while stageAgentsForRuntimeWithConverter takes only a source directory and has no registry-aware path. Third-party agents are never staged into the instruction context, so disclosing them would have put a false claim in a security prompt - worse than the scope creep two reviewers flagged it as. D3's classification stands; D5 now records that Phase 1 implements the skills half and that whether agents should be staged at all is an open maintainer question. Also reverts the premature ADR-2363 ratification. The previous commit flipped it to Accepted and asserted "#3248 merged" while this branch IS #3248 and is unmerged. Status returns to Proposed, and the ADR-1244 back-link - owed only on ratification - is withdrawn. Adds the fast-check property suite CLAUDE.md requires and the direct precedent (reviewer-trust-disclosure) already had: totality, D4 signature invariance, D3 hasExecutable invariance, and renderer totality over adversarial manifests. Refs #3248 * chore(#3248): correct changeset scope claim and backfill pr number The fragment was written against the pre-narrowing commit and still advertised 'skills and agents'. 4d26887e narrowed disclosure to skills only - third-party agents are never staged into the instruction context - but did not touch the fragment, so the release notes would have carried a claim the code does not implement. Also backfills pr:0 -> 3253 and names the prompt-escaping fix, which is user-visible and was absent from the original body. Changeset-only; no code or test changed, so the gsd-test pass recorded for 4d26887e still describes this tree's behavior. Refs #3248 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bc5619dd27 |
docs(#3247): record the capability instruction-surface trust model (#3249)
* docs(#3247): record the capability instruction-surface trust model ADR-2363 records the trust posture for third-party capability SKILL.md bodies, which #2322/#2340 made agent-invocable without any content-level control. The path-level protections that fix shipped are all present; no content scanner exists, and external-descriptor-trust.cts never had one. Nothing was bypassed - the control did not exist and the boundary was never written down. D1 records the posture: skill bodies are trusted, unscanned agent instructions. D2 rejects content scanning on Kerckhoffs (a shipped rule set is readable by the adversary who installs it), on threat-model non-transfer from ADR-1577 (there, instructions are anomalous inside data; here they are the payload's legitimate form), and on Goodhart (a scanned-OK line displaces the judgment the consent prompt exists to provoke). D3 replaces the executable/non-executable binary with three classes, adding instruction surface. D4 keeps instruction surfaces out of the v1 disclosureSignature. The signature is NOT the activation binding - hasProjectConsent compares contentHash only, and a global install carries no consent record at all. What re-encoding would do is perturb the signature of every skill-bearing capability and fire a spurious re-consent prompt on its next upgrade, which is what ADR-2782 D4 rule 5 already forbids. If instruction surfaces ever need to be signature-bound, that lands as a versioned v2 signature with a migration, never an in-place re-encoding. Corrects capability-trust-model.md, which claimed skills get lighter consent because they do not execute code - true, and not the relevant property, since the agent is the interpreter. Adds the author-side boundary to develop-a-capability.md and links it from publish-a-capability.md. Both state that per-skill disclosure at the consent prompt lands with #3248 and does not happen today. Docs-only. No behavior change; no consent record perturbed. D5's mechanism is Phase 1 (#3248), which is why the ADR is Proposed. Refs #2363 * chore(#3247): backfill changeset pr number to 3249 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2a73f53cb3 |
fix(#3204): milestone sectioning is vocabulary, not heading position (#3230)
* test(#3204): failing-first suite for the clobbered phase count A project declaring six phases with four phase directories on disk had state.record-session write progress.total_phases: 4 — #2828 regressing at 1.9.1, reported in #3204 with a deterministic reproduction. Before the fix in the following commit, these rows FAILED (wrote 4, expected 6): a flat roadmap carrying `## Progress`; one carrying `## Overview` and `## Phase Details`; the CRLF variant of the first. Two more, found by adversarial review and added after the first fix attempt, failed against that attempt: structural headings interleaved among flat phase headings, and this repo's own bundled-template shape (a `## Phases` wrapper around a single nested milestone). The #1761 control — sibling milestone sections must keep falling back to the disk count — passes both before and after, so the fix has something it must not break. Assertions read progress.total_phases through the product's own frontmatter parser via `state json --raw`, never a regex over STATE.md. Rows 12 and 13 are hostile: a phase heading carrying a version token, and a version heading inside a fenced code block; neither may count as milestone sectioning. Refs #3185, #3204 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3204): milestone sectioning is vocabulary, not heading position buildStateFrontmatter chooses total_phases between the ROADMAP's declared phase count and the on-disk directory count, and refuses the roadmap count when hasMilestoneSectioning says the document is milestone-sectioned — because a whole-document count would then conflate sibling milestones (#1761). That predicate returned true for ANY non-Phase level-2/3 heading, so a flat roadmap carrying an ordinary `## Progress` was called sectioned and the disk count clobbered the declared one: six declared phases, four directories, total_phases written as 4, converging on the truth only once the last directory happened to exist. That is #2828 regressing at 1.9.1, and it came from this epic — #3184 replaced state.cts's hand-rolled #2828 guard with this predicate, and the replacement is strictly more permissive than the guard it retired. Three position-based models were tried and all failed, because position does not carry milestone-ness: - any non-Phase heading (shipped) — over-detects, giving #3204; - strict nesting/ownership — misses same-level siblings, regressing #1761, and false-positives on the bundled template, where `## Phases` wraps a single `### v1.1`; - adjacency — reproduced live: `## Overview` and `## Notes` interleaved among six phase headings are two owning candidates, so a 6-phase roadmap with 2 directories wrote 2. A heading is now a milestone heading iff it is a non-Phase heading carrying a milestone signal: a version token, a status marker, or the word Milestone. Sectioning means two or more, since one cannot conflate siblings. Known limit, recorded in the doc comment rather than hidden: two milestone sections carrying none of those three signals are not detected. Also drops buildStateFrontmatter's local dedup-key regex, flagged in-source as diverging from the canonical token rule, for phaseKeyFromDir — the remainder of #3185, since #3222 had already routed the enumeration itself through listMilestonePhaseDirs. #1514, #2445 and #3017 are preserved untouched. Closes #3185 Fixes #3204 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3185): changeset, glossary entry and ADR status for the phase-count fix CONTEXT.md's Roadmap Parser Module entry never named hasMilestoneSectioning, so the predicate whose semantics this change reverses had no glossary presence at all — a PR gate for a module/seam change. Added, covering the vocabulary model, the three position-based models that failed, and the residual limit. ADR-3180 recorded the fifth enumeration copy as unowned in four places. It is owned now. Amendment 4's scope table row 1 also carried an error worth keeping visible rather than rewriting: it claimed Phase 3 merged without routing the state writers, when #3222 had in fact routed the enumeration — the audit read Amendment 3's silence about the symbol names as absence of the work. The real gap was the trust discriminator one layer above, which is what #3204 was. Changeset is Fixed and leads with the symptom a user sees — a phase count that shrinks to match how many phase directories happen to exist yet — and carries the known limit forward rather than leaving it in a source comment. Refs #3185, #3204 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3185): stop quoting the retired phase-token regex in a comment The remote runner failed tests/phase-id-drift-guard.test.cjs: the comment explaining that the local dedup regex had been replaced by phaseKeyFromDir quoted that regex verbatim, and scripts/lint-phase-id-drift.cjs scans for the literal token without caring whether it sits in code or in prose. That is the guard being right, not over-eager — a quoted pattern is one paste away from being live again, which is exactly how the copy it replaced spread. Described in prose instead. Worth recording: this guard is check:phase-id-drift, which lint:ci does not run — it is enforced by tests/phase-id-drift-guard.test.cjs. A green lint:ci is therefore not evidence the drift guards pass. Refs #3185 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3185): backfill changeset PR number (#3230) 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
|
86bebcefa2 |
refactor(#3216): bind milestone identity to the canonical locator (#3226)
* refactor(#3216): widen milestone-window guard to literal-## matchers The guard keyed only on the `#{N,M}` quantifier plus a literal version or phase-lookahead token. getMilestoneInfo hand-rolls its milestone-heading match with a literal `^##`/`## ` and an interpolated ${escapedVer}, so it satisfied neither token and the guard reported a clean zero on a file carrying live re-derivations (#3171, #3197) — a zero it did not earn. Widen token (a) to a literal 2-6 `#` run, admitted ONLY inside a heading-MATCHER literal (a regex literal, or a string/template handed to new RegExp) so a heading-BUILDING template is not mistaken for a re-derivation. Widen token (b) with the grouped `v(\d+(?:\.\d+)+)` shape and an interpolated version placeholder. Ships BEFORE the consolidation per ADR-3180 s7.2: a guard widened afterwards measures an already-cleaned surface. It is expected to be RED until the consolidation lands. * test(#3216): failing-first milestone-identity single-owner suite 63 tests across two files, from the matrix in .gsd/phase/. Section H of milestone-window-single-owner.test.cjs covers the 21 input classes of the design's behavior table plus its negative space; milestone-window-drift-guard covers the widened tokens and proves the exemption is function-scoped, not file-scoped. Copy count is 3 found by the guard, not 1 per the epic (ADR-3180 Amendment 3's standing rule, holding for the fourth consecutive phase): both getMilestoneInfo sites plus cmdRoadmapAnalyze's milestone enumeration at roadmap.cts:454, which carries the same #3171 truncation and #3197 phase-heading confusion. Expected RED until the consolidation lands. * refactor(#3216): bind milestone identity to the canonical locator getMilestoneInfo hand-rolled two milestone-heading regexes inside the owner's own file. Both were wrong, differently: the STATE-version site's ^## anchor is level-blind so [^\n]* absorbs a third #, and the fallback site had no anchor at all, so '## ' matched from the second # of '###'. Against '### Phase 7: Close v3.3 gaps' the fallback returned {v3.3, gaps} (#3197). Both captured names with [^\n(], truncating at a parenthetical (#3171). Bind both to the canonical grammar. locateMilestoneHeadings becomes a version-filtered view over one shared source, and a new version-agnostic listMilestoneHeadings enumerates milestone headings for callers that need all of them. getMilestoneInfo returns ScopedResult<MilestoneInfo|null>; the {v1.0,'milestone'} default, which was output-identical to a real v1.0 project, is deleted. The #2245 never-throws invariant is preserved. Copy count: 3 found by the guard, not 1 per the epic. The third was cmdRoadmapAnalyze's own milestone enumeration (roadmap.cts:454), carrying both defects in the implementation the epic blessed. buildStateFrontmatter and archivePhaseDirectories branch on scope: the first writes null rather than a fabricated identity, the second falls through to its dated-label fallback. A fabricated v3.3 passes ARCHIVE_VERSION_LABEL_RE, so it would otherwise misfile phase history. Also fixes an unsafe cast in init.cts that masked these type errors across five call sites, which would have shipped undefined milestone fields under green tsc. * fix(#3216): restore the #1761 unbounded guard and bullet precedence Review and the first full-matrix run surfaced five real defects in the consolidation, all fixed here rather than by relaxing the tests that caught them: - buildStateFrontmatter gated its isMilestoneBoundedInRoadmap check on the scope-gated milestone value, which is null on any non-COMPLETE scope, so the #1761 unbounded guard was silently skipped and state json reported a percent it must omit. It now gates on the STATE-asserted version, independent of identity scope. - The rewrite lost #2135's precedence: the name-bearing progress-marker bullet is consulted before the heading again. - A single-segment version (v3, no dot) did not resolve; the name-extraction fallback now accepts it. - A version carrying regex metacharacters, or a $& / $1 replacement pattern, is matched literally. - listMilestoneHeadings' heading field trimmed, so a CRLF roadmap no longer leaks a trailing carriage return into roadmap analyze's output. Also emits milestone_version / milestone_name / current_milestone as explicit null rather than omitting the key, so the prompt layer cannot render a bare placeholder, and corrects an init.cts comment plus a cast left inconsistent. * test(#3216): update milestone-identity expectations to the scoped contract getMilestoneInfo returns ScopedResult<MilestoneInfo|null> and the {v1.0,'milestone'} default is deleted, so the suites asserting the old shape assert removed behavior. Updated rather than weakened: every touched call site now asserts the scope explicitly against the frozen SCOPE enum. roadmap-parser.test.cjs: 20 expectations moved to {value,scope}. The #1881 unreadable-vs-absent diagnostic assertions are untouched and still prove their original point — only the return shape moved. One pre-existing assert.ok(info) is now a specific UNSCOPED assertion, so that case is stronger than before. new-milestone-clear-phases.test.cjs: the test asserting phases clear archives under the v1.0 default now asserts the dated archived-<YYYYMMDD> fallback, which is the deliberate consequence of deleting that default. Two of this branch's own tests were also corrected after they drove the implementation the wrong way: the parity test compared raw heading text and so pushed a stray ## prefix into roadmap analyze's public output, and the hostile metacharacter row demanded a pathological version resolve, which pushed a widening of the ADR-locked \b boundary. Both now assert what the contract actually requires. * docs(#3216): document milestone identity and correct the CONTEXT.md entry ADR-3180 s7.2 moves to Enforced and gains two rules that were unstated: the name derives from the heading's own version token and drops a trailing status marker, and a free-form legacy ROADMAP with no version anywhere is UNSCOPED with no identity rather than a defaulted v1.0 (decided by the maintainer before implementation, per s7's own rule that an unstated behavior is not decided). Amendment 4 records Phase 6's validation, including that the copy count was a lower bound for the fourth consecutive phase. CONTEXT.md's Roadmap Parser entry described locateMilestoneHeadings as boundary-matched with (?![\w.-]) — the alternative Amendment 2 tried and REVERTED. The code uses \b and says so, and the ADR agrees; the revert updated code and ADR and missed CONTEXT.md, which is the epic's own fixed-on-one-copy failure class in the docs layer, on a file that is itself a PR gate. * fix(#3216): persist the real version on a truncated identity buildStateFrontmatter wrote null for BOTH milestone and milestone_name on any non-COMPLETE scope, discarding a real version. ADR-3180 s7.2 rule 6: a version known with no resolvable name is TRUNCATED carrying {version, name: null} — 'the version is a real answer, the name is a non-answer, and collapsing the two is the failure this contract exists to prevent.' The two fields are now gated by what is actually known: the version whenever one exists (COMPLETE or TRUNCATED), the name only on COMPLETE. Never fabricated. Caught by this phase's own Decision 4(c) consumer-output test, which is the argument for asserting at the consumer rather than the owner — the owner was correct throughout; only the consumer collapsed its answer. * refactor(#3216): extract helpers and make cmdCommit's scope gate explicit From the two-axis code review: - init.cts repeated the identical getMilestoneInfo cast at five sites with copy-pasted comments — duplication inside a PR whose thesis is that duplicates get deleted. Extracted milestoneRecord(cwd); the one site-specific comment is kept, the four generic copies removed. - getMilestoneInfo hand-built its { value, scope } literal at ten return points; a local scoped() constructor now does it once. Every per-branch rationale comment is preserved and no returned value or scope changed. - cmdCommit gated the milestone branch name on plain truthiness, which is also true for TRUNCATED, so an unresolved identity drove branch creation incidentally rather than deliberately. It now gates on the SCOPE enum, accepting COMPLETE or TRUNCATED because both carry a real version, and the comment records why that differs from archivePhaseDirectories — which demands COMPLETE because it uses the value as a filesystem path component. * test(#3216): cover the bare-version-in-prose truncated path The spec review found the bareVersionMatch path — no STATE version, no milestone heading, a version token only in prose — returning TRUNCATED with no test exercising that exact shape, violating Decision 4's boundary-coverage requirement. * docs(#3216): record the missed Tier-2 surfaces and rule 5's corollary Decision 3 requires an explicit call-out for EVERY Tier-2 change, and Amendment 4's first draft named eight surfaces while the change touched thirteen. Adds cmdCommit's branch-name construction and the four init JSON bundles, an incomplete list being the same defect in miniature that this epic removes. s7.2 rule 5 gains a corollary separating two cases the original wording ran together: no version token ANYWHERE is UNSCOPED, while a bare version token in prose or a non-milestone heading is weak but real evidence and yields TRUNCATED under rule 6. * chore(#3216): set changeset fragment pr to 3226 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b9f51836e6 |
refactor(#3180): ADR-3180 behavior contract + cross-surface drift guardrails (#3223)
* refactor(#3180): one owner for completion ratio, a prompt-layer drift guard, and a written behavior contract The 2026-08-08 coverage audit on #3180 found the epic's copy counts were a lower bound for the third consecutive time, and that two derivation families had never been named at all. ADR-3180 gains Decision 7 — a normative behavior contract that says what the right answer IS for each derivation, not merely who owns it. A reviewer with no written rule can only ask "does this look like the others", which is how a fifth copy passes review. Decision 4 gains (d) scan surface is every authored surface and an owner FILE is never exempt, only its named functions; and (e) a surface that cannot be consolidated today ships ratcheted, never unguarded. Completion ratio: `clampPercent` sat exported and unused beside six hand-inlined copies of its own body across five modules. All six now route through it; `clampPercentFromFraction` is added for the one caller that already held a fraction. Every migration is behaviour-identical — clampPercent's first line IS the `total > 0 ? … : 0` ternary each copy carried. Guarded by lint-completion-ratio-drift.cjs, which reports zero re-derivations with no file-level exemption. Prompt layer: workflow markdown re-derives live-plan counting in raw shell (#1762), invisible to every `src/`-scoped guard. lint-planning-prompt-drift.cjs scans it with a shrink-only baseline of the 7 sites that exist today — new sites fail, and a baseline entry that stops firing fails too, so an acknowledgment can never outlive the thing it describes. lint-milestone-window-drift.cjs stops exempting its owner file wholesale; only the four named canonical functions are exempt now. The blanket exemption was pointed at the one file most likely to grow the next copy, and it had. Refs #3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3180): link Phases 6-8 sub-issues (#3216, #3217, #3218) from ADR-3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3180): address orthogonal review — consumer-output identity tests, count-keyed ratchet, property coverage Five findings from the two orthogonal review passes, all fixed. Decision 4(c) breach: the completion-ratio identity test asserted at the OWNER, which is exactly the bypass that decision exists to close — a consumer can call clampPercent and then post-process locally, leaving both the lint and an owner-level test green. It now drives `roadmap analyze`, `query progress` and `stats` and asserts on their own output, over a fixture containing a `status: superseded` plan so a consumer that re-counted raw files would report 60 where the owner reports 75. Decision 4(e) breach: ratchet entries named the epic (#3180) rather than the issue that removes them. They name Phase 8 (#3218) now. The ratchet keyed on (file, text) alone, so plan-phase.md's two byte-identical sites were one indistinguishable key and migrating either would have left the guard green with the other alive. Entries carry an occurrence count; fewer than acknowledged fails as a partial migration, more fails as a new copy. Adds the missing MAX_REGEX_LITERAL_LEN boundary coverage the sibling guard's test already had, and the fast-check property tests CONTRIBUTING requires for clamp/budget-limit functions. Refs #3180 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: stop wrapping a nested double-spawn in a 15s wall-clock budget (bug #641 probes) `tests/ci-test-scope.test.cjs`'s `bug #641` block spawned `run-tests.cjs` under PROBE_TIMEOUT_MS=15000; that child then spawned a nested `node --test`. A fixed wall-clock budget around a double spawn, running inside a container that is concurrently executing the full ~31k-test suite, fails by construction under load. Confirmed against three full matrix runs. Every failure was shaped `null !== 0` — the child was KILLED, never an assertion about the thing under test. One captured probe had already printed the correct resolution (`suite="all" files=2: a.test.cjs b.test.cjs`) and was killed anyway. It reproduces on `next` alone: 5 failures on linux-node22, 0 on linux-node24. The victim subset varies by run and by lane. What these tests are actually about is suite-token RESOLUTION — `unit` as a bare token in --files/--files-from. Executing the seeded trivial files is incidental and is the entire timeout surface, so the assertions move in-process against the same functions `main()` calls, in the same order. `parseArgs`, `selectExplicitFiles`, `selectFiles` and `walkTestFiles` are exported for that; no behavior, signature or logic changed. No coverage lost: `tests/run-tests-harness.test.cjs` already spawns the harness for real and asserts exit codes end to end, on a 120s budget. Pre-existing on `next`, fixed here rather than deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test: delete the three elapsed-time assertions CLAUDE.md forbids asserting on wall-clock time. Three assertions did, and all three are load-sensitive: on a saturated bench each can fail while the code under test is correct. In every case the load-bearing assertion sits on the line above and the timing line adds no discrimination. run-with-timeout: the stated worry — "was this 124 the cap firing or the 30s harness backstop?" — is already answered by the assertion above it. A backstop kills by signal, which surfaces as status null, never 124. Observed directly this session: three matrix runs produced exactly that null shape from killed children. normalize-test-command and context-predicates: both bounded a ReDoS check. A threshold only ever separates "fast" from "slightly slow", which is bench load, not correctness — catastrophic backtracking on 800 KB of input does not take 251ms, it does not finish at all. A real regression therefore shows up as the suite being killed on that test, which is louder and more reliable than a number. The structural assertions (returned unchanged; cleanly rejected) are what actually carry those tests, and they stay. The sweep now reports zero elapsed-time assertions in tests/. The remaining Date.now() uses are unique-path suffixes, barrier deadlines, fixture timestamps and fake mtimes — none of them assertions. Pre-existing on `next`, fixed here rather than deferred. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3180): backfill changeset PR number (#3223) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3180): key the prompt-drift ratchet on POSIX paths so it works on Windows The baseline keys on (file, trimmed text). `file` came from scanTree's `path.relative()`, which uses NATIVE separators, while the committed baseline stores POSIX. On Windows every violation was therefore unmatched — reported as FRESH — and every baseline entry matched nothing — reported as STALE. The guard failed 100% of the time there, on both CI shards: ✖ scanRepo(repoRoot) matches the baseline exactly: zero fresh AND zero stale + { file: 'gsd-core\\workflows\\execute-plan.md', ... } The remote runner this repo gates on is Linux-only and cannot see this class at all; the GitHub Actions Windows lane is what caught it. Normalization is unconditional — never gated on process.platform. A platform-conditional normalizer makes the POSIX path the special case and leaves the Windows branch unexercised on every other OS, which is the same blind spot in a different place. It is applied at one seam inside findPromptDrift, which builds `file` on every returned violation, so the baseline key, the --update writer, the stderr report and the tests all consume one normalized value. The regression tests drive a Windows-shaped relPath directly and run on every OS rather than skipping off-Windows — a test that only runs on the platform where the bug lives is why this escaped. They include a sanity check that un-normalized input does NOT match, so the assertion cannot pass vacuously. Audited the three sibling guards: none keys against a committed cross-platform baseline, and their exemption keys are path.join-built, so producer and consumer share the native convention. Left correct code alone rather than making them look alike. scripts/lib/drift-scan.cjs is untouched — normalizing there would break those three on Windows. 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
|
636ec92107 |
refactor(#3185): phase enumeration has one owner and a decidable scope (#3222)
* test(#3185): failing-first phase-enumeration single-owner suite Covers the enumeration rows with direct code evidence: 999.* backlog dirs listed by progress/stats, the phase-0 sentinel divergence, the #1324 letter-prefixed-decimal negative space, and the destructive-path find — cmdPhasesClear carries a fifth sentinel copy (/^999(?:\.|$)/) that excludes 999 but not 0, so a 0-* directory roadmap.analyze preserves is deleted there. Also covers the pass-all degrade, which is where the defect actually lives: when the milestone window declares no phases the filter becomes a literal () => true and its heading-side sentinel exclusion is unreachable. A fixture carrying phase headings keeps the filter active and never reaches that path. Named for the derivation, not a module: the suite drives commands, phase, milestone, workstream-inventory and state, and both the phase and phase-locator buckets are already at the per-module test-file cap. Committed alone so the remote runner records the failure before the fix. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * refactor(#3185): phase enumeration has one owner and a decidable scope Adds phase-locator.cts::listMilestonePhaseDirs as the single canonical owner of "which phase directories belong to the current milestone". It applies the milestone window AND the sentinel filter and returns a ScopedResult, so a caller can tell a genuinely-empty milestone from an enumeration that could not be scoped. The sentinel test now runs against DIRECTORY NAMES and is unconditional. getMilestonePhaseFilter excludes sentinels from its ROADMAP heading set, but degrades to a literal () => true pass-all predicate when that set is empty -- at which point the heading set is never consulted and its sentinel exclusion is unreachable exactly when it is needed. That degrade is the #3167 path, and it is why stats already used the filter and still listed backlog directories. The narrowing is sentinel-only: pass-all stays over-inclusive otherwise. Sentinel copies deleted, canonical isSentinelPhaseId adopted: - cmdRoadmapAnalyze's local closure (parseInt === 0 || === 999), 2 call sites - cmdPhasesClear's /^999(?:\.|$)/ -- the DESTRUCTIVE path, which excluded 999 but not 0, so a 0-* directory roadmap.analyze preserves was deleted cmdStats also seeded rows from ROADMAP headings with no sentinel filter, so a 999 heading produced a row with no directory; that seed is filtered now. cmdPhasesList routes only its ENUMERATION. --phase lookup searches the physical set (scoping it would report an out-of-window phase as not found) and --include-archived still merges archived dirs (they are by definition from other milestones). Both exempt by documented reason, never a file allowlist. Fixed inline, found while building: isDirInMilestone could not match a #1324 letter-prefixed-decimal directory (P0.0-foundation) to its own Phase P0.0 heading, so stats reported the phase with plans: 0 while its directory held plan files. Defers to phase-id's extractPhaseToken rather than widening a fourth bespoke regex; additive, so it can only admit directories. Refs #3180. Closes #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * refactor(#3185): route the last two enumeration re-derivations workstream-inventory countRoadmapPhases counted every `Phase` heading across the whole ROADMAP -- no window, no sentinel filter -- so it counted 999.* backlog and Phase 0 and spanned every milestone the document ever had. Its own caller already resolved a currentVersion and passed it to getMilestonePhaseFilter elsewhere in the same file; this was the sibling copy that never got the fix. state.cts phaseInventoryProvider enumerated phase dirs with its own /^(\d+)-(.+)$/ convention regex and neither filter, so a rebuilt STATE.md inventory carried backlog and sentinel directories as current-milestone phases. A non-COMPLETE enumeration scope now throws to the outer catch as a real scan failure rather than reporting a confident undercount, mirroring the per-phase scanPhasePlans contract beside it. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * refactor(#3185): consolidate 23 sentinel re-derivations onto one predicate The whole-repo drift guard (ADR-3180 Decision 4a, no file allowlist) found the sentinel rule re-implemented 23 times across 8 modules, in three regex variants plus four integer-comparison forms. Most tested 999 only, so Phase 0 slipped through them while roadmap.analyze and the engine-wide convention (#1580) both treat 0 and 999 alike. That disagreement is the defect class this epic removes. All 23 now call phase-id's isSentinelPhaseId (SENTINEL_RANGES [0,999]). Sites: init recommended-actions and backlog counts, milestone phase scan, the phase-lifecycle progress table, phase.cts used-number collection and the four renumber-on-remove guards, roadmap-parser's heading and bullet milestone counts, roadmap get-phase fallbacks, and state's heading denominator. Excluding Phase 0 at these sites is a deliberate behavior change and the point of the consolidation — several carried comments already saying 0 should be excluded while the literal beside them caught only 999. Adds scripts/lint-phase-enumeration-drift.cjs, wired into lint:ci. It scans the whole src/ tree with no file allowlist and reports both shapes: an independent phases-dir enumeration, and an independent sentinel literal. Exemptions are function-scoped with a written reason. The guard is comment-aware — its first pass flagged JSDoc and a comment documenting that the code below uses the canonical owner, which would have trained readers to exempt prose. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * refactor(#3185): resolve every phases-dir enumeration; drift guard reports zero Per-site triage of the 31 remaining whole-repo guard hits, applying the rule generalized from #3183's Amendment 1: a LOOKUP, DIAGNOSTIC, ARCHIVAL or MUTATION pass wants the physical set; only "which phases belong to this milestone" wants the scoped set. Routed (10): init new-milestone phase_dir_count, init milestone-op fallback count, init manager, init progress, milestone complete stats/dry-run/archive move, phase complete's next-phase scan, state update-progress, state frontmatter stats, and uat audit's active set. Exempt with a written function-scoped reason (never a file allowlist): the audit/UAT/verification sweeps that deliberately scan every directory to report gaps, phase create/insert/rename/renumber mutations, single-phase lookups, roadmap-upgrade's cross-milestone migration, cmdPhasesClear's whole-tree destructive pass, and the reads that list a phase dir's FILES rather than enumerating the phases dir at all. Latent defects fixed by the routing: sentinel directories leaked into cmdInitNewMilestone's phase_dir_count, cmdMilestoneComplete's stats, dry-run AND ARCHIVE MOVE, cmdStateUpdateProgress, buildStateFrontmatter and cmdAuditUat's active set — every one of those hand-rolled an isDirInMilestone filter with no sentinel exclusion, so `milestone complete` was archiving backlog directories. scripts/lint-phase-enumeration-drift.cjs now reports 0 re-derivations and npm run lint:ci is green. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * docs(#3185): document milestone-scoped enumeration and record ADR Amendment 3 Changeset fragment (Changed), CLI-TOOLS/COMMANDS/USER-GUIDE updates for the scoped output of progress, stats, phases list, phases clear and milestone complete, the CONTEXT.md Phase Locator glossary entry naming listMilestonePhaseDirs, and ADR-3180 Amendment 3. Amendment 3 records: the SCOPE contract held unchanged; the declared deviation from Decision 1's provisional signature (the window needs cwd/ws, which the locked roadmapContent parameter cannot supply); the copy count being a lower bound for the third consecutive phase (4 scoped vs 54 found); the load-bearing finding that the sentinel exclusion sat on the heading set and was unreachable under the pass-all degrade; the two destructive-path defects; and the generalized exemption rule. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * fix(#3185): wire scope to consumers; revert two wrong routings the suite caught Review + remote runner findings, all fixed: The three consumers computed the enumeration scope and threw it away, so TRUNCATED/UNSCOPED/UNREADABLE collapsed into the same output as COMPLETE -- reproducing this epic's own output-identical-failure defect one layer up. progress, stats and phases list now emit phase_scope (null on the phases list --phase lookup path, which performs no enumeration). Two routings were wrong and the suite proved it: roadmap-parser's two milestone phase-count scans are reverted to the 999-only literal. isSentinelPhaseId is BROADER than what it replaced: its legacy branch runs /^0*(\d+)/ over "00.1", which backtracks to capture 0, so it read #2554's decimal phase ids as sentinel milestone 0 and stopped counting them. state.cts phaseInventoryProvider is reverted to the physical disk scan. `state rebuild` is a RECONCILIATION pass -- scoping it made it throw on healthy trees whose fixture resolves no window, swallowed the raw readdirSync fault message #3057 B1 requires verbatim, and stopped it dropping orphan STATE.md rows, which is the job. Both are now function-scoped guard exemptions with written reasons, not silent reverts. This is the consolidation trap named in the epic: a canonical rule can cover MORE than the copy it replaces, and only real inputs show it. Adds phases list coverage, a scope-branch test, and a drift-guard unit suite; backports comment-awareness to the milestone-window and plan-count guards so all three siblings share one false-positive profile; names #3161 alongside #3167 in Amendment 3's subsumption record. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * fix(#3185): correct isSentinelPhaseId's decimal-zero misclassification An isolated security review caught this branch committing the epic's own sin: the over-broad predicate was worked around at ONE call site and left live at the destructive ones. isSentinelPhaseId's legacy branch ran /^0*(\d+)/, which backtracks so any id whose leading digit run is all zeros before a non-digit captures 0 -- "0.1", "00.1" and "0.2554" all read as sentinel milestone 0. Two pinned contracts disagree with that: #2554 requires "00.1" to be counted as a real phase, and the 999 icebox is a whole reserved milestone so "999.1" must stay sentinel. The rule is asymmetric and now says so explicitly: 999 is sentinel with or without a decimal part; 0 is sentinel only when bare. A decimal phase under either is a real phase for 0 and reserved for 999, because 999 reserves a MILESTONE while 0 reserves a PHASE. Fixing the owner lets the earlier workaround go: getMilestonePhaseFilter's two scans route through isSentinelPhaseId again and the guard exemption that existed only to accommodate the defect is deleted. The state.cts cmdStateRebuild exemption stays -- that one is a genuine reconciliation-wants-the-physical-set case. Also corrects tests/adr-612-bracket-grammar.test.cjs, which asserted isSentinelPhaseId('0.1') === true and so had encoded the defect as expected behavior. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * fix(#3185): keep isSentinelPhaseId's semantics — 0.x is layered, not wrong Reverts the previous commit. The remote suite failed six tests proving it wrong, and the reason is the sharpest finding of this phase. An isolated security review observed that isSentinelPhaseId reads 0.1 and 00.1 as sentinel milestone 0 and judged that a defect against #2554. Correcting the canonical predicate broke #2949. Both contracts are pinned and both are right, because they ask different questions: #2554 is this dir part of the current milestone's phase SET? -> count 00.1 #2949 must this phase COMPLETE before the milestone closes? -> 0.x sentinel No single global predicate answers both. isSentinelPhaseId keeps its semantics (0.x IS a sentinel, #2949), and the milestone-window layer keeps a narrower 999-only rule (#2554) as a function-scoped guard exemption with a written reason — not a second silent copy. That corrects how Decision 1 reads: "one owner per derivation" governs who computes an answer, not how many questions share it. An over-broad canonical rule is as much a defect as a divergent copy and fails worse, because it looks like consolidation. Recorded in Amendment 3 as the lesson for Phases 4 and 5. Where a review's inference about intent conflicts with a pinned contract, the pinned contract wins; the finding is adjudicated, not fixed. The boundary tables in the enumeration suite are corrected to assert 0.x IS a sentinel, with the layering explained. Refs #3180 #3185. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG * chore(#3185): set changeset fragment pr to 3222 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QELmgcSwcNBgbUs3kzJeqG --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
bc3a3f170f |
docs(#3215): add ADR-3212 lexical seam consolidation (Phase 0) (#3221)
Design-lock ADR for epic #3212 — the lexical layer beneath the #1372 markdown-sectionizer and #2143 table/mutation seams. Locks seven decisions Phases 1-4 execute against: - src/pattern.cts as sole owner of dynamic regex construction, delegating to the built-in RegExp.escape; the ten private escapeRegex/escapeRegExp/escapeRe copies are deleted, not merged - engines.node floor raised to the Active LTS line (>=24), which is what makes RegExp.escape reachable; delegating-shim alternative recorded and rejected - src/text-lines.cts as sole owner of line-terminator handling — the primitive the existing no-crlf-fragile-split prohibition lacks; brings frontmatter.cts in from the #1372 exclusion - tokenizer-first for stateful grammars, with a decidable five-condition test, generalizing the proven hooks/lib/git-cmd.js token-walk (#3129) - bounded quantifiers over caller-supplied content - extend-never-mutate (inherited from ADR-2143 §2) - prohibition with teeth: no-adhoc-regex-escape, no-unbounded-quantifier, no-crlf-fragile-split widened to src/, plus a parity assertion Explicit non-goal: no wholesale regex-to-parser rewrite. A census found 2,113 regex literals across 317 files; most are correct and stay. Docs-only. No production code. Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
342590c70e |
refactor(#3184): milestone windowing has one owner and a decidable failure signal (#3209)
* test(#3184): failing-first milestone-window single-owner suite Covers the 50 input classes in the phase test matrix: scope classification (genuinely-empty vs truncated vs unscoped vs unreadable), the section-end owner's level boundaries, consumer-output identity per ADR-3180 Decision 4(c), the milestone.complete refusal with negative proof that no directory moved, the version-token boundary defect, drift-guard behavior, and three fast-check properties over document-shaped generators. Committed alone so the remote runner records the failure before the fix lands. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * refactor(#3184): milestone windowing routes through one owner Three copies of the milestone section-end walk lived in roadmap-parser.cts — two distinct computeSectionEnd function nodes plus an inline third in getMilestonePhaseFilter's versionOverride branch. computeMilestoneSectionEnd is now the sole owner and the other two are deleted, not kept in sync by comment. The whole-repo drift guard found what the epic did not: state.cts held three more re-derivations of the same vocabulary — two byte-identical milestone bounding checks carrying a defect neither reported copy has (no boundary after the version token, so v2.0 matched inside v2.0.1), and a milestone-sectioning predicate. All three route through the owner now. A composition-level duplicate appeared inside this change's own first pass: getMilestonePhaseFilter and cmdMilestoneComplete each re-assembled a window out of the owner's primitives, and had already diverged on whether to skip a closed milestone heading. sliceMilestoneWindow is the one composition. Windows now carry the ADR-3180 SCOPE discriminator, so a truncated window is distinguishable from a genuinely empty milestone — those were output-identical, which is the whole failure class. roadmap analyze emits it (#3165), and milestone complete refuses to archive on anything but COMPLETE rather than pass-all moving every phase directory on disk (#3166). The pass-all degrade is preserved where its premise holds: making the filter deny-all would trade a silent over-inclusive answer for a silent under-inclusive one on the read paths that count with it. extractCurrentMilestone keeps its signature — 200+ affected symbols across 41 files and 25 process flows — and is a one-line wrapper over the scoped owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * fix(#3184): fence-aware phase detection and one heading-selection owner Review fixes from the two orthogonal passes. The blocker: hasPhaseEntries matched ATX phase headings fence-aware via tokenizeHeadings but tested the #2199 bullet form against un-stripped markdown, so a fenced EXAMPLE of the bullet syntax counted as a real phase. A genuinely empty milestone then classified TRUNCATED and milestone complete refused a legitimate archive — a false positive in the destructive direction, worse than the defect this phase set out to fix. Both that path and getMilestonePhaseFilter own pre-existing bullet scan now run on stripFencedCode, since leaving one meant the owner file gave two different answers to the same question. The selection rule — locate, prefer the non-closed heading, else the first — had been written three more times inside the file whose thesis is single ownership. selectMilestoneHeading owns it; all three sites route through it. The copies were behaviorally identical, so this is de-duplication with no observable change, verified by probing that all three paths select the same heading. roadmap analyze emitting a scope no consumer read left #3165's actual symptom alive, so Route 0 in next.md now treats a non-complete scope as scan-failed rather than as a clean empty scan, and the ADR amendment no longer overstates what shipped. Also: the scope refusal moved above the archive-directory create, so a refusal leaves nothing on disk; the versionOverride comment names all four consumers; COMMANDS.md documents the new guard beside its sibling. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * test(#2658): exclude the changelog from the malformed-path scan The gate walks every emitted .md/.js/.cjs file in an installed tree and asserts none contains `.claude/.trae/rules` or `.trae/.trae/rules`. CHANGELOG.md ships into that tree, and its #2658 entry quotes both malformed paths while describing the fix that removed them — so the release note documenting the fix trips the fix's own regression test. Red on next before this branch. The installer is correct: a probe over a real --trae --local install found 621 emitted files, exactly one hit, and it was gsd-core/CHANGELOG.md. The scan scope was the defect, not the product. Excluded by exact relative path rather than by loosening the patterns or skipping all markdown — the emitted agent and command markdown is precisely what #2658 was about, so the gate stays strong everywhere it matters. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * test(#3184): regenerate install-tree fixtures for the shared drift scanner scripts/lib/ ships in the npm package and installer, so extracting the shared tree-walk into scripts/lib/drift-scan.cjs adds one path to every runtime's install tree. Regenerated via npm run gen:install-tree; the delta is exactly that one path per fixture. The two drift guards themselves do not ship (scripts/lint-*.cjs is excluded), so only the extracted library moves. This matches the existing scripts/lib/allowlist-ratchet.cjs precedent, which is likewise a lint-only helper carried in the shipped tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * fix(#3184): restore the #730 sub-milestone boundary and narrow the refusal The remote runner caught two regressions this branch introduced. Both were mine, and neither review pass found them — only running the existing suite did. The version-token boundary. I replaced locateMilestoneHeadings' \b with (?![\w.-]), reasoning that v2.0 matching inside v2.0.1 was the same defect #2562 fixed in isMilestoneShippedInRoadmap. It is not the same question. A milestone state of v8.0 legitimately selects the '## v8.0-B' sub-milestone section over a closed v8.0-A sibling (#730), and \b is what allows it while the stricter boundary forbids it — nine tests in roadmap-phase-fallback said so. Reverted to \b; the state.cts consolidation is now a straight merge with no behavior change, and the v2.0/v2.0.1 ambiguity is left exactly as it was. The ADR amendment and the design doc no longer claim otherwise. The refusal scope. I refused whenever the window was not COMPLETE, but #3166 is about the TRUNCATED window specifically — the heading is found and the section closes before the phase region, so pass-all archives everything. UNREADABLE and UNSCOPED are pre-existing, legitimately handled states, and refusing on them broke 'handles missing ROADMAP.md gracefully' and three archive tests. Narrowed to TRUNCATED; docs corrected to match. One of the new tests was also wrong: its fixture gave the shipped and current milestones' phases the same numeric id, and the filter matches on that id, so it could not have distinguished the two windows. Fixture corrected to exercise what it claims to. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * fix(#3184): enumerate drift-scan.cjs for uninstall The installer copies scripts/lib/ wholesale, but uninstall removes an explicit set — deliberately, so a user's own helpers in that directory survive. The extracted drift-scan.cjs was copied in and never enumerated, so it outlived uninstall, left the directory non-empty, and the rmdir that follows failed. Added to GSD_SCRIPTS_LIB_FILES, following allowlist-ratchet.cjs, which is likewise a lint-only helper that ships there and is enumerated. Verified with a real install-then-uninstall into a temp target: scripts/lib/ held exactly the three GSD files and was gone afterwards. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * test(#3184): assert install and uninstall agree on scripts/lib and scripts/changeset Found while shipping this phase, and fixed here rather than noted. install() copies scripts/lib/ and scripts/changeset/ into the target WHOLESALE — the comment at the copy site literally says "and any future lib helpers". uninstall() removes them by hardcoded enumeration, deliberately, so a user's own helpers in those directories survive. A wholesale writer paired with an enumerated remover cannot stay in sync by construction: any file added to either directory ships to every user and is then orphaned in their repo forever, since it survives uninstall, leaves the directory non-empty, and the rmdir that follows fails. Nothing reported this. 31,225 tests were green over it. That is the same divergence class this epic exists to delete, sitting in the installer, so it gets the same remedy CLAUDE.md prescribes for it: a parity assertion that fails the moment the two surfaces disagree. The test compares each directory's real contents against its enumeration and names the offending file plus the constant to add it to. Both enumerations are hoisted to module scope and exported, so the test asserts on the actual arrays rather than pattern-matching the installer's source — no allow-test-rule annotation needed. Proven non-vacuous both ways: empty diff on the current tree, correct report when an unenumerated file is injected. scripts/changeset/ turned out to carry the identical defect and is covered too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 * chore(#3184): backfill changeset PR number Also narrows the wording to match the shipped behavior: the refusal fires on a truncated window specifically, not on any non-complete scope. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015kfkRFNUESoBspUYcAQaT3 --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
343835facc |
refactor(#3183): route live-plan counting through scanPhasePlans (#3199)
* refactor(#3183): route live-plan counting through scanPhasePlans scanPhasePlans becomes the sole owner of the live-plan derivation. Twenty-one independent re-derivations across seven modules now route through it, and scripts/lint-plan-count-drift.cjs reports zero, scanning the whole repo rather than an allowlist (ADR-3180 Decision 4a). The epic scoped this at three copies. A whole-repo guard found twenty-six sites across nine files, so Phase 1 absorbs every live-plan re-derivation and Phase 3 narrows to window plus sentinel enumeration. Two sites are exempt with a documented reason rather than a bare allowlist: audit.cts scans one quick task's own directory for a single completion record, and gsd2-import.cts reads a foreign GSD-2 tasks/ layout during a one-time import. Neither is a phase directory. scanPhasePlans gains allPlanFiles (pre-supersession) alongside planFiles so one owner answers both questions: verify.cts's numbering-gap check wants every plan on disk, its pairing check wants the live set. Both fields are additive. Highest-severity fix: cmdPhasePlanIndex, which feeds execute-phase wave scheduling, was scheduling status:superseded plans into waves and reporting zero plans for the post-#3139 nested layout. filterPlanFiles and filterSummaryFiles are deleted; getPhaseFileStats orphaned them and only their own tests still called them. New leaf module src/planning-scope.cts carries the frozen SCOPE discriminator, with its six-gate ripple closed: gitignore, inventory manifest, INVENTORY.md and the CONTEXT.md glossary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * docs(#3183): amend ADR-3180 for the Phase 1/3 boundary re-slice The contract held; the phase boundary did not. The whole-repo drift guard found 26 re-derivations across 9 files against the epic's estimate of 3, and cmdProgressRender re-derives both enumeration and plan counting on adjacent lines, so DW4 was unsatisfiable within Phase 1's original file scope. Records the amended scope, scanPhasePlans's new allPlanFiles field, findOrphanSummaries, the two documented exemptions, the re-derived Tier-2 table, and the describeNonCanonicalPlans trap for later phases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * fix(#3183): complete the canonical pairing rule and gate the naming diagnostic The remote runner went red with 13 deterministic failures on both lanes, and they were right: replacing verify.cts's canonicalPlanStem pairing with summaryCandidates dropped a case the bespoke rule covered. A plan carrying a descriptive slug after its id (68-01-scaffolding-PLAN.md) pairs with its canonical-stem summary (68-01-SUMMARY.md), and summaryCandidates generated no such candidate, so the plan read unsummarized. The fix is to complete the one rule rather than restore a second: summaryCandidates gains a canonical-id candidate, narrowed to fire only when an id pair was actually extracted. countMatchedSummaries, findUnsummarizedPlans and findOrphanSummaries all inherit it. The two-plans-one-summary collision behaviour of the original rule is preserved deliberately and documented in place. Second defect, independently root-caused while verifying: routing the #2893 naming diagnostic through scanPhasePlans exposed it to the loose /PLAN/i fallback, which is correct for counting and wrong for a naming check — a non-canonically-named file was accepted as a valid plan and the diagnostic went silent. cmdPhasesList, cmdFindPhase and cmdPhasePlanIndex now intersect with a strict isCanonicalPlanFile predicate before reporting names. Same class as the describeNonCanonicalPlans trap already recorded in ADR-3180: a question about file naming wants the physical, strictly-matched set; only a question about outstanding work wants the live set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * chore(#3183): register planning-scope.cjs in the eslint migration list tests/repo-invariants.test.cjs asserts every bin/lib/*.cjs is linted xor ignored per its ADR-457 migration state. The new planning-scope module closed five of the six .cts ripple gates - gitignore, inventory manifest, INVENTORY.md and the CONTEXT.md glossary - but not eslint, because that one is enforced by a test rather than by lint:ci, so the local pipeline stayed green while it was missing. Generated from src/planning-scope.cts, so the .cjs is ignored and the .cts is linted, matching every other migrated module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * fix(#3183): replace the plan-count drift detector with a literal tokenizer CodeQL reported 4 high-severity js/redos alerts on REGEX_LITERAL_MD_RE, the backtracking regex that finds "a regex literal mentioning PLAN/SUMMARY and an escaped \.md". Five review rounds found it had two defects, not one: - EXPONENTIAL, then CUBIC. Its "any char" atom `(?:\\.|[^/\r\n])` let a `\.` pair be consumed either as one escape or as two class characters, which is exponential backtracking: 27,464ms on `"/\.mdplan" + "\.".repeat(28) + "X"`. Excluding `\` from the class killed that but left a cubic path — 23ms at N=200, 172ms at N=400, 1362ms at N=800 on `"/" + "PLAN\.md".repeat(N)` with no closing `/`. This guard is the last stage of `npm run lint:ci`, which CI runs on fork pull requests, so a crafted src/*.cts could stall the job. - A DETECTION HOLE. A character class holding a bare, unescaped `/` — e.g. `/SUMMARY[^/]*\.md$/`, an ordinary path-excluding filter — terminated the literal at that `/`, so the scan never reached `\.md` and the guard missed it entirely. (Classes holding an ESCAPED `\/` were already matched; the tests cover those separately as parity, not as regressions.) Both defects have one root cause: regex-literal grammar — `\x` escapes, and `/` inside `[...]` not terminating — is not expressible in a backtracking regex. So the detector is now a tokenizer, not a regex. readRegexLiteralAt reads the literal at a given `/` in a single left-to-right pass with no backtracking, treating escapes as two-character units and suppressing the `/` terminator inside a character class. findRegexLiteralMdMatch restarts it at every `/` on the line, preserving the old "find anywhere" behaviour; MAX_REGEX_LITERAL_LEN (400) bounds each read — including the trailing-flag scan — which keeps the whole-line cost linear. Results: cubic shape flat at 0.06-0.39ms out to N=3200 (25KB), exponential shape 0.01ms at 28 reps and 0.00ms at 64, and the bare-`/` class shapes are now caught. Differential against the old regex over 28,474 lines (those matching FILENAME_TEST_RE but not PLAN_SUMMARY_LITERAL_RE, across src/tests/scripts/ gsd-core/bin/eslint-rules, excluding 265 lines with >6 backslashes on which the old regex hangs): 6 differences, all the tokenizer returning the fuller or newly-correct literal, 0 old-only misses. The `\.md` token stays case-insensitive, matching the `/i` the old regex carried. Also closes three holes in the same new file: - walk() tested entry.isFile(), false for a symlink, so a symlinked src/*.cts was silently unscanned — an evasion of a guard whose stated principle (ADR-3180 Decision 4a) is whole-repo discovery with no allowlist. It now resolves symlinks, but confined: file links must resolve inside the repo root, directory links inside the scanned dir itself. Every sibling drift guard in scripts/ uses the Dirent classification and never follows links, so following them unconfined would have made this the only linter able to read outside the tree — on fork PRs an arbitrary out-of-repo read whose matched fragments reach a public CI log. The narrower directory rule additionally stops `src/up -> ..` from sweeping the whole repo, and the skip list is now checked against resolved paths so `src/g -> ../.git` cannot reach .git/** or node_modules/**. Real paths are de-duplicated and files reported canonically, so a symlink alias cannot shift which FUNCTION_SCOPED_EXEMPTIONS key applies. - Both the reported fragment and the reported FILE PATH are attacker- controlled source text written straight to a CI log, and git permits control bytes in a filename. Both are now escaped — C0/C1/DEL plus the bidi and zero-width controls — so a crafted literal or filename cannot recolour the log, overwrite a line with CR, or fabricate a line that looks like this guard's own success output. Regression coverage in tests/plan-count-single-owner.test.cjs: a child-process probe over both pathological shapes (catastrophic backtracking is synchronous and would freeze the suite rather than fail one test), the bare-`/` class shapes verified to fail against the parent-commit blob, root-confinement tests covering the outside-file, outside-directory, cycle, broken-link and duplicate cases, direct isInsideRoot coverage including the sibling-prefix case that a bare startsWith would let through, sanitizeForReport coverage, and limit-1/limit/limit+1 coverage of MAX_REGEX_LITERAL_LEN derived from the exported constant. The earlier structural assertion was dropped — it checked for the substring `[^/`, which respelling the class as `[^\r\n/]` defeats while staying exponential. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * chore(#3183): backfill changeset PR number Restores b77931869, which a force-push during the ReDoS remediation dropped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
664d49e513 |
docs(#3182): ADR-3180 — planning semantic model single owner (#3196)
* docs(#3182): ADR-3180 — planning semantic model single owner Phase 0 design lock for epic #3180. Names one canonical owner per semantic derivation, specifies the frozen-enum scope contract that distinguishes a genuinely-empty computation from a truncated or unscoped one, and locks the drift-guard contract. Ships no production code. Phases 1-5 execute against this ADR. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma * test(#3182): prune real issue 3182 from phantom-ref guard, fix empty-list regex The guard's own header documents that its list rots: entries are phantom only until the repo's shared issue/PR counter reaches them, and once the counter passes an entry it must be deleted. Creating the Phase-0 sub-issue advanced the counter past 3182, so the guard began rejecting a legitimate citation of a real issue - the failure its header already records happening twice, with PRs 2551 and 2361. 3182 was the last entry, and removing it exposed a latent bug: the regex builder interpolated the list unconditionally, so an empty list yields (?:#(?:)\b)|(?:issues/(?:)\b), whose empty alternation matches every issue reference in the repo. Following the file's own maintenance instruction would have turned a green guard into one failing on nearly every file. buildRefRe() now returns null for an empty list and is exported, with boundary coverage at 0/1/2 entries plus word-boundary and bare-digit negative cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
27aa40f65e |
fix(#3023): stage pi's shared hook bundle outside pi's reserved hooks/ directory (#3175)
* test(#3023): failing-first guard — pi must not stage hooks in its reserved dir pi reserves <configDir>/hooks as its deprecated extension location and warns on every startup when it exists. Assert a pi install stages the shared hook bundle under gsd-hooks/ instead, manifests it there, and never creates hooks/. Also adds pi to the local-scope dir table in install-shared.cjs: pi was in RUNTIME_META but not LOCAL_DIR_NAME, so scope:'local' resolved path.join(root, undefined) and no local pi install could be exercised. Fails before the fix. Verified via the remote runner. * fix(#3023): stage pi's shared hook bundle outside pi's reserved hooks/ dir pi reserves <configDir>/hooks as its now-deprecated extension location and warns on every startup when that directory merely exists — checkDeprecatedExtensionDirs() guards the warning with a bare existsSync(), unlike its tools/ sibling. GSD staged its shared hook bundle exactly there, and pi's advised remediation (move it to extensions/) would break the adapter's paths and expose GSD's .js helpers to pi's extension auto-discovery. The bundle directory name is now runtime-descriptor-driven: hostBehaviors .sharedHooksDirName, defaulting to 'hooks' so all 18 other runtimes are byte-identical. pi sets 'gsd-hooks'. The name is validated as a single path segment — separators, dot-only segments, trailing dots, absolute paths, NUL, and Windows reserved device names all fall back to the default, because the value is joined onto a user's config root and written to. Renamed in place rather than relocated: hook scripts resolve siblings via __dirname/.., so a depth change would silently break them. - install / uninstall / manifest sites all read the resolved name - pi/gsd.cjs probes gsd-hooks then hooks, so dev checkouts and half-upgraded trees still resolve; the never-throws contract is preserved - new migration 009 retires the legacy pi hooks/ dir on upgrade, using a new non-recursive remove-empty-dir engine primitive (rmdirSync only, symlink-refusing, containment-guarded); ADR-0008 amended accordingly - fixes two latent name-dependencies the rename exposed: the stale-hook scan and the injection scanner's self-exclusion both hardcoded 'hooks' Verified on the remote runner. Closes #3023 * fix(#3023): close review findings and align emitted provenance with the rename Adversarial review found two defects, and the remote runner found four failure clusters. All fixed here. Review BLOCKER — detect-custom-files was blind to the renamed bundle. GSD_PREFIX_MANAGED_DIRS in gsd-tools.cjs hardcoded 'hooks', so for pi the whole gsd-hooks/ tree was invisible to the custom-file scan and user-added files there were never backed up before the next update's clean-install wipe. The dir set now resolves via the .gsd-runtime marker plus the shipped capability registry (never bin/install.js, which is not shipped into installed trees), and falls back to scanning every known candidate when the runtime cannot be determined — over-scanning is safe, under-scanning is the data loss. Review MAJOR — the pi adapter bound to an empty bundle. resolveSharedHooksDir accepted any directory, so an interrupted install left gsd-hooks/ winning over a fully-staged legacy hooks/ and every hook silently no-opped. A candidate now qualifies only if it is non-empty. Remote-runner clusters: - emitted-provenance had no rule for the gsd-hooks/ family; added two pi-scoped rules pointing at the same sources the existing hooks/ rules use. The table is total, so an unattributed family is a hard failure by design. - pi tests in install-minimal-hooks and the install integration suite asserted the old layout; updated to derive the dir name from the descriptor rather than hardcoding either name. - 19 unrelated-looking failures on node22 only were a leaked fs mock: t.after() runs in registration order, cleanup was registered before mock.restoreAll(), and node22's JS rimraf calls the public fs.rmdirSync while node24's native path does not — so the EACCES stub leaked process-wide on one lane. Restore now runs first. Verified on the remote runner. * fix(#3023): honor PI_CODING_AGENT_DIR, ack the rename ripple, fix expandTilde pi resolves its agent dir as PI_CODING_AGENT_DIR ?? ~/<CONFIG_DIR_NAME>/agent (packages/coding-agent/src/config.ts). GSD's pi descriptor declared an empty configHome.env, so a user with that variable set had GSD installed where pi never looks. Added the env name; the dot-home-nested resolver already handled the override, so no resolver logic changed. Also fixes expandTilde in the shared runtime-homes resolver, found while adding that: it hardcoded os.homedir() and ignored the opts.home every caller threads, so EVERY runtime's tilde-valued env override (claude, antigravity, windsurf, pi) silently resolved against the real home. That is a correctness bug and a test-escape hazard — a sandboxed test asserting on a tilde override reached the developer's actual home directory. Now threaded through every branch; behavior with no injected home is unchanged. Adds the emitted-drift ack fragment for the 58 pi paths whose emitted location moved with the rename. The provenance rules satisfy the totality gate; the differential gate needs the ack because the hook sources are byte-unchanged — only the installer's target directory moved. The two hook files this branch genuinely edits stay attributed and are not double-acked. Note on piConfig.configDir: it is read from pi's OWN installed package.json (getPackageDir walks up from pi's __dirname), alongside piConfig.name — a white-label setting for a redistributed pi fork, not a per-project user setting. Documented accordingly rather than treated as an unsupported override. Verified on the remote runner. * fix(#3023): reject blank env overrides, pin adapter/descriptor parity Three review findings, all fixed. A whitespace-only config-dir override was accepted verbatim: the guard was `if (val)`, falsy only for the empty string, so PI_CODING_AGENT_DIR=' ' resolved to a literal three-space directory name instead of falling back to the descriptor default. Fixed across every env-consuming branch — dot-home, dot-home-nested, all three xdg steps, and generic-agents-root — not just pi's. Non-blank values are still never trimmed, so '~/My Agent Dir' keeps working. pi/gsd.cjs's probe list and the descriptor were two independent sources of truth for the bundle directory name; a future rename would have desynced them silently and left every pi hook quiet with no error. The probe list stays deliberate — it must resolve in a dev checkout and a half-upgraded tree, where the registry's answer would be wrong — so this adds the parity assertion the repo's generative-fix-divergence rule calls for: the descriptor value must be the FIRST candidate, and the default must remain present. Changeset body rewritten to cover the two later user-facing fixes it had not caught up with. Verified on the remote runner. * chore(#3023): backfill changeset PR number * fix(#3023): anchor injection-scan patterns and fix a macOS detection hole CI's security job flagged CONTEXT.md:124 — pre-existing prose reading 'not the same fact as a genuinely empty or absent one'. The match was the 'act as a' INSIDE 'f-act as a': the pattern had no left word boundary, so any word ending in act tripped it (fact, impact, contract, artifact, interact, redact, abstract). My four-line CONTEXT.md edit dragged the latent false positive into this PR because the scan is diff-scoped by file but reads whole files. Anchored with (^|[^[:alnum:]]) rather than rewording maintainer-owned prose, which would have left the class alive for the next PR touching any file saying 'fact as a'. Auditing the rest of the list for the same class surfaced a real detection hole: the eval/exec/Function patterns matched a quote via \x27, a GNU-grep-only hex escape. BSD/macOS grep reads it as four literal characters, so single-quoted eval('...')/exec('...') payloads were NEVER detected there while passing on GNU-grep CI. Replaced with a literal apostrophe class. Boundaries were added only where a real word-suffix collision exists; exec, jailbreak, developer mode and the role-manipulation family were audited and deliberately left unanchored. 22 new cases cover both directions — the false positives now scan clean, and every real payload still fires, including the quote/punctuation/start-of-line boundary forms. Also builds this branch's injection test fixture at runtime instead of carrying the literal phrase, so the payload keeps its teeth without tripping the scan. Verified on the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
c643320cef |
docs(#3155): ADR-3128 — shipped default is off, not adaptive
The maintainer decided the default after the ADR first merged, by consistency with the shipped workflow config: verification gates default on (research, plan_check, verifier, nyquist_validation, security_enforcement), agent autonomy defaults off (auto_advance, research_before_questions, plan_bounce, cross_ai_execution). Installing probes into tracked source without a second confirmation is autonomy, not a gate. Adds Decision 8, flips the legacy/absent-section default and the precedence tail to off, and marks Open question 2 resolved. Decision 1's justification is corrected rather than deleted. It rested on 'adaptive carries no flag', which the new default makes false. The conclusion is unchanged and the real reason is stronger: precedence includes the saved session policy, so a resumed session that persisted adaptive passes no flag either, and a flag-keyed atom would exclude the section from exactly the sessions already running the protocol. Closes #3155 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
55136a19e9 |
docs(#3155): ADR-3128 adaptive runtime evidence — Phase 0 design lock
Records the design decisions #3128's maintainer approval made a condition: schema v1, the probe/artifact ownership model, and the cleanup state machine that gates terminal transitions. Also amends ADR-1671 with a RESERVED atom rather than a widening. The vocabulary stays at 29 until #3128's implementation lands; the reservation exists so the widening is a coordinated decision rather than an organic edit found in review. The load-bearing decision is the atom's shape. #3128's probe policy is tri-state (adaptive|force|off), so gating on flag:--runtime-probes would exclude the protocol section from every default invocation -- adaptive carries no flag -- and the feature's primary mode could never activate. That is admission gate (2)'s silent-exclusion failure arriving through a different door: not a fact nobody computes, but a fact computed for only one of three policies. The atom is therefore a resolved boolean folded in cmdInitDebug. Closes #3155 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
7203011400 |
feat(#3072): ship the deferred MCP served catalog (resources + prompts) (#3083)
* test(#3072): add failing-first coverage for the mcp served catalog 55 input-class rows from the phase test matrix, across four suites: the catalog module over injected readFile/readDir seams, the protocol surface through handleMessage, the install-vs-catalog parity gate, and fast-check properties for uri round-trip, traversal refusal, and pagination partition. src/mcp-catalog.cts lands as a skeleton whose functions throw, so the suites fail on BEHAVIOR rather than on a missing module. The REASON enum is real so tests assert typed codes instead of message prose. Hostile coverage for the one client-controlled path surface (resources/read): dot-dot and backslash traversal, percent- and double-encoded traversal, absolute posix and windows paths, file:// scheme, null byte, symlink escape, unindexed sibling, non-string and empty uri, wrong root segment. IO faults are injected by monkeypatching the seam, never chmod 0o000 - root bypasses mode bits, so a permission-based test silently passes with zero coverage in root CI. Refs #3072 * feat(#3072): serve the mcp catalog as resources and prompts gsd-mcp-server now serves GSD's own content alongside its three tools: the workflow, reference and command tree as MCP resources (resources/list, cursor paginated, and resources/read over gsd://<segment>/<relpath> uris) and the 71 commands/gsd/*.md as MCP prompts keyed by bare command name. initialize advertises resources and prompts, and deliberately does not advertise subscribe or listChanged - the catalog is fixed for a server process lifetime, so declaring a notification we never send would be a lie a host acts on. Composition scope is SHARED, not re-declared. shouldCompose lives in src/mcp-catalog.cts and bin/install.js now imports it instead of carrying its own regex, so the served catalog and the installed file floor cannot drift on what gets composed. Proven behavior-preserving across all 2871 tracked paths plus windows-backslash, absolute and near-miss-prefix cases: zero mismatches. tests/mcp-catalog-parity.test.cjs asserts served text equals the installer composition-stage text over the real tree, with anti-vacuity guards requiring both a marker-bearing workflow and a non-composed file in the comparison set. Two measurements corrected the literal issue text. Composition is scoped to gsd-core/workflows/ only, because a reference or command that documents marker syntax with an unfenced example would otherwise be parsed as carrying a real marker and have that line lossily dropped. And parity is asserted at the composition stage rather than against an emitted runtime tree, since install applies per-runtime path rewrites afterwards and the catalog is host-agnostic, so byte equality with any one runtime would be false by construction. resources/read is the one client-controlled path surface and is guarded in two independent layers: the uri must be an exact key in the prebuilt index, which defeats every traversal string by construction, and the mapped path is then re-checked with validatePath so a symlink planted inside a root after indexing is still refused. Also fixes a real drift defect found while here: SERVER_VERSION was hardcoded 1.7.0 while the package is at 1.9.1. It now resolves lazily from VERSION or package.json, reusing the precedent in runtime-artifact-conversion. Closes #3072 * test(#3072): make the catalog parity gate drive the real installer Review found the parity gate vacuous: it never imported or spawned bin/install.js, and recomputed the installer side with the SAME shouldCompose and composeWorkflow the catalog calls internally. It therefore proved only that src/mcp-catalog.cts is self-consistent. The old row 52 compared shouldCompose against a regex literal frozen in the test file rather than against the installer at all. An inline divergent regex re-added to bin/install.js - the exact regression ADR-1671 asks this gate to catch - would have left the suite green. The gate now spawns a real bin/install.js and compares the composition DECISION, observed as gsd:section marker survival, against what the catalog serves for the same files. Marker presence is the right observable because the installer applies per-runtime path rewrites after composing while the catalog applies none, so raw byte equality between the two surfaces is false by construction and must not be asserted. Sensitivity was proven, not assumed: overlaying the shouldCompose export that bin/install.js imports so it always returns false makes a real spawned install leave autonomous.md's markers in place while the catalog still strips them, and the row 48 assertion diverges. Anti-vacuity guards are kept and extended - the comparison set must be non-empty, must contain a workflow that actually carries markers, must contain a file the predicate declines to compose, and the install must have emitted a non-zero file count. The marker-documenting reference case has no instance in the real tree, so it uses an overlay fixture built with the same technique workflow-fragments-emission.install.test.cjs already uses. Renamed to .install.test.cjs so it lands in the install suite it now belongs to. Refs #3072 * test(#3072): retarget the unknown-method assertion off a now-implemented method tests/gsd-mcp-server.test.cjs used 'resources/read' as its example of an UNKNOWN JSON-RPC method. The served catalog implements that method, so it now returns -32602 (no uri supplied) rather than -32601. The remote runner caught it deterministically on both linux lanes: -32602 !== -32601. The test's intent is still correct and worth keeping, so it is corrected rather than deleted or weakened. It now uses 'resources/subscribe', which the server deliberately does not implement and deliberately does not advertise in initialize's capabilities, because it never sends the corresponding notification. That turns the assertion into a real contract - the advertised capability surface and the implemented method surface agree - instead of an arbitrary method name a future feature could invalidate the same way. Swept the rest of the suite for other assertions pinning the newly implemented methods; this was the only one. Refs #3072 * chore(#3072): backfill changeset PR number 3083 * test(#3072): make the catalog fake fs separator-agnostic for windows CI caught this on windows-latest (22 and 24): every catalog fixture indexed ZERO entries, surfaced by the anti-vacuity guards as 'fixture catalog must actually index resources for this property to mean anything'. Mechanism: makeFakeFs keyed its dirMap/fileMap on POSIX-joined paths (${root}/${rel}), while production buildCatalog looks paths up with path.join, which is backslash-separated on Windows. Every lookup missed, tryReadDir returned null, and the catalog came back empty. Production is NOT at fault and is unchanged. The same CI run proves it: on windows-latest the real-filesystem tests all passed, including 'installer composition decision matches the served catalog for every file in the real installed tree' and the row-51 non-vacuity proof against a real spawned installer. A real Windows fs accepts both separators; the FAKE did not, so the fake was the unfaithful one and is what changed. Lookup keys are now normalized unconditionally with .replace(/\\/g,'/') in readDir and readFile - never path.sep-conditional, never platform-gated. The row-42/43 injected-fault wrappers got the same treatment, since they compared raw production paths against POSIX-literal fixtures. No assertion was weakened, and the anti-vacuity guards that caught this are untouched - they are the reason this surfaced as a loud failure instead of a suite that silently asserted nothing on Windows. Refs #3072 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
d2e727d3b3 |
docs(#3074): correct adr-1671 mcp citation and stale runtime counts (#3080)
ADR-1671 attributed its MCP deferral to "ADR-857 §7 / #956" at five sites. Neither source supports it: docs/adr/857-capability-system.md contains zero MCP references (its Decision 7 is third-party code-loading, Decision 8 is Runtime-as-Capability), and #956 is the closed first-party MemPalace plugin pre-proposal that ADR-1239 explicitly disclaims in its own header. The deferral itself is sound on ADR-1671's own runtime-partial reasoning and never needed the borrowed citation. Ground it there, cross-reference ADR-1239 as the ADR that owns GSD's MCP surface, and record that a companion MCP server shipped 2026-06-28 with three tools - so "MCP is deferred" is not misread as "GSD has no MCP server". The deferral narrows to the served resources and prompts catalog (#3072). Also record that "deferred-tools", named alongside resources and prompts, is not a deferred surface but an unbuildable one: MCP defines three server primitives and the tools surface is tools/list plus tools/call, so schema deferral is host behavior, not a server capability (#3075). Correct two stale counts: 15 runtimes -> 19 (of 44 capability descriptors). The ADR-857 reference in Open questions is a genuine Phase-6 completion property and is deliberately left untouched. Closes #3074 Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c899f5ada3 |
chore(#3065): build the deterministic load-bearing-fragment contract gate (#3068)
* test(#3065): build the load-bearing contract gate ADR-1671 promised Epic #1671 Phase 7. A post-merge audit of every promise in ADR-1671 against the merged tree found one mitigation asserted-but-absent and two stale records. ADR-1671 names exactly one correctness risk — trimming a load-bearing fragment, with the recorded history of a paraphrased META.RULE causing agent violations — and #2931 amended its mitigation to a deterministic contract gate that proves no load-bearing fragment was omitted or shrunk, treats a floored fragment as a success, and asserts the isolate prefix survives byte-identical, with an explicit anti-vacuity rule. That gate did not exist. What existed was tests/context-composer.test.cjs: synthetic unit tests of the composeWithinBudget primitive over invented fragments, asserting nothing about real declared strategies. The ADR asserted a mitigation that was never built, which is the promised-but-not-built shape the epic's own coverage discipline exists to catch. The gate derives its load-bearing set from declared verbatim strategies rather than a hand-maintained list, so it cannot go stale as upstream changes. It sweeps budgets from 4x total down to a quarter of total and asserts at every step that no load-bearing id appears in omitted or shrunk, that isolatePrefix is byte-identical, and that hardFailed is surfaced rather than silently passed. Both anti-vacuity guards are EXECUTABLE, not comments. One proves the empty load-bearing set guard actually throws. The other proves a sweep that never applies pressure is rejected — because a gate that only ever runs unpressured is exactly how the original mitigation went missing without anyone noticing. Measured: underPressure true at 6 of 7 budgets, false only at 4x total. Three ADR records corrected in the same change, all doc-vs-reality drift: - Decision item 2 describes a composer that trims by priority to fit a measured per-runtime cap. composeWorkflow in fact passes MAX_SAFE_INTEGER with every fragment verbatim (both verified in source), so no trimming happens there; the emitted-byte cap is a separate measure-and-fail gate and Windsurf's limit a bespoke truncation. The wording described an option as shipped behavior. - flag:--converge never reached a terminal state. #2992 withheld six atoms; five were resolved explicitly. This one was resolved in code by reusing state:plan-strategy-converge but recorded nowhere — the same gap #2995 closed for flag:--verify-only, and I closed five of six. - The open-questions list enumerated three questions while two Resolved-by blocks resolved an unlisted Question 4. It is now listed. Refs #3065 * fix(#3065): make the gate assert over production, not a copy of it The isolated review found a blocker, and it was fatal to the gate's purpose: it hand-copied applyBudget's fragment array into the test, so flipping a strategy in src/prompt-budget.cts — say roadmap from verbatim to drop — would leave the gate computing from its own untouched copy and still passing. A guard built as an instance of the very divergence class it exists to prevent (DEFECT.GENERATIVE-FIX) is worse than no guard, because it reports green. Fixed by eliminating the duplicate rather than adding a parity assertion, the same resolution used for the FAMILIES table in #2996. applyBudget's inline construction is extracted to an exported buildBudgetFragments(), which both applyBudget and the gate now call; the 1024 plan floor is exported as PLAN_FLOOR_CHARS instead of being re-declared in the test. The extraction is pure — verified behavior-preserving at budget=2000: hardFailed false, omitted ['context'], projectMd shrunk, plan truncation ~27.8%, all headers present. There is no longer a second copy to diverge from. Also fixed a vacuous assertion the same review caught: isolatePrefix was pinned across the sweep, but no production fragment sets isolate:true, so the value is always '' and the check could never fail. The pinning assertion stays, with an honest comment that nothing in production sets it today, and a second test now constructs an isolate:true fragment set and proves the prefix is non-empty and byte-identical across a roomy and a severely tight budget — which is what makes the first assertion capable of detecting a real change. Refs #3065 * chore(#3065): backfill changeset pr number to 3068 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ed360cd99f |
chore(#2995): extend fragment emission to agents/ and reclaim size-cap headroom (#3058)
* feat(#2995): extend fragment emission to agents/ across every read point Epic #1671 Phase 6.4. `composeWorkflow` stripped `<!-- gsd:section -->` markers only for `gsd-core/workflows/`, so a marked agent shipped its markers verbatim into every runtime — and agent text is loaded into a subagent's context on every dispatch. The issue proposed widening the `copyWithPathReplacement` guard. That is a no-op for agents: agents never traverse that function. Agent content is read for emission at five independent points, and the obvious chokepoint `stageAgentsForProfile` short-circuits on the DEFAULT `full` profile (`skills === '*'` returns the real unstaged directory), so a hook placed there is dead code on most installs. Composition now happens at two call sites instead of five parallel surfaces: `stageAgentsForRuntimeWithConverter` (with `agentsKind` and `kimiAgentsKind` routed through it via an identity converter) and the inline agent loop in bin/install.js. Both compose BEFORE any path rewrite, so a `.claude/` -> `.windsurf/` regex can never reach inside a marker attribute — the ordering #2930 established for workflows. `installCodexConfig` was the fifth read point: Codex embeds each agent's prompt into a per-agent `.toml` via its own readFileSync. Call-graph analysis missed it; the exhaustive per-runtime emission sweep found it. That is why the new guard is behavioral rather than structural — a sixth read point fails the sweep without anyone remembering to extend a list. tests/agent-fragments-emission.install.test.cjs spawns a real installer for every runtime at every agent-bearing scope, derived from RUNTIME_META and the capability registry at run time so a new runtime cannot be silently under-covered. It asserts markers are absent AND the `when="always"` body is retained, so marker-absence cannot be satisfied by dropping content. An identity-composer negative control proves the assertion can fail. Verified: 0 install failures, 0 marker leaks, body retained on 27 runtime/scope paths; red before the wiring on claude(global+local), zcode(global+local), kimi, codex and opencode. Refs #2995 * chore(#2995): give the tightest agents headroom and correct the design lock Epic #1671 Phase 6.4, second half. `agents/gsd-verifier.md` had 12 bytes of headroom under its 49,152-byte LARGE cap and `agents/gsd-debugger.md` had 147 under its 57,344-byte XL cap. Both now extract reference material to `gsd-core/references/` behind an @-reference — the documented DEFECT.AGENT-FILE-SIZE-CAP-BREACH remedy: gsd-verifier 49,140 -> 46,371 B headroom 12 -> 2,781 gsd-debugger 57,197 -> 48,851 B headroom 147 -> 8,493 Byte accounting proves no content was lost: the combined agent+reference delta is exactly the new files' headers plus the agents' slim replacement blocks. Each agent keeps its routing table and a one-line summary per entry, so it degrades gracefully on a runtime that does not inline @-references. `agents/gsd-planner.md` is untouched and still passes both char guards (49,130 < 49,152); it needed no change, so it took none. The other nine LARGE/XL agents carry NO gsd:section markers, and that is deliberate, not deferred. `when=` selection is read from gsd-core/workflows/section-manifest.json, which gen-section-manifest.cjs derives from gsd-core/workflows/*.md only — shape `{workflows: ...}`, no per-agent key, no per-agent init entry point. An agent atom therefore fails admission gate (2) ("a fact the init seam demonstrably computes at a real entry point") and would evaluate false forever while looking like working gating. Marking agents would manufacture exactly the silent-inertness rot the frozen vocabulary exists to prevent. ADR-1671 gains three amendments, two of which close gaps /adr-phase-coverage found against what actually merged: - The 19 -> 29 vocabulary widening shipped in #2994 with no coordinated ADR amendment, which that bullet's own rule forbids. Recorded now. - `flag:--verify-only` was one of six atoms #2992 withheld and deferred to "the LARGE/XL rollout phase". Five shipped; this one is permanently rejected, and that disposition lived only in a merged PR body. - Phase 6.4's own finding: emission extends to agents/, gating does not. CONTEXT.md's glossary was stale on both seams — Workflow Fragments Module still listed the original 4-atom vocabulary and described when= as "not yet acted on", and Section Manifest Module still described InvocationFacts as {waveFlag, phaseNumber, hasPriorPhases}. Both now match the shipped contract. Inventory manifest regenerated AFTER build:lib per the documented ordering landmine; 19 install-tree fixtures pick up the two new references. Refs #2995 * chore(#2995): correct the compose-site count and mark the raw stager Self-review found two comment defects in the prior commit. The agentsKind comment claimed composition lands at TWO call sites; it is three, since installCodexConfig's per-agent .toml writer was added after that comment was written. And stageAgentsForProfile is now production-dead — both callers route through the composing stager — while staying exported and unit-tested, which makes it a trap: it does a raw copyFileSync and short-circuits to the unstaged source directory under the default profile, so a future caller would silently reintroduce the marker-shipping path. Its JSDoc now says so. * test(#2995): guard the marker-documenting-doc class for agents Widening the composer's scope to agents/ makes reachable the exact class #2930 narrowed scope to avoid: a file that DOCUMENTS the marker syntax with an unfenced example is indistinguishable from a real marker, so the composer drops that line from the emitted artifact. Three rows. A fenced example must compose byte-identically. No shipped agent may carry a marker outside a fence — asserted by parsing every real agent and requiring zero explicit sections, which is what makes the fence protection load-bearing rather than decorative. And a non-vacuity row asserts an UNFENCED marker IS parsed as a real marker, so if that ever stops being true the second row is guarding nothing. Also applies two review findings: stageAgentsForProfile's new JSDoc claimed it had no production caller, which is false — bin/install.js's _stageAgents still calls it, and its consumers compose before writing. Corrected to state the invariant instead. And a let/const nit in the emission sweep. * fix(#2995): keep verifier status vocabulary in the agent, fix a wrong fixture The first remote run came back red with three failures. Both root causes were mine. 1. tests/agent-frontmatter.test.cjs requires agents/gsd-verifier.md to literally contain HOLLOW and DISCONNECTED. The Step 4b extraction moved that status vocabulary into gsd-core/references/verifier-wiring-patterns.md, so the agent no longer had it. Byte accounting said no content was lost, and byte-wise that was true — but a contract required those tokens to live IN THE AGENT. That is ADR-1671:66's flexReserve floor stated concretely: a load-bearing fragment must not be trimmed out of its host, and "the bytes still exist somewhere" is not the test. The two status tables are restored to the agent and deliberately mirrored in the reference with a note saying so, so the procedure there still reads standalone. gsd-verifier lands at 47,069 B — headroom 12 -> 2,083, rather than the 2,781 the first attempt claimed. 2. Row 12b of the new marker-documentation guard asserted that an unfenced marker example parses as a real marker, and threw instead: "unmatched /gsd:section close marker". The grammar is WHOLE-LINE only. The fixture had put the OPEN marker inline mid-sentence, so it was correctly not recognised as an open while the close, on its own line, was. That is a real refinement of the hazard this guard exists for: only a marker on its OWN line is mis-parsed — which is exactly how a documentation example is normally written. Row 12b now uses a whole-line marker, and a new row 12c pins the inline case as explicitly NOT a marker. No test was weakened to accommodate the change; the change was corrected to satisfy the tests. Refs #2995 * chore(#2995): backfill changeset pr number to 3058 --------- Co-authored-by: sim <sim@local> |
||
|
Dennis Kim
|
d3ddcaba1c |
fix(#2785): implement missing gate predicate evaluators (#2816)
* fix(#2785): implement missing gate predicate evaluators * fix(#2785): gate predicate numerical coercion * fix(#2785): address evaluator review findings * fix(#2785): use safe frontmatter read seam --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
ad3b9ec486 |
chore(#1671): fragmentize plan-phase.md and repair flag forwarding to the init bundle — Phase 6.2 (#3019)
* chore(#2993): fragmentize plan-phase.md onto the fragment model Epic #1671 Phase 6.2. plan-phase.md is the largest workflow in the repo and carried zero markers; it was deferred out of the Phase 3 pilot for two reasons, both now dead. The 36-byte PRE_PHASE6 headroom was never the blocker it looked like — fragmentizing is net-negative on host source, so the trim is what creates the room. The --mvp interleaving was resolved by measurement in #2992 and no sub-line mechanism is built. - widen WHEN_VOCABULARY 14 -> 19 via a second coordinated ADR-1671 amendment: flag:--ingest, flag:--prd, flag:--research-phase, flag:--reviews, state:chunked-mode - state:chunked-mode is `--chunked` OR config workflow.plan_chunked, and that disjunction is resolved in the FACT, never in the grammar, so a compound condition never becomes an operator - parse the new flags on the plan-phase route; extract six gated bodies to gsd-core/workflows/plan-phase/steps/ behind manifest-gated stubs - prd-express-path.md was already extracted but read unconditionally; its wrapper is now gated, so the existing extraction finally pays off plan-phase.md 94,483 -> 87,575 bytes (cap 94,519): headroom goes from 36 bytes to 6,944. Also closes a surfaced docs gap: five real plan-phase flags (--chunked, --skip-ui, --bounce, --skip-bounce, --granularity) were documented in neither the argument-hint nor help. Making --chunked load-bearing without fixing its siblings would leave the defect class half-open. Refs #2993 * fix(#2993): forward flags to the init bundle so section gating actually fires Blocker found by the correctness review, confirmed directly, and missed by both the isolated reviewer and every test in this branch. Neither workflow forwarded its flags to the init CLI: plan-phase.md:71 INIT=$(gsd_run query init.plan-phase "$PHASE" $GRAN_PARAM) execute-phase.md:84 INIT=$(gsd_run query init.execute-phase "${PHASE_ARG}") So every flag: atom was permanently false in production and its section permanently excluded. For plan-phase that made the PRD express path UNREACHABLE — a regression, since it was an unconditional read before. For execute-phase this is PRE-EXISTING: #2932 shipped `flag:--wave` gating that has never once been true, so `--wave` silently dropped its own wave-filtering guidance. Fixed here under the no-defer rule. Why every test missed it: they drive the init CLI directly with flags, which works. Production goes through the workflow's bash line, which did not pass them — the exact "assert against the shape production uses" trap this branch's own test matrix warns about. - parse and forward --prd/--ingest/--research-phase/--reviews/--chunked (plan-phase) and --wave (execute-phase), using the anchored regex idiom the neighbouring GRAN_PARAM line already uses - add a regression guard DERIVED FROM THE MANIFEST: for every flag:--X section, the owning workflow's init line must forward --X. It fails against the pre-fix files and covers any future atom, rather than spot-checking today's six. Verified through the workflow shape, not the CLI shape: `3 --prd spec.md` now yields ["prd-express-gate"] (was []), `2 --wave 2` yields ["partial-wave"] (was []). Refs #2993 * test(#2993): acknowledge the execute-phase ripple and regenerate install-tree fixtures Remote matrix was red with 46 unique failures, identical on both lanes. Both causes are mechanical consequences of changing shipped workflow content, and neither is visible to any local gate. - emitted-attribution: execute-phase.md grew 163 bytes from the WAVE_PARAM forwarding fix and was unacknowledged, while the ack fragment named plan-phase.md, which SHRANK and therefore needed no ack at all — a stale entry is itself a failure. The reason now names the real ripple. The entry had to merge into the existing 2930 fragment: the ack linter does unconditional cross-fragment duplicate-key detection with no spent/live exception, so a second fragment declaring execute-phase.md collides even when the first is already merged and inert. Resolved per the linter's own guidance and that file's precedent of appending successive ripple reasons to one entry. - golden-install-tree: tests/fixtures/install-tree/*.json are committed and deliberately excluded from the ADR-2719 attribution cutover, so they must be regenerated when shipped tree content changes. Regenerated after build:lib per the ordering landmine. 19 runtimes each gained exactly the six new plan-phase step files; zero paths removed, which is the absolute failure shape those fixtures exist to catch. Refs #2993 * fix(#2993): restore the launcher preamble in an extracted step and follow moved content in its drift guards Second red run: 26 unique failures, identical on both lanes, in two classes. RUNTIME BUG (runtime-launcher-parity, 7 failures) — chunked-planning-mode.md calls gsd_run but carried no canonical launcher preamble, which is what DEFINES gsd_run(). On any non-Claude runtime that step would fail outright. The preamble is now copied verbatim from the canonical source of truth, gsd-core/workflows/_runtime-launcher.snippet.sh, and the fence dedented to column 0 to match the prd-express-path.md sibling (a list-continuation indent breaks the byte-equal preamble match). prd-express-path.md already had a correct one. This is the same defect #2932 hit when it extracted steps; the parity test caught a real bug, not a stale assertion. DRIFT GUARDS (plan-phase-drift-guard, issue-2762-plan-reviews-chunked, skill-frontmatter-contract) — these assert plan-phase.md contains content this branch moved into step files. Retargeted at where the content now lives, with the asserted property unchanged; the ALL-RUNTIMES label COUNT test now reads host + every step file so the count is preserved across the split rather than reduced. Each retargeted guard was verified to still fail when its step file is stripped, so none was weakened into vacuity. No emitted-drift ack was needed: currentSizes() enumerates gsd-core/workflows/*.md non-recursively, so files under plan-phase/steps/ are never in the size ratchet's scope. Refs #2993 * chore(#2993): backfill changeset pr number to 3019 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
f1af47766a |
chore(#1671): widen the when= grammar and key the section manifest per workflow — Phase 6.1 (#3013)
* chore(#2992): widen the when= grammar and key the section manifest per workflow Epic #1671 Phase 6.1. Two blockers stopped the fragment model reaching any file beyond execute-phase.md: the when= vocabulary was frozen at 4 atoms (3 execute-phase-specific), and the section manifest was single-workflow by construction with 'execute-phase' hardcoded into buildSectionManifestField. - widen WHEN_VOCABULARY 4 -> 14 via a coordinated ADR-1671 amendment; the grammar stays CLOSED (one atom, no operators, negation or nesting) and WHEN_PREDICATES stays a hand-written literal map, never deriving a predicate from its atom string - InvocationFacts gains flags: ReadonlySet<string> plus three computed state booleans; add the missing reverse vocabulary/predicate parity guard - key the manifest artifact per workflow; a stale flat {sections:[...]} artifact now fails shape validation instead of being misattributed - wire the field into six init entry points and parse the flags each needs An atom ships only with both a real consuming section and a fact the init seam actually computes. Six surveyed atoms are withheld because their workflows have no dedicated init entry point; an atom without a computed fact evaluates false forever and silently disables its own section. Fixes a defect found while wiring: parseNamedArgs always materializes a boolean flag key, so folding its false into the absent sentinel is required or every flag reads as present and gating is silently always-on. Also resolves ADR-1671:194 by measurement: --mvp stays unmarkable, because its interleaved sites are always-run flag resolution and a ~340 byte block that already delegates lazily. Refs #2992 * fix(#2992): treat any falsy option value as an absent flag and reject unsafe manifest read paths Findings from two orthogonal reviews (Claude /code-review + an isolated adversarial pass); both independently reproduced the first one. - MAJOR: the flags-builder treated only `undefined` as absent, but parseNamedArgs yields `null` for an absent value-flag and `false` for an absent boolean-flag, so `--granularity` read as present on every plan-phase invocation. Fixed at the root: a flag is present iff its option value is truthy. The six per-handler `|| undefined` folds are now redundant and removed, which also closes the duplicate-translation and missed-onboard-handler findings. - MAJOR: state:needs-codebase-map had zero coverage. Added unit, property and real-CLI integration tests. - MINOR: reject absolute, UNC/drive and `..`-traversing `read` paths in the manifest, degrading the whole load to null like every other shape violation. Verified: `/etc/passwd` previously reached section_manifest.read. - MINOR: corrected a stale "4 to 20" doc comment; the vocabulary is 14. Refs #2992 * test(#2992): update the generator suite for the per-workflow manifest shape The remote matrix went red with 5 unique failures, identical on linux-node22 and linux-node24, all in tests/gen-section-manifest.test.cjs. Re-keying the artifact to {workflows:{...}} left this suite asserting the old flat {sections:[...]} shape; nothing else in the tree still does. - three tests read manifest.sections.length, now undefined; retargeted at workflows.<name> with their original intent preserved (a fenced or loop-host marker still asserts NO section is produced, not merely a changed count) - the stale-manifest test wrote its fixture in the OLD shape, so it tripped shape validation and stopped exercising staleness at all. Its fixture is now valid-but-mismatched so FAIL_STALE is genuinely reached again. - added the coverage that exposed: a pre-6.1 flat artifact must report FAIL_MANIFEST_MALFORMED_SHAPE. That is the real upgrade path for an installed tree and nothing covered it. Refs #2992 * chore(#2992): backfill changeset pr number to 3013 --------- Co-authored-by: sim <sim@local> |
||
|
0xdhx
|
c61dd49d95 |
enhance(#2255): blocking catastrophic-shrink guard for curated .planning/ writes (#2301)
* feat(#2255): blocking catastrophic-shrink guard for .planning writes Adds hooks/gsd-write-guard.js, a PreToolUse hook that hard-blocks (decision: 'block', exit 2) a whole-file Write collapsing a curated .planning/ artifact (ROADMAP.md, .planning/milestones/*-ROADMAP.md, STATE.md) below 40% of its on-disk line count. Files under 40 lines are exempt; GSD_ALLOW_PLANNING_SHRINK=1 (named in the block message) bypasses for legitimate milestone resets. Fix 3 of #973 — the only defense independent of per-agent tool config. Registered on the Claude plugin surface (hooks.json), settings-json runtimes (runtime-hooks-surface.cts, self-contained pattern), Kimi spec, and the OpenCode/Kilo plugin buses. Golden install fixtures and INVENTORY regenerated; regression tests negative-controlled (16/16 RED with the hook absent, 16/16 GREEN with it present). * chore(#2255): backfill changeset pr number to 2301 * enhance(#2255): address review — fail-closed reads, typed block output, registration, property test Review fixes for trek-e's CHANGES_REQUESTED on PR #2301: - Blocker 2: register gsd-write-guard.js in BUNDLED_GSD_HOOK_FILES (no-shipping-drift test). - Blocker 3: update the always-on hook enumerations in ADR-766 and CONTEXT.md from six to seven. - Major 4: fail CLOSED on non-ENOENT read errors — only a missing file (new-file Write) passes; EACCES/EISDIR/ELOOP/etc now block, with a typed readError field and the override still honored. Tested, with a negative control against the pre-fix hook. - Major 5: fast-check property test for the SHRINK_RATIO/FLOOR_LINES budget contract (blocked ⟺ newLines < oldLines*SHRINK_RATIO above the floor; sub-floor always exempt), boundary examples pinned. - Major 6: block output now carries typed oldLines/newLines/ overrideEnvVar fields; tests assert on those instead of regexing the free-form reason string. - Minor: CURATED_PATTERNS are case-insensitive (case-insensitive-FS bypass on macOS/Windows); limit+1 boundary tests added for both the floor and the ratio. * enhance(#2255): engage the write guard on Kimi's native payload shape The guard shipped with Claude-vocabulary checks (tool_name 'Write', tool_input.file_path), which #2304 showed leaves a guard dormant on Kimi: the [[hooks]] matcher is registered pre-translated but kimi-cli forwards its native payload verbatim — tool_name 'WriteFile' (bare or module-qualified) and tool_input.path per its tool schemas (src/kimi_cli/tools/file/write.py). The guard matched, saw an unknown name, and exited 0. Apply the same per-guard normalization PR #2326 gives the three sibling guards (name + field mapping, inlined — hook scripts stage as standalone files), and write the block reason to stderr as well as stdout JSON: Kimi feeds stderr, not stdout, back to the model on exit 2, so a stdout-only reason blocks without telling the model why or naming the documented override. Regression tests pipe Kimi-shaped payloads (engage, qualified-name, stderr-reason) plus exemption pins (StrReplaceFile stays out of scope by design; non-curated paths pass) — verified red against the pre-fix guard, green after. * enhance(#2255): rebase onto next; regenerate golden-parity fixtures * enhance(#2255): wire the escape hatch into complete-milestone's reorganize step Review Blocker 1: the guard hard-blocked /gsd:complete-milestone's ROADMAP reorganize — the tree's only legitimate milestone reset and the exact caller GSD_ALLOW_PLANNING_SHRINK was built for. The reorganize step now performs the rewrite through a shell write with the hatch set on the command (a hook inherits the runtime env, so a bare Write cannot carry a per-step override), and a binding test derives the env var name from the guard's typed output and asserts (a) the workflow step sets it and (b) the guard passes the identical catastrophic payload under it — so the next complete-milestone.md edit cannot silently re-break the wiring. * enhance(#2255): drop dead Edit-class mapping from normalizeKimiPayload Review Major 1: StrReplaceFile -> 'Edit' and the old_string/new_string reconstruction were unreachable-by-effect — the guard exits 0 for any tool_name !== 'Write', so nothing ever read the fields they set, leaving guaranteed-surviving mutants against the Stryker bar. The map now carries only WriteFile -> 'Write'; the StrReplaceFile exemption test message states the fall-through it actually exercises. * enhance(#2255): review minors — American spellings; writeSync before exit(2) Minor 1: normalised/normalise -> American house style. Minor 2: the two block paths wrote stdout+stderr via async pipe writes then exit(2) — async-on-Windows, unflushed at exit; fs.writeSync(1/2, ...) makes the block payload durable. * enhance(#2255): assert stderr equals the typed reason, not raw prose Minor 3: the last raw-text match in the suite pinned override-name prose on stderr. The contract is "stderr carries the reason Kimi feeds back" — now asserted as stderr non-empty and byte-equal to the parsed stdout.reason. * enhance(#2255): bind the write-guard's Kimi normalization into the parity test Review Major 2: the guard's normalizeKimiPayload is a 4th inlined copy with nothing binding it. This extends PR #2326's kimi-guard-normalization-parity test (same path and helpers, authored as a superset so either merge order resolves cleanly): sibling byte-parity is existence-gated zero-or-all — trivially green until #2326 lands, full-strength after — and the write-guard copy is bound semantically (map is the value-inverse of convertKimiToolName; the Kimi name for Write must map, or the guard is dormant on Kimi; the path -> file_path half must be present). Byte-parity is deliberately not asserted for this copy: it legitimately omits the Edit-class mapping (Major 1 — dead code in a Write-only guard). * enhance(#2255): refresh golden-parity fixtures for revised guard + workflow * chore(#2255): regenerate golden fixtures after rebase onto next The committed fixture hashes were generated against a tree predating next's latest 11 commits, which independently modified the same install-parity surface. Rebased onto next and regenerated with `npm run gen:golden`. Verified: against upstream/next the regenerated fixtures differ by exactly this PR's own entries -- hooks/gsd-write-guard.js (new), hooks/managed-hooks-registry.cjs, plugins/gsd-core.js, and gsd-core/workflows/complete-milestone.md. No unrelated drift. * fix(#2255): regenerate workflow size baseline for complete-milestone `complete-milestone.md` grew 31071 -> 32061 (+990) when the round-2 review fix bound GSD_ALLOW_PLANNING_SHRINK=1 into the reorganize step, but tests/workflow-size-baseline.json was never regenerated. The per-file workflow baseline test (issue #1074) failed on ubuntu-latest/22 and both macOS shard 1/3 jobs. The growth is justified: it is the escape-hatch binding requested in review round 2 (the guard must not hard-block the tree's only legitimate milestone reset), not incidental bloat. Regenerated via `npm run size:baseline`; the diff is exactly the one entry. * chore(#2255): regenerate golden fixtures and size baseline after rebase onto next * enhance(#2255): bind the shrink escape hatch mechanically — single-use sentinel the guard consumes Round-5 M1: the per-step `GSD_ALLOW_PLANNING_SHRINK=1 tee` prefix was inert (no PreToolUse hook exists on Bash in this family; the write succeeded by dodging the guard, not by the override firing) and the protection was prose. The hatch is now a transport code consults: complete-milestone's reorganize step arms `.planning/.gsd-allow-shrink` with the target's path, keeps the Write tool as the sanctioned path, and the guard — at the block point only — verifies the sentinel is fresh (15 min) and names the pending target, then CONSUMES it and allows that one write. Path-bound + single-use + freshness keep it from becoming a standing unlock. The env var remains as the interactive transport, where it can actually reach the hook. Regression tests written first (negative control: 3 failed pre-fix): the armed-sentinel Write passes and consumes; stale does not exempt; a token for a different file neither exempts nor is consumed; the binding test now takes the sentinel name from the guard's typed output (overrideSentinel), asserts the step arms it, and asserts the step no longer routes the rewrite around Write via a shell pipe. Also in this commit, same file: - m2: block emission is exception-safe — emitBlock() wraps both writeSync sites in their own try/catch that still exits 2, so an EPIPE can no longer convert fail-closed into the outer catch's fail-open. - Header discloses the two reviewed design limits (cumulative sequential shrink; lexical match vs symlinked paths) per round-5 scoping. * docs(#2255): document the sentinel transport across guard surfaces; changeset ends with the (#2255) parenthetical (m4) USER-GUIDE bullet, INVENTORY row (en + ja/ko/pt/zh), the runtime-hooks-surface registration comment, and the changeset now describe both hatches — the single-use sentinel for workflow steps and the env var for interactive use — instead of implying a per-step env can reach a hook. The changeset's trailing `Resolves #2255.` prose becomes the `(#2255)` parenthetical the repo's fragments use (round-5 m4). * chore(#2255): regenerate derived families on the rebased tree (full sweep) Full generator sweep after rebasing onto next @ the body-parser-patched lockfile: build, gen-inventory-manifest, gen:golden, size:baseline. Every regen delta verified to be either a PR-owned entry (gsd-write-guard.js, complete-milestone.md, INVENTORY/USER-GUIDE) or exact convergence to next's committed value for entries our arbitrary-side conflict resolution had left stale (all 18 runtime fixtures checked mechanically). * test(#2255): use helpers.cleanup for sentinel teardown, not raw fs.rmSync The repo's local/no-raw-rmsync-in-tests rule exists for the Windows-EBUSY retry budget; the sentinel disarm now rides it like every other teardown. * chore(#2255): regenerate derived families after rebase onto next Full sweep on the rebased tree (build -> gen-inventory-manifest -> gen:golden -> size:baseline). Every delta is either a PR-owned entry (hooks/gsd-write-guard.js, its registration surfaces hooks/managed-hooks-registry.cjs and the two plugin buses, gsd-core/workflows/complete-milestone.md) or exact convergence to next's committed value across all 18 runtime fixtures. * chore(#2255): regenerate derived families after rebase onto next @ |
||
|
0xdhx
|
cc3ee301a7 |
fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root (#2593)
* fix(#2544): stage the CommonJS marker in GSD-owned dirs, not the config root installSharedHooksBundle wrote `{"type":"commonjs"}` over <configRoot>/package.json unconditionally — no existence check, no merge, no backup — on every install and every /gsd-update re-install. On the 11 affected runtimes that file is often user-owned; on OpenCode and Kilo it is the documented place to declare local-plugin npm dependencies, so a user's name/type/dependencies/scripts were destroyed on each run. The uninstall path already read the file and unlinked it only on an exact content match. That asymmetry was the defect: the discipline existed in the codebase, it just was not applied on the write side. Move the marker into the directories GSD creates and fills with its own .js files — hooks/ (all shared-hooks runtimes, incl. Kimi's own root) and the nativePlugin dir (plugins/ for OpenCode+Kilo, extensions/ for pi) — and stop writing the config root entirely. New src/commonjs-marker.cts owns the marker string plus one ownership predicate (absent / gsd-owned / foreign, fail-closed on an unreadable file) shared by ensureCommonJsMarker and removeCommonJsMarker, so install and uninstall cannot drift apart again. Nothing else depended on the config-root marker: package identity is baked at build time (#378/#498) and version resolution prefers gsd-core/VERSION and already tolerates a missing root package.json (#1383) — Codex has installed without one all along. A package.json in plugins/ or extensions/ is inert to plugin discovery, which globs *.{ts,js} only (see installer-migration 006). Uninstall retires the pre-fix config-root marker, so upgrading users are cleaned up on removal, and still never touches a file it did not write. * fix(#2544): point the changeset fragment at the filed PR The fragment's `pr:` field is only knowable after `gh pr create` returns. * fix(#2544): register commonjs-marker.cjs in the tsc-generated ESLint ignore set bin/lib/commonjs-marker.cjs is tsc output (src/commonjs-marker.cts is the linted source), so it belongs in the ADR-457 ignore list like its siblings. Clears the lint-tests no-var failure and the repo-invariants "linted xor ignored" migration-state test. * fix(#2544): pin the kimi CommonJS marker to hooks/, not the ~/.kimi root The UPGRADE 1 test still asserted the pre-#2544 marker location (~/.kimi/package.json). The marker now lives inside ~/.kimi/hooks — the directory GSD itself creates — matching the updated golden-install-parity and install-tree fixtures. Also asserts the root marker is NOT written. * fix(#2544): make the CommonJS marker write path non-fatal Review round 2, Major 3 + Minor 1 + the stagedHooks nit. ensureCommonJsMarker rethrew any non-EEXIST write error and neither call site caught it, so EACCES on a read-only hooks/, EROFS, or ENOSPC aborted the whole install with a raw stack trace. Every other marker interaction in the module is best-effort — removeCommonJsMarker swallows unlink failures, classifyMarker swallows read failures — and this was the write path, i.e. the one most likely to fail on a locked-down config dir. It now returns a new 'failed' outcome and both call sites warn and continue. Sibling found while sweeping for the same defect class: fs.mkdirSync sat OUTSIDE the try block, so an unwritable parent threw past the guard entirely. Creating the directory is the same environmental hazard as writing into it, so it moved inside. Also in this file: - The hooks marker is now gated on `stagedHooks && hooksOk`, not stagedHooks alone. stagedHooks is computed from the SOURCE listing before the copy loop, so it stays true when the copies land but verifyInstalled() then fails — marking a hooks/ GSD did not successfully populate claims an ownership the install did not earn. - The uninstall rmdir of the native plugin dir is gated on GSD having actually removed something from it. Hoisting it out of the adapter-exists guard (so the marker-only case could prune) had silently widened it into deleting a user-created but empty plugins/ or extensions/ dir — the same "don't touch territory GSD didn't fill" principle this issue is about, inverted. - Kimi's pre-#2544 marker at its native hook root (~/.kimi) is retired at the same call site that writes its replacement. That path is outside kimi's configDir, so installer-migration 007 structurally cannot reach it. * fix(#2544): retire the stale config-root marker via installer-migration 007 Review round 2, Major 1 — the PR's headline claim was false for existing installs. Upgraders kept BOTH markers: the new one under hooks/ and the stale {"type":"commonjs"} at the config root, so their config root stayed pinned to CommonJS and their dependency manifest stayed gone until they uninstalled. The migration is unusual in one way, and it is the part worth reviewing: the config-root marker was never recorded in gsd-file-manifest.json (writeManifest records hooks/, agents/, commands/, scripts/ and the native plugin, never a root package.json), so classifyArtifact answers 'unknown' for it and the planner's own guard downgrades a remove-managed on an 'unknown' classification to preserve-user. 007 therefore supplies the "purpose-built detector for an old GSD-owned shape" that docs/installer-migrations.md#remove-managed sanctions — exact content match, the same predicate removeCommonJsMarker has always used — and declares the resulting classification on the action. A package.json with any other content is left untouched, and there is deliberately no backup-and-remove branch: a non-matching file here is not a patched GSD artifact, it is somebody else's file. Scope is all runtimes. The `runtimes` field is OMITTED rather than `[]`: validateStringArray requires the field to be non-empty WHEN PRESENT, while the runtime filter treats an empty array as "all" — so `runtimes: []` throws at plan time and the migration never runs. The metadata test pins this. Kimi is a deliberate carve-out, named in the migration's own header: its marker lived at ~/.kimi, outside kimi's configDir, and migration relPaths are structurally confined to configDir. It is retired by the installer instead. Registration: shipped-migrations table, .gitignore for the emitted .cjs, the EXPECTED_CHECKSUMS baseline, and the ESLint ignore set. That last one is not copied from migration 006 by rote — 006 needs no entry because it imports nothing, while 007 imports node builtins, so tsc emits its __importDefault helper and the `var` in it trips no-var. This is the same lint gate that made round 1 red. * test(#2544): fault-injection and multi-runtime marker coverage Review round 2, Major 2 + Minors 4 and 5. Major 2 — CONTRIBUTING.md:514-531 is mandatory for install/uninstall flows and the suite had no fs monkeypatching at all. Every branch now covered is one whose doc comment claims it as the module's safety posture: - classifyMarker non-ENOENT lstat error -> 'foreign' (the fail-closed rule), with an ENOENT control alongside it so the test discriminates rather than just asserting one side - classifyMarker readFileSync throw -> 'foreign' (present-but-unreadable never downgrades to the permissive answer) — the fixture's bytes are exactly GSD's marker, so the test fails if the code ever answers on content it could not read - a DIRECTORY at the marker path (CONTRIBUTING:521; the symlink case was already covered with a real symlink, the directory case needs no injection at all) - the ensureCommonJsMarker TOCTOU EEXIST branch — the entire reason for flag:'wx' - the new 'failed' outcome, for both writeFileSync (EACCES/EROFS/ENOSPC) and the mkdirSync that used to sit outside the guard - removeCommonJsMarker unlink throw -> false These save and restore fs methods in `finally` rather than using chmod 0o000, which does not fault under root and would pass vacuously in root Docker and CI. Minor 4 — uninstall was driven for opencode only. pi's extensions/ and both kimi locations now have behavioral coverage, install and uninstall, each paired with a user-authored-file case proving GSD leaves it alone. Minor 5 — the stagedHooks gate had no assertion behind its stated reason. A pre-existing, GSD-untouched hooks/ directory is now driven through a runtime that declares skipSharedHooksInstall and asserted to stay marker-free, with its user content intact. Also regression-tests the uninstall rmdir gate from the previous commit: an empty plugin dir GSD removed nothing from must survive. * docs(#2544): correct stale marker prose, register the module, document the trade-off Review round 2, Minors 2, 3 and 6. Minor 2 — six files asserted the installed ROOT ships the synthetic marker. None was load-bearing (all three walk-up consumers are VERSION-first with try/catch and the marker never carried a `version`), but ADR-457:52 is the rationale for keeping a generated module, so a future reader would mis-derive the constraint from it. Each site is corrected to what is now true: the installed tree carries no package.json with a .name at all, because the only ones GSD stages are {"type":"commonjs"} markers and they now live in GSD's own directories. Two of the six needed more than a location swap. hooks/gsd-check-update-worker.js and the platform-gate test both described `require('../package.json').name` resolving to undefined; post-#2544 that require does not resolve at all, so the history is kept accurate and the present-tense claim corrected rather than just moved. And src/runtime-artifact-conversion.cts described the no-root-package.json case as Codex-only — it is now every runtime, which strengthens that comment's own argument for lazy resolution. The generated .cjs sibling needs no edit: it is gitignored build output, not a tracked file. Minor 3 — src/commonjs-marker.cts had no CONTEXT.md entry, unlike every peer module, and CONTEXT.md is the #2 co-change partner of bin/install.js. Added, including the fail-closed posture and the never-throws contract. Minor 6 — the plugins//extensions/ marker shadows the config root for all .js siblings, so an OpenCode/Kilo user's ESM plugin/*.js stays broken. That is exactly what #2544's Fix section prescribed and it is disclosed in the PR body, but the PR body is not documentation. It now lives in the OpenCode section of docs/how-to/install-on-your-runtime.md, stated as a real constraint rather than a pure improvement, with the .ts mitigation and a fallback for ESM plugins. * test(#2544): attribute the CommonJS marker in the emitted-provenance rules The differential emitted-attribution gate (#2723, landed on `next` after this branch was cut) went red on the macOS shards once this PR rebased onto it. Two distinct causes, both real gaps rather than noise: 1. `plugins/package.json` and `extensions/package.json` matched NO rule — the `native-plugin` rule covers `*.{js,cjs,mjs}` only, so the marker read as an unattributed emitted family. 2. `hooks/package.json` fell through to `hooks-built`, which attributes an emitted `hooks/<X>` to a repo source `hooks/<X>`. There is no `hooks/package.json` in the repo, so it resolved to a nonexistent path. Cause 2 is exactly the failure already documented three lines above it for Copilot's `gsd-session.json` — "a code literal, not a built script" — so the fix follows that precedent rather than inventing one: `package.json` is excluded from `hooks-built` the same way, and a dedicated `commonjs-marker` rule attributes the family across all four roots it can appear in (both hooks roots plus `plugins`/`extensions`) to the sources that actually emit it. Deliberately a RULE, not an entry in tests/emitted-drift-ack.json. An ack is for a one-off ripple and goes stale by design — the gate fails a stale ack precisely so it cannot pre-clear the next change on that path. These markers are a permanent part of the emitted tree from #2544 onward, so they need standing attribution. Verified by reproducing the CI failure locally with GSD_EMITTED_BASE: 3 provenance errors + 12 unattributed paths before, 35/35 green after. * fix(#2544): route the #2717 hooks-surface marker helpers through commonjs-marker #2717 landed a second copy of ensureCommonJsMarker/removeCommonJsMarkerIfGsdOwned in src/runtime-hooks-surface.cts for the runtimes that stage .js hooks via dedicated paths (cursor/windsurf/codex). That copy had drifted from this PR's module on the two properties that matter: - ownership probe: `fs.existsSync` FOLLOWS symlinks and reports false for a DANGLING one, so a dangling package.json symlink classified as absent and the write went straight through it. Demonstrated: against the pre-fix copy, ensureCommonJsMarker() on a hooks/ dir holding a dangling package.json symlink returns true and creates {"type":"commonjs"} OUTSIDE that directory. - create: a plain writeFileSync leaves the classify->write window open, where commonjs-marker creates with flag:'wx' (O_EXCL). Both helpers now delegate to src/commonjs-marker.cts, which is what this PR's own docstring already claimed was the single place these rules are enforced. Exported signatures are unchanged (still boolean), so bin/install.js and the #2717 tests are unaffected. The new subtest is the only coverage that fails if the duplicate is ever reintroduced — the two implementations agree on every non-adversarial input, so the existing suites pass against both. * test(#2544): pin the stagedHooks gate on zcode, not windsurf The Minor-5 coverage picked windsurf because hostBehaviors.skipSharedHooksInstall kept it out of the shared hooks bundle, so GSD staged nothing into hooks/ and the marker was correctly absent. #2717 changed that premise: cursor/windsurf/codex now stage their .js hooks via dedicated paths and get the marker beside those scripts. Measured on this tree, windsurf stages 2 .js hooks and receives a marker — so the assertion was pinning behaviour that is now wrong, not the gate it was written for. ZCode is the durable choice: per #1821 it has hooksSurface:'none' AND no plugin surface to spawn hooks, so GSD stages no .js there by either route (measured: 0 staged, no marker). The property under test is unchanged — a user-created hooks/ directory GSD never fills stays marker-free. * test(#2544): use the shared cleanup helper in the migration test Addresses the review's Major 1. The suppression's stated reason — "no helpers import available" — was not correct: tests/helpers.cjs exports cleanup, and the other test file added in this same PR imports it (tests/commonjs-marker.test.cjs). The local reimplementation dropped two protections that are live on this repo's windows-latest lane: the CWD guard (Windows cannot remove a directory that is the current working directory) and the 20 x 250ms retry budget that absorbs the deferred-scan handle Windows Defender holds on newly-written files. Local function and suppression both removed; local/no-raw-rmsync-in-tests now passes without one. * test(#2544): expect hooks/package.json for the #2717 runtimes The fresh-install contract table predates #2717, which stages cursor/windsurf/ codex .js hooks via dedicated paths and writes the CommonJS marker beside them. All three therefore now receive hooks/package.json legitimately. Measured on this tree: codex stages 3 .js hooks, cursor 6, windsurf 2 — each with the marker; cline/copilot/trae/zcode stage none and get none, so their contracts are unchanged. * fix(#2544): gate the #2717 marker writes on having staged something The three dedicated marker writers #2717 added ran unconditionally. Each one mkdirs hooks/ up front and stages its scripts conditionally on the source existing, so with an absent or empty hook source they created a directory, filled it with nothing, and marked it as GSD's anyway. That is the same write-into-someone-else's-territory this issue is about, and installSharedHooksBundle already guards the identical case with `stagedHooks`. The dedicated paths now carry the matching gate: - cursor / windsurf: `installedScripts.size > 0` - codex: a new `codexStagedHooks` flag. The enclosing guard only proves that hooks/dist EXISTS; it says nothing about whether any CODEX_HOOKS_TO_COPY entry landed. Covered for cursor and windsurf by driving each writer against a src tree whose hooks/ dir is empty. The codex leg is defensive and deliberately uncovered: its trigger state needs a package tree where hooks/dist exists but holds none of the allowlist, which is not constructible from a real checkout. * test(#2544): scope the commonjs-marker sources per root The rule declared one flat source list for every marker root, so `extensions/package.json` was attributed to runtime-hooks-surface.cts (which never writes there) and `.kimi/hooks/package.json` to install-engine.cts. That is not merely untidy. emitted-diff.cjs accepts the FIRST satisfied source, so a flat list containing bin/install.js let any change anywhere in that 13k-line file authorise marker drift for every root — the blanket escape hatch this file's own agents-verbatim comment refuses for exactly the same reason. Sources are now derived per root from ctx.rel. Note the rule ctx is `{ rel, runtime }` and carries no `root`, so keying on ctx.root would have sent every path down one branch silently. * test(#2544): state precisely what the zcode assertion pins The comment claimed the test pinned installSharedHooksBundle's `stagedHooks` gate. It does not, and neither did the windsurf version it replaced: zcode declares skipSharedHooksInstall, so the outer guard skips that helper entirely and the gate is never evaluated. The test passes on the runtime exclusion. What it does pin — the outcome a pre-existing, GSD-untouched hooks/ stays marker-free — is still worth having, and is what the review asked for. The two `staging zero hook scripts` tests are the ones that pin a real staged-nothing gate. Comment corrected rather than left implying coverage that is not there. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
33fd203ccd |
test(#2966): loop QA walk — drive real scenarios across all five loop steps (#2976)
* test(#2966): loop QA walk — drive real scenarios across all five loop steps Adds a headless walk that carries accumulating project state across discuss -> plan -> execute -> verify -> ship against one temp project, layered over the existing tests/helpers.cjs runGsdTools substrate. Findings carry severity. A violation breaks a stated contract and fails the build; a smell is legal under today's implementation but structurally questionable, is recorded, and never reddens CI. Without that split an oracle set derived from current behavior can only ever confirm current behavior -- the harness could not say "this works and is still wrong". The end-to-end test asserts the walk produces at least one smell: a QA harness that reports nothing on a first run against a real engine is far more likely mis-specified than the engine is perfect. It deliberately does not pin smell ids or counts, which would re-freeze current behavior. First run against the real engine: 0 violations, 3 smell classes -- init returns agents_dir outside the project tree; smart-entry emits prose unconditionally so routing cannot be asserted; state-snapshot reports a missing STATE.md through a payload key with exit 0. Also fixes tests/fixtures/index.cjs: createFixture with git:true and planning:false staged nothing, so the commit failed with "nothing to commit". That combination was unreachable until greenfield needed it. Extends RULESET.TESTS.feedback-loop-convergence from estimation to the loop itself. Design lock: docs/adr/2966-loop-qa-walk.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): wire fault injection, make perturbations discriminating Independent review found tests/qa/mutations.cjs entirely unwired: 462 lines exercised only by their own unit tests, with no mutation hook in the scenario DSL and no scenario applying one, while the module header and the ADR described fault injection in the present tense. Dead code documented as live. Adds a `mutate` step field, three perturbation scenarios, and a wiring detector: a self-test scenario whose expectations are known-false and which MUST fail. The previous anti-vacuity check asserted only that the walk produced a smell, which passes on well-known engine behavior regardless of whether the harness wiring works. First perturbation attempt produced zero signal -- progress does not structurally parse ROADMAP.md, so a corrupted roadmap sailed through. A perturbation that cannot fail is the same defect in a new costume. Probes now target roadmap get-phase, and each mutated step runs a clean baseline first so `mutationObserved` records whether the corruption changed anything at all. Also clears four review findings: classify() returned PROSE for exit-0 with empty stdout; `warnings` was structurally unpopulatable on the success path (execFileSync discards it) and is now documented as error-path-only; read-only-idempotence passed vacuously when asked to check idempotence without the data to check it; the ADR miscounted the oracles. Discrimination matrix across 8 mutations x 6 commands: bom, duplicate-phase-id and escaped-pipes are absorbed silently by every probed surface, and progress / smart-entry / roadmap validate never reacted to any mutation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): add path-containment guard for scenario-supplied targets Security review found scenario-supplied paths joined to the temp project with no containment check. step.mutate.target and agent.write keys were validated only as non-empty strings, so a target of ../../../../etc/hosts reached fs.unlinkSync / fs.writeFileSync / fs.symlinkSync outside the project. The symlink mutation was worst: it read the traversed file, wrote a sibling copy, deleted the original and symlinked it back. Not exploitable today -- all shipped scenarios target .planning/ROADMAP.md and scenarios are repo-committed, not runtime input. Fixed anyway: it is a live primitive any future scenario or copied helper can reach. Adds tests/qa/paths.cjs with resolveWithin(): rejects absolute paths, NUL bytes and empty input, normalizes separators unconditionally, and requires containment by path segment so a sibling like <base>-evil is not treated as inside. Non-existent targets resolve via nearest existing ancestor rather than falling back to a lexical compare. Scenario load now rejects traversing or absolute targets up front. oracles.cjs previously carried its own copy of the containment logic; both now share paths.cjs, since a duplicated containment check is exactly the divergence class this repo calls out. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): complete trajectory corpus, report emission, boundary-aware oracle Adds the remaining trajectories and drives all 11 mutations end-to-end. 20 scenarios, 72 steps, 0 violations, 25 smells. Adds qa-report.json with per-step verdicts and a copy-pasteable repro command, plus --keep / GSD_QA_KEEP=1 to preserve a failing tree. A repro line for a tree that was not preserved is marked NOT RUNNABLE rather than emitting a command pointing at a deleted directory. monotonic-progress is now boundary-aware. Two scenarios had been trimmed to stop the oracle complaining at a milestone rollover, which destroys the signal the trajectory exists to produce. Evidence: counters legitimately reset to zero at milestone complete, but the payload milestone_version lags until a new ROADMAP.md is written. So the oracle now scopes by milestone plus workstream, keeps a same-scope decrease as a violation, and records a boundary crossing as a smell. Both scenarios walk the real boundary again. Standards review fixes: oracle findings now carry a structured subject so tests assert on typed fields instead of substring-matching the free-form detail string, resolveWithin throws a typed EPATHESCAPE error, and the absolute-path predicate scenario.cjs had re-implemented now comes from paths.cjs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): fix silently-vacuous fixtures and guard the class Every fixture carried its #2371 provenance comment BEFORE the frontmatter block, and extractFrontmatter returns {} when anything precedes the opening ---. So every scenario reading status/phase/name was operating on an empty object and reporting green. Nine fixtures repositioned; the comment stays, it just moves below the closing ---. Both UAT fixtures lacked a parser-recognized result block, so evaluateUatPassed saw checks.length===0 and could never return passed:true. The uat-fail-then-remediate scenario could not have proven a remediation. Its expect block only inspected blockers, which is empty before AND after, which is why the corpus never noticed. Both fixtures now carry real result blocks and the scenario asserts passed and no_uat_artifacts on each side of the flip. The actual deliverable is the guard: a fixture-integrity block asserting every fixture with a frontmatter shape parses to a non-empty object, that every fixture carries its provenance marker, and that the two UAT fixtures produce opposite verdicts through the real evaluateUatPassed. The first guard written required --- at byte 0, which would never have fired on the regression it exists to prevent; it was rewritten and proven by deliberately re-breaking a fixture. No engine defect here. no_uat_artifacts means no parsed check items, not no UAT files, and it was reporting correctly on fixtures that had none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): make the walk report — smell ratchet, baseline, CI job The harness computed smells into a gitignored qa-report.json that nothing read. In CI it surfaced nothing at all: violations failed the build, but the half of the tool that says "this works and is still wrong" was inert. A QA tool nobody hears is decoration. Adds a ratchet on the same idiom this repo already uses three times over (the regression-test-name allowlist, the emitted-drift acks, the size baseline): a committed smell-baseline.json, per-PR acknowledgment fragments under tests/qa/smell-acks/, and a ratchet script wired into CI. The design invariant is preserved exactly. A smell still never fails a build on its own merits. What fails is an UNACKNOWLEDGED NEW smell -- the absence of a decision -- leaving an author two honest exits: fix it, or record a fragment with a real reason. An empty reason is rejected. The baseline is shrink-only, so a fixed smell must prune its entry. Violations remain unacknowledgeable. Fingerprints are composed only from stable fields (oracle id, scenario, argv, subject discriminator) -- never temp paths, timestamps or counts. Verified byte-identical across two runs in separate temp dirs; an unstable fingerprint would have false-positived every CI run. CI gains a qa-loop-walk job that runs the suite and the ratchet, uploads the report with `if: always()` (it matters most when it failed), and renders a summary a reviewer reads without downloading anything. Also fixes the report runner invoking main() unconditionally on require, so importing it double-ran every scenario and clobbered its own output. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): every smell terminates in a defect or a fixed detector The baseline accepted a smell with a free-text reason. That is a mechanism for designing smells in -- an allowlist nobody revisits. The harness is brand new, so nothing it found is inherited legacy; every finding is a FIRST finding. Each must now terminate in exactly one of two states: REAL -> an assigned defect, entry carries the issue number FALSE POSITIVE -> the detector is wrong and gets fixed, never baselined There is no third "accepted with a good explanation" state, so the ratchet now requires a positive-integer `issue` on every entry. A reason may remain as a human note but can never substitute. `--update` refuses to invent issue numbers: a new smell is written with `issue: null` and a TODO, and the next plain run rejects it, forcing triage rather than accumulation. Working the 21 existing entries through that rule found 16 were my own detectors being wrong: value-hygiene (10) flagged $.agents_dir, a field whose entire contract is to point at the install tree outside any project. Fixed with a leaf-key allowlist of contractually-external fields, verified as the only such key in the init payload. Genuinely unexpected out-of-project paths still smell. monotonic-progress (6) fired on legitimate boundary crossings -- milestone v1.0 to v2.0, workstream beta to alpha -- and on one payload carrying no scope fields at all, where a change cannot even be known. Scope changes now reset silently and scope-less observations are skipped. The same-scope decrease remains a violation; that is the real invariant and is regression- guarded. The five survivors are real and now tracked: soft-error-exit-zero (#2980), untyped-success (#2979). Baseline 25 -> 5. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): keep the ratchet out of the tarball, unpin the qa CI job The remote matrix returned failed -- 3 unique failures, identical on node22 and node24, both root causes in this branch's own diff. The ratchet lives under scripts/, which ships in the npm tarball, and it requires three modules under tests/, which does not. In a published install it is MODULE_NOT_FOUND at load. This is exactly the class the #2858 guard was added to catch, and it caught it. Fixed the way #2858 fixed the same shape for its own repo-only CI script: a targeted files[] negation, so the ratchet stays in the repo for CI and out of the tarball. Not solved by moving or inlining the required modules -- the ratchet must keep using the same code the harness uses, or the two drift. Verified both directions: the script is no longer in the pack list, and build-hooks.js, fix-slash-commands.cjs and gen-capability-registry.cjs are all still shipped. Over-negating there would have broken installs, since bin/install.js requires them. The qa-loop-walk job also carried CI_REBASE_BASE_SHA copied from a neighbouring job without the paired GSD_EMITTED_BASE, which the #2854 invariant forbids by name: diverging them makes the differential compare a tree against a baseline from a different commit. The job runs only the qa suite and the ratchet and invokes no emitted-attribution test, so it needs no rebase-pinned base at all -- the step was removed rather than paired. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2966): stop monotonic-progress going blind on scope-less payloads The full remote suite caught a false NEGATIVE I introduced while fixing a false positive. Silencing the boundary-crossing noise had made the oracle skip ANY observation lacking milestone fields -- so a minimal payload like {total_summaries: n} produced no violation at all, and the oracle stopped catching the exact defect it exists to catch. For a QA tool that is strictly worse than the noise it replaced. Scope is only indeterminate when the two observations DISAGREE about having it: both scoped, same scope, decrease -> VIOLATION both scoped, different scope -> reset silently NEITHER scoped, decrease -> VIOLATION (the regression) mixed -> skip the comparison Implementing the mixed case surfaced a second blind spot: advancing the reference point on a skipped pair lets a scope-less observation sitting between two same-scope ones mask a real decrease. Mixed now leaves the reference untouched. All four branches carry explicit coverage; only one did before, which is why this shipped. The self-test that failed was right and the code was wrong, so the code moved. Corpus behavior is unchanged: still 5 smells, 0 new, 0 stale, 0 violations. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
628648d63a |
chore(#2931): cap emitted per-runtime bytes and single-source windsurf (#2984)
* fix(#2931): preserve protected regions and cap emitted per-runtime bytes Route every runtime brand swap through applyClaudeCodeBrandSwap so "Claude Code" survives verbatim inside <runtime_compatibility> regions (#2284b). The fix existed only in bin/install.js's local copies; the src/*.cts exports still used a naive replace, so binding install.js to the single source -- as this phase does for the Windsurf family -- would have silently regressed those runtimes. A table-driven parity guard now covers all nine brand-swapping converters. De-duplicate the Windsurf converter family: delete the six local copies in bin/install.js and bind the four exported ones by reference, guarded by reference-identity assertions (the ADR-1508/#1675 pattern). The two unexported helpers and an unused tool table go with them. Replace the Windsurf 12,000-byte throw with description truncation, matching the bound its sibling skill converter already applied. The throw could only fire on an ~11.7 KB frontmatter description: the largest emitted workflow is 311 bytes. Truncation makes the cap unreachable by construction and leaves 12,000 in exactly one place, eliminating the dual-surface duplication rather than testing for it. Add the emitted-byte cap gate: buildEmittedSizes captures LF- and <HOME>-normalized bytes from the walk buildParityManifest already performs, and evaluateEmittedCaps asserts them against a per-runtime cap table with dead-rule detection. buildParityManifest's return shape is deliberately unchanged -- diffEmitted compares its values with ===, so making them objects would report all 8,529 emitted paths as moved. A regression test pins the values as strings. Add a deterministic trim-safety gate over composeWithinBudget's omitted/shrunk/floored/isolatePrefix metadata, with an anti-vacuity rule, replacing the model-graded eval gate the issue described. * docs(#2931): correct ADR-1671 windsurf premise and trim-safety contract * fix(#2931): bound the windsurf command name and single-source the brand swap Review findings from the orthogonal passes, all fixed inline. The claim that removing the 12,000-byte throw left total emission "bounded by construction" was false. The #1615 regex constrains the character class but not the length, and commandName is interpolated three times into the emitted workflow: a 20,000-character name emitted 60,162 bytes silently. Add WINDSURF_COMMAND_NAME_MAX=128 as a separate, clearly-labelled size control that THROWS -- commandName is the @-ref path target, so truncating it would point the workflow at a file that does not exist (DEFECT.WORKFLOW-DELEGATION-TARGET-NOT-INSTALLED). The #1615 security regex is untouched and still runs first. 128 is generous: the longest shipped name is gsd-plan-review-convergence at 27. Harmonize convertClaudeCommandToWindsurfSkill onto the code-point-safe truncation helper. It still used a UTF-16 slice(0,177) -- the exact surrogate-splitting bug the helper was written to avoid, in the very sibling the helper's comment cites as its model. Bounds are unchanged, so output is byte-identical for every shipped command (descriptions max out at 99 chars). Export applyClaudeCodeBrandSwap and bind it in bin/install.js, deleting the local copy. Adding it to the .cts left two unlinked implementations of identical logic -- the drift class this change exists to remove. Verified byte-identical across eight fixtures and five sequential calls before merging, and guarded by a reference-identity assertion. Convert three try/finally test bodies to t.after (CONTRIBUTING.md:344), add fast-check property coverage for the trim-safety contract, and use fc.pre instead of a bare return in a property callback. * test(#2931): fix three test-authoring bugs the remote matrix caught The remote runner returned 8 unique failures on 6f15cdeb8. All three causes were in the test files, not the modules under test -- local harnesses exercise the modules directly, so nothing executed the test bodies until the matrix did. `{ __proto__: [...] }` in an object literal sets the prototype instead of an own key, so the JSON round-trip erased it and the cap table never saw a reserved runtime key. The production rejection was already correct; the test could not reach it. Use a computed key. Two cap fixtures tripped orthogonal error paths rather than the paths they name: one declared windsurf in the cap table but omitted it from sizes (UNKNOWN_RUNTIME), the other left the sole windsurf pattern matching nothing (a genuine dead rule). Both now include a compliant artifact so the intended branch is what is asserted. The dead-rule and unknown-runtime contracts are deliberate and unchanged. `const { root } = makeSyntheticConfig({ ... `${root}` })` referenced `root` from inside its own initializer -- a temporal dead zone error. makeSyntheticConfig now optionally takes a (root) => files factory. Also raise the npm pack --dry-run bound 60s -> 120s in the shipped- scripts packaging test. That failure is NOT from this branch: the file is byte-identical to next, a fresh tsc measures 1.98s there vs 2.14s here, and the run recorded 60,637ms against a 60,000ms bound -- a timeout under 28,948-test parallel contention, not a slowdown. Fixed rather than deferred because a bound that tight is fragile regardless of which branch trips it. * chore(#2931): backfill changeset pr number to 2984 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
640eaee16e |
chore(#2930): fragmentize execute-phase.md and prove per-runtime composed emission (#2972)
* feat(#2930): fragmentize plan-phase.md workflow into per-runtime-composed sections Adds src/workflow-fragments.cts (in-file <!-- gsd:section --> marker parser/composer, ADR-1671 epic #1671 Phase 3), wires it into bin/install.js's copyWithPathReplacement emission path, and pilots the marker grammar on gsd-core/workflows/plan-phase.md. Bookkeeping ripple for the new src/*.cts module: .gitignore, eslint.config.mjs, docs/INVENTORY.md + docs/INVENTORY-MANIFEST.json, and a CONTEXT.md glossary entry. Amends ADR-1671 with open questions 1 and 2 resolutions and records the closed when= applicability grammar. Adds docs/reference/workflow-fragments.md and an ARCHITECTURE.md section documenting the marker authoring model. * fix(#2930): put allow-test-rule issue ref on the same line as the marker lint-allow-test-rule-refs.cjs requires the #NNN issue reference on the same source line as `allow-test-rule:`; it was one line below and read as an unreferenced novel exemption. * docs(#2930): link the orphaned gate-predicates reference from the docs index Found while adding the workflow-fragments reference doc: docs/reference/gate-predicates.md shipped without an entry in docs/README.md, so it was unreachable from the docs index. Fixed inline rather than deferred. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2930): scope composition to workflows, add typed failure reasons Review findings from two orthogonal passes: - Scope composeWorkflow to gsd-core/workflows/ only. It previously ran on every .md the installer copied, so a future agent/command/reference doc documenting the marker syntax with an unfenced example would have been mis-parsed and silently stripped — a lossy drop the phase forbids. - Add a frozen REASON enum; failures attach a typed .reason and tests assert on it instead of matching free-form message text (CONTRIBUTING.md:635-694). - Derive the property generator's when= values from WHEN_VOCABULARY instead of duplicating them (DEFECT.GENERATIVE-FIX). - Add adversarial parser fixtures: Unicode headings, NUL, U+FFFD, BOM, fence-within-fence, tilde and indented fences, lone-CR marker line. - Document why --mvp is structurally unmarkable: its content is interleaved, not sectioned, so the whole-line grammar cannot reach it. Also fixes two stale tests on this branch, each reproduced on the unmodified tree before correction. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#2930): retarget the pilot from plan-phase to execute-phase The full remote matrix went red on both Linux lanes. Root cause was ours: tests/phase6-capstone-conformance.test.cjs holds a PRE_PHASE6 ceiling of 94519 bytes for plan-phase.md, asserting an ADR-857 Phase-6 completion property. That is a third size gate beyond the tier caps and the differential ratchet, and it left plan-phase.md just 36 bytes of headroom rather than the 3821 computed from the XL cap. The 330 marker bytes overran it by 294. Raising the ceiling is not an option: it is a red line certifying another ADR's completion. plan-phase.md is reverted to byte-identical origin/next and the pilot moves to execute-phase.md, which has 728 bytes of headroom under its own ceiling and lands at 93147 with 3 marker pairs. The vocabulary narrows to the atoms actually used: always, flag:--wave, state:gap-closure-phase, state:has-prior-phases. Recorded in the ADR: every branch the epic names lives in plan-phase.md, which cannot be fragmentized until caps move from source to emitted bytes. That is direct evidence for the epic's premise and may reorder phases 3-4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * chore(#2930): backfill changeset PR number (#2972) * fix(#2930): make the emission install tests portable on Windows The windows-latest lane went red on three tests in the new install suite; Linux was green. Both causes were in the test harness, not the module. Root normalization: the opencode converter always embeds the install root forward-slashed, but the tests stripped it with the native-separator string from mkdtemp. On Windows that never matched, so the root leaked through unstripped — and because the real and stub install roots have different prefix lengths, that length difference landed directly in the byte-delta assertion (344 observed vs 275 expected). Normalize both text and root to one separator form before stripping. @-ref resolution: the helper stripped only the @~/ and @$HOME/ forms, so a Windows absolute ref (@C:/Users/...) fell through and was joined onto the root, producing ...\@C:\Users\... Strip the @ first, then detect absoluteness from the token's own shape (POSIX, drive-letter, or UNC) with no platform branching, so every OS takes the same path. Neither assertion was weakened; the exact-equality byte check is the point of the test and still holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(#2930): document every REASON member and guard the doc/enum parity Code review found the reference doc's 'Fails closed' list covering 10 of the 11 frozen REASON members — MALFORMED_ATTRIBUTES (parseAttrs rejects malformed key="value" syntax) had no bullet, and it is distinct from UNRECOGNIZED_ATTRIBUTE, which is valid syntax with an unknown key. Two parallel surfaces sharing one constant with nothing asserting they agree is the DEFECT.GENERATIVE-FIX class, so the same commit adds the parity assertion: the test derives the enum side from the built module and the doc side by parsing the reference page, keyed on the reason IDENTIFIER rather than prose so a reworded bullet does not break it, and reports set differences in both directions by name. Proven non-vacuous: removing the MALFORMED_ATTRIBUTES bullet turns the suite red naming that exact member; restoring it returns 44/44. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
9ac0dfad58 |
chore(#2929): generalize prompt-budget into the shared context-composer seam (#2958)
* test(#2929): capture prompt-budget parity corpus pre-refactor Phase 2 of epic #1671 generalizes prompt-budget's trim ladder into a shared context-composer seam. Its success condition is that review-prompt output does not change, and the only authority on "did not change" is the behavior that shipped before the refactor. Capture that behavior now, while it is still the live implementation. 47 characterization cases, every `expected` value computed by executing the current implementation rather than hand-authored — the independence CONTRIBUTING.md "Fixture provenance (#2371)" asks for. A corpus is only worth what it can detect, so this one was validated by mutation rather than assumed. Five deliberate defects were injected and each must be caught by at least one case: - the note reserve deducted unconditionally instead of only under pressure - the pressure test relaxed from `>` to `>=` - a no-op head-shrink still setting the shrunk flag - the per-plan floor dropped from the proportional share - drop order reversed Two of those exposed real holes in the first cut of this corpus, and the cases that close them exist because of it: - `>=` was caught by NOTHING. At exact cap the only trimmable fragment was a floored plan group, and the 1024-char floor absorbed the entire trim, so the mutation was byte-invisible. A3b/A3c put a droppable at exactly the cap, which makes the strict inequality observable as context kept vs omitted. - No case reached proportional-truncate at all — B6 and B7 both hard-failed the min-set pre-check first, leaving planTruncationPct at 0 across every case and the floor semantics entirely unexercised. Rebudgeted to 700 and 1100 so the min-set fits and the truncate step is actually reached; they now record 40.20% and 48.80%. The A4/A10 families sweep the pressure boundary from both sides, which is where this function has regressed before: CONTEXT.md's LEARNING.prompt-budget.boundary-gap records PR #3708 shipping two regressions that only fired when the baseline sat inside the NOTE_RESERVE_TOKENS band, because the suite paired a trivially-fitting budget with a trivially-overflowing one and never sampled between them. A4 pins that nothing is trimmed from the cap down to 81 tokens under it; A10 pins that pressure fires at +1. Together with A3b/A3c they satisfy row (d) of RULESET.TESTS.boundary-coverage.fixtures. Two facts the corpus establishes that the design notes had wrong: - "" and null sections are NOT distinguished. applyBudget uses truthy checks throughout, so an empty-string section is treated as absent: not rendered, not dropped, never recorded in `omitted`. B13b pins this while the ladder is actively trimming, where only the non-empty `research` is dropped. - Sizing matters. B12/B13 were first written at a budget where both hard-failed the min-set check and returned "", so comparing them compared two empty strings and proved nothing. Committed as its own commit, ahead of the refactor, and regenerated against the pre-refactor implementation, so the oracle is demonstrably independent of the change it will adjudicate. Refs #2929 * refactor(#2929): extract the context-composer seam from prompt-budget Epic #1671 needs prompt-budget's budget-trimming logic for a second consumer — per-runtime artifact emission — but it is walled inside the cross-AI review pipeline. Lift it into a shared seam so later phases can call it, without changing what the review pipeline emits. ADR-1671 specifies the composer as "priority + binary-search cutoff to a per-runtime budget". Read against the code it generalizes, that contract cannot express the thing being generalized. applyBudget is not a cutoff: it is a fixed five-step ladder in which each section carries its own shrink strategy, and only three of its eight sections are ever dropped. PROJECT.md is head-shrunk to N lines; plans are proportionally tail-truncated with a per-plan 1024-byte floor; instructions and roadmap are never touched at all. A cutoff composer sorts by priority and discards the tail — it has no way to say "shrink this one", "truncate that one but never below 1 KB each", or "these three are the only droppables, in this order". Building to the literal contract and routing prompt-budget through it would have silently changed review-prompt output, which is the one outcome this phase forbids. So shrink strategies are the core abstraction here, and cutoff becomes one strategy among them — the right one for per-runtime emission in Phases 3-4, not for this ladder. That is an elaboration of the ADR's intent, not a departure from it, and ADR-1671 is updated to say so. Three decisions worth stating: - The composer DECIDES; the caller RENDERS. composeWithinBudget returns a plan of surviving fragments and never a string. assemblePrompt's rendering is prompt-shaped (`## Roadmap`, `### <file>`, the note in position two), and owning it in the composer would force emission to adopt prompt-shaped rendering. The split is what lets one seam serve both consumers. - The budget unit is INJECTED via `measure(text)`. prompt-budget passes its chars/4 estimator; emission will pass a byte counter, which ADR-1671 requires for emission caps. The existing code converts a token budget to a character budget with a hardcoded `* 4`; that assumption is now an explicit `charsPerUnit` inverse, which is precisely what a byte unit needs in order to reuse this. - The entry point is `composeWithinBudget`, not `applyBudget`. That name already exists twice — src/prompt-budget.cts and src/graphify.cts, the latter being an unrelated graph-edge budget. A third would make every symbol search in this repo ambiguous, and it already misresolves: preflight and impact queries for "applyBudget" return graphify's. Behavior is unchanged and proven so: all 47 characterization cases reproduce byte-identically, and the corpus is mutation-validated rather than merely green (see the preceding commit). prompt-budget.cts drops from 436 to 343 lines and from eighteen mutable accumulators to two, both inside a helper copied verbatim. estimateTokens deliberately stays in prompt-budget and keeps its exact math: src/phase-estimation.cts re-exports it as measureTokens, and CONTEXT.md pins plan estimates and recorded actuals to that same scale, so moving or changing it would silently break the calibration loop. Refs #2929 * docs(#2929): document the context-composer seam and amend ADR-1671 Adds the INVENTORY row, the CONTEXT.md glossary entry (a PR gate for new domain modules), and a mutation-matrix entry for the new module. The ADR amendment is the substantive part. ADR-1671 specified the composer as "priority + binary-search cutoff to a per-runtime budget". Implementing Phase 2 established that a cutoff alone cannot express the function the platform generalizes, so the ADR now records shrink strategies as the core abstraction with cutoff as one strategy among them, reserved for per-runtime emission in Phases 3-4. Recording it in the ADR matters because Phases 3-6 are planned against that contract and would otherwise be planned against a mechanism that does not work. The mutation-matrix entry is not bookkeeping. Stryker scores per module against a named .cjs, so relocating the ladder out of prompt-budget.cjs would leave the extracted code unmeasured while prompt-budget's own score floated free of the logic it used to cover. context-composer gets its own entry at the same floor. Refs #2929 * test(#2929): pin the effectiveBudget rounding mode in the parity corpus An isolated correctness review found a real blind spot: mutating `Math.floor` to `Math.round` in the effectiveBudget calculation failed ZERO of the 47 corpus cases. Every (budget, safetyMarginPct) pair in the generator happened to produce a whole number, so floor, round and ceil all agreed and the rounding mode was entirely unpinned by a corpus whose whole job is to pin observable behavior. Three cases fix that by straddling the .5 boundary: A11 95 * 0.90 = 85.5 floor 85, round 86 -> the two disagree A12 97 * 0.90 = 87.3 floor and round agree; ceil (88) does not A13 93 * 0.85 = 79.05 same guard at a non-multiple-of-10 margin, so the margin arithmetic is exercised and not just the budget A11 alone catches the round mutation; all three catch ceil. Regenerated against the pre-refactor implementation (`git show 9557f8552:src/prompt-budget.cts`), so the expanded corpus keeps the independence property the original capture had. The corpus is now mutation-validated against seven injected defects, every one caught: unconditional note reserve, `>` relaxed to `>=`, no-op head-shrink setting its flag, the truncate floor ignored, drop order reversed, and both rounding-mode changes. Refs #2929 * feat(#2929): flexReserve floors and the byte-stable isolate prefix Two of issue #2929's "Done when" items were unimplemented rather than deferred, and an isolated review flagged them alongside my own audit. Both are part of ADR-1671's composer contract, so shipping the seam without them would have left Phases 3-4 building against a contract that does not exist yet. flexReserve is a per-fragment floor in measure units that every strategy must respect, which is what makes it different from the pre-existing floorChars: that one is a chars-denominated detail of proportional-truncate alone and is retained unchanged. A floored fragment is never dropped, is never head-shrunk below its floor, and raises its own proportional cap. A fragment already smaller than its floor is untouchable outright. Metadata gains `floored`, listing the ids whose floor actually prevented a trim — a guarantee no caller can observe is a guarantee no test can hold you to. isolate marks the byte-stable canonical prefix the ADR calls for: never trimmed, never dropped, but still counted, because a prefix excluded from accounting would silently under-count real context. Metadata gains `isolatePrefix` so a caller can hash or assert on the exact bytes. Declaring an isolate fragment after a non-isolate one throws: a prefix that is not at the front is not a prefix, and accepting it would make the cross-runtime stability claim meaningless. Adds tests/context-composer.test.cjs for the exact new semantics and tests/context-composer.property.test.cjs for the five invariants, including the budget-monotonicity property the issue names explicitly. Both are registered in the mutation matrix, since coverage does not migrate with relocated code. prompt-budget uses neither feature, and its output is unchanged: all 50 corpus cases still reproduce byte-identically. Refs #2929 * chore(#2929): allowlist the prompt-budget parity suite The parity corpus needs its own test file and that makes prompt-budget a three-file module against a limit of two. The lint offers consolidation or an allowlist entry with justification; the entry is the right call here. Consolidation would mean folding the characterization suite into prompt-budget.test.cjs, which is the one thing that should not happen to it. The parity suite is a distinct concern with a distinct lifecycle: it is generated rather than hand-written, it is named by scripts/mutation-matrix.cjs as its own scoring target, and its failure means something categorically different from a unit-test failure — not "this behavior is wrong" but "observable output moved". Burying it inside a general unit file would obscure exactly that signal. The allowlist is an identity ratchet, so this entry pins today's three exact filenames: adding a fourth still fails, and dropping back to two requires removing the entry. Refs #2929 * fix(#2929): register the new module with two gates it was missing The remote matrix caught three defects that no local check could, because the local runner is blocked in this repo and these suites had therefore never executed. Eight failures, identical on node22 and node24, so nothing environment-shaped. Two are the new-module ripple. A net-new src/*.cts lands in six places and this change had reached four of them — .gitignore, INVENTORY, the manifest, and the CONTEXT.md glossary — while missing the ESLint ignore list (tsc OUTPUTS must not be linted; repo-invariants asserts linted-xor-ignored) and the mutation ratchet baseline (a deliberate review-visible mirror of the matrix floors, which every COVERED module must carry). Both are now registered, the ratchet at the same floor of 66 the matrix declares. The third was a test asserting an outcome it had made impossible. It set budget:1 alongside a 400-char required fragment, so the group budget came out at -99 and the proportional-truncate step was skipped entirely — the deliberate "non-positive group budget is skipped, never clamped" rule inherited from the original ladder. Nothing was trimmed, and the test then asserted a truncation. Rebudgeted so the step actually runs, with the arithmetic written out in a comment so the next reader does not have to re-derive why 120 rather than 80. Fixing that surfaced a genuine bug in the composer. `floored` is documented as recording fragments whose flexReserve prevented a trim that would otherwise have happened, but the push sat in the else-branch of "content did not change", so it only fired when nothing was trimmed at all. A fragment truncated to a reserve-raised cap has also had a trim prevented — 40 characters' worth in the test above — and was silently absent from the field that exists to make the guarantee observable. The condition was already right; it was in the wrong branch. Now recorded on both paths: a drop prevented outright, and a truncation capped higher than the share alone would have allowed. Parity is unaffected — prompt-budget never sets flexReserve, so the branch is unreachable from every corpus path, and all 50 cases still match. Refs #2929 * chore(#2929): backfill changeset PR number (#2958) * chore(#2929): correct the corpus case count in the changeset fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
5a0a9f0972 |
fix(#2944): remove the catastrophic-backtracking regex from the ADR-1671 example (#2950)
* fix(#2944): remove the catastrophic-backtracking regex from the example The non-shipping Option-E reference example carried its own copy of the predicate-id regex, which nested a dot-containing character class inside a dot-prefixed repeat. A run of N consecutive dots therefore had exponentially many partitions. Measured on next before this change: 30 dots 54ms, 35 66ms, 40 807ms — so roughly 55-60 dots hangs for hours. Not exploitable where it sits: the example is outside tsconfig.build.json, outside the npm package files list, outside the installer and outside tests, so no build step or CI job parses anything with it. Fixed because the entire point of a reference example is that people copy it forward, and ADR-1671 presents this one as the pattern for the platform. Ports the linear per-segment validation that #2928 gave the production module, so the two copies agree: both parse the real CONTEXT.md to 415 predicates across 20 classes with 0 duplicates. Doubled-dot ids are now rejected here too, matching production, and the grammar comment records it. Also refreshes the example's committed index, which #2928 made stale when it removed the duplicate predicate from CONTEXT.md. Closes #2944 * test(#2944): guard predicate-index sync and example/production parity Two regression tests for the two defects in this PR. Index sync: asserts the committed docs/CONTEXT-INDEX.json equals a fresh parse of CONTEXT.md, naming any diverging predicate ids. The merge race that reddened next was invisible to both PRs involved and only surfaced on the next PR to run lint:ci; this puts the same check inside the suite, which runs on every PR, and a mutation test proves the assertion is not vacuous. Example/production parity: asserts both copies of the parser report the same count, classes and duplicates for the real CONTEXT.md, and agree verdict-for- verdict over a table of id shapes. The divergence WAS the bug — production went linear-time while the example kept the backtracking regex, with nothing asserting they agreed. Also pins the example rejecting a 60-dot id, with the clean rejection as the binding assertion and wall-clock only as a smoke check. Notes a real tension rather than hiding it: ADR-1671 says the example sits outside tests/, and this imports it. The ADR's intent is that the example is not compiled, packaged or installed — not that it may silently rot. A parity guard does not ship it. The file states this so a reviewer can object. * fix(#2944): address both isolated review passes Two independent reviewers (correctness and security axes, neither the author). Security found nothing — it measured linearity to 100k chars across dots, hyphens, underscores and mixed classes, and showed prototype pollution is structurally unreachable because the first-segment pattern forbids lowercase and underscore-leading ids. The correctness pass found three blockers, all real. Blocker: the parity test violated ADR-1671 verbatim. The ADR lists FOUR exclusions for the reference example, the fourth being the CI test suite, and the test imported it from tests/ while its own justification comment cited only three -- constructing a rationale around the exclusion it broke. Moved to scripts/lint-example-parser-parity.cjs wired into lint:ci; a lint script is not the test suite, so the exclusion stands. The test file keeps only the docs/CONTEXT-INDEX.json sync check. Blocker: the mutation test leaked its temp dir. Its callback took no `t`, so a failing assertion skipped the bare cleanup call. Now registered via t.after(), matching the convention adr-index-gate.test.cjs documents. Blocker: the example's own committed index carries the identical merge-race staleness this PR fixes for the production one, and nothing guarded it. Deliberately NOT fixed by wiring the example's --check into CI: that artifact bakes line numbers, so it re-drifts on any unrelated CONTEXT.md line shift -- exactly ADR-1671 open question 4 -- and would make CI routinely red. The new lint asserts the line-INDEPENDENT facts instead: count, class map, duplicate set, and every (id, value) pair. Proven non-vacuous both ways: mutating a value fails and names the id, mutating only a line number passes. Major: a real divergence the parity claim would have missed. Production rejects values containing an embedded CR, LF, U+2028 or U+2029; the example did not, so a value with an embedded lone CR was rejected by one copy and accepted by the other. Ported, and now covered by the parity table. Also, found while verifying rather than reported: malformed diagnostics covered only empty values. A doubled-dot id, a space in an id, and a lowercase-leading id were all dropped silently. That contradicts the module's own intent -- a typo should be diagnosable, and a space in an id is a likely one -- and predicates are contractually cited, so a silently vanished predicate is the failure mode that matters. Each rejection class now carries a named reason in both copies, while ordinary inline code still yields none. Trues up counts my own change staled: the example README and ADR-1671's prototype figures said 416 and 393/18 against a real 415/20/0. Closes #2944 * chore(#2944): backfill changeset PR number 2950 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
05b170e448 |
chore(#2928): productionize the CONTEXT.md predicate fact-store and gate it in CI (#2938)
* feat(#2928): port CONTEXT.md predicate fact-store into the src seam Productionizes the ADR-1671 Option-E reference example as a real module: src/context-predicates.cts (parser + selector + index builder) compiled to gsd-core/bin/lib/, plus scripts/gen-context-index.cjs following the repo's --check/--write drift-guard idiom and wired into lint:generated-sync. Parser behavior is deliberately prototype-equivalent in this commit so the next commit's regression matrix binds to the real defects rather than to a missing module. Two locked design deviations from the prototype: - duplicates carry a count, not line numbers - the committed index carries no line field at all, resolving ADR-1671 open question 4: an artifact without line numbers cannot drift on a line shift, so promoting --check to a CI gate does not make it routinely red Also reconciles the one remaining duplicate predicate ID (RULESET.WORKFLOW_MARKDOWN.FENCES was declared twice; the non-MD040 wording is removed) so the gate can land fail-closed on duplicates. Refs #1671 * test(#2928): failing-first matrix for the predicate fact-store Adds the regression matrix from the phase test plan: parser declaration forms, fence and comment regions, ID/value grammar boundaries at limit-1/limit/limit+1, CRLF fidelity, duplicate detection, the drift-guard CLI, the selector query surface, and four document-shaped fast-check properties. Seven rows are RED for behavioral reasons against the ported parser: indented-bare, star-list, plus-list and numbered-list declaration forms are dropped; a tilde fence and a four-backtick fence containing a shorter fence are not skipped; and a multi-line HTML comment is parsed as live. Eleven selector rows are RED because the query surface is not wired yet. Negative fixtures come from real repo documents that predate the grammar (CONTEXT.md, CONTRIBUTING.md's fenced env-assignment examples) per the fixture-provenance rule, and the property generators are document-shaped rather than seeded from our own serializer. Refs #1671 * fix(#2928): consume the shared fence scanner, relocate the index, wire the selector Drives the failing-first matrix green. Parser: replaces the ported naive triple-backtick toggle with the shared markdown-sectionizer fence engine. scanFencedBlocks and FencedBlockRecord gain an export keyword — the only change to that module, which has 71 upstream dependents — because it already returns line-indexed spans, which is exactly what a line-reporting parser needs. It also already documents itself as the second copy of the fence state machine pending consolidation; adding a third copy here would have been the generative-fix divergence this repo warns about. A parity suite now pins predicate fence-skipping against that scanner across eight fence shapes. HTML-comment skipping stays local because the sectionizer has no comment scanner. Declaration forms widen to indented-bare, star, plus and numbered list items. Index location: docs/CONTEXT-INDEX.json, not a module under bin/lib. The remote matrix run caught the original choice — a committed .cjs there ships ~120KB of CONTEXT.md prose into a runtime module, and two content guards fired truthfully on it (a leaked .claude install path, and four hardcoded package-name literals). Neither guard was allowlisted; the artifact moved instead, mirroring docs/INVENTORY-MANIFEST.json. Nothing at runtime needs to require it — it is a drift-detection artifact, so the selector parses CONTEXT.md live and is always current. Generator: adds a frozen REASON enum and --check --json so the gate's outcome is asserted structurally instead of by matching prose, and --context-path/--index-path so tests drive the real CLI against a temp tree with no filesystem monkeypatching. Selector: gsd_run query context-predicates with --class/--prefix/--contains, structured output carrying a matched count, own-property guards, and no project-root resolution. Registering it exposed that the query dispatch table and the usage string had drifted: a new parity test found 20 routed commands missing from the usage list, all added here rather than deferred. Refs #1671 * test(#2928): lock the newly-public scanFencedBlocks contract Exporting scanFencedBlocks made it public API for the first time, so it needs its own contract test independent of the consumer that motivated the export. Memtrace's co-change analysis flagged the gap: this suite changes together with markdown-sectionizer.cts 8 times in 90 days and was absent from the diff. Covers the documented rules: 0-based indices, -1 for an unterminated fence, the same-char/>=length/no-trailing-text closer rule, a shorter fence inside a longer one staying content, CommonMark 4.5 backtick-in-info-string, and <=3-space indent tolerance. Refs #1671 * fix(#2928): address both isolated review passes Two independent reviewers (correctness axis and security axis, neither the author) found seven findings. All are fixed here with regression tests; none deferred. BLOCKER — comment-blind fence scanning caused silent, permanent predicate loss. The HTML-comment scan and the fence scan ran as two independent passes, and the fence scanner is comment-blind, so a fence delimiter inside an HTML comment with no later close read as an unterminated fence and skipped every remaining line to EOF. Worse, the drift-guard could not catch it: it diffs against a baseline produced by the same corrupted parse. The two constructs now interleave in a single pass so each suppresses the other's boundary detection while active, covered in both directions. The parity suite still binds this scanner to markdown-sectionizer's for comment-free documents, so the two cannot diverge unnoticed. BLOCKER — the selector was not consumed anywhere, leaving the phase's acceptance criterion unmet. Now wired into the pre-work predicate-citation step in contributor-standards, which is the repo's actual brief-assembly path; no code-level brief assembler exists to wire into. MAJOR — ReDoS with an unauthenticated CI-hang exploit. The predicate-id regex nested a dot-containing character class inside a dot-prefixed repeat, so N consecutive dots had exponentially many partitions: 40 dots took 565ms and growth was exponential. CI runs this parser over a pull request's own CONTEXT.md, so any contributor could have hung a shared runner with one line. Replaced with linear per-segment validation. Doubled-dot ids are now rejected; the real document contains none. MAJOR — the duplicate-id gate had only ever been proven on synthetic fixtures. A test now re-inserts the exact line this branch removed and asserts the real generator names it. MAJOR — --check together with --write silently let write win, turning the gate into a writer; a missing path value resolved to the cwd and leaked an EISDIR stack trace. Both are now clean usage errors. MINOR — the hoisted skip-list was exported as a live mutable Set; replaced with a read-only predicate. MINOR — flag-shaped selector values were unmatchable; the inline --flag=value form now provides the escape hatch. Refs #1671 * chore(#2928): backfill changeset PR number 2938 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
81eeb8a53a |
docs(#2926): refresh ADR-1671 with findings re-verified on next (#2936)
Re-measured the Option-E prototype's reported figures against CONTEXT.md on next (2026-07-31) and recorded the delta rather than overwriting the June numbers: - index counts 393/18 (2026-06-24) -> 416/20 today; CONTEXT.md gained the PROBE (11) and PROHIB (10) classes - of the 3 duplicate predicate IDs, only RULESET.WORKFLOW_MARKDOWN.FENCES remains; the two RULESET.GEMINI.* went with the Gemini runtime removal - gen-context-index.cjs --check exits 1 on next, so Phase 0's "--check green in CI" criterion is unmet (invisible to CI: the example sits outside tests/) Adds Open question 4 (index keyed on baked line numbers re-drifts on any CONTEXT.md line shift, which matters once Phase 1 promotes --check to a CI gate), and records the reviewer-proposed eval-gate question as resolved by the PROBE.*/PROHIB.* predicate classes (ADR-550 D4/D7, ADR-1606). Also names both surfaces of the Windsurf 12 KB throw in Decision 2, since it is duplicated byte-identically in bin/install.js and src/runtime-artifact-conversion.cts. Docs-only. No code, no runtime-loaded text, no behavior change. Co-authored-by: sim <sim@local> |
||
|
Rezolv
|
2f66788cd3 |
docs(#2619): add ADR-2619 observability and shareable diagnostics (#2862)
Completes ADR-0174 §6's observability rollout and adds the outbound trust boundary that ADR-1577's inbound boundary has no counterpart for. D1 (wire the seam behind the existing opt-in gate) shipped via #2620 / PR #2621. D1b records that the unconditional stderr-on-error rule at 0174:105 is the target state, deferred behind an explicit --json-errors envelope version plus migration note -- disclosed as a partial supersede rather than retconned. D2-D5 are Directional; D6's non-goals are binding. Regenerates docs/adr/README.md via scripts/gen-adr-index.cjs --write. Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |