fix: the fifteen bugs found by the 2026-07-28 audit - #243
Merged
Conversation
SplitAtTopLevelKeyword resolves to `never` when the statement has no
FROM/USING clause, and `never extends { after: infer X extends string }`
passes with X inferred as `string`. ParseFromClause then built a source
whose table name was the `string` type, which ResolveKey matches against
every table in the schema - so every RETURNING column looked like it came
from two sources and strict mode reported `ambiguous column`.
Guard the check with the `[X] extends [never]` form already used
throughout the codebase, and make RETURNING/OUTPUT clause boundaries so
the UPDATE/DELETE WHERE text stops before the returned column list
instead of scanning it as part of the condition.
Fixes #229
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two shapes routed a `$` placeholder into the numbered bucket at slot 0: `$1::int` (a Postgres cast on a numbered placeholder) and `$id` (the SQLite named form the node:sqlite adapter already binds). Both made DigitsToCounter resolve to `never`, and `never extends [unknown, ...infer Position]` passes, so PlaceholderPosition returned a bogus position instead of falling through to the named branch. Two of them in one query intersected into `never` - a parameter tuple no argument can satisfy. Strip the cast in CleanScanToken so `$1::int` binds by index like the `$1` it is, and guard the counter with the `[X] extends [never]` form so a real name reaches the named branch. Fixes #228 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`AfterValues extends string` is distributive, so an INSERT with no VALUES clause made AfterKeyword resolve to `never` and collapsed the whole conditional to `never` - never reaching the unknown[] fallback the README documents. A rest parameter typed `never` rejects every call, including the zero-argument one, so INSERT ... SELECT could not be written at all. Fixes #230 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`$action` is a pseudo-column reporting which branch fired, and ResolveColumnType already types it as such. IsPlaceholder matched it anyway, so a MERGE with `output $action` demanded an extra argument, and UsedPlaceholderStyles reported the query as dollar-styled - which made every such MERGE fail the placeholder-style check against the mssql executor, the only dialect that has MERGE OUTPUT in the first place. Fixes #231 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ent break `select "id;x" from users` was rejected as two statements. Quoted identifiers are deliberately left intact by StripCommentsAndMaskLiterals - the parser needs the name - which makes their bodies the last place raw punctuation survives the masking pass. Two things read that punctuation as SQL: the stacked-statement guard, and RemoveSemicolons, which scrubbed every semicolon in the string rather than the trailing one and so mangled the identifier even once the guard passed. Blank quoted identifiers before the guard's scan, and drop only trailing semicolons in Normalize - a non-trailing one outside a quote is already an error before anything reads that text. Covers all three quoting styles the README documents. Fixes #232 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An UPDATE or DELETE without RETURNING projects no columns, and the empty-row short circuit returned before ApplyWhereCheck ever ran. So strict mode - documented as covering the SELECT list, the WHERE clause and JOIN ON - silently accepted a typo'd column in the WHERE of exactly the two statements where a wrong condition is most expensive. Wrap the empty row in the same clause check instead of returning early. Non-strict inference and writes with no WHERE clause are unaffected: both short-circuit inside ApplyWhereCheck/ApplyClauseError. Fixes #233 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ameters Both scanners walked past `--` and `/* */` comments as if they were SQL, so a `@word`, `$word` or `?` written inside one was bound as a real parameter. Binding is positional, so that shifted every value after the comment: the comment ate one, and the last real placeholder got null. Params<DB, Q> is computed from Normalize<Q>, which does strip comments - the two layers disagreed on how many placeholders a query has, and nothing at compile time could see it. Fold the two near-identical loops into one walker that skips everything the type layer masks (literals, dollar-quoted bodies, comments) plus quoted identifiers, whose bodies are names rather than SQL. Fixes #234 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
MssqlQueryable accepts a Request directly, and that Request is reused by every query() on the executor. node-mssql refuses `input()` for a name it has already declared and never clears the bag between calls, so a second query with the same @name threw, and one with different names silently carried the previous query's values along. Clear the parameters when the caller supplied the Request. A ConnectionPool or Transaction is unaffected: those get a fresh Request per call. The fake in the tests now mirrors node-mssql's actual input() contract - record the parameter, throw on a duplicate name - so a regression here fails the suite instead of passing against a permissive mock. Fixes #235 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
detectDialect routes `file:./app.db` to sqlite, but the normalizer only knew `file://`, so the prefix survived into existsSync and the CLI reported the database missing under a path nobody wrote. Fixes #236 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two writes fell outside the metadata the README promises. A statement led
by a WITH clause was classified by its first word, so `with x as (...)
insert into t ...` looked like a read and lost its row count even though
it correctly ran through run(). And a write with RETURNING goes down the
all() path, which returns rows only - so it reported no metadata at all.
Classify on the first keyword at paren depth zero instead of the first
word, and read changes()/last_insert_rowid() back off the connection for
the RETURNING case. That read is best-effort: metadata is the bonus and
the rows are the answer, so a connection that won't report the counters
costs the caller its meta, never its result set.
A row-returning write now resolves to { rows, meta } instead of a bare
array. createTypedDb already normalizes both shapes, so Result.value is
unchanged and Result.meta simply stops being empty.
Fixes #237
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
SQLITE_PARAM_PREFIXES has bound `:` since the adapter was written, but IsPlaceholder only knew `?`, `$` and `@`. So `where id = :id` typed as taking no parameters - passing the value was a compile error - and strict mode tried to resolve `:id` as a column and reported it unknown. A `::` cast cannot be mistaken for one: CleanScanToken strips the cast before the prefix test, so `id::text` reduces to `id`. Fixes #238 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`host\INSTANCE` is the default shape of a Windows install, and a backslash is a forbidden host character, so `new URL` rejected the whole string and the CLI died with a bare "Invalid URL" - no hint that the ADO form was the way through. Percent-encode the separator before parsing, split the instance back out into the driver's own instanceName option, and when the URL really is malformed say what a valid one looks like instead. The message carries no part of the URL, so a bad password cannot leak through it. Fixes #239 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--table users,ordrs` generated a schema containing only users and printed "Wrote ./schema.ts". The typo cost a table, and the only sign of it was an `unknown` row type much later. A --table name that matches nothing is now an error naming the miss, and the available-tables list it already printed for the match-nothing case is reused. An unmatched --exclude only warns on stderr: excluding a table that isn't there is still the outcome the caller asked for, and scripts do exclude tables that exist in some environments and not others. Fixes #240 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Boolean flags were set before the `=value` form was ever inspected, so `--check=false` silently turned the check on and exited 1 on drift - a red CI build with no explanation, doing the opposite of what was asked. Fixes #241 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The source scanner's alias group consumed the word after the table. When the first table had no alias that word was the next `join` keyword, so the regex cursor moved past it and the joined table was never scanned - every qualified reference to it got a false "unknown alias". Only the bare form broke: with `inner join` or an alias, the consumed word was something else and the `join` survived, which is exactly the shape the existing tests covered. Move the alias into a lookahead so it is never consumed, add the keywords that can legally follow a source (`using` above all, which was being read as the alias), scan comma-joined FROM lists, and let findSourceByAlias fall back to the table name the way the type-level FindSourceByName already does - so a source this heuristic misses means no diagnostic rather than a wrong one. Fixes #242 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three README statements the fixes made true or newly incomplete: strict mode now really does cover the WHERE of a write that returns nothing, node:sqlite takes a fourth placeholder prefix, and the CLI accepts a named instance in the URL form. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Jul 28, 2026
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes every issue opened by the 2026-07-28 audit: #228, #229, #230, #231, #232, #233, #234, #235, #236, #237, #238, #239, #240, #241, #242.
One commit per issue, in dependency order — #229 lands before #233 (once strict mode validates the WHERE of a write, the fabricated source would have made every unqualified column ambiguous), and #228 before #238. A single branch rather than a stack: #224 was marked merged while its content never reached master, and this is fifteen changes.
Every fix ships its regression test, and each test was checked to fail against the pre-fix code.
The three that shared a root cause
#228,#229and#230were all the same trap: a conditional type whose left side resolves tonever.SplitAtTopLevelKeywordisneverwhen there is no FROM/USING clause, soParseFromClausewas handed the typestringas a table name.ResolveKey<DB, string>matches every table, so everyRETURNINGcolumn looked like it came from two sources:delete from users where id = 1 returning idreturnedQueryTypeError<"ambiguous column: id">. Non-strict mode borrowed columns from unrelated tables instead of resolving them tounknown.DigitsToCounterisneverfor anything that isn't all digits, so$1::intand$idwere routed into the numbered bucket at slot 0. Two of them intersected intonever: a parameter tuple no argument can satisfy. Casts also now bind by index, which is whatCleanScanTokenstripping them buys.AfterValues extends stringdistributed overnever, so an INSERT with no VALUES clause collapsed the whole conditional instead of reaching the documentedunknown[].db.query('insert into users (id, name) select ...')could not be written at all — a rest parameter typedneverrejects even the zero-argument call.The guard used throughout the codebase is
[X] extends [never] ? ... : ...; these three were the places it was missing.Type layer
$actionis a MERGE pseudo-column, andResolveColumnTypealready typed it as such, butIsPlaceholdermatched any$…. So a MERGE withoutput $actiondemanded an extra argument and reported the query as dollar-styled, failing the placeholder-style check against the mssql executor — the only dialect that has MERGE OUTPUT at all.RemoveSemicolons, which scrubbed every semicolon rather than the trailing one and so mangled"id;x"even once the guard passed. Both fixed; all three quoting styles covered.SQLITE_PARAM_PREFIXEShas bound:since the adapter was written, but the type layer produced an empty tuple forwhere id = :idand strict mode reported:idas an unknown column.Runtime
--and/* */comments as if they were SQL. Binding is positional, so a@wordin a comment ate one value and the last real placeholder gotnull.Params<DB, Q>is computed from text that does strip comments, so nothing at compile time could see the disagreement. The two near-identical loops are now one walker.MssqlQueryableaccepts aRequest, node-mssql refusesinput()for a name it has already declared, and nothing clears the bag between calls. So a caller-supplied Request worked exactly once per parameter name. The test fake now mirrors node-mssql's realinput()contract instead of accepting everything.with) and lost its row count; a write with RETURNING goes downall()and reported no metadata at all. The RETURNING counters are read back off the connection best-effort: metadata is the bonus, the rows are the answer.CLI and editor plugin
file:./app.dbwas detected as SQLite but the prefix was never stripped, so the file was reported missing under a path nobody wrote.host\INSTANCEis the default Windows shape and a backslash is a forbidden host character, sonew URLrejected it outright. Now parsed into the driver'sinstanceName, and a genuinely malformed URL gets a message saying what a valid one looks like — carrying no part of the URL, so a password cannot leak through it.--table users,ordrsgenerated a schema without the typo'd table and printedWrote ./schema.ts. Now an error. An unmatched--excludeonly warns: excluding a table that isn't there is still the outcome asked for.--check=falseturned the check on and exited 1 on drift.joinkeyword, so the joined table was never scanned and every reference to it read as an unknown alias. Only the barea join bbroke — withinner joinor an alias the consumed word was something else — which is exactly the shape the existing tests covered.JOIN ... USINGand comma-joined FROM lists are handled too, andfindSourceByAliasnow falls back to the table name like the type-level resolver, so a source the heuristic misses means no diagnostic rather than a wrong one.Verification
$actionstrip and the quoted-identifier mask, both on paths every query touches. Under budget, so the ceiling is left where it is.🤖 Generated with Claude Code