mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-22 00:02:23 +00:00
fix(pane): stop reporting a killed command as an honest exit 1
`portable-pty` builds a signalled child's status as `code: 1` — `std::process::ExitStatus::code()` is `None` for one on Unix and its `From` impl falls back to 1 — and keeps the signal in a private field with no accessor. So `exit_code()` returns 1 and `success()` returns false for a command killed by SIGKILL *and* for one that exited 1 on its own, identically. The probe passed that straight through, which reached the CLI as `exit_code_known: true` — the field `docs/cli/reference.mdx` describes as "how you tell a real 1 from a stand-in". An agent reading it was told a killed command had chosen to exit 1. Signalled now reports unknown, which the CLI already renders as an exit of 1 *with* `exit_code_known: false` and a line on stderr. The exit code a caller sees does not change; what changes is that it is no longer claimed as the child's own. Not the `128 + n` an adopted pane gets: that path calls `waitpid` itself and has the number. Here the only thing left is a name from `strsignal`, which is localized, so mapping it back would be a guess dressed as a fact — and a wrong 137 is worse than an honest "unknown". Sniffing `Display` is the only discrimination the crate's public API offers. The test builds both cases from the crate's own constructors and asserts the exact prefix, so an upstream rewording fails there, beside the reasoning, instead of quietly turning every signalled pane back into an exit of 1. It also pins `killed.exit_code() == 1`, which is the trap.
This commit is contained in:
@@ -661,6 +661,34 @@ pub(crate) const OBSERVER_BUDGET: i64 = 8 * 1024 * 1024;
|
||||
/// is a pane that looks frozen to every attached client, so it stays short.
|
||||
const EXIT_CODE_PROBE_WINDOW: Duration = Duration::from_millis(500);
|
||||
|
||||
/// What a finished child's status is worth reporting as, or `None` when the
|
||||
/// honest answer is that we do not know.
|
||||
///
|
||||
/// `portable-pty` stores a signalled child as `code: 1` — `ExitStatus::code()`
|
||||
/// is `None` on Unix for one, and its `From` impl falls back to 1 — while
|
||||
/// keeping the signal in a private field with no accessor. `success()` and
|
||||
/// `exit_code()` therefore read *identically* for a child killed by SIGKILL
|
||||
/// and one that exited 1 of its own accord, and only `Display` tells them
|
||||
/// apart. Passing that 1 along would land in the CLI's `exit_code_known: true`,
|
||||
/// which is documented as the thing that "tells a real 1 from a stand-in".
|
||||
///
|
||||
/// So: signalled reports unknown, which the CLI already renders as an exit of
|
||||
/// 1 *with* `exit_code_known: false` and a line on stderr. Not the `128 + n`
|
||||
/// an adopted pane gets from its own `waitpid` — the name here comes from
|
||||
/// `strsignal`, which is localized, so mapping it back to a number is a guess
|
||||
/// dressed as a fact.
|
||||
///
|
||||
/// Sniffing `Display` is the only discrimination the crate's public API
|
||||
/// offers; `signalled_status_is_not_reported_as_a_plain_exit` pins it against
|
||||
/// the crate's own constructor so an upstream rewording fails there rather
|
||||
/// than here.
|
||||
fn reported_exit_code(status: &portable_pty::ExitStatus) -> Option<i32> {
|
||||
match status.to_string().starts_with("Terminated by") {
|
||||
true => None,
|
||||
false => Some(status.exit_code() as i32),
|
||||
}
|
||||
}
|
||||
|
||||
const EXIT_CODE_PROBE_INTERVAL: Duration = Duration::from_millis(10);
|
||||
|
||||
struct Observer {
|
||||
@@ -1401,7 +1429,7 @@ impl DaemonPane {
|
||||
// below is the only thing clients should ever wait on.
|
||||
if let Ok(mut child) = child.try_lock() {
|
||||
match child.try_wait() {
|
||||
Ok(Some(status)) => return Some(status.exit_code() as i32),
|
||||
Ok(Some(status)) => return reported_exit_code(&status),
|
||||
Ok(None) => {}
|
||||
Err(_) => return None,
|
||||
}
|
||||
@@ -4533,6 +4561,42 @@ mod tests {
|
||||
assert!(rx.try_recv().is_err());
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn signalled_status_is_not_reported_as_a_plain_exit() {
|
||||
use portable_pty::ExitStatus;
|
||||
|
||||
// An ordinary exit keeps its code, whatever the code is.
|
||||
assert_eq!(reported_exit_code(&ExitStatus::with_exit_code(0)), Some(0));
|
||||
assert_eq!(reported_exit_code(&ExitStatus::with_exit_code(1)), Some(1));
|
||||
assert_eq!(
|
||||
reported_exit_code(&ExitStatus::with_exit_code(137)),
|
||||
Some(137)
|
||||
);
|
||||
|
||||
// A signalled one reads as `code: 1` through the whole public API —
|
||||
// `exit_code()` says 1 and `success()` says false, exactly as they do
|
||||
// for the honest exit above — so reporting it as a code would be
|
||||
// reporting a number the child never chose.
|
||||
let killed = ExitStatus::with_signal("Killed");
|
||||
assert_eq!(killed.exit_code(), 1, "the trap this guards");
|
||||
assert_eq!(reported_exit_code(&killed), None);
|
||||
|
||||
// The discrimination is `Display`, which is the only thing the crate
|
||||
// exposes that differs. Built from the crate's own constructor so an
|
||||
// upstream rewording fails here, next to the reason, rather than
|
||||
// silently turning every signalled pane back into an exit of 1.
|
||||
assert!(
|
||||
killed.to_string().starts_with("Terminated by"),
|
||||
"portable-pty changed how it renders a signalled status: {killed}"
|
||||
);
|
||||
assert!(
|
||||
!ExitStatus::with_exit_code(1)
|
||||
.to_string()
|
||||
.starts_with("Terminated by"),
|
||||
"an ordinary exit must not read as signalled"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn probed_cwd_absent_leaves_the_reported_cwd_alone() {
|
||||
let mut st = test_state(true);
|
||||
|
||||
Reference in New Issue
Block a user