diff --git a/.changeset/steady-ibex-run.md b/.changeset/steady-ibex-run.md new file mode 100644 index 000000000..bf1b37a3b --- /dev/null +++ b/.changeset/steady-ibex-run.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2049 +--- +**Skill-bearing capabilities now surface correctly on flat command-layout installs** — on an install using the flat `commands/gsd-.md` source layout (e.g. a Claude Code local project install with no `commands/gsd/` subdir), every skill-bearing capability (`nyquist`, `code-review`, `security`, `ui`, `mempalace`, `ai-integration`, `profile-pipeline`) was silently reported `surfaced:false`/`enabled:false`/`active:false`, so their loop hooks (`verify:post`, `execute:post`, etc.) never fired even with the corresponding `workflow.*` toggle on. The skill-manifest resolver now detects the flat layout and produces the same stems the nested `commands/gsd/*.md` loader does. (#1858) diff --git a/src/capability-state.cts b/src/capability-state.cts index a43b16f3b..9527e930a 100644 --- a/src/capability-state.cts +++ b/src/capability-state.cts @@ -46,7 +46,7 @@ const { loadConfig } = configLoaderMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import installProfilesMod = require('./install-profiles.cjs'); -const { readActiveProfile, loadSkillsManifest, resolveProfile, parseRequires } = installProfilesMod; +const { readActiveProfile, loadSkillsManifest, resolveProfile, parseRequires, parseCallsAgents } = installProfilesMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import surfaceMod = require('./surface.cjs'); @@ -384,22 +384,84 @@ function _loadInstalledSkillsManifest(configDir: string): Map return manifest; } +/** + * #1858 — Build a skill dependency manifest from a FLAT commands/gsd-.md + * source layout (the Claude local project install shape, where the `gsd-` + * prefix is baked into each filename at the commands/ level and there is no + * commands/gsd/ subdir). Strips the `gsd-` prefix so stems match the nested + * loader's output (gsd-validate-phase.md → validate-phase, same as nested + * validate-phase.md). + * + * Map shape is identical to loadSkillsManifest: each stem maps to its + * `requires` deps (parsed via the same shared parseRequires) and carries a + * companion `_calls_agents_` key (parsed via parseCallsAgents) so the + * flat and nested paths cannot drift. + * + * Returns an empty Map when the parent directory does not exist or contains + * no gsd-*.md files (so _resolveManifest can use size>0 as the "flat layout + * present" signal and fall through to the installed-skills branch otherwise). + */ +function _loadFlatCommandsGsdManifest(commandsParentDir: string): Map { + const manifest = new Map(); + let entries: fs.Dirent[]; + try { + entries = fs.readdirSync(commandsParentDir, { withFileTypes: true }); + } catch { + return manifest; + } + for (const entry of entries) { + if (!entry.isFile()) continue; + if (!entry.name.startsWith('gsd-')) continue; + if (!entry.name.endsWith('.md')) continue; + // Strip 'gsd-' prefix (4 chars) and '.md' suffix (3 chars) → stem. + const stem = entry.name.slice(4, -3); + if (!stem) continue; + // 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 { + const content = fs.readFileSync(path.join(commandsParentDir, entry.name), 'utf8'); + manifest.set(stem, parseRequires(content)); + manifest.set(`_calls_agents_${stem}`, parseCallsAgents(content)); + } catch { + manifest.set(stem, []); + manifest.set(`_calls_agents_${stem}`, []); + } + } + return manifest; +} + /** * Resolve the skill dependency manifest for capability-state resolution. * - * Resolution order (fixes #1160 — installed-runtime capability surface): - * 1. If commandsGsdDir exists, load from source (repo-checkout behavior). - * 2. Otherwise, fall back to installed skills at configDir/skills/gsd-[stem]/SKILL.md. + * Resolution order: + * 1. If commandsGsdDir exists, load from the nested source layout + * (repo-checkout behavior: /commands/gsd/*.md). + * 2. #1858 — otherwise, if the flat source layout is present (gsd-.md + * files in dirname(commandsGsdDir)), load from there. This is the Claude + * local project install shape where commands/gsd/ does not exist but + * commands/gsd-.md files do. + * 3. #1160 — otherwise, fall back to installed skills at + * configDir/skills/gsd-[stem]/SKILL.md. * - * In an installed runtime the commands/gsd source tree is absent; only the - * skills/ layout exists. Returning an empty manifest caused resolveSurface to - * materialise the full-sentinel to an empty Set, making every capability appear - * unsurfaced even when the skill was physically installed. + * In an installed runtime both source trees are absent; only the skills/ + * layout exists. Returning an empty manifest caused resolveSurface to + * materialise the full-sentinel to an empty Set, making every skill-bearing + * capability appear unsurfaced even when the skill was physically installed + * (#1160) or authored as a flat command file (#1858). */ function _resolveManifest(commandsGsdDir: string, configDir: string): Map { if (fs.existsSync(commandsGsdDir)) { return loadSkillsManifest(commandsGsdDir); } + // #1858: flat source layout — gsd-.md files at dirname(commandsGsdDir). + // Only claim the flat branch when it actually has gsd-*.md files; otherwise + // fall through to the installed-skills branch (a commands/ dir with no gsd + // files must not shadow an installed skills/ tree). + const flat = _loadFlatCommandsGsdManifest(path.dirname(commandsGsdDir)); + if (flat.size > 0) return flat; return _loadInstalledSkillsManifest(configDir); } @@ -619,6 +681,7 @@ export = { // Exported for tests _resolveCommandsGsdDir, _loadInstalledSkillsManifest, + _loadFlatCommandsGsdManifest, _resolveManifest, _isSafePropKey, }; diff --git a/src/install-profiles.cts b/src/install-profiles.cts index 90674c367..474327ae5 100644 --- a/src/install-profiles.cts +++ b/src/install-profiles.cts @@ -875,6 +875,7 @@ export = { writeActiveProfile, // Shared internals parseRequires, + parseCallsAgents, cleanupStagedSkills, // Back-compat / deprecated MINIMAL_SKILL_ALLOWLIST, diff --git a/tests/capability-state.test.cjs b/tests/capability-state.test.cjs index 66bba4e9c..456e8ee84 100644 --- a/tests/capability-state.test.cjs +++ b/tests/capability-state.test.cjs @@ -22,6 +22,7 @@ const { isCapabilityActive, _isSafePropKey, _loadInstalledSkillsManifest, + _loadFlatCommandsGsdManifest, _resolveManifest, } = require('../gsd-core/bin/lib/capability-state.cjs'); @@ -823,6 +824,223 @@ describe('cmdCapabilityState — end-to-end via gsd-tools CLI', () => { // FAIL before the fix and PASS after, regardless of whether commands/gsd // happens to exist in the current checkout. +describe('regressions: flat commands/gsd-.md layout (#1858)', () => { + // Flat command layout (Claude local project install shape): skills live at + // /commands/gsd-.md — the gsd- prefix is baked into the filename + // and there is NO commands/gsd/ subdir. _resolveManifest must detect this + // layout, strip the gsd- prefix, and produce the same stems the nested + // loader (commands/gsd/.md) would, or every skill-bearing capability + // is silently reported surfaced:false / enabled:false / active:false. + function makeFlatCommandMd(stem, requires) { + const req = requires ? `requires: [${requires.join(', ')}]` : 'requires: [phase]'; + return [ + '---', + `name: gsd:${stem}`, + `description: ${stem} skill`, + 'argument-hint: "[phase number]"', + 'allowed-tools:', + ' - Read', + req, + '---', + 'Execute end-to-end.', + ].join('\n') + '\n'; + } + + // ── Unit tests for _loadFlatCommandsGsdManifest ───────────────────────────── + + test('_loadFlatCommandsGsdManifest: returns empty map when parent dir absent', () => { + const missing = path.join(os.tmpdir(), 'cap-flat-missing-' + Date.now()); + const manifest = _loadFlatCommandsGsdManifest(missing); + assert.ok(manifest instanceof Map, 'should return a Map'); + assert.strictEqual(manifest.size, 0, 'should be empty when parent dir absent'); + }); + + test('_loadFlatCommandsGsdManifest: scans gsd-.md and strips the gsd- prefix', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-flat-scan-')); + try { + fs.writeFileSync(path.join(tmpDir, 'gsd-validate-phase.md'), makeFlatCommandMd('validate-phase'), 'utf8'); + fs.writeFileSync(path.join(tmpDir, 'gsd-secure-phase.md'), makeFlatCommandMd('secure-phase'), 'utf8'); + // Non-gsd file must be ignored + fs.writeFileSync(path.join(tmpDir, 'random-doc.md'), '# not a skill\n', 'utf8'); + // Non-markdown gsd file must be ignored + fs.writeFileSync(path.join(tmpDir, 'gsd-notskill.txt'), 'nope\n', 'utf8'); + + const manifest = _loadFlatCommandsGsdManifest(tmpDir); + assert.ok(manifest.has('validate-phase'), 'flat gsd-validate-phase.md -> stem validate-phase'); + assert.ok(manifest.has('secure-phase'), 'flat gsd-secure-phase.md -> stem secure-phase'); + assert.ok(!manifest.has('gsd-validate-phase'), 'must NOT keep the gsd- prefix on the stem'); + assert.ok(!manifest.has('random-doc'), 'non-gsd file must be ignored'); + assert.ok(!manifest.has('notskill'), 'non-.md gsd file must be ignored'); + } finally { + cleanup(tmpDir); + } + }); + + test('_loadFlatCommandsGsdManifest: parses requires via shared parseRequires (no drift)', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-flat-req-')); + try { + fs.writeFileSync( + path.join(tmpDir, 'gsd-my-skill.md'), + makeFlatCommandMd('my-skill', ['dep-a', 'dep-b']), + 'utf8', + ); + const manifest = _loadFlatCommandsGsdManifest(tmpDir); + assert.deepStrictEqual(manifest.get('my-skill'), ['dep-a', 'dep-b']); + } finally { + cleanup(tmpDir); + } + }); + + test('_loadFlatCommandsGsdManifest: companion _calls_agents_ key present (parity with nested loader)', () => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-flat-agents-')); + try { + fs.writeFileSync(path.join(tmpDir, 'gsd-validate-phase.md'), makeFlatCommandMd('validate-phase'), 'utf8'); + const manifest = _loadFlatCommandsGsdManifest(tmpDir); + assert.ok(manifest.has('_calls_agents_validate-phase'), + 'flat loader must emit the companion _calls_agents_ key (same Map shape as loadSkillsManifest)'); + } finally { + cleanup(tmpDir); + } + }); + + // 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-.md layout when nested commands/gsd/ is absent (#1858)', () => { + const tmpRepo = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rm-flat-repo-')); + try { + // Flat source layout: /commands/gsd-.md + // NO commands/gsd/ subdir, NO skills/ dir. + const commandsDir = path.join(tmpRepo, 'commands'); + fs.mkdirSync(commandsDir, { recursive: true }); + fs.writeFileSync(path.join(commandsDir, 'gsd-validate-phase.md'), makeFlatCommandMd('validate-phase'), 'utf8'); + fs.writeFileSync(path.join(commandsDir, 'gsd-secure-phase.md'), makeFlatCommandMd('secure-phase'), 'utf8'); + + // commandsGsdDir = /commands/gsd (nested — does NOT exist). + // dirname(commandsGsdDir) = /commands (where the flat files live). + const commandsGsdDir = path.join(commandsDir, 'gsd'); + // configDir = a separate empty tmp dir (no skills/ → installed fallback empty). + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rm-flat-cfg-')); + try { + const manifest = _resolveManifest(commandsGsdDir, configDir); + assert.ok(manifest.has('validate-phase'), + 'flat layout must populate validate-phase stem (was empty pre-fix → all skill caps unsurfaced)'); + assert.ok(manifest.has('secure-phase'), 'flat layout must populate secure-phase stem'); + assert.ok(!manifest.has('gsd-validate-phase'), 'stem must have gsd- prefix stripped'); + } finally { + cleanup(configDir); + } + } finally { + cleanup(tmpRepo); + } + }); + + test('_resolveManifest: flat branch does NOT shadow a populated installed skills dir when no flat files exist', () => { + // Precedence: nested > flat-source > installed. If the flat parent dir has + // NO gsd-*.md files, the flat loader returns an empty Map and _resolveManifest + // must fall through to the installed-skills branch (not return empty). + const tmpRepo = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rm-precedence-')); + try { + const commandsDir = path.join(tmpRepo, 'commands'); + fs.mkdirSync(commandsDir, { recursive: true }); + // No gsd-*.md files in commands/ — flat loader yields empty. + // Installed skills present under configDir: + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-rm-precedence-cfg-')); + try { + const secureDir = path.join(configDir, 'skills', 'gsd-secure-phase'); + fs.mkdirSync(secureDir, { recursive: true }); + fs.writeFileSync(path.join(secureDir, 'SKILL.md'), + '---\nname: gsd:secure-phase\nrequires: [phase]\n---\nbody\n', 'utf8'); + + const commandsGsdDir = path.join(commandsDir, 'gsd'); // nested absent + const manifest = _resolveManifest(commandsGsdDir, configDir); + assert.ok(manifest.has('secure-phase'), + 'when flat dir is empty, installed-skills fallback must still work (precedence flat > installed only when flat non-empty)'); + } finally { + cleanup(configDir); + } + } finally { + cleanup(tmpRepo); + } + }); + + // ── Parity: flat loader produces the same stems as the nested loader ──────── + // (DEFECT.GENERATIVE-FIX — guards against silent divergence between the two + // parallel manifest-builders.) + + test('flat loader and nested loader produce identical stems for the same command set (parity)', () => { + const realCommandsGsdDir = path.resolve(__dirname, '..', 'commands', 'gsd'); + if (!fs.existsSync(realCommandsGsdDir)) return; // skip outside a repo checkout + // Build a flat mirror of the real nested commands/gsd/.md as + // commands/gsd-.md in a temp dir, then compare stem sets. + const tmpFlat = fs.mkdtempSync(path.join(os.tmpdir(), 'cap-parity-flat-')); + try { + const nested = require('../gsd-core/bin/lib/install-profiles.cjs').loadSkillsManifest(realCommandsGsdDir); + for (const [stem] of nested) { + if (stem.startsWith('_calls_agents_')) continue; + const src = path.join(realCommandsGsdDir, stem + '.md'); + if (!fs.existsSync(src)) continue; + fs.writeFileSync(path.join(tmpFlat, 'gsd-' + stem + '.md'), fs.readFileSync(src, 'utf8'), 'utf8'); + } + const flat = _loadFlatCommandsGsdManifest(tmpFlat); + const nestedStems = [...nested.keys()].filter((k) => !k.startsWith('_calls_agents_')).sort(); + 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_ 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); + } + }); +}); + describe('regressions: installed-runtime capability surface (#1160)', () => { // Minimal valid SKILL.md content (frontmatter only — matches what install emits) function makeSkillMd(stem) {