Merge pull request #895 from doitian/fix/894-codex-newlines-conpty

fix(terminal): preserve Codex newline chords through ConPTY

src/terminal/input.rs

@@ -6,7 +6,7 @@ use super::view::TerminalView;
 use crate::core::config::Config;
 
 /// Everything about the terminal's current state that changes how a keystroke
-/// is encoded: the kitty protocol flags, plus DECCKM (application cursor keys).
+/// is encoded: keyboard modes and the PTY that receives the bytes.
 #[derive(Clone, Copy, Default)]
 pub(super) struct KeyFlags {
     disambiguate: bool,
@@ -15,6 +15,7 @@ pub(super) struct KeyFlags {
     /// DECCKM. ncurses apps turn this on via `smkx` and then only recognise the
     /// SS3 form of the arrow keys, because that is what `kcuu1` & co. spell.
     app_cursor: bool,
+    local_conpty: bool,
 }
 
 impl KeyFlags {
@@ -24,6 +25,14 @@ impl KeyFlags {
             report_all_keys: mode.contains(TermMode::REPORT_ALL_KEYS_AS_ESC),
             report_text: mode.contains(TermMode::REPORT_ASSOCIATED_TEXT),
             app_cursor: mode.contains(TermMode::APP_CURSOR),
+            local_conpty: false,
+        }
+    }
+
+    pub(super) fn from_mode_with_local_conpty(mode: &TermMode, local_conpty: bool) -> Self {
+        Self {
+            local_conpty,
+            ..Self::from_mode(mode)
         }
     }
 
@@ -31,6 +40,17 @@ impl KeyFlags {
         self.disambiguate || self.report_all_keys
     }
 
+    pub(super) fn legacy_newline_bytes(self) -> &'static [u8] {
+        if self.local_conpty {
+            // ConPTY decodes bare LF as Ctrl+Enter. Explicit Ctrl+J events
+            // preserve the key for native readers and still produce LF for
+            // VT readers such as ssh and WSL.
+            b"\x1b[74;36;10;1;8;1_\x1b[74;36;10;0;8;1_"
+        } else {
+            b"\n"
+        }
+    }
+
     pub(super) fn app_cursor(self) -> bool {
         self.app_cursor
     }
@@ -374,6 +394,9 @@ fn legacy_keystroke_to_bytes(ks: &gpui::Keystroke, flags: KeyFlags) -> Option<Ve
 
     if m.control && !m.platform {
         if let Some(b) = ctrl_c0(key) {
+            if b == b'\n' && !m.alt && !m.shift {
+                return Some(flags.legacy_newline_bytes().to_vec());
+            }
             if m.alt {
                 return Some(vec![0x1b, b]);
             }
@@ -550,6 +573,7 @@ mod tests {
         KeyFlags, defer_to_ime, keystroke_to_bytes, meta_chord_bypasses_ime,
         reshape_option_keystroke, tab_bytes,
     };
+    use alacritty_terminal::term::TermMode;
     use gpui::{Keystroke, Modifiers};
 
     fn full_mode() -> KeyFlags {
@@ -558,6 +582,7 @@ mod tests {
             report_all_keys: true,
             report_text: true,
             app_cursor: false,
+            local_conpty: false,
         }
     }
 
@@ -567,6 +592,7 @@ mod tests {
             report_all_keys: false,
             report_text: false,
             app_cursor: false,
+            local_conpty: false,
         }
     }
 
@@ -582,6 +608,54 @@ mod tests {
         }
     }
 
