[huyplb]

Summary

Migrating a trigger produced SQL that could not run. Two defects compounded; the migration generated from the seeded SQLite demo databases hit both.

1. A . in a comment qualified the next identifier

Qualification was decided by scanning backwards over whitespace for a .:

function identIsQualified(sql, identStart) {
  let k = identStart - 1;
  while (k >= 0 && /\s/.test(sql[k])) k--;
  return k >= 0 && sql[k] === '.';
}

A backward character scan cannot tell a . that is code from one that merely ends a comment. The generator writes review notes above the statements they concern, and one of them ends in a period:

-- review: skip FK … declare FOREIGN KEY in CREATE TABLE only (no ALTER ADD).
CREATE TRIGGER trg_decrement_stock …

That period made CREATE look like schema.CREATE, so onWord skipped it. stmtVerb never became CREATE, TRIGGER was never matched against ROUTINE_OBJECT, inRoutineDdl stayed false, and the body split at the ; inside BEGIN … END:

[10] CREATE TRIGGER … BEGIN UPDATE products SET … WHERE id = NEW.product_id;
[11] END -- ALTER TABLE: ORDERS ALTER TABLE orders ADD COLUMN notes TEXT;

-> incomplete input
-> near "ALTER": syntax error

Qualification now comes from the token stream, which already classifies comments as trivia, so it's correct by construction: a . that ends a comment never reaches it, while a . b and a /* c */ . b still read as qualified. Automated reduction took the trigger down to --. plus a trigger.

2. Trigger definitions were emitted without a terminator

Three sites pushed definition.trim(). SQLite's sqlite_master.sql ends a trigger at END with no ;, so the CREATE TRIGGER never ended and swallowed the following statement — which no splitter can fix, because the text is genuinely ambiguous. They now go through ensureSemicolon. The MySQL and Oracle createTriggerStatement hooks already end in ;, so this is a no-op for them, and it's idempotent besides.

Test plan

Verified end to end against the seeded demo databases, not only in unit tests:

=== trigger-related parts ===
   [10] "CREATE TRIGGER trg_decrement_stock AFTER INSERT ON order_items FOR EACH ROW BEGIN UPDATE products SE"

any part starting with END (the old symptom)? false

executed 16/20 OK
triggers on target after migration: trg_decrement_stock, trg_validate_qty

Previously the trigger split in two and neither half ran. Executed statements went from 14 to 16 of 20.

• npx vitest run — 6883 passed, 1 expected fail, 145 skipped (was 6874; +9 new tests)
• npx eslint . — exit 0
• npm run lint:security — exit 0
• npm run typecheck — exit 0
• bash scripts/seed/seed-all.sh sqlite, compare demo_a → demo_b, execute the generated migration against a copy of the target
• Live non-SQLite dialects — not run; Docker has no daemon in this environment

Regression coverage for the qualifier change

This touches a hot path in the splitter, so the new tests pin both directions — that a comment's . no longer qualifies, and that genuine qualifiers still do:

qualified .begin must not open a block
qualified .end must not close a block
qualified across whitespace  a . begin
qualified across comment  a /*c*/ . begin
real anonymous BEGIN block stays one
BEGIN TRANSACTION is not a block

Notes for the reviewer

• identIsQualified is deleted, not patched — the token-stream check replaces it entirely and it had no other caller.
• apps/web/src/frontend/shared/lib/sql-splitter.ts only re-exports this module, so the SQL editor's execute-all path is covered by the same fix.

Out of scope

The 4 remaining failures in that migration are pre-existing and unrelated — left alone:

• the generator emits SQLite's reserved internal sqlite_autoindex_* index names (×2)
• SQLite rejects non-constant ADD COLUMN defaults such as DEFAULT strftime(...) (×2)

Also still open from the same review: isQuotedIdentifier emits a quote-wrapped identifier verbatim, which allows DDL injection from a source schema's column names. That one needs a design decision and is not touched here.

---

---

Generated by Claude Code

---

Note

Medium Risk
Changes a hot path in splitSqlStatements (used by execute-all and migrations) and trigger DDL emission; behavior is well covered by new tests but touches statement-boundary logic.

Overview
Fixes two bugs that broke SQLite trigger migrations when review comments sat above the DDL and when catalog definitions omitted a trailing semicolon.

SQL splitter: Identifier qualification no longer scans backward for a . before the next word (which treated a period at the end of a -- review: comment as schema.CREATE). Qualification now follows the token stream via prevTokenWasDot, so CREATE TRIGGER … BEGIN … END stays one executable cell while real cases like s.begin / s . begin still count as qualified columns.

Migration generator: Raw trigger bodies from introspection (e.g. SQLite sqlite_master ending at END without ;) are emitted through ensureSemicolon at create-table, create-object, and alter-trigger paths so an unterminated trigger cannot absorb the following statement. Dialect hooks that already end in ; are unchanged.

New unit tests cover comment-period false qualification, genuine qualifiers, and trigger terminator idempotency.

^{Reviewed by Cursor Bugbot for commit 0476ac4. Bugbot is set up for automated code reviews on this repo. Configure here.}

[cursor]

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: 659bc208-f139-4d87-bb2e-33b5d1fe5879)