fix: sync skills incrementally during update

cmd/root_integration_test.go

@@ -536,11 +536,8 @@ func TestIntegration_Shortcut_BusinessError_OutputsEnvelope(t *testing.T) {
 	})
 }
 
-// TestSetupNotices_ColdStart_NoNotice verifies that a missing stamp
-// produces no skills key in the composed notice. Users who installed
-// skills via `npx skills add` (no stamp) must not see the misleading
-// "not installed" notice — only `lark-cli update` users opt into the
-// drift tracker.
+// TestSetupNotices_ColdStart_NoNotice verifies that missing state
+// produces no skills key in the composed notice.
 func TestSetupNotices_ColdStart_NoNotice(t *testing.T) {
 	clearNoticeEnv(t)
 	dir := t.TempDir()
@@ -571,13 +568,13 @@ func TestSetupNotices_ColdStart_NoNotice(t *testing.T) {
 	}
 }
 
-// TestSetupNotices_InSync verifies that a matching stamp produces no
+// TestSetupNotices_InSync verifies that matching state produces no
 // skills key in the composed notice.
 func TestSetupNotices_InSync(t *testing.T) {
 	clearNoticeEnv(t)
 	dir := t.TempDir()
 	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir)
-	if err := skillscheck.WriteStamp("1.0.21"); err != nil {
+	if err := skillscheck.WriteState(skillscheck.SkillsState{Version: "1.0.21"}); err != nil {
 		t.Fatal(err)
 	}
 
@@ -604,13 +601,13 @@ func TestSetupNotices_InSync(t *testing.T) {
 	}
 }
 
-// TestSetupNotices_Drift verifies a mismatching stamp produces the
+// TestSetupNotices_Drift verifies mismatching state produces the
 // drift message with both current and target populated.
 func TestSetupNotices_Drift(t *testing.T) {
 	clearNoticeEnv(t)
 	dir := t.TempDir()
 	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir)
-	if err := skillscheck.WriteStamp("1.0.20"); err != nil {
+	if err := skillscheck.WriteState(skillscheck.SkillsState{Version: "1.0.20"}); err != nil {
 		t.Fatal(err)
 	}
 
@@ -659,7 +656,7 @@ func TestSetupNotices_BothUpdateAndSkills(t *testing.T) {
 	clearNoticeEnv(t)
 	dir := t.TempDir()
 	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", dir)
-	if err := skillscheck.WriteStamp("1.0.20"); err != nil {
+	if err := skillscheck.WriteState(skillscheck.SkillsState{Version: "1.0.20"}); err != nil {
 		t.Fatal(err)
 	}
 

cmd/update/update.go

@@ -31,15 +31,18 @@
 	currentVersion = func() string { return build.Version }
 	currentOS      = runtime.GOOS
 	newUpdater     = func() *selfupdate.Updater { return selfupdate.New() }
+	syncSkills     = func(opts skillscheck.SyncOptions) *skillscheck.SyncResult { return skillscheck.SyncSkills(opts) }
 )
 
 func isWindows() bool { return currentOS == osWindows }
 
-// normalizeVersion canonicalizes a version string for stamp comparison.
+// normalizeVersion canonicalizes a version string for state comparison.
 // Strips a leading "v" so versions written from Makefile (git describe →
 // "v1.0.0") and npm (no prefix → "1.0.0") compare equal.
 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")
 }
 
 func releaseURL(version string) string {
@@ -121,7 +124,9 @@
 	cur := currentVersion()
 	updater := newUpdater()
 
-	updater.CleanupStaleFiles()
+	if !opts.Check {
+		updater.CleanupStaleFiles()
+	}
 	output.PendingNotice = nil
 
 	// 1. Fetch latest version
@@ -137,13 +142,9 @@
 
 	// 3. Compare versions
 	if !opts.Force && !update.IsNewer(latest, cur) {
-		// Run skills sync before returning — covers the case where the
-		// binary is already current but skills were never synced.
-		// Stamp dedup makes this a no-op if skills are already in sync.
-		// Skip side-effects under --check (pure report path per spec §3.6).
-		var skillsResult *selfupdate.NpmResult
+		var skillsResult *skillscheck.SyncResult
 		if !opts.Check {
-			skillsResult = runSkillsAndStamp(updater, io, cur, opts.Force)
+			skillsResult = runSkillsAndState(updater, io, cur, opts.Force)
 		}
 		return reportAlreadyUpToDate(opts, io, cur, latest, skillsResult, opts.Check)
 	}
@@ -185,16 +186,7 @@
 			"message":     fmt.Sprintf("lark-cli %s %s %s available", cur, symArrow(), latest),
 			"url":         releaseURL(latest), "changelog": changelogURL(),
 		}
-		// skills_status: pure report, no side effect, no stamp write.
-		// ReadStamp errors are silently swallowed — if we can't read the
-		// stamp we just omit the block rather than fail the --check.
-		if stamp, err := skillscheck.ReadStamp(); err == nil {
-			out["skills_status"] = map[string]interface{}{
-				"current": stamp,
-				"target":  cur,
-				"in_sync": stamp == cur,
-			}
-		}
+		applySkillsStatus(out, cur)
 		output.PrintJson(io.Out, out)
 		return nil
 	}
