From 0f3ee265fc47c596297e3acdb824965e86be9c11 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Sat, 22 Aug 2026 22:55:43 -0400 Subject: [PATCH] fix(apps): bind app elicitations to the connection that made them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the three findings; the third is answered in the PR thread. **A replaced client could still enqueue.** The one-shot sweep only rejected what was already queued, but the outgoing `InspectorClient` disconnects asynchronously and can enqueue during its own teardown — and that late entry would be rendered by a factory bound to the REPLACEMENT client, reading its resource and answering through a different server. Requests now belong to a session: each constructed client gets one, closing it rejects that session's entries AND refuses anything it queues afterwards, and another session's entries are untouched. **The advertisement no longer has to guess.** Whether a sandbox exists is only known once `/api/config` resolves, and the answer is baked into the client at construction — guessing "available" over-claims, guessing "unavailable" strands the whole session on the native form despite having a sandbox. The connect path now awaits that fetch (already in flight since mount, so no human ever waits; it only orders a deep-link auto-connect racing the same page load), after which an absent `sandboxUrl` means confirmed-absent. Signed-off-by: cliffhall --- clients/web/src/App.tsx | 76 ++++++++++++++----- .../AppElicitationHost.stories.tsx | 1 + .../AppElicitationHost.test.tsx | 1 + .../AppElicitation/AppElicitationHost.tsx | 3 + .../src/lib/appElicitationController.test.ts | 73 +++++++++++++++--- .../web/src/lib/appElicitationController.ts | 70 +++++++++++++---- 6 files changed, 178 insertions(+), 46 deletions(-) diff --git a/clients/web/src/App.tsx b/clients/web/src/App.tsx index 8ad2dfd4..5a0a2bed 100644 --- a/clients/web/src/App.tsx +++ b/clients/web/src/App.tsx @@ -142,7 +142,10 @@ import { clearScrollMemory } from "./hooks/useScrollMemory"; import type { AppRendererHandle } from "./components/elements/AppRenderer/AppRenderer"; import { createAppBridgeFactory } from "./components/elements/AppRenderer/createAppBridgeFactory"; import { AppElicitationHost } from "./components/elements/AppElicitation/AppElicitationHost"; -import { AppElicitationController } from "./lib/appElicitationController"; +import { + AppElicitationController, + type AppElicitationSession, +} from "./lib/appElicitationController"; import type { LogEntryData } from "./components/elements/LogEntry/LogEntry"; import { ServerConfigModal, @@ -750,6 +753,7 @@ function App() { sandboxUrl, writable: serverListWritable, version: inspectorVersion, + loading: initialConfigLoading, } = useInitialConfig({ baseUrl: configBaseUrl, authToken: getAuthToken(), @@ -808,6 +812,31 @@ function App() { const appElicitationControllerRef = useRef(null); appElicitationControllerRef.current ??= new AppElicitationController(); const appElicitationController = appElicitationControllerRef.current; + // The window onto the controller for the CURRENT client. Closing it when the + // client is replaced both rejects that connection's queued requests and + // refuses any it enqueues during its own (asynchronous) teardown — a late + // entry would otherwise be rendered by a factory bound to the replacement + // client, i.e. read and answered through a different server. + const appElicitationSessionRef = useRef(null); + // Whether the sandbox exists is only known once `/api/config` resolves, and + // the answer is baked into the client at construction (it decides whether the + // nested MCP Apps `elicitation` capability is advertised). So a connect waits + // for it rather than guessing: guessing "available" over-claims a capability + // we may not have, and guessing "unavailable" strands the whole session on + // the native form despite having a sandbox. The wait is a local fetch already + // in flight since mount. + const initialConfigSettledRef = useRef<{ + promise: Promise; + resolve: () => void; + }>(null); + initialConfigSettledRef.current ??= (() => { + let resolve!: () => void; + const promise = new Promise((r) => (resolve = r)); + return { promise, resolve }; + })(); + useEffect(() => { + if (!initialConfigLoading) initialConfigSettledRef.current?.resolve(); + }, [initialConfigLoading]); const appElicitations = useSyncExternalStore( appElicitationController.subscribe, appElicitationController.getEntries, @@ -838,6 +867,19 @@ function App() { }), [inspectorClient], ); + /** + * Close the previous client's session and open one for the client being + * constructed. Synchronous, and called at construction, so the swap itself is + * the moment ownership changes hands. + */ + const newAppElicitationSession = useCallback(() => { + appElicitationSessionRef.current?.close( + new Error("Connection replaced before the app answered"), + ); + const session = appElicitationController.openSession(); + appElicitationSessionRef.current = session; + return session; + }, [appElicitationController]); const handleAppElicitationSettle = useCallback( (requestId: string, result: ElicitResult) => { appElicitationController.settle(requestId, result); @@ -850,20 +892,6 @@ function App() { }, [appElicitationController], ); - // Request ownership must not cross connections. `elicitationBridgeFactory` - // resolves its client at call time, so an entry queued by a previous - // InspectorClient would otherwise be able to rebuild against the NEXT one and - // read (or answer) through a different server. Core aborts its own pending - // app requests during teardown, but that teardown is awaited asynchronously - // while the swap here is synchronous — so drop them on the swap instead of - // racing it. The one client alive at a time owns every live entry. - useEffect(() => { - return () => { - appElicitationController.failAll( - new Error("Connection replaced before the app answered"), - ); - }; - }, [appElicitationController, inspectorClient]); const [managedToolsState, setManagedToolsState] = useState(null); @@ -2486,12 +2514,12 @@ function App() { // Web only, and only when the sandbox renderer is actually available: // supplying this advertises the nested MCP Apps `elicitation` // capability, and a client that cannot host an app must not claim it - // (#1854). `sandboxUrl` is undefined while `/api/config` is in flight - // and when the sandbox controller could not start, so this connection - // simply behaves like the CLI/TUI: the native elicitation queue, with - // no claim made to the server. + // (#1854). Callers await `initialConfigSettled` first, so `sandboxUrl` + // here means "confirmed absent" rather than "not known yet" — a + // connection that reaches this with no sandbox behaves like the + // CLI/TUI: native elicitation queue, no claim made to the server. ...(sandboxUrl && { - appElicitation: appElicitationController.render, + appElicitation: newAppElicitationSession().render, }), // Always advertise the roots capability (even with no configured // roots) so the server can issue roots/list and receive @@ -2594,7 +2622,7 @@ function App() { sessionStorageAdapter, onBeforeOAuthRedirect, clientConfig, - appElicitationController.render, + newAppElicitationSession, sandboxUrl, ], ); @@ -2815,6 +2843,12 @@ function App() { const onToggleConnection = useCallback( async (id: string) => { + // Whether this client may advertise app-rendered elicitation is decided + // at construction and cannot be revised afterwards, so wait for the fact + // rather than guess it (see `initialConfigSettledRef`). Already resolved + // by the time any human clicks; this only orders a deep-link auto-connect + // that races the same page load. + await initialConfigSettledRef.current?.promise; // Same server, already connected → disconnect. if ( id === activeServerId && diff --git a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.stories.tsx b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.stories.tsx index a2f653d1..ad06b36d 100644 --- a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.stories.tsx +++ b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.stories.tsx @@ -23,6 +23,7 @@ function entry( ): AppElicitationEntry { return { requestId, + sessionId: 0, resourceUri, params, signal: new AbortController().signal, diff --git a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.test.tsx b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.test.tsx index d11b28b5..420ecf08 100644 --- a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.test.tsx +++ b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.test.tsx @@ -62,6 +62,7 @@ function makeEntry( ): AppElicitationEntry { return { requestId, + sessionId: 0, resourceUri, params, signal: new AbortController().signal, diff --git a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx index b158a088..5b2ea014 100644 --- a/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx +++ b/clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx @@ -236,6 +236,9 @@ function AppElicitationFrame({ return ( (); +function defaultSession( + controller: AppElicitationController, +): AppElicitationSession { + let session = sessions.get(controller); + if (!session) { + session = controller.openSession(); + sessions.set(controller, session); + } + return session; } describe("AppElicitationController (#1854)", () => { @@ -108,18 +129,19 @@ describe("AppElicitationController (#1854)", () => { expect(controller.getEntries()).toHaveLength(0); }); - it("failAll drops every entry so a renderer cannot outlive its connection", async () => { - // The host calls this when the InspectorClient is replaced: the bridge - // factory resolves its client at call time, so an entry left queued could - // otherwise rebuild against the NEXT connection and answer through a + it("closing a session drops the entries that session queued", async () => { + // The host closes the old session when the InspectorClient is replaced: the + // bridge factory resolves its client at call time, so an entry left queued + // could otherwise rebuild against the NEXT connection and answer through a // different server. const controller = new AppElicitationController(); + const session = controller.openSession(); const listener = vi.fn(); - const first = request(controller, "a"); - const second = request(controller, "b"); + const first = request(controller, "a", "ui://a", undefined, session); + const second = request(controller, "b", "ui://b", undefined, session); controller.subscribe(listener); - controller.failAll(new Error("connection replaced")); + session.close(new Error("connection replaced")); await expect(first).rejects.toThrow(/connection replaced/); await expect(second).rejects.toThrow(/connection replaced/); @@ -127,11 +149,40 @@ describe("AppElicitationController (#1854)", () => { expect(listener).toHaveBeenCalledTimes(1); }); - it("failAll on an empty queue notifies nobody", () => { + it("refuses a request a closed session makes during its own teardown", async () => { + // The half a one-shot sweep cannot provide: the replaced client + // disconnects asynchronously and can still enqueue, and that entry would be + // rendered by a factory bound to the REPLACEMENT client. const controller = new AppElicitationController(); + const session = controller.openSession(); + session.close(new Error("connection replaced")); + await expect( + request(controller, "late", "ui://a", undefined, session), + ).rejects.toThrow(/session is closed/); + expect(controller.getEntries()).toHaveLength(0); + }); + + it("leaves another session's entries alone", async () => { + const controller = new AppElicitationController(); + const oldSession = controller.openSession(); + const newSession = controller.openSession(); + const stale = request(controller, "a", "ui://a", undefined, oldSession); + const live = request(controller, "b", "ui://b", undefined, newSession); + + oldSession.close(new Error("connection replaced")); + + await expect(stale).rejects.toThrow(/connection replaced/); + expect(controller.getEntries().map((e) => e.requestId)).toEqual(["b"]); + controller.settle("b", { action: "cancel" }); + await expect(live).resolves.toEqual({ action: "cancel" }); + }); + + it("closing a session with nothing queued notifies nobody", () => { + const controller = new AppElicitationController(); + const session = controller.openSession(); const listener = vi.fn(); controller.subscribe(listener); - controller.failAll(new Error("nothing to drop")); + session.close(new Error("nothing to drop")); expect(listener).not.toHaveBeenCalled(); }); diff --git a/clients/web/src/lib/appElicitationController.ts b/clients/web/src/lib/appElicitationController.ts index 49bdd12d..8ba26b43 100644 --- a/clients/web/src/lib/appElicitationController.ts +++ b/clients/web/src/lib/appElicitationController.ts @@ -13,12 +13,31 @@ import type { * resolve through each other's. */ export interface AppElicitationEntry extends AppElicitationRequest { + /** The session (one InspectorClient) this request belongs to. */ + sessionId: number; /** Hands the app's standard `ElicitResult` back to the server. */ resolve: (result: ElicitResult) => void; /** Asks `InspectorClient` to fall back to the native elicitation UI. */ reject: (error: Error) => void; } +/** + * One client's window onto the controller. + * + * Requests are bound to the connection that made them. A closed session + * rejects immediately, which is what a one-shot sweep cannot do: an + * `InspectorClient` being replaced disconnects asynchronously and can still + * enqueue during its own teardown, and that late entry would otherwise be + * rendered by a factory bound to the *replacement* client — reading its + * resource, and answering, through a different server. + */ +export interface AppElicitationSession { + /** The renderer handed to this client's `InspectorClient`. */ + render: AppElicitationRenderer; + /** Reject everything from this session and refuse anything later. */ + close: (error: Error) => void; +} + /** * Bridges `InspectorClient`'s renderer callback — supplied at construction, * long before any React tree exists — to the React component that actually @@ -32,6 +51,9 @@ export interface AppElicitationEntry extends AppElicitationRequest { export class AppElicitationController { private entries: AppElicitationEntry[] = []; private listeners = new Set<() => void>(); + private nextSessionId = 0; + /** Sessions still accepting requests. A closed one is removed. */ + private openSessions = new Set(); /** Current queue. Stable identity between changes, for `useSyncExternalStore`. */ getEntries = (): AppElicitationEntry[] => this.entries; @@ -48,16 +70,38 @@ export class AppElicitationController { } /** - * The {@link AppElicitationRenderer} handed to `InspectorClient`. Queues the - * request for the UI and resolves when {@link settle} or {@link fail} is - * called for it. + * Open a window for one `InspectorClient`. Its `render` is what that client + * is constructed with; `close` ends it when the client is replaced. + */ + openSession(): AppElicitationSession { + const sessionId = this.nextSessionId++; + this.openSessions.add(sessionId); + return { + render: (request) => this.render(sessionId, request), + close: (error) => this.closeSession(sessionId, error), + }; + } + + /** + * Queue a request for the UI and resolve when {@link settle} or {@link fail} + * is called for it. * * An abort of the originating request (cancelled tool call, disconnect) drops * the entry and rejects, so a modal can never outlive the request behind it. + * A request from a closed session is rejected without ever being queued. */ - render: AppElicitationRenderer = (request: AppElicitationRequest) => + private render = (sessionId: number, request: AppElicitationRequest) => new Promise((resolve, reject) => { - const entry: AppElicitationEntry = { ...request, resolve, reject }; + if (!this.openSessions.has(sessionId)) { + reject(new Error("App elicitation session is closed")); + return; + } + const entry: AppElicitationEntry = { + ...request, + sessionId, + resolve, + reject, + }; const onAbort = () => { this.remove(entry.requestId); reject(new Error("App-rendered elicitation aborted")); @@ -98,17 +142,15 @@ export class AppElicitationController { } /** - * Drop every queued request at once. - * - * For the host's "these entries no longer belong to the live connection" - * moment (a replaced `InspectorClient`): a renderer that outlived its client - * would rebuild against the *next* one and could read or answer through a - * different server. + * End a session: reject everything it queued, and refuse anything it queues + * later. The refusal is the half a one-shot sweep cannot provide — see + * {@link AppElicitationSession}. */ - failAll(error: Error): void { - const dropped = this.entries; + private closeSession(sessionId: number, error: Error): void { + this.openSessions.delete(sessionId); + const dropped = this.entries.filter((e) => e.sessionId === sessionId); if (dropped.length === 0) return; - this.entries = []; + this.entries = this.entries.filter((e) => e.sessionId !== sessionId); this.emit(); for (const entry of dropped) entry.reject(error); }