Skip to content

fix(desktop): route /branch from tile to the tile's session, not the foreground chat - #73236

Open
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/desktop-branch-tile-session
Open

fix(desktop): route /branch from tile to the tile's session, not the foreground chat#73236
JonthanaHanh wants to merge 1 commit into
NousResearch:mainfrom
JonthanaHanh:fix/desktop-branch-tile-session

Conversation

@JonthanaHanh

Copy link
Copy Markdown
Contributor

Summary

Fixes #73207. When /branch is dispatched from a session tile (⌘T tab or split pane), the slash dispatcher forwards the tile's runtime ID as sessionHint. The branch action handler previously ignored sessionHint and called branchCurrentSession(), which reads foreground-only refs (activeSessionIdRef, $messages, busyRef, selectedStoredSessionIdRef) and always branches the primary chat — regardless of which tile the command was entered from.

Root Cause

The branch handler at slash.ts:429 was the only action handler that didn't destructure or forward sessionHint from the SlashActionCtx. All other session-targeting handlers (yolo, handoff, skin) already extract and use it:

// Before (broken)
branch: async () => {
  await branchCurrentSession()
},

// yolo — already correct
yolo: async ({ sessionHint }) => {
  const sid = sessionHint || activeSessionIdRef.current
  // ...

Fix

When sessionHint targets a non-foreground session, resolve the tile's stored session ID from $sessionStates and use branchStoredSession() instead. branchStoredSession reads the stored transcript directly from the backend — no active-session or live message-store dependency — so it correctly branches the tile's session.

The foreground path (no sessionHint, or sessionHint === activeSessionIdRef.current) is unchanged.

Changes

File Change
apps/desktop/src/app/session/hooks/use-prompt-actions/slash.ts Add branchStoredSession to SlashCommandDeps; update branch handler to check sessionHint
apps/desktop/src/app/session/hooks/use-prompt-actions/index.ts Add branchStoredSession to PromptActionsOptions; forward to useSlashCommand
apps/desktop/src/app/contrib/wiring.tsx Pass branchStoredSession from useSessionActions to usePromptActions
apps/desktop/src/app/session/hooks/use-prompt-actions/index.test.tsx Add mock branchStoredSession to test harness

Test Plan

  • Desktop E2E tests pass (CI)
  • Branching from the foreground chat still works (no regression)
  • Branching from a tile creates the branch under the tile's session
  • Branching from a tile with no stored session ID falls back to foreground behavior

…foreground chat

When /branch is dispatched from a session tile (⌘T tab or split pane),
the slash dispatcher forwards the tile's runtime ID as sessionHint.
The branch action handler previously ignored sessionHint and called
branchCurrentSession(), which reads foreground-only refs
(activeSessionIdRef, $messages, busyRef, selectedStoredSessionIdRef)
and always branches the primary chat.

Fix: when sessionHint targets a non-foreground session, resolve the
tile's stored session ID from $sessionStates and use
branchStoredSession() instead.  branchStoredSession reads the stored
transcript directly from the backend — no active-session or live
message-store dependency — so it correctly branches the tile's session.

The foreground path (no sessionHint, or sessionHint === activeSessionId)
is unchanged.

Fixes NousResearch#73207

@andyst-dev andyst-dev 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 picking this up. I found two session-safety gaps in the current approach:

  1. When sessionHint is explicit but $sessionStates has no matching storedSessionId, the handler falls through to branchCurrentSession() and branches the foreground chat. That preserves the exact cross-session misroute this issue is trying to eliminate; an explicit target should fail closed instead.
  2. branchStoredSession() reads the persisted transcript and does not consult the tile runtime's live busy state/messages. During an in-flight tile turn it can therefore branch a stale transcript instead of showing the existing busy guard used by foreground branching. The issue explicitly calls out the target transcript and busy state as part of the broken contract.

There is also no behavioral regression test here—the harness only gains a stub. A safer shape would be to let branchCurrentSession accept the invoking runtime ID, select busy, messages, storedSessionId, and cwd from that runtime's cached state, and refuse to fall back when an explicit runtime is unavailable. Please add dispatcher and session-action tests covering the correct runtime/message count, both directions of busy-state isolation, and the missing-target fail-closed case.

@alt-glitch alt-glitch added type/bug Something isn't working comp/desktop Electron desktop app (apps/desktop/*) area/sessions Session lifecycle, resume, persistence, history P2 Medium — degraded but workaround exists sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 28, 2026
@frizikk

frizikk commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

A complete alternative implementation is available in #73251. It follows the runtime-targeted shape suggested in the review here: sessionHint is forwarded to branchCurrentSession, which reads the invoking runtime’s cached transcript, busy state, stored parent, profile, and workspace and fails closed when an explicit target is unavailable. It also pins session.branch to the invocation gateway across asynchronous profile lookup and prevents an in-flight completion from inserting/revealing the child in a newly active profile. The dispatcher, session-action, gateway-switch, and stale-completion paths have behavioral regressions; 151 focused tests pass.

@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 missing sessionHint handling; current main confirms the native action still calls the foreground-only helper at apps/desktop/src/app/session/hooks/use-prompt-actions/slash.ts:478-480.

Problems

  • The new fallback at apps/desktop/src/app/session/hooks/use-prompt-actions/slash.ts:439-450 calls branchCurrentSession() with no target when an explicit tile runtime has no $sessionStates entry. That silently branches the foreground chat—the behavior this PR is intended to prevent. An explicit target must fail closed.
  • The new route passes only a stored id to branchStoredSession() (slash.ts:440-444). That helper reads persisted history through getSessionMessages and has no live-runtime busy check (apps/desktop/src/app/session/hooks/use-session-actions/index.ts:1267-1291), so it cannot preserve the invoking tile's live transcript or reject an in-flight tile.
  • The test change at index.test.tsx:153 is only a harness mock; it does not exercise /branch with a tile target.

Suggested changes

  • Carry sessionHint into a runtime-aware branch operation, using that runtime's cached transcript, busy state, parent id, and gateway context. Fail closed if the explicit runtime is gone.
  • Add regression coverage for tile targeting, a busy tile, and a missing tile runtime.

Automated hermes-sweeper review.

}
}

await branchCurrentSession()

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.

An explicit non-foreground sessionHint that has disappeared from $sessionStates reaches this foreground-only call. That recreates the cross-session branch bug; an explicit target must fail closed rather than silently branch the active chat.

const storedId = states[sessionHint]?.storedSessionId

if (storedId) {
await branchStoredSession(storedId)

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.

branchStoredSession is the sidebar/stored-session path: it re-reads persisted history and does not use this tile runtime's busy state. /branch from a live tile needs a runtime-aware branch path so an in-flight tile cannot branch stale history.

@teknium1 teknium1 added the sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform label Jul 30, 2026
@andyst-dev

Copy link
Copy Markdown
Contributor

Closing as superseded by #73251.

#73251 covers the full case this PR introduced sessionHint for (its test 'branches the tile runtime that invoked /branch' verifies the exact path) and goes further: it threads the target into branchCurrentSession and resolves offscreen branch state from the invoking runtime's isolated cache, failing closed when the target is gone — which removes the foreground-fallback gap noted in this PR's review (teknium1). Nothing in this diff is missing there; #73251 is a strict superset.

Credit preserved: #73251 was authored on the runtime-targeted shape this PR surfaced. Thanks for the isolation work.

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/*) 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.

[Bug]: Desktop /branch in a session tile branches the foreground chat

5 participants