test(#2665): cover the derivation and the in-process scrub
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.
This commit is contained in:
@@ -2,7 +2,12 @@ const { test, describe } = require('node:test');
|
||||
const assert = require('node:assert/strict');
|
||||
const path = require('node:path');
|
||||
|
||||
const { withIsolatedProcessState } = require('./helpers.cjs');
|
||||
const {
|
||||
withIsolatedProcessState,
|
||||
TEST_ENV_BASE,
|
||||
CONFIG_LOCATION_ENV_KEYS,
|
||||
scrubConfigLocationEnv,
|
||||
} = require('./helpers.cjs');
|
||||
|
||||
describe('withIsolatedProcessState', () => {
|
||||
test('restores env, cwd, and exitCode after callback', () => {
|
||||
@@ -39,3 +44,82 @@ describe('withIsolatedProcessState', () => {
|
||||
assert.strictEqual(process.env.PATH, originalPath);
|
||||
});
|
||||
});
|
||||
|
||||
// ─── #2665: the config-location scrub is DERIVED, and stays that way ──────────
|
||||
//
|
||||
// The recurrence guard. #2665 documents two prior authors independently
|
||||
// diagnosing this class and each fixing only the instance in front of them;
|
||||
// this is the third pass. A hand-maintained scrub list cannot be defended by
|
||||
// review alone, so the invariant is asserted instead of trusted.
|
||||
describe('#2665: TEST_ENV_BASE config-location coverage', () => {
|
||||
test('every runtime configHome env var in the registry is scrubbed', () => {
|
||||
const { runtimes } = require('../gsd-core/bin/lib/capability-registry.cjs');
|
||||
|
||||
const declared = [
|
||||
...new Set(
|
||||
Object.values(runtimes).flatMap((r) => r?.runtime?.configHome?.env ?? []),
|
||||
),
|
||||
].sort();
|
||||
|
||||
// Guards the guard: an empty/renamed registry shape would make the
|
||||
// assertion below vacuously true and silently retire this test.
|
||||
assert.ok(
|
||||
declared.length >= 15,
|
||||
`expected the registry to declare many configHome env vars, got ${declared.length} — ` +
|
||||
'if the registry shape changed, this derivation needs updating, not deleting',
|
||||
);
|
||||
|
||||
const missing = declared.filter((k) => !(k in TEST_ENV_BASE));
|
||||
assert.deepStrictEqual(
|
||||
missing,
|
||||
[],
|
||||
`config-location env vars reachable by the resolver but not scrubbed: ${missing.join(', ')}. ` +
|
||||
'TEST_ENV_BASE derives this set from the capability registry — a gap here means the ' +
|
||||
'derivation broke, not that the list needs a manual entry.',
|
||||
);
|
||||
});
|
||||
|
||||
test('every scrubbed config-location var is blanked, not merely present', () => {
|
||||
for (const key of CONFIG_LOCATION_ENV_KEYS) {
|
||||
assert.strictEqual(
|
||||
TEST_ENV_BASE[key],
|
||||
'',
|
||||
`${key} must be blanked ('') so the child sees a falsy value on the env-first branch`,
|
||||
);
|
||||
}
|
||||
});
|
||||
|
||||
test('the non-registry config-location vars are covered too', () => {
|
||||
// These have no capability descriptor, so the registry derivation alone
|
||||
// cannot reach them: GROK_AGENTS_HOME is a hardcoded branch of
|
||||
// getGlobalConfigDir, GSD_RUNTIME selects which runtime home resolves, and
|
||||
// GSD_PROJECT / GSD_WORKSTREAM move a child's .planning root
|
||||
// (src/planning-workspace.cts). Named explicitly so deleting one from the
|
||||
// helper is a test failure rather than a silent narrowing.
|
||||
for (const key of ['GROK_AGENTS_HOME', 'GSD_RUNTIME', 'GSD_PROJECT', 'GSD_WORKSTREAM']) {
|
||||
assert.strictEqual(TEST_ENV_BASE[key], '', `${key} must be scrubbed`);
|
||||
}
|
||||
});
|
||||
|
||||
test('scrubConfigLocationEnv clears and restores the parent process env', () => {
|
||||
// The in-process half of the fix (Blocker 1): TEST_ENV_BASE only reaches
|
||||
// children, so a test calling install() in-process needs the PARENT's env
|
||||
// cleared. Round-trip both states — set and unset — because restoring an
|
||||
// originally-unset var as '' rather than deleting it is itself a leak.
|
||||
withIsolatedProcessState(() => {
|
||||
process.env.CLAUDE_CONFIG_DIR = '/tmp/ambient-claude';
|
||||
delete process.env.CODEX_HOME;
|
||||
|
||||
const restore = scrubConfigLocationEnv();
|
||||
assert.strictEqual(process.env.CLAUDE_CONFIG_DIR, undefined,
|
||||
'a set config-location var must be deleted, not blanked, on the parent');
|
||||
assert.strictEqual(process.env.CODEX_HOME, undefined);
|
||||
|
||||
restore();
|
||||
assert.strictEqual(process.env.CLAUDE_CONFIG_DIR, '/tmp/ambient-claude',
|
||||
'restore must put back the original value');
|
||||
assert.ok(!('CODEX_HOME' in process.env),
|
||||
'restore must leave an originally-unset var unset, not set it to empty string');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user