fix: history load for non exist directory#29
Conversation
Signed-off-by: Balaji J <[email protected]>
There was a problem hiding this comment.
Pull request overview
Fixes REPL history persistence when the configured history file path points to a file in a non-existent directory (Issue #22), by ensuring the parent directory exists before writing.
Changes:
- Add a
disabledflag anddisableHistory()helper to stop repeated history I/O attempts after fatal errors. - Create the history file’s parent directory (
os.MkdirAll) before opening the history file for append. - Add tests covering parent directory creation and disabling behavior on invalid paths.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| internal/repl/history.go | Ensures parent directory exists before saving history; adds “disable history on error” behavior and centralized logging. |
| internal/repl/history_test.go | Adds test coverage for missing parent directory creation and disabling behavior on invalid history paths. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !os.IsNotExist(err) { | ||
| h.logger.Warn("could not open history file", "path", h.path, "error", err) | ||
| h.disableHistory("failed load histrory form path, err, hisotry is disabled", err) | ||
| } |
There was a problem hiding this comment.
The new error message passed to disableHistory has multiple typos ("histrory", "form", "hisotry") and reads like a placeholder. Please replace it with a clear, correctly spelled message (e.g., "failed to load history from path; disabling history for this session") so logs are actionable.
| file, err := os.Open(h.path) | ||
| if err != nil { | ||
| if !os.IsNotExist(err) { | ||
| h.logger.Warn("could not open history file", "path", h.path, "error", err) | ||
| h.disableHistory("failed load histrory form path, err, hisotry is disabled", err) | ||
| } | ||
| return []prompt.HistoryCommand{} |
There was a problem hiding this comment.
Calling disableHistory on history load failure can prevent future saves even in cases where the history file is writable but not readable (e.g., write-only permissions). Consider not disabling history on load/open errors (log and continue with empty entries), and only disable when a write attempt fails.
| entries, err := loadHistory(file, maxHistoryLines, h.logger) | ||
| if err != nil { | ||
| h.logger.Error("failed to load history", "error", err) | ||
| h.disableHistory("failed load histrory form path, err, hisotry is disabled", err) | ||
| return []prompt.HistoryCommand{} |
There was a problem hiding this comment.
This disableHistory call uses the same misspelled/unclear message string as above. Please fix the spelling and make the message describe the actual failure (load error vs. write error) so it’s easier to diagnose from logs.
| } | ||
|
|
||
| func (h *history) disableHistory(message string, err error) { | ||
| h.logger.Error(message, "path", h.path, "err", err) |
There was a problem hiding this comment.
The log attribute key here is "err", but the rest of the codebase consistently uses "error" for errors. Consider using "error" for consistency with existing slog logs and tooling/filters.
| h.logger.Error(message, "path", h.path, "err", err) | |
| h.logger.Error(message, "path", h.path, "error", err) |
| historyDir := filepath.Dir(h.path) | ||
| if err := os.MkdirAll(historyDir, 0o700); err != nil { | ||
| h.disableHistory("failed save history to path, err, history is disabled", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
The message string "failed save history to path, err, history is disabled" is ungrammatical and includes a literal "err" token. Please make it a clear sentence that identifies the operation (mkdir vs open vs flush) and rely on the structured error field for the error itself.
| f, err := os.OpenFile(h.path, os.O_CREATE|os.O_APPEND|os.O_WRONLY, 0o600) | ||
| if err != nil { | ||
| h.logger.Error("failed to open history file for writing", "error", err) | ||
| h.disableHistory("failed save history to path, err, history is disabled", err) | ||
| return | ||
| } |
There was a problem hiding this comment.
Same issue as above: this error message includes a literal "err" token and doesn’t indicate which operation failed. Please use a specific, grammatically correct message and keep the error in the structured field.
|
|
||
| if err := w.Flush(); err != nil { | ||
| h.logger.Error("failed to flush history file", "error", err) | ||
| h.disableHistory("failed save history to path, err, history is disabled", err) |
There was a problem hiding this comment.
Same issue as above: the flush failure path logs an unclear/ungrammatical message with a literal "err" token. Please make the message specific to flushing the history file and rely on the structured error field.
| h.disableHistory("failed save history to path, err, history is disabled", err) | |
| h.disableHistory("failed to flush history file; history is disabled", err) |
fix: #22