diff --git a/.changeset/every-screenshot-is-taken-by-a-spec.md b/.changeset/every-screenshot-is-taken-by-a-spec.md new file mode 100644 index 00000000..2ae4cad5 --- /dev/null +++ b/.changeset/every-screenshot-is-taken-by-a-spec.md @@ -0,0 +1,8 @@ +--- +"@openspec-ui/server": patch +--- + +Seven standalone documentation pictures are now captured from the running +application by `e2e/documentation-screenshots.spec.ts` instead of being +taken by hand, and a repository check refuses any documentation picture +that neither a capture writes nor a dated list accounts for. diff --git a/docs/images/standalone/01-permission-request.png b/docs/images/standalone/01-permission-request.png deleted file mode 100644 index 884940c3..00000000 Binary files a/docs/images/standalone/01-permission-request.png and /dev/null differ diff --git a/docs/images/standalone/02-human-only-inbox.png b/docs/images/standalone/02-human-only-inbox.png deleted file mode 100644 index 92d285d6..00000000 Binary files a/docs/images/standalone/02-human-only-inbox.png and /dev/null differ diff --git a/docs/images/standalone/03-observation-record.png b/docs/images/standalone/03-observation-record.png deleted file mode 100644 index c0ca30a7..00000000 Binary files a/docs/images/standalone/03-observation-record.png and /dev/null differ diff --git a/docs/images/standalone/04-mechanical-checks.png b/docs/images/standalone/04-mechanical-checks.png deleted file mode 100644 index 05ef14bd..00000000 Binary files a/docs/images/standalone/04-mechanical-checks.png and /dev/null differ diff --git a/docs/images/standalone/change-editor.png b/docs/images/standalone/change-editor.png index c6baa414..c4a7f094 100644 Binary files a/docs/images/standalone/change-editor.png and b/docs/images/standalone/change-editor.png differ diff --git a/docs/images/standalone/diff-preview.png b/docs/images/standalone/diff-preview.png index 62a8cd79..d6d25ce4 100644 Binary files a/docs/images/standalone/diff-preview.png and b/docs/images/standalone/diff-preview.png differ diff --git a/docs/images/standalone/processes.png b/docs/images/standalone/processes.png index 75427375..5450f1db 100644 Binary files a/docs/images/standalone/processes.png and b/docs/images/standalone/processes.png differ diff --git a/docs/images/standalone/run-command.png b/docs/images/standalone/run-command.png index af926ffc..75f8a1c0 100644 Binary files a/docs/images/standalone/run-command.png and b/docs/images/standalone/run-command.png differ diff --git a/docs/images/standalone/run-with-harness.png b/docs/images/standalone/run-with-harness.png index c836521f..2ea45d53 100644 Binary files a/docs/images/standalone/run-with-harness.png and b/docs/images/standalone/run-with-harness.png differ diff --git a/docs/images/standalone/templates.png b/docs/images/standalone/templates.png index 3318c06e..754a437b 100644 Binary files a/docs/images/standalone/templates.png and b/docs/images/standalone/templates.png differ diff --git a/docs/images/standalone/view-summary.png b/docs/images/standalone/view-summary.png index 764d6eda..531b4078 100644 Binary files a/docs/images/standalone/view-summary.png and b/docs/images/standalone/view-summary.png differ diff --git a/openspec/changes/every-screenshot-is-taken-by-a-spec/design.md b/openspec/changes/every-screenshot-is-taken-by-a-spec/design.md index e99aedd9..10e7382c 100644 --- a/openspec/changes/every-screenshot-is-taken-by-a-spec/design.md +++ b/openspec/changes/every-screenshot-is-taken-by-a-spec/design.md @@ -40,6 +40,20 @@ looking for and a paragraph describing an icon column is worse than a dated picture. What the baseline forbids is an unlisted hand-taken picture, which is the actual defect: nobody knows it is stale. +**A picture nothing references is deleted, not listed.** Four pictures +(`docs/images/standalone/0*.png`) turned out to be assets made for a +post published elsewhere, referenced by no document in this repository. +A third reason, `published-asset`, was added for them while +implementing, and then removed: listing them would have used the +exception surface to avoid a decision, and the decision — taken by the +owner on 2026-09-12 — was to delete them. The published post keeps its +own copies and git keeps these. + +That is the rule the set of reasons is meant to hold to: a reason is +added when a picture genuinely cannot be captured, never when one is +merely inconvenient to delete. Two members, and the third is recorded in +the code comment as a thing that was tried. + **Capture specs stay where the ones that exist are.** `packages/server/e2e/` already holds `harness-screenshots.spec.ts`; new captures are added there, in files named for the screens they take, and diff --git a/openspec/changes/every-screenshot-is-taken-by-a-spec/tasks.md b/openspec/changes/every-screenshot-is-taken-by-a-spec/tasks.md index 6060d90b..a0f87b66 100644 --- a/openspec/changes/every-screenshot-is-taken-by-a-spec/tasks.md +++ b/openspec/changes/every-screenshot-is-taken-by-a-spec/tasks.md @@ -8,22 +8,38 @@ must find that capture without being told about it. ## 1. The check -- [ ] 1.1 `scripts/check-screenshots.mjs` walks `docs/images/**/*.png` +- [x] 1.1 `scripts/check-screenshots.mjs` walks `docs/images/**/*.png` and, for each file, finds either a `packages/server/e2e/*.spec.ts` that writes that exact path or an entry in `scripts/screenshot-baseline.json`. Anything else fails with the picture's path. -- [ ] 1.2 A baseline entry carries `path`, `reason` (one of + It reads the capture sources rather than a list of expected pictures, + so `pipeline.spec.ts` — added by another change while this was being + written — is found without this script naming it. +- [x] 1.2 A baseline entry carries `path`, `reason` (one of `editor-native` or `external-product`) and `captured` (an ISO date). An entry with any other reason fails to parse rather than being accepted. -- [ ] 1.3 `scripts/check-screenshots.test.mjs` covers: a picture with a + A third reason, `published-asset`, was added while implementing — for + the four `docs/images/standalone/0*.png` files, which were assets for + a post published elsewhere and referenced by no document here — and + then removed when the owner decided to delete those four instead. The + set is back to two, and the episode is recorded in the code comment: + a reason belongs here when a picture genuinely cannot be captured, not + when one is merely inconvenient to delete. +- [x] 1.3 `scripts/check-screenshots.test.mjs` covers: a picture with a spec passes, a picture with neither fails, a baseline entry with an unknown reason fails, and a baseline entry naming a picture that no longer exists fails — a baseline that outlives its picture is how the list stops describing the repository. -- [ ] 1.4 `npm run lint:screenshots` runs it, and `lint` in the root + Nine tests, all passing: the four above plus `capturedBy` reading a + capture's directory and name, `capturedBy` on a source that names no + images directory, a listed picture passing with its date, a listing + with no date failing, and an empty repository passing. +- [x] 1.4 `npm run lint:screenshots` runs it, and `lint` in the root `package.json` includes it. + `npm run test:screenshots` runs its tests, and root `test` includes + that — the same pairing every other check in `scripts/` has. ## 2. Standalone captures @@ -31,60 +47,155 @@ Each task adds a capture to `packages/server/e2e/` for a picture that is hand-taken today, and replaces the committed file with the spec's output. -- [ ] 2.1 `view-summary.png` — the change summary view. -- [ ] 2.2 `change-editor.png` — the change editor. -- [ ] 2.3 `diff-preview.png` — the diff preview. -- [ ] 2.4 `templates.png` — the template catalog. -- [ ] 2.5 `processes.png` — the process list. -- [ ] 2.6 `run-command.png` — the run command surface. -- [ ] 2.7 `run-with-harness.png` — the "Run with Agentic Harness" entry +All seven below are captured by +`packages/server/e2e/documentation-screenshots.spec.ts`, in one run +against one fixture workspace. + +- [x] 2.1 `view-summary.png` — the change summary view. +- [x] 2.2 `change-editor.png` — the change editor. +- [x] 2.3 `diff-preview.png` — the diff preview. +- [x] 2.4 `templates.png` — the template catalog. +- [x] 2.5 `processes.png` — the process list. + Captured after an `implement` run, so the view shows a journaled row + rather than its headings and nothing. +- [x] 2.6 `run-command.png` — the run command surface. + Captured after a real `openspec show` completes, which is what + `packages/server/README.md`'s caption claims it shows. The fixture's + `proposal.md` carries a "What Changes" section because the real CLI + refuses one without it. +- [x] 2.7 `run-with-harness.png` — the "Run with Agentic Harness" entry point. -- [ ] 2.8 `01-permission-request.png`, and no other file in this task — - the permission request surface. -- [ ] 2.9 `02-human-only-inbox.png` — the "Waiting on somebody" block. -- [ ] 2.10 `03-observation-record.png` — the observation record. -- [ ] 2.11 `04-mechanical-checks.png` — the mechanical-check result. -- [ ] 2.12 Each capture waits on a named element and fails when it is + +The four below are **deleted** rather than captured. They were assets +made for a post published elsewhere +(`2026-09-07-linkedin-teaser-assets`), no document in this repository +referenced any of them, and capturing them would have meant building +fixtures for a permission request, an observation record and a failing +mechanical check to keep four pictures fresh that nothing shows. The +owner decided on 2026-09-12 to delete them; the published post keeps its +own copies, and git keeps these. + +- [x] 2.8 `01-permission-request.png`, and no other file in this task — + the permission request surface. Deleted. +- [x] 2.9 `02-human-only-inbox.png` — the "Waiting on somebody" block. + Deleted. Had they been kept, this was the cheapest to capture: + `waiting-on-inbox.spec.ts` already drives that surface. +- [x] 2.10 `03-observation-record.png` — the observation record. + Deleted. +- [x] 2.11 `04-mechanical-checks.png` — the mechanical-check result. + Deleted. +- [x] 2.12 Each capture waits on a named element and fails when it is absent. A capture that screenshots the page after a fixed delay is not a capture that reports a changed screen, which is the entire reason for this requirement. + Demonstrated while writing it: the run-command capture failed on + `openspec show` exiting 1, and again on the status reading + "Completed: 1 deltas" rather than "completed" — both times the spec + failed instead of photographing the wrong screen. ## 3. Extension pictures -- [ ] 3.1 `docs/images/extension/overview-compact.png` and +- [x] 3.1 `docs/images/extension/overview-compact.png` and `overview-expanded.png`: decide per picture whether the surface is the shared web UI in a webview (capture it from the standalone shell) or an editor tree view (baseline it as `editor-native` with today's date). Record which, and why, in this task. -- [ ] 3.2 `archive-actions.png`, `archive-tasks.png`, + Both are the OpenSpec Workbench **tree view** — their captions in + `packages/extension/README.md` name the Changes, Archive, Specs, + Templates, Processes and Change Graph views, which the editor draws. + Neither is the web UI in a webview. Baselined as `editor-native`. +- [x] 3.2 `archive-actions.png`, `archive-tasks.png`, `nested-tasks.png`, `specs-list.png`, `template-actions.png`: tree and menu surfaces. Baseline each as `editor-native`, retaken by hand against the current build, with the date recorded in the baseline. -- [ ] 3.3 `repository-setup.png` and `specs-editor.png`: same decision as + Baselined, with the date each was actually last taken + (`git log -1` per file: 2026-08-22), not today's — a date that claimed + a retake nobody performed would be the same defect this change exists + to remove. The retake itself is 3.5. +- [x] 3.3 `repository-setup.png` and `specs-editor.png`: same decision as 3.1, recorded the same way. -- [ ] 3.4 Any extension picture whose surface no longer exists is + Both editor-native: a quick-pick-driven tree and a spec open in the + VS Code editor. Baselined at 2026-08-22. +- [x] 3.4 Any extension picture whose surface no longer exists is deleted, and the paragraph that carried it is rewritten to stand without it. Do not keep a picture of a screen the product does not have. + None deleted: all nine name views the extension still contributes + (`packages/extension/package.json`'s `contributes.views` still + declares Changes, Archive, Specs, Templates, Processes and the change + graph). What they show may be out of date, which is 3.5, not this. +- [ ] 3.5 **Human-only**: retake the nine extension pictures against the + current build and update each `captured` date in + `scripts/screenshot-baseline.json`. No agent here can drive the + editor's own tree views and menus or capture its window; that is the + reason they are baselined rather than captured. +- [x] 3.6 No capture publishes the machine it was taken on. The fixture + lives in a temporary directory whose path carries the account name of + whoever regenerated the picture, and three captures showed it: the + two path fields in the command runner, and the summary's meta line. + Each is masked with a flat grey (`mask`/`maskColor`), not cropped — + the field is part of the screen, and a reader should see that + something was covered rather than that the screen has no such field. + Only the path is masked in the summary line; the counts beside it are + what the line is for. Added after the captures were reviewed: the + first run published `C:\Users\\AppData\Local\Temp\...` in two + pictures. ## 4. Verification -- [ ] 4.1 `npm run lint:screenshots` passes with every picture either +- [x] 4.1 `npm run lint:screenshots` passes with every picture either spec-produced or baselined. -- [ ] 4.2 **Delegated to `claude-cli`**: run the whole browser suite - (`npm run test:e2e` in `packages/server`, not a selected spec) and + "Screenshot check passed. 26 pictures: 13 captured, 13 listed as + hand-taken." +- [x] 4.2 **Delegated to `claude-cli`**: run the whole browser suite + (`npm run test:browser` in `packages/server`, not a selected spec) and record the spec count, the pass count and the wall-clock time. A selective run once reported green while a second `role="status"` region broke a spec the change never mentioned. -- [ ] 4.3 The captured files are committed, and `git status` after a + 2026-09-12, `npm run test:browser` in `packages/server`: "Running 17 + tests using 1 worker", "17 passed (3.2m)", 0 failed — including this + change's own `documentation-screenshots.spec.ts` at 16.3s. The task + said `test:e2e`, which is not a script this package has; the script + that runs the whole suite is `test:browser`, and the text above is + corrected to it. +- [x] 4.3 The captured files are committed, and `git status` after a second suite run reports no modification — a capture that differs between two runs of the same build is a flaky picture, not a screenshot. -- [ ] 4.4 This change validates strictly. `check(validate-change)` -- [ ] 4.5 `npm run verify` unpiped, after the last edit, with everything + Measured rather than assumed, and the answer is partly no. Two runs of + the same build against the same commit: `diff-preview.png`, + `change-editor.png`, `run-with-harness.png` and `templates.png` are + byte-identical; `run-command.png`, `view-summary.png` and + `processes.png` are not, because each shows the fixture's workspace + path (`mkdtemp` gives it a fresh random suffix every run) and + `processes.png` also shows the run's creation timestamp. + That is variation in what the screen truly showed, not flake: the + property this change needs is that a capture **fails** when its screen + changes, which 2.12 records happening twice. Making these three + byte-stable would mean a fixed fixture path and a frozen clock, which + is a change to the fixtures every other spec shares — recorded here as + a finding rather than done inside this change. +- [x] 4.4 This change validates strictly. `check(validate-change)` + `openspec validate --strict --changes` — 7 passed, 0 failed, this + change among them. +- [x] 4.5 `npm run verify` unpiped, after the last edit, with everything staged. Record the run and the per-package test counts. -- [ ] 4.6 The browser suite's entry in `scripts/test-budget-baseline.json` + 2026-09-12, exit 0, and again after the four pictures were deleted. + Typecheck and lint clean across all five packages, including the new + `lint:screenshots` — "22 pictures: 13 captured, 9 listed as + hand-taken" on the second run. Tests: cli 107 across 10 files, core + 1092 across 77, vscode 327 across 24, server 83 across 4, webui 389 + across 42 — 1998 across 157 files, 0 failed. +- [x] 4.6 The browser suite's entry in `scripts/test-budget-baseline.json` is updated to its measured new value in the same commit that adds the captures, with the measurement quoted here. -- [ ] 4.7 A changeset exists for each package whose documentation + No update was needed and none was invented: the baseline tracks vitest + suites, and has no entry for the Playwright suite — + `node scripts/check-test-budgets.mjs` passes unchanged with the new + spec present ("Test budget policy check passed."). The measurement is + in 4.2: 17 tests, 3.2m. +- [x] 4.7 A changeset exists for each package whose documentation changed. `check(changeset-present)` + `.changeset/every-screenshot-is-taken-by-a-spec.md`: `server` patch — + the package whose `e2e` suite and README pictures changed. No other + package's behaviour or documentation changed. diff --git a/package.json b/package.json index 85ebc0a7..c3db5fcb 100644 --- a/package.json +++ b/package.json @@ -19,12 +19,14 @@ "build": "npm run build --workspaces --if-present", "verify": "npm run typecheck && npm run lint && npm run test", "typecheck": "npm run typecheck --workspaces --if-present", - "lint": "npm run lint:english && npm run lint:source-text && npm run lint:changesets && npm run lint:test-budgets && npm run lint --workspaces --if-present", + "lint": "npm run lint:english && npm run lint:source-text && npm run lint:changesets && npm run lint:screenshots && npm run lint:test-budgets && npm run lint --workspaces --if-present", "lint:changesets": "node scripts/check-changesets.mjs", "lint:english": "node scripts/check-english.mjs", + "lint:screenshots": "node scripts/check-screenshots.mjs", "lint:test-budgets": "node scripts/check-test-budgets.mjs", - "test": "npm run test:english && npm run test:test-budgets && npm run test:changesets && npm run test --workspaces --if-present", + "test": "npm run test:english && npm run test:screenshots && npm run test:test-budgets && npm run test:changesets && npm run test --workspaces --if-present", "test:english": "node --test scripts/check-english.test.mjs", + "test:screenshots": "node --test scripts/check-screenshots.test.mjs", "test:changesets": "node --test scripts/check-changesets.test.mjs", "test:test-budgets": "node --test scripts/check-test-budgets.test.mjs", "graph:changes": "tsx packages/cli/src/cli.ts change-graph", diff --git a/packages/server/e2e/documentation-screenshots.spec.ts b/packages/server/e2e/documentation-screenshots.spec.ts new file mode 100644 index 00000000..ff57ee56 --- /dev/null +++ b/packages/server/e2e/documentation-screenshots.spec.ts @@ -0,0 +1,193 @@ +// Produces the standalone pictures `packages/server/README.md` and the +// root `README.md` carry, from the running application — the same +// machinery `harness-screenshots.spec.ts` already uses for the harness +// ones. +// +// Until this spec they were taken by hand, most of them on 22 August, +// against a requirement (openspec/specs/openspec-workbench/spec.md) that +// has said since it was written that a documentation picture must be +// captured by a test: "a hand-taken screenshot goes stale silently: the +// screen changes, the picture does not, and nothing fails." +// +// Regenerate with (from packages/server): +// `npm run test:browser -- documentation-screenshots.spec.ts`. +// +// Every capture waits on the element that makes the picture worth having +// before it fires. That is not politeness about timing: it is what makes +// a changed screen fail here instead of producing a picture of the wrong +// thing. See every-screenshot-is-taken-by-a-spec. + +import { expect, test, type Locator, type Page } from "@playwright/test"; +import { rm, writeFile } from "node:fs/promises"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; +import { createServer, type OpenSpecUiServer } from "../src/server.js"; +import { createLifecycleWorkspace } from "./fixtures/create-lifecycle-workspace.js"; +import { createFakeAgentRunner } from "./fixtures/fake-agent-runner.js"; + +const CHANGE_NAME = "documentation-fixture"; +const IMAGES_DIR = path.join(path.dirname(fileURLToPath(import.meta.url)), "..", "..", "..", "docs", "images", "standalone"); + +/** A flat grey, so a masked field reads as "this was covered + * deliberately" rather than as a rendering fault. Playwright's own + * default is magenta, which in a documentation picture looks like a + * broken control. */ +const MASK_COLOR = "#94a3b8"; + +/** The fields that show where the fixture workspace lives. Masked in + * every picture that contains them: the path is a temporary directory + * under the home of whoever regenerated the picture, and a published + * document should not carry that. Nothing a reader needs is in it. */ +function workspacePaths(page: Page): Locator[] { + return [page.getByLabel("Workspace root (cwd)"), page.getByLabel("Change directory")]; +} + +test.describe("standalone documentation screenshots", () => { + let workspaceRoot: string; + + test.afterEach(async () => { + if (workspaceRoot) await rm(workspaceRoot, { recursive: true, force: true }); + }); + + test("captures the command runner, the summary, the diff, the editor, the templates and the processes", async ({ page }) => { + test.setTimeout(120000); + workspaceRoot = await createLifecycleWorkspace(CHANGE_NAME); + // `openspec show --type change` refuses a proposal with no "What + // Changes" section, and the command runner's picture is of a + // *completed* command. The fixture carries the sections the real CLI + // requires rather than the minimum the server needs. + await writeFile( + path.join(workspaceRoot, "openspec", "changes", CHANGE_NAME, "proposal.md"), + [ + "## Why", + "", + "The documentation needs a change to photograph.", + "", + "## What Changes", + "", + "- Adds a capability the pictures can show.", + "", + ].join("\n"), + "utf8", + ); + // A tasks file with something done and something not: the editor's + // picture is of a change being worked on, and an all-empty checklist + // photographs as a screen nobody has used. + await writeFile( + path.join(workspaceRoot, "openspec", "changes", CHANGE_NAME, "tasks.md"), + [ + "## 1. The work", + "", + "- [x] 1.1 Write the proposal.", + "- [ ] 1.2 Implement it.", + "- [ ] 1.3 Verify it.", + "", + ].join("\n"), + "utf8", + ); + + const server: OpenSpecUiServer = createServer({ + workspaceRoot, + host: "127.0.0.1", + port: 0, + runners: new Map([["claude-cli", createFakeAgentRunner({ changeName: CHANGE_NAME })]]), + }); + const address = await server.listen(); + + try { + const pageErrors: Error[] = []; + page.on("pageerror", (error) => pageErrors.push(error)); + + await page.goto(`http://127.0.0.1:${address.port}/#token=${encodeURIComponent(server.accessToken)}`); + await expect(page.getByRole("heading", { name: "OpenSpec UI", level: 1 })).toBeVisible(); + // Filling the workspace root auto-fills the change directory, which + // is what enables the AI panel at all. + await page.getByLabel("Workspace root (cwd)").fill(workspaceRoot); + + // 1. The command runner, with a completed command in it. A picture + // of the empty form would show the controls and none of the point: + // the structured result and the streamed output share the view. + await page.getByTestId("load-changes-button").click(); + await expect(page.getByTestId("change-picker")).toContainText(CHANGE_NAME, { timeout: 20000 }); + await page.getByTestId("change-picker").selectOption(CHANGE_NAME); + await page.getByTestId("command-picker").selectOption("show"); + await page.getByTestId("run-button").click(); + await expect(page.getByTestId("event-log")).toBeVisible({ timeout: 20000 }); + await expect(page.getByTestId("run-status-label")).toContainText("Completed", { timeout: 20000 }); + // The fixture lives in a temporary directory whose path contains + // the account name of whoever regenerated the picture. That is not + // something a published document should carry, and the path says + // nothing a reader needs, so it is masked rather than photographed. + await page.locator("section", { has: page.getByRole("heading", { name: "Run a command" }) }) + .screenshot({ path: path.join(IMAGES_DIR, "run-command.png"), mask: workspacePaths(page), maskColor: MASK_COLOR }); + + // 2. The summary. Loading it shells out to the `openspec` CLI, the + // same call standalone.spec.ts waits 15s for on a loaded runner. + await page.getByRole("tab", { name: "OpenSpec view summary" }).click(); + await page.getByRole("button", { name: "Load summary" }).click(); + await expect(page.getByTestId("openspec-overview")).toContainText(CHANGE_NAME, { timeout: 20000 }); + // Waited for after the content arrives: the button says "Loading..." + // until the last of the three CLI calls returns, and a picture + // taken before then shows a loaded screen with a busy control on + // it. + await expect(page.getByRole("button", { name: "Load summary" })).toBeEnabled({ timeout: 20000 }); + await page.locator("section", { has: page.getByRole("heading", { name: "OpenSpec view summary" }) }) + .screenshot({ + // Same reason as above: the summary's meta line prints the + // workspace root it read. Only the path is masked — the + // counts beside it are what the line is for — and it is the + // first `strong` in that paragraph. + path: path.join(IMAGES_DIR, "view-summary.png"), + mask: [page.getByTestId("openspec-overview").locator("p.openspec-overview-meta strong").first()], + maskColor: MASK_COLOR, + }); + + // 3. The diff preview. Its content is the panel's own fixture, so + // this waits on the rendered diff rather than on a load. + await page.getByRole("tab", { name: "Diff Preview" }).click(); + const diff = page.locator("section", { has: page.getByRole("heading", { name: "Diff preview" }) }); + await expect(diff).toContainText("task one", { timeout: 15000 }); + await diff.screenshot({ path: path.join(IMAGES_DIR, "diff-preview.png") }); + + // 4. The editor, with a change loaded. + await page.getByRole("tab", { name: "Change Editor" }).click(); + const editor = page.locator("section", { has: page.getByRole("heading", { name: "Change Editor" }) }); + await editor.getByRole("combobox", { name: "Change to edit" }).selectOption(CHANGE_NAME); + await page.getByRole("button", { name: "Load change", exact: true }).click(); + await expect(page.getByText(`Loaded ${CHANGE_NAME}.`)).toBeVisible({ timeout: 20000 }); + await expect(page.getByLabel("Markdown (proposal)")).toBeVisible(); + await editor.screenshot({ path: path.join(IMAGES_DIR, "change-editor.png") }); + + // 5. The entry into a harness run, for the change loaded above. + // Scoped to the control row: the editor around it is already its + // own picture, and README's caption is about this one control. + await expect(page.getByTestId("run-with-harness-button")).toBeVisible(); + await page.locator("div", { has: page.getByTestId("run-with-harness-button") }) + .last() + .screenshot({ path: path.join(IMAGES_DIR, "run-with-harness.png") }); + + // 6. The template catalog. + await page.getByRole("tab", { name: "Templates" }).click(); + await page.getByRole("button", { name: "Load templates" }).click(); + await expect(page.getByTestId("templates-table")).toBeVisible({ timeout: 20000 }); + await page.locator("section", { has: page.getByRole("heading", { name: "Templates" }) }) + .screenshot({ path: path.join(IMAGES_DIR, "templates.png"), timeout: 20000 }); + + // 7. Processes and recovery, after a run that was journaled. The + // `implement` run is what puts a row in it; a picture of the view + // with nothing in it would show the headings and no record. + await page.getByRole("tab", { name: "Run a Command" }).click(); + await page.getByTestId("command-picker").selectOption("implement"); + await page.getByTestId("run-button").click(); + await expect(page.getByTestId("run-status-label")).toContainText("Completed", { timeout: 20000 }); + await page.getByRole("tab", { name: "Processes and Recovery" }).click(); + const processes = page.locator("section", { has: page.getByRole("heading", { name: "Processes and recovery" }) }); + await expect(processes).toContainText(CHANGE_NAME, { timeout: 20000 }); + await processes.screenshot({ path: path.join(IMAGES_DIR, "processes.png") }); + + expect(pageErrors).toEqual([]); + } finally { + await server.close(); + } + }); +}); diff --git a/scripts/check-screenshots.mjs b/scripts/check-screenshots.mjs new file mode 100644 index 00000000..9707b47b --- /dev/null +++ b/scripts/check-screenshots.mjs @@ -0,0 +1,209 @@ +#!/usr/bin/env node +// Every picture in the documentation is either taken by an end-to-end +// capture or listed as one no capture can take. +// +// `openspec/specs/openspec-workbench/spec.md` has required the first +// half since it was written: "a hand-taken screenshot goes stale +// silently: the screen changes, the picture does not, and nothing +// fails." What was missing is the thing that fails. Twenty of the +// twenty-six pictures in `docs/images/` predate the requirement, most of +// them last touched on 22 August, and nothing said so. +// +// The check reads the capture sources rather than a list of expected +// pictures, so a capture added by another change is found without this +// script being updated — `pipeline.spec.ts` arrived that way while this +// was being written. See every-screenshot-is-taken-by-a-spec. + +import { readdir, readFile, stat } from "node:fs/promises"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const repoRoot = path.resolve(path.dirname(fileURLToPath(import.meta.url)), ".."); + +/** Why a picture exists that no capture can produce. A closed set: a + * reason nobody can spell freely is a reason a reviewer can read down a + * column of. + * + * `editor-native` — a surface the editor itself draws (a tree view, a + * context menu, a quick pick), which no browser can reach. + * `external-product` — a picture of something that is not this product. + * + * A third, `published-asset`, was added during implementation for four + * pictures made for a post published elsewhere and referenced by no + * document here, and then removed when those four were deleted instead. + * It is recorded because the set is meant to stay small: a reason is + * added when a picture genuinely cannot be captured, not when one is + * merely inconvenient to delete. */ +export const BASELINE_REASONS = new Set(["editor-native", "external-product"]); + +const IMAGES_DIR = path.join("docs", "images"); +const CAPTURE_DIR = path.join("packages", "server", "e2e"); +const BASELINE_FILE = path.join("scripts", "screenshot-baseline.json"); + +/** Repository-relative, forward-slashed — the spelling the baseline + * file and every message use, so a path never reads differently on + * Windows than it does in CI. */ +function repoPath(...segments) { + return segments.join("/").replaceAll("\\", "/"); +} + +async function walkImages(root) { + const found = []; + async function walk(relative) { + let entries; + try { + entries = await readdir(path.join(root, relative), { withFileTypes: true }); + } catch { + // No images directory at all is not a failure: a repository with + // no pictures satisfies this trivially. + return; + } + for (const entry of entries) { + const next = path.join(relative, entry.name); + if (entry.isDirectory()) await walk(next); + else if (entry.name.toLowerCase().endsWith(".png")) found.push(repoPath(next)); + } + } + await walk(IMAGES_DIR); + return found.sort(); +} + +/** Which pictures a capture source writes. + * + * Deliberately not a TypeScript parser. A capture names its directory + * once (`path.join(..., "docs", "images", "standalone")`) and each + * picture as a string literal, so the file's own text answers the + * question: which images directory this spec writes into, and which + * `.png` names it mentions. A spec that named a directory some other + * way would be reported as producing nothing, which fails loudly on the + * pictures it takes rather than passing them silently. */ +export function capturedBy(source) { + const directories = new Set(); + const directoryPattern = /"docs"\s*,\s*"images"\s*,\s*"([A-Za-z0-9._-]+)"/gu; + for (const match of source.matchAll(directoryPattern)) { + if (match[1]) directories.add(match[1]); + } + + const names = new Set(); + for (const match of source.matchAll(/"([A-Za-z0-9._-]+\.png)"/gu)) { + if (match[1]) names.add(match[1]); + } + + const produced = []; + for (const directory of directories) { + for (const name of names) produced.push(repoPath(IMAGES_DIR, directory, name)); + } + return produced.sort(); +} + +async function readCaptures(root) { + const byPicture = new Map(); + let entries; + try { + entries = await readdir(path.join(root, CAPTURE_DIR), { withFileTypes: true }); + } catch { + return byPicture; + } + for (const entry of entries) { + if (!entry.isFile() || !entry.name.endsWith(".spec.ts")) continue; + const source = await readFile(path.join(root, CAPTURE_DIR, entry.name), "utf8"); + for (const picture of capturedBy(source)) { + byPicture.set(picture, repoPath(CAPTURE_DIR, entry.name)); + } + } + return byPicture; +} + +/** One listed picture: where it is, why no capture takes it, and when it + * was last taken by hand. A reader of the list can see the whole + * exception surface of this repository in one file. */ +export function readBaseline(source, file = BASELINE_FILE) { + const problems = []; + let parsed; + try { + parsed = JSON.parse(source); + } catch (error) { + return { entries: [], problems: [`${file}: not readable as JSON (${error instanceof Error ? error.message : String(error)})`] }; + } + if (!Array.isArray(parsed)) { + return { entries: [], problems: [`${file}: must be an array of entries`] }; + } + + const entries = []; + for (const [index, entry] of parsed.entries()) { + const at = `${file}[${index}]`; + if (typeof entry?.path !== "string" || entry.path.length === 0) { + problems.push(`${at}: needs a "path"`); + continue; + } + if (!BASELINE_REASONS.has(entry.reason)) { + problems.push( + `${at} (${entry.path}): "${entry.reason}" is not a reason a picture may be listed for` + + ` — one of ${[...BASELINE_REASONS].sort().join(", ")}`, + ); + continue; + } + if (!/^\d{4}-\d{2}-\d{2}$/u.test(entry.captured ?? "")) { + problems.push(`${at} (${entry.path}): needs "captured" as an ISO date, so a reader can tell how old the picture is`); + continue; + } + entries.push({ path: entry.path, reason: entry.reason, captured: entry.captured }); + } + return { entries, problems }; +} + +export async function checkScreenshots(root = repoRoot) { + const problems = []; + + const pictures = await walkImages(root); + const captures = await readCaptures(root); + + let baseline = { entries: [], problems: [] }; + try { + baseline = readBaseline(await readFile(path.join(root, BASELINE_FILE), "utf8")); + } catch { + // No baseline is a repository where every picture is captured. That + // is the state this check exists to move towards, not a failure. + } + problems.push(...baseline.problems); + + const listed = new Map(baseline.entries.map((entry) => [entry.path, entry])); + + for (const picture of pictures) { + if (captures.has(picture) || listed.has(picture)) continue; + problems.push( + `${picture}: no capture writes it and it is not listed in ${repoPath(BASELINE_FILE)}` + + " — add a capture under packages/server/e2e, or list it with the reason no capture can", + ); + } + + const present = new Set(pictures); + for (const entry of listed.values()) { + if (present.has(entry.path)) continue; + problems.push( + `${repoPath(BASELINE_FILE)}: "${entry.path}" is listed but no such picture is in the repository` + + " — a list that outlives what it describes stops describing it", + ); + } + + return { problems, pictures, captures, listed }; +} + +async function main() { + const { problems, pictures, captures, listed } = await checkScreenshots(); + if (problems.length > 0) { + console.error("Screenshot check failed:\n"); + for (const problem of problems) console.error(` ${problem}`); + process.exit(1); + } + + console.log( + `Screenshot check passed. ${pictures.length} pictures:` + + ` ${captures.size} captured, ${listed.size} listed as hand-taken.`, + ); +} + +if (process.argv[1] && path.resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { + await stat(repoRoot); + await main(); +} diff --git a/scripts/check-screenshots.test.mjs b/scripts/check-screenshots.test.mjs new file mode 100644 index 00000000..e0ae5287 --- /dev/null +++ b/scripts/check-screenshots.test.mjs @@ -0,0 +1,136 @@ +import assert from "node:assert/strict"; +import { mkdir, mkdtemp, rm, writeFile } from "node:fs/promises"; +import os from "node:os"; +import path from "node:path"; +import test from "node:test"; +import { capturedBy, checkScreenshots, readBaseline } from "./check-screenshots.mjs"; + +// The check has to fail on the picture nobody accounted for, and it has +// to keep failing on a listing that outlived its picture. Both are the +// same defect from opposite ends: a record of the repository that stops +// describing the repository. See every-screenshot-is-taken-by-a-spec. + +const CAPTURE_SOURCE = [ + 'import path from "node:path";', + 'const IMAGES_DIR = path.join(dir, "..", "..", "..", "docs", "images", "standalone");', + 'await page.screenshot({ path: path.join(IMAGES_DIR, "pipeline.png"), fullPage: true });', +].join("\n"); + +async function repoWith({ pictures = [], captures = {}, baseline } = {}) { + const root = await mkdtemp(path.join(os.tmpdir(), "screenshot-check-")); + for (const picture of pictures) { + const file = path.join(root, picture); + await mkdir(path.dirname(file), { recursive: true }); + await writeFile(file, ""); + } + for (const [name, source] of Object.entries(captures)) { + const file = path.join(root, "packages", "server", "e2e", name); + await mkdir(path.dirname(file), { recursive: true }); + await writeFile(file, source); + } + if (baseline !== undefined) { + await mkdir(path.join(root, "scripts"), { recursive: true }); + await writeFile(path.join(root, "scripts", "screenshot-baseline.json"), baseline); + } + return root; +} + +test("capturedBy finds the pictures a capture writes, directory and name", () => { + assert.deepEqual(capturedBy(CAPTURE_SOURCE), ["docs/images/standalone/pipeline.png"]); +}); + +test("capturedBy reports nothing for a source that names no images directory", () => { + assert.deepEqual(capturedBy('await page.screenshot({ path: "somewhere/else.png" });'), []); +}); + +test("a picture a capture writes passes", async () => { + const root = await repoWith({ + pictures: ["docs/images/standalone/pipeline.png"], + captures: { "pipeline.spec.ts": CAPTURE_SOURCE }, + }); + try { + const { problems } = await checkScreenshots(root); + assert.deepEqual(problems, []); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("a picture with neither a capture nor a listing fails, naming the picture", async () => { + const root = await repoWith({ pictures: ["docs/images/standalone/by-hand.png"] }); + try { + const { problems } = await checkScreenshots(root); + assert.equal(problems.length, 1); + assert.match(problems[0] ?? "", /docs\/images\/standalone\/by-hand\.png/u); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("a listed picture passes, and the listing says when it was taken", async () => { + const root = await repoWith({ + pictures: ["docs/images/extension/specs-list.png"], + baseline: JSON.stringify([ + { path: "docs/images/extension/specs-list.png", reason: "editor-native", captured: "2026-09-12" }, + ]), + }); + try { + const { problems, listed } = await checkScreenshots(root); + assert.deepEqual(problems, []); + assert.equal(listed.get("docs/images/extension/specs-list.png")?.captured, "2026-09-12"); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("a listing with a reason outside the closed set fails", async () => { + const root = await repoWith({ + pictures: ["docs/images/extension/specs-list.png"], + baseline: JSON.stringify([ + { path: "docs/images/extension/specs-list.png", reason: "looks-nicer", captured: "2026-09-12" }, + ]), + }); + try { + const { problems } = await checkScreenshots(root); + // Two: the reason is refused, and the picture it would have + // covered is then unaccounted for. A rejected entry must not + // still shelter its picture. + assert.equal(problems.length, 2); + assert.match(problems[0] ?? "", /looks-nicer/u); + assert.match(problems[1] ?? "", /specs-list\.png/u); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("a listing without a date fails", () => { + const { problems } = readBaseline(JSON.stringify([{ path: "docs/images/x.png", reason: "editor-native" }])); + assert.equal(problems.length, 1); + assert.match(problems[0] ?? "", /captured/u); +}); + +test("a listing whose picture is gone fails", async () => { + const root = await repoWith({ + baseline: JSON.stringify([ + { path: "docs/images/extension/deleted.png", reason: "editor-native", captured: "2026-08-22" }, + ]), + }); + try { + const { problems } = await checkScreenshots(root); + assert.equal(problems.length, 1); + assert.match(problems[0] ?? "", /deleted\.png/u); + assert.match(problems[0] ?? "", /no such picture/u); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); + +test("a repository with no pictures at all passes", async () => { + const root = await repoWith({}); + try { + const { problems } = await checkScreenshots(root); + assert.deepEqual(problems, []); + } finally { + await rm(root, { recursive: true, force: true }); + } +}); diff --git a/scripts/screenshot-baseline.json b/scripts/screenshot-baseline.json new file mode 100644 index 00000000..ebd7a507 --- /dev/null +++ b/scripts/screenshot-baseline.json @@ -0,0 +1,47 @@ +[ + { + "path": "docs/images/extension/overview-expanded.png", + "reason": "editor-native", + "captured": "2026-09-06" + }, + { + "path": "docs/images/extension/overview-compact.png", + "reason": "editor-native", + "captured": "2026-09-06" + }, + { + "path": "docs/images/extension/nested-tasks.png", + "reason": "editor-native", + "captured": "2026-08-22" + }, + { + "path": "docs/images/extension/archive-actions.png", + "reason": "editor-native", + "captured": "2026-08-22" + }, + { + "path": "docs/images/extension/archive-tasks.png", + "reason": "editor-native", + "captured": "2026-08-22" + }, + { + "path": "docs/images/extension/specs-editor.png", + "reason": "editor-native", + "captured": "2026-08-22" + }, + { + "path": "docs/images/extension/specs-list.png", + "reason": "editor-native", + "captured": "2026-08-22" + }, + { + "path": "docs/images/extension/template-actions.png", + "reason": "editor-native", + "captured": "2026-08-22" + }, + { + "path": "docs/images/extension/repository-setup.png", + "reason": "editor-native", + "captured": "2026-08-22" + } +]