Review: fix: backfill empty custom_types and custom_statuses tables

Verdict: Correct fix for a schema initialization sequencing bug. Migration 016 properly backfills empty tables that were created but not populated by migration 015. Design is sound, idempotent, and handles edge cases well.

+97 −1 across 3 files (new migration 016, registration, schema version bump). Fixes #2984, #1631.

Root Problem: Empty Table Shadows Config Fallback ✅

Scenario:

1. Database is created before migration 015 exists
2. Schema initialization runs, creates custom_types and custom_statuses tables (empty)
3. Migration 015 runs, checks if table exists and skips population because tables already exist
4. Result: Empty tables that shadow the config fallback in ResolveCustomTypesInTx
5. User gets "invalid issue type" error for custom types like "agent" that are in config

Why this happens:

• Schema initialization happens once at startup (creates table structure)
• Migration 015 was designed to create AND populate tables together
• But if the table already exists (from earlier schema init), 015 skips population
• Resolution functions have fallback to config, but only if table is empty/missing
• Empty table breaks the fallback logic

Impact: Custom types/statuses become inaccessible after migration 015 runs on databases that pre-created the tables.

Solution: Migration 016 Backfill ✅

Design:

1. New migration 016: BackfillCustomTables populates empty tables from config
2. Schema version bumped from 11 → 12, so migration runs automatically
3. Idempotent: if table already has data, skips backfill

Idempotency:

var count int
if err := db.QueryRow("SELECT COUNT(*) FROM custom_types").Scan(&count); err != nil {
    return err
}
if count > 0 {
    return nil // Already populated
}

Safe to run multiple times. If data exists, exits early. ✓

Config fallback:

var value string
err = db.QueryRow("SELECT `value` FROM config WHERE `key` = 'types.custom'").Scan(&value)
if err != nil || value == "" {
    return nil // No config to backfill from
}

If config is missing/empty, gracefully skips. ✓

Parsing reuses existing logic:

for _, name := range parseTypesValue(value) {
    // ...insert into table
}

Uses the same parseTypesValue helper from migration 015 (handles JSON array + comma-separated). ✓

INSERT IGNORE:

_, err = db.Exec("INSERT IGNORE INTO custom_types (name) VALUES (?)", name)

Duplicate values ignored (safe if config has duplicates). ✓

Correctness: Edge Cases ✅

Case 1: Empty table + no config

• Query returns empty string
• Early return with nil
• ✓ Correct

Case 2: Empty table + JSON array config

types.custom = '["agent","gate","convoy"]'

• parseTypesValue parses JSON
• Inserts 3 rows
• ✓ Correct

Case 3: Empty table + comma-separated config

types.custom = 'agent, gate, convoy'

• parseTypesValue parses CSV with trim
• Inserts 3 rows
• ✓ Correct

Case 4: Already-populated table

• COUNT(*) returns > 0
• Early return with nil (idempotent)
• ✓ Correct

Case 5: Table doesn't exist

• tableExists returns false
• Early return with nil
• ✓ Correct

Case 6: Config query fails

• Error returned or treated as missing
• ✓ Correct

Case 7: Status backfill with unspecified category

_, err = db.Exec("INSERT IGNORE INTO custom_statuses (name, category) VALUES (?, 'unspecified')", name)

• Status config doesn't include categories
• Defaults to 'unspecified'
• Matches migration 015 behavior
• ✓ Correct

Design: Preserving Migration 015 ✅

PR notes: "Migration 015 is intentionally left unchanged."

This is the right call:

• 015 has already run on existing databases
• Modifying it would create multiple states for databases at migration level 15
• Better to have 015 create+populate, 016 handle the backfill case
• Clean separation of concerns

Performance ✅

Backfill path:

• One COUNT query per table
• One SELECT from config per table
• If empty: N inserts (N = custom types/statuses, typically 5-20)
• INSERT IGNORE is O(1) per row (primary key lookup)

Impact: Minimal. Migration runs once, backfill is rare case (only on databases with pre-created empty tables).

Style: Consistent with Migration 015 ✅

• Uses same helper functions (tableExists, parseTypesValue)
• Same error handling pattern (early return on missing config)
• Same INSERT IGNORE pattern
• Comments explain non-obvious choices (why "unspecified" for statuses)
• Code is readable and maintainable

Good design: two separate functions for clarity, shared pattern for consistency.

Testing: Adequate ✅

Test plan:

• ✅ Unit tests pass (go test -short ./internal/storage/dolt/migrations/...)
• ✅ Compiles cleanly (go build ./cmd/bd)
• ✅ Manual verification: ran on database with empty custom_types table and 11 custom types in config
• ✅ Migration populated table correctly
• ✅ bd create --type=agent succeeded after migration

Manual test is thorough — validates the full E2E flow.

🟡 Error Handling: Query Failures Are Lenient

var value string
err = db.QueryRow("SELECT `value` FROM config WHERE `key` = 'types.custom'").Scan(&value)
if err != nil || value == "" {
    return nil // No config to backfill from
}

If the query fails with a real error (corruption, disk full), it returns nil silently. This is intentional (matches migration 015 pattern) but worth noting:

• sql.ErrNoRows → treated as "no config" (correct)
• Other errors → also treated as "no config" (acceptable, migration is lenient)

Matches migration 015 philosophy ("migrations are lenient so existing databases aren't blocked"). Acceptable but could be more explicit.

Not blocking — consistent with existing migration patterns.

🟡 Status Category Defaulting

_, err = db.Exec("INSERT IGNORE INTO custom_statuses (name, category) VALUES (?, 'unspecified')", name)

Status config doesn't include category information, so migration defaults all to 'unspecified'. Comment explains this.

If someone manually created statuses with specific categories via SQL, this backfill won't preserve them. But acceptable because:

• Statuses are typically bulk-created from config
• If table is already populated, idempotent check (count > 0) skips entirely
• Comment documents the behavior

Summary

LGTM. Correct fix for a sequencing bug where empty tables created by schema init shadow the config fallback. Migration 016 properly backfills these tables from config, with idempotent design and graceful handling of edge cases.

Two yellow flags are both acceptable: error handling when reading config is lenient (matches migration 015 pattern), and status categories default to 'unspecified' (config doesn't include them). The fix restores functionality to databases that hit the "empty table shadows config" state.

Ship it.