Fix skill install for qualified namespace/name references and missing parent directory#4841
Fix skill install for qualified namespace/name references and missing parent directory#4841
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4841 +/- ##
==========================================
- Coverage 69.25% 69.22% -0.04%
==========================================
Files 537 537
Lines 55380 55401 +21
==========================================
- Hits 38354 38351 -3
- Misses 14080 14105 +25
+ Partials 2946 2945 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
JAORMX
left a comment
There was a problem hiding this comment.
Hey! Nice fix. The core approach is solid... the :@ heuristic to distinguish "ambiguous namespace/name" from "unambiguous OCI ref with tag/digest" is clean and easy to reason about. The installFromResolvedRegistry extraction is a good refactor, and the writer.go fix addresses a real first-install bug.
I verified the locking story and it's fine. The fallback path in Install() never holds a lock when calling downstream installers, so no deadlock risk. InstallOptions is passed by value and slice fields are replaced wholesale, not mutated in place. All good there.
A few things I'd like addressed before merging:
The multi-segment OCI edge case. A ref like myregistry.com/org/skill (3 segments, no tag) passes the guard and triggers the fallback. The namespace filtering makes a false match unlikely, but it's a supply-chain confusion vector in principle. Adding || strings.Count(opts.Name, "/") > 1 to the guard closes this cleanly. See inline comment.
The writer.go fix needs a test. All existing WriteFiles tests pre-create the parent directory, so no test actually proves this fix works. A test with a non-existent parent would take ~10 lines and directly exercise the bugfix.
CodeQL alert. The #nosec / lgtm annotations won't suppress CodeQL (different tool). This needs to be dismissed in GitHub's Security tab.
The rest are nits and nice-to-haves (logging the original OCI error before fallback, noting the 404->422 behavioral change in the PR description, tightening gomock.Any() in tests). See inline comments for details.
bb08ce8 to
ce7d910
Compare
ce7d910 to
93c8fc5
Compare
Extend the fallback guard in Install and GetContent to also reject refs with more than one '/' (e.g. ghcr.io/org/skill). Previously only ':' and '@' were checked, so a tagless multi-segment OCI reference could silently trigger a registry lookup on pull failure. Add a Debug log line before the fallback so operators can see when the fallback path is taken. Fix installFromResolvedRegistry passing opts.Name (the OCI reference string) to installAndRegister instead of result.Skill.Metadata.Name, which caused incorrect group entries and broken rollback.
…ases Add three new test cases to TestInstallQualifiedNameOCIFallback: - digest ref (@sha256:...) must not trigger registry fallback - multi-segment OCI ref (ghcr.io/org/skill) must not trigger fallback - multiple registry matches must surface a conflict error to the caller Also pin the Pull mock's third argument in the three qualifying-name cases to the expected reference string instead of gomock.Any().
Replace the inlined string-based guard in Install and GetContent with a named helper that combines parsed-form inspection (name.Digest type assertion) with string checks for the tag and segment-count cases. A pure parsed-form check is not sufficient: nameref.ParseReference normalizes "foo/bar" to "foo/bar:latest" (name.Tag), so the parsed reference cannot distinguish a user-supplied tag from the default. The helper documents this constraint and combines both signals.
93c8fc5 to
f528c88
Compare
Summary
Installto fall back to registry catalog lookup when anamespace/namereference (e.g.io.github.stacklok/skill-creator) fails OCI pull because the namespace is parsed as a registry host. Names with an explicit tag or digest (e.g.ghcr.io/org/skill:v1) or more than one/(e.g.ghcr.io/org/skill) are unambiguously OCI and do not fall backinstallFromResolvedRegistryhelper to share dispatch logic betweeninstallFromRegistryLookupand the new fallback pathWriteFilesfailing to acquire the lock when the parent skills directory does not yet exist (e.g. first install to~/.cline/skills/)installFromResolvedRegistryOCI branch: passresult.Skill.Metadata.Nameinstead ofopts.Name(the OCI ref string) toinstallAndRegister, so group registration and rollback use the correct skill nameBehavioral change note
Previously, a registry entry that existed but had no installable OCI or git package fell through and returned 404 Not Found. It now returns 422 Unprocessable Entity ("resolved but no installable package"), which is a more accurate response.
Type of change
Test plan
task test)task lint-fix)Changes
pkg/skills/skillsvc/skillsvc.goInstallandGetContent; tighten guard to also block multi-segment refs; add debug log before fallback; fixopts.Namebug in OCI branch ofinstallFromResolvedRegistrypkg/skills/skillsvc/skillsvc_test.goTestInstallQualifiedNameOCIFallbackwith digest, multi-segment, and ambiguity cases; pinPullmock argumentspkg/skills/gitresolver/writer.goMkdirAllparent before acquiring file lockpkg/skills/gitresolver/writer_test.goDoes this introduce a user-facing change?
Yes —
thv skill install io.github.stacklok/<skill-name>now works correctly instead of failing with "no such host".