Skip to content
Closed
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
55 changes: 44 additions & 11 deletions pkg/skills/skillsvc/build.go
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,15 @@ func (s *service) Build(ctx context.Context, opts skills.BuildOptions) (*skills.
if tagErr := s.ociStore.Tag(ctx, result.IndexDigest, tag); tagErr != nil {
return nil, fmt.Errorf("tagging artifact: %w", tagErr)
}
// Record build provenance so ListBuilds surfaces this tag. Tags
// created by OCI pulls (install, content preview) deliberately skip
// this step and stay hidden from ListBuilds while their blobs remain
// as a cache.
if s.provenance != nil {
if recErr := s.provenance.Record(tag, result.IndexDigest.String()); recErr != nil {
return nil, fmt.Errorf("recording build provenance: %w", recErr)
}
}
}

ref := func() string {
Expand Down Expand Up @@ -111,38 +120,51 @@ func (s *service) Push(ctx context.Context, opts skills.PushOptions) error {
}

// ListBuilds returns all locally-built OCI skill artifacts in the local store.
// It iterates the build provenance index (populated by Build) rather than the
// full tag list, 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(
errors.New("OCI packaging is not configured"),
http.StatusInternalServerError,
)
}
if s.provenance == nil {
// ociStore is set but provenance failed to initialize; treat as
// empty rather than surfacing every pulled tag.
return []skills.LocalBuild{}, nil
}

tags, err := s.ociStore.ListTags(ctx)
entries, err := s.provenance.List()
if err != nil {
return nil, fmt.Errorf("listing local OCI builds: %w", err)
return nil, fmt.Errorf("listing build provenance: %w", err)
}

builds := make([]skills.LocalBuild, 0, len(tags))
for _, tag := range tags {
d, resolveErr := s.ociStore.Resolve(ctx, tag)
builds := make([]skills.LocalBuild, 0, len(entries))
for _, entry := range entries {
d, resolveErr := s.ociStore.Resolve(ctx, entry.Tag)
if resolveErr != nil {
slog.Debug("failed to resolve tag in local OCI store", "tag", tag, "error", resolveErr)
// Tag no longer resolves (e.g. deleted outside of DeleteBuild).
// Prune the stale provenance entry and skip.
slog.Debug("pruning stale provenance entry", "tag", entry.Tag, "error", resolveErr)
if forgetErr := s.provenance.Forget(entry.Tag); forgetErr != nil {
slog.Debug("failed to prune stale provenance entry", "tag", entry.Tag, "error", forgetErr)
}
continue
}

isSkill, typeErr := s.isSkillArtifact(ctx, d)
if typeErr != nil {
slog.Debug("failed to check artifact type in local OCI store", "tag", tag, "error", typeErr)
slog.Debug("failed to check artifact type in local OCI store", "tag", entry.Tag, "error", typeErr)
continue
}
if !isSkill {
continue
}

build := skills.LocalBuild{
Tag: tag,
Tag: entry.Tag,
Digest: d.String(),
}

Expand All @@ -152,7 +174,7 @@ func (s *service) ListBuilds(ctx context.Context) ([]skills.LocalBuild, error) {
build.Description = cfg.Description
build.Version = cfg.Version
} else if extractErr != nil {
slog.Debug("failed to extract skill config from local build", "tag", tag, "error", extractErr)
slog.Debug("failed to extract skill config from local build", "tag", entry.Tag, "error", extractErr)
}

builds = append(builds, build)
Expand All @@ -163,15 +185,26 @@ 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 provenance entry is also forgotten.
func (s *service) DeleteBuild(ctx context.Context, tag string) error {
if s.ociStore == nil {
return httperr.WithCode(
errors.New("OCI packaging is not configured"),
http.StatusInternalServerError,
)
}
return s.ociStore.DeleteBuild(ctx, tag)
if err := s.ociStore.DeleteBuild(ctx, tag); err != nil {
return err
}
if s.provenance != nil {
if forgetErr := s.provenance.Forget(tag); forgetErr != nil {
// The underlying artifact is gone; failing to clean up the
// provenance entry is non-fatal — ListBuilds will prune stale
// entries on the next call.
slog.Debug("failed to forget provenance entry", "tag", tag, "error", forgetErr)
}
}
return nil
}

// validateLocalPath checks that a path is non-empty, absolute, and does not
Expand Down
147 changes: 147 additions & 0 deletions pkg/skills/skillsvc/build_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -528,6 +528,98 @@ 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()
storeRoot := t.TempDir()
ociStore, err := ociskills.NewStore(storeRoot)
require.NoError(t, err)

// Construct the service first so the provenance migration runs on an
// empty store and records nothing.
svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore))

