Implementation Review Summary

Intent

Restore terminal cursor (and full TUI state) on every exit path — normal q, Ctrl-C / SIGTERM, and panic — so Linux/tmux/VTE users no longer need reset after all-smi exits.

Findings

LOW

• src/view/terminal_manager.rs:132-134 — The inline comment "Use std::io::stdout() directly — no locks, no shared state" is technically inaccurate. std::io::stdout() does acquire an internal Mutex inside execute!; the intent appears to be "no application-managed locks held across the boundary," but the wording could mislead a future maintainer who needs to reason about deadlock potential in a panic-hook context. Suggest rewording to: "Use std::io::stdout() directly — no application-owned locks are held across this call — so this function is safe to call from a panic hook context, from a tokio task, and from Drop."

NIT

• src/view/terminal_manager.rs:75,128,171,etc. — Ordering::SeqCst on TERMINAL_NEEDS_RESTORE is conservative but stronger than necessary. Since the flag does not synchronise any data (only gates a self-contained block of terminal IO), Ordering::Acquire/Ordering::Release (or even Relaxed) would be sufficient. SeqCst is defensible as the simpler default; not worth changing unless you care about the (negligible) extra fence cost.
• src/view/terminal_manager.rs:163-198 — The three unit tests share the global TERMINAL_NEEDS_RESTORE flag and can race under cargo's default parallel test execution within a binary. Each test does set the flag to its needed starting state, so the asserts cannot fail — but a parallel race could cause restore_terminal_is_noop_when_flag_unset to spuriously emit escape codes (the test does not assert against IO). Consider --test-threads=1 for this module or a Mutex<()> test lock. Optional.
• Commit body is hard-wrapped at ~72 cols, which conflicts with the project's documented commit conventions (paragraphs as one long line, per commit-conventions skill). Body content is otherwise excellent — every paragraph clearly explains WHY. Cosmetic only; will not block.

Verification

• All stated requirements implemented (cursor restore on q, signals, panic)
• No placeholder / mock code
• Integrated into project code flow (signal handlers in main.rs, Drop impl, panic hook installed inside TerminalManager::new())
• Project conventions followed (license header, import grouping, no unwrap() on hot paths, let _ = documented)
• Existing modules reused (crossterm, tokio::signal, std::panic::take_hook/set_hook)
• No unintended structural changes
• Tests pass (cargo test --features cli --bin all-smi -- view::terminal_manager::tests → 3/3 ok)
• cargo check --features cli --lib — clean
• cargo check --features cli --lib --tests — clean
• cargo clippy --features cli --lib --tests -- -D warnings — clean
• cargo fmt --check — clean

Cross-cutting checks confirmed

• Panic hook chain ordering: setup_panic_handlers (macOS-native shutdown, no-op on Linux) installs first in main(); install_panic_hook (terminal restore) installs second inside TerminalManager::new(). Execution order on panic: terminal restore → macOS native shutdown → default backtrace hook. Terminal is usable before backtrace prints. Correct.
• Signal-handler ordering: restore_terminal() is called before shutdown_native_metrics_manager / shutdown_hlsmi_manager and before std::process::exit(0) in both SIGINT and SIGTERM handlers. Matches issue's prescribed order.
• Flag set-after-IO ordering: TERMINAL_NEEDS_RESTORE.store(true) runs only after both enable_raw_mode() AND execute!(EnterAlternateScreen, ...) succeed. A failed init leaves the flag false, so restore_terminal() is a clean no-op — no spurious escape codes for non-TUI subcommands or for partially-initialised terminals. Confirmed.
• Idempotency under concurrency: AtomicBool::swap(false, ...) is the correct primitive — exactly one caller wins, all others early-return. No TOCTOU window since the flag-clear and cleanup IO are sequenced atomically per caller (the swap is the gate).
• Feature gating: view::terminal_manager is reachable only via src/main.rs (declared as mod view;), and the all-smi bin requires the cli feature. No extra #[cfg(feature = "cli")] gates needed inside the module.
• No regression on non-TUI subcommands: snapshot / doctor / config / record / api paths never call TerminalManager::new(), so restore_terminal() in signal handlers becomes a single atomic load + early return. record skips the new signal handlers entirely via the existing if !is_record guard.

Verdict

APPROVE — implementation is correct, well-commented, well-tested, and matches the issue's acceptance criteria. The three LOW/NIT items are non-blocking polish suggestions.

PR Finalization Complete

Summary

Comment fix — The misleading "no locks, no shared state" comment at restore_terminal() lines 132-134 has been reworded. The accurate claim is that no application-owned locks are held across the execute! call; stdout() does acquire the stdlib mutex internally and releases it before returning, which is safe from every call site (panic hook, tokio task, Drop).

Test serialization — Added a module-level static TEST_MUTEX: Mutex<()> inside the tests block. Each of the three tests acquires a _guard before touching TERMINAL_NEEDS_RESTORE, ensuring they run sequentially even when cargo parallelises unit tests within a binary. No new dev-dependency introduced.

Documentation — No CHANGELOG.md at repo root; skipped. No user-facing docs describe TUI exit behavior explicitly (the fix is invisible when working correctly); no doc updates needed.

Lint/Format — cargo fmt --check clean. cargo clippy --features cli --lib --tests -- -D warnings passes with zero warnings. All 3 targeted tests pass.

Verification

cargo fmt --check        → clean
cargo clippy ...         → Finished, 0 warnings
cargo test ... view::terminal_manager::tests
  restore_terminal_clears_flag          ... ok
  restore_terminal_is_idempotent        ... ok
  restore_terminal_is_noop_when_flag_unset ... ok
  3 passed; 0 failed

Commit: 65a3013 — ready for merge.