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

fix(#2665): stop watching shared ground, and derive the artifact prefix too

Browse Source 
The previous commit widened the guard's watch set and claimed the enumeration
was complete. Re-running the pre-push adversarial gate on that commit -- which I
should have done before pushing it, and did not -- refuted the claim on four
counts. All four were real.

1. FALSE POSITIVES, which is the worse polarity. `hooks/lib`,
   `hooks/package.json`, `scripts/lib` and `scripts/changeset` were watched
   WHOLESALE. The installer preserves foreign files in every one of them -- it
   removes the CommonJS marker only on an exact content match, because "a
   user-authored package.json is never deleted" -- so a user editing their own
   helper mid-suite tripped the guard. A driven probe produced four violations
   from touching only user-owned files. Watching shared ground is exactly what
   the module's SCOPE note refuses: a guard that cries wolf gets switched off,
   and then catches nothing at all. Now only exact GSD filenames inside those
   dirs are watched, and a test asserts foreign edits stay silent.

2. THE PREFIX WAS HARDCODED, which is this PR's own defect one level down. Each
   artifactLayout declares its OWN prefix, and kimi's `kimi-agents` layout
   declares `gsd` with no hyphen, writing `agents/gsd.yaml` and `agents/gsd.md`.
   A fixed `gsd-` scan is structurally blind to both, as it is to pi's
   `extensions/gsd.js`. The prefix is now derived per parent, as a SET -- the
   same destSubpath carries different prefixes across runtimes (`agents` appears
   with both `gsd` and `gsd-`). `extensions` joins the non-registry parents; pi
   declares no artifactLayout at all, so no registry walk could find it.

3. THE ENTRY BOUND FAILED OPEN on a non-finite limit: `Math.max(0, NaN)` is NaN,
   and every budget comparison against NaN is false, so the walk was unbounded --
   the single thing the constant exists to prevent. Clamped with Number.isFinite.
   The walk also kept invoking itself for every remaining sibling after the
   budget was gone; it now returns.

4. THE RESIDUAL LIST WAS WRONG AGAIN. `agents/subagents/**` (kimi stages under an
   unprefixed intermediate dir), the loose capability generators, and the
   `extensions`/`plugins` CommonJS markers are all unwatched and were unnamed.
   They are named now, and the four shared dirs are recorded as DELIBERATELY not
   watched -- a different thing from missed.

Each fix is negative-controlled and each control fires. The NaN control did not
fire on its first form: the test asserted `truncated: false`, which the broken
code also produces on a small tree, so it discriminated nothing. Repaired with a
NaN perTarget against a small finite ceiling, where the two behaviours differ.
This commit is contained in:
0xdhx
2026-08-06 17:51:52 -05:00
parent 766480967e
commit fae0c6ae1a
2 changed files with 154 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

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

@@ -46,16 +46,23 @@
* 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.
* NAMED RESIDUALS — stated rather than implied, because two successive rounds
* asserted this list was complete and both were refuted. Still NOT watched:
* - `agents/subagents/**` (kimi stages `subagents/gsd-executor.yaml` under an
* UNPREFIXED intermediate dir, so no prefix scan of `agents/` reaches it);
* - the loose capability generators copied to `<root>/scripts/*.cjs`
* (`fix-slash-commands.cjs` is watched by name; the generators are not);
* - `extensions/package.json` and `plugins/package.json` — CommonJS markers in
* dirs GSD fills but does not own, so they fall under the shared-ground rule
* below rather than being watched;
* - the shared-hooks bundle in a NON-registry root's `<root>/hooks/` (kimi) —
* see resolveExtraWatchTargets; closing it is a layout decision.
* DELIBERATELY not watched, which is a different thing from missed: `hooks/lib`,
* `hooks/package.json`, `scripts/lib` and `scripts/changeset`. The installer
* preserves foreign files in each, so watching them wholesale produces false
* positives — and a guard that cries wolf gets switched off.
* Every item above under-watches, 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
@@ -113,24 +120,31 @@ 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'];
const NON_REGISTRY_ARTIFACT_PARENTS = ['hooks', 'plugins', 'scripts', 'extensions'];
/**
* 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.
* Prefixes used for a parent with no registry-declared one. BOTH forms are the
* point: GSD writes `gsd-`-hyphen artifacts (`hooks/gsd-check-update.js`) AND
* bare `gsd.`-dotted ones (pi's `extensions/gsd.js`), and a lone `gsd-` sees
* only the first.
*/
const DEFAULT_ARTIFACT_PREFIXES = ['gsd-', 'gsd.'];
/**
* Files GSD owns by EXACT NAME inside a directory it shares — deliberately NOT
* the directories themselves.
*
* `hooks/lib`, `hooks/package.json`, `scripts/lib` and `scripts/changeset` were
* watched wholesale for exactly one commit, and that was wrong: the installer's
* own uninstall path preserves foreign files in every one of them (it removes the
* CommonJS marker only on an exact content match — "a user-authored package.json
* is never deleted"). Watching them wholesale turns a user editing their own
* helper into a violation, which is the false-positive trap the SCOPE note above
* exists to refuse. Under-watching fails quiet; over-watching disarms the guard.
*/
const GSD_OWNED_NESTED = [
'hooks/lib',
'hooks/package.json',
'scripts/lib',
'scripts/changeset',
'scripts/fix-slash-commands.cjs',
'hooks/managed-hooks-registry.cjs',
];
/**
@@ -148,21 +162,36 @@ const GSD_OWNED_NESTED = [
* 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 parents = new Map();
const addParent = (dest, prefixes) => {
if (!parents.has(dest)) parents.set(dest, new Set());
for (const pre of prefixes) parents.get(dest).add(pre);
};
for (const dest of [...GSD_PREFIXED_PARENTS, ...NON_REGISTRY_ARTIFACT_PARENTS]) {
addParent(dest, DEFAULT_ARTIFACT_PREFIXES);
}
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) {
for (const layout of entry?.runtime?.artifactLayout?.global ?? []) {
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);
// A destination whose own final segment is GSD's (hermes' `skills/gsd`) is
// owned wholesale — that directory is ours, not shared.
if (last.startsWith('gsd')) { owned.add(dest); continue; }
// The layout declares its OWN prefix, and it varies: kimi's `kimi-agents`
// layout declares `gsd` (no hyphen) and writes `agents/gsd.yaml` +
// `agents/gsd.md`, invisible to a fixed `gsd-` scan. The same destSubpath
// also carries different prefixes across runtimes, so a parent maps to a SET.
const declared = typeof layout?.prefix === 'string' && layout.prefix
? [layout.prefix]
: DEFAULT_ARTIFACT_PREFIXES;
addParent(dest, declared);
}
}
return { parents: [...parents].sort(), owned: [...owned].sort() };
const out = {};
for (const [dest, set] of [...parents.entries()].sort()) out[dest] = [...set].sort();
return { parents: out, owned: [...owned].sort() };
}
/** Memoized registry-derived targets; falls back to the static lists unbuilt. */
@@ -175,7 +204,7 @@ function artifactTargets(deps = {}) {
({ 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] };
return deriveArtifactTargets(null);
}
const derived = deriveArtifactTargets(runtimes);
if (!deps.libDir) _artifactTargets = derived;
@@ -422,7 +451,13 @@ function newestMtime(target, budget) {
} catch {
return;
}
for (const entry of entries) walk(path.join(current, entry), depth + 1);
for (const entry of entries) {
// RETURN, not continue: without this the loop keeps invoking walk() for
// every remaining sibling after the budget is gone, so the ceiling bounds
// what is RECORDED but not the work done getting there.
if (budget.remaining <= 0) { truncated = true; return; }
walk(path.join(current, entry), depth + 1);
}
};
walk(target, 0);
@@ -438,8 +473,12 @@ function newestMtime(target, budget) {
function snapshotLiveConfig(roots, extraTargets = [], limits = {}) {
// 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) };
// Number.isFinite, NOT Math.max: `Math.max(0, NaN)` is NaN, and every budget
// comparison against NaN is false — the bound then fails OPEN and the walk is
// unbounded, which is the single thing these constants exist to prevent.
const finite = (v, fallback) => (Number.isFinite(v) && v >= 0 ? v : fallback);
const perTarget = finite(limits.perTarget, MAX_ENTRIES);
const total = { remaining: finite(limits.total, MAX_TOTAL_ENTRIES) };
const snap = {};
const record = (target) => {
@@ -475,8 +514,8 @@ function snapshotLiveConfig(roots, extraTargets = [], limits = {}) {
// 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 watchParents) {
const parentDir = path.join(root, parent);
for (const [parent, prefixes] of Object.entries(watchParents)) {
const parentDir = path.join(root, ...parent.split('/'));
let children;
try {
children = fs.readdirSync(parentDir);
@@ -484,7 +523,7 @@ function snapshotLiveConfig(roots, extraTargets = [], limits = {}) {
continue; // parent absent — nothing of ours can be in it yet
}
for (const child of children) {
if (child.startsWith(GSD_ARTIFACT_PREFIX)) record(path.join(parentDir, child));
if (prefixes.some((pre) => child.startsWith(pre))) record(path.join(parentDir, child));
}
}
}
@@ -584,6 +623,7 @@ module.exports = {
GSD_PREFIXED_PARENTS,
GSD_OWNED_NESTED,
NON_REGISTRY_ARTIFACT_PARENTS,
DEFAULT_ARTIFACT_PREFIXES,
deriveArtifactTargets,
artifactTargets,
GSD_ARTIFACT_PREFIX,

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

@@ -246,32 +246,41 @@ describe('#2665: live-config hermeticity guard', () => {
// 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)}`);
for (const p of ['agents', 'commands', 'command', 'skills', 'hooks', 'plugins', 'scripts', 'extensions']) {
assert.ok(p in parents, `expected derived parent ${p} in ${JSON.stringify(Object.keys(parents))}`);
}
// kimi's kimi-agents layout declares prefix `gsd` (no hyphen); a fixed `gsd-`
// scan cannot see agents/gsd.yaml, which is the defect this derivation closes.
assert.ok(
parents.agents.includes('gsd'),
`agents must carry kimi's bare 'gsd' prefix; got ${JSON.stringify(parents.agents)}`,
);
assert.ok(owned.includes('skills/gsd'), `expected hermes skills/gsd in ${JSON.stringify(owned)}`);
for (const o of ['hooks/lib', 'scripts/lib']) {
// Exact GSD filenames only — never the shared directories that contain them.
for (const o of ['scripts/fix-slash-commands.cjs', 'hooks/managed-hooks-registry.cjs']) {
assert.ok(owned.includes(o), `expected owned nested ${o} in ${JSON.stringify(owned)}`);
}
for (const shared of ['hooks/lib', 'hooks/package.json', 'scripts/lib', 'scripts/changeset']) {
assert.ok(!owned.includes(shared), `${shared} is shared ground and must NOT be watched wholesale`);
}
});
test('detects leaks the gsd- prefix rule structurally cannot see', () => {
test('detects leaks a single hardcoded gsd- prefix cannot see', () => {
const root = tmpRoot();
try {
for (const d of ['skills/gsd', 'hooks/lib', 'plugins', 'command', 'scripts/lib']) {
for (const d of ['skills/gsd', 'agents', 'plugins', 'command', 'extensions']) {
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);
// kimi declares prefix `gsd` (no hyphen) and writes agents/gsd.yaml;
// pi writes extensions/gsd.js. A fixed `gsd-` scan sees neither.
const leaks = [
['skills', 'gsd', 'executor.md'],
['hooks', 'lib', 'git-cmd.js'],
['agents', 'gsd.yaml'],
['extensions', 'gsd.js'],
['plugins', 'gsd-core.js'],
['command', 'gsd-plan.md'],
['scripts', 'lib', 'io.cjs'],
];
for (const seg of leaks) {
const f = path.join(root, ...seg);
@@ -281,7 +290,7 @@ describe('#2665: live-config hermeticity guard', () => {
}
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']) {
for (const expected of ['skills/gsd', 'agents/gsd.yaml', 'extensions/gsd.js', 'plugins/gsd-core.js', 'command/gsd-plan.md']) {
const abs = path.join(root, ...expected.split('/'));
assert.ok(hit.includes(abs), `${expected} leaked undetected; got ${JSON.stringify(hit)}`);
}
@@ -290,6 +299,42 @@ describe('#2665: live-config hermeticity guard', () => {
}
});
test('a user editing their OWN files in a shared dir is not a violation', () => {
// The false-positive case a prior commit shipped: hooks/lib, hooks/package.json,
// scripts/lib and scripts/changeset were watched WHOLESALE, so touching a
// user-authored helper in any of them tripped the guard. The installer itself
// preserves foreign files in all four, so they are not GSD's to watch.
const root = tmpRoot();
try {
for (const d of ['hooks/lib', 'scripts/lib', 'scripts/changeset']) {
fs.mkdirSync(path.join(root, ...d.split('/')), { recursive: true });
}
const foreign = [
['hooks', 'package.json'],
['hooks', 'lib', 'user-helper.js'],
['scripts', 'lib', 'user-helper.cjs'],
['scripts', 'changeset', 'user-tool.cjs'],
];
for (const seg of foreign) fs.writeFileSync(path.join(root, ...seg), 'mine');
const before = snapshotLiveConfig([root]);
const future = new Date(Date.now() + 10000);
for (const seg of foreign) {
const f = path.join(root, ...seg);
fs.writeFileSync(f, 'mine, edited');
fs.utimesSync(f, future, future);
fs.utimesSync(path.dirname(f), future, future);
}
assert.deepStrictEqual(
diffLiveConfig(before, snapshotLiveConfig([root])),
[],
'editing user-owned files in a shared dir must not trip the guard',
);
} 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.
@@ -380,6 +425,26 @@ describe('#2665: live-config hermeticity guard', () => {
}
});
test('a non-finite injected limit falls back to the real bound, never fails open', () => {
// Math.max(0, NaN) is NaN, and every budget comparison against NaN is false,
// so the walk becomes unbounded — the one thing the bound exists to prevent.
// The discriminator has to be a case where the two behaviours DIFFER: pair a
// NaN perTarget with a small finite ceiling. Fixed, perTarget falls back to
// MAX_ENTRIES and the ceiling still bites (truncated). Broken, min(NaN, 3) is
// NaN and nothing truncates at all.
const [dir] = treeWithEntries(6);
try {
const snap = snapshotLiveConfig([], [dir], { perTarget: NaN, total: 3 });
assert.strictEqual(
snap[path.resolve(dir)].truncated,
true,
'a NaN perTarget must fall back to a real bound, not disable budgeting',
);
} finally {
cleanup(dir);
}
});
test('the GLOBAL ceiling still bounds the aggregate, and reports unverified', () => {
// The bound the single budget was really for is kept -- but when it engages,
// the curtailed target is reported rather than silently attested clean.