diff --git a/cmd/picoclaw/internal/skills/install.go b/cmd/picoclaw/internal/skills/install.go index 9bd9cfd90..6c9b2d7c1 100644 --- a/cmd/picoclaw/internal/skills/install.go +++ b/cmd/picoclaw/internal/skills/install.go @@ -13,7 +13,7 @@ func newInstallCommand() *cobra.Command { cmd := &cobra.Command{ Use: "install", - Short: "Install skill from configured registry", + Short: "Install skill from GitHub or a registry", Example: ` picoclaw skills install sipeed/picoclaw-skills/weather picoclaw skills install --registry clawhub github diff --git a/cmd/picoclaw/internal/skills/install_test.go b/cmd/picoclaw/internal/skills/install_test.go index da66f1c80..a8c6ec7ec 100644 --- a/cmd/picoclaw/internal/skills/install_test.go +++ b/cmd/picoclaw/internal/skills/install_test.go @@ -13,7 +13,7 @@ func TestNewInstallSubcommand(t *testing.T) { require.NotNil(t, cmd) assert.Equal(t, "install", cmd.Use) - assert.Equal(t, "Install skill from configured registry", cmd.Short) + assert.Equal(t, "Install skill from GitHub or a registry", cmd.Short) assert.Nil(t, cmd.Run) assert.NotNil(t, cmd.RunE) diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index aaeef6761..b7532fac5 100644 --- a/pkg/tools/skills_install.go +++ b/pkg/tools/skills_install.go @@ -6,6 +6,7 @@ import ( "fmt" "os" "path/filepath" + "strings" "sync" "time" @@ -15,6 +16,8 @@ import ( "github.com/sipeed/picoclaw/pkg/utils" ) +const defaultSkillRegistryName = "github" + // InstallSkillTool allows the LLM agent to install skills from registries. // It shares the same RegistryManager that FindSkillsTool uses, // so all registries configured in config are available for installation. @@ -40,7 +43,7 @@ func (t *InstallSkillTool) Name() string { } func (t *InstallSkillTool) Description() string { - return "Install a skill from a registry by slug. Downloads and extracts the skill into the workspace. Use find_skills first to discover available skills." + return "Install a skill from a registry by slug. Defaults to GitHub when registry is omitted. Downloads and extracts the skill into the workspace. Use find_skills first to discover available skills." } func (t *InstallSkillTool) Parameters() map[string]any { @@ -57,14 +60,14 @@ func (t *InstallSkillTool) Parameters() map[string]any { }, "registry": map[string]any{ "type": "string", - "description": "Registry to install from (required, e.g., 'clawhub')", + "description": "Registry to install from (optional, defaults to 'github')", }, "force": map[string]any{ "type": "boolean", "description": "Force reinstall if skill already exists (default false)", }, }, - "required": []string{"slug", "registry"}, + "required": []string{"slug"}, } } @@ -75,9 +78,15 @@ func (t *InstallSkillTool) Execute(ctx context.Context, args map[string]any) *To defer t.mu.Unlock() slug, _ := args["slug"].(string) + if strings.TrimSpace(slug) == "" { + return ErrorResult("identifier is required and must be a non-empty string") + } // Validate registry registryName, _ := args["registry"].(string) + if registryName == "" { + registryName = defaultSkillRegistryName + } if err := utils.ValidateSkillIdentifier(registryName); err != nil { return ErrorResult(fmt.Sprintf("invalid registry %q: error: %s", registryName, err.Error())) } diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index f0fcaea0a..8fb1d1828 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -150,16 +150,18 @@ func TestInstallSkillToolParameters(t *testing.T) { required, ok := params["required"].([]string) assert.True(t, ok) assert.Contains(t, required, "slug") - assert.Contains(t, required, "registry") + assert.NotContains(t, required, "registry") } func TestInstallSkillToolMissingRegistry(t *testing.T) { - tool := NewInstallSkillTool(skills.NewRegistryManager(), t.TempDir()) + registryMgr := skills.NewRegistryManager() + registryMgr.AddRegistry(&mockGitHubInstallRegistry{}) + tool := NewInstallSkillTool(registryMgr, t.TempDir()) result := tool.Execute(context.Background(), map[string]any{ "slug": "some-skill", }) - assert.True(t, result.IsError) - assert.Contains(t, result.ForLLM, "invalid registry") + assert.False(t, result.IsError) + assert.Contains(t, result.ForLLM, `Successfully installed skill`) } func TestInstallSkillToolAllowsGitHubURLSlug(t *testing.T) { diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index e6e7c0157..b65b10479 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -22,6 +22,8 @@ import ( "github.com/sipeed/picoclaw/pkg/utils" ) +const defaultInstallSkillRegistry = "github" + type skillSupportResponse struct { Skills []skillSupportItem `json:"skills"` } @@ -291,6 +293,9 @@ func (h *Handler) handleInstallSkill(w http.ResponseWriter, r *http.Request) { req.Slug = strings.TrimSpace(req.Slug) req.Registry = strings.TrimSpace(req.Registry) req.Version = strings.TrimSpace(req.Version) + if req.Registry == "" { + req.Registry = defaultInstallSkillRegistry + } if validateErr := utils.ValidateSkillIdentifier(req.Registry); validateErr != nil { http.Error( diff --git a/web/backend/api/skills_test.go b/web/backend/api/skills_test.go index 85ffbf517..40c633fe3 100644 --- a/web/backend/api/skills_test.go +++ b/web/backend/api/skills_test.go @@ -15,6 +15,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/sipeed/picoclaw/pkg/config" ) @@ -560,7 +562,8 @@ func TestHandleSearchSkills(t *testing.T) { t.Fatalf("WriteFile() error = %v", err) } - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/api/v1/search" { http.NotFound(w, r) return @@ -644,7 +647,8 @@ func TestHandleSearchSkillsPagination(t *testing.T) { workspace := filepath.Join(t.TempDir(), "workspace") cfg.Agents.Defaults.Workspace = workspace - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/api/v1/search" { http.NotFound(w, r) return @@ -739,7 +743,8 @@ func TestHandleSearchSkillsClampsRegistryFanout(t *testing.T) { workspace := filepath.Join(t.TempDir(), "workspace") cfg.Agents.Defaults.Workspace = workspace - server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/api/v1/search" { http.NotFound(w, r) return @@ -1014,6 +1019,79 @@ func TestHandleInstallSkillForcePreservesExistingSkillOnFailure(t *testing.T) { } } +func TestHandleInstallSkillDefaultsRegistryToGitHub(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + cfg, loadErr := config.LoadConfig(configPath) + if loadErr != nil { + t.Fatalf("LoadConfig() error = %v", loadErr) + } + workspace := filepath.Join(t.TempDir(), "workspace") + cfg.Agents.Defaults.Workspace = workspace + if saveErr := config.SaveConfig(configPath, cfg); saveErr != nil { + t.Fatalf("SaveConfig() error = %v", saveErr) + } + + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/repos/foo/bar/contents/.agents/skills/pr-review": + assert.Equal(t, "ref=main", r.URL.RawQuery) + json.NewEncoder(w).Encode([]map[string]any{ + { + "type": "file", + "name": "SKILL.md", + "download_url": server.URL + "/raw/foo/bar/main/.agents/skills/pr-review/SKILL.md", + }, + }) + case "/raw/foo/bar/main/.agents/skills/pr-review/SKILL.md": + _, _ = w.Write([]byte("---\nname: pr-review\ndescription: PR review skill\n---\n# PR Review\n")) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + githubRegistry, ok := cfg.Tools.Skills.Registries.Get("github") + if !ok { + t.Fatalf("github registry missing from default config") + } + githubRegistry.BaseURL = server.URL + cfg.Tools.Skills.Registries.Set("github", githubRegistry) + if saveErr := config.SaveConfig(configPath, cfg); saveErr != nil { + t.Fatalf("SaveConfig() error = %v", saveErr) + } + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + body, err := json.Marshal(installSkillRequest{ + Slug: "foo/bar/.agents/skills/pr-review", + }) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodPost, "/api/skills/install", bytes.NewReader(body)) + req.Header.Set("Content-Type", "application/json") + mux.ServeHTTP(rec, req) + + if rec.Code != http.StatusOK { + t.Fatalf("status = %d, want %d, body=%s", rec.Code, http.StatusOK, rec.Body.String()) + } + + var resp installSkillResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("Unmarshal() error = %v", err) + } + if resp.Registry != "github" { + t.Fatalf("resp.Registry = %q, want github", resp.Registry) + } +} + func TestHandleInstallSkillRollsBackOnOriginMetadataWriteFailure(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup()