chore(scripts): check.sh runs the browser suites, and the cycle suite is repeatable (#314)

.agents/skills/process/refactor-protocol/SKILL.md

@@ -29,16 +29,17 @@ All work in the worktree; never the primary checkout. Conventional commits in lo
 - **Every mutation touched must have a refusal-rendering test**: force the refusal, assert the user sees prose (not a raw code, not nothing).
 - E2e fixtures seed all five asset-progress states and at least one batch per batch state when the task touches state-dependent UI.
 - Run the full existing suites (Python + TS) and linters; fix what your change broke, and only that.
-- **Three suites, all of them, before every push** — `scripts/check.sh` runs **no browser suite at all**, so "check.sh green" is not "CI will be green":
+- **Three suites, all of them, before every push — and `bash scripts/check.sh` now runs all three.** It used to run none of the browser ones; #314 made the full run the default and `--fast` the exception, so the rule below is one command rather than three:
 
   ```bash
-  bash scripts/check.sh
-  cd frontend/app && CI=1 npx playwright test
-  cd frontend/app && CI=1 npx playwright test -c playwright.cycle.config.ts
+  bash scripts/check.sh          # everything, including both browser suites
+  bash scripts/check.sh --fast   # inner loop only; prints a banner naming what it skipped
+  bash scripts/check.sh browser  # just the two browser suites
   ```
 
-  The third is the **real-server cycle run**, and it is mandatory for anything touching state, gating, or progress: it was three separate times the *only* suite to catch a regression — a stale job declaration, a label flip standing in for feedback, and a progress counter running backwards. — 2026-08 run, T3/T5/T6
-- **Always `CI=1` for a local Playwright run.** `playwright.config.ts` sets `reuseExistingServer: !CI`, so a stale vite server on :5273 answers instead of your build and produces failures that read as code bugs. — 2026-08 run, T3
+  The script sets `CI=1` for the Playwright steps itself, so that is no longer yours to remember. **`--fast` is never enough before a push.** The real-server cycle run is mandatory for anything touching state, gating, or progress: it was three separate times the *only* suite to catch a regression — a stale job declaration, a label flip standing in for feedback, and a progress counter running backwards. — 2026-08 run, T3/T5/T6; #314
+- **`CI=1` on any Playwright run you invoke by hand.** `playwright.config.ts` sets `reuseExistingServer: !CI`, so a stale vite server on :5273 answers instead of your build and produces failures that read as code bugs. `check.sh` does this for you; `npx playwright test` typed directly does not. — 2026-08 run, T3
+- **To rerun the cycle suite N times, use `--repeat-each=N`** — it costs one build rather than N, because the suite's names are run-scoped since #314. Before that a fixed project name made repeat 2 die on `POST /projects → 409`, and repetition meant N whole invocations at ~90 s of rebuild each.
 - **`git add` new files before trusting any local check run.** Several gates read `git ls-files` — the index, not the working tree — so an untracked new file is invisible to them and passes locally while failing in CI. — 2026-08 run, T4
 - **A test double must not encode invisible-order or frozen-state semantics.** Put defaults in the *unmatched-request fallback* so an explicit stub always wins whichever order it was registered in, and derive stub responses from the state the test walks rather than from frozen literals. Both failure modes make a test assert against the fixture instead of the code, and both are silent. — 2026-08 run, T6/T7/T10
 

CONTRIBUTING.md

@@ -31,9 +31,26 @@ told about them.
 ## Checks that must stay green
 
 **Run them with `bash scripts/check.sh`** (or `pnpm check` — the same script). It is the
-canonical invocation for humans and agents alike: it runs the inner loop below, collects
-*every* failure rather than stopping at the first, and carries `set -euo pipefail`. Take a
-subset with `bash scripts/check.sh python`, `frontend` or `generated`.
+canonical invocation for humans and agents alike: it collects *every* failure rather than
+stopping at the first, carries `set -euo pipefail`, and prints a per-step timing table.
+Take a subset with `bash scripts/check.sh python`, `frontend`, `generated` or `browser`.
+
+**It runs the browser suites, and that is the default.** Until #314 it ran no browser at
+all while calling itself canonical — and during the 2026-08 remediation run the
+real-server cycle suite was three separate times the *only* one to catch a regression
+(#306, #308, #309), one of which shipped on a green run of this script and went red in CI.
+`bash scripts/check.sh --fast` skips them for the inner loop; it says so in a banner rather
+than quietly, because "All checks passed" has always meant "all the checks this invocation
+ran".
+
+The script sets **`CI=1`** for the Playwright steps itself. It is load-bearing:
+`playwright.config.ts` sets `reuseExistingServer: !process.env.CI`, so without it a stale
+vite server left on port 5273 answers instead of the build under test, and the failures
+that follow read as genuine code bugs in unrelated scenarios.
+
+One caveat no exit code will tell you: several gates read `git ls-files`, which is the
+**index** rather than the working tree. A new file you have not `git add`ed is invisible to
+them, so it passes locally and fails in CI. Stage first, then run.
 
 **Never pipe a test runner through `tail` or `head` when the exit code matters.** A
 pipeline's status is the *last* command's, so `uv run pytest -q | tail -20` exits 0 while
@@ -42,9 +59,9 @@ through two task cycles during the #229–#233 run. If you need less output, red
 file and read it (`uv run pytest -q > /tmp/out.log 2>&1; echo $?`), or use the script
 above.
 
-The table below is the full list, and it is wider than the script: the wheel build, the
-30-minute flow, the format smoke tests and everything Playwright are left to CI because
-each costs minutes or needs its own install.
+The table below is the full list, and it is still wider than the script: the wheel build,
+the 30-minute flow, the format smoke tests and the annotator benchmark are left to CI or to
+a deliberate manual run, because each costs minutes or needs its own install.
 
 | Check | Command | In `check.sh` |
 | --- | --- | --- |
@@ -55,8 +72,8 @@ each costs minutes or needs its own install.
 | Frontend build + tests | `pnpm -r build && pnpm test` | `frontend` |
 | Frontend lint | `pnpm -r lint` — **after** a build: `frontend/app` resolves `@visionset/annotator` through its `dist/`, so its typecheck has no declarations until the engine is built | `frontend` |
 | Annotator headless boundary | `pnpm --filter @visionset/annotator lint` | part of `frontend` (`pnpm -r lint`) |
-| Annotator end-to-end (chromium) | `pnpm --filter @visionset/app e2e` (needs `playwright install chromium` once) | — CI |
-| Browser cycle (chromium) | `pnpm --filter @visionset/app cycle` — the whole product against a real `visionset ui`; needs `uv sync` and `playwright install chromium` | — CI |
+| Annotator end-to-end (chromium) | `pnpm --filter @visionset/app e2e` (needs `playwright install chromium` once) | `browser` |
+| Browser cycle (chromium) | `pnpm --filter @visionset/app cycle` — the whole product against a real `visionset ui`; needs `uv sync` and `playwright install chromium`. Repeatable in one workspace: `--repeat-each=N` costs one build rather than N | `browser` |
 | Annotator benchmark (manual) | `pnpm --filter @visionset/app bench` — frame times, recorded not gated | — manual |
 | Browser client | part of `pnpm test` — `ui-core`'s `data/` suite drives the 401 flow, the token form and the error envelope with a stubbed `fetch`, no server | part of `frontend` |
 | Design tokens | part of `pnpm test` — `tests/scripts/design_tokens.test.mjs` refuses a colour inside a class name, and `ui-core`'s `tokens.test.ts` gates the stylesheet against its TypeScript mirror | part of `frontend` |

frontend/app/cycle/cycle.spec.ts

@@ -34,7 +34,7 @@
  * and the browser saves it — which is exactly the part real exporters will inherit.
  */
 
-import { expect, test, type Download, type Page } from "@playwright/test";
+import { expect, test, type Download, type Page, type TestInfo } from "@playwright/test";
 import { readFileSync, readdirSync } from "node:fs";
 import path from "node:path";
 
@@ -53,13 +53,41 @@ function images(): string[] {
     .map((name) => path.join(dir, name));
 }
 
-/** A name nothing else in the workspace will collide with. */
-const PROJECT = "browser-cycle";
 const TAG = "v1";
 
-test("the whole cycle, from opening the app to a downloaded export", async ({ page }) => {
+/**
+ * A project name nothing else in the workspace will collide with — **including
+ * this same spec on another repetition**.
+ *
+ * A project name is unique per workspace, case-insensitively, and the workspace
+ * outlives a repetition: `scripts/cycle_server.sh` rebuilds it once per *server
+ * start*, and `--repeat-each` reuses that one server. So the fixed literal that
+ * used to live here made the flag useless — repeat 2 died on
+ * `POST /projects → 409`, a wall standing in front of everything the suite is
+ * about, and the only way to run the cycle twice was two whole invocations at
+ * about ninety seconds of rebuild each.
+ *
+ * `repeatEachIndex` is the whole of the uniqueness needed. The workspace really
+ * is fresh per invocation (the script `rm -rf`s it before `init`), and
+ * `workers: 1` means two repetitions never overlap. The suffix is unconditional
+ * rather than omitted on the first, so every run's names have one shape and a
+ * failure message reads the same way whether or not somebody passed the flag.
+ *
+ * The project is the only name that has to move, and that is worth stating so
+ * the next collision is looked for rather than assumed: a release tag is unique
+ * per dataset, a batch name is not unique at all, and a source's idempotency key
+ * `(project, kind, path, fps)` leads with the project. All three are already
+ * scoped by a project that is new.
+ */
+function projectFor(info: TestInfo): string {
+  return `browser-cycle-${info.repeatEachIndex}`;
+}
+
+test("the whole cycle, from opening the app to a downloaded export", async ({ page }, info) => {
   test.slow();
 
+  const PROJECT = projectFor(info);
+
   // #161's first acceptance criterion, collected across the whole walk rather than
   // asserted at one moment: a clean load should produce **zero** console errors and
   // no failed request, and the only one there had been was the browser asking for
@@ -238,7 +266,7 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
     // Navigated rather than `goBack()`: history depth is an implementation detail
     // of how the previous steps got here, and a cycle this long should not depend
     // on it.
-    await openProject(page, "batches");
+    await openProject(page, PROJECT, "batches");
     await expect(page.getByTestId("batches-table")).toBeVisible();
     await expect(page.getByTestId("batch-cycle-batch")).toContainText("pending approval");
 
@@ -259,7 +287,7 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
     // read out of the API — which is exactly what this spec used to do, and what
     // made a defect that blocked the whole product invisible to a green suite.
     // Nothing here types a URL.
-    await openProject(page, "batches");
+    await openProject(page, PROJECT, "batches");
     await page.getByTestId("open-batch-cycle-batch").click();
     await expect(page.getByTestId("gallery")).toBeVisible();
 
@@ -368,7 +396,7 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
   });
 
   await test.step("complete the batch", async () => {
-    await openProject(page, "batches");
+    await openProject(page, PROJECT, "batches");
     await expect(page.getByTestId("batches-table")).toBeVisible();
     await page.getByTestId("complete-cycle-batch").click();
     await expect(page.getByTestId("state-cycle-batch")).toHaveText("completed", {
@@ -437,7 +465,7 @@ test("the whole cycle, from opening the app to a downloaded export", async ({ pa
 
     // And the parent has not moved — which is the whole point of correcting
     // forward instead of reopening.
-    await openProject(page, "batches");
+    await openProject(page, PROJECT, "batches");
     await expect(page.getByTestId("state-cycle-batch")).toHaveText("completed");
     await expect(page.getByTestId("promoted-count-cycle-batch")).toHaveText(
       /3 of 3 in the dataset/,
@@ -549,11 +577,12 @@ async function columnsOf(page: Page): Promise<{ rendered: number; expected: numb
  */
 async function openProject(
   page: Page,
+  project: string,
   tab: "schema" | "batches" | "dataset",
 ): Promise<void> {
   await page.getByTestId("rail-projects").click();
-  await expect(page.getByTestId(`open-${PROJECT}`)).toBeVisible();
-  await page.getByTestId(`open-${PROJECT}`).click();
+  await expect(page.getByTestId(`open-${project}`)).toBeVisible();
+  await page.getByTestId(`open-${project}`).click();
   await expect(page.getByTestId("project-screen")).toBeVisible();
   if (tab !== "schema") await page.getByTestId(`tab-${tab}`).click();
 }