From 027380b37f4c0b9bae42ae66b283922af16906e5 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:19:10 +0000 Subject: [PATCH 1/6] fix: report bridge approval outcome on approval.resolved The Python bridge broadcasts approval.resolved for a gateway approval even when resolve_gateway_approval did not resolve anything, and the event carried no outcome. The chat-run forwarder then dropped the flag entirely, so a failed resolution reached the client indistinguishable from a successful one and closed the approval card. Carry the resolved outcome on the broadcast event and forward it when the runtime reports one. --- .../services/bridge/python/bridge_pool.py | 1 + .../services/chat-run/handle-bridge-run.ts | 2 + .../agent-bridge-python-concurrency.test.ts | 51 +++++++ tests/server/run-chat-bridge-resume.test.ts | 137 ++++++++++++++++++ 4 files changed, 191 insertions(+) diff --git a/packages/server/src/modules/hermes/services/bridge/python/bridge_pool.py b/packages/server/src/modules/hermes/services/bridge/python/bridge_pool.py index b31c93cebd..a632f5603f 100644 --- a/packages/server/src/modules/hermes/services/bridge/python/bridge_pool.py +++ b/packages/server/src/modules/hermes/services/bridge/python/bridge_pool.py @@ -2291,6 +2291,7 @@ def respond_approval(self, approval_id: str, choice: str) -> dict[str, Any]: "run_id": gateway_run_id, "approval_id": approval_id, "choice": cleaned, + "resolved": resolved, }) return {"approval_id": approval_id, "resolved": resolved, "choice": cleaned} return {"approval_id": approval_id, "resolved": True, "choice": cleaned} diff --git a/packages/server/src/modules/studio/services/chat-run/handle-bridge-run.ts b/packages/server/src/modules/studio/services/chat-run/handle-bridge-run.ts index fb66e1b8dc..fe1442666f 100644 --- a/packages/server/src/modules/studio/services/chat-run/handle-bridge-run.ts +++ b/packages/server/src/modules/studio/services/chat-run/handle-bridge-run.ts @@ -1532,6 +1532,8 @@ async function applyBridgeChunkAsync( run_id: chunk.run_id, approval_id: ev.approval_id, choice: ev.choice, + // Older runtimes omit resolved; forward only an explicit outcome. + ...(typeof ev.resolved === 'boolean' ? { resolved: ev.resolved } : {}), } replaceState(sessionMap, sessionId, 'approval.resolved', payload) emit('approval.resolved', payload) diff --git a/tests/server/agent-bridge-python-concurrency.test.ts b/tests/server/agent-bridge-python-concurrency.test.ts index 8a4ca0f206..86c79f188d 100644 --- a/tests/server/agent-bridge-python-concurrency.test.ts +++ b/tests/server/agent-bridge-python-concurrency.test.ts @@ -1360,6 +1360,57 @@ assert [(msg["role"], msg["content"]) for msg in messages] == [ `) }) + it('reports the gateway approval outcome on the broadcast approval.resolved event', () => { + runPython(String.raw` +${harness} + +pool, _fake_db = make_pool() + +def notify_gateway_approval(session_id, run_id, request_id, queue_request): + session = bridge.AgentSession(session_id=session_id, agent=object(), current_run_id=run_id) + record = bridge.RunRecord(run_id=run_id, session_id=session_id) + with pool._lock: + pool._sessions[session_id] = session + pool._runs[run_id] = record + if queue_request: + approval.queue_gateway_approval(session_id, request_id) + pool._gateway_approval_notify(session_id)({ + "request_id": request_id, + "command": "printf harmless gateway", + "description": "harmless test command", + }) + approval_id = next( + key for key, value in pool._gateway_approval_requests.items() if value[0] == session_id + ) + return record, approval_id + +# A runtime that queued the request resolves, and the event says so. +record, approval_id = notify_gateway_approval("session-ok", "run-ok", "request-ok", True) +result = pool.respond_approval(approval_id, "once") +assert result["resolved"] is True, result +resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] +assert len(resolved_events) == 1, resolved_events +assert resolved_events[0]["resolved"] is True, resolved_events[0] +assert resolved_events[0]["choice"] == "once", resolved_events[0] + +# A runtime that never registered the request does not resolve, and the event says so. +record, approval_id = notify_gateway_approval("session-stale", "run-stale", "request-stale", False) +result = pool.respond_approval(approval_id, "once") +assert result["resolved"] is False, result +resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] +assert len(resolved_events) == 1, resolved_events +assert resolved_events[0]["resolved"] is False, resolved_events[0] + +# A runtime older than v0.20.5 emits no request_id at all: same honest outcome. +record, approval_id = notify_gateway_approval("session-legacy", "run-legacy", "", False) +result = pool.respond_approval(approval_id, "deny") +assert result["resolved"] is False, result +resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] +assert len(resolved_events) == 1, resolved_events +assert resolved_events[0]["resolved"] is False, resolved_events[0] +`) + }) + it('remembers execute_code approvals inside the bridge without patching upstream files', () => { runPython(String.raw` ${harness} diff --git a/tests/server/run-chat-bridge-resume.test.ts b/tests/server/run-chat-bridge-resume.test.ts index 2d66b06c2d..1a6e14a8c9 100644 --- a/tests/server/run-chat-bridge-resume.test.ts +++ b/tests/server/run-chat-bridge-resume.test.ts @@ -319,6 +319,143 @@ describe('resumeBridgeRun', () => { }, ) + it.each([ + { name: 'a failed gateway resolution', resolved: false }, + { name: 'a successful gateway resolution', resolved: true }, + ])('forwards the bridge approval outcome for $name', async ({ resolved }) => { + const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + const { nsp, emitted } = createNamespace() + const socket = { id: 'socket-1', connected: true, emit: vi.fn() } + const sessionMap = new Map() + sessionMap.set('session-resume', { + messages: [{ id: 1, session_id: 'session-resume', role: 'user', content: 'hello', timestamp: 1 }], + isWorking: true, + events: [], + queue: [], + }) + + const bridge = { + getResult: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'running', + output: '', + deltas: [], + events: [], + })), + getOutput: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'complete', + delta: '', + cursor: 0, + output: '', + done: true, + result: { final_response: '' }, + error: null, + events: [{ + event: 'approval.resolved', + run_id: 'run-resume', + approval_id: 'approval-1', + choice: 'once', + resolved, + }], + event_cursor: 1, + })), + } + + await resumeBridgeRun( + nsp as any, + socket as any, + { + sessionId: 'session-resume', + runId: 'run-resume', + profile: 'default', + instructions: 'system prompt', + model: 'gpt-test', + provider: 'openai', + }, + sessionMap, + bridge as any, + vi.fn(), + ) + + const approvalResolved = emitted.filter(item => item.event === 'approval.resolved') + expect(approvalResolved).toHaveLength(1) + expect(approvalResolved[0].payload).toMatchObject({ + approval_id: 'approval-1', + choice: 'once', + resolved, + }) + }) + + it('omits resolved when an older bridge runtime does not report an approval outcome', async () => { + const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + const { nsp, emitted } = createNamespace() + const socket = { id: 'socket-1', connected: true, emit: vi.fn() } + const sessionMap = new Map() + sessionMap.set('session-resume', { + messages: [{ id: 1, session_id: 'session-resume', role: 'user', content: 'hello', timestamp: 1 }], + isWorking: true, + events: [], + queue: [], + }) + + const bridge = { + getResult: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'running', + output: '', + deltas: [], + events: [], + })), + getOutput: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'complete', + delta: '', + cursor: 0, + output: '', + done: true, + result: { final_response: '' }, + error: null, + events: [{ + event: 'approval.resolved', + run_id: 'run-resume', + approval_id: 'approval-legacy', + choice: 'deny', + }], + event_cursor: 1, + })), + } + + await resumeBridgeRun( + nsp as any, + socket as any, + { + sessionId: 'session-resume', + runId: 'run-resume', + profile: 'default', + instructions: 'system prompt', + model: 'gpt-test', + provider: 'openai', + }, + sessionMap, + bridge as any, + vi.fn(), + ) + + const approvalResolved = emitted.filter(item => item.event === 'approval.resolved') + expect(approvalResolved).toHaveLength(1) + expect(approvalResolved[0].payload).toMatchObject({ approval_id: 'approval-legacy', choice: 'deny' }) + expect('resolved' in approvalResolved[0].payload).toBe(false) + }) + it('completes a timed-out abort when the resumed bridge run reaches a terminal state', async () => { const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') const { nsp, emitted } = createNamespace() From 9daa0b3bd3ce41f405e385a6794d033cbda601ed Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Sun, 20 Sep 2026 14:48:00 +0000 Subject: [PATCH 2/6] test: cover approval outcome boundaries on both bridge run surfaces Split the three-case Python broadcast test so each runtime condition fails independently, add the gateway-raises branch and an interrupt-path control, pin the non-boolean resolved values the typeof guard drops, and cover the live handleBridgeRun surface alongside the resume one. --- .../agent-bridge-python-concurrency.test.ts | 102 ++++++++++++++---- .../run-chat-bridge-final-context.test.ts | 60 +++++++++++ tests/server/run-chat-bridge-resume.test.ts | 72 +++++++++++++ 3 files changed, 213 insertions(+), 21 deletions(-) diff --git a/tests/server/agent-bridge-python-concurrency.test.ts b/tests/server/agent-bridge-python-concurrency.test.ts index 86c79f188d..f8fbc4419f 100644 --- a/tests/server/agent-bridge-python-concurrency.test.ts +++ b/tests/server/agent-bridge-python-concurrency.test.ts @@ -241,6 +241,28 @@ def wait_for(condition, timeout=20): return False ` +const gatewayApprovalPrelude = String.raw` +pool, _fake_db = make_pool() + +def notify_gateway_approval(session_id, run_id, request_id, queue_request): + session = bridge.AgentSession(session_id=session_id, agent=object(), current_run_id=run_id) + record = bridge.RunRecord(run_id=run_id, session_id=session_id) + with pool._lock: + pool._sessions[session_id] = session + pool._runs[run_id] = record + if queue_request: + approval.queue_gateway_approval(session_id, request_id) + pool._gateway_approval_notify(session_id)({ + "request_id": request_id, + "command": "printf harmless gateway", + "description": "harmless test command", + }) + approval_id = next( + key for key, value in pool._gateway_approval_requests.items() if value[0] == session_id + ) + return record, approval_id +` + describe('agent bridge Python session concurrency', () => { it('denies only the interrupted session run generation approval queues', () => { runPython(String.raw` @@ -1360,29 +1382,10 @@ assert [(msg["role"], msg["content"]) for msg in messages] == [ `) }) - it('reports the gateway approval outcome on the broadcast approval.resolved event', () => { + it('reports a resolved gateway approval outcome on the broadcast approval.resolved event', () => { runPython(String.raw` ${harness} - -pool, _fake_db = make_pool() - -def notify_gateway_approval(session_id, run_id, request_id, queue_request): - session = bridge.AgentSession(session_id=session_id, agent=object(), current_run_id=run_id) - record = bridge.RunRecord(run_id=run_id, session_id=session_id) - with pool._lock: - pool._sessions[session_id] = session - pool._runs[run_id] = record - if queue_request: - approval.queue_gateway_approval(session_id, request_id) - pool._gateway_approval_notify(session_id)({ - "request_id": request_id, - "command": "printf harmless gateway", - "description": "harmless test command", - }) - approval_id = next( - key for key, value in pool._gateway_approval_requests.items() if value[0] == session_id - ) - return record, approval_id +${gatewayApprovalPrelude} # A runtime that queued the request resolves, and the event says so. record, approval_id = notify_gateway_approval("session-ok", "run-ok", "request-ok", True) @@ -1392,6 +1395,13 @@ resolved_events = [event for event in record.events if event["event"] == "approv assert len(resolved_events) == 1, resolved_events assert resolved_events[0]["resolved"] is True, resolved_events[0] assert resolved_events[0]["choice"] == "once", resolved_events[0] +`) + }) + + it('reports an unresolved gateway approval outcome when the runtime never registered the request', () => { + runPython(String.raw` +${harness} +${gatewayApprovalPrelude} # A runtime that never registered the request does not resolve, and the event says so. record, approval_id = notify_gateway_approval("session-stale", "run-stale", "request-stale", False) @@ -1400,17 +1410,67 @@ assert result["resolved"] is False, result resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] assert len(resolved_events) == 1, resolved_events assert resolved_events[0]["resolved"] is False, resolved_events[0] +assert resolved_events[0]["choice"] == "once", resolved_events[0] +`) + }) + + it('reports an unresolved gateway approval outcome for a runtime that sends no request id', () => { + runPython(String.raw` +${harness} +${gatewayApprovalPrelude} # A runtime older than v0.20.5 emits no request_id at all: same honest outcome. +# The empty request_id short-circuits before resolve_gateway_approval is called. record, approval_id = notify_gateway_approval("session-legacy", "run-legacy", "", False) +before = len(approval._resolved_gateway_requests) result = pool.respond_approval(approval_id, "deny") assert result["resolved"] is False, result +assert len(approval._resolved_gateway_requests) == before, approval._resolved_gateway_requests +resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] +assert len(resolved_events) == 1, resolved_events +assert resolved_events[0]["resolved"] is False, resolved_events[0] +assert resolved_events[0]["choice"] == "deny", resolved_events[0] +`) + }) + + it('reports an unresolved gateway approval outcome when the approval gateway raises', () => { + runPython(String.raw` +${harness} +${gatewayApprovalPrelude} + +# The except Exception branch: a gateway that raises must not be reported as resolved. +def raising_resolve_gateway_approval(session_key, choice, request_id=None): + raise RuntimeError("gateway unavailable") + +approval.resolve_gateway_approval = raising_resolve_gateway_approval + +record, approval_id = notify_gateway_approval("session-raise", "run-raise", "request-raise", True) +result = pool.respond_approval(approval_id, "once") +assert result["resolved"] is False, result resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] assert len(resolved_events) == 1, resolved_events assert resolved_events[0]["resolved"] is False, resolved_events[0] `) }) + it('keeps reporting the interrupt-path approval.resolved event without an outcome (control)', () => { + runPython(String.raw` +${harness} +${gatewayApprovalPrelude} + +# Control for the respond_approval branch only: the session-interrupt path +# resolves gateway approvals of its own and is untouched by the fix, so its +# event carries a reason and still no resolved key on either arm. +record, approval_id = notify_gateway_approval("session-interrupt", "run-interrupt", "request-interrupt", True) +pool._cancel_pending_approvals_for_generation("session-interrupt", "run-interrupt") +resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] +assert len(resolved_events) == 1, resolved_events +assert "resolved" not in resolved_events[0], resolved_events[0] +assert resolved_events[0]["reason"] == "Session interrupted", resolved_events[0] +assert resolved_events[0]["choice"] == "deny", resolved_events[0] +`) + }) + it('remembers execute_code approvals inside the bridge without patching upstream files', () => { runPython(String.raw` ${harness} diff --git a/tests/server/run-chat-bridge-final-context.test.ts b/tests/server/run-chat-bridge-final-context.test.ts index f10580fd61..5c867c12cf 100644 --- a/tests/server/run-chat-bridge-final-context.test.ts +++ b/tests/server/run-chat-bridge-final-context.test.ts @@ -2202,4 +2202,64 @@ describe('bridge run final context usage', () => { error: expect.stringContaining('origin context is unavailable'), })) }) + + it.each([ + { name: 'a failed gateway resolution', resolved: false }, + { name: 'a successful gateway resolution', resolved: true }, + ])('forwards the bridge approval outcome on a live run for $name', async ({ resolved }) => { + const emit = vi.fn() + const nsp = makeNamespace(emit) + const socket = makeSocket() + const state = makeState() + const sessionMap = new Map([['session-1', state]]) + const bridge = { + chat: vi.fn().mockResolvedValue({ run_id: 'run-approval', status: 'started' }), + contextEstimate: vi.fn().mockResolvedValue({ + token_count: 100, + fixed_context_tokens: 80, + message_count: 0, + tool_count: 0, + system_prompt_chars: 13, + }), + streamOutput: vi.fn(async function* () { + yield { + run_id: 'run-approval', + done: true, + status: 'completed', + output: 'done', + events: [{ + event: 'approval.resolved', + run_id: 'run-approval', + approval_id: 'approval-live', + choice: 'once', + resolved, + }], + } + }), + } as any + + const { handleBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + await handleBridgeRun( + nsp, + socket, + { input: 'hello', session_id: 'session-1' }, + 'research', + sessionMap, + bridge, + false, + vi.fn(), + vi.fn(), + ) + + // handleBridgeRun is the live-run call surface of applyBridgeChunkAsync; + // resumeBridgeRun covers the reconnect one. + const approvalResolved = emit.mock.calls.filter(call => call[0] === 'approval.resolved') + expect(approvalResolved).toHaveLength(1) + expect(approvalResolved[0][1]).toMatchObject({ + approval_id: 'approval-live', + choice: 'once', + resolved, + }) + }) + }) diff --git a/tests/server/run-chat-bridge-resume.test.ts b/tests/server/run-chat-bridge-resume.test.ts index 1a6e14a8c9..e71a9e6038 100644 --- a/tests/server/run-chat-bridge-resume.test.ts +++ b/tests/server/run-chat-bridge-resume.test.ts @@ -456,6 +456,78 @@ describe('resumeBridgeRun', () => { expect('resolved' in approvalResolved[0].payload).toBe(false) }) + it.each([ + { name: 'null', resolved: null }, + { name: 'the number 0', resolved: 0 }, + { name: 'an empty string', resolved: '' }, + { name: 'the string "true"', resolved: 'true' }, + ])('drops a non-boolean resolved value of $name rather than forwarding it (control)', async ({ resolved }) => { + const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + const { nsp, emitted } = createNamespace() + const socket = { id: 'socket-1', connected: true, emit: vi.fn() } + const sessionMap = new Map() + sessionMap.set('session-resume', { + messages: [{ id: 1, session_id: 'session-resume', role: 'user', content: 'hello', timestamp: 1 }], + isWorking: true, + events: [], + queue: [], + }) + + const bridge = { + getResult: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'running', + output: '', + deltas: [], + events: [], + })), + getOutput: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'complete', + delta: '', + cursor: 0, + output: '', + done: true, + result: { final_response: '' }, + error: null, + events: [{ + event: 'approval.resolved', + run_id: 'run-resume', + approval_id: 'approval-odd', + choice: 'once', + resolved, + }], + event_cursor: 1, + })), + } + + await resumeBridgeRun( + nsp as any, + socket as any, + { + sessionId: 'session-resume', + runId: 'run-resume', + profile: 'default', + instructions: 'system prompt', + model: 'gpt-test', + provider: 'openai', + }, + sessionMap, + bridge as any, + vi.fn(), + ) + + // A non-boolean carries no outcome the client can trust: forwarding it would + // make `resolved === false` in the client store depend on the loose value. + const approvalResolved = emitted.filter(item => item.event === 'approval.resolved') + expect(approvalResolved).toHaveLength(1) + expect('resolved' in approvalResolved[0].payload).toBe(false) + }) + it('completes a timed-out abort when the resumed bridge run reaches a terminal state', async () => { const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') const { nsp, emitted } = createNamespace() From e70d31f3b793ccdeaa0d9fdbc988714b7afd5a67 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 21 Sep 2026 04:17:22 +0000 Subject: [PATCH 3/6] test: cover the approval outcome contract at the client boundary Drive the bridge forwarder over a runtime approval.resolved event and feed what it emits to the chat store, so a failed gateway resolution is shown to keep its approval card and reach the expiry notice. Add browser coverage for what the card does with each outcome the forwarder can send. --- .../bridge-approval-outcome-contract.test.ts | 303 ++++++++++++++++++ tests/e2e/chat-streaming.spec.ts | 128 ++++++++ 2 files changed, 431 insertions(+) create mode 100644 tests/client/bridge-approval-outcome-contract.test.ts diff --git a/tests/client/bridge-approval-outcome-contract.test.ts b/tests/client/bridge-approval-outcome-contract.test.ts new file mode 100644 index 0000000000..2c6b0a7fe7 --- /dev/null +++ b/tests/client/bridge-approval-outcome-contract.test.ts @@ -0,0 +1,303 @@ +// @vitest-environment jsdom +// The bridge forwarder decides whether the Web UI can tell a failed approval +// resolution from an older runtime that reports no outcome at all. These cases +// drive the real forwarder and feed whatever it emits to the real chat store, +// so the server payload and the client card are checked against each other +// rather than against a hand-written fixture. +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { createPinia, setActivePinia } from 'pinia' + +const chatApi = vi.hoisted(() => ({ + startRunViaSocket: vi.fn(), + resumeSession: vi.fn(), + registerSessionHandlers: vi.fn(), + unregisterSessionHandlers: vi.fn(), + globalPendingHandler: undefined as undefined | ((event: any) => void), +})) + +vi.mock('@/api/studio/chat', () => ({ + startRunViaSocket: chatApi.startRunViaSocket, + resumeSession: chatApi.resumeSession, + registerSessionHandlers: chatApi.registerSessionHandlers, + unregisterSessionHandlers: chatApi.unregisterSessionHandlers, + getChatRunSocket: vi.fn(() => ({ emit: vi.fn() })), + respondToolApproval: vi.fn(), + respondClarify: vi.fn(), + onPeerUserMessage: vi.fn((handler: (event: any) => void) => { chatApi.globalPendingHandler = handler; return vi.fn() }), + onSessionCommand: vi.fn(() => vi.fn()), + onSessionTitleUpdated: vi.fn(() => vi.fn()), + onSessionWorkspaceUpdated: vi.fn(() => vi.fn()), + onSessionSettingsUpdated: vi.fn(() => vi.fn()), +})) + +vi.mock('@/api/client', () => ({ + getActiveProfileName: () => 'default', + hasApiKey: () => false, +})) + +vi.mock('@/api/studio/sessions', () => ({ + archiveSession: vi.fn(), + deleteSession: vi.fn(), + fetchSession: vi.fn(), + fetchSessions: vi.fn(), + fetchWorkspaceRunChangesForSession: vi.fn(async () => []), + fetchWorkspaceRunChangeFile: vi.fn(async () => null), + setSessionModel: vi.fn(), +})) + +vi.mock('@/api/studio/download', () => ({ + getDownloadUrl: (_path: string, name: string) => `/download/${name}`, +})) + +vi.mock('@/utils/completion-sound', () => ({ + primeCompletionSound: vi.fn(), + playCompletionSound: vi.fn(), +})) + +vi.mock('../../packages/server/src/modules/studio/repositories/session-store', () => ({ + addMessage: vi.fn(() => 42), + createSession: vi.fn(), + getSession: vi.fn(() => ({ id: 'session-bridge', profile: 'default', model: 'gpt-test', provider: 'openai' })), + updateSession: vi.fn(), + updateSessionStats: vi.fn(), +})) + +vi.mock('../../packages/server/src/modules/studio/repositories/usage-store', () => ({ + updateUsage: vi.fn(), +})) + +vi.mock('../../packages/server/src/modules/studio/public/logging', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + bridgeLogger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, +})) + +vi.mock('../../packages/server/src/modules/studio/public/runs/prompt', () => ({ + getSystemPrompt: vi.fn(() => 'system prompt'), +})) + +vi.mock('../../packages/server/src/modules/studio/services/context-compressor', () => ({ + countTokens: vi.fn(() => 1), + SUMMARY_PREFIX: '[Summary] ', +})) + +vi.mock('../../packages/server/src/modules/studio/repositories/compression-snapshot', () => ({ + getCompressionSnapshot: vi.fn(), +})) + +vi.mock('../../packages/server/src/modules/studio/services/chat-run/compression', async () => { + const actual = await vi.importActual('../../packages/server/src/modules/studio/services/chat-run/compression') + return { + ...actual, + buildDbHistory: vi.fn(async () => []), + buildSnapshotAwareHistory: vi.fn(async () => []), + buildCompressedHistory: vi.fn(), + forceCompressBridgeHistory: vi.fn(), + } +}) + +vi.mock('../../packages/server/src/modules/studio/services/chat-run/usage', () => ({ + calcAndUpdateUsage: vi.fn(async () => ({ inputTokens: 3, outputTokens: 2 })), + contextTokensWithCachedOverhead: vi.fn((_state: any, tokens: number) => tokens), + estimateUsageTokensFromMessages: vi.fn(() => ({ inputTokens: 3, outputTokens: 2 })), + getCachedBridgeContextOverhead: vi.fn(() => undefined), + updateMessageContextTokenUsage: vi.fn((_sid: any, state: any, _emit: any, tokens: number) => { + state.contextTokens = tokens + return tokens + }), +})) + +import { useChatStore, type Session } from '@/stores/hermes/chat' + +const SESSION_ID = 'session-bridge' +const APPROVAL_ID = 'approval-bridge' + +function makeSession(): Session { + return { + id: SESSION_ID, + title: SESSION_ID, + messages: [], + createdAt: Date.now(), + updatedAt: Date.now(), + } +} + +function createNamespace() { + const emitted: Array<{ event: string; payload: any }> = [] + return { + emitted, + nsp: { + adapter: { rooms: { get: vi.fn(() => new Set(['socket-1'])) } }, + to: vi.fn(() => ({ + emit: vi.fn((event: string, payload: any) => emitted.push({ event, payload })), + })), + }, + } +} + +// Drives the real bridge forwarder over one runtime approval.resolved event and +// returns the approval.resolved payload it broadcast to the room. +async function forwardRuntimeApproval(runtimeEvent: Record) { + const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + const { nsp, emitted } = createNamespace() + const sessionMap = new Map() + sessionMap.set(SESSION_ID, { + messages: [{ id: 1, session_id: SESSION_ID, role: 'user', content: 'hello', timestamp: 1 }], + isWorking: true, + events: [], + queue: [], + }) + + const bridge = { + getResult: vi.fn(async () => ({ + ok: true, + run_id: 'run-bridge', + session_id: SESSION_ID, + status: 'running', + output: '', + deltas: [], + events: [], + })), + getOutput: vi.fn(async () => ({ + ok: true, + run_id: 'run-bridge', + session_id: SESSION_ID, + status: 'complete', + delta: '', + cursor: 0, + output: '', + done: true, + result: { final_response: '' }, + error: null, + events: [runtimeEvent], + event_cursor: 1, + })), + } + + await resumeBridgeRun( + nsp as any, + { id: 'socket-1', connected: true, emit: vi.fn() } as any, + { + sessionId: SESSION_ID, + runId: 'run-bridge', + profile: 'default', + instructions: 'system prompt', + model: 'gpt-test', + provider: 'openai', + }, + sessionMap, + bridge as any, + vi.fn(), + ) + + const approvals = emitted.filter(item => item.event === 'approval.resolved') + expect(approvals).toHaveLength(1) + return approvals[0].payload +} + +describe('bridge approval outcome reaches the approval card', () => { + beforeEach(() => { + vi.clearAllMocks() + chatApi.globalPendingHandler = undefined + setActivePinia(createPinia()) + chatApi.startRunViaSocket.mockReturnValue({ abort: vi.fn() }) + }) + + function storeWithPendingApproval() { + const store = useChatStore() + const session = makeSession() + store.sessions = [session] + store.activeSessionId = session.id + store.activeSession = session + chatApi.globalPendingHandler?.({ + event: 'approval.requested', + session_id: SESSION_ID, + approval_id: APPROVAL_ID, + command: 'write_file /tmp/gateway.txt', + description: 'Allow write_file to create /tmp/gateway.txt', + choices: ['once', 'deny'], + timeout_ms: 300_000, + }) + expect(store.pendingApprovals.has(SESSION_ID)).toBe(true) + return store + } + + it('keeps the approval card when a gateway resolution failed', async () => { + const store = storeWithPendingApproval() + + const payload = await forwardRuntimeApproval({ + event: 'approval.resolved', + run_id: 'run-bridge', + approval_id: APPROVAL_ID, + choice: 'once', + resolved: false, + }) + chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID }) + + // The card staying up is the user-visible outcome, so it is asserted first: + // on the unfixed forwarder the card is gone by this point. + expect(store.pendingApprovals.get(SESSION_ID)).toMatchObject({ approvalId: APPROVAL_ID }) + expect(payload.resolved).toBe(false) + }) + + it('reports the expiry when a failed gateway resolution is also stale', async () => { + const store = storeWithPendingApproval() + const expired = vi.fn() + window.addEventListener('hermes:pending-interaction-expired', expired) + expect(store.respondApprovalFor(SESSION_ID, APPROVAL_ID, 'once')).toBe('submitted') + + const payload = await forwardRuntimeApproval({ + event: 'approval.resolved', + run_id: 'run-bridge', + approval_id: APPROVAL_ID, + choice: 'once', + resolved: false, + }) + chatApi.globalPendingHandler?.({ + ...payload, + event: 'approval.resolved', + session_id: SESSION_ID, + stale: true, + error: 'Approval is no longer pending.', + }) + + // Without the outcome the card is dropped silently: it disappears either + // way, but only the fixed forwarder reaches the expiry notice. + expect(expired).toHaveBeenCalledTimes(1) + expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) + expect(payload.resolved).toBe(false) + window.removeEventListener('hermes:pending-interaction-expired', expired) + }) + + it('publishes a successful gateway resolution and dismisses the approval card', async () => { + const store = storeWithPendingApproval() + + const payload = await forwardRuntimeApproval({ + event: 'approval.resolved', + run_id: 'run-bridge', + approval_id: APPROVAL_ID, + choice: 'once', + resolved: true, + }) + chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID }) + + // The card is dismissed with or without the fix here; the forwarded true is + // what discriminates, so it is asserted first. + expect(payload.resolved).toBe(true) + expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) + }) + + it('dismisses the approval card for an older runtime that reports no outcome (control)', async () => { + const store = storeWithPendingApproval() + + const payload = await forwardRuntimeApproval({ + event: 'approval.resolved', + run_id: 'run-bridge', + approval_id: APPROVAL_ID, + choice: 'once', + }) + chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID }) + + expect('resolved' in payload).toBe(false) + expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) + }) +}) diff --git a/tests/e2e/chat-streaming.spec.ts b/tests/e2e/chat-streaming.spec.ts index aebbb576f8..bb41f1f545 100644 --- a/tests/e2e/chat-streaming.spec.ts +++ b/tests/e2e/chat-streaming.spec.ts @@ -1603,3 +1603,131 @@ test('restores named resumed tool traces from assistant tool calls after session await expect(page.locator('.message.tool .tool-details')).toContainText('/tmp/history.txt') expect(api.unexpectedRequests).toEqual([]) }) + +// Browser-side contract for the approval outcome. The mocked chat socket feeds +// the card directly, so these hold with or without the bridge forwarder fix and +// are controls for it: they pin what the Web UI must do with each outcome the +// forwarder can send, while tests/client/bridge-approval-outcome-contract.test.ts +// pins that the forwarder actually sends it. +test('keeps the approval card in the browser when the runtime reports a failed resolution (control)', async ({ page }) => { + await authenticate(page, TEST_ACCESS_KEY, 'research') + const api = await mockHermesApi(page) + await mockChatSocket(page) + + await page.goto('/#/hermes/chat') + + await sendChatMessage(page, 'Use write_file behind a gateway approval') + const { run } = await waitForRun(page) + + await page.evaluate((sid) => { + const socket = (window as any).__PW_CHAT_SOCKET__.latest + socket.__trigger('run.started', { event: 'run.started', session_id: sid, run_id: 'run-gateway' }) + socket.__trigger('approval.requested', { + event: 'approval.requested', + session_id: sid, + run_id: 'run-gateway', + approval_id: 'approval-gateway', + command: 'write_file /tmp/gateway.txt', + description: 'Allow write_file to create /tmp/gateway.txt', + choices: ['once', 'deny'], + timeout_ms: 300_000, + remaining_timeout_ms: 300_000, + }) + }, run.session_id) + + const prompt = page.locator('.approval-float-panel') + await expect(prompt).toContainText('Allow write_file to create /tmp/gateway.txt') + await page.getByRole('button', { name: 'Allow once' }).click() + await expect(prompt).toHaveCount(0) + + // The runtime failed to resolve the gateway request, so the decision never + // took effect and the card has to come back rather than stay dismissed. + await page.evaluate((sid) => { + const socket = (window as any).__PW_CHAT_SOCKET__.latest + socket.__trigger('approval.requested', { + event: 'approval.requested', + session_id: sid, + run_id: 'run-gateway', + approval_id: 'approval-gateway', + command: 'write_file /tmp/gateway.txt', + description: 'Allow write_file to create /tmp/gateway.txt', + choices: ['once', 'deny'], + timeout_ms: 300_000, + remaining_timeout_ms: 300_000, + }) + socket.__trigger('approval.resolved', { + event: 'approval.resolved', + session_id: sid, + run_id: 'run-gateway', + approval_id: 'approval-gateway', + choice: 'once', + resolved: false, + }) + }, run.session_id) + + await expect(prompt).toContainText('Allow write_file to create /tmp/gateway.txt') + await expect(page.getByRole('button', { name: 'Allow once' })).toBeVisible() + + // A stale failure is the expiry case: the card closes and the user is told. + await page.evaluate((sid) => { + const socket = (window as any).__PW_CHAT_SOCKET__.latest + socket.__trigger('approval.resolved', { + event: 'approval.resolved', + session_id: sid, + run_id: 'run-gateway', + approval_id: 'approval-gateway', + choice: 'once', + resolved: false, + stale: true, + error: 'Approval is no longer pending.', + }) + }, run.session_id) + await expect(prompt).toHaveCount(0) + + expect(api.unexpectedRequests).toEqual([]) +}) + +test('dismisses the approval card in the browser when the runtime reports a successful resolution (control)', async ({ page }) => { + await authenticate(page, TEST_ACCESS_KEY, 'research') + const api = await mockHermesApi(page) + await mockChatSocket(page) + + await page.goto('/#/hermes/chat') + + await sendChatMessage(page, 'Use write_file behind a resolved approval') + const { run } = await waitForRun(page) + + await page.evaluate((sid) => { + const socket = (window as any).__PW_CHAT_SOCKET__.latest + socket.__trigger('run.started', { event: 'run.started', session_id: sid, run_id: 'run-gateway-ok' }) + socket.__trigger('approval.requested', { + event: 'approval.requested', + session_id: sid, + run_id: 'run-gateway-ok', + approval_id: 'approval-gateway-ok', + command: 'write_file /tmp/gateway-ok.txt', + description: 'Allow write_file to create /tmp/gateway-ok.txt', + choices: ['once', 'deny'], + timeout_ms: 300_000, + remaining_timeout_ms: 300_000, + }) + }, run.session_id) + + const prompt = page.locator('.approval-float-panel') + await expect(prompt).toContainText('Allow write_file to create /tmp/gateway-ok.txt') + + await page.evaluate((sid) => { + const socket = (window as any).__PW_CHAT_SOCKET__.latest + socket.__trigger('approval.resolved', { + event: 'approval.resolved', + session_id: sid, + run_id: 'run-gateway-ok', + approval_id: 'approval-gateway-ok', + choice: 'once', + resolved: true, + }) + }, run.session_id) + + await expect(prompt).toHaveCount(0) + expect(api.unexpectedRequests).toEqual([]) +}) From 534cf3c9ec069dcc43b66cd6f5ef5957e70ddc6b Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Mon, 21 Sep 2026 08:56:17 +0000 Subject: [PATCH 4/6] test: cover the approval outcome on the submit-dismissed card path The chat view dismisses an approval card as soon as the user answers, so a failed gateway resolution arrives with nothing pending for the session and clearPendingApproval takes its no-pending early return. That branch notifies the user only when the event reports resolved false, which is what the forwarder now sends; add the failing case and a successful-resolution control for the same branch. --- .../bridge-approval-outcome-contract.test.ts | 67 ++++++++++++++++++- 1 file changed, 66 insertions(+), 1 deletion(-) diff --git a/tests/client/bridge-approval-outcome-contract.test.ts b/tests/client/bridge-approval-outcome-contract.test.ts index 2c6b0a7fe7..9393668c40 100644 --- a/tests/client/bridge-approval-outcome-contract.test.ts +++ b/tests/client/bridge-approval-outcome-contract.test.ts @@ -12,6 +12,7 @@ const chatApi = vi.hoisted(() => ({ resumeSession: vi.fn(), registerSessionHandlers: vi.fn(), unregisterSessionHandlers: vi.fn(), + respondToolApproval: vi.fn(), globalPendingHandler: undefined as undefined | ((event: any) => void), })) @@ -21,7 +22,7 @@ vi.mock('@/api/studio/chat', () => ({ registerSessionHandlers: chatApi.registerSessionHandlers, unregisterSessionHandlers: chatApi.unregisterSessionHandlers, getChatRunSocket: vi.fn(() => ({ emit: vi.fn() })), - respondToolApproval: vi.fn(), + respondToolApproval: chatApi.respondToolApproval, respondClarify: vi.fn(), onPeerUserMessage: vi.fn((handler: (event: any) => void) => { chatApi.globalPendingHandler = handler; return vi.fn() }), onSessionCommand: vi.fn(() => vi.fn()), @@ -300,4 +301,68 @@ describe('bridge approval outcome reaches the approval card', () => { expect('resolved' in payload).toBe(false) expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) }) + + // The chat view answers an approval through respondApproval(), which dismisses + // the card locally as soon as the choice is sent. The failed resolution then + // arrives with nothing pending for the session, so clearPendingApproval takes + // its `!current` early return — a branch that only notifies when the event + // says resolved === false, which is precisely what the forwarder now sends. + it('reports the expiry after the view already dismissed the card on submit', async () => { + const store = storeWithPendingApproval() + const expired = vi.fn() + window.addEventListener('hermes:pending-interaction-expired', expired) + + expect(store.respondApproval('once')).toBe('submitted') + expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) + + const payload = await forwardRuntimeApproval({ + event: 'approval.resolved', + run_id: 'run-bridge', + approval_id: APPROVAL_ID, + choice: 'once', + resolved: false, + }) + chatApi.globalPendingHandler?.({ + ...payload, + event: 'approval.resolved', + session_id: SESSION_ID, + stale: true, + error: 'Approval is no longer pending.', + }) + + // On the unfixed forwarder the payload carries no outcome, the early return + // notifies nobody, and the user is left believing the command was allowed. + expect(expired).toHaveBeenCalledTimes(1) + expect(payload.resolved).toBe(false) + window.removeEventListener('hermes:pending-interaction-expired', expired) + }) + + // Same early-return branch, successful resolution: there is nothing pending + // and nothing to tell the user, with or without the fix. + it('stays silent after a submit-dismissed card resolves successfully (control)', async () => { + const store = storeWithPendingApproval() + const expired = vi.fn() + window.addEventListener('hermes:pending-interaction-expired', expired) + + expect(store.respondApproval('once')).toBe('submitted') + + const payload = await forwardRuntimeApproval({ + event: 'approval.resolved', + run_id: 'run-bridge', + approval_id: APPROVAL_ID, + choice: 'once', + resolved: true, + }) + chatApi.globalPendingHandler?.({ + ...payload, + event: 'approval.resolved', + session_id: SESSION_ID, + stale: true, + error: 'Approval is no longer pending.', + }) + + expect(expired).not.toHaveBeenCalled() + expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) + window.removeEventListener('hermes:pending-interaction-expired', expired) + }) }) From 94a1c92fca4720a1b57f589190d5ea5701fc8e46 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Tue, 22 Sep 2026 12:37:44 +0000 Subject: [PATCH 5/6] test: name the approval outcome cases after their contracts The added cases carried patch-history narration: test names ending in (control), comments saying what holds with or without the change, and accounting of which assertion discriminates. Name each case after the behaviour it pins, fold the client and browser cases that differ only in outcome into tables, and keep only the comments that explain runtime setup. No assertion input is dropped. --- .../bridge-approval-outcome-contract.test.ts | 122 ++++-------------- tests/e2e/chat-streaming.spec.ts | 14 +- .../agent-bridge-python-concurrency.test.ts | 13 +- .../run-chat-bridge-final-context.test.ts | 2 - tests/server/run-chat-bridge-resume.test.ts | 4 +- 5 files changed, 33 insertions(+), 122 deletions(-) diff --git a/tests/client/bridge-approval-outcome-contract.test.ts b/tests/client/bridge-approval-outcome-contract.test.ts index 9393668c40..092b48554d 100644 --- a/tests/client/bridge-approval-outcome-contract.test.ts +++ b/tests/client/bridge-approval-outcome-contract.test.ts @@ -1,9 +1,7 @@ // @vitest-environment jsdom -// The bridge forwarder decides whether the Web UI can tell a failed approval -// resolution from an older runtime that reports no outcome at all. These cases -// drive the real forwarder and feed whatever it emits to the real chat store, -// so the server payload and the client card are checked against each other -// rather than against a hand-written fixture. +// These cases drive the real bridge forwarder and feed whatever it broadcasts +// into the real chat store, so the run-stream payload and the approval card are +// checked against each other rather than against a hand-written fixture. import { beforeEach, describe, expect, it, vi } from 'vitest' import { createPinia, setActivePinia } from 'pinia' @@ -111,6 +109,7 @@ import { useChatStore, type Session } from '@/stores/hermes/chat' const SESSION_ID = 'session-bridge' const APPROVAL_ID = 'approval-bridge' +const STALE = { stale: true, error: 'Approval is no longer pending.' } function makeSession(): Session { return { @@ -222,7 +221,11 @@ describe('bridge approval outcome reaches the approval card', () => { return store } - it('keeps the approval card when a gateway resolution failed', async () => { + it.each([ + { name: 'a failed gateway resolution', outcome: { resolved: false }, resolved: false, card: APPROVAL_ID }, + { name: 'a successful gateway resolution', outcome: { resolved: true }, resolved: true, card: undefined }, + { name: 'an older runtime that reports no outcome', outcome: {}, resolved: undefined, card: undefined }, + ])('forwards $name to the approval card', async ({ outcome, resolved, card }) => { const store = storeWithPendingApproval() const payload = await forwardRuntimeApproval({ @@ -230,14 +233,13 @@ describe('bridge approval outcome reaches the approval card', () => { run_id: 'run-bridge', approval_id: APPROVAL_ID, choice: 'once', - resolved: false, + ...outcome, }) chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID }) - // The card staying up is the user-visible outcome, so it is asserted first: - // on the unfixed forwarder the card is gone by this point. - expect(store.pendingApprovals.get(SESSION_ID)).toMatchObject({ approvalId: APPROVAL_ID }) - expect(payload.resolved).toBe(false) + expect(store.pendingApprovals.get(SESSION_ID)?.approvalId).toBe(card) + expect(payload.resolved).toBe(resolved) + expect('resolved' in payload).toBe(resolved !== undefined) }) it('reports the expiry when a failed gateway resolution is also stale', async () => { @@ -253,61 +255,22 @@ describe('bridge approval outcome reaches the approval card', () => { choice: 'once', resolved: false, }) - chatApi.globalPendingHandler?.({ - ...payload, - event: 'approval.resolved', - session_id: SESSION_ID, - stale: true, - error: 'Approval is no longer pending.', - }) + chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID, ...STALE }) - // Without the outcome the card is dropped silently: it disappears either - // way, but only the fixed forwarder reaches the expiry notice. expect(expired).toHaveBeenCalledTimes(1) expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) expect(payload.resolved).toBe(false) window.removeEventListener('hermes:pending-interaction-expired', expired) }) - it('publishes a successful gateway resolution and dismisses the approval card', async () => { - const store = storeWithPendingApproval() - - const payload = await forwardRuntimeApproval({ - event: 'approval.resolved', - run_id: 'run-bridge', - approval_id: APPROVAL_ID, - choice: 'once', - resolved: true, - }) - chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID }) - - // The card is dismissed with or without the fix here; the forwarded true is - // what discriminates, so it is asserted first. - expect(payload.resolved).toBe(true) - expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) - }) - - it('dismisses the approval card for an older runtime that reports no outcome (control)', async () => { - const store = storeWithPendingApproval() - - const payload = await forwardRuntimeApproval({ - event: 'approval.resolved', - run_id: 'run-bridge', - approval_id: APPROVAL_ID, - choice: 'once', - }) - chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID }) - - expect('resolved' in payload).toBe(false) - expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) - }) - // The chat view answers an approval through respondApproval(), which dismisses - // the card locally as soon as the choice is sent. The failed resolution then + // the card locally as soon as the choice is sent. A stale resolution then // arrives with nothing pending for the session, so clearPendingApproval takes - // its `!current` early return — a branch that only notifies when the event - // says resolved === false, which is precisely what the forwarder now sends. - it('reports the expiry after the view already dismissed the card on submit', async () => { + // its `!current` early return, which notifies only on resolved === false. + it.each([ + { name: 'a failed gateway resolution', resolved: false, expiries: 1 }, + { name: 'a successful gateway resolution', resolved: true, expiries: 0 }, + ])('reports $name to a card the view already dismissed on submit', async ({ resolved, expiries }) => { const store = storeWithPendingApproval() const expired = vi.fn() window.addEventListener('hermes:pending-interaction-expired', expired) @@ -320,49 +283,12 @@ describe('bridge approval outcome reaches the approval card', () => { run_id: 'run-bridge', approval_id: APPROVAL_ID, choice: 'once', - resolved: false, - }) - chatApi.globalPendingHandler?.({ - ...payload, - event: 'approval.resolved', - session_id: SESSION_ID, - stale: true, - error: 'Approval is no longer pending.', - }) - - // On the unfixed forwarder the payload carries no outcome, the early return - // notifies nobody, and the user is left believing the command was allowed. - expect(expired).toHaveBeenCalledTimes(1) - expect(payload.resolved).toBe(false) - window.removeEventListener('hermes:pending-interaction-expired', expired) - }) - - // Same early-return branch, successful resolution: there is nothing pending - // and nothing to tell the user, with or without the fix. - it('stays silent after a submit-dismissed card resolves successfully (control)', async () => { - const store = storeWithPendingApproval() - const expired = vi.fn() - window.addEventListener('hermes:pending-interaction-expired', expired) - - expect(store.respondApproval('once')).toBe('submitted') - - const payload = await forwardRuntimeApproval({ - event: 'approval.resolved', - run_id: 'run-bridge', - approval_id: APPROVAL_ID, - choice: 'once', - resolved: true, - }) - chatApi.globalPendingHandler?.({ - ...payload, - event: 'approval.resolved', - session_id: SESSION_ID, - stale: true, - error: 'Approval is no longer pending.', + resolved, }) + chatApi.globalPendingHandler?.({ ...payload, event: 'approval.resolved', session_id: SESSION_ID, ...STALE }) - expect(expired).not.toHaveBeenCalled() - expect(store.pendingApprovals.has(SESSION_ID)).toBe(false) + expect(expired).toHaveBeenCalledTimes(expiries) + expect(payload.resolved).toBe(resolved) window.removeEventListener('hermes:pending-interaction-expired', expired) }) }) diff --git a/tests/e2e/chat-streaming.spec.ts b/tests/e2e/chat-streaming.spec.ts index bb41f1f545..cebfe9754a 100644 --- a/tests/e2e/chat-streaming.spec.ts +++ b/tests/e2e/chat-streaming.spec.ts @@ -1604,12 +1604,7 @@ test('restores named resumed tool traces from assistant tool calls after session expect(api.unexpectedRequests).toEqual([]) }) -// Browser-side contract for the approval outcome. The mocked chat socket feeds -// the card directly, so these hold with or without the bridge forwarder fix and -// are controls for it: they pin what the Web UI must do with each outcome the -// forwarder can send, while tests/client/bridge-approval-outcome-contract.test.ts -// pins that the forwarder actually sends it. -test('keeps the approval card in the browser when the runtime reports a failed resolution (control)', async ({ page }) => { +test('keeps the approval card when the runtime reports a failed resolution', async ({ page }) => { await authenticate(page, TEST_ACCESS_KEY, 'research') const api = await mockHermesApi(page) await mockChatSocket(page) @@ -1640,8 +1635,8 @@ test('keeps the approval card in the browser when the runtime reports a failed r await page.getByRole('button', { name: 'Allow once' }).click() await expect(prompt).toHaveCount(0) - // The runtime failed to resolve the gateway request, so the decision never - // took effect and the card has to come back rather than stay dismissed. + // The runtime could not apply the decision, so it asks again before reporting + // the failed outcome for the first request. await page.evaluate((sid) => { const socket = (window as any).__PW_CHAT_SOCKET__.latest socket.__trigger('approval.requested', { @@ -1668,7 +1663,6 @@ test('keeps the approval card in the browser when the runtime reports a failed r await expect(prompt).toContainText('Allow write_file to create /tmp/gateway.txt') await expect(page.getByRole('button', { name: 'Allow once' })).toBeVisible() - // A stale failure is the expiry case: the card closes and the user is told. await page.evaluate((sid) => { const socket = (window as any).__PW_CHAT_SOCKET__.latest socket.__trigger('approval.resolved', { @@ -1687,7 +1681,7 @@ test('keeps the approval card in the browser when the runtime reports a failed r expect(api.unexpectedRequests).toEqual([]) }) -test('dismisses the approval card in the browser when the runtime reports a successful resolution (control)', async ({ page }) => { +test('dismisses the approval card when the runtime reports a successful resolution', async ({ page }) => { await authenticate(page, TEST_ACCESS_KEY, 'research') const api = await mockHermesApi(page) await mockChatSocket(page) diff --git a/tests/server/agent-bridge-python-concurrency.test.ts b/tests/server/agent-bridge-python-concurrency.test.ts index f8fbc4419f..9c87d29765 100644 --- a/tests/server/agent-bridge-python-concurrency.test.ts +++ b/tests/server/agent-bridge-python-concurrency.test.ts @@ -1387,7 +1387,6 @@ assert [(msg["role"], msg["content"]) for msg in messages] == [ ${harness} ${gatewayApprovalPrelude} -# A runtime that queued the request resolves, and the event says so. record, approval_id = notify_gateway_approval("session-ok", "run-ok", "request-ok", True) result = pool.respond_approval(approval_id, "once") assert result["resolved"] is True, result @@ -1403,7 +1402,6 @@ assert resolved_events[0]["choice"] == "once", resolved_events[0] ${harness} ${gatewayApprovalPrelude} -# A runtime that never registered the request does not resolve, and the event says so. record, approval_id = notify_gateway_approval("session-stale", "run-stale", "request-stale", False) result = pool.respond_approval(approval_id, "once") assert result["resolved"] is False, result @@ -1419,8 +1417,7 @@ assert resolved_events[0]["choice"] == "once", resolved_events[0] ${harness} ${gatewayApprovalPrelude} -# A runtime older than v0.20.5 emits no request_id at all: same honest outcome. -# The empty request_id short-circuits before resolve_gateway_approval is called. +# An empty request_id short-circuits before resolve_gateway_approval is reached. record, approval_id = notify_gateway_approval("session-legacy", "run-legacy", "", False) before = len(approval._resolved_gateway_requests) result = pool.respond_approval(approval_id, "deny") @@ -1438,7 +1435,6 @@ assert resolved_events[0]["choice"] == "deny", resolved_events[0] ${harness} ${gatewayApprovalPrelude} -# The except Exception branch: a gateway that raises must not be reported as resolved. def raising_resolve_gateway_approval(session_key, choice, request_id=None): raise RuntimeError("gateway unavailable") @@ -1453,14 +1449,13 @@ assert resolved_events[0]["resolved"] is False, resolved_events[0] `) }) - it('keeps reporting the interrupt-path approval.resolved event without an outcome (control)', () => { + it('omits the outcome on the session-interrupt approval.resolved event', () => { runPython(String.raw` ${harness} ${gatewayApprovalPrelude} -# Control for the respond_approval branch only: the session-interrupt path -# resolves gateway approvals of its own and is untouched by the fix, so its -# event carries a reason and still no resolved key on either arm. +# The interrupt path resolves gateway approvals of its own and reports a reason +# instead of an outcome. record, approval_id = notify_gateway_approval("session-interrupt", "run-interrupt", "request-interrupt", True) pool._cancel_pending_approvals_for_generation("session-interrupt", "run-interrupt") resolved_events = [event for event in record.events if event["event"] == "approval.resolved"] diff --git a/tests/server/run-chat-bridge-final-context.test.ts b/tests/server/run-chat-bridge-final-context.test.ts index 5c867c12cf..272b950102 100644 --- a/tests/server/run-chat-bridge-final-context.test.ts +++ b/tests/server/run-chat-bridge-final-context.test.ts @@ -2251,8 +2251,6 @@ describe('bridge run final context usage', () => { vi.fn(), ) - // handleBridgeRun is the live-run call surface of applyBridgeChunkAsync; - // resumeBridgeRun covers the reconnect one. const approvalResolved = emit.mock.calls.filter(call => call[0] === 'approval.resolved') expect(approvalResolved).toHaveLength(1) expect(approvalResolved[0][1]).toMatchObject({ diff --git a/tests/server/run-chat-bridge-resume.test.ts b/tests/server/run-chat-bridge-resume.test.ts index e71a9e6038..b37acfabcc 100644 --- a/tests/server/run-chat-bridge-resume.test.ts +++ b/tests/server/run-chat-bridge-resume.test.ts @@ -461,7 +461,7 @@ describe('resumeBridgeRun', () => { { name: 'the number 0', resolved: 0 }, { name: 'an empty string', resolved: '' }, { name: 'the string "true"', resolved: 'true' }, - ])('drops a non-boolean resolved value of $name rather than forwarding it (control)', async ({ resolved }) => { + ])('drops a non-boolean resolved value of $name rather than forwarding it', async ({ resolved }) => { const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') const { nsp, emitted } = createNamespace() const socket = { id: 'socket-1', connected: true, emit: vi.fn() } @@ -521,8 +521,6 @@ describe('resumeBridgeRun', () => { vi.fn(), ) - // A non-boolean carries no outcome the client can trust: forwarding it would - // make `resolved === false` in the client store depend on the loose value. const approvalResolved = emitted.filter(item => item.event === 'approval.resolved') expect(approvalResolved).toHaveLength(1) expect('resolved' in approvalResolved[0].payload).toBe(false) From ca908be29b6ff7716fc9ccde837e0fb9f6340a35 Mon Sep 17 00:00:00 2001 From: askalf <263217947+askalf@users.noreply.github.com> Date: Thu, 24 Sep 2026 00:55:35 +0000 Subject: [PATCH 6/6] test: cover the approval outcome on the run event observer and replayed state --- .../run-chat-bridge-final-context.test.ts | 57 ++++++++++++++ tests/server/run-chat-bridge-resume.test.ts | 77 +++++++++++++++++++ 2 files changed, 134 insertions(+) diff --git a/tests/server/run-chat-bridge-final-context.test.ts b/tests/server/run-chat-bridge-final-context.test.ts index 272b950102..5725ea46d6 100644 --- a/tests/server/run-chat-bridge-final-context.test.ts +++ b/tests/server/run-chat-bridge-final-context.test.ts @@ -2260,4 +2260,61 @@ describe('bridge run final context usage', () => { }) }) + it.each([ + { name: 'a failed gateway resolution', resolved: false }, + { name: 'a successful gateway resolution', resolved: true }, + ])('keeps the bridge approval outcome in the replayed run state for $name', async ({ resolved }) => { + const emit = vi.fn() + const nsp = makeNamespace(emit) + const socket = makeSocket() + const state = makeState() + const sessionMap = new Map([['session-1', state]]) + const bridge = { + chat: vi.fn().mockResolvedValue({ run_id: 'run-approval', status: 'started' }), + contextEstimate: vi.fn().mockResolvedValue({ + token_count: 100, + fixed_context_tokens: 80, + message_count: 0, + tool_count: 0, + system_prompt_chars: 13, + }), + streamOutput: vi.fn(async function* () { + yield { + run_id: 'run-approval', + done: true, + status: 'completed', + output: 'done', + events: [{ + event: 'approval.resolved', + run_id: 'run-approval', + approval_id: 'approval-replayed', + choice: 'once', + resolved, + }], + } + }), + } as any + + const { handleBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + await handleBridgeRun( + nsp, + socket, + { input: 'hello', session_id: 'session-1' }, + 'research', + sessionMap, + bridge, + false, + vi.fn(), + vi.fn(), + ) + + const replayed = replaceStateMock.mock.calls.filter(call => call[2] === 'approval.resolved') + expect(replayed).toHaveLength(1) + expect(replayed[0][3]).toMatchObject({ + approval_id: 'approval-replayed', + choice: 'once', + resolved, + }) + }) + }) diff --git a/tests/server/run-chat-bridge-resume.test.ts b/tests/server/run-chat-bridge-resume.test.ts index b37acfabcc..e6cd8b1f3b 100644 --- a/tests/server/run-chat-bridge-resume.test.ts +++ b/tests/server/run-chat-bridge-resume.test.ts @@ -1,4 +1,5 @@ import { beforeEach, describe, expect, it, vi } from 'vitest' +import { foregroundNotification } from '../../packages/server/src/modules/studio/services/chat-run/foreground-notification' const addMessageMock = vi.fn() const updateSessionStatsMock = vi.fn() @@ -391,6 +392,82 @@ describe('resumeBridgeRun', () => { }) }) + it.each([ + { name: 'a failed gateway resolution', resolved: false, notified: false }, + { name: 'a successful gateway resolution', resolved: true, notified: true }, + ])('reports the bridge approval outcome to the run event observer for $name', async ({ resolved, notified }) => { + const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') + const { nsp } = createNamespace() + const socket = { id: 'socket-1', connected: true, emit: vi.fn() } + const sessionMap = new Map() + const onEvent = vi.fn() + sessionMap.set('session-resume', { + messages: [{ id: 1, session_id: 'session-resume', role: 'user', content: 'hello', timestamp: 1 }], + isWorking: true, + events: [], + queue: [], + }) + + const bridge = { + getResult: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'running', + output: '', + deltas: [], + events: [], + })), + getOutput: vi.fn(async () => ({ + ok: true, + run_id: 'run-resume', + session_id: 'session-resume', + status: 'complete', + delta: '', + cursor: 0, + output: '', + done: true, + result: { final_response: '' }, + error: null, + events: [{ + event: 'approval.resolved', + run_id: 'run-resume', + approval_id: 'approval-observed', + choice: 'once', + resolved, + }], + event_cursor: 1, + })), + } + + await resumeBridgeRun( + nsp as any, + socket as any, + { + sessionId: 'session-resume', + runId: 'run-resume', + profile: 'default', + instructions: 'system prompt', + model: 'gpt-test', + provider: 'openai', + onEvent, + }, + sessionMap, + bridge as any, + vi.fn(), + ) + + const observed = onEvent.mock.calls.filter(call => call[0] === 'approval.resolved') + expect(observed).toHaveLength(1) + expect(observed[0][1]).toMatchObject({ + session_id: 'session-resume', + approval_id: 'approval-observed', + choice: 'once', + resolved, + }) + expect(foregroundNotification('approval.resolved', observed[0][1], 10) !== null).toBe(notified) + }) + it('omits resolved when an older bridge runtime does not report an approval outcome', async () => { const { resumeBridgeRun } = await import('../../packages/server/src/modules/studio/services/chat-run/handle-bridge-run') const { nsp, emitted } = createNamespace()