Skip to content

fix(desktop): route /yolo, /handoff and /skin through the shared session resolver - #71832

Open
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/desktop-slash-target-session-resolver-71818
Open

fix(desktop): route /yolo, /handoff and /skin through the shared session resolver#71832
briandevans wants to merge 1 commit into
NousResearch:mainfrom
briandevans:fix/desktop-slash-target-session-resolver-71818

Conversation

@briandevans

Copy link
Copy Markdown
Contributor

What does this PR do?

584b27449 ("slash commands target the user's chat, not a new session") introduced
resolveTargetSessionId and moved the slash pipeline onto it, so a command runs against the
conversation the user is actually looking at. Three action handlers were left behind on the old
idiom
the new resolver's docblock names as the bug it exists to prevent:

line handler current
slash.ts:547 /yolo const sid = sessionHint || activeSessionIdRef.current
slash.ts:577 /handoff const sid = sessionHint || activeSessionIdRef.current
slash.ts:629 /skin const sid = sessionHint || activeSessionIdRef.current

From the chat composer, submitText forwards an explicit target only for a queue drain or a tile
(use-prompt-actions/index.ts:524), so sessionHint is undefined in the ordinary case and the
bare ref is the value used.

That skips the resolver's rung 2 route check (routedNeedsResume). When the durable route names
conversation B while the runtime ref still points at A — profile swap, gateway reconnect,
an in-flight resume — the resolver resumes B, but the bare read returns A.

User-visible symptom. You stream in chat A, click chat B in the sidebar, type /yolo in B.
The per-session approval bypass is written to A (config.set { key: 'yolo', session_id },
lib/yolo-session.ts:15). B keeps prompting for every tool approval, so you press /yolo again —
toggling A back off — while A (possibly a background agent) spent that window auto-approving tool
calls. The confirmation line is appended to A's transcript, so B shows nothing at all.
/handoff <platform> has the same shape and is worse in one respect: the wrong conversation's
transcript leaves the app for Telegram/Discord.

All three now resolve through a resolveExistingSessionId helper defined next to ensureSessionId
— the same ladder with createSession: async () => null, so these verbs can never mint a backend
session and the deliberate no-session behaviour survives byte-for-byte (/yolo arms locally,
/skin toasts instead of spinning up a session just to change a theme, /handoff reports it
cannot run). All three already branch on !sid, so the null return needs no new handling.

Sibling-site sweep

grep -rn "|| activeSessionIdRef.current" apps/desktop/src returns 8 non-test sites. This PR is
the complete set of the ones sharing this root cause; the other five are deliberately excluded:

  • use-prompt-actions/index.ts:421 — the shared handoffSession helper. Its only in-repo caller is
    slash.ts:585, which passes an explicit id once this lands, so the fallback becomes a defensive
    default rather than a live path.
  • workspace-session-target.ts:44, use-cwd-actions.ts:28, use-cwd-actions.ts:72 — presence
    tests (if (… || activeSessionIdRef.current)), not target resolution.
  • use-preview-routing.ts:41 — already a ladder with the route ahead of the ref; different concern.

The other activeSessionIdRef.current reads in index.ts (cancelRun, redirectPrompt,
reloadFromMessage, restoreToMessage, editMessage) are 771f1b7f2's deliberate design: they
act on the foreground thread and must not resume a different conversation.

Related Issue

No filed issue — found by auditing the call sites 584b27449 did not convert.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • apps/desktop/src/app/session/hooks/use-prompt-actions/slash.ts
    • Add resolveExistingSessionId beside ensureSessionId: resolveTargetSessionId with the
      create rung disabled.
    • /yolo (:547), /handoff (:577), /skin (:629) now resolve through it.
    • /yolo snapshots !$yoloActive.get() before the resolve so the toggle still inverts what the
      user saw; /skin applies the theme before the resolve so it stays instant.
  • apps/desktop/src/app/session/hooks/use-prompt-actions/slash-target-session.test.tsx — new
    regression file (kept out of index.test.tsx, which several open PRs are editing).

