feat(apps): negotiated app-rendered form elicitations

[cliffhall]

Closes #1854

Implements negotiated, app-rendered form elicitations: a server may attach an MCP App to a standard elicitation/create, and a host that can run one renders it and returns the app's ordinary ElicitResult. The native Inspector elicitation form stays the fallback on every failure.

No second extension, no custom method, no custom result shape. The only new wire surface is a nested elicitation flag on the existing io.modelcontextprotocol/ui extension on each side, plus _meta.ui.resourceUri on the request.

Proof

The server's app renders the form in the sandbox instead of the Inspector's own:

Clicking Option A inside the sandboxed app returns the standard result to the server, which echoes it back in the tool result — so this is the round trip, not just the host:

The same tool against a server that did not advertise the nested capability falls back to the built-in form. This is the half that matters most — a client that over-claims the capability strands every user of a server that never opted in:

All three are captured by npm run smoke:web:elicit (headless Chromium, in npm run smoke), which drives connect → call the tool → answer inside the app → assert the result, then repeats against the non-negotiating server.

The contract

App rendering is selected only when all four gates hold:

1. the client advertises elicitation.form;
2. the client advertises the MCP Apps MIME type;
3. both peers advertise the nested elicitation setting on io.modelcontextprotocol/ui;
4. the request carries a valid absolute ui:// URI in _meta.ui.resourceUri.

Only the web client advertises the nested client-side setting, and only because it has a sandbox renderer to back it. Supplying InspectorClientOptions.appElicitation is what opts a client in, so CLI and TUI keep advertising the MIME type (they know what an App is) without ever claiming they can resolve an elicitation through one. Disabling form elicitation drops both capabilities; turning the Apps extension off in Server Settings drops the nested one with it.

Routing goes in the one funnel both entry points already share (enqueuePendingElicitation), so the inbound elicitation/create handler and the MRTR driver cannot diverge on it. Ownership is request-scoped: a per-request id keys the renderer, iframe and bridge, so two concurrent elicitations — even for the same app — cannot resolve through each other's bridges.

An explicit decline or cancel is a completed elicitation and is returned to the server. Everything else falls back: absent/malformed/non-ui:// metadata, url mode, a failed resource read, sandbox or bridge init failure, an app that did not advertise elicitation, a timeout, an invalid result, or accepted content that fails the requested schema.

On the upstream dependency

The issue asks that this consume ext-apps#733 rather than inventing an Inspector-local variant. That PR is still open and the released @modelcontextprotocol/ext-apps (1.7.5) exports none of it, so this speaks exactly its wire protocol — same method, same params, same result, same capability keys — while mirroring the two helpers it needs locally:

• core/mcp/appElicitation.ts (negotiation gates, _meta.ui.resourceUri reader/validator, result validation)
• clients/web/src/components/elements/AppRenderer/requestAppElicitation.ts (the bridge send that becomes bridge.requestElicitation(params))

Both are marked for deletion when a release containing #733 ships; nothing on the wire changes then. Pinning the repo to a git ref of an unmerged PR was the alternative and was rejected — the root manifest is what a published install resolves against.

One consequence of the version gap needed its own seam. ext-apps 1.7.5 parses the view's ui/initialize through a schema that strips elicitation, so an app that correctly advertises it reads, through bridge.getAppCapabilities(), as one that did not — silently turning every negotiated elicitation into a fallback (this is what the first end-to-end run actually hit). AppRenderer/appCapabilities.ts records the raw frame instead, and prefers the bridge's own value once it carries the key.

Notable changes

• AppRenderer takes an AppRenderSource union rather than a Tool. An elicitation has no tool — the server names the resource on the request — so the same renderer, bridge factory and sandbox lifecycle now serve both without either faking the other's shape. Reuse still keys on Tool identity for the tool case, so a re-listed tool rebuilds exactly as before.
• The elicitation bridge factory is a separate instance that advertises hostCapabilities.elicitation; an App-tool frame is never handed an elicitation, so claiming it there would tell those apps something untrue.
• The modal is headerless by construction. Concurrent elicitations mean two dialogs are open at once, and Mantine's modal header is a <header> — a duplicate banner landmark that axe rejects. Title and close live in the body, and each dialog is named by its own request.