// Simulate a pull: a skill artifact tagged with a fully-qualified OCI
// reference. Build provenance is intentionally not touched by pulls.
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 Build-recorded tags are listed when pull and build coexist", func(t *testing.T) {
t.Parallel()
storeRoot := t.TempDir()
ociStore, err := ociskills.NewStore(storeRoot)
require.NoError(t, err)

svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore))

// Pre-stage a "pulled" artifact — never goes through provenance.
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"))

// Manually record a build provenance entry to simulate a Build call
// without exercising the full packager path. Pointing at an existing
// skill digest is sufficient for ListBuilds to surface it.
svcImpl := svc.(*service)
built := buildTestArtifact(t, ociStore, "built-skill", "1.0.0")
require.NoError(t, ociStore.Tag(t.Context(), built, "built-skill"))
require.NoError(t, svcImpl.provenance.Record("built-skill", built.String()))

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("migration grandfathers existing simple tags as builds", func(t *testing.T) {
t.Parallel()
storeRoot := t.TempDir()
ociStore, err := ociskills.NewStore(storeRoot)
require.NoError(t, err)

// Tags present BEFORE the service is constructed should be migrated.
builtDigest := buildTestArtifact(t, ociStore, "legacy-build", "1.0.0")
require.NoError(t, ociStore.Tag(t.Context(), builtDigest, "legacy-build"))
pulledDigest := buildTestArtifact(t, ociStore, "legacy-pull", "2.0.0")
require.NoError(t, ociStore.Tag(t.Context(), pulledDigest, "ghcr.io/org/legacy-pull:v2"))

svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore))
artifacts, err := svc.ListBuilds(t.Context())
require.NoError(t, err)
require.Len(t, artifacts, 1, "simple tags migrate, OCI-ref tags do not")
assert.Equal(t, "legacy-build", artifacts[0].Tag)

// builds.json must exist so the migration does not repeat.
info, statErr := os.Stat(filepath.Join(storeRoot, provenanceFileName))
require.NoError(t, statErr)
assert.Greater(t, info.Size(), int64(0))
})

t.Run("stale provenance entries are pruned on list", func(t *testing.T) {
t.Parallel()
storeRoot := t.TempDir()
ociStore, err := ociskills.NewStore(storeRoot)
require.NoError(t, err)

svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore))
svcImpl := svc.(*service)

// Record a provenance entry for a tag that is not (yet) in the store.
require.NoError(t, svcImpl.provenance.Record("ghost-tag", "sha256:aaa"))

artifacts, err := svc.ListBuilds(t.Context())
require.NoError(t, err)
assert.Empty(t, artifacts)

// The stale entry must be pruned as a side effect.
has, err := svcImpl.provenance.Has("ghost-tag")
require.NoError(t, err)
assert.False(t, has, "stale provenance entry should have been pruned")
})
}

func TestDeleteBuild(t *testing.T) {
Expand Down Expand Up @@ -587,4 +679,59 @@ func TestDeleteBuild(t *testing.T) {
require.Len(t, builds, 1)
assert.Equal(t, "tag-b", builds[0].Tag)
})

t.Run("delete removes provenance entry", func(t *testing.T) {
t.Parallel()
ociStore, err := ociskills.NewStore(t.TempDir())
require.NoError(t, err)

d := buildTestArtifact(t, ociStore, "my-skill", "1.0.0")
require.NoError(t, ociStore.Tag(t.Context(), d, "my-skill"))

svc := New(&storage.NoopSkillStore{}, WithOCIStore(ociStore))
svcImpl := svc.(*service)

// Migration should have seeded the entry.
has, err := svcImpl.provenance.Has("my-skill")
require.NoError(t, err)
require.True(t, has)

require.NoError(t, svc.DeleteBuild(t.Context(), "my-skill"))

has, err = svcImpl.provenance.Has("my-skill")
require.NoError(t, err)
assert.False(t, has, "provenance entry must be forgotten after DeleteBuild")
})
}

func TestBuild_RecordsProvenance(t *testing.T) {
t.Parallel()

ctrl := gomock.NewController(t)
ociStore, err := ociskills.NewStore(t.TempDir())
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 build provenance index recorded it.
builds, err := svc.ListBuilds(t.Context())
require.NoError(t, err)
require.Len(t, builds, 1)
assert.Equal(t, "my-skill", builds[0].Tag)
}
4 changes: 4 additions & 0 deletions pkg/skills/skillsvc/content.go
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,10 @@ 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 record build provenance:
// 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
42 changes: 42 additions & 0 deletions pkg/skills/skillsvc/content_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -255,6 +255,48 @@ 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 must not touch build provenance).
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
3 changes: 3 additions & 0 deletions pkg/skills/skillsvc/install_oci.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,9 @@ func (s *service) installFromOCI(
pullCtx, cancel := context.WithTimeout(ctx, ociPullTimeout)
defer cancel()

// Install pulls intentionally do NOT record build provenance: 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