diff --git a/cmd/update/update.go b/cmd/update/update.go index 2d01a4d66..d42f87e9b 100644 --- a/cmd/update/update.go +++ b/cmd/update/update.go @@ -263,7 +263,7 @@ func doAutoUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string "ok": false, "error": map[string]interface{}{ "type": "update_error", "message": fmt.Sprintf("%s install failed: %s", pm, npmResult.Err), "detail": selfupdate.Truncate(combined, maxNpmOutput), - "hint": permissionHint(combined), + "hint": permissionHint(combined, pm), }, }) return output.ErrBare(output.ExitAPI) @@ -275,7 +275,7 @@ func doAutoUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string fmt.Fprint(io.ErrOut, npmResult.Stderr.String()) } fmt.Fprintf(io.ErrOut, "\n%s Update failed: %s\n", symFail(), npmResult.Err) - if hint := permissionHint(combined); hint != "" { + if hint := permissionHint(combined, pm); hint != "" { fmt.Fprintf(io.ErrOut, " %s\n", hint) } return output.ErrBare(output.ExitAPI) @@ -286,7 +286,7 @@ func doAutoUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string if err := updater.VerifyBinary(latest); err != nil { restore() msg := fmt.Sprintf("new binary verification failed: %s", err) - hint := verificationFailureHint(updater, latest) + hint := verificationFailureHint(updater, latest, pm) if opts.JSON { output.PrintJson(io.Out, map[string]interface{}{ "ok": false, @@ -326,17 +326,23 @@ func doAutoUpdate(opts *UpdateOptions, io *cmdutil.IOStreams, cur, latest string return nil } -func permissionHint(npmOutput string) string { - if strings.Contains(npmOutput, "EACCES") && !isWindows() { - return "Permission denied. Try: sudo lark-cli update, or adjust your npm global prefix: https://docs.npmjs.com/resolving-eacces-permissions-errors" +func permissionHint(pmOutput, pm string) string { + if !strings.Contains(pmOutput, "EACCES") || isWindows() { + return "" } - return "" + if pm == "pnpm" { + return "Permission denied. Ensure your pnpm global directory is writable — re-run `pnpm setup`, or see https://pnpm.io/pnpm-cli" + } + return "Permission denied. Try: sudo lark-cli update, or adjust your npm global prefix: https://docs.npmjs.com/resolving-eacces-permissions-errors" } -func verificationFailureHint(updater *selfupdate.Updater, latest string) string { +func verificationFailureHint(updater *selfupdate.Updater, latest, pm string) string { if updater.CanRestorePreviousVersion() { return "the previous version has been restored" } + if pm == "pnpm" { + return fmt.Sprintf("automatic rollback is unavailable on this platform; reinstall manually (skills will not be synced): pnpm add -g %s@%s && pnpm dlx skills add larksuite/cli -y -g, or download %s", selfupdate.NpmPackage, latest, releaseURL(latest)) + } 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)) } diff --git a/cmd/update/update_test.go b/cmd/update/update_test.go index 61e546671..177d4340f 100644 --- a/cmd/update/update_test.go +++ b/cmd/update/update_test.go @@ -867,9 +867,9 @@ func TestPermissionHint(t *testing.T) { origOS := currentOS defer func() { currentOS = origOS }() - // Linux: EACCES should produce a hint with npm prefix guidance. + // Linux + npm: EACCES should produce a hint with npm prefix guidance. currentOS = "linux" - hint := permissionHint("EACCES: permission denied, access '/usr/local/lib'") + hint := permissionHint("EACCES: permission denied, access '/usr/local/lib'", "npm") if !strings.Contains(hint, "npm global prefix") { t.Errorf("expected npm prefix hint on linux, got: %s", hint) } @@ -877,16 +877,25 @@ func TestPermissionHint(t *testing.T) { t.Errorf("should not suggest raw sudo npm install, got: %s", hint) } + // Linux + pnpm: EACCES should point at pnpm setup, not npm prefix/sudo. + pnpmHint := permissionHint("EACCES: permission denied, access '/Users/x/Library/pnpm'", "pnpm") + if !strings.Contains(pnpmHint, "pnpm setup") { + t.Errorf("expected pnpm setup hint, got: %s", pnpmHint) + } + if strings.Contains(pnpmHint, "npm global prefix") || strings.Contains(pnpmHint, "sudo") { + t.Errorf("pnpm hint must not reference npm prefix or sudo, got: %s", pnpmHint) + } + // Windows: EACCES hint is suppressed (no EACCES on Windows). currentOS = "windows" - hint = permissionHint("EACCES: permission denied") + hint = permissionHint("EACCES: permission denied", "npm") if hint != "" { t.Errorf("expected empty hint on Windows, got: %s", hint) } // Non-EACCES error: always empty. currentOS = "linux" - if got := permissionHint("some other error"); got != "" { + if got := permissionHint("some other error", "npm"); got != "" { t.Errorf("expected empty hint for non-EACCES, got: %s", got) } } diff --git a/internal/selfupdate/updater.go b/internal/selfupdate/updater.go index 0bfed53dd..4b8f93a18 100644 --- a/internal/selfupdate/updater.go +++ b/internal/selfupdate/updater.go @@ -113,6 +113,17 @@ type Updater struct { // running binary is successfully renamed to .old. Used by // CanRestorePreviousVersion to report whether rollback is possible. backupCreated bool + + // detectCache memoizes the first real DetectInstallMethod result. How this + // binary was installed cannot change during a single process, so caching is + // the correct semantics — and it is required for correctness: the update + // flow mutates the install (pnpm add -g / npm install -g) before syncing + // skills, so a re-detection at skills time could resolve a now-stale + // os.Executable path and misclassify. Seeded pre-update by the first call + // (updateRun), it keeps the post-update skills launcher consistent with the + // launcher reported to the user. Not goroutine-safe; the update flow is + // sequential. + detectCache *DetectResult } // New creates an Updater with default (real) behavior. @@ -124,6 +135,16 @@ func (u *Updater) DetectInstallMethod() DetectResult { if u.DetectOverride != nil { return u.DetectOverride() } + if u.detectCache != nil { + return *u.detectCache + } + result := u.detectInstallMethod() + u.detectCache = &result + return result +} + +// detectInstallMethod performs the real (uncached) detection. +func (u *Updater) detectInstallMethod() DetectResult { exe, err := vfs.Executable() if err != nil { return DetectResult{Method: InstallManual} diff --git a/internal/selfupdate/updater_test.go b/internal/selfupdate/updater_test.go index 1ed51b7bc..93176265e 100644 --- a/internal/selfupdate/updater_test.go +++ b/internal/selfupdate/updater_test.go @@ -501,3 +501,17 @@ func TestSkillsInvocation(t *testing.T) { }) } } + +// TestDetectInstallMethod_Caches locks the fix for the post-update re-detection +// hazard: DetectInstallMethod must return the first (pre-update) detection on +// subsequent calls, so the skills launcher chosen after the binary is replaced +// stays consistent with what was detected — and reported — before the update. +func TestDetectInstallMethod_Caches(t *testing.T) { + u := New() + cached := DetectResult{Method: InstallPnpm, PnpmAvailable: true, ResolvedPath: "/x/pnpm/store/v11/links/@larksuite/cli/1.0.0/node_modules/@larksuite/cli/bin/lark-cli"} + u.detectCache = &cached + got := u.DetectInstallMethod() + if got.Method != InstallPnpm || !got.PnpmAvailable { + t.Errorf("expected cached pnpm result to be returned, got %+v", got) + } +}