fix(ssh): save a tab's connection whole, not just its address (#438)

The tab menu's Save-as-SSH-Host row went through `edit_ssh_host_of_target`,
which for a session dialled by hand knows only `RemoteTarget::Direct { user,
host, port }`. Everything else the live connection carries — its proxy, its
jump host, its identity files, its forwards, its login script and algorithms
— was dropped on the way to the form, so the host that got saved was one
that could not reach the machine it was saved from. An alias out of
`~/.ssh/config` is the everyday case: `quick_connect` resolves its
`ProxyJump` into the spec, and the row offered it back without the hop and
without a word.

The command palette's own Save-Connection-as-Host has read the live spec
since it was written, and pushes `SshSaveDroppedJumpHost` out loud for the
one field it cannot carry — because, as its comment says, silently dropping
the hop would leave a host that saves fine and then cannot be reached. The
tab row carried nothing and said nothing.

`tab_ssh_host_form` now answers with what the row opens rather than with a
target: the saved host by its id when there is one, otherwise the pane's own
spec, handed to the same `save_ssh_spec_as_host` the command now also goes
through. `ssh_host_target_of_spec` stays the gate and the label, so the tab
menu and the switcher's machine menu still cannot drift on which machines
are editable or on what the row is called.

The two ported tests now assert the session that reaches the form rather
than an address, and a third pins the fields that were being lost.

docs/remote/ssh.mdx

@@ -71,7 +71,9 @@ Right-clicking an SSH tab opens that connection's host form — **Edit Host…**
 for a saved one, **Save as SSH Host…** for an address typed by hand. It is the
 same row the workspace switcher's machine menu carries, so a hostname or
 password typed wrong is corrected from the tab you noticed it on. The menu acts
-on the tab it was opened on, not on whichever pane is focused.
+on the tab it was opened on, not on whichever pane is focused. Saving one opens
+on the whole live connection — its proxy, keys and forwards as well as its
+address — so the host that lands is the one you were already on.
 
 Passwords and key passphrases go in the **OS keychain**, never in
 `config.json` and never on disk in plain text. **Forget Password** in a

src/terminal/view.rs

