fix(selfupdate): use LookPath instead of Executable for binary verification (fixes #836) (#886)

* fix(selfupdate): use LookPath instead of Executable for binary verification (fixes #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.

* test(selfupdate): add LookPath-based tests for VerifyBinary

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.

* test(selfupdate): add os/exec import and isolate config dir in VerifyBinary 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

* test(selfupdate): extract execLookPath to separate lookpath.go

Move the execLookPath variable declaration to its own file so it is
accessible to updater.go without the test-only import cycle.

* fix(selfupdate): remove unused os/exec import from test file

* fix(selfupdate): gofmt + fold lookpath hook and restore version fences

- 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)

Co-authored-by: CatfishGG <catfishgg@users.noreply.github.com>

internal/selfupdate/updater.go

@@ -17,6 +17,13 @@ import (
 	"github.com/larksuite/cli/internal/vfs"
 )
 
+// execLookPath is the LookPath implementation used by VerifyBinary.
+// It defaults to the standard library exec.LookPath but is swapped in tests
+// via lookPathMock to provide controlled binary resolution.
+//
+// Tests that mutate execLookPath must not call t.Parallel().
+var execLookPath = exec.LookPath
+
 // InstallMethod describes how the CLI was installed.
 type InstallMethod int
 
@@ -186,13 +193,13 @@ func (u *Updater) VerifyBinary(expectedVersion string) error {
 	if u.VerifyOverride != nil {
 		return u.VerifyOverride(expectedVersion)
 	}
-	// Prefer the current executable path (what the user actually launched).
-	// Use Executable() directly without EvalSymlinks — after npm install the
-	// symlink target may have changed, but the path itself is still valid for
-	// execution. Fall back to LookPath only if Executable() fails entirely.
-	exe, err := vfs.Executable()
+	// Prefer PATH resolution so npm global bin symlinks pick up the newly
+	// installed binary (#836). If `lark-cli` is not on PATH (e.g. the user
+	// invoked this process by absolute path), fall back to the running
+	// executable — same as the pre-#836 secondary resolution path.
+	exe, err := execLookPath("lark-cli")
 	if err != nil {
-		exe, err = exec.LookPath("lark-cli")
+		exe, err = vfs.Executable()
 		if err != nil {
 			return fmt.Errorf("cannot locate binary: %w", err)
 		}

internal/selfupdate/updater_test.go

@@ -4,6 +4,7 @@
 package selfupdate
 
 import (
+	"fmt"
 	"os"
 	"path/filepath"
 	"runtime"
@@ -12,6 +13,7 @@ import (
 	"github.com/larksuite/cli/internal/vfs"
 )
 
+// executableTestFS mocks vfs for tests that still need vfs.Executable.
 type executableTestFS struct {
 	vfs.OsFs
 	exe string
@@ -19,6 +21,28 @@ type executableTestFS struct {
 
 func (f executableTestFS) Executable() (string, error) { return f.exe, nil }
 
+// lookPathMock patches execLookPath within VerifyBinary for controlled testing.
+// Do not use t.Parallel() in tests that install this mock — it mutates a package-level var.
+type lookPathMock struct {
+	oldLookPath func(string) (string, error)
+	result      string
+	resultErr   error
+}
+
+func (m *lookPathMock) install(bin string) {
+	m.oldLookPath = execLookPath
+	execLookPath = func(name string) (string, error) {
+		if name == bin {
+			return m.result, m.resultErr
+		}
+		return m.oldLookPath(name)
+	}
+}
+
+func (m *lookPathMock) restore() {
+	execLookPath = m.oldLookPath
+}
+
 func TestResolveExe(t *testing.T) {
 	u := New()
 	p, err := u.resolveExe()
@@ -44,46 +68,101 @@ func TestCleanupStaleFiles_NoPanic(t *testing.T) {
 	u.CleanupStaleFiles()
 }
 
-func TestVerifyBinaryChecksVersion(t *testing.T) {
+func TestVerifyBinaryLookPath(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
 	if runtime.GOOS == "windows" {
 		t.Skip("uses a POSIX shell script")
 	}
 
 	dir := t.TempDir()
-	exe := filepath.Join(dir, "lark-cli")
-	// Script prints version string matching real CLI format when --version is passed.
-	script := "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then echo \"lark-cli version 2.0.0\"; exit 0; fi\nexit 12\n"
-	if err := os.WriteFile(exe, []byte(script), 0755); err != nil {
+	bin := filepath.Join(dir, "lark-cli")
+	script := "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then echo \"lark-cli version 2.1.0\"; exit 0; fi\nexit 12\n"
+	if err := os.WriteFile(bin, []byte(script), 0755); err != nil {
 		t.Fatalf("write test binary: %v", err)
 	}
 
-	// Mock vfs.Executable to return our test script, matching VerifyBinary's
-	// primary lookup path. Also prepend to PATH for the LookPath fallback.
-	origFS := vfs.DefaultFS
-	vfs.DefaultFS = executableTestFS{OsFs: vfs.OsFs{}, exe: exe}
-	t.Cleanup(func() { vfs.DefaultFS = origFS })
+	mock := &lookPathMock{result: bin}
+	mock.install("lark-cli")
+	t.Cleanup(mock.restore)
 
-	origPath := os.Getenv("PATH")
-	t.Setenv("PATH", dir+string(os.PathListSeparator)+origPath)
-
-	// Matching version → success.
-	if err := New().VerifyBinary("2.0.0"); err != nil {
-		t.Fatalf("VerifyBinary(matching) error = %v, want nil", err)
+	if err := New().VerifyBinary("2.1.0"); err != nil {
+		t.Fatalf("VerifyBinary(2.1.0) error = %v, want nil", err)
 	}
 
-	// Mismatched version → error.
 	if err := New().VerifyBinary("3.0.0"); err == nil {
 		t.Fatal("VerifyBinary(mismatched) expected error, got nil")
 	}
 
-	// Substring of actual version must not match (e.g. "0.0" is in "2.0.0").
+	// Regression: version must match exactly (not substring / prefix).
 	if err := New().VerifyBinary("0.0"); err == nil {
-		t.Fatal("VerifyBinary(substring) expected error, got nil")
+		t.Fatal("VerifyBinary(substring-style mismatch) expected error, got nil")
 	}
-
-	// Version that is a prefix of actual must not match (e.g. "2.0.0" in "12.0.0").
-	// Binary reports "2.0.0", asking for "12.0.0" must fail.
-	if err := New().VerifyBinary("12.0.0"); err == nil {
-		t.Fatal("VerifyBinary(prefix-mismatch) expected error, got nil")
+	if err := New().VerifyBinary("12.1.0"); err == nil {
+		t.Fatal("VerifyBinary(prefix-style mismatch) expected error, got nil")
+	}
+}
+
+func TestVerifyBinaryLookPathNotFound(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
+	mock := &lookPathMock{result: "", resultErr: fmt.Errorf("not found")}
+	mock.install("lark-cli")
+	t.Cleanup(mock.restore)
+
+	oldFS := vfs.DefaultFS
+	t.Cleanup(func() { vfs.DefaultFS = oldFS })
+	// Without this, VerifyBinary would fall back to the real test binary, which
+	// is not a lark-cli --version implementation.
+	vfs.DefaultFS = executableTestFS{exe: filepath.Join(t.TempDir(), "missing-lark-cli")}
+
+	if err := New().VerifyBinary("2.0.0"); err == nil {
+		t.Fatal("VerifyBinary(not-found) expected error, got nil")
+	}
+}
+
+func TestVerifyBinaryFallbackExecutableWhenNotOnPath(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
+	if runtime.GOOS == "windows" {
+		t.Skip("uses a POSIX shell script")
+	}
+
+	dir := t.TempDir()
+	bin := filepath.Join(dir, "lark-cli-abs")
+	script := "#!/bin/sh\nif [ \"$1\" = \"--version\" ]; then echo \"lark-cli version 2.1.0\"; exit 0; fi\nexit 12\n"
+	if err := os.WriteFile(bin, []byte(script), 0o755); err != nil {
+		t.Fatalf("write test binary: %v", err)
+	}
+
+	mock := &lookPathMock{result: "", resultErr: fmt.Errorf("not on PATH")}
+	mock.install("lark-cli")
+	t.Cleanup(mock.restore)
+
+	oldFS := vfs.DefaultFS
+	t.Cleanup(func() { vfs.DefaultFS = oldFS })
+	vfs.DefaultFS = executableTestFS{exe: bin}
+
+	if err := New().VerifyBinary("2.1.0"); err != nil {
+		t.Fatalf("VerifyBinary(fallback executable) error = %v, want nil", err)
+	}
+}
+
+func TestVerifyBinaryEmptyOutput(t *testing.T) {
+	t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir())
+	if runtime.GOOS == "windows" {
+		t.Skip("uses a POSIX shell script")
+	}
+
+	dir := t.TempDir()
+	bin := filepath.Join(dir, "lark-cli")
+	script := "#!/bin/sh\necho\nexit 0\n"
+	if err := os.WriteFile(bin, []byte(script), 0755); err != nil {
+		t.Fatalf("write test binary: %v", err)
+	}
+
+	mock := &lookPathMock{result: bin}
+	mock.install("lark-cli")
+	t.Cleanup(mock.restore)
+
+	if err := New().VerifyBinary("2.0.0"); err == nil {
+		t.Fatal("VerifyBinary(empty output) expected error, got nil")
 	}
 }