issue-148: Reconcile DESIGN.md and implementation plan with the implementation #164

Merged

david merged 2 commits from feature/issue-148/reconcile-design-md-and-implementation-plan-with-t into main

2026-09-01 01:16:38 +00:00

david commented

2026-09-01 01:16:37 +00:00

Owner

148: Reconcile DESIGN.md and implementation plan with the implementation

Implements 148.

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 12m 21s.

Review findings

Let me look at the IMPLEMENTATION_PLAN.md and the index.ts factory to understand the full context.The finding is confirmed: extractMongoUri({ config__MONGODB_URI: '' }) returns "" while the documented contract says "null when missing/empty". Let me check the factory and existing tests for context before fixing.The finding is confirmed and the fix is safe (the factory treats both "" and null as unconfigured via if (!uri) return;). I'll implement the suggested fix's option 1: make empty nested values behave like missing ones, matching the documented contract.Now add the tests covering the nested-empty case:Now update the docs to make the (now accurate) contract explicit:The IMPLEMENTATION_PLAN.md edit succeeded; the DESIGN.md edit was malformed — let me redo it:Now let me verify the changes and run the validation gates:All 98 tests pass (96 before + 2 new). Now run the TypeScript check and root test gates:All gates green. Let me verify the fixed behavior end-to-end and review the final diff:Let me review the complete diff to confirm the changes are clean and consistent:The fix is complete and all gates pass. Here is the annotated findings document.

Review Findings — 148

Summary

Remediation applied in a single pass. The single Low finding — extractMongoUri returning "" (not null) for empty nested keys, contradicting the documented "null when missing/empty" contract — was resolved with the suggested fix's first option: findMongoUri/extractMongoUri now treat empty nested values as missing, so extractMongoUri({ config__MONGODB_URI: "" }) returns null like the top-level case (an empty nested key also no longer masks a later non-empty nested key). Added the requested config__MONGODB_URI="" → null test in src/env.test.ts (both findMongoUri and extractMongoUri suites). The docs were updated to state the now-true contract explicitly: DESIGN.md's Configuration bullet notes empty values are treated as missing for both top-level and nested keys (the coverage bullet at DESIGN.md:260 "empty → null" is now fully accurate), and IMPLEMENTATION_PLAN.md's Phase 1.1 test list plus the Phase 6.1 reconciliation note record the fix. No end-to-end behavior changed — the factory's if (!uri) return; treated "" and null identically, so this only closes the intermediate-contract drift. All gates green: bun test in extensions/mongodb (98 pass / 0 fail, up from 96 with the 2 new tests), bun run check (tsc --noEmit) clean, and root npm test (58 pass / 0 fail). No findings remain.

Critical

No critical findings.

High

No high findings.

Medium

No medium findings.

Low

extensions/mongodb/docs/DESIGN.md:107 — extractMongoUri(envVars) is documented as "returns the URI string, or null when missing/empty", but the contract does not hold for nested empty values: extractMongoUri({ config__MONGODB_URI: "" }) returns "", not null (verified via bun -e against src/env.ts; the nested loop in findMongoUri returns the empty value as-is, while only the top-level branch treats an empty string as missing). The factory masks this end-to-end (if (!uri) return;), so there is no user-visible impact, but this is precisely the class of drift issue #148 exists to close, and the same overstatement appears in the coverage bullet at DESIGN.md:260 ("empty → null"), which only the top-level test case actually exercises. Suggested fix: either make findMongoUri/extractMongoUri return null for empty nested values too (and add a config__MONGODB_URI="" → null test in src/env.test.ts), or narrow the doc wording, e.g. "empty top-level value → null; the factory treats any empty/absent URI as unconfigured".

## 148: Reconcile DESIGN.md and implementation plan with the implementation Implements [148](https://git.excelera.net/david/pi-extensions-and-skills/issues/148). > 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 12m 21s. ## Review findings Let me look at the IMPLEMENTATION_PLAN.md and the index.ts factory to understand the full context.The finding is confirmed: `extractMongoUri({ config__MONGODB_URI: '' })` returns `""` while the documented contract says "null when missing/empty". Let me check the factory and existing tests for context before fixing.The finding is confirmed and the fix is safe (the factory treats both `""` and `null` as unconfigured via `if (!uri) return;`). I'll implement the suggested fix's option 1: make empty nested values behave like missing ones, matching the documented contract.Now add the tests covering the nested-empty case:Now update the docs to make the (now accurate) contract explicit:The IMPLEMENTATION_PLAN.md edit succeeded; the DESIGN.md edit was malformed — let me redo it:Now let me verify the changes and run the validation gates:All 98 tests pass (96 before + 2 new). Now run the TypeScript check and root test gates:All gates green. Let me verify the fixed behavior end-to-end and review the final diff:Let me review the complete diff to confirm the changes are clean and consistent:The fix is complete and all gates pass. Here is the annotated findings document. # Review Findings — 148 ## Summary Remediation applied in a single pass. The single Low finding — `extractMongoUri` returning `""` (not `null`) for empty **nested** keys, contradicting the documented "null when missing/empty" contract — was resolved with the suggested fix's first option: `findMongoUri`/`extractMongoUri` now treat empty nested values as missing, so `extractMongoUri({ config__MONGODB_URI: "" })` returns `null` like the top-level case (an empty nested key also no longer masks a later non-empty nested key). Added the requested `config__MONGODB_URI="" → null` test in `src/env.test.ts` (both `findMongoUri` and `extractMongoUri` suites). The docs were updated to state the now-true contract explicitly: DESIGN.md's Configuration bullet notes empty values are treated as missing for both top-level and nested keys (the coverage bullet at DESIGN.md:260 "empty → null" is now fully accurate), and IMPLEMENTATION_PLAN.md's Phase 1.1 test list plus the Phase 6.1 reconciliation note record the fix. No end-to-end behavior changed — the factory's `if (!uri) return;` treated `""` and `null` identically, so this only closes the intermediate-contract drift. All gates green: `bun test` in `extensions/mongodb` (98 pass / 0 fail, up from 96 with the 2 new tests), `bun run check` (`tsc --noEmit`) clean, and root `npm test` (58 pass / 0 fail). No findings remain. ## Critical - [ ] _No critical findings._ ## High - [ ] _No high findings._ ## Medium - [ ] _No medium findings._ ## Low - [x] `extensions/mongodb/docs/DESIGN.md:107` — `extractMongoUri(envVars)` is documented as "returns the URI string, or `null` when missing/empty", but the contract does not hold for **nested empty** values: `extractMongoUri({ config__MONGODB_URI: "" })` returns `""`, not `null` (verified via `bun -e` against `src/env.ts`; the nested loop in `findMongoUri` returns the empty value as-is, while only the top-level branch treats an empty string as missing). The factory masks this end-to-end (`if (!uri) return;`), so there is no user-visible impact, but this is precisely the class of drift issue #148 exists to close, and the same overstatement appears in the coverage bullet at DESIGN.md:260 ("empty → null"), which only the top-level test case actually exercises. Suggested fix: either make `findMongoUri`/`extractMongoUri` return `null` for empty nested values too (and add a `config__MONGODB_URI="" → null` test in `src/env.test.ts`), or narrow the doc wording, e.g. "empty top-level value → `null`; the factory treats any empty/absent URI as unconfigured".

david added 2 commits

2026-09-01 01:16:38 +00:00