fix: validate API key before terminal init to prevent garbled output#1630
fix: validate API key before terminal init to prevent garbled output#1630duanchao-lab wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a TerminalCleanupGuard RAII struct to ensure terminal modes like raw mode and alternate screen are properly restored if the TUI returns early. It also adds a pre-validation step for the DeepSeek API key before terminal initialization. A review comment identifies a risk where the cleanup guard is defused before manual restoration steps are complete, which could leave the terminal in an inconsistent state if those steps fail.
| // Defuse the RAII guard — we're doing the cleanup manually here so we | ||
| // can use the terminal backend handle (better than raw stdout). | ||
| _cleanup_guard.defused = true; |
There was a problem hiding this comment.
The RAII guard is defused before the manual terminal restoration begins. If any of the subsequent cleanup steps (lines 528-539) fail and return early via the ? operator, the terminal will be left in an inconsistent state (e.g., raw mode or alternate screen still active) because the guard will no longer execute its fallback cleanup. It is safer to defuse the guard only after all fallible restoration steps have successfully completed. Since the restoration operations are generally idempotent, you could also consider removing the manual defusal entirely to ensure the guard always runs as a final safety net.
When the API key was missing, run_tui would initialize the terminal (alternate screen, raw mode, Kitty keyboard protocol, mouse capture) and then bail at DeepSeekClient::new(config)?, skipping the cleanup code. This left escape sequences in stdout, rendering as garbled text. Two changes: - Pre-validate the API key in run_interactive() before calling run_tui(), so the error message prints cleanly without entering terminal mode. - Add a TerminalCleanupGuard (RAII) inside run_tui() as a safety net: if any other early ?-return occurs after terminal init, the guard restores raw mode, alternate screen, mouse capture, and keyboard flags on drop.
Harvests the cleanup-guard portion of PR #1630 by @duanchao-lab while preserving provider-aware onboarding startup.
|
Thanks for sending this. The safe part of the idea - restoring terminal state when startup exits early - has now landed on main through #1647, with credit in the commit/changelog. I did not merge this PR as-is because the unconditional API-key prevalidation would bypass some provider-aware onboarding paths, but the cleanup guard itself is included for v0.8.38. |
When the API key was missing, run_tui would initialize the terminal (alternate screen, raw mode, Kitty keyboard protocol, mouse capture) and then bail at DeepSeekClient::new(config)?, skipping the cleanup code. This left escape sequences in stdout, rendering as garbled text.
Two changes:
Summary
Testing
cargo test --all-featurescargo fmt --all -- --checkcargo clippy --all-targets --all-featuresChecklist