diff --git a/packages/types/src/skills.ts b/packages/types/src/skills.ts index 2f13b822eb..65768f7f29 100644 --- a/packages/types/src/skills.ts +++ b/packages/types/src/skills.ts @@ -20,6 +20,15 @@ export interface SkillMetadata { modeSlugs?: string[] } +/** A user-actionable problem found while loading a SKILL.md file. */ +export interface SkillDiagnostic { + path: string + source: "global" | "project" + message: string + line?: number + column?: number +} + /** * Skill name validation constants per agentskills.io specification: * https://agentskills.io/specification diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 75a72accde..6b391db0fa 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -14,7 +14,7 @@ import type { GitCommit } from "./git.js" import type { McpServer } from "./mcp.js" import type { ModelRecord, RouterModels } from "./model.js" import type { OpenAiCodexRateLimitInfo } from "./providers/openai-codex-rate-limits.js" -import type { SkillMetadata } from "./skills.js" +import type { SkillDiagnostic, SkillMetadata } from "./skills.js" import type { RuleMetadata } from "./rules.js" import type { TelemetrySetting } from "./telemetry.js" import type { WorktreeIncludeStatus } from "./worktree.js" @@ -180,6 +180,7 @@ export interface ExtensionMessage { list?: string[] // For dismissedUpsells tools?: SerializedCustomToolDefinition[] // For customToolsResult skills?: SkillMetadata[] // For skills response + skillDiagnostics?: SkillDiagnostic[] // For malformed skills omitted from the skills response rules?: RuleMetadata[] // For rules response modes?: { slug: string; name: string }[] // For modes response rooHistoryImportProgress?: { diff --git a/src/core/webview/__tests__/skillsMessageHandler.spec.ts b/src/core/webview/__tests__/skillsMessageHandler.spec.ts index 5d03b95a27..25e1b2b48c 100644 --- a/src/core/webview/__tests__/skillsMessageHandler.spec.ts +++ b/src/core/webview/__tests__/skillsMessageHandler.spec.ts @@ -47,6 +47,7 @@ describe("skillsMessageHandler", () => { const mockLog = vi.fn() const mockPostMessageToWebview = vi.fn() const mockGetSkillsMetadata = vi.fn() + const mockGetSkillDiagnostics = vi.fn() const mockCreateSkill = vi.fn() const mockDeleteSkill = vi.fn() const mockMoveSkill = vi.fn() @@ -57,6 +58,7 @@ describe("skillsMessageHandler", () => { const skillsManager = hasSkillsManager ? { getSkillsMetadata: mockGetSkillsMetadata, + getSkillDiagnostics: mockGetSkillDiagnostics, createSkill: mockCreateSkill, deleteSkill: mockDeleteSkill, moveSkill: mockMoveSkill, @@ -90,6 +92,7 @@ describe("skillsMessageHandler", () => { beforeEach(() => { vi.clearAllMocks() + mockGetSkillDiagnostics.mockReturnValue([]) }) describe("handleRequestSkills", () => { @@ -100,7 +103,34 @@ describe("skillsMessageHandler", () => { const result = await handleRequestSkills(provider) expect(result).toEqual(mockSkills) - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: mockSkills }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: mockSkills, + skillDiagnostics: [], + }) + }) + + it("sends structured malformed-skill diagnostics without hiding valid skills", async () => { + const provider = createMockProvider(true) + const diagnostics = [ + { + path: "/workspace/.roo/skills/broken/SKILL.md", + source: "project" as const, + message: "bad indentation of a mapping entry", + line: 3, + column: 20, + }, + ] + mockGetSkillsMetadata.mockReturnValue(mockSkills) + mockGetSkillDiagnostics.mockReturnValue(diagnostics) + + await handleRequestSkills(provider) + + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: mockSkills, + skillDiagnostics: diagnostics, + }) }) it("returns empty skills when skills manager is not available", async () => { @@ -109,7 +139,11 @@ describe("skillsMessageHandler", () => { const result = await handleRequestSkills(provider) expect(result).toEqual([]) - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: [] }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [], + skillDiagnostics: [], + }) }) it("handles errors and returns empty skills", async () => { @@ -122,7 +156,11 @@ describe("skillsMessageHandler", () => { expect(result).toEqual([]) expect(mockLog).toHaveBeenCalled() - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: [] }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [], + skillDiagnostics: [], + }) }) }) @@ -142,7 +180,11 @@ describe("skillsMessageHandler", () => { expect(result).toEqual(mockSkills) expect(mockCreateSkill).toHaveBeenCalledWith("new-skill", "global", "New skill description", undefined) expect(openFile).toHaveBeenCalledWith("/path/to/new-skill/SKILL.md") - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: mockSkills }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: mockSkills, + skillDiagnostics: [], + }) }) it("creates a skill with mode restriction", async () => { @@ -212,7 +254,11 @@ describe("skillsMessageHandler", () => { expect(result).toEqual([mockSkills[1]]) expect(mockDeleteSkill).toHaveBeenCalledWith("test-skill", "global", undefined) - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: [mockSkills[1]] }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [mockSkills[1]], + skillDiagnostics: [], + }) }) it("deletes a skill with mode restriction", async () => { @@ -280,7 +326,11 @@ describe("skillsMessageHandler", () => { expect(result).toEqual([mockSkills[0]]) expect(mockMoveSkill).toHaveBeenCalledWith("test-skill", "global", undefined, "code") - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: [mockSkills[0]] }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [mockSkills[0]], + skillDiagnostics: [], + }) }) it("moves a skill from one mode to another", async () => { diff --git a/src/core/webview/skillsMessageHandler.ts b/src/core/webview/skillsMessageHandler.ts index 496ff70c24..6781082933 100644 --- a/src/core/webview/skillsMessageHandler.ts +++ b/src/core/webview/skillsMessageHandler.ts @@ -16,15 +16,16 @@ export async function handleRequestSkills(provider: ClineProvider): Promise = new Map() + private diagnostics: SkillDiagnostic[] = [] private providerRef: WeakRef private disposables: vscode.Disposable[] = [] private isDisposed = false @@ -42,6 +43,7 @@ export class SkillsManager { */ async discoverSkills(): Promise { this.skills.clear() + this.diagnostics = [] const skillsDirs = await this.getSkillsDirectories() for (const { dir, source, mode } of skillsDirs) { @@ -100,9 +102,17 @@ export class SkillsManager { try { const fileContent = await fs.readFile(skillMdPath, "utf-8") + let parsed: ReturnType - // Use gray-matter to parse frontmatter - const { data: frontmatter, content: body } = matter(fileContent) + try { + parsed = matter(fileContent) + } catch (error) { + this.recordDiagnostic(skillMdPath, source, error) + console.error(`Failed to parse skill at ${skillDir}:`, error) + return + } + + const { data: frontmatter } = parsed // Validate required fields (only name and description for now) if (!frontmatter.name || typeof frontmatter.name !== "string") { @@ -175,6 +185,20 @@ export class SkillsManager { } } + private recordDiagnostic(path: string, source: "global" | "project", error: unknown): void { + const yamlError = error as { + reason?: unknown + message?: unknown + mark?: { line?: unknown; column?: unknown } + } + const reason = typeof yamlError.reason === "string" ? yamlError.reason : undefined + const errorMessage = error instanceof Error ? error.message.split("\n", 1)[0] : String(error) + const line = typeof yamlError.mark?.line === "number" ? yamlError.mark.line + 1 : undefined + const column = typeof yamlError.mark?.column === "number" ? yamlError.mark.column + 1 : undefined + + this.diagnostics.push({ path, source, message: reason ?? errorMessage, line, column }) + } + /** * Get skills available for the current mode. * Resolves overrides: project > global, mode-specific > generic. @@ -290,6 +314,10 @@ export class SkillsManager { return this.getAllSkills() } + getSkillDiagnostics(): SkillDiagnostic[] { + return [...this.diagnostics] + } + /** * Get a skill by name, source, and optionally mode */ @@ -394,25 +422,19 @@ export class SkillsManager { .map((word) => word.charAt(0).toUpperCase() + word.slice(1)) .join(" ") - // Build frontmatter with optional modeSlugs - const frontmatterLines = [`name: ${name}`, `description: ${trimmedDescription}`] - if (modeSlugs && modeSlugs.length > 0) { - frontmatterLines.push(`modeSlugs:`) - for (const slug of modeSlugs) { - frontmatterLines.push(` - ${slug}`) - } + const frontmatter = { + name, + description: trimmedDescription, + ...(modeSlugs && modeSlugs.length > 0 ? { modeSlugs } : {}), } - - const skillContent = `--- -${frontmatterLines.join("\n")} ---- - + const body = ` # ${titleName} ## Instructions Add your skill instructions here. ` + const skillContent = matter.stringify(body, frontmatter) // Write the SKILL.md file await fs.writeFile(skillMdPath, skillContent, "utf-8") diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts index d36582d893..5dbedff8fa 100644 --- a/src/services/skills/__tests__/SkillsManager.spec.ts +++ b/src/services/skills/__tests__/SkillsManager.spec.ts @@ -1,4 +1,5 @@ import * as path from "path" +import matter from "gray-matter" // Use vi.hoisted to ensure mocks are available during hoisting const { @@ -1233,6 +1234,47 @@ Instructions`) expect(writeCall[1]).toContain("- code") }) + it("round-trips the issue description and mode slugs through YAML frontmatter", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockResolvedValue(undefined) + const description = 'Triggers on: "TDD", "test-driven development"' + + await skillsManager.createSkill("tdd-skill", "global", description, ["code", "debug"]) + + const generatedContent = mockWriteFile.mock.calls[0][1] as string + const parsed = matter(generatedContent) + expect(parsed.data).toEqual({ + name: "tdd-skill", + description, + modeSlugs: ["code", "debug"], + }) + expect(parsed.content).toBe("\n# Tdd Skill\n\n## Instructions\n\nAdd your skill instructions here.\n") + }) + + it.each([ + ["colon-space", "Use when: tests fail"], + ["quotes", "Use \"red-green-refactor\" and 'small steps'"], + ["hash", "Use tests # not comments"], + ["leading punctuation", "- Start from a failing test"], + ["multiline", "First line\nSecond line: with # and quotes"], + ])("preserves YAML-sensitive %s descriptions", async (_caseName, description) => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockResolvedValue(undefined) + + await skillsManager.createSkill("yaml-sensitive", "global", description) + + const generatedContent = mockWriteFile.mock.calls[0][1] as string + expect(matter(generatedContent).data.description).toBe(description) + }) + it("should create a project skill", async () => { mockDirectoryExists.mockResolvedValue(false) mockRealpath.mockImplementation(async (p: string) => p) @@ -1299,6 +1341,45 @@ Instructions`) }) }) + describe("skill diagnostics", () => { + it("reports malformed YAML with location while retaining valid skills", async () => { + const validSkillDir = p(globalSkillsDir, "valid-skill") + const malformedSkillDir = p(globalSkillsDir, "malformed-skill") + const validSkillPath = p(validSkillDir, "SKILL.md") + const malformedSkillPath = p(malformedSkillDir, "SKILL.md") + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => + dir === globalSkillsDir ? ["valid-skill", "malformed-skill"] : [], + ) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockImplementation(async (file: string) => + [validSkillPath, malformedSkillPath].includes(file), + ) + mockReadFile.mockImplementation(async (file: string) => { + if (file === validSkillPath) { + return "---\nname: valid-skill\ndescription: Still visible\n---\nValid instructions" + } + return '---\nname: malformed-skill\ndescription: Triggers on: "TDD"\n---\nBroken instructions' + }) + + await skillsManager.discoverSkills() + + expect(skillsManager.getSkillsMetadata()).toEqual([ + expect.objectContaining({ name: "valid-skill", description: "Still visible" }), + ]) + expect(skillsManager.getSkillDiagnostics()).toEqual([ + expect.objectContaining({ + path: malformedSkillPath, + source: "global", + message: expect.any(String), + line: 3, + column: expect.any(Number), + }), + ]) + }) + }) + describe("deleteSkill", () => { it("should delete an existing skill", async () => { const testSkillDir = p(globalSkillsDir, "test-skill") diff --git a/src/shared/skills.ts b/src/shared/skills.ts index f5151181f6..6dc2cbf8ad 100644 --- a/src/shared/skills.ts +++ b/src/shared/skills.ts @@ -20,6 +20,15 @@ export interface SkillMetadata { modeSlugs?: string[] } +/** A user-actionable problem found while loading a SKILL.md file. */ +export interface SkillDiagnostic { + path: string + source: "global" | "project" + message: string + line?: number + column?: number +} + /** * Full skill content (loaded on activation) */ diff --git a/webview-ui/src/components/settings/SkillsSettings.tsx b/webview-ui/src/components/settings/SkillsSettings.tsx index bf81d0c6c7..e6480ba61d 100644 --- a/webview-ui/src/components/settings/SkillsSettings.tsx +++ b/webview-ui/src/components/settings/SkillsSettings.tsx @@ -1,5 +1,5 @@ import React, { useState, useEffect, useMemo, useCallback } from "react" -import { Plus, Globe, Folder, Edit, Trash2, Settings } from "lucide-react" +import { Plus, Globe, Folder, Edit, Trash2, Settings, TriangleAlert } from "lucide-react" import { Trans } from "react-i18next" import type { SkillMetadata } from "@roo-code/types" @@ -35,7 +35,7 @@ import { CreateSkillDialog } from "./CreateSkillDialog" export const SkillsSettings: React.FC = () => { const { t } = useAppTranslation() - const { cwd, skills: rawSkills, customModes } = useExtensionState() + const { cwd, skills: rawSkills, skillDiagnostics, customModes } = useExtensionState() const skills = useMemo(() => rawSkills ?? [], [rawSkills]) const [deleteDialogOpen, setDeleteDialogOpen] = useState(false) @@ -232,6 +232,34 @@ export const SkillsSettings: React.FC = () => { }} />

+ {skillDiagnostics.length > 0 && ( +
+ +
+
{t("settings:skills.diagnostics.title")}
+
+ {t("settings:skills.diagnostics.description")} +
+
    + {skillDiagnostics.map((diagnostic) => { + const location = diagnostic.line + ? `:${diagnostic.line}${diagnostic.column ? `:${diagnostic.column}` : ""}` + : "" + return ( +
  • + + {diagnostic.path + location} + + : {diagnostic.message} +
    • + ) + })} +
+
+
+ )} {/* Add Skill button */}