golem15/msd-core
3
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

fix(#2665): derive the guard's artifact targets, and close the fallback hole in the extras

Browse Source 
A pre-push adversarial review refuted this round's own completeness claim, and it
was right on all three counts. Fixes, in the order they matter:

1. The watch enumeration was still a hand-list, and it was measurably incomplete.
   It missed kilo's SINGULAR `command/`, hermes' `skills/gsd` (a whole directory
   whose name carries no `gsd-` prefix, so no prefix rule could ever reach it),
   `plugins/gsd-core.js`, and the unprefixed subtrees the installer fills --
   `hooks/lib`, `hooks/package.json`, `scripts/lib`, `scripts/changeset`.
   The parents are now DERIVED from the capability registry's own
   artifactLayout.global destSubpath values, exactly as TEST_ENV_BASE derives its
   keys, plus a named list for the non-registry paths the installer writes
   directly. A capability declaring a new destination now extends the watch set
   in the commit that declares it. Scope note: only `global` is walked --
   `workflows` is declared LOCAL-only (windsurf) and is not a config-root parent.

2. resolveExtraWatchTargets carried the identical ambient-only defect that
   Blocker 3 closed one function over: it resolved $GSD_HOME/.gsd and each kimi
   descriptor from the ambient env alone, so a child that BLANKED those vars
   wrote to the HOME-derived fallback while the guard watched the override. Both
   legs are now unioned, matching resolveLiveConfigRoots.

3. The order-independence claim for the scan budget was too strong. It holds
   BELOW the global ceiling; once MAX_TOTAL_ENTRIES is exhausted, which targets
   get curtailed still depends on iteration order -- inherent to any shared
   aggregate bound. The residual is now named in the docblock and the test title
   says which regime it pins, instead of asserting the general claim. Negative
   limits are clamped at 0 so an injected value cannot masquerade as a scan bound.

The module's KNOWN GAP now names its remaining residuals (the loose generator
scripts, the kimi native-root hook bundle) rather than implying completeness --
an unqualified claim here just invites the same refutation next round. Both
under-watch, which fails quiet.

Reverting the derivation fails two tests; reverting the fallback leg fails a
third.
This commit is contained in:
0xdhx
2026-08-06 17:02:13 -05:00
parent 104fc76f70
commit 766480967e
2 changed files with 204 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files

133
scripts/live-config-guard.cjs
View File

