issue-143: Implement index.ts async factory + lifecycle with tests #160

Merged

david merged 2 commits from feature/issue-143/implement-index-ts-async-factory-lifecycle-with-te into main

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

david commented

2026-09-01 00:40:15 +00:00

Owner

143: Implement index.ts async factory + lifecycle with tests

Implements 143.

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 10m 23s.

Review findings

Now I have a full picture. Let me apply the fixes. First, the test file — extract a withCapturedLog helper and use it across the factory tests, plus assert the URI in the createClient seam:Now the documentation reconciliation for the Medium finding — DESIGN.md (Module layout + Connection Lifecycle) and the plan's 6.1 item:Let me verify the edits and check the package scripts:Now run the gates — mongodb tests and type check in parallel:Mongodb tests pass (93/93) and tsc --noEmit is clean. Now the root gate:All gates green. Let me review the final diff of my remediation changes:All three findings are resolved and all gates are green (bun test 93 pass, bunx tsc --noEmit clean, root npm test 58 pass).

Review Findings — 143

Summary

Stage 1 implements the MongoDB extension async factory + lifecycle (Phase 4.1 of the plan) and adds four tests covering unconfigured-silent behavior, tool registration/execution via registerMongoTools, and lazy-client + session_shutdown cleanup. All gates ran and passed: bun test (mongodb, 93 pass), bunx tsc --noEmit (clean), root npm test (58 pass); no lint gate exists in the repo. No critical or high findings — the remaining items are a design-consistency gap and two minor test-quality nits. All three findings were remediated in this pass: the createClient test seam is now documented in DESIGN.md (Module layout + Connection Lifecycle) with the plan's Phase 6.1 reconciliation item ticked, the lazy-client test asserts the resolved URI is passed to the seam, and the duplicated console.log capture/restore scaffolding is extracted into a shared withCapturedLog helper used across all factory tests. No findings remain.

Critical

(none)

High

(none)

Medium

extensions/mongodb/index.ts:20 — The newly exported MongoExtensionOptions/createClient hook is production API surface that exists solely for tests (its own JSDoc at index.ts:17 points at the test-only src/tools/testClient.ts), and it diverges from the documented "mirrors postgres" factory shape (export default async function (pi) with no options param) in DESIGN.md's Architecture/Connection Lifecycle and the plan's Phase 4.1. It is backward-compatible at runtime, but the seam should be reconciled or narrowed. Suggested fix: document the optional createClient test seam in DESIGN.md's Module layout / Connection Lifecycle (and tick the Phase 6.1 reconciliation item), or keep the interface unexported so production API surface stays parallel to postgres.

Low

extensions/mongodb/index.test.ts:278 — createClient: () => injected ignores the resolved URI, so the test would still pass if the factory stopped passing the URI (or passed a wrong one). Suggested fix: assert the argument inside the injected factory, e.g. createClient: (uri) => { expect(uri).toBe("mongodb://localhost:27017/mydb"); return injected; }.
extensions/mongodb/index.test.ts:153,177 — The console.log capture/restore scaffolding is now duplicated across three tests in the file (the two new "logs nothing" tests plus the existing configured test). Suggested fix: extract a small local helper (e.g. withCapturedLog(fn) returning logged) and use it in all three to reduce repetition.

## 143: Implement index.ts async factory + lifecycle with tests Implements [143](https://git.excelera.net/david/pi-extensions-and-skills/issues/143). > 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 10m 23s. ## Review findings Now I have a full picture. Let me apply the fixes. First, the test file — extract a `withCapturedLog` helper and use it across the factory tests, plus assert the URI in the `createClient` seam:Now the documentation reconciliation for the Medium finding — DESIGN.md (Module layout + Connection Lifecycle) and the plan's 6.1 item:Let me verify the edits and check the package scripts:Now run the gates — mongodb tests and type check in parallel:Mongodb tests pass (93/93) and `tsc --noEmit` is clean. Now the root gate:All gates green. Let me review the final diff of my remediation changes:All three findings are resolved and all gates are green (`bun test` 93 pass, `bunx tsc --noEmit` clean, root `npm test` 58 pass). # Review Findings — 143 ## Summary Stage 1 implements the MongoDB extension async factory + lifecycle (Phase 4.1 of the plan) and adds four tests covering unconfigured-silent behavior, tool registration/execution via `registerMongoTools`, and lazy-client + `session_shutdown` cleanup. All gates ran and passed: `bun test` (mongodb, 93 pass), `bunx tsc --noEmit` (clean), root `npm test` (58 pass); no lint gate exists in the repo. No critical or high findings — the remaining items are a design-consistency gap and two minor test-quality nits. All three findings were remediated in this pass: the `createClient` test seam is now documented in DESIGN.md (Module layout + Connection Lifecycle) with the plan's Phase 6.1 reconciliation item ticked, the lazy-client test asserts the resolved URI is passed to the seam, and the duplicated `console.log` capture/restore scaffolding is extracted into a shared `withCapturedLog` helper used across all factory tests. No findings remain. ## Critical - [ ] (none) ## High - [ ] (none) ## Medium - [x] extensions/mongodb/index.ts:20 — The newly exported `MongoExtensionOptions`/`createClient` hook is production API surface that exists solely for tests (its own JSDoc at index.ts:17 points at the test-only `src/tools/testClient.ts`), and it diverges from the documented "mirrors postgres" factory shape (`export default async function (pi)` with no options param) in DESIGN.md's Architecture/Connection Lifecycle and the plan's Phase 4.1. It is backward-compatible at runtime, but the seam should be reconciled or narrowed. Suggested fix: document the optional `createClient` test seam in DESIGN.md's Module layout / Connection Lifecycle (and tick the Phase 6.1 reconciliation item), or keep the interface unexported so production API surface stays parallel to postgres. ## Low - [x] extensions/mongodb/index.test.ts:278 — `createClient: () => injected` ignores the resolved URI, so the test would still pass if the factory stopped passing the URI (or passed a wrong one). Suggested fix: assert the argument inside the injected factory, e.g. `createClient: (uri) => { expect(uri).toBe("mongodb://localhost:27017/mydb"); return injected; }`. - [x] extensions/mongodb/index.test.ts:153,177 — The `console.log` capture/restore scaffolding is now duplicated across three tests in the file (the two new "logs nothing" tests plus the existing configured test). Suggested fix: extract a small local helper (e.g. `withCapturedLog(fn)` returning `logged`) and use it in all three to reduce repetition.

david added 2 commits

2026-09-01 00:40:15 +00:00