fix(skills): serialize discovery scans, scope the quote hint, and parse frontmatter deterministically

Addresses the CodeRabbit review findings and completes the patch coverage:

- Serialize discoverSkills() runs through a promise chain so overlapping
  watcher-triggered scans never interleave; an older scan can no longer
  append a stale diagnostic after a newer scan has observed the repaired
  file.
- Only emit the unescaped-double-quotes hint when the parser error is
  located on the description line itself, so a valid quoted description
  plus an unrelated YAML error elsewhere no longer produces a misleading
  hint.
- Parse SKILL.md frontmatter with explicit empty options so gray-matter's
  global content-keyed cache is bypassed. The cache is populated 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 object instead of re-throwing - which resurfaces the
  misleading "missing required 'name' field" symptom from issue #859.

Tests:

- SkillsManager.spec: regression test that a delayed older scan cannot
  append stale diagnostics (serialization), a regression test that a
  re-scan of unchanged malformed content keeps reporting the parse
  failure (gray-matter cache poisoning), a no-false-hint case with a
  valid quoted description and an error on another line, and a non-Error
  parse failure exercising recordDiagnostic's defensive fallbacks
  (gray-matter is now vi.mocked with the real parser as the default
  implementation).
- ExtensionStateContext.spec: the skills message test now asserts the
  transition that clears stored skills/diagnostics, including a message
  that omits skills entirely.
- skills-diagnostics e2e: the malformed fixture is now a double-quoted
  description with unescaped inner quotes (the exact #859 failure mode),
  skill files are written atomically (sidecar + rename) so the watcher
  only observes complete files, and teardown removes only the skill
  directories the suite created.
- api-get-skills-state.spec: document why the partial test doubles need
  as-unknown-as casts.
- skillsMessageHandler.spec: cover the omitted newSkillModeSlugs case
  (passes undefined, still refreshes the posted state).

Author: easonliang28

apps/vscode-e2e/src/suite/skills-diagnostics.test.ts

@@ -10,11 +10,12 @@ import { waitFor } from "./utils"
 const GOOD_SKILL = "e2e-skill-good"
 const BAD_SKILL = "e2e-skill-bad"
 
-// Issue #859 reproduction content: unescaped double quotes in the description
-// make the YAML frontmatter unparseable.
+// 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"
+description: "Use when implementing features. Triggers on: "TDD", "test-driven development"
 ---
 
 # E2E Skill Bad
@@ -42,6 +43,17 @@ description: A healthy skill used by the skill diagnostics e2e smoke test.
 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<void> => {
+	const tmpPath = `${finalPath}.tmp`
+	await fs.writeFile(tmpPath, content, "utf8")
+	await fs.rename(tmpPath, finalPath)
+}
+
 suite("Roo Code Skill Diagnostics", function () {
 	setDefaultSuiteTimeout(this)
 
@@ -54,18 +66,24 @@ suite("Roo Code Skill Diagnostics", function () {
 	})
 
 	teardown(async function () {
-		await fs.rm(skillsRoot, { recursive: true, force: true })
+		// 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.
+		// 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 fs.writeFile(path.join(skillsRoot, GOOD_SKILL, "SKILL.md"), GOOD_SKILL_MD, "utf8")
+		await writeSkillFileAtomic(path.join(skillsRoot, GOOD_SKILL, "SKILL.md"), GOOD_SKILL_MD)
 		await fs.mkdir(path.join(skillsRoot, BAD_SKILL), { recursive: true })
-		await fs.writeFile(path.join(skillsRoot, BAD_SKILL, "SKILL.md"), MALFORMED_SKILL_MD, "utf8")
+		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
@@ -96,9 +114,9 @@ suite("Roo Code Skill Diagnostics", function () {
 			"malformed skill should be omitted from skills",
 		)
 
-		// Act: repair the frontmatter in place; the watcher re-discovers and the
-		// diagnostic clears.
-		await fs.writeFile(path.join(skillsRoot, BAD_SKILL, "SKILL.md"), FIXED_SKILL_MD, "utf8")
+		// Act: repair the frontmatter in place (atomically); the watcher
+		// re-discovers and the diagnostic clears.
+		await writeSkillFileAtomic(badSkillMd, FIXED_SKILL_MD)
 
 		await waitFor(
 			async () => {

src/core/webview/__tests__/skillsMessageHandler.spec.ts

@@ -426,6 +426,28 @@ describe("skillsMessageHandler", () => {
 			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]])
+
+			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: [],
+			})
+		})
+
 		it("returns undefined when required fields are missing", async () => {
 			const provider = createMockProvider(true)
 

src/extension/__tests__/api-get-skills-state.spec.ts

@@ -14,6 +14,11 @@ describe("API#getSkillsState", () => {
 	let mockGetSkillsManager: ReturnType<typeof vi.fn>
 
 	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

src/services/skills/SkillsManager.ts

@@ -20,17 +20,24 @@ export type { SkillMetadata, SkillContent, SkillDiagnostic }
 
 /**
  * Extract the raw top-level `key: ...` line from the frontmatter block of a
- * SKILL.md 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.
+ * 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 getRawFrontmatterLine(fileContent: string, key: string): string | undefined {
+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*:`)
-	return match[1].split(/\r?\n/).find((line) => linePattern.test(line))
+	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`
@@ -51,6 +58,7 @@ export class SkillsManager {
 	private providerRef: WeakRef<ClineProvider>
 	private disposables: vscode.Disposable[] = []
 	private isDisposed = false
+	private discoveryChain: Promise<void> = Promise.resolve()
 
 	constructor(provider: ClineProvider) {
 		this.providerRef = new WeakRef(provider)
@@ -67,8 +75,19 @@ 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<void> {
+	discoverSkills(): Promise<void> {
+		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<void> {
 		this.skills.clear()
 		this.diagnostics = []
 		const skillsDirs = await this.getSkillsDirectories()
@@ -135,15 +154,30 @@ export class SkillsManager {
 			// 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)
+				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 - point the user at the exact line.
-				const descriptionLine = getRawFrontmatterLine(fileContent, "description")
-				if (descriptionLine?.includes('"')) {
+				// 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.",

src/services/skills/__tests__/SkillsManager.spec.ts

@@ -68,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: {
@@ -1648,6 +1662,158 @@ Body here.`
 				}),
 			])
 		})
+
+		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<string>((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", () => {