diff --git a/cmd/thv/app/skill_install.go b/cmd/thv/app/skill_install.go index 389ad10a3f..57663ec3c2 100644 --- a/cmd/thv/app/skill_install.go +++ b/cmd/thv/app/skill_install.go @@ -4,6 +4,8 @@ package app import ( + "strings" + "github.com/spf13/cobra" "github.com/stacklok/toolhive/pkg/skills" @@ -11,7 +13,7 @@ import ( var ( skillInstallScope string - skillInstallClient string + skillInstallClientsRaw string skillInstallForce bool skillInstallProjectRoot string skillInstallGroup string @@ -34,7 +36,8 @@ The skill will be fetched from a remote registry and installed locally.`, func init() { skillCmd.AddCommand(skillInstallCmd) - skillInstallCmd.Flags().StringVar(&skillInstallClient, "client", "", "Target client application (e.g. claude-code)") + skillInstallCmd.Flags().StringVar(&skillInstallClientsRaw, "clients", "", + `Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client`) skillInstallCmd.Flags().StringVar(&skillInstallScope, "scope", string(skills.ScopeUser), "Installation scope (user, project)") skillInstallCmd.Flags().BoolVar(&skillInstallForce, "force", false, "Overwrite existing skill directory") skillInstallCmd.Flags().StringVar(&skillInstallProjectRoot, "project-root", "", "Project root path for project-scoped installs") @@ -47,7 +50,7 @@ func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { _, err := c.Install(cmd.Context(), skills.InstallOptions{ Name: args[0], Scope: skills.Scope(skillInstallScope), - Client: skillInstallClient, + Clients: parseSkillInstallClients(skillInstallClientsRaw), Force: skillInstallForce, ProjectRoot: skillInstallProjectRoot, Group: skillInstallGroup, @@ -58,3 +61,23 @@ func skillInstallCmdFunc(cmd *cobra.Command, args []string) error { return nil } + +// parseSkillInstallClients splits a comma-separated --clients flag value. +// Empty input yields nil so the server applies its default client. +func parseSkillInstallClients(raw string) []string { + raw = strings.TrimSpace(raw) + if raw == "" { + return nil + } + parts := strings.Split(raw, ",") + out := make([]string, 0, len(parts)) + for _, p := range parts { + if t := strings.TrimSpace(p); t != "" { + out = append(out, t) + } + } + if len(out) == 0 { + return nil + } + return out +} diff --git a/docs/cli/thv_skill_install.md b/docs/cli/thv_skill_install.md index d8e8703ac8..84ca857c8b 100644 --- a/docs/cli/thv_skill_install.md +++ b/docs/cli/thv_skill_install.md @@ -25,7 +25,7 @@ thv skill install [skill-name] [flags] ### Options ``` - --client string Target client application (e.g. claude-code) + --clients string Comma-separated target client apps (e.g. claude-code,opencode), or "all" for every available client --force Overwrite existing skill directory --group string Group to add the skill to after installation -h, --help help for install diff --git a/docs/server/docs.go b/docs/server/docs.go index 28be938e5f..3394d76499 100644 --- a/docs/server/docs.go +++ b/docs/server/docs.go @@ -2345,9 +2345,13 @@ const docTemplate = `{ "pkg_api_v1.installSkillRequest": { "description": "Request to install a skill", "properties": { - "client": { - "description": "Client is the target client (e.g., \"claude-code\")", - "type": "string" + "clients": { + "description": "Clients lists target client identifiers (e.g., \"claude-code\"),\nor [\"all\"] to target every skill-supporting client.\nOmitting this field installs to all available clients.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false }, "force": { "description": "Force allows overwriting unmanaged skill directories", diff --git a/docs/server/swagger.json b/docs/server/swagger.json index d5708a141c..b6d1508236 100644 --- a/docs/server/swagger.json +++ b/docs/server/swagger.json @@ -2338,9 +2338,13 @@ "pkg_api_v1.installSkillRequest": { "description": "Request to install a skill", "properties": { - "client": { - "description": "Client is the target client (e.g., \"claude-code\")", - "type": "string" + "clients": { + "description": "Clients lists target client identifiers (e.g., \"claude-code\"),\nor [\"all\"] to target every skill-supporting client.\nOmitting this field installs to all available clients.", + "items": { + "type": "string" + }, + "type": "array", + "uniqueItems": false }, "force": { "description": "Force allows overwriting unmanaged skill directories", diff --git a/docs/server/swagger.yaml b/docs/server/swagger.yaml index 8c4673d78c..6382e5df05 100644 --- a/docs/server/swagger.yaml +++ b/docs/server/swagger.yaml @@ -2096,9 +2096,15 @@ components: pkg_api_v1.installSkillRequest: description: Request to install a skill properties: - client: - description: Client is the target client (e.g., "claude-code") - type: string + clients: + description: |- + Clients lists target client identifiers (e.g., "claude-code"), + or ["all"] to target every skill-supporting client. + Omitting this field installs to all available clients. + items: + type: string + type: array + uniqueItems: false force: description: Force allows overwriting unmanaged skill directories type: boolean diff --git a/pkg/api/v1/skills.go b/pkg/api/v1/skills.go index 7b00092f4a..7f39da4b9b 100644 --- a/pkg/api/v1/skills.go +++ b/pkg/api/v1/skills.go @@ -101,7 +101,7 @@ func (s *SkillsRoutes) installSkill(w http.ResponseWriter, r *http.Request) erro Version: req.Version, Scope: req.Scope, ProjectRoot: req.ProjectRoot, - Client: req.Client, + Clients: req.Clients, Force: req.Force, Group: req.Group, }) diff --git a/pkg/api/v1/skills_test.go b/pkg/api/v1/skills_test.go index be2efb0dee..5ad7599923 100644 --- a/pkg/api/v1/skills_test.go +++ b/pkg/api/v1/skills_test.go @@ -298,6 +298,26 @@ func TestSkillsRouter(t *testing.T) { expectedStatus: http.StatusInternalServerError, expectedBody: "Internal Server Error", }, + { + name: "install skill with clients", + method: "POST", + path: "/", + body: `{"name":"my-skill","clients":["claude-code","opencode"]}`, + setupMock: func(svc *skillsmocks.MockSkillService, _ string) { + svc.EXPECT().Install(gomock.Any(), skills.InstallOptions{ + Name: "my-skill", + Clients: []string{"claude-code", "opencode"}, + }).Return(&skills.InstallResult{ + Skill: skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Status: skills.InstallStatusInstalled, + Clients: []string{"claude-code", "opencode"}, + }, + }, nil) + }, + expectedStatus: http.StatusCreated, + expectedBody: `"my-skill"`, + }, // install with version and scope { name: "install skill with version and scope", diff --git a/pkg/api/v1/skills_types.go b/pkg/api/v1/skills_types.go index b87d7c9373..833a9b0aa5 100644 --- a/pkg/api/v1/skills_types.go +++ b/pkg/api/v1/skills_types.go @@ -25,8 +25,10 @@ type installSkillRequest struct { Scope skills.Scope `json:"scope,omitempty"` // ProjectRoot is the project root path for project-scoped installs ProjectRoot string `json:"project_root,omitempty"` - // Client is the target client (e.g., "claude-code") - Client string `json:"client,omitempty"` + // Clients lists target client identifiers (e.g., "claude-code"), + // or ["all"] to target every skill-supporting client. + // Omitting this field installs to all available clients. + Clients []string `json:"clients,omitempty"` // Force allows overwriting unmanaged skill directories Force bool `json:"force,omitempty"` // Group is the group name to add the skill to after installation diff --git a/pkg/skills/client/client.go b/pkg/skills/client/client.go index a7342b2cd4..1285028fd6 100644 --- a/pkg/skills/client/client.go +++ b/pkg/skills/client/client.go @@ -159,8 +159,9 @@ func (c *Client) Install(ctx context.Context, opts skills.InstallOptions) (*skil Version: opts.Version, Scope: opts.Scope, ProjectRoot: opts.ProjectRoot, - Client: opts.Client, + Clients: opts.Clients, Force: opts.Force, + Group: opts.Group, } var resp installResponse diff --git a/pkg/skills/client/client_test.go b/pkg/skills/client/client_test.go index 9dc2d92fd3..abe9c882d4 100644 --- a/pkg/skills/client/client_test.go +++ b/pkg/skills/client/client_test.go @@ -130,14 +130,14 @@ func TestInstall(t *testing.T) { Name: "my-skill", Version: "1.0.0", Scope: skills.ScopeUser, - Client: "claude-code", + Clients: []string{"claude-code"}, Force: true, }, wantBody: installRequest{ Name: "my-skill", Version: "1.0.0", Scope: skills.ScopeUser, - Client: "claude-code", + Clients: []string{"claude-code"}, Force: true, }, response: installResponse{Skill: skills.InstalledSkill{ diff --git a/pkg/skills/client/dto.go b/pkg/skills/client/dto.go index c0b7bf75b3..104f7e2fad 100644 --- a/pkg/skills/client/dto.go +++ b/pkg/skills/client/dto.go @@ -12,8 +12,9 @@ type installRequest struct { Version string `json:"version,omitempty"` Scope skills.Scope `json:"scope,omitempty"` ProjectRoot string `json:"project_root,omitempty"` - Client string `json:"client,omitempty"` + Clients []string `json:"clients,omitempty"` Force bool `json:"force,omitempty"` + Group string `json:"group,omitempty"` } type validateRequest struct { diff --git a/pkg/skills/options.go b/pkg/skills/options.go index 9b60ef17b4..e79f28563e 100644 --- a/pkg/skills/options.go +++ b/pkg/skills/options.go @@ -23,8 +23,8 @@ type InstallOptions struct { Version string `json:"version,omitempty"` // Scope is the installation scope. Scope Scope `json:"scope,omitempty"` - // Client is the target client (e.g., "claude-code"). Empty means first skill-supporting client. - Client string `json:"client,omitempty"` + // Clients lists target clients (e.g., "claude-code"). Empty means first skill-supporting client. + Clients []string `json:"clients,omitempty"` // Force allows overwriting unmanaged skill directories. Force bool `json:"force,omitempty"` // ProjectRoot is the project root path for project-scoped installs. diff --git a/pkg/skills/skillsvc/skillsvc.go b/pkg/skills/skillsvc/skillsvc.go index ef29fbe74a..c3623d659f 100644 --- a/pkg/skills/skillsvc/skillsvc.go +++ b/pkg/skills/skillsvc/skillsvc.go @@ -13,6 +13,7 @@ import ( "net/http" "os" "path/filepath" + "slices" "strings" "sync" "time" @@ -24,6 +25,7 @@ import ( "github.com/stacklok/toolhive-core/httperr" ociskills "github.com/stacklok/toolhive-core/oci/skills" regtypes "github.com/stacklok/toolhive-core/registry/types" + "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/groups" "github.com/stacklok/toolhive/pkg/skills" "github.com/stacklok/toolhive/pkg/skills/gitresolver" @@ -745,14 +747,12 @@ func (s *service) installFromGit( unlock := s.locks.lock(opts.Name, scope, opts.ProjectRoot) defer unlock() - clientType := s.resolveClient(opts.Client) - - targetDir, err := s.pathResolver.GetSkillPath(clientType, opts.Name, scope, opts.ProjectRoot) + clientTypes, clientDirs, err := s.resolveAndValidateClients(opts, opts.Name, scope, opts.ProjectRoot) if err != nil { - return nil, fmt.Errorf("resolving skill path: %w", err) + return nil, err } - return s.applyGitInstall(ctx, opts, scope, clientType, targetDir, resolved.Files) + return s.applyGitInstall(ctx, opts, scope, clientTypes, clientDirs, resolved.Files) } // applyGitInstall handles the create/upgrade/no-op logic for a git-based skill @@ -762,67 +762,142 @@ func (s *service) applyGitInstall( ctx context.Context, opts skills.InstallOptions, scope skills.Scope, - clientType string, - targetDir string, + clientTypes []string, + clientDirs map[string]string, files []gitresolver.FileEntry, ) (*skills.InstallResult, error) { existing, storeErr := s.store.Get(ctx, opts.Name, scope, opts.ProjectRoot) isNotFound := errors.Is(storeErr, storage.ErrNotFound) - - switch { - case storeErr != nil && !isNotFound: + if storeErr != nil && !isNotFound { return nil, fmt.Errorf("checking existing skill: %w", storeErr) + } + if !isNotFound { + return s.applyGitInstallExisting(ctx, opts, scope, existing, clientTypes, clientDirs, files) + } + return s.applyGitInstallFresh(ctx, opts, scope, clientTypes, clientDirs, files) +} - case storeErr == nil && existing.Digest == opts.Digest: - // Same commit hash — already installed, no-op. +func (s *service) applyGitInstallExisting( + ctx context.Context, + opts skills.InstallOptions, + scope skills.Scope, + existing skills.InstalledSkill, + clientTypes []string, + clientDirs map[string]string, + files []gitresolver.FileEntry, +) (*skills.InstallResult, error) { + if existing.Digest != opts.Digest { + allClients, allDirs, err := s.expandToExistingClients( + existing.Clients, clientTypes, clientDirs, opts.Name, scope, opts.ProjectRoot) + if err != nil { + return nil, err + } + return s.gitWriteMultiAndPersist(ctx, opts, scope, allClients, allDirs, files, + allClients, nil, true, true) + } + clientsExplicit := len(opts.Clients) > 0 + if clientsContainAll(existing.Clients, clientTypes) || + (len(existing.Clients) == 0 && len(clientTypes) <= 1 && !clientsExplicit) { + return &skills.InstallResult{Skill: existing}, nil + } + toWrite := missingClients(existing.Clients, clientTypes) + if len(toWrite) == 0 { return &skills.InstallResult{Skill: existing}, nil + } + for _, ct := range toWrite { + dir := filepath.Clean(clientDirs[ct]) + if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] + return nil, httperr.WithCode( + fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), + http.StatusConflict, + ) + } + } + return s.gitWriteMultiAndPersist(ctx, opts, scope, clientTypes, clientDirs, files, + toWrite, existing.Clients, true, false) +} - case storeErr == nil: - // Different commit — upgrade. - return s.writeAndPersistGitSkill(ctx, opts, scope, clientType, targetDir, files, existing.Clients, true) +func missingClients(existing, requested []string) []string { + var out []string + for _, ct := range requested { + if !slices.Contains(existing, ct) { + out = append(out, ct) + } + } + return out +} - default: - // Fresh install — check for unmanaged directory on disk. - if _, statErr := os.Stat(targetDir); statErr == nil && !opts.Force { +func (s *service) applyGitInstallFresh( + ctx context.Context, + opts skills.InstallOptions, + scope skills.Scope, + clientTypes []string, + clientDirs map[string]string, + files []gitresolver.FileEntry, +) (*skills.InstallResult, error) { + for _, ct := range clientTypes { + dir := filepath.Clean(clientDirs[ct]) + if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] return nil, httperr.WithCode( - fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", targetDir), + fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), http.StatusConflict, ) } - return s.writeAndPersistGitSkill(ctx, opts, scope, clientType, targetDir, files, nil, false) } + return s.gitWriteMultiAndPersist(ctx, opts, scope, clientTypes, clientDirs, files, + clientTypes, nil, false, false) } -// writeAndPersistGitSkill writes git skill files to disk, verifies the result, -// and creates or updates the DB record. When existingClients is non-nil, the -// record is updated (upgrade); otherwise a new record is created. -func (s *service) writeAndPersistGitSkill( +// gitWriteMultiAndPersist writes git files to the given client directories, +// verifies each tree, then creates or updates the store record. On failure +// after any write, previously written directories in this call are removed. +func (s *service) gitWriteMultiAndPersist( ctx context.Context, opts skills.InstallOptions, scope skills.Scope, - clientType string, - targetDir string, + allRequested []string, + clientDirs map[string]string, files []gitresolver.FileEntry, + dirsToWrite []string, existingClients []string, - isUpgrade bool, + isUpgrade, writeAggressive bool, ) (*skills.InstallResult, error) { - if writeErr := gitresolver.WriteFiles(files, targetDir, isUpgrade || opts.Force); writeErr != nil { - return nil, fmt.Errorf("writing git skill: %w", writeErr) - } - // Defense in depth: verify the extracted directory post-write. - if checkErr := skills.CheckFilesystem(targetDir); checkErr != nil { - _ = s.installer.Remove(targetDir) - return nil, fmt.Errorf("post-extraction verification failed: %w", checkErr) + var written []string + for _, ct := range dirsToWrite { + dir := filepath.Clean(clientDirs[ct]) + writeMode := opts.Force + if writeAggressive { + writeMode = true + } + if writeErr := gitresolver.WriteFiles(files, dir, writeMode); writeErr != nil { + for _, wct := range written { + _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) + } + return nil, fmt.Errorf("writing git skill: %w", writeErr) + } + if checkErr := skills.CheckFilesystem(dir); checkErr != nil { + _ = s.installer.Remove(dir) + for _, wct := range written { + _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) + } + return nil, fmt.Errorf("post-extraction verification failed: %w", checkErr) + } + written = append(written, ct) } - sk := buildInstalledSkill(opts, scope, clientType, existingClients) + + sk := buildInstalledSkill(opts, scope, allRequested, existingClients) if isUpgrade { if err := s.store.Update(ctx, sk); err != nil { - _ = s.installer.Remove(targetDir) + for _, wct := range written { + _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) + } return nil, err } } else { if err := s.store.Create(ctx, sk); err != nil { - _ = s.installer.Remove(targetDir) + for _, wct := range written { + _ = s.installer.Remove(filepath.Clean(clientDirs[wct])) + } return nil, err } } @@ -1122,136 +1197,326 @@ func (s *service) extractOCIContent(ctx context.Context, d digest.Digest) ([]byt return layerData, skillConfig, nil } -// installWithExtraction handles the full install flow: managed/unmanaged -// detection, extraction, and DB record creation or update. -func (s *service) installWithExtraction( - ctx context.Context, opts skills.InstallOptions, scope skills.Scope, -) (*skills.InstallResult, error) { +// clientsAllSentinel is the reserved value that expands to all skill-supporting clients. +const clientsAllSentinel = "all" + +// resolveAndValidateClients returns the deduplicated client list and a map of +// client identifier to install directory. Empty opts.Clients (or the sentinel +// value "all") expands to every skill-supporting client returned by the path resolver. +func (s *service) resolveAndValidateClients( + opts skills.InstallOptions, + skillName string, + scope skills.Scope, + projectRoot string, +) ([]string, map[string]string, error) { if s.pathResolver == nil { - return nil, httperr.WithCode( - fmt.Errorf("path resolver is required for extraction-based installs"), + return nil, nil, httperr.WithCode( + fmt.Errorf("path resolver is required for skill installs"), http.StatusInternalServerError, ) } - clientType := s.resolveClient(opts.Client) + var requested []string + switch { + case len(opts.Clients) == 0 || (len(opts.Clients) == 1 && strings.EqualFold(opts.Clients[0], clientsAllSentinel)): + clients := s.pathResolver.ListSkillSupportingClients() + if len(clients) == 0 { + return nil, nil, httperr.WithCode( + errors.New("no skill-supporting clients configured"), + http.StatusInternalServerError, + ) + } + requested = clients + default: + for _, c := range opts.Clients { + if c == "" { + return nil, nil, httperr.WithCode( + errors.New("clients entries must be non-empty strings"), + http.StatusBadRequest, + ) + } + if strings.EqualFold(c, clientsAllSentinel) { + return nil, nil, httperr.WithCode( + fmt.Errorf("%q cannot be combined with other client names", clientsAllSentinel), + http.StatusBadRequest, + ) + } + } + requested = dedupeStringsPreserveOrder(opts.Clients) + } + + paths := make(map[string]string, len(requested)) + for _, ct := range requested { + dir, err := s.pathResolver.GetSkillPath(ct, skillName, scope, projectRoot) + if err != nil { + if errors.Is(err, client.ErrUnsupportedClientType) || errors.Is(err, client.ErrSkillsNotSupported) { + return nil, nil, httperr.WithCode( + fmt.Errorf("invalid client %q: %w", ct, err), + http.StatusBadRequest, + ) + } + return nil, nil, fmt.Errorf("resolving skill path for client %q: %w", ct, err) + } + dir = filepath.Clean(dir) + if err := validateResolvedDir(dir); err != nil { + return nil, nil, fmt.Errorf("resolved path for client %q is unsafe: %w", ct, err) + } + paths[ct] = dir + } + return requested, paths, nil +} + +// expandToExistingClients merges existingClients into requestedClients and +// resolves paths for any existing client not already in clientDirs. This +// ensures upgrades write new files to all clients, not just the requested set. +func (s *service) expandToExistingClients( + existingClients, requestedClients []string, + clientDirs map[string]string, + skillName string, scope skills.Scope, projectRoot string, +) ([]string, map[string]string, error) { + allClients := mergeClientLists(requestedClients, existingClients) + allDirs := make(map[string]string, len(allClients)) + for k, v := range clientDirs { + allDirs[k] = v + } + for _, ct := range allClients { + if _, ok := allDirs[ct]; ok { + continue + } + dir, err := s.pathResolver.GetSkillPath(ct, skillName, scope, projectRoot) + if err != nil { + return nil, nil, fmt.Errorf("resolving skill path for existing client %q: %w", ct, err) + } + dir = filepath.Clean(dir) + if err := validateResolvedDir(dir); err != nil { + return nil, nil, fmt.Errorf("resolved path for client %q is unsafe: %w", ct, err) + } + allDirs[ct] = dir + } + return allClients, allDirs, nil +} + +// validateResolvedDir ensures a directory path is absolute and free of +// path-traversal segments. Callers must pass a filepath.Clean'd value. +func validateResolvedDir(dir string) error { + if !filepath.IsAbs(dir) { + return fmt.Errorf("path must be absolute, got %q", dir) + } + for _, seg := range strings.Split(filepath.ToSlash(dir), "/") { + if seg == ".." { + return fmt.Errorf("path contains traversal segment: %q", dir) + } + } + return nil +} + +func dedupeStringsPreserveOrder(in []string) []string { + seen := make(map[string]struct{}, len(in)) + out := make([]string, 0, len(in)) + for _, s := range in { + if _, ok := seen[s]; ok { + continue + } + seen[s] = struct{}{} + out = append(out, s) + } + return out +} + +// clientsContainAll reports whether every value in requested appears in existing. +func clientsContainAll(existing, requested []string) bool { + for _, r := range requested { + if !slices.Contains(existing, r) { + return false + } + } + return true +} + +// mergeClientLists returns existing followed by any requested entries not already present. +func mergeClientLists(existing, requested []string) []string { + out := make([]string, len(existing)) + copy(out, existing) + seen := make(map[string]struct{}, len(existing)+len(requested)) + for _, c := range existing { + seen[c] = struct{}{} + } + for _, c := range requested { + if _, ok := seen[c]; ok { + continue + } + seen[c] = struct{}{} + out = append(out, c) + } + if len(out) == 0 { + return nil + } + return out +} - targetDir, err := s.pathResolver.GetSkillPath(clientType, opts.Name, scope, opts.ProjectRoot) +// installWithExtraction handles the full install flow: managed/unmanaged +// detection, extraction, and DB record creation or update. +func (s *service) installWithExtraction( + ctx context.Context, opts skills.InstallOptions, scope skills.Scope, +) (*skills.InstallResult, error) { + clientTypes, clientDirs, err := s.resolveAndValidateClients(opts, opts.Name, scope, opts.ProjectRoot) if err != nil { - return nil, fmt.Errorf("resolving skill path: %w", err) + return nil, err } - // Check store for existing managed record. existing, storeErr := s.store.Get(ctx, opts.Name, scope, opts.ProjectRoot) isNotFound := errors.Is(storeErr, storage.ErrNotFound) - - switch { - case storeErr != nil && !isNotFound: - // Unexpected store error. + if storeErr != nil && !isNotFound { return nil, fmt.Errorf("checking existing skill: %w", storeErr) + } - case storeErr == nil && existing.Digest == opts.Digest: - // Same digest — already installed, no-op. + if isExtractionNoOp(existing, storeErr, opts, clientTypes) { return &skills.InstallResult{Skill: existing}, nil + } - case storeErr == nil: - // Different digest — upgrade path. - return s.upgradeSkill(ctx, opts, scope, clientType, targetDir, existing) + digestMatches := storeErr == nil && existing.Digest == opts.Digest + if digestMatches && storeErr == nil { + return s.installExtractionSameDigestNewClients(ctx, opts, scope, existing, clientTypes, clientDirs) + } - default: - // Not found in store — check for unmanaged directory. - return s.freshInstall(ctx, opts, scope, clientType, targetDir) + if storeErr == nil { + return s.installExtractionUpgradeDigest(ctx, opts, scope, existing, clientTypes, clientDirs) + } + + return s.installExtractionFresh(ctx, opts, scope, clientTypes, clientDirs) +} + +// isExtractionNoOp reports whether the install can be short-circuited because +// the same digest and all requested clients are already present. Legacy store +// rows (empty Clients slice) are treated as satisfied only when the user did +// not explicitly specify --clients. +func isExtractionNoOp(existing skills.InstalledSkill, storeErr error, opts skills.InstallOptions, clientTypes []string) bool { + if storeErr != nil || existing.Digest != opts.Digest { + return false } + if clientsContainAll(existing.Clients, clientTypes) { + return true + } + return len(existing.Clients) == 0 && len(clientTypes) <= 1 && len(opts.Clients) == 0 } -// upgradeSkill handles re-extraction when the digest differs from the stored record. -func (s *service) upgradeSkill( +func (s *service) installExtractionSameDigestNewClients( ctx context.Context, opts skills.InstallOptions, scope skills.Scope, - clientType, targetDir string, existing skills.InstalledSkill, + clientTypes []string, + clientDirs map[string]string, ) (*skills.InstallResult, error) { - if _, err := s.installer.Extract(opts.LayerData, targetDir, true); err != nil { - return nil, fmt.Errorf("extracting skill upgrade: %w", err) + toWrite := missingClients(existing.Clients, clientTypes) + if len(toWrite) == 0 { + return &skills.InstallResult{Skill: existing}, nil } - - sk := buildInstalledSkill(opts, scope, clientType, existing.Clients) + var written []string + for _, ct := range toWrite { + dir := filepath.Clean(clientDirs[ct]) + if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] + removeSkillDirs(s.installer, clientDirs, written) + return nil, httperr.WithCode( + fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), + http.StatusConflict, + ) + } + if _, exErr := s.installer.Extract(opts.LayerData, dir, opts.Force); exErr != nil { + removeSkillDirs(s.installer, clientDirs, written) + return nil, fmt.Errorf("extracting skill: %w", exErr) + } + written = append(written, ct) + } + sk := buildInstalledSkill(opts, scope, clientTypes, existing.Clients) if err := s.store.Update(ctx, sk); err != nil { - // Rollback: clean up extracted files since the store record wasn't updated. - _ = s.installer.Remove(targetDir) + removeSkillDirs(s.installer, clientDirs, written) return nil, err } return &skills.InstallResult{Skill: sk}, nil } -// freshInstall handles first-time installation when no store record exists. -func (s *service) freshInstall( +func removeSkillDirs(inst skills.Installer, clientDirs map[string]string, clients []string) { + for _, ct := range clients { + _ = inst.Remove(filepath.Clean(clientDirs[ct])) + } +} + +func (s *service) installExtractionUpgradeDigest( ctx context.Context, opts skills.InstallOptions, scope skills.Scope, - clientType, targetDir string, + existing skills.InstalledSkill, + clientTypes []string, + clientDirs map[string]string, ) (*skills.InstallResult, error) { - // Check for unmanaged directory on disk. - if _, statErr := os.Stat(targetDir); statErr == nil && !opts.Force { - return nil, httperr.WithCode( - fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", targetDir), - http.StatusConflict, - ) + allClients, allDirs, err := s.expandToExistingClients( + existing.Clients, clientTypes, clientDirs, opts.Name, scope, opts.ProjectRoot) + if err != nil { + return nil, err } - - if _, err := s.installer.Extract(opts.LayerData, targetDir, opts.Force); err != nil { - return nil, fmt.Errorf("extracting skill: %w", err) + var written []string + for _, ct := range allClients { + dir := filepath.Clean(allDirs[ct]) + if _, exErr := s.installer.Extract(opts.LayerData, dir, true); exErr != nil { + removeSkillDirs(s.installer, allDirs, written) + return nil, fmt.Errorf("extracting skill upgrade: %w", exErr) + } + written = append(written, ct) } - - sk := buildInstalledSkill(opts, scope, clientType, nil) - if err := s.store.Create(ctx, sk); err != nil { - // Rollback: clean up extracted files since the store record wasn't created. - _ = s.installer.Remove(targetDir) + sk := buildInstalledSkill(opts, scope, allClients, nil) + if err := s.store.Update(ctx, sk); err != nil { + removeSkillDirs(s.installer, allDirs, allClients) return nil, err } return &skills.InstallResult{Skill: sk}, nil } -// resolveClient returns the provided client type, or falls back to the first -// skill-supporting client from the path resolver. -func (s *service) resolveClient(clientType string) string { - if clientType != "" { - return clientType +func (s *service) installExtractionFresh( + ctx context.Context, + opts skills.InstallOptions, + scope skills.Scope, + clientTypes []string, + clientDirs map[string]string, +) (*skills.InstallResult, error) { + for _, ct := range clientTypes { + dir := filepath.Clean(clientDirs[ct]) + if _, statErr := os.Stat(dir); statErr == nil && !opts.Force { // lgtm[go/path-injection] + return nil, httperr.WithCode( + fmt.Errorf("directory %q exists but is not managed by ToolHive; use force to overwrite", dir), + http.StatusConflict, + ) + } } - if s.pathResolver != nil { - clients := s.pathResolver.ListSkillSupportingClients() - if len(clients) > 0 { - return clients[0] + var written []string + for _, ct := range clientTypes { + dir := filepath.Clean(clientDirs[ct]) + if _, exErr := s.installer.Extract(opts.LayerData, dir, opts.Force); exErr != nil { + removeSkillDirs(s.installer, clientDirs, written) + return nil, fmt.Errorf("extracting skill: %w", exErr) } + written = append(written, ct) } - return "" + sk := buildInstalledSkill(opts, scope, clientTypes, nil) + if err := s.store.Create(ctx, sk); err != nil { + for _, ct := range clientTypes { + _ = s.installer.Remove(filepath.Clean(clientDirs[ct])) + } + return nil, err + } + return &skills.InstallResult{Skill: sk}, nil } // buildInstalledSkill constructs an InstalledSkill from install options. +// requestedClientTypes is the set of clients targeted by this install; they +// are merged with existingClients for the persisted Clients field. func buildInstalledSkill( opts skills.InstallOptions, scope skills.Scope, - clientType string, + requestedClientTypes []string, existingClients []string, ) skills.InstalledSkill { - clients := func() []string { - if len(existingClients) > 0 { - for _, c := range existingClients { - if c == clientType { - return existingClients - } - } - // Defensive copy to avoid mutating the caller's slice. - newClients := make([]string, len(existingClients), len(existingClients)+1) - copy(newClients, existingClients) - return append(newClients, clientType) - } - if clientType != "" { - return []string{clientType} - } - return nil - }() + clients := mergeClientLists(existingClients, requestedClientTypes) return skills.InstalledSkill{ Metadata: skills.SkillMetadata{ diff --git a/pkg/skills/skillsvc/skillsvc_test.go b/pkg/skills/skillsvc/skillsvc_test.go index 5bab335567..d642c6a1ba 100644 --- a/pkg/skills/skillsvc/skillsvc_test.go +++ b/pkg/skills/skillsvc/skillsvc_test.go @@ -28,6 +28,7 @@ import ( ociskills "github.com/stacklok/toolhive-core/oci/skills" ocimocks "github.com/stacklok/toolhive-core/oci/skills/mocks" regtypes "github.com/stacklok/toolhive-core/registry/types" + "github.com/stacklok/toolhive/pkg/client" "github.com/stacklok/toolhive/pkg/groups" groupmocks "github.com/stacklok/toolhive/pkg/groups/mocks" regmocks "github.com/stacklok/toolhive/pkg/registry/mocks" @@ -376,12 +377,160 @@ func TestInstallWithExtraction(t *testing.T) { _, err := svc.Install(t.Context(), skills.InstallOptions{ Name: "my-skill", LayerData: layerData, - Client: "custom-client", + Clients: []string{"custom-client"}, Digest: "sha256:abc", }) require.NoError(t, err) }) + t.Run("multiple clients fresh install", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) + inst.EXPECT().Extract(layerData, dirA, false).Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirB, false).Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.ElementsMatch(t, []string{"claude-code", "opencode"}, sk.Clients) + return nil + }) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", "opencode"}, + }) + require.NoError(t, err) + }) + + t.Run("same digest adds second client without re-extracting first", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a") + dirB := filepath.Join(t.TempDir(), "b") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:abc", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirB, false).Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + store.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.ElementsMatch(t, []string{"claude-code", "opencode"}, sk.Clients) + return nil + }) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", "opencode"}, + }) + require.NoError(t, err) + }) + + t.Run("invalid client returns bad request", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + + pr.EXPECT().GetSkillPath("not-a-real-client", "my-skill", skills.ScopeUser, "").Return("", fmt.Errorf("%w: not-a-real-client", client.ErrUnsupportedClientType)) + + svc := New(store, WithPathResolver(pr)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"not-a-real-client"}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) + }) + + t.Run("empty string in clients list returns bad request", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + + svc := New(store, WithPathResolver(pr)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", ""}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) + }) + + t.Run("clients all sentinel expands to every skill-supporting client", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + pr.EXPECT().ListSkillSupportingClients().Return([]string{"claude-code", "opencode"}) + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) + inst.EXPECT().Extract(layerData, dirA, false).Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirB, false).Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.ElementsMatch(t, []string{"claude-code", "opencode"}, sk.Clients) + return nil + }) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"all"}, + }) + require.NoError(t, err) + }) + + t.Run("all sentinel mixed with named client returns bad request", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + + svc := New(store, WithPathResolver(pr)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"all", "opencode"}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusBadRequest, httperr.Code(err)) + }) + t.Run("fresh install rolls back extraction on store.Create failure", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -438,6 +587,274 @@ func TestInstallWithExtraction(t *testing.T) { require.Error(t, err) assert.Contains(t, err.Error(), "db update error") }) + + t.Run("multi-client fresh install rolls back first client on second extract failure", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, ""). + Return(skills.InstalledSkill{}, storage.ErrNotFound) + inst.EXPECT().Extract(layerData, dirA, false). + Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirB, false). + Return(nil, fmt.Errorf("disk full")) + inst.EXPECT().Remove(dirA).Return(nil) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", "opencode"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "disk full") + }) + + t.Run("upgrade digest rolls back written clients on second extract failure", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:old", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirA, true). + Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirB, true). + Return(nil, fmt.Errorf("write error")) + inst.EXPECT().Remove(dirA).Return(nil) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:new", + Clients: []string{"claude-code", "opencode"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "write error") + }) + + t.Run("upgrade digest rolls back all clients on store.Update failure", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:old", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirA, true). + Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirB, true). + Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + store.EXPECT().Update(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db failure")) + inst.EXPECT().Remove(dirA).Return(nil) + inst.EXPECT().Remove(dirB).Return(nil) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:new", + Clients: []string{"claude-code", "opencode"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "db failure") + }) + + t.Run("same digest new client rolls back on extract failure", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:abc", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirB, false).Return(nil, fmt.Errorf("extract boom")) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", "opencode"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "extract boom") + }) + + t.Run("same digest new client rolls back on store.Update failure", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:abc", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirB, false). + Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + store.EXPECT().Update(gomock.Any(), gomock.Any()).Return(fmt.Errorf("db update boom")) + inst.EXPECT().Remove(dirB).Return(nil) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", "opencode"}, + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "db update boom") + }) + + t.Run("same digest new client unmanaged dir conflict", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + require.NoError(t, os.MkdirAll(dirB, 0o750)) + + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:abc", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + _, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"claude-code", "opencode"}, + }) + require.Error(t, err) + assert.Equal(t, http.StatusConflict, httperr.Code(err)) + assert.Contains(t, err.Error(), "not managed by ToolHive") + }) + + t.Run("legacy row with explicit client is not a no-op", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:abc", + Clients: []string{}, + } + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirB, false). + Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + store.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.Contains(t, sk.Clients, "opencode") + return nil + }) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + result, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:abc", + Clients: []string{"opencode"}, + }) + require.NoError(t, err) + assert.Equal(t, "my-skill", result.Skill.Metadata.Name) + }) + + t.Run("upgrade extracts to all existing clients not just requested", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + inst := skillsmocks.NewMockInstaller(ctrl) + + dirA := filepath.Join(t.TempDir(), "a", "my-skill") + dirB := filepath.Join(t.TempDir(), "b", "my-skill") + existing := skills.InstalledSkill{ + Metadata: skills.SkillMetadata{Name: "my-skill"}, + Digest: "sha256:old", + Clients: []string{"claude-code"}, + } + pr.EXPECT().GetSkillPath("opencode", "my-skill", skills.ScopeUser, "").Return(dirB, nil) + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(dirA, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(existing, nil) + inst.EXPECT().Extract(layerData, dirB, true). + Return(&skills.ExtractResult{SkillDir: dirB, Files: 1}, nil) + inst.EXPECT().Extract(layerData, dirA, true). + Return(&skills.ExtractResult{SkillDir: dirA, Files: 1}, nil) + store.EXPECT().Update(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.ElementsMatch(t, []string{"opencode", "claude-code"}, sk.Clients) + assert.Equal(t, "sha256:new", sk.Digest) + return nil + }) + + svc := New(store, WithPathResolver(pr), WithInstaller(inst)) + result, err := svc.Install(t.Context(), skills.InstallOptions{ + Name: "my-skill", + LayerData: layerData, + Digest: "sha256:new", + Clients: []string{"opencode"}, + }) + require.NoError(t, err) + assert.ElementsMatch(t, []string{"opencode", "claude-code"}, result.Skill.Clients) + }) } // buildTestArtifact creates a real OCI skill artifact in the store and returns @@ -612,7 +1029,7 @@ func TestInstallFromOCI(t *testing.T) { }, { name: "successful pull and install", - opts: skills.InstallOptions{Name: "ghcr.io/org/my-skill:v1", Client: "claude-code"}, + opts: skills.InstallOptions{Name: "ghcr.io/org/my-skill:v1", Clients: []string{"claude-code"}}, setup: func(t *testing.T, ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() ociStore, err := ociskills.NewStore(tempDir(t)) @@ -646,7 +1063,7 @@ func TestInstallFromOCI(t *testing.T) { }, { name: "name mismatch between artifact and reference is rejected", - opts: skills.InstallOptions{Name: "ghcr.io/org/some-repo:v1", Client: "claude-code"}, + opts: skills.InstallOptions{Name: "ghcr.io/org/some-repo:v1", Clients: []string{"claude-code"}}, setup: func(t *testing.T, ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() ociStore, err := ociskills.NewStore(tempDir(t)) @@ -666,7 +1083,7 @@ func TestInstallFromOCI(t *testing.T) { }, { name: "preserves caller version over config version", - opts: skills.InstallOptions{Name: "ghcr.io/org/my-skill:v1", Version: "override-version", Client: "claude-code"}, + opts: skills.InstallOptions{Name: "ghcr.io/org/my-skill:v1", Version: "override-version", Clients: []string{"claude-code"}}, setup: func(t *testing.T, ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() ociStore, err := ociskills.NewStore(tempDir(t)) @@ -694,7 +1111,7 @@ func TestInstallFromOCI(t *testing.T) { }, { name: "hydrates version from config when caller omits it", - opts: skills.InstallOptions{Name: "ghcr.io/org/my-skill:v1", Client: "claude-code"}, + opts: skills.InstallOptions{Name: "ghcr.io/org/my-skill:v1", Clients: []string{"claude-code"}}, setup: func(t *testing.T, ctrl *gomock.Controller) (ociskills.RegistryClient, *ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() ociStore, err := ociskills.NewStore(tempDir(t)) @@ -793,7 +1210,7 @@ func TestInstallFromLocalStore(t *testing.T) { }{ { name: "happy path: build then install", - opts: skills.InstallOptions{Name: "my-skill", Client: "claude-code"}, + opts: skills.InstallOptions{Name: "my-skill", Clients: []string{"claude-code"}}, setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { t.Helper() ociStore, err := ociskills.NewStore(tempDir(t)) diff --git a/skills/toolhive-cli-user/SKILL.md b/skills/toolhive-cli-user/SKILL.md index f48035b3a0..a641373472 100644 --- a/skills/toolhive-cli-user/SKILL.md +++ b/skills/toolhive-cli-user/SKILL.md @@ -190,7 +190,7 @@ Requires `thv serve` to be running. Skills have two scopes: `user` (global, defa ```bash thv skill install my-skill # Install from registry thv skill install ghcr.io/org/skill:v1.0 # Install by OCI reference -thv skill install my-skill --client claude-code # Target specific client +thv skill install my-skill --clients claude-code # Target specific client(s) thv skill install my-skill --scope project --project-root . # Project-scoped thv skill install my-skill --group development # Add to group thv skill install my-skill --force # Overwrite existing diff --git a/skills/toolhive-cli-user/references/COMMANDS.md b/skills/toolhive-cli-user/references/COMMANDS.md index 9657b4a3aa..82c2782e85 100644 --- a/skills/toolhive-cli-user/references/COMMANDS.md +++ b/skills/toolhive-cli-user/references/COMMANDS.md @@ -426,7 +426,7 @@ thv skill install [flags] SKILL_NAME **Flags:** | Flag | Description | Default | |------|-------------|---------| -| `--client` | Target client application (e.g. claude-code) | | +| `--clients` | Comma-separated target client applications (e.g. claude-code,opencode) | | | `--scope` | Installation scope (user, project) | user | | `--force` | Overwrite existing skill directory | false | | `--project-root` | Project root path (required when scope=project) | | diff --git a/skills/toolhive-cli-user/references/EXAMPLES.md b/skills/toolhive-cli-user/references/EXAMPLES.md index 922e785693..4cb8e4bfed 100644 --- a/skills/toolhive-cli-user/references/EXAMPLES.md +++ b/skills/toolhive-cli-user/references/EXAMPLES.md @@ -471,7 +471,7 @@ Global patterns at `~/.config/toolhive/thvignore` apply to all mounts. Disable w thv skill install code-review # Install targeting a specific client -thv skill install code-review --client claude-code +thv skill install code-review --clients claude-code # Install into a project (requires --project-root) thv skill install code-review --scope project --project-root /home/user/myproject @@ -550,7 +550,7 @@ thv skill build ./my-skill --tag ghcr.io/myorg/my-skill:v1.0 thv skill push ghcr.io/myorg/my-skill:v1.0 # 4. Install from registry to verify -thv skill install ghcr.io/myorg/my-skill:v1.0 --client claude-code +thv skill install ghcr.io/myorg/my-skill:v1.0 --clients claude-code # 5. Confirm installation thv skill info my-skill