feat: add upgrade command and --upgradable flag#31
Merged
Conversation
Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
Implements CheckUpgrades function that queries remote registries to find available upgrades for installed skills. The function: - Filters skills by remote source refs and valid semver versions - Uses injected TagLister for testability - Excludes draft/testing tags from upgrade candidates - Returns skills with newer published versions Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
Add --upgradable flag to list --installed command to show only skills with available updates. The flag requires --installed and displays current and latest versions side-by-side. Add ListTagsForRepo wrapper function to pkg/oci/catalog.go that splits a repository reference (registry/repo) into components and calls the existing ListRemoteTags function. Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
Part of #27 Assisted-By: Claude (Anthropic AI) <noreply@anthropic.com> Signed-off-by: Pavel Anni <panni@redhat.com>
Signed-off-by: Pavel Anni <panni@redhat.com>
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe changes implement upgrade functionality for a CLI tool called Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/CLI
participant Install as install.runInstall
participant OCI as oci.Client
participant LocalStore as Local Skill Store
participant Provenance as skill.yaml
User->>Install: upgrade [skill-name] or --all
Install->>Install: Scan installed skills
Install->>Install: Check upgrades via installed.CheckUpgrades
Install->>OCI: ListTagsForRepo(repo) for each candidate
OCI-->>Install: Latest version tag
Install->>Install: Filter draft/testing, parse semver
Install->>OCI: Pull latest ref
OCI-->>LocalStore: Download OCI artifact
Install->>LocalStore: Unpack into skill directory
Install->>Provenance: Update skill.yaml with new version
Provenance-->>User: Upgrade complete
sequenceDiagram
participant User as User/CLI
participant List as list.runListInstalled
participant Installed as installed.Scan
participant CheckUpgrade as installed.CheckUpgrades
participant OCI as oci.ListTagsForRepo
participant Output as Output Table
User->>List: list --installed --upgradable
List->>Installed: Scan installed skills
Installed-->>List: []InstalledSkill
List->>CheckUpgrade: CheckUpgrades(skills, opts)
CheckUpgrade->>OCI: Query tags for each skill repo
OCI-->>CheckUpgrade: Available tags
CheckUpgrade->>CheckUpgrade: Filter & compare semver
CheckUpgrade-->>List: []UpgradeCandidate
List->>Output: Render table with NAME/VERSION/LATEST/SOURCE/TARGET
Output-->>User: Display upgradable skills
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 47 minutes and 21 seconds.Comment |
When the ref looks remote (has a registry host) and isn't in the local store, install now pulls it automatically before unpacking. Eliminates the need for a separate pull step. Signed-off-by: Pavel Anni <panni@redhat.com>
After unpacking, the skill.yaml version now reflects the tag that was actually installed, not whatever was baked into the image. Fixes stale version after upgrade when the image's embedded version doesn't match its tag. Signed-off-by: Pavel Anni <panni@redhat.com>
- Fix looksRemote false positive on relative paths (./foo, ../bar) - Add warning on registry errors instead of silent skip - Extract shared resolveTargetDirs helper, remove duplication - Add TestCheckUpgrades_RelativePath test case Signed-off-by: Pavel Anni <panni@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dev/plans/2026-04-30-upgrade-command.md`:
- Around line 300-303: The loop silently ignores errors from opts.TagLister
(tags, err := opts.TagLister(ctx, repo, opts.SkipTLSVerify)) by using continue,
which can hide outages and yield false “up to date” results; change the handling
to return or propagate the error (or aggregate it) to the caller instead of
continuing — e.g., when err != nil, wrap/contextualize the error with repo
information and return it (or add it to an error accumulator) so callers can
surface failures explicitly.
- Around line 667-681: The loop over candidates currently logs errors and
continues, then always returns nil which causes a non-all single-skill failure
to exit 0; modify the logic in the function containing
candidates/upgradeSkill/all/upgraded so that when an upgrade attempt fails and
the flag all is false you immediately return a non-nil error (include context
like the skill name and underlying err), and if all is true but upgraded == 0
after processing return an error indicating no skills were upgraded; update any
callers to handle the returned error and keep the existing fmt.Fprintf lines for
stdout/stderr for logging.
In `@internal/cli/install.go`:
- Around line 158-176: The tagFromRef function currently returns the digest
portion when a ref contains '@' (e.g., "quay.io/acme/skill@sha256:..."); change
its behavior so that when an '@' is present it returns an empty string instead
of the digest. Update the tagFromRef function to detect '@' (the existing if idx
:= strings.Index(ref, "@") branch) and immediately return "" for digest-based
refs; leave the rest of the logic (handling ":" after the last "/") unchanged so
normal tag refs like "repo/name:1.2.3" still return the tag.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: fbae9c5f-f146-497d-9b09-121020f5bca6
📒 Files selected for processing (10)
dev/plans/2026-04-30-upgrade-command.mddev/specs/2026-04-30-upgrade-command-design.mdinternal/cli/helpers.gointernal/cli/install.gointernal/cli/list.gointernal/cli/root.gointernal/cli/upgrade.gopkg/installed/check.gopkg/installed/check_test.gopkg/oci/catalog.go
Comment on lines
+300
to
+303
| tags, err := opts.TagLister(ctx, repo, opts.SkipTLSVerify) | ||
| if err != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Do not silently ignore registry tag-list failures.
continue on TagLister error hides real outages and can produce false “up to date” results. Return (or aggregate) the error so callers can surface it explicitly.
Suggested adjustment
- tags, err := opts.TagLister(ctx, repo, opts.SkipTLSVerify)
- if err != nil {
- continue
- }
+ tags, err := opts.TagLister(ctx, repo, opts.SkipTLSVerify)
+ if err != nil {
+ return nil, fmt.Errorf("listing tags for %s: %w", repo, err)
+ }As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| tags, err := opts.TagLister(ctx, repo, opts.SkipTLSVerify) | |
| if err != nil { | |
| continue | |
| } | |
| tags, err := opts.TagLister(ctx, repo, opts.SkipTLSVerify) | |
| if err != nil { | |
| return nil, fmt.Errorf("listing tags for %s: %w", repo, err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/plans/2026-04-30-upgrade-command.md` around lines 300 - 303, The loop
silently ignores errors from opts.TagLister (tags, err := opts.TagLister(ctx,
repo, opts.SkipTLSVerify)) by using continue, which can hide outages and yield
false “up to date” results; change the handling to return or propagate the error
(or aggregate it) to the caller instead of continuing — e.g., when err != nil,
wrap/contextualize the error with repo information and return it (or add it to
an error accumulator) so callers can surface failures explicitly.
Comment on lines
+667
to
+681
| for _, c := range candidates { | ||
| if err := upgradeSkill(ctx, client, c, skipTLSVerify); err != nil { | ||
| fmt.Fprintf(cmd.ErrOrStderr(), "Error upgrading %s: %v\n", c.Installed.Name, err) | ||
| continue | ||
| } | ||
| fmt.Fprintf(cmd.OutOrStdout(), "Upgraded %s %s → %s (%s)\n", | ||
| c.Installed.Name, c.Installed.Version, c.LatestVersion, c.Installed.Target) | ||
| upgraded++ | ||
| } | ||
|
|
||
| if all && upgraded > 0 { | ||
| fmt.Fprintf(cmd.OutOrStdout(), "\nUpgraded %d skill(s).\n", upgraded) | ||
| } | ||
|
|
||
| return nil |
There was a problem hiding this comment.
Single-skill upgrade can fail but still return success.
The loop logs errors and continues, then returns nil. With one candidate, a failed upgrade still exits 0. Return an error when !all and an upgrade attempt fails (or when upgraded == 0 after failures).
Suggested adjustment
for _, c := range candidates {
if err := upgradeSkill(ctx, client, c, skipTLSVerify); err != nil {
- fmt.Fprintf(cmd.ErrOrStderr(), "Error upgrading %s: %v\n", c.Installed.Name, err)
- continue
+ if !all {
+ return fmt.Errorf("upgrading %s: %w", c.Installed.Name, err)
+ }
+ fmt.Fprintf(cmd.ErrOrStderr(), "Error upgrading %s: %v\n", c.Installed.Name, err)
+ continue
}
fmt.Fprintf(cmd.OutOrStdout(), "Upgraded %s %s → %s (%s)\n",
c.Installed.Name, c.Installed.Version, c.LatestVersion, c.Installed.Target)
upgraded++
}As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| for _, c := range candidates { | |
| if err := upgradeSkill(ctx, client, c, skipTLSVerify); err != nil { | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Error upgrading %s: %v\n", c.Installed.Name, err) | |
| continue | |
| } | |
| fmt.Fprintf(cmd.OutOrStdout(), "Upgraded %s %s → %s (%s)\n", | |
| c.Installed.Name, c.Installed.Version, c.LatestVersion, c.Installed.Target) | |
| upgraded++ | |
| } | |
| if all && upgraded > 0 { | |
| fmt.Fprintf(cmd.OutOrStdout(), "\nUpgraded %d skill(s).\n", upgraded) | |
| } | |
| return nil | |
| for _, c := range candidates { | |
| if err := upgradeSkill(ctx, client, c, skipTLSVerify); err != nil { | |
| if !all { | |
| return fmt.Errorf("upgrading %s: %w", c.Installed.Name, err) | |
| } | |
| fmt.Fprintf(cmd.ErrOrStderr(), "Error upgrading %s: %v\n", c.Installed.Name, err) | |
| continue | |
| } | |
| fmt.Fprintf(cmd.OutOrStdout(), "Upgraded %s %s → %s (%s)\n", | |
| c.Installed.Name, c.Installed.Version, c.LatestVersion, c.Installed.Target) | |
| upgraded++ | |
| } | |
| if all && upgraded > 0 { | |
| fmt.Fprintf(cmd.OutOrStdout(), "\nUpgraded %d skill(s).\n", upgraded) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@dev/plans/2026-04-30-upgrade-command.md` around lines 667 - 681, The loop
over candidates currently logs errors and continues, then always returns nil
which causes a non-all single-skill failure to exit 0; modify the logic in the
function containing candidates/upgradeSkill/all/upgraded so that when an upgrade
attempt fails and the flag all is false you immediately return a non-nil error
(include context like the skill name and underlying err), and if all is true but
upgraded == 0 after processing return an error indicating no skills were
upgraded; update any callers to handle the returned error and keep the existing
fmt.Fprintf lines for stdout/stderr for logging.
- Return error on single-skill upgrade failure instead of exit 0 - Return error when --all upgrades nothing due to errors - Skip version sync for digest-based refs (sha256:...) Signed-off-by: Pavel Anni <panni@redhat.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
skillctl upgrade <skill-name> --target <agent>to pull and re-install the latest published version from the source registryskillctl upgrade --all --target <agent>for batch upgradeslist --installed --upgradableto preview available upgradesCheckUpgradesfunction inpkg/installed/with semver comparison and injectable tag lister for testabilityPart of #27
Design decisions
-draftor-testingsuffix are excluded.or:in first segment); local store refs are skippedTagListerfunction type injected viaCheckOptionsfor unit test isolation (no real registry needed)writeProvenancefrom install flow to update skill.yaml after upgradeTest plan
TestCheckUpgrades_HasUpgrade— installed 1.0.0, remote has 2.0.0 publishedTestCheckUpgrades_AlreadyLatest— installed 2.0.0, no newer publishedTestCheckUpgrades_NoProvenance— skill without source skippedTestCheckUpgrades_OnlyDraftTags— newer versions only in draft/testingTestCheckUpgrades_LocalRef— local store ref (no registry host) skippedTestCheckUpgrades_InvalidSemver— non-semver version skippedgo test ./...— all tests passmake lint— 0 issuesSummary by CodeRabbit
New Features
skillctl upgradecommand to upgrade installed skills to the latest published version, with support for upgrading individual skills or all installed skills at once.--upgradableflag tolistcommand to display only installed skills with available updates.Tests