From 5d07c86f665a161e82ea64a67aa5e9be874c61f4 Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Wed, 2 Sep 2026 11:45:33 -0700 Subject: [PATCH 1/3] Trace client agent cleanup failures and pin the group's method set Two follow-ups to #2914, both on the client-hosted agent path. Disconnect cleanup swallowed every error. `removeClientAgent` failing is how a client agent leaks onto the shared dispatcher, and `leaveConversation` failing keeps a dispatcher alive with its idle timer never starting -- both then surface much later with nothing pointing back at the cause. Trace them on `agent-server:connection:error`. Not retried: removal is idempotent and ownership-checked, so a second attempt could only race a reconnect that has legitimately reclaimed the instance. `createMux` builds its method set from whichever proxy created the group, and `getManifestKey` hashes schema text only, so two builds can share a schema and still implement different methods. A device with a different `agentInterface` joined and appeared to support methods it does not, and the call only failed once someone made it. Compare the interface at join, alongside the schema, and for a replacement too, since replacing in place keeps the original mux. Only compared when both sides declared one, so a client that sends none is unaffected. Tests: 4 cases (115 passed, 111 before). Mutation-checked -- disabling the interface check fails both rejection cases and leaves both acceptance cases passing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../server/src/clientAgentRegistry.ts | 47 ++++++++++ .../server/src/connectionHandler.ts | 33 ++++++- .../server/src/conversationManager.ts | 7 +- .../test/clientAgentIntegration.spec.ts | 2 + .../server/test/clientAgentRegistry.spec.ts | 89 +++++++++++++++++++ 5 files changed, 172 insertions(+), 6 deletions(-) diff --git a/ts/packages/agentServer/server/src/clientAgentRegistry.ts b/ts/packages/agentServer/server/src/clientAgentRegistry.ts index 9f2a5b26ed..bb33e446d7 100644 --- a/ts/packages/agentServer/server/src/clientAgentRegistry.ts +++ b/ts/packages/agentServer/server/src/clientAgentRegistry.ts @@ -36,6 +36,11 @@ export type ClientAgentGroup = { manifest: AppAgentManifest; /** Hash of the schema source; instances must agree on it. See {@link getManifestKey}. */ manifestKey: string; + /** + * Normalized `agentInterface` the group was created with, or undefined when + * the creator did not declare one. See {@link getAgentInterfaceKey}. + */ + agentInterfaceKey: string | undefined; /** * Whether the client that created this group opted in to sharing the name. * Off means a second, different client is rejected exactly as it was before @@ -53,6 +58,11 @@ export type ClientAgentRegistration = { connectionId: string; appAgent: AppAgent; manifest: AppAgentManifest; + /** + * Methods the client implements. Optional so a client that does not send + * one keeps working; when present it must match the group's. + */ + agentInterface?: readonly string[] | undefined; /** See {@link ClientAgentGroup.multiInstance}. Only read on the first registration. */ multiInstance?: boolean; }; @@ -135,6 +145,28 @@ export function schemaMismatchMessage(name: string): string { return `Client agent '${name}' is already registered on this conversation with a different schema version. Update the app to the same version as the other device(s), or disconnect them first.`; } +/** + * Normalized `agentInterface`, or undefined when the client did not declare + * one. + * + * The mux is built once, from the first instance's proxy, and + * {@link getManifestKey} only covers schema text -- two app versions can share + * a schema and still implement different methods. Without this, a device with a + * narrower interface joins a group created by a richer one and silently appears + * to support methods it does not; the call only fails once someone makes it. + */ +export function getAgentInterfaceKey( + agentInterface: readonly string[] | undefined, +): string | undefined { + return agentInterface === undefined + ? undefined + : [...new Set(agentInterface)].sort().join("\u0000"); +} + +export function interfaceMismatchMessage(name: string): string { + return `Client agent '${name}' is already registered on this conversation by a device that implements a different set of methods. Update the app to the same version as the other device(s), or disconnect them first.`; +} + /** * Names the user can tell apart. Two phones of the same model both report * "Pixel 8", so duplicates get a numeric suffix in registration order. @@ -530,6 +562,7 @@ export function createClientAgentGroup( name, manifest: registration.manifest, manifestKey: getManifestKey(registration.manifest), + agentInterfaceKey: getAgentInterfaceKey(registration.agentInterface), multiInstance: registration.multiInstance === true, instances: new Map([[instance.instanceId, instance]]), mux: undefined as unknown as AppAgent, @@ -561,6 +594,20 @@ export async function joinClientAgentGroup( throw new Error(schemaMismatchMessage(group.name)); } + // Checked alongside the schema, and for a replacement too: the mux was + // built from the method set of whichever proxy created the group, so an + // instance that arrives with a different one would be routed calls it + // cannot answer. Only compared when both sides declared an interface, so a + // client that sends none keeps working. + const agentInterfaceKey = getAgentInterfaceKey(registration.agentInterface); + if ( + agentInterfaceKey !== undefined && + group.agentInterfaceKey !== undefined && + agentInterfaceKey !== group.agentInterfaceKey + ) { + throw new Error(interfaceMismatchMessage(group.name)); + } + const existing = group.instances.get(registration.instanceId); if (existing !== undefined) { existing.appAgent = registration.appAgent; diff --git a/ts/packages/agentServer/server/src/connectionHandler.ts b/ts/packages/agentServer/server/src/connectionHandler.ts index 115259a451..4dad7ba972 100644 --- a/ts/packages/agentServer/server/src/connectionHandler.ts +++ b/ts/packages/agentServer/server/src/connectionHandler.ts @@ -24,6 +24,13 @@ import type { PortRegistrar } from "agent-dispatcher"; import type { ConversationManager } from "./conversationManager.js"; import { resolveTunnelUrlForDiscovery } from "./tunnelResolver.js"; import { getSpeechToken } from "./speechToken.js"; +import registerDebug from "debug"; + +// Disconnect cleanup is best effort, so a failure cannot be surfaced to anyone: +// the socket it would be reported on is already gone. Without a trace, a client +// agent left behind on the shared dispatcher only shows up much later as a +// routing failure with nothing pointing back at the cause. +const debugError = registerDebug("agent-server:connection:error"); /** * Per-connection handler signature expected by transports (the WebSocket @@ -609,6 +616,7 @@ export function createAgentServerConnectionHandler( displayName, connectionId, param.multiInstance === true, + agentInterface, ); } catch (e) { channelProvider.deleteChannel(`agent:${name}`); @@ -683,8 +691,18 @@ export function createAgentServerConnectionHandler( .removeClientAgent(conversationId, name, instanceId, { ownerConnectionId: connectionId, }) - .catch(() => { - // Best effort on disconnect + .catch((e) => { + // Best effort on disconnect, but not silent: this + // failing is how a client agent leaks onto the + // shared dispatcher. Not retried on purpose -- + // removal is idempotent and ownership-checked, so a + // second attempt could only race a reconnect that + // has legitimately reclaimed the instance. + debugError( + `Failed to remove client agent "${name}" instance ${instanceId} (connection ${connectionId}) from conversation ${conversationId} on disconnect: ${ + e instanceof Error ? e.message : String(e) + }`, + ); }); } } @@ -695,8 +713,15 @@ export function createAgentServerConnectionHandler( ] of joinedConversations.entries()) { conversationManager .leaveConversation(conversationId, connectionId) - .catch(() => { - // Best effort on disconnect + .catch((e) => { + // Best effort on disconnect, but traced: a conversation + // this connection never leaves keeps its dispatcher + // alive and its idle timer from ever starting. + debugError( + `Failed to leave conversation ${conversationId} for connection ${connectionId} on disconnect: ${ + e instanceof Error ? e.message : String(e) + }`, + ); }); } joinedConversations.clear(); diff --git a/ts/packages/agentServer/server/src/conversationManager.ts b/ts/packages/agentServer/server/src/conversationManager.ts index 40d90598a7..5d1a49af20 100644 --- a/ts/packages/agentServer/server/src/conversationManager.ts +++ b/ts/packages/agentServer/server/src/conversationManager.ts @@ -253,8 +253,8 @@ export type ConversationManager = { * same schema: the dynamic agent is added once and each client becomes an * instance behind it. Re-registering the same `instanceId` replaces its * proxy in place, which is how a reconnect recovers. Rejects when the - * schema differs, or when the instance is new and multi-instance support - * is switched off. + * schema or the `agentInterface` differs, or when the instance is new and + * multi-instance support is switched off. */ addClientAgent( conversationId: string, @@ -265,6 +265,7 @@ export type ConversationManager = { displayName: string, connectionId: string, multiInstance: boolean, + agentInterface?: readonly string[], ): Promise; /** * Remove one instance added via {@link addClientAgent}. The dynamic agent @@ -1220,6 +1221,7 @@ export async function createConversationManager( displayName: string, connectionId: string, multiInstance: boolean, + agentInterface?: readonly string[], ): Promise { const record = conversations.get(conversationId); if (record === undefined) { @@ -1232,6 +1234,7 @@ export async function createConversationManager( connectionId, appAgent, manifest, + agentInterface, multiInstance, }); debugConversation( diff --git a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts index 69559bc47b..5e4e7007b2 100644 --- a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts @@ -102,6 +102,7 @@ function createTestServer(): TestServer { displayName: string, connectionId: string, multiInstance: boolean, + agentInterface?: readonly string[], ) { await registry.add(host, name, { instanceId, @@ -109,6 +110,7 @@ function createTestServer(): TestServer { connectionId, appAgent, manifest: agentManifest, + agentInterface, multiInstance, }); }, diff --git a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts index 0a56a053c4..b317d7b072 100644 --- a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts @@ -122,6 +122,7 @@ async function register( connectionId: string; appAgent: AppAgent; manifest?: AppAgentManifest; + agentInterface?: readonly string[]; multiInstance?: boolean; }, ): Promise { @@ -131,6 +132,7 @@ async function register( connectionId: options.connectionId, appAgent: options.appAgent, manifest: options.manifest ?? makeManifest(), + agentInterface: options.agentInterface, // Devices opt in; the tests that pin single-host behaviour pass // false explicitly. multiInstance: options.multiInstance ?? true, @@ -257,6 +259,93 @@ describe("clientAgentRegistry registration", () => { expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); + test("a device implementing a different method set is rejected", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction"], + }); + + // Same schema, older build: it cannot answer getDynamicDisplay. The mux + // was built from A's proxy, so without the check B would be routed + // calls it has no method for. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }), + ).rejects.toThrow(/different set of methods/i); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + }); + + test("the same method set in another order is accepted", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice().appAgent, + agentInterface: ["getDynamicDisplay", "executeAction"], + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + }); + + test("a client that declares no method set is unaffected by the check", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction"], + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice().appAgent, + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + }); + + test("a reconnecting instance cannot change the group's method set", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction"], + }); + + // Replacing in place keeps the mux built from the original proxy, so + // the check has to cover a replacement too, not just a new instance. + await expect( + register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice().appAgent, + agentInterface: ["executeAction", "getDynamicDisplay"], + }), + ).rejects.toThrow(/different set of methods/i); + }); + // Case 12 test("a client that does not opt in stays the only host of its agent", async () => { const registry = createClientAgentRegistry(); From c971f56ca7c0e5980fe28e1786c14444faf45b4f Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Wed, 2 Sep 2026 15:22:54 -0700 Subject: [PATCH 2/3] Require agentInterface, and let a lone device change its method set Follow-ups to the review of the method-set check. agentInterface was optional, so the check skipped whenever it was absent. It cannot be absent: registerClientAgent declares it required and createAgentRpcClient dereferences it to build the proxy, so a registration that reaches the registry always has one. The optional branch was dead code that only weakened the check. It is now required and typed AgentInterfaceFunctionName[] rather than string[], and the two undefined guards are gone. The check also rejected a lone device that upgraded its app: same schema, one more method, and the reconnect failed while its stale instance was still in the group - told to disconnect devices that do not exist. When the registration takes over the group's only instance it now adopts the new set instead. The mux has to be updated in place because the dispatcher keeps the object addDynamicAgent handed it and checks optional methods on it at call time, so replacing group.mux would leave the dispatcher on the old one. Tests: makeDevice now builds a proxy carrying exactly the methods it declares, so the cases exercise the actual misroute instead of just the string comparison. Covers both directions, the empty set, a plain reconnect, and a lone device gaining and losing a method. Mutation-checked: disabling rebuildMux fails only the two mux cases; disabling the rejection fails only the four rejection cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../server/src/clientAgentRegistry.ts | 91 ++++++-- .../server/src/conversationManager.ts | 10 +- .../test/clientAgentIntegration.spec.ts | 3 +- .../server/test/clientAgentRegistry.spec.ts | 207 +++++++++++++++--- 4 files changed, 254 insertions(+), 57 deletions(-) diff --git a/ts/packages/agentServer/server/src/clientAgentRegistry.ts b/ts/packages/agentServer/server/src/clientAgentRegistry.ts index bb33e446d7..f8d028b833 100644 --- a/ts/packages/agentServer/server/src/clientAgentRegistry.ts +++ b/ts/packages/agentServer/server/src/clientAgentRegistry.ts @@ -7,6 +7,7 @@ import { AppAgentManifest, SessionContext, } from "@typeagent/agent-sdk"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; import { createHash } from "node:crypto"; import { createLimiter } from "@typeagent/common-utils"; import registerDebug from "debug"; @@ -37,10 +38,10 @@ export type ClientAgentGroup = { /** Hash of the schema source; instances must agree on it. See {@link getManifestKey}. */ manifestKey: string; /** - * Normalized `agentInterface` the group was created with, or undefined when - * the creator did not declare one. See {@link getAgentInterfaceKey}. + * Normalized `agentInterface` of the instances currently in the group. See + * {@link getAgentInterfaceKey}. */ - agentInterfaceKey: string | undefined; + agentInterfaceKey: string; /** * Whether the client that created this group opted in to sharing the name. * Off means a second, different client is rejected exactly as it was before @@ -59,10 +60,12 @@ export type ClientAgentRegistration = { appAgent: AppAgent; manifest: AppAgentManifest; /** - * Methods the client implements. Optional so a client that does not send - * one keeps working; when present it must match the group's. + * Methods the client implements, which the caller already used to build + * {@link ClientAgentRegistration.appAgent}. Required: `registerClientAgent` + * takes it as a required field and `createAgentRpcClient` cannot build a + * proxy without it, so a registration that reaches here always has one. */ - agentInterface?: readonly string[] | undefined; + agentInterface: readonly AgentInterfaceFunctionName[]; /** See {@link ClientAgentGroup.multiInstance}. Only read on the first registration. */ multiInstance?: boolean; }; @@ -146,8 +149,9 @@ export function schemaMismatchMessage(name: string): string { } /** - * Normalized `agentInterface`, or undefined when the client did not declare - * one. + * Normalized `agentInterface`, order-insensitive and de-duplicated so key order + * cannot cause a false mismatch (the same trap {@link getManifestKey} avoids + * for Android's `org.json.JSONObject`). * * The mux is built once, from the first instance's proxy, and * {@link getManifestKey} only covers schema text -- two app versions can share @@ -156,11 +160,9 @@ export function schemaMismatchMessage(name: string): string { * to support methods it does not; the call only fails once someone makes it. */ export function getAgentInterfaceKey( - agentInterface: readonly string[] | undefined, -): string | undefined { - return agentInterface === undefined - ? undefined - : [...new Set(agentInterface)].sort().join("\u0000"); + agentInterface: readonly AgentInterfaceFunctionName[], +): string { + return [...new Set(agentInterface)].sort().join("\u0000"); } export function interfaceMismatchMessage(name: string): string { @@ -513,6 +515,27 @@ function createMux(group: ClientAgentGroup, template: AppAgent): AppAgent { return mux as unknown as AppAgent; } +/** + * Point the group's existing mux at a new method set, in place. + * + * The dispatcher was handed this exact object by `addDynamicAgent` and keeps + * that reference, checking each optional method on it at call time. Assigning a + * fresh object to `group.mux` would therefore leave the dispatcher on the old + * one, so the methods have to be swapped onto the object it already holds. + */ +function rebuildMux(group: ClientAgentGroup, template: AppAgent): void { + const next = methodsOf(createMux(group, template)); + const current = methodsOf(group.mux); + for (const method of Object.keys(current)) { + if (next[method] === undefined) { + delete current[method]; + } + } + for (const method of Object.keys(next)) { + current[method] = next[method]; + } +} + /** * Bring a device that joined late up to the state the others are in. Failures * are traced, not thrown: one device must not fail another's registration. @@ -579,6 +602,25 @@ export function createClientAgentGroup( return group; } +/** + * True when this registration takes over the group's only instance: either the + * same `instanceId` coming back, or the same connection re-registering under a + * new one (its old proxy died when the new one claimed the `agent:` + * channel). Either way no other device is in the group, so the method set is + * this device's alone to change. + */ +function replacesSoleInstance( + group: ClientAgentGroup, + registration: ClientAgentRegistration, +): boolean { + return ( + group.instances.size === 1 && + (group.instances.has(registration.instanceId) || + findInstanceIdForConnection(group, registration.connectionId) !== + undefined) + ); +} + /** * Add a device, or replace its proxy if the same `instanceId` is already * there. Replacing in place is what makes a reconnect work: the device keeps @@ -597,15 +639,22 @@ export async function joinClientAgentGroup( // Checked alongside the schema, and for a replacement too: the mux was // built from the method set of whichever proxy created the group, so an // instance that arrives with a different one would be routed calls it - // cannot answer. Only compared when both sides declared an interface, so a - // client that sends none keeps working. + // cannot answer. const agentInterfaceKey = getAgentInterfaceKey(registration.agentInterface); - if ( - agentInterfaceKey !== undefined && - group.agentInterfaceKey !== undefined && - agentInterfaceKey !== group.agentInterfaceKey - ) { - throw new Error(interfaceMismatchMessage(group.name)); + if (agentInterfaceKey !== group.agentInterfaceKey) { + // Unless this device is the whole group. A lone device that upgrades + // its app keeps its schema but can gain or lose methods, and rejecting + // that would break a reconnect that used to work - with a message + // telling the user to disconnect devices that do not exist. Nobody else + // is sharing the name, so adopt the new set and bring the mux with it. + if (!replacesSoleInstance(group, registration)) { + throw new Error(interfaceMismatchMessage(group.name)); + } + group.agentInterfaceKey = agentInterfaceKey; + rebuildMux(group, registration.appAgent); + debugGroup( + `${group.name}: sole instance ${registration.instanceId} changed the method set; mux rebuilt`, + ); } const existing = group.instances.get(registration.instanceId); diff --git a/ts/packages/agentServer/server/src/conversationManager.ts b/ts/packages/agentServer/server/src/conversationManager.ts index 5d1a49af20..087b738b68 100644 --- a/ts/packages/agentServer/server/src/conversationManager.ts +++ b/ts/packages/agentServer/server/src/conversationManager.ts @@ -24,6 +24,7 @@ import { ConversationSummaryResult, } from "agent-dispatcher"; import type { AppAgent, AppAgentManifest } from "@typeagent/agent-sdk"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; import type { DisplayLogEntry, PendingInteractionRequest, @@ -253,8 +254,9 @@ export type ConversationManager = { * same schema: the dynamic agent is added once and each client becomes an * instance behind it. Re-registering the same `instanceId` replaces its * proxy in place, which is how a reconnect recovers. Rejects when the - * schema or the `agentInterface` differs, or when the instance is new and - * multi-instance support is switched off. + * schema differs, when the `agentInterface` differs from what the other + * devices implement, or when the instance is new and multi-instance + * support is switched off. */ addClientAgent( conversationId: string, @@ -265,7 +267,7 @@ export type ConversationManager = { displayName: string, connectionId: string, multiInstance: boolean, - agentInterface?: readonly string[], + agentInterface: readonly AgentInterfaceFunctionName[], ): Promise; /** * Remove one instance added via {@link addClientAgent}. The dynamic agent @@ -1221,7 +1223,7 @@ export async function createConversationManager( displayName: string, connectionId: string, multiInstance: boolean, - agentInterface?: readonly string[], + agentInterface: readonly AgentInterfaceFunctionName[], ): Promise { const record = conversations.get(conversationId); if (record === undefined) { diff --git a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts index 5e4e7007b2..76e7cba95a 100644 --- a/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentIntegration.spec.ts @@ -16,6 +16,7 @@ import { } from "@typeagent/agent-rpc/channel"; import type { AppAgent, AppAgentManifest } from "@typeagent/agent-sdk"; import type { TypeAgentAction } from "@typeagent/agent-sdk"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; import type { ClientIO } from "@typeagent/dispatcher-rpc/types"; import type { MacroManager } from "@typeagent/copilot-macros"; import { @@ -102,7 +103,7 @@ function createTestServer(): TestServer { displayName: string, connectionId: string, multiInstance: boolean, - agentInterface?: readonly string[], + agentInterface: readonly AgentInterfaceFunctionName[], ) { await registry.add(host, name, { instanceId, diff --git a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts index b317d7b072..42bcd51309 100644 --- a/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts +++ b/ts/packages/agentServer/server/test/clientAgentRegistry.spec.ts @@ -17,6 +17,7 @@ import { type ClientAgentHost, type ClientAgentRegistry, } from "../src/clientAgentRegistry.js"; +import type { AgentInterfaceFunctionName } from "@typeagent/agent-rpc/server"; const AGENT_NAME = "androidDevice"; const SCHEMA = @@ -44,18 +45,44 @@ function makeManifest( type FakeDevice = { appAgent: AppAgent; executed: TypeAgentAction[]; + dynamicDisplays: string[]; }; -function makeDevice(): FakeDevice { +/** What a device implements unless a test asks for something else. */ +const DEFAULT_INTERFACE: AgentInterfaceFunctionName[] = ["executeAction"]; + +/** + * A device whose proxy carries exactly the methods it declares. The interface + * checks are about a device advertising methods it cannot answer, so a fake + * that always implements the same one would not show the difference. + * `getDynamicDisplay` is the optional method those tests move in and out. + */ +function makeDevice( + agentInterface: readonly AgentInterfaceFunctionName[] = DEFAULT_INTERFACE, +): FakeDevice { const executed: TypeAgentAction[] = []; + const dynamicDisplays: string[] = []; + const available: Record = { + async executeAction(action: TypeAgentAction) { + executed.push(action); + return undefined; + }, + async getDynamicDisplay(_type: string, displayId: string) { + dynamicDisplays.push(displayId); + return { type: "text", content: displayId }; + }, + }; + const appAgent: Record = {}; + for (const method of agentInterface) { + if (available[method] === undefined) { + throw new Error(`makeDevice has no fake for '${method}'`); + } + appAgent[method] = available[method]; + } return { executed, - appAgent: { - async executeAction(action: TypeAgentAction) { - executed.push(action); - return undefined; - }, - }, + dynamicDisplays, + appAgent: appAgent as unknown as AppAgent, }; } @@ -122,7 +149,7 @@ async function register( connectionId: string; appAgent: AppAgent; manifest?: AppAgentManifest; - agentInterface?: readonly string[]; + agentInterface?: readonly AgentInterfaceFunctionName[]; multiInstance?: boolean; }, ): Promise { @@ -132,7 +159,13 @@ async function register( connectionId: options.connectionId, appAgent: options.appAgent, manifest: options.manifest ?? makeManifest(), - agentInterface: options.agentInterface, + // Default to what the proxy actually implements, which is what the + // real client sends: createAgentRpcServer derives agentInterface from + // the agent object. A test that passes one explicitly is deliberately + // making the two disagree. + agentInterface: + options.agentInterface ?? + (Object.keys(options.appAgent) as AgentInterfaceFunctionName[]), // Devices opt in; the tests that pin single-host behaviour pass // false explicitly. multiInstance: options.multiInstance ?? true, @@ -259,26 +292,52 @@ describe("clientAgentRegistry registration", () => { expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); - test("a device implementing a different method set is rejected", async () => { + test("a second device implementing fewer methods is rejected", async () => { const registry = createClientAgentRegistry(); const host = makeHost(); + const a = makeDevice(["executeAction", "getDynamicDisplay"]); await register(registry, host, { instanceId: "a", connectionId: "conn-a", - appAgent: makeDevice().appAgent, - agentInterface: ["executeAction"], + appAgent: a.appAgent, }); + // The mux is built from A's proxy, so the dynamic agent the dispatcher + // holds offers getDynamicDisplay. + expect(getMux(registry).getDynamicDisplay).toBeDefined(); - // Same schema, older build: it cannot answer getDynamicDisplay. The mux - // was built from A's proxy, so without the check B would be routed - // calls it has no method for. + // B is an older build: same schema, but no getDynamicDisplay. Without + // the check it would join, and the first getDynamicDisplay that routed + // to B would fail at call time. await expect( register(registry, host, { instanceId: "b", connectionId: "conn-b", - appAgent: makeDevice().appAgent, - agentInterface: ["executeAction", "getDynamicDisplay"], + appAgent: makeDevice(["executeAction"]).appAgent, + }), + ).rejects.toThrow(/different set of methods/i); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); + }); + + test("a second device implementing extra methods is rejected", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + + // The other direction: B's extra method would be silently unreachable, + // since the mux only carries what A's proxy had. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, }), ).rejects.toThrow(/different set of methods/i); expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); @@ -291,59 +350,145 @@ describe("clientAgentRegistry registration", () => { await register(registry, host, { instanceId: "a", connectionId: "conn-a", - appAgent: makeDevice().appAgent, + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, agentInterface: ["executeAction", "getDynamicDisplay"], }); await register(registry, host, { instanceId: "b", connectionId: "conn-b", - appAgent: makeDevice().appAgent, + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, agentInterface: ["getDynamicDisplay", "executeAction"], }); expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); }); - test("a client that declares no method set is unaffected by the check", async () => { + test("an empty method set is compared like any other", async () => { const registry = createClientAgentRegistry(); const host = makeHost(); await register(registry, host, { instanceId: "a", connectionId: "conn-a", - appAgent: makeDevice().appAgent, - agentInterface: ["executeAction"], + appAgent: makeDevice(["executeAction"]).appAgent, + }); + + // Nothing in common with the group, so it is a mismatch rather than an + // opt-out: the key for [] is the empty string, not undefined. + await expect( + register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice([]).appAgent, + }), + ).rejects.toThrow(/different set of methods/i); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + }); + + test("a device reconnecting with the same method set keeps its slot", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, }); await register(registry, host, { - instanceId: "b", - connectionId: "conn-b", - appAgent: makeDevice().appAgent, + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, }); - expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(2); + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + expect(host.added).toEqual([AGENT_NAME]); }); - test("a reconnecting instance cannot change the group's method set", async () => { + test("a lone device that upgrades its app changes the group's method set", async () => { const registry = createClientAgentRegistry(); const host = makeHost(); await register(registry, host, { instanceId: "a", connectionId: "conn-a", - appAgent: makeDevice().appAgent, - agentInterface: ["executeAction"], + appAgent: makeDevice(["executeAction"]).appAgent, + }); + expect(getMux(registry).getDynamicDisplay).toBeUndefined(); + + // Same device, same schema, new build that implements one more method. + // Nobody else is in the group, so there is no other device to conflict + // with and nothing for the user to disconnect. + const upgraded = makeDevice(["executeAction", "getDynamicDisplay"]); + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: upgraded.appAgent, + }); + + expect(registry.groups.get(AGENT_NAME)!.instances.size).toBe(1); + // The dispatcher still holds the object it was handed, so the new + // method has to show up on that same mux and route to the device. + const { context } = makeSessionContext("conn-a2"); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); + await getMux(registry).getDynamicDisplay!("html", "display-1", context); + expect(upgraded.dynamicDisplays).toEqual(["display-1"]); + }); + + test("a lone device that downgrades loses the method from the mux", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(["executeAction", "getDynamicDisplay"]) + .appAgent, + }); + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a2", + appAgent: makeDevice(["executeAction"]).appAgent, + }); + + // Leaving it on the mux would advertise a method no device can answer. + expect(getMux(registry).getDynamicDisplay).toBeUndefined(); + expect(getMux(registry).executeAction).toBeDefined(); + }); + + test("a reconnecting device cannot change a shared group's method set", async () => { + const registry = createClientAgentRegistry(); + const host = makeHost(); + const shared: AgentInterfaceFunctionName[] = [ + "executeAction", + "getDynamicDisplay", + ]; + + await register(registry, host, { + instanceId: "a", + connectionId: "conn-a", + appAgent: makeDevice(shared).appAgent, + }); + await register(registry, host, { + instanceId: "b", + connectionId: "conn-b", + appAgent: makeDevice(shared).appAgent, }); // Replacing in place keeps the mux built from the original proxy, so // the check has to cover a replacement too, not just a new instance. + // B is still there and still expects getDynamicDisplay to work. await expect( register(registry, host, { instanceId: "a", connectionId: "conn-a2", - appAgent: makeDevice().appAgent, - agentInterface: ["executeAction", "getDynamicDisplay"], + appAgent: makeDevice(["executeAction"]).appAgent, }), ).rejects.toThrow(/different set of methods/i); + expect(getMux(registry).getDynamicDisplay).toBeDefined(); }); // Case 12 From b65af8eac7a592172f4bd05c73bf354ddea782db Mon Sep 17 00:00:00 2001 From: Jebran Syed Date: Wed, 2 Sep 2026 16:23:32 -0700 Subject: [PATCH 3/3] Derive the Android agentInterface from the methods it dispatches The Android client hardcoded its agentInterface as ["executeAction"] while handleAndroidDeviceInvoke separately hardcoded the same string as its guard. Nothing tied the two together, and no CI job builds this module, so adding a method to one and not the other would go unnoticed. That drift is exactly what the server now rejects at join time, and it fails in the worse direction: a device that declares a method it cannot answer is routed the call and fails only when someone makes it. Both now come from AndroidDeviceAgent.SUPPORTED_METHODS. The test asserts the declared array against that list rather than a literal, and pins that an unimplemented method is not claimed - widening the list without adding dispatch fails the test. Verified by hand, since nothing in CI covers android/: gradlew assembleDebug and testDebugUnitTest both pass (171 tests). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../typeagentchat/AndroidDeviceAgent.kt | 23 ++++++++++++++++++- .../example/typeagentchat/WebSocketManager.kt | 5 +++- .../typeagentchat/AndroidDeviceAgentTest.kt | 14 +++++++---- 3 files changed, 36 insertions(+), 6 deletions(-) diff --git a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt index 778609ff3b..7897268e17 100644 --- a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt +++ b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/AndroidDeviceAgent.kt @@ -7,6 +7,24 @@ internal object AndroidDeviceAgent { const val NAME = "androidDevice" const val CHANNEL_NAME = "agent:$NAME" const val SCHEMA_ASSET = "typeagent/androidDeviceSchema.ts" + + /** + * Methods this agent answers on its RPC channel, sent as `agentInterface` + * at registration. + * + * The server builds its proxy from this list and, when several devices host + * `androidDevice`, rejects one whose list differs from the others. So it has + * to describe what `handleAndroidDeviceInvoke` really dispatches: declaring + * a method the device cannot answer fails only later, at the call. Keeping + * one list for both the declaration and the dispatch guard is what stops the + * two from drifting - nothing else checks them against each other, and no CI + * job builds this module. + */ + val SUPPORTED_METHODS = listOf("executeAction") + + /** Whether [SUPPORTED_METHODS] covers an incoming RPC method. */ + fun supports(methodName: String): Boolean = SUPPORTED_METHODS.contains(methodName) + private const val AGENT_DESCRIPTION = "Acts on this Android device: sets alarms and countdown timers, shows the " + "alarm and timer lists, searches for nearby places, shows a place on the " + @@ -34,11 +52,14 @@ internal object AndroidDeviceAgent { .put("actionDefaultEnabled", true) .put("schema", schema) + val agentInterface = JSONArray() + SUPPORTED_METHODS.forEach { agentInterface.put(it) } + return JSONObject() .put("name", NAME) .put("conversationId", conversationId) .put("manifest", manifest) - .put("agentInterface", JSONArray().put("executeAction")) + .put("agentInterface", agentInterface) // Identifies this device so several devices can share one // `androidDevice` agent, and so a reconnect replaces this device // instead of adding another. `multiInstance` is the opt-in: without diff --git a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt index 3756de3e2d..4e90c5f8c0 100644 --- a/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt +++ b/android/samples/mobile-2/app/src/main/java/com/example/typeagentchat/WebSocketManager.kt @@ -748,7 +748,10 @@ class WebSocketManager internal constructor( Log.e(TAG, "Android agent invocation is missing callId.") return } - if (methodName != "executeAction") { + // The same list registration declares as agentInterface, so the guard + // and the declaration cannot drift apart. It has one entry today; a + // second would need its own dispatch below, not just a line in the list. + if (!AndroidDeviceAgent.supports(methodName)) { sendRpcError( channelName, callId, diff --git a/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt b/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt index ed6f955fe3..4ab89b97ac 100644 --- a/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt +++ b/android/samples/mobile-2/app/src/test/java/com/example/typeagentchat/AndroidDeviceAgentTest.kt @@ -23,10 +23,16 @@ class AndroidDeviceAgentTest { assertEquals("instance-1", registration.getString("instanceId")) assertEquals("Pixel 8", registration.getString("displayName")) assertEquals(true, registration.getBoolean("multiInstance")) - assertEquals( - "executeAction", - registration.getJSONArray("agentInterface").getString(0) - ) + // The declared set must be exactly what the RPC dispatcher answers: the + // server builds its proxy from this and, with several devices hosting + // the agent, rejects one that declares a different set. + val declared = registration.getJSONArray("agentInterface") + assertEquals(AndroidDeviceAgent.SUPPORTED_METHODS.size, declared.length()) + for (index in AndroidDeviceAgent.SUPPORTED_METHODS.indices) { + assertEquals(AndroidDeviceAgent.SUPPORTED_METHODS[index], declared.getString(index)) + } + assertTrue(AndroidDeviceAgent.supports("executeAction")) + assertFalse(AndroidDeviceAgent.supports("getDynamicDisplay")) assertEquals( "export type AndroidDeviceAction = never;", registration