Review: fix: custom status validation falls back to config when table is empty

Verdict: Correct fix for schema migration edge case. Two components working in concert: fallback logic in config helpers + error handling in list.go. One observation on test coverage.

+16 −6 across 2 files (cmd/bd/list.go +6, internal/storage/issueops/config_helpers.go +10).

Root Cause: Schema v11 Migration Gap ✅

After schema v11 migration, the custom_statuses and custom_types tables are created but not populated from the existing config values (status.custom / types.custom).

Old code logic:

1. Query table
2. If query succeeds (table exists), return result even if empty → stops here
3. If query fails (table doesn't exist), fall back to config string

Problem: Step 2 returns empty array, bypassing the config fallback. Users with pre-migration status.custom configs lose access to their custom statuses after v11 because the table is empty and the fallback is skipped.

Fix: Two-Layer Approach ✅

Layer 1 (config_helpers.go): Modified fallback logic in ResolveCustomStatusesDetailedInTx and ResolveCustomTypesInTx:

-// Table query succeeded — return result even if empty.
-return result, nil

+if len(result) > 0 {
+    return result, nil
+}
+// Table exists but is empty — fall through to config string.

This shifts the fallback condition from "table exists?" to "table has rows?" Good semantic fix.

Layer 2 (list.go): Previously, GetCustomStatuses errors were silently swallowed:

-cs, _ := store.GetCustomStatuses(rootCtx)
-customStatuses = cs

Now errors are logged (except in JSON output):

+cs, err := store.GetCustomStatuses(rootCtx)
+if err != nil {
+    if !jsonOutput {
+        fmt.Fprintf(os.Stderr, "⚠ Failed to load custom statuses: %v\n", err)
+    }
+} else {
+    customStatuses = cs
+}

This matches the error handling pattern in update.go (per PR description). Good consistency.

Correctness: Precedence and Fallbacks ✅

After the fix, the resolution precedence is:

1. custom_statuses table (if has rows)
2. status.custom config string (if table empty or doesn't exist)
3. YAML config fallback (via GetCustomStatusesFromYAML())

Same logic for custom_types:

1. custom_types table (if has rows)
2. types.custom config string (if table empty or doesn't exist)
3. YAML config fallback (via GetCustomTypesFromYAML())

This is correct. The migration gap is bridged: users with pre-v11 status.custom configs will still work after v11 if the table is empty.

Edge Cases ✅

Case 1: Fully migrated (table populated)

• Query returns rows
• len(result) > 0 → return table result
• Config fallback skipped ✓

Case 2: Post-migration, table empty (THE FIX)

• Query succeeds but returns no rows
• len(result) > 0 is false → fall through to config
• Config string is parsed and returned ✓

Case 3: Pre-migration (table doesn't exist)

• Query fails (QueryContext error)
• Falls through to config string immediately ✓

Case 4: All paths empty/nil

• Table empty, config empty, YAML empty
• Returns nil (safe, no custom statuses defined) ✓

Error Handling: list.go Changes ✅

The error logging is conditional:

if err != nil {
    if !jsonOutput {
        fmt.Fprintf(os.Stderr, "⚠ Failed to load custom statuses: %v\n", err)
    }
}

This respects JSON output mode (doesn't emit stderr in JSON mode). Good. The error is logged but not fatal — the command continues with customStatuses = [] (nil slice). This is safe: list will show issues without custom status filtering/validation, but won't crash.

Test Coverage: Partial ✅

PR test plan shows:

• ✅ bd list --status=in_review works (manual test)
• ✅ bd config get status.custom returns values (manual test)
• ✅ Build passes
• ❌ "Existing tests pass" is marked incomplete

The fix changes core fallback logic in two functions (ResolveCustomStatusesDetailedInTx, ResolveCustomTypesInTx). These functions likely have existing unit tests in config_helpers_test.go or similar. Running existing tests is critical before merging — the two-layer fix could have subtle interactions.

🟡 Empty slice vs nil return semantics

The functions return different types in different paths:

• Table has rows: returns []types.CustomStatus (non-nil slice with len > 0)
• Table empty or doesn't exist: returns fallback result (could be nil, empty slice, or populated)
• All empty: returns nil

This is acceptable (Go convention), but means callers need to handle both len(statuses) == 0 (empty slice) and statuses == nil (nil slice). The list.go code uses len(customStatuses) == 1 (single status) logic, which handles both correctly. But worth noting for future callers.

🟡 Error message in list.go uses generic format

fmt.Fprintf(os.Stderr, "⚠ Failed to load custom statuses: %v\n", err)

The error err comes from store.GetCustomStatuses(), which wraps underlying SQL/storage errors. For user clarity, could be more specific (e.g., "Failed to load custom statuses from database"). But the current message is acceptable and matches update.go pattern.

Summary

LGTM. This is a focused, correct fix for a real schema migration edge case. The two-layer approach (config helper + error logging) is sound. The only blocker is confirming existing tests pass — the PR test plan marks that as incomplete. Once confirmed, this is ready to ship.

Reviewed. The empty-table fallback to config is correct. One suggested improvement from rebasing: in list.go, wrap the stderr warning in if !jsonOutput so JSON consumers don't get warning noise mixed into structured output. This guard was missing from HEAD and combining both makes the final result cleaner. Otherwise LGTM. ✓