fix default skills install registry behavior
This commit is contained in:
parent
9d0954b5c5
commit
f2a5db1349
6 changed files with 106 additions and 12 deletions
|
|
@ -13,7 +13,7 @@ func newInstallCommand() *cobra.Command {
|
||||||
|
|
||||||
cmd := &cobra.Command{
|
cmd := &cobra.Command{
|
||||||
Use: "install",
|
Use: "install",
|
||||||
Short: "Install skill from configured registry",
|
Short: "Install skill from GitHub or a registry",
|
||||||
Example: `
|
Example: `
|
||||||
picoclaw skills install sipeed/picoclaw-skills/weather
|
picoclaw skills install sipeed/picoclaw-skills/weather
|
||||||
picoclaw skills install --registry clawhub github
|
picoclaw skills install --registry clawhub github
|
||||||
|
|
|
||||||
|
|
@ -13,7 +13,7 @@ func TestNewInstallSubcommand(t *testing.T) {
|
||||||
require.NotNil(t, cmd)
|
require.NotNil(t, cmd)
|
||||||
|
|
||||||
assert.Equal(t, "install", cmd.Use)
|
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.Nil(t, cmd.Run)
|
||||||
assert.NotNil(t, cmd.RunE)
|
assert.NotNil(t, cmd.RunE)
|
||||||
|
|
|
||||||
|
|
@ -6,6 +6,7 @@ import (
|
||||||
"fmt"
|
"fmt"
|
||||||
"os"
|
"os"
|
||||||
"path/filepath"
|
"path/filepath"
|
||||||
|
"strings"
|
||||||
"sync"
|
"sync"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
|
@ -15,6 +16,8 @@ import (
|
||||||
"github.com/sipeed/picoclaw/pkg/utils"
|
"github.com/sipeed/picoclaw/pkg/utils"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
const defaultSkillRegistryName = "github"
|
||||||
|
|
||||||
// InstallSkillTool allows the LLM agent to install skills from registries.
|
// InstallSkillTool allows the LLM agent to install skills from registries.
|
||||||
// It shares the same RegistryManager that FindSkillsTool uses,
|
// It shares the same RegistryManager that FindSkillsTool uses,
|
||||||
// so all registries configured in config are available for installation.
|
// so all registries configured in config are available for installation.
|
||||||
|
|
@ -40,7 +43,7 @@ func (t *InstallSkillTool) Name() string {
|
||||||
}
|
}
|
||||||
|
|
||||||
func (t *InstallSkillTool) Description() 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 {
|
func (t *InstallSkillTool) Parameters() map[string]any {
|
||||||
|
|
@ -57,14 +60,14 @@ func (t *InstallSkillTool) Parameters() map[string]any {
|
||||||
},
|
},
|
||||||
"registry": map[string]any{
|
"registry": map[string]any{
|
||||||
"type": "string",
|
"type": "string",
|
||||||
"description": "Registry to install from (required, e.g., 'clawhub')",
|
"description": "Registry to install from (optional, defaults to 'github')",
|
||||||
},
|
},
|
||||||
"force": map[string]any{
|
"force": map[string]any{
|
||||||
"type": "boolean",
|
"type": "boolean",
|
||||||
"description": "Force reinstall if skill already exists (default false)",
|
"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()
|
defer t.mu.Unlock()
|
||||||
|
|
||||||
slug, _ := args["slug"].(string)
|
slug, _ := args["slug"].(string)
|
||||||
|
if strings.TrimSpace(slug) == "" {
|
||||||
|
return ErrorResult("identifier is required and must be a non-empty string")
|
||||||
|
}
|
||||||
|
|
||||||
// Validate registry
|
// Validate registry
|
||||||
registryName, _ := args["registry"].(string)
|
registryName, _ := args["registry"].(string)
|
||||||
|
if registryName == "" {
|
||||||
|
registryName = defaultSkillRegistryName
|
||||||
|
}
|
||||||
if err := utils.ValidateSkillIdentifier(registryName); err != nil {
|
if err := utils.ValidateSkillIdentifier(registryName); err != nil {
|
||||||
return ErrorResult(fmt.Sprintf("invalid registry %q: error: %s", registryName, err.Error()))
|
return ErrorResult(fmt.Sprintf("invalid registry %q: error: %s", registryName, err.Error()))
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -150,16 +150,18 @@ func TestInstallSkillToolParameters(t *testing.T) {
|
||||||
required, ok := params["required"].([]string)
|
required, ok := params["required"].([]string)
|
||||||
assert.True(t, ok)
|
assert.True(t, ok)
|
||||||
assert.Contains(t, required, "slug")
|
assert.Contains(t, required, "slug")
|
||||||
assert.Contains(t, required, "registry")
|
assert.NotContains(t, required, "registry")
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestInstallSkillToolMissingRegistry(t *testing.T) {
|
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{
|
result := tool.Execute(context.Background(), map[string]any{
|
||||||
"slug": "some-skill",
|
"slug": "some-skill",
|
||||||
})
|
})
|
||||||
assert.True(t, result.IsError)
|
assert.False(t, result.IsError)
|
||||||
assert.Contains(t, result.ForLLM, "invalid registry")
|
assert.Contains(t, result.ForLLM, `Successfully installed skill`)
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestInstallSkillToolAllowsGitHubURLSlug(t *testing.T) {
|
func TestInstallSkillToolAllowsGitHubURLSlug(t *testing.T) {
|
||||||
|
|
|
||||||
|
|
@ -22,6 +22,8 @@ import (
|
||||||
"github.com/sipeed/picoclaw/pkg/utils"
|
"github.com/sipeed/picoclaw/pkg/utils"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
const defaultInstallSkillRegistry = "github"
|
||||||
|
|
||||||
type skillSupportResponse struct {
|
type skillSupportResponse struct {
|
||||||
Skills []skillSupportItem `json:"skills"`
|
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.Slug = strings.TrimSpace(req.Slug)
|
||||||
req.Registry = strings.TrimSpace(req.Registry)
|
req.Registry = strings.TrimSpace(req.Registry)
|
||||||
req.Version = strings.TrimSpace(req.Version)
|
req.Version = strings.TrimSpace(req.Version)
|
||||||
|
if req.Registry == "" {
|
||||||
|
req.Registry = defaultInstallSkillRegistry
|
||||||
|
}
|
||||||
|
|
||||||
if validateErr := utils.ValidateSkillIdentifier(req.Registry); validateErr != nil {
|
if validateErr := utils.ValidateSkillIdentifier(req.Registry); validateErr != nil {
|
||||||
http.Error(
|
http.Error(
|
||||||
|
|
|
||||||
|
|
@ -15,6 +15,8 @@ import (
|
||||||
"testing"
|
"testing"
|
||||||
"time"
|
"time"
|
||||||
|
|
||||||
|
"github.com/stretchr/testify/assert"
|
||||||
|
|
||||||
"github.com/sipeed/picoclaw/pkg/config"
|
"github.com/sipeed/picoclaw/pkg/config"
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
@ -560,7 +562,8 @@ func TestHandleSearchSkills(t *testing.T) {
|
||||||
t.Fatalf("WriteFile() error = %v", err)
|
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" {
|
if r.URL.Path != "/api/v1/search" {
|
||||||
http.NotFound(w, r)
|
http.NotFound(w, r)
|
||||||
return
|
return
|
||||||
|
|
@ -644,7 +647,8 @@ func TestHandleSearchSkillsPagination(t *testing.T) {
|
||||||
workspace := filepath.Join(t.TempDir(), "workspace")
|
workspace := filepath.Join(t.TempDir(), "workspace")
|
||||||
cfg.Agents.Defaults.Workspace = 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" {
|
if r.URL.Path != "/api/v1/search" {
|
||||||
http.NotFound(w, r)
|
http.NotFound(w, r)
|
||||||
return
|
return
|
||||||
|
|
@ -739,7 +743,8 @@ func TestHandleSearchSkillsClampsRegistryFanout(t *testing.T) {
|
||||||
workspace := filepath.Join(t.TempDir(), "workspace")
|
workspace := filepath.Join(t.TempDir(), "workspace")
|
||||||
cfg.Agents.Defaults.Workspace = 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" {
|
if r.URL.Path != "/api/v1/search" {
|
||||||
http.NotFound(w, r)
|
http.NotFound(w, r)
|
||||||
return
|
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) {
|
func TestHandleInstallSkillRollsBackOnOriginMetadataWriteFailure(t *testing.T) {
|
||||||
configPath, cleanup := setupOAuthTestEnv(t)
|
configPath, cleanup := setupOAuthTestEnv(t)
|
||||||
defer cleanup()
|
defer cleanup()
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue