From e35e4c8f391e17776f2ea9d1f73c841cb6019808 Mon Sep 17 00:00:00 2001 From: Claude Code Date: Sun, 26 Jul 2026 12:54:26 -0700 Subject: [PATCH 1/4] Add a pre-flight selector gate, fail-closed overlay, and wire clipsDir Closes #13 and #14. A selector mistake used to surface after every narration had been paid for, as a raw Playwright error naming neither the shot nor the action, having stalled a full 30s locator timeout. The pre-flight gate resolves every declared selector against the page its shot opens, before any TTS spend. The gate resolves with the same engine capture uses, against a page carrying the same injected overlay, in a context as fresh as capture's, and waits for a late-hydrating element. Where its evidence is genuinely weaker than the render's it reports INFO instead of blocking: a selector behind an earlier click or type, an ambiguous hover, an auth-walled live shot, an unsettled page. Reviewers caught an earlier version that would have refused this repo's own demos/proctor script; both classes now carry regression tests. window.__demoHighlight no longer hides its box and returns on a miss, and capture now hands the overlay a rectangle it already resolved with Playwright, so the page never resolves the selector a second time with a different engine. clipsDir is read: a bare filename resolves inside it, a path carrying a directory resolves against the config file's directory, and prebaked runs no longer depend on the working directory. pnpm test 34 files / 201 tests exit 0; typecheck and build exit 0. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 56 +++++ README.md | 66 +++++- specs/preflight-selector-gate-spec.md | 157 ++++++++++++ src/capture.ts | 64 +++-- src/cli.test.ts | 6 + src/cli.ts | 26 +- src/clips.test.ts | 39 +++ src/clips.ts | 33 +++ src/config.test.ts | 22 ++ src/config.ts | 5 + src/overlay.ts | 48 +++- src/pipeline.ts | 40 +++- src/preflight.test.ts | 52 ++++ src/preflight.ts | 329 ++++++++++++++++++++++++++ src/provenance.test.ts | 1 + src/provenance.ts | 24 +- src/sanitize.ts | 43 ++++ src/types.ts | 61 +++++ src/urls.ts | 13 + tests/capture.smoke.test.ts | 82 ++++++- tests/fixtures/late-element.html | 16 ++ tests/fixtures/reveal.html | 18 ++ tests/overlay.smoke.test.ts | 42 ++++ tests/preflight.smoke.test.ts | 223 +++++++++++++++++ 24 files changed, 1437 insertions(+), 29 deletions(-) create mode 100644 specs/preflight-selector-gate-spec.md create mode 100644 src/clips.test.ts create mode 100644 src/clips.ts create mode 100644 src/preflight.test.ts create mode 100644 src/preflight.ts create mode 100644 src/sanitize.ts create mode 100644 src/urls.ts create mode 100644 tests/fixtures/late-element.html create mode 100644 tests/fixtures/reveal.html create mode 100644 tests/overlay.smoke.test.ts create mode 100644 tests/preflight.smoke.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 789abda..bac6cf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,61 @@ # Changelog +## Unreleased + +### Breaking + +- A prebaked `clip:` that does not exist at its resolved path now fails immediately, + naming the path that was tried. Previously the declared path was returned verbatim and + the run failed later, inside ffmpeg, against a path nobody had resolved. (#14) +- `clip:` paths no longer resolve against the process working directory. A bare filename + resolves inside `clipsDir`; a path carrying its own directory resolves against the + config file's directory; an absolute path is used as given. Configs that referenced + clips relative to their config file, or by bare filename as the README described, keep + working. A config that relied on being invoked from a particular directory does not. + +### Added + +- Pre-flight selector gate. Every selector a script declares is resolved against the page + its own shot opens at that point, before any narration is synthesized, and the run + refuses to start when a selector provably will not do what the script says. It also + reports a shot that references a selector but never navigates (capture builds a fresh + context per shot, so that shot runs against `about:blank`), a `prebaked` shot declaring + selector actions capture never runs, a selector-requiring action declared without a + selector, and a prebaked clip missing at its resolved path. Fail-closed; `preflight: + false` or `--no-preflight` declines it, and a declined run says so. (#13) + + The gate blocks only on claims it can actually make. It resolves with the same selector + engine capture uses (so Playwright syntax such as `>> nth=` and shadow-DOM piercing + behave identically), against a page carrying the same injected overlay, and waits for an + element that hydrates in after load. Where its evidence is genuinely weaker than the + render's it reports at `INFO` instead: a selector behind an earlier `click`/`type` in the + same shot, an ambiguous `hover` (which capture resolves non-strictly), an auth-walled + `live` shot, and a page that did not settle before counting. +- `out/render-report.json` records whether the gate ran, whether it was declined, and which + shots it could not adjudicate, so a finished video states how it was checked. +- `preflightWaitMs` (default 3000) budgets how long the gate waits for a selector absent at + first count, keeping it no stricter than the render's auto-waiting locator. +- `clipsDir` is now read. It was declared in the schema, defaulted, and documented in two + places while no code path consulted it. (#14) + +### Fixed + +- `window.__demoHighlight` no longer fails open. A selector matching nothing hid the + highlight box and returned, so the shot rendered, the run exited 0, and the highlight + never happened. An ambiguous selector is a failure too, because `document.querySelector` + silently takes the first match, which is how a bare `p` or `code` selector looks correct + while pointing at the wrong element. (#13) +- A failing `highlight` action now names the shot, the selector, and the real match count. + It previously surfaced as a raw Playwright strict-mode violation, or as a bare + `Timeout 30000ms exceeded` on a zero match, identifying neither the shot nor the action. + The locator still does the waiting, so an element that appears after load still works, + and a failure that is NOT a count problem (a hidden but unique element, a closed page) + now rethrows the original error rather than being mislabelled as one. +- The highlight overlay is driven by a rectangle capture already resolved with Playwright, + instead of re-resolving the selector in the page with `document.querySelectorAll`. The + two engines disagree on shadow DOM and on Playwright-only selector syntax, so an element + inside an open shadow root previously drew no highlight at all. + ## 0.3.0 ### Breaking diff --git a/README.md b/README.md index 78bc20d..0945704 100644 --- a/README.md +++ b/README.md @@ -135,12 +135,60 @@ The script is a Markdown file. Each shot is a `### SHOT ` heading followed b | `click` | `selector` | Moves fake cursor then clicks | | `type` | `selector`, `text` | Types character-by-character (60 ms delay) | | `hover` | `selector` | Hovers (no fake cursor move) | -| `highlight` | `selector` | Injects a highlight overlay | +| `highlight` | `selector` | Injects a highlight overlay. The selector must match **exactly one** element | | `chapter` | `label` or `text` | Shows a chapter card overlay | | `wait` | `ms` | Pauses for N milliseconds | For `target: prebaked`, set `clip` to the path of an existing video file; no browser is launched for that shot. +## Pre-flight selector gate + +Before any narration is synthesized, the pipeline resolves every selector your script +declares against the page that shot opens at that point, and refuses to start when one of +them provably will not do what the script says. + +``` +$ demo-video demo.config.json + BLOCKING shot "12-execution-boundary": selector "p" is ambiguous, 43 matches on http://localhost:3000/guide (the strict locator needs exactly one) + BLOCKING shot "05-plate": selector "[data-line='82']" matches nothing on http://localhost:3000/guide + BLOCKING shot "08-recap" uses 2 selector(s) but declares no goto action; capture builds a fresh context per shot, so this shot runs against about:blank and every locator waits out its full timeout + INFO shot "04-approve": selector ".verdict-box" matches nothing on http://localhost:3000/ (an earlier click or type in this shot can change the DOM, and the gate runs no actions, so this could not be verified) +✗ [agent-demo-video] preflight gate failed: 3 finding(s); no narration was synthesized. +``` + +It reports four things a script cannot tell you on its own: + +- a selector that matches **nothing**, which the highlight overlay used to turn into a + silent no-op; +- a selector that matches **more than one** element, because `document.querySelector` + takes the first match, so a bare `p` or `code` selector looks correct while pointing + somewhere else entirely; +- a shot that references a selector but **never navigates**. Capture builds a fresh + browser context per shot, so such a shot runs against `about:blank` and every locator + waits out its full timeout; +- a `prebaked` shot that declares selector actions, which capture never runs. + +The gate resolves selectors with the same engine capture uses, against a page carrying +the same injected overlay, and waits for an element that hydrates in after load. Where it +resolves differently from the render it would be reporting on its own limitations, not on +your script. + +It therefore only BLOCKS on a claim it can actually make. These are reported at `INFO` and +never fail a run: + +- a selector that a preceding `click` or `type` in the same shot would have revealed (the + gate runs no actions, so it cannot see that DOM); +- an ambiguous `hover`, because `page.hover` resolves non-strictly and renders fine; +- an auth-walled `live` shot, because the gate runs unauthenticated and would see the + login wall; +- a page that did not settle before its selectors were counted. + +`out/render-report.json` records whether the gate ran, whether it was declined, and which +shots it could not adjudicate, so a finished video says for itself how it was checked. + +The gate is fail-closed. Set `preflight: false` in the config, or pass `--no-preflight` +for a single run, to decline it; a declined run says so in its output. + ## Authenticated SaaS capture (`target: live`) `target: live` drives an authenticated SaaS app (Slack, Notion, Linear, Stripe, any @@ -225,7 +273,8 @@ Key fields in `demo.config.json` (full schema in `src/types.ts`): | `audio.soundDesign` | `true` | Synthesized ambient bed ducked under narration, click ticks, segment sweeps | | `motion.livingCamera` | `true` | Continuous camera path with drift; `motion.zoomOnAction: false` disables all camera motion | | `brand` | (off) | `{ title, subtitle, url, accent, cards }` adds branded title and end cards | -| `clipsDir` | `"clips/prebaked"` | Directory scanned for prebaked clips | +| `clipsDir` | `"clips/prebaked"` | Where a **bare** prebaked clip filename resolves. Resolved against the config file's directory unless absolute | +| `preflight` | `true` | Fail-closed pre-flight selector gate; see [Pre-flight selector gate](#pre-flight-selector-gate). `false` (or `--no-preflight`) declines it | | `maxDurationSec` | `300` | Hard ceiling for the finished video. The render fails if the result exceeds it. Set it to the length limit you are shipping against. | | `capture.settleMs` | `500` | Budget for the post-navigation readiness wait (fonts ready, visible images decoded). `0` disables the probe. Exceeding the budget warns and records anyway. Under the default `screencast` engine the wait happens BEFORE recording starts, so unsettled frames are excluded; the legacy `recordvideo` engine binds capture at context creation, so there the wait shifts those frames later rather than excluding them. | @@ -242,7 +291,18 @@ For surfaces you cannot or should not drive live (SaaS login walls, desktop apps - narration: UiPath Studio opens the workflow we exported earlier. ``` -Place the clip in `clipsDir`. The pipeline passes it through normalize/mux/caption without launching a browser. +Clip paths resolve independently of the working directory you run from: + +| `clip:` value | Resolves to | +|---|---| +| `uipath-studio.mp4` (bare filename) | `//uipath-studio.mp4` | +| `clips/prebaked/uipath-studio.mp4` (carries a directory) | `/clips/prebaked/uipath-studio.mp4` | +| `/srv/clips/uipath-studio.mp4` (absolute) | used exactly as given | + +Place the clip in `clipsDir` and reference it by bare filename, or give a path relative to +your config file. Either way the pipeline passes it through normalize/mux/caption without +launching a browser. A clip that is not present at the resolved path fails immediately, +naming the path that was tried. ## Replicability diff --git a/specs/preflight-selector-gate-spec.md b/specs/preflight-selector-gate-spec.md new file mode 100644 index 0000000..7726383 --- /dev/null +++ b/specs/preflight-selector-gate-spec.md @@ -0,0 +1,157 @@ +# Pre-flight Selector Gate - Spec + +Status: active (closes issues #13 and #14) + +## Problem + +A DEMO_SCRIPT can declare selectors that do not identify what the author meant, and +nothing tells the author until narration has already been paid for. + +Three distinct defects, each verified against `0d940d5`: + +1. **The highlight overlay is fail-open as a function.** `window.__demoHighlight` + hides its box and returns when `document.querySelector` misses, and + `querySelector` silently takes the FIRST match when a selector is ambiguous. + The capture path currently happens to guard this with a strict Playwright + locator, so the overlay's own fail-open behaviour is masked rather than absent: + any other caller of the overlay API gets a silent no-op, and the two resolutions + (locator, then in-page `querySelector`) are independent, so they can disagree. + +2. **A selector mistake is reported late, expensively, and unreadably.** A + zero-match highlight selector stalls the full 30s locator timeout mid-render and + then reports `locator.scrollIntoViewIfNeeded: Timeout 30000ms exceeded` - naming + neither the shot, nor the selector, nor the fact that a highlight was involved. + An ambiguous selector reports a raw Playwright strict-mode violation. Both land + AFTER every shot's narration has been synthesized, so a script-authoring typo + costs TTS spend. The sibling `click` action already throws a shot-scoped message; + `highlight` is the only selector-bearing action without one. + +3. **A shot that never navigates is a structural trap.** `captureShot` builds a + fresh browser context per shot, so a browser-driven shot carrying selectors but + no `goto` action runs against `about:blank`, where every locator waits out its + full timeout. Nothing detects this before the render. + +Separately, `clipsDir` is declared in the config schema and documented in the README +but read by no code path: prebaked `clip:` paths resolve against the process CWD, so +the same config finds different files depending on where it was invoked from. + +## Goal + +A script author learns that a selector is wrong BEFORE any narration is synthesized, +in a message that names the shot, the selector, and what the selector actually +matched. The overlay API is fail-closed on its own terms rather than by accident of +its caller. Prebaked clip resolution is deterministic and independent of CWD. + +Existing configs that are correct keep rendering unchanged. + +## Scenarios (tracer-bullet slices, dependency order) + +### S1 - Structural findings without a browser + +Given a parsed manifest, when the pre-flight stage inspects it, then every +browser-driven shot that references a selector but declares no `goto` action is +reported as a finding that names the shot and states that a fresh context per shot +means the shot would run against a blank page. A `prebaked` shot that declares +selector-bearing actions is reported too, because a prebaked shot short-circuits +capture and never runs its actions. A shot with no selectors is not reported. This +analysis requires no browser and no network. + +### S2 - Selector resolution against the page each shot actually opens + +Given a manifest whose shots carry selectors, when the pre-flight stage resolves +them, then each selector is resolved against the URL in effect AT THAT POINT in the +shot (a shot may navigate more than once), and a selector is a finding unless it +matches exactly one element. Zero matches, more than one match, and an unparseable +selector are reported as three distinguishable findings, each naming the shot, the +selector, and the observed match count. Shots that share a URL resolve against a +single navigation, in a context as fresh as the one capture builds, so the gate's +verdict never depends on the order shots are declared in. + +### S2b - The gate may only BLOCK on a claim it can actually make + +The gate resolves against a freshly-loaded page and deliberately runs no actions, +so its evidence is weaker than the render's in four specific ways. In each, the +finding is reported at INFO and does not block: + +- a selector downstream of a click or type in the same shot, which the gate cannot + see because it runs no actions; +- an ambiguous `hover`, because capture resolves hover non-strictly and it renders; +- an auth-walled `live` shot, because the gate runs unauthenticated and would see + the login wall; +- a page that did not settle before counting. + +Conversely, the gate must be no stricter than the render where it CAN judge: it +resolves with the same engine capture uses (so Playwright selector syntax and +shadow-DOM piercing behave identically), it counts the page with the same overlay +elements capture injects, and it waits for a late-hydrating element rather than +counting once instantly. + +### S3 - The gate runs before spend, and can be declined + +Given a demo run, when the pipeline starts, then the pre-flight stage runs before +any narration is synthesized and before any capture, and a run with findings fails +with a report of every finding and no TTS spend. An operator may decline the gate +through configuration or a command-line flag, and declining is reported in the run +output rather than silent. A script with no findings renders exactly as it did +before the gate existed. + +### S4 - The highlight overlay fails closed on its own terms + +Given the injected overlay API, when a highlight is requested for a selector that +matches zero elements or more than one, then the overlay raises rather than hiding +its box and returning. Given the highlight action in a capture, when its selector +does not resolve to exactly one element, then the run fails with a message naming +the shot, the selector, and the observed count, and pointing at the pre-flight gate. +A selector that resolves to exactly one element after an initial delay still +succeeds: the action retains the auto-waiting behaviour it has today. + +### S5 - Prebaked clips resolve deterministically + +Given a config that declares `clipsDir`, when a prebaked shot references a clip, +then a bare filename resolves inside `clipsDir`, a path containing a separator +resolves against the config file's directory, and an absolute path is used as +given. `clipsDir` itself resolves against the config file's directory. The same +config finds the same clip from any working directory. A clip that does not exist +at the resolved path fails with a message naming the path that was tried, never a +silent fallback to a different path. + +## Constraints + +- The gate is fail-closed by default with an explicit, reported opt-out. +- Pre-flight must not become a second, divergent definition of "resolves": it + reports a selector as good only under the same exactly-one rule the capture path + enforces. +- No new runtime dependency. Pre-flight uses the Playwright already required. +- Selectors are passed to the page as arguments, never interpolated into evaluated + source. +- Existing configs and scripts that are correct today must render unchanged. + +## Acceptance criteria + +- A script with a zero-match, an ambiguous, and a never-navigating shot fails + pre-flight naming all three, and writes no audio artifact. +- A shot whose element hydrates in after load passes the gate, as it renders. +- A selector using Playwright engine syntax passes the gate, as it renders. +- A selector revealed by an earlier click is reported at INFO and does not block. +- The render receipt records whether the gate ran, was declined, and which shots + it could not adjudicate. +- The same script with those three corrected renders to completion. +- Declining the gate restores the pre-gate behaviour and says so in the output. +- The overlay init script raises on a non-unique selector. +- A prebaked config renders identically from two different working directories. + +## Test seams + +Two seams, both already load-bearing in this repo: + +- `src/preflight.ts` unit tests (`src/preflight.test.ts`) for S1 and for finding + formatting - pure functions over a parsed manifest, no browser. +- `tests/preflight.smoke.test.ts` for S2/S3/S4 - real Chromium against + `tests/fixtures/page.html`, the same seam `tests/capture.smoke.test.ts` uses. +- S5 rides the existing `src/config.test.ts` seam for resolution and + `tests/capture.smoke.test.ts` for the prebaked short-circuit. + +## Verification + +- run: `pnpm test` +- expect: `Test Files` all passed, exit 0 diff --git a/src/capture.ts b/src/capture.ts index b11136f..c24f3a9 100644 --- a/src/capture.ts +++ b/src/capture.ts @@ -13,7 +13,7 @@ import { chromium, type Page } from "playwright"; import { mkdir, rm, writeFile } from "node:fs/promises"; -import { existsSync } from "node:fs"; +import { existsSync, statSync } from "node:fs"; import { join } from "node:path"; import { frameDurations, framesConcatContent, frameTimestampsToSec, cursorMode } from "./screencast.js"; import { ffmpeg, framesEncodeArgs } from "./ffmpeg.js"; @@ -44,21 +44,17 @@ import { clickExpr, chapterExpr, highlightExpr, + highlightBoxExpr, cssInjectScript, } from "./overlay.js"; import { resolveProfileDir } from "./profile.js"; import type { Shot, DemoConfig, TimelineEntry } from "./types.js"; import { waitForReady } from "./ready"; - -/** - * Resolves a URL relative to the dashboardBaseUrl. - * Absolute URLs (http/https/file:) are returned as-is. - */ -function resolveUrl(u: string, baseUrl: string): string { - if (u.startsWith("http") || u.startsWith("file:")) return u; - const base = baseUrl.replace(/\/$/, ""); - return base + (u.startsWith("/") ? u : "/" + u); -} +// Resolves a shot's declared url against dashboardBaseUrl. Lives in its own +// pure module so the pre-flight gate resolves a shot to exactly the page the +// render will open — two definitions could silently disagree. +import { resolveUrl } from "./urls.js"; +import { resolveClipPath } from "./clips.js"; /** * Perform a shot's OPENING navigation and wait for the page to settle, BEFORE @@ -188,9 +184,35 @@ async function runActions( case "highlight": { if (!a.selector) throw new Error(`shot ${shot.id}: highlight action missing selector`); const loc = page.locator(a.selector); - await loc.scrollIntoViewIfNeeded(); - recordEvent(recorder, "highlight", await loc.boundingBox()); - await page.evaluate(highlightExpr(a.selector)); + // The locator still does the WAITING — an element that appears late must + // keep working. Only the diagnosis changes: Playwright reports a + // non-unique selector as a raw strict-mode violation and a missing one + // as a bare timeout, naming neither the shot nor the action. Re-read the + // real count and say what actually went wrong. + try { + await loc.scrollIntoViewIfNeeded(); + } catch (e) { + // Only re-describe the failure when the COUNT actually explains it. + // scrollIntoViewIfNeeded also times out on an actionability problem + // (a display:none element is exactly one match), and rewriting that + // as "resolved to 1 elements, need exactly 1" is self-contradictory + // and points at a gate that would have passed the selector. + const n = await loc.count().catch(() => -1); + if (n === 1) throw e; + throw new Error( + `shot ${shot.id}: highlight selector ${JSON.stringify(a.selector)} resolved to ` + + `${n < 0 ? "an unusable selector" : `${n} elements`}, need exactly 1. ` + + "Run with the preflight gate enabled to catch this before any narration is synthesized.", + { cause: e }, + ); + } + // Resolve the rectangle HERE, with Playwright, and hand the page those + // coordinates. Passing the selector instead would make the page resolve + // it a second time with a different engine (see overlay.ts). + const box = await loc.boundingBox(); + recordEvent(recorder, "highlight", box); + if (!box) throw new Error(`shot ${shot.id}: highlight selector ${JSON.stringify(a.selector)} matched an element with no bounding box (is it hidden?)`); + await page.evaluate(highlightBoxExpr(box)); break; } case "scroll": @@ -393,7 +415,19 @@ export async function captureShot( // Short-circuit for prebaked clips — caller uses the existing file. if (shot.target === "prebaked") { if (!shot.clip) throw new Error(`prebaked shot ${shot.id} has no clip path`); - return shot.clip; + // configDir is absent only when a DemoConfig is built programmatically + // rather than loaded from disk. The CWD fallback keeps that case anchored + // somewhere predictable; it is NOT the old behaviour, which returned the + // declared path verbatim, so a bare filename now resolves through clipsDir. + const clipPath = resolveClipPath(shot.clip, config.clipsDir, config.configDir ?? process.cwd()); + if (!existsSync(clipPath) || !statSync(clipPath).isFile()) { + throw new Error( + `shot ${shot.id}: prebaked clip not found at ${clipPath} ` + + `(clip: ${JSON.stringify(shot.clip)}, clipsDir: ${JSON.stringify(config.clipsDir)}). ` + + "A bare filename resolves inside clipsDir; a path with a directory resolves against the config file's directory.", + ); + } + return clipPath; } await mkdir(outDir, { recursive: true }); diff --git a/src/cli.test.ts b/src/cli.test.ts index ebab31a..40182ff 100644 --- a/src/cli.test.ts +++ b/src/cli.test.ts @@ -8,6 +8,12 @@ describe("parseCommand (CLI dispatch)", () => { it("treats a bare config path as the pipeline run (back-compat)", () => { expect(parseCommand(["demo.config.json"])).toEqual({ cmd: "run", cfgPath: "demo.config.json" }); }); + // A per-run decline of the fail-closed selector gate. The config key is the + // durable declaration; this flag is the one-off override an operator reaches + // for, and it must not swallow the config path positional. + it("accepts --no-preflight as a per-run decline of the selector gate", () => { + expect(parseCommand(["my.json", "--no-preflight"])).toEqual({ cmd: "run", cfgPath: "my.json", preflight: false }); + }); it("defaults to demo.config.json when no path is given", () => { expect(parseCommand([])).toEqual({ cmd: "run", cfgPath: "demo.config.json" }); expect(parseCommand(["login"])).toEqual({ cmd: "login", cfgPath: "demo.config.json" }); diff --git a/src/cli.ts b/src/cli.ts index b435a15..002af01 100644 --- a/src/cli.ts +++ b/src/cli.ts @@ -11,12 +11,26 @@ import { SshTransport } from "./transport"; * - `login ` → interactive auth-profile capture for live SaaS shots. * - ` [--render-host H]` → run the pipeline; --render-host offloads the render * stage to host H over ssh (local render stays default). + * - `--no-preflight` → decline the fail-closed selector gate for this run + * (the config key `preflight` is the durable declaration). */ -export function parseCommand(argv: string[]): { cmd: "login" | "run"; cfgPath: string; renderHost?: string } { +export function parseCommand(argv: string[]): { + cmd: "login" | "run"; + cfgPath: string; + renderHost?: string; + preflight?: boolean; +} { let renderHost: string | undefined; + let preflight: boolean | undefined; const positional: string[] = []; for (let i = 0; i < argv.length; i++) { const a = argv[i]!; + // Consumed as a flag, never pushed to positional: otherwise a leading + // --no-preflight would be read as the config path. + if (a === "--no-preflight") { + preflight = false; + continue; + } if (a === "--render-host") { const next = argv[i + 1]; if (next === undefined || next.startsWith("-")) throw new Error("--render-host requires a host argument"); @@ -31,13 +45,17 @@ export function parseCommand(argv: string[]): { cmd: "login" | "run"; cfgPath: s } positional.push(a); } - if (positional[0] === "login") return { cmd: "login", cfgPath: positional[1] ?? "demo.config.json", renderHost }; - return { cmd: "run", cfgPath: positional[0] ?? "demo.config.json", renderHost }; + if (positional[0] === "login") + return { cmd: "login", cfgPath: positional[1] ?? "demo.config.json", renderHost, ...(preflight === false ? { preflight } : {}) }; + return { cmd: "run", cfgPath: positional[0] ?? "demo.config.json", renderHost, ...(preflight === false ? { preflight } : {}) }; } export async function main(argv: string[]): Promise { - const { cmd, cfgPath, renderHost } = parseCommand(argv); + const { cmd, cfgPath, renderHost, preflight } = parseCommand(argv); const config = loadConfig(cfgPath); + // A per-run decline lands ON the config, so the render receipt's config hash + // records that this artifact shipped without the gate. + if (preflight === false) config.preflight = false; if (cmd === "login") { const dir = await captureLogin(config); console.log("✓ auth profile ready at", dir); diff --git a/src/clips.test.ts b/src/clips.test.ts new file mode 100644 index 0000000..39f5748 --- /dev/null +++ b/src/clips.test.ts @@ -0,0 +1,39 @@ +import { describe, it, expect } from "vitest"; +import { resolveClipPath } from "./clips"; + +describe("resolveClipPath (issue #14)", () => { + // The README promises "place the clip in clipsDir" and then reference it. + // Only a BARE filename gets that treatment: joining clipsDir onto every + // relative path would double it onto the README's own + // `clip: clips/prebaked/uipath-studio.mp4` example and break existing configs. + it("joins a bare filename into clipsDir", () => { + expect(resolveClipPath("uipath.mp4", "clips/prebaked", "/cfg")).toBe("/cfg/clips/prebaked/uipath.mp4"); + }); + + it("resolves a path that already carries a directory against the config dir", () => { + expect(resolveClipPath("clips/prebaked/x.mp4", "clips/prebaked", "/cfg")).toBe("/cfg/clips/prebaked/x.mp4"); + }); + + it("leaves an absolute clip path exactly as given", () => { + expect(resolveClipPath("/elsewhere/x.mp4", "clips/prebaked", "/cfg")).toBe("/elsewhere/x.mp4"); + }); + + // The defect this closes: resolution used to depend on the process CWD, so + // the same config found different files depending on where it was invoked. + it("is independent of the process working directory", () => { + expect(resolveClipPath("x.mp4", "clips/prebaked", "/cfg")).toBe( + resolveClipPath("x.mp4", "clips/prebaked", "/cfg"), + ); + expect(resolveClipPath("x.mp4", "clips/prebaked", "/other")).toBe("/other/clips/prebaked/x.mp4"); + }); + + // "./x.mp4" carries a directory component and must NOT be treated as bare: + // an author writing it means "next to my config", not "inside clipsDir". + it("treats an explicitly-relative path as carrying a directory", () => { + expect(resolveClipPath("./uipath.mp4", "clips/prebaked", "/cfg")).toBe("/cfg/uipath.mp4"); + }); + + it("honours an absolute clipsDir", () => { + expect(resolveClipPath("x.mp4", "/shared/clips", "/cfg")).toBe("/shared/clips/x.mp4"); + }); +}); diff --git a/src/clips.ts b/src/clips.ts new file mode 100644 index 0000000..a7e84b6 --- /dev/null +++ b/src/clips.ts @@ -0,0 +1,33 @@ +import { isAbsolute, resolve } from "node:path"; + +/** + * clips.ts — where a prebaked shot's `clip:` path actually points. + * + * Before this existed, `captureShot` returned `shot.clip` verbatim, so clip + * paths resolved against the PROCESS CWD: the same config found different files + * depending on which directory the pipeline was invoked from, and `clipsDir` + * was declared, defaulted, documented, and read by nothing. + * + * The rule, in order: + * - an absolute clip path is used exactly as given; + * - a BARE filename is joined into `clipsDir` (what the README's "place the + * clip in clipsDir" has always promised); + * - anything else already carries its own directory, and resolves against the + * config file's directory. + * + * The bare-filename split is what keeps this backward compatible: joining + * `clipsDir` onto EVERY relative path would double it onto the README's own + * `clip: clips/prebaked/uipath-studio.mp4` example. + * + * `clipsDir` itself resolves against the config file's directory unless it is + * absolute — the same treatment `dashboardBaseUrl` already gets in loadConfig. + */ +export function resolveClipPath(clip: string, clipsDir: string, configDir: string): string { + if (isAbsolute(clip)) return clip; + // "Bare" means literally no path separator. `dirname(clip) === "."` would + // also call "./x.mp4" bare and quietly send it to clipsDir, contradicting the + // documented rule that a path carrying a directory resolves against the + // config file's directory. + if (!/[\\/]/.test(clip)) return resolve(configDir, clipsDir, clip); + return resolve(configDir, clip); +} diff --git a/src/config.test.ts b/src/config.test.ts index ee62d1b..c1de4a2 100644 --- a/src/config.test.ts +++ b/src/config.test.ts @@ -5,6 +5,28 @@ import { isAbsolute, join } from "node:path"; import { loadConfig } from "./config"; describe("loadConfig", () => { + // Prebaked clip paths used to resolve against the PROCESS CWD, so the same + // config found different files depending on where the pipeline was invoked + // from. Pinning the config file's own directory is what removes that. + it("records the config file's directory, independent of the process CWD", () => { + const dir = mkdtempSync(join(tmpdir(), "cfg-dir-")); + const path = join(dir, "demo.config.json"); + writeFileSync(path, JSON.stringify({ script: "DEMO.md", dashboardBaseUrl: "http://localhost:3000" })); + + const fromHere = loadConfig(path); + const cwd = process.cwd(); + let fromElsewhere; + try { + process.chdir(tmpdir()); + fromElsewhere = loadConfig(path); + } finally { + process.chdir(cwd); + } + + expect(isAbsolute(fromHere.configDir!)).toBe(true); + expect(fromElsewhere.configDir).toBe(fromHere.configDir); + }); + it("applies defaults (fps=30, resolution.width=1920) from minimal config", () => { const dir = mkdtempSync(join(tmpdir(), "cfg-")); const path = join(dir, "demo.config.json"); diff --git a/src/config.ts b/src/config.ts index c74259b..7d15bd5 100644 --- a/src/config.ts +++ b/src/config.ts @@ -18,6 +18,11 @@ export function loadConfig(path: string): DemoConfig { // that a naive string concat would pass through broken. cfg.dashboardBaseUrl = pathToFileURL(resolve(join(dirname(resolve(path)), cfg.dashboardBaseUrl))).href; } + // Prebaked clip paths resolve against THIS file's directory, so the same + // config finds the same clips from any working directory. Recording the dir + // here (rather than resolving clip paths now) keeps the clips themselves in + // the script, which is where an author declares them. + cfg.configDir = dirname(resolve(path)); // Pin the auth profile to an absolute, outside-the-repo path (secrets at rest). if (cfg.capture.auth) { cfg.capture.auth.profileDir = resolveProfileDir(cfg.capture.auth.profileDir, cfg.capture.auth.loginUrl); diff --git a/src/overlay.ts b/src/overlay.ts index 74bd454..be85230 100644 --- a/src/overlay.ts +++ b/src/overlay.ts @@ -136,9 +136,41 @@ export function overlayInitScript(): string { }, 2000); }; + // Draw the box at coordinates the CALLER resolved. + // + // The selector form below resolves with document.querySelectorAll, which is + // CSS-only and does not pierce shadow roots, while capture resolves with + // Playwright's engine. Two engines mean two answers: a selector Playwright + // resolves to one element inside an open shadow root is invisible to + // querySelectorAll. Capture already has a Playwright-resolved bounding box in + // hand, so it passes THAT and no second resolution happens at all. + window.__demoHighlightBox = function(x, y, w, h) { + highlight.style.left = x + 'px'; + highlight.style.top = y + 'px'; + highlight.style.width = w + 'px'; + highlight.style.height = h + 'px'; + highlight.style.opacity = '1'; + setTimeout(function() { highlight.style.opacity = '0'; }, 1500); + }; + window.__demoHighlight = function(selector) { - const el = document.querySelector(selector); - if (!el) { highlight.style.opacity = '0'; return; } + // Fail CLOSED, on this function's own terms. + // + // Hiding the box and returning meant the shot still rendered, the pipeline + // still exited 0, and the highlight simply never happened. AMBIGUITY is a + // failure too: document.querySelector silently takes the FIRST match, which + // is how a bare tag selector looks correct while pointing somewhere else + // entirely. An invalid selector throws out of querySelectorAll by itself, + // which is also the behaviour we want. + const all = document.querySelectorAll(selector); + if (all.length !== 1) { + highlight.style.opacity = '0'; + throw new Error( + '__demoHighlight: selector ' + JSON.stringify(selector) + + ' matched ' + all.length + ' elements (need exactly 1)' + ); + } + const el = all[0]; const r = el.getBoundingClientRect(); highlight.style.left = r.left + 'px'; highlight.style.top = r.top + 'px'; @@ -166,6 +198,18 @@ export function highlightExpr(selector: string): string { return `window.__demoHighlight(${JSON.stringify(selector)})`; } +/** + * Highlight an already-resolved rectangle. + * + * Preferred over `highlightExpr` on any path that has already resolved the + * element with Playwright: it draws exactly what the caller measured, so the + * page never re-resolves the selector with a different engine and cannot + * disagree about which element (or whether any) was meant. + */ +export function highlightBoxExpr(box: { x: number; y: number; width: number; height: number }): string { + return `window.__demoHighlightBox(${box.x}, ${box.y}, ${box.width}, ${box.height})`; +} + /** * Init script that injects an arbitrary CSS stylesheet into the captured page. * Deferred to DOMContentLoaded when is not yet available (addInitScript diff --git a/src/pipeline.ts b/src/pipeline.ts index 310734a..5cf56e5 100644 --- a/src/pipeline.ts +++ b/src/pipeline.ts @@ -4,6 +4,7 @@ import { join, resolve } from "node:path"; import { fileURLToPath } from "node:url"; import type { DemoConfig, TtsResult } from "./types"; import { parseScript } from "./parse-script"; +import { formatPreflightReport, runPreflight } from "./preflight"; import { synthShot } from "./tts"; import { captureShot } from "./capture"; import { titleCardArgs, endCardArgs } from "./cards"; @@ -11,7 +12,7 @@ import { ffmpeg, silentMp3Args } from "./ffmpeg"; import { renderVideo, type RenderResult } from "./render"; import { renderRemote } from "./remote-render"; import type { Transport } from "./transport"; -import { buildRenderReport, digest, stableConfigJson, toolVersions } from "./provenance"; +import { buildRenderReport, digest, stableConfigJson, toolVersions, type PreflightRecord } from "./provenance"; import { resolveTtsMode } from "./tts"; export interface RunPipelineOpts { @@ -67,6 +68,42 @@ export async function runPipeline(config: DemoConfig, opts: RunPipelineOpts = {} const manifest = parseScript(md); const shots = manifest.shots; + // 1.5 Pre-flight selector gate — BEFORE any spend. + // + // Ordering is the whole point. TTS is the first thing this pipeline pays for, + // so a selector that resolves to zero or to many must be caught above it: the + // capture path would otherwise stall a full locator timeout mid-render and + // then report a raw Playwright error naming neither the shot nor the + // selector, with every narration already synthesized. + let preflightRecord: PreflightRecord = { ran: false, declined: true, findings: 0, unverifiedShotIds: [] }; + if (config.preflight) { + const findings = await runPreflight(manifest, config); + if (findings.length > 0) console.warn(formatPreflightReport(findings)); + const blocking = findings.filter((f) => f.severity === "blocking"); + preflightRecord = { + ran: true, + declined: false, + findings: findings.length, + // Which shots shipped with something the gate could NOT adjudicate: + // auth-walled live shots, selectors behind an earlier interaction, a page + // that never settled. Keyed on severity, not on one finding kind, so the + // receipt distinguishes "gated and clean" from "gated, but not where it + // counted" for every reason the gate has to abstain. + unverifiedShotIds: [...new Set(findings.filter((f) => f.severity === "info").map((f) => f.shotId))], + }; + if (blocking.length > 0) { + throw new Error( + `[agent-demo-video] preflight gate failed: ${blocking.length} finding(s); no narration was synthesized. ` + + "Fix the script, or set preflight:false / pass --no-preflight to render anyway.", + ); + } + } else { + // A declined gate must be loud. Silence here would read identically to a + // gate that ran and found nothing, which is the ambiguity the gate exists + // to remove. + console.warn("[agent-demo-video] preflight gate DECLINED: selectors were NOT verified before this render."); + } + // 2. Make dirs const out = resolve(config.out); const audioDir = join(out, "audio"); @@ -195,6 +232,7 @@ export async function runPipeline(config: DemoConfig, opts: RunPipelineOpts = {} render: result.report, maxDurationSec: config.maxDurationSec, renderedOn: opts.render ? "remote" : "local", + preflight: preflightRecord, }); await writeFile(join(out, "render-report.json"), JSON.stringify(report, null, 2)); } catch (e) { diff --git a/src/preflight.test.ts b/src/preflight.test.ts new file mode 100644 index 0000000..2e15352 --- /dev/null +++ b/src/preflight.test.ts @@ -0,0 +1,52 @@ +import { describe, it, expect } from "vitest"; +import { structuralFindings } from "./preflight"; +import { DemoConfigSchema, ManifestSchema } from "./types"; + +const manifest = (shots: unknown[]) => ManifestSchema.parse({ shots }); +const cfg = () => DemoConfigSchema.parse({ script: "x", dashboardBaseUrl: "http://localhost:3000" }); + +describe("structuralFindings", () => { + // captureShot builds a FRESH browser context per shot and closes it after. + // Nothing carries between shots, so a shot that references a selector but + // never navigates for itself runs against about:blank, where every locator + // waits out its full timeout mid-render. That is invisible until the render. + it("flags a browser-driven shot that uses a selector but never navigates", () => { + const f = structuralFindings( + manifest([ + { + id: "no-goto", + target: "dashboard", + narration: "n", + actions: [{ kind: "highlight", selector: "#thing" }], + }, + ]), + cfg(), + ); + expect(f).toHaveLength(1); + expect(f[0]!.shotId).toBe("no-goto"); + expect(f[0]!.kind).toBe("no-navigation"); + expect(f[0]!.message).toMatch(/about:blank/); + }); + + // captureShot short-circuits a prebaked shot before runActions, so selector + // actions on one are dead declarations: they read as instructions and do + // nothing. Reported as their own kind, NOT as no-navigation, because a + // prebaked shot is not supposed to navigate. + it("flags a prebaked shot whose selector actions capture will never run", () => { + const f = structuralFindings( + manifest([ + { + id: "clip-shot", + target: "prebaked", + clip: "x.mp4", + narration: "n", + actions: [{ kind: "highlight", selector: "#thing" }], + }, + ]), + cfg(), + ); + // The nonexistent clip is ALSO a finding now, and both are correct: the + // actions are dead AND the clip is missing. Assert the one under test. + expect(f.map((x) => x.kind).sort()).toEqual(["missing-clip", "prebaked-actions"]); + }); +}); diff --git a/src/preflight.ts b/src/preflight.ts new file mode 100644 index 0000000..612e7ed --- /dev/null +++ b/src/preflight.ts @@ -0,0 +1,329 @@ +import { existsSync, statSync } from "node:fs"; +import { chromium, type Page } from "playwright"; +import { cssInjectScript, overlayInitScript } from "./overlay.js"; +import { waitForReady } from "./ready.js"; +import { resolveUrl } from "./urls.js"; +import { resolveClipPath } from "./clips.js"; +import { redactUrl, scrubControlChars } from "./sanitize.js"; +import type { Action, DemoConfig, Manifest, PreflightFinding, Shot } from "./types"; + +/** Action kinds that cannot run at all without a selector. */ +const SELECTOR_REQUIRED: Action["kind"][] = ["click", "type", "hover", "highlight"]; + +/** + * `hover` is the one selector action capture resolves NON-strictly + * (`page.hover(selector)` defaults to strict:false and hovers the first match), + * so an ambiguous hover renders fine today. The gate mirrors what the render + * actually enforces rather than imposing a stricter rule of its own. + */ +const ambiguityIsFatal = (kind: Action["kind"]): boolean => kind !== "hover"; + +interface SelectorProbe { + shot: Shot; + kind: Action["kind"]; + selector: string; + /** The URL in effect at this action's position, already resolved. */ + url: string; + /** + * True when a click or type ran earlier in this shot since the last + * navigation. The gate deliberately runs NO actions, so anything downstream + * of an interaction is UNKNOWN to it rather than absent. + */ + afterInteraction: boolean; +} + +/** + * Walk a shot's actions in order, tracking the page in effect and whether the + * DOM has been interacted with, so every selector is judged in its own context. + * + * A shot may navigate more than once (`runActions` executes every goto), so + * resolving all of a shot's selectors against its FIRST goto counts later ones + * against the wrong page: fail-open when the name happens to exist on page one, + * false-blocking when it does not. + */ +function selectorProbes(shot: Shot, config: DemoConfig): SelectorProbe[] { + const probes: SelectorProbe[] = []; + let url: string | undefined; + let interacted = false; + for (const a of shot.actions) { + if (a.kind === "goto") { + url = resolveUrl(a.url ?? "/", config.dashboardBaseUrl); + interacted = false; // a navigation rebuilds the DOM + continue; + } + if (a.selector && url) { + probes.push({ shot, kind: a.kind, selector: a.selector, url, afterInteraction: interacted }); + } + if (a.kind === "click" || a.kind === "type") interacted = true; + } + return probes; +} + +/** + * Findings decidable without a browser. + * + * Each is something a render would otherwise discover only after every + * narration has already been paid for. + */ +export function structuralFindings(manifest: Manifest, config: DemoConfig): PreflightFinding[] { + const findings: PreflightFinding[] = []; + for (const shot of manifest.shots) { + // An action that cannot run without a selector, declared without one, is + // invisible to selector resolution (there is nothing to resolve) and throws + // at capture after all TTS spend. + for (const a of shot.actions) { + if (SELECTOR_REQUIRED.includes(a.kind) && !a.selector) { + findings.push({ + shotId: shot.id, + kind: "missing-selector", + severity: "blocking", + message: `shot "${shot.id}": ${a.kind} action declares no selector; capture requires one and would throw mid-render`, + }); + } + } + + if (shot.target === "prebaked") { + // Decidable here, so it costs nothing. A one-character typo in `clip:` + // otherwise synthesizes every narration first and fails at capture. + if (shot.clip) { + const clipPath = resolveClipPath(shot.clip, config.clipsDir, config.configDir ?? process.cwd()); + if (!existsSync(clipPath) || !statSync(clipPath).isFile()) { + findings.push({ + shotId: shot.id, + kind: "missing-clip", + severity: "blocking", + message: + `shot "${shot.id}": prebaked clip not found at ${clipPath} ` + + `(clip: ${JSON.stringify(shot.clip)}, clipsDir: ${JSON.stringify(config.clipsDir)})`, + }); + } + } + // A prebaked shot short-circuits before runActions, so its selector + // actions are dead declarations: they read as instructions and do + // nothing. Never a no-navigation finding, since it is not meant to navigate. + const dead = shot.actions.filter((a) => a.selector).length; + if (dead > 0) { + findings.push({ + shotId: shot.id, + kind: "prebaked-actions", + severity: "blocking", + message: + `shot "${shot.id}" is prebaked but declares ${dead} selector action(s); ` + + "capture returns the clip without opening a browser, so these actions never run", + }); + } + continue; + } + + const selectors = shot.actions.filter((a) => a.selector).length; + if (selectors > 0 && !shot.actions.some((a) => a.kind === "goto")) { + findings.push({ + shotId: shot.id, + kind: "no-navigation", + severity: "blocking", + message: + `shot "${shot.id}" uses ${selectors} selector(s) but declares no goto action; ` + + "capture builds a fresh context per shot, so this shot runs against about:blank " + + "and every locator waits out its full timeout", + }); + } + } + return findings; +} + +/** + * Count matches the way the RENDER counts them. + * + * `page.locator()` is Playwright's engine: it pierces open shadow roots and + * accepts `>> nth=`, `text=`, `:has-text()` and xpath, none of which + * `document.querySelectorAll` understands. Counting with querySelectorAll made + * the gate a second, divergent definition of "resolves" and rejected selectors + * that render correctly today (this repo's own `demos/proctor` ships + * `.toggle-btn >> nth=1`). -1 means genuinely unparseable. + */ +async function countMatches(page: Page, selector: string): Promise { + try { + return await page.locator(selector).count(); + } catch { + return -1; + } +} + +/** + * Resolve every shot's selectors against the page that shot opens at that point. + * + * A selector is good only when it resolves to exactly one element. Zero is a + * silent no-op in the overlay; more than one means the strict locator throws at + * render (except `hover`, which is non-strict). + * + * The gate runs NO actions: driving clicks and typing against a real app before + * a render would mutate the very app being demoed. So anything an interaction + * could have revealed is reported as unverifiable rather than failed. + */ +export async function resolveSelectorFindings( + manifest: Manifest, + config: DemoConfig, +): Promise { + const findings: PreflightFinding[] = []; + const byUrl = new Map(); + + for (const shot of manifest.shots) { + if (shot.target === "prebaked") continue; // structural findings own these + if (!shot.actions.some((a) => a.selector)) continue; + // A live shot drives a saved auth profile. Resolving it with this gate's own + // unauthenticated context would hit the login wall and report every selector + // as a zero-match, so a fail-closed gate would block a correct script. + if (shot.target === "live") { + findings.push({ + shotId: shot.id, + kind: "unverified", + severity: "info", + message: + `shot "${shot.id}" is a live (auth-walled) shot: its selectors were NOT resolved, ` + + "because this gate runs unauthenticated and would see the login wall", + }); + continue; + } + for (const probe of selectorProbes(shot, config)) { + const bucket = byUrl.get(probe.url); + if (bucket) bucket.push(probe); + else byUrl.set(probe.url, [probe]); + } + } + + if (byUrl.size === 0) return findings; + + let browser; + try { + browser = await chromium.launch({ headless: true }); + } catch { + browser = await chromium.launch({ headless: true, channel: "chrome" }); + } + try { + for (const [url, probes] of byUrl) { + // A FRESH context per page, matching capture's fresh-context-per-shot. + // One shared context would accumulate cookies and storage, so a + // first-visit banner would be present for the first URL and gone after, + // making the gate's verdict depend on the ORDER shots are declared in. + const context = await browser.newContext({ viewport: config.resolution }); + // The captured page carries these, and they add elements: the overlay + // appends four divs to document.body. Without them a `div` selector can + // count 1 here and 5 at render. + await context.addInitScript(overlayInitScript()); + if (config.captureCss) await context.addInitScript(cssInjectScript(config.captureCss)); + const page = await context.newPage(); + const safe = redactUrl(url); + try { + let response; + try { + response = await page.goto(url, { waitUntil: "load" }); + } catch (e) { + // The gate is the first thing that touches the network, so a dev + // server that is not up yet would otherwise surface as a raw + // Playwright error naming no shot and never mentioning preflight. + findings.push({ + shotId: probes[0]!.shot.id, + kind: "unreachable", + severity: "blocking", + message: + `could not open ${safe} (${scrubControlChars((e as Error).message, 160)}); ` + + `${probes.length} selector(s) could not be checked`, + }); + continue; + } + // A dev server that serves a 404 page for a bad route would otherwise + // produce one "selector matches nothing" per selector, pointing the + // operator at the script instead of at the route. + if (response && !response.ok()) { + findings.push({ + shotId: probes[0]!.shot.id, + kind: "unreachable", + severity: "blocking", + message: `${safe} returned HTTP ${response.status()}; ${probes.length} selector(s) could not be checked against the real page`, + }); + continue; + } + // waitForReady FAILS OPEN by construction (see ready.ts): it never + // rejects. Dropping its result would let the gate count selectors on a + // pre-hydration skeleton and report that as verified. + const settle = await waitForReady(page, config.capture.settleMs); + if (settle.warning) { + findings.push({ + shotId: probes[0]!.shot.id, + kind: "unverified", + severity: "info", + message: `${safe} did not settle before selectors were counted (${scrubControlChars(settle.warning, 160)}); counts below may not reflect the rendered DOM`, + }); + } + + for (const probe of probes) { + let matches = await countMatches(page, probe.selector); + // The render's locator AUTO-WAITS; counting once, instantly, is + // stricter than the render and would false-block an element that + // hydrates in after load. + if (matches === 0 && !probe.afterInteraction) { + await page + .locator(probe.selector) + .first() + .waitFor({ state: "attached", timeout: config.preflightWaitMs }) + .catch(() => {}); + matches = await countMatches(page, probe.selector); + } + if (matches === 1) continue; + + const where = `shot "${probe.shot.id}": selector ${JSON.stringify(probe.selector)}`; + if (matches < 0) { + findings.push({ + shotId: probe.shot.id, + kind: "invalid-selector", + severity: "blocking", + selector: probe.selector, + message: `${where} is not a selector Playwright can parse`, + }); + continue; + } + // Two reasons the gate must not block on a real difference it cannot + // adjudicate: it ran no actions, and hover is non-strict at render. + const unverifiable = probe.afterInteraction; + const fatal = unverifiable ? false : matches === 0 ? true : ambiguityIsFatal(probe.kind); + const because = unverifiable + ? " (an earlier click or type in this shot can change the DOM, and the gate runs no actions, so this could not be verified)" + : probe.kind === "hover" && matches > 1 + ? " (hover resolves non-strictly at render, so this is reported rather than blocking)" + : ""; + findings.push({ + shotId: probe.shot.id, + kind: matches === 0 ? "no-match" : "ambiguous", + severity: fatal ? "blocking" : "info", + selector: probe.selector, + matches, + message: + matches === 0 + ? `${where} matches nothing on ${safe}${because}` + : `${where} is ambiguous, ${matches} matches on ${safe} (the strict locator needs exactly one)${because}`, + }); + } + } finally { + await context.close(); + } + } + } finally { + await browser.close(); + } + return findings; +} + +/** + * The full gate: structural findings (no browser) plus selector resolution + * against the page each shot opens. Structural findings come first because a + * shot that never navigates cannot be resolved at all, and the author needs to + * see both classes in one pass rather than one render at a time. + */ +export async function runPreflight(manifest: Manifest, config: DemoConfig): Promise { + return [...structuralFindings(manifest, config), ...(await resolveSelectorFindings(manifest, config))]; +} + +/** Render findings as operator-readable lines, severity first so a scan sorts itself. */ +export function formatPreflightReport(findings: PreflightFinding[]): string { + return findings.map((f) => ` ${f.severity === "blocking" ? "BLOCKING" : "INFO "} ${f.message}`).join("\n"); +} diff --git a/src/provenance.test.ts b/src/provenance.test.ts index 3a886b5..d12226e 100644 --- a/src/provenance.test.ts +++ b/src/provenance.test.ts @@ -18,6 +18,7 @@ describe("buildRenderReport", () => { render: { totalSec: 3, segments: 1, ticks: 0, parity: { ok: true, problems: [] } }, maxDurationSec: 300, renderedOn: "local" as const, + preflight: { ran: true, declined: false, findings: 0, unverifiedShotIds: [] as string[] }, }; it("records the resolved voice, so two renders can be compared to explain a runtime change", () => { diff --git a/src/provenance.ts b/src/provenance.ts index 5bb391f..c0dfb6f 100644 --- a/src/provenance.ts +++ b/src/provenance.ts @@ -8,6 +8,22 @@ const exec = promisify(execFile); export type ToolVersions = { ffmpeg: string; ffprobe: string; playwright: string; node: string }; +/** + * Whether this artifact's selectors were verified before it was made. + * + * Without it, a render that DECLINED the gate is indistinguishable from one + * that passed it: the flag survives only inside the 16-hex config digest, which + * also moves for any unrelated edit and cannot be read back. `unverifiedShotIds` + * names the shots the gate could not check at all (auth-walled live shots), so + * "gated and clean" is distinguishable from "gated, but not where it counted". + */ +export type PreflightRecord = { + ran: boolean; + declined: boolean; + findings: number; + unverifiedShotIds: string[]; +}; + export type RenderReport = { voice: DemoConfig["voice"]; ttsMode: TtsMode; @@ -21,6 +37,7 @@ export type RenderReport = { timeline: { entries: TimelineEntry[]; totalSec: number }; render: { totalSec: number; segments: number; ticks: number; parity: { ok: boolean; problems: string[] } }; limits: { maxDurationSec: number }; + preflight: PreflightRecord; }; /** Stable content digest for config/script inputs. Short, for eyeballing in a diff. */ @@ -45,6 +62,7 @@ export function buildRenderReport(o: { render: { totalSec: number; segments: number; ticks: number; parity: { ok: boolean; problems: string[] } }; maxDurationSec: number; renderedOn: "local" | "remote"; + preflight: PreflightRecord; }): RenderReport { return { voice: o.voice, @@ -55,6 +73,7 @@ export function buildRenderReport(o: { timeline: o.timeline, render: o.render, limits: { maxDurationSec: o.maxDurationSec }, + preflight: o.preflight, }; } @@ -96,7 +115,10 @@ export async function toolVersions(): Promise { * which is the exact false attribution this report exists to end. */ export function stableConfigJson(config: DemoConfig): string { - const { out: _out, ...rest } = config; + // configDir is the absolute directory of the config FILE, set by loadConfig. + // Hashing it would give the same committed config a different digest on every + // machine — exactly the machine dependence this function exists to remove. + const { out: _out, configDir: _configDir, ...rest } = config; const auth = rest.capture?.auth; return JSON.stringify({ ...rest, diff --git a/src/sanitize.ts b/src/sanitize.ts new file mode 100644 index 0000000..e94a4e7 --- /dev/null +++ b/src/sanitize.ts @@ -0,0 +1,43 @@ +/** + * sanitize.ts — making untrusted text safe to PRINT. + * + * Pure module: no imports. Two hazards, both from text this pipeline does not + * author: pages it navigates, and URLs an operator declares. + */ + +/** + * Strip control characters from text that originated in a page we may not own. + * + * A page controls its own error strings, and an ANSI escape inside one can + * forge a clean-looking log line in the operator's terminal. `ready.ts` already + * does this for its probe errors; anything else that interpolates page-derived + * text into a printed message needs the same treatment. + */ +export function scrubControlChars(raw: string, max = 200): string { + return String(raw ?? "") + .replace(/[\u0000-\u001f\u007f-\u009f]/g, " ") + .slice(0, max); +} + +/** + * Drop credential-bearing parts of a URL before it is logged. + * + * A `dashboardBaseUrl` legitimately carries a bypass token in its query string + * (a Vercel protection bypass, a signed preview link) or userinfo in + * `https://user:pass@host/`. Pre-flight findings name the URL a selector was + * resolved against, and those findings are exactly what an operator pastes into + * a public issue, so the secret must never reach the message. + */ +export function redactUrl(u: string): string { + try { + const parsed = new URL(u); + parsed.search = ""; + parsed.hash = ""; + parsed.username = ""; + parsed.password = ""; + return parsed.href; + } catch { + // Not parseable as a URL: keep everything before the first query separator. + return (u ?? "").split(/[?#]/)[0] ?? ""; + } +} diff --git a/src/types.ts b/src/types.ts index a2be669..65664a2 100644 --- a/src/types.ts +++ b/src/types.ts @@ -100,7 +100,31 @@ export const DemoConfigSchema = z.object({ // real narration. Real ElevenLabs output has run 19-30% longer. Budget to // roughly 75% of the cap; the check still fires post-render either way. maxDurationSec: z.number().positive().default(300), + // Pre-flight selector gate (specs/preflight-selector-gate-spec.md). Resolves + // every declared selector against the page its own shot opens BEFORE any + // narration is synthesized, and fails the run unless each matches exactly one + // element. Fail-closed by default: a selector mistake otherwise surfaces + // mid-render, after TTS has already been paid for, as a raw Playwright + // locator error that names neither the shot nor the selector. Set false (or + // pass --no-preflight) to decline; declining is reported, never silent. + preflight: z.boolean().default(true), + // How long the gate waits for a selector that is absent at first count before + // calling it missing. The render's locator auto-waits (30s default), so a gate + // that counted once instantly would be STRICTER than the render and would + // false-block an element that hydrates in after load. Kept well under the + // render's timeout so a genuinely absent selector still fails fast. + preflightWaitMs: z.number().min(0).default(3000), + // Directory scanned for prebaked clips. A BARE clip filename resolves inside + // it; a clip path that carries its own directory resolves against the config + // file's directory. Relative clipsDir resolves against the config dir too. clipsDir: z.string().default("clips/prebaked"), + // Absolute directory of the config file. SET BY loadConfig, not authored by + // hand — it is what makes prebaked clip resolution independent of the process + // CWD. Machine-local, so it is excluded from the provenance config hash the + // same way capture.auth.profileDir is. Absent when a DemoConfig is built + // programmatically rather than loaded from disk; resolution then falls back + // to the CWD, which is the pre-existing behaviour. + configDir: z.string().optional(), // Sound design (production-polish S2): synthesized ambient bed auto-ducked // under narration, soft ticks on recorded clicks, quiet sweeps at segment // boundaries. musicPath swaps the synthesized bed for an operator file. @@ -184,6 +208,43 @@ export const DemoConfigSchema = z.object({ }).strict(); export type DemoConfig = z.infer; +/** + * A pre-flight finding: one reason a script would not do what it says. + * + * "structural" kinds are decidable from the manifest alone (no browser); + * "resolution" kinds require loading the page the shot actually opens. + */ +export type PreflightFindingKind = + /** Browser-driven shot references a selector but declares no goto: runs on about:blank. */ + | "no-navigation" + /** An action that cannot run without a selector declares none. */ + | "missing-selector" + /** A prebaked shot's clip is absent at its resolved path. */ + | "missing-clip" + /** The page a shot opens could not be loaded, or returned a non-OK status. */ + | "unreachable" + /** Prebaked shot declares selector actions, which capture short-circuits and never runs. */ + | "prebaked-actions" + /** Selector resolved to zero elements. */ + | "no-match" + /** Selector resolved to more than one element; querySelector would take the first. */ + | "ambiguous" + /** Selector threw in page context (not a valid CSS selector). */ + | "invalid-selector" + /** Shot could not be resolved by the gate (e.g. auth-walled live target). Reported, never silent. */ + | "unverified"; + +export interface PreflightFinding { + shotId: string; + kind: PreflightFindingKind; + /** "blocking" fails the run; "info" is reported and does not gate. */ + severity: "blocking" | "info"; + selector?: string; + /** Observed match count, when the gate got far enough to count. */ + matches?: number; + message: string; +} + export interface Alignment { chars: string[]; startSec: number[]; endSec: number[]; } export interface TtsResult { shotId: string; audioPath: string; durationSec: number; alignment: Alignment; } export interface TimelineEntry { shotId: string; startSec: number; durationSec: number; } diff --git a/src/urls.ts b/src/urls.ts new file mode 100644 index 0000000..b6d961e --- /dev/null +++ b/src/urls.ts @@ -0,0 +1,13 @@ +/** + * urls.ts — how a shot's declared url becomes the URL that shot opens. + * + * Pure module: no imports. Shared by the capture path and the pre-flight gate + * so the gate can never resolve a shot to a different page than the render + * does — a second, divergent definition of "where this shot goes" would be the + * same class of silent disagreement the gate exists to catch. + */ +export function resolveUrl(u: string, baseUrl: string): string { + if (u.startsWith("http") || u.startsWith("file:")) return u; + const base = baseUrl.replace(/\/$/, ""); + return base + (u.startsWith("/") ? u : "/" + u); +} diff --git a/tests/capture.smoke.test.ts b/tests/capture.smoke.test.ts index d2836bb..fa739c4 100644 --- a/tests/capture.smoke.test.ts +++ b/tests/capture.smoke.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect, beforeAll } from "vitest"; -import { mkdtemp, stat, writeFile } from "node:fs/promises"; +import { mkdir, mkdtemp, stat, writeFile } from "node:fs/promises"; import { existsSync } from "node:fs"; import { tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { pathToFileURL } from "node:url"; @@ -28,9 +28,85 @@ describe("stale events artifact (pipeline finding)", () => { it("clears a previous run's events file before capture so obsolete click ticks never leak", async () => { const dir = await mkdtemp(join(tmpdir(), "cap-stale-")); await writeFile(join(dir, "events_p1.json"), JSON.stringify([{ kind: "click", tMs: 500 }])); - const cfg = DemoConfigSchema.parse({ script: "x", dashboardBaseUrl: "http://localhost:3000" }); - const shot = { id: "p1", target: "prebaked" as const, clip: "clips/none.mp4", narration: "demo", actions: [] }; + // The clip must now actually EXIST: prebaked resolution used to return the + // declared path verbatim, so this test previously passed against + // "clips/none.mp4", a file that was never there (issue #14). + await writeFile(join(dir, "clip.mp4"), "stand-in for an mp4"); + const cfg = DemoConfigSchema.parse({ script: "x", dashboardBaseUrl: "http://localhost:3000", configDir: dir, clipsDir: "." }); + const shot = { id: "p1", target: "prebaked" as const, clip: "clip.mp4", narration: "demo", actions: [] }; await captureShot(shot, { shotId: "p1", startSec: 0, durationSec: 1 }, cfg, dir); expect(existsSync(join(dir, "events_p1.json"))).toBe(false); }); }); + +describe("highlight selector diagnostics (issue #13)", () => { + // The sibling `click` action already throws a shot-scoped message. `highlight` + // was the only selector-bearing action without one: an ambiguous selector + // surfaced as a raw Playwright strict-mode violation and a zero-match as a + // bare "Timeout 30000ms exceeded" — neither naming the shot, the selector, or + // even the fact that a highlight was involved. + it("names the shot, the selector, and the real match count when a highlight is not unique", async () => { + const dir = await mkdtemp(join(tmpdir(), "cap-amb-")); + const fixture = pathToFileURL(resolve("tests/fixtures/page.html")).href; + const cfg = DemoConfigSchema.parse({ script: "x", dashboardBaseUrl: "http://localhost:3000", resolution: { width: 1280, height: 720 } }); + const shot = { id: "amb", target: "dashboard" as const, narration: "demo", actions: [ + { kind: "goto" as const, url: fixture }, + { kind: "highlight" as const, selector: "button" }, + ] }; + await expect(captureShot(shot, { shotId: "amb", startSec: 0, durationSec: 1 }, cfg, dir)) + .rejects.toThrow(/shot amb: highlight selector "button" resolved to 2 elements/); + }, 60_000); + + // Regression guard for the fix above. Diagnosing the failure must not cost the + // WAITING: the locator still auto-waits, and only the error message changed. + // Counting matches up front instead would be faster and wrong — an element + // that hydrates in after `load` would start failing a script that works today. + it("still waits for an element that only appears after load", async () => { + const dir = await mkdtemp(join(tmpdir(), "cap-late-")); + const fixture = pathToFileURL(resolve("tests/fixtures/late-element.html")).href; + const cfg = DemoConfigSchema.parse({ script: "x", dashboardBaseUrl: "http://localhost:3000", resolution: { width: 1280, height: 720 } }); + const shot = { id: "late", target: "dashboard" as const, narration: "demo", actions: [ + { kind: "goto" as const, url: fixture }, + { kind: "highlight" as const, selector: "#late" }, + ] }; + const seg = await captureShot(shot, { shotId: "late", startSec: 0, durationSec: 2 }, cfg, dir); + expect(seg.endsWith(".mp4")).toBe(true); + }, 60_000); +}); + +describe("prebaked clip resolution (issue #14)", () => { + /** A config dir holding clips/prebaked/, plus a scratch out dir. */ + async function clipFixture(name: string): Promise<{ cfgDir: string; outDir: string; clipPath: string }> { + const cfgDir = await mkdtemp(join(tmpdir(), "clipcfg-")); + const outDir = await mkdtemp(join(tmpdir(), "clipout-")); + const clipPath = join(cfgDir, "clips", "prebaked", name); + await mkdir(join(cfgDir, "clips", "prebaked"), { recursive: true }); + await writeFile(clipPath, "stand-in for an mp4"); + return { cfgDir, outDir, clipPath }; + } + + // clipsDir was declared in the schema, defaulted, documented in the README at + // :228 and :245 — and read by nothing. `shot.clip` came back verbatim, so a + // bare filename resolved against the PROCESS CWD and a user who followed the + // README got a file-not-found. + it("resolves a bare clip filename inside clipsDir, against the config dir not the CWD", async () => { + const { cfgDir, outDir, clipPath } = await clipFixture("uipath.mp4"); + const cfg = DemoConfigSchema.parse({ + script: "x", dashboardBaseUrl: "http://localhost:3000", configDir: cfgDir, + }); + const shot = { id: "pb", target: "prebaked" as const, clip: "uipath.mp4", narration: "n", actions: [] }; + const got = await captureShot(shot, { shotId: "pb", startSec: 0, durationSec: 1 }, cfg, outDir); + expect(got).toBe(clipPath); + }); + + // No silent fallback to some other path: say which path was tried. + it("fails naming the resolved path when a prebaked clip is missing", async () => { + const { cfgDir, outDir } = await clipFixture("present.mp4"); + const cfg = DemoConfigSchema.parse({ + script: "x", dashboardBaseUrl: "http://localhost:3000", configDir: cfgDir, + }); + const shot = { id: "pb", target: "prebaked" as const, clip: "absent.mp4", narration: "n", actions: [] }; + await expect(captureShot(shot, { shotId: "pb", startSec: 0, durationSec: 1 }, cfg, outDir)) + .rejects.toThrow(/clips[/\\]prebaked[/\\]absent\.mp4/); + }); +}); diff --git a/tests/fixtures/late-element.html b/tests/fixtures/late-element.html new file mode 100644 index 0000000..ed77e2a --- /dev/null +++ b/tests/fixtures/late-element.html @@ -0,0 +1,16 @@ +Late Element Fixture + +

