fix(apps): bind app elicitations to the connection that made them
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 <cliff@futurescale.com>
This commit is contained in:
parent
d97c9bd391
commit
0f3ee265fc
2 changed files with 178 additions and 46 deletions
|
|
@ -1,19 +1,40 @@
|
|||
import { describe, it, expect, vi } from "vitest";
|
||||
import type { ElicitRequest } from "@modelcontextprotocol/client";
|
||||
import { AppElicitationController } from "./appElicitationController";
|
||||
import {
|
||||
AppElicitationController,
|
||||
type AppElicitationSession,
|
||||
} from "./appElicitationController";
|
||||
|
||||
const params: ElicitRequest["params"] = {
|
||||
message: "Choose an option",
|
||||
requestedSchema: { type: "object", properties: {} },
|
||||
};
|
||||
|
||||
/**
|
||||
* Queue a request through a session. Defaults to one session per controller,
|
||||
* which is what a single connection looks like; the session tests open their
|
||||
* own to model a client swap.
|
||||
*/
|
||||
function request(
|
||||
controller: AppElicitationController,
|
||||
requestId: string,
|
||||
resourceUri = "ui://demo/pick.html",
|
||||
signal: AbortSignal = new AbortController().signal,
|
||||
session: AppElicitationSession = defaultSession(controller),
|
||||
) {
|
||||
return controller.render({ requestId, resourceUri, params, signal });
|
||||
return session.render({ requestId, resourceUri, params, signal });
|
||||
}
|
||||
|
||||
const sessions = new WeakMap<AppElicitationController, AppElicitationSession>();
|
||||
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();
|
||||
});
|
||||
|
||||
|
|
|
|||
|
|
@ -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<number>();
|
||||
|
||||
/** 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<ElicitResult>((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);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue