fix(setup): rescue the semicolon from the comment; assert the child SAW the override

Two review nits on the way in.

setup.mjs:393 — the statement's semicolon had been swallowed INTO the trailing comment, so
the line parsed only because ASI rescued it (the next token is `if` on a new line). The repo
has no linter, so nothing would have caught it. Comment moved above the statement.

test-features.mjs — negative control on the prod-gate probe. The reviewer noticed the test's
robustness was INCIDENTAL: because the child env is spread from process.env, it inherits the
parent's own OCP_DIR_OVERRIDE, so a future refactor that renamed the var and missed this test's
explicit `env` object would still have gone red — but by luck, not by assertion. The child now
prints the override it SAW as well as the store it opened, and the test asserts both. The claim
is now 'a prod process saw the override and ignored it', not merely 'a prod process opened the
right store' (which could pass for the wrong reason).

Mutation re-proven after both edits: delete the NODE_ENV gate -> 319 passed, 1 failed; restore
-> 320 passed, 0 failed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR

keys.mjs

@@ -6,26 +6,64 @@ import { join } from "node:path";
 import { mkdirSync, chmodSync } from "node:fs";
 import { homedir } from "node:os";
 
-const OCP_DIR = join(homedir(), ".ocp");
-mkdirSync(OCP_DIR, { recursive: true, mode: 0o700 });
-// Tighten the directory mode in case it already existed with broader permissions.
-try { chmodSync(OCP_DIR, 0o700); } catch { /* ignore EPERM on pre-existing dirs */ }
-const DB_PATH = join(OCP_DIR, "ocp.db");
+// Resolved LAZILY, on first getDb() — not at module top-level. Two reasons, and the second is
+// the bug this fixes:
+//
+//  1. Merely IMPORTING keys.mjs should not, as a side effect, create directories in the
+//     operator's home.
+//  2. OCP_DIR_OVERRIDE exists so the test suite can point the key store at a scratch dir — and
+//     because ESM hoists imports, a top-level `const OCP_DIR = ...` here would be evaluated
+//     BEFORE an importing module's body could set the env var. Eager resolution made the
+//     override unsettable in the one place that needs it. (test-features.mjs carried a comment
+//     claiming it could "set env before the first getDb() call" — it could not, because nothing
+//     here ever read an env var. So `npm test` wrote real, UNREVOKED api_keys rows into the
+//     operator's live ~/.ocp/ocp.db: two per run, unbounded — 737 junk keys against 12 real ones
+//     on the maintainer's host — and two concurrent runs raced one file, which is the ~1-in-6
+//     flake in `listKeys includes quota fields`.)
+//
+// The override is gated on NODE_ENV === "test", and that gate is the ACTUAL guard. An earlier
+// cut of this fix relied on the variable merely having an awkward name — i.e. a naming convention
+// plus a comment — which is precisely the failure mode this whole change exists to indict (a
+// comment describing an intention that nothing enforces). A production server runs without
+// NODE_ENV, so it CANNOT honor the override, however the variable got into its environment
+// (`ocp start`'s nohup fallback inherits the invoking shell's env — a maintainer who exported
+// this while debugging and then started the server would otherwise get a server silently
+// authenticating against an empty key store: in AUTH_MODE=multi, a total auth outage, with
+// nothing logged and nothing on /health to show it).
+function resolveOcpDir() {
+  const override = process.env.NODE_ENV === "test" ? process.env.OCP_DIR_OVERRIDE : null;
+  const dir = override || join(homedir(), ".ocp");
+  mkdirSync(dir, { recursive: true, mode: 0o700 });
+  // Tighten the directory mode in case it already existed with broader permissions.
+  try { chmodSync(dir, 0o700); } catch { /* ignore EPERM on pre-existing dirs */ }
+  return dir;
+}
 
 let db;