How to Test

  1. cd apps/desktop && npx vitest run --project ui src/app/session/hooks/use-prompt-actions/
    136 passing (5 files).

  2. Regression guard — with slash.ts reverted to main and the new test file kept:

    × writes /yolo to the routed chat, not the stale runtime ref
    × hands off the routed chat, not the stale runtime ref
    × prints /skin confirmation into the routed chat, not the stale runtime ref
    Tests  3 failed | 1 passed (4)
    

    /yolo on main produces config.set { session_id: 'rt-chat-a' } and issues no
    session.resume; /handoff produces handoff.request { session_id: 'rt-chat-a' }. With the fix
    applied, all 4 pass. The 4th case ("still arms /yolo locally on a new-chat draft instead of
    minting a session") is green both before and after by design — it pins the behaviour the
    createSession: async () => null rung preserves.

  3. Manually: open chat A and let it stream, click chat B in the sidebar, type /yolo. Before, the
    zap and the config.set land on A. After, they land on B.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate — no open PR touches use-prompt-actions/slash.ts
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • N/A — JS-only change, no Python touched. Ran npx vitest run --project ui src/app/session/ instead: 342 passing (29 files). Also npx eslint (clean) and npx tsc -p . --noEmit (clean).
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS 15 (Darwin 25.4), Node 22

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A (pure renderer logic, no platform-specific code)
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

…ion resolver

584b274 moved slash-command targeting onto `resolveTargetSessionId` so a
command runs against the conversation the user is looking at. Three action
handlers were left on the old bare `hint || activeSessionIdRef.current` read:
`/yolo`, `/handoff` and `/skin`. From the chat composer `sessionHint` is always
undefined, so the stale ref *is* the target.

That skips the resolver's rung 2 route check. When the durable route names
conversation B while the runtime ref still points at A -- profile swap, gateway
reconnect, an in-flight resume -- the resolver resumes B but the bare read
returns A. `/yolo` then writes the per-session approval bypass to A via
`config.set`, so A keeps auto-approving tool calls while B still prompts for
every one, and the confirmation is appended to A's transcript instead of B's.
`/handoff <platform>` is worse in one respect: it ships the wrong
conversation's transcript out of the app to Telegram/Discord.

All three now go through `resolveExistingSessionId` -- the same ladder with the
create rung disabled, so none of them can mint a backend session and the
deliberate no-session behaviour survives unchanged (`/yolo` arms locally,
`/skin` toasts, `/handoff` reports it cannot run).
@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 sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 26, 2026
@briandevans

Copy link
Copy Markdown
Contributor Author

Two commits landed on this exact function today, both the same class this PR closes out: #71805 (merged 06:50 UTC) bound the busy gate and withSlashOutput's writer to the target session instead of the foreground, and 2a6368f04 (#71891, merged 10:15 UTC) forwarded the resolved {sessionId, storedSessionId} pair into submitPromptText so a skill's kickoff lands in the tab that invoked it. Both fix consumers of the dispatcher's resolved target.

The three handlers here are the remaining call sites that still ask the foreground directly — on current main (eb5276056), slash.ts:556, :586 and :638 are each still const sid = sessionHint || activeSessionIdRef.current, for /yolo, /handoff and /skin. This routes those three through the same resolveTargetSessionId ladder, minus the create rung, since none of them should mint a session.

Still MERGEABLE/CLEAN against that tip, with 36 checks reported and zero failures.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for completing the remaining action-handler conversion. The premise is confirmed on current main: /yolo, /handoff, and /skin still bypass the route-aware target resolver at apps/desktop/src/app/session/hooks/use-prompt-actions/slash.ts:624, :715, and :767. The established resolver specifically resumes the routed durable session when its runtime binding disagrees with the active ref (resolve-target-session.ts:74-100).

The PR reuses that resolver with a null create rung, preserving the existing no-session behavior while preventing those three handlers from acting on a stale runtime id. A current-main search found these are the only remaining sessionHint || activeSessionIdRef.current handler sites in the slash dispatcher.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 30, 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/*) P2 Medium — degraded but workaround exists sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data 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