+    #[test]
+    fn legacy_newline_preserves_ctrl_j_for_native_console_readers() {
+        let ctrl_j = Keystroke::parse("ctrl-j").unwrap();
+        for (local_conpty, expected) in [
+            (false, b"\n".as_slice()),
+            (true, b"\x1b[74;36;10;1;8;1_\x1b[74;36;10;0;8;1_".as_slice()),
+        ] {
+            let flags = KeyFlags::from_mode_with_local_conpty(&TermMode::empty(), local_conpty);
+            assert_eq!(flags.legacy_newline_bytes(), expected);
+            assert_eq!(
+                keystroke_to_bytes(&ctrl_j, flags).as_deref(),
+                Some(expected)
+            );
+            for (chord, expected) in [
+                ("enter", b"\r".as_slice()),
+                ("ctrl-c", b"\x03".as_slice()),
+                ("alt-enter", b"\x1b\r".as_slice()),
+                ("ctrl-alt-j", b"\x1b\n".as_slice()),
+            ] {
+                assert_eq!(
+                    keystroke_to_bytes(&Keystroke::parse(chord).unwrap(), flags).as_deref(),
+                    Some(expected),
+                    "{chord}, local_conpty={local_conpty}"
+                );
+            }
+        }
+    }
+
+    #[test]
+    fn kitty_newline_chords_take_precedence_over_conpty_encoding() {
+        for mode in [
+            TermMode::DISAMBIGUATE_ESC_CODES,
+            TermMode::REPORT_ALL_KEYS_AS_ESC,
+        ] {
+            let flags = KeyFlags::from_mode_with_local_conpty(&mode, true);
+            for (chord, expected) in [
+                ("ctrl-j", b"\x1b[106;5u".as_slice()),
+                ("shift-enter", b"\x1b[13;2u".as_slice()),
+            ] {
+                assert_eq!(
+                    keystroke_to_bytes(&Keystroke::parse(chord).unwrap(), flags).as_deref(),
+                    Some(expected),
+                    "{chord}"
+                );
+            }
+        }
+    }
+
     #[test]
     fn plain_text_defers_to_the_ime_unless_kitty_wants_every_key() {
         let plain = Modifiers::default();
@@ -934,6 +1008,7 @@ mod tests {
             report_all_keys: true,
             report_text: true,
             app_cursor: false,
+            local_conpty: false,
         };
         for flags in [kitty(), full] {
             assert_eq!(
@@ -1128,6 +1203,7 @@ mod tests {
             report_all_keys: false,
             report_text: false,
             app_cursor: false,
+            local_conpty: false,
         }
     }
 
@@ -1209,6 +1285,7 @@ mod tests {
             report_all_keys: true,
             report_text: false,
             app_cursor: false,
+            local_conpty: false,
         };
         let none = Modifiers::default();
         assert_eq!(
@@ -1237,6 +1314,7 @@ mod tests {
             report_all_keys: true,
             report_text: false,
             app_cursor: false,
+            local_conpty: false,
         };
         assert_eq!(tab_bytes(false, full), b"\x1b[9u".to_vec());
     }
@@ -1316,6 +1394,7 @@ mod tests {
             report_all_keys: true,
             report_text: true,
             app_cursor: false,
+            local_conpty: false,
         };
         for key in ["f1", "f2", "f4", "f5", "f7", "f10", "f11", "f12"] {
             for mods in [none, shift, ctrl] {
@@ -1352,6 +1431,7 @@ mod tests {
             report_all_keys: true,
             report_text: true,
             app_cursor: false,
+            local_conpty: false,
         };
         let none = Modifiers::default();
         assert_eq!(
@@ -1367,6 +1447,7 @@ mod tests {
             report_all_keys: true,
             report_text: true,
             app_cursor: false,
+            local_conpty: false,
         };
         let none = Modifiers::default();
         assert_eq!(

src/terminal/remote.rs

@@ -110,7 +110,7 @@ struct ReaderSignals {
     /// the reader puts back the cursor a repaint parked. Decided per pane from
     /// its [`PtySource`], and shared rather than copied because the reader can
     /// learn better mid-stream — see the `RemoteContext` arm.
-    repair_cursor: Arc<AtomicBool>,
+    local_conpty: Arc<AtomicBool>,
 }
 
 /// What kind of pty is at the far end of a pane's link, which is what decides
@@ -146,20 +146,6 @@ impl PtySource {
             _ => PtySource::Raw,
         }
     }
-
-    /// Whether the cursor a repaint parked has to be put back — see
-    /// [`crate::terminal::parked_cursor`].
-    ///
-    /// Only conhost parks one. On a raw pty the application owns the cursor and
-    /// is free to end a repaint on the text it just wrote and then echo the
-    /// next keystroke straight after it, with no positioning of its own: vim
-    /// opens its command line that way, and putting the cursor back on the cell
-    /// the repaint hid it on drops the `wq!` typed next onto the row being
-    /// edited (#430, and #774 for the Windows client that reached a Linux host
-    /// and was repaired anyway).
-    fn repairs_parked_cursor(self) -> bool {
-        self == PtySource::LocalConpty
-    }
 }
 
 #[derive(Clone, Debug, PartialEq)]
@@ -613,12 +599,12 @@ pub struct RemoteTerminal {
     /// flag under the term lock before every grid mutation, so once it is set
     /// the abandoned thread can only exit, never write.
     reader_quit: Arc<AtomicBool>,
-    /// Whether this pane's pty is a ConPTY, and so whether the reader repairs
+    /// Whether this pane's pty is a ConPTY, for input encoding and repairing
     /// the cursor a repaint parks. Held here so a relink hands the same answer
     /// to the reader it starts: a pane's pty does not change kind when the link
     /// to it is rebuilt, and the route a relink carries cannot tell a
     /// native-SSH pane from a local shell.
-    repair_cursor: Arc<AtomicBool>,
+    local_conpty: Arc<AtomicBool>,
 }
 
 /// The workspace id a spawn carries, so the pane's shell gets `$TTY7_WS` and a
@@ -944,7 +930,7 @@ impl RemoteTerminal {
                 // rebuilt from `route`: the pty on the far side is the same pty
                 // it was before the link dropped, and only this value still
                 // remembers what a `RemoteContext` taught the old reader.
-                repair_cursor: self.repair_cursor.clone(),
+                local_conpty: self.local_conpty.clone(),
             },
         );
         self.reader_thread = Some(reader);
@@ -1038,7 +1024,7 @@ impl RemoteTerminal {
         let clipboard_write_busy = Arc::new(AtomicBool::new(false));
 
         let reader_quit = Arc::new(AtomicBool::new(false));
-        let repair_cursor = Arc::new(AtomicBool::new(pty.repairs_parked_cursor()));
+        let local_conpty = Arc::new(AtomicBool::new(pty == PtySource::LocalConpty));
         let reader_thread = Self::spawn_reader(
             term.clone(),
             proxy.clone(),
@@ -1062,7 +1048,7 @@ impl RemoteTerminal {
                 images: images.clone(),
                 clipboard_writes: clipboard_writes.clone(),
                 clipboard_write_busy: clipboard_write_busy.clone(),
-                repair_cursor: repair_cursor.clone(),
+                local_conpty: local_conpty.clone(),
             },
         );
 
@@ -1102,7 +1088,7 @@ impl RemoteTerminal {
             proxy,
             reader_thread: Some(reader_thread),
             reader_quit,
-            repair_cursor,
+            local_conpty,
         })
     }
 
@@ -1173,7 +1159,7 @@ impl RemoteTerminal {
                     images,
                     clipboard_writes,
                     clipboard_write_busy,
-                    repair_cursor,
+                    local_conpty,
                 } = signals;
                 let mut awaiting_replay = awaiting_replay;
                 crate::core::threads::promote_to_user_interactive();
@@ -1240,7 +1226,7 @@ impl RemoteTerminal {
                                 // emulator to the cut, act on the state that
                                 // sequence left behind, carry on.
                                 let mut cuts: Vec<(usize, CursorCut)> = Vec::new();
-                                if repair_cursor.load(Ordering::Relaxed) {
+                                if local_conpty.load(Ordering::Relaxed) {
                                     cursor_scan.feed(&out_batch, |off, c| cuts.push((off, c)));
                                 }
                                 {
@@ -1561,7 +1547,7 @@ impl RemoteTerminal {
                                     .as_ref()
                                     .is_some_and(|c| c.kind == RemoteKind::NativeSsh)
                                 {
-                                    repair_cursor.store(false, Ordering::Relaxed);
+                                    local_conpty.store(false, Ordering::Relaxed);
                                 }
                                 if let Ok(mut guard) = remote.lock() {
                                     *guard = ctx;
@@ -1692,6 +1678,10 @@ impl RemoteTerminal {
         self.child_exited.load(Ordering::SeqCst)
     }
 
+    pub(super) fn is_local_conpty(&self) -> bool {
+        self.local_conpty.load(Ordering::Relaxed)
+    }
+
     /// Queues a keystroke — or a paste, or a mouse report — for the link.
     ///
     /// Callers are gpui event handlers on the UI thread, so this returns
@@ -4084,6 +4074,7 @@ mod parked_cursor_tests {
         let (client_side, daemon_side) = socket_pair();
         let term = RemoteTerminal::from_stream_with(client_side, size, Vec::new(), pty)
             .expect("a terminal over a socket pair");
+        assert_eq!(term.is_local_conpty(), pty == PtySource::LocalConpty);
         (term, daemon_side)
     }
 
@@ -4127,8 +4118,6 @@ mod parked_cursor_tests {
                 PtySource::Raw
             },
         );
-        assert!(PtySource::LocalConpty.repairs_parked_cursor());
-        assert!(!PtySource::Raw.repairs_parked_cursor());
     }
 
     #[test]
@@ -4280,6 +4269,7 @@ mod parked_cursor_tests {
             term.remote_context().is_some(),
             "the reader never applied the context"
         );
+        assert!(!term.is_local_conpty());
 
         DaemonMsg::Output(b"\x1b[6;4H".to_vec())
             .encode(&mut daemon_side)

src/terminal/view.rs

@@ -2965,7 +2965,10 @@ impl TerminalView {
     }
 
     pub(super) fn key_flags(&self) -> super::input::KeyFlags {
-        super::input::KeyFlags::from_mode(self.terminal.term.lock().mode())
+        super::input::KeyFlags::from_mode_with_local_conpty(
+            self.terminal.term.lock().mode(),
+            self.terminal.is_local_conpty(),
+        )
     }
 
     fn tab_bytes(&self, shift: bool) -> Vec<u8> {
@@ -4497,7 +4500,7 @@ impl TerminalView {
         {
             cx.propagate();
         } else {
-            self.send_shortcut_bytes(b"\n", "enter", cx);
+            self.send_shortcut_bytes(self.key_flags().legacy_newline_bytes(), "enter", cx);
         }
     }
 
@@ -9894,9 +9897,22 @@ mod gpui_tests {
     use super::*;
     use crate::daemon::protocol::{ClientMsg, DaemonMsg};
     use crate::daemon::transport::Stream;
+    use crate::terminal::remote::PtySource;
     use gpui::{Entity, TestAppContext, point};
 
     fn harness(cx: &mut TestAppContext) -> (gpui::WindowHandle<TerminalView>, Stream) {
+        let pty = if cfg!(windows) {
+            PtySource::LocalConpty
+        } else {
+            PtySource::Raw
+        };
+        harness_on(cx, pty)
+    }
+
+    fn harness_on(
+        cx: &mut TestAppContext,
+        pty: PtySource,
+    ) -> (gpui::WindowHandle<TerminalView>, Stream) {
         // Building a view reads the config. Whether that hit the real user
         // directory used to come down to which test happened to pin the
         // scratch dir first.
@@ -9908,8 +9924,13 @@ mod gpui_tests {
             cx.set_global(Config::default());
         });
         let window = cx.add_window(|window, cx| {
-            let terminal = RemoteTerminal::from_stream(client_side, TermSize::new(80, 24))
-                .expect("socketpair-backed terminal");
+            let terminal = RemoteTerminal::from_stream_with(
+                client_side,
+                TermSize::new(80, 24),
+                Vec::new(),
+                pty,
+            )
+            .expect("socketpair-backed terminal");
             TerminalView::with_terminal(terminal, 1, window, cx)
         });
         (window, daemon_side)
@@ -12516,7 +12537,7 @@ mod gpui_tests {
     #[gpui::test]
     fn shift_enter_reaches_a_foreground_tui_with_kitty_encoding(cx: &mut TestAppContext) {
         crate::core::config::pin_test_config_dir();
-        let (window, mut daemon) = harness(cx);
+        let (window, mut daemon) = harness_on(cx, PtySource::LocalConpty);
         cx.update(|cx| crate::ui::keymap::init(cx));
         DaemonMsg::Output(b"\x1b[>1u".to_vec())
             .encode(&mut daemon)
@@ -12546,12 +12567,18 @@ mod gpui_tests {
             next_input_until_timeout(&mut daemon),
             Some(b"\x1b[13;2u".to_vec())
         );
+
+        vcx.simulate_keystrokes("ctrl-j");
+        assert_eq!(
+            next_input_until_timeout(&mut daemon),
+            Some(b"\x1b[106;5u".to_vec())
+        );
     }
 
     #[gpui::test]
     fn shift_enter_reaches_a_foreground_tui_as_lf_without_kitty(cx: &mut TestAppContext) {
         crate::core::config::pin_test_config_dir();
-        let (window, mut daemon) = harness(cx);
+        let (window, mut daemon) = harness_on(cx, PtySource::Raw);
         cx.update(|cx| crate::ui::keymap::init(cx));
         window
             .update(cx, |view, window, cx| {
@@ -12565,6 +12592,32 @@ mod gpui_tests {
         vcx.simulate_keystrokes("shift-enter");
 
         assert_eq!(next_input_until_timeout(&mut daemon), Some(b"\n".to_vec()));
+
+        vcx.simulate_keystrokes("ctrl-j");
+        assert_eq!(next_input_until_timeout(&mut daemon), Some(b"\n".to_vec()));
+    }
+
+    #[gpui::test]
+    fn newline_chords_reach_conpty_as_ctrl_j_without_kitty(cx: &mut TestAppContext) {
+        let (window, mut daemon) = harness_on(cx, PtySource::LocalConpty);
+        cx.update(|cx| crate::ui::keymap::init(cx));
+        window
+            .update(cx, |view, window, cx| {
+                assert!(!view.input_active());
+                window.activate_window();
+                view.focus_handle.focus(window, cx);
+            })
+            .unwrap();
+
+        let mut vcx = gpui::VisualTestContext::from_window(window.into(), cx);
+        for chord in ["shift-enter", "ctrl-j"] {
+            vcx.simulate_keystrokes(chord);
+            assert_eq!(
+                next_input_until_timeout(&mut daemon),
+                Some(b"\x1b[74;36;10;1;8;1_\x1b[74;36;10;0;8;1_".to_vec()),
+                "{chord} must preserve Ctrl+J for native console readers"
+            );
+        }
     }
 
     #[gpui::test]