fix(tck): resolve capability test failures for extensions and push notifications#804
fix(tck): resolve capability test failures for extensions and push notifications#804sherryfox wants to merge 1 commit intoa2aproject:0.3.x-compatfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates Kafka to version 4.2.0, enhances the deployment scripts with a PostgreSQL pod creation wait loop, and refactors JSON-RPC handling to improve validation and spec compliance. It introduces check() methods for request validation and a custom TypeAdapterFactory to ensure JSON-RPC 2.0 compliance for responses. Feedback was provided regarding the inconsistent validation of the mandatory jsonrpc field in streaming requests and potential precision loss when serializing numeric IDs.
…tifications - Default `extensions` to an empty list in `AgentCapabilities` to ensure it serializes as an array, fixing `test_capabilities_structure`. - Save `pushNotificationConfig` in `onMessageSend` for non-streaming messages, fixing `test_send_message_with_push_notification_config`
2c46514 to
bd65c70
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements push notification configuration storage within DefaultRequestHandler and adds a null-safety check for extensions in the AgentCapabilities record. Review feedback identifies a potential NullPointerException risk when taskId is null due to its use as a key in a ConcurrentHashMap. Additionally, it is recommended to use List.copyOf in the AgentCapabilities constructor to ensure the list is unmodifiable and defensively copied.
| if (shouldAddPushInfo(params)) { | ||
| pushConfigStore.setInfo(taskResult.getId(), params.configuration().pushNotificationConfig()); | ||
| } | ||
| if (taskId != null && !taskId.equals(taskResult.getId())) { |
There was a problem hiding this comment.
While adding the taskId != null check prevents a NullPointerException during the comparison, taskId being null (which occurs for new tasks) still poses a significant risk. Specifically, taskId is used as a key for runningAgents (a ConcurrentHashMap) at lines 242, 256, and 342. Since ConcurrentHashMap does not allow null keys, these operations will throw a NullPointerException before this line is even reached. Consider ensuring a non-null task ID is available (e.g., by generating a temporary one) before interacting with runningAgents.
| List<AgentExtension> extensions) { | ||
|
|
||
| public AgentCapabilities { | ||
| extensions = extensions == null ? List.of() : extensions; |
There was a problem hiding this comment.
Using List.copyOf is preferred here to ensure the extensions list is unmodifiable and to provide a defensive copy if a mutable list was passed to the constructor. This maintains the immutability of the record and aligns with the pattern used in other classes like Task.
| extensions = extensions == null ? List.of() : extensions; | |
| extensions = extensions == null ? List.of() : List.copyOf(extensions); |
extensionsto an empty list inAgentCapabilitiesto ensure it serializes as an array, fixingtest_capabilities_structure.pushNotificationConfiginonMessageSendfor non-streaming messages, fixingtest_send_message_with_push_notification_configFailures before the fixes:
After the fixes all tests are passing: