From 7b8c36f904e26d9c7b90df07378f4510b0cec4a5 Mon Sep 17 00:00:00 2001 From: 0xdhx Date: Mon, 3 Aug 2026 03:06:34 -0500 Subject: [PATCH] fix(#2665): walk skillsHome.env on both descriptor rungs of the scrub derivation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 /gsd-core while missing a real /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. --- scripts/live-config-guard.cjs | 8 +++++ tests/helpers-process-isolation.test.cjs | 44 ++++++++++++++++++++++++ tests/helpers.cjs | 16 +++++++-- 3 files changed, 66 insertions(+), 2 deletions(-) diff --git a/scripts/live-config-guard.cjs b/scripts/live-config-guard.cjs index cdfe4d65f..eec8f65f5 100644 --- a/scripts/live-config-guard.cjs +++ b/scripts/live-config-guard.cjs @@ -116,6 +116,14 @@ function resolveLiveConfigRoots(deps = {}) { const roots = new Set(); // 'grok' is a hardcoded branch of getGlobalConfigDir with no registry entry. + // + // DELIBERATE NON-ROOT: getGlobalSkillsBase(runtime) is NOT added here. The + // skills base (e.g. codex's ~/.agents/skills) is not a config ROOT, and the + // snapshot applies the config-root layout (GSD_OWNED_ENTRIES x + // GSD_PREFIXED_PARENTS) beneath every root it is given — measured on a + // sandboxed HOME, adding it both false-positives on `/gsd-core` + // and misses a real `/gsd-help` write. Watching skills bases + // needs its own layout, like resolveExtraWatchTargets — a separate change. for (const runtime of [...Object.keys(runtimes || {}), 'grok']) { try { const dir = getGlobalConfigDir(runtime); diff --git a/tests/helpers-process-isolation.test.cjs b/tests/helpers-process-isolation.test.cjs index cd93b61d3..d18e4f2b9 100644 --- a/tests/helpers-process-isolation.test.cjs +++ b/tests/helpers-process-isolation.test.cjs @@ -154,6 +154,50 @@ describe('#2665: TEST_ENV_BASE config-location coverage', () => { ); }); + test('skillsHome env vars are walked on BOTH descriptor rungs', () => { + // Round 4. A configHome descriptor can nest a second, independently-resolved + // descriptor (skillsHome → resolveSkillsBaseFromDescriptor), which carries + // its own env array. Walking configHome.env alone is 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: [] — so this asserts the DERIVATION reaches the field, not that any + // var currently flows from it: every skillsHome-declared var (registry and + // non-registry alike) must land in TEST_ENV_BASE the moment one exists. + const { runtimes } = require('../gsd-core/bin/lib/capability-registry.cjs'); + const { + NON_REGISTRY_CONFIG_HOME_DESCRIPTORS, + } = require('../gsd-core/bin/lib/runtime-homes.cjs'); + + const declared = [ + ...new Set([ + ...Object.values(runtimes).flatMap( + (r) => r?.runtime?.configHome?.skillsHome?.env ?? [], + ), + ...NON_REGISTRY_CONFIG_HOME_DESCRIPTORS.flatMap( + (d) => d?.skillsHome?.env ?? [], + ), + ]), + ]; + + // Anti-vacuity: at least one runtime must actually DECLARE skillsHome, or a + // registry reshape could rename the field and retire this test silently. + const declaringRuntimes = Object.values(runtimes).filter( + (r) => r?.runtime?.configHome?.skillsHome !== undefined, + ); + assert.ok( + declaringRuntimes.length >= 1, + 'expected at least one registry runtime to declare configHome.skillsHome — ' + + 'if the field moved, this derivation needs updating, not deleting', + ); + + const missing = declared.filter((k) => !(k in TEST_ENV_BASE)); + assert.deepStrictEqual( + missing, + [], + `skillsHome-declared config-location vars not scrubbed: ${missing.join(', ')}`, + ); + }); + test("GSD's OWN location vars are scrubbed (a second family, not a registry gap)", () => { const { GSD_LOCATION_ENV_KEYS } = require('../gsd-core/bin/lib/runtime-homes.cjs'); diff --git a/tests/helpers.cjs b/tests/helpers.cjs index 8b4a27f42..64161dee9 100644 --- a/tests/helpers.cjs +++ b/tests/helpers.cjs @@ -67,11 +67,23 @@ const NON_REGISTRY_CONFIG_LOCATION_ENV_KEYS = [ const CONFIG_LOCATION_ENV_KEYS = [ ...new Set([ - // 1. Every runtime descriptor the capability registry carries. + // 1. Every runtime descriptor the capability registry carries — including + // the nested skillsHome descriptor, which resolves independently of + // configHome (resolveSkillsBaseFromDescriptor) and can carry its own + // env array. Inert today (only kilo declares skillsHome, with env: []), + // but walking configHome.env alone is the identical gap-shape this PR + // closed twice already, one field over. (#2665 round 4) ...Object.values(runtimes).flatMap((r) => r?.runtime?.configHome?.env ?? []), + ...Object.values(runtimes).flatMap( + (r) => r?.runtime?.configHome?.skillsHome?.env ?? [], + ), // 2. Descriptor-shaped config homes resolved OUTSIDE the registry (kimi's // native config.toml home via KIMI_SHARE_DIR). Derived, not hand-listed. - ...NON_REGISTRY_CONFIG_HOME_DESCRIPTORS.flatMap((d) => d?.env ?? []), + // Same skillsHome walk as rung 1 — a descriptor is a descriptor. + ...NON_REGISTRY_CONFIG_HOME_DESCRIPTORS.flatMap((d) => [ + ...(d?.env ?? []), + ...(d?.skillsHome?.env ?? []), + ]), // 3. GSD's OWN location vars — a different family: they decide where GSD keeps // user-owned state ($GSD_HOME/.gsd/), not where a runtime keeps its config. ...GSD_LOCATION_ENV_KEYS,