Conversation
The SIGINT/SIGTERM/SIGHUP/SIGQUIT handler restored raw mode, the primary screen and the cursor, but never popped the kitty keyboard protocol (CSI u) stack nor disabled bracketed paste, focus reporting or mouse capture. Exiting via Ctrl+C delivered as a signal (e.g. a fast double Ctrl+C racing the normal cleanup, kill -INT, or an external interrupt) left the terminal in kitty mode, so the following shell echoed raw key sequences like 'e1;1:3u' on the prompt (issue 1jehuang#898). Disable every TUI mode and pop the kitty stack in the signal handler before exiting. The pop is harmless on terminals without a kitty stack.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| let _ = crossterm::execute!( | ||
| std::io::stderr(), | ||
| crossterm::event::DisableBracketedPaste, | ||
| crossterm::event::DisableFocusChange, | ||
| crossterm::event::DisableMouseCapture, | ||
| crossterm::terminal::LeaveAlternateScreen, | ||
| crossterm::cursor::Show | ||
| ); |
There was a problem hiding this comment.
If stdout is the interactive terminal but stderr is redirected, signal exit sends the new bracketed-paste, focus, and mouse disable sequences to stderr instead of the terminal where those modes were enabled. Those modes can remain active after jcode exits. Send terminal cleanup to stdout before merging.
Artifacts
Authored PTY signal-test script
- The authored Python script starts the real TUI with stdout on a PTY and stderr on a pipe, sends SIGTERM, and counts cleanup sequences by stream.
Parent-revision signal run with redirected stderr
- The parent binary exited 143; its alternate-screen and cursor cleanup went to stderr, with no cleanup sequences on the PTY.
Candidate signal run with redirected stderr
- The candidate binary exited 143; all five terminal-mode cleanup sequences went to stderr, while the Kitty pop went to stdout.
Before-and-after terminal-stream counts
- An executed comparison command counted the sequences in both captured runs and confirmed the candidate's new cleanup reached stderr, not the PTY.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/terminal.rs
Line: 617-624
Comment:
**Signal cleanup misses terminal**
If stdout is the interactive terminal but stderr is redirected, signal exit sends the new bracketed-paste, focus, and mouse disable sequences to stderr instead of the terminal where those modes were enabled. Those modes can remain active after jcode exits. Send terminal cleanup to stdout before merging.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| reasoning_effort: None, | ||
| context_window: options.context_window, | ||
| input: Vec::new(), | ||
| ..Default::default() |
There was a problem hiding this comment.
Every current model field is already specified, so the default spread changes nothing today. If a field is added later, provider profile creation will silently use its default instead of requiring a decision here. Removing the spread preserves that compile-time check; this is a non-blocking maintenance concern.
Artifacts
Source-derived Rust compilation check
- The executed script extracts the actual type fields and before-or-after constructor, then compiles projections with and without a hypothetical field, making the comparison reproducible.
Before change: future field causes a compiler error
- Running the parent-revision constructor projection with a new field produced Rust error E0063, showing the previous completeness check.
After change: future field compiles
- Running the changed constructor projection with the same new field succeeded, showing that the added default expression bypasses that check.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/cli/commands/provider_setup.rs
Line: 185
Comment:
**Keep model fields explicit**
Every current model field is already specified, so the default spread changes nothing today. If a field is added later, provider profile creation will silently use its default instead of requiring a decision here. Removing the spread preserves that compile-time check; this is a non-blocking maintenance concern.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Closes #1635. Related #898.
The signal handler (SIGINT/SIGTERM/SIGHUP/SIGQUIT) restored raw mode, the primary screen and the cursor but never popped the kitty keyboard stack nor disabled bracketed paste, focus reporting or mouse capture. A signal exit could therefore leave the terminal in CSI u mode and the shell echoed raw sequences like
e1;1:3u.The handler now disables every TUI mode and calls
disable_keyboard_enhancement()before exiting. The pop is a no-op on terminals with no kitty stack, so it is safe unconditionally. This is the signal-exit path; #898 covers the same leak on the normal-exit path inside tmux.Also one-line
..Default::default()fix in provider_setup to keep a struct literal compiling after new fields.11 insertions, no behavior change on the normal-exit path. Single commit rebased onto current master.