diff --git a/cmd/picoclaw/internal/skills/helpers.go b/cmd/picoclaw/internal/skills/helpers.go index d0ad6c962..b3e9af3ee 100644 --- a/cmd/picoclaw/internal/skills/helpers.go +++ b/cmd/picoclaw/internal/skills/helpers.go @@ -104,6 +104,11 @@ func skillsInstallFromRegistry(cfg *config.Config, registryName, target string) fmt.Printf("\u26a0\ufe0f Warning: skill '%s' is flagged as suspicious.\n", target) } + if !workspaceHasValidSkillDirectory(workspace, dirName) { + _ = os.RemoveAll(targetDir) + return fmt.Errorf("✗ failed to install skill: registry archive for %q is not a valid skill", target) + } + normalizedSlug := skills.NormalizeInstallTargetForRegistry(cfg.Tools.Skills, registry.Name(), target) installedAt := time.Now().UnixMilli() if err := writeInstalledSkillOriginMeta(targetDir, installedSkillOriginMeta{ @@ -135,6 +140,19 @@ func writeInstalledSkillOriginMeta(targetDir string, meta installedSkillOriginMe return fileutil.WriteFileAtomic(filepath.Join(targetDir, ".skill-origin.json"), data, 0o600) } +func workspaceHasValidSkillDirectory(workspace, directory string) bool { + loader := skills.NewSkillsLoader(workspace, "", "") + for _, skill := range loader.ListSkills() { + if skill.Source != "workspace" { + continue + } + if filepath.Base(filepath.Dir(skill.Path)) == directory { + return true + } + } + return false +} + func skillsRemoveFromWorkspace(workspace, skillName string) error { name := strings.TrimSpace(skillName) name = strings.Trim(name, "/") diff --git a/cmd/picoclaw/internal/skills/helpers_test.go b/cmd/picoclaw/internal/skills/helpers_test.go index 13310e7be..215cd9af9 100644 --- a/cmd/picoclaw/internal/skills/helpers_test.go +++ b/cmd/picoclaw/internal/skills/helpers_test.go @@ -60,3 +60,40 @@ func TestSkillsInstallFromRegistryWritesOriginMetadata(t *testing.T) { assert.Equal(t, "master", meta.InstalledVersion) assert.NotZero(t, meta.InstalledAt) } + +func TestSkillsInstallFromRegistryRejectsInvalidSkillArchive(t *testing.T) { + workspace := t.TempDir() + cfg := config.DefaultConfig() + cfg.Agents.Defaults.Workspace = workspace + + var server *httptest.Server + server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v3/repos/foo/bar": + require.NoError(t, json.NewEncoder(w).Encode(map[string]any{"default_branch": "master"})) + case "/api/v3/repos/foo/bar/contents/.agents/skills/pr-review": + require.NoError(t, json.NewEncoder(w).Encode([]map[string]any{{ + "type": "file", + "name": "SKILL.md", + "download_url": server.URL + "/raw/foo/bar/master/.agents/skills/pr-review/SKILL.md", + }})) + case "/raw/foo/bar/master/.agents/skills/pr-review/SKILL.md": + _, _ = w.Write([]byte("---\nname: bad_skill\ndescription: Invalid skill name\n---\n# Invalid\n")) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + githubRegistry, ok := cfg.Tools.Skills.Registries.Get("github") + require.True(t, ok) + githubRegistry.BaseURL = server.URL + cfg.Tools.Skills.Registries.Set("github", githubRegistry) + + target := server.URL + "/foo/bar/tree/master/.agents/skills/pr-review" + err := skillsInstallFromRegistry(cfg, "github", target) + require.Error(t, err) + assert.Contains(t, err.Error(), "is not a valid skill") + _, statErr := os.Stat(filepath.Join(workspace, "skills", "pr-review")) + assert.True(t, os.IsNotExist(statErr)) +} diff --git a/pkg/skills/github_registry.go b/pkg/skills/github_registry.go index 25beca5d7..feaf169d4 100644 --- a/pkg/skills/github_registry.go +++ b/pkg/skills/github_registry.go @@ -93,10 +93,10 @@ func (r *GitHubRegistry) SkillURL(target, version string) string { return fmt.Sprintf("%s/%s", base, urlPath) } if ref.SubPath != "" { - return fmt.Sprintf("%s/%s/tree/%s/%s", base, urlPath, url.PathEscape(ref.Ref), ref.SubPath) + return fmt.Sprintf("%s/%s/tree/%s/%s", base, urlPath, ref.Ref, ref.SubPath) } if ref.Ref != "main" { - return fmt.Sprintf("%s/%s/tree/%s", base, urlPath, url.PathEscape(ref.Ref)) + return fmt.Sprintf("%s/%s/tree/%s", base, urlPath, ref.Ref) } return fmt.Sprintf("%s/%s", base, urlPath) } diff --git a/pkg/skills/github_registry_test.go b/pkg/skills/github_registry_test.go index 8c87ddc1e..db816845a 100644 --- a/pkg/skills/github_registry_test.go +++ b/pkg/skills/github_registry_test.go @@ -171,6 +171,11 @@ func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) { "https://ghe.example.com/git/org/repo/tree/dev/skills/pr-review", registry.SkillURL("https://ghe.example.com/git/org/repo/tree/dev/skills/pr-review", ""), ) + assert.Equal( + t, + "https://ghe.example.com/git/org/repo/tree/feature/skills-registry/skills/pr-review", + registry.SkillURL("org/repo/skills/pr-review", "feature/skills-registry"), + ) } func TestGitHubRegistryResolveInstallDirNameSupportsFullURLs(t *testing.T) { diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index 69f3f2eb3..47d5c0a43 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -242,6 +242,15 @@ func (h *Handler) handleSearchSkills(w http.ResponseWriter, r *http.Request) { response := make([]skillSearchResultItem, 0, len(pageResults)) for _, result := range pageResults { installedSkill, installed := installedSkills[result.Slug] + if !installed { + registry := registryMgr.GetRegistry(result.RegistryName) + if registry != nil { + dirName, err := registry.ResolveInstallDirName(result.Slug) + if err == nil { + installedSkill, installed = installedSkills[dirName] + } + } + } item := skillSearchResultItem{ Score: result.Score, Slug: result.Slug, @@ -569,17 +578,19 @@ func buildOccupiedWorkspaceSkillsByDirectory(cfg *config.Config) (map[string]ski continue } - key := filepath.Base(filepath.Dir(skill.Path)) + dirName := filepath.Base(filepath.Dir(skill.Path)) + if dirName != "" { + result[dirName] = skill + } if meta, err := readInstalledSkillOriginMeta(skill.Path); err == nil && meta != nil && meta.Slug != "" { - key = skills.NormalizeInstallTargetForRegistry(cfg.Tools.Skills, meta.Registry, meta.Slug) + key := skills.NormalizeInstallTargetForRegistry(cfg.Tools.Skills, meta.Registry, meta.Slug) if key == "" { key = meta.Slug } + if key != "" { + result[key] = skill + } } - if key == "" { - continue - } - result[key] = skill } return result, nil } diff --git a/web/backend/api/skills_test.go b/web/backend/api/skills_test.go index d29a2e3e0..32dd5f7f3 100644 --- a/web/backend/api/skills_test.go +++ b/web/backend/api/skills_test.go @@ -1311,6 +1311,89 @@ func TestHandleInstallSkillTracksGitHubURLInstallsAsInstalled(t *testing.T) { } } +func TestHandleSearchSkillsMarksDirectoryCollisionAsInstalled(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 + + skillDir := filepath.Join(workspace, "skills", "pr-review") + if err := os.MkdirAll(skillDir, 0o755); err != nil { + t.Fatalf("MkdirAll() error = %v", err) + } + if err := os.WriteFile( + filepath.Join(skillDir, "SKILL.md"), + []byte("---\nname: pr-review\ndescription: Workspace PR review skill\n---\n# PR Review\n"), + 0o644, + ); err != nil { + t.Fatalf("WriteFile(SKILL.md) error = %v", err) + } + if err := writeSkillOriginMeta(skillDir, installedSkillOriginMeta{ + Version: 1, + OriginKind: "third_party", + Registry: "github", + Slug: "foo/bar/.agents/skills/pr-review", + RegistryURL: "https://github.com/foo/bar/tree/master/.agents/skills/pr-review", + InstalledVersion: "master", + InstalledAt: time.Now().UnixMilli(), + }); err != nil { + t.Fatalf("writeSkillOriginMeta() error = %v", err) + } + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v1/search": + json.NewEncoder(w).Encode(map[string]any{ + "results": []map[string]any{{ + "slug": "pr-review", + "displayName": "PR Review", + "summary": "ClawHub PR review skill", + "version": "1.2.3", + }}, + }) + default: + http.NotFound(w, r) + } + })) + defer server.Close() + + setClawHubBaseURL(cfg, server.URL) + githubRegistry, _ := cfg.Tools.Skills.Registries.Get("github") + githubRegistry.Enabled = false + 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) + + rec := httptest.NewRecorder() + req := httptest.NewRequest(http.MethodGet, "/api/skills/search?q=pr+review&limit=5", nil) + 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 skillSearchResponse + if err := json.Unmarshal(rec.Body.Bytes(), &resp); err != nil { + t.Fatalf("Unmarshal() error = %v", err) + } + if len(resp.Results) != 1 { + t.Fatalf("results count = %d, want 1", len(resp.Results)) + } + if !resp.Results[0].Installed || resp.Results[0].InstalledName != "pr-review" { + t.Fatalf("search result should be treated as installed when directory is occupied, got %#v", resp.Results[0]) + } +} + func TestHandleInstallSkillRollsBackOnOriginMetadataWriteFailure(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup()