feat: AgentCardSignature#448
feat: AgentCardSignature#448bartek-gralewicz wants to merge 4 commits intoepic/1.0_breaking_changesfrom
Conversation
🧪 Code Coverage
Generated by coverage-comment.yml |
![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 introduces Agent Card signature support using JWS and JCS canonicalization, adding the jose library as a dependency. Key changes include new signature generation and verification utilities, updates to the client and server request handlers to support signed cards, and comprehensive tests. Feedback focuses on avoiding object mutation in the signer, improving the type safety of object cloning by using destructuring instead of JSON serialization, and refining the canonicalization logic to preserve array semantics and handle Date objects correctly.
…bgralewicz/agent_card_signature
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Agent Card signing and verification capabilities using JWS (JSON Web Signature) and JCS (JSON Canonicalization Scheme). It adds the jose library as a dependency, updates the Node.js engine requirement to version 20, and integrates signature generation and verification into the client and server request handlers. The review feedback identifies a critical issue where the signature generator mutates the internal state of the agent card, leading to an ever-growing list of signatures on the server. Additionally, it is recommended to remove diagnostic logging from the verification loop to avoid cluttering consumer logs and to update the test suite to verify immutability.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces Agent Card signature support using JWS and JCS canonicalization, integrating signing and verification utilities into both the client and server request handlers. Feedback focuses on ensuring the canonicalization logic correctly handles empty array elements and Date objects, avoiding the use of console logging in library code, and reconsidering the minimum Node.js version bump to maintain compatibility.
| if (Array.isArray(d)) { | ||
| const cleanedList = d.map((v) => cleanEmpty(v)); | ||
| return cleanedList.length > 0 ? cleanedList : null; | ||
| } |
There was a problem hiding this comment.
The cleanEmpty function uses map on arrays, which preserves the array length even if elements are "cleaned" to null. This results in arrays like [null, null] if the original contained empty strings. This is likely not the intended behavior for canonicalization. If the goal is to remove empty elements, you should also filter the results. However, please verify if the A2A specification allows changing array indices during canonicalization.
| if (Array.isArray(d)) { | |
| const cleanedList = d.map((v) => cleanEmpty(v)); | |
| return cleanedList.length > 0 ? cleanedList : null; | |
| } | |
| if (Array.isArray(d)) { | |
| const cleanedList = d.map((v) => cleanEmpty(v)).filter((v) => v !== null); | |
| return cleanedList.length > 0 ? cleanedList : null; | |
| } |
| }, | ||
| "engines": { | ||
| "node": ">=18" | ||
| "node": ">=20" |
There was a problem hiding this comment.
Bumping the minimum Node.js version to 20 is a breaking change for users on Node 18. While breaking changes are permissible if documented in the PR description, consider maintaining compatibility with Node 18 since the jose dependency (v6) supports it, unless Node 20 features are specifically required.
References
- Breaking changes to public interfaces are permissible if they are expected and explicitly documented in the pull request description.
| await jose.flattenedVerify(jws, publicKey); | ||
| return; // At least one valid signature found | ||
| } catch (error) { | ||
| console.debug('Signature verification on entry was not successful:', signatureEntry, error); |
There was a problem hiding this comment.
Avoid using console.debug in library code. It can be intrusive for consumers and doesn't provide a way to suppress or redirect the output. It's better to let the caller handle verification failures or provide a configurable logging mechanism.
| } | ||
|
|
||
| if (typeof d === 'object') { | ||
| if (d instanceof Date) return d; |
There was a problem hiding this comment.
The cleanEmpty function explicitly preserves Date objects, but jcsStringify (line 175) does not handle them specifically. Since Date objects have no enumerable properties, jcsStringify will serialize them as {}. If Date objects are expected in the AgentCard, they should be converted to ISO strings during the cleaning process to ensure correct canonicalization.
| if (d instanceof Date) return d; | |
| if (d instanceof Date) return d.toISOString(); |
1 participant
Description
Implementing the feature of
AgentCardSignature. The PR is based on #290.Important note
Most changes are ported 1-1. The main difference between the implementation here and the one on the #290 is that in the initial PR, the
agentCardhad rootsignaturesincremented. This resulted in constantly growingagentCardobject.In the #290 there were also unit tests to confirm that behavior but it seems like an undesired outcome. In this PR, using
generateAgentCardSignaturewill return a newagentCardwith incremented signatures instead of incrementing the original object.Fixes #289 🦕