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..954d0451bd1 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" +import type { SkillsManager } 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,68 @@ 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) + 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") - let skillsList = "" + return `==== - if (modeSpecificSkills.length > 0) { - skillsList += modeSpecificSkills - .map( - (skill) => - ` * "${skill.name}" skill (${currentMode} mode) - ${skill.description} [${getDisplayPath(skill)}]`, - ) - .join("\n") - } +AVAILABLE SKILLS - if (genericSkills.length > 0) { - if (skillsList) skillsList += "\n" - skillsList += genericSkills - .map((skill) => ` * "${skill.name}" skill - ${skill.description} [${getDisplayPath(skill)}]`) - .join("\n") - } + +${skillsXml} + - return `==== + +REQUIRED PRECONDITION -AVAILABLE SKILLS +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. + -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. + +- 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. + -- These are the currently available skills for "${currentMode}" mode: -${skillsList} + +This section is for internal control only. +Do NOT include this section in user-facing output. -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 +After completing the evaluation, internally confirm: +true|false + ` } 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")