From 44359215f515b3908bdb75657c1edbf3404f574f Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 20:57:38 +0000 Subject: [PATCH 1/3] Start one PTY runner for the PTY suites (#1942) Assisted-by: Claude Code:claude-opus-5-5 From e41f3b655449a7c8fe8008c7ab3a3a89eb012223 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 21:01:46 +0000 Subject: [PATCH 2/3] Run PTY test children through pty_io::run Seven PTY suites each hand-rolled the same openpty, spawn, answer, watchdog and collect runner, and the copies drifted: only two pinned TERM, only one could send a second answer, and the update notifier read with its own read_to_end thread. They now all go through pty_io::run (the binary, one answer) or pty_io::run_command (any hermetic command, extra env, several answers), which always pins TERM=xterm-256color. Assisted-by: Claude Code:claude-opus-5-5 --- .../tests/cli/interactive_prompts_e2e.rs | 115 +---------- .../socket-patch-cli/tests/cli/prompts_tty.rs | 180 +++--------------- .../socket-patch-cli/tests/common/pty_io.rs | 96 +++++++++- .../tests/remove/remove_render.rs | 52 +---- .../tests/rollback/rollback_render.rs | 57 +----- .../socket-patch-cli/tests/scan/scan_tty.rs | 86 ++------- .../tests/update/update_notifier_e2e.rs | 67 +------ .../tests/update/update_render.rs | 69 +------ 8 files changed, 160 insertions(+), 562 deletions(-) diff --git a/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs b/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs index 1516063cc..3996923ae 100644 --- a/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs +++ b/crates/socket-patch-cli/tests/cli/interactive_prompts_e2e.rs @@ -10,104 +10,7 @@ use std::path::Path; use std::time::Duration; -use portable_pty::{native_pty_system, PtySize}; - -/// Spawn the socket-patch binary inside a PTY, send `input`, and -/// collect all output until the child exits. Returns `(exit_code, -/// output)`. The timeout is enforced via a watchdog thread that -/// kills the child if it doesn't exit in time. -/// -/// Three pieces compose: -/// * **Reader thread**: `read_to_end` on the master side. -/// Blocks until EOF, which the kernel sends once both the -/// slave fd (dropped here) and the child's last open fd are -/// closed. -/// * **Watchdog thread**: sleeps `timeout` then sends SIGKILL -/// via a cloned ChildKiller. Detaches; no join needed since -/// the killer is idempotent and the child either exits -/// normally first (kill is a no-op) or is killed (we proceed). -/// * **Main thread**: writes input, closes the writer (sends -/// EOF on the child's stdin), blocks on `child.wait()`, then -/// joins the reader. -/// -/// No polling loops, no mpsc channels, no fixed-duration sleeps -/// before sending input — the PTY buffers the input until the -/// child reads it, so timing-coupling isn't needed. -fn run_in_pty(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) { - run_in_pty_bytes(args, cwd, input.as_bytes(), timeout) -} - -/// Byte-level variant of [`run_in_pty`] for input that is not valid -/// UTF-8 (e.g. a Latin-1 paste at an interactive prompt). -fn run_in_pty_bytes(args: &[&str], cwd: &Path, input: &[u8], timeout: Duration) -> (i32, String) { - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment (`tests/common/hermetic.rs`): an ambient - // SOCKET_YES=true would skip the very confirm prompts this file exists - // to drive, and SOCKET_SILENT=true would suppress the output the - // oracles match. It also forces the update notifier off: this suite - // hands the child a REAL terminal, so the stderr-TTY guard does not - // protect it. - let mut cmd = crate::common::hermetic::pty_binary_command(); - for a in args { - cmd.arg(a); - } - cmd.cwd(cwd); - - let mut child = pair - .slave - .spawn_command(cmd) - .expect("spawn socket-patch in PTY"); - // Drop the slave so the master sees EOF once the child closes its - // own copy of the slave fd on exit. - drop(pair.slave); - - // Reader: a single `read_to_end` is sufficient — it blocks until - // EOF, which arrives when (a) the master is dropped (we do that - // below) or (b) the child has exited and its end of the slave is - // closed. The previous design used a chunked read+mpsc loop - // because it interleaved with a try_wait poll; the simplified - // design serializes wait → drop master → read_to_end joins. - let reader_handle = - crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); - - // Watchdog: detach a thread that kills the child after `timeout`. - // The cloned ChildKiller is independent of the main `child` - // handle, so the watchdog can fire without coordinating with the - // main thread. If the child exits naturally first, the kill is a - // no-op against a dead pid. - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - // Writer: send input once a prompt is on screen, then close. - // `ui::confirm` discards typeahead right before it prompts, so input - // written any earlier would be thrown away (see - // `pty_io::send_when_prompted`). - let mut writer = pair.master.take_writer().expect("take writer"); - crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input); - drop(writer); - - // Block until the child exits (watchdog enforces the timeout). - let status = child.wait().expect("child.wait"); - // Drop the master so the reader's `read_to_end` sees EOF and - // returns. - drop(pair.master); - - let output = reader_handle.finish(); - let code = status.exit_code() as i32; - (code, String::from_utf8_lossy(&output).to_string()) -} +use crate::pty_io; // --------------------------------------------------------------------------- // `remove` interactive confirmation @@ -138,7 +41,7 @@ fn remove_interactive_y_proceeds() { let tmp = tempfile::tempdir().unwrap(); write_remove_manifest(tmp.path()); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &[ "remove", "pkg:npm/__interactive_remove__@1.0.0", @@ -186,7 +89,7 @@ fn remove_interactive_n_cancels() { let tmp = tempfile::tempdir().unwrap(); write_remove_manifest(tmp.path()); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &[ "remove", "pkg:npm/__interactive_remove__@1.0.0", @@ -247,14 +150,16 @@ fn remove_interactive_non_utf8_answer_declines_without_panic() { // InvalidData error. Regression: `confirm()` unwrapped that error // and panicked (exit 101) instead of treating the garbage like any // other unrecognized answer (decline). - let (code, output) = run_in_pty_bytes( + let pty_io::PtyRun { code, output, .. } = pty_io::run_command( + crate::common::hermetic::pty_binary_command(), &[ "remove", "pkg:npm/__interactive_remove__@1.0.0", "--skip-rollback", ], tmp.path(), - b"\xE9\n", + &[], + &[b"\xE9\n"], Duration::from_secs(15), ); assert!( @@ -329,7 +234,7 @@ fn remove_detached_interactive_n_cancel_message_respects_silent() { let tmp = tempfile::tempdir().unwrap(); write_detached_vendor_state(tmp.path(), purl, uuid); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &["remove", purl, "--silent"], tmp.path(), "n\n", @@ -365,7 +270,7 @@ fn remove_detached_interactive_n_cancel_message_respects_silent() { // fix that deleted the message instead of gating it. let tmp2 = tempfile::tempdir().unwrap(); write_detached_vendor_state(tmp2.path(), purl, uuid); - let (loud_code, loud_output) = run_in_pty( + let (loud_code, loud_output) = pty_io::run( &["remove", purl], tmp2.path(), "n\n", @@ -393,7 +298,7 @@ fn remove_detached_interactive_n_cancel_message_respects_silent() { #[test] fn apply_in_pty_with_no_manifest_prints_friendly_message() { let tmp = tempfile::tempdir().unwrap(); - let (code, output) = run_in_pty(&["apply"], tmp.path(), "", Duration::from_secs(15)); + let (code, output) = pty_io::run(&["apply"], tmp.path(), "", Duration::from_secs(15)); assert_eq!(code, 0); // Assert the full message, not either half of it. The wording names the // missing manifest, not the `.socket/` folder (hosted/vendored projects diff --git a/crates/socket-patch-cli/tests/cli/prompts_tty.rs b/crates/socket-patch-cli/tests/cli/prompts_tty.rs index 06ba84bdb..8c0dcbac3 100644 --- a/crates/socket-patch-cli/tests/cli/prompts_tty.rs +++ b/crates/socket-patch-cli/tests/cli/prompts_tty.rs @@ -14,12 +14,9 @@ #![cfg(unix)] -use crate::common::binary; - use std::path::Path; use std::time::Duration; -use portable_pty::{native_pty_system, PtySize}; use wiremock::matchers::{method, path}; use wiremock::{Mock, MockServer, ResponseTemplate}; @@ -27,144 +24,12 @@ use wiremock::{Mock, MockServer, ResponseTemplate}; // the "patch has no applicable files" guardrail with a real blob // hash). Read-only reuse of the shared helper module. use crate::common; +use crate::pty_io; const ORG_SLUG: &str = "test-org"; const UUID_A: &str = "11111111-1111-4111-8111-111111111111"; const UUID_B: &str = "22222222-2222-4222-8222-222222222222"; -/// Spawn the socket-patch binary inside a PTY, send `input`, and collect -/// all output until the child exits. Returns `(exit_code, output)`. -/// -/// Same choreography as the sibling `interactive_prompts_e2e.rs` harness -/// (reader thread on the master, detached SIGKILL watchdog, input written -/// once a prompt is on screen via `pty_io::send_when_prompted` — `confirm` -/// discards earlier typeahead — then EOF on the writer) — plus a -/// pinned `TERM` because the `dialoguer`/`console` menu these tests drive -/// derives key handling and rendering from the terminal type, which the -/// ambient environment (some CI shells) may not set at all. -fn run_in_pty(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) { - let (code, _signal, output) = run_in_pty_raw(Sigint::Default, args, cwd, input, timeout); - (code, output) -} - -/// [`run_in_pty`] with a second answer: `then` is written once the y/n -/// confirm (`[Y/n] `) is on screen, after `input` answered the first -/// prompt (the dialoguer menu). -fn run_in_pty_then( - args: &[&str], - cwd: &Path, - input: &str, - then: &str, - timeout: Duration, -) -> (i32, String) { - let (code, _signal, output) = - run_in_pty_inner(Sigint::Default, args, cwd, input, Some(then), timeout); - (code, output) -} - -/// The SIGINT disposition the binary starts with under [`run_in_pty_raw`]. -#[derive(Clone, Copy, PartialEq, Eq)] -enum Sigint { - /// Inherited default: Ctrl-C kills the process. - Default, - /// Ignored (as `nohup` and some launchers do), via - /// `sh -c 'trap "" INT; exec ...'`. - Ignored, -} - -/// [`run_in_pty`], also returning the name of the signal that ended the -/// child (`None` for a normal exit), with the binary started under the -/// given [`Sigint`] disposition. -fn run_in_pty_raw( - sigint: Sigint, - args: &[&str], - cwd: &Path, - input: &str, - timeout: Duration, -) -> (i32, Option, String) { - run_in_pty_inner(sigint, args, cwd, input, None, timeout) -} - -fn run_in_pty_inner( - sigint: Sigint, - args: &[&str], - cwd: &Path, - input: &str, - then: Option<&str>, - timeout: Duration, -) -> (i32, Option, String) { - let input = input.as_bytes(); - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment (`tests/common/hermetic.rs`): an ambient - // SOCKET_YES=true would skip the confirm prompt the bare-Enter test - // drives, and SOCKET_JSON=true flips select_one onto its - // JsonModeNeedsExplicit branch before the dialoguer menu renders. The - // `sh` wrapper `exec`s the binary, which inherits the same environment. - let mut cmd = if sigint == Sigint::Ignored { - let mut cmd = common::hermetic::pty_command("/bin/sh"); - cmd.arg("-c"); - cmd.arg("trap '' INT; exec \"$0\" \"$@\""); - cmd.arg(binary()); - cmd - } else { - common::hermetic::pty_binary_command() - }; - for a in args { - cmd.arg(a); - } - cmd.cwd(cwd); - // `dialoguer`/`console` key handling needs a terminal type; pin one so - // an unset ambient TERM (bare CI shells) can't skew rendering. - cmd.env("TERM", "xterm-256color"); - - let mut child = pair - .slave - .spawn_command(cmd) - .expect("spawn socket-patch in PTY"); - drop(pair.slave); - - let reader_handle = - crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); - - // Watchdog: detached kill after `timeout`; a no-op if the child exits - // naturally first. - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - let mut writer = pair.master.take_writer().expect("take writer"); - crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input); - if let Some(then) = then { - use std::io::Write; - reader_handle.wait_for_count("[Y/n] ", 1, Duration::from_secs(10)); - let _ = writer.write_all(then.as_bytes()); - let _ = writer.flush(); - } - drop(writer); - - let status = child.wait().expect("child.wait"); - drop(pair.master); - - let output = reader_handle.finish(); - let code = status.exit_code() as i32; - ( - code, - status.signal().map(str::to_string), - String::from_utf8_lossy(&output).to_string(), - ) -} - const HIDE_CURSOR: &str = "\x1b[?25l"; const SHOW_CURSOR: &str = "\x1b[?25h"; @@ -214,7 +79,7 @@ fn remove_interactive_bare_enter_proceeds_with_default_yes() { let tmp = tempfile::tempdir().unwrap(); write_remove_manifest(tmp.path()); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &[ "remove", "pkg:npm/__prompts_tty_remove__@1.0.0", @@ -371,7 +236,8 @@ fn get_interactive_dialoguer_enter_accepts_first_ranked_option() { // No `--yes`: it answers the menu with its default without showing it. // "\r" (Enter at a raw-mode terminal) accepts the menu's default, then // "\n" answers the "Download 1 patch?" confirm that follows. - let (code, output) = run_in_pty_then( + let pty_io::PtyRun { code, output, .. } = pty_io::run_command( + common::hermetic::pty_binary_command(), &[ "get", purl, @@ -384,8 +250,8 @@ fn get_interactive_dialoguer_enter_accepts_first_ranked_option() { ORG_SLUG, ], tmp.path(), - "\r", - "\n", + &[], + &[b"\r", b"\n"], Duration::from_secs(20), ); assert_eq!( @@ -453,7 +319,7 @@ fn get_yes_answers_the_menu_with_its_default_without_showing_it() { let uri = mock.uri(); let tmp = tempfile::tempdir().unwrap(); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &[ "get", purl, @@ -480,7 +346,7 @@ fn get_yes_answers_the_menu_with_its_default_without_showing_it() { let manifest: serde_json::Value = serde_json::from_str(&body).unwrap(); assert_eq!(manifest["patches"][purl]["uuid"], UUID_A, "{body}"); // The listing showed two patches; the pick is named before saving. - let screen = crate::pty_io::render(output.as_bytes()).join("\n"); + let screen = pty_io::render(output.as_bytes()).join("\n"); assert!( screen.contains("Selected:\n pkg:npm/prompt-multi-yes@1.0.0 [FREE] 11111111"), "{screen}" @@ -512,7 +378,7 @@ fn get_interactive_dialoguer_quit_cancels_with_exit_zero() { let uri = mock.uri(); let tmp = tempfile::tempdir().unwrap(); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &[ "get", purl, @@ -589,8 +455,12 @@ fn get_interactive_dialoguer_ctrl_c_restores_cursor_and_dies_by_sigint() { let uri = mock.uri(); let tmp = tempfile::tempdir().unwrap(); - let (code, signal, output) = run_in_pty_raw( - Sigint::Default, + let pty_io::PtyRun { + code, + signal, + output, + } = pty_io::run_command( + common::hermetic::pty_binary_command(), &[ "get", purl, @@ -603,7 +473,8 @@ fn get_interactive_dialoguer_ctrl_c_restores_cursor_and_dies_by_sigint() { ORG_SLUG, ], tmp.path(), - "\x03", + &[], + &[b"\x03"], Duration::from_secs(20), ); assert!( @@ -642,8 +513,18 @@ fn get_interactive_dialoguer_ctrl_c_with_sigint_ignored_cancels_cleanly() { let uri = mock.uri(); let tmp = tempfile::tempdir().unwrap(); - let (code, signal, output) = run_in_pty_raw( - Sigint::Ignored, + // `sh` starts the binary with SIGINT ignored (as `nohup` and some + // launchers do); `exec` keeps the hermetic environment. + let mut sh = common::hermetic::pty_command("/bin/sh"); + sh.arg("-c"); + sh.arg("trap '' INT; exec \"$0\" \"$@\""); + sh.arg(common::binary()); + let pty_io::PtyRun { + code, + signal, + output, + } = pty_io::run_command( + sh, &[ "get", purl, @@ -656,7 +537,8 @@ fn get_interactive_dialoguer_ctrl_c_with_sigint_ignored_cancels_cleanly() { ORG_SLUG, ], tmp.path(), - "\x03", + &[], + &[b"\x03"], Duration::from_secs(20), ); assert!( diff --git a/crates/socket-patch-cli/tests/common/pty_io.rs b/crates/socket-patch-cli/tests/common/pty_io.rs index ee818f993..7b4f198bf 100644 --- a/crates/socket-patch-cli/tests/common/pty_io.rs +++ b/crates/socket-patch-cli/tests/common/pty_io.rs @@ -1,4 +1,6 @@ -//! PTY I/O shared by the interactive suites. +//! The PTY runner shared by the interactive suites: [`run`] and +//! [`run_command`] spawn a hermetic child on a real terminal, answer its +//! prompts and collect what it printed. //! //! `confirm()` discards terminal typeahead right before it shows a y/n //! prompt (so an Enter pressed during a long scan cannot answer it). Input @@ -12,6 +14,7 @@ #![allow(dead_code)] use std::io::{Read, Write}; +use std::path::Path; use std::sync::{Arc, Mutex}; use std::thread::JoinHandle; use std::time::{Duration, Instant}; @@ -92,6 +95,97 @@ pub fn send_when_prompted(out: &PtyOutput, writer: &mut dyn Write, input: &[u8]) let _ = writer.flush(); } +/// How a [`run_command`] child ended, and everything it printed. +pub struct PtyRun { + pub code: i32, + /// The signal that ended the child; `None` for a normal exit. + pub signal: Option, + pub output: String, +} + +/// Run the built binary with `args` in `cwd` on a PTY, answer the first +/// prompt with `input`, and return `(exit_code, output)`. +pub fn run(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) { + let run = run_command( + crate::common::hermetic::pty_binary_command(), + args, + cwd, + &[], + &[input.as_bytes()], + timeout, + ); + (run.code, run.output) +} + +/// Run `cmd` (a `hermetic::pty_command`) with `args` in `cwd` on a +/// 24x80 PTY, `env` applied last so it survives the scrub. `answers[0]` +/// is written once any prompt is on screen; `answers[i]` once the `[Y/n] ` +/// confirm hint has appeared `i` times. The writer then closes, and the +/// child is killed if it is still running after `timeout`. +/// +/// `TERM` is pinned: `console`/`dialoguer` derive key handling and +/// rendering from it, and bare CI shells leave it unset or `dumb`. +pub fn run_command( + mut cmd: portable_pty::CommandBuilder, + args: &[&str], + cwd: &Path, + env: &[(&str, &str)], + answers: &[&[u8]], + timeout: Duration, +) -> PtyRun { + let pair = portable_pty::native_pty_system() + .openpty(portable_pty::PtySize { + rows: 24, + cols: 80, + pixel_width: 0, + pixel_height: 0, + }) + .expect("openpty"); + + for a in args { + cmd.arg(a); + } + cmd.cwd(cwd); + cmd.env("TERM", "xterm-256color"); + for (k, v) in env { + cmd.env(k, v); + } + + let mut child = pair.slave.spawn_command(cmd).expect("spawn in PTY"); + // The master sees EOF only once every slave fd is closed: ours now, + // the child's when it exits. + drop(pair.slave); + + let output = PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); + + let mut killer = child.clone_killer(); + std::thread::spawn(move || { + std::thread::sleep(timeout); + let _ = killer.kill(); + }); + + let mut writer = pair.master.take_writer().expect("take writer"); + for (i, answer) in answers.iter().enumerate() { + if i == 0 { + send_when_prompted(&output, &mut writer, answer); + } else { + output.wait_for_count("[Y/n] ", i, Duration::from_secs(10)); + let _ = writer.write_all(answer); + let _ = writer.flush(); + } + } + drop(writer); + + let status = child.wait().expect("child.wait"); + drop(pair.master); + let output = output.finish(); + PtyRun { + code: status.exit_code() as i32, + signal: status.signal().map(str::to_string), + output: String::from_utf8_lossy(&output).into_owned(), + } +} + /// The lib's terminal emulator (`ui::test_support::render`), included /// from the same source file so the two can't drift. #[path = "../../src/ui/test_support.rs"] diff --git a/crates/socket-patch-cli/tests/remove/remove_render.rs b/crates/socket-patch-cli/tests/remove/remove_render.rs index e5743cc55..59d5127ee 100644 --- a/crates/socket-patch-cli/tests/remove/remove_render.rs +++ b/crates/socket-patch-cli/tests/remove/remove_render.rs @@ -461,60 +461,12 @@ fn remove_detached_drift_keep_human_prints_keep_lines() { /// Hosted-only interactive decline, PTY-driven: the non-TTY confirm /// auto-proceeds, so only a real terminal reaches the cancel branch. -/// Runner copied from interactive_prompts_e2e.rs. #[cfg(unix)] mod pty { use super::*; - use portable_pty::{native_pty_system, PtySize}; - use std::path::Path; + use crate::pty_io; use std::time::Duration; - /// Spawn the binary inside a PTY, send `input`, collect all output - /// until exit (watchdog-killed after `timeout`). - fn run_in_pty(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) { - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment: SOCKET_YES would skip the very prompt - // this test drives. - let mut cmd = common::hermetic::pty_binary_command(); - for a in args { - cmd.arg(a); - } - cmd.cwd(cwd); - - let mut child = pair.slave.spawn_command(cmd).expect("spawn in PTY"); - drop(pair.slave); - - let reader_handle = - crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); - - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - let mut writer = pair.master.take_writer().expect("take writer"); - crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input.as_bytes()); - drop(writer); - - let status = child.wait().expect("child.wait"); - drop(pair.master); - let output = reader_handle.finish(); - ( - status.exit_code() as i32, - String::from_utf8_lossy(&output).to_string(), - ) - } - /// Declining the hosted-only confirm prompt must cancel cleanly (exit /// 0, "Cancelled; no changes made.") with the lock byte-identical and no /// `.socket/` state written. @@ -525,7 +477,7 @@ mod pty { std::fs::write(&lock_path, redirected_lock_text()).unwrap(); let lock_before = read_bytes(&lock_path); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &["remove", NPM_PURL], tmp.path(), "n\n", diff --git a/crates/socket-patch-cli/tests/rollback/rollback_render.rs b/crates/socket-patch-cli/tests/rollback/rollback_render.rs index 2402ff207..02a602196 100644 --- a/crates/socket-patch-cli/tests/rollback/rollback_render.rs +++ b/crates/socket-patch-cli/tests/rollback/rollback_render.rs @@ -1114,64 +1114,11 @@ fn manifest_write_failure_prints_human_error_line() { #[cfg(unix)] mod interactive { - use std::path::Path; - - use crate::common; use crate::common::git_sha256; + use crate::pty_io; use crate::rollback_fixture::*; - use portable_pty::{native_pty_system, PtySize}; use std::time::Duration; - /// Spawn the binary inside a PTY, send `input`, collect all output — - /// the `interactive_prompts_e2e.rs` harness (same env scrubbing). - fn run_in_pty(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) { - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment: SOCKET_YES would skip the very prompt - // under test. - let mut cmd = common::hermetic::pty_binary_command(); - for a in args { - cmd.arg(a); - } - cmd.cwd(cwd); - - let mut child = pair - .slave - .spawn_command(cmd) - .expect("spawn socket-patch in PTY"); - drop(pair.slave); - - let reader_handle = - crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); - - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - let mut writer = pair.master.take_writer().expect("take writer"); - crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input.as_bytes()); - drop(writer); - - let status = child.wait().expect("child.wait"); - drop(pair.master); - - let output = reader_handle.finish(); - ( - status.exit_code() as i32, - String::from_utf8_lossy(&output).to_string(), - ) - } - /// Declining the rollback confirm prompt cancels cleanly: the composed /// manifest clause renders with the `[Y/n]` hint, "Cancelled; no changes made." /// prints, the run exits 0, and nothing is mutated. @@ -1192,7 +1139,7 @@ mod interactive { let manifest_before = std::fs::read(socket.join("manifest.json")).expect("read manifest bytes"); - let (code, output) = run_in_pty( + let (code, output) = pty_io::run( &["rollback", "--offline"], tmp.path(), "n\n", diff --git a/crates/socket-patch-cli/tests/scan/scan_tty.rs b/crates/socket-patch-cli/tests/scan/scan_tty.rs index dbaf88e97..7aad29b34 100644 --- a/crates/socket-patch-cli/tests/scan/scan_tty.rs +++ b/crates/socket-patch-cli/tests/scan/scan_tty.rs @@ -3,80 +3,14 @@ //! Unix only (`portable_pty`); the built binary runs in the hermetic //! environment. -use std::path::Path; use std::time::Duration; -use portable_pty::{native_pty_system, PtySize}; use wiremock::MockServer; use crate::common; +use crate::pty_io; use crate::scan_fixture::*; -/// Spawn the binary in a PTY, send `input`, and collect all output -/// until exit (the `interactive_prompts_e2e.rs` harness pattern: -/// reader thread + kill-after-timeout watchdog, no polling). -fn run_in_pty(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, String) { - run_in_pty_with(args, cwd, &[], input, timeout) -} - -/// [`run_in_pty`] with extra child `env`. -fn run_in_pty_with( - args: &[&str], - cwd: &Path, - env: &[(&str, &str)], - input: &str, - timeout: Duration, -) -> (i32, String) { - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment (`tests/common/hermetic.rs`), which also - // forces the update notifier off: this child gets a REAL terminal, - // so the stderr-TTY guard does not protect it. - let mut cmd = common::hermetic::pty_binary_command(); - for a in args { - cmd.arg(a); - } - cmd.cwd(cwd); - // These tests assert VT rendering, even when the parent has TERM=dumb. - cmd.env("TERM", "xterm-256color"); - for (k, v) in env { - cmd.env(k, v); - } - - let mut child = pair.slave.spawn_command(cmd).expect("spawn in PTY"); - drop(pair.slave); - - let reader_handle = - crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); - - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - let mut writer = pair.master.take_writer().expect("take writer"); - crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input.as_bytes()); - drop(writer); - - let status = child.wait().expect("child.wait"); - drop(pair.master); - - let output = reader_handle.finish(); - ( - status.exit_code() as i32, - String::from_utf8_lossy(&output).to_string(), - ) -} - /// v5 scan never prompts, even on a TTY: agent mode applies with no /// input at all. #[tokio::test(flavor = "multi_thread")] @@ -93,7 +27,7 @@ async fn scan_on_a_tty_applies_without_prompting() { let uri = mock.uri(); let cwd = tmp.path().to_path_buf(); let (code, output) = tokio::task::spawn_blocking(move || { - run_in_pty( + pty_io::run( &[&["scan", "--mode", "agent"][..], &api_args(&uri)].concat(), &cwd, "", @@ -151,7 +85,7 @@ async fn scan_json_on_a_tty_never_opens_the_select_menu() { let uri = mock.uri(); let cwd = tmp.path().to_path_buf(); let (code, output) = tokio::task::spawn_blocking(move || { - run_in_pty( + pty_io::run( &[ &["scan", "--json", "--mode", "agent", "--dry-run"][..], &api_args(&uri), @@ -203,7 +137,7 @@ async fn scan_progress_on_a_tty_renders_without_stale_tail() { let uri = mock.uri(); let cwd = tmp.path().to_path_buf(); let (code, output) = tokio::task::spawn_blocking(move || { - run_in_pty( + pty_io::run( &[&["scan"][..], &api_args(&uri)].concat(), &cwd, "n\n", @@ -221,7 +155,7 @@ async fn scan_progress_on_a_tty_renders_without_stale_tail() { "expected a live status update; raw={output:?}" ); // ...and the screen shows only clean result lines. - let screen = crate::pty_io::render(output.as_bytes()); + let screen = pty_io::render(output.as_bytes()); assert!( screen.iter().any(|l| l == "Found 1 package (1 npm)"), "screen:\n{}", @@ -260,19 +194,21 @@ async fn scan_debug_mode_on_a_tty_keeps_debug_lines_off_the_status() { let uri = mock.uri(); let cwd = tmp.path().to_path_buf(); let (code, output) = tokio::task::spawn_blocking(move || { - run_in_pty_with( + let run = pty_io::run_command( + common::hermetic::pty_binary_command(), &[&["scan"][..], &api_args(&uri)].concat(), &cwd, &[("SOCKET_DEBUG", "1")], - "n\n", + &[b"n\n"], Duration::from_secs(60), - ) + ); + (run.code, run.output) }) .await .expect("spawn_blocking join"); assert_eq!(code, 0, "output:\n{output}"); - let screen = crate::pty_io::render(output.as_bytes()); + let screen = pty_io::render(output.as_bytes()); let debug: Vec<&String> = screen .iter() .filter(|l| l.contains("[socket-patch debug]")) diff --git a/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs b/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs index a3196eda6..62999a9e6 100644 --- a/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs +++ b/crates/socket-patch-cli/tests/update/update_notifier_e2e.rs @@ -594,66 +594,7 @@ async fn update_command_never_notifies() { #[cfg(unix)] mod pty { use super::*; - use portable_pty::{native_pty_system, PtySize}; - use std::io::Read; - - /// Minimal PTY runner (same shape as `interactive_prompts_e2e.rs`): - /// reader thread to EOF, detached SIGKILL watchdog, no input. This - /// bypasses `run_installed`, so the update kit is applied by hand on - /// top of `hermetic::pty_command`. - fn run_in_pty( - bin: &Path, - cwd: &Path, - env: &[(&str, &str)], - timeout: Duration, - ) -> (i32, String) { - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment, then the caller's kit, which lands last - // (its `SOCKET_NO_UPDATE_CHECK=0` turns the notifier back on). - let mut cmd = common::hermetic::pty_command(bin); - cmd.arg("apply"); - cmd.cwd(cwd); - for (k, v) in env { - cmd.env(k, v); - } - - let mut child = pair.slave.spawn_command(cmd).expect("spawn in PTY"); - drop(pair.slave); - - let mut reader = pair.master.try_clone_reader().expect("clone reader"); - let reader_handle = std::thread::spawn(move || { - let mut buf = Vec::new(); - let _ = reader.read_to_end(&mut buf); - buf - }); - - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - // No prompt to answer: close the writer immediately so the child's - // stdin sees EOF if anything ever reads it. - drop(pair.master.take_writer().expect("take writer")); - - let status = child.wait().expect("child.wait"); - drop(pair.master); - let output = reader_handle.join().expect("reader join"); - ( - status.exit_code() as i32, - String::from_utf8_lossy(&output).to_string(), - ) - } + use crate::pty_io; #[tokio::test] async fn real_tty_shows_notice_pty() { @@ -675,10 +616,12 @@ mod pty { // `eligible_kit_await_fetch`). ("SOCKET_UPDATE_GRACE_MS", "30000"), ]; - let (code, output) = run_in_pty( - &install.bin, + let pty_io::PtyRun { code, output, .. } = pty_io::run_command( + common::hermetic::pty_command(&install.bin), + &["apply"], &install.workdir, &kit, + &[], Duration::from_secs(30), ); assert_eq!(code, 0, "carrier must succeed in a PTY; got: {output}"); diff --git a/crates/socket-patch-cli/tests/update/update_render.rs b/crates/socket-patch-cli/tests/update/update_render.rs index ba7d9e646..74733c73f 100644 --- a/crates/socket-patch-cli/tests/update/update_render.rs +++ b/crates/socket-patch-cli/tests/update/update_render.rs @@ -127,70 +127,9 @@ async fn update_dry_run_human_reports_update_available() { #[cfg(unix)] mod pty { use super::*; - use portable_pty::{native_pty_system, PtySize}; - use std::path::Path; + use crate::pty_io; use std::time::Duration; - /// Spawn `bin` inside a PTY under the standard SOCKET_* scrub, apply - /// `env` last (so injections survive), send `input`, and collect all - /// output until exit (watchdog-killed after `timeout`). - fn run_in_pty_env( - bin: &Path, - args: &[&str], - cwd: &Path, - env: &[(&str, &str)], - input: &str, - timeout: Duration, - ) -> (i32, String) { - let pty_system = native_pty_system(); - let pair = pty_system - .openpty(PtySize { - rows: 24, - cols: 80, - pixel_width: 0, - pixel_height: 0, - }) - .expect("openpty"); - - // The hermetic environment: SOCKET_YES would skip the very prompt - // this test drives, and the update notifier is forced off (this - // suite hands the child a REAL terminal, so the stderr-TTY guard - // does not protect it). - let mut cmd = crate::common::hermetic::pty_command(bin); - for a in args { - cmd.arg(a); - } - cmd.cwd(cwd); - // Caller env lands last so explicit injections survive the scrub. - for (k, v) in env { - cmd.env(k, v); - } - - let mut child = pair.slave.spawn_command(cmd).expect("spawn in PTY"); - drop(pair.slave); - - let reader_handle = - crate::pty_io::PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); - - let mut killer = child.clone_killer(); - std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); - }); - - let mut writer = pair.master.take_writer().expect("take writer"); - crate::pty_io::send_when_prompted(&reader_handle, &mut writer, input.as_bytes()); - drop(writer); - - let status = child.wait().expect("child.wait"); - drop(pair.master); - let output = reader_handle.finish(); - ( - status.exit_code() as i32, - String::from_utf8_lossy(&output).to_string(), - ) - } - /// Declining the reinstall confirm must cancel with exit 1 and /// "Cancelled; no changes made.", leaving the installed binary byte-identical. /// The pin-to-current + `--force` combination reaches the confirm with @@ -204,15 +143,15 @@ mod pty { let install = staged_install(); let state_dir = install.state_dir.display().to_string(); - let (code, output) = run_in_pty_env( - &install.bin, + let pty_io::PtyRun { code, output, .. } = pty_io::run_command( + crate::common::hermetic::pty_command(&install.bin), &["--update", CURRENT, "--force"], &install.workdir, &[ ("SOCKET_UPDATE_BASE_URL", DEAD_ENDPOINT), ("SOCKET_UPDATE_STATE_DIR", state_dir.as_str()), ], - "n\n", + &[b"n\n"], Duration::from_secs(15), ); From e8cd2b5c9e4590465c95e5e3fceef6a85f978f7a Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 1 Nov 2026 21:11:54 +0000 Subject: [PATCH 3/3] Cancel the PTY watchdog once the child is reaped The detached watchdog slept out its full timeout and then signalled the pid even after the child had exited, so it could hit a reused pid. It now waits on a channel that is dropped after wait(). Also correct the runner's doc (SIGHUP, the newline portable_pty writes on drop, which answers wait for [Y/n]) and the stale update_render note. Assisted-by: Claude Code:claude-opus-5-5 --- .../socket-patch-cli/tests/common/pty_io.rs | 19 +++++++++++++------ .../tests/update/update_render.rs | 6 +++--- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/crates/socket-patch-cli/tests/common/pty_io.rs b/crates/socket-patch-cli/tests/common/pty_io.rs index 7b4f198bf..b0674d4db 100644 --- a/crates/socket-patch-cli/tests/common/pty_io.rs +++ b/crates/socket-patch-cli/tests/common/pty_io.rs @@ -8,8 +8,9 @@ //! away, exactly as a real user's early keystrokes are. These helpers send //! the scripted answer only once a prompt is on screen. //! -//! Pull in with `#[path = "common/pty_io.rs"] mod pty_io;` and use it via -//! `crate::pty_io::...`. +//! Pull in with `#[path = "common/pty_io.rs"] mod pty_io;` next to the +//! crate's `mod common` (the runner spawns through `common::hermetic`), and +//! use it via `crate::pty_io::...`. #![allow(dead_code)] @@ -120,8 +121,10 @@ pub fn run(args: &[&str], cwd: &Path, input: &str, timeout: Duration) -> (i32, S /// Run `cmd` (a `hermetic::pty_command`) with `args` in `cwd` on a /// 24x80 PTY, `env` applied last so it survives the scrub. `answers[0]` /// is written once any prompt is on screen; `answers[i]` once the `[Y/n] ` -/// confirm hint has appeared `i` times. The writer then closes, and the -/// child is killed if it is still running after `timeout`. +/// confirm hint has appeared `i` times (so only `answers[0]` may answer a +/// menu, and the rest answer `[Y/n]` confirms in order). Then the writer is +/// dropped, which portable_pty does by sending a newline and EOF. A child +/// still running after `timeout` gets SIGHUP (`ChildKiller::kill`). /// /// `TERM` is pinned: `console`/`dialoguer` derive key handling and /// rendering from it, and bare CI shells leave it unset or `dumb`. @@ -158,10 +161,13 @@ pub fn run_command( let output = PtyOutput::spawn(pair.master.try_clone_reader().expect("clone reader")); + // Cancelled once the child is reaped, so it never signals a reused pid. + let (reaped, watchdog) = std::sync::mpsc::channel::<()>(); let mut killer = child.clone_killer(); std::thread::spawn(move || { - std::thread::sleep(timeout); - let _ = killer.kill(); + if let Err(std::sync::mpsc::RecvTimeoutError::Timeout) = watchdog.recv_timeout(timeout) { + let _ = killer.kill(); + } }); let mut writer = pair.master.take_writer().expect("take writer"); @@ -177,6 +183,7 @@ pub fn run_command( drop(writer); let status = child.wait().expect("child.wait"); + drop(reaped); drop(pair.master); let output = output.finish(); PtyRun { diff --git a/crates/socket-patch-cli/tests/update/update_render.rs b/crates/socket-patch-cli/tests/update/update_render.rs index 74733c73f..a83c5753f 100644 --- a/crates/socket-patch-cli/tests/update/update_render.rs +++ b/crates/socket-patch-cli/tests/update/update_render.rs @@ -117,9 +117,9 @@ async fn update_dry_run_human_reports_update_available() { // --------------------------------------------------------------------------- // Interactive decline — PTY-driven: ui::confirm // auto-proceeds with default-yes on non-TTY stdin (and under --yes/--json), -// so only a real terminal reaches the cancel branch. Runner copied from -// interactive_prompts_e2e.rs, adapted to spawn the -// STAGED binary with injected env — so even a decline-regression that +// so only a real terminal reaches the cancel branch. The test spawns the +// STAGED binary (`hermetic::pty_command(&install.bin)`) with injected +// env — so even a decline-regression that // proceeds to a swap can only ever touch the tempdir copy, and the dead // SOCKET_UPDATE_BASE_URL kills such a run at download. // ---------------------------------------------------------------------------