fix: detect and re-run migration 029 when forms tables are missing

docs/ai/plans/fix-762-migration-029-reuse-plan.md

@@ -0,0 +1,42 @@
+# Fix #762: Migration ID 029 Reused Across Versions
+
+## Overview
+Migration ID `029` was reassigned from "Ai Search Plugin" to "Add Forms System" between SonicJS releases. Users upgrading from older versions have `029` marked as applied (for AI Search), so the forms migration is skipped, causing cascading failures for migrations `030` and `033` which depend on the `forms` and `form_submissions` tables.
+
+## Requirements
+- [x] Add auto-detection for migration 029 (forms tables) in `autoDetectAppliedMigrations()`
+- [x] If 029 is marked as applied but `forms` table doesn't exist, remove it so it re-runs (same pattern as migration 011)
+- [x] Improve error messaging in admin API to include per-migration failure details
+- [x] Add unit tests for the new detection logic
+
+## Technical Approach
+
+### Architecture
+This follows the existing pattern used for migration `011` (managed column detection), which already handles the case where a migration is marked as applied but its artifacts don't exist. We extend this pattern to migration `029`.
+
+### File Changes
+| File | Action | Description |
+|------|--------|-------------|
+| `packages/core/src/services/migrations.ts` | Modify | Add 029 auto-detection with existence check (like 011 pattern) |
+| `packages/core/src/routes/admin-api.ts` | Modify | Include per-migration error details in response |
+| `packages/core/src/services/migrations.test.ts` | Create/Modify | Unit tests for new detection logic |
+
+### Database Changes
+None — this only changes detection logic, not schema.
+
+## Implementation Steps
+1. Add forms table auto-detection for migration 029 in `autoDetectAppliedMigrations()` using the same bidirectional pattern as migration 011
+2. Improve admin API error response to include specific migration failure details
+3. Write unit tests
+4. Run existing tests to verify no regressions
+
+## Testing Strategy
+
+### Unit Tests
+- Migration 029 marked as applied + forms table exists → keep as applied
+- Migration 029 marked as applied + forms table missing → remove, mark as pending
+- Migration 029 not applied + forms table exists → mark as applied
+- Migration 029 not applied + forms table missing → leave as pending
+
+## Approval
+- [ ] Plan reviewed and approved by user

packages/core/src/routes/admin-api.ts

@@ -719,13 +719,16 @@ adminApiRoutes.post('/migrations/run', async (c) => {
     return c.json({
       success: result.success,
       message: result.message,
-      applied: result.applied
+      applied: result.applied,
+      errors: result.errors
     })
   } catch (error) {
     console.error('Error running migrations:', error)
+    const errorMessage = error instanceof Error ? error.message : String(error)
     return c.json({
       success: false,
-      error: 'Failed to run migrations'
+      error: `Failed to run migrations: ${errorMessage}`,
+      errors: [errorMessage]
     }, 500)
   }
 })

packages/core/src/services/migrations.test.ts

