From 8ee66ade7760bdd518eee5d17bee3f854ef1a63b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 9 Jun 2026 10:25:55 -0400 Subject: [PATCH] fix(#929): cmdSkillManifest discovers nested concrete skills (#933) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Scans `gsd-ns-/skills//SKILL.md` in addition to the existing flat `/SKILL.md` layout, so gsd-health and gsd-settings report the correct concrete skill count on nested-layout runtimes (cline, qwen, hermes, augment, trae, antigravity). Guard: descent into a `skills/` subdir is restricted to `gsd-ns-*` router directories — unrelated user dirs that happen to have a `skills/` subdir are not traversed. Dual-routed concretes (same name under two routers) are deduped within each root. Adds a negative-case regression test: verifies that a non-`gsd-ns-*` dir (e.g. `my-tool/`, `gsd-settings/`) with its own `skills/` subdir does NOT contribute nested entries to the manifest. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .changeset/gallant-lemurs-roar.md | 5 + src/init.cts | 68 +++++++-- tests/skill-manifest.test.cjs | 222 ++++++++++++++++++++++++++++++ 3 files changed, 283 insertions(+), 12 deletions(-) create mode 100644 .changeset/gallant-lemurs-roar.md diff --git a/.changeset/gallant-lemurs-roar.md b/.changeset/gallant-lemurs-roar.md new file mode 100644 index 000000000..5e5380a22 --- /dev/null +++ b/.changeset/gallant-lemurs-roar.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 929 +--- +`cmdSkillManifest` now discovers concrete skills nested under `gsd-ns-*` routers (`/gsd-ns-/skills//SKILL.md`), so `gsd-health` and `gsd-settings` report the correct count on nested-layout runtimes (cline, qwen, hermes, augment, trae, antigravity). The scan is scoped to `gsd-ns-*` router dirs only — unrelated user dirs that happen to have a `skills/` subdirectory are not traversed. Dual-routed concretes (same skill installed under two routers) are deduped by name within each root. (#929) diff --git a/src/init.cts b/src/init.cts index a6379a169..5430342ce 100644 --- a/src/init.cts +++ b/src/init.cts @@ -2146,18 +2146,26 @@ function buildSkillManifest(cwd: string, skillsDir: string | null = null): Skill continue; } - let skillCount = 0; - for (const entry of entries) { - if (!entry.isDirectory()) continue; - - const skillMdPath = path.join(rootPath, entry.name, 'SKILL.md'); - const content = platformReadSync(skillMdPath); - if (content === null) continue; + // Track skill names seen within this root to deduplicate dual-routed concretes + // (e.g. spec-phase nested under both gsd-ns-workflow and gsd-ns-manage). + const seenNamesInRoot = new Set(); + function pushSkillEntry( + // relPath must use forward slashes on all platforms (manifest paths are + // posix-style for cross-platform stability; flat entries use template + // literals that always produce '/'; nested entries are joined below + // with explicit '/' separators rather than path.join). + relPath: string, + content: string, + ): boolean { const frontmatter = extractFrontmatter(content); - const name = (frontmatter['name'] as string) || entry.name; - const description = (frontmatter['description'] as string) || ''; + const dirPart = relPath.replace(/\/SKILL\.md$/, ''); + const stem = dirPart.includes('/') ? dirPart.split('/').pop()! : dirPart; + const name = (frontmatter['name'] as string) || stem; + if (seenNamesInRoot.has(name)) return false; // dedupe dual-routed concretes + seenNamesInRoot.add(name); + const description = (frontmatter['description'] as string) || ''; const triggers: string[] = []; const bodyMatch = content.match(/^---[\s\S]*?---\s*\n([\s\S]*)$/); if (bodyMatch) { @@ -2175,14 +2183,50 @@ function buildSkillManifest(cwd: string, skillsDir: string | null = null): Skill name, description, triggers, - path: entry.name, - file_path: `${entry.name}/SKILL.md`, + path: dirPart, + file_path: relPath, root: rootInfo.root, scope: rootInfo.scope, installed: rootInfo.scope !== 'import-only', deprecated: !!rootInfo.deprecated, }); - skillCount++; + return true; + } + + let skillCount = 0; + for (const entry of entries) { + if (!entry.isDirectory()) continue; + + const skillMdPath = path.join(rootPath, entry.name, 'SKILL.md'); + const content = platformReadSync(skillMdPath); + if (content !== null) { + if (pushSkillEntry(`${entry.name}/SKILL.md`, content)) skillCount++; + } + + // Nested layout: /skills//SKILL.md + // Used by cline, qwen, hermes, augment, trae, antigravity (#69 nested=true). + // Descend exactly one level into /skills/ — no deeper recursion. + // Scope to gsd-ns-* routers only: never vacuum up an unrelated user skill + // that happens to have its own `skills/` subdirectory. + if (!entry.name.startsWith('gsd-ns-')) continue; + const nestedSkillsDir = path.join(rootPath, entry.name, 'skills'); + let nestedEntries: fs.Dirent[] = []; + try { + nestedEntries = fs.readdirSync(nestedSkillsDir, { withFileTypes: true }); + } catch { + // No skills/ subdir — flat layout or unreadable; nothing to do. + nestedEntries = []; + } + for (const nested of nestedEntries) { + if (!nested.isDirectory()) continue; + const nestedSkillMd = path.join(nestedSkillsDir, nested.name, 'SKILL.md'); + const nestedContent = platformReadSync(nestedSkillMd); + if (nestedContent === null) continue; + // Use forward-slash separator explicitly so manifest paths are posix-style + // on all platforms, matching the flat-layout behaviour above. + const relPath = `${entry.name}/skills/${nested.name}/SKILL.md`; + if (pushSkillEntry(relPath, nestedContent)) skillCount++; + } } rootSummary.skill_count = skillCount; diff --git a/tests/skill-manifest.test.cjs b/tests/skill-manifest.test.cjs index c8117ca5f..c80461b61 100644 --- a/tests/skill-manifest.test.cjs +++ b/tests/skill-manifest.test.cjs @@ -148,4 +148,226 @@ describe('skill-manifest', () => { assert.strictEqual(claudeRoot.path, path.join(homeDir, 'claude-custom', 'skills')); assert.strictEqual(codexRoot.path, path.join(homeDir, 'codex-custom', 'skills')); }); + + // bug-929: nested layout discovery + test('bug-929: discovers concrete skills nested under gsd-ns-* routers', () => { + // Mirrors the on-disk shape that stageSkillsForRuntimeAsSkills emits for + // cline/qwen/hermes/augment/trae/antigravity when nested=true: + // /gsd-ns-workflow/SKILL.md — router (top-level) + // /gsd-ns-workflow/skills/plan/SKILL.md — concrete + // /gsd-ns-workflow/skills/execute/SKILL.md — concrete + // /gsd-ns-workflow/skills/spec-phase/SKILL.md — dual-routed concrete + // /gsd-ns-manage/SKILL.md — router (top-level) + // /gsd-ns-manage/skills/progress/SKILL.md — concrete + // /gsd-ns-manage/skills/spec-phase/SKILL.md — same dual-routed concrete (dedupe by name) + // /gsd-standalone/SKILL.md — flat top-level skill (no skills/ subdir) + const skillsDir = fs.mkdtempSync(path.join(require('os').tmpdir(), 'gsd-nested-skills-')); + + function writeNestedSkill(dir, name, description) { + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'SKILL.md'), [ + '---', + `name: ${name}`, + `description: ${description}`, + '---', + '', + `# ${name}`, + ].join('\n')); + } + + // Router 1: gsd-ns-workflow + writeNestedSkill(path.join(skillsDir, 'gsd-ns-workflow'), 'gsd-ns-workflow', 'Workflow router'); + writeNestedSkill(path.join(skillsDir, 'gsd-ns-workflow', 'skills', 'plan'), 'gsd-plan', 'Plan skill'); + writeNestedSkill(path.join(skillsDir, 'gsd-ns-workflow', 'skills', 'execute'), 'gsd-execute', 'Execute skill'); + writeNestedSkill(path.join(skillsDir, 'gsd-ns-workflow', 'skills', 'spec-phase'), 'gsd-spec-phase', 'Spec phase skill'); + + // Router 2: gsd-ns-manage + writeNestedSkill(path.join(skillsDir, 'gsd-ns-manage'), 'gsd-ns-manage', 'Manage router'); + writeNestedSkill(path.join(skillsDir, 'gsd-ns-manage', 'skills', 'progress'), 'gsd-progress', 'Progress skill'); + // Same spec-phase under a second router (dual-routed); must appear exactly once in manifest + writeNestedSkill(path.join(skillsDir, 'gsd-ns-manage', 'skills', 'spec-phase'), 'gsd-spec-phase', 'Spec phase skill'); + + // Flat top-level skill (not a router, no skills/ subdir) + writeNestedSkill(path.join(skillsDir, 'gsd-standalone'), 'gsd-standalone', 'Standalone flat skill'); + + const result = runGsdTools(['skill-manifest', '--skills-dir', skillsDir], tmpDir); + assert.ok(result.success, `Command should succeed: ${result.error || result.output}`); + + const manifest = JSON.parse(result.output); + const skillNames = manifest.skills.map((s) => s.name).sort(); + + // 2 routers + 4 unique concretes (gsd-spec-phase deduped) + 1 flat = 7 total + assert.deepStrictEqual(skillNames, [ + 'gsd-execute', + 'gsd-ns-manage', + 'gsd-ns-workflow', + 'gsd-plan', + 'gsd-progress', + 'gsd-spec-phase', + 'gsd-standalone', + ]); + assert.strictEqual(manifest.counts.skills, 7, 'dual-routed concrete must be deduped to one entry'); + + // Concrete skills should have a forward-slash nested file_path (posix-stable on all platforms) + const planSkill = manifest.skills.find((s) => s.name === 'gsd-plan'); + assert.ok(planSkill, 'gsd-plan should be discovered'); + assert.ok( + planSkill.file_path.includes('skills/plan'), + `gsd-plan file_path should reflect nested location with forward slashes, got: ${planSkill.file_path}` + ); + + // Router should also appear as a skill entry + const routerSkill = manifest.skills.find((s) => s.name === 'gsd-ns-workflow'); + assert.ok(routerSkill, 'gsd-ns-workflow router should be discovered as a top-level skill'); + + cleanup(skillsDir); + }); + + test('bug-929: discovers nested concretes even when router has no top-level SKILL.md', () => { + // Edge case: a router dir has a skills/ subdir with concretes but no top-level SKILL.md. + // The concrete skills should still be discovered. + const skillsDir = fs.mkdtempSync(path.join(require('os').tmpdir(), 'gsd-router-only-skills-')); + + // Router dir with skills/ but no SKILL.md of its own + const concreteDir = path.join(skillsDir, 'gsd-ns-noroot', 'skills', 'orphan-skill'); + fs.mkdirSync(concreteDir, { recursive: true }); + fs.writeFileSync(path.join(concreteDir, 'SKILL.md'), [ + '---', + 'name: gsd-orphan', + 'description: Orphan skill under router without top-level SKILL.md', + '---', + '', + '# gsd-orphan', + ].join('\n')); + + const result = runGsdTools(['skill-manifest', '--skills-dir', skillsDir], tmpDir); + assert.ok(result.success, `Command should succeed: ${result.error || result.output}`); + + const manifest = JSON.parse(result.output); + assert.deepStrictEqual( + manifest.skills.map((s) => s.name).sort(), + ['gsd-orphan'], + ); + assert.strictEqual(manifest.counts.skills, 1); + + cleanup(skillsDir); + }); + + test('bug-929: flat layout (no nested skills/ subdirs) still works correctly', () => { + const skillsDir = fs.mkdtempSync(path.join(require('os').tmpdir(), 'gsd-flat-skills-')); + + function writeFlat(name, description) { + const dir = path.join(skillsDir, name); + fs.mkdirSync(dir, { recursive: true }); + fs.writeFileSync(path.join(dir, 'SKILL.md'), [ + '---', + `name: ${name}`, + `description: ${description}`, + '---', + '', + `# ${name}`, + ].join('\n')); + } + + writeFlat('gsd-alpha', 'Alpha skill'); + writeFlat('gsd-beta', 'Beta skill'); + writeFlat('gsd-gamma', 'Gamma skill'); + + const result = runGsdTools(['skill-manifest', '--skills-dir', skillsDir], tmpDir); + assert.ok(result.success, `Command should succeed: ${result.error || result.output}`); + + const manifest = JSON.parse(result.output); + assert.deepStrictEqual( + manifest.skills.map((s) => s.name).sort(), + ['gsd-alpha', 'gsd-beta', 'gsd-gamma'] + ); + assert.strictEqual(manifest.counts.skills, 3, 'flat layout count should be exact, no phantom nesting'); + + cleanup(skillsDir); + }); + + test('bug-929: non-gsd-ns-* dirs with a skills/ subdir are NOT scanned (guard)', () => { + // Regression guard for the `if (!entry.name.startsWith('gsd-ns-')) continue;` guard + // in buildSkillManifest. A user tool dir like `my-tool/` that happens to have its + // own `skills/` subdirectory must NOT have those skills vacuumed up. + // Only `gsd-ns-/skills//SKILL.md` paths are in scope. + const skillsDir = fs.mkdtempSync(path.join(require('os').tmpdir(), 'gsd-guard-test-')); + + // Non-router dir with a flat SKILL.md at its own root — SHOULD be found (flat scan). + const topLevelDir = path.join(skillsDir, 'my-tool'); + fs.mkdirSync(topLevelDir, { recursive: true }); + fs.writeFileSync(path.join(topLevelDir, 'SKILL.md'), [ + '---', + 'name: my-tool', + 'description: A user-defined top-level skill', + '---', + '', + '# my-tool', + ].join('\n')); + + // Non-router dir with a nested skills/ subdir — nested skills must NOT be discovered. + const nestedDir = path.join(skillsDir, 'my-tool', 'skills', 'helper'); + fs.mkdirSync(nestedDir, { recursive: true }); + fs.writeFileSync(path.join(nestedDir, 'SKILL.md'), [ + '---', + 'name: my-tool-helper', + 'description: A nested skill that must not be vacuumed up', + '---', + '', + '# my-tool-helper', + ].join('\n')); + + // Another non-router dir (prefixed differently, could look router-like but isn't) + const otherDir = path.join(skillsDir, 'gsd-settings'); + fs.mkdirSync(otherDir, { recursive: true }); + fs.writeFileSync(path.join(otherDir, 'SKILL.md'), [ + '---', + 'name: gsd-settings', + 'description: A flat gsd-* skill that is not a router', + '---', + '', + '# gsd-settings', + ].join('\n')); + // Give gsd-settings its own skills/ subdir — must not be traversed since it's not gsd-ns-* + const otherNestedDir = path.join(skillsDir, 'gsd-settings', 'skills', 'subsetting'); + fs.mkdirSync(otherNestedDir, { recursive: true }); + fs.writeFileSync(path.join(otherNestedDir, 'SKILL.md'), [ + '---', + 'name: gsd-subsetting', + 'description: A nested skill that must not be vacuumed up', + '---', + '', + '# gsd-subsetting', + ].join('\n')); + + const result = runGsdTools(['skill-manifest', '--skills-dir', skillsDir], tmpDir); + assert.ok(result.success, `Command should succeed: ${result.error || result.output}`); + + const manifest = JSON.parse(result.output); + const skillNames = manifest.skills.map((s) => s.name).sort(); + + // Only the flat top-level SKILL.md entries should be found; nested non-router skills are ignored + assert.deepStrictEqual( + skillNames, + ['gsd-settings', 'my-tool'], + 'nested skills under non-gsd-ns-* dirs must not be discovered', + ); + assert.strictEqual( + manifest.counts.skills, + 2, + 'only 2 top-level skills; nested non-router helpers must not inflate the count', + ); + + // Confirm the forbidden names are absent + assert.ok( + !skillNames.includes('my-tool-helper'), + 'my-tool/skills/helper/SKILL.md must not appear (guard: my-tool is not gsd-ns-*)', + ); + assert.ok( + !skillNames.includes('gsd-subsetting'), + 'gsd-settings/skills/subsetting/SKILL.md must not appear (guard: gsd-settings is not gsd-ns-*)', + ); + + cleanup(skillsDir); + }); });