From 3e03473675cdab227dd5a4df29a72ca42aa63537 Mon Sep 17 00:00:00 2001 From: dtzp555 Date: Wed, 15 Jul 2026 08:27:42 +1000 Subject: [PATCH] =?UTF-8?q?fix(test):=20make=20the=20F1=20gate=20test=20RE?= =?UTF-8?q?AL=20=E2=80=94=20it=20was=20theatre,=20and=20proved=20it?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR --- setup.mjs | 2 +- test-features.mjs | 38 +++++++++++++++++++++++++------------- 2 files changed, 26 insertions(+), 14 deletions(-) diff --git a/setup.mjs b/setup.mjs index 89fec40..4019444 100755 --- a/setup.mjs +++ b/setup.mjs @@ -390,7 +390,7 @@ 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 }); + if (!existsSync(ocpLogsDir)) mkdirSync(ocpLogsDir, { recursive: true, mode: 0o700 }) // 0700: this call can create ~/.ocp itself on a fresh install; // Uninstall legacy service names if present (upgrade path) if (platform === "darwin") { diff --git a/test-features.mjs b/test-features.mjs index cb06009..bee5db8 100644 --- a/test-features.mjs +++ b/test-features.mjs @@ -11,6 +11,8 @@ import { createSerialMutex, createTtlCache, isTokenExpiring, orderLabelsLastGood import { createHash } from "node:crypto"; import { strict as assert } from "node:assert"; import { join } from "node:path"; +import { pathToFileURL } from "node:url"; +import { execFileSync } from "node:child_process"; import { homedir } from "node:os"; process.env.HOME = homedir(); // normalize HOME so homedir()-derived paths are stable across shells @@ -3826,19 +3828,29 @@ test("the key store under test is a scratch db, NOT the operator's real ~/.ocp/o }); test("a PRODUCTION process (no NODE_ENV) must IGNORE OCP_DIR_OVERRIDE", () => { - // The gate is the actual guard, so it needs its own test. An earlier cut of this fix relied on - // the env var merely having an awkward name — a naming convention plus a comment — which is the - // same shape of non-guard this whole change exists to indict. If a running server ever honored - // this, it would silently authenticate against a DIFFERENT key store: in AUTH_MODE=multi that - // is a total auth outage (every real key 401s) with nothing logged. Assert the gate, in-process, - // by exercising the same predicate keys.mjs uses. - const resolve = (nodeEnv, override) => - (nodeEnv === "test" ? override : null) || join(homedir(), ".ocp"); - const real = join(homedir(), ".ocp"); - - assert.equal(resolve(undefined, "/tmp/evil-store"), real, "a prod server must ignore the override"); - assert.equal(resolve("production", "/tmp/evil-store"), real, "…including with NODE_ENV=production"); - assert.equal(resolve("test", "/tmp/scratch"), "/tmp/scratch", "…but a test run must still isolate"); + // 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; + const probe = `import { getDb, getDbPath, closeDb } from ${JSON.stringify(keysUrl)}; +getDb(); process.stdout.write(getDbPath()); closeDb();`; + const env = { ...process.env, HOME: home, OCP_DIR_OVERRIDE: evil }; + delete env.NODE_ENV; // a production server has no NODE_ENV + const out = execFileSync(process.execPath, ["--input-type=module", "-e", probe], + { env, encoding: "utf8" }).trim(); + assert.equal(out, join(home, ".ocp", "ocp.db"), "a prod process must open HOME/.ocp/ocp.db"); + assert.ok(!out.startsWith(evil), "a prod process must NEVER honor OCP_DIR_OVERRIDE"); + } 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", () => {