diff --git a/packages/agent/src/harness/skills.ts b/packages/agent/src/harness/skills.ts index 0c0b54330..44fd828ed 100644 --- a/packages/agent/src/harness/skills.ts +++ b/packages/agent/src/harness/skills.ts @@ -43,8 +43,9 @@ export function formatSkillInvocation(skill: Skill, additionalInstructions?: str /** * Load skills from one or more directories. * - * Traverses directories recursively, loads `SKILL.md` files, loads direct root `.md` files as skills, honors ignore files, - * and returns diagnostics for invalid skill files. Missing input directories are skipped. + * Traverses directories recursively, loads `SKILL.md` files, loads direct root `.md` files with skill + * frontmatter, honors ignore files, and returns diagnostics for invalid declared skill files. Missing input + * directories are skipped. */ export async function loadSkills( env: ExecutionEnv, @@ -246,6 +247,11 @@ async function loadSkillFromFile( parentDirName: string, ): Promise<{ skill: Skill | null; diagnostics: SkillDiagnostic[] }> { const diagnostics: SkillDiagnostic[] = []; + const isDeclaredSkill = + filePath + .replace(/[\\/]+$/, "") + .split(/[\\/]/) + .pop() === "SKILL.md"; const rawContent = await env.readTextFile(filePath); if (!rawContent.ok) { diagnostics.push({ type: "warning", code: "read_failed", message: rawContent.error.message, path: filePath }); @@ -254,12 +260,17 @@ async function loadSkillFromFile( const parsed = parseFrontmatter(rawContent.value); if (!parsed.ok) { - diagnostics.push({ type: "warning", code: "parse_failed", message: parsed.error.message, path: filePath }); + if (isDeclaredSkill) { + diagnostics.push({ type: "warning", code: "parse_failed", message: parsed.error.message, path: filePath }); + } return { skill: null, diagnostics }; } const { frontmatter, body } = parsed.value; const description = typeof frontmatter.description === "string" ? frontmatter.description : undefined; + if (!isDeclaredSkill && (!description || description.trim() === "")) { + return { skill: null, diagnostics }; + } for (const error of validateDescription(description)) { diagnostics.push({ type: "warning", code: "invalid_metadata", message: error, path: filePath }); diff --git a/packages/agent/test/harness/skills.test.ts b/packages/agent/test/harness/skills.test.ts index bd769ea91..88c155948 100644 --- a/packages/agent/test/harness/skills.test.ts +++ b/packages/agent/test/harness/skills.test.ts @@ -113,4 +113,23 @@ Use this skill. expect(skills.map((skill) => skill.name)).toEqual(["skills"]); expect(skills[0]?.content).toBe("Root content"); }); + + it("ignores root markdown docs that do not declare skills", async () => { + const root = createTempDir(); + const env = new NodeExecutionEnv({ cwd: root }); + await env.createDir("skills/nested-skill", { recursive: true }); + await env.writeFile("skills/README.md", "# Shared skills\n\nDocumentation."); + await env.writeFile("skills/AGENTS.md", "# Agent notes\n\nDocumentation."); + await env.writeFile("skills/CLAUDE.md", "---\ndescription: [invalid\n---\n\nDocumentation."); + await env.writeFile("skills/root.md", "---\ndescription: Root skill\n---\nRoot content"); + await env.writeFile( + "skills/nested-skill/SKILL.md", + "---\nname: nested-skill\ndescription: Nested skill\n---\nNested content", + ); + + const { skills, diagnostics } = await loadSkills(env, "skills"); + + expect(diagnostics).toEqual([]); + expect(skills.map((skill) => skill.name).sort()).toEqual(["nested-skill", "skills"]); + }); }); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index d35b19c5a..eb7f6433c 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -36,6 +36,7 @@ ### Fixed +- Fixed root Markdown files such as `README.md` and `AGENTS.md` in skill directories being reported as broken skills unless they declare valid skill frontmatter ([#7805](https://github.com/earendil-works/pi/issues/7805)). - Fixed single-object `edit` tool inputs failing validation by accepting them as one-edit arrays in both coding-agent and harness edit tools ([#7835](https://github.com/earendil-works/pi/issues/7835)). - Fixed managed-tool downloads delaying TUI startup and hiding diagnostics in fullscreen mode by mounting the TUI first and showing download progress and warnings inside it. - Fixed opening a model selector immediately after startup cancelling and restarting the in-progress model catalog refresh. diff --git a/packages/coding-agent/docs/skills.md b/packages/coding-agent/docs/skills.md index d3ffeea89..198a6c17c 100644 --- a/packages/coding-agent/docs/skills.md +++ b/packages/coding-agent/docs/skills.md @@ -34,9 +34,10 @@ Pi loads skills from: - CLI: `--skill ` (repeatable, additive even with `--no-skills`) Discovery rules: -- In `~/.pi/agent/skills/` and `.pi/skills/`, direct root `.md` files are discovered as individual skills +- In `~/.pi/agent/skills/` and `.pi/skills/`, direct root `.md` files are discovered as individual skills when they have valid skill frontmatter with a non-empty `description` - In all skill locations, directories containing `SKILL.md` are discovered recursively - In `~/.agents/skills/` and project `.agents/skills/`, root `.md` files are ignored +- Root Markdown files other than `SKILL.md` that do not look like skills are ignored silently Disable discovery with `--no-skills` (explicit `--skill` paths still load). @@ -183,7 +184,7 @@ Pi validates skills against the Agent Skills standard. Most issues produce warni Unknown frontmatter fields are ignored. -**Exception:** Skills with missing description are not loaded. +Declared skills with missing descriptions are not loaded. Malformed `SKILL.md` files and `SKILL.md` files without a description produce warnings and are not loaded. Other Markdown files without valid skill frontmatter are ignored. Name collisions (same name from different locations) warn and keep the first skill found. diff --git a/packages/coding-agent/src/core/skills.ts b/packages/coding-agent/src/core/skills.ts index c104c8564..464129b64 100644 --- a/packages/coding-agent/src/core/skills.ts +++ b/packages/coding-agent/src/core/skills.ts @@ -114,10 +114,10 @@ function validateName(name: string): string[] { /** * Validate description per Agent Skills spec. */ -function validateDescription(description: string | undefined): string[] { +function validateDescription(description: unknown): string[] { const errors: string[] = []; - if (!description || description.trim() === "") { + if (typeof description !== "string" || description.trim() === "") { errors.push("description is required"); } else if (description.length > MAX_DESCRIPTION_LENGTH) { errors.push(`description exceeds ${MAX_DESCRIPTION_LENGTH} characters (${description.length})`); @@ -279,49 +279,69 @@ function loadSkillFromFile( source: string, ): { skill: Skill | null; diagnostics: ResourceDiagnostic[] } { const diagnostics: ResourceDiagnostic[] = []; + const isDeclaredSkill = basename(filePath) === "SKILL.md"; + let rawContent: string; try { - const rawContent = readFileSync(filePath, "utf-8"); - const { frontmatter } = parseFrontmatter(rawContent); - const skillDir = dirname(filePath); - const parentDirName = basename(skillDir); - - // Validate description - const descErrors = validateDescription(frontmatter.description); - for (const error of descErrors) { - diagnostics.push({ type: "warning", message: error, path: filePath }); - } - - // Use name from frontmatter, or fall back to parent directory name - const name = frontmatter.name || parentDirName; - - // Validate name - const nameErrors = validateName(name); - for (const error of nameErrors) { - diagnostics.push({ type: "warning", message: error, path: filePath }); - } - - // Still load the skill even with warnings (unless description is completely missing) - if (!frontmatter.description || frontmatter.description.trim() === "") { - return { skill: null, diagnostics }; - } - - return { - skill: { - name, - description: frontmatter.description, - filePath, - baseDir: skillDir, - sourceInfo: createSkillSourceInfo(filePath, skillDir, source), - disableModelInvocation: frontmatter["disable-model-invocation"] === true, - }, - diagnostics, - }; + rawContent = readFileSync(filePath, "utf-8"); } catch (error) { - const message = error instanceof Error ? error.message : "failed to parse skill file"; + const message = error instanceof Error ? error.message : "failed to read skill file"; diagnostics.push({ type: "warning", message, path: filePath }); return { skill: null, diagnostics }; } + + let frontmatter: SkillFrontmatter; + try { + ({ frontmatter } = parseFrontmatter(rawContent)); + } catch (error) { + if (isDeclaredSkill) { + const message = error instanceof Error ? error.message : "failed to parse skill file"; + diagnostics.push({ type: "warning", message, path: filePath }); + } + return { skill: null, diagnostics }; + } + + const description = frontmatter.description; + const hasDescription = typeof description === "string" && description.trim() !== ""; + if (!isDeclaredSkill && !hasDescription) { + return { skill: null, diagnostics }; + } + + const skillDir = dirname(filePath); + const parentDirName = basename(skillDir); + + // Validate description + const descErrors = validateDescription(description); + for (const error of descErrors) { + diagnostics.push({ type: "warning", message: error, path: filePath }); + } + + // Use name from frontmatter, or fall back to parent directory name + const frontmatterName = typeof frontmatter.name === "string" ? frontmatter.name : undefined; + const name = frontmatterName || parentDirName; + + // Validate name + const nameErrors = validateName(name); + for (const error of nameErrors) { + diagnostics.push({ type: "warning", message: error, path: filePath }); + } + + // Still load the skill even with warnings, unless description is missing or empty. + if (!hasDescription) { + return { skill: null, diagnostics }; + } + + return { + skill: { + name, + description, + filePath, + baseDir: skillDir, + sourceInfo: createSkillSourceInfo(filePath, skillDir, source), + disableModelInvocation: frontmatter["disable-model-invocation"] === true, + }, + diagnostics, + }; } /**