+let dbPath;   // resolved on first open, alongside the db handle
 
 export function getDb() {
   if (!db) {
-    db = new DatabaseSync(DB_PATH);
+    dbPath = join(resolveOcpDir(), "ocp.db");
+    // Say which store we opened. Silence was the other half of the bug: a server on the wrong
+    // key store looks exactly like a server on the right one until every request 401s.
+    if (dbPath !== join(homedir(), ".ocp", "ocp.db")) {
+      console.error(`[keys] key store: ${dbPath} (NOT the default ~/.ocp/ocp.db)`);
+    }
+    db = new DatabaseSync(dbPath);
     db.exec("PRAGMA journal_mode = WAL");
     db.exec("PRAGMA foreign_keys = ON");
     initSchema();
     // Tighten mode on the DB file (0600) after creation / first open.
-    try { chmodSync(DB_PATH, 0o600); } catch { /* ignore — same-user access still works */ }
+    try { chmodSync(dbPath, 0o600); } catch { /* ignore — same-user access still works */ }
   }
   return db;
 }
 
+// Which file the key store actually opened. Exported so a test can ASSERT it is not the
+// operator's real db — the bug this replaced was invisible precisely because nothing checked.
+export function getDbPath() { return dbPath; }
+
 function initSchema() {
   db.exec(`
     CREATE TABLE IF NOT EXISTS api_keys (
@@ -426,5 +464,5 @@ export function findKey(idOrName) {
 }
 
 export function closeDb() {
-  if (db) { db.close(); db = null; }
+  if (db) { db.close(); db = null; dbPath = undefined; }  // clear both — a path to a closed db is a footgun
 }

server.mjs

@@ -474,7 +474,12 @@ const SPAWN_HOME_DIR = `${process.env.HOME}/.ocp/spawn-home`;
 // erroring loudly — never a silent auth/credential corruption (there are no credentials here).
 function prepareSpawnHome(dir = SPAWN_HOME_DIR) {
   try {
-    mkdirSync(`${dir}/.claude`, { recursive: true });
+    // mode 0700, and it matters for the PARENT: with `recursive`, this call can create ~/.ocp
+    // itself on a fresh install (spawn homes live under it), and without an explicit mode that
+    // parent lands at the umask default — world-listable 0755. keys.mjs used to pre-create it
+    // 0700 as an import side effect; it no longer does (it resolves its dir lazily), so the
+    // 0700 guarantee has to be stated here rather than inherited by luck.
+    mkdirSync(`${dir}/.claude`, { recursive: true, mode: 0o700 });
     // Belt-and-braces: ensure no settings.json/plugins leak in (this home is fully ours).
     for (const f of [`${dir}/.claude/settings.json`, `${dir}/.claude/settings.local.json`]) {
       try { if (existsSync(f)) rmSync(f, { force: true }); } catch { /* best effort */ }

setup.mjs

@@ -390,7 +390,9 @@ if (!DRY_RUN) {
   // and "ocp-proxy" keeps the proxy invisible to that heuristic.
   const OCP_HOME = join(HOME, ".ocp");
   const ocpLogsDir = join(OCP_HOME, "logs");
-  if (!existsSync(ocpLogsDir)) mkdirSync(ocpLogsDir, { recursive: true });
+  // mode 0700: with `recursive`, this call can create ~/.ocp ITSELF on a fresh install, and
+  // without an explicit mode that parent lands at the umask default (world-listable 0755).
+  if (!existsSync(ocpLogsDir)) mkdirSync(ocpLogsDir, { recursive: true, mode: 0o700 });
 
   // Uninstall legacy service names if present (upgrade path)
   if (platform === "darwin") {

test-env.mjs

@@ -0,0 +1,25 @@
+// Imported FIRST by test-features.mjs, before keys.mjs, so this runs before anything can open
+// the key store. ESM hoists imports and evaluates them in order, so a `process.env.X = ...`
+// statement in the test's own body would run too late — hence a separate module.
+//
+// Why this exists: `npm test` used to write real, UNREVOKED api_keys rows into the operator's
+// live ~/.ocp/ocp.db (the same database the running server reads) — two per run, unbounded.
+// It also made the suite racy: two concurrent runs (e.g. review worktrees) shared one file, so
+// `listKeys()` could miss "test-user-1" and the `in` check would throw on undefined.
+import { mkdtempSync, rmSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+export const TEST_OCP_DIR = mkdtempSync(join(tmpdir(), "ocp-test-"));
+
+// BOTH are required. keys.mjs honors OCP_DIR_OVERRIDE only when NODE_ENV === "test", so that a
+// production server — which runs without NODE_ENV — cannot be redirected onto a different key
+// store no matter how the variable reached its environment.
+process.env.NODE_ENV = "test";
+process.env.OCP_DIR_OVERRIDE = TEST_OCP_DIR;
+
+// Remove the scratch store on exit. Without this the fix would trade unbounded growth in
+// ~/.ocp/ocp.db for unbounded growth in $TMPDIR — better, but still litter.
+process.on("exit", () => {
+  try { rmSync(TEST_OCP_DIR, { recursive: true, force: true }); } catch { /* best effort */ }
+});

test-features.mjs

@@ -3,22 +3,24 @@
  * Integration test for Quota + Cache features.
  * Tests database layer functions directly — no server needed.
  */
-import { getDb, createKey, listKeys, validateKey, recordUsage, checkQuota, updateKeyQuota, getKeyQuota, findKey, cacheHash, getCachedResponse, setCachedResponse, clearCache, getCacheStats, closeDb, hasCacheControl, singleflight, getInflightStats } from "./keys.mjs";
+// MUST come before keys.mjs: redirects the key store to a scratch dir (see test-env.mjs).
+import { TEST_OCP_DIR } from "./test-env.mjs";
+import { getDb, getDbPath, createKey, listKeys, validateKey, recordUsage, checkQuota, updateKeyQuota, getKeyQuota, findKey, cacheHash, getCachedResponse, setCachedResponse, clearCache, getCacheStats, closeDb, hasCacheControl, singleflight, getInflightStats } from "./keys.mjs";
 import { isLoopbackBind } from "./lib/net.mjs";
 import { createSerialMutex, createTtlCache, isTokenExpiring, orderLabelsLastGoodFirst } from "./lib/spawn-auth.mjs";
 import { createHash } from "node:crypto";
 import { strict as assert } from "node:assert";
-import { unlinkSync } from "node:fs";
 import { join } from "node:path";
+import { pathToFileURL } from "node:url";
+import { execFileSync } from "node:child_process";
 import { homedir } from "node:os";
 
-// Use a test database to avoid corrupting real data
-const TEST_DB = join(homedir(), ".ocp", "ocp-test.db");
-try { unlinkSync(TEST_DB); } catch {}
+process.env.HOME = homedir(); // normalize HOME so homedir()-derived paths are stable across shells
 
-// Monkey-patch DB_PATH for testing (override the module-level variable)
-// Since keys.mjs uses lazy init, we can set env before first getDb() call
-process.env.HOME = homedir(); // ensure consistent
+// The scaffolding that used to live here CLAIMED to use "a test database to avoid corrupting
+// real data" by setting an env var before the first getDb(). It never worked: keys.mjs read no
+// env var, and ESM hoisting meant the assignment ran after the import anyway. The redirect is
+// now real, and lives in test-env.mjs (imported above, before keys.mjs). This test proves it.
 
 let passed = 0;
 let failed = 0;
@@ -3811,6 +3813,60 @@ test("REGRESSION: a WARM (pooled) pane streams — the sink comes off the pane,
   assert.equal(out.text, "Hello world", "and the transcript stays authoritative for the final text");
 });
 
+console.log("\nTest isolation (the suite must never touch the operator's live key store):");
+
+test("the key store under test is a scratch db, NOT the operator's real ~/.ocp/ocp.db", () => {
+  // The guard that was missing. `npm test` wrote live, UNREVOKED api_keys rows straight into the
+  // operator's real ~/.ocp/ocp.db — the same database the running server reads — two per run,
+  // unbounded (737 junk keys vs 12 real ones on the maintainer's host before this landed). It
+  // went unnoticed for so long precisely because NOTHING asserted where the store actually was.
+  const real = join(homedir(), ".ocp", "ocp.db");
+  const used = getDbPath();
+  assert.ok(used, "getDb() must have opened something by now");
+  assert.notEqual(used, real, "the suite must NOT open the operator's live key database");
+  assert.ok(used.startsWith(TEST_OCP_DIR), `expected a scratch db under ${TEST_OCP_DIR}, got ${used}`);
+});
+
+test("a PRODUCTION process (no NODE_ENV) must IGNORE OCP_DIR_OVERRIDE", () => {
+  // Must run OUT OF PROCESS. The parent is irreversibly NODE_ENV=test by the time any test runs
+  // (test-env.mjs set it before keys.mjs was imported), so the production path is unreachable
+  // from in here — and an in-process test can only ever RE-IMPLEMENT the predicate, which is
+  // worthless: the first cut of this test did exactly that, and deleting the whole NODE_ENV gate
+  // from keys.mjs still left the suite at 320 passed / 0 failed. A copy of the predicate is not
+  // the predicate. So: spawn a child with no NODE_ENV, the override set, and HOME redirected to
+  // a temp dir (so the real key store is never opened), and assert what the REAL keys.mjs did.
+  const home = mkdtempSync(join(tmpdir(), "ocp-prodsim-"));
+  const evil = mkdtempSync(join(tmpdir(), "ocp-evil-"));
+  try {
+    const keysUrl = pathToFileURL(join(import.meta.dirname, "keys.mjs")).href;
+    // The child prints the override it SAW, then the store it actually opened. Printing both is
+    // the negative control: without it, a future refactor that renamed the env var and missed
+    // this test's `env` object would leave the child with no override at all — and "prod opened
+    // the right store" would pass for the wrong reason. Asserting the child saw it and ignored
+    // it anyway is the claim we actually want to make.
+    const probe = `import { getDb, getDbPath, closeDb } from ${JSON.stringify(keysUrl)};
+getDb(); process.stdout.write(process.env.OCP_DIR_OVERRIDE + "\\n" + getDbPath()); closeDb();`;
+    const env = { ...process.env, HOME: home, OCP_DIR_OVERRIDE: evil };
+    delete env.NODE_ENV;                       // a production server has no NODE_ENV
+    const [seen, opened] = execFileSync(process.execPath, ["--input-type=module", "-e", probe],
+      { env, encoding: "utf8" }).trim().split("\n");
+    assert.equal(seen, evil, "precondition: the child must actually SEE the override");
+    assert.equal(opened, join(home, ".ocp", "ocp.db"), "a prod process must open HOME/.ocp/ocp.db");
+    assert.ok(!opened.startsWith(evil), "…having seen the override, a prod process must IGNORE it");
+  } finally {
+    rmSync(home, { recursive: true, force: true });
+    rmSync(evil, { recursive: true, force: true });
+  }
+});
+
+test("listKeys does not depend on rows left behind by an earlier or concurrent run", () => {
+  // The ~1-in-6 flake: two runs sharing one db file. keys.find() returned undefined and the
+  // caller's `in` check threw a TypeError instead of failing cleanly. With a per-run scratch db
+  // the store starts empty, so the count is exactly what THIS run created.
+  const mine = listKeys().filter((k) => k.name === "test-user-1");
+  assert.equal(mine.length, 1, "exactly one test-user-1 — a shared store would accumulate duplicates");
+});
+
 runAsyncTests().then(() => Promise.all(pendingAsync)).then(() => {
   closeDb();
   console.log(`\n=== Results: ${passed} passed, ${failed} failed ===\n`);