diff --git a/crates/jcode-tui/src/tui/app/commands.rs b/crates/jcode-tui/src/tui/app/commands.rs index 07f7b87ee4..37fd451bc3 100644 --- a/crates/jcode-tui/src/tui/app/commands.rs +++ b/crates/jcode-tui/src/tui/app/commands.rs @@ -3196,7 +3196,29 @@ fn run_interactive_editor_with( resume: impl FnOnce(), ) -> std::io::Result { suspend(); + // Publish only after the restores completed. The flag must never be + // observable while jcode's own modes are still armed: a termination + // signal firing mid-suspend would skip its cleanup and leave the shell in + // raw mode (greptile #4230626960). While the disable writes are in flight + // the flag is still false, so a signal in that window restores everything; + // the flag is then published under the same kitty state lock the signal + // cleanup takes, so cleanup either sees the completed handoff or runs in + // full. + crate::tui::publish_editor_terminal_handoff(); let result = command.status(); + // Reclaim before resume re-arms our modes: while the flag is still set a + // termination signal skips its cleanup, which must only happen while the + // editor really is the one controlling the tty. + crate::tui::reclaim_editor_terminal_handoff(); + // A termination signal that skipped its cleanup while the editor owned + // the terminal recorded termination under the kitty state lock before + // this reclaim ran, and the process is exiting. The reclaim leaves + // ownership set in that case; re-arming modes now would leave them + // enabled on the shell after death because no further cleanup runs + // (greptile #4232838288), so skip the resume entirely. + if crate::tui::editor_owns_terminal() { + return result; + } resume(); result } @@ -3204,6 +3226,10 @@ fn run_interactive_editor_with( fn suspend_terminal_for_editor() { use crossterm::event::{DisableBracketedPaste, DisableFocusChange, DisableMouseCapture}; + // The caller publishes the handoff only after this function returns, so a + // termination signal firing between these restores and the publication + // runs a full cleanup instead of skipping one (greptile #4230626960). + // // These commands are safe even when a mode was not enabled. Disable them // before leaving the alternate screen so the child receives normal input. let _ = crossterm::execute!( @@ -3219,6 +3245,15 @@ fn suspend_terminal_for_editor() { fn resume_terminal_after_editor() { use crossterm::event::{EnableBracketedPaste, EnableFocusChange, EnableMouseCapture}; + // Defense in depth for the run_interactive_editor_with early return: a + // termination signal already recorded that the process is exiting, so + // enabling modes here would leave them active on the shell after death + // (greptile #4232838288). + if crate::tui::editor_owns_terminal() { + return; + } + // The caller reclaimed the handoff before entering here, so a termination + // signal arriving while these modes are re-armed runs the full cleanup. // Re-enter the TUI before returning to the event loop. The existing // ratatui Terminal remains usable and the next loop iteration redraws it. let _ = std::panic::catch_unwind(std::panic::AssertUnwindSafe(ratatui::init)); @@ -3240,9 +3275,15 @@ mod interactive_editor_tests { use super::run_interactive_editor_with; use std::cell::RefCell; use std::rc::Rc; + use std::sync::Arc; + use std::sync::atomic::{AtomicBool, Ordering}; #[test] fn handoff_waits_for_editor_before_resuming_terminal() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); let events = Rc::new(RefCell::new(Vec::new())); let child_events = Rc::clone(&events); let mut command = std::process::Command::new("sh"); @@ -3271,6 +3312,10 @@ mod interactive_editor_tests { #[test] fn handoff_resumes_terminal_after_editor_failure_status() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); let resumed = Rc::new(RefCell::new(false)); let resumed_after = Rc::clone(&resumed); let mut command = std::process::Command::new("sh"); @@ -3287,6 +3332,145 @@ mod interactive_editor_tests { assert_eq!(status.code(), Some(7)); assert!(*resumed.borrow()); + assert!(!crate::tui::editor_owns_terminal()); + } + + #[test] + fn handoff_clears_editor_terminal_ownership_when_command_fails_to_launch() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); + let mut command = std::process::Command::new("/definitely/missing/jcode-editor-test"); + + let result = run_interactive_editor_with(&mut command, || {}, || {}); + + assert!(result.is_err()); + assert!(!crate::tui::editor_owns_terminal()); + } + + #[test] + fn handoff_publishes_ownership_only_while_editor_runs() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); + let saw_ownership = Arc::new(AtomicBool::new(false)); + let owns_during_resume = Rc::new(RefCell::new(true)); + + // The child stays alive until the sampler confirms the flag, so a busy + // runner delaying the sampler cannot shrink the observation window + // below what the sampler needs (greptile #4231010584). With a fixed + // child sleep the test failed on correct code whenever the sampler + // started late. The child gives up after ~5s so broken code that never + // publishes still fails the assertions instead of hanging. + let sentinel = std::env::temp_dir().join(format!( + "jcode-editor-ownership-{}-{}.sentinel", + std::process::id(), + line!() + )); + let _ = std::fs::remove_file(&sentinel); + + let sampler_sentinel = Arc::clone(&saw_ownership); + let sampler_saw = sentinel.clone(); + let sampler = std::thread::spawn(move || { + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while std::time::Instant::now() < deadline { + if crate::tui::editor_owns_terminal() { + sampler_sentinel.store(true, Ordering::SeqCst); + let _ = std::fs::write(&sampler_saw, b""); + return; + } + std::thread::sleep(std::time::Duration::from_millis(1)); + } + }); + + let mut command = std::process::Command::new("sh"); + command + .env("JCODE_OWNERSHIP_SENTINEL", &sentinel) + .args([ + "-c", + "i=0; while [ ! -f \"$JCODE_OWNERSHIP_SENTINEL\" ] && [ \"$i\" -lt 500 ]; do sleep 0.01; i=$((i+1)); done; [ -f \"$JCODE_OWNERSHIP_SENTINEL\" ]", + ]); + let owns_during_resume_after = Rc::clone(&owns_during_resume); + let status = run_interactive_editor_with( + &mut command, + // The restores are still in flight here: the flag must stay false + // so a termination signal in this window cleans up in full instead + // of skipping it and leaving the shell in raw mode (greptile + // #4230626960). + || assert!(!crate::tui::editor_owns_terminal()), + move || *owns_during_resume_after.borrow_mut() = crate::tui::editor_owns_terminal(), + ) + .expect("shell should launch"); + + sampler.join().expect("sampler thread"); + let _ = std::fs::remove_file(&sentinel); + + // Exit 0 means the sampler confirmed ownership while the child was + // still alive; a nonzero exit means the child gave up waiting, which + // only happens when the flag was never published. + assert_eq!( + status.code(), + Some(0), + "editor exited before the sampler observed ownership" + ); + assert!( + saw_ownership.load(Ordering::SeqCst), + "ownership was never published while the editor ran" + ); + // Reclaimed before resume re-armed jcode's modes. + assert!(!*owns_during_resume.borrow()); + assert!(!crate::tui::editor_owns_terminal()); + } + + #[test] + fn reclaim_resume_does_not_reenable_modes_after_termination_started() { + let _guard = crate::tui::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + crate::tui::set_editor_terminal_ownership(false); + + // Greptile #4232838288: reproduce the race window. The signal cleanup + // saw the editor handoff, skipped its restore, recorded termination + // under the kitty state lock, and released the lock while the process + // was still alive. The editor then exited before the process did, so + // this reclaim runs inside that gap and must not let the resume + // re-arm modes that no later cleanup would ever disable again. + crate::tui::publish_editor_terminal_handoff(); + crate::tui::record_termination_started(); + + let resumed = Rc::new(RefCell::new(false)); + let mut command = std::process::Command::new("sh"); + command.args(["-c", "exit 0"]); + let resumed_after = Rc::clone(&resumed); + let status = run_interactive_editor_with( + &mut command, + || {}, + move || { + *resumed_after.borrow_mut() = true; + }, + ) + .expect("shell should launch"); + + assert!(status.success()); + // The reclaim refused while terminating, so ownership stays published + // and the resume that re-arms every mode never runs. + assert!( + crate::tui::editor_owns_terminal(), + "reclaim cleared ownership while termination had already started" + ); + assert!( + !*resumed.borrow(), + "resume re-armed terminal modes after termination started" + ); + // A late keyboard push is refused too, so no entry is left unpopped + // for the shell to inherit. + assert!(!crate::tui::enable_keyboard_enhancement()); + + // Restore the process-global flags for the other tests in this binary. + crate::tui::set_editor_terminal_ownership(false); + crate::tui::reset_termination_started_for_test(); } } diff --git a/crates/jcode-tui/src/tui/mod.rs b/crates/jcode-tui/src/tui/mod.rs index e4fbf5f0d9..b4ce61d9c6 100644 --- a/crates/jcode-tui/src/tui/mod.rs +++ b/crates/jcode-tui/src/tui/mod.rs @@ -84,6 +84,8 @@ pub use app::{App, CloudHandoff, CopyBadgeUiState, ProcessingStatus, RunResult}; use crate::message::ToolCall; use ratatui::prelude::Frame; use ratatui::text::Line; +use std::sync::Mutex; +use std::sync::atomic::{AtomicBool, Ordering}; use std::time::Duration; pub(crate) fn scheduled_notification_text( @@ -116,6 +118,44 @@ fn keyboard_enhancement_flags() -> crossterm::event::KeyboardEnhancementFlags { | KeyboardEnhancementFlags::REPORT_ALTERNATE_KEYS } +/// Whether this process still owns an unbalanced entry on the terminal's +/// Kitty keyboard-protocol stack. +/// +/// The stack is shared with every program on the terminal: an editor spawned +/// via `suspend_terminal_for_editor` may push its own entry after jcode popped +/// ours. A later cleanup (signal handler, guard drop, picker exit) must not pop +/// again, or it would remove the editor's entry instead of ours. +static KITTY_KEYBOARD_PUSH_ACTIVE: AtomicBool = AtomicBool::new(false); + +/// Whether an external editor currently owns the terminal. +/// +/// During editor handoff jcode has already restored cooked input, left the +/// alternate screen, and disabled its reporting modes. If jcode receives a +/// process-only termination signal while the editor remains alive, cleanup must +/// not write mode disables again because the editor may have enabled those modes +/// for itself. +static EDITOR_TERMINAL_OWNERSHIP: AtomicBool = AtomicBool::new(false); + +/// Whether a termination signal has already begun the process's final cleanup. +/// +/// Once termination started, the process is exiting and the shell behind it +/// must inherit a fully restored terminal. Re-arming any mode afterwards (an +/// editor reclaim/resume that lost the race against the signal cleanup) would +/// leave modes enabled after death because no further cleanup runs (greptile +/// #4232838288). Recorded under the kitty state lock so it cannot interleave +/// with the reclaim's ownership swap. +static TERMINATION_STARTED: AtomicBool = AtomicBool::new(false); + +/// Serializes the terminal writes and the ownership flag transitions of the +/// Kitty push/pop pair. A termination signal (tokio task, not a UNIX signal +/// handler, so taking a lock is allowed) runs its terminal cleanup on the +/// event loop; this lock makes that cleanup wait for an in-flight push or pop +/// to finish instead of observing a half-applied state and skipping the pop. +static KITTY_KEYBOARD_STATE_LOCK: Mutex<()> = Mutex::new(()); + +#[cfg(test)] +pub(crate) static TERMINAL_OWNERSHIP_TEST_LOCK: Mutex<()> = Mutex::new(()); + /// Request Kitty keyboard reporting and tmux's extended-key mode. /// /// Intentionally avoid REPORT_ALL_KEYS_AS_ESCAPE_CODES for now. When that flag is enabled, @@ -127,14 +167,43 @@ fn keyboard_enhancement_flags() -> crossterm::event::KeyboardEnhancementFlags { /// /// Returns whether the requests were written, not whether the terminal supports them. pub fn enable_keyboard_enhancement() -> bool { + let _guard = kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + if termination_started() { + crate::logging::info("Keyboard enhancement request skipped: termination already started"); + return false; + } let result = enable_keyboard_enhancement_to(&mut std::io::stdout(), inside_tmux()).is_ok(); crate::logging::info(&format!( "Keyboard enhancement request: {}", if result { "sent" } else { "FAILED" } )); + if result { + // Set while still holding the state lock so a signal cleanup cannot + // observe the push write without the ownership flag. + kitty_keyboard_push_active().store(true, Ordering::Release); + } result } +/// Adopt a Kitty keyboard entry the previous process left on the stack when +/// it exec'd into this one (restart, reload, rebuild, update). +/// +/// The exec handoff deliberately keeps the terminal in raw mode with all +/// modes armed instead of popping and re-pushing, so this process must record +/// that the inherited entry is ours to pop on exit. Otherwise every cleanup +/// gate skips the pop and the following shell is left in CSI u mode. +pub fn adopt_keyboard_enhancement_ownership() { + let _guard = kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + if termination_started() { + return; + } + kitty_keyboard_push_active().store(true, Ordering::Release); +} + fn inside_tmux() -> bool { std::env::var_os("TMUX").is_some_and(|value| !value.is_empty()) } @@ -152,8 +221,131 @@ fn enable_keyboard_enhancement_to( } /// Reset tmux extended keys and pop Kitty keyboard reporting. +/// +/// The pop is skipped when this process has no unbalanced push on the +/// terminal's stack (a suspended editor or the calling shell may own the top +/// entry), so an extra pop cannot remove someone else's keyboard settings. pub fn disable_keyboard_enhancement() { - let _ = disable_keyboard_enhancement_to(&mut std::io::stdout(), inside_tmux()); + if !disable_keyboard_enhancement_if_owned() { + crate::logging::info("Keyboard enhancement pop skipped: no active push owned"); + } +} + +/// Pop the Kitty keyboard entry this process pushed, if it is still unbalanced. +/// +/// Returns whether a pop was performed. The state lock orders this against +/// a concurrent push or signal cleanup, so the flag swap and the pop write +/// happen as one unit: termination waits for the pop instead of exiting with +/// the entry still active. +pub fn disable_keyboard_enhancement_if_owned() -> bool { + disable_keyboard_enhancement_if_owned_to(&mut std::io::stdout(), inside_tmux()) +} + +/// Writer-backed form of [`disable_keyboard_enhancement_if_owned`] so tests +/// can verify the pop bytes without writing to the developer's terminal or +/// resetting tmux extended keys. +fn disable_keyboard_enhancement_if_owned_to( + writer: &mut impl std::io::Write, + inside_tmux: bool, +) -> bool { + let _guard = kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + if !kitty_keyboard_push_active().swap(false, std::sync::atomic::Ordering::AcqRel) { + return false; + } + let _ = disable_keyboard_enhancement_to(writer, inside_tmux); + true +} + +/// Test hook: reset the ownership flag without touching the terminal. +#[cfg(test)] +fn reset_keyboard_enhancement_ownership_for_test() { + kitty_keyboard_push_active().store(false, Ordering::Release); +} + +fn kitty_keyboard_push_active() -> &'static AtomicBool { + &KITTY_KEYBOARD_PUSH_ACTIVE +} + +fn kitty_keyboard_state_lock() -> &'static Mutex<()> { + &KITTY_KEYBOARD_STATE_LOCK +} + +pub fn editor_owns_terminal() -> bool { + EDITOR_TERMINAL_OWNERSHIP.load(Ordering::Acquire) +} + +pub fn set_editor_terminal_ownership(owns_terminal: bool) { + EDITOR_TERMINAL_OWNERSHIP.store(owns_terminal, Ordering::Release); +} + +/// Publish that an external editor owns the terminal, atomically with the +/// signal cleanup's ownership check (both take the kitty state lock). +/// +/// Callers must only publish AFTER jcode's own terminal restores finished +/// (`suspend_terminal_for_editor` completed): while the restores are in flight +/// the flag must stay false so a termination signal in that window performs a +/// full cleanup instead of skipping one and leaving the shell in raw mode +/// (greptile #4230626960). +pub fn publish_editor_terminal_handoff() { + with_kitty_state_lock(|| { + set_editor_terminal_ownership(true); + }); +} + +/// Reclaim the terminal after the editor exited, atomically with the signal +/// cleanup's ownership check. Called before `resume_terminal_after_editor` +/// re-arms jcode's own modes: a signal arriving after the editor exited must +/// run the full cleanup against whatever state the resume loop has armed so +/// far, not skip it. +pub fn reclaim_editor_terminal_handoff() { + with_kitty_state_lock(|| { + // The signal cleanup released the lock but the process has not exited + // yet: this reclaim runs in that gap, and letting the subsequent resume + // re-arm modes would leave them enabled after death because no further + // cleanup runs (greptile #4232838288). Leave ownership set so the + // resume loop sees the terminating state and writes nothing. + if termination_started() { + return; + } + set_editor_terminal_ownership(false); + }); +} + +/// Run `cleanup` while holding the kitty state lock, so an in-flight editor +/// handoff publication or reclaim (which takes the same lock) is ordered +/// against it. A signal arriving mid-handoff then sees a consistent state: +/// either jcode still owns the terminal (restore everything) or the editor +/// does (skip). +pub fn with_kitty_state_lock(cleanup: impl FnOnce() -> R) -> R { + let lock = kitty_keyboard_state_lock(); + let _state = lock.lock().unwrap_or_else(|poisoned| poisoned.into_inner()); + cleanup() +} + +/// Record that termination has begun, under the kitty state lock. +/// +/// Called by the termination-signal cleanup as soon as it decides what to do +/// with the terminal. Recording under the same lock as the editor handoff's +/// publish/reclaim closes the gap between this cleanup releasing the lock and +/// the process actually exiting: a reclaim racing into that gap observes the +/// flag and leaves the terminal modes alone, so the shell receives normal +/// input after jcode dies instead of escape sequences (greptile #4232838288). +pub fn record_termination_started() { + with_kitty_state_lock(|| { + TERMINATION_STARTED.store(true, Ordering::Release); + }); +} + +fn termination_started() -> bool { + TERMINATION_STARTED.load(Ordering::Acquire) +} + +/// Test hook: clear the shared termination flag without touching the terminal. +#[cfg(test)] +fn reset_termination_started_for_test() { + TERMINATION_STARTED.store(false, Ordering::Release); } fn disable_keyboard_enhancement_to( @@ -247,10 +439,118 @@ fn reapply_terminal_modes_after_focus_to( mod terminal_mode_tests { use super::{ disable_keyboard_enhancement_to, enable_keyboard_enhancement_to, - reapply_keyboard_enhancement_to, reapply_terminal_modes_after_focus_to, - reapply_terminal_modes_to, + kitty_keyboard_push_active, reapply_keyboard_enhancement_to, + reapply_terminal_modes_after_focus_to, reapply_terminal_modes_to, + reset_keyboard_enhancement_ownership_for_test, }; + #[test] + fn disable_skips_kitty_pop_when_no_push_is_owned() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228191597: suspend_terminal_for_editor already popped our + // entry while the editor runs, so an extra pop (signal handler firing in + // that window) must be a no-op instead of removing the editor's own + // stack entry. + assert!(!super::disable_keyboard_enhancement_if_owned()); + assert!(!kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + reset_keyboard_enhancement_ownership_for_test(); + } + + #[test] + fn disable_pops_kitty_entry_exactly_once_then_ownership_is_released() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228894633: run the pop against a buffer so the test never + // writes the pop (or the tmux extended-keys reset) to the developer's + // real terminal. + kitty_keyboard_push_active().store(true, std::sync::atomic::Ordering::Release); + let mut output = Vec::new(); + assert!(super::disable_keyboard_enhancement_if_owned_to( + &mut output, + false + )); + assert!(!output.is_empty()); + assert!(!kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + // A second cleanup (SIGTERM during the editor window, or a duplicated + // teardown) must not pop again now that the ownership is released. + let mut second = Vec::new(); + assert!(!super::disable_keyboard_enhancement_if_owned_to( + &mut second, + false + )); + assert!(second.is_empty()); + + reset_keyboard_enhancement_ownership_for_test(); + } + + #[test] + fn adopt_records_inherited_entry_without_pushing() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228894613: after an exec handoff the entry is already on + // the stack, so adopting must only record ownership — no push bytes — + // and the pop must then balance the inherited entry. + super::adopt_keyboard_enhancement_ownership(); + assert!(kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + let mut output = Vec::new(); + assert!(super::disable_keyboard_enhancement_if_owned_to( + &mut output, + false + )); + assert_eq!(output, b"\x1b[<1u"); + + reset_keyboard_enhancement_ownership_for_test(); + } + + #[test] + fn cleanup_waits_for_in_flight_kitty_push_to_finish() { + let _guard = super::TERMINAL_OWNERSHIP_TEST_LOCK + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + reset_keyboard_enhancement_ownership_for_test(); + + // Greptile #4228894626: a termination cleanup must not observe the + // half-applied state between the push write and the ownership flag, + // so the disable waits for the state lock instead of racing it. Hold + // the lock, set the flag as if a push just completed, and confirm a + // concurrent cleanup stays blocked until the lock is released. + let state_guard = super::kitty_keyboard_state_lock() + .lock() + .unwrap_or_else(|poison| poison.into_inner()); + kitty_keyboard_push_active().store(true, std::sync::atomic::Ordering::Release); + + let cleanup = std::thread::spawn(|| { + // Buffer-backed so the blocked cleanup writes no bytes to the + // developer's terminal; the flag swap is the observable effect. + let mut sink = Vec::new(); + super::disable_keyboard_enhancement_if_owned_to(&mut sink, false) + }); + std::thread::sleep(std::time::Duration::from_millis(100)); + // The cleanup is still blocked on the state lock we hold, so the flag + // must be untouched and the thread must not have returned yet. + assert!(!cleanup.is_finished()); + assert!(kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + drop(state_guard); + assert!(cleanup.join().expect("cleanup thread")); + assert!(!kitty_keyboard_push_active().load(std::sync::atomic::Ordering::Acquire)); + + reset_keyboard_enhancement_ownership_for_test(); + } + #[test] fn tmux_keyboard_lifecycle_requests_and_resets_extended_keys() { let mut output = Vec::new(); diff --git a/scripts/code_size_budget.json b/scripts/code_size_budget.json index 07decdcd84..0219b80189 100644 --- a/scripts/code_size_budget.json +++ b/scripts/code_size_budget.json @@ -81,7 +81,7 @@ "crates/jcode-tui/src/tui/app/auth.rs": 3520, "crates/jcode-tui/src/tui/app/auth_account_commands.rs": 1217, "crates/jcode-tui/src/tui/app/auth_account_picker.rs": 1275, - "crates/jcode-tui/src/tui/app/commands.rs": 3796, + "crates/jcode-tui/src/tui/app/commands.rs": 3980, "crates/jcode-tui/src/tui/app/debug_bench.rs": 1286, "crates/jcode-tui/src/tui/app/helpers.rs": 1751, "crates/jcode-tui/src/tui/app/hotkey_feedback.rs": 1222, @@ -101,7 +101,7 @@ "crates/jcode-tui/src/tui/app/turn.rs": 1738, "crates/jcode-tui/src/tui/backend.rs": 2004, "crates/jcode-tui/src/tui/info_widget.rs": 2267, - "crates/jcode-tui/src/tui/mod.rs": 2196, + "crates/jcode-tui/src/tui/mod.rs": 2496, "crates/jcode-tui/src/tui/session_picker.rs": 2493, "crates/jcode-tui/src/tui/session_picker/loading.rs": 3084, "crates/jcode-tui/src/tui/ui.rs": 3647, diff --git a/src/cli/terminal.rs b/src/cli/terminal.rs index f0dcc70c35..3e8b7b1a59 100644 --- a/src/cli/terminal.rs +++ b/src/cli/terminal.rs @@ -400,9 +400,15 @@ pub fn init_tui_runtime() -> Result<(ratatui::DefaultTerminal, TuiRuntimeGuard)> // The previous process intentionally preserved these modes across exec. // Reassert idempotent modes because terminals, multiplexers, or an older // process may have cleared them during the handoff. Do not push Kitty's - // stack-based keyboard enhancement flags again. A later normal exit must - // still disable every inherited mode, so retain them in the guard. + // stack-based keyboard enhancement flags again: the exec handoff left the + // previous process's entry on the stack, so record that this process + // now owns it and a later normal exit must pop it. Without adopting, + // every cleanup gate would skip the pop and leave the shell in CSI u + // mode after the restarted process exits. let modes = inherited_modes.unwrap_or(fallback_modes); + if modes.keyboard_enhanced { + tui::adopt_keyboard_enhancement_ownership(); + } crossterm::execute!(std::io::stdout(), crossterm::event::EnableBracketedPaste)?; if modes.focus_change { crossterm::execute!(std::io::stdout(), crossterm::event::EnableFocusChange)?; @@ -613,12 +619,27 @@ fn signal_crash_reason(sig: i32) -> String { fn handle_termination_signal(sig: i32) -> ! { mark_current_session_crashed(signal_crash_reason(sig)); - let _ = crossterm::terminal::disable_raw_mode(); - let _ = crossterm::execute!( - std::io::stderr(), - crossterm::terminal::LeaveAlternateScreen, - crossterm::cursor::Show - ); + // Record termination before the terminal cleanup runs. The cleanup may + // skip its restore while an editor owns the terminal and then release the + // kitty state lock while this process is still alive: an editor reclaim + // racing into that gap must observe the flag and refuse to re-arm modes, + // or the shell inherits escape sequences once jcode exits (greptile + // #4232838288). Recording first also keeps the normal path intact: a + // reclaim that slips in before the cleanup still leaves ownership clear, + // so the cleanup runs its full restore. + crate::tui::record_termination_started(); + + if signal_cleanup_terminal_modes_to(signal_cleanup_stream()) { + // The signal handler can fire at any point of the TUI lifetime, including + // while the kitty keyboard protocol is active (Ctrl+C races the cleanup + // below). Pop only when this process still owns an unbalanced kitty entry: + // the pop is a no-op on terminals with no kitty stack, and skipping it when + // a suspended editor already popped ours (or pushed its own) avoids + // removing someone else's keyboard settings. Leaving jcode's own entry + // unpopped would leave the terminal in CSI u mode so the following shell + // echoes raw key sequences like `e1;1:3u` after jcode exits (issue #898). + crate::tui::disable_keyboard_enhancement(); + } if let Some(session_id) = get_current_session() { print_session_resume_hint(&session_id); @@ -627,6 +648,115 @@ fn handle_termination_signal(sig: i32) -> ! { std::process::exit(128 + sig); } +fn signal_cleanup_terminal_modes_to(mut writer: impl Write) -> bool { + // The ownership check and the restore writes run under the kitty state + // lock so an in-flight editor handoff cannot split them: the handoff + // publishes ownership under the same lock after its restores finished + // (greptile #4230626960). Without the lock, a signal could observe + // ownership set while jcode's modes were still armed, skip the cleanup, + // and exit leaving the shell in raw mode. + crate::tui::with_kitty_state_lock(|| { + if crate::tui::editor_owns_terminal() { + return false; + } + + let _ = crossterm::terminal::disable_raw_mode(); + let _ = crossterm::execute!( + writer, + crossterm::event::DisableBracketedPaste, + crossterm::event::DisableFocusChange, + crossterm::event::DisableMouseCapture, + crossterm::terminal::LeaveAlternateScreen, + crossterm::cursor::Show + ); + true + }) +} + +/// Stream the signal cleanup must target: stdout, the interactive terminal +/// the TUI requires, where `setup_terminal` enabled every mode this cleanup +/// disables. Writing to stderr would miss the terminal whenever stderr is +/// redirected and leave the modes armed after jcode exits. +#[cfg(unix)] +fn signal_cleanup_stream() -> std::io::Stdout { + std::io::stdout() +} + +#[cfg(unix)] +#[cfg(test)] +mod signal_cleanup_tests { + use super::*; + use std::sync::Mutex; + + /// Serializes tests that flip the shared editor ownership flag so one + /// test's clear cannot interleave with another's check. + static SIGNAL_CLEANUP_TEST_LOCK: Mutex<()> = Mutex::new(()); + + struct EditorOwnershipGuard; + + impl EditorOwnershipGuard { + fn new(owns_terminal: bool) -> Self { + crate::tui::set_editor_terminal_ownership(owns_terminal); + Self + } + } + + impl Drop for EditorOwnershipGuard { + fn drop(&mut self) { + crate::tui::set_editor_terminal_ownership(false); + } + } + + #[test] + fn signal_cleanup_skips_terminal_modes_while_editor_owns_terminal() { + let _test_lock = SIGNAL_CLEANUP_TEST_LOCK.lock().unwrap(); + let _ownership = EditorOwnershipGuard::new(true); + let mut output = Vec::new(); + + assert!(!signal_cleanup_terminal_modes_to(&mut output)); + assert!(output.is_empty()); + } + + #[test] + fn signal_cleanup_waits_for_in_flight_editor_handoff() { + // Greptile #4230626960: the cleanup must not observe a half-applied + // handoff (flag published while jcode's modes are still armed). The + // publication and reclaim hold the kitty state lock, so the cleanup + // waits for them instead of racing the flag. + let _test_lock = SIGNAL_CLEANUP_TEST_LOCK.lock().unwrap(); + let _ownership = EditorOwnershipGuard::new(false); + + let cleanup = crate::tui::with_kitty_state_lock(|| { + let cleanup = std::thread::spawn(|| { + let mut sink = Vec::new(); + let ran = signal_cleanup_terminal_modes_to(&mut sink); + (ran, sink.is_empty()) + }); + std::thread::sleep(std::time::Duration::from_millis(100)); + assert!( + !cleanup.is_finished(), + "cleanup ran without waiting for the handoff lock" + ); + // Mark the editor as owning the terminal so the released cleanup + // takes the early-return path and writes nothing to the terminal. + crate::tui::set_editor_terminal_ownership(true); + cleanup + }); + + let (ran, sink_empty) = cleanup.join().expect("cleanup thread"); + assert!(!ran); + assert!(sink_empty); + } + + #[test] + fn signal_cleanup_targets_stdout_not_stderr() { + // Same stream object as setup_terminal enables modes on. A write via + // this handle lands on the interactive terminal even when stderr is + // redirected to a file or pipe. + let _stream = signal_cleanup_stream(); + } +} + #[cfg(unix)] pub fn spawn_session_signal_watchers() { use tokio::signal::unix::{SignalKind, signal};