navicore/seq-lisp

Fork 0

I 52 #54

Merged

navicore merged 5 commits from i-52 into main

2026-02-01 02:14:46 +00:00

navicore commented

2026-02-01 02:09:00 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-02-01 02:11:04 +00:00

(Migrated from github.com)

Pull Request Review: Vim-style Line Editor Implementation

Summary

This PR successfully implements a vim-style modal line editor in pure Seq, replacing the libedit dependency. This is a significant achievement that adds 865 lines of well-structured editor code with history management and terminal control. All tests pass (449 Lisp + 6 LSP).

✅ Strengths

1. Excellent Code Organization

Clear separation of concerns with well-named sections (Constants, Character Classification, History Management, etc.)
Comprehensive inline comments explaining stack effects and logic
Consistent naming conventions following Seq style

2. Robust Feature Implementation

Modal editing: Normal and Insert modes with proper state management
Vim motions: h/l/0/$/w/b for navigation
Operators: dd/cc/D/C/r/x for editing
History: Persistent history with load/save to ~/.seqlisp_history
Arrow key support: Proper escape sequence handling with non-blocking read

3. Smart Bug Fixes in eval.seq

The fixes to i.divide are correct - Seq 3.0 changed the return type from Int to (Int, Bool):

src/eval.seq:1332: Division with remainder - properly drops the Bool
src/eval.seq:1596: Modulo operation - correctly handles the tuple return

4. Clean Integration

Seamless replacement of libedit with minimal changes to repl.seq
Proper stack management throughout the editor loop
History integrated naturally into the REPL workflow

⚠️ Issues & Concerns

Critical Issues

1. Stack Management Complexity (vim-line.seq:159-162)

# vim-edit returns (History Buffer) where History is unchanged
# History(0) is same as History(3), just drop the copy
drop  # History(2) Acc(1) Env(0)

The comment says "History is unchanged" but the implementation returns a modified History from vim-edit. This creates confusion:

If History can be modified (by k/j navigation adding entries), the comment is misleading
The stack juggling here is complex and error-prone

Recommendation: Consider refactoring to make it clearer whether History is mutable or immutable in the vim-edit call.

2. History Duplication Check Logic (vim-line.seq:316-327)

over scar sstring-val  # History(2) NewString(1) OldString(0)
1 pick                 # History(3) NewString(2) OldString(1) NewString(0)
string.equal? if       # History(2) NewString(1) AreEqual(0)

The duplicate check compares with the most recent entry, but the stack manipulation is fragile. If the History is empty (snil), this would fail before reaching the snil? check.

Wait, this is protected by the over snil? if check at line 313. Actually, this is fine - good defensive programming.

3. History File Race Condition
The history-save function (vim-line.seq:408-415) uses file.spit which will overwrite the file. If multiple REPL sessions are running, the last one to exit will clobber others' history.

Recommendation: Consider appending to history or using file locking, or document this limitation.

Moderate Issues

4. Missing Error Handling

history-load silently returns empty history on file read errors (line 424)
history-save ignores write failures (line 414)
No validation of terminal width/height or escape sequence success

Recommendation: Add debug logging or error reporting for I/O failures.

5. Integer Division Documentation
The comments at eval.seq:10 and eval.seq:19 correctly explain the Seq 3.0 change, but there's no indication whether the Bool (overflow flag) should be checked.

Recommendation: Add a comment explaining why it's safe to ignore the overflow flag for Lisp integer operations.

6. Unbounded History Growth
history-load reads the entire history file into memory (vim-line.seq:418-429). The history-max-entries constant is defined (line 369) but never enforced.

Recommendation: Implement history size limiting in history-add or history-load.

Minor Issues

7. Magic Numbers

Line 253: i.divide drop - Would benefit from a helper like i.div-truncate
Line 659: 10 i./ drop - Inconsistent with other division operations

Recommendation: Consider adding helper functions for common integer operations.

8. Test Coverage Gaps
The test file tests/seq/test_history.seq only tests basic history operations. Missing tests for:

History persistence (save/load)
History deduplication
Empty string handling
String stripping edge cases
Vim editing commands (motions, operators, mode transitions)

Recommendation: Add integration tests for the vim editor itself, not just history.

9. Incomplete Operator Implementation
The PR description mentions "Still pending: dw, db, d0, cw, cb, c0, yy, y+motion, p, P". These operators are partially implemented but commented out or incomplete.

Recommendation: Either complete these operators or remove the dead code to reduce confusion.

🚀 Performance Considerations

Good

History stored as linked list (snil/scons) is appropriate for prepend-heavy workload
O(1) history additions (prepend to front)
Lazy history loading (only on startup)

Potential Concerns

O(n) history lookup: history-get traverses the list (vim-line.seq:342-362)
O(n) string operations: string-delete-at and string-insert-at create new strings
Re-rendering on every keystroke: Clears and rewrites the entire line

These are acceptable for a REPL with typical line lengths (<1000 chars) and history sizes (<1000 entries). No optimization needed unless users report sluggishness.

