From b9fd912ecf07758939da46bf8dc6be9897027fda Mon Sep 17 00:00:00 2001 From: Aliou Diallo Date: Thu, 18 Dec 2025 23:57:54 +0100 Subject: [PATCH 1/5] fix(coding-agent): support symlinked slash commands in discovery --- packages/coding-agent/src/core/slash-commands.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/coding-agent/src/core/slash-commands.ts b/packages/coding-agent/src/core/slash-commands.ts index 57f37f2d..5450d622 100644 --- a/packages/coding-agent/src/core/slash-commands.ts +++ b/packages/coding-agent/src/core/slash-commands.ts @@ -99,7 +99,7 @@ export function substituteArgs(content: string, args: string[]): string { } /** - * Recursively scan a directory for .md files and load them as slash commands + * Recursively scan a directory for .md files (and symlinks to .md files) and load them as slash commands */ function loadCommandsFromDir(dir: string, source: "user" | "project", subdir: string = ""): FileSlashCommand[] { const commands: FileSlashCommand[] = []; @@ -118,7 +118,7 @@ function loadCommandsFromDir(dir: string, source: "user" | "project", subdir: st // Recurse into subdirectory const newSubdir = subdir ? `${subdir}:${entry.name}` : entry.name; commands.push(...loadCommandsFromDir(fullPath, source, newSubdir)); - } else if (entry.isFile() && entry.name.endsWith(".md")) { + } else if ((entry.isFile() || entry.isSymbolicLink()) && entry.name.endsWith(".md")) { try { const rawContent = readFileSync(fullPath, "utf-8"); const { frontmatter, content } = parseFrontmatter(rawContent); From 05b7b813385daf17e3a2e270b3db6a13da298ad2 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 19 Dec 2025 00:11:39 +0100 Subject: [PATCH 2/5] 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); From 3d9bad8fb67f1435a707ac42ba73d8e804499304 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 19 Dec 2025 00:42:08 +0100 Subject: [PATCH 3/5] Expose full tool result content and details in hook tool_result event Breaking change: ToolResultEvent now exposes content and typed details instead of just a result string. Hook handlers returning { result: ... } must change to { content: [...] }. - ToolResultEvent is now a discriminated union based on toolName - Each built-in tool has typed details (BashToolDetails, etc.) - Export tool details types and TruncationResult - Update hooks.md documentation Closes #233 --- packages/coding-agent/CHANGELOG.md | 10 +++ packages/coding-agent/docs/hooks.md | 53 ++++++++++- packages/coding-agent/src/core/hooks/index.ts | 8 ++ .../src/core/hooks/tool-wrapper.ts | 15 ++-- packages/coding-agent/src/core/hooks/types.ts | 89 ++++++++++++++++--- packages/coding-agent/src/core/tools/bash.ts | 2 +- packages/coding-agent/src/core/tools/find.ts | 2 +- packages/coding-agent/src/core/tools/grep.ts | 2 +- packages/coding-agent/src/core/tools/index.ts | 11 +-- packages/coding-agent/src/core/tools/ls.ts | 2 +- packages/coding-agent/src/core/tools/read.ts | 2 +- packages/coding-agent/src/index.ts | 25 +++++- 12 files changed, 187 insertions(+), 34 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d92471b6..104e8da5 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,16 @@ ## [Unreleased] +### Breaking Changes + +- **Hook `tool_result` event restructured**: The `ToolResultEvent` now exposes full tool result data instead of just text. ([#233](https://github.com/badlogic/pi-mono/pull/233)) + - Removed: `result: string` field + - Added: `content: (TextContent | ImageContent)[]` - full content array + - Added: `details: unknown` - tool-specific details (typed per tool via discriminated union on `toolName`) + - `ToolResultEventResult.result` renamed to `ToolResultEventResult.text` (removed), use `content` instead + - Hook handlers returning `{ result: "..." }` must change to `{ content: [{ type: "text", text: "..." }] }` + - Built-in tool details types exported: `BashToolDetails`, `ReadToolDetails`, `GrepToolDetails`, `FindToolDetails`, `LsToolDetails`, `TruncationResult` + ### 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)) diff --git a/packages/coding-agent/docs/hooks.md b/packages/coding-agent/docs/hooks.md index 6fd76869..6d73ded1 100644 --- a/packages/coding-agent/docs/hooks.md +++ b/packages/coding-agent/docs/hooks.md @@ -222,13 +222,60 @@ Fired after tool executes. **Can modify result.** ```typescript pi.on("tool_result", async (event, ctx) => { - // event.toolName, event.toolCallId, event.input - // event.result: string + // event.toolName: string + // event.toolCallId: string + // event.input: Record + // event.content: (TextContent | ImageContent)[] + // event.details: tool-specific (see below) // event.isError: boolean - return { result: "modified" }; // or undefined to keep original + + // Return modified content/details, or undefined to keep original + return { content: [...], details: {...} }; }); ``` +The event type is a discriminated union based on `toolName`. TypeScript will narrow `details` to the correct type: + +```typescript +pi.on("tool_result", async (event, ctx) => { + if (event.toolName === "bash") { + // event.details is BashToolDetails | undefined + if (event.details?.truncation?.truncated) { + // Access full output from temp file + const fullPath = event.details.fullOutputPath; + } + } +}); +``` + +#### Tool Details Types + +Each built-in tool has a typed `details` field. Types are exported from `@mariozechner/pi-coding-agent`: + +| Tool | Details Type | Source | +|------|-------------|--------| +| `bash` | `BashToolDetails` | `src/core/tools/bash.ts` | +| `read` | `ReadToolDetails` | `src/core/tools/read.ts` | +| `edit` | `undefined` | - | +| `write` | `undefined` | - | +| `grep` | `GrepToolDetails` | `src/core/tools/grep.ts` | +| `find` | `FindToolDetails` | `src/core/tools/find.ts` | +| `ls` | `LsToolDetails` | `src/core/tools/ls.ts` | + +Common fields in details: +- `truncation?: TruncationResult` - present when output was truncated +- `fullOutputPath?: string` - path to temp file with full output (bash only) + +`TruncationResult` contains: +- `truncated: boolean` - whether truncation occurred +- `truncatedBy: "lines" | "bytes" | null` - which limit was hit +- `totalLines`, `totalBytes` - original size +- `outputLines`, `outputBytes` - truncated size + +Custom tools use `CustomToolResultEvent` with `details: unknown`. + +**Note:** If you modify `content`, you should also update `details` accordingly. The TUI uses `details` (e.g., truncation info) for rendering, so inconsistent values will cause display issues. + ## Context API Every event handler receives a context object with these methods: diff --git a/packages/coding-agent/src/core/hooks/index.ts b/packages/coding-agent/src/core/hooks/index.ts index 739603e1..b8335913 100644 --- a/packages/coding-agent/src/core/hooks/index.ts +++ b/packages/coding-agent/src/core/hooks/index.ts @@ -4,15 +4,22 @@ export { wrapToolsWithHooks, wrapToolWithHooks } from "./tool-wrapper.js"; export type { AgentEndEvent, AgentStartEvent, + BashToolResultEvent, BranchEvent, BranchEventResult, + CustomToolResultEvent, + EditToolResultEvent, ExecResult, + FindToolResultEvent, + GrepToolResultEvent, HookAPI, HookError, HookEvent, HookEventContext, HookFactory, HookUIContext, + LsToolResultEvent, + ReadToolResultEvent, SessionEvent, ToolCallEvent, ToolCallEventResult, @@ -20,4 +27,5 @@ export type { ToolResultEventResult, TurnEndEvent, TurnStartEvent, + WriteToolResultEvent, } from "./types.js"; diff --git a/packages/coding-agent/src/core/hooks/tool-wrapper.ts b/packages/coding-agent/src/core/hooks/tool-wrapper.ts index f4eb6b07..a36fe8d6 100644 --- a/packages/coding-agent/src/core/hooks/tool-wrapper.ts +++ b/packages/coding-agent/src/core/hooks/tool-wrapper.ts @@ -44,26 +44,21 @@ export function wrapToolWithHooks(tool: AgentTool, hookRunner: HookRu // Emit tool_result event - hooks can modify the result if (hookRunner.hasHandlers("tool_result")) { - // Extract text from result for hooks - const resultText = result.content - .filter((c): c is { type: "text"; text: string } => c.type === "text") - .map((c) => c.text) - .join("\n"); - const resultResult = (await hookRunner.emit({ type: "tool_result", toolName: tool.name, toolCallId, input: params, - result: resultText, + content: result.content, + details: result.details, isError: false, })) as ToolResultEventResult | undefined; // Apply modifications if any - if (resultResult?.result !== undefined) { + if (resultResult) { return { - ...result, - content: [{ type: "text", text: resultResult.result }], + content: resultResult.content ?? result.content, + details: (resultResult.details ?? result.details) as T, }; } } diff --git a/packages/coding-agent/src/core/hooks/types.ts b/packages/coding-agent/src/core/hooks/types.ts index b47e1857..e1677fbc 100644 --- a/packages/coding-agent/src/core/hooks/types.ts +++ b/packages/coding-agent/src/core/hooks/types.ts @@ -6,8 +6,15 @@ */ import type { AppMessage, Attachment } from "@mariozechner/pi-agent-core"; -import type { ToolResultMessage } from "@mariozechner/pi-ai"; +import type { ImageContent, TextContent, ToolResultMessage } from "@mariozechner/pi-ai"; import type { SessionEntry } from "../session-manager.js"; +import type { + BashToolDetails, + FindToolDetails, + GrepToolDetails, + LsToolDetails, + ReadToolDetails, +} from "../tools/index.js"; // ============================================================================ // Execution Context @@ -140,23 +147,83 @@ export interface ToolCallEvent { } /** - * Event data for tool_result event. - * Fired after a tool is executed. Hooks can modify the result. + * Base interface for tool_result events. */ -export interface ToolResultEvent { +interface ToolResultEventBase { type: "tool_result"; - /** Tool name (e.g., "bash", "edit", "write") */ - toolName: string; /** Tool call ID */ toolCallId: string; /** Tool input parameters */ input: Record; - /** Tool result content (text) */ - result: string; + /** Full content array (text and images) */ + content: (TextContent | ImageContent)[]; /** Whether the tool execution was an error */ isError: boolean; } +/** Tool result event for bash tool */ +export interface BashToolResultEvent extends ToolResultEventBase { + toolName: "bash"; + details: BashToolDetails | undefined; +} + +/** Tool result event for read tool */ +export interface ReadToolResultEvent extends ToolResultEventBase { + toolName: "read"; + details: ReadToolDetails | undefined; +} + +/** Tool result event for edit tool */ +export interface EditToolResultEvent extends ToolResultEventBase { + toolName: "edit"; + details: undefined; +} + +/** Tool result event for write tool */ +export interface WriteToolResultEvent extends ToolResultEventBase { + toolName: "write"; + details: undefined; +} + +/** Tool result event for grep tool */ +export interface GrepToolResultEvent extends ToolResultEventBase { + toolName: "grep"; + details: GrepToolDetails | undefined; +} + +/** Tool result event for find tool */ +export interface FindToolResultEvent extends ToolResultEventBase { + toolName: "find"; + details: FindToolDetails | undefined; +} + +/** Tool result event for ls tool */ +export interface LsToolResultEvent extends ToolResultEventBase { + toolName: "ls"; + details: LsToolDetails | undefined; +} + +/** Tool result event for custom/unknown tools */ +export interface CustomToolResultEvent extends ToolResultEventBase { + toolName: string; + details: unknown; +} + +/** + * Event data for tool_result event. + * Fired after a tool is executed. Hooks can modify the result. + * Use toolName to discriminate and get typed details. + */ +export type ToolResultEvent = + | BashToolResultEvent + | ReadToolResultEvent + | EditToolResultEvent + | WriteToolResultEvent + | GrepToolResultEvent + | FindToolResultEvent + | LsToolResultEvent + | CustomToolResultEvent; + /** * Event data for branch event. */ @@ -201,8 +268,10 @@ export interface ToolCallEventResult { * Allows hooks to modify tool results. */ export interface ToolResultEventResult { - /** Modified result text (if not set, original result is used) */ - result?: string; + /** Replacement content array (text and images) */ + content?: (TextContent | ImageContent)[]; + /** Replacement details */ + details?: unknown; /** Override isError flag */ isError?: boolean; } diff --git a/packages/coding-agent/src/core/tools/bash.ts b/packages/coding-agent/src/core/tools/bash.ts index f109d2ec..df69ae53 100644 --- a/packages/coding-agent/src/core/tools/bash.ts +++ b/packages/coding-agent/src/core/tools/bash.ts @@ -21,7 +21,7 @@ const bashSchema = Type.Object({ timeout: Type.Optional(Type.Number({ description: "Timeout in seconds (optional, no default timeout)" })), }); -interface BashToolDetails { +export interface BashToolDetails { truncation?: TruncationResult; fullOutputPath?: string; } diff --git a/packages/coding-agent/src/core/tools/find.ts b/packages/coding-agent/src/core/tools/find.ts index de15513a..c241e3b4 100644 --- a/packages/coding-agent/src/core/tools/find.ts +++ b/packages/coding-agent/src/core/tools/find.ts @@ -18,7 +18,7 @@ const findSchema = Type.Object({ const DEFAULT_LIMIT = 1000; -interface FindToolDetails { +export interface FindToolDetails { truncation?: TruncationResult; resultLimitReached?: number; } diff --git a/packages/coding-agent/src/core/tools/grep.ts b/packages/coding-agent/src/core/tools/grep.ts index 73be92ff..7e75178e 100644 --- a/packages/coding-agent/src/core/tools/grep.ts +++ b/packages/coding-agent/src/core/tools/grep.ts @@ -31,7 +31,7 @@ const grepSchema = Type.Object({ const DEFAULT_LIMIT = 100; -interface GrepToolDetails { +export interface GrepToolDetails { truncation?: TruncationResult; matchLimitReached?: number; linesTruncated?: boolean; diff --git a/packages/coding-agent/src/core/tools/index.ts b/packages/coding-agent/src/core/tools/index.ts index 94af0286..034f554a 100644 --- a/packages/coding-agent/src/core/tools/index.ts +++ b/packages/coding-agent/src/core/tools/index.ts @@ -1,9 +1,10 @@ -export { bashTool } from "./bash.js"; +export { type BashToolDetails, bashTool } from "./bash.js"; export { editTool } from "./edit.js"; -export { findTool } from "./find.js"; -export { grepTool } from "./grep.js"; -export { lsTool } from "./ls.js"; -export { readTool } from "./read.js"; +export { type FindToolDetails, findTool } from "./find.js"; +export { type GrepToolDetails, grepTool } from "./grep.js"; +export { type LsToolDetails, lsTool } from "./ls.js"; +export { type ReadToolDetails, readTool } from "./read.js"; +export type { TruncationResult } from "./truncate.js"; export { writeTool } from "./write.js"; import { bashTool } from "./bash.js"; diff --git a/packages/coding-agent/src/core/tools/ls.ts b/packages/coding-agent/src/core/tools/ls.ts index 2be75755..3edfcffe 100644 --- a/packages/coding-agent/src/core/tools/ls.ts +++ b/packages/coding-agent/src/core/tools/ls.ts @@ -12,7 +12,7 @@ const lsSchema = Type.Object({ const DEFAULT_LIMIT = 500; -interface LsToolDetails { +export interface LsToolDetails { truncation?: TruncationResult; entryLimitReached?: number; } diff --git a/packages/coding-agent/src/core/tools/read.ts b/packages/coding-agent/src/core/tools/read.ts index 678b4535..4b83534b 100644 --- a/packages/coding-agent/src/core/tools/read.ts +++ b/packages/coding-agent/src/core/tools/read.ts @@ -13,7 +13,7 @@ const readSchema = Type.Object({ limit: Type.Optional(Type.Number({ description: "Maximum number of lines to read" })), }); -interface ReadToolDetails { +export interface ReadToolDetails { truncation?: TruncationResult; } diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index b4c313b0..7184d4cd 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -39,13 +39,20 @@ export { discoverAndLoadCustomTools, loadCustomTools } from "./core/custom-tools export type { AgentEndEvent, AgentStartEvent, + BashToolResultEvent, BranchEvent, BranchEventResult, + CustomToolResultEvent, + EditToolResultEvent, + FindToolResultEvent, + GrepToolResultEvent, HookAPI, HookEvent, HookEventContext, HookFactory, HookUIContext, + LsToolResultEvent, + ReadToolResultEvent, SessionEvent, ToolCallEvent, ToolCallEventResult, @@ -53,6 +60,7 @@ export type { ToolResultEventResult, TurnEndEvent, TurnStartEvent, + WriteToolResultEvent, } from "./core/hooks/index.js"; export { messageTransformer } from "./core/messages.js"; export { @@ -89,7 +97,22 @@ export { type SkillWarning, } from "./core/skills.js"; // Tools -export { bashTool, codingTools, editTool, readTool, writeTool } from "./core/tools/index.js"; +export { + type BashToolDetails, + bashTool, + codingTools, + editTool, + type FindToolDetails, + findTool, + type GrepToolDetails, + grepTool, + type LsToolDetails, + lsTool, + type ReadToolDetails, + readTool, + type TruncationResult, + writeTool, +} from "./core/tools/index.js"; // Main entry point export { main } from "./main.js"; From d353e5e2198d2f1b129b1a9d17af6037f90b71a5 Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 19 Dec 2025 00:48:03 +0100 Subject: [PATCH 4/5] Add type guards for tool_result event narrowing - Export isBashToolResult, isReadToolResult, etc. type guards - Update hooks.md with type guard usage examples - Document custom tool handling in hooks.md --- packages/coding-agent/CHANGELOG.md | 1 + packages/coding-agent/docs/hooks.md | 37 ++++++++++++++----- packages/coding-agent/src/core/hooks/index.ts | 9 +++++ packages/coding-agent/src/core/hooks/types.ts | 23 ++++++++++++ packages/coding-agent/src/index.ts | 11 +++++- 5 files changed, 70 insertions(+), 11 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 104e8da5..5962c092 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,6 +11,7 @@ - `ToolResultEventResult.result` renamed to `ToolResultEventResult.text` (removed), use `content` instead - Hook handlers returning `{ result: "..." }` must change to `{ content: [{ type: "text", text: "..." }] }` - Built-in tool details types exported: `BashToolDetails`, `ReadToolDetails`, `GrepToolDetails`, `FindToolDetails`, `LsToolDetails`, `TruncationResult` + - Type guards exported for narrowing: `isBashToolResult`, `isReadToolResult`, `isEditToolResult`, `isWriteToolResult`, `isGrepToolResult`, `isFindToolResult`, `isLsToolResult` ### Changed diff --git a/packages/coding-agent/docs/hooks.md b/packages/coding-agent/docs/hooks.md index 6d73ded1..43899cde 100644 --- a/packages/coding-agent/docs/hooks.md +++ b/packages/coding-agent/docs/hooks.md @@ -234,20 +234,26 @@ pi.on("tool_result", async (event, ctx) => { }); ``` -The event type is a discriminated union based on `toolName`. TypeScript will narrow `details` to the correct type: +The event type is a discriminated union based on `toolName`. Use the provided type guards to narrow `details` to the correct type: ```typescript -pi.on("tool_result", async (event, ctx) => { - if (event.toolName === "bash") { - // event.details is BashToolDetails | undefined - if (event.details?.truncation?.truncated) { - // Access full output from temp file - const fullPath = event.details.fullOutputPath; +import { isBashToolResult, type HookAPI } from "@mariozechner/pi-coding-agent/hooks"; + +export default function (pi: HookAPI) { + pi.on("tool_result", async (event, ctx) => { + if (isBashToolResult(event)) { + // event.details is BashToolDetails | undefined + if (event.details?.truncation?.truncated) { + // Access full output from temp file + const fullPath = event.details.fullOutputPath; + } } - } -}); + }); +} ``` +Available type guards: `isBashToolResult`, `isReadToolResult`, `isEditToolResult`, `isWriteToolResult`, `isGrepToolResult`, `isFindToolResult`, `isLsToolResult`. + #### Tool Details Types Each built-in tool has a typed `details` field. Types are exported from `@mariozechner/pi-coding-agent`: @@ -272,7 +278,18 @@ Common fields in details: - `totalLines`, `totalBytes` - original size - `outputLines`, `outputBytes` - truncated size -Custom tools use `CustomToolResultEvent` with `details: unknown`. +Custom tools use `CustomToolResultEvent` with `details: unknown`. You'll need to cast or validate: + +```typescript +pi.on("tool_result", async (event, ctx) => { + if (event.toolName === "my-custom-tool") { + // Cast to your tool's details type + const details = event.details as MyCustomToolDetails; + } +}); +``` + +Custom tools define their own details type in their `execute` function return value. The hook receives whatever the tool returned, but since the hook system doesn't know about custom tool types at compile time, it's typed as `unknown`. **Note:** If you modify `content`, you should also update `details` accordingly. The TUI uses `details` (e.g., truncation info) for rendering, so inconsistent values will cause display issues. diff --git a/packages/coding-agent/src/core/hooks/index.ts b/packages/coding-agent/src/core/hooks/index.ts index b8335913..a2efab7a 100644 --- a/packages/coding-agent/src/core/hooks/index.ts +++ b/packages/coding-agent/src/core/hooks/index.ts @@ -29,3 +29,12 @@ export type { TurnStartEvent, WriteToolResultEvent, } from "./types.js"; +export { + isBashToolResult, + isEditToolResult, + isFindToolResult, + isGrepToolResult, + isLsToolResult, + isReadToolResult, + isWriteToolResult, +} from "./types.js"; diff --git a/packages/coding-agent/src/core/hooks/types.ts b/packages/coding-agent/src/core/hooks/types.ts index e1677fbc..7d6eb570 100644 --- a/packages/coding-agent/src/core/hooks/types.ts +++ b/packages/coding-agent/src/core/hooks/types.ts @@ -224,6 +224,29 @@ export type ToolResultEvent = | LsToolResultEvent | CustomToolResultEvent; +// Type guards for narrowing ToolResultEvent to specific tool types +export function isBashToolResult(e: ToolResultEvent): e is BashToolResultEvent { + return e.toolName === "bash"; +} +export function isReadToolResult(e: ToolResultEvent): e is ReadToolResultEvent { + return e.toolName === "read"; +} +export function isEditToolResult(e: ToolResultEvent): e is EditToolResultEvent { + return e.toolName === "edit"; +} +export function isWriteToolResult(e: ToolResultEvent): e is WriteToolResultEvent { + return e.toolName === "write"; +} +export function isGrepToolResult(e: ToolResultEvent): e is GrepToolResultEvent { + return e.toolName === "grep"; +} +export function isFindToolResult(e: ToolResultEvent): e is FindToolResultEvent { + return e.toolName === "find"; +} +export function isLsToolResult(e: ToolResultEvent): e is LsToolResultEvent { + return e.toolName === "ls"; +} + /** * Event data for branch event. */ diff --git a/packages/coding-agent/src/index.ts b/packages/coding-agent/src/index.ts index 7184d4cd..2b0e6827 100644 --- a/packages/coding-agent/src/index.ts +++ b/packages/coding-agent/src/index.ts @@ -35,7 +35,6 @@ export type { ToolUIContext, } from "./core/custom-tools/index.js"; export { discoverAndLoadCustomTools, loadCustomTools } from "./core/custom-tools/index.js"; -// Hook system types export type { AgentEndEvent, AgentStartEvent, @@ -62,6 +61,16 @@ export type { TurnStartEvent, WriteToolResultEvent, } from "./core/hooks/index.js"; +// Hook system types and type guards +export { + isBashToolResult, + isEditToolResult, + isFindToolResult, + isGrepToolResult, + isLsToolResult, + isReadToolResult, + isWriteToolResult, +} from "./core/hooks/index.js"; export { messageTransformer } from "./core/messages.js"; export { type CompactionEntry, From e5e7b2a6a09d9693ea2655950ae7ba56dbcb462b Mon Sep 17 00:00:00 2001 From: Mario Zechner Date: Fri, 19 Dec 2025 00:51:21 +0100 Subject: [PATCH 5/5] Improve hooks.md custom tool example with full type guard pattern --- packages/coding-agent/docs/hooks.md | 45 +++++++++++++++++++++++------ 1 file changed, 36 insertions(+), 9 deletions(-) diff --git a/packages/coding-agent/docs/hooks.md b/packages/coding-agent/docs/hooks.md index 43899cde..cbdf07db 100644 --- a/packages/coding-agent/docs/hooks.md +++ b/packages/coding-agent/docs/hooks.md @@ -278,18 +278,45 @@ Common fields in details: - `totalLines`, `totalBytes` - original size - `outputLines`, `outputBytes` - truncated size -Custom tools use `CustomToolResultEvent` with `details: unknown`. You'll need to cast or validate: +Custom tools use `CustomToolResultEvent` with `details: unknown`. Create your own type guard to get full type safety: ```typescript -pi.on("tool_result", async (event, ctx) => { - if (event.toolName === "my-custom-tool") { - // Cast to your tool's details type - const details = event.details as MyCustomToolDetails; - } -}); -``` +import { + isBashToolResult, + type CustomToolResultEvent, + type HookAPI, + type ToolResultEvent, +} from "@mariozechner/pi-coding-agent/hooks"; -Custom tools define their own details type in their `execute` function return value. The hook receives whatever the tool returned, but since the hook system doesn't know about custom tool types at compile time, it's typed as `unknown`. +interface MyCustomToolDetails { + someField: string; +} + +// Type guard that narrows both toolName and details +function isMyCustomToolResult(e: ToolResultEvent): e is CustomToolResultEvent & { + toolName: "my-custom-tool"; + details: MyCustomToolDetails; +} { + return e.toolName === "my-custom-tool"; +} + +export default function (pi: HookAPI) { + pi.on("tool_result", async (event, ctx) => { + // Built-in tool: use provided type guard + if (isBashToolResult(event)) { + if (event.details?.fullOutputPath) { + console.log(`Full output at: ${event.details.fullOutputPath}`); + } + } + + // Custom tool: use your own type guard + if (isMyCustomToolResult(event)) { + // event.details is now MyCustomToolDetails + console.log(event.details.someField); + } + }); +} +``` **Note:** If you modify `content`, you should also update `details` accordingly. The TUI uses `details` (e.g., truncation info) for rendering, so inconsistent values will cause display issues.