Bug Fix: Fix parsing of SKILL.md file frontmatter - regex#411
Bug Fix: Fix parsing of SKILL.md file frontmatter - regex#411Leeaandrob merged 3 commits intosipeed:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug in the SKILL.md frontmatter parsing that prevented skills from being loaded on systems using Windows (\r\n) or classic Mac (\r) line endings. The original regex pattern only supported Unix line endings (\n), causing the parser to fail to extract the required name and description metadata from skill files. This resulted in "description is required" validation errors when loading skills.
Changes:
- Updated the
extractFrontmatterregex to support\r\n,\n, and\rline endings - Added logging for skill metadata read failures to aid debugging
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func (sl *SkillsLoader) stripFrontmatter(content string) string { | ||
| re := regexp.MustCompile(`^---\n.*?\n---\n`) | ||
| return re.ReplaceAllString(content, "") |
There was a problem hiding this comment.
The stripFrontmatter function needs to be updated to handle different line endings (\r\n, \n, \r) consistently with the extractFrontmatter function. Currently, it only handles Unix line endings (\n), which means it will fail to strip frontmatter from files with Windows or classic Mac line endings. This inconsistency could cause the frontmatter to remain in the skill content when it's loaded for use.
pkg/skills/loader.go
Outdated
There was a problem hiding this comment.
The parseSimpleYAML function splits content by \n only (line 296), which won't correctly parse YAML frontmatter from files with Windows (\r\n) or classic Mac (\r) line endings. Since extractFrontmatter now supports all three line ending types, parseSimpleYAML should normalize line endings or use a more robust splitting approach to handle the extracted frontmatter content correctly. Consider using strings.Split with a custom function that handles all three line ending types, or normalize the content before splitting.
| func (sl *SkillsLoader) extractFrontmatter(content string) string { | ||
| // (?s) enables DOTALL mode so . matches newlines | ||
| // Match first ---, capture everything until next --- on its own line | ||
| re := regexp.MustCompile(`(?s)^---\n(.*)\n---`) | ||
| // Support \n (Unix), \r\n (Windows), and \r (classic Mac) line endings for frontmatter blocks | ||
| // (?s) enables DOTALL so . matches newlines; | ||
| // ^--- at start, then ... --- at start of line, honoring all three line ending types | ||
| re := regexp.MustCompile(`(?s)^---(?:\r\n|\n|\r)(.*?)(?:\r\n|\n|\r)---`) | ||
| match := re.FindStringSubmatch(content) | ||
| if len(match) > 1 { | ||
| return match[1] | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
The extractFrontmatter function lacks test coverage for the new multi-line-ending support. Since this fix addresses a critical bug (skills not being loaded due to line ending issues), it would be valuable to add tests that verify the regex correctly extracts frontmatter from content with \n (Unix), \r\n (Windows), and \r (classic Mac) line endings. This would prevent regression and ensure the fix works as intended across all platforms.
There was a problem hiding this comment.
Reviewer to confirm if this is necessary or not.
Bug Fix: Fix parsing of SKILL.md file frontmatter - regex
📝 Description
SKILLS.md files contain frontmatter, that is further used to populate Name and Description for Skills. Current regex for frontmatter parsing did not account for different types of newline characters.
\r\nand \ron top of existing\n`.WARN invalid skill from workspace name=summarize error="description is required"🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Fixes / Closes #398
📚 Technical Context (Skip for Docs)
🧪 Test Environment
Tested on Docker
📸 Evidence (Optional)
Click to view Logs/Screenshots
☑️ Checklist