test(server): #434 isolate Bun.serve integration suites in aggregate runs

docs/tests/aggregate-runner.md

@@ -0,0 +1,119 @@
+# server aggregate test runner (issue #434)
+
+The canonical way to run the CommHub server test suite:
+
+```
+cd server
+bun run test
+```
+
+which delegates to `scripts/test-runner.ts`. Every real-server integration
+suite gets its own isolated Bun child with an independent OS-assigned
+port, a fresh `mkdtemp` DB, and an env allowlist that excludes
+`DATABASE_URL`. Serial by design. Exit code is authoritative; counters are
+evidence.
+
+## Why the runner
+
+Two suites (`api-host-supervisors-fallback.test.ts`,
+`uploads-http.test.ts`) import `./index.js` and call `bootServer()`.
+On the pre-#434 code path, they used a shared `bun test src/` process
+and a parent-side random `PORT = 18000 + Math.random() * 1000`. That
+combination hit three separate problems in the aggregate lane:
+
+1. Only one `Bun.serve` binding wins the port. The loser's tests reach
+   assertions but the fetch calls hit `ECONNREFUSED`.
+2. `db.ts`'s module singleton is set at first import from whatever env
+   was live at that moment — subsequent `beforeAll` env mutations don't
+   move the DB path, so combined fixtures write into the winning suite's
+   database.
+3. The parent-side random port range is a TOCTOU pattern that races
+   with anything else that happens to bind in that range.
+
+The runner replaces the shared process with per-suite children,
+requests `port: 0` on the seam (kernel picks a free port at bind time),
+gives each child its own DB dir, and drives its own manifest instead of
+relying on shell globs.
+
+## Two suite kinds
+
+- `isolated_server` — integration suites that import the server module
+  and call `bootServer(...)`. One Bun child per file.
+- `shared_unit` — pure logic / DB-only fixture code. Runs in a single
+  shared child.
+
+Every `*.test.ts` under `server/src/` (and `server/scripts/`) must be
+registered explicitly in `scripts/test-manifest.ts`. The runner performs
+a set-equality check between manifest and the filesystem at start; a
+missing or extra entry hard-fails the whole run before any test executes.
+
+If a `shared_unit` file is ever found to pollute the shared child (leaked
+DB state, module-singleton mutation), promote it to `isolated_server`
+rather than muting an assertion — the aggregate-fail is the load-bearing
+signal here.
+
+## What the runner does per child
+
+- Env: explicit allowlist of parent keys (`PATH`, `TMPDIR`, `TERM`,
+  `LANG`, `LC_ALL`, `LC_CTYPE`, `CI`, `GITHUB_ACTIONS`, `SHELL`, `USER`,
+  `LOGNAME`). Any other parent variable is dropped, including
+  **`DATABASE_URL` which is unset unconditionally**. This is a
+  defense-in-depth against a leaked shell / CI variable, on top of the
+  `#435` guard inside `db-adapter.ts`. The `scripts/test-runner-self.test.ts`
+  suite proves the runner's env-shaping keeps this invariant.
+- `HOME`, `COMMHUB_DB`, `COMMHUB_UPLOADS_DIR`, `TMPDIR`: each child gets
+  a fresh `mkdtempSync` directory. On child exit the runner rms the
+  whole dir (sweeps `db`, `db-wal`, `db-shm` in one shot).
+- `NODE_ENV`: forced to `"test"` for every child so the `#435` guards
+  are always in scope.
+- `stdin`: ignored. `stdout` / `stderr`: continuously drained;
+  `--verbose` streams live, otherwise the runner prints a tail on
+  non-zero exit.
+- Exit code: authoritative gate. The parsed pass/fail/expect counts are
+  summary-only — a mismatch between counts and exit code always resolves
+  against the exit code (i.e., silent green-washing via count-parse tricks
+  is impossible).
+- Signals: on `SIGINT` / `SIGTERM` the runner forwards to every live
+  child, then cleans up temp dirs before propagating exit.
+
+## `test:raw` is diagnostic-only
+
+The former `test` script (`bun test src/`) is preserved as `test:raw`.
+It prints a stderr warning that it will reproduce the singleton /
+port / DB conflicts under `#434` and is not a CI gate. Use it when you
+need to see the interleaved log lines of the two integration suites at
+the same time; use `bun run test` for anything else.
+
+## CLI shape
+
+```
+bun run scripts/test-runner.ts [--suite=<manifest-path>] [--verbose]
+```
+
+- `--suite=<path>` — filter to a single manifest entry (exact match).
+  Pattern-based filtering is not offered; the manifest is the source
+  of truth for what runs.
+- `--verbose` / `-v` — stream child stdout/stderr live instead of
+  buffering.
+
+Concurrency is fixed at 1 in this PR — the runner runs isolated suites
+serially, then the shared_unit child. Opening `--concurrency=N` is
+deferred until we have live evidence it stays deterministic.
+
+## Reproducibility check
+
+```
+for i in 1 2 3; do bun run test | tail -6; done
+```
+
+Expected: three identical `total pass / total fail / total expects`
+lines. Per-suite wall-clock times may vary slightly; counts and exit
+codes must not.
+
+## Related
+
+- Issue: #434 (this)
+- Related: #435 (`DATABASE_URL` reject guard) — the runner is the
+  operational half of #435's "ordinary test commands explicitly unset
+  DATABASE_URL" requirement.
+- RFC tracking: #428.

