Skip to content

fix(peer): reject event-stream meta that withEventMeta would throw on - #97

Merged
dinwwwh merged 1 commit into
mainfrom
claude/peer-event-validator-mismatch-802cf0
Sep 25, 2026
Merged

dinwwwh merged 1 commit into
mainfrom
claude/peer-event-validator-mismatch-802cf0

Conversation

@dinwwwh

@dinwwwh dinwwwh commented Sep 25, 2026

Copy link
Copy Markdown
Member

isPeerEventStreamMessage accepted event-stream messages that the peer consumer then rejected. A retry of -1 or 1.5, or an id/comment containing CR/LF, passed the validator, then threw EventStreamEncoderError from next() when the consumer attached meta to an object payload. That ended the receiver's for await loop. Such messages are now rejected up front by isClientPeerSendMessage / isServerPeerSendMessage, so callers drop them instead of losing the stream.

Fixes

  • Malformed retry, id and comments on received event-stream messages no longer crash the consuming iterator.
  • Core and peer now use one set of rules (new core exports isEventStreamMessageId / isEventStreamMessageRetry / isEventStreamMessageComment), so the validator can't drift from the encoder asserts again.

Notes for reviewers

  • Adds three public exports to @standard-server/core.
  • assertEventStreamMessageId / assertEventStreamMessageComment now also throw on non-string input (only reachable from untyped JS callers; previously the value was coerced to a string).
  • A conforming peer never sends these values, since outgoing meta is already validated by withEventMeta.

Testing

  • New predicate tests in core and rejection cases in peer; the peer cases fail against the old validator.
  • Full suite passes (1136 tests); tsc and eslint clean.

isPeerEventStreamMessage accepted any finite retry and any id/comment
string, but the consumer re-validates those fields via withEventMeta with
stricter rules (non-negative integer retry, no CR/LF). Messages that passed
the gate could then throw EventStreamEncoderError mid-stream.

Core now exports isEventStreamMessageId/Retry/Comment predicates, which the
encoder asserts and the peer validator both use, so the rules can't drift.
@pkg-pr-new

pkg-pr-new Bot commented Sep 25, 2026

Copy link
Copy Markdown
@standard-server/aws-lambda

npm i https://pkg.pr.new/@standard-server/aws-lambda@97

@standard-server/core

npm i https://pkg.pr.new/@standard-server/core@97

@standard-server/fastify

npm i https://pkg.pr.new/@standard-server/fastify@97

@standard-server/fetch

npm i https://pkg.pr.new/@standard-server/fetch@97

@standard-server/node

npm i https://pkg.pr.new/@standard-server/node@97

@standard-server/peer

npm i https://pkg.pr.new/@standard-server/peer@97

@standard-server/shared

npm i https://pkg.pr.new/@standard-server/shared@97

commit: f41a294

@codecov

codecov Bot commented Sep 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@codspeed

codspeed Bot commented Sep 25, 2026

Copy link
Copy Markdown
Contributor

Merging this PR will not alter performance

✅ 26 untouched benchmarks
⏩ 108 skipped benchmarks1


Comparing claude/peer-event-validator-mismatch-802cf0 (f41a294) with main (e92aea4)2

Open in CodSpeed

Footnotes

  1. 108 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

  2. No successful run was found on main (469d94b) during the generation of this report, so e92aea4 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

@pullfrog pullfrog 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.

ℹ️ No critical issues — one minor documentation gap.

Reviewed changes

  • Shared predicates in core — isEventStreamMessageId, isEventStreamMessageRetry (integer >= 0), and isEventStreamMessageComment are exported, and the matching assertEventStreamMessage* functions now delegate to them instead of duplicating the checks.
  • Peer validator synced with withEventMeta — isPeerEventStreamMessage now rejects id/comments containing CR/LF and non-integer or negative retry (previously Number.isFinite), so malformed meta is dropped at the transport boundary instead of throwing EventStreamEncoderError inside the receiving for await.
  • Tests — predicate cases in core plus peer rejection cases for negative/fractional retry and line-broken id/comment; the peer cases fail against the old validator.

I traced the consumer (packages/peer/src/event-stream.ts:20,38) and the SSE decoder path (packages/core/src/event-stream/decoder.ts:55) — the latter can only produce non-negative integer retry and line-break-free id/comments, so the tightened predicates don't create false rejections there. pnpm exec vitest run packages/core/src/event-stream packages/peer/src/validators.test.ts passes (117 tests).

ℹ️ Nitpicks

  • The "Errors and low-level assertions" export list at packages/core/README.md:446 still names only the four assertEventStreamMessage* helpers; the three new public predicates could be added there so the list stays current.

Pullfrog  | Fix it ➔ | View workflow run | Using DeepSeek Flash (default — pick a model for stronger reviews) | 𝕏

@dinwwwh
dinwwwh merged commit 7b35a15 into main Sep 25, 2026
10 of 11 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.

1 participant