feat: add --skills suite mode to update for partial skill install#1600
feat: add --skills suite mode to update for partial skill install#1600zhangheng023 wants to merge 7 commits into
Conversation
📝 WalkthroughWalkthroughThe update command now accepts an explicit ChangesSuite-aware update flow
Sequence Diagram(s)sequenceDiagram
participant updateRun
participant runSkillsAndState
participant SyncSkills
participant reportError
updateRun->>runSkillsAndState: pass parsed suite selection
runSkillsAndState->>SyncSkills: SyncOptions{Suite: suite}
SyncSkills-->>runSkillsAndState: SyncResult{Suite, InvalidInput}
runSkillsAndState-->>updateRun: result or validation error
updateRun->>reportError: emit JSON hint when InvalidInput is set
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@7eabb27272cd9eb24ec1043d58735e4ecddb760e🧩 Skill updatenpx skills add larksuite/cli#feat/update-skills-suite-mode -y -g |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1600 +/- ##
==========================================
- Coverage 74.74% 74.67% -0.08%
==========================================
Files 799 804 +5
Lines 80380 81145 +765
==========================================
+ Hits 60084 60595 +511
- Misses 15849 16043 +194
- Partials 4447 4507 +60 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/update/update_test.go (1)
1663-1712: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the invalid-suite path before side effects, and assert typed metadata.
These tests only exercise JSON / already-up-to-date flows, so they would not catch the update-available case where
RunNpmInstallhappens before suite validation. They also only assert exit code / JSON substrings. Please add a companion non-JSON case withcurrentVersion < latest, assert npm install is never invoked, then verify subtype/category viaerrs.ProblemOfandParamviaerrors.As(*errs.ValidationError).As per coding guidelines,
**/*_test.goerror-path tests must assert typed metadata and cause preservation. Based on learnings, useerrors.AsforValidationError.Parambecauseerrs.ProblemOfdoes not exposeParam.🤖 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 `@cmd/update/update_test.go` around lines 1663 - 1712, The current update tests only cover JSON and up-to-date paths, so they miss the update-available flow where `RunNpmInstall` can happen before suite validation. Add a non-JSON test around `NewCmdUpdate` with `currentVersion < fetchLatest` that verifies npm install is not invoked, then assert the validation subtype/category using `errs.ProblemOf`. Also check the `Param` on the resulting `errs.ValidationError` with `errors.As`, since `errs.ProblemOf` does not expose it, and keep the cause/error chain preserved.Sources: Coding guidelines, Learnings
🤖 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 `@cmd/update/update.go`:
- Around line 138-140: The validation error in update handling is missing both
the flag metadata and the wrapped cause. In the branches using reportError in
update.go, update the errs.NewValidationError call to attach the user-facing
parameter with WithParam("--skills") and preserve the original failure with
WithCause(err) before adding the hint. Make the same change in the other
matching validation branch so the typed error contract and error unwrapping stay
intact.
- Around line 317-320: Reject unknown or non-official --skills values before any
npm update side effects occur. In update.go, move the official-name validation
out of suiteInputError/runSkillsAndState flow so invalid skills are checked
before RunNpmInstall and VerifyBinary can run, and make sure update() returns
the validation error immediately for cases like lark-bogus. Keep the validation
tied to the existing skill/suite helpers used by suiteInputError and
runSkillsAndState, but split the preflight check from the sync/install
execution.
---
Nitpick comments:
In `@cmd/update/update_test.go`:
- Around line 1663-1712: The current update tests only cover JSON and up-to-date
paths, so they miss the update-available flow where `RunNpmInstall` can happen
before suite validation. Add a non-JSON test around `NewCmdUpdate` with
`currentVersion < fetchLatest` that verifies npm install is not invoked, then
assert the validation subtype/category using `errs.ProblemOf`. Also check the
`Param` on the resulting `errs.ValidationError` with `errors.As`, since
`errs.ProblemOf` does not expose it, and keep the cause/error chain preserved.
🪄 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: 07d6e86f-8aa2-4108-a776-4f3d59f272da
📒 Files selected for processing (7)
cmd/update/update.gocmd/update/update_test.gointernal/skillscheck/state.gointernal/skillscheck/state_test.gointernal/skillscheck/sync.gointernal/skillscheck/sync_test.goskills/lark-shared/SKILL.md
| return reportError(opts, io, "validation", | ||
| errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err). | ||
| WithHint("e.g. --skills lark-calendar,lark-im (or --skills all to reset)")) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Attach --skills metadata and preserve the underlying cause.
Both branches are surfacing user-input validation, but they currently drop the flag name and wrapped error. That weakens the typed error contract and loses errors.Is / errors.Unwrap on the original failure.
Suggested fix
- return reportError(opts, io, "validation",
- errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err).
- WithHint("e.g. --skills lark-calendar,lark-im (or --skills all to reset)"))
+ return reportError(opts, io, "validation",
+ errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err).
+ WithParam("--skills").
+ WithHint("e.g. --skills lark-calendar,lark-im (or --skills all to reset)").
+ WithCause(err))
@@
- return reportError(opts, io, "validation",
- errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", r.Err).
- WithHint("use a valid official skill name; nothing was installed"))
+ return reportError(opts, io, "validation",
+ errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", r.Err).
+ WithParam("--skills").
+ WithHint("use a valid official skill name; nothing was installed").
+ WithCause(r.Err))As per coding guidelines, cmd/**/*.go user flag validation must use .WithParam("--flag"), and **/*.go errors must preserve underlying failures with .WithCause(err).
Also applies to: 381-383
🤖 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 `@cmd/update/update.go` around lines 138 - 140, The validation error in update
handling is missing both the flag metadata and the wrapped cause. In the
branches using reportError in update.go, update the errs.NewValidationError call
to attach the user-facing parameter with WithParam("--skills") and preserve the
original failure with WithCause(err) before adding the hint. Make the same
change in the other matching validation branch so the typed error contract and
error unwrapping stay intact.
Source: Coding guidelines
| skillsResult := runSkillsAndState(updater, io, latest, opts.Force, suite) | ||
| if err := suiteInputError(opts, io, skillsResult); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Reject unknown --skills before upgrading the CLI.
In the npm path, suiteInputError runs only after RunNpmInstall and VerifyBinary, so lark-cli update --skills lark-bogus can still replace the CLI and then exit 2. That breaks the new contract that invalid/non-official skill names fail validation without installing anything. Preflight the official-name check before npm update side effects, or split suite validation from sync execution so unknown names are rejected first.
🤖 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 `@cmd/update/update.go` around lines 317 - 320, Reject unknown or non-official
--skills values before any npm update side effects occur. In update.go, move the
official-name validation out of suiteInputError/runSkillsAndState flow so
invalid skills are checked before RunNpmInstall and VerifyBinary can run, and
make sure update() returns the validation error immediately for cases like
lark-bogus. Keep the validation tied to the existing skill/suite helpers used by
suiteInputError and runSkillsAndState, but split the preflight check from the
sync/install execution.
1 participant
Summary
lark-cli updatepreviously always synced all official skills. This PR adds a--skillsflag so users can install only a chosen subset (a "suite"), which is remembered (sticky) inskills-state.json— subsequent plainupdateruns sync only that subset.--skills allresets back to all official skills.Changes
--skillsflag, early format validation, suite plumbing, version-early-return bypass,skills_suiteoutput, and exit-2 validation errors tocmd/update/update.goSuiteSelection/ParseSuiteSelection,SyncOptions.Suite,SyncResult.Suite/InvalidInput, and suite resolution / official-name validation / set narrowing / no-fallback / persistence tointernal/skillscheck/sync.gosuite_skills(omitempty, backward compatible) field tointernal/skillscheck/state.goskills/lark-shared/SKILL.mdTest Plan
make unit-testpassedupdatecommand has no existing E2E caseslark-cli update --help, andlark-cli update --skills ""/all,lark-im/"bad name"(text + JSON) — all exit 2 with a clear message and hintRelated Issues
N/A
Summary by CodeRabbit
New Features
lark-cli updatenow supports--skillsto install or update only a selected subset of official skills.--skills allrestores updating all skills.Bug Fixes