Fix Claude skill markdown warnings#4441
Conversation
github-actions
Bot
added
v2
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates skill discovery to treat .claude/skills as a mixed-content directory by requiring “skill-like” frontmatter before flat Markdown files are considered skills, and adds tests to lock in the new behavior.
Changes:
- Add a Claude-root-specific gate that ignores flat
<name>.mdfiles unless they contain recognized skill frontmatter keys - Refactor parsing flow (
parse/parseFlat→parseSkill) and thread a “require flat marker” flag through discovery - Add tests asserting non-skill Markdown in
.claude/skillsis ignored and that missingdescriptionemits a warning for skill-like files
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| internal/skill/skill.go | Adds Claude-root marker requirement for flat Markdown discovery; refactors parsing and adds marker detection helper |
| internal/skill/skill_test.go | Adds coverage for ignoring non-skill Markdown in .claude/skills and warning on skill-like files missing description |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Dir string | ||
| Scope Scope | ||
| Priority int | ||
| Status PathStatus | ||
| requireFlatMarker bool |
| func hasSkillMarker(fm map[string]string) bool { | ||
| for _, key := range []string{"description", "name", "runas", "context", "agent", "allowed-tools", "model", "effort"} { | ||
| if strings.TrimSpace(fm[key]) != "" { | ||
| return true | ||
| } | ||
| } | ||
| return false | ||
| } |
| } | ||
|
|
||
| func hasSkillMarker(fm map[string]string) bool { | ||
| for _, key := range []string{"description", "name", "runas", "context", "agent", "allowed-tools", "model", "effort"} { |
|
|
||
| func TestNonSkillMarkdownInClaudeSkillRootsIgnored(t *testing.T) { | ||
| proj := t.TempDir() | ||
| writeSkill(t, proj, ".claude/skills/README.md", "# Skill notes\n\nThis is documentation, not a skill.") |
| for _, name := range []string{"README", "notes"} { | ||
| if _, ok := find(list, name); ok { | ||
| t.Errorf("non-skill markdown %q should not be listed", name) | ||
| } | ||
| } |
thomasvan
changed the title
|
Thanks @thomasvan for this fix. I integrated the Claude flat-Markdown warning behavior and regression tests into #4510 together with the overlapping Windows linked directory work from #4442, because the two PRs conflict in either merge order. Authorship note: @SivanCola is the maintainer integrator for #4510. @thomasvan contributed as a repository contributor through this PR, and that contribution is preserved and acknowledged in #4510. Closing this PR as superseded by #4510 so the repository has one conflict-free merge path. |
Summary
.claude/skillsroots as skills.SKILL.mdloading and existing non-Claude flat skill behavior.Validation
go test ./internal/skill ./internal/configgo test ./internal/bootwith isolatedHOMEandXDG_CONFIG_HOMEgit diff --check -- internal/skill/skill.go internal/skill/skill_test.goNote:
go test ./internal/clifailed in the default environment because configured model credentials madeTestModelRefsSkipsUnconfiguredsee provider refs. A clean-env rerun was started but did not finish after several minutes, so it was stopped.