repl history

[navicore]

⏺ The implementation is complete. Here's what was added:

New functions in ffi:libedit:

Word	Stack Effect	Description
read-history	( String -- Int )	Load history from file (0 = success)
write-history	( String -- Int )	Save history to file (0 = success)

Files changed:

• crates/compiler/ffi/libedit.toml - Added the two new function bindings
• docs/FFI_GUIDE.md - Updated documentation with new functions and persistent history example

Usage example:
include ffi:libedit

: main ( -- Int )
"/.seqlisp_history" read-history drop # Load on startup
run-repl
"/.seqlisp_history" write-history drop # Save on exit
0
;

[claude]

Pull Request Review: REPL History

Thank you for this contribution! I've reviewed the implementation and have identified both strengths and critical issues.

Strengths

1. Consistent Documentation: Updates to docs/FFI_GUIDE.md follow existing patterns well
2. Good Code Examples: Proper error handling demonstrated with drop to ignore return codes
3. Proper TOML Structure: FFI manifest entries follow established patterns
4. Clear Comments: Good explanatory comments about function behavior

CRITICAL ISSUE: Wrong API for libedit

The read_history and write_history functions are from GNU readline, NOT libedit.

The Problem

Libedit (BSD) and GNU readline (GPL) have different APIs for history management:

• GNU readline: read_history(filename) and write_history(filename)
• libedit: Uses history() function with different opcodes

These functions will FAIL TO LINK on systems using pure libedit (macOS, BSDs) because the symbols read_history and write_history don't exist in libedit. The comment in libedit.toml:23-24 correctly notes that function names match GNU readline's API for readline and add_history, but this is NOT true for read_history/write_history.

Impact

• macOS users: Linker errors
• Linux users: Might work with readline compatibility wrapper (defeats BSD-license purpose)
• BSD users: Will fail to link

Solutions

Option A: Use readline compatibility layer (document this requirement)
Option B: Use libedit's native API (recommended) - bind to history_init() and history()
Option C: Create separate ffi:readline manifest for GNU readline

Other Issues

1. Missing Test Coverage

No automated tests verify the functions work correctly. Add integration test similar to examples/ffi/libedit-demo.seq

2. Error Handling Documentation Gap

Examples at docs/FFI_GUIDE.md:243,249 use drop without explaining:

• Return codes (0=success, non-zero=failure)
• When errors occur (permissions, disk full, etc.)
• When safe to ignore vs must check

3. Tilde Expansion Unclear

Examples use ~/.myapp_history but don't clarify if tilde expansion is handled by the library

4. Missing Stack Effect Validation

Add test in crates/compiler/src/ffi.rs to verify libedit manifest parses correctly

Summary

PR needs SIGNIFICANT REWORK due to critical API incompatibility. Code quality and documentation style are good, but fundamental approach is flawed.

Required Changes

1. BLOCKER: Fix libedit API compatibility
2. Add test coverage
3. Improve error handling docs

Suggested Enhancements

4. Clarify tilde expansion
5. Add unit test for manifest parsing

References

• https://man.openbsd.org/editline.3
• https://tiswww.case.edu/php/chet/readline/history.html

Would you like help implementing the native libedit API or switching to GNU readline?

[claude]

PR Review: Add Persistent History Support to ffi:libedit

Summary

This PR adds read-history and write-history functions to the ffi:libedit module, enabling persistent command history across REPL sessions. The implementation addresses issue #82 and includes proper documentation and a working example.

---

✅ Strengths

1. Clean FFI Binding Design

• The TOML function declarations follow established patterns correctly (crates/compiler/ffi/libedit.toml:56-80)
• Stack effects are properly documented: ( String -- Int )
• Return types correctly indicate success/failure with int
• Naming convention matches existing readline and add-history functions

2. Excellent Documentation

• Clear inline comments explain the API behavior (libedit.toml:56-71)
• Important path resolution caveat is well-documented in multiple places
• The note about shell expansion (~) being the application's responsibility shows good understanding of FFI boundaries
• Updated FFI_GUIDE.md includes a complete, practical example (docs/FFI_GUIDE.md:225-252)

3. Practical Demo

