Return artifact source from show-artifact

.changeset/artifact-external-links.md

@@ -0,0 +1,5 @@
+---
+"executor": patch
+---
+
+**Fix: links in generated artifacts (`<a target="_blank">`) 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.

.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.

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 });

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<string> => {
+  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<string | null>;
+  close: () => void;
+}
+
+/** A small server the guest's browser hits if the link really left the app. */
+const listenForOpen = async (hostAddress: string): Promise<OpenProbe> => {
+  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<void>((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<string | null>((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<number> => {
+  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));
+    }),
+  );
+}

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 <p>Original source marker</p>; }";
+    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,
+        ),
+      ),
+    );
+  }),
+);

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 (
     <div className="flex h-full flex-col gap-4">
       <div data-testid="artifact-header" className="shrink-0">
         <h2>Release Readiness</h2>
         <p data-testid="artifact-marker">${marker}</p>
+        ${
+          linkUrl === undefined
+            ? ""
+            : `<a data-testid="artifact-pr-link" href={${JSON.stringify(linkUrl)}} target="_blank" rel="noreferrer">Open pull request</a>`
+        }
       </div>
       <div data-testid="artifact-scroll" className="min-h-0 flex-1 overflow-auto">
         {Array.from({ length: ${ARTIFACT_ROW_COUNT} }, (_, i) => (
@@ -144,14 +149,14 @@ const recordHandshakeOrdering = async (page: Page): Promise<void> => {
 const readHandshakeOrdering = (page: Page): Promise<ReadonlyArray<string>> =>
   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(() =>