diff --git a/pkg/skills/skillsvc/install_oci.go b/pkg/skills/skillsvc/install_oci.go index fb7249be6b..f77be955df 100644 --- a/pkg/skills/skillsvc/install_oci.go +++ b/pkg/skills/skillsvc/install_oci.go @@ -13,8 +13,10 @@ import ( "time" nameref "github.com/google/go-containerregistry/pkg/name" + "github.com/opencontainers/go-digest" "github.com/stacklok/toolhive-core/httperr" + ociskills "github.com/stacklok/toolhive-core/oci/skills" "github.com/stacklok/toolhive/pkg/skills" ) @@ -113,15 +115,38 @@ func (s *service) installFromOCI( return s.installWithExtraction(ctx, opts, scope) } -// resolveFromLocalStore attempts to resolve a skill name as a tag in the local -// OCI store. On success it hydrates opts with layer data, digest, and version -// from the artifact. Returns (true, nil) when resolved, (false, nil) when the -// tag is not found, or (false, err) on validation/extraction failure. +// resolveFromLocalStore attempts to resolve a skill name against the local +// OCI store. It first tries a direct tag lookup (the common "build then +// install by skill name" case). When that misses, or when the direct match's +// declared version does not satisfy opts.Version, it falls back to scanning +// local-build-marked tags for one whose declared skill name matches opts.Name +// (so users can install a build that was tagged with something other than +// the skill name, e.g. `--tag v0.0.1`). Returns (true, nil) when resolved +// (opts is hydrated with layer data, digest, reference, and version), +// (false, nil) when no match is found, or (false, err) on validation, +// extraction, or ambiguity failures. func (s *service) resolveFromLocalStore(ctx context.Context, opts *skills.InstallOptions) (bool, error) { + resolved, err := s.tryDirectLocalTag(ctx, opts) + if err != nil || resolved { + return resolved, err + } + return s.tryLocalBuildScan(ctx, opts) +} + +// tryDirectLocalTag attempts to resolve opts.Name as a literal tag in the +// local OCI store and verifies the artifact's declared skill name matches. +// Returns (true, nil) when the tag resolves and both name and (if specified) +// version match. Returns (false, err) on supply-chain mismatch or extraction +// failure. Returns (false, nil) when no such tag exists OR when name matches +// but a caller-supplied version does not — letting the caller scan for a +// better match instead of erroring. +func (s *service) tryDirectLocalTag( + ctx context.Context, + opts *skills.InstallOptions, +) (bool, error) { d, err := s.ociStore.Resolve(ctx, opts.Name) if err != nil { // Tag not found in the local store — not an error, just unresolved. - slog.Debug("skill name not found in local OCI store", "name", opts.Name, "error", err) return false, nil } @@ -150,14 +175,179 @@ func (s *service) resolveFromLocalStore(ctx context.Context, opts *skills.Instal ) } + // Version constraint: when the caller specifies a version, the direct + // match must agree. If not, fall through to the scan so a sibling build + // with the requested version can win. + if opts.Version != "" && skillConfig.Version != opts.Version { + return false, nil + } + + hydrateOptsFromLocalBuild(opts, layerData, d, skillConfig, opts.Name) + return true, nil +} + +// tryLocalBuildScan walks local-build-marked tags for a skill artifact whose +// declared name (and version, when specified) matches opts. With one match +// it hydrates opts; with multiple, it applies the tag-equals-name tie-breaker +// before surfacing a 409 ambiguity error. +func (s *service) tryLocalBuildScan( + ctx context.Context, + opts *skills.InstallOptions, +) (bool, error) { + matches, err := s.findLocalBuildsByName(ctx, opts.Name, opts.Version) + if err != nil { + return false, err + } + if len(matches) == 0 { + slog.Debug("skill name not found in local OCI store", + "name", opts.Name, "version", opts.Version) + return false, nil + } + + chosen := pickLocalBuildMatch(matches, opts.Name) + if chosen == nil { + return false, ambiguousLocalBuildError(opts.Name, matches) + } + hydrateOptsFromLocalBuild(opts, chosen.LayerData, chosen.Digest, chosen.Config, chosen.Tag) + return true, nil +} + +// localBuildMatch is a candidate match found by scanning local-build-marked +// tags for a skill with a given name (and optional version). +type localBuildMatch struct { + Tag string + Digest digest.Digest + LayerData []byte + Config *ociskills.SkillConfig +} + +// findLocalBuildsByName scans local-build-marked tags for skill artifacts +// whose declared name matches `name`. When version is non-empty, it also +// filters by the artifact's declared version. Tags without the local-build +// marker (e.g. install/content caches) and non-skill artifacts are skipped, +// mirroring ListBuilds. Per-tag inspection errors are logged and the tag is +// skipped, so a single corrupt entry does not poison the whole lookup. +func (s *service) findLocalBuildsByName( + ctx context.Context, + name, version string, +) ([]localBuildMatch, error) { + tags, err := s.ociStore.ListTags(ctx) + if err != nil { + return nil, fmt.Errorf("listing local OCI tags: %w", err) + } + + var matches []localBuildMatch + for _, tag := range tags { + local, markerErr := isLocalBuild(ctx, s.ociStore, tag) + if markerErr != nil { + slog.Debug("failed to read local-build marker", "tag", tag, "error", markerErr) + continue + } + if !local { + continue + } + + d, resolveErr := s.ociStore.Resolve(ctx, tag) + if resolveErr != nil { + slog.Debug("failed to resolve local-build tag", "tag", tag, "error", resolveErr) + continue + } + + isSkill, typeErr := s.isSkillArtifact(ctx, d) + if typeErr != nil { + slog.Debug("failed to check artifact type", "tag", tag, "error", typeErr) + continue + } + if !isSkill { + continue + } + + layerData, cfg, extractErr := s.extractOCIContent(ctx, d) + if extractErr != nil { + slog.Debug("failed to extract local-build content", "tag", tag, "error", extractErr) + continue + } + if cfg == nil || cfg.Name != name { + continue + } + // Match `version` against either the artifact's declared version + // (cfg.Version, populated from SKILL.md frontmatter) or the OCI tag + // in the local store. SKILL.md frontmatter often omits the version + // while the build tag carries it (e.g. `thv skill build --tag v0.0.1`), + // and callers seeing `{tag: "v0.0.1"}` in GET /skills/builds naturally + // pass that string as `version`. This also matches the OCI-name path, + // which already treats `version` as the tag (see the splice in + // install.go). + if version != "" && cfg.Version != version && tag != version { + continue + } + + matches = append(matches, localBuildMatch{ + Tag: tag, + Digest: d, + LayerData: layerData, + Config: cfg, + }) + } + return matches, nil +} + +// pickLocalBuildMatch selects a single match among scan results. With one +// match it returns it directly; with multiple, it prefers the match whose +// tag exactly equals name (the "tagged with skill name" case). When more +// than one match remains and none has Tag == name, returns nil to signal +// the caller to surface an ambiguity error. +func pickLocalBuildMatch(matches []localBuildMatch, name string) *localBuildMatch { + if len(matches) == 1 { + return &matches[0] + } + for i := range matches { + if matches[i].Tag == name { + return &matches[i] + } + } + return nil +} + +// ambiguousLocalBuildError builds a 409 error listing each ambiguous match's +// tag and version so the caller knows which builds collide. Returned only +// when the tag-equals-name tie-breaker fails to resolve a unique winner. +func ambiguousLocalBuildError(name string, matches []localBuildMatch) error { + lines := make([]string, 0, len(matches)) + for _, m := range matches { + var version string + if m.Config != nil { + version = m.Config.Version + } + lines = append(lines, fmt.Sprintf(" - tag %q, version %q", m.Tag, version)) + } + return httperr.WithCode( + fmt.Errorf( + "multiple local builds match skill %q; specify version to disambiguate. Candidates:\n%s", + name, strings.Join(lines, "\n"), + ), + http.StatusConflict, + ) +} + +// hydrateOptsFromLocalBuild populates opts.LayerData, Digest, Reference, and +// Version from a resolved local-build artifact. Reference defaults to the tag +// in the local store so subsequent operations (e.g. push) can re-resolve it. +// A caller-supplied Version takes precedence over the artifact's declared +// version; a caller-supplied Reference is preserved. +func hydrateOptsFromLocalBuild( + opts *skills.InstallOptions, + layerData []byte, + d digest.Digest, + cfg *ociskills.SkillConfig, + tag string, +) { opts.LayerData = layerData opts.Digest = d.String() if opts.Reference == "" { - opts.Reference = opts.Name + opts.Reference = tag } - if opts.Version == "" && skillConfig.Version != "" { - opts.Version = skillConfig.Version + if opts.Version == "" && cfg != nil && cfg.Version != "" { + opts.Version = cfg.Version } - - return true, nil } diff --git a/pkg/skills/skillsvc/install_oci_test.go b/pkg/skills/skillsvc/install_oci_test.go index ee29ede8d8..db4e1daa58 100644 --- a/pkg/skills/skillsvc/install_oci_test.go +++ b/pkg/skills/skillsvc/install_oci_test.go @@ -466,6 +466,216 @@ func TestInstallFromLocalStore(t *testing.T) { }, wantErr: "checking OCI content type", }, + { + // Mirrors the original GH issue: a build tagged with an OCI tag + // (e.g. `--tag v1.0.0`) must still be installable by skill name. + name: "scan resolves by skill name when tag differs", + opts: skills.InstallOptions{Name: "my-skill", Clients: []string{"claude-code"}}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + // Build "my-skill" but tag it as "v1.0.0" (typical + // `thv skill build --tag v1.0.0` flow). + indexDigest := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, indexDigest, "v1.0.0")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + targetDir := filepath.Join(tempDir(t), "installed", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) + store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.Equal(t, "my-skill", sk.Metadata.Name) + assert.Equal(t, "1.0.0", sk.Metadata.Version) + // Reference defaults to the local store tag so push + // can re-resolve the artifact later. + assert.Equal(t, "v1.0.0", sk.Reference) + assert.Contains(t, sk.Digest, "sha256:") + assert.Equal(t, skills.InstallStatusInstalled, sk.Status) + return nil + }) + return ociStore, store, pr + }, + wantStatus: string(skills.InstallStatusInstalled), + wantVersion: "1.0.0", + wantDigest: true, + }, + { + // User-facing reproducer: GET /skills/builds shows + // `{tag: "v0.0.1", name: ""}` (cfg.Version may be empty + // or differ from the tag), and a caller passing the displayed + // `tag` as `version` must still resolve. `version` is matched + // against either cfg.Version OR the local store tag, mirroring + // the OCI-name path that already treats `version` as the tag. + name: "scan version matches local store tag", + opts: skills.InstallOptions{Name: "my-skill", Version: "v1.0.0", Clients: []string{"claude-code"}}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + // Artifact's cfg.Version is "1.0.0" (no leading 'v'); the + // caller passes the tag "v1.0.0" as `version`. + d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "v1.0.0")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + targetDir := filepath.Join(tempDir(t), "installed", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) + store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + // Reference defaults to the resolved tag. + assert.Equal(t, "v1.0.0", sk.Reference) + // cfg.Version (1.0.0) hydrates Metadata.Version + // because the caller-supplied version was matched + // via the tag, not via cfg.Version. + assert.Equal(t, "v1.0.0", sk.Metadata.Version) + return nil + }) + return ociStore, store, pr + }, + wantStatus: string(skills.InstallStatusInstalled), + }, + { + // Two local builds share a skill name but differ in version. + // A caller-supplied version narrows the scan to one match. + name: "scan version filter selects matching build", + opts: skills.InstallOptions{Name: "my-skill", Version: "2.0.0", Clients: []string{"claude-code"}}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + d1 := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d1, "v1")) + d2 := buildTestArtifact(t, ociStore, "my-skill", "2.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d2, "v2")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + targetDir := filepath.Join(tempDir(t), "installed", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) + store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.Equal(t, "2.0.0", sk.Metadata.Version) + assert.Equal(t, "v2", sk.Reference) + return nil + }) + return ociStore, store, pr + }, + wantStatus: string(skills.InstallStatusInstalled), + wantVersion: "2.0.0", + }, + { + // Two builds with the same skill name and same version (only + // differ by tag), and no version specified: the scan is + // genuinely ambiguous and must surface a 409 listing both. + name: "scan ambiguous matches return 409 with candidate list", + opts: skills.InstallOptions{Name: "my-skill"}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + d1 := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d1, "alpha")) + d2 := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d2, "beta")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + return ociStore, store, pr + }, + wantCode: http.StatusConflict, + wantErr: "multiple local builds match", + }, + { + // A caller-supplied version that no local build satisfies must + // fall through to the registry lookup (which returns 404 here + // since no registry is configured) rather than picking a + // non-matching build or returning the ambiguity error. + name: "scan version mismatch falls through to registry", + opts: skills.InstallOptions{Name: "my-skill", Version: "9.9.9"}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "v1")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + return ociStore, store, pr + }, + wantCode: http.StatusNotFound, + wantErr: "not found in local store or registry", + }, + { + // A direct tag match whose artifact name agrees with opts.Name + // but whose version disagrees must fall through to the scan, + // which can then locate a sibling build with the right version. + name: "direct version mismatch falls through to scan", + opts: skills.InstallOptions{Name: "my-skill", Version: "2.0.0", Clients: []string{"claude-code"}}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + // Canonical tag at version 1.0.0. + d1 := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d1, "my-skill")) + // Sibling build at version 2.0.0. + d2 := buildTestArtifact(t, ociStore, "my-skill", "2.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d2, "next")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + targetDir := filepath.Join(tempDir(t), "installed", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) + store.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, "").Return(skills.InstalledSkill{}, storage.ErrNotFound) + store.EXPECT().Create(gomock.Any(), gomock.Any()).DoAndReturn( + func(_ context.Context, sk skills.InstalledSkill) error { + assert.Equal(t, "2.0.0", sk.Metadata.Version) + assert.Equal(t, "next", sk.Reference) + return nil + }) + return ociStore, store, pr + }, + wantStatus: string(skills.InstallStatusInstalled), + wantVersion: "2.0.0", + }, + { + // A tag carrying the local-build marker on a non-skill artifact + // (defensive case — Build never produces this, but the index + // file is user-editable) must be ignored by the scan. + name: "scan ignores non-skill local-build-marked tags", + opts: skills.InstallOptions{Name: "my-skill"}, + setup: func(t *testing.T, ctrl *gomock.Controller) (*ociskills.Store, *storemocks.MockSkillStore, *skillsmocks.MockPathResolver) { + t.Helper() + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + // Local-build-marked tag pointing at an index whose + // ArtifactType is not the skill type. + otherIndex := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.index.v1+json","artifactType":"application/vnd.docker.distribution.manifest.v2","manifests":[]}` + d, putErr := ociStore.PutManifest(t.Context(), []byte(otherIndex)) + require.NoError(t, putErr) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "non-skill-tag")) + + store := storemocks.NewMockSkillStore(ctrl) + pr := skillsmocks.NewMockPathResolver(ctrl) + return ociStore, store, pr + }, + wantCode: http.StatusNotFound, + wantErr: "not found in local store or registry", + }, } for _, tt := range tests { @@ -488,6 +698,9 @@ func TestInstallFromLocalStore(t *testing.T) { if tt.wantCode != 0 { require.Error(t, err) assert.Equal(t, tt.wantCode, httperr.Code(err)) + if tt.wantErr != "" { + assert.Contains(t, err.Error(), tt.wantErr) + } return } if tt.wantErr != "" {