fix: align error code mappings with upstream spec fix (A2A#1627)#58
fix: align error code mappings with upstream spec fix (A2A#1627)#58
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Code Review
This pull request refactors the A2AError structure to use a vector of values for error details instead of a hashmap, facilitating a transition to a typed-object array format. It also standardizes error code mappings across REST, gRPC, and SlimRPC interfaces, notably shifting several "Unimplemented" statuses to "Failed Precondition" and adjusting HTTP status codes. Feedback highlights that the current implementation filters out RestErrorInfo during parsing, leading to data loss, and suggests preserving these objects in the details list. Additionally, the reviewer noted that the new gRPC error mapping is lossy and recommended future implementation of gRPC error details to maintain specificity.
| for raw_detail in envelope.error.details { | ||
| if let Ok(info) = serde_json::from_value::<RestErrorInfo>(raw_detail.clone()) { | ||
| if info.type_url == REST_ERROR_INFO_TYPE_URL && info.domain == REST_ERROR_DOMAIN { | ||
| code = reason_to_error_code(&info.reason).or(code); | ||
| for (key, value) in info.metadata { | ||
| details.insert(key, Value::String(value)); | ||
| } | ||
| continue; | ||
| } | ||
| } | ||
|
|
||
| if let Value::Object(values) = raw_detail { | ||
| details.extend(values); | ||
| } | ||
| extra_details.push(raw_detail); | ||
| } |
There was a problem hiding this comment.
The RestErrorInfo object is being filtered out of the details vector after extracting the error code. This causes any metadata (such as a server-provided timestamp) or the domain field within that object to be lost. Since the protocol is moving towards an "array-of-typed-objects" format, it is more appropriate to keep the RestErrorInfo object in the details list so that all information sent by the server is preserved and accessible to the client.
for raw_detail in envelope.error.details {
if let Ok(info) = serde_json::from_value::<RestErrorInfo>(raw_detail.clone()) {
if info.type_url == REST_ERROR_INFO_TYPE_URL && info.domain == REST_ERROR_DOMAIN {
code = reason_to_error_code(&info.reason).or(code);
}
}
extra_details.push(raw_detail);
}| assert_eq!(details.len(), 1); | ||
| assert_eq!(details[0], serde_json::json!({"resource": "task"})); |
There was a problem hiding this comment.
Updating the test to reflect that the RestErrorInfo object is now preserved in the details vector.
| assert_eq!(details.len(), 1); | |
| assert_eq!(details[0], serde_json::json!({"resource": "task"})); | |
| assert_eq!(details.len(), 2); | |
| assert_eq!(details[1], serde_json::json!({"resource": "task"})); |
| @@ -31,7 +31,7 @@ pub fn status_to_a2a_error(status: &tonic::Status) -> A2AError { | |||
| let code = match status.code() { | |||
| tonic::Code::NotFound => error_code::TASK_NOT_FOUND, | |||
| tonic::Code::FailedPrecondition => error_code::TASK_NOT_CANCELABLE, | |||
There was a problem hiding this comment.
Mapping all FailedPrecondition gRPC errors back to TASK_NOT_CANCELABLE is now more ambiguous because several other error codes (such as PUSH_NOTIFICATION_NOT_SUPPORTED, UNSUPPORTED_OPERATION, and EXTENDED_CARD_NOT_CONFIGURED) also map to FailedPrecondition. This makes the conversion back to A2AError lossy. Consider implementing support for gRPC error details (e.g., google.rpc.ErrorInfo) in the future to preserve the specific error code during round-trips.
61523a0 to
1a654ca
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request standardizes error handling across the A2A protocol by introducing a structured TypedDetail system and centralizing error code mapping logic. It updates the REST, JSON-RPC, gRPC, and SlimRPC transports to support these detailed error responses, including field violations and protocol-specific error information. Feedback was provided to ensure that ErrorInfo details are correctly merged in the REST server implementation to avoid duplicates, maintaining consistency with the JSON-RPC implementation.
| let metadata = HashMap::from([("timestamp".to_string(), Utc::now().to_rfc3339())]); | ||
|
|
||
| let mut details = vec![ | ||
| serde_json::to_value(RestErrorInfo { | ||
| type_url: REST_ERROR_INFO_TYPE_URL, | ||
| reason, | ||
| domain: REST_ERROR_DOMAIN, | ||
| metadata, | ||
| }) | ||
| .expect("rest error info should serialize"), | ||
| ]; | ||
| let mut details = Vec::new(); | ||
|
|
||
| if !extra_details.is_empty() { | ||
| details.push(Value::Object(extra_details)); | ||
| if let Some(extra) = err.details { | ||
| details.extend(extra); | ||
| } | ||
|
|
||
| details.push(TypedDetail::error_info( | ||
| reason, | ||
| errordetails::PROTOCOL_DOMAIN, | ||
| Some(metadata), | ||
| )); |
There was a problem hiding this comment.
The current implementation appends a new ErrorInfo detail without checking if one already exists in err.details. This can lead to duplicate ErrorInfo objects in the response, especially in proxy or gateway scenarios where the A2AError might have been parsed from an upstream response. Additionally, it doesn't merge metadata from any existing ErrorInfo into the new one.
It is better to filter out existing ErrorInfo details and merge their metadata into the new one, ensuring a single, consistent ErrorInfo at the end of the details array. This aligns the REST transport behavior with the JSON-RPC implementation in A2AError::to_jsonrpc_error.
let mut metadata = HashMap::from([("timestamp".to_string(), Utc::now().to_rfc3339())]);
let mut details = Vec::new();
if let Some(extra) = err.details {
for detail in extra {
if detail.type_url == errordetails::ERROR_INFO_TYPE {
if let Some(Value::Object(meta)) = detail.value.get("metadata") {
for (k, v) in meta {
if let Some(s) = v.as_str() {
metadata.entry(k.clone()).or_insert_with(|| s.to_string());
}
}
}
} else {
details.push(detail);
}
}
}
details.push(TypedDetail::error_info(
reason,
errordetails::PROTOCOL_DOMAIN,
Some(metadata),
));3ac32dd to
19f90c9
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request standardizes error handling across the library by introducing a new errordetails module and refactoring the A2AError struct to support structured error details like ErrorInfo and BadRequest. The changes include updated error code mappings for REST, gRPC, and SlimRPC transports, along with improved JSON-RPC error parsing. Feedback suggests refining the parse_jsonrpc_error implementation to collect and format multiple field violations more cleanly, ensuring consistency with the REST transport's logic.
| let mut code = err.code; | ||
| let mut message = err.message; | ||
|
|
||
| if let Some(ref details) = details { | ||
| for detail in details { | ||
| match detail.type_url.as_str() { | ||
| errordetails::BAD_REQUEST_TYPE => { | ||
| if let Some(Value::Array(violations)) = detail.value.get("fieldViolations") { | ||
| let violation_strs: Vec<String> = violations | ||
| .iter() | ||
| .filter_map(|v| { | ||
| let field = v.get("field")?.as_str()?; | ||
| let desc = v.get("description")?.as_str()?; | ||
| Some(format!("{field}: {desc}")) | ||
| }) | ||
| .collect(); | ||
| if !violation_strs.is_empty() { | ||
| message = format!("{}: {}", message, violation_strs.join("; ")); | ||
| } | ||
| } | ||
| if code == error_code::INTERNAL_ERROR { | ||
| code = error_code::INVALID_PARAMS; | ||
| } | ||
| } | ||
| errordetails::ERROR_INFO_TYPE => { | ||
| if let Some(Value::String(domain)) = detail.value.get("domain") { | ||
| if domain == errordetails::PROTOCOL_DOMAIN { | ||
| if let Some(Value::String(reason)) = detail.value.get("reason") { | ||
| if let Some(c) = reason_to_error_code(reason) { | ||
| code = c; | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The current implementation of parse_jsonrpc_error appends field violations to the message string inside the loop. If multiple BadRequest details are present, this results in awkward formatting (e.g., "original message: field1: desc1: field2: desc2").
It is better to collect all violations from all details first and then append them once to the message using a consistent separator, matching the logic used in the REST transport.
let mut code = err.code;
let mut message = err.message;
let mut field_violations = Vec::new();
if let Some(ref details) = details {
for detail in details {
match detail.type_url.as_str() {
errordetails::BAD_REQUEST_TYPE => {
if let Some(Value::Array(violations)) = detail.value.get("fieldViolations") {
for v in violations {
if let Ok(fv) = serde_json::from_value::<FieldViolation>(v.clone()) {
field_violations.push(fv);
}
}
}
if code == error_code::INTERNAL_ERROR {
code = error_code::INVALID_PARAMS;
}
}
errordetails::ERROR_INFO_TYPE => {
if let Some(Value::String(domain)) = detail.value.get("domain") {
if domain == errordetails::PROTOCOL_DOMAIN {
if let Some(Value::String(reason)) = detail.value.get("reason") {
if let Some(c) = reason_to_error_code(reason) {
code = c;
}
}
}
}
}
_ => {}
}
}
}
if !field_violations.is_empty() {
let violations_str: Vec<String> = field_violations
.iter()
.map(|fv| format!("{}: {}", fv.field, fv.description))
.collect();
message = format!("{}: {}", message, violations_str.join("; "));
}Update gRPC/HTTP/REST error mappings to match the corrected google.rpc.Code-based table and change A2AError.details from HashMap<String, Value> to Vec<Value> to match the spec's array-of-typed-objects format for JSON-RPC error data. gRPC: PUSH_NOTIFICATION_NOT_SUPPORTED, UNSUPPORTED_OPERATION, and EXTENDED_CARD_NOT_CONFIGURED now map to FailedPrecondition (was Unimplemented). HTTP: TASK_NOT_CANCELABLE → 400 (was 409), CONTENT_TYPE_NOT_SUPPORTED → 400 (was 415). Ref: a2aproject/A2A@757f0ec Signed-off-by: Mauro Sardara <msardara@cisco.com>
19f90c9 to
ee77ca5
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request standardizes error handling across the A2A protocol by introducing a structured TypedDetail system. It adds a new errordetails module, refactors A2AError to support multiple typed details (like ErrorInfo and BadRequest), and updates the client, server, and various transport implementations (REST, JSON-RPC, gRPC, SlimRPC) to utilize this shared logic. Feedback focuses on improving the formatting of field violations when field names are empty, removing redundant error message strings in payload responses, and ensuring that metadata from all ErrorInfo details is correctly merged during JSON-RPC error conversion.
| let violation_strs: Vec<String> = violations | ||
| .iter() | ||
| .filter_map(|v| { | ||
| let field = v.get("field")?.as_str()?; | ||
| let desc = v.get("description")?.as_str()?; | ||
| Some(format!("{field}: {desc}")) | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
When a FieldViolation has an empty field name, the current formatting results in a leading colon and space (e.g., : description). This can lead to awkward error messages like message: : description. It is better to omit the field name and colon if the field is empty.
let violation_strs: Vec<String> = violations
.iter()
.filter_map(|v| {
let field = v.get("field")?.as_str()?;
let desc = v.get("description")?.as_str()?;
if field.is_empty() {
Some(desc.to_string())
} else {
Some(format!("{field}: {desc}"))
}
})
.collect();| description: error.to_string(), | ||
| }; | ||
| rest_error_response( | ||
| A2AError::invalid_params(format!("invalid request body: {error}")) |
There was a problem hiding this comment.
In rest_payload_error_response, the error message is constructed as format!("invalid request body: {error}"), and a FieldViolation is also added with the same {error} as the description. When the client receives this and calls a2a_error_from_details, it appends the violation description to the message, resulting in a redundant string like invalid request body: {error}: {error}. Using a more generic message here would avoid this duplication.
| A2AError::invalid_params(format!("invalid request body: {error}")) | |
| A2AError::invalid_params("invalid request body") |
| if let Some(details) = &self.details { | ||
| if let Some(existing) = details | ||
| .iter() | ||
| .find(|d| d.type_url == errordetails::ERROR_INFO_TYPE) | ||
| { | ||
| if let Some(Value::Object(meta)) = existing.value.get("metadata") { | ||
| if let Some(Value::Object(info_meta)) = error_info.value.get_mut("metadata") { | ||
| for (k, v) in meta { | ||
| info_meta.entry(k.clone()).or_insert_with(|| v.clone()); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The to_jsonrpc_error method currently only merges metadata from the first ErrorInfo detail it finds in self.details. If multiple ErrorInfo objects are present, metadata from subsequent ones will be lost. For consistency with the server-side implementation in a2a-server/src/rest.rs, it should iterate through and merge metadata from all existing ErrorInfo details.
| if let Some(details) = &self.details { | |
| if let Some(existing) = details | |
| .iter() | |
| .find(|d| d.type_url == errordetails::ERROR_INFO_TYPE) | |
| { | |
| if let Some(Value::Object(meta)) = existing.value.get("metadata") { | |
| if let Some(Value::Object(info_meta)) = error_info.value.get_mut("metadata") { | |
| for (k, v) in meta { | |
| info_meta.entry(k.clone()).or_insert_with(|| v.clone()); | |
| } | |
| } | |
| } | |
| } | |
| } | |
| if let Some(details) = &self.details { | |
| for existing in details | |
| .iter() | |
| .filter(|d| d.type_url == errordetails::ERROR_INFO_TYPE) | |
| { | |
| if let Some(Value::Object(meta)) = existing.value.get("metadata") { | |
| if let Some(Value::Object(info_meta)) = error_info.value.get_mut("metadata") { | |
| for (k, v) in meta { | |
| info_meta.entry(k.clone()).or_insert_with(|| v.clone()); | |
| } | |
| } | |
| } | |
| } | |
| } |
…fix-a2aprojecta2a1627 Signed-off-by: Mauro Sardara <msardara@cisco.com>
7cdf7bb to
09a8197
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request standardizes error handling across the A2A protocol by introducing a structured TypedDetail system, replacing the previous HashMap-based approach. Key changes include the addition of the errordetails module, refactoring A2AError to support multiple typed details, and updating error code mappings for REST, gRPC, and JSON-RPC transports to ensure consistency with the Go SDK. I have no feedback to provide as there were no review comments to assess.
Update gRPC/HTTP/REST error mappings to match the corrected google.rpc.Code-based table and change A2AError.details from HashMap<String, Value> to Vec to match the spec's array-of-typed-objects
format for JSON-RPC error data.
gRPC:
PUSH_NOTIFICATION_NOT_SUPPORTED → FailedPrecondition (was Unimplemented)
UNSUPPORTED_OPERATION → FailedPrecondition (was Unimplemented)
EXTENDED_CARD_NOT_CONFIGURED → FailedPrecondition (was Unimplemented)
HTTP:
TASK_NOT_CANCELABLE → 400 (was 409),
CONTENT_TYPE_NOT_SUPPORTED → 400 (was 415).
Ref: a2aproject/A2A@757f0ec
Fixes: #57