diff --git a/pkg/config/config_old.go b/pkg/config/config_old.go index b1711d220..d2bc27b29 100644 --- a/pkg/config/config_old.go +++ b/pkg/config/config_old.go @@ -970,14 +970,16 @@ func (v *clawHubRegistryConfigV0) ToSkillRegistryConfig() SkillRegistryConfig { } type skillsGithubConfigV0 struct { - Token string `json:"token" env:"PICOCLAW_TOOLS_SKILLS_GITHUB_TOKEN"` - Proxy string `json:"proxy,omitempty" env:"PICOCLAW_TOOLS_SKILLS_GITHUB_PROXY"` + BaseURL string `json:"base_url,omitempty" env:"PICOCLAW_TOOLS_SKILLS_GITHUB_BASE_URL"` + Token string `json:"token" env:"PICOCLAW_TOOLS_SKILLS_GITHUB_TOKEN"` + Proxy string `json:"proxy,omitempty" env:"PICOCLAW_TOOLS_SKILLS_GITHUB_PROXY"` } func (v *skillsGithubConfigV0) ToSkillsGithubConfig() SkillsGithubConfig { return SkillsGithubConfig{ - Token: *NewSecureString(v.Token), - Proxy: v.Proxy, + BaseURL: v.BaseURL, + Token: *NewSecureString(v.Token), + Proxy: v.Proxy, } } diff --git a/pkg/config/config_struct_test.go b/pkg/config/config_struct_test.go index 07bb14b7b..9b27d117b 100644 --- a/pkg/config/config_struct_test.go +++ b/pkg/config/config_struct_test.go @@ -262,3 +262,16 @@ func TestSkillsRegistriesConfigMarshalJSONPreservesObjectShape(t *testing.T) { assert.True(t, ok) assert.Equal(t, "https://clawhub.ai", clawhub.BaseURL) } + +func TestSkillsGithubConfigV0ToSkillsGithubConfigPreservesBaseURL(t *testing.T) { + legacy := skillsGithubConfigV0{ + BaseURL: "https://ghe.example.com/git", + Token: "ghp-test-token", + Proxy: "http://127.0.0.1:7890", + } + + converted := legacy.ToSkillsGithubConfig() + assert.Equal(t, "https://ghe.example.com/git", converted.BaseURL) + assert.Equal(t, "ghp-test-token", converted.Token.String()) + assert.Equal(t, "http://127.0.0.1:7890", converted.Proxy) +} diff --git a/pkg/skills/config_bridge.go b/pkg/skills/config_bridge.go index bb607401b..49b21ee37 100644 --- a/pkg/skills/config_bridge.go +++ b/pkg/skills/config_bridge.go @@ -94,3 +94,22 @@ func LookupRegistryFromToolsConfig(cfg config.SkillsToolsConfig, name string) Sk } return nil } + +func NormalizeInstallTargetForRegistry(cfg config.SkillsToolsConfig, registryName, target string) string { + if registryName == "" || target == "" { + return target + } + registry := LookupRegistryFromToolsConfig(cfg, registryName) + if registry == nil { + return target + } + ghRegistry, ok := registry.(*GitHubRegistry) + if !ok { + return target + } + normalized, err := canonicalGitHubRegistrySlugWithBaseURL(target, ghRegistry.webBase) + if err != nil || normalized == "" { + return target + } + return normalized +} diff --git a/pkg/skills/github_registry.go b/pkg/skills/github_registry.go index ac8ccf6e3..59f7d4fd7 100644 --- a/pkg/skills/github_registry.go +++ b/pkg/skills/github_registry.go @@ -147,10 +147,12 @@ func (r *GitHubRegistry) Search(ctx context.Context, query string, limit int) ([ return nil, fmt.Errorf("failed to read github search response: %w", err) } if resp.StatusCode == http.StatusUnauthorized && r.installer.githubToken == "" && isGitHubAuthRequiredError(body) { - return nil, fmt.Errorf("github search requires authentication; %s", githubAuthTokenHelp) + slog.Warn("github search requires authentication; returning no results", "help", githubAuthTokenHelp) + return []SearchResult{}, nil } if resp.StatusCode == http.StatusForbidden && r.installer.githubToken == "" && isGitHubRateLimitError(body) { - return nil, fmt.Errorf("github search hit the unauthenticated rate limit; %s", githubAuthTokenHelp) + slog.Warn("github search hit unauthenticated rate limit; returning no results", "help", githubAuthTokenHelp) + return []SearchResult{}, nil } if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("github search failed: HTTP %d: %s", resp.StatusCode, string(body)) @@ -238,7 +240,23 @@ func githubSearchDisplayName(item gitHubCodeSearchItem) string { return strings.TrimSpace(item.Repository.FullName) } +func canonicalGitHubRegistrySlugWithBaseURL(target, githubBaseURL string) (string, error) { + ref, err := parseGitHubRefWithBaseURL(target, githubBaseURL, "") + if err != nil { + return "", err + } + slug := path.Join(ref.Owner, ref.RepoName) + if ref.SubPath != "" { + slug = path.Join(slug, ref.SubPath) + } + return slug, nil +} + func (r *GitHubRegistry) GetSkillMeta(ctx context.Context, target string) (*SkillMeta, error) { + slug, err := canonicalGitHubRegistrySlugWithBaseURL(target, r.webBase) + if err != nil { + return nil, err + } ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "") if err != nil { return nil, err @@ -250,7 +268,7 @@ func (r *GitHubRegistry) GetSkillMeta(ctx context.Context, target string) (*Skil } } return &SkillMeta{ - Slug: target, + Slug: slug, DisplayName: ref.RepoName, LatestVersion: ref.Ref, RegistryName: r.Name(), diff --git a/pkg/skills/github_registry_test.go b/pkg/skills/github_registry_test.go index 57f4c08f6..8c87ddc1e 100644 --- a/pkg/skills/github_registry_test.go +++ b/pkg/skills/github_registry_test.go @@ -103,7 +103,7 @@ func TestGitHubRegistryProviderDecodesProxyParam(t *testing.T) { assert.Equal(t, "http://127.0.0.1:7890", ghRegistry.installer.proxy) } -func TestGitHubRegistrySearchReturnsHelpfulErrorOnUnauthenticatedRateLimit(t *testing.T) { +func TestGitHubRegistrySearchReturnsNoResultsOnUnauthenticatedRateLimit(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("Authorization")) w.WriteHeader(http.StatusForbidden) @@ -115,12 +115,11 @@ func TestGitHubRegistrySearchReturnsHelpfulErrorOnUnauthenticatedRateLimit(t *te require.NotNil(t, registry) results, err := registry.Search(context.Background(), "pr review", 5) - require.Error(t, err) - assert.Nil(t, results) - assert.Contains(t, err.Error(), "registries.github.auth_token") + require.NoError(t, err) + assert.Empty(t, results) } -func TestGitHubRegistrySearchReturnsHelpfulErrorOnUnauthenticatedAuthRequired(t *testing.T) { +func TestGitHubRegistrySearchReturnsNoResultsOnUnauthenticatedAuthRequired(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { assert.Empty(t, r.Header.Get("Authorization")) w.WriteHeader(http.StatusUnauthorized) @@ -134,9 +133,25 @@ func TestGitHubRegistrySearchReturnsHelpfulErrorOnUnauthenticatedAuthRequired(t require.NotNil(t, registry) results, err := registry.Search(context.Background(), "pr review", 5) - require.Error(t, err) - assert.Nil(t, results) - assert.Contains(t, err.Error(), "registries.github.auth_token") + require.NoError(t, err) + assert.Empty(t, results) +} + +func TestGitHubRegistryGetSkillMetaCanonicalizesURLSlug(t *testing.T) { + registry := GitHubRegistryConfig{ + Enabled: true, + BaseURL: "https://ghe.example.com/git", + }.BuildRegistry() + require.NotNil(t, registry) + + meta, err := registry.GetSkillMeta( + context.Background(), + "https://ghe.example.com/git/org/repo/tree/dev/skills/pr-review", + ) + require.NoError(t, err) + require.NotNil(t, meta) + assert.Equal(t, "org/repo/skills/pr-review", meta.Slug) + assert.Equal(t, "dev", meta.LatestVersion) } func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) { diff --git a/pkg/skills/registry_test.go b/pkg/skills/registry_test.go index b870b3df9..6ac5ffbf3 100644 --- a/pkg/skills/registry_test.go +++ b/pkg/skills/registry_test.go @@ -238,3 +238,20 @@ func TestExplicitGithubRegistryBaseURLBeatsLegacyCompat(t *testing.T) { assert.True(t, ok) assert.Equal(t, "https://ghe-explicit.example.com/scm", ghRegistry.webBase) } + +func TestNormalizeInstallTargetForRegistryCanonicalizesGitHubURLs(t *testing.T) { + cfg := config.DefaultConfig().Tools.Skills + cfg.Registries.Set("github", config.SkillRegistryConfig{ + Name: "github", + Enabled: true, + BaseURL: "https://ghe.example.com/git", + Param: map[string]any{}, + }) + + got := NormalizeInstallTargetForRegistry( + cfg, + "github", + "https://ghe.example.com/git/org/repo/tree/dev/skills/pr-review", + ) + assert.Equal(t, "org/repo/skills/pr-review", got) +} diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index 8adaafe3b..69f3f2eb3 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -372,12 +372,13 @@ func (h *Handler) handleInstallSkill(w http.ResponseWriter, r *http.Request) { } installedAt := time.Now().UnixMilli() + normalizedSlug := skills.NormalizeInstallTargetForRegistry(cfg.Tools.Skills, registry.Name(), req.Slug) if err := persistSkillOriginMeta(stagedTargetDir, installedSkillOriginMeta{ Version: 1, OriginKind: "third_party", Registry: registry.Name(), - Slug: req.Slug, - RegistryURL: registrySkillURL(cfg, registry.Name(), req.Slug, result.Version), + Slug: normalizedSlug, + RegistryURL: registrySkillURL(cfg, registry.Name(), normalizedSlug, result.Version), InstalledVersion: result.Version, InstalledAt: installedAt, }); err != nil { @@ -412,7 +413,7 @@ func (h *Handler) handleInstallSkill(w http.ResponseWriter, r *http.Request) { Description: validatedSkill.Description, OriginKind: "third_party", RegistryName: registry.Name(), - RegistryURL: registrySkillURL(cfg, registry.Name(), req.Slug, result.Version), + RegistryURL: registrySkillURL(cfg, registry.Name(), normalizedSlug, result.Version), InstalledVersion: result.Version, InstalledAt: installedAt, } @@ -570,7 +571,10 @@ func buildOccupiedWorkspaceSkillsByDirectory(cfg *config.Config) (map[string]ski key := filepath.Base(filepath.Dir(skill.Path)) if meta, err := readInstalledSkillOriginMeta(skill.Path); err == nil && meta != nil && meta.Slug != "" { - key = meta.Slug + key = skills.NormalizeInstallTargetForRegistry(cfg.Tools.Skills, meta.Registry, meta.Slug) + if key == "" { + key = meta.Slug + } } if key == "" { continue diff --git a/web/backend/api/skills_test.go b/web/backend/api/skills_test.go index 0174a08a2..d29a2e3e0 100644 --- a/web/backend/api/skills_test.go +++ b/web/backend/api/skills_test.go @@ -711,6 +711,56 @@ func TestHandleSearchSkillsUsesGitHubResultVersionInURL(t *testing.T) { } } +func TestHandleSearchSkillsGitHubRateLimitDegradesGracefully(t *testing.T) { + configPath, cleanup := setupOAuthTestEnv(t) + defer cleanup() + + cfg, err := config.LoadConfig(configPath) + if err != nil { + t.Fatalf("LoadConfig() error = %v", err) + } + workspace := filepath.Join(t.TempDir(), "workspace") + cfg.Agents.Defaults.Workspace = workspace + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.URL.Path != "/api/v3/search/code" { + http.NotFound(w, r) + return + } + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"API rate limit exceeded for 1.2.3.4"}`)) + })) + defer server.Close() + + setGithubBaseURL(cfg, server.URL) + clawHubRegistry, _ := cfg.Tools.Skills.Registries.Get("clawhub") + clawHubRegistry.Enabled = false + cfg.Tools.Skills.Registries.Set("clawhub", clawHubRegistry) + if err := config.SaveConfig(configPath, cfg); err != nil { + t.Fatalf("SaveConfig() error = %v", err) + } + + 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) != 0 { + t.Fatalf("results count = %d, want 0", len(resp.Results)) + } +} + func TestHandleSearchSkillsPagination(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup() @@ -1169,6 +1219,98 @@ func TestHandleInstallSkillDefaultsRegistryToGitHub(t *testing.T) { } } +func TestHandleInstallSkillTracksGitHubURLInstallsAsInstalled(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 + + 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": + json.NewEncoder(w).Encode(map[string]any{"default_branch": "master"}) + case "/api/v3/repos/foo/bar/contents/.agents/skills/pr-review": + assert.Equal(t, "ref=master", r.URL.RawQuery) + 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 "/api/v3/search/code": + json.NewEncoder(w).Encode(map[string]any{ + "items": []map[string]any{{ + "path": ".agents/skills/pr-review/SKILL.md", + "score": 10, + "repository": map[string]any{ + "full_name": "foo/bar", + "name": "bar", + "description": "PR review skill", + "default_branch": "master", + }, + }}, + }) + case "/raw/foo/bar/master/.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() + + setGithubBaseURL(cfg, server.URL) + clawHubRegistry, _ := cfg.Tools.Skills.Registries.Get("clawhub") + clawHubRegistry.Enabled = false + cfg.Tools.Skills.Registries.Set("clawhub", clawHubRegistry) + if saveErr := config.SaveConfig(configPath, cfg); saveErr != nil { + t.Fatalf("SaveConfig() error = %v", saveErr) + } + + h := NewHandler(configPath) + mux := http.NewServeMux() + h.RegisterRoutes(mux) + + installBody, err := json.Marshal(installSkillRequest{ + Slug: server.URL + "/foo/bar/tree/master/.agents/skills/pr-review", + }) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + + installRec := httptest.NewRecorder() + installReq := httptest.NewRequest(http.MethodPost, "/api/skills/install", bytes.NewReader(installBody)) + installReq.Header.Set("Content-Type", "application/json") + mux.ServeHTTP(installRec, installReq) + + if installRec.Code != http.StatusOK { + t.Fatalf("install status = %d, want %d, body=%s", installRec.Code, http.StatusOK, installRec.Body.String()) + } + + searchRec := httptest.NewRecorder() + searchReq := httptest.NewRequest(http.MethodGet, "/api/skills/search?q=pr+review&limit=5", nil) + mux.ServeHTTP(searchRec, searchReq) + + if searchRec.Code != http.StatusOK { + t.Fatalf("search status = %d, want %d, body=%s", searchRec.Code, http.StatusOK, searchRec.Body.String()) + } + + var searchResp skillSearchResponse + if err := json.Unmarshal(searchRec.Body.Bytes(), &searchResp); err != nil { + t.Fatalf("Unmarshal(search response) error = %v", err) + } + if len(searchResp.Results) != 1 { + t.Fatalf("search results count = %d, want 1", len(searchResp.Results)) + } + if !searchResp.Results[0].Installed || searchResp.Results[0].InstalledName != "pr-review" { + t.Fatalf("search result should be treated as installed after URL install, got %#v", searchResp.Results[0]) + } +} + func TestHandleInstallSkillRollsBackOnOriginMetadataWriteFailure(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup()