navicore/patch-rexx

Fork 0

vi mode #22

Merged

navicore merged 4 commits from line-editor into main

2026-02-12 23:58:50 +00:00

navicore commented

2026-02-12 23:17:41 +00:00

(Migrated from github.com)

No description provided.

claude[bot] commented

2026-02-12 23:20:49 +00:00

(Migrated from github.com)

Review: PR #22 "vi mode"

This PR replaces rustyline with vim-line + crossterm for vi-mode REPL editing. I've identified several issues that need to be addressed:

Critical Issues

1. Terminal not restored on parser/evaluator errors — src/repl.rs:158-179

In the submit() function, raw mode is disabled before running the REXX code, but if run_line() returns an error, the function re-enables raw mode and continues the REPL. However, if enable_raw_mode() at line 179 fails (e.g., if the terminal is no longer available), it will panic with expect("failed to re-enable raw mode").

Problem: If the terminal cannot re-enter raw mode (SIGTERM during execution, terminal closed, etc.), the REPL will panic instead of gracefully exiting.

Fix: Change line 179 to handle the error:

if enable_raw_mode().is_err() {
    return true; // Exit REPL gracefully
}

2. Panic hook captures `original_hook` but never restores it — src/repl.rs:199-205

The panic hook is installed but never removed. When the REPL exits normally (Ctrl-D, EXIT command, Action::Cancel), the custom panic hook remains installed for the entire process lifetime.

Problem: If a panic occurs after the REPL exits (unlikely but possible in destructors, atexit handlers, or if repl::run() is called multiple times), the panic hook will attempt to call disable_raw_mode() when raw mode is not active, which is harmless but incorrect.

Impact: Low severity but violates cleanup expectations.

Fix: Restore the original hook before returning:

// Before line 303
std::panic::set_hook(original_hook);

When navigating history with j/k in normal mode, the code calls editor.set_cursor(input.len(), &input) (lines 242, 250) to position the cursor at the end of the loaded history entry.

Problem: The second parameter to set_cursor is the buffer content, but at this point input has already been replaced with the history entry. This is correct. However, the prompt used in render_line is hardcoded to "rexx: " (lines 244, 252), which is the normal-mode prompt. This is inconsistent with the actual prompt calculation at lines 295-299.