@@ -42,8 +42,20 @@
* entries and MISSED a real leak into `<live>/skills/gsd-dev-preferences/`.
*
* KNOWN GAP — a leak into a file GSD does not own (e.g. mutating the host's own
* `.claude.json`) is outside this guard by construction. Closing it would require
* watching shared files, which is the false-positive trap above.
* `.claude.json`, `settings.json`, `hooks.json`, `kilo.json`, `opencode.json`) is
* outside this guard by construction. Closing it would require watching shared
* files, which is the false-positive trap above.
*
* NAMED RESIDUALS — stated rather than implied, because round 5's adversarial
* review refuted a completeness claim made here and an unqualified one would
* simply invite the same refutation again:
* - the loose generator scripts the installer copies to `<root>/scripts/*.cjs`
* (`fix-slash-commands.cjs` is watched by name; the capability generators are
* not, and they carry no `gsd-` prefix to scan for);
* - the shared-hooks bundle written into a NON-registry root's `<root>/hooks/`
* (kimi) — see resolveExtraWatchTargets, where closing it is a layout
* decision rather than one more path.
* Both under-watch, which fails quiet: a missed leak, never a false alarm.
*
* SEVERITY — reports by default, fails only under GSD_STRICT_LIVE_CONFIG_GUARD=1.
* Not timidity: on its first CI run this guard found PRE-EXISTING leaks on the
@@ -97,6 +109,79 @@ const GSD_OWNED_ENTRIES = [
const GSD_PREFIXED_PARENTS = ['agents', 'commands', 'skills', 'hooks'];
const GSD_ARTIFACT_PREFIX = 'gsd-';
/**
* Artifact parents that are NOT registry-declared — the installer writes these
* directly rather than through a capability's artifactLayout.
*/
const NON_REGISTRY_ARTIFACT_PARENTS = ['hooks', 'plugins', 'scripts'];
/**
* Paths GSD owns WHOLESALE inside a shared root, which the `gsd-` prefix rule
* cannot see because their names carry no prefix. Each is created and filled by
* the installer:
* hooks/lib, hooks/package.json -- GSD_HOOK_LIB_FILES + the CommonJS marker
* scripts/lib, scripts/changeset, -- copied wholesale into the config dir
* scripts/fix-slash-commands.cjs
* Watched as exact paths rather than by widening the parent scan, so a host
* agent's own `scripts/` or `hooks/` entries are still ignored.
*/
const GSD_OWNED_NESTED = [
'hooks/lib',
'hooks/package.json',
'scripts/lib',
'scripts/changeset',
'scripts/fix-slash-commands.cjs',
];
/**
* DERIVE the artifact parents from the capability registry rather than naming
* them, for the same reason TEST_ENV_BASE derives its keys: a hand-list is only
* ever as complete as its author's recall, and this one was measurably not.
*
* Round 5's adversarial review found the hand-list missing Kilo's SINGULAR
* `command/`, `workflows/`, and Hermes' `skills/gsd` -- the last being a whole
* directory whose name carries no `gsd-` prefix, so no prefix rule reaches it.
* A capability that declares a new destSubpath now extends this set in the same
* commit that declares it.
*
* @returns {{parents: string[], owned: string[]}} parents = watch gsd-prefixed
* children only; owned = watch the path wholesale (its own name is GSD's).
*/
function deriveArtifactTargets(runtimes) {
const parents = new Set([...GSD_PREFIXED_PARENTS, ...NON_REGISTRY_ARTIFACT_PARENTS]);
const owned = new Set(GSD_OWNED_NESTED);
for (const entry of Object.values(runtimes || {})) {
const layouts = entry?.runtime?.artifactLayout?.global ?? [];
for (const layout of layouts) {
const dest = layout?.destSubpath;
if (typeof dest !== 'string' || !dest) continue;
const last = dest.split('/').pop() || '';
// A destination whose own final segment is GSD's (e.g. hermes' `skills/gsd`)
// is owned wholesale; anything else is a shared parent we scan by prefix.
if (last.startsWith('gsd')) owned.add(dest);
else parents.add(dest);
}
}
return { parents: [...parents].sort(), owned: [...owned].sort() };
}
/** Memoized registry-derived targets; falls back to the static lists unbuilt. */
let _artifactTargets = null;
function artifactTargets(deps = {}) {
if (_artifactTargets && !deps.libDir) return _artifactTargets;
const libDir = deps.libDir || path.join(__dirname, '..', 'gsd-core', 'bin', 'lib');
let runtimes;
try {
({ runtimes } = require(path.join(libDir, 'capability-registry.cjs')));
} catch {
// Unbuilt tree: the static lists are a strict subset, never a wrong answer.
return { parents: [...GSD_PREFIXED_PARENTS, ...NON_REGISTRY_ARTIFACT_PARENTS].sort(), owned: [...GSD_OWNED_NESTED] };
}
const derived = deriveArtifactTargets(runtimes);
if (!deps.libDir) _artifactTargets = derived;
return derived;
}
/**
* The file GSD writes into a NON-REGISTRY config home.
*
@@ -126,6 +211,14 @@ const NON_REGISTRY_OWNED_FILE = 'config.toml';
* was really for. It engages only when the per-target bounds together exceed it;
* when it does, the targets it curtails are reported `unverified` -- never
* silently clean.
*
* NAMED RESIDUAL, because the obvious stronger claim is FALSE: order-independence
* holds BELOW the global ceiling, not above it. Once MAX_TOTAL_ENTRIES is
* exhausted, which targets get curtailed still depends on iteration order -- that
* is inherent to any shared aggregate bound, and the fix here removes the ordinary
* case (one large target cascading over everything after it) rather than the
* pathological one. The curtailed targets are reported `unverified`, so the
* residual costs legibility, never a false clean.
*/
const MAX_ENTRIES = 20000;
const MAX_TOTAL_ENTRIES = 200000;
@@ -247,7 +340,14 @@ function resolveExtraWatchTargets(deps = {}) {
const env = deps.env || process.env;
const homedir = (deps.os || os).homedir;
const targets = [path.resolve(path.join(env.GSD_HOME || homedir(), '.gsd'))];
// BOTH resolutions, exactly as resolveLiveConfigRoots does — the ambient one and
// the one a child that BLANKED the var falls back to. Watching only the ambient
// path leaves the fallback unwatched, which is the same defect B3 closed for the
// registry roots; it lived here too until round 5's adversarial review found it.
const targets = [
path.resolve(path.join(env.GSD_HOME || homedir(), '.gsd')),
path.resolve(path.join(homedir(), '.gsd')),
];
try {
const {
@@ -266,6 +366,12 @@ function resolveExtraWatchTargets(deps = {}) {
// process.env and os.homedir() regardless of `deps`, leaving the seam
// untestable and the targets resolved against different worlds.
for (const descriptor of NON_REGISTRY_CONFIG_HOME_DESCRIPTORS) {
// The fallback leg, per the note above: a child that blanks KIMI_SHARE_DIR /
// KIMI_CODE_HOME writes to the HOME-derived root instead of the ambient one.
const fallbackDir = resolveConfigHomeFromDescriptor(descriptor, { env: {}, home: homedir() });
if (typeof fallbackDir === 'string' && fallbackDir.length > 0) {
targets.push(path.resolve(path.join(fallbackDir, NON_REGISTRY_OWNED_FILE)));
}
const dir = resolveConfigHomeFromDescriptor(descriptor, { env, home: homedir() });
// The root belongs to the runtime, so it is never watched wholesale — only
// the named file below.
@@ -284,7 +390,9 @@ function resolveExtraWatchTargets(deps = {}) {
} catch {
// Unbuilt tree — same posture as resolveLiveConfigRoots: advisory, never fatal.
}
return targets;
// Both legs can coincide when no override is set; the snapshot keys on path,
// but dedupe anyway so the RV-facing target count means what it says.
return [...new Set(targets)];
}
/**
@@ -328,8 +436,10 @@ function newestMtime(target, budget) {
* keyed by absolute entry path.
*/
function snapshotLiveConfig(roots, extraTargets = [], limits = {}) {
const perTarget = limits.perTarget ?? MAX_ENTRIES;
const total = { remaining: limits.total ?? MAX_TOTAL_ENTRIES };
// Clamp at 0: a negative injected limit would start the ceiling below empty and
// make every target report truncated for a reason that is not a scan bound.
const perTarget = Math.max(0, limits.perTarget ?? MAX_ENTRIES);
const total = { remaining: Math.max(0, limits.total ?? MAX_TOTAL_ENTRIES) };
const snap = {};
const record = (target) => {
@@ -354,13 +464,18 @@ function snapshotLiveConfig(roots, extraTargets = [], limits = {}) {
// pull the developer's real ~/.gsd into its snapshot.
for (const target of extraTargets) record(path.resolve(target));
const { parents: watchParents, owned: watchOwned } = artifactTargets();
for (const root of roots) {
for (const entry of GSD_OWNED_ENTRIES) record(path.join(root, entry));
// Paths whose own name is GSD's, nested inside a shared root (hermes'
// `skills/gsd`, `hooks/lib`, `scripts/lib`, …) — no prefix rule sees these.
for (const entry of watchOwned) record(path.join(root, ...entry.split('/')));
// Shared dirs: enumerate only gsd-prefixed children. A child that appears
// between the two snapshots is absent from `before` entirely — diffLiveConfig
// treats after-only paths as created, which is exactly the leak signal.
for (const parent of GSD_PREFIXED_PARENTS) {
for (const parent of watchParents) {
const parentDir = path.join(root, parent);
let children;
try {
@@ -467,6 +582,10 @@ function formatViolations(violations) {
module.exports = {
GSD_OWNED_ENTRIES,
GSD_PREFIXED_PARENTS,
GSD_OWNED_NESTED,
NON_REGISTRY_ARTIFACT_PARENTS,
deriveArtifactTargets,
artifactTargets,
GSD_ARTIFACT_PREFIX,
MAX_ENTRIES,
MAX_TOTAL_ENTRIES,

81
tests/live-config-guard.test.cjs
View File

@@ -10,6 +10,7 @@ const fc = require('fast-check');
const {
GSD_OWNED_ENTRIES,
artifactTargets,
MAX_DEPTH,
resolveLiveConfigRoots,
resolveExtraWatchTargets,
@@ -147,8 +148,14 @@ describe('#2665: live-config hermeticity guard', () => {
const root = tmpRoot();
try {
const snap = snapshotLiveConfig([root]);
const watched = Object.keys(snap).map((p) => path.basename(p)).sort();
assert.deepStrictEqual(watched, [...GSD_OWNED_ENTRIES].sort());
const watched = Object.keys(snap).map((p) => path.relative(root, p)).sort();
// Top-level owned entries PLUS the nested paths GSD owns wholesale inside a
// shared root; prefixed children contribute nothing in an empty root.
const expected = [
...GSD_OWNED_ENTRIES,
...artifactTargets().owned.map((o) => path.join(...o.split('/'))),
].sort();
assert.deepStrictEqual(watched, expected);
} finally {
cleanup(root);
}
@@ -230,6 +237,74 @@ describe('#2665: live-config hermeticity guard', () => {
}
});
test('artifact parents are DERIVED from the registry, not hand-listed', () => {
// Round 5's adversarial review refuted the completeness claim of the
// hand-list: it missed Kilo's SINGULAR `command/`, `workflows/`, and hermes'
// `skills/gsd` -- a whole directory whose name carries no `gsd-` prefix, so
// no prefix rule could ever reach it.
const { parents, owned } = artifactTargets();
// NB: `workflows` is declared only in LOCAL scope (windsurf), so it is
// deliberately NOT a global config-root parent -- the derivation walks
// artifactLayout.global only.
for (const p of ['agents', 'commands', 'command', 'skills', 'hooks', 'plugins', 'scripts']) {
assert.ok(parents.includes(p), `expected derived parent ${p} in ${JSON.stringify(parents)}`);
}
assert.ok(owned.includes('skills/gsd'), `expected hermes skills/gsd in ${JSON.stringify(owned)}`);
for (const o of ['hooks/lib', 'scripts/lib']) {
assert.ok(owned.includes(o), `expected owned nested ${o} in ${JSON.stringify(owned)}`);
}
});
test('detects leaks the gsd- prefix rule structurally cannot see', () => {
const root = tmpRoot();
try {
for (const d of ['skills/gsd', 'hooks/lib', 'plugins', 'command', 'scripts/lib']) {
fs.mkdirSync(path.join(root, ...d.split('/')), { recursive: true });
}
const before = snapshotLiveConfig([root]);
// Pre-existing dirs register as MODIFIED, so bump mtimes explicitly rather
// than racing a coarse filesystem clock -- same reason the `modified` test
// above uses utimesSync.
const future = new Date(Date.now() + 10000);
const leaks = [
['skills', 'gsd', 'executor.md'],
['hooks', 'lib', 'git-cmd.js'],
['plugins', 'gsd-core.js'],
['command', 'gsd-plan.md'],
['scripts', 'lib', 'io.cjs'],
];
for (const seg of leaks) {
const f = path.join(root, ...seg);
fs.writeFileSync(f, '// leaked');
fs.utimesSync(f, future, future);
fs.utimesSync(path.dirname(f), future, future);
}
const hit = diffLiveConfig(before, snapshotLiveConfig([root])).map((v) => v.path);
for (const expected of ['skills/gsd', 'hooks/lib', 'plugins/gsd-core.js', 'command/gsd-plan.md', 'scripts/lib']) {
const abs = path.join(root, ...expected.split('/'));
assert.ok(hit.includes(abs), `${expected} leaked undetected; got ${JSON.stringify(hit)}`);
}
} finally {
cleanup(root);
}
});
test('extra watch targets cover the fallback root as well as the ambient one', () => {
// The MISSED finding from round 5's review: B3 closed this for the registry
// roots and left the identical hole in resolveExtraWatchTargets.
const targets = resolveExtraWatchTargets({
env: { GSD_HOME: '/ambient-gsd-home', KIMI_SHARE_DIR: '/ambient-kimi' },
os: { homedir: () => '/fallback-home' },
});
assert.ok(targets.includes(path.resolve('/ambient-gsd-home/.gsd')), 'ambient $GSD_HOME/.gsd');
assert.ok(targets.includes(path.resolve('/fallback-home/.gsd')), 'HOME-derived .gsd fallback');
assert.ok(
targets.some((t) => t === path.resolve('/fallback-home/.kimi/config.toml')),
`HOME-derived kimi fallback missing from ${JSON.stringify(targets)}`,
);
});
test('detects a DELETED top-level GSD entry', () => {
const root = tmpRoot();
try {
@@ -290,7 +365,7 @@ describe('#2665: live-config hermeticity guard', () => {
}
});
test('the truncation verdict does not depend on target ORDER', () => {
test('the truncation verdict does not depend on target ORDER (below the global ceiling)', () => {
const [big] = treeWithEntries(8);
const [small] = treeWithEntries(2);
try {