Remove retention limit support #11

fuba · 2025-11-06T18:34:21Z

Summary

remove the -max-entries flag and revert AddEntry to its single-parameter form
retain the disk space safeguard when inserting history records
update documentation and tests to match the simplified API

Testing

go test ./...

https://chatgpt.com/codex/tasks/task_e_68ee0033e9648323893ad9588fe0b8ee

Copilot

Pull Request Overview

This PR adds disk space checking functionality to prevent writing history entries when storage is unavailable, along with testing infrastructure to support this feature. The changes introduce platform-specific syscall usage and modifications to the database entry insertion logic.

Adds disk space verification before inserting history entries
Introduces a testable architecture with dependency injection for disk space checking
Updates documentation to describe retention controls and safety policies

Reviewed Changes

Copilot reviewed 3 out of 4 changed files in this pull request and generated 8 comments.

File	Description
pkg/histree/histree.go	Adds disk space checking logic using syscall.Statfs, stores database path in DB struct, and provides test hook via SetDiskSpaceChecker
cmd/histree-core/main_test.go	Adds test case verifying that entries are skipped when disk space is unavailable; includes whitespace cleanup
cmd/histree-core/main.go	Whitespace-only changes (trailing whitespace removal)
README.md	Documents new retention controls, safety policies, and `-max-entries` flag; whitespace cleanup

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T18:38:38Z

pkg/histree/histree.go

+	var stat syscall.Statfs_t
+	if err := syscall.Statfs(dir, &stat); err != nil {


The syscall.Statfs and syscall.Statfs_t types are not available on Windows. This will cause compilation errors on Windows platforms. Consider using platform-specific build tags with separate implementations for Unix-like systems and Windows (using syscall.GetDiskFreeSpaceEx on Windows), or using a cross-platform library like golang.org/x/sys/unix and golang.org/x/sys/windows.

Copilot · 2025-11-06T18:38:38Z

pkg/histree/histree.go

+		return false, fmt.Errorf("failed to stat filesystem: %w", err)
+	}
+
+	return stat.Bavail > 0, nil


The disk space check only verifies that stat.Bavail > 0 (at least 1 available block). This is an extremely minimal threshold that could still lead to write failures. Consider implementing a more robust check with a reasonable minimum threshold (e.g., a few MB) or minimum percentage of free space to ensure successful database operations.

ここは閾値を設定できるべきですね。デフォルト 1GB ぐらいがよさそう

Copilot · 2025-11-06T18:38:39Z

pkg/histree/histree.go

