313: MSSQL spike: scaffold spike directory and must-reject corpus #335

Merged

david merged 2 commits from feature/issue-313/mssql-spike-scaffold-spike-directory-and-must-reje into main

2026-11-10 22:26:35 +00:00

david commented

2026-11-10 22:26:34 +00:00

Owner

313: MSSQL spike: scaffold spike directory and must-reject corpus

Implements 313.

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

Review findings

Line numbers in the findings differ from the file; I'll edit by content.# Review Findings — 313

Summary

I resolved all five findings in extensions/mssql/spike/corpus-reject.mjs.

Tests: I added corpus-reject.test.mjs, a node:test file that checks label and sql types and label uniqueness. Both tests pass when run directly. I did not add the file to the root npm test glob.
Policy header: I added a header comment recording the strict single-SELECT/CTE allow-list assumption.
Relabelled cases: I renamed the OUTPUT case and the unterminated-comment case.
New cases: I added INSERT ... EXEC, sp_OACreate, xp_dirtree, xp_regread and a SELECT ... WITH (UPDLOCK) case.

I left out SELECT ... FOR XML/JSON calling a write-capable function. I also did not add an INSERT nested in a derived table; the OUTPUT case was relabelled instead. The project has no build or lint script.

Critical

High

Medium

extensions/mssql/spike/corpus-reject.mjs:1 — The corpus has no structural check, so a malformed entry (missing sql, duplicate label, non-string) would only surface when the later guard spike consumes it. Suggested fix: add a small node:test file (e.g. corpus-reject.test.mjs) asserting each entry has a non-empty string label and a string sql, and that labels are unique. This also matches the follow-up in ci-coverage.md, which asks for node:test-compatible tests.
extensions/mssql/spike/corpus-reject.mjs:101 — The label says "subquery: write in derived table via OUTPUT", but the SQL is a plain INSERT ... OUTPUT, with no derived table or subquery. The label misdescribes the case, and the intended case, an INSERT nested in a derived table, is not covered. Suggested fix: rename the label to "output: INSERT with OUTPUT". If the case was meant to exist, add SELECT * FROM (INSERT ...)-style SQL, or INSERT ... OUTPUT inside INSERT ... EXEC.
extensions/mssql/spike/corpus-reject.mjs:73 — Several cases are only rejected by the guard if it applies a policy decision that the file doesn't record: SET NOCOUNT ON, DECLARE, BEGIN TRAN, USE, GO. A guard that allows harmless SET or DECLARE would "fail" the corpus. Suggested fix: add a header comment stating that the corpus assumes a strict single-SELECT/CTE-only allow-list. Alternatively, move the debatable cases into a separate "policy-dependent" group.

Low

extensions/mssql/spike/corpus-reject.mjs:1 — The corpus has no cases for SELECT ... FOR XML/JSON that call a write-capable function, for SELECT with locking hints (WITH (UPDLOCK)) or FOR UPDATE-style locking, or for INSERT ... EXEC. It also has no case for sp_OACreate, xp_dirtree or xp_regread. Suggested fix: add a few of these. UPDLOCK and HOLDLOCK hints are read-only in effect but can block other sessions, which is relevant to a read-only guard.
extensions/mssql/spike/corpus-reject.mjs:56 — The "unterminated block comment" case is labelled "swallowing nothing before write", which is hard to follow. Suggested fix: reword it to something like "write precedes an unterminated trailing block comment".

## 313: MSSQL spike: scaffold spike directory and must-reject corpus Implements [313](https://git.excelera.net/david/pi-extensions-and-skills/issues/313). > 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 12s. ## Review findings Line numbers in the findings differ from the file; I'll edit by content.# Review Findings — 313 ## Summary I resolved all five findings in `extensions/mssql/spike/corpus-reject.mjs`. - **Tests:** I added `corpus-reject.test.mjs`, a `node:test` file that checks label and `sql` types and label uniqueness. Both tests pass when run directly. I did not add the file to the root `npm test` glob. - **Policy header:** I added a header comment recording the strict single-`SELECT`/CTE allow-list assumption. - **Relabelled cases:** I renamed the OUTPUT case and the unterminated-comment case. - **New cases:** I added `INSERT ... EXEC`, `sp_OACreate`, `xp_dirtree`, `xp_regread` and a `SELECT ... WITH (UPDLOCK)` case. I left out `SELECT ... FOR XML/JSON` calling a write-capable function. I also did not add an `INSERT` nested in a derived table; the OUTPUT case was relabelled instead. The project has no build or lint script. ## Critical ## High ## Medium - [x] extensions/mssql/spike/corpus-reject.mjs:1 — The corpus has no structural check, so a malformed entry (missing `sql`, duplicate label, non-string) would only surface when the later guard spike consumes it. Suggested fix: add a small `node:test` file (e.g. `corpus-reject.test.mjs`) asserting each entry has a non-empty string `label` and a string `sql`, and that labels are unique. This also matches the follow-up in `ci-coverage.md`, which asks for `node:test`-compatible tests. - [x] extensions/mssql/spike/corpus-reject.mjs:101 — The label says "subquery: write in derived table via OUTPUT", but the SQL is a plain `INSERT ... OUTPUT`, with no derived table or subquery. The label misdescribes the case, and the intended case, an `INSERT` nested in a derived table, is not covered. Suggested fix: rename the label to "output: INSERT with OUTPUT". If the case was meant to exist, add `SELECT * FROM (INSERT ...)`-style SQL, or `INSERT ... OUTPUT` inside `INSERT ... EXEC`. - [x] extensions/mssql/spike/corpus-reject.mjs:73 — Several cases are only rejected by the guard if it applies a policy decision that the file doesn't record: `SET NOCOUNT ON`, `DECLARE`, `BEGIN TRAN`, `USE`, `GO`. A guard that allows harmless `SET` or `DECLARE` would "fail" the corpus. Suggested fix: add a header comment stating that the corpus assumes a strict single-`SELECT`/CTE-only allow-list. Alternatively, move the debatable cases into a separate "policy-dependent" group. ## Low - [x] extensions/mssql/spike/corpus-reject.mjs:1 — The corpus has no cases for `SELECT ... FOR XML/JSON` that call a write-capable function, for `SELECT` with locking hints (`WITH (UPDLOCK)`) or `FOR UPDATE`-style locking, or for `INSERT ... EXEC`. It also has no case for `sp_OACreate`, `xp_dirtree` or `xp_regread`. Suggested fix: add a few of these. `UPDLOCK` and `HOLDLOCK` hints are read-only in effect but can block other sessions, which is relevant to a read-only guard. - [x] extensions/mssql/spike/corpus-reject.mjs:56 — The "unterminated block comment" case is labelled "swallowing nothing before write", which is hard to follow. Suggested fix: reword it to something like "write precedes an unterminated trailing block comment".

david added 2 commits

2026-11-10 22:26:34 +00:00