fix(selfupdate): use LookPath instead of Executable for binary verification (fixes #836)#886
Conversation
…cation (fixes larksuite#836) VerifyBinary was using vfs.Executable() to find the binary to run --version against. On Linux with global npm install, this returns the inode of the running binary (old version), not the newly installed one that sits behind npm's bin symlink. Switch to exec.LookPath("lark-cli") which resolves the PATH entry and follows npm's bin symlink to the correct newly installed version, matching what the user actually runs.
|
|
|
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:
📝 WalkthroughWalkthroughVerifyBinary now resolves the executable by calling execLookPath("lark-cli") directly; tests add a package-level execLookPath hook and three lookup-focused test cases (success, not found, empty output). ChangesBinary verification executable resolution
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
github-actions
Bot
added
the
size/M
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #886 +/- ##
==========================================
+ Coverage 65.89% 65.91% +0.02%
==========================================
Files 518 520 +2
Lines 48945 49277 +332
==========================================
+ Hits 32254 32483 +229
- Misses 13921 14022 +101
- Partials 2770 2772 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b9f20192f713a47e063333ee715275398fbc8049🧩 Skill updatenpx skills add CatfishGG/cli#fix-issue-836 -y -g |
|
Hi @CatfishGG, thanks for the fix 👍 The direction matches suggestion #1 from issue #836. One thing blocking merge: the CLA hasn't been signed yet. Please sign at https://cla-assistant.io/larksuite/cli?pullRequest=886 — once |
Add TestVerifyBinaryLookPath, TestVerifyBinaryLookPathNotFound, and TestVerifyBinaryEmptyOutput. Expose execLookPath variable so tests can inject a mock LookPath and cover the full VerifyBinary execution path including version parsing and error branches.
|
Thanks for the review @liangshuo-1! CLA is now signed (see the green license/cla check). I've also just pushed a fix for the 50% patch coverage issue — added tests that inject a mock LookPath to cover the full VerifyBinary execution path. PTAL when you get a chance 👍 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/selfupdate/updater_test.go (1)
6-14:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd missing
os/execimport.Line 47 uses
exec.LookPath, which requires importingos/exec. The import is not present in the file's import block (lines 6-14), causing a compile failure.Proposed fix
import ( "fmt" + "os/exec" "os" "path/filepath" "runtime" "testing" "github.com/larksuite/cli/internal/vfs" )🤖 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 6 - 14, The test file is missing the os/exec import used by exec.LookPath; update the import block in internal/selfupdate/updater_test.go to include "os/exec" so calls to exec.LookPath resolve (locate the import alongside existing imports like "os", "path/filepath", "runtime", "testing" and "github.com/larksuite/cli/internal/vfs").
🤖 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_test.go`:
- Around line 74-128: The new tests (TestVerifyBinaryLookPath,
TestVerifyBinaryLookPathNotFound, TestVerifyBinaryEmptyOutput) do not isolate
CLI config state; add t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) at the
start of each test (before calling New() or anything that may read config) so
the calls to New().VerifyBinary use an isolated config directory and avoid
cross-test coupling.
---
Outside diff comments:
In `@internal/selfupdate/updater_test.go`:
- Around line 6-14: The test file is missing the os/exec import used by
exec.LookPath; update the import block in internal/selfupdate/updater_test.go to
include "os/exec" so calls to exec.LookPath resolve (locate the import alongside
existing imports like "os", "path/filepath", "runtime", "testing" and
"github.com/larksuite/cli/internal/vfs").
🪄 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: 5125e095-c98d-429b-80af-e3a62faf75f6
📒 Files selected for processing (2)
internal/selfupdate/updater.gointernal/selfupdate/updater_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/selfupdate/updater.go
…Binary tests CodeRabbit feedback: - Add missing os/exec import for execLookPath variable - Add t.Setenv(LARKSUITE_CLI_CONFIG_DIR, ...) to each new test for config isolation
Move the execLookPath variable declaration to its own file so it is accessible to updater.go without the test-only import cycle.
- Move execLookPath into updater.go (drops redundant lookpath.go) - Document package-level mock: no t.Parallel() - Extend TestVerifyBinaryLookPath with exact-match regressions (0.0, 12.1.0 vs 2.1.0) Fixes fast-gate gofmt failure on PR larksuite#886. Change-Id: Ia6c784754fa8c8a917df7bf58fe821a23b017945 Co-authored-by: Cursor <cursoragent@cursor.com>
Claude F4 / option A: keep LookPath first for npm bin symlink (larksuite#836), but restore Executable fallback for absolute-path launches without PATH entry. - Adjust LookPath-not-found test to mock DefaultFS Executable - Add TestVerifyBinaryFallbackExecutableWhenNotOnPath Change-Id: If8acf96f757c3454ba2f670a1fbbc0ae511c93e5 Co-authored-by: Cursor <cursoragent@cursor.com>
Process note (review-side)During review, I pushed two commits directly to this PR branch on your fork (`4f03752`, `b9f2019`) to address CI (gofmt / fast-gate), version-matching regression tests, and the `LookPath` vs `vfs.Executable` fallback discussed in review. That was a mistake on process: I should not have updated your fork without your explicit OK. Sorry for bypassing your ownership of the branch. Please treat those commits as optional: you are welcome to `revert`, `reset`, `cherry-pick`, squash, or re-implement the same changes yourself. If you prefer, you can ask maintainers to land follow-up fixes via a separate branch/PR instead. Going forward (my side): for external-contributor PRs I will stick to review comments and patches in the thread, or a maintainer-owned follow-up PR—not direct pushes to the author’s fork unless you’ve asked for that explicitly. Again, apologies for the churn. |
|
Hey @liangshuo-1 no apology needed, really. The commits you pushed were exactly what the PR needed: the gofmt fix, the version-matching regression tests, and the LookPath + vfs.Executable fallback that made the whole thing robust. All of that was genuinely helpful. If anything, the process was refreshingly direct, you saw what needed fixing and just did it rather than going back and forth endlessly. Much appreciated. As for going forward: your proposed approach (review comments + patches in-thread, or a maintainer follow-up PR) makes total sense for larger changes. For small fixes like this one, direct pushes with a heads-up work fine too, so no need to be overly formal about it. Thanks for the smooth experience overall, and glad the PR landed cleanly in main. |
3 participants
Summary
Issue: #836 —
lark-cli updatereports "binary verification failed" on Linux global npm install even though the new version is correctly installed and working.Root Cause
VerifyBinary()ininternal/selfupdate/updater.gowas usingvfs.Executable()(wrappingos.Executable()) as the primary way to locate the binary to run--versionagainst.On Linux,
os.Executable()returns the inode path of the currently running binary — the old version. After npm installs the new version tonode_modules/.bin/lark-cli, the running process is still the old binary at its original inode path. The verification step was runninglark-cli --versionon the old binary, which reported the old version → verification failed even though the new version was correctly installed and fully functional.Fix
Flip the lookup priority: use
exec.LookPath("lark-cli")as the primary mechanism instead ofvfs.Executable().LookPathsearches PATH and follows symlinks — on Linux with a global npm install, the PATH entry forlark-cliresolves through npm's bin symlink to the newly installed version. This is exactly what the user gets when they runlark-clinormally, and matches the suggested fix in the issue ("verify the final resolved executable after npm install").The old comment even noted this was a known edge case but chose the wrong approach. The fix aligns with the issue's own suggested direction.
Changes
vfs.Executable()as primary lookup inVerifyBinaryLookPathfallback (now the primary and only lookup)Testing
go test ./internal/selfupdate/...andgo test ./cmd/update/...— all pass.Fixes #836
Summary by CodeRabbit
Bug Fixes
Tests