From 24b201c6eebd50a2e6f7b7aabf6a127db705ec98 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Thu, 20 Aug 2026 23:06:47 -0500 Subject: [PATCH 01/17] feat: pure compile core for compiled skills Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/compile.test.ts | 227 +++++++++++++++++++++++++++ lib/skills/compile.ts | 172 ++++++++++++++++++++ lib/skills/types.ts | 29 ++++ 3 files changed, 428 insertions(+) create mode 100644 lib/skills/__tests__/compile.test.ts create mode 100644 lib/skills/compile.ts create mode 100644 lib/skills/types.ts diff --git a/lib/skills/__tests__/compile.test.ts b/lib/skills/__tests__/compile.test.ts new file mode 100644 index 00000000..4fa06658 --- /dev/null +++ b/lib/skills/__tests__/compile.test.ts @@ -0,0 +1,227 @@ +import { describe, expect, test } from "bun:test"; +import { compileSkill } from "../compile.ts"; +import type { AttachmentSource, CompiledFile, StepSource, VerbDef } from "../types.ts"; + +const verb: VerbDef = { + name: "watch-ci", + engine: "watch-ci-engine", + description: "Watch CI until it goes green", +}; + +const step: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.2.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "Poll the pipeline every 30s and report status. See ${CLAUDE_SKILL_DIR}/scripts/ci-watch.sh for the poller.", + slots: { + domain: { contract: "watch-ci-domain@1", required: true }, + forge: { contract: "ci-forge@1", required: true }, + }, + allowedTools: ["Bash(gh:*)", "Read"], + scriptFiles: ["scripts/ci-watch.sh"], +}; + +const domainFill: AttachmentSource = { + binding: "claimview:watch-ci-domain", + plugin: "claimview", + version: "0.3.0", + dir: "/plugins/claimview/attachments/watch-ci-domain", + body: "Domain rules live at ${CLAUDE_SKILL_DIR}/ci-config.json for details.", + provides: "watch-ci-domain@1", + allowedTools: ["Read(${CLAUDE_SKILL_DIR}/ci-config.json)"], + extraFiles: ["ci-config.json"], +}; + +const forgeFill: AttachmentSource = { + binding: "mattstack:gitlab-forge", + plugin: "mattstack", + version: "1.2.0", + dir: "/plugins/mattstack/skills/gitlab-forge", + body: "Talk to GitLab via glab.", + provides: "ci-forge@1", + allowedTools: ["Bash(glab:*)", "Read"], + extraFiles: [], +}; + +const roster = new Set(["mattstack:watch-ci", "claimview:watch-ci-domain", "mattstack:gitlab-forge"]); + +function skillFileContent(files: CompiledFile[]): string { + const skillFile = files[0]; + if (!skillFile || !("content" in skillFile)) { + throw new Error("expected files[0] to be a content file"); + } + return skillFile.content; +} + +function toolLinesBetween(content: string): string[] { + const match = content.match(/allowed-tools:\n([\s\S]*?)\nmetadata:/); + if (!match) { + throw new Error("allowed-tools block not found"); + } + return (match[1] as string).split("\n").map((line) => line.trim()); +} + +describe("compileSkill", () => { + test("both slots bound: frontmatter, allowed-tools union, seams, body rewrite", () => { + const result = compileSkill(verb, step, { domain: domainFill, forge: forgeFill }, roster); + + expect(result.warnings).toEqual([]); + + const skillFile = result.files[0]; + expect(skillFile?.path).toBe("SKILL.md"); + const content = skillFileContent(result.files); + + expect(content).toContain('name: "watch-ci"'); + expect(content).toContain('description: "Watch CI until it goes green"'); + expect(content).toContain( + 'metadata:\n compiled: "mattstack@1.2.0 + claimview:watch-ci-domain@0.3.0 + mattstack:gitlab-forge@1.2.0"', + ); + + expect(toolLinesBetween(content)).toEqual([ + '- "Bash(gh:*)"', + '- "Read"', + '- "Read(${CLAUDE_SKILL_DIR}/parts/domain/ci-config.json)"', + '- "Bash(glab:*)"', + ]); + + expect(content).toContain(""); + expect(content).toContain( + "", + ); + expect(content).toContain( + "", + ); + + expect(content).toContain( + "Domain rules live at ${CLAUDE_SKILL_DIR}/parts/domain/ci-config.json for details.", + ); + expect(content).toContain( + "Poll the pipeline every 30s and report status. See ${CLAUDE_SKILL_DIR}/scripts/ci-watch.sh for the poller.", + ); + }); + + test("optional slot unbound: no seam, no part, no warning", () => { + const stepWithOptional: StepSource = { + ...step, + slots: { + ...step.slots, + notify: { contract: "notify@1", required: false }, + }, + }; + + const result = compileSkill( + verb, + stepWithOptional, + { domain: domainFill, forge: forgeFill, notify: null }, + roster, + ); + + expect(result.warnings).toEqual([]); + const content = skillFileContent(result.files); + expect(content).not.toContain("slot:notify"); + expect(result.files.some((f) => f.path.startsWith("parts/notify/"))).toBe(false); + }); + + test("required slot unbound throws naming verb, slot, contract", () => { + let error: Error | undefined; + try { + compileSkill(verb, step, { domain: null, forge: forgeFill }, roster); + } catch (e) { + error = e as Error; + } + expect(error).toBeInstanceOf(Error); + expect(error?.message).toContain("watch-ci"); + expect(error?.message).toContain("domain"); + expect(error?.message).toContain("watch-ci-domain@1"); + }); + + test("provides mismatch throws naming expected vs actual", () => { + const badDomainFill: AttachmentSource = { ...domainFill, provides: "watch-ci-domain@2" }; + + let error: Error | undefined; + try { + compileSkill(verb, step, { domain: badDomainFill, forge: forgeFill }, roster); + } catch (e) { + error = e as Error; + } + expect(error).toBeInstanceOf(Error); + expect(error?.message).toContain("watch-ci"); + expect(error?.message).toContain("domain"); + expect(error?.message).toContain("watch-ci-domain@1"); + expect(error?.message).toContain("watch-ci-domain@2"); + expect(error?.message).toContain("claimview:watch-ci-domain"); + }); + + test("vendoring: scriptFiles and extraFiles map to exact copyFrom paths", () => { + const result = compileSkill(verb, step, { domain: domainFill, forge: forgeFill }, roster); + + expect(result.files).toContainEqual({ + path: "scripts/ci-watch.sh", + copyFrom: "/plugins/mattstack/skills/pipeline/watch-ci/scripts/ci-watch.sh", + }); + expect(result.files).toContainEqual({ + path: "parts/domain/ci-config.json", + copyFrom: "/plugins/claimview/attachments/watch-ci-domain/ci-config.json", + }); + expect(result.files.some((f) => f.path.startsWith("parts/forge/"))).toBe(false); + }); + + test("name lint: unregistered token warns, roster member does not", () => { + const lintStep: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.0.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "This step defers domain judgment to claimview:watch-ci-domain and never invokes claimview:nonexistent.", + slots: {}, + allowedTools: [], + scriptFiles: [], + }; + const lintRoster = new Set(["mattstack:watch-ci", "claimview:watch-ci-domain"]); + + const result = compileSkill(verb, lintStep, {}, lintRoster); + + expect(result.warnings).toEqual([ + "body references claimview:nonexistent which is not invocable", + ]); + }); + + test("path lint: vendored file reference is clean, missing file reference warns", () => { + const stepWithMissingRef: StepSource = { + ...step, + body: `${step.body} Also see \${CLAUDE_SKILL_DIR}/parts/domain/missing.json for details.`, + }; + + const result = compileSkill( + verb, + stepWithMissingRef, + { domain: domainFill, forge: forgeFill }, + roster, + ); + + expect(result.warnings).toEqual([ + "body references ${CLAUDE_SKILL_DIR}/parts/domain/missing.json which is not an emitted file", + ]); + }); + + test("determinism: structurally equal inputs produce identical content", () => { + const verbCopy: VerbDef = JSON.parse(JSON.stringify(verb)); + const stepCopy: StepSource = JSON.parse(JSON.stringify(step)); + const domainFillCopy: AttachmentSource = JSON.parse(JSON.stringify(domainFill)); + const forgeFillCopy: AttachmentSource = JSON.parse(JSON.stringify(forgeFill)); + const rosterCopy = new Set(roster); + + const resultA = compileSkill(verb, step, { domain: domainFill, forge: forgeFill }, roster); + const resultB = compileSkill( + verbCopy, + stepCopy, + { domain: domainFillCopy, forge: forgeFillCopy }, + rosterCopy, + ); + + expect(skillFileContent(resultB.files)).toBe(skillFileContent(resultA.files)); + expect(resultB.files).toEqual(resultA.files); + expect(resultB.warnings).toEqual(resultA.warnings); + }); +}); diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts new file mode 100644 index 00000000..2860f261 --- /dev/null +++ b/lib/skills/compile.ts @@ -0,0 +1,172 @@ +import type { AttachmentSource, CompiledFile, CompileResult, StepSource, VerbDef } from "./types.ts"; + +const CLAUDE_SKILL_DIR_TOKEN = "${CLAUDE_SKILL_DIR}"; + +const HEADER_COMMENT = + ""; + +const REGISTERED_NAME_RE = /\b(mattstack|claimview|assured):[a-z][a-z0-9-]*\b/g; +const SKILL_DIR_PATH_RE = /\$\{CLAUDE_SKILL_DIR\}\/[^\s"'`)]+/g; + +type BoundSlot = { slotName: string; fill: AttachmentSource }; + +function rewriteSkillDirRefs(text: string, slotName: string): string { + return text.split(CLAUDE_SKILL_DIR_TOKEN).join(`${CLAUDE_SKILL_DIR_TOKEN}/parts/${slotName}`); +} + +function dedupePreserveOrder(entries: string[]): string[] { + const seen = new Set(); + const out: string[] = []; + for (const entry of entries) { + if (!seen.has(entry)) { + seen.add(entry); + out.push(entry); + } + } + return out; +} + +function yamlQuote(value: string): string { + return `"${value.replace(/\\/g, "\\\\").replace(/"/g, '\\"')}"`; +} + +function resolveBoundSlots( + verb: VerbDef, + step: StepSource, + fills: Record, +): BoundSlot[] { + const boundSlots: BoundSlot[] = []; + + for (const slotName of Object.keys(step.slots)) { + const spec = step.slots[slotName]; + if (!spec) continue; + const fill = fills[slotName] ?? null; + + if (fill === null) { + if (spec.required) { + throw new Error( + `verb "${verb.name}": slot "${slotName}" requires contract "${spec.contract}" but is unbound`, + ); + } + continue; + } + + if (fill.provides !== spec.contract) { + throw new Error( + `verb "${verb.name}": slot "${slotName}" requires contract "${spec.contract}" but binding "${fill.binding}" provides "${fill.provides}"`, + ); + } + + boundSlots.push({ slotName, fill }); + } + + return boundSlots; +} + +function buildAllowedTools(step: StepSource, boundSlots: BoundSlot[]): string[] { + const entries = [ + ...step.allowedTools, + ...boundSlots.flatMap(({ slotName, fill }) => + fill.allowedTools.map((tool) => rewriteSkillDirRefs(tool, slotName)), + ), + ]; + return dedupePreserveOrder(entries); +} + +function buildFrontmatter(verb: VerbDef, allowedTools: string[], compiledParts: string[]): string { + const lines: string[] = ["---"]; + lines.push(`name: ${yamlQuote(verb.name)}`); + lines.push(`description: ${yamlQuote(verb.description)}`); + lines.push("allowed-tools:"); + for (const tool of allowedTools) { + lines.push(` - ${yamlQuote(tool)}`); + } + lines.push("metadata:"); + lines.push(` compiled: ${yamlQuote(compiledParts.join(" + "))}`); + lines.push("---"); + return lines.join("\n"); +} + +function buildBody(step: StepSource, boundSlots: BoundSlot[]): string { + const sections: string[] = [HEADER_COMMENT]; + + sections.push(``); + sections.push(step.body); + + for (const { slotName, fill } of boundSlots) { + sections.push(``); + sections.push(rewriteSkillDirRefs(fill.body, slotName)); + } + + return sections.join("\n\n"); +} + +function buildVendoredFiles(step: StepSource, boundSlots: BoundSlot[]): CompiledFile[] { + const files: CompiledFile[] = []; + + for (const entry of step.scriptFiles) { + const tail = entry.startsWith("scripts/") ? entry.slice("scripts/".length) : entry; + files.push({ path: `scripts/${tail}`, copyFrom: `${step.dir}/${entry}` }); + } + + for (const { slotName, fill } of boundSlots) { + for (const entry of fill.extraFiles) { + files.push({ path: `parts/${slotName}/${entry}`, copyFrom: `${fill.dir}/${entry}` }); + } + } + + return files; +} + +function lintReferences(body: string, roster: Set, files: CompiledFile[]): string[] { + const warnings: string[] = []; + + const seenNames = new Set(); + for (const match of body.matchAll(REGISTERED_NAME_RE)) { + const token = match[0]; + if (seenNames.has(token)) continue; + seenNames.add(token); + if (!roster.has(token)) { + warnings.push(`body references ${token} which is not invocable`); + } + } + + const emittedPaths = new Set(files.map((f) => f.path)); + const seenPaths = new Set(); + for (const match of body.matchAll(SKILL_DIR_PATH_RE)) { + const full = match[0]; + if (seenPaths.has(full)) continue; + seenPaths.add(full); + const relPath = full.slice(`${CLAUDE_SKILL_DIR_TOKEN}/`.length); + if (!emittedPaths.has(relPath)) { + warnings.push(`body references ${full} which is not an emitted file`); + } + } + + return warnings; +} + +export function compileSkill( + verb: VerbDef, + step: StepSource, + fills: Record, + roster: Set, +): CompileResult { + const boundSlots = resolveBoundSlots(verb, step, fills); + + const allowedTools = buildAllowedTools(step, boundSlots); + const compiledParts = [ + `${step.plugin}@${step.version}`, + ...boundSlots.map(({ fill }) => `${fill.binding}@${fill.version}`), + ]; + + const body = buildBody(step, boundSlots); + const frontmatter = buildFrontmatter(verb, allowedTools, compiledParts); + const content = `${frontmatter}\n\n${body}\n`; + + const files: CompiledFile[] = [{ path: "SKILL.md", content }, ...buildVendoredFiles(step, boundSlots)]; + + const warnings = lintReferences(body, roster, files); + + return { files, warnings }; +} diff --git a/lib/skills/types.ts b/lib/skills/types.ts new file mode 100644 index 00000000..da58fe13 --- /dev/null +++ b/lib/skills/types.ts @@ -0,0 +1,29 @@ +export type SlotSpec = { contract: string; required?: boolean }; + +export type StepSource = { + name: string; // engine name, e.g. "watch-ci" + plugin: string; // e.g. "mattstack" + version: string; // plugin version string + dir: string; // absolute source dir + body: string; // frontmatter-stripped markdown + slots: Record; + allowedTools: string[]; // raw entries from frontmatter allowed-tools + scriptFiles: string[]; // paths relative to dir under scripts/, may be empty +}; + +export type AttachmentSource = { + binding: string; // e.g. "claimview:watch-ci-domain" + plugin: string; + version: string; + dir: string; + body: string; + provides: string; // frontmatter metadata.provides, e.g. "watch-ci-domain@1" + allowedTools: string[]; + extraFiles: string[]; // non-SKILL.md files relative to dir, vendored under parts// +}; + +export type VerbDef = { name: string; engine: string; description: string }; + +export type CompiledFile = { path: string; content: string } | { path: string; copyFrom: string }; + +export type CompileResult = { files: CompiledFile[]; warnings: string[] }; From 430060741df602a7b0b698c19445b285dc0e4049 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Thu, 20 Aug 2026 23:14:11 -0500 Subject: [PATCH 02/17] fix: exclude compiler-injected seam comments from reference lint Assert the exact header comment text, and stop the name/path lint from scanning the compiler's own seam/header comment lines so an internal attachment's non-invocable binding no longer self-triggers a warning; author prose is still fully linted. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/compile.test.ts | 30 ++++++++++++++++++++++++++++ lib/skills/compile.ts | 19 ++++++++++++++++-- 2 files changed, 47 insertions(+), 2 deletions(-) diff --git a/lib/skills/__tests__/compile.test.ts b/lib/skills/__tests__/compile.test.ts index 4fa06658..57b2668a 100644 --- a/lib/skills/__tests__/compile.test.ts +++ b/lib/skills/__tests__/compile.test.ts @@ -85,6 +85,9 @@ describe("compileSkill", () => { '- "Bash(glab:*)"', ]); + expect(content).toContain( + "", + ); expect(content).toContain(""); expect(content).toContain( "", @@ -187,6 +190,33 @@ describe("compileSkill", () => { ]); }); + test("name lint: a bound fill's own seam comment does not warn even when its binding is unregistered", () => { + const rosterWithoutDomain = new Set(["mattstack:watch-ci", "mattstack:gitlab-forge"]); + + const result = compileSkill(verb, step, { domain: domainFill, forge: forgeFill }, rosterWithoutDomain); + + expect(result.warnings).toEqual([]); + }); + + test("name lint: the same binding name in author body prose still warns", () => { + const rosterWithoutDomain = new Set(["mattstack:watch-ci", "mattstack:gitlab-forge"]); + const stepMentioningBinding: StepSource = { + ...step, + body: `${step.body} This step composes with claimview:watch-ci-domain for domain rules.`, + }; + + const result = compileSkill( + verb, + stepMentioningBinding, + { domain: domainFill, forge: forgeFill }, + rosterWithoutDomain, + ); + + expect(result.warnings).toEqual([ + "body references claimview:watch-ci-domain which is not invocable", + ]); + }); + test("path lint: vendored file reference is clean, missing file reference warns", () => { const stepWithMissingRef: StepSource = { ...step, diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts index 2860f261..aa11f10a 100644 --- a/lib/skills/compile.ts +++ b/lib/skills/compile.ts @@ -118,11 +118,26 @@ function buildVendoredFiles(step: StepSource, boundSlots: BoundSlot[]): Compiled return files; } +function isCompilerCommentLine(line: string): boolean { + const trimmed = line.trim(); + return trimmed === HEADER_COMMENT || trimmed.startsWith("`); sections.push(rewriteSkillDirRefs(fill.body, slotName)); } diff --git a/lib/skills/types.ts b/lib/skills/types.ts index da58fe13..4b7bfe41 100644 --- a/lib/skills/types.ts +++ b/lib/skills/types.ts @@ -20,6 +20,7 @@ export type AttachmentSource = { provides: string; // frontmatter metadata.provides, e.g. "watch-ci-domain@1" allowedTools: string[]; extraFiles: string[]; // non-SKILL.md files relative to dir, vendored under parts// + registered: boolean; // true = top-level skill under skills/, referenced not inlined; false = internal attachment under attachments/, inlined }; export type VerbDef = { name: string; engine: string; description: string }; From 07c2a023a62dba0b935ed556de7b1020970d21a3 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Thu, 20 Aug 2026 23:32:26 -0500 Subject: [PATCH 04/17] feat: skill source and manifest IO for the compiler Adds lib/skills/sources.ts: stripJsonc/stripFrontmatter parsing primitives, resolvePluginRoots (thin, subprocess-backed), and the loadStepSource/loadAttachment/readVerbRoster/readManifestBindings/ invocableRoster functions that feed Task 1's compile core. Fixture-driven tests via mkdtemp cover the fs-backed functions; resolvePluginRoots stays untested per the plan (Task 5 exercises it live). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/sources.test.ts | 285 +++++++++++++++++++++++++++ lib/skills/sources.ts | 271 +++++++++++++++++++++++++ 2 files changed, 556 insertions(+) create mode 100644 lib/skills/__tests__/sources.test.ts create mode 100644 lib/skills/sources.ts diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts new file mode 100644 index 00000000..8b5dc3d8 --- /dev/null +++ b/lib/skills/__tests__/sources.test.ts @@ -0,0 +1,285 @@ +import { describe, test, expect, beforeEach } from "bun:test"; +import { mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { join } from "path"; +import { + invocableRoster, + loadAttachment, + loadStepSource, + readManifestBindings, + readVerbRoster, + stripFrontmatter, + stripJsonc, + type PluginRoots, +} from "../sources.ts"; + +function writeFile(path: string, content: string): void { + mkdirSync(join(path, ".."), { recursive: true }); + writeFileSync(path, content); +} + +const WATCH_CI_SKILL_MD = `--- +name: watch-ci +description: "Watch CI until it goes green" +type: pipeline-step +allowed-tools: + - "Bash(gh:*)" + - "Read" +slots: + domain: { contract: "watch-ci-domain@1" } + forge: { contract: "ci-forge@1", required: true } +--- + +Poll the pipeline every 30s and report status. +`; + +const WATCH_CI_SCRIPT = "#!/bin/sh\necho polling\n"; + +const WATCH_CI_DOMAIN_SKILL_MD = `--- +name: watch-ci-domain +description: "Domain rules for watch-ci" +metadata: + provides: "watch-ci-domain@1" +allowed-tools: + - "Read(\${CLAUDE_SKILL_DIR}/ci-config.json)" +--- + +Domain rules live at \${CLAUDE_SKILL_DIR}/ci-config.json for details. +`; + +const CI_CONFIG_JSON = `{ "noisy": ["flaky-job"] }\n`; + +/** + * mattstack root: skills/pipeline/watch-ci/SKILL.md (+ scripts/ci-watch.sh), + * matching the real plugin's group/engine nesting. + * claimview root: attachments/watch-ci-domain/SKILL.md (+ ci-config.json), the + * unregistered fill; a registered copy also lives under skills/watch-ci-domain + * so loadAttachment can be exercised against both search roots. + */ +function makeFixtureRoots(): { rootDir: string; roots: PluginRoots } { + const rootDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-src-"))); + + const mattstackDir = join(rootDir, "mattstack"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "SKILL.md"), WATCH_CI_SKILL_MD); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", "ci-watch.sh"), WATCH_CI_SCRIPT); + writeFile(join(mattstackDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + + const untypedSkillMd = WATCH_CI_SKILL_MD.replace("type: pipeline-step\n", ""); + writeFile(join(mattstackDir, "skills", "pipeline", "untyped-step", "SKILL.md"), untypedSkillMd); + + const claimviewDir = join(rootDir, "claimview"); + writeFile(join(claimviewDir, "attachments", "watch-ci-domain", "SKILL.md"), WATCH_CI_DOMAIN_SKILL_MD); + writeFile(join(claimviewDir, "attachments", "watch-ci-domain", "ci-config.json"), CI_CONFIG_JSON); + writeFile(join(claimviewDir, "skills", "cvi-gates", "SKILL.md"), WATCH_CI_DOMAIN_SKILL_MD.replace("watch-ci-domain", "cvi-gates")); + writeFile(join(claimviewDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "0.3.0" })); + + const roots: PluginRoots = { + byName: { + mattstack: { dir: mattstackDir, version: "1.2.0" }, + claimview: { dir: claimviewDir, version: "0.3.0" }, + }, + }; + + return { rootDir, roots }; +} + +describe("stripJsonc", () => { + test("removes full-line // comments but leaves inline content alone", () => { + const raw = [ + "// header comment", + "{", + ' "url": "http://example.com",', + " // trailing comment", + ' "n": 1', + "}", + ].join("\n"); + + const stripped = stripJsonc(raw); + + expect(stripped).toBe(["{", ' "url": "http://example.com",', ' "n": 1', "}"].join("\n")); + expect(JSON.parse(stripped)).toEqual({ url: "http://example.com", n: 1 }); + }); + + test("tolerates indented // comment lines", () => { + const raw = ['{', ' "a": 1,', ' // indented comment', ' "b": 2', "}"].join("\n"); + const stripped = stripJsonc(raw); + expect(stripped).toBe(['{', ' "a": 1,', ' "b": 2', "}"].join("\n")); + }); +}); + +describe("stripFrontmatter", () => { + test("round-trips body exactly and parses frontmatter", () => { + const body = "Line one.\nLine two.\n\nLine four after a blank."; + const md = `---\nname: foo\nnested:\n x: 1\n---\n\n${body}\n`; + + const result = stripFrontmatter(md); + + expect(result.body).toBe(body); + expect(result.frontmatter).toEqual({ name: "foo", nested: { x: 1 } }); + }); + + test("no frontmatter block returns the trimmed body and empty frontmatter", () => { + const result = stripFrontmatter("Just prose, no frontmatter.\n"); + expect(result.body).toBe("Just prose, no frontmatter."); + expect(result.frontmatter).toEqual({}); + }); +}); + +describe("loadStepSource", () => { + test("finds the engine under skills/pipeline/, parses slots, lists scriptFiles", () => { + const { roots } = makeFixtureRoots(); + + const step = loadStepSource("watch-ci", roots); + + expect(step.name).toBe("watch-ci"); + expect(step.plugin).toBe("mattstack"); + expect(step.version).toBe("1.2.0"); + expect(step.dir.endsWith(join("skills", "pipeline", "watch-ci"))).toBe(true); + expect(step.body).toBe("Poll the pipeline every 30s and report status."); + expect(step.slots).toEqual({ + domain: { contract: "watch-ci-domain@1" }, + forge: { contract: "ci-forge@1", required: true }, + }); + expect(step.allowedTools).toEqual(["Bash(gh:*)", "Read"]); + expect(step.scriptFiles).toEqual(["scripts/ci-watch.sh"]); + }); + + test("throws naming the file when the engine has no type: pipeline-step", () => { + const { roots } = makeFixtureRoots(); + + expect(() => loadStepSource("untyped-step", roots)).toThrow(/untyped-step.*SKILL\.md/s); + }); + + test("throws listing searched paths when the engine is absent entirely", () => { + const { roots } = makeFixtureRoots(); + + expect(() => loadStepSource("no-such-engine", roots)).toThrow(/no-such-engine/); + }); +}); + +describe("loadAttachment", () => { + test("finds a registered skill under skills/", () => { + const { roots } = makeFixtureRoots(); + + const fill = loadAttachment("claimview:cvi-gates", "domain", roots); + + expect(fill.registered).toBe(true); + expect(fill.binding).toBe("claimview:cvi-gates"); + expect(fill.plugin).toBe("claimview"); + expect(fill.version).toBe("0.3.0"); + expect(fill.provides).toBe("watch-ci-domain@1"); + expect(fill.dir.endsWith(join("skills", "cvi-gates"))).toBe(true); + }); + + test("finds an unregistered skill under attachments/", () => { + const { roots } = makeFixtureRoots(); + + const fill = loadAttachment("claimview:watch-ci-domain", "domain", roots); + + expect(fill.registered).toBe(false); + expect(fill.binding).toBe("claimview:watch-ci-domain"); + expect(fill.provides).toBe("watch-ci-domain@1"); + expect(fill.body).toBe("Domain rules live at ${CLAUDE_SKILL_DIR}/ci-config.json for details."); + expect(fill.allowedTools).toEqual(["Read(${CLAUDE_SKILL_DIR}/ci-config.json)"]); + expect(fill.dir.endsWith(join("attachments", "watch-ci-domain"))).toBe(true); + }); + + test("extraFiles excludes SKILL.md and includes nested files", () => { + const { rootDir, roots } = makeFixtureRoots(); + writeFile( + join(rootDir, "claimview", "attachments", "watch-ci-domain", "nested", "extra.txt"), + "extra\n", + ); + + const fill = loadAttachment("claimview:watch-ci-domain", "domain", roots); + + expect(fill.extraFiles.sort()).toEqual(["ci-config.json", "nested/extra.txt"]); + }); + + test("throws naming the slot and binding when neither search root has the skill", () => { + const { roots } = makeFixtureRoots(); + + expect(() => loadAttachment("claimview:no-such-skill", "domain", roots)).toThrow( + /domain.*claimview:no-such-skill/s, + ); + }); +}); + +describe("readVerbRoster", () => { + test("parses the real stubs.jsonc shape (comment-bearing JSONC)", () => { + const rootDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-pack-"))); + const stubsJsonc = `// Verb stubs for this pack. generate-stubs.sh renders skills//SKILL.md +// from these; regenerate after edits, never hand-edit generated files. +{ + "verbs": { + "watch-ci": { + "engine": "watch-ci", + "description": "Use when watching or triaging CI." + }, + "ship": { + "engine": "ship", + // inline verb comment + "description": "Use when ready to ship." + } + } +} +`; + writeFile(join(rootDir, "pack", "stubs.jsonc"), stubsJsonc); + + const roster = readVerbRoster(rootDir); + + expect(roster).toEqual([ + { name: "watch-ci", engine: "watch-ci", description: "Use when watching or triaging CI." }, + { name: "ship", engine: "ship", description: "Use when ready to ship." }, + ]); + }); +}); + +describe("readManifestBindings", () => { + test("parses a fixture copied from the real manifest's bindings shape", () => { + const rootDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-manifest-"))); + const manifestJsonc = `// GENERATED by merge-manifests.sh -- do not hand-edit for keeps; +// the next pack install or merge run rewrites this file. +{ + "version": 1, + "skills": { "enabled": ["mattstack:watch-ci"] }, + "bindings": { + "mattstack:watch-ci": { + "domain": "claimview:watch-ci-domain", + "forge": "mattstack:ci-forge-gitlab" + }, + "mattstack:work": { + "tiering": "mattstack:model-tiering" + } + } +} +`; + const manifestPath = join(rootDir, "skills.jsonc"); + writeFile(manifestPath, manifestJsonc); + + const bindings = readManifestBindings(manifestPath); + + expect(bindings).toEqual({ + "mattstack:watch-ci": { + domain: "claimview:watch-ci-domain", + forge: "mattstack:ci-forge-gitlab", + }, + "mattstack:work": { + tiering: "mattstack:model-tiering", + }, + }); + }); +}); + +describe("invocableRoster", () => { + test("lists plugin:skillDirName for one- and two-level skills/ entries", () => { + const { roots } = makeFixtureRoots(); + + const roster = invocableRoster(roots); + + expect(roster.has("mattstack:watch-ci")).toBe(true); + expect(roster.has("mattstack:untyped-step")).toBe(true); + expect(roster.has("claimview:cvi-gates")).toBe(true); + expect(roster.has("claimview:watch-ci-domain")).toBe(false); + }); +}); diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts new file mode 100644 index 00000000..3e8cfef2 --- /dev/null +++ b/lib/skills/sources.ts @@ -0,0 +1,271 @@ +import { execSync } from "child_process"; +import { existsSync, readdirSync, readFileSync, realpathSync } from "fs"; +import { join } from "path"; +import { parse as parseYaml } from "yaml"; +import type { AttachmentSource, SlotSpec, StepSource, VerbDef } from "./types.ts"; + +const FRONTMATTER_RE = /^---\r?\n([\s\S]*?)\r?\n---\r?\n?/; + +export function stripJsonc(raw: string): string { + return raw + .split("\n") + .filter((line) => !line.trim().startsWith("//")) + .join("\n"); +} + +export function stripFrontmatter(md: string): { body: string; frontmatter: Record } { + const match = md.match(FRONTMATTER_RE); + if (!match) { + return { body: md.trim(), frontmatter: {} }; + } + const parsed = parseYaml(match[1] ?? ""); + const frontmatter = parsed && typeof parsed === "object" ? (parsed as Record) : {}; + const body = md.slice(match[0].length).trim(); + return { body, frontmatter }; +} + +export type PluginRoots = { byName: Record }; + +/** + * Thin by design: subprocess + filesystem reads, not unit-tested (see plan + * constraint). Task 5 exercises it live against real installed plugins. + */ +export function resolvePluginRoots(): PluginRoots { + const raw = execSync("claude plugin list --json", { encoding: "utf8" }); + const list = JSON.parse(raw) as Array<{ id: string; installPath: string }>; + const byName: PluginRoots["byName"] = {}; + + for (const entry of list) { + const name = entry.id.split("@")[0]; + if (!name) continue; + const dir = realpathSync(entry.installPath); + let version = "unknown"; + try { + const pluginJson = JSON.parse(readFileSync(join(dir, ".claude-plugin", "plugin.json"), "utf8")); + if (typeof pluginJson.version === "string") version = pluginJson.version; + } catch { + // best-effort: a plugin without a readable manifest still resolves a root + } + byName[name] = { dir, version }; + } + + return { byName }; +} + +function parseSlots(raw: unknown): Record { + if (!raw || typeof raw !== "object") return {}; + const out: Record = {}; + for (const [key, value] of Object.entries(raw as Record)) { + if (!value || typeof value !== "object") continue; + const spec = value as Record; + if (typeof spec.contract !== "string") continue; + out[key] = { + contract: spec.contract, + ...(typeof spec.required === "boolean" ? { required: spec.required } : {}), + }; + } + return out; +} + +function parseAllowedTools(raw: unknown): string[] { + if (Array.isArray(raw)) return raw.filter((entry): entry is string => typeof entry === "string"); + if (typeof raw === "string") { + return raw + .split(",") + .map((entry) => entry.trim()) + .filter(Boolean); + } + return []; +} + +function listFilesUnder(dir: string, exclude: Set): string[] { + const out: string[] = []; + const walk = (sub: string) => { + const abs = sub ? join(dir, sub) : dir; + if (!existsSync(abs)) return; + for (const entry of readdirSync(abs, { withFileTypes: true })) { + const rel = sub ? `${sub}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + walk(rel); + } else if (entry.isFile() && !exclude.has(rel)) { + out.push(rel); + } + } + }; + walk(""); + return out.sort(); +} + +function listDirs(dir: string): string[] { + try { + return readdirSync(dir, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(); + } catch { + return []; + } +} + +export function loadStepSource(engineName: string, roots: PluginRoots): StepSource { + const mattstack = roots.byName.mattstack; + if (!mattstack) { + throw new Error(`loadStepSource: no "mattstack" plugin root registered`); + } + + const skillsDir = join(mattstack.dir, "skills"); + const searched: string[] = []; + let foundDir: string | null = null; + + for (const group of listDirs(skillsDir)) { + const candidate = join(skillsDir, group, engineName, "SKILL.md"); + searched.push(candidate); + if (existsSync(candidate)) { + foundDir = join(skillsDir, group, engineName); + break; + } + } + + if (!foundDir) { + throw new Error( + `loadStepSource: engine "${engineName}" not found under ${skillsDir}/*; searched:\n${searched.join("\n")}`, + ); + } + + const skillMdPath = join(foundDir, "SKILL.md"); + const { body, frontmatter } = stripFrontmatter(readFileSync(skillMdPath, "utf8")); + + if (frontmatter.type !== "pipeline-step") { + throw new Error(`loadStepSource: "${skillMdPath}" is not typed "type: pipeline-step"`); + } + + return { + name: engineName, + plugin: "mattstack", + version: mattstack.version, + dir: foundDir, + body, + slots: parseSlots(frontmatter.slots), + allowedTools: parseAllowedTools(frontmatter["allowed-tools"]), + scriptFiles: listFilesUnder(join(foundDir, "scripts"), new Set()).map((entry) => `scripts/${entry}`), + }; +} + +export function loadAttachment(binding: string, slot: string, roots: PluginRoots): AttachmentSource { + const [plugin, skillName] = binding.split(":"); + if (!plugin || !skillName) { + throw new Error(`loadAttachment: slot "${slot}": binding "${binding}" is not ":"`); + } + + const pluginRoot = roots.byName[plugin]; + if (!pluginRoot) { + throw new Error( + `loadAttachment: slot "${slot}": no plugin root registered for "${plugin}" (binding "${binding}")`, + ); + } + + const searched: string[] = []; + let foundDir: string | null = null; + let registered = false; + + const skillsDir = join(pluginRoot.dir, "skills"); + const oneLevel = join(skillsDir, skillName, "SKILL.md"); + searched.push(oneLevel); + if (existsSync(oneLevel)) { + foundDir = join(skillsDir, skillName); + registered = true; + } + + if (!foundDir) { + for (const group of listDirs(skillsDir)) { + const candidate = join(skillsDir, group, skillName, "SKILL.md"); + searched.push(candidate); + if (existsSync(candidate)) { + foundDir = join(skillsDir, group, skillName); + registered = true; + break; + } + } + } + + if (!foundDir) { + const attachmentPath = join(pluginRoot.dir, "attachments", skillName, "SKILL.md"); + searched.push(attachmentPath); + if (existsSync(attachmentPath)) { + foundDir = join(pluginRoot.dir, "attachments", skillName); + registered = false; + } + } + + if (!foundDir) { + throw new Error( + `loadAttachment: slot "${slot}": binding "${binding}" not found; searched:\n${searched.join("\n")}`, + ); + } + + const skillMdPath = join(foundDir, "SKILL.md"); + const { body, frontmatter } = stripFrontmatter(readFileSync(skillMdPath, "utf8")); + + const metadata = frontmatter.metadata && typeof frontmatter.metadata === "object" + ? (frontmatter.metadata as Record) + : {}; + const provides = typeof metadata.provides === "string" ? metadata.provides : ""; + if (!provides) { + throw new Error(`loadAttachment: slot "${slot}": "${skillMdPath}" has no metadata.provides`); + } + + return { + binding, + plugin, + version: pluginRoot.version, + dir: foundDir, + body, + provides, + allowedTools: parseAllowedTools(frontmatter["allowed-tools"]), + extraFiles: listFilesUnder(foundDir, new Set(["SKILL.md"])), + registered, + }; +} + +export function readVerbRoster(packDir: string): VerbDef[] { + const stubsPath = join(packDir, "pack", "stubs.jsonc"); + const parsed = JSON.parse(stripJsonc(readFileSync(stubsPath, "utf8"))) as { + verbs?: Record; + }; + const verbs = parsed.verbs ?? {}; + return Object.entries(verbs).map(([name, def]) => ({ + name, + engine: def.engine, + description: def.description, + })); +} + +export function readManifestBindings(manifestPath: string): Record> { + const parsed = JSON.parse(stripJsonc(readFileSync(manifestPath, "utf8"))) as { + bindings?: Record>; + }; + return parsed.bindings ?? {}; +} + +export function invocableRoster(roots: PluginRoots): Set { + const roster = new Set(); + + for (const [pluginName, root] of Object.entries(roots.byName)) { + const skillsDir = join(root.dir, "skills"); + + for (const topName of listDirs(skillsDir)) { + if (existsSync(join(skillsDir, topName, "SKILL.md"))) { + roster.add(`${pluginName}:${topName}`); + } + + const groupDir = join(skillsDir, topName); + for (const subName of listDirs(groupDir)) { + if (existsSync(join(groupDir, subName, "SKILL.md"))) { + roster.add(`${pluginName}:${subName}`); + } + } + } + } + + return roster; +} From 98592d0624a2ba9780b9381217f806b34ae7845c Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Thu, 20 Aug 2026 23:39:24 -0500 Subject: [PATCH 05/17] fix: assert concrete searched-path content in the not-found tests The two "not found" tests for loadStepSource/loadAttachment previously asserted only an identifier substring, so they would pass even if the searched-paths listing were empty or broken. Assert the thrown message contains the fixture's actual expected search paths (the SKILL.md candidate under skills/pipeline/, and for loadAttachment both a skills/ candidate and the attachments/ candidate) so a regression that drops the searched list fails the test. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/sources.test.ts | 36 ++++++++++++++++++++++++---- 1 file changed, 32 insertions(+), 4 deletions(-) diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts index 8b5dc3d8..1e47521f 100644 --- a/lib/skills/__tests__/sources.test.ts +++ b/lib/skills/__tests__/sources.test.ts @@ -152,8 +152,23 @@ describe("loadStepSource", () => { test("throws listing searched paths when the engine is absent entirely", () => { const { roots } = makeFixtureRoots(); + const expectedSearchedPath = join( + roots.byName.mattstack!.dir, + "skills", + "pipeline", + "no-such-engine", + "SKILL.md", + ); + + let thrown: unknown; + try { + loadStepSource("no-such-engine", roots); + } catch (err) { + thrown = err; + } - expect(() => loadStepSource("no-such-engine", roots)).toThrow(/no-such-engine/); + expect(thrown).toBeInstanceOf(Error); + expect((thrown as Error).message).toContain(expectedSearchedPath); }); }); @@ -198,10 +213,23 @@ describe("loadAttachment", () => { test("throws naming the slot and binding when neither search root has the skill", () => { const { roots } = makeFixtureRoots(); + const claimviewDir = roots.byName.claimview!.dir; + const expectedSkillsPath = join(claimviewDir, "skills", "no-such-skill", "SKILL.md"); + const expectedAttachmentsPath = join(claimviewDir, "attachments", "no-such-skill", "SKILL.md"); + + let thrown: unknown; + try { + loadAttachment("claimview:no-such-skill", "domain", roots); + } catch (err) { + thrown = err; + } - expect(() => loadAttachment("claimview:no-such-skill", "domain", roots)).toThrow( - /domain.*claimview:no-such-skill/s, - ); + expect(thrown).toBeInstanceOf(Error); + const message = (thrown as Error).message; + expect(message).toContain("domain"); + expect(message).toContain("claimview:no-such-skill"); + expect(message).toContain(expectedSkillsPath); + expect(message).toContain(expectedAttachmentsPath); }); }); From aa1ee36b124adff6840c1226894c719f5ac2e98f Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 00:03:10 -0500 Subject: [PATCH 06/17] feat: rt skills compile and check verbs Wires the compile core (Task 1) and source/manifest IO (Task 2) into `rt skills compile` and `rt skills check`. --pack-dir/--mattstack-dir are hidden test-only flags so fixture tests never touch real ~/.mattstack or ~/.claude; --mattstack-dir also stands in for the Claude plugin cache in tests since execSync-based PATH shims don't reliably reach a mocked `claude` binary from inside this process. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/__tests__/skills.test.ts | 322 ++++++++++++++++++++++++++++++ commands/skills.ts | 265 ++++++++++++++++++++++++ lib/command-tree-def.ts | 27 +++ lib/module-registry.ts | 2 + 4 files changed, 616 insertions(+) create mode 100644 commands/__tests__/skills.test.ts create mode 100644 commands/skills.ts diff --git a/commands/__tests__/skills.test.ts b/commands/__tests__/skills.test.ts new file mode 100644 index 00000000..47dcb366 --- /dev/null +++ b/commands/__tests__/skills.test.ts @@ -0,0 +1,322 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import { chmodSync, existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, statSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { dirname, join } from "path"; +import { skillsCheck, skillsCompile } from "../skills.ts"; +import { compileSkill } from "../../lib/skills/compile.ts"; +import { invocableRoster, loadAttachment, loadStepSource } from "../../lib/skills/sources.ts"; +import type { PluginRoots } from "../../lib/skills/sources.ts"; +import type { VerbDef } from "../../lib/skills/types.ts"; + +function writeFile(path: string, content: string): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content); +} + +const WATCH_CI_SKILL_MD = `--- +name: watch-ci +description: "Watch CI until it goes green" +type: pipeline-step +allowed-tools: + - "Bash(gh:*)" + - "Read" +slots: + domain: { contract: "watch-ci-domain@1" } + forge: { contract: "ci-forge@1", required: true } +--- + +Poll the pipeline every 30s and report status. See \${CLAUDE_SKILL_DIR}/scripts/ci-watch.sh for the poller. +`; + +const WATCH_CI_SCRIPT = "#!/bin/sh\necho polling\n"; + +const DOMAIN_SKILL_MD = `--- +name: watch-ci-domain +description: "Domain rules for watch-ci" +metadata: + provides: "watch-ci-domain@1" +allowed-tools: + - "Read(\${CLAUDE_SKILL_DIR}/ci-config.json)" +--- + +Domain rules live at \${CLAUDE_SKILL_DIR}/ci-config.json for details. +`; + +const CI_CONFIG_JSON = `{ "noisy": ["flaky-job"] }\n`; + +const FORGE_SKILL_MD = `--- +name: gitlab-forge +description: "Talk to GitLab via glab" +metadata: + provides: "ci-forge@1" +allowed-tools: + - "Bash(glab:*)" +--- + +Talk to GitLab via glab. +`; + +const STUBS_JSONC = `{ + "verbs": { + "watch-ci": { + "engine": "watch-ci", + "description": "Use when watching or triaging CI." + } + } +} +`; + +function manifestJsonc(team: string, withForge: boolean): string { + const bindings = withForge + ? `{ + "domain": "claimview:watch-ci-domain", + "forge": "mattstack:gitlab-forge" + }` + : `{ + "domain": "claimview:watch-ci-domain" + }`; + return `// GENERATED by merge-manifests.sh -- do not hand-edit; +// provenance (binding <- source): +// mattstack:watch-ci domain <- ${team}@${team} +{ + "bindings": { + "mattstack:watch-ci": ${bindings} + } +} +`; +} + +/** + * Fixture "mattstack home" root: plugins// mirrors resolvePluginRoots()'s + * real shape (dir + .claude-plugin/plugin.json) so skillsCompile's test-mode + * root resolution and the golden compile below walk identical trees. + */ +function makeMattstackDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-cli-mattstack-"))); + + const mattstackPluginDir = join(dir, "plugins", "mattstack"); + writeFile(join(mattstackPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "watch-ci", "SKILL.md"), WATCH_CI_SKILL_MD); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "watch-ci", "scripts", "ci-watch.sh"), WATCH_CI_SCRIPT); + chmodSync(join(mattstackPluginDir, "skills", "pipeline", "watch-ci", "scripts", "ci-watch.sh"), 0o755); + writeFile(join(mattstackPluginDir, "skills", "gitlab-forge", "SKILL.md"), FORGE_SKILL_MD); + + const claimviewPluginDir = join(dir, "plugins", "claimview"); + writeFile(join(claimviewPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "0.3.0" })); + writeFile(join(claimviewPluginDir, "attachments", "watch-ci-domain", "SKILL.md"), DOMAIN_SKILL_MD); + writeFile(join(claimviewPluginDir, "attachments", "watch-ci-domain", "ci-config.json"), CI_CONFIG_JSON); + + return dir; +} + +function makePackDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-cli-pack-"))); + writeFile(join(dir, "pack", "stubs.jsonc"), STUBS_JSONC); + return dir; +} + +function makeManifest(team: string, withForge = true): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-cli-manifest-"))); + const path = join(dir, "skills.jsonc"); + writeFile(path, manifestJsonc(team, withForge)); + return path; +} + +function computeGolden(mattstackDir: string) { + const roots: PluginRoots = { + byName: { + mattstack: { dir: join(mattstackDir, "plugins", "mattstack"), version: "1.2.0" }, + claimview: { dir: join(mattstackDir, "plugins", "claimview"), version: "0.3.0" }, + }, + }; + const verb: VerbDef = { name: "watch-ci", engine: "watch-ci", description: "Use when watching or triaging CI." }; + const step = loadStepSource("watch-ci", roots); + const domain = loadAttachment("claimview:watch-ci-domain", "domain", roots); + const forge = loadAttachment("mattstack:gitlab-forge", "forge", roots); + const invocable = invocableRoster(roots); + return compileSkill(verb, step, { domain, forge }, invocable); +} + +let logSpy: ReturnType; +let logs: string[]; + +beforeEach(() => { + logs = []; + logSpy = spyOn(console, "log").mockImplementation((...args: unknown[]) => { + logs.push(args.map(String).join(" ")); + }); +}); + +afterEach(() => { + logSpy.mockRestore(); + // skillsCheck sets this on staleness; never let it leak into the suite's own exit status. + process.exitCode = undefined; +}); + +describe("skillsCompile", () => { + test("dry-run prints a would-write summary and writes nothing", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--dry-run", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + const summary = logs.find((l) => /would write \d+ files/.test(l)); + expect(summary).toBeDefined(); + expect(summary).toContain("watch-ci"); + expect(existsSync(join(packDir, "skills", "watch-ci"))).toBe(false); + }); + + test("real run emits SKILL.md + vendored files matching a golden compile, byte for byte", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + expect(logs.some((l) => l.includes("compiled watch-ci") && l.includes("3 files"))).toBe(true); + + const golden = computeGolden(mattstackDir); + const outDir = join(packDir, "skills", "watch-ci"); + + for (const file of golden.files) { + const dest = join(outDir, file.path); + expect(existsSync(dest)).toBe(true); + const expected = "content" in file ? file.content : readFileSync(file.copyFrom, "utf8"); + expect(readFileSync(dest, "utf8")).toBe(expected); + } + + const scriptPath = join(outDir, "scripts", "ci-watch.sh"); + expect(statSync(scriptPath).mode & 0o111).not.toBe(0); + }); + + test("missing required binding: nonzero exit, error names verb and slot", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t", false); + + let thrown: unknown; + try { + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + } catch (err) { + thrown = err; + } + + expect(thrown).toBeInstanceOf(Error); + const message = (thrown as Error).message; + expect(message).toContain("watch-ci"); + expect(message).toContain("forge"); + expect(existsSync(join(packDir, "skills", "watch-ci"))).toBe(false); + }); + + test("default pack dir formula (--team + --mattstack-dir, no --pack-dir)", async () => { + const mattstackDir = makeMattstackDir(); + const manifestPath = makeManifest("t"); + writeFile(join(mattstackDir, "teams", "t", "mattstack", "packs", "t", "pack", "stubs.jsonc"), STUBS_JSONC); + + await skillsCompile([ + "--team", "t", + "--dry-run", + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + ]); + + expect(logs.some((l) => /would write \d+ files/.test(l) && l.includes("watch-ci"))).toBe(true); + }); + + test("default manifest lookup (--mattstack-dir only, no --manifest)", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + writeFile(join(mattstackDir, "repos", "some-repo", "skills.jsonc"), manifestJsonc("t", true)); + + await skillsCompile([ + "--team", "t", + "--dry-run", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--verb", "watch-ci", + ]); + + expect(logs.some((l) => /would write \d+ files/.test(l))).toBe(true); + }); +}); + +describe("skillsCheck", () => { + test("reports current right after a compile", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + logs = []; + + await skillsCheck([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + expect(logs.some((l) => l.includes("watch-ci") && l.includes("current"))).toBe(true); + expect(logs.some((l) => l.includes("stale"))).toBe(false); + expect(process.exitCode).not.toBe(1); + }); + + test("hand-edited SKILL.md reports stale and exits 1", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + const skillMdPath = join(packDir, "skills", "watch-ci", "SKILL.md"); + writeFileSync(skillMdPath, readFileSync(skillMdPath, "utf8") + "\nhand-edited drift\n"); + logs = []; + + await skillsCheck([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + const staleLine = logs.find((l) => l.includes("stale")); + expect(staleLine).toBeDefined(); + expect(staleLine).toContain("watch-ci"); + expect(staleLine).toContain("SKILL.md"); + expect(process.exitCode).toBe(1); + }); +}); diff --git a/commands/skills.ts b/commands/skills.ts new file mode 100644 index 00000000..447397b3 --- /dev/null +++ b/commands/skills.ts @@ -0,0 +1,265 @@ +/** + * rt skills -- compile pack verbs from step sources + manifest bindings into + * committed SKILL.md files, and check compiled output against its sources. + * + * rt skills compile [--team ] [--verb ...] [--manifest ] [--dry-run] + * rt skills check [--team ] [--verb ...] [--manifest ] + * + * --pack-dir / --mattstack-dir are test-only escape hatches (hidden from the + * command tree): they let tests point the whole resolution chain at a + * mkdtemp fixture instead of the real ~/.mattstack, without a PATH-shimmed + * `claude` binary -- execSync inside this process ignores runtime PATH + * mutations (resolved at Bun's own startup), so a fake `claude` on PATH is + * not a reliable test seam. --mattstack-dir stands in for both the + * ~/.mattstack root (pack-dir and manifest defaults) and the Claude plugin + * cache (mirrored under /plugins//) that resolvePluginRoots() + * queries for real via `claude plugin list --json`. + */ + +import { chmodSync, copyFileSync, existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"; +import { dirname, join } from "path"; +import { mattstackHome } from "../lib/rt-paths.ts"; +import { compileSkill } from "../lib/skills/compile.ts"; +import { + invocableRoster, + loadAttachment, + loadStepSource, + readManifestBindings, + readVerbRoster, + resolvePluginRoots, + type PluginRoots, +} from "../lib/skills/sources.ts"; +import type { AttachmentSource, CompileResult, VerbDef } from "../lib/skills/types.ts"; + +type Flags = { + team: string; + verbs: string[] | null; + manifest: string | null; + dryRun: boolean; + packDir: string | null; + mattstackDir: string | null; +}; + +function parseFlags(args: string[]): Flags { + const verbs: string[] = []; + let team = "claimview"; + let manifest: string | null = null; + let dryRun = false; + let packDir: string | null = null; + let mattstackDir: string | null = null; + + for (let i = 0; i < args.length; i++) { + const a = args[i]!; + switch (a) { + case "--team": team = args[++i] ?? team; break; + case "--verb": { const v = args[++i]; if (v) verbs.push(v); break; } + case "--manifest": manifest = args[++i] ?? null; break; + case "--dry-run": dryRun = true; break; + case "--pack-dir": packDir = args[++i] ?? null; break; + case "--mattstack-dir": mattstackDir = args[++i] ?? null; break; + default: + throw new Error(`rt skills: unrecognized argument "${a}"`); + } + } + + return { team, verbs: verbs.length ? verbs : null, manifest, dryRun, packDir, mattstackDir }; +} + +function packRootDir(mattstackRoot: string, team: string): string { + return join(mattstackRoot, "teams", team, "mattstack", "packs", team); +} + +function leadingCommentBlock(raw: string): string { + const lines: string[] = []; + for (const line of raw.split("\n")) { + if (line.trim().startsWith("//")) { + lines.push(line); + continue; + } + break; + } + return lines.join("\n"); +} + +function listSubdirs(dir: string): string[] { + if (!existsSync(dir)) return []; + return readdirSync(dir, { withFileTypes: true }) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(); +} + +function findDefaultManifest(mattstackRoot: string, team: string): string { + const reposRoot = join(mattstackRoot, "repos"); + const candidates: { path: string; mtimeMs: number }[] = []; + + for (const repoName of listSubdirs(reposRoot)) { + const manifestPath = join(reposRoot, repoName, "skills.jsonc"); + if (!existsSync(manifestPath)) continue; + const header = leadingCommentBlock(readFileSync(manifestPath, "utf8")); + if (!header.includes(team)) continue; + candidates.push({ path: manifestPath, mtimeMs: statSync(manifestPath).mtimeMs }); + } + + if (candidates.length === 0) { + throw new Error( + `rt skills: no skills.jsonc under ${reposRoot}/*/ names team "${team}" in its provenance header; pass --manifest explicitly`, + ); + } + + candidates.sort((a, b) => b.mtimeMs - a.mtimeMs); + const newest = candidates[0]!; + const tied = candidates.filter((c) => c.mtimeMs === newest.mtimeMs); + if (tied.length > 1) { + throw new Error( + `rt skills: ambiguous manifest for team "${team}" -- candidates tie for newest:\n${tied.map((c) => c.path).join("\n")}\npass --manifest explicitly`, + ); + } + + return newest.path; +} + +/** Test-mode-only: scans /plugins//.claude-plugin/plugin.json, bypassing the real `claude plugin list --json`. */ +function resolvePluginRootsFromDir(dir: string): PluginRoots { + const pluginsDir = join(dir, "plugins"); + const byName: PluginRoots["byName"] = {}; + + for (const name of listSubdirs(pluginsDir)) { + const pluginDir = join(pluginsDir, name); + let version = "unknown"; + try { + const parsed = JSON.parse(readFileSync(join(pluginDir, ".claude-plugin", "plugin.json"), "utf8")); + if (typeof parsed.version === "string") version = parsed.version; + } catch { + // best-effort: a fixture plugin without a readable manifest still resolves a root + } + byName[name] = { dir: pluginDir, version }; + } + + return { byName }; +} + +function selectVerbs(roster: VerbDef[], names: string[] | null): VerbDef[] { + if (!names) return roster; + const byName = new Map(roster.map((v) => [v.name, v])); + return names.map((name) => { + const verb = byName.get(name); + if (!verb) throw new Error(`rt skills: verb "${name}" not found in roster`); + return verb; + }); +} + +type Resolved = { + packDir: string; + roster: VerbDef[]; + bindings: Record>; + pluginRoots: PluginRoots; + invocable: Set; +}; + +function resolve(flags: Flags): Resolved { + const mattstackRoot = flags.mattstackDir ?? mattstackHome(); + const packDir = flags.packDir ?? packRootDir(mattstackRoot, flags.team); + const manifestPath = flags.manifest ?? findDefaultManifest(mattstackRoot, flags.team); + + const roster = selectVerbs(readVerbRoster(packDir), flags.verbs); + const bindings = readManifestBindings(manifestPath); + const pluginRoots = flags.mattstackDir ? resolvePluginRootsFromDir(mattstackRoot) : resolvePluginRoots(); + const invocable = invocableRoster(pluginRoots); + + return { packDir, roster, bindings, pluginRoots, invocable }; +} + +function compileVerb(verb: VerbDef, resolved: Resolved): CompileResult { + let step; + try { + step = loadStepSource(verb.engine, resolved.pluginRoots); + } catch (err) { + throw new Error(`verb "${verb.name}": ${(err as Error).message}`); + } + + const slotBindings = resolved.bindings[`${step.plugin}:${verb.engine}`] ?? {}; + const fills: Record = {}; + for (const slotName of Object.keys(step.slots)) { + const bindingName = slotBindings[slotName]; + if (!bindingName) { + fills[slotName] = null; + continue; + } + try { + fills[slotName] = loadAttachment(bindingName, slotName, resolved.pluginRoots); + } catch (err) { + throw new Error(`verb "${verb.name}": ${(err as Error).message}`); + } + } + + return compileSkill(verb, step, fills, resolved.invocable); +} + +function writeCompiledVerb(outDir: string, result: CompileResult): void { + rmSync(outDir, { recursive: true, force: true }); + mkdirSync(outDir, { recursive: true }); + + for (const file of result.files) { + const dest = join(outDir, file.path); + mkdirSync(dirname(dest), { recursive: true }); + if ("content" in file) { + writeFileSync(dest, file.content); + } else { + copyFileSync(file.copyFrom, dest); + chmodSync(dest, statSync(file.copyFrom).mode); + } + } +} + +export async function skillsCompile(args: string[]): Promise { + const flags = parseFlags(args); + const resolved = resolve(flags); + + for (const verb of resolved.roster) { + const result = compileVerb(verb, resolved); + const outDir = join(resolved.packDir, "skills", verb.name); + + if (flags.dryRun) { + console.log(`would write ${result.files.length} files for ${verb.name}`); + for (const warning of result.warnings) console.log(` ${warning}`); + continue; + } + + writeCompiledVerb(outDir, result); + console.log(`compiled ${verb.name} (${result.files.length} files, ${result.warnings.length} warnings)`); + for (const warning of result.warnings) console.log(` ${warning}`); + } +} + +export async function skillsCheck(args: string[]): Promise { + const flags = parseFlags(args); + const resolved = resolve(flags); + + let anyStale = false; + + for (const verb of resolved.roster) { + const outDir = join(resolved.packDir, "skills", verb.name); + if (!existsSync(outDir)) continue; + + const result = compileVerb(verb, resolved); + const staleFiles: string[] = []; + + for (const file of result.files) { + const dest = join(outDir, file.path); + const expected = "content" in file ? Buffer.from(file.content) : readFileSync(file.copyFrom); + if (!existsSync(dest) || !readFileSync(dest).equals(expected)) { + staleFiles.push(file.path); + } + } + + if (staleFiles.length > 0) { + anyStale = true; + console.log(`${verb.name}: stale (recompile or investigate drift with git diff) -- ${staleFiles.join(", ")}`); + } else { + console.log(`${verb.name}: current`); + } + } + + if (anyStale) process.exitCode = 1; +} diff --git a/lib/command-tree-def.ts b/lib/command-tree-def.ts index cdc380fc..37c6e7f9 100644 --- a/lib/command-tree-def.ts +++ b/lib/command-tree-def.ts @@ -750,6 +750,33 @@ export const TREE: Record = { }, }, + skills: { + description: "Compile and check the pack's committed skills", + subcommands: { + compile: { + description: "Compile pack verbs from step sources + manifest bindings into committed SKILL.md files", + module: "./commands/skills.ts", + fn: "skillsCompile", + args: [ + { name: "Team", flag: "--team", type: "text", placeholder: "claimview", hint: "Pack team; default claimview" }, + { name: "Verb", flag: "--verb", type: "text", placeholder: "watch-ci", hint: "Compile only this verb (repeatable); omit for every verb in the roster" }, + { name: "Manifest", flag: "--manifest", type: "text", placeholder: "/path/to/skills.jsonc", hint: "Manifest path; omit to auto-find the newest ~/.mattstack/repos/*/skills.jsonc naming this team" }, + { name: "Dry run", flag: "--dry-run", type: "boolean", default: false, hint: "Print what would be written without touching disk" }, + ], + }, + check: { + description: "Report compiled skills that no longer match their sources", + module: "./commands/skills.ts", + fn: "skillsCheck", + args: [ + { name: "Team", flag: "--team", type: "text", placeholder: "claimview", hint: "Pack team; default claimview" }, + { name: "Verb", flag: "--verb", type: "text", placeholder: "watch-ci", hint: "Check only this verb (repeatable); omit for every compiled verb" }, + { name: "Manifest", flag: "--manifest", type: "text", placeholder: "/path/to/skills.jsonc", hint: "Manifest path; omit to auto-find the newest ~/.mattstack/repos/*/skills.jsonc naming this team" }, + ], + }, + }, + }, + plugin: { description: "Manage user plugins", subcommands: { diff --git a/lib/module-registry.ts b/lib/module-registry.ts index f862c755..ba9cf06a 100644 --- a/lib/module-registry.ts +++ b/lib/module-registry.ts @@ -17,6 +17,7 @@ import * as run from "../commands/run.ts"; import * as secrets from "../commands/secrets.ts"; import * as settings from "../commands/settings.ts"; import * as settingsKeys from "../commands/settings-keys.ts"; +import * as skills from "../commands/skills.ts"; import * as sync from "../commands/sync.ts"; import * as rebase from "../commands/git/rebase.ts"; import * as reset from "../commands/git/reset.ts"; @@ -47,6 +48,7 @@ export const MODULE_REGISTRY: Record = { "./commands/secrets.ts": secrets, "./commands/settings.ts": settings, "./commands/settings-keys.ts": settingsKeys, + "./commands/skills.ts": skills, "./commands/sync.ts": sync, "./commands/git/rebase.ts": rebase, "./commands/git/reset.ts": reset, From 33b174facd50bf6ba7b94c915015991f06dd45f8 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 00:39:42 -0500 Subject: [PATCH 07/17] fix: clean rt skills error reporting; skip stale plugin cache entries Review fix round 1: - commands/skills.ts: wrap skillsCompile/skillsCheck in a SkillsUsageError boundary matching commands/secrets.ts and commands/settings-keys.ts -- expected domain errors (missing binding, ambiguous/absent manifest, unknown verb, unrecognized argument) now print a one-line "rt skills: " to stderr and exit 1 with no stack trace; anything else still propagates. - lib/skills/sources.ts: resolvePluginRoots now skips a plugin-list entry whose installPath no longer exists (real case on dev machines: a stale cache entry) instead of crashing resolution for every other plugin, printing a one-line warning naming the plugin and path. Entry-processing logic split into buildPluginRoots so it's fixture-testable; the subprocess call itself stays thin and untested, as before. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/__tests__/skills.test.ts | 103 ++++++++++++++++++++--- commands/skills.ts | 117 +++++++++++++++++---------- lib/skills/__tests__/sources.test.ts | 63 ++++++++++++++- lib/skills/sources.ts | 29 +++++-- 4 files changed, 249 insertions(+), 63 deletions(-) diff --git a/commands/__tests__/skills.test.ts b/commands/__tests__/skills.test.ts index 47dcb366..b2457bf6 100644 --- a/commands/__tests__/skills.test.ts +++ b/commands/__tests__/skills.test.ts @@ -153,6 +153,33 @@ afterEach(() => { process.exitCode = undefined; }); +/** + * Expected/domain errors print a clean one-liner and process.exit(1) instead + * of throwing a bare stack trace (matches commands/secrets.ts, + * commands/settings-keys.ts). Mock process.exit to throw a sentinel so the + * real test process never dies, and read the spies' recorded calls BEFORE + * mockRestore() -- bun's mockRestore() clears .mock.calls, unlike jest's. + */ +async function runExpectingCleanExit(fn: () => Promise): Promise<{ exitCode: number | undefined; errors: string[] }> { + const errors: string[] = []; + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit sentinel"); + }); + const errorSpy = spyOn(console, "error").mockImplementation((...args: unknown[]) => { + errors.push(args.map(String).join(" ")); + }); + try { + await fn(); + return { exitCode: undefined, errors }; + } catch { + const exitCode = exitSpy.mock.calls.at(-1)?.[0] as number | undefined; + return { exitCode, errors }; + } finally { + exitSpy.mockRestore(); + errorSpy.mockRestore(); + } +} + describe("skillsCompile", () => { test("dry-run prints a would-write summary and writes nothing", async () => { const mattstackDir = makeMattstackDir(); @@ -203,31 +230,81 @@ describe("skillsCompile", () => { expect(statSync(scriptPath).mode & 0o111).not.toBe(0); }); - test("missing required binding: nonzero exit, error names verb and slot", async () => { + test("missing required binding: clean one-line error naming verb and slot, exit 1, no partial write", async () => { const mattstackDir = makeMattstackDir(); const packDir = makePackDir(); const manifestPath = makeManifest("t", false); - let thrown: unknown; - try { - await skillsCompile([ + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsCompile([ "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, "--verb", "watch-ci", - ]); - } catch (err) { - thrown = err; - } - - expect(thrown).toBeInstanceOf(Error); - const message = (thrown as Error).message; - expect(message).toContain("watch-ci"); - expect(message).toContain("forge"); + ]), + ); + + expect(exitCode).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).not.toContain("\n at "); // no stack trace + expect(errors[0]).toContain("watch-ci"); + expect(errors[0]).toContain("forge"); expect(existsSync(join(packDir, "skills", "watch-ci"))).toBe(false); }); + test("unrecognized argument: clean one-line error, exit 1", async () => { + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsCompile(["--bogus-flag"]), + ); + + expect(exitCode).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("--bogus-flag"); + }); + + test("unknown verb: clean one-line error, exit 1", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "no-such-verb", + ]), + ); + + expect(exitCode).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("no-such-verb"); + }); + + test("absent manifest (no skills.jsonc names the team): clean one-line error, exit 1", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--verb", "watch-ci", + ]), + ); + + expect(exitCode).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("skills.jsonc"); + }); + test("default pack dir formula (--team + --mattstack-dir, no --pack-dir)", async () => { const mattstackDir = makeMattstackDir(); const manifestPath = makeManifest("t"); diff --git a/commands/skills.ts b/commands/skills.ts index 447397b3..5233ffc8 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -31,6 +31,26 @@ import { } from "../lib/skills/sources.ts"; import type { AttachmentSource, CompileResult, VerbDef } from "../lib/skills/types.ts"; +/** + * Marks an error as an expected, user-facing condition (bad flags, absent + * binding, unknown verb) rather than a bug in this command -- withCleanErrors + * prints these as a one-line "rt skills: " and exits 1 with no + * stack trace; anything else propagates to the top-level crash handler. + */ +class SkillsUsageError extends Error {} + +async function withCleanErrors(fn: () => Promise): Promise { + try { + await fn(); + } catch (err) { + if (err instanceof SkillsUsageError) { + console.error(`rt skills: ${err.message}`); + process.exit(1); + } + throw err; + } +} + type Flags = { team: string; verbs: string[] | null; @@ -58,7 +78,7 @@ function parseFlags(args: string[]): Flags { case "--pack-dir": packDir = args[++i] ?? null; break; case "--mattstack-dir": mattstackDir = args[++i] ?? null; break; default: - throw new Error(`rt skills: unrecognized argument "${a}"`); + throw new SkillsUsageError(`unrecognized argument "${a}"`); } } @@ -102,8 +122,8 @@ function findDefaultManifest(mattstackRoot: string, team: string): string { } if (candidates.length === 0) { - throw new Error( - `rt skills: no skills.jsonc under ${reposRoot}/*/ names team "${team}" in its provenance header; pass --manifest explicitly`, + throw new SkillsUsageError( + `no skills.jsonc under ${reposRoot}/*/ names team "${team}" in its provenance header; pass --manifest explicitly`, ); } @@ -111,8 +131,8 @@ function findDefaultManifest(mattstackRoot: string, team: string): string { const newest = candidates[0]!; const tied = candidates.filter((c) => c.mtimeMs === newest.mtimeMs); if (tied.length > 1) { - throw new Error( - `rt skills: ambiguous manifest for team "${team}" -- candidates tie for newest:\n${tied.map((c) => c.path).join("\n")}\npass --manifest explicitly`, + throw new SkillsUsageError( + `ambiguous manifest for team "${team}" -- candidates tie for newest:\n${tied.map((c) => c.path).join("\n")}\npass --manifest explicitly`, ); } @@ -144,7 +164,7 @@ function selectVerbs(roster: VerbDef[], names: string[] | null): VerbDef[] { const byName = new Map(roster.map((v) => [v.name, v])); return names.map((name) => { const verb = byName.get(name); - if (!verb) throw new Error(`rt skills: verb "${name}" not found in roster`); + if (!verb) throw new SkillsUsageError(`verb "${name}" not found in roster`); return verb; }); } @@ -175,7 +195,7 @@ function compileVerb(verb: VerbDef, resolved: Resolved): CompileResult { try { step = loadStepSource(verb.engine, resolved.pluginRoots); } catch (err) { - throw new Error(`verb "${verb.name}": ${(err as Error).message}`); + throw new SkillsUsageError(`verb "${verb.name}": ${(err as Error).message}`); } const slotBindings = resolved.bindings[`${step.plugin}:${verb.engine}`] ?? {}; @@ -189,11 +209,16 @@ function compileVerb(verb: VerbDef, resolved: Resolved): CompileResult { try { fills[slotName] = loadAttachment(bindingName, slotName, resolved.pluginRoots); } catch (err) { - throw new Error(`verb "${verb.name}": ${(err as Error).message}`); + throw new SkillsUsageError(`verb "${verb.name}": ${(err as Error).message}`); } } - return compileSkill(verb, step, fills, resolved.invocable); + try { + return compileSkill(verb, step, fills, resolved.invocable); + } catch (err) { + // compileSkill's own message already names verb + slot -- pass it through unchanged. + throw new SkillsUsageError((err as Error).message); + } } function writeCompiledVerb(outDir: string, result: CompileResult): void { @@ -213,53 +238,57 @@ function writeCompiledVerb(outDir: string, result: CompileResult): void { } export async function skillsCompile(args: string[]): Promise { - const flags = parseFlags(args); - const resolved = resolve(flags); - - for (const verb of resolved.roster) { - const result = compileVerb(verb, resolved); - const outDir = join(resolved.packDir, "skills", verb.name); + await withCleanErrors(async () => { + const flags = parseFlags(args); + const resolved = resolve(flags); + + for (const verb of resolved.roster) { + const result = compileVerb(verb, resolved); + const outDir = join(resolved.packDir, "skills", verb.name); + + if (flags.dryRun) { + console.log(`would write ${result.files.length} files for ${verb.name}`); + for (const warning of result.warnings) console.log(` ${warning}`); + continue; + } - if (flags.dryRun) { - console.log(`would write ${result.files.length} files for ${verb.name}`); + writeCompiledVerb(outDir, result); + console.log(`compiled ${verb.name} (${result.files.length} files, ${result.warnings.length} warnings)`); for (const warning of result.warnings) console.log(` ${warning}`); - continue; } - - writeCompiledVerb(outDir, result); - console.log(`compiled ${verb.name} (${result.files.length} files, ${result.warnings.length} warnings)`); - for (const warning of result.warnings) console.log(` ${warning}`); - } + }); } export async function skillsCheck(args: string[]): Promise { - const flags = parseFlags(args); - const resolved = resolve(flags); + await withCleanErrors(async () => { + const flags = parseFlags(args); + const resolved = resolve(flags); - let anyStale = false; + let anyStale = false; - for (const verb of resolved.roster) { - const outDir = join(resolved.packDir, "skills", verb.name); - if (!existsSync(outDir)) continue; + for (const verb of resolved.roster) { + const outDir = join(resolved.packDir, "skills", verb.name); + if (!existsSync(outDir)) continue; - const result = compileVerb(verb, resolved); - const staleFiles: string[] = []; + const result = compileVerb(verb, resolved); + const staleFiles: string[] = []; - for (const file of result.files) { - const dest = join(outDir, file.path); - const expected = "content" in file ? Buffer.from(file.content) : readFileSync(file.copyFrom); - if (!existsSync(dest) || !readFileSync(dest).equals(expected)) { - staleFiles.push(file.path); + for (const file of result.files) { + const dest = join(outDir, file.path); + const expected = "content" in file ? Buffer.from(file.content) : readFileSync(file.copyFrom); + if (!existsSync(dest) || !readFileSync(dest).equals(expected)) { + staleFiles.push(file.path); + } } - } - if (staleFiles.length > 0) { - anyStale = true; - console.log(`${verb.name}: stale (recompile or investigate drift with git diff) -- ${staleFiles.join(", ")}`); - } else { - console.log(`${verb.name}: current`); + if (staleFiles.length > 0) { + anyStale = true; + console.log(`${verb.name}: stale (recompile or investigate drift with git diff) -- ${staleFiles.join(", ")}`); + } else { + console.log(`${verb.name}: current`); + } } - } - if (anyStale) process.exitCode = 1; + if (anyStale) process.exitCode = 1; + }); } diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts index 1e47521f..53478fe2 100644 --- a/lib/skills/__tests__/sources.test.ts +++ b/lib/skills/__tests__/sources.test.ts @@ -1,8 +1,9 @@ -import { describe, test, expect, beforeEach } from "bun:test"; +import { describe, test, expect, beforeEach, spyOn } from "bun:test"; import { mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { join } from "path"; import { + buildPluginRoots, invocableRoster, loadAttachment, loadStepSource, @@ -311,3 +312,63 @@ describe("invocableRoster", () => { expect(roster.has("claimview:watch-ci-domain")).toBe(false); }); }); + +describe("buildPluginRoots", () => { + test("skips an entry whose installPath does not exist, warns once, and still resolves the rest", () => { + const rootDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-plugin-roots-"))); + + const mattstackDir = join(rootDir, "mattstack"); + writeFile(join(mattstackDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + + const claimviewDir = join(rootDir, "claimview"); + writeFile(join(claimviewDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "0.3.0" })); + + const staleInstallPath = join(rootDir, "current-time", "0.1.0"); + + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + let roots: PluginRoots; + let callCount: number; + let warning: string; + try { + roots = buildPluginRoots([ + { id: "mattstack@mattstack", installPath: mattstackDir }, + { id: "current-time@mattstack", installPath: staleInstallPath }, + { id: "claimview@assured", installPath: claimviewDir }, + ]); + // mockRestore() clears .mock.calls (bun, unlike jest), so read it before restoring. + callCount = errorSpy.mock.calls.length; + warning = errorSpy.mock.calls[0]?.join(" ") ?? ""; + } finally { + errorSpy.mockRestore(); + } + + expect(roots.byName.mattstack).toEqual({ dir: mattstackDir, version: "1.2.0" }); + expect(roots.byName.claimview).toEqual({ dir: claimviewDir, version: "0.3.0" }); + expect(roots.byName["current-time"]).toBeUndefined(); + + expect(callCount).toBe(1); + expect(warning).toContain("current-time"); + expect(warning).toContain(staleInstallPath); + }); + + test("no missing entries: every plugin resolves, no warning printed", () => { + const { roots: fixtureRoots } = makeFixtureRoots(); + + const errorSpy = spyOn(console, "error").mockImplementation(() => {}); + let roots: PluginRoots; + let callCount: number; + try { + roots = buildPluginRoots([ + { id: "mattstack@mattstack", installPath: fixtureRoots.byName.mattstack!.dir }, + { id: "claimview@assured", installPath: fixtureRoots.byName.claimview!.dir }, + ]); + callCount = errorSpy.mock.calls.length; + } finally { + errorSpy.mockRestore(); + } + + expect(roots.byName.mattstack?.version).toBe("1.2.0"); + expect(roots.byName.claimview?.version).toBe("0.3.0"); + expect(callCount).toBe(0); + }); +}); diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index 3e8cfef2..bf5ca80a 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -26,18 +26,26 @@ export function stripFrontmatter(md: string): { body: string; frontmatter: Recor export type PluginRoots = { byName: Record }; +export type PluginListEntry = { id: string; installPath: string }; + /** - * Thin by design: subprocess + filesystem reads, not unit-tested (see plan - * constraint). Task 5 exercises it live against real installed plugins. + * Entry-processing half of resolvePluginRoots, split out so it's testable + * without shelling out to `claude`. `claude plugin list --json` can list an + * entry whose installPath was since removed or moved (an uninstalled or + * relocated plugin the cache index hasn't caught up with yet) -- that must + * not take down resolution for every other plugin, so a missing installPath + * is skipped with a warning instead of throwing. */ -export function resolvePluginRoots(): PluginRoots { - const raw = execSync("claude plugin list --json", { encoding: "utf8" }); - const list = JSON.parse(raw) as Array<{ id: string; installPath: string }>; +export function buildPluginRoots(list: PluginListEntry[]): PluginRoots { const byName: PluginRoots["byName"] = {}; for (const entry of list) { const name = entry.id.split("@")[0]; if (!name) continue; + if (!existsSync(entry.installPath)) { + console.error(`rt: skipping plugin "${name}" -- installPath does not exist: ${entry.installPath}`); + continue; + } const dir = realpathSync(entry.installPath); let version = "unknown"; try { @@ -52,6 +60,17 @@ export function resolvePluginRoots(): PluginRoots { return { byName }; } +/** + * Thin by design: the subprocess call itself is not unit-tested (see plan + * constraint); buildPluginRoots carries the tested logic. Task 5 exercises + * this live against real installed plugins. + */ +export function resolvePluginRoots(): PluginRoots { + const raw = execSync("claude plugin list --json", { encoding: "utf8" }); + const list = JSON.parse(raw) as PluginListEntry[]; + return buildPluginRoots(list); +} + function parseSlots(raw: unknown): Record { if (!raw || typeof raw !== "object") return {}; const out: Record = {}; From 9b8106c3ece68024622f5d1467accf8d6305d97d Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 01:34:03 -0500 Subject: [PATCH 08/17] fix: omit allowed-tools frontmatter key when the union is empty A slot-free engine with no allowed-tools compiled to a bare 'allowed-tools:' line -- YAML null, not an empty list. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/compile.test.ts | 25 +++++++++++++++++++++++++ lib/skills/compile.ts | 8 +++++--- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/skills/__tests__/compile.test.ts b/lib/skills/__tests__/compile.test.ts index 991d40c8..84eabea5 100644 --- a/lib/skills/__tests__/compile.test.ts +++ b/lib/skills/__tests__/compile.test.ts @@ -285,6 +285,31 @@ describe("compileSkill", () => { ]); }); + test("empty allowed-tools union omits the key entirely", () => { + const bareStep: StepSource = { + name: "self-review", + plugin: "mattstack", + version: "1.2.0", + dir: "/plugins/mattstack/skills/review/self-review", + body: "Review your own diff before shipping.", + slots: {}, + allowedTools: [], + scriptFiles: [], + }; + const bareVerb: VerbDef = { + name: "self-review", + engine: "self-review", + description: "Review your own change", + }; + + const result = compileSkill(bareVerb, bareStep, {}, roster); + + expect(result.warnings).toEqual([]); + const content = skillFileContent(result.files); + expect(content).not.toContain("allowed-tools"); + expect(content).toContain('description: "Review your own change"\nmetadata:'); + }); + test("determinism: structurally equal inputs produce identical content", () => { const verbCopy: VerbDef = JSON.parse(JSON.stringify(verb)); const stepCopy: StepSource = JSON.parse(JSON.stringify(step)); diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts index 0a5c4316..b58c9faa 100644 --- a/lib/skills/compile.ts +++ b/lib/skills/compile.ts @@ -77,9 +77,11 @@ function buildFrontmatter(verb: VerbDef, allowedTools: string[], compiledParts: const lines: string[] = ["---"]; lines.push(`name: ${yamlQuote(verb.name)}`); lines.push(`description: ${yamlQuote(verb.description)}`); - lines.push("allowed-tools:"); - for (const tool of allowedTools) { - lines.push(` - ${yamlQuote(tool)}`); + if (allowedTools.length > 0) { + lines.push("allowed-tools:"); + for (const tool of allowedTools) { + lines.push(` - ${yamlQuote(tool)}`); + } } lines.push("metadata:"); lines.push(` compiled: ${yamlQuote(compiledParts.join(" + "))}`); From 6031df25b00c8b2372158613218b95b73189193a Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 09:25:50 -0500 Subject: [PATCH 09/17] feat: surface.jsonc enforcement in the skill compiler compileSkill gains opts.internalRoster: a body token or registered fill binding named in the internal roster is either flagged as an error (prose reference) or inlined despite registered=true (slot fill, covering the transition window before a file physically moves). skillsCompile skips non-public verbs (removing their compiled dir), aborts on compile errors, and verifies skills/ placement against surface.jsonc's public list after compiling -- reporting but never moving misplaced entries. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/skills.ts | 51 ++++- lib/skills/__tests__/surface.test.ts | 291 +++++++++++++++++++++++++++ lib/skills/compile.ts | 50 ++++- lib/skills/sources.ts | 12 ++ lib/skills/types.ts | 2 +- 5 files changed, 395 insertions(+), 11 deletions(-) create mode 100644 lib/skills/__tests__/surface.test.ts diff --git a/commands/skills.ts b/commands/skills.ts index 5233ffc8..3642cd75 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -25,9 +25,11 @@ import { loadAttachment, loadStepSource, readManifestBindings, + readSurface, readVerbRoster, resolvePluginRoots, type PluginRoots, + type SurfaceConfig, } from "../lib/skills/sources.ts"; import type { AttachmentSource, CompileResult, VerbDef } from "../lib/skills/types.ts"; @@ -175,8 +177,27 @@ type Resolved = { bindings: Record>; pluginRoots: PluginRoots; invocable: Set; + surface: SurfaceConfig | null; + internalRoster: Set; }; +/** + * Internal roster tokens mirror invocableRoster's ":" shape so + * they line up with body-prose tokens and fill bindings. A dir under + * /skills/ not (yet) named in surface.jsonc's public list is + * internal by default -- this is what lets a fill inline through the + * transition window before it physically moves. + */ +function computeInternalRoster(team: string, packDir: string, surface: SurfaceConfig | null): Set { + const internal = new Set(); + if (!surface) return internal; + const publicSet = new Set(surface.public); + for (const name of listSubdirs(join(packDir, "skills"))) { + if (!publicSet.has(name)) internal.add(`${team}:${name}`); + } + return internal; +} + function resolve(flags: Flags): Resolved { const mattstackRoot = flags.mattstackDir ?? mattstackHome(); const packDir = flags.packDir ?? packRootDir(mattstackRoot, flags.team); @@ -186,8 +207,10 @@ function resolve(flags: Flags): Resolved { const bindings = readManifestBindings(manifestPath); const pluginRoots = flags.mattstackDir ? resolvePluginRootsFromDir(mattstackRoot) : resolvePluginRoots(); const invocable = invocableRoster(pluginRoots); + const surface = readSurface(packDir); + const internalRoster = computeInternalRoster(flags.team, packDir, surface); - return { packDir, roster, bindings, pluginRoots, invocable }; + return { packDir, roster, bindings, pluginRoots, invocable, surface, internalRoster }; } function compileVerb(verb: VerbDef, resolved: Resolved): CompileResult { @@ -214,7 +237,7 @@ function compileVerb(verb: VerbDef, resolved: Resolved): CompileResult { } try { - return compileSkill(verb, step, fills, resolved.invocable); + return compileSkill(verb, step, fills, resolved.invocable, { internalRoster: resolved.internalRoster }); } catch (err) { // compileSkill's own message already names verb + slot -- pass it through unchanged. throw new SkillsUsageError((err as Error).message); @@ -241,11 +264,24 @@ export async function skillsCompile(args: string[]): Promise { await withCleanErrors(async () => { const flags = parseFlags(args); const resolved = resolve(flags); + const publicSet = resolved.surface ? new Set(resolved.surface.public) : null; for (const verb of resolved.roster) { - const result = compileVerb(verb, resolved); const outDir = join(resolved.packDir, "skills", verb.name); + if (publicSet && !publicSet.has(verb.name)) { + console.log(`internal: ${verb.name} (not compiled; roster entry retired)`); + if (!flags.dryRun && existsSync(outDir)) { + rmSync(outDir, { recursive: true, force: true }); + } + continue; + } + + const result = compileVerb(verb, resolved); + if (result.errors.length > 0) { + throw new SkillsUsageError(`verb "${verb.name}": ${result.errors.join("; ")}`); + } + if (flags.dryRun) { console.log(`would write ${result.files.length} files for ${verb.name}`); for (const warning of result.warnings) console.log(` ${warning}`); @@ -256,6 +292,15 @@ export async function skillsCompile(args: string[]): Promise { console.log(`compiled ${verb.name} (${result.files.length} files, ${result.warnings.length} warnings)`); for (const warning of result.warnings) console.log(` ${warning}`); } + + if (publicSet) { + for (const name of listSubdirs(join(resolved.packDir, "skills"))) { + if (!publicSet.has(name)) { + console.log(`misplaced: ${name} (run rt skills surface apply, or move it)`); + process.exitCode = 1; + } + } + } }); } diff --git a/lib/skills/__tests__/surface.test.ts b/lib/skills/__tests__/surface.test.ts new file mode 100644 index 00000000..7bdc37dd --- /dev/null +++ b/lib/skills/__tests__/surface.test.ts @@ -0,0 +1,291 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import { existsSync, mkdirSync, mkdtempSync, realpathSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { dirname, join } from "path"; +import { skillsCompile } from "../../../commands/skills.ts"; +import { compileSkill } from "../compile.ts"; +import { readSurface } from "../sources.ts"; +import type { AttachmentSource, StepSource, VerbDef } from "../types.ts"; + +function writeFile(path: string, content: string): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content); +} + +describe("readSurface", () => { + test("parses a comment-bearing surface.jsonc", () => { + const packDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-pack-"))); + const surfaceJsonc = `// surface.jsonc -- names this pack's public skills/ directories. +{ + "public": [ + "watch-ci", + // hand-authored skill, not a compiled verb + "cvi-gates" + ] +} +`; + writeFile(join(packDir, "pack", "surface.jsonc"), surfaceJsonc); + + const surface = readSurface(packDir); + + expect(surface).toEqual({ public: ["watch-ci", "cvi-gates"] }); + }); + + test("returns null when surface.jsonc is absent", () => { + const packDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-pack-"))); + + expect(readSurface(packDir)).toBeNull(); + }); +}); + +describe("compileSkill with internalRoster", () => { + const verb: VerbDef = { name: "watch-ci", engine: "watch-ci-engine", description: "Watch CI" }; + const roster = new Set(["mattstack:watch-ci", "claimview:watch-ci-domain"]); + + test("a body token naming an internal skill produces an errors entry with fix options; a public token does not", () => { + const step: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.0.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "This step defers to claimview:cvi-gates for the internal check and to claimview:watch-ci-domain for the public one.", + slots: {}, + allowedTools: [], + scriptFiles: [], + }; + const internalRoster = new Set(["claimview:cvi-gates"]); + + const result = compileSkill(verb, step, {}, roster, { internalRoster }); + + expect(result.errors).toEqual([ + "body references claimview:cvi-gates which is surface-internal; inline it, reference it by path, or list it in surface.jsonc's public array", + ]); + }); + + test("no internalRoster entries: errors is empty", () => { + const step: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.0.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "This step defers to claimview:watch-ci-domain.", + slots: {}, + allowedTools: [], + scriptFiles: [], + }; + + const result = compileSkill(verb, step, {}, roster); + + expect(result.errors).toEqual([]); + }); + + test("a fill whose binding is internal-listed inlines despite registered=true, with a printed note", () => { + const stepWithSlot: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.0.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "Poll CI.", + slots: { domain: { contract: "watch-ci-domain@1", required: true } }, + allowedTools: [], + scriptFiles: [], + }; + const registeredInternalFill: AttachmentSource = { + binding: "claimview:cvi-gates", + plugin: "claimview", + version: "0.3.0", + dir: "/plugins/claimview/skills/cvi-gates", + body: "Domain rules inlined from cvi-gates.", + provides: "watch-ci-domain@1", + allowedTools: [], + extraFiles: [], + registered: true, + }; + const internalRoster = new Set(["claimview:cvi-gates"]); + + const result = compileSkill(verb, stepWithSlot, { domain: registeredInternalFill }, roster, { + internalRoster, + }); + + const skillFile = result.files[0]; + if (!skillFile || !("content" in skillFile)) throw new Error("expected files[0] to have content"); + + expect(skillFile.content).toContain("Domain rules inlined from cvi-gates."); + expect(skillFile.content).not.toContain("invoke that skill when this flow needs it"); + expect(result.warnings).toContain("note: claimview:cvi-gates is surface-internal; inlined"); + }); + + test("a fill whose binding is NOT internal-listed still references, not inlines, when registered=true", () => { + const stepWithSlot: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.0.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "Poll CI.", + slots: { domain: { contract: "watch-ci-domain@1", required: true } }, + allowedTools: [], + scriptFiles: [], + }; + const registeredPublicFill: AttachmentSource = { + binding: "claimview:cvi-gates", + plugin: "claimview", + version: "0.3.0", + dir: "/plugins/claimview/skills/cvi-gates", + body: "Domain rules for cvi-gates.", + provides: "watch-ci-domain@1", + allowedTools: [], + extraFiles: [], + registered: true, + }; + + const result = compileSkill(verb, stepWithSlot, { domain: registeredPublicFill }, roster, { + internalRoster: new Set(), + }); + + const skillFile = result.files[0]; + if (!skillFile || !("content" in skillFile)) throw new Error("expected files[0] to have content"); + + expect(skillFile.content).toContain("invoke that skill when this flow needs it"); + expect(skillFile.content).not.toContain("Domain rules for cvi-gates."); + expect(result.warnings).not.toContain("note: claimview:cvi-gates is surface-internal; inlined"); + }); +}); + +const WATCH_CI_SKILL_MD = `--- +name: watch-ci +description: "Watch CI until it goes green" +type: pipeline-step +--- + +Poll the pipeline every 30s and report status. +`; + +const OLD_VERB_SKILL_MD = `--- +name: old-verb +description: "A retired verb" +type: pipeline-step +--- + +This verb is retired. +`; + +const STRAY_SKILL_MD = `--- +name: stray-skill +description: "Hand-authored skill not yet declared public" +--- + +Stray content. +`; + +function makeMattstackDir(): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-mattstack-"))); + const mattstackPluginDir = join(dir, "plugins", "mattstack"); + writeFile(join(mattstackPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "watch-ci", "SKILL.md"), WATCH_CI_SKILL_MD); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "old-verb", "SKILL.md"), OLD_VERB_SKILL_MD); + return dir; +} + +function makePackDir(stubsJsonc: string, surfaceJsonc: string | null): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-pack-"))); + writeFile(join(dir, "pack", "stubs.jsonc"), stubsJsonc); + if (surfaceJsonc !== null) { + writeFile(join(dir, "pack", "surface.jsonc"), surfaceJsonc); + } + return dir; +} + +function makeManifest(team: string): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-manifest-"))); + const path = join(dir, "skills.jsonc"); + writeFile( + path, + `// GENERATED -- provenance: ${team}@${team}\n{\n "bindings": {}\n}\n`, + ); + return path; +} + +const STUBS_TWO_VERBS = `{ + "verbs": { + "watch-ci": { "engine": "watch-ci", "description": "Watch CI." }, + "old-verb": { "engine": "old-verb", "description": "Retired." } + } +} +`; + +let logSpy: ReturnType; +let logs: string[]; + +beforeEach(() => { + logs = []; + logSpy = spyOn(console, "log").mockImplementation((...args: unknown[]) => { + logs.push(args.map(String).join(" ")); + }); +}); + +afterEach(() => { + logSpy.mockRestore(); + // Bun's process.exitCode setter ignores undefined once set truthy -- 0 is + // the only value that actually clears it between tests in this file. + process.exitCode = 0; +}); + +describe("skillsCompile with a surface config", () => { + test("skips a non-public verb, prints an internal line, and removes its compiled skills/ dir", async () => { + const mattstackDir = makeMattstackDir(); + const surfaceJsonc = `{ "public": ["watch-ci"] }\n`; + const packDir = makePackDir(STUBS_TWO_VERBS, surfaceJsonc); + writeFile(join(packDir, "skills", "old-verb", "SKILL.md"), "stale compiled output\n"); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + ]); + + expect(logs).toContain("internal: old-verb (not compiled; roster entry retired)"); + expect(existsSync(join(packDir, "skills", "old-verb"))).toBe(false); + expect(existsSync(join(packDir, "skills", "watch-ci", "SKILL.md"))).toBe(true); + }); + + test("placement verification flags a non-public dir left under skills/ and sets a nonzero exit", async () => { + const mattstackDir = makeMattstackDir(); + const surfaceJsonc = `{ "public": ["watch-ci"] }\n`; + const packDir = makePackDir(STUBS_TWO_VERBS, surfaceJsonc); + writeFile(join(packDir, "skills", "stray-skill", "SKILL.md"), STRAY_SKILL_MD); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + ]); + + expect(logs).toContain("misplaced: stray-skill (run rt skills surface apply, or move it)"); + expect(process.exitCode).toBe(1); + // it never moves anything + expect(existsSync(join(packDir, "skills", "stray-skill", "SKILL.md"))).toBe(true); + }); + + test("no surface.jsonc present: all verbs compile, no internal/misplaced lines, no exit code", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(STUBS_TWO_VERBS, null); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + ]); + + expect(logs.some((l) => l.startsWith("internal:"))).toBe(false); + expect(logs.some((l) => l.startsWith("misplaced:"))).toBe(false); + expect(existsSync(join(packDir, "skills", "watch-ci", "SKILL.md"))).toBe(true); + expect(existsSync(join(packDir, "skills", "old-verb", "SKILL.md"))).toBe(true); + expect(process.exitCode).not.toBe(1); + }); +}); diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts index b58c9faa..b7e76f78 100644 --- a/lib/skills/compile.ts +++ b/lib/skills/compile.ts @@ -89,25 +89,39 @@ function buildFrontmatter(verb: VerbDef, allowedTools: string[], compiledParts: return lines.join("\n"); } -function buildBody(step: StepSource, boundSlots: BoundSlot[]): string { +function buildBody( + step: StepSource, + boundSlots: BoundSlot[], + internalRoster: Set, +): { body: string; notes: string[] } { const sections: string[] = [HEADER_COMMENT]; + const notes: string[] = []; sections.push(``); sections.push(step.body); for (const { slotName, fill } of boundSlots) { - if (fill.registered) { - // Registered skills stay singly-canonical: reference, never inline. + const internal = internalRoster.has(fill.binding); + + if (fill.registered && !internal) { + // Registered, surface-public skills stay singly-canonical: reference, never inline. sections.push( `Slot ${slotName} is bound to \`${fill.binding}\` (${fill.binding}@${fill.version}) -- invoke that skill when this flow needs it.`, ); continue; } + + if (fill.registered && internal) { + // Transition window: the file already lives under skills/ but surface.jsonc + // has not declared it public yet -- inline so the compiled output stays correct. + notes.push(`note: ${fill.binding} is surface-internal; inlined`); + } + sections.push(``); sections.push(rewriteSkillDirRefs(fill.body, slotName)); } - return sections.join("\n\n"); + return { body: sections.join("\n\n"), notes }; } function buildVendoredFiles(step: StepSource, boundSlots: BoundSlot[]): CompiledFile[] { @@ -170,12 +184,33 @@ function lintReferences(body: string, roster: Set, files: CompiledFile[] return warnings; } +function lintInternalRoster(body: string, internalRoster: Set): string[] { + const errors: string[] = []; + const lintableBody = stripCompilerComments(body); + + const seenNames = new Set(); + for (const match of lintableBody.matchAll(REGISTERED_NAME_RE)) { + const token = match[0]; + if (seenNames.has(token)) continue; + seenNames.add(token); + if (internalRoster.has(token)) { + errors.push( + `body references ${token} which is surface-internal; inline it, reference it by path, or list it in surface.jsonc's public array`, + ); + } + } + + return errors; +} + export function compileSkill( verb: VerbDef, step: StepSource, fills: Record, roster: Set, + opts?: { internalRoster?: Set }, ): CompileResult { + const internalRoster = opts?.internalRoster ?? new Set(); const boundSlots = resolveBoundSlots(verb, step, fills); const allowedTools = buildAllowedTools(step, boundSlots); @@ -184,13 +219,14 @@ export function compileSkill( ...boundSlots.map(({ fill }) => `${fill.binding}@${fill.version}`), ]; - const body = buildBody(step, boundSlots); + const { body, notes } = buildBody(step, boundSlots, internalRoster); const frontmatter = buildFrontmatter(verb, allowedTools, compiledParts); const content = `${frontmatter}\n\n${body}\n`; const files: CompiledFile[] = [{ path: "SKILL.md", content }, ...buildVendoredFiles(step, boundSlots)]; - const warnings = lintReferences(body, roster, files); + const warnings = [...lintReferences(body, roster, files), ...notes]; + const errors = lintInternalRoster(body, internalRoster); - return { files, warnings }; + return { files, warnings, errors }; } diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index bf5ca80a..9ad598f4 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -266,6 +266,18 @@ export function readManifestBindings(manifestPath: string): Record typeof entry === "string") + : []; + return { public: publicList }; +} + export function invocableRoster(roots: PluginRoots): Set { const roster = new Set(); diff --git a/lib/skills/types.ts b/lib/skills/types.ts index 4b7bfe41..55420465 100644 --- a/lib/skills/types.ts +++ b/lib/skills/types.ts @@ -27,4 +27,4 @@ export type VerbDef = { name: string; engine: string; description: string }; export type CompiledFile = { path: string; content: string } | { path: string; copyFrom: string }; -export type CompileResult = { files: CompiledFile[]; warnings: string[] }; +export type CompileResult = { files: CompiledFile[]; warnings: string[]; errors: string[] }; From 59e33620766ba859bc90c23ae263a52679cbadf6 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 09:47:45 -0500 Subject: [PATCH 10/17] fix: guard verb names against path breakout; add surface integration coverage readVerbRoster now rejects stubs.jsonc verb keys containing "/", "\\", or ".." before they can flow into join()+rmSync at the two destructive call sites (writeCompiledVerb, the internal-verb skip). Also adds an end-to-end skillsCompile test where the pack dir and the resolved plugin root are the same fixture tree, exercising computeInternalRoster against real loadAttachment resolution (inline + misplaced flag) and routing an internal name-reference through the errors-abort path. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/sources.test.ts | 16 +++ lib/skills/__tests__/surface.test.ts | 149 ++++++++++++++++++++++++++- lib/skills/sources.ts | 23 ++++- 3 files changed, 182 insertions(+), 6 deletions(-) diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts index 53478fe2..fc077c59 100644 --- a/lib/skills/__tests__/sources.test.ts +++ b/lib/skills/__tests__/sources.test.ts @@ -262,6 +262,22 @@ describe("readVerbRoster", () => { { name: "ship", engine: "ship", description: "Use when ready to ship." }, ]); }); + + test("throws naming the offending key when a verb name is a path breakout", () => { + const rootDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-pack-"))); + const stubsJsonc = `{ + "verbs": { + "../evil": { + "engine": "watch-ci", + "description": "Use when watching or triaging CI." + } + } +} +`; + writeFile(join(rootDir, "pack", "stubs.jsonc"), stubsJsonc); + + expect(() => readVerbRoster(rootDir)).toThrow(/"\.\.\/evil"/); + }); }); describe("readManifestBindings", () => { diff --git a/lib/skills/__tests__/surface.test.ts b/lib/skills/__tests__/surface.test.ts index 7bdc37dd..21edce60 100644 --- a/lib/skills/__tests__/surface.test.ts +++ b/lib/skills/__tests__/surface.test.ts @@ -1,5 +1,5 @@ import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; -import { existsSync, mkdirSync, mkdtempSync, realpathSync, writeFileSync } from "fs"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { dirname, join } from "path"; import { skillsCompile } from "../../../commands/skills.ts"; @@ -289,3 +289,150 @@ describe("skillsCompile with a surface config", () => { expect(process.exitCode).not.toBe(1); }); }); + +/** + * computeInternalRoster reads /skills/ while loadAttachment resolves + * ":X" bindings against the plugin root claude resolves for that team -- + * in production those are the same physical tree (the pack IS the installed + * plugin). The tests above never exercise that: makeMattstackDir/makePackDir + * keep the pack dir and the team's plugin dir separate. These fixtures make + * plugins/claimview/ BOTH --pack-dir and the resolved "claimview" plugin + * root, so a real end-to-end skillsCompile run is exercised. + */ +const CVI_GATES_SKILL_MD = `--- +name: cvi-gates +description: "Domain gating rules" +metadata: + provides: "watch-ci-domain@1" +--- + +Domain rules inlined from cvi-gates for the transition window. +`; + +const WATCH_CI_WITH_SLOT_SKILL_MD = `--- +name: watch-ci +description: "Watch CI until it goes green" +type: pipeline-step +slots: + domain: { contract: "watch-ci-domain@1", required: true } +--- + +Poll CI. +`; + +const GATE_CHECK_SKILL_MD = `--- +name: gate-check +description: "Gate check" +type: pipeline-step +--- + +This step defers to claimview:cvi-gates for domain judgment. +`; + +function makeManifestAt(bindingsJson: string): string { + const dir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-int-manifest-"))); + const path = join(dir, "skills.jsonc"); + writeFile(path, `// GENERATED -- provenance: claimview@claimview\n{ "bindings": ${bindingsJson} }\n`); + return path; +} + +async function runExpectingCleanExit( + fn: () => Promise, +): Promise<{ exitCode: number | undefined; errors: string[] }> { + const errors: string[] = []; + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit sentinel"); + }); + const errorSpy = spyOn(console, "error").mockImplementation((...args: unknown[]) => { + errors.push(args.map(String).join(" ")); + }); + try { + await fn(); + return { exitCode: undefined, errors }; + } catch { + const exitCode = exitSpy.mock.calls.at(-1)?.[0] as number | undefined; + return { exitCode, errors }; + } finally { + exitSpy.mockRestore(); + errorSpy.mockRestore(); + } +} + +describe("computeInternalRoster integration (pack dir doubles as plugin root)", () => { + test("a registered-but-not-yet-public fill inlines end to end, notes, and is flagged misplaced", async () => { + const mattstackDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-int-"))); + + const claimviewDir = join(mattstackDir, "plugins", "claimview"); + writeFile(join(claimviewDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "0.3.0" })); + writeFile( + join(claimviewDir, "pack", "stubs.jsonc"), + `{ "verbs": { "watch-ci": { "engine": "watch-ci", "description": "Watch CI." } } }\n`, + ); + writeFile(join(claimviewDir, "pack", "surface.jsonc"), `{ "public": ["watch-ci"] }\n`); + writeFile(join(claimviewDir, "skills", "cvi-gates", "SKILL.md"), CVI_GATES_SKILL_MD); + + const mattstackPluginDir = join(mattstackDir, "plugins", "mattstack"); + writeFile(join(mattstackPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + writeFile( + join(mattstackPluginDir, "skills", "pipeline", "watch-ci", "SKILL.md"), + WATCH_CI_WITH_SLOT_SKILL_MD, + ); + + const manifestPath = makeManifestAt('{ "mattstack:watch-ci": { "domain": "claimview:cvi-gates" } }'); + + await skillsCompile([ + "--team", "claimview", + "--pack-dir", claimviewDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + const content = readFileSync(join(claimviewDir, "skills", "watch-ci", "SKILL.md"), "utf8"); + expect(content).toContain("Domain rules inlined from cvi-gates for the transition window."); + expect(content).not.toContain("invoke that skill when this flow needs it"); + + expect(logs).toContain(" note: claimview:cvi-gates is surface-internal; inlined"); + // cvi-gates is still physically under skills/ and isn't in surface.public -- + // it compiles (inlined) AND is flagged for the move surface apply would do. + expect(logs).toContain("misplaced: cvi-gates (run rt skills surface apply, or move it)"); + expect(process.exitCode).toBe(1); + }); + + test("a body reference to an internal skill aborts that verb via the errors channel, cleanly, with no partial write", async () => { + const mattstackDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-int-"))); + + const claimviewDir = join(mattstackDir, "plugins", "claimview"); + writeFile(join(claimviewDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "0.3.0" })); + writeFile( + join(claimviewDir, "pack", "stubs.jsonc"), + `{ "verbs": { "gate-check": { "engine": "gate-check", "description": "Gate check." } } }\n`, + ); + writeFile(join(claimviewDir, "pack", "surface.jsonc"), `{ "public": ["gate-check"] }\n`); + writeFile(join(claimviewDir, "skills", "cvi-gates", "SKILL.md"), CVI_GATES_SKILL_MD); + + const mattstackPluginDir = join(mattstackDir, "plugins", "mattstack"); + writeFile(join(mattstackPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "gate-check", "SKILL.md"), GATE_CHECK_SKILL_MD); + + const manifestPath = makeManifestAt("{}"); + + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsCompile([ + "--team", "claimview", + "--pack-dir", claimviewDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "gate-check", + ]), + ); + + expect(exitCode).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("gate-check"); + expect(errors[0]).toContain("claimview:cvi-gates"); + expect(errors[0]).toContain("surface-internal"); + expect(existsSync(join(claimviewDir, "skills", "gate-check"))).toBe(false); + }); +}); diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index 9ad598f4..fc4d93b9 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -246,17 +246,30 @@ export function loadAttachment(binding: string, slot: string, roots: PluginRoots }; } +/** + * Verb names flow unsanitized into join(packDir, "skills", name) at two + * destructive call sites (writeCompiledVerb's rmSync + the internal-verb + * skip's rmSync in commands/skills.ts) -- reject path-breakout characters + * here, at the one place every verb name is read from disk. + */ +function assertSafeVerbName(name: string, stubsPath: string): void { + if (name.includes("/") || name.includes("\\") || name.includes("..")) { + throw new Error( + `readVerbRoster: verb key "${name}" in ${stubsPath} is not a safe directory name (must not contain "/", "\\", or "..")`, + ); + } +} + export function readVerbRoster(packDir: string): VerbDef[] { const stubsPath = join(packDir, "pack", "stubs.jsonc"); const parsed = JSON.parse(stripJsonc(readFileSync(stubsPath, "utf8"))) as { verbs?: Record; }; const verbs = parsed.verbs ?? {}; - return Object.entries(verbs).map(([name, def]) => ({ - name, - engine: def.engine, - description: def.description, - })); + return Object.entries(verbs).map(([name, def]) => { + assertSafeVerbName(name, stubsPath); + return { name, engine: def.engine, description: def.description }; + }); } export function readManifestBindings(manifestPath: string): Record> { From 0000275cc2f1181936a9f32066372d5bfd95a644 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 10:00:04 -0500 Subject: [PATCH 11/17] feat: rt skills surface -- list, set, apply, fzf palette Adds skillsSurface (list/set/apply/bare fzf palette) on top of the Task 1 surface.jsonc enforcement: set bootstraps surface.jsonc from the current registered set on first use; apply git-mv's hand-authored skills between skills/ and attachments/ (plain rename when the pack dir isn't a git repo) and delegates recompilation to the existing skillsCompile flow -- compiled verb dirs are never git-mv'd. Wires `rt skills surface` into the command tree; commands/skills.ts was already in the module registry. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/__tests__/skills-surface.test.ts | 323 +++++++++++++++++++++ commands/skills.ts | 327 +++++++++++++++++++++- lib/command-tree-def.ts | 10 + lib/skills/compile.ts | 3 +- 4 files changed, 660 insertions(+), 3 deletions(-) create mode 100644 commands/__tests__/skills-surface.test.ts diff --git a/commands/__tests__/skills-surface.test.ts b/commands/__tests__/skills-surface.test.ts new file mode 100644 index 00000000..bb55a005 --- /dev/null +++ b/commands/__tests__/skills-surface.test.ts @@ -0,0 +1,323 @@ +import { afterEach, beforeEach, describe, expect, spyOn, test } from "bun:test"; +import { execFileSync } from "child_process"; +import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { dirname, join } from "path"; +import { skillsSurface } from "../skills.ts"; + +function writeFile(path: string, content: string): void { + mkdirSync(dirname(path), { recursive: true }); + writeFileSync(path, content); +} + +function makePackDir(): string { + return realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-pack-"))); +} + +function writeStubs(packDir: string, verbs: Record): void { + writeFile(join(packDir, "pack", "stubs.jsonc"), JSON.stringify({ verbs })); +} + +/** Trivial no-slot pipeline-step engine + fixture mattstack root, for apply's compile delegation. */ +function makeEngineFixture(): { mattstackDir: string; manifestPath: string } { + const mattstackDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-mattstack-"))); + writeFile( + join(mattstackDir, "plugins", "mattstack", ".claude-plugin", "plugin.json"), + JSON.stringify({ version: "1.0.0" }), + ); + writeFile( + join(mattstackDir, "plugins", "mattstack", "skills", "pipeline", "my-verb", "SKILL.md"), + `---\nname: my-verb\ndescription: "Do the thing"\ntype: pipeline-step\n---\n\nDo the thing.\n`, + ); + + const manifestDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-manifest-"))); + const manifestPath = join(manifestDir, "skills.jsonc"); + writeFile(manifestPath, JSON.stringify({ bindings: {} })); + + return { mattstackDir, manifestPath }; +} + +let logSpy: ReturnType; +let logs: string[]; + +beforeEach(() => { + logs = []; + logSpy = spyOn(console, "log").mockImplementation((...args: unknown[]) => { + logs.push(args.map(String).join(" ")); + }); +}); + +afterEach(() => { + logSpy.mockRestore(); + process.exitCode = undefined; +}); + +async function runExpectingCleanExit(fn: () => Promise): Promise<{ exitCode: number | undefined; errors: string[] }> { + const errors: string[] = []; + const exitSpy = spyOn(process, "exit").mockImplementation(() => { + throw new Error("process.exit sentinel"); + }); + const errorSpy = spyOn(console, "error").mockImplementation((...args: unknown[]) => { + errors.push(args.map(String).join(" ")); + }); + try { + await fn(); + return { exitCode: undefined, errors }; + } catch { + const exitCode = exitSpy.mock.calls.at(-1)?.[0] as number | undefined; + return { exitCode, errors }; + } finally { + exitSpy.mockRestore(); + errorSpy.mockRestore(); + } +} + +describe("skillsSurface list", () => { + test("no surface.jsonc: infers public/internal from current skills/ + attachments/ + stub verbs", async () => { + const packDir = makePackDir(); + writeStubs(packDir, { "my-verb": { engine: "my-verb", description: "Do the thing" } }); + writeFile(join(packDir, "skills", "hand-authored-public", "SKILL.md"), "---\nname: x\n---\nbody\n"); + writeFile(join(packDir, "attachments", "hand-authored-internal", "SKILL.md"), "---\nname: y\n---\nbody\n"); + + await skillsSurface(["list", "--team", "t", "--pack-dir", packDir]); + + const out = logs.join("\n"); + expect(out).toContain("no surface.jsonc"); + expect(out).toMatch(/public.*hand-authored-public/); + expect(out).toMatch(/internal.*hand-authored-internal/); + expect(out).toMatch(/public.*compiled.*my-verb/); + }); + + test("with surface.jsonc: statuses reflect the config, not disk placement", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "skills", "still-under-skills", "SKILL.md"), "---\nname: z\n---\nbody\n"); + writeFile(join(packDir, "pack", "surface.jsonc"), JSON.stringify({ public: [] })); + + await skillsSurface(["list", "--team", "t", "--pack-dir", packDir]); + + const out = logs.join("\n"); + expect(out).toContain("pack/surface.jsonc"); + expect(out).toMatch(/internal.*still-under-skills/); + }); + + test("unrecognized argument: clean one-line error, exit 1", async () => { + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsSurface(["list", "--bogus"]), + ); + expect(exitCode).toBe(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("--bogus"); + }); +}); + +describe("skillsSurface set", () => { + test("--public on an attachments/ dir: bootstraps surface.jsonc, moves it to skills/ via apply (plain rename, no git repo)", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "attachments", "my-attach", "SKILL.md"), "---\nname: a\n---\nbody\n"); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "set", "my-attach", "--public", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + const surfacePath = join(packDir, "pack", "surface.jsonc"); + expect(existsSync(surfacePath)).toBe(true); + const surface = JSON.parse(readFileSync(surfacePath, "utf8").replace(/^\/\/.*\n/, "")); + expect(surface.public).toContain("my-attach"); + + expect(existsSync(join(packDir, "skills", "my-attach", "SKILL.md"))).toBe(true); + expect(existsSync(join(packDir, "attachments", "my-attach"))).toBe(false); + + const out = logs.join("\n"); + expect(out).toContain("not a git repo"); + }); + + test("--internal on a skills/ dir: bootstraps surface.jsonc, moves it to attachments/ via apply", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "skills", "my-skill", "SKILL.md"), "---\nname: s\n---\nbody\n"); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "set", "my-skill", "--internal", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + expect(existsSync(join(packDir, "attachments", "my-skill", "SKILL.md"))).toBe(true); + expect(existsSync(join(packDir, "skills", "my-skill"))).toBe(false); + + const surfacePath = join(packDir, "pack", "surface.jsonc"); + const surface = JSON.parse(readFileSync(surfacePath, "utf8").replace(/^\/\/.*\n/, "")); + expect(surface.public).not.toContain("my-skill"); + }); + + test("--internal on a compiled stub verb: surface.jsonc updated, apply removes the compiled dir (never git-mv'd)", async () => { + const packDir = makePackDir(); + writeStubs(packDir, { "my-verb": { engine: "my-verb", description: "Do the thing" } }); + writeFile(join(packDir, "skills", "my-verb", "SKILL.md"), "---\nname: my-verb\n---\nold compiled content\n"); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "set", "my-verb", "--internal", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + expect(existsSync(join(packDir, "skills", "my-verb"))).toBe(false); + expect(existsSync(join(packDir, "attachments", "my-verb"))).toBe(false); + + const out = logs.join("\n"); + expect(out).not.toContain("moved my-verb"); + }); + + test("--public on a compiled stub verb (bootstrap default already public): apply compiles it", async () => { + const packDir = makePackDir(); + writeStubs(packDir, { "my-verb": { engine: "my-verb", description: "Do the thing" } }); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "set", "my-verb", "--public", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + const skillMd = readFileSync(join(packDir, "skills", "my-verb", "SKILL.md"), "utf8"); + expect(skillMd).toContain("compiled by rt skills compile"); + }); + + test("uses a real git mv when the pack dir is a git repo", async () => { + const packDir = makePackDir(); + execFileSync("git", ["init", "-q"], { cwd: packDir }); + execFileSync("git", ["config", "user.email", "t@example.com"], { cwd: packDir }); + execFileSync("git", ["config", "user.name", "t"], { cwd: packDir }); + writeStubs(packDir, {}); + writeFile(join(packDir, "skills", "my-skill", "SKILL.md"), "---\nname: s\n---\nbody\n"); + execFileSync("git", ["add", "-A"], { cwd: packDir }); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "set", "my-skill", "--internal", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + expect(existsSync(join(packDir, "attachments", "my-skill", "SKILL.md"))).toBe(true); + expect(existsSync(join(packDir, "skills", "my-skill"))).toBe(false); + // git mv on a never-committed add reports as a plain staged add at the new + // path (git has no prior commit to diff a rename against). + const status = execFileSync("git", ["status", "--porcelain"], { cwd: packDir, encoding: "utf8" }); + expect(status).toContain("attachments/my-skill"); + + const out = logs.join("\n"); + expect(out).not.toContain("not a git repo"); + }); + + test("unknown name: clean one-line error, exit 1", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsSurface(["set", "no-such-skill", "--public", "--pack-dir", packDir]), + ); + + expect(exitCode).toBe(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("no-such-skill"); + }); + + test("missing --public/--internal: clean one-line error, exit 1", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "skills", "my-skill", "SKILL.md"), "---\nname: s\n---\nbody\n"); + + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsSurface(["set", "my-skill", "--pack-dir", packDir]), + ); + + expect(exitCode).toBe(1); + expect(errors[0]).toStartWith("rt skills: "); + }); + + test("missing name: clean one-line error, exit 1", async () => { + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsSurface(["set", "--public"]), + ); + + expect(exitCode).toBe(1); + expect(errors[0]).toStartWith("rt skills: "); + }); +}); + +describe("skillsSurface apply", () => { + test("dry-run prints planned moves and touches nothing", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "attachments", "my-attach", "SKILL.md"), "---\nname: a\n---\nbody\n"); + writeFile(join(packDir, "pack", "surface.jsonc"), JSON.stringify({ public: ["my-attach"] })); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "apply", "--dry-run", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + expect(existsSync(join(packDir, "attachments", "my-attach"))).toBe(true); + expect(existsSync(join(packDir, "skills", "my-attach"))).toBe(false); + expect(logs.some((l) => l.includes("would move") && l.includes("my-attach"))).toBe(true); + }); + + test("no surface.jsonc: no moves, compiles the roster as usual", async () => { + const packDir = makePackDir(); + writeStubs(packDir, { "my-verb": { engine: "my-verb", description: "Do the thing" } }); + const { mattstackDir, manifestPath } = makeEngineFixture(); + + await skillsSurface([ + "apply", + "--team", "t", "--pack-dir", packDir, "--mattstack-dir", mattstackDir, "--manifest", manifestPath, + ]); + + expect(logs.some((l) => l.includes("no moves needed"))).toBe(true); + expect(existsSync(join(packDir, "skills", "my-verb", "SKILL.md"))).toBe(true); + }); + + test("unrecognized argument: clean one-line error, exit 1", async () => { + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsSurface(["apply", "--bogus"]), + ); + expect(exitCode).toBe(1); + expect(errors[0]).toStartWith("rt skills: "); + }); +}); + +describe("skillsSurface bare invocation (fzf palette)", () => { + test("non-tty: prints the list and the set-command hint, does not crash or write config", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "skills", "my-skill", "SKILL.md"), "---\nname: s\n---\nbody\n"); + + await skillsSurface(["--team", "t", "--pack-dir", packDir]); + + expect(existsSync(join(packDir, "pack", "surface.jsonc"))).toBe(false); + const out = logs.join("\n"); + expect(out).toContain("my-skill"); + expect(out).toContain("rt skills surface set"); + }); + + test("empty pack: prints a no-skills message instead of crashing", async () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + + await skillsSurface(["--pack-dir", packDir]); + + expect(logs.some((l) => l.includes("no skills registered"))).toBe(true); + }); + + test("unrecognized subcommand: clean one-line error, exit 1", async () => { + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsSurface(["bogus-mode"]), + ); + expect(exitCode).toBe(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("bogus-mode"); + }); +}); diff --git a/commands/skills.ts b/commands/skills.ts index 3642cd75..044a060b 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -16,10 +16,12 @@ * queries for real via `claude plugin list --json`. */ -import { chmodSync, copyFileSync, existsSync, mkdirSync, readdirSync, readFileSync, rmSync, statSync, writeFileSync } from "fs"; +import { execFileSync, spawnSync } from "child_process"; +import { chmodSync, copyFileSync, existsSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, statSync, writeFileSync } from "fs"; import { dirname, join } from "path"; +import { resolveFzf } from "../lib/fzf.ts"; import { mattstackHome } from "../lib/rt-paths.ts"; -import { compileSkill } from "../lib/skills/compile.ts"; +import { compileSkill, HEADER_COMMENT } from "../lib/skills/compile.ts"; import { invocableRoster, loadAttachment, @@ -28,6 +30,7 @@ import { readSurface, readVerbRoster, resolvePluginRoots, + stripFrontmatter, type PluginRoots, type SurfaceConfig, } from "../lib/skills/sources.ts"; @@ -337,3 +340,323 @@ export async function skillsCheck(args: string[]): Promise { if (anyStale) process.exitCode = 1; }); } + +// ─── rt skills surface -- list / set / apply / fzf palette ──────────────── + +type SurfaceFlags = { + team: string; + dryRun: boolean; + packDir: string | null; + mattstackDir: string | null; + manifest: string | null; +}; + +type SurfaceRow = { name: string; kind: "compiled" | "hand-authored"; status: "public" | "internal" }; + +function parseSurfaceFlags(args: string[]): { flags: SurfaceFlags; rest: string[] } { + let team = "claimview"; + let dryRun = false; + let packDir: string | null = null; + let mattstackDir: string | null = null; + let manifest: string | null = null; + const rest: string[] = []; + + for (let i = 0; i < args.length; i++) { + const a = args[i]!; + switch (a) { + case "--team": team = args[++i] ?? team; break; + case "--dry-run": dryRun = true; break; + case "--pack-dir": packDir = args[++i] ?? null; break; + case "--mattstack-dir": mattstackDir = args[++i] ?? null; break; + case "--manifest": manifest = args[++i] ?? null; break; + default: rest.push(a); + } + } + + return { flags: { team, dryRun, packDir, mattstackDir, manifest }, rest }; +} + +function resolveSurfacePaths(flags: SurfaceFlags): { packDir: string } { + const mattstackRoot = flags.mattstackDir ?? mattstackHome(); + const packDir = flags.packDir ?? packRootDir(mattstackRoot, flags.team); + return { packDir }; +} + +function isCompiledDir(dir: string): boolean { + const skillMdPath = join(dir, "SKILL.md"); + if (!existsSync(skillMdPath)) return false; + const { body } = stripFrontmatter(readFileSync(skillMdPath, "utf8")); + return body.startsWith(HEADER_COMMENT); +} + +/** Stub verb names are always compile targets; a materialized dir carrying the compiler header is one too, even if its verb was since retired from stubs.jsonc. */ +function classify(name: string, dir: string | null, verbNames: Set): "compiled" | "hand-authored" { + if (verbNames.has(name)) return "compiled"; + if (dir && isCompiledDir(dir)) return "compiled"; + return "hand-authored"; +} + +function collectRegistry(packDir: string, verbNames: Set) { + const skillsNames = new Set(listSubdirs(join(packDir, "skills"))); + const attachmentNames = new Set(listSubdirs(join(packDir, "attachments"))); + const allNames = new Set([...skillsNames, ...attachmentNames, ...verbNames]); + return { skillsNames, attachmentNames, allNames }; +} + +/** The set `set`'s first use bootstraps surface.jsonc from -- so the first edit is a delta from reality, not a cliff. */ +function defaultPublicSet(skillsNames: Set, verbNames: Set): Set { + return new Set([...skillsNames, ...verbNames]); +} + +function computeRows( + packDir: string, + verbNames: Set, + surface: SurfaceConfig | null, +): { source: string; rows: SurfaceRow[] } { + const { skillsNames, attachmentNames, allNames } = collectRegistry(packDir, verbNames); + const publicSet = surface ? new Set(surface.public) : defaultPublicSet(skillsNames, verbNames); + const source = surface + ? "pack/surface.jsonc" + : "(no surface.jsonc yet -- inferred from current skills/ + stubs.jsonc placement)"; + + const rows = [...allNames].sort().map((name) => { + const dir = skillsNames.has(name) + ? join(packDir, "skills", name) + : attachmentNames.has(name) + ? join(packDir, "attachments", name) + : null; + return { + name, + kind: classify(name, dir, verbNames), + status: (publicSet.has(name) ? "public" : "internal") as "public" | "internal", + }; + }); + + return { source, rows }; +} + +function writeSurfaceConfig(packDir: string, publicList: string[]): void { + const path = join(packDir, "pack", "surface.jsonc"); + mkdirSync(dirname(path), { recursive: true }); + const json = JSON.stringify({ public: publicList }, null, 2); + writeFileSync(path, `// surface.jsonc -- names this pack's public skills/ directories.\n${json}\n`); +} + +function compileArgs(flags: SurfaceFlags, packDir: string): string[] { + const args = ["--team", flags.team, "--pack-dir", packDir]; + if (flags.mattstackDir) args.push("--mattstack-dir", flags.mattstackDir); + if (flags.manifest) args.push("--manifest", flags.manifest); + if (flags.dryRun) args.push("--dry-run"); + return args; +} + +function isInsideGitWorkTree(dir: string): boolean { + try { + execFileSync("git", ["rev-parse", "--is-inside-work-tree"], { cwd: dir, stdio: "pipe" }); + return true; + } catch { + return false; + } +} + +/** git mv keeps rename history for the common case; fixtures (and any non-git pack dir) fall back to a plain rename. */ +function moveHandAuthoredDir(packDir: string, name: string, from: "skills" | "attachments", to: "skills" | "attachments"): string | null { + const fromRel = join(from, name); + const toRel = join(to, name); + mkdirSync(join(packDir, to), { recursive: true }); + + if (isInsideGitWorkTree(packDir)) { + execFileSync("git", ["mv", fromRel, toRel], { cwd: packDir, stdio: "pipe" }); + return null; + } + + renameSync(join(packDir, fromRel), join(packDir, toRel)); + return "plain rename -- pack dir is not a git repo"; +} + +function printSurfaceRows(flags: SurfaceFlags, source: string, rows: SurfaceRow[]): void { + console.log(`rt skills surface -- team ${flags.team}`); + console.log(`source: ${source}`); + for (const row of rows) { + console.log(` ${row.status.padEnd(9)}${row.kind.padEnd(15)}${row.name}`); + } +} + +async function runList(flags: SurfaceFlags): Promise { + const { packDir } = resolveSurfacePaths(flags); + const verbNames = new Set(readVerbRoster(packDir).map((v) => v.name)); + const surface = readSurface(packDir); + const { source, rows } = computeRows(packDir, verbNames, surface); + + printSurfaceRows(flags, source, rows); + if (rows.length === 0) console.log("(no skills registered in this pack)"); +} + +async function runApply(flags: SurfaceFlags): Promise { + const { packDir } = resolveSurfacePaths(flags); + const verbNames = new Set(readVerbRoster(packDir).map((v) => v.name)); + const surface = readSurface(packDir); + const { skillsNames, attachmentNames } = collectRegistry(packDir, verbNames); + const publicSet = surface ? new Set(surface.public) : defaultPublicSet(skillsNames, verbNames); + + const candidates = [...new Set([...skillsNames, ...attachmentNames])].sort(); + let moved = 0; + + for (const name of candidates) { + const currentlyUnderSkills = skillsNames.has(name); + const dir = join(packDir, currentlyUnderSkills ? "skills" : "attachments", name); + if (classify(name, dir, verbNames) === "compiled") continue; // regenerated/removed by the compile step below, never git-mv'd + + const wantPublic = publicSet.has(name); + if (currentlyUnderSkills === wantPublic) continue; + + const from = currentlyUnderSkills ? "skills" : "attachments"; + const to = currentlyUnderSkills ? "attachments" : "skills"; + moved++; + + if (flags.dryRun) { + console.log(`would move ${name}: ${from}/ -> ${to}/`); + continue; + } + + const note = moveHandAuthoredDir(packDir, name, from, to); + console.log(`moved ${name}: ${from}/ -> ${to}/${note ? ` (${note})` : ""}`); + } + + if (moved === 0) console.log("no moves needed"); + + await skillsCompile(compileArgs(flags, packDir)); +} + +async function runSet(name: string, want: "public" | "internal", flags: SurfaceFlags): Promise { + const { packDir } = resolveSurfacePaths(flags); + const verbNames = new Set(readVerbRoster(packDir).map((v) => v.name)); + const { skillsNames, allNames } = collectRegistry(packDir, verbNames); + + if (!allNames.has(name)) { + throw new SkillsUsageError( + `"${name}" is not a known skill or verb in this pack (checked skills/, attachments/, stubs.jsonc)`, + ); + } + + const surface = readSurface(packDir); + const publicSet = surface ? new Set(surface.public) : defaultPublicSet(skillsNames, verbNames); + + if (want === "public") publicSet.add(name); + else publicSet.delete(name); + + writeSurfaceConfig(packDir, [...publicSet].sort()); + console.log(`${name}: ${want}`); + + await runApply(flags); +} + +async function runPalette(flags: SurfaceFlags): Promise { + const { packDir } = resolveSurfacePaths(flags); + const verbNames = new Set(readVerbRoster(packDir).map((v) => v.name)); + const surface = readSurface(packDir); + const { source, rows } = computeRows(packDir, verbNames, surface); + + if (rows.length === 0) { + console.log("(no skills registered in this pack)"); + return; + } + + const fzfPath = resolveFzf(); + if (!fzfPath || !process.stdin.isTTY) { + printSurfaceRows(flags, source, rows); + console.log(""); + console.log("no tty or fzf not found -- edit one at a time: rt skills surface set --public|--internal"); + return; + } + + const preselected = rows + .map((row, i) => (row.status === "public" ? i + 1 : null)) + .filter((i): i is number => i !== null); + const loadBind = preselected.length + ? `load:${preselected.map((pos) => `pos(${pos})+toggle`).join("+")}+pos(1)` + : "load:pos(1)"; + + const input = rows + .map((row) => `${row.name}\t${row.status.padEnd(9)}${row.kind.padEnd(15)}${row.name}`) + .join("\n"); + + const result = spawnSync( + "fzf", + [ + "--multi", + "--with-nth=2..", + "--delimiter=\t", + "--layout=reverse", + "--border=rounded", + "--border-label= rt skills surface ", + "--prompt= filter: ", + "--header=space: toggle public tab: toggle+next enter: apply esc: cancel", + "--no-mouse", + "--bind=space:toggle,tab:toggle+down", + `--bind=${loadBind}`, + ], + { input, stdio: ["pipe", "pipe", "inherit"], encoding: "utf8" }, + ); + + if (result.status !== 0) { + console.log("cancelled -- no changes made"); + return; + } + + const selected = (result.stdout ?? "") + .replace(/\n$/, "") + .split("\n") + .map((line) => line.split("\t")[0]!) + .filter(Boolean); + + writeSurfaceConfig(packDir, [...new Set(selected)].sort()); + console.log(`surface.jsonc updated: ${selected.length} public`); + + await runApply(flags); +} + +export async function skillsSurface(args: string[]): Promise { + await withCleanErrors(async () => { + const mode = args[0]; + + if (mode === "list") { + const { flags, rest } = parseSurfaceFlags(args.slice(1)); + if (rest.length) throw new SkillsUsageError(`unrecognized argument "${rest[0]}"`); + await runList(flags); + return; + } + + if (mode === "apply") { + const { flags, rest } = parseSurfaceFlags(args.slice(1)); + if (rest.length) throw new SkillsUsageError(`unrecognized argument "${rest[0]}"`); + await runApply(flags); + return; + } + + if (mode === "set") { + const name = args[1]; + if (!name || name.startsWith("--")) { + throw new SkillsUsageError("set requires a skill name: rt skills surface set --public|--internal"); + } + const { flags, rest } = parseSurfaceFlags(args.slice(2)); + let want: "public" | "internal" | null = null; + for (const a of rest) { + if (a === "--public") want = "public"; + else if (a === "--internal") want = "internal"; + else throw new SkillsUsageError(`unrecognized argument "${a}"`); + } + if (!want) throw new SkillsUsageError("set requires --public or --internal"); + await runSet(name, want, flags); + return; + } + + if (mode !== undefined && !mode.startsWith("--")) { + throw new SkillsUsageError(`unrecognized subcommand "${mode}" (expected list, set, or apply)`); + } + + const { flags, rest } = parseSurfaceFlags(args); + if (rest.length) throw new SkillsUsageError(`unrecognized argument "${rest[0]}"`); + await runPalette(flags); + }); +} diff --git a/lib/command-tree-def.ts b/lib/command-tree-def.ts index 37c6e7f9..2a5f0d05 100644 --- a/lib/command-tree-def.ts +++ b/lib/command-tree-def.ts @@ -774,6 +774,16 @@ export const TREE: Record = { { name: "Manifest", flag: "--manifest", type: "text", placeholder: "/path/to/skills.jsonc", hint: "Manifest path; omit to auto-find the newest ~/.mattstack/repos/*/skills.jsonc naming this team" }, ], }, + surface: { + description: "List, set, or apply the pack's public/internal skill surface (bare invocation opens an fzf multi-toggle palette)", + module: "./commands/skills.ts", + fn: "skillsSurface", + args: [ + { name: "Mode", type: "text", placeholder: "list", hint: "list | set --public|--internal | apply; omit for the fzf palette" }, + { name: "Team", flag: "--team", type: "text", placeholder: "claimview", hint: "Pack team; default claimview" }, + { name: "Dry run", flag: "--dry-run", type: "boolean", default: false, hint: "apply only: print planned moves without touching disk" }, + ], + }, }, }, diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts index b7e76f78..e7894bb4 100644 --- a/lib/skills/compile.ts +++ b/lib/skills/compile.ts @@ -2,7 +2,8 @@ import type { AttachmentSource, CompiledFile, CompileResult, StepSource, VerbDef const CLAUDE_SKILL_DIR_TOKEN = "${CLAUDE_SKILL_DIR}"; -const HEADER_COMMENT = +/** Exported so `rt skills surface` can classify a directory as compiled by checking its SKILL.md body prefix. */ +export const HEADER_COMMENT = ""; const REGISTERED_NAME_RE = /\b(mattstack|claimview|assured):[a-z][a-z0-9-]*\b/g; From 9c4975eb7575a4972c18ec028eb4fc114198f402 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 10:25:51 -0500 Subject: [PATCH 12/17] fix: gate the surface palette's accept path on a reviewed confirm fzf's default --multi accept emits the cursor row on Enter even when nothing is marked, so unchecking everything could silently reintroduce one skill into public. Compute the public/internal delta before writing anything, print it, and require an explicit y/N confirm when it's non-empty; "no changes" short-circuits without prompting. The decision itself (decidePaletteAction) is a pure function so the gate is unit- tested without spawning fzf or reading a terminal. Also corrects lib/fzf.ts's module doc, which claimed every fzf spawn site calls ensureFzf() -- the surface palette's non-tty/missing-fzf fallback is a deliberate exception. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/__tests__/skills-surface.test.ts | 53 ++++++++++++- commands/skills.ts | 94 +++++++++++++++++++++-- lib/fzf.ts | 5 +- 3 files changed, 142 insertions(+), 10 deletions(-) diff --git a/commands/__tests__/skills-surface.test.ts b/commands/__tests__/skills-surface.test.ts index bb55a005..57fc5e22 100644 --- a/commands/__tests__/skills-surface.test.ts +++ b/commands/__tests__/skills-surface.test.ts @@ -3,7 +3,7 @@ import { execFileSync } from "child_process"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { dirname, join } from "path"; -import { skillsSurface } from "../skills.ts"; +import { decidePaletteAction, skillsSurface } from "../skills.ts"; function writeFile(path: string, content: string): void { mkdirSync(dirname(path), { recursive: true }); @@ -321,3 +321,54 @@ describe("skillsSurface bare invocation (fzf palette)", () => { expect(errors[0]).toContain("bogus-mode"); }); }); + +describe("decidePaletteAction", () => { + test("zero-marked cursor-row artifact shows as a +1 delta the user can decline", () => { + // Everything was internal; fzf's default --multi accept on Enter with + // nothing marked emits the cursor row anyway -- that row now reads + // "public" in resultRows even though the user meant to uncheck everything. + const previousPublic = new Set(); + const resultRows = [{ name: "x", status: "public" as const }]; + + const action = decidePaletteAction(previousPublic, resultRows, false); + + expect(action.kind).toBe("declined"); + if (action.kind !== "no-changes") { + expect(action.delta.toPublic).toEqual(["x"]); + expect(action.delta.toInternal).toEqual([]); + } + }); + + test("decline writes nothing: the confirmed=false path never yields a write action", () => { + const previousPublic = new Set(); + const resultRows = [{ name: "x", status: "public" as const }]; + + const action = decidePaletteAction(previousPublic, resultRows, false); + + expect(action.kind).not.toBe("write"); + }); + + test("confirming a real delta yields a write action carrying it", () => { + const previousPublic = new Set(["y"]); + const resultRows = [ + { name: "x", status: "public" as const }, + { name: "y", status: "internal" as const }, + ]; + + const action = decidePaletteAction(previousPublic, resultRows, true); + + expect(action.kind).toBe("write"); + if (action.kind === "write") { + expect(action.delta.toPublic).toEqual(["x"]); + expect(action.delta.toInternal).toEqual(["y"]); + } + }); + + test("no changes short-circuits regardless of the confirm answer", () => { + const previousPublic = new Set(["x"]); + const resultRows = [{ name: "x", status: "public" as const }]; + + expect(decidePaletteAction(previousPublic, resultRows, false).kind).toBe("no-changes"); + expect(decidePaletteAction(previousPublic, resultRows, true).kind).toBe("no-changes"); + }); +}); diff --git a/commands/skills.ts b/commands/skills.ts index 044a060b..1d946c67 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -18,6 +18,7 @@ import { execFileSync, spawnSync } from "child_process"; import { chmodSync, copyFileSync, existsSync, mkdirSync, readdirSync, readFileSync, renameSync, rmSync, statSync, writeFileSync } from "fs"; +import { createInterface } from "node:readline"; import { dirname, join } from "path"; import { resolveFzf } from "../lib/fzf.ts"; import { mattstackHome } from "../lib/rt-paths.ts"; @@ -551,10 +552,65 @@ async function runSet(name: string, want: "public" | "internal", flags: SurfaceF await runApply(flags); } +export type SurfaceDelta = { toPublic: string[]; toInternal: string[] }; + +export type PaletteAction = + | { kind: "no-changes" } + | { kind: "write"; delta: SurfaceDelta } + | { kind: "declined"; delta: SurfaceDelta }; + +/** + * Pure decision seam for the palette's accept path -- fzf's default --multi + * semantics emit the cursor row on Enter even when nothing is marked, so a + * deliberate "uncheck everything" can silently reintroduce one row. Called + * once (confirmed=false) to compute the delta for the pre-write preview, and + * again with the real answer once the user has seen it. + */ +export function decidePaletteAction( + previousPublic: Set, + resultRows: { name: string; status: "public" | "internal" }[], + confirmed: boolean, +): PaletteAction { + const toPublic: string[] = []; + const toInternal: string[] = []; + + for (const row of resultRows) { + const was = previousPublic.has(row.name); + const now = row.status === "public"; + if (was === now) continue; + if (now) toPublic.push(row.name); + else toInternal.push(row.name); + } + toPublic.sort(); + toInternal.sort(); + + if (toPublic.length === 0 && toInternal.length === 0) return { kind: "no-changes" }; + const delta = { toPublic, toInternal }; + return confirmed ? { kind: "write", delta } : { kind: "declined", delta }; +} + +function printDelta(delta: SurfaceDelta): void { + console.log("changes:"); + for (const name of delta.toPublic) console.log(` + public ${name}`); + for (const name of delta.toInternal) console.log(` - public ${name}`); +} + +function confirmYesNo(promptText: string): Promise { + return new Promise((resolve) => { + const rl = createInterface({ input: process.stdin, output: process.stderr }); + rl.question(promptText, (answer) => { + rl.close(); + resolve(answer.trim().toLowerCase() === "y" || answer.trim().toLowerCase() === "yes"); + }); + }); +} + async function runPalette(flags: SurfaceFlags): Promise { const { packDir } = resolveSurfacePaths(flags); const verbNames = new Set(readVerbRoster(packDir).map((v) => v.name)); const surface = readSurface(packDir); + const { skillsNames } = collectRegistry(packDir, verbNames); + const previousPublic = surface ? new Set(surface.public) : defaultPublicSet(skillsNames, verbNames); const { source, rows } = computeRows(packDir, verbNames, surface); if (rows.length === 0) { @@ -591,7 +647,7 @@ async function runPalette(flags: SurfaceFlags): Promise { "--border=rounded", "--border-label= rt skills surface ", "--prompt= filter: ", - "--header=space: toggle public tab: toggle+next enter: apply esc: cancel", + "--header=space: toggle public tab: toggle+next enter: review changes esc: cancel", "--no-mouse", "--bind=space:toggle,tab:toggle+down", `--bind=${loadBind}`, @@ -604,14 +660,36 @@ async function runPalette(flags: SurfaceFlags): Promise { return; } - const selected = (result.stdout ?? "") - .replace(/\n$/, "") - .split("\n") - .map((line) => line.split("\t")[0]!) - .filter(Boolean); + const selectedSet = new Set( + (result.stdout ?? "") + .replace(/\n$/, "") + .split("\n") + .map((line) => line.split("\t")[0]!) + .filter(Boolean), + ); + + const resultRows = rows.map((row) => ({ + name: row.name, + status: (selectedSet.has(row.name) ? "public" : "internal") as "public" | "internal", + })); + + const preview = decidePaletteAction(previousPublic, resultRows, false); + if (preview.kind === "no-changes") { + console.log("no changes -- surface.jsonc left as is"); + return; + } + + printDelta(preview.delta); + const confirmed = await confirmYesNo(" apply these changes? [y/N] "); + const decision = decidePaletteAction(previousPublic, resultRows, confirmed); + + if (decision.kind !== "write") { + console.log("declined -- no changes made"); + return; + } - writeSurfaceConfig(packDir, [...new Set(selected)].sort()); - console.log(`surface.jsonc updated: ${selected.length} public`); + writeSurfaceConfig(packDir, [...selectedSet].sort()); + console.log(`surface.jsonc updated: ${selectedSet.size} public`); await runApply(flags); } diff --git a/lib/fzf.ts b/lib/fzf.ts index 7b23ef38..70dbbecb 100644 --- a/lib/fzf.ts +++ b/lib/fzf.ts @@ -4,7 +4,10 @@ * — never installed, or reaped by `brew autoremove`/`brew cleanup` — we want a * single, actionable error rather than silently degrading to a non-fuzzy * picker or crashing with an opaque spawn ENOENT. Every fzf spawn site calls - * ensureFzf() before spawning. + * ensureFzf() before spawning, except `rt skills surface`'s bare palette: + * that one deliberately soft-falls-back to printing the static list instead + * of erroring, since the same config-write path is also reachable via + * `rt skills surface set`. */ import { bold, dim, yellow, reset } from "./tui.ts"; From 05eb39e45ed3012069a90c65bd591721c7e8283b Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 10:47:30 -0500 Subject: [PATCH 13/17] feat: surface.jsonc root fallback; vendor all step files, not just scripts/ readSurface now falls back to /surface.jsonc for packs without a pack/ config dir (the mattstack plugin repo). loadStepSource lists every non-SKILL.md step file (references/, scripts/, ...) and the compiler vendors them path-preserving, so an engine's load-bearing references/ files survive into the compiled verb. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/compile.test.ts | 12 ++++++++---- lib/skills/__tests__/sources.test.ts | 5 +++-- lib/skills/__tests__/surface.test.ts | 23 +++++++++++++++++++---- lib/skills/compile.ts | 5 ++--- lib/skills/sources.ts | 9 ++++++--- lib/skills/types.ts | 2 +- 6 files changed, 39 insertions(+), 17 deletions(-) diff --git a/lib/skills/__tests__/compile.test.ts b/lib/skills/__tests__/compile.test.ts index 84eabea5..3142eb91 100644 --- a/lib/skills/__tests__/compile.test.ts +++ b/lib/skills/__tests__/compile.test.ts @@ -19,7 +19,7 @@ const step: StepSource = { forge: { contract: "ci-forge@1", required: true }, }, allowedTools: ["Bash(gh:*)", "Read"], - scriptFiles: ["scripts/ci-watch.sh"], + stepFiles: ["references/polling-notes.md", "scripts/ci-watch.sh"], }; const domainFill: AttachmentSource = { @@ -170,13 +170,17 @@ describe("compileSkill", () => { expect(error?.message).toContain("claimview:watch-ci-domain"); }); - test("vendoring: scriptFiles and extraFiles map to exact copyFrom paths", () => { + test("vendoring: stepFiles and extraFiles map to exact copyFrom paths", () => { const result = compileSkill(verb, step, { domain: domainFill, forge: forgeFill }, roster); expect(result.files).toContainEqual({ path: "scripts/ci-watch.sh", copyFrom: "/plugins/mattstack/skills/pipeline/watch-ci/scripts/ci-watch.sh", }); + expect(result.files).toContainEqual({ + path: "references/polling-notes.md", + copyFrom: "/plugins/mattstack/skills/pipeline/watch-ci/references/polling-notes.md", + }); expect(result.files).toContainEqual({ path: "parts/domain/ci-config.json", copyFrom: "/plugins/claimview/attachments/watch-ci-domain/ci-config.json", @@ -229,7 +233,7 @@ describe("compileSkill", () => { body: "This step defers domain judgment to claimview:watch-ci-domain and never invokes claimview:nonexistent.", slots: {}, allowedTools: [], - scriptFiles: [], + stepFiles: [], }; const lintRoster = new Set(["mattstack:watch-ci", "claimview:watch-ci-domain"]); @@ -294,7 +298,7 @@ describe("compileSkill", () => { body: "Review your own diff before shipping.", slots: {}, allowedTools: [], - scriptFiles: [], + stepFiles: [], }; const bareVerb: VerbDef = { name: "self-review", diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts index fc077c59..385c9de9 100644 --- a/lib/skills/__tests__/sources.test.ts +++ b/lib/skills/__tests__/sources.test.ts @@ -63,6 +63,7 @@ function makeFixtureRoots(): { rootDir: string; roots: PluginRoots } { const mattstackDir = join(rootDir, "mattstack"); writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "SKILL.md"), WATCH_CI_SKILL_MD); writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", "ci-watch.sh"), WATCH_CI_SCRIPT); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "references", "polling-notes.md"), "Polling notes.\n"); writeFile(join(mattstackDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); const untypedSkillMd = WATCH_CI_SKILL_MD.replace("type: pipeline-step\n", ""); @@ -127,7 +128,7 @@ describe("stripFrontmatter", () => { }); describe("loadStepSource", () => { - test("finds the engine under skills/pipeline/, parses slots, lists scriptFiles", () => { + test("finds the engine under skills/pipeline/, parses slots, lists stepFiles", () => { const { roots } = makeFixtureRoots(); const step = loadStepSource("watch-ci", roots); @@ -142,7 +143,7 @@ describe("loadStepSource", () => { forge: { contract: "ci-forge@1", required: true }, }); expect(step.allowedTools).toEqual(["Bash(gh:*)", "Read"]); - expect(step.scriptFiles).toEqual(["scripts/ci-watch.sh"]); + expect(step.stepFiles).toEqual(["references/polling-notes.md", "scripts/ci-watch.sh"]); }); test("throws naming the file when the engine has no type: pipeline-step", () => { diff --git a/lib/skills/__tests__/surface.test.ts b/lib/skills/__tests__/surface.test.ts index 21edce60..bb34c156 100644 --- a/lib/skills/__tests__/surface.test.ts +++ b/lib/skills/__tests__/surface.test.ts @@ -36,6 +36,21 @@ describe("readSurface", () => { expect(readSurface(packDir)).toBeNull(); }); + + test("falls back to a root-level surface.jsonc when pack/ has none", () => { + const packDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-pack-"))); + writeFile(join(packDir, "surface.jsonc"), `{ "public": ["editing-skills"] }\n`); + + expect(readSurface(packDir)).toEqual({ public: ["editing-skills"] }); + }); + + test("pack/surface.jsonc wins over a root-level copy", () => { + const packDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-pack-"))); + writeFile(join(packDir, "pack", "surface.jsonc"), `{ "public": ["from-pack"] }\n`); + writeFile(join(packDir, "surface.jsonc"), `{ "public": ["from-root"] }\n`); + + expect(readSurface(packDir)).toEqual({ public: ["from-pack"] }); + }); }); describe("compileSkill with internalRoster", () => { @@ -51,7 +66,7 @@ describe("compileSkill with internalRoster", () => { body: "This step defers to claimview:cvi-gates for the internal check and to claimview:watch-ci-domain for the public one.", slots: {}, allowedTools: [], - scriptFiles: [], + stepFiles: [], }; const internalRoster = new Set(["claimview:cvi-gates"]); @@ -71,7 +86,7 @@ describe("compileSkill with internalRoster", () => { body: "This step defers to claimview:watch-ci-domain.", slots: {}, allowedTools: [], - scriptFiles: [], + stepFiles: [], }; const result = compileSkill(verb, step, {}, roster); @@ -88,7 +103,7 @@ describe("compileSkill with internalRoster", () => { body: "Poll CI.", slots: { domain: { contract: "watch-ci-domain@1", required: true } }, allowedTools: [], - scriptFiles: [], + stepFiles: [], }; const registeredInternalFill: AttachmentSource = { binding: "claimview:cvi-gates", @@ -124,7 +139,7 @@ describe("compileSkill with internalRoster", () => { body: "Poll CI.", slots: { domain: { contract: "watch-ci-domain@1", required: true } }, allowedTools: [], - scriptFiles: [], + stepFiles: [], }; const registeredPublicFill: AttachmentSource = { binding: "claimview:cvi-gates", diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts index e7894bb4..a77b4cdd 100644 --- a/lib/skills/compile.ts +++ b/lib/skills/compile.ts @@ -128,9 +128,8 @@ function buildBody( function buildVendoredFiles(step: StepSource, boundSlots: BoundSlot[]): CompiledFile[] { const files: CompiledFile[] = []; - for (const entry of step.scriptFiles) { - const tail = entry.startsWith("scripts/") ? entry.slice("scripts/".length) : entry; - files.push({ path: `scripts/${tail}`, copyFrom: `${step.dir}/${entry}` }); + for (const entry of step.stepFiles) { + files.push({ path: entry, copyFrom: `${step.dir}/${entry}` }); } for (const { slotName, fill } of boundSlots) { diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index fc4d93b9..992b6e60 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -166,7 +166,7 @@ export function loadStepSource(engineName: string, roots: PluginRoots): StepSour body, slots: parseSlots(frontmatter.slots), allowedTools: parseAllowedTools(frontmatter["allowed-tools"]), - scriptFiles: listFilesUnder(join(foundDir, "scripts"), new Set()).map((entry) => `scripts/${entry}`), + stepFiles: listFilesUnder(foundDir, new Set(["SKILL.md"])), }; } @@ -282,8 +282,11 @@ export function readManifestBindings(manifestPath: string): Record existsSync(candidate)); + if (!surfacePath) return null; const parsed = JSON.parse(stripJsonc(readFileSync(surfacePath, "utf8"))) as { public?: unknown }; const publicList = Array.isArray(parsed.public) ? parsed.public.filter((entry): entry is string => typeof entry === "string") diff --git a/lib/skills/types.ts b/lib/skills/types.ts index 55420465..5acaff2d 100644 --- a/lib/skills/types.ts +++ b/lib/skills/types.ts @@ -8,7 +8,7 @@ export type StepSource = { body: string; // frontmatter-stripped markdown slots: Record; allowedTools: string[]; // raw entries from frontmatter allowed-tools - scriptFiles: string[]; // paths relative to dir under scripts/, may be empty + stepFiles: string[]; // non-SKILL.md files relative to dir (scripts/, references/, ...), vendored path-preserving }; export type AttachmentSource = { From c8c9a40733a95352612e244de84c44418ca01930 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 11:27:00 -0500 Subject: [PATCH 14/17] fix: lint verb descriptions, seed internal roster from retired stubs, exclude junk from vendoring Review fixes on the surface-ratchet wave: (1) compileSkill lints the verb description against the internal roster, and non-public stub verbs seed that roster even when their compiled dirs are already deleted -- dangling references to retired doors now fail the compile; (2) vendoring skips dotfiles/dotdirs, __pycache__/, *.pyc, tests/ dirs, *.test.sh, and README.md so development junk stays out of compiled artifacts. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/skills.ts | 19 +++++-- lib/skills/__tests__/sources.test.ts | 7 +++ lib/skills/__tests__/surface.test.ts | 74 ++++++++++++++++++++++++++++ lib/skills/compile.ts | 13 +++-- lib/skills/sources.ts | 15 ++++++ 5 files changed, 119 insertions(+), 9 deletions(-) diff --git a/commands/skills.ts b/commands/skills.ts index 1d946c67..29670efe 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -190,15 +190,25 @@ type Resolved = { * they line up with body-prose tokens and fill bindings. A dir under * /skills/ not (yet) named in surface.jsonc's public list is * internal by default -- this is what lets a fill inline through the - * transition window before it physically moves. + * transition window before it physically moves. Non-public stub verbs seed + * the roster too: a retired verb's dir is deleted, so the dir scan alone + * would let dangling references to it slip through. */ -function computeInternalRoster(team: string, packDir: string, surface: SurfaceConfig | null): Set { +function computeInternalRoster( + team: string, + packDir: string, + surface: SurfaceConfig | null, + fullRoster: VerbDef[], +): Set { const internal = new Set(); if (!surface) return internal; const publicSet = new Set(surface.public); for (const name of listSubdirs(join(packDir, "skills"))) { if (!publicSet.has(name)) internal.add(`${team}:${name}`); } + for (const verb of fullRoster) { + if (!publicSet.has(verb.name)) internal.add(`${team}:${verb.name}`); + } return internal; } @@ -207,12 +217,13 @@ function resolve(flags: Flags): Resolved { const packDir = flags.packDir ?? packRootDir(mattstackRoot, flags.team); const manifestPath = flags.manifest ?? findDefaultManifest(mattstackRoot, flags.team); - const roster = selectVerbs(readVerbRoster(packDir), flags.verbs); + const fullRoster = readVerbRoster(packDir); + const roster = selectVerbs(fullRoster, flags.verbs); const bindings = readManifestBindings(manifestPath); const pluginRoots = flags.mattstackDir ? resolvePluginRootsFromDir(mattstackRoot) : resolvePluginRoots(); const invocable = invocableRoster(pluginRoots); const surface = readSurface(packDir); - const internalRoster = computeInternalRoster(flags.team, packDir, surface); + const internalRoster = computeInternalRoster(flags.team, packDir, surface, fullRoster); return { packDir, roster, bindings, pluginRoots, invocable, surface, internalRoster }; } diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts index 385c9de9..9eb992c8 100644 --- a/lib/skills/__tests__/sources.test.ts +++ b/lib/skills/__tests__/sources.test.ts @@ -64,6 +64,13 @@ function makeFixtureRoots(): { rootDir: string; roots: PluginRoots } { writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "SKILL.md"), WATCH_CI_SKILL_MD); writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", "ci-watch.sh"), WATCH_CI_SCRIPT); writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "references", "polling-notes.md"), "Polling notes.\n"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", ".DS_Store"), "junk"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "README.md"), "readme"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", ".gitignore"), "*.pyc"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", "__pycache__", "x.pyc"), "pyc"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", "helper.pyc"), "pyc"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "scripts", "poll.test.sh"), "test"); + writeFile(join(mattstackDir, "skills", "pipeline", "watch-ci", "tests", "harness.sh"), "test"); writeFile(join(mattstackDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); const untypedSkillMd = WATCH_CI_SKILL_MD.replace("type: pipeline-step\n", ""); diff --git a/lib/skills/__tests__/surface.test.ts b/lib/skills/__tests__/surface.test.ts index bb34c156..bb258c7c 100644 --- a/lib/skills/__tests__/surface.test.ts +++ b/lib/skills/__tests__/surface.test.ts @@ -77,6 +77,31 @@ describe("compileSkill with internalRoster", () => { ]); }); + test("a description token naming an internal skill produces a description-labeled errors entry", () => { + const dirtyVerb: VerbDef = { + name: "watch-ci", + engine: "watch-ci-engine", + description: "Watch CI; for a gut check use claimview:cvi-gates first.", + }; + const step: StepSource = { + name: "watch-ci", + plugin: "mattstack", + version: "1.0.0", + dir: "/plugins/mattstack/skills/pipeline/watch-ci", + body: "Clean body.", + slots: {}, + allowedTools: [], + stepFiles: [], + }; + const internalRoster = new Set(["claimview:cvi-gates"]); + + const result = compileSkill(dirtyVerb, step, {}, roster, { internalRoster }); + + expect(result.errors).toEqual([ + "description references claimview:cvi-gates which is surface-internal; inline it, reference it by path, or list it in surface.jsonc's public array", + ]); + }); + test("no internalRoster entries: errors is empty", () => { const step: StepSource = { name: "watch-ci", @@ -184,6 +209,15 @@ type: pipeline-step This verb is retired. `; +const DANGLING_STEP_SKILL_MD = `--- +name: dangling-step +description: "References a retired door" +type: pipeline-step +--- + +Defers to claimview:old-verb for cleanup. +`; + const STRAY_SKILL_MD = `--- name: stray-skill description: "Hand-authored skill not yet declared public" @@ -198,6 +232,7 @@ function makeMattstackDir(): string { writeFile(join(mattstackPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); writeFile(join(mattstackPluginDir, "skills", "pipeline", "watch-ci", "SKILL.md"), WATCH_CI_SKILL_MD); writeFile(join(mattstackPluginDir, "skills", "pipeline", "old-verb", "SKILL.md"), OLD_VERB_SKILL_MD); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "dangling-step", "SKILL.md"), DANGLING_STEP_SKILL_MD); return dir; } @@ -285,6 +320,45 @@ describe("skillsCompile with a surface config", () => { expect(existsSync(join(packDir, "skills", "stray-skill", "SKILL.md"))).toBe(true); }); + test("a retired stub verb with no dir on disk still seeds the internal roster: dangling references error", async () => { + const mattstackDir = makeMattstackDir(); + const stubs = `{ + "verbs": { + "dangling": { "engine": "dangling-step", "description": "Dangling." }, + "old-verb": { "engine": "old-verb", "description": "Retired." } + } +} +`; + const surfaceJsonc = `{ "public": ["dangling"] }\n`; + const packDir = makePackDir(stubs, surfaceJsonc); + const manifestPath = makeManifest("claimview"); + + const errors: string[] = []; + const errSpy = spyOn(console, "error").mockImplementation((...args: unknown[]) => { + errors.push(args.map(String).join(" ")); + }); + const exitSpy = spyOn(process, "exit").mockImplementation((() => { + throw new Error("process.exit called"); + }) as never); + + try { + await expect( + skillsCompile([ + "--team", "claimview", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + ]), + ).rejects.toThrow("process.exit called"); + + expect(errors.join("\n")).toContain("claimview:old-verb"); + expect(errors.join("\n")).toContain("surface-internal"); + } finally { + errSpy.mockRestore(); + exitSpy.mockRestore(); + } + }); + test("no surface.jsonc present: all verbs compile, no internal/misplaced lines, no exit code", async () => { const mattstackDir = makeMattstackDir(); const packDir = makePackDir(STUBS_TWO_VERBS, null); diff --git a/lib/skills/compile.ts b/lib/skills/compile.ts index a77b4cdd..fc64fbcf 100644 --- a/lib/skills/compile.ts +++ b/lib/skills/compile.ts @@ -184,18 +184,18 @@ function lintReferences(body: string, roster: Set, files: CompiledFile[] return warnings; } -function lintInternalRoster(body: string, internalRoster: Set): string[] { +function lintInternalRoster(text: string, internalRoster: Set, where: string): string[] { const errors: string[] = []; - const lintableBody = stripCompilerComments(body); + const lintableText = stripCompilerComments(text); const seenNames = new Set(); - for (const match of lintableBody.matchAll(REGISTERED_NAME_RE)) { + for (const match of lintableText.matchAll(REGISTERED_NAME_RE)) { const token = match[0]; if (seenNames.has(token)) continue; seenNames.add(token); if (internalRoster.has(token)) { errors.push( - `body references ${token} which is surface-internal; inline it, reference it by path, or list it in surface.jsonc's public array`, + `${where} references ${token} which is surface-internal; inline it, reference it by path, or list it in surface.jsonc's public array`, ); } } @@ -226,7 +226,10 @@ export function compileSkill( const files: CompiledFile[] = [{ path: "SKILL.md", content }, ...buildVendoredFiles(step, boundSlots)]; const warnings = [...lintReferences(body, roster, files), ...notes]; - const errors = lintInternalRoster(body, internalRoster); + const errors = [ + ...lintInternalRoster(body, internalRoster, "body"), + ...lintInternalRoster(verb.description, internalRoster, "description"), + ]; return { files, warnings, errors }; } diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index 992b6e60..7c2e96f6 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -97,12 +97,27 @@ function parseAllowedTools(raw: unknown): string[] { return []; } +/** + * Vendoring exclusions: development junk that must not ship inside a + * compiled verb -- dotfiles/dotdirs (.DS_Store, .gitignore), python + * bytecode, test harnesses, and READMEs. Load-bearing runtime files + * (scripts/, references/, data files) all pass. + */ +const VENDOR_EXCLUDED_DIRS = new Set(["__pycache__", "tests"]); + +function isVendorExcluded(name: string, isDir: boolean): boolean { + if (name.startsWith(".")) return true; + if (isDir) return VENDOR_EXCLUDED_DIRS.has(name); + return name.endsWith(".pyc") || name.endsWith(".test.sh") || name === "README.md"; +} + function listFilesUnder(dir: string, exclude: Set): string[] { const out: string[] = []; const walk = (sub: string) => { const abs = sub ? join(dir, sub) : dir; if (!existsSync(abs)) return; for (const entry of readdirSync(abs, { withFileTypes: true })) { + if (isVendorExcluded(entry.name, entry.isDirectory())) continue; const rel = sub ? `${sub}/${entry.name}` : entry.name; if (entry.isDirectory()) { walk(rel); From 81a8027ca12dc0770c4f30efb21f22c35f60bbe6 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 12:42:48 -0500 Subject: [PATCH 15/17] fix: seed internal roster from attachments/ dirs; reword two doc comments computeInternalRoster only scanned skills/ and non-public stub verbs, so a name that migrated to attachments/ via `surface apply` dropped out of the roster -- a body/description token still naming it downgraded from a compile error to a mere "not invocable" warning. Scan attachments/ too. Also: resolvePluginRoots' doc comment stated its constraint via process artifacts ("plan constraint", "Task 5") instead of the constraint itself; and the skills node description in the command tree omitted surface. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/skills.ts | 9 ++++++- lib/command-tree-def.ts | 2 +- lib/skills/__tests__/surface.test.ts | 39 ++++++++++++++++++++++++++++ lib/skills/sources.ts | 7 ++--- 4 files changed, 52 insertions(+), 5 deletions(-) diff --git a/commands/skills.ts b/commands/skills.ts index 29670efe..464cb69e 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -192,7 +192,11 @@ type Resolved = { * internal by default -- this is what lets a fill inline through the * transition window before it physically moves. Non-public stub verbs seed * the roster too: a retired verb's dir is deleted, so the dir scan alone - * would let dangling references to it slip through. + * would let dangling references to it slip through. attachments/ dirs seed + * it as well: `surface apply` moves a skill there once it goes internal, so + * without this a name that migrated from skills/ to attachments/ falls out + * of the roster and a body token naming it downgrades from a compile error + * to a mere "not invocable" warning. */ function computeInternalRoster( team: string, @@ -206,6 +210,9 @@ function computeInternalRoster( for (const name of listSubdirs(join(packDir, "skills"))) { if (!publicSet.has(name)) internal.add(`${team}:${name}`); } + for (const name of listSubdirs(join(packDir, "attachments"))) { + if (!publicSet.has(name)) internal.add(`${team}:${name}`); + } for (const verb of fullRoster) { if (!publicSet.has(verb.name)) internal.add(`${team}:${verb.name}`); } diff --git a/lib/command-tree-def.ts b/lib/command-tree-def.ts index 2a5f0d05..a2127bfc 100644 --- a/lib/command-tree-def.ts +++ b/lib/command-tree-def.ts @@ -751,7 +751,7 @@ export const TREE: Record = { }, skills: { - description: "Compile and check the pack's committed skills", + description: "Compile, check, and manage the surface of the pack's committed skills", subcommands: { compile: { description: "Compile pack verbs from step sources + manifest bindings into committed SKILL.md files", diff --git a/lib/skills/__tests__/surface.test.ts b/lib/skills/__tests__/surface.test.ts index bb258c7c..9f28c0f9 100644 --- a/lib/skills/__tests__/surface.test.ts +++ b/lib/skills/__tests__/surface.test.ts @@ -524,4 +524,43 @@ describe("computeInternalRoster integration (pack dir doubles as plugin root)", expect(errors[0]).toContain("surface-internal"); expect(existsSync(join(claimviewDir, "skills", "gate-check"))).toBe(false); }); + + test("post-move steady state: a skill already under attachments/ still seeds the internal roster; a body reference errors", async () => { + const mattstackDir = realpathSync(mkdtempSync(join(tmpdir(), "rt-skills-surface-int-"))); + + const claimviewDir = join(mattstackDir, "plugins", "claimview"); + writeFile(join(claimviewDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "0.3.0" })); + writeFile( + join(claimviewDir, "pack", "stubs.jsonc"), + `{ "verbs": { "gate-check": { "engine": "gate-check", "description": "Gate check." } } }\n`, + ); + writeFile(join(claimviewDir, "pack", "surface.jsonc"), `{ "public": ["gate-check"] }\n`); + // cvi-gates already moved by a prior `surface apply` -- lives under + // attachments/, not skills/, and is absent from surface.public. + writeFile(join(claimviewDir, "attachments", "cvi-gates", "SKILL.md"), CVI_GATES_SKILL_MD); + + const mattstackPluginDir = join(mattstackDir, "plugins", "mattstack"); + writeFile(join(mattstackPluginDir, ".claude-plugin", "plugin.json"), JSON.stringify({ version: "1.2.0" })); + writeFile(join(mattstackPluginDir, "skills", "pipeline", "gate-check", "SKILL.md"), GATE_CHECK_SKILL_MD); + + const manifestPath = makeManifestAt("{}"); + + const { exitCode, errors } = await runExpectingCleanExit(() => + skillsCompile([ + "--team", "claimview", + "--pack-dir", claimviewDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "gate-check", + ]), + ); + + expect(exitCode).toBe(1); + expect(errors).toHaveLength(1); + expect(errors[0]).toStartWith("rt skills: "); + expect(errors[0]).toContain("gate-check"); + expect(errors[0]).toContain("claimview:cvi-gates"); + expect(errors[0]).toContain("surface-internal"); + expect(existsSync(join(claimviewDir, "skills", "gate-check"))).toBe(false); + }); }); diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index 7c2e96f6..1ab50f68 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -61,9 +61,10 @@ export function buildPluginRoots(list: PluginListEntry[]): PluginRoots { } /** - * Thin by design: the subprocess call itself is not unit-tested (see plan - * constraint); buildPluginRoots carries the tested logic. Task 5 exercises - * this live against real installed plugins. + * Thin by design: shelling out to `claude plugin list` is slow and + * environment-dependent, so the subprocess call itself is not unit-tested; + * buildPluginRoots carries the tested logic and this wrapper is exercised + * live against real installed plugins in integration coverage instead. */ export function resolvePluginRoots(): PluginRoots { const raw = execSync("claude plugin list --json", { encoding: "utf8" }); From b1f01c8dca033728cd854419b57c2473855b780c Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 13:16:05 -0500 Subject: [PATCH 16/17] feat: loadStepSource falls back to attachments/ for unregistered engines Non-public mattstack engines are moving from skills// to attachments// (unregistered, so they drop out of the user's slash autocomplete while staying invocable by name). Search the same shape loadAttachment uses -- flat attachments//SKILL.md then one group level deep -- and report every path tried on a miss. invocableRoster already only walks skills/, so it excludes attachments by construction; added a roster assertion to make that explicit. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- lib/skills/__tests__/sources.test.ts | 40 +++++++++++++++++++++++++--- lib/skills/sources.ts | 26 +++++++++++++++++- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/lib/skills/__tests__/sources.test.ts b/lib/skills/__tests__/sources.test.ts index 9eb992c8..89119fee 100644 --- a/lib/skills/__tests__/sources.test.ts +++ b/lib/skills/__tests__/sources.test.ts @@ -52,7 +52,9 @@ const CI_CONFIG_JSON = `{ "noisy": ["flaky-job"] }\n`; /** * mattstack root: skills/pipeline/watch-ci/SKILL.md (+ scripts/ci-watch.sh), - * matching the real plugin's group/engine nesting. + * matching the real plugin's group/engine nesting, plus + * attachments/pipeline/ship/SKILL.md -- an engine already moved out of + * skills/ (unregistered) that loadStepSource must still resolve. * claimview root: attachments/watch-ci-domain/SKILL.md (+ ci-config.json), the * unregistered fill; a registered copy also lives under skills/watch-ci-domain * so loadAttachment can be exercised against both search roots. @@ -76,6 +78,12 @@ function makeFixtureRoots(): { rootDir: string; roots: PluginRoots } { const untypedSkillMd = WATCH_CI_SKILL_MD.replace("type: pipeline-step\n", ""); writeFile(join(mattstackDir, "skills", "pipeline", "untyped-step", "SKILL.md"), untypedSkillMd); + const shipSkillMd = WATCH_CI_SKILL_MD.replace(/name: watch-ci/, "name: ship").replace( + "Poll the pipeline every 30s and report status.", + "Ship it.", + ); + writeFile(join(mattstackDir, "attachments", "pipeline", "ship", "SKILL.md"), shipSkillMd); + const claimviewDir = join(rootDir, "claimview"); writeFile(join(claimviewDir, "attachments", "watch-ci-domain", "SKILL.md"), WATCH_CI_DOMAIN_SKILL_MD); writeFile(join(claimviewDir, "attachments", "watch-ci-domain", "ci-config.json"), CI_CONFIG_JSON); @@ -161,13 +169,26 @@ describe("loadStepSource", () => { test("throws listing searched paths when the engine is absent entirely", () => { const { roots } = makeFixtureRoots(); - const expectedSearchedPath = join( + const expectedSkillsPath = join( roots.byName.mattstack!.dir, "skills", "pipeline", "no-such-engine", "SKILL.md", ); + const expectedAttachmentsFlatPath = join( + roots.byName.mattstack!.dir, + "attachments", + "no-such-engine", + "SKILL.md", + ); + const expectedAttachmentsGroupPath = join( + roots.byName.mattstack!.dir, + "attachments", + "pipeline", + "no-such-engine", + "SKILL.md", + ); let thrown: unknown; try { @@ -177,7 +198,19 @@ describe("loadStepSource", () => { } expect(thrown).toBeInstanceOf(Error); - expect((thrown as Error).message).toContain(expectedSearchedPath); + expect((thrown as Error).message).toContain(expectedSkillsPath); + expect((thrown as Error).message).toContain(expectedAttachmentsFlatPath); + expect((thrown as Error).message).toContain(expectedAttachmentsGroupPath); + }); + + test("falls back to attachments///SKILL.md for an engine moved out of skills/", () => { + const { roots } = makeFixtureRoots(); + + const step = loadStepSource("ship", roots); + + expect(step.name).toBe("ship"); + expect(step.dir.endsWith(join("attachments", "pipeline", "ship"))).toBe(true); + expect(step.body).toBe("Ship it."); }); }); @@ -334,6 +367,7 @@ describe("invocableRoster", () => { expect(roster.has("mattstack:untyped-step")).toBe(true); expect(roster.has("claimview:cvi-gates")).toBe(true); expect(roster.has("claimview:watch-ci-domain")).toBe(false); + expect(roster.has("mattstack:ship")).toBe(false); }); }); diff --git a/lib/skills/sources.ts b/lib/skills/sources.ts index 1ab50f68..e238ff7f 100644 --- a/lib/skills/sources.ts +++ b/lib/skills/sources.ts @@ -149,6 +149,7 @@ export function loadStepSource(engineName: string, roots: PluginRoots): StepSour } const skillsDir = join(mattstack.dir, "skills"); + const attachmentsDir = join(mattstack.dir, "attachments"); const searched: string[] = []; let foundDir: string | null = null; @@ -161,9 +162,32 @@ export function loadStepSource(engineName: string, roots: PluginRoots): StepSour } } + // Non-public engines live unregistered under attachments/ once moved out of + // skills/ (registration under skills/ is what puts an engine in the user's + // slash autocomplete); fall back there in the same shape loadAttachment + // searches -- flat, then one group level deep. + if (!foundDir) { + const flatCandidate = join(attachmentsDir, engineName, "SKILL.md"); + searched.push(flatCandidate); + if (existsSync(flatCandidate)) { + foundDir = join(attachmentsDir, engineName); + } + } + + if (!foundDir) { + for (const group of listDirs(attachmentsDir)) { + const candidate = join(attachmentsDir, group, engineName, "SKILL.md"); + searched.push(candidate); + if (existsSync(candidate)) { + foundDir = join(attachmentsDir, group, engineName); + break; + } + } + } + if (!foundDir) { throw new Error( - `loadStepSource: engine "${engineName}" not found under ${skillsDir}/*; searched:\n${searched.join("\n")}`, + `loadStepSource: engine "${engineName}" not found under ${skillsDir}/* or ${attachmentsDir}/*; searched:\n${searched.join("\n")}`, ); } From 1ba9731ea0f4dbd7e9c58d631589b1083b4dea79 Mon Sep 17 00:00:00 2001 From: Matthew Goodwin Date: Fri, 21 Aug 2026 14:12:36 -0500 Subject: [PATCH 17/17] fix: address CodeRabbit findings on the skills surface/check PR - skills-surface.test.ts: stub process.stdin.isTTY false in the non-tty palette test so it never spawns real fzf on an interactive run. - skills.test.ts, skills-surface.test.ts: afterEach assigns process.exitCode = 0, not undefined -- Bun ignores undefined once the code is truthy, so undefined could leak a nonzero exit into the suite. - skills.ts skillsCheck: compare the emitted path set against the actual outDir contents (extra files report as stale orphans) and treat a missing outDir for a public verb as stale instead of skipping it. - skills.ts computeRows/runPalette: include surface.public names absent from skills/, attachments/, and stubs.jsonc as rows (kind "missing") so the palette's delta preview and write no longer silently drop them. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012sKRekfZXwMyEiEajouQHY --- commands/__tests__/skills-surface.test.ts | 70 ++++++++++++++++++++++- commands/__tests__/skills.test.ts | 55 +++++++++++++++++- commands/skills.ts | 55 +++++++++++++++--- 3 files changed, 168 insertions(+), 12 deletions(-) diff --git a/commands/__tests__/skills-surface.test.ts b/commands/__tests__/skills-surface.test.ts index 57fc5e22..7571e479 100644 --- a/commands/__tests__/skills-surface.test.ts +++ b/commands/__tests__/skills-surface.test.ts @@ -3,7 +3,7 @@ import { execFileSync } from "child_process"; import { existsSync, mkdirSync, mkdtempSync, readFileSync, realpathSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import { dirname, join } from "path"; -import { decidePaletteAction, skillsSurface } from "../skills.ts"; +import { computeRows, decidePaletteAction, skillsSurface } from "../skills.ts"; function writeFile(path: string, content: string): void { mkdirSync(dirname(path), { recursive: true }); @@ -49,7 +49,8 @@ beforeEach(() => { afterEach(() => { logSpy.mockRestore(); - process.exitCode = undefined; + // Bun ignores process.exitCode = undefined once truthy; 0 is the only value that clears it. + process.exitCode = 0; }); async function runExpectingCleanExit(fn: () => Promise): Promise<{ exitCode: number | undefined; errors: string[] }> { @@ -295,7 +296,15 @@ describe("skillsSurface bare invocation (fzf palette)", () => { writeStubs(packDir, {}); writeFile(join(packDir, "skills", "my-skill", "SKILL.md"), "---\nname: s\n---\nbody\n"); - await skillsSurface(["--team", "t", "--pack-dir", packDir]); + // bun test inherits stdin from the shell; force the non-tty fallback so this + // never spawns real fzf on an interactive run. + const previousIsTTY = process.stdin.isTTY; + Object.defineProperty(process.stdin, "isTTY", { value: false, configurable: true }); + try { + await skillsSurface(["--team", "t", "--pack-dir", packDir]); + } finally { + Object.defineProperty(process.stdin, "isTTY", { value: previousIsTTY, configurable: true }); + } expect(existsSync(join(packDir, "pack", "surface.jsonc"))).toBe(false); const out = logs.join("\n"); @@ -372,3 +381,58 @@ describe("decidePaletteAction", () => { expect(decidePaletteAction(previousPublic, resultRows, true).kind).toBe("no-changes"); }); }); + +describe("computeRows -- previously-public names absent from skills/, attachments/, stubs.jsonc", () => { + test("surfaces the orphan as a 'missing' row instead of dropping it", () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + writeFile(join(packDir, "skills", "real-skill", "SKILL.md"), "---\nname: real-skill\n---\nbody\n"); + + const surface = { public: ["ghost", "real-skill"] }; + const { rows } = computeRows(packDir, new Set(), surface); + + const ghostRow = rows.find((r) => r.name === "ghost"); + expect(ghostRow).toEqual({ name: "ghost", kind: "missing", status: "public" }); + const realRow = rows.find((r) => r.name === "real-skill"); + expect(realRow?.status).toBe("public"); + }); + + test("combined with decidePaletteAction: leaving the missing row untouched yields no delta", () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + + const surface = { public: ["ghost"] }; + const { rows } = computeRows(packDir, new Set(), surface); + const previousPublic = new Set(surface.public); + + // Simulates the palette round trip: the fzf row for "ghost" exists and stays + // preselected because the user never touched it. + const resultRows = rows.map((r) => ({ name: r.name, status: r.status })); + + const action = decidePaletteAction(previousPublic, resultRows, false); + expect(action.kind).toBe("no-changes"); + }); + + test("combined with decidePaletteAction: explicitly demoting the missing row surfaces the removal", () => { + const packDir = makePackDir(); + writeStubs(packDir, {}); + + const surface = { public: ["ghost"] }; + const { rows } = computeRows(packDir, new Set(), surface); + const previousPublic = new Set(surface.public); + + const resultRows = rows.map((r) => ({ name: r.name, status: "internal" as const })); + + const preview = decidePaletteAction(previousPublic, resultRows, false); + expect(preview.kind).toBe("declined"); + if (preview.kind !== "no-changes") { + expect(preview.delta.toInternal).toEqual(["ghost"]); + } + + const decision = decidePaletteAction(previousPublic, resultRows, true); + expect(decision.kind).toBe("write"); + if (decision.kind === "write") { + expect(decision.delta.toInternal).toEqual(["ghost"]); + } + }); +}); diff --git a/commands/__tests__/skills.test.ts b/commands/__tests__/skills.test.ts index b2457bf6..63acf1b1 100644 --- a/commands/__tests__/skills.test.ts +++ b/commands/__tests__/skills.test.ts @@ -149,8 +149,9 @@ beforeEach(() => { afterEach(() => { logSpy.mockRestore(); - // skillsCheck sets this on staleness; never let it leak into the suite's own exit status. - process.exitCode = undefined; + // skillsCheck sets this on staleness; Bun ignores process.exitCode = undefined once + // truthy, so 0 is the only value that clears it before the suite's own exit status. + process.exitCode = 0; }); /** @@ -396,4 +397,54 @@ describe("skillsCheck", () => { expect(staleLine).toContain("SKILL.md"); expect(process.exitCode).toBe(1); }); + + test("orphan file left over from a previous compile reports stale and exits 1", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + await skillsCompile([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + writeFileSync(join(packDir, "skills", "watch-ci", "leftover.txt"), "stale content\n"); + logs = []; + + await skillsCheck([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + const staleLine = logs.find((l) => l.includes("stale")); + expect(staleLine).toBeDefined(); + expect(staleLine).toContain("watch-ci"); + expect(staleLine).toContain("leftover.txt"); + expect(process.exitCode).toBe(1); + }); + + test("never-compiled public verb (missing outDir) reports stale and exits 1", async () => { + const mattstackDir = makeMattstackDir(); + const packDir = makePackDir(); + const manifestPath = makeManifest("t"); + + await skillsCheck([ + "--team", "t", + "--pack-dir", packDir, + "--mattstack-dir", mattstackDir, + "--manifest", manifestPath, + "--verb", "watch-ci", + ]); + + const staleLine = logs.find((l) => l.includes("stale")); + expect(staleLine).toBeDefined(); + expect(staleLine).toContain("watch-ci"); + expect(process.exitCode).toBe(1); + }); }); diff --git a/commands/skills.ts b/commands/skills.ts index 464cb69e..27dc95d8 100644 --- a/commands/skills.ts +++ b/commands/skills.ts @@ -115,6 +115,20 @@ function listSubdirs(dir: string): string[] { .sort(); } +function listFilesRecursive(dir: string, prefix = ""): string[] { + if (!existsSync(dir)) return []; + const files: string[] = []; + for (const entry of readdirSync(dir, { withFileTypes: true })) { + const relPath = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + files.push(...listFilesRecursive(join(dir, entry.name), relPath)); + } else { + files.push(relPath); + } + } + return files; +} + function findDefaultManifest(mattstackRoot: string, team: string): string { const reposRoot = join(mattstackRoot, "repos"); const candidates: { path: string; mtimeMs: number }[] = []; @@ -330,15 +344,26 @@ export async function skillsCheck(args: string[]): Promise { await withCleanErrors(async () => { const flags = parseFlags(args); const resolved = resolve(flags); + // No surface.jsonc means the pack has no public/internal split yet -- every roster verb is public. + const publicSet = resolved.surface ? new Set(resolved.surface.public) : null; let anyStale = false; for (const verb of resolved.roster) { const outDir = join(resolved.packDir, "skills", verb.name); - if (!existsSync(outDir)) continue; + const isPublic = !publicSet || publicSet.has(verb.name); + + if (!existsSync(outDir)) { + if (isPublic) { + anyStale = true; + console.log(`${verb.name}: stale (never compiled -- outDir missing; run rt skills compile)`); + } + continue; + } const result = compileVerb(verb, resolved); const staleFiles: string[] = []; + const expectedPaths = new Set(result.files.map((f) => f.path)); for (const file of result.files) { const dest = join(outDir, file.path); @@ -348,6 +373,12 @@ export async function skillsCheck(args: string[]): Promise { } } + // A file left behind by an earlier compile: writeCompiledVerb would delete it on + // the next real compile, so "current" here would be a false clean bill of health. + for (const onDisk of listFilesRecursive(outDir)) { + if (!expectedPaths.has(onDisk)) staleFiles.push(`${onDisk} (orphan)`); + } + if (staleFiles.length > 0) { anyStale = true; console.log(`${verb.name}: stale (recompile or investigate drift with git diff) -- ${staleFiles.join(", ")}`); @@ -370,7 +401,11 @@ type SurfaceFlags = { manifest: string | null; }; -type SurfaceRow = { name: string; kind: "compiled" | "hand-authored"; status: "public" | "internal" }; +type SurfaceRow = { name: string; kind: "compiled" | "hand-authored" | "missing"; status: "public" | "internal" }; + +function kindLabel(kind: SurfaceRow["kind"]): string { + return kind === "missing" ? "(no files on disk)" : kind; +} function parseSurfaceFlags(args: string[]): { flags: SurfaceFlags; rest: string[] } { let team = "claimview"; @@ -427,7 +462,7 @@ function defaultPublicSet(skillsNames: Set, verbNames: Set): Set return new Set([...skillsNames, ...verbNames]); } -function computeRows( +export function computeRows( packDir: string, verbNames: Set, surface: SurfaceConfig | null, @@ -438,7 +473,13 @@ function computeRows( ? "pack/surface.jsonc" : "(no surface.jsonc yet -- inferred from current skills/ + stubs.jsonc placement)"; - const rows = [...allNames].sort().map((name) => { + // A name in surface.jsonc's public list but absent from skills/, attachments/, and + // stubs.jsonc would otherwise never become a row -- the palette write derives the new + // public list from rows alone, so omitting it here means the write silently drops it. + const names = new Set(allNames); + for (const name of publicSet) names.add(name); + + const rows = [...names].sort().map((name) => { const dir = skillsNames.has(name) ? join(packDir, "skills", name) : attachmentNames.has(name) @@ -446,7 +487,7 @@ function computeRows( : null; return { name, - kind: classify(name, dir, verbNames), + kind: allNames.has(name) ? classify(name, dir, verbNames) : ("missing" as const), status: (publicSet.has(name) ? "public" : "internal") as "public" | "internal", }; }); @@ -497,7 +538,7 @@ function printSurfaceRows(flags: SurfaceFlags, source: string, rows: SurfaceRow[ console.log(`rt skills surface -- team ${flags.team}`); console.log(`source: ${source}`); for (const row of rows) { - console.log(` ${row.status.padEnd(9)}${row.kind.padEnd(15)}${row.name}`); + console.log(` ${row.status.padEnd(9)}${kindLabel(row.kind).padEnd(15)}${row.name}`); } } @@ -652,7 +693,7 @@ async function runPalette(flags: SurfaceFlags): Promise { : "load:pos(1)"; const input = rows - .map((row) => `${row.name}\t${row.status.padEnd(9)}${row.kind.padEnd(15)}${row.name}`) + .map((row) => `${row.name}\t${row.status.padEnd(9)}${kindLabel(row.kind).padEnd(15)}${row.name}`) .join("\n"); const result = spawnSync(