Reflects the merged pick-a-name convoy (PR #21, squash commit9abbab6) in repo documentation. **This is the first post-P0 cleanup** — add-rate-limiting (squash708ef45) closed the LAST P0 ship-blocker on 2026-05-24, and pick-a-name immediately follows as the first post-P0 P1 quality-bar convoy, closing P1 #12 (brand-consistency — the inconsistency `AGENTS.md` line 5 had flagged since project setup). Two file-disjoint briefs landed serially (B1 commitac8c998display + comment sweep, B2 commit1c18d21infrastructure + email migration); five canonical-string D-decisions ratified verbatim at gate-1 plus Risk 4 PRESERVE on the negative regression-lock literal. Architect commit 50ce9ab; no scope expansions and no implementer deviations from the verbatim spec beyond the parallel-vs-serial dispatch choice (parallelizable in theory; serial in practice for lockfile-regen + migration-script audit cleanliness). .convoys/pick-a-name.md: - frontmatter Status: in-progress -> shipped (added Shipped: 2026-05-24 line, matching the file's existing **Status:** / **Opened:** markdown-line shape rather than YAML frontmatter). - new ## As-shipped section replacing the placeholder "(To be appended post-merge by role-doc-writer.)" stub. Opens with the post-P0 transition language pointing at ship-readiness's "8 of 8 RESOLVED" baseline + this convoy's role as the first P1 quality-bar closure. Decisions section captures all five D-ratifications verbatim (D1 Deck Hearth two-word internal-cap; D2 deck-hearth kebab matching the existing CSS class + Node convention; D3 deckhearth single-token for IDs/Redis; D4 admin@deckhearth.com placeholder TLD pre-domain-acquisition, swept across admin/alice/bob uniformly; D5 full deckhearth Redis prefix not abbreviated dh, with the architect's self-documenting-prefix-vs-memory-savings rationale preserved) plus the Risk 4 PRESERVE on test/lib/permission-middleware.test.js line 87's negative regression-lock literal — including the project-pattern justification (Gotchas #2/#3/#4/#5/#8 in AGENTS.md all preserve historical-shape locks for the same audit-trail reason). As-shipped surface broken into B1/B2 split with file-disjoint inventory + the serial-in-practice dispatch note (the conductor's parallel-via-multitask path stayed open as fallback). As-shipped diff captures B1 (7+/7-) + B2 (133+/30-) + lockfile STOP-on-churn confirmation (only the two `name` lines diffed; npm ci clean against regen). Empirical CI metrics from post-merge run 26387909625 (Aggregate gate 10s, Lint 45s 128 baseline, forbidden-endpoints + forbidden-cors-headers 4s each, Playwright smoke 1m4s 3/3, Screenshot diff 1m5s continue-on-error swallow per Decision 4, vitest 21/21 in 35s with Risk 4 PRESERVE test green). Cross-validation finding: 4th convoy in a row where the same 3-test smoke spec defends the auth surface through a sweeping change (PR #15 Layout -> #19 CORS -> #20 rate-limit -> #21 brand-rename). Implementer-vs-architect calibration note on the B2 forecast paragraph vs § Full surface inventory — both correct, cross-reference between them could be tighter for the next architect. Operator-action-required-going-forward: run the new migration script before the next admin login attempt (idempotent + UNIQUE-collision-safe). What did NOT change audit trail (~25 lines of historical .convoys/ tcgvault references stay byte-identical per no-go-zones, plus seed-collections-*.js historical scripts with stale WHERE-clauses that now no-op harmlessly post-migration, plus repo-name references deferred to rename-repo-and-vercel-project, plus the docs/agent-context + add-api-route SKILL.md repo-name references that sweep with the same downstream convoy). .convoys/ship-readiness.md: - § Status summary: P1-quality-bar-work-in-progress line added below the existing 8/8 RESOLVED milestone footer, citing pick-a-name as the first such convoy. - § P0 launch sequence step 5 (pick-a-name): marked RESOLVED 2026-05-24 with the squash commit + brief shape + smoke metrics + operator-action callout inline, mirroring the format of step 4 (add-rate-limiting RESOLVED) introduced by the previous doc-writer pass. - § Queued convoys: four new entries surfaced by pick-a-name inserted as a logical group after purge-weak-creds-from-helpers (the natural neighbor — all brand-bound + credential-bound post-pick-a-name follow-ups): - rename-repo-and-vercel-project (P2 polish; GitHub repo + Vercel project rename; auto-redirects make low-urgency; lists the 8 architect-verified literal-repo references that sweep together; Risk 5 .agent-context-manifest.yml source-tag re-evaluation noted). - point-domain-at-deckhearth (P2 polish; blocked on domain acquisition; DNS pointer + Vercel domain claim + admin email TLD swap if non-`.com`; current placeholder-email rationale preserved — auth uses email as identity not as mail target). - regenerate-brand-assets (P2 polish; favicon, OG images, social cards; requires design pass; cross-references the original Role-design-system-auditor finding about unifying DH logo + AnimatedFireLogo). - convert-reset-db-to-esm (P2 hygiene; CJS-in-ESM bug at scripts/reset-db.js:142; fold-into-purge-weak-creds-from-helpers noted since both convoys touch the same file). - seed-visual-baselines-on-linux entry: extended with the AFTER-pick-a-name ordering note (now satisfied —9abbab6merged 2026-05-24); first Linux baseline will capture Deck Hearth strings not pre-rename TCG Vault strings. - purge-weak-creds-from-helpers entry: scope narrowed — the @tcgvault.com email-half is now satisfied by pick-a-name B2; remaining scope is the admin123/alice123/bob123 weak password literals only (which survived pick-a-name deliberately per Brief 2's scope boundary). Fold-into-convert-reset-db-to-esm suggested (overlapping file). AGENTS.md: - line 5 (top-of-file branding note): rewritten in full from the B1-installed transitional shape ("Deck Hearth is the canonical product brand (ratified 2026-05-24...)") to the post-merge documented shape with the squash commit + PR # citation + the Vercel-project parenthetical + the Risk 4 PRESERVE caveat citing test/lib/permission-middleware.test.js. **This is the showstopper change** — it's the line that's been flagged as inconsistent since project setup; the rewrite finally closes the brand-consistency loop end-to-end. - line 13 (§ 1 Project overview Auth bullet): stale `admin@tcgvault.com` reference updated to `admin@deckhearth.com` to match the post-B2 seed state. Per Brief 2's "Files explicitly out of scope" line: "AGENTS.md line 13 = doc-writer scope". - Gotcha #4 (Default admin credentials in the seed): extended with a new paragraph below the existing operator-caveat copy documenting the post-pick-a-name email rename + the migration script + the migration-FIRST-then-setup-db ordering rule + the UNIQUE-collision-safe failure mode. Existing drop-public-setup operator-caveat copy preserved verbatim above the new content. - Gotcha #12 (Rate-limit env vars): per-class limiter table extended with a Redis-prefix column showing the renamed `deckhearth:*` values (was implicit/missing); one-liner about the rename + accepted counter reset added immediately below the table. Original env-var contract paragraph (KV_REST_API_URL / KV_REST_API_TOKEN, fail-closed-in-prod / warn-and-noop-in-dev) preserved verbatim above the new table. The defensive THROW + gate-ordering-rule + sixth-class-shape paragraphs below stay byte-identical from the add-rate-limiting milestone doc-writer pass. .cursor/rules/api-routes.mdc: - § Rate limiting opening lineage line: extended to add the pick-a-name attribution (squash9abbab6, PR #21, 2026-05-24) and to note the rename is namespace-only — call shape, return shape, and gate-ordering rules below are byte-identical post-rename. The convoy-attribution lineage now reads: fix-auth-bypass Brief 4 (auth class) -> add-rate-limiting (the other four classes + 7 routes) -> pick-a-name (Redis prefix rename). No example code blocks in this rule had `tcgvault:` literals to swap (verified via re-read); the per-class table earlier in the rule doesn't enumerate prefix values either, so the lineage line is the only edit needed. Files explicitly NOT changed: package.json, package-lock.json, lib/rate-limit.js, lib/**, pages/**, components/**, scripts/**, test/**, tests/**, .github/workflows/**, README.md, TESTING_GUIDE.md, playwright.config.js, eslint.config.mjs, scripts/migrations/2026-05-24-rename-admin-email.js. Doc-writer pass touches docs only. The four queued downstream convoys (rename-repo-and-vercel-project, point-domain-at-deckhearth, regenerate-brand-assets, convert-reset-db-to-esm) own the remaining brand-bound + helper-bound source-code work; this commit only records them in the queued-convoys ledger. Co-authored-by: Cursor <cursoragent@cursor.com>
424 lines
70 KiB
Markdown
424 lines
70 KiB
Markdown
---
|
||
name: ship-readiness
|
||
classification: epic
|
||
success_metric: tcg-vault is safe to expose to anonymous internet traffic with a documented launch checklist green
|
||
skip: []
|
||
status: open
|
||
created: 2026-05-22
|
||
---
|
||
|
||
# Ship-readiness convoy
|
||
|
||
Umbrella convoy capturing the full agent-pipeline review of tcg-vault as of 2026-05-22. Findings are grouped by L2 role lens (Reviewer / Architect / Design-system / A11y / IA / Doc-writer) and severity. Each item points to the convoy that will execute the fix.
|
||
|
||
Code graph: 122 files, 628 nodes, 5602 edges, 11 communities. Indexed by `user-code-review-graph` MCP.
|
||
|
||
## Status summary (as of 2026-05-24)
|
||
|
||
**P0 ship-blockers: 8 of 8 RESOLVED. Launch-readiness P0 checklist is empty.**
|
||
|
||
| Item | Status | Convoy |
|
||
| --- | --- | --- |
|
||
| P0 #1 — `getUserFromRequest` hardcoded admin | **RESOLVED** 2026-05-23 | `fix-auth-bypass` Brief 2 (`258e479`) |
|
||
| P0 #2 — `JWT_SECRET` hardcoded fallback | **RESOLVED** 2026-05-23 | `fix-auth-bypass` Brief 1 (`4a10dce`) |
|
||
| P0 #3 — Default admin credentials in seed | **RESOLVED** 2026-05-23 | `drop-public-setup` (`ff80753` + `b63b509`) |
|
||
| P0 #4 — Dev-only test endpoints | **RESOLVED** 2026-05-23 | `fix-auth-bypass` Brief 3 (`fc0dd73`) |
|
||
| P0 #5 — Wildcard CORS on API surface | **RESOLVED** 2026-05-24 | `fix-auth-bypass` Brief 4 (`297afca`) + `cors-tighten` (`da50d78`) |
|
||
| P0 #6 — No rate limiting | **RESOLVED** 2026-05-24 | `fix-auth-bypass` Brief 4 (`297afca`, login + register) + `add-rate-limiting` (`708ef45`, the remaining surface) |
|
||
| P0 #7 — Layout default-prop leaks email | **RESOLVED** 2026-05-24 | `fix-layout-default-user` (`ca302a8`) |
|
||
| P0 #8 — Next.js 15.4.3 vulnerable version | **RESOLVED** 2026-05-23 | `bump-next-js` (`e57ea17`) |
|
||
|
||
**Milestone reached 2026-05-24:** `add-rate-limiting` (PR #20, squash
|
||
commit `708ef45`) closed P0 #6 — the last open P0 — flipping the
|
||
ship-blocker set from 7/8 to **8/8 RESOLVED**. The security gate is
|
||
closed; remaining launch work is P1 quality bar (single SQL client,
|
||
single auth provider, lint baseline cleanup, brand decision, test
|
||
coverage expansion) plus the P2 / P3 polish lanes in this file's
|
||
Queued convoys section. None of those are P0 ship-blockers.
|
||
|
||
**P1 quality-bar work in progress** (pick-a-name shipped 2026-05-24,
|
||
PR #21 squash `9abbab6`, closing P1 #12 brand-consistency — the
|
||
inconsistency `AGENTS.md` line 5 had flagged since project setup;
|
||
single-auth-provider, single-sql-client, migration-tool queued).
|
||
|
||
## P0 — ship-blockers (security)
|
||
|
||
These MUST land before any anonymous traffic touches the production URL.
|
||
|
||
### 1. `getUserFromRequest` returns a hardcoded admin when no Bearer token is present — **RESOLVED 2026-05-23**
|
||
|
||
- **Resolved by:** `fix-auth-bypass` Brief 2, commit `258e479` (PR #8). Follow-up Brief 6 hotfix `1fca3aa` added explicit 401 guards to the cards-collection POST/PUT/DELETE branches that previously masked the bug as 500s.
|
||
- **File:** `lib/permission-middleware.js` lines 13-17.
|
||
- **Impact:** Every API route that calls `getUserFromRequest` (30+ handlers — see `user-code-review-graph` cross-community edges from `api-handler` → `lib-admin`) accepts unauthenticated requests as admin user 1.
|
||
- **Repro:** `curl https://<host>/api/collections` with no `Authorization` header returns admin's collections.
|
||
- **Fix:** Delete lines 13-17. Return `null` when no Bearer token. Update every caller to handle `null` properly (most already do; the broken fallback was masking the right path).
|
||
- **As-shipped:** The helper now returns `null` for any unauthenticated request. 16 unit tests in `test/lib/permission-middleware.test.js` lock in the contract (including a negative regression against the old synthetic-admin shape). `pages/api/auth/verify.js` returns 401 on the no-token branch instead of fetching the seed admin row.
|
||
- **Owns:** `role-architect` + `role-implementer` (one PR; small surface area in the helper, callers already check `!user`).
|
||
|
||
### 2. JWT_SECRET hardcoded fallback in 7 files — **RESOLVED 2026-05-23**
|
||
|
||
- **Resolved by:** `fix-auth-bypass` Brief 1, commit `4a10dce` (PR #7).
|
||
- **Files:**
|
||
- `pages/api/auth-utils.js` (`'your-secret-key'`)
|
||
- `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/auth/verify.js`
|
||
- `pages/api/favorites.js`, `pages/api/users/search.js`
|
||
- `lib/permission-middleware.js`
|
||
- **Impact:** If `JWT_SECRET` env var is unset (e.g. preview/staging misconfig), tokens are signed with `'your-secret-key-change-in-production'` — an attacker can sign their own admin token in 5 seconds.
|
||
- **Fix:** Centralize JWT_SECRET access in one helper that `throw`s at module load if `process.env.JWT_SECRET` is unset. Every other file imports from there.
|
||
- **Bonus:** Token expiry is inconsistent (`/api/auth/login.js` uses 24h, `pages/api/auth-utils.js` uses 7d). Pick one.
|
||
- **As-shipped:** `lib/auth-secret.js` is the single source of truth and throws at module load if `JWT_SECRET` is unset. Canonical TTL is `JWT_TOKEN_TTL = '24h'`. All 7 literal fallback sites are converted to import-and-throw. `test/lib/auth-secret.test.js` (3 tests) covers the fail-loud path.
|
||
- **Owns:** `role-architect` + `role-implementer`.
|
||
|
||
### 3. Default admin credentials in seed + README — **RESOLVED 2026-05-23**
|
||
|
||
- **Resolved by:** `drop-public-setup` Brief 1 (commit `ff80753`) + Brief 2 (commit `b63b509`). PR #13.
|
||
- **Files:**
|
||
- `scripts/setup-neon-db.js` lines 130-138 — creates `admin@tcgvault.com` / `admin123`
|
||
- `README.md` documents the credentials
|
||
- `pages/api/setup-database.js` — duplicates the setup AND is an UNAUTHENTICATED public POST endpoint with `Access-Control-Allow-Origin: *`
|
||
- **Impact:** Anyone who hits `/api/setup-database` can re-trigger DDL. The `admin123` password is one Google away from public knowledge.
|
||
- **Fix:**
|
||
1. Delete `pages/api/setup-database.js`. Schema setup is a one-time job; it should not be a route.
|
||
2. Change `setup-neon-db.js` to require a `ADMIN_INITIAL_PASSWORD` env var (no default).
|
||
3. Strip the admin password from README — replace with "run `npm run setup-db` and follow the prompt".
|
||
- **As-shipped:**
|
||
1. `pages/api/setup-database.js` already deleted by `fix-auth-bypass` Brief 3 (commit `fc0dd73`); the `forbidden-endpoints` CI job blocks re-introduction.
|
||
2. `scripts/setup-neon-db.js` now reads `ADMIN_INITIAL_PASSWORD` from `process.env`; if unset or empty, the script writes an actionable error (names the env var, points at `.env.local`, suggests `openssl rand -base64 24`, mentions CI-secret alternative, references README) and exits with code 1 **before** opening any DB connection. The bcrypt input is the env-var value, not the literal `admin123`. The two `console.log` lines that previously echoed `Admin User: admin@tcgvault.com` + `Admin Password: admin123` are deleted (R3 — stdout-leak prevention into CI logs); replaced with a single `Admin user ready (email: admin@tcgvault.com)` line that does NOT echo the password.
|
||
3. `README.md`'s "Default Admin Account" section replaced with "First-time admin setup" copy that documents the env-var requirement, the `openssl rand -base64 24` generation tip, the CI-secret alternative, and an operator-rotation note for envs that pre-date this convoy.
|
||
4. **Bonus (Decision D, Brief 2):** `scripts/setup-neon-db.js` converted from CommonJS to ESM so `npm run setup-db` actually executes on Node 22.x. The `bump-next-js` convoy added `"type": "module"` to `package.json` for ESLint v9 flat config; the seed script's `require()` calls were silently broken since that landed. Without Brief 2, Brief 1's env-var gate would have been theatrical (script throws `ReferenceError` before reaching the gate).
|
||
- **Operator caveat (R1, Decision A — going-forward only):** the seed is idempotent (`ON CONFLICT (email) DO NOTHING`); re-running `npm run setup-db` on an env that already has the admin row does NOT rotate the password. Any deployed env that ran setup before this convoy still has the weak `admin123` hash in its DB — operators must rotate manually via the app's profile settings, or wait for the queued `rotate-default-admin` follow-up convoy. Documented in `AGENTS.md` Gotcha #4 and the README's First-time admin setup blockquote.
|
||
- **Sibling weak-cred references deferred:** `scripts/reset-db.js`, `scripts/create-test-users.js`, and `TESTING_GUIDE.md` still hardcode `admin@tcgvault.com` / `admin123` — out of scope here per the no-go-zones rule (historical scripts) and the convoy spec. Queued for `purge-weak-creds-from-helpers` follow-up (or fold into `pick-a-name` since the email is also changing).
|
||
- **Owns:** `role-implementer`.
|
||
|
||
### 4. Dev-only test endpoints shipped to production — **RESOLVED 2026-05-23**
|
||
|
||
- **Resolved by:** `fix-auth-bypass` Brief 3, commit `fc0dd73` (PR #6).
|
||
- **Files:** `pages/api/simple.js`, `pages/api/test-auth.js`, `pages/api/test-db.js`, `pages/api/setup-database.js`.
|
||
- **Impact:** Unknown — depends on what they expose. `/api/test-db` likely returns the DB connection string; `/api/test-auth` may leak token-handling details.
|
||
- **Fix:** Delete all four. Add a CI grep that fails the build if any file matching `pages/api/(test-|simple|setup-)*.js` exists.
|
||
- **As-shipped:** All four files deleted. `.github/workflows/ci.yml` has a new `forbidden-endpoints` job (blocking) that fails the build if any of the four paths reappear OR if a new `pages/api/test-*.js` file is added. Local simulation in the implementer PR confirmed clean → OK, with `test-fake.js` → FAIL, post-cleanup → OK.
|
||
- **Owns:** `role-implementer`.
|
||
|
||
### 5. CORS `Access-Control-Allow-Origin: *` on auth endpoints — **RESOLVED 2026-05-24**
|
||
|
||
- **Resolved by:** `fix-auth-bypass` Brief 4, commit `297afca` (PR #9, login + register) + `cors-tighten`, squash commit `da50d78` (PR #19, the remaining 24 handlers + CI regression-lock).
|
||
- **Files:** at minimum `pages/api/auth/login.js`, `pages/api/auth/register.js`, `pages/api/setup-database.js` (verify others).
|
||
- **Impact:** Any origin can submit credentials. Combined with the no-rate-limit problem below, credential stuffing is wide open.
|
||
- **Fix:** Set `Access-Control-Allow-Origin` to the literal frontend origin (`https://tcgvault.com` / preview domain), or remove the header entirely if the API and the frontend are same-origin (they are, on Vercel).
|
||
- **As-shipped (Brief 4, 2026-05-23):** `pages/api/auth/login.js` and `pages/api/auth/register.js` dropped the four `setHeader` calls + the OPTIONS preflight handler. `pages/api/setup-database.js` was deleted entirely by Brief 3.
|
||
- **As-shipped (`cors-tighten`, 2026-05-24, squash commit `da50d78`, PR #19, architect-commit `ec22b70`, implementer-commit `a843736`):**
|
||
1. **24 `pages/api/**` handlers swept** — `admin/index.js`, `auth/verify.js`, `cards/[id]/ownership.js`, `cards/owned.js`, `cards/search.js`, `collections.js`, `collections/[identifier].js`, `collections/[identifier]/activity.js`, `collections/[identifier]/cards.js`, `collections/[identifier]/permissions.js`, `collections/[identifier]/thumbnails.js`, `community/collections.js`, `favorites.js`, `invite/accept.js`, `invite/decline.js`, `public/collections.js`, `user/avatar.js`, `user/avatar/generate.js`, `user/delete.js`, `user/password.js`, `user/profile.js`, `user/settings.js`, `user/stats.js`, `users/search.js`. Each diff is a pure deletion of 9-11 lines (the leading `// Set CORS headers` comment + 3 `setHeader` calls + the leading `// Handle preflight requests` comment + the 4-line OPTIONS-if block + the trailing blank line). No additions per source file. **Pattern split: 16 Pattern A (top-level method gate after the CORS block) + 8 Pattern B (method-branched inside the `try` block).** Both shapes documented verbatim in `.convoys/cors-tighten/brief-1-sweep-wildcard-cors.md`.
|
||
2. **New blocking `forbidden-cors-headers` CI job** in `.github/workflows/ci.yml`, modeled verbatim on the existing `forbidden-endpoints` job (added by `fix-auth-bypass` Brief 3). Greps `pages/api/` for `Access-Control-Allow-(Origin|Methods|Headers)`, emits `::error file= line=::` annotations on hit, exits 1. No `continue-on-error`, no `|| true` wrapper. Sits between `forbidden-endpoints` and `test` in the YAML for logical grouping (both `forbidden-*` checks are static-source guards before the runtime test job). Runs in ~4 seconds; zero new dependencies.
|
||
3. **All five architect decisions self-ratified at gate 1** (no operator decisions needed) — D1 Option B (expanded sweep, all 24 files), D2 delete the OPTIONS preflight handler entirely (Option (a)), D3 `verify.js` `Allow-Methods` tightening moot (subsumed by D2), D4 no new per-route handler tests in this convoy (deferred to queued `fill-vitest-handler-coverage`), D5 add the new CI regression-lock job.
|
||
4. **Diff: 25 files, +29 / -261** (pure deletion across 24 source files; 29 additions = the new CI job).
|
||
- **As-shipped metrics (post-merge run 26378806555 + subsequent runs):**
|
||
- `forbidden-cors-headers` (new) — PASS in **4s**. First live exercise of the regression-lock; greps clean against the post-sweep tree.
|
||
- `Playwright smoke` — PASS in **56s, 3/3 tests in 3.3s** against the post-CORS-removal Vercel preview. Cross-validates that CORS removal is safe for the auth surface (smoke's sign-in check still passes against `/login`; `/api/health` still serves anonymously). Surfaced as a real CI signal even though Decision D4 deferred per-route handler tests — the existing smoke spec transitively defends the auth + public surfaces against this convoy's deletions.
|
||
- `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 (Decision-4 end state of `adopt-playwright-smoke`). 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` (see § Queued convoys).
|
||
- All other gates (`Lint`, `Vitest`, `Schema map up to date`, `forbidden-endpoints`) — green.
|
||
- **Implementer subagent-retry footnote (transient).** The implementer's PR report flagged that HEAD was already at the implementer commit (`a843736`) when its retry subagent woke up — a prior implementer run had completed the work, and the retry's "STOP per branch mismatch" rule kicked in; the retry then ran verification only (lint baseline, vitest 21/21, grep clean, YAML valid) and reported success. This is a transient subagent retry, not a process gap. The implementer commit `a843736` is canonical; the squash `da50d78` rolls up the architect plan + Brief 1 + the implementer's work without duplication.
|
||
- **Operator action required going forward:** **none.** No env vars to seed, no secrets to rotate, no infra changes. The `forbidden-cors-headers` job is self-contained (plain bash grep on the runner); future PRs that accidentally re-scaffold a wildcard CORS header will fail the build with a file-and-line pointer to the offending line.
|
||
- **Owns:** `role-implementer`.
|
||
|
||
### 6. No rate limiting anywhere — **RESOLVED 2026-05-24** *(milestone — last P0 closed)*
|
||
|
||
- **Resolved by:** `fix-auth-bypass` Brief 4, commit `297afca` (PR #9, login + register only) + `add-rate-limiting`, squash commit `708ef45` (PR #20, the remaining surface + 3-import-route gating + 1 atomic admin UI fix + 1 rule extension).
|
||
- **Impact:** Login endpoint accepts unlimited attempts; card-search endpoint can be hammered; image upload endpoints can be exhausted. The `pages/api/cards/import-*.js` endpoints externally hit Scryfall/Pokémon APIs with no caller throttling.
|
||
- **Fix:** Adopt `@upstash/ratelimit` (free tier covers a small launch) or Vercel's built-in middleware-based rate limiting. Apply to: `/api/auth/login`, `/api/auth/register`, `/api/users/search`, `/api/cards/search`, all `/api/cards/import-*`, and `/api/user/avatar*` (upload).
|
||
- **As-shipped (Brief 4, 2026-05-23):** `lib/rate-limit.js` (new) provides `checkAuthRateLimit(req)` via `@upstash/ratelimit@^2.0.8` + `@upstash/redis@^1.38.0` (5 attempts / 15-min sliding window per IP). Wired into login + register. Env vars are `KV_REST_API_URL` / `KV_REST_API_TOKEN` (auto-provisioned by Vercel's Upstash Marketplace integration — note this is a rename from the brief's original `UPSTASH_REDIS_REST_*` spec; see `.convoys/fix-auth-bypass/brief-4-tighten-auth-surface.md` § Post-merge addendum). Fails closed in prod when env vars are unset; warn-and-no-ops in dev. Search / import / avatar endpoints were unchanged at that point (the deferred surface that `add-rate-limiting` then closed).
|
||
- **As-shipped (`add-rate-limiting`, 2026-05-24, squash commit `708ef45`, PR #20, architect-commit `60b842e`, implementer-commit `51a3a97`):**
|
||
1. **`lib/rate-limit.js` refactored** from a single auth-only `Ratelimit` instance into a `Map<className, Ratelimit>` cache with one shared Redis client and **5 `Ratelimit` instances** (one per class, distinct Redis prefix). Five exported functions: `checkAuthRateLimit(req)` (Brief 4 contract preserved byte-identical), `checkSearchRateLimit(req)`, `checkUploadRateLimit(req, userId)`, `checkGenerateRateLimit(req, userId)`, `checkImportRateLimit(req, userId)`. Internal `check(className, identifier)` shared helper. `LIMITER_CONFIG` is a top-level `const` map of `{ limit, window, prefix }` per class; adding a sixth class is a one-line addition + one new exported function.
|
||
2. **6 routes newly gated** with the appropriate per-class limiter at the correct ordering (auth before user-keyed limiter; method check first):
|
||
|
||
| Limiter | Limit/window | Key | Routes |
|
||
| --- | --- | --- | --- |
|
||
| `checkAuthRateLimit` (unchanged from Brief 4) | 5 / 15 min | IP | `auth/login.js`, `auth/register.js` |
|
||
| `checkSearchRateLimit` (new) | 60 / 1 min | IP | `users/search.js`, `cards/search.js` |
|
||
| `checkUploadRateLimit` (new) | 10 / 1 hour | user | `user/avatar.js` |
|
||
| `checkGenerateRateLimit` (new) | 5 / 1 hour | user | `user/avatar/generate.js` |
|
||
| `checkImportRateLimit` (new) | 5 / 1 hour | user (admin-only) | `cards/import-mtg.js`, `cards/import-pokemon.js`, `cards/import-lorcana.js` |
|
||
|
||
3. **`extractUserIdentifier(userId)` THROWS** on `null` / `undefined` / `''` / `NaN` (Decision 4 defensive shape). Surfaces gate-ordering bugs at dev time rather than silently falling back to IP and converting a per-user limit into a per-IP limit — which would lock other household members out for one user's behavior. Numeric `0` is intentionally accepted (returns `'user:0'`) for forward-compat.
|
||
4. **Three `pages/api/cards/import-*.js` routes newly auth-gated.** Each grew `getUserFromRequest` + `if (user.role !== 'admin') return 403` + `checkImportRateLimit(req, user.userId)` before the existing `try` block. Closes the publicly-callable anonymous-abuse vector the architect's pre-brief audit flagged (each handler hits Scryfall / Pokémon-TCG / Lorcana APIs and performs UPSERTs into `cards` with no caller throttling pre-convoy). Lorcana was gated defensively despite zero current frontend callers — see § Queued convoys for the `delete-dead-lorcana-import` cleanup follow-up.
|
||
5. **Atomic admin UI fix in `pages/admin/card-import.js`.** Added `'Authorization': \`Bearer ${localStorage.getItem('auth_token')}\`` to the import fetch's headers (one-line addition). **This was the architect's critical pre-brief discovery and the reason Decision 1 routed back to the operator** — gating the import APIs without this matching client fetch fix would have closed P0 #6 but introduced an immediate 401 on every "Import Cards" click, producing a visible UX regression on the only live admin tooling that exercises the gated routes. Shipping the API gate + the client fix in the same atomic PR is what made Decision 1 Option A viable.
|
||
6. **`.cursor/rules/api-routes.mdc` § Rate limiting extended** with the per-class table + verbatim call shape + gate-ordering rules (method check first; auth before any user-keyed limiter; admin-role check goes between auth and rate-limit for the import routes) + identifier-extraction documentation + uniform 429 response shape + fail-closed env-var contract + fail-open Upstash-outage behavior. Doc-writer pass verified the implementer's extension is complete; no further touch-ups needed.
|
||
7. **All six architect decisions ratified at gate 1.** D1 operator-ratified (Option A — gate all three import routes plus the atomic admin UI fix); D2-D6 architect-self-ratified per the precedent established by `cors-tighten` D2-D5 and `fix-vercel-deployment-protection-in-ci` A/B/D (hybrid named-limiter shape; per-class limit values with tuning evidence; two-extractor shape with defensive THROW; uniform 429 message; no new vitest / playwright specs in this convoy).
|
||
8. **Diff: 12 files, +1612 / -23 in the squash.** The 1612-addition figure is dominated by the architect's convoy + brief files (676 + 751 lines) which the squash includes because the architect commit preceded the implementer commit on the same branch. Actual source-file diff is much smaller: `lib/rate-limit.js` +90/-23 (lib refactor); `.cursor/rules/api-routes.mdc` +41 (rule extension); 6 route files +69 total (3× +16 for import routes, 4× +7 for search/avatar/generate/users-search); `pages/admin/card-import.js` +1 (Bearer-header addition).
|
||
- **As-shipped metrics (post-merge run 26382185019 + subsequent runs):**
|
||
- `Playwright smoke` — PASS in **59s, 3/3 tests in 3.8s** against the post-rate-limit Vercel preview (home redirects ✓ 431ms / sign-in page renders ✓ 331ms / `/api/health` ✓ 193ms). **Critical cross-validation:** smoke calls `/api/health` once per run (well below the new search class's 60/min ceiling), and the home + sign-in routes don't touch any of the 6 newly-gated endpoints — so the new search limiter does NOT 429 the smoke spec. The cross-validation lineage now accumulates across three convoys: smoke test 2 still passes against post-PR-#15 Layout default-user + post-PR-#19 CORS-tighten + post-PR-#20 rate-limiting — the same 3-test spec has defended the auth surface through three sweeping changes without anyone writing a dedicated test.
|
||
- `forbidden-cors-headers` (from `cors-tighten`) — PASS. The convoy is purely additive of rate-limit gate code; no CORS headers were reintroduced.
|
||
- `forbidden-endpoints` (from `fix-auth-bypass` Brief 3) — PASS. No new `pages/api/test-*.js` or deleted-endpoint shapes reintroduced.
|
||
- `Unit tests (vitest)` — PASS, **21/21 in 27s**. Decision 6 (no new vitest specs) verified at architect time (`rg 'rate-limit|@upstash' test/` returns zero matches; the existing 21 specs don't transitively import `lib/rate-limit.js`, so the lib refactor was strictly safer than the convoy file's stale § Known constraints implied).
|
||
- `Lint` — 128 problems (baseline preserved, no regression). Zero new lint problems from the lib refactor, the 6 route edits, or the admin UI one-liner.
|
||
- `Screenshot diff` — `continue-on-error: true` swallow per `adopt-playwright-smoke` Decision 4 (no baseline committed yet). Triggered on PR #20 because the `paths:` filter `pages/**` matches the 6 route edits under `pages/api/`; same minor false-positive as PR #19, tracked by the queued `tighten-visual-diff-path-filter` follow-up. Not a regression.
|
||
- All other gates (`Schema map up to date`, `Aggregate gate`) — green.
|
||
- **Operator action required going forward:** **none.** All Upstash env vars (`KV_REST_API_URL` / `KV_REST_API_TOKEN`) were already auto-provisioned via the Vercel Marketplace integration for Brief 4. No new secrets, no infra changes, no CI gates to enable. The fail-loud-in-prod predicate in `lib/rate-limit.js::init()` is self-defending: if a future deploy unsets either env var, every gated route fails closed on the first call (`throw new Error('[rate-limit] Upstash not configured...')`). If a follow-up tuning need surfaces (search 60/min too tight, generate 5/hour too tight), the fix is a single-line `LIMITER_CONFIG` edit; surface as `tune-search-rate-limit` or `tiered-rate-limits` only if real users 429.
|
||
- **Owns:** `role-architect` (pattern + Decision 1 routing) → `role-implementer` (per-route).
|
||
|
||
### 7. Layout default-prop leaks maintainer email — **RESOLVED 2026-05-24**
|
||
|
||
- **Resolved by:** `fix-layout-default-user` convoy (PR #15, squash commit `ca302a8`). Brief 1 (pre-squash `ddf8fd2`) shipped the Layout default-null + logged-out branch + vitest lock-in; Brief 2 (pre-squash `8c7d127`, rebased to `0f6bfbb` pre-merge) swept the 7 pages that needed page-level fixes.
|
||
- **File:** `components/Layout.js` line 562: `function Layout({ children, user = { email: 'me@randallstillwell.com', role: 'user' }, ... })`.
|
||
- **Impact:** Any page that renders Layout without passing a `user` prop displays your real email and impersonates you as the logged-in user.
|
||
- **Fix:** Default `user = null` and render a logged-out state branch. Verify every page passes `user` explicitly (the graph shows ~13 pages call `Layout`; audit each).
|
||
- **As-shipped:**
|
||
1. `components/Layout.js` default prop changed from hardcoded `{ email: 'me@randallstillwell.com', role: 'user' }` to `null`. `UserProfileDropdown` now branches on `user === null` and renders a `<Link href="/login">Sign in</Link>` CTA in place of the avatar + email + dropdown menu (`NavigationContent`'s `authenticatedNavigation` / `myCollectionNavigation` / `adminNavigation` were already null-safe via existing optional chains; no change there).
|
||
2. **7 pages swept** (Brief 2, 11 `<Layout>` call sites total). `pages/scanner.js` (×1), `pages/decks.js` (×3), `pages/deck-builder.js` (×4), `pages/deck/[id].js` (×3) now pass `user={user}` explicitly. `pages/profile.js` and `pages/settings.js` replaced their leaky `useState({ email: 'me@randallstillwell.com', role: 'admin' })` initializer with `useState(null)` (15 sync `user.*` reads in profile + 1 in settings got null-guards). `pages/card/[id].js` replaced its hardcoded `const user = { email: 'me@…', role: 'user' }` with `const { user } = useAuth()` from `lib/use-auth.js`.
|
||
3. **10 pages already correct** (architect's per-page audit, Decision B in `.convoys/fix-layout-default-user.md`): `dashboard`, `my-cards`, `cards`, `collections`, `collection/[identifier]`, `community/collections`, `admin/card-import`, `admin/card-editor`, `invite/accept`, `invite/decline`. No changes there.
|
||
4. **Test coverage:** `test/components/Layout.test.js` (new) adds 5 regression-lock assertions — no maintainer email when `user` is `null`/omitted; "Sign in" link present when logged out; supplied email renders when supplied; no accidental `Guest` placeholder. Vitest 21/21 green at merge (16 pre-existing auth tests still green).
|
||
5. **New devDeps:** `jsdom@^29` + `@testing-library/react@^16` (test-only). `vitest.config.js` got a 3-line `esbuild` block to parse JSX in `.js` files (per-file `// @vitest-environment jsdom` directive — no global env change).
|
||
6. **Verification at merge:** `rg 'me@randallstillwell.com' pages/` → 0 hits; anonymous `curl /cards` returned HTTP 200 with no maintainer email; lint baseline match (128 problems, unchanged); CI Aggregate gate / Lint / Vitest / Vercel preview / forbidden-endpoints all green. `Playwright smoke` + `Screenshot diff` red but for an unrelated CI-infra reason — see CI infrastructure side-effect note below.
|
||
- **Flagged-but-deferred** (deliberately out of scope per the convoy spec):
|
||
1. 4 pages still import `useAuth` from `lib/auth-context.js` (`pages/scanner.js`, `pages/decks.js`, `pages/deck-builder.js`, `pages/deck/[id].js`) — collapsing the three parallel client-side auth surfaces is the queued `single-auth-provider` convoy (P1 #9 in this file), not this one.
|
||
2. `components/MobileNavigation.js` still receives a dead `user` prop (it accepts `{ user, onMenuOpen }` but never reads `user.*` — the bottom-bar items are static). Queued as `cleanup-mobile-nav-dead-props` (or fold into `god-component-split` if that lands first).
|
||
3. `pages/card/[id].js` still imports `useIsAdmin` from `lib/admin-auth.js` — third parallel auth surface; same `single-auth-provider` convoy will collapse it.
|
||
- **CI infrastructure side-effect (not part of this convoy).** PR #16 (squash commit `7e97254`) landed alongside as a CI permissions fix, adding scoped `permissions:` blocks to `.github/workflows/preview-smoke.yml` + `.github/workflows/visual-diff.yml`. That fixed the 5-second 403 "Resource not accessible by integration" failure on both workflows but exposed a second issue: with permissions correct, both now reach the actual deployment check and 10-min-timeout against Vercel Deployment Protection's 401 SSO challenge (anonymous GitHub runner GETs the preview URL). New queued convoy `fix-vercel-deployment-protection-in-ci` (`.convoys/fix-vercel-deployment-protection-in-ci.md`) tracks that follow-up.
|
||
- **Owns:** `role-implementer`.
|
||
|
||
### 8. Next.js 15.4.3 — Vercel platform blocks deploys (vulnerable version) — **RESOLVED 2026-05-23**
|
||
|
||
- **Resolved by:** `bump-next-js` convoy, single-brief PR commit `e57ea17` ("bump: next 15.4.3 -> 16.2.6, ESLint flat config (v9 fallback), typescript devDep"). The Vercel platform gate cleared with the first successful deploy on the same date; every subsequent PR (`fix-auth-bypass`, `drop-public-setup`, `fix-layout-default-user`, the CI permissions fix) has had a green Vercel preview.
|
||
- **Discovered:** 2026-05-22 during the bootstrap PR CI run. Vercel build completes successfully (~29s) but the deployment exits with status `Error` and `"Vulnerable version of Next.js detected, please update immediately"`.
|
||
- **Files:** `package.json` line 22 (`"next": "^15.4.2"` → locked at `15.4.3`), `package-lock.json`.
|
||
- **Impact:** **Vercel will not deploy any branch — including `main` — until Next.js is bumped.** Preview URLs are unavailable, which means `preview-smoke.yml` and `visual-diff.yml` can't fire. The last successful deploy on `main` was 2025-08-01; production may already be running an outdated build.
|
||
- **CVE context:** Next.js shipped a middleware auth-bypass advisory (CVE-2025-29927) patched in 15.2.3, plus subsequent advisories. The exact CVE Vercel is flagging on 15.4.3 needs confirmation via `npm audit` and the Next.js security advisory page.
|
||
- **Fix:** Bump `next` to the latest secure 15.x (`npm install next@^15.5` and run smoke tests) OR the latest 16.x (`next@^16.2.6` — major bump; review breaking changes in [Next.js 16 release notes](https://nextjs.org/blog/next-16)).
|
||
- **As-shipped (Decision A in `.convoys/bump-next-js.md` — leapfrog to 16):**
|
||
1. `next`: `^15.4.2` → `^16.2.6` (resolves to `16.2.6`).
|
||
2. `eslint-config-next`: `15.4.2` → `^16.2.6`. Config migrated from `.eslintrc.json` to `eslint.config.mjs` (eslint-config-next@16 is flat-config-only).
|
||
3. `eslint`: `^8` → `^9.39.4` (Decision D fallback — v10 surfaced Risk R15 empirically because `@typescript-eslint/scope-manager@8.59.4` bundled by `eslint-config-next@16` doesn't implement v10's new `addGlobals` API; v10 adoption deferred to a separate `bump-eslint-10` convoy, upstream-blocked on typescript-eslint).
|
||
4. `typescript`: newly added at `^5.9.3` as a devDep (Decision C — required by the typescript-eslint chain regardless of ESLint major; no project source migration to TS).
|
||
5. `scripts.lint`: `"next lint"` → `"eslint ."` (next lint removed in 16). Lint baseline grew from ~100 to **128 problems** (81 errors, 47 warnings) due to `eslint-plugin-react-hooks@7.1.1` + `@next/eslint-plugin-next@16.2.6` rule additions; CI tolerates this via the `|| true` wrapper in `.github/workflows/ci.yml` per P1 #11.5 (`fix-lint-baseline`).
|
||
6. `next.config.js`: `images.domains` → `images.remotePatterns` (deprecated and removed in 16; preserves Scryfall, Pokémon TCG, Lorcana API hosts for eventual `next/image` adoption).
|
||
7. Verification at merge: `npm install` clean (no ERESOLVE), `npm run build` exit 0 with Turbopack (~1.4s compile, 23 static pages + 47 API routes), first green Vercel deploy on `main` since 2025-08-01.
|
||
- **Side-effects (deliberately deferred, not part of this convoy):**
|
||
- `bump-react` (React 18 → 19) — held until 18.x EOL or until a feature needs it.
|
||
- App Router migration — multi-month effort; queued indefinitely.
|
||
- `adopt-vitest` ✅ shipped as `fix-auth-bypass` Brief 5; `adopt-playwright-smoke` partially shipped via the Vercel-bound workflows (CI infra now blocked by `fix-vercel-deployment-protection-in-ci`).
|
||
- `fix-lint-baseline` (P1 #11.5) — drop the CI `|| true` wrapper once the 128-problem baseline is cleared.
|
||
- `bump-eslint-10` + `bump-typescript-6` — upstream-blocked on typescript-eslint shipping v10-tested releases.
|
||
- **Doc drift note:** this resolution was applied as part of the `fix-layout-default-user` post-convoy cleanup (commit reflecting `b7ddd08`'s sibling) — the `bump-next-js` convoy never ran a dedicated doc-writer pass, so this RESOLVED entry was added ~24h after the fix actually shipped.
|
||
- **Owns:** `role-architect` (pick target version + assess breaking changes) → `role-implementer` (bump + verify dev/build/start + smoke).
|
||
- **Convoy:** `bump-next-js` — ran before `fix-auth-bypass`. **Without this convoy, every L3 gate that depends on a Vercel preview was non-functional.**
|
||
|
||
## P1 — pre-launch quality bar
|
||
|
||
### 8. Two SQL clients in parallel (`@neondatabase/serverless` + `@vercel/postgres`)
|
||
|
||
- **Impact:** Two different param-handling APIs, two different transaction stories, two different connection-pool stories. Plus `lib/database.js`'s manual interpolation + `sql.unsafe(query)` is a SQL-injection vector if any caller passes user input through.
|
||
- **Fix:** Pick `@vercel/postgres` (tagged-template, no injection vector). Migrate every call site of `lib/database.js::db.query`. Delete `lib/database.js`.
|
||
- **Reviewer/Architect call:** small enough to fit in one convoy; touches ~3 files based on graph.
|
||
|
||
### 9. Three parallel client-side auth implementations
|
||
|
||
- **Files:** `lib/auth-context.js` (`AuthProvider` / `useAuth`), `lib/admin-auth.js` (`AdminProvider` / `useAdmin` / `useIsAdmin`), `lib/use-auth.js` (`useAuth`).
|
||
- **Impact:** Pages randomly import from one of three places. State is duplicated. Logout in one provider doesn't necessarily clear the others. Token-verify roundtrips happen 3× on initial page load if all three providers mount.
|
||
- **Fix:** Collapse to `lib/use-auth.js` as the canonical hook. Migrate every importer. Delete `auth-context.js` and `admin-auth.js`. Roll up `useIsAdmin` semantics into `useAuth().user?.role === 'admin'`.
|
||
- **Owns:** `role-architect` (decision) → `role-implementer` (per-page migration; ~30 importers).
|
||
|
||
### 10. No tests
|
||
|
||
- **Impact:** The first agent-driven refactor of `getUserFromRequest` (P0 #1) is high-blast-radius with no safety net.
|
||
- **Fix sequence:**
|
||
1. Install `vitest`. Add `npm run test:run` script. **RESOLVED** by `fix-auth-bypass` Brief 5, commit `1629afb`.
|
||
2. Install `@playwright/test`. Wire up `tests/smoke/app.smoke.spec.ts` (already drafted; needs `playwright.config.ts`). **RESOLVED 2026-05-24** by `adopt-playwright-smoke`, PR #18 squash `7b6f751` — 3/3 smoke tests pass in 2.9s, full workflow 59s, zero secret leaks. See § Queued convoys and `.convoys/adopt-playwright-smoke.md` § As-shipped.
|
||
3. Re-enable the `test:` job in `.github/workflows/ci.yml` (commented out at install time). Next remaining step in this fix sequence.
|
||
4. Add unit tests for `lib/permission-middleware.js`, `lib/slug-utils.js`, `pages/api/auth-utils.js`.
|
||
5. Wire `preview-smoke.yml` to run against the Vercel preview URL. **RESOLVED 2026-05-24** by `fix-vercel-deployment-protection-in-ci` (PR #17, `9a3e077`) + `adopt-playwright-smoke` (PR #18, `7b6f751`).
|
||
- **Owns:** `role-architect` (test strategy) → `role-implementer` (initial suite).
|
||
|
||
### 11. No migration tool — `scripts/add-*.js` graveyard
|
||
|
||
- **Files:** 27 scripts in `scripts/` of the form `add-foo-column.js`, `fix-bar-constraint.js`, `seed-baz.js`. No idempotency tracking, no `schema_migrations` table, no rollback.
|
||
- **Impact:** Onboarding a new env requires re-running every script in the right order. No way to know what's been run on a given Neon branch. Every new column is at risk of being missed in prod.
|
||
- **Fix:** Adopt `node-pg-migrate` (lightweight, matches the existing pattern best) OR migrate to `drizzle-kit` if the team wants schema-as-code. Backfill a single "initial" migration matching current prod schema. From there, every new column ships as a migration file.
|
||
- **Owns:** `role-architect` (tool selection) → `role-implementer` (backfill + first new migration).
|
||
|
||
### 11.5. Codebase has ~100 pre-existing ESLint errors
|
||
|
||
- **Discovered:** 2026-05-22 during the bootstrap PR. The repo had `"lint": "next lint"` in `package.json` but no `.eslintrc.json` — meaning lint was never run. Bootstrap added the config; lint now surfaces ~100 errors.
|
||
- **Most serious:** `react-hooks/rules-of-hooks` violations (hooks called conditionally) in several components. These are **real bugs** — React's hook ordering is undefined when hooks are called after early returns. They likely manifest as state-loss / stale-closure bugs in edge cases.
|
||
- **Less serious:** `react/no-unescaped-entities` (cosmetic), `react-hooks/exhaustive-deps` (warnings about missing useEffect deps), `@next/next/no-img-element` (cosmetic).
|
||
- **Impact:** The L3 CI lint job is currently `continue-on-error: true` (see `.github/workflows/ci.yml`) so it doesn't block PRs. Lint output is visible in logs but PRs merge regardless of lint state until this is cleaned up.
|
||
- **Fix:** Triage each error. The rules-of-hooks ones need genuine code restructuring (move hooks before any early returns). The unescaped-entities are mechanical (`'` → `'`). After cleanup, remove `continue-on-error: true`.
|
||
- **Convoy:** `fix-lint-baseline` — run after `fix-auth-bypass` and `drop-public-setup`. Multitask-safe: split into briefs by file group.
|
||
- **Owns:** `role-architect` (group strategy) → `role-implementer` (per-group fan-out).
|
||
|
||
### 12. Branding mismatch — "TCG Vault" vs. "Deck Hearth"
|
||
|
||
- **Files:** README, `package.json`, seed data say "TCG Vault" / `admin@tcgvault.com`. `components/Layout.js` lines 596 + 689 render "Deck Hearth" + "DH" logo. The `.env.local` template, `vercel.json`, and Vercel project name should also be audited.
|
||
- **Impact:** Confusing for users. Confusing for marketing. Confusing for analytics. Pick one.
|
||
- **Fix:** Brand workshop → final name → global replace → update README, package.json `"name"`, every UI string, Vercel project name, email sender, support pages. Schedule a redirect from the old domain.
|
||
- **Owns:** `role-ia-architect` (which name? — needs human decision) → `role-implementer`.
|
||
|
||
## P2 — refactor priorities
|
||
|
||
### 13. God components (10 files over 500 lines)
|
||
|
||
| File | Lines | Notes |
|
||
| --- | --- | --- |
|
||
| `pages/cards.js` | 1499 | `AuthenticatedCards` (886) + `Card3D` (502) live in one file. Split into `pages/cards/index.js` + `components/Card3D.js`. |
|
||
| `pages/collection/[identifier].js` | 1044 | `CollectionView` is one mega-component. Extract: header, card-grid, share-modal-wrapper, edit-form. |
|
||
| `pages/collections.js` | 989 | Similar structure to collection/[identifier]. Possibly share extracted pieces. |
|
||
| `pages/card/[id].js` | 913 | `CardDetail` — split into header, owned-badge, add-to-collection-flow. |
|
||
| `pages/deck-builder.js` | 823 | `DeckBuilder` — extract card-search, deck-list, mana-curve panels. |
|
||
| `components/CameraScanner.js` | 817 | Camera + AI-OCR + detection-loop — extract the detection loop into a hook. |
|
||
| `pages/admin/card-editor.js` | 778 | Form heavy. Use a `useFormState` pattern + separate the search-results subview. |
|
||
| `pages/scanner.js` | 776 | Mirror of CameraScanner concerns plus queue management. |
|
||
| `pages/settings.js` | 669 | One screen per settings section is the usual fix. |
|
||
| `pages/profile.js` | 625 | Avatar generation logic alone is ~150 lines — extract `useGeneratedAvatar` hook. |
|
||
|
||
Each is one convoy of its own. Use the architect role's `slice_dependencies:` to fan out implementers safely.
|
||
|
||
### 14. Schema-design smells (documented in `docs/SCHEMA_MAP.md`)
|
||
|
||
- `users` has two avatar columns (`profile_image_url` + `avatar_url`). Reconcile.
|
||
- `collections` has two visibility flags (`is_public BOOLEAN` + `visibility VARCHAR`). Reconcile.
|
||
- `cards.quantity` + `cards.favorited` are unused (they belong on `user_cards` / `user_favorites`). Drop.
|
||
- `user_settings` table duplicates several `users` columns. Reconcile.
|
||
- All enum-shaped VARCHARs (`role`, `condition`, `theme`, `game`, `visibility`) should be CHECK-constrained or proper Postgres ENUMs.
|
||
- `collections.tags` is `TEXT` (comma-separated). Migrate to `JSONB` or a join table.
|
||
|
||
### 15. Component coupling warning from graph
|
||
|
||
`user-code-review-graph` flagged:
|
||
- High coupling (44 edges) between `components-handle` and `pages-handle` (largely `Layout`, `CardItem`, `ManaCost` — expected for a shared UI surface).
|
||
- High coupling (34 edges) between `lib-admin` and `api-handler` — almost all via `getUserFromRequest`. After P0 #1 is fixed, this number stays high because the auth check is genuinely shared — that's fine.
|
||
|
||
### 16. Lots of inline SVG and emoji
|
||
|
||
The `getIcon` registry in `Layout.js` and `MobileNavigation.js` redefines the same SVG paths. Extract to `components/icons/` with named exports. Then audit the codebase for inline SVG that should be a named import. Bonus: lazy-load the larger icon families.
|
||
|
||
## P3 — UX, IA, design-system
|
||
|
||
### Role-ia-architect findings
|
||
|
||
- **URL structure** — solid. `/cards`, `/collections`, `/collection/[slug]`, `/deck-builder`, `/community/collections`. Coherent. One quirk: `/card/[id]` (singular) for detail vs. `/cards` (plural) for index — typical Next.js shape but worth a redirect rule so `/cards/[id]` also resolves.
|
||
- **Logged-out homepage** — current `pages/index.js` is 316 lines; needs an editorial pass. What's the value prop in one sentence? Right now it's mostly "we have cards".
|
||
- **Onboarding** — signup → profile setup → first collection → scan-or-import card. Currently each step is a separate page. Consider a multi-step wizard at `/onboarding` to keep the new user in flow.
|
||
- **Discoverability** — `/community/decks` and `/community/forums` are in the nav but flagged as placeholders. Either ship the MVP for each before launch (forums likely too big) or hide the nav items until they exist.
|
||
|
||
### Role-ux-reviewer findings
|
||
|
||
- **Loading states** — most data fetches set `loading: true` then re-render; very few show skeletons. Card grids should use shimmer placeholders; modals should disable submit while in flight.
|
||
- **Error states** — error messages bubble to `console.error` and toast nothing. Add a global toast system (e.g. `sonner`) and wire every catch block.
|
||
- **Empty states** — `/my-cards` and `/collections` when empty drop to "no cards yet". Replace with first-time CTA: "Scan your first card" or "Browse popular sets".
|
||
- **Mobile drawer** — `MobileNavigation` is solid (recent commit `442e906`). One thing: the bottom-bar's active state contrast looks low in light mode; verify against AA.
|
||
- **Camera scanner UX** — 817 lines of detection loop. Add a one-line "scanning…" status under the viewfinder and a single "captured N cards" badge. The current toolbar is busy.
|
||
|
||
### Role-design-system-auditor findings
|
||
|
||
- **Two visual languages mixing** — Tailwind classes AND CSS variables on the same elements. This is documented in `.cursor/rules/ui-and-theming.mdc`; the cleanup is to define which property goes where and enforce.
|
||
- **Hardcoded hex colors** — grep for `bg-\[#` and `style={{ backgroundColor: '#`. There are still a handful; convert to theme tokens.
|
||
- **Logo + brand** — see P1 #12. Then once the name is settled, the "DH" logo + AnimatedFireLogo need to be unified into one brand mark.
|
||
- **Modal patterns** — `CollectionSelectionModal`, `ShareModal`, `UploadImageModal` each have their own backdrop + focus-trap implementation. Extract `<Modal>` primitive. Use `headlessui` or `radix-ui`'s Dialog to get focus management for free.
|
||
- **Card grid spacing + density** — `pages/cards.js` (the 1499-line monster) does responsive grid math inline. Extract a `<CardGrid>` component that handles density (compact / comfortable / spacious) + sort + filter chrome.
|
||
|
||
### Role-a11y-auditor findings
|
||
|
||
- **Focus traps in modals** — none of the modals trap focus. Tab through `ShareModal` and you leave to the background. Critical for keyboard users + screen readers.
|
||
- **ESC to close modals** — inconsistent. Some have it, some don't.
|
||
- **Skip-to-content** — no `<a href="#main" class="sr-only focus:not-sr-only">`. Add to `_app.js`.
|
||
- **Image alts** — card images use `alt={card.name}` (good); avatar images sometimes have empty alts. Audit.
|
||
- **Color contrast** — verify the muted text colors (`var(--text-secondary)`) hit AA on both themes. The mobile bottom-bar inactive state is a likely fail.
|
||
- **Form errors** — login/signup form errors are visually red but not connected to inputs via `aria-describedby`. Screen readers don't know which field failed.
|
||
- **Keyboard ops on non-button elements** — most clickable `<div>`s already have `onKeyDown` but a few don't (audit with `rg "onClick" components pages | rg -v "<button"`).
|
||
|
||
### Role-doc-writer findings
|
||
|
||
- **README** — needs a public-facing rewrite. Currently mixes user docs + dev setup + admin credentials. Split into `README.md` (project landing) + `docs/DEVELOPMENT.md` (dev setup) + delete the admin credentials section entirely.
|
||
- **`docs/SCHEMA_MAP.md`** — installed at bootstrap (this convoy). Keep it fresh on every schema change.
|
||
- **CHANGELOG** — none yet. Adopt Keep-a-Changelog format. Backfill `[0.1.0] — initial private alpha` covering everything to date.
|
||
- **`TESTING_GUIDE.md`** — currently the only test doc; rename to `docs/MANUAL_QA.md` once `vitest` + `playwright` land.
|
||
- **`docs/API_REFERENCE.md`** — would help. Could be auto-generated by walking `pages/api/**/*.js` and extracting JSDoc; or hand-curated to start.
|
||
- **Privacy policy / Terms of service** — required before public launch. Use a template (Termly / Iubenda) and customize.
|
||
|
||
## Proposed launch sequence
|
||
|
||
Each phase is one Conductor-created convoy. Don't run more than two in parallel until tests exist.
|
||
|
||
0. **`bump-next-js`** (P0 #8). One PR. **MUST land first** — Vercel is currently blocking all deployments, which makes every other PR's preview-smoke / visual-diff gate non-functional. Trivial bump; risk is breaking changes if going to 16.x.
|
||
1. **`fix-auth-bypass`** (P0 #1, #2, #4, #5, #6 partial). One PR. Highest risk; needs human review.
|
||
2. **`drop-public-setup`** (P0 #3, #4). One PR. Trivial; do as a hotfix.
|
||
3. **`fix-layout-default-user`** (P0 #7). One PR. Trivial.
|
||
3.5. **`fix-lint-baseline`** (P1 #11.5). 2-4 PRs via multitask. Closes the lint gate (drops `continue-on-error`).
|
||
4. **`add-rate-limiting`** (P0 #6 full). One PR. Adds @upstash/ratelimit + applies to listed routes. **RESOLVED 2026-05-24** — PR #20 squash `708ef45`; closes P0 #6 (last open P0), flipping the ship-blocker set to **8/8 RESOLVED**. 5 named limiters (auth/search/upload/generate/import), 6 routes newly gated + the 3 import routes auth-gated atomically with a `pages/admin/card-import.js` Bearer-header fix. Smoke 3/3 green in 3.8s post-merge — confirms the new 60/min search limiter doesn't 429 the smoke spec. See § Queued convoys and P0 #6 above for the full as-shipped block.
|
||
5. **`pick-a-name`** (P1 #12). Human decision first, then one or two PRs. **RESOLVED 2026-05-24** — PR #21 squash `9abbab6`; closes P1 #12 (brand-consistency, the inconsistency `AGENTS.md` line 5 had flagged since project setup). Two file-disjoint briefs landed serially (B1 commit `ac8c998` display + comment sweep across 7 files; B2 commit `1c18d21` infrastructure + email migration across 10 modified + 1 new migration script). Five canonical-string D-decisions ratified verbatim at gate-1 (Deck Hearth / `deck-hearth` / `deckhearth` / `admin@deckhearth.com` / full `deckhearth` Redis prefix) plus Risk 4 PRESERVE on `test/lib/permission-middleware.test.js` line 87's negative regression-lock literal. Smoke 3/3 green in 1m4s post-merge — fourth convoy in a row (PR #15 → #19 → #20 → #21) where the same 3-test smoke spec defends the auth surface through a sweeping change. **Operator action required:** run `node scripts/migrations/2026-05-24-rename-admin-email.js` against prod Neon BEFORE the next admin login attempt with the new email (idempotent, UNIQUE-collision-safe). See § Queued convoys for the downstream `rename-repo-and-vercel-project` + `point-domain-at-deckhearth` + `regenerate-brand-assets` follow-ups, and `.convoys/pick-a-name.md` § As-shipped for the full record.
|
||
6. **`adopt-vitest`** (P1 #10 step 1). One PR. Enables testing every future change.
|
||
7. **`migration-tool`** (P1 #11). One PR. Backfill + first new migration.
|
||
8. **`single-sql-client`** (P1 #8). 2-3 PRs, fanned out via multitask once per-file briefs are written.
|
||
9. **`single-auth-provider`** (P1 #9). 3-5 PRs via multitask.
|
||
10. **`adopt-playwright-smoke`** (P1 #10 step 2). One PR. **RESOLVED 2026-05-24** — PR #18 squash `7b6f751`; smoke 3/3 green in 2.9s, full workflow 59s, zero secret leaks. See § Queued convoys for the full as-shipped block.
|
||
11. **`schema-cleanup`** (P2 #14). Multi-PR convoy via multitask.
|
||
12. **`god-component-split`** (P2 #13). One convoy per file; fan out via multitask once architect's `slice_dependencies` are written.
|
||
13. **`launch-polish`** (P3). UX/IA/a11y/docs convoy.
|
||
|
||
Total: ~14 convoys to get from current state to public-launch-ready. Estimate 4-8 weeks at one human-in-the-loop reviewer per convoy. Multitask + Cursor 3.2 worktrees compress steps 8-12 substantially.
|
||
|
||
## Queued convoys
|
||
|
||
Follow-ups surfaced mid-convoy or mid-PR that didn't fit the original launch sequence but need to land before public traffic. Listed in priority order; not all will be P0/P1 — most are CI / DX / hygiene polish.
|
||
|
||
- **`rotate-default-admin`** (priority: P2 hygiene). Operator-rotation script for envs that ran `setup-neon-db.js` before `drop-public-setup` and still carry the weak `admin123` bcrypt hash. Surfaced in P0 #3 § Operator caveat. Optional: do nothing if no audit finds a deployed env with the weak hash.
|
||
- **`delete-dead-lorcana-import`** (priority: P3 polish). Delete `pages/api/cards/import-lorcana.js` (and possibly `scripts/import-lorcana.js`) if Lorcana stays out of the admin UI's `<select>` permanently. Surfaced 2026-05-24 in `add-rate-limiting` Decision 1: the architect ran `rg 'import-lorcana' pages/ components/` and found zero frontend callers — `pages/admin/card-import.js`'s `<select>` only offers `'mtg'` and `'pokemon'`. The route is gated defensively (auth + admin-role + rate-limit) as part of PR #20 so the future Lorcana admin UI path inherits protection automatically, but if Lorcana is never wired in, this is the cleanup convoy. Strictly easier than gating-then-deleting because the gating shape is uniform across all three import routes today (mtg + pokemon + lorcana); a future cleanup only needs to delete the lorcana file + remove the `'lorcana'` enum option from `.cursor/rules/api-routes.mdc`'s import-routes table. Do nothing if Lorcana support gets wired into the admin UI in a feature convoy; cancel the entry then.
|
||
- **`tighten-visual-diff-path-filter`** (priority: P3 polish). Tighten `.github/workflows/visual-diff.yml`'s `paths:` filter from `pages/**` to something like `pages/!(api)/**` so API-only PRs don't trigger the visual workflow. Surfaced on PR #19 (`cors-tighten`) which was API-only but triggered `Screenshot diff` anyway, wasting ~55s of CI time. PR #20 (`add-rate-limiting`) re-triggered the same false-positive — same shape, same swallow via `continue-on-error: true`. Low priority — works as designed today, just chatty. One-line YAML tweak; verify the negated-glob syntax against GitHub Actions' [path-filter semantics](https://docs.github.com/en/actions/using-workflows/triggering-a-workflow#filter-pattern-cheat-sheet) before merging (the `!()` extglob may need to be expressed differently if Actions uses minimatch without extglob support).
|
||
- **`purge-weak-creds-from-helpers`** (priority: P2 hygiene). Sweep `scripts/reset-db.js`, `scripts/create-test-users.js`, and `TESTING_GUIDE.md` for the remaining `admin123` / `alice123` / `bob123` weak password literals. **Email-half satisfied 2026-05-24** by `pick-a-name` Brief 2 (squash `9abbab6`) — the `admin@tcgvault.com` / `alice@tcgvault.com` / `bob@tcgvault.com` references in those three helper paths were swept to `@deckhearth.com` together with the migration script. The remaining scope is the weak-password literals only (`admin123` etc. survived `pick-a-name` deliberately per Brief 2's scope boundary). May fold into `convert-reset-db-to-esm` (the CJS-in-ESM bug at `scripts/reset-db.js:142` overlaps the same file).
|
||
- **`rename-repo-and-vercel-project`** (priority: P2 polish). Rename the GitHub repo + the Vercel project from `tcg-vault` to `deck-hearth` to match the canonical product brand ratified in `pick-a-name` (squash `9abbab6`, 2026-05-24). Auto-redirects on both GitHub and Vercel make this low-urgency; the surface is a one-line update to local git remotes (`git remote set-url origin git@github.com:<owner>/deck-hearth.git`) + a Vercel project-settings rename + the 8 architect-verified literal-repo references documented in `.convoys/pick-a-name.md` § Full surface inventory § Repo / Vercel project name (out-of-scope) — `README.md` lines 30 + 105, `AGENTS.md` line 1, `.github/workflows/ci.yml` lines 12 + 123, `.github/workflows/visual-diff.yml` line 5, `.agent-context-manifest.yml` source tags. Also re-evaluate the `.agent-context-manifest.yml` `source: "tcg-vault-local"` tag at that point (Risk 5 of `pick-a-name` — renaming the source tag could break the `sync-agent-context` skill's drift tracking; do this convoy with the sync-skill author's input). Surfaced 2026-05-24 as the explicit downstream of `pick-a-name`.
|
||
- **`point-domain-at-deckhearth`** (priority: P2 polish; **blocked on domain acquisition**). Once the operator buys `deckhearth.com` (or `.app` / `.gg` / other), wire DNS to the Vercel deployment + claim the domain in Vercel's project settings + update the seed admin email's TLD if the purchased TLD is anything other than `.com` (a one-line REPLACE migration mirroring `scripts/migrations/2026-05-24-rename-admin-email.js`). Surfaced 2026-05-24 as the explicit downstream of `pick-a-name` (the convoy seed § *"DNS / domain — out of scope — separate convoy `point-domain-at-deckhearth` (you don't own a `deckhearth.*` domain yet per operator's pre-convoy statement)"*). Until this lands, the seed admin email `admin@deckhearth.com` is a placeholder STRING used as a unique identifier — auth uses email as identity, not as a mail target, so no working mailbox is required for login to function.
|
||
- **`regenerate-brand-assets`** (priority: P2 polish). Regenerate the favicon (`public/favicon.ico`), Open Graph images, and social share cards with the new Deck Hearth identity. Requires a design pass — out-of-scope for any single agent-driven convoy; queue when an asset-design pass is scheduled. Surfaced 2026-05-22 originally in `.convoys/ship-readiness.md` § Role-design-system-auditor findings ("Then once the name is settled, the 'DH' logo + AnimatedFireLogo need to be unified into one brand mark"); reaffirmed 2026-05-24 in `pick-a-name` Out-of-scope queued follow-ups.
|
||
- **`convert-reset-db-to-esm`** (priority: P2 hygiene). `scripts/reset-db.js` line 142 uses CommonJS `require('bcryptjs')` inside an ESM file (`package.json` has `"type": "module"` since `bump-next-js`). Same bug pattern that hit `setup-neon-db.js` pre-`drop-public-setup` Brief 2. The script is broken on Node 22.x — `npm run reset-db` doesn't actually work today. Surfaced 2026-05-24 in `pick-a-name` architect inventory (Brief 2 Boot-the-brief Finding 3); B2 only renamed the email literals on lines 147 + 155 and left the CJS bug + weak-password literals on lines 143 + 156 byte-identical because both fixes are out-of-scope for the brand-rename convoy. **May fold into `purge-weak-creds-from-helpers`** if more such bugs accumulate in helper scripts — both convoys touch the same file (`scripts/reset-db.js`) and the same fix sequence (CJS → ESM, then strip the weak-password literals + the password-echo console log) keeps the diff atomic.
|
||
- **`single-auth-provider`** (priority: P1 quality, also listed as launch sequence step 9). Collapse `lib/auth-context.js` + `lib/admin-auth.js` into `lib/use-auth.js`. Surfaced again as a follow-up in P0 #7 § Flagged-but-deferred (4 pages still import the legacy `useAuth`).
|
||
- **`cleanup-mobile-nav-dead-props`** (priority: P3 polish). `components/MobileNavigation.js` accepts a dead `user` prop; remove it. Surfaced in P0 #7 § Flagged-but-deferred. May fold into `god-component-split` (P2 #13) if that lands first.
|
||
- **`bump-eslint-10`** (priority: P2 hygiene; upstream-blocked). Bump ESLint from v9 to v10 once `typescript-eslint` ships a v10-tested release and `eslint-config-next` bundles it. Surfaced in `.convoys/bump-next-js.md` § Decisions D.
|
||
- **`harden-multipart-parser`** (priority: P2 quality). Surfaced 2026-05-24 in `add-rate-limiting` § Risk list. `pages/api/user/avatar.js`'s `parseMultipartFormData` consumes the 5MB multipart body via `req.on('data')` before any response is sent, so an attacker can still exhaust the 5MB body even on a 429 path from the new `checkUploadRateLimit` gate. Real defense requires moving the parse into a separate edge function or using `read-up-to` semantics. Not a release-blocker — the gate-ordering in PR #20 places the limiter BEFORE the method branches that call `parseMultipartFormData`, so when this hardening lands, the gate ordering is already correct. Surface as P1 only if a real abuse incident occurs.
|
||
- **`god-function-split` / `refactor-cards-search-sql`** (priority: P2 refactor). Surfaced 2026-05-24 in `add-rate-limiting` § Files explicitly out of scope. `pages/api/cards/search.js` has a 240-line god-function shape with 7+ conditional `SELECT * FROM cards WHERE …` branches; the PR #20 rate-limit gate sits at the top of the handler and leaves the SQL byte-identical. Splitting is its own scope (probably one convoy per branch group with `slice_dependencies:` for safe multitask fan-out). Not security-critical; deferred to the P2 lane.
|
||
- **`withAdmin(handler)` wrapper extraction** (priority: P3 polish / DX). Surfaced 2026-05-24 in `add-rate-limiting` Decision 1 + § What did NOT change. `.cursor/rules/auth-and-permissions.mdc` notes *"check `user.role === 'admin'` directly; consider extracting `withAdmin()` if a third call site appears"* — the three `cards/import-*.js` routes are the third+fourth+fifth call sites in the codebase, but PR #20 kept the inline shape for uniformity across the three import routes and for the convoy's atomic-close-P0-#6 goal. A future convoy can extract `withAdmin(handler)` to `lib/permission-middleware.js` (or wherever the architect decides) and sweep all 5 admin-role check sites onto it. Pure refactor; no security delta either way.
|
||
- **`seed-visual-baselines-on-linux`** (priority: P2 CI infra; **operator action required**). Generate Linux baselines for `tests/visual/__screenshots__/` in the `mcr.microsoft.com/playwright:v1.60.0-noble` Docker image and commit them in a small follow-up PR. Mac-generated baselines would silently overwrite Linux CI baselines because `playwright.config.js`'s custom `snapshotPathTemplate` has no `{platform}` token (Risk R3 + Boot-the-brief Finding 7 in `.convoys/adopt-playwright-smoke.md`). Until this PR lands, every `Screenshot diff` run on a PR touching `pages/**` / `components/**` / `styles/**` / Tailwind/PostCSS config fails at the test step and posts a "Visual Diff — view run" comment with empty artifacts — that's the documented Decision-4 end state of `adopt-playwright-smoke`, not a regression. One small PR with just the PNG baseline(s). Surfaced 2026-05-24 as the follow-up to `adopt-playwright-smoke` (PR #18). **Ordering: MUST run AFTER `pick-a-name` (now satisfied — squash `9abbab6` merged 2026-05-24); the first Linux baseline will capture Deck Hearth strings, not the pre-rename TCG Vault strings** (per `.convoys/pick-a-name.md` § Test plan + the architect's `update-seed-visual-baselines-on-linux-ordering` follow-up note).
|
||
- **`adopt-playwright-smoke`** (priority: P1 quality, also listed as launch sequence step 10 / P1 #10 step 2) — **RESOLVED 2026-05-24**.
|
||
- **Resolved by:** squash commit `7b6f751` (PR #18, architect-commit `3ac527e`, implementer-commit `c72d006`). Brief 1 shipped as planned with two small lint-baseline-preserving deviations from the brief's verbatim shape (documented in the convoy file's § As-shipped).
|
||
- **As-shipped surface:** `@playwright/test@^1.60.0` added to `devDependencies`; new `playwright.config.js` at repo root (ESM, two projects partitioned by `testMatch` — `smoke` + `visual`, CI-fail-loud / dev-warn predicate on `VERCEL_AUTOMATION_BYPASS_SECRET` per Decision 2, `snapshotPathTemplate: 'tests/visual/__screenshots__/{arg}{ext}'` aligned with `visual-diff.yml`'s artifact upload path); new `tests/visual/homepage.spec.ts` (1 test, no baseline committed per Decision 4); three new `package.json` scripts (`test:smoke`, `test:visual`, `test:visual:update`); three new `.gitignore` entries (`/playwright-report/`, `/test-results/`, `/.playwright/`). No `eslint.config.mjs` change (Decision 5 + Finding 2 verified clean empirically). No source touched under `pages/**` / `components/**` / `lib/**`.
|
||
- **Implementer deviations** (both behavior-neutral, both lint-baseline-preserving):
|
||
1. Removed Brief 1's `// eslint-disable-next-line no-console` directive on `playwright.config.js`'s `console.warn` branch — the current ESLint config does not flag `console.warn` at all, so the disable directive itself would have regressed lint from 128 → 129 as an "Unused eslint-disable directive" error.
|
||
2. Placed `@playwright/test` first in `devDependencies` for strict alphabetical correctness — the brief's prose was internally inconsistent on neighbors (`@playwright` sorts lexically before `@testing-library/react`).
|
||
- **As-shipped metrics** (from post-merge `Playwright smoke` run 26376162598 on `main`):
|
||
- `Playwright smoke` workflow total runtime: **59 seconds**, exit 0 (was: fast-fail at "playwright not installed" / "no config" before this convoy).
|
||
- `Run smoke tests` step: **3/3 tests pass in 2.9s** against the Vercel preview with `x-vercel-protection-bypass` header applied — `home redirects or renders without 5xx` ✓ 683ms / `sign-in page renders` ✓ 459ms / `public health endpoint responds` ✓ 571ms.
|
||
- `Screenshot diff` workflow: **not triggered on PR #18 itself** because its `paths:` filter excludes test-infra-only changes; first real trigger fires on the next PR touching `pages/**` / `components/**` / `styles/**` / `tailwind.config.js` / `postcss.config.js`. At that point the documented Decision-4 end state runs live (test fails on missing baseline → `continue-on-error: true` swallows → comment-on-PR step posts run link with empty artifacts).
|
||
- Bypass secret leak check: **0 matches** in the raw workflow log. GitHub Actions auto-masks registered secrets; our Decision-2 branches name the env var but never interpolate the value into any string.
|
||
- **Cross-validation finding** (not a planned AC; surfaced organically from CI green): smoke test 2 (`'sign-in page renders'`) asserts `await expect(page.getByRole('button', { name: /sign in/i })).toBeVisible()` against `/login`, which only passes because `components/Layout.js` renders the `<Link href="/login">Sign in</Link>` CTA on the logged-out branch that PR #15 (`fix-layout-default-user`, `ca302a8`) introduced. P0 #7's resolved state is now defended by a live CI signal — if a future PR reverts to a hardcoded default user or breaks the CTA wording, smoke fails the PR (in addition to the 5 vitest assertions in `test/components/Layout.test.js`).
|
||
- **Operator action required going forward:** `seed-visual-baselines-on-linux` (above) is the follow-up. Until it lands, `Screenshot diff` runs post a "Visual Diff — view run" comment with empty artifacts on every UI-touching PR — that is the Decision-4 end state, not a regression. No operator action is required to keep `Playwright smoke` green.
|
||
- **Flagged-but-deferred** (deliberately out of scope per the convoy file, restated here for the audit trail):
|
||
1. `seed-visual-baselines-on-linux` — see above.
|
||
2. `adopt-test-smoke-local` (possible follow-up) — a `test:smoke:local` wrapper that auto-boots `next dev`. Explicitly rejected by Decision 6; queue only if dev friction proves out.
|
||
3. Deeper E2E coverage beyond the 3 existing smoke checks — per-feature work in feature convoys, not a test-infra concern.
|
||
- **Owns:** `role-architect` (3 of 6 decisions self-ratified — D2 CI predicate, D3 two-project shape, D5 no-eslint-change; 3 of 6 operator-ratified — D1 keep `.ts`, D4 defer baselines, D6 simple scripts) → `role-implementer` (Brief 1, plus the two deviations above).
|
||
- **`fix-vercel-deployment-protection-in-ci`** (priority: P2 CI infra) — **RESOLVED 2026-05-24**.
|
||
- **Resolved by:** squash commit `9a3e077` (PR #17), comprising three commits, not one. Operator prereq seeded 2026-05-24T20:03:31Z (`gh secret set VERCEL_AUTOMATION_BYPASS_SECRET`; confirmed via `gh secret list`); the implementer dispatch waited on that visibility per the convoy file's "Operator action required" gate.
|
||
- **Three-commit reality** (Brief 1 + two scope expansions found during CI validation):
|
||
1. `365e9f0` Brief 1 — bypass plumbing per spec. Both `.github/workflows/preview-smoke.yml` and `.github/workflows/visual-diff.yml` got the same shape change: `wait-for-vercel-preview@v1.3.2`'s `path:` input now carries `/?x-vercel-protection-bypass=${{ secrets.VERCEL_AUTOMATION_BYPASS_SECRET }}&x-vercel-set-bypass-cookie=true` (Decision A's original cookie-variant shape — later corrected in commit 3); `max_timeout: 600 → 120` (Decision B); the `gate:` job's Decide step short-circuits on `github.event.pull_request.head.repo.fork == true` with a `::notice::` annotation, before the existing PR-body skip directive runs (Decision D); and the Playwright/screenshot step exports `VERCEL_AUTOMATION_BYPASS_SECRET` as `env:` for forward-compat with `adopt-playwright-smoke`.
|
||
2. `b6f8688` shell-injection hardening (latent pre-existing bug surfaced by PR #17's own CI validation). Decision D's gate step inlined `${{ github.event.pull_request.body }}` directly into bash, which broke when the PR body contained shell metacharacters like `(` or backticks. PR #17's own description bit this with `"unexpected token \`('"` because of phrasing like *"(was: 10-minute timeout)"*. Fix is the standard GitHub Actions hardening pattern (their official "Security hardening" guide flags inline `${{ }}` in shell as both a syntax-error risk and a shell-injection vector): route the body and the fork flag through the step's `env:` block as `PR_BODY` and `PR_IS_FORK`, then quote them as `"$PR_BODY"` / `"$PR_IS_FORK"` in the shell condition. Same change in both workflows; ~9 LOC each. Documented in commit message as technically beyond Brief 1's scope but bundled into the convoy because the bug actively blocked Brief 1's success criterion from being validated.
|
||
3. `043a6ee` drop `&x-vercel-set-bypass-cookie=true` from the wait-action `path:` — corrects Decision A's exact shape. With the cookie variant, Vercel responds **307 + Set-Cookie**, and axios in Node has no cookie jar — it follows the redirect to the bare URL without the cookie, which then 401s. Empirically confirmed by operator's local curl: bare `?x-vercel-protection-bypass=X` → HTTP/2 200, while `?x-vercel-protection-bypass=X&x-vercel-set-bypass-cookie=true` → HTTP/2 307 (the broken path). For a one-shot healthcheck the per-request bypass query is enough. The cookie variant stays reserved for the future Playwright config (`adopt-playwright-smoke`) where a real browser cookie jar exists. An inline comment in `preview-smoke.yml` explains this so the next agent doesn't accidentally re-add the cookie param.
|
||
- **As-shipped metrics** (from PR #17's CI run, post-validation):
|
||
- `Wait for Vercel Preview deployment` step elapsed: **194 milliseconds** (was: 10-minute timeout before this convoy).
|
||
- `Playwright smoke` workflow total runtime: **59 seconds** (was: 10+ minutes).
|
||
- Step breakdown: `Wait for Vercel Preview deployment` → success in 194ms; `npm ci`, `setup-node`, `playwright install` → success; `Run smoke tests` → **failure** (expected — see next bullet).
|
||
- `Screenshot diff` workflow: not triggered on PR #17 itself because its path filter excludes workflow-only changes; will fire on the next PR touching `pages/**` / `components/**` / `styles/**` / Tailwind/PostCSS config.
|
||
- **Documented expected red**: `Playwright smoke` now reaches `npx playwright test` and fast-fails because `playwright.config.js` doesn't exist in the tree yet. That is `adopt-playwright-smoke`'s scope (P1 #10 step 2 / launch sequence step 10), not this convoy's. Per the convoy file's Test plan § and Brief 1 acceptance criterion #1, a real downstream failure with the wait-action reaching `Received success status code` first counts as success for this convoy — the failure mode shifted from "401 timeout in the wait step" to "playwright not installed", which is precisely the target state.
|
||
- **Operator-rotation caveat** (R6 in the convoy file). The Vercel bypass token does not auto-expire. If/when it's rotated from the Vercel dashboard, the operator must re-seed the GitHub secret via `gh secret set VERCEL_AUTOMATION_BYPASS_SECRET --body "<new value>"`. Same human-responsibility pattern as `JWT_SECRET` rotation; not preventable from workflow YAML. No automation here.
|
||
- **Flagged-but-deferred** (from the convoy file's "Anything flagged but not acted on" section, unchanged at merge):
|
||
1. `replace-wait-for-vercel-preview` — the wait-action's last release was Mar 2024; could be replaced with a few lines of `gh api` + `curl`-loop. Out of scope for this convoy; queue if the action ages out further or gets a security advisory.
|
||
2. `adopt-playwright-smoke` — owns the actual `playwright.config.js`, `tests/smoke/`, and `@playwright/test` dep. The bypass plumbing here is forward-compat for that convoy (env var available on the smoke step). Listed in P1 #10 step 2 / launch sequence step 10 above.
|
||
3. `Screenshot diff` baseline authoring — orthogonal scope; the visual-diff workflow has nothing to compare against on its first real run.
|
||
- **Owns:** `role-architect` (3 Decisions ratified — A query-param, B 120s timeout, D fork-PR skip) → `role-implementer` (Brief 1) + two scope-expansion commits.
|
||
|
||
## Self-analytics
|
||
|
||
After each convoy, `scripts/log-convoy-event.sh` emits a record to `.convoys/.metrics.jsonl` (gitignored). After 3-5 convoys, run the upstream `agent-pipeline/analytics/` aggregator to see where token spend goes — that data feeds whether to add or remove rules.
|
||
|
||
## How to start
|
||
|
||
Per `.cursor/agents/role-conductor.md`, start the next convoy with:
|
||
|
||
> *"Run role-conductor: start a new convoy `fix-auth-bypass` to address P0 #1, #2, #4, #5, #6 partial in `.convoys/ship-readiness.md`. Success = `getUserFromRequest` returns null for missing tokens; no API route accepts unauthenticated requests; CI green."*
|
||
|
||
The Conductor will set classification, skip flags, and hand off to subsequent roles.
|