fix: support complete agent card URL in A2ACardResolver#837
Conversation
There was a problem hiding this comment.
Code Review
The pull request enhances A2ACardResolver to accept either a base URL or a complete agent card URL, updating the internal URL construction logic to handle these variations correctly. It also introduces several unit tests to validate URL resolution with tenants, custom paths, and authentication headers, while improving error handling in the test suite. Feedback was provided to address a potential double-slash issue when concatenating the base URL and the agent card path.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the A2ACardResolver to support base URLs that already include the agent card path, ensuring the resolver uses such URLs verbatim. The URL construction logic was modified to handle manual concatenation when URI.resolve() is insufficient, and comprehensive unit tests were added to cover new functionality and error cases. Feedback was provided regarding a potential double-slash issue in the manual URL concatenation logic, along with a suggestion for a more robust normalization approach.
jmesnil
left a comment
There was a problem hiding this comment.
I'm not sure if we should make the card resolver more complex.
I see 2 use case for fetching an agent card:
- use the spec's well-known endpoint that is appended to whatever the baseUrl is (it can be http://example.com, http://example.com/agents/v1/foo)
- point to a specific URL that returns an agent card. This does not have to end with
.well-known/.agent-card.json. This URL can be fetched from a registry for example. In that case, there is no URL resolution at all, it's used verbatim.
So the API would be:
- A2AResolver(String url) (and all its variant)
- AgentCard getWellKnownAgentCard() -> append `.well-known/.agent-card.json to the base URL
- AgentCard getAgentCard() -> use the URL verbatim
That's be a API break change so maybe we could have just a new method:
- AgentCard getAgentCard(boolean fromWellKnownLocation)
- getAgentCard() delegates to getAgentCard(true)
- the resolution of the URL is deferred until getAgentCard(boolean) is called to figure out which URL to connect to.
|
@jmesnil I broke the API because I think it is cleaner and clearer that way |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the A2ACardResolver utility to use a builder pattern and introduces separate methods for retrieving well-known and configured agent cards. It also adds logging, improves URL resolution logic, and updates relevant tests and examples. Feedback was provided regarding the authHeader builder method, which could throw an UnsupportedOperationException if an immutable map is used, and a suggestion was made to normalize the Authorization header casing for consistency.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the A2ACardResolver to use a builder pattern and introduces distinct methods for fetching well-known versus configured agent cards. It includes URL normalization logic to handle trailing slashes and path prefixes more robustly, along with enhanced logging and updated test coverage. Feedback was provided regarding the resolveAgentCardUrl method, noting that its path-matching guard is currently hardcoded to the default path and should instead dynamically check against the specific path being resolved to avoid incorrect results when custom paths are used.
URI.resolve() silently drops intermediate path segments when the resolved path starts with '/', causing incorrect URLs for agents hosted under a path prefix (e.g. /spec03). Replace with direct concatenation and add a pass-through guard when the supplied URL already ends with the agent card path. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
URI.resolve() silently drops intermediate path segments when the
resolved path starts with '/', causing incorrect URLs for agents
hosted under a path prefix (e.g. /spec03). Replace with direct
concatenation and add a pass-through guard when the supplied URL
already ends with the agent card path.
Fixes #771 🦕