fix(skills): install agent skills by copy#1635
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds the ChangesAdd
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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. Comment |
github-actions
Bot
added
the
size/L
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/selfupdate/updater.go (1)
376-389: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse typed
errs.*errors for launcher lookup and timeout failures.
runSkillsCommandalso surfaces these failures to the CLI, but it returns barefmt.Errorffor bothLookPathand timeout paths. Please switch these to the repo’s typederrs.*wrappers so the update flow preserves the expected error classification. As per coding guidelines, command-facing failures must use typederrs.*errors, never barefmt.Errorf.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/selfupdate/updater.go` around lines 376 - 389, In runSkillsCommand, the launcher lookup and timeout paths currently return bare fmt.Errorf values instead of the repo’s typed errs.* wrappers. Update the error handling around exec.LookPath and the context deadline check so both failures are wrapped with the appropriate errs.* error type, preserving classification for the CLI while keeping the existing context from launcher and skillsUpdateTimeout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/selfupdate/updater.go`:
- Around line 235-249: RunPnpmInstall is returning plain fmt.Errorf values for
command-facing failures; switch those to the appropriate typed errs.* errors
instead. Update the missing pnpm branch and the timeout branch in RunPnpmInstall
so they construct the matching errs.* error types while preserving the
underlying error/context, and keep the command execution flow unchanged.
---
Outside diff comments:
In `@internal/selfupdate/updater.go`:
- Around line 376-389: In runSkillsCommand, the launcher lookup and timeout
paths currently return bare fmt.Errorf values instead of the repo’s typed errs.*
wrappers. Update the error handling around exec.LookPath and the context
deadline check so both failures are wrapped with the appropriate errs.* error
type, preserving classification for the CLI while keeping the existing context
from launcher and skillsUpdateTimeout.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2131ed73-01ab-457d-9890-2e69dd33867e
📒 Files selected for processing (7)
README.mdREADME.zh.mdcmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.goscripts/install-wizard.js
✅ Files skipped from review due to trivial changes (4)
- README.md
- cmd/update/update.go
- README.zh.md
- internal/selfupdate/updater_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/update/update_test.go
- scripts/install-wizard.js
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/selfupdate/updater.go (1)
376-389: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUse typed
errs.*errors for launcher lookup and timeout failures.
runSkillsCommandalso surfaces these failures to the CLI, but it returns barefmt.Errorffor bothLookPathand timeout paths. Please switch these to the repo’s typederrs.*wrappers so the update flow preserves the expected error classification. As per coding guidelines, command-facing failures must use typederrs.*errors, never barefmt.Errorf.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/selfupdate/updater.go` around lines 376 - 389, In runSkillsCommand, the launcher lookup and timeout paths currently return bare fmt.Errorf values instead of the repo’s typed errs.* wrappers. Update the error handling around exec.LookPath and the context deadline check so both failures are wrapped with the appropriate errs.* error type, preserving classification for the CLI while keeping the existing context from launcher and skillsUpdateTimeout.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/selfupdate/updater.go`:
- Around line 235-249: RunPnpmInstall is returning plain fmt.Errorf values for
command-facing failures; switch those to the appropriate typed errs.* errors
instead. Update the missing pnpm branch and the timeout branch in RunPnpmInstall
so they construct the matching errs.* error types while preserving the
underlying error/context, and keep the command execution flow unchanged.
---
Outside diff comments:
In `@internal/selfupdate/updater.go`:
- Around line 376-389: In runSkillsCommand, the launcher lookup and timeout
paths currently return bare fmt.Errorf values instead of the repo’s typed errs.*
wrappers. Update the error handling around exec.LookPath and the context
deadline check so both failures are wrapped with the appropriate errs.* error
type, preserving classification for the CLI while keeping the existing context
from launcher and skillsUpdateTimeout.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2131ed73-01ab-457d-9890-2e69dd33867e
📒 Files selected for processing (7)
README.mdREADME.zh.mdcmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.goscripts/install-wizard.js
✅ Files skipped from review due to trivial changes (4)
- README.md
- cmd/update/update.go
- README.zh.md
- internal/selfupdate/updater_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/update/update_test.go
- scripts/install-wizard.js
🛑 Comments failed to post (1)
internal/selfupdate/updater.go (1)
235-249: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use typed
errs.*errors for install failures.
RunPnpmInstallis part of the update command path, but it still returns rawfmt.Errorfvalues for missingpnpmand timeouts. As per coding guidelines, command-facing failures must use typederrs.*errors instead of barefmt.Errorf.🧰 Tools
🪛 ast-grep (0.44.1)
[error] 241-241: An argument passed to
exec.Command/exec.CommandContextis built by concatenating a string literal with dynamic input. If that input is attacker-controlled (and especially when the command is a shell such assh -c/bash -c), this enables OS command injection. Pass untrusted data as separate, fixed arguments instead of interpolating it into a command string, avoid invoking a shell, and validate/escape the input where a shell is unavoidable.
Context: exec.CommandContext(ctx, pnpmPath, "add", "-g", NpmPackage+"@"+version)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(command-injection-exec-concat-arg-go)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/selfupdate/updater.go` around lines 235 - 249, RunPnpmInstall is returning plain fmt.Errorf values for command-facing failures; switch those to the appropriate typed errs.* errors instead. Update the missing pnpm branch and the timeout branch in RunPnpmInstall so they construct the matching errs.* error types while preserving the underlying error/context, and keep the command execution flow unchanged.Source: Coding guidelines
1 participant
Summary
Install lark-cli agent skills with
skills add --copyso Claude Code receives real skill directories instead of symlinked entries it may fail to load.Changes
--copyto full and incremental skills sync commands used bylark-cli update.npx skills add larksuite/cli -y -g --copy.Test Plan
GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./internal/selfupdate -run TestSkillsCommandsUseExpectedArgs -count=1GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./cmd/update -run 'TestUpdateNpmVerifyFail_JSON_NoRestoreHintWhenBackupUnavailable|TestUpdate.*Skills|TestUpdate.*Manual' -count=1\n- [x]GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./internal/skillscheck -count=1\n- [x]git diff --check\n- [ ]GOCACHE=$PWD/.cache/go-build GOPROXY=https://goproxy.cn,direct go test ./internal/selfupdate ./internal/skillscheck -count=1(sandbox cannot bind httptest listener:operation not permitted; targeted selfupdate tests above passed)\n\n## Related Issues\n- Fixes cluade code skills中的lark-cli skills不要用软连接 #1491Summary by CodeRabbit
Bug Fixes
--copyoption, including scenarios for failed automatic updates and fallback reinstalls.npx/pnpmguidance with--copy.Documentation
npx skills add ... --copyformat.--copy.