From 61d544bbd32f43bc663ed7a51e19b045d89ae122 Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Fri, 14 Aug 2026 21:13:03 -0300 Subject: [PATCH] fix(db): keep test runs off the operator's real DATA_DIR MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A script or test that opens the DB without setting DATA_DIR resolved to ~/.omniroute — the operator's live database, provider credentials included. tests/_setup/isolateDataDir.ts only covers the npm scripts; the documented single-file command and any ad-hoc probe bypass it. resolveWritableDataDir now redirects a test-context process with no DATA_DIR to a throwaway temp dir (stable per process), keeping the documented command working instead of failing it. OMNIROUTE_ALLOW_DEFAULT_DATA_DIR=1 opts back in. Closes #10428 --- src/lib/dataPaths.ts | 42 ++++++ .../datadir-test-context-guard-10428.test.ts | 120 ++++++++++++++++++ 2 files changed, 162 insertions(+) create mode 100644 tests/unit/datadir-test-context-guard-10428.test.ts diff --git a/src/lib/dataPaths.ts b/src/lib/dataPaths.ts index 5ad61ddbee..c73d139f36 100644 --- a/src/lib/dataPaths.ts +++ b/src/lib/dataPaths.ts @@ -83,12 +83,54 @@ export function resolveDataDir({ isCloud = false }: { isCloud?: boolean } = {}): * Use this only at the single startup site that owns directory creation * (currently `db/core.ts`); everywhere else keep using the pure resolver. */ +/** + * #10428: true when this process looks like a test run rather than a server start. + * + * `NODE_TEST_CONTEXT` is set by `node --test` in every spawned test process, `VITEST` by + * vitest, and `NODE_ENV=test` by the npm scripts — between them they cover both runners + * plus the AGENTS.md single-file command, which does NOT load + * `tests/_setup/isolateDataDir.ts`. + */ +function isTestContext(): boolean { + return ( + process.env.NODE_ENV === "test" || + !!process.env.VITEST || + !!process.env.NODE_TEST_CONTEXT || + process.execArgv.includes("--test") || + process.argv.includes("--test") + ); +} + +/** Process-wide redirect target, so repeated calls share one DB instead of one per call. */ +let testContextDataDir: string | null = null; + export function resolveWritableDataDir({ isCloud = false }: { isCloud?: boolean } = {}): string { const resolved = resolveDataDir({ isCloud }); // Cloud/serverless never owns a writable home dir; leave its sentinel alone. if (isCloud) return resolved; + // #10428: a test/ad-hoc run that never chose a DATA_DIR would otherwise open the + // OPERATOR'S REAL database (~/.omniroute/storage.sqlite — live provider credentials). + // Redirect to a throwaway dir instead of throwing: the documented single-file command + // (`node --import tsx/esm --test tests/unit/x.test.ts`) does not load the isolation + // setup, and a hard failure there would only teach people to disable the guard. + // `OMNIROUTE_ALLOW_DEFAULT_DATA_DIR=1` opts back in, so the intent is recorded. + if ( + !process.env.DATA_DIR && + isTestContext() && + process.env.OMNIROUTE_ALLOW_DEFAULT_DATA_DIR !== "1" + ) { + if (!testContextDataDir) { + testContextDataDir = fs.mkdtempSync(path.join(os.tmpdir(), `${APP_NAME}-testctx-`)); + console.warn( + `[DATA_DIR] test context without DATA_DIR → using '${testContextDataDir}' instead of ` + + `'${resolved}'. Set DATA_DIR explicitly (or load tests/_setup/isolateDataDir.ts) to silence this.` + ); + } + return testContextDataDir; + } + // No explicit override → already the default user dir; nothing to fall back to. const configured = normalizeConfiguredPath(process.env.DATA_DIR); if (!configured) return resolved; diff --git a/tests/unit/datadir-test-context-guard-10428.test.ts b/tests/unit/datadir-test-context-guard-10428.test.ts new file mode 100644 index 0000000000..e957a490fa --- /dev/null +++ b/tests/unit/datadir-test-context-guard-10428.test.ts @@ -0,0 +1,120 @@ +import test from "node:test"; +import assert from "node:assert/strict"; +import os from "node:os"; +import path from "node:path"; +import fs from "node:fs"; + +/** + * #10428 — a script or test that opens the DB without setting DATA_DIR resolves to the + * operator's REAL database (`~/.omniroute/storage.sqlite`, credentials included). + * `tests/_setup/isolateDataDir.ts` protects the npm test scripts, but it is opt-in per + * invocation: the AGENTS.md-documented single-file command + * (`node --import tsx/esm --test tests/unit/x.test.ts`) does NOT load it, and neither does + * an ad-hoc `node --import tsx probe.ts`. + * + * The guard therefore lives at the one place that actually opens the DB + * (`resolveWritableDataDir`, consumed only by `src/lib/db/core.ts`): in a test context + * pointing at the default user data dir, it redirects to a throwaway temp dir instead of + * touching the real one. Redirecting rather than throwing keeps the documented + * single-file command working — a hard failure there would just teach people to unset the + * guard. + */ + +const { resolveWritableDataDir, getDefaultDataDir } = await import("../../src/lib/dataPaths.ts"); + +function withEnv(overrides: Record, run: () => void) { + const saved: Record = {}; + for (const [key, value] of Object.entries(overrides)) { + saved[key] = process.env[key]; + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + try { + run(); + } finally { + for (const [key, value] of Object.entries(saved)) { + if (value === undefined) delete process.env[key]; + else process.env[key] = value; + } + } +} + +test("G1: a test context with no DATA_DIR never resolves to the operator's real data dir", () => { + withEnv({ DATA_DIR: undefined, NODE_ENV: "test", OMNIROUTE_ALLOW_DEFAULT_DATA_DIR: undefined }, () => { + const resolved = resolveWritableDataDir(); + assert.notEqual( + resolved, + getDefaultDataDir(), + "a test run must never be handed the operator's real DATA_DIR" + ); + assert.ok( + resolved.startsWith(os.tmpdir()), + `expected a throwaway temp dir, got ${resolved}` + ); + assert.ok(fs.existsSync(resolved), "the redirected dir must exist and be usable"); + }); +}); + +test("G2: an explicit DATA_DIR still wins inside a test context", () => { + const explicit = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-explicit-")); + withEnv({ DATA_DIR: explicit, NODE_ENV: "test" }, () => { + assert.equal(resolveWritableDataDir(), explicit); + }); + fs.rmSync(explicit, { recursive: true, force: true }); +}); + +test("G3: the escape hatch restores the old behavior for deliberate runs", () => { + withEnv( + { DATA_DIR: undefined, NODE_ENV: "test", OMNIROUTE_ALLOW_DEFAULT_DATA_DIR: "1" }, + () => { + assert.equal( + resolveWritableDataDir(), + getDefaultDataDir(), + "an explicit opt-in must still reach the real dir, so the intent is recorded" + ); + } + ); +}); + +test("G4: a normal server run (no test markers) is untouched", () => { + withEnv( + { + DATA_DIR: undefined, + NODE_ENV: "production", + VITEST: undefined, + NODE_TEST_CONTEXT: undefined, + OMNIROUTE_ALLOW_DEFAULT_DATA_DIR: undefined, + }, + () => { + assert.equal( + resolveWritableDataDir(), + getDefaultDataDir(), + "the server must keep resolving to the real data dir" + ); + } + ); +}); + +test("G5: node:test subprocesses are detected through NODE_TEST_CONTEXT too", () => { + withEnv( + { + DATA_DIR: undefined, + NODE_ENV: undefined, + NODE_TEST_CONTEXT: "child-v8", + OMNIROUTE_ALLOW_DEFAULT_DATA_DIR: undefined, + }, + () => { + const resolved = resolveWritableDataDir(); + assert.notEqual(resolved, getDefaultDataDir()); + assert.ok(resolved.startsWith(os.tmpdir())); + } + ); +}); + +test("G6: the redirect is stable within a process (same dir on repeated calls)", () => { + withEnv({ DATA_DIR: undefined, NODE_ENV: "test" }, () => { + const first = resolveWritableDataDir(); + const second = resolveWritableDataDir(); + assert.equal(first, second, "a per-call temp dir would split the DB across handles"); + }); +});