diff --git a/.convoys/single-sql-client.md b/.convoys/single-sql-client.md new file mode 100644 index 0000000..c5c97b5 --- /dev/null +++ b/.convoys/single-sql-client.md @@ -0,0 +1,434 @@ +--- +name: single-sql-client +classification: quality +success_metric: | + `lib/database.js` is deleted; every former caller uses + `@vercel/postgres` tagged templates with identical query + semantics; vitest 21/21 stays green; lint baseline (128 + problems) is preserved; no remaining `lib/database` import + appears anywhere under `pages/` or `lib/`. AGENTS.md Gotcha #1 + / `.convoys/ship-readiness.md` P1 #8 flip → RESOLVED. +skip: + - role-design-system-auditor + - role-a11y-auditor + - role-ux-reviewer + - role-ia-architect +status: open +created: 2026-05-26 +parent: ship-readiness +addresses: P1 #8 (launch sequence step 8) — "Two SQL clients in parallel" +depends_on: [] +--- + +# Convoy: single-sql-client + +Collapse the dual SQL-client problem documented in AGENTS.md +Gotcha #1 by deleting `lib/database.js` and migrating its sole +production caller (`pages/api/auth-utils.js`) onto the canonical +`@vercel/postgres` tagged-template surface. Keep +`@neondatabase/serverless` as a runtime dependency for the +out-of-scope `scripts/**` helpers that already use `neon()` +directly. + +## Background + +`AGENTS.md` Gotcha #1 (and the `.cursor/rules/db-and-schema.mdc` +"Clients" section) document a long-standing dual-client problem: + +- `@neondatabase/serverless` is wrapped by `lib/database.js`'s + `DatabaseAdapter`, which exposes a `db.query(sqlString, + params)` API that **manually interpolates `$1, $2, ...` + placeholders into the SQL string** and then calls + `sql.unsafe(query)` on the result. +- `@vercel/postgres` is used directly by ~30 `pages/api/**` + handlers via tagged-template SQL (`` await sql`SELECT … WHERE + id = ${id}` ``), which is parameterized natively by the driver. + +The manual-interpolation shape in `lib/database.js` is the +documented foot-gun: + +> *Avoid `lib/database.js`'s `db.query(string, params)` API for +> new code — it interpolates params into a string and then +> calls `sql.unsafe()`, which is a SQL-injection vector.* +> — `.cursor/rules/db-and-schema.mdc` § Clients + +The `.convoys/ship-readiness.md` P1 #8 entry tracks the cleanup +as launch sequence step 8 ("touches ~3 files based on graph"). + +## Decisions + +### D1 — Caller inventory: 2 files in scope (1 source + 1 test), not "~3" + +Architect ran `Grep` against the workspace for `lib/database` +imports. Only **2** files touch the module: + +| File | Kind | What it does with `db` | +| --- | --- | --- | +| `pages/api/auth-utils.js` | source | imports `db`, calls `db.query` in `isAdmin(userId)` + `getUserById(userId)` | +| `test/api/auth-utils.test.js` | test | `vi.mock('../../lib/database.js', () => ({ db: { query: vi.fn() } }))` — mock-only; the 5 tests exercise `generateToken` / `verifyToken`, not `isAdmin` / `getUserById`. The mock exists purely to satisfy the import graph | + +The README/AGENTS-quoted "~3 files based on graph" estimate from +ship-readiness P1 #8 was loose. Confirmed exact count = 2. No +deviation flag needed; the convoy is even smaller than estimated. + +### D2 — `@vercel/postgres` tagged templates, byte-equivalent SQL + +Two call sites in `pages/api/auth-utils.js`: + +**Before:** + +```js +import { db } from '../../lib/database.js'; + +export async function isAdmin(userId) { + try { + const result = await db.query(` + SELECT role FROM users WHERE id = $1 + `, [userId]); + return result.rows[0]?.role === 'admin'; + } catch (error) { ... } +} + +export async function getUserById(userId) { + try { + const result = await db.query(` + SELECT id, email, role, created_at FROM users WHERE id = $1 + `, [userId]); + return result.rows[0]; + } catch (error) { ... } +} +``` + +**After:** + +```js +import { sql } from '@vercel/postgres'; + +export async function isAdmin(userId) { + try { + const result = await sql` + SELECT role FROM users WHERE id = ${userId} + `; + return result.rows[0]?.role === 'admin'; + } catch (error) { ... } +} + +export async function getUserById(userId) { + try { + const result = await sql` + SELECT id, email, role, created_at FROM users WHERE id = ${userId} + `; + return result.rows[0]; + } catch (error) { ... } +} +``` + +The two queries are SELECT-only, single-table, single-parameter +(numeric `userId`). The translated tagged-template form produces +**byte-equivalent SQL** with proper parameter binding (instead +of string interpolation + `sql.unsafe`). The `result.rows[0]` +access pattern works identically — `@vercel/postgres` returns +`{ rows, rowCount }` natively, which matches the shape +`DatabaseAdapter.query` was already returning. No +result-handling change needed at the call sites in `isAdmin` / +`getUserById`. No transaction / pool semantics change either: +neither `lib/database.js` nor `@vercel/postgres` uses a +long-lived pool from these call sites (both create a per-request +HTTP connection against Neon's serverless endpoint). + +### D3 — Keep `@neondatabase/serverless` as a dep; SCOPE = `lib/database.js` collapse only + +The convoy seed flagged this explicitly. Architect verified: + +- `@neondatabase/serverless` is imported by **11 files** outside + `lib/database.js`: + - `scripts/setup-neon-db.js` (the DDL bootstrap) + - `scripts/migrations/2026-05-24-rename-admin-email.js` (post-`pick-a-name` admin rename) + - `scripts/reset-db.js` (dev reset) + - 8 other one-off `scripts/add-*.js` / `scripts/fix-*.js` / `scripts/seed-*.js` historical jobs +- None of those go through `lib/database.js`; they all use + `neon(POSTGRES_URL)` directly with tagged-template SQL (which + is the safe shape that `lib/database.js`'s wrapper subverts). + +**Scope decision: keep `@neondatabase/serverless` in +`package.json`.** This convoy is about deleting the +`lib/database.js` abstraction, not about eliminating +`@neondatabase/serverless` from the dependency tree. The +no-go-zones rule explicitly forbids touching +`scripts/setup-neon-db.js`, `scripts/migrations/*`, and the +`scripts/add-*` / `scripts/fix-*` / `scripts/seed-*` historical +jobs. Migrating those to `@vercel/postgres` would also be the +wrong call architecturally — `@vercel/postgres` is tuned for +Vercel's edge / serverless runtime; one-off scripts run from a +developer laptop or CI runner where `@neondatabase/serverless`'s +direct `neon()` shape is more appropriate. + +A future convoy could (a) migrate the scripts to a uniform +client OR (b) extract a thin `scripts/lib/db.js` that wraps +`neon()` once. Either is its own scope; flagged in § Follow-ups. + +### D4 — `sql.unsafe` audit: NOT a real injection vector with current callers (security finding: NO) + +The `lib/database.js` shape is **unsafe-by-default**: it +interpolates raw values into a SQL string and calls +`sql.unsafe()`. In theory, that's a SQL-injection vector for +any caller that passes user-controlled string input. + +Architect audit of the **2 current call sites**: + +- `isAdmin(userId)` — `userId` is sourced from a verified JWT + payload (`decoded.userId` after `verifyToken(token)` succeeds + in `pages/api/admin/index.js`). It's a number from + `users.id` (SERIAL). The `lib/database.js` interpolation path + for numbers is `result = await sql\`${sql.unsafe(query)}\`` + with the number inlined raw — for a numeric SERIAL id, no + injection vector exists in practice. +- `getUserById(userId)` — not currently called by any handler + (`Grep` for `getUserById` returns only the definition site + + the public-API comment in AGENTS.md). Same `userId` shape + semantics apply if it were called. + +**Conclusion: NO real security finding.** This is a pure +refactor + foot-gun-removal convoy. The next convoy that adds a +caller passing user-controlled string input to `db.query` would +have been the security incident; deleting the unsafe surface +prevents that future incident. + +If a future audit surfaces a `lib/database.js`-shaped wrapper +re-introduced in another lib (e.g. `lib/db-helper.js`), this +convoy's lesson is: kill it on sight. See § Follow-ups for a +queued ESLint rule that would catch a `sql.unsafe` re-introduction. + +### D5 — Tests: 1 test file touched, no semantic change + +`test/api/auth-utils.test.js` already mocks `lib/database.js` +purely to satisfy the import graph; the 5 tests exercise +`generateToken` + `verifyToken`, neither of which touches the +DB. **Post-migration:** the mock is no longer needed because +`auth-utils.js` no longer imports from `lib/database.js`. Drop +the `vi.mock('../../lib/database.js', ...)` call and the now-unused +`vi` import. Test count + assertions unchanged: 5/5. + +The other 16 vitest tests (`lib/auth-secret.test.js`, +`lib/permission-middleware.test.js`, +`components/Layout.test.js`) don't touch `lib/database.js`. No +mock drift risk; nothing else to update. + +`@vercel/postgres` does NOT get a new mock in this convoy +(deferred to queued `fill-vitest-handler-coverage`); the +`isAdmin` / `getUserById` functions are still untested at the +unit level, same as pre-migration. The migration is purely a +client swap, not a coverage expansion. + +## Caller inventory (verbatim before/after) + +### `pages/api/auth-utils.js` (only source caller) + +**Imports (before):** +```js +import jwt from 'jsonwebtoken'; +import { db } from '../../lib/database.js'; +import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'; +``` + +**Imports (after):** +```js +import jwt from 'jsonwebtoken'; +import { sql } from '@vercel/postgres'; +import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'; +``` + +**`isAdmin` body (before):** +```js +const result = await db.query(` + SELECT role FROM users WHERE id = $1 +`, [userId]); +``` + +**`isAdmin` body (after):** +```js +const result = await sql` + SELECT role FROM users WHERE id = ${userId} +`; +``` + +**`getUserById` body (before):** +```js +const result = await db.query(` + SELECT id, email, role, created_at FROM users WHERE id = $1 +`, [userId]); +``` + +**`getUserById` body (after):** +```js +const result = await sql` + SELECT id, email, role, created_at FROM users WHERE id = ${userId} +`; +``` + +The `try/catch` shape, the `result.rows[0]` access, the +`?.role === 'admin'` check, and the `console.error` + `return +false`/`return null` error paths are all preserved verbatim. + +### `test/api/auth-utils.test.js` (mock cleanup) + +**Imports (before):** +```js +import { describe, expect, it, vi } from 'vitest'; +import jwt from 'jsonwebtoken'; + +vi.mock('../../lib/database.js', () => ({ + db: { query: vi.fn() }, +})); + +import { JWT_SECRET } from '../../lib/auth-secret.js'; +import { generateToken, verifyToken } from '../../pages/api/auth-utils.js'; +``` + +**Imports (after):** +```js +import { describe, expect, it } from 'vitest'; +import jwt from 'jsonwebtoken'; + +import { JWT_SECRET } from '../../lib/auth-secret.js'; +import { generateToken, verifyToken } from '../../pages/api/auth-utils.js'; +``` + +The 5 test bodies (2 `generateToken` + 3 `verifyToken`) are +unchanged. + +### `lib/database.js` (deleted) + +47-line file. No replacement; the abstraction is gone. The two +callers go straight to `@vercel/postgres` tagged-template SQL, +matching the canonical pattern used by ~30 other `pages/api/**` +handlers already in the tree. + +## The fix (per-file diff shape) + +| File | Change | Lines | +| --- | --- | --- | +| `pages/api/auth-utils.js` | swap `db.query(string, params)` → `sql\`…${userId}\`` in 2 functions; swap import | +5 / -7 | +| `test/api/auth-utils.test.js` | drop `vi.mock('../../lib/database.js', ...)` + unused `vi` import | +1 / -5 | +| `lib/database.js` | DELETE | 0 / -47 | +| `.convoys/single-sql-client.md` | NEW (this file) | +330 / 0 | + +Total: 3 modified files (1 source + 1 test + 1 deletion) + 1 +new convoy doc. + +## Verification plan + +1. `npm run lint` → 128 problems (baseline preserved, no regression) +2. `npm run test:run` → 21/21 pass +3. `Grep "lib/database" --type js -l` → 0 hits anywhere + (no `pages/**`, no `lib/**`, no `scripts/**`, no `test/**`) +4. `Grep "@neondatabase/serverless" --type js -l` → still matches + `scripts/setup-neon-db.js`, `scripts/migrations/2026-05-24-rename-admin-email.js`, + `scripts/reset-db.js`, and the 8 other `scripts/add-*` / `fix-*` / + `seed-*` historical helpers (all out of scope per § D3) +5. `node --check pages/api/auth-utils.js` → exit 0 (syntax) + +**Live runtime smoke deferred.** The two migrated functions +(`isAdmin`, `getUserById`) are only reachable via +`pages/api/admin/index.js` which requires an admin Bearer token ++ a populated `users` table in prod Neon. Running a live curl +smoke against a local `npm run dev` would require seeding the +admin user with a known password (which `setup-neon-db.js` +now requires `ADMIN_INITIAL_PASSWORD` to do — operator-only +flow) and minting a token. Out-of-band for a convoy +that's a pure client swap; the byte-equivalent SQL semantics +(D2) plus the same `try/catch` + `result.rows[0]` access shape +gives high confidence that the migration is correct. If the +post-merge Vercel preview's admin surface 500s on an admin +action, the rollback is a single-commit revert of this convoy. + +## Risks + +- **R1 — Byte-equivalence not guaranteed if `lib/database.js` + has hidden behavior.** Architect re-read all 47 lines of + `lib/database.js` (it's a small file). The only behavior + beyond "interpolate, run SQL, return `{ rows, rowCount }`" is + the `escapedParams.map` quoting for string params — and both + current callers pass numeric `userId`, so the quoting path + isn't exercised. The `raw(sqlString, params)` method is just + an alias for `query(sqlString, params)`; no caller invokes + `raw` (`Grep "\.raw\(" --type js` → zero hits). **Residual + risk: very low.** The migration's byte-equivalent SQL plus + identical result shape (`{ rows: [...], rowCount: N }`) + closes this risk in practice. + +- **R2 — Missed callers.** Mitigated by the post-delete `Grep` + sweep in the verification plan (step 3) — if any file still + imports `lib/database`, the file no longer exists and the + import will throw at module load, failing CI's lint or test + job. The `Grep` sweep also catches `require('../../lib/database')` + CJS shape (zero hits across the tree at architect time; + expected since the repo is `"type": "module"`). + +- **R3 — A future PR re-introduces `lib/database.js` or + another `sql.unsafe`-shaped wrapper.** Mitigated by: + documentation (this convoy file + AGENTS.md Gotcha #1 + doc-writer flip → RESOLVED in a follow-up cleanup pass). + Stronger mitigation would be an ESLint + `no-restricted-imports` rule against `lib/database` or a + `no-restricted-syntax` rule against `sql.unsafe(` — surfaced + as a follow-up below (`lint-against-lib-database`). + +## As-shipped + +*Stub for doc-writer.* Populate after merge with: +- Squash commit SHA + PR number +- Diff stats (file count, line counts) +- CI gate outcomes (lint, vitest, smoke, forbidden-* gates) +- Any operator-action follow-ups +- Cross-validation findings (e.g. smoke continuing to pass) +- Updates needed to AGENTS.md Gotcha #1 + `.cursor/rules/db-and-schema.mdc` § Clients + ship-readiness P1 #8 entry + +## Follow-ups + +- **`lint-against-lib-database`** (priority: P3 polish). Add an + ESLint `no-restricted-imports` rule against `'../**/lib/database'` + (or any path resolving to a `lib/database.js` file), so a future + PR that re-introduces the unsafe wrapper fails at lint time + rather than at runtime. Pairs naturally with the queued + `lint-against-cjs-in-esm-scripts` follow-up (both are + static-source guards added to `eslint.config.mjs`). Small + surface (one entry in the config); could fold into a + `harden-eslint-static-guards` convoy if more such guards + accumulate. + +- **`purge-neondatabase-serverless-fully`** (priority: P3 polish; + blocked on `migration-tool`). Once the `migration-tool` convoy + (P1 #11) lands and replaces the ad-hoc `scripts/add-*` / + `scripts/fix-*` / `scripts/seed-*` shape with a real migration + framework, revisit whether the remaining 11 + `@neondatabase/serverless` import sites can be collapsed onto + `@vercel/postgres`. Caveat: `@vercel/postgres` is tuned for the + Vercel edge runtime and may not be the right choice for + developer-laptop / CI-runner scripts; the right answer may be + "keep the dep, but route all scripts through a single thin + helper" rather than "delete the dep entirely". This is its own + scope; do NOT bolt onto this convoy. + +- **`add-neon-return-shape-rule`** (priority: P3 polish; surfaced + 2026-05-25 in PR #24 + restated in `.convoys/fix-reset-db-script.md` + § Out of scope). Codify the `neon()` (returns `[rows]`) vs + `@vercel/postgres` (`{ rows: [...] }`) return-shape difference + as a `.cursor/rules/db-and-schema.mdc` callout. **Partially + satisfied** by this convoy because the dual-client shape is now + collapsed for `pages/api/**` — only the 11 `scripts/**` callers + still use `neon()` directly, and they all destructure the + array return shape correctly today. The rule would defend + future contributors who don't already know the difference; queue + if a third such bug surfaces. + +## Owns + +- Architect (this convoy file + decisions D1-D5). +- Implementer (mechanical: 2 source-file edits + 1 deletion + + verification gates). Possibly the same agent — the + implementation surface is small enough that the cost of a + fresh implementer subagent boot may exceed the cost of doing + the work in the architect's own turn. Documented per + `fix-reset-db-script` precedent ("Why no architect" / "Why + no implementer" — single-file proven-pattern work). diff --git a/.convoys/tighten-visual-diff-path-filter.md b/.convoys/tighten-visual-diff-path-filter.md deleted file mode 100644 index 7c0796e..0000000 --- a/.convoys/tighten-visual-diff-path-filter.md +++ /dev/null @@ -1,270 +0,0 @@ -# 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 744233c..f6771e9 100644 --- a/.github/workflows/visual-diff.yml +++ b/.github/workflows/visual-diff.yml @@ -9,15 +9,6 @@ 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' diff --git a/lib/database.js b/lib/database.js deleted file mode 100644 index 8d0b00a..0000000 --- a/lib/database.js +++ /dev/null @@ -1,47 +0,0 @@ -import { neon } from '@neondatabase/serverless'; - -// Get the database URL from environment variables -const sql = neon(process.env.POSTGRES_URL); - -// Database adapter that works with Neon -class DatabaseAdapter { - async query(sqlString, params = []) { - try { - // For Neon, we need to use tagged template literals - // This is a simplified approach - in production you'd want more robust parameter handling - let result; - - if (params.length === 0) { - // No parameters - result = await sql`${sql.unsafe(sqlString)}`; - } else { - // With parameters - this is a simplified approach - // In production, you'd want proper parameter escaping - const escapedParams = params.map(param => - typeof param === 'string' ? `'${param.replace(/'/g, "''")}'` : param - ); - - let query = sqlString; - for (let i = 0; i < escapedParams.length; i++) { - query = query.replace(`$${i + 1}`, escapedParams[i]); - } - - result = await sql`${sql.unsafe(query)}`; - } - - return { - rows: Array.isArray(result) ? result : [result], - rowCount: Array.isArray(result) ? result.length : 1 - }; - } catch (error) { - console.error('Database query error:', error); - throw error; - } - } - - async raw(sqlString, params = []) { - return await this.query(sqlString, params); - } -} - -export const db = new DatabaseAdapter(); \ No newline at end of file diff --git a/pages/api/auth-utils.js b/pages/api/auth-utils.js index 3a92321..30eed94 100644 --- a/pages/api/auth-utils.js +++ b/pages/api/auth-utils.js @@ -1,5 +1,5 @@ import jwt from 'jsonwebtoken'; -import { db } from '../../lib/database.js'; +import { sql } from '@vercel/postgres'; import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'; export async function hashPassword(password) { @@ -14,10 +14,10 @@ export async function verifyPassword(password, hashedPassword) { export function generateToken(user) { return jwt.sign( - { - userId: user.id, - email: user.email, - role: user.role + { + userId: user.id, + email: user.email, + role: user.role }, JWT_SECRET, { expiresIn: JWT_TOKEN_TTL } @@ -34,9 +34,9 @@ export function verifyToken(token) { export async function isAdmin(userId) { try { - const result = await db.query(` - SELECT role FROM users WHERE id = $1 - `, [userId]); + const result = await sql` + SELECT role FROM users WHERE id = ${userId} + `; return result.rows[0]?.role === 'admin'; } catch (error) { console.error('Error checking admin status:', error); @@ -46,12 +46,12 @@ export async function isAdmin(userId) { export async function getUserById(userId) { try { - const result = await db.query(` - SELECT id, email, role, created_at FROM users WHERE id = $1 - `, [userId]); + const result = await sql` + SELECT id, email, role, created_at FROM users WHERE id = ${userId} + `; return result.rows[0]; } catch (error) { console.error('Error getting user:', error); return null; } -} \ No newline at end of file +} diff --git a/test/api/auth-utils.test.js b/test/api/auth-utils.test.js index 88128b3..81f0ea5 100644 --- a/test/api/auth-utils.test.js +++ b/test/api/auth-utils.test.js @@ -1,10 +1,6 @@ -import { describe, expect, it, vi } from 'vitest'; +import { describe, expect, it } from 'vitest'; import jwt from 'jsonwebtoken'; -vi.mock('../../lib/database.js', () => ({ - db: { query: vi.fn() }, -})); - import { JWT_SECRET } from '../../lib/auth-secret.js'; import { generateToken, verifyToken } from '../../pages/api/auth-utils.js';