@@ -210,7 +202,7 @@
 }
 
 func doManualUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, detect selfupdate.DetectResult, updater *selfupdate.Updater) error {
-	skillsResult := runSkillsAndStamp(updater, io, cur, opts.Force)
+	skillsResult := runSkillsAndState(updater, io, cur, opts.Force)
 
 	reason := detect.ManualReason()
 	if opts.JSON {
@@ -288,10 +280,7 @@
 		return output.ErrBare(output.ExitAPI)
 	}
 
-	// Skills update (best-effort) — uses runSkillsAndStamp so the
-	// stamp gets persisted on success and dedup applies if a previous
-	// run already stamped this version.
-	skillsResult := runSkillsAndStamp(updater, io, latest, opts.Force)
+	skillsResult := runSkillsAndState(updater, io, latest, opts.Force)
 
 	if opts.JSON {
 		result := map[string]interface{}{
@@ -328,52 +317,37 @@
 	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))
 }
 
-// runSkillsAndStamp triggers updater.RunSkillsUpdate and persists the
-// stamp on success. Skips the npx invocation when the stamp already
-// matches stampVersion (unless force is true). The stamp write failure
-// emits a warning to io.ErrOut but does NOT fail the update command —
-// best-effort. ReadStamp errors are swallowed (fail-closed: treated as
-// out-of-sync, so npx re-runs). Returns nil iff skipped due to stamp
-// dedup; otherwise returns the underlying *NpmResult with Err semantics
-// from RunSkillsUpdate.
-func runSkillsAndStamp(updater *selfupdate.Updater, io *cmdutil.IOStreams, stampVersion string, force bool) *selfupdate.NpmResult {
+func runSkillsAndState(updater *selfupdate.Updater, io *cmdutil.IOStreams, stateVersion string, force bool) *skillscheck.SyncResult {
 	if !force {
-		if existing, _ := skillscheck.ReadStamp(); normalizeVersion(existing) == normalizeVersion(stampVersion) {
+		if existing, ok := skillscheck.ReadSyncedVersion(); ok && normalizeVersion(existing) == normalizeVersion(stateVersion) {
 			return nil
 		}
 	}
-	r := updater.RunSkillsUpdate()
-	if r.Err == nil {
-		if err := skillscheck.WriteStamp(stampVersion); err != nil {
-			fmt.Fprintf(io.ErrOut, "warning: skills synced but stamp not written: %v\n", err)
-		}
+	result := syncSkills(skillscheck.SyncOptions{
+		Version: stateVersion,
+		Force:   force,
+		Runner:  updater,
+	})
+	if result.Err != nil && strings.Contains(result.Err.Error(), "state not written") {
+		fmt.Fprintf(io.ErrOut, "warning: %v\n", result.Err)
 	}
-	return r
+	return result
 }
 
 // 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
 // report path (spec §3.6): no side-effects, JSON envelope uses
 // skills_status (spec §4.2) instead of skills_action.
-func reportAlreadyUpToDate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, skillsResult *selfupdate.NpmResult, check bool) error {
+func reportAlreadyUpToDate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string, skillsResult *skillscheck.SyncResult, check bool) error {
 	if opts.JSON {
 		out := map[string]interface{}{
 			"ok": true, "previous_version": cur, "current_version": cur,
 			"latest_version": latest, "action": "already_up_to_date",
 			"message": fmt.Sprintf("lark-cli %s is already up to date", cur),
 		}
 		if check {
-			// Pure report — read stamp directly, emit skills_status block.
-			// ReadStamp errors are silently swallowed — if we can't read
-			// the stamp we just omit the block rather than fail the --check.
-			if stamp, err := skillscheck.ReadStamp(); err == nil {
-				out["skills_status"] = map[string]interface{}{
-					"current": stamp,
-					"target":  cur,
-					"in_sync": stamp == cur,
-				}
-			}
+			applySkillsStatus(out, cur)
 		} else {
 			applySkillsResult(out, skillsResult)
 		}
@@ -387,36 +361,70 @@
 	return nil
 }
 
-// applySkillsResult mutates the JSON envelope to include skills_action
-// (and skills_warning when failed). nil result = "in_sync" (dedup hit).
-func applySkillsResult(env map[string]interface{}, r *selfupdate.NpmResult) {
+func applySkillsStatus(env map[string]interface{}, target string) {
+	state, readable, err := skillscheck.ReadState()
+	if err != nil || !readable || state.Version == "" {
+		return
+	}
+	status := map[string]interface{}{
+		"current": state.Version,
+		"target":  target,
+		"in_sync": normalizeVersion(state.Version) == normalizeVersion(target),
+	}
+	if len(state.OfficialSkills) > 0 {
+		status["official"] = len(state.OfficialSkills)
+	}
+	if len(state.UpdatedSkills) > 0 {
+		status["updated"] = len(state.UpdatedSkills)
+	}
+	if len(state.SkippedDeletedSkills) > 0 {
+		status["skipped_deleted"] = state.SkippedDeletedSkills
+	}
+	env["skills_status"] = status
+}
+
+func applySkillsResult(env map[string]interface{}, r *skillscheck.SyncResult) {
 	switch {
 	case r == nil:
 		env["skills_action"] = "in_sync"
 	case r.Err != nil:
 		env["skills_action"] = "failed"
 		env["skills_warning"] = fmt.Sprintf("skills update failed: %s", r.Err)
-		if detail := strings.TrimSpace(r.Stderr.String()); detail != "" {
-			env["skills_detail"] = selfupdate.Truncate(detail, maxNpmOutput)
-		}
+		env["skills_summary"] = skillsSummary(r)
 	default:
 		env["skills_action"] = "synced"
+		env["skills_summary"] = skillsSummary(r)
+	}
+}
+
+func skillsSummary(r *skillscheck.SyncResult) map[string]interface{} {
+	summary := map[string]interface{}{
+		"official":        len(r.Official),
+		"updated":         len(r.Updated),
+		"added":           len(r.Added),
+		"skipped_deleted": len(r.SkippedDeleted),
 	}
+	if len(r.Failed) > 0 {
+		summary["failed"] = r.Failed
+	}
+	return summary
 }
 
-// emitSkillsTextHints prints human-readable feedback about the skills
-// sync result for non-JSON output.
-func emitSkillsTextHints(io *cmdutil.IOStreams, r *selfupdate.NpmResult) {
+func emitSkillsTextHints(io *cmdutil.IOStreams, r *skillscheck.SyncResult) {
 	switch {
 	case r == nil:
-		// dedup hit — silent (already up to date)
 	case r.Err != nil:
 		fmt.Fprintf(io.ErrOut, "%s Skills update failed: %v\n", symWarn(), r.Err)
-		if detail := strings.TrimSpace(r.Stderr.String()); detail != "" {
-			fmt.Fprintf(io.ErrOut, "  %s\n", selfupdate.Truncate(detail, maxStderrDetail))
+		if len(r.Failed) > 0 {
+			fmt.Fprintf(io.ErrOut, "  Failed skills: %s\n", strings.Join(r.Failed, ", "))
 		}
-		fmt.Fprintf(io.ErrOut, "  Run manually: npx -y skills add larksuite/cli -g -y\n")
+		fmt.Fprintf(io.ErrOut, "  To retry all official skills: lark-cli update --force\n")
+	case r.Force:
+		fmt.Fprintf(io.ErrOut, "%s Skills updated: restored all %d official skills\n", symOK(), len(r.Official))
 	default:
-		fmt.Fprintf(io.ErrOut, "%s Skills updated\n", symOK())
+		fmt.Fprintf(io.ErrOut, "%s Skills updated: %d official, %d updated, %d added, %d skipped because deleted locally\n", symOK(), len(r.Official), len(r.Updated), len(r.Added), len(r.SkippedDeleted))
+		if len(r.SkippedDeleted) > 0 {
+			fmt.Fprintf(io.ErrOut, "  To restore all official skills: lark-cli update --force\n")
+		}
 	}
 }