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/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(); } 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"