Refresh skills at all installed locations on CLI upgrade#356
Refresh skills at all installed locations on CLI upgrade#356
Conversation
RefreshSkillsIfVersionChanged now updates the skill file at every known install location (Claude Code, OpenCode, Codex) — not just the baseline ~/.agents/ directory. Broken Claude symlinks are repaired automatically. Project-relative paths are skipped since there is no reliable project root in the post-run hook.
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/skill.go">
<violation number="1" location="internal/commands/skill.go:358">
P1: Version sentinel can be advanced after only a partial multi-location refresh, which suppresses retry for failed installed locations.
(Based on your team's feedback about only updating the sentinel after a successful refresh so retries still happen on failure.) [FEEDBACK_USED]</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f2c5b258a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Pull request overview
This PR updates the CLI upgrade hook so embedded skill content is refreshed across all previously-installed global locations (Agents baseline, Claude, OpenCode, Codex), and adds automatic repair for broken Claude skill symlinks.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Changes:
- Replace baseline-only refresh logic with
refreshAllInstalledSkills()to overwriteSKILL.mdat each known installed global location. - Add
repairClaudeSkillLink()and invoke it during version-change refresh when Claude is detected. - Add unit tests covering multi-location refresh behavior, absent locations, project-relative skipping, and Claude symlink repair.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/commands/skill.go | Refresh installed skill files across known locations on version change; repair broken Claude symlink. |
| internal/commands/skill_test.go | Add tests validating multi-location refresh and Claude symlink repair behavior. |
Comments suppressed due to low confidence (1)
internal/commands/skill.go:349
refreshAllInstalledSkills()returns true when any location was successfully written, butRefreshSkillsIfVersionChanged()uses that boolean to decide whether refreshing “succeeded” and it’s safe to update the version sentinel. This can incorrectly advance the sentinel on partial failure (e.g. baseline SKILL.md write fails due to permissions, but another location succeeds), preventing retries and leaving baseline stale. Consider havingrefreshAllInstalledSkillsreport per-location failures (or anallSucceededflag), and gate the sentinel update on baseline refresh success (and/or all attempted locations succeeding) rather than “any file updated”.
refreshed := refreshAllInstalledSkills()
// Repair Claude symlink if broken (e.g. baseline dir was recreated)
if harness.DetectClaude() {
repairClaudeSkillLink()
}
// Update sentinel only when no refresh was needed or it succeeded.
// On transient failure, leave the sentinel stale so the next run retries.
needsRefresh := baselineSkillInstalled()
if !needsRefresh || refreshed {
_ = os.MkdirAll(filepath.Dir(sentinelPath), 0o755) //nolint:gosec // G301: config dir
_ = os.WriteFile(sentinelPath, []byte(version.Version), 0o644) //nolint:gosec // G306: not a secret
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d2287872c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/commands/skill_test.go">
<violation number="1" location="internal/commands/skill_test.go:553">
P2: Do not ignore `os.Getwd()` errors when changing global working directory in a test; failed CWD capture can leave the process in the wrong directory.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
internal/commands/skill.go:349
- The sentinel update logic only checks
baselineSkillInstalled(). If the user has the skill installed only in a non-baseline location (e.g. OpenCode/Codex) andrefreshAllInstalledSkills()fails (or partially fails),needsRefreshwill be false and the code will still write the version sentinel, preventing future retries despite the refresh failing. Consider basing the sentinel decision on whether any known install location was present and refreshed successfully (e.g. haverefreshAllInstalledSkillsreturn attempted/success, or treatupdated==0 && failed==0as “no refresh needed”, but keep the sentinel stale whenfailed>0).
// Update sentinel only when no refresh was needed or it succeeded.
// On transient failure, leave the sentinel stale so the next run retries.
needsRefresh := baselineSkillInstalled()
if !needsRefresh || refreshed {
_ = os.MkdirAll(filepath.Dir(sentinelPath), 0o755) //nolint:gosec // G301: config dir
_ = os.WriteFile(sentinelPath, []byte(version.Version), 0o644) //nolint:gosec // G306: not a secret
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
RefreshSkillsIfVersionChanged()now updates the skill file at every known install location (Claude Code, OpenCode, Codex) — not just the baseline~/.agents/directory~/.claude/skills/basecampare detected and repaired automaticallyPreviously, only
~/.agents/skills/basecamp/SKILL.mdwas refreshed on upgrade. Users who installed the skill to~/.claude/skills/basecamp/(as a copy, not symlink),~/.config/opencode/skill/basecamp/, or~/.codex/skills/basecamp/would not get updates until they manually re-ranbasecamp skill install.Test plan
TestRefreshAllInstalledSkills_MultipleLocations— installs to 3 locations, all get refreshedTestRefreshAllInstalledSkills_SkipsAbsentLocations— doesn't create files at uninstalled locationsTestRefreshAllInstalledSkills_SkipsProjectRelativePaths— doesn't touch project-relative skillsTestRepairClaudeSkillLink_BrokenSymlink— broken symlink gets repairedTestRepairClaudeSkillLink_HealthySymlink— healthy symlink is left aloneSummary by cubic
Refreshes the Basecamp skill on CLI upgrade across all installed locations (Claude Code, OpenCode, Codex), not just
~/.agents. Repairs broken Claude symlinks, skips project-relative installs, and handles missing vs. unreadable paths correctly.RefreshSkillsIfVersionChanged()updatesSKILL.mdat all detected global install paths; skips absent and project-relative paths, and treats permission/IO errors as failures.~/.claude/skills/basecampsymlink when Claude is present; leaves healthy links and directory copies untouched.Written for commit 5e7dbd9. Summary will update on new commits.