From d025403923d4f2fc8523d962f5e9d51a854ac801 Mon Sep 17 00:00:00 2001 From: Christian Rocha Date: Tue, 19 May 2026 15:33:14 -0400 Subject: [PATCH] fix(server): display available skills in client --- internal/agent/coordinator.go | 113 ++++----- internal/app/app.go | 15 +- internal/backend/backend.go | 78 +++++- internal/backend/backend_test.go | 170 +++++++++++++ internal/client/proto.go | 4 + internal/cmd/root.go | 30 ++- internal/proto/proto.go | 4 + internal/proto/skills.go | 28 +++ internal/pubsub/events.go | 1 + internal/server/events.go | 27 +++ internal/server/events_test.go | 39 +++ internal/skills/manager.go | 180 ++++++++++++++ internal/skills/manager_test.go | 249 ++++++++++++++++++++ internal/ui/model/session_test.go | 142 +++++++++++ internal/workspace/client_workspace.go | 56 ++++- internal/workspace/client_workspace_test.go | 79 +++++++ 16 files changed, 1130 insertions(+), 85 deletions(-) create mode 100644 internal/backend/backend_test.go create mode 100644 internal/proto/skills.go create mode 100644 internal/skills/manager.go create mode 100644 internal/skills/manager_test.go create mode 100644 internal/ui/model/session_test.go 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/backend.go b/internal/backend/backend.go index 642b48a9222de132ffd24f9c356d4b7152a38591..8d2ebb017bda61914f6002f68a5a62adeb3bdabe 100644 --- a/internal/backend/backend.go +++ b/internal/backend/backend.go @@ -15,6 +15,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" @@ -47,10 +48,11 @@ type Backend 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 } // New creates a new [Backend]. @@ -106,17 +108,25 @@ 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) } ws := &Workspace{ - App: appWorkspace, - ID: id, - Path: args.Path, - Cfg: cfg, - Env: args.Env, + App: appWorkspace, + ID: id, + Path: args.Path, + Cfg: cfg, + Env: args.Env, + Skills: skillsMgr, } b.workspaces.Set(id, ws) @@ -141,11 +151,53 @@ func (b *Backend) CreateWorkspace(args proto.Workspace) (*Workspace, proto.Works YOLO: cfg.Overrides().SkipPermissionRequests, Config: cfg.Config(), Env: args.Env, + Skills: skillStatesToProto(skillStates), } return ws, result, 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 +} + // DeleteWorkspace shuts down and removes a workspace. If it was the // last workspace, the shutdown callback is invoked. func (b *Backend) DeleteWorkspace(id string) { @@ -195,7 +247,7 @@ func (b *Backend) Shutdown() { 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, @@ -203,4 +255,8 @@ func workspaceToProto(ws *Workspace) proto.Workspace { Debug: cfg.Options.Debug, Config: cfg, } + if ws.Skills != nil { + out.Skills = skillStatesToProto(ws.Skills.States()) + } + return out } diff --git a/internal/backend/backend_test.go b/internal/backend/backend_test.go new file mode 100644 index 0000000000000000000000000000000000000000..b1dabc58540ab0b165378a93b9fad4617a10928b --- /dev/null +++ b/internal/backend/backend_test.go @@ -0,0 +1,170 @@ +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/stretchr/testify/require" +) + +// TestBackend_WorkspaceSkillsIsolation verifies that skill discovery +// state and SSE events are per-workspace, not process-global. This +// guards the structural change in §5 of the plan: two workspaces in the +// same backend process must not see each other's discoveries (either in +// their initial snapshot or in subsequent PublishStates events). +// +// Before that change landed, the package-level latestStates cache and +// the package-level broker meant that: +// - workspace A's PublishStates would arrive on workspace B's SSE +// stream, and +// - the most recent SetLatestStates would silently overwrite the +// other workspace's cache. +// +// This test fails on either scenario. +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) + + wsA, _, err := b.CreateWorkspace(proto.Workspace{ + Path: wdA, + DataDir: filepath.Join(wdA, ".crush"), + }) + require.NoError(t, err) + t.Cleanup(func() { b.DeleteWorkspace(wsA.ID) }) + + wsB, _, err := b.CreateWorkspace(proto.Workspace{ + Path: wdB, + DataDir: filepath.Join(wdB, ".crush"), + }) + require.NoError(t, err) + t.Cleanup(func() { b.DeleteWorkspace(wsB.ID) }) + + 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 213120273e5adb849236ac346ccfb5802d1cf16c..e261b172bd041b719d680ababfda8bd9bf130fd3 100644 --- a/internal/client/proto.go +++ b/internal/client/proto.go @@ -168,6 +168,10 @@ func (c *Client) SubscribeEvents(ctx context.Context, id string) (<-chan any, er var e pubsub.Event[proto.AgentEvent] _ = 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 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/proto/proto.go b/internal/proto/proto.go index 87dafd24abc44dabff608ed6744c17703c244a37..22a503b4879806d8b13d109e079604055ebba78b 100644 --- a/internal/proto/proto.go +++ b/internal/proto/proto.go @@ -21,6 +21,10 @@ type Workspace struct { Version string `json:"version,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 44963e3cfbdefc2ddc4657c293615df5329d885d..689d7242970953f048f539252210b5f33ab1a49a 100644 --- a/internal/pubsub/events.go +++ b/internal/pubsub/events.go @@ -24,6 +24,7 @@ const ( PayloadTypeSession PayloadType = "session" PayloadTypeFile PayloadType = "file" PayloadTypeAgentEvent PayloadType = "agent_event" + 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 2c1401fe1f6a3e7293d4f983fe7aab7ef770439f..e596e2d5866268fba4a4e42a98efbb4971e40f8b 100644 --- a/internal/server/events.go +++ b/internal/server/events.go @@ -14,6 +14,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 @@ -91,6 +92,11 @@ func wrapEvent(ev any) *pubsub.Payload { Type: proto.AgentEventType(e.Payload.Type), }, }) + 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 @@ -224,6 +230,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/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/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/workspace/client_workspace.go b/internal/workspace/client_workspace.go index 82fde1c5bbcf8393d854f98ecff6aa2a64fe0de9..6b3fc362ea923b2092c22ba65de88a92b8675ba2 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, } } @@ -552,7 +563,7 @@ func (w *ClientWorkspace) Subscribe(program *tea.Program) { } for ev := range evc { - translated := translateEvent(ev) + translated := w.translateEvent(ev) if translated != nil { program.Send(translated) } @@ -564,8 +575,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]{ @@ -640,6 +653,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 @@ -792,6 +814,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 43d7e3a0b0554d8028541e91f952797338c3038f..0e51d21b97df4502147b0aca0e1ca0477b196640 100644 --- a/internal/workspace/client_workspace_test.go +++ b/internal/workspace/client_workspace_test.go @@ -5,6 +5,8 @@ import ( "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 +46,80 @@ func TestProtoToMessageToolResult(t *testing.T) { require.Equal(t, `{"file_path":"/tmp/x","content":"hi"}`, tr.Metadata) require.False(t, tr.IsError) } + +// 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) +}