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 e64406fa9c..5e200cdc12 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" import type { WebviewThemeFixture } from "./vscode-extension-host.js" export type RooCodeAPIEvents = RooCodeEvents @@ -58,6 +59,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[] + } /** * Checks for an ordered user turn and assistant tool call in persisted API history. * @param taskId The ID of the task. 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 dc407325d9..f07d2dd305 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" @@ -200,6 +200,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, makeClineProviderFactory(), 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 3a33510346..b12a03b381 100644 --- a/src/extension/api.ts +++ b/src/extension/api.ts @@ -300,6 +300,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(), + } + } + /** Checks persisted turn ordering without exposing conversation contents to tests. */ public async hasTaskApiConversationHistorySequence( taskId: string, diff --git a/src/services/skills/SkillsManager.ts b/src/services/skills/SkillsManager.ts index 92ff4e0e09..418a13e42e 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,11 +16,45 @@ import { import { t } from "../../i18n" // Re-export for convenience -export type { SkillMetadata, SkillContent } +export type { SkillMetadata, SkillContent, SkillDiagnostic } // Direct children (new skill folders or container symlinks) and their SKILL.md files. const SKILLS_WATCH_PATTERN = "{*,*/SKILL.md}" +/** + * 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 } + /** Mutable state shared by every scan in a single discovery pass. */ interface DiscoveryPass { /** Skills found in this pass. Published to the manager only when the pass ends. */ @@ -35,6 +69,8 @@ interface DiscoveryPass { * unreadable for a moment. Everything else is replaced by the results of the pass. */ failedPaths: string[] + /** Frontmatter parse failures recorded during this pass. Published to the manager only when the pass ends. */ + diagnostics: SkillDiagnostic[] } function isNotFoundError(error: unknown): boolean { @@ -57,8 +93,13 @@ function isHiddenEntry(name: string): boolean { return name.startsWith(".") } +// Stale-sidecar sweep: a matching temp file younger than this may belong to a live write in +// another process, so only older sidecars are treated as abandoned by a previous host. +const SKILL_TEMP_SIDECAR_STALE_MS = 10 * 60 * 1000 + export class SkillsManager { private skills: Map = new Map() + private diagnostics: SkillDiagnostic[] = [] // SKILL.md paths of skills that come from a symlinked container. These are read-only for move/delete. private containerSkillPaths: Set = new Set() // Published symlinked containers: real path -> the root entry that links to it @@ -73,14 +114,79 @@ export class SkillsManager { // Runs discovery passes one at a time, so an older pass can't finish after a newer one private discoveryQueue: Promise = Promise.resolve() private isDisposed = false + // Atomic SKILL.md writes in flight, tracked so dispose() can remove temp sidecars if the + // extension host stops between the staged write and the rename. + private activeTempFiles: Set = new Set() + // Per-directory serialization for SKILL.md sidecar cleanup and writes. The queue is static so + // every SkillsManager instance in this process shares it: a sweep cannot unlink a sidecar that + // another instance is still writing, and writes cannot interleave their validate/rename steps. + private static directoryQueues: Map> = new Map() constructor(provider: ClineProvider) { this.providerRef = new WeakRef(provider) } - async initialize(): Promise { - await this.discoverSkills() - await this.setupFileWatchers() + private initialization: Promise | null = null + + initialize(): Promise { + if (this.initialization) return this.initialization + this.initialization = (async () => { + // A previous extension-host stop can leave a staged SKILL.md temp sidecar behind; the + // in-memory tracking cannot survive that, so sweep the known skills directories once. + try { + await this.sweepStaleSkillTempFiles() + } catch { + // Stale-cleanup is best effort; discovery and watcher setup still run. + } + // Disposal can land during the sweep: stop before touching discovery or watchers. + if (this.isDisposed) return + await this.discoverSkills() + if (this.isDisposed) return + await this.setupFileWatchers() + })() + return this.initialization + } + + /** + * Remove SKILL.md temp sidecars left by a previous host: the name pattern is the one + * writeSkillFileAtomically generates, and only the skill directories under the known skills + * roots are scanned. + */ + private async sweepStaleSkillTempFiles(): Promise { + const tempPattern = /^\.SKILL\.md\.\d+\.[a-z0-9]+\.tmp$/ + const roots = (await this.getSkillsDirectories()).map((entry) => entry.dir) + for (const root of roots) { + let entries: unknown[] + try { + entries = await fs.readdir(root, { withFileTypes: true }) + } catch { + continue + } + const directories = [root] + for (const entry of entries) { + const dirent = entry as { name?: string; isDirectory?: () => boolean } + if (!dirent?.isDirectory?.()) continue + directories.push(path.join(root, String(dirent.name))) + } + for (const directory of directories) { + await this.withDirectoryLock(directory, async () => { + let files: string[] + try { + files = await fs.readdir(directory) + } catch { + return + } + for (const file of files) { + if (!tempPattern.test(file)) continue + const stalePath = path.join(directory, file) + const stats = await fs.stat(stalePath).catch(() => undefined) + // Leave fresh sidecars alone: they may belong to a write in another process. + if (!stats || Date.now() - stats.mtimeMs < SKILL_TEMP_SIDECAR_STALE_MS) continue + await fs.unlink(stalePath).catch(() => {}) + } + }) + } + } } /** @@ -108,6 +214,7 @@ export class SkillsManager { containerSkillPaths: new Set(), containers: new Map(), failedPaths: [], + diagnostics: [], } const skillsDirs = await this.getSkillsDirectories() @@ -148,6 +255,7 @@ export class SkillsManager { this.skills = pass.skills this.containerSkillPaths = pass.containerSkillPaths this.containers = pass.containers + this.diagnostics = pass.diagnostics } /** @@ -315,8 +423,44 @@ export class SkillsManager { } try { - // Use gray-matter to parse frontmatter - const { data: frontmatter } = matter(fileContent) + // Parse the frontmatter. 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. + let parsed: ReturnType + try { + parsed = matter(fileContent, {}) + } catch (error) { + this.recordDiagnostic(skillMdPath, source, error, pass.diagnostics) + 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 descriptionLine = getFrontmatterLine(fileContent, "description") + const errorMark = (error as { mark?: { line?: unknown } }).mark + if ( + descriptionLine?.line.includes('"') && + typeof errorMark?.line === "number" && + errorMark.line + 1 === descriptionLine.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 undefined + } + + const { data: frontmatter } = parsed // Validate required fields (only name and description for now) if (!frontmatter.name || typeof frontmatter.name !== "string") { @@ -391,6 +535,29 @@ export class SkillsManager { } } + /** + * Record a frontmatter parse failure so the user learns which SKILL.md is broken + * and why, with the parser's own reason and location when available (see issue #859). + */ + private recordDiagnostic( + path: string, + source: "global" | "project", + error: unknown, + diagnostics: SkillDiagnostic[], + ): 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 + + diagnostics.push({ path, source, message: reason ?? errorMessage, line, column }) + } + /** * Get skills available for the current mode. * Resolves overrides: project > global, mode-specific > generic. @@ -502,6 +669,10 @@ export class SkillsManager { * Get all skills metadata (for UI display) * Returns skills from all sources without content */ + getSkillDiagnostics(): SkillDiagnostic[] { + return [...this.diagnostics] + } + getSkillsMetadata(): SkillMetadata[] { return this.getAllSkills() } @@ -561,6 +732,77 @@ export class SkillsManager { * @param modeSlugs - Optional mode restrictions (undefined/empty = any mode) * @returns Path to created SKILL.md file */ + /** + * Write one SKILL.md atomically: stage the content in a same-directory temp file, + * await the complete write, then rename it over the target. A crash or failed write + * must not leave a truncated SKILL.md that the loader would silently misparse. + * The temp file is removed when the write or rename fails. + */ + private async withDirectoryLock(directory: string, operation: () => Promise): Promise { + // Normalize through realpath first so a symlinked alias shares the same queue key; fall + // back to the resolved path when the directory cannot be resolved (e.g. not created yet). + const key = await fs.realpath(directory).catch(() => path.resolve(directory)) + const previous = SkillsManager.directoryQueues.get(key) ?? Promise.resolve() + const run = previous.then(async () => { + // A manager disposed while this operation waited must not run at all. + if (this.isDisposed) throw new Error("SkillsManager is disposed") + return operation() + }) + const tracked = run.then( + () => undefined, + () => undefined, + ) + SkillsManager.directoryQueues.set(key, tracked) + try { + return await run + } finally { + if (SkillsManager.directoryQueues.get(key) === tracked) { + SkillsManager.directoryQueues.delete(key) + } + } + } + + private async writeSkillFileAtomically( + filePath: string, + content: string, + validateTarget?: (targetPath: string) => Promise, + ): Promise { + const directory = path.dirname(filePath) + await this.withDirectoryLock(directory, async () => { + const temporaryPath = path.join( + directory, + `.${path.basename(filePath)}.${Date.now()}.${Math.random().toString(36).slice(2)}.tmp`, + ) + this.activeTempFiles.add(temporaryPath) + // Only a write that actually created the sidecar through exclusive creation may remove + // it: when wx fails because the path already exists, that file belongs to someone else. + let ownsTemporaryPath = false + try { + if (this.isDisposed) throw new Error("SkillsManager is disposed") + // Exclusive creation: a pre-created symlink at the generated name cannot be followed. + await fs.writeFile(temporaryPath, content, { encoding: "utf-8", flag: "wx" }) + ownsTemporaryPath = true + if (this.isDisposed) throw new Error("SkillsManager is disposed") + // Re-validate the final target immediately before the rename: the target (or an + // ancestor) can change between the caller's resolution and this publish. + await validateTarget?.(filePath) + if (this.isDisposed) throw new Error("SkillsManager is disposed") + await fs.rename(temporaryPath, filePath) + } catch (error) { + // Best effort: a partial temp file must not be left behind, but an EEXIST failure + // means exclusive creation never acquired the path and the existing file belongs to + // someone else. Any other failure may have created a partial sidecar and is cleaned + // up; the original failure is the one the caller needs to see. + if (ownsTemporaryPath || (error as { code?: string })?.code !== "EEXIST") { + await fs.unlink(temporaryPath).catch(() => {}) + } + throw error + } finally { + this.activeTempFiles.delete(temporaryPath) + } + }) + } + async createSkill( name: string, source: "global" | "project", @@ -601,8 +843,9 @@ export class SkillsManager { throw new Error(t("skills:errors.already_exists", { name, path: skillMdPath })) } - // Create the skill directory - await fs.mkdir(skillDir, { recursive: true }) + // Create the skill directory. mkdir reports the path it created (when any), so a + // failed persistence can remove exactly the directory this call introduced. + const createdDir = await fs.mkdir(skillDir, { recursive: true }) // Generate SKILL.md content with frontmatter const titleName = name @@ -610,28 +853,38 @@ 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}`) - } + // 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 skillContent = `--- -${frontmatterLines.join("\n")} ---- - + const body = ` # ${titleName} ## Instructions Add your skill instructions here. ` - - // Write the SKILL.md file - await fs.writeFile(skillMdPath, skillContent, "utf-8") + const skillContent = matter.stringify(body, frontmatter, SKILL_YAML_DUMP_OPTIONS) + // Publish the SKILL.md atomically, and leave no half-created skill directory behind + // when persistence fails. mkdir(recursive) can report the first ancestor it created + // (.roo or skills), so a recursive removal of that value could delete unrelated + // skills: only this call's own skill directory is removed, and only while it is + // empty. Any created ancestor stays behind like the empty directory it is. + try { + await this.writeSkillFileAtomically(skillMdPath, skillContent) + } catch (error) { + if (createdDir) { + await fs.rmdir(skillDir).catch(() => {}) + } + throw error + } // Refresh skills list await this.discoverSkills() @@ -927,6 +1180,26 @@ Add your skill instructions here. * @param source - Where the skill is located ("global" or "project") * @param newModeSlugs - New mode slugs (undefined/empty = any mode) */ + /** + * The resolved SKILL.md target must stay inside one of the configured skills roots for the + * source: a direct skill-directory symlink can point anywhere, and publishing through it + * would write outside the user selected skills folder. + */ + private async assertTargetWithinSkillsRoots(targetPath: string, source: "global" | "project"): Promise { + const roots = (await this.getSkillsDirectories()) + .filter((entry) => entry.source === source) + .map((entry) => entry.dir) + const inside = async (root: string): Promise => { + const resolvedRoot = await fs.realpath(root).catch(() => root) + const relative = path.relative(resolvedRoot, targetPath) + return relative.length > 0 && !relative.startsWith("..") && !path.isAbsolute(relative) + } + for (const root of roots) { + if (await inside(root)) return + } + throw new Error(`Refusing to update SKILL.md outside the ${source} skills directories: ${targetPath}`) + } + async updateSkillModes(name: string, source: "global" | "project", newModeSlugs?: string[]): Promise { // Find any skill with this name and source (regardless of current mode) let skill: SkillMetadata | undefined @@ -958,9 +1231,22 @@ Add your skill instructions here. delete frontmatter.mode } - // Serialize back to SKILL.md format - const newContent = matter.stringify(body, frontmatter) - await fs.writeFile(skill.path, newContent, "utf-8") + // 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) + // Resolve the link target first: publishing through the temp+rename must update the file + // the skill was discovered through, not replace a symlinked SKILL.md with a regular file. + // A resolution failure propagates: publishing through the unresolved path could replace a + // symlink with a regular file (or write somewhere other than the target the caller saw). + const targetPath = await fs.realpath(skill.path) + await this.assertTargetWithinSkillsRoots(targetPath, source) + await this.writeSkillFileAtomically(targetPath, newContent, async (target) => { + // Fail closed: a resolution failure must propagate so the staged temp write is removed + // and the rename is skipped, rather than publishing through a stale path. + const freshTarget = await fs.realpath(target) + await this.assertTargetWithinSkillsRoots(freshTarget, source) + }) // Refresh skills list await this.discoverSkills() @@ -1051,6 +1337,7 @@ Add your skill instructions here. } private async setupFileWatchers(): Promise { + if (this.isDisposed) return // Skip if test environment is detected or VSCode APIs are not available if (process.env.NODE_ENV === "test" || !vscode.workspace.createFileSystemWatcher) { return @@ -1090,6 +1377,7 @@ Add your skill instructions here. } private watchDirectory(dirPath: string): void { + if (this.isDisposed) return const watcher = this.createSkillsWatcher(dirPath) if (watcher) { this.disposables.push(watcher) @@ -1141,6 +1429,11 @@ Add your skill instructions here. async dispose(): Promise { this.isDisposed = true + // Any sidecar still tracked is a write that never reached its rename. + for (const temporaryPath of this.activeTempFiles) { + await fs.unlink(temporaryPath).catch(() => {}) + } + this.activeTempFiles.clear() this.disposables.forEach((d) => d.dispose()) this.disposables = [] this.containerWatchers.forEach((w) => w.dispose()) diff --git a/src/services/skills/__tests__/SkillsManager.spec.ts b/src/services/skills/__tests__/SkillsManager.spec.ts index 1d6727605e..15f5d3db47 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 { @@ -94,6 +95,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: { @@ -421,6 +436,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 @@ -2065,7 +2236,12 @@ Instructions`) mockMkdir.mockResolvedValue(undefined) mockWriteFile.mockResolvedValue(undefined) - const createdPath = await skillsManager.createSkill("new-skill", "global", "A new skill description") + // Longer than js-yaml's default 80-column fold width: createSkill must + // write it as a single description line, not a folded scalar. + const description = + 'Use when implementing features. Triggers on: "TDD", "test-driven development" and "refactoring" workflows' + + const createdPath = await skillsManager.createSkill("new-skill", "global", description) expect(createdPath).toBe(p(GLOBAL_ROO_DIR, "skills", "new-skill", "SKILL.md")) expect(mockMkdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "new-skill"), { recursive: true }) @@ -2073,9 +2249,14 @@ Instructions`) // Verify the content written const writeCall = mockWriteFile.mock.calls[0] - expect(writeCall[0]).toBe(p(GLOBAL_ROO_DIR, "skills", "new-skill", "SKILL.md")) - expect(writeCall[1]).toContain("name: new-skill") - expect(writeCall[1]).toContain("description: A new skill description") + expect(String(writeCall[0])).toMatch(/[/\\]new-skill[/\\]\.SKILL\.md\.[^/\\]+\.tmp$/) + expect(mockRename).toHaveBeenCalledWith(writeCall[0], p(GLOBAL_ROO_DIR, "skills", "new-skill", "SKILL.md")) + const written = String(writeCall[1]) + expect(written).toContain("name: new-skill") + const descriptionLine = written.split("\n").find((line) => line.startsWith("description:")) + // Long descriptions are quoted (js-yaml quotes strings containing quotes). + expect(descriptionLine).toBe(`description: '${description}'`) + expect(written).not.toContain("> ") }) it("should create a mode-specific skill with modeSlugs array", async () => { @@ -2097,6 +2278,163 @@ 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("cleans the temp file and the newly created skill directory when persistence fails", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + const createdDir = p(GLOBAL_ROO_DIR, "skills", "failing-skill") + mockMkdir.mockResolvedValue(createdDir) + mockWriteFile.mockRejectedValueOnce(Object.assign(new Error("ENOSPC"), { code: "ENOSPC" })) + mockUnlink.mockResolvedValue(undefined) + mockRm.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + + await expect(skillsManager.createSkill("failing-skill", "global", "Description")).rejects.toThrow("ENOSPC") + + // The partially written temp file is removed and the directory this call created is + // removed only while empty (rmdir, never a recursive rm that could take ancestors + // reported by mkdir(recursive) with unrelated skills); the original persistence + // error is what the caller sees. + expect(mockUnlink).toHaveBeenCalledWith(expect.stringMatching(/\.SKILL\.md\.[^/\\]+\.tmp$/)) + expect(mockRmdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "failing-skill")) + expect(mockRm).not.toHaveBeenCalled() + }) + + it("leaves a non-empty skill directory alone when the empty-only removal fails", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills", "busy-skill")) + mockWriteFile.mockRejectedValueOnce(Object.assign(new Error("ENOSPC"), { code: "ENOSPC" })) + mockUnlink.mockResolvedValue(undefined) + mockRmdir.mockRejectedValueOnce(Object.assign(new Error("ENOTEMPTY"), { code: "ENOTEMPTY" })) + + // A failed cleanup (the temp file survived, another skill appeared, ...) must not + // mask or replace the persistence failure. + await expect(skillsManager.createSkill("busy-skill", "global", "Description")).rejects.toThrow("ENOSPC") + expect(mockRmdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "busy-skill")) + expect(mockRm).not.toHaveBeenCalled() + }) + + it("never recursively removes an ancestor reported by recursive mkdir", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + // mkdir(recursive) reports the first ancestor it created; removing that value + // recursively could take the sibling skill directories under it with it. + mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills")) + mockWriteFile.mockRejectedValueOnce(Object.assign(new Error("ENOSPC"), { code: "ENOSPC" })) + mockUnlink.mockResolvedValue(undefined) + mockRm.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + + await expect(skillsManager.createSkill("isolated-skill", "global", "Description")).rejects.toThrow("ENOSPC") + + // Only this skill's directory is considered, and only for an empty-only removal. + expect(mockRmdir).toHaveBeenCalledWith(p(GLOBAL_ROO_DIR, "skills", "isolated-skill")) + expect(mockRm).not.toHaveBeenCalled() + }) + + it("preserves an existing file when exclusive staging fails because the name exists", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills", "exists")) + const existsError = Object.assign(new Error("EEXIST: file already exists"), { code: "EEXIST" }) + mockWriteFile.mockRejectedValueOnce(existsError) + mockUnlink.mockResolvedValue(undefined) + + await expect(skillsManager.createSkill("exists", "global", "Description")).rejects.toBe(existsError) + + // wx failed before this write owned the path: the existing file must not be unlinked. + expect(mockUnlink).not.toHaveBeenCalled() + expect(mockRename).not.toHaveBeenCalled() + expect(skillsManager["activeTempFiles"].size).toBe(0) + }) + + it("removes the temp file and rethrows the original error when the create rename fails", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + mockMkdir.mockResolvedValue(p(GLOBAL_ROO_DIR, "skills", "rename-fails")) + mockWriteFile.mockResolvedValue(undefined) + const renameError = Object.assign(new Error("EEXIST: rename failed"), { code: "EEXIST" }) + mockRename.mockRejectedValueOnce(renameError) + mockUnlink.mockResolvedValue(undefined) + mockRmdir.mockResolvedValue(undefined) + + await expect(skillsManager.createSkill("rename-fails", "global", "Description")).rejects.toBe(renameError) + + const temporaryPath = String(mockWriteFile.mock.calls[0][0]) + expect(temporaryPath).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/) + expect(mockUnlink).toHaveBeenCalledWith(temporaryPath) + expect(skillsManager["activeTempFiles"].size).toBe(0) + }) + + it("does not remove a pre-existing skill directory when persistence fails", async () => { + mockDirectoryExists.mockResolvedValue(false) + mockRealpath.mockImplementation(async (p: string) => p) + mockReaddir.mockResolvedValue([]) + mockFileExists.mockResolvedValue(false) + // mkdir resolves undefined when the directory already existed: it is not ours to remove. + mockMkdir.mockResolvedValue(undefined) + mockWriteFile.mockRejectedValueOnce(new Error("EACCES")) + mockUnlink.mockResolvedValue(undefined) + mockRm.mockResolvedValue(undefined) + + await expect(skillsManager.createSkill("owned-skill", "global", "Description")).rejects.toThrow("EACCES") + + expect(mockRm).not.toHaveBeenCalled() + expect(mockRmdir).not.toHaveBeenCalled() + expect(mockUnlink).toHaveBeenCalledWith(expect.stringMatching(/\.SKILL\.md\.[^/\\]+\.tmp$/)) + }) + it("should create a project skill", async () => { mockDirectoryExists.mockResolvedValue(false) mockRealpath.mockImplementation(async (p: string) => p) @@ -2110,6 +2448,266 @@ 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) + + // Deliberately longer than js-yaml's default 80-column fold width: without + // lineWidth: -1 the serializer wraps this into a rolled scalar. + const description = + 'Use when implementing features. Triggers on: "TDD", "test-driven development", "red-green-refactor" and "refactoring" workflows' + 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]) + // The rewrite lands through the same atomic temp+rename path as createSkill. + expect(String(mockWriteFile.mock.calls[0][0])).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/) + expect(mockRename).toHaveBeenCalledWith(mockWriteFile.mock.calls[0][0], skillMd) + 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: without it js-yaml folds + // a description this long across several lines, so pin the exact line and + // the absence of any continuation. + const descriptionLine = rewritten.split("\n").find((line) => line.startsWith("description:")) + expect(descriptionLine).toBe(`description: '${description}'`) + 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("propagates a realpath failure instead of writing through the unresolved path", async () => { + const skillDir = p(globalSkillsDir, "unresolvable-skill") + const skillMd = p(skillDir, "SKILL.md") + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockReaddir.mockImplementation(async (dir: string) => + dir === globalSkillsDir ? ["unresolvable-skill"] : [], + ) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue("---\nname: unresolvable-skill\ndescription: A skill\n---\nInstructions") + + await skillsManager.discoverSkills() + // The discovered path resolves, but the publish-time resolution fails: the write must + // not fall back to the unresolved (potentially symlinked) path. + const resolutionError = Object.assign(new Error("ENOENT: no such file or directory"), { code: "ENOENT" }) + mockRealpath.mockRejectedValueOnce(resolutionError) + + await expect(skillsManager.updateSkillModes("unresolvable-skill", "global", ["code"])).rejects.toBe( + resolutionError, + ) + expect(mockWriteFile).not.toHaveBeenCalled() + expect(mockRename).not.toHaveBeenCalled() + }) + + it("removes the temp file and rethrows the original error when the update rename fails", async () => { + const skillDir = p(globalSkillsDir, "rename-update") + const skillMd = p(skillDir, "SKILL.md") + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["rename-update"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue("---\nname: rename-update\ndescription: A skill\n---\nInstructions") + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + mockWriteFile.mockResolvedValue(undefined) + const renameError = Object.assign(new Error("EIO: rename failed"), { code: "EIO" }) + mockRename.mockRejectedValueOnce(renameError) + mockUnlink.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + + await expect(skillsManager.updateSkillModes("rename-update", "global", ["code"])).rejects.toBe(renameError) + + const temporaryPath = String(mockWriteFile.mock.calls[0][0]) + expect(temporaryPath).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/) + expect(mockWriteFile.mock.calls[0][2]).toEqual(expect.objectContaining({ flag: "wx" })) + expect(mockUnlink).toHaveBeenCalledWith(temporaryPath) + // The existing SKILL.md was never replaced: the only write went to the temp sidecar and + // the failed rename never committed. + expect(mockWriteFile).toHaveBeenCalledTimes(1) + expect(mockRename).toHaveBeenCalledWith(temporaryPath, skillMd) + // The existing SKILL.md and its directory remain untouched. + expect(mockReadFile).toHaveBeenCalledWith(skillMd, "utf-8") + expect(mockWriteFile).not.toHaveBeenCalledWith(skillMd, expect.anything(), expect.anything()) + expect(mockRmdir).not.toHaveBeenCalled() + expect(mockRm).not.toHaveBeenCalled() + expect(skillsManager["activeTempFiles"].size).toBe(0) + }) + + it("fails closed when the pre-rename target resolution fails", async () => { + const skillDir = p(globalSkillsDir, "resolution-fails") + const skillMd = p(skillDir, "SKILL.md") + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["resolution-fails"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue("---\nname: resolution-fails\ndescription: A skill\n---\nInstructions") + // The first resolution succeeds; the immediate pre-rename resolution fails and must + // propagate instead of publishing through the stale path. + let resolutions = 0 + mockRealpath.mockImplementation(async (pathArg: string) => { + if (pathArg === skillMd) { + resolutions += 1 + if (resolutions === 1) { + return skillMd + } + throw Object.assign(new Error("EIO: resolution failed"), { code: "EIO" }) + } + return pathArg + }) + mockWriteFile.mockResolvedValue(undefined) + mockUnlink.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + + await expect(skillsManager.updateSkillModes("resolution-fails", "global", ["code"])).rejects.toThrow( + "EIO: resolution failed", + ) + const temporaryPath = String(mockWriteFile.mock.calls[0][0]) + expect(mockUnlink).toHaveBeenCalledWith(temporaryPath) + expect(mockRename).not.toHaveBeenCalled() + }) + + it("refuses to update a SKILL.md that resolves outside the skills roots", async () => { + const skillDir = p(globalSkillsDir, "escaping") + const skillMd = p(skillDir, "SKILL.md") + const outsidePath = p("/outside/root", "SKILL.md") + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? ["escaping"] : [])) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg === skillDir) { + return { isDirectory: () => true } + } + throw enoent() + }) + mockFileExists.mockImplementation(async (file: string) => file === skillMd) + mockReadFile.mockResolvedValue("---\nname: escaping\ndescription: A skill\n---\nInstructions") + // Discovery saw the root path, but the publish-time resolution points outside every + // configured skills root: a direct skill-dir symlink must not let the write escape. + mockRealpath.mockImplementation(async (pathArg: string) => (pathArg === skillMd ? outsidePath : pathArg)) + mockWriteFile.mockResolvedValue(undefined) + mockRename.mockResolvedValue(undefined) + + await skillsManager.discoverSkills() + + await expect(skillsManager.updateSkillModes("escaping", "global", ["code"])).rejects.toThrow( + /Refusing to update SKILL\.md outside the global skills directories/, + ) + expect(mockWriteFile).not.toHaveBeenCalled() + expect(mockRename).not.toHaveBeenCalled() + }) + 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", @@ -2189,7 +2787,248 @@ Instructions`) const created = await skillsManager.createSkill("my-skill", "global", "Description") expect(created).toBe(rooSkillMd) - expect(mockWriteFile).toHaveBeenCalledWith(rooSkillMd, expect.any(String), "utf-8") + const staged = mockWriteFile.mock.calls[0][0] + expect(String(staged)).toMatch(/\.SKILL\.md\.[^/\\]+\.tmp$/) + expect(mockRename).toHaveBeenCalledWith(staged, rooSkillMd) + }) + }) + + 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 expose a half-built snapshot to a reader while a scan is in progress", async () => { + const stablePath = p(globalSkillsDir, "stable-skill", "SKILL.md") + const slowPath = p(globalSkillsDir, "slow-skill", "SKILL.md") + const stableContent = `--- +name: stable-skill +description: Stable before the scan +--- + +Body.` + const slowContent = `--- +name: slow-skill +description: Loaded after the scan completes +--- + +Body.` + + mockDirectoryExists.mockImplementation(async (dir: string) => dir === globalSkillsDir) + mockRealpath.mockImplementation(async (pathArg: string) => pathArg) + // The second scan sees an extra skill, and that skill's read is parked, so + // the reader runs while the scan is mid-flight. Committing incrementally + // would let the reader see the scan after the clear but before the slow + // skill was loaded. + let entries = ["stable-skill"] + mockReaddir.mockImplementation(async (dir: string) => (dir === globalSkillsDir ? entries : [])) + mockStat.mockResolvedValue({ isDirectory: () => true }) + mockFileExists.mockImplementation(async (file: string) => file === stablePath || file === slowPath) + let resolveSlowRead!: (content: string) => void + const slowRead = new Promise((resolve) => { + resolveSlowRead = resolve + }) + mockReadFile.mockImplementation(async (file: string) => (file === slowPath ? slowRead : stableContent)) + + await skillsManager.discoverSkills() + expect(skillsManager.getSkillsMetadata()).toHaveLength(1) + + // Slow skill first: the reader runs after the scan started but before any of + // its own skills were added, so an incremental commit would show an empty list. + entries = ["slow-skill", "stable-skill"] + const scan = skillsManager.discoverSkills() + await vi.waitFor(() => expect(mockReadFile.mock.calls.map((c) => c[0])).toContain(slowPath)) + expect(skillsManager.getSkillsMetadata()).toHaveLength(1) + expect(skillsManager.getSkillDiagnostics()).toEqual([]) + + resolveSlowRead(slowContent) + await scan + + expect(skillsManager.getSkillsMetadata()).toHaveLength(2) + }) + 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, + }, + ]) }) }) @@ -3273,4 +4112,142 @@ Instructions`) }) }) }) + it("stops creating watchers once the manager is disposed", async () => { + const createWatcher = vi.mocked(vscode.workspace.createFileSystemWatcher) + const originalNodeEnv = process.env.NODE_ENV + process.env.NODE_ENV = "development" + try { + createWatcher.mockClear() + await skillsManager["setupFileWatchers"]() + expect(createWatcher).toHaveBeenCalled() + + createWatcher.mockClear() + await skillsManager.dispose() + await skillsManager["setupFileWatchers"]() + expect(createWatcher).not.toHaveBeenCalled() + } finally { + process.env.NODE_ENV = originalNodeEnv + } + }) + + it("sweeps only a staged sidecar left by a previous host", async () => { + const staleName = ".SKILL.md.123.abc.tmp" + const freshName = ".SKILL.md.456.def.tmp" + mockReaddir.mockImplementation(async (_dir: string, options?: { withFileTypes?: boolean }) => { + if (options?.withFileTypes) { + return [] + } + return [staleName, freshName] + }) + mockStat.mockImplementation(async (pathArg: string) => { + if (pathArg.endsWith(staleName)) { + // Older than the stale threshold: abandoned by a previous host. + return { mtimeMs: Date.now() - 60 * 60 * 1000 } + } + if (pathArg.endsWith(freshName)) { + return { mtimeMs: Date.now() } + } + throw enoent() + }) + mockUnlink.mockResolvedValue(undefined) + + await skillsManager["sweepStaleSkillTempFiles"]() + + expect(mockUnlink).toHaveBeenCalledWith(p(globalSkillsDir, staleName)) + // A fresh matching sidecar may belong to a live write and must survive. + expect(mockUnlink).not.toHaveBeenCalledWith(p(globalSkillsDir, freshName)) + }) + + it("shares one directory queue across symlink aliases", async () => { + const order: string[] = [] + let releaseFirst!: () => void + const firstGate = new Promise((resolve) => { + releaseFirst = resolve + }) + const alias = p("/alias", "skills") + mockRealpath.mockImplementation(async (pathArg: string) => (pathArg === alias ? globalSkillsDir : pathArg)) + + const first = skillsManager["withDirectoryLock"](alias, async () => { + order.push("first:start") + await firstGate + order.push("first:end") + }) + try { + // The lock key is resolved through realpath asynchronously before the operation runs. + await vi.waitFor(() => expect(order).toEqual(["first:start"])) + const second = skillsManager["withDirectoryLock"](globalSkillsDir, async () => { + order.push("second") + }) + await Promise.resolve() + // The alias resolves to the same real directory, so the second operation waits. + expect(order).toEqual(["first:start"]) + + releaseFirst() + await Promise.all([first, second]) + expect(order).toEqual(["first:start", "first:end", "second"]) + } finally { + // Never leave the shared queue holding a gate if an assertion above fails. + releaseFirst() + await first.catch(() => undefined) + } + }) + + it("does not start watchers when initialization runs after disposal", async () => { + const createWatcher = vi.mocked(vscode.workspace.createFileSystemWatcher) + await skillsManager.dispose() + createWatcher.mockClear() + + await skillsManager.initialize() + + expect(createWatcher).not.toHaveBeenCalled() + }) + + it("rejects a queued write once the manager is disposed", async () => { + await skillsManager.dispose() + mockWriteFile.mockResolvedValue(undefined) + mockUnlink.mockResolvedValue(undefined) + + await expect( + skillsManager["writeSkillFileAtomically"](p(globalSkillsDir, "SKILL.md"), "content"), + ).rejects.toThrow("SkillsManager is disposed") + expect(mockWriteFile).not.toHaveBeenCalled() + expect(mockRename).not.toHaveBeenCalled() + }) + + it("never runs a queued directory operation after the manager is disposed", async () => { + let releaseFirst!: () => void + const firstGate = new Promise((resolve) => { + releaseFirst = resolve + }) + let firstStarted = false + let secondRan = false + + const first = skillsManager["withDirectoryLock"](globalSkillsDir, async () => { + firstStarted = true + await firstGate + }) + await vi.waitFor(() => expect(firstStarted).toBe(true)) + + // The second operation is queued behind the first and must be rejected after disposal + // without running its callback. + const second = skillsManager["withDirectoryLock"](globalSkillsDir, async () => { + secondRan = true + }) + await skillsManager.dispose() + releaseFirst() + + await first + await expect(second).rejects.toThrow("SkillsManager is disposed") + expect(secondRan).toBe(false) + }) + + it("removes tracked temp sidecars on dispose", async () => { + mockUnlink.mockResolvedValue(undefined) + skillsManager["activeTempFiles"].add("/mock/skills/foo/.SKILL.md.123.abc.tmp") + + await skillsManager.dispose() + + expect(mockUnlink).toHaveBeenCalledWith("/mock/skills/foo/.SKILL.md.123.abc.tmp") + expect(skillsManager["activeTempFiles"].size).toBe(0) + }) }) 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/playwright/gallery/stories.tsx b/webview-ui/playwright/gallery/stories.tsx index 85d6919db4..c44f2e2c66 100644 --- a/webview-ui/playwright/gallery/stories.tsx +++ b/webview-ui/playwright/gallery/stories.tsx @@ -438,4 +438,45 @@ export const stories: Record = { ) }, + + "skills-settings-diagnostics": async () => { + const [{ AppProviders }, { SkillsSettings }] = await Promise.all([ + import("../AppProviders"), + import("@/components/settings/SkillsSettings"), + ]) + // Deterministic diagnostics: one project skill with a YAML location, one global + // skill without position data, so the alert's path/location/message rendering is + // fully covered by the screenshot. + const skills = [ + { + name: "diagnostics-demo", + description: "Demo skill that loads successfully next to the broken ones", + path: "/workspace/.roo/skills/diagnostics-demo/SKILL.md", + source: "project" as const, + }, + ] + const skillDiagnostics = [ + { + path: "/workspace/.roo/skills/broken-skill/SKILL.md", + source: "project" as const, + message: "Invalid YAML frontmatter: unexpected end of the stream", + line: 3, + column: 12, + }, + { + path: "/home/user/.roo/skills/legacy-mode/SKILL.md", + source: "global" as const, + message: "Missing required 'description' field", + }, + ] + return ( + +
+ +
+ + ) + }, } 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 */}