From 51af631aecac6fad4e23cff0e00c0436c906b3ae Mon Sep 17 00:00:00 2001 From: howie-mac-mini <2318485+howie@users.noreply.github.com> Date: Fri, 22 May 2026 10:19:37 +0800 Subject: [PATCH] fix(acp): surface JSON-RPC error data.details to users MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `JsonRpcError` previously deserialized only `code` and `message`, silently dropping the optional `data` field defined in JSON-RPC 2.0 §5.1. ACP adapters such as codex-acp put actionable detail strings in `data.details` (model id, auth hint, peer-dep failure, etc.), so every -32603 reaches users as the same opaque "Internal Error" regardless of cause. This change: - adds `data: Option` to `JsonRpcError` with a `data_details()` accessor mirroring the codex-acp / acpx convention, - threads the full error through `format_coded_error`, appending `data.details` when present and not already echoed in `message`, - updates the synthetic "connection closed" error in connection.rs and the call site in adapter.rs accordingly. Backward compatible: errors emitted by agents that don't set `data` deserialize unchanged (`#[serde(default)]`), and existing display formatting is preserved when `data.details` is absent. --- src/acp/connection.rs | 1 + src/acp/protocol.rs | 88 +++++++++++++++++++++++++++- src/adapter.rs | 2 +- src/error_display.rs | 133 +++++++++++++++++++++++++++++++++++++----- 4 files changed, 208 insertions(+), 16 deletions(-) diff --git a/src/acp/connection.rs b/src/acp/connection.rs index 90c0eae2..57f36e4f 100644 --- a/src/acp/connection.rs +++ b/src/acp/connection.rs @@ -246,6 +246,7 @@ pub(crate) async fn run_reader_loop( error: Some(crate::acp::protocol::JsonRpcError { code: -1, message: "connection closed".into(), + data: None, }), params: None, }); diff --git a/src/acp/protocol.rs b/src/acp/protocol.rs index 40dfdf07..73daba53 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 diagnostic payload per JSON-RPC 2.0 §5.1. ACP adapters + // (codex-acp in particular) put actionable detail in `data.details` + // — without it, every -32603 surfaces to the user as the same opaque + // "Internal Error" regardless of cause (model deprecation, auth + // failure, missing peer dep, …). See `data_details()`. + #[serde(default)] + pub data: Option, +} + +impl JsonRpcError { + /// Returns the `data.details` string if `data` is an object with a string + /// `details` field. Mirrors the codex-acp / acpx convention: + /// + pub fn data_details(&self) -> Option<&str> { + self.data.as_ref()?.get("details")?.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) + match self.data_details() { + Some(d) if !d.is_empty() && !self.message.contains(d) => { + write!(f, "JSON-RPC error {}: {} ({})", self.code, self.message, d) + } + _ => write!(f, "JSON-RPC error {}: {}", self.code, self.message), + } } } @@ -382,4 +403,69 @@ mod tests { assert_eq!(opts.len(), 1); assert_eq!(opts[0].id, "model"); } + + // ─── JsonRpcError data passthrough ────────────────────────────────────── + + #[test] + fn jsonrpc_error_deserializes_without_data_field() { + // Backward compat: pre-existing agents that emit just {code, message} + // must still deserialize cleanly. + let raw = json!({"code": -32602, "message": "Invalid params"}); + let err: JsonRpcError = serde_json::from_value(raw).unwrap(); + assert_eq!(err.code, -32602); + assert_eq!(err.message, "Invalid params"); + assert!(err.data.is_none()); + assert!(err.data_details().is_none()); + } + + #[test] + fn jsonrpc_error_deserializes_with_data_details() { + // codex-acp shape observed in the wild + let raw = json!({ + "code": -32603, + "message": "Internal error", + "data": {"details": "model 'gpt-5.2-codex' is no longer supported"} + }); + let err: JsonRpcError = serde_json::from_value(raw).unwrap(); + assert_eq!(err.code, -32603); + assert_eq!( + err.data_details(), + Some("model 'gpt-5.2-codex' is no longer supported") + ); + } + + #[test] + fn jsonrpc_error_data_details_is_none_for_non_string() { + let raw = json!({ + "code": -32603, + "message": "Internal error", + "data": {"details": 42} + }); + let err: JsonRpcError = serde_json::from_value(raw).unwrap(); + assert!(err.data_details().is_none()); + assert!(err.data.is_some(), "raw data is preserved for callers"); + } + + #[test] + fn jsonrpc_error_display_appends_details_when_present() { + let err = JsonRpcError { + code: -32603, + message: "Internal error".into(), + data: Some(json!({"details": "model deprecated"})), + }; + assert_eq!( + err.to_string(), + "JSON-RPC error -32603: Internal error (model deprecated)" + ); + } + + #[test] + fn jsonrpc_error_display_omits_details_when_absent() { + let err = JsonRpcError { + code: -32602, + message: "Invalid params".into(), + data: None, + }; + assert_eq!(err.to_string(), "JSON-RPC error -32602: Invalid params"); + } } diff --git a/src/adapter.rs b/src/adapter.rs index c8a2be45..29ac9262 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)); } break; } diff --git a/src/error_display.rs b/src/error_display.rs index b4e3a850..6c1e9135 100644 --- a/src/error_display.rs +++ b/src/error_display.rs @@ -51,8 +51,13 @@ 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. /// Public for reuse by other adapters (e.g. Slack). -pub fn format_coded_error(code: i64, message: &str) -> String { - let prefix = match code { +/// +/// When the upstream JSON-RPC error carries `data.details` (codex-acp / +/// acpx convention), the detail string is appended so users can tell +/// apart causes that otherwise share a single code+label (e.g. -32603 +/// "Internal Error" for model deprecation vs. auth vs. peer-dep failure). +pub fn format_coded_error(err: &crate::acp::protocol::JsonRpcError) -> String { + let prefix = match err.code { 400 => "**Bad Request**", 401 => "**Unauthorized**", 403 => "**Forbidden**", @@ -70,11 +75,19 @@ pub fn format_coded_error(code: i64, message: &str) -> String { -32099..=-32000 => "**Server Error**", _ => "**Error**", }; - if message.is_empty() { - format!("{} (code: {})", prefix, code) - } else { - format!("{} (code: {})\n{}", prefix, code, message) - } + let head = format!("{} (code: {})", prefix, err.code); + let details = err.data_details().unwrap_or("").trim(); + + let body = match (err.message.as_str(), details) { + ("", "") => return head, + ("", d) => d.to_string(), + (m, "") => m.to_string(), + // Avoid duplicate text when the adapter already echoed `details` + // into `message` (some agents do this for legacy clients). + (m, d) if m.contains(d) => m.to_string(), + (m, d) => format!("{}\n{}", m, d), + }; + format!("{}\n{}", head, body) } #[cfg(test)] @@ -164,9 +177,28 @@ mod tests { // ─── format_coded_error tests ─────────────────────────────────────────── + use crate::acp::protocol::JsonRpcError; + use serde_json::json; + + fn err(code: i64, message: &str) -> JsonRpcError { + JsonRpcError { + code, + message: message.into(), + data: None, + } + } + + fn err_with_data(code: i64, message: &str, data: serde_json::Value) -> JsonRpcError { + JsonRpcError { + code, + message: message.into(), + data: Some(data), + } + } + #[test] fn test_format_coded_error_401() { - let result = format_coded_error(401, "invalid token"); + let result = format_coded_error(&err(401, "invalid token")); assert!(result.contains("Unauthorized")); assert!(result.contains("401")); assert!(result.contains("invalid token")); @@ -174,7 +206,7 @@ mod tests { #[test] fn test_format_coded_error_429() { - let result = format_coded_error(429, ""); + let result = format_coded_error(&err(429, "")); assert!(result.contains("Rate Limited")); assert!(result.contains("429")); assert!(!result.contains("\n")); // no message, no newline @@ -182,7 +214,7 @@ mod tests { #[test] fn test_format_coded_error_503() { - let result = format_coded_error(503, "service unavailable"); + let result = format_coded_error(&err(503, "service unavailable")); assert!(result.contains("Service Unavailable")); assert!(result.contains("503")); assert!(result.contains("service unavailable")); @@ -190,30 +222,103 @@ mod tests { #[test] fn test_format_coded_error_json_rpc() { - let result = format_coded_error(-32602, "missing required parameter"); + let result = format_coded_error(&err(-32602, "missing required parameter")); 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(&err(-32050, "internal failure")); 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(&err(-32000, "connection refused")); 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(&err(999, "something happened")); assert!(result.contains("Error")); assert!(result.contains("999")); assert!(result.contains("something happened")); } + + // ─── data.details surfacing (new behavior) ───────────────────────────── + + #[test] + fn test_format_coded_error_appends_data_details() { + // codex-acp model deprecation: message is generic, details has model id + let result = format_coded_error(&err_with_data( + -32603, + "Internal error", + json!({"details": "model 'gpt-5.2-codex' is no longer supported"}), + )); + assert!(result.contains("Internal Error")); + assert!(result.contains("-32603")); + assert!(result.contains("Internal error")); + assert!(result.contains("gpt-5.2-codex")); + } + + #[test] + fn test_format_coded_error_details_only_when_message_empty() { + let result = format_coded_error(&err_with_data( + -32603, + "", + json!({"details": "query closed before response received"}), + )); + assert!(result.contains("Internal Error")); + assert!(result.contains("query closed before response received")); + } + + #[test] + fn test_format_coded_error_no_duplicate_when_message_contains_details() { + // Some adapters echo details into message for legacy clients + let result = format_coded_error(&err_with_data( + -32603, + "Internal error: session timed out", + json!({"details": "session timed out"}), + )); + assert_eq!(result.matches("session timed out").count(), 1); + } + + #[test] + fn test_format_coded_error_ignores_non_string_details() { + let result = format_coded_error(&err_with_data( + -32603, + "Internal error", + json!({"details": {"nested": "object"}}), + )); + assert!(result.contains("Internal error")); + assert!(!result.contains("nested")); + assert!(!result.contains("object")); + } + + #[test] + fn test_format_coded_error_ignores_unknown_data_shape() { + let result = format_coded_error(&err_with_data( + -32603, + "Internal error", + json!({"unrelated": "field"}), + )); + assert!(result.contains("Internal error")); + assert!(!result.contains("unrelated")); + } + + #[test] + fn test_format_coded_error_handles_empty_details_string() { + let result = format_coded_error(&err_with_data( + -32603, + "Internal error", + json!({"details": " "}), + )); + // Whitespace-only details must not produce a trailing blank line. + assert!(!result.ends_with('\n')); + assert!(result.contains("Internal error")); + } }