From 0ab54616b6f4ec4c86ffa4d082bc3b63cfb3d1db Mon Sep 17 00:00:00 2001 From: Finesssee <90105158+Finesssee@users.noreply.github.com> Date: Sat, 29 Aug 2026 14:38:43 +0700 Subject: [PATCH 1/2] fix: preserve process exit code in CommandRunner after stream EOF Windows PowerShell tears down its stdout/stderr handles a few milliseconds before the process object transitions to signaled, so the single try_wait() in finish_child() saw Ok(None) even though the process had exited with code 0. The old teardown path killed the just-finished child and returned a None exit code, which made agent session discovery report 'Windows process discovery failed; verify PowerShell and CIM access' on every scan while discarding the perfectly valid ~30-84KB CIM JSON payload (issue #396). finish_child() now polls try_wait() on a bounded 250ms grace period before falling back to kill/teardown, preserving the real exit code. Also improve the discovery failure message to surface the actual exit code and a redacted stderr tail instead of the generic hint, per the reporter's ask, and add a regression test that captures >64KB of PowerShell output end-to-end. --- rust/src/agent_sessions.rs | 36 +++++++++++++++----- rust/src/host/command_runner.rs | 60 ++++++++++++++++++++++++++++----- 2 files changed, 79 insertions(+), 17 deletions(-) diff --git a/rust/src/agent_sessions.rs b/rust/src/agent_sessions.rs index 52f7b35c62..d1c62649ec 100644 --- a/rust/src/agent_sessions.rs +++ b/rust/src/agent_sessions.rs @@ -382,21 +382,41 @@ impl LocalAgentSessionScanner { 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."), + Some( + "Windows process discovery timed out; file-only sessions may still appear." + .to_string(), + ), ), 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.", - ), - ), + Ok(result) => { + let stderr_tail = crate::logging::safe_error_message(result.text); + let stderr_tail = stderr_tail + .lines() + .rev() + .take(3) + .collect::>() + .join("; "); + 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.", + 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(), ), ), }; diff --git a/rust/src/host/command_runner.rs b/rust/src/host/command_runner.rs index a2c0beff10..8125c6859a 100755 --- a/rust/src/host/command_runner.rs +++ b/rust/src/host/command_runner.rs @@ -251,18 +251,35 @@ 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, + } + if remaining.is_zero() { + break; } - Err(_) => None, + let step = remaining.min(Duration::from_millis(10)); + std::thread::sleep(step); + remaining -= step; } + // Teardown after timeout/capture: kill/wait results cannot + // change the outcome, already reported separately. + let _killed = child.kill(); + let _reaped = child.wait(); + None } /// Capture output from a running process @@ -557,6 +574,31 @@ mod tests { ); } + #[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() + ); + } + #[test] fn test_error_display() { let err = CommandError::BinaryNotFound("codex".to_string()); From d22f99e071485f241aab231b522897e7e9bcbf3a Mon Sep 17 00:00:00 2001 From: Finesssee <90105158+Finesssee@users.noreply.github.com> Date: Sat, 29 Aug 2026 15:43:02 +0700 Subject: [PATCH 2/2] Capture stderr, surface parse errors, human-readable exit codes --- rust/src/agent_sessions.rs | 49 +++++++++++++++---- rust/src/agent_sessions/parsers.rs | 22 ++++++--- rust/src/agent_sessions/tests.rs | 52 +++++++++++++++++++- rust/src/host/command_runner.rs | 77 ++++++++++++++++++++++++++---- 4 files changed, 173 insertions(+), 27 deletions(-) diff --git a/rust/src/agent_sessions.rs b/rust/src/agent_sessions.rs index d1c62649ec..c35477a662 100644 --- a/rust/src/agent_sessions.rs +++ b/rust/src/agent_sessions.rs @@ -388,16 +388,19 @@ impl LocalAgentSessionScanner { ), ), Ok(result) if result.exit_code == Some(0) => { - (WindowsProcessOutputParser::parse(&result.text), None) + 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 = crate::logging::safe_error_message(result.text); - let stderr_tail = stderr_tail - .lines() - .rev() - .take(3) - .collect::>() - .join("; "); + let stderr_tail = Self::error_tail(&result.stderr); let hint = if stderr_tail.is_empty() { String::new() } else { @@ -406,9 +409,10 @@ impl LocalAgentSessionScanner { ( Vec::new(), Some(format!( - "Windows process discovery failed with exit code {:?}{}; \ + "Windows process discovery failed with exit code {}{}; \ file-only sessions may still appear.", - result.exit_code, hint + Self::exit_code_label(result.exit_code), + hint )), ) } @@ -456,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 8125c6859a..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, }) @@ -275,21 +281,31 @@ impl CommandRunner { std::thread::sleep(step); remaining -= step; } - // Teardown after timeout/capture: kill/wait results cannot - // change the outcome, already reported separately. - let _killed = child.kill(); - let _reaped = child.wait(); - None + // 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); @@ -298,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; @@ -309,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); @@ -332,7 +349,7 @@ impl CommandRunner { } } - Ok((output, false)) + Ok((output, stderr_output, false)) } fn stdout_reader(child: &mut Child) -> Result, CommandError> { @@ -574,6 +591,7 @@ mod tests { ); } + #[cfg(windows)] #[test] fn large_capture_is_not_truncated() { let runner = CommandRunner::new(); @@ -599,6 +617,45 @@ mod tests { ); } + #[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());