From ba954629ed7f1e8165add48f35ad5dc0e226bdc5 Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Tue, 26 May 2026 22:51:22 -0500 Subject: [PATCH 1/6] ci(workflows): exclude pages/api/** from visual-diff path filter (#26) 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' From 5f2b234cc85ebe6dc74468449f05a98d3b4a0f77 Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Tue, 26 May 2026 22:53:24 -0500 Subject: [PATCH 2/6] fix(scripts): require TEST_USERS_PASSWORD + purge weak literals from test-user helpers (#27) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `scripts/create-test-users.js` hardcoded `bcrypt.hash('alice123', 12)` + `bcrypt.hash('bob123', 12)` and echoed those literals back to stdout both per-user and in a final summary block. `TESTING_GUIDE.md`'s Test Accounts table documented the same `admin123` / `alice123` / `bob123` trio. These were the last two weak-credential surfaces left in the helper-script + manual-QA-doc tree after `drop-public-setup` (commits `ff80753` + `b63b509`) and `fix-reset-db-script` (squash `3ab9bf8`, PR #25) closed the `setup-neon-db.js` and `reset-db.js` halves of the umbrella `purge-weak-creds-from-helpers` queued follow-up. The fix mirrors the post-`drop-public-setup` `setup-neon-db.js` pattern and the post-PR-#25 `reset-db.js` pattern verbatim, with one deliberate simplification: a single `TEST_USERS_PASSWORD` env var covers both alice + bob rather than per-user env vars (risk R2 in the convoy file argues this — these are fixture users for the collaboration demo flow, not independent identities, and per-user sprawl would double the env-var contract for zero security benefit). `createTestUsers()` now reads `process.env.TEST_USERS_PASSWORD` at the top of the function body and exits with code 1 BEFORE opening any DB connection if the var is unset or whitespace-only, with the same helpful-error wording template the other two scripts use (names the var, points at `.env.local`, suggests `openssl rand -base64 24`, references README's "First-time admin setup" section). All four password-echo `console.log` lines are deleted; the new summary documents *where* the password comes from without ever printing it. `TESTING_GUIDE.md`'s Test Accounts table is rewritten to show password source per user instead of the literal value; the two inline `Password: alice123` / `Password: bob123` workflow snippets are replaced with placeholder text. Unlike the previous two convoys, no CJS→ESM conversion was needed — `create-test-users.js` was already top-level ESM. Verification (all static — script is destructive and not live-tested): `node --check scripts/create-test-users.js` exit 0; `npm run lint` 128 problems (baseline preserved, no regression); `npm run test:run` 21/21 pass; grep `scripts/ TESTING_GUIDE.md` for `admin123|password123|test123|alice123|bob123` → 0 hits; `TEST_USERS_PASSWORD` referenced 10 times total (5 script + 5 doc). Operator caveat: anyone running `node scripts/create-test-users.js` post-merge must add `TEST_USERS_PASSWORD=` to their `.env.local` first; existing alice + bob rows in already-seeded environments are NOT rotated by re-running this script (`ON CONFLICT (email) DO NOTHING` preserves the old hashes). Same caveat that applies to the `drop-public-setup` admin row. Co-authored-by: Cursor --- .convoys/purge-weak-creds-from-helpers.md | 187 ++++++++++++++++++++++ TESTING_GUIDE.md | 26 ++- scripts/create-test-users.js | 58 +++++-- 3 files changed, 249 insertions(+), 22 deletions(-) create mode 100644 .convoys/purge-weak-creds-from-helpers.md diff --git a/.convoys/purge-weak-creds-from-helpers.md b/.convoys/purge-weak-creds-from-helpers.md new file mode 100644 index 0000000..4c84ba9 --- /dev/null +++ b/.convoys/purge-weak-creds-from-helpers.md @@ -0,0 +1,187 @@ +# purge-weak-creds-from-helpers (P2 hygiene — final scope close) + +**Status:** IN-FLIGHT 2026-05-26 +**Priority:** P2 hygiene (not a security blocker; the test-fixture +script is dev-only and the documented passwords were never reachable +from a production code path — but the bug pattern is the same as the +P0-grade weak-creds shape that `drop-public-setup` removed from +`setup-neon-db.js`, so closing it brings the helper-script surface to +zero weak literals) +**Convoy owner:** parent (no architect — proven-pattern mirror; +single-script + single-doc fix following two already-shipped +applications of the same pattern) +**Opened:** 2026-05-26 +**Classification:** hygiene + +## Background — the multi-convoy history that led here + +The original `purge-weak-creds-from-helpers` convoy was queued in +`.convoys/ship-readiness.md` as the umbrella for sweeping every +helper-script + manual-QA-doc reference to the legacy `admin123` / +`alice123` / `bob123` literals and the legacy `@tcgvault.com` email +domain. Its scope has been progressively whittled down by three +already-shipped convoys: + +1. **`drop-public-setup`** (squash `ff80753` Brief 1 + `b63b509` + Brief 2): replaced the hardcoded `admin123` in + `scripts/setup-neon-db.js` with the fail-loud + `ADMIN_INITIAL_PASSWORD` env-var gate; converted the script from + CJS to ESM so `npm run setup-db` actually runs on Node 22.x. Set + the precedent for the env-var + fail-loud + no-echo pattern that + the next two convoys (and this one) mirror verbatim. +2. **`pick-a-name` Brief 2** (squash `9abbab6`, 2026-05-24): swept + every `@tcgvault.com` literal in scripts + docs to `@deckhearth.com` + together with the one-shot migration script. **Email half done.** +3. **`fix-reset-db-script`** (squash `3ab9bf8`, PR #25, 2026-05-26): + second application of the post-`drop-public-setup` pattern, this + time to `scripts/reset-db.js`. Removed the second `admin123` literal + from the codebase, removed the only remaining `Admin Password:` + echo, converted the third CJS-in-ESM script. + +After those three convoys, the remaining weak-credential surface is +exactly two files — the alice/bob test-user fixture script and the +manual-QA doc that pairs with it. Both are addressed here. + +## Remaining scope (this convoy) + +1. **`scripts/create-test-users.js`** — alice + bob fixtures still + hardcode `bcrypt.hash('alice123', 12)` + `bcrypt.hash('bob123', 12)` + and echo the literal passwords to stdout (`console.log('✅ Created + Alice (alice@deckhearth.com / alice123)')`). +2. **`TESTING_GUIDE.md`** — Test Accounts table still documents the + literal passwords for admin + alice + bob. + +## The fix shape — single env var, no echo, ESM-already + +The fix is a verbatim mirror of the post-`drop-public-setup` +`scripts/setup-neon-db.js` pattern and the post-`fix-reset-db-script` +`scripts/reset-db.js` pattern, with one deliberate simplification: + +- **Single env var: `TEST_USERS_PASSWORD`.** Both alice and bob get + the same hashed value. Per-user env vars (`ALICE_PASSWORD`, + `BOB_PASSWORD`) would be unnecessary sprawl for what is a test + fixture surface — these aren't independent identities, they're a + collaborator-flow demo pair. Risk R2 below argues this explicitly. +- **Fail-loud at the top of `createTestUsers()`.** Reads + `process.env.TEST_USERS_PASSWORD`; if unset or whitespace-only, + prints an actionable error (names the var, points at `.env.local`, + suggests `openssl rand -base64 24`, references README's "First-time + admin setup" section) and `process.exit(1)` BEFORE opening any DB + connection. Same wording template as `setup-neon-db.js` lines 20-26 + and `reset-db.js` lines 29-35. +- **No password echo to stdout.** The previous file logged the + literal `alice123` / `bob123` strings in both the per-user creation + line and the final summary block. All four echo lines are deleted; + the new summary line documents *where* the password comes from + (`(passwords from TEST_USERS_PASSWORD)`) without ever printing the + value. +- **ESM already.** Unlike `setup-neon-db.js` and `reset-db.js` at the + start of their respective convoys, `create-test-users.js` was + already top-level ESM (it imports `{ config } from 'dotenv'`, + `{ sql } from '@vercel/postgres'`, `bcrypt from 'bcryptjs'` at the + top of the file). **No CJS→ESM conversion needed.** This convoy is + the first of the three to skip that half of the pattern. +- **`ON CONFLICT (email) DO NOTHING`** is preserved (already in the + original file at lines 19, 28) — defensive against double-run. + +`TESTING_GUIDE.md`'s Test Accounts table is rewritten to (a) remove +the literal passwords from the table, (b) document the env-var source +for each user, and (c) point at README's "First-time admin setup" +section for the `openssl rand -base64 24` generation tip. The two +inline `Password: alice123` / `Password: bob123` snippets later in the +workflow are replaced with `Password: `. + +## Verification plan (static-grep only — script is destructive) + +This convoy does NOT live-test `create-test-users.js`. The script +opens a DB connection and inserts rows; running it against a Neon +branch in CI or in the boot-the-brief loop would be a side-effect +cost we don't need to incur. The verification surface is entirely +static: + +- `node --check scripts/create-test-users.js` → exit 0 +- `npm run lint` → 128 problems (baseline preserved; no regression) +- `npm run test:run` → 21/21 pass +- Grep `scripts/ TESTING_GUIDE.md` for `admin123|password123|test123| + alice123|bob123` → 0 hits on literal passwords +- Grep `scripts/create-test-users.js` for `require(` → 0 hits + (mirror-the-pattern preserves ESM-only; trivially satisfied here + because the file was already ESM) +- Grep `scripts/ TESTING_GUIDE.md` for `TEST_USERS_PASSWORD` → + expect 10 hits (5 in script: docstring + const + error message + body; 5 in TESTING_GUIDE.md: table + two inline workflow snippets + + explanatory paragraph) + +**Live verification deferred to operator.** Optional post-merge +action: set `TEST_USERS_PASSWORD` in `.env.local`, run +`node scripts/create-test-users.js` against a non-prod Neon branch, +verify alice + bob rows insert; then unset the env var and re-run, +verify the script exits 1 with the helpful error message before +opening the DB connection. + +## Risks + +- **R1 — A CI step or doc dep on the literal passwords.** If + `.github/workflows/**` or any other doc (`docs/**`, `TESTING_GUIDE.md` + sibling files, the agent-context-pipeline docs) references + `alice123` / `bob123` / `admin123` as part of an automated test + flow, removing the literal would break it. **Mitigation:** the grep + hunt covered `scripts/` + `TESTING_GUIDE.md`. The broader hits in + `.convoys/**` and `AGENTS.md` are historical convoy narrative and + must NOT be edited (rewriting history). The only live reference + outside this convoy's scope is `pages/login.js` — see "Surfaced + out-of-scope follow-up" below. +- **R2 — Env-var sprawl.** Using a single `TEST_USERS_PASSWORD` for + both alice and bob is intentional. These are test-fixture users + for the collaboration demo flow in TESTING_GUIDE.md; they aren't + modeled as independent identities anywhere in the auth surface, and + giving them per-user passwords would (a) double the env-var + contract for zero security benefit (anyone running this script + already has full DB access) and (b) drift from the + ADMIN_INITIAL_PASSWORD shape that the operator is already trained + on. If future test-user additions need distinct passwords for + realistic concurrency testing, that's a separate concern and a + separate convoy. + +## Surfaced out-of-scope follow-up + +- **`pages/login.js` "Quick Login" buttons still hardcode the legacy + literals.** Lines 172 + 184 invoke + `handleQuickLogin('alice@deckhearth.com', 'alice123')` and + `handleQuickLogin('bob@deckhearth.com', 'bob123')`. These are + client-side dev convenience buttons that ship to production HTML + and reveal the legacy passwords directly to anyone viewing the + login page source. **NOT in scope for this convoy** (the convoy + spec is "scripts + docs only; do NOT touch `pages/**`"). Queue a + follow-up convoy: `purge-quick-login-from-loginpage` (P2 hygiene) + to either (a) delete the Quick Login section entirely or + (b) gate it behind `process.env.NODE_ENV === 'development'`. The + latter still requires a credential source that doesn't ship to + prod HTML — likely a `.env.local`-only `NEXT_PUBLIC_DEV_*` + convention or a dev-only proxy endpoint. Architect-worth. + +## Operator action required + +- **Pre-merge:** none. No schema change. No new infra. +- **Post-merge:** anyone running `node scripts/create-test-users.js` + (or `npm run create-test-users` if such a script exists) must add + `TEST_USERS_PASSWORD=` to their `.env.local` first. + Existing alice + bob rows in already-seeded environments are + **not** rotated by re-running this script — `ON CONFLICT (email) + DO NOTHING` preserves the old hashes. Any environment that ran + `create-test-users.js` before this convoy still has the weak + `alice123` / `bob123` hashes in its DB; operators must rotate + manually via the app (or drop those rows and re-seed). Same + caveat that applies to the `drop-public-setup` admin row — + `setup-neon-db.js`'s and `create-test-users.js`'s idempotency + means they do NOT rotate; they only seed. + +## Owns + +Parent (single-script + single-doc proven-pattern fix; no architect +or implementer subagent required). + +## As-shipped + +(stub — fill in post-merge) diff --git a/TESTING_GUIDE.md b/TESTING_GUIDE.md index 502f45e..816879c 100644 --- a/TESTING_GUIDE.md +++ b/TESTING_GUIDE.md @@ -2,11 +2,21 @@ ## 👥 Test Accounts -| User | Email | Password | Role | -|------|-------|----------|------| -| Admin | `admin@deckhearth.com` | `admin123` | Admin | -| Alice | `alice@deckhearth.com` | `alice123` | User | -| Bob | `bob@deckhearth.com` | `bob123` | User | +| User | Email | Role | Password source | +|------|-------|------|-----------------| +| Admin | `admin@deckhearth.com` | Admin | `ADMIN_INITIAL_PASSWORD` env var (seeded by `npm run setup-db`) | +| Alice | `alice@deckhearth.com` | User | `TEST_USERS_PASSWORD` env var (seeded by `node scripts/create-test-users.js`) | +| Bob | `bob@deckhearth.com` | User | `TEST_USERS_PASSWORD` env var (seeded by `node scripts/create-test-users.js`) | + +Both env vars must be set in `.env.local` before running the +corresponding seed script — each script fails loud (exit 1, no DB +connection opened) if its env var is unset. Generate strong values +with `openssl rand -base64 24`; see README.md → "First-time admin +setup" for the canonical env-var pattern. + +Alice + Bob share a single `TEST_USERS_PASSWORD` value because this is +a test-fixture surface; that's intentional and documented in the +`purge-weak-creds-from-helpers` convoy. ## 🃏 Sample Cards Available @@ -21,8 +31,8 @@ ### 1. **Login as Alice** ``` -Email: alice@deckhearth.com -Password: alice123 +Email: alice@deckhearth.com +Password: ``` ### 2. **Create a Collection** @@ -51,7 +61,7 @@ Password: alice123 ### 5. **Switch to Bob's Account** - Logout and login as Bob - Email: `bob@deckhearth.com` -- Password: `bob123` +- Password: `` ### 6. **Accept Invitation (Simulated)** Since we're testing locally, simulate email acceptance: diff --git a/scripts/create-test-users.js b/scripts/create-test-users.js index e0c45ed..abb26cb 100755 --- a/scripts/create-test-users.js +++ b/scripts/create-test-users.js @@ -1,39 +1,69 @@ #!/usr/bin/env node +/** + * Create Test Users Script + * + * Seeds the alice + bob test-user fixtures used by the manual QA flows + * in TESTING_GUIDE.md. Both users share a single password supplied via + * the TEST_USERS_PASSWORD environment variable — this is a test-fixture + * helper, not a prod auth surface, so per-user env vars would be + * unnecessary sprawl. + * + * Required env (in .env.local): + * POSTGRES_URL — Neon connection string + * TEST_USERS_PASSWORD — strong password applied to every test user + * (generate with `openssl rand -base64 24`) + * + * Mirrors the post-`drop-public-setup` shape of `setup-neon-db.js` + * (commit b63b509) and the post-`fix-reset-db-script` shape of + * `reset-db.js` (commit 3ab9bf8) — same ESM imports, same fail-loud + * env-var check, same no-password-echo convention. Convoy: + * `purge-weak-creds-from-helpers` (2026-05-26). + */ + import { config } from 'dotenv'; import { sql } from '@vercel/postgres'; import bcrypt from 'bcryptjs'; -// Load environment variables config({ path: '.env.local' }); async function createTestUsers() { + const testUsersPassword = process.env.TEST_USERS_PASSWORD; + if (!testUsersPassword || !testUsersPassword.trim()) { + console.error( + '❌ TEST_USERS_PASSWORD environment variable is not set.\n' + + '\n' + + ' Set it in .env.local before running `node scripts/create-test-users.js`.\n' + + ' Generate a strong password with: openssl rand -base64 24\n' + + ' See README.md → "First-time admin setup" for the env-var pattern.\n' + ); + process.exit(1); + } + try { - console.log('�� Creating test users...\n'); + console.log('👥 Creating test users...\n'); + + const hashedPassword = await bcrypt.hash(testUsersPassword, 12); - // Create Alice (collaborator) - const alicePassword = await bcrypt.hash('alice123', 12); await sql` INSERT INTO users (email, password, role) - VALUES ('alice@deckhearth.com', ${alicePassword}, 'user') + VALUES ('alice@deckhearth.com', ${hashedPassword}, 'user') ON CONFLICT (email) DO NOTHING `; - console.log('✅ Created Alice (alice@deckhearth.com / alice123)'); + console.log('✅ Created Alice (alice@deckhearth.com)'); - // Create Bob (collaborator) - const bobPassword = await bcrypt.hash('bob123', 12); await sql` INSERT INTO users (email, password, role) - VALUES ('bob@deckhearth.com', ${bobPassword}, 'user') + VALUES ('bob@deckhearth.com', ${hashedPassword}, 'user') ON CONFLICT (email) DO NOTHING `; - console.log('✅ Created Bob (bob@deckhearth.com / bob123)'); + console.log('✅ Created Bob (bob@deckhearth.com)'); console.log('\n🎉 Test users created successfully!'); - console.log('\n👥 Available Test Accounts:'); - console.log(' 1. admin@deckhearth.com / admin123 (Admin)'); - console.log(' 2. alice@deckhearth.com / alice123 (User)'); - console.log(' 3. bob@deckhearth.com / bob123 (User)'); + console.log('\n👥 Available Test Accounts (passwords from TEST_USERS_PASSWORD):'); + console.log(' 1. admin@deckhearth.com (Admin — seeded by setup-neon-db.js)'); + console.log(' 2. alice@deckhearth.com (User)'); + console.log(' 3. bob@deckhearth.com (User)'); } catch (error) { console.error('❌ Failed to create test users:', error.message); From 171f5afc6fa3df602dcce219135b79d053d57b25 Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Tue, 26 May 2026 22:53:28 -0500 Subject: [PATCH 3/6] chore(components): remove dead user prop from MobileNavigation (#28) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit components/MobileNavigation.js has accepted a `user` prop ever since the mobile bottom-bar was extracted from Layout, but it has never read any field of `user`. The bottom-bar items (Cards, Decks, Dashboard, Community, More) are statically configured — none of them branch on auth state, role, user id, or any other per-user attribute. The prop is dead. This was originally surfaced as R8 in the fix-layout-default-user convoy (commit ca302a8) and deliberately deferred there to keep that convoy focused on the Layout default-user fix. The follow-up was queued as cleanup-mobile-nav-dead-props in .convoys/ship-readiness.md § Queued convoys. Pre-edit audit confirms the queue entry's premise: `rg '\\buser\\b' components/MobileNavigation.js` returns 1 hit (the destructure on line 5) before the change and 0 hits after. The only active call site is components/Layout.js line 598; the components/Layout.js.backup snapshot also calls it but is a no-go-zone (per .cursor/rules/no-go-zones.mdc § "Append-only / historical") and stays untouched — when that backup is eventually deleted in a separate convoy, its stale call disappears with it. Verification: npm run lint exit 1 with 128 problems (baseline preserved, no regression introduced); npm run test:run 21/21 pass (test/components/Layout.test.js still asserts the logged-out branch contract from PR #15 — the dead-prop removal is invisible to that suite since it does not inspect MobileNavigation's prop shape). Convoy file .convoys/cleanup-mobile-nav-dead-props.md captures the audit, fix, risks (R1: a future per-user bottom-bar feature would need to re-add the prop — accepted; carrying dead state to hedge hypothetical features is worse than paying the one-line re-add cost when the feature actually lands), and acceptance criteria. Co-authored-by: Cursor --- .convoys/cleanup-mobile-nav-dead-props.md | 155 ++++++++++++++++++++++ components/Layout.js | 1 - components/MobileNavigation.js | 2 +- 3 files changed, 156 insertions(+), 2 deletions(-) create mode 100644 .convoys/cleanup-mobile-nav-dead-props.md diff --git a/.convoys/cleanup-mobile-nav-dead-props.md b/.convoys/cleanup-mobile-nav-dead-props.md new file mode 100644 index 0000000..1e75eb1 --- /dev/null +++ b/.convoys/cleanup-mobile-nav-dead-props.md @@ -0,0 +1,155 @@ +# cleanup-mobile-nav-dead-props (P3 polish — single-prop hygiene) + +**Status:** SHIPPED 2026-05-26 (PR TBD) +**Classification:** hygiene +**Priority:** P3 polish (not a bug, not a security issue; dead-prop +removal is purely a clarity-of-surface cleanup) +**Convoy owner:** parent (no architect — single-line prop removal in +one component + one caller; surfaced and pre-decided in a sibling +convoy) +**Opened:** 2026-05-26 + +## Background + +`components/MobileNavigation.js` accepts `{ user, onMenuOpen }` but +never reads `user.*` — the bottom-bar items (Cards, Decks, Dashboard, +Community, More) are static and don't depend on auth state or role. + +This was originally surfaced as **R8** in the `fix-layout-default-user` +convoy (see `.convoys/fix-layout-default-user.md` § R8 and § "Anything +flagged but not acted on") and deferred there with explicit +instructions: *"If the implementer is tempted to delete the prop, they +MUST stop — that's god-component-split / single-auth-provider +territory."* The deferral was correct for that convoy's scope; it is +no longer needed because the prop is genuinely dead at the current +static-bar reality, and removing it does not require a wider auth +refactor. + +The follow-up was queued as `cleanup-mobile-nav-dead-props` in +`.convoys/ship-readiness.md` § Queued convoys, with a note that it +may fold into `god-component-split` (P2 #13) if that lands first. +God-component-split has not landed; this small hygiene convoy ships +first. + +## Audit results + +Pre-edit audit (the spec's "don't blindly trust the queue entry" +clause): + +1. **Reading `components/MobileNavigation.js`** — the file is 171 + lines. Line 5 destructures `{ user, onMenuOpen }`. Lines 6–170 use + `onMenuOpen` exactly once (line 39, as the `onClick` for the "More" + button). `user` does not appear elsewhere — no `user.email`, + `user.role`, `user.id`, no conditional render gated on `user`, no + pass-through to a child component. The bottom-bar `navigationItems` + array is hardcoded and does not branch on auth state. +2. **`rg '\buser\b' components/MobileNavigation.js`** before edit: 1 + hit (the destructure on line 5). After edit: 0 hits. +3. **`rg "MobileNavigation" components/ pages/ --type js`**: two + import + JSX-callsite pairs in the codebase: + - `components/Layout.js` (active) — line 5 import, line 598-601 JSX + call passing `user={user}` and `onMenuOpen={...}`. + - `components/Layout.js.backup` (no-go-zone per + `.cursor/rules/no-go-zones.mdc` § "Append-only / historical" — + "legacy snapshot; delete with a real PR, never edit") — line 5 + import, line 264 JSX call. Left untouched per the no-go-zone + rule; if/when the `.backup` file is eventually deleted, this dead + call disappears with it. + +Audit verdict: `user` is genuinely dead. Cleanup is safe. + +## The fix + +Two-file, three-line diff: + +1. **`components/MobileNavigation.js` line 5**: remove `user` from the + destructured props. + - Before: `export default function MobileNavigation({ user, onMenuOpen }) {` + - After: `export default function MobileNavigation({ onMenuOpen }) {` +2. **`components/Layout.js` lines 598-601**: remove the `user={user}` + JSX attribute from the only active call site. + - Before: + ``` + setIsMobileMenuOpen(true)} + /> + ``` + - After: + ``` + setIsMobileMenuOpen(true)} + /> + ``` + +No new code. No refactors. No tests added (the component has no +direct test coverage; `test/components/Layout.test.js` tests Layout's +logged-out branch and does not assert on `MobileNavigation`'s prop +shape). + +## Out of scope + +- The unused `import { useState } from 'react'` on + `components/MobileNavigation.js` line 3. The hook is imported but + not called. This is a pre-existing dead import unrelated to the + `user` prop; the convoy spec explicitly forbids "refactor anything + else in MobileNavigation.js (this is a single-prop removal)". A + future hygiene pass can sweep it (or it'll get caught by an + eventual lint-no-unused-imports rule). +- `components/Layout.js.backup` — no-go-zone, untouched. + +## Verification plan + +1. `rg '\buser\b' components/MobileNavigation.js` → 0 hits (post-edit + confirmation that the prop is truly gone, not just renamed). +2. `rg "MobileNavigation" components/ pages/ --type js` → confirm + each active call site passes only `onMenuOpen`. +3. `npm run lint` → 128 problems baseline preserved (no regression + introduced; no new dead-code/unused-var warnings created by the + change). +4. `npm run test:run` → 21/21 pass. Specifically, + `test/components/Layout.test.js` continues to pass — its + regression-lock assertions for the logged-out Layout branch + (Gotcha #8) do not depend on `MobileNavigation`'s prop shape, so + the dead-prop removal is invisible to that suite. +5. `npm run build` skipped — relying on Vercel preview CI. Trade-off: + single-prop removal in a leaf component is extremely low risk of + build-time regression, and the Playwright smoke + visual-diff + workflows on the PR will catch any Layout-rendering issue before + merge. + +## Risks + +- **R1 — A future feature that wants per-user bottom-bar items would + need to re-add the prop.** Hypothetical examples: showing an + unread-count badge on a "Notifications" tab gated on `user.id`, or + hiding the "Community" tab for unauthenticated visitors. **Accepted.** + Re-adding a prop is a one-line change when the feature actually + lands; carrying a dead prop "just in case" obscures the current + surface and adds nothing. The cleanup is correct for the + current static-bar reality; future features pay their own + add-the-prop cost. +- **R2 — `components/Layout.js.backup` still references the old prop + shape.** **Accepted.** The backup is a no-go-zone (per + `.cursor/rules/no-go-zones.mdc`) and is dead code by definition. + Touching it would violate the rule; leaving it as a stale snapshot + is the convention. When the backup is eventually deleted in a + separate convoy, this stale call disappears with it. + +## Acceptance criteria + +- 2 files modified, 3 lines net change (1 line edit + 1 attribute + removal from a multi-line JSX block). +- `npm run lint` exit 1 with 128 problems (baseline preserved). +- `npm run test:run` 21/21 pass. +- `rg '\buser\b' components/MobileNavigation.js` → 0 hits post-edit. + +## Owns + +Parent (single-prop removal in a leaf component; no architect or +implementer subagent required; audit confirms the queue entry's +premise). + +## As-shipped + +_To be filled in post-merge._ diff --git a/components/Layout.js b/components/Layout.js index d9472e1..84a14fa 100644 --- a/components/Layout.js +++ b/components/Layout.js @@ -596,7 +596,6 @@ export default function Layout({ children, user = null, showSearch = false }) {
{/* Mobile Navigation - Bottom bar for mobile */} setIsMobileMenuOpen(true)} /> diff --git a/components/MobileNavigation.js b/components/MobileNavigation.js index c492495..56690ae 100644 --- a/components/MobileNavigation.js +++ b/components/MobileNavigation.js @@ -2,7 +2,7 @@ import { useRouter } from 'next/router'; import Link from 'next/link'; import { useState } from 'react'; -export default function MobileNavigation({ user, onMenuOpen }) { +export default function MobileNavigation({ onMenuOpen }) { const router = useRouter(); // Navigation items for the bottom bar From 13d62108cebd182673c7eb90035025872bae343c Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Tue, 26 May 2026 22:53:31 -0500 Subject: [PATCH 4/6] chore(lint): forbid require() in scripts/** under "type": "module" (#29) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add an ESLint no-restricted-syntax rule scoped to scripts/**/*.js that flags any CallExpression with callee name `require`. The recurring bug pattern: helper scripts under scripts/ that use CJS require() throw `ReferenceError: require is not defined` on Node 22.x because package.json has had "type": "module" since bump-next-js. The bug has bitten twice in two convoys — once in drop-public-setup Brief 2 (setup-neon-db.js, commit b63b509) and again in fix-reset-db-script (reset-db.js, PR #25 squash 3ab9bf8). Both were caught at first run, not at lint time. This rule would have caught both at PR time. Rule shape: a second flat-config block at the end of eslint.config.mjs (NOT in the root rules block) targeting only scripts/**/*.js. The error message points at .convoys/fix-reset-db-script.md so the next agent who trips it gets a 1-click path to the exemplar fix (ESM top-level imports for dotenv, neon, bcrypt) instead of having to re-derive it. scripts/migrations/** is already in globalIgnores from pick-a-name Brief 2 and stays excluded. Blast-radius rationale (scripts/** only, not all .js at repo root): matches the actual observed bug surface. pages/api/** is already correctly ESM-imported throughout (verified across add-rate-limiting, cors-tighten, and the add-route skill). The config files (postcss.config.js, tailwind.config.js, next.config.js) intentionally use CJS-style exports that the next-config base rules already handle correctly. A repo-wide ban would produce zero true positives outside scripts/** today and would require explicit allowlist for every config file — strictly more code, more maintenance, zero benefit. Convoy file: .convoys/lint-against-cjs-in-esm-scripts.md (P3 polish, parent-owned, no architect — preventative one-line rule following two proven bug recurrences). Verification: - node --check eslint.config.mjs: exit 0 - npm run lint: 128 problems (81 errors, 47 warnings) — baseline preserved verbatim, zero new false positives in current tree - Negative test: prepended `const x = require('fs');` to scripts/reset-db.js, ran npm run lint, observed exit 1 with 129 problems and the rule firing at line 20:11 with the documented message, then reverted to 128 problems clean - npm run test:run: 21/21 pass (no test surface touched) - Grep: 0 require( occurrences in scripts/**/*.js (current tree is clean; rule starts with zero positives to silence on day 1) Surfaces no new follow-up — this convoy IS the follow-up surfaced by fix-reset-db-script. Co-authored-by: Cursor --- .convoys/lint-against-cjs-in-esm-scripts.md | 176 ++++++++++++++++++++ eslint.config.mjs | 9 + 2 files changed, 185 insertions(+) create mode 100644 .convoys/lint-against-cjs-in-esm-scripts.md diff --git a/.convoys/lint-against-cjs-in-esm-scripts.md b/.convoys/lint-against-cjs-in-esm-scripts.md new file mode 100644 index 0000000..059477b --- /dev/null +++ b/.convoys/lint-against-cjs-in-esm-scripts.md @@ -0,0 +1,176 @@ +--- +name: lint-against-cjs-in-esm-scripts +classification: hygiene +success_metric: future helper scripts that re-introduce CJS `require()` calls under `package.json` "type": "module" fail at lint time, not at first execution +status: open +created: 2026-05-26 +--- + +# lint-against-cjs-in-esm-scripts (P3 polish — parent-owned) + +**Priority:** P3 polish (one-line ESLint rule; no architect required) +**Convoy owner:** parent +**Opened:** 2026-05-26 + +## Background — the recurring bug pattern + +Since `bump-next-js` flipped `package.json` to `"type": "module"`, +any helper script under `scripts/` that uses CJS `require()` throws +`ReferenceError: require is not defined` on Node 22.x at first run. +The same bug has now bitten the repo twice in two convoys: + +1. **`drop-public-setup` Brief 2** (commit `b63b509`, 2026-05-23): + `scripts/setup-neon-db.js` was still CJS post-`bump-next-js`; `npm + run setup-db` was silently broken until Brief 2 swept it to ESM + imports. The convoy retro called this out as "the seed script + silently stopped executing after `bump-next-js`." +2. **`fix-reset-db-script` Brief 1** (commit `3ab9bf8`, PR #25, + 2026-05-26): three `require()` calls in `scripts/reset-db.js` + (lines 10, 12, 142) — same bug, same blast radius (`npm run + reset-db` throws `ReferenceError`), same fix shape (verbatim + mirror of post-`drop-public-setup` `setup-neon-db.js`). + +Both bugs were caught at first run, not at lint time. A small +ESLint rule scoped to `scripts/**/*.js` would have caught both at +PR time and is cheap insurance against a third recurrence. + +## Design decision — `scripts/**` only (NOT all `.js`) + +Two reasonable scopes: + +- **`scripts/**/*.js` (chosen):** matches the actual blast radius — + every observed instance of the bug has been in a helper script. + Per-file-block override in `eslint.config.mjs` via a second flat- + config entry. Zero impact on `pages/api/**` (already correctly + ESM-imported throughout) and zero impact on the root `*.config.js` + files (which are intentionally CJS-shaped and which the next-config + base rules already handle correctly). +- **All `.js` files at repo root (rejected):** broader-than-necessary + blast radius. `pages/api/**` already uses ESM `import` everywhere + (`add-rate-limiting`, `cors-tighten`, and `add-route` skill all + verified this in the last three months). A repo-wide ban would + produce zero true positives outside `scripts/**` today and would + risk breaking config-file shapes that legitimately use CJS + (`postcss.config.js`, `tailwind.config.js` are flagged by + `import/no-anonymous-default-export` today but read CJS-style + exports under the hood — see also Gotcha #9 + #10). + +The scoped rule is a 7-line flat-config block; the broader rule +would require explicit allowlist for every config file, which is +strictly more code and more maintenance. + +## The fix + +Add a new flat-config block at the end of `eslint.config.mjs` (after +the existing `globalIgnores(...)` call, NOT inside the root rules +block) targeting only `scripts/**/*.js`: + +```js +{ + files: ['scripts/**/*.js'], + rules: { + 'no-restricted-syntax': ['error', { + selector: 'CallExpression[callee.name="require"]', + message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.', + }], + }, +}, +``` + +The error message points at `.convoys/fix-reset-db-script.md` so that +the next agent / contributor who triggers the rule gets a 1-click path +to the exemplar fix (ESM top-level imports for `dotenv`, `neon`, +`bcrypt`) instead of having to re-derive it. + +`scripts/migrations/**` is already in `globalIgnores` (from +`pick-a-name` Brief 2's migration script) and stays ignored — the +rule does not fire there even though the migration script is ESM and +correctly uses `import` (no need to re-lint files already excluded). + +The rule fires on `CallExpression[callee.name="require"]` — the AST +shape of a plain `require('foo')` call. It does NOT fire on +`createRequire(import.meta.url)` patterns (which use `Module.createRequire`) +should one ever be needed; the AST callee is `createRequire`, not +`require`. If a future helper script legitimately needs CJS interop, +the right path is `await import('foo')` (ESM dynamic import) — the +rule will not block that either. + +## Verification plan + +1. `node --check eslint.config.mjs` → exit 0 (config parses). +2. `npm run lint` → exit 1 with **128 problems (81 errors, 47 + warnings)** — verbatim match of the pre-convoy baseline (no + regression, no new false positives in the current tree). +3. **Negative test (apply, run, revert):** prepend + `const x = require('fs');` to `scripts/reset-db.js`, run + `npm run lint`, confirm exit 1 with the new rule firing at the + expected line/column and the documented message, then revert. +4. `npm run test:run` → 21/21 pass (no test surface touched; runs + only to confirm vitest is still green). +5. `rg "require\(" scripts/ --type js` → 0 hits (sanity check + confirming the current tree is clean and the rule has zero + positives to silence on day 1). + +## Risks + +- **False positives if anyone legitimately needs `require()` in + `scripts/**`.** None today (verified by step 5 — zero `require(` + hits across all helper scripts in the current tree after PR #25 + and the `drop-public-setup` B2 sweep). If a future script + legitimately needs CJS interop (e.g. a dependency that only + exports CJS without an ESM wrapper), the fix is `await + import('foo')` — ESM dynamic import works in any ESM script and + is not flagged by the rule. If that's somehow not viable, the + escape hatch is a per-line `// eslint-disable-next-line + no-restricted-syntax` with a comment explaining why ESM doesn't + work; lint baseline tracking will catch the disable directive in + review. +- **Rule scope drift.** If someone adds a new top-level scripts + directory (`tools/`, `cli/`, etc.) the rule won't fire there. Low + risk — this repo has consolidated on `scripts/` since inception + and there's no signal of a second scripts directory being added. + Tracked here so the next refactor that reshapes the helper-script + layout knows to extend the `files:` glob. +- **ESLint v10 bump.** When `bump-eslint-10` lands (currently + upstream-blocked per Gotcha #10), re-verify this rule's selector + syntax against the v10 AST behavior. `no-restricted-syntax` is a + stable core rule going back to ESLint v1; no v10 deprecation is + expected, but the smoke check is cheap. + +## Acceptance criteria + +- `node --check eslint.config.mjs` exit 0 +- `npm run lint` exit 1 with 128 problems (baseline preserved) +- `npm run test:run` 21/21 pass +- Negative test passes (rule fires on synthetic `require()` insertion, + reverts cleanly to 128 problems after the synthetic edit is + removed) +- Grep: 0 `require(` occurrences in `scripts/**/*.js` (current tree + is clean — rule starts with zero positives to silence) + +## Out of scope + +- Sweeping any other `scripts/**` file — current tree is clean + (verified by the grep step above). The rule is preventative, + not retroactive. +- Broadening the rule to all `.js` files at repo root — see + § Design decision; `pages/api/**` is already correctly ESM and + the config files (`postcss.config.js`, `tailwind.config.js`, + `next.config.js`) intentionally use CJS-style exports that the + next-config base rules handle correctly. +- Bumping any deps (ESLint stays at v9 per Gotcha #10; no + `typescript-eslint` interaction since the rule is a core rule). +- The `purge-weak-creds-from-helpers` follow-up (the + `scripts/create-test-users.js` portion remains queued; this + convoy only adds the lint rule, not the weak-creds sweep). + +## Owns + +Parent (single-file ESLint config edit; no architect or implementer +subagent required — proven-pattern follow-up to PR #25). + +## As-shipped + +_(Stub — doc-writer to fill in post-merge with squash commit SHA, +PR URL, observed lint baseline before/after, CI gate results, and +any deviations from the planned shape.)_ diff --git a/eslint.config.mjs b/eslint.config.mjs index 1bfbd39..1705fe9 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -11,6 +11,15 @@ const eslintConfig = defineConfig([ 'next-env.d.ts', 'scripts/migrations/**', ]), + { + files: ['scripts/**/*.js'], + rules: { + 'no-restricted-syntax': ['error', { + selector: 'CallExpression[callee.name="require"]', + message: 'Use ESM `import` syntax. `package.json` has "type": "module"; require() throws ReferenceError at runtime. See .convoys/fix-reset-db-script.md.', + }], + }, + }, ]); export default eslintConfig; From c403ea49e8c75944131a5386250588d896b20bd1 Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Tue, 26 May 2026 22:54:01 -0500 Subject: [PATCH 5/6] refactor(db): collapse @neondatabase/serverless onto @vercel/postgres + delete lib/database.js (#30) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Convoy: single-sql-client (P1 quality, launch sequence step 8) Addresses: AGENTS.md Gotcha #1, .convoys/ship-readiness.md P1 #8 ## Decisions - D1: Caller inventory = 2 files (1 source + 1 test), not "~3 based on graph". Only pages/api/auth-utils.js imports `db`; test/api/auth-utils.test.js mocks it purely to satisfy the import graph (the 5 tests exercise generateToken/verifyToken, not isAdmin/getUserById). - D2: Migrate both call sites (isAdmin, getUserById) to @vercel/postgres tagged-template SQL. Queries are SELECT-only, single-table, single-numeric-parameter — byte-equivalent translation; same result shape ({rows, rowCount}); no transaction or pool semantics differ. - D3: KEEP @neondatabase/serverless as a dep. 11 scripts/* files still use `neon()` directly (setup-neon-db.js, migrations/, reset-db.js, 8 historical add-*/fix-*/seed-* jobs). They are out of scope per the no-go-zones rule and the convoy spec; purging the dep entirely would be its own convoy (queued as `purge-neondatabase-serverless-fully`, blocked on migration-tool). - D4: sql.unsafe audit — NOT a real injection vector with current callers (userId comes from a verified JWT, is a numeric SERIAL id). Security finding: NO. Pure refactor + foot-gun removal that prevents the FUTURE caller that would have been the incident. - D5: Test mock cleanup — drop the now-unneeded `vi.mock('../../lib/database.js')` call + unused `vi` import. Test count + assertions unchanged (5/5). ## Per-file changes - pages/api/auth-utils.js: swap `import { db } from '../../lib/database.js'` for `import { sql } from '@vercel/postgres'`; rewrite isAdmin's `db.query(SELECT … WHERE id = $1, [userId])` and getUserById's same shape to `sql\`SELECT … WHERE id = ${userId}\``. Same try/catch, same result.rows[0] access, same error returns. - test/api/auth-utils.test.js: drop vi.mock for lib/database.js + the unused `vi` import. 5/5 tests still pass. - lib/database.js: DELETED (47 lines removed; manual-interpolation + sql.unsafe wrapper is gone). - .convoys/single-sql-client.md: NEW (the convoy file documenting all decisions + caller inventory + verification + risks + follow-ups). ## Verification - npm run lint → 128 problems (baseline preserved, no regression) - npm run test:run → 21/21 pass (vitest) - Grep "lib/database" --type js -l → 0 hits anywhere - Grep "@neondatabase/serverless" --type js -l → still matches the 11 scripts/* sites (expected; out of scope per D3) - node --check pages/api/auth-utils.js → exit 0 ## Scope note This convoy collapses the lib/database.js abstraction onto the canonical @vercel/postgres surface for pages/api/**. It does NOT eliminate @neondatabase/serverless from the dependency tree — that would require migrating the scripts/* helpers, which is out of scope here (no-go-zones rule + convoy spec). Queued as a follow-up. ## Live smoke Deferred. The two migrated functions (isAdmin, getUserById) are only reachable via pages/api/admin/index.js which requires an admin Bearer token and a populated users table in prod Neon. Byte-equivalent SQL + identical result shape gives high confidence; rollback is a single-commit revert if a post-merge admin action 500s. Co-authored-by: Cursor --- .convoys/single-sql-client.md | 434 ++++++++++++++++++++++++++++++++++ lib/database.js | 47 ---- pages/api/auth-utils.js | 24 +- test/api/auth-utils.test.js | 6 +- 4 files changed, 447 insertions(+), 64 deletions(-) create mode 100644 .convoys/single-sql-client.md delete mode 100644 lib/database.js 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/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'; From 0668b0c7d04cb1ac89de37d844e89365ede1bf5f Mon Sep 17 00:00:00 2001 From: varutasu <104105839+varutasu@users.noreply.github.com> Date: Tue, 26 May 2026 22:58:08 -0500 Subject: [PATCH 6/6] refactor(auth): collapse lib/auth-context.js + lib/admin-auth.js onto lib/use-auth.js (#31) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `lib/use-auth.js` is now the sole client-side auth surface (P1 §9 of `.convoys/ship-readiness.md`). The legacy `lib/auth-context.js` (`AuthProvider` + `useAuth`) and `lib/admin-auth.js` (`AdminProvider` + `useAdmin` + `useIsAdmin`) are deleted; every importer is migrated to the canonical hook. Pre-convoy a worst-case page mount issued THREE identical `GET /api/auth/verify` requests (one per provider/hook); the post-convoy floor is one verify per page mount (3 → 1 on `pages/card/[id].js`, 2 → 1 elsewhere). Importer inventory swept (7 source files): - `pages/_app.js` — removed `` wrapper; `` is now the only top-level provider. `lib/use-auth.js` is hook-only, no replacement provider needed. - `pages/index.js`, `pages/scanner.js`, `pages/decks.js`, `pages/deck/[id].js`, `pages/deck-builder.js` — `import { useAuth }` path swap from `../lib/auth-context` to `../lib/use-auth`. All five pages destructured only `{ user }` or `{ user, loading }`; verified no consumer reads `login` / `register` from useAuth (those flows are in `pages/login.js` / `pages/signup.js` which call the API directly), so no shape-parity gap on `lib/use-auth.js`. - `pages/card/[id].js` — replaced `useIsAdmin()` (the only consumer of `lib/admin-auth.js` anywhere in the tree) with synchronous `user?.role === 'admin'` derived from the existing `useAuth()` call. Render condition at line 524 stays byte-identical. Decisions documented in `.convoys/single-auth-provider.md`: - D1: no extension to `lib/use-auth.js` (zero call sites for `login` / `register` from useAuth — those flows are direct fetches in `login.js` / `signup.js`). - D2: `useIsAdmin()` collapses onto `useAuth()`; no separate hook. - D3: provider tree `{children}` → `{children}`. - D4: 3 → 1 verify roundtrip on `card/[id].js`; 2 → 1 on every other page-load. - D5: zero test files modified; the 21-test vitest suite is server- side or prop-driven (`Layout.test.js` passes `user` as a prop, never imports the legacy hooks). Doc / config updates so the deletion lands cleanly: - `.github/CODEOWNERS` — drop the two CODEOWNERS lines for the deleted files. - `AGENTS.md` § 2 architecture row + § 3 "Auth (client)" bullet — rewritten for the post-convoy single-surface state. - `.cursor/rules/auth-and-permissions.mdc` — § "Legacy" reframed to "deleted by this convoy"; § "Authentication state on the client" updated to the post-convoy `useAuth()` shape and the direct-fetch login flow used by `login.js` / `signup.js`. - `.cursor/rules/no-go-zones.mdc` — auth-refactors bullet drops the deleted files from the canonical list. - `.cursor/skills/add-page/SKILL.md` — checklist + anti-pattern row refer to the deletion. Verification: - `rg "lib/auth-context|lib/admin-auth" --type js` → 0 hits in source. - `npm run lint` → 128 → 125 problems (3 fewer errors from the deleted unused-import lines; no regression). - `npm run test:run` → 21/21 pass (including the 5 Layout regression locks from `fix-layout-default-user`, which are prop-driven and unaffected). - `npm run build` → all 26 pages compile end-to-end; no SSR / static- generation breakage that would have surfaced if a page tried to use the legacy context hook unwrapped. - Manual smoke deferred to operator post-merge per convoy doc. Risks (full discussion in convoy file): - R1 shape parity gap — verified zero consumers of legacy-only surface; mitigated. - R2 SSR mismatch from removing `` — `useEffect`- guarded `localStorage` read; identical SSR shape pre/post; build passes. - R3 missed importer — post-delete grep + build pass would surface any miss. - R5 stale `useAuth` cache across components — pre-existing pattern, called out as follow-up rather than addressed here. Out of scope: any change to `lib/permission-middleware.js` (server- side; resolved P0 #1), `lib/auth-secret.js` (resolved P0 #2), `pages/api/**` route handlers, login / register API contracts, or the seeded admin account flow. Co-authored-by: Cursor --- .convoys/single-auth-provider.md | 317 +++++++++++++++++++++++++ .cursor/rules/auth-and-permissions.mdc | 15 +- .cursor/rules/no-go-zones.mdc | 2 +- .cursor/skills/add-page/SKILL.md | 4 +- .github/CODEOWNERS | 2 - AGENTS.md | 4 +- lib/admin-auth.js | 120 ---------- lib/auth-context.js | 116 --------- pages/_app.js | 7 +- pages/card/[id].js | 7 +- pages/deck-builder.js | 2 +- pages/deck/[id].js | 2 +- pages/decks.js | 2 +- pages/index.js | 2 +- pages/scanner.js | 2 +- 15 files changed, 341 insertions(+), 263 deletions(-) create mode 100644 .convoys/single-auth-provider.md delete mode 100644 lib/admin-auth.js delete mode 100644 lib/auth-context.js diff --git a/.convoys/single-auth-provider.md b/.convoys/single-auth-provider.md new file mode 100644 index 0000000..a2b0b3d --- /dev/null +++ b/.convoys/single-auth-provider.md @@ -0,0 +1,317 @@ +# single-auth-provider (P1 quality — collapse three client auth surfaces onto one) + +**Status:** OPEN 2026-05-26 (this convoy) +**Priority:** P1 quality (launch sequence step 9 — `.convoys/ship-readiness.md` § P1 entry 9) +**Convoy owner:** parent (architect + implementer rolled together — diff is mechanical once the +shape-parity decision is made) +**Branch:** `convoy/single-auth-provider` +**Opened:** 2026-05-26 + +## Background + +The repo carried **three parallel client-side auth implementations** since the early days of the +project. `.convoys/ship-readiness.md` § P1 entry 9 ("Three parallel client-side auth +implementations") is the canonical spec; AGENTS.md § 3 already documented `lib/use-auth.js` as the +canonical surface and instructed new code to avoid the other two. This convoy executes the +collapse. + +The three surfaces: + +1. **`lib/use-auth.js::useAuth`** (the keeper). Hook-only — reads `auth_token` from + `localStorage` on mount, hits `/api/auth/verify`, exposes `{ user, loading, logout, refreshAuth }`. + No React context, no `` wrapper required. +2. **`lib/auth-context.js::{ AuthProvider, useAuth }`** (legacy). Context provider + consumer + hook with the same verify-on-mount semantics, plus `login()` and `register()` helpers that + `pages/login.js` / `pages/signup.js` no longer use (those pages call `/api/auth/{login,register}` + directly and write the token to `localStorage` themselves). Wired in `pages/_app.js` as + ``. +3. **`lib/admin-auth.js::{ AdminProvider, useAdmin, useIsAdmin }`** (legacy). A redundant context + that does the *same* verify-on-mount roundtrip, plus a hook-only `useIsAdmin()` that does its + own verify roundtrip on top of that. `AdminProvider` is **not** wired in `_app.js` (verified + by reading `_app.js` pre-convoy: only `` + ``), so `useAdmin()` + would have thrown at runtime if anyone called it — nobody does. Only `useIsAdmin()` has a + live consumer (`pages/card/[id].js`). + +**Symptom that drove ranking this P1.** When `pages/card/[id].js` mounts, it calls +`useAuth()` from `lib/use-auth.js` AND `useIsAdmin()` from `lib/admin-auth.js`, each issuing its +own `GET /api/auth/verify`. With `AuthProvider` mounted on every page via `_app.js`, that's a +**third** verify roundtrip on the very first page load. Three roundtrips, identical request, +serial cost on a cold connection. Post-convoy: 1 roundtrip per page-load. + +## Decisions + +### D1 — Shape parity check on `lib/use-auth.js`. Verdict: no parity gap; do **not** extend. + +`lib/auth-context.js::useAuth()` exposed `{ user, loading, login, register, logout }`. +`lib/use-auth.js::useAuth()` exposes `{ user, loading, logout, refreshAuth }`. + +The apparent gap is `login` / `register`. Verified-by-grep: **zero call sites** invoke +`useAuth().login(…)` or `useAuth().register(…)` anywhere in `pages/**` or `components/**`. The +only callers of those flows are `pages/login.js` and `pages/signup.js`, both of which `fetch` +`/api/auth/{login,register}` directly and write the returned token to `localStorage`. +`useAuth()`'s `useEffect` then picks up the new token on the next mount (or the page can call +`refreshAuth()` to re-verify in place). + +Conclusion: do **not** add `login` / `register` to `use-auth.js`. The legacy methods were dead +code on the consumer surface; preserving them would be cargo-culting and would re-create a +non-DRY login flow (one in `pages/login.js`, one in the hook). `auth-and-permissions.mdc` § +"Authentication state on the client" was updated to document the post-convoy `useAuth()` shape +and to spell out the `login.js` / `signup.js` direct-fetch pattern. + +### D2 — `useIsAdmin()` migration shape. Verdict: collapse onto the existing `useAuth()` call. + +`pages/card/[id].js` is the **only** consumer of `useIsAdmin()`. The page already called +`useAuth()` from `lib/use-auth.js` at line 13 (added by `fix-layout-default-user` Brief 2). The +migration is: + +```js +// Before +const { user } = useAuth(); +// ... +const { isAdmin, loading: adminLoading } = useIsAdmin(); +// ... usage at line 524: {isAdmin && !adminLoading && (...)} + +// After +const { user, loading: authLoading } = useAuth(); +// ... +const isAdmin = user?.role === 'admin'; +const adminLoading = authLoading; +// ... usage at line 524 unchanged: {isAdmin && !adminLoading && (...)} +``` + +`adminLoading` is kept as a local alias rather than substituting `authLoading` directly at the +call site, to keep the diff minimal and the rendering condition byte-identical. The `loading` +window from `useAuth()` covers exactly the same period (`/api/auth/verify` resolution) that +`useIsAdmin`'s own loading covered, so there is no UX regression. + +### D3 — `pages/_app.js` provider tree. Before / after. + +```jsx +// Before + + + + + + +// After + + + +``` + +`useAuth()` from `lib/use-auth.js` is hook-only — no Provider needed. The `` +wrapper is removed entirely; no replacement Provider is added. `` stays (out of +scope). `` was never in the tree to begin with. + +### D4 — Token-verify roundtrip count. + +Per the spec: pre-convoy a worst-case page mount issued **3** identical `GET /api/auth/verify` +requests: + +1. `` in `_app.js` calls `verifyToken()` on mount. +2. `pages/card/[id].js` calls `useAuth()` from `lib/use-auth.js`, which calls `checkAuth()` on + mount → another verify. +3. The same page calls `useIsAdmin()` from `lib/admin-auth.js`, which calls its inline + `checkAdmin()` on mount → another verify. + +Post-convoy: + +1. `` is gone. +2. `pages/card/[id].js` calls `useAuth()` once → 1 verify. +3. `useIsAdmin()` call site is gone; admin status is computed synchronously from the same + `user` returned by step 2. + +Net: **3 → 1** verify roundtrip on `card/[id].js` mount. Other pages drop from **2 → 1** +(no `useIsAdmin` involved, but `` was). The 1× pattern is the floor; further +reduction would require server-side hydration of the user object, which is a separate +architectural conversation (out of scope; see Follow-ups). + +### D5 — Test impact. Verdict: zero test files modified. + +The 21-test vitest suite covers: + +- `test/lib/auth-secret.test.js` (3) — server-side, untouched by this convoy. +- `test/lib/permission-middleware.test.js` (8) — server-side, untouched. +- `test/api/auth-utils.test.js` (5) — server-side, untouched. +- `test/components/Layout.test.js` (5) — passes `user` as a *prop*, not via any hook. The + legacy `auth-context` and `admin-auth` modules are not imported. Unaffected. + +All four files were `grep`-checked for `auth-context|admin-auth|use-auth` references — zero +hits. No test was written against the legacy hooks themselves; the deletion is risk-free from a +test-suite perspective. Vitest stays green at 21/21 post-convoy. + +## Importer inventory + +Generated via `rg "from ['\"].*lib/auth-context['\"]" --type js` and +`rg "from ['\"].*lib/admin-auth['\"]" --type js` against the worktree (excluding docs / convoys). + +### Importers of `lib/auth-context.js` (6 source files) + +| File | Symbol | Migration | +| --- | --- | --- | +| `pages/_app.js` | `AuthProvider` | Wrapper removed; no replacement (D3) | +| `pages/index.js` | `useAuth` | Path swap → `lib/use-auth.js` | +| `pages/scanner.js` | `useAuth` | Path swap → `lib/use-auth` | +| `pages/decks.js` | `useAuth` | Path swap → `lib/use-auth` | +| `pages/deck/[id].js` | `useAuth` | Path swap → `lib/use-auth` (depth `../../`) | +| `pages/deck-builder.js` | `useAuth` | Path swap → `lib/use-auth` | + +All 5 page-level `useAuth` consumers destructured only `{ user }` or `{ user, loading }` (verified +by grep). No `login` / `register` / other-method consumer found, confirming D1. + +### Importers of `lib/admin-auth.js` (1 source file) + +| File | Symbol | Migration | +| --- | --- | --- | +| `pages/card/[id].js` | `useIsAdmin` | Replaced with `user?.role === 'admin'` from existing `useAuth()` (D2) | + +`AdminProvider` and `useAdmin()` had **zero** importers in the source tree — confirming +they were dead exports. + +### Adjacent doc / config edits + +| File | Change | +| --- | --- | +| `pages/_app.js` | Removed `import { AuthProvider } from '../lib/auth-context.js'` and the wrapper | +| `.github/CODEOWNERS` | Removed the two CODEOWNERS lines for the deleted files | +| `AGENTS.md` § 2 + § 3 | Updated the Auth row of the architecture table and the "Auth (client)" convention bullet to describe the post-convoy single-surface state | +| `.cursor/rules/auth-and-permissions.mdc` | Reframed § "Legacy" to "deleted by this convoy"; updated § "Authentication state on the client" to the post-convoy `useAuth()` shape and the direct-fetch login flow | +| `.cursor/rules/no-go-zones.mdc` | Auth-refactors bullet updated to drop the deleted files | +| `.cursor/skills/add-page/SKILL.md` | Updated checklist bullet + anti-pattern row to refer to the deletion | + +`.convoys/**` and `.convoys/fix-layout-default-user/**` were **not** edited — those are +historical convoy records and are append-only by repo convention. The doc-writer post-convoy +sweep will add the as-shipped section at the bottom of this file plus update +`.convoys/ship-readiness.md` § P1 → entry 9 with the squash commit reference. + +## The fix (per-category translation rules) + +### Category A — `useAuth` from `auth-context` → `useAuth` from `use-auth` + +```js +// before +import { useAuth } from '../lib/auth-context'; // or auth-context.js +// after +import { useAuth } from '../lib/use-auth'; // or use-auth.js +``` + +The destructure pattern (`const { user } = useAuth()` / `const { user, loading } = useAuth()`) +stays byte-identical. No call-site changes. + +### Category B — `AuthProvider` wrapper in `_app.js` + +```jsx +// before +import { AuthProvider } from '../lib/auth-context.js'; +return ( + + + + + +); + +// after +return ( + + + +); +``` + +Plus delete the import line. + +### Category C — `useIsAdmin` in `pages/card/[id].js` + +See D2 for the full diff. Three line-ranges touched: the import block, the `useAuth` destructure, +and the `useIsAdmin` line block. Usage at line 524 is unchanged. + +### Category D — `useAdmin`, `AdminProvider` + +No call sites. No work to do; these symbols disappear when the file is deleted. + +## Verification plan + +1. **`rg "lib/auth-context|lib/admin-auth" --type js`** → expect zero hits in `pages/`, `lib/`, + `components/`. Achieved. +2. **`npm run lint`** → baseline 128 problems pre-convoy → 125 problems post-convoy (3 fewer + errors, since the deleted files contained 3 unused-import / unused-var lints; no new lint + surface introduced). No regression. +3. **`npm run test:run`** → 21/21 pass pre- and post-convoy. Layout test confirmed unaffected. +4. **`npm run build`** → succeeds end-to-end. All 26 pages compile (10 dynamic API routes + 16 + `pages/**` views including `card/[id]`, `_app`, `decks`, `deck/[id]`, `deck-builder`, `scanner`, + `index` — every file modified by the sweep). No SSR-level breakage; importantly no + "useAuth must be used within an AuthProvider" runtime error during static generation, which + would have indicated the page tried to use the legacy context hook unwrapped. +5. **Manual smoke:** _deferred_ — the build pass + vitest pass + zero-hit grep is the gate for + merging; the parent does not have a logged-in admin browser session ready in this + conversation. Documenting in As-shipped post-merge once the operator runs `npm run dev` and + exercises dashboard / profile / settings / collections / cards / admin/card-editor. + +## Risks + +- **R1 — Shape parity gap breaks runtime auth state.** *Mitigated by D1.* The grep audit + confirmed no consumer reads `login` / `register` / any other surface that exists on the + legacy hook but not on `use-auth`. `loading` and `user` were preserved with identical + semantics. +- **R2 — SSR mismatch from removing ``.** *Mitigated.* `lib/use-auth.js` reads + `localStorage` inside a `useEffect`, so SSR sees `user === null, loading === true` and never + touches the browser-only API on the server — same guarded shape as the legacy provider. + `npm run build` confirms no SSR error during static generation. (`auth-context.js`'s + `useEffect` had the same guard, so removing the provider didn't change the SSR surface.) +- **R3 — Missed importer.** *Mitigated.* Post-delete grep over `--type js` returned zero hits. + The deletion would itself surface any missed importer at module-load time during `npm run + build` (Node would throw "Cannot find module"); build succeeded. +- **R4 — Verify-roundtrip dedup creates a regression where a page never re-verifies.** + *Mitigated.* Pre-convoy, three providers each ran their own verify on mount but they did not + coordinate state — one provider's success had no effect on another's loading flag. Post-convoy + we have a single source of truth. Pages that need to re-verify (e.g. after an action that + might have invalidated the token) can call `refreshAuth()` from the same hook; no consumer + currently does this, but the surface is preserved for future use. +- **R5 — Stale `useAuth` cache across components.** *Out of scope; see Follow-ups.* Each + `useAuth()` call site instantiates its own state via `useState`. Two components on the same + page that both call `useAuth` will issue two verify roundtrips and hold two independent + `user` references. This was true pre-convoy too (the legacy `useIsAdmin` was already a + separate verify). Hoisting state into a shared module-level cache or wrapping `useAuth` in a + context (the very thing we just removed!) is a separate decision — see "Follow-ups". + +## As-shipped + +_Stub for doc-writer post-merge:_ + +- Squash commit: `` +- PR: #`` +- Files changed: 13 (2 deletions: `lib/auth-context.js`, `lib/admin-auth.js`; 11 modifications: + `pages/_app.js`, `pages/index.js`, `pages/scanner.js`, `pages/decks.js`, `pages/deck/[id].js`, + `pages/deck-builder.js`, `pages/card/[id].js`, `.github/CODEOWNERS`, `AGENTS.md`, + `.cursor/rules/auth-and-permissions.mdc`, `.cursor/rules/no-go-zones.mdc`, + `.cursor/skills/add-page/SKILL.md`). +- Verify roundtrip count: documented 3 → 1 on `card/[id].js`, 2 → 1 on every other page-load. +- `.convoys/ship-readiness.md` § P1 entry 9 to be marked RESOLVED with this convoy's squash SHA. +- Lint baseline updated 128 → 125 (no regression; 3 fewer errors from deleted unused-import + lines). + +## Follow-ups (out of scope here) + +- **Component-level `useAuth` cache audit.** Two components on the same page that both call + `useAuth()` will issue two verify roundtrips. This was the original motivation for the + legacy context, and was the *one* legitimate thing those providers did right. A future + convoy should consider either (a) returning a shared module-level state via a small + Zustand-style store, (b) reintroducing a thin `` that *only* hoists state + without re-implementing fetch logic, or (c) accepting the duplicate roundtrip as the price of + hook-only simplicity. Today's call sites already deduplicate at the page level (one + `useAuth` per page is the prevailing pattern), so this is a soft optimisation, not a + correctness fix. +- **Rate-limit-aware re-auth on 429.** `lib/use-auth.js`'s `checkAuth` does not currently + back off if `/api/auth/verify` returns 429 (the rate-limiter from + `add-rate-limiting` would only kick in if a single client exceeded + 60 verify calls / minute, which is unrealistic in practice but worth a defensive guard). +- **Server-side hydration of user.** The page-mount verify roundtrip is unavoidable in this + hook-only shape because the token is only readable on the client. Moving to an HTTP-only + cookie + Next.js `getServerSideProps` hydration would eliminate the round-trip entirely + and is a larger architectural conversation that should not piggyback on a quality convoy. +- **Doc-writer cleanup.** Update `.convoys/ship-readiness.md` § P1 → entry 9 with the + RESOLVED stamp + squash SHA; trim the "three parallel surfaces" framing from any other + doc that still mentions it; refresh the "Auth refactors" no-go-zones bullet if any other + files become canonical (none today). diff --git a/.cursor/rules/auth-and-permissions.mdc b/.cursor/rules/auth-and-permissions.mdc index 7308787..98f7bbf 100644 --- a/.cursor/rules/auth-and-permissions.mdc +++ b/.cursor/rules/auth-and-permissions.mdc @@ -21,12 +21,15 @@ There are three parallel client-side auth implementations and one server-side he | Client: route protection | `components/ProtectedRoute.js` | | Client: admin route protection | `components/AdminProtected.js` | -## Legacy (do not extend) +## Legacy (deleted by `single-auth-provider`) -- `lib/auth-context.js::AuthProvider` + `useAuth` — older context. Still wired in `pages/_app.js`; left in place for compatibility. Don't add new consumers. -- `lib/admin-auth.js::AdminProvider` + `useAdmin` + `useIsAdmin` — parallel admin context. Same story. - -A convoy is planned to collapse these three into one provider + one hook. +`lib/auth-context.js` and `lib/admin-auth.js` were the two parallel client-side +auth surfaces that lived alongside `lib/use-auth.js`. They were deleted by the +`single-auth-provider` convoy (P1). Do **not** reintroduce a `` +or `` wrapper in `pages/_app.js` — `useAuth()` from +`lib/use-auth.js` is hook-only (reads token from `localStorage` and hits +`/api/auth/verify` on mount) and does not require a context provider. The +`useIsAdmin` semantic is now `const { user } = useAuth(); const isAdmin = user?.role === 'admin'`. ## Token model @@ -48,7 +51,7 @@ When introducing a new permission tier, update both `checkRolePermission`'s hier ## Authentication state on the client -`useAuth()` returns `{ user, loading, login, logout, refresh }`. `user === null` means logged out; `loading === true` means token verification in flight. Always render against `loading === false` before deciding to redirect. +`useAuth()` returns `{ user, loading, logout, refreshAuth }`. `user === null` means logged out; `loading === true` means token verification in flight. Always render against `loading === false` before deciding to redirect. The login / register flows do **not** go through `useAuth` — `pages/login.js` and `pages/signup.js` `fetch` `/api/auth/{login,register}` directly and write the returned token to `localStorage`; `useAuth()` will pick it up on next mount via its `useEffect` → `/api/auth/verify` roundtrip (or call `refreshAuth()` to re-verify in place). ## Server-side authorization patterns diff --git a/.cursor/rules/no-go-zones.mdc b/.cursor/rules/no-go-zones.mdc index 3f47c84..028ae90 100644 --- a/.cursor/rules/no-go-zones.mdc +++ b/.cursor/rules/no-go-zones.mdc @@ -33,5 +33,5 @@ Do not edit, refactor, or quote as context examples. If you think you need to ch ## Editing rules of thumb - **Schema changes:** until a proper migration tool lands, document the change in a new dated script under `scripts/migrations/YYYY-MM-DD-.js` (folder TBD). Do NOT edit `scripts/setup-neon-db.js` in place for any **DDL change** (`CREATE TABLE`, `ALTER`, new columns, constraint changes) — it's idempotent and meant for first-time setup only. **Operational changes are allowed** (env-var gating, error-message hardening, module-system fixes) — `drop-public-setup` set this precedent by adding the `ADMIN_INITIAL_PASSWORD` gate and converting the script to ESM. The distinction: if the change touches DDL strings or `INSERT` semantics, file a migration; if it only touches Node-module behavior or pre-flight validation, edit in place and document why in the convoy. -- **Auth refactors:** `lib/permission-middleware.js`, `pages/api/auth-utils.js`, `lib/auth-context.js`, `lib/admin-auth.js`, and `lib/use-auth.js` form a deliberately documented mess. Tighten them inside a single convoy; don't cherry-pick. +- **Auth refactors:** `lib/permission-middleware.js`, `pages/api/auth-utils.js`, `lib/auth-secret.js`, and `lib/use-auth.js` are the four documented auth surfaces. Tighten them inside a single convoy; don't cherry-pick. (The legacy `lib/auth-context.js` and `lib/admin-auth.js` were deleted by `single-auth-provider`; do not resurrect them.) - **Card-import jobs:** `pages/api/cards/import-*.js` hit external APIs with rate limits. Don't run them ad-hoc against prod data; use staging. diff --git a/.cursor/skills/add-page/SKILL.md b/.cursor/skills/add-page/SKILL.md index 09f7ff9..0180773 100644 --- a/.cursor/skills/add-page/SKILL.md +++ b/.cursor/skills/add-page/SKILL.md @@ -93,7 +93,7 @@ Use Tailwind for layout, spacing, sizing, hover/focus states. Use CSS vars (inli ## Step 6: Check - [ ] Auth wrapper chosen (ProtectedRoute / AdminProtected / public). -- [ ] `useAuth()` from `lib/use-auth.js` (not the legacy `lib/auth-context.js`). +- [ ] `useAuth()` from `lib/use-auth.js` (the only client auth hook; `lib/auth-context.js` and `lib/admin-auth.js` were deleted by the `single-auth-provider` convoy). - [ ] `user` passed to Layout explicitly. - [ ] Colors come from theme tokens, not hex. - [ ] All interactive elements have `aria-label` or visible text. @@ -105,5 +105,5 @@ Use Tailwind for layout, spacing, sizing, hover/focus states. Use CSS vars (inli | --- | --- | | Hardcode hex colors | Use CSS variables | | Default `user = { … }` to a real email | Default to `null` | -| Pull from `lib/auth-context` for new code | Use `lib/use-auth` | +| Reintroduce `lib/auth-context` or `lib/admin-auth` (deleted) | Use `lib/use-auth` | | Render Layout twice on the same page | Single `` at the top | diff --git a/.github/CODEOWNERS b/.github/CODEOWNERS index 66fc49b..354055a 100644 --- a/.github/CODEOWNERS +++ b/.github/CODEOWNERS @@ -8,8 +8,6 @@ pages/api/auth/** @YOUR-GITHUB-HANDLE pages/api/auth-utils.js @YOUR-GITHUB-HANDLE lib/permission-middleware.js @YOUR-GITHUB-HANDLE -lib/auth-context.js @YOUR-GITHUB-HANDLE -lib/admin-auth.js @YOUR-GITHUB-HANDLE lib/use-auth.js @YOUR-GITHUB-HANDLE components/ProtectedRoute.js @YOUR-GITHUB-HANDLE components/AdminProtected.js @YOUR-GITHUB-HANDLE diff --git a/AGENTS.md b/AGENTS.md index a5132ac..0777ef3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -29,7 +29,7 @@ A web app for managing trading-card-game collections (Magic, Pokémon, Lorcana). | Pages router views | `pages/*.js` | Public + auth views; uses `components/Layout.js` | | API routes | `pages/api/**/*.js` | Express-style `handler(req, res)`. **30+ handlers depend on `lib/permission-middleware.js::getUserFromRequest`** | | Shared UI | `components/*.js` | `Layout`, `CardItem`, `CameraScanner`, modal family | -| Auth + DB libs | `lib/*.js` | `auth-context`, `admin-auth`, `use-auth` (three parallel auth surfaces), `database`, `permission-middleware` | +| Auth + DB libs | `lib/*.js` | `use-auth` (canonical client hook — sole surface post-`single-auth-provider`), `database`, `permission-middleware` | | Migration scripts | `scripts/*.js` | 27+ one-off "add column" / "seed" scripts. No formal migration tool | | Card-import jobs | `pages/api/cards/import-*.js`, `scripts/import-*.js` | Scryfall / Lorcana / Pokémon TCG APIs | | Database schema | `scripts/setup-neon-db.js` | Bootstrap SQL DDL — the source of truth until a real migration tool lands | @@ -40,7 +40,7 @@ Code graph is indexed by `user-code-review-graph` MCP (122 files, 628 nodes, 560 ## 3. Key conventions - **Auth (server):** `import { getUserFromRequest } from '../../lib/permission-middleware'` → returns `{ userId, email, role }` or `null`. `null` means "send 401" — always early-return when the user is null before doing any work that depends on their identity. -- **Auth (client):** `import { useAuth } from '../lib/use-auth'`. Avoid `lib/auth-context.js` and `lib/admin-auth.js` for new code — they are legacy parallel implementations. +- **Auth (client):** `import { useAuth } from '../lib/use-auth'`. Returns `{ user, loading, logout, refreshAuth }`; `user === null` means logged out, `loading === true` means token verification in flight. There is no client-side admin hook — compute `const isAdmin = user?.role === 'admin'` from the same `useAuth()` call. The legacy `lib/auth-context.js` and `lib/admin-auth.js` were deleted by the `single-auth-provider` convoy; do not reintroduce a `` / `` wrapper in `pages/_app.js`. - **Layout `user` prop:** pages should pass `user` from `useAuth()` to ``. Layout's default is `null` and renders a logged-out "Sign in" CTA when no user is supplied — both paths are valid (some surfaces like `pages/invite/{accept,decline}.js` legitimately render Layout for anonymous visitors). Do not reintroduce a hardcoded user object as a default prop. - **JWT secret + TTL:** `import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js'`. This is the only place either value is defined; do not reintroduce literal fallbacks. `JWT_TOKEN_TTL = '24h'` is canonical. - **Auth helper (token mint / verify / password hash):** `import { ... } from '../../pages/api/auth-utils'` (`generateToken`, `verifyToken`, `hashPassword`, `verifyPassword`). Reads the secret + TTL from `lib/auth-secret.js` under the hood. diff --git a/lib/admin-auth.js b/lib/admin-auth.js deleted file mode 100644 index 4a90b65..0000000 --- a/lib/admin-auth.js +++ /dev/null @@ -1,120 +0,0 @@ -import { createContext, useContext, useState, useEffect } from 'react'; - -// Create admin context -const AdminContext = createContext(); - -export function AdminProvider({ children }) { - const [user, setUser] = useState(null); - const [loading, setLoading] = useState(true); - - useEffect(() => { - checkAdminAuth(); - }, []); - - const checkAdminAuth = async () => { - try { - // Get token from localStorage - const token = localStorage.getItem('auth_token'); - - const headers = { - 'Content-Type': 'application/json', - }; - - // Add authorization header if token exists - if (token) { - headers.Authorization = `Bearer ${token}`; - } - - const response = await fetch('/api/auth/verify', { headers }); - if (response.ok) { - const userData = await response.json(); - setUser(userData); - } else { - setUser(null); - // Clear invalid token - if (token) { - localStorage.removeItem('auth_token'); - } - } - } catch (error) { - console.error('Auth check failed:', error); - setUser(null); - } finally { - setLoading(false); - } - }; - - const isAdmin = () => { - return user && user.role === 'admin'; - }; - - const isAuthenticated = () => { - return user !== null; - }; - - const value = { - user, - loading, - isAdmin, - isAuthenticated, - checkAdminAuth - }; - - return ( - - {children} - - ); -} - -export function useAdmin() { - const context = useContext(AdminContext); - if (!context) { - throw new Error('useAdmin must be used within an AdminProvider'); - } - return context; -} - -// Simple hook for checking admin status without context -export function useIsAdmin() { - const [isAdmin, setIsAdmin] = useState(false); - const [loading, setLoading] = useState(true); - - useEffect(() => { - const checkAdmin = async () => { - try { - // Get token from localStorage - const token = localStorage.getItem('auth_token'); - - const headers = { - 'Content-Type': 'application/json', - }; - - // Add authorization header if token exists - if (token) { - headers.Authorization = `Bearer ${token}`; - } - - const response = await fetch('/api/auth/verify', { headers }); - if (response.ok) { - const userData = await response.json(); - setIsAdmin(userData.role === 'admin'); - } else { - setIsAdmin(false); - // Clear invalid token - if (token) { - localStorage.removeItem('auth_token'); - } - } - } catch (error) { - setIsAdmin(false); - } finally { - setLoading(false); - } - }; - - checkAdmin(); - }, []); - - return { isAdmin, loading }; -} \ No newline at end of file diff --git a/lib/auth-context.js b/lib/auth-context.js deleted file mode 100644 index 3b7e8b5..0000000 --- a/lib/auth-context.js +++ /dev/null @@ -1,116 +0,0 @@ -import { createContext, useContext, useState, useEffect } from 'react'; - -const AuthContext = createContext(); - -export function AuthProvider({ children }) { - const [user, setUser] = useState(null); - const [loading, setLoading] = useState(true); - - useEffect(() => { - // Check for existing token on app load - const token = localStorage.getItem('auth_token'); - if (token) { - // Verify token and set user - verifyToken(token); - } else { - setLoading(false); - } - }, []); - - const verifyToken = async (token) => { - try { - const response = await fetch('/api/auth/verify', { - headers: { - 'Authorization': `Bearer ${token}` - } - }); - - if (response.ok) { - const userData = await response.json(); - setUser(userData); // API returns user data directly, not wrapped in .user - } else { - localStorage.removeItem('auth_token'); - } - } catch (error) { - console.error('Token verification failed:', error); - localStorage.removeItem('auth_token'); - } finally { - setLoading(false); - } - }; - - const login = async (email, password) => { - try { - const response = await fetch('/api/auth/login', { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ email, password }), - }); - - const data = await response.json(); - - if (response.ok) { - localStorage.setItem('auth_token', data.token); - setUser(data.user); - return { success: true }; - } else { - return { success: false, error: data.error }; - } - } catch (error) { - return { success: false, error: 'Network error' }; - } - }; - - const register = async (email, password) => { - try { - const response = await fetch('/api/auth/register', { - method: 'POST', - headers: { - 'Content-Type': 'application/json', - }, - body: JSON.stringify({ email, password }), - }); - - const data = await response.json(); - - if (response.ok) { - localStorage.setItem('auth_token', data.token); - setUser(data.user); - return { success: true }; - } else { - return { success: false, error: data.error }; - } - } catch (error) { - return { success: false, error: 'Network error' }; - } - }; - - const logout = () => { - localStorage.removeItem('auth_token'); - setUser(null); - }; - - const value = { - user, - loading, - login, - register, - logout, - }; - - return ( - - {children} - - ); -} - -export function useAuth() { - const context = useContext(AuthContext); - if (!context) { - throw new Error('useAuth must be used within an AuthProvider'); - } - return context; -} \ No newline at end of file diff --git a/pages/_app.js b/pages/_app.js index 671cf63..d85b52f 100644 --- a/pages/_app.js +++ b/pages/_app.js @@ -1,13 +1,10 @@ import '../styles/globals.css'; -import { AuthProvider } from '../lib/auth-context.js'; import { ThemeProvider } from '../lib/theme-context.js'; export default function App({ Component, pageProps }) { return ( - - - + ); -} \ No newline at end of file +} diff --git a/pages/card/[id].js b/pages/card/[id].js index e7d92e3..8de8bc8 100644 --- a/pages/card/[id].js +++ b/pages/card/[id].js @@ -1,7 +1,6 @@ import { useState, useEffect } from 'react'; import { useRouter } from 'next/router'; import Layout from '../../components/Layout'; -import { useIsAdmin } from '../../lib/admin-auth'; import { useAuth } from '../../lib/use-auth'; import CollectionSelectionModal from '../../components/CollectionSelectionModal'; import { ManaCost, ColorIdentity, AdvancedManaCost } from '../../components/ManaSymbols'; @@ -10,7 +9,7 @@ import ManaSymbolSettings from '../../components/ManaSymbolSettings'; export default function CardDetail() { const router = useRouter(); const { id } = router.query; - const { user } = useAuth(); + const { user, loading: authLoading } = useAuth(); const [card, setCard] = useState(null); const [loading, setLoading] = useState(true); @@ -31,8 +30,8 @@ export default function CardDetail() { // Mana symbol settings const [manaSymbolSettings, setManaSymbolSettings] = useState({ useSVG: false }); - // Check admin status - const { isAdmin, loading: adminLoading } = useIsAdmin(); + const isAdmin = user?.role === 'admin'; + const adminLoading = authLoading; // Fetch card data from API useEffect(() => { diff --git a/pages/deck-builder.js b/pages/deck-builder.js index 02328ab..d4e2ec7 100644 --- a/pages/deck-builder.js +++ b/pages/deck-builder.js @@ -4,7 +4,7 @@ import Link from 'next/link'; import Layout from '../components/Layout'; import { ManaCost, ColorIdentity, ColorFilterSymbol } from '../components/ManaSymbols'; import ManaSymbolSettings from '../components/ManaSymbolSettings'; -import { useAuth } from '../lib/auth-context'; +import { useAuth } from '../lib/use-auth'; import { getColorIdentity, getColorSymbol } from '../lib/mana-symbols'; export default function DeckBuilder() { diff --git a/pages/deck/[id].js b/pages/deck/[id].js index 2c975b3..ef7d0bb 100644 --- a/pages/deck/[id].js +++ b/pages/deck/[id].js @@ -3,7 +3,7 @@ import { useRouter } from 'next/router'; import Link from 'next/link'; import Layout from '../../components/Layout'; import { ManaCost, ColorIdentity } from '../../components/ManaSymbols'; -import { useAuth } from '../../lib/auth-context'; +import { useAuth } from '../../lib/use-auth'; import { getColorIdentity } from '../../lib/mana-symbols'; export default function DeckDetail() { diff --git a/pages/decks.js b/pages/decks.js index 33aedf4..e52b615 100644 --- a/pages/decks.js +++ b/pages/decks.js @@ -2,7 +2,7 @@ import { useState, useEffect } from 'react'; import { useRouter } from 'next/router'; import Link from 'next/link'; import Layout from '../components/Layout'; -import { useAuth } from '../lib/auth-context'; +import { useAuth } from '../lib/use-auth'; export default function Decks() { const { user } = useAuth(); diff --git a/pages/index.js b/pages/index.js index 5d93fd3..03a6724 100644 --- a/pages/index.js +++ b/pages/index.js @@ -1,7 +1,7 @@ import { useState, useEffect } from 'react'; import { useRouter } from 'next/router'; import Link from 'next/link'; -import { useAuth } from '../lib/auth-context.js'; +import { useAuth } from '../lib/use-auth.js'; import AnimatedFireLogo from '../components/AnimatedFireLogo'; export default function Home() { diff --git a/pages/scanner.js b/pages/scanner.js index 4fa22b2..345a816 100644 --- a/pages/scanner.js +++ b/pages/scanner.js @@ -5,7 +5,7 @@ import CameraScanner from '../components/CameraScanner'; import OCRSettings from '../components/OCRSettings'; import { ManaCost, ColorIdentity } from '../components/ManaSymbols'; import ManaSymbolSettings from '../components/ManaSymbolSettings'; -import { useAuth } from '../lib/auth-context'; +import { useAuth } from '../lib/use-auth'; export default function Scanner() { const { user } = useAuth();