diff --git a/sdk/src/query/skills.test.ts b/sdk/src/query/skills.test.ts index cc1556214..db5f42add 100644 --- a/sdk/src/query/skills.test.ts +++ b/sdk/src/query/skills.test.ts @@ -1,80 +1,123 @@ /** * Tests for agent skills query handler. + * + * Verifies the handler reads `config.agent_skills[agentType]` from + * `.planning/config.json` and returns the `` XML block + * workflows interpolate into Task() prompts (regression for #2555). */ -import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { describe, it, expect, beforeEach, afterEach } from 'vitest'; import { mkdtemp, mkdir, rm, writeFile } from 'node:fs/promises'; import { join } from 'node:path'; import { tmpdir } from 'node:os'; import { agentSkills } from './skills.js'; -function writeSkill(rootDir: string, name: string, description = 'Skill under test') { +async function writeSkill(rootDir: string, name: string) { const skillDir = join(rootDir, name); - return mkdir(skillDir, { recursive: true }).then(() => writeFile(join(skillDir, 'SKILL.md'), [ - '---', - `name: ${name}`, - `description: ${description}`, - '---', - '', - `# ${name}`, - ].join('\n'))); + await mkdir(skillDir, { recursive: true }); + await writeFile( + join(skillDir, 'SKILL.md'), + `---\nname: ${name}\ndescription: test skill\n---\n\n# ${name}\n`, + ); +} + +async function writeConfig(projectDir: string, config: unknown) { + await mkdir(join(projectDir, '.planning'), { recursive: true }); + await writeFile(join(projectDir, '.planning', 'config.json'), JSON.stringify(config, null, 2)); } describe('agentSkills', () => { let tmpDir: string; - let homeDir: string; - - it('returns empty string when no agent type (matches gsd-tools)', async () => { - const r = await agentSkills([], tmpdir()); - expect(r.data).toBe(''); - }); beforeEach(async () => { tmpDir = await mkdtemp(join(tmpdir(), 'gsd-skills-')); - homeDir = await mkdtemp(join(tmpdir(), 'gsd-skills-home-')); - await writeSkill(join(tmpDir, '.cursor', 'skills'), 'my-skill'); - await writeSkill(join(tmpDir, '.codex', 'skills'), 'project-codex'); - await mkdir(join(tmpDir, '.claude', 'skills', 'orphaned-dir'), { recursive: true }); - await writeSkill(join(homeDir, '.claude', 'skills'), 'global-claude'); - await writeSkill(join(homeDir, '.codex', 'skills'), 'global-codex'); - await writeSkill(join(homeDir, '.claude', 'get-shit-done', 'skills'), 'legacy-import'); - vi.stubEnv('HOME', homeDir); - // Windows `os.homedir()` reads USERPROFILE, not HOME - vi.stubEnv('USERPROFILE', homeDir); }); afterEach(async () => { - vi.unstubAllEnvs(); await rm(tmpDir, { recursive: true, force: true }); - await rm(homeDir, { recursive: true, force: true }); }); - it('returns deduped skill names from project and managed global skill dirs', async () => { - const r = await agentSkills(['gsd-executor'], tmpDir); - const data = r.data as Record; - const skills = data.skills as string[]; - - expect(skills).toEqual(expect.arrayContaining([ - 'my-skill', - 'project-codex', - 'global-claude', - 'global-codex', - ])); - expect(skills).not.toContain('orphaned-dir'); - expect(skills).not.toContain('legacy-import'); - expect(data.skill_count).toBe(skills.length); + it('returns empty string when no agent type is provided', async () => { + const r = await agentSkills([], tmpDir); + expect(r.data).toBe(''); }); - it('counts deduped skill names when the same skill exists in multiple roots', async () => { - await writeSkill(join(tmpDir, '.claude', 'skills'), 'shared-skill'); - await writeSkill(join(tmpDir, '.agents', 'skills'), 'shared-skill'); + it('returns empty string when project has no config', async () => { + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe(''); + }); - const r = await agentSkills(['gsd-executor'], tmpDir); - const data = r.data as Record; - const skills = data.skills as string[]; + it('returns empty string when agent type not in config.agent_skills', async () => { + await writeConfig(tmpDir, { agent_skills: { 'gsd-executor': ['.claude/skills/foo'] } }); + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe(''); + }); - expect(skills.filter((skill) => skill === 'shared-skill')).toHaveLength(1); - expect(data.skill_count).toBe(skills.length); + it('returns block for each configured skill path', async () => { + await writeSkill(join(tmpDir, '.claude', 'skills'), 'skill-a'); + await writeSkill(join(tmpDir, '.claude', 'skills'), 'skill-b'); + await writeConfig(tmpDir, { + agent_skills: { + 'gsd-planner': ['.claude/skills/skill-a', '.claude/skills/skill-b'], + }, + }); + + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe( + '\n' + + 'Read these user-configured skills:\n' + + '- @.claude/skills/skill-a/SKILL.md\n' + + '- @.claude/skills/skill-b/SKILL.md\n' + + '', + ); + }); + + it('accepts a single string skill path (normalizes to array)', async () => { + await writeSkill(join(tmpDir, '.claude', 'skills'), 'only-one'); + await writeConfig(tmpDir, { + agent_skills: { 'gsd-planner': '.claude/skills/only-one' }, + }); + + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe( + '\n' + + 'Read these user-configured skills:\n' + + '- @.claude/skills/only-one/SKILL.md\n' + + '', + ); + }); + + it('skips skills whose SKILL.md is missing', async () => { + await writeSkill(join(tmpDir, '.claude', 'skills'), 'exists'); + await mkdir(join(tmpDir, '.claude', 'skills', 'missing-md'), { recursive: true }); + await writeConfig(tmpDir, { + agent_skills: { + 'gsd-planner': ['.claude/skills/exists', '.claude/skills/missing-md'], + }, + }); + + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe( + '\n' + + 'Read these user-configured skills:\n' + + '- @.claude/skills/exists/SKILL.md\n' + + '', + ); + }); + + it('rejects path traversal escaping the project root', async () => { + await writeConfig(tmpDir, { + agent_skills: { 'gsd-planner': ['../evil-skill'] }, + }); + + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe(''); + }); + + it('returns empty string when agent_skills value is an empty array', async () => { + await writeConfig(tmpDir, { agent_skills: { 'gsd-planner': [] } }); + const r = await agentSkills(['gsd-planner'], tmpDir); + expect(r.data).toBe(''); }); }); diff --git a/sdk/src/query/skills.ts b/sdk/src/query/skills.ts index 9fe24d962..2ee4f8dd1 100644 --- a/sdk/src/query/skills.ts +++ b/sdk/src/query/skills.ts @@ -1,26 +1,53 @@ /** - * Agent skills query handler — scan installed skill directories. + * Agent skills query handler — read configured skills from `.planning/config.json` + * and emit the `` XML block workflows interpolate into Task() prompts. * - * Reads from project `.claude/skills/`, `.agents/skills/`, `.cursor/skills/`, - * `.github/skills/`, `.codex/skills/`, plus managed global `~/.claude/skills/` - * and `~/.codex/skills/` roots. + * Ports `buildAgentSkillsBlock` semantics from + * `get-shit-done/bin/lib/init.cjs` so the SDK path honors + * `config.agent_skills[agentType]` the same way the legacy + * `gsd-tools.cjs agent-skills ` path does. Fixes #2555. * * @example * ```typescript * import { agentSkills } from './skills.js'; * - * await agentSkills(['gsd-executor'], '/project'); - * // { data: { agent_type: 'gsd-executor', skills: ['plan', 'verify'], skill_count: 2 } } + * // With config.agent_skills = { "gsd-planner": [".claude/skills/demo-skill"] } + * await agentSkills(['gsd-planner'], '/project'); + * // { data: '\nRead these user-configured skills:\n- @.claude/skills/demo-skill/SKILL.md\n' } + * + * // No agent type → empty string (matches gsd-tools cmdAgentSkills). * await agentSkills([], '/project'); - * // { data: '' } — matches gsd-tools when no agent type is passed + * // { data: '' } * ``` */ -import { existsSync, readdirSync } from 'node:fs'; -import { join } from 'node:path'; +import { existsSync, realpathSync } from 'node:fs'; +import { join, resolve, sep } from 'node:path'; import { homedir } from 'node:os'; import type { QueryHandler } from './utils.js'; +import { loadConfig } from '../config.js'; + +const GLOBAL_SKILL_NAME_RE = /^[a-zA-Z0-9_-]+$/; + +/** + * Resolve `target` and ensure it stays inside `baseDir` after symlink resolution. + * Mirrors the symlink-escape guard in `bin/lib/security.cjs#validatePath`. + */ +function resolveWithinBase(target: string, baseDir: string): string | null { + try { + const resolvedBase = existsSync(baseDir) ? realpathSync(baseDir) : resolve(baseDir); + const absTarget = resolve(baseDir, target); + const resolvedTarget = existsSync(absTarget) ? realpathSync(absTarget) : absTarget; + const baseWithSep = resolvedBase.endsWith(sep) ? resolvedBase : resolvedBase + sep; + if (resolvedTarget !== resolvedBase && !resolvedTarget.startsWith(baseWithSep)) { + return null; + } + return resolvedTarget; + } catch { + return null; + } +} export const agentSkills: QueryHandler = async (args, projectDir) => { const agentType = (args[0] || '').trim(); @@ -28,35 +55,75 @@ export const agentSkills: QueryHandler = async (args, projectDir) => { if (!agentType) { return { data: '' }; } - const skillDirs = [ - join(projectDir, '.claude', 'skills'), - join(projectDir, '.agents', 'skills'), - join(projectDir, '.cursor', 'skills'), - join(projectDir, '.github', 'skills'), - join(projectDir, '.codex', 'skills'), - join(homedir(), '.claude', 'skills'), - join(homedir(), '.codex', 'skills'), - ]; - const skills: string[] = []; - for (const dir of skillDirs) { - if (!existsSync(dir)) continue; - try { - const entries = readdirSync(dir, { withFileTypes: true }); - for (const entry of entries) { - if (!entry.isDirectory()) continue; - if (!existsSync(join(dir, entry.name, 'SKILL.md'))) continue; - skills.push(entry.name); - } - } catch { /* skip */ } + let config; + try { + config = await loadConfig(projectDir); + } catch { + return { data: '' }; } - const dedupedSkills = [...new Set(skills)]; + const raw = config.agent_skills?.[agentType]; + if (!raw) return { data: '' }; + + let skillPaths: unknown[]; + if (typeof raw === 'string') { + skillPaths = [raw]; + } else if (Array.isArray(raw)) { + skillPaths = raw; + } else { + return { data: '' }; + } + if (skillPaths.length === 0) return { data: '' }; + + const globalSkillsBase = join(homedir(), '.claude', 'skills'); + const validEntries: Array<{ ref: string }> = []; + + for (const entry of skillPaths) { + if (typeof entry !== 'string') continue; + + // `global:` — skill installed under ~/.claude/skills// (#1992). + if (entry.startsWith('global:')) { + const skillName = entry.slice(7); + if (!skillName) { + process.stderr.write('[agent-skills] WARNING: "global:" prefix with empty skill name — skipping\n'); + continue; + } + if (!GLOBAL_SKILL_NAME_RE.test(skillName)) { + process.stderr.write(`[agent-skills] WARNING: Invalid global skill name "${skillName}" — skipping\n`); + continue; + } + const skillDir = join(globalSkillsBase, skillName); + const skillMd = join(skillDir, 'SKILL.md'); + if (!existsSync(skillMd)) { + process.stderr.write(`[agent-skills] WARNING: Global skill not found at "~/.claude/skills/${skillName}/SKILL.md" — skipping\n`); + continue; + } + if (resolveWithinBase(skillMd, globalSkillsBase) === null) { + process.stderr.write(`[agent-skills] WARNING: Global skill "${skillName}" failed path check (symlink escape?) — skipping\n`); + continue; + } + validEntries.push({ ref: `~/.claude/skills/${skillName}/SKILL.md` }); + continue; + } + + // Project-relative path — must resolve within projectDir. + if (resolveWithinBase(entry, projectDir) === null) { + process.stderr.write(`[agent-skills] WARNING: Skipping unsafe path "${entry}"\n`); + continue; + } + const skillMd = join(projectDir, entry, 'SKILL.md'); + if (!existsSync(skillMd)) { + process.stderr.write(`[agent-skills] WARNING: Skill not found at "${entry}/SKILL.md" — skipping\n`); + continue; + } + validEntries.push({ ref: `${entry}/SKILL.md` }); + } + + if (validEntries.length === 0) return { data: '' }; + + const lines = validEntries.map((e) => `- @${e.ref}`).join('\n'); return { - data: { - agent_type: agentType, - skills: dedupedSkills, - skill_count: dedupedSkills.length, - }, + data: `\nRead these user-configured skills:\n${lines}\n`, }; };