+	}
+
+	if !hasSpace {
+		return nil


Silently skipping the entry when disk space is low could lead to unexpected data loss without user awareness. Consider logging a warning or returning an informative error that calling code can choose to ignore, rather than silently discarding the entry. This would allow applications to notify users that history is not being saved.

Suggested change

return nil

return fmt.Errorf("insufficient disk space: history entry not saved")

Copilot · 2025-11-06T18:38:39Z

pkg/histree/histree.go

+var checkDiskSpace = hasSufficientDiskSpace
+
+// SetDiskSpaceChecker allows tests to override the disk space check logic.
+// Passing nil restores the default checker.
+func SetDiskSpaceChecker(fn func(string) (bool, error)) {
+	if fn == nil {
+		checkDiskSpace = hasSufficientDiskSpace
+		return
+	}
+	checkDiskSpace = fn
+}


The checkDiskSpace variable and SetDiskSpaceChecker function are not safe for concurrent access. If multiple goroutines call AddEntry while another goroutine calls SetDiskSpaceChecker, this could lead to race conditions. Consider using sync.RWMutex to protect access to the checkDiskSpace variable or document that this function should only be called during test setup before concurrent operations begin.

Copilot · 2025-11-06T18:38:39Z

README.md

+  - Configure how many history records to keep with the `-max-entries` flag
+  - Automatically prunes the oldest records when the limit is exceeded


The README documents a -max-entries flag for configuring retention limits and automatic pruning of oldest records, but this flag is not implemented in the code. The flag should either be implemented or removed from the documentation to avoid misleading users.

Suggested change

- Configure how many history records to keep with the `-max-entries` flag

- Automatically prunes the oldest records when the limit is exceeded

Copilot · 2025-11-06T18:38:40Z

README.md

+  - Configure how many history records to keep with the `-max-entries` flag
+  - Automatically prunes the oldest records when the limit is exceeded
+  - Skips writing new history entries when no disk space is available, preventing repeated command errors


The documentation describes retention controls with -max-entries flag and automatic pruning, as well as disk space safety. However:

The -max-entries flag is not implemented in the code

The disk space check only verifies Bavail > 0 (any available blocks), which won't prevent errors when disk space is critically low but not completely exhausted

Either implement these features as documented, or update the documentation to accurately reflect the actual implementation (simple check that skips writes only when completely out of space).

Suggested change

- Configure how many history records to keep with the `-max-entries` flag

- Automatically prunes the oldest records when the limit is exceeded

- Skips writing new history entries when no disk space is available, preventing repeated command errors

- Skips writing new history entries only when the disk is completely out of space, preventing repeated command errors in that case

- (Retention limits and automatic pruning are not currently implemented)

Copilot · 2025-11-06T18:38:40Z

pkg/histree/histree.go

+	if os.IsNotExist(err) {
+		if err := os.MkdirAll(dir, 0o755); err != nil {
+			return fmt.Errorf("failed to create directory %s: %w", dir, err)
+		}
+		return nil


The ensureDirExists function automatically creates directories with os.MkdirAll when they don't exist. This side effect in a disk space checking function (called from AddEntry) is unexpected and could lead to unintended directory creation. Consider either documenting this behavior clearly or separating directory creation from the disk space check to make the behavior more explicit and predictable.

Copilot · 2025-11-06T18:38:40Z

README.md

 -dir string     Current directory for filtering entries
 -format string  Output format: json, simple, or verbose (default "simple")
 -limit int      Number of entries to retrieve (default 100)
+-max-entries int  Maximum number of history entries to keep (0 for unlimited)


The -max-entries flag is documented here but is not implemented in cmd/histree-core/main.go. Remove this line or implement the flag.

Copilot

Pull Request Overview

Copilot reviewed 3 out of 4 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-06T19:07:07Z

pkg/histree/histree.go

+	if !hasSpace {
+		return nil
+	}


Silently returning nil when disk is full may confuse users who expect their commands to be recorded. Consider logging a warning or returning a specific error type that callers can handle appropriately. This allows the application to inform users why history isn't being saved.

Copilot · 2025-11-06T19:07:07Z

pkg/histree/histree.go

 // AddEntry adds a new command history entry to the database
 func (db *DB) AddEntry(entry *HistoryEntry) error {
-	_, err := db.Exec(
+	hasSpace, err := checkDiskSpace(db.path)


Checking disk space on every AddEntry call could impact performance, especially for high-frequency command recording. Consider caching the disk space check result with a time-based expiry (e.g., check once per minute) to reduce syscall overhead.

Copilot

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-07T13:45:21Z

pkg/histree/histree.go

 	}
+
 	return nil
 }


Missing blank line between AddEntry function and checkDiskSpace function. Go convention recommends blank lines between top-level declarations for better readability.

Suggested change

}

}

Copilot · 2025-11-07T13:45:21Z

pkg/histree/histree.go

+	info, err := os.Stat(dir)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return false, fmt.Errorf("database directory %s does not exist", dir)
+		}
+		return false, fmt.Errorf("failed to access database directory %s: %w", dir, err)
+	}
+
+	if !info.IsDir() {
+		return false, fmt.Errorf("path %s is not a directory", dir)


hasSufficientDiskSpace checks if the directory exists and returns an error if it doesn't, but this is inconsistent with the calling context. In OpenDB (line 56), ensureDirExists is called first to create the directory if needed. However, hasSufficientDiskSpace is also called from AddEntry (line 163) where the directory should already exist. The error path at line 214 for non-existent directories can never be reached in normal flow since OpenDB ensures the directory exists, making this check redundant. Consider removing the existence check (lines 211-217) and only checking if the path is a directory, or document why this defensive check is needed.

Copilot

Pull Request Overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2025-11-09T13:04:00Z

pkg/histree/histree.go

 	}
+
 	return nil
 }


The exported function SetDiskSpaceChecker and the package-level variables diskSpaceCheckerMu and diskSpaceCheckerFn are documented, but the internal helper function checkDiskSpace lacks documentation. Consider adding a comment explaining its purpose as a thread-safe wrapper around the configurable disk space checker.

Suggested change

}

}

// checkDiskSpace is a thread-safe wrapper around the configurable disk space checker.

// It acquires a read lock to safely access the current disk space checker function.

Copilot · 2025-11-09T13:04:01Z

pkg/histree/histree.go

+	}
+	diskSpaceCheckerFn = fn
+}
+


This function lacks documentation. Consider adding a comment explaining that it's the default implementation for checking disk space that validates the directory exists before delegating to the platform-specific implementation.

Suggested change

