ci(workflows): exclude pages/api/** from visual-diff path filter (#26)
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 <cursoragent@cursor.com>
This commit is contained in:
parent
5f1fa60cba
commit
ba954629ed
2 changed files with 279 additions and 0 deletions
270
.convoys/tighten-visual-diff-path-filter.md
Normal file
270
.convoys/tighten-visual-diff-path-filter.md
Normal file
|
|
@ -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/<non-api>/<api-like>` 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: `<TBD>`
|
||||||
|
- PR: `<TBD>`
|
||||||
|
- Diff stat: `<TBD>` (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).
|
||||||
9
.github/workflows/visual-diff.yml
vendored
9
.github/workflows/visual-diff.yml
vendored
|
|
@ -9,6 +9,15 @@ on:
|
||||||
branches: [main]
|
branches: [main]
|
||||||
paths:
|
paths:
|
||||||
- 'pages/**'
|
- '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/**'
|
- 'components/**'
|
||||||
- 'styles/**'
|
- 'styles/**'
|
||||||
- 'tailwind.config.js'
|
- 'tailwind.config.js'
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue