From 927093aabf5a82082d0f275fe57f94d23b14fb01 Mon Sep 17 00:00:00 2001 From: lxowalle Date: Thu, 9 Apr 2026 21:24:16 +0800 Subject: [PATCH] fix github registry URL parsing and versioned skill links --- pkg/skills/clawhub_registry.go | 2 +- pkg/skills/github_registry.go | 12 +++-- pkg/skills/github_registry_test.go | 19 ++++++++ pkg/skills/installer.go | 60 ++++++++++++++++++++---- pkg/skills/installer_test.go | 34 ++++++++++++++ pkg/skills/registry.go | 3 +- pkg/skills/registry_test.go | 2 +- pkg/tools/skills_install_test.go | 4 +- web/backend/api/skills.go | 12 ++--- web/backend/api/skills_test.go | 75 ++++++++++++++++++++++++++++++ 10 files changed, 200 insertions(+), 23 deletions(-) diff --git a/pkg/skills/clawhub_registry.go b/pkg/skills/clawhub_registry.go index 9e1e2a2a9..677a57f18 100644 --- a/pkg/skills/clawhub_registry.go +++ b/pkg/skills/clawhub_registry.go @@ -126,7 +126,7 @@ func (c *ClawHubRegistry) ResolveInstallDirName(target string) (string, error) { return target, nil } -func (c *ClawHubRegistry) SkillURL(slug string) string { +func (c *ClawHubRegistry) SkillURL(slug, _ string) string { if slug == "" { return "" } diff --git a/pkg/skills/github_registry.go b/pkg/skills/github_registry.go index 6ff865910..834dc7a1b 100644 --- a/pkg/skills/github_registry.go +++ b/pkg/skills/github_registry.go @@ -68,11 +68,15 @@ func (r *GitHubRegistry) Name() string { } func (r *GitHubRegistry) ResolveInstallDirName(target string) (string, error) { - return githubInstallDirName(target) + return githubInstallDirNameWithBaseURL(target, r.webBase) } -func (r *GitHubRegistry) SkillURL(target string) string { - ref, err := parseGitHubRef(target) +func (r *GitHubRegistry) SkillURL(target, version string) string { + defaultRef := strings.TrimSpace(version) + if defaultRef == "" { + defaultRef = "main" + } + ref, err := parseGitHubRefWithBaseURL(target, r.webBase, defaultRef) if err != nil { return "" } @@ -233,7 +237,7 @@ func githubSearchDisplayName(item gitHubCodeSearchItem) string { } func (r *GitHubRegistry) GetSkillMeta(_ context.Context, target string) (*SkillMeta, error) { - ref, err := parseGitHubRef(target) + ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "main") if err != nil { return nil, err } diff --git a/pkg/skills/github_registry_test.go b/pkg/skills/github_registry_test.go index 911b4ac4c..a773b533c 100644 --- a/pkg/skills/github_registry_test.go +++ b/pkg/skills/github_registry_test.go @@ -136,3 +136,22 @@ func TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedAuthRequired(t *testin require.NoError(t, err) assert.Empty(t, results) } + +func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) { + registry := GitHubRegistryConfig{ + Enabled: true, + BaseURL: "https://ghe.example.com/git", + }.BuildRegistry() + require.NotNil(t, registry) + + assert.Equal( + t, + "https://ghe.example.com/git/org/repo/tree/master/skills/pr-review", + registry.SkillURL("org/repo/skills/pr-review", "master"), + ) + assert.Equal( + 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", ""), + ) +} diff --git a/pkg/skills/installer.go b/pkg/skills/installer.go index 16a2e55c9..092eb568f 100644 --- a/pkg/skills/installer.go +++ b/pkg/skills/installer.go @@ -137,10 +137,48 @@ func resolveGitHubEndpoints(baseURL string) (gitHubEndpoints, error) { }, nil } +func parseGitHubRefPathParts(repoURL *url.URL, githubBaseURL string) []string { + parts := strings.Split(strings.Trim(repoURL.Path, "/"), "/") + if len(parts) == 0 { + return parts + } + if githubBaseURL == "" { + return parts + } + baseURL, err := url.Parse(strings.TrimSpace(githubBaseURL)) + if err != nil { + return parts + } + if !strings.EqualFold(repoURL.Host, baseURL.Host) || !strings.EqualFold(repoURL.Scheme, baseURL.Scheme) { + return parts + } + baseParts := strings.Split(strings.Trim(baseURL.Path, "/"), "/") + if len(baseParts) == 1 && baseParts[0] == "" { + baseParts = nil + } + if len(baseParts) == 0 || len(parts) < len(baseParts)+2 { + return parts + } + for i, part := range baseParts { + if parts[i] != part { + return parts + } + } + return parts[len(baseParts):] +} + // parseGitHubRef parses a GitHub reference. // Supports: "owner/repo", "owner/repo/path", or full URL like "https://github.com/owner/repo/tree/ref/path" func parseGitHubRef(repo string) (GitHubRef, error) { + return parseGitHubRefWithBaseURL(repo, "", "main") +} + +func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRef, error) { repo = strings.TrimSpace(repo) + defaultRef = strings.TrimSpace(defaultRef) + if defaultRef == "" { + defaultRef = "main" + } // Handle full URL if strings.HasPrefix(repo, "http://") || strings.HasPrefix(repo, "https://") { @@ -148,14 +186,14 @@ func parseGitHubRef(repo string) (GitHubRef, error) { if err != nil { return GitHubRef{}, fmt.Errorf("invalid URL: %w", err) } - parts := strings.Split(strings.Trim(u.Path, "/"), "/") + parts := parseGitHubRefPathParts(u, githubBaseURL) if len(parts) < 2 { return GitHubRef{}, fmt.Errorf("invalid GitHub URL") } ref := GitHubRef{ Owner: parts[0], RepoName: parts[1], - Ref: "main", + Ref: defaultRef, } // Look for /tree/ or /blob/ in the path for i := 2; i < len(parts); i++ { @@ -178,7 +216,7 @@ func parseGitHubRef(repo string) (GitHubRef, error) { ref := GitHubRef{ Owner: parts[0], RepoName: parts[1], - Ref: "main", + Ref: defaultRef, } if len(parts) > 2 { ref.SubPath = strings.Join(parts[2:], "/") @@ -187,10 +225,16 @@ func parseGitHubRef(repo string) (GitHubRef, error) { } func githubInstallDirName(repo string) (string, error) { - if err := ValidateInstallTarget(repo); err != nil { - return "", err + return githubInstallDirNameWithBaseURL(repo, "") +} + +func githubInstallDirNameWithBaseURL(repo, githubBaseURL string) (string, error) { + if !strings.HasPrefix(repo, "http://") && !strings.HasPrefix(repo, "https://") { + if err := ValidateInstallTarget(repo); err != nil { + return "", err + } } - ref, err := parseGitHubRef(repo) + ref, err := parseGitHubRefWithBaseURL(repo, githubBaseURL, "main") if err != nil { return "", err } @@ -201,7 +245,7 @@ func githubInstallDirName(repo string) (string, error) { } func (si *SkillInstaller) InstallFromGitHub(ctx context.Context, repo string) error { - skillName, err := githubInstallDirName(repo) + skillName, err := githubInstallDirNameWithBaseURL(repo, si.githubBaseURL) if err != nil { return err } @@ -218,7 +262,7 @@ func (si *SkillInstaller) InstallFromGitHubToDir( ctx context.Context, repo, version, skillDirectory string, ) (*InstallResult, error) { - ref, err := parseGitHubRef(repo) + ref, err := parseGitHubRefWithBaseURL(repo, si.githubBaseURL, "main") if err != nil { return nil, err } diff --git a/pkg/skills/installer_test.go b/pkg/skills/installer_test.go index d292ae525..5281c68ad 100644 --- a/pkg/skills/installer_test.go +++ b/pkg/skills/installer_test.go @@ -127,6 +127,40 @@ func TestParseGitHubRef(t *testing.T) { } } +func TestParseGitHubRefWithBaseURL(t *testing.T) { + ref, err := parseGitHubRefWithBaseURL( + "https://ghe.example.com/git/org/repo/tree/dev/skills/test", + "https://ghe.example.com/git", + "main", + ) + if err != nil { + t.Fatalf("parseGitHubRefWithBaseURL() unexpected error = %v", err) + } + if ref.Owner != "org" { + t.Fatalf("owner = %q, want org", ref.Owner) + } + if ref.RepoName != "repo" { + t.Fatalf("repo = %q, want repo", ref.RepoName) + } + if ref.Ref != "dev" { + t.Fatalf("ref = %q, want dev", ref.Ref) + } + if ref.SubPath != "skills/test" { + t.Fatalf("subPath = %q, want skills/test", ref.SubPath) + } + + dirName, err := githubInstallDirNameWithBaseURL( + "https://ghe.example.com/git/org/repo/tree/dev/skills/test", + "https://ghe.example.com/git", + ) + if err != nil { + t.Fatalf("githubInstallDirNameWithBaseURL() unexpected error = %v", err) + } + if dirName != "test" { + t.Fatalf("dirName = %q, want test", dirName) + } +} + func TestShouldDownload(t *testing.T) { tests := []struct { name string diff --git a/pkg/skills/registry.go b/pkg/skills/registry.go index e35179175..871c10226 100644 --- a/pkg/skills/registry.go +++ b/pkg/skills/registry.go @@ -61,7 +61,8 @@ type SkillRegistry interface { // differently (for example, a slug vs owner/repo/path). ResolveInstallDirName(target string) (string, error) // SkillURL returns the web URL for a skill slug if the registry exposes one. - SkillURL(slug string) string + // version is optional and can be used by registries whose URLs depend on a ref. + SkillURL(slug, version string) string // Search searches the registry for skills matching the query. Search(ctx context.Context, query string, limit int) ([]SearchResult, error) // GetSkillMeta retrieves metadata for a specific skill by slug. diff --git a/pkg/skills/registry_test.go b/pkg/skills/registry_test.go index 1c3dfda65..a50f8831c 100644 --- a/pkg/skills/registry_test.go +++ b/pkg/skills/registry_test.go @@ -26,7 +26,7 @@ func (m *mockRegistry) Name() string { return m.name } func (m *mockRegistry) ResolveInstallDirName(target string) (string, error) { return target, nil } -func (m *mockRegistry) SkillURL(slug string) string { return "https://example.com/skills/" + slug } +func (m *mockRegistry) SkillURL(slug, _ string) string { return "https://example.com/skills/" + slug } func (m *mockRegistry) Search(_ context.Context, _ string, _ int) ([]SearchResult, error) { return m.searchResults, m.searchErr diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index 8fb1d1828..2bbd239d6 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -20,7 +20,7 @@ func (m *mockInstallRegistry) ResolveInstallDirName(target string) (string, erro return target, nil } -func (m *mockInstallRegistry) SkillURL(slug string) string { return slug } +func (m *mockInstallRegistry) SkillURL(slug, _ string) string { return slug } func (m *mockInstallRegistry) Search(context.Context, string, int) ([]skills.SearchResult, error) { return nil, nil @@ -47,7 +47,7 @@ func (m *mockGitHubInstallRegistry) ResolveInstallDirName(target string) (string return "pr-review", nil } -func (m *mockGitHubInstallRegistry) SkillURL(slug string) string { return slug } +func (m *mockGitHubInstallRegistry) SkillURL(slug, _ string) string { return slug } func (m *mockGitHubInstallRegistry) Search(context.Context, string, int) ([]skills.SearchResult, error) { return nil, nil diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index b65b10479..8adaafe3b 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -249,7 +249,7 @@ func (h *Handler) handleSearchSkills(w http.ResponseWriter, r *http.Request) { Summary: result.Summary, Version: result.Version, RegistryName: result.RegistryName, - URL: registrySkillURL(cfg, result.RegistryName, result.Slug), + URL: registrySkillURL(cfg, result.RegistryName, result.Slug, result.Version), Installed: installed, } if installed { @@ -377,7 +377,7 @@ func (h *Handler) handleInstallSkill(w http.ResponseWriter, r *http.Request) { OriginKind: "third_party", Registry: registry.Name(), Slug: req.Slug, - RegistryURL: registrySkillURL(cfg, registry.Name(), req.Slug), + RegistryURL: registrySkillURL(cfg, registry.Name(), req.Slug, result.Version), InstalledVersion: result.Version, InstalledAt: installedAt, }); err != nil { @@ -412,7 +412,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), + RegistryURL: registrySkillURL(cfg, registry.Name(), req.Slug, result.Version), InstalledVersion: result.Version, InstalledAt: installedAt, } @@ -726,7 +726,7 @@ func writeSkillOriginMeta(targetDir string, meta installedSkillOriginMeta) error return fileutil.WriteFileAtomic(filepath.Join(targetDir, ".skill-origin.json"), data, 0o600) } -func registrySkillURL(cfg *config.Config, registryName, slug string) string { +func registrySkillURL(cfg *config.Config, registryName, slug, version string) string { if cfg == nil || registryName == "" || slug == "" { return "" } @@ -734,7 +734,7 @@ func registrySkillURL(cfg *config.Config, registryName, slug string) string { if registry == nil { return "" } - return registry.SkillURL(slug) + return registry.SkillURL(slug, version) } func registrySkillURLFromMeta(cfg *config.Config, meta *installedSkillOriginMeta) string { @@ -747,7 +747,7 @@ func registrySkillURLFromMeta(cfg *config.Config, meta *installedSkillOriginMeta if cfg == nil || meta.Registry == "" { return "" } - return registrySkillURL(cfg, meta.Registry, meta.Slug) + return registrySkillURL(cfg, meta.Registry, meta.Slug, meta.InstalledVersion) } func normalizeImportedSkillName(filename string, content []byte) (string, error) { diff --git a/web/backend/api/skills_test.go b/web/backend/api/skills_test.go index 40c633fe3..6b41239bd 100644 --- a/web/backend/api/skills_test.go +++ b/web/backend/api/skills_test.go @@ -26,6 +26,15 @@ func setClawHubBaseURL(cfg *config.Config, baseURL string) { cfg.Tools.Skills.Registries.Set("clawhub", registryCfg) } +func setGithubBaseURL(cfg *config.Config, baseURL string) { + registryCfg, ok := cfg.Tools.Skills.Registries.Get("github") + if !ok { + return + } + registryCfg.BaseURL = baseURL + cfg.Tools.Skills.Registries.Set("github", registryCfg) +} + func TestHandleListSkills(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup() @@ -636,6 +645,72 @@ func TestHandleSearchSkills(t *testing.T) { } } +func TestHandleSearchSkillsUsesGitHubResultVersionInURL(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 + + var server *httptest.Server + 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 + } + json.NewEncoder(w).Encode(map[string]any{ + "items": []map[string]any{ + { + "path": "skills/pr-review/SKILL.md", + "score": 10, + "repository": map[string]any{ + "full_name": "foo/bar", + "name": "bar", + "description": "Review pull requests", + "default_branch": "master", + }, + }, + }, + }) + })) + 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) != 1 { + t.Fatalf("results count = %d, want 1", len(resp.Results)) + } + if resp.Results[0].URL != server.URL+"/foo/bar/tree/master/skills/pr-review" { + t.Fatalf("result URL = %q", resp.Results[0].URL) + } +} + func TestHandleSearchSkillsPagination(t *testing.T) { configPath, cleanup := setupOAuthTestEnv(t) defer cleanup()