// hasSufficientDiskSpace is the default implementation for checking disk space.

// It validates that the directory containing the database exists and is a directory

// before delegating to the platform-specific implementation.

Copilot · 2025-11-09T13:04:01Z

pkg/histree/histree.go

+
+	return platformHasSufficientDiskSpace(dir)
+}
+


This function lacks documentation. Consider adding a comment explaining that it creates the directory and all necessary parent directories if they don't exist, or validates that the path is a directory if it already exists.

Suggested change

// ensureDirExists checks if the given path exists and is a directory.

// If the directory does not exist, it creates the directory and all necessary parent directories.

// If the path exists but is not a directory, it returns an error.

// Returns nil if the directory exists or is successfully created, otherwise returns an error.

Copilot · 2025-11-09T13:04:01Z

pkg/histree/disk_windows.go

+	"fmt"
+	"syscall"
+)
+


This function lacks documentation. Consider adding a comment explaining that it checks available disk space on Windows using the GetDiskFreeSpaceEx system call and returns true if any free space is available.

Suggested change

// platformHasSufficientDiskSpace checks available disk space on Windows using the

// GetDiskFreeSpaceEx system call and returns true if any free space is available.

Copilot · 2025-11-09T13:04:01Z

pkg/histree/disk_unix.go

+	"fmt"
+	"syscall"
+)
+


This function lacks documentation. Consider adding a comment explaining that it checks available disk space on Unix-like systems using the Statfs system call and returns true if any available blocks exist.

Suggested change

// platformHasSufficientDiskSpace checks available disk space on Unix-like systems

// using the Statfs system call and returns true if any available blocks exist.

Copilot · 2025-11-09T13:04:02Z

pkg/histree/histree.go

+	hasSpace, err := checkDiskSpace(db.path)
+	if err != nil {
+		return fmt.Errorf("failed to check disk space: %w", err)
+	}
+
+	if !hasSpace {
+		return fmt.Errorf("%w: unable to record command history entry in %s", ErrInsufficientDiskSpace, db.path)
+	}


The disk space check is performed on every AddEntry call, which could impact performance for high-frequency history recording. Consider adding a caching mechanism with a short TTL (e.g., 1-5 seconds) to avoid redundant system calls when recording multiple commands in quick succession.

Remove retention limit support

c06dc3c

fuba added the codex label Nov 6, 2025 — with ChatGPT Codex Connector

fuba requested a review from Copilot November 6, 2025 18:34

Copilot AI reviewed Nov 6, 2025

View reviewed changes

Update README for disk space safeguard

15a7fc9

fuba requested a review from Copilot November 6, 2025 19:04

Copilot AI reviewed Nov 6, 2025

View reviewed changes

Handle disk space errors explicitly

723bb06

fuba requested a review from Copilot November 7, 2025 13:43

Copilot AI reviewed Nov 7, 2025

View reviewed changes

Relax disk space checker path guard

805eb34

fuba requested a review from Copilot November 9, 2025 13:01

Copilot AI reviewed Nov 9, 2025

View reviewed changes

		var stat syscall.Statfs_t
		if err := syscall.Statfs(dir, &stat); err != nil {

	return nil
	return fmt.Errorf("insufficient disk space: history entry not saved")

		- Configure how many history records to keep with the `-max-entries` flag
		- Automatically prunes the oldest records when the limit is exceeded

-}
+}
+// checkDiskSpace is a thread-safe wrapper around the configurable disk space checker.
+// It acquires a read lock to safely access the current disk space checker function.

+// hasSufficientDiskSpace is the default implementation for checking disk space.
+// It validates that the directory containing the database exists and is a directory
+// before delegating to the platform-specific implementation.

+// ensureDirExists checks if the given path exists and is a directory.
+// If the directory does not exist, it creates the directory and all necessary parent directories.
+// If the path exists but is not a directory, it returns an error.
+// Returns nil if the directory exists or is successfully created, otherwise returns an error.



	// platformHasSufficientDiskSpace checks available disk space on Windows using the
	// GetDiskFreeSpaceEx system call and returns true if any free space is available.



	// platformHasSufficientDiskSpace checks available disk space on Unix-like systems
	// using the Statfs system call and returns true if any available blocks exist.

Remove retention limit support #11

Are you sure you want to change the base?

Remove retention limit support #11

Conversation

fuba commented Nov 6, 2025

Summary

Testing

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

fuba Nov 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 6, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 7, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 9, 2025

Choose a reason for hiding this comment

Uh oh!

fuba Nov 6, 2025 •

edited

Loading