Skip to content

test: cover fragmented SSE framing and invalid UTF-8 - #24

Merged
erikleblansch merged 3 commits into
VOLPAROSSA:mainfrom
Aaqibhafeezkhan:issue-23-sse-framing-tests
Oct 27, 2026
Merged

erikleblansch merged 3 commits into
VOLPAROSSA:mainfrom
Aaqibhafeezkhan:issue-23-sse-framing-tests

Conversation

@Aaqibhafeezkhan

@Aaqibhafeezkhan Aaqibhafeezkhan commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Cover CRLF event delimiters split across separate network writes, including the blank-line terminator.
  • Verify multiple valid SSE events in one write are emitted once and in order, while SSE comment lines are ignored.
  • Verify malformed UTF-8 in an SSE data line closes the client without emitting an event.

Testing

  • Added focused cases to tests/opencode-client.test.cjs.

Closes #23

Copy link
Copy Markdown
Contributor

Thanks for picking this up and adding these focused cases! The source-contracts workflow now passes on your current commit.

I found two test reliability points to address before merging:

  1. In the two-event case, the listener is removed as soon as the second event arrives. A third or later duplicate would therefore be invisible, even though the test is intended to check that events are emitted once. Could the test keep observing through a bounded completion point and then assert the complete event list?
  2. If the 500 ms deadline expires, check continues scheduling itself with setImmediate, so the test process can remain alive instead of failing cleanly. Please stop the polling on both success and timeout (or use a bounded event-based wait with cleanup).

For the split-CRLF case, could you also check whether the fixture makes the client receive the intended boundaries separately? Separate write calls do not by themselves establish what the client observed. A deterministic fixture or an assertion on observed chunks would make that coverage clearer.

Thank you again — the changes are limited to the intended test file, and I look forward to reviewing the update.

@Aaqibhafeezkhan Aaqibhafeezkhan left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kept the SSE listener active through a bounded completion window, ensured timeout and success paths clean up correctly, and verified the CRLF test against observed response chunks.

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

Thanks for the update. Reviewed ac3d477, including an independent technical review. The earlier points are addressed: the two-event listener remains active through a bounded quiet window and checks the full observed sequence; both success and timeout clear the timers and listener; and the CRLF fixture waits for each segment to be observed by the client before sending the next. The malformed UTF-8 case verifies closure without event emission.

The existing source-contracts run is now successful: https://github.com/VOLPAROSSA/volparossa-code/actions/runs/40426420911 (job 121944425077, Node v22.23.3, test merge e37cec0 against main 06271e4). All three new cases passed; the Node suite reports 363 passed, 0 failed, 1 skipped. Python suites report 7/169/10 tests, with 1/3/0 skipped, respectively, and no failures.

No blocking findings. A nonblocking follow-up would add per-test deadlines to the CRLF/invalid-UTF8 waits; currently a missing event or closure is bounded by the workflow's five-minute timeout. This approval covers the synthetic HTTP/SSE test changes only.

@erikleblansch
erikleblansch merged commit ce25245 into VOLPAROSSA:main Oct 27, 2026
1 check 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.

test: cover fragmented SSE framing and invalid UTF-8 in OpenCode client

2 participants