From 5896ff36b1a3faf99056a7beb9a2cdafffa54611 Mon Sep 17 00:00:00 2001 From: Nathan Herald Date: Wed, 4 Nov 2026 04:05:18 +0100 Subject: [PATCH] Decline attach's restart prompt when nobody answers (#214) attach on a dead session asks Restart? [Y/n] and read stdin with no terminal check. EOF read as an empty answer, which restarts, so a caller without a terminal reran the stored command although nobody agreed to it. --force did the same. The prompt now counts only an answer someone gave (decision 0020): - At a terminal, any line but a lone n restarts, as before. - Off a terminal, only y or yes restarts. - EOF never does. With no answer, attach exits 1, keeps the exited record and names -r. A lone n still exits 0, -r still restarts without asking, and --force still answers nothing. The Node tool restarts nothing at EOF either, but it exits 0 there, and off a terminal it restarts on any line but n. The conformance ledger records the three new tests that fail on Node as intended. pty restart's Kill and restart? prompt is unchanged. At EOF it still stops a running session and starts it again, where Node leaves the session running. --- README.md | 7 ++ crates/pty-conformance/divergences.toml | 37 ++++++ .../tests/attach_no_restart.rs | 86 ++++++++++++++ crates/pty/src/cli/ask.rs | 110 +++++++++++++++--- crates/pty/src/cli/attach.rs | 17 ++- ...nswered-restart-prompt-does-not-restart.md | 78 +++++++++++++ docs/parity.md | 1 + 7 files changed, 320 insertions(+), 16 deletions(-) create mode 100644 docs/decisions/0020-an-unanswered-restart-prompt-does-not-restart.md diff --git a/README.md b/README.md index 9b66b73b..2fd94fe4 100644 --- a/README.md +++ b/README.md @@ -497,6 +497,13 @@ A legacy record that already has an exit while its daemon is still finishing is refused the same way, before any restart prompt: a restart waits for proof that the daemon is gone, and a live legacy daemon cannot give it. +**A dead session restarts only on an answer.** `attach` on a session that has +exited shows its last lines and asks `Restart? [Y/n]`. At a terminal, Enter +restarts it. Without one, only `y` or `yes` does. At EOF, or on any other +input, attach exits 1 and the stored command does not run. `-r` restarts +without asking, `--no-restart` never restarts, and `--force` answers nothing. +The Node tool differs here ([decision 0020](docs/decisions/0020-an-unanswered-restart-prompt-does-not-restart.md)). + `run` and `restart` are different and deliberately so: from inside a session `run` runs the command directly and `restart` restarts without attaching. Both did what was asked, so both exit 0. diff --git a/crates/pty-conformance/divergences.toml b/crates/pty-conformance/divergences.toml index e4f57e13..d0a3371d 100644 --- a/crates/pty-conformance/divergences.toml +++ b/crates/pty-conformance/divergences.toml @@ -297,3 +297,40 @@ f005432ad5c73b79abd7c5eaa8a0d6eb66ee9194 on Node 22 exceeded the five-second \ harness deadline in CI run 40986814385. Remove when Node honors the quiet \ session-first timeout. """ + +[[divergence]] +test = "attach_no_restart::a_restart_prompt_at_eof_declines" +side = "node" +kind = "intended" +reason = """ +Decision 0020: attach's dead-session prompt at EOF declines, exits 1 and \ +names -r. Pinned Node f005432ad5c73b79abd7c5eaa8a0d6eb66ee9194 restarts \ +nothing there either, but its readline question never resolves and it exits \ +0 after the prompt, measured on Node 22.23.2 and 24.18.0 on Linux. Remove \ +this entry if Node exits 1 there, or if Rust drops the decision. +""" + +[[divergence]] +test = "attach_no_restart::a_restart_prompt_off_a_terminal_needs_a_yes" +side = "node" +kind = "intended" +reason = """ +Decision 0020: with stdin not a terminal, only y or yes restarts a dead \ +session from attach's prompt. Pinned Node \ +f005432ad5c73b79abd7c5eaa8a0d6eb66ee9194 restarts on any line but a lone n, \ +so a piped empty line or relayed input reruns the stored command, measured \ +on Node 22.23.2 on Linux. Remove this entry if Node requires an explicit \ +answer, or if Rust drops the decision. +""" + +[[divergence]] +test = "attach_no_restart::force_is_not_an_answer_to_the_restart_prompt" +side = "node" +kind = "intended" +reason = """ +Decision 0020: --force answers nothing, so attach --force at EOF declines, \ +exits 1 and names -r. Pinned Node f005432ad5c73b79abd7c5eaa8a0d6eb66ee9194 \ +also restarts nothing there but exits 0 after the prompt, measured on Node \ +22.23.2 on Linux. The test's piped-y half restarts on both. Remove this \ +entry with the EOF entry above. +""" diff --git a/crates/pty-conformance/tests/attach_no_restart.rs b/crates/pty-conformance/tests/attach_no_restart.rs index 59fcae6d..cfb9f9c9 100644 --- a/crates/pty-conformance/tests/attach_no_restart.rs +++ b/crates/pty-conformance/tests/attach_no_restart.rs @@ -4,6 +4,8 @@ //! default policy prompts `Restart? [Y/n]` and `--auto-restart` re-launches //! without asking. The terminal cases run the CLI inside a real pty //! (`Rig::pty_tty_raw`) with the same 30×100 geometry the Node test uses. +//! The cases after them have no terminal: there the prompt restarts only on a +//! `y`, which departs from Node (decision 0020). use pty_conformance::*; use serde_json::Value; @@ -213,3 +215,87 @@ fn auto_restart_relaunches_without_prompting() { assert_eq!(invocation_count(&marker), 2); assert_eq!(event_count(&rig, id, "session_start"), 1); } + +/// An attach that the prompt declined for want of an answer: exit 1, a way to +/// restart named, and the old record left in place for a later answer. +fn expect_unanswered(rig: &Rig, out: &Out, id: &str, marker: &Path) { + expect_status(out, 1); + expect_contains(&out.stdout(), "Restart? [Y/n]"); + expect_contains(&out.stderr(), &format!("Session \"{id}\" was not restarted")); + expect_contains(&out.stderr(), "pass -r"); + assert_eq!(invocation_count(marker), 1); + let kept = rig.meta(id).expect("the exited record is kept"); + assert_eq!(kept["exitCode"], 42, "{kept}"); +} + +/// Decision 0020: stdin at EOF answers nothing, so the stored command does +/// not run again. +#[test] +fn a_restart_prompt_at_eof_declines() { + let rig = Rig::new(); + let marker = rig.tmp().join("invocations"); + let id = "eof-target"; + spawn_retained_once(&rig, id, &marker); + + let out = rig.pty(&["attach", id]); + expect_unanswered(&rig, &out, id, &marker); +} + +/// Decision 0020: off a terminal, only `y` or `yes` restarts. An empty line, +/// which says yes at a terminal, does not. +#[test] +fn a_restart_prompt_off_a_terminal_needs_a_yes() { + let rig = Rig::new(); + let marker = rig.tmp().join("invocations"); + let id = "piped-target"; + spawn_retained_once(&rig, id, &marker); + + for input in [&b"\n"[..], b"future-relay-input\n"] { + let out = rig.pty_stdin(input, &["attach", id]); + expect_unanswered(&rig, &out, id, &marker); + } +} + +/// A piped `y` is an answer: the session restarts, as at a terminal. +#[test] +fn a_piped_yes_restarts() { + let rig = Rig::new(); + let marker = rig.tmp().join("invocations"); + let id = "yes-target"; + spawn_retained_once(&rig, id, &marker); + + let out = rig.pty_stdin(b"y\n", &["attach", id]); + expect_contains(&out.stdout(), &format!("Session \"{id}\" restarted.")); + wait_until("second invocation", || invocation_count(&marker) == 2); +} + +/// `-r` restarts without a terminal and without asking. +#[test] +fn auto_restart_needs_no_terminal() { + let rig = Rig::new(); + let marker = rig.tmp().join("invocations"); + let id = "auto-piped-target"; + spawn_retained_once(&rig, id, &marker); + + let out = rig.pty(&["attach", "-r", id]); + expect_not_contains(&out.stdout(), "Restart?"); + expect_contains(&out.stdout(), &format!("Session \"{id}\" restarted.")); + wait_until("second invocation", || invocation_count(&marker) == 2); +} + +/// `--force` only allows nesting. It answers nothing, and a `y` after it still +/// restarts. +#[test] +fn force_is_not_an_answer_to_the_restart_prompt() { + let rig = Rig::new(); + let marker = rig.tmp().join("invocations"); + let id = "force-target"; + spawn_retained_once(&rig, id, &marker); + + let out = rig.pty(&["attach", "--force", id]); + expect_unanswered(&rig, &out, id, &marker); + + let out = rig.pty_stdin(b"y\n", &["attach", "--force", id]); + expect_contains(&out.stdout(), &format!("Session \"{id}\" restarted.")); + wait_until("second invocation", || invocation_count(&marker) == 2); +} diff --git a/crates/pty/src/cli/ask.rs b/crates/pty/src/cli/ask.rs index 75ab4507..bac22253 100644 --- a/crates/pty/src/cli/ask.rs +++ b/crates/pty/src/cli/ask.rs @@ -1,38 +1,83 @@ -//! The `[Y/n]` prompt: write the question to stdout, read one line from -//! stdin. No tty check, exactly like Node's `readline` prompt; a closed -//! stdin reads as an empty answer, which proceeds. +//! The `[Y/n]` prompts: write the question to stdout, read one line from +//! stdin. +//! +//! [`ask`] has no tty check, like Node's `readline` prompt, and only a lone +//! `n` declines. A closed stdin reads as an empty answer, which proceeds. +//! Node differs there: its question never resolves at EOF, and the process +//! exits 0 having done nothing. `pty restart` still asks this way. +//! +//! [`ask_consent`] asks for the dead-session restart in `attach`, where only an +//! answer someone gave counts (decision 0020). //! //! node: src/cli.ts:4069-4078 (`ask`), 3922-3926 and 1836-1840 (callers). #![allow(dead_code)] // callers: the restart / dead-session prompts -use std::io::{BufRead, Write}; +use std::io::{BufRead, IsTerminal, Write}; /// Print `prompt` (no newline) and return the answer line without its /// line terminator. pub fn ask(prompt: &str) -> String { + read_answer(prompt).unwrap_or_default() +} + +/// Did the answer decline? Node compares `answer.toLowerCase() === "n"` +/// (the restart prompts) — only a lone `n`/`N` says no; anything else, +/// including an empty line, proceeds. +pub fn declined(answer: &str) -> bool { + answer.to_lowercase() == "n" +} + +/// What the dead-session restart prompt was told. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Consent { + Yes, + /// A lone `n`. + No, + /// Stdin was at EOF, or is not a terminal and did not say `y` or `yes`. + NoAnswer, +} + +/// Print `prompt` and read one line, like [`ask`], but count only an answer. +/// At a terminal, as in Node, anything but a lone `n` says yes, an empty line +/// included. Anywhere else only `y` or `yes` does. EOF is never an answer. +pub fn ask_consent(prompt: &str) -> Consent { + let answer = read_answer(prompt); + consent(answer.as_deref(), std::io::stdin().is_terminal()) +} + +fn consent(answer: Option<&str>, terminal: bool) -> Consent { + match answer { + None => Consent::NoAnswer, + Some(answer) if declined(answer) => Consent::No, + Some(_) if terminal => Consent::Yes, + Some(answer) if answer.eq_ignore_ascii_case("y") || answer.eq_ignore_ascii_case("yes") => { + Consent::Yes + } + Some(_) => Consent::NoAnswer, + } +} + +/// Print `prompt` and read one line. `None` at EOF or on a read error. +fn read_answer(prompt: &str) -> Option { let mut out = std::io::stdout().lock(); let _ = out.write_all(prompt.as_bytes()); let _ = out.flush(); drop(out); let mut line = String::new(); - let _ = std::io::stdin().lock().read_line(&mut line); + match std::io::stdin().lock().read_line(&mut line) { + Ok(0) | Err(_) => return None, + Ok(_) => {} + } while line.ends_with('\n') || line.ends_with('\r') { line.pop(); } - line -} - -/// Did the answer decline? Node compares `answer.toLowerCase() === "n"` -/// (the restart prompts) — only a lone `n`/`N` says no; anything else, -/// including an empty line, proceeds. -pub fn declined(answer: &str) -> bool { - answer.to_lowercase() == "n" + Some(line) } #[cfg(test)] mod tests { - use super::declined; + use super::{Consent, consent, declined}; #[test] fn only_a_lone_n_declines() { @@ -42,4 +87,41 @@ mod tests { assert!(!declined("y")); assert!(!declined("no")); } + + #[test] + fn consent_needs_an_answer_and_off_a_terminal_a_yes() { + for terminal in [true, false] { + assert_eq!( + consent(None, terminal), + Consent::NoAnswer, + "terminal={terminal}" + ); + for no in ["n", "N"] { + assert_eq!( + consent(Some(no), terminal), + Consent::No, + "{no:?} terminal={terminal}" + ); + } + for yes in ["y", "Y", "yes", "YES"] { + assert_eq!( + consent(Some(yes), terminal), + Consent::Yes, + "{yes:?} terminal={terminal}" + ); + } + } + for other in ["", "no", "future-relay-input"] { + assert_eq!( + consent(Some(other), true), + Consent::Yes, + "{other:?} at a terminal" + ); + assert_eq!( + consent(Some(other), false), + Consent::NoAnswer, + "{other:?} off a terminal" + ); + } + } } diff --git a/crates/pty/src/cli/attach.rs b/crates/pty/src/cli/attach.rs index 0cbd3186..167a0c8c 100644 --- a/crates/pty/src/cli/attach.rs +++ b/crates/pty/src/cli/attach.rs @@ -228,8 +228,21 @@ fn handle_dead_session(name: &str, meta: Option, auto_restart: println!("Command was: {command}"); println!(); - if !auto_restart && super::ask::declined(&ask::ask("Restart? [Y/n] ")) { - return Ok(0); + if !auto_restart { + match ask::ask_consent("Restart? [Y/n] ") { + ask::Consent::Yes => {} + ask::Consent::No => return Ok(0), + // Nobody said yes, so the stored command does not run again + // (decision 0020). + ask::Consent::NoAnswer => { + println!(); + eprintln!( + "pty attach: Session \"{name}\" was not restarted: the restart prompt got no answer." + ); + eprintln!(" Answer y, or pass -r to restart it without asking."); + return Ok(1); + } + } } // An exited session's daemon can still be finishing, and it needs the diff --git a/docs/decisions/0020-an-unanswered-restart-prompt-does-not-restart.md b/docs/decisions/0020-an-unanswered-restart-prompt-does-not-restart.md new file mode 100644 index 00000000..7e9cd68e --- /dev/null +++ b/docs/decisions/0020-an-unanswered-restart-prompt-does-not-restart.md @@ -0,0 +1,78 @@ +# 0020 — An unanswered restart prompt does not restart + +**Status:** accepted + +**Node behavior.** `handleDeadSession` (`src/cli.ts`) asks `Restart? [Y/n] ` +through `readline/promises`, with no terminal check. Any line but a lone `n` +restarts the session, an empty line included. When stdin reaches EOF first, +the question never resolves: the process exits 0 after the prompt, and +nothing restarts. + +Measured with no terminal, against the pinned Node reference on Node 22.23.2 +and 24.18.0, and against Rust before this change: + +| stdin | Node | Rust before | +| --- | --- | --- | +| `" was not restarted: the restart prompt got no answer. + Answer y, or pass -r to restart it without asking. + ``` +- A lone `n` still exits 0. `-r` still restarts without asking. `--force` + still only allows attaching from inside another session, and it answers + nothing. + +**Why.** Agents and scripts call `attach` without a terminal (#214). A prompt +that nobody answered is not consent to run a stored command again, and a +rerun repeats whatever the command does. Off a terminal, an empty line or +relayed input is no more a person's answer than EOF is, so only `y` counts +there. Node already restarts nothing at EOF, but it exits 0, the same as a +declined prompt. Exit 1, with the flag that restarts, tells the caller what +happened and what to do. + +**Client effect.** +- A script or seat that relied on `attach` restarting a dead session without a + terminal must pass `-r` now, or pipe `y`. +- At EOF, `attach` exits 1 where Node exits 0. Neither restarts. +- Off a terminal, any line but `y`, `yes` or `n` exits 1 where Node restarts. +- At a terminal nothing changes, except that EOF (Ctrl+D) exits 1. + +**Not changed.** `pty restart` asks `Kill and restart? [Y/n]` through the old +`ask`, so at EOF it still stops a running session and starts it again. Node +exits 0 there and leaves the session running. That is a parity defect outside +this decision and is left for a separate change. + +**Test.** `crates/pty-conformance/tests/attach_no_restart.rs`: +- `a_restart_prompt_at_eof_declines`, + `a_restart_prompt_off_a_terminal_needs_a_yes` and + `force_is_not_an_answer_to_the_restart_prompt` fail on Node. + `crates/pty-conformance/divergences.toml` records all three as intended. +- `a_piped_yes_restarts` and `auto_restart_needs_no_terminal` pass on both. +- The terminal cases before them are unchanged and pass on both. + +The unit test `consent_needs_an_answer_and_off_a_terminal_a_yes` in +`crates/pty/src/cli/ask.rs` covers each answer at a terminal and off one. + +**Migration / negotiation.** Pass `-r` to restart a dead session without a +terminal, or `--no-restart` never to restart one. Release note: `pty attach` +no longer restarts a dead session when the restart prompt gets no answer. +That covers stdin at EOF, and stdin that is not a terminal unless it gives `y` +or `yes`. It exits 1 instead, without running the stored command. Any +script or seat that relied on `attach` restarting a dead session +non-interactively must pass `-r` now. diff --git a/docs/parity.md b/docs/parity.md index 2a891d9b..c2cb44f8 100644 --- a/docs/parity.md +++ b/docs/parity.md @@ -381,6 +381,7 @@ Decided on 2026-08-29. Each row records the decision. | `up`/`down` and `pty.toml` | S | Kept. Already ported; the binding rule needs the tag pair. | | `queryStats` waiting out its 2 s timeout on a daemon that closes without STATUS, and `peek -f` hanging on a plain close | S | Not reproduced, on purpose. Both end promptly instead. Deliberate improvements, not gaps — accepted, decision 0006. | | `peek --wait` missing a pattern that scrolls off between two 200 ms screen reads | S | Not reproduced, on purpose. The wait also watches the output stream, and a timeout says when the pattern is in the scrollback from before the wait. Accepted, decision 0019. | +| `attach`'s dead-session prompt restarting with no terminal on any line but `n` | S | Not reproduced, on purpose. Off a terminal only `y` or `yes` restarts, and EOF never does. With no answer, attach exits 1 and names `-r`, where Node exits 0 at EOF and restarts on any other line. Accepted, decision 0020. | ## 12c. What differs because the kernel differs