fixup macos zsh_history (merge wave history => ~/.zsh_history) #2625
Conversation
WalkthroughAdds macOS-specific Zsh history handling and an automatic startup fix. New public helpers and constants were added to pkg/util/shellutil: ZshHistoryFileName, ZshExtendedHistoryPattern, HasWaveZshHistory(), IsExtendedZshHistoryFile(), and FixupWaveZshHistory() to detect, validate (including extended-history detection), merge, and remove a Wave-provided zsh history file. The zsh rc fragment pkg/util/shellutil/shellintegration/zsh_zshrc.sh was updated to redirect HISTFILE on macOS when it points at Wave’s ZDOTDIR. cmd/server/main-server.go now calls FixupWaveZshHistory() at startup and logs errors. pkg/util/envutil gained SliceToEnv and EnvToSlice. Telemetry retention logic in pkg/telemetry/telemetry.go now computes daysToKeep dynamically. pkg/wcloud/wcloud.go changed the timing and error handling for CleanOldTEvents. go.mod added an indirect dependency. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
pkg/util/shellutil/shellutil.go (3)
232-248: Consider compiling the regex at package level.The regex
extendedPatternis compiled on every function call. For a function that may be called during startup, this is a minor performance concern. Consider moving it to a package-level variable.+var extendedHistoryPattern = regexp.MustCompile(`^: [0-9]+:`) + func IsExtendedZshHistoryFile(fileName string) (bool, error) { // ... - extendedPattern := regexp.MustCompile(`^: [0-9]+:`) for _, line := range lines { line = strings.TrimSpace(line) if line == "" { continue } - return extendedPattern.MatchString(line), nil + return extendedHistoryPattern.MatchString(line), nil } // ... }
480-483: Consider logging removal failure.The
os.Removeerror is silently ignored. If the removal fails, the empty file persists but causes no harm on the next startup (the function will just try to remove it again). However, logging the error would help with debugging.if size == 0 { - os.Remove(waveHistFile) + if err := os.Remove(waveHistFile); err != nil { + log.Printf("error removing empty wave zsh history file: %v\n", err) + } return nil }
524-525: Log or return error if post-merge removal fails.If the merge succeeds but the file removal fails, the history will be re-merged on the next startup, potentially causing duplicate entries. Consider logging the error or returning it.
-os.Remove(waveHistFile) -log.Printf("successfully merged wave zsh history %s into ~/.zsh_history\n", waveHistFile) +if err := os.Remove(waveHistFile); err != nil { + log.Printf("warning: could not remove wave zsh history file after merge: %v\n", err) +} +log.Printf("successfully merged wave zsh history %s into ~/.zsh_history\n", waveHistFile)cmd/server/main-server.go (1)
468-468: Log the error from FixupWaveZshHistory.The function returns an error that is currently ignored. While history fixup is non-fatal, logging the error would help with debugging, consistent with the error handling pattern at lines 449-452 for
InitCustomShellStartupFiles.-shellutil.FixupWaveZshHistory() +if err := shellutil.FixupWaveZshHistory(); err != nil { + log.Printf("error fixing up wave zsh history: %v\n", err) +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/server/main-server.go(1 hunks)pkg/util/shellutil/shellintegration/zsh_zshrc.sh(1 hunks)pkg/util/shellutil/shellutil.go(3 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-01-22T01:28:41.417Z
Learnt from: esimkowitz
Repo: wavetermdev/waveterm PR: 1790
File: pkg/remote/fileshare/wshfs/wshfs.go:122-122
Timestamp: 2025-01-22T01:28:41.417Z
Learning: The RpcClient in pkg/remote/fileshare/wshfs/wshfs.go is initialized and handled downstream by either main-server or wshcmd-connserver, as documented in the package comment.
Applied to files:
cmd/server/main-server.go
🧬 Code graph analysis (2)
cmd/server/main-server.go (1)
pkg/util/shellutil/shellutil.go (1)
FixupWaveZshHistory(467-528)
pkg/util/shellutil/shellutil.go (1)
pkg/util/utilfn/utilfn.go (1)
ShellQuote(122-139)
🔇 Additional comments (3)
pkg/util/shellutil/shellutil.go (2)
77-77: LGTM!The constant is appropriately named and co-located with other shell integration path constants.
212-220: LGTM!Clean implementation that correctly handles the file existence check and size retrieval.
pkg/util/shellutil/shellintegration/zsh_zshrc.sh (1)
21-24: LGTM!The macOS-specific history fix correctly redirects
HISTFILEwhen it points to Wave's integration directory. This works in tandem withFixupWaveZshHistory()in the Go code: the Go function merges existing history at startup, while this shell script ensures new history entries go to the correct location going forward.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/server/main-server.go(1 hunks)pkg/util/shellutil/shellutil.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/server/main-server.go
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/util/shellutil/shellutil.go (1)
pkg/util/utilfn/utilfn.go (1)
ShellQuote(122-139)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build for TestDriver.ai
🔇 Additional comments (4)
pkg/util/shellutil/shellutil.go (4)
50-51: LGTM!The regex pattern correctly identifies zsh extended history format which uses the
: timestamp:duration;commandsyntax.
79-79: LGTM!Standard zsh history filename constant.
214-222: LGTM!Clean helper function that checks for the wave zsh history file existence and size.
468-535: Solid implementation with proper startup timing.The merge logic using
fc -Randfc -Wis correct, and the extended history detection properly ensures format compatibility. The function is called early in the server startup sequence (before web listeners are created), which ensures no concurrent zsh sessions can write to~/.zsh_historyduring the merge. Shells are spawned only after clients connect to the running server, eliminating the race condition risk.
| buf := make([]byte, 1024) | ||
| n, err := file.Read(buf) | ||
| if err != nil { | ||
| return false, err | ||
| } |
There was a problem hiding this comment.
Handle io.EOF when reading small files.
file.Read() can return io.EOF along with n > 0 for files smaller than the buffer. This would cause the function to incorrectly return an error for small history files.
Apply this diff:
buf := make([]byte, 1024)
n, err := file.Read(buf)
- if err != nil {
+ if err != nil && err != io.EOF {
return false, err
}
+ if n == 0 {
+ return false, nil
+ }Also add "io" to the imports.
🤖 Prompt for AI Agents
In pkg/util/shellutil/shellutil.go around lines 234 to 238, the code reads into
a fixed buffer and currently returns any non-nil error directly; modify the
logic to import "io" and treat io.EOF as non-fatal when n>0 by only returning an
error if err != nil && err != io.EOF, and ensure subsequent processing uses only
the first n bytes of the buffer (e.g., buf[:n]) so small files that fill less
than the buffer are handled correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
pkg/telemetry/telemetry.go(1 hunks)pkg/wcloud/wcloud.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
pkg/wcloud/wcloud.go (1)
pkg/telemetry/telemetry.go (1)
CleanOldTEvents(256-267)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Analyze (go)
- GitHub Check: Build for TestDriver.ai
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
pkg/telemetry/telemetry.go (1)
257-261: The review comment is based on an incorrect premise. Verification of the git history and codebase shows:
- No evidence of a previous 28-day retention policy - The commit message explicitly documents the current design as intentional: "cap telemetry at 7-days BEFORE send. cap to 1-day for telemetry off"
- The retention values are documented design decisions - The 7-day retention when telemetry is enabled and 1-day when disabled are intentional policies, not reductions from a higher baseline
- No codebase references to 28-day retention - Searches across the telemetry code found no evidence of a previous 28-day policy
The concern about data loss from a 28→7 day reduction is based on a false comparison. The actual code implements the intentional retention policy as designed.
Likely an incorrect or invalid review comment.
No description provided.