Skip to content

fix(ack-id): return reply nonce for response handshakes - #175

Open
Kewe63 wants to merge 1 commit into
agentcommercekit:mainfrom
Kewe63:fix-174-a2a-reply-nonce
Open

fix(ack-id): return reply nonce for response handshakes#175
Kewe63 wants to merge 1 commit into
agentcommercekit:mainfrom
Kewe63:fix-174-a2a-reply-nonce

Conversation

@Kewe63

@Kewe63 Kewe63 commented Aug 23, 2026

Copy link
Copy Markdown

Summary

Fixes #174

createA2AHandshakeMessage() returned payload.nonce for every handshake message.

For response handshakes, createA2AHandshakePayload({ requestNonce }) keeps the peer's original request nonce in payload.nonce and puts the newly generated nonce in payload.replyNonce. Returning payload.nonce meant callers received the old request nonce instead of the fresh reply nonce needed to correlate the next handshake leg.

This PR returns payload.replyNonce when present, while preserving the existing initiator behavior by falling back to payload.nonce.


Changes

  • Return payload.replyNonce ?? payload.nonce from createA2AHandshakeMessage().
  • Add a regression test for response handshakes with requestNonce.

How to Test

pnpm --filter @agentcommercekit/ack-id test -- src/a2a/sign-message.test.ts
pnpm --filter @agentcommercekit/ack-id test
pnpm --filter @agentcommercekit/ack-id build
pnpm run lint
pnpm run check:format
pnpm run check:packages
git diff --check

Note: pnpm run check was also attempted. Build passed, but the full parallel test run hit Vitest fork worker timeout/EPIPE errors across unrelated packages in this WSL environment. The focused package tests and repo lint/format/package checks passed.


Checklist

  • Tests pass — focused package tests green, new regression test added
  • Lint, format, and package checks pass
  • Follows Conventional Commits
  • Changes scoped to this fix only

Risk & Impact

Low. The fallback (payload.replyNonce ?? payload.nonce) preserves the existing initiator behavior byte-for-byte — only response handshakes with a replyNonce present see a behavior change, which is the exact case this fixes.

Type: 🐛 Bug fix
Fixes: #174

Summary by CodeRabbit

  • Bug Fixes

    • Improved handshake responses to correctly return the requested nonce when responding to an existing handshake.
    • Initial handshakes continue to generate and return a fresh nonce.
  • Tests

    • Added coverage verifying nonce handling for handshake responses.

@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 3c1a506e-fe6f-4a45-a375-819b433a6be0

📥 Commits

Reviewing files that changed from the base of the PR and between 0b8fdaa and 7dbf8a2.

📒 Files selected for processing (2)
  • packages/ack-id/src/a2a/sign-message.test.ts
  • packages/ack-id/src/a2a/sign-message.ts

Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review.


Walkthrough

The A2A handshake message now returns the fresh reply nonce for response handshakes. A regression test verifies this behavior when a request nonce is provided.

Changes

A2A handshake nonce handling

Layer / File(s) Summary
Select and validate the handshake nonce
packages/ack-id/src/a2a/sign-message.ts, packages/ack-id/src/a2a/sign-message.test.ts
The message returns payload.replyNonce when available and otherwise returns payload.nonce. The test verifies the fresh reply nonce for response handshakes.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to 7dbf8

This localized fix returns the fresh reply nonce for response handshakes while preserving initiator behavior, with regression coverage and normal checks reported as passing; no actionable merge-blocking risk remains.

Suggested reviewers: venables

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The code returns replyNonce for response handshakes, preserves nonce for initiators, and adds the required regression test for issue #174.
Out of Scope Changes check ✅ Passed All changes are limited to the requested handshake nonce fix and its regression test.
Docstring Coverage ✅ Passed Docstring check was indeterminate for this PR — some files could not be analyzed in time. Not blocking.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the fix to return the reply nonce for response handshakes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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.

[Bug]: A2A response handshakes return request nonce instead of fresh reply nonce

1 participant