Fixture

app_choose_option plus a self-contained ui://demo/choose-option.html app (accept / decline / cancel), served by test-servers/configs/app-elicitation-http.json. Its -native- sibling is the same tool and app on a server that never advertises the capability — that pair is what makes the negotiation observable rather than asserted.

Tests

• Capability serialization: web opt-in, form-disabled, and the non-rendering (CLI/TUI) client.
• All four negotiation gates, driven through the real inbound handler over a fake transport.
• Renderer/bridge: the request reaches the exact app instance and its result comes back unchanged.
• Two simultaneous requests for different resource URIs, answered out of order, cannot cross.
• Every fallback route, plus decline/cancel not falling back.
• Live integration against the fixture over a real transport (src/test/integration/mcp/appElicitation.test.ts) — this is where a _meta dropped by the SDK's own elicitInput would surface.
• Storybook stories for the single, concurrent and render-failure states.
• npm run ci passes locally (validate → coverage → build gate → smokes → Storybook).

[Copilot]

Pull request overview

Adds negotiated MCP App-rendered form elicitations to the web client while retaining native UI fallback.

Changes:

• Adds capability negotiation, validation, routing, and request-scoped rendering.
• Generalizes the App renderer and introduces elicitation modals/controllers.
• Adds fixtures, tests, smoke coverage, and documentation.

Reviewed changes

Copilot reviewed 38 out of 38 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
AGENTS.md	Documents the new core feature.
README.md	Documents usage, negotiation, and smoke coverage.
package.json	Registers the new smoke test.
scripts/smoke-web-elicitation.mjs	Exercises app and fallback flows.
test-servers/configs/app-elicitation-http.json	Adds negotiated fixture configuration.
test-servers/configs/app-elicitation-native-http.json	Adds fallback fixture configuration.
test-servers/src/composable-test-server.ts	Advertises server elicitation capability.
test-servers/src/load-config.ts	Defines the fixture option.
test-servers/src/preset-registry.ts	Registers fixture presets.
test-servers/src/resolve-config.ts	Resolves the fixture option.
test-servers/src/test-server-fixtures.ts	Implements the tool and app resource.
core/mcp/appElicitation.ts	Adds negotiation and result validation helpers.
core/mcp/extensions.ts	Adds client capability advertisement.
core/mcp/inspectorClient.ts	Routes and tracks app elicitations.
core/mcp/types.ts	Exposes the renderer option.
clients/web/README.md	Documents automation attributes.
clients/web/src/App.tsx	Wires the web renderer and controller.
clients/web/src/lib/appElicitationController.ts	Manages pending app requests.
clients/web/src/lib/appElicitationController.test.ts	Tests controller lifecycle and concurrency.
clients/web/src/components/screens/AppsScreen/AppsScreen.tsx	Adopts generalized render sources.
clients/web/src/components/elements/AppRenderer/AppRenderer.tsx	Generalizes rendering and forwards elicitations.
clients/web/src/components/elements/AppRenderer/AppRenderer.test.tsx	Tests generalized rendering behavior.
clients/web/src/components/elements/AppRenderer/AppRenderer.stories.tsx	Updates renderer stories.
clients/web/src/components/elements/AppRenderer/appCapabilities.ts	Preserves raw app capabilities.
clients/web/src/components/elements/AppRenderer/appCapabilities.test.ts	Tests capability observation.
clients/web/src/components/elements/AppRenderer/appRenderSource.ts	Defines tool/resource render sources.
clients/web/src/components/elements/AppRenderer/appRenderSource.test.ts	Tests source identity and titles.
clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.ts	Adds resource sources and host capability wiring.
clients/web/src/components/elements/AppRenderer/createAppBridgeFactory.test.ts	Updates factory tests for source inputs.
clients/web/src/components/elements/AppRenderer/requestAppElicitation.ts	Sends standard bridge requests.
clients/web/src/components/elements/AppRenderer/requestAppElicitation.test.ts	Tests bridge request behavior.
clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx	Renders request-specific elicitation modals.
clients/web/src/components/elements/AppElicitation/AppElicitationHost.test.tsx	Tests rendering and fallback paths.
clients/web/src/components/elements/AppElicitation/AppElicitationHost.stories.tsx	Adds elicitation UI stories.
clients/web/src/test/core/mcp/appElicitation.test.ts	Tests negotiation and validation helpers.
clients/web/src/test/core/mcp/extensions.test.ts	Tests capability serialization.
clients/web/src/test/core/mcp/inspectorClient-app-elicitation.test.ts	Tests routing, fallback, and teardown.
clients/web/src/test/integration/mcp/appElicitation.test.ts	Tests the live server round trip.

