fix: sync skills incrementally during update#1042
Open
zhangheng023 wants to merge 2 commits into
Open
Conversation
zhangheng023
added
bug
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughMigrate skills persistence from a single-line stamp file to JSON ChangesSkills State and Synchronization Overhaul
Sequence Diagram(s)sequenceDiagram
participant UpdateCmd as update command
participant RunSkillsAndState
participant SyncSkills as SyncSkills orchestration
participant Runner as Updater
participant StateFile as skills-state.json
UpdateCmd->>RunSkillsAndState: targetVersion
alt Dedup hit (synced version matches)
RunSkillsAndState-->>UpdateCmd: return prior state, skip sync
else Need sync (cold start or new version)
RunSkillsAndState->>SyncSkills: SyncOptions{version, runner}
SyncSkills->>Runner: ListOfficialSkills()
Runner-->>SyncSkills: NpmResult(stdout)
SyncSkills->>SyncSkills: ParseSkillsList → plan
SyncSkills->>Runner: InstallSkill(ToUpdate) or InstallAllSkills()
Runner-->>SyncSkills: result
SyncSkills->>StateFile: WriteState(updated version, skills lists)
StateFile-->>SyncSkills: persisted
SyncSkills-->>RunSkillsAndState: SyncResult
end
RunSkillsAndState-->>UpdateCmd: SyncResult or nil
UpdateCmd->>UpdateCmd: emit JSON/text via applySkillsStatus or emitSkillsTextHints
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 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 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@ead4f9769a0827cfb4175a3e3fac40609a427d62🧩 Skill updatenpx skills add larksuite/cli#fix/incremental-skills-update -y -g |
![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
🤖 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/skillscheck/state.go`:
- Around line 46-54: ReadState currently accepts JSON with extra/unexpected
fields because it uses json.Unmarshal into SkillsState; change the second
unmarshal to use a json.Decoder with DisallowUnknownFields so unknown fields
cause an error. Specifically, after the initial syntax check (the first
json.Unmarshal into raw), create a decoder from the input bytes (e.g.,
bytes.NewReader(data)), call decoder.DisallowUnknownFields(), then
decoder.Decode(&state) into the SkillsState and return ErrUnreadableState on
decode errors; ensure errors reference ErrUnreadableState where currently
returned.
🪄 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: b579cd2e-e130-4a76-a940-42a58c44413e
📒 Files selected for processing (14)
cmd/root_integration_test.gocmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/stamp.gointernal/skillscheck/stamp_test.gointernal/skillscheck/state.gointernal/skillscheck/state_test.gointernal/skillscheck/sync.gointernal/skillscheck/sync_test.go
💤 Files with no reviewable changes (2)
- internal/skillscheck/stamp_test.go
- internal/skillscheck/stamp.go
Comment on lines
+46
to
+54
| var raw map[string]interface{} | ||
| if err := json.Unmarshal(data, &raw); err != nil { | ||
| return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err) | ||
| } | ||
|
|
||
| var state SkillsState | ||
| if err := json.Unmarshal(data, &state); err != nil { | ||
| return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err) | ||
| } |
There was a problem hiding this comment.
Unknown-schema state files are currently treated as valid.
Line 46 validates JSON syntax, but not schema. A payload with unexpected fields (for example schema_version) still unmarshals into SkillsState, so ReadState() returns ok=true instead of ErrUnreadableState.
💡 Proposed fix (strict decode with unknown-field rejection)
import (
+ "bytes"
"encoding/json"
"errors"
"fmt"
+ "io"
"io/fs"
"path/filepath"
@@
- var raw map[string]interface{}
- if err := json.Unmarshal(data, &raw); err != nil {
- return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err)
- }
-
var state SkillsState
- if err := json.Unmarshal(data, &state); err != nil {
+ dec := json.NewDecoder(bytes.NewReader(data))
+ dec.DisallowUnknownFields()
+ if err := dec.Decode(&state); err != nil {
+ return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err)
+ }
+ if err := dec.Decode(&struct{}{}); err != io.EOF {
return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err)
}
return &state, true, nil
}📝 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
| var raw map[string]interface{} | |
| if err := json.Unmarshal(data, &raw); err != nil { | |
| return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err) | |
| } | |
| var state SkillsState | |
| if err := json.Unmarshal(data, &state); err != nil { | |
| return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err) | |
| } | |
| var state SkillsState | |
| dec := json.NewDecoder(bytes.NewReader(data)) | |
| dec.DisallowUnknownFields() | |
| if err := dec.Decode(&state); err != nil { | |
| return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err) | |
| } | |
| if err := dec.Decode(&struct{}{}); err != io.EOF { | |
| return nil, false, fmt.Errorf("%w: %v", ErrUnreadableState, err) | |
| } | |
| return &state, true, nil | |
| } |
🤖 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/skillscheck/state.go` around lines 46 - 54, ReadState currently
accepts JSON with extra/unexpected fields because it uses json.Unmarshal into
SkillsState; change the second unmarshal to use a json.Decoder with
DisallowUnknownFields so unknown fields cause an error. Specifically, after the
initial syntax check (the first json.Unmarshal into raw), create a decoder from
the input bytes (e.g., bytes.NewReader(data)), call
decoder.DisallowUnknownFields(), then decoder.Decode(&state) into the
SkillsState and return ErrUnreadableState on decode errors; ensure errors
reference ErrUnreadableState where currently returned.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1042 +/- ##
==========================================
+ Coverage 67.75% 67.82% +0.07%
==========================================
Files 590 591 +1
Lines 55188 55467 +279
==========================================
+ Hits 37392 37622 +230
- Misses 14684 14712 +28
- Partials 3112 3133 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
zhangheng023
force-pushed
the
fix/incremental-skills-update
branch
from
2b24d66 to
b411e4c
Compare
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)
cmd/update/update.go (1)
39-44:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winHandle uppercase
Vin version normalization too.
normalizeVersiononly strips lowercasev, so values likeV1.0.21can be treated as out-of-sync in this command path.Suggested patch
func normalizeVersion(s string) string { - return strings.TrimPrefix(strings.TrimSpace(s), "v") + s = strings.TrimSpace(s) + s = strings.TrimPrefix(s, "v") + return strings.TrimPrefix(s, "V") }🤖 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 39 - 44, normalizeVersion currently only removes a lowercase "v" prefix so inputs like "V1.0.21" remain unchanged; update the normalizeVersion function to strip either "v" or "V" (e.g., check the first rune and remove it if it equals 'v' or 'V', or use a case-insensitive trim) after trimming whitespace so both "v1.2.3" and "V1.2.3" normalize to "1.2.3".
🤖 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/skillscheck/sync.go`:
- Around line 254-257: The code currently treats empty/nil from ParseSkillsList
as “no local skills” which can silently mis-plan syncs; update the block that
calls opts.Runner.ListGlobalSkills() so that after verifying localResult != nil
&& localResult.Err == nil you parse stdout into parsed :=
ParseSkillsList(localResult.Stdout.String()) and then detect an unparsable case
(stdout is non-empty but parsed is nil/empty) and in that case mark it as
unparsable and fall back to the full-install path (e.g., set local to nil or set
the same fallback flag used by the official-list handling) instead of treating
it as no skills; keep using the same symbols ParseSkillsList, localResult,
opts.Runner.ListGlobalSkills and the local variable so the planner will choose
full install when parsing fails.
---
Outside diff comments:
In `@cmd/update/update.go`:
- Around line 39-44: normalizeVersion currently only removes a lowercase "v"
prefix so inputs like "V1.0.21" remain unchanged; update the normalizeVersion
function to strip either "v" or "V" (e.g., check the first rune and remove it if
it equals 'v' or 'V', or use a case-insensitive trim) after trimming whitespace
so both "v1.2.3" and "V1.2.3" normalize to "1.2.3".
🪄 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: 3a049394-a990-42e9-944f-00eed52d7298
📒 Files selected for processing (14)
cmd/root_integration_test.gocmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/skillscheck/check.gointernal/skillscheck/check_test.gointernal/skillscheck/notice.gointernal/skillscheck/stamp.gointernal/skillscheck/stamp_test.gointernal/skillscheck/state.gointernal/skillscheck/state_test.gointernal/skillscheck/sync.gointernal/skillscheck/sync_test.go
💤 Files with no reviewable changes (2)
- internal/skillscheck/stamp_test.go
- internal/skillscheck/stamp.go
✅ Files skipped from review due to trivial changes (1)
- internal/skillscheck/notice.go
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
This PR makes
lark-cli updatesync bundled skills incrementally instead of reinstalling every official skill. It records official skill state, updates installed and newly added official skills, preserves intentionally deleted skills, and falls back to a full sync when incremental discovery fails.Changes
cmd/update/update.goto run skill sync state handling during update flows and include skill sync status in human and JSON output.internal/skillscheck/sync.goandinternal/skillscheck/state.go.internal/selfupdate/updater.gowith skill list/install command helpers for official, global, selected, and full skill sync operations.cmd/update/update_test.go,internal/skillscheck/*_test.go, andinternal/selfupdate/updater_test.go.Test Plan
make unit-testnot rerun during PR drafting; branch already contains committed test updatesRelated Issues
N/A
Summary by CodeRabbit
Refactor
New Features
Tests