Merge pull request #2049 from open-gsd/fix/1858-flat-layout-skill-manifest
fix(#1858): detect flat commands/gsd-*.md layout in _resolveManifest
This commit is contained in:
Tom Boucher
5
.changeset/steady-ibex-run.md
Normal file
5
.changeset/steady-ibex-run.md
Normal file
@@ -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-<stem>.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)
|
||||
@@ -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<string, string[]>
|
||||
return manifest;
|
||||
}
|
||||
|
||||
/**
|
||||
* #1858 — Build a skill dependency manifest from a FLAT commands/gsd-<stem>.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_<stem>` 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<string, string[]> {
|
||||
const manifest = new Map<string, string[]>();
|
||||
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: <repo>/commands/gsd/*.md).
|
||||
* 2. #1858 — otherwise, if the flat source layout is present (gsd-<stem>.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-<stem>.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<string, string[]> {
|
||||
if (fs.existsSync(commandsGsdDir)) {
|
||||
return loadSkillsManifest(commandsGsdDir);
|
||||
}
|
||||
// #1858: flat source layout — gsd-<stem>.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,
|
||||
};
|
||||
|
||||
@@ -875,6 +875,7 @@ export = {
|
||||
writeActiveProfile,
|
||||
// Shared internals
|
||||
parseRequires,
|
||||
parseCallsAgents,
|
||||
cleanupStagedSkills,
|
||||
// Back-compat / deprecated
|
||||
MINIMAL_SKILL_ALLOWLIST,
|
||||
|
||||
@@ -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-<stem>.md layout (#1858)', () => {
|
||||
// Flat command layout (Claude local project install shape): skills live at
|
||||
// <repo>/commands/gsd-<stem>.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/<stem>.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-<stem>.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_<stem> 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-<stem>.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: <repo>/commands/gsd-<stem>.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 = <repo>/commands/gsd (nested — does NOT exist).
|
||||
// dirname(commandsGsdDir) = <repo>/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/<stem>.md as
|
||||
// commands/gsd-<stem>.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_<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);
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe('regressions: installed-runtime capability surface (#1160)', () => {
|
||||
// Minimal valid SKILL.md content (frontmatter only — matches what install emits)
|
||||
function makeSkillMd(stem) {
|
||||
|
||||
Reference in New Issue
Block a user