rstillwell/deckhearth
1
0
Fork 0
Code Issues 1 Pull requests Projects Releases Packages Wiki Activity Actions 10

architect(drop-public-setup): expand scope with brief 2 (CJS→ESM)

Browse source 
Mid-convoy discovery: brief 1's implementer confirmed scripts/setup-neon-db.js
does not actually run on Node 22.x because bump-next-js added "type": "module"
to package.json but the seed script still uses CJS require() calls.
Throws ReferenceError immediately on `npm run setup-db`.

Architect's original "Anything flagged but not acted on" #1 claim that "it
runs successfully today under Node 22" was incorrect for Node 22.14.0.

Decision D (ratified by user 2026-05-23): expand convoy to include brief 2
rather than queue a separate convert-setup-db-to-esm follow-up. Rationale:
brief 1's env-var gate is theatrical security on a script no operator can
execute; the CJS→ESM conversion is mechanical (~6 LOC, same file, no
functional changes); splitting into two convoys creates a regression window
where operators on Node 22.x cannot bootstrap a database.

Brief 2 scope: pure module-system conversion in scripts/setup-neon-db.js:
  - require('dotenv').config(...) → import dotenv + dotenv.config(...)
  - require('@neondatabase/serverless') → import { neon }
  - inline require('bcryptjs') hoisted to top-of-file import
  - no functional changes; same DDL, same env-var gate, same console.logs

Verification: smoke must now show npm run setup-db actually executes (no
ReferenceError); brief-1 env-var gate must still fire as documented;
all 16 vitest tests must still pass.

