History

Resolve MCP-Apps support at render time, not from the initialize hook

The persisted-capability seed alone did not fix the production symptom. On cold restore the agents runtime replays the persisted initialize REQUEST, which does re-establish the server's client capabilities — but it never replays the notifications/initialized NOTIFICATION, and oninitialized (the only hook that re-runs syncToolAvailability) fires solely on that notification. A restored session therefore held full apps capabilities while the cached flag still read its seeded value, and the replay is dispatched un-awaited, so a tool call can also land ahead of it. deliverArtifact now reads the live server's capabilities per render and reconciles the app-only tools when they disagree, so both orderings answer identically. The seeded value remains the fallback for the window before any capabilities exist. Also guard the persist path: a reading taken with no capabilities present must never be written back, or one such replay would make the deep-link downgrade durable for every future restore of the session.

Rhys Sullivan committed 3 months ago

Commit 08cc11b1 parent ed7bf082 files · +148 −18Browse files

Unified · Split

2 files
  1. packages/hosts/mcp/src/artifacts-tools.test.ts +75 −0
  2. packages/hosts/mcp/src/tool-server.ts +73 −18
packages/hosts/mcp/src/artifacts-tools.test.ts+75−0
@@ -292,6 +292,81 @@ describe("MCP host — artifact tool visibility", () => { · 291 unmodified lines
292292 await serverTransport.close();
293293 });
294294
295+ // The restore path the `agents` runtime actually takes, and the one that
296+ // reached production. On cold restore it replays the PERSISTED `initialize`
297+ // REQUEST (`reinitializeServer`), which does re-establish the server's client
298+ // capabilities — but it never replays the `notifications/initialized`
299+ // NOTIFICATION, and `oninitialized` (the only hook that re-runs
300+ // `syncToolAvailability`) fires solely on that notification. So the rebuilt
301+ // server holds full apps capabilities while nothing re-reads them, and the
302+ // replay is dispatched un-awaited, so a tool call can also land ahead of it.
303+ //
304+ // Both orderings are covered here: capabilities present but no hook, and a
305+ // tool call dispatched before the replay arrives at all.
306+ it("renders inline on a restore that replays initialize without the initialized notification", async () => {
307+ const store = makeArtifactStore();
308+ const mcpServer = await Effect.runPromise(
309+ createExecutorMcpServer({
310+ engine: makeStubEngine({}),
311+ artifacts: store.port,
312+ artifactsEnabled: true,
313+ loadAppShellHtml: () => Promise.resolve(SHELL_HTML),
314+ // Nothing persisted: the capability must come from the replay alone, so
315+ // a passing assertion cannot be coming from the seed.
316+ restoredAppsEnabled: false,
317+ artifactUrl: artifactUrlFor("https://executor.test"),
318+ }),
319+ );
320+ const [clientTransport, serverTransport] = InMemoryTransport.createLinkedPair();
321+ await mcpServer.connect(serverTransport);
322+
323+ const responses = new Map<string | number, Record<string, unknown>>();
324+ clientTransport.onmessage = (message) => {
325+ const envelope = message as { id?: string | number; result?: Record<string, unknown> };
326+ if (envelope.id !== undefined && envelope.result) responses.set(envelope.id, envelope.result);
327+ };
328+ await clientTransport.start();
329+
330+ // Exactly what `agents/mcp` replays: the initialize request under its
331+ // restore sentinel id, and no `notifications/initialized` after it.
332+ await clientTransport.send({
333+ jsonrpc: "2.0",
334+ id: "__worker_transport_restore__",
335+ method: "initialize",
336+ params: {
337+ protocolVersion: "2025-06-18",
338+ capabilities: APPS_CAPS,
339+ clientInfo: { name: "ClaudeAI", version: "1.0.0" },
340+ },
341+ } as never);
342+ await vi.waitFor(() =>
343+ expect(mcpServer.server.getClientCapabilities()).toMatchObject(APPS_CAPS),
344+ );
345+
346+ const call = async (id: number, method: string, params: Record<string, unknown>) => {
347+ await clientTransport.send({ jsonrpc: "2.0", id, method, params });
348+ await vi.waitFor(() => expect(responses.has(id)).toBe(true));
349+ return responses.get(id) ?? {};
350+ };
351+
352+ const created = (await call(1, "tools/call", {
353+ name: "create-artifact",
354+ arguments: { code: COUNTER_CODE, title: "Active users dashboard" },
355+ })) as { structuredContent?: Record<string, unknown> };
356+ expect(created.structuredContent).toEqual({ code: COUNTER_CODE, artifactId: "art_1" });
357+ expect(created.structuredContent).not.toHaveProperty("status", "fallback_url");
358+
359+ // The widget is useless if it cannot call back in, so visibility has to
360+ // reconcile on the same restore.
361+ const listed = (await call(2, "tools/list", {})) as { tools: Array<{ name: string }> };
362+ const names = listed.tools.map((tool) => tool.name);
363+ expect(names).toContain("execute-action");
364+ expect(names).toContain("execute-action-resume");
365+
366+ await clientTransport.close();
367+ await serverTransport.close();
368+ });
369+
295370 it("still falls back to a deep link when the restored session had no apps support", async () => {
296371 const store = makeArtifactStore();
297372 await withClient(
packages/hosts/mcp/src/tool-server.ts+73−18
@@ -1525,13 +1525,35 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 1524 unmodified lines
15251525
15261526 // Set from the client's advertised capabilities at `initialize`. Read by
15271527 // the render handlers to choose inline widget vs. deep link. Seeded from
1528− // the host's persisted value so a cold-restored session — rebuilt with no
1529− // `initialize` to replay — keeps rendering inline for a client that had
1530− // already negotiated apps support.
1528+ // the host's persisted value so a cold-restored session keeps rendering
1529+ // inline for a client that had already negotiated apps support.
1530+ //
1531+ // This is a cache, not the source of truth: `appsSupported()` below reads
1532+ // the live server on every render, because a cold restore re-establishes
1533+ // capabilities without ever running the hook that maintains this variable.
15311534 let appsEnabled = config.restoredAppsEnabled ?? false;
15321535 let executeActionTool: { enable: () => void; disable: () => void } | undefined;
15331536 let executeActionResumeTool: { enable: () => void; disable: () => void } | undefined;
15341537
1538+ /**
1539+ * Move the cached flag and the app-only tools together.
1540+ *
1541+ * `execute-action` is only callable from inside a rendered app, so a client
1542+ * that can't render one should never see it. `create-artifact`,
1543+ * `list-artifacts` and `show-artifact` stay visible regardless: they still
1544+ * persist, and still return something useful (a deep link).
1545+ */
1546+ const applyAppsEnabled = (next: boolean): void => {
1547+ appsEnabled = next;
1548+ if (next) {
1549+ executeActionTool?.enable();
1550+ executeActionResumeTool?.enable();
1551+ } else {
1552+ executeActionTool?.disable();
1553+ executeActionResumeTool?.disable();
1554+ }
1555+ };
1556+
15351557 // Best-effort usage observation; a failing observer never affects the tool.
15361558 const notifyArtifactUsage = (action: "created" | "viewed" | "updated"): Effect.Effect<void> =>
15371559 config.onArtifactUsage
@@ -1558,19 +1580,56 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 20 unmodified lines
15581580 preview: input.preview ?? null,
15591581 });
15601582 yield* notifyArtifactUsage(input.existingId === undefined ? "created" : "updated");
1583+ // Resolve once and report the value actually used, so the span can
1584+ // never disagree with what the client received.
1585+ const delivered = deliverArtifact({
1586+ code: saved.code,
1587+ artifactId: saved.id,
1588+ title: saved.title,
1589+ });
15611590 yield* Effect.annotateCurrentSpan({
15621591 "mcp.artifact.id": saved.id,
15631592 "mcp.artifact.apps_enabled": appsEnabled,
15641593 });
1565− return deliverArtifact({ code: saved.code, artifactId: saved.id, title: saved.title });
1594+ return delivered;
15661595 });
15671596
1597+ /**
1598+ * Whether the client can render an app, resolved at render time.
1599+ *
1600+ * `appsEnabled` alone is not enough. On a cold restore the host replays the
1601+ * persisted `initialize` *request* — which does set the server's client
1602+ * capabilities — but never the `notifications/initialized` notification,
1603+ * and `oninitialized` (the only hook that re-runs `syncToolAvailability`)
1604+ * fires solely on that notification. So a restored session can hold full
1605+ * apps capabilities while `appsEnabled` still reads its seeded value, and
1606+ * the replay is dispatched un-awaited, so a tool call can land before it.
1607+ *
1608+ * Reading the live server here makes both orderings produce the same
1609+ * answer, and keeps the seeded value as the fallback for the window before
1610+ * any capabilities exist.
1611+ */
1612+ const appsSupported = (): boolean => {
1613+ const live = server.server.getClientCapabilities();
1614+ if (!live) return appsEnabled;
1615+ const uiCapability = getUiCapability(
1616+ live as ClientCapabilities & { extensions?: Record<string, unknown> },
1617+ );
1618+ const supported = Boolean(uiCapability?.mimeTypes?.includes(RESOURCE_MIME_TYPE));
1619+ // Reconcile the tools too: a restore that re-established capabilities
1620+ // without firing `oninitialized` would otherwise render inline while
1621+ // `execute-action` — the tool that rendered app calls back into — stayed
1622+ // hidden, leaving the widget unable to do anything.
1623+ if (supported !== appsEnabled) applyAppsEnabled(supported);
1624+ return supported;
1625+ };
1626+
15681627 const deliverArtifact = (input: {
15691628 readonly code: string;
15701629 readonly artifactId: string;
15711630 readonly title: string;
15721631 }): McpToolResult => {
1573− if (appsEnabled) return renderedInAppResult(input);
1632+ if (appsSupported()) return renderedInAppResult(input);
15741633 const url = config.artifactUrl?.(input.artifactId);
15751634 return url
15761635 ? renderedAsLinkResult({ url, artifactId: input.artifactId, title: input.title })
@@ -1936,6 +1995,11 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 359 unmodified lines
19361995 // answered from whatever is registered at that moment — so app-only tool
19371996 // visibility has to be re-synced from the `oninitialized` hook rather than
19381997 // decided at construction.
1998+ //
1999+ // This hook covers live clients only. It does NOT run on a cold restore:
2000+ // the host replays the persisted `initialize` request, but `oninitialized`
2001+ // fires on the `notifications/initialized` notification, which is never
2002+ // persisted. `appsSupported()` is what makes the restored case correct.
19392003 const syncToolAvailability = () => {
19402004 const clientCapabilities = server.server.getClientCapabilities();
19412005 const uiCapability = getUiCapability(
@@ -1954,10 +2018,13 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 12 unmodified lines
19542018 ? Boolean(uiCapability?.mimeTypes?.includes(RESOURCE_MIME_TYPE))
19552019 : appsEnabled;
19562020 const changed = negotiated !== appsEnabled;
1957− appsEnabled = negotiated;
2021+ applyAppsEnabled(negotiated);
19582022
19592023 // Persist only a real negotiation that moved the value, so the next cold
19602024 // restore seeds itself. Best-effort: the session must not fail on it.
2025+ // The `clientCapabilities` guard matters beyond skipping a no-op write:
2026+ // persisting an absent-capability reading would make a downgrade durable
2027+ // for every future restore of the session.
19612028 const onAppsEnabledChange = config.onAppsEnabledChange;
19622029 if (clientCapabilities && changed && onAppsEnabledChange) {
19632030 // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: `oninitialized` is a sync SDK hook; persistence is fire-and-forget and its failure must not fail the session
@@ -1966,18 +2033,6 @@ export const createExecutorMcpServer = <E extends Cause.YieldableError>( · 2 unmodified lines
19662033 );
19672034 }
19682035
1969− // `execute-action` is only callable from inside a rendered app, so a
1970− // client that can't render one should never see it. `create-artifact`,
1971− // `list-artifacts` and `show-artifact` stay visible regardless: they
1972− // still persist, and still return something useful (a deep link).
1973− if (appsEnabled) {
1974− executeActionTool?.enable();
1975− executeActionResumeTool?.enable();
1976− } else {
1977− executeActionTool?.disable();
1978− executeActionResumeTool?.disable();
1979− }
1980−
19812036 console.error(
19822037 "[executor] MCP session mode",
19832038 JSON.stringify({