Skip to content

feat: harden spec immutability#968

Merged
kabir merged 3 commits into
a2aproject:mainfrom
Sh1bari:feat/spec-record-immutability
Jul 13, 2026
Merged

feat: harden spec immutability#968
kabir merged 3 commits into
a2aproject:mainfrom
Sh1bari:feat/spec-record-immutability

Conversation

@Sh1bari

@Sh1bari Sh1bari commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

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 gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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 Sh1bari force-pushed the feat/spec-record-immutability branch 5 times, most recently from be8709f to 1151c80 Compare July 4, 2026 03:35
@kabir

kabir commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/AgentCardSignature.java Outdated
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/AgentExtension.java Outdated
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/Artifact.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/MessageSendParams.java Outdated
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/TaskArtifactUpdateEvent.java Outdated
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/TaskStatusUpdateEvent.java Outdated
@Sh1bari

Sh1bari commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

i will fix it

@Sh1bari

Sh1bari commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author
image or no what do you think? @kabir

@kabir

kabir commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

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

Screenshot 2026-07-06 at 14 59 15

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 Sh1bari force-pushed the feat/spec-record-immutability branch from 1151c80 to 231dfc5 Compare July 6, 2026 15:32
@Sh1bari

Sh1bari commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

@kabir done

@kabir

kabir commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

/gemini review

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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
@kabir kabir requested a review from jmesnil July 6, 2026 16:17
@kabir

kabir commented Jul 6, 2026

Copy link
Copy Markdown
Collaborator

@jmesnil Can you please take a look?

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this test related to the spec hardening?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/AgentCard.java Outdated
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/TaskStatusUpdateEvent.java Outdated
@Sh1bari Sh1bari force-pushed the feat/spec-record-immutability branch from 231dfc5 to 19693d3 Compare July 7, 2026 08:45
@Sh1bari Sh1bari requested a review from jmesnil July 7, 2026 09:05
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/AgentCard.java Outdated
@Sh1bari Sh1bari force-pushed the feat/spec-record-immutability branch from d478368 to 48d517f Compare July 7, 2026 18:23
@Sh1bari Sh1bari requested a review from jmesnil July 8, 2026 12:14

@jmesnil jmesnil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the helper methods looks good except for the 2 method names that are a bit misleading.

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

@jmesnil jmesnil left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sh1bari Helper methods look good, thanks

@Sh1bari Sh1bari requested a review from jmesnil July 8, 2026 13:31
@Sh1bari

Sh1bari commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

Great, I’ll update the PR within an hour.

@Sh1bari Sh1bari force-pushed the feat/spec-record-immutability branch from 4e3edb9 to ff2853e Compare July 8, 2026 17:05
@Sh1bari

Sh1bari commented Jul 8, 2026

Copy link
Copy Markdown
Contributor Author

@jmesnil Done

Comment thread spec/src/main/java/org/a2aproject/sdk/spec/AgentSkill.java Outdated
Comment thread spec/src/main/java/org/a2aproject/sdk/spec/MessageSendParams.java Outdated
@Sh1bari Sh1bari force-pushed the feat/spec-record-immutability branch from ff2853e to d5fff38 Compare July 9, 2026 17:54
@Sh1bari Sh1bari requested a review from jmesnil July 9, 2026 17:57
@Sh1bari Sh1bari force-pushed the feat/spec-record-immutability branch 2 times, most recently from 84fd427 to 93761dc Compare July 11, 2026 12:59
@Sh1bari

Sh1bari commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

ITK image build is broken.
1.912 Reading state information...
1.915 Package curl is not available, but is referred to by another package.
1.915 This may mean that the package is missing, has been obsoleted, or
1.915 is only available from another source
1.915
1.917 E: Version '7.88.1-10+deb12u14' for 'curl' was not found

@kabir

kabir commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

@Sh1bari yeah, I think the ITK looks broken, and not related to this

Change indentation in CollectionCopies to use spaces, not tabs
@kabir

kabir commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Hi @Sh1bari I opened Sh1bari#2 against your repo with fixes suggested by Claude

feat: fix metadata and error details to allow null map values
@Sh1bari

Sh1bari commented Jul 13, 2026

Copy link
Copy Markdown
Contributor Author

@kabir merged, maybe it makes sense to add a checkstyle to follow conventions and automatically format the code? Or am I missing something?

@kabir

kabir commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

@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

@kabir kabir merged commit 5d56ed5 into a2aproject:main Jul 13, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants