Conversation
commit: |
Validates wire format conformance for all 3 WebSocket protocols: - WS Responses API (event structure, IDs, types, deltas) - WS Realtime API (session lifecycle, event_ids, response sequences) - WS Gemini Live (setupComplete, serverContent, toolCall shapes) - Cross-protocol invariants (upgrade acceptance, malformed JSON)
jpr5
force-pushed
the
test/ws-conformance
branch
from
1b59319 to
ecdd4fd
Compare
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only addition, no production code changes. The two nits about misleading test names are worth fixing but don't block merge.
Extended reasoning...
Overview
This PR adds a single new file src/__tests__/ws-api-conformance.test.ts containing 53 tests that validate wire format conformance for three WebSocket protocols: WS Responses API, WS Realtime API, and WS Gemini Live BidiGenerateContent. No production code is modified — this is purely additive test coverage.
Security risks
None. The PR only adds test code that runs against a local test server instance. No authentication, permissions, or external service interactions are involved.
Level of scrutiny
Low scrutiny warranted. Test-only PRs that add new files cannot break production behavior. The tests follow established patterns (using existing connectWebSocket helper, createServer, and Fixture types), and the assertions are straightforward conformance checks against known API shapes.
Other factors
The two bugs found by the bug hunting system are both cosmetic nits: (1) a test named after /v1/responses that actually tests /nonexistent-path, and (2) a test name saying call- (hyphen) when the actual prefix is call_ (underscore). In both cases the test logic and assertions are correct — only the human-readable descriptions are misleading. These are worth fixing for documentation clarity but do not affect test correctness or justify blocking the PR.
| const ws = await connectWebSocket(instance.url, GEMINI_WS_PATH); | ||
| ws.send(geminiSetup()); | ||
| await ws.waitForMessages(1); | ||
| ws.send(geminiClientContent("error-test")); | ||
| const raw = await ws.waitForMessages(2); | ||
| ws.close(); | ||
| const msg = JSON.parse(raw[1]) as any; |
There was a problem hiding this comment.
🟡 Test name "non-WS HTTP path /v1/responses (HTTP) rejects WebSocket upgrade" is misleading — the test body actually hits /nonexistent-path, not /v1/responses. Since /v1/responses is a valid WS endpoint that accepts upgrades, the test would fail if it matched its name. Rename to something like "non-registered path rejects WebSocket upgrade".
Extended reasoning...
What the bug is
The test at line 788 is named "non-WS HTTP path /v1/responses (HTTP) rejects WebSocket upgrade" but the test body calls connectWebSocket(instance.url, "/nonexistent-path"). The test name and test body describe two completely different things.
Why this is wrong
/v1/responses is one of the three valid WebSocket endpoints — the test at line 777 ("all 3 WS paths accept WebSocket upgrade") proves that /v1/responses successfully accepts WebSocket upgrades. If this test actually used /v1/responses as its name implies, connectWebSocket would resolve successfully instead of rejecting, and the rejects.toThrow("Upgrade failed") assertion would fail.
Step-by-step proof
- Line 779:
connectWebSocket(instance.url, "/v1/responses")succeeds (upgrade accepted). - Line 788: Test is named
"non-WS HTTP path /v1/responses (HTTP) rejects WebSocket upgrade". - Line 792: Test body calls
connectWebSocket(instance.url, "/nonexistent-path")— not/v1/responses. - The comment on line 789-790 even acknowledges the mismatch: "The WS endpoint is /v1/responses but only accepts the upgrade — this verifies the server would reject other non-registered paths."
- So the test correctly verifies that unregistered paths reject WS upgrades, but the test name claims it is verifying
/v1/responsesbehavior.
Impact
Someone reading test output or doing test-driven development would incorrectly believe that /v1/responses HTTP-only behavior is being verified. The test itself is functionally correct — it properly tests that non-registered paths reject upgrades — but the misleading name reduces the value of the test suite as documentation.
Suggested fix
Rename the test to accurately describe what it tests, e.g.: "non-registered path /nonexistent-path rejects WebSocket upgrade" or "unknown path rejects WebSocket upgrade". The comment can stay as-is since it explains the intent.
| }); | ||
|
|
||
| it("response.content_part.added has part with type text", async () => { | ||
| const frames = await getTextResponseFrames(); |
There was a problem hiding this comment.
🟡 Nit: Test name says "call- prefix" (hyphen) but the actual ID prefix is call_ (underscore). The assertion correctly checks /^call_/, so the test logic is fine — just the description is misleading. Should read "call_ prefix" to match generateToolCallId() behavior.
Extended reasoning...
What the bug is
The test at line 485 is named:
"response.output_item.added has type function_call, call_id (call- prefix), name, empty arguments"
It says "call- prefix" with a hyphen, but the actual prefix used by the system is call_ with an underscore.
Why underscore, not hyphen
In src/helpers.ts, there are two ID-generation functions:
generateId(prefix)(line 12): returns`${prefix}-${randomBytes(12).toString("base64url")}`— uses a hyphengenerateToolCallId()(line 16): returns`call_${randomBytes(12).toString("base64url")}`— uses an underscore
All other ID prefixes in the test names (resp-, evt-, sess-) correctly use hyphens because they go through generateId(). The call_ prefix is the exception because it uses the separate generateToolCallId() function, which hardcodes an underscore to match the OpenAI API format.
Step-by-step proof
- Line 485: test name says "call- prefix" (hyphen)
- Line 496: assertion checks
expect(item.call_id).toMatch(/^call_/)(underscore) — this is correct helpers.ts:17:generateToolCallId()returnscall_xxx(underscore)- Therefore the test name is inconsistent with both the assertion and the actual behavior
Impact
This is purely cosmetic — the test logic and assertion are correct. However, the misleading test name could confuse developers reading the test output, especially since all other prefix descriptions in the file accurately reflect their separator character (hyphen for resp-, evt-, sess-).
Fix
Change the test name on line 485 from (call- prefix) to (call_ prefix).
## Summary - Adds `ws-api-conformance.test.ts` with 53 tests validating wire format conformance for all 3 WebSocket protocols - WS Responses API: event structure, ID prefixes (`resp-`, `call_`), types, delta/done shapes - WS Realtime API: session lifecycle, `evt-`/`sess-` ID prefixes, response event sequences, tool call structure - WS Gemini Live: `setupComplete` shape, `serverContent` frames, `toolCall.functionCalls` with parsed object args, error codes/statuses - Cross-protocol invariants: upgrade acceptance on all 3 WS paths, rejection on non-WS paths, malformed JSON handling ## Test plan - [x] All 53 new tests pass - [x] Red-green verified on 3 structural assertions (resp- prefix, evt- prefix, Gemini args-as-object) - [x] Format and lint clean - [x] Full suite still passes (468 + 53 = 521 tests) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
1 participant
Summary
ws-api-conformance.test.tswith 53 tests validating wire format conformance for all 3 WebSocket protocolsresp-,call_), types, delta/done shapesevt-/sess-ID prefixes, response event sequences, tool call structuresetupCompleteshape,serverContentframes,toolCall.functionCallswith parsed object args, error codes/statusesTest plan
🤖 Generated with Claude Code