Skip to content

feat: add --skills suite mode to update for partial skill install #1600

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
zhangheng023 wants to merge 7 commits into
base: main
Choose a base branch
from
Open

feat: add --skills suite mode to update for partial skill install #1600

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d4f458
feat: add SuiteSkills field to skills state
zhangheng023 Jun 26, 2026
d592022
feat: add ParseSuiteSelection for --skills flag parsing
zhangheng023 Jun 26, 2026
eab2063
feat: apply skills suite selection in SyncSkills
zhangheng023 Jun 26, 2026
95ee561
feat: add --skills flag to update for suite mode
zhangheng023 Jun 26, 2026
9e2cac6
chore: gofmt align SyncResult struct comment
zhangheng023 Jun 26, 2026
45399ca
docs: document update --skills suite mode in lark-shared
zhangheng023 Jun 26, 2026
7eabb27
fix: include hint in update JSON errors and explain skill-name charset
zhangheng023 Jun 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 67 additions & 19 deletions cmd/update/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,12 @@

// UpdateOptions holds inputs for the update command.
type UpdateOptions struct {
Factory *cmdutil.Factory
JSON bool
Force bool
Check bool
Factory *cmdutil.Factory
JSON bool
Force bool
Check bool
Skills []string
SuiteProvided bool
}

