From 2c853822a172307386dd8b7a4ce4a1db8fb41610 Mon Sep 17 00:00:00 2001
From: Tom Boucher <trekkie@nomorestars.com>
Date: Mon, 6 Jul 2026 21:35:24 -0400
Subject: [PATCH] fix(#1858): address code+security review findings
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

- 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).
---
 src/capability-state.cts        | 16 +++++++----
 tests/capability-state.test.cjs | 49 +++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 6 deletions(-)

diff --git a/src/capability-state.cts b/src/capability-state.cts
index cead2c8b5..9527e930a 100644
--- a/src/capability-state.cts
+++ b/src/capability-state.cts
@@ -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;
 }
diff --git a/tests/capability-state.test.cjs b/tests/capability-state.test.cjs
index fa2553c7a..456e8ee84 100644
--- a/tests/capability-state.test.cjs
+++ b/tests/capability-state.test.cjs
@@ -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);
     }