fix: simplify skills sync to stamp-based approach

[MaxHuang22]

Summary

Remove the incremental skills sync mechanism and restore the simpler stamp-based approach. The incremental sync added complexity (state.json, per-skill install, plan/diff logic) without sufficient benefit over the straightforward full sync via npx skills add.

Changes

• Restore internal/skillscheck/stamp.go — simple version stamp read/write
• Remove internal/skillscheck/state.go, sync.go and their tests — incremental sync logic
• Simplify internal/skillscheck/check.go to use stamp-based drift detection
• Revert cmd/update/update.go to use runSkillsAndStamp instead of runSkillsAndState
• Remove per-skill install methods from internal/selfupdate/updater.go
• Simplify scripts/install-wizard.js to full sync instead of incremental plan

Test Plan

• skipped: CI will validate

Related Issues

N/A

Summary by CodeRabbit

• New Features

  • Skills synchronization now occurs during updates even when the binary version is already current.

• Improvements

  • Simplified skills tracking mechanism for more reliable management.
  • Enhanced cleanup of stale files during update operations.
  • Installation wizard streamlined with reduced complexity.

• Tests

  • Refactored integration and unit tests to align with updated architecture.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR migrates skills version tracking from a complex SkillsState JSON model to a lightweight skills.stamp file, simplifying drift detection and skills synchronization throughout the CLI. Stamp-based logic is integrated into the update command, check/notice flow, and installation script while removing now-unused state machinery.

Changes

Skills stamp infrastructure and drift detection

Layer / File(s)	Summary
Stamp file I/O and path management internal/skillscheck/stamp.go, internal/skillscheck/stamp_test.go	ReadStamp reads and trims the version from skills.stamp, returning empty string and nil on missing file; WriteStamp atomically writes the version after ensuring the base config directory exists. Tests cover missing stamp (ENOENT safe), normal read, newline tolerance, empty files, directory creation, overwrites, and MkdirAll failure modes.
Stale-version detection using stamps internal/skillscheck/check.go, internal/skillscheck/check_test.go, internal/skillscheck/notice.go	Init() replaces synced-version lookup with stamp-based drift detection: fails closed on read errors, skips on cold start (empty stamp), and sets StaleNotice when stamp diverges from current version. Tests verify in-sync (no notice), drift (notice with Current/Target), skip conditions, and I/O error handling. Notice docs clarified for stamp semantics.
Deletion of state JSON persistence internal/skillscheck/state.go (removed), internal/skillscheck/state_test.go (removed), internal/skillscheck/sync.go (removed), internal/skillscheck/sync_test.go (removed)	Removes SkillsState type, JSON read/write, synced-version retrieval, and the entire SyncSkills orchestration with plan/diff logic; no replacement needed as stamp-based approach is simpler and does not require sync planning.

Update command skills integration and reporting

Layer / File(s)	Summary
Updater skills entry point cleanup internal/selfupdate/updater.go, internal/selfupdate/updater_test.go	Removes ListOfficialSkills, ListGlobalSkills, InstallSkill exported methods and SkillsCommandOverride test hook; consolidates to runSkillsAdd(source) which always invokes npx -y skills add <source> -g -y with fixed arguments and no override. Removes skills-related test cases and strings import.
Update command skills sync and cleanup cmd/update/update.go	Introduces runSkillsAndStamp helper that deduplicates skills updates (skips when stamp matches target unless forced), persists stamp on success best-effort (warns on write failure), returns nil on dedup. Updates all skills-sync paths (manual, npm, already-up-to-date) and refactors --check output to report skills_status via ReadStamp, omitting the block on read error. Removes old syncSkills var override.
Skills result formatting and hints cmd/update/update.go	Rewrites applySkillsResult and emitSkillsTextHints to consume *selfupdate.NpmResult semantics: sets skills_action to in_sync/synced/failed, adds skills_warning on error, optionally emits truncated stderr detail, provides "Run manually" hint on failure instead of prior sync-summary messaging.
Update command test refactor for stamp-based skills cmd/update/update_test.go	Refactors mockDetectAndNpm to use SkillsUpdateOverride instead of skills-command override; updates isolation comments to prevent stamp clobbering. Adds comprehensive runSkillsAndStamp unit tests (dedup hit, forced bypass, successful stamp write, failure stamp preservation, write-failure warning). Updates all update integration tests to trigger/verify stamp-aware behavior and adjusts output assertions for new skills_* fields.

