Skip to content

Hide pulled skill artifacts from local builds listing#4971

Closed
samuv wants to merge 3 commits intomainfrom
skill-local-build
Closed

Hide pulled skill artifacts from local builds listing#4971
samuv wants to merge 3 commits intomainfrom
skill-local-build

Conversation

@samuv
Copy link
Copy Markdown
Contributor

@samuv samuv commented Apr 21, 2026

Summary

  • GET /api/v1beta/skills/content and OCI-based install both tag the shared local OCI store as a cache — then ListBuilds (GET /api/v1beta/skills/builds) iterates every tag and reports them as local builds, so every previewed or installed remote skill pollutes the "what have I built locally?" view.
  • Introduce a small file-backed build provenance index at <ociStoreRoot>/builds.json that tracks only tags produced by Build. ListBuilds now iterates this index instead of ociStore.ListTags(); pulled tags stay in the store as a cache but are invisible to the builds listing.
  • Build records, DeleteBuild forgets, ListBuilds prunes stale entries on the fly. OCI pulls (install + content) deliberately skip the index — comments at each site explain why.
  • New() runs a one-shot migration that grandfathers pre-existing simple tags (no /, : or @) so users with artifacts from before this change keep seeing their builds.

Type of change

  • Bug fix (user-facing behaviour of ListBuilds / GET /api/v1beta/skills/builds)

Test plan

  • Unit tests (task test on pkg/skills/skillsvc/...)
  • Linting (task lint-fix — 0 issues)
  • New regression tests assert that after a content-API preview pull or an install pull, ListBuilds still returns what it returned before the call
  • New migration test asserts pre-existing simple tags remain visible while pre-existing fully-qualified OCI-ref tags are hidden

Changes

File Change
pkg/skills/skillsvc/provenance.go New: file-backed buildProvenance (Record / Forget / List / Has / Seed / Exists) with atomic temp-file + rename writes and in-process mutex
pkg/skills/skillsvc/service.go Add provenance *buildProvenance to service, initialise it in New() from ociStore.Root(), and add migrateBuildProvenance + looksLikePulledRef for the one-shot grandfather migration
pkg/skills/skillsvc/build.go Build records provenance after tag; ListBuilds iterates provenance entries and prunes stale ones; DeleteBuild forgets provenance after the store delete
pkg/skills/skillsvc/content.go Comment at the registry.Pull site clarifying that content-preview pulls intentionally skip provenance
pkg/skills/skillsvc/install_oci.go Same intent-documenting comment at the install pull site
pkg/skills/skillsvc/provenance_test.go New: unit tests for Record/Forget/List/Has/Seed, atomic write behaviour, nil-safety, concurrent writes, corrupt-JSON handling, looksLikePulledRef
pkg/skills/skillsvc/build_test.go New TestListBuilds cases (pulled tags hidden, pull + build mix, migration, stale-entry pruning), new TestDeleteBuild case (provenance forgotten), new TestBuild_RecordsProvenance
pkg/skills/skillsvc/content_test.go New remote pull does not pollute ListBuilds case
pkg/skills/skillsvc/install_oci_test.go New TestInstallFromOCI_DoesNotLeakIntoListBuilds

Does this introduce a user-facing change?

Yes — the response of GET /api/v1beta/skills/builds changes.

  • Before: any skill pulled by install or by the content API appeared as a local build, because the endpoint listed every tag in the shared OCI store.
  • After: only skills produced by thv skill build (or equivalent POST /api/v1beta/skills/build) appear. Pulled skills stay on disk as a cache (transparent speed-up for re-previews / re-installs) but are no longer surfaced as builds.

Existing users are migrated automatically on first service start: simple tags already in the store are grandfathered as builds, fully-qualified OCI references are treated as pulls and omitted.

samuv added 3 commits April 21, 2026 16:45
Introduce a file-backed index at <ociStoreRoot>/builds.json that
records which tags in the shared local OCI store were produced by a
local Build. The index lives next to the OCI layout, uses atomic
temp-file + rename writes, and serializes access with an in-process
mutex.

This is a self-contained addition — no callers yet. Subsequent commits
wire it into the skill service so ListBuilds can filter out artifacts
that were only pulled (by install or the content API) for caching.
Skills pulled into the shared OCI store by install or the content
API were tagged for caching and then surfaced by ListBuilds, making
every previewed or installed remote skill look like a local build.

Wire the build provenance index into the skill service:

- New() initialises provenance from ociStore.Root() and runs a
  one-shot migration that grandfathers pre-existing simple tags
  (no '/', ':' or '@') so existing users do not lose their builds.
- Build records each tagged artifact after the ociStore.Tag call.
- DeleteBuild forgets the provenance entry after the store delete.
- ListBuilds iterates provenance entries instead of every OCI tag
  and prunes stale entries whose tags no longer resolve.

OCI pulls continue to tag the local store as a cache but are no
longer visible to ListBuilds because those paths deliberately skip
provenance recording.
Add regression tests that simulate the real Pull tagging side
effect for both GetContent and installFromOCI and assert ListBuilds
stays empty afterwards. Annotate each registry.Pull call site with
a comment stating that skipping build provenance is intentional, so
these paths are not silently 'fixed' later by adding a Record call.
@samuv samuv self-assigned this Apr 21, 2026
@samuv samuv requested a review from JAORMX as a code owner April 21, 2026 14:46
@github-actions github-actions Bot added the size/L Large PR: 600-999 lines changed label Apr 21, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 21, 2026

