Accept trailing comments in deployment migrations

CHANGELOG.md

@@ -29,6 +29,7 @@ Intermediate package/UI version bumps are grouped under the next published GitHu
 
 ### Fixed
 
+- Deployment accepts trailing SQL comments and checks every pending migration's statement terminator before database changes, preventing false semicolon errors and avoidable partial upgrades. [#173](https://github.com/apptrackit/finance/issues/173)
 - Cash and investment balance changes use decimal arithmetic to prevent floating-point residue while preserving genuine fractional amounts. Payments, transfers, adjustments, pending confirmations and recurring execution update balances and audit records together, with guards against duplicate or conflicting changes.
 - Dashboard net worth respects investment holding exclusions; Investments continues to show all active holdings.
 - Buttons, tabs, switches, menus, selects, checkboxes and other enabled click targets show a pointer cursor again across the app; Tailwind CSS 4 had reset buttons to the default arrow. Disabled controls keep the default cursor. [#157](https://github.com/apptrackit/finance/issues/157)

api/migrations/README.md

@@ -23,6 +23,7 @@ Example: `001-init.sql`, `002-add-user-table.sql`
 - Migrations already applied are skipped automatically
 - API/MCP-only deployments check migration history and stop if any are pending. Add `-- --migrations` to explicitly apply them before deploying that Worker.
 - A history-query failure stops deployment. Each migration and its history insert are submitted together; do not substitute Wrangler's separate built-in migration ledger.
+- The final SQL statement must end with a semicolon; trailing line/block comments are allowed. All pending files are checked for this terminator before any migration write. Retrying a stopped deployment skips migrations already recorded by their complete name.
 
 ## For Developers
 

scripts/deploy.mjs

@@ -140,9 +140,40 @@ export function d1Rows(output) {
   return result.flatMap(item => item.results)
 }
 
+function hasFinalSqlTerminator(sql) {
+  let lastToken
+  for (let index = 0; index < sql.length; index++) {
+    const char = sql[index]
+    if (/\s/.test(char)) continue
+    if (char === '-' && sql[index + 1] === '-') {
+      while (index < sql.length && !/[\r\n]/.test(sql[index])) index++
+      continue
+    }
+    if (char === '/' && sql[index + 1] === '*') {
+      const end = sql.indexOf('*/', index + 2)
+      if (end === -1) return false
+      index = end + 1
+      continue
+    }
+    // Comment markers and semicolons inside SQL strings/identifiers are data.
+    // SQLite escapes quotes by doubling them; bracket identifiers end at ].
+    if (['\'', '"', '`', '['].includes(char)) {
+      const closing = char === '[' ? ']' : char
+      for (index++; index < sql.length; index++) {
+        if (sql[index] !== closing) continue
+        if (closing !== ']' && sql[index + 1] === closing) index++
+        else break
+      }
+      if (index === sql.length) return false
+      lastToken = closing
+    } else lastToken = char
+  }
+  return lastToken === ';'
+}
+
 export function migrationSqlWithHistory(sql, name) {
   const migration = sql.trimEnd()
-  if (!migration.endsWith(';')) throw new Error(`Migration ${name} must end with a semicolon.`)
+  if (!hasFinalSqlTerminator(migration)) throw new Error(`Migration ${name} must end with a semicolon.`)
   // A second standalone semicolon is an empty statement rejected by D1's
   // remote file importer, even though the local executor accepts it.
   return `${migration}\nINSERT INTO migration_history (id, migration_name) VALUES ('${name}', '${name}');\n`
@@ -374,14 +405,19 @@ export async function deploy(options, { root = repositoryRoot, run = runCommand,
         throw new Error(`Pending migrations: ${pending.join(', ')}. Run npm run deploy:migrations or add -- --migrations to deploy:${options.target}.`)
       }
       if (plan.migrations && pending.length) {
-        await d1('Initializing migration history', ['--command', 'CREATE TABLE IF NOT EXISTS migration_history (id TEXT PRIMARY KEY, migration_name TEXT NOT NULL UNIQUE, executed_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP)'])
-        for (const file of pending) {
+        // Validate every pending payload before the first remote write, so a
+        // later file-format error cannot leave an avoidable partial upgrade.
+        const prepared = await Promise.all(pending.map(async file => {
           const name = file.slice(0, -4)
           const sql = await readFile(join(root, 'api/migrations', file), 'utf8')
+          return { file, name, sql: migrationSqlWithHistory(sql, name) }
+        }))
+        await d1('Initializing migration history', ['--command', 'CREATE TABLE IF NOT EXISTS migration_history (id TEXT PRIMARY KEY, migration_name TEXT NOT NULL UNIQUE, executed_at TEXT NOT NULL DEFAULT CURRENT_TIMESTAMP)'])
+        for (const { file, name, sql } of prepared) {
           const migrationFile = join(temporary, file)
           // Record success in the same SQL submission, rather than ignoring a
           // separate history-write failure and replaying ALTERs on the next run.
-          await writeFile(migrationFile, migrationSqlWithHistory(sql, name), { mode: 0o600 })
+          await writeFile(migrationFile, sql, { mode: 0o600 })
           // Wrangler prints file-import progress to stdout even with --json, so
           // its output cannot be parsed as a JSON query result. Verify the
           // history row with a separate read after the import exits successfully.

scripts/deploy.test.mjs

@@ -1,6 +1,6 @@
 import assert from 'node:assert/strict'
 import { spawnSync } from 'node:child_process'
-import { access, mkdir, mkdtemp, readFile, rm, stat, symlink, writeFile } from 'node:fs/promises'
+import { access, mkdir, mkdtemp, readFile, readdir, rm, stat, symlink, writeFile } from 'node:fs/promises'
 import { tmpdir } from 'node:os'
 import { dirname, join, resolve } from 'node:path'
 import { test } from 'node:test'
@@ -289,6 +289,62 @@ test('migration-only deployment initializes an empty database without building o
   await f.assertClean()
 })
 
+test('migration validation accepts trailing comments without changing SQL or adding empty statements', async () => {
+  const statements = [
+    "CREATE TABLE example (value TEXT DEFAULT '--not a comment;');",
+    'CREATE TABLE "example/*not a comment*/" ("a""--" TEXT);',
+    'CREATE TABLE `example``--` ([/*column*/] TEXT);',
+    "CREATE TRIGGER example AFTER DELETE ON entries BEGIN SELECT 'it''s; -- quoted'; END;",
+  ]
+  for (const statement of statements) {
+    for (const suffix of [' -- note', '\n-- note\r\n-- another', ' /* note */', '\n/* note\n; */ -- final']) {
+      const sql = statement + suffix
+      assert.equal(migrationSqlWithHistory(sql, 'test'), `${sql}\nINSERT INTO migration_history (id, migration_name) VALUES ('test', 'test');\n`)
+    }
+  }
+  for (const sql of [
+    'CREATE TABLE example (id TEXT) -- ;',
+    'CREATE TABLE example (id TEXT) /* ; */',
+    "SELECT ';' -- ;",
+    'SELECT "a;" /* ; */',
+    'SELECT `a;`',
+    'SELECT [a;]',
+    "SELECT 'unterminated;",
+    'SELECT 1; /* unterminated;',
+    '-- only comments;',
+    '/* only comments; */',
+  ]) assert.throws(() => migrationSqlWithHistory(sql, 'test'), /must end with a semicolon/)
+
+  const directory = resolve(dirname(script), '../api/migrations')
+  for (const file of (await readdir(directory)).filter(file => file.endsWith('.sql'))) {
+    const sql = await readFile(join(directory, file), 'utf8')
+    assert.ok(migrationSqlWithHistory(sql, file.slice(0, -4)).startsWith(sql.trimEnd() + '\nINSERT'))
+  }
+})
+
+test('an invalid later pending migration prevents all remote writes', async t => {
+  const f = await fixture(t, { applied: ['001-init'] })
+  await writeFile(join(f.root, 'api/migrations/003-invalid.sql'), 'SELECT 1 -- missing terminator;')
+  await assert.rejects(f.execute(['api', '--migrations']), /Migration 003-invalid must end with a semicolon/)
+  assert.equal(f.mutations().length, 0)
+  await f.assertClean()
+})
+
+test('retry after recorded investment links applies commented split migration without replaying links', async t => {
+  const f = await fixture(t, { applied: ['001-init', '002-change', '017-investment-transfer-links'] })
+  const directory = resolve(dirname(script), '../api/migrations')
+  for (const file of ['017-investment-transfer-links.sql', '017-transaction-splits.sql']) {
+    await writeFile(join(f.root, 'api/migrations', file), await readFile(join(directory, file), 'utf8'))
+  }
+  await f.execute(['migrations'])
+  assert.equal(f.sqlFiles.length, 1)
+  assert.match(f.sqlFiles[0], /INSERT INTO migration_history .*'017-transaction-splits'/)
+  assert.ok(f.sqlFiles[0].includes('-- keeps the shared fields in sync instead.\nINSERT INTO migration_history'))
+  await f.execute(['migrations'])
+  assert.equal(f.sqlFiles.length, 1, 'a second run must not replay either migration')
+  await f.assertClean()
+})
+
 test('full release preserves MCP preference and completes checks before any remote mutation', async t => {
   const f = await fixture(t, { config: { ...fixtureConfig, DEPLOY_MCP: 'true' }, applied: ['001-init'] })
   await f.execute([])