diff --git a/packages/agents/src/commands/__tests__/sync.test.ts b/packages/agents/src/commands/__tests__/sync.test.ts index 41d8410d..e6c64d21 100644 --- a/packages/agents/src/commands/__tests__/sync.test.ts +++ b/packages/agents/src/commands/__tests__/sync.test.ts @@ -535,6 +535,45 @@ describe(syncCommand, () => { expect(existsSync(path.dirname(skillPath('people-report')))).toBe(false); }); + + it('applies include expansion and tool-name and link rewriting when deploying a declared skill', async () => { + await mkdir(path.join(contentDir, 'subagents', '_data'), { recursive: true }); + await writeFile( + path.join(contentDir, 'subagents', '_data', 'claude.yaml'), + '_tools:\n Read: open_files\n', + 'utf8', + ); + const skillDir = path.join(contentDir, 'skills', 'demo'); + await mkdir(path.join(skillDir, '_partials'), { recursive: true }); + await writeFile( + path.join(skillDir, 'SKILL.md'), + '---\nname: demo\ndeploy: declared\n---\n\n\n\nUse {tool:Read}. See [guide](./guide.md).\n', + 'utf8', + ); + await writeFile(path.join(skillDir, '_partials', 'frag.md'), 'Shared fragment.\n', 'utf8'); + await writeFile(path.join(skillDir, 'guide.md'), '# Guide\n', 'utf8'); + await declareSkills('demo'); + + await syncCommand(makeOptions(), projectRoot, contentDir); + + const skill = await readFile(skillPath('demo'), 'utf8'); + expect(skill).toContain('Shared fragment.'); + expect(skill).toContain('Use open_files.'); + expect(skill).toContain('[guide](~/.claude/skills/demo/guide.md)'); + expect(skill).not.toContain('{tool:Read}'); + expect(existsSync(path.join(projectRoot, '.claude', 'skills', 'demo', '_partials'))).toBe(false); + }); + + it('fails before writing when a declared skill has an unmapped tool placeholder, dry-run included', async () => { + await writeLibrarySkill('demo', { body: 'Use {tool:Read}.' }); + await declareSkills('demo'); + + await expect(syncCommand(makeOptions({ dryRun: true }), projectRoot, contentDir)).rejects.toThrow( + /Unmapped tool name "Read" in skills\/demo\/SKILL\.md/, + ); + await expect(syncCommand(makeOptions(), projectRoot, contentDir)).rejects.toThrow(/Unmapped tool name "Read"/); + expect(existsSync(skillPath('demo'))).toBe(false); + }); }); describe('declared subagents', () => { diff --git a/packages/agents/src/commands/install.ts b/packages/agents/src/commands/install.ts index 292f572d..c2495700 100644 --- a/packages/agents/src/commands/install.ts +++ b/packages/agents/src/commands/install.ts @@ -6,6 +6,7 @@ import { readDeploy } from '../lib/deploy-frontmatter.ts'; import { expandIncludes } from '../lib/directive-expander.ts'; import { pruneOrphanedEntries } from '../lib/entry-remover.ts'; import { HARNESSES, resolveHarnessIds, resolveHarnessPaths } from '../lib/harness.js'; +import { loadHarnessOverlay } from '../lib/harness-overlay.ts'; import { checkSymlinkSafety, copyItem, linkItem, removeItem, unlinkIfSymlink } from '../lib/installer.ts'; import { computeContentHash, @@ -21,9 +22,10 @@ import { injectMarkersInDirectory, injectProvenanceMarker, } from '../lib/marker-injector.js'; -import { rewritePathsInDirectory, rewritePathsInFile } from '../lib/path-rewriter.js'; +import { rewritePathsInFile } from '../lib/path-rewriter.js'; import { renderPromptsYml } from '../lib/prompts-yml.ts'; -import { loadSubagentOverlay, renderSubagentForHarness } from '../lib/subagent-transform.ts'; +import { type RenderedSkillEntry, renderSkillDirectory } from '../lib/skill-transform.ts'; +import { renderSubagentForHarness } from '../lib/subagent-transform.ts'; import { loadToolMapping, rewriteToolNames } from '../lib/tool-name-rewriter.js'; import { isEnoent, isMissingFile } from '../lib/type-guards.ts'; import type { @@ -87,7 +89,7 @@ export async function installCommand( // Load the harness overlay once per harness. The raw YAML feeds the frontmatter merger (subagents only); // the parsed `_tools:` mapping feeds the body-text placeholder rewriter (subagents and skills). const harnessConfig = HARNESSES[harnessId]; - const overlayYaml = await loadSubagentOverlay(contentDir, harnessConfig); + const overlayYaml = await loadHarnessOverlay(contentDir, harnessConfig); const toolMapping = loadToolMapping(overlayYaml); // Install skills (shared + harness-specific) @@ -306,18 +308,21 @@ async function installSkillEntry( toolMapping: ReadonlyMap, label = '', ): Promise { - // Eagerly resolves include directives at source-tree level. Run before the dry-run gate so missing targets, cycles, - // and out-of-tree references surface even when no files are written. - // Directory entries traverse their tree and cache each expanded `.md` file's content; - // file entries expand the file directly. - // After expansion, applies the tool-name rewriter in-memory so the cached content carries the final body text the - // write phase will emit — no second disk pass, no read-back-from-disk. + // Eagerly render the skill's final body before the dry-run gate, so missing include targets, cycles, out-of-tree + // references, and unmapped tool placeholders surface even when no files are written. Directory entries render the + // whole tree (include expansion, tool-name + link/template rewriting) into the exact bytes the write phase emits; + // single-file `.md` entries expand and tool-rewrite directly. const srcStats = await stat(srcPath); let expandedFileContent: string | undefined; - let expandedDirContents: ReadonlyMap | undefined; + let renderedDir: ReadonlyArray | undefined; if (srcStats.isDirectory()) { - const rawExpanded = await preExpandSkillDirectory(srcPath, contentDir); - expandedDirContents = rewriteToolNamesInExpansionMap(rawExpanded, contentDir, toolMapping); + renderedDir = await renderSkillDirectory(srcPath, path.basename(destPath), { + contentDir, + toolMapping, + pathPrefix: skillsPrefix, + homeDir, + harnessId, + }); } else if (srcPath.endsWith('.md')) { const expanded = await expandIncludes(srcPath, contentDir); expandedFileContent = rewriteToolNames(expanded, toolMapping, relativeFromContent(contentDir, srcPath)); @@ -339,12 +344,9 @@ async function installSkillEntry( } if (srcStats.isDirectory()) { - // Per-file walk: Writes expanded `.md` files from the cache populated during the pre-expand pass, - // mirrors the directory structure to the destination, and copies non-`.md` files plainly. - // The `_partials/` exclusion is applied during the walk. - // The cache is non-undefined here because srcStats.isDirectory() implies the directory branch above ran. - if (expandedDirContents === undefined) { - throw new Error(`Invariant violation: expandedDirContents undefined for directory ${srcPath}`); + // The rendered tree is non-undefined here because srcStats.isDirectory() implies the directory branch above ran. + if (renderedDir === undefined) { + throw new Error(`Invariant violation: renderedDir undefined for directory ${srcPath}`); } // Clean-write directories CodeAssembly previously installed: remove the prior copy so files deleted from the // source skill don't survive in the destination. Gated on prior ownership (a manifest entry exists) so a @@ -352,9 +354,7 @@ async function installSkillEntry( if (existingEntry) { await removeItem(destPath); } - await writeExpandedSkillDir(srcPath, destPath, expandedDirContents); - const skillsDestDir = path.dirname(destPath); - await rewritePathsInDirectory(destPath, skillsDestDir, skillsPrefix, homeDir, harnessId); + await writeRenderedSkillDir(destPath, renderedDir); await injectMarkersInDirectory(destPath, (fileRelPath) => buildSourceUrl(`${sourceRelativeRoot}/${fileRelPath}`)); } else if (srcPath.endsWith('.md') && expandedFileContent !== undefined) { // Single-file `.md` skill entries: write the previously expanded content directly. @@ -376,67 +376,14 @@ async function installSkillEntry( } /** - * Eagerly walks a skill source directory, runs `expandIncludes` on each `.md` file to surface include errors before - * any file is written, and returns a map keyed by absolute source path with the expanded content. - * `_partials/` directories are skipped because their contents are referenced through includes, not installed. - * The returned map is consumed by `writeExpandedSkillDir` so each `.md` file is expanded once per install. - */ -async function preExpandSkillDirectory(srcDir: string, contentDir: string): Promise> { - const expandedBySrcPath = new Map(); - await collectExpansions(srcDir, contentDir, expandedBySrcPath); - return expandedBySrcPath; -} - -async function collectExpansions( - srcDir: string, - contentDir: string, - expandedBySrcPath: Map, -): Promise { - const entries = await readdir(srcDir); - for (const entry of entries) { - if (entry === '_partials' || entry.startsWith('.')) { - continue; - } - const fullPath = path.join(srcDir, entry); - const info = await stat(fullPath); - if (info.isDirectory()) { - await collectExpansions(fullPath, contentDir, expandedBySrcPath); - } else if (entry.endsWith('.md')) { - expandedBySrcPath.set(fullPath, await expandIncludes(fullPath, contentDir)); - } - } -} - -/** - * Recursively writes a skill source directory to the destination. `.md` files are read from the pre-computed expansion - * cache (populated by `preExpandSkillDirectory`); non-`.md` files are copied verbatim. - * `_partials/` subdirectories are skipped at any depth — their contents are include targets, not installed artifacts. + * Writes a rendered skill directory to `destDir`: markdown entries are written from their transformed content, asset + * entries are copied verbatim from source. Each entry's parent directory is created as needed. */ -async function writeExpandedSkillDir( - srcDir: string, - destDir: string, - expandedBySrcPath: ReadonlyMap, -): Promise { - await mkdir(destDir, { recursive: true }); - const entries = await readdir(srcDir); +async function writeRenderedSkillDir(destDir: string, entries: ReadonlyArray): Promise { for (const entry of entries) { - if (entry === '_partials' || entry.startsWith('.')) { - continue; - } - const srcPath = path.join(srcDir, entry); - const destPath = path.join(destDir, entry); - const info = await stat(srcPath); - if (info.isDirectory()) { - await writeExpandedSkillDir(srcPath, destPath, expandedBySrcPath); - } else if (entry.endsWith('.md')) { - const expanded = expandedBySrcPath.get(srcPath); - if (expanded === undefined) { - throw new Error(`Invariant violation: pre-expand cache missing entry for ${srcPath}`); - } - await writeFile(destPath, expanded, 'utf8'); - } else { - await copyItem(srcPath, destPath); - } + const destPath = path.join(destDir, entry.relPath); + await mkdir(path.dirname(destPath), { recursive: true }); + await (entry.kind === 'markdown' ? writeFile(destPath, entry.content, 'utf8') : copyItem(entry.srcPath, destPath)); } } @@ -858,21 +805,3 @@ async function installHarnessGuidance( function relativeFromContent(contentDir: string, srcPath: string): string { return path.relative(contentDir, srcPath).split(path.sep).join('/'); } - -/** - * Returns a new map mirroring `rawExpanded`, with every value processed through the tool-name rewriter. - * The map is preserved by-reference (same keys, same iteration order); only the string values change. Each entry's - * `contextLabel` is its content-relative POSIX path, so an unmapped placeholder surfaces a usable file reference. - */ -function rewriteToolNamesInExpansionMap( - rawExpanded: ReadonlyMap, - contentDir: string, - toolMapping: ReadonlyMap, -): Map { - const rewritten = new Map(); - for (const [absSrcPath, content] of rawExpanded) { - const label = relativeFromContent(contentDir, absSrcPath); - rewritten.set(absSrcPath, rewriteToolNames(content, toolMapping, label)); - } - return rewritten; -} diff --git a/packages/agents/src/commands/sync.ts b/packages/agents/src/commands/sync.ts index 74012229..ceb78e2c 100644 --- a/packages/agents/src/commands/sync.ts +++ b/packages/agents/src/commands/sync.ts @@ -10,18 +10,19 @@ import { resolveContentDir } from '../lib/content-resolver.ts'; import { resolveClosure } from '../lib/dependency-resolver.ts'; import { readFileOrEmpty, writeIfChanged } from '../lib/fs-helpers.ts'; import { HARNESSES, resolveHarnessIds, resolveHarnessPaths } from '../lib/harness.ts'; +import { loadHarnessOverlay } from '../lib/harness-overlay.ts'; import { renderPromptsYml } from '../lib/prompts-yml.ts'; import { parseRulebookFile } from '../lib/rulebook-schema.ts'; import { extractRulebookSkillSlug, renderSkillFile, resolveSkillName } from '../lib/rulebook-skill.ts'; import { extractInstalledSlugs, injectRulebook, removeRulebook } from '../lib/sentinel-inliner.ts'; import { deploySkill, resolveDeclaredSkill, type ResolvedSkill } from '../lib/skill-deploy.ts'; +import { renderSkillDirectory, type SkillDeployContext } from '../lib/skill-transform.ts'; import { deploySubagent, resolveDeclaredSubagent, type ResolvedSubagent, type SubagentDeployContext, } from '../lib/subagent-deploy.ts'; -import { loadSubagentOverlay } from '../lib/subagent-transform.ts'; import { loadToolMapping } from '../lib/tool-name-rewriter.ts'; import { isEnoent, isMissingFile } from '../lib/type-guards.ts'; import type { HarnessId, InstallOptions } from '../lib/types.ts'; @@ -45,6 +46,12 @@ interface HarnessSubagentTarget { readonly deployContext: SubagentDeployContext; } +/** One targeted harness's project-local skills dir paired with the per-harness inputs the skill transform needs. */ +interface HarnessSkillTarget { + readonly skillsDir: string; + readonly deployContext: SkillDeployContext; +} + /** The one per-domain difference: the base dir to resolve and deploy under, and the file that hosts ambient blocks. */ export interface SyncDomain { readonly baseDir: string; @@ -166,10 +173,16 @@ async function reconcileDomain( assertNoCrossNamespaceCollisions([...desiredSkillDirs.values()], declaredSkillSet); // Skill delivery targets project-local harness skills dirs, gated by detection (or `--harness`). Passing - // `projectRoot` as the base is what keeps the skills project-scoped, and keeps tests out of the real home dir. - const harnessSkillDirs = resolveHarnessIds(options.harness, domain.baseDir).map( - (harnessId) => resolveHarnessPaths(harnessId, domain.baseDir).skillsDir, + // `projectRoot` as the base is what keeps the skills project-scoped, and keeps tests out of the real home dir. Each + // target carries the per-harness skill-transform inputs so declared-skill deployment applies include expansion and + // tool-name/link rewriting. `harnessSkillDirs` is the plain dir list the rulebook-skill passes and orphan scans use, + // which need no transform context. + const harnessSkillTargets = await Promise.all( + resolveHarnessIds(options.harness, domain.baseDir).map((harnessId) => + resolveSkillTarget(harnessId, domain.baseDir, contentDir), + ), ); + const harnessSkillDirs = harnessSkillTargets.map((target) => target.skillsDir); // Subagent delivery targets each harness's project-local subagents dir, loading that harness's overlay and tool // mapping so the deploy applies the same transform `install` would. Resolved separately from skills because the @@ -225,6 +238,11 @@ async function reconcileDomain( collectOwnedTargets(harnessSkillDirs, resolved, resolvedSkills, harnessSubagentTargets, resolvedSubagents), ); + // Render every declared skill against every targeted harness up front, so a broken include or an unmapped tool + // placeholder fails the whole run — dry-run included — before any file is written. The rendered output is discarded + // here; `deploySkill` re-renders it at write time. + await assertDeclaredSkillsRender(harnessSkillTargets, resolvedSkills); + if (options.dryRun) { reportDryRun({ ambientHostName: path.basename(ambientHostPath), @@ -290,14 +308,7 @@ async function reconcileDomain( // Reconcile declared skills per targeted harness, independently of the rulebook-skill pass above: retract owned // declared-skill dirs no longer declared, then deploy each declared skill into `//`. - for (const { skillsDir, orphans } of declaredSkillOrphansByDir) { - for (const dir of orphans) { - await rm(path.join(skillsDir, dir), { recursive: true, force: true }); - } - for (const skill of resolvedSkills) { - await deploySkill(skill, path.join(skillsDir, skill.slug)); - } - } + await reconcileDeclaredSkills(harnessSkillTargets, declaredSkillOrphansByDir, resolvedSkills); // Reconcile declared subagents per targeted harness, independently of the skill passes: retract owned subagent // files no longer declared, then deploy each declared subagent as `/.md` with the harness @@ -594,6 +605,43 @@ async function reconcileDeclaredSubagents( } } +/** + * Retracts owned declared-skill dirs no longer declared, then deploys each declared skill into every targeted harness's + * skills dir with that harness's transform applied. Orphans were computed against the pre-write filesystem, so + * retracting before writing lets a slug freed in one dir be re-created in the same sync rather than clobbered. + */ +async function reconcileDeclaredSkills( + targets: ReadonlyArray, + orphansByDir: ReadonlyArray<{ skillsDir: string; orphans: ReadonlyArray }>, + resolvedSkills: ReadonlyArray, +): Promise { + for (const target of targets) { + const orphans = orphansByDir.find((entry) => entry.skillsDir === target.skillsDir)?.orphans ?? []; + for (const dir of orphans) { + await rm(path.join(target.skillsDir, dir), { recursive: true, force: true }); + } + for (const skill of resolvedSkills) { + await deploySkill(skill, path.join(target.skillsDir, skill.slug), target.deployContext); + } + } +} + +/** + * Renders every declared skill against every targeted harness, discarding the output, so a broken include or an + * unmapped tool placeholder throws before any file is written. The deploy pass re-renders at write time; this pass + * exists only to fail the run closed, including under `--dry-run`. + */ +async function assertDeclaredSkillsRender( + targets: ReadonlyArray, + resolvedSkills: ReadonlyArray, +): Promise { + for (const target of targets) { + for (const skill of resolvedSkills) { + await renderSkillDirectory(skill.srcDir, skill.slug, target.deployContext); + } + } +} + /** * Regenerates the home-domain Rovo Dev `prompts.yml` so home-deployed skills appear in its available-skills list. As a * pure projection of the on-disk skills dir, it also drops any just-retracted skill. A no-op for the repo domain (which @@ -706,7 +754,7 @@ async function resolveSubagentTarget( contentDir: string, ): Promise { const harnessConfig = HARNESSES[harnessId]; - const overlayYaml = await loadSubagentOverlay(contentDir, harnessConfig); + const overlayYaml = await loadHarnessOverlay(contentDir, harnessConfig); return { subagentsDir: resolveHarnessPaths(harnessId, projectRoot).subagentsDir, deployContext: { @@ -719,4 +767,28 @@ async function resolveSubagentTarget( }; } +/** + * Resolves one harness's project-local skills dir together with the per-harness inputs the skill transform needs: the + * tool-name mapping (from the harness overlay), the link-rewrite prefix, the home-dir segment, and the harness id. + * Passing `projectRoot` as the base keeps delivery project-scoped. + */ +async function resolveSkillTarget( + harnessId: HarnessId, + projectRoot: string, + contentDir: string, +): Promise { + const harnessConfig = HARNESSES[harnessId]; + const overlayYaml = await loadHarnessOverlay(contentDir, harnessConfig); + return { + skillsDir: resolveHarnessPaths(harnessId, projectRoot).skillsDir, + deployContext: { + contentDir, + toolMapping: loadToolMapping(overlayYaml), + pathPrefix: `${harnessConfig.homeDir}/${harnessConfig.skillsDirName}`, + homeDir: harnessConfig.homeDir, + harnessId: harnessConfig.id, + }, + }; +} + // endregion | Helpers diff --git a/packages/agents/src/lib/__tests__/harness-overlay.test.ts b/packages/agents/src/lib/__tests__/harness-overlay.test.ts new file mode 100644 index 00000000..76e0c93c --- /dev/null +++ b/packages/agents/src/lib/__tests__/harness-overlay.test.ts @@ -0,0 +1,33 @@ +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { HARNESSES } from '../harness.ts'; +import { loadHarnessOverlay } from '../harness-overlay.ts'; + +const OVERLAY = '_tools:\n Read: Read\n'; + +describe(loadHarnessOverlay, () => { + let contentDir: string; + + beforeEach(async () => { + contentDir = path.join(tmpdir(), `agents-test-overlay-${Date.now()}-${Math.random().toString(36).slice(2)}`); + await mkdir(path.join(contentDir, 'subagents', '_data'), { recursive: true }); + }); + + afterEach(async () => { + await rm(contentDir, { recursive: true, force: true }); + }); + + it('reads the harness overlay file from subagents/_data', async () => { + await writeFile(path.join(contentDir, 'subagents', '_data', 'claude.yaml'), OVERLAY, 'utf8'); + + expect(await loadHarnessOverlay(contentDir, HARNESSES.claude)).toBe(OVERLAY); + }); + + it('returns an empty string when the overlay file is absent', async () => { + expect(await loadHarnessOverlay(contentDir, HARNESSES.rovodev)).toBe(''); + }); +}); diff --git a/packages/agents/src/lib/__tests__/skill-deploy.test.ts b/packages/agents/src/lib/__tests__/skill-deploy.test.ts index 0ee86993..62e9f016 100644 --- a/packages/agents/src/lib/__tests__/skill-deploy.test.ts +++ b/packages/agents/src/lib/__tests__/skill-deploy.test.ts @@ -5,7 +5,11 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { expandIncludes } from '../directive-expander.ts'; +import { rewriteMarkdownPaths, rewriteTemplateVariables } from '../path-rewriter.ts'; import { deploySkill, resolveDeclaredSkill } from '../skill-deploy.ts'; +import { type SkillDeployContext } from '../skill-transform.ts'; +import { rewriteToolNames } from '../tool-name-rewriter.ts'; describe(resolveDeclaredSkill, () => { let librarySkillsDir: string; @@ -71,71 +75,129 @@ describe(deploySkill, () => { await rm(destParent, { recursive: true, force: true }); }); - /** Writes a library skill directory from a map of relative file paths to contents. */ - async function writeLibrarySkill(slug: string, files: Record): Promise { - for (const [relPath, content] of Object.entries(files)) { - const full = path.join(librarySkillsDir, slug, relPath); - await mkdir(path.dirname(full), { recursive: true }); - await writeFile(full, content, 'utf8'); - } - } - it('writes the SKILL.md with the ownership marker into the destination', async () => { await writeLibrarySkill('people-report', { 'SKILL.md': '---\nname: people-report\n---\n\n# People report\n' }); - const skill = resolvedSkill('people-report'); const destDir = path.join(destParent, 'people-report'); - await deploySkill(skill, destDir); + await deploySkill(resolvedSkill('people-report'), destDir, context()); const deployed = await readFile(path.join(destDir, 'SKILL.md'), 'utf8'); expect(deployed).toContain(''); expect(deployed).toContain('# People report'); }); - it('re-deploys an unchanged single-file skill without rewriting SKILL.md', async () => { + it('re-deploys an unchanged skill without rewriting SKILL.md', async () => { await writeLibrarySkill('people-report', { 'SKILL.md': '---\nname: people-report\n---\n\n# People report\n' }); - const skill = resolvedSkill('people-report'); const destDir = path.join(destParent, 'people-report'); - await deploySkill(skill, destDir); + await deploySkill(resolvedSkill('people-report'), destDir, context()); const firstMtime = statSync(path.join(destDir, 'SKILL.md')).mtimeMs; - await deploySkill(skill, destDir); + await deploySkill(resolvedSkill('people-report'), destDir, context()); expect(statSync(path.join(destDir, 'SKILL.md')).mtimeMs).toBe(firstMtime); }); - it('mirrors auxiliary files and subdirectories verbatim, marking only the root SKILL.md', async () => { + it('mirrors auxiliary assets verbatim and marks only the root SKILL.md', async () => { await writeLibrarySkill('multi', { 'SKILL.md': '---\nname: multi\n---\n\n# Multi\n', 'reference.md': '# Reference\n', 'data/table.csv': 'a,b\n1,2\n', }); - const skill = resolvedSkill('multi'); const destDir = path.join(destParent, 'multi'); - await deploySkill(skill, destDir); + await deploySkill(resolvedSkill('multi'), destDir, context()); expect(await readFile(path.join(destDir, 'reference.md'), 'utf8')).toBe('# Reference\n'); expect(await readFile(path.join(destDir, 'data', 'table.csv'), 'utf8')).toBe('a,b\n1,2\n'); - // The marker belongs only on the root SKILL.md. expect(await readFile(path.join(destDir, 'reference.md'), 'utf8')).not.toContain('codeassembly-skill'); }); it('removes a destination file the source no longer carries on re-deploy', async () => { await writeLibrarySkill('multi', { 'SKILL.md': '---\nname: multi\n---\n\n# Multi\n', 'stale.md': 'old\n' }); const destDir = path.join(destParent, 'multi'); - await deploySkill(resolvedSkill('multi'), destDir); + await deploySkill(resolvedSkill('multi'), destDir, context()); expect(existsSync(path.join(destDir, 'stale.md'))).toBe(true); await rm(path.join(librarySkillsDir, 'multi', 'stale.md')); - await deploySkill(resolvedSkill('multi'), destDir); + await deploySkill(resolvedSkill('multi'), destDir, context()); expect(existsSync(path.join(destDir, 'stale.md'))).toBe(false); expect(existsSync(path.join(destDir, 'SKILL.md'))).toBe(true); }); + it('expands includes and rewrites tool placeholders and bare-relative links in deployed .md files', async () => { + await writeLibrarySkill('demo', { + 'SKILL.md': + '---\nname: demo\n---\n\n\n\nUse {tool:Read}. See [guide](./reference/guide.md).\n', + '_partials/frag.md': 'Shared fragment.\n', + 'reference/guide.md': 'Back to [home](../SKILL.md). Run `{harness_home_dir}/x`.\n', + }); + const destDir = path.join(destParent, 'demo'); + + await deploySkill(resolvedSkill('demo'), destDir, context(new Map([['Read', 'open_files']]))); + + const skillMd = await readFile(path.join(destDir, 'SKILL.md'), 'utf8'); + expect(skillMd).toContain('Shared fragment.'); + expect(skillMd).toContain('Use open_files.'); + expect(skillMd).toContain('[guide](~/.claude/skills/demo/reference/guide.md)'); + expect(skillMd).not.toContain('{tool:Read}'); + const guide = await readFile(path.join(destDir, 'reference', 'guide.md'), 'utf8'); + expect(guide).toContain('[home](~/.claude/skills/demo/SKILL.md)'); + expect(guide).toContain('~/.claude/x'); + // The partial is an include target, never deployed. + expect(existsSync(path.join(destDir, '_partials'))).toBe(false); + }); + + it('deploys the same body install composes from the shared transform steps, markers aside', async () => { + const toolMapping = new Map([['Read', 'open_files']]); + await writeLibrarySkill('demo', { + 'SKILL.md': + '---\nname: demo\n---\n\nUse {tool:Read}. See [guide](./reference/guide.md). Run `{harness_home_dir}/x`.\n', + 'reference/guide.md': '# Guide\n', + }); + const destDir = path.join(destParent, 'demo'); + + await deploySkill(resolvedSkill('demo'), destDir, context(toolMapping)); + + // Recompose install's pipeline for SKILL.md independently: expand → tools → markdown paths → template vars. + const expanded = await expandIncludes(path.join(librarySkillsDir, 'demo', 'SKILL.md'), librarySkillsDir); + const tooled = rewriteToolNames(expanded, toolMapping, 'demo/SKILL.md'); + const pathed = rewriteMarkdownPaths(tooled, 'demo/SKILL.md', '.claude/skills'); + const expected = rewriteTemplateVariables(pathed, '.claude', 'claude'); + + const deployed = await readFile(path.join(destDir, 'SKILL.md'), 'utf8'); + const withoutMarker = deployed + .split('\n') + .filter((line) => !line.includes('codeassembly-skill:')) + .join('\n'); + expect(withoutMarker).toBe(expected); + }); + + // region | Helpers + + function context(toolMapping: ReadonlyMap = new Map()): SkillDeployContext { + return { + contentDir: librarySkillsDir, + toolMapping, + pathPrefix: '.claude/skills', + homeDir: '.claude', + harnessId: 'claude', + }; + } + + /** Writes a library skill directory from a map of relative file paths to contents. */ + async function writeLibrarySkill(slug: string, files: Record): Promise { + for (const [relPath, content] of Object.entries(files)) { + const full = path.join(librarySkillsDir, slug, relPath); + await mkdir(path.dirname(full), { recursive: true }); + await writeFile(full, content, 'utf8'); + } + } + /** Builds a ResolvedSkill without the deploy-field check, so deploySkill tests can use minimal fixtures. */ function resolvedSkill(slug: string): { slug: string; srcDir: string } { return { slug, srcDir: path.join(librarySkillsDir, slug) }; } + + // endregion | Helpers }); diff --git a/packages/agents/src/lib/__tests__/skill-transform.test.ts b/packages/agents/src/lib/__tests__/skill-transform.test.ts new file mode 100644 index 00000000..c04dd0e0 --- /dev/null +++ b/packages/agents/src/lib/__tests__/skill-transform.test.ts @@ -0,0 +1,114 @@ +import { mkdir, rm, writeFile } from 'node:fs/promises'; +import { tmpdir } from 'node:os'; +import path from 'node:path'; + +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; + +import { DirectiveExpansionError } from '../directive-expander.ts'; +import { type RenderedSkillEntry, renderSkillDirectory, type SkillDeployContext } from '../skill-transform.ts'; +import { ToolNameRewriteError } from '../tool-name-rewriter.ts'; + +const TOOL_MAPPING = new Map([['Read', 'open_files']]); + +describe(renderSkillDirectory, () => { + let contentDir: string; + let skillDir: string; + + beforeEach(async () => { + const stamp = `${Date.now()}-${Math.random().toString(36).slice(2)}`; + contentDir = path.join(tmpdir(), `agents-test-st-${stamp}`); + skillDir = path.join(contentDir, 'skills', 'demo'); + await mkdir(skillDir, { recursive: true }); + }); + + afterEach(async () => { + await rm(contentDir, { recursive: true, force: true }); + }); + + it('expands includes, rewrites tool placeholders, and expands template variables in SKILL.md', async () => { + await writeSkill({ + 'SKILL.md': + '# Demo\n\n\n\nUse the {tool:Read} tool at `{harness_home_dir}/x`.\n', + '_partials/frag.md': 'Shared fragment.\n', + }); + + const entries = await renderSkillDirectory(skillDir, 'demo', context()); + + const content = markdownContent(entries, 'SKILL.md'); + expect(content).toContain('Shared fragment.'); + expect(content).toContain('Use the open_files tool'); + expect(content).toContain('~/.claude/x'); + expect(content).not.toContain('{tool:Read}'); + expect(content).not.toContain('include:'); + // The partial is an include target, never a deployed entry. + expect(entries.some((entry) => entry.relPath.startsWith('_partials'))).toBe(false); + }); + + it('rewrites a bare-relative link in a nested .md against the skill slug and prefix', async () => { + await writeSkill({ 'SKILL.md': '# Demo\n', 'reference/guide.md': 'See [the data](../data/table.csv).\n' }); + + const entries = await renderSkillDirectory(skillDir, 'demo', context()); + + expect(markdownContent(entries, 'reference/guide.md')).toContain( + '[the data](~/.claude/skills/demo/data/table.csv)', + ); + }); + + it('returns non-.md files as assets pointing at the source path', async () => { + await writeSkill({ 'SKILL.md': '# Demo\n', 'data/table.csv': 'a,b\n1,2\n' }); + + const entries = await renderSkillDirectory(skillDir, 'demo', context()); + + expect(entries.find((entry) => entry.relPath === 'data/table.csv')).toEqual({ + kind: 'asset', + relPath: 'data/table.csv', + srcPath: path.join(skillDir, 'data', 'table.csv'), + }); + }); + + it('throws a file/line-anchored error for an unmapped tool placeholder', async () => { + await writeSkill({ 'SKILL.md': '# Demo\n\nUse {tool:Bash}.\n' }); + + await expect(renderSkillDirectory(skillDir, 'demo', context())).rejects.toThrow(ToolNameRewriteError); + await expect(renderSkillDirectory(skillDir, 'demo', context())).rejects.toThrow(/skills\/demo\/SKILL\.md:3/); + }); + + it('throws on a broken include directive', async () => { + await writeSkill({ 'SKILL.md': '# Demo\n\n\n' }); + + await expect(renderSkillDirectory(skillDir, 'demo', context())).rejects.toThrow(DirectiveExpansionError); + }); + + // region | Helpers + + function context(overrides: Partial = {}): SkillDeployContext { + return { + contentDir, + toolMapping: TOOL_MAPPING, + pathPrefix: '.claude/skills', + homeDir: '.claude', + harnessId: 'claude', + ...overrides, + }; + } + + /** Writes files into the demo skill directory from a relative-path → content map. */ + async function writeSkill(files: Record): Promise { + for (const [rel, content] of Object.entries(files)) { + const full = path.join(skillDir, rel); + await mkdir(path.dirname(full), { recursive: true }); + await writeFile(full, content, 'utf8'); + } + } + + /** Returns the transformed content of the markdown entry at relPath, failing if it is absent or an asset. */ + function markdownContent(entries: ReadonlyArray, relPath: string): string { + const entry = entries.find((candidate) => candidate.relPath === relPath); + if (entry?.kind !== 'markdown') { + throw new Error(`Expected a markdown entry at ${relPath}, got ${entry?.kind ?? 'nothing'}`); + } + return entry.content; + } + + // endregion | Helpers +}); diff --git a/packages/agents/src/lib/__tests__/subagent-transform.test.ts b/packages/agents/src/lib/__tests__/subagent-transform.test.ts index ab94942f..207ff7aa 100644 --- a/packages/agents/src/lib/__tests__/subagent-transform.test.ts +++ b/packages/agents/src/lib/__tests__/subagent-transform.test.ts @@ -1,14 +1,9 @@ -import { mkdir, rm, writeFile } from 'node:fs/promises'; -import { tmpdir } from 'node:os'; -import path from 'node:path'; - import { unindent } from '@williamthorsen/toolbelt.strings/candidate'; -import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { describe, expect, it } from 'vitest'; import { mergeFrontmatter } from '../frontmatter-merger.ts'; -import { HARNESSES } from '../harness.ts'; import { rewriteMarkdownPaths, rewriteTemplateVariables } from '../path-rewriter.ts'; -import { loadSubagentOverlay, renderSubagentForHarness } from '../subagent-transform.ts'; +import { renderSubagentForHarness } from '../subagent-transform.ts'; import { loadToolMapping, rewriteToolNames, ToolNameRewriteError } from '../tool-name-rewriter.ts'; const SOURCE = unindent` @@ -42,29 +37,6 @@ const ROVODEV_OVERLAY = unindent` `; -describe(loadSubagentOverlay, () => { - let contentDir: string; - - beforeEach(async () => { - contentDir = path.join(tmpdir(), `agents-test-overlay-${Date.now()}-${Math.random().toString(36).slice(2)}`); - await mkdir(path.join(contentDir, 'subagents', '_data'), { recursive: true }); - }); - - afterEach(async () => { - await rm(contentDir, { recursive: true, force: true }); - }); - - it('reads the harness overlay file from subagents/_data', async () => { - await writeFile(path.join(contentDir, 'subagents', '_data', 'claude.yaml'), CLAUDE_OVERLAY, 'utf8'); - - expect(await loadSubagentOverlay(contentDir, HARNESSES.claude)).toBe(CLAUDE_OVERLAY); - }); - - it('returns an empty string when the overlay file is absent', async () => { - expect(await loadSubagentOverlay(contentDir, HARNESSES.rovodev)).toBe(''); - }); -}); - describe(renderSubagentForHarness, () => { it('merges _defaults, rewrites the tool placeholder, and expands {harness_home_dir} for claude', () => { const output = renderSubagentForHarness(SOURCE, { diff --git a/packages/agents/src/lib/harness-overlay.ts b/packages/agents/src/lib/harness-overlay.ts new file mode 100644 index 00000000..cf3cf6b8 --- /dev/null +++ b/packages/agents/src/lib/harness-overlay.ts @@ -0,0 +1,22 @@ +import { readFile } from 'node:fs/promises'; +import path from 'node:path'; + +import { isEnoent } from './type-guards.ts'; +import type { HarnessConfig } from './types.ts'; + +/** + * Reads a harness's overlay YAML, returning an empty string when the file is absent. Though it lives under + * `subagents/_data/`, the overlay is harness-level: it carries both the subagent frontmatter `_defaults`/per-agent merge + * and the shared `{tool:NAME}` mapping. + */ +export async function loadHarnessOverlay(contentDir: string, harnessConfig: HarnessConfig): Promise { + const overlayPath = path.join(contentDir, 'subagents', '_data', harnessConfig.frontmatterFile); + try { + return await readFile(overlayPath, 'utf8'); + } catch (error: unknown) { + if (!isEnoent(error)) { + throw error; + } + return ''; + } +} diff --git a/packages/agents/src/lib/skill-deploy.ts b/packages/agents/src/lib/skill-deploy.ts index 2485e0a7..577262a9 100644 --- a/packages/agents/src/lib/skill-deploy.ts +++ b/packages/agents/src/lib/skill-deploy.ts @@ -4,6 +4,7 @@ import path from 'node:path'; import { makeArtifactMarker } from './artifact-marker.ts'; import { readDeploy } from './deploy-frontmatter.ts'; import { writeIfChanged } from './fs-helpers.ts'; +import { renderSkillDirectory, type SkillDeployContext } from './skill-transform.ts'; import { isEnoent, isMissingFile } from './type-guards.ts'; const skillMarker = makeArtifactMarker('skill'); @@ -46,26 +47,28 @@ export async function resolveDeclaredSkill(slug: string, librarySkillsDir: strin } /** - * Materializes a resolved skill into `destDir/`, mirroring its library directory verbatim and stamping the - * declared-skill ownership marker into the deployed root `SKILL.md`. - * The mirror is byte-stable: Unchanged files are left untouched and destination files the source no longer carries - * are removed) — so re-running sync on an unchanged skill makes no filesystem changes. - * Verbatim only: no include expansion or path rewriting (deferred to the first skill that needs it). + * Materializes a resolved skill into `destDir/` for one harness: every `.md` file is include-expanded and + * tool-name/link/template-rewritten through the shared skill transform, non-`.md` files are mirrored verbatim, and the + * declared-skill ownership marker is stamped into the deployed root `SKILL.md`. + * The write is byte-stable: unchanged files are left untouched, and destination files the source no longer carries — + * along with any directory left empty by their removal — are pruned, so re-deploying an unchanged skill makes no + * filesystem change. */ -export async function deploySkill(skill: ResolvedSkill, destDir: string): Promise { +export async function deploySkill(skill: ResolvedSkill, destDir: string, context: SkillDeployContext): Promise { + const entries = await renderSkillDirectory(skill.srcDir, skill.slug, context); await mkdir(destDir, { recursive: true }); - const srcEntries = await readdir(skill.srcDir, { withFileTypes: true }); - await removeOrphans(destDir, srcEntries); - for (const entry of srcEntries) { - const srcPath = path.join(skill.srcDir, entry.name); - const destPath = path.join(destDir, entry.name); - if (entry.isDirectory()) { - await copySubtree(srcPath, destPath); - } else if (entry.name === 'SKILL.md') { - await writeIfChanged(destPath, skillMarker.injectMarker(await readFile(srcPath, 'utf8'), skill.slug)); + const expectedFiles = new Set(entries.map((entry) => entry.relPath)); + await pruneOrphans(destDir, '', expectedFiles); + + for (const entry of entries) { + const destPath = path.join(destDir, entry.relPath); + await mkdir(path.dirname(destPath), { recursive: true }); + if (entry.kind === 'markdown') { + const body = entry.relPath === 'SKILL.md' ? skillMarker.injectMarker(entry.content, skill.slug) : entry.content; + await writeIfChanged(destPath, body); } else { - await copyFileIfChanged(srcPath, destPath); + await copyFileIfChanged(entry.srcPath, destPath); } } } @@ -83,17 +86,21 @@ async function copyFileIfChanged(srcPath: string, destPath: string): Promise { - await mkdir(destDir, { recursive: true }); - const srcEntries = await readdir(srcDir, { withFileTypes: true }); - await removeOrphans(destDir, srcEntries); - - for (const entry of srcEntries) { - const srcPath = path.join(srcDir, entry.name); - const destPath = path.join(destDir, entry.name); - await (entry.isDirectory() ? copySubtree(srcPath, destPath) : copyFileIfChanged(srcPath, destPath)); + * Removes every destination file absent from `expectedFiles`, then any directory left empty by those removals, so a + * skill's dropped files — and the directories that held them — do not linger across re-deploys. + */ +async function pruneOrphans(destDir: string, relDir: string, expectedFiles: ReadonlySet): Promise { + for (const entry of await readdir(path.join(destDir, relDir), { withFileTypes: true })) { + const rel = relDir === '' ? entry.name : `${relDir}/${entry.name}`; + const absPath = path.join(destDir, rel); + if (entry.isDirectory()) { + await pruneOrphans(destDir, rel, expectedFiles); + if ((await readdir(absPath)).length === 0) { + await rm(absPath, { recursive: true, force: true }); + } + } else if (!expectedFiles.has(rel)) { + await rm(absPath, { force: true }); + } } } @@ -109,14 +116,4 @@ async function readFileOrUndefined(filePath: string): Promise): Promise { - const srcNames = new Set(srcEntries.map((entry) => entry.name)); - for (const destEntry of await readdir(destDir, { withFileTypes: true })) { - if (!srcNames.has(destEntry.name)) { - await rm(path.join(destDir, destEntry.name), { recursive: true, force: true }); - } - } -} - // endregion | Helpers diff --git a/packages/agents/src/lib/skill-transform.ts b/packages/agents/src/lib/skill-transform.ts new file mode 100644 index 00000000..5d9ab2ce --- /dev/null +++ b/packages/agents/src/lib/skill-transform.ts @@ -0,0 +1,91 @@ +import { readdir } from 'node:fs/promises'; +import path from 'node:path'; + +import { expandIncludes } from './directive-expander.ts'; +import { rewriteMarkdownPaths, rewriteTemplateVariables } from './path-rewriter.ts'; +import { rewriteToolNames } from './tool-name-rewriter.ts'; + +/** The per-harness inputs a declared-skill render depends on, resolved once per harness by the caller. */ +export interface SkillDeployContext { + /** Library content root, used to resolve include directives and to label unmapped-tool errors. */ + readonly contentDir: string; + /** Canonical → harness tool-name mapping for the `{tool:NAME}` body-text rewriter. */ + readonly toolMapping: ReadonlyMap; + /** Harness-relative prefix under which `~/`-prefixed Markdown link targets are built (e.g. `.claude/skills`). */ + readonly pathPrefix: string; + /** Harness home segment that `{harness_home_dir}` tokens expand to (e.g. `.claude`). */ + readonly homeDir: string; + /** Harness identifier that `{harness_id}` tokens expand to (e.g. `claude`). */ + readonly harnessId: string; +} + +/** + * One file of a rendered skill directory, keyed by its POSIX path relative to the skill's destination root. A `markdown` + * entry carries its fully transformed text; an `asset` entry carries its source path for the caller to copy verbatim. + */ +export type RenderedSkillEntry = + | { readonly kind: 'markdown'; readonly relPath: string; readonly content: string } + | { readonly kind: 'asset'; readonly relPath: string; readonly srcPath: string }; + +/** + * Renders a declared skill's directory for one harness: Every `.md` file is include-expanded, then tool-name-rewritten, + * then link/template-rewritten; non-`.md` files are returned as assets to copy verbatim. + * Read-only; the caller composes its own write strategy and markers around the transform. + * Throws (with a file:line anchor) on a broken include or an unmapped `{tool:NAME}` placeholder. + * + * `slug` anchors link rewriting: a relative Markdown link resolves against `/` under `pathPrefix`, matching + * how the deployed skill sits at `//`. `_partials/` directories and dotfiles are skipped at every + * depth — partials are include targets, never deployed artifacts. + */ +export async function renderSkillDirectory( + srcDir: string, + slug: string, + context: SkillDeployContext, +): Promise> { + const entries: Array = []; + await collectEntries(srcDir, '', slug, context, entries); + return entries; +} + +// region | Helpers + +/** Recursively walks `dir`, skipping `_partials/` and dotfiles, accumulating rendered entries keyed by relative path. */ +async function collectEntries( + dir: string, + relDir: string, + slug: string, + context: SkillDeployContext, + out: Array, +): Promise { + for (const entry of await readdir(dir, { withFileTypes: true })) { + if (entry.name === '_partials' || entry.name.startsWith('.')) { + continue; + } + const srcPath = path.join(dir, entry.name); + const relPath = relDir === '' ? entry.name : `${relDir}/${entry.name}`; + if (entry.isDirectory()) { + await collectEntries(srcPath, relPath, slug, context, out); + } else if (entry.name.endsWith('.md')) { + out.push({ kind: 'markdown', relPath, content: await renderMarkdown(srcPath, relPath, slug, context) }); + } else { + out.push({ kind: 'asset', relPath, srcPath }); + } + } +} + +/** Applies the skill `.md` transform chain: include expansion, tool-name rewrite, link rewrite, template expansion. */ +async function renderMarkdown( + srcPath: string, + relPath: string, + slug: string, + context: SkillDeployContext, +): Promise { + const { contentDir, toolMapping, pathPrefix, homeDir, harnessId } = context; + const contextLabel = path.relative(contentDir, srcPath).split(path.sep).join('/'); + const expanded = await expandIncludes(srcPath, contentDir); + const toolRewritten = rewriteToolNames(expanded, toolMapping, contextLabel); + const pathRewritten = rewriteMarkdownPaths(toolRewritten, `${slug}/${relPath}`, pathPrefix); + return rewriteTemplateVariables(pathRewritten, homeDir, harnessId); +} + +// endregion | Helpers diff --git a/packages/agents/src/lib/subagent-transform.ts b/packages/agents/src/lib/subagent-transform.ts index f5570da8..0adb019e 100644 --- a/packages/agents/src/lib/subagent-transform.ts +++ b/packages/agents/src/lib/subagent-transform.ts @@ -1,27 +1,6 @@ -import { readFile } from 'node:fs/promises'; -import path from 'node:path'; - import { mergeFrontmatter } from './frontmatter-merger.ts'; import { rewriteMarkdownPaths, rewriteTemplateVariables } from './path-rewriter.ts'; import { rewriteToolNames } from './tool-name-rewriter.ts'; -import { isEnoent } from './type-guards.ts'; -import type { HarnessConfig } from './types.ts'; - -/** - * Reads the subagent overlay YAML for a harness, returning an empty string when the file does not exist. - * Shared by `install` and `sync` so both apply the same `_defaults`/per-agent merge and `{tool:NAME}` mapping. - */ -export async function loadSubagentOverlay(contentDir: string, harnessConfig: HarnessConfig): Promise { - const overlayPath = path.join(contentDir, 'subagents', '_data', harnessConfig.frontmatterFile); - try { - return await readFile(overlayPath, 'utf8'); - } catch (error: unknown) { - if (!isEnoent(error)) { - throw error; - } - return ''; - } -} /** The harness-specific inputs a subagent render depends on, resolved once per harness by the caller. */ export interface SubagentRenderContext {