• The example code demonstrates proper usage patterns (examples/ffi/libedit-demo.seq:69-79)
• Shows error handling strategy (drop for ignoring errors when file doesn't exist)
• Uses /tmp path for simplicity in demo, with clear comments about production use

4. Addresses the Original Issue

• Directly implements the requested functionality from issue Add read_history and write_history to ffi:libedit #82
• Matches the exact C signatures specified in the issue
• Provides the use case described (persistent history for SeqLisp)

---

🔍 Areas for Consideration

1. Error Handling Pattern

In both the documentation example and demo, errors are silently ignored:

history-file read-history drop  # Ignores return value

While this is reasonable for read-history (file might not exist on first run), write-history failures could result in silent data loss.

Suggestion: Consider documenting error handling patterns more explicitly:

# Example with error handling
: load-history ( String -- )
  read-history
  0 = not if
    "Warning: Could not load history" write_line
  then
;

2. File Path Expansion Limitation

The documentation correctly notes that shell expansion isn't performed, but this could be a usability issue. Users might expect ~/.seq_history to work.

Considerations:

• The note about future std:os module is good forward-thinking
• Could mention this limitation more prominently in the initial function comments (libedit.toml:18-26)
• The example using /tmp is safe but less realistic

3. Test Coverage

Missing: No automated tests for the new functionality.

Suggestions:

• Add integration test that verifies:
  • Writing history to a temp file
  • Reading it back
  • Handling non-existent files
  • Example: tests/integration/src/test-libedit-history.seq

4. Memory Safety Considerations

The FFI passes strings as c_string (null-terminated char*):

args = [
  { type = "string", pass = "c_string" }
]

Questions to verify:

• Is the string lifetime guaranteed during the C call?
• Are returned error codes the only failure mode (no segfaults on bad paths)?

The libedit API is well-established and should be safe, but worth documenting FFI safety assumptions.

---

🔒 Security Concerns

1. Path Traversal (Low Risk)

• Users can write to any path accessible to the process
• This is expected FFI behavior but worth noting in security-sensitive contexts
• The example using /tmp is appropriate

2. File Permissions

• No validation of file permissions before write
• Again, expected for a low-level FFI wrapper
• Applications should handle this at a higher level

Verdict: Security concerns are minimal and appropriate for an FFI binding. The code doesn't introduce vulnerabilities beyond standard file I/O risks.

---

⚡ Performance Considerations

• File I/O is blocking (expected for startup/shutdown operations)
• No performance concerns for the intended use case
• History files are typically small (<10KB)

Note: The demo calls these functions at startup and shutdown, which is the correct pattern.

---

📊 Code Quality

Formatting & Style:

• ✅ Consistent indentation and spacing
• ✅ TOML structure follows existing conventions
• ✅ Comments are clear and helpful
• ✅ Function naming follows kebab-case pattern

Documentation:

• ✅ Stack effects documented
• ✅ Return values explained
• ✅ Usage examples provided
• ⚠️ Could benefit from mentioning error codes explicitly (0 = success)

---

🎯 Recommendations

Must Have (Blockers)

None - the PR is functional and well-documented.

Should Have (Strongly Recommended)

1. Add integration tests for the new functions
2. Enhance error handling in the example to show best practices
3. Document return codes more explicitly (which non-zero values mean what)

Nice to Have

1. Consider a helper function for common error patterns:

   : try-load-history ( String -- )
     read-history 0 = not if
       "Could not load history from: " swap string-concat write_line
     then
   ;
   

2. Add a note in the documentation about history file format (is it portable between libedit versions?)

---

✅ Verdict

Approved with suggestions.

This is a solid, well-documented addition that directly addresses the requested feature. The FFI bindings are correct, the documentation is thorough, and the example demonstrates proper usage. The main gap is test coverage, which should be added before merging.

The code quality is high, and the implementation shows good understanding of both the FFI system and the libedit API. The notes about path expansion limitations and the clear separation of concerns between FFI and application-level logic are particularly well done.

Recommendation: Merge after adding basic integration tests.

---

Great work on the implementation and documentation! The attention to detail in explaining the path expansion limitation and providing context about future std:os improvements shows mature software engineering thinking.