Merge pull request #13236 from cli/sammorrowdrums/skill-install-upstream-provenance
This commit is contained in:
Sam Morrow
GitHub
commit
92e812b749
2 changed files with 352 additions and 1 deletions
|
|
@ -1,6 +1,7 @@
|
|||
package install
|
||||
|
||||
import (
|
||||
"encoding/base64"
|
||||
"errors"
|
||||
"fmt"
|
||||
"io"
|
||||
|
|
@ -57,6 +58,7 @@ type InstallOptions struct {
|
|||
Force bool
|
||||
FromLocal bool // treat SkillSource as a local directory path
|
||||
AllowHiddenDirs bool // include skills in dot-prefixed directories
|
||||
Upstream bool // install from upstream when re-published skill detected
|
||||
|
||||
repo ghrepo.Interface // set when SkillSource is a GitHub repository
|
||||
localPath string // set when FromLocal is true
|
||||
|
|
@ -193,6 +195,10 @@ func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru
|
|||
return err
|
||||
}
|
||||
|
||||
if err := cmdutil.MutuallyExclusive("--from-local and --upstream cannot be used together", opts.FromLocal, opts.Upstream); err != nil {
|
||||
return err
|
||||
}
|
||||
|
||||
if opts.Pin != "" && opts.SkillName != "" && strings.Contains(opts.SkillName, "@") {
|
||||
return cmdutil.FlagErrorf("cannot use --pin with an inline @version in the skill name")
|
||||
}
|
||||
|
|
@ -212,6 +218,7 @@ func NewCmdInstall(f *cmdutil.Factory, telemetry ghtelemetry.CommandRecorder, ru
|
|||
cmd.Flags().BoolVarP(&opts.Force, "force", "f", false, "Overwrite existing skills without prompting")
|
||||
cmd.Flags().BoolVar(&opts.FromLocal, "from-local", false, "Treat the argument as a local directory path instead of a repository")
|
||||
cmd.Flags().BoolVar(&opts.AllowHiddenDirs, "allow-hidden-dirs", false, "Include skills in hidden directories (e.g. .claude/skills/, .agents/skills/)")
|
||||
cmd.Flags().BoolVar(&opts.Upstream, "upstream", false, "Install from the upstream source when a re-published skill is detected")
|
||||
cmdutil.DisableAuthCheckFlag(cmd.Flags().Lookup("from-local"))
|
||||
|
||||
return cmd
|
||||
|
|
@ -248,13 +255,17 @@ func installRun(opts *InstallOptions) error {
|
|||
// Kick off the visibility fetch in parallel with the install work so
|
||||
// the extra API roundtrip doesn't add latency on the critical path.
|
||||
// The result is consumed when the telemetry event is emitted below.
|
||||
// Capture repo fields now to avoid a data race if opts.repo is
|
||||
// swapped during an upstream redirect.
|
||||
type visResult struct {
|
||||
vis discovery.RepoVisibility
|
||||
err error
|
||||
}
|
||||
visCh := make(chan visResult, 1)
|
||||
visOwner := opts.repo.RepoOwner()
|
||||
visRepo := opts.repo.RepoName()
|
||||
go func() {
|
||||
vis, err := discovery.FetchRepoVisibility(apiClient, hostname, opts.repo.RepoOwner(), opts.repo.RepoName())
|
||||
vis, err := discovery.FetchRepoVisibility(apiClient, hostname, visOwner, visRepo)
|
||||
visCh <- visResult{vis: vis, err: err}
|
||||
}()
|
||||
|
||||
|
|
@ -293,6 +304,43 @@ func installRun(opts *InstallOptions) error {
|
|||
}
|
||||
}
|
||||
|
||||
// Track upstream provenance detection result for telemetry.
|
||||
upstreamSource := "none"
|
||||
|
||||
// Check if the selected skill was re-published from an upstream source.
|
||||
// The re-publisher's SKILL.md will have github-repo metadata pointing
|
||||
// to the original source repo. If detected, offer to install directly
|
||||
// from upstream instead.
|
||||
if len(selectedSkills) == 1 && selectedSkills[0].BlobSHA != "" {
|
||||
upstreamRepo, detected, err := checkUpstreamProvenance(opts, apiClient, hostname, selectedSkills[0], resolved.SHA)
|
||||
if err != nil {
|
||||
return err
|
||||
}
|
||||
if upstreamRepo != nil {
|
||||
redirectDims := map[string]string{}
|
||||
select {
|
||||
case r := <-visCh:
|
||||
if r.err == nil && r.vis == discovery.RepoVisibilityPublic {
|
||||
redirectDims["from_owner"] = visOwner
|
||||
redirectDims["from_repo"] = visRepo
|
||||
}
|
||||
case <-time.After(visibilityWaitTimeout):
|
||||
}
|
||||
opts.Telemetry.Record(ghtelemetry.Event{
|
||||
Type: "skill_upstream_redirect",
|
||||
Dimensions: redirectDims,
|
||||
})
|
||||
opts.repo = upstreamRepo
|
||||
opts.SkillSource = ghrepo.FullName(upstreamRepo)
|
||||
opts.version = ""
|
||||
opts.Pin = ""
|
||||
return installRun(opts)
|
||||
}
|
||||
if detected {
|
||||
upstreamSource = "republisher"
|
||||
}
|
||||
}
|
||||
|
||||
printPreInstallDisclaimer(opts.IO.ErrOut, cs)
|
||||
|
||||
selectedHosts, err := resolveHosts(opts, canPrompt)
|
||||
|
|
@ -355,6 +403,7 @@ func installRun(opts *InstallOptions) error {
|
|||
dims := map[string]string{
|
||||
"agent_hosts": mapAgentHostsToIDs(selectedHosts),
|
||||
"skill_host_type": ghinstance.CategorizeHost(opts.repo.RepoHost()),
|
||||
"upstream_source": upstreamSource,
|
||||
}
|
||||
select {
|
||||
case r := <-visCh:
|
||||
|
|
@ -1169,3 +1218,85 @@ func filterHiddenDirSkills(opts *InstallOptions, allSkills []discovery.Skill) ([
|
|||
|
||||
return standard, nil
|
||||
}
|
||||
|
||||
// checkUpstreamProvenance fetches the skill's SKILL.md via the contents API
|
||||
// to check if it contains github-repo metadata pointing to a different
|
||||
// repository, indicating the skill was re-published from an upstream source.
|
||||
// In interactive mode, the user is asked whether to install from the
|
||||
// re-publisher or redirect to the upstream. Non-interactive mode always
|
||||
// installs from the re-publisher.
|
||||
// Returns (repo to redirect to, whether upstream was detected, error).
|
||||
func checkUpstreamProvenance(opts *InstallOptions, client *api.Client, hostname string, skill discovery.Skill, commitSHA string) (ghrepo.Interface, bool, error) {
|
||||
apiPath := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s",
|
||||
opts.repo.RepoOwner(), opts.repo.RepoName(),
|
||||
skill.Path+"/SKILL.md", commitSHA)
|
||||
var fileResp struct {
|
||||
Content string `json:"content"`
|
||||
Encoding string `json:"encoding"`
|
||||
}
|
||||
if err := client.REST(hostname, "GET", apiPath, nil, &fileResp); err != nil {
|
||||
return nil, false, nil //nolint:nilerr // best-effort check; failing to fetch is not fatal
|
||||
}
|
||||
if fileResp.Encoding != "base64" {
|
||||
return nil, false, nil
|
||||
}
|
||||
decoded, decodeErr := io.ReadAll(base64.NewDecoder(base64.StdEncoding, strings.NewReader(fileResp.Content)))
|
||||
if decodeErr != nil {
|
||||
return nil, false, nil //nolint:nilerr // best-effort; decode failure is not fatal
|
||||
}
|
||||
content := string(decoded)
|
||||
|
||||
result, parseErr := frontmatter.Parse(content)
|
||||
if parseErr != nil || result.Metadata.Meta == nil {
|
||||
//nolint:nilerr // unparseable frontmatter means no upstream to detect
|
||||
return nil, false, nil
|
||||
}
|
||||
|
||||
existingRepo, _ := result.Metadata.Meta["github-repo"].(string)
|
||||
if existingRepo == "" {
|
||||
return nil, false, nil
|
||||
}
|
||||
|
||||
currentRepoURL := source.BuildRepoURL(hostname, opts.repo.RepoOwner(), opts.repo.RepoName())
|
||||
if existingRepo == currentRepoURL {
|
||||
return nil, false, nil
|
||||
}
|
||||
|
||||
upstreamRepo, parseErr := source.ParseRepoURL(existingRepo)
|
||||
if parseErr != nil {
|
||||
//nolint:nilerr // invalid repo URL means we can't redirect; install normally
|
||||
return nil, false, nil
|
||||
}
|
||||
|
||||
cs := opts.IO.ColorScheme()
|
||||
upstreamLabel := ghrepo.FullName(upstreamRepo)
|
||||
repoSource := ghrepo.FullName(opts.repo)
|
||||
|
||||
fmt.Fprintf(opts.IO.ErrOut, "%s This skill was originally published in %s\n", cs.WarningIcon(), upstreamLabel)
|
||||
|
||||
if opts.Upstream {
|
||||
fmt.Fprintf(opts.IO.ErrOut, "Redirecting install to %s...\n", upstreamLabel)
|
||||
return upstreamRepo, true, nil
|
||||
}
|
||||
|
||||
if !opts.IO.CanPrompt() {
|
||||
fmt.Fprintf(opts.IO.ErrOut, " Installing from %s (use --upstream or interactive mode to choose upstream)\n", repoSource)
|
||||
return nil, true, nil
|
||||
}
|
||||
|
||||
choices := []string{
|
||||
fmt.Sprintf("%s (re-publisher, recommended)", repoSource),
|
||||
fmt.Sprintf("%s (upstream)", upstreamLabel),
|
||||
}
|
||||
idx, err := opts.Prompter.Select("Install from:", "", choices)
|
||||
if err != nil {
|
||||
return nil, true, err
|
||||
}
|
||||
|
||||
if idx == 1 {
|
||||
fmt.Fprintf(opts.IO.ErrOut, "Redirecting install to %s...\n", upstreamLabel)
|
||||
return upstreamRepo, true, nil
|
||||
}
|
||||
|
||||
return nil, true, nil
|
||||
}
|
||||
|
|
|
|||
|
|
@ -125,6 +125,16 @@ func TestNewCmdInstall(t *testing.T) {
|
|||
cli: "monalisa/skills-repo --allow-hidden-dirs",
|
||||
wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", Scope: "project", AllowHiddenDirs: true},
|
||||
},
|
||||
{
|
||||
name: "upstream flag",
|
||||
cli: "monalisa/skills-repo git-commit --upstream",
|
||||
wantOpts: InstallOptions{SkillSource: "monalisa/skills-repo", SkillName: "git-commit", Scope: "project", Upstream: true},
|
||||
},
|
||||
{
|
||||
name: "from-local with --upstream is mutually exclusive",
|
||||
cli: "--from-local ./local-dir --upstream",
|
||||
wantErr: true,
|
||||
},
|
||||
}
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
|
|
@ -2487,3 +2497,213 @@ func TestInstallRun_TelemetryMultipleSkills(t *testing.T) {
|
|||
assert.Contains(t, names, "code-review")
|
||||
assert.Contains(t, names, "git-commit")
|
||||
}
|
||||
|
||||
var republishedContent = heredoc.Doc(`
|
||||
---
|
||||
name: git-commit
|
||||
description: Writes commits
|
||||
metadata:
|
||||
github-repo: https://github.com/monalisa/original-skills
|
||||
github-tree-sha: upstreamTreeSHA
|
||||
github-path: skills/git-commit
|
||||
---
|
||||
# Git Commit
|
||||
`)
|
||||
|
||||
func stubContentsAPI(reg *httpmock.Registry, owner, repo, path, content string) {
|
||||
encoded := base64.StdEncoding.EncodeToString([]byte(content))
|
||||
reg.Register(
|
||||
httpmock.REST("GET", fmt.Sprintf("repos/%s/%s/contents/%s", owner, repo, path)),
|
||||
httpmock.StringResponse(fmt.Sprintf(`{"content": %q, "encoding": "base64"}`, encoded)),
|
||||
)
|
||||
}
|
||||
|
||||
func TestInstallRun_UpstreamDetection(t *testing.T) {
|
||||
tests := []struct {
|
||||
name string
|
||||
isTTY bool
|
||||
stubs func(*httpmock.Registry)
|
||||
opts func(t *testing.T, ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions
|
||||
wantErr string
|
||||
wantStdout string
|
||||
wantStderr string
|
||||
}{
|
||||
{
|
||||
name: "detects re-published skill and user picks re-publisher",
|
||||
isTTY: true,
|
||||
stubs: func(reg *httpmock.Registry) {
|
||||
stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123")
|
||||
stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123",
|
||||
singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA"))
|
||||
stubContentsAPI(reg, "monalisa", "skills-repo",
|
||||
"skills/git-commit/SKILL.md", republishedContent)
|
||||
stubInstallFiles(reg, "monalisa", "skills-repo",
|
||||
"treeSHA", "blobSHA", republishedContent)
|
||||
},
|
||||
opts: func(t *testing.T, ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions {
|
||||
return &InstallOptions{
|
||||
IO: ios,
|
||||
HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil },
|
||||
GitClient: &git.Client{RepoDir: t.TempDir()},
|
||||
Prompter: &prompter.PrompterMock{
|
||||
SelectFunc: func(_ string, _ string, choices []string) (int, error) {
|
||||
require.Len(t, choices, 2)
|
||||
assert.Contains(t, choices[0], "monalisa/skills-repo")
|
||||
assert.Contains(t, choices[1], "monalisa/original-skills")
|
||||
return 0, nil
|
||||
},
|
||||
},
|
||||
Telemetry: &telemetry.NoOpService{},
|
||||
SkillSource: "monalisa/skills-repo",
|
||||
SkillName: "git-commit",
|
||||
Agent: "github-copilot",
|
||||
Scope: "project",
|
||||
ScopeChanged: true,
|
||||
Dir: t.TempDir(),
|
||||
}
|
||||
},
|
||||
wantStderr: "originally published in monalisa/original-skills",
|
||||
wantStdout: "Installed git-commit",
|
||||
},
|
||||
{
|
||||
name: "detects re-published skill and user picks upstream",
|
||||
isTTY: true,
|
||||
stubs: func(reg *httpmock.Registry) {
|
||||
stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123")
|
||||
stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123",
|
||||
singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA"))
|
||||
stubContentsAPI(reg, "monalisa", "skills-repo",
|
||||
"skills/git-commit/SKILL.md", republishedContent)
|
||||
stubResolveVersion(reg, "monalisa", "original-skills", "v2.0.0", "upstream456")
|
||||
stubDiscoverTree(reg, "monalisa", "original-skills", "upstream456",
|
||||
singleSkillTreeJSON("git-commit", "upTreeSHA", "upBlobSHA"))
|
||||
stubContentsAPI(reg, "monalisa", "original-skills",
|
||||
"skills/git-commit/SKILL.md", gitCommitContent)
|
||||
stubInstallFiles(reg, "monalisa", "original-skills",
|
||||
"upTreeSHA", "upBlobSHA", gitCommitContent)
|
||||
},
|
||||
opts: func(t *testing.T, ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions {
|
||||
return &InstallOptions{
|
||||
IO: ios,
|
||||
HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil },
|
||||
GitClient: &git.Client{RepoDir: t.TempDir()},
|
||||
Prompter: &prompter.PrompterMock{
|
||||
SelectFunc: func(_ string, _ string, choices []string) (int, error) {
|
||||
require.Len(t, choices, 2)
|
||||
assert.Contains(t, choices[0], "monalisa/skills-repo")
|
||||
assert.Contains(t, choices[1], "monalisa/original-skills")
|
||||
return 1, nil
|
||||
},
|
||||
},
|
||||
Telemetry: &telemetry.NoOpService{},
|
||||
SkillSource: "monalisa/skills-repo",
|
||||
SkillName: "git-commit",
|
||||
Agent: "github-copilot",
|
||||
Scope: "project",
|
||||
ScopeChanged: true,
|
||||
Dir: t.TempDir(),
|
||||
}
|
||||
},
|
||||
wantStderr: "Redirecting install to monalisa/original-skills",
|
||||
wantStdout: "Installed git-commit",
|
||||
},
|
||||
{
|
||||
name: "non-interactive defaults to re-publisher with notice",
|
||||
isTTY: false,
|
||||
stubs: func(reg *httpmock.Registry) {
|
||||
stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123")
|
||||
stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123",
|
||||
singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA"))
|
||||
stubContentsAPI(reg, "monalisa", "skills-repo",
|
||||
"skills/git-commit/SKILL.md", republishedContent)
|
||||
stubInstallFiles(reg, "monalisa", "skills-repo",
|
||||
"treeSHA", "blobSHA", republishedContent)
|
||||
},
|
||||
opts: func(t *testing.T, ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions {
|
||||
return &InstallOptions{
|
||||
IO: ios,
|
||||
HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil },
|
||||
GitClient: &git.Client{RepoDir: t.TempDir()},
|
||||
Telemetry: &telemetry.NoOpService{},
|
||||
SkillSource: "monalisa/skills-repo",
|
||||
SkillName: "git-commit",
|
||||
Agent: "github-copilot",
|
||||
Scope: "project",
|
||||
ScopeChanged: true,
|
||||
Dir: t.TempDir(),
|
||||
}
|
||||
},
|
||||
wantStderr: "use --upstream",
|
||||
wantStdout: "Installed git-commit",
|
||||
},
|
||||
{
|
||||
name: "non-interactive with --upstream redirects to upstream",
|
||||
isTTY: false,
|
||||
stubs: func(reg *httpmock.Registry) {
|
||||
stubResolveVersion(reg, "monalisa", "skills-repo", "v1.0.0", "abc123")
|
||||
stubDiscoverTree(reg, "monalisa", "skills-repo", "abc123",
|
||||
singleSkillTreeJSON("git-commit", "treeSHA", "blobSHA"))
|
||||
stubContentsAPI(reg, "monalisa", "skills-repo",
|
||||
"skills/git-commit/SKILL.md", republishedContent)
|
||||
stubResolveVersion(reg, "monalisa", "original-skills", "v2.0.0", "upstream456")
|
||||
stubDiscoverTree(reg, "monalisa", "original-skills", "upstream456",
|
||||
singleSkillTreeJSON("git-commit", "upTreeSHA", "upBlobSHA"))
|
||||
stubContentsAPI(reg, "monalisa", "original-skills",
|
||||
"skills/git-commit/SKILL.md", gitCommitContent)
|
||||
stubInstallFiles(reg, "monalisa", "original-skills",
|
||||
"upTreeSHA", "upBlobSHA", gitCommitContent)
|
||||
},
|
||||
opts: func(t *testing.T, ios *iostreams.IOStreams, reg *httpmock.Registry) *InstallOptions {
|
||||
return &InstallOptions{
|
||||
IO: ios,
|
||||
HttpClient: func() (*http.Client, error) { return &http.Client{Transport: reg}, nil },
|
||||
GitClient: &git.Client{RepoDir: t.TempDir()},
|
||||
Telemetry: &telemetry.NoOpService{},
|
||||
SkillSource: "monalisa/skills-repo",
|
||||
SkillName: "git-commit",
|
||||
Agent: "github-copilot",
|
||||
Scope: "project",
|
||||
ScopeChanged: true,
|
||||
Dir: t.TempDir(),
|
||||
Upstream: true,
|
||||
}
|
||||
},
|
||||
wantStderr: "Redirecting install to monalisa/original-skills",
|
||||
wantStdout: "Installed git-commit",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tt := range tests {
|
||||
t.Run(tt.name, func(t *testing.T) {
|
||||
reg := &httpmock.Registry{}
|
||||
defer reg.Verify(t)
|
||||
tt.stubs(reg)
|
||||
|
||||
homeDir := t.TempDir()
|
||||
t.Setenv("HOME", homeDir)
|
||||
t.Setenv("USERPROFILE", homeDir)
|
||||
|
||||
ios, _, stdout, stderr := iostreams.Test()
|
||||
ios.SetStdoutTTY(tt.isTTY)
|
||||
ios.SetStdinTTY(tt.isTTY)
|
||||
ios.SetStderrTTY(tt.isTTY)
|
||||
opts := tt.opts(t, ios, reg)
|
||||
|
||||
err := installRun(opts)
|
||||
|
||||
if tt.wantErr != "" {
|
||||
require.Error(t, err)
|
||||
assert.Contains(t, err.Error(), tt.wantErr)
|
||||
return
|
||||
}
|
||||
|
||||
require.NoError(t, err)
|
||||
if tt.wantStdout != "" {
|
||||
assert.Contains(t, stdout.String(), tt.wantStdout)
|
||||
}
|
||||
if tt.wantStderr != "" {
|
||||
assert.Contains(t, stderr.String(), tt.wantStderr)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue