Review round 4, Minor 3. A configHome descriptor can nest a second,
independently-resolved descriptor (skillsHome -> resolveSkillsBaseFromDescriptor)
carrying its own env array, and the derivation walked configHome.env alone —
the identical walk-one-field gap-shape rounds 2-3 closed for the registry
and the non-registry set. Inert today (only kilo declares skillsHome, with
env: []), closed before it is live rather than after.
The guard's root enumeration deliberately does NOT gain the skills base:
getGlobalSkillsBase returns a skills directory (codex: ~/.agents/skills),
not a config root, and the snapshot applies the config-root layout beneath
every root — adding it false-positives on <skillsBase>/gsd-core while
missing a real <skillsBase>/gsd-help write (found by this round's pre-push
adversarial review). Watching skills bases needs its own layout, like
resolveExtraWatchTargets; a comment in resolveLiveConfigRoots records the
non-action.
New derivation test asserts both skillsHome rungs land in TEST_ENV_BASE,
with an anti-vacuity check that at least one runtime actually declares the
field.
Review round 4, Major 2, answering the explicit report-only-vs-strict
question: strict now. A future regression of the class this PR closes
should fail CI, not print a warning nobody reads — that is what the PR
title promises.
Scoped deliberately: GSD_STRICT_LIVE_CONFIG_GUARD=1 on the Linux/macOS
lanes of all three test jobs; Windows lanes stay report-only because the
guard's first run found pre-existing USERPROFILE leaks there (~190 test
sites sandbox HOME alone) — flipping them strict today reddens next on a
defect class this PR does not carry. Promote once that sweep lands (the
SEVERITY note in live-config-guard.cjs and the CONTEXT.md seam both now
record that state).
Review round 4, Major 1: scripts/live-config-guard.cjs is pure test
instrumentation and was shipping to every npm install. The repo already
carries the exclusion convention (gen-emitted-baseline, qa-smell-ratchet)
in the same files[] array.
Excluding the guard alone would trip the #2858 shipped-requires-only-shipped
gate: run-tests.cjs (shipped) requires it at load time, and
affected-tests-lib.cjs / run-affected-tests.cjs sit on the same chain. The
four files are one closed require chain of test instrumentation, so the
exclusion covers the chain, not one link. The guard's LOCATION header cited
affected-tests-lib.cjs as "the precedent for a non-shipped helper", which
npm pack disproves — rewritten to the tarball-exclusion fact.
Follow-up to 38c9395d, found while fact-checking the round-3 response rather
than by a test.
That commit made TEST_ENV_BASE derive its keys from
NON_REGISTRY_CONFIG_HOME_DESCRIPTORS, but had the guard call
resolveKimiHooksTomlDir directly. Both halves covered kimi, so nothing was
broken — but only one of them would pick up a SECOND descriptor. That is the
same partial-enumeration defect that put KIMI_SHARE_DIR outside the scrub set,
reintroduced one layer over, in the very commit that closed it.
resolveExtraWatchTargets now iterates the array and resolves each descriptor
through resolveConfigHomeFromDescriptor, so the scrub set and the guard derive
from one source and cannot drift apart.
Verified: a synthetic second descriptor is picked up automatically (it was not
before); kimi's target is unchanged on both the default (~/.kimi/config.toml)
and KIMI_SHARE_DIR override paths.
The new test asserts one target per descriptor plus the store root. The COUNT
is the load-bearing half — every per-descriptor assertion passes vacuously
today with a single entry, so only the count fails when the array grows and the
guard does not follow.
NAMED RESIDUAL, documented at NON_REGISTRY_OWNED_FILE: this assumes every
non-registry descriptor is written the same way (config.toml). A descriptor
whose owned file differs needs a per-descriptor mapping. It fails toward
under-watching rather than false positives, so it is called out rather than
left to be discovered.
Round 2, both Majors. They are one defect seen twice: the recurrence guard did
not cover the surface it exists to guard.
Blind surfaces. resolveLiveConfigRoots enumerates getGlobalConfigDir per registry
runtime plus a hardcoded grok branch, so it can only ever see runtime config
ROOTS. Two live write surfaces are not roots and passed through silently:
$GSD_HOME/.gsd — GSD's user-owned store. Watched WHOLESALE: unlike ~/.claude
this root is exclusively ours, so the shared-root
false-positive trap the module documents does not apply.
<kimi>/config.toml — the file GSD writes its native [[hooks]] block into. The
INVERSE case: ~/.kimi belongs to Kimi CLI, so only the one
file GSD writes is watched, never the root.
That asymmetry is why this is not a two-line "add two roots" patch — one target
needs the whole tree, the other needs exactly one file, and collapsing them
either under-watches the store or trips the guard's own documented
false-positive trap on a third party's directory.
Extras are passed to snapshotLiveConfig explicitly rather than resolved inside
it, so a caller snapshotting a fixture root cannot silently pull the developer's
real ~/.gsd into its own assertions. run-tests.cjs now snapshots when EITHER the
roots or the extras are non-empty — previously an unbuilt tree yielding zero
roots disabled the entire guard without saying so.
Budget coverage. The MAX_ENTRIES/MAX_DEPTH bound and the truncated -> 'unverified'
branch had zero tests, despite this module's own docstring naming "a truncated
scan reading as clean" as the safety-critical case. Added per
RULESET.TESTS.boundary-coverage (N in {limit-1, limit, limit+1}, exercised
through newestMtime's injected budget so the boundary is real without
materialising 20000 files) and RULESET.TESTS.property-based-testing (fast-check:
truncation is monotone in the budget; reported newest never exceeds the true
maximum). A regression flipping `truncated` to false on an exhausted budget now
breaks the property for every budget below the tree size.
Negative-controlled: neutering the extras wiring fails exactly the two
new-surface tests and nothing else. 21/21 green with it restored.
Its first CI run found PRE-EXISTING leaks on the Windows lane —
C:\Users\runneradmin\.claude\gsd-core and skills\gsd-dev-preferences — with all
1196 Windows tests otherwise passing. os.homedir() reads USERPROFILE on Windows,
and ~190 test sites across 31 files sandbox HOME alone, so the suite has been
installing GSD into the runner's real home directory invisibly. That is exactly
the class the guard exists to surface, and exactly the class this PR's review
said CI could never catch.
It is also a different defect from the one #2665 closes, and too large to fold in
here. A brand-new gate that immediately reds an unrelated lane gets bypassed or
reverted rather than obeyed, so the guard reports by default and fails only under
GSD_STRICT_LIVE_CONFIG_GUARD=1.
This is the repo's own established ratchet, not a hedge: the local/no-source-grep
ESLint rule shipped at `warn` and was promoted to `error` after its cleanup sweep
(ADR 452). Promote this the same way once the USERPROFILE sweep lands.
The recurrence guard, and #2665's own "Optional hardening". This class is silent
by construction: TEST_ENV_BASE cannot see an in-process caller, and CI cannot see
the class at all because CI never has these env vars set. It damages the
developer's machine and reports nothing -- which is how two prior authors each
diagnosed it and fixed only the instance in front of them.
run-tests.cjs snapshots GSD's install footprint in every live runtime config dir
before the suite and re-checks it after, failing the run on a create or a modify.
Roots come from the product's own getGlobalConfigDir, so the guard watches
wherever the product actually points, including through an ambient var.
Scope is ownership-based, not whole-root: the top-level install footprint plus
gsd-prefixed children of dirs GSD shares with the host agent. A config root like
~/.claude is shared, and watching it wholesale would false-positive on the host's
own history.jsonl or settings.json -- a guard that cries wolf gets disabled, and
then catches nothing. The prefix test is load-bearing: the first version watched
only the three top-level entries and MISSED a real leak into skills/gsd-*.
It earned its place immediately -- it is what found the fifth in-process leak in
runtime-artifact-layout.test.cjs, which no amount of reading the review would have
surfaced. Known gap documented in the module: a write to a file GSD does not own
is out of scope by construction.
Lives in scripts/, deliberately NOT scripts/lib/ -- the installer copies that dir
into every user's config dir wholesale while uninstall removes only an allowlist,
so a test-only module there would ship to users and survive uninstall.
Addresses review finding: Minor 8.
