From 80c00a16e126fe031c7a61183b6c72fe8f3d51b0 Mon Sep 17 00:00:00 2001 From: AnnatarHe Date: Sun, 17 May 2026 02:35:34 +0800 Subject: [PATCH] fix(daemon): prefer Homebrew binary over curl-installer location Reorder ResolveDaemonBinaryPath to check PATH/Homebrew before ~/.shelltime/bin/shelltime-daemon, so that brew-managed installs actually drive the running daemon. On install, remove the stale curl-installer daemon when a system-managed binary is being used. Co-Authored-By: Claude Opus 4.7 (1M context) --- commands/daemon.install.go | 16 +++++- model/path.go | 52 +++++++++++------ model/path_test.go | 114 +++++++++++++++++++++++++++++++++++++ 3 files changed, 163 insertions(+), 19 deletions(-) diff --git a/commands/daemon.install.go b/commands/daemon.install.go index 35a3150..9c47f16 100644 --- a/commands/daemon.install.go +++ b/commands/daemon.install.go @@ -39,7 +39,7 @@ func commandDaemonInstall(c *cli.Context) error { } } - // Resolve daemon binary (curl-installer, Homebrew, or PATH) + // Resolve daemon binary (Homebrew/PATH preferred, curl-installer fallback) daemonBinPath, err := model.ResolveDaemonBinaryPath() if err != nil { color.Yellow.Println("⚠️ shelltime-daemon not found.") @@ -49,6 +49,20 @@ func commandDaemonInstall(c *cli.Context) error { } color.Green.Printf("✅ Found daemon binary at: %s\n", daemonBinPath) + // If we picked a system-managed binary but a stale curl-installer copy + // still lives under ~/.shelltime/bin, remove it so future resolution + // stays unambiguous. + curlDaemonPath := model.GetCurlInstallerDaemonPath() + if daemonBinPath != curlDaemonPath { + if info, statErr := os.Stat(curlDaemonPath); statErr == nil && !info.IsDir() { + if rmErr := os.Remove(curlDaemonPath); rmErr == nil { + color.Yellow.Printf("🧹 Removed stale curl-installer daemon at %s\n", curlDaemonPath) + } else { + color.Yellow.Printf("⚠️ Could not remove stale daemon at %s: %v\n", curlDaemonPath, rmErr) + } + } + } + installer, err := model.NewDaemonInstaller(baseFolder, username, daemonBinPath) if err != nil { return err diff --git a/model/path.go b/model/path.go index cc52929..0d206c5 100644 --- a/model/path.go +++ b/model/path.go @@ -114,34 +114,50 @@ func GetDaemonErrFilePath() string { return GetStoragePath("logs", "shelltime-daemon.err") } +// GetCurlInstallerDaemonPath returns the legacy curl-installer daemon location +// (~/.shelltime/bin/shelltime-daemon). +func GetCurlInstallerDaemonPath() string { + return filepath.Join(GetBaseStoragePath(), "bin", "shelltime-daemon") +} + +// daemonHomebrewSearchPaths lists explicit Homebrew/Linuxbrew bin dirs to +// probe when PATH is stripped (e.g. launchd-spawned shells). Exposed as a var +// so tests can swap it out. +var daemonHomebrewSearchPaths = []string{ + "/opt/homebrew/bin", + "/usr/local/bin", + "/home/linuxbrew/.linuxbrew/bin", +} + // ResolveDaemonBinaryPath finds the shelltime-daemon binary. -// It checks the curl-installer location first, then PATH (Homebrew), -// then known Homebrew prefix paths as fallback. +// It prefers a system-managed binary (Homebrew or anything on PATH) over the +// legacy curl-installer location, so that `brew upgrade shelltime` is what +// actually drives the running daemon. func ResolveDaemonBinaryPath() (string, error) { const binaryName = "shelltime-daemon" + curlPath := GetCurlInstallerDaemonPath() - // 1. Check curl-installer location - curlPath := filepath.Join(GetBaseStoragePath(), "bin", binaryName) - if info, err := os.Stat(curlPath); err == nil && !info.IsDir() { - return curlPath, nil - } - - // 2. Check PATH (covers Homebrew and other package managers) + // 1. Check PATH (covers Homebrew and other package managers). Ignore the + // result if it happens to resolve to the curl-installer path — we want + // step 3 to be the only branch that returns that location. if path, err := exec.LookPath(binaryName); err == nil { - return path, nil + if resolved, _ := filepath.Abs(path); resolved != curlPath { + return path, nil + } } - // 3. Explicit Homebrew fallback paths - homebrewPaths := []string{ - filepath.Join("/opt/homebrew/bin", binaryName), - filepath.Join("/usr/local/bin", binaryName), - filepath.Join("/home/linuxbrew/.linuxbrew/bin", binaryName), - } - for _, p := range homebrewPaths { + // 2. Explicit Homebrew/Linuxbrew fallback paths, in case PATH was stripped. + for _, dir := range daemonHomebrewSearchPaths { + p := filepath.Join(dir, binaryName) if info, err := os.Stat(p); err == nil && !info.IsDir() { return p, nil } } - return "", fmt.Errorf("%s not found at %s, on PATH, or in standard Homebrew locations", binaryName, curlPath) + // 3. Curl-installer fallback. + if info, err := os.Stat(curlPath); err == nil && !info.IsDir() { + return curlPath, nil + } + + return "", fmt.Errorf("%s not found on PATH, in standard Homebrew locations, or at %s", binaryName, curlPath) } diff --git a/model/path_test.go b/model/path_test.go index d26933d..93d0e27 100644 --- a/model/path_test.go +++ b/model/path_test.go @@ -289,3 +289,117 @@ func TestPathsAreClean(t *testing.T) { } } } + +// writeFakeDaemon creates an executable file at the given path so exec.LookPath +// will accept it as resolvable. Returns the absolute path actually written. +func writeFakeDaemon(t *testing.T, dir string) string { + t.Helper() + if err := os.MkdirAll(dir, 0o755); err != nil { + t.Fatalf("failed to mkdir %s: %v", dir, err) + } + p := filepath.Join(dir, "shelltime-daemon") + if err := os.WriteFile(p, []byte("#!/bin/sh\nexit 0\n"), 0o755); err != nil { + t.Fatalf("failed to write fake daemon: %v", err) + } + return p +} + +// withIsolatedDaemonResolution sets up a hermetic environment for +// ResolveDaemonBinaryPath: a temp $HOME, an empty $PATH (caller can re-add +// dirs), and an empty Homebrew search list so the host machine's installs +// (e.g. real /opt/homebrew/bin/shelltime-daemon) don't leak into assertions. +// Returns the temp home directory. +func withIsolatedDaemonResolution(t *testing.T) string { + t.Helper() + home := t.TempDir() + t.Setenv("HOME", home) + t.Setenv("PATH", "") + + prev := daemonHomebrewSearchPaths + daemonHomebrewSearchPaths = nil + t.Cleanup(func() { daemonHomebrewSearchPaths = prev }) + + return home +} + +func TestResolveDaemonBinaryPath(t *testing.T) { + t.Run("returns curl-installer path when nothing else is on PATH", func(t *testing.T) { + home := withIsolatedDaemonResolution(t) + + curl := writeFakeDaemon(t, filepath.Join(home, COMMAND_BASE_STORAGE_FOLDER, "bin")) + + got, err := ResolveDaemonBinaryPath() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != curl { + t.Errorf("expected %s, got %s", curl, got) + } + }) + + t.Run("prefers PATH-resolved binary over curl-installer path", func(t *testing.T) { + home := withIsolatedDaemonResolution(t) + + brewDir := t.TempDir() + brewPath := writeFakeDaemon(t, brewDir) + t.Setenv("PATH", brewDir) + + // stale curl-installer copy that should be ignored + writeFakeDaemon(t, filepath.Join(home, COMMAND_BASE_STORAGE_FOLDER, "bin")) + + got, err := ResolveDaemonBinaryPath() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != brewPath { + t.Errorf("expected PATH-resolved %s, got %s", brewPath, got) + } + }) + + t.Run("uses explicit Homebrew search list when PATH is empty", func(t *testing.T) { + home := withIsolatedDaemonResolution(t) + + brewDir := t.TempDir() + brewPath := writeFakeDaemon(t, brewDir) + daemonHomebrewSearchPaths = []string{brewDir} + + // stale curl-installer copy that should be ignored + writeFakeDaemon(t, filepath.Join(home, COMMAND_BASE_STORAGE_FOLDER, "bin")) + + got, err := ResolveDaemonBinaryPath() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != brewPath { + t.Errorf("expected Homebrew search result %s, got %s", brewPath, got) + } + }) + + t.Run("skips PATH result that points back at the curl-installer path", func(t *testing.T) { + home := withIsolatedDaemonResolution(t) + + curlBin := filepath.Join(home, COMMAND_BASE_STORAGE_FOLDER, "bin") + curl := writeFakeDaemon(t, curlBin) + // Put only the curl-installer dir on PATH; LookPath would return curl, + // but the resolver must fall through and still return the curl path + // from step 3 (so the cleanup logic in daemon.install can detect it). + t.Setenv("PATH", curlBin) + + got, err := ResolveDaemonBinaryPath() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got != curl { + t.Errorf("expected curl-installer fallback %s, got %s", curl, got) + } + }) + + t.Run("returns error when no daemon is found anywhere", func(t *testing.T) { + withIsolatedDaemonResolution(t) + + _, err := ResolveDaemonBinaryPath() + if err == nil { + t.Fatal("expected error when no daemon binary exists, got nil") + } + }) +}