diff --git a/src/cli/src/commands/skills.ts b/src/cli/src/commands/skills.ts index 7c53397..89f95e6 100644 --- a/src/cli/src/commands/skills.ts +++ b/src/cli/src/commands/skills.ts @@ -14,6 +14,7 @@ import { } from '../utils/skills-state.js'; import { installSkillAtomic, + ensureSkillFrontmatter, removeSkillAtomic, type SkillBody, } from '../utils/skills-disk.js'; @@ -230,7 +231,11 @@ export async function runSkillsSync(opts: SyncOpts, deps: SyncDeps): Promise(`/api/v1/skills/${encodeURIComponent(v.id)}`); const body: SkillBody = { - content: full.content, + // mcpd keeps name/description as columns, so a skill whose content is + // just the body would land on disk without the frontmatter every host + // requires and be silently rejected. + content: ensureSkillFrontmatter(full.content, full.name || v.name, full.description || v.description || ''), ...(Object.keys(full.files ?? {}).length > 0 ? { files: full.files } : {}), }; const fileStates = await installSkillAtomic(targetDir, body); diff --git a/src/cli/src/utils/skills-disk.ts b/src/cli/src/utils/skills-disk.ts index a58c15a..23d2264 100644 --- a/src/cli/src/utils/skills-disk.ts +++ b/src/cli/src/utils/skills-disk.ts @@ -18,6 +18,40 @@ export interface SkillBody { files?: Record; } +/** + * Guarantee the YAML frontmatter every Agent Skills host requires. + * + * mcpd stores `name` and `description` as columns, and a skill's `content` is + * frequently just the body — `propose-learnings` starts straight at its `#` + * heading. Written verbatim, such a skill is rejected by the agent that loads + * it (prime-agent logs `description is required` and never loads it; Claude + * Code and pi need the same keys), so the sync has to put the frontmatter back. + * + * Content that already carries a block is left alone apart from filling in a + * missing `name`/`description` — we never rewrite a skill author's own header. + */ +export function ensureSkillFrontmatter(content: string, name: string, description: string): string { + const match = /^---[ \t]*\r?\n([\s\S]*?)\r?\n---[ \t]*\r?\n?/.exec(content); + if (match === null) { + return `---\nname: ${yamlScalar(name)}\ndescription: ${yamlScalar(description)}\n---\n\n${content}`; + } + const block = match[1] ?? ''; + const additions: string[] = []; + if (!/^name[ \t]*:/m.test(block)) additions.push(`name: ${yamlScalar(name)}`); + if (!/^description[ \t]*:/m.test(block)) additions.push(`description: ${yamlScalar(description)}`); + if (additions.length === 0) return content; + return `---\n${[block, ...additions].join('\n')}\n---\n${content.slice(match[0].length)}`; +} + +/** + * Emit a YAML scalar. Descriptions routinely contain `:` and `#`, which break a + * bare scalar — a JSON string is a valid YAML double-quoted scalar and escapes + * quotes and newlines correctly. + */ +function yamlScalar(value: string): string { + return JSON.stringify(value); +} + /** * Write a skill atomically into `targetDir`. If a previous install exists, * it's renamed to `.mcpctl-trash-` and rmtree'd after the diff --git a/src/cli/tests/utils/prime-agent-skills.test.ts b/src/cli/tests/utils/prime-agent-skills.test.ts index 284c346..72f14a8 100644 --- a/src/cli/tests/utils/prime-agent-skills.test.ts +++ b/src/cli/tests/utils/prime-agent-skills.test.ts @@ -192,7 +192,8 @@ describe('runPrimeAgentSkillsSync', () => { const af = { 'a-1': { id: 'a-1', name: 'x-skill', description: 'd', semver: '1.0.0', contentHash: 'sha256:a', content: '# version-a\n', files: {} } }; const clientA = mockClient({ visible: av, full: af }); await runPrimeAgentSkillsSync({ project: 'projA', installRoot, statePath }, deps(clientA)); - expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toBe('# version-a\n'); + // Frontmatter is synthesised on write, so assert the body, not exact bytes. + expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toContain('# version-a'); // Project B also has a skill named X with different content. const bv = [ @@ -204,7 +205,8 @@ describe('runPrimeAgentSkillsSync', () => { expect(resultB.preserved).toContain('x-skill'); // A's version is untouched (not clobbered by B's). - expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toBe('# version-a\n'); + // Frontmatter is synthesised on write, so assert the body, not exact bytes. + expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toContain('# version-a'); }); it('does not delete legacy, ownership-less state belonging to another project', async () => { @@ -279,7 +281,7 @@ describe('runPrimeAgentSkillsSync', () => { expect(resultB.updated).toContain('shared-global'); expect(resultB.preserved).toEqual([]); - expect(readFileSync(join(installRoot, 'shared-global', 'SKILL.md'), 'utf-8')).toBe('# v2\n'); + expect(readFileSync(join(installRoot, 'shared-global', 'SKILL.md'), 'utf-8')).toContain('# v2'); }); it('does not let a global-only sync clobber a project-owned skill', async () => { @@ -299,7 +301,8 @@ describe('runPrimeAgentSkillsSync', () => { const result = await runPrimeAgentSkillsSync({ cwd: empty, installRoot, statePath }, deps(mockClient({ visible: gv, full: gf }))); expect(result.preserved).toContain('x-skill'); - expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toBe('# version-a\n'); + // Frontmatter is synthesised on write, so assert the body, not exact bytes. + expect(readFileSync(join(installRoot, 'x-skill', 'SKILL.md'), 'utf-8')).toContain('# version-a'); }); it('removes global orphans on a global-only sync', async () => { diff --git a/src/cli/tests/utils/skill-frontmatter.test.ts b/src/cli/tests/utils/skill-frontmatter.test.ts new file mode 100644 index 0000000..dce0a11 --- /dev/null +++ b/src/cli/tests/utils/skill-frontmatter.test.ts @@ -0,0 +1,53 @@ +import { describe, it, expect } from 'vitest'; +import { ensureSkillFrontmatter } from '../../src/utils/skills-disk.js'; + +/** + * mcpd stores name/description as columns; a skill's `content` is often just + * the body. Written verbatim it lands on disk without the frontmatter every + * Agent Skills host requires — prime-agent logs "description is required" and + * refuses to load it. Observed live with the `propose-learnings` skill. + */ +describe('ensureSkillFrontmatter', () => { + it('adds frontmatter to a body that has none', () => { + const out = ensureSkillFrontmatter('# propose-learnings\n\nBody text.\n', 'propose-learnings', 'How to capture knowledge.'); + expect(out).toBe( + '---\nname: "propose-learnings"\ndescription: "How to capture knowledge."\n---\n\n# propose-learnings\n\nBody text.\n', + ); + }); + + it('leaves an existing complete header untouched', () => { + const original = '---\nname: commit\ndescription: Commit changes\n---\n\n# commit\n'; + expect(ensureSkillFrontmatter(original, 'commit', 'something else')).toBe(original); + }); + + it('fills in only the missing key, preserving the author\'s other fields', () => { + const out = ensureSkillFrontmatter('---\nname: x\nallowed-tools: Bash\n---\n\nBody\n', 'x', 'The description'); + expect(out).toContain('allowed-tools: Bash'); + expect(out).toContain('description: "The description"'); + // name was already there — not duplicated + expect(out.match(/name:/g)).toHaveLength(1); + expect(out).toContain('\n\nBody\n'); + }); + + it('quotes descriptions containing YAML metacharacters', () => { + // A bare scalar breaks on ':' and '#'; these are common in descriptions. + const out = ensureSkillFrontmatter('body', 'n', 'Use when: debugging #hard problems'); + expect(out).toContain('description: "Use when: debugging #hard problems"'); + }); + + it('escapes quotes and newlines rather than emitting invalid YAML', () => { + const out = ensureSkillFrontmatter('body', 'n', 'He said "hi"\nthen left'); + expect(out).toContain('description: "He said \\"hi\\"\\nthen left"'); + }); + + it('handles CRLF frontmatter delimiters', () => { + const out = ensureSkillFrontmatter('---\r\nname: x\r\n---\r\nBody', 'x', 'D'); + expect(out).toContain('description: "D"'); + expect(out).toContain('Body'); + }); + + it('still emits the key when the description is empty', () => { + // Can't invent one, but the key must exist for the host to parse it. + expect(ensureSkillFrontmatter('body', 'n', '')).toContain('description: ""'); + }); +});