feat: Add GetExtendedAgentCard Support to RequestHandlers#919
feat: Add GetExtendedAgentCard Support to RequestHandlers#919sokoliva wants to merge 6 commits intoa2aproject:1.0-devfrom
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 refactors the management of AgentCard and its modifiers by centralizing them within the RequestHandler architecture, removing the need to pass these objects individually to gRPC, REST, and JSON-RPC handlers. It also introduces centralized capability validation using an updated validate decorator and adds a specific error for missing push notification configurations. Feedback highlights a regression in the validate decorator which now fails to correctly wrap standard coroutine functions, as well as a violation of the style guide regarding the direct raising of exception classes.
🧪 Code Coverage (vs
|
| Base | PR | Delta | |
|---|---|---|---|
| src/a2a/client/transports/http_helpers.py | 94.44% | 90.28% | 🔴 -4.17% |
| src/a2a/server/request_handlers/default_request_handler.py | 96.66% | 93.53% | 🔴 -3.13% |
| src/a2a/server/request_handlers/grpc_handler.py | 94.61% | 94.34% | 🔴 -0.27% |
| src/a2a/server/routes/jsonrpc_dispatcher.py | 86.32% | 86.84% | 🟢 +0.53% |
| src/a2a/server/routes/jsonrpc_routes.py | 66.67% | 62.50% | 🔴 -4.17% |
| src/a2a/server/routes/rest_dispatcher.py | 92.43% | 94.64% | 🟢 +2.21% |
| src/a2a/utils/helpers.py | 97.19% | 97.46% | 🟢 +0.27% |
| Total | 91.58% | 91.48% | 🔴 -0.10% |
Generated by coverage-comment.yml
| @validate( | ||
| lambda self: self.agent_card.capabilities.extended_agent_card, | ||
| error_message='The agent does not have an extended agent card configured', | ||
| error_type=ExtendedAgentCardNotConfiguredError, | ||
| ) |
There was a problem hiding this comment.
According to spec, it should return ExtendedAgentCardNotConfiguredError if there is no extended_agent_card_defined.
If it is not supported the error is UnsupportedOperationError
| if not self._push_config_store: | ||
| raise PushNotificationNotSupportedError | ||
|
|
There was a problem hiding this comment.
I would leave this check here and below to avoid the ignore[union-attr]
| @property | ||
| @abstractmethod | ||
| def agent_card(self) -> AgentCard: | ||
| """The core agent card to serve logic against.""" | ||
|
|
There was a problem hiding this comment.
Why did you add this? I don't think it should be needed
| # This is needed for typechecker to recognise this method as an async generator. | ||
| if False: | ||
| yield |
There was a problem hiding this comment.
Let's leave it as it was for now, and tackle this in a separate PR maybe
| # This is needed for typechecker to recognise the method as an async generator. | ||
| if False: | ||
| yield |
| normalized_path = rpc_url | ||
| if not rpc_url.startswith('/'): | ||
| logger.warning( | ||
| "The rpc_url '%s' does not start with a leading slash '/'. automatically prepending it.", | ||
| rpc_url, | ||
| ) | ||
| normalized_path = f'/{rpc_url}' | ||
|
|
There was a problem hiding this comment.
let's not modify the rpc_url, if the user passes one without a leading / the server will not start showing an error
| context = self._build_call_context(request) | ||
| card_to_serve = await maybe_await( | ||
| self.extended_card_modifier(card_to_serve, context) | ||
| async def _handler( |
There was a problem hiding this comment.
Add the version validation also on this method
| return config | ||
|
|
||
| raise InternalError(message='Push notification config not found') | ||
| raise PushNotificationConfigNotFoundError |
There was a problem hiding this comment.
it should return TaskNotFoundError
https://a2a-protocol.org/latest/specification/#318-get-push-notification-config
| class PushNotificationConfigNotFoundError(A2AError): | ||
| """Exception raised when a push notification configuration is not found.""" | ||
|
|
||
| message = 'Push notification config not found' | ||
|
|
||
|
|
There was a problem hiding this comment.
this is not defined anywhere in the specifications
| finally: | ||
| await inner.aclose() | ||
|
|
||
| return async_gen_wrapper |
There was a problem hiding this comment.
This might cause a FAIL-LATE error corrupting the code behaviour, let's discuss tomorrow what could be the best way to handle this
2 participants
Description
The
GetExtendedAgentCardcapability was defined in the spec but not implemented in therequest_handler.py.Changes
on_get_extended_agent_cardto the baseRequestHandlerand its child classDefaultRequestHandler.GetExtendedAgentCardmethod implementations from the Transport layer and consequently movedAgentCardinformations from the Transport layer to theRequestHandlers.validatelogic from the transport layer to request handlerFixes
Fixes #866 🦕