server/package.json

@@ -14,7 +14,8 @@
   "scripts": {
     "start": "bun run src/index.ts",
     "dev": "bun --watch run src/index.ts",
-    "test": "COMMHUB_DB=/tmp/test-commhub-$$.db bun test src/"
+    "test": "bun run scripts/test-runner.ts",
+    "test:raw": "echo '⚠️  test:raw is diagnostic-only — it runs bun test src/ in a SINGLE process, which reproduces the module-singleton / port / DB conflicts documented in #434. NOT a CI gate. For canonical results run: bun run test' 1>&2 && COMMHUB_DB=/tmp/test-commhub-raw-$$.db bun test src/"
   },
   "keywords": [
     "commhub",

server/scripts/test-manifest.ts

@@ -0,0 +1,74 @@
+// #434 — exhaustive test manifest.
+//
+// The aggregate runner (scripts/test-runner.ts) uses this manifest to
+// decide, per test file, whether the file needs its own isolated Bun
+// child process (an integration suite that binds `Bun.serve` and/or
+// mutates module-singleton state via `./db` and `./index`) or can share
+// a single child with the rest of the pure-unit files.
+//
+// Registration is required for EVERY `*.test.ts` under `server/src/`.
+// The runner performs a set-equality check between the union of both
+// arrays here and the filesystem enumeration at start; a missing or
+// extra entry fails the whole run before any test executes. That way
+// a future PR that introduces a new integration suite cannot silently
+// regress the isolation contract by defaulting to the shared child.
+//
+// Two kinds:
+//
+//   isolated_server — imports `./index.js` and calls `bootServer(...)`
+//     (or otherwise starts `Bun.serve`), or holds any module-level
+//     singleton that would race under a shared runtime (e.g. an
+//     integration DB). Each such suite gets its own Bun.spawn child.
+//
+//   shared_unit — pure logic / DB-only fixture code that is safe to
+//     load in a single shared child. If we later find one of these is
+//     silently polluting shared state, promote it to isolated_server
+//     rather than adding runtime workarounds that would hide the
+//     leak (per #434 acceptance line "don't hide with assertion
+//     changes").
+//
+// Paths are relative to `server/` and end in `.test.ts`.
+
+export const ISOLATED_SERVER_SUITES: readonly string[] = [
+  "src/api-host-supervisors-fallback.test.ts",
+  "src/uploads-http.test.ts",
+] as const;
+
+export const SHARED_UNIT_SUITES: readonly string[] = [
+  "src/ack-create-request.test.ts",
+  "src/api-nodes-shape.test.ts",
+  "src/auth-kdf-migration.test.ts",
+  "src/auth-tokens.test.ts",
+  "src/auth-validate.test.ts",
+  "src/auth_login_guard.test.ts",
+  "src/config-apply-sec1.test.ts",
+  "src/config-apply-validate.test.ts",
+  "src/create-node-validate.test.ts",
+  "src/create-node.test.ts",
+  "src/cross-tenant-injection.test.ts",
+  "src/list-host-supervisors.test.ts",
+  "src/password-dict.test.ts",
+  "src/probe-validate.test.ts",
+  "src/probe.test.ts",
+  "src/push.test.ts",
+  "src/response-charset.test.ts",
+  "src/retention.test.ts",
+  "src/send_dedup.test.ts",
+  "src/shared/probe-host-allowlist-drift.test.ts",
+  "src/shared/reserved-env-drift.test.ts",
+  "src/shared/reserved-env.test.ts",
+  "src/stale-sweeper.test.ts",
+  "src/stop-delete-node.test.ts",
+  "src/update-provider.test.ts",
+  "src/uploads.test.ts",
+  "src/vault.test.ts",
+  // Runner's own hermetic self-test — proves inheritance-of-fake-prod
+  // DATABASE_URL becomes UNSET in the spawned child (issue #434 rule 9,
+  // #435 double-safety). Doesn't need isolation; validates spawn env.
+  "scripts/test-runner-self.test.ts",
+] as const;
+
+/** All manifest-registered suites, as a `Set` for fast lookup. */
+export function manifestSet(): Set<string> {
+  return new Set<string>([...ISOLATED_SERVER_SUITES, ...SHARED_UNIT_SUITES]);
+}

server/scripts/test-runner-self.test.ts

@@ -0,0 +1,93 @@
+// #434 rule 9 / #435 defense-in-depth — the aggregate runner's child
+// env MUST NOT contain a leaked DATABASE_URL. This suite spawns a
+// second-level Bun child under the SAME allowlist rules the runner
+// uses, deliberately inheriting a fake-prod DATABASE_URL from the
+// current process, and asserts the spawned child sees `DATABASE_URL`
+// as `undefined` (not "the parent's value", not "the guard's error
+// message" — properly unset).
+//
+// If this ever regresses, an operator with a leaked shell DATABASE_URL
+// could be silently connecting a test process to production. The
+// runner-level defense makes the #435 guard's job the *second* line;
+// this test proves the first line holds.
+
+import { describe, expect, test } from "bun:test";
+import { spawnSync } from "child_process";
+import { join, resolve } from "path";
+
+// Duplicated deliberately — importing from the runner would create a
+// module coupling that could hide a regression if the allowlist ever
+// grew. This is the canonical set the child MUST see.
+const RUNNER_ALLOWED_KEYS = new Set([
+  "PATH", "TMPDIR", "TERM", "LANG", "LC_ALL", "LC_CTYPE",
+  "CI", "GITHUB_ACTIONS", "SHELL", "USER", "LOGNAME",
+]);
+
+function buildRunnerLikeChildEnv(parentEnv: NodeJS.ProcessEnv): Record<string, string> {
+  const env: Record<string, string> = {};
+  for (const k of RUNNER_ALLOWED_KEYS) {
+    const v = parentEnv[k];
+    if (v !== undefined) env[k] = v;
+  }
+  env.NODE_ENV = "test";
+  delete env.DATABASE_URL;
+  return env;
+}
+
+describe("#434 runner allowlist — DATABASE_URL cannot leak into children", () => {
+  test("parent DATABASE_URL='postgres://fake…' → child sees DATABASE_URL=undefined", () => {
+    const FAKE_PROD = "postgres://fake-prod-user:pw@prod.example:5432/commhub";
+    const parentEnv: NodeJS.ProcessEnv = {
+      ...process.env,
+      DATABASE_URL: FAKE_PROD,
+    };
+    const childEnv = buildRunnerLikeChildEnv(parentEnv);
+
+    // Sanity — the allowlist did drop DATABASE_URL from our own object.
+    expect(childEnv.DATABASE_URL).toBeUndefined();
+
+    // The actual proof: spawn a Bun child using THAT env, ask it to
+    // print `process.env.DATABASE_URL || "UNSET"`, assert it says UNSET.
+    // Not testing #435 here — we intentionally omit COMMHUB_DB so the
+    // child would trip the SQLite guard if it tried to open a DB, but
+    // we never call createAdapter; we only probe env visibility.
+    const child = spawnSync("bun", [
+      "-e",
+      "console.log(process.env.DATABASE_URL || 'UNSET')",
+    ], {
+      env: childEnv,
+      encoding: "utf8",
+      timeout: 10_000,
+    });
+
+    expect(child.status).toBe(0);
+    expect(child.stdout.trim()).toBe("UNSET");
+  });
+
+  test("parent has no DATABASE_URL → child still sees it as UNSET", () => {
+    // Baseline: without any inheritance, the child obviously sees UNSET.
+    // Included so the "leaked→UNSET" and "clean→UNSET" cases both live
+    // in the file and one can't accidentally start diverging.
+    const clean: NodeJS.ProcessEnv = { ...process.env };
+    delete clean.DATABASE_URL;
+    const childEnv = buildRunnerLikeChildEnv(clean);
+    expect(childEnv.DATABASE_URL).toBeUndefined();
+
+    const child = spawnSync("bun", [
+      "-e",
+      "console.log(process.env.DATABASE_URL || 'UNSET')",
+    ], {
+      env: childEnv,
+      encoding: "utf8",
+      timeout: 10_000,
+    });
+
+    expect(child.status).toBe(0);
+    expect(child.stdout.trim()).toBe("UNSET");
+  });
+
+  test("smoke: NODE_ENV is set to 'test' for every child", () => {
+    const childEnv = buildRunnerLikeChildEnv(process.env);
+    expect(childEnv.NODE_ENV).toBe("test");
+  });
+});