From 8b006263de1d23b87ec415258fc0abc670dbd364 Mon Sep 17 00:00:00 2001 From: Michal Date: Sat, 8 Aug 2026 18:16:22 +0100 Subject: [PATCH] fix(skills): write the frontmatter every Agent Skills host requires MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mcpd keeps `name` and `description` as columns, and a skill's `content` is often just the body — `propose-learnings` starts straight at its `#` heading. The sync wrote `full.content` verbatim, so the SKILL.md landed with no YAML frontmatter and the host rejected it. Observed live: [Skill warning] ~/.prime/agent/skills/propose-learnings/SKILL.md description is required Claude Code and pi require the same keys, so this affected every target; it only surfaced now because prime-agent prints the warning at startup. `ensureSkillFrontmatter` synthesises the block from the columns when it is absent, and fills in only a missing `name`/`description` when the author already supplied a header — an existing complete header is never rewritten. Values are emitted as JSON strings (valid YAML double-quoted scalars) because descriptions routinely contain `:` and `#`, which break a bare scalar. Also: `--force` now re-fetches skills whose server content is unchanged. Without it a skill already on disk could never be repaired by a client-side fix — the content hash still matched, so it was skipped forever, which is exactly what happened on the first attempt to repair the file above. Three existing assertions compared SKILL.md to the raw server content; they now assert the body survives rather than exact bytes. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_017BMXdb2qZbPSh8Q7XpTyjB --- src/cli/src/commands/skills.ts | 12 ++++- src/cli/src/utils/skills-disk.ts | 34 ++++++++++++ .../tests/utils/prime-agent-skills.test.ts | 11 ++-- src/cli/tests/utils/skill-frontmatter.test.ts | 53 +++++++++++++++++++ 4 files changed, 104 insertions(+), 6 deletions(-) create mode 100644 src/cli/tests/utils/skill-frontmatter.test.ts 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: ""'); + }); +});