From 3029db6e103860ef28365596393a1eb5a93142f1 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" Date: Sun, 10 May 2026 01:55:53 +0000 Subject: [PATCH 1/2] refactor(host-browser): remove dead WS resolveResult path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The chrome extension migrated to HTTP POST in PR #29829. There is no /v1/browser-relay WebSocket endpoint in the current runtime — no WS handler calls HostBrowserProxy.resolveResult. Removing the public method eliminates the foot-gun of a future WS handler bypassing the kind-check and same-actor guard that the HTTP route enforces. The route now inlines pendingInteractions.resolve() + rpcResolve directly. Tests are updated to use a local resolveResult helper that mirrors the same call sequence. Co-Authored-By: Claude Sonnet 4.6 --- .../src/__tests__/host-browser-proxy.test.ts | 37 ++++++++---- .../src/__tests__/host-browser-routes.test.ts | 60 ++++++------------- assistant/src/daemon/host-browser-proxy.ts | 21 ------- .../src/runtime/routes/host-browser-routes.ts | 16 +---- 4 files changed, 45 insertions(+), 89 deletions(-) diff --git a/assistant/src/__tests__/host-browser-proxy.test.ts b/assistant/src/__tests__/host-browser-proxy.test.ts index d9e5b779153..883a03be91d 100644 --- a/assistant/src/__tests__/host-browser-proxy.test.ts +++ b/assistant/src/__tests__/host-browser-proxy.test.ts @@ -63,6 +63,19 @@ function getPublishedMessages(): unknown[] { return publishedEvents; } +/** + * Simulate the HTTP route resolving a host_browser result. Mirrors what + * `resolveHostBrowserResultByRequestId` does after its guards pass: consume + * the pending interaction and invoke `rpcResolve` with the response. + */ +function resolveResult( + requestId: string, + response: { content: string; isError: boolean }, +): void { + const interaction = pendingInteractions.resolve(requestId); + interaction?.rpcResolve?.(response); +} + // ── Tests ──────────────────────────────────────────────────────────── describe("HostBrowserProxy", () => { @@ -103,7 +116,7 @@ describe("HostBrowserProxy", () => { const requestId = sent.requestId as string; expect(pendingInteractions.get(requestId)).toBeDefined(); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); const result = await resultPromise; expect(result.content).toBe("ok"); @@ -131,7 +144,7 @@ describe("HostBrowserProxy", () => { }); expect(sent.cdpSessionId).toBe("session-abc"); - proxy.resolveResult(sent.requestId as string, { + resolveResult(sent.requestId as string, { content: "Example Domain", isError: false, }); @@ -146,7 +159,7 @@ describe("HostBrowserProxy", () => { ); const sent = getPublishedMessages()[0] as Record; - proxy.resolveResult(sent.requestId as string, { + resolveResult(sent.requestId as string, { content: "Navigation failed", isError: true, }); @@ -168,7 +181,7 @@ describe("HostBrowserProxy", () => { const requestId = sent.requestId as string; expect(pendingInteractions.get(requestId)).toBeDefined(); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); expect(pendingInteractions.get(requestId)).toBeUndefined(); await resultPromise; @@ -293,7 +306,7 @@ describe("HostBrowserProxy", () => { describe("resolve with unknown requestId", () => { test("silently ignores unknown requestId", () => { - proxy.resolveResult("nonexistent", { content: "stale", isError: false }); + resolveResult("nonexistent", { content: "stale", isError: false }); }); }); @@ -365,7 +378,7 @@ describe("HostBrowserProxy", () => { const requestId = (getPublishedMessages()[0] as Record) .requestId as string; - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); await resultPromise; expect(spy.removeCalls).toEqual(["abort"]); @@ -438,7 +451,7 @@ describe("HostBrowserProxy", () => { expect(pending?.targetClientId).toBe("ext-client"); expect(pending?.targetActorPrincipalId).toBe("user-1"); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); const result = await resultPromise; expect(result.isError).toBe(false); }); @@ -499,7 +512,7 @@ describe("HostBrowserProxy", () => { const pending = pendingInteractions.get(requestId); expect(pending?.targetClientId).toBe("ext-client"); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); await resultPromise; }); @@ -528,7 +541,7 @@ describe("HostBrowserProxy", () => { expect(pending?.targetClientId).toBe("macos-client"); expect(pending?.targetActorPrincipalId).toBe("user-1"); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); await resultPromise; }); @@ -555,7 +568,7 @@ describe("HostBrowserProxy", () => { expect(pending?.targetClientId).toBe("test-client"); expect(pending?.targetActorPrincipalId).toBeUndefined(); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); await resultPromise; }); @@ -629,7 +642,7 @@ describe("HostBrowserProxy", () => { const pending = pendingInteractions.get(requestId); expect(pending?.targetClientId).toBe("macos-client"); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); const result = await resultPromise; expect(result.isError).toBe(false); }); @@ -739,7 +752,7 @@ describe("HostBrowserProxy", () => { const pending = pendingInteractions.get(requestId); expect(pending?.targetClientId).toBe("ext-client"); - proxy.resolveResult(requestId, { content: "ok", isError: false }); + resolveResult(requestId, { content: "ok", isError: false }); await resultPromise; }); }); diff --git a/assistant/src/__tests__/host-browser-routes.test.ts b/assistant/src/__tests__/host-browser-routes.test.ts index d09942cc677..0aae7752019 100644 --- a/assistant/src/__tests__/host-browser-routes.test.ts +++ b/assistant/src/__tests__/host-browser-routes.test.ts @@ -1,9 +1,9 @@ /** * Unit tests for the /v1/host-browser-result route handler. * - * Resolution goes through HostBrowserProxy.instance (singleton). The - * mock below spies on resolveResult; the real pendingInteractions - * module provides the guard check for unknown request IDs. + * The route inlines pendingInteractions resolution directly — no proxy + * singleton is involved. The real pendingInteractions module provides both + * the guard check for unknown request IDs and the rpcResolve invocation. */ import { afterAll, beforeEach, describe, expect, mock, test } from "bun:test"; @@ -20,28 +20,6 @@ mock.module("../runtime/local-actor-identity.js", () => ({ resolveActorPrincipalIdForLocalGuardian: (raw: string | undefined) => raw, })); -interface ResolveCall { - requestId: string; - response: { content: string; isError: boolean }; -} - -const resolveSpy: ResolveCall[] = []; - -mock.module("../daemon/host-browser-proxy.js", () => ({ - HostBrowserProxy: { - get instance() { - return { - resolveResult( - requestId: string, - response: { content: string; isError: boolean }, - ) { - resolveSpy.push({ requestId, response }); - }, - }; - }, - }, -})); - // Use the real pending-interactions module for the guard check. const pendingInteractions = await import("../runtime/pending-interactions.js"); @@ -68,14 +46,15 @@ const handleHostBrowserResult = ROUTES.find( describe("handleHostBrowserResult", () => { beforeEach(() => { pendingInteractions.clear(); - resolveSpy.length = 0; }); - test("happy path: resolves a pending host_browser request via singleton", async () => { + test("happy path: resolves a pending host_browser request", async () => { const requestId = "browser-req-happy"; + const resolved: unknown[] = []; pendingInteractions.register(requestId, { conversationId: "conv-1", kind: "host_browser", + rpcResolve: (v: unknown) => resolved.push(v), }); const result = await handleHostBrowserResult({ @@ -83,9 +62,9 @@ describe("handleHostBrowserResult", () => { }); expect(result).toEqual({ accepted: true }); - expect(resolveSpy).toHaveLength(1); - expect(resolveSpy[0].requestId).toBe(requestId); - expect(resolveSpy[0].response).toEqual({ content: "ok", isError: false }); + expect(pendingInteractions.get(requestId)).toBeUndefined(); + expect(resolved).toHaveLength(1); + expect(resolved[0]).toEqual({ content: "ok", isError: false }); }); test("missing body: throws BadRequestError", () => { @@ -125,23 +104,23 @@ describe("handleHostBrowserResult", () => { // Interaction must NOT have been consumed — the bash proxy can still resolve it expect(pendingInteractions.get(requestId)).toBeDefined(); - // resolveResult should never have been called - expect(resolveSpy).toHaveLength(0); }); test("defaults: missing content/isError default to '' and false", async () => { const requestId = "browser-req-defaults"; + const resolved: unknown[] = []; pendingInteractions.register(requestId, { conversationId: "conv-1", kind: "host_browser", + rpcResolve: (v: unknown) => resolved.push(v), }); const result = await handleHostBrowserResult({ body: { requestId } }); expect(result).toEqual({ accepted: true }); - expect(resolveSpy).toHaveLength(1); - expect(resolveSpy[0].requestId).toBe(requestId); - expect(resolveSpy[0].response).toEqual({ content: "", isError: false }); + expect(pendingInteractions.get(requestId)).toBeUndefined(); + expect(resolved).toHaveLength(1); + expect(resolved[0]).toEqual({ content: "", isError: false }); }); }); @@ -150,7 +129,6 @@ describe("handleHostBrowserResult", () => { describe("handleHostBrowserResult — same-actor guard", () => { beforeEach(() => { pendingInteractions.clear(); - resolveSpy.length = 0; }); // ── Targeted + correct headers → 200 ───────────────────────────────── @@ -174,8 +152,7 @@ describe("handleHostBrowserResult — same-actor guard", () => { }); expect(result).toEqual({ accepted: true }); - expect(resolveSpy).toHaveLength(1); - expect(resolveSpy[0].requestId).toBe(requestId); + expect(pendingInteractions.get(requestId)).toBeUndefined(); }); test("trims whitespace from x-vellum-client-id before comparing", async () => { @@ -253,7 +230,6 @@ describe("handleHostBrowserResult — same-actor guard", () => { } expect(pendingInteractions.get(requestId)).toBeDefined(); - expect(resolveSpy).toHaveLength(0); }); }); @@ -330,7 +306,6 @@ describe("handleHostBrowserResult — same-actor guard", () => { } expect(pendingInteractions.get(requestId)).toBeDefined(); - expect(resolveSpy).toHaveLength(0); }); }); @@ -416,7 +391,6 @@ describe("handleHostBrowserResult — same-actor guard", () => { } expect(pendingInteractions.get(requestId)).toBeDefined(); - expect(resolveSpy).toHaveLength(0); }); }); @@ -435,7 +409,7 @@ describe("handleHostBrowserResult — same-actor guard", () => { }); expect(result).toEqual({ accepted: true }); - expect(resolveSpy).toHaveLength(1); + expect(pendingInteractions.get(requestId)).toBeUndefined(); }); test("accepts when header is present (header ignored when untargeted)", async () => { @@ -451,7 +425,7 @@ describe("handleHostBrowserResult — same-actor guard", () => { }); expect(result).toEqual({ accepted: true }); - expect(resolveSpy).toHaveLength(1); + expect(pendingInteractions.get(requestId)).toBeUndefined(); }); }); }); diff --git a/assistant/src/daemon/host-browser-proxy.ts b/assistant/src/daemon/host-browser-proxy.ts index e35480a961e..3b9447cb4d4 100644 --- a/assistant/src/daemon/host-browser-proxy.ts +++ b/assistant/src/daemon/host-browser-proxy.ts @@ -270,27 +270,6 @@ export class HostBrowserProxy { }); } - /** - * Process a client result and resolve the RPC. Called by route handlers. - */ - resolveResult( - requestId: string, - response: { content: string; isError: boolean }, - ): void { - const interaction = pendingInteractions.resolve(requestId); - if (!interaction?.rpcResolve) { - log.debug( - { requestId }, - "Ignoring host_browser_result for unknown or already-resolved request", - ); - return; - } - interaction.rpcResolve({ - content: response.content, - isError: response.isError, - }); - } - dispose(): void { for (const entry of pendingInteractions.getByKind("host_browser")) { pendingInteractions.resolve(entry.requestId); diff --git a/assistant/src/runtime/routes/host-browser-routes.ts b/assistant/src/runtime/routes/host-browser-routes.ts index fef3353b1bc..807042113a6 100644 --- a/assistant/src/runtime/routes/host-browser-routes.ts +++ b/assistant/src/runtime/routes/host-browser-routes.ts @@ -10,7 +10,6 @@ import { markTargetInvalidated, publishCdpEvent, } from "../../browser-session/events.js"; -import { HostBrowserProxy } from "../../daemon/host-browser-proxy.js"; import { enforceSameActorOrThrow, SAME_ACTOR_FORBIDDEN_DESCRIPTION, @@ -26,9 +25,7 @@ import { import type { RouteDefinition, RouteHandlerArgs } from "./types.js"; /** - * Result of attempting to resolve a host browser result frame. Used by both - * the HTTP endpoint and the WS relay path so they share the same validation - * and resolution semantics. + * Result of attempting to resolve a host browser result frame. * * Success → the pending interaction was consumed and the conversation was * notified. @@ -58,13 +55,6 @@ export type HostBrowserResultResolution = * client at registration time. Mirrors the host-cu / host-bash result * routes. * - * NOTE: The WebSocket `host_browser_result` frame path does NOT go - * through this function — it is handled by `HostBrowserProxy.resolveResult` - * directly, which only consults `pendingInteractions` and does not - * currently perform a kind check. That asymmetry is pre-existing; if - * the WS path is ever opened to less-trusted clients, it should adopt - * the same kind-check + actor-binding guards added here. - * * This function does NOT perform auth — callers are expected to have * already authenticated the caller (the HTTP route uses * `requireBoundGuardian`). @@ -159,8 +149,8 @@ export function resolveHostBrowserResultByRequestId( const normalizedContent = typeof content === "string" ? content : ""; const normalizedIsError = typeof isError === "boolean" ? isError : false; - const proxy = HostBrowserProxy.instance; - proxy.resolveResult(requestId, { + const interaction = pendingInteractions.resolve(requestId); + interaction?.rpcResolve?.({ content: normalizedContent, isError: normalizedIsError, }); From 6a6be58f6c92b6749fb5e47683ee7db7a86e1da1 Mon Sep 17 00:00:00 2001 From: "credence-the-bot[bot]" Date: Sun, 10 May 2026 01:56:00 +0000 Subject: [PATCH 2/2] test(host-proxy-preactivation): add cross-client drain-path coverage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #30154 enabled web/iOS turns to drive host_app_control and host_cu on a connected macOS client (cross-client routing). The existing drain-path tests cover macOS-native + chrome-extension + slack; this adds the missing cross-client cases: - web source + listClientsByCapability returns 1 macOS client → both app-control and computer-use skills re-added - web source + listClientsByCapability returns [] → neither re-added Introduces a per-test mockCapabilityClients variable (with afterEach reset) so hub state doesn't bleed across tests. Co-Authored-By: Claude Sonnet 4.6 --- ...-process-app-control-preactivation.test.ts | 95 ++++++++++++++++++- 1 file changed, 93 insertions(+), 2 deletions(-) diff --git a/assistant/src/__tests__/conversation-process-app-control-preactivation.test.ts b/assistant/src/__tests__/conversation-process-app-control-preactivation.test.ts index 4668c5c8bc5..b451dc99c9d 100644 --- a/assistant/src/__tests__/conversation-process-app-control-preactivation.test.ts +++ b/assistant/src/__tests__/conversation-process-app-control-preactivation.test.ts @@ -15,7 +15,7 @@ * instantiation block use at first-message time. */ -import { describe, expect, mock, test } from "bun:test"; +import { afterEach, describe, expect, mock, test } from "bun:test"; // --------------------------------------------------------------------------- // Module mocks for downstream side effects (DB writes, slash resolution, @@ -28,9 +28,16 @@ mock.module("../util/logger.js", () => ({ new Proxy({} as Record, { get: () => () => {} }), })); +/** + * Per-test capability client roster. Set in individual tests to simulate + * a connected macOS client for cross-client drain-path coverage. Reset in + * afterEach so tests don't bleed state. + */ +let mockCapabilityClients: Array<{ clientId: string; actorPrincipalId?: string }> = []; + mock.module("../runtime/assistant-event-hub.js", () => ({ assistantEventHub: { - listClientsByCapability: () => [], + listClientsByCapability: () => mockCapabilityClients, }, broadcastMessage: () => {}, })); @@ -178,6 +185,10 @@ function makeQueuedMessage(opts: { // --------------------------------------------------------------------------- describe("drainQueue preactivation re-add for host-proxy interfaces", () => { + afterEach(() => { + mockCapabilityClients = []; + }); + test("drainSingleMessage re-adds 'app-control' for macOS-sourced queued message", async () => { const queue = new MessageQueue(); const ifCtx: TurnInterfaceContext = { @@ -288,4 +299,84 @@ describe("drainQueue preactivation re-add for host-proxy interfaces", () => { expect(ctx.preactivatedSkillIdCalls).not.toContain("computer-use"); expect(ctx.preactivatedSkillIdCalls).not.toContain("app-control"); }); + + // ── Cross-client drain-path: web source + macOS client connected ────── + + test("drainSingleMessage re-adds 'app-control' for web-sourced message when macOS client is connected", async () => { + mockCapabilityClients = [ + { clientId: "macos-client-1", actorPrincipalId: "user-1" }, + ]; + const queue = new MessageQueue(); + const ifCtx: TurnInterfaceContext = { + userMessageInterface: "web", + assistantMessageInterface: "web", + }; + queue.push( + makeQueuedMessage({ requestId: "req-web-1", turnInterfaceContext: ifCtx }), + ); + const ctx = makeFakeContext({ queue, turnInterfaceContext: ifCtx }); + + await drainQueue(ctx); + + // web natively supports neither host_cu nor host_app_control, but the + // connected macOS client provides both via cross-client routing — so + // both skills must be re-preactivated. + expect(ctx.preactivatedSkillIdCalls).toContain("app-control"); + expect(ctx.preactivatedSkillIds).toContain("app-control"); + expect(ctx.preactivatedSkillIdCalls).toContain("computer-use"); + }); + + test("drainSingleMessage does NOT re-add 'app-control' for web-sourced message when no capable client is connected", async () => { + // mockCapabilityClients remains [] (reset by afterEach from prior test) + const queue = new MessageQueue(); + const ifCtx: TurnInterfaceContext = { + userMessageInterface: "web", + assistantMessageInterface: "web", + }; + queue.push( + makeQueuedMessage({ requestId: "req-web-2", turnInterfaceContext: ifCtx }), + ); + const ctx = makeFakeContext({ queue, turnInterfaceContext: ifCtx }); + + await drainQueue(ctx); + + expect(ctx.preactivatedSkillIdCalls).not.toContain("app-control"); + expect(ctx.preactivatedSkillIdCalls).not.toContain("computer-use"); + }); + + test("drainSingleMessage re-adds 'computer-use' for web-sourced message when macOS client is connected", async () => { + mockCapabilityClients = [ + { clientId: "macos-client-1", actorPrincipalId: "user-1" }, + ]; + const queue = new MessageQueue(); + const ifCtx: TurnInterfaceContext = { + userMessageInterface: "web", + assistantMessageInterface: "web", + }; + queue.push( + makeQueuedMessage({ requestId: "req-web-3", turnInterfaceContext: ifCtx }), + ); + const ctx = makeFakeContext({ queue, turnInterfaceContext: ifCtx }); + + await drainQueue(ctx); + + expect(ctx.preactivatedSkillIdCalls).toContain("computer-use"); + expect(ctx.preactivatedSkillIds).toContain("computer-use"); + }); + + test("drainSingleMessage does NOT re-add 'computer-use' for web-sourced message when no capable client is connected", async () => { + const queue = new MessageQueue(); + const ifCtx: TurnInterfaceContext = { + userMessageInterface: "web", + assistantMessageInterface: "web", + }; + queue.push( + makeQueuedMessage({ requestId: "req-web-4", turnInterfaceContext: ifCtx }), + ); + const ctx = makeFakeContext({ queue, turnInterfaceContext: ifCtx }); + + await drainQueue(ctx); + + expect(ctx.preactivatedSkillIdCalls).not.toContain("computer-use"); + }); });