diff --git a/.changeset/artifact-external-links.md b/.changeset/artifact-external-links.md new file mode 100644 index 0000000000..68cf2257a5 --- /dev/null +++ b/.changeset/artifact-external-links.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +**Fix: links in generated artifacts (``) did nothing when clicked.** The sandbox iframe deliberately has no `allow-popups`, so the browser blocked the new browsing context and the click went nowhere. A trusted user click is now relayed across the frame boundary to the host's `openLink` capability — guarded by a per-render nonce so generated code cannot forge or observe it — and the host opens only `http`/`https` URLs. diff --git a/.changeset/artifact-source-text.md b/.changeset/artifact-source-text.md new file mode 100644 index 0000000000..b87f7d8bba --- /dev/null +++ b/.changeset/artifact-source-text.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +**Fix: `show-artifact` now returns the saved component source to MCP clients that cannot render Apps.** Agents can read the current source and make targeted edits instead of receiving only a link to the artifact. diff --git a/apps/cloud/src/mcp/session-build-semaphore.test.ts b/apps/cloud/src/mcp/session-build-semaphore.test.ts index 3d4ad76343..584b65ee0e 100644 --- a/apps/cloud/src/mcp/session-build-semaphore.test.ts +++ b/apps/cloud/src/mcp/session-build-semaphore.test.ts @@ -1,4 +1,4 @@ -import { describe, expect, it, beforeEach } from "@effect/vitest"; +import { describe, expect, it, beforeEach, afterEach, vi } from "@effect/vitest"; import { acquireBuildSlot, @@ -13,6 +13,10 @@ describe("session-build-semaphore", () => { resetBuildSlotsForTest(); }); + afterEach(() => { + vi.useRealTimers(); + }); + it("grants up to the cap immediately, with no wait", async () => { const results = await Promise.all([ acquireBuildSlot().promise, @@ -214,6 +218,7 @@ describe("session-build-semaphore", () => { }); it("proceeds without a slot when the queue wait exceeds the timeout, and does not count it as active", async () => { + vi.useFakeTimers(); await Promise.all([ acquireBuildSlot().promise, acquireBuildSlot().promise, @@ -223,6 +228,10 @@ describe("session-build-semaphore", () => { expect(currentActiveBuildsForTest()).toBe(4); const timedOutHandle = acquireBuildSlot(10); + await vi.advanceTimersByTimeAsync(9); + expect(currentQueueLengthForTest()).toBe(1); + expect(currentActiveBuildsForTest()).toBe(4); + await vi.advanceTimersByTimeAsync(1); const result = await timedOutHandle.promise; expect(result).toEqual({ acquired: false, waitMs: expect.any(Number), timedOut: true }); diff --git a/e2e/desktop-vm/artifact-external-link.test.ts b/e2e/desktop-vm/artifact-external-link.test.ts new file mode 100644 index 0000000000..99b91a5671 --- /dev/null +++ b/e2e/desktop-vm/artifact-external-link.test.ts @@ -0,0 +1,170 @@ +// The packaged desktop app, running in a GUI guest, checking that a link inside +// a generated artifact opens in the user's real browser. +// +// On desktop the console runs artifact UI in a sandbox that can't open popups, +// so a clicked link is handed to the app, which calls window.open. Electron +// then sends plain link clicks out to the system browser instead of opening a +// window inside the app. The cloud e2e covers the click getting that far; this +// covers what happens next, which only a real Electron build can show. +// +// To check it without guessing whether "a browser opened", we point the link at +// a small HTTP server on the host and see who asks for it. If the system +// browser fetches it (its user-agent has no "Electron" in it) and the app +// didn't gain a new window, the link left the app like it should. An in-app +// window would fail on both counts. +import { writeFileSync } from "node:fs"; +import http from "node:http"; +import type { AddressInfo } from "node:net"; +import { join } from "node:path"; + +import { expect, it } from "@effect/vitest"; +import { Effect } from "effect"; + +import { scenario } from "../src/scenario"; +import { RunDir } from "../src/services"; +import { CdpPage, guestSsh, pageWsUrl, recordGuestScreen, sleep } from "../src/vm/desktop"; + +const NAME = "Desktop (packaged, in a VM) · an artifact link opens in the system browser"; +const cdpPort = process.env.E2E_DESKTOP_CDP_PORT; +const guestIp = process.env.E2E_DESKTOP_VM_IP; +const recSeconds = Number(process.env.E2E_DESKTOP_REC_SECONDS ?? "12"); +const os: "macos" | "linux" | "windows" = + process.env.E2E_TARGET === "desktop-windows" + ? "windows" + : process.env.E2E_TARGET === "desktop-linux" + ? "linux" + : "macos"; + +/** The host's address as seen from the guest. On both tart bridges the guest's + * default gateway is the host, so a link pointed here comes back to us when + * the guest's browser follows it. */ +const hostAddressFromGuest = async (ip: string): Promise => { + const command = + os === "linux" + ? "ip route show default 2>/dev/null | awk '{print $3; exit}'" + : "route -n get default 2>/dev/null | awk '/gateway/{print $2; exit}'"; + const { stdout } = await guestSsh(ip, command); + return stdout.trim(); +}; + +interface OpenProbe { + readonly url: string; + /** The user-agent of whoever fetched the link, or null if nobody did in time. */ + waitForOpen: (timeoutMs: number) => Promise; + close: () => void; +} + +/** A small server the guest's browser hits if the link really left the app. */ +const listenForOpen = async (hostAddress: string): Promise => { + const path = "/opened-from-desktop"; + let seenUserAgent: string | null = null; + let notify: ((ua: string) => void) | null = null; + + const server = http.createServer((req, res) => { + if ((req.url ?? "").startsWith(path)) { + seenUserAgent = String(req.headers["user-agent"] ?? ""); + notify?.(seenUserAgent); + notify = null; + } + res.end("ok"); + }); + await new Promise((resolve) => server.listen(0, "0.0.0.0", () => resolve())); + const { port } = server.address() as AddressInfo; + + return { + url: `http://${hostAddress}:${port}${path}`, + waitForOpen: (timeoutMs: number) => + new Promise((resolve) => { + if (seenUserAgent !== null) return resolve(seenUserAgent); + notify = resolve; + setTimeout(() => { + notify = null; + resolve(seenUserAgent); + }, timeoutMs); + }), + close: () => server.close(), + }; +}; + +/** How many pages the app has open right now — a new in-app window bumps this. */ +const pageTargetCount = async (): Promise => { + const targets = (await fetch(`http://127.0.0.1:${cdpPort}/json/list`) + .then((r) => (r.ok ? r.json() : [])) + .catch(() => [])) as ReadonlyArray<{ type: string }>; + return targets.filter((t) => t.type === "page").length; +}; + +const run = async (runDir: string) => { + const cdp = await CdpPage.connect(await pageWsUrl(Number(cdpPort))); + try { + await cdp.command("Runtime.enable"); + await cdp.command("Page.enable"); + + // Film the guest while we drive it, so the recording shows the browser + // coming to the front when the link opens. + const recording = recordGuestScreen( + guestIp as string, + recSeconds, + join(runDir, "session.mp4"), + os, + ); + + // Wait for the console to load before we do anything with it. + await cdp.waitForText("Integrations", 60_000).catch(() => cdp.waitForText("Settings", 60_000)); + + const hostAddress = await hostAddressFromGuest(guestIp as string); + expect(hostAddress, "the guest reported the host address it routes through").toMatch( + /^\d+\.\d+\.\d+\.\d+$/, + ); + + const probe = await listenForOpen(hostAddress); + try { + const pagesBefore = await pageTargetCount(); + + // The same call the console makes when a `target="_blank"` link is + // clicked (see packages/react/src/api/shell-host.ts). We run it directly + // here — the click-to-open path is already covered by the cloud e2e, and + // what we care about on desktop is what Electron does with this call. + await cdp.command("Runtime.evaluate", { + expression: `window.open(${JSON.stringify(probe.url)}, "_blank", "noopener,noreferrer")`, + }); + + const fetcherUserAgent = await probe.waitForOpen(15_000); + await sleep(1500); + const pagesAfter = await pageTargetCount(); + + writeFileSync(join(runDir, "01-link-opened-externally.png"), await cdp.screenshot()); + + expect(fetcherUserAgent, "the desktop handed the link to the system browser").not.toBeNull(); + expect( + fetcherUserAgent ?? "", + "the OS browser fetched the link, not an in-app Electron window", + ).not.toContain("Electron"); + expect(pagesAfter, "the app opened no in-app browser window for the link").toBe(pagesBefore); + } finally { + probe.close(); + } + + await recording; + } finally { + cdp.close(); + } +}; + +if (!cdpPort || !guestIp || os === "windows") { + const why = + os === "windows" + ? "the host-listener probe needs the tart bridge; the Windows guest attaches over an SSH jump" + : "needs a desktop guest — set E2E_DESKTOP_VM_IP or run the desktop-macos/desktop-linux project"; + it.skip(`${NAME} (${why})`, () => {}); +} else { + // Literal name (not NAME) so the run's test.ts review artifact captures it. + scenario( + "Desktop (packaged, in a VM) · an artifact link opens in the system browser", + { timeout: 180_000 }, + Effect.gen(function* () { + const runDir = yield* RunDir; + yield* Effect.promise(() => run(runDir)); + }), + ); +} diff --git a/e2e/scenarios/artifact-source-roundtrip.test.ts b/e2e/scenarios/artifact-source-roundtrip.test.ts new file mode 100644 index 0000000000..e8a056a7a7 --- /dev/null +++ b/e2e/scenarios/artifact-source-roundtrip.test.ts @@ -0,0 +1,77 @@ +import { expect } from "@effect/vitest"; +import { Effect, Schema } from "effect"; +import { composePluginApi } from "@executor-js/api/server"; +import { ArtifactId } from "@executor-js/sdk/shared"; + +import { scenario } from "../src/scenario"; +import { Api, Browser, Mcp, Target } from "../src/services"; +import { visit } from "../src/surfaces/browser"; + +const api = composePluginApi([] as const); +const savedArtifact = Schema.Struct({ artifactId: ArtifactId, url: Schema.String }); +const sourceResult = Schema.Struct({ code: Schema.String }); +const structured = Schema.Struct({ structuredContent: Schema.Unknown }); + +scenario( + "Artifacts · text-only clients read current source and edit the same artifact", + { timeout: 120_000 }, + Effect.gen(function* () { + const target = yield* Target; + const mcp = yield* Mcp; + const browser = yield* Browser; + const { client: makeClient } = yield* Api; + const identity = yield* target.newIdentity(); + const client = yield* makeClient(api, identity); + const session = mcp.session(identity); + const source = "function App() { return

