#892 Apply skill transforms when sync deploys declared skills

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<!-- include: _partials/frag.md / -->\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', () => {

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<string, string>,
   label = '',
 ): Promise<ManifestEntry> {
-  // 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<string, string> | undefined;
+  let renderedDir: ReadonlyArray<RenderedSkillEntry> | 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,22 +344,17 @@ 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
     // first-time install never wipes a coincidentally same-named directory the user already had.
     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<Map<string, string>> {
-  const expandedBySrcPath = new Map<string, string>();
-  await collectExpansions(srcDir, contentDir, expandedBySrcPath);
-  return expandedBySrcPath;
-}
-
-async function collectExpansions(
-  srcDir: string,
-  contentDir: string,
-  expandedBySrcPath: Map<string, string>,
-): Promise<void> {
-  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<string, string>,
-): Promise<void> {
-  await mkdir(destDir, { recursive: true });
-  const entries = await readdir(srcDir);
+async function writeRenderedSkillDir(destDir: string, entries: ReadonlyArray<RenderedSkillEntry>): Promise<void> {
   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<string, string>,
-  contentDir: string,
-  toolMapping: ReadonlyMap<string, string>,
-): Map<string, string> {
-  const rewritten = new Map<string, string>();
-  for (const [absSrcPath, content] of rawExpanded) {
-    const label = relativeFromContent(contentDir, absSrcPath);
-    rewritten.set(absSrcPath, rewriteToolNames(content, toolMapping, label));
-  }
-  return rewritten;
-}

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 `<skillsDir>/<slug>/`.
-  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 `<subagentsDir>/<slug>.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<HarnessSkillTarget>,
+  orphansByDir: ReadonlyArray<{ skillsDir: string; orphans: ReadonlyArray<string> }>,
+  resolvedSkills: ReadonlyArray<ResolvedSkill>,
+): Promise<void> {
+  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<HarnessSkillTarget>,
+  resolvedSkills: ReadonlyArray<ResolvedSkill>,
+): Promise<void> {
+  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<HarnessSubagentTarget> {
   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<HarnessSkillTarget> {
+  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