Post-rebase validity finding, not a new defect. The base range added
local/no-unbounded-spawn (#3143, 2afe17bb) and then DELETED its allowlist
(#3148, 9faacc0c), so the cold-require probe this PR added in 4bc6b0a2 --
legal when written -- is now an error. Bounded at 30s: a cold require is
sub-second, so that is ~30x headroom and still fails loudly rather than
hanging a lane.
Also routes this round's own teardown through cleanup() instead of raw
fs.rmSync, per local/no-raw-rmsync-in-tests, which carries the Windows-EBUSY
retry budget.
eslint clean on the file.
Two halves, and the second is the one that matters.
The contract half asserts installSpawnEnv() and installerEnv() both replace the
ambient HOME, keep USERPROFILE tracking it (os.homedir() reads that one on
Windows), and still let an explicit override win.
The behavioural half drives the REAL installer against a REAL ambient HOME: it
points process.env.HOME at a canary, runs `install.js --cursor --local`, and
asserts the canary gains no .gsd. No assertion about the scrub set can stand in
for this, because writeNonClaudeDefaults() resolves through os.homedir(), which
reads no GSD variable -- a set-membership test would pass with the bug fully
live.
Negative-controlled against the pre-fix tree rather than assumed: reverting
installerEnv() to `{ ...process.env, ...overrides }` fails BOTH halves, the
behavioural one reporting the actual artifact
("the installer wrote GSD's user store into the ambient HOME: defaults.json").
tests/helpers.cjs required gsd-core/bin/lib at module scope to derive the
config-location scrub set. That lib is BUILT, so on an unbuilt tree the require
threw inside `require('./helpers.cjs')` -- before a single test() had registered
-- turning one missing `npm run build:lib` into a whole-suite crash with no
message naming the remedy. This is the file ~370 test files import, so the blast
radius is the suite. `npm test` builds via its pretest hook; the shape that
reaches this is a direct `node --test` invocation, which is exactly what a
contributor reaches for when running one file.
The require is now memoized behind builtLib(), and the two derived exports
(TEST_ENV_BASE, CONFIG_LOCATION_ENV_KEYS) are enumerable lazy getters, so
destructuring and Object.keys() behave as before. Reading either is what forces
the build; a test file that needs neither now imports cleanly. When the build IS
missing, the error names `npm run build:lib` instead of surfacing a bare
MODULE_NOT_FOUND.
Verified by a cold-child probe rather than by inspection -- this process has
already loaded everything, so an in-process assertion would pass vacuously. The
probe checks require.cache before and after touching TEST_ENV_BASE, and fails
when the require is moved back to module scope.
Addresses review finding: Major 7.
Found by the pre-publication claim audit of this round's response comment,
which refuted the sentence "none of the unscrubbed env reads names a write
destination" on the grounds that naming a path is not the same property as
influencing where writes land.
GSD_ALLOW_SYMLINKED_DEST is boolean and names no path, so every rung of the
derivation is structurally incapable of reaching it: not a registry configHome,
not descriptor-shaped, not one of GSD's own location vars. It is still a #2665
leak vector. install-engine.cts reads it env-first (:214) and threads it as
allowOptInFollow into the symlink-escape guard at four call sites, each gating
a write (:361/:367, :416/:424, :785/:790, :927/:932). That guard is what stops
a write leaving the install root, so an ambient =1 disarms it for the whole
suite — the #2665 hazard arriving through a permission rather than a path.
Added as its own named family (WRITE_ESCAPE_PERMISSION_ENV_KEYS) rather than
folded into a location rung, for the same reason GSD_HOME got its own family in
round 3: the list should not misdescribe what its members are.
Blanking is fail-safe in the only direction that matters — '' is neither '1'
nor 'true', so a blanked value makes the guard stricter, never looser. That
asymmetry is what licenses scrubbing it wholesale rather than reasoning about
each call site.
The guard test names the variable literally rather than iterating the family
constant: a test that asserts over the constant shrinks its own expectation
when the family is emptied, which is the enumeration-relative failure that let
the kimi-code descriptor go unwatched earlier in this same round.
#2393's opt-in suite is unaffected (99/100, 0 fail): it sets process.env
directly in-process and never routes through scrubConfigLocationEnv, and an
explicit env argument still wins over TEST_ENV_BASE in childEnv.
None of the round-4 fixes had a test that fails on reversion: re-shipping
the test-instrumentation chain passes #2858 (everything ships, so every
require resolves), dropping the strict env from test.yml demotes the guard
to report-only with nothing red, and the skillsHome derivation tests are
enumeration-relative over declarations that are all empty today. One
guard each, every one negative-controlled against its reverted fix (fails
pre-fix, passes post-fix):
- packaging-shipped-scripts-require-only-shipped.test.cjs asserts the four
chain files are absent from the npm pack file list (reuses the tarball
set the #2858 gate already resolves — no second npm pack).
- live-config-guard.test.cjs asserts all three test jobs wire
GSD_STRICT_LIVE_CONFIG_GUARD, matching the WHOLE expression anchored —
a prefix match accepted both a Windows-silently-strict tail and a
malformed one.
- helpers-process-isolation.test.cjs cold-requires helpers.cjs in a child
with sentinel skillsHome env vars injected into both enumerations, so
the walk itself is under test rather than today's empty declarations.
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.
Two tests for round 3's change (round 2 Blockers 1 and 2): KIMI_SHARE_DIR must
arrive via NON_REGISTRY_CONFIG_HOME_DESCRIPTORS rather than a literal, and every
GSD_LOCATION_ENV_KEYS entry must be blanked. Both assert a floor on their source
first, so a renamed export fails loudly instead of passing vacuously.
Negative-controlled against the registry-only derivation: both fail there, and
only those two.
And the Nit, which is the more useful half. This block asserts that TEST_ENV_BASE
is not narrower than the enumerations it derives from. It cannot prove those
enumerations are complete — a var no enumeration carries is invisible to every
test here, and they stay green.
That is exactly how round 2 found GSD_HOME and KIMI_SHARE_DIR while this block
was fully green: one belonged to no enumeration at all, the other sat inside a
function body where nothing could enumerate it. So the scope boundary is now
written down at the top of the block, naming where the completeness question is
actually answered — a source census re-derived each round, and
live-config-guard.cjs observing real writes at runtime — so that a green run
here is not misread as "the set is exhaustive."
Deliberately NOT added: an assertion per reviewer-named variable. That is the
hand-maintained list wearing a test's clothes, and it fails the same way.
The single regression test exercised CLAUDE_CONFIG_DIR only, so deleting
GSD_RUNTIME or CODEX_HOME from any literal broke nothing -- a mutation of 24 of
the 27 added key-value pairs survived.
Four tests here: every configHome env var the registry declares is scrubbed;
every scrubbed key is blanked rather than merely present; the four non-registry
vars are named explicitly so deleting one is a failure rather than a silent
narrowing; and scrubConfigLocationEnv round-trips both a set and an unset var
(restoring an originally-unset var as '' would itself be a leak).
Both derivation tests assert a floor on the registry first, so a renamed registry
shape fails loudly instead of making the assertions vacuously true.
Negative-controlled against the hand-written 3-key list this PR shipped: the
parity test fails there and names all 19 missing vars.
Addresses review findings: Major 6, Minor 8.
