From 39f3e3affe811b291f7785c478c55990c88416d2 Mon Sep 17 00:00:00 2001 From: Randall Stillwell Date: Tue, 26 May 2026 22:48:56 -0500 Subject: [PATCH] ci(workflows): exclude pages/api/** from visual-diff path filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `Screenshot diff` workflow's `paths:` filter included `pages/**` which matches `pages/api/**` too, so API-only PRs triggered the visual-diff workflow even though they can't possibly move a single rendered pixel. PR #19 (cors-tighten) and PR #20 (add-rate-limiting) both empirically hit this — each was an API-only sweep, and each burned ~55s of CI runtime on a `Screenshot diff` job that `continue-on-error: true` then swallowed. Documented as a queued follow-up in `.convoys/ship-readiness.md` § Queued convoys, ratified for fix in this convoy. The fix is a single negated-glob entry inserted immediately after `pages/**` in the `paths:` list. GitHub Actions evaluates `paths:` with minimatch and supports `!`-prefixed exclusions per the published path-filter cheatsheet, but the order matters: a `!pattern` only takes effect if it appears AFTER an include that already matched the path. Keeping `!pages/api/**` second in the list (right after `pages/**`, before all the other includes) is the canonical shape. All five existing entries are preserved verbatim; only the one exclusion entry plus an inline comment explaining the ordering rule and the empirical motivation is added. `preview-smoke.yml` is intentionally untouched — verified its `on:` block has no `paths:` filter at all (it triggers on every PR targeting main, with skip-via-PR-body-directive in the gate job), so there's no false-positive shape to fix there. Smoke SHOULD run on every PR including API-only ones because changes to `pages/api/**` can break the home redirect + sign-in + `/api/health` endpoints the smoke spec exercises. Co-authored-by: Cursor --- .convoys/tighten-visual-diff-path-filter.md | 270 ++++++++++++++++++++ .github/workflows/visual-diff.yml | 9 + 2 files changed, 279 insertions(+) create mode 100644 .convoys/tighten-visual-diff-path-filter.md diff --git a/.convoys/tighten-visual-diff-path-filter.md b/.convoys/tighten-visual-diff-path-filter.md new file mode 100644 index 0000000..7c0796e --- /dev/null +++ b/.convoys/tighten-visual-diff-path-filter.md @@ -0,0 +1,270 @@ +# tighten-visual-diff-path-filter (P3 polish — single-file YAML tweak) + +**Status:** OPEN 2026-05-26 (this PR) +**Priority:** P3 polish (CI-cost / signal-noise; not a security or +correctness issue — `Screenshot diff` already swallows its own +"snapshot doesn't exist" failure via `continue-on-error: true`, so +the false-trigger is wasted runtime + a slightly chatty checks tab, +nothing more) +**Convoy owner:** parent (no architect — single-file workflow YAML +tweak following published GitHub Actions path-filter semantics) +**Opened:** 2026-05-26 +**Branch:** `convoy/tighten-visual-diff-path-filter` + +## Background + +`.github/workflows/visual-diff.yml` is supposed to fire only on UI +changes. Its current `paths:` filter: + +```yaml +paths: + - 'pages/**' + - 'components/**' + - 'styles/**' + - 'tailwind.config.js' + - 'postcss.config.js' +``` + +But `pages/**` matches `pages/api/**` too, and the `tcg-vault` repo's +API surface lives entirely under `pages/api/` (Next.js Pages router). +That means **API-only PRs trigger the visual-diff workflow** even +though they can't possibly move a single rendered pixel. The +empirical false-positives: + +- **PR #19 `cors-tighten`** (squash `da50d78`, 2026-05-24) — touched + 24 files all under `pages/api/**`. Triggered `Screenshot diff`, + documented in `.convoys/ship-readiness.md` § P0 #5 As-shipped + metrics: *"Screenshot diff — workflow exited 0 because of + `continue-on-error: true`, but the actual visual test failed with + the documented 'snapshot doesn't exist' error… Triggered on PR #19 + despite this being API-only because its `paths:` filter is + `pages/**` which matches `pages/api/**` too — minor false-positive + queued as `tighten-visual-diff-path-filter`."* +- **PR #20 `add-rate-limiting`** (squash `708ef45`, 2026-05-24) — + touched 6 routes under `pages/api/` (plus `lib/rate-limit.js` and + `pages/admin/card-import.js`). Same false-trigger, same swallow. + Documented in `.convoys/ship-readiness.md` § P0 #6 As-shipped + metrics. + +Cost per false-trigger: **~55s of CI runtime** (the `wait-for-vercel-preview` +step hits its 120s budget against the deployed preview, then +Playwright `npm ci` + `npx playwright install` + the `visual` project +runs — even though the test ultimately can't compare against a +non-existent baseline). The `Screenshot diff` job exits 0 because of +`continue-on-error: true` (Decision-4 end state of +`adopt-playwright-smoke`, until `seed-visual-baselines-on-linux` +lands), but it still posts a "Visual Diff — view run" comment and +clutters the PR Checks tab with a green-but-meaningless run. + +## Design decision — negated-glob `!pages/api/**` + +GitHub Actions evaluates `paths:` with [minimatch](https://github.com/isaacs/minimatch) +and supports `!`-prefixed exclusion patterns per the +[official path-filter cheatsheet](https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#patterns-to-match-file-paths). +Order matters: a `!pattern` only takes effect if it comes AFTER an +include that already matched the path. So the canonical shape is: + +```yaml +paths: + - 'pages/**' + - '!pages/api/**' # must follow 'pages/**' to subtract from it + - 'components/**' + ... +``` + +**Considered alternatives:** + +1. **Per-feature paths** — replace `pages/**` with explicit + subdirectory globs (`pages/!(api)/**` *or* `pages/dashboard.js`, + `pages/cards/**`, `pages/decks/**`, …). Rejected: too verbose, + needs to be touched every time a top-level page is added, + defeats the "trigger on UI changes" intent. +2. **Extglob `pages/!(api)/**`** — would work in bash with extglob + enabled, but minimatch's default options used by GitHub Actions + do NOT enable extglob without a flag we can't set from YAML. + The queue entry explicitly flagged this risk; the negated-glob + shape is the safer documented path. +3. **Move the gate into the `gate:` job** — add a step that diffs + `pages/api/**` and sets `should_run=false` if every changed file + is API-only. Rejected: more code, more surface, doesn't actually + fire faster (the gate job itself spins up a runner). The native + `paths:` filter short-circuits BEFORE any runner spins up, + which is the cheapest possible exclusion. + +The simple negation is sufficient and matches GitHub's published +guidance. + +## The fix + +Single edit in `.github/workflows/visual-diff.yml`. Insert +`!pages/api/**` immediately after `pages/**`, with an inline comment +explaining the ordering rule and the empirical motivation: + +```yaml +paths: + - 'pages/**' + # Exclude API-only edits — they don't render UI, so they can't move + # any visual-diff pixels. Order matters: GitHub Actions evaluates the + # `paths:` list with minimatch and applies `!`-prefixed exclusions + # only after they've already matched a prior include. Keep this entry + # immediately AFTER `pages/**`. + # Surfaced by `tighten-visual-diff-path-filter` after PR #19 + # (cors-tighten) and PR #20 (add-rate-limiting) both falsely + # triggered Screenshot diff at ~55s/PR. + - '!pages/api/**' + - 'components/**' + - 'styles/**' + - 'tailwind.config.js' + - 'postcss.config.js' +``` + +All five existing entries are preserved; only the one exclusion entry +is added. + +### `.github/workflows/preview-smoke.yml` — left untouched + +Verified the sibling workflow's shape: + +```yaml +on: + pull_request: + branches: [main] + types: [labeled, opened, synchronize, reopened] +``` + +`preview-smoke.yml` has **no `paths:` filter at all** — it triggers +on every PR targeting `main` (modulo the in-job `gate:` skip via +`pipeline: skip smoke` in the PR body). This is intentional: a smoke +test that hits the home redirect, the sign-in page, and `/api/health` +SHOULD run on every PR including API-only ones, because changes to +`pages/api/**` can break those routes too. There is no false-positive +shape to fix here. Leaving `preview-smoke.yml` strictly out of scope. + +## Verification plan + +1. **YAML parse** — `python3 -c "import yaml; ..."` confirms the + `paths:` list deserializes to the expected 6-entry list with + `'!pages/api/**'` at index 1 (immediately after `'pages/**'`). + Done at gate time, see § Acceptance criteria. +2. **`npm run lint`** — exits 1 with **128 problems** (baseline + preserved, no regression). YAML files don't go through ESLint; + verification here is just that we didn't accidentally edit a + `.js` source file. +3. **`npm run test:run`** — **21/21 pass**. YAML changes don't touch + any test surface; verification only. +4. **Post-merge CI behavior verification — DEFERRED.** The only true + verification that the `!pages/api/**` exclusion actually fires + the way we expect is observing the **next API-only PR after this + merges** and confirming `Screenshot diff` does NOT appear in its + Checks tab. We document this explicitly here so the doc-writer + pass that closes the convoy can record the next API-only PR's + number + a "Screenshot diff: not triggered" line as the + as-shipped success criterion (mirroring the + `fix-reset-db-script` convoy's Screenshot-diff-not-triggered + line in its as-shipped block). + + We do NOT attempt to live-verify the path filter at convoy time + (e.g. by pushing a throwaway API-only commit to a sacrificial + branch and watching CI). That'd be theater — GitHub's path-filter + semantics are documented and stable, and the YAML parse + the + syntax match against the published cheatsheet is enough + pre-merge confidence for a P3 polish convoy. + +## Acceptance criteria + +- `python3 -c "import yaml; d=yaml.safe_load(open('.github/workflows/visual-diff.yml')); print(d[True]['pull_request']['paths'])"` + → `['pages/**', '!pages/api/**', 'components/**', 'styles/**', 'tailwind.config.js', 'postcss.config.js']` + (order-sensitive) +- `npm run lint` → exit 1 with 128 problems (baseline preserved) +- `npm run test:run` → 21/21 pass +- `.github/workflows/preview-smoke.yml` unchanged in this PR's diff +- No other workflow files touched + +## Risks + +- **R1 — minimatch syntax compatibility.** GitHub Actions uses + minimatch internally; the `!` prefix at the start of a pattern is + documented as the canonical exclusion syntax. If for any reason + Actions rejects this shape on the next workflow load (unlikely — + this exact shape is shown in the published cheatsheet), the + workflow would either fail to register OR silently treat the `!` + pattern as a literal include. **Fallback:** restructure to + per-feature path globs (`pages/dashboard.js`, `pages/cards/**`, + `pages/decks/**`, `pages/deck/**`, `pages/deck-builder.js`, + `pages/scanner.js`, `pages/profile.js`, `pages/settings.js`, + `pages/login.js`, `pages/register.js`, `pages/admin/**`, + `pages/collection/**`, `pages/collections.js`, `pages/community/**`, + `pages/invite/**`, `pages/my-cards.js`, `pages/card/**`, + `pages/_app.js`, `pages/_document.js`, `pages/_error.js`, `pages/index.js`). + More verbose, but unambiguously valid. Track as a follow-up convoy + ONLY if the post-merge verification step (next API-only PR) shows + the exclusion didn't fire. +- **R2 — future `pages//` subdir.** If someone + later adds a directory like `pages/server/**` that contains both + API-style endpoints AND visual UI pages, the simple `!pages/api/**` + exclusion would not catch it, and visual-diff would fire on + changes to that directory. Documented but accepted: the repo + convention for the foreseeable future is "all backend lives under + `pages/api/**`", and the only realistic alternative ("server + components" or similar) would warrant its own paths-filter + revisit at that point. R2 is a "watch this space" risk, not a + blocker. + +## Scope + +- **In scope:** `.github/workflows/visual-diff.yml` only (plus this + convoy planning doc). +- **Out of scope:** any other workflow file. Verified + `preview-smoke.yml` has no `paths:` filter and intentionally fires + on every PR, so no mirror-fix is needed there. + +## Why no architect + +This is a **single-file YAML tweak following published vendor +documentation**. No new precedents; no new decisions; the queue +entry in `.convoys/ship-readiness.md` § Queued convoys already +ratified the negated-glob direction. Parent applies the fix, runs +the bounded checks (YAML parse + lint baseline + vitest), opens the +PR. If anything surprising surfaces (the YAML doesn't parse, +minimatch rejects the syntax), the parent stops and dispatches an +architect mid-execution. + +## Out of scope (queued follow-ups) + +- **`seed-visual-baselines-on-linux`** (priority: P3 polish; **was + already queued** by `adopt-playwright-smoke`) — once visual + baselines are seeded under `tests/visual/__screenshots__/` from a + Linux runner (or the documented Playwright Docker container), the + `Screenshot diff` job will start posting real visual-diff + comparisons and `continue-on-error: true` can be removed. This + convoy's path-filter tightening is orthogonal to baseline seeding — + both are needed eventually, but neither blocks the other. Surfaced + in `AGENTS.md` § 6 Testing. + +## As-shipped + +*(stub — populated by post-merge doc-writer pass)* + +- Squash commit: `` +- PR: `` +- Diff stat: `` (expected: 2 files, +N / -0 — `visual-diff.yml` + +N for the one entry + comment block; this convoy file +M for the + full planning doc) +- Verification at merge: + - YAML parse: paths list includes `!pages/api/**` immediately after + `pages/**` + - Lint: 128 problems (baseline preserved) + - Vitest: 21/21 + - All pre-existing CI gates green at merge +- **Post-merge success criterion** (the deferred verification from + § Verification plan): the next API-only PR after this merges does + NOT show `Screenshot diff` in its Checks tab. Doc-writer to record + that PR's number + the absence of `Screenshot diff` as the + as-shipped success line, mirroring `.convoys/fix-reset-db-script.md`'s + *"Screenshot diff: not triggered (script-only PR — `paths:` filter + excludes `scripts/**`…)"* line. + +## Owns + +Parent (single-file proven-pattern fix; no architect or implementer +subagent required). diff --git a/.github/workflows/visual-diff.yml b/.github/workflows/visual-diff.yml index f6771e9..744233c 100644 --- a/.github/workflows/visual-diff.yml +++ b/.github/workflows/visual-diff.yml @@ -9,6 +9,15 @@ on: branches: [main] paths: - 'pages/**' + # Exclude API-only edits — they don't render UI, so they can't move + # any visual-diff pixels. Order matters: GitHub Actions evaluates the + # `paths:` list with minimatch and applies `!`-prefixed exclusions + # only after they've already matched a prior include. Keep this entry + # immediately AFTER `pages/**`. + # Surfaced by `tighten-visual-diff-path-filter` after PR #19 + # (cors-tighten) and PR #20 (add-rate-limiting) both falsely + # triggered Screenshot diff at ~55s/PR. + - '!pages/api/**' - 'components/**' - 'styles/**' - 'tailwind.config.js'