From b11fa083ab093dc6ae677ddd8ed7d18d9f4c6c59 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 00:46:50 -0700 Subject: [PATCH 1/5] test(harness): unset the DB env keys instead of assigning undefined MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `process.env.PG_HOST = undefined` stores the STRING 'undefined', which is truthy. In unit suites that satisfied every `if (process.env.PG_HOST)` guard in the backend (server.ts:68 and :363, controllers/podController.ts:22/:426/:627, controllers/authController.ts:121, routes/agentsRuntime.ts:3103) and made db-pg.ts:75 build a real Pool for host 'undefined', so connectPG()'s not-configured branch (:106-108) was unreachable. Same for MONGO_URI. Delete the keys instead. MONGO_URI introduces no new failure mode: server.ts:297 calls connectDB() unconditionally and config/db.ts:11-13 answers a connect failure with console.error + process.exit(1) — which is already what the string URL 'undefined' produced. The mongoose mock keeps that inert in tests. Witness (both assertions red before the change, green after): __tests__/unit/config/setupEnvAbsence.test.js asserts neither var is the literal string, and that no Pool is built for host 'undefined'. The second is read off the pool's own options rather than `pool === null`, so a developer with a real PG_HOST in the environment still passes; it also confirms the row's claim at the consequence level rather than only at the env-var level. Dependents, searched rather than assumed: every test file that reads PG_HOST also sets it, and testUtils.js builds its Pool only under useRealServices(). The remaining unconditional readers are the backend guards above, which now take the not-configured path — the shape a unit suite is supposed to see. --- backend/__tests__/setup.js | 13 ++++++++--- .../unit/config/setupEnvAbsence.test.js | 23 +++++++++++++++++++ 2 files changed, 33 insertions(+), 3 deletions(-) create mode 100644 backend/__tests__/unit/config/setupEnvAbsence.test.js diff --git a/backend/__tests__/setup.js b/backend/__tests__/setup.js index 6ad064fb55..ebe79e283a 100644 --- a/backend/__tests__/setup.js +++ b/backend/__tests__/setup.js @@ -31,7 +31,14 @@ if (process.env.INTEGRATION_TEST) { process.env.PG_PASSWORD = process.env.PG_PASSWORD || 'postgres'; process.env.PG_SSL_ENABLED = 'false'; } else { - // In-memory mode — no real DB connections (default for unit tests) - process.env.PG_HOST = undefined; - process.env.MONGO_URI = undefined; + // In-memory mode — no real DB connections (default for unit tests). + // DELETE the keys; do not assign `undefined`. Assignment stores the STRING + // 'undefined', which is truthy, so every `if (process.env.PG_HOST)` guard in + // the backend (server.ts:68/:363, controllers/podController.ts:22/:426/:627, + // controllers/authController.ts:121, routes/agentsRuntime.ts:3103) takes the + // configured branch, and db-pg.ts:75 builds a real Pool for host 'undefined' + // instead of taking its not-configured path. Measured: PG_HOST read back as + // the string "undefined" at module scope in a file that never touched it. + delete process.env.PG_HOST; + delete process.env.MONGO_URI; } diff --git a/backend/__tests__/unit/config/setupEnvAbsence.test.js b/backend/__tests__/unit/config/setupEnvAbsence.test.js new file mode 100644 index 0000000000..d5c28b0aee --- /dev/null +++ b/backend/__tests__/unit/config/setupEnvAbsence.test.js @@ -0,0 +1,23 @@ +/* eslint-disable import/no-unresolved, import/extensions */ +// setup.js runs before every test file (jest `setupFiles`). process.env is per +// test FILE (measured), so what this file observes is setup.js's own effect and +// nothing another suite did. +const { pool } = require('../../../config/db-pg'); + +describe('unit-suite DB env (setup.js in-memory mode)', () => { + it('does not store the string "undefined" in PG_HOST / MONGO_URI', () => { + // The defect signature: `process.env.PG_HOST = undefined` stores the STRING + // 'undefined', which is truthy and satisfies every `if (process.env.PG_HOST)` + // guard in the backend. + expect(process.env.PG_HOST).not.toBe('undefined'); + expect(process.env.MONGO_URI).not.toBe('undefined'); + }); + + it('builds no Pool for the host string "undefined"', () => { + // The consequence (db-pg.ts:75): a truthy host string constructs a real Pool. + // Asserted on the pool's own config rather than on `pool === null`, so a + // developer with a real PG_HOST in the environment still passes. + const host = pool && pool.options && pool.options.host; + expect(host).not.toBe('undefined'); + }); +}); From b15788203f26ad3c9e7f782b2f64b00ab62a2ea3 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 00:46:50 -0700 Subject: [PATCH 2/5] test(harness): stub the pool in the init-pg-db suite Found by the full-suite run the harness change required, and it was relying on the defect: `__tests__/unit/config/init-pg-db.test.js:17` reaches for a REAL `pool` object, which existed only because PG_HOST was the truthy string 'undefined'. Once the harness stopped lying about the host, `pool` was null and both tests died on `Cannot set properties of null (setting 'connect')`. The unit under test is `initializeDatabase`, and its pool is a dependency: stub `config/db-pg` and keep asserting the schema execution and the missing-file path. --- backend/__tests__/unit/config/init-pg-db.test.js | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/backend/__tests__/unit/config/init-pg-db.test.js b/backend/__tests__/unit/config/init-pg-db.test.js index f1238f3be2..22f7fd7cb4 100644 --- a/backend/__tests__/unit/config/init-pg-db.test.js +++ b/backend/__tests__/unit/config/init-pg-db.test.js @@ -1,4 +1,11 @@ +/* eslint-disable import/no-unresolved, import/extensions */ jest.mock('fs'); +// The unit under test is initializeDatabase; its pool dependency is stubbed. This +// suite previously reached for a REAL Pool object, which existed only because +// setup.js left PG_HOST as the truthy string 'undefined' and db-pg.ts:75 built a +// Pool for that host — so it broke the moment the harness stopped lying about the +// host, not because initializeDatabase changed (TASK-128). +jest.mock('../../../config/db-pg', () => ({ pool: { connect: jest.fn() } })); const fs = require('fs'); const { pool } = require('../../../config/db-pg'); From 440b0b06a294da7966f6069a1cf2fa6e65163a62 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:08:41 -0700 Subject: [PATCH 3/5] docs(testing): the per-file process.env, and the reason behind rule 31 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit TESTING.md: `process.env` is per test FILE, not per test worker. `jest-environment-node` gives each file its own detached copy (`jest-util`'s `createProcessObject`), measured with two files in one `--runInBand` worker on a single pid, neither observing the other's writes. Corollary for child processes: pass env explicitly, because `child_process`'s default env reads the real environment, not the test file's copy. `docs/development/review-checklist.md` rule 31 called the TMPDIR mechanism unexplained, which was honest; the mechanism is that same per-file copy, so the rule keeps its conclusion and loses only that rider — the reason is rewritten rather than the rule deleted (rule 15's second rider). --- backend/TESTING.md | 4 ++- backend/jest.setup.js | 42 ---------------------------- docs/development/review-checklist.md | 2 +- 3 files changed, 4 insertions(+), 44 deletions(-) delete mode 100644 backend/jest.setup.js diff --git a/backend/TESTING.md b/backend/TESTING.md index 7db16901ae..f32b8e3f7f 100644 --- a/backend/TESTING.md +++ b/backend/TESTING.md @@ -74,7 +74,9 @@ Tier-1 setup logs `[tier1] Connected to real MongoDB …` and `[tier1] Connected | `generateTestToken(userId)` | signs with `process.env.JWT_SECRET` | same | | `createTestUser / Pod / Message` | Mongoose model instantiation | same | -The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests__/setup.js` reads this at suite start and populates `MONGO_URI` / `PG_*` defaults when set; when unset, it nulls them so accidental real-DB connections fail loudly. +The branch is controlled by `process.env.INTEGRATION_TEST === 'true'`. `__tests__/setup.js` reads this at suite start and populates `MONGO_URI` / `PG_*` defaults when set; when unset, it **deletes** them (`delete process.env.PG_HOST`) so accidental real-DB connections fail loudly. Assigning `undefined` does not do that: Node stores the **string** `'undefined'`, which is truthy, and every `if (process.env.PG_HOST)` guard in the backend then takes the configured branch — including `config/db-pg.ts:75`, which builds a `Pool` for a host named `undefined` instead of leaving `pool` null (#1873). + +**`process.env` is per test FILE, not per test worker.** `jest-environment-node` gives each file its own detached copy (`jest-util`'s `createProcessObject`), so two files in one `--runInBand` worker — same pid — never observe each other's writes. Two consequences: no cross-file cleanup is owed for a `process.env` write inside a file, and a variable set in one file cannot be inherited by the next. The copy is also detached from the *real* environment, which is why writing `process.env.TMPDIR` does not move `os.tmpdir()` under Jest (see `docs/development/review-checklist.md` rule 31) — **a child process needs its env passed explicitly**; `child_process`'s default env reads the real environment, not the test file's copy. ## Authoring rules diff --git a/backend/jest.setup.js b/backend/jest.setup.js deleted file mode 100644 index 9521585102..0000000000 --- a/backend/jest.setup.js +++ /dev/null @@ -1,42 +0,0 @@ -// Set up environment variables for testing -process.env.NODE_ENV = 'test'; - -// Use the test JWT secret from environment if available, otherwise use a default -process.env.JWT_SECRET = process.env.TEST_JWT_SECRET || 'test-jwt-secret'; - -// Use the test MongoDB URI from environment if available, otherwise use a default local one -process.env.MONGO_URI = process.env.TEST_MONGO_URI || 'mongodb://localhost:27017/test'; - -// Port for testing -process.env.PORT = process.env.TEST_PORT || '5001'; - -// Increase timeout for tests -jest.setTimeout(30000); - -// Suppress console output during tests to keep test output clean -// You can comment these out if you need to debug tests -global.console = { - ...console, - log: jest.fn(), - debug: jest.fn(), - info: jest.fn(), - warn: jest.fn(), - error: jest.fn(), -}; - -// Additional environment variables -process.env.PG_USER = 'test'; -process.env.PG_PASSWORD = 'test'; -process.env.PG_HOST = 'localhost'; -process.env.PG_PORT = '5432'; -process.env.PG_DATABASE = 'test'; -process.env.SENDGRID_API_KEY = 'fake-api-key'; -process.env.SENDGRID_FROM_EMAIL = 'test@example.com'; -process.env.FRONTEND_URL = 'http://localhost:3000'; - -// Silence console logs during tests -console.log = jest.fn(); -console.info = jest.fn(); -console.warn = jest.fn(); -// Keep error logs for debugging -// console.error = jest.fn(); diff --git a/docs/development/review-checklist.md b/docs/development/review-checklist.md index d467facf29..5c52e850ce 100644 --- a/docs/development/review-checklist.md +++ b/docs/development/review-checklist.md @@ -98,7 +98,7 @@ And then note where the fix landed. Someone hit the audience gap, felt it, and b 30. **A question about what a rendered value MEANS is answered from the store, not from the renderer.** A renderer's fallback chain tells you which inputs *could* have produced a string; it cannot tell you which one did, and reading the chain to reason about the cause settles nothing. Enumerate the chain from source — each `||` is a hypothesis with a different mechanism — then measure the inputs in the store. Prefer a decisive experiment to more reading where one exists: a single write plus a single re-capture falsified a membership hypothesis in about a minute (and the probe was reverted, so the committed evidence still matches its seed). When the answer is "the artifact is a harness artifact", say so **with the artifact**: a screenshot cannot distinguish a fixture row from live data, so any evidence built from seeded records must name its seed and the fixture-shaped fields in the same breath as the image, or a reader will read the fixture as usage. Rider: a hand-seeded row is not equivalent to a posted one — the real write path also creates the identity row and the membership, so a hand-inserted record reproduces neither. *(Earned: 2026-09-19, the #1758 evidence — the seeded card rendered as `Unknown User` and the file-worthy hypothesis was pod membership; the measurement refuted it (added the member, the page read `2 members`, the label unchanged). The cause was the PG mirror: `backend/models/pg/Message.ts:214,261` LEFT JOINs `users` for `u.username`, and the hand-inserted card's author has no row there because it never went through `agentIdentityService.ts:618` (`INSERT INTO users`) inside `getOrCreateAgentUser`, which `agentMessageService.ts:1245–1249` calls together with `ensureAgentInPod` before posting. Nothing filed — and four seeded `tool_calls` rows read as live usage by a human until their provenance was stated.)* -31. **A flake fix is proven by making the defect reproducible, not by rerunning the flake — and an assertion that counts a process-global resource must watch the subject's own identity.** "It passed ten times" is not evidence about a flaky assertion: the flake's rate bounds its cost, never whether it is fixed. The reproducible form is to mutate the guarded behaviour into existence and watch the same assertion go red, which also proves the assertion measures the thing it names. The follow-up question is what the assertion counts: a count over `os.tmpdir()` contents, a socket table or a lock directory is process- or machine-global state a sibling test or another actor can perturb, so the assertion must watch the *subject's own* identity — this run's path, this seat's row — not the pool it lives in. One negative result worth carrying, because it looks like an escape hatch and is not: setting `process.env.TMPDIR` does **not** redirect `os.tmpdir()` inside jest, though a bare `node` v20 does follow it — mechanism unexplained, do not build isolation on it. *(Earned: TASK-073/#1769 — the codex adapter creates its temp dir at `codex.js:442` and removes it in a `finally` at `:506`; the test now watches the directory that call created, so dropping that `rm` reddens exactly that test, with an in-seam `existsSync` control so it cannot pass against an adapter that never creates the directory. Control for the flake itself: branch 3/15, main 3/15, alone 0/10 — the rate was pre-existing; the instrument was the fix.)* +31. **A flake fix is proven by making the defect reproducible, not by rerunning the flake — and an assertion that counts a process-global resource must watch the subject's own identity.** "It passed ten times" is not evidence about a flaky assertion: the flake's rate bounds its cost, never whether it is fixed. The reproducible form is to mutate the guarded behaviour into existence and watch the same assertion go red, which also proves the assertion measures the thing it names. The follow-up question is what the assertion counts: a count over `os.tmpdir()` contents, a socket table or a lock directory is process- or machine-global state a sibling test or another actor can perturb, so the assertion must watch the *subject's own* identity — this run's path, this seat's row — not the pool it lives in. One negative result worth carrying, because it looks like an escape hatch and is not: setting `process.env.TMPDIR` does **not** redirect `os.tmpdir()` inside jest, though a bare `node` v20 does follow it — the mechanism is the per-file `process.env`: a test file's env is `jest-util`'s detached `createProcessObject` copy, so its writes never reach the real environment, while `os.tmpdir()` (POSIX native `getTempDir()`) and `child_process`'s default env both read the real one, so an `execSync` child does not see a variable the test file set. Do not build isolation on it — and pass env explicitly to any child that needs it. *(Reason replaced 2026-09-25; the conclusion is unchanged and only the "unexplained" rider is withdrawn. Measured on TASK-128 by @wren and reproduced on this box: Jest 29.7 gives each test file its own `process.env` — two files in one `--runInBand` worker, same pid, neither saw the other's writes — on Node 22 and on Node 20.18.1. See `backend/TESTING.md`.)* *(Earned: TASK-073/#1769 — the codex adapter creates its temp dir at `codex.js:442` and removes it in a `finally` at `:506`; the test now watches the directory that call created, so dropping that `rm` reddens exactly that test, with an in-seam `existsSync` control so it cannot pass against an adapter that never creates the directory. Control for the flake itself: branch 3/15, main 3/15, alone 0/10 — the rate was pre-existing; the instrument was the fix.)* ## Merge mechanics From 67127300e1136da80b1a5f0d28a77f9f2c725142 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:08:41 -0700 Subject: [PATCH 4/5] refactor(test): drop what the harness fix makes dead MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - `agentIdentityService.syncUserToPostgreSQL` no longer needs its `pgHost === 'undefined'` clause (#1546). It compensated for the harness storing the literal string 'undefined'; `!process.env.PG_HOST` covers the unconfigured case on its own, because `config/db-pg.ts:75` leaves `pool` null for it. - `backend/jest.setup.js` is orphaned. Verified rather than assumed: `jest.config.js:35` lists `__tests__/setup.js` as the only `setupFilesAfterEnv` entry, `package.json` has no `jest` key, no script passes `--config`, and a repo-wide grep finds the file named only in `tsconfig.build.json`'s exclude — so the file and that exclude line both go. `tsconfig.json:23` stays: it records a landing-time count, which is history rather than configuration. --- backend/services/agentIdentityService.ts | 12 ++++++------ backend/tsconfig.build.json | 1 - 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/backend/services/agentIdentityService.ts b/backend/services/agentIdentityService.ts index 6c957b1d7a..7456809d8a 100644 --- a/backend/services/agentIdentityService.ts +++ b/backend/services/agentIdentityService.ts @@ -580,12 +580,12 @@ class AgentIdentityService { } static async syncUserToPostgreSQL(user: InstanceType): Promise { - const pgHost = process.env.PG_HOST; - // Some test harnesses clear process.env with `undefined`, which Node - // serializes as the literal string "undefined". Treat that the same as - // an unconfigured PostgreSQL mirror rather than reporting a false sync - // failure to an otherwise successful profile save. - if (!PGMessage || !pgHost || pgHost === 'undefined' || !dbPg) return true; + // `!PG_HOST` is the unconfigured-mirror case: config/db-pg.ts:75 builds a + // Pool only when PG_HOST is set, so there is nothing to sync into. A test + // harness that stored the literal string "undefined" made that guard truthy + // anyway, so this used to name the string explicitly; __tests__/setup.js + // deletes the key instead and the compensation is gone. + if (!PGMessage || !process.env.PG_HOST || !dbPg) return true; try { const { pool } = dbPg; const checkQuery = 'SELECT _id FROM users WHERE _id = $1'; diff --git a/backend/tsconfig.build.json b/backend/tsconfig.build.json index 93f6fdefbf..2f95acc541 100644 --- a/backend/tsconfig.build.json +++ b/backend/tsconfig.build.json @@ -11,7 +11,6 @@ "**/*.test.js", "**/*.spec.ts", "jest.config.js", - "jest.setup.js", "cleanup-test-data.ts", "test-discord-commands.ts", "test-discord-integration.ts", From 93c19d59849d3f697d14704ab843c65d13d5d24f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Fri, 25 Sep 2026 01:09:52 -0700 Subject: [PATCH 5/5] docs(test): correct the locators in the setup.js comment (vera 73709/73720) Five of the seven sites named were TASK-126-branch offsets carried onto a comment that merges to main and outlives the branch. Re-derived against `9a32fca5` before writing them down: server.ts:68/:378, podController.ts:22/:462/:663, authController.ts:171, agentsRuntime.ts:3188. The list also read as the population when it was an enumeration, so it now states the count it is a sample of: 25 `process.env.PG_HOST` reads across 11 runtime modules (26/12 with the backend-root `testPgConnection.ts`), measured by grep excluding `__tests__/` and `backend/scripts/`. --- backend/__tests__/setup.js | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/backend/__tests__/setup.js b/backend/__tests__/setup.js index ebe79e283a..ea000476bf 100644 --- a/backend/__tests__/setup.js +++ b/backend/__tests__/setup.js @@ -34,11 +34,12 @@ if (process.env.INTEGRATION_TEST) { // In-memory mode — no real DB connections (default for unit tests). // DELETE the keys; do not assign `undefined`. Assignment stores the STRING // 'undefined', which is truthy, so every `if (process.env.PG_HOST)` guard in - // the backend (server.ts:68/:363, controllers/podController.ts:22/:426/:627, - // controllers/authController.ts:121, routes/agentsRuntime.ts:3103) takes the - // configured branch, and db-pg.ts:75 builds a real Pool for host 'undefined' - // instead of taking its not-configured path. Measured: PG_HOST read back as - // the string "undefined" at module scope in a file that never touched it. + // the backend takes the configured branch — 25 reads across 11 runtime + // modules, including server.ts:68/:378, podController.ts:22/:462/:663, + // authController.ts:171 and agentsRuntime.ts:3188 — and db-pg.ts:75 builds a + // real Pool for host 'undefined' instead of taking connectPG's not-configured + // path at :106-108. Measured: PG_HOST read back as the string "undefined" at + // module scope in a file that never touched it. delete process.env.PG_HOST; delete process.env.MONGO_URI; }