From 764a4aee02f47dfcea3f1fc0abc9d78195dc439c Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Sat, 18 Jul 2026 22:24:55 -0300 Subject: [PATCH] fix(base-red): fix execArgv test-env leak masking mass-migration abort + heal legacy refresh_token before index MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent bugs, not migration 126: 1. tests/unit/db-migration-runner.test.ts and tests/unit/migration-safety-abort-6260.test.ts: withNonTestEnvironment() only sanitized process.argv, not process.execArgv. #7359 made isAutomatedTestProcess() also scan execArgv (to catch `node --test`), so under the node:test runner execArgv always retains `--test` and the "simulate a non-test environment" helper became a no-op. The mass-migration safety-abort check (gated on !isTestEnvironment) never fired, migrations ran for real, and hit the hardcoded version-032 apikey-lifecycle special case against fixtures that never created api_keys — surfacing as "no such table: api_keys" instead of the expected MigrationSafetyAbortError. Fix: also strip test-token args from process.execArgv in the test helper. 2. tests/unit/db-core-init.test.ts: SCHEMA_SQL created idx_pc_auth_active_refresh on provider_connections(refresh_token) unconditionally, before ensureProviderConnectionsColumns() ran its column-healing pass — and that function never healed refresh_token in the first place. A legacy provider_connections table predating that column (simulated by the "max_concurrent column is healed" fixture) fails startup with "no such column: refresh_token" instead of healing. Fix: move the index into ensureProviderConnectionsColumns(), after adding a defensive refresh_token backfill. --- src/lib/db/core.ts | 1 - src/lib/db/schemaColumns.ts | 10 ++++++++++ tests/unit/db-migration-runner.test.ts | 8 ++++++++ tests/unit/migration-safety-abort-6260.test.ts | 7 +++++++ 4 files changed, 25 insertions(+), 1 deletion(-) diff --git a/src/lib/db/core.ts b/src/lib/db/core.ts index 165ecae456..f798e8d455 100644 --- a/src/lib/db/core.ts +++ b/src/lib/db/core.ts @@ -250,7 +250,6 @@ const SCHEMA_SQL = ` CREATE INDEX IF NOT EXISTS idx_pc_provider ON provider_connections(provider); CREATE INDEX IF NOT EXISTS idx_pc_active ON provider_connections(is_active); CREATE INDEX IF NOT EXISTS idx_pc_priority ON provider_connections(provider, priority); - CREATE INDEX IF NOT EXISTS idx_pc_auth_active_refresh ON provider_connections(auth_type, is_active, refresh_token); CREATE TABLE IF NOT EXISTS provider_nodes ( id TEXT PRIMARY KEY, diff --git a/src/lib/db/schemaColumns.ts b/src/lib/db/schemaColumns.ts index 2951e214e8..3381b7e4b3 100644 --- a/src/lib/db/schemaColumns.ts +++ b/src/lib/db/schemaColumns.ts @@ -62,9 +62,19 @@ export function ensureProviderConnectionsColumns(db: SqliteDatabase) { db.exec("ALTER TABLE provider_connections ADD COLUMN rate_limit_overrides_json TEXT"); console.log("[DB] Added provider_connections.rate_limit_overrides_json column"); } + // `refresh_token` is part of 001_initial_schema.sql, but `CREATE TABLE IF NOT EXISTS` + // is a no-op on a pre-existing legacy table that predates it — heal it defensively + // before the index below relies on it, or a very old DB fails startup entirely. + if (!columnNames.has("refresh_token")) { + db.exec("ALTER TABLE provider_connections ADD COLUMN refresh_token TEXT"); + console.log("[DB] Added provider_connections.refresh_token column"); + } db.exec( "CREATE INDEX IF NOT EXISTS idx_pc_max_concurrent ON provider_connections(provider, max_concurrent)" ); + db.exec( + "CREATE INDEX IF NOT EXISTS idx_pc_auth_active_refresh ON provider_connections(auth_type, is_active, refresh_token)" + ); } catch (error: unknown) { const message = error instanceof Error ? error.message : String(error); console.warn("[DB] Failed to verify provider_connections schema:", message); diff --git a/tests/unit/db-migration-runner.test.ts b/tests/unit/db-migration-runner.test.ts index 2c830969c0..9bbff9f5ce 100644 --- a/tests/unit/db-migration-runner.test.ts +++ b/tests/unit/db-migration-runner.test.ts @@ -127,16 +127,24 @@ function withNonTestEnvironment(fn) { const originalVitest = process.env.VITEST; const originalDisableAutoBackup = process.env.DISABLE_SQLITE_AUTO_BACKUP; const originalArgv = [...process.argv]; + const originalExecArgv = [...process.execArgv]; delete process.env.NODE_ENV; delete process.env.VITEST; delete process.env.DISABLE_SQLITE_AUTO_BACKUP; process.argv = process.argv.filter((arg) => !arg.includes("test")); + // #7359 made isAutomatedTestProcess() also scan process.execArgv (so `node --test` + // is caught even when NODE_ENV/VITEST/argv are clean). This harness runs under + // `node --test`, so execArgv always carries `--test` — strip it here too, or the + // "non-test" simulation is a no-op and the mass-migration safety checks under + // test never actually exercise their real-environment code path. + process.execArgv = process.execArgv.filter((arg) => !arg.includes("test")); try { return fn(); } finally { process.argv = originalArgv; + process.execArgv = originalExecArgv; if (originalNodeEnv === undefined) delete process.env.NODE_ENV; else process.env.NODE_ENV = originalNodeEnv; diff --git a/tests/unit/migration-safety-abort-6260.test.ts b/tests/unit/migration-safety-abort-6260.test.ts index ae649116e0..eaea742e23 100644 --- a/tests/unit/migration-safety-abort-6260.test.ts +++ b/tests/unit/migration-safety-abort-6260.test.ts @@ -66,16 +66,23 @@ function withNonTestEnvironment(fn: () => T): T { const originalVitest = process.env.VITEST; const originalDisableAutoBackup = process.env.DISABLE_SQLITE_AUTO_BACKUP; const originalArgv = [...process.argv]; + const originalExecArgv = [...process.execArgv]; delete process.env.NODE_ENV; delete process.env.VITEST; delete process.env.DISABLE_SQLITE_AUTO_BACKUP; process.argv = process.argv.filter((arg) => !arg.includes("test")); + // #7359 made isAutomatedTestProcess() also scan process.execArgv (so `node --test` + // is caught even when NODE_ENV/VITEST/argv are clean). This harness runs under + // `node --test`, so execArgv always carries `--test` — strip it here too, or the + // "non-test" simulation is a no-op. + process.execArgv = process.execArgv.filter((arg) => !arg.includes("test")); try { return fn(); } finally { process.argv = originalArgv; + process.execArgv = originalExecArgv; if (originalNodeEnv === undefined) delete process.env.NODE_ENV; else process.env.NODE_ENV = originalNodeEnv; if (originalVitest === undefined) delete process.env.VITEST;