diff --git a/.github/scripts/capture-shots.sh b/.github/scripts/capture-shots.sh new file mode 100755 index 00000000..4e2496bb --- /dev/null +++ b/.github/scripts/capture-shots.sh @@ -0,0 +1,92 @@ +#!/usr/bin/env bash +# Serves a built site on :4126, photographs every route in routes.json at both +# viewports, and moves the captures to $2. +# +# capture-shots.sh +# +# Both sides of the visual job are captured by this script, from the head's +# checkout, after both builds have been produced. That ordering is deliberate: +# the merge base predates the harness that photographs it, so checking it out +# and running its own `scripts/verify` would either fail outright or compare two +# captures taken by two different procedures. Only the built output should +# differ between the two sides. +# +# Reusing :4126 across two builds is the one reliable way to produce a +# confident, meaningless green: if the first server survives, the second sweep +# photographs the first build and parity passes because it compared a build to +# itself. That is not hypothetical — it happened on the first CI run of this +# job. `npx` spawns http-server as a child, so killing the npx wrapper left the +# real server holding the port, the second `npx http-server` died of +# EADDRINUSE in the background where nothing was watching, and the sweep +# cheerfully re-photographed the merge base and called it identical. +# +# Two guards, because teardown is a thing that can fail in ways nobody +# anticipated: +# +# 1. The whole process group is killed and the port is confirmed free, both +# before starting and after finishing. +# 2. The served bytes are checked against the bytes on disk before a single +# screenshot is taken. Whatever else goes wrong, this sweep cannot +# photograph a build other than the one it was handed. +set -euo pipefail + +dist="$1" +destination="$2" +root="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +cd "$root" + +test -d "$dist" || { + echo "no build at $dist" >&2 + exit 1 +} + +port_free() { ! curl -fsS -o /dev/null --max-time 2 "http://127.0.0.1:4126/" 2>/dev/null; } + +await_port_free() { + for _ in $(seq 1 20); do + port_free && return 0 + sleep 0.5 + done + return 1 +} + +if ! port_free; then + echo "something is already serving :4126; refusing to photograph it" >&2 + exit 1 +fi + +rm -rf scripts/verify/out + +# `setsid` puts the server and every child npx spawns into one process group, so +# teardown can take the group down rather than only the wrapper. +setsid npx --yes http-server "$dist" -p 4126 --silent & +server=$! +cleanup() { + kill -TERM "-$server" 2>/dev/null || kill -TERM "$server" 2>/dev/null || true + wait "$server" 2>/dev/null || true + await_port_free || { + echo ":4126 is still held after teardown; the next sweep would photograph this build" >&2 + exit 1 + } +} +trap cleanup EXIT + +for _ in $(seq 1 60); do + curl -fsS -o /dev/null "http://127.0.0.1:4126/" && break + sleep 1 +done + +# The identity check. If this passes, the sweep is photographing $dist and +# nothing else — no surviving server, no stale build, no silent green. +served=$(curl -fsS "http://127.0.0.1:4126/" | shasum | cut -d' ' -f1) +ondisk=$(shasum < "$dist/index.html" | cut -d' ' -f1) +if [ "$served" != "$ondisk" ]; then + echo "‽ :4126 is not serving $dist (served $served, on disk $ondisk)" >&2 + exit 1 +fi + +npm run verify:shots:all > /dev/null + +mkdir -p "$(dirname "$destination")" +rm -rf "$destination" +mv scripts/verify/out "$destination" diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 43324c55..05177d59 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -2,12 +2,20 @@ name: CI on: pull_request: + # `labeled`/`unlabeled` are here for the visual job: its enforcement is + # chosen by the PR's labels, so applying `visual-change` or `dependencies` + # has to be able to re-decide a run that already happened. + types: [opened, synchronize, reopened, labeled, unlabeled] workflow_dispatch: concurrency: group: ci-${{ github.ref }} cancel-in-progress: true +env: + # Pinned so the gates are always read by the same browser. Bump deliberately. + PLAYWRIGHT_VERSION: 1.62.1 + jobs: lint: runs-on: ubuntu-latest @@ -43,3 +51,175 @@ jobs: # receiver URL never reaches fork-triggered runs. The deploy workflow # injects the secret for the production build. - run: npm run build + + # Screenshots cannot see a transition that plays in the wrong place and then + # corrects: the landed page is correct, only the frames in between are not. + # This gate drives real client-side navigations and asserts the arrival is a + # single motion. It is required, and it is cheap enough to stay that way. + motion: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version-file: .tool-versions + cache: npm + - run: npm ci + - name: Install Playwright + run: | + npm install --no-save --no-audit --no-fund "playwright@$PLAYWRIGHT_VERSION" + npx playwright install --with-deps chromium + - run: npm run build + - name: Serve the build + run: | + nohup npx --yes http-server dist -p 4126 --silent > /dev/null 2>&1 & + for _ in $(seq 1 60); do + curl -fsS -o /dev/null http://127.0.0.1:4126/ && exit 0 + sleep 1 + done + echo "the built site never came up on :4126" >&2 + exit 1 + - run: npm run verify:motion:all + + # Pixel parity against the PR's merge base. No baselines are committed — the + # job builds and photographs both sides itself, so it cannot go stale and + # cannot be blessed by hand. What it does with the answer depends on the + # labels; see PARITY_MODE below. + visual: + runs-on: ubuntu-latest + if: >- + github.event_name == 'pull_request' && + !contains(github.event.pull_request.labels.*.name, 'visual-change') + permissions: + contents: read + pull-requests: write + env: + # `dependencies` is what Dependabot applies: a bump that moves rendering + # must not merge silently, so it is held to zero differing pixels. + # Everything else measures and reports without failing. + PARITY_MODE: >- + ${{ contains(github.event.pull_request.labels.*.name, 'dependencies') && 'enforce' || 'report' }} + COMMENT_MARKER: '' + steps: + - uses: actions/checkout@v4 + with: + ref: ${{ github.event.pull_request.head.sha }} + fetch-depth: 0 + - name: Resolve the merge base and the scope + id: scope + run: | + git fetch --no-tags --prune origin "$GITHUB_BASE_REF" + base=$(git merge-base "origin/$GITHUB_BASE_REF" HEAD) + echo "base=$base" >> "$GITHUB_OUTPUT" + echo "Merge base: $base" + changed=$(git diff --name-only "$base" HEAD -- src public package-lock.json) + if [ -z "$changed" ]; then + echo "Nothing rendered changed; skipping the visual sweep." + echo "run=false" >> "$GITHUB_OUTPUT" + else + echo "$changed" + echo "run=true" >> "$GITHUB_OUTPUT" + fi + - uses: actions/setup-node@v4 + if: steps.scope.outputs.run == 'true' + with: + node-version-file: .tool-versions + cache: npm + - name: Install ImageMagick and Playwright + if: steps.scope.outputs.run == 'true' + run: | + sudo apt-get update -qq + sudo apt-get install -y -qq imagemagick + # Playwright lives outside the checkout so it survives the two + # `npm ci` runs this job performs (merge base, then head), and the + # gates are pointed at it by PLAYWRIGHT_MODULE. `runner.temp` is not + # a context job-level `env:` can read, so it is exported here. + mkdir -p "$RUNNER_TEMP/playwright" + cd "$RUNNER_TEMP/playwright" + npm init -y >/dev/null + npm install --no-audit --no-fund "playwright@$PLAYWRIGHT_VERSION" + npx playwright install --with-deps chromium + echo "PLAYWRIGHT_MODULE=$RUNNER_TEMP/playwright/node_modules/playwright/index.js" >> "$GITHUB_ENV" + - name: Build the merge base + if: steps.scope.outputs.run == 'true' + run: | + git checkout --detach "${{ steps.scope.outputs.base }}" + npm ci + npm run build + mv dist "$RUNNER_TEMP/dist-base" + - name: Build the head + if: steps.scope.outputs.run == 'true' + run: | + git checkout --detach "${{ github.event.pull_request.head.sha }}" + npm ci + npm run build + mv dist "$RUNNER_TEMP/dist-head" + # Both sides are photographed from the head's checkout, after both builds + # exist. The merge base predates the harness that photographs it — running + # its own `scripts/verify` would either fail outright or compare two + # captures taken by two different procedures. One camera, two subjects. + - name: Capture both builds + if: steps.scope.outputs.run == 'true' + run: | + .github/scripts/capture-shots.sh "$RUNNER_TEMP/dist-base" "$RUNNER_TEMP/shots/base" + .github/scripts/capture-shots.sh "$RUNNER_TEMP/dist-head" "$RUNNER_TEMP/shots/head" + - name: Compare + if: steps.scope.outputs.run == 'true' + id: compare + run: | + mkdir -p "$RUNNER_TEMP/parity" + for viewport in 1440 420; do + # verify-parity exits non-zero on any difference; the verdict is + # made from the reports, not from this exit code. + npm run --silent verify:parity -- \ + "$RUNNER_TEMP/shots/base/$viewport" "$RUNNER_TEMP/shots/head/$viewport" \ + --out "$RUNNER_TEMP/parity/diff-$viewport" \ + > "$RUNNER_TEMP/parity/report-$viewport.json" || true + done + - name: Summarise + if: steps.scope.outputs.run == 'true' + id: summary + run: | + node scripts/verify/parity-summary.mjs \ + --mode "$PARITY_MODE" \ + --marker "$COMMENT_MARKER" \ + --out "$RUNNER_TEMP/parity/comment.md" \ + --verdict "$RUNNER_TEMP/parity/verdict.txt" \ + "1440x1000=$RUNNER_TEMP/parity/report-1440.json" \ + "420x900=$RUNNER_TEMP/parity/report-420.json" + echo "verdict=$(cat "$RUNNER_TEMP/parity/verdict.txt")" >> "$GITHUB_OUTPUT" + - name: Post or update the PR comment + # Fork PRs get a read-only token, so a comment is best-effort; the + # verdict below is not. + if: steps.scope.outputs.run == 'true' && !github.event.pull_request.head.repo.fork + continue-on-error: true + env: + GH_TOKEN: ${{ secrets.GITHUB_TOKEN }} + PR: ${{ github.event.pull_request.number }} + run: | + existing=$(gh api "repos/$GITHUB_REPOSITORY/issues/$PR/comments" --paginate \ + --jq "[.[] | select(.body | startswith(\"$COMMENT_MARKER\"))] | last | .id // empty") + if [ -n "$existing" ]; then + gh api -X PATCH "repos/$GITHUB_REPOSITORY/issues/comments/$existing" \ + -F body=@"$RUNNER_TEMP/parity/comment.md" >/dev/null + else + gh api -X POST "repos/$GITHUB_REPOSITORY/issues/$PR/comments" \ + -F body=@"$RUNNER_TEMP/parity/comment.md" >/dev/null + fi + - name: Upload the diffs + if: steps.scope.outputs.run == 'true' && steps.summary.outputs.verdict == 'moved' + uses: actions/upload-artifact@v4 + with: + name: visual-parity-diffs + path: ${{ runner.temp }}/parity + if-no-files-found: ignore + - name: Verdict + if: steps.scope.outputs.run == 'true' + run: | + cat "$RUNNER_TEMP/parity/comment.md" >> "$GITHUB_STEP_SUMMARY" + verdict='${{ steps.summary.outputs.verdict }}' + echo "Verdict: $verdict (mode: $PARITY_MODE)" + if [ "$verdict" = "moved" ] && [ "$PARITY_MODE" = "enforce" ]; then + echo "Labelled 'dependencies' and pixels moved against the merge base." >&2 + exit 1 + fi diff --git a/AGENTS.md b/AGENTS.md index 167ddaff..0a3c5536 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -9,10 +9,18 @@ All changes land on `master` through a pull request; direct pushes are blocked b 1. Branch from `master` (agents work in their own git worktree so parallel tracks never share a dirty checkout). 2. Commit with comprehensive messages. Do not append session URLs to commit messages or PR bodies. 3. Push the branch and open a PR with `gh pr create`. -4. CI (`.github/workflows/ci.yml`) runs three required checks: `lint` (ESLint and Prettier), `typecheck` (the `astro check` ratchet), and `build` (the production build, in demo mode without `FORM_ENDPOINT`). +4. CI (`.github/workflows/ci.yml`) runs four required checks — `lint` (ESLint and Prettier), `typecheck` (the `astro check` ratchet), `build` (the production build, in demo mode without `FORM_ENDPOINT`) and `motion` (the view-transition settle gate) — plus `visual`, a merge-base pixel-parity sweep whose enforcement depends on the PR's labels. 5. Merge once CI is green. The merge landing on `master` triggers the production deploy. -CI does not yet run the Playwright DOM gate, so `npm run verify:dom -- --viewport 1440x1000` (and `420x900`) for affected routes remains a local pre-PR responsibility, as does the rest of the definition of done below. +The `visual` job builds and photographs both the merge base and the head, so there are no committed baselines to go stale. It skips entirely when nothing under `src/`, `public/` or `package-lock.json` changed, and otherwise: + +| PR label | Behaviour | +| --------------- | ----------------------------------------------------------------------------------------------------------- | +| `dependencies` | Zero differing pixels required; the job fails otherwise. A bump that moves rendering cannot merge silently. | +| `visual-change` | Skipped. The author is declaring that pixels are meant to move. | +| anything else | Measured and posted as a PR comment (routes changed, pixel counts, peak channel delta). Never fails. | + +CI still does not run the Playwright DOM gate, so `npm run verify:dom -- --viewport 1440x1000` (and `420x900`) for affected routes remains a local pre-PR responsibility, as does the rest of the definition of done below. ## Companion documents diff --git a/package.json b/package.json index a00ba7c4..2456beb2 100644 --- a/package.json +++ b/package.json @@ -16,6 +16,8 @@ "verify:dom:all": "sh -c 'node scripts/verify/verify-dom.mjs --viewport 1440x1000 \"$@\" && node scripts/verify/verify-dom.mjs --viewport 420x900 \"$@\"' --", "verify:shots": "node scripts/verify/verify-shots.mjs", "verify:shots:all": "sh -c 'node scripts/verify/verify-shots.mjs --viewport 1440x1000 \"$@\" && node scripts/verify/verify-shots.mjs --viewport 420x900 \"$@\"' --", + "verify:motion": "node scripts/verify/verify-motion.mjs", + "verify:motion:all": "sh -c 'node scripts/verify/verify-motion.mjs --viewport 1440x1000 \"$@\" && node scripts/verify/verify-motion.mjs --viewport 420x900 \"$@\"' --", "verify:parity": "node scripts/verify/verify-parity.mjs", "verify:typewriter": "node scripts/verify/verify-typewriter.mjs", "postbuild": "pagefind --site dist", diff --git a/scripts/verify/README.md b/scripts/verify/README.md index 2975574f..ed08df4c 100644 --- a/scripts/verify/README.md +++ b/scripts/verify/README.md @@ -6,8 +6,11 @@ These scripts are the reviewed development harness required by ADR-008. Start a npm run verify:dom:all npm run verify:shots:all npm run verify:typewriter +npm run verify:motion:all ``` +Playwright is not a repository dependency — it is a browser download nobody building the site should pay for. `playwright.mjs` resolves it at run time from `node_modules` if a caller installed it there (CI does, pinned), otherwise from `PLAYWRIGHT_MODULE` or the local npx cache the harness has always been driven from. + Pass routes after `--` to check a safe subset. Set `VERIFY_BASE_URL` only when deliberately verifying another local origin. `--viewport WIDTH` or `--viewport WIDTHxHEIGHT` changes either responsive gate; `VERIFY_VIEWPORT` is the environment equivalent. Screenshot files and their manifest go to viewport-specific folders under the git-ignored `scripts/verify/out/`. ## Themes @@ -37,6 +40,18 @@ readable whatever it scored in dark. Run the dark gate before the light gate; a light run with no recorded baseline fails rather than passing vacuously. +## Motion + +`verify-motion` is the one gate that watches the journey rather than the destination. A real bug survived on `master` through every branch and was found by accident: navigating into a principle played its view transition 26px too low and snapped up on completion. Screenshots could not see it — the page was never in the wrong place, the _snapshot_ was. `.principle-prose` is both a `[data-reveal]` element and the named `principle-page` transition group, and the reveal settle was deferred by `requestAnimationFrame`, which runs after the browser photographs `::view-transition-new`. Fixed in `c38ef92` by settling in a microtask instead. + +The gate clicks a real link (never `goto`), then samples every animation frame for two seconds after the click: the scroll offset, and each tracked element's `getBoundingClientRect()`. The primer that leaks into the snapshot is a `translateY(26px)` on the real element, and `getBoundingClientRect` sees transforms, so the bad frame is measurable even though the pseudo-element is not. + +**The invariant is that arrival happens in a single motion.** From `astro:after-swap` onwards, every tracked element's document-space position — `scrollY + rect.top`, which cancels the router's own scroll — must already be its resting position. A trajectory that visits a position it later corrects away from by more than 2px is two motions. The resting value is the median of the window's last samples, and the assertion is stated against it rather than against "the first plateau" deliberately: a bad frame can be a single sample, too short to form a plateau of its own, and a plateau-relative rule would score it as the settle it corrects into. The 2px tolerance absorbs sub-pixel settle and layout rounding; it is not somewhere to hide a jump. + +`reducedMotion` is left at `no-preference` on purpose — under `reduce`, `Reveal` marks everything revealed at construction and the gate would assert nothing. + +Cases cover `/principles/` into a principle, principle prev/next in both directions, `/newsletter/` into an issue, and newsletter prev/next, at 1440x1000 and 420x900. Reverting `c38ef92` makes every principle case and both newsletter step cases fail with a measured 26px correction at post-swap frame 0, naming the route; the newsletter index card at 420 stays green because at that width the revealed elements are below the fold, so nothing on screen is photographed wrong. + ## Parity Compare a fresh capture with a stored baseline mechanically: @@ -95,3 +110,9 @@ Every entry is echoed with its reason into the JSON report and cropped into `' --out FILE --verdict FILE