diff --git a/src/acp/connection.rs b/src/acp/connection.rs index 90c0eae2..8df3451f 100644 --- a/src/acp/connection.rs +++ b/src/acp/connection.rs @@ -121,6 +121,7 @@ pub struct AcpConnection { pub last_active: Instant, pub session_reset: bool, _reader_handle: JoinHandle<()>, + _stderr_handle: Option>, } /// Build the final set of env vars for the agent subprocess. @@ -246,6 +247,7 @@ pub(crate) async fn run_reader_loop( error: Some(crate::acp::protocol::JsonRpcError { code: -1, message: "connection closed".into(), + data: None, }), params: None, }); @@ -269,7 +271,7 @@ impl AcpConnection { cmd.args(args) .stdin(std::process::Stdio::piped()) .stdout(std::process::Stdio::piped()) - .stderr(std::process::Stdio::null()) + .stderr(std::process::Stdio::piped()) .current_dir(working_dir); // Create a new process group so we can kill the entire tree. // SAFETY: setpgid is async-signal-safe (POSIX.1-2008) and called @@ -355,6 +357,36 @@ impl AcpConnection { let stdin = proc.stdin.take().ok_or_else(|| anyhow!("no stdin"))?; let stdin = Arc::new(Mutex::new(stdin)); + // Capture agent stderr and log it (ACP spec: agents MAY write to stderr + // for logging; clients MAY capture or ignore it). + let stderr_handle = if let Some(stderr) = proc.stderr.take() { + let cmd_name = command.to_string(); + Some(tokio::spawn(async move { + let mut reader = BufReader::new(stderr); + let mut line = String::new(); + loop { + line.clear(); + match reader.read_line(&mut line).await { + Ok(0) => break, + Ok(_) => { + let trimmed = line.trim(); + if !trimmed.is_empty() { + let sanitized: String = trimmed.chars() + .filter(|c| !c.is_control() || *c == '\t') + .collect(); + if !sanitized.is_empty() { + tracing::warn!(agent = %cmd_name, "{sanitized}"); + } + } + } + Err(_) => break, + } + } + })) + } else { + None + }; + let pending: Arc>>> = Arc::new(Mutex::new(HashMap::new())); let notify_tx: Arc>>> = @@ -380,6 +412,7 @@ impl AcpConnection { last_active: Instant::now(), session_reset: false, _reader_handle: reader_handle, + _stderr_handle: stderr_handle, }) } @@ -657,6 +690,9 @@ impl AcpConnection { impl Drop for AcpConnection { fn drop(&mut self) { + if let Some(handle) = self._stderr_handle.take() { + handle.abort(); + } self.kill_process_group(); } } diff --git a/src/acp/protocol.rs b/src/acp/protocol.rs index 40dfdf07..099d98b7 100644 --- a/src/acp/protocol.rs +++ b/src/acp/protocol.rs @@ -55,11 +55,32 @@ pub struct JsonRpcMessage { pub struct JsonRpcError { pub code: i64, pub message: String, + /// Optional structured data from the agent (JSON-RPC `error.data`). + /// Agents like codex-acp include `{"message": "...", "codex_error_info": "..."}`. + pub data: Option, +} + +impl JsonRpcError { + /// Extract a human-readable detail from `error.data.message` if present. + /// + /// The `"message"` key is a convention used by codex-acp and aligns with + /// common JSON-RPC practice, but is NOT mandated by the ACP spec. + /// Other agents may use `"detail"`, `"reason"`, etc. — extend here if needed. + pub fn data_message(&self) -> Option<&str> { + self.data + .as_ref() + .and_then(|d| d.get("message")) + .and_then(|m| m.as_str()) + } } impl std::fmt::Display for JsonRpcError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - write!(f, "JSON-RPC error {}: {}", self.code, self.message) + write!(f, "JSON-RPC error {}: {}", self.code, self.message)?; + if let Some(detail) = self.data_message() { + write!(f, " — {detail}")?; + } + Ok(()) } } diff --git a/src/adapter.rs b/src/adapter.rs index c8a2be45..baaf2c78 100644 --- a/src/adapter.rs +++ b/src/adapter.rs @@ -564,7 +564,7 @@ impl AdapterRouter { continue; } if let Some(ref err) = notification.error { - response_error = Some(format_coded_error(err.code, &err.message)); + response_error = Some(format_coded_error(err.code, &err.message, err.data_message())); } break; } diff --git a/src/error_display.rs b/src/error_display.rs index b4e3a850..e822f503 100644 --- a/src/error_display.rs +++ b/src/error_display.rs @@ -50,8 +50,9 @@ pub fn format_user_error(message: &str) -> String { /// Format coded error from ACP agent for display in Discord. /// Used for response errors that have a JSON-RPC or HTTP status code. +/// `data_message` is the optional detail extracted from `error.data.message`. /// Public for reuse by other adapters (e.g. Slack). -pub fn format_coded_error(code: i64, message: &str) -> String { +pub fn format_coded_error(code: i64, message: &str, data_message: Option<&str>) -> String { let prefix = match code { 400 => "**Bad Request**", 401 => "**Unauthorized**", @@ -70,11 +71,18 @@ pub fn format_coded_error(code: i64, message: &str) -> String { -32099..=-32000 => "**Server Error**", _ => "**Error**", }; - if message.is_empty() { + let mut out = if message.is_empty() { format!("{} (code: {})", prefix, code) } else { format!("{} (code: {})\n{}", prefix, code, message) + }; + if let Some(detail) = data_message { + if !detail.is_empty() && !message.contains(detail) { + out.push_str("\n> "); + out.push_str(detail); + } } + out } #[cfg(test)] @@ -166,7 +174,7 @@ mod tests { #[test] fn test_format_coded_error_401() { - let result = format_coded_error(401, "invalid token"); + let result = format_coded_error(401, "invalid token", None); assert!(result.contains("Unauthorized")); assert!(result.contains("401")); assert!(result.contains("invalid token")); @@ -174,7 +182,7 @@ mod tests { #[test] fn test_format_coded_error_429() { - let result = format_coded_error(429, ""); + let result = format_coded_error(429, "", None); assert!(result.contains("Rate Limited")); assert!(result.contains("429")); assert!(!result.contains("\n")); // no message, no newline @@ -182,7 +190,7 @@ mod tests { #[test] fn test_format_coded_error_503() { - let result = format_coded_error(503, "service unavailable"); + let result = format_coded_error(503, "service unavailable", None); assert!(result.contains("Service Unavailable")); assert!(result.contains("503")); assert!(result.contains("service unavailable")); @@ -190,30 +198,44 @@ mod tests { #[test] fn test_format_coded_error_json_rpc() { - let result = format_coded_error(-32602, "missing required parameter"); + let result = format_coded_error(-32602, "missing required parameter", None); assert!(result.contains("Invalid Params")); assert!(result.contains("-32602")); } #[test] fn test_format_coded_error_server_error_range() { - let result = format_coded_error(-32050, "internal failure"); + let result = format_coded_error(-32050, "internal failure", None); assert!(result.contains("Server Error")); assert!(result.contains("-32050")); } #[test] fn test_format_coded_error_connection_error() { - let result = format_coded_error(-32000, "connection refused"); + let result = format_coded_error(-32000, "connection refused", None); assert!(result.contains("Server Error")); // -32000 falls in -32099..=-32000 range assert!(result.contains("-32000")); } #[test] fn test_format_coded_error_unknown_code() { - let result = format_coded_error(999, "something happened"); + let result = format_coded_error(999, "something happened", None); assert!(result.contains("Error")); assert!(result.contains("999")); assert!(result.contains("something happened")); } + + #[test] + fn test_format_coded_error_with_data_message() { + let result = format_coded_error(-32603, "Internal error", Some("model not supported")); + assert!(result.contains("Internal Error")); + assert!(result.contains("model not supported")); + } + + #[test] + fn test_format_coded_error_data_message_not_duplicated() { + // If data_message is already in message, don't repeat it + let result = format_coded_error(-32603, "model not supported", Some("model not supported")); + assert_eq!(result.matches("model not supported").count(), 1); + } }