diff --git a/README.md b/README.md index ef6f2a2..0ea5acf 100644 --- a/README.md +++ b/README.md @@ -160,6 +160,7 @@ pilotty snapshot --format text # Plain text with cursor indicator # Wait for screen to change before returning (no more manual sleep!) pilotty snapshot --await-change $HASH # Block until hash differs pilotty snapshot --await-change $HASH --settle 100 # Then wait for stability +pilotty snapshot --settle 100 --strict # Fail on deadline or session exit ``` ### Input @@ -207,6 +208,7 @@ The `snapshot` command returns structured data about the terminal screen: ```json { + "outcome": "immediate", "snapshot_id": 42, "size": { "cols": 80, "rows": 24 }, "cursor": { "row": 5, "col": 10, "visible": true }, @@ -221,6 +223,11 @@ The `snapshot` command returns structured data about the terminal screen: } ``` +JSON snapshots always include an `outcome`: `immediate`, `changed`, `settled`, +`deadline`, or `exited`. Deadline and exit outcomes return the latest available screen evidence +instead of replacing it with an error. Exited captures include process exit metadata and +whether the final output was completely drained. + ## UI Elements (Contextual) pilotty automatically detects interactive UI elements in terminal applications. Elements provide **read-only context** to help understand UI structure, with position data (row, col) for use with the click command. @@ -272,6 +279,12 @@ pilotty snapshot --await-change $HASH --settle 100 # Wait 100ms after last chan - `--await-change `: Block until `content_hash` differs from this value - `--settle `: After change detected, wait for screen to be stable for this many ms - `-t, --timeout `: Maximum wait time (default: 30000) +- `--strict`: Preserve printed evidence but exit 3 on deadline or 4 on session exit + +Without `--strict`, every capture outcome exits 0. CLI exit categories are: 0 +success, 1 generic/API error, 2 command-line usage, 3 timing deadline, and 4 session +lifecycle. Commands that require a live process also exit 4 when they receive +`SESSION_EXITED`. **Why this matters:** - No more flaky automation due to race conditions diff --git a/crates/pilotty-cli/src/args.rs b/crates/pilotty-cli/src/args.rs index 6c06362..7788be2 100644 --- a/crates/pilotty-cli/src/args.rs +++ b/crates/pilotty-cli/src/args.rs @@ -43,7 +43,8 @@ Wait for change: HASH=$(pilotty snapshot | jq -r '.content_hash') pilotty key Enter pilotty snapshot --await-change $HASH # Block until screen changes - pilotty snapshot --await-change $HASH --settle 100 # Wait for 100ms stability")] + pilotty snapshot --await-change $HASH --settle 100 # Wait for 100ms stability + pilotty snapshot --settle 100 --strict # Exit nonzero on deadline/exit")] Snapshot(SnapshotArgs), /// Type text at the current cursor position @@ -192,6 +193,10 @@ pub struct SnapshotArgs { /// Total timeout in milliseconds for await-change and settle combined (default: 30s) #[arg(short, long, default_value_t = 30000)] pub timeout: u64, + + /// Exit 3 on deadline or 4 on session exit, after printing capture evidence + #[arg(long)] + pub strict: bool, } #[derive(Debug, Clone, Copy, ValueEnum)] @@ -319,9 +324,10 @@ pilotty list-sessions #[cfg(test)] mod tests { - use super::{Cli, Commands}; use clap::Parser; + use crate::args::{Cli, Commands}; + #[test] fn test_spawn_parses_hyphenated_args() { let cli = Cli::parse_from(["pilotty", "spawn", "bash", "-c", "echo hello"]); @@ -353,4 +359,14 @@ mod tests { _ => panic!("Expected logs command"), } } + + #[test] + fn snapshot_parses_strict_mode() { + let cli = Cli::parse_from(["pilotty", "snapshot", "--settle", "100", "--strict"]); + + match cli.command { + Commands::Snapshot(args) => assert!(args.strict), + _ => panic!("Expected snapshot command"), + } + } } diff --git a/crates/pilotty-cli/src/daemon/server.rs b/crates/pilotty-cli/src/daemon/server.rs index 0f39fd6..b32a728 100644 --- a/crates/pilotty-cli/src/daemon/server.rs +++ b/crates/pilotty-cli/src/daemon/server.rs @@ -8,7 +8,8 @@ use anyhow::{Context, Result}; use pilotty_core::error::ApiError; use pilotty_core::input::encode_mouse_click_combined; use pilotty_core::protocol::{ - supports_protocol, Command, Request, Response, ResponseData, SnapshotFormat, + supports_protocol, CaptureExit, CaptureOutcome, Command, Request, Response, ResponseData, + ScreenCapture, SnapshotFormat, }; use pilotty_core::snapshot::{CursorState, ScreenState, TerminalSize}; use tokio::io::{AsyncWriteExt, BufReader}; @@ -20,8 +21,10 @@ use tracing::{debug, error, info, warn}; use crate::daemon::paths; use crate::daemon::pty::TermSize; use crate::daemon::retention::DEFAULT_RETAIN_BYTES; -use crate::daemon::session::{ObservationEvent, SessionEvidence, SessionId, SessionManager}; -use crate::daemon::tombstone::Tombstone; +use crate::daemon::session::{ + ObservationEvent, SessionEvidence, SessionId, SessionManager, SessionObserver, SnapshotData, +}; +use crate::daemon::tombstone::{ExitMetadata, Tombstone}; const RETAIN_BYTES_ENV: &str = "PILOTTY_RETAIN_BYTES"; @@ -575,11 +578,13 @@ async fn handle_request( handle_snapshot( &request_id, &sessions, - session, - format, - await_change, - settle_ms, - timeout_ms, + SnapshotRequestOptions { + session, + format, + await_change, + settle_ms, + timeout_ms, + }, ) .await } @@ -756,27 +761,30 @@ async fn handle_status( } } -/// Minimum useful settle window retained for CLI compatibility. -const MIN_SETTLE_MS: u64 = 50; - -/// Handle snapshot command. -/// -/// Supports optional wait-for-change semantics: -/// - `await_change`: Block until content_hash differs from this value -/// - `settle_ms`: After change detected, wait for screen to be stable this long -/// - `timeout_ms`: Maximum time to wait for change/settle -async fn handle_snapshot( - request_id: &str, - sessions: &SessionManager, +/// Snapshot behavior requested over the wire. +struct SnapshotRequestOptions { session: Option, - format: Option, + format: SnapshotFormat, await_change: Option, settle_ms: u64, timeout_ms: u64, +} + +/// Handle immediate, wait-for-change, and settle captures. +async fn handle_snapshot( + request_id: &str, + sessions: &SessionManager, + options: SnapshotRequestOptions, ) -> Response { use std::time::{Duration, Instant}; - let format = format.unwrap_or(SnapshotFormat::Full); + let SnapshotRequestOptions { + session, + format, + await_change, + settle_ms, + timeout_ms, + } = options; let evidence = match sessions.resolve_evidence(session.as_deref()).await { Ok(evidence) => evidence, Err(e) => return Response::error(request_id, e), @@ -788,35 +796,62 @@ async fn handle_snapshot( } }; - let with_elements = matches!(format, SnapshotFormat::Full); let timeout = Duration::from_millis(timeout_ms); - // Retain the shipped minimum settle window. - let settle = Duration::from_millis(if settle_ms > 0 { - settle_ms.max(MIN_SETTLE_MS) - } else { - 0 - }); + let settle = Duration::from_millis(settle_ms); let start = Instant::now(); let mut observer = match sessions.observe_session(&session_id).await { Ok(observer) => observer, - Err(error) => return Response::error(request_id, error), + Err(error) => { + if let Some(response) = + finalized_snapshot_response(request_id, sessions, &session_id, format).await + { + return response; + } + return Response::error(request_id, error); + } }; + let mut output_closed = false; // Phase 1: If await_change is set, wait until content_hash differs if let Some(baseline_hash) = await_change { loop { + if output_closed { + match exited_live_snapshot_response( + request_id, + sessions, + &session_id, + &mut observer, + format, + ) + .await + { + Ok(Some(response)) => return response, + Ok(None) if start.elapsed() < timeout => { + let remaining = timeout.saturating_sub(start.elapsed()); + tokio::time::sleep(remaining.min(Duration::from_millis(25))).await; + continue; + } + Ok(None) => {} + Err(response) => return response, + } + } + if start.elapsed() >= timeout { - return Response::error( + return snapshot_deadline_response( request_id, - ApiError::command_failed_with_suggestion( - format!( + sessions, + &session_id, + &mut observer, + SnapshotDeadline { + format, + output_closed, + note: format!( "Timeout after {}ms waiting for screen to change from hash {}", - timeout_ms, - baseline_hash + timeout_ms, baseline_hash ), - "Screen content did not change. The application may be idle or waiting for input.", - ), - ); + }, + ) + .await; } let snapshot = observer.current(false).await; @@ -835,10 +870,15 @@ async fn handle_snapshot( match observer.wait_for_update(remaining).await { ObservationEvent::Updated => {} ObservationEvent::OutputClosed => { - tokio::time::sleep(remaining).await; + output_closed = true; } ObservationEvent::Deadline => {} ObservationEvent::PumpFailed => { + if let Some(response) = + finalized_snapshot_response(request_id, sessions, &session_id, format).await + { + return response; + } return pump_failure_response(request_id); } } @@ -853,19 +893,43 @@ async fn handle_snapshot( let mut stable_since = Instant::now(); loop { + if output_closed { + match exited_live_snapshot_response( + request_id, + sessions, + &session_id, + &mut observer, + format, + ) + .await + { + Ok(Some(response)) => return response, + Ok(None) if start.elapsed() < timeout => { + let remaining = timeout.saturating_sub(start.elapsed()); + tokio::time::sleep(remaining.min(Duration::from_millis(25))).await; + continue; + } + Ok(None) => {} + Err(response) => return response, + } + } + if start.elapsed() >= timeout { - return Response::error( + return snapshot_deadline_response( request_id, - ApiError::command_failed_with_suggestion( - format!( + sessions, + &session_id, + &mut observer, + SnapshotDeadline { + format, + output_closed, + note: format!( "Timeout after {}ms waiting for screen to stabilize for {}ms (last hash: {})", - timeout_ms, - settle_ms, - last_hash + timeout_ms, settle_ms, last_hash ), - "Screen kept changing. Try increasing --timeout or --settle.", - ), - ); + }, + ) + .await; } if stable_since.elapsed() >= settle { @@ -888,21 +952,74 @@ async fn handle_snapshot( stable_since = Instant::now(); } } - ObservationEvent::OutputClosed => tokio::time::sleep(wait).await, + ObservationEvent::OutputClosed => { + output_closed = true; + } ObservationEvent::Deadline => {} ObservationEvent::PumpFailed => { + if let Some(response) = + finalized_snapshot_response(request_id, sessions, &session_id, format).await + { + return response; + } return pump_failure_response(request_id); } } } } - // Phase 3: Take final snapshot with requested format + let outcome = if settle_ms > 0 { + CaptureOutcome::Settled + } else if await_change.is_some() { + CaptureOutcome::Changed + } else { + CaptureOutcome::Immediate + }; + live_snapshot_response( + request_id, + sessions, + &session_id, + &mut observer, + format, + CaptureDetails { + outcome, + exit: None, + note: None, + }, + ) + .await +} + +struct CaptureDetails { + outcome: CaptureOutcome, + exit: Option, + note: Option, +} + +async fn live_snapshot_response( + request_id: &str, + sessions: &SessionManager, + session_id: &SessionId, + observer: &mut SessionObserver, + format: SnapshotFormat, + details: CaptureDetails, +) -> Response { + let with_elements = matches!(format, SnapshotFormat::Full); let snapshot = observer.current(with_elements).await; debug!( "Captured session {} at revision {}", session_id, snapshot.revision ); + snapshot_response(request_id, sessions, snapshot, format, details) +} + +fn snapshot_response( + request_id: &str, + sessions: &SessionManager, + snapshot: SnapshotData, + format: SnapshotFormat, + details: CaptureDetails, +) -> Response { let (cursor_row, cursor_col) = snapshot.cursor_pos; match format { @@ -914,6 +1031,9 @@ async fn handle_snapshot( ResponseData::Snapshot { format: SnapshotFormat::Text, content: output, + outcome: details.outcome, + exit: details.exit, + note: details.note, }, ) } @@ -934,7 +1054,15 @@ async fn handle_snapshot( elements: snapshot.elements, content_hash: Some(snapshot.content_hash), }; - Response::success(request_id, ResponseData::ScreenState(screen_state)) + Response::success( + request_id, + ResponseData::ScreenState(ScreenCapture { + screen: screen_state, + outcome: details.outcome, + exit: details.exit, + note: details.note, + }), + ) } SnapshotFormat::Compact => { let snapshot_id = sessions.next_snapshot_id(); @@ -953,9 +1081,127 @@ async fn handle_snapshot( elements: None, content_hash: None, }; - Response::success(request_id, ResponseData::ScreenState(screen_state)) + Response::success( + request_id, + ResponseData::ScreenState(ScreenCapture { + screen: screen_state, + outcome: details.outcome, + exit: details.exit, + note: details.note, + }), + ) + } + } +} + +async fn snapshot_deadline_response( + request_id: &str, + sessions: &SessionManager, + session_id: &SessionId, + observer: &mut SessionObserver, + deadline: SnapshotDeadline, +) -> Response { + let SnapshotDeadline { + format, + output_closed, + note, + } = deadline; + + if let Some(response) = + finalized_snapshot_response(request_id, sessions, session_id, format).await + { + return response; + } + + if output_closed { + match exited_live_snapshot_response(request_id, sessions, session_id, observer, format) + .await + { + Ok(Some(response)) => return response, + Ok(None) => {} + Err(response) => return response, } } + + let note = if output_closed { + "PTY output closed, but the process is still running; EOF alone is not an exit.".to_string() + } else { + note + }; + live_snapshot_response( + request_id, + sessions, + session_id, + observer, + format, + CaptureDetails { + outcome: CaptureOutcome::Deadline, + exit: None, + note: Some(note), + }, + ) + .await +} + +struct SnapshotDeadline { + format: SnapshotFormat, + output_closed: bool, + note: String, +} + +async fn exited_live_snapshot_response( + request_id: &str, + sessions: &SessionManager, + session_id: &SessionId, + observer: &mut SessionObserver, + format: SnapshotFormat, +) -> Result, Response> { + let exit = observer + .exit_metadata() + .map_err(|error| Response::error(request_id, error))?; + let Some(exit) = exit else { + return Ok(None); + }; + + Ok(Some( + live_snapshot_response( + request_id, + sessions, + session_id, + observer, + format, + CaptureDetails { + outcome: CaptureOutcome::Exited, + exit: Some(capture_exit(exit, true)), + note: None, + }, + ) + .await, + )) +} + +async fn finalized_snapshot_response( + request_id: &str, + sessions: &SessionManager, + session_id: &SessionId, + format: SnapshotFormat, +) -> Option { + match sessions.resolve_evidence(Some(&session_id.0)).await { + Ok(SessionEvidence::Exited(tombstone)) => { + Some(exited_snapshot_response(request_id, *tombstone, format)) + } + Ok(SessionEvidence::Live(_)) | Err(_) => None, + } +} + +fn capture_exit(exit: ExitMetadata, output_complete: bool) -> CaptureExit { + CaptureExit { + exit_code: exit.code, + signal: exit.signal, + success: exit.success, + killed_by_client: exit.killed_by_client, + output_complete, + } } fn exited_snapshot_response( @@ -963,20 +1209,36 @@ fn exited_snapshot_response( tombstone: Tombstone, format: SnapshotFormat, ) -> Response { + let details = CaptureDetails { + outcome: CaptureOutcome::Exited, + exit: Some(capture_exit(tombstone.exit, tombstone.output_complete)), + note: None, + }; + match format { SnapshotFormat::Full => Response::success( request_id, - ResponseData::ScreenState(tombstone.final_screen), + ResponseData::ScreenState(ScreenCapture { + screen: tombstone.final_screen, + outcome: details.outcome, + exit: details.exit, + note: details.note, + }), ), SnapshotFormat::Compact => Response::success( request_id, - ResponseData::ScreenState(ScreenState { - snapshot_id: tombstone.final_screen.snapshot_id, - size: tombstone.final_screen.size, - cursor: tombstone.final_screen.cursor, - text: None, - elements: None, - content_hash: None, + ResponseData::ScreenState(ScreenCapture { + screen: ScreenState { + snapshot_id: tombstone.final_screen.snapshot_id, + size: tombstone.final_screen.size, + cursor: tombstone.final_screen.cursor, + text: None, + elements: None, + content_hash: None, + }, + outcome: details.outcome, + exit: details.exit, + note: details.note, }), ), SnapshotFormat::Text => { @@ -995,6 +1257,9 @@ fn exited_snapshot_response( ResponseData::Snapshot { format: SnapshotFormat::Text, content, + outcome: details.outcome, + exit: details.exit, + note: details.note, }, ) } @@ -1516,7 +1781,6 @@ async fn handle_shutdown( #[cfg(test)] mod tests { - use super::*; use pilotty_core::error::ErrorCode; use pilotty_core::protocol::{Command, SessionStatus, PROTOCOL_VERSION}; use std::time::Duration; @@ -1526,6 +1790,8 @@ mod tests { use tokio::time::timeout; use uuid::Uuid; + use crate::daemon::server::*; + async fn socket_request( reader: &mut BufReader, writer: &mut OwnedWriteHalf, @@ -1749,7 +2015,7 @@ mod tests { "snapshot", Command::Snapshot { session: Some("finalized".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: None, settle_ms: 0, timeout_ms: 1000, @@ -1761,8 +2027,12 @@ mod tests { .await; assert!(matches!( snapshot.data, - Some(ResponseData::ScreenState(ScreenState { text: Some(text), .. })) - if text.contains("recovered-evidence") + Some(ResponseData::ScreenState(ScreenCapture { + screen: ScreenState { text: Some(text), .. }, + outcome: CaptureOutcome::Exited, + exit: Some(CaptureExit { exit_code: Some(9), .. }), + .. + })) if text.contains("recovered-evidence") )); let logs = handle_request( @@ -1782,6 +2052,126 @@ mod tests { )); } + #[tokio::test] + async fn snapshot_wait_returns_exit_evidence_before_reaper_runs() { + let sessions = Arc::new(SessionManager::new()); + sessions + .create_session( + vec![ + "sh".to_string(), + "-c".to_string(), + "printf ready; sleep 0.2; exit 7".to_string(), + ], + Some("exits-during-wait".to_string()), + None, + None, + ) + .await + .expect("create exiting session"); + tokio::time::sleep(Duration::from_millis(75)).await; + + let baseline = handle_request( + Request::new( + "baseline", + Command::Snapshot { + session: Some("exits-during-wait".to_string()), + format: SnapshotFormat::Full, + await_change: None, + settle_ms: 0, + timeout_ms: 1000, + }, + ), + sessions.clone(), + Arc::new(Notify::new()), + ) + .await; + let baseline_hash = match baseline.data { + Some(ResponseData::ScreenState(capture)) => { + capture.screen.content_hash.expect("baseline hash") + } + _ => panic!("expected baseline screen capture"), + }; + + let waited = handle_request( + Request::new( + "wait-for-exit", + Command::Snapshot { + session: Some("exits-during-wait".to_string()), + format: SnapshotFormat::Full, + await_change: Some(baseline_hash), + settle_ms: 2000, + timeout_ms: 3000, + }, + ), + sessions, + Arc::new(Notify::new()), + ) + .await; + + assert!(matches!( + waited.data, + Some(ResponseData::ScreenState(ScreenCapture { + screen: ScreenState { text: Some(text), .. }, + outcome: CaptureOutcome::Exited, + exit: Some(CaptureExit { + exit_code: Some(7), + success: false, + output_complete: true, + .. + }), + .. + })) if text.contains("ready") + )); + } + + #[tokio::test] + async fn deadline_does_not_treat_output_eof_as_process_exit() { + let sessions = Arc::new(SessionManager::new()); + let session_id = sessions + .create_session( + vec!["sleep".to_string(), "10".to_string()], + Some("closed-output".to_string()), + None, + None, + ) + .await + .expect("create live session"); + let mut observer = sessions + .observe_session(&session_id) + .await + .expect("observe live session"); + + let response = snapshot_deadline_response( + "wait-after-eof", + &sessions, + &session_id, + &mut observer, + SnapshotDeadline { + format: SnapshotFormat::Full, + output_closed: true, + note: "deadline".to_string(), + }, + ) + .await; + + assert!( + matches!( + &response.data, + Some(ResponseData::ScreenState(ScreenCapture { + outcome: CaptureOutcome::Deadline, + exit: None, + note: Some(note), + .. + })) if note.contains("EOF alone is not an exit") + ), + "got: {response:?}" + ); + sessions + .kill_session(&session_id) + .await + .expect("stop output-closing session"); + } + #[tokio::test] async fn logs_returns_bounded_ordered_raw_output_over_the_socket() { let temp_dir = std::env::temp_dir(); @@ -2149,7 +2539,7 @@ mod tests { id: "snap-1".to_string(), command: Command::Snapshot { session: Some("test-snap".to_string()), - format: Some(SnapshotFormat::Text), + format: SnapshotFormat::Text, await_change: None, settle_ms: 0, timeout_ms: 30000, @@ -2172,7 +2562,10 @@ mod tests { assert!(snap_response.success, "Snapshot should succeed"); // Verify the snapshot contains our text - if let Some(ResponseData::Snapshot { format, content }) = snap_response.data { + if let Some(ResponseData::Snapshot { + format, content, .. + }) = snap_response.data + { assert_eq!(format, SnapshotFormat::Text); assert!( content.contains("hello from test"), @@ -2252,7 +2645,7 @@ mod tests { id: "snap-full".to_string(), command: Command::Snapshot { session: Some("full-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: None, settle_ms: 0, timeout_ms: 30000, @@ -2277,30 +2670,36 @@ mod tests { if let Some(ResponseData::ScreenState(screen_state)) = snap_response.data { // Check snapshot_id is non-zero assert!( - screen_state.snapshot_id > 0, + screen_state.screen.snapshot_id > 0, "snapshot_id should be positive" ); // Check size - assert_eq!(screen_state.size.cols, 80, "Default cols should be 80"); - assert_eq!(screen_state.size.rows, 24, "Default rows should be 24"); + assert_eq!( + screen_state.screen.size.cols, 80, + "Default cols should be 80" + ); + assert_eq!( + screen_state.screen.size.rows, 24, + "Default rows should be 24" + ); // Check cursor position is valid assert!( - screen_state.cursor.row < screen_state.size.rows, + screen_state.screen.cursor.row < screen_state.screen.size.rows, "Cursor row should be within bounds" ); assert!( - screen_state.cursor.col < screen_state.size.cols, + screen_state.screen.cursor.col < screen_state.screen.size.cols, "Cursor col should be within bounds" ); // Check text is included in Full format assert!( - screen_state.text.is_some(), + screen_state.screen.text.is_some(), "Full format should include text" ); - let text = screen_state.text.unwrap(); + let text = screen_state.screen.text.unwrap(); assert!( text.contains("full format test"), "Text should contain output: {}", @@ -2403,7 +2802,7 @@ mod tests { id: "snap-1".to_string(), command: Command::Snapshot { session: Some("type-test".to_string()), - format: Some(SnapshotFormat::Text), + format: SnapshotFormat::Text, await_change: None, settle_ms: 0, timeout_ms: 30000, @@ -3235,7 +3634,7 @@ mod tests { id: "snap-baseline".to_string(), command: Command::Snapshot { session: Some("await-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: None, settle_ms: 0, timeout_ms: 30000, @@ -3260,7 +3659,9 @@ mod tests { ); let baseline_hash = match baseline_response.data { - Some(ResponseData::ScreenState(state)) => state.content_hash.expect("should have hash"), + Some(ResponseData::ScreenState(state)) => { + state.screen.content_hash.expect("should have hash") + } _ => panic!("Expected ScreenState"), }; @@ -3291,7 +3692,7 @@ mod tests { id: "snap-await".to_string(), command: Command::Snapshot { session: Some("await-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: Some(baseline_hash), settle_ms: 50, timeout_ms: 5000, @@ -3325,14 +3726,19 @@ mod tests { // Verify hash actually changed if let Some(ResponseData::ScreenState(state)) = await_response.data { + assert_eq!(state.outcome, CaptureOutcome::Settled); assert_ne!( - state.content_hash, + state.screen.content_hash, Some(baseline_hash), "Hash should have changed" ); // Verify content contains what we typed assert!( - state.text.as_ref().is_some_and(|t| t.contains("hello")), + state + .screen + .text + .as_ref() + .is_some_and(|t| t.contains("hello")), "Text should contain 'hello'" ); } else { @@ -3349,7 +3755,7 @@ mod tests { /// first iteration and await_change returned instantly on an unchanged /// screen. #[tokio::test] - async fn test_snapshot_await_change_times_out_on_static_screen() { + async fn snapshot_await_change_deadline_returns_latest_evidence() { let temp_dir = std::env::temp_dir(); let socket_path = temp_dir.join(format!("pilotty-await-static-{}.sock", std::process::id())); @@ -3404,7 +3810,7 @@ mod tests { id: "snap-baseline".to_string(), command: Command::Snapshot { session: Some("await-static-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: None, settle_ms: 0, timeout_ms: 30000, @@ -3424,7 +3830,9 @@ mod tests { let baseline_response: Response = serde_json::from_str(&response_line).expect("parse baseline response"); let baseline_hash = match baseline_response.data { - Some(ResponseData::ScreenState(state)) => state.content_hash.expect("should have hash"), + Some(ResponseData::ScreenState(state)) => { + state.screen.content_hash.expect("should have hash") + } _ => panic!("Expected ScreenState"), }; @@ -3435,7 +3843,7 @@ mod tests { id: "snap-await".to_string(), command: Command::Snapshot { session: Some("await-static-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: Some(baseline_hash), settle_ms: 0, timeout_ms: 500, @@ -3458,21 +3866,24 @@ mod tests { let await_response: Response = serde_json::from_str(&response_line).expect("parse await response"); - assert!( - !await_response.success, - "Awaiting a change on a static screen should time out, not succeed" - ); + assert!(await_response.success); assert!( elapsed >= Duration::from_millis(400), "Should have waited close to the 500ms timeout, returned after {:?}", elapsed ); - let error = await_response.error.expect("should have error"); - assert!( - error.message.contains("Timeout"), - "Error should mention timeout, got: {}", - error.message - ); + assert!(matches!( + await_response.data, + Some(ResponseData::ScreenState(ScreenCapture { + screen: ScreenState { + content_hash: Some(hash), + .. + }, + outcome: CaptureOutcome::Deadline, + exit: None, + note: Some(note), + })) if hash == baseline_hash && note.contains("Timeout") + )); server_handle.abort(); let _ = std::fs::remove_file(&socket_path); @@ -3483,7 +3894,7 @@ mod tests { /// as stable and settle degraded to a fixed sleep — the "kept changing" /// timeout branch was unreachable. #[tokio::test] - async fn test_snapshot_settle_times_out_when_screen_keeps_changing() { + async fn snapshot_settle_deadline_returns_latest_evidence() { let temp_dir = std::env::temp_dir(); let socket_path = temp_dir.join(format!("pilotty-settle-busy-{}.sock", std::process::id())); let pid_path = socket_path.with_extension("pid"); @@ -3542,7 +3953,7 @@ mod tests { id: "snap-settle".to_string(), command: Command::Snapshot { session: Some("settle-busy-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: None, settle_ms: 200, timeout_ms: 800, @@ -3565,21 +3976,21 @@ mod tests { let settle_response: Response = serde_json::from_str(&response_line).expect("parse settle response"); - assert!( - !settle_response.success, - "Settle on a continuously changing screen should time out, not succeed" - ); + assert!(settle_response.success); assert!( elapsed >= Duration::from_millis(700), "Should have waited close to the 800ms timeout, returned after {:?}", elapsed ); - let error = settle_response.error.expect("should have error"); - assert!( - error.message.contains("stabilize"), - "Error should mention stabilization, got: {}", - error.message - ); + assert!(matches!( + settle_response.data, + Some(ResponseData::ScreenState(ScreenCapture { + screen: ScreenState { text: Some(text), .. }, + outcome: CaptureOutcome::Deadline, + exit: None, + note: Some(note), + })) if text.contains("tick") && note.contains("stabilize") + )); server_handle.abort(); let _ = std::fs::remove_file(&socket_path); @@ -3719,7 +4130,7 @@ mod tests { id: "snap-elem".to_string(), command: Command::Snapshot { session: Some("elem-test".to_string()), - format: Some(SnapshotFormat::Full), + format: SnapshotFormat::Full, await_change: None, settle_ms: 0, timeout_ms: 30000, @@ -3744,23 +4155,23 @@ mod tests { if let Some(ResponseData::ScreenState(screen_state)) = snap_response.data { // Full format includes text assert!( - screen_state.text.is_some(), + screen_state.screen.text.is_some(), "Full format should include text" ); // Full format SHOULD include elements assert!( - screen_state.elements.is_some(), + screen_state.screen.elements.is_some(), "Full format should include elements" ); // Full format SHOULD include content_hash assert!( - screen_state.content_hash.is_some(), + screen_state.screen.content_hash.is_some(), "Full format should include content_hash" ); - let elements = screen_state.elements.unwrap(); + let elements = screen_state.screen.elements.unwrap(); // Should detect at least the toggles (checkboxes are high confidence) // [x] -> Toggle checked=true, [ ] -> Toggle checked=false diff --git a/crates/pilotty-cli/src/daemon/session.rs b/crates/pilotty-cli/src/daemon/session.rs index 9714133..e83bf89 100644 --- a/crates/pilotty-cli/src/daemon/session.rs +++ b/crates/pilotty-cli/src/daemon/session.rs @@ -381,6 +381,21 @@ impl SessionObserver { self.session.snapshot(with_elements).await } + /// Inspect the direct child without treating PTY EOF as process exit. + pub(crate) fn exit_metadata(&self) -> Result, ApiError> { + self.session + .observe_process_exit() + .map(|exit| { + exit.map(|exit| ExitMetadata { + code: Some(exit.status.exit_code()), + signal: exit.status.signal().map(ToOwned::to_owned), + success: exit.status.success(), + killed_by_client: false, + }) + }) + .map_err(|error| ApiError::internal(format!("Failed to inspect session exit: {error}"))) + } + /// Wait for output, EOF, pump failure, or the supplied duration. pub(crate) async fn wait_for_update(&mut self, duration: Duration) -> ObservationEvent { match tokio::time::timeout(duration, self.pump_state.changed()).await { @@ -977,7 +992,7 @@ fn tombstone_status(tombstone: Tombstone) -> SessionStatus { #[cfg(test)] mod tests { - use super::*; + use crate::daemon::session::*; #[tokio::test] async fn test_create_and_get_session() { diff --git a/crates/pilotty-cli/src/main.rs b/crates/pilotty-cli/src/main.rs index e02c2be..59a066a 100644 --- a/crates/pilotty-cli/src/main.rs +++ b/crates/pilotty-cli/src/main.rs @@ -4,7 +4,10 @@ mod args; mod daemon; use clap::Parser; -use pilotty_core::protocol::{Command, Request, ResponseData, ScrollDirection, SnapshotFormat}; +use pilotty_core::error::ErrorCode; +use pilotty_core::protocol::{ + CaptureOutcome, Command, Request, ResponseData, ScrollDirection, SnapshotFormat, +}; use std::io::Write; use tracing::{error, info}; use uuid::Uuid; @@ -13,7 +16,21 @@ use crate::args::{Cli, Commands}; use crate::daemon::client::DaemonClient; use crate::daemon::server::DaemonServer; -fn main() { +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum CliExitCode { + Success = 0, + GenericError = 1, + Timing = 3, + Lifecycle = 4, +} + +impl CliExitCode { + fn value(self) -> u8 { + self as u8 + } +} + +fn main() -> std::process::ExitCode { // Initialize tracing tracing_subscriber::fmt() .with_env_filter( @@ -27,14 +44,25 @@ fn main() { // Daemon command runs the server, all other commands are clients if let Commands::Daemon = cli.command { run_daemon(); - return; + return std::process::ExitCode::SUCCESS; } // All other commands talk to the daemon - if let Err(e) = run_client_command(cli) { - error!("{}", e); - std::process::exit(1); + match run_client_command(cli) { + Ok(CliExitCode::Success) => {} + Ok(code) => { + if let Err(error) = std::io::stdout().flush() { + error!("Failed to flush command evidence: {}", error); + } + return std::process::ExitCode::from(code.value()); + } + Err(e) => { + error!("{}", e); + return std::process::ExitCode::from(CliExitCode::GenericError.value()); + } } + + std::process::ExitCode::SUCCESS } /// Convert CLI args to a protocol Command. @@ -58,11 +86,11 @@ fn cli_to_command(cli: &Cli) -> Option { }), Commands::Snapshot(args) => Some(Command::Snapshot { session: args.session.clone(), - format: Some(match args.format { + format: match args.format { crate::args::SnapshotFormat::Full => SnapshotFormat::Full, crate::args::SnapshotFormat::Compact => SnapshotFormat::Compact, crate::args::SnapshotFormat::Text => SnapshotFormat::Text, - }), + }, await_change: args.await_change, settle_ms: args.settle, timeout_ms: args.timeout, @@ -114,14 +142,27 @@ fn cli_to_command(cli: &Cli) -> Option { } /// Run a client command by connecting to the daemon. -fn run_client_command(cli: Cli) -> anyhow::Result<()> { +fn run_client_command(cli: Cli) -> anyhow::Result { + let strict = matches!( + &cli.command, + Commands::Snapshot(args) if args.strict + ); + let targets_live_session = matches!( + &cli.command, + Commands::Type(_) + | Commands::Key(_) + | Commands::Click(_) + | Commands::Scroll(_) + | Commands::Resize(_) + ); + // Handle commands that don't need daemon communication let Some(command) = cli_to_command(&cli) else { // Examples command just prints and exits if let Commands::Examples = cli.command { println!("{}", crate::args::EXAMPLES_TEXT); } - return Ok(()); + return Ok(CliExitCode::Success); }; let runtime = tokio::runtime::Runtime::new()?; @@ -137,12 +178,14 @@ fn run_client_command(cli: Cli) -> anyhow::Result<()> { let response = client.request(request).await?; // Print response - if response.success { + let exit_code = if response.success { if let Some(data) = response.data { + let outcome = data.capture_outcome(); match data { ResponseData::Snapshot { format: SnapshotFormat::Text, content, + .. } => { println!("{}", content); } @@ -162,16 +205,43 @@ fn run_client_command(cli: Cli) -> anyhow::Result<()> { } _ => println!("{}", serde_json::to_string_pretty(&data)?), } + + capture_exit_code(strict, outcome) + } else { + CliExitCode::Success } } else if let Some(err) = response.error { eprintln!("Error: {}", err); - std::process::exit(1); - } + api_error_exit_code(targets_live_session, &err.code) + } else { + CliExitCode::GenericError + }; - Ok(()) + Ok(exit_code) }) } +fn capture_exit_code(strict: bool, outcome: Option) -> CliExitCode { + if !strict { + return CliExitCode::Success; + } + + match outcome { + Some(CaptureOutcome::Deadline) => CliExitCode::Timing, + Some(CaptureOutcome::Exited) => CliExitCode::Lifecycle, + Some(CaptureOutcome::Immediate | CaptureOutcome::Settled | CaptureOutcome::Changed) + | None => CliExitCode::Success, + } +} + +fn api_error_exit_code(targets_live_session: bool, error: &ErrorCode) -> CliExitCode { + if targets_live_session && *error == ErrorCode::SessionExited { + CliExitCode::Lifecycle + } else { + CliExitCode::GenericError + } +} + /// Run the daemon server with graceful signal handling. /// /// Handles SIGINT (Ctrl+C) and SIGTERM for clean shutdown. @@ -239,3 +309,47 @@ async fn sigterm() { async fn sigterm() { std::future::pending::<()>().await; } + +#[cfg(test)] +mod tests { + use pilotty_core::error::ErrorCode; + use pilotty_core::protocol::CaptureOutcome; + + use crate::{api_error_exit_code, capture_exit_code, CliExitCode}; + + #[test] + fn strict_capture_exit_codes_are_categorical() { + assert_eq!( + capture_exit_code(true, Some(CaptureOutcome::Deadline)), + CliExitCode::Timing + ); + assert_eq!( + capture_exit_code(true, Some(CaptureOutcome::Exited)), + CliExitCode::Lifecycle + ); + assert_eq!( + capture_exit_code(true, Some(CaptureOutcome::Settled)), + CliExitCode::Success + ); + assert_eq!( + capture_exit_code(false, Some(CaptureOutcome::Deadline)), + CliExitCode::Success + ); + } + + #[test] + fn exited_input_uses_lifecycle_exit_code() { + assert_eq!( + api_error_exit_code(true, &ErrorCode::SessionExited), + CliExitCode::Lifecycle + ); + assert_eq!( + api_error_exit_code(false, &ErrorCode::SessionExited), + CliExitCode::GenericError + ); + assert_eq!( + api_error_exit_code(true, &ErrorCode::InvalidInput), + CliExitCode::GenericError + ); + } +} diff --git a/crates/pilotty-core/src/protocol.rs b/crates/pilotty-core/src/protocol.rs index 1c432df..d0d3fa6 100644 --- a/crates/pilotty-core/src/protocol.rs +++ b/crates/pilotty-core/src/protocol.rs @@ -93,7 +93,8 @@ pub enum Command { /// stabilizes for a specified duration. Snapshot { session: Option, - format: Option, + #[serde(default)] + format: SnapshotFormat, /// If set, block until content_hash differs from this value. #[serde(default)] await_change: Option, @@ -166,13 +167,13 @@ impl Command { retain_bytes: Some(_), .. } + | Self::Snapshot { .. } | Self::Logs { .. } | Self::Status { .. } => PROTOCOL_V2, Self::Spawn { retain_bytes: None, .. } | Self::Kill { .. } - | Self::Snapshot { .. } | Self::Type { .. } | Self::Key { .. } | Self::Click { .. } @@ -198,6 +199,39 @@ pub enum SnapshotFormat { Text, } +/// How an outcome-aware snapshot completed. +#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] +#[serde(rename_all = "snake_case")] +pub enum CaptureOutcome { + Immediate, + Settled, + Changed, + Deadline, + Exited, +} + +/// Process evidence attached when a capture observes an exited session. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct CaptureExit { + pub exit_code: Option, + pub signal: Option, + pub success: bool, + pub killed_by_client: bool, + pub output_complete: bool, +} + +/// A screen capture with optional wait and lifecycle evidence. +#[derive(Debug, Clone, PartialEq, Serialize, Deserialize)] +pub struct ScreenCapture { + #[serde(flatten)] + pub screen: ScreenState, + pub outcome: CaptureOutcome, + #[serde(skip_serializing_if = "Option::is_none")] + pub exit: Option, + #[serde(skip_serializing_if = "Option::is_none")] + pub note: Option, +} + /// Scroll direction. #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] #[serde(rename_all = "snake_case")] @@ -262,11 +296,16 @@ impl Response { #[serde(tag = "type", rename_all = "snake_case")] pub enum ResponseData { /// Full screen state snapshot. - ScreenState(ScreenState), + ScreenState(ScreenCapture), /// Text-format snapshot. Snapshot { format: SnapshotFormat, content: String, + outcome: CaptureOutcome, + #[serde(skip_serializing_if = "Option::is_none")] + exit: Option, + #[serde(skip_serializing_if = "Option::is_none")] + note: Option, }, /// Session created response. SessionCreated { session_id: String, message: String }, @@ -299,15 +338,24 @@ impl ResponseData { /// older client. pub fn minimum_protocol(&self) -> u32 { match self { - Self::Logs { .. } | Self::Status(_) => PROTOCOL_V2, - Self::ScreenState(_) - | Self::Snapshot { .. } - | Self::SessionCreated { .. } + Self::ScreenState(_) | Self::Snapshot { .. } | Self::Logs { .. } | Self::Status(_) => { + PROTOCOL_V2 + } + Self::SessionCreated { .. } | Self::Sessions { .. } | Self::WaitForResult { .. } | Self::Ok { .. } => LEGACY_PROTOCOL_VERSION, } } + + /// Return the outcome attached to a screen or text capture. + pub fn capture_outcome(&self) -> Option { + match self { + Self::ScreenState(capture) => Some(capture.outcome), + Self::Snapshot { outcome, .. } => Some(*outcome), + _ => None, + } + } } /// Information about an active session. @@ -361,7 +409,7 @@ pub enum SessionStatus { #[cfg(test)] mod tests { - use super::*; + use crate::protocol::*; #[test] fn request_serializes_with_protocol_version() { @@ -488,9 +536,17 @@ mod tests { truncated: true, }, }; + let outcome_snapshot = Command::Snapshot { + session: None, + format: SnapshotFormat::Full, + await_change: None, + settle_ms: 0, + timeout_ms: 30_000, + }; assert_eq!(plain_spawn.minimum_protocol(), LEGACY_PROTOCOL_VERSION); assert_eq!(configured_spawn.minimum_protocol(), PROTOCOL_V2); + assert_eq!(outcome_snapshot.minimum_protocol(), PROTOCOL_V2); assert_eq!( Command::Logs { session: None }.minimum_protocol(), PROTOCOL_V2 @@ -502,6 +558,27 @@ mod tests { assert_eq!(ResponseData::Status(status).minimum_protocol(), PROTOCOL_V2); } + #[test] + fn capture_outcome_round_trips_as_flat_snapshot_evidence() { + let response = ResponseData::ScreenState(ScreenCapture { + screen: ScreenState::empty(80, 24), + outcome: CaptureOutcome::Deadline, + exit: None, + note: Some("Screen kept changing".to_string()), + }); + + assert_eq!(response.minimum_protocol(), PROTOCOL_V2); + let json = serde_json::to_string(&response).expect("serialize capture outcome"); + assert!(json.contains("\"type\":\"screen_state\""), "got: {json}"); + assert!(json.contains("\"outcome\":\"deadline\""), "got: {json}"); + assert!(json.contains("\"snapshot_id\":0"), "got: {json}"); + assert!(!json.contains("\"screen\""), "got: {json}"); + + let decoded: ResponseData = + serde_json::from_str(&json).expect("deserialize capture outcome"); + assert_eq!(decoded, response); + } + #[test] fn logs_response_requires_current_protocol_and_preserves_raw_bytes() { let response = ResponseData::Logs { diff --git a/npm/README.md b/npm/README.md index db0f597..49e4857 100644 --- a/npm/README.md +++ b/npm/README.md @@ -88,6 +88,7 @@ The `snapshot` command returns structured data about the terminal screen: ```json { + "outcome": "immediate", "snapshot_id": 42, "size": { "cols": 80, "rows": 24 }, "cursor": { "row": 5, "col": 10, "visible": true }, @@ -97,6 +98,8 @@ The `snapshot` command returns structured data about the terminal screen: Use the cursor position and text content to understand the screen state and navigate using keyboard commands (Tab, Enter, arrow keys) or click at specific coordinates. +Snapshots report `immediate`, `changed`, `settled`, `deadline`, or `exited` while preserving the latest screen evidence. Use `--strict` when scripts should exit 3 on a deadline or 4 when the session exits. + ## Documentation See the **[GitHub repository](https://github.com/msmps/pilotty)** for full documentation including: diff --git a/skills/pilotty/SKILL.md b/skills/pilotty/SKILL.md index 6d15191..ed77a5f 100644 --- a/skills/pilotty/SKILL.md +++ b/skills/pilotty/SKILL.md @@ -74,6 +74,7 @@ HASH=$(pilotty snapshot | jq '.content_hash') pilotty key Enter pilotty snapshot --await-change $HASH # Block until screen changes pilotty snapshot --await-change $HASH --settle 50 # Wait for 50ms stability +pilotty snapshot --settle 50 --strict # Exit nonzero on deadline/exit ``` ### Input @@ -132,6 +133,7 @@ pilotty wait-for "~" -s editor # Wait in specific session | `--delay ` | Delay between keys in a sequence (default: 0, max: 10000) | | `--await-change ` | Block snapshot until content_hash differs | | `--settle ` | Wait for screen to be stable for this many ms (default: 0) | +| `--strict` | Exit 3 on capture deadline or 4 when the session exits | ### Environment variables @@ -147,6 +149,7 @@ The `snapshot` command returns structured JSON with detected UI elements: ```json { + "outcome": "immediate", "snapshot_id": 42, "size": { "cols": 80, "rows": 24 }, "cursor": { "row": 5, "col": 10, "visible": true }, @@ -229,6 +232,20 @@ pilotty snapshot --await-change $HASH --settle 100 | `--await-change ` | Block until `content_hash` differs from this value | | `--settle ` | After change detected, wait for screen to be stable for MS | | `-t, --timeout ` | Maximum wait time (default: 30000) | +| `--strict` | Keep evidence but exit nonzero for `deadline` or `exited` | + +Every snapshot reports why it returned: + +| Outcome | Meaning | Agent action | +|---------|---------|--------------| +| `immediate` | No wait requested | Read the current screen | +| `changed` | `--await-change` was satisfied | Continue with the new screen | +| `settled` | The requested stability window was satisfied | Treat the screen as stable | +| `deadline` | Time expired; latest evidence is included | Inspect evidence, then retry with more time if useful | +| `exited` | The session ended; final evidence is included | Do not send more input; inspect exit metadata or logs | + +Deadline and exit outcomes still exit 0 by default. Add `--strict` only when shell +control flow should fail for those outcomes. **Why this is better than sleep:** - `sleep 1` is a guess - too short causes race conditions, too long slows automation @@ -516,7 +533,7 @@ pilotty uses a background daemon for session management: - **Auto-start**: Daemon starts on first command - **Auto-stop**: Shuts down after 5 minutes with no sessions -- **Session cleanup**: Sessions removed when process exits (within 500ms) +- **Final evidence**: Exited sessions retain status, final screen, and bounded logs for up to 10 minutes - **Shared state**: Multiple CLI calls share sessions You rarely need to manage the daemon manually. @@ -525,6 +542,14 @@ You rarely need to manage the daemon manually. Errors include actionable suggestions: +| Exit | Category | Agent action | +|------|----------|--------------| +| `0` | Command completed | Inspect snapshot `outcome`; it may be `deadline` or `exited` without `--strict` | +| `1` | Generic/API error | Read the error code, message, and suggestion | +| `2` | Usage error | Fix the command syntax | +| `3` | Strict capture deadline | Inspect printed evidence and retry with more time if appropriate | +| `4` | Session lifecycle ended | Do not retry input against that session; use `status`, `snapshot`, or `logs` | + ```json { "code": "SESSION_NOT_FOUND",