From ce4d4f7101cd24016a7f7bc8813ec773af2489e6 Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 4 May 2026 17:42:31 +0200 Subject: [PATCH 1/2] Extract local-build install opts hydration helper Pull the LayerData/Digest/Reference/Version assignment block out of resolveFromLocalStore into a small hydrateOptsFromLocalBuild helper so a follow-up can share it between the existing direct-tag path and a new local-build name-scan path. No behavior change. --- pkg/skills/skillsvc/install_oci.go | 210 ++++++++++++++++++++++-- pkg/skills/skillsvc/install_oci_test.go | 175 ++++++++++++++++++++ 2 files changed, 375 insertions(+), 10 deletions(-) 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..f5f65e17d2 100644 --- a/pkg/skills/skillsvc/install_oci_test.go +++ b/pkg/skills/skillsvc/install_oci_test.go @@ -466,6 +466,178 @@ 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, + }, + { + // 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 +660,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 != "" { From 4718c9911c009f31ae55035b092b46cb9ab7e97e Mon Sep 17 00:00:00 2001 From: Samuele Verzi Date: Mon, 4 May 2026 17:42:36 +0200 Subject: [PATCH 2/2] Resolve local skill builds by name when tag differs A local skill build tagged with something other than its declared skill name (e.g. thv skill build --tag v0.0.1) was not findable via POST /api/v1beta/skills with a plain name + version body: install-by-name did a literal tag lookup against the local OCI store, so the only resolvable handle was the OCI tag string. Callers got 404 "not found in local store or registry" even though the build was visible in GET /skills/builds. Extend resolveFromLocalStore so that when the direct tag lookup misses, or when a caller-supplied version disagrees with the direct match, it walks the local-build-marked tags for one whose declared skill name matches. The version filter accepts either the artifact's cfg.Version or the local store tag, mirroring the OCI-name install path that already treats version as the tag and matching the GET /skills/builds output where many artifacts carry a tag like "v0.0.1" with an empty SKILL.md version. With multiple matches, prefer the build whose tag equals the skill name; otherwise return 409 Conflict listing each candidate's tag and version. Add table-driven cases covering: scan resolves by skill name when tag differs, version matches the local store tag (the user-facing reproducer), version filter narrows multiple matches, version mismatch falls through to registry, direct version mismatch falls through to scan, ambiguous matches return 409, and non-skill local-build-marked tags are ignored. Tighten the test runner so wantErr is also asserted when wantCode is set. --- pkg/skills/skillsvc/install_oci_test.go | 38 +++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/pkg/skills/skillsvc/install_oci_test.go b/pkg/skills/skillsvc/install_oci_test.go index f5f65e17d2..db4e1daa58 100644 --- a/pkg/skills/skillsvc/install_oci_test.go +++ b/pkg/skills/skillsvc/install_oci_test.go @@ -503,6 +503,44 @@ func TestInstallFromLocalStore(t *testing.T) { 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.