issue-136: Implement src/env.ts with unit tests (MONGODB_URI resolution) #154

Merged

david merged 2 commits from feature/issue-136/implement-src-env-ts-with-unit-tests-mongodb-uri-r into main

2026-08-31 23:46:51 +00:00

david commented

2026-08-31 23:46:50 +00:00

Owner

136: Implement src/env.ts with unit tests (MONGODB_URI resolution)

Implements 136.

This pull request was generated automatically by pi-loop and opened as a
Draft for human review. Do not merge without reviewing the changes.

Generated in 6m 08s.

Review findings

Let me look at the postgres env.ts for reference and check the dotenv version installed:Now I have full context. The three findings are clear:

Medium (env.ts:8): JSDoc says "Mirrors" but implementation diverges from postgres's dotenv.config approach — reword.
Low (env.test.ts:75): original captured but never restored in afterEach — restore/delete it.
Low (env.ts:19): Document that DOTENV_KEY/.env.vault is intentionally dropped (dotenv 16.6.1's config() supports vaults; parse() does not).

Let me apply the fixes:Let me verify the edits and check the root project for gate commands:The diff looks correct. Now let me run the gates — bun test, tsc --noEmit in the extension, and the repo-root npm test:All gates pass: bun test in extensions/mongodb (66 pass / 0 fail), bunx tsc --noEmit (exit 0), and repo-root npm test (58 pass / 0 fail). No lint tooling is configured in the repo (no eslint config or lint script), so that gate is not applicable.

All three findings were feasible and resolved in this single pass:

Medium — env.ts:8: Reworded the JSDoc from "Mirrors extensions/postgres/env.ts" to "Based on extensions/postgres/env.ts, but parses via dotenv.parse (not dotenv.config)" so future "cleanup" back to dotenv.config is not invited.
Low — env.test.ts:75: Moved process.env.MONGODB_URI capture to describe scope and restored/deleted it in afterEach, so a regression that mutates process.env can no longer leak state into later tests.
Low — env.ts:19: Added a JSDoc note that DOTENV_KEY/.env.vault handling (present in dotenv.config, dropped by dotenv.parse) is intentionally not supported by the mongodb loader.

Review Findings — 136

Summary

The change reworks extensions/mongodb/src/env.ts's loadEnvFile() to read .env via fs.readFileSync + dotenv.parse inside a try/catch (removing the process.env mutation side effect of the previous dotenv.config({ path }) approach) and adds three unit tests covering unreadable-.env behavior, non-mutation of process.env, and mixed-case nested MONGODB_URI lookup. All gates ran and passed: bun test in extensions/mongodb (66 pass / 0 fail), bunx tsc --noEmit (the extension's check script, exit 0), and the repo-root npm test (58 pass / 0 fail). No lint tooling is configured anywhere in the repo (no eslint config or lint script), so the lint gate is not applicable. The change meets the issue's stated scope and the Phase 1.1 test checklist in extensions/mongodb/docs/IMPLEMENTATION_PLAN.md; only minor documentation/consistency and test-hygiene nits remain.

All three findings were remediated in this pass: the JSDoc now says the module is "Based on" (not "Mirrors") the postgres loader and explains the dotenv.parse vs dotenv.config divergence plus the intentional drop of DOTENV_KEY/.env.vault; and the process.env.MONGODB_URI capture is now restored/deleted in afterEach. The follow-up suggestion to align extensions/postgres/env.ts remains a candidate for a separate issue.

Critical

(none)

High

(none)

Medium

extensions/mongodb/src/env.ts:8 — The JSDoc still says the module "Mirrors extensions/postgres/env.ts", but the implementation now deliberately diverges: extensions/postgres/env.ts still uses dotenv.config({ path }), which populates process.env, while this change uses dotenv.parse specifically to avoid that mutation. Repo docs (README.md, DESIGN.md, IMPLEMENTATION_PLAN.md) also repeatedly describe the mongodb extension as "mirroring" the postgres extension, so the comment is misleading and invites a future "cleanup" back to dotenv.config that would reintroduce the exact process.env mutation this issue removes. Suggested fix: reword the JSDoc, e.g. "Based on extensions/postgres/env.ts, but parses via dotenv.parse (not dotenv.config) so loading the project's .env never mutates process.env", and consider a follow-up issue to align extensions/postgres/env.ts with the same approach.

Low

extensions/mongodb/src/env.test.ts:75 — Test hygiene: process.env.MONGODB_URI is captured into original but never restored or deleted in afterEach. If a future regression makes loadEnvFile mutate process.env, this test fails and leaves MONGODB_URI set for the remainder of the test process, potentially masking/affecting later tests. Suggested fix: in afterEach, restore/delete the variable (e.g. delete process.env.MONGODB_URI or set it back to original).
extensions/mongodb/src/env.ts:19 — Subtle behavior change vs. the previous implementation worth documenting: dotenv.config({ path }) honored DOTENV_KEY/.env.vault when DOTENV_KEY was set in the environment, whereas dotenv.parse reads the plain .env file directly and ignores the vault path. This repo does not use dotenv vaults, so there is no practical impact, but the JSDoc note about "approaches the issue specifies" does not mention that vault/DOTENV_KEY handling is dropped. Suggested fix: add a one-line note in the JSDoc (or README) that DOTENV_KEY/.env.vault is intentionally not supported by the mongodb extension's loader.

