Skip to content

fix(desktop): guard session.info state patches with explicitSid - #60973

Closed
yingliang-zhang wants to merge 1 commit into
NousResearch:mainfrom
yingliang-zhang:fix/desktop-race3-session-info-state-patch
Closed

fix(desktop): guard session.info state patches with explicitSid#60973
yingliang-zhang wants to merge 1 commit into
NousResearch:mainfrom
yingliang-zhang:fix/desktop-race3-session-info-state-patch

Conversation

@yingliang-zhang

Copy link
Copy Markdown
Contributor

Summary

The desktop event handler in gateway-event.ts resolves sessionId = explicitSid || activeSessionIdRef.current. When explicitSid is empty (unscoped event), events fall back to the active session — which is correct for display (deltas, completions are the focused turn's own output, see #42178). But the session.info state-patch path (updateSessionState) used the same fallback sessionId to patch per-session model/cwd/branch.

During a session switch, an unscoped session.info could patch the active session's state attributes with a background session's values — the Race 3 gap left open by #53936.

Fix: require explicitSid for the updateSessionState call in the session.info handler. Display operations (delta append, completion) still use the fallback — only per-session state mutations are gated.

Before (leaks state patches via fallback sessionId)

if (sessionId && hasStatePatch) {
  updateSessionState(sessionId, state => ({ ...state, ...statePatch, ... }))
}

After (only patches when we have a real session_id)

if (explicitSid && sessionId && hasStatePatch) {
  updateSessionState(sessionId, state => ({ ...state, ...statePatch, ... }))
}

Why this is safe

  • Background sessions with explicit session_id still get their state patches (the common case).
  • Unscoped session.info events from the active turn still apply global setters (setCurrentModel, setCurrentCwd, etc.) via the existing apply guard — only the per-session updateSessionState call is gated.
  • Display operations (message.delta, message.complete, reasoning.delta, tool.start, etc.) are unaffected — they use sessionId for rendering but don't call updateSessionState with state patches.

Relationship to existing work

PR Scope Status
#53936 TUI-layer null-sid + empty-string session_id filter (Races 2, 2b) Open
This PR Desktop Electron layer session.info state-patch guard (Race 3)

#53936 explicitly notes the desktop Electron layer fallback as remaining. This PR closes that gap.

Tests

  • NODE_ENV=test npm --workspace apps/desktop run typecheck — passes
  • vitest run src/lib/gateway-events.test.ts — 3/3 pass
  • NODE_ENV=production npm --workspace apps/desktop run build — passes

Checklist

  • No personal info / internal paths in diff
  • Git author is zhangyingliang@outlook.com
  • No secrets / .env in diff

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state P3 Low — cosmetic, nice to have labels Jul 8, 2026

@teknium1 teknium1 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 isolating the state-patch path rather than changing display-event routing.

Problems

  • The changed guard has no direct regression coverage. apps/desktop/src/lib/gateway-events.test.ts:21-25 only checks the classifier; it does not invoke useGatewayEventHandler or verify updateSessionState for an unscoped, state-patched session.info.
  • The claimed unscoped background-event route was not established from current main. _emit always serializes session_id in tui_gateway/server.py:1143-1147, while the desktop routing contract says missing IDs represent the focused turn in apps/desktop/src/lib/gateway-events.ts:21-23.

Suggested changes

  • Add a hook-level regression test for an unscoped state patch with an active session (no per-session mutation), plus an explicit-ID event that updates the addressed session.
  • Identify the valid gateway emission path that produces the Race 3 event so the new session.info exception is covered by a concrete contract.

Automated hermes-sweeper review.

// Only apply per-session state patches when we have a real session_id.
// Without explicitSid, sessionId falls back to activeSessionIdRef.current
// — patching state with that fallback could misattribute a background
// session's model/cwd/branch onto the active session during a switch.

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.

Please add a hook-level regression test for this guard. The current gateway-events.test.ts only permits unscoped session.info; it does not verify that an unscoped state patch cannot call updateSessionState for the active session, while an explicit session ID still can.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 10, 2026
…o state patches

Race 5 (composer queue cross-session migration): When a prompt is queued
in session A and the user switches to session B before the queue drains,
the drain used the currently-active session id (B) instead of the source
session (A). Now each QueuedPromptEntry stamps sourceRuntimeId and
sourceStoredId at enqueue time, and runDrain threads them through
onSubmit to submitPromptText so the prompt always lands in the original
session. removeQueuedPrompt also uses the entry's source key.

Fixes NousResearch#56390 (complementary to PR NousResearch#56444 which takes a similar approach).

Race 3 (desktop event layer fallback): session.info events with empty
session_id fell back to activeSessionIdRef.current for
updateSessionState calls, potentially patching the active session's
model/cwd/branch with a background session's state during a switch.
Now the per-session state-patch path requires explicitSid - unscoped
events still render (deltas, completions) but can't mutate per-session
state attributes.
@yingliang-zhang
yingliang-zhang force-pushed the fix/desktop-race3-session-info-state-patch branch from e9dba00 to 616a3a4 Compare July 11, 2026 05:21
@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

Closing after re-auditing the current gateway/Desktop contract against the maintainer review.

On current main, _emit() always serializes session_id; WebSocket/shared routing forwards params unchanged; all identified background session.info producers supply explicit IDs. Missing IDs are explicitly documented as focused-turn output, not background state. The queue source-session changes from the old branch are now patch-equivalent upstream. Therefore no valid unscoped background state-patch producer remains, and adding a synthetic hook test would preserve speculative behavior rather than a production contract.

Verification on current main: Desktop gateway-events Vitest 6 passed, Desktop typecheck PASS, and the replacement worktree has no delta. Thanks for the review—it correctly exposed that the original Race 3 premise was not established.

@yingliang-zhang

Copy link
Copy Markdown
Contributor Author

Correction to my previous comment: the old combined commit is not wholly patch-equivalent to current main (git cherry origin/main still reports it unique). The close decision remains correct for this PR's stated Race 3 scope because the unscoped background session.info producer path does not exist. The separate queued-prompt source-boundary behavior from that historical combined branch is being handled in the broader #63298/local integration line, not treated as already merged here. I should not have described that queue portion as upstream-equivalent.

@teknium1 teknium1 added the area/sessions Session lifecycle, resume, persistence, history label Jul 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants