diff --git a/internal/agent/agentic_fetch_tool.go b/internal/agent/agentic_fetch_tool.go index ffbe0f49e45c259db3f0bba9f07fda771ad3ecd4..03a3e116ebd5ff3784a67bbcf3304d2ce515ad1e 100644 --- a/internal/agent/agentic_fetch_tool.go +++ b/internal/agent/agentic_fetch_tool.go @@ -169,7 +169,7 @@ func (c *coordinator) agenticFetchTool(_ context.Context, client *http.Client) ( tools.NewGlobTool(tmpDir), tools.NewGrepTool(tmpDir, c.cfg.Config().Tools.Grep), tools.NewSourcegraphTool(client), - tools.NewViewTool(c.lspManager, c.permissions, c.filetracker, tmpDir), + tools.NewViewTool(c.lspManager, c.permissions, c.filetracker, nil, tmpDir), } agent := NewSessionAgent(SessionAgentOptions{ diff --git a/internal/agent/common_test.go b/internal/agent/common_test.go index 5fddc3c10f7c68986ec64b6ccd9bdf10a1a6de91..d9d173c49a73521f3e7cd785385901acf5b75159 100644 --- a/internal/agent/common_test.go +++ b/internal/agent/common_test.go @@ -217,7 +217,7 @@ func coderAgent(r *vcr.Recorder, env fakeEnv, large, small fantasy.LanguageModel tools.NewGrepTool(env.workingDir, cfg.Config().Tools.Grep), tools.NewLsTool(env.permissions, env.workingDir, cfg.Config().Tools.Ls), tools.NewSourcegraphTool(r.GetDefaultClient()), - tools.NewViewTool(nil, env.permissions, *env.filetracker, env.workingDir), + tools.NewViewTool(nil, env.permissions, *env.filetracker, nil, env.workingDir), tools.NewWriteTool(nil, env.permissions, env.history, *env.filetracker, env.workingDir), } diff --git a/internal/agent/coordinator.go b/internal/agent/coordinator.go index cd7a0b411e03f6270b66cafd3548bbf8e224ce10..1130438a61217b3491bd21e388376f303631e9ec 100644 --- a/internal/agent/coordinator.go +++ b/internal/agent/coordinator.go @@ -25,6 +25,7 @@ import ( "github.com/charmbracelet/crush/internal/config" "github.com/charmbracelet/crush/internal/filetracker" "github.com/charmbracelet/crush/internal/history" + "github.com/charmbracelet/crush/internal/home" "github.com/charmbracelet/crush/internal/log" "github.com/charmbracelet/crush/internal/lsp" "github.com/charmbracelet/crush/internal/message" @@ -32,6 +33,7 @@ import ( "github.com/charmbracelet/crush/internal/permission" "github.com/charmbracelet/crush/internal/pubsub" "github.com/charmbracelet/crush/internal/session" + "github.com/charmbracelet/crush/internal/skills" "golang.org/x/sync/errgroup" "charm.land/fantasy/providers/anthropic" @@ -87,6 +89,11 @@ type coordinator struct { currentAgent SessionAgent agents map[string]SessionAgent + // Skills discovery results (session-start snapshot). + allSkills []*skills.Skill // Pre-filter: all discovered after dedup. + activeSkills []*skills.Skill // Post-filter: active skills only. + skillTracker *skills.Tracker + readyWg errgroup.Group } @@ -101,16 +108,23 @@ func NewCoordinator( lspManager *lsp.Manager, notify pubsub.Publisher[notify.Notification], ) (Coordinator, error) { + // Discover skills once at session start. + allSkills, activeSkills := discoverSkills(cfg) + skillTracker := skills.NewTracker(activeSkills) + c := &coordinator{ - cfg: cfg, - sessions: sessions, - messages: messages, - permissions: permissions, - history: history, - filetracker: filetracker, - lspManager: lspManager, - notify: notify, - agents: make(map[string]SessionAgent), + cfg: cfg, + sessions: sessions, + messages: messages, + permissions: permissions, + history: history, + filetracker: filetracker, + lspManager: lspManager, + notify: notify, + agents: make(map[string]SessionAgent), + allSkills: allSkills, + activeSkills: activeSkills, + skillTracker: skillTracker, } agentCfg, ok := cfg.Config().Agents[config.AgentCoder] @@ -450,7 +464,7 @@ func (c *coordinator) buildTools(ctx context.Context, agent config.Agent) ([]fan allTools = append(allTools, tools.NewBashTool(c.permissions, c.cfg.WorkingDir(), c.cfg.Config().Options.Attribution, modelName), - tools.NewCrushInfoTool(c.cfg, c.lspManager), + tools.NewCrushInfoTool(c.cfg, c.lspManager, c.allSkills, c.activeSkills, c.skillTracker), tools.NewCrushLogsTool(logFile), tools.NewJobOutputTool(), tools.NewJobKillTool(), @@ -463,7 +477,7 @@ func (c *coordinator) buildTools(ctx context.Context, agent config.Agent) ([]fan tools.NewLsTool(c.permissions, c.cfg.WorkingDir(), c.cfg.Config().Tools.Ls), tools.NewSourcegraphTool(nil), tools.NewTodosTool(c.sessions), - tools.NewViewTool(c.lspManager, c.permissions, c.filetracker, c.cfg.WorkingDir(), c.cfg.Config().Options.SkillsPaths...), + tools.NewViewTool(c.lspManager, c.permissions, c.filetracker, c.skillTracker, c.cfg.WorkingDir(), c.cfg.Config().Options.SkillsPaths...), tools.NewWriteTool(c.lspManager, c.permissions, c.history, c.filetracker, c.cfg.WorkingDir()), ) @@ -1043,3 +1057,32 @@ func (c *coordinator) updateParentSessionCost(ctx context.Context, childSessionI return nil } + +// discoverSkills runs the skill discovery pipeline and returns both the +// pre-filter (all discovered, after dedup) and post-filter (active) lists. +func discoverSkills(cfg *config.ConfigStore) (allSkills, activeSkills []*skills.Skill) { + discovered := skills.DiscoverBuiltin() + + opts := cfg.Config().Options + if opts != nil && len(opts.SkillsPaths) > 0 { + expandedPaths := make([]string, 0, len(opts.SkillsPaths)) + for _, pth := range opts.SkillsPaths { + expanded := home.Long(pth) + if strings.HasPrefix(expanded, "$") { + if resolved, err := cfg.Resolver().ResolveValue(expanded); err == nil { + expanded = resolved + } + } + expandedPaths = append(expandedPaths, expanded) + } + discovered = append(discovered, skills.Discover(expandedPaths)...) + } + + allSkills = skills.Deduplicate(discovered) + var disabledSkills []string + if opts != nil { + disabledSkills = opts.DisabledSkills + } + activeSkills = skills.Filter(allSkills, disabledSkills) + return allSkills, activeSkills +} diff --git a/internal/agent/tools/crush_info.go b/internal/agent/tools/crush_info.go index bbc96b60954273fe4342d5634529f608c8b7dead..9491725eeacea68226a5bfd037ff75c2b8f922f7 100644 --- a/internal/agent/tools/crush_info.go +++ b/internal/agent/tools/crush_info.go @@ -11,6 +11,7 @@ import ( "github.com/charmbracelet/crush/internal/agent/tools/mcp" "github.com/charmbracelet/crush/internal/config" "github.com/charmbracelet/crush/internal/lsp" + "github.com/charmbracelet/crush/internal/skills" ) const CrushInfoToolName = "crush_info" @@ -23,16 +24,19 @@ type CrushInfoParams struct{} func NewCrushInfoTool( cfg *config.ConfigStore, lspManager *lsp.Manager, + allSkills []*skills.Skill, + activeSkills []*skills.Skill, + skillTracker *skills.Tracker, ) fantasy.AgentTool { return fantasy.NewAgentTool( CrushInfoToolName, string(crushInfoDescription), func(ctx context.Context, _ CrushInfoParams, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { - return fantasy.NewTextResponse(buildCrushInfo(cfg, lspManager)), nil + return fantasy.NewTextResponse(buildCrushInfo(cfg, lspManager, allSkills, activeSkills, skillTracker)), nil }) } -func buildCrushInfo(cfg *config.ConfigStore, lspManager *lsp.Manager) string { +func buildCrushInfo(cfg *config.ConfigStore, lspManager *lsp.Manager, allSkills []*skills.Skill, activeSkills []*skills.Skill, skillTracker *skills.Tracker) string { var b strings.Builder writeConfigFiles(&b, cfg) @@ -41,6 +45,7 @@ func buildCrushInfo(cfg *config.ConfigStore, lspManager *lsp.Manager) string { writeProviders(&b, cfg) writeLSP(&b, lspManager, cfg) writeMCP(&b, mcp.GetStates(), cfg) + writeSkills(&b, allSkills, activeSkills, skillTracker, cfg) writePermissions(&b, cfg) writeDisabledTools(&b, cfg) writeOptions(&b, cfg) @@ -284,6 +289,59 @@ func writeMCP(b *strings.Builder, states map[string]mcp.ClientInfo, cfg *config. } } +func writeSkills(b *strings.Builder, allSkills []*skills.Skill, activeSkills []*skills.Skill, tracker *skills.Tracker, cfg *config.ConfigStore) { + var disabled []string + if cfg.Config().Options != nil { + disabled = cfg.Config().Options.DisabledSkills + } + if len(activeSkills) == 0 && len(disabled) == 0 { + return + } + + // Build origin map from the pre-filter list. + originMap := make(map[string]string, len(allSkills)) + for _, s := range allSkills { + if s.Builtin { + originMap[s.Name] = "builtin" + } else { + originMap[s.Name] = "user" + } + } + + type entry struct { + name string + origin string + state string + } + var entries []entry + + // Active skills: loaded or unloaded. + for _, s := range activeSkills { + state := "unloaded" + if tracker.IsLoaded(s.Name) { + state = "loaded" + } + origin := originMap[s.Name] + entries = append(entries, entry{name: s.Name, origin: origin, state: state}) + } + + // Disabled skills. + for _, name := range disabled { + origin := originMap[name] + if origin == "" { + origin = "user" + } + entries = append(entries, entry{name: name, origin: origin, state: "disabled"}) + } + + slices.SortFunc(entries, func(a, b entry) int { return strings.Compare(a.name, b.name) }) + b.WriteString("[skills]\n") + for _, e := range entries { + fmt.Fprintf(b, "%s = %s, %s\n", e.name, e.origin, e.state) + } + b.WriteString("\n") +} + func writePermissions(b *strings.Builder, cfg *config.ConfigStore) { c := cfg.Config() overrides := cfg.Overrides() diff --git a/internal/agent/tools/crush_info.md b/internal/agent/tools/crush_info.md index e4e0d923a824673d6e0c8083d6d03c62244e9d96..ade1c1d57ba64bd64c7085775ba3c9dbbdcef764 100644 --- a/internal/agent/tools/crush_info.md +++ b/internal/agent/tools/crush_info.md @@ -2,8 +2,8 @@ Get information about Crush's current runtime configuration and service state. -- Shows active model and provider, LSP/MCP server status, permissions mode, - disabled tools, and key options +- Shows active model and provider, LSP/MCP server status, skills, + permissions mode, disabled tools, and key options - Use when diagnosing why something isn't working (missing diagnostics, provider errors, MCP disconnections) - No parameters needed — always returns the full current state @@ -12,5 +12,7 @@ state. - Check [lsp] and [mcp] sections for service health - Check [providers] to see which providers are enabled and available +- Check [skills] to see which skills are available and whether they have been + loaded this session - Pair with the crush-config skill to fix configuration issues diff --git a/internal/agent/tools/crush_info_test.go b/internal/agent/tools/crush_info_test.go index 83b14884b1bab4f7d41f30e7b6c3c85794a456cc..5ec0f59001e25739bc5d22ebd0df1329f7c129a1 100644 --- a/internal/agent/tools/crush_info_test.go +++ b/internal/agent/tools/crush_info_test.go @@ -13,6 +13,7 @@ import ( "github.com/charmbracelet/crush/internal/config" "github.com/charmbracelet/crush/internal/csync" "github.com/charmbracelet/crush/internal/lsp" + "github.com/charmbracelet/crush/internal/skills" "github.com/stretchr/testify/require" ) @@ -22,7 +23,7 @@ func TestCrushInfo_MinimalConfig(t *testing.T) { cfg := config.NewTestStore(&config.Config{ Providers: csync.NewMap[string, config.ProviderConfig](), }) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.NotContains(t, output, "[providers]") require.NotContains(t, output, "[lsp]") require.NotContains(t, output, "[mcp]") @@ -38,7 +39,7 @@ func TestCrushInfo_ConfigFiles(t *testing.T) { "/home/user/.config/crush/crush.json", "/project/.crush/crush.json", ) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[config_files]") require.Contains(t, output, "/home/user/.config/crush/crush.json") require.Contains(t, output, "/project/.crush/crush.json") @@ -54,7 +55,7 @@ func TestCrushInfo_Models(t *testing.T) { }, Providers: csync.NewMap[string, config.ProviderConfig](), }) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[model]") require.Contains(t, output, "large = claude-sonnet-4-20250514 (anthropic)") require.Contains(t, output, "small = claude-haiku-3-20250307 (anthropic)") @@ -68,7 +69,7 @@ func TestCrushInfo_Providers(t *testing.T) { providers.Set("anthropic", config.ProviderConfig{Models: make([]catwalk.Model, 12)}) cfg := config.NewTestStore(&config.Config{Providers: providers}) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[providers]") anthropicIdx := strings.Index(output, "anthropic = enabled") openaiIdx := strings.Index(output, "openai = enabled") @@ -87,7 +88,7 @@ func TestCrushInfo_DisabledProvidersOmitted(t *testing.T) { providers.Set("anthropic", config.ProviderConfig{Models: make([]catwalk.Model, 12)}) cfg := config.NewTestStore(&config.Config{Providers: providers}) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "anthropic = enabled") require.NotContains(t, output, "openai") } @@ -107,7 +108,7 @@ func TestCrushInfo_LSPStates(t *testing.T) { mgr.Clients().Set("pyright", errorClient) cfg := config.NewTestStore(&config.Config{Providers: csync.NewMap[string, config.ProviderConfig]()}) - output := buildCrushInfo(cfg, mgr) + output := buildCrushInfo(cfg, mgr, nil, nil, nil) require.Contains(t, output, "[lsp]") require.Contains(t, output, "gopls = ready") require.Contains(t, output, "pyright = error") @@ -158,7 +159,7 @@ func TestCrushInfo_YoloMode(t *testing.T) { }) cfg.Overrides().SkipPermissionRequests = true - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[permissions]") require.Contains(t, output, "mode = yolo") } @@ -171,7 +172,7 @@ func TestCrushInfo_AllowedTools(t *testing.T) { Permissions: &config.Permissions{AllowedTools: []string{"edit:write", "bash"}}, }) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[permissions]") require.Contains(t, output, "allowed_tools = bash, edit:write") } @@ -184,7 +185,7 @@ func TestCrushInfo_DisabledTools(t *testing.T) { Options: &config.Options{DisabledTools: []string{"sourcegraph", "agentic_fetch"}}, }) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[tools]") require.Contains(t, output, "disabled = agentic_fetch, sourcegraph") } @@ -201,7 +202,7 @@ func TestCrushInfo_Options(t *testing.T) { }, }) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.Contains(t, output, "[options]") require.Contains(t, output, "auto_lsp = true") require.Contains(t, output, "auto_summarize = false") @@ -216,14 +217,14 @@ func TestCrushInfo_AutoSummarizeInversion(t *testing.T) { Providers: csync.NewMap[string, config.ProviderConfig](), Options: &config.Options{DisableAutoSummarize: true}, }) - outputFalse := buildCrushInfo(cfgFalse, nil) + outputFalse := buildCrushInfo(cfgFalse, nil, nil, nil, nil) require.Contains(t, outputFalse, "auto_summarize = false") cfgTrue := config.NewTestStore(&config.Config{ Providers: csync.NewMap[string, config.ProviderConfig](), Options: &config.Options{DisableAutoSummarize: false}, }) - outputTrue := buildCrushInfo(cfgTrue, nil) + outputTrue := buildCrushInfo(cfgTrue, nil, nil, nil, nil) require.Contains(t, outputTrue, "auto_summarize = true") } @@ -237,7 +238,7 @@ func TestCrushInfo_NoSecrets(t *testing.T) { }) cfg := config.NewTestStore(&config.Config{Providers: providers}) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.NotContains(t, output, "sk-super-secret-key-12345") require.NotContains(t, output, "secret") require.Contains(t, output, "openai = enabled (8 models)") @@ -273,7 +274,7 @@ func TestCrushInfo_DeterministicOrdering(t *testing.T) { zMcpIdx := strings.Index(mcpOutput, "z-mcp = connected") require.Less(t, aMcpIdx, zMcpIdx) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) alphaIdx := strings.Index(output, "alpha = enabled") middleIdx := strings.Index(output, "middle = enabled") @@ -294,11 +295,12 @@ func TestCrushInfo_EmptySectionsOmitted(t *testing.T) { Options: &config.Options{}, }) - output := buildCrushInfo(cfg, nil) + output := buildCrushInfo(cfg, nil, nil, nil, nil) require.NotContains(t, output, "[tools]") require.NotContains(t, output, "[permissions]") require.NotContains(t, output, "[lsp]") require.NotContains(t, output, "[mcp]") + require.NotContains(t, output, "[skills]") } func TestCrushInfo_ConfigStaleness_Clean(t *testing.T) { @@ -315,7 +317,7 @@ func TestCrushInfo_ConfigStaleness_Clean(t *testing.T) { // Capture snapshot (normally done in Load) store.CaptureStalenessSnapshot([]string{configPath}) - output := buildCrushInfo(store, nil) + output := buildCrushInfo(store, nil, nil, nil, nil) require.Contains(t, output, "[config]") require.Contains(t, output, "dirty = false") require.NotContains(t, output, "changed_paths") @@ -340,7 +342,7 @@ func TestCrushInfo_ConfigStaleness_Dirty(t *testing.T) { time.Sleep(10 * time.Millisecond) require.NoError(t, os.WriteFile(configPath, []byte(`{"debug": true}`), 0o600)) - output := buildCrushInfo(store, nil) + output := buildCrushInfo(store, nil, nil, nil, nil) require.Contains(t, output, "[config]") require.Contains(t, output, "dirty = true") require.Contains(t, output, "changed_paths") @@ -364,9 +366,110 @@ func TestCrushInfo_ConfigStaleness_MissingPath(t *testing.T) { // Delete file to trigger missing state require.NoError(t, os.Remove(configPath)) - output := buildCrushInfo(store, nil) + output := buildCrushInfo(store, nil, nil, nil, nil) require.Contains(t, output, "[config]") require.Contains(t, output, "dirty = true") require.Contains(t, output, "missing_paths") require.Contains(t, output, configPath) } + +func TestCrushInfo_Skills_NoSkills(t *testing.T) { + t.Parallel() + + cfg := config.NewTestStore(&config.Config{ + Providers: csync.NewMap[string, config.ProviderConfig](), + }) + output := buildCrushInfo(cfg, nil, nil, nil, nil) + require.NotContains(t, output, "[skills]") +} + +func TestCrushInfo_Skills_MixedLoadedUnloaded(t *testing.T) { + t.Parallel() + + allSkills := []*skills.Skill{ + {Name: "go-doc", Builtin: false}, + {Name: "bash", Builtin: false}, + {Name: "crush-config", Builtin: true}, + } + activeSkills := allSkills + + tracker := skills.NewTracker(activeSkills) + tracker.MarkLoaded("bash") + tracker.MarkLoaded("crush-config") + + cfg := config.NewTestStore(&config.Config{ + Providers: csync.NewMap[string, config.ProviderConfig](), + }) + output := buildCrushInfo(cfg, nil, allSkills, activeSkills, tracker) + require.Contains(t, output, "[skills]") + require.Contains(t, output, "bash = user, loaded") + require.Contains(t, output, "crush-config = builtin, loaded") + require.Contains(t, output, "go-doc = user, unloaded") +} + +func TestCrushInfo_Skills_DisabledSkills(t *testing.T) { + t.Parallel() + + allSkills := []*skills.Skill{ + {Name: "bash", Builtin: false}, + {Name: "crush-config", Builtin: true}, + {Name: "image-convert", Builtin: false}, + } + activeSkills := []*skills.Skill{ + {Name: "bash", Builtin: false}, + {Name: "crush-config", Builtin: true}, + } + + tracker := skills.NewTracker(activeSkills) + + cfg := config.NewTestStore(&config.Config{ + Providers: csync.NewMap[string, config.ProviderConfig](), + Options: &config.Options{DisabledSkills: []string{"image-convert"}}, + }) + output := buildCrushInfo(cfg, nil, allSkills, activeSkills, tracker) + require.Contains(t, output, "[skills]") + require.Contains(t, output, "bash = user, unloaded") + require.Contains(t, output, "crush-config = builtin, unloaded") + require.Contains(t, output, "image-convert = user, disabled") +} + +func TestCrushInfo_Skills_Ordering(t *testing.T) { + t.Parallel() + + allSkills := []*skills.Skill{ + {Name: "z-skill", Builtin: false}, + {Name: "a-skill", Builtin: true}, + {Name: "m-skill", Builtin: false}, + } + activeSkills := allSkills + tracker := skills.NewTracker(activeSkills) + + cfg := config.NewTestStore(&config.Config{ + Providers: csync.NewMap[string, config.ProviderConfig](), + }) + output := buildCrushInfo(cfg, nil, allSkills, activeSkills, tracker) + + aIdx := strings.Index(output, "a-skill") + mIdx := strings.Index(output, "m-skill") + zIdx := strings.Index(output, "z-skill") + require.Less(t, aIdx, mIdx) + require.Less(t, mIdx, zIdx) +} + +func TestCrushInfo_Skills_BuiltinOrigin(t *testing.T) { + t.Parallel() + + allSkills := []*skills.Skill{ + {Name: "crush-config", Builtin: true}, + {Name: "my-skill", Builtin: false}, + } + activeSkills := allSkills + tracker := skills.NewTracker(activeSkills) + + cfg := config.NewTestStore(&config.Config{ + Providers: csync.NewMap[string, config.ProviderConfig](), + }) + output := buildCrushInfo(cfg, nil, allSkills, activeSkills, tracker) + require.Contains(t, output, "crush-config = builtin, unloaded") + require.Contains(t, output, "my-skill = user, unloaded") +} diff --git a/internal/agent/tools/crush_logs_test.go b/internal/agent/tools/crush_logs_test.go index 6ab5e5e32e569b683978624d0ddfccba8c04b52d..0a4c95345900d674c84df3fb8a0ac00e552cec38 100644 --- a/internal/agent/tools/crush_logs_test.go +++ b/internal/agent/tools/crush_logs_test.go @@ -3,6 +3,7 @@ package tools import ( "encoding/json" "fmt" + "maps" "os" "path/filepath" "strings" @@ -43,9 +44,7 @@ func makeLogEntry(level, msg, source string, line int, extra map[string]any) map "line": line, }, } - for k, v := range extra { - entry[k] = v - } + maps.Copy(entry, extra) return entry } @@ -88,7 +87,7 @@ func TestCrushLogs_DefaultLines(t *testing.T) { t.Parallel() // Create 100 log entries. var entries []map[string]any - for i := 0; i < 100; i++ { + for i := range 100 { entries = append(entries, makeLogEntry("INFO", fmt.Sprintf("Entry %d", i), "app.go", i, nil)) } @@ -109,7 +108,7 @@ func TestCrushLogs_MaxCap(t *testing.T) { t.Parallel() // Create 200 log entries. var entries []map[string]any - for i := 0; i < 200; i++ { + for i := range 200 { entries = append(entries, makeLogEntry("INFO", fmt.Sprintf("Entry %d", i), "app.go", i, nil)) } @@ -350,7 +349,7 @@ func TestCrushLogs_PartialTrailingLine(t *testing.T) { require.NoError(t, err) // Create valid entries. - for i := 0; i < 5; i++ { + for i := range 5 { entry := makeLogEntry("INFO", fmt.Sprintf("Entry %d", i), "app.go", i, nil) line, _ := json.Marshal(entry) file.WriteString(string(line) + "\n") diff --git a/internal/agent/tools/view.go b/internal/agent/tools/view.go index 01b405056eaad03a0328f23bcb28cb94c975ea26..dc05e53bc1eb9e947e994827886f57971be4219e 100644 --- a/internal/agent/tools/view.go +++ b/internal/agent/tools/view.go @@ -63,6 +63,7 @@ func NewViewTool( lspManager *lsp.Manager, permissions permission.Service, filetracker filetracker.Service, + skillTracker *skills.Tracker, workingDir string, skillsPaths ...string, ) fantasy.AgentTool { @@ -76,7 +77,8 @@ func NewViewTool( // Handle builtin skill files (crush: prefix). if strings.HasPrefix(params.FilePath, skills.BuiltinPrefix) { - return readBuiltinFile(params) + resp, err := readBuiltinFile(params, skillTracker) + return resp, err } // Handle relative paths @@ -222,6 +224,7 @@ func NewViewTool( meta.ResourceType = ViewResourceSkill meta.ResourceName = skill.Name meta.ResourceDescription = skill.Description + skillTracker.MarkLoaded(skill.Name) } } @@ -381,7 +384,7 @@ func isInSkillsPath(filePath string, skillsPaths []string) bool { } // readBuiltinFile reads a file from the embedded builtin skills filesystem. -func readBuiltinFile(params ViewParams) (fantasy.ToolResponse, error) { +func readBuiltinFile(params ViewParams, skillTracker *skills.Tracker) (fantasy.ToolResponse, error) { embeddedPath := "builtin/" + strings.TrimPrefix(params.FilePath, skills.BuiltinPrefix) builtinFS := skills.BuiltinFS() @@ -425,6 +428,7 @@ func readBuiltinFile(params ViewParams) (fantasy.ToolResponse, error) { meta.ResourceType = ViewResourceSkill meta.ResourceName = skill.Name meta.ResourceDescription = skill.Description + skillTracker.MarkLoaded(skill.Name) } return fantasy.WithResponseMetadata( diff --git a/internal/agent/tools/view_test.go b/internal/agent/tools/view_test.go index c821f9e2eabd697acef442dd900d93524e4f9d7f..3293e0202c57a2990e280727ad8df9fea101ee22 100644 --- a/internal/agent/tools/view_test.go +++ b/internal/agent/tools/view_test.go @@ -95,7 +95,7 @@ func TestReadBuiltinFile(t *testing.T) { resp, err := readBuiltinFile(ViewParams{ FilePath: "crush://skills/crush-config/SKILL.md", - }) + }, nil) require.NoError(t, err) require.NotEmpty(t, resp.Content) require.Contains(t, resp.Content, "Crush Configuration") @@ -106,7 +106,7 @@ func TestReadBuiltinFile(t *testing.T) { resp, err := readBuiltinFile(ViewParams{ FilePath: "crush://skills/nonexistent/SKILL.md", - }) + }, nil) require.NoError(t, err) require.True(t, resp.IsError) }) @@ -116,7 +116,7 @@ func TestReadBuiltinFile(t *testing.T) { resp, err := readBuiltinFile(ViewParams{ FilePath: "crush://skills/crush-config/SKILL.md", - }) + }, nil) require.NoError(t, err) var meta ViewResponseMetadata @@ -132,7 +132,7 @@ func TestReadBuiltinFile(t *testing.T) { resp, err := readBuiltinFile(ViewParams{ FilePath: "crush://skills/crush-config/SKILL.md", Offset: 5, - }) + }, nil) require.NoError(t, err) require.NotContains(t, resp.Content, " 1|") }) diff --git a/internal/skills/tracker.go b/internal/skills/tracker.go new file mode 100644 index 0000000000000000000000000000000000000000..4910d8390e6a95ebe3974d43a802e9ec6d82164e --- /dev/null +++ b/internal/skills/tracker.go @@ -0,0 +1,53 @@ +package skills + +import "sync" + +// Tracker tracks which skills have been loaded (read) during a session. +// It is safe for concurrent use. +// +// Note: Tracking is name-based and limited to active skills only. If a builtin +// skill is overridden by a user skill, only the user skill (which is active) +// can be marked as loaded. This prevents misattribution when reading builtin +// files that have been overridden. +type Tracker struct { + mu sync.RWMutex + loaded map[string]bool + activeNames map[string]bool // Set of active skill names (post-dedup, post-filter) +} + +// NewTracker creates a new skill tracker with the given active skill names. +// Only skills in activeSkills can be marked as loaded. +func NewTracker(activeSkills []*Skill) *Tracker { + activeNames := make(map[string]bool, len(activeSkills)) + for _, s := range activeSkills { + activeNames[s.Name] = true + } + return &Tracker{ + loaded: make(map[string]bool), + activeNames: activeNames, + } +} + +// MarkLoaded marks a skill as having been loaded. +// Only marks as loaded if the skill is in the active set (not overridden/disabled). +func (t *Tracker) MarkLoaded(name string) { + if t == nil { + return + } + t.mu.Lock() + defer t.mu.Unlock() + // Only track if this skill is actually active (not overridden by user skill). + if t.activeNames[name] { + t.loaded[name] = true + } +} + +// IsLoaded returns true if the skill has been loaded. +func (t *Tracker) IsLoaded(name string) bool { + if t == nil { + return false + } + t.mu.RLock() + defer t.mu.RUnlock() + return t.loaded[name] +} diff --git a/internal/skills/tracker_test.go b/internal/skills/tracker_test.go new file mode 100644 index 0000000000000000000000000000000000000000..4919a731ad83d81371bfa829418bb2e02d6ea2e4 --- /dev/null +++ b/internal/skills/tracker_test.go @@ -0,0 +1,102 @@ +package skills + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func TestTracker_MarkLoadedAndIsLoaded(t *testing.T) { + t.Parallel() + + activeSkills := []*Skill{ + {Name: "go-doc"}, + {Name: "bash"}, + } + tracker := NewTracker(activeSkills) + + // Initially not loaded. + require.False(t, tracker.IsLoaded("go-doc")) + require.False(t, tracker.IsLoaded("bash")) + + // Mark as loaded. + tracker.MarkLoaded("go-doc") + require.True(t, tracker.IsLoaded("go-doc")) + require.False(t, tracker.IsLoaded("bash")) + + // Mark another. + tracker.MarkLoaded("bash") + require.True(t, tracker.IsLoaded("go-doc")) + require.True(t, tracker.IsLoaded("bash")) +} + +func TestTracker_NonActiveSkillCannotBeMarkedLoaded(t *testing.T) { + t.Parallel() + + activeSkills := []*Skill{ + {Name: "go-doc"}, + } + tracker := NewTracker(activeSkills) + + // Cannot mark non-active skill as loaded. + tracker.MarkLoaded("bash") + require.False(t, tracker.IsLoaded("bash")) + + // Can mark active skill as loaded. + tracker.MarkLoaded("go-doc") + require.True(t, tracker.IsLoaded("go-doc")) +} + +func TestTracker_NilSafety(t *testing.T) { + t.Parallel() + + var tracker *Tracker + + // Should not panic. + tracker.MarkLoaded("go-doc") + require.False(t, tracker.IsLoaded("go-doc")) +} + +func TestTracker_BuiltinSkillTracking(t *testing.T) { + t.Parallel() + + // Simulate active skills including a builtin skill (crush-config). + activeSkills := []*Skill{ + {Name: "crush-config", Description: "Crush config", Builtin: true}, + {Name: "go-doc", Description: "Go docs", Builtin: false}, + } + tracker := NewTracker(activeSkills) + + // Initially not loaded. + require.False(t, tracker.IsLoaded("crush-config")) + require.False(t, tracker.IsLoaded("go-doc")) + + // Mark builtin skill as loaded (simulating read via crush://...). + tracker.MarkLoaded("crush-config") + require.True(t, tracker.IsLoaded("crush-config")) + + // Mark user skill as loaded. + tracker.MarkLoaded("go-doc") + require.True(t, tracker.IsLoaded("go-doc")) +} + +func TestTracker_OverriddenBuiltinNotTracked(t *testing.T) { + t.Parallel() + + // Simulate scenario where builtin "bash" is overridden by user "bash". + // After dedup, only user "bash" is active. + activeSkills := []*Skill{ + {Name: "bash", Description: "User bash override", Builtin: false}, + } + tracker := NewTracker(activeSkills) + + // Trying to mark the builtin "bash" as loaded should not work + // because the active skill is the user override. + tracker.MarkLoaded("bash") + require.True(t, tracker.IsLoaded("bash")) + + // But if we somehow tried to mark a different builtin that's not active, + // it wouldn't get marked. + tracker.MarkLoaded("nonexistent-builtin") + require.False(t, tracker.IsLoaded("nonexistent-builtin")) +}