From 5c7618d3f2de9a2ccc531815aafd202a9a9ad06e Mon Sep 17 00:00:00 2001 From: git-hulk Date: Mon, 31 Aug 2026 21:30:00 +0800 Subject: [PATCH] Resolve skill repository versions over Git transport Add the Git transport layer of repository-versioned skill management: querying refs and fetching snapshots through git instead of the GitHub Releases, Tags, or Commits REST APIs, so the same code path supports GitHub, GitHub Enterprise, and other Git hosts without REST quotas. - ResolveVersion locks a user-supplied spec to an immutable version: "latest" (or an omitted version) selects the highest stable SemVer tag via ls-remote, falling back to the default branch's full HEAD SHA when the repository has no stable SemVer tags; a branch or short commit resolves to the full commit SHA; a prerelease tag is selected only when named explicitly. - fetchSnapshot shallow-fetches one tag or commit into a directory and strips Git metadata, with a full-history fallback for hosts that refuse fetching by SHA. - Private repositories reuse whatever authentication Git already has (SSH agent, credential helpers, .netrc, insteadOf rules); prompts are disabled and stderr is credential-sanitized before it is attached to errors. Integration tests run against local fixture repositories routed through Git insteadOf rules, which also exercises the design's claim that local Git configuration chooses the transport. Adds golang.org/x/mod for SemVer comparison. Assistant By Fable 5 Co-Authored-By: Claude Fable 5 --- go.mod | 3 +- go.sum | 2 + internal/skill/git.go | 202 +++++++++++++++++++++++++++++++++ internal/skill/git_test.go | 130 +++++++++++++++++++++ internal/skill/helpers_test.go | 84 ++++++++++++++ internal/skill/repo.go | 5 + 6 files changed, 425 insertions(+), 1 deletion(-) create mode 100644 internal/skill/git.go create mode 100644 internal/skill/git_test.go create mode 100644 internal/skill/helpers_test.go diff --git a/go.mod b/go.mod index f5b5d74..fae8b4a 100644 --- a/go.mod +++ b/go.mod @@ -1,11 +1,12 @@ module github.com/git-hulk/clime -go 1.25 +go 1.25.0 require ( github.com/alperdrsnn/clime v1.1.2 github.com/spf13/cobra v1.10.2 github.com/stretchr/testify v1.12.1 + golang.org/x/mod v0.40.0 golang.org/x/term v0.33.0 gopkg.in/yaml.v3 v3.0.1 ) diff --git a/go.sum b/go.sum index 02ab10c..67f3149 100644 --- a/go.sum +++ b/go.sum @@ -13,6 +13,8 @@ github.com/stretchr/testify v1.12.1/go.mod h1:MDEgiDPPsNp5cuIrHPPCyornHKgEVbtFUm go.yaml.in/yaml/v3 v3.0.4/go.mod h1:DhzuOOF2ATzADvBadXxruRBLzYTpT36CKvDb3+aBEFg= go.yaml.in/yaml/v3 v3.0.5 h1:N6y/pJk8buWs9NY5ERU2HSMfm+IuD/OtfdAnq6kESPw= go.yaml.in/yaml/v3 v3.0.5/go.mod h1:HVTZu1O7/Vkt2N+BFy8Zza+lnLsABggaTM2ZpNIGuKg= +golang.org/x/mod v0.40.0 h1:hUv+3cXcdRHz08UmSiOob7sadHig73uo5bkXxQ/tvUs= +golang.org/x/mod v0.40.0/go.mod h1:0/weTWkPWGBikyTWAX3dkjVztMmBA5hM0DH6BElSupE= golang.org/x/sys v0.34.0 h1:H5Y5sJ2L2JRdyv7ROF1he/lPdvFsd0mJHFw2ThKHxLA= golang.org/x/sys v0.34.0/go.mod h1:BJP2sWEmIv4KK5OTEluFJCKSidICx8ciO85XgH3Ak8k= golang.org/x/term v0.33.0 h1:NuFncQrRcaRvVmgRkvM3j/F00gWIAlcmlB8ACEKmGIg= diff --git a/internal/skill/git.go b/internal/skill/git.go new file mode 100644 index 0000000..5c0813b --- /dev/null +++ b/internal/skill/git.go @@ -0,0 +1,202 @@ +package skill + +import ( + "bytes" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + + "golang.org/x/mod/semver" +) + +// runGit executes git with prompts disabled and returns stdout. Stderr is +// credential-sanitized before it is attached to an error. It is a variable so +// tests can observe or disable network access. +var runGit = func(dir string, args ...string) (string, error) { + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), "GIT_TERMINAL_PROMPT=0") + var stdout, stderr bytes.Buffer + cmd.Stdout = &stdout + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + return "", fmt.Errorf("git %s failed: %w: %s", + args[0], err, strings.TrimSpace(SanitizeCredentials(stderr.String()))) + } + return stdout.String(), nil +} + +type remoteRefs struct { + headSHA string + defaultBranch string + branches map[string]string + tags map[string]string +} + +// lsRemote queries the remote's refs and default branch over Git transport. +func lsRemote(id RepoID) (*remoteRefs, error) { + out, err := runGit("", "ls-remote", "--symref", id.CloneURL()) + if err != nil { + return nil, fmt.Errorf("failed to query %s: %w", id.Canonical(), err) + } + refs := &remoteRefs{branches: map[string]string{}, tags: map[string]string{}} + for line := range strings.SplitSeq(out, "\n") { + fields := strings.Fields(line) + if len(fields) < 2 { + continue + } + if fields[0] == "ref:" && fields[len(fields)-1] == "HEAD" { + refs.defaultBranch = strings.TrimPrefix(fields[1], "refs/heads/") + continue + } + sha, ref := fields[0], fields[1] + switch { + case ref == "HEAD": + refs.headSHA = sha + case strings.HasPrefix(ref, "refs/heads/"): + refs.branches[strings.TrimPrefix(ref, "refs/heads/")] = sha + case strings.HasPrefix(ref, "refs/tags/"): + name := strings.TrimPrefix(ref, "refs/tags/") + // A peeled entry (tag^{}) points at the commit; prefer it. + if peeled, ok := strings.CutSuffix(name, "^{}"); ok { + refs.tags[peeled] = sha + } else if _, ok := refs.tags[name]; !ok { + refs.tags[name] = sha + } + } + } + return refs, nil +} + +// latestStableTag returns the highest stable SemVer tag name, or "" when the +// repository has no stable SemVer tags. Tags with and without a leading "v" +// are both recognized; prereleases and build-metadata tags are skipped. +func latestStableTag(tags map[string]string) string { + var best, bestCanon string + for name := range tags { + canon := name + if !strings.HasPrefix(canon, "v") { + canon = "v" + canon + } + if !semver.IsValid(canon) || semver.Prerelease(canon) != "" || semver.Build(canon) != "" { + continue + } + if best == "" || semver.Compare(canon, bestCanon) > 0 { + best, bestCanon = name, canon + } + } + return best +} + +// ResolveVersion resolves a user-supplied version spec against the remote to +// a locked version: a tag name kept verbatim, or a full commit SHA. An empty +// spec or "latest" selects the highest stable SemVer tag, falling back to the +// default branch's HEAD commit when the repository has no stable SemVer tags. +func ResolveVersion(id RepoID, spec string) (string, error) { + if IsFullCommit(spec) { + return spec, nil + } + refs, err := lsRemote(id) + if err != nil { + return "", err + } + if spec == "" || spec == "latest" { + if tag := latestStableTag(refs.tags); tag != "" { + return tag, nil + } + if refs.headSHA == "" { + return "", fmt.Errorf("repository %s has no stable SemVer tag and no HEAD commit", id.Canonical()) + } + return refs.headSHA, nil + } + if _, ok := refs.tags[spec]; ok { + return spec, nil + } + if sha, ok := refs.branches[spec]; ok { + return sha, nil + } + if looksLikeShortCommit(spec) { + return expandShortCommit(id, spec) + } + return "", fmt.Errorf("version %q not found in %s: no matching tag, branch, or commit", spec, id.Canonical()) +} + +// expandShortCommit resolves a short commit hash to a full SHA by fetching +// history metadata into a throwaway repository. +func expandShortCommit(id RepoID, short string) (string, error) { + tmp, err := os.MkdirTemp("", "clime-resolve-*") + if err != nil { + return "", err + } + defer os.RemoveAll(tmp) + + if _, err := runGit(tmp, "init", "-q", "--bare"); err != nil { + return "", err + } + if _, err := runGit(tmp, "remote", "add", "origin", id.CloneURL()); err != nil { + return "", err + } + refspecs := []string{"+refs/heads/*:refs/heads/*", "+refs/tags/*:refs/tags/*"} + // A treeless fetch keeps this cheap; fall back for hosts without + // partial-clone support. + if _, err := runGit(tmp, append([]string{"fetch", "-q", "--filter=tree:0", "origin"}, refspecs...)...); err != nil { + if _, err := runGit(tmp, append([]string{"fetch", "-q", "origin"}, refspecs...)...); err != nil { + return "", fmt.Errorf("failed to resolve commit %q in %s: %w", short, id.Canonical(), err) + } + } + out, err := runGit(tmp, "rev-parse", "--verify", short+"^{commit}") + if err != nil { + return "", fmt.Errorf("commit %q not found in %s: %w", short, id.Canonical(), err) + } + return strings.TrimSpace(out), nil +} + +func initRemote(dir string, id RepoID) error { + if _, err := runGit(dir, "init", "-q"); err != nil { + return err + } + _, err := runGit(dir, "remote", "add", "origin", id.CloneURL()) + return err +} + +// fetchSnapshot checks out the given locked version (tag or full commit SHA) +// into dir and strips Git metadata, returning the resolved commit SHA. +func fetchSnapshot(id RepoID, version, dir string) (string, error) { + if err := initRemote(dir, id); err != nil { + return "", err + } + if IsFullCommit(version) { + if _, err := runGit(dir, "fetch", "-q", "--depth", "1", "origin", version); err != nil { + // Some hosts refuse fetching by SHA; fall back to a full + // branch fetch and check out the commit from history. + if _, ferr := runGit(dir, "fetch", "-q", "origin", "+refs/heads/*:refs/remotes/origin/*"); ferr != nil { + return "", fmt.Errorf("failed to fetch %s@%s: %w", id.Canonical(), version, err) + } + if _, cerr := runGit(dir, "checkout", "-q", "--detach", version); cerr != nil { + return "", fmt.Errorf("failed to fetch %s@%s: %w", id.Canonical(), version, err) + } + return finishSnapshot(dir) + } + } else { + if _, err := runGit(dir, "fetch", "-q", "--depth", "1", "origin", "refs/tags/"+version); err != nil { + return "", fmt.Errorf("failed to fetch %s@%s: %w", id.Canonical(), version, err) + } + } + if _, err := runGit(dir, "checkout", "-q", "--detach", "FETCH_HEAD"); err != nil { + return "", fmt.Errorf("failed to check out %s@%s: %w", id.Canonical(), version, err) + } + return finishSnapshot(dir) +} + +func finishSnapshot(dir string) (string, error) { + out, err := runGit(dir, "rev-parse", "HEAD") + if err != nil { + return "", err + } + if err := os.RemoveAll(filepath.Join(dir, ".git")); err != nil { + return "", err + } + return strings.TrimSpace(out), nil +} diff --git a/internal/skill/git_test.go b/internal/skill/git_test.go new file mode 100644 index 0000000..61337a6 --- /dev/null +++ b/internal/skill/git_test.go @@ -0,0 +1,130 @@ +package skill + +import ( + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const fixtureRepo = "test/skills" + +func TestResolveVersionLatestSelectsHighestStableSemver(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + gitCmd(t, dir, "tag", "v1.0.0") + gitCmd(t, dir, "tag", "v1.2.0") + gitCmd(t, dir, "tag", "v1.10.0") + gitCmd(t, dir, "tag", "v2.0.0-rc.1") + gitCmd(t, dir, "tag", "banana") + routeRepos(t, map[string]string{fixtureRepo: dir}) + + id, _ := ParseRepo(fixtureRepo) + for _, spec := range []string{"", "latest"} { + version, err := ResolveVersion(id, spec) + require.NoError(t, err, "ResolveVersion(%q)", spec) + assert.Equal(t, "v1.10.0", version, "ResolveVersion(%q)", spec) + } +} + +func TestResolveVersionLatestWithoutSemverTagsUsesHeadSHA(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + gitCmd(t, dir, "tag", "release-1") + routeRepos(t, map[string]string{fixtureRepo: dir}) + head := gitCmd(t, dir, "rev-parse", "HEAD") + + id, _ := ParseRepo(fixtureRepo) + version, err := ResolveVersion(id, "latest") + require.NoError(t, err) + assert.Equal(t, head, version, "latest should lock to the default branch HEAD") + assert.True(t, IsFullCommit(version), "version %q is not a full commit SHA", version) +} + +func TestResolveVersionBranchLocksToFullSHA(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + gitCmd(t, dir, "checkout", "-q", "-b", "dev") + writeSkillFixtureContent(t, dir, "v2", "a-skill") + gitCmd(t, dir, "add", "-A") + gitCmd(t, dir, "commit", "-q", "-m", "dev change") + devHead := gitCmd(t, dir, "rev-parse", "HEAD") + gitCmd(t, dir, "checkout", "-q", "main") + routeRepos(t, map[string]string{fixtureRepo: dir}) + + id, _ := ParseRepo(fixtureRepo) + version, err := ResolveVersion(id, "dev") + require.NoError(t, err) + assert.Equal(t, devHead, version) +} + +func TestResolveVersionShortCommitExpandsToFullSHA(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + routeRepos(t, map[string]string{fixtureRepo: dir}) + head := gitCmd(t, dir, "rev-parse", "HEAD") + + id, _ := ParseRepo(fixtureRepo) + version, err := ResolveVersion(id, head[:8]) + require.NoError(t, err) + assert.Equal(t, head, version) +} + +func TestResolveVersionExplicitTagKeptVerbatim(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + gitCmd(t, dir, "tag", "v2.0.0-rc.1") + routeRepos(t, map[string]string{fixtureRepo: dir}) + + id, _ := ParseRepo(fixtureRepo) + // A prerelease is selected only when requested explicitly. + version, err := ResolveVersion(id, "v2.0.0-rc.1") + require.NoError(t, err) + assert.Equal(t, "v2.0.0-rc.1", version) +} + +func TestResolveVersionFullSHAPassesThroughWithoutNetwork(t *testing.T) { + disableGit(t) + id, _ := ParseRepo(fixtureRepo) + sha := strings.Repeat("ab12", 10) + version, err := ResolveVersion(id, sha) + require.NoError(t, err) + assert.Equal(t, sha, version) +} + +func TestResolveVersionUnknownSpecFails(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + routeRepos(t, map[string]string{fixtureRepo: dir}) + + id, _ := ParseRepo(fixtureRepo) + _, err := ResolveVersion(id, "no-such-thing") + assert.Error(t, err, "unknown version spec should fail") +} + +func TestFetchSnapshotByTagAndBySHA(t *testing.T) { + dir := initSkillFixture(t, "a-skill") + gitCmd(t, dir, "tag", "v1.0.0") + routeRepos(t, map[string]string{fixtureRepo: dir}) + head := gitCmd(t, dir, "rev-parse", "HEAD") + id, _ := ParseRepo(fixtureRepo) + + for _, version := range []string{"v1.0.0", head} { + tmp := t.TempDir() + commit, err := fetchSnapshot(id, version, tmp) + require.NoError(t, err, "fetchSnapshot(%q)", version) + assert.Equal(t, head, commit, "fetchSnapshot(%q) commit", version) + assert.FileExists(t, filepath.Join(tmp, "skills", "a-skill", "SKILL.md")) + assert.NoDirExists(t, filepath.Join(tmp, ".git"), "snapshot still contains .git metadata") + } +} + +func TestGitErrorsAreCredentialFree(t *testing.T) { + orig := runGit + t.Cleanup(func() { runGit = orig }) + + // Simulate a git failure whose stderr leaks HTTPS userinfo, as the + // real runGit sanitizes stderr before wrapping it. + _, err := orig("", "ls-remote", "https://alice:hunter2@invalid.invalid/acme/skills.git") + if err == nil { + t.Skip("expected ls-remote against invalid host to fail") + } + assert.NotContains(t, err.Error(), "hunter2", "git error leaks credentials") + assert.NotContains(t, err.Error(), "alice:", "git error leaks credentials") +} diff --git a/internal/skill/helpers_test.go b/internal/skill/helpers_test.go new file mode 100644 index 0000000..1a648de --- /dev/null +++ b/internal/skill/helpers_test.go @@ -0,0 +1,84 @@ +package skill + +import ( + "errors" + "fmt" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +// gitCmd runs git in dir for building test fixtures. +func gitCmd(t *testing.T, dir string, args ...string) string { + t.Helper() + cmd := exec.Command("git", args...) + cmd.Dir = dir + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=test", "GIT_AUTHOR_EMAIL=test@example.com", + "GIT_COMMITTER_NAME=test", "GIT_COMMITTER_EMAIL=test@example.com", + ) + out, err := cmd.CombinedOutput() + require.NoError(t, err, "git %v failed:\n%s", args, out) + return strings.TrimSpace(string(out)) +} + +// initSkillFixture creates a Git repository containing a skills.yaml catalog +// with the given skill names and returns its directory. +func initSkillFixture(t *testing.T, skills ...string) string { + t.Helper() + dir := t.TempDir() + gitCmd(t, dir, "init", "-q", "-b", "main") + gitCmd(t, dir, "config", "uploadpack.allowAnySHA1InWant", "true") + writeSkillFixtureContent(t, dir, "v1", skills...) + gitCmd(t, dir, "add", "-A") + gitCmd(t, dir, "commit", "-q", "-m", "initial") + return dir +} + +// writeSkillFixtureContent (re)writes the catalog and skill directories. +func writeSkillFixtureContent(t *testing.T, dir, marker string, skills ...string) { + t.Helper() + var catalog strings.Builder + catalog.WriteString("skills:\n") + for _, name := range skills { + fmt.Fprintf(&catalog, " - name: %s\n description: Test skill %s\n path: skills/%s\n", name, name, name) + skillDir := filepath.Join(dir, "skills", name) + require.NoError(t, os.RemoveAll(skillDir)) + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + md := fmt.Sprintf("---\nname: %s\ndescription: Test skill %s\n---\nContent %s\n", name, name, marker) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, "SKILL.md"), []byte(md), 0o644)) + require.NoError(t, os.WriteFile(filepath.Join(skillDir, marker+".txt"), []byte(marker), 0o644)) + } + require.NoError(t, os.WriteFile(filepath.Join(dir, "skills.yaml"), []byte(catalog.String()), 0o644)) +} + +// routeRepos points the canonical HTTPS clone URLs of the given repositories +// at local fixture directories through Git insteadOf rules, and isolates the +// tests from the user's real Git configuration. +func routeRepos(t *testing.T, routes map[string]string) { + t.Helper() + var cfg strings.Builder + for repo, dir := range routes { + id, err := ParseRepo(repo) + require.NoError(t, err) + fmt.Fprintf(&cfg, "[url \"%s\"]\n\tinsteadOf = %s\n", dir, id.CloneURL()) + } + cfgPath := filepath.Join(t.TempDir(), "gitconfig") + require.NoError(t, os.WriteFile(cfgPath, []byte(cfg.String()), 0o644)) + t.Setenv("GIT_CONFIG_GLOBAL", cfgPath) + t.Setenv("GIT_CONFIG_SYSTEM", os.DevNull) +} + +// disableGit makes every Git invocation fail, proving an operation is offline. +func disableGit(t *testing.T) { + t.Helper() + orig := runGit + runGit = func(dir string, args ...string) (string, error) { + return "", errors.New("network disabled by test") + } + t.Cleanup(func() { runGit = orig }) +} diff --git a/internal/skill/repo.go b/internal/skill/repo.go index 496ba92..9af9435 100644 --- a/internal/skill/repo.go +++ b/internal/skill/repo.go @@ -44,6 +44,7 @@ func (id RepoID) IsZero() bool { var ( scpLikeRe = regexp.MustCompile(`^([A-Za-z0-9._-]+)@([A-Za-z0-9._-]+):(.+)$`) fullCommitRe = regexp.MustCompile(`^[0-9a-f]{40}([0-9a-f]{24})?$`) + shortHexRe = regexp.MustCompile(`^[0-9a-f]{4,39}$`) userinfoRe = regexp.MustCompile(`(?i)([a-z][a-z0-9+.-]*://)[^/@\s]+@`) ) @@ -149,6 +150,10 @@ func IsFullCommit(v string) bool { return fullCommitRe.MatchString(v) } +func looksLikeShortCommit(v string) bool { + return shortHexRe.MatchString(v) +} + // SanitizeCredentials removes URL userinfo (user:password@, token@) from s so // errors and output never disclose credentials. func SanitizeCredentials(s string) string {