Original source marker

; }"; + const created = yield* session.call("create-artifact", { + title: "Source round trip", + code: source, + }); + expect(created.ok).toBe(true); + const envelope = yield* Schema.decodeUnknownEffect(structured)(created.raw); + const saved = yield* Schema.decodeUnknownEffect(savedArtifact)(envelope.structuredContent); + yield* Effect.gen(function* () { + const shown = yield* session.call("show-artifact", { id: saved.artifactId }); + expect(shown.ok).toBe(true); + const shownEnvelope = yield* Schema.decodeUnknownEffect(structured)(shown.raw); + const current = yield* Schema.decodeUnknownEffect(sourceResult)( + shownEnvelope.structuredContent, + ); + expect(current.code).toBe(source); + expect(shown.text).toContain(current.code); + const updated = current.code.replace("Original source marker", "Updated source marker"); + const edited = yield* session.call("edit-artifact", { + artifactId: saved.artifactId, + edits: [{ oldText: current.code, newText: updated }], + }); + expect(edited.ok, edited.text).toBe(true); + const afterEdit = yield* session.call("show-artifact", { id: saved.artifactId }); + expect(afterEdit.ok).toBe(true); + const afterEnvelope = yield* Schema.decodeUnknownEffect(structured)(afterEdit.raw); + const afterSource = yield* Schema.decodeUnknownEffect(sourceResult)( + afterEnvelope.structuredContent, + ); + expect(afterSource.code).toBe(updated); + expect(afterEdit.text).toContain(updated); + expect(afterEdit.text).not.toContain("Original source marker"); + yield* browser.session(identity, async ({ page, step }) => { + await step("Open the artifact edited from its returned source", async () => { + await visit(page, saved.url); + await page + .frameLocator('[data-testid="artifact-shell-frame"]') + .frameLocator("iframe") + .getByText("Updated source marker", { exact: true }) + .waitFor({ timeout: 30_000 }); + }); + }); + }).pipe( + Effect.ensuring( + client.artifacts.remove({ params: { artifactId: saved.artifactId } }).pipe( + // oxlint-disable-next-line executor/no-effect-escape-hatch -- boundary: cleanup must fail the scenario if the API cannot remove the fixture + Effect.orDie, + ), + ), + ); + }), +); diff --git a/e2e/scenarios/artifacts.test.ts b/e2e/scenarios/artifacts.test.ts index fa9a82d03b..89328c0770 100644 --- a/e2e/scenarios/artifacts.test.ts +++ b/e2e/scenarios/artifacts.test.ts @@ -51,13 +51,18 @@ const api = composePluginApi([] as const); */ const ARTIFACT_ROW_COUNT = 40; -const artifactSource = (marker: string) => ` +const artifactSource = (marker: string, linkUrl?: string) => ` function App() { return (

