feat(skills): align with Agent Skills spec (#10409)

src/core/prompts/sections/__tests__/skills.spec.ts

@@ -0,0 +1,32 @@
+import { getSkillsSection } from "../skills"
+
+describe("getSkillsSection", () => {
+	it("should emit <available_skills> 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("<available_skills>")
+		expect(result).toContain("</available_skills>")
+		expect(result).toContain("<skill>")
+		expect(result).toContain("<name>pdf-processing</name>")
+		// Ensure XML escaping for '&'
+		expect(result).toContain("<description>Extracts text &amp; tables from PDFs</description>")
+		// For filesystem-based agents, location should be the absolute path to SKILL.md
+		expect(result).toContain("<location>/abs/path/pdf-processing/SKILL.md</location>")
+	})
+
+	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("")
+	})
+})

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<SkillsManager, "getSkillsForMode">
+
+function escapeXml(value: string): string {
+	return value
+		.replace(/&/g, "&amp;")
+		.replace(/</g, "&lt;")
+		.replace(/>/g, "&gt;")
+		.replace(/\"/g, "&quot;")
+		.replace(/'/g, "&apos;")
 }
 
 /**
@@ -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<string> {
 	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)
-
-	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 `  <skill>\n    <name>${name}</name>\n    <description>${description}</description>\n    <location>${location}</location>\n  </skill>`
+		})
+		.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.
+<available_skills>
+${skillsXml}
+</available_skills>
 
-- These are the currently available skills for "${currentMode}" mode:
-${skillsList}
+<mandatory_skill_check>
+REQUIRED PRECONDITION
 
-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
+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 <description> entries in <available_skills>.
+- Determine whether at least one skill clearly and unambiguously applies.
+
+Step 2: Branching Decision
+
+<if_skill_applies>
+- Select EXACTLY ONE skill.
+- Prefer the most specific skill when multiple skills match.
+- Read the full SKILL.md file at the skill's <location>.
+- Load the SKILL.md contents fully into context BEFORE continuing.
+- Follow the SKILL.md instructions precisely.
+- Do NOT respond outside the skill-defined flow.
+</if_skill_applies>
+
+<if_no_skill_applies>
+- Proceed with a normal response.
+- Do NOT load any SKILL.md files.
+</if_no_skill_applies>
+
+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.
+</mandatory_skill_check>
+
+<context_notes>
+- 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.
+</context_notes>
+
+<internal_verification>
+This section is for internal control only.
+Do NOT include this section in user-facing output.
+
+After completing the evaluation, internally confirm:
+<skill_check_completed>true|false</skill_check_completed>
+</internal_verification>
 `
 }

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

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")