// NewCmdUpdate creates the update command.
Expand All @@ -108,13 +110,16 @@
Use --json for structured output (for AI agents and scripts).
Use --check to only check for updates without installing.`,
RunE: func(cmd *cobra.Command, args []string) error {
opts.SuiteProvided = cmd.Flags().Changed("skills")
return updateRun(opts)
},
}
cmdutil.DisableAuthCheck(cmd)
cmd.Flags().BoolVar(&opts.JSON, "json", false, "structured JSON output")
cmd.Flags().BoolVar(&opts.Force, "force", false, "force reinstall even if already up to date")
cmd.Flags().BoolVar(&opts.Check, "check", false, "only check for updates, do not install")
cmd.Flags().StringSliceVar(&opts.Skills, "skills", nil,
"comma-separated lark skill names to install and remember (the suite); use --skills all to reset to all official skills")
cmdutil.SetRisk(cmd, "high-risk-write")

return cmd
Expand All @@ -125,6 +130,18 @@
cur := currentVersion()
updater := newUpdater()

// 早期格式校验:在任何网络/安装动作之前 fail-fast。
var suite *skillscheck.SuiteSelection
if opts.SuiteProvided {
parsed, err := skillscheck.ParseSuiteSelection(opts.Skills)
if err != nil {
return reportError(opts, io, "validation",
errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", err).
WithHint("e.g. --skills lark-calendar,lark-im (or --skills all to reset)"))
Comment on lines +138 to +140

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🗄️ 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

}
suite = parsed
}

if !opts.Check {
updater.CleanupStaleFiles()
}
Expand All @@ -147,7 +164,10 @@
if !opts.Force && !update.IsNewer(latest, cur) {
var skillsResult *skillscheck.SyncResult
if !opts.Check {
skillsResult = runSkillsAndState(updater, io, cur, opts.Force)
skillsResult = runSkillsAndState(updater, io, cur, opts.Force, suite)
if err := suiteInputError(opts, io, skillsResult); err != nil {
return err
}
}
return reportAlreadyUpToDate(opts, io, cur, latest, skillsResult, opts.Check)
}
Expand All @@ -162,22 +182,26 @@

// 6. Execute update
if !detect.CanAutoUpdate() {
return doManualUpdate(opts, io, cur, latest, detect, updater)
return doManualUpdate(opts, io, cur, latest, detect, updater, suite)
}
return doNpmUpdate(opts, io, cur, latest, updater)
return doNpmUpdate(opts, io, cur, latest, updater, suite)
}

// --- Output helpers ---

// reportError emits the failure on the requested surface: JSON mode prints the
// {ok:false, error:{type, message}} envelope to stdout and signals the typed
// error's exit code bare; human mode returns the typed error for the
// dispatcher to render.
// {ok:false, error:{type, message, hint?}} envelope to stdout and signals the
// typed error's exit code bare; human mode returns the typed error for the
// dispatcher to render. The hint is included only when the typed error carries
// one, so AI-agent/script consumers reading JSON get the same actionable
// guidance humans see on stderr.
func reportError(opts *UpdateOptions, io *cmdutil.IOStreams, errType string, typedErr errs.TypedError) error {
if opts.JSON {
output.PrintJson(io.Out, map[string]interface{}{
"ok": false, "error": map[string]interface{}{"type": errType, "message": typedErr.ProblemDetail().Message},
})
errObj := map[string]interface{}{"type": errType, "message": typedErr.ProblemDetail().Message}
if hint := typedErr.ProblemDetail().Hint; hint != "" {
errObj["hint"] = hint
}
output.PrintJson(io.Out, map[string]interface{}{"ok": false, "error": errObj})
return output.ErrBare(output.ExitCodeOf(typedErr))
}
return typedErr
Expand Down Expand Up @@ -207,8 +231,11 @@
return nil
}

func doManualUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, detect selfupdate.DetectResult, updater *selfupdate.Updater) error {
skillsResult := runSkillsAndState(updater, io, cur, opts.Force)
func doManualUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, detect selfupdate.DetectResult, updater *selfupdate.Updater, suite *skillscheck.SuiteSelection) error {
skillsResult := runSkillsAndState(updater, io, cur, opts.Force, suite)
if err := suiteInputError(opts, io, skillsResult); err != nil {
return err

Check warning on line 237 in cmd/update/update.go

View check run for this annotation

Codecov / codecov/patch

cmd/update/update.go#L237 

Added line #L237 was not covered by tests
}

reason := detect.ManualReason()
if opts.JSON {
Expand All @@ -231,7 +258,7 @@
return nil
}

func doNpmUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, updater *selfupdate.Updater) error {
func doNpmUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, updater *selfupdate.Updater, suite *skillscheck.SuiteSelection) error {
restore, err := updater.PrepareSelfReplace()
if err != nil {
return reportError(opts, io, "update_error",
Expand Down Expand Up @@ -287,7 +314,10 @@
return output.ErrBare(output.ExitAPI)
}

skillsResult := runSkillsAndState(updater, io, latest, opts.Force)
skillsResult := runSkillsAndState(updater, io, latest, opts.Force, suite)
if err := suiteInputError(opts, io, skillsResult); err != nil {
return err

Check warning on line 319 in cmd/update/update.go

View check run for this annotation

Codecov / codecov/patch

cmd/update/update.go#L319 

Added line #L319 was not covered by tests
}
Comment on lines +317 to +320

@coderabbitai coderabbitai Bot Jun 26, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.


if opts.JSON {
result := map[string]interface{}{
Expand Down Expand Up @@ -324,8 +354,8 @@
return fmt.Sprintf("automatic rollback is unavailable on this platform; reinstall manually (skills will not be synced): npm install -g %s@%s && npx skills add larksuite/cli -y -g, or download %s", selfupdate.NpmPackage, latest, releaseURL(latest))
}

func runSkillsAndState(updater *selfupdate.Updater, io *cmdutil.IOStreams, stateVersion string, force bool) *skillscheck.SyncResult {
if !force {
func runSkillsAndState(updater *selfupdate.Updater, io *cmdutil.IOStreams, stateVersion string, force bool, suite *skillscheck.SuiteSelection) *skillscheck.SyncResult {
if !force && suite == nil {
if existing, ok := skillscheck.ReadSyncedVersion(); ok && normalizeVersion(existing) == normalizeVersion(stateVersion) {
return nil
}
Expand All @@ -334,13 +364,25 @@
Version: stateVersion,
Force: force,
Runner: updater,
Suite: suite,
})
if result.Err != nil && strings.Contains(result.Err.Error(), "state not written") {
fmt.Fprintf(io.ErrOut, "warning: %v\n", result.Err)
}
return result
}

// suiteInputError 把 suite 名字非法(InvalidInput)的 sync 结果映射为退出码 2 的 validation 错误。
// 返回 nil 表示不是输入错误,调用方继续正常输出流程。
func suiteInputError(opts *UpdateOptions, io *cmdutil.IOStreams, r *skillscheck.SyncResult) error {
if r == nil || !r.InvalidInput {
return nil
}
return reportError(opts, io, "validation",
errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", r.Err).
WithHint("use a valid official skill name; nothing was installed"))
}

// reportAlreadyUpToDate emits the JSON / pretty output for the
// already-up-to-date branch, including any skills_action / skills_warning
// fields derived from skillsResult. When check is true, this is the pure
Expand Down Expand Up @@ -402,6 +444,9 @@
env["skills_action"] = "synced"
env["skills_summary"] = skillsSummary(r)
}
if r != nil && len(r.Suite) > 0 {
env["skills_suite"] = r.Suite
}
}

func skillsSummary(r *skillscheck.SyncResult) map[string]interface{} {
Expand Down Expand Up @@ -434,4 +479,7 @@
fmt.Fprintf(io.ErrOut, " To restore all official skills: lark-cli update --force\n")
}
}
if r != nil && len(r.Suite) > 0 {
fmt.Fprintf(io.ErrOut, " Suite: %s (run `lark-cli update --skills all` to restore all)\n", strings.Join(r.Suite, ", "))
}
}
Loading
Loading