From c824cdc53cb25547aa31dd7d9dfcc4d2acee2b6a Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Tue, 30 Dec 2025 13:31:10 -0700 Subject: [PATCH 1/4] feat(skills): align with Agent Skills spec --- .../prompts/sections/__tests__/skills.spec.ts | 32 +++++ src/core/prompts/sections/skills.ts | 78 ++++++------ src/services/skills/SkillsManager.ts | 33 ++++- .../skills/__tests__/SkillsManager.spec.ts | 115 ++++++++++++++++++ 4 files changed, 214 insertions(+), 44 deletions(-) create mode 100644 src/core/prompts/sections/__tests__/skills.spec.ts diff --git a/src/core/prompts/sections/__tests__/skills.spec.ts b/src/core/prompts/sections/__tests__/skills.spec.ts new file mode 100644 index 00000000000..707d151252f --- /dev/null +++ b/src/core/prompts/sections/__tests__/skills.spec.ts @@ -0,0 +1,32 @@ +import { getSkillsSection } from "../skills" + +describe("getSkillsSection", () => { + it("should emit XML with name, description, and location", async () => { + const mockSkillsManager = { + getSkillsForMode: vi.fn().mockReturnValue([ + { + name: "pdf-processing", + description: "Extracts text & tables from PDFs", + path: "/abs/path/pdf-processing/SKILL.md", + source: "global" as const, + }, + ]), + } + + const result = await getSkillsSection(mockSkillsManager, "code") + + expect(result).toContain("") + expect(result).toContain("") + expect(result).toContain("") + expect(result).toContain("pdf-processing") + // Ensure XML escaping for '&' + expect(result).toContain("Extracts text & tables from PDFs") + // For filesystem-based agents, location should be the absolute path to SKILL.md + expect(result).toContain("/abs/path/pdf-processing/SKILL.md") + }) + + it("should return empty string when skillsManager or currentMode is missing", async () => { + await expect(getSkillsSection(undefined, "code")).resolves.toBe("") + await expect(getSkillsSection({ getSkillsForMode: vi.fn() }, undefined)).resolves.toBe("") + }) +}) diff --git a/src/core/prompts/sections/skills.ts b/src/core/prompts/sections/skills.ts index 999d4f135f6..ff3255bab2b 100644 --- a/src/core/prompts/sections/skills.ts +++ b/src/core/prompts/sections/skills.ts @@ -1,16 +1,14 @@ import { SkillsManager, SkillMetadata } from "../../../services/skills/SkillsManager" -/** - * Get a display-friendly relative path for a skill. - * Converts absolute paths to relative paths to avoid leaking sensitive filesystem info. - * - * @param skill - The skill metadata - * @returns A relative path like ".roo/skills/name/SKILL.md" or "~/.roo/skills/name/SKILL.md" - */ -function getDisplayPath(skill: SkillMetadata): string { - const basePath = skill.source === "project" ? ".roo" : "~/.roo" - const skillsDir = skill.mode ? `skills-${skill.mode}` : "skills" - return `${basePath}/${skillsDir}/${skill.name}/SKILL.md` +type SkillsManagerLike = Pick + +function escapeXml(value: string): string { + return value + .replace(/&/g, "&") + .replace(//g, ">") + .replace(/\"/g, """) + .replace(/'/g, "'") } /** @@ -22,7 +20,7 @@ function getDisplayPath(skill: SkillMetadata): string { * @param currentMode - The current mode slug (e.g., 'code', 'architect') */ export async function getSkillsSection( - skillsManager: SkillsManager | undefined, + skillsManager: SkillsManagerLike | undefined, currentMode: string | undefined, ): Promise { if (!skillsManager || !currentMode) return "" @@ -31,41 +29,35 @@ export async function getSkillsSection( const skills = skillsManager.getSkillsForMode(currentMode) if (skills.length === 0) return "" - // Separate generic and mode-specific skills for display - const genericSkills = skills.filter((s) => !s.mode) - const modeSpecificSkills = skills.filter((s) => s.mode === currentMode) - - let skillsList = "" - - if (modeSpecificSkills.length > 0) { - skillsList += modeSpecificSkills - .map( - (skill) => - ` * "${skill.name}" skill (${currentMode} mode) - ${skill.description} [${getDisplayPath(skill)}]`, - ) - .join("\n") - } - - if (genericSkills.length > 0) { - if (skillsList) skillsList += "\n" - skillsList += genericSkills - .map((skill) => ` * "${skill.name}" skill - ${skill.description} [${getDisplayPath(skill)}]`) - .join("\n") - } + const skillsXml = skills + .map((skill) => { + const name = escapeXml(skill.name) + const description = escapeXml(skill.description) + // Per the Agent Skills integration guidance for filesystem-based agents, + // location should be an absolute path to the SKILL.md file. + const location = escapeXml(skill.path) + return ` \n ${name}\n ${description}\n ${location}\n ` + }) + .join("\n") return `==== AVAILABLE SKILLS -Skills are pre-packaged instructions for specific tasks. When a user request matches a skill description, read the full SKILL.md file to get detailed instructions. - -- These are the currently available skills for "${currentMode}" mode: -${skillsList} - -To use a skill: -1. Identify which skill matches the user's request based on the description -2. Use read_file to load the full SKILL.md file from the path shown in brackets -3. Follow the instructions in the skill file -4. Access any bundled files (scripts, references, assets) as needed + +${skillsXml} + + +How to use skills: +- This list is already filtered for the current mode ("${currentMode}") and includes any mode-specific skills from skills-${currentMode}/ (with project overriding global). +- Select a skill ONLY when the user's request clearly matches the skill's . +- If multiple skills match, prefer the most specific one for the current task. +- Do NOT load every SKILL.md up front. Load the full SKILL.md only after you've decided to use that skill. + +Activate a skill: +1. Load the full SKILL.md content into context. + - Use execute_command to read it (e.g., cat ""). +2. Follow the skill instructions precisely. +3. Only load additional bundled files (scripts/, references/, assets/) if the SKILL.md instructions require them. ` } diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts index 267d0c21832..59b50cf1713 100644 --- a/src/services/skills/SkillsManager.ts +++ b/src/services/skills/SkillsManager.ts @@ -116,12 +116,43 @@ export class SkillsManager { return } + // Strict spec validation (https://agentskills.io/specification) + // Name constraints: + // - 1-64 chars + // - lowercase letters/numbers/hyphens only + // - must not start/end with hyphen + // - must not contain consecutive hyphens + if (effectiveSkillName.length < 1 || effectiveSkillName.length > 64) { + console.error( + `Skill name "${effectiveSkillName}" is invalid: name must be 1-64 characters (got ${effectiveSkillName.length})`, + ) + return + } + const nameFormat = /^[a-z0-9]+(?:-[a-z0-9]+)*$/ + if (!nameFormat.test(effectiveSkillName)) { + console.error( + `Skill name "${effectiveSkillName}" is invalid: must be lowercase letters/numbers/hyphens only (no leading/trailing hyphen, no consecutive hyphens)`, + ) + return + } + + // Description constraints: + // - 1-1024 chars + // - non-empty (after trimming) + const description = frontmatter.description.trim() + if (description.length < 1 || description.length > 1024) { + console.error( + `Skill "${effectiveSkillName}" has an invalid description length: must be 1-1024 characters (got ${description.length})`, + ) + return + } + // Create unique key combining name, source, and mode for override resolution const skillKey = this.getSkillKey(effectiveSkillName, source, mode) this.skills.set(skillKey, { name: effectiveSkillName, - description: frontmatter.description, + description, path: skillMdPath, source, mode, // undefined for generic skills, string for mode-specific diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts index e6f00e5aa48..4b6549108bb 100644 --- a/src/services/skills/__tests__/SkillsManager.spec.ts +++ b/src/services/skills/__tests__/SkillsManager.spec.ts @@ -340,6 +340,121 @@ description: Name doesn't match directory expect(skills).toHaveLength(0) }) + it("should skip skills with invalid name formats (spec compliance)", async () => { + const invalidNames = [ + "PDF-processing", // uppercase + "-pdf", // leading hyphen + "pdf-", // trailing hyphen + "pdf--processing", // consecutive hyphens + ] + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? invalidNames : [])) + + mockStat.mockImplementation(async (pathArg: string) => { + if (invalidNames.some((name) => pathArg === p(globalSkillsDir, name))) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => { + return invalidNames.some((name) => file === p(globalSkillsDir, name, "SKILL.md")) + }) + + mockReadFile.mockImplementation(async (file: string) => { + const match = invalidNames.find((name) => file === p(globalSkillsDir, name, "SKILL.md")) + if (!match) throw new Error("File not found") + return `--- +name: ${match} +description: Invalid name format +--- + +# Invalid Skill` + }) + + await skillsManager.discoverSkills() + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(0) + }) + + it("should skip skills with name longer than 64 characters (spec compliance)", async () => { + const longName = "a".repeat(65) + const longDir = p(globalSkillsDir, longName) + const longMd = p(longDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? [longName] : [])) + + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === longDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + + mockFileExists.mockImplementation(async (file: string) => file === longMd) + mockReadFile.mockResolvedValue(`--- +name: ${longName} +description: Too long name +--- + +# Long Name Skill`) + + await skillsManager.discoverSkills() + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(0) + }) + + it("should skip skills with empty/whitespace-only description (spec compliance)", async () => { + const skillDir = p(globalSkillsDir, "valid-name") + const skillMd = p(skillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["valid-name"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue(`--- +name: valid-name +description: " " +--- + +# Empty Description`) + + await skillsManager.discoverSkills() + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(0) + }) + + it("should skip skills with too-long descriptions (spec compliance)", async () => { + const skillDir = p(globalSkillsDir, "valid-name") + const skillMd = p(skillDir, "SKILL.md") + const longDescription = "d".repeat(1025) + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["valid-name"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) return { isDirectory: () => true } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue(`--- +name: valid-name +description: ${longDescription} +--- + +# Too Long Description`) + + await skillsManager.discoverSkills() + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(0) + }) + it("should handle symlinked skills directory", async () => { const sharedSkillDir = p(SHARED_DIR, "shared-skill") const sharedSkillMd = p(sharedSkillDir, "SKILL.md") From 5551bba6e476ee8a3a2b50976961e475970c725b Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Tue, 30 Dec 2025 14:01:01 -0700 Subject: [PATCH 2/4] chore(skills): type-only import --- src/core/prompts/sections/skills.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/prompts/sections/skills.ts b/src/core/prompts/sections/skills.ts index ff3255bab2b..f7fea17c2a9 100644 --- a/src/core/prompts/sections/skills.ts +++ b/src/core/prompts/sections/skills.ts @@ -1,4 +1,4 @@ -import { SkillsManager, SkillMetadata } from "../../../services/skills/SkillsManager" +import type { SkillsManager } from "../../../services/skills/SkillsManager" type SkillsManagerLike = Pick From d993201fd74a0a25397a3004ff70012127608cce Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Tue, 30 Dec 2025 14:42:55 -0700 Subject: [PATCH 3/4] Make skills prompt tool-agnostic when loading SKILL.md --- src/core/prompts/sections/skills.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/core/prompts/sections/skills.ts b/src/core/prompts/sections/skills.ts index f7fea17c2a9..c4bdb2cabd6 100644 --- a/src/core/prompts/sections/skills.ts +++ b/src/core/prompts/sections/skills.ts @@ -56,7 +56,7 @@ How to use skills: Activate a skill: 1. Load the full SKILL.md content into context. - - Use execute_command to read it (e.g., cat ""). + - Read the file at "" to load its contents into context. 2. Follow the skill instructions precisely. 3. Only load additional bundled files (scripts/, references/, assets/) if the SKILL.md instructions require them. ` From 85712a2fed969ba87acf764ff673417f9d26e962 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Tue, 30 Dec 2025 15:20:25 -0700 Subject: [PATCH 4/4] improove the system prompt for skill checks. --- src/core/prompts/sections/skills.ts | 55 +++++++++++++++++++++++------ 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/src/core/prompts/sections/skills.ts b/src/core/prompts/sections/skills.ts index c4bdb2cabd6..954d0451bd1 100644 --- a/src/core/prompts/sections/skills.ts +++ b/src/core/prompts/sections/skills.ts @@ -48,16 +48,49 @@ AVAILABLE SKILLS ${skillsXml} -How to use skills: -- This list is already filtered for the current mode ("${currentMode}") and includes any mode-specific skills from skills-${currentMode}/ (with project overriding global). -- Select a skill ONLY when the user's request clearly matches the skill's . -- If multiple skills match, prefer the most specific one for the current task. -- Do NOT load every SKILL.md up front. Load the full SKILL.md only after you've decided to use that skill. - -Activate a skill: -1. Load the full SKILL.md content into context. - - Read the file at "" to load its contents into context. -2. Follow the skill instructions precisely. -3. Only load additional bundled files (scripts/, references/, assets/) if the SKILL.md instructions require them. + +REQUIRED PRECONDITION + +Before producing ANY user-facing response, you MUST perform a skill applicability check. + +Step 1: Skill Evaluation +- Evaluate the user's request against ALL available skill entries in . +- Determine whether at least one skill clearly and unambiguously applies. + +Step 2: Branching Decision + + +- Select EXACTLY ONE skill. +- Prefer the most specific skill when multiple skills match. +- Read the full SKILL.md file at the skill's . +- Load the SKILL.md contents fully into context BEFORE continuing. +- Follow the SKILL.md instructions precisely. +- Do NOT respond outside the skill-defined flow. + + + +- Proceed with a normal response. +- Do NOT load any SKILL.md files. + + +CONSTRAINTS: +- Do NOT load every SKILL.md up front. +- Load SKILL.md ONLY after a skill is selected. +- Do NOT skip this check. +- FAILURE to perform this check is an error. + + + +- The skill list is already filtered for the current mode: "${currentMode}". +- Mode-specific skills may come from skills-${currentMode}/ with project-level overrides taking precedence over global skills. + + + +This section is for internal control only. +Do NOT include this section in user-facing output. + +After completing the evaluation, internally confirm: +true|false + ` }