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	open	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

Stub for doc-writer. Populate after merge with:

• Squash commit SHA + PR number
• Diff stats (file count, line counts)
• CI gate outcomes (lint, vitest, smoke, forbidden-* gates)
• Any operator-action follow-ups
• Cross-validation findings (e.g. smoke continuing to pass)
• Updates needed to AGENTS.md Gotcha #1 + .cursor/rules/db-and-schema.mdc § Clients + ship-readiness P1 #8 entry

Follow-ups

• lint-against-lib-database (priority: P3 polish). Add an ESLint no-restricted-imports rule against '../**/lib/database' (or any path resolving to a lib/database.js file), so a future PR that re-introduces the unsafe wrapper fails at lint time rather than at runtime. Pairs naturally with the queued lint-against-cjs-in-esm-scripts follow-up (both are static-source guards added to eslint.config.mjs). Small surface (one entry in the config); could fold into a harden-eslint-static-guards convoy if more such guards accumulate.

• purge-neondatabase-serverless-fully (priority: P3 polish; blocked on migration-tool). Once the migration-tool convoy (P1 #11) lands and replaces the ad-hoc scripts/add-* / scripts/fix-* / scripts/seed-* shape with a real migration framework, revisit whether the remaining 11 @neondatabase/serverless import sites can be collapsed onto @vercel/postgres. Caveat: @vercel/postgres is tuned for the Vercel edge runtime and may not be the right choice for developer-laptop / CI-runner scripts; the right answer may be "keep the dep, but route all scripts through a single thin helper" rather than "delete the dep entirely". This is its own scope; do NOT bolt onto this convoy.

• add-neon-return-shape-rule (priority: P3 polish; surfaced 2026-05-25 in PR #24 + restated in .convoys/fix-reset-db-script.md § Out of scope). Codify the neon() (returns [rows]) vs @vercel/postgres ({ rows: [...] }) return-shape difference as a .cursor/rules/db-and-schema.mdc callout. Partially satisfied by this convoy because the dual-client shape is now collapsed for pages/api/** — only the 11 scripts/** callers still use neon() directly, and they all destructure the array return shape correctly today. The rule would defend future contributors who don't already know the difference; queue if a third such bug surfaces.

Owns

• Architect (this convoy file + decisions D1-D5).
• Implementer (mechanical: 2 source-file edits + 1 deletion + verification gates). Possibly the same agent — the implementation surface is small enough that the cost of a fresh implementer subagent boot may exceed the cost of doing the work in the architect's own turn. Documented per fix-reset-db-script precedent ("Why no architect" / "Why no implementer" — single-file proven-pattern work).