326: MSSQL: mssql_query and mssql_query_one tools #350

Merged

david merged 2 commits from feature/issue-326/mssql-mssql-query-and-mssql-query-one-tools into main

2026-11-11 00:20:09 +00:00

david commented

2026-11-11 00:20:08 +00:00

Owner

326: MSSQL: mssql_query and mssql_query_one tools

Implements 326.

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

Review findings

Existing test expects no note with 2 rows; that's now intended behaviour, so I'll update it.# Review Findings — 326

Summary

I fixed the Medium and Low findings. sanitiseErrorMessage now also redacts ADO-style credentials. mssql_query_one now runs with maxRows: 1 and appends a note when more rows exist. Its tool description now says only the first row is returned. I added tests for ADO redaction, the single-row truncation note and the timeout path, and commented the pool.query stub. After these changes the mssql tests pass (141 pass, 0 fail), and tsc --noEmit reports no mssql errors. The High finding is left open because wiring the extension into index.ts and package.json looks like a later issue's scope, and I haven't confirmed that a follow-up issue exists.

Critical

High

extensions/mssql/index.ts:4 — registerMssqlTools is never called. The entry point is still an empty function, and extensions/mssql/index.ts is not in the pi.extensions list in package.json. As a result mssql_query and mssql_query_one are not available to users after this change. This may be deliberate if a later issue does the wiring, but the issue title implies the tools should exist. Suggested fix: confirm the wiring is covered by a follow-up issue. If it isn't, build the pool, read the settings, create the audit logger and call registerMssqlTools from index.ts, then register the extension in package.json.

Medium

extensions/mssql/tools.ts:17 — sanitiseErrorMessage only redacts scheme://… URLs. Driver errors can also contain ADO-style connection strings such as Password=…; or User ID=…. Those pass through to both the tool output and the audit log. Suggested fix: also redact (password|pwd|user id|uid)\s*=\s*[^;\s]+ case-insensitively, and add a test for it.
extensions/mssql/tools.ts:94 — mssql_query_one runs the query with the full maxRows cap and then discards every row but the first. That wastes bandwidth and server work. Suggested fix: pass maxRows: 1 to runQuery for this tool. guardedQuery would need a maxRows override, and truncation should then be reported as "more rows exist" rather than as a cap hit.
extensions/mssql/tools.ts:95 — mssql_query_one returns details.truncated when the cap was hit, but the text never tells the model that more rows exist, so it may read the first row as the only one. Suggested fix: append a short note when truncated is true, or document in the tool description that only the first row is returned.

Low

extensions/mssql/tools.test.ts:14 — The pool type RequestPool & { query: () => never } is a workaround for the "never use pool.query" assertion. It does not appear to be checked against the real mssql pool. Suggested fix: add a comment explaining the intent, or drop the query stub and assert only on request().
extensions/mssql/tools.test.ts:38 — There is no test for the timeout path (QueryTimeoutError) through the tools, so audit and message handling for timeouts is unverified. Suggested fix: add one test where the fake request never emits done, using a small queryTimeoutMs.

## 326: MSSQL: mssql_query and mssql_query_one tools Implements [326](https://git.excelera.net/david/pi-extensions-and-skills/issues/326). > 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 18s. ## Review findings Existing test expects no note with 2 rows; that's now intended behaviour, so I'll update it.# Review Findings — 326 ## Summary I fixed the Medium and Low findings. `sanitiseErrorMessage` now also redacts ADO-style credentials. `mssql_query_one` now runs with `maxRows: 1` and appends a note when more rows exist. Its tool description now says only the first row is returned. I added tests for ADO redaction, the single-row truncation note and the timeout path, and commented the `pool.query` stub. After these changes the mssql tests pass (141 pass, 0 fail), and `tsc --noEmit` reports no mssql errors. The High finding is left open because wiring the extension into `index.ts` and `package.json` looks like a later issue's scope, and I haven't confirmed that a follow-up issue exists. ## Critical ## High - [ ] extensions/mssql/index.ts:4 — `registerMssqlTools` is never called. The entry point is still an empty function, and `extensions/mssql/index.ts` is not in the `pi.extensions` list in `package.json`. As a result `mssql_query` and `mssql_query_one` are not available to users after this change. This may be deliberate if a later issue does the wiring, but the issue title implies the tools should exist. Suggested fix: confirm the wiring is covered by a follow-up issue. If it isn't, build the pool, read the settings, create the audit logger and call `registerMssqlTools` from `index.ts`, then register the extension in `package.json`. ## Medium - [x] extensions/mssql/tools.ts:17 — `sanitiseErrorMessage` only redacts `scheme://…` URLs. Driver errors can also contain ADO-style connection strings such as `Password=…;` or `User ID=…`. Those pass through to both the tool output and the audit log. Suggested fix: also redact `(password|pwd|user id|uid)\s*=\s*[^;\s]+` case-insensitively, and add a test for it. - [x] extensions/mssql/tools.ts:94 — `mssql_query_one` runs the query with the full `maxRows` cap and then discards every row but the first. That wastes bandwidth and server work. Suggested fix: pass `maxRows: 1` to `runQuery` for this tool. `guardedQuery` would need a `maxRows` override, and truncation should then be reported as "more rows exist" rather than as a cap hit. - [x] extensions/mssql/tools.ts:95 — `mssql_query_one` returns `details.truncated` when the cap was hit, but the text never tells the model that more rows exist, so it may read the first row as the only one. Suggested fix: append a short note when `truncated` is true, or document in the tool description that only the first row is returned. ## Low - [x] extensions/mssql/tools.test.ts:14 — The `pool` type `RequestPool & { query: () => never }` is a workaround for the "never use `pool.query`" assertion. It does not appear to be checked against the real `mssql` pool. Suggested fix: add a comment explaining the intent, or drop the `query` stub and assert only on `request()`. - [x] extensions/mssql/tools.test.ts:38 — There is no test for the timeout path (`QueryTimeoutError`) through the tools, so audit and message handling for timeouts is unverified. Suggested fix: add one test where the fake request never emits `done`, using a small `queryTimeoutMs`.

david added 2 commits

2026-11-11 00:20:08 +00:00