314: MSSQL spike: must-accept corpus #336

Merged

david merged 2 commits from feature/issue-314/mssql-spike-must-accept-corpus into main

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

david commented

2026-11-10 22:28:33 +00:00

Owner

314: MSSQL spike: must-accept corpus

Implements 314.

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

Review findings

SELECT INTO is already in the reject corpus. Now the edits.The BSD sed failed on the package.json edit, so I'll make it with the Edit tool.# Review Findings — 314

Summary

I resolved all five findings. The spike tests are now in the root npm test glob. I added 11 accept-corpus entries: a trailing -- comment with no newline, nested comments, '' and ]] escapes, NOLOCK, FOR JSON/XML, CROSS/OUTER APPLY, PIVOT and OPTION (RECOMPILE). SELECT INTO was already in the reject corpus, so I didn't add it. The overlap check now normalises whitespace and case. I added a test that rejects data-modifying keywords outside comments and strings, and a comment noting the limits of the leading-keyword check. The spike tests pass 8 of 8. I did not run the full npm test, which also runs the penpot live-write stub. The repo has no build or lint script.

Critical

High

Medium

extensions/mssql/spike/corpus-accept.test.mjs:1 — The spike tests are not included in the root npm test glob, so they never run under npm test. Suggested fix: add extensions/mssql/spike/*.test.mjs to the node --test list in package.json, or document the manual command in the spike notes.
extensions/mssql/spike/corpus-accept.mjs:73 — The entry "comment: line comment at end" ends with \n, and the test only checks that the SQL is non-empty after trimming. The edge case is covered, but nothing asserts the guard handles a trailing -- comment with no newline, which is the harder case. Suggested fix: add a SELECT Id FROM dbo.Orders -- no trailing newline entry.
extensions/mssql/spike/corpus-accept.mjs:1 — The corpus has no coverage of several read-only constructs a guard is likely to misclassify. Examples are table hints such as WITH (NOLOCK), which starts with the keyword WITH mid-statement, and SELECT ... FOR JSON/XML. Others are CROSS APPLY/OUTER APPLY, PIVOT, OPTION (RECOMPILE), SELECT INTO (a write, so it belongs in the reject corpus), nested comments, an escaped '' inside a string, and ]] inside a bracketed identifier. Suggested fix: add entries for the read-only items, and confirm SELECT INTO is in the reject corpus.

Low

extensions/mssql/spike/corpus-accept.test.mjs:28 — The "startsWithSelectOrWith" check is a weak invariant. A statement like WITH x AS (...) DELETE ... would pass it, though the corpus is meant to hold only read-only statements. Suggested fix: also assert that no accepted entry contains a data-modifying keyword outside comments and strings, or note the limitation in a comment.
extensions/mssql/spike/corpus-accept.test.mjs:31 — The overlap check compares exact SQL strings only, so whitespace or case variants of a rejected statement would not be caught. Suggested fix: normalise (trim, collapse whitespace, lowercase) before comparing.

## 314: MSSQL spike: must-accept corpus Implements [314](https://git.excelera.net/david/pi-extensions-and-skills/issues/314). > 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 04s. ## Review findings SELECT INTO is already in the reject corpus. Now the edits.The BSD sed failed on the package.json edit, so I'll make it with the Edit tool.# Review Findings — 314 ## Summary I resolved all five findings. The spike tests are now in the root `npm test` glob. I added 11 accept-corpus entries: a trailing `--` comment with no newline, nested comments, `''` and `]]` escapes, `NOLOCK`, `FOR JSON/XML`, `CROSS/OUTER APPLY`, `PIVOT` and `OPTION (RECOMPILE)`. `SELECT INTO` was already in the reject corpus, so I didn't add it. The overlap check now normalises whitespace and case. I added a test that rejects data-modifying keywords outside comments and strings, and a comment noting the limits of the leading-keyword check. The spike tests pass 8 of 8. I did not run the full `npm test`, which also runs the penpot live-write stub. The repo has no build or lint script. ## Critical ## High ## Medium - [x] extensions/mssql/spike/corpus-accept.test.mjs:1 — The spike tests are not included in the root `npm test` glob, so they never run under `npm test`. Suggested fix: add `extensions/mssql/spike/*.test.mjs` to the `node --test` list in `package.json`, or document the manual command in the spike notes. - [x] extensions/mssql/spike/corpus-accept.mjs:73 — The entry "comment: line comment at end" ends with `\n`, and the test only checks that the SQL is non-empty after trimming. The edge case is covered, but nothing asserts the guard handles a trailing `--` comment with no newline, which is the harder case. Suggested fix: add a `SELECT Id FROM dbo.Orders -- no trailing newline` entry. - [x] extensions/mssql/spike/corpus-accept.mjs:1 — The corpus has no coverage of several read-only constructs a guard is likely to misclassify. Examples are table hints such as `WITH (NOLOCK)`, which starts with the keyword `WITH` mid-statement, and `SELECT ... FOR JSON/XML`. Others are `CROSS APPLY`/`OUTER APPLY`, `PIVOT`, `OPTION (RECOMPILE)`, `SELECT INTO` (a write, so it belongs in the reject corpus), nested comments, an escaped `''` inside a string, and `]]` inside a bracketed identifier. Suggested fix: add entries for the read-only items, and confirm `SELECT INTO` is in the reject corpus. ## Low - [x] extensions/mssql/spike/corpus-accept.test.mjs:28 — The "startsWithSelectOrWith" check is a weak invariant. A statement like `WITH x AS (...) DELETE ...` would pass it, though the corpus is meant to hold only read-only statements. Suggested fix: also assert that no accepted entry contains a data-modifying keyword outside comments and strings, or note the limitation in a comment. - [x] extensions/mssql/spike/corpus-accept.test.mjs:31 — The overlap check compares exact SQL strings only, so whitespace or case variants of a rejected statement would not be caught. Suggested fix: normalise (trim, collapse whitespace, lowercase) before comparing.

david added 2 commits

2026-11-10 22:28:33 +00:00