fix: restore terminal mouse tracking state on PTY session disconnect

[inureyes]

Summary

• After a PTY session disconnects (normal exit, Ctrl+C, network drop, or panic), remote interactive programs (vim, tmux, htop, less) may have enabled mouse tracking on the local terminal. Without cleanup, the terminal remains in tracking mode and prints raw SGR escape sequences (0;59;35M32;58;35M...) on any mouse movement.
• Extended TerminalStateGuard::restore_terminal_state() (the Drop path) and force_terminal_cleanup() (last-resort call sites) to emit the complete set of mouse-tracking-off sequences (modes 1000, 1002, 1003, 1006, 1015) plus cursor-show (?25h) and alternate-screen-exit (?1049l) on teardown.
• Simplified TerminalGuard::restore_terminal() in interactive_signal.rs (the panic-hook path) to delegate to force_terminal_cleanup() instead of maintaining its own incomplete cleanup — this centralises the logic and closes the gap on the panic path (option a from the issue).

Cleanup paths covered

Path	Before	After
TerminalStateGuard::Drop (normal session exit)	Raw mode off, bracketed-paste off	+ mouse modes off, cursor show, alt-screen exit
force_terminal_cleanup() (dispatcher, execution, pty/mod call sites)	Raw mode off only	+ mouse modes off, cursor show, alt-screen exit
TerminalGuard::restore_terminal() — panic hook	Raw mode off, cursor show, leave alt-screen	Delegates to force_terminal_cleanup() (same as above)

Panic-hook path decision

Chose option (a): TerminalGuard::restore_terminal() now calls force_terminal_cleanup(). This avoids duplicating the escape-sequence list and ensures the panic path receives any future improvements to force_terminal_cleanup() automatically.

Signal handler observation

The existing Ctrl+C and SIGTERM handlers (via ctrlc and tokio::signal) set the shutdown flag and rely on the existing call sites and TerminalStateGuard::Drop to perform cleanup. The three existing force_terminal_cleanup() call sites (dispatcher.rs:429, execution.rs:99, pty/mod.rs:161) are sufficient. No new signal handler was added.

Test plan

• cargo fmt --check passes
• cargo check --all-targets passes (clean compilation)
• cargo test --lib passes (1183 tests, 0 failed)
• cargo clippy --all-targets -- -D warnings: the 9 errors reported are all pre-existing in unrelated files (hostlist/expander.rs, server/sftp.rs, ssh/ssh_config/path.rs, ssh/tokio_client/channel_manager.rs, ui/tui/event.rs); zero errors in modified files
• Manual verification: run bssh -H user@host, start vim with :set mouse=a, quit, disconnect — confirm no garbled mouse sequences appear (must be run by maintainer in an interactive terminal)

Closes #189

[inureyes]

Implementation Review Summary

Intent

Fix mouse tracking escape sequence leak after PTY session disconnect by emitting disable sequences (1000/1002/1003/1006/1015) plus alt-screen and cursor-show on every cleanup path.

Findings Addressed

• Panic-hook re-entrant deadlock risk (HIGH) — TerminalGuard::restore_terminal() (panic hook) now delegates to force_terminal_cleanup(), which acquired TERMINAL_MUTEX via lock().unwrap(). If a panic occurs while the same thread already holds the mutex (e.g., panic inside TerminalStateGuard::new(), enter_raw_mode, exit_raw_mode, or restore_terminal_state()), std::sync::Mutex re-entrance from the same thread deadlocks on most platforms. Additionally, a poisoned mutex from a prior panic would cause unwrap() to panic again, producing a process abort and suppressing the panic message. The previous code in the panic hook called terminal::disable_raw_mode() directly without touching TERMINAL_MUTEX and was safer in this regard. Fixed in commit 932960f by switching force_terminal_cleanup() to use try_lock().ok(), which never blocks and never panics on a poisoned mutex; the underlying stdout writes and disable_raw_mode are individually safe to run without the mutex.

Remaining Items

None.

Verification

