Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 15 additions & 1 deletion commands/daemon.install.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.")
Expand All @@ -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)
}
}
}
Comment on lines +56 to +64
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Comparing daemonBinPath and curlDaemonPath via string equality might be insufficient if one is a symlink to the other. If daemonBinPath is a symlink pointing to curlDaemonPath, deleting the latter will break the installation as the binary will be missing. It is safer to use os.SameFile to verify they are distinct physical files before attempting removal.

Suggested change
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)
}
}
}
curlDaemonPath := model.GetCurlInstallerDaemonPath()
if info, err := os.Stat(curlDaemonPath); err == nil && !info.IsDir() {
// Ensure we don't delete the binary if the resolved path is just a symlink to it
if binInfo, err := os.Stat(daemonBinPath); err == nil && !os.SameFile(info, binInfo) {
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
Expand Down
52 changes: 34 additions & 18 deletions model/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
}
Comment on lines 143 to 147
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current check resolved != curlPath only performs a string comparison, which may fail to identify the curl-installer binary if symlinks are involved (e.g., if a path in PATH is a symlink to the .shelltime/bin location). Using os.SameFile is a more robust way to ensure that the 'system-managed' binary isn't actually the one we're trying to avoid in this step. Additionally, returning the absolute path ensures consistency across all resolution branches.

	if path, err := exec.LookPath(binaryName); err == nil {
		if absPath, err := filepath.Abs(path); err == nil {
			if info, err := os.Stat(absPath); err == nil {
				curlInfo, err := os.Stat(curlPath)
				if err != nil || !os.SameFile(info, curlInfo) {
					return absPath, 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() {
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject non-executable fallback binaries

ResolveDaemonBinaryPath now checks hard-coded Homebrew/Linuxbrew locations before the curl-installer fallback, but this branch only uses os.Stat and treats any non-directory file as valid. In environments where PATH is empty/stripped (the exact case this fallback is meant to handle), a non-executable file at /opt/homebrew/bin/shelltime-daemon (for example after permission drift or a partial upgrade) will be selected and written into the service config, causing daemon startup to fail even when a valid ~/.shelltime/bin/shelltime-daemon exists. This lookup should enforce executability before returning the path.

Useful? React with 👍 / 👎.

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)
}
114 changes: 114 additions & 0 deletions model/path_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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")
}
})
}
Loading