From 05b7b813385daf17e3a2e270b3db6a13da298ad2 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 19 Dec 2025 00:11:39 +0100 Subject: [PATCH] Skills standard compliance Implement Agent Skills standard (https://agentskills.io/specification): - Validate name (must match parent dir, lowercase, max 64 chars) - Validate description (required, max 1024 chars) - Warn on unknown frontmatter fields - Warn on name collisions (keep first) - Change prompt format to XML structure - Remove {baseDir} placeholder (use relative paths) - Add tests and update documentation fixes #231 --- packages/ai/src/models.generated.ts | 46 +-- packages/coding-agent/CHANGELOG.md | 4 + packages/coding-agent/README.md | 15 +- packages/coding-agent/docs/skills.md | 89 +++-- packages/coding-agent/src/core/skills.ts | 312 +++++++++++++----- .../coding-agent/src/core/system-prompt.ts | 4 +- packages/coding-agent/src/index.ts | 2 + .../src/modes/interactive/interactive-mode.ts | 9 +- .../skills-collision/first/calendar/SKILL.md | 8 + .../skills-collision/second/calendar/SKILL.md | 8 + .../skills/consecutive-hyphens/SKILL.md | 8 + .../skills/invalid-name-chars/SKILL.md | 8 + .../test/fixtures/skills/long-name/SKILL.md | 8 + .../skills/missing-description/SKILL.md | 7 + .../fixtures/skills/name-mismatch/SKILL.md | 8 + .../skills/nested/child-skill/SKILL.md | 8 + .../fixtures/skills/no-frontmatter/SKILL.md | 3 + .../fixtures/skills/unknown-field/SKILL.md | 10 + .../test/fixtures/skills/valid-skill/SKILL.md | 8 + packages/coding-agent/test/skills.test.ts | 272 +++++++++++++++ packages/mom/src/agent.ts | 4 +- 21 files changed, 692 insertions(+), 149 deletions(-) create mode 100644 packages/coding-agent/test/fixtures/skills-collision/first/calendar/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills-collision/second/calendar/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/consecutive-hyphens/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/invalid-name-chars/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/long-name/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/missing-description/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/name-mismatch/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/nested/child-skill/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/no-frontmatter/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/unknown-field/SKILL.md create mode 100644 packages/coding-agent/test/fixtures/skills/valid-skill/SKILL.md create mode 100644 packages/coding-agent/test/skills.test.ts diff --git a/packages/ai/src/models.generated.ts b/packages/ai/src/models.generated.ts index 41981e3c..0b6d2130 100644 --- a/packages/ai/src/models.generated.ts +++ b/packages/ai/src/models.generated.ts @@ -6003,9 +6003,9 @@ export const MODELS = { contextWindow: 32768, maxTokens: 4096, } satisfies Model<"openai-completions">, - "anthropic/claude-3.5-haiku": { - id: "anthropic/claude-3.5-haiku", - name: "Anthropic: Claude 3.5 Haiku", + "anthropic/claude-3.5-haiku-20241022": { + id: "anthropic/claude-3.5-haiku-20241022", + name: "Anthropic: Claude 3.5 Haiku (2024-10-22)", api: "openai-completions", provider: "openrouter", baseUrl: "https://openrouter.ai/api/v1", @@ -6020,9 +6020,9 @@ export const MODELS = { contextWindow: 200000, maxTokens: 8192, } satisfies Model<"openai-completions">, - "anthropic/claude-3.5-haiku-20241022": { - id: "anthropic/claude-3.5-haiku-20241022", - name: "Anthropic: Claude 3.5 Haiku (2024-10-22)", + "anthropic/claude-3.5-haiku": { + id: "anthropic/claude-3.5-haiku", + name: "Anthropic: Claude 3.5 Haiku", api: "openai-completions", provider: "openrouter", baseUrl: "https://openrouter.ai/api/v1", @@ -6258,23 +6258,6 @@ export const MODELS = { contextWindow: 128000, maxTokens: 16384, } satisfies Model<"openai-completions">, - "meta-llama/llama-3.1-405b-instruct": { - id: "meta-llama/llama-3.1-405b-instruct", - name: "Meta: Llama 3.1 405B Instruct", - api: "openai-completions", - provider: "openrouter", - baseUrl: "https://openrouter.ai/api/v1", - reasoning: false, - input: ["text"], - cost: { - input: 3.5, - output: 3.5, - cacheRead: 0, - cacheWrite: 0, - }, - contextWindow: 130815, - maxTokens: 4096, - } satisfies Model<"openai-completions">, "meta-llama/llama-3.1-8b-instruct": { id: "meta-llama/llama-3.1-8b-instruct", name: "Meta: Llama 3.1 8B Instruct", @@ -6292,6 +6275,23 @@ export const MODELS = { contextWindow: 131072, maxTokens: 16384, } satisfies Model<"openai-completions">, + "meta-llama/llama-3.1-405b-instruct": { + id: "meta-llama/llama-3.1-405b-instruct", + name: "Meta: Llama 3.1 405B Instruct", + api: "openai-completions", + provider: "openrouter", + baseUrl: "https://openrouter.ai/api/v1", + reasoning: false, + input: ["text"], + cost: { + input: 3.5, + output: 3.5, + cacheRead: 0, + cacheWrite: 0, + }, + contextWindow: 130815, + maxTokens: 4096, + } satisfies Model<"openai-completions">, "meta-llama/llama-3.1-70b-instruct": { id: "meta-llama/llama-3.1-70b-instruct", name: "Meta: Llama 3.1 70B Instruct", diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 63a725bb..d92471b6 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Changed + +- **Skills standard compliance**: Skills now adhere to the [Agent Skills standard](https://agentskills.io/specification). Validates name (must match parent directory, lowercase, max 64 chars), description (required, max 1024 chars), and frontmatter fields. Warns on violations but remains lenient. Prompt format changed to XML structure. Removed `{baseDir}` placeholder in favor of relative paths. ([#231](https://github.com/badlogic/pi-mono/issues/231)) + ## [0.23.4] - 2025-12-18 ### Added diff --git a/packages/coding-agent/README.md b/packages/coding-agent/README.md index b4b8155e..6358cd4d 100644 --- a/packages/coding-agent/README.md +++ b/packages/coding-agent/README.md @@ -488,7 +488,9 @@ Usage: `/component Button "onClick handler" "disabled support"` ### Skills -Skills are self-contained capability packages that the agent loads on-demand. A skill provides specialized workflows, setup instructions, helper scripts, and reference documentation for specific tasks. Skills are loaded when the agent decides a task matches the description, or when you explicitly ask to use one. +Skills are self-contained capability packages that the agent loads on-demand. Pi implements the [Agent Skills standard](https://agentskills.io/specification), warning about violations but remaining lenient. + +A skill provides specialized workflows, setup instructions, helper scripts, and reference documentation for specific tasks. Skills are loaded when the agent decides a task matches the description, or when you explicitly ask to use one. **Example use cases:** - Web search and content extraction (Brave Search API) @@ -508,6 +510,7 @@ Skills are self-contained capability packages that the agent loads on-demand. A ```markdown --- +name: brave-search description: Web search via Brave Search API. Use for documentation, facts, or web content. --- @@ -515,18 +518,18 @@ description: Web search via Brave Search API. Use for documentation, facts, or w ## Setup \`\`\`bash -cd {baseDir} && npm install +cd /path/to/brave-search && npm install \`\`\` ## Usage \`\`\`bash -{baseDir}/search.js "query" # Basic search -{baseDir}/search.js "query" --content # Include page content +./search.js "query" # Basic search +./search.js "query" --content # Include page content \`\`\` ``` -- `description`: Required. Determines when the skill is loaded. -- `{baseDir}`: Placeholder for the skill's directory. +- `name`: Required. Must match parent directory name. Lowercase, hyphens, max 64 chars. +- `description`: Required. Max 1024 chars. Determines when the skill is loaded. **Disable skills:** `pi --no-skills` or set `skills.enabled: false` in settings. diff --git a/packages/coding-agent/docs/skills.md b/packages/coding-agent/docs/skills.md index 745906f7..de433888 100644 --- a/packages/coding-agent/docs/skills.md +++ b/packages/coding-agent/docs/skills.md @@ -2,6 +2,8 @@ Skills are self-contained capability packages that the agent loads on-demand. A skill provides specialized workflows, setup instructions, helper scripts, and reference documentation for specific tasks. +Pi implements the [Agent Skills standard](https://agentskills.io/specification). + **Example use cases:** - Web search and content extraction (Brave Search API) - Browser automation via Chrome DevTools Protocol @@ -65,14 +67,13 @@ description: What this skill does and when to use it. Be specific. Run once before first use: \`\`\`bash -cd {baseDir} -npm install +cd /path/to/skill && npm install \`\`\` ## Usage \`\`\`bash -{baseDir}/scripts/process.sh +./scripts/process.sh \`\`\` ## Workflow @@ -84,20 +85,54 @@ npm install ### Frontmatter Fields -| Field | Required | Description | +Per the [Agent Skills specification](https://agentskills.io/specification#frontmatter-required): + +| Field | Required | Constraints | |-------|----------|-------------| -| `description` | Yes | What the skill does and when to use it | -| `name` | No | Override skill name (defaults to directory name) | +| `name` | Yes | Max 64 chars. Lowercase a-z, 0-9, hyphens only. Must match parent directory name. | +| `description` | Yes | Max 1024 chars. What the skill does and when to use it. | +| `license` | No | License name or reference to bundled license file. | +| `compatibility` | No | Max 500 chars. Environment requirements (system packages, network access, etc.). | +| `metadata` | No | Arbitrary key-value mapping for additional metadata. | +| `allowed-tools` | No | Space-delimited list of pre-approved tools (experimental). | -The `description` is critical. It's shown in the system prompt and determines when the agent loads the skill. Be specific about both what it does and when to use it. +#### Name Validation -### The `{baseDir}` Placeholder +The `name` field must: +- Be 1-64 characters +- Contain only lowercase letters (a-z), numbers (0-9), and hyphens +- Not start or end with a hyphen +- Not contain consecutive hyphens (--) +- Match the parent directory name exactly -Use `{baseDir}` to reference files in the skill's directory. The agent sees each skill's base directory and substitutes it when following instructions: +Valid: `pdf-processing`, `data-analysis`, `code-review` +Invalid: `PDF-Processing`, `-pdf`, `pdf--processing` + +#### Description Best Practices + +The `description` is critical. It determines when the agent loads the skill. Be specific about both what it does and when to use it. + +Good: +```yaml +description: Extracts text and tables from PDF files, fills PDF forms, and merges multiple PDFs. Use when working with PDF documents or when the user mentions PDFs, forms, or document extraction. +``` + +Poor: +```yaml +description: Helps with PDFs. +``` + +### File References + +Use relative paths from the skill directory: ```markdown -Helper scripts: {baseDir}/scripts/ -Config template: {baseDir}/assets/config.json +See [the reference guide](references/REFERENCE.md) for details. + +Run the extraction script: +\`\`\`bash +./scripts/extract.py input.pdf +\`\`\` ``` ## Skill Locations @@ -110,21 +145,28 @@ Skills are discovered from these locations (later wins on name collision): 4. `~/.pi/agent/skills/**/SKILL.md` (Pi user, recursive) 5. `/.pi/skills/**/SKILL.md` (Pi project, recursive) -### Subdirectory Naming - -Pi skills in subdirectories use colon-separated names: -- `~/.pi/agent/skills/db/migrate/SKILL.md` → `db:migrate` -- `/.pi/skills/aws/s3/upload/SKILL.md` → `aws:s3:upload` - ## How Skills Work 1. At startup, pi scans skill locations and extracts names + descriptions -2. The system prompt includes a list of available skills with their descriptions +2. The system prompt includes available skills in XML format 3. When a task matches, the agent uses `read` to load the full SKILL.md -4. The agent follows the instructions, using `{baseDir}` to reference scripts/assets +4. The agent follows the instructions, using relative paths to reference scripts/assets This is progressive disclosure: only descriptions are always in context, full instructions load on-demand. +## Validation Warnings + +Pi validates skills against the Agent Skills standard and warns (but still loads) non-compliant skills: + +- Name doesn't match parent directory +- Name exceeds 64 characters +- Name contains invalid characters +- Name starts/ends with hyphen or has consecutive hyphens +- Description missing or exceeds 1024 characters +- Unknown frontmatter fields + +Name collisions (same name from different locations) warn and keep the first skill found. + ## Example: Web Search Skill ``` @@ -146,21 +188,20 @@ description: Web search and content extraction via Brave Search API. Use for sea ## Setup \`\`\`bash -cd {baseDir} -npm install +cd /path/to/brave-search && npm install \`\`\` ## Search \`\`\`bash -{baseDir}/search.js "query" # Basic search -{baseDir}/search.js "query" --content # Include page content +./search.js "query" # Basic search +./search.js "query" --content # Include page content \`\`\` ## Extract Page Content \`\`\`bash -{baseDir}/content.js https://example.com +./content.js https://example.com \`\`\` ``` diff --git a/packages/coding-agent/src/core/skills.ts b/packages/coding-agent/src/core/skills.ts index a20abd52..969a82ce 100644 --- a/packages/coding-agent/src/core/skills.ts +++ b/packages/coding-agent/src/core/skills.ts @@ -3,9 +3,29 @@ import { homedir } from "os"; import { basename, dirname, join, resolve } from "path"; import { CONFIG_DIR_NAME } from "../config.js"; +/** + * Standard frontmatter fields per Agent Skills spec. + * See: https://agentskills.io/specification#frontmatter-required + */ +const ALLOWED_FRONTMATTER_FIELDS = new Set([ + "name", + "description", + "license", + "compatibility", + "metadata", + "allowed-tools", +]); + +/** Max name length per spec */ +const MAX_NAME_LENGTH = 64; + +/** Max description length per spec */ +const MAX_DESCRIPTION_LENGTH = 1024; + export interface SkillFrontmatter { name?: string; - description: string; + description?: string; + [key: string]: unknown; } export interface Skill { @@ -16,6 +36,16 @@ export interface Skill { source: string; } +export interface SkillWarning { + skillPath: string; + message: string; +} + +export interface LoadSkillsResult { + skills: Skill[]; + warnings: SkillWarning[]; +} + type SkillFormat = "recursive" | "claude"; function stripQuotes(value: string): string { @@ -25,28 +55,30 @@ function stripQuotes(value: string): string { return value; } -function parseFrontmatter(content: string): { frontmatter: SkillFrontmatter; body: string } { - const frontmatter: SkillFrontmatter = { description: "" }; +function parseFrontmatter(content: string): { frontmatter: SkillFrontmatter; body: string; allKeys: string[] } { + const frontmatter: SkillFrontmatter = {}; + const allKeys: string[] = []; const normalizedContent = content.replace(/\r\n/g, "\n").replace(/\r/g, "\n"); if (!normalizedContent.startsWith("---")) { - return { frontmatter, body: normalizedContent }; + return { frontmatter, body: normalizedContent, allKeys }; } const endIndex = normalizedContent.indexOf("\n---", 3); if (endIndex === -1) { - return { frontmatter, body: normalizedContent }; + return { frontmatter, body: normalizedContent, allKeys }; } const frontmatterBlock = normalizedContent.slice(4, endIndex); const body = normalizedContent.slice(endIndex + 4).trim(); for (const line of frontmatterBlock.split("\n")) { - const match = line.match(/^(\w+):\s*(.*)$/); + const match = line.match(/^(\w[\w-]*):\s*(.*)$/); if (match) { const key = match[1]; const value = stripQuotes(match[2].trim()); + allKeys.push(key); if (key === "name") { frontmatter.name = value; } else if (key === "description") { @@ -55,7 +87,65 @@ function parseFrontmatter(content: string): { frontmatter: SkillFrontmatter; bod } } - return { frontmatter, body }; + return { frontmatter, body, allKeys }; +} + +/** + * Validate skill name per Agent Skills spec. + * Returns array of validation error messages (empty if valid). + */ +function validateName(name: string, parentDirName: string): string[] { + const errors: string[] = []; + + if (name !== parentDirName) { + errors.push(`name "${name}" does not match parent directory "${parentDirName}"`); + } + + if (name.length > MAX_NAME_LENGTH) { + errors.push(`name exceeds ${MAX_NAME_LENGTH} characters (${name.length})`); + } + + if (!/^[a-z0-9-]+$/.test(name)) { + errors.push(`name contains invalid characters (must be lowercase a-z, 0-9, hyphens only)`); + } + + if (name.startsWith("-") || name.endsWith("-")) { + errors.push(`name must not start or end with a hyphen`); + } + + if (name.includes("--")) { + errors.push(`name must not contain consecutive hyphens`); + } + + return errors; +} + +/** + * Validate description per Agent Skills spec. + */ +function validateDescription(description: string | undefined): string[] { + const errors: string[] = []; + + if (!description || description.trim() === "") { + errors.push(`description is required`); + } else if (description.length > MAX_DESCRIPTION_LENGTH) { + errors.push(`description exceeds ${MAX_DESCRIPTION_LENGTH} characters (${description.length})`); + } + + return errors; +} + +/** + * Check for unknown frontmatter fields. + */ +function validateFrontmatterFields(keys: string[]): string[] { + const errors: string[] = []; + for (const key of keys) { + if (!ALLOWED_FRONTMATTER_FIELDS.has(key)) { + errors.push(`unknown frontmatter field "${key}"`); + } + } + return errors; } export interface LoadSkillsFromDirOptions { @@ -63,30 +153,23 @@ export interface LoadSkillsFromDirOptions { dir: string; /** Source identifier for these skills */ source: string; - /** Use colon-separated path names (e.g., db:migrate) instead of simple directory name */ - useColonPath?: boolean; } /** * Load skills from a directory recursively. * Skills are directories containing a SKILL.md file with frontmatter including a description. */ -export function loadSkillsFromDir(options: LoadSkillsFromDirOptions, subdir: string = ""): Skill[] { - const { dir, source, useColonPath = false } = options; - return loadSkillsFromDirInternal(dir, source, "recursive", useColonPath, subdir); +export function loadSkillsFromDir(options: LoadSkillsFromDirOptions): LoadSkillsResult { + const { dir, source } = options; + return loadSkillsFromDirInternal(dir, source, "recursive"); } -function loadSkillsFromDirInternal( - dir: string, - source: string, - format: SkillFormat, - useColonPath: boolean = false, - subdir: string = "", -): Skill[] { +function loadSkillsFromDirInternal(dir: string, source: string, format: SkillFormat): LoadSkillsResult { const skills: Skill[] = []; + const warnings: SkillWarning[] = []; if (!existsSync(dir)) { - return skills; + return { skills, warnings }; } try { @@ -106,30 +189,15 @@ function loadSkillsFromDirInternal( if (format === "recursive") { // Recursive format: scan directories, look for SKILL.md files if (entry.isDirectory()) { - const newSubdir = subdir ? `${subdir}:${entry.name}` : entry.name; - skills.push(...loadSkillsFromDirInternal(fullPath, source, format, useColonPath, newSubdir)); + const subResult = loadSkillsFromDirInternal(fullPath, source, format); + skills.push(...subResult.skills); + warnings.push(...subResult.warnings); } else if (entry.isFile() && entry.name === "SKILL.md") { - try { - const rawContent = readFileSync(fullPath, "utf-8"); - const { frontmatter } = parseFrontmatter(rawContent); - - if (!frontmatter.description) { - continue; - } - - const skillDir = dirname(fullPath); - // useColonPath: db:migrate (pi), otherwise just: migrate (codex) - const nameFromPath = useColonPath ? subdir || basename(skillDir) : basename(skillDir); - const name = frontmatter.name || nameFromPath; - - skills.push({ - name, - description: frontmatter.description, - filePath: fullPath, - baseDir: skillDir, - source, - }); - } catch {} + const result = loadSkillFromFile(fullPath, source); + if (result.skill) { + skills.push(result.skill); + } + warnings.push(...result.warnings); } } else if (format === "claude") { // Claude format: only one level deep, each directory must contain SKILL.md @@ -137,40 +205,77 @@ function loadSkillsFromDirInternal( continue; } - const skillDir = fullPath; - const skillFile = join(skillDir, "SKILL.md"); - + const skillFile = join(fullPath, "SKILL.md"); if (!existsSync(skillFile)) { continue; } - try { - const rawContent = readFileSync(skillFile, "utf-8"); - const { frontmatter } = parseFrontmatter(rawContent); - - if (!frontmatter.description) { - continue; - } - - const name = frontmatter.name || entry.name; - - skills.push({ - name, - description: frontmatter.description, - filePath: skillFile, - baseDir: skillDir, - source, - }); - } catch {} + const result = loadSkillFromFile(skillFile, source); + if (result.skill) { + skills.push(result.skill); + } + warnings.push(...result.warnings); } } } catch {} - return skills; + return { skills, warnings }; +} + +function loadSkillFromFile(filePath: string, source: string): { skill: Skill | null; warnings: SkillWarning[] } { + const warnings: SkillWarning[] = []; + + try { + const rawContent = readFileSync(filePath, "utf-8"); + const { frontmatter, allKeys } = parseFrontmatter(rawContent); + const skillDir = dirname(filePath); + const parentDirName = basename(skillDir); + + // Validate frontmatter fields + const fieldErrors = validateFrontmatterFields(allKeys); + for (const error of fieldErrors) { + warnings.push({ skillPath: filePath, message: error }); + } + + // Validate description + const descErrors = validateDescription(frontmatter.description); + for (const error of descErrors) { + warnings.push({ skillPath: filePath, message: error }); + } + + // Use name from frontmatter, or fall back to parent directory name + const name = frontmatter.name || parentDirName; + + // Validate name + const nameErrors = validateName(name, parentDirName); + for (const error of nameErrors) { + warnings.push({ skillPath: filePath, message: error }); + } + + // Still load the skill even with warnings (unless description is completely missing) + if (!frontmatter.description || frontmatter.description.trim() === "") { + return { skill: null, warnings }; + } + + return { + skill: { + name, + description: frontmatter.description, + filePath, + baseDir: skillDir, + source, + }, + warnings, + }; + } catch { + return { skill: null, warnings }; + } } /** * Format skills for inclusion in a system prompt. + * Uses XML format per Agent Skills standard. + * See: https://agentskills.io/integrate-skills */ export function formatSkillsForPrompt(skills: Skill[]): string { if (skills.length === 0) { @@ -178,16 +283,18 @@ export function formatSkillsForPrompt(skills: Skill[]): string { } const lines = [ - "\n\n", - "The following skills provide specialized instructions for specific tasks.", + "\n\nThe following skills provide specialized instructions for specific tasks.", "Use the read tool to load a skill's file when the task matches its description.", - "Skills may contain {baseDir} placeholders - replace them with the skill's base directory path.\n", + "", + "", ]; for (const skill of skills) { - lines.push(`- ${skill.name}: ${skill.description}`); - lines.push(` File: ${skill.filePath}`); - lines.push(` Base directory: ${skill.baseDir}`); + lines.push(" "); + lines.push(` ${escapeXml(skill.name)}`); + lines.push(` ${escapeXml(skill.description)}`); + lines.push(` ${escapeXml(skill.filePath)}`); + lines.push(" "); } lines.push(""); @@ -195,36 +302,59 @@ export function formatSkillsForPrompt(skills: Skill[]): string { return lines.join("\n"); } -export function loadSkills(): Skill[] { - const skillMap = new Map(); +function escapeXml(str: string): string { + return str + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/"/g, """) + .replace(/'/g, "'"); +} - // Codex: recursive, simple directory name - const codexUserDir = join(homedir(), ".codex", "skills"); - for (const skill of loadSkillsFromDirInternal(codexUserDir, "codex-user", "recursive", false)) { - skillMap.set(skill.name, skill); +/** + * Load skills from all configured locations. + * Returns skills and any validation warnings. + */ +export function loadSkills(): LoadSkillsResult { + const skillMap = new Map(); + const allWarnings: SkillWarning[] = []; + const collisionWarnings: SkillWarning[] = []; + + function addSkills(result: LoadSkillsResult) { + allWarnings.push(...result.warnings); + for (const skill of result.skills) { + const existing = skillMap.get(skill.name); + if (existing) { + collisionWarnings.push({ + skillPath: skill.filePath, + message: `name collision: "${skill.name}" already loaded from ${existing.filePath}, skipping this one`, + }); + } else { + skillMap.set(skill.name, skill); + } + } } + // Codex: recursive + const codexUserDir = join(homedir(), ".codex", "skills"); + addSkills(loadSkillsFromDirInternal(codexUserDir, "codex-user", "recursive")); + // Claude: single level only const claudeUserDir = join(homedir(), ".claude", "skills"); - for (const skill of loadSkillsFromDirInternal(claudeUserDir, "claude-user", "claude", false)) { - skillMap.set(skill.name, skill); - } + addSkills(loadSkillsFromDirInternal(claudeUserDir, "claude-user", "claude")); const claudeProjectDir = resolve(process.cwd(), ".claude", "skills"); - for (const skill of loadSkillsFromDirInternal(claudeProjectDir, "claude-project", "claude", false)) { - skillMap.set(skill.name, skill); - } + addSkills(loadSkillsFromDirInternal(claudeProjectDir, "claude-project", "claude")); - // Pi: recursive, colon-separated path names + // Pi: recursive const globalSkillsDir = join(homedir(), CONFIG_DIR_NAME, "agent", "skills"); - for (const skill of loadSkillsFromDirInternal(globalSkillsDir, "user", "recursive", true)) { - skillMap.set(skill.name, skill); - } + addSkills(loadSkillsFromDirInternal(globalSkillsDir, "user", "recursive")); const projectSkillsDir = resolve(process.cwd(), CONFIG_DIR_NAME, "skills"); - for (const skill of loadSkillsFromDirInternal(projectSkillsDir, "project", "recursive", true)) { - skillMap.set(skill.name, skill); - } + addSkills(loadSkillsFromDirInternal(projectSkillsDir, "project", "recursive")); - return Array.from(skillMap.values()); + return { + skills: Array.from(skillMap.values()), + warnings: [...allWarnings, ...collisionWarnings], + }; } diff --git a/packages/coding-agent/src/core/system-prompt.ts b/packages/coding-agent/src/core/system-prompt.ts index 1b3d6ac0..b1305d30 100644 --- a/packages/coding-agent/src/core/system-prompt.ts +++ b/packages/coding-agent/src/core/system-prompt.ts @@ -149,7 +149,7 @@ export function buildSystemPrompt(options: BuildSystemPromptOptions = {}): strin // Append skills section (only if read tool is available) const customPromptHasRead = !selectedTools || selectedTools.includes("read"); if (skillsEnabled && customPromptHasRead) { - const skills = loadSkills(); + const { skills } = loadSkills(); prompt += formatSkillsForPrompt(skills); } @@ -255,7 +255,7 @@ Documentation: // Append skills section (only if read tool is available) if (skillsEnabled && hasRead) { - const skills = loadSkills(); + const { skills } = loadSkills(); prompt += formatSkillsForPrompt(skills); } diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 89f606cf..b4c313b0 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -81,10 +81,12 @@ export { export { formatSkillsForPrompt, type LoadSkillsFromDirOptions, + type LoadSkillsResult, loadSkills, loadSkillsFromDir, type Skill, type SkillFrontmatter, + type SkillWarning, } from "./core/skills.js"; // Tools export { bashTool, codingTools, editTool, readTool, writeTool } from "./core/tools/index.js"; diff --git a/packages/coding-agent/src/modes/interactive/interactive-mode.ts b/packages/coding-agent/src/modes/interactive/interactive-mode.ts index 35023b58..45a3515c 100644 --- a/packages/coding-agent/src/modes/interactive/interactive-mode.ts +++ b/packages/coding-agent/src/modes/interactive/interactive-mode.ts @@ -305,13 +305,20 @@ export class InteractiveMode { } // Show loaded skills - const skills = loadSkills(); + const { skills, warnings: skillWarnings } = loadSkills(); if (skills.length > 0) { const skillList = skills.map((s) => theme.fg("dim", ` ${s.filePath}`)).join("\n"); this.chatContainer.addChild(new Text(theme.fg("muted", "Loaded skills:\n") + skillList, 0, 0)); this.chatContainer.addChild(new Spacer(1)); } + // Show skill warnings if any + if (skillWarnings.length > 0) { + const warningList = skillWarnings.map((w) => theme.fg("warning", ` ${w.skillPath}: ${w.message}`)).join("\n"); + this.chatContainer.addChild(new Text(theme.fg("warning", "Skill warnings:\n") + warningList, 0, 0)); + this.chatContainer.addChild(new Spacer(1)); + } + // Show loaded custom tools if (this.customTools.size > 0) { const toolList = Array.from(this.customTools.values()) diff --git a/packages/coding-agent/test/fixtures/skills-collision/first/calendar/SKILL.md b/packages/coding-agent/test/fixtures/skills-collision/first/calendar/SKILL.md new file mode 100644 index 00000000..a2e355fc --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills-collision/first/calendar/SKILL.md @@ -0,0 +1,8 @@ +--- +name: calendar +description: First calendar skill. +--- + +# Calendar (First) + +This is the first calendar skill. diff --git a/packages/coding-agent/test/fixtures/skills-collision/second/calendar/SKILL.md b/packages/coding-agent/test/fixtures/skills-collision/second/calendar/SKILL.md new file mode 100644 index 00000000..d90dd154 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills-collision/second/calendar/SKILL.md @@ -0,0 +1,8 @@ +--- +name: calendar +description: Second calendar skill. +--- + +# Calendar (Second) + +This is the second calendar skill. diff --git a/packages/coding-agent/test/fixtures/skills/consecutive-hyphens/SKILL.md b/packages/coding-agent/test/fixtures/skills/consecutive-hyphens/SKILL.md new file mode 100644 index 00000000..76c5b6ea --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/consecutive-hyphens/SKILL.md @@ -0,0 +1,8 @@ +--- +name: bad--name +description: A skill with consecutive hyphens in the name. +--- + +# Consecutive Hyphens + +This skill has consecutive hyphens in its name. diff --git a/packages/coding-agent/test/fixtures/skills/invalid-name-chars/SKILL.md b/packages/coding-agent/test/fixtures/skills/invalid-name-chars/SKILL.md new file mode 100644 index 00000000..fc90d0c9 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/invalid-name-chars/SKILL.md @@ -0,0 +1,8 @@ +--- +name: Invalid_Name +description: A skill with invalid characters in the name. +--- + +# Invalid Name + +This skill has uppercase and underscore in the name. diff --git a/packages/coding-agent/test/fixtures/skills/long-name/SKILL.md b/packages/coding-agent/test/fixtures/skills/long-name/SKILL.md new file mode 100644 index 00000000..e2563b7b --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/long-name/SKILL.md @@ -0,0 +1,8 @@ +--- +name: this-is-a-very-long-skill-name-that-exceeds-the-sixty-four-character-limit-set-by-the-standard +description: A skill with a name that exceeds 64 characters. +--- + +# Long Name + +This skill's name is too long. diff --git a/packages/coding-agent/test/fixtures/skills/missing-description/SKILL.md b/packages/coding-agent/test/fixtures/skills/missing-description/SKILL.md new file mode 100644 index 00000000..b6031d48 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/missing-description/SKILL.md @@ -0,0 +1,7 @@ +--- +name: missing-description +--- + +# Missing Description + +This skill has no description field. diff --git a/packages/coding-agent/test/fixtures/skills/name-mismatch/SKILL.md b/packages/coding-agent/test/fixtures/skills/name-mismatch/SKILL.md new file mode 100644 index 00000000..cdc8cefe --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/name-mismatch/SKILL.md @@ -0,0 +1,8 @@ +--- +name: different-name +description: A skill with a name that doesn't match the directory. +--- + +# Name Mismatch + +This skill's name doesn't match its parent directory. diff --git a/packages/coding-agent/test/fixtures/skills/nested/child-skill/SKILL.md b/packages/coding-agent/test/fixtures/skills/nested/child-skill/SKILL.md new file mode 100644 index 00000000..ae43b964 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/nested/child-skill/SKILL.md @@ -0,0 +1,8 @@ +--- +name: child-skill +description: A nested skill in a subdirectory. +--- + +# Child Skill + +This skill is nested in a subdirectory. diff --git a/packages/coding-agent/test/fixtures/skills/no-frontmatter/SKILL.md b/packages/coding-agent/test/fixtures/skills/no-frontmatter/SKILL.md new file mode 100644 index 00000000..e14f6a31 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/no-frontmatter/SKILL.md @@ -0,0 +1,3 @@ +# No Frontmatter + +This skill has no YAML frontmatter at all. diff --git a/packages/coding-agent/test/fixtures/skills/unknown-field/SKILL.md b/packages/coding-agent/test/fixtures/skills/unknown-field/SKILL.md new file mode 100644 index 00000000..a7f6e4cc --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/unknown-field/SKILL.md @@ -0,0 +1,10 @@ +--- +name: unknown-field +description: A skill with an unknown frontmatter field. +author: someone +version: 1.0 +--- + +# Unknown Field + +This skill has non-standard frontmatter fields. diff --git a/packages/coding-agent/test/fixtures/skills/valid-skill/SKILL.md b/packages/coding-agent/test/fixtures/skills/valid-skill/SKILL.md new file mode 100644 index 00000000..1a76da29 --- /dev/null +++ b/packages/coding-agent/test/fixtures/skills/valid-skill/SKILL.md @@ -0,0 +1,8 @@ +--- +name: valid-skill +description: A valid skill for testing purposes. +--- + +# Valid Skill + +This is a valid skill that follows the Agent Skills standard. diff --git a/packages/coding-agent/test/skills.test.ts b/packages/coding-agent/test/skills.test.ts new file mode 100644 index 00000000..3266a73f --- /dev/null +++ b/packages/coding-agent/test/skills.test.ts @@ -0,0 +1,272 @@ +import { join, resolve } from "path"; +import { describe, expect, it } from "vitest"; +import { formatSkillsForPrompt, loadSkillsFromDir, type Skill } from "../src/core/skills.js"; + +const fixturesDir = resolve(__dirname, "fixtures/skills"); +const collisionFixturesDir = resolve(__dirname, "fixtures/skills-collision"); + +describe("skills", () => { + describe("loadSkillsFromDir", () => { + it("should load a valid skill", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "valid-skill"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(skills[0].name).toBe("valid-skill"); + expect(skills[0].description).toBe("A valid skill for testing purposes."); + expect(skills[0].source).toBe("test"); + expect(warnings).toHaveLength(0); + }); + + it("should warn when name doesn't match parent directory", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "name-mismatch"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(skills[0].name).toBe("different-name"); + expect(warnings.some((w) => w.message.includes("does not match parent directory"))).toBe(true); + }); + + it("should warn when name contains invalid characters", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "invalid-name-chars"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(warnings.some((w) => w.message.includes("invalid characters"))).toBe(true); + }); + + it("should warn when name exceeds 64 characters", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "long-name"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(warnings.some((w) => w.message.includes("exceeds 64 characters"))).toBe(true); + }); + + it("should warn and skip skill when description is missing", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "missing-description"), + source: "test", + }); + + expect(skills).toHaveLength(0); + expect(warnings.some((w) => w.message.includes("description is required"))).toBe(true); + }); + + it("should warn when unknown frontmatter fields are present", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "unknown-field"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(warnings.some((w) => w.message.includes('unknown frontmatter field "author"'))).toBe(true); + expect(warnings.some((w) => w.message.includes('unknown frontmatter field "version"'))).toBe(true); + }); + + it("should load nested skills recursively", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "nested"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(skills[0].name).toBe("child-skill"); + expect(warnings).toHaveLength(0); + }); + + it("should skip files without frontmatter", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "no-frontmatter"), + source: "test", + }); + + // no-frontmatter has no description, so it should be skipped + expect(skills).toHaveLength(0); + expect(warnings.some((w) => w.message.includes("description is required"))).toBe(true); + }); + + it("should warn when name contains consecutive hyphens", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: join(fixturesDir, "consecutive-hyphens"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(warnings.some((w) => w.message.includes("consecutive hyphens"))).toBe(true); + }); + + it("should load all skills from fixture directory", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: fixturesDir, + source: "test", + }); + + // Should load all skills that have descriptions (even with warnings) + // valid-skill, name-mismatch, invalid-name-chars, long-name, unknown-field, nested/child-skill, consecutive-hyphens + // NOT: missing-description, no-frontmatter (both missing descriptions) + expect(skills.length).toBeGreaterThanOrEqual(6); + }); + + it("should return empty for non-existent directory", () => { + const { skills, warnings } = loadSkillsFromDir({ + dir: "/non/existent/path", + source: "test", + }); + + expect(skills).toHaveLength(0); + expect(warnings).toHaveLength(0); + }); + + it("should use parent directory name when name not in frontmatter", () => { + // The no-frontmatter fixture has no name in frontmatter, so it should use "no-frontmatter" + // But it also has no description, so it won't load + // Let's test with a valid skill that relies on directory name + const { skills } = loadSkillsFromDir({ + dir: join(fixturesDir, "valid-skill"), + source: "test", + }); + + expect(skills).toHaveLength(1); + expect(skills[0].name).toBe("valid-skill"); + }); + }); + + describe("formatSkillsForPrompt", () => { + it("should return empty string for no skills", () => { + const result = formatSkillsForPrompt([]); + expect(result).toBe(""); + }); + + it("should format skills as XML", () => { + const skills: Skill[] = [ + { + name: "test-skill", + description: "A test skill.", + filePath: "/path/to/skill/SKILL.md", + baseDir: "/path/to/skill", + source: "test", + }, + ]; + + const result = formatSkillsForPrompt(skills); + + expect(result).toContain(""); + expect(result).toContain(""); + expect(result).toContain(""); + expect(result).toContain("test-skill"); + expect(result).toContain("A test skill."); + expect(result).toContain("/path/to/skill/SKILL.md"); + }); + + it("should include intro text before XML", () => { + const skills: Skill[] = [ + { + name: "test-skill", + description: "A test skill.", + filePath: "/path/to/skill/SKILL.md", + baseDir: "/path/to/skill", + source: "test", + }, + ]; + + const result = formatSkillsForPrompt(skills); + const xmlStart = result.indexOf(""); + const introText = result.substring(0, xmlStart); + + expect(introText).toContain("The following skills provide specialized instructions"); + expect(introText).toContain("Use the read tool to load a skill's file"); + }); + + it("should escape XML special characters", () => { + const skills: Skill[] = [ + { + name: "test-skill", + description: 'A skill with & "characters".', + filePath: "/path/to/skill/SKILL.md", + baseDir: "/path/to/skill", + source: "test", + }, + ]; + + const result = formatSkillsForPrompt(skills); + + expect(result).toContain("<special>"); + expect(result).toContain("&"); + expect(result).toContain(""characters""); + }); + + it("should format multiple skills", () => { + const skills: Skill[] = [ + { + name: "skill-one", + description: "First skill.", + filePath: "/path/one/SKILL.md", + baseDir: "/path/one", + source: "test", + }, + { + name: "skill-two", + description: "Second skill.", + filePath: "/path/two/SKILL.md", + baseDir: "/path/two", + source: "test", + }, + ]; + + const result = formatSkillsForPrompt(skills); + + expect(result).toContain("skill-one"); + expect(result).toContain("skill-two"); + expect((result.match(//g) || []).length).toBe(2); + }); + }); + + describe("collision handling", () => { + it("should detect name collisions and keep first skill", () => { + // Load from first directory + const first = loadSkillsFromDir({ + dir: join(collisionFixturesDir, "first"), + source: "first", + }); + + const second = loadSkillsFromDir({ + dir: join(collisionFixturesDir, "second"), + source: "second", + }); + + // Simulate the collision behavior from loadSkills() + const skillMap = new Map(); + const collisionWarnings: Array<{ skillPath: string; message: string }> = []; + + for (const skill of first.skills) { + skillMap.set(skill.name, skill); + } + + for (const skill of second.skills) { + const existing = skillMap.get(skill.name); + if (existing) { + collisionWarnings.push({ + skillPath: skill.filePath, + message: `name collision: "${skill.name}" already loaded from ${existing.filePath}`, + }); + } else { + skillMap.set(skill.name, skill); + } + } + + expect(skillMap.size).toBe(1); + expect(skillMap.get("calendar")?.source).toBe("first"); + expect(collisionWarnings).toHaveLength(1); + expect(collisionWarnings[0].message).toContain("name collision"); + }); + }); +}); diff --git a/packages/mom/src/agent.ts b/packages/mom/src/agent.ts index 8c6d4202..50261e10 100644 --- a/packages/mom/src/agent.ts +++ b/packages/mom/src/agent.ts @@ -118,7 +118,7 @@ function loadMomSkills(channelDir: string, workspacePath: string): Skill[] { // Load workspace-level skills (global) const workspaceSkillsDir = join(hostWorkspacePath, "skills"); - for (const skill of loadSkillsFromDir({ dir: workspaceSkillsDir, source: "workspace" })) { + for (const skill of loadSkillsFromDir({ dir: workspaceSkillsDir, source: "workspace" }).skills) { // Translate paths to container paths for system prompt skill.filePath = translatePath(skill.filePath); skill.baseDir = translatePath(skill.baseDir); @@ -127,7 +127,7 @@ function loadMomSkills(channelDir: string, workspacePath: string): Skill[] { // Load channel-specific skills (override workspace skills on collision) const channelSkillsDir = join(channelDir, "skills"); - for (const skill of loadSkillsFromDir({ dir: channelSkillsDir, source: "channel" })) { + for (const skill of loadSkillsFromDir({ dir: channelSkillsDir, source: "channel" }).skills) { skill.filePath = translatePath(skill.filePath); skill.baseDir = translatePath(skill.baseDir); skillMap.set(skill.name, skill);