feat(errors): add structured error codes and details to A2A error types#743
feat(errors): add structured error codes and details to A2A error types#743ehsavoie wants to merge 1 commit intoa2aproject:mainfrom
Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the error handling mechanisms across gRPC and REST transports by introducing a structured error system. By moving away from string-based error identification to explicit error codes and a detailed context map, the changes aim to provide more consistent, machine-readable, and robust error information. This improvement will streamline debugging, facilitate better error-driven logic, and ensure greater interoperability within the A2A protocol. Highlights
Changelog
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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.
client/transport/grpc/src/main/java/io/a2a/client/transport/grpc/GrpcErrorMapper.java
Outdated
Show resolved
Hide resolved
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 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.
| private static @Nullable String extractReason(Throwable e) { | ||
| try { | ||
| com.google.rpc.Status rpcStatus = StatusProto.fromThrowable(e); | ||
| if (rpcStatus != null) { | ||
| for (com.google.protobuf.Any detail : rpcStatus.getDetailsList()) { | ||
| if (detail.is(com.google.rpc.ErrorInfo.class)) { | ||
| com.google.rpc.ErrorInfo errorInfo = detail.unpack(com.google.rpc.ErrorInfo.class); | ||
| if ("a2a-protocol.org".equals(errorInfo.getDomain())) { | ||
| return errorInfo.getReason(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } catch (InvalidProtocolBufferException ignored) { | ||
| // Fall through to status code-based mapping | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
The extractReason method currently only extracts the reason string from the ErrorInfo details and discards the associated metadata map. This leads to a loss of the structured error details that are sent by the server.
To preserve this information, I recommend modifying this method to return both the reason and the metadata. You could use a simple record or a Pair for this. The call chain (mapGrpcError and mapByErrorCode) should then be updated to propagate this metadata into the details field of the created A2AError instances, ensuring no data is lost during error mapping.
| private static @org.jspecify.annotations.Nullable String extractReason(JsonObject errorObj) { | ||
| if (errorObj.has("details") && errorObj.get("details").isJsonArray()) { | ||
| var details = errorObj.getAsJsonArray("details"); | ||
| if (!details.isEmpty() && details.get(0).isJsonObject()) { | ||
| JsonObject detail = details.get(0).getAsJsonObject(); | ||
| if (detail.has("reason")) { | ||
| return detail.get("reason").getAsString(); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Similar to the issue in GrpcErrorMapper, this extractReason method only extracts the reason string from the error details object and ignores the metadata field within the same object. This results in the loss of structured error details sent by the server.
To fix this, this method should be updated to extract and return both the reason and the metadata map. Subsequently, mapRestErrorByReason should be modified to use this metadata when constructing the A2AError instances, ensuring the details are preserved.
| case EXTENDED_AGENT_CARD_NOT_CONFIGURED -> new A2AClientException(fullMessage, new ExtendedAgentCardNotConfiguredError(null, errorMessage, null)); | ||
| case EXTENSION_SUPPORT_REQUIRED -> new A2AClientException(fullMessage, new ExtensionSupportRequiredError(null, errorMessage, null)); | ||
| case VERSION_NOT_SUPPORTED -> new A2AClientException(fullMessage, new VersionNotSupportedError(null, errorMessage, null)); | ||
| case INVALID_REQUEST, JSON_PARSE -> new A2AClientException(fullMessage, new InvalidRequestError()); |
There was a problem hiding this comment.
The JSON_PARSE error code is being mapped to InvalidRequestError, which loses the specific error type. JSONParseError is a distinct class and should be instantiated here to maintain error specificity. This appears to be a regression from previous behavior.
This case should be split to handle INVALID_REQUEST and JSON_PARSE separately, instantiating InvalidRequestError and io.a2a.spec.JSONParseError respectively.
References
- Adhere to the project's specification for gRPC error code mappings, ensuring specific error types are instantiated to maintain error specificity.
| if (!error.getDetails().isEmpty()) { | ||
| error.getDetails().forEach((k, v) -> errorInfoBuilder.putMetadata(k, String.valueOf(v))); | ||
| } |
There was a problem hiding this comment.
Using String.valueOf(v) to convert all values from the details map to strings can lead to data loss or non-standard serialization for complex objects like nested maps or lists. The ErrorInfo metadata map only supports Map<String, String>, but to preserve the structure of complex details, it would be more robust to serialize non-string values to a standard format like JSON.
Consider checking the type of the value and serializing it to JSON if it's not a simple string. This would make the error details more reliably transferable, although it would require the client to handle potential JSON-encoded values in the metadata.
1 participant
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.
🦕