Skip to content

fix: history load for non exist directory #29

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Balaji01-4D merged 1 commit into from
Apr 4, 2026
+55 −5
Merged

fix: history load for non exist directory #29

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions internal/repl/history.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ type history struct {
path string
loadCount int
logger *slog.Logger
disabled bool
}

func newHistory(historyPath string, logger *slog.Logger) (*history, []prompt.HistoryCommand) {
Expand All @@ -37,7 +38,7 @@ func (h *history) loadHistory() []prompt.HistoryCommand {
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)
}
Comment on lines 40 to 42
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
return []prompt.HistoryCommand{}
Comment on lines 38 to 43
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
Expand All @@ -49,7 +50,7 @@ func (h *history) loadHistory() []prompt.HistoryCommand {

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{}
Comment on lines 51 to 54
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
}
return entries
Expand Down Expand Up @@ -82,18 +83,25 @@ func loadHistory(r io.Reader, maxHistoryLines int, logger *slog.Logger) ([]promp
}

func (h *history) saveHistory(entries []prompt.HistoryCommand) {
if h.disabled {
return
}

if len(entries) <= h.loadCount {
return
}

newCommands := entries[h.loadCount:]
if len(newCommands) == 0 {

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
}
Comment on lines +96 to 100
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.

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
}
Comment on lines 102 to 106
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot uses AI. Check for mistakes.
defer func() {
Expand All @@ -114,12 +122,17 @@ func (h *history) saveHistory(entries []prompt.HistoryCommand) {
}

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)
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
h.disableHistory("failed save history to path, err, history is disabled", err)
h.disableHistory("failed to flush history file; history is disabled", err)

Copilot uses AI. Check for mistakes.
return
}
h.logger.Debug("history saved", "new_entries", len(newCommands))
}

func (h *history) disableHistory(message string, err error) {
h.logger.Error(message, "path", h.path, "err", err)
Copy link

Copilot AI Apr 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Suggested change
h.logger.Error(message, "path", h.path, "err", err)
h.logger.Error(message, "path", h.path, "error", err)

Copilot uses AI. Check for mistakes.
h.disabled = true
}

func getHistoryFilePath() string {
homeDir, err := os.UserHomeDir()
if err != nil {
Expand Down
37 changes: 37 additions & 0 deletions internal/repl/history_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import (
"encoding/json"
"log/slog"
"os"
"path/filepath"
"strings"
"testing"

Expand Down Expand Up @@ -96,6 +97,42 @@ func TestHistorySaveHistory_EntriesShorterThanLoadCount(t *testing.T) {
assert.Equal(t, "", string(data))
}

func TestHistorySaveHistory_CreatesMissingParentDirectoryAndFile(t *testing.T) {
baseDir, err := os.MkdirTemp("", "history_parent_test")
assert.NoError(t, err)
defer func() {
err := os.RemoveAll(baseDir)
assert.NoError(t, err)
}()

historyPath := filepath.Join(baseDir, "non_exist_folder", "user_provided_name.jsonl")
h := history{path: historyPath, loadCount: 0, logger: testLogger()}
entries := []prompt.HistoryCommand{{Command: "select 1"}}

h.saveHistory(entries)

_, err = os.Stat(filepath.Dir(historyPath))
assert.NoError(t, err)

data, err := os.ReadFile(historyPath)
assert.NoError(t, err)
assert.NotEmpty(t, strings.TrimSpace(string(data)))
}

func TestHistorySaveHistory_DisablesHistoryOnInvalidPath(t *testing.T) {
h := history{path: "\x00", loadCount: 0, logger: testLogger()}
entries := []prompt.HistoryCommand{{Command: "select 1"}}

h.saveHistory(entries)

assert.True(t, h.disabled)

// Second call should no-op because history is disabled.
assert.NotPanics(t, func() {
h.saveHistory(entries)
})
}

func TestLoadHistory(t *testing.T) {
r := strings.NewReader(strings.Join([]string{
`{"command":"query1","timestamp":"2026-04-04T10:00:00Z"}`,
Expand Down
Loading