Integration and install script updates

Layer / File(s)	Summary
Root integration test stamp usage cmd/root_integration_test.go	Updates TestSetupNotices_* tests to use skillscheck.WriteStamp instead of skillscheck.WriteState; aligns test setup with stamp-based drift detection while preserving assertions on notice["skills"] presence and drift message content.
Install wizard skills installation flow scripts/install-wizard.js	Simplifies stepInstallSkills by adding skillsAlreadyInstalled() check (via npx skills ls -g) and skipping installation when skills are already present. Removes prior plan/sync logic and state-file management. Changes stepInstallGlobally return type from version string to boolean, and updates main() to call stepInstallSkills directly after global install.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related PRs

• larksuite/cli#801: Overlaps on skillscheck.Init stamp-based drift detection and cold-start notice suppression behavior.
• larksuite/cli#884: Both PRs modify setupNotices skills notice composition in cmd/root_integration_test.go.
• larksuite/cli#723: Implements the same core stamp-based skills flow refactor across skillscheck and update command.

Suggested labels

size/L

Suggested reviewers

• sang-neo03

Poem

A rabbit hops through the stamp, so neat,
Where state files once danced in JSON's beat.
Now lightweight stamps track versions true,
No more syncing plans to muddle through. 🐇✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.85% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title 'fix: simplify skills sync to stamp-based approach' accurately describes the main change: replacing incremental sync with a stamp-based approach.
Description check	✅ Passed	The description includes summary, changes, test plan, and related issues sections matching the template, with substantive details about the stamp restoration and removed complexity.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/simplify-skills-sync

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ESLint

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[github-actions]

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@35bcd4a25eea7393c55242ecef42b76357957c17

🧩 Skill update

npx skills add larksuite/cli#fix/simplify-skills-sync -y -g

[coderabbitai]

Actionable comments posted: 2

🤖 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 191-196: The in_sync boolean currently compares stamp == cur raw;
change it to compare normalized versions to match runSkillsAndStamp's dedup
logic: call normalizeVersion on both stamp and cur (e.g.,
normalizeVersion(stamp) == normalizeVersion(cur)) when setting
out["skills_status"]["in_sync"]; locate the ReadStamp use and the map
construction around skillscheck.ReadStamp(), and update the in_sync expression
accordingly so normalization is applied to both sides.
- Around line 370-375: In reportAlreadyUpToDate, the comparison uses raw stamp
and cur which can differ in format; call normalizeVersion on both stamp and cur
before setting out["skills_status"] so "current" and "target" use
normalizedVersion values and "in_sync" compares normalizedStamp ==
normalizedCur; reference normalizeVersion, stamp, cur, reportAlreadyUpToDate,
and the out["skills_status"] map to locate and update the code.

🪄 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: af8a6a56-2f10-445c-af50-d9c09377d397

📥 Commits

Reviewing files that changed from the base of the PR and between e19e090 and 35bcd4a.

📒 Files selected for processing (15)

• cmd/root_integration_test.go
• cmd/update/update.go
• cmd/update/update_test.go
• internal/selfupdate/updater.go
• internal/selfupdate/updater_test.go
• internal/skillscheck/check.go
• internal/skillscheck/check_test.go
• internal/skillscheck/notice.go
• internal/skillscheck/stamp.go
• internal/skillscheck/stamp_test.go
• internal/skillscheck/state.go
• internal/skillscheck/state_test.go
• internal/skillscheck/sync.go
• internal/skillscheck/sync_test.go
• scripts/install-wizard.js

💤 Files with no reviewable changes (5)

• internal/skillscheck/state_test.go
• internal/skillscheck/sync_test.go
• internal/skillscheck/state.go
• internal/skillscheck/sync.go
• internal/selfupdate/updater_test.go