fix skills registry yaml and github default branch handling
This commit is contained in:
parent
91d445987a
commit
61a43b173a
7 changed files with 176 additions and 25 deletions
|
|
@ -505,6 +505,24 @@ func (v *SkillsRegistriesConfig) UnmarshalYAML(value *yaml.Node) error {
|
||||||
logger.Errorf("Decode error: %v", err)
|
logger.Errorf("Decode error: %v", err)
|
||||||
return 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 {
|
for _, registry := range *v {
|
||||||
if registry == nil {
|
if registry == nil {
|
||||||
continue
|
continue
|
||||||
|
|
@ -512,6 +530,21 @@ func (v *SkillsRegistriesConfig) UnmarshalYAML(value *yaml.Node) error {
|
||||||
sec := mm[registry.Name]
|
sec := mm[registry.Name]
|
||||||
if sec != nil {
|
if sec != nil {
|
||||||
registry.AuthToken = sec.AuthToken
|
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
|
return nil
|
||||||
|
|
|
||||||
|
|
@ -204,6 +204,22 @@ func TestSkillsRegistriesConfigMarshalYAMLIncludesRegistryToken(t *testing.T) {
|
||||||
assert.Equal(t, "registry-auth-token", github.AuthToken.String())
|
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) {
|
func TestSkillsRegistriesConfigMarshalJSONPreservesObjectShape(t *testing.T) {
|
||||||
registries := SkillsRegistriesConfig{
|
registries := SkillsRegistriesConfig{
|
||||||
&SkillRegistryConfig{
|
&SkillRegistryConfig{
|
||||||
|
|
|
||||||
|
|
@ -47,6 +47,8 @@ type GitHubRegistry struct {
|
||||||
webBase string
|
webBase string
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const githubAuthTokenHelp = "configure registries.github.auth_token"
|
||||||
|
|
||||||
func (c GitHubRegistryConfig) IsEnabled() bool {
|
func (c GitHubRegistryConfig) IsEnabled() bool {
|
||||||
return c.Enabled
|
return c.Enabled
|
||||||
}
|
}
|
||||||
|
|
@ -73,19 +75,19 @@ func (r *GitHubRegistry) ResolveInstallDirName(target string) (string, error) {
|
||||||
|
|
||||||
func (r *GitHubRegistry) SkillURL(target, version string) string {
|
func (r *GitHubRegistry) SkillURL(target, version string) string {
|
||||||
defaultRef := strings.TrimSpace(version)
|
defaultRef := strings.TrimSpace(version)
|
||||||
if defaultRef == "" {
|
|
||||||
defaultRef = "main"
|
|
||||||
}
|
|
||||||
ref, err := parseGitHubRefWithBaseURL(target, r.webBase, defaultRef)
|
ref, err := parseGitHubRefWithBaseURL(target, r.webBase, defaultRef)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return ""
|
return ""
|
||||||
}
|
}
|
||||||
base := strings.TrimRight(r.webBase, "/")
|
base := strings.TrimRight(r.webBase, "/")
|
||||||
urlPath := path.Join(ref.Owner, ref.RepoName)
|
urlPath := path.Join(ref.Owner, ref.RepoName)
|
||||||
|
if ref.Ref == "" {
|
||||||
|
return fmt.Sprintf("%s/%s", base, urlPath)
|
||||||
|
}
|
||||||
if ref.SubPath != "" {
|
if ref.SubPath != "" {
|
||||||
return fmt.Sprintf("%s/%s/tree/%s/%s", base, urlPath, url.PathEscape(ref.Ref), 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/tree/%s", base, urlPath, url.PathEscape(ref.Ref))
|
||||||
}
|
}
|
||||||
return fmt.Sprintf("%s/%s", base, urlPath)
|
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)
|
return nil, fmt.Errorf("failed to read github search response: %w", err)
|
||||||
}
|
}
|
||||||
if resp.StatusCode == http.StatusUnauthorized && r.installer.githubToken == "" && isGitHubAuthRequiredError(body) {
|
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) {
|
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 {
|
if resp.StatusCode < 200 || resp.StatusCode >= 300 {
|
||||||
return nil, fmt.Errorf("github search failed: HTTP %d: %s", resp.StatusCode, string(body))
|
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)
|
return strings.TrimSpace(item.Repository.FullName)
|
||||||
}
|
}
|
||||||
|
|
||||||
func (r *GitHubRegistry) GetSkillMeta(_ context.Context, target string) (*SkillMeta, error) {
|
func (r *GitHubRegistry) GetSkillMeta(ctx context.Context, target string) (*SkillMeta, error) {
|
||||||
ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "main")
|
ref, err := parseGitHubRefWithBaseURL(target, r.webBase, "")
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
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{
|
return &SkillMeta{
|
||||||
Slug: target,
|
Slug: target,
|
||||||
DisplayName: ref.RepoName,
|
DisplayName: ref.RepoName,
|
||||||
|
|
|
||||||
|
|
@ -103,7 +103,7 @@ func TestGitHubRegistryProviderDecodesProxyParam(t *testing.T) {
|
||||||
assert.Equal(t, "http://127.0.0.1:7890", ghRegistry.installer.proxy)
|
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) {
|
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
assert.Empty(t, r.Header.Get("Authorization"))
|
assert.Empty(t, r.Header.Get("Authorization"))
|
||||||
w.WriteHeader(http.StatusForbidden)
|
w.WriteHeader(http.StatusForbidden)
|
||||||
|
|
@ -115,11 +115,12 @@ func TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedRateLimit(t *testing.T
|
||||||
require.NotNil(t, registry)
|
require.NotNil(t, registry)
|
||||||
|
|
||||||
results, err := registry.Search(context.Background(), "pr review", 5)
|
results, err := registry.Search(context.Background(), "pr review", 5)
|
||||||
require.NoError(t, err)
|
require.Error(t, err)
|
||||||
assert.Empty(t, results)
|
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) {
|
server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
assert.Empty(t, r.Header.Get("Authorization"))
|
assert.Empty(t, r.Header.Get("Authorization"))
|
||||||
w.WriteHeader(http.StatusUnauthorized)
|
w.WriteHeader(http.StatusUnauthorized)
|
||||||
|
|
@ -133,8 +134,9 @@ func TestGitHubRegistrySearchReturnsEmptyOnUnauthenticatedAuthRequired(t *testin
|
||||||
require.NotNil(t, registry)
|
require.NotNil(t, registry)
|
||||||
|
|
||||||
results, err := registry.Search(context.Background(), "pr review", 5)
|
results, err := registry.Search(context.Background(), "pr review", 5)
|
||||||
require.NoError(t, err)
|
require.Error(t, err)
|
||||||
assert.Empty(t, results)
|
assert.Nil(t, results)
|
||||||
|
assert.Contains(t, err.Error(), "registries.github.auth_token")
|
||||||
}
|
}
|
||||||
|
|
||||||
func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) {
|
func TestGitHubRegistrySkillURLUsesProvidedVersionAndBasePath(t *testing.T) {
|
||||||
|
|
|
||||||
|
|
@ -4,6 +4,7 @@ import (
|
||||||
"context"
|
"context"
|
||||||
"encoding/json"
|
"encoding/json"
|
||||||
"fmt"
|
"fmt"
|
||||||
|
"io"
|
||||||
"net/http"
|
"net/http"
|
||||||
"net/url"
|
"net/url"
|
||||||
"os"
|
"os"
|
||||||
|
|
@ -176,9 +177,6 @@ func parseGitHubRef(repo string) (GitHubRef, error) {
|
||||||
func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRef, error) {
|
func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRef, error) {
|
||||||
repo = strings.TrimSpace(repo)
|
repo = strings.TrimSpace(repo)
|
||||||
defaultRef = strings.TrimSpace(defaultRef)
|
defaultRef = strings.TrimSpace(defaultRef)
|
||||||
if defaultRef == "" {
|
|
||||||
defaultRef = "main"
|
|
||||||
}
|
|
||||||
|
|
||||||
// Handle full URL
|
// Handle full URL
|
||||||
if strings.HasPrefix(repo, "http://") || strings.HasPrefix(repo, "https://") {
|
if strings.HasPrefix(repo, "http://") || strings.HasPrefix(repo, "https://") {
|
||||||
|
|
@ -224,6 +222,64 @@ func parseGitHubRefWithBaseURL(repo, githubBaseURL, defaultRef string) (GitHubRe
|
||||||
return ref, nil
|
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) {
|
func githubInstallDirNameWithBaseURL(repo, githubBaseURL string) (string, error) {
|
||||||
if !strings.HasPrefix(repo, "http://") && !strings.HasPrefix(repo, "https://") {
|
if !strings.HasPrefix(repo, "http://") && !strings.HasPrefix(repo, "https://") {
|
||||||
if err := ValidateInstallTarget(repo); err != nil {
|
if err := ValidateInstallTarget(repo); err != nil {
|
||||||
|
|
@ -258,13 +314,10 @@ func (si *SkillInstaller) InstallFromGitHubToDir(
|
||||||
ctx context.Context,
|
ctx context.Context,
|
||||||
repo, version, skillDirectory string,
|
repo, version, skillDirectory string,
|
||||||
) (*InstallResult, error) {
|
) (*InstallResult, error) {
|
||||||
ref, err := parseGitHubRefWithBaseURL(repo, si.githubBaseURL, "main")
|
ref, err := si.resolveGitHubRef(ctx, repo, version)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
return nil, err
|
return nil, err
|
||||||
}
|
}
|
||||||
if version != "" {
|
|
||||||
ref.Ref = version
|
|
||||||
}
|
|
||||||
|
|
||||||
// Build GitHub API URL
|
// Build GitHub API URL
|
||||||
apiPath := path.Join(ref.Owner, ref.RepoName, "contents")
|
apiPath := path.Join(ref.Owner, ref.RepoName, "contents")
|
||||||
|
|
|
||||||
|
|
@ -159,6 +159,43 @@ func TestParseGitHubRefWithBaseURL(t *testing.T) {
|
||||||
if dirName != "test" {
|
if dirName != "test" {
|
||||||
t.Fatalf("dirName = %q, want test", dirName)
|
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) {
|
func TestShouldDownload(t *testing.T) {
|
||||||
|
|
|
||||||
|
|
@ -1111,16 +1111,18 @@ func TestHandleInstallSkillDefaultsRegistryToGitHub(t *testing.T) {
|
||||||
var server *httptest.Server
|
var server *httptest.Server
|
||||||
server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
server = httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
|
||||||
switch r.URL.Path {
|
switch r.URL.Path {
|
||||||
case "/repos/foo/bar/contents/.agents/skills/pr-review":
|
case "/api/v3/repos/foo/bar":
|
||||||
assert.Equal(t, "ref=main", r.URL.RawQuery)
|
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{
|
json.NewEncoder(w).Encode([]map[string]any{
|
||||||
{
|
{
|
||||||
"type": "file",
|
"type": "file",
|
||||||
"name": "SKILL.md",
|
"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"))
|
_, _ = w.Write([]byte("---\nname: pr-review\ndescription: PR review skill\n---\n# PR Review\n"))
|
||||||
default:
|
default:
|
||||||
http.NotFound(w, r)
|
http.NotFound(w, r)
|
||||||
|
|
|
||||||
Loading…
Add table
Reference in a new issue