Skip to content

Commit 495d84f

Browse files
schicklingclaude
andcommitted
fix(codex): stoppable wrapper, faithful terminal exits, and discontinuity marking
Review-pass fixes: the wrapper installs the shared stop handler and the monitor observes it, ending the session through the ordinary terminal-write path (previously st2's own stop SIGTERMed the wrapper dead before the post-join write); the terminal record's exit carries the actually-observed ExitStatus on every arm, error text stays diagnostic in reason; evidence loss interrupts the writer so a state restated across an unproven interval opens a fresh transition; and ptySession records the wrapper's runtime ID, which only aliases the identity on driver-expanded seats. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 7c8c065 commit 495d84f

1 file changed

Lines changed: 95 additions & 16 deletions

File tree

src/codex_app_server.rs

Lines changed: 95 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ use std::os::unix::fs::{FileTypeExt as _, OpenOptionsExt as _, PermissionsExt as
1818
use std::os::unix::io::AsRawFd as _;
1919
use std::os::unix::net::UnixStream;
2020
use std::os::unix::process::CommandExt as _;
21+
use std::os::unix::process::ExitStatusExt as _;
2122
use std::path::{Path, PathBuf};
2223
use std::process::{Child, Command, ExitStatus, Stdio};
2324
use std::sync::mpsc::{self, Receiver, Sender};
@@ -403,12 +404,14 @@ impl CodexInboxDelivery {
403404
// refreshes, harness-state transitions) into the same agent dir, and those must not wake it.
404405
let watcher = crate::watch::watch_delivery_inputs(&config.agent_dir, wake_tx);
405406
let state = load_delivery_state(&state_path, &config.identity, runtime.runtime_id())?;
406-
// An agent IS its pty: the session whose liveness vouches for the record is the identity.
407+
// The pty session whose liveness vouches for the record is the wrapper's task: the
408+
// runtime ID names the pty registry entry, and only aliases the identity on
409+
// driver-expanded seats — a hand-authored seat may declare a different task ID.
407410
let harness_writer = harness_state::Writer::new(
408411
&config.agent_dir,
409412
config.identity.clone(),
410413
"codex",
411-
Some(config.identity.clone()),
414+
Some(runtime.runtime_id().to_string()),
412415
);
413416
Ok(Self {
414417
config,
@@ -437,7 +440,13 @@ impl CodexInboxDelivery {
437440
let _ = self.harness_writer.observe(observation);
438441
self.harness_evidence = true;
439442
}
440-
None => self.harness_evidence = false,
443+
None => {
444+
// Evidence lost: stop heartbeating, and mark the stream discontinuous so a state
445+
// restated after the gap opens a fresh transition instead of claiming continuity
446+
// across an interval nothing observed.
447+
self.harness_evidence = false;
448+
self.harness_writer.interrupt();
449+
}
441450
}
442451
}
443452

@@ -1285,6 +1294,10 @@ fn run_connected(
12851294
delivery: CodexDeliveryConfig,
12861295
diagnostics: &mut WrapperDiagnostics,
12871296
) -> Result<()> {
1297+
// st2's own stop path SIGTERMs this wrapper. Without a handler the wrapper dies before the
1298+
// post-join terminal write below, so a stopped seat would read its last live state until the
1299+
// staleness horizon — the exact window the Claude wrapper's ordering closes.
1300+
crate::provider_session::install_signal_handler();
12881301
let state_dir = state_dir(&delivery.catalog_root, &delivery.identity);
12891302
let endpoint = format!("unix://{}", socket_path.display());
12901303
let tui_args = controlled_tui_args(&endpoint, &codex_argv[1..], resume_thread)?;
@@ -1350,7 +1363,7 @@ fn run_connected(
13501363
.with_context(|| format!("starting controlled {} TUI", codex_argv[0]));
13511364
}
13521365
};
1353-
let result = (|| -> Result<()> {
1366+
let result = (|| -> Result<TuiEnd> {
13541367
diagnostics.record("tuiStarted", json!({ "pid": tui.id() }))?;
13551368
if let Some(ready) = resume_ready_tx.take() {
13561369
ready
@@ -1371,24 +1384,51 @@ fn run_connected(
13711384
let _ = event_thread.join();
13721385
// The pump is gone, so nothing can observe this session again: publish the terminal
13731386
// observation with the outcome the wrapper actually saw, before any staleness horizon.
1387+
// Consumers must not branch on `reason`, so the observed exit always lands in `exit`.
13741388
let mut harness_writer = harness_state::Writer::new(
13751389
&harness_agent_dir,
13761390
harness_identity.clone(),
13771391
"codex",
1378-
Some(harness_identity),
1392+
Some(runtime.runtime_id().to_string()),
13791393
);
13801394
let _ = match &result {
1381-
Ok(()) => harness_writer.ended("exit 0"),
1382-
Err(error) => harness_writer.observe(
1383-
harness_state::Observation::new(
1384-
harness_state::Activity::Ended,
1385-
harness_state::BlockedOn::None,
1386-
harness_state::InputBuffer::Unknown,
1395+
Ok(TuiEnd::Exited(status)) => harness_writer.ended(describe_tui_exit(Some(*status))),
1396+
Ok(TuiEnd::Stopped(status)) => harness_writer.ended(describe_tui_exit(*status)),
1397+
Err(error) => {
1398+
let observed_exit = tui.try_wait().ok().flatten();
1399+
harness_writer.observe(
1400+
harness_state::Observation::new(
1401+
harness_state::Activity::Ended,
1402+
harness_state::BlockedOn::None,
1403+
harness_state::InputBuffer::Unknown,
1404+
)
1405+
.with_exit(describe_tui_exit(observed_exit))
1406+
.with_reason(format!("{error}")),
13871407
)
1388-
.with_reason(format!("{error}")),
1389-
),
1408+
}
13901409
};
1391-
result
1410+
match result {
1411+
Ok(TuiEnd::Exited(status)) => completed_tui(status),
1412+
// The wrapper stopped its own session: not a failure, mirroring the shared wrapper body.
1413+
Ok(TuiEnd::Stopped(_)) => Ok(()),
1414+
Err(error) => Err(error),
1415+
}
1416+
}
1417+
1418+
/// How the controlled TUI session came to an end, as the monitor saw it.
1419+
enum TuiEnd {
1420+
/// The TUI exited on its own with this status.
1421+
Exited(ExitStatus),
1422+
/// The wrapper's stop flag ended the session; the reaped status when one was observable.
1423+
Stopped(Option<ExitStatus>),
1424+
}
1425+
1426+
fn describe_tui_exit(status: Option<ExitStatus>) -> String {
1427+
match status.map(|status| (status.code(), status.signal())) {
1428+
Some((Some(code), _)) => format!("exit {code}"),
1429+
Some((None, Some(signal))) => format!("signal {signal}"),
1430+
_ => "exit unknown".to_string(),
1431+
}
13921432
}
13931433

13941434
/// Start app-server with the authored global configuration inputs that its CLI supports.
@@ -2321,10 +2361,16 @@ fn wait_for_binding(
23212361
}
23222362
}
23232363

2324-
fn monitor_bound_tui(tui: &mut Child, events: &Receiver<ControlEvent>) -> Result<()> {
2364+
fn monitor_bound_tui(tui: &mut Child, events: &Receiver<ControlEvent>) -> Result<TuiEnd> {
23252365
loop {
2366+
if crate::provider_session::STOP.load(std::sync::atomic::Ordering::SeqCst) {
2367+
// st2's stop path: end the session and return through the ordinary terminal-write
2368+
// path so the record carries the observed outcome before the wrapper exits.
2369+
terminate_child(tui);
2370+
return Ok(TuiEnd::Stopped(tui.try_wait().ok().flatten()));
2371+
}
23262372
if let Some(status) = tui.try_wait()? {
2327-
return completed_tui(status);
2373+
return Ok(TuiEnd::Exited(status));
23282374
}
23292375
match events.recv_timeout(CONTROL_POLL) {
23302376
Ok(ControlEvent::TuiThreadLoaded(acknowledge)) => {
@@ -2915,6 +2961,39 @@ mod tests {
29152961
);
29162962
}
29172963

2964+
#[test]
2965+
fn evidence_loss_marks_the_stream_discontinuous_for_a_restated_state() {
2966+
use crate::harness_state::{self, Activity};
2967+
let tmp = tempfile::tempdir().unwrap();
2968+
let config = delivery_config(tmp.path());
2969+
let record_path = harness_state::harness_state_path(&config.agent_dir);
2970+
let mut delivery = inbox_delivery(tmp.path(), config);
2971+
2972+
delivery.observe_harness(&CodexObservedState::Active {
2973+
turn_id: "turn-a".into(),
2974+
});
2975+
let before = fs::read(&record_path).unwrap();
2976+
2977+
// The same tuple restated across an unproven interval must not coalesce into the
2978+
// pre-gap record — continuity was not observed, so a fresh transition opens.
2979+
delivery.observe_harness(&CodexObservedState::Held {
2980+
reason: CodexHoldReason::SystemError,
2981+
turn_id: None,
2982+
});
2983+
delivery.observe_harness(&CodexObservedState::Active {
2984+
turn_id: "turn-a".into(),
2985+
});
2986+
assert_ne!(
2987+
fs::read(&record_path).unwrap(),
2988+
before,
2989+
"a restated state after an evidence gap must open a fresh transition"
2990+
);
2991+
assert_eq!(
2992+
harness_state::read(&record_path, None).unwrap().state,
2993+
Activity::Active
2994+
);
2995+
}
2996+
29182997
#[test]
29192998
fn delivery_client_id_is_stable_and_binds_every_identity_component() {
29202999
let id =

0 commit comments

Comments
 (0)