Skip to content

fix(desktop): use activeSessionIdRef.current for restore/edit/reload/steer - #66695

Closed
Automata-intelligentsia wants to merge 1 commit into
NousResearch:mainfrom
Automata-intelligentsia:fix/stale-active-session-restore-edit-steer
Closed

Automata-intelligentsia wants to merge 1 commit into
NousResearch:mainfrom
Automata-intelligentsia:fix/stale-active-session-restore-edit-steer

Conversation

@Automata-intelligentsia

Copy link
Copy Markdown
Contributor

Problem

In usePromptActions, several actions that mutate session history were reading the closure-captured activeSessionId prop. Because the actions bag is a stable ref that does not re-render when the active session changes, those closures could hold a stale session ID and send prompt.submit / session.steer requests to the previously-active session.

This led to data loss: restoring/rewinding a message in the current chat would instead truncate the history of the previously-active session. The user would then see the restored prompt appear in the wrong session, and all messages after the restore point in that session were deleted.

Fix

Read the current session ID from activeSessionIdRef.current, matching the existing pattern used by cancelRun (which already has a comment explaining exactly this issue). Updated:

  • steerPrompt
  • reloadFromMessage
  • restoreToMessage
  • editMessage

This makes all of these actions target the actual currently-active session instead of a stale closure value.

Verification

The diff is minimal: it replaces activeSessionId || activeSessionIdRef.current with activeSessionIdRef.current in the affected callbacks, and updates the useCallback dependency arrays accordingly.

…steer

The restore, edit, reload, and steer actions were reading the closure-captured

activeSessionId, which can be stale when the actions bag is a stable ref. This

caused rewind/truncate operations to be sent to the wrong session, deleting

messages in the previously-active session instead of the current one.

Read from activeSessionIdRef.current, matching the existing cancelRun pattern.
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 18, 2026
@alt-glitch

Copy link
Copy Markdown
Contributor

This was generated by AI during triage.

Related to merged #66485: both eliminate stale session-ID reads in Desktop actions, but this PR covers restore/edit/reload/steer rather than the Stop action.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

+12/-11 desktop fix to use activeSessionIdRef.current for restore/edit/reload/steer. Fixes session state tracking in the desktop app. No concerns.

Reviewed by Hermes Agent

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

+12/-11 desktop fix to use activeSessionIdRef.current for restore/edit/reload/steer. Fixes session state tracking in the desktop app. No concerns.

Reviewed by Hermes Agent

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for tracing this to the stable actions bag. The premise is verified on current main: cancelRun documents the stale-closure hazard and already uses activeSessionIdRef.current at apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts:520-526, while the four callbacks in this PR still read activeSessionId first (:609, :638-654, :686, :732).

Problems

  • Regression coverage is missing. The PR changes only index.ts; the existing harness exposes restore and steer but not reload or edit (apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx:60-65), and current tests cover only the normal session-ID path (:712-729, :786-816).

Suggested changes

  • Add a stale-ref test per changed callback: capture the action with session A, move activeSessionIdRef.current to session B without replacing that action, then assert gateway payloads and optimistic session-state writes use B. Extend the harness to expose reload and edit for the same coverage.

Automated hermes-sweeper review.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for this — your diagnosis was correct and it's now on main via #71462 (merged as 771f1b7), with your authorship preserved as Co-authored-by:.

Your read of the root cause was exactly right: the actions bag is a stable ref mutated in place, the pane surfaces are memoized on it, so the closure-captured activeSessionId never updates and activeSessionId || activeSessionIdRef.current prefers the stale value whenever it's non-null. cancelRun already documented that hazard right above the broken sites.

Your branch had gone CONFLICTING against current main, so rather than ask you to rebase we carried the work forward and extended it:

  • same four sites (redirectPrompt/steerPrompt, reloadFromMessage, restoreToMessage, editMessage)
  • plus changeSessionCwd in use-cwd-actions.ts, which had the identical defect — a stale target re-anchored another conversation's workspace and pointed that agent's terminal/file tools at the wrong project
  • dropped the now-unused activeSessionId option from useCwdActions instead of leaving it as a footgun
  • 4 regression tests, each verified to fail against the pre-fix code and under a scripted revert of all four sites

One thing worth flagging for context: #59305 landed a day before the merge and fixed the mechanism (the ref now mirrors synchronously during render instead of a lagging useEffect) but left the call sites alone — so the ref became trustworthy while these actions kept ignoring it. Your fix was the necessary other half.

Closing as landed. Appreciated.

@teknium1 teknium1 closed this Jul 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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.

4 participants