feat: harden spec immutability by Sh1bari · Pull Request #968 · a2aproject/a2a-java

Sh1bari · 2026-07-04T01:07:37Z

Spec cleanup: immutable records and copy semantics

Build fails after actions/checkout was bumped from v6 to v7 in PR #948:
#948

gemini-code-assist

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.

kabir · 2026-07-06T13:36:57Z

/gemini review

gemini-code-assist

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.

gemini-code-assist · 2026-07-06T13:40:33Z

    public AgentCardSignature {
        Assert.checkNotNullParam("protectedHeader", protectedHeader);
        Assert.checkNotNullParam("signature", signature);
+        header = header == null ? null : Map.copyOf(header);


Using Map.copyOf on arbitrary header maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in metadata/header maps. If a client sends a payload with a null value in the header, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

header = header == null ? null : Map.copyOf(header);

header = header == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(header));

gemini-code-assist · 2026-07-06T13:40:33Z

     */
    public AgentExtension {
        Assert.checkNotNullParam("uri", uri);
+        params = params == null ? null : Map.copyOf(params);


Using Map.copyOf on arbitrary parameter maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in parameters. If a client sends a payload with a null value in the parameters, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

params = params == null ? null : Map.copyOf(params);

params = params == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(params));

gemini-code-assist · 2026-07-06T13:40:33Z

            throw new IllegalArgumentException("Parts cannot be empty");
        }
+        parts = List.copyOf(parts);
+        metadata = metadata == null ? null : Map.copyOf(metadata);


Using Map.copyOf on arbitrary metadata maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in metadata. If a client sends a payload with a null value in the metadata, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

metadata = metadata == null ? null : Map.copyOf(metadata);

metadata = metadata == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata));

gemini-code-assist · 2026-07-06T13:40:33Z

     */
    public CancelTaskParams {
        Assert.checkNotNullParam("id", id);
+        metadata = Map.copyOf(metadata);


Using Map.copyOf on arbitrary metadata maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in metadata. If a client sends a payload with a null value in the metadata, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

metadata = Map.copyOf(metadata);

metadata = java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata));

gemini-code-assist · 2026-07-06T13:40:33Z

     */
    public MessageSendParams {
        Assert.checkNotNullParam("message", message);
+        metadata = metadata == null ? null : Map.copyOf(metadata);


Using Map.copyOf on arbitrary metadata maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in metadata. If a client sends a payload with a null value in the metadata, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

metadata = metadata == null ? null : Map.copyOf(metadata);

metadata = metadata == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata));

gemini-code-assist · 2026-07-06T13:40:33Z

        Assert.checkNotNullParam("taskId", taskId);
        Assert.checkNotNullParam("artifact", artifact);
        Assert.checkNotNullParam("contextId", contextId);
+        metadata = metadata == null ? null : Map.copyOf(metadata);


Using Map.copyOf on arbitrary metadata maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in metadata. If a client sends a payload with a null value in the metadata, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

metadata = metadata == null ? null : Map.copyOf(metadata);

metadata = metadata == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata));

gemini-code-assist · 2026-07-06T13:40:33Z

        Assert.checkNotNullParam("taskId", taskId);
        Assert.checkNotNullParam("status", status);
        Assert.checkNotNullParam("contextId", contextId);
+        metadata = metadata != null ? Map.copyOf(metadata) : null;


Using Map.copyOf on arbitrary metadata maps (like Map<String, Object>) is risky because Map.copyOf throws a NullPointerException if the map contains any null values. In JSON-based protocols, null values are common and valid in metadata. If a client sends a payload with a null value in the metadata, deserialization will fail with a NullPointerException. Using Collections.unmodifiableMap(new HashMap<>(...)) is a safer alternative that preserves immutability while supporting null values.

Suggested change

metadata = metadata != null ? Map.copyOf(metadata) : null;

metadata = metadata != null ? java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata)) : null;

Sh1bari · 2026-07-06T13:42:38Z

i will fix it

Sh1bari · 2026-07-06T13:45:36Z

or no what do you think? @kabir

kabir · 2026-07-06T14:02:50Z

@Sh1bari I see this in the javadoc for Map.copyOf so it might have a point

However, my local Claude instance says that there are other occurrences of Map.copyOIf in Message, Task, TextPart, FilePart and DataPart which are not touched by this PR. But I think you can update the ones you changed, and I can do the rest at some point if it becomes an issue

Sh1bari · 2026-07-06T15:33:36Z

@kabir done

kabir · 2026-07-06T15:54:59Z

