fix(agent): auto-inject SKILL.md into LLM context when user references a skill#1253
fix(agent): auto-inject SKILL.md into LLM context when user references a skill#1253anugotta wants to merge 3 commits intosipeed:mainfrom
Conversation
|
|
sipeed-bot
bot
added
type: bug
There was a problem hiding this comment.
Pull request overview
This PR wires the skills subsystem into the agent loop so that when a user references an installed skill by name, the corresponding SKILL.md instructions are automatically injected into the system prompt for that request (improving reliability on smaller/local models).
Changes:
- Added per-message skill name matching and skill-context loading via
ContextBuilderhelpers. - Extended
ContextBuilder.BuildMessageswith an optional variadic skill context parameter and injected it into the system message. - Wired skill matching/injection into
AgentLoop.runAgentLoop, and added unit tests for matching/loading/injection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/agent/loop.go | Matches skills from the user message and passes loaded SKILL.md context into message building. |
| pkg/agent/context.go | Adds skill matching/loading helpers and injects active skill instructions into the system prompt via BuildMessages. |
| pkg/agent/context_skills_test.go | New tests for skill name matching, skill context loading, and system prompt injection behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Auto-inject SKILL.md content when the user references an installed skill. | ||
| var skillCtx string | ||
| if matched := agent.ContextBuilder.MatchSkillsInMessage(opts.UserMessage); len(matched) > 0 { | ||
| skillCtx = agent.ContextBuilder.LoadSkillContext(matched) | ||
| logger.DebugCF("agent", "Skills matched in user message", | ||
| map[string]any{"matched": matched, "context_len": len(skillCtx)}) | ||
| } | ||
|
|
||
| messages := agent.ContextBuilder.BuildMessages( | ||
| history, | ||
| summary, | ||
| opts.UserMessage, | ||
| opts.Media, | ||
| opts.Channel, | ||
| opts.ChatID, | ||
| skillCtx, | ||
| ) |
There was a problem hiding this comment.
skillCtx is injected into the initial BuildMessages call, but when a context-window error triggers compression retry, messages are rebuilt without passing skillCtx (later BuildMessages call in this function). That means the retry may lose the injected SKILL.md instructions even though the user message is still in history. Preserve or recompute skillCtx for the compressed retry path so behavior is consistent under large histories.
| allSkills := cb.skillsLoader.ListSkills() | ||
| if len(allSkills) == 0 { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
MatchSkillsInMessage calls skillsLoader.ListSkills() on every message, which performs directory scans/stat calls across workspace/global/builtin roots. If this runs on constrained devices or with many skills, it can add noticeable per-request I/O overhead. Consider caching the skill name->path index (e.g., alongside the existing system prompt cache baseline / skill mtimes) and only rebuilding it when skill files change.
There was a problem hiding this comment.
ListSkills() is lightweight for typical setups (a few stat calls across < 20 skills). The system prompt cache already tracks skill file mtimes for invalidation, so adding a separate skill name cache would duplicate that logic for minimal gain. Better take up if profiling shows it's a bottleneck?
| func (cb *ContextBuilder) LoadSkillContext(skillNames []string) string { | ||
| if cb.skillsLoader == nil { | ||
| return "" | ||
| } | ||
| return cb.skillsLoader.LoadSkillsForContext(skillNames) |
There was a problem hiding this comment.
MatchSkillsInMessage returns SkillInfo.Name from SkillsLoader.ListSkills (which may come from SKILL.md frontmatter). SkillsLoader.LoadSkillsForContext/LoadSkill then tries to open //SKILL.md, so if the frontmatter name differs from the directory name the context load will silently fail (see pkg/skills/loader_test.go: TestListSkillsMetadataNameDedup where dir name != metadata name). Consider resolving matched skills to their SkillInfo.Path (or directory name derived from Path) when loading, or updating SkillsLoader.LoadSkill/LoadSkillsForContext to map metadata names to paths via ListSkills before reading files.
| func (cb *ContextBuilder) LoadSkillContext(skillNames []string) string { | |
| if cb.skillsLoader == nil { | |
| return "" | |
| } | |
| return cb.skillsLoader.LoadSkillsForContext(skillNames) | |
| // | |
| // The provided skillNames may be either: | |
| // - metadata names (from SKILL.md frontmatter), as returned by MatchSkillsInMessage, or | |
| // - directory names corresponding to the skill paths. | |
| // | |
| // To ensure correct loading even when metadata names differ from directory names, this | |
| // method resolves the inputs to directory names using the SkillsLoader's ListSkills data. | |
| func (cb *ContextBuilder) LoadSkillContext(skillNames []string) string { | |
| if cb.skillsLoader == nil || len(skillNames) == 0 { | |
| return "" | |
| } | |
| allSkills := cb.skillsLoader.ListSkills() | |
| if len(allSkills) == 0 { | |
| return "" | |
| } | |
| // Build a case-insensitive mapping from both metadata names and directory | |
| // names to the canonical directory name derived from SkillInfo.Path. | |
| nameToDir := make(map[string]string, len(allSkills)*2) | |
| for _, s := range allSkills { | |
| dirName := filepath.Base(s.Path) | |
| if dirName == "" { | |
| continue | |
| } | |
| lowerDir := strings.ToLower(dirName) | |
| nameToDir[lowerDir] = dirName | |
| if s.Name != "" { | |
| lowerMeta := strings.ToLower(s.Name) | |
| nameToDir[lowerMeta] = dirName | |
| } | |
| } | |
| resolved := make([]string, 0, len(skillNames)) | |
| for _, n := range skillNames { | |
| if n == "" { | |
| continue | |
| } | |
| key := strings.ToLower(n) | |
| if dir, ok := nameToDir[key]; ok { | |
| resolved = append(resolved, dir) | |
| } else { | |
| // Fall back to the original name to preserve existing behavior | |
| // for callers that already pass directory names or rely on the | |
| // previous direct pass-through behavior. | |
| resolved = append(resolved, n) | |
| } | |
| } | |
| if len(resolved) == 0 { | |
| return "" | |
| } | |
| return cb.skillsLoader.LoadSkillsForContext(resolved) |
|
Hi @anugotta , thanks for the PR! One concern from my side: String-based matching is fragile. Simple skill names ( |
Thanks @huaaudio, that's a fair point about string matching limitations. The reason I went with name matching is that this PR is specifically addressing the core problem in #1249 — models that don't reliably call tools on their own. The issue reporter notes that skills are "effectively broken for local models that don't independently decide to read SKILL.md files" and that "this impacts all Ollama-based setups." The system already tells the LLM to read_file the SKILL.md, but these models don't do it. I think an intent-based approach like a use_skill tool could have the same limitation since it still depends on the model deciding to make a tool call. Name matching works as a deterministic safety net for these models — zero latency, no extra LLM calls, and LoadSkillsForContext() was already designed to take skill names (it was implemented and tested but never wired up). That said, I completely agree that intent-based activation would be the better experience for more capable models. Would it make sense to keep this PR as the baseline fix for simpler models, and open a follow-up issue/PR for intent-based skill matching? The two approaches aren't mutually exclusive. |
Hi @anugotta , thanks for the reply! I was explicitly mentioning this because there is now an implementation of BM25 searching from recently merged PR #1243, which could be an alternative approach that gives us better results, and its code is made reusable in |
Hi @huaaudio , Got it! Thanks for letting me know about PR #1243 . I will take a look and revert. |
…s a skill Wire up the existing but unused LoadSkillsForContext() so that skill instructions are automatically injected into the system prompt when the user's message references an installed skill by name. Previously the LLM only saw a skills summary listing names and file paths, with instructions to "read the SKILL.md using read_file". Smaller and local models (e.g. Ollama) do not reliably decide to call read_file on their own, leaving skills effectively broken. Changes: - Add MatchSkillsInMessage() for case-insensitive whole-word matching of skill names against the user message - Add LoadSkillContext() wrapper to surface LoadSkillsForContext() - Extend BuildMessages() with a variadic skillContext parameter (backward-compatible — all existing callers are unchanged) - Inject matched skill content as a dynamic content block in the system message under "Active Skill Instructions" - Wire the matching and injection into runAgentLoop() before BuildMessages() is called - Add comprehensive tests for matching, boundary detection, context injection, and nil-safety Fixes sipeed#1249 Made-with: Cursor
Made-with: Cursor
- Preserve skill context during compression retry so SKILL.md instructions are not lost when context window is exceeded - Resolve frontmatter names to directory names in LoadSkillContext so skills whose metadata name differs from the directory name are loaded correctly - Add test for metadata name != directory name case Made-with: Cursor
anugotta
force-pushed
the
fix/skill-context-auto-injection
branch
from
62f556e to
e20bfd6
Compare
Hi @huaaudio I went through PR #1243 and BM25 implementation. Its cool and great help. Also read in PR #1243 desc: Not exactly sure if low end models would be smart enough to do this round-trip. I feel we should go ahead with the name matching approach to address the core problem in #1249 — "models that don't reliably call tools on their own". This is just my thought. If you think we should go ahead with the BM25 approach, I can look into that too. :) |
|
@anugotta Hi! This PR has had no activity for over 2 weeks, so I'm closing it for now to keep things organized. Feel free to reopen anytime if you'd like to continue. |
📝 Description
Wire up the existing but unused
LoadSkillsForContext()so that skill instructions (SKILL.md) are automatically injected into the LLM context when the user's message references an installed skill by name.Problem: The system prompt lists installed skills in an XML
<skills>summary and tells the LLM: "To use a skill, read its SKILL.md file using the read_file tool." However, smaller and local models (e.g. Ollama qwen3:14b) do not reliably decide to callread_fileon their own. The LLM only sees the skill name and path — never the actual instructions. This makes skills effectively broken for local model setups.Fix:
MatchSkillsInMessage()— case-insensitive whole-word matching of installed skill names against the user messageLoadSkillContext()— thin wrapper that calls the previously dead-codeLoadSkillsForContext()BuildMessages()with a variadicskillContextparameter (backward-compatible — all existing callers unchanged)runAgentLoop()beforeBuildMessages()is calledNote on Agent Refactor (#1216): This is a narrow fix that wires up existing dead code (
LoadSkillsForContextinpkg/skills/loader.go), not a new Agent abstraction or semantic expansion. No new concepts are introduced.🗣️ Type of Change
🤖 AI Code Generation
🔗 Related Issue
Fixes #1249
📚 Technical Context (Skip for Docs)
LoadSkillsForContext()inpkg/skills/loader.go:177was implemented and tested but never called from anywhere in the codebase. The context builder already generates a<skills>XML summary in the system prompt (Level 1), but the full SKILL.md content injection (Level 2) was dead code. This PR wires it up with message-level skill name matching so the LLM receives the actual skill instructions when a user references a skill.🧪 Test Environment
📸 Evidence (Optional)
Click to view Logs/Screenshots
all modules verified Run generate... Run generate complete ok github.com/sipeed/picoclaw/cmd/picoclaw 0.829s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal 1.381s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/agent 2.824s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/auth 2.033s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/cron 3.392s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/gateway 4.196s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/migrate 4.679s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/onboard 5.232s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/skills 5.271s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/status 5.335s ok github.com/sipeed/picoclaw/cmd/picoclaw/internal/version 5.247s ok github.com/sipeed/picoclaw/cmd/picoclaw-launcher/internal/server 5.304s ok github.com/sipeed/picoclaw/pkg/agent 6.418s ok github.com/sipeed/picoclaw/pkg/auth 5.137s ok github.com/sipeed/picoclaw/pkg/bus 5.052s ok github.com/sipeed/picoclaw/pkg/channels 18.259s ok github.com/sipeed/picoclaw/pkg/channels/discord 5.131s ok github.com/sipeed/picoclaw/pkg/channels/feishu 5.128s ok github.com/sipeed/picoclaw/pkg/channels/irc 5.124s ok github.com/sipeed/picoclaw/pkg/channels/matrix 4.909s ok github.com/sipeed/picoclaw/pkg/channels/slack 4.625s ok github.com/sipeed/picoclaw/pkg/channels/telegram 5.353s ok github.com/sipeed/picoclaw/pkg/channels/wecom 5.489s ok github.com/sipeed/picoclaw/pkg/channels/whatsapp 5.222s ok github.com/sipeed/picoclaw/pkg/commands 5.148s ok github.com/sipeed/picoclaw/pkg/config 5.199s ok github.com/sipeed/picoclaw/pkg/cron 4.531s ok github.com/sipeed/picoclaw/pkg/heartbeat 3.301s ok github.com/sipeed/picoclaw/pkg/identity 3.634s ok github.com/sipeed/picoclaw/pkg/logger 4.057s ok github.com/sipeed/picoclaw/pkg/mcp 4.021s ok github.com/sipeed/picoclaw/pkg/media 4.736s ok github.com/sipeed/picoclaw/pkg/memory 9.930s ok github.com/sipeed/picoclaw/pkg/migrate 4.875s ok github.com/sipeed/picoclaw/pkg/migrate/internal 4.756s ok github.com/sipeed/picoclaw/pkg/migrate/sources/openclaw 5.229s ok github.com/sipeed/picoclaw/pkg/providers 20.150s ok github.com/sipeed/picoclaw/pkg/providers/anthropic 4.491s ok github.com/sipeed/picoclaw/pkg/providers/openai_compat 4.578s ok github.com/sipeed/picoclaw/pkg/routing 3.675s ok github.com/sipeed/picoclaw/pkg/session 3.655s ok github.com/sipeed/picoclaw/pkg/skills 9.172s ok github.com/sipeed/picoclaw/pkg/state 4.545s ok github.com/sipeed/picoclaw/pkg/tools 13.302s ok github.com/sipeed/picoclaw/pkg/utils 3.660s ok github.com/sipeed/picoclaw/pkg/voice 3.956s
☑️ Checklist