fix: retain double-click selections and make tests root-safe (#3854)

refs #3847

Co-authored-by: akbash-bot <300245827+akbash-bot@users.noreply.github.com>

4 files changed, 163 insertions, 92 deletions

diff --git a/src/client/shell/mouse.rs b/src/client/shell/mouse.rs
index ec231c92..24310907 100644
--- a/src/client/shell/mouse.rs
+++ b/src/client/shell/mouse.rs
@@ -1665,7 +1665,11 @@ impl ClientShellState {
             if copied && self.config.copy_on_select {
                 self.request_selection_copy(outcome, false);
                 self.selection = None;
-            } else if !copied {
+            } else if self
+                .selection
+                .as_ref()
+                .is_some_and(crate::selection::Selection::is_just_click)
+            {
                 self.selection = None;
             }
             if copied {
diff --git a/src/client/shell/tests/mouse_selection.rs b/src/client/shell/tests/mouse_selection.rs
index a8f5c532..a23ff45f 100644
--- a/src/client/shell/tests/mouse_selection.rs
+++ b/src/client/shell/tests/mouse_selection.rs
@@ -232,32 +232,39 @@ fn disabled_mouse_chrome_keeps_tab_wheel_but_removes_split_drag_hits() {
 
 #[test]
 fn client_double_click_selects_and_copies_endpoint_row_word() {
-    let mut state = ClientShellState::new(ClientShellConfig::from_config(&Config::default()));
-    state.set_snapshot(Box::new(snapshot()));
-    state.set_pane_surface(surface());
-    state.compose(106, 20).expect("composed frame");
-    let pane = state.hits.panes[0].clone();
-    let click = || {
-        RawInputEvent::Mouse(crossterm::event::MouseEvent {
-            kind: MouseEventKind::Down(MouseButton::Left),
-            column: pane.inner_rect.x + 1,
-            row: pane.inner_rect.y,
-            modifiers: KeyModifiers::empty(),
-        })
-    };
-    let release = || {
-        RawInputEvent::Mouse(crossterm::event::MouseEvent {
-            kind: MouseEventKind::Up(MouseButton::Left),
-            column: pane.inner_rect.x + 1,
-            row: pane.inner_rect.y,
-            modifiers: KeyModifiers::empty(),
-        })
-    };
+    // The row response may arrive on either side of the second mouse release.
+    for (copy_on_select, release_before_response) in
+        [(false, false), (false, true), (true, false), (true, true)]
+    {
+        let mut config = Config::default();
+        config.ui.copy_on_select = copy_on_select;
+        let mut state = ClientShellState::new(ClientShellConfig::from_config(&config));
+        state.set_snapshot(Box::new(snapshot()));
+        state.set_pane_surface(surface());
+        state.compose(106, 20).expect("composed frame");
+        let pane = state.hits.panes[0].clone();
+        let click = || {
+            RawInputEvent::Mouse(crossterm::event::MouseEvent {
+                kind: MouseEventKind::Down(MouseButton::Left),
+                column: pane.inner_rect.x + 1,
+                row: pane.inner_rect.y,
+                modifiers: KeyModifiers::empty(),
+            })
+        };
+        let release = || {
+            RawInputEvent::Mouse(crossterm::event::MouseEvent {
+                kind: MouseEventKind::Up(MouseButton::Left),
+                column: pane.inner_rect.x + 1,
+                row: pane.inner_rect.y,
+                modifiers: KeyModifiers::empty(),
+            })
+        };
 
-    state.handle_raw_events(vec![click()]);
-    state.handle_raw_events(vec![release()]);
-    let second = state.handle_raw_events(vec![click()]);
-    let ClientShellAction::Endpoint { request, .. } = second
+        state.handle_raw_events(vec![click()]);
+        state.handle_raw_events(vec![release()]);
+        assert!(state.selection.is_none(), "plain clicks must not select");
+        let second = state.handle_raw_events(vec![click()]);
+        let ClientShellAction::Endpoint { request, .. } = second
         .actions
         .iter()
         .find(|action| {
@@ -271,48 +278,79 @@ fn client_double_click_selects_and_copies_endpoint_row_word() {
     else {
         unreachable!()
     };
-    let word_request_id = request.id.clone();
-    assert!(matches!(
-        &request.method,
-        crate::api::schema::Method::PaneSelectionRead(params)
-            if params.anchor == crate::api::schema::PaneTextPoint { row: 0, col: 0 }
-                && params.cursor == crate::api::schema::PaneTextPoint { row: 0, col: 3 }
-    ));
+        let word_request_id = request.id.clone();
+        assert!(matches!(
+            &request.method,
+            crate::api::schema::Method::PaneSelectionRead(params)
+                if params.anchor == crate::api::schema::PaneTextPoint { row: 0, col: 0 }
+                    && params.cursor == crate::api::schema::PaneTextPoint { row: 0, col: 3 }
+        ));
 
-    let (repaint, actions) = state.handle_endpoint_result(
-        "boot-1",
-        &word_request_id,
-        Ok(crate::api::schema::ResponseResult::PaneSelection {
-            pane_id: "pane_1".into(),
-            text: "LIVE".into(),
-        }),
-    );
-    assert!(repaint);
-    assert!(state
-        .selection
-        .as_ref()
-        .is_some_and(crate::selection::Selection::is_finalized));
-    let [ClientShellAction::Endpoint { request, .. }] = &actions[..] else {
-        panic!("auto-copy should read the selected word");
-    };
-    let copy_request_id = request.id.clone();
-    assert!(matches!(
-        &request.method,
-        crate::api::schema::Method::PaneSelectionRead(params)
-            if params.anchor.col == 0 && params.cursor.col == 3
-    ));
-    let (_, actions) = state.handle_endpoint_result(
-        "boot-1",
-        &copy_request_id,
-        Ok(crate::api::schema::ResponseResult::PaneSelection {
-            pane_id: "pane_1".into(),
-            text: "LIVE".into(),
-        }),
-    );
-    assert!(matches!(
-        &actions[..],
-        [ClientShellAction::ClipboardWrite(bytes)] if bytes == b"LIVE"
-    ));
+        if release_before_response {
+            let released = state.handle_raw_events(vec![release()]);
+            assert!(released.actions.is_empty());
+        }
+        let (repaint, actions) = state.handle_endpoint_result(
+            "boot-1",
+            &word_request_id,
+            Ok(crate::api::schema::ResponseResult::PaneSelection {
+                pane_id: "pane_1".into(),
+                text: "LIVE".into(),
+            }),
+        );
+        assert!(repaint);
+        assert!(state
+            .selection
+            .as_ref()
+            .is_some_and(crate::selection::Selection::is_finalized));
+        let deadline = state.selection_highlight_clear_deadline;
+        if !release_before_response {
+            let released = state.handle_raw_events(vec![release()]);
+            assert!(released.actions.is_empty(), "release must not copy twice");
+        }
+        assert!(
+            state.selection.as_ref().is_some_and(crate::selection::Selection::is_finalized),
+            "mouse release must retain the finalized word selection (copy_on_select={copy_on_select})"
+        );
+        assert_eq!(
+            state.selection.as_ref().unwrap().ordered_cells(),
+            ((0, 0), (0, 3))
+        );
+        assert_eq!(state.selection_highlight_clear_deadline, deadline);
+        if !copy_on_select {
+            assert!(actions.is_empty(), "manual selection must not auto-copy");
+            assert!(state.selection_highlight_clear_deadline.is_none());
+            state.tick_copy_feedback(std::time::Instant::now() + std::time::Duration::from_secs(1));
+            assert!(
+                state.selection.is_some(),
+                "manual selection must not expire"
+            );
+            continue;
+        }
+        let [ClientShellAction::Endpoint { request, .. }] = &actions[..] else {
+            panic!("auto-copy should read the selected word");
+        };
+        let copy_request_id = request.id.clone();
+        assert!(matches!(
+            &request.method,
+            crate::api::schema::Method::PaneSelectionRead(params)
+                if params.anchor.col == 0 && params.cursor.col == 3
+        ));
+        let (_, actions) = state.handle_endpoint_result(
+            "boot-1",
+            &copy_request_id,
+            Ok(crate::api::schema::ResponseResult::PaneSelection {
+                pane_id: "pane_1".into(),
+                text: "LIVE".into(),
+            }),
+        );
+        assert!(matches!(
+            &actions[..],
+            [ClientShellAction::ClipboardWrite(bytes)] if bytes == b"LIVE"
+        ));
+        assert!(state.tick_copy_feedback(deadline.expect("auto-copy highlight deadline")));
+        assert!(state.selection.is_none());
+    }
 }
 
 #[test]
diff --git a/src/workspace/git/discovery.rs b/src/workspace/git/discovery.rs
index de02fa73..24d9fa3c 100644
--- a/src/workspace/git/discovery.rs
+++ b/src/workspace/git/discovery.rs
@@ -498,10 +498,10 @@ mod tests {
 
     #[cfg(unix)]
     #[test]
-    fn unreadable_loose_ref_dir_is_unavailable_not_absent() {
-        use std::os::unix::fs::PermissionsExt;
+    fn symlink_loop_loose_ref_is_unavailable_not_absent() {
+        use std::os::unix::fs::symlink;
 
-        let root = temp_test_dir("unreadable-loose-ref");
+        let root = temp_test_dir("symlink-loop-loose-ref");
         let refs_dir = root.join("refs/heads");
         std::fs::create_dir_all(&refs_dir).unwrap();
         std::fs::write(
@@ -509,19 +509,17 @@ mod tests {
             "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa refs/heads/main\n",
         )
         .unwrap();
-        std::fs::write(
-            refs_dir.join("main"),
-            "bbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbbb\n",
-        )
-        .unwrap();
-        std::fs::set_permissions(&refs_dir, std::fs::Permissions::from_mode(0o000)).unwrap();
-
+        // Unlike chmod(000), a symlink loop produces an open error even as root.
+        // It exercises the same unavailable-ref branch as permission errors.
+        let loose_ref = refs_dir.join("main");
+        symlink("main", &loose_ref).unwrap();
+        let open_error = std::fs::File::open(&loose_ref).unwrap_err();
         let oid = read_ref_oid(&root, "refs/heads/main");
-        std::fs::set_permissions(&refs_dir, std::fs::Permissions::from_mode(0o755)).unwrap();
         std::fs::remove_dir_all(root).unwrap();
+        assert_eq!(open_error.raw_os_error(), Some(libc::ELOOP));
         assert_eq!(
             oid, None,
-            "a loose ref behind a metadata error must be unavailable, not fall back to the stale packed OID"
+            "a loose ref behind an open error must be unavailable, not fall back to the stale packed OID"
         );
     }
 
diff --git a/tests/support/mod.rs b/tests/support/mod.rs
index b2dae4be..f4ccebe8 100644
--- a/tests/support/mod.rs
+++ b/tests/support/mod.rs
@@ -749,12 +749,14 @@ fn runtime_dir_owner_alive(runtime_dir: &Path) -> bool {
     process_exists(owner_pid)
 }
 
-fn current_checkout_root() -> &'static Path {
-    Path::new(env!("CARGO_MANIFEST_DIR"))
-}
-
 fn is_test_herdr_binary(path: &Path) -> bool {
-    path.ends_with("target/debug/herdr") && path.starts_with(current_checkout_root())
+    // /proc resolves executable symlinks. Match only this Cargo build, including
+    // custom target directories; binary identity alone never grants ownership.
+    static TEST_BINARY: OnceLock<Option<PathBuf>> = OnceLock::new();
+    TEST_BINARY
+        .get_or_init(|| fs::canonicalize(env!("CARGO_BIN_EXE_herdr")).ok())
+        .as_deref()
+        .is_some_and(|binary| path == binary)
 }
 
 extern "C" fn run_atexit_cleanup() {
@@ -877,19 +879,48 @@ mod tests {
     }
 
     #[test]
-    fn test_binary_matcher_accepts_current_checkout_debug_binary() {
-        let binary = current_checkout_root().join("target/debug/herdr");
+    fn watchdog_scoping_preserves_registered_live_owner() {
+        let runtime_dir = unique_missing_runtime_dir("live-owner");
+        fs::create_dir_all(&runtime_dir).unwrap();
+        fs::write(
+            runtime_dir.join(RUNTIME_OWNER_MARKER),
+            std::process::id().to_string(),
+        )
+        .unwrap();
+        let registered_runtime_dirs = HashSet::from([runtime_dir.clone()]);
+        let should_terminate = should_terminate_runtime_dir(&runtime_dir, &registered_runtime_dirs);
+        fs::remove_dir_all(runtime_dir).unwrap();
+        assert!(!should_terminate, "a live test owner must remain protected");
+    }
+
+    #[test]
+    fn test_binary_matcher_accepts_cargo_test_binary() {
+        let binary = std::fs::canonicalize(env!("CARGO_BIN_EXE_herdr"))
+            .expect("Cargo-built binary must exist");
         assert!(
             is_test_herdr_binary(&binary),
-            "current checkout debug binary should be considered test-owned"
+            "Cargo-built binary should be considered test-owned regardless of target directory"
         );
     }
 
     #[test]
-    fn test_binary_matcher_rejects_installed_binary() {
-        assert!(
-            !is_test_herdr_binary(Path::new("/home/can/.local/bin/herdr")),
-            "installed binaries must not be considered test-owned"
-        );
+    fn test_binary_matcher_rejects_other_binaries() {
+        let nested_build = Path::new(env!("CARGO_MANIFEST_DIR")).join("other/target/debug/herdr");
+        let sibling_build = Path::new(env!("CARGO_BIN_EXE_herdr"))
+            .parent()
+            .unwrap()
+            .join("other-build/herdr");
+        for binary in [
+            Path::new("/home/can/.local/bin/herdr"),
+            Path::new("/tmp/other-checkout/target/debug/herdr"),
+            nested_build.as_path(),
+            sibling_build.as_path(),
+        ] {
+            assert!(
+                !is_test_herdr_binary(binary),
+                "other binaries must not be considered test-owned: {}",
+                binary.display()
+            );
+        }
     }
 }