Late Element Fixture

+
waiting
+ + diff --git a/tests/fixtures/reveal.html b/tests/fixtures/reveal.html new file mode 100644 index 0000000..c821b3e --- /dev/null +++ b/tests/fixtures/reveal.html @@ -0,0 +1,18 @@ +Reveal Fixture + +

Reveal Fixture

+ +
+ + diff --git a/tests/overlay.smoke.test.ts b/tests/overlay.smoke.test.ts new file mode 100644 index 0000000..96728ef --- /dev/null +++ b/tests/overlay.smoke.test.ts @@ -0,0 +1,42 @@ +import { describe, it, expect, beforeAll, afterAll } from "vitest"; +import { chromium, type Browser, type Page } from "playwright"; +import { resolve } from "node:path"; +import { pathToFileURL } from "node:url"; +import { highlightExpr, overlayInitScript } from "../src/overlay"; + +/** + * The overlay API is exercised in a REAL page rather than asserted against the + * emitted source. A `toContain("querySelectorAll")` string test passes whether + * or not the function actually raises, which is the same "green means nothing" + * failure this whole change is about. + */ +describe("overlay __demoHighlight (smoke)", () => { + let browser: Browser; + let page: Page; + + beforeAll(async () => { + browser = await chromium.launch({ headless: true }); + const context = await browser.newContext(); + await context.addInitScript(overlayInitScript()); + page = await context.newPage(); + await page.goto(pathToFileURL(resolve("tests/fixtures/page.html")).href, { waitUntil: "load" }); + }, 60_000); + + afterAll(async () => { await browser?.close(); }); + + it("highlights a selector that resolves to exactly one element", async () => { + await expect(page.evaluate(highlightExpr("#bootstrap"))).resolves.toBeUndefined(); + }); + + // The fail-open case that started this: hiding the box and returning means the + // shot renders, the run exits 0, and the highlight simply never happened. + it("raises instead of silently hiding the box when nothing matches", async () => { + await expect(page.evaluate(highlightExpr("#definitely-not-here"))).rejects.toThrow(/matched 0 elements/); + }); + + // document.querySelector silently takes the FIRST match, so an ambiguous + // selector is indistinguishable from a correct one until you inspect frames. + it("raises on an ambiguous selector rather than taking the first match", async () => { + await expect(page.evaluate(highlightExpr("button"))).rejects.toThrow(/matched 2 elements/); + }); +}); diff --git a/tests/preflight.smoke.test.ts b/tests/preflight.smoke.test.ts new file mode 100644 index 0000000..5b3492f --- /dev/null +++ b/tests/preflight.smoke.test.ts @@ -0,0 +1,223 @@ +import { describe, it, expect, beforeAll } from "vitest"; +import { existsSync } from "node:fs"; +import { mkdtemp, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join, resolve } from "node:path"; +import { pathToFileURL } from "node:url"; +import { resolveSelectorFindings } from "../src/preflight"; +import { runPipeline } from "../src/pipeline"; +import { DemoConfigSchema, ManifestSchema } from "../src/types"; + +const fixture = () => pathToFileURL(resolve("tests/fixtures/page.html")).href; + +const shot = (id: string, selector: string) => ({ + id, + target: "dashboard" as const, + narration: "n", + actions: [ + { kind: "goto" as const, url: fixture() }, + { kind: "highlight" as const, selector }, + ], +}); + +describe("resolveSelectorFindings (smoke)", () => { + // The fixture page carries exactly one #bootstrap and exactly two