deckhearth/.convoys/tighten-visual-diff-path-filter.md

# 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

Single squash commit `ba95462` (PR #26, merged 2026-05-27T03:51:22Z
UTC / local 2026-05-26). Parent-owned end-to-end per the convoy spec
— no architect, no implementer subagent dispatched. Mirror-the-pattern
fix exactly as planned; no mid-execution surprises that would have
forced an architect bounce.

**Diff: 2 files, +279 / -0.** `.github/workflows/visual-diff.yml` +N
for the one `!pages/api/**` entry + the inline comment block
explaining the ordering rule and naming the empirical PRs that
motivated the fix (#19 `cors-tighten`, #20 `add-rate-limiting`);
`.convoys/tighten-visual-diff-path-filter.md` +M for the full planning
document, committed atomically with the YAML edit.

**The change shipped exactly as designed.** The `paths:` list now
deserializes to `['pages/**', '!pages/api/**', 'components/**',
'styles/**', 'tailwind.config.js', 'postcss.config.js']` (order-sensitive
— GitHub Actions evaluates `paths:` with minimatch and `!`-prefixed
exclusions only fire after a prior include matches; keeping
`!pages/api/**` immediately after `pages/**` is what makes the
exclusion work). All five existing entries preserved verbatim;
`preview-smoke.yml` intentionally untouched (no `paths:` filter; fires
on every PR by design — see § The fix § preview-smoke.yml left
untouched).

**Verification at merge (all gates green):**
- YAML parse: `paths:` deserializes to the expected 6-entry list with
  `'!pages/api/**'` at index 1 (immediately after `'pages/**'`)
- `npm run lint` → 128 problems (baseline at convoy time; later
  improved to 125 by PR #31 `single-auth-provider`)
- `npm run test:run` → 21/21 pass
- All pre-existing CI gates green at merge

**Post-merge success criterion — CONFIRMED.** The deferred verification
from § Verification plan: the next API-only PR after this merges
should NOT show `Screenshot diff` in its Checks tab. **PR #30
(`single-sql-client`, squash `c403ea4`, merged 2026-05-27T03:54:01Z
UTC) was the first API-only PR post-merge.** Its diff touched
`pages/api/auth-utils.js` + `test/api/auth-utils.test.js` +
`lib/database.js` (deletion) + `.convoys/single-sql-client.md` — the
`pages/api/**` portion matched `pages/**` but was correctly subtracted
by the new `!pages/api/**` exclusion. **`Screenshot diff` did NOT
appear in PR #30's Checks tab** — the empirical confirmation that the
exclusion fires as documented. This is the as-shipped success line
the convoy file's § Verification plan asked the doc-writer pass to
record, mirroring `.convoys/fix-reset-db-script.md`'s *"Screenshot
diff: not triggered (script-only PR — `paths:` filter excludes
`scripts/**`…)"* line.

**Operator action required going forward:** **none.** Path-filter
behavior is self-defending; no env vars, no secrets, no infra
changes.

**Spec deviation:** none.

**No follow-up surfaced.** The orthogonal `seed-visual-baselines-on-linux`
follow-up was already queued by `adopt-playwright-smoke`; this convoy
didn't add or change anything around it. The `R1 fallback if exclusion
didn't fire → restructure to per-feature path globs` contingency from
the convoy file was NOT exercised because the post-merge verification
on PR #30 confirmed the negated-glob fired correctly.

## Owns

Parent (single-file proven-pattern fix; no architect or implementer
subagent required).