fix: reject messages with mismatching contextId and taskId#729
fix: reject messages with mismatching contextId and taskId#729ehsavoie 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 addresses a data integrity issue where 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 correctly fixes a validation issue by rejecting messages with a contextId that mismatches the taskId's context. The core logic change in DefaultRequestHandler is sound, and the addition of comprehensive tests across all transports (gRPC, JSON-RPC, REST) for both streaming and non-streaming scenarios is excellent. I have one suggestion to simplify the logic in one of the new integration tests for better readability.
| Throwable cause = error; | ||
| boolean foundInvalidParams = false; | ||
| while (cause != null && !foundInvalidParams) { | ||
| if (cause instanceof InvalidParamsError) { | ||
| foundInvalidParams = true; | ||
| } else if (cause instanceof A2AClientException && cause.getCause() instanceof InvalidParamsError) { | ||
| foundInvalidParams = true; | ||
| } | ||
| cause = cause.getCause(); | ||
| } | ||
| assertTrue(foundInvalidParams, | ||
| "Should receive InvalidParamsError for mismatching contextId and taskId"); |
There was a problem hiding this comment.
The logic to walk the cause chain to find the InvalidParamsError is more complex than necessary. The else if condition is redundant because the next iteration of the while loop would check cause.getCause() anyway. You can simplify this loop for better readability and maintainability.
Throwable cause = error;
boolean foundInvalidParams = false;
while (cause != null) {
if (cause instanceof InvalidParamsError) {
foundInvalidParams = true;
break;
}
cause = cause.getCause();
}
assertTrue(foundInvalidParams,
"Should receive InvalidParamsError for mismatching contextId and taskId");Validate that the contextId provided in a SendMessage request matches the contextId of the referenced task, returning InvalidParamsError when they differ. Previously, the mismatch was silently ignored and the request succeeded. Add tests covering both streaming and non-streaming paths across all transports. Signed-off-by: Emmanuel Hugonnet <ehugonne@redhat.com>
jmesnil
left a comment
jmesnil
left a comment
There was a problem hiding this comment.
The grpc, jsonrpc and rest tests are superfluous: they all end up using the same code path from the DefaultRequestHandler (for both streaming and non-streaming).
|
@jmesnil I agree BUT if somehow this would change we are covered on all protocols. |
|
Right but this issue is not about transports, it is about validation. The transport tests bring no value. They can stay but I don't think we have to necessarily add transports tests whenever an issue if found in a deeper layer of the SDK. |
|
Superseded by #728 |
2 participants
Validate that the contextId provided in a SendMessage request matches the contextId of the referenced task, returning InvalidParamsError when they differ. Previously, the mismatch was silently ignored and the request succeeded.
Add tests covering both streaming and non-streaming paths across all transports.
Fixes #723 🦕