Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
111 changes: 60 additions & 51 deletions assistant/src/__tests__/host-browser-proxy.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,14 @@ mock.module("../util/logger.js", () => ({

/** Events published through the mock event hub. */
let publishedEvents: unknown[] = [];
let mockHasConnection = true;

/**
* Per-test client roster used by the same-actor binding tests below.
*
* When non-empty, `listClientsByCapability` and `getActorPrincipalIdForClient`
* read from this list. The legacy `mockHasConnection` boolean continues to
* drive `getPreferredClientByCapability`, which is the fallback path used
* when the caller has no `sourceActorPrincipalId` (legacy/internal flows).
* Per-test client roster. Drives `listClientsByCapability` and
* `getMostRecentClientByCapability` — the two hub methods the proxy
* uses for client resolution. Order matters: the first entry whose
* capabilities include the requested cap is the "most recent", which
* matches the production `listClientsByCapability` contract of
* returning clients in `lastActiveAt`-desc order.
*/
type MockClient = {
clientId: string;
Expand All @@ -34,15 +33,8 @@ mock.module("../runtime/assistant-event-hub.js", () => ({
publish: async (event: unknown, _options?: unknown) => {
publishedEvents.push(event);
},
getPreferredClientByCapability: (cap: string, _preference?: unknown) =>
cap === "host_browser" && mockHasConnection
? {
type: "client",
clientId: "test-client",
interfaceId: "macos",
capabilities: ["host_browser"],
}
: undefined,
getMostRecentClientByCapability: (cap: string) =>
mockClients.find((c) => c.capabilities.includes(cap)),
listClientsByCapability: (cap: string) =>
mockClients.filter((c) => c.capabilities.includes(cap)),
getActorPrincipalIdForClient: (clientId: string) =>
Expand Down Expand Up @@ -81,12 +73,21 @@ function resolveResult(
describe("HostBrowserProxy", () => {
let proxy: InstanceType<typeof HostBrowserProxy>;

/**
* A single anonymous host_browser client, used as the default fixture
* for tests that don't care about actor identity.
*/
const DEFAULT_CLIENT: MockClient = {
clientId: "test-client",
interfaceId: "macos",
capabilities: ["host_browser"],
};

beforeEach(() => {
HostBrowserProxy.reset();
pendingInteractions.clear();
publishedEvents = [];
mockHasConnection = true;
mockClients = [];
mockClients = [DEFAULT_CLIENT];
proxy = HostBrowserProxy.instance;
});

Expand Down Expand Up @@ -257,12 +258,12 @@ describe("HostBrowserProxy", () => {

describe("isAvailable", () => {
test("returns true when a connection exists in the registry", () => {
mockHasConnection = true;
mockClients = [DEFAULT_CLIENT];
expect(proxy.isAvailable()).toBe(true);
});

test("returns false when no connection exists", () => {
mockHasConnection = false;
mockClients = [];
expect(proxy.isAvailable()).toBe(false);
});
});
Expand Down Expand Up @@ -312,7 +313,7 @@ describe("HostBrowserProxy", () => {

describe("send failure", () => {
test("rejects when no connection exists at send time", async () => {
mockHasConnection = false;
mockClients = [];

const resultPromise = proxy.request(
{ cdpMethod: "Page.navigate", cdpParams: { url: "https://x.test" } },
Expand Down Expand Up @@ -407,21 +408,21 @@ describe("HostBrowserProxy", () => {
// ---------------------------------------------------------------------------
// Same-actor binding (cross-user enforcement)
//
// When the caller does not supply `targetClientId`, the proxy auto-resolves
// using `resolveTargetClient(sourceActorPrincipalId)` which filters clients
// to the same actor before applying the interface-preference order:
// When the caller does not supply `targetClientId`, the proxy auto-
// resolves using `resolveTargetClient(sourceActorPrincipalId)`:
//
// 1. `resolveTargetClient(sourceActorPrincipalId)` filters candidate
// clients to those owned by the caller's actor before applying the
// chrome-extension-first interface preference. When the caller has
// no actor (legacy/internal flow), it falls back to the legacy
// `getPreferredClientByCapability` path used by other tests above.
// 1. Candidate clients are filtered to those owned by the caller's
// actor; the first match (lastActiveAt-desc) wins. When the
// caller has no actor, the resolver falls through to the most-
// recently-active host_browser client without same-actor filtering.
// 2. The proxy persists `targetClientId` and `targetActorPrincipalId`
// on the pending interaction so the result-route's same-actor check
// has authoritative bindings to compare against (mirrors host-cu).
// on the pending interaction so the result-route's same-actor
// check has authoritative bindings to compare against (mirrors
// host-cu).
//
// These tests focus on (1) and (2). Result-side guard coverage lives in
// `host-browser-routes.test.ts` (HTTP 400/403 against the same bindings).
// These tests focus on (1) and (2). Result-side guard coverage lives
// in `host-browser-routes.test.ts` (HTTP 400/403 against the same
// bindings).
// ---------------------------------------------------------------------------

describe("same-actor binding", () => {
Expand Down Expand Up @@ -482,7 +483,11 @@ describe("HostBrowserProxy", () => {
expect(getPublishedMessages()).toHaveLength(0);
});

test("prefers chrome-extension over macos among same-actor clients", async () => {
test("picks the most-recently-active same-actor client among multiple transports", async () => {
// Mock `listClientsByCapability` returns mockClients in array
// order, which mirrors production's `lastActiveAt`-desc ordering.
// The first same-actor entry wins regardless of transport; LLMs
// pin a specific transport via `target_client_id`.
mockClients = [
{
clientId: "macos-client",
Expand Down Expand Up @@ -510,7 +515,7 @@ describe("HostBrowserProxy", () => {
const requestId = sent.requestId as string;

const pending = pendingInteractions.get(requestId);
expect(pending?.targetClientId).toBe("ext-client");
expect(pending?.targetClientId).toBe("macos-client");

resolveResult(requestId, { content: "ok", isError: false });
await resultPromise;
Expand Down Expand Up @@ -545,13 +550,13 @@ describe("HostBrowserProxy", () => {
await resultPromise;
});

test("legacy callers without a sourceActorPrincipalId use the unfiltered fallback path", async () => {
// No mockClients populatedbut mockHasConnection is true, so the
// legacy `getPreferredClientByCapability` branch returns its default
// `test-client` candidate. The pending interaction binds to that
// client without an actor, preserving prior single-user behavior.
mockHasConnection = true;
mockClients = [];
test("legacy callers without a sourceActorPrincipalId fall through to the most-recently-active client", async () => {
// No `sourceActorPrincipalId` suppliedthe proxy falls back to
// the unfiltered roster and picks the first entry. Mirrors the
// singleton-style behavior expected by registry-driven callers
// that haven't been threaded with an actor identity. The pending
// interaction binds to the resolved client without an actor.
mockClients = [DEFAULT_CLIENT];

const resultPromise = proxy.request(
{ cdpMethod: "Page.navigate", cdpParams: { url: "https://a.test" } },
Expand All @@ -575,8 +580,9 @@ describe("HostBrowserProxy", () => {
test("rejects when caller has actor but no host_browser-capable client is connected for that actor", async () => {
// Same-user filter returns empty even though listClientsByCapability
// would return a non-empty list (because that list is for a
// different actor). The legacy fallback IS NOT consulted in this
// branch — we don't want to silently broadcast to anyone.
// different actor). The unfiltered fallback path runs only when
// the caller has no actor — we don't silently broadcast to anyone
// when the caller IS authenticated to a specific actor.
mockClients = [
{
clientId: "other-user-ext",
Expand All @@ -603,8 +609,8 @@ describe("HostBrowserProxy", () => {
// ---------------------------------------------------------------------------
// Explicit targetClientId routing
//
// When `targetClientId` is supplied, the proxy skips the interface-preference
// sort and routes directly to the named client (subject to the same-actor
// When `targetClientId` is supplied, the proxy skips auto-resolution
// and routes directly to the named client (subject to the same-actor
// enforcement that runs on all host-proxy requests).
// ---------------------------------------------------------------------------

Expand All @@ -625,8 +631,9 @@ describe("HostBrowserProxy", () => {
},
];

// Explicitly target the macOS client even though chrome-extension
// would win under normal interface-preference ordering.
// Explicitly target the macOS client even though it isn't the
// first entry in the roster — explicit targeting overrides
// auto-resolution.
const resultPromise = proxy.request(
{ cdpMethod: "Page.navigate", cdpParams: { url: "https://a.test" } },
"session-1",
Expand Down Expand Up @@ -722,7 +729,7 @@ describe("HostBrowserProxy", () => {
expect(getPublishedMessages()).toHaveLength(0);
});

test("no targetClientId falls back to interface-preference order (regression guard)", async () => {
test("no targetClientId auto-resolves to the most-recently-active same-actor client", async () => {
mockClients = [
{
clientId: "macos-client",
Expand All @@ -738,7 +745,9 @@ describe("HostBrowserProxy", () => {
},
];

// No targetClientId — should auto-resolve to chrome-extension (higher priority).
// No targetClientId — falls through to the first same-actor
// entry by lastActiveAt-desc. Mock array order is the proxy's
// input contract.
const resultPromise = proxy.request(
{ cdpMethod: "Page.navigate", cdpParams: { url: "https://a.test" } },
"session-1",
Expand All @@ -750,7 +759,7 @@ describe("HostBrowserProxy", () => {
const requestId = (getPublishedMessages()[0] as Record<string, unknown>)
.requestId as string;
const pending = pendingInteractions.get(requestId);
expect(pending?.targetClientId).toBe("ext-client");
expect(pending?.targetClientId).toBe("macos-client");

resolveResult(requestId, { content: "ok", isError: false });
await resultPromise;
Expand Down
64 changes: 24 additions & 40 deletions assistant/src/daemon/host-browser-proxy.ts
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚩 Stale JSDoc on request() still references removed "interface-preference resolution"

The request() method's JSDoc at assistant/src/daemon/host-browser-proxy.ts:138-140 still says "falls back to interface-preference resolution without an actor filter" but the underlying resolveTargetClient now simply returns candidates[0] (most-recently-active). This comment was not updated alongside the implementation change and now describes behavior that no longer exists. The assistant/AGENTS.md rule states: "When writing or updating comments, do not reference code that has been removed." Two other files also have stale references: factory.ts:102 and extension-cdp-client.ts:56 both mention "interface-preference order" in their targetClientId JSDoc comments.

(Refers to lines 138-140)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import { v4 as uuid } from "uuid";

import type { InterfaceId } from "../channels/types.js";
import {
assistantEventHub,
broadcastMessage,
Expand All @@ -25,12 +24,6 @@ export type HostBrowserInput = DistributiveOmit<

const log = getLogger("host-browser-proxy");

/** Interface priority order for host_browser: Chrome Extension first, macOS SSE bridge second. */
const HOST_BROWSER_INTERFACE_PREFERENCE: InterfaceId[] = [
"chrome-extension",
"macos",
];

/**
* Pick the host_browser-capable client to dispatch to.
*
Expand All @@ -40,14 +33,21 @@ const HOST_BROWSER_INTERFACE_PREFERENCE: InterfaceId[] = [
* `sourceActorPrincipalId` is present.
*
* When `sourceActorPrincipalId` is supplied (and no explicit target),
* candidate clients are filtered down to those owned by the same actor
* before applying the interface-preference order. Returns `undefined`
* when no same-actor client is connected; the caller surfaces this as
* the existing "no active extension connection" rejection.
* candidate clients are filtered down to those owned by the same actor.
* Returns `undefined` when no same-actor client is connected; the
* caller surfaces this as the existing "no active extension connection"
* rejection.
*
* When neither is supplied (legacy callers without a resolved actor
* identity), falls through to the prior behavior so the registry
* singleton continues to work as before.
* identity), falls through to the most-recently-active host_browser
* client so the registry singleton continues to work for single-client
* setups.
*
* Within each branch, ties are broken by `lastActiveAt` descending —
* the natural order returned by `listClientsByCapability`. Callers that
* need a specific transport (e.g. Chrome Extension's `chrome.debugger`
* over the macOS CDP bridge) must pass `targetClientId` explicitly via
* the LLM-facing param added in #30066.
*/
function resolveTargetClient(
sourceActorPrincipalId: string | undefined,
Expand All @@ -58,28 +58,14 @@ function resolveTargetClient(
return clients.find((c) => c.clientId === targetClientId);
}

const candidates =
assistantEventHub.listClientsByCapability("host_browser");
if (sourceActorPrincipalId == null) {
return assistantEventHub.getPreferredClientByCapability(
"host_browser",
HOST_BROWSER_INTERFACE_PREFERENCE,
);
return candidates[0];
}

const sameActorClients = assistantEventHub
.listClientsByCapability("host_browser")
.filter((c) => c.actorPrincipalId === sourceActorPrincipalId);
if (sameActorClients.length === 0) return undefined;

// Stable sort by interface preference; lastActiveAt is the implicit
// tiebreaker because listClientsByCapability already returns clients
// in lastActiveAt-desc order.
return [...sameActorClients].sort((a, b) => {
const ai = HOST_BROWSER_INTERFACE_PREFERENCE.indexOf(a.interfaceId);
const bi = HOST_BROWSER_INTERFACE_PREFERENCE.indexOf(b.interfaceId);
const ea = ai === -1 ? HOST_BROWSER_INTERFACE_PREFERENCE.length : ai;
const eb = bi === -1 ? HOST_BROWSER_INTERFACE_PREFERENCE.length : bi;
return ea - eb;
})[0];
return candidates.find(
(c) => c.actorPrincipalId === sourceActorPrincipalId,
);
}

export class HostBrowserProxy {
Expand Down Expand Up @@ -119,10 +105,7 @@ export class HostBrowserProxy {
*/
isAvailable(): boolean {
return (
assistantEventHub.getPreferredClientByCapability(
"host_browser",
HOST_BROWSER_INTERFACE_PREFERENCE,
) != null
assistantEventHub.getMostRecentClientByCapability("host_browser") != null
);
}

Expand Down Expand Up @@ -152,9 +135,10 @@ export class HostBrowserProxy {
* so that the result-route's same-actor check can verify the submitting
* client at result time.
*
* When `sourceActorPrincipalId` is undefined (legacy/internal flows with
* no resolved actor identity), falls back to interface-preference
* resolution without an actor filter, preserving prior behavior.
* When `sourceActorPrincipalId` is undefined (legacy/internal flows
* with no resolved actor identity), falls back to the most-recently-
* active host_browser client without an actor filter so the registry
* singleton continues to work for single-client setups.
*/
request(
input: HostBrowserInput,
Expand Down
29 changes: 0 additions & 29 deletions assistant/src/runtime/assistant-event-hub.ts
Original file line number Diff line number Diff line change
Expand Up @@ -420,35 +420,6 @@ export class AssistantEventHub {
return this.listClientsByCapability(capability)[0];
}

/**
* Return the best client for the given capability using an explicit
* interface preference order. Among clients that support `capability`,
* the one whose `interfaceId` appears earliest in `interfacePreference`
* wins. Within the same interface tier, `lastActiveAt` is the tiebreaker
* (most recent first). Clients not in the preference list are considered last.
*
* Used by {@link HostBrowserProxy} to prefer the Chrome Extension
* (`chrome-extension`) over the macOS SSE bridge (`macos`) when both are
* connected, so `chrome.debugger` is used ahead of the localhost:9222 path.
*/
getPreferredClientByCapability(
capability: HostProxyCapability,
interfacePreference: InterfaceId[],
): ClientEntry | undefined {
const clients = this.listClientsByCapability(capability);
if (clients.length === 0) return undefined;
// listClientsByCapability returns clients sorted by lastActiveAt desc
// (most recent first). A stable sort by preference index preserves that
// ordering within each interface tier.
return clients.sort((a, b) => {
const ai = interfacePreference.indexOf(a.interfaceId);
const bi = interfacePreference.indexOf(b.interfaceId);
const ea = ai === -1 ? interfacePreference.length : ai;
const eb = bi === -1 ? interfacePreference.length : bi;
return ea - eb;
})[0];
}

/**
* Return all client subscribers with the given interface type,
* sorted by `lastActiveAt` descending.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,9 @@ export class ExtensionCdpClient implements ScopedCdpClient {
*/
private readonly sourceActorPrincipalId?: string,
/**
* Explicit target client id. When provided, the proxy routes directly to
* that client instead of applying the interface-preference order.
* Explicit target client id. When provided, the proxy routes directly
* to that client instead of auto-resolving to the most-recently-active
* same-actor host_browser client.
*/
private readonly targetClientId?: string,
) {}
Expand Down
5 changes: 3 additions & 2 deletions assistant/src/tools/browser/cdp-client/factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -99,8 +99,9 @@ export interface GetCdpClientOptions {
mode?: BrowserMode;
/**
* Explicit target client id. When provided, the extension backend routes
* to this specific client instead of applying the interface-preference
* order. Mirrors the `target_client_id` pattern on host_bash/host_file/host_cu.
* to this specific client instead of auto-resolving to the most-recently-
* active same-actor host_browser client. Mirrors the `target_client_id`
* pattern on host_bash/host_file/host_cu.
*/
targetClientId?: string;
}
Expand Down
Loading