/gemini review

gemini-code-assist

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.

kabir · 2026-07-06T16:17:26Z

@jmesnil Can you please take a look?

jmesnil · 2026-07-07T08:16:13Z

@@ -0,0 +1,109 @@
+package org.a2aproject.sdk.server.tasks;


is this test related to the spec hardening?

No, this test was accidentally deleted in feat: Implement MainEventBus architecture for event queue processing (#611). I just rollback it

jmesnil · 2026-07-07T08:17:48Z

+        defaultInputModes = List.copyOf(defaultInputModes);
+        defaultOutputModes = List.copyOf(defaultOutputModes);
+        skills = List.copyOf(skills);
+        securitySchemes = securitySchemes == null ? null : Map.copyOf(securitySchemes);


There are cases where you use nullability?: + Map.copyOf, others with Collections.unmodifiableMap(new HashMap<> with or without nullabillity?: and it's not clear why these choices were made.

The code would be more readable if these was using helper methods that would describe the hardening choices.

is it? I was not asking to use Collections.unmodifiableMap instead of copyOf.
I was suggesting to use helper methods to give more helpful information on the initialization of the data structures of the spec (eg this field can be safely copied, this one requires to use an unmodifiable map because null entry and/or values are allowed).

+1 to using helper methods.

FYI Map.copyOf() has some issues if any of the keys/values are null, so we were steered towards using Collections.unmodifiableMap()

spec/src/main/java/org/a2aproject/sdk/util/CollectionCopies.java

Pushed a utility class with helper methods for the defensive-copy strategies. Could you please take a look?

I propose using this helper to make the intent explicit at call sites. If the approach looks good to you, I’ll update the usages in this PR to use it.
@jmesnil @kabir

jmesnil · 2026-07-07T13:06:06Z

-import java.util.Collections;
-import java.util.List;
-import java.util.Map;
+import java.util.*;


star imports are not allowed

gemini-code-assist Bot reviewed Jul 4, 2026

View reviewed changes

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/MessageSendConfiguration.java Outdated

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/CancelTaskParams.java Outdated

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/CancelTaskParams.java

Sh1bari force-pushed the feat/spec-record-immutability branch 5 times, most recently from be8709f to 1151c80 Compare July 4, 2026 03:35

gemini-code-assist Bot reviewed Jul 6, 2026

View reviewed changes

Sh1bari force-pushed the feat/spec-record-immutability branch from 1151c80 to 231dfc5 Compare July 6, 2026 15:32

gemini-code-assist Bot reviewed Jul 6, 2026

View reviewed changes

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/CancelTaskParams.java

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/CancelTaskParams.java

kabir approved these changes Jul 6, 2026

View reviewed changes

kabir requested a review from jmesnil July 6, 2026 16:17

jmesnil reviewed Jul 7, 2026

View reviewed changes

Sh1bari force-pushed the feat/spec-record-immutability branch from 231dfc5 to 19693d3 Compare July 7, 2026 08:45

feat: harden spec immutability

19693d3

Sh1bari requested a review from jmesnil July 7, 2026 09:05

jmesnil reviewed Jul 7, 2026

View reviewed changes

Sh1bari force-pushed the feat/spec-record-immutability branch from d478368 to 48d517f Compare July 7, 2026 18:23

feat: harden spec immutability

48d517f

	header = header == null ? null : Map.copyOf(header);
	header = header == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(header));

	params = params == null ? null : Map.copyOf(params);
	params = params == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(params));

	metadata = metadata == null ? null : Map.copyOf(metadata);
	metadata = metadata == null ? null : java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata));

	metadata = Map.copyOf(metadata);
	metadata = java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata));

	metadata = metadata != null ? Map.copyOf(metadata) : null;
	metadata = metadata != null ? java.util.Collections.unmodifiableMap(new java.util.HashMap<>(metadata)) : null;

Uh oh!

Conversation

Sh1bari commented Jul 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kabir commented Jul 6, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist Bot Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

Sh1bari commented Jul 6, 2026

Uh oh!

Sh1bari commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

kabir commented Jul 6, 2026

Uh oh!

Sh1bari commented Jul 6, 2026

Uh oh!

kabir commented Jul 6, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

kabir commented Jul 6, 2026

Uh oh!

jmesnil Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Sh1bari Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

jmesnil Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Sh1bari Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

jmesnil Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

kabir Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

Sh1bari Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Sh1bari commented Jul 4, 2026 •

edited

Loading

Sh1bari commented Jul 6, 2026 •

edited

Loading

Sh1bari Jul 7, 2026 •

edited

Loading