From eea79dead49356bc9465ca9170f099d44ec25b5f Mon Sep 17 00:00:00 2001 From: Hare Date: Tue, 1 Sep 2026 23:44:24 +0900 Subject: [PATCH] fix: bound Web UX review evidence and cleanup --- .gitignore | 1 - tools/web-ux/README.md | 34 ++-- .../browser-tests/capture_smoke_test.ts | 39 +++- tools/web-ux/browser-tests/fixture_server.ts | 5 +- .../scenarios/workspace-control-plane.json | 7 +- tools/web-ux/src/artifacts.ts | 61 ++++++- tools/web-ux/src/capture.ts | 172 ++++++++++++++---- tools/web-ux/src/compare.ts | 4 +- tools/web-ux/src/processes.ts | 103 +++++++++-- tools/web-ux/src/scenario.ts | 15 ++ tools/web-ux/src/types.ts | 47 ++++- tools/web-ux/tests/lifecycle_test.ts | 99 +++++++++- tools/web-ux/tests/scenario_test.ts | 17 +- 13 files changed, 518 insertions(+), 86 deletions(-) diff --git a/.gitignore b/.gitignore index 838d45b8..fe1edfae 100644 --- a/.gitignore +++ b/.gitignore @@ -5,4 +5,3 @@ .worktree *.local* .env -.web-ux/ diff --git a/tools/web-ux/README.md b/tools/web-ux/README.md index 14a5b147..e78bab0c 100644 --- a/tools/web-ux/README.md +++ b/tools/web-ux/README.md @@ -51,15 +51,17 @@ screen-owned selector. ```sh export WEB_UX_BASE_URL='http://127.0.0.1:5173' export WORKSPACE_ID='' +export XDG_STATE_HOME="${XDG_STATE_HOME:-$HOME/.local/state}" ``` ## Authentication fixtures -Authentication state is local sensitive material. `.web-ux/` is gitignored, files are written with -mode `0600`, state contents are never copied into a review bundle, and the CLI never prints cookies -or credentials. Each profile has a sidecar binding it to the exact persona and base URL origin with -a 12-hour default expiry. Capture fails explicitly when metadata is missing, the origin differs, or -the profile has expired; it never silently reuses or refreshes that state. +Authentication state is local sensitive material stored under `$XDG_STATE_HOME/yoi/web-ux/auth/`, +outside the Repository and Workdir. Files are written with mode `0600`, state contents are never +copied into a review bundle, and the CLI never prints cookies or credentials. Each profile has a +sidecar binding it to the exact persona and base URL origin with a 12-hour default expiry. Capture +fails explicitly when metadata is missing, the origin differs, or the profile has expired; it never +silently reuses or refreshes that state. For an interactive Passkey/browser login: @@ -125,11 +127,15 @@ The command exits `2` when it produced evidence but observed UI/tool errors, and capture itself failed. It continues other route/persona captures after a bounded route failure. Inspect: -- `review-context.json` for the exact context, hashes, HTTP status, and failures; -- `contact-sheet.png` with an image-capable reviewer for composition, hierarchy, density, clipping, - empty/error states, and permission-specific affordances; -- each `accessibility.md` for landmark/name/state evidence that a screenshot cannot prove; -- `process-logs/` when the scenario owns a server process. Logs are redacted before writing. +- `review-context.json` for the exact context, hashes, HTTP status, retained/truncated diagnostic + counts, route and capture-point readiness, and the redacted interaction sequence; +- `contact-sheet.png` through its manifest `workdirPath` with an image-capable reviewer for + composition, hierarchy, density, clipping, empty/error states, and permission-specific + affordances; +- each `accessibility.md` through its manifest `workdirPath` for landmark/name/state evidence that a + screenshot cannot prove; +- `process-logs/` when the scenario owns a server process. Each stdout/stderr stream is redacted, + capped at 1 MiB, and paired with truncation metadata. The implementing agent must inspect the actual contact sheet (for example with `ViewImage`), record concrete findings, fix them, recapture under the same persona/route/viewport filters, and inspect @@ -166,10 +172,10 @@ deno task web-ux cleanup --output ../../target/web-ux --keep 5 --older-than-days deno task web-ux cleanup --output ../../target/web-ux --keep 5 --older-than-days 14 ``` -Cleanup recognizes only directories containing `review-context.json`. `.web-ux/` and the repository -`target/` tree are ignored by Git. `capture` defaults to `target/web-ux` when `--output` is omitted. -Keep a bundle outside Git or publish it through the approved immutable artifact channel when durable -review evidence is required. +Cleanup recognizes only directories containing `review-context.json`. The repository `target/` tree +is ignored by Git, while authentication state remains outside the repository. `capture` defaults to +`target/web-ux` when `--output` is omitted. Keep a bundle outside Git or publish it through the +approved immutable artifact channel when durable review evidence is required. ## Adding a scenario diff --git a/tools/web-ux/browser-tests/capture_smoke_test.ts b/tools/web-ux/browser-tests/capture_smoke_test.ts index cce16b14..550f1689 100644 --- a/tools/web-ux/browser-tests/capture_smoke_test.ts +++ b/tools/web-ux/browser-tests/capture_smoke_test.ts @@ -12,6 +12,9 @@ async function freePort(): Promise { Deno.test("browser smoke captures distinct owner and non-owner evidence and cleans its server", async () => { const directory = await Deno.makeTempDir(); + const previousSecret = Deno.env.get("WEB_UX_FIXTURE_SECRET"); + const fixtureSecret = "fixture-canary-secret"; + Deno.env.set("WEB_UX_FIXTURE_SECRET", fixtureSecret); const port = await freePort(); const baseUrl = `http://127.0.0.1:${port}`; try { @@ -45,6 +48,10 @@ Deno.test("browser smoke captures distinct owner and non-owner evidence and clea id: "browser-smoke", title: "Browser smoke", baseUrl, + redact: { + selectors: ["[data-web-ux-redact]"], + text: ["${WEB_UX_FIXTURE_SECRET}"], + }, personas: [ { id: "owner", label: "Owner", auth: { kind: "storage-state", path: "auth/owner.json" } }, { @@ -61,17 +68,26 @@ Deno.test("browser smoke captures distinct owner and non-owner evidence and clea goal: "Verify permission-specific composition", dataState: "Deterministic fixture repository", ready: { kind: "selector", selector: "main" }, - capturePoints: [{ id: "initial", label: "Initial" }], + capturePoints: [{ + id: "initial", + label: "Initial", + interaction: [{ + action: "wait", + ready: { kind: "selector", selector: "h1" }, + }], + }], }], processes: [{ id: "fixture-server", command: Deno.execPath(), args: [ "run", + "--allow-env", "--allow-net", join(Deno.cwd(), "browser-tests/fixture_server.ts"), String(port), ], + env: { WEB_UX_FIXTURE_SECRET: "${WEB_UX_FIXTURE_SECRET}" }, readyUrl: `${baseUrl}/health`, }], }), @@ -81,15 +97,28 @@ Deno.test("browser smoke captures distinct owner and non-owner evidence and clea outputDirectory: join(directory, "artifacts"), runId: "multi-persona", }); - assertEquals(manifest.status, "completed"); + assertEquals(manifest.status, "completed-with-errors"); assertEquals(manifest.captures.map((item) => item.persona.id), ["owner", "non-owner"]); assertEquals(manifest.captures.every((item) => item.screenshots.length === 1), true); - assertEquals(manifest.contactSheet.png, "contact-sheet.png"); + assertEquals(manifest.captures[0].route.ready.kind, "selector"); + assertEquals(manifest.captures[0].interactions[0].action, "wait"); + assertEquals(manifest.captures[0].errorSummary, { + observed: 150, + retained: 100, + truncated: true, + limit: 100, + }); + assertEquals(manifest.contactSheet.png?.bundlePath, "contact-sheet.png"); const runDirectory = join(directory, "artifacts", "multi-persona"); const reviewContext = await Deno.readTextFile(join(runDirectory, "review-context.json")); assertEquals(reviewContext.includes('"cookies"'), false); + assertEquals(reviewContext.includes(fixtureSecret), false); + const processLog = await Deno.readTextFile( + join(runDirectory, "process-logs", "fixture-server.stdout.log"), + ); + assertEquals(processLog.includes(fixtureSecret), false); if (Deno.build.os !== "windows") { - const screenshot = join(runDirectory, manifest.captures[0].screenshots[0].path); + const screenshot = join(runDirectory, manifest.captures[0].screenshots[0].bundlePath); assertEquals((await Deno.stat(screenshot)).mode! & 0o777, 0o600); } await assertRejects( @@ -97,6 +126,8 @@ Deno.test("browser smoke captures distinct owner and non-owner evidence and clea TypeError, ); } finally { + if (previousSecret === undefined) Deno.env.delete("WEB_UX_FIXTURE_SECRET"); + else Deno.env.set("WEB_UX_FIXTURE_SECRET", previousSecret); await Deno.remove(directory, { recursive: true }); } }); diff --git a/tools/web-ux/browser-tests/fixture_server.ts b/tools/web-ux/browser-tests/fixture_server.ts index 7aa607f6..e2de13f5 100644 --- a/tools/web-ux/browser-tests/fixture_server.ts +++ b/tools/web-ux/browser-tests/fixture_server.ts @@ -1,6 +1,9 @@ const port = Number(Deno.args[0]); if (!Number.isInteger(port) || port <= 0) throw new Error("port is required"); +const canary = Deno.env.get("WEB_UX_FIXTURE_SECRET") ?? ""; +console.log(`Authorization: Bearer ${canary}`); + Deno.serve({ hostname: "127.0.0.1", port }, (request) => { const url = new URL(request.url); if (url.pathname === "/health") return new Response("ok"); @@ -11,7 +14,7 @@ Deno.serve({ hostname: "127.0.0.1", port }, (request) => { ? '' : '