Codecov Report

❌ Patch coverage is 68.27957% with 59 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.56%. Comparing base (f3c9f24) to head (4c3598b).
⚠️ Report is 13 commits behind head on main.

Files with missing lines Patch % Lines
pkg/skills/skillsvc/provenance.go 73.64% 21 Missing and 13 partials ⚠️
pkg/skills/skillsvc/service.go 57.14% 9 Missing and 6 partials ⚠️
pkg/skills/skillsvc/build.go 54.54% 6 Missing and 4 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4971      +/-   ##
==========================================
+ Coverage   69.52%   69.56%   +0.03%     
==========================================
  Files         551      552       +1     
  Lines       55918    56109     +191     
==========================================
+ Hits        38879    39032     +153     
- Misses      14042    14061      +19     
- Partials     2997     3016      +19     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@JAORMX
Copy link
Copy Markdown
Collaborator

JAORMX commented Apr 22, 2026

So, quick thought on the provenance name first. In the OCI/supply-chain world "provenance" means signed build attestations... SLSA, cosign, in-toto. Opening provenance.go I expected attestation code and found json.Marshal of tag → digest. That's going to cost comprehension every time someone lands here. This is really a local "did I build this tag, or did it come from a pull?" index. buildIndex would be my vote. builds.json for the file is already good.

Before renaming anything though, I'd like to push back on the sidecar itself. What if we encoded the "I built this" signal directly on the OCI manifest as an annotation? Something like dev.stacklok.toolhive.local-build=true, set by Build and stripped by Push before it goes to the remote. ListBuilds filters by the annotation. You're already doing Resolve + manifest read in isSkillArtifact, so per-tag cost is basically unchanged.

Couple of reasons I like that better. Single source of truth, no sidecar to drift, no stale-entry pruning on list, no atomic-write path to maintain. No migration heuristic either, which is the part of the current PR that worries me most... looksLikePulledRef silently hiding an existing my-skill:v1.0.0 tag is a real failure mode, and with annotations pre-feature builds just don't show up until rebuilt. Honest gap beats silent misclassification. And the same marker composes with locally-built container images down the line, not just skills.

One trade-off to flag: push mutates the manifest before sending, so the pushed digest differs from the local one. Reasonable cost for a local-intent marker, but worth naming.

wdyt?

@samuv
Copy link
Copy Markdown
Contributor Author

samuv commented Apr 22, 2026

So, quick thought on the provenance name first. In the OCI/supply-chain world "provenance" means signed build attestations... SLSA, cosign, in-toto. Opening provenance.go I expected attestation code and found json.Marshal of tag → digest. That's going to cost comprehension every time someone lands here. This is really a local "did I build this tag, or did it come from a pull?" index. buildIndex would be my vote. builds.json for the file is already good.

Before renaming anything though, I'd like to push back on the sidecar itself. What if we encoded the "I built this" signal directly on the OCI manifest as an annotation? Something like dev.stacklok.toolhive.local-build=true, set by Build and stripped by Push before it goes to the remote. ListBuilds filters by the annotation. You're already doing Resolve + manifest read in isSkillArtifact, so per-tag cost is basically unchanged.

Couple of reasons I like that better. Single source of truth, no sidecar to drift, no stale-entry pruning on list, no atomic-write path to maintain. No migration heuristic either, which is the part of the current PR that worries me most... looksLikePulledRef silently hiding an existing my-skill:v1.0.0 tag is a real failure mode, and with annotations pre-feature builds just don't show up until rebuilt. Honest gap beats silent misclassification. And the same marker composes with locally-built container images down the line, not just skills.

One trade-off to flag: push mutates the manifest before sending, so the pushed digest differs from the local one. Reasonable cost for a local-intent marker, but worth naming.

wdyt?

Fair points, wanted to see the annotation approach in the flesh before answering. I agree, it's noticeably more straightforward and less mechanical than the sidecar: single source of truth, no builds.json to drift, no atomic-write path, no stale-entry pruning, and crucially no looksLikePulledRef grandfather heuristic that could silently hide an existing tag.

On the "provenance" name: you're right, that's a loaded term in the OCI/supply-chain world and buildIndex would've been the honest rename. Moot now that the sidecar is gone.

On the push-mutates-the-manifest trade-off you flagged: turned out not to bite us. Stamping the marker at the descriptor level in index.json (instead of inside the manifest blob) means the artifact digest is unchanged, and oras-go strips descriptor annotations when it resolves a reference by digest, which is exactly what push does. So the marker is genuinely local-only, no pre-push strip step needed, and pulled digests match exactly. Same mechanism is why install/content pulls don't accidentally inherit the marker either: Registry.Pull tags by digest, gets a plain descriptor back, so pulled tags are invisible to ListBuilds for free.

Opened #5000 with that implementation and closing this one as superseded. The one honest gap vs the sidecar: pre-feature local tags won't show up in ListBuilds until rebuilt, no migration. I think that's the right trade (honest gap > silent misclassification) but flagging it explicitly. I don't think we have stable user based tho

@samuv
Copy link
Copy Markdown
Contributor Author

samuv commented Apr 22, 2026

superseed by #5000

@samuv samuv closed this Apr 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/L Large PR: 600-999 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants