feat(rest): implement RFC 9457 problem details for errors#771
feat(rest): implement RFC 9457 problem details for errors#771
Conversation
- Updates REST error handlers to return RFC 9457 compliant `application/problem+json` responses. - Adds `A2AErrorToTypeURI` and `A2AErrorToTitle` mappings for A2A protocol errors. - Adds a global StarletteHTTPException handler in the FastAPI app. - Wraps the `/well-known/agent.json` endpoint with `rest_error_handler`. - Updates unit tests to verify the new Problem Details response format. Refs #722
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 mechanism for REST endpoints by adopting the RFC 9457 Problem Details standard. This change provides a consistent, machine-readable format for API error responses, improving interoperability and client-side error processing. It centralizes error formatting, ensuring that both application-specific and generic HTTP exceptions adhere to the new standard. Highlights
Changelog
Activity
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 implements RFC 9457 for problem details in error responses, standardizing the error format to enhance the REST API's clarity and predictability for clients. However, the implementation in src/a2a/utils/error_handlers.py introduces a potential information exposure vulnerability by automatically including all fields from the error.data dictionary in the public API response, which could lead to the leakage of sensitive data. Additionally, there is a minor suggestion to improve the robustness of error logging.
| data = getattr(error, 'data', None) | ||
| if isinstance(data, dict): | ||
| for key, value in data.items(): | ||
| if key not in payload: | ||
| payload[key] = value |
There was a problem hiding this comment.
The _build_problem_details_response function now includes all fields from the error.data dictionary in the JSON response payload. This can lead to the exposure of internal state or sensitive information that was intended only for logging or internal processing. Previously, only the message and type were exposed. Blindly merging the data dictionary into the response payload violates the principle of least privilege and can result in unintended information disclosure.
| ', Data=' + str(getattr(error, 'data', '')) | ||
| if getattr(error, 'data', None) | ||
| else '', |
There was a problem hiding this comment.
The current logic for logging the data attribute of an error will not log falsy values (e.g., an empty dictionary {}), which might be valid data that should be logged. Using hasattr(error, 'data') is a more reliable way to check for the attribute's existence. This suggestion also simplifies the expression using an f-string for better readability.
| ', Data=' + str(getattr(error, 'data', '')) | |
| if getattr(error, 'data', None) | |
| else '', | |
| f", Data={error.data}" if hasattr(error, 'data') else '' |
ishymko
left a comment
ishymko
left a comment
There was a problem hiding this comment.
Nice work 👍
Left a few minor comments.
| @rest_error_handler | ||
| async def get_agent_card(request: Request) -> Response: | ||
| card = await self._adapter.handle_get_agent_card(request) | ||
| return JSONResponse(card) |
There was a problem hiding this comment.
Not sure it should be wrapped here, that's true that it can throw, but .well-known/agent-card.json is "outside" of transport in general, just one per server: 14.3. Well-Known URI Registration.
It's another question why things are wired up like this in the current SDK, I'll take a look separately, let's for now keep "static" agent card as is.
| 'detail': getattr(error, 'message', str(error)), | ||
| } | ||
|
|
||
| data = getattr(error, 'data', None) |
There was a problem hiding this comment.
Maybe let's add an explicit property data to A2AError? I believe that now we don't have it on any of the errors but that's true that the spec gives some good examples on what can be put there, like in 11.6. Error Handling.
We may also put a comment explicitly stating that this data is going to be transmitted unaltered in a transport-specific way (to address this).
| } | ||
|
|
||
|
|
||
| def _build_problem_details_response(error: A2AError) -> JSONResponse: |
There was a problem hiding this comment.
nit: maybe this one can return something typed shaped as a problem+json payload so that it can also handle non-A2A errors?
This way the code below
except Exception:
logger.exception('Unknown error occurred')
return JSONResponse(
content={
'type': 'about:blank',
'title': 'Internal Error',
'status': 500,
'detail': 'Unknown exception',
},
status_code=500,
media_type='application/problem+json',
)won't have to duplicate it JSONResponse(... can be used right away on the return value of this function.
| AuthenticatedExtendedCardNotConfiguredError: 400, | ||
| } | ||
|
|
||
| A2AErrorToTypeURI: dict[_A2AErrorType, str] = { |
There was a problem hiding this comment.
Here and below: should be possible to only put domain errors inheriting A2AError from the utils/errors.py to properly map them to problem+json.
This list actually reflects what should be handled, various JSONRPCXxx errors below are leftovers from former eras of this.
2 participants
Description
feat(rest): implement RFC 9457 problem details for errors
Updates REST error handlers to return RFC 9457 compliant
application/problem+jsonresponses.Adds
A2AErrorToTypeURIandA2AErrorToTitlemappings for A2A protocol errors.Adds a global StarletteHTTPException handler in the FastAPI app.
Wraps the
/well-known/agent.jsonendpoint withrest_error_handler.Updates unit tests to verify the new Problem Details response format.
Follow the
CONTRIBUTINGGuide.Make your Pull Request title in the https://www.conventionalcommits.org/ specification.
fix:which represents bug fixes, and correlates to a SemVer patch.feat:represents a new feature, and correlates to a SemVer minor.feat!:, orfix!:,refactor!:, etc., which represent a breaking change (indicated by the!) and will result in a SemVer major.Ensure the tests and linter pass (Run
bash scripts/format.shfrom the repository root to format)Appropriate docs were updated (if necessary)
Fixes #722 🦕