diff --git a/apps/vscode-e2e/src/suite/skills-diagnostics.test.ts b/apps/vscode-e2e/src/suite/skills-diagnostics.test.ts new file mode 100644 index 0000000000..71662e6cfe --- /dev/null +++ b/apps/vscode-e2e/src/suite/skills-diagnostics.test.ts @@ -0,0 +1,138 @@ +import * as assert from "assert" +import * as fs from "fs/promises" +import * as path from "path" + +import * as vscode from "vscode" + +import { setDefaultSuiteTimeout } from "./test-utils" +import { waitFor } from "./utils" + +const GOOD_SKILL = "e2e-skill-good" +const BAD_SKILL = "e2e-skill-bad" + +// Issue #859 reproduction content: the description is a double-quoted YAML +// scalar whose inner double quotes are left unescaped, which makes the +// frontmatter unparseable. +const MALFORMED_SKILL_MD = `--- +name: ${BAD_SKILL} +description: "Use when implementing features. Triggers on: "TDD", "test-driven development" +--- + +# E2E Skill Bad + +Instructions here. +` + +const FIXED_SKILL_MD = `--- +name: ${BAD_SKILL} +description: 'Use when implementing features. Triggers on: "TDD", "test-driven development"' +--- + +# E2E Skill Bad + +Instructions here. +` + +const GOOD_SKILL_MD = `--- +name: ${GOOD_SKILL} +description: A healthy skill used by the skill diagnostics e2e smoke test. +--- + +# E2E Skill Good + +Instructions here. +` + +// Write a skill file atomically (write to a sidecar, then rename over the +// target) so the extension host's file watcher only ever observes complete +// content. An in-place fs.writeFile is visible mid-write, the watcher can +// fire for that moment, and - because discovery scans are serialized - a +// mid-write event could be the last one, leaving a stale scan result. +const writeSkillFileAtomic = async (finalPath: string, content: string): Promise => { + const tmpPath = `${finalPath}.tmp` + await fs.writeFile(tmpPath, content, "utf8") + await fs.rename(tmpPath, finalPath) +} + +suite("Roo Code Skill Diagnostics", function () { + setDefaultSuiteTimeout(this) + + let skillsRoot: string + + setup(async function () { + const workspaceRoot = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath + assert.ok(workspaceRoot, "e2e workspace folder must be open") + skillsRoot = path.join(workspaceRoot, ".roo", "skills") + }) + + teardown(async function () { + // Remove only the skill directories this suite created so pre-existing + // or other suites' fixtures under .roo/skills are left intact. + await Promise.all( + [GOOD_SKILL, BAD_SKILL].map((name) => fs.rm(path.join(skillsRoot, name), { recursive: true, force: true })), + ) + }) + + test("should surface a malformed SKILL.md as a diagnostic without hiding healthy skills", async function () { + this.timeout(180_000) + + // Arrange: one healthy skill and one malformed skill on real disk in the + // workspace's .roo/skills directory, written atomically so the watcher + // only observes complete files. + await fs.mkdir(path.join(skillsRoot, GOOD_SKILL), { recursive: true }) + await writeSkillFileAtomic(path.join(skillsRoot, GOOD_SKILL, "SKILL.md"), GOOD_SKILL_MD) + await fs.mkdir(path.join(skillsRoot, BAD_SKILL), { recursive: true }) + const badSkillMd = path.join(skillsRoot, BAD_SKILL, "SKILL.md") + await writeSkillFileAtomic(badSkillMd, MALFORMED_SKILL_MD) + + // Act: the extension host's file watcher re-discovers skills; wait until + // the real SkillsManager reports the healthy skill and a diagnostic for + // the malformed one. + await waitFor( + async () => { + const state = globalThis.api.getSkillsState() + const goodVisible = state.skills.some((skill) => skill.name === GOOD_SKILL) + const badDiagnosed = state.skillDiagnostics.some((diagnostic) => diagnostic.path.includes(BAD_SKILL)) + return goodVisible && badDiagnosed + }, + { timeout: 60_000, interval: 500 }, + ) + + // Assert: the malformed skill is skipped with a diagnostic pointing at it, + // while the healthy skill is unaffected. + const state = globalThis.api.getSkillsState() + const diagnostic = state.skillDiagnostics.find((d) => d.path.includes(BAD_SKILL)) + assert.ok(diagnostic, "malformed SKILL.md should produce a diagnostic") + assert.strictEqual(diagnostic.source, "project") + assert.ok(diagnostic.message.length > 0, "diagnostic should carry the parse error message") + assert.ok( + state.skills.some((skill) => skill.name === GOOD_SKILL), + "healthy skill should still be discovered", + ) + assert.ok( + !state.skills.some((skill) => skill.name === BAD_SKILL), + "malformed skill should be omitted from skills", + ) + + // Act: repair the frontmatter in place (atomically); the watcher + // re-discovers and the diagnostic clears. + await writeSkillFileAtomic(badSkillMd, FIXED_SKILL_MD) + + await waitFor( + async () => { + const next = globalThis.api.getSkillsState() + const cleared = !next.skillDiagnostics.some((d) => d.path.includes(BAD_SKILL)) + const loaded = next.skills.some((skill) => skill.name === BAD_SKILL) + return cleared && loaded + }, + { timeout: 60_000, interval: 500 }, + ) + + const fixed = globalThis.api.getSkillsState() + assert.ok( + fixed.skills.some((skill) => skill.name === BAD_SKILL), + "fixed skill should load after repair", + ) + assert.ok(fixed.skills.find((skill) => skill.name === BAD_SKILL)?.description.includes("TDD") === true) + }) +}) diff --git a/packages/types/src/api.ts b/packages/types/src/api.ts index 89e9c8bc2b..b702f67b32 100644 --- a/packages/types/src/api.ts +++ b/packages/types/src/api.ts @@ -6,6 +6,7 @@ import type { RooCodeSettings } from "./global-settings.js" import type { HistoryItem } from "./history.js" import type { ProviderSettingsEntry, ProviderSettings } from "./provider-settings.js" import type { IpcMessage, IpcServerEvents } from "./ipc.js" +import type { SkillDiagnostic, SkillMetadata } from "./skills.js" export type RooCodeAPIEvents = RooCodeEvents @@ -51,6 +52,15 @@ export interface RooCodeAPI extends EventEmitter { * @returns The number of persisted API conversation history entries, or 0 if unavailable. */ getTaskApiConversationHistoryLength(taskId: string): Promise + /** + * Returns the skill metadata and load diagnostics currently discovered by the + * extension host. Intended for use in tests only. + * @returns The discovered skills and any diagnostics for skills that failed to load. + */ + getSkillsState(): { + skills: SkillMetadata[] + skillDiagnostics: SkillDiagnostic[] + } /** * Returns the current task stack. * @returns An array of task IDs. 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 ea52c09599..8b9cce6586 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -18,7 +18,7 @@ import { OllamaModelsMessageType } from "./providers/ollama.js" import { OpenAiModelsMessageType } from "./providers/openai.js" import { VsCodeLmModelsMessageType } from "./providers/vscode-llm.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" @@ -184,6 +184,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..f5fa7eec38 100644 --- a/src/core/webview/__tests__/skillsMessageHandler.spec.ts +++ b/src/core/webview/__tests__/skillsMessageHandler.spec.ts @@ -27,6 +27,7 @@ vi.mock("../../../i18n", () => ({ "skills:errors.manager_unavailable": "Skills manager not available", "skills:errors.missing_delete_fields": "Missing required fields: skillName or source", "skills:errors.missing_move_fields": "Missing required fields: skillName or source", + "skills:errors.missing_update_modes_fields": "Missing required fields: skillName or source", "skills:errors.skill_not_found": `Skill "${params?.name}" not found`, } return translations[key] || key @@ -41,15 +42,18 @@ import { handleDeleteSkill, handleMoveSkill, handleOpenSkillFile, + handleUpdateSkillModes, } from "../skillsMessageHandler" 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() + const mockUpdateSkillModes = vi.fn() const mockGetSkill = vi.fn() const mockFindSkillByNameAndSource = vi.fn() @@ -57,9 +61,11 @@ describe("skillsMessageHandler", () => { const skillsManager = hasSkillsManager ? { getSkillsMetadata: mockGetSkillsMetadata, + getSkillDiagnostics: mockGetSkillDiagnostics, createSkill: mockCreateSkill, deleteSkill: mockDeleteSkill, moveSkill: mockMoveSkill, + updateSkillModes: mockUpdateSkillModes, getSkill: mockGetSkill, findSkillByNameAndSource: mockFindSkillByNameAndSource, } @@ -90,6 +96,7 @@ describe("skillsMessageHandler", () => { beforeEach(() => { vi.clearAllMocks() + mockGetSkillDiagnostics.mockReturnValue([]) }) describe("handleRequestSkills", () => { @@ -100,7 +107,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 +143,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 +160,11 @@ describe("skillsMessageHandler", () => { expect(result).toEqual([]) expect(mockLog).toHaveBeenCalled() - expect(mockPostMessageToWebview).toHaveBeenCalledWith({ type: "skills", skills: [] }) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [], + skillDiagnostics: [], + }) }) }) @@ -142,7 +184,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 +258,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 +330,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 () => { @@ -334,6 +388,132 @@ describe("skillsMessageHandler", () => { }) }) + describe("handleUpdateSkillModes", () => { + it("updates a skill's mode slugs successfully", async () => { + const provider = createMockProvider(true) + mockUpdateSkillModes.mockResolvedValue(undefined) + mockGetSkillsMetadata.mockReturnValue([mockSkills[0]]) + + const result = await handleUpdateSkillModes(provider, { + type: "updateSkillModes", + skillName: "test-skill", + source: "global", + newSkillModeSlugs: ["code"], + } as WebviewMessage) + + expect(result).toEqual([mockSkills[0]]) + expect(mockUpdateSkillModes).toHaveBeenCalledWith("test-skill", "global", ["code"]) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [mockSkills[0]], + skillDiagnostics: [], + }) + }) + + it("clears a skill's mode restriction with empty slugs", async () => { + const provider = createMockProvider(true) + mockUpdateSkillModes.mockResolvedValue(undefined) + mockGetSkillsMetadata.mockReturnValue([mockSkills[1]]) + + const result = await handleUpdateSkillModes(provider, { + type: "updateSkillModes", + skillName: "project-skill", + source: "project", + newSkillModeSlugs: [], + } as WebviewMessage) + + expect(result).toEqual([mockSkills[1]]) + expect(mockUpdateSkillModes).toHaveBeenCalledWith("project-skill", "project", []) + }) + + it("passes undefined mode slugs and refreshes state when newSkillModeSlugs is omitted", async () => { + const provider = createMockProvider(true) + mockUpdateSkillModes.mockResolvedValue(undefined) + mockGetSkillsMetadata.mockReturnValue([mockSkills[0]]) + // Forward a concrete (non-empty) diagnostic so the assertion proves the + // handler relays the diagnostics list rather than always posting [] + // (which would pass even if the field were dropped or hard-coded). + const diagnostics = [ + { + path: "/global/.roo/skills/broken/SKILL.md", + source: "global" as const, + message: "can not read a block mapping entry", + line: 3, + column: 10, + }, + ] + mockGetSkillDiagnostics.mockReturnValue(diagnostics) + + const message: WebviewMessage = { + type: "updateSkillModes", + skillName: "test-skill", + source: "global", + // newSkillModeSlugs omitted + } + const result = await handleUpdateSkillModes(provider, message) + + expect(result).toEqual([mockSkills[0]]) + expect(mockUpdateSkillModes).toHaveBeenCalledWith("test-skill", "global", undefined) + expect(mockPostMessageToWebview).toHaveBeenCalledWith({ + type: "skills", + skills: [mockSkills[0]], + skillDiagnostics: diagnostics, + }) + }) + + it("returns undefined when required fields are missing", async () => { + const provider = createMockProvider(true) + + const result = await handleUpdateSkillModes(provider, { + type: "updateSkillModes", + skillName: "test-skill", + // missing source + } as WebviewMessage) + + expect(result).toBeUndefined() + expect(mockUpdateSkillModes).not.toHaveBeenCalled() + expect(mockLog).toHaveBeenCalledWith( + "Error updating skill modes: Missing required fields: skillName or source", + ) + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to update skill modes: Missing required fields: skillName or source", + ) + }) + + it("returns undefined when skills manager is not available", async () => { + const provider = createMockProvider(false) + + const result = await handleUpdateSkillModes(provider, { + type: "updateSkillModes", + skillName: "test-skill", + source: "global", + newSkillModeSlugs: ["code"], + } as WebviewMessage) + + expect(result).toBeUndefined() + expect(mockLog).toHaveBeenCalledWith("Error updating skill modes: Skills manager not available") + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith( + "Failed to update skill modes: Skills manager not available", + ) + }) + + it("returns undefined and reports the error when updateSkillModes rejects", async () => { + const provider = createMockProvider(true) + mockUpdateSkillModes.mockRejectedValue(new Error("boom")) + + const result = await handleUpdateSkillModes(provider, { + type: "updateSkillModes", + skillName: "test-skill", + source: "global", + newSkillModeSlugs: ["code"], + } as WebviewMessage) + + expect(result).toBeUndefined() + expect(mockLog).toHaveBeenCalledWith("Error updating skill modes: boom") + expect(vscode.window.showErrorMessage).toHaveBeenCalledWith("Failed to update skill modes: boom") + }) + }) + describe("handleOpenSkillFile", () => { it("opens a skill file successfully", async () => { const provider = createMockProvider(true) diff --git a/src/core/webview/skillsMessageHandler.ts b/src/core/webview/skillsMessageHandler.ts index 2eab270734..8687791c59 100644 --- a/src/core/webview/skillsMessageHandler.ts +++ b/src/core/webview/skillsMessageHandler.ts @@ -16,15 +16,16 @@ export async function handleRequestSkills(provider: ClineProvider): Promise { + let api: API + let mockOutputChannel: vscode.OutputChannel + let mockProvider: ClineProvider + let mockGetSkillsManager: ReturnType + + beforeEach(() => { + // mockOutputChannel and mockProvider are intentionally partial + // doubles: they implement only the members API touches (appendLine; + // context, getSkillsManager, on). The as-unknown-as casts are the + // last resort because the partial shapes are not subtypes of the full + // vscode.OutputChannel / ClineProvider types. + mockOutputChannel = { + appendLine: vi.fn(), + } as unknown as vscode.OutputChannel + + mockGetSkillsManager = vi.fn() + + mockProvider = { + context: {} as vscode.ExtensionContext, + getSkillsManager: mockGetSkillsManager, + on: vi.fn(), + } as unknown as ClineProvider + + api = new API(mockOutputChannel, mockProvider, undefined, true) + }) + + it("returns the skills and diagnostics from the skills manager", () => { + const skills = [ + { + name: "good-skill", + description: "A healthy skill.", + path: "/skills/good-skill/SKILL.md", + source: "project" as const, + }, + ] + const skillDiagnostics = [ + { path: "/skills/bad-skill/SKILL.md", source: "project" as const, message: "YAML syntax error", line: 3 }, + ] + mockGetSkillsManager.mockReturnValue({ + getSkillsMetadata: vi.fn().mockReturnValue(skills), + getSkillDiagnostics: vi.fn().mockReturnValue(skillDiagnostics), + }) + + expect(api.getSkillsState()).toEqual({ skills, skillDiagnostics }) + }) + + it("returns empty arrays when the skills manager is unavailable", () => { + mockGetSkillsManager.mockReturnValue(undefined) + + expect(api.getSkillsState()).toEqual({ skills: [], skillDiagnostics: [] }) + }) +}) diff --git a/src/extension/api.ts b/src/extension/api.ts index b57dc89b74..80c2e90864 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -250,6 +250,17 @@ export class API extends EventEmitter implements RooCodeAPI { } } + public getSkillsState() { + const skillsManager = this.sidebarProvider.getSkillsManager() + if (!skillsManager) { + return { skills: [], skillDiagnostics: [] } + } + return { + skills: skillsManager.getSkillsMetadata(), + skillDiagnostics: skillsManager.getSkillDiagnostics(), + } + } + public getCurrentTaskStack() { return this.sidebarProvider.getCurrentTaskStack() } diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts index 0959b977c9..bdf3ee5f93 100644 --- a/src/services/skills/SkillsManager.ts +++ b/src/services/skills/SkillsManager.ts @@ -6,7 +6,7 @@ import matter from "gray-matter" import type { ClineProvider } from "../../core/webview/ClineProvider" import { getGlobalRooDirectory, getGlobalAgentsDirectory, getProjectAgentsDirectoryForCwd } from "../roo-config" import { directoryExists, fileExists } from "../roo-config" -import { SkillMetadata, SkillContent } from "../../shared/skills" +import { SkillMetadata, SkillContent, SkillDiagnostic } from "../../shared/skills" import { modes, getAllModes } from "../../shared/modes" import { validateSkillName as validateSkillNameShared, @@ -16,13 +16,49 @@ import { import { t } from "../../i18n" // Re-export for convenience -export type { SkillMetadata, SkillContent } +export type { SkillMetadata, SkillContent, SkillDiagnostic } + +/** + * Extract the raw top-level `key: ...` line from the frontmatter block of a + * SKILL.md file, along with its 1-based line number in the file. Used to + * point users at the exact line when YAML parsing fails (see issue #859). + * Returns undefined when the file has no frontmatter block or no matching + * top-level line. + */ +function getFrontmatterLine(fileContent: string, key: string): { line: string; lineNumber: number } | undefined { + const match = fileContent.match(/^---\r?\n([\s\S]*?)\r?\n---/) + if (!match) { + return undefined + } + const linePattern = new RegExp(`^${key}\\s*:`) + const frontmatterLines = match[1].split(/\r?\n/) + const index = frontmatterLines.findIndex((line) => linePattern.test(line)) + if (index === -1) { + return undefined + } + // The opening `---` occupies file line 1, so frontmatter index 0 is file line 2. + return { line: frontmatterLines[index], lineNumber: index + 2 } +} + +// gray-matter's bundled typings predate its options passthrough: `stringify` +// forwards its options object to js-yaml's safeDump, so js-yaml dump options +// such as `lineWidth` are honored at runtime even though the declared +// `GrayMatterOption` interface does not list them. This type names the +// supported subset explicitly (see issue #859). +type SkillYamlDumpOptions = matter.GrayMatterOption & { lineWidth: number } + +// `lineWidth: -1` keeps long plain scalars (e.g. descriptions) on a single +// line instead of reflowing them into folded block scalars, keeping SKILL.md +// files stable across edits. +const SKILL_YAML_DUMP_OPTIONS: SkillYamlDumpOptions = { lineWidth: -1 } export class SkillsManager { private skills: Map = new Map() + private diagnostics: SkillDiagnostic[] = [] private providerRef: WeakRef private disposables: vscode.Disposable[] = [] private isDisposed = false + private discoveryChain: Promise = Promise.resolve() constructor(provider: ClineProvider) { this.providerRef = new WeakRef(provider) @@ -39,9 +75,21 @@ export class SkillsManager { * Also supports symlinks: * - .roo/skills can be a symlink to a directory containing skill subdirectories * - .roo/skills/[dirname] can be a symlink to a skill directory + * + * Scans are serialized so overlapping watcher-triggered runs never + * interleave: an older scan must not commit state after a newer scan has + * already observed the (possibly repaired) files. */ - async discoverSkills(): Promise { + discoverSkills(): Promise { + const run = this.discoveryChain.then(() => this.performDiscovery()) + // Keep the chain alive even if a scan rejects so later scans still run. + this.discoveryChain = run.catch(() => undefined) + return run + } + + private async performDiscovery(): Promise { this.skills.clear() + this.diagnostics = [] const skillsDirs = await this.getSkillsDirectories() for (const { dir, source, mode } of skillsDirs) { @@ -100,9 +148,45 @@ export class SkillsManager { try { const fileContent = await fs.readFile(skillMdPath, "utf-8") + let parsed: ReturnType + + // Parse errors are handled separately from field validation so that + // YAML syntax problems (e.g. unescaped double quotes in the + // description) report the actual cause instead of a misleading + // "missing required field" message (see issue #859). + // + // The `{}` options argument is deliberate: gray-matter keeps a global + // content-keyed cache and populates it *before* parsing, so a + // frontmatter that throws on first parse is cached with an empty data + // object and every later parse of the same content silently returns + // that empty object instead of re-throwing. Passing options disables + // the cache for this call, keeping the parse deterministic. + try { + parsed = matter(fileContent, {}) + } catch (error) { + this.recordDiagnostic(skillMdPath, source, error) + console.error(`Failed to parse skill at ${skillDir}:`, error) + // The most common cause is unescaped double quotes in the + // description value. Only hint at that when the parser error is + // located on the description line itself, so a valid quoted + // description plus an unrelated YAML error elsewhere does not + // produce a misleading hint (see issue #859). + const description = getFrontmatterLine(fileContent, "description") + const errorMark = (error as { mark?: { line?: unknown } }).mark + if ( + description?.line.includes('"') && + typeof errorMark?.line === "number" && + errorMark.line + 1 === description.lineNumber + ) { + console.error( + `Hint: the "description" value in ${skillMdPath} contains unescaped double quotes. ` + + "Wrap the value in single quotes (or escape the double quotes) and save.", + ) + } + return + } - // Use gray-matter to parse frontmatter - const { data: frontmatter, content: body } = matter(fileContent) + const { data: frontmatter } = parsed // Validate required fields (only name and description for now) if (!frontmatter.name || typeof frontmatter.name !== "string") { @@ -175,6 +259,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 +388,10 @@ export class SkillsManager { return this.getAllSkills() } + getSkillDiagnostics(): SkillDiagnostic[] { + return [...this.diagnostics] + } + /** * Get a skill by name, source, and optionally mode */ @@ -394,25 +496,25 @@ 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 skillContent = `--- -${frontmatterLines.join("\n")} ---- - + // Build the frontmatter as data and serialize it with gray-matter so + // values containing YAML special characters (e.g. double quotes in the + // description) are quoted/escaped automatically. Hand-built frontmatter + // produced unparseable SKILL.md files that silently failed to load + // (see issue #859). lineWidth: -1 keeps plain values on a single line, + // matching the previous output format. + const frontmatter = { + name, + description: trimmedDescription, + ...(modeSlugs && modeSlugs.length > 0 ? { modeSlugs } : {}), + } + const body = ` # ${titleName} ## Instructions Add your skill instructions here. ` + const skillContent = matter.stringify(body, frontmatter, SKILL_YAML_DUMP_OPTIONS) // Write the SKILL.md file await fs.writeFile(skillMdPath, skillContent, "utf-8") @@ -553,8 +655,10 @@ Add your skill instructions here. delete frontmatter.mode } - // Serialize back to SKILL.md format - const newContent = matter.stringify(body, frontmatter) + // Serialize back to SKILL.md format. lineWidth: -1 prevents long + // values from being reflowed into folded block scalars, keeping the + // file stable (see issue #859). + const newContent = matter.stringify(body, frontmatter, SKILL_YAML_DUMP_OPTIONS) await fs.writeFile(skill.path, newContent, "utf-8") // Refresh skills list diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts index d36582d893..1aac22732f 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 { @@ -67,6 +68,20 @@ vi.mock("os", () => ({ homedir: mockHomedir, })) +// Keep the real gray-matter parser by default, but let individual tests script +// the failure shape (e.g. a non-Error throw) to exercise recordDiagnostic's +// defensive fallbacks (see issue #859). vi.importActual resolves the CJS module +// to a namespace that exposes the parser as `default`, which the module's +// declared `export =` type does not carry, so the namespace is bridged through +// `unknown` once; the double assertion then bridges the mock back to the +// module's declared type. +vi.mock("gray-matter", async () => { + const actual = await vi.importActual("gray-matter") + const realParse = (actual as { default: typeof matter }).default + const parse = Object.assign(vi.fn(realParse), actual) as unknown as typeof matter + return { default: parse } +}) + // Mock vscode vi.mock("vscode", () => ({ workspace: { @@ -391,6 +406,162 @@ description: Name doesn't match directory expect(skills).toHaveLength(0) }) + it("should skip skills with invalid YAML frontmatter and log the real cause (issue #859)", async () => { + const badSkillDir = p(globalSkillsDir, "test-skill") + const badSkillMd = p(badSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir + }) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir ? ["test-skill"] : [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === badSkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === badSkillMd) + mockReadFile.mockImplementation(async (file: string) => { + if (file === badSkillMd) { + return `--- +name: test-skill +description: Use when implementing features. Triggers on: "TDD", "test-driven development" +--- + +# Test Skill + +Instructions here.` + } + throw new Error("File not found") + }) + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await skillsManager.discoverSkills() + + // The skill is skipped... + expect(skillsManager.getAllSkills()).toHaveLength(0) + + // ...and the real cause (a YAML syntax error) is reported instead + // of the misleading "missing required 'name' field" message. + const logged = consoleErrorSpy.mock.calls.map((call) => call.join(" ")) + expect(logged.some((line) => line.includes("YAMLException"))).toBe(true) + expect(logged.some((line) => line.includes("missing required 'name' field"))).toBe(false) + // The unescaped-double-quotes hint points at the exact problem. + expect(logged.some((line) => line.includes("unescaped double quotes"))).toBe(true) + + // The structured diagnostic captures the parse failure location. + const diagnostics = skillsManager.getSkillDiagnostics() + expect(diagnostics).toHaveLength(1) + expect(diagnostics[0].path).toBe(badSkillMd) + expect(diagnostics[0].source).toBe("global") + expect(typeof diagnostics[0].message).toBe("string") + expect(diagnostics[0].line).toBe(3) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("should log a distinct YAML error for other malformed frontmatter (no quote hint)", async () => { + const badSkillDir = p(globalSkillsDir, "broken-skill") + const badSkillMd = p(badSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir + }) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir ? ["broken-skill"] : [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === badSkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === badSkillMd) + mockReadFile.mockImplementation(async (file: string) => { + if (file === badSkillMd) { + return `--- +name broken-skill +description: Missing colon makes this invalid YAML +--- + +# Broken skill` + } + throw new Error("File not found") + }) + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + + const logged = consoleErrorSpy.mock.calls.map((call) => call.join(" ")) + expect(logged.some((line) => line.includes("YAMLException"))).toBe(true) + expect(skillsManager.getSkillDiagnostics()).toHaveLength(1) + // The unescaped-quotes hint only applies when the description line + // actually contains double quotes. + expect(logged.some((line) => line.includes("unescaped double quotes"))).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("should skip skills with unterminated frontmatter and a dangling quote (no line hint)", async () => { + const badSkillDir = p(globalSkillsDir, "unclosed-skill") + const badSkillMd = p(badSkillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir + }) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => { + return dir === globalSkillsDir ? ["unclosed-skill"] : [] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === badSkillDir) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === badSkillMd) + mockReadFile.mockImplementation(async (file: string) => { + if (file === badSkillMd) { + // Unterminated frontmatter: the opening --- is never closed, and the + // dangling double quote makes gray-matter throw. The raw-line lookup + // cannot find a closing delimiter, so no quote hint is possible. + return `--- +name: unclosed-skill +description: "unclosed quote +rest of the file` + } + throw new Error("File not found") + }) + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await skillsManager.discoverSkills() + + expect(skillsManager.getAllSkills()).toHaveLength(0) + + const logged = consoleErrorSpy.mock.calls.map((call) => call.join(" ")) + expect(logged.some((line) => line.includes("YAMLException"))).toBe(true) + expect(skillsManager.getSkillDiagnostics()).toHaveLength(1) + // Without a closing frontmatter delimiter there is no line to hint at. + expect(logged.some((line) => line.includes("unescaped double quotes"))).toBe(false) + } finally { + consoleErrorSpy.mockRestore() + } + }) + it("should skip skills with invalid name formats (spec compliance)", async () => { const invalidNames = [ "PDF-processing", // uppercase @@ -1233,6 +1404,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) @@ -1246,6 +1458,120 @@ Instructions`) expect(createdPath).toBe(p(PROJECT_DIR, ".roo", "skills", "project-skill", "SKILL.md")) }) + it("should escape double quotes in the description so the created skill loads (issue #859)", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockResolvedValue(undefined) + + const description = 'Use when implementing features. Triggers on: "TDD", "test-driven development"' + const createdPath = await skillsManager.createSkill("test-skill", "global", description) + + // The written frontmatter must be valid YAML that round-trips the + // description, with the raw value quoted instead of unescaped. + const writtenContent = String(mockWriteFile.mock.calls[0][1]) + const parsed = matter(writtenContent) + expect(parsed.data.name).toBe("test-skill") + expect(parsed.data.description).toBe(description) + expect(writtenContent).toContain(`description: '${description}'`) + + // Re-discovery loads the created skill instead of silently skipping it. + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["test-skill"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === p(globalSkillsDir, "test-skill")) { + return { isDirectory: () => true } + } + throw new Error("Not found") + }) + mockFileExists.mockImplementation(async (file: string) => file === createdPath) + mockReadFile.mockImplementation(async (file: string) => + file === createdPath ? writtenContent : Promise.reject(new Error("File not found")), + ) + + await skillsManager.discoverSkills() + + const skills = skillsManager.getAllSkills() + expect(skills).toHaveLength(1) + expect(skills[0].name).toBe("test-skill") + expect(skills[0].description).toBe(description) + }) + + it("should quote descriptions that would otherwise parse as YAML non-strings", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockResolvedValue(undefined) + + await skillsManager.createSkill("flag-skill", "global", "yes") + + const writtenContent = String(mockWriteFile.mock.calls[0][1]) + const parsed = matter(writtenContent) + // Without quoting, js-yaml resolves "yes" to a boolean and the skill + // is rejected with a misleading "missing required 'description' field". + expect(typeof parsed.data.description).toBe("string") + expect(parsed.data.description).toBe("yes") + }) + + it("should rewrite modeSlugs via gray-matter serialization without reflowing the description", async () => { + const skillDir = p(globalSkillsDir, "modes-skill") + const skillMd = p(skillDir, "SKILL.md") + const description = 'Use when implementing features. Triggers on: "TDD", "test-driven development"' + const originalContent = `--- +name: modes-skill +description: '${description}' +--- + +# Modes Skill + +## Instructions + +Body here.` + + // Initial discovery so the skill is registered. + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["modes-skill"] : [])) + 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(originalContent) + mockWriteFile.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + expect(skillsManager.getAllSkills()).toHaveLength(1) + + // Apply a mode restriction; the file is rewritten with the new slugs. + await skillsManager.updateSkillModes("modes-skill", "global", ["code"]) + + expect(mockWriteFile).toHaveBeenCalledTimes(1) + const rewritten = String(mockWriteFile.mock.calls[0][1]) + const parsed = matter(rewritten) + // The description survives the round-trip exactly and stays quoted. + expect(parsed.data.description).toBe(description) + expect(rewritten).toContain(`description: '${description}'`) + // The new modeSlugs were written and the body is preserved. + expect(parsed.data.modeSlugs).toEqual(["code"]) + expect(rewritten).toContain("Body here.") + // lineWidth: -1 keeps the value on a single line (no folded block scalar). + expect(rewritten).not.toContain("> ") + + // Clearing the restriction removes modeSlugs from the frontmatter. + await skillsManager.updateSkillModes("modes-skill", "global", []) + + const rewritten2 = String(mockWriteFile.mock.calls[1][1]) + const parsed2 = matter(rewritten2) + expect(parsed2.data.modeSlugs).toBeUndefined() + expect(parsed2.data.description).toBe(description) + }) + it("should throw error for invalid skill name", async () => { await expect(skillsManager.createSkill("Invalid-Name", "global", "Description")).rejects.toThrow( "Skill name must be lowercase letters/numbers/hyphens only", @@ -1299,6 +1625,197 @@ 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), + }), + ]) + }) + + it("serializes overlapping scans so an older scan cannot append stale diagnostics", async () => { + const skillDir = p(globalSkillsDir, "flaky-skill") + const skillPath = p(skillDir, "SKILL.md") + const staleContent = `--- +name: flaky-skill +description: "Broken "quote" frontmatter +--- + +Body.` + const goodContent = `--- +name: flaky-skill +description: Healthy after repair +--- + +Body.` + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["flaky-skill"] : [])) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockImplementation(async (file: string) => file === skillPath) + + // The first scan's read is delayed (as when a watcher fires while the + // file is still being written) and observes malformed content. A second + // scan started before that read resolves must run afterwards, and its + // result must win. + let resolveFirstRead!: (content: string) => void + const firstRead = new Promise((resolve) => { + resolveFirstRead = resolve + }) + let reads = 0 + mockReadFile.mockImplementation(async () => { + reads += 1 + return reads === 1 ? firstRead : goodContent + }) + + const first = skillsManager.discoverSkills() + const second = skillsManager.discoverSkills() + resolveFirstRead(staleContent) + await Promise.all([first, second]) + + expect(skillsManager.getSkillsMetadata()).toEqual([ + expect.objectContaining({ name: "flaky-skill", description: "Healthy after repair" }), + ]) + expect(skillsManager.getSkillDiagnostics()).toEqual([]) + }) + + it("does not hint at unescaped quotes when the parse error is on another line", async () => { + const skillDir = p(globalSkillsDir, "hint-skill") + const skillPath = p(skillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["hint-skill"] : [])) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockImplementation(async (file: string) => file === skillPath) + // The description is a valid single-quoted value that itself contains + // double quotes; the YAML error is on the unclosed name line instead, + // so the unescaped-quotes hint must not fire. + mockReadFile.mockResolvedValue(`--- +name: "unclosed +description: 'Triggers on "TDD" - valid quotes' +--- + +Body.`) + + const consoleErrorSpy = vi.spyOn(console, "error").mockImplementation(() => {}) + + try { + await skillsManager.discoverSkills() + + const logged = consoleErrorSpy.mock.calls.map((call) => call.join(" ")) + expect(logged.some((line) => line.includes("unescaped double quotes"))).toBe(false) + expect(skillsManager.getSkillDiagnostics()).toEqual([ + expect.objectContaining({ path: skillPath, source: "global", line: 4 }), + ]) + } finally { + consoleErrorSpy.mockRestore() + } + }) + + it("keeps reporting the same malformed skill on a re-scan (gray-matter cache must not swallow the error)", async () => { + const skillDir = p(globalSkillsDir, "cached-bad-skill") + const skillPath = p(skillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["cached-bad-skill"] : [])) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockImplementation(async (file: string) => file === skillPath) + mockReadFile.mockResolvedValue(`--- +name: cached-bad-skill +description: "Broken "quote" description +--- + +Body.`) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkillsMetadata()).toEqual([]) + expect(skillsManager.getSkillDiagnostics()).toHaveLength(1) + + // A re-scan of the identical (unchanged) content must keep reporting the + // parse failure. gray-matter keeps a global content-keyed cache that it + // populates before parsing, so without bypassing it the first throw is + // cached with an empty data object and every later parse of the same + // content silently returns that object instead of re-throwing - which + // would resurface the misleading "missing required 'name' field" symptom + // from issue #859. + await skillsManager.discoverSkills() + expect(skillsManager.getSkillsMetadata()).toEqual([]) + expect(skillsManager.getSkillDiagnostics()).toHaveLength(1) + expect(skillsManager.getSkillDiagnostics()[0]).toEqual( + expect.objectContaining({ path: skillPath, source: "global" }), + ) + }) + + it("records a diagnostic from a non-Error parse failure without location details", async () => { + const skillDir = p(globalSkillsDir, "raw-error-skill") + const skillPath = p(skillDir, "SKILL.md") + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["raw-error-skill"] : [])) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockImplementation(async (file: string) => file === skillPath) + mockReadFile.mockResolvedValue(`--- +name: raw-error-skill +description: Healthy +--- + +Body.`) + // Script a parse failure that is a plain string (no YAML reason, no + // mark, not an Error) so every defensive fallback in recordDiagnostic + // is exercised. + vi.mocked(matter).mockImplementationOnce(() => { + throw "gray-matter exploded" + }) + + await skillsManager.discoverSkills() + + expect(skillsManager.getSkillsMetadata()).toEqual([]) + expect(skillsManager.getSkillDiagnostics()).toEqual([ + { + path: skillPath, + source: "global", + message: "gray-matter exploded", + line: undefined, + column: undefined, + }, + ]) + }) + }) + 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..9c250e8514 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,8 +35,9 @@ import { CreateSkillDialog } from "./CreateSkillDialog" export const SkillsSettings: React.FC = () => { const { t } = useAppTranslation() - const { cwd, skills: rawSkills, customModes } = useExtensionState() + const { cwd, skills: rawSkills, skillDiagnostics: rawSkillDiagnostics, customModes } = useExtensionState() const skills = useMemo(() => rawSkills ?? [], [rawSkills]) + const skillDiagnostics = useMemo(() => rawSkillDiagnostics ?? [], [rawSkillDiagnostics]) const [deleteDialogOpen, setDeleteDialogOpen] = useState(false) const [skillToDelete, setSkillToDelete] = useState(null) @@ -232,6 +233,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 */}