From 9d9bfd6a7512afc93dd69e2d11cd268fcb39caed Mon Sep 17 00:00:00 2001 From: Robert Borkowski Date: Mon, 7 Sep 2026 01:28:32 -0700 Subject: [PATCH] fix: keep secondary profile approvals routable without cached bindings Preserve profile ownership proven at the secondary socket boundary rather than trusting arbitrary wire profile fields. Retire transient local owners with the profile pool, while keeping durable and exact remote ownership first. Salvage #103774 with two invariant tests and routing documentation. The profile-only fallback was also identified in the earlier #103770; this version retains the producer provenance and retirement boundary. Live Desktop renderer with two isolated serve backends: Reject previously failed after clearing durable bindings, leaving approval pending. With this change, the same action sends deny to the owning backend and pending clears. Existing-binding controls pass on both sides. No vendor inference used. Fixes #103755 Salvaged-from: 8a3c545e255b66b6ca4bc4b99cd725c5f7a08632 --- .../src/app/gateway/hooks/use-gateway-boot.ts | 2 + ...way-profile-only-owner.integration.test.ts | 185 ++++++++++++++++++ apps/desktop/src/store/gateway.ts | 19 +- .../src/store/session-event-provenance.ts | 31 +++ apps/desktop/src/store/session-states.ts | 57 ++++-- .../user-guide/multi-connection-desktop.md | 7 + 6 files changed, 285 insertions(+), 16 deletions(-) create mode 100644 apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts create mode 100644 apps/desktop/src/store/session-event-provenance.ts diff --git a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts index f5ee2ca66c..bbb560257b 100644 --- a/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts +++ b/apps/desktop/src/app/gateway/hooks/use-gateway-boot.ts @@ -72,6 +72,7 @@ import { $sessionTiles, $workingSessionIds, foregroundSessionScopes, + forgetProfileOnlyRuntimeOwners, liveSessionScopes, openTileGatewayScopes, reconcileBusyStatesOnReconnect, @@ -760,6 +761,7 @@ export function useGatewayBoot({ // primary thread or a just-created session's owner hold is bound to // (#93892). foregroundScopes: foregroundSessionScopes, + onLocalProfileRetired: forgetProfileOnlyRuntimeOwners, onActiveConnectionChanged: publish, // Keep $activeGatewayProfile in lockstep with the registry's OWN record // of which profile the active socket serves. The registry is the only diff --git a/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts b/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts new file mode 100644 index 0000000000..931180a353 --- /dev/null +++ b/apps/desktop/src/store/gateway-profile-only-owner.integration.test.ts @@ -0,0 +1,185 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const gatewayMocks = vi.hoisted(() => { + const instances: Array<{ + connectionState: string + emit: (event: { payload?: Record; session_id?: string; type: string }) => void + request: ReturnType + }> = [] + + return { instances } +}) + +vi.mock('@/hermes', async importActual => ({ + ...(await importActual>()), + setApiRequestConnection: vi.fn(), + HermesGateway: class { + connectionState = 'closed' + private eventHandlers = new Set<(event: { payload?: Record; session_id?: string; type: string }) => void>() + request = vi.fn(async (method: string, params: Record) => ({ method, params })) + + constructor() { + gatewayMocks.instances.push(this as never) + } + + connect = async (): Promise => { + this.connectionState = 'open' + } + + close = (): void => { + this.connectionState = 'closed' + } + + onEvent = ( + handler: (event: { payload?: Record; session_id?: string; type: string }) => void + ): (() => void) => { + this.eventHandlers.add(handler) + + return () => this.eventHandlers.delete(handler) + } + + onState = (): (() => void) => () => undefined + + emit = (event: { payload?: Record; session_id?: string; type: string }): void => { + for (const handler of this.eventHandlers) { + handler(event) + } + } + } +})) + +const { + closeSecondaryGateways, + configureGatewayRegistry, + ensureGatewayForProfile, + retireLocalProfileGateways, + setPrimaryGateway +} = await import('./gateway') + +const { $profiles } = await import('./profile') + +const { $activeSessionId, _resetSessionOwnerHintsForTests, setSessionOwnerHint, setSessions } = await import('./session') +const { $gateway } = await import('./gateway') +const { clearAllPrompts } = await import('./prompts') + +const { + $sessionStates, + $sessionTiles, + clearAllSessionStates, + forgetProfileOnlyRuntimeOwners, + knownOwnerForSession, + recordSessionEventScope, + requestForOwnedSession +} = await import('./session-states') + +const { isSessionOwnerResolutionError } = await import('./session-owner-resolution') + +function installDesktop(): void { + ;(window as unknown as { hermesDesktop: unknown }).hermesDesktop = { + getConnection: vi.fn(async (profile: string) => ({ + authMode: 'token', + mode: 'local', + profile, + token: 'test-token', + wsUrl: `wss://${profile}.invalid/ws` + })), + notify: vi.fn() + } +} + +beforeEach(() => { + installDesktop() + gatewayMocks.instances.length = 0 + $profiles.set([{ name: 'default' }, { name: 'research' }] as never) + $sessionTiles.set([]) + setSessions([]) + clearAllSessionStates() + clearAllPrompts() + $activeSessionId.set(null) + _resetSessionOwnerHintsForTests({ storage: true }) + + // This is the exact event fan-in installed by useGatewayBoot: the secondary + // producer calls config.onEvent, which records scope before UI dispatch. + configureGatewayRegistry({ + onLocalProfileRetired: forgetProfileOnlyRuntimeOwners, + onEvent: event => { + recordSessionEventScope(event) + } + }) + const primary = { connectionState: 'open', request: vi.fn() } + $gateway.set(primary as never) + setPrimaryGateway(primary as never, 'default') +}) + +afterEach(() => { + closeSecondaryGateways() + clearAllSessionStates() + $sessionTiles.set([]) + $profiles.set([]) + setSessions([]) + _resetSessionOwnerHintsForTests({ storage: true }) + clearAllPrompts() + $activeSessionId.set(null) + $gateway.set(null) + delete (window as { hermesDesktop?: unknown }).hermesDesktop +}) + +describe('profile-only secondary approval ownership', () => { + it('keeps a profile-only secondary event owner across a focus switch and dispatches approval on that secondary', async () => { + await expect(ensureGatewayForProfile('research')).resolves.toBeUndefined() + const secondary = gatewayMocks.instances[0] + + expect(secondary).toBeTruthy() + // No tile, row, hint, or runtime→stored state mirror is available. + expect($sessionTiles.get()).toEqual([]) + expect($sessionStates.get()).toEqual({}) + + secondary.emit({ session_id: 'rt-secondary', type: 'approval.request' }) + await expect(ensureGatewayForProfile('default')).resolves.toBeUndefined() + + expect(knownOwnerForSession('rt-secondary')).toBe('research') + + const ambient = vi.fn(async () => ({ via: 'ambient' })) + await expect( + requestForOwnedSession('rt-secondary', ambient as never, 'approval.respond', { + choice: 'once', + session_id: 'rt-secondary' + }) + ).resolves.toEqual({ + method: 'approval.respond', + params: { choice: 'once', session_id: 'rt-secondary' } + }) + + expect(secondary.request).toHaveBeenCalledWith('approval.respond', { + choice: 'once', + session_id: 'rt-secondary' + }) + expect(ambient).not.toHaveBeenCalled() + }) + + it('rejects unproven profiles and retires only transient local ownership, never durable exact routes', async () => { + recordSessionEventScope({ profile: 'research', session_id: 'rt-unproven' }) + expect(knownOwnerForSession('rt-unproven')).toBeUndefined() + await expect(requestForOwnedSession('rt-unproven', vi.fn() as never, 'approval.respond', {})) + .rejects.toSatisfy(isSessionOwnerResolutionError) + + await ensureGatewayForProfile('research') + const secondary = gatewayMocks.instances[0] + secondary.emit({ session_id: 'rt-retired', type: 'approval.request' }) + secondary.emit({ session_id: 'rt-durable', type: 'approval.request' }) + expect(knownOwnerForSession('rt-retired')).toBe('research') + const exact = { connectionId: 'remote-same-name', profile: 'research' } + setSessionOwnerHint('rt-durable', exact) + recordSessionEventScope({ ...exact, session_id: 'rt-remote' }) + expect(knownOwnerForSession('rt-durable')).toEqual(exact) + retireLocalProfileGateways('research') + expect(knownOwnerForSession('rt-retired')).toBeUndefined() + expect(knownOwnerForSession('rt-durable')).toEqual(exact) + expect(knownOwnerForSession('rt-remote')).toEqual(exact) + const getConnection = window.hermesDesktop!.getConnection + vi.mocked(getConnection).mockClear() + await expect(requestForOwnedSession('rt-retired', vi.fn() as never, 'approval.respond', {})) + .rejects.toSatisfy(isSessionOwnerResolutionError) + expect(getConnection).not.toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/src/store/gateway.ts b/apps/desktop/src/store/gateway.ts index 0812aec00f..f9ce195cb1 100644 --- a/apps/desktop/src/store/gateway.ts +++ b/apps/desktop/src/store/gateway.ts @@ -7,6 +7,7 @@ import { reconnectBackoffDelayMs } from '@/lib/reconnect-backoff' import { RECONNECT_ATTEMPT_TIMEOUT_MS, withTimeout } from '@/lib/with-timeout' import { markNativeNotifyBaseline } from '@/store/notify-baseline' import { setConnection, setGatewayState } from '@/store/session' +import { stampSecondaryProfileOwner } from '@/store/session-event-provenance' // ── Multi-profile gateway routing ────────────────────────────────────────── // Concurrent sessions across profiles need concurrent sockets: the renderer's @@ -61,6 +62,10 @@ interface RegistryConfig { * (#89206: the stale-profile split-brain that stranded bot wake-ups). */ onActiveRouteChanged?: (profile: string) => void + /** Drop transient profile-pool runtime routes when local profile teardown + * permanently retires their owning secondary. Exact registry routes are + * deliberately outside this callback: a remote source may share the name. */ + onLocalProfileRetired?: (profile: string) => void /** * Scopes a FOREGROUND surface is bound to right now — every mounted * session tile's owner and the primary thread's (foregroundSessionScopes in @@ -757,9 +762,13 @@ function createSecondary(profile: string, connectionId: null | string = null): S } // Events keep carrying the bare profile — session routing is profile-keyed - // everywhere. connectionId rides along for surfaces that need the source. + // everywhere. A pool secondary with no registry connection has no exact + // connection id, so stamp this closure-owned profile before registry fan-in; + // the recorder must not promote an arbitrary wire `profile` field instead. entry.offEvent = gateway.onEvent(event => { - g.config?.onEvent({ ...event, profile, ...(connectionId ? { connectionId } : {}) }) + const scopedEvent = stampSecondaryProfileOwner({ ...event, ...(connectionId ? { connectionId } : {}) }, profile) + + g.config?.onEvent(scopedEvent) releaseTerminalTurnLease(entry.scope, event) }) entry.offState = gateway.onState(state => { @@ -1828,6 +1837,12 @@ export function retireLocalProfileGateways(profile: string): void { const scopes = new Set([key, registryBackendScopeKey('local', key)]) let activeInvalidated = false + // A profile-only owner is a claim about the legacy local pool, not durable + // session identity. Clear it with that pool before a delayed session action + // can recreate the deleted/old-name backend. Exact remote owners are + // descriptors and remain routable even when they share this profile name. + g.config?.onLocalProfileRetired?.(key) + for (const scope of scopes) { const entry = g.secondaries.get(scope) diff --git a/apps/desktop/src/store/session-event-provenance.ts b/apps/desktop/src/store/session-event-provenance.ts new file mode 100644 index 0000000000..0be840405a --- /dev/null +++ b/apps/desktop/src/store/session-event-provenance.ts @@ -0,0 +1,31 @@ +import type { GatewayEvent } from '@hermes/shared' + +// A profile name received over the wire is descriptive, not an ownership +// claim: primary and arbitrary synthetic events can carry one. The secondary +// socket closure is the authority for profile-only pool routes, so stamp its +// outgoing copy with a non-serializable marker before it crosses registry +// fan-in. JSON-RPC payloads cannot forge a Symbol property. +const secondaryProfileOwner = Symbol('secondaryProfileOwner') + +type SecondaryProfileOwnedEvent = GatewayEvent & { + [secondaryProfileOwner]?: string +} + +export function stampSecondaryProfileOwner(event: GatewayEvent, profile: string): GatewayEvent { + const scoped = { ...event, profile } as SecondaryProfileOwnedEvent + + Object.defineProperty(scoped, secondaryProfileOwner, { + configurable: false, + enumerable: false, + value: profile, + writable: false + }) + + return scoped +} + +export function secondaryProfileOwnerForEvent(event: GatewayEvent): string | undefined { + const profile = (event as SecondaryProfileOwnedEvent)[secondaryProfileOwner] + + return typeof profile === 'string' && profile.trim() ? profile.trim() : undefined +} diff --git a/apps/desktop/src/store/session-states.ts b/apps/desktop/src/store/session-states.ts index afd217b79b..19c54c58e9 100644 --- a/apps/desktop/src/store/session-states.ts +++ b/apps/desktop/src/store/session-states.ts @@ -16,7 +16,7 @@ * itself here as the delegate so tile UI stays dependency-light. */ -import { LOCAL_CONNECTION_ID, registryBackendScopeKey } from '@hermes/shared' +import { type GatewayEvent, LOCAL_CONNECTION_ID, registryBackendScopeKey } from '@hermes/shared' import { atom, computed } from 'nanostores' import type { ClientSessionState } from '@/app/types' @@ -56,6 +56,7 @@ import { setBusy, setSessions } from './session' +import { secondaryProfileOwnerForEvent } from './session-event-provenance' import { assertSessionOwnerResolved } from './session-owner-resolution' import { requestForSessionProfile, @@ -84,22 +85,50 @@ export const $sessionStates = atom>({}) const sessionScopeByRuntimeId = new Map() -// Structured twin of the scope ledger: the same inbound events also carry the -// exact (connectionId, profile) owner, which the composite scope string -// cannot give back. Consumed as the LAST rung of knownOwnerForSession so a -// runtime whose event source already proved its owner can still route -// session-scoped RPCs (approval.respond) when every durable binding -// (tile / hint / row) is absent — while durable stored identity keeps -// outranking it (#97511). -const sessionOwnerByRuntimeId = new Map() +// Structured twin of the scope ledger: inbound events can carry either an +// exact (connectionId, profile) owner or a producer-proven profile-only pool +// owner. Consumed as the LAST rung of knownOwnerForSession so a runtime whose +// event source already proved its owner can still route session-scoped RPCs +// (approval.respond) when every durable binding (tile / hint / row) is absent +// — while durable stored identity keeps outranking it (#97511). +const sessionOwnerByRuntimeId = new Map() export function recordSessionEventScope(event: { connectionId?: string; profile?: string; session_id?: string }): void { - if (event.session_id && event.connectionId) { + if (!event.session_id) { + return + } + + if (event.connectionId) { sessionScopeByRuntimeId.set(event.session_id, registryBackendScopeKey(event.connectionId, event.profile)) sessionOwnerByRuntimeId.set(event.session_id, { connectionId: event.connectionId, profile: String(event.profile ?? '').trim() || 'default' }) + + return + } + + // Only gateway.ts's secondary closure can add this non-serializable marker. + // A profile field from a primary or arbitrary inbound event is descriptive, + // not an owner route, and must keep failing closed in multi-profile installs. + const profile = secondaryProfileOwnerForEvent(event as GatewayEvent) + + if (profile) { + sessionOwnerByRuntimeId.set(event.session_id, profile) + } +} + +/** Forget only profile-pool runtime owners during permanent LOCAL profile + * teardown. These string routes came exclusively from the legacy secondary + * producer; exact connection descriptors must survive a same-named remote + * profile's local delete/rename. */ +export function forgetProfileOnlyRuntimeOwners(profile: string): void { + const retired = normalizeProfileKey(profile) + + for (const [runtimeId, owner] of sessionOwnerByRuntimeId) { + if (typeof owner === 'string' && normalizeProfileKey(owner) === retired) { + sessionOwnerByRuntimeId.delete(runtimeId) + } } } @@ -989,10 +1018,10 @@ export function openTileGatewayScopes(): Set { * Last rung: the owner recorded from the inbound runtime event itself * (sessionOwnerByRuntimeId, #97511) — an orphan runtime whose tile/hint/row * binding is absent or stale still routes through the exact - * (connectionId, profile) its events proved, while every durable rung above - * keeps outranking it, so a stored-id collision never inherits a stale - * runtime ledger entry. Untagged events record nothing, so unknown owners in - * multi-profile topology still fail closed. + * (connectionId, profile) or secondary socket's proven local profile. Every + * durable rung above keeps outranking it, so a stored-id collision never + * inherits a stale runtime ledger entry. Unproven profile fields record + * nothing, so unknown owners in multi-profile topology still fail closed. * Returns undefined when no owner is known — the caller fails closed * (assertSessionOwnerResolved), never falls to "active". */ diff --git a/website/docs/user-guide/multi-connection-desktop.md b/website/docs/user-guide/multi-connection-desktop.md index 13ce2e88b6..9746d02cdc 100644 --- a/website/docs/user-guide/multi-connection-desktop.md +++ b/website/docs/user-guide/multi-connection-desktop.md @@ -157,6 +157,13 @@ Each `(connection, profile)` pair gets its own backend and socket, pooled with the same idle-reaping as local per-profile backends — background agents keep streaming while you look at another gateway. +Approval buttons route back to the session's owning backend, not whichever +profile is currently selected. For a local secondary profile, Desktop can use +the socket that delivered the request even when the cached session binding is +missing. Saved session ownership still takes precedence, and deleting or +renaming that local profile clears this temporary route rather than reconnecting +an obsolete backend. + ### Switching and scoping The sidebar foot follows one hierarchy: **gateway → profile → sessions**.