@@ -8892,14 +8892,25 @@ pub(crate) fn quiet_test_ssh_pane_of(
     profile_id: Option<uuid::Uuid>,
     window: &mut Window,
     cx: &mut gpui::App,
+) -> (gpui::Entity<TerminalView>, crate::daemon::transport::Stream) {
+    let mut spec: crate::daemon::protocol::NativeSshSpec =
+        serde_json::from_str(r#"{"host":"build-box","port":22,"user":"me","auth_mode":"auto"}"#)
+            .expect("a minimal NativeSshSpec decodes");
+    spec.profile_id = profile_id.map(|id| id.to_string());
+    quiet_test_ssh_pane_with(pane_id, spec, window, cx)
+}
+
+/// The same again, over a spec the caller shaped — for everything a live
+/// connection carries beyond its address.
+#[cfg(test)]
+pub(crate) fn quiet_test_ssh_pane_with(
+    pane_id: u64,
+    spec: crate::daemon::protocol::NativeSshSpec,
+    window: &mut Window,
+    cx: &mut gpui::App,
 ) -> (gpui::Entity<TerminalView>, crate::daemon::transport::Stream) {
     let (view, stream) = quiet_test_pane(pane_id, window, cx);
     view.update(cx, |view, _| {
-        let mut spec: crate::daemon::protocol::NativeSshSpec = serde_json::from_str(
-            r#"{"host":"build-box","port":22,"user":"me","auth_mode":"auto"}"#,
-        )
-        .expect("a minimal NativeSshSpec decodes");
-        spec.profile_id = profile_id.map(|id| id.to_string());
         view.ssh_spec = Some(Box::new(spec));
     });
     (view, stream)

src/ui/ssh_connect.rs

@@ -186,7 +186,18 @@ impl Tty7App {
         let Some(spec) = self.unsaved_ssh_session(window, cx) else {
             return;
         };
-        let profile = profile_from_live_spec(&spec);
+        self.save_ssh_spec_as_host(&spec, window, cx);
+    }
+
+    /// The same form, for a connection named by the caller rather than by the
+    /// focus — the tab menu's row offers it for the tab it was opened on.
+    pub(crate) fn save_ssh_spec_as_host(
+        &mut self,
+        spec: &NativeSshSpec,
+        window: &mut gpui::Window,
+        cx: &mut gpui::Context<Self>,
+    ) {
+        let profile = profile_from_live_spec(spec);
         let jumped = spec.jump.is_some();
         self.open_settings_section(crate::ui::settings::SettingsSection::Ssh, window, cx);
         self.ssh_form_load(&profile, window, cx);
@@ -261,12 +272,33 @@ impl Tty7App {
         index: usize,
         window: &gpui::Window,
         cx: &gpui::App,
-    ) -> Option<(crate::core::session::RemoteTarget, &'static str)> {
+    ) -> Option<(TabHostForm, &'static str)> {
         let leaf = self.tabs.get(index)?.pane.focused_or_first(window, cx)?;
         let spec = leaf.read(cx).ssh_spec()?;
         let target = ssh_host_target_of_spec(&spec, &cx.global::<Config>().ssh_profiles);
         let label = crate::ui::switcher::host_form_label(&target)?;
-        Some((target, label))
+        let form = match target {
+            crate::core::session::RemoteTarget::Profile { .. } => TabHostForm::Saved(target),
+            _ => TabHostForm::Unsaved(spec),
+        };
+        Some((form, label))
+    }
+
+    /// Open what the row offered. A saved host goes to its own record; an
+    /// unsaved one goes through the same "save this connection" path the
+    /// command already uses, so the proxy, the identity files and the forwards
+    /// the session was dialled with land in the draft rather than being
+    /// thrown away with everything that does not fit in `user@host:port`.
+    pub(crate) fn open_tab_ssh_host_form(
+        &mut self,
+        form: &TabHostForm,
+        window: &mut gpui::Window,
+        cx: &mut gpui::Context<Self>,
+    ) {
+        match form {
+            TabHostForm::Saved(target) => self.edit_ssh_host_of_target(target, window, cx),
+            TabHostForm::Unsaved(spec) => self.save_ssh_spec_as_host(spec, window, cx),
+        }
     }
 
     fn bump_ssh_frecency(&mut self, profile_id: uuid::Uuid, cx: &mut gpui::Context<Self>) {
@@ -434,13 +466,33 @@ fn build_spec_inner(
     }
 }
 
+/// What a tab's host row opens when it is taken.
+///
+/// The two halves are not the same form. A saved host is already a record, so
+/// it is addressed by the target that names it and nothing about the live
+/// session is needed. An unsaved one is only ever the session, and it goes to
+/// the form whole: an address dialled by hand carries a proxy, a jump host,
+/// identity files and forwards, and a draft built from `user@host:port` alone
+/// would save fine and then not connect.
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub(crate) enum TabHostForm {
+    Saved(crate::core::session::RemoteTarget),
+    Unsaved(Box<NativeSshSpec>),
+}
+
 /// Which host form a live connection belongs to: the saved host it was opened
 /// from, or the address it was dialled by.
 ///
 /// A transient profile is handed a fresh uuid on its way to the daemon, so an
 /// id alone does not mean a host was saved — only one that still resolves
-/// against the saved list does. Anything else is an address worth keeping, and
-/// [`Tty7App::edit_ssh_host_of_target`] opens a new host prefilled from it.
+/// against the saved list does. Anything else is an address worth keeping.
+///
+/// The `Direct` this hands back is the gate and the label, not the draft:
+/// [`host_form_label`] reads it to decide the row exists and what it says,
+/// while the form itself opens on the whole live spec, which carries far more
+/// than an address does.
+///
+/// [`host_form_label`]: crate::ui::switcher::host_form_label
 pub(crate) fn ssh_host_target_of_spec(
     spec: &NativeSshSpec,
     profiles: &[SshProfile],

src/ui/tab_strip.rs

@@ -1433,13 +1433,13 @@ impl Tty7App {
         // is the switcher machine menu's, word for word: the saved host when
         // there is one, an offer to keep the address when it was dialled by
         // hand, and nothing at all for a tab with no host form behind it.
-        if let Some((target, label)) = this.tab_ssh_host_form(index, window, cx) {
+        if let Some((form, label)) = this.tab_ssh_host_form(index, window, cx) {
             menu = menu.separator().item(PopupMenuItem::new(label).on_click({
                 let app = app.clone();
                 move |_, window, cx| {
-                    let target = target.clone();
+                    let form = form.clone();
                     let _ = app.update(cx, |this, cx| {
-                        this.edit_ssh_host_of_target(&target, window, cx)
+                        this.open_tab_ssh_host_form(&form, window, cx)
                     });
                 }
             }));
@@ -1939,10 +1939,14 @@ mod ssh_host_row_tests {
     use crate::core::config::Config;
     use crate::core::session::RemoteTarget;
     use crate::core::ssh_profile::SshProfile;
-    use crate::terminal::view::{quiet_test_pane, quiet_test_ssh_pane, quiet_test_ssh_pane_of};
+    use crate::daemon::protocol::SshProxy;
+    use crate::terminal::view::{
+        quiet_test_pane, quiet_test_ssh_pane, quiet_test_ssh_pane_of, quiet_test_ssh_pane_with,
+    };
     use crate::ui::app::{Tab, test_window::harness};
     use crate::ui::i18n::{L10nKey, set_locale, t};
     use crate::ui::pane::{Pane, PaneSlot};
+    use crate::ui::ssh_connect::TabHostForm;
     use gpui::TestAppContext;
 
     #[gpui::test]
@@ -1984,19 +1988,29 @@ mod ssh_host_row_tests {
             );
 
             // An address typed by hand is worth keeping, not editing: there is
-            // no saved host behind it yet.
-            let (target, label) = app
+            // no saved host behind it yet, so the live session itself is what
+            // the form opens on.
+            let (form, label) = app
                 .tab_ssh_host_form(1, window, cx)
                 .expect("a tab dialled by hand offers to save the host");
-            assert_eq!(target, RemoteTarget::direct("me", "build-box", 22));
+            let TabHostForm::Unsaved(spec) = form else {
+                panic!("a hand-dialled tab must offer its own session, not a bare address");
+            };
+            assert_eq!(
+                (spec.user.as_str(), spec.host.as_str(), spec.port),
+                ("me", "build-box", 22)
+            );
             assert_eq!(label, t(L10nKey::SwitcherSaveAsHost));
 
             // One opened from a saved host edits that host — by its id, so the
             // form lands on the record the connection actually came from.
-            let (target, label) = app
+            let (form, label) = app
                 .tab_ssh_host_form(2, window, cx)
                 .expect("a tab on a saved host offers to edit it");
-            assert_eq!(target, RemoteTarget::Profile { id: saved });
+            assert_eq!(
+                form,
+                TabHostForm::Saved(RemoteTarget::Profile { id: saved })
+            );
             assert_eq!(label, t(L10nKey::SwitcherEditHost));
         });
     }
@@ -2018,13 +2032,67 @@ mod ssh_host_row_tests {
         vcx.background_executor.run_until_parked();
 
         app.update_in(&mut vcx, |app, window, cx| {
-            let (target, label) = app
+            let (form, label) = app
                 .tab_ssh_host_form(0, window, cx)
                 .expect("an unresolvable profile id still names a host worth keeping");
-            assert_eq!(target, RemoteTarget::direct("me", "build-box", 22));
+            let TabHostForm::Unsaved(spec) = form else {
+                panic!("a dangling profile id must not open a form on a host that is gone");
+            };
+            assert_eq!(
+                (spec.user.as_str(), spec.host.as_str(), spec.port),
+                ("me", "build-box", 22)
+            );
             assert_eq!(label, t(L10nKey::SwitcherSaveAsHost));
         });
     }
+
+    /// The row says "Save as SSH Host", and a host saved without the proxy it
+    /// was reached through is a host that will not connect. What the session
+    /// was dialled with has to reach the form whole — an address is only the
+    /// part of it that fits in `user@host:port`.
+    #[gpui::test]
+    fn saving_a_hand_dialled_tab_keeps_what_it_was_dialled_with(cx: &mut TestAppContext) {
+        set_locale("en");
+        let (app, mut vcx) = harness(cx);
+        let _end = app.update_in(&mut vcx, |app, window, cx| {
+            let mut spec: crate::daemon::protocol::NativeSshSpec = serde_json::from_str(
+                r#"{"host":"build-box","port":2222,"user":"me","auth_mode":"auto"}"#,
+            )
+            .expect("a minimal NativeSshSpec decodes");
+            spec.proxy = SshProxy::Socks {
+                host: "127.0.0.1".to_string(),
+                port: 1080,
+            };
+            spec.identity_files = vec!["/keys/id_ed25519".to_string()];
+            spec.login_script = vec!["tmux attach".to_string()];
+            let (view, end) = quiet_test_ssh_pane_with(1, spec, window, cx);
+            app.tabs.push(Tab::new(Pane::leaf(PaneSlot::Ready(view))));
+            app.active = 0;
+            cx.notify();
+            end
+        });
+        vcx.background_executor.run_until_parked();
+
+        app.update_in(&mut vcx, |app, window, cx| {
+            let (form, _) = app
+                .tab_ssh_host_form(0, window, cx)
+                .expect("a hand-dialled tab offers to save the host");
+            let TabHostForm::Unsaved(spec) = form else {
+                panic!("nothing here is saved, so nothing here is an edit");
+            };
+            assert_eq!(
+                spec.proxy,
+                SshProxy::Socks {
+                    host: "127.0.0.1".to_string(),
+                    port: 1080,
+                },
+                "the proxy the session was reached through was dropped on the way to the form"
+            );
+            assert_eq!(spec.identity_files, vec!["/keys/id_ed25519".to_string()]);
+            assert_eq!(spec.login_script, vec!["tmux attach".to_string()]);
+            assert_eq!(spec.port, 2222, "a non-default port is part of the address");
+        });
+    }
 }
 
 #[cfg(test)]