## 136: Implement src/env.ts with unit tests (MONGODB_URI resolution) Implements [136](https://git.excelera.net/david/pi-extensions-and-skills/issues/136). > This pull request was generated automatically by pi-loop and opened as a > **Draft** for human review. Do not merge without reviewing the changes. Generated in 6m 08s. ## Review findings Let me look at the postgres env.ts for reference and check the dotenv version installed:Now I have full context. The three findings are clear: 1. **Medium** (env.ts:8): JSDoc says "Mirrors" but implementation diverges from postgres's `dotenv.config` approach — reword. 2. **Low** (env.test.ts:75): `original` captured but never restored in `afterEach` — restore/delete it. 3. **Low** (env.ts:19): Document that `DOTENV_KEY`/`.env.vault` is intentionally dropped (dotenv 16.6.1's `config()` supports vaults; `parse()` does not). Let me apply the fixes:Let me verify the edits and check the root project for gate commands:The diff looks correct. Now let me run the gates — `bun test`, `tsc --noEmit` in the extension, and the repo-root `npm test`:All gates pass: `bun test` in `extensions/mongodb` (66 pass / 0 fail), `bunx tsc --noEmit` (exit 0), and repo-root `npm test` (58 pass / 0 fail). No lint tooling is configured in the repo (no eslint config or lint script), so that gate is not applicable. All three findings were feasible and resolved in this single pass: 1. **Medium — env.ts:8**: Reworded the JSDoc from "Mirrors `extensions/postgres/env.ts`" to "Based on `extensions/postgres/env.ts`, but parses via `dotenv.parse` (not `dotenv.config`)" so future "cleanup" back to `dotenv.config` is not invited. 2. **Low — env.test.ts:75**: Moved `process.env.MONGODB_URI` capture to describe scope and restored/deleted it in `afterEach`, so a regression that mutates `process.env` can no longer leak state into later tests. 3. **Low — env.ts:19**: Added a JSDoc note that `DOTENV_KEY`/`.env.vault` handling (present in `dotenv.config`, dropped by `dotenv.parse`) is intentionally not supported by the mongodb loader. # Review Findings — 136 ## Summary The change reworks `extensions/mongodb/src/env.ts`'s `loadEnvFile()` to read `.env` via `fs.readFileSync` + `dotenv.parse` inside a try/catch (removing the `process.env` mutation side effect of the previous `dotenv.config({ path })` approach) and adds three unit tests covering unreadable-`.env` behavior, non-mutation of `process.env`, and mixed-case nested `MONGODB_URI` lookup. All gates ran and passed: `bun test` in `extensions/mongodb` (66 pass / 0 fail), `bunx tsc --noEmit` (the extension's `check` script, exit 0), and the repo-root `npm test` (58 pass / 0 fail). No lint tooling is configured anywhere in the repo (no eslint config or lint script), so the lint gate is not applicable. The change meets the issue's stated scope and the Phase 1.1 test checklist in `extensions/mongodb/docs/IMPLEMENTATION_PLAN.md`; only minor documentation/consistency and test-hygiene nits remain. All three findings were remediated in this pass: the JSDoc now says the module is "Based on" (not "Mirrors") the postgres loader and explains the `dotenv.parse` vs `dotenv.config` divergence plus the intentional drop of `DOTENV_KEY`/`.env.vault`; and the `process.env.MONGODB_URI` capture is now restored/deleted in `afterEach`. The follow-up suggestion to align `extensions/postgres/env.ts` remains a candidate for a separate issue. ## Critical (none) ## High (none) ## Medium - [x] `extensions/mongodb/src/env.ts:8` — The JSDoc still says the module "Mirrors `extensions/postgres/env.ts`", but the implementation now deliberately diverges: `extensions/postgres/env.ts` still uses `dotenv.config({ path })`, which populates `process.env`, while this change uses `dotenv.parse` specifically to avoid that mutation. Repo docs (README.md, DESIGN.md, IMPLEMENTATION_PLAN.md) also repeatedly describe the mongodb extension as "mirroring" the postgres extension, so the comment is misleading and invites a future "cleanup" back to `dotenv.config` that would reintroduce the exact `process.env` mutation this issue removes. Suggested fix: reword the JSDoc, e.g. "Based on `extensions/postgres/env.ts`, but parses via `dotenv.parse` (not `dotenv.config`) so loading the project's `.env` never mutates `process.env`", and consider a follow-up issue to align `extensions/postgres/env.ts` with the same approach. ## Low - [x] `extensions/mongodb/src/env.test.ts:75` — Test hygiene: `process.env.MONGODB_URI` is captured into `original` but never restored or deleted in `afterEach`. If a future regression makes `loadEnvFile` mutate `process.env`, this test fails and leaves `MONGODB_URI` set for the remainder of the test process, potentially masking/affecting later tests. Suggested fix: in `afterEach`, restore/delete the variable (e.g. `delete process.env.MONGODB_URI` or set it back to `original`). - [x] `extensions/mongodb/src/env.ts:19` — Subtle behavior change vs. the previous implementation worth documenting: `dotenv.config({ path })` honored `DOTENV_KEY`/`.env.vault` when `DOTENV_KEY` was set in the environment, whereas `dotenv.parse` reads the plain `.env` file directly and ignores the vault path. This repo does not use dotenv vaults, so there is no practical impact, but the JSDoc note about "approaches the issue specifies" does not mention that vault/`DOTENV_KEY` handling is dropped. Suggested fix: add a one-line note in the JSDoc (or README) that `DOTENV_KEY`/`.env.vault` is intentionally not supported by the mongodb extension's loader.

david added 2 commits

2026-08-31 23:46:50 +00:00