fix(test): stop the suite writing live API keys into the operator's real key store (#163)

* fix(test): stop the suite writing live API keys into the operator's real key store

`npm test` wrote real, UNREVOKED rows into ~/.ocp/ocp.db — the SAME sqlite database the
running server reads — two per run, unbounded. On the maintainer's host that had accumulated
**737 test-suite keys against 12 real operator keys** (749 rows total), all revoked=0.

Not a credential leak: createKey() mints `randomBytes(24)` and the suite discards the
plaintext, so nobody holds a usable token. But it is real damage:
  - the operator's key store grows by 2 rows on every test run, forever
  - `ocp keys list` is unusable (749 rows, 12 of them real)
  - the suite is RACY: two concurrent runs (e.g. two review worktrees) share one file, so
    listKeys() can miss "test-user-1" and `"quota_daily" in undefined` throws a TypeError
    rather than failing cleanly. That is the ~1-in-6 flake in `listKeys includes quota
    fields`, reported by a reviewer and initially not reproducible serially — it needs a
    concurrent run to surface, which is exactly what four parallel reviewers produced.

Root cause: keys.mjs resolved `OCP_DIR`/`DB_PATH` at MODULE TOP-LEVEL and read no env var.
test-features.mjs carried a comment claiming it could "set env before the first getDb() call"
— it could not, on two counts: nothing in keys.mjs read an env var, and ESM hoists imports, so
the assignment would have run after keys.mjs was already evaluated anyway. The isolation was
never real; it just LOOKED real, which is why it survived.

Fix:
  - keys.mjs resolves the dir lazily, inside getDb(), honoring OCP_DIR_OVERRIDE. Deliberately
    NOT a generic `OCP_DIR`: pointing a RUNNING server at a different key store silently
    changes which credentials authenticate, so this must be awkward to set by accident.
  - new test-env.mjs, imported BEFORE keys.mjs, mints a per-run scratch dir. A separate module
    is required — ESM hoisting means a statement in the test's own body is too late.
  - export getDbPath() so the store's location can be asserted.
  - as a side effect, importing keys.mjs no longer creates directories in the operator's home.

Two guards added, both MUTATION-TESTED (revert the override → both fail, 317/2):
  - the key store under test is a scratch db, NOT the operator's real ~/.ocp/ocp.db
  - listKeys does not depend on rows left behind by an earlier or concurrent run

Proven, not asserted: the real ~/.ocp/ocp.db held at 749 rows across two full test runs
(it previously grew by 2 each run). The 737 existing junk rows are NOT cleaned up here —
that is a destructive change to the maintainer's live database and is his call, not a
side effect of a test fix.

npm test: 319 passed, 0 failed (was 317).

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

* fix(keys): gate the test override behind NODE_ENV so a prod server can never honor it

Review fold-in. The reviewer landed the sharpest possible critique: the first cut closed a
test-hygiene hole by opening a quieter AUTH-CORRECTNESS one, and its only guard against that
was "the variable has an awkward name" — a naming convention plus a comment. That is precisely
the failure mode this whole PR exists to indict (a comment describing an intention that nothing
enforces). It was demonstrated live:

    OCP_DIR_OVERRIDE=/tmp/evil-store  ->  server opens /tmp/evil-store/ocp.db, 0 keys visible

server.mjs imports keys.mjs, and `ocp start`'s nohup fallback inherits the invoking shell's env
— so a maintainer who exported the var while debugging THIS issue and then started the server
would get a server silently authenticating against an empty key store. In AUTH_MODE=multi that
is a total auth outage: every real key 401s, nothing logged, nothing on /health.

F1 — the gate is now the actual guard: OCP_DIR_OVERRIDE is honored ONLY when
     NODE_ENV === "test". A production server runs without NODE_ENV and therefore CANNOT be
     redirected, however the variable reached its environment. Proven both directions:
       no NODE_ENV      + OCP_DIR_OVERRIDE=/tmp/evil-store -> /Users/<op>/.ocp/ocp.db  (ignored)
       NODE_ENV=test    + OCP_DIR_OVERRIDE=/tmp/scratch    -> /tmp/scratch/ocp.db      (honored)
     Plus: getDb() now LOGS the store whenever it is not the default. Silence was the other half
     of the bug — a server on the wrong key store looks exactly like one on the right store until
     every request 401s.

