diff --git a/packages/agents/content/guidance/rulebooks/codeassembly-content-specification.md b/packages/agents/content/guidance/rulebooks/codeassembly-content-specification.md index d43aaf9d..6991bdb7 100644 --- a/packages/agents/content/guidance/rulebooks/codeassembly-content-specification.md +++ b/packages/agents/content/guidance/rulebooks/codeassembly-content-specification.md @@ -2,7 +2,7 @@ slug: codeassembly-content-specification description: The declaration contract for CodeAssembly skills, subagents, rulebooks, and collections -- frontmatter fields, dependency blocks, and invocation tokens. delivery: skill -version: 4 +version: 5 --- # CodeAssembly content specification @@ -13,7 +13,7 @@ The declaration contract for CodeAssembly artifacts -- skills, subagents, rulebo Every rule below belongs to one of three classes, marked where it appears. -**Validated on parse.** A malformed `slug` or `skill-name`, a `delivery` value outside `ambient`/`skill`, an unknown artifact-type key, a non-list value under one, and a `members:` block on anything but a collection each fail the run with an error naming the source file. Two more fail outside the parser: a token naming an artifact that does not exist fails the run with an error naming the slug and the directories searched, and a harness that declares no sigil is a type error at its `HarnessConfig` literal, so the build fails. +**Validated on parse.** A malformed `slug` or `skill-name`, a `delivery` value outside `ambient`/`skill`, an unknown artifact-type key, a non-list value under one, and a `members:` block on anything but a collection each fail the run with an error naming the source file. Three more fail outside the parser: a token naming an artifact that does not exist fails the run with an error naming the slug and the directories searched, a rulebook link target outside a linkable root fails the run before anything is written, and a harness that declares no sigil is a type error at its `HarnessConfig` literal, so the build fails. **Enforced by test.** The suites in `packages/agents/src/__tests__/` read the shipped library and assert its conventions hold. A rule one of them guards names its test. @@ -44,7 +44,17 @@ Slugs are kebab-case and letter-led (`[a-z][a-z0-9-]*`). The sigils are a typed A token is also a dependency edge: `sync` extracts the tokens from a skill's or subagent's include-expanded body and pulls each target into the deploy closure. An inline invocation is therefore expressed once, as the token -- it needs no duplicate `dependencies:` entry, and a token naming a non-existent artifact fails the run just as a missing `dependencies:` edge does. Because extraction runs on the include-expanded body, a token inside a shared `_partials` file becomes an edge for every skill that includes it. -Tokens are honored only in skills and subagents -- the types whose bodies pass through the render pass. Rulebooks (embedded without that pass) and collections keep `dependencies:` / `members:`. Reserve a `dependencies:` entry for a non-inline edge; use a token for any invocation that appears in the body. _(Convention; not enforced.)_ +Tokens are honored only in skills and subagents. A rulebook body does pass through a render pass, but a narrower one: it receives link and template rewriting (see [Links in rulebook bodies](#links-in-rulebook-bodies)) and not token rewriting, which is why rulebooks keep `dependencies:`, as collections keep `members:`. Reserve a `dependencies:` entry for a non-inline edge; use a token for any invocation that appears in the body. _(Convention; not enforced.)_ + +## Links in rulebook bodies + +A rulebook addresses a file by linking to it, not by naming it in prose. Author the target relative to the rulebook's own place in the content tree, which is `guidance/rulebooks/.md`, and `sync` emits the absolute path each target harness can follow. A target of `../../skills/_data/concision.md` reaches Claude as `~/.claude/skills/_data/concision.md` and Rovo as `~/.rovodev/skills/_data/concision.md`. `{harness_home_dir}` and `{harness_id}` expand per harness, including where one opens a link target. + +A rulebook may link only into `skills/` and `scripts/`, the two trees whose source layout matches where they deploy under every harness home. Any other target fails the run, with an error naming the rulebook, the target as authored, and whether it resolved outside a linkable root or escaped the content root. `subagents/` is rejected because a subagent is dispatched rather than read, so no link into one is worth authoring; `_partials/`, `collections/`, and `guidance/` never deploy as files, so a link into one would name nothing. Nor can a rulebook link to the skill another rulebook delivers: that `SKILL.md` is generated rather than authored, and `dependencies:` already expresses the relationship. _(Validated on parse.)_ + +A target that is rooted correctly but names a file that has moved or been deleted is caught separately, by `content-link-resolution.test.ts`, which also resolves every anchor fragment to exactly one heading. _(Enforced by test.)_ + +One limitation is worth knowing before writing a rulebook that documents linking: rewriting runs over the whole body, so a Markdown link inside a code fence or an inline code span is rewritten along with the rest. A rulebook cannot show a relative link verbatim as an example, and must describe the target instead. ## Collections diff --git a/packages/agents/src/__tests__/content-link-resolution.test.ts b/packages/agents/src/__tests__/content-link-resolution.test.ts index 66fc08a0..10622904 100644 --- a/packages/agents/src/__tests__/content-link-resolution.test.ts +++ b/packages/agents/src/__tests__/content-link-resolution.test.ts @@ -1,10 +1,13 @@ import { existsSync } from 'node:fs'; -import { readdir } from 'node:fs/promises'; +import { readdir, readFile } from 'node:fs/promises'; import path from 'node:path'; import { beforeAll, describe, expect, it } from 'vitest'; import { expandIncludes } from '../lib/directive-expander.ts'; +import { isRewritableLinkTarget, MARKDOWN_LINK_REGEX } from '../lib/path-rewriter.ts'; +import { parseRulebookFile } from '../lib/rulebook-schema.ts'; +import { renderRulebookBody } from '../lib/rulebook-transform.ts'; // A Markdown link in installable content is rewritten at install time by `rewriteMarkdownPaths`, which resolves a // relative target against the *host* file's directory — the skill or subagent the link renders into, not the partial @@ -18,16 +21,26 @@ import { expandIncludes } from '../lib/directive-expander.ts'; // // Host roots only. A `_partials/` file is never installed standalone, and its links are authored against the host that // inlines it — checking one in isolation would misresolve every `../` it carries. Include expansion below reaches them -// through each host, which is the only context where they mean anything. `guidance/` is copied verbatim with no link -// rewriting, so it is out of scope. -const HOST_ROOTS: ReadonlyArray = ['skills', 'subagents']; +// through each host, which is the only context where they mean anything. +// +// `guidance/rulebooks/` is a host root because `sync` renders a rulebook body per harness and resolves its links the +// same way this test does: against the file's own place in the content tree. The rest of `guidance/` stays out of +// scope for the opposite reason. `_harnesses/` files are rewritten at install time, but anchored at the harness home +// they install into rather than at their source directory, so resolving one here against the source tree would +// misreport every link it carries. `shared/` installs verbatim to a harness-neutral location, which no rewritten path +// could name a harness in. +// +// A rulebook carries a second requirement the file-existence check cannot express: its target must be rooted in a tree +// that deploys under a harness home. A link to `subagents/canary.md` names a file that exists, so it satisfies +// everything above, and still fails every `sync`. The last suite below closes that gap over shipped rulebooks. +const RULEBOOK_ROOT = 'guidance/rulebooks'; + +const HOST_ROOTS: ReadonlyArray = [RULEBOOK_ROOT, 'skills', 'subagents']; const CONTENT_ROOT = new URL('../../content/', import.meta.url).pathname; const HEADING_REGEX = /^#{1,6}\s+(.+?)\s*$/gm; -const MARKDOWN_LINK_REGEX = /\[[^\]]*\]\(([^)]+)\)/g; - type Reason = 'ambiguous-anchor' | 'dead-anchor' | 'missing-file'; interface Violation { @@ -76,8 +89,10 @@ async function findViolations(): Promise> { const file = path.relative(CONTENT_ROOT, hostFile); for (const match of body.matchAll(MARKDOWN_LINK_REGEX)) { - const target = match[1]; - if (target === undefined || !isRelativeTarget(target)) { + // The rewriter's own set, plus anchor-only targets: those name no file to rewrite, but they do name a fragment + // this test resolves against the host's own headings. + const target = match[2]; + if (target === undefined || !(isRewritableLinkTarget(target) || target.startsWith('#'))) { continue; } @@ -123,11 +138,6 @@ function formatViolations(violations: ReadonlyArray): string { return [header, ...lines].join('\n'); } -/** Reports whether a link target is one the install pipeline resolves — a relative path, an anchor, or both. */ -function isRelativeTarget(target: string): boolean { - return !(/^https?:\/\//.test(target) || target.startsWith('/') || target.startsWith('~') || target.startsWith('{')); -} - async function readHeadingSlugs( file: string, cache: Map>, @@ -191,3 +201,31 @@ describe('installable-content link resolution', () => { expect(anchors, formatViolations(anchors)).toEqual([]); }); }); + +describe('shipped rulebook link deliverability', () => { + it('every rulebook link target is rooted in a tree that deploys under a harness home', async () => { + const rejections = await findRulebookRejections(); + expect(rejections, rejections.join('\n')).toEqual([]); + }); +}); + +/** + * Renders every shipped rulebook the way `sync` does, collecting the error from each that names an undeliverable link + * target. The root allowlist is lexical and harness-invariant, so one harness context stands for all of them. + */ +async function findRulebookRejections(): Promise> { + const rulebookFiles: Array = []; + await collectHostFiles(path.join(CONTENT_ROOT, RULEBOOK_ROOT), rulebookFiles); + + const rejections: Array = []; + for (const file of rulebookFiles) { + const slug = path.basename(file, '.md'); + const { body } = parseRulebookFile(await readFile(file, 'utf8'), `${slug}.md`); + try { + renderRulebookBody(body, slug, { homeDir: '.claude', harnessId: 'claude' }); + } catch (error) { + rejections.push(error instanceof Error ? error.message : String(error)); + } + } + return rejections; +} diff --git a/packages/agents/src/commands/__tests__/sync.test.ts b/packages/agents/src/commands/__tests__/sync.test.ts index de5407d0..1535db7f 100644 --- a/packages/agents/src/commands/__tests__/sync.test.ts +++ b/packages/agents/src/commands/__tests__/sync.test.ts @@ -1155,6 +1155,95 @@ describe(syncCommand, () => { }); }); + describe('rulebook body rendering', () => { + /** Creates both harness home dirs so `harness: 'all'` detects claude and rovodev. */ + async function detectBothHarnesses(): Promise { + await mkdir(path.join(projectRoot, '.claude'), { recursive: true }); + await mkdir(path.join(projectRoot, '.rovodev'), { recursive: true }); + } + + it('rewrites a relative link into the target harness absolute path in skill delivery', async () => { + await writeLibraryRulebook('alpha', 'delivery: skill', 'See [concision](../../skills/_data/concision.md).'); + await declareRulebooks('alpha'); + + await syncCommand(makeOptions(), projectRoot, contentDir); + + expect(await readFile(skillPath('consult-alpha'), 'utf8')).toContain( + 'See [concision](~/.claude/skills/_data/concision.md).', + ); + }); + + it('preserves an anchor fragment on a rewritten target', async () => { + await writeLibraryRulebook('alpha', 'delivery: skill', 'See [block](../../skills/_data/action-items.md#block).'); + await declareRulebooks('alpha'); + + await syncCommand(makeOptions(), projectRoot, contentDir); + + expect(await readFile(skillPath('consult-alpha'), 'utf8')).toContain( + '(~/.claude/skills/_data/action-items.md#block)', + ); + }); + + it('expands harness template variables in the delivered body', async () => { + await writeLibraryRulebook('alpha', 'delivery: skill', 'Run {harness_home_dir}/scripts/x.sh as {harness_id}.'); + await declareRulebooks('alpha'); + + await syncCommand(makeOptions(), projectRoot, contentDir); + + expect(await readFile(skillPath('consult-alpha'), 'utf8')).toContain('Run ~/.claude/scripts/x.sh as claude.'); + }); + + it('gives each harness its own absolute path, in both skill and ambient delivery', async () => { + await detectBothHarnesses(); + await writeLibraryRulebook( + 'alpha', + 'delivery: [ambient, skill]', + 'See [concision](../../skills/_data/concision.md).', + ); + await declareRulebooks('alpha'); + + await syncCommand(makeOptions({ harness: 'all' }), projectRoot, contentDir); + + expect(await readFile(skillPath('consult-alpha', '.claude'), 'utf8')).toContain( + '~/.claude/skills/_data/concision.md', + ); + expect(await readFile(skillPath('consult-alpha', '.rovodev'), 'utf8')).toContain( + '~/.rovodev/skills/_data/concision.md', + ); + expect(await readFile(localHostPath('CLAUDE.local.md'), 'utf8')).toContain('~/.claude/skills/_data/concision.md'); + expect(await readFile(localHostPath('AGENTS.local.md'), 'utf8')).toContain( + '~/.rovodev/skills/_data/concision.md', + ); + }); + + it('fails the run when a link target is not under a linkable root, naming the rulebook and the target', async () => { + await writeLibraryRulebook('alpha', 'delivery: skill', 'See [canary](../../subagents/canary.md).'); + await declareRulebooks('alpha'); + + await expect(syncCommand(makeOptions(), projectRoot, contentDir)).rejects.toThrow( + /alpha[\s\S]*subagents\/canary\.md/, + ); + }); + + it('fails the run when a link target escapes the content root', async () => { + await writeLibraryRulebook('alpha', 'delivery: ambient', 'See [x](../../../elsewhere/a.md).'); + await declareRulebooks('alpha'); + + await expect(syncCommand(makeOptions(), projectRoot, contentDir)).rejects.toThrow(/escapes the content root/); + }); + + it('fails a dry run on a bad link target, writing nothing', async () => { + await writeLibraryRulebook('alpha', 'delivery: [ambient, skill]', 'See [canary](../../subagents/canary.md).'); + await declareRulebooks('alpha'); + + await expect(syncCommand(makeOptions({ dryRun: true }), projectRoot, contentDir)).rejects.toThrow( + /unusable Markdown link target/, + ); + expect(existsSync(skillPath('consult-alpha'))).toBe(false); + expect(existsSync(localHostPath())).toBe(false); + }); + }); + describe('declared subagents', () => { const CLAUDE_OVERLAY = unindent` _tools: @@ -1164,7 +1253,7 @@ describe(syncCommand, () => { permissionMode: bypassPermissions `; - const ROVODEV_OVERLAY = unindent` + const ROVO_OVERLAY = unindent` _tools: Read: open_files @@ -1181,7 +1270,7 @@ describe(syncCommand, () => { const dataDir = path.join(contentDir, 'subagents', '_data'); await mkdir(dataDir, { recursive: true }); await writeFile(path.join(dataDir, 'claude.yaml'), CLAUDE_OVERLAY, 'utf8'); - await writeFile(path.join(dataDir, 'rovodev.yaml'), ROVODEV_OVERLAY, 'utf8'); + await writeFile(path.join(dataDir, 'rovodev.yaml'), ROVO_OVERLAY, 'utf8'); } /** Writes a fixture subagent `.md` into the temp content library's `subagents/`. */ diff --git a/packages/agents/src/commands/sync.ts b/packages/agents/src/commands/sync.ts index c8937c71..c1da5ec7 100644 --- a/packages/agents/src/commands/sync.ts +++ b/packages/agents/src/commands/sync.ts @@ -6,15 +6,10 @@ import process from 'node:process'; import { appendAmbientRegion, classifyAmbientRegion, injectAmbientRegion } from '../lib/ambient-region.ts'; import { makeArtifactMarker } from '../lib/artifact-marker.ts'; -import { ARTIFACT_TYPE_VALUES, artifactFrontmatterPath, type ArtifactType } from '../lib/artifact-types.ts'; +import { ARTIFACT_TYPE_VALUES, type ArtifactType } from '../lib/artifact-types.ts'; import { resolveDeclaration } from '../lib/codeassembly-manifest.ts'; import { resolveContentDir } from '../lib/content-resolver.ts'; -import { - createSourceResolver, - describeSearchedLocations, - hasLibraryArtifact, - type SourceResolver, -} from '../lib/content-sources.ts'; +import { createSourceResolver, hasLibraryArtifact, type SourceResolver } from '../lib/content-sources.ts'; import { type DirectArtifacts, resolveClosure } from '../lib/dependency-resolver.ts'; import { readDirEntries, readFileOrEmpty, writeIfChanged } from '../lib/fs-helpers.ts'; import { checkGitIgnored } from '../lib/git-ignore.ts'; @@ -24,8 +19,9 @@ import { enumerateCatalogSlugs } from '../lib/library-catalog.ts'; import { findUndeclaredGuidancePackages, resolvePackageSources } from '../lib/package-sources.ts'; import { collectPromptEntries, renderPromptEntries } from '../lib/prompts-yml.ts'; import { hasPromptsRegion, injectPromptsRegion, removePromptsRegion } from '../lib/prompts-yml-region.ts'; -import { parseRulebookFile } from '../lib/rulebook-schema.ts'; -import { extractRulebookSkillSlug, renderSkillFile, resolveSkillName } from '../lib/rulebook-skill.ts'; +import { type ResolvedRulebook, resolveRulebook } from '../lib/rulebook-deploy.ts'; +import { extractRulebookSkillSlug, renderSkillFile } from '../lib/rulebook-skill.ts'; +import { renderRulebookBody, type RulebookRenderContext } from '../lib/rulebook-transform.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'; @@ -42,18 +38,6 @@ import type { AmbientHostKind, HarnessId, InstallOptions } from '../lib/types.ts const skillMarker = makeArtifactMarker('skill'); const subagentMarker = makeArtifactMarker('subagent'); -/** A declared rulebook resolved against its owning source: its neutral body and which delivery modes it requests. */ -interface ResolvedRulebook { - readonly slug: string; - readonly skillName: string; - readonly body: string; - readonly ambient: boolean; - readonly skill: boolean; - readonly description: string | undefined; - /** The name of the declared source it resolved from, or `undefined` for the built-in library. */ - readonly source: string | undefined; -} - /** * One deployed artifact's resolution outcome: its type and slug, the source it resolved from (`undefined` = library), * and whether it masks a same-slug library artifact. Drives both the dry-run resolution report and the real-run shadow @@ -72,6 +56,12 @@ interface HarnessSubagentTarget { readonly deployContext: SubagentDeployContext; } +/** One targeted harness's ambient host, paired with the harness id whose paths its rulebook bodies render for. */ +interface AmbientHostTarget { + readonly harnessId: HarnessId; + readonly hostPath: string; +} + /** One targeted harness's id and project-local skills dir paired with the per-harness inputs the skill transform needs. */ interface HarnessSkillTarget { readonly harnessId: HarnessId; @@ -213,33 +203,32 @@ async function reconcileDomain( // delivery namespaces would clobber, so reject the overlap before any write. assertNoCrossNamespaceCollisions(desiredSkillDirs.values().toArray(), declaredSkillSet); + // Every delivery pass below targets this one set of harnesses, and each renders its content for the harness it + // lands on, so the set is resolved once and threaded rather than re-derived per pass. + const harnessIds = resolveHarnessIds(options.harness, domain.baseDir); + // 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. 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. + // tool-name/link rewriting, and the harness id each rulebook body renders for. const harnessSkillTargets = await Promise.all( - resolveHarnessIds(options.harness, domain.baseDir).map((harnessId) => - resolveSkillTarget(harnessId, domain.baseDir, contentDir), - ), + harnessIds.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 // transform is harness-specific, and subagents live in a distinct flat dir from skills. const declaredSubagentSet = new Set(resolvedSubagents.map((subagent) => subagent.slug)); const harnessSubagentTargets = await Promise.all( - resolveHarnessIds(options.harness, domain.baseDir).map((harnessId) => - resolveSubagentTarget(harnessId, domain.baseDir, contentDir), - ), + harnessIds.map((harnessId) => resolveSubagentTarget(harnessId, domain.baseDir, contentDir)), ); // A skill dir is sync-owned only when its `SKILL.md` carries the provenance marker; that gate is what keeps // hand-authored skills safe. An owned dir is an orphan when its marker slug no longer maps to that directory — // because the rulebook is no longer skill-delivered, or because its resolved skill name (and dir) changed. const skillOrphansByDir = await Promise.all( - harnessSkillDirs.map(async (skillsDir) => ({ + harnessSkillTargets.map(async ({ harnessId, skillsDir }) => ({ + harnessId, skillsDir, orphans: (await listOwnedSkills(skillsDir)) .filter(({ dir, slug }) => desiredSkillDirs.get(slug) !== dir) @@ -286,9 +275,14 @@ async function reconcileDomain( // here; `deploySkill` re-renders it at write time. await assertDeclaredSkillsRender(harnessSkillTargets, resolvedSkills); + // Same gate for rulebooks: a link target the delivery pipeline cannot honor fails the run before either delivery + // pass writes, rather than shipping a path that resolves to nothing. + assertRulebooksRender(harnessIds, resolved); + // Reject a sync-owned ambient host whose region is half-written before anything is written, dry-run included. // Appending beside a stray marker is the one path in this command that can destroy hand-authored content. - await assertAmbientHostsWritable(options, domain, resolved); + const ambientHosts = resolveAmbientHosts(harnessIds, domain); + await assertAmbientHostsWritable(ambientHosts, domain, resolved); // Attribute each deployed artifact to the source it resolved from, flagging any that shadows a same-slug library // artifact. Built once, off the write path, and consumed by both the dry-run report and the real-run shadow warning. @@ -297,7 +291,7 @@ async function reconcileDomain( if (options.dryRun) { await retireRetiredOutputs(options, domain); reportDryRun({ - ambientHostPreviews: await previewAmbientHosts(options, domain, resolved), + ambientHostPreviews: await previewAmbientHosts(ambientHosts, domain, resolved), resolutionReport, resolved, harnessSkillTargets, @@ -307,23 +301,24 @@ async function reconcileDomain( resolvedSubagents, harnessSubagentTargets, subagentOrphansByDir, - promptsYmlPaths: resolvePromptsYmlPaths(options, domain), + promptsYmlPaths: resolvePromptsYmlPaths(harnessIds, domain), }); return; } await retireRetiredOutputs(options, domain); - await deliverAmbient(options, domain, resolved); + await deliverAmbient(ambientHosts, domain, resolved); // Reconcile skill files per targeted harness: Retract sync-owned skill dirs that are no longer current, then // write every skill-delivery rulebook. Orphans were computed against the pre-write filesystem, so retracting // before writing lets a skill name freed by one rulebook be recreated for another in the same sync, instead // of the write being clobbered by a later retract. - for (const { skillsDir, orphans } of skillOrphansByDir) { + for (const { harnessId, skillsDir, orphans } of skillOrphansByDir) { for (const dir of orphans) { await rm(path.join(skillsDir, dir), { recursive: true, force: true }); } + const context = resolveRulebookRenderContext(harnessId); for (const rulebook of resolved) { if (!rulebook.skill) { continue; @@ -332,7 +327,12 @@ async function reconcileDomain( await mkdir(skillDir, { recursive: true }); await writeIfChanged( path.join(skillDir, 'SKILL.md'), - renderSkillFile(rulebook.skillName, rulebook.slug, rulebook.description, rulebook.body), + renderSkillFile( + rulebook.skillName, + rulebook.slug, + rulebook.description, + renderRulebookBody(rulebook.body, rulebook.slug, context), + ), ); } } @@ -346,10 +346,10 @@ async function reconcileDomain( // transform applied and the ownership marker stamped. await reconcileDeclaredSubagents(harnessSubagentTargets, subagentOrphansByDir, resolvedSubagents); - await refreshPromptsYml(options, domain); + await refreshPromptsYml(harnessIds, domain); const skillRetractions = skillOrphansByDir.reduce((total, harness) => total + harness.orphans.length, 0); - const skillFilesWritten = desiredSkillDirs.size * harnessSkillDirs.length; + const skillFilesWritten = desiredSkillDirs.size * harnessSkillTargets.length; const declaredSkillRetractions = declaredSkillOrphansByDir.reduce( (total, harness) => total + harness.orphans.length, 0, @@ -364,7 +364,7 @@ async function reconcileDomain( `Synced ${resolved.length} rulebook(s), ${resolvedSkills.length} declared skill(s), and ` + `${resolvedSubagents.length} declared subagent(s); delivered ${skillFilesWritten} rulebook-skill file(s), ` + `${declaredSkillsDeployed} declared-skill dir(s), and ${subagentsDeployed} declared-subagent file(s) across ` + - `${harnessSkillDirs.length} harness(s); retracted ` + + `${harnessSkillTargets.length} harness(s); retracted ` + `${skillRetractions} rulebook-skill dir(s), ${declaredSkillRetractions} declared-skill dir(s), and ` + `${subagentRetractions} declared-subagent file(s).`, ); @@ -752,18 +752,32 @@ async function assertDeclaredSkillsRender( } } +/** + * Renders every resolved rulebook against every targeted harness, discarding the output, so a link target the + * delivery pipeline cannot honor throws before any file is written. Both delivery passes re-render at write time; + * this pass exists only to fail the run closed, including under `--dry-run`. + */ +function assertRulebooksRender(harnessIds: ReadonlyArray, resolved: ReadonlyArray): void { + for (const harnessId of harnessIds) { + const context = resolveRulebookRenderContext(harnessId); + for (const rulebook of resolved) { + renderRulebookBody(rulebook.body, rulebook.slug, context); + } + } +} + /** * Delivers the resolved ambient rulebooks into the ambient region of each targeted harness's host, regenerating the * region's content wholesale (an empty ambient set empties an existing region). Both domains share this one path, * differing only in the host each targets and in who owns region creation there. */ async function deliverAmbient( - options: InstallOptions, + hosts: ReadonlyArray, domain: SyncDomain, resolved: ReadonlyArray, ): Promise { - const body = renderAmbientBody(resolved); - for (const hostPath of resolveAmbientHostPaths(options, domain)) { + for (const { harnessId, hostPath } of hosts) { + const body = renderAmbientBody(resolved, harnessId); const plan = planAmbientHost(domain.ambient, await probeAmbientHost(hostPath), hostPath, body); if (plan.kind === 'skip') { if (plan.warn) { @@ -851,16 +865,20 @@ function planAmbientHost( /** Pairs each targeted host with the plan a real run would carry out, so the dry-run preview cannot drift from it. */ async function previewAmbientHosts( - options: InstallOptions, + hosts: ReadonlyArray, domain: SyncDomain, resolved: ReadonlyArray, ): Promise> { - const body = renderAmbientBody(resolved); return Promise.all( - resolveAmbientHostPaths(options, domain).map(async (hostPath) => + hosts.map(async ({ harnessId, hostPath }) => describeAmbientHostPlan( hostPath, - planAmbientHost(domain.ambient, await probeAmbientHost(hostPath), hostPath, body), + planAmbientHost( + domain.ambient, + await probeAmbientHost(hostPath), + hostPath, + renderAmbientBody(resolved, harnessId), + ), ), ), ); @@ -907,15 +925,17 @@ async function probeAmbientHost(hostPath: string): Promise { * such a file with a warning instead. Runs before any write so a dry-run surfaces the conflict with nothing changed. */ async function assertAmbientHostsWritable( - options: InstallOptions, + hosts: ReadonlyArray, domain: SyncDomain, resolved: ReadonlyArray, ): Promise { - if (domain.ambient !== 'project-local' || renderAmbientBody(resolved) === '') { + // Asks whether anything would be delivered, which is a property of the declaration alone. Rendering could answer it + // too, but rendering can now fail on a bad link, and this guard is about region damage rather than link validity. + if (domain.ambient !== 'project-local' || resolved.every((rulebook) => !rulebook.ambient)) { return; } const malformed: Array = []; - for (const hostPath of resolveAmbientHostPaths(options, domain)) { + for (const { hostPath } of hosts) { if ((await probeAmbientHost(hostPath)).status === 'malformed') { malformed.push(hostPath); } @@ -1005,22 +1025,39 @@ async function retireRetiredOutputs(options: InstallOptions, domain: SyncDomain) } } -/** Renders the ambient rulebooks as concatenated sentinel blocks — the wholesale content of an ambient region. */ -function renderAmbientBody(resolved: ReadonlyArray): string { +/** + * Renders the ambient rulebooks as concatenated sentinel blocks — the wholesale content of one harness's ambient + * region. Each body is rendered for `harnessId`, so the same rulebook yields that harness's own absolute paths. + */ +function renderAmbientBody(resolved: ReadonlyArray, harnessId: HarnessId): string { + const context = resolveRulebookRenderContext(harnessId); let body = ''; for (const rulebook of resolved) { if (rulebook.ambient) { - body = injectRulebook(body, rulebook.slug, rulebook.body); + body = injectRulebook(body, rulebook.slug, renderRulebookBody(rulebook.body, rulebook.slug, context)); } } return body; } -/** Lists the guidance files whose ambient regions a sync of `domain` targets — one per targeted harness. */ -function resolveAmbientHostPaths(options: InstallOptions, domain: SyncDomain): ReadonlyArray { - return resolveHarnessIds(options.harness, domain.baseDir).map((harnessId) => - resolveAmbientHostPath(harnessId, domain.ambient, domain.baseDir), - ); +/** The per-harness inputs a rulebook render depends on, read off the harness config. */ +function resolveRulebookRenderContext(harnessId: HarnessId): RulebookRenderContext { + const config = HARNESSES[harnessId]; + return { homeDir: config.homeDir, harnessId: config.id }; +} + +/** + * Lists the guidance files whose ambient regions a sync of `domain` targets — one per targeted harness, each paired + * with the harness id its content is rendered for. + */ +function resolveAmbientHosts( + harnessIds: ReadonlyArray, + domain: SyncDomain, +): ReadonlyArray { + return harnessIds.map((harnessId) => ({ + harnessId, + hostPath: resolveAmbientHostPath(harnessId, domain.ambient, domain.baseDir), + })); } /** @@ -1030,8 +1067,8 @@ function resolveAmbientHostPaths(options: InstallOptions, domain: SyncDomain): R * no-op for non-Rovo Dev harnesses and for a file carrying no codeassembly region. Both domains share this one path, so * the home file is merged rather than whole-file overwritten, matching the repo file's non-clobbering shape. */ -async function refreshPromptsYml(options: InstallOptions, domain: SyncDomain): Promise { - for (const harnessId of resolveHarnessIds(options.harness, domain.baseDir)) { +async function refreshPromptsYml(harnessIds: ReadonlyArray, domain: SyncDomain): Promise { + for (const harnessId of harnessIds) { if (harnessId !== 'rovodev') { continue; } @@ -1056,8 +1093,8 @@ async function refreshPromptsYml(options: InstallOptions, domain: SyncDomain): P } /** Lists the Rovo Dev `prompts.yml` paths a sync of `domain` would reconcile — one per targeted Rovo Dev harness. */ -function resolvePromptsYmlPaths(options: InstallOptions, domain: SyncDomain): ReadonlyArray { - return resolveHarnessIds(options.harness, domain.baseDir) +function resolvePromptsYmlPaths(harnessIds: ReadonlyArray, domain: SyncDomain): ReadonlyArray { + return harnessIds .filter((harnessId) => harnessId === 'rovodev') .map((harnessId) => path.join(resolveHarnessPaths(harnessId, domain.baseDir).harnessHome, 'prompts.yml')); } @@ -1216,42 +1253,6 @@ function renderShadowWarning(shadows: ReadonlyArray): string { return `⚠️ ${shadows.length} artifact${plural} shadow${verb} a library slug: ${details}`; } -/** - * Reads a rulebook from its owning source (a declared source or the library, resolved through `resolver`), validates - * its frontmatter, and returns its neutral body and delivery. A missing frontmatter file throws an error naming the - * resolving source. - */ -async function resolveRulebook(slug: string, resolver: SourceResolver): Promise { - const resolved = await resolver.resolve('rulebook', slug); - if (resolved === undefined) { - const searched = describeSearchedLocations(resolver, 'rulebook', slug); - throw new Error(`Declared rulebook "${slug}" was not found in any of: ${searched}`); - } - - const srcPath = path.join(resolved.dir, artifactFrontmatterPath('rulebook', slug)); - let content: string; - try { - content = await readFile(srcPath, 'utf8'); - } catch (error: unknown) { - if (isEnoent(error)) { - const origin = resolved.source === undefined ? 'the library' : `source "${resolved.source}"`; - throw new Error(`Declared rulebook "${slug}" was not found in ${origin} at ${srcPath}`); - } - throw error; - } - - const { rulebook, body } = parseRulebookFile(content, `${slug}.md`); - return { - slug, - skillName: resolveSkillName(slug, rulebook['skill-name']), - body: `${body.trim()}\n`, - ambient: rulebook.delivery.includes('ambient'), - skill: rulebook.delivery.includes('skill'), - description: rulebook.description, - source: resolved.source, - }; -} - /** * Resolves one harness's project-local subagents dir together with the per-harness inputs the deploy transform needs: * the harness overlay YAML, its tool-name mapping, the home-dir segment, and the harness id. Passing `projectRoot` as diff --git a/packages/agents/src/lib/__tests__/invocation-tokens.test.ts b/packages/agents/src/lib/__tests__/invocation-tokens.test.ts index f262a2cd..2838827d 100644 --- a/packages/agents/src/lib/__tests__/invocation-tokens.test.ts +++ b/packages/agents/src/lib/__tests__/invocation-tokens.test.ts @@ -2,8 +2,8 @@ import { describe, expect, it } from 'vitest'; import { extractInvocationEdges, type InvocationSigils, rewriteInvocationTokens } from '../invocation-tokens.ts'; -const CLAUDE: InvocationSigils = { skillSigil: '/', subagentSigil: '' }; -const ROVODEV: InvocationSigils = { skillSigil: '!', subagentSigil: '' }; +const CLAUDE_SIGILS: InvocationSigils = { skillSigil: '/', subagentSigil: '' }; +const ROVO_SIGILS: InvocationSigils = { skillSigil: '!', subagentSigil: '' }; describe(extractInvocationEdges, () => { it('returns empty groups when no tokens are present', () => { @@ -32,17 +32,23 @@ describe(extractInvocationEdges, () => { describe(rewriteInvocationTokens, () => { it('returns content unchanged when no tokens are present', () => { const content = 'Plain prose mentioning a skill but using no token.'; - expect(rewriteInvocationTokens(content, CLAUDE)).toBe(content); + expect(rewriteInvocationTokens(content, CLAUDE_SIGILS)).toBe(content); }); it('renders a skill token as the skill sigil plus the slug', () => { - expect(rewriteInvocationTokens('Invoke {skill:capture-event} now.', CLAUDE)).toBe('Invoke /capture-event now.'); - expect(rewriteInvocationTokens('Invoke {skill:capture-event} now.', ROVODEV)).toBe('Invoke !capture-event now.'); + expect(rewriteInvocationTokens('Invoke {skill:capture-event} now.', CLAUDE_SIGILS)).toBe( + 'Invoke /capture-event now.', + ); + expect(rewriteInvocationTokens('Invoke {skill:capture-event} now.', ROVO_SIGILS)).toBe( + 'Invoke !capture-event now.', + ); }); it('renders a subagent token as the bare slug when the sigil is empty', () => { - expect(rewriteInvocationTokens('Dispatch {subagent:code-reviewer}.', CLAUDE)).toBe('Dispatch code-reviewer.'); - expect(rewriteInvocationTokens('Dispatch {subagent:code-reviewer}.', ROVODEV)).toBe('Dispatch code-reviewer.'); + expect(rewriteInvocationTokens('Dispatch {subagent:code-reviewer}.', CLAUDE_SIGILS)).toBe( + 'Dispatch code-reviewer.', + ); + expect(rewriteInvocationTokens('Dispatch {subagent:code-reviewer}.', ROVO_SIGILS)).toBe('Dispatch code-reviewer.'); }); it('renders a subagent token with a non-empty sigil', () => { @@ -51,25 +57,25 @@ describe(rewriteInvocationTokens, () => { }); it('handles hyphenated multi-segment slugs', () => { - expect(rewriteInvocationTokens('{skill:plan-orchestrable-steps}', CLAUDE)).toBe('/plan-orchestrable-steps'); + expect(rewriteInvocationTokens('{skill:plan-orchestrable-steps}', CLAUDE_SIGILS)).toBe('/plan-orchestrable-steps'); }); it('renders multiple tokens of both kinds on a single line', () => { const content = 'First {skill:plan}, then {subagent:planner}, then {skill:review-branch}.'; - expect(rewriteInvocationTokens(content, CLAUDE)).toBe('First /plan, then planner, then /review-branch.'); + expect(rewriteInvocationTokens(content, CLAUDE_SIGILS)).toBe('First /plan, then planner, then /review-branch.'); }); it('preserves adjacent prose and inline-code backticks around a token', () => { - expect(rewriteInvocationTokens('Use `{skill:commit}` here.', CLAUDE)).toBe('Use `/commit` here.'); + expect(rewriteInvocationTokens('Use `{skill:commit}` here.', CLAUDE_SIGILS)).toBe('Use `/commit` here.'); }); it('does not match a token whose slug is not letter-led', () => { const content = 'Not tokens: {skill:9lives} and {skill:-leading-hyphen}.'; - expect(rewriteInvocationTokens(content, CLAUDE)).toBe(content); + expect(rewriteInvocationTokens(content, CLAUDE_SIGILS)).toBe(content); }); it('does not match an empty slug or an unknown kind', () => { const content = 'Not tokens: {skill:} and {agent:foo} and {tool:Read}.'; - expect(rewriteInvocationTokens(content, CLAUDE)).toBe(content); + expect(rewriteInvocationTokens(content, CLAUDE_SIGILS)).toBe(content); }); }); diff --git a/packages/agents/src/lib/__tests__/path-rewriter.test.ts b/packages/agents/src/lib/__tests__/path-rewriter.test.ts index a9726286..104dcd05 100644 --- a/packages/agents/src/lib/__tests__/path-rewriter.test.ts +++ b/packages/agents/src/lib/__tests__/path-rewriter.test.ts @@ -4,7 +4,33 @@ import path from 'node:path'; import { afterEach, beforeEach, describe, expect, it } from 'vitest'; -import { rewriteMarkdownPaths, rewritePathsInDirectory, rewriteTemplateVariables } from '../path-rewriter.js'; +import { + isRewritableLinkTarget, + rewriteMarkdownPaths, + rewritePathsInDirectory, + rewriteTemplateVariables, +} from '../path-rewriter.js'; + +describe(isRewritableLinkTarget, () => { + it.each(['../_data/concision.md', './modules/review-cycle.md', 'SKILL.md', 'scripts/run.sh'])( + 'treats %s as rewritable', + (target) => { + expect(isRewritableLinkTarget(target)).toBe(true); + }, + ); + + it.each([ + 'https://example.com/docs', + // eslint-disable-next-line unicorn/prefer-https -- Test data: the passthrough set covers plain HTTP too, which is what the regex's optional `s` exists for. + 'http://example.com/docs', + '/absolute/path.md', + '~/.claude/skills/_data/concision.md', + '#anchor-only', + '{harness_home_dir}/scripts/describe-change.sh', + ])('treats %s as passthrough', (target) => { + expect(isRewritableLinkTarget(target)).toBe(false); + }); +}); describe(rewriteMarkdownPaths, () => { const skillsPrefix = '.claude/skills'; @@ -30,12 +56,6 @@ describe(rewriteMarkdownPaths, () => { content: 'Visit [docs](https://example.com/docs) for info.', expected: 'Visit [docs](https://example.com/docs) for info.', }, - { - name: 'leaves http URL links untouched', - fileRelPath: 'commit/SKILL.md', - content: 'Visit [docs](https://example.com/docs) for info.', - expected: 'Visit [docs](https://example.com/docs) for info.', - }, { name: 'leaves absolute paths untouched', fileRelPath: 'commit/SKILL.md', @@ -66,6 +86,12 @@ describe(rewriteMarkdownPaths, () => { content: 'See [review cycle](./modules/review-cycle.md) for details.', expected: 'See [review cycle](~/.claude/skills/orchestrate/modules/review-cycle.md) for details.', }, + { + name: 'leaves template-variable-prefixed targets untouched', + fileRelPath: 'commit/SKILL.md', + content: 'Run [describe-change]({harness_home_dir}/scripts/describe-change.sh) first.', + expected: 'Run [describe-change]({harness_home_dir}/scripts/describe-change.sh) first.', + }, { name: 'leaves anchor-only links untouched', fileRelPath: 'create-pr/SKILL.md', @@ -235,6 +261,21 @@ describe(rewritePathsInDirectory, () => { ); }); + it('expands a template variable inside a link target instead of resolving it as a relative path', async () => { + const skillDir = path.join(skillsDestDir, 'commit'); + await mkdir(skillDir, { recursive: true }); + await writeFile( + path.join(skillDir, 'SKILL.md'), + 'Run [describe-change]({harness_home_dir}/scripts/describe-change.sh) first.', + 'utf8', + ); + + await rewritePathsInDirectory(skillDir, skillsDestDir, '.claude/skills', '.claude', 'claude'); + + const result = await readFile(path.join(skillDir, 'SKILL.md'), 'utf8'); + expect(result).toBe('Run [describe-change](~/.claude/scripts/describe-change.sh) first.'); + }); + it('does not write files when no changes are needed', async () => { const skillDir = path.join(skillsDestDir, 'test-skill'); await mkdir(skillDir, { recursive: true }); diff --git a/packages/agents/src/lib/__tests__/rulebook-transform.test.ts b/packages/agents/src/lib/__tests__/rulebook-transform.test.ts new file mode 100644 index 00000000..dbaa07d6 --- /dev/null +++ b/packages/agents/src/lib/__tests__/rulebook-transform.test.ts @@ -0,0 +1,115 @@ +import { describe, expect, it } from 'vitest'; + +import { renderRulebookBody, type RulebookRenderContext } from '../rulebook-transform.ts'; + +const CLAUDE_CONTEXT: RulebookRenderContext = { homeDir: '.claude', harnessId: 'claude' }; +const ROVO_CONTEXT: RulebookRenderContext = { homeDir: '.rovodev', harnessId: 'rovodev' }; + +describe(renderRulebookBody, () => { + describe('link rewriting', () => { + it.each([ + { + name: 'emits an absolute path for a target under skills/', + body: 'Full principle: [concision](../../skills/_data/concision.md).', + expected: 'Full principle: [concision](~/.claude/skills/_data/concision.md).', + }, + { + name: 'emits an absolute path for a target under scripts/', + body: 'Run [describe-change](../../scripts/describe-change.sh).', + expected: 'Run [describe-change](~/.claude/scripts/describe-change.sh).', + }, + { + name: 'preserves an anchor fragment', + body: 'See [the block](../../skills/_data/action-items.md#the-block).', + expected: 'See [the block](~/.claude/skills/_data/action-items.md#the-block).', + }, + { + name: 'rewrites every target in the body', + body: 'Both [a](../../skills/_data/a.md) and [b](../../scripts/b.sh).', + expected: 'Both [a](~/.claude/skills/_data/a.md) and [b](~/.claude/scripts/b.sh).', + }, + ])('$name', ({ body, expected }) => { + expect(renderRulebookBody(body, 'shell-conventions', CLAUDE_CONTEXT)).toBe(expected); + }); + + it('anchors the rewrite at the rulebook, two levels below the content root', () => { + // A sibling tree is reached with `../../`; one level shallower lands inside `guidance/`, which never deploys. + expect(() => renderRulebookBody('[x](../skills/a.md)', 'a-rulebook', CLAUDE_CONTEXT)).toThrow( + /resolves to "guidance\/skills\/a\.md"/, + ); + }); + + it.each([ + 'Visit [docs](https://example.com/docs).', + 'See [file](/absolute/path.md).', + 'See [file](~/.claude/skills/_data/concision.md).', + 'See [section](#a-heading).', + ])('leaves a passthrough target untouched: %s', (body) => { + expect(renderRulebookBody(body, 'a-rulebook', CLAUDE_CONTEXT)).toBe(body); + }); + + it('expands a template variable that opens a link target', () => { + expect( + renderRulebookBody('Run [helper]({harness_home_dir}/scripts/helper.sh).', 'a-rulebook', CLAUDE_CONTEXT), + ).toBe('Run [helper](~/.claude/scripts/helper.sh).'); + }); + }); + + describe('template variables', () => { + it('expands {harness_home_dir} and {harness_id}', () => { + const body = 'Run {harness_home_dir}/scripts/emit.mjs --harness {harness_id}.'; + expect(renderRulebookBody(body, 'a-rulebook', CLAUDE_CONTEXT)).toBe( + 'Run ~/.claude/scripts/emit.mjs --harness claude.', + ); + }); + }); + + describe('per-harness output', () => { + it('yields each harness its own absolute path for one authored target', () => { + const body = 'Full principle: [concision](../../skills/_data/concision.md).'; + expect(renderRulebookBody(body, 'a-rulebook', CLAUDE_CONTEXT)).toContain('~/.claude/skills/_data/concision.md'); + expect(renderRulebookBody(body, 'a-rulebook', ROVO_CONTEXT)).toContain('~/.rovodev/skills/_data/concision.md'); + }); + }); + + describe('validation', () => { + it.each([ + { name: 'a target under subagents/', target: '../../subagents/canary.md' }, + { name: 'a target under _partials/', target: '../../_partials/shared.md' }, + { name: 'a target under collections/', target: '../../collections/library.md' }, + { name: 'a sibling rulebook', target: './other-rulebook.md' }, + { name: 'a target in the content root itself', target: '../../README.md' }, + ])('rejects $name', ({ target }) => { + expect(() => renderRulebookBody(`See [x](${target}).`, 'a-rulebook', CLAUDE_CONTEXT)).toThrow( + /not under a linkable root/, + ); + }); + + it('rejects a target escaping the content root', () => { + expect(() => renderRulebookBody('See [x](../../../elsewhere/a.md).', 'a-rulebook', CLAUDE_CONTEXT)).toThrow( + /escapes the content root/, + ); + }); + + it('names the rulebook and the target as authored', () => { + expect(() => + renderRulebookBody('See [x](../../subagents/canary.md).', 'shell-conventions', CLAUDE_CONTEXT), + ).toThrow(/shell-conventions[\s\S]*\.\.\/\.\.\/subagents\/canary\.md/); + }); + + it('reports every offending target in one error', () => { + const body = 'See [a](../../subagents/a.md) and [b](../../collections/b.md).'; + expect(() => renderRulebookBody(body, 'a-rulebook', CLAUDE_CONTEXT)).toThrow(/2 unusable Markdown link target/); + }); + + it('validates before rewriting, so a bad target yields no partial output', () => { + const body = 'Good [a](../../skills/a.md), bad [b](../../subagents/b.md).'; + expect(() => renderRulebookBody(body, 'a-rulebook', CLAUDE_CONTEXT)).toThrow(); + }); + }); + + it('returns a body with no links or variables unchanged', () => { + const body = '# Heading\n\nPlain guidance text.\n'; + expect(renderRulebookBody(body, 'a-rulebook', CLAUDE_CONTEXT)).toBe(body); + }); +}); diff --git a/packages/agents/src/lib/__tests__/subagent-transform.test.ts b/packages/agents/src/lib/__tests__/subagent-transform.test.ts index 2561c5da..9175aa87 100644 --- a/packages/agents/src/lib/__tests__/subagent-transform.test.ts +++ b/packages/agents/src/lib/__tests__/subagent-transform.test.ts @@ -29,7 +29,7 @@ const CLAUDE_OVERLAY = unindent` permissionMode: bypassPermissions `; -const ROVODEV_OVERLAY = unindent` +const ROVO_OVERLAY = unindent` _tools: Read: open_files @@ -61,8 +61,8 @@ describe(renderSubagentForHarness, () => { it('applies the harness-native tool name and home dir for rovodev', () => { const output = renderSubagentForHarness(SOURCE, { - overlayYaml: ROVODEV_OVERLAY, - toolMapping: loadToolMapping(ROVODEV_OVERLAY), + overlayYaml: ROVO_OVERLAY, + toolMapping: loadToolMapping(ROVO_OVERLAY), fileRelPath: 'demo-agent.md', sourceLabel: 'subagents/demo-agent.md', pathPrefix: '.rovodev', @@ -104,8 +104,8 @@ describe(renderSubagentForHarness, () => { expect(claude).not.toContain('{subagent:'); const rovo = renderSubagentForHarness(source, { - overlayYaml: ROVODEV_OVERLAY, - toolMapping: loadToolMapping(ROVODEV_OVERLAY), + overlayYaml: ROVO_OVERLAY, + toolMapping: loadToolMapping(ROVO_OVERLAY), fileRelPath: 'demo-agent.md', sourceLabel: 'subagents/demo-agent.md', pathPrefix: '.rovodev', @@ -151,7 +151,7 @@ describe(renderSubagentForHarness, () => { it.each([ { harnessId: 'claude', overlayYaml: CLAUDE_OVERLAY, homeDir: '.claude', skillSigil: '/', subagentSigil: '' }, - { harnessId: 'rovodev', overlayYaml: ROVODEV_OVERLAY, homeDir: '.rovodev', skillSigil: '!', subagentSigil: '' }, + { harnessId: 'rovodev', overlayYaml: ROVO_OVERLAY, homeDir: '.rovodev', skillSigil: '!', subagentSigil: '' }, ])( 'produces the same $harnessId output as the standalone merge → tools → invocations → markdown-path → template steps', ({ harnessId, overlayYaml, homeDir, skillSigil, subagentSigil }) => { diff --git a/packages/agents/src/lib/__tests__/tool-name-rewriter.test.ts b/packages/agents/src/lib/__tests__/tool-name-rewriter.test.ts index 15a567e7..86ac817d 100644 --- a/packages/agents/src/lib/__tests__/tool-name-rewriter.test.ts +++ b/packages/agents/src/lib/__tests__/tool-name-rewriter.test.ts @@ -2,7 +2,7 @@ import { describe, expect, it } from 'vitest'; import { loadToolMapping, rewriteToolNames, ToolNameRewriteError } from '../tool-name-rewriter.js'; -const IDENTITY = new Map([ +const IDENTITY_MAP = new Map([ ['Bash', 'Bash'], ['Edit', 'Edit'], ['Glob', 'Glob'], @@ -11,7 +11,7 @@ const IDENTITY = new Map([ ['Write', 'Write'], ]); -const ROVODEV = new Map([ +const ROVO_TOOLS_MAP = new Map([ ['Bash', 'bash'], ['Edit', 'find_and_replace_code'], ['Glob', 'expand_folder'], @@ -23,43 +23,45 @@ const ROVODEV = new Map([ describe('rewriteToolNames', () => { it('returns content unchanged when no placeholders are present', () => { const content = 'Plain text with no placeholders.'; - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe(content); + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe(content); }); it('leaves canonical names intact when given an identity mapping', () => { const content = 'Use {tool:Glob}, {tool:Grep}, and {tool:Read} to explore.'; - expect(rewriteToolNames(content, IDENTITY, 'test.md')).toBe('Use Glob, Grep, and Read to explore.'); + expect(rewriteToolNames(content, IDENTITY_MAP, 'test.md')).toBe('Use Glob, Grep, and Read to explore.'); }); it('replaces placeholders with harness-native names for a non-identity mapping', () => { const content = 'Use {tool:Glob}, {tool:Grep}, and {tool:Read} to explore.'; - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe('Use expand_folder, grep, and open_files to explore.'); + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe( + 'Use expand_folder, grep, and open_files to explore.', + ); }); it('replaces multiple placeholders on a single line', () => { const content = 'Not a `{tool:Read}`, not a `{tool:Grep}`, not a `{tool:Bash}` — a `{tool:Write}`.'; - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe( + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe( 'Not a `open_files`, not a `grep`, not a `bash` — a `create_file`.', ); }); it('preserves inline-code backticks around placeholders', () => { const content = 'You have `{tool:Write}` but not `{tool:Edit}`.'; - expect(rewriteToolNames(content, IDENTITY, 'test.md')).toBe('You have `Write` but not `Edit`.'); - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe( + expect(rewriteToolNames(content, IDENTITY_MAP, 'test.md')).toBe('You have `Write` but not `Edit`.'); + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe( 'You have `create_file` but not `find_and_replace_code`.', ); }); it('throws ToolNameRewriteError for an unmapped name', () => { const content = 'Use {tool:NonExistent} for nothing.'; - expect(() => rewriteToolNames(content, ROVODEV, 'test.md')).toThrow(ToolNameRewriteError); + expect(() => rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toThrow(ToolNameRewriteError); }); it('carries toolName, contextLabel, and line on the error', () => { const content = ['Line one is fine.', 'Line two has {tool:NonExistent} on it.'].join('\n'); try { - rewriteToolNames(content, ROVODEV, 'fixtures/sample.md'); + rewriteToolNames(content, ROVO_TOOLS_MAP, 'fixtures/sample.md'); expect.fail('Expected ToolNameRewriteError to be thrown'); } catch (error) { expect(error).toBeInstanceOf(ToolNameRewriteError); @@ -77,7 +79,7 @@ describe('rewriteToolNames', () => { it('reports a line of 1 for placeholders on the first line', () => { const content = '{tool:Unknown} at the very start.'; try { - rewriteToolNames(content, ROVODEV, 'x.md'); + rewriteToolNames(content, ROVO_TOOLS_MAP, 'x.md'); expect.fail('Expected ToolNameRewriteError'); } catch (error) { if (!(error instanceof ToolNameRewriteError)) { @@ -102,17 +104,17 @@ describe('rewriteToolNames', () => { it('does not match malformed placeholders with internal whitespace', () => { const content = 'Not a match: {tool: Read} and {tool : Read}.'; - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe(content); + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe(content); }); it('does not match empty placeholder content', () => { const content = 'Not a match: {tool:} and {tool:_leading_underscore}.'; - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe(content); + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe(content); }); it('does not match placeholders missing the closing brace', () => { const content = 'Not a match: {tool:Read without close.'; - expect(rewriteToolNames(content, ROVODEV, 'test.md')).toBe(content); + expect(rewriteToolNames(content, ROVO_TOOLS_MAP, 'test.md')).toBe(content); }); it('throws on empty mapping when any placeholder is present', () => { diff --git a/packages/agents/src/lib/path-rewriter.ts b/packages/agents/src/lib/path-rewriter.ts index 3811e14b..83b59014 100644 --- a/packages/agents/src/lib/path-rewriter.ts +++ b/packages/agents/src/lib/path-rewriter.ts @@ -1,6 +1,46 @@ import { lstat, readdir, readFile, writeFile } from 'node:fs/promises'; import path from 'node:path'; +/** + * The Markdown link grammar this module rewrites: `[text](target)`, capturing text then target. Exported because the + * grammar and the passthrough predicate together define what gets rewritten, so a caller inspecting links must match + * on both to see the same set. Safe to share despite the `g` flag: `replace` and `matchAll` each leave `lastIndex` + * untouched between calls. + */ +export const MARKDOWN_LINK_REGEX = /\[([^\]]*)\]\(([^)]+)\)/g; + +/** + * Reports whether a Markdown link target is one this module resolves as a source-tree-relative path. False for the + * forms that already name their destination or name nothing to resolve: `http(s)` URLs, absolute paths, `~`-prefixed + * paths, anchor-only links, and targets opening with a `{template_variable}`, which expands to its own absolute path + * after this pass. Exported so a caller that validates link targets tests exactly the set that gets rewritten. + */ +export function isRewritableLinkTarget(target: string): boolean { + return !( + /^https?:\/\//.test(target) || + target.startsWith('/') || + target.startsWith('~') || + target.startsWith('#') || + target.startsWith('{') + ); +} + +/** + * Lists the link targets in `content` that `rewriteMarkdownPaths` would rewrite, in source order and with duplicates + * kept, so a caller that validates link targets tests exactly the set that gets rewritten. A caller needing a wider + * set — anchor-only targets, say — matches on `MARKDOWN_LINK_REGEX` and filters for itself. + */ +export function listRewritableLinkTargets(content: string): ReadonlyArray { + const targets: Array = []; + for (const match of content.matchAll(MARKDOWN_LINK_REGEX)) { + const target = match[2]; + if (target !== undefined && isRewritableLinkTarget(target)) { + targets.push(target); + } + } + return targets; +} + /** * Rewrites relative Markdown link targets in `content` to absolute `~`-prefixed paths. * Resolves each relative target against the directory of `fileRelPath` within the tree rooted @@ -11,10 +51,8 @@ import path from 'node:path'; export function rewriteMarkdownPaths(content: string, fileRelPath: string, pathPrefix: string): string { const fileDir = path.posix.dirname(fileRelPath); - // Match Markdown links [text](target) where target is a relative path - return content.replace(/\[([^\]]*)\]\(([^)]+)\)/g, (_match, text: string, target: string) => { - // Skip non-relative targets: URLs, absolute paths, tilde paths, anchor-only links - if (/^https?:\/\//.test(target) || target.startsWith('/') || target.startsWith('~') || target.startsWith('#')) { + return content.replace(MARKDOWN_LINK_REGEX, (_match, text: string, target: string) => { + if (!isRewritableLinkTarget(target)) { return `[${text}](${target})`; } diff --git a/packages/agents/src/lib/rulebook-deploy.ts b/packages/agents/src/lib/rulebook-deploy.ts new file mode 100644 index 00000000..a9b7b093 --- /dev/null +++ b/packages/agents/src/lib/rulebook-deploy.ts @@ -0,0 +1,56 @@ +import { readFile } from 'node:fs/promises'; +import path from 'node:path'; + +import { artifactFrontmatterPath } from './artifact-types.ts'; +import { describeSearchedLocations, type SourceResolver } from './content-sources.ts'; +import { parseRulebookFile } from './rulebook-schema.ts'; +import { resolveSkillName } from './rulebook-skill.ts'; +import { isEnoent } from './type-guards.ts'; + +/** A declared rulebook resolved against its owning source: its neutral body and which delivery modes it requests. */ +export interface ResolvedRulebook { + readonly slug: string; + readonly skillName: string; + readonly body: string; + readonly ambient: boolean; + readonly skill: boolean; + readonly description: string | undefined; + /** The name of the declared source it resolved from, or `undefined` for the built-in library. */ + readonly source: string | undefined; +} + +/** + * Reads a rulebook from its owning source (a declared source or the library, resolved through `resolver`), validates + * its frontmatter, and returns its neutral body and delivery. A missing frontmatter file throws an error naming the + * resolving source. + */ +export async function resolveRulebook(slug: string, resolver: SourceResolver): Promise { + const resolved = await resolver.resolve('rulebook', slug); + if (resolved === undefined) { + const searched = describeSearchedLocations(resolver, 'rulebook', slug); + throw new Error(`Declared rulebook "${slug}" was not found in any of: ${searched}`); + } + + const srcPath = path.join(resolved.dir, artifactFrontmatterPath('rulebook', slug)); + let content: string; + try { + content = await readFile(srcPath, 'utf8'); + } catch (error: unknown) { + if (isEnoent(error)) { + const origin = resolved.source === undefined ? 'the library' : `source "${resolved.source}"`; + throw new Error(`Declared rulebook "${slug}" was not found in ${origin} at ${srcPath}`); + } + throw error; + } + + const { rulebook, body } = parseRulebookFile(content, `${slug}.md`); + return { + slug, + skillName: resolveSkillName(slug, rulebook['skill-name']), + body: `${body.trim()}\n`, + ambient: rulebook.delivery.includes('ambient'), + skill: rulebook.delivery.includes('skill'), + description: rulebook.description, + source: resolved.source, + }; +} diff --git a/packages/agents/src/lib/rulebook-transform.ts b/packages/agents/src/lib/rulebook-transform.ts new file mode 100644 index 00000000..fafe2877 --- /dev/null +++ b/packages/agents/src/lib/rulebook-transform.ts @@ -0,0 +1,89 @@ +import path from 'node:path'; + +import { listRewritableLinkTargets, rewriteMarkdownPaths, rewriteTemplateVariables } from './path-rewriter.ts'; + +/** The per-harness inputs a rulebook body render depends on, resolved once per harness by the caller. */ +export interface RulebookRenderContext { + /** + * Harness home segment: both the prefix `~/`-prefixed link targets are built under and the `{harness_home_dir}` + * expansion target (e.g. `.claude`). + */ + readonly homeDir: string; + /** Harness identifier that `{harness_id}` tokens expand to (e.g. `claude`). */ + readonly harnessId: string; +} + +/** + * Content-root-relative directory a rulebook source lives in. Anchoring the rewrite here is what lets a rulebook + * address a sibling tree the way its own source tree is laid out. + */ +const RULEBOOK_SOURCE_DIR = 'guidance/rulebooks'; + +/** + * Content-root children a rulebook link target may address, because each deploys to a same-named directory under + * every harness home. `subagents/` is excluded: a subagent definition is dispatched rather than read, so there is no + * link worth authoring into one. `_partials/`, `collections/`, and `guidance/` never deploy as files at all. + */ +const LINKABLE_ROOTS: ReadonlyArray = ['scripts', 'skills']; + +/** + * Renders one rulebook's neutral body for a single harness: relative Markdown links become that harness's absolute + * paths, and template variables expand. Link targets are validated first, so a target naming a location the delivery + * pipeline never creates fails the run instead of shipping as a dead path. + * + * `slug` anchors link rewriting: a relative target resolves against `guidance/rulebooks/.md`, matching where the + * rulebook sits in its content root, and the emitted path is rooted at the harness home. Invocation tokens are + * deliberately not rewritten -- they carry dependency-edge semantics that `dependencies:` expresses for rulebooks. + */ +export function renderRulebookBody(body: string, slug: string, context: RulebookRenderContext): string { + assertLinkTargetsAreDeliverable(body, slug); + const pathRewritten = rewriteMarkdownPaths(body, `${RULEBOOK_SOURCE_DIR}/${slug}.md`, context.homeDir); + return rewriteTemplateVariables(pathRewritten, context.homeDir, context.harnessId); +} + +// region | Helpers + +/** + * Throws when any rewritable link target in `body` names a location the delivery pipeline never creates: one escaping + * the content root, or one rooted outside `LINKABLE_ROOTS`. Every offending target is reported together, so an author + * fixing a rulebook sees the whole list rather than one target per run. + * + * Existence is deliberately not checked here. A target resolves against the deployed tree, which unions library + * content with each declared source's content, so testing it against the one content root this rulebook came from + * would reject a project or machine-local rulebook's link to library content. + */ +function assertLinkTargetsAreDeliverable(body: string, slug: string): void { + const rejections: Array = []; + for (const target of listRewritableLinkTargets(body)) { + const reason = describeRejection(target); + if (reason !== undefined) { + rejections.push(` ${target} -- ${reason}`); + } + } + + if (rejections.length > 0) { + throw new Error( + `Rulebook "${slug}" carries ${rejections.length} unusable Markdown link target(s). A rulebook may link only ` + + `into ${LINKABLE_ROOTS.join('/ and ')}/, the trees that deploy under every harness home:\n` + + rejections.join('\n'), + ); + } +} + +/** Names why a link target cannot be delivered, or `undefined` when it resolves into a linkable root. */ +function describeRejection(target: string): string | undefined { + const hashIndex = target.indexOf('#'); + const pathPart = hashIndex === -1 ? target : target.slice(0, hashIndex); + const resolved = path.posix.normalize(path.posix.join(RULEBOOK_SOURCE_DIR, pathPart)); + + if (resolved === '..' || resolved.startsWith('../')) { + return 'escapes the content root'; + } + const root = resolved.split('/', 1)[0]; + if (root === undefined || !LINKABLE_ROOTS.includes(root)) { + return `resolves to "${resolved}", which is not under a linkable root`; + } + return undefined; +} + +// endregion | Helpers