fix(daemon): propagate error when daemon binary not found on reinstall#279
Conversation
`shelltime daemon reinstall` printed "successfully reinstalled" even when the install step could not locate `shelltime-daemon` (common on Linux without Homebrew/curl-installer). The install handler swallowed the ResolveDaemonBinaryPath error with `return nil`, so the reinstall wrapper treated it as success. Return a wrapped error instead so the reinstall command exits non-zero and skips the green success line, and surface the hint in red. https://claude.ai/code/session_01LBLq3b7dqsgfcSAEmnWDcY
![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 updates the commandDaemonInstall function to return an error when the daemon binary is not found, rather than returning nil. It also updates the console output to display a red error message. A review comment suggests returning the error directly instead of wrapping it with a redundant message, as the underlying error is already descriptive and a similar message is printed to the console.
| 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 | ||
| return fmt.Errorf("shelltime-daemon binary not found: %w", err) |
There was a problem hiding this comment.
The error message wrapping here is redundant. The err returned by model.ResolveDaemonBinaryPath() already contains a descriptive message (e.g., "shelltime-daemon not found on PATH..."). Additionally, a similar message is already printed to the console on line 45. Wrapping it with the same prefix results in a repetitive output like shelltime-daemon binary not found: shelltime-daemon not found on PATH.... Returning the error directly is cleaner and avoids unnecessary redundancy.
| return fmt.Errorf("shelltime-daemon binary not found: %w", err) | |
| return err |
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:
|
2 participants
Summary
shelltime daemon reinstallon Linux printed a green "✅ Daemon service has been successfully reinstalled!" even when the install step couldn't locate theshelltime-daemonbinary — the service was uninstalled and never replaced, but the CLI claimed success.commandDaemonInstallswallowed themodel.ResolveDaemonBinaryPath()error withreturn nilafter printing a yellow hint, so the reinstall wrapper sawniland printed the success line.daemon install) actually exit non-zero. Also switched the "not found" line from yellowTest plan
go build -o /tmp/shelltime ./cmd/cli/main.go— compilesgo test -run TestResolveDaemonBinaryPath ./model/— passesshelltime-daemonon PATH or under~/.shelltime/bin/:shelltime daemon reinstallshould print the red "not found" line plus install hints, omit the green success line, and exit non-zeroshelltime-daemonavailable on PATH:shelltime daemon reinstallstill prints the green success line and exits 0https://claude.ai/code/session_01LBLq3b7dqsgfcSAEmnWDcY
Generated by Claude Code