Repository navigation
feat(errors): add structured error codes and details to A2A error types - #743
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the error handling mechanism across gRPC and REST transports. The move from brittle string-matching on error descriptions to a structured approach using A2AErrorCodes and a details map is a major improvement for robustness and maintainability. The introduction of the A2AErrorCodes enum and the adoption of standard error formats (Google Cloud API for REST and ErrorInfo for gRPC) are excellent changes.
The refactoring has been applied consistently across the codebase, including the specification classes, transport handlers, and utility classes. The tests have also been thoroughly updated to reflect and validate the new structured error handling.
I've found a potential bug in the fallback error mapping in GrpcErrorMapper that could lead to a NullPointerException, and a couple of minor improvements in JSONRPCUtils regarding Gson instance reuse, which also align with project rules on consistency. Please see the detailed comments.
Overall, this is a high-quality contribution that greatly improves the project's error handling infrastructure.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable refactoring of the error handling mechanism, moving from brittle string-matching to a structured, code-based approach. The introduction of A2AErrorCodes, the use of google.rpc.ErrorInfo in gRPC, and the adoption of a consistent Google Cloud API-style error format for REST are all excellent improvements that will enhance the robustness and maintainability of the error handling logic. My review focuses on a few areas where the new implementation could be made even more robust by ensuring the new structured error details are fully preserved across the transport layer and that error codes are mapped with appropriate specificity.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request is a significant and valuable feature enhancement that refactors error handling across the A2A protocol. The move from brittle string-matching to a structured approach using the A2AErrorCodes enum and a details map is a major improvement for robustness and maintainability. Aligning gRPC and REST error responses with standard formats like ErrorInfo and the Google Cloud API error model is also a great step forward. The changes are consistently applied, and the tests are well-updated to reflect the new error handling mechanism. I've identified one critical issue in the GrpcErrorMapper that appears to be a compilation error due to a generics type mismatch, which I've detailed in a specific comment.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a significant and valuable refactoring of the error handling mechanism across gRPC, REST, and JSON-RPC transports. By replacing brittle string-matching with a structured approach using the new A2AErrorCodes enum and a details map in error types, the system becomes much more robust and maintainable. The alignment of gRPC and REST error formats with the Google Cloud API style is a solid design choice. The changes are extensive and well-executed throughout the codebase, including the necessary updates to tests.
My review identified one potential issue in the client-side REST error mapper where parsing error metadata could fail for non-string values. A fix is suggested to ensure robust parsing.
| JsonObject metaObj = detail.getAsJsonObject("metadata"); | ||
| Map<String, Object> metaMap = new HashMap<>(); | ||
| for (var entry : metaObj.entrySet()) { | ||
| metaMap.put(entry.getKey(), entry.getValue().getAsString()); |
There was a problem hiding this comment.
The current implementation for parsing metadata assumes all values are strings by using entry.getValue().getAsString(). This will cause an IllegalStateException if the metadata contains non-string values like numbers, booleans, or nested objects. The RestHandler serializes the details map from A2AError, which is a Map<String, Object>, so other JSON types are possible in the response.
To fix this, you should deserialize the JsonElement to a generic Object using JsonUtil.OBJECT_MAPPER, which correctly handles various JSON types.
| metaMap.put(entry.getKey(), entry.getValue().getAsString()); | |
| metaMap.put(entry.getKey(), JsonUtil.OBJECT_MAPPER.fromJson(entry.getValue(), Object.class)); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the A2A error handling mechanism by replacing the generic data field in A2AError with a structured details map and converting A2AErrorCodes into an enum that centralizes error code mappings to gRPC and HTTP statuses. The JSON-RPC and REST transport layers are updated to leverage this new standardized error structure, including adopting the Google Cloud API error format for REST responses. A bug was identified where the RestErrorMapper incorrectly parsed metadata values as strings, potentially causing ClassCastException or data loss; this should be fixed by using JsonUtil.OBJECT_MAPPER for proper deserialization. Additionally, the HTTP status code for UNSUPPORTED_OPERATION should be updated to 501 (Not Implemented) for better alignment with its gRPC counterpart and to resolve an inconsistency in RestHandler.mapErrorToHttpStatus.
| Map<String, Object> metaMap = new HashMap<>(); | ||
| for (var entry : metaObj.entrySet()) { | ||
| metaMap.put(entry.getKey(), entry.getValue().getAsString()); | ||
| } | ||
| metadata = metaMap; |
There was a problem hiding this comment.
The current implementation of metadata parsing forces all values to be strings using entry.getValue().getAsString(). This can lead to loss of data structure for complex metadata objects and may cause a ClassCastException if a value is not a primitive. Since the server-side RestHandler now serializes a Map<String, Object> for metadata, the client should deserialize it accordingly to preserve the structure.
I suggest using JsonUtil.OBJECT_MAPPER to correctly deserialize the entire metadata object at once. This is cleaner and ensures that complex nested objects within the metadata are handled correctly.
| Map<String, Object> metaMap = new HashMap<>(); | |
| for (var entry : metaObj.entrySet()) { | |
| metaMap.put(entry.getKey(), entry.getValue().getAsString()); | |
| } | |
| metadata = metaMap; | |
| metadata = JsonUtil.OBJECT_MAPPER.fromJson(metaObj, Map.class); |
|
|
||
| /** Error code indicating the requested operation is not supported (-32004). */ | ||
| int UNSUPPORTED_OPERATION_ERROR_CODE = -32004; | ||
| UNSUPPORTED_OPERATION(-32004, "UNIMPLEMENTED", 400), |
There was a problem hiding this comment.
The HTTP status code for UNSUPPORTED_OPERATION should be 501 (Not Implemented) to better align with its meaning and the corresponding UNIMPLEMENTED gRPC status. The current value of 400 (Bad Request) is less specific. This change will also resolve an inconsistency in RestHandler.mapErrorToHttpStatus.
| UNSUPPORTED_OPERATION(-32004, "UNIMPLEMENTED", 400), | |
| UNSUPPORTED_OPERATION(-32004, "UNIMPLEMENTED", 501), |
References
- Adhere to the project's specification for gRPC error code mappings. For instance,
VersionNotSupportedErrorshould be mapped toUNIMPLEMENTEDas per the spec, even if other options seem more consistent. This comment suggests correcting the HTTP status code forUNSUPPORTED_OPERATIONto 501 to align with the gRPCUNIMPLEMENTEDstatus, ensuring adherence to project-specific error mapping specifications.
There was a problem hiding this comment.
The A2A specifications define this mapping: cf. https://a2a-protocol.org/v1.0.0/specification/#54-error-code-mappings
Replace brittle string-matching error detection in gRPC and REST transports with a structured approach using error codes (A2AErrorCodes) and a details field. The GrpcErrorMapper now extracts ErrorInfo from gRPC status details via a REASON_MAP lookup, and error types carry richer context through a dedicated details field. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors A2A error handling across gRPC, REST, and JSON-RPC transport layers to standardize error representation and mapping. Key changes include converting A2AErrorCodes from an interface to an enum with associated gRPC status and HTTP codes, replacing the generic data field in A2AError with a structured details map, and updating JSON serialization/deserialization to handle this new details format. The gRPC error mapper now prioritizes extracting detailed google.rpc.ErrorInfo for more precise error mapping, falling back to status code mapping if not available. Similarly, the REST error mapper now uses a Google Cloud API error format for responses, extracting reason and metadata from error details. All related error classes and tests have been updated to reflect these changes, ensuring consistent and detailed error reporting across all communication protocols.
…es (a2aproject#743) Replace brittle string-matching error detection in gRPC and REST transports with a structured approach using error codes (A2AErrorCodes) and a details field. The GrpcErrorMapper now extracts ErrorInfo from gRPC status details via a REASON_MAP lookup, and error types carry richer context through a dedicated details field. 🦕 Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
3 participants
Replace brittle string-matching error detection in gRPC and REST transports with a structured approach using error codes (A2AErrorCodes) and a details field. The GrpcErrorMapper now extracts ErrorInfo from gRPC status details via a REASON_MAP lookup, and error types carry richer context through a dedicated details field.
🦕