fix(server): add sessionId to user_message echo#347
Conversation
…ion bleed user_message events were echoed to clients without sessionId, bypassing the v2 session filter in the frontend store. The filter treated them as global events, causing user input from other sessions to appear in the active session view. Adding sessionId + v:2 to the echo payload at all three emission sites (resume, send, interrupt) aligns user_message with every other session-scoped v2 event. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
4c4f0af to
681c987
Compare
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Centaur ReviewFound 4 issue(s) (2 warning).
|
dimakis
left a comment
There was a problem hiding this comment.
Centaur Review
Found 3 issue(s) (1 warning).
server/__tests__/send-to-chat.test.ts
Correct fix — sessionId is added consistently across all three echo sites with proper conditional handling, and the type change is aligned with existing v2 conventions. Main gap is that no test asserts the new sessionId field is actually present in the echo.
- 🟡 missing_tests: Existing tests for sendToChat and interruptChat assert
toMatchObject({ type: 'user_message', text: '...' })but never verify the newv: 2orsessionIdfields. Since the entire point of this PR is preventing cross-session bleed by adding sessionId, at least one test should assert thatsessionIdappears in the echoed message whensession.sessionIdis set (which it is in all test cases — e.g. line 33:session.sessionId = 'sess-123'). A simpleexpect(userMsgEvents[0]).toMatchObject({ v: 2, sessionId: 'sess-123' })would lock in the fix.[fixable] - 🔵 missing_tests: No test covers the case where
session.sessionIdis falsy (before SDK resolves it). The conditional spread...(session.sessionId ? { sessionId: session.sessionId } : {})should be exercised with a test that omits settingsession.sessionIdand asserts the echo has nosessionIdproperty. This would guard against regressions where someone changes the conditional to always include it (potentially sendingundefined).[fixable]
server/chat.ts
Correct fix — sessionId is added consistently across all three echo sites with proper conditional handling, and the type change is aligned with existing v2 conventions. Main gap is that no test asserts the new sessionId field is actually present in the echo.
- 🔵 style (L1095): The conditional spread
...(session.sessionId ? { sessionId: session.sessionId } : {})appears identically in both sendToChat and interruptChat. This is fine for two occurrences, but if you wanted consistency with the resume path (line 962:sessionId: options.resume), you could use the simpler...(session.sessionId && { sessionId: session.sessionId }). Minor style nit — current form is perfectly correct.[fixable]
| v: 2, | ||
| messageId, | ||
| text: fullPrompt, | ||
| ...(session.sessionId ? { sessionId: session.sessionId } : {}), |
There was a problem hiding this comment.
🔵 style: The conditional spread ...(session.sessionId ? { sessionId: session.sessionId } : {}) appears identically in both sendToChat and interruptChat. This is fine for two occurrences, but if you wanted consistency with the resume path (line 962: sessionId: options.resume), you could use the simpler ...(session.sessionId && { sessionId: session.sessionId }). Minor style nit — current form is perfectly correct. [fixable]
Summary
user_messageWebSocket events were echoed to clients withoutsessionId, bypassing the frontend's v2 session filter (store.tswsListener)task_state), causing user input from other sessions to bleed into the active session viewsessionId+v: 2to the echo payload at all three emission sites inchat.ts(resume, send, interrupt)UserMessageMsgtype inws-messages.tsto include optionalsessionIdRoot cause
Every other session-scoped v2 event (
message_end,session_end, etc.) goes throughsendOrBuffer()which auto-enriches withsessionId. Theuser_messageecho bypassed that path entirely, using directsend()+broadcastToObservers().Test plan
🤖 Generated with Claude Code