---

💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.

[cliffhall]

Review round 1 — all eight findings addressed (7c77c2b1)

Mirroring the inline replies at PR level, since they get folded away once the threads go outdated.

Three of the eight were the same underlying problem — ownership across connections — and together they were the real find:

1. clearAndAnnouncePendingPeerRequests ignored app elicitations. onclose reaches teardown only through that helper, so a connection dropped mid-session with a lone app elicitation pending left the modal alive for a connection that was gone. activeAppElicitations.size is now part of its emptiness check, with a test that drives transport.onclose().
2. A replaced InspectorClient could be answered through by an old entry. The web bridge factory resolves its client at call time, and core's abort is awaited asynchronously while the React swap is synchronous. The host now drops every entry synchronously on the swap (AppElicitationController.failAll, from an effect cleanup keyed on inspectorClient) rather than racing that teardown.
3. Request ids were only unique within one client. Each replacement client's first request was app-elicitation-1, so settling that id could resolve the wrong server's request. Ids now carry a per-instance prefix.

The rest:

4. Result validation now parses the whole value with ElicitResultSchema before the requested-schema check. { action: "decline", content: { x: {} } } used to pass — the standard result permits only primitives and string arrays in content. That exact case is now a test.
5. The dialog's accessible name is genuinely per request (request id + prompt + URI). Two concurrent elicitations can be for the same app URI, which the previous name could not distinguish — the thing its own comment claimed to prevent.
6. smoke:web:elicit fails on an uncaught page error after a successful drive, and captures the async half of that class off the console channel, matching its two sibling smokes. It could otherwise have printed OK over a broken bundle.
7. The Transport double cast is gone. observeAppCapabilities takes a minimal structural onmessage contract — generic over the message type, because a handler typed for a narrower message is not assignable to one typed for unknown and that would have reintroduced the cast.
8. Added the two factory tests asked for: advertiseElicitation: true reaches the AppBridge constructor as hostCapabilities.elicitation, and the default does not.

npm run ci passes locally on the updated branch (validate → coverage → build gate → smokes → Storybook).

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 2 comments.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/App.tsx:2490

• appElicitation is supplied even when sandboxUrl is unavailable. The sandbox controller explicitly degrades to sandboxUrl: undefined when it cannot bind, and useInitialConfig also starts with it undefined, so this client can advertise io.modelcontextprotocol/ui.elicitation even though AppElicitationHost can only reject every request. Gate this option on confirmed sandbox availability (and account for the async config load) so the capability is never over-claimed.

        // Web only: hands form elicitations that name a `ui://` resource to the
        // MCP App the server chose, and advertises the nested MCP Apps
        // `elicitation` capability that makes a server willing to send one
        // (#1854). The native elicitation queue stays the fallback.
        appElicitation: appElicitationController.render,

clients/web/src/components/elements/AppRenderer/appCapabilities.ts:91

• The raw frame is untrusted, but this truthiness check accepts malformed capability values such as elicitation: true or elicitation: []. Those do not satisfy the draft's object capability schema, yet the host will forward the form instead of failing closed to the native UI. Validate the selected parsed/raw value as a non-null, non-array object.

