I 274

[navicore]

#276

[claude]

PR Review: Terminal Builtins for Raw Input Mode

Summary

This PR adds terminal control builtins to enable interactive TUI applications in Seq. The implementation is well-structured and follows the codebase patterns. Below are my findings across code quality, potential issues, performance, security, and testing.

---

✅ Code Quality & Best Practices

Strengths:

• Consistent with codebase patterns: Follows the same structure as io.rs and os.rs modules with proper #[unsafe(no_mangle)] exports and stack effect documentation
• Clear documentation: Excellent module-level docs explaining signal safety contract and safety guarantees
• Good naming conventions: Function names follow the patch_seq_terminal_* pattern consistently
• Proper error handling: Uses default fallbacks (80x24) for terminal size queries
• Clean separation: Signal handling logic is well-separated from core functionality

Areas for improvement:

1. Inconsistent unsafe block placement (crates/runtime/src/terminal.rs:44-58):

   pub unsafe extern "C" fn patch_seq_terminal_raw_mode(stack: Stack) -> Stack {
       assert!(!stack.is_null(), "terminal_raw_mode: stack is empty");
       let (rest, value) = unsafe { pop(stack) };  // ← nested unsafe unnecessary

   Since the function is already unsafe extern "C", you don't need unsafe {} blocks inside. This pattern differs from os.rs where unsafe {} blocks are used consistently within unsafe functions. Recommendation: Choose one pattern and apply consistently across the module (I'd suggest explicit blocks as in os.rs for clarity).

2. Missing global_string usage (throughout terminal.rs):
   Other modules like os.rs use global_string() wrapper for string values to support potential future string interning optimizations. This module doesn't use it, which may cause inconsistency if string interning is implemented.

---

⚠️ Potential Bugs & Issues

Critical Issues

1. Race condition in signal handlers (terminal.rs:213-221):

if libc::sigaction(libc::SIGINT, &new_action, &mut old_sigint) == 0 {
    SAVED_SIGINT_ACTION = Some(old_sigint);
}

The SAVED_SIGINT_ACTION and SAVED_SIGTERM_ACTION are static mut accessed without synchronization. If multiple threads call enable_raw_mode() concurrently, this could cause undefined behavior.

Recommendation: Use AtomicBool for guards and consider parking_lot::Mutex or similar for the saved state, OR document that these functions are not thread-safe and must only be called from the main thread.

2. Potential terminal state leak on panic (terminal.rs:267-288):
If a panic occurs between enable_raw_mode() and disable_raw_mode(), the terminal won't be restored. The signal handlers only catch SIGINT/SIGTERM.

Recommendation: Consider using a Drop guard pattern:

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

Or document that applications using raw mode should set a panic hook.

3. File descriptor assumptions (terminal.rs:104, 159, others):
The code assumes:

• FD 0 = stdin
• FD 1 = stdout

This works in most cases but may fail if the process has closed/redirected these descriptors.

Recommendation: Use libc::STDIN_FILENO and libc::STDOUT_FILENO constants for clarity, and consider checking isatty() before operations that require a terminal.

Medium Issues

4. Non-blocking mode leak (terminal.rs:100-119):
patch_seq_terminal_read_char_nonblock temporarily sets O_NONBLOCK but if fcntl fails on restoration, the flag remains set.

Recommendation:

if flags >= 0 {
    unsafe { libc::fcntl(0, libc::F_SETFL, flags | libc::O_NONBLOCK) };
    // ... read ...
    unsafe { libc::fcntl(0, libc::F_SETFL, flags) };
}

5. fsync on stdout (terminal.rs:159):

unsafe { libc::fsync(1) };

fsync() is for file descriptors, not stdout. For stdout, you should use libc::fflush() or better yet, the existing STDOUT_MUTEX pattern from io.rs.

Recommendation:

use std::io::{self, Write};
let _ = io::stdout().flush();

---

🚀 Performance Considerations

Generally good, with minor concerns:

