Skip to content

refactor skills loader markdown metadata parsing#1354

Merged
yinwm merged 1 commit intosipeed:mainfrom
nayihz:fix/skill-loader-markdown-metadata
Mar 11, 2026
Merged

refactor skills loader markdown metadata parsing#1354
yinwm merged 1 commit intosipeed:mainfrom
nayihz:fix/skill-loader-markdown-metadata

Conversation

@nayihz
Copy link
Contributor

@nayihz nayihz commented Mar 11, 2026

📝 Description

This PR is primarily intended to fix the startup failure where picoclaw reports invalid skill from workspace while loading workspace skills.

To address that, it refactors pkg/skills/loader.go metadata extraction for SKILL.md files:

  • Parse markdown body via gomarkdown AST and extract metadata from first H1 + first paragraph.
  • Parse frontmatter once (splitFrontmatter) and reuse body/frontmatter slices instead of scanning twice.
  • Replace ad-hoc YAML parsing with yaml.v3 for robust multiline/structured frontmatter support.
  • Add/extend unit tests for markdown metadata extraction, multiline YAML description, heading fallback, and HTML comment tolerance.

🗣️ Type of Change

  • 🐞 Bug fix (non-breaking change which fixes an issue)
  • ✨ New feature (non-breaking change which adds functionality)
  • 📖 Documentation update
  • ⚡ Code refactoring (no functional changes, no api changes)

🤖 AI Code Generation

  • 🤖 Fully AI-generated (100% AI, 0% Human)
  • 🛠️ Mostly AI-generated (AI draft, Human verified/modified)
  • 👨‍💻 Mostly Human-written (Human lead, AI assisted or none)

🔗 Related Issue

N/A

📚 Technical Context (Skip for Docs)

  • Reference URL: https://github.com/gomarkdown/markdown
  • Reasoning: The key goal is to make skill metadata parsing tolerant to valid markdown variants (including comment blocks and richer frontmatter formatting) so workspace skills are not incorrectly rejected as invalid during process startup.
  • Binary size impact: This PR introduces a new direct dependency on github.com/gomarkdown/markdown. Based on local darwin/arm64 builds, binary size changed from 20,815,442 bytes to 21,240,146 bytes, i.e. +424,704 bytes (+2.04%).

🧪 Test Environment

  • Hardware: MacBook (local dev machine)
  • OS: macOS
  • Model/Provider: N/A (unit tests only)
  • Channels: N/A

📸 Evidence (Optional)

Click to view Logs/Screenshots

go test ./pkg/skills -run "TestGetSkillMetadata|Test(ExtractFrontmatter|StripFrontmatter|ListSkills|SkillRoots)" -count=1

Binary size check:

  • Before: -rwxr-xr-x ... 20815442 ... picoclaw-darwin-arm64
  • After: -rwxr-xr-x ... 21240146 ... picoclaw-darwin-arm64

☑️ Checklist

  • My code/docs follow the style of this project.
  • I have performed a self-review of my own changes.
  • I have updated the documentation accordingly.

@CLAassistant
Copy link

CLAassistant commented Mar 11, 2026

CLA assistant check
All committers have signed the CLA.

@nayihz nayihz force-pushed the fix/skill-loader-markdown-metadata branch from 96a1363 to 236209a Compare March 11, 2026 09:06
@sipeed-bot sipeed-bot bot added type: bug Something isn't working domain: skill go Pull requests that update go code dependencies Pull requests that update a dependency file labels Mar 11, 2026
Copy link
Collaborator

@yinwm yinwm left a comment

Choose a reason for hiding this comment

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

LGTM! This is a solid refactor that improves metadata parsing robustness:

  • Standard yaml.v3 parser replaces ad-hoc YAML parsing
  • Markdown AST parsing is more accurate than regex
  • Backward compatibility preserved (JSON → YAML → Markdown → fallback)
  • Good test coverage for new functionality

Minor suggestion for follow-up: update the stale comments in parseSimpleYAML and add cross-platform newline tests.

@yinwm yinwm merged commit 8a39898 into sipeed:main Mar 11, 2026
4 checks passed
fishtrees pushed a commit to fishtrees/picoclaw that referenced this pull request Mar 12, 2026
dj-oyu pushed a commit to dj-oyu/picoclaw that referenced this pull request Mar 14, 2026
dj-oyu pushed a commit to dj-oyu/picoclaw that referenced this pull request Mar 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dependencies Pull requests that update a dependency file domain: skill go Pull requests that update go code type: bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants