From ae76baa7bc76fe82345df9782757ac0f8d5954b6 Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 19:10:57 -0700 Subject: [PATCH 1/2] test(app): the cycle suite's project name is run-scoped, so --repeat-each works `scripts/cycle_server.sh` rebuilds the workspace once per *server start* and `--repeat-each` reuses that one server, so a project created from a fixed literal made the flag useless: repeat 2 onward died on `POST /projects -> 409`, a wall standing in front of everything the suite is about. Repetition meant N whole invocations at about ninety seconds of rebuild each. Unique names rather than teardown, because teardown that has to survive a mid-run crash is a second bug surface, and there is nothing to tear down when nothing collides. The project is the only name that had to move, and the spec now says so, 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. Verified rather than reasoned about: --repeat-each=3 gives three `POST /projects -> 201`, zero 409 anywhere in the run, and the only 4xx at all is the expected schema-less-project 404, once per repetition. Refs #314 --- frontend/app/cycle/cycle.spec.ts | 49 +++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 10 deletions(-) diff --git a/frontend/app/cycle/cycle.spec.ts b/frontend/app/cycle/cycle.spec.ts index f9c30d19..7d8fca8e 100644 --- a/frontend/app/cycle/cycle.spec.ts +++ b/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 { 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(); } From 64bee7ed83ea8e6ca72262449da5264123d49d5d Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 19:11:11 -0700 Subject: [PATCH 2/2] chore(scripts): check.sh runs the browser suites, with --fast and timings check.sh ran no browser suite while calling itself the canonical "before you say it works" invocation. Two of the three suites were invisible to it, and the real-server cycle run was three separate times the *only* one to catch a regression during the 2026-08 remediation run (#306, #308, #309) -- one of which shipped on a green run of this script and went red in CI. The full run is the default and --fast is the exception. That inversion is the point: the previous default was silently the fast one. - `browser` is addressable as a group and is a complete run on its own, because each Playwright config's webServer.command builds the engine and the design system first. - CI=1 is set by the script, for the Playwright steps, from frontend/app, in a subshell so the cd cannot leak into a later group. It is load-bearing: reuseExistingServer: !CI means a stale vite server on :5273 answers instead of the build under test. The lesson lived in a skill and in memories; it lives in the tooling now. - Skipping prints a banner, *after* the verdict -- it exists to qualify "All checks passed", so it has to be the thing still on screen once that line has scrolled off. It prints whenever the browser suites did not run, not only under --fast: a partial run is the same lie however it was asked for. - Per-step timings with a total, so the cost is visible and a step that quietly doubles gets noticed. - --fast together with an explicit `browser` group is refused rather than resolved. Either answer would guess at which half of a contradictory command line was meant. The timing table is length-checked before expanding, and the reason is written down: macOS still ships bash 3.2, where "${arr[@]}" on an empty array is an unbound variable under set -u and kills the script. The neighbouring `failed` block had always been written that way and never said why. No CI or ruleset change. The two jobs already run these suites and stay separate -- each is a required check with its own cache and report artifact -- and no job name moved. Refs #314 --- .../skills/process/refactor-protocol/SKILL.md | 13 +- CONTRIBUTING.md | 33 +++- scripts/check.sh | 145 +++++++++++++++++- 3 files changed, 169 insertions(+), 22 deletions(-) diff --git a/.agents/skills/process/refactor-protocol/SKILL.md b/.agents/skills/process/refactor-protocol/SKILL.md index 70989bef..e417f334 100644 --- a/.agents/skills/process/refactor-protocol/SKILL.md +++ b/.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 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index db7e848d..e15b771a 100644 --- a/CONTRIBUTING.md +++ b/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` | diff --git a/scripts/check.sh b/scripts/check.sh index b5100132..2165bfbb 100755 --- a/scripts/check.sh +++ b/scripts/check.sh @@ -15,15 +15,53 @@ # matters". # # Usage: -# bash scripts/check.sh # python + frontend + generated -# bash scripts/check.sh python # one group +# bash scripts/check.sh # everything: python, frontend, +# # generated, browser +# bash scripts/check.sh --fast # the same minus the browser suites +# bash scripts/check.sh browser # one group # bash scripts/check.sh python frontend # several # pnpm check # the same thing, from the other half # +# **The three suites, because there are three and two of them used to be +# invisible here** (#314). Until that issue this script ran no browser at all, +# while calling itself the canonical "before you say it works" invocation: +# +# python | frontend | generated pytest, vitest, ruff, mypy, import-linter, +# eslint, and the four drift gates +# browser, part 1 frontend/app's e2e — the annotator's +# scenarios and the app's, all stubbed +# (CI job: `annotator e2e (chromium)`) +# browser, part 2 the whole cycle against a **real server and +# a real kernel**, from a pasted token to a +# downloaded export +# (CI job: `browser cycle (chromium)`) +# +# That second browser suite is not a luxury. During the 2026-08 remediation run +# it was three separate times the *only* suite to catch a regression: a job's +# stale capability declaration (#306), a promote button whose only feedback was +# its own label (#308), and a progress counter that ran backwards when a frame +# was accepted (#309). One of those shipped on a green run of this script and +# went red in CI. +# +# **`CI=1` is set here, for the Playwright steps, and 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 — which produces failures in unrelated scenarios that read as genuine +# code bugs. The lesson lived in a skill and in three people's memories; it +# lives in the script now. +# +# **Caveat that 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 here and fails in CI. Stage +# first, then run. +# # Groups deliberately *not* here, because each costs minutes or needs an # install, and CI is where they belong: the wheel build, the 30-minute flow, the -# format smoke tests (ultralytics brings torch), Playwright's e2e/cycle/bench. -# `CONTRIBUTING.md`'s table stays the full list; this is the inner loop. +# format smoke tests (ultralytics brings torch), and Playwright's bench config — +# `annotator bench (chromium, manual)` is `workflow_dispatch`-only and must stay +# out of any default, here and in the branch ruleset alike. +# `CONTRIBUTING.md`'s table stays the full list; this is everything a pull +# request's own required checks will run. set -euo pipefail root="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" @@ -35,6 +73,11 @@ cd "$root" # it twice is two. `|| status=$?` is what disarms errexit for the step itself — # the pipefail above still applies inside whatever the step runs. declare -a failed=() +# One ` ` line per step, printed as a table at the end. The cost +# of the full gate is now minutes rather than seconds, so it is worth seeing +# where they go — both to choose `--fast` knowingly and to notice the day a step +# quietly doubles. +declare -a timings=() step() { local name="$1" @@ -42,8 +85,10 @@ step() { echo echo "==> $name" echo " \$ $*" + local start=$SECONDS local status=0 "$@" || status=$? + timings+=("$(printf '%5ds %s' "$((SECONDS - start))" "$name")") if [[ $status -ne 0 ]]; then echo " FAILED ($name, exit $status)" >&2 failed+=("$name") @@ -102,9 +147,54 @@ run_generated() { # `tests/server/test_wire_fixtures.py`, so the `python` group already runs it. } -groups=("$@") +# Both suites are invoked from `frontend/app`, in a subshell so the `cd` cannot +# leak into a later group, and both build what they need themselves: each +# config's `webServer.command` compiles `@visionset/annotator` and +# `@visionset/ui-core` first, because `frontend/app` resolves them through their +# `dist/` and an unbuilt change is invisible in a browser rather than a compile +# error. So `check.sh browser` on its own is a complete run, not a half of one. +# +# No `require_playwright_browsers` to match `require_node_modules`: Playwright's +# own error already names `npx playwright install` as the remedy, and a check +# that restates a message which is already good is a second place to keep +# current. +browser_e2e() { + ( cd "$root/frontend/app" && CI=1 npx playwright test ) +} + +browser_cycle() { + ( cd "$root/frontend/app" && CI=1 npx playwright test -c playwright.cycle.config.ts ) +} + +run_browser() { + require_node_modules + step "annotator + app e2e (chromium)" browser_e2e + step "browser cycle, real server (chromium)" browser_cycle +} + +declare -a groups=() +fast=0 +for arg in "$@"; do + case "$arg" in + --fast) fast=1 ;; + -*) + echo "error: unknown flag '$arg' (want: --fast)" >&2 + exit 2 + ;; + *) groups+=("$arg") ;; + esac +done + +# The full run is the default and `--fast` is the exception, which is the whole +# point of #314: the previous default was silently the fast one. if [[ ${#groups[@]} -eq 0 ]]; then groups=(python frontend generated) + [[ $fast -eq 1 ]] || groups+=(browser) +elif [[ $fast -eq 1 && " ${groups[*]} " == *" browser "* ]]; then + # Refused rather than resolved. Either answer would be a guess about which + # half of a contradictory command line the caller meant. + echo "error: --fast and an explicit 'browser' group contradict each other" >&2 + exit 2 fi for group in "${groups[@]}"; do @@ -112,16 +202,55 @@ for group in "${groups[@]}"; do python) run_python ;; frontend) run_frontend ;; generated) run_generated ;; + browser) run_browser ;; *) - echo "error: unknown group '$group' (want: python, frontend, generated)" >&2 + echo "error: unknown group '$group' (want: python, frontend, generated, browser)" >&2 exit 2 ;; esac done echo +echo "Timing" +# Length-checked before expanding, which looks like belt and braces and is not: +# macOS still ships bash **3.2**, where `"${arr[@]}"` on an empty array is an +# unbound variable under `set -u` and kills the script. The `failed` block below +# has always been written this way for the same reason. +if [[ ${#timings[@]} -gt 0 ]]; then + for line in "${timings[@]}"; do + echo " $line" + done +fi +printf ' %5ds total\n' "$SECONDS" + +echo +verdict=0 if [[ ${#failed[@]} -gt 0 ]]; then echo "FAILED: ${failed[*]}" >&2 - exit 1 + verdict=1 +else + echo "All checks passed." +fi + +# Printed **after** the verdict, and that is the placement rather than an +# accident: the banner exists to qualify "All checks passed", so it has to be +# the thing still on screen once that line has scrolled into the backlog. +# Printed whenever the browser suites did not run, not only under `--fast` — a +# partial run is the same lie however it was asked for. ASCII rather than box +# drawing, so it survives every terminal it lands in. +if [[ " ${groups[*]} " != *" browser "* ]]; then + echo + echo "=============================================================================" >&2 + echo " !! THE BROWSER SUITES DID NOT RUN -- this is not what CI runs !!" >&2 + echo "=============================================================================" >&2 + echo " skipped: annotator + app e2e CI job: annotator e2e (chromium)" >&2 + echo " browser cycle, real server CI job: browser cycle (chromium)" >&2 + echo "" >&2 + echo " The real-server cycle run was three separate times the ONLY suite to" >&2 + echo " catch a regression during the 2026-08 remediation run (#306, #308, #309)." >&2 + echo "" >&2 + echo " Run them: bash scripts/check.sh browser" >&2 + echo "=============================================================================" >&2 fi -echo "All checks passed." + +exit "$verdict"