deckhearth/.convoys/single-sql-client.md

name	classification	success_metric	skip	status	created	parent	addresses	depends_on
single-sql-client	quality	`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.	role-design-system-auditor role-a11y-auditor role-ux-reviewer role-ia-architect	shipped	2026-05-26	ship-readiness	P1

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:

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:

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):

import jwt from 'jsonwebtoken';
import { db } from '../../lib/database.js';
import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';

Imports (after):

import jwt from 'jsonwebtoken';
import { sql } from '@vercel/postgres';
import { JWT_SECRET, JWT_TOKEN_TTL } from '../../lib/auth-secret.js';

isAdmin body (before):

const result = await db.query(`
  SELECT role FROM users WHERE id = $1
`, [userId]);

isAdmin body (after):

const result = await sql`
  SELECT role FROM users WHERE id = ${userId}
`;

getUserById body (before):

const result = await db.query(`
  SELECT id, email, role, created_at FROM users WHERE id = $1
`, [userId]);

getUserById body (after):

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):

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):

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

Single squash commit c403ea4 (PR #30, merged 2026-05-27T03:54:01Z UTC / local 2026-05-26). Parent-owned end-to-end per the "Owns" line — no architect, no implementer subagent dispatched. Mirror-the-pattern fix exactly as planned; no mid-execution surprises that would have forced an architect bounce. AGENTS.md Gotcha #1 flipped from open → RESOLVED in the post-convoy doc-writer pass (see .convoys/ship-readiness.md § P1 #8 entry post-flip and the AGENTS.md sweep in this same wave).

Diff: 4 files, +447 / -64. The 447-addition figure is dominated by .convoys/single-sql-client.md (~330 lines for the planning document, committed atomically with the fix). Actual source-file diff is small: pages/api/auth-utils.js +5 / -7 (import swap + 2 query shape conversions); test/api/auth-utils.test.js +1 / -5 (drop the now-unused vi.mock + unused vi import); lib/database.js 0 / -47 (deletion).

The change shipped exactly as designed:

1. lib/database.js deleted. 47-line file; no replacement. The DatabaseAdapter 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.
2. pages/api/auth-utils.js swept (the only source caller).
   • Import: import { db } from '../../lib/database.js' → import { sql } from '@vercel/postgres'.
   • isAdmin(userId): await db.query(\SELECT role FROM users WHERE id = $1`, [userId]) → `` await sqlSELECT role FROM users WHERE id = ${userId}` ``.
   • getUserById(userId): await db.query(\SELECT id, email, role, created_at FROM users WHERE id = $1`, [userId]) → `` await sqlSELECT 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.
   • SQL is byte-equivalent (single-parameter numeric userId); result shape is identical ({ rows, rowCount } from @vercel/postgres matches what DatabaseAdapter.query was returning).
3. test/api/auth-utils.test.js mock cleanup. Dropped vi.mock('../../lib/database.js', () => ({ db: { query: vi.fn() } })) (no longer needed because auth-utils.js no longer imports from lib/database.js) and the unused vi import. The 5 test bodies (2 generateToken + 3 verifyToken) are unchanged. Test count stays at 21/21.

@neondatabase/serverless retained as a runtime dep, per D3 of this convoy file. 11 scripts/* helpers still import neon() directly (scripts/setup-neon-db.js, scripts/migrations/2026-05-24-rename-admin-email.js, scripts/reset-db.js, plus 8 historical add-* / fix-* / seed-* jobs); all out of scope per the no-go-zones rule. The dep-purge is tracked as the newly queued purge-neondatabase-serverless-fully follow-up (now unblocked by PR #32 migration-tool — the migration helpers go through node-pg-migrate's pg client, not @neondatabase/serverless, so the only remaining direct neon() consumers post-PR-#32 are setup-neon-db.js, reset-db.js, and the historical graveyard).

Verification (all gates green at merge):

• node --check pages/api/auth-utils.js → exit 0
• npm run lint → 125 problems (post-PR-#31 baseline preserved; zero regression). The lib/database.js deletion did not change lint count because the file was already lint-clean.
• npm run test:run → 21/21 pass
• rg "lib/database" --type js -l → 0 hits anywhere (no pages/**, no lib/**, no scripts/**, no test/**) — confirms no missed importer (R2 mitigation)
• rg "@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)
• CI on PR #30: Lint ✓ | Vitest 21/21 ✓ | Playwright smoke 3/3 ✓ | forbidden-endpoints ✓ | forbidden-cors-headers ✓ | Vercel preview deploy ✓ | Aggregate gate ✓
• Screenshot diff: NOT triggered. PR #30's diff is pages/api/** + lib/** + test/** + .convoys/** — the post-PR-#26 !pages/api/** exclusion correctly held. This PR is the first empirical confirmation that the tighten-visual-diff-path-filter (PR #26, ba95462) exclusion fires as documented — the post-merge success criterion that PR #26's § Verification plan deferred until "the next API-only PR after this merges". Screenshot diff did NOT appear in PR #30's Checks tab; the convoy-file as-shipped block of PR #26 now records this confirmation.

Live runtime smoke deferred per § Verification plan — 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. Running a live curl smoke would require seeding the admin user with a known password (operator-only flow per drop-public-setup's contract) and minting a token. The byte-equivalent SQL semantics (D2) plus identical try/catch + result.rows[0] access shape gives high confidence the migration is correct; if a post-merge Vercel preview admin action 500s, rollback is a single-commit revert.

Cross-validation finding (continues the lineage). The Playwright smoke 3/3 PASS confirms the deployed preview is unaffected by the lib-database deletion — the smoke spec doesn't exercise isAdmin / getUserById, but the auth surface that smoke does exercise (/api/health, sign-in render) is correctly insensitive to the DatabaseAdapter removal. Eighth consecutive convoy where the same 3-test smoke spec defends the auth surface through a sweeping change (PR #15 → #19 → #20 → #21 → #25 → #32 → #27 → this PR).

Operator action required going forward: none. No env-var change; no schema change; no infra change. The migration is purely a client swap; the existing POSTGRES_URL contract is preserved verbatim (both lib/database.js and @vercel/postgres read the same POSTGRES_URL).

Spec deviation: none of substance. The convoy spec's "~3 files based on graph" estimate was loose; architect grep (D1) confirmed exactly 2 source-tree callers (1 source + 1 test mock). No deviation flag needed; the convoy is even smaller than estimated. The post-flip ship-readiness P1 #8 entry records this fact explicitly.

Doc surface flipped atomically (in the post-convoy doc-writer pass): AGENTS.md Gotcha #1 → RESOLVED with the as-shipped paragraph (tool deletion + retained-dep caveat + the 11-script neon() direct consumers list); AGENTS.md § 3 "DB access" bullet trimmed (the warning about lib/database.js is gone — the file doesn't exist); .cursor/rules/db-and-schema.mdc § Clients section refreshed to match (handled in the doc-writer commit alongside this convoy in the 7-wave cleanup).

Surfaced follow-ups (newly queued in .convoys/ship-readiness.md):

• purge-neondatabase-serverless-fully (P3 polish; unblocked by PR #32).
• lint-against-lib-database (P3 polish; from this convoy's pre-existing § Follow-ups list — would prevent re-introduction).

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).