fix(chat): ignore malformed stream payloads - #29
Conversation
📝 WalkthroughWalkthrough
ChangesStream event validation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 97f607d030
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| knownReasoningPartIDs?: Set<string>, | ||
| pendingRenderableTextPart?: { partID?: string; messageID?: string }, | ||
| ): void { | ||
| if (!payload || typeof payload !== "object") return; |
There was a problem hiding this comment.
When the host sends { type: "streamEvent", event: null, processing: true }, the stream case sets payload to the outer data object via asRecord(data.event) ?? data, so this guard never returns; moreover, the shared pre-switch logic has already dispatched SET_PROCESSING, which can leave a phantom loading state. Reject a missing/non-record data.event at the message boundary before processing bootstrap, and exercise that path through the handler rather than matching source text.
AGENTS.md reference: AGENTS.md:L9-L14
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/regression/malformed-stream-event-regression.test.mjs`:
- Around line 11-17: Extend the regression test around handleStreamEvent to
invoke the handler through the existing test seam with null, undefined, and
primitive payloads, then assert state processing is not triggered. Retain the
current source-regex assertion as a supplementary implementation check, but make
the behavioral assertions the primary coverage.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8fef596-3c36-4a79-9466-91dbec3a8743
📒 Files selected for processing (2)
tests/regression/malformed-stream-event-regression.test.mjswebview/shared/src/chat/lib/messageHandler.ts
| test("malformed stream events are ignored before state processing", () => { | ||
| assert.match( | ||
| source, | ||
| /function handleStreamEvent\([\s\S]*?\): void \{\s*if \(!payload \|\| typeof payload !== "object"\) return;/s, | ||
| "the stream handler must ignore null or missing event payloads", | ||
| ); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise handleStreamEvent in the regression test.
This test only matches the guard text in messageHandler.ts. It does not call handleStreamEvent with null, undefined, or a primitive, and it does not verify that state processing is skipped. Add a behavior-level test through the handler test seam. Keep this source assertion only as a supplementary implementation check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/regression/malformed-stream-event-regression.test.mjs` around lines 11
- 17, Extend the regression test around handleStreamEvent to invoke the handler
through the existing test seam with null, undefined, and primitive payloads,
then assert state processing is not triggered. Retain the current source-regex
assertion as a supplementary implementation check, but make the behavioral
assertions the primary coverage.
|
Follow-up review addressed: malformed stream envelopes are rejected at the webview boundary before state processing, including arrays. The regression now invokes the handler and verifies state remains unchanged; the source-regex-only test was removed. Validation: focused handler regression, guard:prepush, guard:streaming, typecheck, webview build, and extension build. |
1 participant
Summary
Verification
Subtest: malformed stream events are ignored before state processing
ok 1 - malformed stream events are ignored before state processing
duration_ms: 1.227042
...
1..1
tests 1
suites 0
pass 1
fail 0
cancelled 0
skipped 0
todo 0
duration_ms 63.790167
Closes #23
Summary by CodeRabbit
Bug Fixes
Tests