feat: add update command with self-update, verification, and rollback#391
Merged
liangshuo-1 merged 42 commits intomainfrom Apr 10, 2026
Merged
feat: add update command with self-update, verification, and rollback#391liangshuo-1 merged 42 commits intomainfrom
liangshuo-1 merged 42 commits intomainfrom
Conversation
Change-Id: Ic4af400ad8fc8c2b37f046343b31e1c63c607f81
Change-Id: I0e15ff0ea489125c0e593baf4e7c8f2e0ef68226
Change-Id: Ia5920e72707aea734099197cde1dce809711aa10
Change-Id: Ibca2396e41264f5c05d82591d0f68e230a6b9f4b
Change-Id: Ifdda45ddfa0b3d81965f81f9afa946585617093f
Change-Id: I5a8129b9c68c9d3a871a9e9f1b9dc3f28027f87a
Change-Id: I4c3d59121a65ca285c68c55ebccf45c769fee275
…background goroutine Change-Id: I531eaaf7b344edcbb21fc1b2a06ae33be2f1d409
Change-Id: Icc65569c98459748eb3c07ec5a75e23059e8a90d
…d URLs, changelog, neutral permission hint, npm-not-found fallback Change-Id: I26db4d249887c8b5a701a06b320a9a0a4bade080
Change-Id: I8df0df2e5edf27659c188e53ae3f168cb8904adb
…L signature, platform-safe tests Change-Id: I6e3a12ea07cb6dde316460078164f720cac19696
Change-Id: Ib023b475d82456c770a0135eda2d46153134238b
Change-Id: I55f7f8acaef4dde103fb56fd3a8e844b1a45f5af
Change-Id: If78e774f781f939a2d805313e13c523b816cf7e2
…cy terminals Change-Id: I7f49399eb22fce73b69c26eb19faa6f2e2b1f58f
…ests Change-Id: Ie967c764056e61c34a452c6a2b8f743d696bf689
…d Windows check tests Change-Id: I066e9c7ec4bb5a3de7395af0f586b83b6eb4822a
…ts, clean up isWindows() Change-Id: I0de5c85d21201704e0efab3c0c421bef718cf68a
Introduce internal/selfupdate/ with two capabilities: - ReplaceSelf: cross-platform binary replacement that uses atomic rename on Unix and the rename-to-.old trick on Windows (where the running exe is locked). - Backup/Rollback: maintains up to 5 versioned backups of the CLI binary under ~/.lark-cli/backups/, with ListBackups, Rollback, and RollbackTo for restoring previous versions. Change-Id: Ib8b374d259c15972d351d969932db3849717f791
…remove Windows lock workaround Change-Id: Ice3e4c50650c45980d48421db6625edb7d1b0d82
… for real self-update Change-Id: Ic8a0b11c38ac0599578f76fa6f555cfb5b99ebeb
…auto-recover on startup if .old exists without original Change-Id: Ice3d1e277745f8721bee070506623b19383b47f1
… npm paths, extract reportError, remove duplicate lookPath Change-Id: Iba3b86df6b7be518024b872db99c999057f83e93
Change-Id: I7d410c69f6549336b4b5049538fd1b5fe7d575ce
…param Change-Id: I552096599aae6c0a3363db50f3a19f299f634b18
…unix.go, replace_windows.go) Change-Id: I3b015037a4940987b8c041b0090fa90d80d3740c
…roject convention) Change-Id: I328c20c4dc05dda561311eedb1607f5dbb00f3c2
… in selfupdate, eliminate raw os.* calls Change-Id: I9b0c29d86258d358d9bf10359a4ad2ed981f4566
…im cmd/update to orchestration+output only Change-Id: I47741cbba3b672695dd687d68f5301bf20f2b44f
Change-Id: Ic955d3882b1d4d9672a33a4cd1de8372d3ca84c9
…ale comment filenames Change-Id: Ie608c4881d4af5e497ba75c172c71c34c5fc7bcf
… and robustness improvements - Verify new binary reports expected version after npm update; rollback to .old backup on failure with platform-aware hints - Add context timeouts to npm install, skills update, and verify commands - Support full semver prerelease comparison in IsNewer (rc, beta, etc.) - Enhance run.js .old recovery: health-check current binary before discarding backup, restore from .old when current binary is corrupt - Refactor mail_watch signal handling to use select for clean shutdown Change-Id: Id10e3c83cfaa193abcfeb390816d2c5c91d592fa
github-actions
bot
added
the
size/L
|
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:
📝 WalkthroughWalkthroughAdds a top-level Changes
Sequence DiagramsequenceDiagram
participant User as CLI User
participant Cmd as Update Command
participant Updater as Updater
participant FS as VFS
participant NPM as npm
participant Verify as Verify
User->>Cmd: invoke `lark-cli update` or `--check`
Cmd->>Updater: DetectInstallMethod()
Updater->>FS: Executable() / EvalSymlinks()
Updater-->>Cmd: DetectResult (method, path, npm available)
Cmd->>Cmd: fetch latest version
alt check-only or manual-required
Cmd-->>User: show release/changelog URLs and manual instructions
else npm auto-update path
Cmd->>Updater: PrepareSelfReplace()
Updater->>FS: rename exe -> exe.old (Windows) / noop (Unix)
Cmd->>NPM: run `npm install -g `@larksuite/cli`@<version>`
NPM-->>Cmd: npm result (stdout/stderr/err)
alt npm succeeded
Cmd->>Verify: VerifyBinary(<version>)
Verify->>FS: run exe --version
Verify-->>Cmd: match?
alt match
Cmd->>Updater: RunSkillsUpdate()
Updater-->>Cmd: skills result
Cmd-->>User: success (optional skills warning)
else mismatch
Updater->>FS: restore exe.old -> exe
Cmd-->>User: verification failed + rollback/manual hint
end
else npm failed
Updater->>FS: restore exe.old -> exe
Cmd-->>User: npm failure + permission/manual hint
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@68e08e45f42d67a49d7c22f97c6047dcc813d83c🧩 Skill updatenpx skills add larksuite/cli#feat/upgrade-command -y -g |
Greptile SummaryThis PR adds a Confidence Score: 4/5Safe to merge; previously flagged correctness issues have been resolved, remaining findings are P2 UX suggestions that do not affect the happy path. All P0/P1 issues from prior review threads (backupCreated tracking, double-restore race, restore error swallowing) are now addressed. Three P2 findings remain: missing Windows EPERM permission hint, unhandled restoreOldBinary return value in run.js crash-recovery, and a slightly misleading rollback message when Rename fails after Remove succeeds. None block functionality. cmd/update/update.go (permissionHint), scripts/run.js (restoreOldBinary return value), internal/selfupdate/updater_windows.go (restore hint accuracy) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant run.js
participant GoProcess as lark-cli (Go)
participant npm
participant Registry as npm registry
User->>run.js: lark-cli update
run.js->>run.js: check .old backup (crash recovery)
run.js->>GoProcess: execFileSync(bin, ["update"])
GoProcess->>GoProcess: CleanupStaleFiles()
GoProcess->>Registry: FetchLatest()
Registry-->>GoProcess: latest version
alt already up to date
GoProcess-->>User: ✓ already up to date
else update available
GoProcess->>GoProcess: DetectInstallMethod()
alt npm install detected
GoProcess->>GoProcess: PrepareSelfReplace() → rename exe → exe.old (Windows only)
GoProcess->>npm: npm install -g @larksuite/cli@version
npm-->>GoProcess: result
alt npm install failed
GoProcess->>GoProcess: restore() → rename exe.old → exe
GoProcess-->>User: ✗ Update failed + hint
else npm install succeeded
GoProcess->>GoProcess: VerifyBinary(expectedVersion)
alt verification failed
GoProcess->>GoProcess: restore() → rename exe.old → exe
GoProcess-->>User: ✗ verification failed + rollback hint
else verification passed
GoProcess->>npm: RunSkillsUpdate() (best-effort)
GoProcess-->>User: ✓ Updated + skills status
end
end
else manual install
GoProcess-->>User: download URL + npm install instructions
end
end
Reviews (8): Last reviewed commit: "fix: pass -y to both npx and skills to p..." | Re-trigger Greptile |
![greptile-apps[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/update/update.go (1)
146-171:⚠️ Potential issue | 🟠 MajorValidate the cache file path before reading or writing it.
loadState()andsaveState()usestatePath()directly forvfs.ReadFile/validate.AtomicWrite, but the path comes fromcore.GetConfigDir()and never goes throughvalidate.SafeInputPath. That bypasses the repo’s required safety check on both the read and write sides.As per coding guidelines: "Validate paths using
validate.SafeInputPathbefore any file I/O operations".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/update/update.go` around lines 146 - 171, loadState and saveState call statePath() and perform file I/O without validating the path, bypassing the required safety check; update loadState and saveState to call validate.SafeInputPath on the path returned by statePath() (or validate within statePath itself) and handle/propagate any validation error before calling vfs.ReadFile or validate.AtomicWrite, ensuring you use the validated path for the subsequent operations and return the validation error when present.
🧹 Nitpick comments (2)
internal/selfupdate/updater_test.go (1)
24-33: MakeTestResolveExeexercise the VFS hook, not the host process path.Right now this only asserts that the real test executable path is absolute, so it would still pass if
resolveExe()stopped honoringvfs.Executable()orvfs.EvalSymlinks(). Pointvfs.DefaultFSatresolveTestFShere and assert the exact resolved path instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater_test.go` around lines 24 - 33, Update TestResolveExe to exercise the VFS hook by swapping vfs.DefaultFS to the provided resolveTestFS before calling u.resolveExe(), then restore the original vfs.DefaultFS after the test; call u.resolveExe() and assert the returned path equals the expected exact path produced by resolveTestFS (rather than only testing filepath.IsAbs), so the test fails if resolveExe() stops using vfs.Executable() or vfs.EvalSymlinks(). Ensure you reference TestResolveExe and resolveExe when making the change and perform proper teardown to reset vfs.DefaultFS.cmd/update/update_test.go (1)
19-24: Add a JSON decode helper for the stdout assertions.Most of the JSON-mode tests only look for substrings, so malformed envelopes, bad escaping, or trailing garbage would still pass. A small helper that unmarshals stdout into a
map[string]anywould validate the AI-agent contract this PR is adding and make the assertions less brittle.Based on learnings: "Applies to **/*_test.go : Use
cmdutil.TestFactory(t, config)for creating test factories in Go tests".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/update/update_test.go` around lines 19 - 24, Add a JSON decode helper to validate JSON-mode test output by unmarshalling the stdout buffer into a map[string]any and failing the test on error; implement a helper function (e.g., decodeJSONStdout(t *testing.T, stdout *bytes.Buffer) map[string]any) alongside newTestFactory in update_test.go that reads stdout.Bytes(), calls json.Unmarshal into map[string]any, and uses t.Fatalf on unmarshal errors, then update JSON-mode tests to call decodeJSONStdout and assert against the returned map instead of substring checks to ensure well-formed envelopes and correct escaping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/update/update.go`:
- Around line 159-168: The helper reportError currently writes a JSON envelope
but returns output.ErrBare, which strips structured error details; change
reportError to return output.Errorf (or output.ErrWithHint where applicable)
even in opts.JSON path so the returned error carries the type/message contract
expected by callers; update the opts.JSON branch in reportError (and the similar
npm-install / verify-failure branches) to construct the same structured JSON
output via output.PrintJson but then return output.Errorf(exitCode, errType,
"%s", msg) (or output.ErrWithHint when a hint is appropriate) so RunE callers
receive a structured, actionable error instead of output.ErrBare.
In `@internal/selfupdate/updater_test.go`:
- Around line 35-47: Tests call New() and then PrepareSelfReplace() /
CleanupStaleFiles(), which on Windows perform real vfs.Remove/vfs.Rename against
the resolved executable; fix by overriding vfs.DefaultFS with a mock FS backed
by a temporary directory for these tests (follow the pattern used in
TestVerifyBinaryChecksVersion): create a custom vfs.FS implementation that
points to temp dir, set vfs.DefaultFS = mockFS before calling
New()/PrepareSelfReplace()/CleanupStaleFiles(), and restore the original
DefaultFS with t.Cleanup() so file ops act on isolated fixtures rather than the
test binary; update both TestPrepareSelfReplace_ReturnsNoError and
TestCleanupStaleFiles_NoPanic to use this setup.
In `@internal/selfupdate/updater_windows.go`:
- Around line 35-39: The restore closure currently ignores errors from
vfs.Remove(exe) and vfs.Rename(oldPath, exe); change restore to return an error
(or accept a logger) and propagate/log any failures so callers can detect
rollback failure — update the restore closure signature in
internal/selfupdate/updater_windows.go to capture and return errors from
vfs.Remove and vfs.Rename, and update the caller in cmd/update/update.go to
check the returned error and handle/log it (or surface it to the user) instead
of assuming success.
In `@scripts/run.js`:
- Around line 31-32: The execFileSync invocation in scripts/run.js that runs the
binary (execFileSync(bin, ["--version"], { stdio: "ignore" })) lacks a timeout
and can hang; add a timeout option of 10000 ms to the options object (matching
the Go VerifyBinary behavior) so the call becomes execFileSync(..., { stdio:
"ignore", timeout: 10000 }) and ensure errors from timeout are
handled/propagated the same way existing exec failures are.
---
Outside diff comments:
In `@internal/update/update.go`:
- Around line 146-171: loadState and saveState call statePath() and perform file
I/O without validating the path, bypassing the required safety check; update
loadState and saveState to call validate.SafeInputPath on the path returned by
statePath() (or validate within statePath itself) and handle/propagate any
validation error before calling vfs.ReadFile or validate.AtomicWrite, ensuring
you use the validated path for the subsequent operations and return the
validation error when present.
---
Nitpick comments:
In `@cmd/update/update_test.go`:
- Around line 19-24: Add a JSON decode helper to validate JSON-mode test output
by unmarshalling the stdout buffer into a map[string]any and failing the test on
error; implement a helper function (e.g., decodeJSONStdout(t *testing.T, stdout
*bytes.Buffer) map[string]any) alongside newTestFactory in update_test.go that
reads stdout.Bytes(), calls json.Unmarshal into map[string]any, and uses
t.Fatalf on unmarshal errors, then update JSON-mode tests to call
decodeJSONStdout and assert against the returned map instead of substring checks
to ensure well-formed envelopes and correct escaping.
In `@internal/selfupdate/updater_test.go`:
- Around line 24-33: Update TestResolveExe to exercise the VFS hook by swapping
vfs.DefaultFS to the provided resolveTestFS before calling u.resolveExe(), then
restore the original vfs.DefaultFS after the test; call u.resolveExe() and
assert the returned path equals the expected exact path produced by
resolveTestFS (rather than only testing filepath.IsAbs), so the test fails if
resolveExe() stops using vfs.Executable() or vfs.EvalSymlinks(). Ensure you
reference TestResolveExe and resolveExe when making the change and perform
proper teardown to reset vfs.DefaultFS.
🪄 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: 61d2ccf0-1cec-459f-82df-3d57207f5704
📒 Files selected for processing (14)
cmd/doctor/doctor.gocmd/root.gocmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/selfupdate/updater_unix.gointernal/selfupdate/updater_windows.gointernal/update/update.gointernal/update/update_test.gointernal/vfs/default.gointernal/vfs/fs.gointernal/vfs/osfs.goscripts/run.js
Change-Id: I5e0dd48223deb008815d99123bd13ef559d7c65b
Change-Id: Ibac20469663721f3a5b17fec9b71a23a8a9114d2
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/selfupdate/updater.go`:
- Around line 200-206: The Truncate function can panic when maxLen is
non-positive; update Truncate(s string, maxLen int) to check if maxLen <= 0 and
return "" in that case, otherwise proceed with the existing rune conversion and
slice logic (keep the function name Truncate and its behavior for positive
maxLen).
- Around line 102-105: The current check uses strings.Contains(resolved,
"node_modules") which can false-positive and is case-sensitive; instead
normalize the path (e.g., filepath.Clean and filepath.ToSlash), split it into
path elements, and test each element with strings.EqualFold against
"node_modules" before setting method to InstallNpm; update the code around the
resolved variable and the install-method decision (where method is initialized
to InstallManual and compared to set InstallNpm) to use this segment-aware,
case-insensitive check.
- Around line 157-160: The npx invocation builds cmd in exec.CommandContext(ctx,
npxPath, "skills", "add", "larksuite/cli", "-g", "-y") so the "-y" is being
passed to the "skills" subcommand instead of npx itself; change the argument
order when constructing cmd (in the same block where cmd := exec.CommandContext
is defined and before setting cmd.Stdout/cmd.Stderr and running cmd.Run()) to
pass "-y" immediately after npxPath (i.e., move the "-y" token before "skills")
so npx itself receives the non-interactive flag.
🪄 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: 59e65cf4-c55e-48c4-bada-6d41344abca3
📒 Files selected for processing (1)
internal/selfupdate/updater.go
…n, add safety guards - Track backupCreated in Updater so CanRestorePreviousVersion reflects actual state instead of unconditionally returning true on Windows - Move -y flag before command name in npx invocation so npx consumes it (prevents interactive prompt that could hang until timeout) - Add timeout to run.js execFileSync health check (10s, matching Go side) - Guard Truncate against non-positive maxLen Change-Id: I27ec302968f57895ac1c7975623682f7969ec97d
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/selfupdate/updater.go (1)
104-107:⚠️ Potential issue | 🟡 MinorUse segment-aware matching for
node_modulesdetection.Line 105 still uses substring matching, which can false-positive (e.g.
node_modules-cache) and is case-sensitive on Windows paths. Match path segments case-insensitively instead.Proposed fix
method := InstallManual - if strings.Contains(resolved, "node_modules") { - method = InstallNpm + for _, part := range strings.FieldsFunc(resolved, func(r rune) bool { + return r == '/' || r == '\\' + }) { + if strings.EqualFold(part, "node_modules") { + method = InstallNpm + break + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` around lines 104 - 107, The current substring check on resolved can false-positive and is case-sensitive; replace the strings.Contains(resolved, "node_modules") check with a segment-aware, case-insensitive check: split resolved by filepath.Separator (e.g. strings.Split(resolved, string(os.PathSeparator))) and iterate segments, using strings.EqualFold(segment, "node_modules") to detect a real path segment; if any match, set method = InstallNpm (otherwise leave as InstallManual). Reference symbols: resolved, method, InstallManual, InstallNpm.
🧹 Nitpick comments (1)
internal/selfupdate/updater.go (1)
146-146: Update the staleRunSkillsUpdatecomment to match actual args.Line 146 says
npx skills add ... -g -y, but Line 159 correctly runsnpx -y skills add ... -g. Keep the comment aligned to avoid accidental regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` at line 146, The comment for RunSkillsUpdate is stale and shows the wrong npx argument order; update the function comment to match the actual command invoked in RunSkillsUpdate (i.e., "npx -y skills add larksuite/cli -g") so the docstring accurately reflects the executed args and avoids future regressions—locate the RunSkillsUpdate function and replace the comment text accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/selfupdate/updater.go`:
- Line 83: The struct field backupCreated in updater.go is flagged by
golangci-lint as unused because it's only referenced in updater_windows.go; add
an explicit nolint on the field declaration to silence the linter and document
why: change the field line to include a comment like "//nolint:unused // used
only on Windows builds (see updater_windows.go)" so CI won't fail while
preserving the Windows-only usage; alternatively, if you prefer not to use a
nolint, move the backupCreated field out of updater.go into the Windows-specific
file (updater_windows.go) so the field exists only under the windows build tag.
---
Duplicate comments:
In `@internal/selfupdate/updater.go`:
- Around line 104-107: The current substring check on resolved can
false-positive and is case-sensitive; replace the strings.Contains(resolved,
"node_modules") check with a segment-aware, case-insensitive check: split
resolved by filepath.Separator (e.g. strings.Split(resolved,
string(os.PathSeparator))) and iterate segments, using
strings.EqualFold(segment, "node_modules") to detect a real path segment; if any
match, set method = InstallNpm (otherwise leave as InstallManual). Reference
symbols: resolved, method, InstallManual, InstallNpm.
---
Nitpick comments:
In `@internal/selfupdate/updater.go`:
- Line 146: The comment for RunSkillsUpdate is stale and shows the wrong npx
argument order; update the function comment to match the actual command invoked
in RunSkillsUpdate (i.e., "npx -y skills add larksuite/cli -g") so the docstring
accurately reflects the executed args and avoids future regressions—locate the
RunSkillsUpdate function and replace the comment text accordingly.
🪄 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: b43fef17-a175-47ae-9bb6-f6c7f5a9ccd8
📒 Files selected for processing (4)
internal/selfupdate/updater.gointernal/selfupdate/updater_windows.gointernal/update/update_test.goscripts/run.js
✅ Files skipped from review due to trivial changes (2)
- scripts/run.js
- internal/update/update_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/selfupdate/updater_windows.go
os.Executable returns the currently running (old) binary path. After npm install replaces the package, the file layout may differ (e.g. symlink targets renamed), causing EvalSymlinks to fail. Use LookPath to find the newly installed binary in PATH instead. Change-Id: I8d73e98186edcb148f9442909af3f3efa9af6619
…tions Change-Id: I3ceece4e8af279115dae8829315958029526f447
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/selfupdate/updater.go (2)
83-83:⚠️ Potential issue | 🟠 Major
backupCreatedcan still trip non-Windows lint runsLine 83 keeps
backupCreatedin a cross-platform file, but its usages are Windows-only (internal/selfupdate/updater_windows.go). This can still failunusedlint in default build contexts.Proposed fix
- backupCreated bool + backupCreated bool //nolint:unused // used only on Windows builds (see internal/selfupdate/updater_windows.go)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` at line 83, The cross-platform declaration backupCreated is unused on non-Windows builds and triggers lint errors; remove the backupCreated variable from the shared declaration in updater.go and instead declare it in the Windows-only implementation (updater_windows.go) so it exists only for Windows builds (or alternatively place the declaration behind a Windows build tag); update references in the Windows code to use that Windows-scoped backupCreated variable.
104-107:⚠️ Potential issue | 🟡 MinorUse segment-aware matching for
node_modulesdetectionLine 105 uses substring matching, which can false-positive (
.../node_modules-cache/...) and miss case variants on Windows. Prefer path-segment matching withstrings.EqualFold.Proposed fix
method := InstallManual - if strings.Contains(resolved, "node_modules") { - method = InstallNpm + for _, part := range strings.FieldsFunc(resolved, func(r rune) bool { + return r == '/' || r == '\\' + }) { + if strings.EqualFold(part, "node_modules") { + method = InstallNpm + break + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` around lines 104 - 107, The current substring check for "node_modules" on the variable resolved in updater.go can false-positive and is not case-insensitive on Windows; replace the logic in the block that sets method (InstallManual/InstallNpm) so you split the resolved path into path segments (use filepath.Clean and strings.Split with string(os.PathSeparator) or use filepath.SplitList), then iterate segments and use strings.EqualFold(segment, "node_modules") to detect a segment match and set method = InstallNpm; keep the rest of the method selection unchanged and reference the variables resolved and method and the constants InstallManual/InstallNpm to locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/selfupdate/updater.go`:
- Line 146: The comment for RunSkillsUpdate no longer matches the executed
command; update the function comment for RunSkillsUpdate to reflect the real
command being run (npx -y skills add larksuite/cli -g) instead of "npx skills
add ... -g -y" so the docstring matches the implementation and prevents future
confusion.
---
Duplicate comments:
In `@internal/selfupdate/updater.go`:
- Line 83: The cross-platform declaration backupCreated is unused on non-Windows
builds and triggers lint errors; remove the backupCreated variable from the
shared declaration in updater.go and instead declare it in the Windows-only
implementation (updater_windows.go) so it exists only for Windows builds (or
alternatively place the declaration behind a Windows build tag); update
references in the Windows code to use that Windows-scoped backupCreated
variable.
- Around line 104-107: The current substring check for "node_modules" on the
variable resolved in updater.go can false-positive and is not case-insensitive
on Windows; replace the logic in the block that sets method
(InstallManual/InstallNpm) so you split the resolved path into path segments
(use filepath.Clean and strings.Split with string(os.PathSeparator) or use
filepath.SplitList), then iterate segments and use strings.EqualFold(segment,
"node_modules") to detect a segment match and set method = InstallNpm; keep the
rest of the method selection unchanged and reference the variables resolved and
method and the constants InstallManual/InstallNpm to locate where to change.
🪄 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: f0c6215d-e861-47cd-9258-f66111b6dc96
📒 Files selected for processing (2)
internal/selfupdate/updater.gointernal/selfupdate/updater_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/selfupdate/updater_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (3)
internal/selfupdate/updater.go (3)
146-146:⚠️ Potential issue | 🟡 MinorKeep the
RunSkillsUpdatecomment in sync with the actual command.Line 146 documents
npx skills add larksuite/cli -g -y, but line 159 executesnpx -y skills add larksuite/cli -g. Update the comment to match current behavior.Proposed fix
-// RunSkillsUpdate executes npx skills add larksuite/cli -g -y. +// RunSkillsUpdate executes npx -y skills add larksuite/cli -g.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` at line 146, Update the doc comment for RunSkillsUpdate to match the actual executed command: change the description from "npx skills add larksuite/cli -g -y" to "npx -y skills add larksuite/cli -g" (or otherwise mirror the exact order/flags used in the npx invocation inside RunSkillsUpdate) so the comment accurately reflects the command being run.
104-107:⚠️ Potential issue | 🟡 MinorMatch
node_modulesas a path segment, not a substring.
strings.Contains(resolved, "node_modules")will false-positive on paths like/opt/node_modules-cache/...and is case-sensitive on Windows. Splitting the path and checking each segment withstrings.EqualFoldis safer.Proposed fix
method := InstallManual - if strings.Contains(resolved, "node_modules") { - method = InstallNpm + for _, part := range strings.FieldsFunc(resolved, func(r rune) bool { + return r == '/' || r == '\\' + }) { + if strings.EqualFold(part, "node_modules") { + method = InstallNpm + break + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` around lines 104 - 107, The code sets method using strings.Contains(resolved, "node_modules") which can match substrings and is case-sensitive; update the logic around the method variable (InstallManual/InstallNpm) to treat "node_modules" as a full path segment by cleaning the path (e.g., filepath.Clean), splitting resolved on the OS path separator, and iterating segments using strings.EqualFold(segment, "node_modules"); if any segment matches, set method = InstallNpm, otherwise leave as InstallManual so Windows case differences and partial matches (like "node_modules-cache") are avoided.
83-83:⚠️ Potential issue | 🟡 MinorFix the lint-blocking unused field (
backupCreated).CI fails because
backupCreatedis only used inupdater_windows.go(lines 34, 75), which is excluded from non-Windows builds. Add a//nolint:unuseddirective with rationale, or move the field to the Windows-specific file.Proposed fix (nolint directive)
- backupCreated bool + backupCreated bool //nolint:unused // used only on Windows (see updater_windows.go)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/selfupdate/updater.go` at line 83, The struct field backupCreated in updater.go is unused on non-Windows builds and causes lint failure; either move the backupCreated field into the Windows-specific file updater_windows.go (so it only exists on Windows builds) or annotate the field in updater.go with a //nolint:unused comment plus a short rationale referencing that it is used only in updater_windows.go; update the declaration of backupCreated (or relocate it) accordingly so non-Windows builds no longer see an unused field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/selfupdate/updater.go`:
- Line 146: Update the doc comment for RunSkillsUpdate to match the actual
executed command: change the description from "npx skills add larksuite/cli -g
-y" to "npx -y skills add larksuite/cli -g" (or otherwise mirror the exact
order/flags used in the npx invocation inside RunSkillsUpdate) so the comment
accurately reflects the command being run.
- Around line 104-107: The code sets method using strings.Contains(resolved,
"node_modules") which can match substrings and is case-sensitive; update the
logic around the method variable (InstallManual/InstallNpm) to treat
"node_modules" as a full path segment by cleaning the path (e.g.,
filepath.Clean), splitting resolved on the OS path separator, and iterating
segments using strings.EqualFold(segment, "node_modules"); if any segment
matches, set method = InstallNpm, otherwise leave as InstallManual so Windows
case differences and partial matches (like "node_modules-cache") are avoided.
- Line 83: The struct field backupCreated in updater.go is unused on non-Windows
builds and causes lint failure; either move the backupCreated field into the
Windows-specific file updater_windows.go (so it only exists on Windows builds)
or annotate the field in updater.go with a //nolint:unused comment plus a short
rationale referencing that it is used only in updater_windows.go; update the
declaration of backupCreated (or relocate it) accordingly so non-Windows builds
no longer see an unused field.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e4250606-3905-4a75-bff6-736bf127e19f
📒 Files selected for processing (1)
internal/selfupdate/updater.go
Both Unix and Windows CanRestorePreviousVersion now use u.backupCreated. Unix never sets it (always false), Windows sets it after successful rename. Fixes golangci-lint unused field error. Change-Id: If47f5acac814265c862385deb9f54f06130e55e7
…ate comment - Check .old exists before restore to prevent deleting the only working binary when run.js has already recovered it during VerifyBinary - Update RunSkillsUpdate doc comment to match actual npx -y flag order Change-Id: Ib20a0afda9871bd80b31bc474b52ad7ae1918c42
…px hints - VerifyBinary now prefers vfs.Executable() (current process path) over LookPath, avoiding false failures when PATH contains a different lark-cli. Falls back to LookPath only if Executable() fails. - Windows restore() clears backupCreated on failure so CanRestorePreviousVersion reflects actual outcome, preventing misleading "previous version has been restored" messages. - Unify npx hint messages to "npx -y skills add larksuite/cli -g" matching the actual command execution order. Change-Id: I7933d057b05d5800ae3366953fcea2d89012fbaf
npx -y suppresses the package install prompt, but skills add also has its own -y/--yes flag to skip interactive skill selection. Without both, skills add blocks on "Select skills to install" prompt and hangs until the 2-minute timeout. Change-Id: I63f4fb789630536f55572489cc592ce21b903c45
MaxHuang22
approved these changes
Apr 10, 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
lark-cli updatecommand: auto-detect npm install, fetch latest version, runnpm install -g, and update skills.oldbackup on failure (Windows)rc.1 < rc.2 < stable)--checkflag for update availability check without installing,--forcefor reinstall,--jsonfor structured output.oldbackup andrun.jscrash recoveryTest plan
go test ./cmd/update/ ./internal/selfupdate/ ./internal/update/all passlark-cli update, verified upgrade to 1.0.7.oldbackup/restore andrun.jsrecovery--jsonoutput is parseable by AI agentsSummary by CodeRabbit
New Features
Bug Fixes
lark-cli update(ornpm install -g @larksuite/cli) when updates are available