Updated:
  - .convoys/drop-public-setup.md Decomposition (brief 2 added, depends_on brief 1)
  - .convoys/drop-public-setup.md slice_dependencies YAML
  - .convoys/drop-public-setup.md § Decisions (added Decision D)
  - .convoys/drop-public-setup.md § Anything flagged but not acted on
    (item #1 marked resolved by brief 2)

addresses: P0 #3 from .convoys/ship-readiness.md + Node 22.x compat
parent: ship-readiness
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Randall Stillwell 2026-05-23 15:03:25 -05:00 committed by varutasu
parent ff80753fe4
commit 792439b5eb
2 changed files with 193 additions and 2 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
.convoys/drop-public-setup.md
View file

@ -133,6 +133,7 @@ Implementer pastes the stdout from steps 1 + 2 into the PR description for the r
| Brief # | Title | Files | Depends on | Estimated PR size |
| --- | --- | --- | --- | --- |
| 1 | Require `ADMIN_INITIAL_PASSWORD` env var; strip credentials from README | `scripts/setup-neon-db.js`, `README.md` | — | ~25 LOC net (10 added, 15 removed) |
| 2 | Convert `scripts/setup-neon-db.js` from CJS to ESM | `scripts/setup-neon-db.js` | brief 1 | ~6 LOC net (3 added, 3 removed) |
### Slice dependencies (multitask-ready)
@ -143,9 +144,14 @@ slice_dependencies:
files:
- scripts/setup-neon-db.js
- README.md
- brief: 2
depends_on:
- brief: 1
files:
- scripts/setup-neon-db.js
```
No fan-out — one brief, two files, single PR. `/multitask` is not applicable.
No fan-out — both briefs touch the same script and ship in the same PR on `convoy/drop-public-setup`. `/multitask` is not applicable. Brief 2 must land *after* brief 1 because brief 2's "do not touch the env-var check block" acceptance criterion references brief 1's code shape verbatim.
## Decisions
@ -161,10 +167,16 @@ The no-go-zones rule prohibits editing `scripts/setup-neon-db.js` for **schema c
The setup script runs a handful of times per environment lifetime; its failure modes are loud (`process.exit(1)` + clear stderr) and immediate; and the manual-smoke acceptance criteria in the brief cover both the happy and fail-loud paths. **Rationale (3 sentences):** unit-testing the env-var path would require either extracting a `lib/seed-admin.js` module (Decision C3 — scope expansion, ~2× brief LOC, plus a new file to maintain) or spawning `node scripts/setup-neon-db.js` from vitest and mocking `@neondatabase/serverless` (slow, fragile, low signal). Following the same heuristic the existing vitest suite uses — "tests cover the runtime auth surface that 30+ handlers depend on, not the operational scripts that run once per env" — leaves the test inventory focused on the highest-blast-radius surfaces. If a future convoy extracts seed logic for any other reason (e.g. multi-env seed templates), it can add tests at that point for free.
### Decision D: scope expansion to include CJS → ESM conversion → **Option B (expand this convoy)**
**Discovered mid-convoy:** the implementer for brief 1 confirmed that `scripts/setup-neon-db.js` does not actually run via `npm run setup-db` on Node 22.x. The `bump-next-js` convoy added `"type": "module"` to `package.json` (required for ESLint v9 flat config); the seed script still uses CJS `require()` calls and throws `ReferenceError: require is not defined in ES module scope` on first invocation. The architect's note in § "Anything flagged but not acted on" #1 — "it runs successfully today under Node 22" — was incorrect for Node 22.14.0.
**Decision:** expand this convoy to include brief 2 (`convert-setup-db-to-esm`) rather than queue a separate `convert-setup-db-to-esm` follow-up convoy. **Rationale (3 sentences):** brief 1's env-var gate is theatrical security on a script no operator can actually execute on Node 22.x, so the two fixes are logically coupled and shipping them in one PR creates a single coherent "setup-db is now both safe and functional" change. The CJS→ESM conversion is mechanical (~6 LOC, no functional changes) and touches the same file as brief 1, so review and audit overhead is near-zero. Splitting into two convoys would mean operators on Node 22.x cannot bootstrap a database between PRs — an unnecessary regression window for a fix that fits cleanly in the same surface area. User ratified the expansion (Option B) on 2026-05-23 after the implementer's gate-1 report surfaced the breakage.
## Anything flagged but not acted on
These are real findings surfaced during architecture but **deliberately out of scope** for this convoy. Each should be tracked separately so the audit trail survives.
1. **CommonJS `require()` in an ESM package (`scripts/setup-neon-db.js`).** The repo's `package.json` has `"type": "module"` (since the `bump-next-js` convoy), but this script still uses `require('dotenv')` / `require('@neondatabase/serverless')` at the top. It runs successfully today under Node 22 — `node scripts/setup-neon-db.js` executes the file; `require()` interop appears to be working — but this is a fragile state and could break on a future Node minor or under stricter loader semantics. **Why not fixed here:** the convoy's stated scope is the env-var gate + README, and converting CJS → ESM in the same brief would (a) widen the diff beyond the 25-LOC estimate, (b) require also adjusting the inline `require('bcryptjs')` plus the dotenv loader pattern, and (c) is a separate concern that should land on its own so the diff is reviewable in isolation. **Follow-up suggestion:** queue a tiny `convert-setup-db-to-esm` convoy if anyone runs into a Node-loader regression, or fold it into the queued `migration-tool` convoy when that lands and rewrites the seed surface anyway.
1. ~~**CommonJS `require()` in an ESM package (`scripts/setup-neon-db.js`).**~~ **Resolved by brief 2 (added 2026-05-23 mid-convoy per Decision D).** Original architect's claim that "it runs successfully today under Node 22" was incorrect — the implementer for brief 1 confirmed the script throws `ReferenceError: require is not defined in ES module scope` on Node 22.14.0. Scope was expanded to include the CJS→ESM conversion in this convoy rather than queue it as a separate follow-up. See § Decisions D for the ratification.
2. **Sibling weak-credential references in `scripts/reset-db.js`, `scripts/create-test-users.js`, and `TESTING_GUIDE.md`.** `reset-db.js` mirrors `setup-neon-db.js`'s admin-INSERT and password echo (lines ~141-156); `create-test-users.js` prints `admin@tcgvault.com / admin123 (Admin)` as a usage hint (line ~34); `TESTING_GUIDE.md` has the same row in its test-account table (line ~7). **Why not fixed here:** the convoy spec's "Out of scope" list explicitly excludes "any other `scripts/seed-*.js` or `scripts/add-*.js` files (those are one-off historical jobs per no-go-zones)", and `reset-db.js` is in the `scripts/fix-*` / historical family. `TESTING_GUIDE.md` is the manual-QA doc that doc-writer is expected to rename to `docs/MANUAL_QA.md` in the launch-polish convoy (P3, ship-readiness § Role-doc-writer findings) — folding it in here adds review overhead. **Follow-up suggestion:** queue a small `purge-weak-creds-from-helpers` convoy as part of the launch-polish phase, or roll it into `pick-a-name` (since the email itself is also changing).
3. **The success-summary `console.log(' Admin User: admin@tcgvault.com')` line.** This brief deletes it along with the password line (R3), but the admin email itself is still hardcoded in the SQL `INSERT` (line 135) and is going to be renamed under the queued `pick-a-name` convoy (P1 #12). Not a credentials issue, but worth noting that the email is still a known constant. **Why not fixed here:** branding decision belongs to `pick-a-name`; this convoy is credentials-only.

179
.convoys/drop-public-setup/brief-2-convert-setup-db-to-esm.md Normal file
View file

@ -0,0 +1,179 @@
---
convoy: drop-public-setup
brief_number: 2
depends_on:
- brief: 1
files:
- scripts/setup-neon-db.js
---
# Brief 2: Convert `scripts/setup-neon-db.js` from CJS to ESM
## Goal (1 sentence)
Convert `scripts/setup-neon-db.js` from CommonJS (`require()`) to native ES modules (`import`) so it runs on Node 22.x where `package.json` has `"type": "module"`; no functional changes — pure module-system conversion.
## Why this brief exists (context)
`bump-next-js` added `"type": "module"` to `package.json` so ESLint v9's flat config (`eslint.config.mjs`) could be picked up under the default loader semantics. As a side effect, every untyped `.js` file in the repo is now treated as ESM by Node. `scripts/setup-neon-db.js` still uses `require()` and immediately throws on Node 22.x:
```
ReferenceError: require is not defined in ES module scope
This file is being treated as an ES module because it has a '.js' file extension
and '/.../package.json' contains "type": "module".
```
Brief 1 added an `ADMIN_INITIAL_PASSWORD` env-var gate to this script, but the gate is theatrical until the script actually executes. This brief makes the script executable so brief 1's hardening takes effect.
The CJS→ESM conversion was originally flagged in `.convoys/drop-public-setup.md` § "Anything flagged but not acted on" #1 as a separate follow-up. After mid-convoy discovery (the implementer for brief 1 confirmed the script is non-functional today on Node 22.14.0), the scope was expanded to land both fixes together. See `.convoys/drop-public-setup.md` § Decisions D for the ratification.
## Files in scope (do not edit anything else)
- `scripts/setup-neon-db.js` — modified (module-system conversion only)
**Do NOT** edit `README.md`, `package.json`, `package-lock.json`, any other `scripts/*.js`, or any file under `lib/`, `pages/`, `components/`, `.github/`, or `.cursor/`. Brief 1's README changes already shipped on the convoy branch; do not touch them.
## Conventions to follow
- **No functional changes.** This brief is a 1:1 module-system conversion. Same logic, same DDL, same `process.exit(1)`, same env-var gate, same `console.log` lines. The diff should be 4-6 lines of `require` → `import` plus the `dotenv` invocation change.
- **No new dependencies.** `bcryptjs`, `dotenv`, and `@neondatabase/serverless` are already installed (verified by `npm run setup-db` failing on `require` rather than on missing modules).
- **Match brief 1's verbatim env-var-check block.** Do not touch the `if (!adminPassword || !adminPassword.trim())` block from brief 1 — only the `require()` calls around it change.
- **No top-level `await`.** ESM supports it, but the existing `setupNeonDatabase()` bottom-of-file invocation is intentionally fire-and-forget (the function does its own `try/catch` + `process.exit(1)`). Keep that pattern; do not introduce `await setupNeonDatabase()`.
- **Move `bcrypt` import to the top of the file.** The CJS version uses inline `const bcrypt = require('bcryptjs')` mid-function. ESM has no equivalent of late `require()` — all `import` statements must be at the top. This is a structural requirement of ESM, not a stylistic preference.
- **No `__dirname` or `__filename`.** The script does not currently use either, so no ESM equivalents (`import.meta.url`, `fileURLToPath`) are needed.
- **No `.cjs` rename.** The right answer is real ESM, not bypassing `"type": "module"` with a `.cjs` extension. A `.cjs` rename would also break the `setup-db` npm script (`node scripts/setup-neon-db.js`) without a corresponding `package.json` edit, which is out of scope here.
## Acceptance criteria
### `scripts/setup-neon-db.js`
- [ ] **Replace the `dotenv` invocation** (currently line 11):
```js
// before:
require('dotenv').config({ path: '.env.local' });
// after:
import dotenv from 'dotenv';
dotenv.config({ path: '.env.local' });
```
- [ ] **Replace the `neon` import** (currently line 13):
```js
// before:
const { neon } = require('@neondatabase/serverless');
// after:
import { neon } from '@neondatabase/serverless';
```
- [ ] **Hoist the `bcryptjs` import to the top of the file**, alongside the other imports. Remove the inline `const bcrypt = require('bcryptjs');` from inside `setupNeonDatabase()` (currently line 142):
```js
// at the top of the file, after the dotenv block:
import bcrypt from 'bcryptjs';
// inside setupNeonDatabase(), delete this line:
const bcrypt = require('bcryptjs');
```
- [ ] **Final import-block order at the top of the file** (after the shebang + JSDoc header):
```js
#!/usr/bin/env node
/**
* Neon Database Setup Script
* ... (existing JSDoc) ...
*/
import dotenv from 'dotenv';
dotenv.config({ path: '.env.local' });
import { neon } from '@neondatabase/serverless';
import bcrypt from 'bcryptjs';
async function setupNeonDatabase() {
// ... env-var check from brief 1, unchanged ...
// ... rest of function, with the inline require deleted ...
}
setupNeonDatabase();
```
**Note:** placing `dotenv.config(...)` *between* the import statements is intentional — `dotenv` must run before any other module reads `process.env`. ESM hoists `import` declarations but executes them in source order, so the `import dotenv from 'dotenv'` declaration hoists to the top, the `dotenv.config({...})` call executes after the import resolves, and the subsequent `import { neon }` / `import bcrypt` resolve after that. If the linter complains about "imports not grouped together," resolve by moving the `dotenv.config({...})` call into a separate side-effect import (`import 'dotenv/config'` will not work here because we need the custom `.env.local` path).
- [ ] **The env-var check from brief 1 is unchanged.** Lines 15-26 (the `const adminPassword = ...` block, the `if (!adminPassword || !adminPassword.trim())` block, and the `process.exit(1)` call) stay byte-identical. This brief only touches the import block at the top of the file and the inline `require('bcryptjs')` deletion.
- [ ] **The DDL, `INSERT`, `ON CONFLICT` clause, success summary `console.log` lines, and `try/catch/process.exit(1)` are all unchanged.** Pure module-system conversion.
- [ ] **`setupNeonDatabase();` at the bottom of the file stays as-is** (bare call, no `await`).
### Verification
- [ ] **Smoke: fail-loud path.** With `ADMIN_INITIAL_PASSWORD` unset:
```bash
unset ADMIN_INITIAL_PASSWORD
npm run setup-db
echo "exit=$?"
```
Expected:
- Exit code: `1`
- Stderr contains the brief-1 actionable error message (names `ADMIN_INITIAL_PASSWORD`, points at `.env.local`, suggests `openssl rand -base64 24`)
- **Stdout does NOT contain `✅ Connecting to Neon database...`** — the env-var check fires before `neon(...)`, just like in the brief-1 smoke
- **No `ReferenceError: require is not defined`** — this is the bug brief 2 fixes; its absence is the primary signal
- **No `SyntaxError: Cannot use import statement outside a module`** — would indicate the conversion picked the wrong direction
- [ ] **Smoke: happy path.** With the env var set:
```bash
export ADMIN_INITIAL_PASSWORD=brief-2-smoke-temporary-pw
npm run setup-db
echo "exit=$?"
```
Expected:
- Exit code: `0`
- Stdout contains `✅ Connecting to Neon database...` and `✅ Created admin user` (or the equivalent for an already-existing admin row — `ON CONFLICT DO NOTHING` keeps the script idempotent)
- Stdout does **NOT** contain `admin123` or `brief-2-smoke-temporary-pw` (R3 from brief 1 — credentials never echoed to stdout)
- No `ReferenceError`, no `SyntaxError`, no `import`/`require` complaints
- [ ] **Lint.** `npm run lint` exits 0 (or matches the existing baseline — pre-existing lint errors are fine; do not introduce new ones).
- [ ] **Tests.** `npm run test:run` is green. All 16 existing vitest tests should still pass — this brief does not touch any file they cover.
- [ ] **Grep sanity.** `rg 'require\(' scripts/setup-neon-db.js` returns **zero** hits after the conversion.
- [ ] **Brief-1 smoke still passes.** Re-run the brief-1 end-to-end login smoke against `npm run dev`:
```bash
curl -sS -X POST http://localhost:3000/api/auth/login \
-H 'content-type: application/json' \
-d '{"email":"admin@tcgvault.com","password":"brief-2-smoke-temporary-pw"}'
```
Expected: same R1 case-A behavior as brief 1 (if the local admin row predates `ADMIN_INITIAL_PASSWORD`, the new password 401s and the old `admin123` still 200s; if the admin row was newly created, the new password 200s with a JWT). Paste the result into the PR description.
### Pre-merge checklist (paste into PR description, alongside brief 1's checklist)
- [ ] **R1 callout (carried over from brief 1):** existing deployed admin rows with the weak `admin123` hash are NOT rotated by this PR. Operators rotate manually after merge.
- [ ] **Brief 2 makes `npm run setup-db` executable again on Node 22.x.** Before this brief, the script threw `ReferenceError: require is not defined in ES module scope` immediately on invocation. After this brief, the env-var gate from brief 1 actually fires.
- [ ] **Reviewer ran the brief-2 fail-loud and happy-path smoke locally** OR confirmed the PR description includes terminal output proving both paths.
### Out of scope (do not do these)
- [ ] No `package.json` change. `"type": "module"` stays; this brief makes the script align with that setting, not the other way around.
- [ ] No edit to `scripts/reset-db.js`, `scripts/create-test-users.js`, or any other `scripts/*.js`. They have the same CJS-in-ESM bug, but converting all of them widens the diff and bleeds into the no-go-zones rule for historical scripts. Queue `convert-helper-scripts-to-esm` as a separate convoy if/when those scripts need to run.
- [ ] No edit to `README.md`. Brief 1 owns the README changes; this brief is code-only.
- [ ] No `.cjs` rename. Real ESM only.
- [ ] No top-level `await` introduction at the bottom of the file.
- [ ] No new dependencies.
- [ ] No edit to `lib/`, `pages/`, `components/`, `.github/`, or `.cursor/`.
- [ ] No vitest tests. The smoke commands above are the verification; setup-db is not a runtime-auth surface (Decision C1 from brief 1 still applies — operational scripts use manual smoke, not unit tests).
## Rationale (≤3 sentences)
`bump-next-js`'s `"type": "module"` flag silently broke `npm run setup-db` on Node 22.x because the script still uses CJS `require()` calls. Brief 1's `ADMIN_INITIAL_PASSWORD` env-var gate is the right hardening but lands as theatrical security on a script no operator can actually execute — converting to native ESM imports here ensures the gate fires as intended and unblocks first-time DB setup on any Node 22+ environment (including Vercel's default). The diff is 4-6 lines of mechanical conversion with no functional changes, so review risk is minimal and the convoy stays a single coherent PR.