fix(daemon): compare daemon paths by file identity, not string

[AnnatarHe]

Problem

On Linux, running shelltime daemon reinstall (or daemon install) could leave ~/.shelltime/bin/ with shelltime and shelltime-daemon.bak but no shelltime-daemon. The daemon then can't start, and re-running install/reinstall never recovers — it stays stuck. The .bak file holds the correct, working daemon version. Because install.bash runs shelltime daemon reinstall on every upgrade, this path is hit routinely.

Root cause

Two pieces of code interacted destructively:

1. model/path.go → ResolveDaemonBinaryPath() prefers a binary on PATH / in /opt/homebrew/bin, /usr/local/bin, /home/linuxbrew/.linuxbrew/bin over the curl-installer copy at ~/.shelltime/bin/shelltime-daemon. It decided "this is a different binary" with a string comparison (resolved != curlPath).
2. commands/daemon.install.go then renamed curlPath → curlPath+".bak" whenever the resolved path string differed, and pointed the systemd/launchd service at the resolved path.

When the same on-disk file was reachable via a different path string — a symlink in /usr/local/bin pointing back at the curl binary, a symlinked $HOME, or a realpath'd PATH entry — the string compare wrongly reported "different". The only real binary was moved to .bak and the service was pointed at a now-dangling alias path, so the daemon failed to launch. On the next reinstall the restore block renamed .bak → shelltime-daemon, but resolution again returned the alias string and the backup block re-hid it → infinite stuck loop.

commands/daemon.install.go and model/path.go are platform-agnostic, so the same bug existed on macOS.

Fix

Compare daemon paths by file identity (os.SameFile, i.e. device+inode) instead of string equality, at both layers:

• ResolveDaemonBinaryPath(): a PATH/Homebrew candidate that is the same file as the curl binary now falls through and returns the canonical curl path (guarded in both the LookPath branch and the Homebrew-search loop, since PATH is often stripped under systemd/launchd).
• commands/daemon.install.go: a new shouldPreserveCurlDaemon helper only renames the curl copy to .bak for a genuinely distinct file — never the binary the service is about to point at.

os.SameFile is correct across symlinks, hardlinks, bind mounts and case-insensitive filesystems, and returns false across different filesystems (no inode collisions). It mirrors the existing symlink-aware handling in ResolveCLIBinaryPath (model/updater.go).

Recovery is automatic

On a currently-stuck machine, the next daemon install/reinstall first restores .bak → shelltime-daemon, resolution now recognizes the alias and returns the curl path, and the backup block no longer re-hides it — no manual cleanup, no GitHub re-download. Real, distinct Homebrew/system installs still win, preserving the intended "brew drives the daemon" behavior.

Out of scope

Runtime verification of the resolved binary (running shelltime-daemon -v before preferring it) would additionally cover a genuinely-distinct-but-broken system binary. It's intentionally excluded here (different failure mode; adds risk in sandboxed/noexec/slow environments) and can be a separate change.

Tests

• model/path_test.go: new subtests for a PATH symlink and a Homebrew-search symlink that point back at the curl binary (both fail before the fix, pass after); a Test_sameFile table test (same path, symlink↔target, distinct files, missing operands, broken symlink). The existing "prefers a distinct PATH binary" case still passes, guarding against over-correction.
• commands/daemon.install_test.go (new): table test for shouldPreserveCurlDaemon — identical strings → don't preserve; distinct files → preserve; symlink alias of curl → don't preserve.

Verification run

• go build ./... ✅, go vet ./... ✅ (one pre-existing unrelated warning in cmd/cli/main.go)
• go test ./model/ ./commands/ ./stloader/ ✅
• The only failing package is daemon (TestGitInfoTestSuite), confirmed pre-existing and environmental — it fails identically on a clean tree without these changes (git branch detection returns empty in the sandbox).

🤖 Generated with Claude Code

https://claude.ai/code/session_016Pjkb6uP7ttH8ogFQspMQY

---

Generated by Claude Code

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[codecov]

Codecov Report

❌ Patch coverage is 87.50000% with 2 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
commands/daemon.install.go	66.66%	1 Missing ⚠️
model/path.go	92.30%	1 Missing ⚠️

❌ Your project check has failed because the head coverage (78.73%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage.

Flag	Coverage Δ
unittests	78.73% <87.50%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
commands/daemon.install.go	3.77% <66.66%> (+3.77%)	⬆️
model/path.go	95.77% <92.30%> (-0.90%)	⬇️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gemini-code-assist]

Code Review

This pull request resolves an installation loop issue on Linux by ensuring that symlinks or aliases pointing back to the curl-installer binary are not treated as distinct system installations. It introduces a sameFile helper using os.SameFile to correctly identify identical on-disk files. The review feedback suggests a minor optimization to sameFile to return early if the two path strings are identical, avoiding redundant os.Stat calls.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

An optimization and robustness improvement can be made by checking if the two path strings are identical (a == b) at the very beginning of sameFile. If they are identical, they are guaranteed to refer to the same file path, and we can return true immediately. This avoids two redundant os.Stat system calls and ensures correct behavior even if the file is temporarily inaccessible or does not exist.