@@ -0,0 +1,241 @@
+/**
+ * Migration Service Tests - Auto-detection for migration 029
+ * Tests the fix for issue #762: Migration ID 029 reused across versions
+ */
+
+import { describe, it, expect, beforeEach, vi } from 'vitest'
+import { MigrationService } from './migrations'
+
+// Mock D1Database with configurable responses
+function createMockDb(options: {
+  appliedMigrations?: Array<{ id: string; name: string; filename: string; applied_at: string }>;
+  existingTables?: string[];
+  existingColumns?: Array<{ table: string; column: string }>;
+} = {}) {
+  const {
+    appliedMigrations = [],
+    existingTables = [],
+    existingColumns = []
+  } = options
+
+  const mockRun = vi.fn().mockResolvedValue({ success: true })
+  const mockFirst = vi.fn()
+  const mockAll = vi.fn()
+  const mockBind = vi.fn()
+
+  // Track queries to respond appropriately
+  let lastQuery = ''
+
+  const chainable = {
+    bind: (...args: any[]) => {
+      mockBind(...args)
+      // For table existence checks
+      if (lastQuery.includes('sqlite_master')) {
+        const tableName = args[0]
+        return {
+          first: vi.fn().mockResolvedValue(
+            existingTables.includes(tableName) ? { name: tableName } : null
+          ),
+          all: mockAll,
+          run: mockRun,
+          bind: chainable.bind
+        }
+      }
+      // For column existence checks
+      if (lastQuery.includes('pragma_table_info')) {
+        const [table, column] = args
+        const exists = existingColumns.some(c => c.table === table && c.column === column)
+        return {
+          first: vi.fn().mockResolvedValue(exists ? { name: column } : null),
+          all: mockAll,
+          run: mockRun,
+          bind: chainable.bind
+        }
+      }
+      // For migration table queries
+      if (lastQuery.includes('SELECT id, name, filename, applied_at FROM migrations')) {
+        return {
+          first: mockFirst,
+          all: vi.fn().mockResolvedValue({ results: appliedMigrations }),
+          run: mockRun,
+          bind: chainable.bind
+        }
+      }
+      // For migration applied check (SELECT COUNT)
+      if (lastQuery.includes('SELECT COUNT')) {
+        const migrationId = args[0]
+        const exists = appliedMigrations.some(m => m.id === migrationId)
+        return {
+          first: vi.fn().mockResolvedValue({ count: exists ? 1 : 0 }),
+          all: mockAll,
+          run: mockRun,
+          bind: chainable.bind
+        }
+      }
+      return {
+        first: mockFirst,
+        all: mockAll,
+        run: mockRun,
+        bind: chainable.bind
+      }
+    },
+    first: mockFirst,
+    all: vi.fn().mockResolvedValue({ results: appliedMigrations }),
+    run: mockRun
+  }
+
+  const mockPrepare = vi.fn((query: string) => {
+    lastQuery = query
+    // For CREATE TABLE (init migrations table)
+    if (query.includes('CREATE TABLE IF NOT EXISTS migrations')) {
+      return { run: mockRun, bind: chainable.bind, first: mockFirst, all: mockAll }
+    }
+    // For SELECT from migrations (applied migrations list)
+    if (query.includes('SELECT id, name, filename, applied_at FROM migrations')) {
+      return {
+        all: vi.fn().mockResolvedValue({ results: appliedMigrations }),
+        bind: chainable.bind,
+        first: mockFirst,
+        run: mockRun
+      }
+    }
+    return chainable
+  })
+
+  return {
+    prepare: mockPrepare,
+    _mocks: { prepare: mockPrepare, bind: mockBind, first: mockFirst, all: mockAll, run: mockRun }
+  }
+}
+
+describe('MigrationService', () => {
+  describe('autoDetectAppliedMigrations - migration 029 (forms)', () => {
+    it('should mark 029 as applied when forms tables exist but migration is not recorded', async () => {
+      const db = createMockDb({
+        appliedMigrations: [],
+        existingTables: ['forms', 'form_submissions', 'form_files'],
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      const migrations = await service.getAvailableMigrations()
+
+      const migration029 = migrations.find(m => m.id === '029')
+      expect(migration029?.applied).toBe(true)
+
+      // Verify markMigrationApplied was called for 029
+      const insertCalls = db._mocks.prepare.mock.calls.filter(
+        (call: any[]) => call[0].includes('INSERT OR REPLACE')
+      )
+      const marked029 = insertCalls.some((call: any[]) => {
+        // The bind call after INSERT would contain '029'
+        return true // We just verify the INSERT was called
+      })
+      expect(insertCalls.length).toBeGreaterThan(0)
+    })
+
+    it('should remove 029 from applied when marked as applied but forms tables are missing', async () => {
+      const db = createMockDb({
+        appliedMigrations: [
+          { id: '029', name: 'Ai Search Plugin', filename: '029_ai_search_plugin.sql', applied_at: '2024-01-01' }
+        ],
+        existingTables: [], // No forms tables
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      const migrations = await service.getAvailableMigrations()
+
+      const migration029 = migrations.find(m => m.id === '029')
+      expect(migration029?.applied).toBe(false)
+
+      // Verify removeMigrationApplied was called (DELETE query)
+      const deleteCalls = db._mocks.prepare.mock.calls.filter(
+        (call: any[]) => call[0].includes('DELETE FROM migrations')
+      )
+      expect(deleteCalls.length).toBeGreaterThan(0)
+    })
+
+    it('should keep 029 as applied when marked as applied and forms tables exist', async () => {
+      const db = createMockDb({
+        appliedMigrations: [
+          { id: '029', name: 'Add Forms System', filename: '029_add_forms_system.sql', applied_at: '2024-01-01' }
+        ],
+        existingTables: ['forms', 'form_submissions', 'form_files'],
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      const migrations = await service.getAvailableMigrations()
+
+      const migration029 = migrations.find(m => m.id === '029')
+      expect(migration029?.applied).toBe(true)
+    })
+
+    it('should leave 029 as pending when not applied and forms tables do not exist', async () => {
+      const db = createMockDb({
+        appliedMigrations: [],
+        existingTables: [], // No forms tables
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      const migrations = await service.getAvailableMigrations()
+
+      const migration029 = migrations.find(m => m.id === '029')
+      expect(migration029?.applied).toBe(false)
+    })
+
+    it('should not mark 029 as applied when only some forms tables exist', async () => {
+      const db = createMockDb({
+        appliedMigrations: [],
+        existingTables: ['forms'], // Missing form_submissions and form_files
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      const migrations = await service.getAvailableMigrations()
+
+      const migration029 = migrations.find(m => m.id === '029')
+      expect(migration029?.applied).toBe(false)
+    })
+  })
+
+  describe('runPendingMigrations', () => {
+    it('should include errors array in response', async () => {
+      const db = createMockDb({
+        appliedMigrations: [],
+        existingTables: ['users', 'content', 'collections', 'media'],
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      const result = await service.runPendingMigrations()
+
+      expect(result).toHaveProperty('errors')
+      expect(Array.isArray(result.errors)).toBe(true)
+    })
+
+    it('should return empty errors when all migrations are up to date', async () => {
+      // Create a db where all bundled migrations are already applied
+      const db = createMockDb({
+        appliedMigrations: [],
+        existingTables: ['users', 'content', 'collections', 'media'],
+        existingColumns: []
+      })
+
+      const service = new MigrationService(db as any)
+      // Mock getMigrationStatus to return no pending
+      vi.spyOn(service, 'getMigrationStatus').mockResolvedValue({
+        totalMigrations: 0,
+        appliedMigrations: 0,
+        pendingMigrations: 0,
+        migrations: []
+      })
+
+      const result = await service.runPendingMigrations()
+      expect(result.errors).toEqual([])
+      expect(result.success).toBe(true)
+    })
+  })
+})

packages/core/src/services/migrations.ts

@@ -228,6 +228,27 @@ export class MigrationService {
       }
     }
 
+    // Check if forms tables exist (migration 029)
+    // Migration 029 was reassigned between releases: older versions used it for "Ai Search Plugin",
+    // newer versions use it for "Add Forms System". This handles the case where 029 is marked as
+    // applied (from the old AI Search migration) but the forms tables don't actually exist.
+    const hasFormsTables = await this.checkTablesExist(['forms', 'form_submissions', 'form_files'])
+    if (!appliedMigrations.has('029') && hasFormsTables) {
+      appliedMigrations.set('029', {
+        id: '029',
+        applied_at: new Date().toISOString(),
+        name: 'Add Forms System',
+        filename: '029_add_forms_system.sql'
+      })
+      await this.markMigrationApplied('029', 'Add Forms System', '029_add_forms_system.sql')
+    } else if (appliedMigrations.has('029') && !hasFormsTables) {
+      // Migration was marked as applied (possibly from old "Ai Search Plugin" migration)
+      // but forms tables don't exist - remove it so the forms migration will re-run
+      console.log('[Migration] Migration 029 marked as applied but forms tables missing - will re-run')
+      appliedMigrations.delete('029')
+      await this.removeMigrationApplied('029')
+    }
+
     // Check if user_profiles table exists (migration 032)
     // Table may already exist from app-level migration (my-sonicjs-app/migrations/018_user_profiles.sql)
     if (!appliedMigrations.has('032')) {
@@ -361,7 +382,7 @@ export class MigrationService {
   /**
    * Run pending migrations
    */
-  async runPendingMigrations(): Promise<{ success: boolean; message: string; applied: string[] }> {
+  async runPendingMigrations(): Promise<{ success: boolean; message: string; applied: string[]; errors: string[] }> {
     await this.initializeMigrationsTable()
 
     const status = await this.getMigrationStatus()
@@ -371,7 +392,8 @@ export class MigrationService {
       return {
         success: true,
         message: 'All migrations are up to date',
-        applied: []
+        applied: [],
+        errors: []
       }
     }
 
@@ -399,7 +421,8 @@ export class MigrationService {
       return {
         success: false,
         message: `Failed to apply migrations: ${errors.join('; ')}`,
-        applied
+        applied,
+        errors
       }
     }
 
@@ -408,7 +431,8 @@ export class MigrationService {
       message: applied.length > 0
         ? `Applied ${applied.length} migration(s)${errors.length > 0 ? ` (${errors.length} failed)` : ''}`
         : 'No migrations applied',
-      applied
+      applied,
+      errors
     }
   }