🔒 Security Concerns

Low Risk

No command injection vectors (doesn't shell out)
No SQL injection (doesn't use databases)
No obvious buffer overflows (Seq strings are safe)

Potential Issues

Path traversal: history-file-path uses $HOME directly without sanitization. If $HOME is maliciously set, could write to arbitrary locations.

Mitigation: This is low risk because setting $HOME requires shell access, but consider validating the path.
Terminal escape injection: The editor writes user-controlled text to the terminal. If a user pastes malicious escape sequences, they could manipulate the terminal.

Mitigation: Seq's io.write should handle this, but consider sanitizing control characters in user input.

🧪 Test Coverage

Existing Tests

✅ Basic history operations (add, get, length)
✅ Lisp tests pass (449 tests)
✅ LSP tests pass (6 tests)

Missing Tests

❌ History persistence (save/load from file)
❌ History deduplication edge cases
❌ Vim editor commands (h/l/i/a/x/dd/cc/etc.)
❌ Mode transitions (normal ↔ insert)
❌ Arrow key handling
❌ Escape sequence parsing
❌ Multi-line input handling in REPL
❌ Terminal edge cases (very long lines, cursor at boundaries)

Recommendation: Add integration tests that simulate keystrokes and verify buffer state.

📋 Recommendations Summary

Must Fix (Blocking)

None - the PR is functionally correct and all tests pass.

Should Fix (High Priority)

Implement history size limiting using history-max-entries
Add integration tests for vim editor commands
Clarify stack management in repl-loop and vim-edit

Nice to Have (Low Priority)

Document or fix history file race condition for multiple sessions
Add error logging for I/O failures
Validate terminal escape sequences for security
Consider extracting vim-line.seq as a standalone library

🎯 Conclusion

Recommendation: APPROVE with minor follow-up

This is excellent work that successfully achieves the goal of replacing libedit with a pure Seq implementation. The code is well-structured, thoroughly documented, and demonstrates impressive mastery of both Seq and vim-style editing.

The issues identified are mostly minor and can be addressed in follow-up PRs. The core functionality is solid, and the integration is clean.

Suggested next steps:

Merge this PR to unblock Issue #52
Create follow-up issues for history size limiting and test coverage
Consider documenting the vim keybindings in the README or help text

Great job! 🎉

Review generated by Claude Code

## Pull Request Review: Vim-style Line Editor Implementation ### Summary This PR successfully implements a vim-style modal line editor in pure Seq, replacing the libedit dependency. This is a significant achievement that adds 865 lines of well-structured editor code with history management and terminal control. All tests pass (449 Lisp + 6 LSP). --- ## ✅ Strengths ### 1. **Excellent Code Organization** - Clear separation of concerns with well-named sections (Constants, Character Classification, History Management, etc.) - Comprehensive inline comments explaining stack effects and logic - Consistent naming conventions following Seq style ### 2. **Robust Feature Implementation** - **Modal editing**: Normal and Insert modes with proper state management - **Vim motions**: h/l/0/$/w/b for navigation - **Operators**: dd/cc/D/C/r/x for editing - **History**: Persistent history with load/save to `~/.seqlisp_history` - **Arrow key support**: Proper escape sequence handling with non-blocking read ### 3. **Smart Bug Fixes in eval.seq** The fixes to `i.divide` are correct - Seq 3.0 changed the return type from `Int` to `(Int, Bool)`: - `src/eval.seq:1332`: Division with remainder - properly drops the Bool - `src/eval.seq:1596`: Modulo operation - correctly handles the tuple return ### 4. **Clean Integration** - Seamless replacement of libedit with minimal changes to repl.seq - Proper stack management throughout the editor loop - History integrated naturally into the REPL workflow --- ## ⚠️ Issues & Concerns ### Critical Issues **1. Stack Management Complexity (vim-line.seq:159-162)** ```seq # vim-edit returns (History Buffer) where History is unchanged # History(0) is same as History(3), just drop the copy drop # History(2) Acc(1) Env(0) ``` The comment says "History is unchanged" but the implementation returns a modified History from `vim-edit`. This creates confusion: - If History can be modified (by k/j navigation adding entries), the comment is misleading - The stack juggling here is complex and error-prone **Recommendation**: Consider refactoring to make it clearer whether History is mutable or immutable in the vim-edit call. **2. History Duplication Check Logic (vim-line.seq:316-327)** ```seq over scar sstring-val # History(2) NewString(1) OldString(0) 1 pick # History(3) NewString(2) OldString(1) NewString(0) string.equal? if # History(2) NewString(1) AreEqual(0) ``` The duplicate check compares with the most recent entry, but the stack manipulation is fragile. If the History is empty (snil), this would fail before reaching the `snil?` check. **Wait, this is protected by the `over snil? if` check at line 313.** Actually, this is fine - good defensive programming. **3. History File Race Condition** The `history-save` function (vim-line.seq:408-415) uses `file.spit` which will overwrite the file. If multiple REPL sessions are running, the last one to exit will clobber others' history. **Recommendation**: Consider appending to history or using file locking, or document this limitation. ### Moderate Issues **4. Missing Error Handling** - `history-load` silently returns empty history on file read errors (line 424) - `history-save` ignores write failures (line 414) - No validation of terminal width/height or escape sequence success **Recommendation**: Add debug logging or error reporting for I/O failures. **5. Integer Division Documentation** The comments at eval.seq:10 and eval.seq:19 correctly explain the Seq 3.0 change, but there's no indication whether the Bool (overflow flag) should be checked. **Recommendation**: Add a comment explaining why it's safe to ignore the overflow flag for Lisp integer operations. **6. Unbounded History Growth** `history-load` reads the entire history file into memory (vim-line.seq:418-429). The `history-max-entries` constant is defined (line 369) but never enforced. **Recommendation**: Implement history size limiting in `history-add` or `history-load`. ### Minor Issues **7. Magic Numbers** - Line 253: `i.divide drop` - Would benefit from a helper like `i.div-truncate` - Line 659: `10 i./ drop` - Inconsistent with other division operations **Recommendation**: Consider adding helper functions for common integer operations. **8. Test Coverage Gaps** The test file `tests/seq/test_history.seq` only tests basic history operations. Missing tests for: - History persistence (save/load) - History deduplication - Empty string handling - String stripping edge cases - Vim editing commands (motions, operators, mode transitions) **Recommendation**: Add integration tests for the vim editor itself, not just history. **9. Incomplete Operator Implementation** The PR description mentions "Still pending: dw, db, d0, cw, cb, c0, yy, y+motion, p, P". These operators are partially implemented but commented out or incomplete. **Recommendation**: Either complete these operators or remove the dead code to reduce confusion. --- ## 🚀 Performance Considerations ### Good - History stored as linked list (snil/scons) is appropriate for prepend-heavy workload - O(1) history additions (prepend to front) - Lazy history loading (only on startup) ### Potential Concerns - **O(n) history lookup**: `history-get` traverses the list (vim-line.seq:342-362) - **O(n) string operations**: `string-delete-at` and `string-insert-at` create new strings - **Re-rendering on every keystroke**: Clears and rewrites the entire line These are acceptable for a REPL with typical line lengths (<1000 chars) and history sizes (<1000 entries). No optimization needed unless users report sluggishness. --- ## 🔒 Security Concerns ### Low Risk - No command injection vectors (doesn't shell out) - No SQL injection (doesn't use databases) - No obvious buffer overflows (Seq strings are safe) ### Potential Issues - **Path traversal**: `history-file-path` uses `$HOME` directly without sanitization. If `$HOME` is maliciously set, could write to arbitrary locations. **Mitigation**: This is low risk because setting `$HOME` requires shell access, but consider validating the path. - **Terminal escape injection**: The editor writes user-controlled text to the terminal. If a user pastes malicious escape sequences, they could manipulate the terminal. **Mitigation**: Seq's `io.write` should handle this, but consider sanitizing control characters in user input. --- ## 🧪 Test Coverage ### Existing Tests - ✅ Basic history operations (add, get, length) - ✅ Lisp tests pass (449 tests) - ✅ LSP tests pass (6 tests) ### Missing Tests - ❌ History persistence (save/load from file) - ❌ History deduplication edge cases - ❌ Vim editor commands (h/l/i/a/x/dd/cc/etc.) - ❌ Mode transitions (normal ↔ insert) - ❌ Arrow key handling - ❌ Escape sequence parsing - ❌ Multi-line input handling in REPL - ❌ Terminal edge cases (very long lines, cursor at boundaries) **Recommendation**: Add integration tests that simulate keystrokes and verify buffer state. --- ## 📋 Recommendations Summary ### Must Fix (Blocking) None - the PR is functionally correct and all tests pass. ### Should Fix (High Priority) 1. **Implement history size limiting** using `history-max-entries` 2. **Add integration tests** for vim editor commands 3. **Clarify stack management** in `repl-loop` and `vim-edit` ### Nice to Have (Low Priority) 4. Document or fix history file race condition for multiple sessions 5. Add error logging for I/O failures 6. Validate terminal escape sequences for security 7. Consider extracting vim-line.seq as a standalone library --- ## 🎯 Conclusion **Recommendation: APPROVE with minor follow-up** This is excellent work that successfully achieves the goal of replacing libedit with a pure Seq implementation. The code is well-structured, thoroughly documented, and demonstrates impressive mastery of both Seq and vim-style editing. The issues identified are mostly minor and can be addressed in follow-up PRs. The core functionality is solid, and the integration is clean. **Suggested next steps:** 1. Merge this PR to unblock Issue #52 2. Create follow-up issues for history size limiting and test coverage 3. Consider documenting the vim keybindings in the README or help text Great job! 🎉 --- *Review generated by Claude Code*