diff --git a/cmd/picoclaw/internal/skills/helpers.go b/cmd/picoclaw/internal/skills/helpers.go index b3e9af3ee..af552d22a 100644 --- a/cmd/picoclaw/internal/skills/helpers.go +++ b/cmd/picoclaw/internal/skills/helpers.go @@ -109,14 +109,14 @@ func skillsInstallFromRegistry(cfg *config.Config, registryName, target string) 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) + normalizedSlug, registryURL := skills.BuildInstallMetadataForRegistryInstance(registry, target, result.Version) installedAt := time.Now().UnixMilli() if err := writeInstalledSkillOriginMeta(targetDir, installedSkillOriginMeta{ Version: 1, OriginKind: "third_party", Registry: registry.Name(), Slug: normalizedSlug, - RegistryURL: registry.SkillURL(normalizedSlug, result.Version), + RegistryURL: registryURL, InstalledVersion: result.Version, InstalledAt: installedAt, }); err != nil { diff --git a/pkg/skills/config_bridge.go b/pkg/skills/config_bridge.go index 49b21ee37..9f92d6103 100644 --- a/pkg/skills/config_bridge.go +++ b/pkg/skills/config_bridge.go @@ -113,3 +113,15 @@ func NormalizeInstallTargetForRegistry(cfg config.SkillsToolsConfig, registryNam } return normalized } + +func BuildInstallMetadataForRegistryInstance(registry SkillRegistry, target, version string) (string, string) { + normalizedTarget := NormalizeInstallTargetForRegistryInstance(registry, target) + if registry == nil { + return normalizedTarget, "" + } + registryURL := registry.SkillURL(target, version) + if registryURL == "" { + registryURL = registry.SkillURL(normalizedTarget, version) + } + return normalizedTarget, registryURL +} diff --git a/pkg/skills/github_registry.go b/pkg/skills/github_registry.go index 5597eda38..79c76f035 100644 --- a/pkg/skills/github_registry.go +++ b/pkg/skills/github_registry.go @@ -83,11 +83,12 @@ func (r *GitHubRegistry) NormalizeInstallTarget(target string) string { func (r *GitHubRegistry) SkillURL(target, version string) string { defaultRef := strings.TrimSpace(version) - ref, err := parseGitHubRefWithBaseURL(target, r.webBase, defaultRef) + parsedTarget, err := parseGitHubTargetWithBaseURL(target, r.webBase, defaultRef) if err != nil { return "" } - base := strings.TrimRight(r.webBase, "/") + ref := parsedTarget.Ref + base := strings.TrimRight(parsedTarget.Endpoints.WebBaseURL, "/") urlPath := path.Join(ref.Owner, ref.RepoName) if ref.Ref == "" { return fmt.Sprintf("%s/%s", base, urlPath) @@ -269,12 +270,18 @@ func (r *GitHubRegistry) GetSkillMeta(ctx context.Context, target string) (*Skil if err != nil { return nil, err } - ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "") + parsedTarget, err := parseGitHubTargetWithBaseURL(target, r.webBase, "") if err != nil { return nil, err } + ref := parsedTarget.Ref if ref.Ref == "" { - ref.Ref, err = r.installer.fetchDefaultBranch(ctx, ref.Owner, ref.RepoName) + ref.Ref, err = r.installer.fetchDefaultBranchWithAPIBaseURL( + ctx, + parsedTarget.Endpoints.APIBaseURL, + ref.Owner, + ref.RepoName, + ) if err != nil { return nil, err } diff --git a/pkg/skills/github_registry_test.go b/pkg/skills/github_registry_test.go index f6436b11e..69f5adc91 100644 --- a/pkg/skills/github_registry_test.go +++ b/pkg/skills/github_registry_test.go @@ -181,6 +181,11 @@ func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) { "https://ghe.example.com/git/org/repo/blob/main/.agents/skills/pr-review/SKILL.md", registry.SkillURL("https://ghe.example.com/git/org/repo/blob/main/.agents/skills/pr-review/SKILL.md", ""), ) + assert.Equal( + t, + "https://github.com/org/repo/tree/main/.agents/skills/pr-review", + registry.SkillURL("https://github.com/org/repo/tree/main/.agents/skills/pr-review", ""), + ) } func TestGitHubRegistryResolveInstallDirNameSupportsFullURLs(t *testing.T) { diff --git a/pkg/skills/installer.go b/pkg/skills/installer.go index 599668baf..43985f2c2 100644 --- a/pkg/skills/installer.go +++ b/pkg/skills/installer.go @@ -34,6 +34,11 @@ type GitHubRef struct { SubPath string // Path within the repository } +type gitHubTarget struct { + Ref GitHubRef + Endpoints gitHubEndpoints +} + type SkillInstaller struct { workspace string client *http.Client @@ -168,18 +173,18 @@ func parseGitHubRefPathParts(repoURL *url.URL, githubBaseURL string) []string { return parts[len(baseParts):] } -func isSupportedGitHubURL(repoURL *url.URL, githubBaseURL string) bool { +func supportedGitHubBaseURL(repoURL *url.URL, githubBaseURL string) string { if repoURL == nil { - return false - } - if matchesGitHubWebBase(repoURL, "https://github.com") { - return true + return "" } trimmedBaseURL := strings.TrimSpace(githubBaseURL) - if trimmedBaseURL == "" { - return false + if trimmedBaseURL != "" && matchesGitHubWebBase(repoURL, trimmedBaseURL) { + return trimmedBaseURL } - return matchesGitHubWebBase(repoURL, trimmedBaseURL) + if matchesGitHubWebBase(repoURL, "https://github.com") { + return "https://github.com" + } + return "" } func matchesGitHubWebBase(repoURL *url.URL, webBaseURL string) bool { @@ -245,6 +250,14 @@ func parseGitHubRef(repo string) (GitHubRef, error) { } func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRef, error) { + target, err := parseGitHubTargetWithBaseURL(repo, githubBaseURL, defaultRef) + if err != nil { + return GitHubRef{}, err + } + return target.Ref, nil +} + +func parseGitHubTargetWithBaseURL(repo, githubBaseURL, defaultRef string) (gitHubTarget, error) { repo = strings.TrimSpace(repo) defaultRef = strings.TrimSpace(defaultRef) @@ -252,21 +265,26 @@ func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRe if strings.HasPrefix(repo, "http://") || strings.HasPrefix(repo, "https://") { u, err := url.Parse(repo) if err != nil { - return GitHubRef{}, fmt.Errorf("invalid URL: %w", err) + return gitHubTarget{}, fmt.Errorf("invalid URL: %w", err) } - if !isSupportedGitHubURL(u, githubBaseURL) { - return GitHubRef{}, fmt.Errorf("invalid GitHub URL host %q", u.Host) + matchedBaseURL := supportedGitHubBaseURL(u, githubBaseURL) + if matchedBaseURL == "" { + return gitHubTarget{}, fmt.Errorf("invalid GitHub URL host %q", u.Host) } - parts := parseGitHubRefPathParts(u, githubBaseURL) + endpoints, err := resolveGitHubEndpoints(matchedBaseURL) + if err != nil { + return gitHubTarget{}, err + } + parts := parseGitHubRefPathParts(u, matchedBaseURL) if len(parts) < 2 { - return GitHubRef{}, fmt.Errorf("invalid GitHub URL") + return gitHubTarget{}, fmt.Errorf("invalid GitHub URL") } if len(parts) > 2 { if parts[2] != "tree" && parts[2] != "blob" { - return GitHubRef{}, fmt.Errorf("invalid GitHub repository URL path %q", u.Path) + return gitHubTarget{}, fmt.Errorf("invalid GitHub repository URL path %q", u.Path) } if len(parts) < 4 { - return GitHubRef{}, fmt.Errorf("invalid GitHub %s URL path %q", parts[2], u.Path) + return gitHubTarget{}, fmt.Errorf("invalid GitHub %s URL path %q", parts[2], u.Path) } } ref := GitHubRef{ @@ -283,13 +301,18 @@ func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRe break } } - return ref, nil + return gitHubTarget{Ref: ref, Endpoints: endpoints}, nil + } + + endpoints, err := resolveGitHubEndpoints(githubBaseURL) + if err != nil { + return gitHubTarget{}, err } // Handle shorthand format parts := strings.Split(strings.Trim(repo, "/"), "/") if len(parts) < 2 { - return GitHubRef{}, fmt.Errorf("invalid format %q: expected 'owner/repo'", repo) + return gitHubTarget{}, fmt.Errorf("invalid format %q: expected 'owner/repo'", repo) } ref := GitHubRef{ Owner: parts[0], @@ -299,35 +322,43 @@ func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRe if len(parts) > 2 { ref.SubPath = strings.Join(parts[2:], "/") } - return ref, nil + return gitHubTarget{Ref: ref, Endpoints: endpoints}, nil } type gitHubRepository struct { DefaultBranch string `json:"default_branch"` } -func (si *SkillInstaller) resolveGitHubRef(ctx context.Context, repo, version string) (GitHubRef, error) { - ref, err := parseGitHubRefWithBaseURL(repo, si.githubBaseURL, "") +func (si *SkillInstaller) resolveGitHubTarget(ctx context.Context, repo, version string) (gitHubTarget, error) { + target, err := parseGitHubTargetWithBaseURL(repo, si.githubBaseURL, "") if err != nil { - return GitHubRef{}, err + return gitHubTarget{}, err } if version != "" { - ref.Ref = version - return ref, nil + target.Ref.Ref = version + return target, nil } - if ref.Ref != "" { - return ref, nil + if target.Ref.Ref != "" { + return target, nil } - defaultBranch, err := si.fetchDefaultBranch(ctx, ref.Owner, ref.RepoName) + defaultBranch, err := si.fetchDefaultBranchWithAPIBaseURL( + ctx, + target.Endpoints.APIBaseURL, + target.Ref.Owner, + target.Ref.RepoName, + ) if err != nil { - return GitHubRef{}, err + return gitHubTarget{}, err } - ref.Ref = defaultBranch - return ref, nil + target.Ref.Ref = defaultBranch + return target, nil } -func (si *SkillInstaller) fetchDefaultBranch(ctx context.Context, owner, repo string) (string, error) { - apiURL := fmt.Sprintf("%s/repos/%s/%s", strings.TrimRight(si.githubAPIBaseURL, "/"), owner, repo) +func (si *SkillInstaller) fetchDefaultBranchWithAPIBaseURL( + ctx context.Context, + apiBaseURL, owner, repo string, +) (string, error) { + apiURL := fmt.Sprintf("%s/repos/%s/%s", strings.TrimRight(apiBaseURL, "/"), owner, repo) req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) if err != nil { return "", err @@ -401,22 +432,24 @@ func (si *SkillInstaller) InstallFromGitHubToDir( ctx context.Context, repo, version, skillDirectory string, ) (*InstallResult, error) { - ref, err := si.resolveGitHubRef(ctx, repo, version) + target, err := si.resolveGitHubTarget(ctx, repo, version) if err != nil { return nil, err } + ref := target.Ref // Build GitHub API URL apiPath := path.Join(ref.Owner, ref.RepoName, "contents") if ref.SubPath != "" { apiPath = path.Join(apiPath, ref.SubPath) } - apiURL := fmt.Sprintf("%s/repos/%s?ref=%s", si.githubAPIBaseURL, apiPath, url.QueryEscape(ref.Ref)) + apiURL := fmt.Sprintf("%s/repos/%s?ref=%s", target.Endpoints.APIBaseURL, apiPath, url.QueryEscape(ref.Ref)) if err := si.getGithubDirAllFiles(ctx, apiURL, skillDirectory, true); err != nil { // Fallback to raw download if downloadErr := si.downloadRaw( ctx, + target.Endpoints.RawBaseURL, ref.Owner, ref.RepoName, ref.Ref, @@ -482,7 +515,10 @@ func (si *SkillInstaller) getGithubDirAllFiles(ctx context.Context, apiURL, loca } // downloadRaw is a fallback that downloads just SKILL.md from raw.githubusercontent.com -func (si *SkillInstaller) downloadRaw(ctx context.Context, owner, repo, ref, subPath, localDir string) error { +func (si *SkillInstaller) downloadRaw( + ctx context.Context, + rawBaseURL, owner, repo, ref, subPath, localDir string, +) error { urlPath := path.Join(owner, repo, ref) if subPath != "" { if isSkillMarkdownPath(subPath) { @@ -491,7 +527,7 @@ func (si *SkillInstaller) downloadRaw(ctx context.Context, owner, repo, ref, sub urlPath = path.Join(urlPath, subPath) } } - url := fmt.Sprintf("%s/%s/SKILL.md", si.githubRawBaseURL, urlPath) + url := fmt.Sprintf("%s/%s/SKILL.md", strings.TrimRight(rawBaseURL, "/"), urlPath) req, err := http.NewRequestWithContext(ctx, "GET", url, nil) if err != nil { diff --git a/pkg/skills/installer_test.go b/pkg/skills/installer_test.go index 6c334d00f..683689b35 100644 --- a/pkg/skills/installer_test.go +++ b/pkg/skills/installer_test.go @@ -260,6 +260,35 @@ func TestParseGitHubRefWithBaseURL(t *testing.T) { } } +func TestParseGitHubTargetWithBaseURLPreservesSourceEndpoints(t *testing.T) { + target, err := parseGitHubTargetWithBaseURL( + "https://github.com/org/repo/tree/main/.agents/skills/pr-review", + "https://ghe.example.com/git", + "", + ) + if err != nil { + t.Fatalf("parseGitHubTargetWithBaseURL() unexpected error = %v", err) + } + if target.Endpoints.WebBaseURL != "https://github.com" { + t.Fatalf("web base = %q, want https://github.com", target.Endpoints.WebBaseURL) + } + if target.Endpoints.APIBaseURL != "https://api.github.com" { + t.Fatalf("api base = %q, want https://api.github.com", target.Endpoints.APIBaseURL) + } + if target.Endpoints.RawBaseURL != "https://raw.githubusercontent.com" { + t.Fatalf("raw base = %q, want https://raw.githubusercontent.com", target.Endpoints.RawBaseURL) + } + if target.Ref.Owner != "org" || target.Ref.RepoName != "repo" { + t.Fatalf("unexpected ref = %+v", target.Ref) + } + if target.Ref.Ref != "main" { + t.Fatalf("ref = %q, want main", target.Ref.Ref) + } + if target.Ref.SubPath != ".agents/skills/pr-review" { + t.Fatalf("subPath = %q, want .agents/skills/pr-review", target.Ref.SubPath) + } +} + func TestSkillInstallerResolveGitHubRefUsesDefaultBranch(t *testing.T) { server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { @@ -277,10 +306,11 @@ func TestSkillInstallerResolveGitHubRefUsesDefaultBranch(t *testing.T) { t.Fatalf("NewSkillInstallerWithBaseURL() error = %v", err) } - ref, err := installer.resolveGitHubRef(context.Background(), "org/repo/skills/test", "") + target, err := installer.resolveGitHubTarget(context.Background(), "org/repo/skills/test", "") if err != nil { - t.Fatalf("resolveGitHubRef() error = %v", err) + t.Fatalf("resolveGitHubTarget() error = %v", err) } + ref := target.Ref if ref.Ref != "master" { t.Fatalf("ref = %q, want master", ref.Ref) } diff --git a/pkg/tools/skills_install.go b/pkg/tools/skills_install.go index be8206edc..1bab82bf2 100644 --- a/pkg/tools/skills_install.go +++ b/pkg/tools/skills_install.go @@ -222,12 +222,10 @@ type originMeta struct { } func writeOriginMeta(targetDir string, registry skills.SkillRegistry, slug, version string) error { - normalizedSlug := skills.NormalizeInstallTargetForRegistryInstance(registry, slug) + normalizedSlug, registryURL := skills.BuildInstallMetadataForRegistryInstance(registry, slug, version) registryName := "" - registryURL := "" if registry != nil { registryName = registry.Name() - registryURL = registry.SkillURL(normalizedSlug, version) } meta := originMeta{ diff --git a/pkg/tools/skills_install_test.go b/pkg/tools/skills_install_test.go index f9248743d..ec8902b36 100644 --- a/pkg/tools/skills_install_test.go +++ b/pkg/tools/skills_install_test.go @@ -267,6 +267,34 @@ func TestInstallSkillToolAllowsGitHubURLSlug(t *testing.T) { assert.NotZero(t, meta.InstalledAt) } +func TestInstallSkillToolPreservesGitHubSourceURLWithEnterpriseRegistry(t *testing.T) { + registry := skills.GitHubRegistryConfig{Enabled: true, BaseURL: "https://ghe.example.com/git"}.BuildRegistry() + githubRegistry, ok := registry.(*skills.GitHubRegistry) + require.True(t, ok) + + registryMgr := skills.NewRegistryManager() + registryMgr.AddRegistry(&stubGitHubInstallRegistry{GitHubRegistry: githubRegistry}) + workspace := t.TempDir() + tool := NewInstallSkillTool(registryMgr, workspace) + + slug := "https://github.com/synthetic-lab/octofriend/tree/main/.agents/skills/pr-review" + result := tool.Execute(context.Background(), map[string]any{ + "slug": slug, + "registry": "github", + }) + + assert.False(t, result.IsError) + + data, err := os.ReadFile(filepath.Join(workspace, "skills", "pr-review", ".skill-origin.json")) + require.NoError(t, err) + + var meta originMeta + require.NoError(t, json.Unmarshal(data, &meta)) + assert.Equal(t, "synthetic-lab/octofriend/.agents/skills/pr-review", meta.Slug) + assert.Equal(t, slug, meta.RegistryURL) + assert.Equal(t, "main", meta.InstalledVersion) +} + func TestInstallSkillToolRejectsInvalidInstalledSkill(t *testing.T) { workspace := t.TempDir() registryMgr := skills.NewRegistryManager() diff --git a/web/backend/api/skills.go b/web/backend/api/skills.go index 47d5c0a43..8d8995fd9 100644 --- a/web/backend/api/skills.go +++ b/web/backend/api/skills.go @@ -381,13 +381,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) + normalizedSlug, registryURL := skills.BuildInstallMetadataForRegistryInstance(registry, req.Slug, result.Version) if err := persistSkillOriginMeta(stagedTargetDir, installedSkillOriginMeta{ Version: 1, OriginKind: "third_party", Registry: registry.Name(), Slug: normalizedSlug, - RegistryURL: registrySkillURL(cfg, registry.Name(), normalizedSlug, result.Version), + RegistryURL: registryURL, InstalledVersion: result.Version, InstalledAt: installedAt, }); err != nil { @@ -422,7 +422,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(), normalizedSlug, result.Version), + RegistryURL: registryURL, InstalledVersion: result.Version, InstalledAt: installedAt, }