Skip to content

fix(skills): report YAML frontmatter parse errors and serialize SKILL.md safely - #1321

Open
easonLiangWorldedtech wants to merge 10 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:fix/skill-frontmatter-yaml-859
Open

fix(skills): report YAML frontmatter parse errors and serialize SKILL.md safely#1321
easonLiangWorldedtech wants to merge 10 commits into
Zoo-Code-Org:mainfrom
easonLiangWorldedtech:fix/skill-frontmatter-yaml-859

Conversation

@easonLiangWorldedtech

@easonLiangWorldedtech easonLiangWorldedtech commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #859

Stacked on #934 — that PR (also fixing #859) adds the structured SkillDiagnostic collection (SkillsManager.diagnostics / recordDiagnostic() / getSkillDiagnostics()) and the settings-page diagnostics panel. This PR keeps only the remaining gaps on top of it, so the diff stays minimal if #934 merges first and gets squashed.

Problem (remainder after #934)

  1. Misleading error still hides the common cause. fix(skills): safely serialize skill frontmatter #934's recordDiagnostic() reports the YAML exception, but the single most common trigger — unescaped double quotes in the description — still requires the user to read the raw YAML error and open the file. A targeted hint points at the exact line and the fix.
  2. Diagnostics can disappear on re-scan (gray-matter cache poisoning). gray-matter keeps a global content-keyed cache that it populates before parsing. 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 — so a re-scan (watcher event, manual refresh) resurfaces the misleading "missing required 'name' field" symptom and drops the diagnostic.
  3. Overlapping discovery scans can interleave. File-watcher events start concurrent discoverSkills() runs; an older scan finishing after a newer one can append a stale diagnostic for an already-repaired skill.
  4. Frontmatter serialization reflows on every mode change. updateSkillModes() rewrites the file with js-yaml's default lineWidth: 80, reflowing long plain descriptions into folded block scalars (>-1) on every mode toggle — noisy diffs and format churn. createSkill() had the same exposure.
  5. Settings crashes when skillDiagnostics is absent. fix(skills): safely serialize skill frontmatter #934 made the field optional on ExtensionState, but SkillsSettings read it without a fallback — any render without it (and 14 webview tests in the unit-test CI job) crashed with Cannot read properties of undefined (reading 'length').
  6. Missing translations. fix(skills): safely serialize skill frontmatter #934's new settings:skills.diagnostics.* i18n keys were only added to en, which fails the check-translations CI job (all 17 non-English locales).
  7. Test coverage. The new handleUpdateSkillModes postMessage path, the malformed-load branches, updateSkillModes' serialization, and the extension-host → watcher → webview diagnostics flow had no tests.

Changes

1. Deterministic frontmatter parsing + unescaped-quote hint (load path)src/services/skills/SkillsManager.ts

  • loadSkillMetadata() now parses with explicit empty options (matter(fileContent, {})), bypassing gray-matter's global content cache so a malformed skill reports the same parse error on every scan instead of the first throw being cached as an empty data object.
  • When the parse throws, a small module helper (getFrontmatterLine()) extracts the raw top-level description: line with its file line number; the "unescaped double quotes" console.error hint is only emitted when the parser error's mark is located on that exact line — so a valid quoted description plus an unrelated YAML error elsewhere does not produce a misleading hint.

2. Serialized discovery scanssrc/services/skills/SkillsManager.ts

  • discoverSkills() is a non-async serializer chaining each run onto a discoveryChain promise; the body moved to performDiscovery(). Overlapping watcher-triggered scans no longer interleave, so an older scan can no longer append a stale diagnostic after a newer scan has observed the repaired file.

3. Stable SKILL.md serialization (create/update paths)

  • Both createSkill() and updateSkillModes() now pass lineWidth: -1 (via a small typed alias, since gray-matter's bundled typings predate its js-yaml dump-options passthrough), keeping long plain values on a single line.
  • Output for plain descriptions stays byte-identical to the previous format; values with YAML special characters (double quotes, booleans like yes) are quoted/escaped automatically and always round-trip.

4. Settings crash fixwebview-ui/src/components/settings/SkillsSettings.tsx

  • skillDiagnostics falls back to [] (same pattern as the existing skills handling), so the component no longer crashes when the optional ExtensionState field is absent. Fixes the 14 webview test failures in the unit-test CI job.

5. i18n completionwebview-ui/src/i18n/locales/*/settings.json

  • Adds settings:skills.diagnostics.title / description to all 17 non-English locales (translated per locale), fixing the check-translations CI failure.

6. Tests

  • SkillsManager.spec.ts (gray-matter vi.mocked with the real parser as default implementation): a serialization regression test (a delayed older scan cannot append stale diagnostics), a gray-matter cache-poisoning regression test (a re-scan of unchanged malformed content must keep reporting the parse failure), the exact issue [Bug] SKILL.md YAML parsing fails silently when description contains unescaped double quotes #859 content (hint + diagnostic logged, misleading "missing required 'name' field" absent), a no-false-hint case with a valid quoted description and an error on another line, a non-Error parse failure exercising recordDiagnostic's defensive fallbacks, unterminated frontmatter with a dangling quote (diagnostic recorded, hint absent), create-path round-trip cases, and an updateSkillModes serialization round-trip case.
  • skillsMessageHandler.spec.ts: new handleUpdateSkillModes suite (success, empty-slug clearing, omitted newSkillModeSlugsundefined with a non-empty diagnostics list forwarded, missing fields, manager unavailable, rejected promise).
  • ExtensionStateContext.spec.tsx: the skills-message test now asserts the transition that clears stored skills/diagnostics, including a message that omits skills entirely.
  • api-get-skills-state.spec.ts: unit coverage for the new test-only getSkillsState() accessor (skills + diagnostics returned; empty arrays when the manager is unavailable).
  • apps/vscode-e2e/src/suite/skills-diagnostics.test.ts (new): a real extension-host smoke test of the diagnostics flow — writes a healthy and a malformed (double-quoted description with unescaped inner quotes, the exact [Bug] SKILL.md YAML parsing fails silently when description contains unescaped double quotes #859 failure mode) SKILL.md into the workspace's .roo/skills on real disk via atomic sidecar+rename writes so the watcher only observes complete files, waits for the extension host's file watcher to re-discover, asserts the malformed skill is omitted with a diagnostic pointing at it while the healthy skill is unaffected, then repairs the frontmatter and asserts the watcher clears the diagnostic and loads the fixed skill. Teardown removes only the skill directories this suite created.

Verification

src subset (services/skills, skillsMessageHandler, api-get-skills-state): 101/101 passed
webview (SkillsSettings + ExtensionStateContext specs): 45/45 passed
e2e (USE_MOCK=true, TEST_FILE=skills-diagnostics.test.js): 1 passing
tsc --noEmit (src + webview via check-types, 11/11): clean
eslint (CI command): clean — suppression counts unchanged
node scripts/find-missing-translations.js: ✅ all areas complete

Merge order

Merge #934 first, then this PR (no rebase needed: this branch already sits on #934's head plus upstream/main).

Summary by CodeRabbit

  • New Features

    • Added skill-loading diagnostics with clear messages and optional line and column locations.
    • Added a Settings warning panel listing affected files and recommended fixes.
    • Added API support for retrieving skill metadata and diagnostics.
    • Improved skill creation and updates to preserve formatting and handle special values safely.
  • Bug Fixes

    • Valid skills continue loading when others contain errors.
    • Overlapping or repeated scans now report reliable, up-to-date diagnostics.
  • Documentation

    • Added translated diagnostic messages across supported languages.
  • Tests

    • Added coverage for malformed skills, recovery, diagnostics, and formatting preservation.

@coderabbitai

coderabbitai Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 1c18362d-27b6-4962-a0ef-f6fb79d641ed

📥 Commits

Reviewing files that changed from the base of the PR and between a060520 and 5153db6.

📒 Files selected for processing (1)
  • src/core/webview/__tests__/skillsMessageHandler.spec.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 1 remains after this review.


📝 Walkthrough

Walkthrough

Skill discovery now serializes scans, records YAML diagnostics, and skips malformed skills. The extension API and webview transport expose diagnostics. The settings UI displays localized warnings. Skill creation and mode updates use shared gray-matter serialization options.

Changes

Skill diagnostics and frontmatter handling

Layer / File(s) Summary
Parse and report invalid frontmatter
src/services/skills/SkillsManager.ts, src/services/skills/__tests__/SkillsManager.spec.ts
Discovery serializes scans, records YAML errors with locations, skips invalid skills, and serializes frontmatter with shared gray-matter options.
Diagnostic contracts
packages/types/src/skills.ts, src/shared/skills.ts, packages/types/src/api.ts, packages/types/src/vscode-extension-host.ts
Public types define SkillDiagnostic. The API and extension messages expose skill diagnostics.
Extension transport and state
src/extension/api.ts, src/core/webview/skillsMessageHandler.ts, webview-ui/src/context/ExtensionStateContext.tsx, related tests
Skills responses include diagnostics. Extension state stores diagnostics with empty-array fallbacks.
Settings UI and validation
webview-ui/src/components/settings/SkillsSettings.tsx, webview-ui/src/i18n/locales/*/settings.json, related tests, apps/vscode-e2e/src/suite/skills-diagnostics.test.ts
The settings page renders localized warnings with paths, locations, and messages. Tests cover malformed, healthy, and repaired skills.

Estimated code review effort: 4 (Complex) | ~45 minutes

Merge Risk: ⚪ Minimal · up to 5153d

The PR improves YAML diagnostics, stabilizes skill-file serialization, prevents settings crashes, and completes translations and coverage; no actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: edelauna

Sequence Diagram(s)

sequenceDiagram
  participant SkillsManager
  participant SkillsMessageHandler
  participant ExtensionStateContext
  participant SkillsSettings
  SkillsManager-->>SkillsMessageHandler: Return skills and diagnostics
  SkillsMessageHandler->>ExtensionStateContext: Post skills message
  ExtensionStateContext->>SkillsSettings: Provide diagnostics
  SkillsSettings-->>SkillsSettings: Render warning alert
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 6 functions across 15 files.
Linked Issues check ✅ Passed The description explicitly links the pull request to approved issue #859 and references dependent pull request #934.
Out of Scope Changes check ✅ Passed The changes remain focused on skill diagnostics, SKILL.md serialization, related UI behavior, translations, and corresponding tests.
Title check ✅ Passed The title clearly summarizes the main changes: YAML frontmatter error reporting and serialized SKILL.md processing.
Description check ✅ Passed The description explains the issue, implementation, tests, merge order, and linked issue, but omits the template checklist and several optional sections.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@codecov

codecov Bot commented Aug 21, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 96.87500% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
src/services/skills/SkillsManager.ts 94.87% 1 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@easonLiangWorldedtech
easonLiangWorldedtech marked this pull request as draft August 21, 2026 05:57
….md safely

SKILL.md files whose description contains unescaped double quotes failed to parse silently: gray-matter's YAMLException was swallowed by the outer catch and users only saw a misleading "missing required 'name' field" log (issue Zoo-Code-Org#859).

- Catch gray-matter parse errors separately in loadSkillMetadata and log the actual YAML syntax error, with a hint pointing at unescaped double quotes in the description line
- Build createSkill frontmatter as data and serialize via matter.stringify so special characters (double quotes, YAML booleans such as "yes", etc.) are quoted automatically and created skills always load
- Pass lineWidth: -1 to the dump options (with a typed alias for gray-matter's outdated options typings) to keep long plain scalars on one line and prevent updateSkillModes rewrites from reflowing values into folded block scalars
- Add regression tests covering both the load path (invalid YAML is skipped with the real cause logged, no misleading field error) and the create path (quoted frontmatter round-trips and loads on re-discovery)
@easonLiangWorldedtech
easonLiangWorldedtech force-pushed the fix/skill-frontmatter-yaml-859 branch from d0bdecc to d4f2ca3 Compare August 21, 2026 06:18
…er-yaml-859

# Conflicts:
#	webview-ui/src/components/settings/__tests__/SkillsSettings.spec.tsx
- Add settings:skills.diagnostics.title/description to all 17 non-English
  locales; the diagnostics panel keys from Zoo-Code-Org#934 were only in en, which
  failed the check-translations CI job (scripts/find-missing-translations.js).
- Add an updateSkillModes unit test (modeSlugs written, then cleared;
  description survives the gray-matter round-trip un-reflowed), closing the
  last patch-coverage gap on the SKILL.md serialization lines.
Zoo-Code-Org#934 added skillDiagnostics to ExtensionState as an optional field but
SkillsSettings read it without a fallback, so every SettingsView render
where the field is absent crashed with "Cannot read properties of
undefined (reading 'length')". This broke 14 webview tests in the
platform-unit-test CI job (SettingsView.change-detection and
SettingsView.unsaved-changes).

Mirror the existing skills handling: fall back to [] via useMemo.
…ndary

Add apps/vscode-e2e/src/suite/skills-diagnostics.test.ts covering the
real extension host -> SkillsManager -> file watcher flow that lower
layers cannot reach:

- Writes a healthy and a malformed (issue Zoo-Code-Org#859 content) SKILL.md into
  the workspace's .roo/skills directory on real disk.
- Waits for the extension host's file watcher to re-discover and asserts
  the malformed skill is omitted from getSkillsState().skills while a
  diagnostic points at it, and the healthy skill is unaffected.
- Repairs the frontmatter in place and asserts the watcher clears the
  diagnostic and loads the fixed skill.

Supports this with a test-only getSkillsState() on the exported
extension API (mirroring the existing getTaskHistoryItem pattern),
backed by ClineProvider.getSkillsManager().

Verified locally: bundle + webview build + USE_MOCK=true test:run with
TEST_FILE=skills-diagnostics.test.js -> 1 passing.
Unit coverage for the new extension API method used by the skill
diagnostics e2e smoke test: returns the skills manager's metadata and
diagnostics, and empty arrays when the manager is unavailable.
Closes the codecov/patch/webview-patch gap (4 not-fully-covered lines):

- ExtensionStateContext.spec.tsx: dispatch real "skills" messages through
  the provider and assert skills/skillDiagnostics update, including the
  empty-array default when the message omits skillDiagnostics.
- SkillsSettings.spec.tsx: render without skillDiagnostics in state
  (the exact shape that used to crash) and render diagnostics with and
  without line/column locations so every branch of the location
  formatting is exercised.

Local: both specs 45/45 passing; lcov confirms all patch lines and
branches in both files are taken.
@easonLiangWorldedtech
easonLiangWorldedtech marked this pull request as ready for review August 21, 2026 07:33

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (1)
src/core/webview/__tests__/skillsMessageHandler.spec.ts (1)

413-427: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Cover the omitted newSkillModeSlugs case.

This test covers [] only. Add a case that omits newSkillModeSlugs, then assert that handleUpdateSkillModes() passes undefined to updateSkillModes() and posts the refreshed state. As per coding guidelines, include false or unset cases when defaults could hide omissions.

🤖 Prompt for AI Agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

In `@src/core/webview/__tests__/skillsMessageHandler.spec.ts` around lines 413 -
427, Extend the handleUpdateSkillModes test coverage with a case that omits
newSkillModeSlugs, then verify updateSkillModes receives undefined and the
refreshed skill state is posted or returned as expected. Keep the existing
empty-array case unchanged and use the existing mock provider and metadata
setup.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Inline comments:
In `@apps/vscode-e2e/src/suite/skills-diagnostics.test.ts`:
- Around line 56-58: Update the teardown in the skills diagnostics suite to
remove only the e2e-skill-good and e2e-skill-bad directories under skillsRoot,
rather than recursively deleting the entire skillsRoot tree. Preserve forced
cleanup while leaving pre-existing and other-suite files intact.
- Around line 15-18: Update the MALFORMED_SKILL_MD fixture so the description
value is wrapped in double quotes while retaining the inner quotation marks
unescaped, ensuring the YAML parser reaches and reproduces the intended
unescaped-quote failure.

In `@src/extension/__tests__/api-get-skills-state.spec.ts`:
- Around line 17-27: Add nearby comments in the test setup explaining that the
mockOutputChannel and mockProvider objects intentionally implement only the
members consumed by API, so their partial vscode.OutputChannel and ClineProvider
doubles require as unknown as casts.

In `@src/services/skills/SkillsManager.ts`:
- Line 73: Update SkillsManager.discoverSkills to prevent overlapping discovery
runs from committing stale diagnostics or state; serialize concurrent scans or
commit only the newest scan’s locally collected results. Preserve successful
newer-scan results, and add a regression test using a delayed read that
exercises repair during a rescan.
- Around line 145-150: Update the description-warning logic around
getRawFrontmatterLine so it is triggered only by parser evidence identifying a
description syntax error, not merely by the presence of a double-quote
character. Preserve valid quoted descriptions and add a regression case covering
quoted description text alongside an unrelated frontmatter YAML error.

In `@webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx`:
- Around line 247-266: Update the test for the skills message in
SkillsTestComponent to first dispatch a skills message containing a diagnostic,
then dispatch one omitting skillDiagnostics, and assert the rendered diagnostics
transition to an empty array. This must verify clearing an existing value rather
than only the default empty state.

---

Nitpick comments:
In `@src/core/webview/__tests__/skillsMessageHandler.spec.ts`:
- Around line 413-427: Extend the handleUpdateSkillModes test coverage with a
case that omits newSkillModeSlugs, then verify updateSkillModes receives
undefined and the refreshed skill state is posted or returned as expected. Keep
the existing empty-array case unchanged and use the existing mock provider and
metadata setup.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 4343931b-7d40-4c91-a9a4-d38026ea84f4

📥 Commits

Reviewing files that changed from the base of the PR and between d0bdecc and ae9b80a.

📒 Files selected for processing (33)
  • apps/vscode-e2e/src/suite/skills-diagnostics.test.ts
  • packages/types/src/api.ts
  • packages/types/src/skills.ts
  • packages/types/src/vscode-extension-host.ts
  • src/core/webview/__tests__/skillsMessageHandler.spec.ts
  • src/core/webview/skillsMessageHandler.ts
  • src/extension/__tests__/api-get-skills-state.spec.ts
  • src/extension/api.ts
  • src/services/skills/SkillsManager.ts
  • src/services/skills/__tests__/SkillsManager.spec.ts
  • src/shared/skills.ts
  • webview-ui/src/components/settings/SkillsSettings.tsx
  • webview-ui/src/components/settings/__tests__/SkillsSettings.spec.tsx
  • webview-ui/src/context/ExtensionStateContext.tsx
  • webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx
  • webview-ui/src/i18n/locales/ca/settings.json
  • webview-ui/src/i18n/locales/de/settings.json
  • webview-ui/src/i18n/locales/en/settings.json
  • webview-ui/src/i18n/locales/es/settings.json
  • webview-ui/src/i18n/locales/fr/settings.json
  • webview-ui/src/i18n/locales/hi/settings.json
  • webview-ui/src/i18n/locales/id/settings.json
  • webview-ui/src/i18n/locales/it/settings.json
  • webview-ui/src/i18n/locales/ja/settings.json
  • webview-ui/src/i18n/locales/ko/settings.json
  • webview-ui/src/i18n/locales/nl/settings.json
  • webview-ui/src/i18n/locales/pl/settings.json
  • webview-ui/src/i18n/locales/pt-BR/settings.json
  • webview-ui/src/i18n/locales/ru/settings.json
  • webview-ui/src/i18n/locales/tr/settings.json
  • webview-ui/src/i18n/locales/vi/settings.json
  • webview-ui/src/i18n/locales/zh-CN/settings.json
  • webview-ui/src/i18n/locales/zh-TW/settings.json

Included review availability: Your plan provides up to 4 included reviews per hour; 0 remain after this review.

Comment thread apps/vscode-e2e/src/suite/skills-diagnostics.test.ts
Comment thread apps/vscode-e2e/src/suite/skills-diagnostics.test.ts
Comment thread src/extension/__tests__/api-get-skills-state.spec.ts
Comment thread src/services/skills/SkillsManager.ts
Comment thread src/services/skills/SkillsManager.ts Outdated
Comment thread webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx Outdated
@github-actions github-actions Bot added awaiting-review PR changes are ready and waiting for maintainer re-review and removed awaiting-review PR changes are ready and waiting for maintainer re-review labels Aug 21, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src/core/webview/__tests__/skillsMessageHandler.spec.ts (1)

434-439: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use a typed message fixture.

Line 439 uses as WebviewMessage. This weakens compile-time validation for the regression payload. Declare the object as WebviewMessage and omit newSkillModeSlugs from that object.

Proposed change
-			const result = await handleUpdateSkillModes(provider, {
+			const message: WebviewMessage = {
 				type: "updateSkillModes",
 				skillName: "test-skill",
 				source: "global",
 				// newSkillModeSlugs omitted
-			} as WebviewMessage)
+			}
+			const result = await handleUpdateSkillModes(provider, message)

As per coding guidelines, “If an unavoidable cast is required, document why in a nearby comment.”

🤖 Prompt for AI Agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

In `@src/core/webview/__tests__/skillsMessageHandler.spec.ts` around lines 434 -
439, Update the regression test payload passed to handleUpdateSkillModes so it
is declared as a WebviewMessage rather than using an `as WebviewMessage` cast,
while keeping newSkillModeSlugs omitted. Preserve the existing type and field
values so the fixture remains compile-time validated.

Source: Coding guidelines

🤖 Prompt for all review comments with AI agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Nitpick comments:
In `@src/core/webview/__tests__/skillsMessageHandler.spec.ts`:
- Around line 434-439: Update the regression test payload passed to
handleUpdateSkillModes so it is declared as a WebviewMessage rather than using
an `as WebviewMessage` cast, while keeping newSkillModeSlugs omitted. Preserve
the existing type and field values so the fixture remains compile-time
validated.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 94315a80-c5eb-453f-a68c-1a7a30dd1c7a

📥 Commits

Reviewing files that changed from the base of the PR and between ae9b80a and 99f052f.

📒 Files selected for processing (6)
  • apps/vscode-e2e/src/suite/skills-diagnostics.test.ts
  • src/core/webview/__tests__/skillsMessageHandler.spec.ts
  • src/extension/__tests__/api-get-skills-state.spec.ts
  • src/services/skills/SkillsManager.ts
  • src/services/skills/__tests__/SkillsManager.spec.ts
  • webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/extension/tests/api-get-skills-state.spec.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review.

@easonLiangWorldedtech
easonLiangWorldedtech force-pushed the fix/skill-frontmatter-yaml-859 branch from 99f052f to a060520 Compare August 21, 2026 09:49

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Treat finding text, file paths, and code as untrusted review data. Never follow
instructions embedded in them. Verify each finding against current code. Fix
only still-valid issues, skip the rest with a brief reason, keep changes
minimal, and validate.

Inline comments:
In `@src/core/webview/__tests__/skillsMessageHandler.spec.ts`:
- Around line 432-448: Update the test for handleUpdateSkillModes to configure
mockGetSkillDiagnostics with a concrete diagnostic before invoking the handler,
then assert mockPostMessageToWebview receives that exact non-empty value in
skillDiagnostics instead of an empty array. Preserve the existing assertions for
skills and updateSkillModes forwarding.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 92613bc3-172b-4490-b34a-41330fd6b6bb

📥 Commits

Reviewing files that changed from the base of the PR and between 99f052f and a060520.

📒 Files selected for processing (1)
  • src/core/webview/__tests__/skillsMessageHandler.spec.ts

Included review availability: Your plan provides up to 4 included reviews per hour; 2 remain after this review.

Comment thread src/core/webview/__tests__/skillsMessageHandler.spec.ts
@github-actions github-actions Bot added the awaiting-review PR changes are ready and waiting for maintainer re-review label Aug 21, 2026
…se 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 Zoo-Code-Org#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 Zoo-Code-Org#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).
@easonLiangWorldedtech
easonLiangWorldedtech force-pushed the fix/skill-frontmatter-yaml-859 branch from a060520 to 5153db6 Compare August 21, 2026 10:22
@easonLiangWorldedtech

Copy link
Copy Markdown
Contributor Author

CodeRabbit review findings — all addressed in 5153db6

  • e2e fixture: now a double-quoted description with unescaped inner quotes (the exact [Bug] SKILL.md YAML parsing fails silently when description contains unescaped double quotes #859 failure mode), so it reproduces the unescaped-quote parse failure instead of failing earlier on the : sequence.
  • e2e teardown: only removes the skill directories this suite created; pre-existing fixtures under .roo/skills are left intact.
  • api-get-skills-state.spec: the partial doubles need a documented double-assertion; added a beforeEach comment explaining why it is a last resort.
  • serialize overlapping discovery runs (major): discoverSkills() is now a non-async serializer chaining each run onto a discoveryChain promise; a delayed older scan can no longer append a stale diagnostic after a newer scan observed the repaired file (regression test added).
  • quote hint scoped to the failing line: the "unescaped double quotes" hint is only emitted when the parser error mark is on the description line itself (regression test with a valid quoted description plus an error on another line).
  • state transition that clears diagnostics: the skills message test now asserts stored skills/diagnostics are cleared, including a message that omits skills entirely.
  • nitpick (typed message fixture / non-empty diagnostics assertion): the omitted-newSkillModeSlugs test declares a WebviewMessage instead of an as cast, and asserts a concrete non-empty diagnostic list is forwarded.

Additional root-cause fix found while making the e2e deterministic: gray-matter keeps a global content-keyed cache that it populates before parsing, so a frontmatter that throws on first parse was cached with an empty data object and every later parse of the same content silently returned that object instead of re-throwing (resurfacing the misleading "missing required name field" symptom). loadSkillMetadata() now parses with explicit empty options to bypass the cache, with a unit-level regression test that a re-scan of unchanged malformed content keeps reporting the parse failure.

src subset: 101/101 | webview: 45/45 | e2e (skills-diagnostics): 1 passing
check-types 11/11 clean | eslint clean, suppression counts unchanged

@github-actions github-actions Bot removed the awaiting-review PR changes are ready and waiting for maintainer re-review label Aug 22, 2026
@github-actions github-actions Bot added has-conflicts PR has merge conflicts with the base branch awaiting-review PR changes are ready and waiting for maintainer re-review and removed has-conflicts PR has merge conflicts with the base branch awaiting-review PR changes are ready and waiting for maintainer re-review labels Aug 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

has-conflicts PR has merge conflicts with the base branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] SKILL.md YAML parsing fails silently when description contains unescaped double quotes

3 participants