Repository navigation
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughFixes SQLite statement classification for ChangesSQL Parser RETURNING handling
Conditional compilation attribute reformatting
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
|
Updated 2:43 AM PT - May 22nd, 2026
✅ @robobun, your commit 9aeb6beb70e62d069872dde52ad75eba133428ea passed in 🧪 To try this PR locally: bunx bun-pr 30814That installs a local version of the PR into your bun-30814 --bun |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/sqlite.ts`:
- Around line 142-148: The leading WITH handling still sets canReturnRows = true
unconditionally, causing CTE-based INSERT/UPDATE/DELETE without RETURNING to be
treated as row-returning; update the post-loop logic that sets canReturnRows
(the code referencing canReturnRows and the leading "WITH" branch) to inspect
the statement following the WITH: only set canReturnRows to true if the
following top-level statement is a SELECT or any DML that includes a RETURNING
clause; for DML (INSERT/UPDATE/DELETE) without RETURNING leave canReturnRows
false so the code uses sqlite3_changes() instead of stmt.all().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7544d818-68ff-4162-bccb-c129d9bb5e85
📒 Files selected for processing (12)
src/crash_handler/lib.rssrc/errno/lib.rssrc/js/internal/sql/sqlite.tssrc/perf/tracy.rssrc/runtime/cli/Arguments.rssrc/runtime/cli/run_command.rssrc/runtime/cli/upgrade_command.rssrc/runtime/jsc_hooks.rssrc/runtime/webview/ChromeProcess.rssrc/spawn/process.rssrc/spawn_sys/spawn_process.rstest/js/sql/sqlite-sql.test.ts
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/sqlite.ts`:
- Around line 65-107: stripSQLComments currently only recognizes single and
double quotes; update the quoted handling in stripSQLComments to also support
backticks and bracket identifiers: extend the quoted variable type to include
"`" and "[" (e.g., false | "'" | '"' | "`" | "["), treat encountering "`" like
other quote characters (push into quoted state and support doubled-character
escape for backticks the same way as "'" and "\""), and treat "[" as an opening
bracket that must close on "]" (no doubled-bracket escaping). Update the
closing/escape logic inside the quoted branch to compute the closingChar =
quoted === "[" ? "]" : quoted and only apply the doubled-character skip when
closingChar equals quoted (i.e., for "'", '"' and "`"); ensure you still append
characters to out and handle index increments exactly as in the existing logic
so comments and tokens remain correctly separated.
In `@test/js/sql/sqlite-sql.test.ts`:
- Around line 1369-1371: The test uses a bound parameter (`const lit = await
sql<{ quoted: string }[]>`SELECT ${"hello -- world"} AS quoted``) so
stripSQLComments() never sees the `--`; change the test to use an actual inline
SQL literal (e.g. embed the text directly in the template or use sql.unsafe(...)
so the `--` appears in the SQL string) to exercise the quoted-literal parser
path, and add an additional case that uses a `/* ... */` literal to verify
block-comment handling; update references in the test to use the `lit` variable
and keep assertions on `lit[0].quoted` unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c1363283-9043-4cb3-acb4-223708269d35
📒 Files selected for processing (2)
src/js/internal/sql/sqlite.tstest/js/sql/sqlite-sql.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/internal/sql/sqlite.ts`:
- Around line 53-69: The reverse-token walk in parseSQLQuery that sets quoted
mode only for single and double quotes must also recognize backtick (`) and
bracket ([) quoting so identifiers like `[ RETURNING ]` or `` `RETURNING` `` are
preserved; update the logic in the reverse scan (the part that toggles
`quoted`/`quoteChar` and skips tokens) to treat '`' as an opening quote that
closes on '`' and '[' as an opening quote that closes on ']', mirroring
stripSQLComments behavior, and ensure tokenization does not expose inner words
like RETURNING; add a regression unit test such as `INSERT INTO t([ RETURNING ])
VALUES (1)` to cover this path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 65ce7d1f-88d2-4a75-b803-7005b6d3e1e2
📒 Files selected for processing (2)
src/js/internal/sql/sqlite.tstest/js/sql/sqlite-sql.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/js/internal/sql/sqlite.ts:291-301— Nit: the newsawDMLgate introduces a narrow regression forWITH ... SELECTqueries that reference a bracket- or backtick-quoted identifier containing a space-boundedINSERT/UPDATE/DELETEkeyword — e.g.WITH cte AS (SELECT [last update time] FROM t) SELECT * FROM cte. The reverse-walk tokenizer's quote handling at line 239 only recognizes'/"(not[or`), soUPDATEinside[last update time]is tokenized as a standalone keyword and setssawDML = true; the post-loopcase "WITH": if (!sawDML)then leavescanReturnRows = falseand the SELECT routes throughdb.run(), silently returning zero rows. Pre-PR this query returned rows correctly (both the mid-loopSELECTcase and the unconditional post-loopWITHwould have flippedcanReturnRows). Distinct from thestripSQLCommentsbracket issue flagged elsewhere — this is the reverse-walk's own quote handling, and fixing one does not fix the other.Extended reasoning...
What this is
Commit f2d1def added a
sawDMLflag toparseSQLQueryso that a leadingWITHfollowed byINSERT/UPDATE/DELETEroutes throughdb.run(). The flag is set whenever the reverse token walk sees a whitespace-boundedINSERT/UPDATE/DELETEtoken anywhere in the query (sqlite.ts:158-181), and the post-loopcase "WITH"now only flipscanReturnRowswhen!sawDML(sqlite.ts:291-301).The reverse walk's quote-skipping logic in the
defaultarm at sqlite.ts:239 only recognizes'and"as quote delimiters. SQLite also accepts[...](MS Access/SQL Server compat) and`...`(MySQL compat) as identifier quotes, and the tokenizer treats those characters as ordinary token text. So a bracket- or backtick-quoted identifier whose contents contain a DML keyword bounded by spaces on both sides — e.g.[last update time],[insert date here],`my delete log`— gets tokenized as separate words, and the innerUPDATE/INSERT/DELETEmatches the mid-loopcasearm and setssawDML = true.Step-by-step trace
Query:
WITH cte AS (SELECT [last update time] FROM t) SELECT * FROM ctestripSQLComments(query.toUpperCase())→ unchanged (no--or/*);.trim()→ unchanged. Leading token will beWITH.- Reverse walk from the end. The default arm at line 239 checks only
char === '"' || char === "'";[and]fall through totoken = char + token. - Tokens encountered (right-to-left, at each whitespace boundary):
CTE→ default;FROM→ default;*→ default;SELECT→ default (no longer flipscanReturnRowsafter this PR);T)→ default;FROM→ default;TIME]→ default;UPDATE→ matchescase "UPDATE":→sawDML = true;[LAST→ default;(SELECT→ default;AS→ default;CTE→ default. - Loop terminates at
i < 0withtoken = "WITH". - Post-loop
case "WITH": if (!sawDML) canReturnRows = true—sawDMListrue, socanReturnRowsstaysfalse. SQLiteQueryHandle.run()checksparsedInfo.canReturnRows === false→ takesdb.run(sql, ...)→ returns{ changes: 0, lastInsertRowid: 0 }for a SELECT →sqlResultis an emptySQLResultArraywithcount = 0. The rows are silently dropped.
For comparison, the same query with a double-quoted identifier (
"last update time") does not trigger this: the default arm enters thequotedstate on", suppressestoken = char + tokenfor the inner characters, and the inner spaces produce empty tokens that hit the whitespace-switchdefault—UPDATEis never seen as a standalone token.Why this is a regression
Before this PR, two safety nets existed for this query:
- The mid-loop whitespace switch had
case "SELECT":which setcanReturnRows = truefor the trailingSELECT * FROM ctetoken. - The post-loop
case "WITH":unconditionally setcanReturnRows = true.
Commit 783a287 removed the first; commit f2d1def gated the second on
!sawDML. The reverse walk's lack of[/`handling is pre-existing, but it was previously harmless forWITH ... SELECTbecause one of those two paths always flippedcanReturnRows. Now neither does, so this specific shape regresses from "returns rows" to "returns empty".Why nothing else catches it
stripSQLCommentsdoesn't help: there are no comment delimiters in the query, so it passes through verbatim.- The new
WITH ... SELECT still returns rowsassertion in the test suite (sqlite-sql.test.ts:1303) uses unquoted column names, so it doesn't exercise bracket identifiers. - This is distinct from the CodeRabbit-flagged
stripSQLCommentsbracket issue (which is about--//*inside[...]being mis-stripped). That bug is in a different function with a different failure mode (truncated query →RETURNINGlost). FixingstripSQLCommentsto skip[...]does not change the reverse-walk tokenizer at line 239, so both need the same fix applied independently.
Impact
A row-returning
WITH ... SELECTsilently returns an empty result instead of the rows — no error is thrown. The trigger is admittedly contrived (requires a bracket/backtick identifier containing a space-bounded DML keyword inside aWITH ... SELECT;[update_log]or[updated]would not trigger it), but column names like[Last Update Time]or[Insert Date]are plausible in schemas migrated from SQL Server or Access, which is exactly the population that uses[...]quoting in SQLite.Suggested fix
Extend the reverse-walk's default arm to also treat
`and[/]as quote delimiters (same change CodeRabbit suggested forstripSQLComments, applied here too):default: { if (char === '"' || char === "'" || char === "`") { if (quoted === char) quoted = false; else if (!quoted) quoted = char; continue; } if (char === "]") { if (!quoted) quoted = "["; continue; } if (char === "[") { if (quoted === "[") quoted = false; continue; } if (!quoted) token = char + token; }
(with
quoted's type widened tofalse | "'" | '"' | "" | "["). Alternatively, since the pre-PR behavior was correct for this case, restoring an unconditionalcanReturnRows = truesomewhere on theWITH ... ... SELECTpath would also work, but fixing the tokenizer's quote handling is the more principled fix and also hardens thesawDML` heuristic against other false positives.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/js/internal/sql/sqlite.ts (1)
142-142:⚠️ Potential issue | 🟠 Major | ⚡ Quick winQuote backtick and bracket identifiers in the reverse scan too.
stripSQLComments()now preserves`...`/[...], but Lines 142 and 248-259 still only enter quoted mode for'and". A valid identifier like[ RETURNING ]or` RETURNING `is therefore exposed as bareRETURNING, which flipscanReturnRows = trueand routes non-RETURNINGDML back throughstmt.all()instead ofdb.run(). Please mirror the backtick/bracket handling here and add a regression with a whitespace-delimited keyword inside the quoted identifier.Suggested fix
- let quoted: false | "'" | '"' = false; + let quoted: false | "'" | '"' | "`" | "]" = false; @@ default: { // skip quoted commands - if (char === '"' || char === "'") { - if (quoted === char) { - quoted = false; - } else { - quoted = char; - } + if (quoted) { + if ((quoted === "]" && char === "[") || quoted === char) { + quoted = false; + } + continue; + } + if (char === '"' || char === "'" || char === "`" || char === "]") { + quoted = char === "]" ? "]" : char; continue; } - if (!quoted) { - token = char + token; - } + token = char + token; }Also applies to: 248-259
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/internal/sql/sqlite.ts` at line 142, stripSQLComments now preserves backtick (`...`) and bracket ([...]) identifiers, but the reverse scan logic (variable quoted declared as let quoted: false | "'" | '"' = false and the quoted-mode branches in the reverse-scan loop around the code that sets canReturnRows) only recognizes single-quote and double-quote, so identifiers like `\` RETURNING \`` or `[ RETURNING ]` are treated as unquoted; update the reverse-scan logic in the same function (where quoted is declared and later in the reverse-scan loop) to also enter/exit quoted mode for backticks and bracket pairs (treat ` as a quote like '"' and treat '[' as opening and ']' as closing), ensure escaping/paired behavior mirrors the forward pass, and add a regression test that uses a whitespace-delimited keyword inside a quoted identifier (e.g., "` RETURNING `" or "[ RETURNING ]") to assert canReturnRows stays false and that DML without an actual RETURNING uses db.run().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@src/js/internal/sql/sqlite.ts`:
- Line 142: stripSQLComments now preserves backtick (`...`) and bracket ([...])
identifiers, but the reverse scan logic (variable quoted declared as let quoted:
false | "'" | '"' = false and the quoted-mode branches in the reverse-scan loop
around the code that sets canReturnRows) only recognizes single-quote and
double-quote, so identifiers like `\` RETURNING \`` or `[ RETURNING ]` are
treated as unquoted; update the reverse-scan logic in the same function (where
quoted is declared and later in the reverse-scan loop) to also enter/exit quoted
mode for backticks and bracket pairs (treat ` as a quote like '"' and treat '['
as opening and ']' as closing), ensure escaping/paired behavior mirrors the
forward pass, and add a regression test that uses a whitespace-delimited keyword
inside a quoted identifier (e.g., "` RETURNING `" or "[ RETURNING ]") to assert
canReturnRows stays false and that DML without an actual RETURNING uses
db.run().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0232149-f186-45f2-b88c-ce7f0caf9b2b
📒 Files selected for processing (2)
src/js/internal/sql/sqlite.tstest/js/sql/sqlite-sql.test.ts
e0c2f57 to
550a4be
Compare
| // `i` points at the whitespace immediately AFTER `REPLACE` | ||
| // in source order; peek forward past any further whitespace | ||
| // for an `INTO` token to disambiguate. | ||
| let peek = i + "REPLACE".length + 1; |
There was a problem hiding this comment.
🟡 Nit (comment only — code is correct): this says i points at the whitespace "immediately AFTER REPLACE in source order", but the reverse walk fires this case when i is at the whitespace immediately before REPLACE in source order — i+1..i+7 hold REPLACE itself, which is exactly why peek = i + "REPLACE".length + 1 lands on the first char after it. The arithmetic is right; only the comment is inverted, which could mislead a future maintainer into "fixing" the +8 offset.
Extended reasoning...
What this is
The new case "REPLACE": arm in the mid-loop whitespace switch carries a comment explaining the forward-peek arithmetic:
ipoints at the whitespace immediately AFTERREPLACEin source order; peek forward past any further whitespace for anINTOtoken to disambiguate.
This description of i is backwards. The code that follows it (peek = i + "REPLACE".length + 1) is correct; only the comment text is wrong.
Why the comment is wrong
parseSQLQuery walks the (uppercased, comment-stripped) query right-to-left:
for (let i = text_len - 1; i >= 0; i--) {
const char = text[i];
switch (char) {
case " ": case "\n": ... { // whitespace → flush `token`
switch (token) {
case "REPLACE": { ... }The default arm builds token by prepending: token = char + token. So when the whitespace case fires with token === "REPLACE", the seven characters R E P L A C E were accumulated from indices i+1, i+2, …, i+7, and text[i] is the whitespace character to the left of the R. In left-to-right source order, that whitespace is immediately before REPLACE, not after it.
The comment explicitly says "in source order", which rules out a "after, in reverse-walk order" reading — it is unambiguously inverted.
Step-by-step proof
Take text = "X REPLACE INTO":
| index | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | 8 | 9 | 10 | 11 | 12 | 13 |
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| char | X | ␠ | R | E | P | L | A | C | E | ␠ | I | N | T | O |
Reverse walk:
i=13..10accumulatetoken="INTO";i=9(space) → flush,lastToken="INTO",token="".i=8..2accumulatetoken="REPLACE".i=1(space) → whitespace case fires withtoken === "REPLACE".
At this point i = 1, which is the space before REPLACE in source order. Then:
peek = i + "REPLACE".length + 1 = 1 + 7 + 1 = 9
text[9] is the space after the trailing E; the inner while skips it to peek = 10, and text[10..13] === "INTO" → sawDML = true. ✅
If i were actually "the whitespace immediately AFTER REPLACE" as the comment claims (i.e. i = 9), then peek = 9 + 7 + 1 = 17, which is past the end of the string — the INTO check would never match. The arithmetic only works because i is before REPLACE.
Why nothing else catches it
This is purely a documentation error. The runtime behaviour is correct and is covered by the new REPLACE as scalar function inside WITH ... SELECT and WITH ... REPLACE INTO regression tests in test/regression/issue/30811.test.ts. No test can catch a misleading comment.
Impact
No runtime impact. The risk is maintainer confusion: someone reading "i is after REPLACE" and then seeing peek = i + 8 would reasonably conclude the offset should be i + 1 (one past the trailing whitespace) and "fix" it, breaking the REPLACE INTO disambiguation.
Suggested fix
Reword the comment to match the code, e.g.:
// `i` points at the whitespace immediately BEFORE `REPLACE` in
// source order (`i+1..i+7` is `REPLACE` itself); peek forward from
// `i+8` past any further whitespace for an `INTO` token to
// disambiguate.On the refutations
Both refutations flag this as a duplicate of the other (bug_001 ↔ bug_005), not as a false positive — they agree the comment is wrong. The two reports have been merged into this single comment, so there is no duplication concern.
|
CI infra noise only. Diff is green on every lane that ran my test file:
All three are infra/agent hiccups on Darwin-aarch64, Alpine, and Windows lanes that my JS-only diff cannot cause. The umbrella Ready for a maintainer to merge. |
…subquery SELECT `parseSQLQuery` in the SQLite adapter walked the SQL token-by-token in reverse and flipped `canReturnRows = true` whenever it saw `SELECT`, `PRAGMA`, `WITH`, or `EXPLAIN` — regardless of whether the keyword was the leading statement token or a subquery inside a DML statement. For `INSERT ... SELECT` and related shapes (`UPDATE ... WHERE id IN (SELECT ...)`, `WITH ... INSERT/UPDATE/DELETE/REPLACE`, `/* … */ SELECT ...`) without a `RETURNING` clause, the handle then routed through `stmt.all()`, which returned an empty array — so `result.count` came back as `0` and `lastInsertRowid` / `affectedRows` as `null`, even though SQLite actually mutated rows (and `changes()` reported the correct number). The fix rewrites the classifier: - `SELECT` / `PRAGMA` / `WITH` / `EXPLAIN` only flip `canReturnRows` when they appear as the leading statement keyword (handled in the post-loop block), not when seen mid-query as part of a subquery. `RETURNING` keeps flipping it everywhere. - A new `stripSQLComments` pre-pass removes `/* … */` and `-- …` runs so a query tagged with a leading comment (sqlcommenter / APM tagging) still has its leading statement keyword visible to the walker. The stripper preserves quoted regions for all four SQLite identifier styles (`'`, `"`, \`, `[...]`). - A `sawDML` flag tracks INSERT/UPDATE/DELETE/REPLACE tokens across the walk so a leading `WITH` only flips `canReturnRows` when the CTE-prefixed statement is actually a SELECT. - `REPLACE` is overloaded in SQLite (statement keyword vs. the `replace(X,Y,Z)` scalar function), so the REPLACE arm peeks forward for an `INTO` token to disambiguate — otherwise `SELECT REPLACE (name, 'a', 'b')` (with whitespace before `(`) would falsely flip `sawDML`. Fixes #30811
550a4be to
9aeb6be
Compare
| // `WITH cte AS (...) INSERT/UPDATE/DELETE ...` (does not, unless | ||
| // RETURNING was seen). Only RETURNING flips `canReturnRows` mid-loop. | ||
| let sawDML = false; | ||
| let quoted: false | "'" | '"' = false; |
There was a problem hiding this comment.
🟡 Nit: CodeRabbit comment 3248701495 ("handle backtick/bracket quoting in the reverse token walk") is marked ✅ addressed in 1e32b07..1b2bc34, but only stripSQLComments was extended — the reverse walk's quoted state here is still false | "'" | '"' and the default arm only checks '/". Combined with the new sawDML gate, a bracket/backtick identifier with internal whitespace like [ DELETE ] inside a WITH … SELECT now flushes a bare DELETE token, sets sawDML, and suppresses canReturnRows (pre-PR the mid-loop SELECT case covered this). Vanishingly rare trigger, just flagging since the resolved comment implies this was fixed when it wasn't.
Extended reasoning...
What this is
CodeRabbit inline comment 3248701495 asked for the reverse token walk in parseSQLQuery to recognise `...` and [...] identifiers (mirroring the change made to stripSQLComments), and the timeline marks it "✅ Addressed in commits 1e32b07 to 1b2bc34". However in the final code:
stripSQLComments(sqlite.ts:69) declaresquoted: false | "'" | '"' | "\" | "["` — it was extended.- The reverse walk (sqlite.ts:142) still declares
let quoted: false | "'" | '"' = false;, and the default arm at ~288–297 only checkschar === '"' || char === "'". Backtick and bracket characters fall through totoken = char + tokenlike any other char, and the outer whitespace case has noquotedguard.
So the resolved comment's claimed fix was applied to one function but not the other.
Why it matters now (and didn't before this PR)
On main, the mid-loop whitespace switch had case "SELECT": canReturnRows = true. This PR removes that case (the #30811 fix) and instead uses the new sawDML flag to decide whether a leading WITH returns rows: case "WITH": if (!sawDML) canReturnRows = true. That makes the reverse walk's tokenisation of identifier contents load-bearing in a way it wasn't before — a spurious DELETE/INSERT/UPDATE token now actively suppresses row-returning behaviour for WITH … SELECT.
Step-by-step trace
Query: WITH cte AS (SELECT [ DELETE ] AS col FROM t) SELECT * FROM cte (SQLite accepts [ DELETE ] as an identifier; brackets may contain spaces).
After stripSQLComments(...).toUpperCase().trim() the brackets are preserved verbatim (the new prepass treats [...] as quoted). The reverse walk:
]→ default arm; not'/",!quoted→token = "]".- space → whitespace case flushes
token = "]"via the default arm;token = "". E,T,E,L,E,D→ default arm accumulatestoken = "DELETE".- space → whitespace case flushes
token = "DELETE"→ matches the new mid-loopcase "DELETE":→sawDML = true. - Walk continues through
[,(SELECT,AS,CTE; the trailing-SELECTand embedded-SELECTtokens hit the default arm (deliberately — the Bun.SQL SQLite returns count 0 for INSERT ... SELECT without RETURNING #30811 fix). - Loop exits with
token = "WITH". - Post-loop
case "WITH": if (!sawDML) canReturnRows = true—sawDMLis true, socanReturnRowsstaysfalse. SQLiteQueryHandle.run()takes thedb.run()branch → SELECT executes but rows are discarded; result is an emptySQLResultArraywithcount = 0.
Pre-PR, step 5's mid-loop SELECT case would have set canReturnRows = true regardless, so this query returned its rows. So this is technically a regression on valid SQLite, introduced by the interaction of the new sawDML mechanism with the pre-existing reverse-walk gap.
The same applies to ` UPDATE `, [ INSERT ], or [ RETURNING ] (the original CodeRabbit example) — any DML keyword padded with whitespace inside a backtick/bracket identifier inside a WITH … SELECT.
On the refutation
The refutation argues this is implausible because the trigger requires an identifier whose actual name is e.g. ' DELETE ' — a DML keyword padded with literal spaces — which no human, ORM, or query builder produces. That's fair, and is exactly why this is a nit, not a normal-severity bug. The reason it's still worth a comment rather than dropping entirely:
- The PR timeline marks CodeRabbit 3248701495 as "✅ Addressed" when it demonstrably wasn't (only
stripSQLCommentsgot the backtick/bracket handling). Worth knowing whether the reverse-walk half of that fix was intentionally deferred or accidentally dropped between 1e32b07 and 1b2bc34. - It IS a behavioural regression on valid SQL (however pathological) — pre-PR the embedded
SELECTmasked this gap; the newsawDMLmechanism exposes it.
This is not a merge blocker; parseSQLQuery is an acknowledged heuristic and the trigger has effectively zero real-world surface area.
Suggested fix
Mirror stripSQLComments's delimiter set in the reverse walk (CodeRabbit's suggested diff is already correct for a right-to-left scan — enter quoted mode on ] and exit on [):
let quoted: false | "'" | '"' | "\`" | "]" = false;
...
default: {
if (quoted) {
if ((quoted === "]" && char === "[") || quoted === char) quoted = false;
continue;
}
if (char === '"' || char === "'" || char === "\`" || char === "]") {
quoted = char;
continue;
}
token = char + token;
}Or simply note in the resolved comment that the reverse-walk half is intentionally left as-is given the trigger's rarity.
🔬 also observed by coderabbitai
| // Regression tests for oven-sh/bun#30811. | ||
| // | ||
| // `Bun.SQL` SQLite's `parseSQLQuery` classifier reverse-walked tokens and | ||
| // flipped `canReturnRows = true` whenever it saw a `SELECT` / `PRAGMA` / | ||
| // `WITH` / `EXPLAIN` token — so `INSERT … SELECT` (without `RETURNING`), | ||
| // `WITH … INSERT/UPDATE/DELETE/REPLACE`, and queries whose leading token | ||
| // is hidden by a `/* … */` or `-- …` comment all routed through the | ||
| // `stmt.all()` branch and reported `count: 0` / `lastInsertRowid: null` | ||
| // for mutations that actually affected rows. | ||
| import { SQL } from "bun"; | ||
| import { describe, expect, test } from "bun:test"; | ||
|
|
There was a problem hiding this comment.
🟡 Nit: per CLAUDE.md / test/CLAUDE.md, test/regression/issue/${issueNumber}.test.ts is reserved for true regressions (worked in a prior release, then broke) — bugs that were never correct go in the module's existing test file. The PR's own Cause section describes parseSQLQuery's original design (not a later breakage), and the description even says "Four tests added in test/js/sql/sqlite-sql.test.ts". Suggest moving these into test/js/sql/sqlite-sql.test.ts alongside the existing INSERT/UPDATE/DELETE row-count tests.
Extended reasoning...
What the convention says
Both root CLAUDE.md:66 and test/CLAUDE.md:153 state verbatim:
test/regression/issue/${issueNumber}.test.tsis only for bugs that have a GitHub issue number and are true regressions (worked in a previous release, then broke). An issue number alone does not qualify — if it was never correct, put the test in the module's existing test file instead.
So having a GitHub issue (#30811) is necessary but not sufficient for placing a test under test/regression/issue/ — the bug must also have been a regression in the strict sense (a previously-working release broke).
Why this PR doesn't appear to qualify
The PR's own Cause section describes the original implementation of parseSQLQuery:
parseSQLQueryinsrc/js/internal/sql/sqlite.tswalks the SQL token-by-token in reverse and flipscanReturnRows = truethe first time it seesSELECT,PRAGMA,WITH, orEXPLAIN.
There's no reference to a prior commit or release that handled INSERT … SELECT correctly and then broke — the reverse-walk classifier appears to have had this gap from the start. That makes #30811 a never-worked bug rather than a true regression, which per the convention belongs in the module's own test file.
Description / diff mismatch
The PR description's Verification section explicitly says:
Four tests added in
test/js/sql/sqlite-sql.test.tsunder SQLite-specific features
…and references running them "alongside the existing INSERT with RETURNING, UPDATE with affected rows, DELETE with affected rows" tests in that file. But the actual diff creates test/regression/issue/30811.test.ts instead. So the author seemingly targeted the convention-correct location originally and the diff drifted (or the description was never updated). Either way, the description and diff disagree on where the tests live.
Step-by-step
test/CLAUDE.md:153reservestest/regression/issue/for true regressions and explicitly says "an issue number alone does not qualify".- PR Cause section describes
parseSQLQuery's original token-walk design; nothing indicates it ever worked. - → Bun.SQL SQLite returns count 0 for INSERT ... SELECT without RETURNING #30811 is a never-correct bug, not a regression.
- → Convention says: put the tests "in the module's existing test file instead" — here,
test/js/sql/sqlite-sql.test.ts. - PR description already names
test/js/sql/sqlite-sql.test.tsas the target, confirming that's the natural home. - Diff creates
test/regression/issue/30811.test.ts→ convention violation + description/diff mismatch.
Impact
Zero runtime impact — this is test-file placement only. Filed as a nit: it's an explicit, documented repo convention (in two CLAUDE.md files) and the PR description itself contradicts the diff, but it doesn't affect correctness or CI.
Suggested fix
Move the four describe("#30811: …") tests into test/js/sql/sqlite-sql.test.ts (e.g. under the SQLite-specific features block referenced in the description, near the existing INSERT with RETURNING / UPDATE with affected rows / DELETE with affected rows tests), and drop test/regression/issue/30811.test.ts. If #30811 actually does document a specific prior release where INSERT … SELECT reported the correct count, then the current location is fine and the PR description's Cause/Verification sections should be updated instead.
| peek + 4 <= text_len && | ||
| text[peek] === "I" && | ||
| text[peek + 1] === "N" && | ||
| text[peek + 2] === "T" && | ||
| text[peek + 3] === "O" && | ||
| (peek + 4 === text_len || | ||
| text[peek + 4] === " " || | ||
| text[peek + 4] === "\t" || | ||
| text[peek + 4] === "\n" || | ||
| text[peek + 4] === "\r" || | ||
| text[peek + 4] === "\f" || | ||
| text[peek + 4] === "\v") | ||
| ) { |
There was a problem hiding this comment.
🟡 Nit: the INTO boundary check at text[peek + 4] only accepts whitespace or end-of-string, but SQLite also accepts a quoted identifier immediately after INTO with no space — REPLACE INTO"dst", REPLACE INTO`dst` , and REPLACE INTO[dst] are all valid. In those forms text[peek+4] is " / ` / [, the check fails, sawDML stays false, and WITH cte AS (...) REPLACE INTO"dst" SELECT ... still routes through stmt.all() → count = 0. Not a regression (pre-PR behaved the same), just a small completeness gap in the new peek — adding " / ` / [ to the peek + 4 boundary set would close it.
Extended reasoning...
What this is
The new mid-loop case "REPLACE": arm peeks forward from the whitespace before REPLACE, skips further whitespace, then checks for the four characters I N T O followed by a token boundary at text[peek + 4]. The boundary set is peek + 4 === text_len or one of the six whitespace characters ( , \t, \n, \r, \f, \v).
SQLite's tokenizer, however, also starts a new token when it sees an identifier-quote character — ", `, or [ — immediately after a keyword, with no intervening whitespace. So all of these are valid SQLite and parse as REPLACE + INTO + quoted-identifier:
REPLACE INTO"dst" SELECT ...
REPLACE INTO`dst` SELECT ...
REPLACE INTO[dst] SELECT ...When INTO is written that way, text[peek + 4] is " / ` / [, which is not in the whitespace boundary set, so the INTO check fails and sawDML is not set.
Step-by-step trace
Query: WITH cte AS (SELECT id, name FROM src) REPLACE INTO"dst" SELECT id, name FROM cte
After stripSQLComments(...).toUpperCase().trim() the string is unchanged (no comments). The reverse walk:
- Walking right-to-left, the default arm's quote handling skips the contents of
"DST"(the"chars togglequotedon/off, and theD/S/Tbetween them are dropped because!quotedis false). The token accumulated immediately to the left of the first"isINTO, which flushes via the default arm at the next space. - Next the loop accumulates
REPLACE(indicesi+1..i+7) and ati= the space between)andRit enterscase "REPLACE":. peek = i + 8lands on theIofINTO. The whitespace-skipwhileis a no-op (there is exactly one space, already consumed by the+1in the offset).text[peek..peek+3] === "INTO"✅.text[peek + 4]is"— not whitespace, andpeek + 4 !== text_len. The boundary check fails →sawDMLstaysfalse.- None of the remaining tokens (
SRC),FROM,NAME,ID,,(SELECT,AS,CTE) matchINSERT/UPDATE/DELETE, sosawDMLis never set. - The loop exits with
token = "WITH". Post-loopcase "WITH": if (!sawDML) canReturnRows = truefires. SQLiteQueryHandle.run()takes thestmt.all()branch →[]→result.count = 0,lastInsertRowidunset — exactly the Bun.SQL SQLite returns count 0 for INSERT ... SELECT without RETURNING #30811 symptom for a shape this PR'sREPLACEhandling is meant to cover.
The same trace applies with ` or [ in place of ".
Why nothing else catches it
stripSQLComments preserves quoted identifiers verbatim (including the " / ` / [ delimiters), so the boundary character reaches the reverse walk unchanged. The reverse walk's own quote handling correctly skips the contents of "dst", but that only affects token accumulation — the forward INTO peek is a separate character-by-character check against text, and its boundary set is hard-coded to whitespace + EOS. The new WITH ... REPLACE INTO regression test in test/regression/issue/30811.test.ts writes REPLACE INTO dst with a space and an unquoted identifier, so it doesn't exercise this shape.
Not a regression
Before this PR, the mid-loop SELECT (inside the CTE body) and/or the leading WITH would have flipped canReturnRows = true anyway, so WITH ... REPLACE INTO"dst" SELECT ... already misrouted to stmt.all() on main. This is purely a completeness gap in the brand-new REPLACE-INTO peek added in this PR.
Scope / impact
Very narrow trigger: requires (1) a leading WITH, (2) REPLACE INTO (rather than INSERT/UPDATE/DELETE, which don't use the peek), (3) a quoted table name, (4) no space between INTO and the opening quote, and (5) no RETURNING. Writing INTO"tbl" with no space is unusual hand-written style and not something ORMs typically emit, so this is filed as a nit rather than a blocker. parseSQLQuery is an explicitly heuristic classifier with other known edge cases.
Suggested fix
Add the three identifier-quote openers to the peek + 4 boundary set:
(peek + 4 === text_len ||
text[peek + 4] === " " ||
text[peek + 4] === "\t" ||
text[peek + 4] === "\n" ||
text[peek + 4] === "\r" ||
text[peek + 4] === "\f" ||
text[peek + 4] === "\v" ||
text[peek + 4] === "\"" ||
text[peek + 4] === "`" ||
text[peek + 4] === "[")(INTO cannot be followed by ( or ' in the REPLACE INTO grammar, so those aren't needed.)
|
Superseded by #33583, which replaces the row-returning heuristic with the prepared statement's column count instead of extending the tokenizer. The extra shapes covered by this PR's tests (WITH ... UPDATE/DELETE/REPLACE INTO, subquery UPDATE/DELETE, comment-prefixed writes, comment markers inside literals, REPLACE as a function) are now covered in test/js/sql/sqlite-sql-row-detection.test.ts over there. |
1 participant
Fixes #30811.
Repro
Same shape fails for
UPDATE ... WHERE id IN (SELECT ...)andDELETE ... WHERE id IN (SELECT ...)without aRETURNINGclause.Cause
parseSQLQueryinsrc/js/internal/sql/sqlite.tswalks the SQL token-by-token in reverse and flipscanReturnRows = truethe first time it seesSELECT,PRAGMA,WITH, orEXPLAIN. The SQLite adapter then usescanReturnRowsto pick between two branches:true→db.prepare(sql).all(...)— results come from the prepared statement's row array.false→db.run(sql, ...)— results come fromchanges.changes/changes.lastInsertRowid.For
INSERT ... SELECTwithoutRETURNING, the mid-querySELECTflipped the flag, so the handle ranstmt.all()which returns an empty array —sqlResult.countbecameresult.length === 0andlastInsertRowid/affectedRowswere never populated.Fix
Drop
SELECT/PRAGMA/WITH/EXPLAINfrom the mid-loop whitespace case. They still flipcanReturnRowsin the post-loop block when they are the leading statement keyword (soSELECT * FROM t,PRAGMA ...,WITH cte AS (...) SELECT ..., andEXPLAIN ...still return rows viastmt.all()).RETURNINGkeeps flippingcanReturnRowseverywhere, soINSERT ... SELECT ... RETURNING *still works.Verification
Four tests added in
test/js/sql/sqlite-sql.test.tsunder SQLite-specific features:INSERT ... SELECTwithoutRETURNING—count= rows inserted,lastInsertRowidset, rows actually in the table.INSERT ... SELECT ... RETURNING—count= rows inserted, result contains the returned rows.UPDATE ... WHERE id IN (SELECT ...)withoutRETURNING—count= rows updated.DELETE ... WHERE id IN (SELECT ...)withoutRETURNING—count= rows deleted.Gate:
expect(result.count).toBe(2)receives0.INSERT with RETURNING,UPDATE with affected rows,DELETE with affected rows, andINSERT containing literal 'RETURNING'tests.sqlite-sql.test.ts: 232/235 pass. The three failures (handles exotic but valid SQL patterns,handles special characters in database paths,properly finalizes prepared statements) are pre-existing debug-build timeouts and reproduce on unmodifiedmain.