diff --git a/rust/src/agent_sessions.rs b/rust/src/agent_sessions.rs index 52f7b35c62..c35477a662 100644 --- a/rust/src/agent_sessions.rs +++ b/rust/src/agent_sessions.rs @@ -381,22 +381,46 @@ impl LocalAgentSessionScanner { .await; let (processes, error) = match process_result { Ok(result) if result.timed_out => ( - Vec::new(), - Some("Windows process discovery timed out; file-only sessions may still appear."), - ), - Ok(result) if result.exit_code == Some(0) => { - (WindowsProcessOutputParser::parse(&result.text), None) - } - Ok(_) => ( Vec::new(), Some( - "Windows process discovery failed; verify PowerShell and CIM access. File-only sessions may still appear.", + "Windows process discovery timed out; file-only sessions may still appear." + .to_string(), ), ), + Ok(result) if result.exit_code == Some(0) => { + match WindowsProcessOutputParser::parse(&result.text) { + Ok(processes) => (processes, None), + Err(parse_error) => ( + Vec::new(), + Some(format!( + "Windows process discovery could not parse its output \ + ({parse_error}); file-only sessions may still appear." + )), + ), + } + } + Ok(result) => { + let stderr_tail = Self::error_tail(&result.stderr); + let hint = if stderr_tail.is_empty() { + String::new() + } else { + format!(": {stderr_tail}") + }; + ( + Vec::new(), + Some(format!( + "Windows process discovery failed with exit code {}{}; \ + file-only sessions may still appear.", + Self::exit_code_label(result.exit_code), + hint + )), + ) + } Err(_) => ( Vec::new(), Some( - "Unable to launch PowerShell for process discovery; file-only sessions may still appear.", + "Unable to launch PowerShell for process discovery; file-only sessions may still appear." + .to_string(), ), ), }; @@ -436,6 +460,31 @@ impl LocalAgentSessionScanner { } } + /// Redacted, length-bounded tail of captured stderr for error messages. + fn error_tail(stderr: &str) -> String { + let tail = crate::logging::safe_error_message(stderr); + tail.lines() + .map(|line| { + if line.chars().count() > CommandRunner::MAX_LINE_CHARS { + let cut: String = line.chars().take(CommandRunner::MAX_LINE_CHARS).collect(); + format!("{cut}\u{2026}") + } else { + line.to_string() + } + }) + .rev() + .take(3) + .collect::>() + .join("; ") + } + + /// Human-readable exit-code label for user-facing messages. + fn exit_code_label(exit_code: Option) -> String { + exit_code + .map(|code| code.to_string()) + .unwrap_or_else(|| "unknown".to_string()) + } + fn scan_files( &self, host: &str, diff --git a/rust/src/agent_sessions/parsers.rs b/rust/src/agent_sessions/parsers.rs index b0c6fd4105..6cec88ac3f 100644 --- a/rust/src/agent_sessions/parsers.rs +++ b/rust/src/agent_sessions/parsers.rs @@ -113,18 +113,28 @@ struct WindowsProcessMetadata { } impl WindowsProcessOutputParser { - pub fn parse(output: &str) -> Vec { - let Ok(value) = serde_json::from_str::(output.trim()) else { - return Vec::new(); + /// Parse `ConvertTo-Json` output from the Windows process query. + /// + /// Distinguishes "legitimately no processes" (empty array) from an + /// unparseable response: only genuinely malformed JSON is an error, so + /// callers can surface parse failures instead of silently reporting an + /// empty discovery result. + pub fn parse(output: &str) -> Result, String> { + let trimmed = output.trim(); + if trimmed.is_empty() { + return Err("process query produced no output".to_string()); + } + let Ok(value) = serde_json::from_str::(trimmed) else { + return Err("process query output was not valid JSON".to_string()); }; let values = match value { Value::Array(values) => values, Value::Object(_) => vec![value], - _ => return Vec::new(), + _ => return Err("process query output was not a JSON array".to_string()), }; let mut seen = HashSet::new(); - values + Ok(values .into_iter() .filter_map(|value| serde_json::from_value::(value).ok()) .filter(|process| process.process_id > 0 && seen.insert(process.process_id)) @@ -153,7 +163,7 @@ impl WindowsProcessOutputParser { command: None, } }) - .collect() + .collect()) } } diff --git a/rust/src/agent_sessions/tests.rs b/rust/src/agent_sessions/tests.rs index e843f81c9a..9315cdd24a 100644 --- a/rust/src/agent_sessions/tests.rs +++ b/rust/src/agent_sessions/tests.rs @@ -206,7 +206,7 @@ bad line } ]"#; - let records = WindowsProcessOutputParser::parse(output); + let records = WindowsProcessOutputParser::parse(output).unwrap(); assert_eq!(records.len(), 2); assert_eq!(records[0].provider, Some(AgentSessionProvider::Claude)); @@ -216,6 +216,56 @@ bad line assert!(!records[0].executable.contains("super-secret")); } + #[test] + fn windows_process_parser_treats_empty_array_as_empty_result() { + let records = WindowsProcessOutputParser::parse("[]").unwrap(); + assert!(records.is_empty()); + } + + #[test] + fn windows_process_parser_reports_unparseable_output() { + let error = WindowsProcessOutputParser::parse("gateway timeout") + .expect_err("malformed JSON must be an error"); + assert!(error.contains("JSON"), "{error}"); + + let error = WindowsProcessOutputParser::parse(" ") + .expect_err("empty output must be an error"); + assert!(error.contains("no output"), "{error}"); + } + + #[test] + fn error_tail_caps_line_length_and_redacts() { + let stderr = "Authorization: Bearer sk-secret-token\nshort line\n"; + let tail = LocalAgentSessionScanner::error_tail(stderr); + + assert!(!tail.contains("sk-secret-token"), "{tail}"); + assert!(tail.contains("[REDACTED]"), "{tail}"); + for line in tail.split("; ") { + assert!(line.chars().count() <= CommandRunner::MAX_LINE_CHARS + 1); + } + } + + #[test] + fn error_tail_truncates_long_lines() { + let long_line = "x".repeat(CommandRunner::MAX_LINE_CHARS + 50); + let tail = LocalAgentSessionScanner::error_tail(&long_line); + + assert!(tail.ends_with('\u{2026}'), "{tail}"); + assert!( + tail.chars().count() <= CommandRunner::MAX_LINE_CHARS + 1, + "{tail}" + ); + } + + #[test] + fn exit_code_label_is_human_readable() { + assert_eq!(LocalAgentSessionScanner::exit_code_label(Some(1)), "1"); + assert_eq!( + LocalAgentSessionScanner::exit_code_label(None), + "unknown" + ); + } + #[test] fn windows_process_query_never_requests_raw_command_lines() { let options = LocalAgentSessionScanner::process_options(Duration::from_secs(2)); diff --git a/rust/src/host/command_runner.rs b/rust/src/host/command_runner.rs index a2c0beff10..c27775f6b9 100755 --- a/rust/src/host/command_runner.rs +++ b/rust/src/host/command_runner.rs @@ -69,8 +69,10 @@ impl Default for CommandOptions { /// Result of running a command #[derive(Debug, Clone)] pub struct CommandResult { - /// Captured output text + /// Captured output text (stdout) pub text: String, + /// Captured stderr text + pub stderr: String, /// Whether the command timed out pub timed_out: bool, /// Exit code if available @@ -112,7 +114,10 @@ pub struct CommandRunner { } impl CommandRunner { + /// Upper bound for a single captured line fed into user-facing tails. + pub const MAX_LINE_CHARS: usize = 200; const MAX_CAPTURE_BYTES: usize = 1024 * 1024; + pub fn new() -> Self { Self { env_additions: HashMap::new(), @@ -151,12 +156,13 @@ impl CommandRunner { Self::send_initial_input(&mut child, input, options.initial_delay, deadline); // Capture output - let (output, timed_out) = self.capture_output(&mut child, options, deadline)?; + let (output, stderr, timed_out) = self.capture_output(&mut child, options, deadline)?; let exit_code = Self::finish_child(&mut child); Ok(CommandResult { text: output, + stderr, timed_out, exit_code, }) @@ -251,28 +257,55 @@ impl CommandRunner { let _flushed_stdin = stdin.flush(); } + /// Grace period between stdout/stderr EOF and process-object exit. + /// + /// Windows is the motivating case: PowerShell tears down its console + /// handles slightly before the process object transitions to signaled, so + /// a single `try_wait` right after capture sees `Ok(None)` even though the + /// process exited successfully milliseconds later. Waiting (instead of + /// immediately killing) preserves the real exit code. + const EXIT_GRACE_PERIOD: Duration = Duration::from_millis(250); + fn finish_child(child: &mut Child) -> Option { - match child.try_wait() { - Ok(Some(status)) => status.code(), - Ok(None) => { - // Teardown after timeout/capture: kill/wait results cannot - // change the outcome, already reported separately. - let _killed = child.kill(); - let _reaped = child.wait(); - None + let mut remaining = Self::EXIT_GRACE_PERIOD; + loop { + match child.try_wait() { + Ok(Some(status)) => return status.code(), + Ok(None) => {} + Err(_) => return None, } - Err(_) => None, + if remaining.is_zero() { + break; + } + let step = remaining.min(Duration::from_millis(10)); + std::thread::sleep(step); + remaining -= step; + } + // Teardown after timeout/capture. If the process exited between the + // grace loop and kill (kill fails on an already-dead process), `wait` + // still yields the true exit status; when the kill lands, the code is + // ours and already reported separately. + let killed = child.kill().is_ok(); + let reaped = child.wait().ok(); + if killed { + return None; } + reaped.and_then(|status| status.code()) } /// Capture output from a running process + /// + /// Returns `(stdout, stderr, timed_out)`. Stderr lines are kept in a + /// separate buffer (bounded like stdout) so callers can surface real + /// diagnostics; they are never folded into the stdout `text` field. fn capture_output( &self, child: &mut Child, options: &CommandOptions, deadline: Instant, - ) -> Result<(String, bool), CommandError> { + ) -> Result<(String, String, bool), CommandError> { let mut output = String::new(); + let mut stderr_output = String::new(); let mut last_output_time = Instant::now(); let (sender, receiver) = mpsc::channel(); Self::read_stream(Self::stdout_reader(child)?, sender.clone(), true); @@ -281,7 +314,7 @@ impl CommandRunner { loop { if Self::past_deadline(deadline) { - return Ok((output, true)); + return Ok((output, stderr_output, true)); } if Self::idle_timed_out(options.idle_timeout, last_output_time) { break; @@ -292,6 +325,7 @@ impl CommandRunner { Ok(StreamEvent::Line { text, capture }) => { last_output_time = Instant::now(); if !capture { + Self::append_output_line(&mut stderr_output, &text); continue; } Self::append_output_line(&mut output, &text); @@ -315,7 +349,7 @@ impl CommandRunner { } } - Ok((output, false)) + Ok((output, stderr_output, false)) } fn stdout_reader(child: &mut Child) -> Result, CommandError> { @@ -557,6 +591,71 @@ mod tests { ); } + #[cfg(windows)] + #[test] + fn large_capture_is_not_truncated() { + let runner = CommandRunner::new(); + let options = CommandOptions { + initial_delay: Duration::ZERO, + extra_args: vec![ + "-NoProfile".to_string(), + "-NonInteractive".to_string(), + "-Command".to_string(), + "$big = 'A' * 90000; Write-Output $big".to_string(), + ], + ..CommandOptions::default() + }; + + let result = runner.run("powershell.exe", None, &options).unwrap(); + + assert!(!result.timed_out); + assert_eq!(result.exit_code, Some(0)); + assert!( + result.text.len() >= 90_000, + "expected >= 90000 bytes, got {}", + result.text.len() + ); + } + + #[cfg(windows)] + #[test] + fn stderr_is_captured_separately_from_stdout() { + let runner = CommandRunner::new(); + let options = CommandOptions { + initial_delay: Duration::ZERO, + extra_args: vec![ + "-NoProfile".to_string(), + "-NonInteractive".to_string(), + "-Command".to_string(), + concat!( + "$ErrorActionPreference='Stop';", + "Write-Output 'stdout line';", + "Write-Error 'boom diagnostics'" + ) + .to_string(), + ], + ..CommandOptions::default() + }; + + let result = runner.run("powershell.exe", None, &options).unwrap(); + + assert_eq!(result.exit_code, Some(1)); + assert!(result.text.contains("stdout line"), "stdout: {}", result.text); + assert!( + result.stderr.contains("boom diagnostics"), + "stderr: {}", + result.stderr + ); + // PowerShell's error banner echoes the whole script line into + // stderr, so asserting stderr lacks "stdout line" would be wrong; + // instead assert stderr diagnostics never leaked into stdout. + assert!( + !result.text.contains("boom diagnostics"), + "stderr leaked into stdout: {}", + result.text + ); + } + #[test] fn test_error_display() { let err = CommandError::BinaryNotFound("codex".to_string());