Suggested change

	func sameFile(a, b string) bool {
	ai, err := os.Stat(a)
	func sameFile(a, b string) bool {
	if a == b {
	return true
	}
	ai, err := os.Stat(a)

[AnnatarHe]

@claude ultrareview

[claude]

Claude finished @AnnatarHe's task in 1m 52s —— View job

---

Ultra Review

• Read changed files
• Analyze core logic and correctness
• Review tests
• Check edge cases and security
• Post final review

---

Summary

This is a well-targeted, minimal fix for a genuine data-loss bug. The root cause analysis in the PR description is accurate and the approach — replacing string equality with os.SameFile (device+inode comparison) — is the correct tool for this class of problem.

Verdict: Approve with minor suggestions.

---

Core Logic (model/path.go)

The fix is correct. The sameFile helper using os.Stat + os.SameFile correctly handles all symlink/hardlink/alias cases across different filesystems without inode collisions.

Small improvement in the same change: The old LookPath branch silently discarded the filepath.Abs result and returned the raw path string:

// old
if resolved, _ := filepath.Abs(path); resolved != curlPath {
    return path, nil   // ← raw, possibly relative
}
// new  
if resolved, absErr := filepath.Abs(path); absErr == nil && !sameFile(resolved, curlPath) {
    return resolved, nil  // ← abs-clean path
}

Returning resolved instead of path and checking absErr is strictly better. The filepath.Abs error is extremely rare (only on getcwd failure), but the fix is more correct.

Minor: SameDaemonFile public export (model/path.go:197). This one-liner exists solely to bridge package visibility — commands can't call the unexported sameFile. It's a workable structure, but it does add to the model public API for something that is conceptually a commands-layer concern. Not a blocker; just something to keep in mind if model's surface grows further.

---

Installer Logic (commands/daemon.install.go)

shouldPreserveCurlDaemon is correct.

func shouldPreserveCurlDaemon(daemonBinPath, curlDaemonPath string) bool {
    return daemonBinPath != curlDaemonPath && !model.SameDaemonFile(daemonBinPath, curlDaemonPath)
}

The string pre-check (daemonBinPath != curlDaemonPath) is technically redundant — sameFile("x", "x") would stat the same path twice and return true, making !sameFile false anyway. But it's a valid micro-optimization that avoids two syscalls in the common case, and it's exactly the early-return that Gemini suggested adding inside sameFile. Addressing it at the call site vs. inside the helper are both correct; the current placement is fine.

Recovery path logic (the stuck-loop fix) works end-to-end:

1. .bak exists → rename .bak → shelltime-daemon ✅
2. ResolveDaemonBinaryPath() now sees the restored curl binary ✅
3. shouldPreserveCurlDaemon(curlPath, curlPath) → false (won't re-hide it) ✅
4. Service installs and points at the correct binary ✅

---

Tests

Test quality is high. Key observations:

withIsolatedDaemonResolution (model/path_test.go:312) is a well-designed hermetic isolation helper — resets HOME, PATH, and daemonHomebrewSearchPaths so host machine installs can't bleed into assertions.

writeFakeDaemon creates an actual executable — necessary because exec.LookPath checks the executable bit. Correct approach.

The two new symlink subtests precisely reproduce the failure scenarios described in the PR:

• Symlink on PATH → curl path (skips PATH result that is a symlink back to the curl path)
• Symlink in Homebrew search list → curl path (skips homebrew-search result that is a symlink back to the curl path)

Test_sameFile covers the important cases: same path, symlink↔target, distinct files with identical content, missing operands, broken symlink.

One missing test case: hardlinks. sameFile is documented to handle them, and os.SameFile does handle them at the OS level — but there's no test exercising os.Link. Not a bug, just a coverage note.

Regarding the Codecov gaps (87.5% patch coverage):

• commands/daemon.install.go: The uncovered line is almost certainly the rnErr != nil warning branch inside commandDaemonInstall (the rename failure path for the .bak operation). This requires actual OS-level rename failure to trigger — not worth forcing.
• model/path.go: The uncovered line is likely the absErr != nil fallthrough in the LookPath branch — requires getcwd to fail to trigger. Also not worth forcing.

Neither gap represents a real risk.

---

Issues to Address

None are blockers. One is worth a quick fix:

1. (Minor) sameFile could return early on identical strings — already addressed at call sites, but adding it inside the helper would make sameFile robust for any future callers:

// model/path.go:182
func sameFile(a, b string) bool {
    if a == b {
        return true  // avoid two stat calls on the hot path
    }
    ai, err := os.Stat(a)
    ...
}

Fix this →

2. (Nit) Add hardlink test case to Test_sameFile — the docstring claims hardlink support, it would be consistent to test it:

// in Test_sameFile setup
hardA := filepath.Join(dir, "hard-a")
_ = os.Link(fileA, hardA)
// ...
{"hardlink of a", hardA, fileA, true},

---

No security concerns

The fix only changes comparison semantics. It doesn't expand filesystem access, doesn't introduce new exec paths, and doesn't change how binaries are sourced. The failure mode when sameFile returns false positives is "don't move the file" — conservative and safe.
|