329: MSSQL: index.ts wiring (load env, refuse on invalid limits, pool, shutdown) #353

Merged

david merged 2 commits from feature/issue-329/mssql-index-ts-wiring-load-env-refuse-on-invalid-l into main

2026-11-11 00:36:11 +00:00

david commented

2026-11-11 00:36:09 +00:00

Owner

329: MSSQL: index.ts wiring (load env, refuse on invalid limits, pool, shutdown)

Implements 329.

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 1m 28s.

Review findings

Review Findings — 329

Summary

I fixed the High finding, all three Medium findings and the two Low findings that touched code or tests. node --test extensions/mssql/index.test.ts passes all 14 tests. The repo has no build or lint script, so those gates did not run. I did not run the full npm test suite.

One finding remains: the root README.md still has no mssql entry. I left it for a follow-up documentation issue. I did document the env precedence in extensions/mssql/README.md.

Critical

High

extensions/mssql/index.ts:56 — No error listener is attached to the pool after connect(). mssql.ConnectionPool is an EventEmitter. A later connection drop or idle-connection error emitted with no listener becomes an uncaught exception and can crash the host pi process. Suggested fix: add ManagedPool.on?("error", ...) (or extend the interface with on) and attach a handler right after createPool that logs a generic message without credentials. Add a test that emitting error does not throw.

Medium

extensions/mssql/index.ts:73 — The session_shutdown handler awaits pool.close() without a try/catch. A rejected close during shutdown surfaces as an unhandled error. Suggested fix: use await pool.close().catch(() => undefined), as the connect-failure path already does. Add a test where close rejects.
extensions/mssql/index.ts:30 — audit.log sits inside the same try as the permission check. If the audit write throws, the catch prints "could not verify that this login is read-only", which is misleading because the check actually succeeded. The failure is also silently lost. Suggested fix: wrap only the checkWriteAccess call in the try/catch and keep audit failures separate, since createAuditLogger already reports write failures to stderr.
extensions/mssql/index.ts:80 — { ...loadEnvFile(), ...pickStrings(process.env) } merges the two sources by exact key. findEnvValue is case-insensitive and returns the first match. Keys that differ only in case (for example .env mssql_url against process MSSQL_URL) therefore resolve to the .env value, not the process value. The intended precedence is unclear, and the mongodb extension and README say .env only. Suggested fix: decide on one precedence. Either drop the process-env source, or normalise keys to upper case before merging so process env wins deterministically. Document the choice in extensions/mssql/README.md, and add a test for the precedence.

Low

extensions/mssql/index.ts:31 — createAuditLogger() is called separately in warnOnWriteAccess (line 31) and again at the registerMssqlTools call (line 68) when deps.audit is absent. Suggested fix: resolve const audit = deps.audit ?? createAuditLogger() once in loadMssqlExtension and pass it to both.
extensions/mssql/index.test.ts:96 — load_permissionCheckThrows_stillRegistersTools only asserts that the password is not leaked. It does not assert that the "could not verify" warning is printed. Suggested fix: add assert.ok(output.includes("could not verify")).
README.md:1 — The README extension table ("Extensions (10)", "Prerequisites", "Extension notes" and the repository layout) has no mssql entry. Suggested fix: add mssql documentation, either in this change or in a follow-up documentation issue.

## 329: MSSQL: index.ts wiring (load env, refuse on invalid limits, pool, shutdown) Implements [329](https://git.excelera.net/david/pi-extensions-and-skills/issues/329). > 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 1m 28s. ## Review findings # Review Findings — 329 ## Summary I fixed the High finding, all three Medium findings and the two Low findings that touched code or tests. `node --test extensions/mssql/index.test.ts` passes all 14 tests. The repo has no build or lint script, so those gates did not run. I did not run the full `npm test` suite. One finding remains: the root `README.md` still has no `mssql` entry. I left it for a follow-up documentation issue. I did document the env precedence in `extensions/mssql/README.md`. ## Critical ## High - [x] extensions/mssql/index.ts:56 — No `error` listener is attached to the pool after `connect()`. `mssql.ConnectionPool` is an EventEmitter. A later connection drop or idle-connection error emitted with no listener becomes an uncaught exception and can crash the host pi process. Suggested fix: add `ManagedPool.on?("error", ...)` (or extend the interface with `on`) and attach a handler right after `createPool` that logs a generic message without credentials. Add a test that emitting `error` does not throw. ## Medium - [x] extensions/mssql/index.ts:73 — The `session_shutdown` handler awaits `pool.close()` without a try/catch. A rejected close during shutdown surfaces as an unhandled error. Suggested fix: use `await pool.close().catch(() => undefined)`, as the connect-failure path already does. Add a test where `close` rejects. - [x] extensions/mssql/index.ts:30 — `audit.log` sits inside the same `try` as the permission check. If the audit write throws, the catch prints "could not verify that this login is read-only", which is misleading because the check actually succeeded. The failure is also silently lost. Suggested fix: wrap only the `checkWriteAccess` call in the try/catch and keep audit failures separate, since `createAuditLogger` already reports write failures to stderr. - [x] extensions/mssql/index.ts:80 — `{ ...loadEnvFile(), ...pickStrings(process.env) }` merges the two sources by exact key. `findEnvValue` is case-insensitive and returns the first match. Keys that differ only in case (for example `.env` `mssql_url` against process `MSSQL_URL`) therefore resolve to the `.env` value, not the process value. The intended precedence is unclear, and the mongodb extension and README say `.env` only. Suggested fix: decide on one precedence. Either drop the process-env source, or normalise keys to upper case before merging so process env wins deterministically. Document the choice in `extensions/mssql/README.md`, and add a test for the precedence. ## Low - [x] extensions/mssql/index.ts:31 — `createAuditLogger()` is called separately in `warnOnWriteAccess` (line 31) and again at the `registerMssqlTools` call (line 68) when `deps.audit` is absent. Suggested fix: resolve `const audit = deps.audit ?? createAuditLogger()` once in `loadMssqlExtension` and pass it to both. - [x] extensions/mssql/index.test.ts:96 — `load_permissionCheckThrows_stillRegistersTools` only asserts that the password is not leaked. It does not assert that the "could not verify" warning is printed. Suggested fix: add `assert.ok(output.includes("could not verify"))`. - [ ] README.md:1 — The README extension table ("Extensions (10)", "Prerequisites", "Extension notes" and the repository layout) has no `mssql` entry. Suggested fix: add `mssql` documentation, either in this change or in a follow-up documentation issue.

david added 2 commits

2026-11-11 00:36:09 +00:00