diff --git a/server/src/__tests__/vite-html-renderer.test.ts b/server/src/__tests__/vite-html-renderer.test.ts index add3ba8004..46b065f913 100644 --- a/server/src/__tests__/vite-html-renderer.test.ts +++ b/server/src/__tests__/vite-html-renderer.test.ts @@ -1,7 +1,7 @@ import fs from "node:fs"; import os from "node:os"; import path from "node:path"; -import { afterEach, describe, expect, it } from "vitest"; +import { afterEach, describe, expect, it, vi } from "vitest"; import { createCachedViteHtmlRenderer, type ViteWatcherHost } from "../vite-html-renderer.js"; function createWatcher() { @@ -32,7 +32,7 @@ describe("createCachedViteHtmlRenderer", () => { } }); - it("reuses the injected dev html shell until index.html changes", async () => { + it("caches the branded template until index.html changes while transforming every request", async () => { const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-vite-html-")); tempDirs.push(tempDir); const indexPath = path.join(tempDir, "index.html"); @@ -43,25 +43,36 @@ describe("createCachedViteHtmlRenderer", () => { ); const watcher = createWatcher(); + const transformIndexHtml = vi.fn(async (_url: string, html: string) => + html.replace( + '', + '\n', + ), + ); + const brandHtml = vi.fn((html: string) => html.replace("", '')); const vite: ViteWatcherHost = { watcher, + transformIndexHtml, }; - const renderer = createCachedViteHtmlRenderer({ vite, uiRoot: tempDir }); + const renderer = createCachedViteHtmlRenderer({ vite, uiRoot: tempDir, brandHtml }); await expect(renderer.render("/")).resolves.toContain("/@vite/client"); - await expect(renderer.render("/")).resolves.toContain('"/@react-refresh"'); const first = await renderer.render("/"); const second = await renderer.render("/issues"); expect(first).toBe(second); + expect(first).toContain('data-brand="paperclip"'); expect(first.match(/\/@vite\/client/g)?.length).toBe(1); - expect(first).toContain("window.$RefreshReg$"); + expect(brandHtml).toHaveBeenCalledTimes(1); + expect(transformIndexHtml).toHaveBeenCalledTimes(3); + expect(transformIndexHtml).toHaveBeenLastCalledWith("/issues", expect.stringContaining("v1")); const sourcePath = path.join(tempDir, "src", "main.tsx"); fs.mkdirSync(path.dirname(sourcePath), { recursive: true }); fs.writeFileSync(sourcePath, "export {};\n", "utf8"); watcher.emit("change", sourcePath); expect(await renderer.render("/")).toBe(first); + expect(brandHtml).toHaveBeenCalledTimes(1); fs.writeFileSync( indexPath, @@ -71,27 +82,33 @@ describe("createCachedViteHtmlRenderer", () => { watcher.emit("change", indexPath); await expect(renderer.render("/")).resolves.toContain("v2"); + expect(brandHtml).toHaveBeenCalledTimes(2); renderer.dispose(); }); - it("does not duplicate the vite client tag or react refresh preamble when already present", async () => { + it("runs Vite's HTML transform on every render so HMR entry timestamps stay current", async () => { const tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "paperclip-vite-html-")); tempDirs.push(tempDir); fs.writeFileSync( path.join(tempDir, "index.html"), - '', + '', "utf8", ); + let timestamp = 0; + const transformIndexHtml = vi.fn(async (_url: string, html: string) => + html.replace("/src/main.tsx", `/src/main.tsx?t=${++timestamp}`), + ); const vite: ViteWatcherHost = { watcher: createWatcher(), + transformIndexHtml, }; const renderer = createCachedViteHtmlRenderer({ vite, uiRoot: tempDir }); - const html = await renderer.render("/"); - expect(html.match(/\/@vite\/client/g)?.length).toBe(1); - expect(html.match(/\/@react-refresh/g)?.length).toBe(1); + await expect(renderer.render("/")).resolves.toContain("/src/main.tsx?t=1"); + await expect(renderer.render("/issues/ISS-1")).resolves.toContain("/src/main.tsx?t=2"); + expect(transformIndexHtml).toHaveBeenCalledTimes(2); }); }); diff --git a/server/src/app.ts b/server/src/app.ts index 3c0b284341..669823a648 100644 --- a/server/src/app.ts +++ b/server/src/app.ts @@ -774,9 +774,18 @@ export async function createApp( res.end("Upgrade Required"); }); const { createServer: createViteServer } = await import("vite"); + const configuredViteCacheDir = process.env.PAPERCLIP_VITE_CACHE_DIR?.trim(); const vite = await createViteServer({ root: uiRoot, + ...(configuredViteCacheDir + ? { cacheDir: path.resolve(configuredViteCacheDir) } + : {}), appType: "custom", + // Vite otherwise discovers every HTML entry below the UI root. Generated + // Storybook output can reference dependencies that are intentionally not + // part of the application install, poisoning a clean embedded dev-server + // cache before the browser opens. The embedded UI has one real entry. + optimizeDeps: { entries: [path.resolve(uiRoot, "index.html")] }, server: { // Listener binding and browser HMR hostname are deliberately separate: // exposed branch runtimes stay loopback-only while the browser uses the diff --git a/server/src/vite-html-renderer.ts b/server/src/vite-html-renderer.ts index 983a3ea158..171686aed3 100644 --- a/server/src/vite-html-renderer.ts +++ b/server/src/vite-html-renderer.ts @@ -4,6 +4,7 @@ import path from "node:path"; type ViteWatcherEvent = "add" | "change" | "unlink"; export interface ViteWatcherHost { + transformIndexHtml(url: string, html: string): Promise; watcher?: { on?: (event: ViteWatcherEvent, listener: (file: string) => void) => unknown; off?: (event: ViteWatcherEvent, listener: (file: string) => void) => unknown; @@ -16,28 +17,6 @@ export interface CachedViteHtmlRenderer { } const WATCHER_EVENTS: ViteWatcherEvent[] = ["add", "change", "unlink"]; -const MAIN_ENTRY_TAG = ''; -const VITE_CLIENT_TAG = ''; -const REACT_REFRESH_PREAMBLE = ``; - -function injectViteDevPreamble(html: string): string { - let injectedHtml = html; - if (!injectedHtml.includes('"/@react-refresh"') && !injectedHtml.includes("'/@react-refresh'")) { - injectedHtml = injectedHtml.includes("") - ? injectedHtml.replace("", ` ${REACT_REFRESH_PREAMBLE}\n `) - : `${REACT_REFRESH_PREAMBLE}\n${injectedHtml}`; - } - if (injectedHtml.includes(VITE_CLIENT_TAG)) return injectedHtml; - if (injectedHtml.includes(MAIN_ENTRY_TAG)) { - return injectedHtml.replace(MAIN_ENTRY_TAG, `${VITE_CLIENT_TAG}\n ${MAIN_ENTRY_TAG}`); - } - return injectedHtml.replace("", ` ${VITE_CLIENT_TAG}\n `); -} export function createCachedViteHtmlRenderer(opts: { vite: ViteWatcherHost; @@ -47,18 +26,18 @@ export function createCachedViteHtmlRenderer(opts: { const uiRoot = path.resolve(opts.uiRoot); const templatePath = path.resolve(uiRoot, "index.html"); const brandHtml = opts.brandHtml ?? ((html: string) => html); - let cachedHtml: string | null = null; + let cachedTemplate: string | null = null; - function loadHtml(): string { - if (cachedHtml === null) { + function loadTemplate(): string { + if (cachedTemplate === null) { const rawTemplate = fs.readFileSync(templatePath, "utf-8"); - cachedHtml = injectViteDevPreamble(brandHtml(rawTemplate)); + cachedTemplate = brandHtml(rawTemplate); } - return cachedHtml; + return cachedTemplate; } function invalidate(): void { - cachedHtml = null; + cachedTemplate = null; } function onWatchEvent(filePath: string): void { @@ -73,8 +52,13 @@ export function createCachedViteHtmlRenderer(opts: { } return { - render(): Promise { - return Promise.resolve(loadHtml()); + render(url): Promise { + // Vite's transform does more than inject the dev client and React + // refresh preamble. It also keeps entry-module timestamps aligned with + // the module graph after an HMR invalidation. Serving the raw entry tag + // can otherwise evaluate main.tsx twice (unversioned + timestamped), + // creating two React roots in the same container. + return opts.vite.transformIndexHtml(url, loadTemplate()); }, dispose(): void { diff --git a/tests/runner-e2e/runner.spec.ts b/tests/runner-e2e/runner.spec.ts index 298108d0e0..de8602e001 100644 --- a/tests/runner-e2e/runner.spec.ts +++ b/tests/runner-e2e/runner.spec.ts @@ -1024,14 +1024,25 @@ for (const execution of executions) { requestId: restartRequestId, deadlineAt, }); - await page.goto( - `/${encodeURIComponent(issuePrefix)}/issues/${encodeURIComponent(issue.identifier ?? issue.id)}`, - // A restarted Vite dev server may keep loading its fresh module - // graph after the task UI is already usable. Bind navigation only - // to the committed canonical route, then let the explicit UI and - // API assertions below prove readiness and preserved state. - { waitUntil: "commit" }, + const documentSentinel = `__paperclip_runner_restart_${nonce.replaceAll("-", "_")}`; + await page.evaluate( + (key) => Reflect.set(window, key, true), + documentSentinel, ); + try { + await page.goto( + `/${encodeURIComponent(issuePrefix)}/issues/${encodeURIComponent(issue.identifier ?? issue.id)}`, + // The replacement Vite server can commit and render a fresh + // document while its navigation lifecycle remains unsettled. + // The sentinel and explicit assertions below prove the new + // document and durable state even when Playwright times out. + { waitUntil: "commit" }, + ); + } catch (error) { + if (!(error instanceof Error) || error.name !== "TimeoutError") { + throw error; + } + } await expect( page .getByRole("radio", { @@ -1040,6 +1051,12 @@ for (const execution of executions) { }) .last(), ).toBeVisible({ timeout: 30_000 }); + expect( + await page.evaluate( + (key) => Reflect.get(window, key) === true, + documentSentinel, + ), + ).toBe(false); const reloadedInteractions = await api.get( `/api/issues/${issue.id}/interactions`, ); diff --git a/tests/runner-e2e/select-rerun-artifacts.test.ts b/tests/runner-e2e/select-rerun-artifacts.test.ts index baba8e0d82..cc27acf09a 100644 --- a/tests/runner-e2e/select-rerun-artifacts.test.ts +++ b/tests/runner-e2e/select-rerun-artifacts.test.ts @@ -57,7 +57,9 @@ async function addArtifact(input: { workflowAttempt: number; status: "passed" | "failed"; sourceSha?: string; + resultExecutionId?: string; campaignName?: string; + flattened?: boolean; }) { const artifactName = `runner-e2e-${RUN_ID}-${input.workflowAttempt}-${input.executionId}`; const campaignName = @@ -65,13 +67,16 @@ async function addArtifact(input: { `gha-${RUN_ID}-${input.workflowAttempt}-${input.executionId}`; const directory = path.join( input.root, - artifactName, + ...(input.flattened ? [] : [artifactName]), campaignName, "results", "attempt-1", ); await mkdir(directory, { recursive: true }); - const value = result(input.executionId, input.status); + const value = result( + input.resultExecutionId ?? input.executionId, + input.status, + ); await writeFile( path.join(directory, "result.json"), JSON.stringify({ @@ -142,7 +147,148 @@ function selectionInput(paths: Awaited>) { }; } +function singletonSelectionInput(paths: Awaited>) { + const input = selectionInput(paths); + return { + ...input, + jobs: { + ...input.jobs, + jobs: input.jobs.jobs.filter((job) => job.name === RERUN), + }, + expectedExecutionIds: [RERUN], + }; +} + describe("runner E2E workflow rerun artifact selection", () => { + it("accepts the v8 flattened layout for one expected artifact", async () => { + const paths = await fixture(); + const latest = await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + workflowAttempt: 2, + status: "passed", + flattened: true, + }); + + const selected = await selectRerunArtifacts(singletonSelectionInput(paths)); + + expect(selected).toEqual([ + { + executionId: RERUN, + workflowAttempt: 2, + artifactName: latest.artifactName, + }, + ]); + const selectedResult = JSON.parse( + await readFile( + path.join( + paths.selectedRoot, + latest.artifactName, + latest.campaignName, + "results", + "attempt-1", + "result.json", + ), + "utf8", + ), + ); + expect(selectedResult.status).toBe("passed"); + }); + + it("does not let an older flattened campaign mask a latest missing artifact", async () => { + const paths = await fixture(); + await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + workflowAttempt: 1, + status: "passed", + flattened: true, + }); + + const selected = await selectRerunArtifacts(singletonSelectionInput(paths)); + + expect(selected).toEqual([]); + }); + + it("rejects a flattened campaign when multiple artifacts are expected", async () => { + const paths = await fixture(); + await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + workflowAttempt: 2, + status: "passed", + flattened: true, + }); + + await expect(selectRerunArtifacts(selectionInput(paths))).rejects.toThrow( + /downloaded unexpected runner artifact/u, + ); + }); + + it("rejects a flattened campaign beside another root entry", async () => { + const paths = await fixture(); + await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + workflowAttempt: 2, + status: "passed", + flattened: true, + }); + await writeFile(path.join(paths.artifactRoot, "unexpected.txt"), "no"); + + await expect( + selectRerunArtifacts(singletonSelectionInput(paths)), + ).rejects.toThrow(/downloaded unexpected runner artifact/u); + }); + + it("rejects an unrecognized flattened campaign", async () => { + const paths = await fixture(); + await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + workflowAttempt: 2, + status: "passed", + campaignName: `gha-another-run-2-${RERUN}`, + flattened: true, + }); + + await expect( + selectRerunArtifacts(singletonSelectionInput(paths)), + ).rejects.toThrow(/downloaded unexpected runner artifact/u); + }); + + it("applies source validation to a flattened campaign", async () => { + const paths = await fixture(); + await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + workflowAttempt: 2, + status: "passed", + sourceSha: "ffffffffffffffffffffffffffffffffffffffff", + flattened: true, + }); + + await expect( + selectRerunArtifacts(singletonSelectionInput(paths)), + ).rejects.toThrow("contains result from another source"); + }); + + it("applies execution validation to a flattened campaign", async () => { + const paths = await fixture(); + await addArtifact({ + root: paths.artifactRoot, + executionId: RERUN, + resultExecutionId: RETAINED, + workflowAttempt: 2, + status: "passed", + flattened: true, + }); + + await expect( + selectRerunArtifacts(singletonSelectionInput(paths)), + ).rejects.toThrow(`contains result for ${RETAINED}`); + }); + it("combines retained successes with the latest rerun artifact", async () => { const paths = await fixture(); await addArtifact({ diff --git a/tests/runner-e2e/select-rerun-artifacts.ts b/tests/runner-e2e/select-rerun-artifacts.ts index af5294fd09..358d064dda 100644 --- a/tests/runner-e2e/select-rerun-artifacts.ts +++ b/tests/runner-e2e/select-rerun-artifacts.ts @@ -154,14 +154,27 @@ export async function selectRerunArtifacts(input: SelectRerunArtifactsInput) { string, { executionId: string; workflowAttempt: number } >(); + const recognizedCampaignNames = new Map< + string, + Array<{ + artifactName: string; + executionId: string; + workflowAttempt: number; + }> + >(); for (const executionId of expected) { for (const workflowAttempt of attemptsByExecution .get(executionId)! .keys()) { - recognizedArtifactNames.set( - `runner-e2e-${runId}-${workflowAttempt}-${executionId}`, - { executionId, workflowAttempt }, - ); + const artifactName = `runner-e2e-${runId}-${workflowAttempt}-${executionId}`; + recognizedArtifactNames.set(artifactName, { + executionId, + workflowAttempt, + }); + const campaignName = `gha-${runId}-${workflowAttempt}-${executionId}`; + const identities = recognizedCampaignNames.get(campaignName) ?? []; + identities.push({ artifactName, executionId, workflowAttempt }); + recognizedCampaignNames.set(campaignName, identities); } } @@ -171,16 +184,41 @@ export async function selectRerunArtifacts(input: SelectRerunArtifactsInput) { if (error.code === "ENOENT") return []; throw error; }); - const artifactDirectories = new Map(); - for (const entry of artifactEntries) { - const identity = recognizedArtifactNames.get(entry.name); - if (!identity || !entry.isDirectory()) { - throw new Error(`downloaded unexpected runner artifact ${entry.name}`); + const artifactDirectories = new Map< + string, + | { layout: "wrapped"; directory: string } + | { layout: "flattened"; directory: string; campaignName: string } + >(); + const singletonEntry = artifactEntries[0]; + const singletonCampaignIdentities = singletonEntry + ? recognizedCampaignNames.get(singletonEntry.name) + : undefined; + // download-artifact v8 flattens a single pattern match into the requested + // path. Accept that shape only when the expected set and campaign identity + // make the missing artifact-name wrapper unambiguous. + if ( + expected.length === 1 && + artifactEntries.length === 1 && + singletonEntry?.isDirectory() && + singletonCampaignIdentities?.length === 1 + ) { + const identity = singletonCampaignIdentities[0]!; + artifactDirectories.set(identity.artifactName, { + layout: "flattened", + directory: path.join(input.artifactRoot, singletonEntry.name), + campaignName: singletonEntry.name, + }); + } else { + for (const entry of artifactEntries) { + const identity = recognizedArtifactNames.get(entry.name); + if (!identity || !entry.isDirectory()) { + throw new Error(`downloaded unexpected runner artifact ${entry.name}`); + } + artifactDirectories.set(entry.name, { + layout: "wrapped", + directory: path.join(input.artifactRoot, entry.name), + }); } - artifactDirectories.set( - entry.name, - path.join(input.artifactRoot, entry.name), - ); } const selections: Array<{ @@ -197,19 +235,29 @@ export async function selectRerunArtifacts(input: SelectRerunArtifactsInput) { if (!artifactDirectory) continue; const campaignName = `gha-${runId}-${workflowAttempt}-${executionId}`; - const topLevelEntries = await readdir(artifactDirectory, { - withFileTypes: true, - }); - if ( - topLevelEntries.length !== 1 || - topLevelEntries[0]?.name !== campaignName || - !topLevelEntries[0].isDirectory() - ) { - throw new Error( - `${artifactName} must contain only its exact campaign ${campaignName}`, - ); + let campaignDirectory: string; + if (artifactDirectory.layout === "flattened") { + if (artifactDirectory.campaignName !== campaignName) { + throw new Error( + `${artifactName} must contain only its exact campaign ${campaignName}`, + ); + } + campaignDirectory = artifactDirectory.directory; + } else { + const topLevelEntries = await readdir(artifactDirectory.directory, { + withFileTypes: true, + }); + if ( + topLevelEntries.length !== 1 || + topLevelEntries[0]?.name !== campaignName || + !topLevelEntries[0].isDirectory() + ) { + throw new Error( + `${artifactName} must contain only its exact campaign ${campaignName}`, + ); + } + campaignDirectory = path.join(artifactDirectory.directory, campaignName); } - const campaignDirectory = path.join(artifactDirectory, campaignName); const resultFiles = (await walk(campaignDirectory)).filter( (file) => path.basename(file) === "result.json", );