328: MSSQL: mssql_table_schema tool (parameterised) #352

Merged

david merged 2 commits from feature/issue-328/mssql-mssql-table-schema-tool-parameterised into main

2026-11-11 00:33:50 +00:00

david commented

2026-11-11 00:33:48 +00:00

Owner

328: MSSQL: mssql_table_schema tool (parameterised)

Implements 328.

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

Review findings

Review Findings — 328

Summary

I fixed all six findings. The mssql suite passes: node --test extensions/mssql/*.test.ts ran 155 tests with 154 passing, 0 failing and 1 todo. runQuery now throws a clear error when params are supplied but input is unavailable. I added three runner tests for binding order, an error thrown by input, and that missing-input case. The README documents the new tool. The not-found message now names the schema and table, and the existing test expects the new text. The audit entry for that case stays as "executed", because the query itself ran successfully. The mssql import is renamed to mssql, which removes the shadowing. Parameters are now NVarChar(128), and the PK join also matches on TABLE_SCHEMA.

Critical

High

extensions/mssql/runner.ts:120 — The new params loop in runQuery has no direct test in runner.test.ts. Binding is only exercised through the stubbed tools test. request.input?.(...) also silently skips binding if input is missing, so the query would then fail with an opaque "must declare the scalar variable" error. Suggested fix: add runner tests covering params bound in order before query(), and an error thrown by input being rejected. Replace the optional call with an explicit check that throws a clear error when params are supplied but input is unavailable.

Medium

extensions/mssql/README.md:3 — The README still says only mssql_query, mssql_query_one and mssql_list_tables are implemented. Suggested fix: add mssql_table_schema, with its parameters (table_name, schema_name defaulting to dbo) and the INFORMATION_SCHEMA visibility caveat.
extensions/mssql/tools.ts:178 — The tool returns the "Table not found or not accessible." message for an empty result without audit-logging that outcome differently. It also gives no hint when the schema name is wrong. Suggested fix: confirm the audit entry for this case is intended, and mention the schema in the message, for example Table '<schema>.<table>' not found or not accessible.

Low

extensions/mssql/tools.ts:50 — The sql parameter of runAndAudit shadows the newly imported sql (mssql) default import. Suggested fix: import as import mssql from "mssql" and use mssql.NVarChar, or rename the parameter to statement.
extensions/mssql/tools.ts:97 — The PK join matches on TABLE_NAME and CONSTRAINT_SCHEMA but not TABLE_SCHEMA. It is filtered later by pk.TABLE_SCHEMA = c.TABLE_SCHEMA, so the result is correct. Suggested fix: add kcu.TABLE_SCHEMA = tc.TABLE_SCHEMA to the join for clarity.
extensions/mssql/tools.ts:166 — NVarChar is declared without a length, so the driver infers one. Suggested fix: use sql.NVarChar(128) to match sysname and keep plan caching stable.

## 328: MSSQL: mssql_table_schema tool (parameterised) Implements [328](https://git.excelera.net/david/pi-extensions-and-skills/issues/328). > 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 14s. ## Review findings # Review Findings — 328 ## Summary I fixed all six findings. The mssql suite passes: `node --test extensions/mssql/*.test.ts` ran 155 tests with 154 passing, 0 failing and 1 todo. `runQuery` now throws a clear error when params are supplied but `input` is unavailable. I added three runner tests for binding order, an error thrown by `input`, and that missing-`input` case. The README documents the new tool. The not-found message now names the schema and table, and the existing test expects the new text. The audit entry for that case stays as "executed", because the query itself ran successfully. The `mssql` import is renamed to `mssql`, which removes the shadowing. Parameters are now `NVarChar(128)`, and the PK join also matches on `TABLE_SCHEMA`. ## Critical ## High - [x] extensions/mssql/runner.ts:120 — The new `params` loop in `runQuery` has no direct test in `runner.test.ts`. Binding is only exercised through the stubbed tools test. `request.input?.(...)` also silently skips binding if `input` is missing, so the query would then fail with an opaque "must declare the scalar variable" error. Suggested fix: add runner tests covering params bound in order before `query()`, and an error thrown by `input` being rejected. Replace the optional call with an explicit check that throws a clear error when params are supplied but `input` is unavailable. ## Medium - [x] extensions/mssql/README.md:3 — The README still says only `mssql_query`, `mssql_query_one` and `mssql_list_tables` are implemented. Suggested fix: add `mssql_table_schema`, with its parameters (`table_name`, `schema_name` defaulting to `dbo`) and the INFORMATION_SCHEMA visibility caveat. - [x] extensions/mssql/tools.ts:178 — The tool returns the "Table not found or not accessible." message for an empty result without audit-logging that outcome differently. It also gives no hint when the schema name is wrong. Suggested fix: confirm the audit entry for this case is intended, and mention the schema in the message, for example `Table '<schema>.<table>' not found or not accessible.` ## Low - [x] extensions/mssql/tools.ts:50 — The `sql` parameter of `runAndAudit` shadows the newly imported `sql` (mssql) default import. Suggested fix: import as `import mssql from "mssql"` and use `mssql.NVarChar`, or rename the parameter to `statement`. - [x] extensions/mssql/tools.ts:97 — The PK join matches on `TABLE_NAME` and `CONSTRAINT_SCHEMA` but not `TABLE_SCHEMA`. It is filtered later by `pk.TABLE_SCHEMA = c.TABLE_SCHEMA`, so the result is correct. Suggested fix: add `kcu.TABLE_SCHEMA = tc.TABLE_SCHEMA` to the join for clarity. - [x] extensions/mssql/tools.ts:166 — NVarChar is declared without a length, so the driver infers one. Suggested fix: use `sql.NVarChar(128)` to match `sysname` and keep plan caching stable.

david added 2 commits

2026-11-11 00:33:48 +00:00