diff --git a/pkg/skills/skillsvc/build.go b/pkg/skills/skillsvc/build.go index c5a762c58a..bcfe32f1c2 100644 --- a/pkg/skills/skillsvc/build.go +++ b/pkg/skills/skillsvc/build.go @@ -64,7 +64,13 @@ func (s *service) Build(ctx context.Context, opts skills.BuildOptions) (*skills. } if tag != "" { - if tagErr := s.ociStore.Tag(ctx, result.IndexDigest, tag); tagErr != nil { + // Tag the artifact and stamp the local-build marker on the root-index + // descriptor entry so ListBuilds can distinguish this tag from ones + // created by OCI pulls (install, content preview). The marker lives + // at the descriptor level in index.json, not in the manifest blob, + // so it doesn't change the artifact digest and is not carried across + // push (push resolves by digest, which returns a plain descriptor). + if tagErr := tagAsLocalBuild(ctx, s.ociStore, result.IndexDigest, tag); tagErr != nil { return nil, fmt.Errorf("tagging artifact: %w", tagErr) } } @@ -111,6 +117,9 @@ func (s *service) Push(ctx context.Context, opts skills.PushOptions) error { } // ListBuilds returns all locally-built OCI skill artifacts in the local store. +// Tags are filtered by the local-build descriptor annotation (set by Build), +// so artifacts pulled into the store by install or the content API for +// caching do not appear here. func (s *service) ListBuilds(ctx context.Context) ([]skills.LocalBuild, error) { if s.ociStore == nil { return nil, httperr.WithCode( @@ -121,11 +130,20 @@ func (s *service) ListBuilds(ctx context.Context) ([]skills.LocalBuild, error) { tags, err := s.ociStore.ListTags(ctx) if err != nil { - return nil, fmt.Errorf("listing local OCI builds: %w", err) + return nil, fmt.Errorf("listing OCI tags: %w", err) } builds := make([]skills.LocalBuild, 0, len(tags)) 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 tag in local OCI store", "tag", tag, "error", resolveErr) @@ -163,7 +181,8 @@ func (s *service) ListBuilds(ctx context.Context) ([]skills.LocalBuild, error) { // DeleteBuild removes a locally-built OCI skill artifact from the local store. // It deletes the tag and, when no other tag shares the same digest, also -// garbage-collects all associated blobs. +// garbage-collects all associated blobs. The local-build descriptor +// annotation disappears from index.json together with the tag. func (s *service) DeleteBuild(ctx context.Context, tag string) error { if s.ociStore == nil { return httperr.WithCode( diff --git a/pkg/skills/skillsvc/build_test.go b/pkg/skills/skillsvc/build_test.go index 04569e8e7a..d30ede4aa3 100644 --- a/pkg/skills/skillsvc/build_test.go +++ b/pkg/skills/skillsvc/build_test.go @@ -4,6 +4,7 @@ package skillsvc import ( + "encoding/json" "fmt" "net/http" "os" @@ -11,6 +12,7 @@ import ( "strings" "testing" + ocispec "github.com/opencontainers/image-spec/specs-go/v1" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "go.uber.org/mock/gomock" @@ -446,7 +448,7 @@ func TestListBuilds(t *testing.T) { // Build a real artifact via the packager so extractOCIContent works. d := buildTestArtifact(t, ociStore, "my-skill", "1.2.3") - require.NoError(t, ociStore.Tag(t.Context(), d, "my-skill")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "my-skill")) svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) artifacts, err := svc.ListBuilds(t.Context()) @@ -465,9 +467,9 @@ func TestListBuilds(t *testing.T) { require.NoError(t, err) d1 := buildTestArtifact(t, ociStore, "skill-a", "1.0.0") - require.NoError(t, ociStore.Tag(t.Context(), d1, "skill-a")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d1, "skill-a")) d2 := buildTestArtifact(t, ociStore, "skill-b", "2.0.0") - require.NoError(t, ociStore.Tag(t.Context(), d2, "skill-b")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d2, "skill-b")) svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) artifacts, err := svc.ListBuilds(t.Context()) @@ -493,7 +495,7 @@ func TestListBuilds(t *testing.T) { skillIndex := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.index.v1+json","artifactType":"dev.toolhive.skills.v1","manifests":[]}` d, putErr := ociStore.PutManifest(t.Context(), []byte(skillIndex)) require.NoError(t, putErr) - require.NoError(t, ociStore.Tag(t.Context(), d, "bare-skill-tag")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "bare-skill-tag")) svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) artifacts, err := svc.ListBuilds(t.Context()) @@ -513,14 +515,15 @@ func TestListBuilds(t *testing.T) { // Store a valid skill artifact that should be returned. skillDigest := buildTestArtifact(t, ociStore, "real-skill", "1.0.0") - require.NoError(t, ociStore.Tag(t.Context(), skillDigest, "real-skill")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, skillDigest, "real-skill")) - // Store an index whose ArtifactType is not the skill type — simulates a - // remotely-pulled non-skill OCI artifact sharing the same store. + // Store an index whose ArtifactType is not the skill type. Tagging it + // as a local build simulates a caller that mistakenly flagged a + // non-skill artifact — ListBuilds must still exclude it by type. otherIndex := `{"schemaVersion":2,"mediaType":"application/vnd.oci.image.index.v1+json","artifactType":"application/vnd.docker.distribution.manifest.v2","manifests":[]}` otherDigest, putErr := ociStore.PutManifest(t.Context(), []byte(otherIndex)) require.NoError(t, putErr) - require.NoError(t, ociStore.Tag(t.Context(), otherDigest, "non-skill-tag")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, otherDigest, "non-skill-tag")) svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) artifacts, err := svc.ListBuilds(t.Context()) @@ -528,6 +531,62 @@ func TestListBuilds(t *testing.T) { require.Len(t, artifacts, 1) assert.Equal(t, "real-skill", artifacts[0].Tag) }) + + t.Run("pulled tags are hidden from ListBuilds", func(t *testing.T) { + t.Parallel() + ociStore, err := ociskills.NewStore(t.TempDir()) + require.NoError(t, err) + + svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) + + // Simulate a pull: tag the artifact via the plain ociStore.Tag path, + // which mirrors what Registry.Pull does internally (resolve by digest + // → plain descriptor → no local-build annotation). + d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, ociStore.Tag(t.Context(), d, "ghcr.io/org/my-skill:v1.0.0")) + + artifacts, err := svc.ListBuilds(t.Context()) + require.NoError(t, err) + assert.Empty(t, artifacts, "pulled tags must not appear in ListBuilds") + }) + + t.Run("only locally-built tags are listed when pull and build coexist", func(t *testing.T) { + t.Parallel() + ociStore, err := ociskills.NewStore(t.TempDir()) + require.NoError(t, err) + + svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) + + // Pulled artifact: tagged without the local-build marker. + pulled := buildTestArtifact(t, ociStore, "pulled-skill", "9.9.9") + require.NoError(t, ociStore.Tag(t.Context(), pulled, "ghcr.io/org/pulled-skill:v9.9.9")) + + // Locally-built artifact: tagged with the marker. + built := buildTestArtifact(t, ociStore, "built-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, built, "built-skill")) + + artifacts, err := svc.ListBuilds(t.Context()) + require.NoError(t, err) + require.Len(t, artifacts, 1) + assert.Equal(t, "built-skill", artifacts[0].Tag) + }) + + t.Run("pre-feature tags without the marker do not appear", func(t *testing.T) { + t.Parallel() + ociStore, err := ociskills.NewStore(t.TempDir()) + require.NoError(t, err) + + // Tag via the plain path, as if the artifact had been built before + // this feature existed. The honest gap: ListBuilds hides it until + // the user rebuilds and re-tags. + d := buildTestArtifact(t, ociStore, "legacy-build", "1.0.0") + require.NoError(t, ociStore.Tag(t.Context(), d, "legacy-build")) + + svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) + artifacts, err := svc.ListBuilds(t.Context()) + require.NoError(t, err) + assert.Empty(t, artifacts) + }) } func TestDeleteBuild(t *testing.T) { @@ -547,7 +606,7 @@ func TestDeleteBuild(t *testing.T) { require.NoError(t, err) d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") - require.NoError(t, ociStore.Tag(t.Context(), d, "my-skill")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "my-skill")) svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) require.NoError(t, svc.DeleteBuild(t.Context(), "my-skill")) @@ -575,8 +634,8 @@ func TestDeleteBuild(t *testing.T) { require.NoError(t, err) d := buildTestArtifact(t, ociStore, "shared-skill", "1.0.0") - require.NoError(t, ociStore.Tag(t.Context(), d, "tag-a")) - require.NoError(t, ociStore.Tag(t.Context(), d, "tag-b")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "tag-a")) + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "tag-b")) svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) require.NoError(t, svc.DeleteBuild(t.Context(), "tag-a")) @@ -587,4 +646,79 @@ func TestDeleteBuild(t *testing.T) { require.Len(t, builds, 1) assert.Equal(t, "tag-b", builds[0].Tag) }) + + t.Run("delete removes local-build marker from index.json", func(t *testing.T) { + t.Parallel() + storeRoot := t.TempDir() + ociStore, err := ociskills.NewStore(storeRoot) + require.NoError(t, err) + + d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + require.NoError(t, tagAsLocalBuild(t.Context(), ociStore, d, "my-skill")) + + // Sanity: the marker is on the tagged descriptor. + require.True(t, indexContainsTaggedMarker(t, storeRoot, "my-skill")) + + svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore)) + require.NoError(t, svc.DeleteBuild(t.Context(), "my-skill")) + + assert.False(t, indexContainsTaggedMarker(t, storeRoot, "my-skill"), + "descriptor carrying the marker must be gone after DeleteBuild") + }) +} + +func TestBuild_StampsLocalBuildAnnotation(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + storeRoot := t.TempDir() + ociStore, err := ociskills.NewStore(storeRoot) + require.NoError(t, err) + + d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + + p := ocimocks.NewMockSkillPackager(ctrl) + p.EXPECT().Package(gomock.Any(), "/some/dir", gomock.Any()). + Return(&ociskills.PackageResult{ + IndexDigest: d, + Config: &ociskills.SkillConfig{Name: "my-skill"}, + }, nil) + + svc := New(&storage.NoopSkillStore{}, + WithPackager(p), + WithOCIStore(ociStore), + ) + + _, err = svc.Build(t.Context(), skills.BuildOptions{Path: "/some/dir", Tag: "my-skill"}) + require.NoError(t, err) + + // After a successful Build, the tag must be surfaced by ListBuilds + // because the root-index descriptor carries the local-build marker. + builds, err := svc.ListBuilds(t.Context()) + require.NoError(t, err) + require.Len(t, builds, 1) + assert.Equal(t, "my-skill", builds[0].Tag) + + // The marker must land on the descriptor entry in index.json. + assert.True(t, indexContainsTaggedMarker(t, storeRoot, "my-skill"), + "root index.json must carry the local-build annotation for the tag") +} + +// indexContainsTaggedMarker reads the OCI store's root index.json and reports +// whether the descriptor entry tagged `tag` has the local-build annotation. +func indexContainsTaggedMarker(t *testing.T, storeRoot, tag string) bool { + t.Helper() + data, err := os.ReadFile(filepath.Join(storeRoot, "index.json")) + require.NoError(t, err) + var idx ocispec.Index + require.NoError(t, json.Unmarshal(data, &idx)) + for _, m := range idx.Manifests { + if m.Annotations[ocispec.AnnotationRefName] != tag { + continue + } + if m.Annotations[LocalBuildAnnotation] == "true" { + return true + } + } + return false } diff --git a/pkg/skills/skillsvc/content.go b/pkg/skills/skillsvc/content.go index 507cf6a13d..f2285fe409 100644 --- a/pkg/skills/skillsvc/content.go +++ b/pkg/skills/skillsvc/content.go @@ -165,6 +165,12 @@ func (s *service) getContentFromOCI(ctx context.Context, ref string) (*skills.Sk pullCtx, cancel := context.WithTimeout(ctx, ociPullTimeout) defer cancel() + // Content-preview pulls intentionally do NOT carry the local-build + // marker: Registry.Pull tags by digest, which returns a plain + // descriptor from the OCI store, so no annotations land on the + // root-index entry. The pulled blobs stay in the OCI store as a + // cache, but the tag is invisible to ListBuilds so remote skills + // browsed via the content API don't pollute the local builds listing. var pullErr error d, pullErr = s.registry.Pull(pullCtx, s.ociStore, qualifiedRef) if pullErr != nil { diff --git a/pkg/skills/skillsvc/content_test.go b/pkg/skills/skillsvc/content_test.go index 2ee52b512a..1eb66bc674 100644 --- a/pkg/skills/skillsvc/content_test.go +++ b/pkg/skills/skillsvc/content_test.go @@ -255,6 +255,49 @@ func TestGetContent(t *testing.T) { assert.Contains(t, content.Body, "Skill Creator") }) + t.Run("remote pull does not pollute ListBuilds", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + + ociStore, err := ociskills.NewStore(t.TempDir()) + require.NoError(t, err) + + // The real Pull would tag the pulled artifact in the local store. + // Simulate that side-effect here so we can verify ListBuilds still + // reports an empty list: pulls tag by digest, which yields a plain + // descriptor, so the local-build marker is never applied. + reg := ocimocks.NewMockRegistryClient(ctrl) + reg.EXPECT(). + Pull(gomock.Any(), ociStore, "ghcr.io/org/my-skill:v1"). + DoAndReturn(func(ctx context.Context, store *ociskills.Store, _ string) (godigest.Digest, error) { + d := buildTestArtifact(t, store, "my-skill", "1.0.0") + require.NoError(t, store.Tag(ctx, d, "ghcr.io/org/my-skill:v1")) + return d, nil + }) + + svc := New(&storage.NoopSkillStore{}, + WithOCIStore(ociStore), + WithRegistryClient(reg), + ) + + // Baseline: no builds before the content request. + builds, err := svc.ListBuilds(t.Context()) + require.NoError(t, err) + require.Empty(t, builds) + + content, err := svc.GetContent(t.Context(), skills.ContentOptions{ + Reference: "ghcr.io/org/my-skill:v1", + }) + require.NoError(t, err) + assert.Equal(t, "my-skill", content.Name) + + // After a content-preview pull, ListBuilds must still be empty: the + // blobs stay on disk as a cache but the tag is not treated as a local build. + builds, err = svc.ListBuilds(t.Context()) + require.NoError(t, err) + assert.Empty(t, builds, "content API must not leak pulled artifacts into ListBuilds") + }) + t.Run("qualified namespace/name filters registry matches", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) diff --git a/pkg/skills/skillsvc/install_oci.go b/pkg/skills/skillsvc/install_oci.go index bdd7bad17f..0b7ca5dc32 100644 --- a/pkg/skills/skillsvc/install_oci.go +++ b/pkg/skills/skillsvc/install_oci.go @@ -52,6 +52,11 @@ func (s *service) installFromOCI( pullCtx, cancel := context.WithTimeout(ctx, ociPullTimeout) defer cancel() + // Install pulls intentionally do NOT carry the local-build marker: + // Registry.Pull tags by digest, which returns a plain descriptor from + // the OCI store, so no annotations land on the root-index entry. The + // pulled blobs stay in the OCI store as a cache, but the tag is invisible + // to ListBuilds so installed remote skills don't appear as local builds. pulledDigest, err := s.registry.Pull(pullCtx, s.ociStore, ociRef) if err != nil { return nil, httperr.WithCode( diff --git a/pkg/skills/skillsvc/install_oci_test.go b/pkg/skills/skillsvc/install_oci_test.go index 9a51da7ac5..ee29ede8d8 100644 --- a/pkg/skills/skillsvc/install_oci_test.go +++ b/pkg/skills/skillsvc/install_oci_test.go @@ -301,6 +301,60 @@ func TestInstallFromOCI(t *testing.T) { } } +func TestInstallFromOCI_DoesNotLeakIntoListBuilds(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + + ociStore, err := ociskills.NewStore(tempDir(t)) + require.NoError(t, err) + + indexDigest := buildTestArtifact(t, ociStore, "my-skill", "1.0.0") + + // Simulate the real Pull side effect: tag the artifact in the local + // store. installFromOCI must not apply the local-build marker — Pull + // tags by digest, which yields a plain descriptor. + reg := ocimocks.NewMockRegistryClient(ctrl) + reg.EXPECT(). + Pull(gomock.Any(), ociStore, "ghcr.io/org/my-skill:v1"). + DoAndReturn(func(ctx context.Context, store *ociskills.Store, _ string) (godigest.Digest, error) { + require.NoError(t, store.Tag(ctx, indexDigest, "ghcr.io/org/my-skill:v1")) + return indexDigest, nil + }) + + skillStore := storemocks.NewMockSkillStore(ctrl) + skillStore.EXPECT().Get(gomock.Any(), "my-skill", skills.ScopeUser, ""). + Return(skills.InstalledSkill{}, storage.ErrNotFound) + skillStore.EXPECT().Create(gomock.Any(), gomock.Any()).Return(nil) + + pr := skillsmocks.NewMockPathResolver(ctrl) + targetDir := filepath.Join(tempDir(t), "installed", "my-skill") + pr.EXPECT().GetSkillPath("claude-code", "my-skill", skills.ScopeUser, "").Return(targetDir, nil) + + svc := New(skillStore, + WithRegistryClient(reg), + WithOCIStore(ociStore), + WithPathResolver(pr), + ) + + // Baseline: no builds before the install. + builds, err := svc.ListBuilds(t.Context()) + require.NoError(t, err) + require.Empty(t, builds) + + _, err = svc.Install(t.Context(), skills.InstallOptions{ + Name: "ghcr.io/org/my-skill:v1", + Clients: []string{"claude-code"}, + }) + require.NoError(t, err) + + // After the install, the OCI store contains the pulled artifact but + // ListBuilds must still be empty — only `thv skill build` output shows up. + builds, err = svc.ListBuilds(t.Context()) + require.NoError(t, err) + assert.Empty(t, builds, "install pulls must not leak into ListBuilds") +} + func TestInstallFromLocalStore(t *testing.T) { t.Parallel() diff --git a/pkg/skills/skillsvc/local_build_marker.go b/pkg/skills/skillsvc/local_build_marker.go new file mode 100644 index 0000000000..6b85b92bf3 --- /dev/null +++ b/pkg/skills/skillsvc/local_build_marker.go @@ -0,0 +1,63 @@ +// SPDX-FileCopyrightText: Copyright 2025 Stacklok, Inc. +// SPDX-License-Identifier: Apache-2.0 + +package skillsvc + +import ( + "context" + "fmt" + + "github.com/opencontainers/go-digest" + + ociskills "github.com/stacklok/toolhive-core/oci/skills" +) + +// LocalBuildAnnotation marks a tag in the local OCI store as produced by +// `thv skill build` rather than an OCI pull (install, content preview). +// The value is always "true" when set; absence means "not a local build". +// +// The annotation is stamped at the descriptor level inside the store's root +// index.json, not on the manifest content. Two properties follow from that: +// +// 1. Push resolves the artifact by digest, which returns a plain descriptor +// (oras-go's oci.Store strips annotations when the reference is a digest), +// so the marker never crosses the wire. +// 2. Pull calls Store.Tag with the pulled digest, which also resolves by +// digest before tagging, so pulled tags inherit no annotations and stay +// invisible to ListBuilds. +// +// The key is reverse-DNS namespaced so it composes with other locally-built +// artifact types (containers, MCP server images) in the future. +const LocalBuildAnnotation = "dev.stacklok.toolhive.local-build" + +// tagAsLocalBuild tags digest d with the given tag and stamps the local-build +// marker on the root-index descriptor entry. Equivalent to ociStore.Tag plus +// a descriptor annotation; callers must only invoke it from code paths that +// genuinely produced the artifact locally (currently only Build). +func tagAsLocalBuild(ctx context.Context, store *ociskills.Store, d digest.Digest, tag string) error { + target := store.Target() + desc, err := target.Resolve(ctx, d.String()) + if err != nil { + return fmt.Errorf("resolving digest for tag: %w", err) + } + // Resolve-by-digest returns a plain descriptor in oras-go, so overwriting + // Annotations can't clobber anything meaningful on the descriptor itself. + // (Content-level annotations live on the manifest/index blob and are + // unaffected.) + desc.Annotations = map[string]string{LocalBuildAnnotation: "true"} + if err := target.Tag(ctx, desc, tag); err != nil { + return fmt.Errorf("tagging artifact as local build: %w", err) + } + return nil +} + +// isLocalBuild reports whether the given tag in the local OCI store carries +// the local-build marker. Tags created by OCI pulls do not carry it, so this +// is the filter used by ListBuilds to hide cached remote artifacts. +func isLocalBuild(ctx context.Context, store *ociskills.Store, tag string) (bool, error) { + desc, err := store.Target().Resolve(ctx, tag) + if err != nil { + return false, err + } + return desc.Annotations[LocalBuildAnnotation] == "true", nil +}