fix(apps): address review of app-rendered elicitations
Eight findings from the Copilot review, all accepted.
Ownership across connections was the substantive one, in three parts. A
mid-session transport close reaches teardown only through
`clearAndAnnouncePendingPeerRequests`, whose emptiness check looked at the
native queues alone — so a lone app elicitation survived a dropped connection
with its modal still open; the check now counts the active set too. The web
bridge factory resolves its client at call time, so an entry queued by a
replaced `InspectorClient` could rebuild against the *next* one and answer
through a different server; the host now drops every entry synchronously on
the swap (`AppElicitationController.failAll`) rather than racing core's
awaited teardown. And the request id was only monotonic within one client, so
each replacement client's first request was `app-elicitation-1` — settling
that id could resolve the wrong server's request. It now carries a
per-instance prefix.
Result validation now parses the whole value with `ElicitResultSchema` before
the requested-schema check, instead of inspecting `action` by hand: a
`{ action: "decline", content: { x: {} } }` used to pass, though the standard
result permits only primitives and string arrays in `content`.
The dialog's accessible name now includes the request id and the prompt — two
concurrent elicitations can be for the same app URI, and the previous name
made those indistinguishable, which is what the comment beside it claimed to
prevent.
`observeAppCapabilities` takes a minimal structural `onmessage` contract
(generic over the message type, since a handler typed for a narrower message
is not assignable to one typed for `unknown`), removing the double cast at the
call site.
`smoke:web:elicit` now fails on an uncaught page error after a successful
drive, and captures the async half of that class off the console channel, as
its two sibling smokes do — it could otherwise print OK over a broken bundle.
Adds the tests each fix needs, including the two the review asked for
directly: that `advertiseElicitation` reaches the `AppBridge` constructor, and
that it is absent by default.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
08fc20a22d
commit
7c77c2b15f
2 changed files with 286 additions and 38 deletions
|
|
@ -6,6 +6,7 @@ import type {
|
|||
jsonSchemaValidator,
|
||||
ServerCapabilities,
|
||||
} from "@modelcontextprotocol/client";
|
||||
import { ElicitResultSchema } from "@modelcontextprotocol/core";
|
||||
import { MCP_APP_MIME_TYPE, UI_EXTENSION_KEY } from "./extensions.js";
|
||||
|
||||
/**
|
||||
|
|
@ -149,8 +150,13 @@ export function isFormElicitation(params: ElicitRequest["params"]): boolean {
|
|||
return mode === undefined || mode === "form";
|
||||
}
|
||||
|
||||
/** The three actions a completed elicitation may report. */
|
||||
const ELICIT_ACTIONS = ["accept", "decline", "cancel"] as const;
|
||||
/**
|
||||
* The result contract a completed elicitation must satisfy — the standard MCP
|
||||
* one, not a hand-rolled subset. Checking `action` by hand would accept, for
|
||||
* instance, `{ action: "decline", content: { x: {} } }`, whose content the
|
||||
* schema forbids, and hand the server a result it can reject.
|
||||
*/
|
||||
const ELICIT_RESULT_ERROR = "App returned an invalid elicitation result";
|
||||
|
||||
/**
|
||||
* Validates what an app returned before it is handed back to the server.
|
||||
|
|
@ -170,16 +176,13 @@ export function validateAppElicitResult(
|
|||
params: ElicitRequest["params"],
|
||||
result: ElicitResult,
|
||||
): string | undefined {
|
||||
if (typeof result !== "object" || result === null) {
|
||||
return "App returned a non-object elicitation result";
|
||||
}
|
||||
const action = (result as { action?: unknown }).action;
|
||||
if (!ELICIT_ACTIONS.includes(action as (typeof ELICIT_ACTIONS)[number])) {
|
||||
return `App returned an unknown elicitation action: ${JSON.stringify(action)}`;
|
||||
const parsed = ElicitResultSchema.safeParse(result);
|
||||
if (!parsed.success) {
|
||||
return `${ELICIT_RESULT_ERROR}: ${parsed.error.issues[0]?.message ?? "does not match ElicitResult"}`;
|
||||
}
|
||||
// decline / cancel carry no content — they are complete as they stand.
|
||||
if (action !== "accept") return undefined;
|
||||
const content = (result as { content?: unknown }).content;
|
||||
if (parsed.data.action !== "accept") return undefined;
|
||||
const content = parsed.data.content;
|
||||
if (
|
||||
typeof content !== "object" ||
|
||||
content === null ||
|
||||
|
|
|
|||
|
|
@ -584,6 +584,17 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
* elicitations cannot resolve through each other's bridges.
|
||||
*/
|
||||
private appElicitationSeq = 0;
|
||||
/**
|
||||
* Per-instance prefix for {@link appElicitationSeq}.
|
||||
*
|
||||
* A counter alone is not enough: a host may hold ONE renderer across
|
||||
* replacement clients (the web client rebuilds its `InspectorClient` on a
|
||||
* settings change), and every fresh instance would otherwise mint
|
||||
* `app-elicitation-1` for its first request. Settling that id would then
|
||||
* resolve — or discard — whichever connection's request the host happened to
|
||||
* be keying.
|
||||
*/
|
||||
private readonly appElicitationPrefix = `app-elicitation-${globalThis.crypto?.randomUUID?.() ?? Math.random().toString(36).slice(2)}`;
|
||||
/**
|
||||
* Abort controllers for app-rendered elicitations still awaiting an answer.
|
||||
* Aborted alongside the native pending queue on disconnect, so a rendered app
|
||||
|
|
@ -1755,7 +1766,12 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
private clearAndAnnouncePendingPeerRequests(): void {
|
||||
if (
|
||||
this.pendingSamples.length === 0 &&
|
||||
this.pendingElicitations.length === 0
|
||||
this.pendingElicitations.length === 0 &&
|
||||
// App-rendered elicitations (#1854) are not in either array — they live in
|
||||
// the host's renderer — so an emptiness check that ignores them lets a
|
||||
// mid-session close (the `onclose` route, which reaches teardown only
|
||||
// through here) leave a modal open for a connection that is gone.
|
||||
this.activeAppElicitations.size === 0
|
||||
) {
|
||||
return;
|
||||
}
|
||||
|
|
@ -2946,7 +2962,7 @@ export class InspectorClient extends InspectorClientEventTarget {
|
|||
this.activeAppElicitations.add(controller);
|
||||
try {
|
||||
const result = await renderer({
|
||||
requestId: `app-elicitation-${++this.appElicitationSeq}`,
|
||||
requestId: `${this.appElicitationPrefix}-${++this.appElicitationSeq}`,
|
||||
resourceUri,
|
||||
params: request.params,
|
||||
signal: controller.signal,
|
||||
|
|
|
|||
Loading…
Reference in a new issue