ddeb141bd28314d130ff81223828576104f2719b
539 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
9410f7e6e6 |
enhance(#3897): ADR-3473 §8.3 rungs 2-4 — runtime marker, derived Codex sandbox, short-form depends_on (#3941)
* test(#3897): failing-first coverage for §8.3 rungs 2-4 ADR-3473 §8.3 has four rungs; #3883/PR #3896 shipped the first. This pins the other three RED before any fix. Rung 2 — the install marker has four readers and resolveRuntime is not one. resolveRuntime resolves GSD_RUNTIME > config.runtime > 'claude' and reads no marker at all, while bin/install.js writes one (#2297) and FOUR hand-rolled readInstallRuntimeMarker copies exist: src/model-resolver.cts:65 (cached, with test seams), hooks/gsd-agent-isolation-guard.js:112, and TWICE in hooks/gsd-cursor-subagent-start.js at :346 and :355. Four copies of one rule. Fixtures and seam names mined from PR #3382 rather than re-derived; it implemented this rung and was closed "not on the merits". Rung 3 — the sandbox map, and the fallback that was the real defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: - all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements — the map carries nothing the contract does not - 24 roles fall through `|| 'read-only'`, of which 16 declare Write or Edit So the map is redundant and the silent fallback is the defect. The maintainer chose to derive but hold those 16 at read-only pending the question of whether Codex enforces sandbox_mode or merely advises; HALT.md records it. T20 asserts the emitted sandbox_mode PER ROLE against a captured baseline, not in aggregate — an aggregate passes while one role silently widens, which is the proxy-instead-of-identity shape this repo names. T24 and T25 fail on a stale hold, so the hold list cannot rot into the subset map being deleted. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: `git log -S shortFormToId` returns only documentation commits. That was the wrong instrument. Direct inspection of sdk/src/query/phase.ts at 11918dcc3^ shows five occurrences, and the tests match that code rather than a guess at its semantics — including first-write-wins on a duplicate short form. T43 asserts at the consumer's output: the emitted `waves` map from the real CLI, which pre-fix collapses to {"1":[...]} because every short-form edge is dropped. A unit assertion on resolveDependencyId would have passed throughout this defect's life. Observed RED, this tree: rung 2 11/11 fail — no marker rung, no seams rung 3 T23,T24,T25,T26,T30 fail; T28 fails (validate agents passes a TOML whose sandbox_mode disagrees — it checks presence only) rung 4 T42,T44 fail; T43,T49 fail with waves collapsed to a single wave 1 Green and staying green: T20/T21/T22/T27 as captured baselines, #3885's unresolvable-token warning and wave-verdict suppression, and #3785's display-mapping passthrough. If the third tier over-reaches, those go red — that is their job. Disclosed weakness: T45 (a canonical id with no dash is not short-form indexed) cannot be isolated behaviorally, because planMap always masks it. It is a non-crash boundary pin, weaker than the other rows, and is recorded as such rather than presented as equivalent. Design: .gsd/phase/feat-3897-adr3473-83-rungs/40-design.md Test matrix: .gsd/phase/feat-3897-adr3473-83-rungs/50-test-matrix.md Decision: .gsd/phase/feat-3897-adr3473-83-rungs/HALT.md Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3897): §8.3 rungs 2-4 — one marker reader, a derived sandbox, the third depends_on tier ADR-3473 §8.3 has four rungs. #3883/PR #3896 shipped the first. These are the other three. Rung 2 — the install marker had four readers, and resolveRuntime was not one. resolveRuntime resolved GSD_RUNTIME > config.runtime > 'claude' and read no marker, while bin/install.js writes one (#2297) and four hand-rolled readInstallRuntimeMarker copies existed: src/model-resolver.cts (cached, with seams), hooks/gsd-agent-isolation-guard.js, and twice in hooks/gsd-cursor-subagent-start.js. model-resolver's was already the house idiom, so it was promoted rather than replaced: src/runtime-slash.cts now owns it, and model-resolver plus both hooks delegate. The hooks reach it through ensureRuntimeBuild(), the seam lint-hooks-runtime-build-seam enforces. No import cycle existed - checked both directions before moving anything. The marker is the THIRD rung: env > project config > marker > 'claude'. N1 was checked rather than assumed, and my first reading of it was wrong. A marker holding an unknown name comes back essentially verbatim, which looked like a validation gap. Measured against the env rung with the same inputs - including "../../etc/passwd" and "claude;rm -rf /" - the two are identical, because they share resolveRuntimeNameFromCandidates. N1 asks for exactly that, and it is met. The residual (the shared normalizer normalizes shape, it does not validate against the known-runtime set) is pre-existing on the env rung and plausibly deliberate, since a new runtime should not need a code change. The marker also does not widen the trust boundary in any real sense: it lives inside the install tree beside the code, so anyone who can write it can write runtime-slash.cjs itself. Rung 3 — the map was redundant; the silent fallback was the defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements. The map carried nothing the contract did not already have, so it is DELETED rather than reconciled. What was actually broken is `|| 'read-only'`, which silently under-granted 24 of 35 roles. 16 of those 24 declare Write or Edit and would widen under derivation. Per the maintainer's decision (HALT.md), they are held at read-only pending the question of whether Codex enforces sandbox_mode or merely advises. Emitted TOML is therefore byte-identical for all 35 roles - asserted per role, not in aggregate, because an aggregate passes while one role silently widens. The hold list self-invalidates. A hold whose role no longer derives broader fails, and so does a hold naming a role with no agents/<name>.md. Without that it would rot into exactly the hand-maintained subset map being deleted, and this commit's own ledger claim would become false over time. Both cases were proved by injecting them and watching them throw. Two committed tests asserted the deleted map's existence and contents. They were pinning the thing being removed, so the tests moved rather than the production code: the 11 role-value pairs survive as a test-local PRE_3897_CODEX_AGENT_SANDBOX baseline, and the assertions now drive the real derivation against real agents/*.md. The coverage is preserved; only its source moved out of production code. validate agents gains checkCodexSandboxPosture, mirroring the existing checkCodexModelPosture: each installed TOML's sandbox_mode must equal the role's expected value, failing with role, expected and found. It previously checked file presence and manifest completeness only, so a TOML whose sandbox_mode disagreed passed. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: git log -S returns only documentation commits. Wrong instrument. sdk/src/query/phase.ts at 11918dcc3^ carries five occurrences, and the implementation here matches that code rather than a guess at its semantics - including first-write-wins on a duplicate short form, deterministic from the sorted plan order. It resolves the bare plan number: depends_on: ["01"] now reaches 26-01-auth-hardening. That is a control-flow change, not a diagnostic one - plans that silently collapsed into a single wave 1 now execute in their declared waves, and execute-phase.md consumes those wave values. In-phase only, by construction: the map is built from this phase's rawPlans, so a same-named short form in another phase does not resolve. #3785's display-mapping passthrough and #3885's unresolvable-token warning and wave-verdict suppression are untouched and stay green. If the third tier had over-reached, those are what would have caught it. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open I introduced, and wire the posture check to its command Two blockers from review. Both are mine, and one is a security regression my own change created. 1. A held role could escape its hold by editing its own frontmatter. The Codex install loop set the sandbox identity from the agent's frontmatter `name:` field rather than from its filename, so the hold lookup keyed off a value the file itself declares: deriveCodexSandboxMode('gsd-doc-writer', <real file>) -> read-only deriveCodexSandboxMode('gsd-doc-writer-x', <same file, name: edited>) -> workspace-write deriveCodexSandboxMode('GSD-Doc-Writer', <same file, name: recased>) -> workspace-write What makes this a blocker rather than a nit is the DIRECTION. The deleted CODEX_AGENT_SANDBOX map had the identical lookup-key quirk, but it was an allowlist: an unmatched key fell back to read-only, which is safe. The new scheme derives workspace-write from the tool contract and uses the hold as a subtraction, so the same mismatch fails OPEN. I converted a fail-closed quirk into a fail-open one and did not notice; the isolated reviewer proved it by execution. Neither safety net caught it. validateCodexSandboxHolds only checks that <key>.md exists, never that a file's derived identity matches its key. checkCodexSandboxPosture looks the canonical source up by the installed TOML's filename, finds nothing for a renamed agent, and treats it as a custom non-roster agent — silently no violation. The identity is now the FILENAME STEM, which is what validateCodexSandboxHolds already validates and what an attacker editing frontmatter cannot change without renaming the file — at which point the existing validator catches it. The lookup is case-insensitive so a recase does not slip past either. The frontmatter name still drives the TOML body and filename, unchanged; only the sandbox identity moved. All 35 roster files were checked: name matches filename stem everywhere, so a stricter "they must agree or throw" invariant would have been safe against real content. It is deliberately NOT added — it would abort an install on a tampered file where emitting a correctly-derived read-only TOML is the safer outcome. Recorded as a fork rather than decided silently. 2. checkCodexSandboxPosture was exported and never called. cmdValidateAgents (src/verify.cts) called checkAgentsInstalled and checkCodexModelPosture only; grep for the sandbox check in that file returned nothing. So criterion 3 — "validate agents fails on semantic drift, not only on missing files" — was unmet, and `validate agents` behaved exactly as before. That is ADR-3473 Decision 2's named shape: a declared policy with no executor. It also meant the T28 test asserted at the helper's return value while the COMMAND stayed broken — the ADR-3180 Decision 4(b) failure this epic exists to close, committed by me while enforcing it elsewhere in the same epic. Now wired as an additive `sandbox_posture` field beside `codex_posture`, following the sibling precedent exactly. Drift is report-only, not a non-zero exit, because that is what checkCodexModelPosture does — two sibling posture checks disagreeing about whether a violation is fatal would be its own defect. The choice is recorded in a comment rather than left implicit. A consumer-output test now drives the real CLI and asserts on the emitted JSON, and was shown failing before the wiring and passing after. Also corrected a stale artifact: the design's Known limit L1 still claimed rung 3 was not in this deliverable, written while it was halted and false once the maintainer unblocked it. Verified after both fixes: the three bypass probes all return read-only, the per-role table is 35/35 byte-identical, and both hold self-invalidation cases still throw. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the marker rung, the derived sandbox, and the bare plan-number depends_on Reference: the runtime precedence ladder in docs/CLI-TOOLS.md gains the install marker rung; docs/COMMANDS.md documents validate agents' new sandbox_posture field; docs/reference/plan-md.md documents that depends_on accepts the bare plan number. Explanation: a docs/features fragment keyed id 3897, so it cannot collide with a concurrent PR hand-allocating a section number, regenerated into FEATURES.md. ADR-3473 §8.3 gains an ANSWER blockquote in the document's own correction style, recording what was measured and built against the section's 2026-08-26 correction - including the qualification that checkAgentsInstalled itself still checks presence only, and the semantic assertion lives in a sibling wired into validate agents rather than folded into it. No how-to. Both user-visible changes are zero-step: a non-Claude install resolving its own runtime, and plans executing in their declared waves, both happen without the user doing anything. docs/how-to/control-the-reported-host-runtime.md covers a DIFFERENT ladder (resolveReportedRuntime / agent_runtime) that this change does not touch, and was deliberately left alone rather than edited by association. No tutorial - nothing multi-step to walk through. docs/AGENTS.md unchanged: it documents Claude-side tools frontmatter, never Codex sandbox_mode, and the emitted tools contract did not change. The prompt layer documents depends_on only by example, not by schema, so nothing there needed editing - and few-shot-examples/plan-checker.md already showed depends_on: ['01'], which now actually resolves. Translated copies of plan-md.md are untouched; the project treats translations as community-maintained. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): move the sandbox derivation out of the installer, off the install path, and off a third parser The full suite came back with 26 failures across four files. Three distinct causes, mapped individually rather than assuming the first explained the rest. A. Requiring bin/install.js printed the GSD banner to stdout and corrupted `validate agents` JSON. Unexpected token '', "[36m ██"... is not valid JSON checkCodexSandboxPosture reached deriveCodexSandboxMode by lazily requiring bin/install.js, whose module load prints the ASCII banner. So the command emitted banner bytes before its JSON and every JSON consumer broke, including ten tests that predate this branch. src/ reaching into bin/ was backwards layering that happened to also be loud. The derivation now lives in src/codex-agent-toml.cts - the existing Codex TOML domain module, no new module and no six-gate ripple - and both bin/install.js and src/agent-install-check.cts import it. One owner, which is §8.3's rule applied to the fix for §8.3. B. The stale-hold throw fired on a legitimate partial source dir, and masked a security assertion. validateCodexSandboxHolds treated "this hold's .md is absent from the install SOURCE dir" as a stale hold and threw. A test fixture, or any partial install source, legitimately contains a couple of agents. Worse, it threw BEFORE the path-escape check, so a test asserting that a `../../evil` frontmatter name is rejected got my unrelated error instead of the traversal rejection it was written for. A fail-closed check of mine was hiding a real security check. The "no stale holds, shrink-only" invariant is a property of the repo's canonical agents/ roster, not of whatever directory an install happens to read. It is off the runtime path and enforced where it belongs, in the tests that already existed for it. A partial source dir now installs cleanly, and the evil-name case throws with its own escapes-configHome message again. C. T8 depended on ambient process.env state. The marker/env parity assertion round-tripped through live process.env. It now compares against resolveExplicitRuntime's already-exported dependency-injection parameter - deterministic and hermetic, same claim. Proven still falsifiable rather than assumed: with the marker rung's normalization temporarily bypassed the two rungs diverge ("codex\n../../etc/passwd" vs "codex-../../etc/passwd") and the assertion fails, then passes again once reverted. One correction folded in along the way. The first version of the move added private _extractFrontmatterAndBody/_extractFrontmatterField helpers to codex-agent-toml.cts - a THIRD copy of frontmatter extraction, where the graph already shows two (bin/install.js:2348, runtime-artifact-conversion.cts:893). Adding a third inside the epic whose thesis is one implementation per rule is not defensible. deriveCodexSandboxMode no longer parses anything: it takes (identity, toolsValue) and each caller supplies the tools value using the extractor it already has. Both helpers are deleted. The identity argument is still the filename stem, so the fail-open fix is untouched. Verified after all three: `validate agents --raw` emits parseable JSON with no banner and both posture fields; the four hold-bypass probes still return read-only; the per-role table is 35/35 byte-identical at 26 read-only / 9 workspace-write; the hold list is still 16. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): drop a dev-only transitive dep, make the derivation total, retire a stale fallback test Suite down to 7 failures from 26. Three more causes, mapped individually. A. My extractor import dragged in a script that does not exist in an installed tree. Cannot find module '../../../scripts/fix-slash-commands.cjs' Chain: src/agent-install-check.cts imported runtime-artifact-conversion.cjs, which requires command-roster.cjs, whose line 36 requires ../../../scripts/fix-slash-commands.cjs. That path exists in the repo and not in an install, so every test exercising a synthetic install dir died at module load. I picked that extractor for convenience without checking what it pulls in - the same mistake that produced the banner bug, one layer further out. agent-install-check now uses a single-purpose extractToolsLine on codex-agent-toml.cts. That is deliberately NOT a general frontmatter parser: we deleted those helpers a commit ago for good reason, and this reads one line. Verified from outside the repo root that requiring either module prints nothing and does not throw. B. A test pinned the deleted name-based fallback. 'defaults unknown agents to read-only' called generateCodexAgentToml with a fixture declaring tools: Read, Write, Edit. Under derivation an unknown agent with a writing contract correctly derives workspace-write - design row S6, a new writing role gets the contract, not the pin. The behavior it asserted was the silent fallback this rung deleted; identity no longer decides the sandbox. Replaced with two rows rather than a flipped string: no tools declared -> read-only (absence is not a grant), and Write/Edit declared -> workspace-write. Strictly more coverage than the row it replaces. C. The stale-hold check still threw per derivation call. Last commit took the roster-existence check off the install path, but deriveCodexSandboxMode itself still threw when a hold's role did not derive broader FOR THE CONTENT IT WAS HANDED - so it fired on any synthetic fixture for a held role. The throw is gone, and it cost nothing: if a held role's content does not derive broader, the hold pins read-only and derivation returns read-only anyway, so the hold is a no-op and there is nothing to fail about. The staleness invariant is a property of the real agents/ roster, and validateCodexSandboxHolds still enforces it there - confirmed against the real roster after the change, not assumed. deriveCodexSandboxMode is now total: every (identity, toolsValue) including undefined and null returns read-only or workspace-write, never throws. Verified: validate agents emits parseable JSON; the four hold-bypass probes return read-only; the per-role table is 35/35 at 26 read-only / 9 workspace-write. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): put the rung-3 decision in the shipped docs instead of pointing at an ignored path The ADR entry and the feature fragment both ended their rung-3 explanation with "see .gsd/phase/feat-3897-adr3473-83-rungs/45-decision-rung3-sandbox.md". That directory is gitignored (.gitignore:55), so the rationale for holding 16 roles at read-only was reachable only from the machine that produced it. A reader of the ADR got a pointer to nothing. Both now carry the reasoning inline: the criterion asks both that the sandbox derive from the declared tool contract and that no role gain a broader sandbox, and those cannot both hold, because a faithful derivation widens 16 roles the deleted map never listed and that fell through its silent read-only default. The resolution is derive-and-hold - the derivation owns the rule now, each hold is released as its enforcement question is answered, and a hold is reversible where a widened sandbox that turns out to be enforced is not. Checked before assuming this was a defect class: CONTEXT.md cites .gsd/phase/<slug>/40-design.md as its standard Design: provenance line in eight module entries, and four other shipped docs do the same. Citing a phase artifact is an established convention here, so those are left alone. What was wrong was specific to these two: they put load-bearing rationale behind the pointer instead of provenance. docs/FEATURES.md regenerated from the fragment via scripts/gen-features.cjs rather than hand-edited. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open, stop a silent mis-resolution, and read a declaration as a declaration Two orthogonal reviews on the shipped sha. Three of the findings are the same failure class this epic exists to close, committed inside it. 1. BLOCKER - the sandbox was decided for one identity and applied to another. bin/install.js derived sandbox_mode for the filename stem and then wrote the result to `${name}.toml`, where name comes from the file's own frontmatter. Make the two disagree and a HELD role's artifact goes wide: rename gsd-doc-writer.md -> gsd-doc-writer-v2.md, keep name: gsd-doc-writer -> stem is unheld, derives workspace-write, lands on gsd-doc-writer.toml add any gsd-*.md whose frontmatter name: is a held role -> clobbers that role's toml with workspace-write Both emit read-only on origin/next, because the deleted map was an allowlist and a miss fell back safe. This is a regression my change introduced. The previous review round moved the HOLD KEY off frontmatter to the filename stem and left the OUTPUT PATH on frontmatter; my own comment at install.js:6985 calls that value attacker-editable, four lines above the line that uses it as the filename. The decision is now made over BOTH candidate identities, most-restrictive wins: if either the stem or the emitted name is held, the mode is read-only. 2. MAJOR - hold matching was toLowerCase() only, so confusables escaped. Turkish dotted/dotless i, fullwidth, NFD, trailing space/NBSP/dot/newline, ./ and ../agents/ all slipped the hold and emitted workspace-write. Identities are now basenamed, trimmed of NBSP/zero-width/control characters, NFKC-normalized and lowercased - and anything still carrying a character outside [a-z0-9._-] is treated as suspicious and derives read-only. We do not enumerate confusables; every shipped roster file is ASCII, so refusing to widen on an identity we cannot recognize is fail-closed with no false positives on real content. 3. MAJOR - the short-form depends_on tier mis-resolved SILENTLY. shortFormToId keyed on the last dash-segment of any canonical id with no constraint that it is a plan number, so a phase holding 09-FIX-auth-PLAN.md made depends_on: ["auth"] bind at wave 2 with zero warnings. This is the worst shape in the epic: the unresolvable-token warning fires on a DROPPED token, so a MIS-RESOLVED one is invisible and the tool reports a confident wave assignment built from a wrong edge. A wrong edge is worse than a missing one. The segment must now match /^\d+$/, which is exactly the contract docs/reference/plan-md.md already documents. This tier was recovered verbatim from the retired SDK lineage, which carried the same defect; we are deliberately NOT preserving it bug-for-bug, and the comment says so, so the next reader does not "restore" it. 4. MAJOR - the derivation was reading a declaration as an absence. extractToolsLine read one line, so a YAML list-form tools: block returned only its first item. Two roster files use list form, and gsd-nyquist-auditor declares Write and Edit there - parsed as "- Read", found no write tool, and emitted read-only. Rung 3's headline claim is that sandbox_mode derives from the declared tool contract; that claim was false for 2 of 35 roles and materially wrong for 1. Reading a declaration as an absence is the silent-drop class this epic exists to close. Renamed extractToolsValue and taught it both shapes. gsd-nyquist-auditor now derives workspace-write and joins CODEX_SANDBOX_HOLDS as its 17th entry, per the standing derive-and-hold decision - so emitted TOML stays byte-identical at 26 read-only / 9 workspace-write while the hold list finally records every role that would widen. A previous pass declined this fix because it moved the count; that inverts the priority. Byte-identity is preserved THROUGH the hold, not by leaving a parser broken. Divergence check, because this is where that bug hides: both paths feeding sandbox derivation - install.js's emitter and checkCodexSandboxPosture - now route through the one extractor. The tools readers in runtime-artifact-conversion and install.js's other frontmatter call sites serve Claude-side emission and do not feed sandbox derivation. Also fixed, each real: the posture check's `found` used a naive whole-file regex where its own sibling uses the block-aware scanner, so prose inside developer_instructions produced a false violation; `found` skipped truncatePostureValue and leaked a 300-char value into validate agents output; deriveCodexSandboxMode's absolute never-throws claim was false for an object with a throwing toString; T49 could not falsify cross-phase leakage (its target phase had its own 01, so a globally-scoped map passed too); T20/N6 iterated a hardcoded table and pinned the FIXTURE size, so a 36th agent would be silently unchecked; three tests reimplemented the code they were testing instead of importing it; and T2-T4 deleted GSD_RUNTIME without restoring it. Verified: hold list 17, gsd-nyquist-auditor derives workspace-write unheld and emits read-only held, roster 35/35 at 26/9, depends_on ["auth"] no longer resolves while ["01"] still does, both identity-bypass cases and every confusable vector emit read-only. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the hold list is 17, and the reason the 17th was missing The count read 16 because the derivation could not read the declaration it claimed to derive from: the tools reader was single-line, so a YAML list-form tools: block returned only its first item and gsd-nyquist-auditor's declared Write and Edit were read as an absence. Both the ADR entry and the feature fragment now carry the corrected count and the reason for it, rather than a silently updated number. Deriving from a declaration you cannot parse is not deriving, and a flattering count is worse than a wrong one because it looks settled. docs/FEATURES.md regenerated from the fragment. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3897): backfill changeset pr number Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c5f2b94b27 |
enhance(#3907): gates report no-input instead of a verdict they never reached (#3932)
* feat(#3907): gates report no-input instead of asserting a verdict they never reached The three stdin-reading gates bound 2 to a stdin read error only, with no arm for stdin closed at zero bytes - so empty input flowed into the detector, found nothing, and exited 1, which each module's own comment defines as a negative verdict. An unset PHASE_SECTION made the UI gate assert the phase has no UI. Empty and whitespace-only input now exit NO_INPUT, and a read error exits UNAVAILABLE rather than a locally-invented 2, both resolved through the registry and delivered by terminateNow. The exit code was only half of it: under --json the same input emitted {detected:false}, byte-identical to the fabricated payload #3909 exists to fix, and the blocking coverage gate reads that payload. Empty input now emits the in-tree {skipped:true,reason} form with no detected key at all. teams-status is excluded: it never reads stdin and has no invented 2, so the four-module framing in the issue and ADR is wrong. The dead root bin/lib/ui-safety-gate.cjs is deleted - no installer reference, no workflow invocation, and the live fallback chains are for other modules. Its removal restores the unit tests to the module that actually ships; they had been asserting the stale copy's two-field shape, which is why it drifted unnoticed. * fix(#3907): drive gate tests through the process seam, and make removed-but-needed basename-precise CONTRIBUTING requires every subprocess go through tests/helpers/process-seam.cjs; two of the three gate suites hand-rolled spawnSync while the third, added in the same change, used runNode correctly for the identical injection case. Converted the blocks this change added, leaving pre-existing ones alone. Deleting one of two files sharing a basename made lint-removed-but-needed report 14 references that were all to the surviving canonical module - the false-positive class its own docstring names. It now matches on the deleted file's full path when a surviving file shares its basename, which is more precise rather than weaker: a genuine full-path reference still fails, and behaviour is unchanged when no basename collides. It immediately caught a docstring on this branch that spelled the deleted path. * test(#3907): update the one existing assertion that pinned the old empty-stdin verdict A pre-existing test asserted exit 1 on empty stdin - the defect this phase removes - and was missed because the change added new blocks without auditing existing ones pinning the old contract. Audited the rest: the other three status-1 assertions in that file all feed real input and are the genuine-negative controls that must keep returning 1, so exactly one was stale. The retired 2 is gone from the describe's contract comment too. * chore(#3907): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
941b62249e |
enhance(#3906): two terminators over one registry, with a versioned exit projection (#3924)
* feat(#3906): two terminators over one registry, with a versioned projection Adds terminateNow (write-then-terminate, for callers that cannot wait for the event loop) beside runMain (drain-then-exit), both projecting through one shared function so they cannot disagree - the parity the ADR makes mandatory. A failed write does not change the exit code: letting it propagate would fail a hook open, which is what the fail-closed branches exist to prevent. The projection is versioned. v1 reproduces today's integers, including keeping a payload-carried degraded result at exit 0 - ADR-2980 ratified that across 60 sites and declined normalizing it on measured blast radius. v2 applies the registry. --exit-contract=v2 or GSD_EXIT_CONTRACT=v2 selects it; an unrecognized version throws rather than silently defaulting. The registry is now emitted beside both copies of the exit module, so it resolves as a sibling in the built tree and in the committed scripts/ copy that must load on an unbuilt clone. * fix(#3906): actually restrict code 2 to terminateNow, and generate the registry's type The claim that terminateNow is the only place 2 can be produced was false: runMain's outcome arm applied no guard, so runMain(()=>'HOOK_DENY') set exitCode 2 through the drain path - and the parity matrix demonstrated it while calling it parity. runMain now refuses any outcome projecting to the hook-protocol code, gated on the code rather than the name so an alias cannot slip past, and the matrix asserts the restriction instead of contradicting it. The ambient type for the generated registry was hand-written with no gate against the generator's actual output - the declared-surface-diverges-from-runtime defect class this epic exists to close, reintroduced inside it. It is now a third generated artifact covered by the same --check. Also converts every test-body try/finally to t.after(). * test(#3906): derive the glossary fixture's dependencies instead of hand-listing them Adding a require to scripts/lib/cli-exit.cjs broke 31 tests in one suite that built its fixture from a hand-written dependency list, so the new sibling was absent and the copied script could not load. copyScriptWithDeps walks the require graph and exists for exactly this class - #3412 paid the same bill when one new require broke 82 tests across two suites. Migrating rather than adding another copyFileSync line keeps the class closed. The other nine suites referencing that path were triaged; none copies-and-spawns, so none needed migrating. * fix(#3906): enumerate the new shipped file, drop a vendor name from shipped data, and fix three test defects install: scripts/lib/exit-code-registry.cjs was missing from GSD_SCRIPTS_LIB_FILES, so it shipped to every install and orphaned on uninstall. The registry gave HOOK_DENY a meaning naming one harness, and that string ships into every runtime's tree - a guard correctly caught it leaking into the hermes and qwen installs. The registry is runtime-neutral infrastructure; the vendor name belongs in the ADR, not in shipped data. Two more fixture harnesses built their trees from hand-listed dependencies and broke on the new require; both migrated to the derived helper, and all 23 copy-and-spawn candidates were enumerated so the class is closed rather than patched. One generator test used a fixture code that collided with a real allocation, so the generator correctly reported a duplicate where the test expected drift. The large-payload test embedded a 256KB literal in the child's argv, exceeding Linux's 128KiB MAX_ARG_STRLEN so the child never started - it now builds the payload inside the child. * chore(#3906): backfill changeset pr number * docs(#3906): document the exit-code contract selector P2 is the first phase of this epic with a user-invocable surface, so the flag and env var owe a reference entry. Records what actually differs between v1 and v2 today (one outcome), that an unrecognized value is rejected rather than silently defaulted, and the fail-safe property that makes switching safe. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
39673ae9ff |
fix(#3738): antigravity global skills/agents install to ~/.gemini/config (#3921)
* test(#3738): antigravity global skills/agents must resolve under ~/.gemini/config Regression tests (RED first): --skills-root and gsd-tools query surfaces, install-plan dest dirs, and converter skills-path rewrite. * fix(#3738): antigravity global skills/agents install to ~/.gemini/config Antigravity's machine-local discovery scans ~/.gemini/config/{skills,agents}; the configHome (~/.gemini/antigravity) is deprecated for artifacts. Declare the ADR-1239 skills/agents 'home' override on the antigravity global layout — the same mechanism codex uses (.agents) — and divert ~/.claude/skills/ references in converted global content to ~/.gemini/config/skills/. configHome, settings, probe/migration semantics, and the local .agents layout are unchanged. * fix(#3738): retire deprecated configHome artifacts via installer migration 010 Next install converges an existing antigravity install: manifest-managed skills/gsd-*/ and agents/gsd-*.md under the configHome (a location AGY does not scan) are removed — modified files backed up first, unmanifested and non-gsd entries preserved — and now-empty containers retired. Global scope only; the local .agents surface is live. Docs + inventory updated. * fix(#3738): converter sync in bin/install.js, harness emit-root coverage, migration baseline - bin/install.js converter gains the same ~/.claude/skills → ~/.gemini/config/ rewrite as src (ADR-1508 dual copy must stay in sync). - Parity-manifest walk covers home-override emit roots (extraEmitRootsFor) so antigravity's emitted skills/agents stay differential-visible at their new install root; install-tree fixture regen confirms an unchanged key set. - skills-from-commands rule declares the antigravity converter as a runtime-scoped transform; one ack fragment covers the identity-classed workflow whose antigravity copy embeds the old skills path. - Migration 010 checksum baseline + home-override set doc updated; existing tests updated to the #3738 contract (global dest, golden parity via layout dest, integration expectations). * fix(#3738): tolerate an absent extra emit root on baseline-side measurement The base tree's installer predates the home override, so <HOME>/.gemini/config does not exist there; walk() threw ENOENT and the in-job baseline build failed. An absent extra root is the legitimate pre-override shape — skip it. * fix(#3738): review findings — manifest agents root, bare skills-path rewrite, guard comment - writeManifest resolves the agents-kind home override (_kindDestDirSafe), so the manifest records agents at their actual install root and drift detection keeps working (isolated review finding 1, major). - Converter bare forms ~/.claude/skills and $HOME/.claude/skills (no trailing slash) divert to ~/.gemini/config/skills instead of falling through to the retired configHome path (finding 2). - real-home-guard comment updated: antigravity's global agents kind is the first agents-kind home override (finding 3, doc-only). - Regression tests for both behavioral findings. * chore(#3738): changeset fragment (pr number backfilled after PR creation) * chore(#3738): backfill changeset PR number (3921) * fix(#3738): sandbox HOME in tests that install antigravity global artifacts antigravity is the first home-override runtime in the golden-parity and skills-wrapper suites (codex is not in their runtime lists), so those tests never needed HOME sandboxing — the real-home guard now (correctly) refuses their un-sandboxed global installs on CI, where HOME is the passwd home. * fix(#3738): stop the K3 sequential-sandbox env leak; sandbox L2's home-override plans K3's two back-to-back sandboxHome calls leave HOME pointing at the first sandbox once the after-hooks restore (each call saves the env as it found it, so the second saves the first's sandbox as 'original'). On the windows matrix that leaked gsd-k3-qwen-* home into the L2 property, whose antigravity/global run then (correctly) refused via the #3712 real-home guard — antigravity is the runtime that made L2's plan escape into os.homedir(). K3 now manages the env with a single restore; L2 sandboxes HOME per run, mirroring L1. * fix(#3738): L2 property's HOME sandbox must exist on disk The #3712 guard's sandbox exemption fails closed when identify(effectiveHome) is 'absent' — L2 never created its configDir, so on the windows matrix (tmpdir under the real home) the antigravity/global run refused even with HOME sandboxed. Create the per-run sandbox dir and clean it up. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9d65cd5404 |
fix(#3664): warn when config-dir targets a foreign-agent destination (#3794)
* test(#3664): failing-first rows for the config-dir foreign-agent warning * fix(#3664): warn when config-dir targets a foreign-agent destination * test(#3664): fold the foreign-agent warning rows into the install-regressions suite * fix(#3664): close review findings — kimi-agents kind, gsd.md ownership, e2e gate * test(#3664): sync boolean call sites and the path-vocab registries * chore(#3664): backfill changeset pr number * test(#3663): skip the posix case-pin on win32 where folding is the fix --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4af59f8dd3 |
fix(#3662): resolve managed hook node runners at hook-fire time (#3790)
* test(#3662): failing-first suite for runtime-resolving hook runners * fix(#3662): resolve managed hook node runners at hook-fire time * fix(#3662): close review findings and document the resolver * fix(#3662): close adversarial and security review findings * chore(#3662): backfill changeset pr number * test(#3662): honor win32 skip return and platform-aware sh runner pin * test(#3662): pin the bare win32-claude sh-hook shape omitting the bash runner --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
107eb8c1d9 |
feat(#3753): run docs guards on the PR that changes the docs they read (#3787)
A PR whose diff is entirely under docs/ runs zero tests, so a guard whose INPUT
is shipped prose cannot protect the PR lane of the diffs it exists to check. Its
only firing opportunity is after merge, on the shared branch -- which is how next
went red on
|
||
|
Behruz Nassre Esfahani
|
a44d513566 |
fix(#3712): confine in-process installs to a sandboxed HOME (#3725)
* fix(#3712): confine in-process installs to a sandboxed HOME
A runtime kind may declare a global `home` override resolved from os.homedir()
rather than from the caller's configDir — codex's skills kind (`home: ".agents"`,
ADR-1239 / #2088) is the only live case. Sandboxing configDir/targetDir does not
contain it, and assertDestWithinConfigHome cannot see the class: that gate
confines a destSubpath to whatever root it is handed, and here the root IS the
escaped home. So an in-process caller that forgot to sandbox HOME wrote to, and
pruned gsd-* entries from, the developer's REAL ~/.agents/skills.
tests/agent-descriptor-parity.install.test.cjs's K1 loop did exactly that: it
iterates every agents-kind runtime (codex included) with a sandboxed targetDir
and an un-sandboxed HOME. Reproduced against a canary home on next @
|
||
|
Tom Boucher
|
004e9dd741 |
fix(#3007): resolve Codex reasoning effort per model and make every clamp visible (#3765)
* test(#3007): failing-first suite for per-model Codex effort capability RED by construction. Binds to behavior renderEffortForRuntime does not yet have: an optional third `model` argument, a per-model advertised-level table, `max` passing through instead of clamping to `xhigh`, `minimal` clamping to `low`, `ultra` rejected outright, and clamp visibility (`requested`/`clamped`/ `reason`) so a downgrade is legible from resolver output rather than silent. Two of these pin defects that exist on next today: - `max` is discarded. Both Codex models whose catalog entries are retrievable (sol, luna) advertise `max`; GSD clamps it to `xhigh` and reports nothing. - `minimal` is emitted to a model that refuses it. providerPresets.openai. haiku.low pairs gpt-5.6-luna with reasoning_effort "minimal", and luna's advertised floor is `low`. GSD is sending a value into a document Codex itself validates. The parity test is what pins that fixed, and it names the offending path/model/effort when it trips. Also corrects tests/model-resolver.test.cjs:351, which asserted renderEffortForRuntime('codex','max').value === 'xhigh' -- the defect pinned as though it were a contract. ADR-443 recorded "Codex has no max" as fact and it was true when written; Codex has since added both `max` and `ultra`. That is a stale premise, so the assertion is corrected here rather than worked around. The property test asserts the invariant the whole change exists for: a rendered effort is always a level the target model actually advertises, or an explicit rejection. There is no third outcome. * fix(#3007): resolve Codex effort per model, and make every clamp visible Codex declares supported_reasoning_levels per MODEL and validates against it, so a single per-runtime capability set cannot be right for all of them. GSD's was wrong in both directions at once. `max` reaches Codex now. ADR-443 recorded "Codex has no max" as fact and clamped max -> xhigh on that basis; it was accurate when written, and Codex has since added both `max` and `ultra`. Every Codex model whose catalog entry is retrievable advertises `max`, so the clamp was discarding a level the provider supports, silently, on the most-used path. `minimal` stops reaching Codex. No Codex model advertises it -- both retrievable entries floor at `low` -- yet providerPresets.openai.haiku.low paired gpt-5.6-luna with reasoning_effort "minimal". GSD was writing a value the receiver validates and refuses into a file the receiver reads. Being unconservative in what you send is the half of Postel's rule with no defensible reading, so that preset is corrected and a parity test pins it. `ultra` is refused rather than laddered. Codex's own catalog calls it "Maximum reasoning with automatic task delegation": at ultra, effective_multi_agent_mode returns Proactive and Codex spawns sub-agents on its own initiative, underneath GSD's orchestration rather than inside it (#2167). It is a mode switch, not a reasoning depth, so it is not added to the universal ladder -- which stays provider-agnostic by ADR-443's design -- and it is rejected even for gpt-5.6-sol, which does advertise it. Clamping it down to `max` was considered and rejected: that silently discards what the user actually asked for. Clamping is now visible. RenderedEffort carries requested/clamped/reason and resolve-execution surfaces them. The previous table clamped correctly but invisibly, so a user asking for `max` on Codex had no way to find out they were getting `xhigh` -- exactly the failure mode the robustness principle's modern critique warns about, and why "be liberal" has to mean "liberal and loud". Also closes a latent trap found while reviewing the implementation: the clamp-up loop walks the ladder upward, and for a future model advertising `ultra` but not `max` it would have selected `ultra` as the clamp target -- re-entering by the back door the mode the rejection above exists to keep out. A clamp may never produce a value that a direct request for that value would refuse. Unreachable with today's catalog, which is why no test caught it; a test now asserts the invariant directly. Signature stability is preserved: the third `model` argument is optional and the two-argument form still resolves, against the family baseline. That form's BEHAVIOR does change for `max` and `minimal`, and it must -- keeping the old answer would have fixed the defect only where a model happened to be threaded through and left it live everywhere else. tests/model-resolver.test.cjs:351 asserted the defect as if it were a contract and is corrected here rather than worked around. * fix(#3007): close every review finding on the Codex effort alignment Two isolated reviewers, correctness and security. Both found the same two blockers, and the per-model work was inert on every surface that matters until this commit. BLOCKER — resolve-execution never passed the model and discarded the clamp. cmdResolveExecution called the two-argument form and emitted only effort_rendered/effort_param/effort_propagation, so the per-model table was unreachable from production code (tests were its only caller) and requested/ clamped/reason were computed and thrown away. Requested outcome 3 names "the effective rendered effort in resolver output" specifically, so the feature was unmet on the exact surface the issue asks for. Now passes the resolved model and emits effort_requested / effort_clamped / effort_clamp_reason, flat, matching the existing key convention rather than introducing a nested object. BLOCKER — the docs described output that did not exist. CONFIGURATION.md showed a nested {"effort": ...} sample; the real result is flat and those keys were absent entirely. A reference doc asserting a JSON path a reader can copy is worse than no doc. Corrected against the actual emitted key set. MAJOR — the argv channel still shipped both original defects. EFFORT_ARGV.codex kept minimal in its supported set and still clamped max down to xhigh, so the invocation-time and install-time channels disagreed about the same runtime's capability: --host codex with max emitted xhigh while the generated TOML said max. This is the repo's documented generative-fix-divergence class, so both tables now cross-reference each other and a parity test fails if they ever diverge again. MAJOR — malformed catalog data failed OPEN and could crash the CLI. A null _baseline became an EMPTY Set that is nonetheless truthy, so the nullish fallback never fired and every effort rendered as null. And a non-array value made the Set constructor throw at module load — model-catalog.cjs is required across the whole CLI, so one bad JSON value killed every command, not just codex effort. Guarded on size and filtered to array values; both degrade to the hardcoded baseline. MAJOR — value widened to a nullable string with two consumers left behind. runtime-artifact-conversion passed it straight into injectEffortFrontmatter (a null effort key in generated frontmatter); install-effort-resolver still declared a non-nullable return, a structural lie that silently defeated null checking. Both corrected, both omitting the key on null — the same posture as 'inherit', where omission means "follow the host default". MAJOR — the per-model table is inert today, and the docs now say so. All three shipped models advertise the same usable range and ultra (sol's only differentiator) is rejected for every model, so no observable output differs by model. The table stays because Codex declares capability per model and the sets are free to diverge — a single per-runtime assumption is precisely what went stale and produced this issue — but overselling it as a visible per-model feature would have been the same class of error as the doc blocker above. Tests: three passed under a full revert and are strengthened rather than deleted, since each guards a real contract (#3533's inherit rule, the undeclared-host rule, off-ladder handling) — they now also assert the clamp-visibility fields, which only exist after this change. The fast-check property is kept for its shrinking, and a deterministic nested loop over the full cross-product now sits beside it so coverage is exhaustive rather than sampled. Also folded in earlier: bin/install.js generated the Codex TOML with the two-arg form and would have written a literal null reasoning effort on the ultra path; CONTEXT.md's Model Catalog Module glossary entry now records CODEX_MODEL_EFFORT. The installer defect was found by the co-change gate, not by a reviewer — install.js is a historical co-change partner of model-catalog.cts that this diff had not touched. * test(#3007): correct assertions that pinned Codex's stale effort premise Thirteen pre-existing tests encoded "Codex has no max" as fact and failed on the shipped commit. Every one is a stale pin, not a defect: each was probed against the built module before its expectation was changed, and none failed for a reason other than this premise correction. Kept as its own commit per CONTRIBUTING — a test-fixture correction made stale by a production change must not ride inside another commit, because the release-sdk hotfix cherry-pick filter routes by subject prefix and a correction buried under the wrong prefix ships a half-state (v1.42.3, #3621). The most valuable one was tests/model-resolver.test.cjs's cross-provider validity invariant, which hardcoded the Codex enum as `minimal|low|medium|high|xhigh` and failed with "real API would 400". That message is now false in both directions: Codex accepts `max`, and rejects `minimal`, which no model advertises. The enum is corrected to `low|medium|high|xhigh|max` and the guard is kept intact — it is exactly the "would the real API refuse this" check worth having, and it was right to fail here. It simply carried the stale fact in its own fixture. Test NAMES were corrected alongside their assertions wherever the name asserted the old behavior — "max is Anthropic-only", "max clamps to xhigh", "minimal passthrough". A renamed test that still claims the old thing is worse than a failing one, and a green test whose name states a falsehood is how the next reader inherits the wrong premise. Both channels are covered: install-time (renderEffortForRuntime, and the generated .toml in install-runtime-artifacts) and invocation-time argv (effort-surface-axis). They were deliberately brought into agreement in this change, so their assertions had to move together. Each site carries a #3007 comment recording that Codex gained max/ultra and that capability is declared per model, so a future reader can tell this was a deliberate premise correction rather than a test bent to fit an implementation. * test(#3007): separate the effort-precedence case from the clamp case The previous stale-assertion pass over-corrected one test. It saw `effort: { default: 'max' }` on codex expecting `effort_rendered: 'xhigh'`, assumed the xhigh came from the max→xhigh clamp #3007 removes, renamed it to "max passes through" and changed the expectation to `max`. The remote runner disagreed. Reproduced against the real CLI: with that config and `gsd-planner`, the resolver emits `effort: "xhigh"`, `effort_requested: "xhigh"`, `effort_clamped: false`. The xhigh is produced by effort-resolution PRECEDENCE — gsd-planner is heavy/opus tier and its routing-tier default outranks `effort.default` — so `max` never reaches the renderer at all. The test says nothing about clamping and never did; it only looked like a clamp pin because both mechanisms happened to yield the same string. Restored to `xhigh` and renamed to say what it actually tests. It now also asserts `effort_clamped === false` and `effort_requested === 'xhigh'`, which is what makes it impossible to mistake for a clamp pin again: those two fields prove the value is what the resolver produced rather than something the renderer downgraded. Before #3007 there was no way to tell the two apart from the output — which is precisely why the previous pass could not tell them apart either. Added the test that was actually missing: `effort.agent_overrides`, which outranks the tier default, so the requested level genuinely reaches the renderer and `max` survives to `effort_rendered` end-to-end through the real CLI. Verified by probe before asserting. One test now pins the precedence rule and the other pins the #3007 behavior, and neither can be read as the other. That the clamp-visibility fields are what resolved this is a small argument for having added them. * chore(#3007): backfill changeset pr number to 3765 * test(#3007): put model-catalog under the mutation gate The Stryker shard showed as `skipping` on this PR despite the diff rewriting model-catalog's effort logic. That was legitimate, not a detection bug: `model-catalog` was never in scripts/mutation-matrix.cjs's COVERED map, so the whole module — including everything #3007 touches — sat entirely outside mutation scoring with has_work "false". Registered, with a dedicated spawn-free surface. tests/model-catalog.unit.test.cjs is new: 44 in-process tests, no runGsdTools, no child process, no filesystem, no temp dirs. That shape is not stylistic — it is the #2790 precedent this file already documents. Stryker's command runner treats a whole `node --test <file>` invocation as ONE test costing whatever its slowest case costs, and re-runs it per mutant, so pointing a shard at tests/model-resolver.test.cjs (which uses runGsdTools throughout) would reproduce exactly the 15-minute shard-cap cancellation #2790 hit. The integration file is unaffected and keeps running in full in the normal test job. Coverage spans the module rather than only the diff, because the score is measured over the whole file: effort rendering across every model and ladder level in both channels, the prototype-chain host guard, the exported enums and maps, isAnthropicFlavoredModel's provider namespacings, the profile projections, nextTier, and mergeEffortTierDefaults. The last two were nearly left out and are worth naming — every uncovered exported function is score given away, and mergeEffortTierDefaults turned out to have a genuinely interesting contract (#3531: a partial override merges over the built-ins rather than replacing them, and isValid gates the VALUE, not the tier name, so an unknown tier key is still merged in). Every expectation was probed against the built module before being asserted. minScore is 1 and that is a PLACEHOLDER, flagged as such in the registry comment. Floors in this repo are measured, not chosen — the existing entries sit at 94, 75 and 56 — and they can only be measured in CI, because mutation shards run `node --test`, which is hard-blocked locally. The first CI run on this branch reports the real number and the floor gets ratcheted to it before merge. A placeholder of 1 reaching `next` would make the gate decorative: it would pass whether or not a single mutant is ever killed. Note the target is "never regress from measured", not a fixed 80 — planning-inspect sits at 56 and is documented as an accepted ratchet candidate. * test(#3007): bootstrap model-catalog's mutation floor legally The placeholder floor was structurally illegal and the remote run said so. tests/mutation-matrix-ratchet.test.cjs guards the guard: every COVERED module must carry a matching RATCHET_BASELINE entry in the same diff, minScore must EQUAL that baseline, and it must be at least 50. `minScore: 1` failed all three. That is the ratchet working exactly as intended — a floor nobody can satisfy accidentally is the point of it. Bootstrapped at 50 in both places. Fifty is not a measured score and the comment says so plainly: it is the minimum the guard permits, and it coincides with Stryker's own configured `break` threshold, so it is the lowest legal starting point for a module that has never been measured. It still must be ratcheted to floor(measured) - 1 before this PR merges. Also corrected a real defect in the file's own instructions. "HOW TO UPDATE" step 1 read "Run the per-module Stryker shard locally" — which cannot be done here, and which the same file contradicts eighty lines further down, where the #2790 scores are recorded as "not a local run; mutation shards run `node --test`, hard-blocked in this repo's local environment". stryker.config.mjs confirms the command runner invokes `node --test` once per mutant, and .claude/hooks/block-local-node-test.sh denies exactly that. So the documented first step sends the next contributor at a wall. Rewritten to describe the path that works — push, read the measured score off the CI shard, then set the floor and its baseline together in one diff — and to say why local measurement is not available, so nobody rediscovers it the slow way. GOODHART SAFETY is untouched. The two-step is inherent to the environment rather than a shortcut: a floor cannot be measured before the first CI run exists, and the guard rightly refuses to accept an unmeasured one below its minimum. * test(#3007): ratchet model-catalog's mutation floor to its measured score The shard ran in CI and reported 59.62% — 248 mutants killed, 168 survived, no timeouts, no errors (run 32605073352, job 97108869486). Floor set to 58 per this file's own rule, minScore = floor(measured) - 1, which is the same arithmetic every sibling entry used: 57.03 to 56, 76.58 to 75, 95.65 to 94. Both halves moved together, because the ratchet guard asserts minScore equals its RATCHET_BASELINE entry and would reject them drifting apart. The spawn-free unit surface is vindicated by the clock: 57 seconds, against a 15-minute shard cap and a 9m46s frontmatter shard in the same run. That was the whole reason for creating tests/model-catalog.unit.test.cjs rather than pointing the shard at tests/model-resolver.test.cjs — #2790 recorded shards being CANCELLED at that cap when they targeted a runGsdTools-heavy integration file. The registry comment is rewritten rather than deleted. It previously warned that the floor was provisional and must not ship that way; leaving that text next to a measured floor would make the file lie in the other direction. It now records the measurement the way the sibling entries do, including that 59.62 sits below TARGET (80) and is therefore a ratchet candidate like planning-inspect at 56 — comfortably clear of its own floor with real room to grow. Raise it as the tests improve; never lower it. Worth stating plainly: 168 surviving mutants is not a clean bill of health. It is an honest floor for a module that had NO mutation coverage at all an hour ago, and it is now pinned so it cannot silently regress. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
72819a4616 |
fix(#3031): opt-in reclaim of GSD hooks orphaned in ~/.kimi (#3731)
* test(#3031): failing-first coverage for opt-in ~/.kimi legacy reclaim Drives the user-reachable installer surface against a sandbox HOME seeded with the pre-#2755 wreckage: a GSD [[hooks]] block, hooks bundle and CommonJS marker orphaned in ~/.kimi by a --kimi-code install. Covers the reclaim itself plus the four guards the diagnosis identified as negative space: opt-in only (no flag, no deletion), user-authored TOML and hook files preserved, a --kimi install never reclaiming its own root, and the KIMI_SHARE_DIR/KIMI_CODE_HOME collision where both roots resolve to one directory. Adds a fast-check property that stripping the block never destroys user content. Red until --reclaim-kimi-legacy exists. Refs #3031 * fix(#3031): opt-in reclaim of GSD hooks orphaned in ~/.kimi A --kimi-code install older than 1.10.0 wrote its GSD [[hooks]] block, hook bundle and CommonJS marker into Kimi CLI's ~/.kimi. #2755 fixed the destination but could not reclaim what the old bug already wrote: the stale block is byte-identical to a legitimate Kimi CLI one — both runtimes render the same bytes for the same root, since the command paths derive from the hooks root, not the runtime — so no inspection can tell litter from a working install. Cleanup is therefore opt-in. `--reclaim-kimi-legacy` on a --kimi-code install removes GSD's own artifacts from the legacy root; without it nothing is touched, so a dual-product machine keeps Kimi CLI's hooks and #2755's acceptance criterion holds. Extracts the uninstall path's removal sequence into reclaimKimiHooksRoot() and drives both callers through it, so the reclaim removes precisely what a real uninstall removes rather than a hand-copied second implementation. Guards the wrong-runtime case (a --kimi install would delete its own hooks) and the KIMI_SHARE_DIR/KIMI_CODE_HOME collision where both roots resolve to one directory. Also corrects two pre-#2755 leftovers in the same surface that told users to run `--kimi --config-dir ~/.kimi-code` — the form that produces this very defect, since --config-dir moves only the skills root — and adds the missing --kimi-code entry to the installer's own help. Regression coverage folded into tests/kimi-upgrades.test.cjs beside the #2755 cases, per the regression-test-naming lint. Fixes #3031 * fix(#3031): never reclaim ~/.kimi when this run also installs kimi Found by the isolated adversarial review pass and independently while tracing --all ordering, then reproduced. selectRuntimesFromArgs orders 'kimi' before 'kimi-code' in both --all and an explicit --kimi --kimi-code, and installAllRuntimes installs in that order. So --all --reclaim-kimi-legacy installed a fresh, legitimate Kimi CLI hooks block into ~/.kimi and then deleted it moments later from the kimi-code leg — exiting 0 and reporting success while leaving the user with no Kimi CLI hooks at all. The collision guard could not catch it: kimi-code's own root is ~/.kimi-code, a genuinely different directory. The flag asserts "I only use Kimi Code"; installing kimi in the same invocation falsifies that, so the reclaim is skipped with a notice. Also hardens the collision guard itself. It compared path.resolve strings, which returns false for two spellings of ONE directory — measured, not assumed: a symlinked alias and a case variant on a case-insensitive filesystem both compared unequal, so the guard would not have fired and the install would have deleted its own freshly-written hooks. isSameDirectory now compares directories via resolve, then dev+ino identity, then realpath. Regression tests for all three cases; the two alias tests probe the real filesystem and t.skip() where the alias cannot exist. Refs #3031 * docs(#3031): reattach reclaimKimiHooksRoot's JSDoc to its own function Inserting isSameDirectory anchored on the function name, which placed the helper between reclaimKimiHooksRoot's doc block and the function it documents. isSameDirectory ended up with two stacked doc blocks above it and reclaimKimiHooksRoot with none. Refs #3031 * fix(#3031): warn when --reclaim-kimi-legacy cannot apply The flag only acts inside the kimi-code GLOBAL install branch. Passed with any other runtime, or with --local, it was consumed in silence: exit 0, no cleanup, no message. For a cleanup the user explicitly asked for, silence is indistinguishable from "it ran and found nothing". The scope warning is raised at argument-resolution time rather than inside install(). kimi-code declares hostBehaviors.localInstallDeferred, so install() returns early at the deferral check long before the kimi-hooks-toml branch — a guard placed there is unreachable, which is both dead code and a linted drift shape in this repo. Verified reachable by spawning the real installer. Neither case is a hard error: the flag stays composable with --all, where it is legitimately inert for the other seventeen runtimes. Refs #3031 * docs(#3031): document every case where --reclaim-kimi-legacy skips Refs #3031 * fix(#3031): resolve local config dirs from RUNTIME_META alone in the install harness The remote runner surfaced this: the #3031 warning test drives a local kimi-code install and died with "The path argument must be of type string. Received undefined". runMinimalInstall carried a SECOND, hand-maintained local-dir map beside RUNTIME_META, and it had drifted — four runtimes present in RUNTIME_META (hermes, kimi, kimi-code, zcode) were missing from it, so scope:'local' for any of them resolved path.join(root, undefined) and threw a bare TypeError naming neither the runtime nor the map at fault. #3023 had already hit exactly this for pi and fixed it by adding one more entry, which left the divergence itself in place for the next runtime to rediscover. Local scope now reads RUNTIME_META.localDir, the same table the global branch already reads, with the same loud named error the global branch raises. Parity verified for all 14 previously-supported runtimes: every one resolves to a byte-identical configDir. cline keeps its ternary — its local artifacts land at the project root itself, which is a real exception, not a directory name. Guarded in golden-parity-single-source.test.cjs beside the buildParityManifest anti-divergence test, and both arms of that guard were proven able to fail. Refs #3031 * chore(#3031): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
bad1f045b1 |
fix(#3610): hoist surviving top-level codex config keys to file scope on merge (#3690)
* test(#3610): pin top-level key hoisting above the codex managed block * fix(#3610): hoist surviving top-level keys above the codex managed block * chore(#3610): add changeset * fix(#3610): hoist to file scope (before the first table header) with reviewer-driven coverage * chore(#3610): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
682eaae3f0 |
enh(#2876): retire the dead and pass-through exports from bin/install.js (#3615)
* enh(#2876): retire the dead and pass-through exports from bin/install.js The installer exported 197 names and had zero production consumers - every non-test require of it repo-wide sits inside a comment. Its interface was shaped by test access, not by callers. Removes 9 dead exports and 61 pass-throughs, repointing their tests onto the extracted modules' own interfaces. 197 down to 127. Every count in the issue was wrong: 197 exports not 188, 9 dead not 12, 61 pass-throughs not 49, 44 test files not 42 - and the audit itself then missed 7 more consumer files. restoreUserArtifacts was on the dead list but ceased to exist in phase 6, and two _GSD_EFFORT_MANIFEST_* names listed as dead are now genuinely asserted, so acting on that list would have deleted live exports. 7 of the 9 dead names collide with an independent declaration that install.js delegates TO. Each removal was justified by which declaration a reference resolves to, never by whether the name appears somewhere. Coverage parity was the gate rather than test greenness: per-file counts were captured before any edit and diffed after. 44 of 45 files are byte-identical; the single delta is one added assertion, not a loss. The sweep for scattered require sites found two forms static grep misses - require(VARIABLE) and multi-line require() - plus tests asserting that install.js re-exports the SAME object, which now assert retirement instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2876): close review findings — restore the duplicate-body guard, sweep orphaned code Both review engines found real defects in the first cut. The DEFECT.GENERATIVE-FIX single-owner guard from #1511 had been repointed from a reference-identity check to install.X === undefined. Those are not equivalent: the guard exists to catch a duplicate function body reintroduced into install.js, and the replacement passes cleanly if that duplicate is used internally and never exported. It now walks bin/install.js's real top-level bindings, so it catches a duplicate under either shape, exported or not - strictly stronger than the check it replaced. Proved by injecting a duplicate and watching it go red. That weakening survived the coverage-parity gate because the assertion count never moved. The gate compares counts, so an assertion that changes meaning rather than number is invisible to it. Removing the exports had orphaned their wrapper bodies: 14 dead wrappers, 9 consts and 9 destructure entries, several pre-existing and found by the same sweep. Dead code left in the file this phase exists to shrink. Three more comments claimed re-exports this phase removed, and tests were reading Cursor and Windsurf hook constants from install.js's local copy while calling functions from the hooks surface - equal today, with nothing holding them equal. The local consts now reference the owning module. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2876): backfill changeset pr number 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
|
3ab0007164 |
enh(#2875): materialization primitives — durable user-artifact staging and descriptor-authoritative agents (#3600)
* fix(#2875): stage user artifacts durably across install wipes (#1874-F19) preserveUserArtifacts held user files only in an in-memory Map across the wipe, so any process death between preserve and restore lost them outright. Seven call sites, not the four the issue records. Three of them never called the helper at all - they open-coded the same read/wipe/write - so searching for callers under-counted by construction; the extra sites were found by sweeping for the pattern instead. The worst is the mainline install path, where the crash window spans the entire gsd-core tree copy rather than a single rmSync. Adds src/user-artifact-staging.cts: durable on-disk staging with a record written after the copies land as the commit point, plus recovery of orphaned batches on the next run - without recovery the staged bytes survive but the user's file is still gone, which would pass its own test while delivering nothing. Routes copyPreservingSymlink through installFs() so staging cannot bypass the install fs seam, and reunites its symlink-safety docblock with the function it documents. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-3574 with four claims disproved by implementation Implementing Phase 6 disproved four statements the ADR rests on. The central decision - no single materializer - is unaffected and stands. Corrected: decision 3 was already satisfied, so nothing was extracted; the agents-bypass runtime set omitted claude, kilo and opencode, and closing it needed three new pieces of descriptor contract rather than proceeding on its own terms; three of the four blockers the layout comment names were already stale; and F19 is seven call sites, not four. Records the generalizable lesson: the defect is the pattern of holding user data in memory across a wipe, not the helper, so searching for callers of the helper under-counts by construction. Also resolves the ADR's open question on USER_OWNED_ARTIFACTS membership, and notes that copyPreservingSymlink needed routing through the install fs seam before it could be reused. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close dangling-symlink blind spot and harden staging recovery An adversarial review found the F19 staging work shipped red and unsafe. Root cause, shared by two arbitrary-write findings: hasExistingSymlinkBetween missed dangling symlinks in both its root check and its per-segment walk, because it probed with existsSync, which is false for a link whose target does not exist. Fixing only the new module would have reused a guard that was itself blind. This guard protects the whole install tree. Recovery no longer throws: it degrades per entry and per file, so one bad batch cannot block the others. Previously an unrecoverable entry propagated out of the first statement of install and uninstall, before the cleanup that would have removed it - wedging the installer permanently. Partial fs adapters now throw on any omitted method instead of silently reaching the real filesystem, closing the trap that let a test poison list pass while real IO happened. Staged names must be flat, recovery refuses a dangling destination symlink, and a batch whose recovery genuinely failed is no longer swept - it was discarding the only durable copy of the file it had just failed to restore. Replaces three tests that could not fail, including the one labelled negative proof. Known limitation, documented not closed: concurrent installs sharing a staging key can still lose a batch. A real fix needs a cross-process lock. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enh(#2875): make the descriptor authoritative for the agents kind Deletes the inline agent-staging loop in bin/install.js and the _DESCRIPTOR_AGENTS_RUNTIMES set, so every runtime materializes agents from its capability descriptor instead of an inline hostBehaviors dispatch. Closing it needed three pieces of contract the descriptor pipeline never had, all reducible to one missing input - per-agent resolution context: a frontmatter-extensions step for claude's effort and disallowedTools, per-agent model-override resolution for kilo and opencode, and a named branding converter for hermes, whose rewrite data was already declared. Seven runtimes were on the loop, not the six the design recorded - kimi-code was found by a golden fixture, not by analysis. claude-local and kimi-code both silently lost their agents mid-change; the fixtures caught both and the cause was fixed rather than the fixtures regenerated. A parity harness gates the migration: both pipelines over identical inputs, byte-identical output including filenames, per runtime. It is demonstrated red before being trusted. Surface and install paths converge for all seven, which also fixes surface previously writing no agents for these runtimes. Codex's config.toml strip stays put - it mutates host config, which no descriptor kind models. Also routes install-model-override-resolver and install-effort-resolver through the install fs seam. Both leaked real filesystem IO from the install call tree; the stricter adapter is what exposed them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): record the agents-descriptor migration and correct the ADR count The _DESCRIPTOR_AGENTS_RUNTIMES allow-list no longer exists, so the host integration guide told readers to join a set that is gone. Replaces that with what is now true - declare an agents entry and it installs, on the surface path as well as install - and points anyone needing a per-agent transform at the three extension points rather than at a new inline branch. Corrects the ADR amendment: seven runtimes were on the inline loop, not six. kimi-code was found by a golden fixture going red, not by reading. That is the third short count this phase, all from enumerating by symbol or set membership when the thing that matters is a behavior. Adds the Changed changeset for the surface-path convergence. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): amend ADR-2866 - claude global always wrote agents on disk The claude row's global=[skills] described what capability.json declared, not what the installer wrote. bin/install.js's inline agent-staging loop was never scope-gated and never consulted the descriptor, so a claude --global install has always written agents/gsd-*.md. Phase 6 closes the gap by deleting that loop and declaring agents on claude's descriptor at global scope. On-disk bytes are unchanged - the golden fixtures did not move, which is the evidence that the descriptor, not the installer, was incomplete. #2218 is unaffected: agents are not trigger-bearing, so the wider row does not introduce a new shadowing case. Records the warning that an incomplete descriptor is invisible while a second code path silently does its work, and only surfaces when the two are forced into agreement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close review findings across staging, agents and the parity harness Two independent reviews of this branch found defects the local gates missed. Security: a dangling symlink at a migration destination allowed writing outside configDir - the same class this change claimed to close, missed at the terminal write of the flow being added. The staging-root resolver threw as the first statement of install and uninstall, so a hostile symlink bricked both, and symlinked-configDir users lost uninstall as well as install; it now degrades instead of aborting. Recovery gained a source-side symlink check and now refuses a relative destDir, which resolved against cwd. Converter dispatch gained a runtime allowlist - lint-time validation stopped mattering once this branch promoted that dispatch from the surface path to real installs. Correctness: claude --local --minimal exited 1 because the minimal profile legitimately yields zero agents and the new path treated that as a failure. cline --local silently lost its agents - its descriptor declared none while the deleted loop wrote them unconditionally. The agents prune was widened to any gsd-* entry and destroyed user files it never owned. The parity harness, on which the migration's safety argument rested, drove a synthetic registry and never byte-compared the shipped descriptors; two of its trap rows could not fail. It now drives the real registry across 13 runtime-scope rows including kimi-code and cline-local, and its red-proof is demonstrated by corrupting a live capability.json. Three goldens that had encoded the cline regression as expected behavior were corrected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): close findings from both mandated review engines /security-review found the staging source-side walk honouring GSD_ALLOW_SYMLINKED_DEST, an opt-in documented as relaxing only the write destination. A symlinked files/ component dereferenced because copyPreservingSymlink lstats the leaf only, so an intermediate link is followed. The source walk no longer honours the opt-in; the destination check still does. /code-review spec axis found this branch had reintroduced its own bug: migrateLegacyDevPreferencesToSkill's new symlink refusal threw unguarded after the legacy dir was wiped and before the staged batch was restored, so a planted symlink bricked uninstall permanently and orphaned the batch. Refusal kept, abort removed. kimi-code local silently lost its agents, the same class as the cline bug, and the parity harness recorded that exclusion as intentional - the third test in this branch to pin a regression as correct. --minimal now creates an empty agents/ dir that never existed. Behaviour restored rather than softening the changeset, so its byte-identical claim stays true. Standards axis: try/finally removed from twelve test bodies, fast-check properties added for parseOwnerPid, boundary coverage at the grace window and the ancestor-probe depth, a parity assertion for the staging-root helper duplicated across two files, and the 8-deep config walk deduplicated. Records 60-review.json with every finding and disposition from five passes, including the smells left unfixed and why. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2875): prune stale agents unconditionally in minimal mode The previous round stopped an empty agents/ directory being created when the resolved profile yields no agents. That was implemented by skipping the agents kind entirely, which also skipped its stale-agent prune - so a full to minimal downgrade left stale gsd-* agents behind. The deleted inline loop pruned unconditionally and only skipped writing. Those are three separate conditions, not one: prune always, write only when there is something to write, create the directory only when writing. Both call sites now run _removeGsdEntries before the empty-staged early exit. The symlink-escape guard moved with it, since the prune also touches dest. Codex .toml agents and the config.toml stanzas are cleaned again, and user-owned agents are still preserved. The agents/ directory is left in place after a prune empties it, matching every sibling kind - none of them remove the destination directory itself. Golden fixtures confirmed byte-identical: the prune is a no-op on a fresh install, so fixture generation is unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2875): document interrupted-install recovery for user-owned files The durable-staging fix is invisible to the user it protects. Someone whose install died mid-flight has no way to know USER-PROFILE.md was staged before the delete, that the next run restores it, or that recovery happens at the start of that run rather than in the background. Written as the task the user has - finish the interrupted command - rather than as a description of the mechanism, and states what it will not do: overwrite a file already present, or touch staging belonging to another install still running. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2875): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2875): assert the J8 model override without building a regex CodeQL flagged incomplete string escaping: the assertion interpolated the override value into a RegExp while escaping only forward slashes, which is meaningless in a constructor, leaving real metacharacters unescaped. The failure direction was the dangerous one - a metacharacter would have made the match more permissive, so the row would pass when it should fail. That matters here because J8 exists precisely because an earlier revision was a tautology; the rewrite reintroduced a different way for the same assertion to stop discriminating. Replaced with a line-wise exact match, so no regex is constructed at all. Swept the other test files this branch adds; no sibling instances. lint:ci passed on the original - lint-no-adhoc-regex-escape matches a full metachar-escape copy, so a single slash replace slipped under 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
|
c2e453e2a1 |
fix(#3543): bake no tier model when the effective model_profile is unverifiable (#3563)
* test(#3543): add failing-first regression for unverifiable profile bake * fix(#3543): bake no tier model when the effective model_profile is unverifiable * test(#3543): use cleanup helper for planning dir removal in regression test * test(#3543): use shared temp-dir and console-capture helpers in regression suite * chore(#3543): backfill changeset pr number * test(#3543): clear ambient xdg env overrides in global install tests * test(#3543): assert baked model line by equality instead of dynamic regexp --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
fd2b97a52a |
fix(#3544): restore tilde form for at-refs in the global spec tree (#3551)
* fix(#3544): restore tilde form for at-refs in the global spec tree A global claude install emitted @$HOME/.claude/gsd-core/references/*.md in its workflows and references. $HOME does not expand in a Claude Code @-import - only relative, absolute and ~ are documented, and a controlled /context test confirmed a $HOME import loads nothing - so 54 includes across 22 files silently resolved to nothing on a live install. This is a divergence, not a new bug. #3133 already applies exactly this correction to skill and command bodies through _applyRuntimeRewrites's claude case; copyWithPathReplacement, the spec-tree emit path, never had it. Both now call one exported helper, so the two surfaces cannot drift apart again. Deliberately narrower than changing computePathPrefix's return value: shipped markdown also carries double-quoted "$HOME/.claude/..." shell invocations, and ~ does not expand inside double quotes, so rewriting the prefix wholesale would regress #1284. Only @-prefixed references move. Refs #3544 * fix(#3544): derive the tilde restore from the resolved prefix Three review findings, one batch. The restore was hardcoded to the literal .claude directory, so a global install with --config-dir pointing anywhere else silently no-opped and reproduced the very defect this fixes. It now derives the tilde form from the resolved prefix, which also closes the same latent gap in #3133's original path since both call sites share the helper. The @-anchor is quote-aware, so a double-quoted shell path is never rewritten into a form the shell does not expand. Deliberately a lookbehind rather than a line-start anchor: @-references are documented to work mid-line, and anchoring would have traded a theoretical bug for a real one. Found while testing the above: the bare-form rewrites re-matched their own output whenever a config dir name extends .claude, emitting .claude-work-work. Guarded with the same negative-lookahead convention this file already uses to preserve .claude-plugin. The tests prove the emitted form, never that the host resolves it - no CI test can - and both the helper and the suite now say so, because an undocumented verification boundary is how this defect stayed green for its whole life. Refs #3544 * test(#3544): acknowledge the tilde-restore emitted drift The converter change moves 94 emitted paths that no source-file diff can explain, which is exactly the case the per-PR ack fragment exists for. Verified before acknowledging rather than after: both trees were built from real installs and every one of the 211 changed lines across all 94 paths is @$HOME becoming @~, with nothing outside that single kind. Nine spent entries were pruned from the #3151 and #2658 fragments. Those paths moved again here, and two ack sources naming one path is a hard duplicate error rather than last-wins, so the inert entries had to go before this one could land. Both fragments retain their remaining entries. Refs #3544 * chore(#3544): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3d17569d5b | Merge pull request #3537 from open-gsd/feat/2873-cross-scope-shadowing | ||
|
Tom Boucher
|
50d5368add | fix(#3533): effort inherit — expressible, omitted at writers, never re-added (#3541) | ||
|
sim
|
2641e6cb67 |
feat(#2873): detect cross-scope shadowing and reach the local spec tree
4a - the detection floor. A shadowed install now reports which triggers are shadowed and which scope wins, at install time and through a new W028 /gsd-health diagnostic. Exit codes are untouched: a shadowed install is a warning, not a failure. Only triggers whose stem exists at BOTH scopes are reported, so a global full profile beside a local core profile no longer names local artifacts the user does not have. 4b - spec-root reachability, claude runtime and global scope only. The winning global skill stops carrying a static workflow @-include and instead resolves its spec at runtime: prefer the project-local copy, fall back to the global one, stop if neither exists. Every other @-include stays static, and the local emission is byte-identical. It runs after the staged-skills rewrite pass, whose claude branch would otherwise mangle the literal tilde path into an undocumented $HOME form. Also fixed inline: readInstallManifest classified a top-level JSON array as an installed v1 manifest, because typeof [] is object. Refs #2873 |
||
|
Tom Boucher
|
268ca7e32d |
fix(#3504): harden hook injection patterns and force-add guard (#3510)
* test(#3504): add failing-first parity, fail-closed, and bypass suites * fix(#3504): harden hook injection patterns and force-add guard * test(#3504): stage the scanner lib dependency in shared-hooks fixture * chore(#3504): backfill changeset pr number * test(#3504): build parity samples from fragments for the ci scan --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
71180983a0 |
fix(#3423): standardize on <required_reading>, retire the files_to_read emit tag (#3432)
* fix(#3423): standardize on required_reading, retire files_to_read emit tag * test(#3423): flip tag assertions, extend consistency guard to spawner surfaces * fix(#3423): sweep capabilities fragments, regen registry+skills, anchor executor test * chore(#3423): acknowledge tag-rename emitted ripples and workflow growth * chore(#3423): broaden emitted-ripple acknowledgment to all embedders * chore(#3423): settle emitted-drift acks post-rebase (merge 3004/1689-owned keys) * chore(#3423): drop stale ripple acks, ack execute-phase growth * chore(#3423): restore pristine 3004 fragment, keep only consumed appends * chore(#3423): backfill changeset pr number * chore(#3423): settle emitted-drift acks post-merge (move code-review-fix ripple into 3190, tag-rename ripples into 3191/3297) * chore(#3423): re-arm 3324 ack for execute-phase.md tag-rename ripple * fix(#3423): trim 8 bytes from execute-phase model note to hold ADR-857 margin, re-arm 3370 ack for net +4 growth --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
967bddba37 |
fix(#3384): strip mcp__* tool grants from zcode-installed subagents (#3483)
* fix(#3384): strip mcp__* tool grants from zcode-installed subagents ZCode's dispatcher treats every mcp__<server>__* entry in an agent's tools: frontmatter as a required MCP server and hard-fails the subagent spawn (CONFIGURATION_ERROR) when it is not connected, whereas Claude Code treats the same grants as an optional allowlist. ZCode shared Claude's verbatim agents copy (converter: null), so all 8 MCP-granted agents failed to spawn out of the box with zero MCP servers configured. Add convertClaudeAgentToZcodeAgent — a line-surgical converter that filters mcp__* entries out of the frontmatter tools: grant list (both inline comma and YAML block-list shapes) and preserves every other byte. Declare it on both of zcode's capability.json agents entries and cut zcode over to the descriptor-driven agents path (_DESCRIPTOR_AGENTS_RUNTIMES) so the legacy inline loop stops deleting+re-copying the converted agents raw. Claude Code, Kimi, and Gemini install behavior is unchanged. * chore(#3384): link changeset fragment to pr 3483 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8bbb3eaabc |
fix(#3329): reconcile stale managed .sh hook commands on install/update (#3460)
* fix(#3329): reconcile stale managed .sh hook commands on install/update applySettingsJsonHooks registers the four .sh managed hooks only-if-absent, so entries registered before the #580/#3393 shellHookOmitsBashRunner fix kept their bash-runner-prefixed commands forever — /gsd-update re-invokes the installer but never re-derived existing entries. Add reconcileManagedShellHookCommands (wired into applySettingsJsonHooks): on win32+claude it rewrites existing managed .sh entries to the command this install would generate today, scoped to exact managed basenames so user-authored hooks are untouched, and inert wherever the bash runner is still the correct shape. Also bumps the allow-test-rule ceiling 301→302: PR #3455 added tests/milestone-lock.test.cjs (the 302nd marked file) without the ratchet bump, leaving lint-tests red on next. * chore(#3329): add changeset fragment * chore(#3329): backfill changeset pr number 3460 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b9adedbc86 |
fix(#3151): stop emitting effort: into skill frontmatter (cache invalidation) (#3425)
Claude Code applies SKILL.md effort: as output_config.effort; any change from the session baseline invalidates the prompt cache at BOTH scope boundaries (entry + exit, the latter often machine-fired via subagent-completion notification). The reporter's owned measurement confirms it: /gsd-progress (effort:low) in a medium session → cache_creation 63,404 (entry) + 18,589 (exit), while a no-effort skill shows none. ~76% of invocations paid in full. Fix (trek-e AC#2/AC#4): convertClaudeCommandToClaudeSkill no longer emits effort: into Claude-runtime skill frontmatter (src/runtime-artifact-conversion.cts + duplicated bin/install.js). normalizeClaudeSkillEffort removed (dead). The six declaring skills (plan-phase/execute-phase/autonomous/next/progress/stats) no longer carry effort. Source command files keep effort (input, used elsewhere); the separate agent-effort surface (#3160) is untouched. Tests: install-runtime-artifacts #769 block flipped to assert effort is ABSENT from installed SKILL.md + converter output (the AC#4 behavioral coverage). Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
dc3c81e93d |
chore(#3212): src/pattern.cts is the sole owner of runtime-value regex construction — Phase 1 (#3416)
* test(#3412): failing-first suite for the pattern-construction seam Phase 1 of epic #3212 (ADR-3212 §1/§2/§7). Tests only — src/pattern.cts and eslint-rules/no-adhoc-regex-escape.cjs do not exist yet, so both suites fail with MODULE_NOT_FOUND, which is the intended RED. Locks the measured behavior rather than the assumed behavior: RegExp.escape hex-escapes the leading character of nearly every string ("abc" -> "\x61bc"), so the suite asserts match-equivalence against an inlined historical oracle (the implementation being deleted) rather than byte-equivalence of pattern text — 200 seeded fast-check runs plus a fixed corpus, 0 mismatches. Also locks the latent character-class range bug this phase fixes as a side effect: a hyphen-bearing value interpolated into [...] currently forms a real range and matches an unintended character; post-migration it must not. * chore(#3412): src/pattern.cts owns runtime-value regex construction Phase 1 of epic #3212 (ADR-3212 §1/§2/§6/§7). Adds the pattern seam delegating to the built-in RegExp.escape, deletes every hand-rolled copy, and raises the Node floor to the Active LTS line. The census was low, three times over. ADR-3212 counted 10 copies; a graph query found 12; the new lint rule — once live — found 27 more. The difference is that the census counted named helper FUNCTIONS while the rule counts the escape SHAPE, so inline .replace(<class>, '\$&') copies were never in scope. ADR §1's actual requirement is that no module outside the seam escapes a value for regex use, so all of them are, and CLAUDE.md's no-defer rule makes them this change's work. Fourth consecutive epic here whose copy count was low — the argument for ADR-3180 Amendment 3's "state N found by the guard" rule. Also corrected mid-implementation: the survey reported phase-id.cts's escapeRegex had 0 external importers. It had 8 production importers, making its removal a public-surface change to an ADR-2121-owned module and requiring an update to that ADR's locked-surface test. Blast radius revised Medium-High -> High. RegExp.escape is match-equivalent but NOT text-equivalent: it hex-escapes the leading char of nearly every string ("abc" -> "\x61bc"). Equivalence is proven by a seeded fast-check property test against the deleted implementation as oracle. It also fixes a latent bug: a hyphen-bearing value interpolated into a character class previously formed a real range and matched an unintended character. Node floor 22 -> 24 (RegExp.escape is Node 24+), across engines, .nvmrc, package-lock, 9 CI matrix entries, and 5 docs. The aggregate `required-tests` context is unchanged and no job was added or removed, so branch protection cannot be orphaned by the dropped lanes. Enforced by eslint-rules/no-adhoc-regex-escape.cjs (shape-matched, with structural provenance for reviewed pattern-fragment constants rather than a name heuristic) plus a whole-tree companion guard covering the directories ESLint's globs miss. * fix(#3412): close the _SOURCE guard evasion, correct two false claims Three findings from the orthogonal review pass, all fixed. 1. The ESLint rule's `_SOURCE` provenance fallback was pure identifier- name matching with no binding check, so `new RegExp(userInput_SOURCE)` — a function parameter — sailed past the guard. That is the same rename-evasion class issue #3410 documents, reopened by the very fallback meant to complement the structural check. Now bound to the identifier's actual binding kind: import, require-derived const, or module-scope const; parameters, `let`/`var`, and unresolvable bindings fail closed. Four RuleTester cases cover the evasion and prove the legitimate cross-module case still passes. 2. src/pattern.cts's own header carried the stale pre-correction counts (12 copies / 17 call sites) while CONTEXT.md and the design doc carried the corrected ones (~39 / ~44) — a self-contradiction inside the PR whose entire purpose is deleting divergent copies. Rewritten, preserving the durable lesson: a named-function census cannot see inline copies; only a shape-matching guard can. 3. The claim that all deleted copies threw TypeError on non-string was false. phase-id.cts's copy — the one with 8 external importers — did String(value).replace(...) and never threw. The seam's locked signature does not coerce, so this is a real, now-disclosed behavior change rather than the pure preservation the tests asserted. Audited all 32 invocations across the 8 importers and 6 in-file callers: every one is safe by construction (upstream truthy guard or a string-producing derivation), verified by runtime probe against the compiled modules rather than by TS compilation, which cannot see a runtime undefined. Corrected the false claim in both the test comment and the design doc, and added it to Known limits. * docs(#3412): add Changed changeset for the Node 24 floor The only user-visible break in this phase. The escape-behavior change is internal and match-equivalent, so it carries no user-facing note. * fix(#3412): resolve the seam's require graph in script fixtures and packaging Checkpoint 2 came back red with 90 failures on the node24 lane. Three distinct defects, all introduced by routing scripts/ through the new pattern seam, none reproducible by any local gate: 1. ~82 failures — tests/adr-index-gate.test.cjs and tests/removed-but-needed-lint.test.cjs copy a scripts/*.cjs into an mkdtemp fixture and spawn it there (necessary: those scripts resolve their scan root from __dirname/.., so running the real script would scan the real repo). Each harness hand-listed the dependencies to copy alongside. Adding require('../gsd-core/bin/lib/pattern.cjs') to gen-adr-index.cjs made both lists silently incomplete -> MODULE_NOT_FOUND, plus 17 downstream 'did not emit parseable JSON' failures from the same crash. Fixed as a class, not an instance: new tests/helpers/copy-script- fixture.cjs walks a script's transitive static relative-require graph and copies it, so dependencies are derived and never re-declared. It throws (naming the unbuilt artifact) instead of letting the child die with a bare MODULE_NOT_FOUND. Verified for all four seam-consuming scripts: gen-adr-index, lint-removed-but-needed, gen-loop-host- contract, sync-runtime-launcher. 2. 2 failures — scripts/ ships wholesale but eslint-rules/ does not, so the new scripts/lint-no-adhoc-regex-escape.cjs would be MODULE_NOT_FOUND in a published install (#2858 guard). Excluded from the tarball, matching the existing precedent for gen-emitted- baseline.cjs, which is excluded for the identical reason, and locked with a test modeled on that one. Confirmed against a real npm pack: 890 files, 0 from eslint-rules/, and gsd-core/bin/lib/pattern.cjs present (so the other four scripts' requires are legitimate). 3. 6 failures — tests/phase-id.test.cjs asserted the literal escaped source text ('0*29', 'PROJ-42'). RegExp.escape is match-equivalent to the retired hand-rolled escaper but NOT text-equivalent: it hex- escapes the leading character and all hyphens ('0*\x329', '\x50ROJ\x2d42'). Verified NOT a behavior change — 576 match decisions across all three real interpolation prefixes, zero divergence. Those tests now compile each source into the same heading regex src/roadmap.cts's searchPhaseInContent builds and assert what matches and what does not, including the 'i'-flag canonicalization the hex escape has to preserve. Re-pinning the new literals would have rebuilt the same brittleness one layer down. Adds a test for the property the escape exists for: a dot in '1.2' must not act as a wildcard. Also shares one definition of 'a require' between the packaging guard and the fixture copier, so the two cannot disagree about what they scan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): refuse to copy a fixture dependency outside the fixture root copyScriptWithDeps resolved each relative require and joined the repo-relative result onto fixtureRoot. A require resolving OUTSIDE the repo yields a '../'-prefixed relative path, so path.join climbed out of the fixture and wrote into the surrounding temp dir (verified: repoRoot=/repo + depAbs=/etc/passwd wrote /tmp/etc/passwd). No script in the tree does this today, so this closes an available escape rather than an active one. Refuses via the existing unresolved- require path so the failure names the offending specifier. Covered by a negative proof that the guard fires and that nothing lands outside the fixture. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3412): parse requires instead of pattern-matching them; restore the foreign-prefix contract Applies all findings from the second orthogonal review round, re-run because real code changed after round 1. HIGH (security) — extractRequires stripped BLOCK comments before LINE comments, so a '//' comment containing '/*' opened a phantom block comment, and a '//' inside a string literal truncated the line. Both hid real requires: 'const u="http://x"; require("./real.cjs")' returned [], and four real requires in gsd-core/bin/gsd-tools.cjs were invisible. Replaced with a real AST parse via espree. This is ADR-3212's own Decision 4 — tokenizer-first for stateful grammars — applied to the case it describes; comment/string/regex nesting is exactly such a grammar, which is why the regex version was wrong. The function was moved byte-identical out of the #2858 packaging guard, so the bug PRE-DATES this branch and has been a live blind spot there: a shipped script could have required an unshipped path undetected. Fixing it makes that guard strictly stronger than on next. espree is promoted from a transitive eslint dependency to an explicit devDependency rather than relying on hoisting. The script parse attempt sets ecmaFeatures.globalReturn because Node wraps CommonJS bodies in a function, making a top-level return legal — scripts/check-coverage-gate .cjs relies on it, and without the flag the guard throws on a file it is supposed to scan. Verified 0 unparseable across all 324 .cjs/.js under scripts/, bin/, and gsd-core/bin/, and 0 new violations against a real npm pack, so the exact extractor does not newly fail the guard. MEDIUM (security) — the repo-containment check guarded dependencies but not the entry path. One escapesContainment predicate now guards both. LOW (security) — containment was lexical while fs follows symlinks, and a directory symlink could mint a fresh dedupe key per level. realpath now resolves both repoRoot and each dependency before the decision, and the realpath-derived path is the dedupe key. Destination layout still uses the original repo-relative path, so copied trees are unchanged. MAJOR (standards) — the round-1 behavioral rewrite of phase-id tests lost the foreign-prefix contract: every assertion was satisfied by an impl returning [A-Z]+\x2d42, i.e. ANY project code — the exact #3599 bug class the exact-source prevents. The literal assertions it replaced were catching this. Now asserts the compiled regex REJECTS a different prefix with the same number. MAJOR (standards) — the test hand-duplicated production's heading regex with no parity guard (CLAUDE.md's 'Generative Fix Divergence'). Removed the parallel surface instead of policing it: src/roadmap.cts exports buildPhaseHeadingRegex, searchPhaseInContent calls it, the test imports it. Byte-identical .source and .flags verified for both escaped forms. MINOR — '..foo' no longer false-flagged as an escape; the inverted spurious-vs-missing doc claim corrected; the dead allow-test-rule header removed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3412): backfill changeset pr number to 3416 * fix(#3412): make the escape guard's own regex linear, reword an injection-scan collision Two CI failures on PR #3416, both in code this branch added. CodeQL js/redos (high) — REPLACE_CALL_RE's outer alternation let a bracket run be consumed EITHER by the character-class branch OR one character at a time by the trailing catch-all, so a failing match explored both parses of every pair. Measured on the real regex: n=26 -> 204ms, n=28 -> 791ms, n=30 -> 3475ms, a clean 2^n. This script scans repo source, so a file with a long bracket run after '.replace(/' would hang CI outright — a guard against undisciplined pattern construction was itself the worst pattern in the diff. Fixed the way ADR-3212 already prescribes: the catch-all branch now excludes '[' and ']' so a bracket can only be consumed by the class branch (this is what makes it linear), and every quantifier is bounded (the locked bounded-quantifiers decision) as a second line of defense. Now 0ms at n=2000. Disclosed coverage tradeoff, recorded at the constant: a regex literal with a BARE unescaped ']' outside a class is no longer matched by this backstop. No census shape has that form, and the AST rule remains the primary detector. Verified the guard did not go blind doing it: a real census-shape violation is still reported, and an allow-adhoc-regex-escape suppression comment is still honored. Regression test drives the exported findViolations on a 2000-repetition adversarial input and asserts the RESULT. It makes no wall-clock assertion — elapsed-time tests are forbidden — so a regression surfaces as a harness timeout, which is the correct signal. Prompt injection scan — 'must not act as a regex wildcard' in a test comment matched the scanner's jailbreak pattern act\s+as\s+(a|an|if| my). Reworded to 'behave as'. Deliberately NOT allowlisted: silencing a whole test file over one phrase would blunt the scanner permanently, and the comment has nothing to do with injection. Neither failure was reachable from the remote runner — CodeQL and the injection scan are not in that matrix, so the sha it passed was green and still wrong. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
33fca50d8a |
test(#3333): fold the runtime & install surface fix-* cluster — Wave 1 (#3341)
* test(#3333): fold the runtime & install surface fix-* cluster — Wave 1 Folds 11 legacy tests/fix-*.test.cjs regression files into their module's main test suite: 6 folded into existing suites (host-integration-descriptors, effort-surface-axis, trae-imperative-reference, hermes-skills-migration, gsd-agent-isolation-guard), 5 renamed to become the module's sole suite (cursor-hook-workspace-roots, cursor-subagent-isolation, lint-compiled-artifact-sync, hooks-commonjs-marker, shared-hooks-dir-resolution). All 195 test() blocks preserved with zero drops; lint-test-file-count.cjs and eslint remain clean. No production code changed. Wave 1 of 7 in #3315 (H3 of epic #3053). * test(#3333): replace try/finally with t.after() in isolation-guard tests CONTRIBUTING.md bans try/finally inside test bodies (masks failures, not an approved pattern). The fold in the prior commit carried 27 instances forward verbatim from the deleted fix-3045-dispatch-isolation-resolver.test.cjs into an otherwise-clean file. Converts each to the approved per-test t.after() cleanup pattern — same cleanup call, registered instead of finally-wrapped. No assertion, fixture, or test-name change; test( count unchanged at 50. Found by the Standards review pass on Wave 1 (#3333, H3 of epic #3053). * fix(#3333): restore raw NUL byte mangled by the fold in hermes-skills-migration.test.cjs The prior fold commit copied fix-2284-hermes-agent-delegate-task-projection's "collision-robust" test via a text-based Read/Write pipeline, which silently turned a raw NUL byte (0x00) embedded in two string literals into a regular space character. That corrupted the test's actual purpose (proving a NUL byte survives a string-rewrite operation untouched) and produced a genuine gsd-test failure: `24 !== 1` for `out.split(' ').length`, because splitting on a space finds every space in the sentence instead of the single NUL byte the test meant to isolate. Root-caused by diffing the raw bytes (via `git cat-file blob` + `cat -v`) between the pre-fold source and the folded target — confirmed exactly two bytes differ. Restored via a byte-precise patch (latin1 round-trip) touching only those two lines; test( count and every other byte unchanged. * fix(#3333): use \x00 escape sequence instead of a raw NUL byte in test fixture The prior commit restored a byte-exact raw NUL byte matching the original fix-2284 source, and the production function (applyClaudeCodeBrandSwap) was confirmed correct in a standalone repro. But the same raw byte still failed through gsd-test's remote pipeline. Root cause is upstream of gsd-core: some step in that transfer path does not carry a raw 0x00 byte through untouched. A raw embedded NUL byte was never necessary here — `\x00` as a 4-character escape sequence in the source text produces the identical runtime character (U+0000) without ever putting a raw byte in the tracked file, sidestepping any byte-oriented transfer step. Applied at both call sites (the fixture string and the split() delimiter). No behavior change; test( count unchanged at 76. * fix(#3333): harden copyWithPathReplacement against a source file vanishing mid-copy (TOCTOU) Surfaced by this PR's own gsd-test run: tests/install-minimal-hooks.test.cjs and tests/opencode-command-dir-plural.test.cjs intermittently crashed with ENOENT reading gsd-core/workflows/zzz-e5-drift-fixture.md. Root cause is unrelated to test-file consolidation — tests/planning-prompt-drift.test.cjs writes that fixture directly into the real, shared gsd-core/workflows/ tree (main() hardcodes its scan root to the real repo) and deletes it in t.after(); copyWithPathReplacement's readdirSync-then-read loop has no protection against the listed file vanishing before it gets there, so a concurrently-running install path can crash entirely on what is otherwise a completely benign race. Fixed by skipping (not crashing on) a listed entry that no longer exists by the time the loop reaches it. Added a regression test that deterministically reproduces the race (readdirSync snapshot still lists the file; it is deleted immediately after) and proves both outcomes: no throw, and the vanished entry's destination is never partially written. Per CLAUDE.md's no-defer rule, a defect surfaced while verifying this PR is fixed inline rather than deferred — this overrides one-concern-per-PR. * fix(#3333): fix third NUL-byte-mangled occurrence missed by prior fix passes The fold originally mangled three raw-NUL-byte occurrences to spaces, not two — the earlier byte-restore and escape-sequence commits both only targeted the fixture string and the split() delimiter, missing out.includes('[ ]') a few lines below (should read out.includes('[\x00]')). A remote gsd-test run kept failing on this exact assertion even after both prior fixes, which is what surfaced the miss. Verified via a standalone repro using the file's real (not retyped) fixture content: all six assertions in the collision-robust test now pass. Zero raw NUL bytes remain in the file; test( count unchanged at 76. * chore(#3333): add changeset for the copyWithPathReplacement TOCTOU fix Fixed-type fragment for the production defect fixed inline in this PR (bin/install.js's copyWithPathReplacement). Exempt from docs/ requirements per CONTRIBUTING.md (only Added/Changed/Deprecated/Removed require it). * chore(#3333): backfill changeset PR number (pr:0 -> pr:3341) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7a7bf19fc1 |
enhance(#2872): record scope and runtime in the install manifest (#3323)
* enhance(#2872): record scope and runtime in the install manifest gsd-file-manifest.json gains manifestVersion, runtime and scope, and a new read-only Installed Surface Resolver Module reads both install scopes for a runtime in one call -- the first code path in the repo that does. Phase 3 of epic #2866 (ADR-2866). Blocks Phase 4 (#2873), which resolves #2218: the resolver's shadowedBy field is that defect expressed as a value for the first time. It ships computed-and-unread here. Installed-ness is decided by manifest PRESENCE, never by the new fields, so a manifest written by an older GSD stays fully functional and no user needs to reinstall. Recorded runtime/scope are corroboration: a disagreement with the probed config dir is reported as declaredScopeMatchesProbe: false, never silently corrected. readInstallManifest is widened additively -- version/timestamp/mode/files keep their exact names, types and meanings for all four existing callers. manifestVersion is a new field rather than a reinterpretation of version, which holds the package version and is read by the golden-parity fixtures. Stems are derived from the installed manifest's own file keys, the inverse of Phase 2's filename composition, guarded by a fast-check round-trip property plus a kebab-case charset check so a crafted manifest key cannot put a traversal segment, control character or ANSI escape into a trigger that Phase 4 renders back to the user. Also fixes two defects found while working: - bin/install.js hardcoded manifestVersion: 2 while the reader owned MANIFEST_SCHEMA_VERSION = 2. Now single-sourced, with a parity test. - docs/installer-migrations.md documented an install-state schema of five snake_case fields that have never been written; InstallState has only ever been { schemaVersion, appliedMigrations }. Corrected with a dated note. Verification runs on the remote runner. * fix(#2872): fold review findings from three independent engines Standards axis: - convert the manifest-schema suite from a hybrid setup(t) closure to beforeEach/afterEach (CONTRIBUTING.md:319-354 Pattern 1). The hybrid was neither approved pattern and a new test forgetting the call got no warning. - SCOPE_ORDER was declared twice with no parity test -- this repo's recorded generative-fix-divergence class. Give the ordering one owner: install-scope exports it frozen, the layout module and the resolver both import it, and a test locks it against scopeRank so the constant and the ranks cannot drift. - drop the defaultReadManifest passthrough (Middle Man). Spec axis: - add the VOLATILE_FILES exclusion test and source comment the acceptance table promised and did not deliver. gsd-file-manifest.json stays excluded: the new fields are deterministic, but timestamp -- the original reason -- is unchanged. Security axis: - bound the reported manifest runtime at 64 chars, matching the truncatePostureValue convention already used in this subsystem. It reached declaredRuntime unbounded while the adjacent stems were gated by SAFE_STEM; an inconsistent posture on the same attacker-influenceable document. The charset stays ungated on purpose -- declaredRuntimeMatchesProbe needs to see the real value -- so Phase 4 must sanitize before rendering, recorded in the design's Known limits. Both new parity tests were verified to FAIL when the two sides are made to disagree, then pass again on revert. Verification runs on the remote runner. * chore(#2872): backfill changeset pr number to 3323 * fix(#2872): give git fixture construction its own timeout class PR #3323's full test (windows-latest, 22, shard 2/3) failed with gitOrThrow: 'git init' failed -- outcome=timed_out exitCode=null gitOrThrow: 'git commit --allow-empty' failed -- outcome=timed_out from drift-detection.test.cjs's beforeEach, a file this branch never touched. Every other lane passed the same commit, including windows-latest node 24 on all three shards, and next is green. Root cause is a bound sized for the wrong class. DEFAULT_GIT_TIMEOUT_MS is 15000 and its own comment scopes it to plumbing READS -- rev-parse, branch, log -- against an existing repo. createFixture uses it for six sequential repo-CONSTRUCTION spawns: init, three config writes, add -A, commit. init and commit each write dozens of files, and on Windows every spawn is Defender-scanned. Sibling tests in the failing block took 15.6-22.0s against a 15000ms bound. This repo already diagnosed this exact shape once: timeouts.cjs's HOOK_FANOUT_TIMEOUT_MS records PR #3285 failing in the SAME job with the SAME outcome=timed_out exitCode=null signature at the SAME bound while every other lane passed, and concludes 'a bound sized for the wrong class, not a slow machine'. It was fixed by splitting out a heavier class-norm at 60000. Same remedy here: GIT_FIXTURE_TIMEOUT_MS = 60000, 4x the bound that failed and half INSTALL_TIMEOUT_MS. DEFAULT_GIT_TIMEOUT_MS deliberately stays at 15000 -- a blanket raise would stop a genuinely hung plumbing read from surfacing fast. Verified the value reaches the spawn rather than being an ignored option: spawnSync was monkeypatched before requiring the fixture module, and all six git construction calls were captured carrying timeout: 60000. This branch's two new test files shift shard composition, which is how a pre-existing fragility landed in the heaviest shard on the slowest lane. Fixed here rather than deferred, per the no-defer rule. Verification runs on the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c2f24265f2 |
feat(#2870): resolve install scope as a value (#3278)
* test(#2870): failing-first suite for the Install Scope Module 19 tests over the 50-test-matrix rows 1-19. RED by construction: the module under test does not exist yet, so the suite fails at require with MODULE_NOT_FOUND until src/install-scope.cts lands. Every row asserts a returned value with injected env/home/existsSync -- no filesystem, per the issue's acceptance criterion that tests assert the resolved value directly. Row 7 asserts the RELATION rank(global) > rank(local) rather than a literal, so Phase 2 (#2871) can re-base the numbers without a fixture edit. Row 4 iterates the real runtime registry rather than a hardcoded list, excluding vscode, which declares configHome.kind none and is never CLI-installed. * feat(#2870): add the Install Scope Module Scope becomes one resolved value instead of a bare string re-derived at every layer. resolveScope({id, runtime, ...}) returns {id, configHome, settingsFile, consentRequired, hostPrecedenceRank}. It COMPOSES resolveConfigHomeFromDescriptor rather than extending it. That function has 60 dependents across 13 files and 2 process flows -- a CRITICAL blast radius -- so adding a scope parameter to it, which the issue's framing invites, would ripple through all of them. Composing costs nothing and leaves every existing caller byte-identical. The module owns the InstallScope type name, which previously lived privately in runtime-artifact-install-plan.cts; that module now imports it. A fifth spelling of the same concept would have defeated the phase. settingsFile is null for the 18 runtimes that declare no settingsFileByScope -- absence is a value, not an error, and inventing a Claude-shaped default would leak that host's shape onto every other one. hostPrecedenceRank ships unread: Phase 2 (#2871) is its first consumer. It is carried as data only, per this issue's out-of-scope note that precedence semantics belong to that phase. Vocabulary: the install axis standardizes on local. ConsentRecord.scope keeps project deliberately -- that literal is serialized into consent records in the user's home, and renaming it would silently deactivate every project-scoped capability on the machine. CONTEXT.md records the boundary mapping instead. Every environmental input is injectable (env, home, existsSync, cwd), so the resolved value is assertable with no filesystem at all. Registration ripple: .gitignore, eslint.config.mjs, CONTEXT.md glossary, docs/INVENTORY.md, and the inventory manifest (regenerated after build:lib, never before). Verified via the remote runner. * refactor(#2870): route scope re-derivations through the module bin/install.js resolves scope once per function instead of inline at each of its 12 sites, and the settingsFileByScope consumer reads it through resolveScope(). Seven downstream boolean re-derivations now call the module's isGlobalScope() instead of comparing the literal independently: runtime-artifact-install-plan, both runtime-artifact-layout kind builders, dispatchKindEntry, surface, and two install-engine sites. The fifth through seventh were not named in the issue -- they are the same re-derivation class, and leaving them would have made the acceptance criterion false. _computePathPrefix keeps its isGlobal boolean API, so the projection is centralized rather than eliminated. resolveScope and isGlobalScope share one validator, so the two surfaces cannot drift. TWO SITES DELIBERATELY NOT ROUTED: runtime-artifact-conversion's rewriteStagedSkillBodies and rewriteStagedCommandBodies. ADR-1508 fixes the direction as installer/layout -> conversion, never upward, and install-scope composes runtime-homes, so importing it into the conversion module would invert that direction. Left as-is on purpose. Behavior-preserving throughout. Each step was proven by capturing full layout and plan output -- including every kind's home field and the hashed contents of emitted files -- before and after, across both scopes for claude, codex, opencode, hermes, kimi and kilo. Byte-identical. surface.cts keeps a scope ?? 'global' default before the call because Layout.scope is optional there; isGlobalScope throws where the old inline compare returned false, and that difference would have been a placement regression. Verified via the remote runner. * fix(#2870): cover the no-config-home throw and document the strictness Two findings from the isolated adversarial review. The vscode case was implemented but untested. resolveScope throws for a runtime whose descriptor declares configHome.kind 'none', which is the design's own behavior-table row 13, but the registry sweep excluded vscode rather than asserting the throw -- so the behavior shipped with no test. The exclusion is now legitimate because the case has its own test naming the runtime in the assertion. isGlobalScope throws where the inline compare it replaced returned false. No reachable caller can deliver an out-of-union value today, but the types are not enforced at runtime, so a future caller passing an optional Layout.scope would crash rather than silently misroute. That is the better failure -- misrouting writes artifacts to the wrong place -- but it was undocumented, so the reason is now on the function. Adds the changeset the acceptance criteria require. * refactor(#2870): route the last two sites; correct the ADR-1508 claim The previous commit declined to route runtime-artifact-conversion's rewriteStagedSkillBodies and rewriteStagedCommandBodies, claiming ADR-1508's dependency direction forbade the import. That reasoning was wrong, and this commit corrects it. Two independent reviewers checked the actual import graph: runtime-artifact-conversion already imports capability-registry, command-roster, runtime-name-policy and shell-command-projection -- it depends on leaf-tier siblings today. install-scope imports only runtime-homes plus node builtins, and runtime-homes imports only node builtins, so there is no cycle at any depth. ADR-1508 governs the installer/layout to conversion boundary, not a leaf-to-leaf sibling import of the same shape conversion already makes. With those two routed, every isGlobal re-derivation in the tree now goes through one owner and acceptance criterion 1 is fully met rather than partially. Nine sites, not the four the issue enumerated. Also from the review: Tests were falling through to the real process.cwd() at five local-scope call sites, which contradicts the acceptance criterion that the resolved value be assertable with no filesystem. Every one now injects a cwd. One of the five was a site the review had not spotted. bin/install.js carried two near-identical copies of the guarded resolveScope block, one in install() and one in uninstall() -- duplicated scope logic in the phase whose purpose is removing it. Extracted to one helper, and the new sites use the file's existing ternary idiom rather than the if/else that replaced it. Equivalence re-proven across both scopes for claude, codex, opencode, kilo and hermes, now including the staged skill and command body rewrites hashed per file, since those decide the literal spec-root path baked into every emitted artifact. Byte-identical. Verified via the remote runner. * fix(#2870): assert configHome portably instead of with a native separator The windows-latest node24 shard failed on two install-scope assertions. The module was right and the tests were wrong: they built their expected value with path.join, which emits \fake\home\.claude on Windows, while resolveScope normalizes separators unconditionally to /fake/home/.claude. That unconditional normalization is deliberate -- backslash paths arrive on Linux too, so normalizing via path.sep is the documented defect this repo guards against. Weakening it to make the assertion pass would have inverted the fix. Every path.join-built expectation in the suite now goes through toPosixPath from tests/helpers.cjs, which is the pattern the no-path-literal-in-assert rule's own valid-case list sanctions. It splits on the running platform's path.sep and rejoins with forward slashes, so it reverses whatever path.join produced on that same platform and the expectation is invariant everywhere. Two more call sites had the same latent problem and passed on Linux and macOS by luck; they are fixed too. This is the class of defect the remote runner structurally cannot catch -- its matrix is Linux-only, so a green pass there is not evidence of portability, and CI's Windows lane is the only place it surfaces. Verified via the remote runner. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
0c413bbc9c |
chore(#3059): close the ESLint glob-coverage escape and guard it (#3277)
* chore(#3059): close the ESLint glob-coverage escape and guard it 62 tracked source files matched no `files:` glob in eslint.config.mjs, so ESLint skipped them entirely while `eslint .` still exited 0 — including all 26 files under hooks/, the enforcement machinery itself. Covers 56 of them (eslint-rules/, hooks/, bin/lib/, pi/, examples/, vscode/, the plugin shims, root *.mjs) and allowlists the 6 deliberate must-not-compile brand-typing fixtures with a recorded reason each. hooks/** is covered with n/no-process-exit deliberately off: a hook's whole contract is its exit code, several exits are load-bearing stdin-timeout guards where nothing else terminates the process, and ADR-0012/0174 scope the no-process-exit convention to the Command Routing Hub. bin/lib/ui-safety-gate.cjs is dual-mode, so it keeps the rule live and takes two targeted disables in its require.main===module tail instead. Adds scripts/lint-eslint-glob-coverage.cjs + a node:test drift guard so the class cannot regrow: allowlist entries require a non-empty reason, the list ratchets down only (a stale entry fails), and a tracked-count floor means a broken `git ls-files` fails rather than reporting clean. Closes #3059 * chore(#3059): apply review findings — correct the changeset count, add parser properties Isolated adversarial review, confirmed by rebuilding a byte-for-byte replica of the pre-change eslint.config.mjs: the changeset claimed 44 previously- unlinted files. The real figure is 56 (56 covered + 6 allowlisted = 62). That was user-facing CHANGELOG text and was wrong; corrected, along with three consequential figures in the design record. CLAUDE.md requires a fast-check property test for parsers and budget limits, and listTrackedSourceFiles is a parser. The standards review called this "satisfied in spirit"; it is not. Adds three properties driving the real exported parser through an injected execFile: extension totality/soundness including a trailing terminator, backslash-normalization totality, and CRLF/LF equivalence — the invariant the repo's recurring CRLF defect class breaks. Also de-duplicates the anchor rows onto one shared resolver, kept deliberately independent of the guard's own resolveFileCoverage so an anchor still fails if that resolution regresses, and records in the guard's header why the bin/install.js family is NOT allowlisted: it resolves to 2 rules under ADR-1703, so an entry would trip the allowlist_stale ratchet. * fix(#3059): make the coverage guard's git call container-safe The remote runner reported the guard degrading to `git_failed` on both Node lanes: fatal: detected dubious ownership in repository at '/work' The runner executes in a container where the repo is owned by a different UID, so git refuses to operate on it. The guard's degraded-verdict path worked exactly as designed — it reported the failure instead of throwing or falsely reporting clean — but a guard that cannot run in CI is not a gate. `git ls-files` is now invoked as `git -c safe.directory=* ls-files`. `-c` scopes the override to the single invocation and mutates no config file, and the wildcard is appropriate because this command only enumerates tracked paths in the repository it is already executing inside. Adds a regression test that captures the argv through the injected execFile seam and asserts `-c safe.directory=*` precedes `ls-files`, so the container case is pinned behaviorally rather than by reading the script's source. * chore(#3059): backfill changeset PR number pr:0 placeholder replaced with the real number now that #3277 exists. --------- 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
|
4b69dc346b |
fix(#2725): repoint the pre-commit alias guard at sources git can actually stage, drop nine dead ones (#3273)
* test(#2725): failing-first coverage for the inert pre-commit alias guard Replaces two stale tests that asserted on `sdk/src/query/command-manifest.phase.ts` — a path retired with the SDK boundary (ADR-0174), so both passed forever while guarding nothing. The new matrix drives .githooks/pre-commit through its GIT_OVERRIDE/NPM_OVERRIDE seams and asserts on the real tracked sources the drift checker reads. Red until the guard is repointed off the gitignored build outputs it currently watches. * fix(#2725): repoint the pre-commit alias guard at sources git can actually stage `.githooks/pre-commit` carried ten staged-path guards and not one of them could do its job. Nine anchored on `^sdk/…`, a tree retired by ADR-0174, and invoked npm scripts that no longer exist (`check:state-document-fresh` and eight siblings). Their only reachable behavior was to abort the commit with `Missing script` — which required matching a path that cannot exist, so they were dead twice over. The tenth was the real defect. Its npm target does exist, but it matched `^gsd-core/bin/lib/command-aliases\.cjs$` — a gitignored build output (.gitignore:172). An ignored path never appears in `git diff --cached --name-only`, so the guard was not stale, it was structurally unmatchable: it watched the derived layer instead of the source layer, from the day it was written. Repointed at the nine tracked `src/*.cts` sources `scripts/check-alias-drift.cjs` actually reads. The family table moves to `scripts/lib/alias-drift-families.cjs` so the checker and the hook derive their surface from one list, and a new parity test fails if a family is added to the checker without the hook learning to watch its source — the rot mechanism, not just this instance of it. Matching is now `grep -Fxqf` (fixed strings, whole line): exact path equality, no regex anchors to get wrong as the list grows. Staged paths are collected into a variable before matching, because `grep -q` exits on first match and would SIGPIPE its upstream, which under `set -o pipefail` turns a successful match into a non-zero pipeline status. The two CONTRIBUTING.md recipes pasted copies of the hook bodies inline — a third parallel surface, and one that had already drifted: the pre-commit copy carried the same dead `sdk/` patterns, and the pre-push copy would have overwritten the committed hook with a paraphrase that drops the GIT_OVERRIDE seam its test drives. Both now just point git at the committed files. The pre-push recipe's `'@example-corp\\.com$'` also never matched anything — inside single quotes bash keeps both backslashes. `.githooks/pre-push` was audited for the same rot and has none: it keys on no paths, no-ops unless GSD_BLOCKED_AUTHOR_REGEX is set, and is covered. Unchanged. Hooks stay opt-in. Nothing registers `core.hooksPath` for you, per CONTRIBUTING.md's documented one-time setup. * test(#2725): make the hook/checker parity assertion bidirectional Review finding from the isolated adversarial pass: the parity row only caught the hook UNDER-watching relative to scripts/lib/alias-drift-families.cjs. Drop a family from the module and the hook would keep watching its source with nothing to notice — the same divergence class this change exists to close, just pointed the other way. The new row takes every `src/*-command-router.cts` on disk as the universe and asserts the hook stays silent for the eight routers the drift check does not read. Both directions are now covered by running the real hook, not by comparing two lists in the test. Also corrects a CONTRIBUTING.md overclaim caught by the standards pass: 9 of the 11 watched paths derive from the module, not all 11 — bash cannot require a CJS module, so the hook carries literals and the test is what binds them. * fix(#2725): ship the shared family table and fix the mock that hid its own rows Three defects the remote runner caught that local probing did not. The mock `git` in the regression test emitted its staged-path payload as `printf '%s' "src/command-aliases.cts\n"`. Bash does not expand `\n` inside a double-quoted string and printf does not expand escapes in a `%s` argument, so the mock produced one unterminated line containing a literal backslash-n. No whole-line match could ever succeed, and every row that expects the hook to FIRE failed while every row that expects silence passed — which is exactly the signature the run reported: 9 failures, all of them fire-expecting rows. The payload now goes through a file the mock `cat`s, which is byte-exact and is what makes the CR-terminated and empty-staged-list rows mean what they say. `scripts/lib/alias-drift-families.cjs` was not enumerated in `GSD_SCRIPTS_LIB_FILES` (bin/install.js:377), so the installer never copied it. That is not cosmetic: `scripts/check-alias-drift.cjs` ships, and it now requires this module — an installed tree would have failed with MODULE_NOT_FOUND the first time a consumer ran `check:alias-drift`. Added to the manifest, which is what the #3184 install/uninstall parity tests assert against `readdirSync`. Regenerated the 19 committed install-tree fixtures via `npm run gen:install-tree` to record the new emitted path. The diff is +1 line per fixture and nothing else. `npm run lint:ci` exits 0. The earlier claim that `scripts/` is outside the emitted surface was wrong: `scripts/lib/` is copied into every runtime's install tree, which is why 19 golden-install-tree cases moved. * chore(#2725): backfill changeset pr: 3273 --------- Co-authored-by: sim <sim@local> |
||
|
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
|
3f349e551d |
fix(#3024): route sync-skills through the shipped gsd-tools instead of an unshipped install.js (#3195)
* fix(#3024): sync-skills workflow uses gsd-tools query skills-root instead of unshipped install.js The sync-skills workflow Step 2 shelled out to gsd-core/bin/install.js --skills-root, but install.js is not shipped in installed trees (only in the npm tarball root bin/). Every /gsd-update --sync invocation failed with MODULE_NOT_FOUND. Fix: added 'gsd-tools query skills-root <runtime>' subcommand (gsd-tools IS shipped) that calls the same getGlobalSkillsBase function install.js used. Updated the workflow to call gsd_run query skills-root instead of the dead install.js path. Also documented the #3025 verbatim-cp limitation in Step 5 with a workaround. * test(#3024): failing-first guards for the three defects in the adopted fix The cherry-picked commit came from an aborted run that never executed its own tests. Its raw-path assertion fails as written, which is the clearest evidence the work never reached verification. Covers: - --raw must emit a bare path, not JSON (output() takes a third rawValue arg that routeSkillsRoot omits, so the raw branch never fires) - an unknown, empty, whitespace, traversing, or metacharacter-bearing runtime must be rejected, not silently resolved to claude's skills root - sync-skills.md must contain zero references to the unshipped install.js, including the guard's remediation text — the issue's second reported defect - parity across every runtime in the registry, not three hardcoded ones, so the two entry points cannot drift Also converts the adopted tests off a hand-rolled spawnSync onto the bounded process seam, per CONTRIBUTING. Fails before the fix. Verified via the remote runner. * fix(#3024): make the skills-root query actually work and reach non-Claude runtimes The cherry-picked commit never ran its own tests. Six defects, all fixed here. --raw was ignored: output() is output(result, raw, rawValue) and the third argument was omitted, so the raw branch never fired and the workflow captured a JSON blob as SRC_SKILLS_ROOT. Every downstream cp -r then resolved against a nonexistent path — the command would have shipped still broken. An unknown runtime silently resolved to claude's skills root, because getGlobalSkillsBase falls back rather than returning null, leaving the existing === null guard dead. The runtime id is now validated at the CLI boundary against the shipped registry, so a typo'd --from/--to fails instead of reading from or writing into the wrong runtime's tree. getGlobalSkillsBase('vscode') threw a raw TypeError. vscode is non-installable by descriptor, so it has no skills root — null is the answer, not a crash. The resolver now short-circuits configHome.kind 'none', which also fixes the same latent crash in install.js --skills-root vscode. Every caller already gates on === null. sync-skills.md used gsd_run WITHOUT the canonical launcher preamble, so gsd_run was undefined on non-Claude runtimes — the fix would have been dead in exactly the place the original bug bit. Preamble propagated via sync-runtime-launcher. Also registers skills-root in TOP_LEVEL_USAGE (the help/dispatch parity guard caught it), removes the last two install.js references including the guard's remediation text (the issue's second reported defect), and updates the stale assertion that still described the removed contract. Verified on the remote runner. * fix(#3024): align the documented runtime list with the registry and gate both entry points Isolated review returned BLOCK on two findings. The workflow's Supported-runtimes list and its --to all expansion named grok and gemini, neither of which is a registered runtime. Once this branch added validation, --to all — a documented first-class feature — aborted. The list was hand-copied prose shadowing the registry, so correcting it alone would drift again; a parity assertion now fails in BOTH directions if the doc and the registry disagree. vscode is excluded by name: it is installSurface 'none', so syncing skills to it is meaningless and would abort. bin/install.js --skills-root reached getGlobalSkillsBase with no own-property gate, so --skills-root __proto__ silently resolved to claude's skills root. This branch had just hardened the OTHER entry point to the same function; leaving one of two parallel surfaces open is the same divergence class as the first finding. Both now call one shared isRegisteredRuntimeId() rather than a copied check, and the parity test covers the hostile ids so the two can never disagree again. Also guards the workflow's root resolution: neither command substitution checked its exit status and only the source had an existence guard, so a failed destination resolution left DEST_ROOT empty and turned rm -rf "$DEST_ROOT/$SKILL" into an absolute path at filesystem root. Both resolutions are now checked, and Step 5 requires both roots to be non-empty and absolute before any destructive command. Verified on the remote runner. * test(#3024): anchor the runtime-list parity extractor to the list span The extractor captured (.+) to end of line, so it swallowed the em-dash prose that explains the vscode exclusion — and that sentence contains backticked `runtimes` and `null`, which is where the three phantom ids came from. The documented list was correct; the test was reading its own explanation back as data. Anchored to the id-list span. Both directions still fail as intended: proven by injecting a bogus id and by removing a registered one. * test(#3024): anchor the --to all extractor and fail loudly on empty captures The workflow has three TO_RUNTIMES= assignments and the regex matched the first one — an empty array initializer at line 28 — so the extractor captured nothing and the assertion diffed [] against 18 ids as if that were data. That is the same failure twice, so the fix is the general one: every extractor in this test now asserts it captured a plausible list before comparing, naming which extractor found nothing and what it was looking for. An extractor that silently yields [] is a confident wrong answer, and a parity guard that reports it as a data mismatch teaches the reader to loosen the assertion. Verified against the real workflow and against doctored copies with each target construct removed, plus both teeth directions. * fix(#3024): merge duplicate process-seam import after rebase The rebase applied cleanly but left runNode declared twice: next had gained its own import of the seam while this branch added one carrying OUTCOME. A clean rebase is not a correct one — the file no longer parsed. Merged into a single import providing both. * fix(#3024): bind DEST_ROOT per destination instead of a dangling map Step 2 stored each destination's root into DEST_SKILLS_ROOTS, which nothing ever read, while Steps 3 and 5 used a scalar DEST_ROOT that nothing ever assigned. The array was also never declare -A'd, so on bash 3.2 — macOS system bash, which this repo supports — every destination collapsed onto index 0. The absolute-path guard added earlier was the only thing standing between that and rm -rf "/$SKILL"; it turned a silent disaster into a hard stop, but the feature still could not complete. Each destination now binds its own DEST_ROOT where it is used, and the unread map is gone rather than replaced. Step 2 keeps eager validation, so a bad runtime id in a multi-destination --to aborts before any destination is written rather than after some already have been. Verified on bash 3.2 with a two-destination run binding distinct roots, and with a bad id aborting before any destructive call. * fix(#3024): restore grok support broken by the registry gate The registry gate added earlier rejected grok, and that was my error. I confirmed grok was absent from the capability registry and concluded the hardcoded branch was dead — without checking what it resolved to. It resolves to ~/.agents/skills, a real grok-specific path, exactly as the pre-fix workflow documented ('grok uses the ~/.agents layout'), and there is a support discussion doc for it. So a working, documented runtime silently lost --skills-root and sync-skills support as a side effect of prototype-pollution hardening — and the parity test I added locked that in as correct. gemini is the one that really was dead: it fell through to CLAUDE's skills root, so rejecting it is right and it stays rejected, as do bogus ids, __proto__, empty, whitespace and traversal. The validator's real question is 'does this id have a genuine runtime-specific resolution', not 'is it in the registry map'. Registry membership was a proxy that happened to miss grok. Legacy non-registry runtimes with dedicated resolution branches are now a named, documented set; enumerating every hardcoded branch in getGlobalConfigDir against the registry confirms grok is the only one. The new tests assert grok resolves UNDER .agents and specifically not to claude's root. Allow-listing an id proves nothing about whether it resolves correctly — that assertion is what would have caught my mistake. Also uses the shared PROBE_TIMEOUT_MS instead of a duplicate literal, and guards Step 3's DEST_ROOT re-resolution, which contradicted the file's own stated guarantee. Verified on the remote runner. * test(#3024): guard against LEGACY_NON_REGISTRY_RUNTIME_IDS drifting The named legacy set is a second hand-maintained proxy for the same predicate the registry check got wrong — 'does this id resolve runtime-specifically'. Nothing stopped a third hardcoded branch being added to getGlobalConfigDir without updating the Set, reproducing the exact class of bug that broke grok. Production stays explicit and greppable; the test derives the truth instead. It resolves a sentinel id to learn the generic fallback, classifies every candidate against it, and fails in both directions — an id resolving runtime-specifically that is in neither the registry nor the Set, or a Set entry that no longer earns its exemption. The failure message names the remedy. Confirms grok resolves runtime-specifically and gemini does not, which is the distinction the original registry check could not see. Also reverts the shared-timeout swap: SKILLS_ROOT_PROBE_TIMEOUT_MS is pre-existing on next and arrived by rebase, so changing it here was scope creep into another issue's territory. Verified on the remote runner. * chore(#3024): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
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> |
||
|
Tom Boucher
|
0e6fa2e2cf |
enhance(#3118): close the dead injectables and the shell projection follow-on — Wave 4 (#3124)
* test(#3118): failing-first coverage for the dead injectables and the shell projection Adds the counter-tests Wave 4 closes against, before any fix: - antigravityWatermark had zero test references. The four existing tests that look like watermark coverage hand the fallback a literal mark and never call the producer, so nothing pinned whether a real run's mark is correct. Covers all six branches plus the non-object cache classes. - Pins the fail-open: a transcript read that throws reports lines:0, indistinguishable from a genuinely empty transcript, and the consumer then replays a previous run's review as this run's. - Pins the export-line escaping across the repair, persist and win32 bash lanes, including the parity assertion that they must not diverge. - sliceCurrentPositionSection: empty-vs-absent, fenced heading, second occurrence, H3, CRLF. - Proves deps.progressProvider is inert by supplying a throwing stub to all ten transition intents. Verification through the remote runner only. Refs #3118 * fix(#3118): distinguish an unreadable transcript from an empty one antigravityWatermark's final read can throw on a transcript that indisputably exists. It returned lines:0, which is the same value a genuinely empty transcript produces, so the caller could not tell the two apart. antigravityTranscriptFallback derives its skip from that count. A mark of {convId:'c1', lines:0} for a conversation that pre-dates the run makes it skip nothing and return the last PLANNER_RESPONSE in a transcript written before this run started — a previous review presented as this one's, which is exactly what the function's own 'never stale' docstring promises cannot happen. The unreadable case now sets unreadable:true and the fallback declines for a same-conv-id unreadable mark. An absent or empty transcript is untouched: those genuinely have zero prior lines. * fix(#3118): escape the export line for the file it lands in, not the echo Three lanes emit export PATH="<dir>:$PATH". repair escaped it with escapePosixDoubleQuoted; persist and the win32 Git Bash lane escaped it with escapeSingleQuotedShellLiteral instead. The single-quoting is correct for the echo, so nothing runs when the user pastes the command. But the bytes appended to ~/.bashrc are the export line itself, and inside double quotes in an rc file a $(...) or a backtick in the directory name is command substitution that runs on every new shell. Those characters are legal in a path on both POSIX and Windows, so the path was reachable. projectPathExportLine is now the single source of that line and escapes for its final rc-file context; each lane still applies its own transport escaping on top. fish keeps the single-quote escaper — its value really does stay single-quoted. The cmd.exe lane interpolated into a cmd double-quoted string with no cmd-level escaping, so a quote closed the region and &cmd& ran. A quote is reserved on Windows and cannot appear in a real path, so there is no correct command to suggest: the win32 lanes now fail closed for one. Metacharacter-free paths render byte-identically on every lane. * fix(#3118): drop a stray carriage return and a deps field nobody reads locateCurrentPosition subtracted a fixed one byte to exclude the newline before the next heading, which assumes LF. On a CRLF document the slice kept an unpaired trailing carriage return. It now walks back over the newline and over a preceding carriage return if there is one. StateTransitionDeps also required a progressProvider that 33 sites supplied and no site ever called. A required field nothing reads widens the module's interface without changing its implementation, which is the shape epic #3051 cites as its reason for refusing blanket injection. Removed along with the ProgressRecord alias that existed only as its return type; state-document.cts's unrelated interface of the same name is untouched. * fix(#3118): stop an empty span duplicating bytes, and name the empty results Three findings from the isolated review pass. locateCurrentPosition could return end < start when the section was empty and the next heading followed with no blank line between. Every mutator splices with slice(0,start) + body + slice(end), so an inverted span duplicated the region between them — a blank line silently inserted into STATE.md on every transition, two bytes on CRLF. The span is now clamped, and an empty section is a zero-length span, which is what it always meant. The win32 fail-closed path left the installer printing 'Add it with one of:' with nothing under it. An empty shellActions folded two different facts together, so projectPathActionProjection now carries a frozen PATH_ACTION_REASON and the installer branches on it. Two empty results with different causes staying distinguishable is the subject of the epic this belongs to. fish_add_path parses a leading dash as an option, so a directory named -v printed 'No paths to add' instead of being added. Verified against fish 4.8.1: the end-of-options separator fixes it. Replaces the console-prose test the second fix first arrived with — a regex over captured stdout is what CONTRIBUTING prohibits, and the typed reason is the surface it asks for instead. * fix(#3118): escape TOML control characters, and stop a test name overstating Five findings from the two review axes. escapeTomlDoubleQuotedString escaped only backslash and quote. TOML basic strings also require U+0000-U+0008, U+000A-U+001F and U+007F to be escaped, so a value carrying a raw newline or NUL wrote a config.toml no parser accepts — rejecting the whole file, not just that value. Four of its call sites write real config. Tab stays raw; the grammar exempts it. The byte-identity test claimed every lane was unchanged for an ordinary path, which is false: fish now takes the end-of-options separator on every path, not only hostile ones. Renamed, and the one intended delta now has its own named test instead of hiding inside a claim that read as broader than it was. Also: exact-equality assertions in place of substring checks that could pass on a subtly wrong escape, newline and null-byte cases for all five quoting primitives, and a temp dir registered with t.after so it is removed when an assertion fails. * docs(#3118): add the changeset fragments * fix(#3118): degrade instead of throwing on a null conversation cache A cache file whose whole content is the literal null — what a truncated or zeroed write leaves behind — made both antigravityWatermark and antigravityTranscriptFallback throw. JSON.parse('null') succeeds, so the try/catch wrapping the parse never fired, and resolveConvId then called hasOwnProperty on null. Both functions advertise the opposite; the existing test next to them is named 'a missing cache or transcript degrades to empty, never throws'. Parsing successfully is not the same fact as the payload being usable, and a guard that only wraps the parse cannot tell them apart. resolveConvId is now total for any non-object input, so one guard covers both callers. Caught by the null case in this wave's own cache matrix. * test(#3118): correct a stale fish expectation and a parity comparison The pre-existing 'POSIX persist mode escapes single quotes' test pinned fish_add_path without the end-of-options separator this wave adds, so it asserted behavior that is no longer correct. A repo-wide scan found one such hardcoded expectation; every other site derives its expectation from the projection. The new parity test compared the token from a POSIX path against the win32 lane, which posix-normalizes its input first — two different inputs, so the tokens differed for a reason that had nothing to do with the parity it claims to check. It now derives the win32 expectation from the same input the lane receives. * docs(#3118): reword a comment the injection scanner reads as an instruction The scanner pattern act\s+as\s+(?:a|an|the)\s+ carries no word boundary, so 'the same fact as the payload' matched on the tail of 'fact'. Reworded per the documented remedy for this collision. The missing boundary is a scanner defect rather than a prose problem — any contributor writing 'fact as the' trips it — but the pattern is gate plumbing, which the sibling epic owns, so it is surfaced rather than changed here. * chore(#3118): backfill changeset pr number to 3124 * chore(#3118): backfill changeset pr number to 3124 * fix(#2784): make the negation scan single-pass and index it correctly Three defects in the negation suppression added by #3127, all in one block, none of which had a test. The pair scan was verbs.some(nouns.some(...)) with a slice and a split per pair, so it grew cubically with clause length: 1.1ms before that PR and 8462ms after, on 800 verb+noun pairs in one clause. api-coverage's property test generates documents large enough to reach the runner's 600s file cap, which is why it hangs as 'fail 0, cancelled 1' rather than failing an assertion. Every (verb, noun) window is a subset of the single widest one, so one scan of that window answers the same question in a linear pass. Verified equivalent against the old predicate over 20,000 generated clauses. Both checks also subtracted clause.start from offsets that collectTerm- Matches already returns clause-local. The first clause on a line has start 0 so it worked there and nowhere else: later clauses went negative, and slice reads a negative index from the end, so suppression silently examined unrelated text. The comment claimed 'without any API integration' was suppressed. It is not — the qualifier sits outside the two-word lookback and the noun precedes the verb. Widening the window would trade a false positive that costs one declaration line for a false negative that slips a real integration past a blocking gate, so the behavior stands and the comment now says so. Pinned by a test. The qualifier sets were also rebuilt for every line of every document. |
||
|
Tom Boucher
|
b181c2f8c3 |
fix(#3039): clamp max/xhigh effort to high for Claude-runtime skills (#3119)
* fix(#3039): clamp max/xhigh effort to high for Claude-runtime skills effort: max in plan-phase, execute-phase, and autonomous SKILL.md frontmatter was passed through as output_config.effort, which the Anthropic API rejects when extended thinking is disabled (400: effort 'max' is not supported when thinking is disabled on this model). The frontmatter is static at install time and the installer cannot know whether thinking will be on or off at invocation. normalizeClaudeSkillEffort now clamps both 'max' and 'xhigh' to 'high' — the maximum value that works in both thinking states on all supported models. Applied in both src/runtime-artifact-conversion.cts and bin/install.js. * chore(#3039): backfill changeset PR number 3119 * fix(#3039): regenerate skills with clamped effort: high --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
60cf18999b |
fix(#3026): document --pi and --gemini in installer --help (#3112)
* fix(#3026): document --pi and --gemini in installer --help --help documented 16 runtime flags but the installer accepts 18: --pi (named only in banner prose) and --gemini (invisible everywhere). Both install correctly when passed but are undiscoverable via --help. Added both to the Options section. Added a parity test that asserts every accepted runtime flag appears in --help output, with an exclusion set for legacy aliases (--both, --kimi-code). * chore(#3026): backfill changeset PR number 3112 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4926c2e904 |
fix(#3004): update Codex adapter collaboration-tool vocabulary (#3104)
* fix(#3004): update Codex adapter collaboration-tool vocabulary The generated Codex skill adapter documented stale tool vocabulary: - wait(ids) → collaboration.wait_agent(timeout_ms=...) (the real tool), with explicit disambiguation from the unrelated exec-cell functions.wait - close_agent(id) unconditional → gated on tool visibility (same schema- detection pattern already used for spawn_agent's agent_type field) - Missing required task_name field and fork_turns parameter → added alongside the existing fork_context guidance (coexist, not replace) Updated the regression test to assert the new vocabulary (wait_agent not wait(ids), functions.wait disambiguation, task_name, fork_turns, tool_search gate on close_agent). * chore(#3004): backfill changeset PR number 3104 --------- Co-authored-by: sim <sim@local> |
||
|
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
|
c9dd4aa951 |
fix(#2940): preserve codex config.toml content after the GSD marker on update (#3067)
* test(#2940): prove gsd-update discards codex config after the GSD marker Failing-first regression for #2940. mergeCodexConfig Case 2 (marker present) preserves content before the marker but discards everything from the marker to EOF, so user/Codex-CLI settings ([model], [mcp_servers.*], [profiles.*]) added after a fresh install are wiped on every update. Rows 1-2 assert trailing user content survives; rows 3-7 cover idempotency, the #2406 leaked-section non-regression, the [agents] namespace, both regions, and the zero-trailing boundary. * fix(#2940): preserve codex config.toml content after the GSD marker on update mergeCodexConfig Case 2 (marker present) preserved content before the marker but discarded everything from the marker to EOF, so user/Codex-CLI settings added after a fresh install ([model], [mcp_servers.*], [profiles.*]) were wiped on every gsd-update. Route the trailing region through the existing stripLeakedGsdCodexSections, which removes GSD's own managed/leaked sections (the bare [agents] table GSD regenerates, legacy [agents.gsd-*], [[agents]]) while preserving genuine user TOML. The marker comment line (and the optional codex_hooks ownership line) is stripped from the trailing region first so it is not duplicated alongside the regenerated block. #2406's de-dup still holds (leaked sections after the marker are still stripped), and re-merge is idempotent. * fix(#2940): use CRLF-safe regex in regression test (lint:ci no-crlf-fragile-split) The Row 7 blank-line assertion used a bare \n which trips local/no-crlf-fragile-split under Windows git-autocrlf. Use \r?\n. * fix(#2940): correct Row 5 to valid single-[agents] TOML shape The isolated adversarial review found Row 5 (bareAgentsAfterMarkerHandled) constructed TWO [agents] tables (the GSD-managed one + a user trailing one), which is invalid TOML — a duplicate table definition. extractCodexUserAgentsScalars reads only the first [agents] and stripLeakedGsdCodexSections removes ALL bare [agents] tables, so the user's max_threads would be silently dropped. The valid, realistic shape is the user folding max_threads INTO the managed [agents] block (which the existing spliceCodexAgentsScalars path preserves) plus a separate trailing [model]. Reword Row 5 to that shape. A duplicate-[agents] input is invalid TOML Codex itself rejects, so it is out of scope for this fix. * chore(#2940): add changeset fragment * chore(#2940): backfill changeset PR number 3067 --------- 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> |
||
|
Tom Boucher
|
c6ce4d1d9a |
fix(#2755): resolve the kimi hooks-TOML root per runtime (#3032)
* test(#2755): failing-first coverage for per-runtime kimi hooks root Install/uninstall filesystem-shape rows over a sandbox HOME (no permission tricks) plus resolver unit rows. Covers both uninstall directions, which is where a fix applied only to the install call site would drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2755): resolve the kimi hooks-TOML root per runtime resolveKimiHooksTomlDir took no runtime argument and hardcoded ~/.kimi, but both kimi and kimi-code route through the single hooksSurface=kimi-hooks-toml branch. A --kimi-code install therefore wrote its [[hooks]] block, hook bundle and CommonJS marker into Kimi CLI's config file, and a --kimi-code uninstall stripped Kimi CLI's block. Adds a runtime selector to the resolver -- kimi keeps ~/.kimi + KIMI_SHARE_DIR, kimi-code gets ~/.kimi-code + KIMI_CODE_HOME, per Kimi Code's own upstream data-locations and hooks docs -- and passes the runtime at both the install and uninstall call sites. An omitted or unrecognized runtime still resolves ~/.kimi, so the exported no-arg contract is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2755): use centralized helpers and add a divergence guard Review findings, all fixed in-PR: - The new test block reimplemented runMinimalInstall, createTempDir and toPosixPath. Extends runMinimalInstall with optional root/extraEnv instead (back-compat: every existing caller passes neither) and uses the centralized helpers, per CONTRIBUTING's Use Centralized Test Helpers rule. - Adds a parity assertion between the capability registry and the resolver: a third runtime declaring hooksSurface kimi-hooks-toml would silently inherit ~/.kimi, re-creating this very defect. The guard fires the moment those two surfaces drift. - Adds an installer-level test proving KIMI_SHARE_DIR and KIMI_CODE_HOME do not interfere when both are set, which only the resolver unit covered before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2755): track the kimi-code hooks root in the emitted-artifact gates The remote runner caught a real ripple: moving kimi-code hooks to ~/.kimi-code made 31 emitted paths unattributable and 58 emitted hashes unexplained, because three parallel surfaces keyed on the literal .kimi path. - HOOK_CONFIG_RELATIVE_PATHS excluded only .kimi/config.toml, so kimi-code's config.toml became manifest-visible; it embeds a platform-varying node-runner command and must stay out for both products. - HOOKS_ROOTS, the package.json-marker branch and the synthesized-install-metadata pattern each named .kimi only. - tests/fixtures/install-tree/kimi-code.json still recorded the old paths; regenerated via gen:install-tree. Adds the per-PR drift acknowledgment for the 58 paths whose bytes are unchanged but whose destination moved - a ripple no source diff can show, since no hook script was edited. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2755): clear production-tree security advisories The remote runner's npm-integrity gate reported 2 high advisories in the production dependency tree. My diff touches neither package.json nor package-lock.json, so these come from the base -- but a red gate is not something to wave off as pre-existing, so it is fixed here rather than deferred. Lockfile-only, semver-in-range, via npm audit fix: fast-uri 3.1.4 -> 3.1.5 (host confusion via backslash authority introducer) ip-address 10.2.0 -> 10.4.0 (three SSRF / trust-boundary bypasses) hono 4.12.31 -> 4.13.0 (moderate; reverting it traded a high for a moderate, so the full remedy is taken) npm audit now reports 0 vulnerabilities at every severity, npm ci installs clean from the updated lockfile, and the build and the kimi behavior both re-verified afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2755): backfill changeset pr numbers Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
𝚌𝚕𝚎𝚣𝚌𝚘𝚍𝚒𝚗𝚐
|
88f6d9bd1b |
fix(#2644): deduplicate Cursor slash menu (#2812)
* fix(#2644): deduplicate Cursor slash menu * fix: preserve installer executable mode * chore: add changeset for PR #2812 * test(#2644): acknowledge Cursor emission changes * test(#2644): drop spent emitted drift acknowledgments * fix(#2644): remove retired Cursor command converter --------- Co-authored-by: clezcoding <clezcoding@users.noreply.github.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
51f32d2d40 |
fix(#2658): detect trae runtime and resolve its instruction file to a concrete rules file (#3006)
* test(#2658): add failing-first regression for trae runtime detection and instruction path Covers all three collided defects reported in #2658 plus a fourth instance of defect 1 (ingest-docs.md) found while diagnosing it: missing trae detection in workflow runtime-detection blocks, the CLAUDE.md path-mutilation bug in both the js/cjs and md install-time converters, and the missing projectInstructionFile capability declaration. Fails against current source; the next commit fixes it. * fix(#2658): detect trae runtime and resolve its instruction file to a concrete rules file Three defects collided to produce the reported ".claude/.trae/rules/" path: 1. new-project.md and ingest-docs.md's runtime-detection blocks only recognized codex/gemini/opencode and fell through to RUNTIME=claude for trae. Both now recognize the /.trae/ execution-context path and the TRAE_CONFIG_DIR env var before the claude fallback. 2. RUNTIME_CONTENT_DISPATCH.trae.js (bin/install.js) replaced bare "CLAUDE.md" before the ".claude/" prefix was handled, mutilating ".claude/CLAUDE.md" into ".claude/.trae/rules/". Now replaces the full ".claude/CLAUDE.md" path first, and targets a concrete file. 3. convertClaudeToTraeMarkdown (mirrored in bin/install.js and src/runtime-artifact-conversion.cts per the #2094 output-parity test) had the same class of bug with a different wrong output (".trae/.trae/rules/", from its generic ".claude/" rewrite firing after the bare CLAUDE.md rewrite). Both mirrors now match full-path forms before the bare/generic patterns, converging on the same concrete file as the js/cjs converter. 4. capabilities/trae/capability.json didn't declare hostBehaviors.projectInstructionFile, so getProjectInstructionFile fell through to the generic AGENTS.md default even when RUNTIME=trae was resolved correctly. Now declares ".trae/rules/rules.md", regenerated into gsd-core/bin/lib/capability-registry.cjs via npm run gen:capability-registry. Closes #2658 * chore(#2658): add changeset * fix(#2658): preserve arbitrary runtime-dir prefixes in the trae path rewrite Found by the end-to-end --trae install regression test (not by static trace) across two verification runs: 1. copyWithPathReplacement runs a generic ~/.claude/, $HOME/.claude/, and ./.claude/ -> runtime-dir rewrite on every .md file BEFORE calling convertClaudeToTraeMarkdown. The prior fix's .claude/CLAUDE.md-specific patterns never fire on that already-rewritten text, and the bare fallback still doubled whatever prefix the generic pass substituted. A first attempt handled only the fixed "./.trae/" shape and missed the $HOME/.claude/ and ~/.claude/ forms gsd-core/workflows/profile-user.md actually uses, which post-rewrite become an arbitrary absolute local-install-root path, not the fixed relative shape. Fixed with a prefix-preserving pattern that captures whatever precedes a ".trae/" tail and fixes only the filename suffix, instead of assuming one fixed shape. 2. The fix's own explanatory comments literally spelled out the malformed strings and the instruction filename as contiguous text. Since these two files ship verbatim into local --trae installs, where they are themselves run through the same find/replace, the comments got "fixed" right along with the real code, leaking the malformed string into the installed tree. Rewrote every comment in both mirror copies to never spell either the instruction filename or a malformed shape as one contiguous token. Adds an emitted-drift-ack fragment: the corrected replacement target for every CLAUDE.md mention (bare directory -> concrete file) changes trae-emitted output for every repo file that mentions CLAUDE.md, not only the ones that hit the originally reported bug. * test(#2658): extend parity test with arbitrary-prefix .trae/ inputs The bin/install.js vs runtime-artifact-conversion.cjs parity assertion for convertClaudeToTraeMarkdown only fed the pre-existing bare .claude/CLAUDE.md input through both implementations. Feed the prefix-preserving cases (relative, nested-absolute, tilde, $HOME, backtick-wrapped) plus a property-based check through both, so a future edit to only one copy of the .trae/-tail regex fails this test instead of silently diverging. * chore(#2658): backfill changeset PR number to 3006 --------- Co-authored-by: sim <sim@local> |
||
|
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
|
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
|
00c859fae5 |
fix(#2834): write defaults.json before agent TOML generation on clean Codex install (#2900)
* fix(#2834): write defaults.json before agent TOML generation on clean Codex install Extracted writeNonClaudeDefaults(runtime) and called it BEFORE installCodexConfig so the runtime-aware model resolver has resolve_model_ids=omit + runtime=codex in ~/.gsd/defaults.json before agent TOMLs are generated. Pre-fix, a clean first Codex install generated TOMLs with no model fields (the resolver didn't know the runtime); a second run fixed it. The original inline defaults-write block (which ran AFTER agent generation) is replaced by the earlier function call (idempotent). * chore(#2834): changeset fragment * fix+test(#2834): acknowledge codex TOML drift (emitted-attribution) + fix test comment window The emitted-attribution gate flags 19 codex agent TOMLs that now carry model-routing fields (the fix's correct effect) but can't link them to a .md or src/ change (the fix is in bin/install.js ordering). Acknowledge the drift in emitted-drift-ack.json. Fix the test's comment-detection window (300 chars to capture the #2834 rationale). * fix(#2834): ack remaining 15 codex TOML drift paths * chore(#2834): backfill changeset PR number (2900) * chore(#2834): ack code-review.md growth from concurrent merge (rebase pickup) * fix(#2834): remove stale code-review.md ack (emitted-attribution failure) CI failed: 'differential attribution over the real tree' — the code-review.md ack added in 6e0b4b3b8 ('ack code-review.md growth from concurrent merge') is STALE: this PR's diff does not touch code-review.md (only bin/install.js + tests + changeset), so the ack explains growth that isn't here. The base already absorbed the concurrent code-review.md growth; the ack is inert here and the gate flags it as stale. Remove it. --------- Co-authored-by: Test <test@example.com> |
||
|
Tom Boucher
|
4f6935e29b |
fix(#2717): write CommonJS marker for cursor/windsurf/codex staged .js hooks (#2846)
* test(#2717): CommonJS marker for cursor/windsurf/codex staged .js hooks Cursor/windsurf (skipSharedHooksInstall) and codex (!isCodex gate) stage .js hook scripts via dedicated paths that bypass installSharedHooksBundle — the only writer of the {"type":"commonjs"} marker. Under a config root declaring {"type":"module"}, Node loaded those scripts as ESM and every require() failed with 'require is not defined', silently disabling the runtime's hooks. Adds regression tests (RED first, fix lands next commit): - parametrized cursor/windsurf/codex install asserts hooks/package.json exists with exactly GSD's marker content; - end-to-end: a cursor require()-using hook loads under a planted ESM-typed config root without the require-is-not-defined error; - the ensureCommonJsMarker / removeCommonJsMarkerIfGsdOwned contract: GSD markers are removed on uninstall, user-authored package.json is never touched. * fix(#2717): write CommonJS marker for cursor/windsurf/codex staged .js hooks The {"type":"commonjs"} marker lived only inside installSharedHooksBundle, which cursor/windsurf (skipSharedHooksInstall) and codex (!isCodex gate) never reach. Their .js hooks are staged by dedicated paths, so under a config root declaring {"type":"module"} Node loaded them as ESM and every require() failed with 'require is not defined', silently disabling those runtimes' hooks. Decouple the marker write into a shared helper so any code path that stages .js hooks can ensure it lands in the SAME directory as the scripts: - src/runtime-hooks-surface.cts: add ensureCommonJsMarker(dir) + removeCommonJsMarkerIfGsdOwned(dir) (byte-identical content to installSharedHooksBundle's marker; preserves a user-authored package.json on both write and uninstall). Call ensureCommonJsMarker(hooksDir) from writeCursorHooksJson + writeWindsurfHooksJson; call removeCommonJsMarkerIfGsdOwned on their matching remove paths. Export both. - bin/install.js: call hooksSurface.ensureCommonJsMarker after the codex hook copy; call hooksSurface.removeCommonJsMarkerIfGsdOwned in the generic hooks-removal loop (safe no-op where no marker exists). No change to which runtimes receive the shared bundle, the !isCodex gate, skipSharedHooksInstall, or kimi/kimi-code/cline/copilot/trae/zcode (all unchanged — audit in the diagnosis). RED @ dbb7d2bb (6 failures: 3 missing markers + the ESM require error + missing helpers); GREEN pending. * docs(#2717): changeset fragment (pr:0, backfilled post-PR) * chore(#2717): regen codex/cursor/windsurf install-tree fixtures + attribution ack The fix adds hooks/package.json to those three runtimes' install trees (the new CommonJS marker), so the golden install-tree fixtures gain one path each (regenerated via npm run gen:install-tree). emitted-attribution (ADR-2719) flags the 3 emitted hooks/package.json paths under the hooks-built rule; acknowledge them. Also drops 5 spent ack entries left by now-merged PRs (#2694 code-review.md/code-review-fix.md, #2695 worker/registry, #2794 review.md) — they are stale on this branch (base already carries them). * fix(#2717): codex ESM-root behavioral test + hooks-built provenance for package.json Two review-driven follow-ups on the #2717 fix: - Adversarial review noted the ESM-root behavioral test covered only cursor; refactor it into a helper and add a codex case (the !isCodex-gated path most likely to regress, whose marker write lives in bin/install.js). gsd-check-update.js require()s at module load, so it surfaces the ESM failure immediately. - emitted-provenance flagged hooks/package.json as 'attributed source does not exist' — the marker is code-derived (a fixed literal emitted by ensureCommonJsMarker at install time), not built from a tracked source. Route the hooks-built rule's sources/transforms for package.json to the surface source file, mirroring the existing .cmd-shim sub-family. * chore(#2717): drop now-redundant hooks/package.json attribution ack The hooks-built provenance routing (prior commit) now self-attributes the emitted hooks/package.json to src/runtime-hooks-surface.cts, which IS in this diff — so the attribution is self-explaining and the emitted-drift-ack entry became stale. Delete the (now-empty) ack file per ADR-2719's empty-file rule. * docs(changeset): backfill #2717 PR number to 2846 |
||
|
Tom Boucher
|
cfdfdf0b4c |
fix(#2695): deliver the complete four-file codex hook set for every profile (#2822)
* test(#2695): failing-first regression for codex hook worker/registry omission * fix(#2695): deliver the complete four-file codex hook set for every profile * test(#2695): regenerate codex install-tree + acknowledge emitted hook ripple * test(#2695): pin core config.toml/hooks.json wiring + fix agent-extension guard (review) * docs(changeset): backfill #2695 PR number to 2822 * fix(#2695): SessionStart wiring assertion is extension-portable (Windows .cmd shim) |
||
|
Tom Boucher
|
46bae2f9ff |
fix(#2624): write the .gsd-source marker before staging reads it (#2811)
* test(#2624): failing-first regression for stale .gsd-source marker read-before-write
Adds failing-first regression proving a Claude-global upgrade currently lets
staging read a stale prior-version .gsd-source marker before install() rewrites
it. Pre-seeds a stale marker pointing at a still-existing fake source, spies on
findInstallSourceRoot to capture which source staging resolves, and asserts
staging never resolves the stale path. Also covers fresh-install and ghost-marker
negative space.
* fix(#2624): write the .gsd-source marker before staging reads it
A Claude-global upgrade silently installed skill content from the PREVIOUS
version. findInstallSourceRoot prefers <configDir>/.gsd-source, and install()
used to rewrite that marker AFTER staging had already read it — so on an
upgrade the marker still pointed at the prior version's source (an npx cache
dir that still exists on disk) and every converted skill was generated from
the OLD commands/gsd, with generateManifest then recording the stale content's
hash as correct.
Extract the marker write into _writeGsdSourceMarker(runtime, targetDir, src,
isGlobal) and call it BEFORE the staging pass (before the _isSkillsRuntime
branch), preserving the original sourceMarkerFile && isGlobal guard, the
half-published-package existsSync guard, and the non-fatal write-failure warn.
This closes the read-before-write hole for every findInstallSourceRoot
consumer (skills, commands, /gsd-surface, capability-state) in one move.
Long-standing (marker write added in
|