Ask a Workspace owner to change repository access.

'; return new Response( - `${title}
Workspace

${title}

main

SSH repository access is configured.

${action}
`, + `${title}
Workspace

${title}

main

SSH repository access is configured.

${action}${canary}
`, { headers: { "content-type": "text/html; charset=utf-8" } }, ); }); diff --git a/tools/web-ux/scenarios/workspace-control-plane.json b/tools/web-ux/scenarios/workspace-control-plane.json index e23962fc..1b548f29 100644 --- a/tools/web-ux/scenarios/workspace-control-plane.json +++ b/tools/web-ux/scenarios/workspace-control-plane.json @@ -15,13 +15,16 @@ { "id": "owner", "label": "Workspace owner", - "auth": { "kind": "storage-state", "path": "../../../.web-ux/auth/owner.json" }, + "auth": { "kind": "storage-state", "path": "${XDG_STATE_HOME}/yoi/web-ux/auth/owner.json" }, "login": { "path": "/", "successUrl": "/w/" } }, { "id": "non-owner", "label": "Authenticated non-owner", - "auth": { "kind": "storage-state", "path": "../../../.web-ux/auth/non-owner.json" }, + "auth": { + "kind": "storage-state", + "path": "${XDG_STATE_HOME}/yoi/web-ux/auth/non-owner.json" + }, "login": { "path": "/", "successUrl": "/w/" } } ], diff --git a/tools/web-ux/src/artifacts.ts b/tools/web-ux/src/artifacts.ts index ebfbc178..64db7585 100644 --- a/tools/web-ux/src/artifacts.ts +++ b/tools/web-ux/src/artifacts.ts @@ -62,13 +62,70 @@ export async function writePrivateJson(path: string, value: unknown): Promise { + const files: string[] = []; + const visit = async (directory: string) => { + for await (const entry of Deno.readDir(directory)) { + const path = resolve(directory, entry.name); + if (entry.isDirectory) await visit(path); + else if (entry.isFile) files.push(path); + } + }; + await visit(root); + return files; +} + +export async function assertReviewBundleIsSecretFree( + root: string, + exactSecrets: string[] = [], +): Promise { + const secrets = exactSecrets.filter(Boolean); + const overlapLength = Math.max(256, ...secrets.map((secret) => secret.length + 1)); + for (const path of await artifactFiles(root)) { + const isText = TEXT_ARTIFACT_EXTENSIONS.some((extension) => path.endsWith(extension)); + const file = await Deno.open(path, { read: true }); + const decoder = new TextDecoder(); + let overlap = ""; + try { + const buffer = new Uint8Array(64 * 1024); + while (true) { + const count = await file.read(buffer); + if (count === null) break; + const content = overlap + decoder.decode(buffer.subarray(0, count), { stream: true }); + for (const secret of secrets) { + if (content.includes(secret)) { + throw new Error( + `review bundle artifact contains configured secret text: ${relative(root, path)}`, + ); + } + } + if (isText) assertBundleIsSecretFree(content, secrets); + overlap = content.slice(-overlapLength); + } + const final = overlap + decoder.decode(); + for (const secret of secrets) { + if (final.includes(secret)) { + throw new Error( + `review bundle artifact contains configured secret text: ${relative(root, path)}`, + ); + } + } + if (isText) assertBundleIsSecretFree(final, secrets); + } finally { + file.close(); + } + } } export async function sha256File(path: string): Promise { diff --git a/tools/web-ux/src/capture.ts b/tools/web-ux/src/capture.ts index 4608723e..05ddc18c 100644 --- a/tools/web-ux/src/capture.ts +++ b/tools/web-ux/src/capture.ts @@ -3,12 +3,14 @@ import { type Browser, chromium, type Page, type Response } from "playwright"; import { validateAuthState } from "./auth_state.ts"; import { assertBundleIsSecretFree, + assertReviewBundleIsSecretFree, bounded, ensurePrivateDirectory, makePrivate, redactText, safeUrl, sha256File, + workdirLogicalPath, } from "./artifacts.ts"; import { type RunningProcess, startOwnedProcesses, stopOwnedProcesses } from "./processes.ts"; import { @@ -21,7 +23,9 @@ import type { CaptureError, CaptureEvidence, CapturePoint, + DiagnosticSummary, Interaction, + InteractionEvidence, Persona, ReadyCondition, ReviewContext, @@ -43,6 +47,32 @@ export type CaptureOptions = { }; type SourceState = { revision: string | null; dirty: boolean | null }; +type ErrorCollector = { errors: CaptureError[]; observed: number; limit: number }; + +const CAPTURE_ERROR_LIMIT = 100; + +function recordError(collector: ErrorCollector, error: CaptureError): void { + collector.observed++; + if (collector.errors.length < collector.limit) collector.errors.push(error); +} + +function errorSummary(collector: ErrorCollector): DiagnosticSummary { + return { + observed: collector.observed, + retained: collector.errors.length, + truncated: collector.observed > collector.errors.length, + limit: collector.limit, + }; +} + +function interactionEvidence(interaction: Interaction): InteractionEvidence { + if (interaction.action === "wait") return { action: "wait", ready: interaction.ready }; + if (interaction.action === "click") return { action: "click", selector: interaction.selector }; + if (interaction.action === "fill") { + return { action: "fill", selector: interaction.selector, value: "[REDACTED]" }; + } + return { action: "press", selector: interaction.selector, key: interaction.key }; +} function slug(value: string): string { return value.replaceAll(/[^a-zA-Z0-9.-]+/g, "-").replaceAll(/^-+|-+$/g, "").toLowerCase(); @@ -73,6 +103,20 @@ async function sourceState(): Promise { } } +async function repositoryRoot(): Promise { + try { + const result = await new Deno.Command("git", { + args: ["rev-parse", "--show-toplevel"], + stdout: "piped", + stderr: "null", + }).output(); + if (result.success) return resolve(new TextDecoder().decode(result.stdout).trim()); + } catch { + // Fall back to the invocation directory outside a Git checkout. + } + return resolve(Deno.cwd()); +} + function selectById( values: T[], requested: string[] | undefined, @@ -162,7 +206,7 @@ export function isVisibleUiErrorText(content: string): boolean { async function collectVisibleUiErrors( page: Page, - errors: CaptureError[], + collector: ErrorCollector, secrets: string[], ): Promise { const alerts = page.locator('[role="alert"], [aria-live="assertive"]'); @@ -172,8 +216,10 @@ async function collectVisibleUiErrors( const content = (await alert.innerText().catch(() => "")).trim(); if (!isVisibleUiErrorText(content)) continue; const message = `visible UI error: ${bounded(redactText(content, secrets), 500)}`; - if (!errors.some((error) => error.kind === "document" && error.message === message)) { - errors.push({ kind: "document", message }); + if ( + !collector.errors.some((error) => error.kind === "document" && error.message === message) + ) { + recordError(collector, { kind: "document", message }); } } } @@ -181,19 +227,24 @@ async function collectVisibleUiErrors( async function capturePoint( page: Page, runDirectory: string, + repositoryRoot: string, persona: Persona, route: RouteScenario, viewport: Viewport, point: CapturePoint, documentResponse: Response | null, - errors: CaptureError[], + collector: ErrorCollector, + executedInteractions: InteractionEvidence[], scenario: Scenario, ): Promise { const startedAt = new Date().toISOString(); - for (const interaction of point.interaction ?? []) await performInteraction(page, interaction); + for (const interaction of point.interaction ?? []) { + await performInteraction(page, interaction); + executedInteractions.push(interactionEvidence(interaction)); + } if (point.ready) await waitReady(page, point.ready); await hideRedactedSelectors(page, scenario.redact?.selectors ?? []); - await collectVisibleUiErrors(page, errors, scenario.redact?.text ?? []); + await collectVisibleUiErrors(page, collector, scenario.redact?.text ?? []); const directory = join( runDirectory, "captures", @@ -208,7 +259,8 @@ async function capturePoint( await makePrivate(viewportScreenshot); const screenshots: ScreenshotEvidence[] = [{ kind: "viewport", - path: relative(runDirectory, viewportScreenshot), + bundlePath: relative(runDirectory, viewportScreenshot), + workdirPath: workdirLogicalPath(repositoryRoot, viewportScreenshot), sha256: await sha256File(viewportScreenshot), }]; if (point.fullPage) { @@ -217,19 +269,23 @@ async function capturePoint( await makePrivate(fullPageScreenshot); screenshots.push({ kind: "full-page", - path: relative(runDirectory, fullPageScreenshot), + bundlePath: relative(runDirectory, fullPageScreenshot), + workdirPath: workdirLogicalPath(repositoryRoot, fullPageScreenshot), sha256: await sha256File(fullPageScreenshot), }); } - let snapshotPath: string | null = null; + let snapshot: { bundlePath: string; workdirPath: string | null } | null = null; try { - const snapshot = await page.locator("body").ariaSnapshot({ timeout: 5_000 }); - const redacted = redactText(snapshot, scenario.redact?.text ?? []); + const accessibility = await page.locator("body").ariaSnapshot({ timeout: 5_000 }); + const redacted = redactText(accessibility, scenario.redact?.text ?? []); const target = join(directory, "accessibility.md"); await Deno.writeTextFile(target, redacted, { mode: 0o600 }); - snapshotPath = relative(runDirectory, target); + snapshot = { + bundlePath: relative(runDirectory, target), + workdirPath: workdirLogicalPath(repositoryRoot, target), + }; } catch (error) { - errors.push({ + recordError(collector, { kind: "tool", message: `accessibility snapshot failed: ${ bounded(error instanceof Error ? error.message : String(error)) @@ -238,14 +294,22 @@ async function capturePoint( } return { persona: { id: persona.id, label: persona.label }, - route: { id: route.id, path: route.path, goal: route.goal, dataState: route.dataState }, + route: { + id: route.id, + path: route.path, + goal: route.goal, + dataState: route.dataState, + ready: route.ready, + }, viewport, theme: scenario.colorScheme ?? "light", - capturePoint: { id: point.id, label: point.label }, + capturePoint: { id: point.id, label: point.label, ready: point.ready ?? null }, + interactions: [...executedInteractions], document: { url: safeUrl(page.url()), status: documentResponse?.status() ?? null }, screenshots, - snapshotPath, - errors: [...errors], + snapshot, + errors: [...collector.errors], + errorSummary: errorSummary(collector), startedAt, finishedAt: new Date().toISOString(), }; @@ -267,14 +331,18 @@ function escapeHtml(value: string): string { async function createContactSheet( browser: Browser, runDirectory: string, + repositoryRoot: string, captures: CaptureEvidence[], -): Promise<{ html: string | null; png: string | null }> { +): Promise<{ + html: { bundlePath: string; workdirPath: string | null } | null; + png: { bundlePath: string; workdirPath: string | null } | null; +}> { const cells: string[] = []; for (const capture of captures) { const screenshot = capture.screenshots.find((item) => item.kind === "viewport") ?? capture.screenshots[0]; if (!screenshot) continue; - const bytes = await Deno.readFile(join(runDirectory, screenshot.path)); + const bytes = await Deno.readFile(join(runDirectory, screenshot.bundlePath)); cells.push( `
${ escapeHtml(capture.persona.label) @@ -305,11 +373,21 @@ async function createContactSheet( } finally { await page.close(); } - return { html: relative(runDirectory, htmlPath), png: relative(runDirectory, pngPath) }; + return { + html: { + bundlePath: relative(runDirectory, htmlPath), + workdirPath: workdirLogicalPath(repositoryRoot, htmlPath), + }, + png: { + bundlePath: relative(runDirectory, pngPath), + workdirPath: workdirLogicalPath(repositoryRoot, pngPath), + }, + }; } export async function capture(options: CaptureOptions): Promise { const scenarioPath = resolve(options.scenarioPath); + const repository = await repositoryRoot(); const scenario = await loadScenario(scenarioPath); const baseUrl = validateBaseUrl( interpolateEnvironment( @@ -332,8 +410,13 @@ export async function capture(options: CaptureOptions): Promise { let browser: Browser | null = null; let processes: RunningProcess[] = []; const captures: CaptureEvidence[] = []; - const diagnostics: CaptureError[] = []; - let contactSheet = { html: null as string | null, png: null as string | null }; + const globalCollector: ErrorCollector = { + errors: [], + observed: 0, + limit: CAPTURE_ERROR_LIMIT, + }; + const diagnostics = globalCollector.errors; + let contactSheet: ReviewContext["contactSheet"] = { html: null, png: null }; let browserVersion = "unknown"; let status: ReviewContext["status"] = "completed"; try { @@ -362,11 +445,17 @@ export async function capture(options: CaptureOptions): Promise { }); try { for (const route of routes) { - const routeErrors: CaptureError[] = []; + const routeCollector: ErrorCollector = { + errors: [], + observed: 0, + limit: CAPTURE_ERROR_LIMIT, + }; + const routeErrors = routeCollector.errors; + const executedInteractions: InteractionEvidence[] = []; const page = await context.newPage(); page.on("console", (message) => { if (message.type() === "error") { - routeErrors.push({ + recordError(routeCollector, { kind: "console", message: bounded(redactText(message.text(), secrets)), }); @@ -375,7 +464,7 @@ export async function capture(options: CaptureOptions): Promise { page.on( "pageerror", (error) => - routeErrors.push({ + recordError(routeCollector, { kind: "page", message: bounded(redactText(error.message, secrets)), }), @@ -383,7 +472,7 @@ export async function capture(options: CaptureOptions): Promise { page.on( "requestfailed", (request) => - routeErrors.push({ + recordError(routeCollector, { kind: "request", message: bounded( redactText(request.failure()?.errorText ?? "request failed", secrets), @@ -393,7 +482,7 @@ export async function capture(options: CaptureOptions): Promise { ); page.on("response", (response) => { if (response.status() >= 400) { - routeErrors.push({ + recordError(routeCollector, { kind: "request", message: `HTTP ${response.status()}`, url: safeUrl(response.url()), @@ -426,7 +515,7 @@ export async function capture(options: CaptureOptions): Promise { } }); if (response && response.status() >= 400) { - routeErrors.push({ + recordError(routeCollector, { kind: "document", message: `document returned HTTP ${response.status()}`, url: safeUrl(response.url()), @@ -438,19 +527,21 @@ export async function capture(options: CaptureOptions): Promise { await capturePoint( page, runDirectory, + repository, persona, route, viewport, point, response, - routeErrors, + routeCollector, + executedInteractions, scenario, ), ); } } catch (error) { status = "completed-with-errors"; - routeErrors.push({ + recordError(routeCollector, { kind: "tool", message: bounded( redactText(error instanceof Error ? error.message : String(error), secrets), @@ -463,14 +554,17 @@ export async function capture(options: CaptureOptions): Promise { path: route.path, goal: route.goal, dataState: route.dataState, + ready: route.ready, }, viewport, theme: scenario.colorScheme ?? "light", - capturePoint: { id: "failed", label: "Capture failed" }, + capturePoint: { id: "failed", label: "Capture failed", ready: null }, + interactions: [...executedInteractions], document: { url: safeUrl(page.url()), status: null }, screenshots: [], - snapshotPath: null, - errors: routeErrors, + snapshot: null, + errors: [...routeErrors], + errorSummary: errorSummary(routeCollector), startedAt: new Date().toISOString(), finishedAt: new Date().toISOString(), }); @@ -483,24 +577,24 @@ export async function capture(options: CaptureOptions): Promise { } } } - contactSheet = await createContactSheet(browser, runDirectory, captures); + contactSheet = await createContactSheet(browser, runDirectory, repository, captures); if (captures.some((capture) => capture.errors.length > 0)) status = "completed-with-errors"; } catch (error) { status = "failed"; - diagnostics.push({ + recordError(globalCollector, { kind: "tool", message: bounded(redactText(error instanceof Error ? error.message : String(error), secrets)), }); } finally { if (browser) { await browser.close().catch((error) => - diagnostics.push({ + recordError(globalCollector, { kind: "tool", message: `browser cleanup failed: ${bounded(String(error))}`, }) ); } - diagnostics.push(...await stopOwnedProcesses(processes)); + for (const error of await stopOwnedProcesses(processes)) recordError(globalCollector, error); } if (diagnostics.length > 0 && status === "completed") status = "completed-with-errors"; const manifest: ReviewContext = { @@ -509,7 +603,7 @@ export async function capture(options: CaptureOptions): Promise { scenario: { id: scenario.id, title: scenario.title, - sourcePath: relative(Deno.cwd(), scenarioPath), + sourcePath: workdirLogicalPath(repository, scenarioPath), }, source: await sourceState(), baseUrl: safeUrl(baseUrl), @@ -524,10 +618,12 @@ export async function capture(options: CaptureOptions): Promise { captures, contactSheet, diagnostics, + diagnosticSummary: errorSummary(globalCollector), }; const serialized = `${JSON.stringify(manifest, null, 2)}\n`; assertBundleIsSecretFree(serialized, secrets); await Deno.writeTextFile(join(runDirectory, "review-context.json"), serialized, { mode: 0o600 }); + await assertReviewBundleIsSecretFree(runDirectory, secrets); if (status === "failed") { throw new Error(`capture failed; inspect ${join(runDirectory, "review-context.json")}`); } diff --git a/tools/web-ux/src/compare.ts b/tools/web-ux/src/compare.ts index 05f8d767..a6b95fca 100644 --- a/tools/web-ux/src/compare.ts +++ b/tools/web-ux/src/compare.ts @@ -39,8 +39,8 @@ async function readManifest(path: string): Promise { } function viewportScreenshot(capture: CaptureEvidence): string | null { - return capture.screenshots.find((item) => item.kind === "viewport")?.path ?? - capture.screenshots[0]?.path ?? null; + return capture.screenshots.find((item) => item.kind === "viewport")?.bundlePath ?? + capture.screenshots[0]?.bundlePath ?? null; } function dataUrl(bytes: Uint8Array): string { diff --git a/tools/web-ux/src/processes.ts b/tools/web-ux/src/processes.ts index 7fca0876..27082f15 100644 --- a/tools/web-ux/src/processes.ts +++ b/tools/web-ux/src/processes.ts @@ -1,11 +1,15 @@ import { dirname, isAbsolute, resolve } from "@std/path"; -import { bounded, redactText } from "./artifacts.ts"; +import { bounded, redactText, writePrivateJson } from "./artifacts.ts"; import type { CaptureError, OwnedProcess } from "./types.ts"; +export const PROCESS_LOG_BYTE_LIMIT = 1024 * 1024; +const PROCESS_STOP_TIMEOUT_MS = 3_000; + export type RunningProcess = { id: string; pid: number; child: Deno.ChildProcess; + status: Promise; output: Promise; }; @@ -20,15 +24,45 @@ async function appendOutput( write: true, mode: 0o600, }); + const encoder = new TextEncoder(); + const overlapCharacters = Math.max(512, ...secrets.map((secret) => secret.length + 128)); + let pending = ""; + let bytesObserved = 0; + let bytesWritten = 0; + let truncated = false; + const writeRedacted = async (value: string) => { + const encoded = encoder.encode(redactText(value, secrets)); + const remaining = Math.max(0, PROCESS_LOG_BYTE_LIMIT - bytesWritten); + if (encoded.length > remaining) truncated = true; + if (remaining > 0) { + const output = encoded.subarray(0, remaining); + await file.write(output); + bytesWritten += output.length; + } + }; try { const reader = stream.pipeThrough(new TextDecoderStream()).getReader(); while (true) { const { value, done } = await reader.read(); if (done) break; - await file.write(new TextEncoder().encode(redactText(value, secrets))); + bytesObserved += encoder.encode(value).length; + pending += value; + if (pending.length > overlapCharacters * 2) { + const splitAt = pending.length - overlapCharacters; + await writeRedacted(pending.slice(0, splitAt)); + pending = pending.slice(splitAt); + } } + await writeRedacted(pending); } finally { file.close(); + await writePrivateJson(`${destination}.meta.json`, { + schemaVersion: 1, + byteLimit: PROCESS_LOG_BYTE_LIMIT, + bytesObserved, + bytesWritten, + truncated, + }); } } @@ -83,17 +117,19 @@ export async function startOwnedProcesses( `${logsDirectory}/${specification.id}.stderr.log`, secrets, ); + const status = child.status; const process = { id: specification.id, pid: child.pid, child, + status, output: Promise.all([stdout, stderr]).then(() => undefined), }; running.push(process); if (specification.readyUrl) { await Promise.race([ waitForReady(specification.readyUrl, specification.readyTimeoutMs ?? 30_000), - child.status.then((status) => { + status.then((status) => { throw new Error( `owned process ${specification.id} exited before readiness: ${status.code}`, ); @@ -145,23 +181,64 @@ function tryKill(pid: number, signal: Deno.Signal): void { } } +async function livePids(pids: number[]): Promise { + if (Deno.build.os === "windows") return []; + if (pids.length === 0) return []; + try { + const result = await new Deno.Command("ps", { + args: ["-o", "pid=", "-p", pids.join(",")], + stdout: "piped", + stderr: "null", + }).output(); + if (!result.success && result.code !== 1) return pids; + const live = new Set( + new TextDecoder().decode(result.stdout).trim().split(/\s+/).map(Number).filter( + Number.isFinite, + ), + ); + return pids.filter((pid) => live.has(pid)); + } catch { + return pids; + } +} + +async function waitForPidsToExit(pids: number[], timeoutMs: number): Promise { + const deadline = Date.now() + timeoutMs; + let live = await livePids(pids); + while (live.length > 0 && Date.now() < deadline) { + await new Promise((resolve) => setTimeout(resolve, 50)); + live = await livePids(live); + } + return live; +} + export async function stopOwnedProcesses(processes: RunningProcess[]): Promise { const diagnostics: CaptureError[] = []; for (const process of [...processes].reverse()) { try { - for (const pid of await descendantPids(process.pid)) tryKill(pid, "SIGTERM"); + const descendants = await descendantPids(process.pid); tryKill(process.pid, "SIGTERM"); + for (const pid of descendants) tryKill(pid, "SIGTERM"); let timer: number | undefined; - const exited = await Promise.race([ - process.child.status.then(() => true), - new Promise((resolve) => { - timer = setTimeout(() => resolve(false), 3_000); - }), - ]).finally(() => clearTimeout(timer)); - if (!exited) { - for (const pid of await descendantPids(process.pid)) tryKill(pid, "SIGKILL"); + const [parentExited, liveDescendants] = await Promise.all([ + Promise.race([ + process.status.then(() => true), + new Promise((resolve) => { + timer = setTimeout(() => resolve(false), PROCESS_STOP_TIMEOUT_MS); + }), + ]).finally(() => clearTimeout(timer)), + waitForPidsToExit(descendants, PROCESS_STOP_TIMEOUT_MS), + ]); + if (!parentExited || liveDescendants.length > 0) { + const lateDescendants = await descendantPids(process.pid); + const forceTargets = [...new Set([...liveDescendants, ...lateDescendants])]; + for (const pid of forceTargets) tryKill(pid, "SIGKILL"); tryKill(process.pid, "SIGKILL"); - await process.child.status; + await process.status; + const survivors = await waitForPidsToExit(forceTargets, 1_000); + if (survivors.length > 0) { + throw new Error(`descendant processes did not exit: ${survivors.join(",")}`); + } } await process.output; } catch (error) { diff --git a/tools/web-ux/src/scenario.ts b/tools/web-ux/src/scenario.ts index b09fbd53..fa0b7796 100644 --- a/tools/web-ux/src/scenario.ts +++ b/tools/web-ux/src/scenario.ts @@ -70,6 +70,9 @@ function parseCapturePoint(value: unknown, at: string): CapturePoint { if (source.ready !== undefined) result.ready = parseReady(source.ready, `${at}.ready`); if (source.interaction !== undefined) { if (!Array.isArray(source.interaction)) throw new Error(`${at}.interaction must be an array`); + if (source.interaction.length > 20) { + throw new Error(`${at}.interaction must not exceed 20 items`); + } result.interaction = source.interaction.map((raw, index) => { const action = record(raw, `${at}.interaction[${index}]`); const name = text(action.action, `${at}.interaction[${index}].action`); @@ -148,6 +151,9 @@ function parseRoute(value: unknown, at: string): RouteScenario { if (!Array.isArray(source.capturePoints) || source.capturePoints.length === 0) { throw new Error(`${at}.capturePoints must have at least one item`); } + if (source.capturePoints.length > 12) { + throw new Error(`${at}.capturePoints must not exceed 12 items`); + } return { id: identifier(source.id, `${at}.id`), label: text(source.label, `${at}.label`), @@ -201,12 +207,15 @@ export async function loadScenario(sourcePath: string): Promise { if (!Array.isArray(source.personas) || source.personas.length === 0) { throw new Error("scenario.personas must have at least one item"); } + if (source.personas.length > 8) throw new Error("scenario.personas must not exceed 8 items"); if (!Array.isArray(source.viewports) || source.viewports.length === 0) { throw new Error("scenario.viewports must have at least one item"); } + if (source.viewports.length > 8) throw new Error("scenario.viewports must not exceed 8 items"); if (!Array.isArray(source.routes) || source.routes.length === 0) { throw new Error("scenario.routes must have at least one item"); } + if (source.routes.length > 40) throw new Error("scenario.routes must not exceed 40 items"); const personas = source.personas.map((value, index) => parsePersona(value, `scenario.personas[${index}]`) ); @@ -239,6 +248,7 @@ export async function loadScenario(sourcePath: string): Promise { }; if (source.processes !== undefined) { if (!Array.isArray(source.processes)) throw new Error("scenario.processes must be an array"); + if (source.processes.length > 8) throw new Error("scenario.processes must not exceed 8 items"); scenario.processes = source.processes.map((value, index) => { const at = `scenario.processes[${index}]`; const process = record(value, at); @@ -266,5 +276,10 @@ export async function loadScenario(sourcePath: string): Promise { uniqueIds(personas, "scenario.personas"); uniqueIds(routes, "scenario.routes"); for (const route of routes) uniqueIds(route.capturePoints, `route ${route.id} capturePoints`); + const captureCount = personas.length * scenario.viewports.length * + routes.reduce((total, route) => total + route.capturePoints.length, 0); + if (captureCount > 200) { + throw new Error(`scenario capture matrix must not exceed 200 items (received ${captureCount})`); + } return scenario; } diff --git a/tools/web-ux/src/types.ts b/tools/web-ux/src/types.ts index 296639a5..bfee1633 100644 --- a/tools/web-ux/src/types.ts +++ b/tools/web-ux/src/types.ts @@ -80,22 +80,51 @@ export type CaptureError = { status?: number; }; -export type ScreenshotEvidence = { +export type ArtifactReference = { + bundlePath: string; + workdirPath: string | null; +}; + +export type ScreenshotEvidence = ArtifactReference & { kind: "viewport" | "full-page"; - path: string; sha256: string; }; +export type InteractionEvidence = + | { action: "click"; selector: string } + | { action: "fill"; selector: string; value: "[REDACTED]" } + | { action: "press"; selector: string; key: string } + | { action: "wait"; ready: ReadyCondition }; + +export type DiagnosticSummary = { + observed: number; + retained: number; + truncated: boolean; + limit: number; +}; + export type CaptureEvidence = { persona: { id: string; label: string }; - route: { id: string; path: string; goal: string; dataState: string }; + route: { + id: string; + path: string; + goal: string; + dataState: string; + ready: ReadyCondition; + }; viewport: Viewport; theme: string; - capturePoint: { id: string; label: string }; + capturePoint: { + id: string; + label: string; + ready: ReadyCondition | null; + }; + interactions: InteractionEvidence[]; document: { url: string; status: number | null }; screenshots: ScreenshotEvidence[]; - snapshotPath: string | null; + snapshot: ArtifactReference | null; errors: CaptureError[]; + errorSummary: DiagnosticSummary; startedAt: string; finishedAt: string; }; @@ -103,7 +132,7 @@ export type CaptureEvidence = { export type ReviewContext = { schemaVersion: 1; runId: string; - scenario: { id: string; title: string; sourcePath: string }; + scenario: { id: string; title: string; sourcePath: string | null }; source: { revision: string | null; dirty: boolean | null }; baseUrl: string; browser: { name: "chromium"; version: string }; @@ -111,6 +140,10 @@ export type ReviewContext = { status: "completed" | "completed-with-errors" | "failed"; filters: { personas: string[]; routes: string[]; viewports: string[] }; captures: CaptureEvidence[]; - contactSheet: { html: string | null; png: string | null }; + contactSheet: { + html: ArtifactReference | null; + png: ArtifactReference | null; + }; diagnostics: CaptureError[]; + diagnosticSummary: DiagnosticSummary; }; diff --git a/tools/web-ux/tests/lifecycle_test.ts b/tools/web-ux/tests/lifecycle_test.ts index 6e5775b3..735a2111 100644 --- a/tools/web-ux/tests/lifecycle_test.ts +++ b/tools/web-ux/tests/lifecycle_test.ts @@ -6,7 +6,11 @@ import { safeUrl, writePrivateJson, } from "../src/artifacts.ts"; -import { startOwnedProcesses, stopOwnedProcesses } from "../src/processes.ts"; +import { + PROCESS_LOG_BYTE_LIMIT, + startOwnedProcesses, + stopOwnedProcesses, +} from "../src/processes.ts"; Deno.test("redaction removes common credentials and query values", () => { const redacted = redactText( @@ -69,6 +73,99 @@ Deno.test("owned process is terminated and its logs are redacted", async () => { const log = await Deno.readTextFile(logPath); assertEquals(log.includes("secret-value"), false); assertStringIncludes(log, "[REDACTED]"); + const metadata = JSON.parse(await Deno.readTextFile(`${logPath}.meta.json`)); + assertEquals(metadata.truncated, false); + } finally { + await Deno.remove(directory, { recursive: true }); + } +}); + +Deno.test("owned process logs stop at the byte limit and record truncation", async () => { + const directory = await Deno.makeTempDir(); + const scenario = join(directory, "scenario.json"); + await Deno.writeTextFile(scenario, "{}"); + try { + const processes = await startOwnedProcesses( + [{ + id: "large-output", + command: Deno.execPath(), + args: [ + "eval", + `console.log("x".repeat(${ + PROCESS_LOG_BYTE_LIMIT + 32_768 + })); setInterval(() => {}, 1000)`, + ], + }], + scenario, + join(directory, "logs"), + [], + ); + const logPath = join(directory, "logs", "large-output.stdout.log"); + for (let attempt = 0; attempt < 100; attempt++) { + try { + if ((await Deno.stat(logPath)).size >= PROCESS_LOG_BYTE_LIMIT) break; + } catch { + // The output pump creates the file asynchronously. + } + await new Promise((resolve) => setTimeout(resolve, 10)); + } + assertEquals(await stopOwnedProcesses(processes), []); + assertEquals((await Deno.stat(logPath)).size, PROCESS_LOG_BYTE_LIMIT); + const metadata = JSON.parse(await Deno.readTextFile(`${logPath}.meta.json`)); + assertEquals(metadata.byteLimit, PROCESS_LOG_BYTE_LIMIT); + assertEquals(metadata.truncated, true); + assertEquals(metadata.bytesWritten, PROCESS_LOG_BYTE_LIMIT); + } finally { + await Deno.remove(directory, { recursive: true }); + } +}); + +Deno.test("forced cleanup terminates a TERM-resistant descendant", async () => { + if (Deno.build.os === "windows") return; + const directory = await Deno.makeTempDir(); + const scenario = join(directory, "scenario.json"); + const childPidPath = join(directory, "child.pid"); + await Deno.writeTextFile(scenario, "{}"); + try { + const childProgram = 'Deno.addSignalListener("SIGTERM", () => {}); setInterval(() => {}, 1000)'; + const parentProgram = ` + const child = new Deno.Command(Deno.execPath(), { + args: ["eval", ${JSON.stringify(childProgram)}], + stdout: "null", + stderr: "null" + }).spawn(); + Deno.writeTextFileSync(Deno.args[0], String(child.pid)); + Deno.addSignalListener("SIGTERM", () => {}); + setInterval(() => {}, 1000); + `; + const processes = await startOwnedProcesses( + [{ + id: "process-tree", + command: Deno.execPath(), + args: ["eval", parentProgram, childPidPath], + }], + scenario, + join(directory, "logs"), + [], + ); + let childPid = 0; + for (let attempt = 0; attempt < 100; attempt++) { + try { + childPid = Number(await Deno.readTextFile(childPidPath)); + if (childPid > 0) break; + } catch { + // The fixture publishes its descendant PID after spawn. + } + await new Promise((resolve) => setTimeout(resolve, 10)); + } + assertEquals(childPid > 0, true); + assertEquals(await stopOwnedProcesses(processes), []); + const status = await new Deno.Command("ps", { + args: ["-p", String(childPid), "-o", "pid="], + stdout: "piped", + stderr: "null", + }).output(); + assertEquals(new TextDecoder().decode(status.stdout).trim(), ""); } finally { await Deno.remove(directory, { recursive: true }); } diff --git a/tools/web-ux/tests/scenario_test.ts b/tools/web-ux/tests/scenario_test.ts index 961464ee..abbb4c1d 100644 --- a/tools/web-ux/tests/scenario_test.ts +++ b/tools/web-ux/tests/scenario_test.ts @@ -1,7 +1,12 @@ import { assertEquals, assertRejects, assertThrows } from "@std/assert"; import { join } from "@std/path"; import { cleanup } from "../src/lifecycle.ts"; -import { interpolateEnvironment, loadScenario, validateBaseUrl } from "../src/scenario.ts"; +import { + interpolateEnvironment, + loadScenario, + resolveScenarioPath, + validateBaseUrl, +} from "../src/scenario.ts"; function minimalScenario(extra = ""): string { return `{ @@ -77,6 +82,16 @@ Deno.test("environment interpolation fails closed", () => { ); }); +Deno.test("committed auth profiles resolve outside the repository", async () => { + const source = "scenarios/workspace-control-plane.json"; + const scenario = await loadScenario(source); + for (const persona of scenario.personas) { + if (persona.auth.kind !== "storage-state") continue; + const statePath = resolveScenarioPath(source, persona.auth.path); + assertEquals(statePath.startsWith(Deno.cwd()), false); + } +}); + Deno.test("cleanup removes only complete review bundles beyond retention", async () => { const directory = await Deno.makeTempDir(); try {