fix: sync skills incrementally during update#1042
Merged
Merged
Conversation
zhangheng023
added
bug
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 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, Force, Runner}
SyncSkills->>Runner: ListOfficialSkills()
Runner-->>SyncSkills: NpmResult(stdout)
SyncSkills->>SyncSkills: ParseSkillsList → PlanSync
alt Incremental install available
SyncSkills->>Runner: InstallSkill(ToUpdate)
Runner-->>SyncSkills: NpmResult
else Fall back to full
SyncSkills->>Runner: InstallAllSkills()
Runner-->>SyncSkills: NpmResult
end
SyncSkills->>StateFile: WriteState(updated version, 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 reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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@136dba2203d8a41da9eda389f37ca524e6319fd2🧩 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
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1042 +/- ##
==========================================
+ Coverage 67.75% 67.99% +0.23%
==========================================
Files 590 604 +14
Lines 55188 56057 +869
==========================================
+ Hits 37392 38115 +723
- Misses 14684 14785 +101
- Partials 3112 3157 +45 ☔ 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
zhangheng023
force-pushed
the
fix/incremental-skills-update
branch
from
ead4f97 to
136dba2
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
internal/skillscheck/state.go (1)
46-54:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject unknown fields when decoding skills state JSON.
ReadStatecurrently accepts JSON documents with extra fields, so incompatible schema changes can be treated as valid state instead of unreadable state.Suggested fix
import ( + "bytes" "encoding/json" "errors" "fmt" + "io" "io/fs" "path/filepath" @@ - var state SkillsState - if err := json.Unmarshal(data, &state); err != nil { + 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 allows extra JSON fields because it unmarshals into a map first and then into SkillsState; instead decode strictly by creating a json.Decoder over the data (e.g., json.NewDecoder(bytes.NewReader(data))), call decoder.DisallowUnknownFields(), and decode directly into the SkillsState variable (SkillsState) so any unknown fields produce an error you wrap with ErrUnreadableState; remove the initial raw map unmarshal and ensure the error path still returns nil, false, and fmt.Errorf("%w: %v", ErrUnreadableState, err).
🤖 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_test.go`:
- Around line 1265-1286: The live integration test
TestUpdateCommand_RealSkillsSyncRewritesState currently runs whenever npx is
present, making CI flaky; update the test to be opt-in by adding a gating check
at the start (before exec.LookPath): skip unless either testing.Short() is false
AND an explicit env var is set (for example require
os.Getenv("RUN_LIVE_NPX_SKILLS_TESTS") == "1"), or simply require the env var;
apply the same gating change to the other live npx skills test referenced in the
diff (the second Test* block around lines 1378-1401) so both tests only run when
the env var is present (and optionally when not testing.Short()).
- Around line 44-53: The tests that don't exercise the live skills command are
missing a stub for SkillsCommandOverride, so updateRun can call
runSkillsAndState and accidentally execute a real npx skills; modify the test
helper(s) that build the test updater (e.g., mockDetectAndNpm and any similar
helper that assigns newUpdater) to always set u.SkillsCommandOverride =
successfulSkillsCommand() on the returned *selfupdate.Updater (and ensure
newUpdater replacement used by TestUpdateAlreadyUpToDate_* and
TestUpdateManual_* goes through that same helper), so every non-live test uses
the stubbed skills command instead of invoking the real binary.
In `@cmd/update/update.go`:
- Around line 380-381: The status map currently sets status["skipped_deleted"] =
state.SkippedDeletedSkills (a []string) which breaks JSON consumers expecting
counts for skills_status; change this to emit the count instead, e.g.
status["skipped_deleted"] = len(state.SkippedDeletedSkills), so the key remains
a numeric count like "official" and "updated" (alternatively, if you prefer to
keep the slice, rename the key to "skipped_deleted_skills" and leave the slice
value). Update the assignment in the same block where status is populated
(referencing state.SkippedDeletedSkills and the status map) accordingly.
In `@internal/skillscheck/sync.go`:
- Around line 329-338: The current branch returns a SyncResult with Action
"fallback_synced" when WriteState(state) fails, masking the write failure;
update the error handling in the WriteState error path inside the function that
constructs SyncResult so that instead of returning a successful
"fallback_synced" result with no Err, you return a SyncResult that includes the
write error (set Err to writeErr) and/or change the control flow to propagate
the writeErr upward (return nil, writeErr) so the caller sees the persistence
failure; locate the block that calls WriteState(state) and constructs the
SyncResult and ensure Err is populated (or the function returns the error) when
WriteState returns an error.
---
Duplicate comments:
In `@internal/skillscheck/state.go`:
- Around line 46-54: ReadState currently allows extra JSON fields because it
unmarshals into a map first and then into SkillsState; instead decode strictly
by creating a json.Decoder over the data (e.g.,
json.NewDecoder(bytes.NewReader(data))), call decoder.DisallowUnknownFields(),
and decode directly into the SkillsState variable (SkillsState) so any unknown
fields produce an error you wrap with ErrUnreadableState; remove the initial raw
map unmarshal and ensure the error path still returns nil, false, and
fmt.Errorf("%w: %v", ErrUnreadableState, err).
🪄 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: 5c034d82-e411-4461-a639-96a8aebcaba4
📒 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.go
- internal/skillscheck/stamp_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/skillscheck/notice.go
MaxHuang22
approved these changes
May 26, 2026
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
2 participants
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