fix(daemon): prefer Homebrew binary over curl-installer location#277
Conversation
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) <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 2 files with indirect coverage changes 🚀 New features to boost your workflow:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 80c00a16e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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() { |
There was a problem hiding this comment.
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 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the daemon binary resolution logic to prioritize system-managed installations (such as Homebrew or those on the system PATH) over the legacy curl-installer location. It also introduces a cleanup mechanism to remove stale curl-installer binaries and adds comprehensive unit tests for the resolution logic. The review feedback suggests improving path comparison by using os.SameFile instead of string equality to robustly handle symlinks and prevent the accidental deletion of active binaries.
| if path, err := exec.LookPath(binaryName); err == nil { | ||
| return path, nil | ||
| if resolved, _ := filepath.Abs(path); resolved != curlPath { | ||
| return path, nil | ||
| } | ||
| } |
There was a problem hiding this comment.
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
}
}
}
}| 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) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
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.
| 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) | |
| } | |
| } | |
| } |
1 participant
Summary
ResolveDaemonBinaryPathto prefer PATH/Homebrew over~/.shelltime/bin/shelltime-daemon, sobrew upgrade shelltimedrives the running daemon instead of a stale curl-installer copy.shelltime daemon install, remove the stale~/.shelltime/bin/shelltime-daemonwhen a system-managed binary is chosen (keepsshelltime.bakfrom the CLI upgrade flow intact).TestResolveDaemonBinaryPathcovering five resolution cases.Background
A user with both a curl-installed daemon (
~/.shelltime/bin/shelltime-daemon, Apr 7) and a Homebrew install (/opt/homebrew/bin/shelltime-daemon, today) found the plist still pointed at the April binary. The old resolution order preferred~/.shelltime/bin/first.Test plan
go test ./model/... -run TestResolveDaemonBinaryPath(5 subtests pass)go test -timeout 3m ./model/...(no regressions)go build ./...(whole module builds)shelltime daemon install— output showsFound daemon binary at: /opt/homebrew/bin/shelltime-daemonandRemoved stale curl-installer daemon at .... The plist now contains/opt/homebrew/bin/shelltime-daemonandpgrep -lf shelltime-daemonshows the Homebrew-managed process running.🤖 Generated with Claude Code