Skip to content

Conversation

@ShivaReddyVanja
Copy link
Contributor

Problem

Fixed a critical bug where user messages containing a payload property were incorrectly identified as protocol-wrapped messages, causing data corruption.
The original implementation only checked for the presence of payload to determine if a message was wrapped, leading to false positives when user data legitimately contained a payload field.

Solution

Enhanced protocol detection to require both payload AND version properties, plus validation that payload is a plain object (not array/null/primitive).

Changes:

  • Enhanced hasValidPayload check to require both 'payload' AND 'version'
  • Added isObject() helper to exclude arrays and null values
  • Refactored message normalization into dedicated normalizeMessage() method
  • Added comprehensive test suite with 11 test cases covering all edge cases

The fix ensures that only messages with the full protocol signature (version + payload as object) are treated as wrapped messages.

✅ Checklist

  • I have followed every step in the contributing guide
  • The PR title follows the convention.
  • I ran and tested the code works

Testing

✅ Added a new test file covering 11 cases
✅ All 11 new tests passing
✅ All 392 existing core tests passing

Changelog

Fixes: Protocol ambiguity where user data with 'payload' field was misinterpreted
Added Tests: 11/11 passing, including edge cases for non-object payloads and missing version

Screenshots

Screenshot 2026-01-16 at 2 22 36 PM Screenshot 2026-01-16 at 2 24 49 PM

💯

…rruption

Fixed a critical bug where user messages containing a 'payload' property
were incorrectly identified as protocol-wrapped messages, causing data loss.

Changes:
- Enhanced hasValidPayload check to require both 'payload' AND 'version'
- Added isObject() helper to exclude arrays and null values
- Refactored message normalization into dedicated normalizeMessage() method
- Added comprehensive test suite with 11 test cases covering all edge cases

The fix ensures that only messages with the full protocol signature
(version + payload as object) are treated as wrapped messages.

Fixes: Protocol ambiguity where user data with 'payload' field was misinterpreted
Tests: 11/11 passing, including edge cases for non-object payloads and missing version
@changeset-bot
Copy link

changeset-bot bot commented Jan 16, 2026

⚠️ No Changeset found

Latest commit: 25f3fea

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jan 16, 2026

Caution

Review failed

The pull request is closed.

Walkthrough

This pull request refactors message handling in ZodSocketMessageHandler by introducing an internal normalization layer. The change consolidates how different incoming message formats are processed into a consistent internal representation (NormalizedMessage), replacing previous conditional logic with a unified normalization function. A comprehensive test suite is added covering protocol detection, payload handling, validation, error scenarios, and callback integration.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 7a7c4b1 and 25f3fea.

📒 Files selected for processing (2)
  • packages/core/src/v3/zodSocket.ts
  • packages/core/test/zodSocket.test.ts

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


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 and usage tips.

@ericallam
Copy link
Member

@ShivaReddyVanja appreciate the PR but this is the type of change that we can't really accept. The amount of risk changing something like this is too high, and would require extensive testing

@ericallam ericallam closed this Jan 16, 2026
@ShivaReddyVanja
Copy link
Contributor Author

ok, i completely understand . Is there any other area that i can help, happy to fix !

@vibe-kanban-cloud
Copy link

Review Complete

Your review story is ready!

View Story

Comment !reviewfast on this PR to re-generate the story.

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.

2 participants