fix(#2665): walk skillsHome.env on both descriptor rungs of the scrub derivation
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.
This commit is contained in:
@@ -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 `<skillsBase>/gsd-core`
|
||||
// and misses a real `<skillsBase>/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);
|
||||
|
||||
@@ -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');
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
Reference in New Issue
Block a user