321: MSSQL: certificate trust setting and limit validation (MSSQL_TRUST_SERVER_CERT, MSSQL_MAX_ROWS, MSSQL_QUERY_TIMEOUT_MS) #345

Merged

david merged 2 commits from feature/issue-321/mssql-certificate-trust-setting-and-limit-validati into main

2026-11-11 00:04:33 +00:00

david commented

2026-11-11 00:04:31 +00:00

Owner

321: MSSQL: certificate trust setting and limit validation (MSSQL_TRUST_SERVER_CERT, MSSQL_MAX_ROWS, MSSQL_QUERY_TIMEOUT_MS)

Implements 321.

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

Review findings

Review Findings — 321

Summary

I fixed one Low finding: I added a test that pins the validation order, so MSSQL_TRUST_SERVER_CERT is reported before the limits. node --test extensions/mssql/env.test.ts now passes 61 of 61 tests. I left the Medium finding unresolved. Wiring loadSettings into index.ts is staged for a follow-up step and goes beyond this issue's scope. The trimming finding needs no code change, so I left it unchecked.

Critical

High

Medium

extensions/mssql/env.ts:149 — loadSettings has no caller. DESIGN.md line 97 says an invalid limit means the extension does not load, but nothing enforces that yet, and trustServerCert is not passed to the connection config. Suggested fix: in a follow-up step, call loadSettings(loadEnvFile()) in index.ts, refuse to load and print error on failure, and pass trustServerCert to options.trustServerCertificate. If this issue is meant to cover that wiring, do it here.

Low

extensions/mssql/env.ts:112 — MSSQL_TRUST_SERVER_CERT is compared without trimming, so " true" is rejected. Tests lock this in, and failing closed is arguably right. Suggested fix: no change needed. Optionally add a sentence to DESIGN.md saying that whitespace is not tolerated.
extensions/mssql/env.test.ts:178 — The test file has no case where MSSQL_TRUST_SERVER_CERT is invalid and a limit is also invalid, so the order of validation is untested. Suggested fix: add one test that pins which variable is reported first.

## 321: MSSQL: certificate trust setting and limit validation (MSSQL_TRUST_SERVER_CERT, MSSQL_MAX_ROWS, MSSQL_QUERY_TIMEOUT_MS) Implements [321](https://git.excelera.net/david/pi-extensions-and-skills/issues/321). > 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 53s. ## Review findings # Review Findings — 321 ## Summary I fixed one Low finding: I added a test that pins the validation order, so `MSSQL_TRUST_SERVER_CERT` is reported before the limits. `node --test extensions/mssql/env.test.ts` now passes 61 of 61 tests. I left the Medium finding unresolved. Wiring `loadSettings` into `index.ts` is staged for a follow-up step and goes beyond this issue's scope. The trimming finding needs no code change, so I left it unchecked. ## Critical ## High ## Medium - [ ] extensions/mssql/env.ts:149 — `loadSettings` has no caller. DESIGN.md line 97 says an invalid limit means the extension does not load, but nothing enforces that yet, and `trustServerCert` is not passed to the connection config. Suggested fix: in a follow-up step, call `loadSettings(loadEnvFile())` in `index.ts`, refuse to load and print `error` on failure, and pass `trustServerCert` to `options.trustServerCertificate`. If this issue is meant to cover that wiring, do it here. ## Low - [ ] extensions/mssql/env.ts:112 — `MSSQL_TRUST_SERVER_CERT` is compared without trimming, so `" true"` is rejected. Tests lock this in, and failing closed is arguably right. Suggested fix: no change needed. Optionally add a sentence to DESIGN.md saying that whitespace is not tolerated. - [x] extensions/mssql/env.test.ts:178 — The test file has no case where `MSSQL_TRUST_SERVER_CERT` is invalid and a limit is also invalid, so the order of validation is untested. Suggested fix: add one test that pins which variable is reported first.

david added 2 commits

2026-11-11 00:04:31 +00:00