export function appAdvertisesElicitation(bridge: AppBridge): boolean {
  const parsed = bridge.getAppCapabilities() as
    | Record<string, unknown>
    | undefined;
  if (parsed?.elicitation) return true;
  return Boolean(rawAppCapabilities.get(bridge)?.elicitation);

[cliffhall]

Review round 2 — both findings addressed (44010c31)

1. The modern MRTR leg was untested. The routing change sits in the funnel both entry points share, but only the legacy inbound handler was actually driven. Added mrtr_app_choose_option — a modern MRTR tool whose embedded elicitation carries _meta.ui.resourceUri — and three live tests over a real transport:

   • the app answers, the retry carries that answer through inputResponses, and the server echoes it as the tool's result (so the assertion is on the completed original call, not on the host);
   • the tool call is cancelled while the app is up, and the request-scoped signal aborts;
   • the same tool against a modern server that never advertised the capability goes to the native queue.

   Writing the cancellation case surfaced a real gap: tryAppElicitation would mount an app for an already-aborted signal — the caller cancelled during an earlier MRTR round — leaving a modal nobody was waiting on and a promise that could never settle. It now throws the abort straight through.

2. README smoke count. "two" → "three", with the elicitation smoke listed alongside the other two instead of tacked on.

npm run ci passes locally on 44010c31.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

test-servers/src/test-server-fixtures.ts:582

• This App resource is published as generic text/html, so the public fixture is not a conforming MCP App resource. The upstream registerAppResource contract defaults App resources to text/html;profile=mcp-app; using plain HTML means this end-to-end test can pass even though a MIME-validating host would reject the fixture. Use the MCP Apps MIME type here.
  clients/web/src/App.tsx:2490
• The web client opts into the nested capability even when sandboxUrl is unavailable. useInitialConfig explicitly treats an omitted URL (including sandbox-controller startup failure or config-fetch failure) as “Apps unavailable” (core/react/useInitialConfig.ts:7-12), but every subsequently created client still advertises app elicitation here. That tells servers to choose an app path this host cannot render. Only supply appElicitation when the sandbox is available (and account for sandboxUrl in the client-factory dependencies).

        // Web only: hands form elicitations that name a `ui://` resource to the
        // MCP App the server chose, and advertises the nested MCP Apps
        // `elicitation` capability that makes a server willing to send one
        // (#1854). The native elicitation queue stays the fallback.
        appElicitation: appElicitationController.render,

[cliffhall]

Right on both counts — fixed in 9b775b04. AGENTS.md now lists smoke:web:elicit in the npm run smoke chain and credits the shared prod-web-server.mjs helper with four consumers rather than three, and the new smoke has its own entry beside its siblings: what it drives, why the not-negotiated half is the load-bearing one, and the two mechanics that are easy to get wrong (the main-view tabs are a Mantine SegmentedControl, so there is no role="tab" — the clickable element is the sibling label[for$="-Tools"]; and the prompt string also appears in the hidden Protocol-tab payload, so the fallback assertion is scoped to the dialog).

.github/copilot-instructions.md deliberately does not change: it is a distillation that references npm run smoke as a whole and never enumerates the individual smokes, and this adds no rule a reviewer would cite against a diff.

[cliffhall]

Review round 3 — addressed (9b775b04)

The one finding was real: adding a command left the source-of-truth docs stale. AGENTS.md now lists smoke:web:elicit in the npm run smoke chain, credits the shared prod-web-server.mjs helper with four consumers rather than three, and gives the new smoke its own entry beside its siblings — what it drives, why the not-negotiated half is the load-bearing one, and the two mechanics that are easy to get wrong (the main-view tabs are a Mantine SegmentedControl with no role="tab"; the prompt string also appears in the hidden Protocol-tab payload, so the fallback assertion is scoped to the dialog).

.github/copilot-instructions.md is intentionally unchanged — it is a distillation that references npm run smoke as a whole and never enumerates the individual smokes, and this adds no rule a reviewer would cite against a diff.

Documentation-only, so the gate outcome is unchanged from 44010c31, where npm run ci passed.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

[cliffhall]

Review round 4 — addressed (cc0770ba)

Real, and it undercut the "fail closed" claim the module makes about itself: the observer recorded params.appCapabilities from any frame whose method was ui/initialize, before the bridge validated it — so a view could send a second, malformed initialize (one the bridge rejects, keeping the capabilities it had already accepted) and flip elicitation on here, after which the host would forward a request the bridge never negotiated.

Now:

• only a well-formed initialize request is recorded — a JSON-RPC id plus the handshake's required protocolVersion and appInfo;
• only the first one is recorded, mirroring the bridge, so no later frame can change what the gate reports;
• a malformed first frame records nothing.

Kept deliberately as a gate rather than a second copy of the bridge's schema — the bridge stays the authority on the rest of the frame, and duplicating it here would create the very drift this shim exists to survive. appCapabilities is still read off the original value, so the old schema cannot strip it.

npm run ci passes locally on cc0770ba.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/App.tsx:2490

• The renderer is supplied unconditionally, so InspectorClient advertises app-rendered elicitation even when sandboxUrl is unavailable. useInitialConfig explicitly leaves that value undefined until loading completes and for legacy/builds without a sandbox controller, where this host cannot render an app. Only pass appElicitation when sandboxUrl exists (and add it to this callback's dependencies) so the wire capability reflects the renderer actually available for that connection.

        // Web only: hands form elicitations that name a `ui://` resource to the
        // MCP App the server chose, and advertises the nested MCP Apps
        // `elicitation` capability that makes a server willing to send one
        // (#1854). The native elicitation queue stays the fallback.
        appElicitation: appElicitationController.render,

[cliffhall]

Review round 5 — addressed (b86db5c7)

Correct, and it reverses my round-4 assumption. Checked against ext-apps 1.7.5 rather than taking it on trust — _oninitialize says so itself:

if (this._appInfo !== void 0)
  console.warn("[ext-apps] AppBridge received a second ui/initialize. … Responding normally; the latest appInfo/appCapabilities replace the previous values.");
return this._appCapabilities = X.params.appCapabilities, this._appInfo = X.params.appInfo, …

Freezing at the first handshake therefore left the gate reporting capabilities the bridge no longer holds — including the exact case named: elicitation advertised, then a re-handshake without it, still reading as advertised.

Now every frame that passes the accept gate replaces the recorded value, and appCapabilities joins protocolVersion and appInfo in that gate since the bridge's own initialize schema requires all three. A frame the bridge would reject still records nothing and leaves the previous value alone — neither a route to setting the flag nor to clearing one the bridge still holds. Round 4's guarantee survives; only the freeze is gone.

npm run ci passes locally on b86db5c7.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

Previously missed (3) — in code that hasn't changed since the last review.

core/mcp/inspectorClient.ts:2892

• The legacy inbound handler still calls this funnel without its request-context signal (inspectorClient.ts:1430-1480). When the server times out or cancels elicitation/create, ctx.mcpReq.signal aborts, but this app request remains active (potentially for the 10-minute bridge timeout) and can leave a modal for an expired server request. Thread the handler context signal into enqueuePendingElicitation, as the MRTR path already does.

    const appResult = await this.tryAppElicitation(request, signal);

clients/web/src/App.tsx:2490

• This always opts the client into app-rendered elicitation even when sandboxUrl is unavailable. useInitialConfig explicitly leaves that value undefined before loading, for legacy backends, and for builds without the sandbox controller; in those states this client advertises a capability it cannot provide and only fails after accepting a server request. Supply appElicitation only when the sandbox URL is available (and include that state in the client-factory dependencies).

        // Web only: hands form elicitations that name a `ui://` resource to the
        // MCP App the server chose, and advertises the nested MCP Apps
        // `elicitation` capability that makes a server willing to send one
        // (#1854). The native elicitation queue stays the fallback.
        appElicitation: appElicitationController.render,

package.json:68

• Adding the fourth web-smoke consumer leaves the shared helper's documentation stale: scripts/lib/prod-web-server.mjs:4-9, :32, and :94 still describe only three web smokes and omit smoke:web:elicit. Update those references so the helper's consumer/isolation contract matches this command chain.

    "smoke": "npm run smoke:launcher && npm run smoke:cli && npm run smoke:tui && npm run smoke:web && npm run smoke:web:browser && npm run smoke:web:app && npm run smoke:web:elicit",

clients/web/src/App.tsx:866

• A passive-effect cleanup is not synchronous with setInspectorClient: React runs cleanup/setup for the already-committed tree before flushing the external-store update. During that pass, an existing AppRenderer can set up with the new elicitationBridgeFactory while its old entry is still rendered, recreating the cross-connection resource-read/bridge race this cleanup is intended to prevent. Drop entries in the synchronous client-replacement path before installing the new client, or bind each entry to its originating client.

  useEffect(() => {
    return () => {
      appElicitationController.failAll(
        new Error("Connection replaced before the app answered"),
      );
    };
  }, [appElicitationController, inspectorClient]);

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

[cliffhall]

Review round 8 — addressed (d97c9bd3)

Real, and it turns out to predate this PR: a native answer arriving after tasks/cancel overwrote cancelled the same way — the app path just gave it a second door.

• Each ReceiverTaskRecord now owns an AbortController, aborted by cancelReceiverTask and by clearReceiverTasks (session teardown).
• The task-augmented branch passes that signal to tryAppElicitation and wires it to the native queue entry, so both answer routes are torn down identically — no modal outlives the task it belongs to.
• completeTask / failTask return early on a terminal status, so a late answer through either route cannot resurrect the task.

Test: cancel while the app is up, assert the renderer's signal aborted and the task reads cancelled over tasks/list, then settle the app's promise anyway and assert the status is still exactly cancelled.

npm run ci passes locally on d97c9bd3.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (3)

Previously missed (2) — in code that hasn't changed since the last review.

clients/web/src/App.tsx:2495

• sandboxUrl is also undefined while the initial /api/config request is still in flight. If a user clicks Connect—or an existing deep-link catalog row auto-connects—before that fetch resolves, this client is permanently constructed without appElicitation; when sandboxUrl arrives, the live client is not rebuilt, so the web session silently uses native fallback despite having a sandbox. Gate client construction on the initial-config loading state (while still allowing a confirmed missing sandbox), or reconnect/rebuild when availability becomes known.

        ...(sandboxUrl && {
          appElicitation: appElicitationController.render,
        }),

clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx:240

• opened is a static behavioral prop on this extracted Mantine subcomponent. Under the repository's .withProps() rule, static props must be baked into ElicitationModal; add opened: true to its declaration and remove this call-site prop.

    <ElicitationModal
      opened
      onClose={dismiss}

clients/web/src/App.tsx:866

• This cleanup only rejects entries that already exist at the client swap. The old InspectorClient.disconnect() is fire-and-forget, and core does not abort/clear app requests until after await client.close() (core/mcp/inspectorClient.ts:2343-2362), so the old client can still enqueue a new request through this shared controller during teardown. That new entry is then rendered by the factory bound to the replacement client, allowing its resource read/answer to cross server connections. Bind each request/controller to its originating client (or synchronously close the old controller to future renders), rather than relying on a one-time failAll.

  useEffect(() => {
    return () => {
      appElicitationController.failAll(
        new Error("Connection replaced before the app answered"),
      );
    };
  }, [appElicitationController, inspectorClient]);

[cliffhall]

Review round 9 — the three suppressed findings (0f3ee265)

1. A replaced client could still enqueue during its own teardown. Correct — the one-shot sweep only rejected what was already queued, and a late entry would then be rendered by a factory bound to the replacement client. Requests now belong to a session: each constructed client gets one; closing it rejects that session's entries and refuses anything it queues afterwards; another session's entries are untouched. Three tests cover it, including the specific "enqueues after close" case.

2. The advertisement had to guess while /api/config was in flight. Also correct, and the exact mirror of round 7 — guessing "available" over-claims, guessing "unavailable" strands the whole session on the native form despite having a sandbox. Neither guess is right, so the connect path now awaits the 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 that, an absent sandboxUrl means confirmed absent, and the gate is honest in both directions without rebuilding a live client.

3. opened should be baked into the .withProps() constant. This one I could not take, and the reason is in a comment at the call site now: opened is required by ModalRootProps, and .withProps() supplies defaults without relaxing a required prop — baking it in leaves every call site failing TS2741. The .withProps() rule targets static styling/layout/behavior props the type system lets you default; this is not one of them.

npm run ci passes locally on 0f3ee265.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

core/mcp/inspectorClient.ts:3011

• The direct server→client handler still calls enqueuePendingElicitation without the request context's cancellation signal. When the server cancels an in-flight elicitation/create, the SDK aborts ctx.mcpReq.signal, but this controller is not linked to it, so the app iframe/modal can outlive the cancelled request and later answer work the server abandoned. Accept the handler context and pass its signal through this path; add a direct-request cancellation test.

    // Request-scoped abort: forwards the caller's signal (MRTR cancellation)
    // and is aborted by `settleAndDropPendingPeerRequests` on disconnect, so a
    // rendered app cannot outlive the connection that asked for it.
    // Already cancelled before we got here (the caller aborted while an earlier
    // MRTR round was in flight): don't mount an app nobody is waiting on.

[cliffhall]

Review round 10 — addressed (ce263366)

The one suppressed finding was correct: the direct server→client handler ignored its request context, so notifications/cancelled aborted ctx.mcpReq.signal and nothing was listening — the modal and its bridge outlived the cancelled request and could still answer work the server had abandoned.

The handler now threads that signal into enqueuePendingElicitation, which already wires it to both answer surfaces (the native queue entry and the app renderer). The task-augmented branch deliberately does not use it: that request is answered immediately with a CreateTaskResult, so its lifetime is the task's, which carries its own abort (round 8).

Test added as asked: inject notifications/cancelled for the in-flight elicitation, assert the renderer's request-scoped signal aborts and that the request is not reopened natively.

npm run ci passes locally on ce263366.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (1)

clients/web/src/App.tsx:2851

• Awaiting this promise does not refresh the callback's captured setupClientForServer. If a click or deep-link auto-connect starts before /api/config settles, this invocation resumes with the render where sandboxUrl was still undefined, so line 2521 omits appElicitation; the deep-link one-shot guard then prevents a retry and the entire connection silently loses the advertised capability despite having a sandbox. The OAuth callback path also calls setupClientForServer without this wait. Gate those connection effects until initialConfigLoading is false (before setting their one-shot guards), or make client setup read the settled sandbox URL from a current ref.

      // 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;

[cliffhall]

Review round 11 — addressed (356e8eaa)

Correct, and it is a bug in round 9's own fix: setupClientForServer is synchronous and memoized, so awaiting the config resumed with the sandboxUrl captured by the render the connect started in — undefined, on exactly the load the wait was added for. The deep-link one-shot guard then prevented a retry, so that connection silently lost the capability despite having a sandbox. And the OAuth-callback path did not wait at all.

Took the second of the two suggestions, since it covers every entry point rather than each one separately: client construction now reads a ref written on every render, so it sees the current value whoever reached it — connect, deep link, or OAuth callback. The OAuth path also waits now, like the connect path.

Both halves are needed and do different jobs: the wait is what makes an absent value mean confirmed absent rather than not known yet; the ref is what makes the value actually read be the settled one.

npm run ci passes locally on 356e8eaa.

[Copilot]

Pull request overview

Copilot reviewed 38 out of 38 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

clients/web/src/components/elements/AppElicitation/AppElicitationHost.tsx:259

• Only the top modal owns the focus trap, but every lower Modal.Content remains an exposed aria-modal dialog with active controls and an iframe. Screen-reader browse navigation can therefore reach dialogs that are visually covered by the top overlay, while each dialog claims the rest of the page is inert. Hide/inert non-top contents (or use Mantine's modal stack) while keeping their React trees mounted.

      <Modal.Content
        aria-label={`Elicitation ${entry.requestId} — ${entry.params.message} — rendered by ${entry.resourceUri}`}
        data-app-elicitation-status={status}
        data-testid="app-elicitation"
      >

[cliffhall]

Review round 12 — addressed (beaaad49)

Correct: round 7 gave the focus trap to the top modal but left every lower Modal.Content an exposed aria-modal dialog with reachable controls and a live iframe — so browse navigation could land in a dialog that is visually covered and that itself claims the rest of the page is inert.

Non-top contents are now inert: out of the accessibility tree and out of focus order, without unmounting — which is why inert rather than Mantine's modal stack, since each covered app must keep its bridge and its handshake and be live the moment it becomes top. The concurrent-modal test now also asserts the covered dialog carries inert and the top one does not.

npm run ci passes locally on beaaad49.

---

That closes this review pass: twelve rounds, seventeen findings, all seventeen either fixed or answered with a reason (two declined and argued: opened cannot be baked into .withProps() because it is a required prop, and deferring auto-connect wholesale was replaced by the narrower ref + wait). The reviews were consistently worth the round trips — several found real bugs rather than style, notably the receiver-task cancellation overwrite (which predated this PR) and the two ownership-across-connections gaps.