F2 — restore the 0700 guarantee on ~/.ocp. Removing keys.mjs's top-level mkdirSync (a good
     change on its own) silently dropped it: prepareSpawnHome (server.mjs:477) does
     mkdirSync(recursive) with NO mode, so on a fresh install it can create ~/.ocp as a
     world-listable 0755 parent. Verified: 755 via the spawn-home path vs 700 via resolveOcpDir.
     The invariant used to be inherited by luck; it is now stated.

F3 — test-env.mjs removes its scratch dir on exit. Otherwise the fix traded unbounded growth in
     ~/.ocp/ocp.db for unbounded growth in $TMPDIR. Verified: 2 runs, delta 0 dirs.
F4 — closeDb() clears dbPath; getDbPath() no longer hands back a path to a closed db.
F5 — dropped the dead unlinkSync import and explained the leftover HOME normalization.

New test, and it is the one that matters: "a PRODUCTION process (no NODE_ENV) must IGNORE
OCP_DIR_OVERRIDE" — so nothing can re-widen the gate without a red test.

server.mjs IS touched (one mkdirSync mode). Not endpoint-touching: no request handler, endpoint,
header, or wire field — so no cli.js citation applies (ALIGNMENT.md Rule 2 / CLAUDE.md hard-req #1).
Note memory/constitution.md § II lists keys.mjs as a protected file requiring maintainer approval.

npm test: 320 passed, 0 failed. Real ~/.ocp/ocp.db unchanged at 751 rows throughout.

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

* fix(test): make the F1 gate test REAL — it was theatre, and proved it

The reviewer deleted the entire NODE_ENV gate from keys.mjs and the suite still reported
320 passed, 0 failed. The one test written to stop this bug recurring was the one thing in
the PR that would have let it recur — and it would have merged green, with a false sense of
coverage.

Why it was worthless: it re-implemented the predicate INSIDE THE TEST BODY —

    const resolve = (nodeEnv, override) =>
      (nodeEnv === "test" ? override : null) || join(homedir(), ".ocp");

— and never called resolveOcpDir(), getDb(), or getDbPath(). It asserted that a closure
defined three lines above behaved as written. A copy of the predicate is not the predicate.
Its own comment said "exercising the same predicate keys.mjs uses" — that phrase was the tell.

This is the same failure class the PR exists to indict (an assertion of an intention that
nothing enforces), reproduced one layer up, in the fix for it. Fourth time in this repo that
a correctly-named test has vouched for nothing.

The real test must run OUT OF PROCESS: the parent is irreversibly NODE_ENV=test by the time
any test runs (test-env.mjs sets it before keys.mjs is imported), so the production path is
simply unreachable in-process. It now spawns 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 asserts what the
REAL keys.mjs actually did.

MUTATION-PROVEN, against the exact revert that used to pass:
  delete the whole NODE_ENV gate -> 319 passed, 1 failed
    ✗ a PRODUCTION process (no NODE_ENV) must IGNORE OCP_DIR_OVERRIDE
  restore                        -> 320 passed, 0 failed

Also folded in: setup.mjs created ~/.ocp at the umask default (755, world-listable) on a fresh
install via the logs dir — pre-existing, self-healing on first server start, now stated
explicitly (mode 0700) rather than left to luck. Same class as the F2 fix.

npm test: 320 passed, 0 failed. Real ~/.ocp/ocp.db unchanged at 751 rows.

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

* 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

---------

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>

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 });
+// 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(OCP_DIR, 0o700); } catch { /* ignore EPERM on pre-existing dirs */ }
-const DB_PATH = join(OCP_DIR, "ocp.db");
+  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`);