Impact: Low severity but creates visual inconsistency — the prompt will always show normal mode (rexx: ) even if the user is in insert mode when they press j/k (though vim-line likely ensures this can't happen).

Recommendation: Extract prompt selection to a helper function to maintain consistency.

4. Ctrl-D doesn't save history — src/repl.rs:219-224

Ctrl-D breaks out of the event loop immediately without calling history.save(). History is only saved at line 305, after the loop.

Problem: If the user exits with Ctrl-D, history is saved. If they exit via EXIT command or Action::Cancel, history is saved. This is actually correct — Ctrl-D does reach line 305. False alarm on my part; the code is correct.

Safety and Robustness

5. `event::read()` error handling is ignored — src/repl.rs:214

The loop uses while let Ok(ev) = event::read(). If event::read() returns an error, the loop exits, raw mode is disabled, and history is saved. This is correct.

No issue here.

6. Cursor position calculation assumes valid UTF-8 — src/repl.rs:141

Line 141: input[..cursor.min(input.len())].chars().count()

Problem: If cursor (a byte offset) is not on a UTF-8 character boundary, slicing input[..cursor] will panic with "byte index X is not a char boundary".

Impact: If vim-line ever returns an invalid byte offset (e.g., due to multibyte character handling bugs), the REPL will crash.

Fix: Add validation:

let cursor_offset = cursor.min(input.len());
let cursor_offset = input.floor_char_boundary(cursor_offset); // Rust 1.83+
let cursor_col = prompt.len() + input[..cursor_offset].chars().count();

Or use input.char_indices() to iterate safely.

REXX Semantic Correctness

7. No issues with REXX semantics in this PR

The REPL correctly:

Trims input and passes it to run_line()
Handles empty lines (no-op)
Uses case-insensitive comparison for exit (line 166)
Preserves the environment across REPL iterations (correct REXX semantics — variables persist)
Does not interfere with argument handling (args are always empty in REPL mode, which is correct)

The stdin handling in main.rs:68-89 is also correct — it reads piped stdin as a script, which matches standard REXX behavior (e.g., Regina REXX).

Summary

Must fix before merge:

Line 179: Handle enable_raw_mode() error instead of panicking
Line 141: Validate cursor byte offset to prevent panics on multibyte characters

Should fix:
3. Restore original panic hook before returning (lines 303)

Minor:
4. Extract prompt selection to helper function for consistency

No issues:

REXX semantics are preserved
History saving works correctly for all exit paths
Stdin handling is correct

Let me know if you'd like me to prepare a patch for issues #1 and #6.

## Review: PR #22 "vi mode" This PR replaces `rustyline` with `vim-line` + `crossterm` for vi-mode REPL editing. I've identified several issues that need to be addressed: --- ### Critical Issues #### 1. **Terminal not restored on parser/evaluator errors** — src/repl.rs:158-179 In the `submit()` function, raw mode is disabled before running the REXX code, but if `run_line()` returns an error, the function re-enables raw mode and continues the REPL. However, if `enable_raw_mode()` at line 179 fails (e.g., if the terminal is no longer available), it will **panic** with `expect("failed to re-enable raw mode")`. **Problem**: If the terminal cannot re-enter raw mode (SIGTERM during execution, terminal closed, etc.), the REPL will panic instead of gracefully exiting. **Fix**: Change line 179 to handle the error: ```rust if enable_raw_mode().is_err() { return true; // Exit REPL gracefully } ``` --- #### 2. **Panic hook captures `original_hook` but never restores it** — src/repl.rs:199-205 The panic hook is installed but never removed. When the REPL exits normally (Ctrl-D, EXIT command, Action::Cancel), the custom panic hook remains installed for the entire process lifetime. **Problem**: If a panic occurs after the REPL exits (unlikely but possible in destructors, atexit handlers, or if `repl::run()` is called multiple times), the panic hook will attempt to call `disable_raw_mode()` when raw mode is not active, which is harmless but incorrect. **Impact**: Low severity but violates cleanup expectations. **Fix**: Restore the original hook before returning: ```rust // Before line 303 std::panic::set_hook(original_hook); ``` --- #### 3. **History navigation doesn't preserve cursor position correctly** — src/repl.rs:239-256 When navigating history with `j`/`k` in normal mode, the code calls `editor.set_cursor(input.len(), &input)` (lines 242, 250) to position the cursor at the end of the loaded history entry. **Problem**: The second parameter to `set_cursor` is the buffer content, but at this point `input` has already been replaced with the history entry. This is correct. However, the prompt used in `render_line` is hardcoded to `"rexx: "` (lines 244, 252), which is the normal-mode prompt. This is **inconsistent** with the actual prompt calculation at lines 295-299. **Impact**: Low severity but creates visual inconsistency — the prompt will always show normal mode (`rexx: `) even if the user is in insert mode when they press `j`/`k` (though vim-line likely ensures this can't happen). **Recommendation**: Extract prompt selection to a helper function to maintain consistency. --- #### 4. **Ctrl-D doesn't save history** — src/repl.rs:219-224 Ctrl-D breaks out of the event loop immediately without calling `history.save()`. History is only saved at line 305, **after** the loop. **Problem**: If the user exits with Ctrl-D, history is saved. If they exit via EXIT command or Action::Cancel, history is saved. This is actually **correct** — Ctrl-D does reach line 305. False alarm on my part; the code is correct. --- ### Safety and Robustness #### 5. **`event::read()` error handling is ignored** — src/repl.rs:214 The loop uses `while let Ok(ev) = event::read()`. If `event::read()` returns an error, the loop exits, raw mode is disabled, and history is saved. This is correct. **No issue here.** --- #### 6. **Cursor position calculation assumes valid UTF-8** — src/repl.rs:141 Line 141: `input[..cursor.min(input.len())].chars().count()` **Problem**: If `cursor` (a byte offset) is not on a UTF-8 character boundary, slicing `input[..cursor]` will **panic** with "byte index X is not a char boundary". **Impact**: If `vim-line` ever returns an invalid byte offset (e.g., due to multibyte character handling bugs), the REPL will crash. **Fix**: Add validation: ```rust let cursor_offset = cursor.min(input.len()); let cursor_offset = input.floor_char_boundary(cursor_offset); // Rust 1.83+ let cursor_col = prompt.len() + input[..cursor_offset].chars().count(); ``` Or use `input.char_indices()` to iterate safely. --- ### REXX Semantic Correctness #### 7. **No issues with REXX semantics in this PR** The REPL correctly: - Trims input and passes it to `run_line()` - Handles empty lines (no-op) - Uses case-insensitive comparison for `exit` (line 166) - Preserves the environment across REPL iterations (correct REXX semantics — variables persist) - Does not interfere with argument handling (args are always empty in REPL mode, which is correct) The stdin handling in `main.rs:68-89` is also correct — it reads piped stdin as a script, which matches standard REXX behavior (e.g., Regina REXX). --- ### Summary **Must fix before merge:** 1. Line 179: Handle `enable_raw_mode()` error instead of panicking 2. Line 141: Validate cursor byte offset to prevent panics on multibyte characters **Should fix:** 3. Restore original panic hook before returning (lines 303) **Minor:** 4. Extract prompt selection to helper function for consistency **No issues:** - REXX semantics are preserved - History saving works correctly for all exit paths - Stdin handling is correct --- Let me know if you'd like me to prepare a patch for issues #1 and #6.

claude[bot] commented

2026-02-12 23:23:06 +00:00

(Migrated from github.com)

PR Review: vi mode (REXX Interpreter)

Reviewed the REPL replacement from rustyline to vim-line + crossterm. Found 3 critical safety issues that can cause panics and terminal corruption, plus several medium-priority bugs.

🚨 CRITICAL ISSUES (Must Fix)

1. Panic on Terminal Mode Setup - src/repl.rs:207, 179

Severity: HIGH

enable_raw_mode().expect("failed to enable raw mode");  // Line 207
enable_raw_mode().expect("failed to re-enable raw mode");  // Line 179

Problem: Panics if:

Running without a TTY (Docker, CI, automated tests)
Terminal doesn't support raw mode
Permission issues

Impact: Application crashes instead of graceful error. This is problematic since main.rs:68 checks is_terminal() but REPL still calls enable_raw_mode() unconditionally.

Fix:

enable_raw_mode().map_err(|e| {
    eprintln!("rexx: cannot enable raw mode: {e}");
    std::process::exit(1);
})?;

2. UTF-8 Byte Boundary Panic - src/repl.rs:141

Severity: HIGH

let cursor_col = prompt.len() + input[..cursor.min(input.len())].chars().count();

Problem: cursor is a byte offset from vim-line, but code uses it as character index for slicing. Panics if cursor falls mid-character in multi-byte UTF-8.

Example that crashes:

let input = "你好";  // 6 bytes, 2 chars
let cursor = 3;      // middle of first character
input[..cursor]      // PANICS: byte 3 not a char boundary

Impact:

Crashes on emoji, Chinese, or multi-byte chars
Terminal left in raw mode (corrupted)

Fix: Validate character boundary or convert to char index before slicing.

3. Terminal State Not Restored on Early Exit - src/repl.rs:223, 294

Severity: HIGH

// Ctrl-D handler
if key_event.modifiers.contains(KeyModifiers::CONTROL)
    && key_event.code == CtKeyCode::Char('d')
{
    break;  // Line 223 - exits without disable_raw_mode()
}

// Cancel action
Action::Cancel => {
    break;  // Line 294 - same issue
}

Problem: Early loop exits skip cleanup at line 309:

let _ = disable_raw_mode();  // Line 309 - never reached

Impact: Terminal left in raw mode:

No echo, no line buffering
Ctrl-C doesn't work
Shell unusable until reset or stty sane

Fix: Use a Drop guard:

struct RawModeGuard;
impl Drop for RawModeGuard {
    fn drop(&mut self) { let _ = disable_raw_mode(); }
}
let _guard = RawModeGuard;
enable_raw_mode()?;

⚠️ HIGH-PRIORITY ISSUES

4. History File Race Condition - src/repl.rs:47-55

Severity: MEDIUM

fn save(&self) {
    let Ok(mut file) = fs::File::create(&path) else { return };
    // ...
}

Problem: Concurrent REPL instances cause data loss:

Instance A reads history
Instance B reads history
Instance A writes (truncates file)
Instance B writes (truncates, loses A's entries)

Fix: Use file locking or append-only writes with deduplication on load.

5. Silent History Save Failures - src/repl.rs:49-50

Severity: MEDIUM

let Ok(mut file) = fs::File::create(&path) else {
    return;  // Silently ignores disk full, permissions, I/O errors
};

Problem: Users have no indication history isn't persisted.

Fix:

Err(e) => {
    eprintln!("rexx: warning: cannot save history: {e}");
    return;
}

6. Missing Ctrl-C Handling - src/repl.rs:219-224

Severity: MEDIUM

Problem: Only Ctrl-D is handled. In raw mode, Ctrl-C doesn't generate signals. Expected behavior (clear line or exit) is missing.

Fix:

if key_event.modifiers.contains(KeyModifiers::CONTROL)
    && key_event.code == CtKeyCode::Char('c')
{
    input.clear();
    editor.reset();
    println!("^C");
    render_line("rexx> ", &input, editor.cursor());
    continue;
}

ℹ️ LOWER-PRIORITY ISSUES

7. Panic Hook Never Restored - src/repl.rs:199-205

Severity: LOW - Global state pollution

Custom panic hook persists after REPL exit, affecting entire process lifetime. Consider restoring original hook.

✅ VERIFIED CORRECT

REXX execution flow: Properly delegates to run_line(), preserves environment state
EXIT command: Correctly handled (line 167)
Error reporting: Runtime errors caught and displayed (line 171)
Piped stdin: New logic in main.rs:68-89 correctly detects and handles non-terminal input
History navigation: prev()/next() logic is sound
Key conversion: Properly maps crossterm to vim-line events

📋 SUMMARY

Issue	Severity	File:Line
Panic on `enable_raw_mode()`	CRITICAL	repl.rs:207, 179
UTF-8 byte boundary panic	CRITICAL	repl.rs:141
Terminal not restored on exit	CRITICAL	repl.rs:223, 294
History file race condition	High	repl.rs:47-55
Silent history save failures	Medium	repl.rs:49-50
Missing Ctrl-C handling	Medium	repl.rs:219-224

🎯 RECOMMENDATIONS

Must fix before merge:

Replace .expect() with proper error handling
Fix UTF-8 cursor calculation
Add Drop guard for terminal cleanup
Add Ctrl-C handling

Should fix:
5. File locking for history
6. Log history errors to stderr

The vi-mode implementation is well-structured with good test coverage, but the 3 critical panics must be addressed to ensure safe operation.

## PR Review: vi mode (REXX Interpreter) Reviewed the REPL replacement from `rustyline` to `vim-line` + `crossterm`. Found **3 critical safety issues** that can cause panics and terminal corruption, plus several medium-priority bugs. --- ## 🚨 CRITICAL ISSUES (Must Fix) ### 1. **Panic on Terminal Mode Setup** - src/repl.rs:207, 179 **Severity:** HIGH ```rust enable_raw_mode().expect("failed to enable raw mode"); // Line 207 enable_raw_mode().expect("failed to re-enable raw mode"); // Line 179 ``` **Problem:** Panics if: - Running without a TTY (Docker, CI, automated tests) - Terminal doesn't support raw mode - Permission issues **Impact:** Application crashes instead of graceful error. This is problematic since `main.rs:68` checks `is_terminal()` but REPL still calls `enable_raw_mode()` unconditionally. **Fix:** ```rust enable_raw_mode().map_err(|e| { eprintln!("rexx: cannot enable raw mode: {e}"); std::process::exit(1); })?; ``` --- ### 2. **UTF-8 Byte Boundary Panic** - src/repl.rs:141 **Severity:** HIGH ```rust let cursor_col = prompt.len() + input[..cursor.min(input.len())].chars().count(); ``` **Problem:** `cursor` is a **byte offset** from vim-line, but code uses it as character index for slicing. **Panics** if `cursor` falls mid-character in multi-byte UTF-8. **Example that crashes:** ```rust let input = "你好"; // 6 bytes, 2 chars let cursor = 3; // middle of first character input[..cursor] // PANICS: byte 3 not a char boundary ``` **Impact:** - Crashes on emoji, Chinese, or multi-byte chars - Terminal left in raw mode (corrupted) **Fix:** Validate character boundary or convert to char index before slicing. --- ### 3. **Terminal State Not Restored on Early Exit** - src/repl.rs:223, 294 **Severity:** HIGH ```rust // Ctrl-D handler if key_event.modifiers.contains(KeyModifiers::CONTROL) && key_event.code == CtKeyCode::Char('d') { break; // Line 223 - exits without disable_raw_mode() } // Cancel action Action::Cancel => { break; // Line 294 - same issue } ``` **Problem:** Early loop exits skip cleanup at line 309: ```rust let _ = disable_raw_mode(); // Line 309 - never reached ``` **Impact:** Terminal left in raw mode: - No echo, no line buffering - Ctrl-C doesn't work - Shell unusable until `reset` or `stty sane` **Fix:** Use a `Drop` guard: ```rust struct RawModeGuard; impl Drop for RawModeGuard { fn drop(&mut self) { let _ = disable_raw_mode(); } } let _guard = RawModeGuard; enable_raw_mode()?; ``` --- ## ⚠️ HIGH-PRIORITY ISSUES ### 4. **History File Race Condition** - src/repl.rs:47-55 **Severity:** MEDIUM ```rust fn save(&self) { let Ok(mut file) = fs::File::create(&path) else { return }; // ... } ``` **Problem:** Concurrent REPL instances cause data loss: 1. Instance A reads history 2. Instance B reads history 3. Instance A writes (truncates file) 4. Instance B writes (truncates, loses A's entries) **Fix:** Use file locking or append-only writes with deduplication on load. --- ### 5. **Silent History Save Failures** - src/repl.rs:49-50 **Severity:** MEDIUM ```rust let Ok(mut file) = fs::File::create(&path) else { return; // Silently ignores disk full, permissions, I/O errors }; ``` **Problem:** Users have no indication history isn't persisted. **Fix:** ```rust Err(e) => { eprintln!("rexx: warning: cannot save history: {e}"); return; } ``` --- ### 6. **Missing Ctrl-C Handling** - src/repl.rs:219-224 **Severity:** MEDIUM **Problem:** Only Ctrl-D is handled. In raw mode, Ctrl-C doesn't generate signals. Expected behavior (clear line or exit) is missing. **Fix:** ```rust if key_event.modifiers.contains(KeyModifiers::CONTROL) && key_event.code == CtKeyCode::Char('c') { input.clear(); editor.reset(); println!("^C"); render_line("rexx> ", &input, editor.cursor()); continue; } ``` --- ## ℹ️ LOWER-PRIORITY ISSUES ### 7. **Panic Hook Never Restored** - src/repl.rs:199-205 **Severity:** LOW - Global state pollution Custom panic hook persists after REPL exit, affecting entire process lifetime. Consider restoring original hook. --- ## ✅ VERIFIED CORRECT - **REXX execution flow:** Properly delegates to `run_line()`, preserves environment state - **EXIT command:** Correctly handled (line 167) - **Error reporting:** Runtime errors caught and displayed (line 171) - **Piped stdin:** New logic in `main.rs:68-89` correctly detects and handles non-terminal input - **History navigation:** `prev()`/`next()` logic is sound - **Key conversion:** Properly maps crossterm to vim-line events --- ## 📋 SUMMARY | Issue | Severity | File:Line | |-------|----------|-----------| | Panic on `enable_raw_mode()` | **CRITICAL** | repl.rs:207, 179 | | UTF-8 byte boundary panic | **CRITICAL** | repl.rs:141 | | Terminal not restored on exit | **CRITICAL** | repl.rs:223, 294 | | History file race condition | High | repl.rs:47-55 | | Silent history save failures | Medium | repl.rs:49-50 | | Missing Ctrl-C handling | Medium | repl.rs:219-224 | --- ## 🎯 RECOMMENDATIONS **Must fix before merge:** 1. Replace `.expect()` with proper error handling 2. Fix UTF-8 cursor calculation 3. Add `Drop` guard for terminal cleanup 4. Add Ctrl-C handling **Should fix:** 5. File locking for history 6. Log history errors to stderr The vi-mode implementation is well-structured with good test coverage, but the **3 critical panics** must be addressed to ensure safe operation.

claude[bot] commented

2026-02-12 23:42:47 +00:00

(Migrated from github.com)

Review of PR #22: vi mode

This PR replaces rustyline with vim-line + crossterm to add vi-mode editing to the REPL. The implementation is generally solid with good test coverage. However, I identified several issues:

Critical Issues

1. Panic hook not properly restored (src/repl.rs:212-218, 322)

The panic hook captures original_hook by move, but line 322 calls drop(std::panic::take_hook()) which doesn't restore it — it just installs the default hook. The original hook is lost.

Fix:

// At line 322, replace:
drop(std::panic::take_hook());

// With:
let _ = std::panic::set_hook(original_hook);

However, this won't work because original_hook is moved into the closure. You need to restructure this:

// Don't capture original_hook in the closure
std::panic::set_hook(Box::new(move |info| {
    let _ = disable_raw_mode();
    let _ = io::stdout().flush();
    // Re-print panic without calling original hook here
    eprintln!("{info}");
}));

// Later, restore default
let _ = std::panic::take_hook();

Or use a scope guard pattern instead of a panic hook.

2. Panic hook not removed on early return (src/repl.rs:220-223)

If enable_raw_mode() fails at line 220, the function returns without removing the panic hook. The hook remains installed globally even after REPL exit.

Fix: Remove the hook before returning:

if enable_raw_mode().is_err() {
    let _ = std::panic::take_hook(); // Remove our hook
    eprintln!("rexx: cannot enable raw mode");
    return;
}

3. Unsafe cursor position handling with multi-byte characters (src/repl.rs:259, 267, 296, 302)

Lines 259, 267, 296, 302 call editor.set_cursor(input.len(), &input), passing the byte length as a cursor position. If vim-line's set_cursor() expects a character offset (not byte offset), this will crash or misbehave on multi-byte UTF-8 input like emoji or Chinese characters.

Verification needed: Check whether VimLineEditor::set_cursor() expects bytes or chars. If chars, use:

editor.set_cursor(input.chars().count(), &input);

Similarly, verify that editor.cursor() at line 142 returns bytes, not chars. If it returns chars, the conversion logic at lines 142-147 is incorrect.

Medium Issues

4. Fragile string comparisons for editor mode (src/repl.rs:245, 254, 312)

The code uses string equality checks like editor.status() == "INSERT". If vim-line changes the string format (e.g., to "Insert"), this breaks silently.

Suggestion: If vim-line provides constants or an enum for mode, use those. Otherwise, add a comment noting this is fragile and depends on vim-line's string format.

5. History test has incorrect expectations (src/repl.rs:343-344)

assert_eq!(h.prev("current"), Some("third"));
assert_eq!(h.prev("current"), Some("second"));  // Wrong!

After the first prev(), h.position is Some, so the second call shouldn't save "current" again — it should use the already-saved value. The test passes because the implementation matches the test, but the semantics are questionable.

In practice this doesn't matter because the REPL doesn't call prev() multiple times with different inputs, but the test is misleading. Consider:

assert_eq!(h.prev("current"), Some("third"));
assert_eq!(h.prev("ignored because already navigating"), Some("second"));

Minor Issues

6. History file race condition (src/repl.rs:47-56)

save() uses fs::File::create() which truncates. If two REPL instances run simultaneously, they may overwrite each other's history.

Not critical for a REPL, but consider using atomic write-rename if this becomes an issue.

7. Missing welcome message (src/main.rs:138-141)

The old REPL printed a welcome message. The new one doesn't. This is a UX regression, not a bug, but worth noting.

Summary

Must fix: Issues #1, #2, #3 (panic hook, cursor position)
Should fix: Issue #4 (string comparisons)
Nice to fix: Issues #5, #6, #7

The implementation is well-tested and handles most edge cases correctly. The main concerns are panic hook management and potential Unicode handling bugs.

## Review of PR #22: vi mode This PR replaces `rustyline` with `vim-line` + `crossterm` to add vi-mode editing to the REPL. The implementation is generally solid with good test coverage. However, I identified several issues: ### Critical Issues **1. Panic hook not properly restored (src/repl.rs:212-218, 322)** The panic hook captures `original_hook` by move, but line 322 calls `drop(std::panic::take_hook())` which doesn't restore it — it just installs the default hook. The original hook is lost. **Fix:** ```rust // At line 322, replace: drop(std::panic::take_hook()); // With: let _ = std::panic::set_hook(original_hook); ``` However, this won't work because `original_hook` is moved into the closure. You need to restructure this: ```rust // Don't capture original_hook in the closure std::panic::set_hook(Box::new(move |info| { let _ = disable_raw_mode(); let _ = io::stdout().flush(); // Re-print panic without calling original hook here eprintln!("{info}"); })); // Later, restore default let _ = std::panic::take_hook(); ``` Or use a scope guard pattern instead of a panic hook. **2. Panic hook not removed on early return (src/repl.rs:220-223)** If `enable_raw_mode()` fails at line 220, the function returns without removing the panic hook. The hook remains installed globally even after REPL exit. **Fix:** Remove the hook before returning: ```rust if enable_raw_mode().is_err() { let _ = std::panic::take_hook(); // Remove our hook eprintln!("rexx: cannot enable raw mode"); return; } ``` **3. Unsafe cursor position handling with multi-byte characters (src/repl.rs:259, 267, 296, 302)** Lines 259, 267, 296, 302 call `editor.set_cursor(input.len(), &input)`, passing the **byte length** as a cursor position. If `vim-line`'s `set_cursor()` expects a **character offset** (not byte offset), this will crash or misbehave on multi-byte UTF-8 input like emoji or Chinese characters. **Verification needed:** Check whether `VimLineEditor::set_cursor()` expects bytes or chars. If chars, use: ```rust editor.set_cursor(input.chars().count(), &input); ``` Similarly, verify that `editor.cursor()` at line 142 returns bytes, not chars. If it returns chars, the conversion logic at lines 142-147 is incorrect. ### Medium Issues **4. Fragile string comparisons for editor mode (src/repl.rs:245, 254, 312)** The code uses string equality checks like `editor.status() == "INSERT"`. If `vim-line` changes the string format (e.g., to "Insert"), this breaks silently. **Suggestion:** If `vim-line` provides constants or an enum for mode, use those. Otherwise, add a comment noting this is fragile and depends on vim-line's string format. **5. History test has incorrect expectations (src/repl.rs:343-344)** ```rust assert_eq!(h.prev("current"), Some("third")); assert_eq!(h.prev("current"), Some("second")); // Wrong! ``` After the first `prev()`, `h.position` is `Some`, so the second call shouldn't save `"current"` again — it should use the already-saved value. The test passes because the implementation matches the test, but the semantics are questionable. In practice this doesn't matter because the REPL doesn't call `prev()` multiple times with different inputs, but the test is misleading. Consider: ```rust assert_eq!(h.prev("current"), Some("third")); assert_eq!(h.prev("ignored because already navigating"), Some("second")); ``` ### Minor Issues **6. History file race condition (src/repl.rs:47-56)** `save()` uses `fs::File::create()` which truncates. If two REPL instances run simultaneously, they may overwrite each other's history. Not critical for a REPL, but consider using atomic write-rename if this becomes an issue. **7. Missing welcome message (src/main.rs:138-141)** The old REPL printed a welcome message. The new one doesn't. This is a UX regression, not a bug, but worth noting. --- ## Summary - **Must fix:** Issues #1, #2, #3 (panic hook, cursor position) - **Should fix:** Issue #4 (string comparisons) - **Nice to fix:** Issues #5, #6, #7 The implementation is well-tested and handles most edge cases correctly. The main concerns are panic hook management and potential Unicode handling bugs.

claude[bot] commented

2026-02-12 23:49:33 +00:00

(Migrated from github.com)

Code Review: Vi Mode REPL Implementation

I've reviewed PR #22 which replaces rustyline with vim-line + crossterm for vi mode support in the REPL. The implementation is solid overall, but I found several issues that need attention:

Critical Issues

1. Terminal state not restored on panic during key handling (src/repl.rs:217-224)

The panic hook is installed to restore the terminal, but if a panic occurs during the main event loop (lines 237-325), raw mode may not be disabled properly. Specifically:

The panic hook calls disable_raw_mode() (line 220)
But then the main loop continues and could panic during event::read() or key handling (lines 237-324)
If a panic occurs between lines 237-324, the hook runs, but then execution might resume in an inconsistent state

Impact: Terminal could be left in raw mode if event processing panics.

Recommendation: Wrap the entire event loop in a catch_unwind or ensure cleanup via a Drop guard:

struct RawModeGuard;
impl Drop for RawModeGuard {
    fn drop(&mut self) {
        let _ = disable_raw_mode();
    }
}

pub fn run(...) {
    // ... setup ...
    let _guard = RawModeGuard;
    enable_raw_mode()?;
    
    // event loop...
}

2. Potential infinite loop on `event::read()` errors (src/repl.rs:237)

The main loop is:

while let Ok(ev) = event::read() {
    // ...
}

If event::read() returns Err, the loop exits silently without restoring the terminal or saving history. This can happen due to:

Terminal I/O errors
Signal interruptions (though SIGINT is handled)
Broken pipe conditions

Impact: History not saved on error; terminal state potentially corrupted.

Recommendation: Handle errors explicitly:

loop {
    match event::read() {
        Ok(Event::Key(key_event)) => { /* handle */ }
        Ok(_) => continue,
        Err(e) => {
            eprintln\!("rexx: terminal error: {e}");
            break;
        }
    }
}

When navigating history with j/k, the code updates input and editor.set_cursor(), but if the user then makes an edit via vim-line (lines 284-290), the history position is NOT reset. This means:

User presses k → loads "old command" from history
User presses i then types → modifies the line
User presses Esc then k again → might jump to wrong history entry

Impact: Confusing history navigation after editing a recalled command.

Recommendation: Reset history position after applying edits from vim-line:

// Apply edits in reverse to preserve byte offsets
for edit in result.edits.into_iter().rev() {
    edit.apply(&mut input);
}
if \!result.edits.is_empty() {
    history.reset_position();  // Add this
}

Safety Issues

4. Unchecked char boundary assumption in render_line (src/repl.rs:142-146)

The code attempts to handle invalid UTF-8 byte positions:

let byte_pos = cursor.min(input.len());
let safe_pos = (0..=byte_pos)
    .rev()
    .find(|&i| input.is_char_boundary(i))
    .unwrap_or(0);

However, this assumes input is valid UTF-8. If input contains invalid UTF-8 (which shouldn't happen with crossterm, but could occur if vim-line has a bug), this could still panic.

Impact: Potential panic on malformed input (low probability but possible).

Recommendation: Since input: String is guaranteed to be valid UTF-8 in Rust, this is already safe. However, the comment on line 141 is misleading—it says "avoid panicking on multi-byte input" but the real issue is cursor position validation, not UTF-8 validity. Consider clarifying:

// Clamp cursor to a valid char boundary (handles mid-character positions from vim-line)

Correctness Issues

5. History deduplication not implemented (src/repl.rs:58-60)

The History::add method always appends, even if the command is identical to the most recent entry. This leads to:

rexx> say 1
rexx> say 1  
rexx> say 1

Creating three identical history entries.

Impact: Cluttered history file; poor UX.

Recommendation: Skip duplicates:

fn add(&mut self, line: &str) {
    if self.entries.last().map_or(true, |last| last \!= line) {
        self.entries.push(line.to_string());
    }
    self.reset_position();
}

6. Empty lines added to history (src/repl.rs:167-171)

The code adds trimmed input to history even when empty:

let trimmed = input.trim().to_string();
let should_exit = if trimmed.is_empty() {
    false
} else {
    history.add(&trimmed);  // Only added when non-empty
    // ...
}

Wait, I misread—this is actually correct. Empty lines are NOT added to history (line 171 only executes when trimmed is non-empty). No issue here.

7. Key::char constructor may not exist (src/repl.rs:194, 233)

The code uses:

editor.handle_key(Key::char('i'), input)

But I cannot verify from the diff whether vim_line::Key has a char constructor. If this method doesn't exist, the code won't compile.

Status: The code compiles (per your test output), so this is NOT an issue. Just noting for completeness.

Performance Issues

On each j/k press in normal mode:

input = entry.to_string();  // Allocates new String

For large history entries (e.g., multi-line REXX programs), this allocates on every navigation step.

Impact: Minor performance degradation when navigating history with long entries.

Recommendation: This is acceptable for a REPL. No change needed unless profiling shows issues.

9. History file rewritten on every exit (src/repl.rs:47-56)

History::save truncates and rewrites the entire file:

let Ok(mut file) = fs::File::create(&path) else { return };

For 1000 entries, this is ~50KB of writes per session.

Impact: Minor I/O overhead; potential data loss if process is killed during write.

Recommendation: This is standard for REPL history. No change needed.

Test Coverage

10. No tests for main REPL loop logic

The tests cover:

History navigation (lines 343-418)
convert_key mapping (lines 421-467)

But there are NO tests for:

submit function behavior
render_line edge cases (cursor at end, empty input, wide characters)
Main loop intercepts (Ctrl-D, Ctrl-C, Enter in insert mode)
Integration between history and vim-line editing

Impact: Critical paths untested; regressions possible.

Recommendation: Add integration tests for submit logic and key intercepts. At minimum:

#[test]
fn submit_with_exit_command() {
    // Mock environment and verify submit returns true
}

#[test]
fn ctrl_d_exits_immediately() {
    // Verify Ctrl-D bypass logic
}

Minor Issues

11. Inconsistent prompt symbols (src/repl.rs:268, 276, 319-322)

The code uses:

"rexx> " for INSERT mode
"rexx: " for NORMAL mode

But history navigation (lines 268, 276) hardcodes "rexx: " even though the mode might be INSERT.

Impact: Prompt shows NORMAL indicator during history navigation even in INSERT mode.

Recommendation: Use mode-aware prompt consistently:

let prompt = if editor.status() == "INSERT" { "rexx> " } else { "rexx: " };
render_line(prompt, &input, editor.cursor());

12. Unicode em-dash in banner (src/repl.rs:212)

println\!("rexx {} \u{2014} interactive mode", ...);

This uses U+2014 (EM DASH) which may not render on all terminals.

Impact: Cosmetic; some terminals show ? or garbled output.

Recommendation: Use ASCII dash for maximum compatibility:

println\!("rexx {} - interactive mode", ...);

Summary

Must Fix (Critical):

Terminal state restoration on panic (#1)
event::read() error handling (#2)
History position reset on edits (#3)

Should Fix (Correctness):
5. History deduplication (#5)
11. Consistent prompt during history nav (#11)

Nice to Have:
10. Test coverage for REPL loop (#10)
12. ASCII dash in banner (#12)

Not Issues:
4, 6, 7, 8, 9 (explained above)

Overall, this is a well-structured implementation with good test coverage for the core components. The critical issues are around error handling and terminal state management—standard pitfalls when working with raw terminal mode. Once fixed, this will be a solid vi-mode REPL.

Let me know if you'd like me to prepare a follow-up PR with fixes for issues #1-3 and #5.

## Code Review: Vi Mode REPL Implementation I've reviewed PR #22 which replaces `rustyline` with `vim-line` + `crossterm` for vi mode support in the REPL. The implementation is solid overall, but I found several issues that need attention: --- ### Critical Issues #### 1. **Terminal state not restored on panic during key handling** (src/repl.rs:217-224) The panic hook is installed to restore the terminal, but if a panic occurs during the main event loop (lines 237-325), raw mode may not be disabled properly. Specifically: - The panic hook calls `disable_raw_mode()` (line 220) - But then the main loop continues and could panic during `event::read()` or key handling (lines 237-324) - If a panic occurs between lines 237-324, the hook runs, but then execution might resume in an inconsistent state **Impact**: Terminal could be left in raw mode if event processing panics. **Recommendation**: Wrap the entire event loop in a catch_unwind or ensure cleanup via a Drop guard: ```rust struct RawModeGuard; impl Drop for RawModeGuard { fn drop(&mut self) { let _ = disable_raw_mode(); } } pub fn run(...) { // ... setup ... let _guard = RawModeGuard; enable_raw_mode()?; // event loop... } ``` --- #### 2. **Potential infinite loop on `event::read()` errors** (src/repl.rs:237) The main loop is: ```rust while let Ok(ev) = event::read() { // ... } ``` If `event::read()` returns `Err`, the loop exits silently without restoring the terminal or saving history. This can happen due to: - Terminal I/O errors - Signal interruptions (though SIGINT is handled) - Broken pipe conditions **Impact**: History not saved on error; terminal state potentially corrupted. **Recommendation**: Handle errors explicitly: ```rust loop { match event::read() { Ok(Event::Key(key_event)) => { /* handle */ } Ok(_) => continue, Err(e) => { eprintln\!("rexx: terminal error: {e}"); break; } } } ``` --- #### 3. **History navigation doesn't reset position on direct edits** (src/repl.rs:259-281) When navigating history with j/k, the code updates `input` and `editor.set_cursor()`, but if the user then makes an edit via vim-line (lines 284-290), the history position is NOT reset. This means: 1. User presses `k` → loads "old command" from history 2. User presses `i` then types → modifies the line 3. User presses `Esc` then `k` again → might jump to wrong history entry **Impact**: Confusing history navigation after editing a recalled command. **Recommendation**: Reset history position after applying edits from vim-line: ```rust // Apply edits in reverse to preserve byte offsets for edit in result.edits.into_iter().rev() { edit.apply(&mut input); } if \!result.edits.is_empty() { history.reset_position(); // Add this } ``` --- ### Safety Issues #### 4. **Unchecked char boundary assumption in render_line** (src/repl.rs:142-146) The code attempts to handle invalid UTF-8 byte positions: ```rust let byte_pos = cursor.min(input.len()); let safe_pos = (0..=byte_pos) .rev() .find(|&i| input.is_char_boundary(i)) .unwrap_or(0); ``` However, this assumes `input` is valid UTF-8. If `input` contains invalid UTF-8 (which shouldn't happen with crossterm, but could occur if vim-line has a bug), this could still panic. **Impact**: Potential panic on malformed input (low probability but possible). **Recommendation**: Since `input: String` is guaranteed to be valid UTF-8 in Rust, this is already safe. However, the comment on line 141 is misleading—it says "avoid panicking on multi-byte input" but the real issue is cursor position validation, not UTF-8 validity. Consider clarifying: ```rust // Clamp cursor to a valid char boundary (handles mid-character positions from vim-line) ``` --- ### Correctness Issues #### 5. **History deduplication not implemented** (src/repl.rs:58-60) The `History::add` method always appends, even if the command is identical to the most recent entry. This leads to: ``` rexx> say 1 rexx> say 1 rexx> say 1 ``` Creating three identical history entries. **Impact**: Cluttered history file; poor UX. **Recommendation**: Skip duplicates: ```rust fn add(&mut self, line: &str) { if self.entries.last().map_or(true, |last| last \!= line) { self.entries.push(line.to_string()); } self.reset_position(); } ``` --- #### 6. **Empty lines added to history** (src/repl.rs:167-171) The code adds trimmed input to history even when empty: ```rust let trimmed = input.trim().to_string(); let should_exit = if trimmed.is_empty() { false } else { history.add(&trimmed); // Only added when non-empty // ... } ``` Wait, I misread—this is actually **correct**. Empty lines are NOT added to history (line 171 only executes when `trimmed` is non-empty). No issue here. --- #### 7. **Key::char constructor may not exist** (src/repl.rs:194, 233) The code uses: ```rust editor.handle_key(Key::char('i'), input) ``` But I cannot verify from the diff whether `vim_line::Key` has a `char` constructor. If this method doesn't exist, the code won't compile. **Status**: The code compiles (per your test output), so this is NOT an issue. Just noting for completeness. --- ### Performance Issues #### 8. **Quadratic string allocation in history navigation** (src/repl.rs:264-265, 272-273) On each j/k press in normal mode: ```rust input = entry.to_string(); // Allocates new String ``` For large history entries (e.g., multi-line REXX programs), this allocates on every navigation step. **Impact**: Minor performance degradation when navigating history with long entries. **Recommendation**: This is acceptable for a REPL. No change needed unless profiling shows issues. --- #### 9. **History file rewritten on every exit** (src/repl.rs:47-56) `History::save` truncates and rewrites the entire file: ```rust let Ok(mut file) = fs::File::create(&path) else { return }; ``` For 1000 entries, this is ~50KB of writes per session. **Impact**: Minor I/O overhead; potential data loss if process is killed during write. **Recommendation**: This is standard for REPL history. No change needed. --- ### Test Coverage #### 10. **No tests for main REPL loop logic** The tests cover: - `History` navigation (lines 343-418) - `convert_key` mapping (lines 421-467) But there are NO tests for: - `submit` function behavior - `render_line` edge cases (cursor at end, empty input, wide characters) - Main loop intercepts (Ctrl-D, Ctrl-C, Enter in insert mode) - Integration between history and vim-line editing **Impact**: Critical paths untested; regressions possible. **Recommendation**: Add integration tests for submit logic and key intercepts. At minimum: ```rust #[test] fn submit_with_exit_command() { // Mock environment and verify submit returns true } #[test] fn ctrl_d_exits_immediately() { // Verify Ctrl-D bypass logic } ``` --- ### Minor Issues #### 11. **Inconsistent prompt symbols** (src/repl.rs:268, 276, 319-322) The code uses: - `"rexx> "` for INSERT mode - `"rexx: "` for NORMAL mode But history navigation (lines 268, 276) hardcodes `"rexx: "` even though the mode might be INSERT. **Impact**: Prompt shows NORMAL indicator during history navigation even in INSERT mode. **Recommendation**: Use mode-aware prompt consistently: ```rust let prompt = if editor.status() == "INSERT" { "rexx> " } else { "rexx: " }; render_line(prompt, &input, editor.cursor()); ``` --- #### 12. **Unicode em-dash in banner** (src/repl.rs:212) ```rust println\!("rexx {} \u{2014} interactive mode", ...); ``` This uses U+2014 (EM DASH) which may not render on all terminals. **Impact**: Cosmetic; some terminals show `?` or garbled output. **Recommendation**: Use ASCII dash for maximum compatibility: ```rust println\!("rexx {} - interactive mode", ...); ``` --- ## Summary **Must Fix (Critical)**: 1. Terminal state restoration on panic (#1) 2. `event::read()` error handling (#2) 3. History position reset on edits (#3) **Should Fix (Correctness)**: 5. History deduplication (#5) 11. Consistent prompt during history nav (#11) **Nice to Have**: 10. Test coverage for REPL loop (#10) 12. ASCII dash in banner (#12) **Not Issues**: 4, 6, 7, 8, 9 (explained above) --- Overall, this is a well-structured implementation with good test coverage for the core components. The critical issues are around error handling and terminal state management—standard pitfalls when working with raw terminal mode. Once fixed, this will be a solid vi-mode REPL. Let me know if you'd like me to prepare a follow-up PR with fixes for issues #1-3 and #5.

No reviewers

No labels

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

navicore/patch-rexx!22

No description provided.

Rows
Columns

vi mode #22

Review: PR #22 "vi mode"

Critical Issues

1. Terminal not restored on parser/evaluator errors — src/repl.rs:158-179

2. Panic hook captures original_hook but never restores it — src/repl.rs:199-205

3. History navigation doesn't preserve cursor position correctly — src/repl.rs:239-256

4. Ctrl-D doesn't save history — src/repl.rs:219-224

Safety and Robustness

5. event::read() error handling is ignored — src/repl.rs:214

6. Cursor position calculation assumes valid UTF-8 — src/repl.rs:141

REXX Semantic Correctness

7. No issues with REXX semantics in this PR

Summary

PR Review: vi mode (REXX Interpreter)

🚨 CRITICAL ISSUES (Must Fix)

1. Panic on Terminal Mode Setup - src/repl.rs:207, 179

2. UTF-8 Byte Boundary Panic - src/repl.rs:141

3. Terminal State Not Restored on Early Exit - src/repl.rs:223, 294

⚠️ HIGH-PRIORITY ISSUES

4. History File Race Condition - src/repl.rs:47-55

5. Silent History Save Failures - src/repl.rs:49-50

6. Missing Ctrl-C Handling - src/repl.rs:219-224

ℹ️ LOWER-PRIORITY ISSUES

7. Panic Hook Never Restored - src/repl.rs:199-205

✅ VERIFIED CORRECT

📋 SUMMARY

🎯 RECOMMENDATIONS

Review of PR #22: vi mode

Critical Issues

Medium Issues

Minor Issues

Summary

Code Review: Vi Mode REPL Implementation

Critical Issues

1. Terminal state not restored on panic during key handling (src/repl.rs:217-224)

2. Potential infinite loop on event::read() errors (src/repl.rs:237)

3. History navigation doesn't reset position on direct edits (src/repl.rs:259-281)

Safety Issues

4. Unchecked char boundary assumption in render_line (src/repl.rs:142-146)

Correctness Issues

5. History deduplication not implemented (src/repl.rs:58-60)

6. Empty lines added to history (src/repl.rs:167-171)

7. Key::char constructor may not exist (src/repl.rs:194, 233)

Performance Issues

8. Quadratic string allocation in history navigation (src/repl.rs:264-265, 272-273)

9. History file rewritten on every exit (src/repl.rs:47-56)

Test Coverage

10. No tests for main REPL loop logic

Minor Issues

11. Inconsistent prompt symbols (src/repl.rs:268, 276, 319-322)

12. Unicode em-dash in banner (src/repl.rs:212)

Summary

2. Panic hook captures `original_hook` but never restores it — src/repl.rs:199-205

5. `event::read()` error handling is ignored — src/repl.rs:214

2. Potential infinite loop on `event::read()` errors (src/repl.rs:237)