feat(cli): support Homebrew binary paths for daemon and hooks#272
feat(cli): support Homebrew binary paths for daemon and hooks#272
Conversation
Homebrew installs binaries to /opt/homebrew/bin (Apple Silicon) or /usr/local/bin (Intel), not ~/.shelltime/bin. This adds proper binary resolution across install methods and embeds hook scripts so Homebrew users don't need the curl installer for shell hooks. - Add ResolveDaemonBinaryPath() to search curl-installer, PATH, and Homebrew fallback locations - Update plist/systemd templates to use resolved DaemonBinPath - Embed bash/zsh/fish hook scripts via go:embed for self-contained Homebrew installs - Auto-download bash-preexec.sh dependency during hooks install - Update hooks install to accept shelltime on PATH (not just in ~/.shelltime/bin) - Add Homebrew install method to README Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![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 introduces Homebrew support for shelltime by updating the installation logic to resolve the daemon binary from multiple locations (Homebrew, curl-installer, or PATH) and embedding shell hook scripts directly into the binary. Feedback includes suggestions to background shell tracking and garbage collection tasks in hook scripts to reduce shell latency, adding timeouts and size limits to network requests when downloading dependencies, and improving error handling during daemon installation. Additionally, a more efficient method for retrieving the parent process ID in Fish shell was recommended.
| return | ||
| fi | ||
|
|
||
| shelltime track -s=bash -id=$SESSION_ID -cmd="$CMD" -p=pre --ppid=$PPID &> /dev/null |
There was a problem hiding this comment.
Executing shelltime track synchronously before every command adds latency to the shell. Backgrounding the process prevents blocking the user's workflow.
| shelltime track -s=bash -id=$SESSION_ID -cmd="$CMD" -p=pre --ppid=$PPID &> /dev/null | |
| shelltime track -s=bash -id=$SESSION_ID -cmd="$CMD" -p=pre --ppid=$PPID &> /dev/null & |
| return | ||
| fi | ||
|
|
||
| shelltime track -s=bash -id=$SESSION_ID -cmd="$CMD" -p=post -r=$LAST_RESULT --ppid=$PPID &> /dev/null |
There was a problem hiding this comment.
The post-command tracking should also be backgrounded to ensure the prompt returns immediately after a command finishes.
| shelltime track -s=bash -id=$SESSION_ID -cmd="$CMD" -p=post -r=$LAST_RESULT --ppid=$PPID &> /dev/null | |
| shelltime track -s=bash -id=$SESSION_ID -cmd="$CMD" -p=post -r=$LAST_RESULT --ppid=$PPID &> /dev/null & |
| return | ||
| end | ||
| # This event is triggered before each prompt, which is after each command | ||
| shelltime track -s=fish -id=$SESSION_ID -cmd="$argv" -p=post -r=$LAST_RESULT --ppid=$FISH_PPID > /dev/null |
There was a problem hiding this comment.
Backgrounding the tracking call in Fish prevents latency between command completion and the next prompt. Also, redirecting stderr ensures that any errors from the tracking command don't clutter the terminal.
shelltime track -s=fish -id=$SESSION_ID -cmd="$argv" -p=post -r=$LAST_RESULT --ppid=$FISH_PPID > /dev/null 2>&1 &
| if [[ $CMD =~ ^(exit|logout|reboot) ]]; then | ||
| return | ||
| fi | ||
| shelltime track -s=zsh -id=$SESSION_ID -cmd="$CMD" -p=post -r=$LAST_RESULT --ppid=$PPID &> /dev/null |
There was a problem hiding this comment.
Backgrounding the tracking call in Zsh ensures that the prompt returns immediately after a command finishes, avoiding any perceived lag.
shelltime track -s=zsh -id=$SESSION_ID -cmd="$CMD" -p=post -r=$LAST_RESULT --ppid=$PPID &> /dev/null &
| color.Yellow.Println("⚠️ shelltime-daemon not found.") | ||
| color.Yellow.Println("Install via Homebrew: brew install shelltime/tap/shelltime") | ||
| color.Yellow.Println("Or via curl installer: curl -sSL https://shelltime.xyz/i | bash") | ||
| return nil |
There was a problem hiding this comment.
The command returns nil when the daemon binary is not found. It should return an error to ensure the CLI exits with a non-zero status code, which is essential for scripts and automation to detect failures.
| return nil | |
| return fmt.Errorf("shelltime-daemon binary not found") |
| return nil // already exists | ||
| } | ||
|
|
||
| resp, err := http.Get(bashPreexecURL) |
There was a problem hiding this comment.
Using http.Get without a timeout can cause the process to hang indefinitely if the network is unstable. It's safer to use an http.Client with a reasonable timeout.
client := &http.Client{Timeout: 15 * 1000 * 1000 * 1000} // 15 seconds
resp, err := client.Get(bashPreexecURL)| return fmt.Errorf("failed to download bash-preexec.sh: HTTP %d", resp.StatusCode) | ||
| } | ||
|
|
||
| body, err := io.ReadAll(resp.Body) |
There was a problem hiding this comment.
Reading the entire response body without a size limit can lead to high memory consumption if the server returns an unexpectedly large response. Using io.LimitReader is a more secure practice.
| body, err := io.ReadAll(resp.Body) | |
| body, err := io.ReadAll(io.LimitReader(resp.Body, 1024*1024)) |
| then | ||
| echo "Warning: shelltime CLI not found. Please install it to enable time tracking." | ||
| else | ||
| shelltime gc |
There was a problem hiding this comment.
Running shelltime gc synchronously on every shell startup can slow down terminal opening. Backgrounding this task improves shell responsiveness.
| shelltime gc | |
| shelltime gc &> /dev/null & |
| set -g SESSION_ID (date +%Y%m%d%H%M%S) | ||
|
|
||
| # Capture parent process ID at shell startup (fish doesn't have native $PPID) | ||
| set -g FISH_PPID (ps -o ppid= -p %self | string trim) |
There was a problem hiding this comment.
In Fish shell, status ppid is a more efficient and built-in way to retrieve the parent process ID compared to spawning an external ps process.
set -g FISH_PPID (status ppid)
![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: 02368fbe0b
ℹ️ 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".
| if [ -f "bash-preexec.sh" ]; then | ||
| source "bash-preexec.sh" |
There was a problem hiding this comment.
The new Bash hook will source bash-preexec.sh from the caller’s current working directory before checking the ShellTime hook directory, which allows arbitrary local files to be executed whenever a user starts a Bash subshell in an untrusted folder. This is a security regression for any user who installs embedded hooks, because a project containing a malicious bash-preexec.sh would be executed implicitly.
Useful? React with 👍 / 👎.
| return nil // already exists | ||
| } | ||
|
|
||
| resp, err := http.Get(bashPreexecURL) |
There was a problem hiding this comment.
ensureBashPreexec uses http.Get with the default client, which has no request timeout, so shelltime hooks install can hang indefinitely on slow/broken networks (for example, captive portals or stalled TLS handshakes). Adding a bounded timeout avoids a user-visible freeze during installation.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Claude finished @AnnatarHe's task in 1m 14s —— View job
Adding HTTP timeout to bash-preexec download
- Read current
model/shell.bash.go - Add 1-minute timeout to
http.Getcall - Commit and push changes
Added a 1-minute timeout via http.Client{Timeout: 1 * time.Minute} to replace the default http.Get (which has no timeout). Also added io.LimitReader(resp.Body, 1024*1024) to cap the response body at 1MB as an additional safety measure.
|
…download Use http.Client with 1-minute timeout instead of http.Get to prevent hanging on slow/broken networks. Also add io.LimitReader (1MB) to prevent excessive memory usage from unexpected large responses. Co-authored-by: Le He <AnnatarHe@users.noreply.github.com>
1 participant
Summary
ResolveDaemonBinaryPath()that searches~/.shelltime/bin, PATH, and standard Homebrew locations (/opt/homebrew/bin,/usr/local/bin,/home/linuxbrew/.linuxbrew/bin){{.DaemonBinPath}}instead of hardcoded{{.BaseFolder}}/bin/shelltime-daemonbash.bash,zsh.zsh,fish.fish) viago:embedsoshelltime hooks installworks without the curl installerbash-preexec.shdependency during bash hooks installshelltime hooks installto acceptshelltimeon PATH (Homebrew) in addition to~/.shelltime/binTest plan
go build ./...passesgo vet ./model/... ./commands/... ./daemon/...passesgo test -timeout 3m ./...passesshelltime daemon installresolves binary correctly for Homebrew installsshelltime hooks installcreates hook files in~/.shelltime/hooks/for Homebrew users🤖 Generated with Claude Code