Skip to content

Apply request version to tag-less skill OCI install ref#5078

Merged
samuv merged 1 commit intomainfrom
skill-install-fix
Apr 27, 2026
Merged

Apply request version to tag-less skill OCI install ref#5078
samuv merged 1 commit intomainfrom
skill-install-fix

Conversation

@samuv
Copy link
Copy Markdown
Contributor

@samuv samuv commented Apr 27, 2026

Summary

  • POST /api/v1beta/skills with {"name":"ghcr.io/<org>/<path>/<skill>","version":"0.1.0"} was silently pulling :latest and returning 404 Not Found from the upstream registry, even though the JSON body declared the version explicitly.
  • Motivating case: installing the firebase-security-rules-auditor registry entry whose OCI package identifier is ghcr.io/stacklok/dockyard/skills/firebase-security-rules-auditor:0.1.0. Sending the bare repo path + version: 0.1.0 (which is what callers building the request from a parsed registry entry naturally do) hit :latest, which is not published.
  • Single-commit fix: the install handler now splices opts.Version into the OCI reference as the tag when the caller supplies a tag-less ref. An explicit tag on the name still wins, and registry-resolved refs (which already carry a tag) are unaffected.

Fix — splice request version into tag-less OCI refs

pkg/skills/skillsvc/install.go::Install parses opts.Name via parseOCIReference, which delegates to nameref.ParseReference. For a tag-less ref like ghcr.io/org/path/skill, go-containerregistry normalizes the parsed reference to name.Tag with Identifier() == "latest". qualifiedOCIRef then reproduces that as …/skill:latest for the pull. opts.Version was only used as a fallback to populate the install record's version from the artifact config (lines 105–107 of install_oci.go) — it was never used to qualify the pull URL.

The fix splices the version in before the reference is parsed, so parseOCIReference, the unambiguous-ref check (isUnambiguousOCIRef), and qualifiedOCIRef all observe the fully qualified reference uniformly:

if opts.Version != "" &&
    strings.ContainsRune(opts.Name, '/') &&
    !strings.ContainsAny(opts.Name, ":@") {
    opts.Name = opts.Name + ":" + opts.Version
}

Guards:

  • opts.Version != "" — only act when the caller explicitly asked for a version.
  • strings.ContainsRune(opts.Name, '/') — only act on names that already look OCI-like; plain skill names (firebase-security-rules-auditor) are routed to the registry resolver and must not be silently mutated into OCI refs.
  • !strings.ContainsAny(opts.Name, ":@") — explicit tag/digest in the name wins. This preserves the existing precedence and leaves the registry-resolved path (where opts.Name = resolved.OCIRef.String() already includes the tag) untouched.

Flow after the change

flowchart TD
    in[POST /skills name+version] --> git{git:// ref?}
    git -- yes --> gitInstall[installFromGit]
    git -- no --> tagLess{name has '/' and no ':' or '@'?}
    tagLess -- yes + version --> splice["opts.Name = name + ':' + version"]
    tagLess -- no --> parse[parseOCIReference]
    splice --> parse
    parse --> isOCI{OCI ref?}
    isOCI -- yes --> oci[installFromOCI pulls qualifiedOCIRef]
    isOCI -- no --> name[installByName / registry lookup]
Loading

Type of change

  • Bug fix (install pull resolves to caller-requested version instead of :latest)

Test plan

  • task build
  • task lint-fix
  • Manual reproduction against a running thv serve:
    • Before: POST /api/v1beta/skills {"name":"ghcr.io/stacklok/dockyard/skills/firebase-security-rules-auditor","scope":"user","version":"0.1.0"}pulling OCI artifact "…/firebase-security-rules-auditor:latest": … not found.
    • After: the pull targets …/firebase-security-rules-auditor:0.1.0. (For this specific registry entry the install then trips the existing supply-chain check because the published artifact's SKILL.md declares a different name — that's a publisher-side packaging issue, not a regression.)
  • An explicit tag in the name still wins: name: ghcr.io/org/foo:1.2.3 + version: 0.1.0 → pulls :1.2.3 (no splice because the name contains :).
  • Plain skill names are not mutated: name: firebase-security-rules-auditor + version: 0.1.0 → still routed to the registry resolver (no / in name, splice skipped).
  • Registry-resolved path is unaffected: installFromResolvedRegistry sets opts.Name = resolved.OCIRef.String(), which already contains a :, so the splice never fires there.

Changes

File Change
pkg/skills/skillsvc/install.go Splice opts.Version in as the tag when the install name is a tag-less OCI-like ref; new strings import

Does this introduce a user-facing change?

Yes — bug-fix only. Callers that pass an OCI reference + a separate version field on POST /api/v1beta/skills now actually pull the requested version instead of silently falling through to :latest. Existing callers that already include the tag in the name, or that pass plain skill names through the registry resolver, see no behavioural change.

When POST /api/v1beta/skills receives an OCI reference without an explicit tag or digest (e.g. ghcr.io/stacklok/dockyard/skills/foo) and the JSON body supplies a separate "version" field, parseOCIReference + qualifiedOCIRef defaulted the pull to ":latest" and silently dropped opts.Version, surfacing a misleading "not found" instead of pulling the requested version.

Splice opts.Version in as the tag before parsing when the name is OCI-like (contains '/') and has no ':' or '@'. An explicit tag in the name still wins, and registry-resolved refs (which already include a tag) are unaffected.
@samuv samuv requested a review from JAORMX as a code owner April 27, 2026 15:56
@samuv samuv self-assigned this Apr 27, 2026
@github-actions github-actions Bot added the size/XS Extra small PR: < 100 lines changed label Apr 27, 2026
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 27, 2026

Codecov Report

❌ Patch coverage is 50.00000% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 69.82%. Comparing base (d82a03b) to head (e734bf6).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/skills/skillsvc/install.go 50.00% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #5078      +/-   ##
==========================================
- Coverage   69.82%   69.82%   -0.01%     
==========================================
  Files         564      564              
  Lines       56649    56653       +4     
==========================================
+ Hits        39555    39557       +2     
- Misses      14065    14066       +1     
- Partials     3029     3030       +1     

☔ 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.

@samuv samuv merged commit 340ac12 into main Apr 27, 2026
43 checks passed
@samuv samuv deleted the skill-install-fix branch April 27, 2026 16:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/XS Extra small PR: < 100 lines changed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants