322: MSSQL: read-only guard module (validateSql) with corpus tests #346

Merged

david merged 2 commits from feature/issue-322/mssql-read-only-guard-module-validatesql-with-corp into main

2026-11-11 00:06:24 +00:00

david commented

2026-11-11 00:06:22 +00:00

Owner

322: MSSQL: read-only guard module (validateSql) with corpus tests

Implements 322.

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

Review findings

Move the entry, and add the Low tests.# Review Findings — 322

Summary

I fixed the nested-comment finding and the two test-coverage findings. Three findings remain unresolved.

Nested-comment entry: it was not a gap. I removed it from corpus-reject.mjs and from KNOWN_GAPS. The SQL is valid, so I added it to guard.test.ts as nestedCommentHidingWrite_isAllowed. It couldn't go in the accept corpus, because the accept-corpus test requires every entry to start with SELECT or WITH.
Test coverage: I added tests for SELECT 1\nGO\nSELECT 2, SELECT * FROM #tmp, [into] and FOR XML, which pin the documented behaviour.
Left open: the index.ts wiring (a scope decision), the tsc script (reserved for a separate chore) and the empty KNOWN_FALSE_REJECTS list (the new tests show those cases are allowed, so it stays empty).
Test gate: the mssql tests pass, with 113 pass and 0 fail. No build or lint scripts exist.

Critical

High

Medium

extensions/mssql/guard.test.ts:11 — The nested-comment entry in KNOWN_GAPS ("comment: nested comment closes early if not nested-aware") is not a gap. The guard handles nested comments correctly, and the file's own comment says so. Listing it hides a corpus label whose expected behaviour differs from SQL Server's actual behaviour, and it weakens the "must-reject" corpus. Suggested fix: remove the entry from corpus-reject.mjs, or move it to corpus-accept.mjs if the SQL is valid. Then drop it from KNOWN_GAPS.
extensions/mssql/guard.ts:115 — validateSql is not wired into mssql_query in index.ts, and the guard is not exported or referenced anywhere. If that is intentional for this issue, there is no gap. Otherwise the guard has no effect. Suggested fix: confirm a follow-up issue covers the wiring. If it does not, call validateSql before executing in index.ts.

Low

extensions/mssql/guard.ts:109 — The GO batch-separator check is effectively redundant. Anything after GO must still start a statement and pass the keyword denylist, and GO lines containing ; are already rejected. It also rejects a harmless line that starts with GO, such as an alias on its own line. Suggested fix: keep it as defence in depth, but add a test for SELECT 1\nGO\nSELECT 2 to document the intent.
extensions/mssql/guard.ts:34 — KNOWN_FALSE_REJECTS is an empty array, and the comment above it lists examples (FOR XML, temp tables, unbracketed denied-keyword columns) that are not tested. Suggested fix: add accept or reject tests for SELECT * FROM #tmp, a column named [into], and FOR XML so the documented behaviour is pinned.
package.json:31 — There is no lint or typecheck script, so the new .ts files (which use ! non-null assertions and a reason union) are unchecked beyond Node's type stripping. Suggested fix: add a tsc --noEmit script in a separate chore.

## 322: MSSQL: read-only guard module (validateSql) with corpus tests Implements [322](https://git.excelera.net/david/pi-extensions-and-skills/issues/322). > 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 37s. ## Review findings Move the entry, and add the Low tests.# Review Findings — 322 ## Summary I fixed the nested-comment finding and the two test-coverage findings. Three findings remain unresolved. - **Nested-comment entry:** it was not a gap. I removed it from `corpus-reject.mjs` and from `KNOWN_GAPS`. The SQL is valid, so I added it to `guard.test.ts` as `nestedCommentHidingWrite_isAllowed`. It couldn't go in the accept corpus, because the accept-corpus test requires every entry to start with SELECT or WITH. - **Test coverage:** I added tests for `SELECT 1\nGO\nSELECT 2`, `SELECT * FROM #tmp`, `[into]` and `FOR XML`, which pin the documented behaviour. - **Left open:** the `index.ts` wiring (a scope decision), the `tsc` script (reserved for a separate chore) and the empty `KNOWN_FALSE_REJECTS` list (the new tests show those cases are allowed, so it stays empty). - **Test gate:** the mssql tests pass, with 113 pass and 0 fail. No build or lint scripts exist. ## Critical ## High ## Medium - [x] extensions/mssql/guard.test.ts:11 — The nested-comment entry in `KNOWN_GAPS` (`"comment: nested comment closes early if not nested-aware"`) is not a gap. The guard handles nested comments correctly, and the file's own comment says so. Listing it hides a corpus label whose expected behaviour differs from SQL Server's actual behaviour, and it weakens the "must-reject" corpus. Suggested fix: remove the entry from `corpus-reject.mjs`, or move it to `corpus-accept.mjs` if the SQL is valid. Then drop it from `KNOWN_GAPS`. - [ ] extensions/mssql/guard.ts:115 — `validateSql` is not wired into `mssql_query` in `index.ts`, and the guard is not exported or referenced anywhere. If that is intentional for this issue, there is no gap. Otherwise the guard has no effect. Suggested fix: confirm a follow-up issue covers the wiring. If it does not, call `validateSql` before executing in `index.ts`. ## Low - [x] extensions/mssql/guard.ts:109 — The `GO` batch-separator check is effectively redundant. Anything after `GO` must still start a statement and pass the keyword denylist, and `GO` lines containing `;` are already rejected. It also rejects a harmless line that starts with `GO`, such as an alias on its own line. Suggested fix: keep it as defence in depth, but add a test for `SELECT 1\nGO\nSELECT 2` to document the intent. - [ ] extensions/mssql/guard.ts:34 — `KNOWN_FALSE_REJECTS` is an empty array, and the comment above it lists examples (FOR XML, temp tables, unbracketed denied-keyword columns) that are not tested. Suggested fix: add accept or reject tests for `SELECT * FROM #tmp`, a column named `[into]`, and `FOR XML` so the documented behaviour is pinned. - [ ] package.json:31 — There is no lint or typecheck script, so the new `.ts` files (which use `!` non-null assertions and a `reason` union) are unchecked beyond Node's type stripping. Suggested fix: add a `tsc --noEmit` script in a separate chore.

david added 2 commits

2026-11-11 00:06:22 +00:00