fix: support pnpm global installs in self-update#1705
Merged
Conversation
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:
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 (3)
📝 WalkthroughWalkthroughThis PR adds pnpm-aware update handling across detection, install execution, update messaging, and skills sync invocation. Changespnpm-aware update support
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant updateRun
participant Updater
participant pnpm
participant skills
User->>updateRun: lark-cli update
updateRun->>Updater: DetectInstallMethod()
Updater-->>updateRun: install method + pnpm availability
updateRun->>Updater: RunNpmInstall() or RunPnpmInstall()
Updater->>pnpm: install `@larksuite/cli`@version
pnpm-->>Updater: install result
updateRun->>skills: runSkillsCommand()
skills-->>updateRun: sync result
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1705 +/- ##
==========================================
+ Coverage 74.40% 74.42% +0.02%
==========================================
Files 854 854
Lines 88375 88445 +70
==========================================
+ Hits 65752 65825 +73
+ Misses 17552 17546 -6
- Partials 5071 5074 +3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@568883ee18f2e22ea80fd28ac03e54d142d8611f🧩 Skill updatenpx skills add larksuite/cli#fix/pnpm-selfupdate-detection -y -g |
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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 `@internal/selfupdate/updater.go`:
- Around line 346-349: The launcher lookup path in updater.go is returning an
untyped fmt.Errorf, which breaks command-facing error classification. Update the
exec.LookPath handling in the updater logic to construct the failure with the
appropriate errs.* typed error for a missing launcher, and attach the original
exec.LookPath error via .WithCause(err) so the cause is preserved. Use the
existing launcher/binPath lookup flow in the updater function to locate the
change, and keep r.Err set to the typed error before returning.
- Around line 171-173: The pnpm detection in containsPnpmMarker is too broad
because matching plain /pnpm/ or \pnpm\ can misclassify npm installs as
InstallPnpm. Tighten the check in containsPnpmMarker to only recognize
pnpm-specific paths such as .pnpm or the documented pnpm/store layout, and keep
the existing cross-platform path handling intact.
🪄 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: ba320ccf-eae8-43db-96f5-422208b02fe1
📒 Files selected for processing (3)
cmd/update/update.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/selfupdate/updater_test.go
- cmd/update/update.go
leave330
force-pushed
the
fix/pnpm-selfupdate-detection
branch
from
a25525d to
bfb4817
Compare
leave330
force-pushed
the
fix/pnpm-selfupdate-detection
branch
from
df60cd3 to
568883e
Compare
liangshuo-1
approved these changes
Jul 6, 2026
This was referenced Jul 6, 2026
Closed
Merged
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
3 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 updateclassified pnpm global installs as npm because both use anode_modules/@larksuite/clilayout. When npm was also onPATH, the updater rannpm install -g, creating a duplicate install under npm's global prefix instead of updating the pnpm-managed one (#1681). Users ended up with twolark-clibinaries of different versions, and which one ran depended onPATHorder.This detects pnpm from the resolved binary path and updates in place with
pnpm add -g @larksuite/cli@<version>. When pnpm is detected but not onPATH, it falls back to the existing manual path (GitHub Releases URL) with a pnpm-specific hint. The npm and manual paths are unchanged.Detection matches a
.pnpmpath segment, or apnpmsegment immediately followed bystore— covering both pnpm layouts (the classic virtual storenode_modules/.pnpm/…and pnpm 11's global content-addressable store~/Library/pnpm/store/v11/links/…) without misclassifying an npm install that merely lives under a directory namedpnpm. Verified end-to-end on a real pnpm 11 / macOS install (the environment reported in #1681):updateprintsvia pnpmand runspnpm add -gin place, and skills sync then runs and reports its launcher.Changes
internal/selfupdate/updater.go: addInstallPnpmdetection viacontainsPnpmMarker(matches apnpmor.pnpmpath segment, POSIX and Windows separators), aPnpmAvailablefield, andRunPnpmInstall(pnpm add -g, argv form). Classification is refactored into a pure, unit-testabledetectFromResolved.cmd/update/update.go: route auto-updates through the detected package manager (doNpmUpdate→ parameterizeddoAutoUpdate), show a pnpm-specific hint on the manual fallback, report the actual package manager in the failure message, and document the pnpm branch in--help. The JSON envelope key set is unchanged (pnpm success isaction: "updated").internal/selfupdate/updater.go: post-update skills sync is now package-manager-aware —runSkillsCommandusespnpm dlx skills …when the CLI is a pnpm install and pnpm is available, otherwisenpx skills …(selection extracted into a pure, unit-testedskillsInvocation). This addresses pnpm-only environments: pnpm's standalone installer bundles Node without puttingnpm/npxonPATH, so an unconditionalnpx skills addwould fail withnpx not found in PATHright after a successful CLI update. The manual-fallback "sync skills separately" hint is aligned topnpm dlxon the pnpm branch, and the update output reports the skills launcher (Updating skills via pnpm dlx/npx ...).internal/selfupdate/updater.go:DetectInstallMethodnow memoizes its first result on theUpdater. The update flow mutates the install (pnpm add -g) before syncing skills; without caching, the skills-time re-detection could resolve a now-staleos.Executable()and silently fall back tonpx, diverging from the launcher reported to the user. Caching is the correct semantics (install method is process-invariant) and keeps the reported and executed launcher consistent; it also avoids repeatedEvalSymlinks+LookPathper skills command.cmd/update/update.go: the EACCES permission hint and the no-rollback reinstall hint are now package-manager-aware — pnpm users getpnpm setup/pnpm add -g … && pnpm dlx skills add …instead of npm-flavored advice that would otherwise send them toward a second duplicate install.Known limitation: a non-default pnpm setup whose resolved path passes through neither a
.pnpmvirtual-store segment nor thepnpmhome/store directory would still be classified as npm. Default and common layouts (virtual store and global store) are covered.Test Plan
containsPnpmMarkerboundaries (virtual store, global store on macOS/Linux/Windows, and.pnpmfoo/pnpmfoonegatives),RunPnpmInstallsuccess/error, and command-layer pnpm success / install-failure / pnpm-unavailable manual fallback (asserts the npm install path is not invoked on the pnpm branch)~/Library/pnpm/store/v11/links/…/@larksuite/cli/bin/lark-cli update) — detectedvia pnpmand updated in place withpnpm add -g; alsolark-cli update --check --json/--help(non-destructive)Related Issues
Closes #1681
Summary by CodeRabbit
New Features
pnpm dlxfor skills when available.Bug Fixes
Tests
skillslauncher argument handling.