feat: harden spec immutability#968
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces defensive copying and immutability for collection fields across several record classes and builders in the org.a2aproject.sdk.spec package using List.copyOf and Map.copyOf. The review feedback suggests further refining these changes by replacing @Nullable Boolean with a primitive boolean in MessageSendConfiguration and utilizing explicit canonical constructors in CancelTaskParams to avoid redundant null checks on record accessors when non-null default values are guaranteed.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
be8709f to
1151c80
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive copying of collections and maps across various specification records (such as AgentCard, AgentSkill, Artifact, and others) to ensure immutability, along with comprehensive unit tests to verify this behavior. The review feedback correctly points out a critical issue: using Map.copyOf on arbitrary metadata or parameter maps will throw a NullPointerException if any values are null, which is common in JSON-based protocols. It is recommended to use Collections.unmodifiableMap(new HashMap<>(...)) instead to safely preserve immutability while supporting null values.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
i will fix it |
or no
what do you think? @kabir
|
|
@Sh1bari I see this in the javadoc for
However, my local Claude instance says that there are other occurrences of |
1151c80 to
231dfc5
Compare
|
@kabir done |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces defensive copying and immutability enhancements across various record classes in the SDK specification (such as AgentCard, AgentCapabilities, and CancelTaskParams) to prevent external mutation of internal collections and maps. It also adds a comprehensive suite of unit tests to verify these immutability behaviors. The review feedback highlights potential NullPointerException risks in CancelTaskParams when metadata is null, recommending graceful handling by defaulting to empty maps in both the compact constructor and the builder.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
@jmesnil Can you please take a look? |
| @@ -0,0 +1,109 @@ | |||
| package org.a2aproject.sdk.server.tasks; | |||
There was a problem hiding this comment.
is this test related to the spec hardening?
There was a problem hiding this comment.
No, this test was accidentally deleted in feat: Implement MainEventBus architecture for event queue processing (#611). I just rollback it

231dfc5 to
19693d3
Compare
d478368 to
48d517f
Compare
jmesnil
left a comment
There was a problem hiding this comment.
the helper methods looks good except for the 2 method names that are a bit misleading.
|
Great, I’ll update the PR within an hour. |
4e3edb9 to
ff2853e
Compare
|
@jmesnil Done |
ff2853e to
d5fff38
Compare
84fd427 to
93761dc
Compare
|
ITK image build is broken. |
|
@Sh1bari yeah, I think the ITK looks broken, and not related to this |
Change indentation in CollectionCopies to use spaces, not tabs
feat: fix metadata and error details to allow null map values
|
@kabir merged, maybe it makes sense to add a checkstyle to follow conventions and automatically format the code? Or am I missing something? |
|
@Sh1bari Checkstyle could be good, we use that in https://github.com/wildfly/wildfly but haven't got around to setting that up here. Last year or so I worked on another project which I can't find now, where we used some other formatter which I can't remember (since I can't find the repository!). I think that wrapped the Google Code Formatter, but I found it a bit too opinionated and intrusive |


Spec cleanup: immutable records and copy semantics
Build fails after
actions/checkoutwas bumped from v6 to v7 in PR #948:#948