1. Repeated ioctl calls (terminal.rs:300-319): terminal.width and terminal.height call ioctl(TIOCGWINSZ) every time. For applications that check size frequently, consider caching with a refresh mechanism (though this may be premature optimization).

2. Atomic ordering: Using SeqCst for RAW_MODE_ENABLED (terminal.rs:22) is conservative but correct. Relaxed might suffice since there's no cross-thread coordination needed, but SeqCst is safer.

---

🔒 Security Concerns

Medium Severity

1. Signal handler safety (terminal.rs:186-199):
The signal handler calls non-async-signal-safe functions:

extern "C" fn signal_handler(sig: libc::c_int) {
    unsafe {
        if let Some(ref saved) = SAVED_TERMIOS {  // ← Rust references in signal handler
            libc::tcsetattr(0, libc::TCSANOW, saved);
        }
    }

According to POSIX, signal handlers should only call async-signal-safe functions. tcsetattr is safe, but Rust's reference handling and the Option match might not be. In practice, this is likely fine for terminal cleanup, but it's not strictly correct.

Recommendation: Document this limitation or restructure to avoid Rust-level operations in the signal handler.

2. No validation on terminal settings (terminal.rs:242-277):
The code directly modifies termios without validation. Malformed terminal state could theoretically be exploited, though this is low risk in practice.

---

🧪 Test Coverage

Current tests (terminal.rs:320-364):

• ✅ Terminal size queries
• ✅ Stack value types
• ✅ Raw mode toggle

Missing test coverage:

1. Signal handler behavior: No tests for SIGINT/SIGTERM during raw mode
2. Error conditions: No tests for non-TTY file descriptors
3. read-char functions: No tests for the actual read operations (though these are hard to test in CI)
4. Non-blocking behavior: No tests for terminal.read-char?
5. Concurrent access: No tests for thread safety
6. Integration test: The example terminal-demo.seq exists but isn't run as a test

Recommendations:

• Add integration tests that run the demo program
• Add tests for terminal detection (isatty())
• Consider property-based tests for terminal size values
• Add documentation about testing limitations (can't test interactivity in CI)

---

📝 Documentation

Good:

• Clear stack effect signatures
• Module-level safety documentation
• Inline comments explaining signal handling

Could be improved:

• Add examples in builtin docs (builtins.rs:1080-1103)
• Document thread-safety guarantees/limitations
• Add a note about Windows compatibility (currently Unix-only)
• Mention the demo program in the builtin docs

---

🎯 Additional Recommendations

1. Platform support: Consider adding Windows support or explicitly documenting Unix-only nature with compile-time checks:

   #[cfg(not(unix))]
   compile_error!("Terminal builtins require Unix");

2. Consistency with io.rs: The io.rs module uses STDOUT_MUTEX for thread safety. Terminal operations that write (like flush) should consider using the same mechanism.

3. Example program: The demo in terminal-demo.seq is excellent! Consider adding more examples showing:

   • Arrow key handling (escape sequences)
   • Terminal size change detection
   • Full-screen TUI pattern

---

Summary

This is a solid implementation that addresses a real need (issue #274/276). The code follows codebase conventions and the signal handling approach is thoughtful.

Before merging, please address:

1. 🔴 Signal handler thread safety (static mut access)
2. 🟡 Use fflush instead of fsync for stdout
3. 🟡 Consistent unsafe block usage
4. 🟡 Document thread-safety expectations
5. 🟢 Consider adding isatty() checks

The current implementation is functional for single-threaded use cases, which is likely the primary use case for TUI applications. Great work!

[claude]

Pull Request Review: Terminal Builtins Implementation

Thank you for this implementation! This is a solid addition that enables interactive TUI applications in Seq. Below is my detailed review covering code quality, potential issues, and suggestions.

---

✅ Strengths

Code Quality

• Well-documented: Excellent module-level documentation explaining platform support, thread safety, and signal safety considerations
• Consistent with codebase patterns: Follows the same structure as other runtime modules (e.g., time_ops.rs)
• Comprehensive API: The 6 terminal operations provide a complete foundation for TUI applications
• Good example: terminal-demo.seq demonstrates the API effectively

Safety & Robustness

• Signal handling: Properly installs SIGINT/SIGTERM handlers to restore terminal state on exit
• Graceful degradation: Operations check isatty() and silently degrade when not on a TTY
• State tracking: Uses AtomicBool to track raw mode state and prevent double-enable/disable
• Proper restoration: Saves original termios settings and restores them when exiting raw mode

---

🔍 Issues & Concerns

1. CRITICAL: Unsafe static mutable globals (terminal.rs:36-40)

static mut SAVED_TERMIOS: Option<libc::termios> = None;
static mut SAVED_SIGINT_ACTION: Option<libc::sigaction> = None;
static mut SAVED_SIGTERM_ACTION: Option<libc::sigaction> = None;

Problem: Accessing mutable static variables from multiple places without synchronization is undefined behavior in Rust. While the current usage may appear safe (single-threaded terminal operations), this violates Rust's safety guarantees.

Solution: Wrap these in Mutex or OnceCell:

use std::sync::Mutex;
use std::sync::OnceLock;

static SAVED_TERMIOS: Mutex<Option<libc::termios>> = Mutex::new(None);
static SAVED_SIGINT_ACTION: OnceLock<Mutex<libc::sigaction>> = OnceLock::new();
static SAVED_SIGTERM_ACTION: OnceLock<Mutex<libc::sigaction>> = OnceLock::new();

2. Signal Handler Safety Violation (terminal.rs:185-201)

extern "C" fn signal_handler(sig: libc::c_int) {
    unsafe {
        if let Some(ref saved) = SAVED_TERMIOS {  // ⚠️ Not async-signal-safe
            libc::tcsetattr(libc::STDIN_FILENO, libc::TCSANOW, saved);
        }
    }
    // ...
}

Problem: Accessing SAVED_TERMIOS (a static mut) from a signal handler is problematic. Signal handlers can interrupt code at any point, including while it's being modified. While tcsetattr is async-signal-safe, accessing Rust's Option with pattern matching is not guaranteed to be.

Solution: Use a raw pointer or atomic flag:

static SAVED_TERMIOS_PTR: AtomicPtr<libc::termios> = AtomicPtr::new(std::ptr::null_mut());

extern "C" fn signal_handler(sig: libc::c_int) {
    let ptr = SAVED_TERMIOS_PTR.load(Ordering::Relaxed);
    if !ptr.is_null() {
        unsafe { libc::tcsetattr(libc::STDIN_FILENO, libc::TCSANOW, &*ptr); }
    }
    // ... rest of handler
}

3. Memory Leak in Signal Handler (terminal.rs:189)

The saved termios struct is boxed/allocated but never freed. If raw mode is toggled multiple times, this could leak memory (though it's small).

Solution: Store inline in a static or use proper cleanup.

4. Platform Limitation Not Enforced (terminal.rs:8-9)

Documentation says "Unix-only" but there's no compile-time enforcement. This will compile on Windows and fail at runtime.

Solution: Add platform guards:

#[cfg(not(unix))]
compile_error!("Terminal operations require Unix (POSIX termios)");

Or provide stub implementations for non-Unix platforms.

5. Non-blocking Read May Fail to Restore Flags (terminal.rs:113-136)

If fcntl(F_SETFL) fails to restore flags (line 127), the function continues silently, leaving stdin in non-blocking mode.

Solution: At minimum, log the error or consider it more carefully:

if unsafe { libc::fcntl(libc::STDIN_FILENO, libc::F_SETFL, flags) } < 0 {
    eprintln!("Warning: Failed to restore stdin flags");
}

6. Missing Cleanup on Process Exit

If the process exits normally (without signals) while in raw mode, the terminal isn't restored unless disable_raw_mode() is called explicitly. Consider registering an atexit handler.

Solution:

use libc::atexit;

extern "C" fn cleanup_terminal() {
    if RAW_MODE_ENABLED.load(Ordering::SeqCst) {
        disable_raw_mode();
    }
}

// In enable_raw_mode(), register once:
static ATEXIT_REGISTERED: AtomicBool = AtomicBool::new(false);
if !ATEXIT_REGISTERED.swap(true, Ordering::SeqCst) {
    unsafe { atexit(cleanup_terminal); }
}

---

🧪 Testing

Current Coverage

The tests in terminal.rs:351-395 are minimal:

• Basic size queries (pass in CI/non-TTY environments)
• Raw mode toggle (may silently no-op in CI)
• No signal handler tests
• No edge case coverage (e.g., multiple enable/disable cycles, non-TTY behavior)

Recommendations

1. Add integration test: Create a test that spawns a pseudo-TTY and verifies raw mode actually works
2. Test signal handling: Mock signal delivery and verify terminal restoration
3. Test non-blocking read: Verify it doesn't block when no input is available
4. Test edge cases: Multiple enable/disable, calling on non-TTY, calling with stdin closed

---

🎯 Performance

No significant concerns. The operations are thin wrappers around libc calls with minimal overhead.

Minor optimization: get_terminal_size() (terminal.rs:313) could cache results for a short duration if called frequently in a tight loop, but this is likely premature optimization.

---

🔒 Security

Potential Issues

1. Terminal state corruption: If the process crashes before restoring terminal state, the user's terminal will be left in raw mode. The signal handlers mitigate this for SIGINT/SIGTERM, but not for SIGSEGV, SIGABRT, or panics.

2. DoS via rapid raw mode toggling: Repeatedly enabling/disabling raw mode could be abused, but this is an application-level concern.

3. No input validation: read_char returns raw bytes (0-255) without validation. This is expected, but applications should be careful with the values.

Recommendations

• Consider adding panic handlers to restore terminal state
• Document that raw mode should be used carefully (it's a global resource)

---

📝 Documentation & API

Strengths

• Stack effects are clearly documented
• Return values are well-explained (-1 for errors/EOF)
• The demo example is helpful

Suggestions

1. Add safety notes to public functions (terminal.rs:53):

   /// # Safety
   /// Must only be called from the main thread (terminal state is process-global)
   

2. Clarify non-blocking behavior in terminal.read-char? docs:
   Currently just says "Returns -1 otherwise" but should clarify it returns immediately.

3. Document signal handler limitations in module docs:
   Mention that SIGSEGV/SIGABRT won't restore terminal state.

4. Add example for typical cleanup pattern:

   : with-raw-mode ( quot -- )
     true terminal.raw-mode
     call
     false terminal.raw-mode
   ;
   

---

🔧 Minor Issues

1. Inconsistent spacing (builtins.rs:575):

   builtin!(sigs, "terminal.flush", (a - -a));  // Should be (a -- a)

2. Typo in comment (terminal.rs:25):
   "These functions are designed to be called ONLY by compiler-generated code."
   This is true for all runtime functions - not special to terminal ops.

3. Unused imports warning (potential):
   Double-check that all imports in terminal.rs are actually used.

---

✅ Summary

This is a well-implemented feature that follows project conventions and provides a solid foundation for TUI applications. The main concerns are:

1. ❗ Fix unsafe static mutables (safety violation)
2. ❗ Fix signal handler async-signal-safety (UB risk)
3. ⚠️ Add atexit handler for cleanup
4. ⚠️ Add platform guards or stubs for non-Unix
5. ⚠️ Improve test coverage

Once the unsafe statics and signal handler issues are resolved, this would be safe to merge. The other items are nice-to-haves that improve robustness.

---

🎉 Overall Assessment

Recommend: Request Changes

The implementation is solid and the feature is valuable, but the static mut usage and signal handler safety issues should be fixed before merging. These are relatively small changes that will make the code properly safe and future-proof.

Great work on this feature! 🚀

[claude]

Pull Request Review: Terminal Operations (PR #277)

Summary

This PR adds terminal control builtins to the Seq runtime, enabling raw terminal mode and character-level input for building TUI applications. The implementation adds 6 new builtin functions (terminal.raw-mode, terminal.read-char, terminal.read-char?, terminal.width, terminal.height, terminal.flush) along with an example demonstration program.

Overall Assessment

Status: Approved with minor suggestions ✓

This is a well-implemented feature with good documentation and sensible design choices. The code quality is high and follows existing patterns in the codebase.

---

Code Quality & Best Practices

Strengths ✓

• Excellent documentation: The module-level docs in terminal.rs:1-26 clearly explain platform support, thread safety, and signal safety
• Consistent patterns: Follows existing runtime module conventions (compare with time_ops.rs, io.rs)
• Proper error handling: Graceful degradation when stdin/stdout isn't a TTY (terminal.rs:252-254, terminal.rs:314-316)
• Signal safety: Thoughtful handling of SIGINT/SIGTERM with proper terminal restoration (terminal.rs:186-204)
• Clear stack effects: All functions document their stack signatures

Minor Suggestions

1. Consider using std::mem::MaybeUninit instead of std::mem::zeroed() (terminal.rs:256, terminal.rs:213-214, terminal.rs:318)

   • zeroed() is convenient but can be UB for types with invalid zero-bit patterns
   • MaybeUninit::zeroed().assume_init() is more explicit about intent
   • Not critical here since libc::termios and libc::sigaction are POD types

2. Documentation precision: The docs say "read a single byte" but the variable/function names use "char"

   • Consider clarifying that this reads bytes (0-255), not Unicode codepoints
   • The current docs at builtins.rs:1086 are accurate ("Read a single byte from stdin")

---

Potential Bugs & Issues

Critical: Mutable Static Safety ⚠️

Issue: The use of mutable statics without proper synchronization (terminal.rs:39-40)

static mut SAVED_TERMIOS: Option<libc::termios> = None;
static mut SAVED_SIGINT_ACTION: Option<libc::sigaction> = None;
static mut SAVED_SIGTERM_ACTION: Option<libc::sigaction> = None;

Problem: These are accessed from both:

• User code (via enable_raw_mode/disable_raw_mode)
• Signal handlers (via signal_handler:194-197)

Signal handlers can interrupt normal execution at any time, creating race conditions. While in practice this may "work" because signal handlers run on the same thread and most operations are atomic-ish, it's still undefined behavior.

Recommendations:

1. Short term: Document that terminal operations must only be called from a single thread (which you already state, but emphasize the signal handler interaction)
2. Medium term: Consider using AtomicBool for tracking state and document that this is only safe in single-threaded terminal apps
3. Long term: Use a proper mutex (though note that mutexes aren't async-signal-safe, so signal handlers would need a different approach)

Given that TUI applications are inherently single-threaded and the docs already state "not thread-safe", this is acceptable for v1 but worth a code comment explaining the signal handler interaction.

Minor Issues

1. Potential fd leak in read_char_nonblock (terminal.rs:114-126)

   • If a panic occurs between F_SETFL calls, flags won't be restored
   • Consider using a guard struct with Drop impl
   • Low priority since panics should be rare

2. Silent failures (terminal.rs:257-258, terminal.rs:278-279)

   • When tcgetattr/tcsetattr fail, the functions silently return
   • Consider logging errors or exposing failure states to Seq code
   • Current behavior (graceful degradation) is reasonable but worth noting

---

Performance Considerations

Positive ✓

• Minimal allocations (none in hot paths)
• Direct libc calls avoid Rust std overhead
• Non-blocking read properly uses fcntl rather than polling

Observations

• RAW_MODE_ENABLED uses SeqCst ordering (terminal.rs:247, terminal.rs:298) but Relaxed would suffice since there's no ordering dependency between threads
• Not critical, but Ordering::Relaxed would be more appropriate for the documented single-threaded usage

---

Security Concerns

Low Risk ✓

1. Input validation: All user inputs (Bool for raw-mode) are type-checked by the compiler
2. No buffer overflows: Single-byte reads have fixed buffer size
3. Signal handling: Signal handlers only use async-signal-safe functions (tcsetattr, signal, raise)
4. Resource cleanup: Terminal state is restored on exit

Considerations

1. DOS via raw mode: If a program crashes without disabling raw mode, the terminal can be left in a broken state

   • Mitigated by signal handlers, but not all signals are caught (SIGKILL can't be caught)
   • Consider documenting a recovery command (reset or stty sane) for users

2. TOCTOU in is_tty checks: The TTY status could theoretically change between check and use

   • Extremely unlikely in practice
   • Not a security issue, just a potential robustness edge case

---

Test Coverage

Existing Tests ✓

The PR includes unit tests in terminal.rs:345-395:

• ✓ test_terminal_size - dimensions return positive values
• ✓ test_terminal_width_stack - width works with stack operations
• ✓ test_terminal_height_stack - height works with stack operations
• ✓ test_raw_mode_toggle - raw mode enable/disable doesn't crash

Gaps & Suggestions

1. Missing: Non-blocking read test

   • terminal_read_char_nonblock is not tested
   • Could test that it returns -1 when no input available

2. Missing: Integration test

   • The example examples/terminal/terminal-demo.seq should be compiled and validated
   • Consider adding to CI (maybe with a non-interactive test variant)

3. Missing: Signal handler test

   • Difficult to test signal handlers in unit tests
   • Could be documented in manual testing checklist

4. Limited stack operation tests

   • read_char is not tested (hard to test without mock stdin)
   • Consider property-based tests: push/pop maintains stack integrity

Recommendation

Add a compile-time test for the example:

#[test]
fn test_terminal_example_compiles() {
    // Ensure the example at least compiles
    // This would require integration test setup
}

---

Architecture & Design

Excellent Choices ✓

1. Unix-only with graceful degradation: Appropriate for v1, avoids Windows complexity
2. Builtin vs FFI: Correct choice - builtins are easier to use and distribute
3. Minimal API surface: 6 functions cover the essential use cases without bloat
4. Signal handler design: Installing handlers only when raw mode is enabled is clean

Questions

1. Should terminal.read-char? use a timeout instead of pure non-blocking?

   • Current design: returns immediately with -1 if no input
   • Alternative: could accept a timeout in milliseconds
   • Current design is simpler and more composable

2. Should terminal dimensions be cached or always queried?

   • Current: queries on every call via ioctl
   • Impact: negligible for typical use
   • Could add SIGWINCH handler to update cached dimensions (future enhancement)

---

Comparison with Existing Code

The implementation quality matches existing runtime modules:

Aspect	terminal.rs	time_ops.rs	io.rs
Doc quality	Excellent	Excellent	Excellent
Safety annotations	Consistent	Consistent	Consistent
Error handling	Graceful	Graceful	Graceful
Test coverage	Basic	Basic	Basic

---

Recommendations Summary

Must Fix (before merge)

None - the code is functional and safe for its documented use case.

Should Fix (minor improvements)

1. Add explicit comment about signal handler interaction with mutable statics
2. Add test for terminal_read_char_nonblock
3. Consider integration test that compiles the example

Nice to Have (future work)

1. Windows support (probably via crossterm crate)
2. Cached terminal dimensions with SIGWINCH handler
3. Return error codes instead of silent failures
4. Replace std::mem::zeroed() with MaybeUninit

---

Conclusion

This is a solid implementation that adds valuable functionality to Seq. The code is well-documented, follows existing patterns, and handles edge cases thoughtfully. The identified issues are minor and don't block merging.

Approved with minor suggestions. ✓

Great work! The signal handler design is particularly well-thought-out, and the graceful degradation for non-TTY environments shows good defensive programming.