fix github registry target resolution and metadata links
This commit is contained in:
parent
0af73b1774
commit
5743708b95
9 changed files with 165 additions and 49 deletions
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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) {
|
||||
|
|
|
|||
|
|
@ -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 {
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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{
|
||||
|
|
|
|||
|
|
@ -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()
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue