320: MSSQL: loadEnvFile and MSSQL_URL parsing #344

Merged

david merged 2 commits from feature/issue-320/mssql-loadenvfile-and-mssql-url-parsing into main

2026-11-11 00:00:13 +00:00

david commented

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

Owner

320: MSSQL: loadEnvFile and MSSQL_URL parsing

Implements 320.

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 51s.

Review findings

Decisions: reject empty user/database and multi-segment paths (return null).# Review Findings — 320

Summary

I fixed every finding except the dotenv import note. extractMssqlConnInfo now has tests. parseMssqlUrl returns null for an empty user, an empty database or a multi-segment path, and each case has a test. I renamed the misleading test and asserted it returns null. I removed the existsSync call from loadEnvFile. node --test extensions/mssql/env.test.ts passes all 23 tests. The repo has no build or lint script, and I did not run the full npm test suite. I left the dotenv note unresolved because it asked only for confirmation and needed no change. The namespace import works in the current test run.

Critical

High

extensions/mssql/env.ts:82 — extractMssqlConnInfo is exported and holds the case-insensitive MSSQL_URL lookup plus parse, but env.test.ts never exercises it. Suggested fix: add tests for a present URL (including a lowercase key), an absent key returning null, an empty-string value returning null, and an invalid URL returning null.

Medium

extensions/mssql/env.ts:67 — parseMssqlUrl accepts a URL with an empty username (mssql://h/db) or an empty database (mssql://u@h) and returns user: "" or database: "". ConnInfo types both as required strings, so callers could attempt a connection with empty credentials or an empty database. Suggested fix: decide the intended behaviour and either return null when user or database is empty, or make the fields optional. Add tests for whichever you choose.
extensions/mssql/env.ts:68 — parsed.pathname.slice(1) leaves any extra path segments in the database name (/db/extra becomes db/extra). Suggested fix: reject or truncate a multi-segment path, with a test.

Low

extensions/mssql/env.test.ts:84 — invalidUrl_doesNotThrowMessageContainingPassword only asserts that the call doesn't throw, so its name overstates what it checks. Suggested fix: rename it to invalidUrl_doesNotThrow, or assert the result is null.
extensions/mssql/env.ts:15 — loadEnvFile uses existsSync and then readFileSync, which is a small TOCTOU gap. It is harmless here because the read is already wrapped in a try/catch. Suggested fix: drop the existsSync call and rely on the try/catch alone.
extensions/mssql/env.ts:1 — dotenv is imported with a namespace import. Suggested fix: confirm dotenv.parse resolves under Node ESM type-stripping. It works in the current test run, so this is only a note.

## 320: MSSQL: loadEnvFile and MSSQL_URL parsing Implements [320](https://git.excelera.net/david/pi-extensions-and-skills/issues/320). > 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 51s. ## Review findings Decisions: reject empty user/database and multi-segment paths (return null).# Review Findings — 320 ## Summary I fixed every finding except the `dotenv` import note. `extractMssqlConnInfo` now has tests. `parseMssqlUrl` returns `null` for an empty user, an empty database or a multi-segment path, and each case has a test. I renamed the misleading test and asserted it returns `null`. I removed the `existsSync` call from `loadEnvFile`. `node --test extensions/mssql/env.test.ts` passes all 23 tests. The repo has no build or lint script, and I did not run the full `npm test` suite. I left the `dotenv` note unresolved because it asked only for confirmation and needed no change. The namespace import works in the current test run. ## Critical ## High - [x] extensions/mssql/env.ts:82 — `extractMssqlConnInfo` is exported and holds the case-insensitive `MSSQL_URL` lookup plus parse, but `env.test.ts` never exercises it. Suggested fix: add tests for a present URL (including a lowercase key), an absent key returning `null`, an empty-string value returning `null`, and an invalid URL returning `null`. ## Medium - [x] extensions/mssql/env.ts:67 — `parseMssqlUrl` accepts a URL with an empty username (`mssql://h/db`) or an empty database (`mssql://u@h`) and returns `user: ""` or `database: ""`. `ConnInfo` types both as required strings, so callers could attempt a connection with empty credentials or an empty database. Suggested fix: decide the intended behaviour and either return `null` when `user` or `database` is empty, or make the fields optional. Add tests for whichever you choose. - [x] extensions/mssql/env.ts:68 — `parsed.pathname.slice(1)` leaves any extra path segments in the database name (`/db/extra` becomes `db/extra`). Suggested fix: reject or truncate a multi-segment path, with a test. ## Low - [x] extensions/mssql/env.test.ts:84 — `invalidUrl_doesNotThrowMessageContainingPassword` only asserts that the call doesn't throw, so its name overstates what it checks. Suggested fix: rename it to `invalidUrl_doesNotThrow`, or assert the result is `null`. - [x] extensions/mssql/env.ts:15 — `loadEnvFile` uses `existsSync` and then `readFileSync`, which is a small TOCTOU gap. It is harmless here because the read is already wrapped in a try/catch. Suggested fix: drop the `existsSync` call and rely on the try/catch alone. - [ ] extensions/mssql/env.ts:1 — `dotenv` is imported with a namespace import. Suggested fix: confirm `dotenv.parse` resolves under Node ESM type-stripping. It works in the current test run, so this is only a note.

david added 2 commits

2026-11-11 00:00:12 +00:00