fix(a2a-server): delimit SSE events with a blank line in /executeCommand#27549
fix(a2a-server): delimit SSE events with a blank line in /executeCommand#27549Pluviobyte wants to merge 1 commit into
Conversation
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a protocol non-compliance issue in the A2A server's streaming /executeCommand endpoint. By correctly delimiting SSE events with a blank line, it ensures that standard EventSource clients can properly parse individual events. Additionally, the existing streaming test was refactored to be truly asynchronous, providing a reliable regression test for this fix. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request updates the SSE streaming response in packages/a2a-server/src/http/app.ts to terminate events with a double newline (\n\n) to comply with the Server-Sent Events specification. It also refactors the /executeCommand streaming E2E test in app.test.ts to use async/await and parse the full response text directly, simplifying the test logic. There are no review comments, so I have no feedback to provide.
The streaming `/executeCommand` handler wrote each Server-Sent Event with a single trailing newline (`data: <json>\n`). The SSE specification requires events to be separated by a blank line (`\n\n`); without it a spec-compliant EventSource client coalesces every event into one record and never dispatches a well-formed event, so streaming command output is unusable for real clients. The existing streaming test did not catch this because it used the Mocha-style `done` callback, which Vitest 3 does not honour: the test body returned before its assertions ran, so the test always passed regardless of the payload. Convert that test to async/await so it actually verifies the emitted events. It fails before this fix (1 event parsed instead of 2) and passes after.
e1ade13 to
e98c346
Compare
Summary
The streaming
/executeCommandendpoint in the A2A server emits Server-Sent Events that are not separated by a blank line, so a spec-compliant SSE/EventSourceclient cannot parse the individual events. This is a one-line framing fix plus a test that actually exercises the path.Details
In
packages/a2a-server/src/http/app.tseach event was written as:Per the SSE spec, an event is only dispatched when a blank line (
\n\n) is encountered. With a single\n, consecutivedata:lines are concatenated into one record and the final event is never terminated, so a realEventSourceconsumer ends up with one malformed event instead of N. The rest of the codebase already uses\n\nfor SSE (seepackages/core/src/code_assist/server.test.ts). The fix appends the missing newline.While fixing this I found the existing streaming test never actually asserted anything: it used the Mocha-style
donecallback, which Vitest 3 does not honor, so the test body returned before theres.on('end')assertions ran and it passed regardless of output. I converted it toasync/await(matching the other SSE tests in the file) so it genuinely validates the stream — it fails before this fix (expected 1 to be 2) and passes after.Related Issues
Fixes #27587
How to Validate
To see the bug without the production fix, revert only the
app.tschange and re-run the streaming test — it fails withexpected 1 to be 2, confirming the new test guards the regression.Pre-Merge Checklist
vitest+tsc --noEmitfor@google/gemini-cli-a2a-server)