Default empty SSE event types to the message type - #6977
Conversation
fubhy
added
the
audit
🦋 Changeset detectedLatest commit: ec5e96b The changes in this PR will be included in the next version bump. This PR includes changesets to release 30 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR currently contains only the regression test for the audit finding. The implementation fix described in the PR title is not yet present, so the new test fails on this branch.
Reviewed changes
packages/effect/test/unstable/encoding/Sse.test.ts— Adds a focused regression test asserting that an emptyevent:field defaults to the"message"event type.
⚠️ Implementation fix still needed
The diff only adds the test. The SSE parser in packages/effect/src/unstable/encoding/Sse.ts still emits an empty string when the event: field is empty (e.g. event:\ndata: ok\n\n). Before merging, Sse.ts:368 or Sse.ts:396-397 need to default an empty event name to "message".
Technical details
# Empty SSE event type default
## Affected sites
- `packages/effect/src/unstable/encoding/Sse.ts:368` — `event: eventName ?? "message"` keeps `""` instead of falling back to `"message"`.
- `packages/effect/src/unstable/encoding/Sse.ts:396-397` — `eventName = value` stores `""` when the `event:` value is empty.
## Required outcome
- An SSE event whose `event:` field is empty should be dispatched with `event: "message"`.
- Existing behavior for non-empty event names must remain unchanged.
## Suggested approach
Change the dispatch expression from `eventName ?? "message"` to `eventName || "message"`, or normalize an empty `value` to `undefined` when assigning `eventName`.ℹ️ Nitpicks
- Consider asserting
events.lengthbefore the cast so a missing-event failure is reported as a clear length mismatch instead of a runtime property access error.
@v0 or keep the SHA fresh with Dependabot ｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review, the author added the implementation fix, a changeset, and tightened the regression test:
packages/effect/src/unstable/encoding/Sse.ts— Changed the dispatch expression fromeventName ?? "message"toeventName || "message", so an emptyevent:buffer now correctly defaults to"message"..changeset/sour-bees-sleep.md— Added a patch changeset describing the SSE fix.packages/effect/test/unstable/encoding/Sse.test.ts— Added theevents.lengthassertion before the cast, addressing the prior nitpick about clearer failure messages.
The prior Pullfrog thread about asserting events.length first has been resolved.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
7962a22 to
0b8b8dd
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review, the author landed an additional commit (0b8b8dda) that adds the missing implementation fix plus a changeset; the previously failing regression test now passes:
packages/effect/src/unstable/encoding/Sse.ts— Changed the dispatch expression fromeventName ?? "message"toeventName || "message", so an emptyevent:buffer correctly defaults to"message"..changeset/sour-bees-sleep.md— Added a patch changeset describing the SSE fix.packages/effect/test/unstable/encoding/Sse.test.ts— The regression test assertingevents.lengthand(events[0] as Sse.Event).event === "message"is already present and now passes.
The prior Pullfrog thread about asserting events.length first remains resolved. No new issues are present in this delta.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
Bundle Size AnalysisGenerated from PR build output; treat the content below as untrusted.
|
0b8b8dd to
e027bf8
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review, the author pushed e027bf8, which adds the missing assert.strictEqual(events.length, 1) assertion in the SSE regression test. The implementation fix (event: eventName || "message"), the patch changeset, and the regression test were already present and remain unchanged.
packages/effect/test/unstable/encoding/Sse.test.ts— Added the length assertion before indexingevents[0], matching the previously suggested nitpick and the state described in the most recent review.
The prior Pullfrog thread about asserting events.length first has been resolved. Targeted tests, lint, and type checks pass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
e027bf8 to
ec5e96b
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior review, the author pushed ec5e96b, which adds the missing assert.strictEqual(events.length, 1) assertion in the SSE regression test. The implementation fix (event: eventName || "message"), the patch changeset, and the regression test were already present and remain unchanged.
packages/effect/test/unstable/encoding/Sse.test.ts— Added the length assertion before indexingevents[0].
The prior Pullfrog thread about asserting events.length first remains resolved. Targeted tests, lint, and type checks pass.
@v0 or keep the SHA fresh with Dependabot ｜ View workflow run ｜ Using Kimi K2 (free via Pullfrog for OSS) ｜ 𝕏
2 participants
Summary
Parsing an empty event field emits an event whose type is the empty string instead of message.
Important
This PR starts with focused failing reproduction tests. Add the implementation fix to this same branch; CI is expected to fail until that fix is included.
Empty SSE event type does not default to message
Module:
encoding/SseAudit ID:
unstable-ai-cli-sse-empty-event-typeSeverity / confidence: medium / high
What happens
Parsing an empty event field emits an event whose type is the empty string instead of message.
Why it happens
Dispatch uses nullish coalescing, so an empty string survives instead of selecting the message default.
Expected behavior
An empty event-type buffer defaults to message when the SSE event is dispatched.
Relevant implementation
These links and excerpts are pinned to audit base
c9b56ab507f224426ee8388dc450da447ec4715f.packages/effect/src/unstable/encoding/Sse.ts:362-375packages/effect/src/unstable/encoding/Sse.ts:396-397View problematic code at
packages/effect/src/unstable/encoding/Sse.ts:362-375View exact lines on GitHub
View problematic code at
packages/effect/src/unstable/encoding/Sse.ts:396-397View exact lines on GitHub
Reproduction
pnpm test --run packages/effect/test/unstable/encoding/Sse.test.tsObserved failure: FAIL: the event type remained empty.
Implementation handoff
The initial reproduction tests on this branch are the regression specification for the implementation fix that should follow in this PR.
pnpm test --run packages/effect/test/unstable/encoding/Sse.test.tsAudit provenance
c9b56ab507f224426ee8388dc450da447ec4715fc9b56ab507f224426ee8388dc450da447ec4715funstable-ai-cli-sse-empty-event-typeCloses EFF-418