From ee0367dfb5fc01b8c8141e84f2d56efc667add57 Mon Sep 17 00:00:00 2001 From: Thomas Joise Date: Tue, 21 Jul 2026 00:14:20 +0530 Subject: [PATCH] fix(db-pg): make requireDbUrlForEnv guard outcome-based, not NODE_ENV-gated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The guard only threw when NODE_ENV==='production' AND resolution came back empty. But NODE_ENV is exactly what isn't reliably set for the maintenance scripts this guards (they're run by hand from an operator shell with no service-definition env and no dotenv), so on the real prod box the guard never fired: resolution silently returned '', the script opened a brand-new empty SQLite file under /data, and printed a false "0 project(s) rebuilt" success. Now the guard fires on the OUTCOME instead: whenever resolution selects SQLite, refuse to run unless the target file already exists — a migration/maintenance script must never CREATE a database. Applies only to requireDbUrlForEnv (the script entry path); server.ts calls resolveDbUrl directly and is untouched, so first boot still creates its DB as before. Co-Authored-By: Claude Opus 4.8 (1M context) --- apps/hq/src/db-pg.ts | 44 ++++++++++++++++++------- apps/hq/test/db-resolve.test.ts | 57 +++++++++++++++++++++++++++++---- 2 files changed, 83 insertions(+), 18 deletions(-) diff --git a/apps/hq/src/db-pg.ts b/apps/hq/src/db-pg.ts index c467b24..54a5726 100644 --- a/apps/hq/src/db-pg.ts +++ b/apps/hq/src/db-pg.ts @@ -1,6 +1,7 @@ import { AsyncLocalStorage } from 'node:async_hooks' +import fs from 'node:fs' import pg from 'pg' -import type { DB } from './db' +import { sqliteFilePath, type DB } from './db' import { PG_MIGRATIONS } from './migrations-pg' /** @@ -124,19 +125,38 @@ export function resolveDbUrl(env: NodeJS.ProcessEnv = process.env, hardcoded = H } /** - * Same resolution, but hard-fails instead of silently falling back to SQLite when running in - * production — for one-off deploy scripts, where a throwaway SQLite write is never the right - * outcome (unlike the server, which legitimately runs SQLite in dev). `hardcoded` is - * injectable so tests can simulate the hardcoded URL being accidentally emptied out. + * Same resolution, but hard-fails instead of silently falling back to a THROWAWAY SQLite + * database — for one-off deploy/maintenance scripts, where creating a brand-new empty DB is + * never the right outcome (unlike the server, which legitimately creates one on first boot in + * dev — this function is never called from server.ts). + * + * The guard is OUTCOME-based, not NODE_ENV-based. The original fix gated the hard-fail on + * `NODE_ENV === 'production'`, but NODE_ENV is exactly what these scripts don't reliably have: + * the server gets it from its service definition, while the scripts are run by hand + * (`npx tsx apps/hq/scripts/…`) from an operator shell with no NODE_ENV and no dotenv. So on + * the real prod box the old guard never fired, resolution returned `''`, and the script + * silently opened/created a brand-new empty SQLite file in `/data` — "succeeding" against + * a throwaway DB while production was never touched. + * + * Fix: whenever resolution selects SQLite, refuse UNLESS the target file already exists — a + * migration/maintenance script must never CREATE a database, regardless of what NODE_ENV is or + * isn't set to. `hardcoded` and `sqliteExists` are both injectable so tests never touch a real + * Postgres URL or the real filesystem. */ -export function requireDbUrlForEnv(env: NodeJS.ProcessEnv = process.env, hardcoded = HARDCODED_DATABASE_URL): string { +export function requireDbUrlForEnv( + env: NodeJS.ProcessEnv = process.env, + hardcoded = HARDCODED_DATABASE_URL, + sqliteExists: (file: string) => boolean = fs.existsSync, +): string { const pgUrl = resolveDbUrl(env, hardcoded) - if (pgUrl === '' && env['NODE_ENV'] === 'production') { - throw new Error( - '[db] refusing to run: NODE_ENV=production but no Postgres target resolved ' + - '(DATABASE_URL is unset and the hardcoded production URL is empty) — this would ' + - 'otherwise silently create a throwaway SQLite database instead of touching production data.', - ) + if (pgUrl === '') { + const file = sqliteFilePath(env['HQ_DATA_DIR']) + if (file !== ':memory:' && !sqliteExists(file)) { + throw new Error( + `[db] refusing to run: this would CREATE a new SQLite database at ${file}. ` + + 'Set DATABASE_URL to target Postgres, or point HQ_DATA_DIR at the existing database.', + ) + } } return pgUrl } diff --git a/apps/hq/test/db-resolve.test.ts b/apps/hq/test/db-resolve.test.ts index 8bd38bd..ff17361 100644 --- a/apps/hq/test/db-resolve.test.ts +++ b/apps/hq/test/db-resolve.test.ts @@ -4,6 +4,9 @@ // the scripts computed it as `DATABASE_URL ?? ''` (no production fallback), so on the // production box — where DATABASE_URL is unset — each script silently opened a brand-new // empty SQLite file instead of the real Postgres. +import fs from 'node:fs' +import os from 'node:os' +import path from 'node:path' import { describe, it, expect } from 'vitest' import { pgTargetDescription, requireDbUrlForEnv, resolveDbUrl } from '../src/db-pg' @@ -26,16 +29,58 @@ describe('resolveDbUrl (D19 engine selection, the one shared resolver)', () => { }) }) -describe('requireDbUrlForEnv (deploy-script guard — hard-fail instead of a throwaway SQLite write)', () => { - it('behaves exactly like resolveDbUrl when a Postgres target is available', () => { +describe('requireDbUrlForEnv (deploy-script guard — OUTCOME-based, not NODE_ENV-based)', () => { + it('behaves exactly like resolveDbUrl when a Postgres target is available (no SQLite file check runs at all)', () => { expect(requireDbUrlForEnv({ DATABASE_URL: 'postgres://x/y' })).toBe('postgres://x/y') expect(requireDbUrlForEnv({ NODE_ENV: 'production' }, HARDCODED)).toBe(HARDCODED) - expect(requireDbUrlForEnv({})).toBe('') // dev with no env: SQLite is fine, no hard-fail }) - it('hard-fails instead of silently returning SQLite when NODE_ENV=production resolves nothing', () => { - // Simulates the hardcoded URL having been accidentally emptied out. - expect(() => requireDbUrlForEnv({ NODE_ENV: 'production' }, '')).toThrow(/refusing to run/i) + it('resolves to SQLite without hard-failing when the target file already exists (mocked fs check)', () => { + expect(requireDbUrlForEnv({}, undefined, () => true)).toBe('') + // Even NODE_ENV=production is fine as long as the SQLite file it would land on already exists. + expect(requireDbUrlForEnv({ NODE_ENV: 'production' }, '', () => true)).toBe('') + }) + + it(':memory: never hard-fails — the guard never even calls the file-existence check', () => { + expect( + requireDbUrlForEnv({ HQ_DATA_DIR: ':memory:' }, undefined, () => { + throw new Error('must not be called for :memory:') + }), + ).toBe('') + }) + + it('refuses when resolution selects SQLite and the target does not exist — regardless of NODE_ENV', () => { + // This is the original prod bug: an operator shell has no NODE_ENV set at all, so the old + // NODE_ENV==='production' gate never fired. The guard must fire on the OUTCOME instead. + expect(() => requireDbUrlForEnv({}, undefined, () => false)).toThrow(/refusing to run/i) + expect(() => requireDbUrlForEnv({}, undefined, () => false)).toThrow(/CREATE a new SQLite database/) + // Also still refuses in the NODE_ENV=production case (hardcoded emptied out + no existing file). + expect(() => requireDbUrlForEnv({ NODE_ENV: 'production' }, '', () => false)).toThrow(/refusing to run/i) + }) +}) + +describe('requireDbUrlForEnv — real filesystem (the actual script entry path, no mocks)', () => { + it('refuses a non-existent SQLite path', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hq-db-resolve-')) + try { + expect(() => requireDbUrlForEnv({ HQ_DATA_DIR: dir })).toThrow(/refusing to run/i) + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it('accepts an existing SQLite path', () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'hq-db-resolve-')) + try { + fs.writeFileSync(path.join(dir, 'hq.db'), '') + expect(requireDbUrlForEnv({ HQ_DATA_DIR: dir })).toBe('') + } finally { + fs.rmSync(dir, { recursive: true, force: true }) + } + }) + + it(':memory: still works with no filesystem check', () => { + expect(requireDbUrlForEnv({ HQ_DATA_DIR: ':memory:' })).toBe('') }) })