diff --git a/.github/cla-signatures.json b/.github/cla-signatures.json index 7d761c9d27cc6c4148f121ff3a8b101beb0ed905..ea07a185b6d6ddaca3f235c0a7a354b63dba88d2 100644 --- a/.github/cla-signatures.json +++ b/.github/cla-signatures.json @@ -1807,6 +1807,14 @@ "created_at": "2026-05-19T03:37:32Z", "repoId": 987670088, "pullRequestNo": 2954 + }, + { + "name": "mei2jun1", + "id": 285962643, + "comment_id": 4495440988, + "created_at": "2026-05-20T06:53:43Z", + "repoId": 987670088, + "pullRequestNo": 2964 } ] } \ No newline at end of file diff --git a/README.md b/README.md index 3221ab9d3687d6679337a354823c70c8eb680900..f3ddaf7afa23014f3b0bb75297e0cac3da81318b 100644 --- a/README.md +++ b/README.md @@ -525,6 +525,37 @@ git clone https://github.com/anthropics/skills.git _temp mv _temp/skills/* . ; rm -r -force _temp ``` +#### User-Invocable Skills + +Skills can be made invocable as commands from the commands palette (Ctrl+P). Add `user-invocable: true` to the skill's YAML frontmatter: + +```yaml +--- +name: my-skill +description: A skill that can be invoked as a command. +user-invocable: true +--- +``` + +User-invocable skills appear in the commands palette with a `user:` or `project:` prefix: +- Skills from global directories show as `user:skill-name` +- Skills from project directories show as `project:skill-name` + +When invoked, the skill's instructions are loaded into the conversation context. + +To prevent the model from auto-triggering a skill (while still allowing user invocation), add `disable-model-invocation: true`: + +```yaml +--- +name: my-skill +description: Only invocable by users, not the model. +user-invocable: true +disable-model-invocation: true +--- +``` + +Skills with `disable-model-invocation` won't appear in the model's available skills list but can still be invoked manually by users. + ### Desktop notifications Crush sends desktop notifications when a tool call requires permission and when diff --git a/internal/agent/coordinator.go b/internal/agent/coordinator.go index 2b750a1c29d4f7ef98cb3900c886901683e19eb2..145665eee8d3bca81d054e8796d3ab80f4d85dd9 100644 --- a/internal/agent/coordinator.go +++ b/internal/agent/coordinator.go @@ -26,7 +26,6 @@ import ( "github.com/charmbracelet/crush/internal/event" "github.com/charmbracelet/crush/internal/filetracker" "github.com/charmbracelet/crush/internal/history" - "github.com/charmbracelet/crush/internal/home" "github.com/charmbracelet/crush/internal/hooks" "github.com/charmbracelet/crush/internal/log" "github.com/charmbracelet/crush/internal/lsp" @@ -118,9 +117,19 @@ func NewCoordinator( filetracker filetracker.Service, lspManager *lsp.Manager, notify pubsub.Publisher[notify.Notification], + skillsMgr *skills.Manager, ) (Coordinator, error) { - // Discover skills once at session start. - allSkills, activeSkills := discoverSkills(cfg) + // Skills are pre-discovered by the caller (see app.New / + // backend.CreateWorkspace) and passed in via the manager. If no + // manager was provided (legacy callers), fall back to an in-line + // discovery so the coordinator still works. + var allSkills, activeSkills []*skills.Skill + if skillsMgr != nil { + allSkills = skillsMgr.AllSkills() + activeSkills = skillsMgr.ActiveSkills() + } else { + allSkills, activeSkills = discoverSkills(cfg) + } skillTracker := skills.NewTracker(activeSkills) c := &coordinator{ @@ -1138,53 +1147,31 @@ 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. -// It also emits a single diagnostic log line summarising the outcome to -// help track skill-loading health over time. +// discoverSkills is a thin fallback wrapper used only when no +// skills.Manager has been threaded through to the coordinator. All +// production call sites (backend.CreateWorkspace, setupLocalWorkspace) +// run discovery in advance and pass the results via the manager; +// reaching this path means a caller bypassed both. It deliberately does +// NOT publish to the package-level broker — there are no subscribers in +// that case, so doing so would be misleading without delivering the +// snapshot anywhere useful. func discoverSkills(cfg *config.ConfigStore) (allSkills, activeSkills []*skills.Skill) { - builtin, builtinStates := skills.DiscoverBuiltinWithStates() - discovered := append([]*skills.Skill(nil), builtin...) - - var userStates []*skills.SkillState - var userPaths []string - opts := cfg.Config().Options - if opts != nil && len(opts.SkillsPaths) > 0 { - userPaths = 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 - } - } - userPaths = append(userPaths, expanded) - } - var userSkills []*skills.Skill - userSkills, userStates = skills.DiscoverWithStates(userPaths) - discovered = append(discovered, userSkills...) - } - - allSkills = skills.Deduplicate(discovered) - var disabledSkills []string + var paths, disabled []string if opts != nil { - disabledSkills = opts.DisabledSkills + paths = opts.SkillsPaths + disabled = opts.DisabledSkills } - activeSkills = skills.Filter(allSkills, disabledSkills) - - allStates := append([]*skills.SkillState(nil), builtinStates...) - allStates = append(allStates, userStates...) - - allStates = skills.DeduplicateStates(allStates) - - slices.SortStableFunc(allStates, func(a, b *skills.SkillState) int { - return strings.Compare(strings.ToLower(a.Path), strings.ToLower(b.Path)) + var resolver func(string) (string, error) + if r := cfg.Resolver(); r != nil { + resolver = r.ResolveValue + } + allSkills, activeSkills, states := skills.DiscoverFromConfig(skills.DiscoveryConfig{ + SkillsPaths: paths, + DisabledSkills: disabled, + Resolver: resolver, }) - skills.SetLatestStates(allStates) - skills.PublishStates(allStates) - - logDiscoveryStats(builtin, builtinStates, userStates, userPaths, allSkills, activeSkills, disabledSkills) + logDiscoveryStats(states, paths, allSkills, activeSkills, disabled) return allSkills, activeSkills } @@ -1232,28 +1219,26 @@ func logTurnSkillUsage( // logDiscoveryStats emits a single structured log line summarising skill // discovery for the current session. It is intentionally low-volume: one -// line per session start. +// line per session start. Builtin vs user counts are derived from the +// SkillState.Path — builtin states use the "builtin/" embed prefix. func logDiscoveryStats( - builtin []*skills.Skill, - builtinStates, userStates []*skills.SkillState, + states []*skills.SkillState, userPaths []string, allSkills, activeSkills []*skills.Skill, disabled []string, ) { - countErrors := func(states []*skills.SkillState) int { - n := 0 - for _, s := range states { - if s.State == skills.StateError { - n++ - } - } - return n - } - - userOK := 0 - for _, s := range userStates { - if s.State == skills.StateNormal { + var builtinOK, builtinErr, userOK, userErr int + for _, s := range states { + isBuiltin := strings.HasPrefix(s.Path, "builtin/") + switch { + case isBuiltin && s.State == skills.StateNormal: + builtinOK++ + case isBuiltin && s.State == skills.StateError: + builtinErr++ + case !isBuiltin && s.State == skills.StateNormal: userOK++ + case !isBuiltin && s.State == skills.StateError: + userErr++ } } @@ -1267,10 +1252,10 @@ func logDiscoveryStats( slog.Info( "Skill discovery complete", "component", "skills", - "builtin_ok", len(builtin), - "builtin_errors", countErrors(builtinStates), + "builtin_ok", builtinOK, + "builtin_errors", builtinErr, "user_ok", userOK, - "user_errors", countErrors(userStates), + "user_errors", userErr, "user_paths", len(userPaths), "deduped_total", len(allSkills), "active", len(activeSkills), diff --git a/internal/app/app.go b/internal/app/app.go index f627cc928fa62abaa51ae1a7f92351ee4afaad7e..8fd3c9d9749b112d66dff8bb236734e92ab2f843 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -62,6 +62,8 @@ type App struct { LSPManager *lsp.Manager + Skills *skills.Manager + config *config.ConfigStore serviceEventsWG *sync.WaitGroup @@ -75,8 +77,11 @@ type App struct { agentNotifications *pubsub.Broker[notify.Notification] } -// New initializes a new application instance. -func New(ctx context.Context, conn *sql.DB, store *config.ConfigStore) (*App, error) { +// New initializes a new application instance. skillsMgr carries the +// per-workspace skill discovery results computed by the caller; the +// caller is responsible for constructing it (typically via +// skills.NewManager + skills.DiscoverFromConfig). +func New(ctx context.Context, conn *sql.DB, store *config.ConfigStore, skillsMgr *skills.Manager) (*App, error) { q := db.New(conn) sessions := session.NewService(q, conn) messages := message.NewService(q) @@ -95,6 +100,7 @@ func New(ctx context.Context, conn *sql.DB, store *config.ConfigStore) (*App, er Permissions: permission.NewPermissionService(store.WorkingDir(), skipPermissionsRequests, allowedTools), FileTracker: filetracker.NewService(q), LSPManager: lsp.NewManager(store), + Skills: skillsMgr, globalCtx: ctx, @@ -481,7 +487,9 @@ func (app *App) setupEvents() { setupSubscriber(ctx, app.serviceEventsWG, "agent-notifications", app.agentNotifications.Subscribe, app.events) setupSubscriber(ctx, app.serviceEventsWG, "mcp", mcp.SubscribeEvents, app.events) setupSubscriber(ctx, app.serviceEventsWG, "lsp", SubscribeLSPEvents, app.events) - setupSubscriber(ctx, app.serviceEventsWG, "skills", skills.SubscribeEvents, app.events) + if app.Skills != nil { + setupSubscriber(ctx, app.serviceEventsWG, "skills", app.Skills.SubscribeEvents, app.events) + } cleanupFunc := func(context.Context) error { cancel() app.serviceEventsWG.Wait() @@ -532,6 +540,7 @@ func (app *App) InitCoderAgent(ctx context.Context) error { app.FileTracker, app.LSPManager, app.agentNotifications, + app.Skills, ) if err != nil { slog.Error("Failed to create coder agent", "err", err) diff --git a/internal/backend/agent.go b/internal/backend/agent.go index 8cc2ada26ca737bbe79fe989cc52e4ede9712dc5..346e54bf8069af5f0f8b3bbde049aaed70e86cff 100644 --- a/internal/backend/agent.go +++ b/internal/backend/agent.go @@ -19,7 +19,7 @@ func (b *Backend) SendMessage(ctx context.Context, workspaceID string, msg proto return ErrAgentNotInitialized } - _, err = ws.AgentCoordinator.Run(ctx, msg.SessionID, msg.Prompt) + _, err = ws.AgentCoordinator.Run(ctx, msg.SessionID, msg.Prompt, proto.AttachmentsToMessage(msg.Attachments)...) return err } diff --git a/internal/backend/backend.go b/internal/backend/backend.go index 55e3a2785a77de4ab849cc367eb189abae81dbdb..5f0c47d1193acd799581ac50adc30ecd25f70e15 100644 --- a/internal/backend/backend.go +++ b/internal/backend/backend.go @@ -18,6 +18,7 @@ import ( "github.com/charmbracelet/crush/internal/csync" "github.com/charmbracelet/crush/internal/db" "github.com/charmbracelet/crush/internal/proto" + "github.com/charmbracelet/crush/internal/skills" "github.com/charmbracelet/crush/internal/ui/util" "github.com/charmbracelet/crush/internal/version" "github.com/google/uuid" @@ -96,10 +97,11 @@ type clientState struct { // associated resources and state. type Workspace struct { *app.App - ID string - Path string - Cfg *config.ConfigStore - Env []string + ID string + Path string + Cfg *config.ConfigStore + Env []string + Skills *skills.Manager // resolvedPath is the path used as the dedup key in // Backend.pathIndex. It is filepath.EvalSymlinks(filepath.Abs(Path)) @@ -228,7 +230,14 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works return nil, proto.Workspace{}, fmt.Errorf("failed to connect to database: %w", err) } - appWorkspace, err := app.New(b.ctx, conn, cfg) + // Discover skills once per workspace, before app.New. The backend + // hosts multiple workspaces concurrently, so the manager is + // constructed WITHOUT WithGlobalMirror to prevent last-writer-wins + // cross-talk between workspaces. + allSkills, activeSkills, skillStates := skills.DiscoverFromConfig(skillsDiscoveryConfig(cfg)) + skillsMgr := skills.NewManager(allSkills, activeSkills, skillStates) + + appWorkspace, err := app.New(b.ctx, conn, cfg, skillsMgr) if err != nil { return nil, proto.Workspace{}, fmt.Errorf("failed to create app workspace: %w", err) } @@ -239,6 +248,7 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works Path: args.Path, Cfg: cfg, Env: args.Env, + Skills: skillsMgr, resolvedPath: key, clients: make(map[string]*clientState), } @@ -282,6 +292,47 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works return ws, workspaceToProto(ws), nil } +// skillsDiscoveryConfig adapts a *config.ConfigStore to the +// skills.DiscoveryConfig that DiscoverFromConfig consumes. +func skillsDiscoveryConfig(cfg *config.ConfigStore) skills.DiscoveryConfig { + opts := cfg.Config().Options + var paths, disabled []string + if opts != nil { + paths = opts.SkillsPaths + disabled = opts.DisabledSkills + } + var resolver func(string) (string, error) + if r := cfg.Resolver(); r != nil { + resolver = r.ResolveValue + } + return skills.DiscoveryConfig{ + SkillsPaths: paths, + DisabledSkills: disabled, + Resolver: resolver, + } +} + +// skillStatesToProto converts internal skill discovery states into the +// wire format. +func skillStatesToProto(states []*skills.SkillState) []proto.SkillState { + if len(states) == 0 { + return nil + } + out := make([]proto.SkillState, len(states)) + for i, s := range states { + entry := proto.SkillState{ + Name: s.Name, + Path: s.Path, + State: proto.SkillDiscoveryState(s.State), + } + if s.Err != nil { + entry.Error = s.Err.Error() + } + out[i] = entry + } + return out +} + // AttachClient registers a new SSE stream for the given client on the // workspace. The stream's deferred cleanup must call DetachClient with // the same arguments to release the claim. @@ -598,7 +649,7 @@ func validateClientID(id string) (string, error) { func workspaceToProto(ws *Workspace) proto.Workspace { cfg := ws.Cfg.Config() - return proto.Workspace{ + out := proto.Workspace{ ID: ws.ID, Path: ws.Path, YOLO: ws.Cfg.Overrides().SkipPermissionRequests, @@ -608,6 +659,10 @@ func workspaceToProto(ws *Workspace) proto.Workspace { Env: ws.Env, Version: version.Version, } + if ws.Skills != nil { + out.Skills = skillStatesToProto(ws.Skills.States()) + } + return out } // logFirstWinsMismatch emits a debug line whenever a second diff --git a/internal/backend/backend_skills_test.go b/internal/backend/backend_skills_test.go new file mode 100644 index 0000000000000000000000000000000000000000..4f7dfa8f30e45976b9edba771576911a3205812a --- /dev/null +++ b/internal/backend/backend_skills_test.go @@ -0,0 +1,167 @@ +package backend_test + +import ( + "context" + "fmt" + "os" + "path/filepath" + "testing" + "time" + + tea "charm.land/bubbletea/v2" + "github.com/charmbracelet/crush/internal/backend" + "github.com/charmbracelet/crush/internal/config" + "github.com/charmbracelet/crush/internal/proto" + "github.com/charmbracelet/crush/internal/pubsub" + "github.com/charmbracelet/crush/internal/skills" + "github.com/google/uuid" + "github.com/stretchr/testify/require" +) + +// TestBackend_WorkspaceSkillsIsolation verifies that skill discovery +// state and SSE events are per-workspace, not process-global. Two +// workspaces in the same backend process must not see each other's +// discoveries (either in their initial snapshot or in subsequent +// PublishStates events). +func TestBackend_WorkspaceSkillsIsolation(t *testing.T) { + // Isolate all of config.Init's filesystem reads from the host. The + // project-local .agents/skills//SKILL.md per working dir is + // what we actually want each workspace to see; everything else + // (global skills, XDG dirs, etc.) must be empty/deterministic. + hostHome := t.TempDir() + t.Setenv("HOME", hostHome) + t.Setenv("XDG_CONFIG_HOME", filepath.Join(hostHome, ".config")) + t.Setenv("XDG_DATA_HOME", filepath.Join(hostHome, ".local", "share")) + t.Setenv("XDG_CACHE_HOME", filepath.Join(hostHome, ".cache")) + t.Setenv("CRUSH_SKILLS_DIR", t.TempDir()) + + // Each workspace gets its own working directory containing a + // distinct project-local skill so the discovery output differs. + wdA := t.TempDir() + wdB := t.TempDir() + writeSkill(t, wdA, "wsa-only-skill", "Workspace A only skill.") + writeSkill(t, wdB, "wsb-only-skill", "Workspace B only skill.") + + srvCfg, err := config.Init(wdA, "", false) + require.NoError(t, err) + b := backend.New(t.Context(), srvCfg, nil) + + cidA := uuid.New().String() + cidB := uuid.New().String() + + wsA, _, err := b.CreateWorkspace(proto.Workspace{ + ClientID: cidA, + Path: wdA, + DataDir: filepath.Join(wdA, ".crush"), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = b.DeleteWorkspace(wsA.ID, cidA) }) + + wsB, _, err := b.CreateWorkspace(proto.Workspace{ + ClientID: cidB, + Path: wdB, + DataDir: filepath.Join(wdB, ".crush"), + }) + require.NoError(t, err) + t.Cleanup(func() { _ = b.DeleteWorkspace(wsB.ID, cidB) }) + + require.NotNil(t, wsA.Skills, "workspace A must have its own skills.Manager") + require.NotNil(t, wsB.Skills, "workspace B must have its own skills.Manager") + require.NotSame(t, wsA.Skills, wsB.Skills, "managers must be distinct instances per workspace") + + // Initial snapshots see each workspace's own filesystem skill, and + // neither sees the other's. + statesA := wsA.Skills.States() + statesB := wsB.Skills.States() + require.True(t, containsSkillName(statesA, "wsa-only-skill"), + "workspace A snapshot missing its own skill") + require.False(t, containsSkillName(statesA, "wsb-only-skill"), + "workspace A snapshot leaked workspace B's skill") + require.True(t, containsSkillName(statesB, "wsb-only-skill"), + "workspace B snapshot missing its own skill") + require.False(t, containsSkillName(statesB, "wsa-only-skill"), + "workspace B snapshot leaked workspace A's skill") + + // Subscribe to each workspace's SSE event stream. + ctxA, cancelA := context.WithCancel(t.Context()) + t.Cleanup(cancelA) + chA, err := b.SubscribeEvents(ctxA, wsA.ID) + require.NoError(t, err) + + ctxB, cancelB := context.WithCancel(t.Context()) + t.Cleanup(cancelB) + chB, err := b.SubscribeEvents(ctxB, wsB.ID) + require.NoError(t, err) + + // Trigger a republish on workspace A only. The marker name lets us + // distinguish this event from any incidental backend activity. + const marker = "wsa-republish-marker" + wsA.Skills.PublishStates([]*skills.SkillState{ + {Name: marker, State: skills.StateNormal}, + }) + + // Workspace A must receive its own event. + require.True(t, + waitForSkillsEvent(t, chA, marker, 2*time.Second), + "workspace A never received its own skills event") + + // Workspace B must NOT receive workspace A's event. + require.False(t, + waitForSkillsEvent(t, chB, marker, 250*time.Millisecond), + "workspace B leaked workspace A's skills event") + + // And A's published states are now visible on its manager's + // snapshot (verifies PublishStates updates the cache, not just the + // broker). + updatedA := wsA.Skills.States() + require.True(t, containsSkillName(updatedA, marker), + "PublishStates must update Manager.States()") + + // B's manager snapshot is untouched. + require.False(t, containsSkillName(wsB.Skills.States(), marker), + "workspace B's Manager.States() leaked workspace A's republish") +} + +func writeSkill(t *testing.T, workingDir, name, desc string) { + t.Helper() + skillDir := filepath.Join(workingDir, ".agents", "skills", name) + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + content := fmt.Sprintf("---\nname: %s\ndescription: %s\n---\n%s\n", name, desc, desc) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(content), 0o644)) +} + +func containsSkillName(states []*skills.SkillState, name string) bool { + for _, s := range states { + if s.Name == name { + return true + } + } + return false +} + +// waitForSkillsEvent drains the given event channel until either a +// pubsub.Event[skills.Event] containing a state named marker arrives or +// the timeout elapses. Non-skills events on the channel are silently +// skipped — the backend fans in many event types and we only care +// about skills here. +func waitForSkillsEvent(t *testing.T, ch <-chan pubsub.Event[tea.Msg], marker string, timeout time.Duration) bool { + t.Helper() + deadline := time.After(timeout) + for { + select { + case ev, ok := <-ch: + if !ok { + return false + } + se, ok := ev.Payload.(pubsub.Event[skills.Event]) + if !ok { + continue + } + if containsSkillName(se.Payload.States, marker) { + return true + } + case <-deadline: + return false + } + } +} diff --git a/internal/client/proto.go b/internal/client/proto.go index ab8b7cb7a04e5c527f333d7deaf28474def96e8f..5a57262679df0a7edc3f9269dc763fc124e778cd 100644 --- a/internal/client/proto.go +++ b/internal/client/proto.go @@ -199,6 +199,10 @@ func (c *Client) SubscribeEvents(ctx context.Context, id string) (<-chan any, er var e pubsub.Event[proto.ConfigChanged] _ = json.Unmarshal(p.Payload, &e) sendEvent(ctx, events, e) + case pubsub.PayloadTypeSkillsEvent: + var e pubsub.Event[proto.SkillsEvent] + _ = json.Unmarshal(p.Payload, &e) + sendEvent(ctx, events, e) default: slog.Warn("Unknown event type", "type", p.Type) continue @@ -366,19 +370,10 @@ func (c *Client) UpdateAgent(ctx context.Context, id string) error { // SendMessage sends a message to the agent for a workspace. func (c *Client) SendMessage(ctx context.Context, id string, sessionID, prompt string, attachments ...message.Attachment) error { - protoAttachments := make([]proto.Attachment, len(attachments)) - for i, a := range attachments { - protoAttachments[i] = proto.Attachment{ - FilePath: a.FilePath, - FileName: a.FileName, - MimeType: a.MimeType, - Content: a.Content, - } - } rsp, err := c.post(ctx, fmt.Sprintf("/workspaces/%s/agent", id), nil, jsonBody(proto.AgentMessage{ SessionID: sessionID, Prompt: prompt, - Attachments: protoAttachments, + Attachments: proto.AttachmentsFromMessage(attachments), }), http.Header{"Content-Type": []string{"application/json"}}) if err != nil { return fmt.Errorf("failed to send message to agent: %w", err) diff --git a/internal/cmd/root.go b/internal/cmd/root.go index 3f5971c01262b3375ee8c6becef91de96afc4483..d94f2b0be08317f8adb8a3f4f3ad703fbc92add6 100644 --- a/internal/cmd/root.go +++ b/internal/cmd/root.go @@ -34,6 +34,7 @@ import ( "github.com/charmbracelet/crush/internal/proto" "github.com/charmbracelet/crush/internal/server" "github.com/charmbracelet/crush/internal/session" + "github.com/charmbracelet/crush/internal/skills" "github.com/charmbracelet/crush/internal/ui/common" ui "github.com/charmbracelet/crush/internal/ui/model" "github.com/charmbracelet/crush/internal/version" @@ -286,7 +287,14 @@ func setupLocalWorkspace(cmd *cobra.Command) (workspace.Workspace, func(), error logFile := filepath.Join(cfg.Options.DataDirectory, "logs", "crush.log") crushlog.Setup(logFile, debug) - appInstance, err := app.New(ctx, conn, store) + // Discover skills once before app.New. Local mode hosts a single + // workspace per process, so WithGlobalMirror keeps the package + // globals (which the TUI reads via skills.GetLatestStates) in sync + // with the manager. + allSkills, activeSkills, skillStates := skills.DiscoverFromConfig(localSkillsDiscoveryConfig(store)) + skillsMgr := skills.NewManager(allSkills, activeSkills, skillStates, skills.WithGlobalMirror()) + + appInstance, err := app.New(ctx, conn, store, skillsMgr) if err != nil { _ = conn.Close() slog.Error("Failed to create app instance", "error", err) @@ -302,6 +310,26 @@ func setupLocalWorkspace(cmd *cobra.Command) (workspace.Workspace, func(), error return ws, cleanup, nil } +// localSkillsDiscoveryConfig adapts a *config.ConfigStore to the inputs +// skills.DiscoverFromConfig expects. +func localSkillsDiscoveryConfig(store *config.ConfigStore) skills.DiscoveryConfig { + opts := store.Config().Options + var paths, disabled []string + if opts != nil { + paths = opts.SkillsPaths + disabled = opts.DisabledSkills + } + var resolver func(string) (string, error) + if r := store.Resolver(); r != nil { + resolver = r.ResolveValue + } + return skills.DiscoveryConfig{ + SkillsPaths: paths, + DisabledSkills: disabled, + Resolver: resolver, + } +} + // setupClientServerWorkspace connects to a server process and wraps the // result in a ClientWorkspace. func setupClientServerWorkspace(cmd *cobra.Command) (workspace.Workspace, func(), error) { diff --git a/internal/commands/commands.go b/internal/commands/commands.go index fe9d7e71606ebc15af1c5bdbddf85ab0b9a5c1fb..58a5c2e123a8ad655097c189213a4f5e8e847920 100644 --- a/internal/commands/commands.go +++ b/internal/commands/commands.go @@ -12,6 +12,7 @@ import ( "github.com/charmbracelet/crush/internal/agent/tools/mcp" "github.com/charmbracelet/crush/internal/config" "github.com/charmbracelet/crush/internal/home" + "github.com/charmbracelet/crush/internal/skills" ) var namedArgPattern = regexp.MustCompile(`\$([A-Z][A-Z0-9_]*)`) @@ -45,6 +46,8 @@ type CustomCommand struct { Name string Content string Arguments []Argument + // Skill is set when this command represents a user-invocable skill + Skill *skills.Skill } type commandSource struct { @@ -58,6 +61,70 @@ func LoadCustomCommands(cfg *config.Config) ([]CustomCommand, error) { return loadAll(buildCommandSources(cfg)) } +// LoadSkillCommands loads user-invocable skills as custom commands. +func LoadSkillCommands() []CustomCommand { + var commands []CustomCommand + + // Load from global skills directories with "user:" prefix + for _, dir := range config.GlobalSkillsDirs() { + commands = append(commands, loadInvocableSkillsFromDir(dir, userCommandPrefix)...) + } + + return commands +} + +// LoadProjectSkillCommands loads user-invocable skills from project directories as custom commands. +func LoadProjectSkillCommands(workingDir string) []CustomCommand { + var commands []CustomCommand + + // Load from project skills directories with "project:" prefix + for _, dir := range config.ProjectSkillsDir(workingDir) { + commands = append(commands, loadInvocableSkillsFromDir(dir, projectCommandPrefix)...) + } + + return commands +} + +func loadInvocableSkillsFromDir(dir, prefix string) []CustomCommand { + if _, err := os.Stat(dir); os.IsNotExist(err) { + return nil + } + + var commands []CustomCommand + + entries, err := os.ReadDir(dir) + if err != nil { + return nil + } + + for _, entry := range entries { + if !entry.IsDir() { + continue + } + + skillPath := filepath.Join(dir, entry.Name(), skills.SkillFileName) + skill, err := skills.Parse(skillPath) + if err != nil { + continue + } + + if !skill.UserInvocable { + continue + } + + name := prefix + skill.Name + commands = append(commands, CustomCommand{ + ID: name, + Name: name, + Content: skill.Instructions, + Arguments: nil, + Skill: skill, + }) + } + + return commands +} + // LoadMCPPrompts loads custom commands from available MCP servers. func LoadMCPPrompts() ([]MCPPrompt, error) { var commands []MCPPrompt diff --git a/internal/permission/permission.go b/internal/permission/permission.go index ea7c21e8a4114ec4c6ace76f020ce2d0e25d4385..25e4994a807f44b0a9fd994a1d6ebe548641202a 100644 --- a/internal/permission/permission.go +++ b/internal/permission/permission.go @@ -6,6 +6,7 @@ import ( "path/filepath" "slices" "sync" + "sync/atomic" "github.com/charmbracelet/crush/internal/csync" "github.com/charmbracelet/crush/internal/pubsub" @@ -100,7 +101,7 @@ type permissionService struct { pendingRequests *csync.Map[string, chan bool] autoApproveSessions map[string]bool autoApproveSessionsMu sync.RWMutex - skip bool + skip atomic.Bool allowedTools []string // used to make sure we only process one request at a time @@ -178,7 +179,7 @@ func (s *permissionService) Deny(permission PermissionRequest) bool { } func (s *permissionService) Request(ctx context.Context, opts CreatePermissionRequest) (bool, error) { - if s.skip { + if s.skip.Load() { return true, nil } @@ -287,22 +288,23 @@ func (s *permissionService) SubscribeNotifications(ctx context.Context) <-chan p } func (s *permissionService) SetSkipRequests(skip bool) { - s.skip = skip + s.skip.Store(skip) } func (s *permissionService) SkipRequests() bool { - return s.skip + return s.skip.Load() } func NewPermissionService(workingDir string, skip bool, allowedTools []string) Service { - return &permissionService{ + svc := &permissionService{ Broker: pubsub.NewBroker[PermissionRequest](), notificationBroker: pubsub.NewBroker[PermissionNotification](), workingDir: workingDir, sessionPermissions: csync.NewMap[PermissionKey, bool](), autoApproveSessions: make(map[string]bool), - skip: skip, allowedTools: allowedTools, pendingRequests: csync.NewMap[string, chan bool](), } + svc.skip.Store(skip) + return svc } diff --git a/internal/permission/permission_test.go b/internal/permission/permission_test.go index 42f3b40378185c4da5f90f654589ba988ddffa7d..9d464f7a0c16b04491b5a6e0e621a0b64fa94ff4 100644 --- a/internal/permission/permission_test.go +++ b/internal/permission/permission_test.go @@ -81,6 +81,21 @@ func TestPermissionService_AllowedCommands(t *testing.T) { } } +func TestSkipRace(t *testing.T) { + svc := NewPermissionService("/tmp", false, nil) + var wg sync.WaitGroup + wg.Add(2) + go func() { + defer wg.Done() + svc.SetSkipRequests(true) + }() + go func() { + defer wg.Done() + svc.SkipRequests() + }() + wg.Wait() +} + func TestPermissionService_SkipMode(t *testing.T) { service := NewPermissionService("/tmp", true, []string{}) diff --git a/internal/proto/message.go b/internal/proto/message.go index f1ae259cf2cd3238b522c31cd46387abc4fd6173..307dd71ff2de48e942a9dce77b2a9fcb8a6507d4 100644 --- a/internal/proto/message.go +++ b/internal/proto/message.go @@ -8,6 +8,7 @@ import ( "time" "charm.land/catwalk/pkg/catwalk" + "github.com/charmbracelet/crush/internal/message" ) // CreateMessageParams represents parameters for creating a message. @@ -621,6 +622,47 @@ type Attachment struct { Content []byte `json:"content"` } +// ToMessage converts a proto Attachment to a [message.Attachment]. +func (a Attachment) ToMessage() message.Attachment { + return message.Attachment{ + FilePath: a.FilePath, + FileName: a.FileName, + MimeType: a.MimeType, + Content: a.Content, + } +} + +// AttachmentFromMessage converts a [message.Attachment] to a proto +// Attachment. +func AttachmentFromMessage(a message.Attachment) Attachment { + return Attachment{ + FilePath: a.FilePath, + FileName: a.FileName, + MimeType: a.MimeType, + Content: a.Content, + } +} + +// AttachmentsToMessage converts a slice of proto Attachments to a slice +// of [message.Attachment]. +func AttachmentsToMessage(as []Attachment) []message.Attachment { + out := make([]message.Attachment, len(as)) + for i, a := range as { + out[i] = a.ToMessage() + } + return out +} + +// AttachmentsFromMessage converts a slice of [message.Attachment] to a +// slice of proto Attachments. +func AttachmentsFromMessage(as []message.Attachment) []Attachment { + out := make([]Attachment, len(as)) + for i, a := range as { + out[i] = AttachmentFromMessage(a) + } + return out +} + // MarshalJSON implements the [json.Marshaler] interface. func (a Attachment) MarshalJSON() ([]byte, error) { type Alias Attachment diff --git a/internal/proto/proto.go b/internal/proto/proto.go index adb13f146061cb4b17181d5a3f0ac887f39dabca..2896a87b94338e01ac809933e761f8c0ef0efa0f 100644 --- a/internal/proto/proto.go +++ b/internal/proto/proto.go @@ -22,6 +22,10 @@ type Workspace struct { ClientID string `json:"client_id,omitempty"` Config *config.Config `json:"config,omitempty"` Env []string `json:"env,omitempty"` + // Skills carries the snapshot of skill discovery state at workspace + // creation time. Subsequent updates flow through the SSE event + // stream. + Skills []SkillState `json:"skills,omitempty"` } // Error represents an error response. diff --git a/internal/proto/skills.go b/internal/proto/skills.go new file mode 100644 index 0000000000000000000000000000000000000000..708b01bae685b528f4d639f18d3192867e7d3203 --- /dev/null +++ b/internal/proto/skills.go @@ -0,0 +1,28 @@ +package proto + +// SkillDiscoveryState mirrors skills.DiscoveryState across the wire. +// Values must stay in sync with internal/skills.DiscoveryState; do not +// reorder without a coordinated server/client bump. +type SkillDiscoveryState int + +const ( + // SkillStateNormal indicates the skill was parsed and validated + // successfully. + SkillStateNormal SkillDiscoveryState = iota + // SkillStateError indicates discovery encountered a scan/parse/validate + // error. + SkillStateError +) + +// SkillState is the wire representation of skills.SkillState. +type SkillState struct { + Name string `json:"name"` + Path string `json:"path"` + State SkillDiscoveryState `json:"state"` + Error string `json:"error,omitempty"` +} + +// SkillsEvent is the wire representation of skills.Event. +type SkillsEvent struct { + States []SkillState `json:"states"` +} diff --git a/internal/pubsub/events.go b/internal/pubsub/events.go index 6056940fe6eb221c70025b99edc258ab36d4a717..7f75d7d19e39f2a714fccc5be0232a19fadab7b9 100644 --- a/internal/pubsub/events.go +++ b/internal/pubsub/events.go @@ -25,6 +25,7 @@ const ( PayloadTypeFile PayloadType = "file" PayloadTypeAgentEvent PayloadType = "agent_event" PayloadTypeConfigChanged PayloadType = "config_changed" + PayloadTypeSkillsEvent PayloadType = "skills_event" ) // Payload wraps a discriminated JSON payload with a type tag. diff --git a/internal/server/events.go b/internal/server/events.go index 8a0ab777d77dca9ece44cbaca579676b520708ca..f38619c52528679bf75675780eb4bb47961bd640 100644 --- a/internal/server/events.go +++ b/internal/server/events.go @@ -15,6 +15,7 @@ import ( "github.com/charmbracelet/crush/internal/proto" "github.com/charmbracelet/crush/internal/pubsub" "github.com/charmbracelet/crush/internal/session" + "github.com/charmbracelet/crush/internal/skills" ) // wrapEvent converts a raw tea.Msg (a pubsub.Event[T] from the app @@ -94,6 +95,11 @@ func wrapEvent(ev any) *pubsub.Payload { }) case pubsub.Event[proto.ConfigChanged]: return envelope(pubsub.PayloadTypeConfigChanged, e) + case pubsub.Event[skills.Event]: + return envelope(pubsub.PayloadTypeSkillsEvent, pubsub.Event[proto.SkillsEvent]{ + Type: e.Type, + Payload: skillsEventToProto(e.Payload), + }) default: slog.Warn("Unrecognized event type for SSE wrapping", "type", fmt.Sprintf("%T", ev)) return nil @@ -250,6 +256,27 @@ func messageToProto(m message.Message) proto.Message { return msg } +// skillsEventToProto converts a skills.Event into its wire form. Errors +// are flattened to strings because error does not round-trip over JSON. +func skillsEventToProto(e skills.Event) proto.SkillsEvent { + if len(e.States) == 0 { + return proto.SkillsEvent{} + } + out := proto.SkillsEvent{States: make([]proto.SkillState, len(e.States))} + for i, s := range e.States { + entry := proto.SkillState{ + Name: s.Name, + Path: s.Path, + State: proto.SkillDiscoveryState(s.State), + } + if s.Err != nil { + entry.Error = s.Err.Error() + } + out.States[i] = entry + } + return out +} + func messagesToProto(msgs []message.Message) []proto.Message { out := make([]proto.Message, len(msgs)) for i, m := range msgs { diff --git a/internal/server/events_test.go b/internal/server/events_test.go index 80b9428c104651bc3a372bf614403177fe2ab5d7..b32d694d793e04f216a51035098489588aa39628 100644 --- a/internal/server/events_test.go +++ b/internal/server/events_test.go @@ -1,10 +1,14 @@ package server import ( + "encoding/json" + "errors" "testing" "github.com/charmbracelet/crush/internal/message" "github.com/charmbracelet/crush/internal/proto" + "github.com/charmbracelet/crush/internal/pubsub" + "github.com/charmbracelet/crush/internal/skills" "github.com/stretchr/testify/require" ) @@ -44,3 +48,38 @@ func TestMessageToProtoToolResult(t *testing.T) { require.Equal(t, `{"file_path":"/tmp/x","content":"hi"}`, tr.Metadata) require.False(t, tr.IsError) } + +// TestSkillsEventToProto_RoundTrip verifies that a pubsub.Event[skills.Event] +// can be wrapped, marshaled, and unmarshaled back through the SSE +// envelope without losing state values or error messages. +func TestSkillsEventToProto_RoundTrip(t *testing.T) { + t.Parallel() + + src := pubsub.Event[skills.Event]{ + Type: pubsub.UpdatedEvent, + Payload: skills.Event{ + States: []*skills.SkillState{ + {Name: "ok", Path: "/p/ok", State: skills.StateNormal}, + {Name: "broken", Path: "/p/broken", State: skills.StateError, Err: errors.New("bad frontmatter")}, + }, + }, + } + + env := wrapEvent(src) + require.NotNil(t, env) + require.Equal(t, pubsub.PayloadTypeSkillsEvent, env.Type) + + var decoded pubsub.Event[proto.SkillsEvent] + require.NoError(t, json.Unmarshal(env.Payload, &decoded)) + require.Equal(t, pubsub.UpdatedEvent, decoded.Type) + require.Len(t, decoded.Payload.States, 2) + + require.Equal(t, "ok", decoded.Payload.States[0].Name) + require.Equal(t, "/p/ok", decoded.Payload.States[0].Path) + require.Equal(t, proto.SkillStateNormal, decoded.Payload.States[0].State) + require.Empty(t, decoded.Payload.States[0].Error) + + require.Equal(t, "broken", decoded.Payload.States[1].Name) + require.Equal(t, proto.SkillStateError, decoded.Payload.States[1].State) + require.Equal(t, "bad frontmatter", decoded.Payload.States[1].Error) +} diff --git a/internal/skills/builtin/crush-config/SKILL.md b/internal/skills/builtin/crush-config/SKILL.md index df4f6444b4145bcbd1d1de9a2c078aedb5b105e9..dd69a3733e58fa5f41debcb6bd83b00ab9d203ad 100644 --- a/internal/skills/builtin/crush-config/SKILL.md +++ b/internal/skills/builtin/crush-config/SKILL.md @@ -211,6 +211,37 @@ reviewed. Other options: `context_paths`, `progress`, `disable_notifications`, `disable_auto_summarize`, `disable_metrics`, `disable_provider_auto_update`, `disable_default_providers`, `data_directory`, `initialize_as`. +## User-Invocable Skills + +Skills can be made invocable as commands from the commands palette. Add `user-invocable: true` to the skill's YAML frontmatter: + +```yaml +--- +name: my-skill +description: A skill that can be invoked as a command. +user-invocable: true +--- +``` + +User-invocable skills appear in the commands palette with a prefix: +- Skills from global directories: `user:skill-name` +- Skills from project directories: `project:skill-name` + +When invoked, the skill's instructions are loaded into the conversation context. + +To prevent the model from auto-triggering a skill (while still allowing user invocation), add `disable-model-invocation: true`: + +```yaml +--- +name: my-skill +description: Only invocable by users, not the model. +user-invocable: true +disable-model-invocation: true +--- +``` + +Skills with `disable-model-invocation` won't appear in the model's available skills list but can still be invoked manually by users. + ## Hooks Hooks are user-defined shell commands that fire on agent events. Currently only `PreToolUse` is supported, which runs before a tool is executed. diff --git a/internal/skills/manager.go b/internal/skills/manager.go new file mode 100644 index 0000000000000000000000000000000000000000..8128665c0422fbabccb76382f099999aad8885d2 --- /dev/null +++ b/internal/skills/manager.go @@ -0,0 +1,180 @@ +package skills + +import ( + "context" + "slices" + "strings" + "sync" + + "github.com/charmbracelet/crush/internal/home" + "github.com/charmbracelet/crush/internal/pubsub" +) + +// Manager owns per-workspace skill discovery state: the latest discovery +// snapshot, the full skill metadata (with Instructions) for the +// coordinator, and a pubsub broker for change events. There is exactly +// one Manager per workspace. +// +// Package-level helpers (GetLatestStates, SetLatestStates, +// PublishStates, SubscribeEvents) are preserved for callers that share a +// process with the TUI. To bridge a Manager to those globals, construct +// it with WithGlobalMirror. Only do this when the process hosts a single +// workspace (local mode or a client process); the backend server hosts +// multiple workspaces concurrently and must not enable mirroring. +type Manager struct { + mu sync.RWMutex + allSkills []*Skill + activeSkills []*Skill + states []*SkillState + + broker *pubsub.Broker[Event] + globalMirror bool +} + +// ManagerOption configures a Manager at construction time. +type ManagerOption func(*Manager) + +// WithGlobalMirror causes the manager to forward SetLatestStates and +// PublishStates calls to the package-level cache and broker. Only safe +// when the process hosts at most one Manager (e.g. local mode or the +// client process). +func WithGlobalMirror() ManagerOption { + return func(m *Manager) { + m.globalMirror = true + } +} + +// NewManager constructs a workspace-scoped Manager with the given +// pre-computed discovery results. The slices are stored as-is; callers +// should not mutate them afterwards. +func NewManager(allSkills, activeSkills []*Skill, states []*SkillState, opts ...ManagerOption) *Manager { + m := &Manager{ + allSkills: allSkills, + activeSkills: activeSkills, + states: states, + broker: pubsub.NewBroker[Event](), + } + for _, opt := range opts { + opt(m) + } + if m.globalMirror { + SetLatestStates(states) + } + return m +} + +// AllSkills returns the deduplicated list of all discovered skills. +func (m *Manager) AllSkills() []*Skill { + m.mu.RLock() + defer m.mu.RUnlock() + return m.allSkills +} + +// ActiveSkills returns the post-filter list of active skills (after +// removing disabled entries). +func (m *Manager) ActiveSkills() []*Skill { + m.mu.RLock() + defer m.mu.RUnlock() + return m.activeSkills +} + +// States returns a clone of the latest discovery state snapshot. +func (m *Manager) States() []*SkillState { + m.mu.RLock() + defer m.mu.RUnlock() + return cloneStates(m.states) +} + +// SetLatestStates updates the manager's cached discovery snapshot. +func (m *Manager) SetLatestStates(states []*SkillState) { + m.mu.Lock() + m.states = cloneStates(states) + m.mu.Unlock() + if m.globalMirror { + SetLatestStates(states) + } +} + +// PublishStates updates the manager's cached snapshot and publishes a +// discovery event to subscribers. Callers should not call +// SetLatestStates separately — PublishStates is the single mutation +// point, keeping Manager.States(), workspaceToProto, and (when +// WithGlobalMirror is set) skills.GetLatestStates consistent with what +// subscribers observe. +func (m *Manager) PublishStates(states []*SkillState) { + m.mu.Lock() + m.states = cloneStates(states) + m.mu.Unlock() + if m.globalMirror { + SetLatestStates(states) + } + m.broker.Publish(pubsub.UpdatedEvent, Event{States: cloneStates(states)}) + if m.globalMirror { + PublishStates(states) + } +} + +// SubscribeEvents returns a channel of discovery events for the +// manager's workspace. +func (m *Manager) SubscribeEvents(ctx context.Context) <-chan pubsub.Event[Event] { + return m.broker.Subscribe(ctx) +} + +// Shutdown releases broker resources. +func (m *Manager) Shutdown() { + if m.broker != nil { + m.broker.Shutdown() + } +} + +// DiscoverFromConfig walks the embedded builtin FS and every path in +// cfg.Options.SkillsPaths (after home / env expansion), then dedups and +// filters by cfg.Options.DisabledSkills. It returns the three slices the +// rest of the system needs: +// +// - allSkills: deduplicated, pre-filter (includes disabled). +// - activeSkills: post-filter (DisabledSkills removed). +// - states: per-file discovery outcome for diagnostics/UI. +func DiscoverFromConfig(cfg DiscoveryConfig) (allSkills, activeSkills []*Skill, states []*SkillState) { + builtin, builtinStates := DiscoverBuiltinWithStates() + discovered := append([]*Skill(nil), builtin...) + + var userStates []*SkillState + var userPaths []string + if len(cfg.SkillsPaths) > 0 { + userPaths = make([]string, 0, len(cfg.SkillsPaths)) + for _, pth := range cfg.SkillsPaths { + expanded := home.Long(pth) + if strings.HasPrefix(expanded, "$") && cfg.Resolver != nil { + if resolved, err := cfg.Resolver(expanded); err == nil { + expanded = resolved + } + } + userPaths = append(userPaths, expanded) + } + var userSkills []*Skill + userSkills, userStates = DiscoverWithStates(userPaths) + discovered = append(discovered, userSkills...) + } + + allSkills = Deduplicate(discovered) + activeSkills = Filter(allSkills, cfg.DisabledSkills) + + allStates := append([]*SkillState(nil), builtinStates...) + allStates = append(allStates, userStates...) + allStates = DeduplicateStates(allStates) + slices.SortStableFunc(allStates, func(a, b *SkillState) int { + return strings.Compare(strings.ToLower(a.Path), strings.ToLower(b.Path)) + }) + return allSkills, activeSkills, allStates +} + +// DiscoveryConfig contains the inputs DiscoverFromConfig needs. Using a +// dedicated struct (rather than importing internal/config) keeps the +// skills package's dependency graph small. +type DiscoveryConfig struct { + SkillsPaths []string + DisabledSkills []string + // Resolver expands $VAR-style references in paths. May be nil. + Resolver func(string) (string, error) +} diff --git a/internal/skills/manager_test.go b/internal/skills/manager_test.go new file mode 100644 index 0000000000000000000000000000000000000000..49af295c32fb1a4a52e2c6801a69ea5c80caeb11 --- /dev/null +++ b/internal/skills/manager_test.go @@ -0,0 +1,249 @@ +package skills + +import ( + "context" + "errors" + "os" + "path/filepath" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestManager_NoGlobalMirrorByDefault(t *testing.T) { + // Not parallel - touches package-level cache. + prev := GetLatestStates() + t.Cleanup(func() { SetLatestStates(prev) }) + + SetLatestStates(nil) + + mgrA := NewManager(nil, nil, []*SkillState{{Name: "a", State: StateNormal}}) + mgrB := NewManager(nil, nil, []*SkillState{{Name: "b", State: StateNormal}}) + + mgrA.PublishStates(mgrA.States()) + mgrB.PublishStates(mgrB.States()) + + // Without WithGlobalMirror, the package-level cache must not be + // touched by manager construction or PublishStates calls. + require.Nil(t, GetLatestStates(), "package global must remain untouched") + require.Equal(t, "a", mgrA.States()[0].Name) + require.Equal(t, "b", mgrB.States()[0].Name) +} + +func TestManager_GlobalMirror(t *testing.T) { + // Not parallel - touches package-level cache. + prev := GetLatestStates() + t.Cleanup(func() { SetLatestStates(prev) }) + + SetLatestStates(nil) + + mgr := NewManager(nil, nil, []*SkillState{{Name: "x", State: StateNormal}}, WithGlobalMirror()) + + got := GetLatestStates() + require.Len(t, got, 1) + require.Equal(t, "x", got[0].Name) + + // PublishStates with mirror enabled forwards to the global cache. + mgr.SetLatestStates([]*SkillState{{Name: "y", State: StateNormal}}) + got = GetLatestStates() + require.Len(t, got, 1) + require.Equal(t, "y", got[0].Name) +} + +func TestManager_PublishStatesUpdatesCache(t *testing.T) { + // Not parallel - exercises WithGlobalMirror, which touches the + // package-level cache. + prev := GetLatestStates() + t.Cleanup(func() { SetLatestStates(prev) }) + + SetLatestStates(nil) + + mgr := NewManager(nil, nil, []*SkillState{{Name: "old"}}, WithGlobalMirror()) + t.Cleanup(mgr.Shutdown) + + // PublishStates must update every observable snapshot, not just the + // SSE subscribers: Manager.States() (used by workspaceToProto on + // the backend) and skills.GetLatestStates() (read by the TUI on the + // client process and in local mode) must reflect the new value. + mgr.PublishStates([]*SkillState{{Name: "new"}}) + + got := mgr.States() + require.Len(t, got, 1) + require.Equal(t, "new", got[0].Name) + + cached := GetLatestStates() + require.Len(t, cached, 1) + require.Equal(t, "new", cached[0].Name) +} + +func TestManager_SubscribeReceivesPublishedStates(t *testing.T) { + t.Parallel() + + mgr := NewManager(nil, nil, nil) + t.Cleanup(mgr.Shutdown) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + ch := mgr.SubscribeEvents(ctx) + + want := []*SkillState{{Name: "k", State: StateNormal}} + go mgr.PublishStates(want) + + select { + case ev := <-ch: + require.Equal(t, "k", ev.Payload.States[0].Name) + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for manager event") + } +} + +func TestManager_ConcurrentWorkspacesAreIsolated(t *testing.T) { + t.Parallel() + + // Two managers without WithGlobalMirror should not see each other's + // events; this models the multi-workspace backend. + mgrA := NewManager(nil, nil, nil) + mgrB := NewManager(nil, nil, nil) + t.Cleanup(mgrA.Shutdown) + t.Cleanup(mgrB.Shutdown) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + chA := mgrA.SubscribeEvents(ctx) + chB := mgrB.SubscribeEvents(ctx) + + go mgrA.PublishStates([]*SkillState{{Name: "from-a"}}) + + select { + case ev := <-chA: + require.Equal(t, "from-a", ev.Payload.States[0].Name) + case <-time.After(2 * time.Second): + t.Fatal("workspace A never received its own event") + } + + select { + case ev := <-chB: + t.Fatalf("workspace B received workspace A's event: %v", ev) + case <-time.After(100 * time.Millisecond): + // Expected — B's stream is isolated. + } +} + +func TestDiscoverFromConfig(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + skillDir := filepath.Join(tmp, "custom-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(skillDir, SkillFileName), + []byte("---\nname: custom-skill\ndescription: A custom skill for tests.\n---\nDo a thing.\n"), + 0o644, + )) + + allSkills, activeSkills, states := DiscoverFromConfig(DiscoveryConfig{ + SkillsPaths: []string{tmp}, + DisabledSkills: nil, + }) + + // Builtins plus our one custom skill. + require.NotEmpty(t, allSkills) + require.NotEmpty(t, activeSkills) + require.GreaterOrEqual(t, len(allSkills), 2) + require.GreaterOrEqual(t, len(activeSkills), 2) + + // The custom skill is present with full Instructions populated, so + // the coordinator can render system prompts without re-walking the + // filesystem. + var custom *Skill + for _, s := range allSkills { + if s.Name == "custom-skill" { + custom = s + break + } + } + require.NotNil(t, custom) + require.NotEmpty(t, custom.Instructions, "DiscoverFromConfig must return Skill.Instructions") + + // State snapshot includes the custom skill too. + foundCustom := false + for _, s := range states { + if s.Name == "custom-skill" { + foundCustom = true + require.Equal(t, StateNormal, s.State) + } + } + require.True(t, foundCustom, "states slice should include the custom skill") +} + +func TestDiscoverFromConfig_DisabledFiltered(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + skillDir := filepath.Join(tmp, "off-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(skillDir, SkillFileName), + []byte("---\nname: off-skill\ndescription: Should be filtered.\n---\nx\n"), + 0o644, + )) + + allSkills, activeSkills, states := DiscoverFromConfig(DiscoveryConfig{ + SkillsPaths: []string{tmp}, + DisabledSkills: []string{"off-skill"}, + }) + + // All discovered: yes; active: no. + hasInAll := false + for _, s := range allSkills { + if s.Name == "off-skill" { + hasInAll = true + } + } + require.True(t, hasInAll, "DisabledSkills must not be removed from allSkills") + + for _, s := range activeSkills { + require.NotEqual(t, "off-skill", s.Name, "DisabledSkills must be removed from activeSkills") + } + + // State snapshot still carries discovered entries (UI re-applies filter). + hasInStates := false + for _, s := range states { + if s.Name == "off-skill" { + hasInStates = true + } + } + require.True(t, hasInStates) +} + +func TestDiscoverFromConfig_Resolver(t *testing.T) { + t.Parallel() + + tmp := t.TempDir() + skillDir := filepath.Join(tmp, "envvar-skill") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.WriteFile( + filepath.Join(skillDir, SkillFileName), + []byte("---\nname: envvar-skill\ndescription: Env-resolved.\n---\nx\n"), + 0o644, + )) + + allSkills, _, _ := DiscoverFromConfig(DiscoveryConfig{ + SkillsPaths: []string{"$CUSTOM_SKILLS_DIR"}, + Resolver: func(s string) (string, error) { + if s == "$CUSTOM_SKILLS_DIR" { + return tmp, nil + } + return s, errors.New("unknown") + }, + }) + + found := false + for _, s := range allSkills { + if s.Name == "envvar-skill" { + found = true + } + } + require.True(t, found, "DiscoverFromConfig must expand $VAR via Resolver") +} diff --git a/internal/skills/skills.go b/internal/skills/skills.go index 7071ba61820158a5324a48fbb5faf5cc89bd5f38..9b431214c809843c3356585f9f729a8d597590b5 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -36,15 +36,17 @@ var ( // Skill represents a parsed SKILL.md file. type Skill struct { - Name string `yaml:"name" json:"name"` - Description string `yaml:"description" json:"description"` - License string `yaml:"license,omitempty" json:"license,omitempty"` - Compatibility string `yaml:"compatibility,omitempty" json:"compatibility,omitempty"` - Metadata map[string]string `yaml:"metadata,omitempty" json:"metadata,omitempty"` - Instructions string `yaml:"-" json:"instructions"` - Path string `yaml:"-" json:"path"` - SkillFilePath string `yaml:"-" json:"skill_file_path"` - Builtin bool `yaml:"-" json:"builtin"` + Name string `yaml:"name" json:"name"` + Description string `yaml:"description" json:"description"` + UserInvocable bool `yaml:"user-invocable" json:"user_invocable"` + DisableModelInvocation bool `yaml:"disable-model-invocation" json:"disable_model_invocation"` + License string `yaml:"license,omitempty" json:"license,omitempty"` + Compatibility string `yaml:"compatibility,omitempty" json:"compatibility,omitempty"` + Metadata map[string]string `yaml:"metadata,omitempty" json:"metadata,omitempty"` + Instructions string `yaml:"-" json:"instructions"` + Path string `yaml:"-" json:"path"` + SkillFilePath string `yaml:"-" json:"skill_file_path"` + Builtin bool `yaml:"-" json:"builtin"` } // DiscoveryState represents the outcome of discovering a single skill file. @@ -293,6 +295,7 @@ func DiscoverWithStates(paths []string) ([]*Skill, []*SkillState) { } // ToPromptXML generates XML for injection into the system prompt. +// Skills with DisableModelInvocation set to true are excluded. func ToPromptXML(skills []*Skill) string { if len(skills) == 0 { return "" @@ -301,6 +304,10 @@ func ToPromptXML(skills []*Skill) string { var sb strings.Builder sb.WriteString("\n") for _, s := range skills { + // Skip skills that have disable-model-invocation set + if s.DisableModelInvocation { + continue + } sb.WriteString(" \n") fmt.Fprintf(&sb, " %s\n", escape(s.Name)) fmt.Fprintf(&sb, " %s\n", escape(s.Description)) @@ -314,6 +321,20 @@ func ToPromptXML(skills []*Skill) string { return sb.String() } +// FormatInvocation generates XML for a skill when invoked as a user command. +func (s *Skill) FormatInvocation() string { + var sb strings.Builder + sb.WriteString("\n") + fmt.Fprintf(&sb, " %s\n", escape(s.Name)) + fmt.Fprintf(&sb, " %s\n", escape(s.Description)) + fmt.Fprintf(&sb, " %s\n", escape(s.SkillFilePath)) + sb.WriteString(" \n") + sb.WriteString(escape(s.Instructions)) + sb.WriteString("\n \n") + sb.WriteString("") + return sb.String() +} + func escape(s string) string { return promptReplacer.Replace(s) } diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index f11a48c37f58fe727da97a90939efdedd0189250..fc78b8dc62a0b855cdc0f37861219128ff1dc9c8 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -312,6 +312,20 @@ func TestToPromptXML(t *testing.T) { require.Contains(t, xml, "&") // XML escaping } +func TestToPromptXMLDisableModelInvocation(t *testing.T) { + t.Parallel() + + skills := []*Skill{ + {Name: "visible-skill", Description: "This one appears.", SkillFilePath: "/skills/visible/SKILL.md"}, + {Name: "hidden-skill", Description: "This one is hidden.", SkillFilePath: "/skills/hidden/SKILL.md", DisableModelInvocation: true}, + } + + xml := ToPromptXML(skills) + + require.Contains(t, xml, "visible-skill") + require.NotContains(t, xml, "hidden-skill") +} + func TestToPromptXMLEmpty(t *testing.T) { t.Parallel() require.Empty(t, ToPromptXML(nil)) diff --git a/internal/ui/chat/user.go b/internal/ui/chat/user.go index 2aca4495b2dd15e115416a564daa072c4b1421a8..f355ad0fd545bafa10994f8aea421b185f5eceac 100644 --- a/internal/ui/chat/user.go +++ b/internal/ui/chat/user.go @@ -1,6 +1,7 @@ package chat import ( + "encoding/xml" "strings" tea "charm.land/bubbletea/v2" @@ -12,6 +13,14 @@ import ( "github.com/charmbracelet/crush/internal/ui/styles" ) +// skillInvocation represents the XML structure for a loaded skill. +type skillInvocation struct { + Name string `xml:"name"` + Description string `xml:"description"` + Location string `xml:"location"` + Instructions string `xml:"instructions"` +} + // UserMessageItem represents a user message in the chat UI. type UserMessageItem struct { *list.Versioned @@ -54,13 +63,19 @@ func (m *UserMessageItem) RawRender(width int) string { return m.renderHighlighted(content, cappedWidth, height) } + msgContent := strings.TrimSpace(m.message.Content().Text) + + // Check if this is a skill invocation (loaded_skill XML) + if strings.HasPrefix(msgContent, "") { + content = m.renderSkillInvocation(msgContent, cappedWidth) + height = lipgloss.Height(content) + m.setCachedRender(content, cappedWidth, height) + return m.renderHighlighted(content, cappedWidth, height) + } + renderer := common.MarkdownRenderer(m.sty, cappedWidth) - msgContent := strings.TrimSpace(m.message.Content().Text) - mu := common.LockMarkdownRenderer(renderer) - mu.Lock() result, err := renderer.Render(msgContent) - mu.Unlock() if err != nil { content = msgContent } else { @@ -81,6 +96,22 @@ func (m *UserMessageItem) RawRender(width int) string { return m.renderHighlighted(content, cappedWidth, height) } +// renderSkillInvocation renders a loaded_skill XML as a special UI element. +func (m *UserMessageItem) renderSkillInvocation(content string, width int) string { + var skill skillInvocation + if err := xml.Unmarshal([]byte(content), &skill); err != nil { + // If parsing fails, just render as markdown + renderer := common.MarkdownRenderer(m.sty, width) + result, err := renderer.Render(content) + if err != nil { + return content + } + return strings.TrimSuffix(result, "\n") + } + + return toolOutputSkillContent(m.sty, skill.Name, skill.Description) +} + // Render implements MessageItem. func (m *UserMessageItem) Render(width int) string { // Bypass the prefix cache while a highlight range is active so diff --git a/internal/ui/dialog/actions.go b/internal/ui/dialog/actions.go index a2de6513c13a9d00febd8ca510472542e687ce4a..4ec9a59316a703ab49ba571cbf29446d6af3829d 100644 --- a/internal/ui/dialog/actions.go +++ b/internal/ui/dialog/actions.go @@ -14,6 +14,7 @@ import ( "github.com/charmbracelet/crush/internal/oauth" "github.com/charmbracelet/crush/internal/permission" "github.com/charmbracelet/crush/internal/session" + "github.com/charmbracelet/crush/internal/skills" "github.com/charmbracelet/crush/internal/ui/common" "github.com/charmbracelet/crush/internal/ui/util" ) @@ -71,6 +72,7 @@ type ( Content string Arguments []commands.Argument Args map[string]string // Actual argument values + Skill *skills.Skill // Set when this is a skill command } // ActionRunMCPPrompt is a message to run a custom command. ActionRunMCPPrompt struct { diff --git a/internal/ui/dialog/commands.go b/internal/ui/dialog/commands.go index a222124381e9058853c2170afb10016ce83a64ca..185185e5ea3ffc15744d75e605bf8b17b94a8081 100644 --- a/internal/ui/dialog/commands.go +++ b/internal/ui/dialog/commands.go @@ -393,6 +393,7 @@ func (c *Commands) setCommandItems(commandType CommandType) { action := ActionRunCustomCommand{ Content: cmd.Content, Arguments: cmd.Arguments, + Skill: cmd.Skill, } commandItems = append(commandItems, NewCommandItem(c.com.Styles, "custom_"+cmd.ID, cmd.Name, "", action)) } diff --git a/internal/ui/model/session_test.go b/internal/ui/model/session_test.go new file mode 100644 index 0000000000000000000000000000000000000000..7aa1a3f9f359fcf8255739bdc094d43d31f530fb --- /dev/null +++ b/internal/ui/model/session_test.go @@ -0,0 +1,142 @@ +package model + +import ( + "strings" + "testing" + + "charm.land/lipgloss/v2" + "github.com/charmbracelet/crush/internal/history" + "github.com/charmbracelet/crush/internal/ui/styles" + "github.com/stretchr/testify/require" +) + +func TestFileList(t *testing.T) { + t.Parallel() + + t.Run("empty stats no truncation needed", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "main.go"}, Additions: 0, Deletions: 0}, + } + got := fileList(st, "/", files, 30, 10) + require.Contains(t, stripANSI(got), "main.go") + }) + + t.Run("empty stats path truncates to width", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "/very/long/path/to/some/deeply/nested/file.go"}, Additions: 0, Deletions: 0}, + } + got := fileList(st, "/", files, 10, 10) + plain := stripANSI(got) + for _, line := range strings.Split(plain, "\n") { + require.LessOrEqual(t, lipgloss.Width(line), 10, "line exceeds sidebar width: %q", line) + } + }) + + t.Run("with additions and deletions fits within width", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "main.go"}, Additions: 5, Deletions: 3}, + } + got := fileList(st, "/", files, 20, 10) + plain := stripANSI(got) + require.Contains(t, plain, "+5") + require.Contains(t, plain, "-3") + for _, line := range strings.Split(plain, "\n") { + require.LessOrEqual(t, lipgloss.Width(line), 20, "line exceeds sidebar width: %q", line) + } + }) + + t.Run("narrow width with stats clamps path to zero", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "main.go"}, Additions: 100, Deletions: 200}, + } + got := fileList(st, "/", files, 5, 10) + plain := stripANSI(got) + require.NotContains(t, plain, "main.go") + require.Equal(t, "+100 -200", strings.TrimSpace(plain)) + }) + + t.Run("single addition only", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "main.go"}, Additions: 3, Deletions: 0}, + } + got := fileList(st, "/", files, 20, 10) + plain := stripANSI(got) + require.Contains(t, plain, "+3") + require.NotContains(t, plain, "-0") + for _, line := range strings.Split(plain, "\n") { + require.LessOrEqual(t, lipgloss.Width(line), 20, "line exceeds sidebar width: %q", line) + } + }) + + t.Run("single deletion only", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "main.go"}, Additions: 0, Deletions: 7}, + } + got := fileList(st, "/", files, 20, 10) + plain := stripANSI(got) + require.NotContains(t, plain, "+0") + require.Contains(t, plain, "-7") + for _, line := range strings.Split(plain, "\n") { + require.LessOrEqual(t, lipgloss.Width(line), 20, "line exceeds sidebar width: %q", line) + } + }) + + t.Run("max items zero returns empty", func(t *testing.T) { + t.Parallel() + + st := minimalFileStyles() + files := []SessionFile{ + {FirstVersion: history.File{Path: "main.go"}, Additions: 1, Deletions: 1}, + } + got := fileList(st, "/", files, 20, 0) + require.Empty(t, got) + }) +} + +func minimalFileStyles() *styles.Styles { + st := styles.CharmtonePantera() + st.Files.Path = lipgloss.NewStyle() + st.Files.Additions = lipgloss.NewStyle() + st.Files.Deletions = lipgloss.NewStyle() + st.Files.SectionTitle = lipgloss.NewStyle() + st.Files.EmptyMessage = lipgloss.NewStyle() + st.Files.TruncationHint = lipgloss.NewStyle() + return &st +} + +func stripANSI(s string) string { + var b strings.Builder + esc := false + for i := 0; i < len(s); i++ { + if s[i] == '\x1b' { + esc = true + continue + } + if esc { + if s[i] >= 'a' && s[i] <= 'z' || s[i] >= 'A' && s[i] <= 'Z' { + esc = false + } + continue + } + b.WriteByte(s[i]) + } + return b.String() +} diff --git a/internal/ui/model/ui.go b/internal/ui/model/ui.go index e7de8fd175555c497ef31d3d84d19d87539188d5..2a21fe364e4fc38f567bb550571c486a599ae465 100644 --- a/internal/ui/model/ui.go +++ b/internal/ui/model/ui.go @@ -468,6 +468,10 @@ func (m *UI) loadCustomCommands() tea.Cmd { if err != nil { slog.Error("Failed to load custom commands", "error", err) } + // Append user-invocable skills as commands + skillCommands := commands.LoadSkillCommands() + skillCommands = append(skillCommands, commands.LoadProjectSkillCommands(m.com.Workspace.WorkingDir())...) + customCommands = append(customCommands, skillCommands...) return userCommandsLoadedMsg{Commands: customCommands} } } @@ -1541,6 +1545,10 @@ func (m *UI) handleDialogMsg(msg tea.Msg) tea.Cmd { if msg.Args != nil { content = substituteArgs(content, msg.Args) } + // If this is a skill command, format it using the skill's FormatInvocation method + if msg.Skill != nil { + content = msg.Skill.FormatInvocation() + } cmds = append(cmds, m.sendMessage(content)) m.dialog.CloseFrontDialog() case dialog.ActionRunMCPPrompt: diff --git a/internal/workspace/client_workspace.go b/internal/workspace/client_workspace.go index 09a050f4769ffbc58c2a43b516d3511a1a96c880..074ec52f461791e8d86404508e1ecfe888a8135d 100644 --- a/internal/workspace/client_workspace.go +++ b/internal/workspace/client_workspace.go @@ -2,6 +2,7 @@ package workspace import ( "context" + "errors" "fmt" "log/slog" "strings" @@ -22,6 +23,7 @@ import ( "github.com/charmbracelet/crush/internal/proto" "github.com/charmbracelet/crush/internal/pubsub" "github.com/charmbracelet/crush/internal/session" + "github.com/charmbracelet/crush/internal/skills" "github.com/charmbracelet/x/powernap/pkg/lsp/protocol" ) @@ -32,20 +34,29 @@ import ( type ClientWorkspace struct { client *client.Client - mu sync.RWMutex - ws proto.Workspace + mu sync.RWMutex + ws proto.Workspace + skills *skills.Manager } // NewClientWorkspace creates a new ClientWorkspace that proxies all // operations through the given client SDK. The ws parameter is the -// proto.Workspace snapshot returned by the server at creation time. +// proto.Workspace snapshot returned by the server at creation time. The +// snapshot's Skills field seeds a process-local skills.Manager so the +// TUI sees discovery state before the first SSE event arrives. The +// manager is constructed with WithGlobalMirror because the client +// process represents exactly one workspace and the TUI reads +// skills.GetLatestStates directly at construction time. func NewClientWorkspace(c *client.Client, ws proto.Workspace) *ClientWorkspace { if ws.Config != nil { ws.Config.SetupAgents() } + states := protoToSkillStates(ws.Skills) + mgr := skills.NewManager(nil, nil, states, skills.WithGlobalMirror()) return &ClientWorkspace{ client: c, ws: ws, + skills: mgr, } } @@ -575,7 +586,7 @@ func (w *ClientWorkspace) consumeEvents(evc <-chan any, send func(tea.Msg)) { w.refreshWorkspace() continue } - translated := translateEvent(ev) + translated := w.translateEvent(ev) if translated != nil && send != nil { send(translated) } @@ -587,8 +598,10 @@ func (w *ClientWorkspace) Shutdown() { } // translateEvent converts proto-typed SSE events into the domain types -// that the TUI's Update() method expects. -func translateEvent(ev any) tea.Msg { +// that the TUI's Update() method expects. Skills events also update the +// process-local skills.Manager so callers reading +// skills.GetLatestStates see fresh data. +func (w *ClientWorkspace) translateEvent(ev any) tea.Msg { switch e := ev.(type) { case pubsub.Event[proto.LSPEvent]: return pubsub.Event[LSPEvent]{ @@ -663,6 +676,15 @@ func translateEvent(ev any) tea.Msg { Type: notify.Type(e.Payload.Type), }, } + case pubsub.Event[proto.SkillsEvent]: + states := protoToSkillStates(e.Payload.States) + if w.skills != nil { + w.skills.SetLatestStates(states) + } + return pubsub.Event[skills.Event]{ + Type: e.Type, + Payload: skills.Event{States: states}, + } default: slog.Warn("Unknown event type in translateEvent", "type", fmt.Sprintf("%T", ev)) return nil @@ -822,6 +844,28 @@ func sessionToProto(s session.Session) proto.Session { } } +// protoToSkillStates reconstructs internal skill state slices from +// their wire representation. Non-empty Error strings are turned into +// synthetic error values; the TUI never type-asserts on Err. +func protoToSkillStates(in []proto.SkillState) []*skills.SkillState { + if len(in) == 0 { + return nil + } + out := make([]*skills.SkillState, len(in)) + for i, s := range in { + state := &skills.SkillState{ + Name: s.Name, + Path: s.Path, + State: skills.DiscoveryState(s.State), + } + if s.Error != "" { + state.Err = errors.New(s.Error) + } + out[i] = state + } + return out +} + func todosToProto(todos []session.Todo) []proto.Todo { if len(todos) == 0 { return nil diff --git a/internal/workspace/client_workspace_test.go b/internal/workspace/client_workspace_test.go index 6b0adcdd37215b6b65ff3a88a9d57cb86a764e8f..d88100457ed006fa537e66592fb04a6c190ad214 100644 --- a/internal/workspace/client_workspace_test.go +++ b/internal/workspace/client_workspace_test.go @@ -12,6 +12,8 @@ import ( "github.com/charmbracelet/crush/internal/message" "github.com/charmbracelet/crush/internal/permission" "github.com/charmbracelet/crush/internal/proto" + "github.com/charmbracelet/crush/internal/pubsub" + "github.com/charmbracelet/crush/internal/skills" "github.com/stretchr/testify/require" ) @@ -132,3 +134,80 @@ func TestClientWorkspace_PermissionGrantMapping(t *testing.T) { }) } } + +// TestProtoToSkillStates verifies that the wire representation of skill +// discovery states reconstructs identical values on the client, +// including synthetic errors derived from Error strings. +func TestProtoToSkillStates(t *testing.T) { + t.Parallel() + + in := []proto.SkillState{ + {Name: "ok", Path: "/p/ok", State: proto.SkillStateNormal}, + {Name: "broken", Path: "/p/broken", State: proto.SkillStateError, Error: "bad frontmatter"}, + } + + got := protoToSkillStates(in) + require.Len(t, got, 2) + require.Equal(t, "ok", got[0].Name) + require.Equal(t, skills.StateNormal, got[0].State) + require.NoError(t, got[0].Err) + require.Equal(t, "broken", got[1].Name) + require.Equal(t, skills.StateError, got[1].State) + require.EqualError(t, got[1].Err, "bad frontmatter") +} + +// TestTranslateEvent_Skills verifies that an incoming proto.SkillsEvent +// is converted into pubsub.Event[skills.Event] and that the +// client-process skill cache is updated as a side effect, so callers +// reading skills.GetLatestStates see fresh data after each delta. +func TestTranslateEvent_Skills(t *testing.T) { + // Not parallel - touches the package-level skills cache via the + // manager constructed with WithGlobalMirror. + prev := skills.GetLatestStates() + t.Cleanup(func() { skills.SetLatestStates(prev) }) + + skills.SetLatestStates(nil) + + w := NewClientWorkspace(nil, proto.Workspace{}) + ev := pubsub.Event[proto.SkillsEvent]{ + Type: pubsub.UpdatedEvent, + Payload: proto.SkillsEvent{ + States: []proto.SkillState{ + {Name: "from-server", Path: "/p", State: proto.SkillStateNormal}, + }, + }, + } + + out := w.translateEvent(ev) + got, ok := out.(pubsub.Event[skills.Event]) + require.True(t, ok, "expected pubsub.Event[skills.Event], got %T", out) + require.Len(t, got.Payload.States, 1) + require.Equal(t, "from-server", got.Payload.States[0].Name) + + // Manager (with WithGlobalMirror) propagated to the package cache. + cached := skills.GetLatestStates() + require.Len(t, cached, 1) + require.Equal(t, "from-server", cached[0].Name) +} + +// TestNewClientWorkspace_SeedsSkillsCache verifies that the snapshot in +// proto.Workspace.Skills populates the package-level cache the TUI +// reads at construction time, eliminating the race between TUI startup +// and the first SSE event. +func TestNewClientWorkspace_SeedsSkillsCache(t *testing.T) { + // Not parallel - touches the package-level skills cache. + prev := skills.GetLatestStates() + t.Cleanup(func() { skills.SetLatestStates(prev) }) + + skills.SetLatestStates(nil) + + _ = NewClientWorkspace(nil, proto.Workspace{ + Skills: []proto.SkillState{ + {Name: "seeded", Path: "/p", State: proto.SkillStateNormal}, + }, + }) + + got := skills.GetLatestStates() + require.Len(t, got, 1) + require.Equal(t, "seeded", got[0].Name) +}