fix: support pnpm self-update#1698
Open
xiawiie wants to merge 2 commits into
Open
Conversation
github-actions
Bot
added
the
size/L
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR adds pnpm support to self-update detection and execution. Changespnpm-aware self-update
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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 |
There was a problem hiding this comment.
Pull request overview
This PR fixes lark-cli update misclassifying pnpm-managed installations by adding pnpm-aware install detection and a pnpm-based self-update path, keeping the existing self-replace/verify/skills-sync flow consistent across package managers.
Changes:
- Added pnpm as a first-class install method in self-update detection (including
.pnpmpath detection and pnpm shim heuristics). - Implemented a pnpm update runner (
pnpm add -g @larksuite/cli@<version>) and routedupdateto it when pnpm is detected. - Expanded unit tests to cover pnpm detection,
--checkauto-update state, pnpm install invocation, and pnpm-specific messaging.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/selfupdate/updater.go | Adds pnpm detection and a pnpm install runner for self-update. |
| internal/selfupdate/updater_test.go | Adds unit tests for pnpm path + shim detection and pnpm install args. |
| cmd/update/update.go | Routes pnpm installs to a pnpm update path and generalizes package-manager update messaging. |
| cmd/update/update_test.go | Adds update/check tests for pnpm flows and pnpm-specific hints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+335
to
+341
| func manualInstallHint(method selfupdate.InstallMethod, latest string) string { | ||
| manager := "npm" | ||
| if method == selfupdate.InstallPnpm { | ||
| manager = "pnpm" | ||
| } | ||
| return fmt.Sprintf("\nOr install via %s (note: skills will not be synced):\n %s\n npx skills add larksuite/cli -y -g # sync skills separately\n", manager, reinstallCommand(manager, latest)) | ||
| } |
Author
There was a problem hiding this comment.
Fixed in f4f62dd. pnpm manual fallback now uses pnpm dlx skills add larksuite/cli -y -g, while npm keeps npx skills add ....
Comment on lines
351
to
+355
| if updater.CanRestorePreviousVersion() { | ||
| return "the previous version has been restored" | ||
| } | ||
| 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)) | ||
| return fmt.Sprintf("automatic rollback is unavailable on this platform; reinstall manually (skills will not be synced): %s && npx skills add larksuite/cli -y -g, or download %s", reinstall, releaseURL(latest)) | ||
| } |
Author
There was a problem hiding this comment.
Fixed in f4f62dd. Verification failure hints now take the detected package manager and use pnpm dlx skills add ... for pnpm rollback/manual recovery guidance.
Comment on lines
+910
to
+923
| func TestManualInstallHintUsesDetectedPackageManager(t *testing.T) { | ||
| pnpmHint := manualInstallHint(selfupdate.InstallPnpm, "2.0.0") | ||
| if !strings.Contains(pnpmHint, "pnpm add -g @larksuite/cli@2.0.0") { | ||
| t.Fatalf("pnpm manual hint = %q, want pnpm add command", pnpmHint) | ||
| } | ||
| if strings.Contains(pnpmHint, "npm install -g") { | ||
| t.Fatalf("pnpm manual hint should not suggest npm install: %q", pnpmHint) | ||
| } | ||
|
|
||
| npmHint := manualInstallHint(selfupdate.InstallNpm, "2.0.0") | ||
| if !strings.Contains(npmHint, "npm install -g @larksuite/cli@2.0.0") { | ||
| t.Fatalf("npm manual hint = %q, want npm install command", npmHint) | ||
| } | ||
| } |
Author
There was a problem hiding this comment.
Fixed in f4f62dd. The manual hint test now asserts manager-specific skills sync commands and verifies pnpm hints do not suggest npm/npx.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
internal/selfupdate/updater_test.go (1)
218-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert argv boundaries, not only the joined command string.
"$*"flattens all arguments, so a broken implementation that passes"add -g@larksuite/cli@2.0.0"as one argv element can still produce the same log. Log each arg on its own line and compare the exact slice.Proposed test tightening
- if err := os.WriteFile(script, []byte("#!/bin/sh\nprintf '%s\\n' \"$*\" >> \""+logPath+"\"\nexit 0\n"), 0o755); err != nil { + if err := os.WriteFile(script, []byte("#!/bin/sh\nfor arg do printf '%s\\n' \"$arg\"; done >> \""+logPath+"\"\nexit 0\n"), 0o755); err != nil { t.Fatal(err) } @@ - if got, want := strings.TrimSpace(string(raw)), "add -g `@larksuite/cli`@2.0.0"; got != want { + if got, want := strings.Split(strings.TrimSpace(string(raw)), "\n"), []string{"add", "-g", "`@larksuite/cli`@2.0.0"}; !reflect.DeepEqual(got, want) { t.Fatalf("args = %q, want %q", got, want) }🤖 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/selfupdate/updater_test.go` around lines 218 - 232, The RunPnpmInstall test is only checking a flattened command string, so it can miss argv boundary bugs. Update the test around New().RunPnpmInstall to log each argument separately from the script and assert the exact argument slice, using the RunPnpmInstall symbol and the script fixture setup to verify “add”, “-g”, and “@larksuite/cli@2.0.0” are passed as distinct argv elements.cmd/update/update.go (1)
325-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParameter
outputshadows the importedoutputpackage.
packageManagerPermissionHint(manager, output string)names its second parameteroutput, shadowing theinternal/outputpackage alias used throughout this file. Currently harmless since the function body doesn't reference the package, but it's a landmine for anyone extending this function later.♻️ Suggested rename
-func packageManagerPermissionHint(manager, output string) string { +func packageManagerPermissionHint(manager, combinedOutput string) string { if manager == "npm" { - return permissionHint(output) + return permissionHint(combinedOutput) } - if manager == "pnpm" && strings.Contains(output, "EACCES") && !isWindows() { + if manager == "pnpm" && strings.Contains(combinedOutput, "EACCES") && !isWindows() { return "Permission denied. Check pnpm global directory permissions or run: pnpm setup" } return "" }🤖 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 325 - 333, The `packageManagerPermissionHint` function currently uses `output` as a parameter name, which shadows the imported `internal/output` package alias in this file. Rename that parameter to something non-conflicting, and update the `permissionHint` call site inside `packageManagerPermissionHint` accordingly; use the function name to locate the change.
🤖 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 910-923: The current tests only cover npm and pnpm success hints,
so add coverage for the missing manual/other path and pnpm-specific failure
branches. Extend the tests around manualInstallHint, doManualUpdate,
packageManagerPermissionHint, and doPackageManagerUpdate to verify that a
non-npm/non-pnpm detect.Method produces the true manual install guidance instead
of defaulting to npm, that pnpm EACCES returns the correct permission hint, and
that pnpm install/verification failures are exercised as explicit failure cases.
Use the existing TestManualInstallHintUsesDetectedPackageManager as the starting
point and add focused assertions for the new branches.
In `@cmd/update/update.go`:
- Around line 335-341: manualInstallHint currently defaults every non-pnpm case
to npm, which misleads the genuine manual/binary-install path. Update
manualInstallHint to distinguish selfupdate.InstallNpm from the manual/other
InstallMethod used by doManualUpdate/DetectResult.ManualReason, and only emit
npm/pnpm package-manager commands when the method actually matches those
managers. For the non-npm/non-pnpm case, return guidance that does not suggest
npm install -g, and use the existing manual-install flow in update.go to keep
the message consistent with the GitHub Releases download instructions.
In `@internal/selfupdate/updater.go`:
- Around line 229-242: The pnpm runner in updater.go is returning plain
fmt.Errorf errors, which drops the typed category/subtype metadata expected by
the update command. Update the error paths in the pnpm install flow around the
LookPath handling and the timeout check in the runner to use the appropriate
errs.* constructors, and attach the underlying failures with .WithCause(...).
Preserve both the missing-pnpm failure and the context deadline timeout as typed
command-facing errors, with the timeout path carrying context.DeadlineExceeded
as its cause.
- Around line 164-172: In detectInstallMethod, the pnpm-specific path check is
too narrow and the generic "/node_modules/" branch can misclassify pnpm global
shim targets as npm. Update the logic so detectInstallMethod recognizes pnpm
global target shapes like the one covered in updater_test before falling back to
InstallNpm, keeping the pnpm check ahead of the generic node_modules match and
extending it to cover the resolved global target path pattern used by pnpm.
---
Nitpick comments:
In `@cmd/update/update.go`:
- Around line 325-333: The `packageManagerPermissionHint` function currently
uses `output` as a parameter name, which shadows the imported `internal/output`
package alias in this file. Rename that parameter to something non-conflicting,
and update the `permissionHint` call site inside `packageManagerPermissionHint`
accordingly; use the function name to locate the change.
In `@internal/selfupdate/updater_test.go`:
- Around line 218-232: The RunPnpmInstall test is only checking a flattened
command string, so it can miss argv boundary bugs. Update the test around
New().RunPnpmInstall to log each argument separately from the script and assert
the exact argument slice, using the RunPnpmInstall symbol and the script fixture
setup to verify “add”, “-g”, and “@larksuite/cli@2.0.0” are passed as distinct
argv elements.
🪄 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: 15970cf7-f9d4-4446-bf38-1333cab7747e
📒 Files selected for processing (4)
cmd/update/update.gocmd/update/update_test.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.go
|
|
Author
|
Addressed the latest review feedback in f4f62dd. Changes made:
Verification:
|
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
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
Fixes #1681 by recognizing pnpm-managed installations in
lark-cli updateand using pnpm for automatic self-update instead of falling back to manual/npm-only update guidance.Changes
.pnpmglobal paths and pnpm command shims.--checkand update decisions report the correctauto_updatestate.RunPnpmInstall, which runspnpm add -g @larksuite/cli@<version>when the CLI was installed through pnpm.--check, and manual install hints.Test Plan
go test ./internal/selfupdate ./cmd/updatego vet ./...gofmt -l .go mod tidyfollowed bygit diff -- go.mod go.sumwith no diffgo run github.com/golangci/golangci-lint/v2/cmd/golangci-lint@v2.1.6 run --new-from-rev=origin/main(0 issues; existing config warning only)make script-testmake examples-buildgo test -v -count=1 ./tests/cli_e2e/... -run 'DryRun|StdinRegression|ResolveBinaryPath|BuildArgs|SkipWithoutUserToken|EventConsumeUnknownKeyRegression|EventSubscribeInvalidRouteRegression|GitCredentialListLocal|GitCredentialRemoveLocal'make unit-testwas attempted. It did not complete green for environment/unrelated reasons outside this change:internal/event/consumefailed once withdial tcp 127.0.0.1:<port>: connect: can't assign requested address; rerunninggo test -race -gcflags="all=-N -l" -count=1 ./internal/event/consumepassed.shortcuts/minutesfails because this machine resolvesexample.comto198.18.4.33, so URL safety rejects fixture download URLs as local/internal. Confirmed withdscacheutil -q host -a name example.comand Pythonsocket.getaddrinfo('example.com', 443).make integration-testwas attempted. The build step succeeded and dry-run/local tests passed, but live E2E is blocked by the current test credentials/scopes, including missingbase:app:create,drive:drive,space:folder:create,wiki:*, and user IM scopes such asim:chat:create_by_user/im:feed.shortcut:*.Related Issues
Summary by CodeRabbit
New Features
Bug Fixes