feat: skills framework — builtin skills, matching, CLI, and MCP tools#50
feat: skills framework — builtin skills, matching, CLI, and MCP tools#50JordanCoin wants to merge 2 commits intofeat/intelligent-routingfrom
Conversation
1b97680 to
73f6020
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces a new “skills framework” to codemap: skills are Markdown files with YAML frontmatter that can be loaded from builtin/project/global locations, matched against prompt intent/files/languages, and injected into hook output; it also exposes skills via a CLI subcommand and new MCP tools.
Changes:
- Added
skills/package for parsing/loading embedded + local skills and matching the top skills by signals (intent category, languages, path patterns, priority). - Added 5 embedded builtin skills under
skills/builtin/and tests for parsing/loading/matching. - Integrated skills into
hook prompt-submit, plus addedcodemap skill list|show|initand MCP toolslist_skills/get_skill.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| skills/types.go | Defines skill metadata/types and indexing helpers. |
| skills/loader.go | Implements frontmatter parsing, multi-source loading, matching, and path matching. |
| skills/embed.go | Embeds builtin skill markdown files into the binary. |
| skills/loader_test.go | Adds unit/integration coverage for parsing/loading/matching skills. |
| skills/builtin/test-first.md | Builtin “test-first” skill content. |
| skills/builtin/refactor.md | Builtin “refactor” skill content. |
| skills/builtin/hub-safety.md | Builtin “hub-safety” skill content. |
| skills/builtin/handoff.md | Builtin “handoff” skill content. |
| skills/builtin/explore.md | Builtin “explore” skill content. |
| cmd/hooks.go | Injects matched skills into prompt-submit output (marker + bodies) and fixes hub edit counting. |
| cmd/hooks_test.go | Updates session progress test for unique hub-file counting. |
| cmd/skill.go | Adds codemap skill CLI subcommand (list/show/init). |
| mcp/main.go | Adds MCP tools to list/get skills. |
| main.go | Routes skill subcommand before global flag parsing. |
| go.mod | Adds YAML dependency for frontmatter parsing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ## Test file conventions | ||
|
|
||
| | Language | Convention | | ||
| |----------|-----------| | ||
| | Go | `*_test.go` in same package | | ||
| | TypeScript | `*.test.ts` or `*.spec.ts` | | ||
| | Python | `test_*.py` or `*_test.py` | |
There was a problem hiding this comment.
The markdown table header uses || (empty first column), which renders incorrectly in many Markdown parsers. It likely should be a standard table like | Language | Convention | with |---|---| separator row.
| ## Artifacts written | ||
|
|
||
| | File | Content | | ||
| |------|---------| | ||
| | `.codemap/handoff.latest.json` | Full artifact (all layers) | | ||
| | `.codemap/handoff.prefix.json` | Stable prefix snapshot | | ||
| | `.codemap/handoff.delta.json` | Dynamic delta snapshot | |
There was a problem hiding this comment.
The markdown tables use || (empty first column), which often renders incorrectly. Use standard table syntax (single leading |) for better compatibility.
| // matchPath does a simple glob-like match: supports * and ** patterns, | ||
| // and falls back to substring matching. | ||
| func matchPath(pattern, path string) bool { | ||
| // Simple substring match for non-glob patterns | ||
| if !strings.ContainsAny(pattern, "*?") { | ||
| return strings.Contains(path, pattern) | ||
| } | ||
| // Use filepath.Match for simple globs | ||
| matched, _ := filepath.Match(pattern, path) | ||
| if matched { | ||
| return true | ||
| } | ||
| // Also try matching against just the filename | ||
| matched, _ = filepath.Match(pattern, filepath.Base(path)) | ||
| return matched |
There was a problem hiding this comment.
The comment says matchPath supports ** patterns, but the implementation uses filepath.Match, which does not implement ** recursive glob semantics. Either implement ** handling (e.g., by translating to a recursive match / doublestar) or adjust the comment and documented pattern support.
| golang.org/x/oauth2 v0.30.0 // indirect | ||
| golang.org/x/sys v0.38.0 // indirect | ||
| golang.org/x/text v0.3.8 // indirect | ||
| gopkg.in/yaml.v3 v3.0.1 // indirect |
There was a problem hiding this comment.
gopkg.in/yaml.v3 is imported directly (skills/loader.go) but is listed as // indirect in go.mod. Running go mod tidy (or moving it to the direct require block) will keep module metadata accurate.
| if strings.HasPrefix(path, "~/") { | ||
| home := os.Getenv("HOME") | ||
| path = filepath.Join(home, path[2:]) | ||
| } |
There was a problem hiding this comment.
Tilde expansion here uses os.Getenv("HOME"), which won’t work on all platforms (notably Windows). Prefer os.UserHomeDir() (and handle its error) to make ~/ expansion portable.
| if totalBytes+len(body) > maxSkillOutputBytes { | ||
| remaining := maxSkillOutputBytes - totalBytes | ||
| if remaining > 100 { | ||
| body = body[:remaining] + "\n... (skill truncated)" | ||
| } else { | ||
| break |
There was a problem hiding this comment.
Skill output truncation can exceed the intended 8KB budget because you slice to remaining and then append the truncation suffix, making the final string longer than remaining. Also, byte-slicing can split a UTF-8 rune, producing invalid output. Consider truncating to a rune boundary and subtracting the suffix length from the remaining budget (or count bytes after adding the suffix).
| fmt.Println("Commands:") | ||
| fmt.Println(" list Show all available skills with descriptions") | ||
| fmt.Println(" show <name> Print full skill content") | ||
| fmt.Println(" init Create a SKILL.md template in .codemap/skills/") |
There was a problem hiding this comment.
This help text says init creates a SKILL.md template, but the implementation writes .codemap/skills/my-skill.md. Update the message or the filename to avoid confusing users about the expected convention.
| fmt.Println(" init Create a SKILL.md template in .codemap/skills/") | |
| fmt.Println(" init Create a my-skill.md template in .codemap/skills/") |
| func runSkillInit(root string) { | ||
| skillsDir := root + "/.codemap/skills" | ||
| if err := os.MkdirAll(skillsDir, 0755); err != nil { | ||
| fmt.Fprintf(os.Stderr, "Error creating skills directory: %v\n", err) | ||
| os.Exit(1) | ||
| } | ||
|
|
||
| template := `--- | ||
| name: my-skill | ||
| description: Describe when this skill should activate and what it does | ||
| priority: 5 | ||
| keywords: ["keyword1", "keyword2"] | ||
| languages: ["go"] | ||
| --- | ||
|
|
||
| # My Skill | ||
|
|
||
| Instructions for the AI agent when this skill is activated. | ||
|
|
||
| ## When to use | ||
|
|
||
| Describe the situations where this skill applies. | ||
|
|
||
| ## Steps | ||
|
|
||
| 1. First step | ||
| 2. Second step | ||
| 3. Third step | ||
| ` | ||
|
|
||
| path := skillsDir + "/my-skill.md" | ||
| if _, err := os.Stat(path); err == nil { |
There was a problem hiding this comment.
Paths are constructed via string concatenation (root + "/.codemap/skills", skillsDir + "/my-skill.md"), which is not portable on Windows and inconsistent with the rest of the repo’s filepath.Join usage. Use filepath.Join(root, ".codemap", "skills") and filepath.Join(skillsDir, "my-skill.md").
|
|
||
| path := filepath.Join(dir, entry.Name()) | ||
| data, err := os.ReadFile(path) | ||
| if err != nil { | ||
| continue | ||
| } | ||
|
|
||
| skill, err := ParseSkill(string(data), path, source) | ||
| if err != nil { | ||
| continue | ||
| } | ||
| skills = append(skills, *skill) | ||
| } |
There was a problem hiding this comment.
loadSkillsFromDir silently skips files that fail to read or parse (continue), so a single malformed skill file will just disappear from codemap skill list/show with no user-visible error. Consider returning an aggregated error (or at least the first error) when the directory exists, and only ignore os.IsNotExist at the call site.
| continue | ||
| } | ||
|
|
||
| skill, err := ParseSkill(string(data), "builtin/"+entry.Name(), builtinSource) | ||
| if err != nil { | ||
| continue |
There was a problem hiding this comment.
loadBuiltinSkills silently ignores embedded file read/parse errors. If an embedded skill is malformed, the binary will start without it and there’s no error surfaced to the caller. Consider failing fast (returning an error) when a builtin skill can’t be loaded.
| continue | |
| } | |
| skill, err := ParseSkill(string(data), "builtin/"+entry.Name(), builtinSource) | |
| if err != nil { | |
| continue | |
| return nil, err | |
| } | |
| skill, err := ParseSkill(string(data), "builtin/"+entry.Name(), builtinSource) | |
| if err != nil { | |
| return nil, err |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1b97680f2d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Use filepath.Match for simple globs | ||
| matched, _ := filepath.Match(pattern, path) | ||
| if matched { |
There was a problem hiding this comment.
Handle recursive
** in path pattern matching
matchPath sends glob patterns to filepath.Match, but that matcher does not implement recursive ** and * cannot cross path separators. In practice, patterns users are likely to write for skill scoping (for example **/*.go or src/**) will fail to match nested paths like cmd/hooks.go, so relevant path-scoped skills are silently never selected.
Useful? React with 👍 / 👎.
…ools Phase 2 of codemap's evolution. Skills are markdown files with YAML frontmatter that provide context-aware guidance to AI agents. They're matched against the user's intent, mentioned files, and detected languages. New package: skills/ - types.go — SkillMeta, Skill, SkillIndex with fast lookup by name/keyword/language - loader.go — Multi-source loading (builtin → project → global), YAML frontmatter parsing, deduplication (later sources override), weighted matching - embed.go — go:embed for builtin skills - 5 builtin skills: hub-safety, refactor, test-first, explore, handoff New: cmd/skill.go — CLI subcommand (list, show, init) New: MCP tools — list_skills (metadata only) and get_skill (full body) Hook integration: - hookPromptSubmit matches skills against intent + files + languages - Top 3 skills injected with <!-- codemap:skills --> structured marker - Budget-capped at 8KB to prevent context blowup Bug fixes from parallel reviews: - [P1] Priority boost only applies after real signal (codex finding) - [P2] Fallback name derived from filename for frontmatter-less skills - [P1] showSessionProgress now counts unique hub files, not hub events (Claude code-reviewer finding from Phase 1 worktree review) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
73f6020 to
0484f8e
Compare
…oadmap Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Phase 2 of the AI coding system evolution. Builds on #49 (intelligent routing).
Skills are markdown files with YAML frontmatter that provide context-aware guidance to AI agents. They're automatically matched against the user's intent, mentioned files, and detected languages — then injected into the prompt-submit hook output.
What's new
skills/package — YAML frontmatter parser, multi-source loader (builtin → project-local → global), deduplication, weighted matching enginehub-safety,refactor,test-first,explore,handoffcodemap skill list|show|initCLI subcommandlist_skills/get_skillMCP tools with progressive disclosure (list = metadata, get = full body)<!-- codemap:skills -->marker, budget-capped at 8KBThe community contribution surface
Anyone can add a skill by dropping a
.mdfile in.codemap/skills/— no Go code needed. Project-local skills override builtins. This is the sustainability flywheel.Bug fixes from parallel reviews
Test plan
go test ./... -race— all 10 packages passcodemap skill listshows 5 builtinscodemap skill show hub-safetyrenders full content🤖 Generated with Claude Code