fix(#1858): address code+security review findings

- restructure _loadFlatCommandsGsdManifest try/catch to wrap read+parse+set
  together (mirrors loadSkillsManifest exactly), so a thrown parser degrades
  both keys to [] — closes the latent catch-scope parity drift [Nit-1]
- add boundary test: gsd-.md (empty stem) skipped, gsd-x.md single-char stem
  kept (slice(4,-3) boundary) [Low-1]
- add unreadable-file test (POSIX-gated): both keys degrade to [] [Low-2]
- strengthen parity test to also compare requires + _calls_agents_ VALUES,
  not just the stem set [Nit-2]

Both orthogonal reviews returned APPROVE with no Critical/High findings.
Security review confirmed no new trust-boundary crossing, prototype-pollution
immune (Map/Set throughout), and symlink/path-traversal surface identical to
the pre-existing nested loader (not a regression).
This commit is contained in:
Tom Boucher
2026-07-06 21:35:24 -04:00
parent e3d053619c
commit 2c853822a1
2 changed files with 59 additions and 6 deletions

View File

@@ -416,15 +416,19 @@ function _loadFlatCommandsGsdManifest(commandsParentDir: string): Map<string, st
// Strip 'gsd-' prefix (4 chars) and '.md' suffix (3 chars) → stem.
const stem = entry.name.slice(4, -3);
if (!stem) continue;
let content = '';
// Mirror loadSkillsManifest's try/catch structure exactly: wrap read +
// parse + set together so an unreadable file OR a thrown parser degrades
// both keys to [] (parity; closes the latent catch-scope drift a reviewer
// flagged — both parsers are non-throwing today, but the structural
// match future-proofs the "identical Map shape" contract).
try {
content = fs.readFileSync(path.join(commandsParentDir, entry.name), 'utf8');
const content = fs.readFileSync(path.join(commandsParentDir, entry.name), 'utf8');
manifest.set(stem, parseRequires(content));
manifest.set(`_calls_agents_${stem}`, parseCallsAgents(content));
} catch {
// Unreadable file — register with empty deps + agents (parity with
// loadSkillsManifest's readFileSync catch branch).
manifest.set(stem, []);
manifest.set(`_calls_agents_${stem}`, []);
}
manifest.set(stem, content ? parseRequires(content) : []);
manifest.set(`_calls_agents_${stem}`, content ? parseCallsAgents(content) : []);
}
return manifest;
}

View File

@@ -903,6 +903,47 @@ describe('regressions: flat commands/gsd-<stem>.md layout (#1858)', () => {
}
});
// Low-1 (review): boundary — empty stem (gsd-.md) skipped, single-char stem kept.
test('_loadFlatCommandsGsdManifest: skips gsd-.md (empty stem) and keeps single-char stem (slice boundary)', () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-flat-edge-'));
try {
fs.writeFileSync(path.join(tmpDir, 'gsd-.md'), makeFlatCommandMd(''), 'utf8');
fs.writeFileSync(path.join(tmpDir, 'gsd-x.md'), makeFlatCommandMd('x'), 'utf8');
const manifest = _loadFlatCommandsGsdManifest(tmpDir);
assert.ok(!manifest.has(''), 'gsd-.md must NOT register an empty-string stem');
assert.ok(!manifest.has('_calls_agents_'), 'no companion key for an empty stem');
assert.ok(manifest.has('x'), 'gsd-x.md -> single-char stem "x" (slice(4,-3) boundary)');
} finally {
cleanup(tmpDir);
}
});
// Low-2 (review): unreadable file degrades both keys to [] (parity with nested
// loader's catch). POSIX-only AND must not run as root — root bypasses POSIX
// read permission bits, so chmod 0o000 would NOT make the file unreadable and
// the test would assert [] against the real parsed deps (false failure).
// Skip on win32 (DEFECT.WINDOWS-POSIX-MODE-BIT-ASSERT) and when getuid()==0.
const _skipUnreadable = process.platform === 'win32' || (typeof process.getuid === 'function' && process.getuid() === 0);
test('_loadFlatCommandsGsdManifest: unreadable file degrades to empty deps + agents (parity)', { skip: _skipUnreadable }, () => {
const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-flat-unread-'));
let madeUnreadable = false;
try {
const skillPath = path.join(tmpDir, 'gsd-secure.md');
fs.writeFileSync(skillPath, '---\nname: gsd:secure\nrequires: [phase]\n---\nbody\n', { mode: 0o644 });
fs.chmodSync(skillPath, 0o000);
madeUnreadable = true;
const manifest = _loadFlatCommandsGsdManifest(tmpDir);
assert.deepStrictEqual(manifest.get('secure'), [], 'unreadable file -> empty requires (parity with nested catch)');
assert.deepStrictEqual(manifest.get('_calls_agents_secure'), [], 'unreadable file -> empty agents (parity with nested catch)');
} finally {
// Restore writability so cleanup() can rm the tmp tree.
if (madeUnreadable) {
try { fs.chmodSync(path.join(tmpDir, 'gsd-secure.md'), 0o644); } catch { /* best effort */ }
}
cleanup(tmpDir);
}
});
// ── _resolveManifest picks the flat branch when nested is absent ────────────
test('_resolveManifest: detects flat commands/gsd-<stem>.md layout when nested commands/gsd/ is absent (#1858)', () => {
@@ -986,6 +1027,14 @@ describe('regressions: flat commands/gsd-<stem>.md layout (#1858)', () => {
const flatStems = [...flat.keys()].filter((k) => !k.startsWith('_calls_agents_')).sort();
assert.deepStrictEqual(flatStems, nestedStems,
'flat loader stem set must match nested loader stem set for the real command tree');
// Nit-2 (review): also compare _calls_agents_<stem> VALUES, not just the
// stem set — proves the shared parseCallsAgents output is identical.
for (const stem of flatStems) {
assert.deepStrictEqual(flat.get(`_calls_agents_${stem}`), nested.get(`_calls_agents_${stem}`),
`agent refs for stem "${stem}" must match between flat and nested loaders`);
assert.deepStrictEqual(flat.get(stem), nested.get(stem),
`requires for stem "${stem}" must match between flat and nested loaders`);
}
} finally {
cleanup(tmpFlat);
}