fix(tui): drop cross-session events during null-sid switch window (#51058) - #53936
fix(tui): drop cross-session events during null-sid switch window (#51058)#53936yingliang-zhang wants to merge 2 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved
Clean one-line fix for cross-session event bleed during null-sid switch (#51058). The change from ev.session_id !== sid to !sid || ev.session_id !== sid ensures ALL non-gateway session events are dropped when sid is null (during session switch/reset). This prevents another live session's events from bleeding into the view.
Reviewed by Hermes Agent
1e99e6d to
d039af3
Compare
d039af3 to
5090ff3
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the TUI routing window; current main still has the null-sid short-circuit at ui-tui/src/app/createGatewayEventHandler.ts:407, and resetSession() does set sid: null at ui-tui/src/app/useSessionLifecycle.ts:159.
Problems
- The proposed predicate starts with
evSid &&(ui-tui/src/app/createGatewayEventHandler.ts:418in this diff), sosession_id: ''bypasses it. That does not implement the PR's stated empty-string filtering. - The new negative tests do not exercise transcript routing.
message.deltaonly records buffered state (ui-tui/src/app/createGatewayEventHandler.ts:939-942); assistant messages are appended onmessage.complete(:943-949). The new cases send only a delta before asserting no appended assistant message. Thelength >= 0checks are tautological.
Suggested changes
- Preserve the null-sid fix, and align the empty-ID behavior with a verified event contract.
- Make the regression tests send a sequence that would append without filtering, including
message.complete, then assert the transcript remains unchanged for rejected sessions.
Automated hermes-sweeper review.
|
Addressed the automated review in |
a496575 to
ee6b42b
Compare
…ross-session bleed (NousResearch#51058) Rebased on current upstream/main. Combines two fixes: 1. null-sid guard: when sid is null during session switch/reset, drop ALL non-gateway events instead of letting them through (original NousResearch#51058 fix) 2. empty-string session_id: _emit() can set session_id='' when callers omit it. Use explicit equality (evSid !== sid) instead of truthiness, and broaden global-event bypass to gateway./pet./skin./billing. Closes NousResearch#51058 (cherry picked from commit 638fc85ba945da7eb0a10b066a4237266d55515d)
(cherry picked from commit ee6b42b7c42fed7e376a79e4426c6aafc5577d35)
ee6b42b to
3cd3597
Compare
Problem
During session switch/reset,
getUiState().sidis momentarilynull. The old event filterev.session_id && sid && ev.session_id !== sidshort-circuits tofalsewhensidis null, so events from any live session bleed into the view — the cross-session bleed reported in #51058.Fix
Change the condition so that when
sidis null, all non-gateway session events are dropped:(!sid || ev.session_id !== sid)means: drop the event if there is no active session or the event belongs to a different session. Gateway-meta events (gateway.*) are still always allowed through.Scope
1 file, 1 condition change. Supersedes #53928 which was opened from a
mainbranch and accidentally bundled 3 unrelated fixes.Closes #51058