Release Readiness

${marker}

+ ${ + linkUrl === undefined + ? "" + : `Open pull request` + }
{Array.from({ length: ${ARTIFACT_ROW_COUNT} }, (_, i) => ( @@ -144,14 +149,14 @@ const recordHandshakeOrdering = async (page: Page): Promise => { const readHandshakeOrdering = (page: Page): Promise> => page.evaluate(() => globalThis.__handshakeOrder ?? []); -const readConsoleStyle = (page: Page): Promise<{ primary: string; buttonBg: string }> => - page.evaluate(() => { - const button = document.querySelector("button"); - return { - primary: getComputedStyle(document.documentElement).getPropertyValue("--primary").trim(), - buttonBg: button ? getComputedStyle(button).backgroundColor : "", - }; - }); +const readConsoleStyle = async (page: Page): Promise<{ primary: string; buttonBg: string }> => { + const button = page.getByRole("button", { name: "Rename", exact: true }); + await button.waitFor(); + return button.evaluate((element) => ({ + primary: getComputedStyle(document.documentElement).getPropertyValue("--primary").trim(), + buttonBg: getComputedStyle(element).backgroundColor, + })); +}; // The shell's compiled stylesheet declares `--mcp-apps-shell-stylesheet: 1` // on `:root` as a provenance marker (see the shell's globals.css): the shell's @@ -187,6 +192,8 @@ scenario( const suffix = uniqueSuffix(); const title = `Release Readiness ${suffix}`; const marker = `artifact-ok-${suffix}`; + const pullRequestUrl = new URL("/policies?from=artifact-link", target.baseUrl).toString(); + const source = artifactSource(marker, pullRequestUrl).trim(); // Tracked so cleanup runs even when an assertion below fails. let artifactId: ArtifactId | undefined; @@ -215,7 +222,7 @@ scenario( ); const rendered = yield* session.call("create-artifact", { - code: artifactSource(marker), + code: source, title, description: "Whether the current release is ready to ship", }); @@ -308,6 +315,19 @@ scenario( .not.toContain("Connecting"); }); + await step("A pull request link opens with a normal left-click", async () => { + const openedPage = page.context().waitForEvent("page"); + await artifactContent(page).getByTestId("artifact-pr-link").click(); + const popup = await openedPage; + await popup.waitForURL(pullRequestUrl, { timeout: 20_000 }); + + expect( + popup.url(), + "the sandbox handed the link to the host, which opened a new tab", + ).toBe(pullRequestUrl); + await popup.close(); + }); + await step("The host was listening before the shell could speak", async () => { // The regression guard for the handshake race. Asserting only that the // artifact rendered is not enough: the previous implementation @@ -552,6 +572,10 @@ scenario( String(structuredOf(shown).url ?? shown.text), "show-artifact delivers the same deep link for a non-Apps client", ).toContain(String(artifactId)); + expect( + shown.text, + "show-artifact includes the current source in its text result for a non-Apps client", + ).toContain(`Source:\n\`\`\`tsx\n${source}\n\`\`\``); }).pipe( Effect.ensuring( Effect.suspend(() => diff --git a/packages/hosts/mcp-apps-shell/src/shell/inner-renderer.tsx b/packages/hosts/mcp-apps-shell/src/shell/inner-renderer.tsx index a8d16a5ba2..cc1af323f9 100644 --- a/packages/hosts/mcp-apps-shell/src/shell/inner-renderer.tsx +++ b/packages/hosts/mcp-apps-shell/src/shell/inner-renderer.tsx @@ -94,10 +94,32 @@ Object.assign(globalThis, { SharedWorker: blockedNetwork("SharedWorker"), }); +// Grab these before any model-written code runs. The saved postMessage and the +// private nonce mean generated code can't fake or snoop on our open-link +// messages, even if it later overwrites `parent.postMessage`. +const postParentMessage = window.parent.postMessage.bind(window.parent); +const openLinkNonce = Array.from(crypto.getRandomValues(new Uint32Array(4))).join("-"); + const sendParent = (message: Record) => { - window.parent.postMessage({ ...message, token }, "*"); + postParentMessage({ ...message, token }, "*"); }; +// The sandbox can't open popups on its own (no `allow-popups`, on purpose), so +// a plain `` click does nothing. We hand those clicks to the +// host, which opens the link for us. Normal same-page links are left alone. +document.addEventListener("click", (event) => { + // Only real user clicks count. Generated code can call `element.click()`, but + // it can't forge a trusted event, so those are ignored here. + if (!event.isTrusted || event.defaultPrevented || event.button !== 0) return; + const target = event.target; + if (!(target instanceof Element)) return; + const anchor = target.closest("a[href]"); + if (!anchor || anchor.target.toLowerCase() !== "_blank" || anchor.href === "") return; + + event.preventDefault(); + sendParent({ type: "executor.openLink", url: anchor.href, openLinkNonce }); +}); + const requestParent = (message: ParentRequestPayload): Promise => { const requestId = ++nextRequestId; return new Promise((resolve, reject) => { @@ -429,4 +451,4 @@ const resizeObserver = new ResizeObserver(([entry]) => { }); resizeObserver.observe(document.body); -sendParent({ type: "executor.renderer.ready" }); +sendParent({ type: "executor.renderer.ready", openLinkNonce }); diff --git a/packages/hosts/mcp-apps-shell/src/shell/mcp-app.browser.test.ts b/packages/hosts/mcp-apps-shell/src/shell/mcp-app.browser.test.ts index acf79110e7..a05f0f9262 100644 --- a/packages/hosts/mcp-apps-shell/src/shell/mcp-app.browser.test.ts +++ b/packages/hosts/mcp-apps-shell/src/shell/mcp-app.browser.test.ts @@ -69,6 +69,7 @@ type HostState = { readonly initialized: boolean; readonly toolCalls: HostToolCall[]; readonly resumeCalls: HostToolCall[]; + readonly openLinks: string[]; }; type BrowserHostWindow = Window & { @@ -394,6 +395,23 @@ function App() { } `; +const generatedExternalLinkCode = ` +function App() { + return ( + + + + Open pull request + + + Unsafe link + + + + ); +} +`; + /** A different artifact, for asserting what a SECOND delivery does and does not * do — its own marker so the test waits on the new render, not the old one. */ const generatedSecondRenderCode = ` @@ -764,6 +782,7 @@ const createHostHtml = (shellUrl: string) => ` initialized: false, toolCalls: [], resumeCalls: [], + openLinks: [], }; window.__mcpHostState = state; @@ -827,6 +846,12 @@ const createHostHtml = (shellUrl: string) => ` return; } + if (message.method === "ui/open-link" && message.id !== undefined) { + state.openLinks.push(message.params?.url); + respond(event.source, message.id, {}); + return; + } + if (message.method === "tools/call" && message.id !== undefined) { const params = message.params ?? {}; state.toolCalls.push(params); @@ -1582,6 +1607,70 @@ describe("MCP app generated UI browser isolation", () => { } }, 30_000); + it("routes pull request links through the host without granting popup access", async () => { + if (!browser || !hostServer) throw new Error("Browser harness did not start."); + const { page, shellFrame } = await openHarness(browser, hostServer.url); + + try { + const innerFrame = await renderGeneratedUi(page, shellFrame, generatedExternalLinkCode); + await innerFrame.locator("#pr-link").waitFor({ timeout: 10_000 }); + + // Generated code knows the public renderer token, but must not be able + // to replace the private click authorization by repeating the handshake. + await innerFrame.evaluate(() => { + const token = document + .querySelector('meta[name="executor-render-token"]') + ?.getAttribute("content"); + if (!token) throw new Error("Renderer token missing"); + window.parent.postMessage( + { type: "executor.renderer.ready", token, openLinkNonce: "forged" }, + "*", + ); + window.parent.postMessage( + { + type: "executor.openLink", + token, + openLinkNonce: "forged", + url: "https://example.com/forged", + }, + "*", + ); + document.querySelector("#pr-link")?.click(); + }); + await page.waitForTimeout(100); + expect((await getHostState(page)).openLinks).toEqual([]); + + // Click the nested label, not the anchor itself: real links commonly wrap + // text and icons, and the renderer must recover the owning anchor. + await innerFrame.locator("#pr-link span").click(); + await page.waitForFunction( + () => (window as unknown as BrowserHostWindow).__mcpHostState.openLinks.length === 1, + ); + + const hostState = await getHostState(page); + expect(hostState.openLinks).toEqual(["https://example.com/pulls/123"]); + + // The frame can ask only for web URLs. Carrying `javascript:` into the + // host would move model-written code from the opaque sandbox origin into + // the console's origin, so it is neither opened nor executed. + await innerFrame.locator("#unsafe-link").click(); + await page.waitForTimeout(100); + expect((await getHostState(page)).openLinks).toEqual(["https://example.com/pulls/123"]); + expect(await innerFrame.locator("body").getAttribute("data-unsafe")).toBeNull(); + + expect( + await innerFrame.locator("#pr-link").count(), + "the generated frame was not navigated away", + ).toBe(1); + expect( + await shellFrame.locator('iframe[title="Generated UI"]').getAttribute("sandbox"), + "model-written code still has no direct popup permission", + ).toBe("allow-scripts"); + } finally { + await page.close(); + } + }, 30_000); + // An artifact that uses two accounts of one integration tags each call site // with a role. The role has to survive four hops — the inner proxy's `apply` // trap, the TanStack cache key, the postMessage bridge, and the diff --git a/packages/hosts/mcp-apps-shell/src/shell/shell-app.tsx b/packages/hosts/mcp-apps-shell/src/shell/shell-app.tsx index c9cfbe52bc..363b6972ab 100644 --- a/packages/hosts/mcp-apps-shell/src/shell/shell-app.tsx +++ b/packages/hosts/mcp-apps-shell/src/shell/shell-app.tsx @@ -69,7 +69,8 @@ type RendererRequest = args: unknown; role?: unknown; } - | { type: "executor.renderer.ready"; token: string } + | { type: "executor.renderer.ready"; token: string; openLinkNonce: unknown } + | { type: "executor.openLink"; token: string; url: unknown; openLinkNonce: unknown } | { type: "executor.renderer.config"; token: string; config: unknown } | { type: "executor.renderer.size"; token: string; height: unknown } | { type: "executor.renderer.error"; token: string; message: unknown } @@ -254,6 +255,14 @@ const buildRendererSrcDoc = (token: string): string => { const isRecord = (value: unknown): value is Record => typeof value === "object" && value !== null && !Array.isArray(value); +/** The frame is untrusted, so only allow plain web links through. A + * `javascript:` URL would run as the host page, not inside the sandbox. */ +const isExternalWebUrl = (value: unknown): value is string => { + if (typeof value !== "string" || !URL.canParse(value)) return false; + const protocol = new URL(value).protocol; + return protocol === "http:" || protocol === "https:"; +}; + // --------------------------------------------------------------------------- // Remembered approvals ("Approve and don't ask again") // --------------------------------------------------------------------------- @@ -378,6 +387,10 @@ export function McpAppsShell({ const pendingInteractionRef = useRef(null); const rendererFrameRef = useRef(null); const rendererRef = useRef(null); + // The nonce the renderer made before running any generated code. We never + // send it back into the frame, so a link request has to prove it knows this + // nonce, not just the public renderer token. + const openLinkAuthorizationRef = useRef<{ token: string; nonce: string } | null>(null); // Whether the embedding host can store a preview snapshot. Held in a ref // because the renderer message handler is built once and must not be rebuilt // when the context changes. @@ -533,6 +546,14 @@ export function McpAppsShell({ }; if (data.type === "executor.renderer.ready") { + // The bootstrap runs before generated code. A later ready message is + // untrusted and must not replace its private click authorization. + if (openLinkAuthorizationRef.current?.token === current.token) return; + if (typeof data.openLinkNonce !== "string" || data.openLinkNonce === "") return; + openLinkAuthorizationRef.current = { + token: current.token, + nonce: data.openLinkNonce, + }; postToRenderer({ type: "executor.render", code: current.code, @@ -550,6 +571,23 @@ export function McpAppsShell({ return; } + // The frame can't open links itself, so it relays the click to us and we + // ask the host to open it. We check the nonce and the URL first. + if (data.type === "executor.openLink") { + const authorization = openLinkAuthorizationRef.current; + if ( + authorization?.token !== current.token || + data.openLinkNonce !== authorization.nonce || + !isExternalWebUrl(data.url) + ) { + return; + } + app.openLink({ url: data.url }).catch((error: unknown) => { + console.error("[executor-shell] Failed to open generated link:", error); + }); + return; + } + if (data.type === "executor.renderer.config") { setRenderer((prev) => prev && prev.token === current.token @@ -633,7 +671,7 @@ export function McpAppsShell({ window.addEventListener("message", handleRendererMessage); return () => window.removeEventListener("message", handleRendererMessage); - }, [hostContext?.theme, postToRenderer]); + }, [app, hostContext?.theme, postToRenderer]); useEffect(() => { if (renderer) { @@ -674,6 +712,7 @@ export function McpAppsShell({ height: 240, }; rendererRef.current = nextRenderer; + openLinkAuthorizationRef.current = null; setRenderer(nextRenderer); setComponent(null); setError(null); @@ -682,6 +721,7 @@ export function McpAppsShell({ setError(`Compilation error: ${msg}`); setComponent(null); rendererRef.current = null; + openLinkAuthorizationRef.current = null; setRenderer(null); } }, []); @@ -745,6 +785,7 @@ export function McpAppsShell({ }; setComponent(() => DataView); rendererRef.current = null; + openLinkAuthorizationRef.current = null; setRenderer(null); setError(null); }; diff --git a/packages/hosts/mcp/src/artifacts-tools.test.ts b/packages/hosts/mcp/src/artifacts-tools.test.ts index bc0a958b20..a761f12bb8 100644 --- a/packages/hosts/mcp/src/artifacts-tools.test.ts +++ b/packages/hosts/mcp/src/artifacts-tools.test.ts @@ -182,6 +182,13 @@ const structuredOf = (result: Awaited>): Record>): string => (result.content as Array<{ type: string; text: string }>)[0].text; +/** Assert source is available through both MCP result channels. */ +const expectArtifactSource = (result: Awaited>, code: string) => { + expect(structuredOf(result).code).toBe(code); + expect(textOf(result)).toContain("Source:"); + expect(textOf(result)).toContain(code); +}; + const toolNames = async (client: Client): Promise => (await client.listTools()).tools.map((tool) => tool.name); @@ -688,8 +695,11 @@ describe("MCP host — create-artifact", () => { url: "https://executor.test/artifacts/art_1", artifactId: "art_1", }); - // The model needs to be told to hand the URL over. + // The model needs to be told to hand the URL over. Source is a + // show-artifact read, not part of the create confirmation. expect(textOf(result)).toContain("https://executor.test/artifacts/art_1"); + expect(textOf(result)).not.toContain("Source:"); + expect(structuredOf(result)).not.toHaveProperty("code"); // Persistence is what makes the fallback possible at all. expect(store.calls).toHaveLength(1); expect(store.rows.get("art_1")?.code).toBe(COUNTER_CODE); @@ -1285,6 +1295,10 @@ describe("MCP host — artifact retrieval", () => { code: COUNTER_CODE, artifactId: "art_1", }); + // Apps-capable hosts still need the source on the text channel: a + // later restore or a client that starts advertising apps must not + // make `show-artifact` unusable for `edit-artifact`. + expectArtifactSource(shown, COUNTER_CODE); }, { artifacts: store.port }, ); @@ -1368,7 +1382,12 @@ describe("MCP host — artifact retrieval", () => { status: "fallback_url", url: "https://executor.test/artifacts/art_1", artifactId: "art_1", + code: COUNTER_CODE, }); + // The URL instruction stays; the source rides after it so a text-only + // host can copy `oldText` for `edit-artifact` from this result. + expect(textOf(shown)).toContain("https://executor.test/artifacts/art_1"); + expectArtifactSource(shown, COUNTER_CODE); }, { artifacts: store.port, @@ -1377,6 +1396,35 @@ describe("MCP host — artifact retrieval", () => { ); }); + it("returns show-artifact source when the client has no apps support and no web UI", async () => { + const store = makeArtifactStore(); + await Effect.runPromise( + store.port.save({ + title: "Saved earlier", + description: null, + code: COUNTER_CODE, + }), + ); + await withClient( + makeStubEngine({}), + NO_APPS_CAPS, + async (client) => { + const shown = await client.callTool({ + name: "show-artifact", + arguments: { id: "art_1" }, + }); + expect(structuredOf(shown)).toEqual({ + status: "fallback_unavailable", + reason: "mcp_apps_unsupported", + artifactId: "art_1", + code: COUNTER_CODE, + }); + expectArtifactSource(shown, COUNTER_CODE); + }, + { artifacts: store.port }, + ); + }); + it("reports a miss as an error result rather than failing the tool call", async () => { const store = makeArtifactStore(); await withClient( diff --git a/packages/hosts/mcp/src/tool-server.ts b/packages/hosts/mcp/src/tool-server.ts index 730c5bfd89..c78c2e8adb 100644 --- a/packages/hosts/mcp/src/tool-server.ts +++ b/packages/hosts/mcp/src/tool-server.ts @@ -900,6 +900,8 @@ const startMarker = (name: string, attributes: Record): Effect. // user as an inline widget when the client renders MCP Apps, and as a link into // the web app when it doesn't. Both carry `artifactId`, because either way the // artifact was saved and can be reopened later. +// `show-artifact` returns source on both channels; create/edit only confirm +// saves. const renderRejectedResult = (reason: string): McpToolResult => ({ content: [{ type: "text", text: `create-artifact rejected: ${reason}` }], @@ -983,6 +985,24 @@ const bindingUnresolvedResult = (input: { isError: true, }); +/** Format the stored source for the text result channel. */ +const artifactSourceText = (code: string): string => `Source:\n\`\`\`tsx\n${code}\n\`\`\``; + +/** Add source to both MCP result channels. */ +const withArtifactSource = (result: McpToolResult, code: string): McpToolResult => { + const source = artifactSourceText(code); + const content = result.content.map((block, index) => + index === 0 && block.type === "text" + ? { type: "text" as const, text: `${block.text}\n\n${source}` } + : block, + ); + return { + ...result, + content, + structuredContent: { ...result.structuredContent, code }, + }; +}; + const renderedInAppResult = (input: { readonly code: string; readonly artifactId: string; @@ -2042,11 +2062,14 @@ export const createExecutorMcpServer = ( .pipe(Effect.catchCause(() => Effect.succeed(null))); if (!artifact) return artifactNotFoundResult(id); yield* notifyArtifactUsage("viewed"); - return deliverArtifact({ - code: artifact.code, - artifactId: artifact.id, - title: artifact.title, - }); + return withArtifactSource( + deliverArtifact({ + code: artifact.code, + artifactId: artifact.id, + title: artifact.title, + }), + artifact.code, + ); }).pipe( Effect.withSpan("mcp.host.tool.show_artifact", { attributes: { "mcp.tool.name": "show-artifact", "mcp.artifact.id": id }, @@ -2245,7 +2268,7 @@ export const createExecutorMcpServer = ( description: [ "Re-render a saved UI artifact by id.", "Use `list-artifacts` first to find the id whose title or description matches what the user asked for.", - "Clients that cannot display MCP apps receive a link to the artifact instead.", + "Returns the artifact's current source. Clients that cannot display MCP apps also receive a link to the artifact; pass it to the user.", ].join("\n"), inputSchema: { id: z.string().trim().min(1).describe("The artifact id from `list-artifacts`."),