diff --git a/pkg/config/config_struct.go b/pkg/config/config_struct.go index 58e2b4337..3ce1ff3f5 100644 --- a/pkg/config/config_struct.go +++ b/pkg/config/config_struct.go @@ -505,6 +505,24 @@ func (v *SkillsRegistriesConfig) UnmarshalYAML(value *yaml.Node) error { logger.Errorf("Decode error: %v", err) return err } + if len(*v) == 0 { + keys := make([]string, 0, len(mm)) + for name := range mm { + keys = append(keys, name) + } + sort.Strings(keys) + list := make([]*SkillRegistryConfig, 0, len(keys)) + for _, name := range keys { + registry := mm[name] + if registry == nil { + continue + } + registry.Name = name + list = append(list, registry) + } + *v = list + return nil + } for _, registry := range *v { if registry == nil { continue @@ -512,6 +530,21 @@ func (v *SkillsRegistriesConfig) UnmarshalYAML(value *yaml.Node) error { sec := mm[registry.Name] if sec != nil { registry.AuthToken = sec.AuthToken + if registry.BaseURL == "" { + registry.BaseURL = sec.BaseURL + } + if !registry.Enabled { + registry.Enabled = sec.Enabled + } + if registry.Param == nil { + registry.Param = map[string]any{} + } + for key, value := range sec.Param { + if _, ok := registry.Param[key]; ok { + continue + } + registry.Param[key] = value + } } } return nil diff --git a/pkg/config/config_struct_test.go b/pkg/config/config_struct_test.go index 0b5ad9627..07bb14b7b 100644 --- a/pkg/config/config_struct_test.go +++ b/pkg/config/config_struct_test.go @@ -204,6 +204,22 @@ func TestSkillsRegistriesConfigMarshalYAMLIncludesRegistryToken(t *testing.T) { assert.Equal(t, "registry-auth-token", github.AuthToken.String()) } +func TestSkillsRegistriesConfigUnmarshalYAMLBuildsEntriesFromEmptySlice(t *testing.T) { + var registries SkillsRegistriesConfig + err := yaml.Unmarshal([]byte(`github: + enabled: true + base_url: https://ghe.example.com/git + proxy: http://127.0.0.1:7890 +`), ®istries) + assert.NoError(t, err) + + github, ok := registries.Get("github") + assert.True(t, ok) + assert.True(t, github.Enabled) + assert.Equal(t, "https://ghe.example.com/git", github.BaseURL) + assert.Equal(t, "http://127.0.0.1:7890", github.Param["proxy"]) +} + func TestSkillsRegistriesConfigMarshalJSONPreservesObjectShape(t *testing.T) { registries := SkillsRegistriesConfig{ &SkillRegistryConfig{ diff --git a/pkg/skills/github_registry.go b/pkg/skills/github_registry.go index 834dc7a1b..ac8ccf6e3 100644 --- a/pkg/skills/github_registry.go +++ b/pkg/skills/github_registry.go @@ -47,6 +47,8 @@ type GitHubRegistry struct { webBase string } +const githubAuthTokenHelp = "configure registries.github.auth_token" + func (c GitHubRegistryConfig) IsEnabled() bool { return c.Enabled } @@ -73,19 +75,19 @@ func (r *GitHubRegistry) ResolveInstallDirName(target string) (string, error) { 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 "" } base := strings.TrimRight(r.webBase, "/") urlPath := path.Join(ref.Owner, ref.RepoName) + if ref.Ref == "" { + 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) } - if ref.Ref != "" && ref.Ref != "main" { + if ref.Ref != "main" { return fmt.Sprintf("%s/%s/tree/%s", base, urlPath, url.PathEscape(ref.Ref)) } return fmt.Sprintf("%s/%s", base, urlPath) @@ -145,10 +147,10 @@ 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, nil + return nil, fmt.Errorf("github search requires authentication; %s", githubAuthTokenHelp) } if resp.StatusCode == http.StatusForbidden && r.installer.githubToken == "" && isGitHubRateLimitError(body) { - return nil, nil + return nil, fmt.Errorf("github search hit the unauthenticated rate limit; %s", githubAuthTokenHelp) } if resp.StatusCode < 200 || resp.StatusCode >= 300 { return nil, fmt.Errorf("github search failed: HTTP %d: %s", resp.StatusCode, string(body)) @@ -236,11 +238,17 @@ func githubSearchDisplayName(item gitHubCodeSearchItem) string { return strings.TrimSpace(item.Repository.FullName) } -func (r *GitHubRegistry) GetSkillMeta(_ context.Context, target string) (*SkillMeta, error) { - ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "main") +func (r *GitHubRegistry) GetSkillMeta(ctx context.Context, target string) (*SkillMeta, error) { + ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "") if err != nil { return nil, err } + if ref.Ref == "" { + ref.Ref, err = r.installer.fetchDefaultBranch(ctx, ref.Owner, ref.RepoName) + if err != nil { + return nil, err + } + } return &SkillMeta{ Slug: target, DisplayName: ref.RepoName, diff --git a/pkg/skills/github_registry_test.go b/pkg/skills/github_registry_test.go index 1f47bb97a..57f4c08f6 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 TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedRateLimit(t *testing.T) { +func TestGitHubRegistrySearchReturnsHelpfulErrorOnUnauthenticatedRateLimit(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,11 +115,12 @@ func TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedRateLimit(t *testing.T require.NotNil(t, registry) results, err := registry.Search(context.Background(), "pr review", 5) - require.NoError(t, err) - assert.Empty(t, results) + require.Error(t, err) + assert.Nil(t, results) + assert.Contains(t, err.Error(), "registries.github.auth_token") } -func TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedAuthRequired(t *testing.T) { +func TestGitHubRegistrySearchReturnsHelpfulErrorOnUnauthenticatedAuthRequired(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) @@ -133,8 +134,9 @@ func TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedAuthRequired(t *testin require.NotNil(t, registry) results, err := registry.Search(context.Background(), "pr review", 5) - require.NoError(t, err) - assert.Empty(t, results) + require.Error(t, err) + assert.Nil(t, results) + assert.Contains(t, err.Error(), "registries.github.auth_token") } func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) { diff --git a/pkg/skills/installer.go b/pkg/skills/installer.go index f420dc6d1..20c8cc086 100644 --- a/pkg/skills/installer.go +++ b/pkg/skills/installer.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "net/http" "net/url" "os" @@ -176,9 +177,6 @@ func parseGitHubRef(repo string) (GitHubRef, error) { 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://") { @@ -224,6 +222,64 @@ func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRe return ref, 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, "") + if err != nil { + return GitHubRef{}, err + } + if version != "" { + ref.Ref = version + return ref, nil + } + if ref.Ref != "" { + return ref, nil + } + defaultBranch, err := si.fetchDefaultBranch(ctx, ref.Owner, ref.RepoName) + if err != nil { + return GitHubRef{}, err + } + ref.Ref = defaultBranch + return ref, 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) + req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) + if err != nil { + return "", err + } + if si.githubToken != "" { + req.Header.Set("Authorization", "Bearer "+si.githubToken) + } + + resp, err := utils.DoRequestWithRetry(si.client, req) + if err != nil { + return "", err + } + defer resp.Body.Close() + + body, err := io.ReadAll(io.LimitReader(resp.Body, 1<<20)) + if err != nil { + return "", fmt.Errorf("failed to read repository metadata: %w", err) + } + if resp.StatusCode != http.StatusOK { + return "", fmt.Errorf("failed to resolve default branch: HTTP %d: %s", resp.StatusCode, string(body)) + } + + var repository gitHubRepository + if err := json.Unmarshal(body, &repository); err != nil { + return "", fmt.Errorf("failed to parse repository metadata: %w", err) + } + if strings.TrimSpace(repository.DefaultBranch) == "" { + return "", fmt.Errorf("repository %s/%s did not report a default branch", owner, repo) + } + return repository.DefaultBranch, nil +} + func githubInstallDirNameWithBaseURL(repo, githubBaseURL string) (string, error) { if !strings.HasPrefix(repo, "http://") && !strings.HasPrefix(repo, "https://") { if err := ValidateInstallTarget(repo); err != nil { @@ -258,13 +314,10 @@ func (si *SkillInstaller) InstallFromGitHubToDir( ctx context.Context, repo, version, skillDirectory string, ) (*InstallResult, error) { - ref, err := parseGitHubRefWithBaseURL(repo, si.githubBaseURL, "main") + ref, err := si.resolveGitHubRef(ctx, repo, version) if err != nil { return nil, err } - if version != "" { - ref.Ref = version - } // Build GitHub API URL apiPath := path.Join(ref.Owner, ref.RepoName, "contents") diff --git a/pkg/skills/installer_test.go b/pkg/skills/installer_test.go index 5281c68ad..abdd5e042 100644 --- a/pkg/skills/installer_test.go +++ b/pkg/skills/installer_test.go @@ -159,6 +159,43 @@ func TestParseGitHubRefWithBaseURL(t *testing.T) { if dirName != "test" { t.Fatalf("dirName = %q, want test", dirName) } + + ref, err = parseGitHubRefWithBaseURL("https://ghe.example.com/git/org/repo", "https://ghe.example.com/git", "") + if err != nil { + t.Fatalf("parseGitHubRefWithBaseURL() unexpected error = %v", err) + } + if ref.Ref != "" { + t.Fatalf("ref = %q, want empty", ref.Ref) + } +} + +func TestSkillInstallerResolveGitHubRefUsesDefaultBranch(t *testing.T) { + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + switch r.URL.Path { + case "/api/v3/repos/org/repo": + w.Header().Set("Content-Type", "application/json") + _, _ = w.Write([]byte(`{"default_branch":"master"}`)) + default: + t.Fatalf("unexpected path: %s", r.URL.Path) + } + })) + defer server.Close() + + installer, err := NewSkillInstallerWithBaseURL(t.TempDir(), server.URL, "", "") + if err != nil { + t.Fatalf("NewSkillInstallerWithBaseURL() error = %v", err) + } + + ref, err := installer.resolveGitHubRef(context.Background(), "org/repo/skills/test", "") + if err != nil { + t.Fatalf("resolveGitHubRef() error = %v", err) + } + if ref.Ref != "master" { + t.Fatalf("ref = %q, want master", ref.Ref) + } + if ref.SubPath != "skills/test" { + t.Fatalf("subPath = %q, want skills/test", ref.SubPath) + } } func TestShouldDownload(t *testing.T) { diff --git a/web/backend/api/skills_test.go b/web/backend/api/skills_test.go index 6b41239bd..0174a08a2 100644 --- a/web/backend/api/skills_test.go +++ b/web/backend/api/skills_test.go @@ -1111,16 +1111,18 @@ func TestHandleInstallSkillDefaultsRegistryToGitHub(t *testing.T) { var server *httptest.Server server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { switch r.URL.Path { - case "/repos/foo/bar/contents/.agents/skills/pr-review": - assert.Equal(t, "ref=main", r.URL.RawQuery) + 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/main/.agents/skills/pr-review/SKILL.md", + "download_url": server.URL + "/raw/foo/bar/master/.agents/skills/pr-review/SKILL.md", }, }) - case "/raw/foo/bar/main/.agents/skills/pr-review/SKILL.md": + 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)