diff --git a/agent/skill_preprocessing.py b/agent/skill_preprocessing.py index 44c5714b9b39..5bbfaf034d95 100644 --- a/agent/skill_preprocessing.py +++ b/agent/skill_preprocessing.py @@ -69,6 +69,27 @@ def run_inline_shell(command: str, cwd: Path | None, timeout: int) -> str: raising, so one bad snippet can't wreck the whole skill message. """ _popen_kwargs = {"creationflags": windows_hide_flags()} if IS_WINDOWS else {} + # Same central env factory as every other spawn surface: the snippet gets + # the session-context stamps (HERMES_SESSION_*, the write-only Desktop + # connection-mode stamp) and passes through the inherited-value scrub — + # a value inherited from the user's shell is stripped, never honored. + try: + from tools.environments.local import build_subprocess_env + + _run_env = build_subprocess_env() + except Exception: + # FAIL CLOSED. Falling back to ``env=None`` makes subprocess.run + # inherit the RAW parent environment, which is the one door the scrub + # above exists to shut: an ambient HERMES_DESKTOP_CONNECTION_MODE=local + # set in the user's shell would reach the snippet and be read as the + # resolved Desktop mode, re-opening the spoofing path. Not running the + # snippet is the safe outcome and costs nothing the caller cannot + # absorb — it already treats a marker as a non-fatal result. + logger.warning( + "build_subprocess_env unavailable for inline shell; refusing to run the snippet", + exc_info=True, + ) + return "[inline-shell error: sanitized environment unavailable]" try: completed = subprocess.run( ["bash", "-c", command], @@ -78,6 +99,7 @@ def run_inline_shell(command: str, cwd: Path | None, timeout: int) -> str: timeout=max(1, int(timeout)), check=False, stdin=subprocess.DEVNULL, + env=_run_env, **_popen_kwargs, ) except subprocess.TimeoutExpired: diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts index 04f53f785b48..e4c1bdf28d39 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-request.ts @@ -3,6 +3,7 @@ import { useStore } from '@nanostores/react' import { useCallback, useEffect, useRef } from 'react' import type { HermesGateway } from '@/hermes' +import { announceConnectionMode } from '@/lib/connection-mode' import { $gateway, ensureActiveGatewayOpen, isActivePrimary } from '@/store/gateway' import { $activeGatewayProfile } from '@/store/profile' import { $gatewayState, setConnection } from '@/store/session' @@ -104,15 +105,22 @@ export function useGatewayRequest() { }, []) const requestGateway = useCallback( - async (method: string, params: Record = {}, timeoutMs?: number, signal?: AbortSignal) => { + async (method: string, rawParams: Record = {}, timeoutMs?: number, signal?: AbortSignal) => { const gateway = gatewayRef.current if (!gateway) { throw new Error('Hermes gateway unavailable') } + // Announce the live connection mode on session/prompt RPCs (#82140). + // Resolved per attempt, not per call: $connection is published in the + // same synchronous frame as a profile switch (ensureGatewayProfile) and + // is rewritten by the reconnect below, so re-reading on the retry sends + // the mode of the connection the retry actually lands on. + const announce = () => announceConnectionMode(method, rawParams) + try { - return await gateway.request(method, params, timeoutMs, signal) + return await gateway.request(method, announce(), timeoutMs, signal) } catch (error) { const message = error instanceof Error ? error.message : String(error) @@ -138,7 +146,7 @@ export function useGatewayRequest() { throw error } - return recovered.request(method, params, timeoutMs, signal) + return recovered.request(method, announce(), timeoutMs, signal) } }, [ensureGatewayOpen] diff --git a/apps/desktop/src/contrib/plugin.test.ts b/apps/desktop/src/contrib/plugin.test.ts index 9f522fe7038b..495887ff0413 100644 --- a/apps/desktop/src/contrib/plugin.test.ts +++ b/apps/desktop/src/contrib/plugin.test.ts @@ -1,6 +1,8 @@ -import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { HermesConnection } from '@/global' import { dispatchPluginNativeNotification } from '@/store/native-notifications' +import { $connection, setConnection } from '@/store/session' import { createPluginContext } from './plugin' @@ -60,3 +62,118 @@ describe('createPluginContext.os', () => { } }) }) + +describe('createPluginContext.connection', () => { + afterEach(() => { + setConnection(null) + }) + + const conn = (mode?: 'local' | 'remote') => + ({ baseUrl: 'http://127.0.0.1:8787', mode, token: 'secret' }) as unknown as HermesConnection + + it('reports the live mode without exposing the descriptor', () => { + setConnection(conn('remote')) + const ctx = createPluginContext('demo') + + expect(ctx.connection.mode()).toBe('remote') + // The whole door is two functions — there is no descriptor to reach past. + expect(Object.keys(ctx.connection).sort()).toEqual(['mode', 'onModeChange']) + }) + + it('reports null before a connection resolves', () => { + expect(createPluginContext('demo').connection.mode()).toBeNull() + }) + + it('fires immediately and on every real transition', () => { + setConnection(conn('local')) + const seen: Array<'local' | 'remote' | null> = [] + createPluginContext('demo').connection.onModeChange(mode => seen.push(mode)) + + setConnection(conn('remote')) + setConnection(null) + + expect(seen).toEqual(['local', 'remote', null]) + }) + + it('stays quiet when a descriptor refresh does not move the mode', () => { + setConnection(conn('remote')) + const listener = vi.fn() + createPluginContext('demo').connection.onModeChange(listener) + + // A reconnect re-mints the descriptor (new token/wsUrl) on the same mode. + setConnection({ ...conn('remote'), token: 'rotated' } as unknown as HermesConnection) + + expect(listener).toHaveBeenCalledTimes(1) + }) + + it('stops listening when the plugin unloads, even if the disposer is ignored', () => { + setConnection(conn('local')) + const disposers: Array<() => void> = [] + const listener = vi.fn() + createPluginContext('demo', dispose => disposers.push(dispose)).connection.onModeChange(listener) + + disposers.forEach(dispose => dispose()) + setConnection(conn('remote')) + + expect(listener).toHaveBeenCalledTimes(1) + expect($connection.get()?.mode).toBe('remote') + }) + + it('contains a throw from the immediate notification', () => { + // The immediate call runs inside the plugin's register; a throw must not + // escape into the loader. + setConnection(conn('local')) + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined) + + let unsubscribe = () => {} + + try { + expect(() => { + unsubscribe = createPluginContext('demo').connection.onModeChange(() => { + throw new Error('plugin bug') + }) + }).not.toThrow() + expect(error).toHaveBeenCalledWith(expect.stringContaining('demo'), expect.any(Error)) + } finally { + unsubscribe() + error.mockRestore() + } + }) + + it('contains a throw on a real transition and keeps other listeners running', () => { + // The subscription fires inside core setConnection (boot, reconnect, + // profile switch); a plugin throw escaping there would abort profile + // synchronization. It must be contained, attributed, and must not starve + // sibling listeners — including the thrower staying subscribed for + // later transitions. + setConnection(conn('local')) + const error = vi.spyOn(console, 'error').mockImplementation(() => undefined) + const unsubscribes: Array<() => void> = [] + + try { + const seen: Array<'local' | 'remote' | null> = [] + unsubscribes.push( + createPluginContext('bad').connection.onModeChange(mode => { + if (mode === 'remote') { + throw new Error('plugin bug') + } + }) + ) + unsubscribes.push(createPluginContext('good').connection.onModeChange(mode => seen.push(mode))) + + expect(() => setConnection(conn('remote'))).not.toThrow() + expect(seen).toEqual(['local', 'remote']) + expect(error).toHaveBeenCalledWith(expect.stringContaining('bad'), expect.any(Error)) + + // The throwing listener is still subscribed and hears later transitions + // (a non-throwing one this time — nothing new is reported). + error.mockClear() + expect(() => setConnection(conn('local'))).not.toThrow() + expect(seen).toEqual(['local', 'remote', 'local']) + expect(error).not.toHaveBeenCalled() + } finally { + unsubscribes.forEach(unsubscribe => unsubscribe()) + error.mockRestore() + } + }) +}) diff --git a/apps/desktop/src/contrib/plugin.ts b/apps/desktop/src/contrib/plugin.ts index aa5d0f107ad7..50b64c933990 100644 --- a/apps/desktop/src/contrib/plugin.ts +++ b/apps/desktop/src/contrib/plugin.ts @@ -14,13 +14,16 @@ import { pluginRest, type PluginRestOptions, pluginSocket } from '@/hermes' import { createPluginI18n, type PluginI18n } from '@/i18n' +import { type HermesConnectionMode, resolveConnectionMode } from '@/lib/connection-mode' import { readKey, writeKey } from '@/lib/storage' import { dispatchPluginNativeNotification, type PluginNativeNotificationInput } from '@/store/native-notifications' +import { $connection } from '@/store/session' import { registry } from './registry' import type { Contribution } from './types' export type { PluginRestOptions } from '@/hermes' +export type { HermesConnectionMode } from '@/lib/connection-mode' export type { PluginNativeNotificationInput } from '@/store/native-notifications' /** A contribution as a plugin author writes it — provenance + id scoping are @@ -56,6 +59,25 @@ export interface PluginOs { writeClipboard: (text: string) => Promise } +/** The supported read of the resolved backend connection — the connection's + * *shape*, never its credentials. `mode` is `'local'` when this Desktop drives + * its own local backend (a path the agent reports is already openable here), + * `'remote'` when it drives an SSH/URL/cloud backend (a gateway-side path must + * be transferred first), and `null` when it isn't resolved yet. + * + * Base URL, host, tokens, SSH keys, and auth mode stay behind the Electron + * bridge on purpose; a plugin that needs to move a file should ask the backend + * to do it, not dial the backend itself. See #82140. */ +export interface PluginConnection { + /** The live mode, read at call time. */ + mode: () => HermesConnectionMode | null + /** Subscribe to mode changes (connection switch, profile switch, reconnect). + * Fires immediately with the current value. Returns an unsubscribe; it is + * also registered with `onDispose`, so a plugin that ignores the return + * value still stops listening when it unloads. */ + onModeChange: (listener: (mode: HermesConnectionMode | null) => void) => () => void +} + export interface PluginContext { /** The resolved plugin source tag, e.g. `'plugin:cost-meter'`. */ readonly source: string @@ -81,6 +103,9 @@ export interface PluginContext { * manager, clipboard — attributed to this plugin, result-shaped (never * throws for a missing capability). */ os: PluginOs + /** Is the backend's filesystem the machine the user is looking at? The + * supported answer to that question — mode only, no credentials. */ + connection: PluginConnection /** Plugin-scoped persistence. */ storage: PluginStorage /** Plugin-scoped i18n: ship + register locale bundles under this plugin, @@ -156,6 +181,49 @@ function createPluginOs(pluginId: string): PluginOs { } } +// Reads the resolved mode off the live connection atom rather than calling the +// Electron bridge: the atom is what stays in lockstep with the ACTIVE profile +// (published atomically with a profile switch), so a plugin sees the same mode the session +// RPCs announce. A raw bridge.getConnection() would describe the primary window +// backend, which is the wrong answer whenever a background profile is active. +function createPluginConnection(pluginId: string, track: (dispose: () => void) => () => void): PluginConnection { + // Isolate every listener call: the subscription below runs inside core + // connection updates (setConnection during boot, reconnect, and profile + // switches), so a plugin throw escaping here would abort profile + // synchronization/reconnect code — one bad plugin destabilizing the + // renderer. Same containment contract as gateway event listeners. + const invoke = (listener: (mode: HermesConnectionMode | null) => void, mode: HermesConnectionMode | null) => { + try { + listener(mode) + } catch (error) { + console.error(`[plugins] ${pluginId}: connection mode listener failed`, error) + } + } + + return { + mode: () => resolveConnectionMode($connection.get()), + onModeChange: listener => { + let previous = resolveConnectionMode($connection.get()) + + invoke(listener, previous) + + // $connection changes on every reconnect and descriptor refresh, most of + // which don't move the mode. Only forward real transitions so a plugin + // can put its transfer/cleanup work straight in the listener. + return track( + $connection.subscribe(connection => { + const next = resolveConnectionMode(connection) + + if (next !== previous) { + previous = next + invoke(listener, next) + } + }) + ) + } + } +} + /** Build the scoped context handed to a plugin's `register`. `onDispose` * receives every registration's disposer (the loader's unload/reload hook). */ export function createPluginContext(pluginId: string, onDispose?: (dispose: () => void) => void): PluginContext { @@ -176,6 +244,7 @@ export function createPluginContext(pluginId: string, onDispose?: (dispose: () = rest: (path: string, opts?: PluginRestOptions) => pluginRest(pluginId, path, opts), socket: (path, onMessage) => track(pluginSocket(pluginId, path, onMessage)), os: createPluginOs(pluginId), + connection: createPluginConnection(pluginId, track), storage: createPluginStorage(pluginId), i18n: createPluginI18n(pluginId, track) } diff --git a/apps/desktop/src/lib/connection-mode.test.ts b/apps/desktop/src/lib/connection-mode.test.ts new file mode 100644 index 000000000000..e6f1b486aab8 --- /dev/null +++ b/apps/desktop/src/lib/connection-mode.test.ts @@ -0,0 +1,86 @@ +import { describe, expect, it } from 'vitest' + +import type { HermesConnection } from '@/global' + +import { resolveConnectionMode, withConnectionMode } from './connection-mode' + +const conn = (over: Partial = {}) => + ({ baseUrl: 'http://127.0.0.1:8787', ...over }) as HermesConnection + +describe('resolveConnectionMode', () => { + it.each(['local', 'remote'] as const)('passes through the resolved %s mode', mode => { + expect(resolveConnectionMode(conn({ mode }))).toBe(mode) + }) + + it.each([ + ['no descriptor yet', null], + ['bridge unavailable', undefined] + ])('resolves null when there is %s', (_label, value) => { + expect(resolveConnectionMode(value)).toBeNull() + }) + + it('resolves null rather than guessing local for an unset mode', () => { + // An older shell that predates the field. Claiming "local" would tell an + // extension a gateway-side file is openable here when it may not be. + expect(resolveConnectionMode(conn())).toBeNull() + expect(resolveConnectionMode(conn({ mode: 'cloud' as never }))).toBeNull() + }) +}) + +describe('withConnectionMode', () => { + it.each(['session.create', 'session.resume', 'prompt.submit'])('stamps the mode onto %s', method => { + expect(withConnectionMode(method, { text: 'hi' }, 'remote')).toEqual({ + connection_mode: 'remote', + text: 'hi' + }) + }) + + it('leaves unrelated RPCs untouched', () => { + const params = { limit: 40 } + + expect(withConnectionMode('session.list', params, 'remote')).toBe(params) + }) + + it('announces an explicit null when the mode is unknown', () => { + // NOT omission. Omitting the key means "leave the stored value alone" to + // _remember_connection_mode, so a `local` announced before a reconnect + // would survive into turns that can no longer prove it. An explicit null + // normalizes to None and clears it. + expect(withConnectionMode('prompt.submit', { text: 'hi' }, null)).toEqual({ + connection_mode: null, + text: 'hi' + }) + }) + + it('overrides a caller-supplied mode with the live one', () => { + // connection_mode is renderer-owned. The plugin SDK reaches this same + // door, so a caller value winning would let a plugin on a live REMOTE + // session announce `local` and be believed. + expect(withConnectionMode('prompt.submit', { connection_mode: 'local' }, 'remote')).toEqual({ + connection_mode: 'remote' + }) + }) + + it('never lets a caller-supplied local survive an unknown live mode', () => { + expect(withConnectionMode('prompt.submit', { connection_mode: 'local' }, null)).toEqual({ + connection_mode: null + }) + }) + + it('clears a previously announced local when the live mode goes unknown', () => { + // The reconnect window: the backend is holding `local` from an earlier + // turn and this turn cannot resolve a descriptor. Unknown must never be + // guessed as local, so the announcement has to clear rather than skip. + const reconnecting = withConnectionMode('prompt.submit', { text: 'hi' }, null) + + expect(reconnecting).toHaveProperty('connection_mode', null) + expect('connection_mode' in reconnecting).toBe(true) + }) + + it('does not mutate the caller params', () => { + const params = { text: 'hi' } + withConnectionMode('prompt.submit', params, 'local') + + expect(params).toEqual({ text: 'hi' }) + }) +}) diff --git a/apps/desktop/src/lib/connection-mode.ts b/apps/desktop/src/lib/connection-mode.ts new file mode 100644 index 000000000000..289c61636ab3 --- /dev/null +++ b/apps/desktop/src/lib/connection-mode.ts @@ -0,0 +1,91 @@ +/** + * The resolved Desktop connection mode — the one connection fact extensions are + * allowed to see (NousResearch/hermes-agent#82140). + * + * `local` — this Desktop drives its own local backend, so a path the agent + * reports is already a path on the machine the user is looking at. + * `remote` — this Desktop drives an SSH/URL/cloud backend, so a gateway-side + * path has to be transferred before the Desktop can open it. + * + * That distinction is the whole point: without it an extension can't tell + * whether `/home/user/report.md` is openable here, which is what makes `MEDIA:` + * and file-link handling ambiguous on remote gateways. + * + * Everything else on the connection descriptor — base URL, host, identity, + * tokens, auth mode — stays behind the Electron bridge. Extensions get the + * shape of the connection, never the credentials for it. + */ + +import type { HermesConnection } from '@/global' +import { $connection } from '@/store/session' + +export type HermesConnectionMode = 'local' | 'remote' + +/** RPCs on which the renderer announces its live mode to the backend. Session + * lifecycle pins it for new/resumed chats; `prompt.submit` re-announces every + * turn so switching the active connection or profile lands immediately rather + * than being stuck at whatever was true when the chat opened. */ +const CONNECTION_MODE_METHODS = new Set(['prompt.submit', 'session.create', 'session.resume']) + +/** + * Narrow a connection descriptor to its mode. + * + * The descriptor's `mode` is already resolved (a `cloud` saved config resolves + * to a `remote` connection), so this only has to guard the "no descriptor yet" + * and "older shell that predates the field" cases — both of which resolve to + * null. Null means "unknown", never "local": telling an extension a remote file + * is local hands the user a link to a file that isn't on their machine. + */ +export function resolveConnectionMode(connection: HermesConnection | null | undefined): HermesConnectionMode | null { + const mode = connection?.mode + + return mode === 'local' || mode === 'remote' ? mode : null +} + +/** + * Stamp `connection_mode` onto the params of an RPC that carries it. + * + * Applied at the single `requestGateway` choke point rather than at each of the + * ~10 call sites, so a new session/prompt path announces correctly by + * construction instead of by remembering to. + * + * `connection_mode` is a RESERVED, renderer-owned field: whatever the caller + * put there is discarded and the live resolved mode is written in its place. + * The plugin SDK's `host.request` reaches this same door, so honouring a + * caller value would let a plugin driving a live REMOTE session announce + * `local` and have the backend hand its skills and MCP context paths as though + * they were on the user's machine — the exact spoof the field exists to + * prevent. Only the renderer can see the descriptor, so only the renderer may + * answer. + * + * An unresolved mode announces an explicit `null` rather than omitting the + * key. Omitting it means "leave the stored value alone" to the backend + * (`_remember_connection_mode`), so a `local` announced before a reconnect + * would survive into turns that can no longer prove it — and "unknown must + * never be guessed as local" is the whole safety rule here. + * `normalize_desktop_connection_mode(None)` is `None`, so this clears it. + */ +export function withConnectionMode( + method: string, + params: Record, + mode: HermesConnectionMode | null +): Record { + if (!CONNECTION_MODE_METHODS.has(method)) { + return params + } + + return { ...params, connection_mode: mode } +} + +/** + * The one announcement helper every gateway-request door shares. + * + * Reads the live `$connection` at CALL time (so a retry announces the mode of + * the connection it actually lands on) and stamps it via `withConnectionMode`. + * Both `useGatewayRequest` (app/hook callers) and the plugin SDK's + * `host.request` go through here — a request door that skips it lets a plugin + * drive a Desktop session whose skills/MCP context never learns the mode. + */ +export function announceConnectionMode(method: string, params: Record): Record { + return withConnectionMode(method, params, resolveConnectionMode($connection.get())) +} diff --git a/apps/desktop/src/sdk/index.test.ts b/apps/desktop/src/sdk/index.test.ts index 83c68d9a7b38..28815af1927f 100644 --- a/apps/desktop/src/sdk/index.test.ts +++ b/apps/desktop/src/sdk/index.test.ts @@ -1,8 +1,10 @@ -import { afterEach, describe, expect, it } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' +import type { HermesConnection } from '@/global' import { createClientSessionState } from '@/lib/chat-runtime' import { host } from '@/sdk' -import { setActiveSessionId, setAwaitingResponse, setBusy } from '@/store/session' +import { $gateway } from '@/store/gateway' +import { setActiveSessionId, setAwaitingResponse, setBusy, setConnection } from '@/store/session' import { clearAllSessionStates, publishSessionState } from '@/store/session-states' describe('host.state turn flags', () => { @@ -107,3 +109,159 @@ describe('host.state turn flags', () => { $sessionTiles.set([]) }) }) + +/** + * The plugin SDK's `host.request` door must announce the Desktop connection + * mode on session/prompt RPCs exactly like `useGatewayRequest` does — it used + * to send straight through `$gateway.get().request()`, letting a runtime + * plugin create or drive a session whose skills/MCP context never learned the + * mode (#82187 follow-up review, item 3). + */ + +const conn = (mode?: 'local' | 'remote') => ({ baseUrl: 'http://127.0.0.1:8787', mode }) as unknown as HermesConnection + +describe('host.request connection-mode announcement', () => { + afterEach(() => { + $gateway.set(null as never) + setConnection(null) + }) + + const installGateway = () => { + const request = vi.fn().mockResolvedValue('ok') + $gateway.set({ request } as never) + + return request + } + + it.each(['session.create', 'session.resume', 'prompt.submit'])('stamps the live mode onto %s', async method => { + setConnection(conn('remote')) + const request = installGateway() + + await expect(host.request(method, { text: 'hi' })).resolves.toBe('ok') + expect(request).toHaveBeenCalledWith(method, { connection_mode: 'remote', text: 'hi' }) + }) + + it('leaves unrelated RPCs untouched', async () => { + setConnection(conn('remote')) + const request = installGateway() + const params = { limit: 3 } + + await host.request('session.list', params) + + expect(request).toHaveBeenCalledWith('session.list', params) + }) + + it('announces an explicit null when the mode is unknown', async () => { + // Null descriptor (reconnect window / older shell). Announcing an explicit + // null CLEARS the backend's remembered mode; omitting the key leaves it + // alone (`_remember_connection_mode` only writes when the key is present), + // so a `local` announced before the reconnect would survive into turns + // that can no longer prove it. Unknown must never read as local. + const request = installGateway() + + await host.request('prompt.submit', { text: 'hi' }) + + expect(request).toHaveBeenCalledWith('prompt.submit', { connection_mode: null, text: 'hi' }) + }) + + it('still throws when no gateway socket is live', async () => { + setConnection(conn('remote')) + + await expect(host.request('prompt.submit', {})).rejects.toThrow('Hermes gateway unavailable') + }) +}) + +/** + * `host.getGateway()` is the SDK's OTHER request door. It hands out the live + * instance for components that take a `HermesGateway` prop, so a plugin can + * reach `getGateway().request(...)` — which would otherwise bypass the + * announcement that `host.request` performs and reopen exactly the gap item 3 + * of the follow-up review closed. + */ +describe('host.getGateway connection-mode announcement', () => { + afterEach(() => { + $gateway.set(null as never) + setConnection(null) + }) + + it('announces on session/prompt RPCs through the returned instance', async () => { + setConnection(conn('remote')) + const request = vi.fn().mockResolvedValue('ok') + $gateway.set({ request } as never) + + await host.getGateway()?.request('prompt.submit', { text: 'hi' }) + + expect(request).toHaveBeenCalledWith('prompt.submit', { connection_mode: 'remote', text: 'hi' }) + }) + + it('leaves unrelated RPCs untouched', async () => { + setConnection(conn('remote')) + const request = vi.fn().mockResolvedValue('ok') + $gateway.set({ request } as never) + const params = { limit: 3 } + + await host.getGateway()?.request('session.list', params) + + expect(request).toHaveBeenCalledWith('session.list', params) + }) + + it('forwards the timeout and abort signal the caller passed', async () => { + // HermesGateway.request is (method, params, timeoutMs, signal). The + // announcing wrapper stands in for it, so a two-argument wrapper silently + // dropped arguments three and four: the request fell back to the default + // deadline and could no longer be aborted. Every other test here calls the + // two-argument form, so green CI did not cover it. + setConnection(conn('remote')) + const request = vi.fn().mockResolvedValue('ok') + $gateway.set({ request } as never) + const controller = new AbortController() + + await host.getGateway()?.request('prompt.submit', { text: 'hi' }, 1234, controller.signal) + + expect(request).toHaveBeenCalledWith( + 'prompt.submit', + { connection_mode: 'remote', text: 'hi' }, + 1234, + controller.signal + ) + // Identity, not shape: a copied-but-equal signal would abort nothing. + expect(request.mock.calls[0][3]).toBe(controller.signal) + }) + + it('forwards a timeout and signal on RPCs it does not stamp', async () => { + setConnection(conn('remote')) + const request = vi.fn().mockResolvedValue('ok') + $gateway.set({ request } as never) + const controller = new AbortController() + const params = { limit: 3 } + + await host.getGateway()?.request('session.list', params, 99, controller.signal) + + expect(request).toHaveBeenCalledWith('session.list', params, 99, controller.signal) + }) + + it('delegates non-request members to the real instance', () => { + const close = vi.fn() + setConnection(conn('remote')) + $gateway.set({ close, request: vi.fn(), wsUrl: 'ws://127.0.0.1:8787' } as never) + + const gateway = host.getGateway() as unknown as { close: () => void; wsUrl: string } + + expect(gateway.wsUrl).toBe('ws://127.0.0.1:8787') + gateway.close() + expect(close).toHaveBeenCalledOnce() + }) + + it('hands back a stable reference for one live gateway', () => { + // SDK components take this as a React prop; a fresh wrapper per call would + // churn every memo/effect dependency keyed on it. + setConnection(conn('remote')) + $gateway.set({ request: vi.fn() } as never) + + expect(host.getGateway()).toBe(host.getGateway()) + }) + + it('stays null before the first socket opens', () => { + expect(host.getGateway()).toBeNull() + }) +}) diff --git a/apps/desktop/src/sdk/index.ts b/apps/desktop/src/sdk/index.ts index 032a58bd4d44..aa80685f4e76 100644 --- a/apps/desktop/src/sdk/index.ts +++ b/apps/desktop/src/sdk/index.ts @@ -26,6 +26,7 @@ import type { ClientSessionState } from '@/app/types' import { $narrowViewport } from '@/components/pane-shell/tree/store' import { onGatewayEvent } from '@/contrib/events' import { deleteProfile, getLogs, getStatus, type HermesGateway } from '@/hermes' +import { announceConnectionMode } from '@/lib/connection-mode' import { $gateway, openGatewayForAgent, @@ -93,6 +94,58 @@ export interface PluginProfileRoute { targetProfile: string } +// One announcing view per real gateway, so repeated `getGateway()` calls hand +// back a stable reference — SDK components take this as a React prop, and a +// fresh wrapper per render would churn every memo and effect dependency on it. +const announcingGateways = new WeakMap() + +/** A gateway whose `request` announces the connection mode, like `host.request`. + * + * A Proxy rather than a spread copy or a subclass: `HermesGateway` is the live + * socket wrapper, so its methods close over connection state that only exists + * on the real instance. Every member except `request` passes straight through, + * bound to the target — calling a delegated method with the proxy as `this` + * would break any private-field access inside it. + */ +const announcingGateway = (gateway: HermesGateway): HermesGateway => { + const cached = announcingGateways.get(gateway) + + if (cached) { + return cached + } + + const wrapped = new Proxy(gateway, { + get(target, prop) { + if (prop === 'request') { + // Mirror the FULL HermesGateway.request signature. A two-argument + // wrapper silently swallows `timeoutMs` and `signal`, so any SDK + // caller passing them lost its custom deadline and its ability to + // abort - a wrapper must not narrow the contract it stands in for. + // + // The tail is forwarded as a REST spread rather than as two named + // parameters so the delegated call carries exactly the arguments the + // caller made. Naming them re-materializes omitted arguments as + // explicit `undefined`, which is invisible to a defaulted parameter + // but not to anything reading `arguments.length` - and it makes every + // pass-through call site un-assertable on its real shape. + return ( + method: string, + params: Record = {}, + ...rest: [timeoutMs?: number, signal?: AbortSignal] + ): Promise => target.request(method, announceConnectionMode(method, params), ...rest) + } + + const value = Reflect.get(target, prop) + + return typeof value === 'function' ? value.bind(target) : value + } + }) + + announcingGateways.set(gateway, wrapped) + + return wrapped +} + /** Window geometry + the app's responsive posture, one readonly rect. */ export interface ViewportRect { width: number @@ -401,7 +454,10 @@ export const host = { ): Promise => requestPluginProfile(route, method, params), /** Gateway JSON-RPC — sessions, config, skills, cron, kanban, everything - * the app itself uses. Lazy: resolves the LIVE socket per call. */ + * the app itself uses. Lazy: resolves the LIVE socket per call. Session and + * prompt RPCs announce the live Desktop connection mode through the same + * helper `useGatewayRequest` uses, so a plugin-driven session's skills/MCP + * context sees the mode a hook-driven one would (#82140). */ request: async (method: string, params: Record = {}): Promise => { const gateway = $gateway.get() @@ -409,15 +465,23 @@ export const host = { throw new Error('Hermes gateway unavailable') } - return gateway.request(method, params) + return gateway.request(method, announceConnectionMode(method, params)) }, /** The LIVE gateway instance for the active profile (null before the first * socket opens). Most plugins want `host.request`; this exists for SDK * components that take a `HermesGateway` prop directly (e.g. `McpTab`), * which need the instance, not just a JSON-RPC door. Re-read per use — the - * active instance changes on a profile swap. */ - getGateway: (): HermesGateway | null => $gateway.get() + * active instance changes on a profile swap. + * + * Announcing, like `host.request`: this is the SDK's other request door, and + * a door that skips the announcement lets a plugin drive a Desktop session + * whose skills/MCP context never learns the mode (#82140). */ + getGateway: (): HermesGateway | null => { + const gateway = $gateway.get() + + return gateway ? announcingGateway(gateway) : gateway + } } // -- react bridge ------------------------------------------------------------- @@ -523,7 +587,9 @@ export { Textarea } from '@/components/ui/textarea' export { Tip, Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from '@/components/ui/tooltip' export type { GatewayEventListener } from '@/contrib/events' export type { + HermesConnectionMode, HermesPlugin, + PluginConnection, PluginContext, PluginContribution, PluginNativeNotificationInput, diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index b9640203143b..c907ca7b4415 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -606,13 +606,27 @@ export async function openGatewayForAgent(connectionId: null | string, profile: } } -export async function ensureGatewayForAgent(connectionId: null | string, profile: string): Promise { +// The agent-scoped analogue of prepareGatewayForProfile, and the same +// publication seam: dial the agent's socket without publishing anything, and +// hand back the synchronous activation thunk. A null connection id falls +// through to the profile seam, so both doors into an activation share one +// atomicity contract instead of drifting apart; an explicit `local` id is a +// registry identity (`registryBackendScopeKey` keeps its own scope for it) and +// stays on the registry route. +// +// The thunk reports whether it actually published, preserving the `activated` +// contract callers rely on: a source edit/remove can dispose this entry while +// its dial is in flight, and a caller must be able to tell "switched" from +// "the target stopped existing" rather than assume the former. +export async function prepareGatewayForAgent(connectionId: null | string, profile: string): Promise<() => boolean> { const scope = registryBackendScopeKey(connectionId, profile) + // Genuinely-local scope: the profile door owns this route, so hand back ITS + // thunk unchanged. Wrapping it to return an unconditional `true` would have + // reported a rejected activation as a successful one and let the agent + // caller publish companion state for a switch that never happened. if (scope === normKey(profile)) { - await ensureGatewayForProfile(profile) - - return true + return prepareGatewayForProfile(profile) } if (!window.hermesDesktop?.getConnectionFor) { @@ -641,42 +655,54 @@ export async function ensureGatewayForAgent(connectionId: null | string, profile } } - // A source edit/remove may dispose this entry while its dial is still in - // flight. Only the still-registered, still-owned activation may publish. - const activated = - entry.wantOpen && - g.secondaries.get(scope) === entry && - Boolean(entry.connection) && - applyActive(scope, activationEpoch) + // Bind the entry this dial settled on; see prepareGatewayForProfile. + const prepared = entry + + return () => { + // A source edit/remove may dispose this entry while its dial is still in + // flight. Only the still-registered, still-owned activation may publish. + const activated = + prepared.wantOpen && + g.secondaries.get(scope) === prepared && + Boolean(prepared.connection) && + applyActive(scope, activationEpoch) - if (activated && entry.connection) { - publishActiveConnection(entry.connection) + if (activated && prepared.connection) { + publishActiveConnection(prepared.connection) + } + + return activated } +} - return activated +export async function ensureGatewayForAgent(connectionId: null | string, profile: string): Promise { + return (await prepareGatewayForAgent(connectionId, profile))() } -// Make `profile` the active gateway, lazily opening its socket if needed. The -// primary is a no-op fast path. Background sockets are never closed here. -export async function ensureGatewayForProfile(profile: string): Promise { +// Open `profile`'s socket if needed and hand back a synchronous activation +// thunk — the publication seam for atomic profile switches. The caller invokes +// the thunk in the same synchronous frame as its own atom writes (profile +// pointer, connection descriptor), so no subscriber can observe the active +// gateway pointing at one backend while companion state still describes +// another. Nothing is published until the thunk runs. +export async function prepareGatewayForProfile(profile: string): Promise<() => boolean> { const key = normKey(profile) const activationEpoch = beginGatewayActivation() if (key === g.primaryProfile) { - applyActive(key, activationEpoch) - - return + return () => applyActive(key, activationEpoch) } // Global-remote share (routing case 3): one remote host serves every // profile through the PRIMARY socket, scoped per request. Activate the // primary instead of dialing a doomed duplicate socket at the same - // descriptor — $activeGatewayProfile still moves to `key`, so request - // scoping and profile-aware surfaces behave identically. + // descriptor - $activeGatewayProfile still moves to `key`, so request + // scoping and profile-aware surfaces behave identically. Checked BEFORE + // createSecondary so a shared-remote profile never mints a secondary + // entry, and returned as a thunk like every other path here so this + // switch publishes as atomically as a dedicated-socket one. if (await sharedPrimaryRoute(key)) { - applyActive(g.primaryProfile, activationEpoch) - - return + return () => applyActive(g.primaryProfile, activationEpoch) } let entry = g.secondaries.get(key) @@ -699,11 +725,36 @@ export async function ensureGatewayForProfile(profile: string): Promise { } } - if (entry.wantOpen && g.secondaries.get(key) === entry && applyActive(key, activationEpoch) && entry.connection) { - publishActiveConnection(entry.connection) + // Bind the entry the await settled on. `g.secondaries.get(key)` can be a + // DIFFERENT object by the time the thunk runs (a teardown + redial between + // prepare and publish), and publishing that one's descriptor would be the + // very mismatch this seam exists to prevent, so the identity re-check below + // compares against this exact entry. + const prepared = entry + + // Reports whether the ACTIVATION was accepted, which is a different question + // from whether a descriptor was published: an accepted activation with no + // cached connection still moved the gateway, so the caller must still move + // its companion state. Only a rejected activation (disposed entry, or an + // epoch superseded by a newer switch while this one was dialing) must leave + // every companion store alone. + return () => { + const activated = prepared.wantOpen && g.secondaries.get(key) === prepared && applyActive(key, activationEpoch) + + if (activated && prepared.connection) { + publishActiveConnection(prepared.connection) + } + + return activated } } +// Make `profile` the active gateway, lazily opening its socket if needed. The +// primary is a no-op fast path. Background sockets are never closed here. +export async function ensureGatewayForProfile(profile: string): Promise { + ;(await prepareGatewayForProfile(profile))() +} + // Reconnect the active gateway after a transient request failure. Primary // reconnects are owned by use-gateway-boot, so we only drive secondaries here. export async function ensureActiveGatewayOpen(): Promise { diff --git a/apps/desktop/src/store/profile-agent-activation.test.ts b/apps/desktop/src/store/profile-agent-activation.test.ts index 9e4105629272..124da3010eac 100644 --- a/apps/desktop/src/store/profile-agent-activation.test.ts +++ b/apps/desktop/src/store/profile-agent-activation.test.ts @@ -13,18 +13,56 @@ import type { HermesConnection } from '@/global' // 2. Agent activations share the gatewaySwitch mutex with profile switches — // without it, two rapid activations could complete out of order and the // EARLIER setActive() landed last. +// 3. A SUCCEEDING activation publishes the gateway, the profile pointer and +// the connection descriptor with no asynchronous gap between them. +// Activating first and awaiting the descriptor after left $gateway on the +// new backend while $connection still described the old one. +// 4. A FAILING descriptor lookup publishes none of the three. Swallowing the +// rejection and publishing anyway produced the same mixed state as (3), +// except permanent: (3) closes when the descriptor arrives, whereas a +// failed lookup never arrives and the split survived until an unrelated +// reconnect or switch repaired it. +// +// Both doors go through the prepare/publish seam (prepareGatewayFor*, which +// dial without publishing and return the activation thunk), so these mocks +// hand back a spy thunk instead of activating on call. + +// Distinct gateway identities so a listener can tell WHICH backend it was +// handed. A bare vi.fn() thunk never touches $gateway, which would let an +// out-of-order publication pass unnoticed. +const INITIAL_GATEWAY = { id: 'live-socket' } +const AGENT_GATEWAY = { id: 'agent-socket' } +const PROFILE_GATEWAY = { id: 'profile-socket' } + +const activateAgent = vi.fn(() => { + $gateway.set(AGENT_GATEWAY) + + return true +}) + +const activateProfile = vi.fn(() => { + $gateway.set(PROFILE_GATEWAY) + + return true +}) + +// Annotated with the SEAM's thunk types, not the spies' own. Inferred, the +// resolved type is the MockInstance itself, and a test can no longer hand back +// a plain `() => false` to stand in for a disposed entry. +const prepareGatewayForAgent = vi.fn( + async (_connectionId: null | string, _profile: string): Promise<() => boolean> => activateAgent +) -const ensureGatewayForAgent = vi.fn(async (_connectionId: null | string, _profile: string) => true) -const ensureGatewayForProfile = vi.fn(async (_profile: string) => undefined) +const prepareGatewayForProfile = vi.fn(async (_profile: string): Promise<() => boolean> => activateProfile) const openGatewayForProfile = vi.fn(async (_profile: string) => undefined) -const $gateway = atom({ id: 'live-socket' }) +const $gateway = atom(INITIAL_GATEWAY) const resetStarmapGraph = vi.fn() vi.mock('@/store/gateway', () => ({ $gateway, - ensureGatewayForAgent, - ensureGatewayForProfile, - openGatewayForProfile + openGatewayForProfile, + prepareGatewayForAgent, + prepareGatewayForProfile })) vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), @@ -60,9 +98,13 @@ function deferred(): { promise: Promise; resolve: () => void } { beforeEach(() => { getConnection.mockReset() getConnectionFor.mockReset() - ensureGatewayForAgent.mockClear() - ensureGatewayForProfile.mockClear() - $gateway.set({ id: 'live-socket' }) + prepareGatewayForAgent.mockReset() + prepareGatewayForAgent.mockResolvedValue(activateAgent) + prepareGatewayForProfile.mockReset() + prepareGatewayForProfile.mockResolvedValue(activateProfile) + activateAgent.mockClear() + activateProfile.mockClear() + $gateway.set(INITIAL_GATEWAY) $activeGatewayProfile.set('default') $connection.set(localConn()) vi.stubGlobal('window', { hermesDesktop: { getConnection, getConnectionFor } }) @@ -81,31 +123,83 @@ describe('ensureGatewayAgent → $connection / $activeGatewayProfile sync', () = await ensureGatewayAgent('homelab', 'research') - expect(ensureGatewayForAgent).toHaveBeenCalledWith('homelab', 'research') + expect(prepareGatewayForAgent).toHaveBeenCalledWith('homelab', 'research') + expect(activateAgent).toHaveBeenCalledTimes(1) expect(getConnectionFor).toHaveBeenCalledWith({ connectionId: 'homelab', profile: 'research' }) expect($activeGatewayProfile.get()).toBe('research') expect($connection.get()?.mode).toBe('remote') expect($connection.get()?.profile).toBe('research') }) - it('leaves the prior connection intact when the descriptor fetch fails', async () => { + it('fails the switch closed when the descriptor lookup rejects', async () => { + // Previously this path swallowed the rejection and published anyway, which + // left $gateway and $activeGatewayProfile on the NEW backend while + // $connection still described the old one. Unlike the pending-descriptor + // race below, that state did not close on its own: it survived until some + // later reconnect or switch happened to repair it. getConnectionFor.mockRejectedValue(new Error('source unreachable')) - await ensureGatewayAgent('homelab', 'research') + await expect(ensureGatewayAgent('homelab', 'research')).rejects.toThrow('source unreachable') - expect($activeGatewayProfile.get()).toBe('research') - // Best-effort: boot/reconnect resyncs later; we must not null it out here. + // Nothing published: all three still describe the previous backend, and the + // caller can retry the switch. + expect(activateAgent).not.toHaveBeenCalled() + expect($gateway.get()).toBe(INITIAL_GATEWAY) + expect($activeGatewayProfile.get()).toBe('default') expect($connection.get()?.mode).toBe('local') + expect($connection.get()?.profile).toBe('default') }) it('does not republish a registry identity invalidated during activation', async () => { - ensureGatewayForAgent.mockResolvedValueOnce(false) + // The thunk reports false: the entry was disposed (source edited/removed) + // between dial and publish. Nothing may publish, $gateway included. + prepareGatewayForAgent.mockResolvedValueOnce(() => false) await ensureGatewayAgent('removed-source', 'research') expect($activeGatewayProfile.get()).toBe('default') expect($connection.get()?.mode).toBe('local') - expect(getConnectionFor).not.toHaveBeenCalled() + expect($gateway.get()).toBe(INITIAL_GATEWAY) + // The descriptor lookup DOES run: it is issued concurrently with the dial + // so both can be resolved before anything is published, which is the whole + // point of the seam. Resolving it lazily (only after the thunk reports a + // live entry) would put an await between the identity check and the + // publication and reopen the gap. The cost is one redundant read-only + // lookup in the rare disposed-entry case; the invariant that matters - + // nothing is PUBLISHED - is asserted above. + expect(getConnectionFor).toHaveBeenCalledTimes(1) + }) + + it('never shows a $gateway listener the new backend beside stale companions', async () => { + // The assertion the earlier tests could not make. A spy thunk that never + // touches $gateway proves only that it was CALLED at the right moment; + // it cannot prove that the three public stores become visible together. + // Nanostores drains listeners synchronously on every .set(), so without + // batch() a $gateway listener runs between the writes and reads the new + // gateway next to the previous profile and descriptor. + getConnectionFor.mockResolvedValue(agentConn()) + const seen: { connection?: string; gateway: unknown; profile: string }[] = [] + + const stop = $gateway.listen(gateway => { + seen.push({ + connection: $connection.get()?.profile, + gateway, + profile: $activeGatewayProfile.get() + }) + }) + + try { + await ensureGatewayAgent('homelab', 'research') + } finally { + stop() + } + + expect(seen).toHaveLength(1) + // When the listener sees the agent's gateway, the profile pointer and the + // descriptor must ALREADY identify that same backend. + expect(seen[0].gateway).toBe(AGENT_GATEWAY) + expect(seen[0].profile).toBe('research') + expect(seen[0].connection).toBe('research') }) it('falls through to the profile path for a null connectionId', async () => { @@ -113,8 +207,8 @@ describe('ensureGatewayAgent → $connection / $activeGatewayProfile sync', () = await ensureGatewayAgent(null, 'research') - expect(ensureGatewayForProfile).toHaveBeenCalledWith('research') - expect(ensureGatewayForAgent).not.toHaveBeenCalled() + expect(prepareGatewayForProfile).toHaveBeenCalledWith('research') + expect(prepareGatewayForAgent).not.toHaveBeenCalled() expect(getConnectionFor).not.toHaveBeenCalled() }) @@ -123,10 +217,90 @@ describe('ensureGatewayAgent → $connection / $activeGatewayProfile sync', () = await ensureGatewayAgent('local', 'research') - expect(ensureGatewayForAgent).toHaveBeenCalledWith('local', 'research') - expect(ensureGatewayForProfile).not.toHaveBeenCalled() + expect(prepareGatewayForAgent).toHaveBeenCalledWith('local', 'research') + expect(prepareGatewayForProfile).not.toHaveBeenCalled() expect(getConnectionFor).toHaveBeenCalledWith({ connectionId: 'local', profile: 'research' }) }) + + it('never publishes the agent gateway before its connection descriptor', async () => { + // The same mixed-state window the profile path closes, through the door + // added for the SDK's ensureAgent. A slow getConnectionFor must not leave + // $gateway/$activeGatewayProfile on the agent's backend while $connection + // still describes the previous one — anything requesting in that window + // announces the WRONG mode to the new backend. + let resolveDescriptor: (conn: HermesConnection) => void = () => undefined + getConnectionFor.mockReturnValue( + new Promise(resolve => { + resolveDescriptor = resolve + }) + ) + + const switching = ensureGatewayAgent('homelab', 'research') + // Let the socket-dial half settle; the descriptor is still pending. + await Promise.resolve() + await Promise.resolve() + + expect(activateAgent).not.toHaveBeenCalled() + expect($gateway.get()).toBe(INITIAL_GATEWAY) + expect($activeGatewayProfile.get()).toBe('default') + expect($connection.get()?.mode).toBe('local') + + resolveDescriptor(agentConn()) + await switching + + expect(activateAgent).toHaveBeenCalledTimes(1) + expect($activeGatewayProfile.get()).toBe('research') + expect($connection.get()?.mode).toBe('remote') + }) +}) + +describe('ensureGatewayProfile publishes under the same activation guard', () => { + it('publishes nothing when the profile activation is superseded', async () => { + // The profile-door mirror of "does not republish a registry identity + // invalidated during activation". applyActive() returns false when its + // captured epoch has been superseded — a newer switch or a teardown + // landed while this preparation was awaiting its route or socket. + // + // Discarding that boolean does not produce a torn publication; batch() + // makes the writes observer-atomic either way. It produces something + // subtler and worse: ONE complete, internally inconsistent tuple, the + // CURRENT gateway paired with the stale target's profile pointer and + // descriptor. Atomicity cannot make a rejected activation correct, so the + // caller has to decline to publish at all. + getConnection.mockResolvedValue(localConn({ profile: 'worker' })) + prepareGatewayForProfile.mockResolvedValueOnce(() => false) + + const seen: unknown[] = [] + const stop = $gateway.listen(gateway => seen.push(gateway)) + + try { + await ensureGatewayProfile('worker') + } finally { + stop() + } + + // All three still describe the complete route that was already active. + expect($gateway.get()).toBe(INITIAL_GATEWAY) + expect($activeGatewayProfile.get()).toBe('default') + expect($connection.get()?.profile).toBe('default') + expect($connection.get()?.mode).toBe('local') + // And no subscriber was handed a tuple to disagree about. + expect(seen).toEqual([]) + }) + + it('publishes the companions when the profile activation is accepted', async () => { + // The other half: the guard must not swallow a legitimate switch. Without + // this, returning a constant false from every thunk would pass the test + // above and break the feature. + getConnection.mockResolvedValue(localConn({ profile: 'worker' })) + + await ensureGatewayProfile('worker') + + expect(activateProfile).toHaveBeenCalledTimes(1) + expect($gateway.get()).toBe(PROFILE_GATEWAY) + expect($activeGatewayProfile.get()).toBe('worker') + expect($connection.get()?.profile).toBe('worker') + }) }) describe('ensureGatewayAgent shares the gatewaySwitch mutex with profile switches', () => { @@ -134,14 +308,16 @@ describe('ensureGatewayAgent shares the gatewaySwitch mutex with profile switche const profileGate = deferred() const order: string[] = [] - ensureGatewayForProfile.mockImplementation(async (profile: string) => { + prepareGatewayForProfile.mockImplementation(async (profile: string) => { order.push(`profile:${profile}`) await profileGate.promise + + return activateProfile }) - ensureGatewayForAgent.mockImplementation(async (_connectionId, profile) => { + prepareGatewayForAgent.mockImplementation(async (_connectionId, profile) => { order.push(`agent:${profile}`) - return true + return activateAgent }) getConnection.mockResolvedValue(localConn({ profile: 'worker' })) getConnectionFor.mockResolvedValue(agentConn()) @@ -170,14 +346,16 @@ describe('ensureGatewayAgent shares the gatewaySwitch mutex with profile switche const agentGate = deferred() const order: string[] = [] - ensureGatewayForAgent.mockImplementation(async (_connectionId, profile) => { + prepareGatewayForAgent.mockImplementation(async (_connectionId, profile) => { order.push(`agent:${profile}`) await agentGate.promise - return true + return activateAgent }) - ensureGatewayForProfile.mockImplementation(async (profile: string) => { + prepareGatewayForProfile.mockImplementation(async (profile: string) => { order.push(`profile:${profile}`) + + return activateProfile }) getConnection.mockResolvedValue(localConn({ profile: 'worker' })) getConnectionFor.mockResolvedValue(agentConn()) diff --git a/apps/desktop/src/store/profile.test.ts b/apps/desktop/src/store/profile.test.ts index c2e7cf36c02b..7ac9dbb6bc8a 100644 --- a/apps/desktop/src/store/profile.test.ts +++ b/apps/desktop/src/store/profile.test.ts @@ -6,13 +6,25 @@ import type { ProfileInfo } from '@/types/hermes' // Keep profile.ts's side-effecting imports inert: the gateway socket layer and // the REST query client must not run for real in a unit test. +// Returns true: both prepare seams hand back a thunk reporting whether the +// activation was ACCEPTED, and a caller publishes its companion state only on +// true. A bare vi.fn() returns undefined, which now reads as "superseded" and +// would silently suppress every publication these tests assert on. +const activateGateway = vi.fn(() => true) const ensureGatewayForProfile = vi.fn(async () => undefined) -const ensureGatewayForAgent = vi.fn(async () => undefined) +const prepareGatewayForAgent = vi.fn(async (_connectionId: null | string, _profile: string) => activateGateway) +const prepareGatewayForProfile = vi.fn(async (_profile: string) => activateGateway) const openGatewayForProfile = vi.fn(async (_profile: string) => undefined) const $gateway = atom({ id: 'live-socket' }) const resetStarmapGraph = vi.fn() -vi.mock('@/store/gateway', () => ({ $gateway, ensureGatewayForAgent, ensureGatewayForProfile, openGatewayForProfile })) +vi.mock('@/store/gateway', () => ({ + $gateway, + ensureGatewayForProfile, + openGatewayForProfile, + prepareGatewayForAgent, + prepareGatewayForProfile +})) vi.mock('@/hermes', () => ({ getProfiles: vi.fn(async () => ({ profiles: [] })), setApiRequestProfile: vi.fn() @@ -53,7 +65,9 @@ const getConnection = vi.fn<(profile?: string | null) => Promise { getConnection.mockReset() + activateGateway.mockClear() ensureGatewayForProfile.mockClear() + prepareGatewayForProfile.mockClear() openGatewayForProfile.mockClear() $gateway.set({ id: 'live-socket' }) $activeGatewayProfile.set('default') @@ -79,7 +93,8 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { await ensureGatewayProfile('vps-remote') - expect(ensureGatewayForProfile).toHaveBeenCalledWith('vps-remote') + expect(prepareGatewayForProfile).toHaveBeenCalledWith('vps-remote') + expect(activateGateway).toHaveBeenCalledTimes(1) expect(getConnection).toHaveBeenCalledWith('vps-remote') expect($connection.get()?.mode).toBe('remote') expect($connection.get()?.profile).toBe('vps-remote') @@ -96,13 +111,49 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { expect($connection.get()?.mode).toBe('local') }) - it('leaves the prior connection intact when the descriptor fetch fails', async () => { + it('fails as a unit when the descriptor fetch fails — no mixed state', async () => { + // Previously the gateway was activated and $activeGatewayProfile set even + // when the descriptor lookup failed, leaving $gateway on the new backend + // while $connection kept describing the old one for the rest of the + // session. Now nothing is published: every atom still consistently + // describes the previous profile and the user can retry. getConnection.mockRejectedValue(new Error('backend unreachable')) await ensureGatewayProfile('vps-remote') - // Best-effort: boot/reconnect resyncs later; we must not null it out here. + expect(activateGateway).not.toHaveBeenCalled() + expect($activeGatewayProfile.get()).toBe('default') + expect($connection.get()?.mode).toBe('local') + }) + + it('never publishes the new gateway before its connection descriptor', async () => { + // The exact mixed-state window from the follow-up review: a slow + // descriptor fetch must not leave $gateway/$activeGatewayProfile on the + // remote backend while $connection still says local. All three flip + // together only once the descriptor is in hand. + let resolveDescriptor: (conn: HermesConnection) => void = () => undefined + getConnection.mockReturnValue( + new Promise(resolve => { + resolveDescriptor = resolve + }) + ) + + const switching = ensureGatewayProfile('vps-remote') + // Let the socket-open half of the switch settle; the descriptor is still + // deliberately pending. + await Promise.resolve() + await Promise.resolve() + + expect(activateGateway).not.toHaveBeenCalled() + expect($activeGatewayProfile.get()).toBe('default') expect($connection.get()?.mode).toBe('local') + + resolveDescriptor(remoteConn()) + await switching + + expect(activateGateway).toHaveBeenCalledTimes(1) + expect($activeGatewayProfile.get()).toBe('vps-remote') + expect($connection.get()?.mode).toBe('remote') }) it('does not churn $connection when the target is already the active profile', async () => { @@ -112,7 +163,7 @@ describe('ensureGatewayProfile → $connection sync (#46651)', () => { await ensureGatewayProfile('vps-remote') expect(getConnection).not.toHaveBeenCalled() - expect(ensureGatewayForProfile).not.toHaveBeenCalled() + expect(prepareGatewayForProfile).not.toHaveBeenCalled() expect($connection.get()?.mode).toBe('remote') }) }) diff --git a/apps/desktop/src/store/profile.ts b/apps/desktop/src/store/profile.ts index 99fd3dd7054c..60bd3d44841c 100644 --- a/apps/desktop/src/store/profile.ts +++ b/apps/desktop/src/store/profile.ts @@ -1,5 +1,6 @@ -import { atom, computed } from 'nanostores' +import { atom, batch, computed } from 'nanostores' +import type { HermesConnection } from '@/global' import { getProfiles, setApiRequestProfile, STARTUP_REQUEST_TIMEOUT_MS } from '@/hermes' import { invalidateProfileScopedQueries } from '@/lib/query-client' import { @@ -12,7 +13,7 @@ import { storedStringRecord } from '@/lib/storage' import { invalidateCronModelImpactScopeState } from '@/store/cron-model-impact-scope' -import { $gateway, ensureGatewayForAgent, ensureGatewayForProfile, openGatewayForProfile } from '@/store/gateway' +import { $gateway, openGatewayForProfile, prepareGatewayForAgent, prepareGatewayForProfile } from '@/store/gateway' import { setConnection } from '@/store/session' import { resetStarmapGraph } from '@/store/starmap' import type { ProfileInfo } from '@/types/hermes' @@ -258,30 +259,24 @@ export function prewarmProfileBackend(name: string): void { let gatewaySwitch: Promise | null = null -// Keep the renderer's $connection (mode / baseUrl / profile) in lockstep with -// the profile the live gateway is now on. $connection seeds from the PRIMARY +// The target profile's connection descriptor (mode / baseUrl / …), fetched +// BEFORE activation so the switch can publish it in the same synchronous frame +// as the gateway and profile pointer. $connection seeds from the PRIMARY // (window) backend at boot and otherwise only refreshes on a sleep/wake -// reconnect — so activating a *background* profile left $connection describing -// the primary, with the wrong `mode` for everything that branches on -// local-vs-remote. Headline symptom: with a local primary and a remote pool -// profile active, image attachments went out via the path-based `image.attach` -// instead of `image.attach_bytes`, handing the remote gateway a client-only -// path it can't resolve ("image not found: C:\…"), while the /api/fs/* file -// browser and /api/media fetches targeted the wrong machine (#46651). -// Best-effort: a failed descriptor fetch leaves the prior connection intact for -// boot/reconnect to resync. -async function syncConnectionToActiveProfile(profile: string): Promise { +// reconnect — so activating a *background* profile without this left +// $connection describing the primary, with the wrong `mode` for everything +// that branches on local-vs-remote (#46651: path-based `image.attach` against +// a remote gateway, /api/fs/* and /api/media on the wrong machine). +// +// Null means "no desktop bridge" (plain browser) — there is no descriptor to +// sync then. A bridge REJECTION propagates: the caller aborts the whole switch +// rather than activating a backend whose descriptor (and thus mode) is +// unknown, which previously left $gateway on the new backend while +// $connection kept describing the old one for the rest of the session. +async function resolveConnectionForProfile(profile: string) { const getConnection = window.hermesDesktop?.getConnection - if (!getConnection) { - return - } - - try { - setConnection(await getConnection(profile)) - } catch { - // Leave the prior connection in place; boot/reconnect resyncs it later. - } + return getConnection ? getConnection(profile) : null } // Make `profile`'s backend the active gateway, lazily opening its socket if it @@ -320,14 +315,46 @@ export async function ensureGatewayProfile(profile: string | null | undefined): $gatewaySwapTarget.set(target) gatewaySwitch = (async () => { - // ensureGatewayForProfile opens (or reuses) the target's socket and points - // the active gateway at it — without closing the profile you came from. - await ensureGatewayForProfile(target) - $activeGatewayProfile.set(target) - // The active backend just changed; resync $connection so remote-aware - // paths (image.attach_bytes vs image.attach, /api/fs/*, /api/media) follow. - await syncConnectionToActiveProfile(target) - })() + // Resolve the target's connection descriptor and open (or reuse) its + // socket BEFORE anything is published — without closing the profile you + // came from. The gateway used to be activated (and the profile atom set) + // while the descriptor fetch was still in flight, so during that window + // $gateway already targeted the new backend while $connection still + // described the previous one — and any request or plugin mode-listener + // firing then announced the WRONG mode to the new backend. + const [connection, activate] = await Promise.all([ + resolveConnectionForProfile(target), + prepareGatewayForProfile(target) + ]) + + // ONE publication. batch() defers Nanostores' notifications to the end of + // the callback, so the active gateway, $activeGatewayProfile and + // $connection become visible together. Without it these are sequential + // .set() calls that each drain their listeners synchronously, and a + // $gateway listener runs while the other two still name the old backend. + batch(() => { + // A rejected activation publishes NOTHING, exactly like the agent path. + // applyActive() returns false when its captured epoch was superseded -- + // a newer switch (or a teardown) landed while this one was awaiting its + // route or socket. Publishing the companions anyway would leave the + // CURRENT gateway paired with the stale profile pointer and descriptor, + // and batch() cannot rescue that: it would make the mismatched tuple + // atomically observable rather than prevent it. + if (!activate()) { + return + } + + $activeGatewayProfile.set(target) + + if (connection) { + setConnection(connection) + } + }) + })().catch(() => { + // Descriptor lookup failed: the switch fails as a unit. Nothing was + // published, so every atom still consistently describes the previous + // profile; the user can retry the switch. + }) try { await gatewaySwitch @@ -339,25 +366,30 @@ export async function ensureGatewayProfile(profile: string | null | undefined): // Registry-aware sibling of syncConnectionToActiveProfile: a connection-scoped // agent's descriptor comes from getConnectionFor (its SOURCE connection), not -// getConnection (the local pool). Same best-effort contract. -async function syncConnectionToActiveAgent(connectionId: string, profile: string): Promise { +// getConnection (the local pool). +// Resolve only — publication is the caller's, so the descriptor can be in hand +// BEFORE the activation frame rather than an await after it. +// +// Null means "no desktop bridge" (plain browser) and nothing else, matching +// resolveConnectionForProfile. A bridge REJECTION propagates so the caller +// aborts the whole switch. Collapsing the two into null instead let a failed +// lookup publish the new gateway and profile while $connection kept describing +// the OLD backend, and unlike the pending-descriptor race that state did not +// close on its own: it survived until some later reconnect or switch happened +// to repair it, which is the same invariant this path exists to establish. +async function resolveConnectionForActiveAgent( + connectionId: string, + profile: string +): Promise { const getConnectionFor = window.hermesDesktop?.getConnectionFor - if (!getConnectionFor) { - return - } - - try { - setConnection(await getConnectionFor({ connectionId, profile })) - } catch { - // Leave the prior connection in place; boot/reconnect resyncs it later. - } + return getConnectionFor ? getConnectionFor({ connectionId, profile }) : null } // Activate a connection-scoped agent's gateway — the (connectionId, profile) // analogue of ensureGatewayProfile, and the door the SDK's ensureAgent goes -// through. Two invariants the raw store call (ensureGatewayForAgent) does not -// provide on its own: +// through. Three invariants the raw store call (ensureGatewayForAgent) does +// not provide on its own: // - Every activation moves $activeGatewayProfile and resyncs $connection, // exactly like the profile path — otherwise activating an ALREADY-OPEN // registry agent left both describing the previous backend, routing @@ -366,6 +398,10 @@ async function syncConnectionToActiveAgent(connectionId: string, profile: string // - Activations share the gatewaySwitch mutex with profile switches, so a // rapid agent↔profile (or agent↔agent) interleave can't finish out of // order and leave the EARLIER setActive() as the last write. +// - The gateway, the profile pointer and the connection descriptor publish in +// ONE synchronous frame, via the same prepare/publish seam the profile path +// uses, so no subscriber sees the new backend paired with the old +// descriptor. // Only a null connectionId falls through to the legacy profile path. Explicit // `local` is a registry identity and must use the genuinely-local route. export async function ensureGatewayAgent(connectionId: null | string, profile: string): Promise { @@ -383,16 +419,40 @@ export async function ensureGatewayAgent(connectionId: null | string, profile: s $gatewaySwapTarget.set(target) gatewaySwitch = (async () => { - const activated = await ensureGatewayForAgent(connection, target) - - if (!activated) { - return - } - - $activeGatewayProfile.set(target) - // The active backend just changed; resync $connection so remote-aware - // paths (image.attach_bytes vs image.attach, /api/fs/*, /api/media) follow. - await syncConnectionToActiveAgent(connection, target) + // Dial the agent's socket and resolve its descriptor without publishing + // either, exactly like the profile path above. Activating first and then + // awaiting the descriptor left $gateway on the new backend while + // $connection still described the old one, so anything requesting during + // that window announced the WRONG mode to the new backend. + const [descriptor, activate] = await Promise.all([ + resolveConnectionForActiveAgent(connection, target), + prepareGatewayForAgent(connection, target) + ]) + + // ONE publication. batch() defers Nanostores' notifications to the end of + // the callback, so a $gateway listener cannot run while the profile + // pointer and the connection descriptor still name the previous backend. + // Without it these are three sequential .set() calls, each draining its + // listeners synchronously, and the first listener observes exactly the + // mismatch this seam exists to prevent. + batch(() => { + // A disposed target (source edited/removed mid-dial) publishes nothing + // at all, rather than moving the profile pointer to a backend that no + // longer has a socket. + if (!activate()) { + return + } + + $activeGatewayProfile.set(target) + + // Remote-aware paths (image.attach_bytes vs image.attach, /api/fs/*, + // /api/media) follow $connection. Null here is only the no-bridge case, + // so keeping the previous descriptor is correct; a failed lookup + // rejected above and never reached this frame. + if (descriptor) { + setConnection(descriptor) + } + }) })() try { diff --git a/gateway/session_context.py b/gateway/session_context.py index 7a2c53ab3a95..9d63fc9c7cb2 100644 --- a/gateway/session_context.py +++ b/gateway/session_context.py @@ -135,6 +135,35 @@ def session_context_engaged() -> bool: _CRON_AUTO_DELIVER_CHAT_ID: ContextVar = ContextVar("HERMES_CRON_AUTO_DELIVER_CHAT_ID", default=_UNSET) _CRON_AUTO_DELIVER_THREAD_ID: ContextVar = ContextVar("HERMES_CRON_AUTO_DELIVER_THREAD_ID", default=_UNSET) +# The env var name used for the write-only subprocess stamp (see +# tools/environments/local.py). Never read back as configuration. +DESKTOP_CONNECTION_MODE_ENV = "HERMES_DESKTOP_CONNECTION_MODE" + +# The resolved Desktop connection mode for this turn: 'local' when the Desktop +# app drives its own local backend, 'remote' when it drives an SSH/URL/cloud +# backend on another machine. ``None`` for every non-Desktop surface (CLI, TUI, +# messaging, cron, API server) and for a Desktop client too old to announce it. +# +# Deliberately NOT a member of ``_VAR_MAP``: mapped vars are readable through +# ``get_session_env``, which falls back to ``os.environ``. That fallback is what +# makes a var user-configurable, and this value must be authoritative — a user +# who exports HERMES_DESKTOP_CONNECTION_MODE=local must not be able to convince +# a skill that a gateway-side file is sitting on their Desktop machine. The only +# writer is :func:`set_desktop_connection_mode`, driven by the connection +# descriptor the Desktop shell already resolves (``getConnection().mode``). +# +# Read it with :func:`desktop_connection_mode`. The subprocess bridge in +# ``tools/environments/local.py`` stamps it onto child environments write-only +# (always overwritten, stripped when unset) so skills and their helper scripts +# can branch on it without it ever becoming an input. +_DESKTOP_CONNECTION_MODE: ContextVar = ContextVar(DESKTOP_CONNECTION_MODE_ENV, default=_UNSET) + +# Saved-config connection modes that resolve to a backend on another machine. +# The Desktop descriptor already collapses these to 'remote', but the RPC edge +# accepts them so a client that forwards its raw saved mode still lands on a +# correct answer rather than an unavailable one. +_REMOTE_LIKE_CONNECTION_MODES = frozenset({"cloud", "remote", "ssh", "url"}) + _VAR_MAP = { "HERMES_SESSION_PLATFORM": _SESSION_PLATFORM, "HERMES_SESSION_SOURCE": _SESSION_SOURCE, @@ -158,6 +187,56 @@ def session_context_engaged() -> bool: } +def normalize_desktop_connection_mode(value: Any) -> str | None: + """Coerce a client-announced connection mode to ``'local'``/``'remote'``/``None``. + + ``'local'`` stays local; every remote-shaped saved mode (``remote``, + ``cloud``, ``ssh``, ``url``) resolves to ``'remote'``. Anything else — + empty, ``None``, a typo, a hostile string — resolves to ``None`` (mode + unknown), because a wrong answer here is worse than no answer: an extension + that believes a gateway-side path is Desktop-local will hand the user a link + to a file that isn't on their machine. + """ + text = str(value or "").strip().lower() + if text == "local": + return "local" + if text in _REMOTE_LIKE_CONNECTION_MODES: + return "remote" + return None + + +def set_desktop_connection_mode(value: Any) -> None: + """Bind the resolved Desktop connection mode for this task. + + Called by the Desktop-facing RPC edge with the mode the Desktop shell + resolved via ``window.hermesDesktop.getConnection()``. Non-Desktop surfaces + never call this, so :func:`desktop_connection_mode` keeps returning ``None`` + for them. + """ + _DESKTOP_CONNECTION_MODE.set(normalize_desktop_connection_mode(value)) + + +def desktop_connection_mode() -> str | None: + """The resolved Desktop connection mode, or ``None`` when not applicable. + + ``'local'`` — the Desktop app is driving its own local backend, so a + gateway-side path is already a path on the user's machine. + ``'remote'`` — the Desktop app is driving an SSH/URL/cloud backend, so a + gateway-side path must be transferred before the Desktop can + open it. + ``None`` — not a Desktop session (CLI, TUI, messaging, cron, API + server), or a Desktop client that didn't announce a mode. + + This is the only supported read path on the Python side. It reports the + connection *shape* and nothing else — no base URL, host, token, SSH key, or + auth mode ever passes through here. + """ + value = _DESKTOP_CONNECTION_MODE.get() + if value is _UNSET: + return None + return value + + def set_current_session_id(session_id: str) -> None: """Synchronize ``HERMES_SESSION_ID`` across ContextVar and ``os.environ``. @@ -232,6 +311,7 @@ def set_session_vars( async_delivery: bool = True, ui_session_id: str = "", cron_session: Any = _UNSET, + desktop_connection_mode: Any = None, ) -> list: """Set all session context variables and return reset tokens. @@ -251,6 +331,11 @@ def set_session_vars( ``cron_session`` is tri-state: ``_UNSET`` preserves legacy ``os.environ["HERMES_CRON_SESSION"]`` fallback, ``"1"`` marks a cron job, and ``""`` explicitly marks a non-cron session while masking leaked env. + + ``desktop_connection_mode`` is the Desktop shell's resolved connection mode + (see :func:`desktop_connection_mode`). Every caller that isn't the + Desktop-facing RPC edge leaves it ``None``, which is exactly the "not a + Desktop session" answer non-Desktop surfaces should report. """ # Mark the session-context machinery engaged for this process. The # subprocess-env bridge uses this to switch from "os.environ fallback" to @@ -275,6 +360,7 @@ def set_session_vars( _SESSION_PROFILE.set(profile), _CRON_SESSION.set(cron_session), _SESSION_ASYNC_DELIVERY.set(bool(async_delivery)), + _DESKTOP_CONNECTION_MODE.set(normalize_desktop_connection_mode(desktop_connection_mode)), ] try: from agent.runtime_cwd import set_session_cwd @@ -320,6 +406,11 @@ def clear_session_vars(tokens: list) -> None: # behavior (CLI / unaware paths), not be mistaken for an opted-out # stateless adapter. _SESSION_ASYNC_DELIVERY.set(_UNSET) + # A finished handler is no longer a Desktop turn. Setting None (rather than + # _UNSET) is the same "explicitly cleared" posture the mapped vars take — + # both read as "no Desktop connection", and there is no os.environ fallback + # behind this var for the distinction to matter. + _DESKTOP_CONNECTION_MODE.set(None) try: from agent.runtime_cwd import clear_session_cwd @@ -368,6 +459,11 @@ def reset_session_vars() -> None: # same inheritance-leak reason as the mapped vars above — see clear_session_vars, # which resets this var on the handler-exit path for the symmetric concern. _SESSION_ASYNC_DELIVERY.set(_UNSET) + # Same leak concern, sharper consequence: a task spawned from a context where + # a concurrent Desktop turn had bound 'local' would otherwise inherit it, and + # a skill running for a *remote* Desktop client (or for the CLI) would be told + # its gateway-side files are already on the user's machine. + _DESKTOP_CONNECTION_MODE.set(_UNSET) try: from agent.runtime_cwd import clear_session_cwd diff --git a/tests/agent/test_skill_preprocessing_env.py b/tests/agent/test_skill_preprocessing_env.py new file mode 100644 index 000000000000..026f3630018d --- /dev/null +++ b/tests/agent/test_skill_preprocessing_env.py @@ -0,0 +1,134 @@ +"""SKILL.md inline-shell snippets must use the central subprocess env factory. + +``!`cmd``` expansion used to call ``subprocess.run()`` with no ``env`` at all, +so — unlike every terminal/tool spawn — the snippet inherited the raw process +environment: no session-context stamps, no Desktop connection-mode stamp, and +no scrub of a ``HERMES_DESKTOP_CONNECTION_MODE`` value inherited from the +user's shell (#82187 follow-up review, item 2). +""" + +import os +import subprocess +from types import SimpleNamespace + +import pytest + +import gateway.session_context as sc +from gateway.session_context import ( + DESKTOP_CONNECTION_MODE_ENV as MODE_ENV, + _VAR_MAP, + set_desktop_connection_mode, + set_session_vars, +) + +from agent.skill_preprocessing import run_inline_shell + + +@pytest.fixture(autouse=True) +def _isolate_session_context(): + """Clean ContextVar + os.environ + engaged-latch slate per test, restored.""" + tracked = list(_VAR_MAP.keys()) + [MODE_ENV] + saved_env = {k: os.environ.get(k) for k in tracked} + saved_ctx = {name: var.get() for name, var in _VAR_MAP.items()} + saved_mode = sc._DESKTOP_CONNECTION_MODE.get() + saved_engaged = sc._session_context_engaged + for var in _VAR_MAP.values(): + var.set(sc._UNSET) + sc._DESKTOP_CONNECTION_MODE.set(sc._UNSET) + sc._session_context_engaged = False + try: + yield + finally: + for var, val in zip(_VAR_MAP.values(), saved_ctx.values()): + var.set(val) + sc._DESKTOP_CONNECTION_MODE.set(saved_mode) + sc._session_context_engaged = saved_engaged + for k, v in saved_env.items(): + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v + + +def _capture_spawn_env(monkeypatch) -> dict: + """Run one inline snippet with subprocess.run stubbed; return its env kwarg.""" + captured = {} + + def _fake_run(argv, **kwargs): + captured.update({"argv": argv, "env": kwargs.get("env")}) + return SimpleNamespace(stdout="ok\n", stderr="", returncode=0) + + monkeypatch.setattr(subprocess, "run", _fake_run) + assert run_inline_shell("echo hi", None, timeout=5) == "ok" + return captured + + +def test_inline_shell_passes_a_factory_built_env(monkeypatch): + """The spawn must supply an explicit env, not inherit the raw process one.""" + captured = _capture_spawn_env(monkeypatch) + assert captured["env"] is not None + + +def test_live_mode_is_stamped_for_the_snippet(monkeypatch): + set_session_vars(session_key="k", source="desktop") + set_desktop_connection_mode("remote") + captured = _capture_spawn_env(monkeypatch) + assert captured["env"][MODE_ENV] == "remote" + + +def test_live_mode_overrides_an_ambient_shell_value(monkeypatch): + """A live remote ContextVar wins over HERMES_DESKTOP_CONNECTION_MODE=local + inherited from the user's shell.""" + monkeypatch.setenv(MODE_ENV, "local") + set_session_vars(session_key="k", source="desktop") + set_desktop_connection_mode("remote") + captured = _capture_spawn_env(monkeypatch) + assert captured["env"][MODE_ENV] == "remote" + + +def test_ambient_value_is_stripped_when_no_mode_is_bound(monkeypatch): + """Engaged session context with no bound mode: the inherited shell value is + stripped rather than honored (write-only stamp contract).""" + monkeypatch.setenv(MODE_ENV, "remote") + set_session_vars(session_key="k", source="tui") + captured = _capture_spawn_env(monkeypatch) + assert MODE_ENV not in captured["env"] + + +def test_snippet_is_not_spawned_when_the_env_factory_fails(monkeypatch): + """A sanitizer that cannot be built must fail CLOSED, not fall back to env=None. + + ``subprocess.run(env=None)`` inherits the raw parent environment, so the + old ``except: _run_env = None`` fallback handed the snippet an ambient + ``HERMES_DESKTOP_CONNECTION_MODE=local`` verbatim — reopening exactly the + spoofing path the scrub exists to close, and only on the error branch where + nobody would look for it (#82187 follow-up review, item 3). + """ + monkeypatch.setenv(MODE_ENV, "local") + set_session_vars(session_key="k", source="desktop") + set_desktop_connection_mode("remote") + + import tools.environments.local as local_env + + def _boom(): + raise RuntimeError("sanitizer unavailable") + + monkeypatch.setattr(local_env, "build_subprocess_env", _boom) + + spawned = [] + + def _record(argv, **kwargs): + spawned.append({"argv": argv, "env": kwargs.get("env")}) + return SimpleNamespace(stdout="ok\n", stderr="", returncode=0) + + monkeypatch.setattr(subprocess, "run", _record) + + result = run_inline_shell("echo hi", None, timeout=5) + + # The strongest assertion available: the child never existed, so there is + # no environment for it to have inherited. + assert spawned == [], ( + "the snippet was spawned with a non-sanitized environment: " + f"{spawned[0]['env'] if spawned else None}" + ) + assert "inline-shell error" in result diff --git a/tests/gateway/test_desktop_connection_mode.py b/tests/gateway/test_desktop_connection_mode.py new file mode 100644 index 000000000000..90d9a0671cb8 --- /dev/null +++ b/tests/gateway/test_desktop_connection_mode.py @@ -0,0 +1,143 @@ +"""The resolved Desktop connection mode exposed to skills, MCP, and plugins. + +See NousResearch/hermes-agent#82140. The value answers one question — is the +gateway's filesystem the same machine the user is looking at? — and must answer +it authoritatively, so the tests below pin three properties: + +1. Only ``'local'``, ``'remote'``, or ``None`` ever come out. +2. A user-set ``HERMES_DESKTOP_CONNECTION_MODE`` in the environment is NOT a + source of truth (issue acceptance criterion: no user-configurable env var). +3. The value is task-local, so a concurrent local-Desktop turn can't convince a + remote-Desktop turn (or the CLI) that gateway files are already local. +""" + +import asyncio + +import pytest + +from gateway.session_context import ( + _DESKTOP_CONNECTION_MODE, + _UNSET, + _VAR_MAP, + DESKTOP_CONNECTION_MODE_ENV, + clear_session_vars, + desktop_connection_mode, + get_session_env, + normalize_desktop_connection_mode, + reset_session_vars, + set_desktop_connection_mode, + set_session_vars, +) + + +@pytest.fixture(autouse=True) +def _reset_contextvars(): + """Tests share one thread context; restore the "never bound" sentinel.""" + yield + for var in _VAR_MAP.values(): + var.set(_UNSET) + _DESKTOP_CONNECTION_MODE.set(_UNSET) + + +class TestNormalize: + @pytest.mark.parametrize("value", ["local", "LOCAL", " Local "]) + def test_local_variants_resolve_local(self, value): + assert normalize_desktop_connection_mode(value) == "local" + + @pytest.mark.parametrize("value", ["remote", "cloud", "ssh", "url", "SSH"]) + def test_remote_like_saved_modes_resolve_remote(self, value): + """A client forwarding its raw saved mode still gets a usable answer.""" + assert normalize_desktop_connection_mode(value) == "remote" + + @pytest.mark.parametrize("value", ["", None, " ", "lokal", "true", 0, [], {"mode": "local"}]) + def test_unknown_values_resolve_none_not_a_guess(self, value): + """Unknown must be None: a wrong 'local' sends the user to a missing file.""" + assert normalize_desktop_connection_mode(value) is None + + +class TestAccessor: + def test_unbound_session_reports_none(self): + assert desktop_connection_mode() is None + + def test_bound_mode_is_readable(self): + set_desktop_connection_mode("remote") + assert desktop_connection_mode() == "remote" + + def test_bound_garbage_reports_none(self): + set_desktop_connection_mode("something-else") + assert desktop_connection_mode() is None + + +class TestNotUserConfigurable: + """The acceptance criterion: no user-configurable HERMES_* env var.""" + + def test_env_var_is_not_a_source_of_truth(self, monkeypatch): + monkeypatch.setenv(DESKTOP_CONNECTION_MODE_ENV, "local") + assert desktop_connection_mode() is None + + def test_env_var_cannot_override_a_bound_remote_session(self, monkeypatch): + monkeypatch.setenv(DESKTOP_CONNECTION_MODE_ENV, "local") + set_desktop_connection_mode("remote") + assert desktop_connection_mode() == "remote" + + def test_not_reachable_through_get_session_env(self, monkeypatch): + """Mapped vars fall back to os.environ; this one must not be mapped.""" + assert DESKTOP_CONNECTION_MODE_ENV not in _VAR_MAP + monkeypatch.setenv(DESKTOP_CONNECTION_MODE_ENV, "local") + assert get_session_env(DESKTOP_CONNECTION_MODE_ENV, "") == "local" # raw env read + assert desktop_connection_mode() is None # the supported API is unmoved + + +class TestSessionLifecycle: + def test_set_session_vars_binds_the_mode(self): + set_session_vars(source="desktop", desktop_connection_mode="remote") + assert desktop_connection_mode() == "remote" + + def test_set_session_vars_defaults_to_none_for_non_desktop_surfaces(self): + set_session_vars(platform="telegram", chat_id="-100") + assert desktop_connection_mode() is None + + def test_clear_session_vars_drops_the_mode(self): + tokens = set_session_vars(source="desktop", desktop_connection_mode="local") + clear_session_vars(tokens) + assert desktop_connection_mode() is None + + def test_reset_session_vars_drops_an_inherited_mode(self): + """A freshly-spawned task must not inherit a sibling turn's mode.""" + set_desktop_connection_mode("local") + reset_session_vars() + assert desktop_connection_mode() is None + + def test_rebinding_reflects_a_connection_switch(self): + """Switching the active Desktop connection re-announces; last write wins.""" + set_session_vars(source="desktop", desktop_connection_mode="local") + assert desktop_connection_mode() == "local" + set_session_vars(source="desktop", desktop_connection_mode="remote") + assert desktop_connection_mode() == "remote" + + +def test_mode_is_task_local_across_concurrent_sessions(): + """Two concurrent Desktop clients on one gateway keep their own answers.""" + + async def scenario(): + seen: dict[str, str | None] = {} + started = asyncio.Event() + + async def turn(name: str, mode: str, wait_for_sibling: bool) -> None: + set_session_vars(source="desktop", desktop_connection_mode=mode) + if wait_for_sibling: + started.set() + else: + await started.wait() + # Yield so the sibling task definitely interleaves before we read. + await asyncio.sleep(0) + seen[name] = desktop_connection_mode() + + await asyncio.gather( + turn("local-client", "local", wait_for_sibling=True), + turn("remote-client", "remote", wait_for_sibling=False), + ) + return seen + + seen = asyncio.run(scenario()) + assert seen == {"local-client": "local", "remote-client": "remote"} diff --git a/tests/tools/test_desktop_connection_mode_env.py b/tests/tools/test_desktop_connection_mode_env.py new file mode 100644 index 000000000000..fedfd3151ce1 --- /dev/null +++ b/tests/tools/test_desktop_connection_mode_env.py @@ -0,0 +1,115 @@ +"""The skill-facing read path for the Desktop connection mode (#82140). + +Skills branch on ``HERMES_DESKTOP_CONNECTION_MODE`` from their helper scripts to +decide whether a gateway-side artifact is already on the user's machine or has +to be transferred first. The subprocess bridge stamps it — **write-only**, on +every spawn — which is precisely what keeps the issue's "no user-configurable +``HERMES_*`` env var" criterion true: a value inherited from the user's shell is +stripped rather than honored, and the contextvar remains the only source. +""" + +import os + +import pytest + +import gateway.session_context as sc +from gateway.session_context import ( + DESKTOP_CONNECTION_MODE_ENV as MODE_ENV, + _VAR_MAP, + clear_session_vars, + set_desktop_connection_mode, + set_session_vars, +) +from tools.environments.local import _make_run_env + +SESSION_VARS = list(_VAR_MAP.keys()) + + +@pytest.fixture(autouse=True) +def _isolate_session_context(): + """Clean ContextVar + os.environ + engaged-latch slate per test, restored.""" + tracked = SESSION_VARS + [MODE_ENV] + saved_env = {k: os.environ.get(k) for k in tracked} + saved_ctx = {name: var.get() for name, var in _VAR_MAP.items()} + saved_mode = sc._DESKTOP_CONNECTION_MODE.get() + saved_engaged = sc._session_context_engaged + for var in _VAR_MAP.values(): + var.set(sc._UNSET) + sc._DESKTOP_CONNECTION_MODE.set(sc._UNSET) + sc._session_context_engaged = False + try: + yield + finally: + for var, val in zip(_VAR_MAP.values(), saved_ctx.values()): + var.set(val) + sc._DESKTOP_CONNECTION_MODE.set(saved_mode) + sc._session_context_engaged = saved_engaged + for k, v in saved_env.items(): + if v is None: + os.environ.pop(k, None) + else: + os.environ[k] = v + + +@pytest.mark.parametrize("mode", ["local", "remote"]) +def test_bound_mode_is_stamped_for_the_child(mode): + set_desktop_connection_mode(mode) + assert _make_run_env({})[MODE_ENV] == mode + + +def test_remote_like_saved_mode_is_stamped_normalized(): + set_desktop_connection_mode("ssh") + assert _make_run_env({})[MODE_ENV] == "remote" + + +def test_unbound_session_stamps_nothing(): + """CLI, TUI, messaging, cron: the var is simply absent.""" + assert MODE_ENV not in _make_run_env({}) + + +def test_inherited_env_value_is_stripped_when_no_mode_is_bound(monkeypatch): + """The criterion: a user-set value is NOT a source of truth. + + A user who exports HERMES_DESKTOP_CONNECTION_MODE=local in their shell must + not be able to convince a CLI-session skill that gateway files are sitting + on a Desktop machine. + """ + monkeypatch.setenv(MODE_ENV, "local") + assert MODE_ENV not in _make_run_env({}) + + +def test_inherited_env_value_cannot_override_the_live_mode(monkeypatch): + """A remote Desktop session stays remote no matter what the shell says.""" + monkeypatch.setenv(MODE_ENV, "local") + set_desktop_connection_mode("remote") + assert _make_run_env({})[MODE_ENV] == "remote" + + +def test_stale_value_from_a_previous_turn_does_not_survive(monkeypatch): + """Each spawn re-derives; a cleared session strips rather than lingers.""" + tokens = set_session_vars(source="desktop", desktop_connection_mode="local") + assert _make_run_env({})[MODE_ENV] == "local" + monkeypatch.setenv(MODE_ENV, "local") # simulate a leaked process-global + clear_session_vars(tokens) + assert MODE_ENV not in _make_run_env({}) + + +def test_set_session_vars_carries_the_mode_through_to_the_child(): + tokens = set_session_vars(source="desktop", desktop_connection_mode="remote") + try: + assert _make_run_env({})[MODE_ENV] == "remote" + finally: + clear_session_vars(tokens) + + +def test_no_connection_details_are_ever_stamped(): + """Only the mode crosses the boundary — never a URL, host, or token.""" + set_desktop_connection_mode("remote") + env = _make_run_env({}) + leaked = [ + key + for key in env + if key.startswith("HERMES_DESKTOP") and key != MODE_ENV + ] + assert leaked == [] + assert env[MODE_ENV] in {"local", "remote"} diff --git a/tests/tools/test_mcp_connection_mode_meta.py b/tests/tools/test_mcp_connection_mode_meta.py new file mode 100644 index 000000000000..6308ca425fdc --- /dev/null +++ b/tests/tools/test_mcp_connection_mode_meta.py @@ -0,0 +1,158 @@ +"""MCP servers read the Desktop connection mode from per-call ``_meta`` (#82140). + +An MCP server can't read the gateway's contextvars, and its stdio env is fixed +at spawn time while the mode is per-session — one gateway can serve a local +Desktop client and a remote one at once. Per-call ``_meta`` is the only vehicle +that is both live and session-correct. +""" + +import pytest + +import gateway.session_context as sc +from gateway.session_context import set_desktop_connection_mode +from tools.mcp_tool import ( + MCP_DESKTOP_CONNECTION_MODE_META_KEY as META_KEY, + _call_tool_meta, + _call_tool_supports_meta, +) + + +@pytest.fixture(autouse=True) +def _reset_mode(): + saved = sc._DESKTOP_CONNECTION_MODE.get() + sc._DESKTOP_CONNECTION_MODE.set(sc._UNSET) + try: + yield + finally: + sc._DESKTOP_CONNECTION_MODE.set(saved) + + +@pytest.mark.parametrize("mode", ["local", "remote"]) +def test_bound_mode_becomes_call_meta(mode): + set_desktop_connection_mode(mode) + assert _call_tool_meta() == {META_KEY: mode} + + +def test_remote_like_saved_mode_is_normalized_before_it_leaves(): + set_desktop_connection_mode("cloud") + assert _call_tool_meta() == {META_KEY: "remote"} + + +def test_non_desktop_session_sends_no_meta(): + """CLI/TUI/messaging requests keep exactly today's shape.""" + assert _call_tool_meta() is None + + +def test_meta_carries_the_mode_and_nothing_else(): + """No base URL, host, token, SSH key, or auth mode may ride along.""" + set_desktop_connection_mode("remote") + meta = _call_tool_meta() + assert list(meta) == [META_KEY] + assert meta[META_KEY] in {"local", "remote"} + + +def test_meta_key_does_not_squat_the_reserved_spec_prefix(): + """MCP reserves `modelcontextprotocol.io/` for the spec itself.""" + assert not META_KEY.startswith("modelcontextprotocol.io/") + assert "/" in META_KEY + + +class TestSdkCapabilityProbe: + def test_probe_is_boolean_and_never_raises_without_the_sdk(self): + _call_tool_supports_meta.cache_clear() + try: + assert isinstance(_call_tool_supports_meta(), bool) + finally: + _call_tool_supports_meta.cache_clear() + + def test_probe_reports_false_when_the_sdk_lacks_meta(self, monkeypatch): + """An older SDK degrades to today's request shape instead of raising.""" + import sys + import types + + class _Session: + async def call_tool(self, name, arguments=None): # no `meta` param + ... + + module = types.ModuleType("mcp") + module.ClientSession = _Session + monkeypatch.setitem(sys.modules, "mcp", module) + _call_tool_supports_meta.cache_clear() + try: + assert _call_tool_supports_meta() is False + finally: + _call_tool_supports_meta.cache_clear() + + def test_probe_reports_true_when_the_sdk_accepts_meta(self, monkeypatch): + import sys + import types + + class _Session: + async def call_tool(self, name, arguments=None, meta=None): + ... + + module = types.ModuleType("mcp") + module.ClientSession = _Session + monkeypatch.setitem(sys.modules, "mcp", module) + _call_tool_supports_meta.cache_clear() + try: + assert _call_tool_supports_meta() is True + finally: + _call_tool_supports_meta.cache_clear() + + +class TestDocsExample: + """The published FastMCP example must be executable against the pinned SDK. + + The original example showed ``@server.call_tool()`` (not a decorator in + this tree's ``mcp`` SDK) and ``context.meta`` (absent on ``Context``); + the supported shape is a ``@server.tool()`` handler reading + ``ctx.request_context.meta`` (#82187 follow-up review, item 6). This pins + the docs snippet to the real SDK so a future SDK bump or docs edit that + breaks the pairing fails here instead of on a reader's machine. + """ + + def _docs_fastmcp_snippet(self) -> str: + import pathlib + import re + + doc = ( + pathlib.Path(__file__).resolve().parents[2] + / "website" + / "docs" + / "developer-guide" + / "desktop-connection-mode.md" + ) + blocks = re.findall(r"```python\n(.*?)```", doc.read_text(encoding="utf-8"), re.DOTALL) + sdk_blocks = [block for block in blocks if "FastMCP" in block] + assert len(sdk_blocks) == 1, "expected exactly one FastMCP example in the docs page" + return sdk_blocks[0] + + def test_example_executes_against_the_pinned_sdk(self): + pytest.importorskip("mcp.server.fastmcp") + snippet = self._docs_fastmcp_snippet() + namespace: dict = {} + # Executing (not just compiling) registers the tool: FastMCP inspects + # the handler signature at decoration time, so an unsupported decorator + # or Context parameter shape fails right here. + exec(compile(snippet, "desktop-connection-mode.md", "exec"), namespace) + assert namespace["MODE_KEY"] == META_KEY + + def test_documented_meta_access_reads_the_namespaced_key(self): + mcp_types = pytest.importorskip("mcp.types") + meta = mcp_types.RequestParams.Meta(**{META_KEY: "remote"}) + # The exact expression the docs show, on the real metadata model. + assert (meta.model_extra or {}).get(META_KEY) == "remote" + + def test_pinned_sdk_still_lacks_the_shapes_the_old_example_used(self): + """If the SDK grows Context.meta or a call_tool decorator, revisit the + docs example rather than silently drifting.""" + fastmcp = pytest.importorskip("mcp.server.fastmcp") + import inspect + + assert "meta" not in dir(fastmcp.Context) + assert "request_context" in dir(fastmcp.Context) + # call_tool is the dispatch method (self, name, arguments), not a + # decorator factory like tool(). + params = list(inspect.signature(fastmcp.FastMCP.call_tool).parameters) + assert params[:3] == ["self", "name", "arguments"] diff --git a/tests/tui_gateway/test_desktop_connection_mode_rpc.py b/tests/tui_gateway/test_desktop_connection_mode_rpc.py new file mode 100644 index 000000000000..488641839897 --- /dev/null +++ b/tests/tui_gateway/test_desktop_connection_mode_rpc.py @@ -0,0 +1,372 @@ +"""The TUI gateway's Desktop connection-mode plumbing (#82140). + +The Desktop shell already resolves ``local``/``remote`` via +``window.hermesDesktop.getConnection()``. These tests pin the server side of +that announcement: where it is stored, when it is refreshed, and which sessions +are allowed to have one at all. + +The helpers under test are pure dict/param transforms, so they run without +standing up a gateway. +""" + +import threading + +import pytest + +from gateway.session_context import _DESKTOP_CONNECTION_MODE, _UNSET, _VAR_MAP + + +def _srv(): + import tui_gateway.server as srv + + return srv + + +@pytest.fixture(autouse=True) +def _reset_contextvars(): + yield + for var in _VAR_MAP.values(): + var.set(_UNSET) + _DESKTOP_CONNECTION_MODE.set(_UNSET) + + +def _desktop_session(**extra) -> dict: + return {"session_key": "k", "source": "desktop", **extra} + + +class TestNormalizeParam: + def test_reads_and_normalizes_the_param(self): + assert _srv()._normalize_connection_mode_param({"connection_mode": "cloud"}) == "remote" + + @pytest.mark.parametrize("params", [None, {}, {"connection_mode": ""}, {"connection_mode": "nope"}]) + def test_missing_or_unknown_is_none(self, params): + assert _srv()._normalize_connection_mode_param(params) is None + + +class TestSessionConnectionMode: + def test_desktop_session_reports_its_mode(self): + session = _desktop_session(connection_mode="remote") + assert _srv()._session_connection_mode(session) == "remote" + + @pytest.mark.parametrize("source", ["tui", "telegram", "cli", "kanban"]) + def test_non_desktop_sources_never_report_a_mode(self, source): + """A stray connection_mode from a non-Desktop client must not be honored.""" + session = {"session_key": "k", "source": source, "connection_mode": "local"} + assert _srv()._session_connection_mode(session) is None + + def test_missing_session_is_none(self): + assert _srv()._session_connection_mode(None) is None + + def test_desktop_session_without_an_announcement_is_none(self): + assert _srv()._session_connection_mode(_desktop_session()) is None + + +class TestRememberConnectionMode: + def test_refreshes_the_stored_mode(self): + """This is what makes a mid-session connection switch land.""" + session = _desktop_session(connection_mode="local") + _srv()._remember_connection_mode(session, {"connection_mode": "remote"}) + assert session["connection_mode"] == "remote" + + def test_omitted_param_leaves_the_stored_mode_alone(self): + """An older Desktop build must not erase a mode a newer one announced.""" + session = _desktop_session(connection_mode="remote") + _srv()._remember_connection_mode(session, {"text": "hello"}) + assert session["connection_mode"] == "remote" + + def test_explicit_unknown_value_clears_to_none(self): + """Explicitly unknown is 'I don't know', not 'keep believing local'.""" + session = _desktop_session(connection_mode="local") + _srv()._remember_connection_mode(session, {"connection_mode": "banana"}) + assert session["connection_mode"] is None + + def test_no_session_is_a_noop(self): + _srv()._remember_connection_mode(None, {"connection_mode": "remote"}) + + +class TestBindSessionContext: + """``_set_session_context`` is what every turn runs through.""" + + def test_binds_a_desktop_session_mode(self, monkeypatch): + from gateway.session_context import desktop_connection_mode + + srv = _srv() + session = _desktop_session(connection_mode="remote") + monkeypatch.setattr(srv, "_sessions", {"s1": session}, raising=False) + srv._set_session_context("k") + assert desktop_connection_mode() == "remote" + + def test_non_desktop_session_binds_none(self, monkeypatch): + from gateway.session_context import desktop_connection_mode + + srv = _srv() + session = {"session_key": "k", "source": "tui", "connection_mode": "local"} + monkeypatch.setattr(srv, "_sessions", {"s1": session}, raising=False) + srv._set_session_context("k") + assert desktop_connection_mode() is None + + def test_unknown_session_key_binds_none(self, monkeypatch): + from gateway.session_context import desktop_connection_mode + + srv = _srv() + monkeypatch.setattr(srv, "_sessions", {}, raising=False) + srv._set_session_context("no-such-key") + assert desktop_connection_mode() is None + + def test_explicit_mode_wins_for_ephemeral_ids(self, monkeypatch): + """bg_*/preview_* task IDs aren't session keys; the caller-supplied + parent mode must bind instead of the (empty) lookup result.""" + from gateway.session_context import desktop_connection_mode + + srv = _srv() + monkeypatch.setattr(srv, "_sessions", {}, raising=False) + srv._set_session_context("bg_abc123", connection_mode="remote") + assert desktop_connection_mode() == "remote" + + def test_explicit_none_is_not_second_guessed(self, monkeypatch): + """An explicit None ('parent has no Desktop mode') must not be + overridden by a coincidental session-map hit.""" + from gateway.session_context import desktop_connection_mode + + srv = _srv() + session = _desktop_session(connection_mode="remote") + monkeypatch.setattr(srv, "_sessions", {"s1": session}, raising=False) + srv._set_session_context("k", connection_mode=None) + assert desktop_connection_mode() is None + + +def test_new_session_records_carry_a_connection_mode_slot(): + """Both live-session record shapes must have the field _set_session_context reads.""" + srv = _srv() + record = srv._deferred_session_record( + "key", cols=80, cwd="", history=[], lease=None, source="desktop", + connection_mode="remote", + ) + assert record["connection_mode"] == "remote" + + +def _host(): + import io + + from tui_gateway.compute_host import ComputeHost + + return ComputeHost(stdout=io.StringIO(), heartbeat_secs=0) + + +def _live_session(**extra) -> dict: + return { + "session_key": "k", + "source": "desktop", + "history": [], + "history_lock": threading.Lock(), + "history_version": 0, + "attached_images": [], + "cols": 80, + "cwd": "/w", + **extra, + } + + +class TestComputeHostBoundary: + """Dashboard turn isolation must not erase the Desktop connection mode. + + The compute-host child rebuilds the session from the ``turn.start`` frame, + so the frame must carry the parent's resolved mode and + ``_ensure_server_session`` must apply it on create and refresh it on reuse + — otherwise every isolated Desktop turn binds ``None`` and skills/MCP lose + the announcement (#82140). + """ + + def test_turn_frame_carries_the_resolved_mode(self): + frame = _srv()._compute_host_turn_frame( + "rid", "s1", _live_session(connection_mode="remote"), "hi" + ) + assert frame["connection_mode"] == "remote" + + def test_turn_frame_for_non_desktop_session_carries_none(self): + """A stray mode on a non-Desktop session must not cross the boundary.""" + frame = _srv()._compute_host_turn_frame( + "rid", "s1", _live_session(source="tui", connection_mode="local"), "hi" + ) + assert frame["connection_mode"] is None + + def test_child_new_session_receives_the_frame_mode(self, monkeypatch): + """The create path hands the frame mode to _init_session.""" + srv = _srv() + host = _host() + received = {} + + def _fake_init_session(sid, key, agent, history, **kwargs): + received.update(kwargs) + srv._sessions[sid] = { + "agent": agent, + "session_key": key, + "history": list(history), + "history_lock": threading.Lock(), + "source": srv._resolve_session_source(kwargs.get("source")), + "connection_mode": kwargs.get("connection_mode"), + } + + monkeypatch.setattr(srv, "_sessions", {}, raising=False) + monkeypatch.setattr(srv, "_make_agent", lambda *a, **k: object()) + monkeypatch.setattr(srv, "_transfer_db_to_agent", lambda *a, **k: False) + monkeypatch.setattr(srv, "_init_session", _fake_init_session) + frame = _srv()._compute_host_turn_frame( + "rid", "s1", _live_session(connection_mode="remote"), "hi" + ) + session = host._ensure_server_session(srv, frame) + assert received["connection_mode"] == "remote" + assert session["connection_mode"] == "remote" + + def test_child_fallback_session_keeps_the_frame_mode(self, monkeypatch): + """The minimal host-owned session (init machinery unavailable) too.""" + srv = _srv() + host = _host() + + def _boom(*a, **k): + raise RuntimeError("slash worker unavailable") + + monkeypatch.setattr(srv, "_sessions", {}, raising=False) + monkeypatch.setattr(srv, "_make_agent", lambda *a, **k: object()) + monkeypatch.setattr(srv, "_transfer_db_to_agent", lambda *a, **k: False) + monkeypatch.setattr(srv, "_init_session", _boom) + frame = _srv()._compute_host_turn_frame( + "rid", "s1", _live_session(connection_mode="remote"), "hi" + ) + session = host._ensure_server_session(srv, frame) + assert session["connection_mode"] == "remote" + + def test_child_reuse_refreshes_the_mode_and_binds_it(self, monkeypatch): + """A remote turn, then a switch to local: the reused child session must + refresh and the child's own turn context must observe the new mode.""" + from gateway.session_context import desktop_connection_mode + + srv = _srv() + host = _host() + child_session = _live_session(connection_mode="remote") + monkeypatch.setattr(srv, "_sessions", {"s1": child_session}, raising=False) + + frame = srv._compute_host_turn_frame( + "rid", "s1", _live_session(connection_mode="local"), "hi" + ) + reused = host._ensure_server_session(srv, frame) + assert reused is child_session + assert reused["connection_mode"] == "local" + + # What _run_prompt_submit's context bind now sees in the child. + srv._set_session_context("k") + assert desktop_connection_mode() == "local" + + def test_child_reuse_with_an_older_parent_frame_keeps_the_mode(self, monkeypatch): + """A frame without the key (older parent) must not erase the mode.""" + srv = _srv() + host = _host() + child_session = _live_session(connection_mode="remote") + monkeypatch.setattr(srv, "_sessions", {"s1": child_session}, raising=False) + host._ensure_server_session(srv, {"sid": "s1", "session_key": "k"}) + assert child_session["connection_mode"] == "remote" + + +class TestEphemeralAgentInheritance: + """Background and preview agents must inherit the parent Desktop mode. + + prompt.background and preview.restart bind fresh ``bg_*`` / ``preview_*`` + task IDs that are not in ``_sessions``, so the lookup-based derivation in + ``_set_session_context`` finds nothing; the handlers must hand the parent + session's resolved mode across explicitly (#82187 follow-up review, item 1). + Each probe reads all three surfaces INSIDE the detached agent thread: the + Python accessor, the subprocess env stamp, and the MCP per-call ``_meta``. + """ + + def _capture_inside_detached_agent(self, monkeypatch, method_name, params, session): + import queue + + import run_agent + + srv = _srv() + captured: queue.Queue = queue.Queue() + + class _ProbeAgent: + def __init__(self, **kwargs): + pass + + def run_conversation(self, **kwargs): + from gateway.session_context import ( + DESKTOP_CONNECTION_MODE_ENV, + desktop_connection_mode, + ) + from tools.environments.local import _make_run_env + from tools.mcp_tool import _call_tool_meta + + captured.put( + { + "accessor": desktop_connection_mode(), + "env": _make_run_env({}).get(DESKTOP_CONNECTION_MODE_ENV), + "meta": _call_tool_meta(), + } + ) + return {"final_response": "done"} + + monkeypatch.setattr(run_agent, "AIAgent", _ProbeAgent) + monkeypatch.setattr(srv, "_sessions", {"s1": session}, raising=False) + monkeypatch.setattr( + srv, "_background_agent_kwargs", lambda agent, task_id: {}, raising=False + ) + monkeypatch.setattr( + srv, "_ephemeral_preview_agent_kwargs", lambda agent, task_id: {}, raising=False + ) + monkeypatch.setattr( + srv, "_preview_restart_callbacks", lambda parent, task_id: {}, raising=False + ) + monkeypatch.setattr(srv, "_emit", lambda *a, **k: None, raising=False) + resp = srv._methods[method_name]("rid", {"session_id": "s1", **params}) + assert resp.get("error") is None, resp + return captured.get(timeout=15) + + def _parent(self, **extra) -> dict: + return { + "session_key": "k", + "source": "desktop", + "agent": object(), + "history": [], + "history_lock": threading.Lock(), + "cwd": "", + **extra, + } + + def test_background_agent_sees_the_parent_mode(self, monkeypatch): + from tools.mcp_tool import MCP_DESKTOP_CONNECTION_MODE_META_KEY + + seen = self._capture_inside_detached_agent( + monkeypatch, + "prompt.background", + {"text": "hi"}, + self._parent(connection_mode="remote"), + ) + assert seen["accessor"] == "remote" + assert seen["env"] == "remote" + assert seen["meta"] == {MCP_DESKTOP_CONNECTION_MODE_META_KEY: "remote"} + + def test_preview_agent_sees_the_parent_mode(self, monkeypatch): + from tools.mcp_tool import MCP_DESKTOP_CONNECTION_MODE_META_KEY + + seen = self._capture_inside_detached_agent( + monkeypatch, + "preview.restart", + {"url": "http://localhost:3000"}, + self._parent(connection_mode="remote"), + ) + assert seen["accessor"] == "remote" + assert seen["env"] == "remote" + assert seen["meta"] == {MCP_DESKTOP_CONNECTION_MODE_META_KEY: "remote"} + + def test_non_desktop_parent_spawns_modeless_children(self, monkeypatch): + """A TUI parent's stray connection_mode must not leak into children.""" + seen = self._capture_inside_detached_agent( + monkeypatch, + "prompt.background", + {"text": "hi"}, + self._parent(source="tui", connection_mode="local"), + ) + assert seen["accessor"] is None + assert seen["meta"] is None diff --git a/tools/environments/local.py b/tools/environments/local.py index de2a6e034670..5c89b96a5ed3 100644 --- a/tools/environments/local.py +++ b/tools/environments/local.py @@ -436,6 +436,8 @@ def _inject_session_context_env(env: dict) -> None: from gateway.session_context import ( _UNSET, _VAR_MAP, + DESKTOP_CONNECTION_MODE_ENV, + desktop_connection_mode, session_context_engaged, ) except Exception: @@ -452,6 +454,24 @@ def _inject_session_context_env(env: dict) -> None: # inherited global so a sibling session's value can't leak in. env.pop(var_name, None) + # The Desktop connection mode (#82140) is the skill-facing read path for + # "is the gateway's filesystem the machine the user is looking at?" — + # skills and their helper scripts branch on it to decide whether a file has + # to be transferred before it can be presented for local viewing/editing. + # + # STRICTLY WRITE-ONLY, unconditionally: stamped when a mode is bound and + # POPPED otherwise, on every spawn. That is deliberate and is what keeps the + # value from becoming a user-configurable env var — an inherited + # HERMES_DESKTOP_CONNECTION_MODE from the user's shell (or a stale one from + # a previous turn) is removed rather than passed through, so a child can + # only ever see what the live session actually resolved. Nothing in Hermes + # reads this name back; the source of truth is the contextvar. + mode = desktop_connection_mode() + if mode: + env[DESKTOP_CONNECTION_MODE_ENV] = mode + else: + env.pop(DESKTOP_CONNECTION_MODE_ENV, None) + def _sanitize_subprocess_env(base_env: dict | None, extra_env: dict | None = None) -> dict: """Filter Hermes-managed secrets from a subprocess environment.""" diff --git a/tools/mcp_tool.py b/tools/mcp_tool.py index 49fbf6f7eea4..e9e4e84de31f 100644 --- a/tools/mcp_tool.py +++ b/tools/mcp_tool.py @@ -99,6 +99,7 @@ import concurrent.futures import errno import fnmatch +import functools import inspect import json import logging @@ -571,6 +572,52 @@ def _context_var_value(ref: str) -> Optional[str]: return None +# --------------------------------------------------------------------------- +# Per-call request metadata +# --------------------------------------------------------------------------- + +# The `_meta` key carrying the resolved Desktop connection mode to MCP servers +# (#82140). MCP reserves the `modelcontextprotocol.io/` prefix for the spec, so +# this uses the documented third-party form: a domain we own plus a path. +MCP_DESKTOP_CONNECTION_MODE_META_KEY = "hermes-agent.nousresearch.com/desktop-connection-mode" + + +@functools.lru_cache(maxsize=1) +def _call_tool_supports_meta() -> bool: + """Whether the installed MCP SDK's ``call_tool`` accepts per-call ``meta``. + + Per-call metadata landed after the transport API stabilized, so probe rather + than pin behavior to a version: on an SDK without it we simply omit the + field and MCP servers see exactly today's request shape. + """ + try: + from mcp import ClientSession + + return "meta" in inspect.signature(ClientSession.call_tool).parameters + except Exception: + return False + + +def _call_tool_meta() -> Optional[dict]: + """Per-call ``_meta`` for the current turn, or ``None`` when there's nothing to say. + + Carries the resolved Desktop connection mode so an MCP server can tell + whether a path it returns will be openable on the machine the user is + looking at. Deliberately narrow: the mode and nothing else — no base URL, + host, token, SSH key, or auth mode. Non-Desktop sessions (CLI, TUI, + messaging, cron) contribute no key at all, so their requests are unchanged. + """ + try: + from gateway.session_context import desktop_connection_mode + + mode = desktop_connection_mode() + except Exception: + return None + if not mode: + return None + return {MCP_DESKTOP_CONNECTION_MODE_META_KEY: mode} + + # --------------------------------------------------------------------------- # Security helpers # --------------------------------------------------------------------------- @@ -5428,8 +5475,19 @@ async def _call(): # task, which doesn't inherit our contextvars) can replay # it and detect the gateway platform / session for routing. server._pending_call_context = contextvars.copy_context() + # Per-call `_meta` rides the request so a server can see the + # resolved Desktop connection mode (#82140). Read here, inside + # the agent's context, and only sent when the SDK supports it + # and there is a mode to report — otherwise the request shape + # is byte-identical to before. + call_meta = _call_tool_meta() if _call_tool_supports_meta() else None try: - result = await server.session.call_tool(tool_name, arguments=args) + if call_meta: + result = await server.session.call_tool( + tool_name, arguments=args, meta=call_meta + ) + else: + result = await server.session.call_tool(tool_name, arguments=args) finally: server._pending_call_context = None # The RPC round-trip completed — the session is demonstrably diff --git a/tui_gateway/compute_host.py b/tui_gateway/compute_host.py index c4d5a6ae7c7b..d9f8b784883c 100644 --- a/tui_gateway/compute_host.py +++ b/tui_gateway/compute_host.py @@ -536,6 +536,11 @@ def _ensure_server_session(self, server: Any, frame: dict[str, Any]) -> dict: session["profile_home"] = str(frame.get("profile_home")) if isinstance(frame.get("attached_images"), list): session["attached_images"] = list(frame.get("attached_images") or []) + if "connection_mode" in frame: + # Refresh so a mid-session Desktop connection switch lands on + # the very next isolated turn (#82140). An OMITTED key (older + # parent) must not erase a mode a newer frame already carried. + session["connection_mode"] = frame.get("connection_mode") return session history = frame.get("history") if isinstance(frame.get("history"), list) else [] @@ -598,6 +603,7 @@ def _ensure_server_session(self, server: Any, frame: dict[str, Any]) -> dict: cwd=str(frame.get("cwd") or "") or None, session_db=session_db, source=frame.get("source"), + connection_mode=frame.get("connection_mode"), ) finally: reset_transport(token) @@ -625,7 +631,12 @@ def _ensure_server_session(self, server: Any, frame: dict[str, Any]) -> dict: "edit_snapshots": {}, "tool_started_at": {}, "model_override": frame.get("model_override"), - "source": server._sanitize_client_source(frame.get("source")), + # _resolve_session_source, same as _init_session: the previous + # _sanitize_client_source reference never existed on the server + # module, so this fallback died with AttributeError instead of + # keeping a minimal host-owned session. + "source": server._resolve_session_source(frame.get("source")), + "connection_mode": frame.get("connection_mode"), "transport": self._transport, } session = server._sessions[sid] diff --git a/tui_gateway/methods_prompt.py b/tui_gateway/methods_prompt.py index 745a855e3d6b..9fbbb9b5923e 100644 --- a/tui_gateway/methods_prompt.py +++ b/tui_gateway/methods_prompt.py @@ -319,6 +319,12 @@ def _(rid, params: dict) -> dict: # in turn: a stale "hud" would tell the model the user is still floating # over another app when they are back in Hermes. session["client_surface"] = "hud" if params.get("surface") == "hud" else "" + # Same reasoning for the Desktop connection mode (#82140): the user can + # switch the active connection or profile between turns, and an extension + # that acts on a stale "local" hands them a link to a file that lives on the + # gateway machine. The client re-announces on every submit; an omitted + # ``connection_mode`` (older client) leaves the stored value alone. + _remember_connection_mode(session, params) has_truncation = ( truncate_user_ordinal is not None or params.get("truncate_before_row_id") is not None @@ -1208,7 +1214,14 @@ def _(rid, params: dict) -> dict: task_id = f"bg_{uuid.uuid4().hex[:6]}" def run(): - session_tokens = _set_session_context(task_id, cwd=_session_cwd(session)) + # task_id is ephemeral (not in _sessions), so the context bind cannot + # derive the Desktop connection mode by lookup — inherit the parent + # session's resolved mode explicitly (#82140). + session_tokens = _set_session_context( + task_id, + cwd=_session_cwd(session), + connection_mode=_session_connection_mode(session), + ) try: from run_agent import AIAgent @@ -1321,7 +1334,13 @@ def _(rid, params: dict) -> dict: def run(): # Pin the validated preview cwd, else the parent workspace — never an # invalid client path, which would silently fall back to the launch dir. - session_tokens = _set_session_context(task_id, cwd=(preview_cwd or _session_cwd(session))) + # Ephemeral preview task: inherit the parent's Desktop connection mode + # explicitly, same as prompt.background (#82140). + session_tokens = _set_session_context( + task_id, + cwd=(preview_cwd or _session_cwd(session)), + connection_mode=_session_connection_mode(session), + ) try: from run_agent import AIAgent from tools.terminal_tool import register_task_env_overrides diff --git a/tui_gateway/methods_session.py b/tui_gateway/methods_session.py index 281a18b53fe1..ec94db0122d5 100644 --- a/tui_gateway/methods_session.py +++ b/tui_gateway/methods_session.py @@ -83,6 +83,11 @@ def _(rid, params: dict) -> dict: "close_on_disconnect": is_truthy_value(params.get("close_on_disconnect", False)), "active_session_lease": lease, "cols": cols, + # The Desktop shell's resolved 'local'/'remote' connection mode for + # THIS backend (#82140). Refreshed on every resume/prompt so a + # connection or profile switch lands on the next turn. None for + # every non-Desktop client. + "connection_mode": _normalize_connection_mode_param(params), "created_at": now, "edit_snapshots": {}, "explicit_cwd": explicit_cwd, @@ -443,6 +448,11 @@ def _reuse_live_payload(sid: str, session: dict) -> dict: with _session_resume_lock: live = _find_live_session_by_key(target) if live is not None: + # Reopening a live chat is also a re-announcement: the client may + # have switched connection/profile since this session was + # registered (#82140). prompt.submit refreshes it again before + # any turn runs, so this only tightens the window. + _remember_connection_mode(live[1], params) return _ok(rid, _reuse_live_payload(*live)) # Lazy/watch resume: register the live session WITHOUT building an agent. @@ -482,6 +492,7 @@ def _reuse_live_payload(sid: str, session: dict) -> dict: close_on_disconnect=is_truthy_value(params.get("close_on_disconnect", False)), profile_home=profile_home, lazy=True, + connection_mode=_normalize_connection_mode_param(params), ) if (live := _claim_or_reuse_live(sid, target, record, lease)) is not None: return _ok(rid, _reuse_live_payload(*live)) @@ -646,6 +657,7 @@ def _reuse_live_payload(sid: str, session: dict) -> dict: profile_home=profile_home, model_override=overrides.get("model_override"), resume_runtime_overrides=overrides or None, + connection_mode=_normalize_connection_mode_param(params), ) if (live := _claim_or_reuse_live(sid, target, record, lease)) is not None: return _ok(rid, _reuse_live_payload(*live)) @@ -790,6 +802,7 @@ def _reuse_live_payload(sid: str, session: dict) -> dict: cwd=profile_resume_cwd, session_db=db, source=source, + connection_mode=_normalize_connection_mode_param(params), ) # Ownership TRANSFER — the registered session's agent now # holds this handle for its whole life, and _init_session @@ -3061,6 +3074,9 @@ def _visible_branch_history(messages): session_db=branch_db, source=source, profile_home=parent_home, + # A branch inherits the parent chat's connection mode: it is the + # same Desktop client talking to the same backend. + connection_mode=_session_connection_mode(session), ) # Ownership TRANSFER — the branched session's agent holds this # handle for its whole life and closes it on teardown. Drop is diff --git a/tui_gateway/server.py b/tui_gateway/server.py index bd3c38ffde28..9390ccc3c81e 100644 --- a/tui_gateway/server.py +++ b/tui_gateway/server.py @@ -1784,6 +1784,11 @@ def _compute_host_turn_frame( "reasoning_config_override": session.get("create_reasoning_override"), "service_tier_override": session.get("create_service_tier_override"), "source": _session_source(session), + # Resolved Desktop connection mode (#82140). The compute-host child + # rebuilds the session from this frame, so without it an isolated turn + # would bind None and skills/MCP would lose the mode the Desktop + # announced to the parent. + "connection_mode": _session_connection_mode(session), "attached_images": attached_images, "queued_prompt_generation": queued_prompt_generation, } @@ -2878,6 +2883,63 @@ def _session_source(session: dict | None) -> str: return _resolve_session_platform() +def _session_connection_mode(session: dict | None) -> str | None: + """The Desktop connection mode this session's client announced, if any. + + Gated on ``source == 'desktop'``: the mode describes the Desktop shell's + relationship to this backend, so it is meaningless coming from the TUI, a + messaging platform, or a plugin-opened session — and a stray + ``connection_mode`` param from one of those must not be honored. Every + non-Desktop surface therefore reports ``None``, which is what + ``desktop_connection_mode()`` promises them. See #82140. + """ + if not session or _session_source(session) != "desktop": + return None + try: + from gateway.session_context import normalize_desktop_connection_mode + + return normalize_desktop_connection_mode(session.get("connection_mode")) + except Exception: + return None + + +def _normalize_connection_mode_param(params: dict | None) -> str | None: + """Read ``connection_mode`` out of RPC *params* for a brand-new session.""" + try: + from gateway.session_context import normalize_desktop_connection_mode + + return normalize_desktop_connection_mode((params or {}).get("connection_mode")) + except Exception: + return None + + +def _remember_connection_mode(session: dict | None, params: dict | None) -> None: + """Refresh a session's announced Desktop connection mode from RPC *params*. + + The Desktop re-announces on every session.create/resume and every + prompt.submit, so switching the active connection or profile mid-session is + reflected on the very next turn rather than being pinned to whatever was + true when the chat was opened. + + An OMITTED ``connection_mode`` leaves the stored value alone — an older + Desktop build (or an internal caller that reuses these handlers) must not + silently erase a mode a newer client already announced. An explicitly + unrecognized value stores ``None`` ("mode unknown"), which is the safe + answer: extensions fall back to treating the location as unknown instead of + assuming local. + """ + if session is None or not params or "connection_mode" not in params: + return + try: + from gateway.session_context import normalize_desktop_connection_mode + + session["connection_mode"] = normalize_desktop_connection_mode( + params.get("connection_mode") + ) + except Exception: + pass + + def _register_session_cwd(session: dict | None) -> None: if not session: return @@ -3399,11 +3461,18 @@ def _cwd_for_session_key(session_key: str) -> str: return "" +# Sentinel for _set_session_context: "caller did not supply a mode, derive it +# from the live session map". Distinct from None, which is a real answer +# ("no Desktop mode") that must not be second-guessed by the lookup. +_DERIVE_CONNECTION_MODE = object() + + def _set_session_context( session_key: str, cwd: str | None = None, *, ui_session_id: str = "", + connection_mode: object = _DERIVE_CONNECTION_MODE, ) -> list: try: from gateway.session_context import set_session_vars @@ -3424,6 +3493,11 @@ def _set_session_context( # fall back to the session_key (matching the id derivation used at # session-finalize), so an identified session is never left blank. session_id = session_key + # Ephemeral task IDs (background, preview) aren't in `_sessions` either, + # so the loop below can't find a mode for them. Callers that hold the + # parent session pass its resolved mode explicitly (#82140); the + # session-map derivation only runs when nothing was supplied. + mode = None if connection_mode is _DERIVE_CONNECTION_MODE else connection_mode with _sessions_lock: for sess in list(_sessions.values()): if sess.get("session_key") == session_key: @@ -3431,6 +3505,8 @@ def _set_session_context( session_id = ( getattr(sess.get("agent"), "session_id", None) or session_key ) + if connection_mode is _DERIVE_CONNECTION_MODE: + mode = _session_connection_mode(sess) break return set_session_vars( session_key=session_key, @@ -3439,6 +3515,7 @@ def _set_session_context( cwd=resolved, ui_session_id=ui_session_id, cron_session="", + desktop_connection_mode=mode, ) except Exception: return [] @@ -7027,6 +7104,7 @@ def _init_session( session_db=None, source: str | None = None, profile_home: str | None = None, + connection_mode: str | None = None, ): now = time.time() with _sessions_lock: @@ -7047,6 +7125,10 @@ def _init_session( "slash_worker": None, "show_reasoning": _load_show_reasoning(), "source": _resolve_session_source(source), + # Desktop shell's resolved 'local'/'remote' connection mode (#82140); + # None for every non-Desktop client. Refreshed per turn from + # prompt.submit so a connection/profile switch lands immediately. + "connection_mode": connection_mode, "tool_progress_mode": _load_tool_progress_mode(), "edit_snapshots": {}, "tool_started_at": {}, @@ -8388,6 +8470,7 @@ def _deferred_session_record( lazy: bool = False, model_override=None, resume_runtime_overrides: dict | None = None, + connection_mode: str | None = None, ) -> dict: """A live-session record whose AIAgent is built later (lazy watch / cold resume) — _init_session's shape minus the agent.""" @@ -8400,6 +8483,9 @@ def _deferred_session_record( "close_on_disconnect": close_on_disconnect, "active_session_lease": lease, "cols": cols, + # Desktop shell's resolved 'local'/'remote' connection mode (#82140); + # None for every non-Desktop client. + "connection_mode": connection_mode, "created_at": now, "cwd": cwd, "display_history_prefix": display_history_prefix or [], diff --git a/website/docs/developer-guide/desktop-connection-mode.md b/website/docs/developer-guide/desktop-connection-mode.md new file mode 100644 index 000000000000..572cf4331cdd --- /dev/null +++ b/website/docs/developer-guide/desktop-connection-mode.md @@ -0,0 +1,167 @@ +--- +sidebar_label: "Desktop Connection Mode" +title: "Desktop Connection Mode" +description: "Read whether the Desktop app is driving a local or a remote backend — from a skill, an MCP server, or a Desktop plugin — so file links point somewhere the user can actually open." +--- + +# Desktop Connection Mode + +Hermes can execute on a gateway while you sit in front of the Desktop app on a +different machine. When the agent produces a path like `/home/user/report.md`, +that path is real *on the gateway* — and may be meaningless on the machine +rendering the chat. + +**Connection mode** is the one fact that disambiguates it: + +| Mode | Meaning | +|------|---------| +| `local` | The Desktop app is driving its own local backend. A path the agent reports is already a path on the machine the user is looking at. | +| `remote` | The Desktop app is driving an SSH, URL, or Hermes Cloud backend. A gateway-side path must be transferred before the Desktop can open it. | +| unavailable / `null` | Not a Desktop session (CLI, TUI, messaging, cron, API server), or the client didn't announce a mode. | + +The typical use: + +```text +if mode == "local": present the file directly +elif mode == "remote": copy it to the Desktop machine first, then present it +else: don't claim the file is locally openable +``` + +:::info Only the mode is exposed +Every read path below returns the connection's *shape* and nothing else. Base +URL, remote host, identity, tokens, SSH keys, and auth mode stay behind the +Electron bridge — a plugin that needs to move a file asks the backend to do it +rather than dialling the backend itself. +::: + +## Where the value comes from + +The Desktop shell already resolves the mode for its own use via +`window.hermesDesktop.getConnection()`; a `cloud` saved config resolves to a +`remote` connection, so only `local` and `remote` ever come out. + +The renderer announces that resolved mode to the backend on `session.create`, +`session.resume`, and — critically — on **every `prompt.submit`**. The per-turn +re-announcement is what makes switching the active connection or profile land +immediately, instead of pinning the answer to whatever was true when the chat +was opened. + +The gateway stores it on the live session and binds it into session context for +the turn. It is bound only for sessions whose `source` is `desktop`, so a stray +parameter from another client is ignored. + +:::warning Not an environment variable +The source of truth is a task-local context variable, not configuration. A +`HERMES_DESKTOP_CONNECTION_MODE` exported in your shell is **not** read anywhere +— on the subprocess path below it is actively stripped. That is deliberate: an +extension convinced a remote file is local hands the user a link to a file that +isn't on their machine. +::: + +## Reading it from a skill + +Skills invoke helper scripts through the `terminal` tool, and the subprocess +bridge stamps the mode onto every child environment as +`HERMES_DESKTOP_CONNECTION_MODE`. The variable is **absent** when there is no +Desktop session, so treat absence as "unknown", never as "local". + +```python +import os + +mode = os.environ.get("HERMES_DESKTOP_CONNECTION_MODE") # 'local' | 'remote' | None + +if mode == "local": + present(path) +elif mode == "remote": + present(transfer_to_desktop(path)) +else: + print(f"Path is on the Hermes host: {path}") +``` + +The stamp is re-derived on every spawn, so a mid-session connection switch is +reflected on the next command the skill runs. + +## Reading it from an MCP server + +A stdio MCP server's environment is fixed at spawn time, while the mode is +per-session — one gateway can serve a local Desktop client and a remote one at +the same moment. So the mode rides each request as MCP `_meta`: + +```json +{ + "_meta": { + "hermes-agent.nousresearch.com/desktop-connection-mode": "remote" + } +} +``` + +The key is absent for non-Desktop sessions, so those requests keep exactly the +shape they have today. It is also omitted when the installed `mcp` SDK predates +per-call metadata. + +Reading it with the Python SDK (FastMCP): + +```python +from mcp.server.fastmcp import Context, FastMCP + +server = FastMCP("file-delivery") + +MODE_KEY = "hermes-agent.nousresearch.com/desktop-connection-mode" + + +@server.tool() +async def deliver(path: str, ctx: Context) -> str: + meta = ctx.request_context.meta + # The key is namespaced (slashes/dots), so it lands in the metadata + # model's extra fields rather than as a declared attribute. + mode = (meta.model_extra or {}).get(MODE_KEY) if meta is not None else None + ... +``` + +## Reading it from a Desktop plugin + +`PluginContext` carries a `connection` door — the supported alternative to +reaching through the raw Electron bridge: + +```ts +export default { + id: 'file-delivery', + register(ctx) { + // Point-in-time read. + const mode = ctx.connection.mode() // 'local' | 'remote' | null + + // Or react to switches. Fires immediately with the current value, then on + // every real transition (connection switch, profile switch, reconnect). + ctx.connection.onModeChange(next => { + if (next === 'remote') { + enableTransferBeforeOpen() + } + }) + } +} +``` + +`onModeChange` returns an unsubscribe, and also registers one with the plugin's +disposers — a plugin that ignores the return value still stops listening when it +unloads. It fires only on genuine transitions; a reconnect that re-mints the +descriptor on the same mode is not a change. + +The value is read from the app's live connection atom rather than from +`getConnection()` directly, so it tracks the **active** profile. A raw bridge +call describes the primary window backend, which is the wrong answer whenever a +background profile is active. + +## Non-Desktop surfaces + +CLI, TUI, messaging platforms, cron, and the API server are unaffected: the +Python accessor returns `None`, the environment variable is not stamped, and MCP +requests carry no extra `_meta` key. + +## Reference + +| Surface | Read path | Absent value | +|---------|-----------|--------------| +| Python (core, tools) | `gateway.session_context.desktop_connection_mode()` | `None` | +| Skill scripts | `HERMES_DESKTOP_CONNECTION_MODE` | variable not set | +| MCP servers | `_meta["hermes-agent.nousresearch.com/desktop-connection-mode"]` | key not present | +| Desktop plugins | `ctx.connection.mode()` / `ctx.connection.onModeChange()` | `null` | diff --git a/website/sidebars.ts b/website/sidebars.ts index d857d33ed666..717aeca0d8ac 100644 --- a/website/sidebars.ts +++ b/website/sidebars.ts @@ -778,6 +778,7 @@ const sidebars: SidebarsConfig = { 'developer-guide/plugin-llm-access', 'developer-guide/subagent-lifecycle-api', 'developer-guide/desktop-plugin-sdk', + 'developer-guide/desktop-connection-mode', 'developer-guide/memory-provider-plugin', 'developer-guide/context-engine-plugin', 'developer-guide/secret-source-plugin',