• All stated requirements implemented (mouse modes 1000/1002/1003/1006/1015 + 1049 + cursor visibility on every cleanup path)
• No placeholder/mock code remaining
• Integrated into project code flow (Drop, three explicit call sites in dispatcher/execution/pty mod, and the panic hook all reach the new sequences)
• Project conventions followed (use std::io::Write; grouped with std imports; byte literals well-commented; best-effort let _ = … discipline; CHANGELOG uses [Unreleased] / ### Fixed)
• Existing modules reused where applicable (panic hook delegates to centralized force_terminal_cleanup())
• No unintended structural changes
• cargo fmt --check clean
• cargo check --all-targets clean
• cargo test --lib passes (1183 / 0 failed)
• cargo clippy --all-targets -- -D warnings: only pre-existing errors in unrelated files (hostlist/expander.rs, server/sftp.rs, ssh/ssh_config/path.rs, ssh/tokio_client/channel_manager.rs, ui/tui/event.rs); zero in modified files

Notes on items called out in the review brief

1. Escape sequence correctness: All sequences validated. Modes 1000/1002/1003/1006/1015 correctly emit CSI ? N l (disable). ?1049l exits alternate screen. ?25h shows cursor (note h = high = enable for cursor visibility — correct). Order does not matter; well-behaved terminals ignore disables for already-off modes.
2. Best-effort discipline: Every write uses let _ = … and flush() is also let _ = …. No ? propagation. Confirmed.
3. All cleanup paths covered: TerminalStateGuard::Drop → restore_terminal_state(); the three force_terminal_cleanup() call sites (src/app/dispatcher.rs:429, src/commands/interactive/execution.rs:99, src/pty/mod.rs:161); panic hook via TerminalGuard::restore_terminal(). The TUI's separate src/ui/tui/terminal_guard.rs is independent (used for the dashboard, not PTY) and out of scope for fix: restore terminal mouse tracking state on PTY session disconnect #189.
4. Panic-hook deadlock: see "Findings Addressed" above. Fixed.
5. Integration check: traced clean Ctrl+C, network drop, and panic — all three reach the new sequences.
6. Non-PTY regression: call sites are gated by interactive/PTY mode, which requires a TTY. Disable sequences are no-ops on terminals not in tracking mode.
7. CHANGELOG quality: correct [Unreleased] placement consistent with prior entries.
8. Conventions: use std::io::Write; properly placed; byte literals commented with mode meanings.

Final Verdict

Approve — ready to merge once the orchestrator transitions the status label.

[inureyes]

PR Finalization

Tests (4 added)

File: src/pty/terminal.rs — new #[cfg(test)] mod tests block

Test	What it verifies
test_force_terminal_cleanup_idempotent	Calling force_terminal_cleanup() twice does not panic. Covers the idempotency requirement.
test_try_lock_ok_survives_poisoned_mutex	try_lock().ok() yields None (no secondary panic) when the mutex is poisoned — the exact contract force_terminal_cleanup() relies on for TERMINAL_MUTEX. Uses a local Mutex to avoid poisoning the global.
test_try_lock_ok_does_not_block_when_held	try_lock().ok() returns immediately (not a deadlock) when the lock is already held on the same thread — the re-entrant panic-hook scenario. Uses a local Mutex for the same isolation reason.
test_terminal_state_default	TerminalState::default() produces sane zero values (was_raw_mode=false, size=80×24).

Test count before: 1183. After: 1187. All pass, 0 failed.

A comment block in the test module documents the gap: TTY-dependent operations (raw mode toggle, alternate screen) and the escape-sequence output cannot be meaningfully asserted in a non-TTY cargo test environment. Manual reproduction with vim or tmux remains the only path to verify the actual mouse-tracking fix.

Docstring update

force_terminal_cleanup() in src/pty/terminal.rs — added one paragraph clarifying that "unsynchronized" means "without the lock", not "skipped". Addresses the LOW-severity finding from the security review.

CHANGELOG

[Unreleased] ### Fixed entry is well-formed: symptom, trigger conditions, all three cleanup paths, exact mode numbers, and #189 reference. No change needed.

Docs / i18n

No docs/ content covers terminal cleanup internals at this level. No update needed.

Lint / format

cargo fmt --check: clean. cargo clippy on PR-modified files (src/pty/terminal.rs, src/commands/interactive_signal.rs): no new issues.

All checks passing. Ready for merge.