Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 22 additions & 3 deletions pkg/skills/skillsvc/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
}
Expand Down Expand Up @@ -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(
Expand All @@ -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)
Expand Down Expand Up @@ -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(
Expand Down
156 changes: 145 additions & 11 deletions pkg/skills/skillsvc/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,13 +4,15 @@
package skillsvc

import (
"encoding/json"
"fmt"
"net/http"
"os"
"path/filepath"
"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"
Expand Down Expand Up @@ -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())
Expand All @@ -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())
Expand All @@ -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())
Expand All @@ -513,21 +515,78 @@ 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())
require.NoError(t, err)
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) {
Expand All @@ -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"))
Expand Down Expand Up @@ -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"))
Expand All @@ -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
}
6 changes: 6 additions & 0 deletions pkg/skills/skillsvc/content.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
43 changes: 43 additions & 0 deletions pkg/skills/skillsvc/content_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
5 changes: 5 additions & 0 deletions pkg/skills/skillsvc/install_oci.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand Down
Loading
Loading