fix: Commit() excludes config table to prevent issue_prefix corruption (GH#2455)

DOLT_COMMIT('-Am') stages ALL dirty tables, sweeping up stale config
changes from concurrent operations. Replace with selective staging that
queries dolt_status and skips the config table. Add CommitWithConfig()
for callers that intentionally modify config (bd dolt commit).

Also fix RunMigrations(), FK migration, and merge conflict resolution
to use explicit DOLT_ADD instead of -Am.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: steveyegge

internal/storage/dolt/migrations.go

@@ -40,8 +40,20 @@ func RunMigrations(db *sql.DB) error {
 		}
 	}
 
-	// Commit schema changes via Dolt (idempotent - no-ops if nothing changed)
-	_, err := db.Exec("CALL DOLT_COMMIT('-Am', 'schema: auto-migrate')")
+	// GH#2455: Stage only schema tables (not config) to avoid sweeping up
+	// stale issue_prefix changes from concurrent operations. The old '-Am'
+	// approach staged ALL dirty tables including config.
+	migrationTables := []string{
+		"issues", "wisps", "events", "wisp_events", "dependencies",
+		"wisp_dependencies", "labels", "wisp_labels", "comments",
+		"wisp_comments", "metadata", "child_counters", "issue_counter",
+		"issue_snapshots", "compaction_snapshots", "federation_peers",
+		"dolt_ignore",
+	}
+	for _, table := range migrationTables {
+		_, _ = db.Exec("CALL DOLT_ADD(?)", table)
+	}
+	_, err := db.Exec("CALL DOLT_COMMIT('-m', 'schema: auto-migrate')")
 	if err != nil {
 		// "nothing to commit" is expected when migrations were already applied
 		if !strings.Contains(strings.ToLower(err.Error()), "nothing to commit") {

internal/storage/dolt/store.go

@@ -18,7 +18,6 @@ import (
 	"errors"
 	"fmt"
 	"hash/fnv"
-	"log"
 	"net"
 	"os"
 	"os/exec"
@@ -1017,7 +1016,9 @@ func initSchemaOnDB(ctx context.Context, db *sql.DB) error {
 	_, err = db.ExecContext(ctx, "ALTER TABLE dependencies DROP FOREIGN KEY fk_dep_depends_on")
 	if err == nil {
 		// DDL change succeeded - commit it so it persists (required for Dolt server mode)
-		_, _ = db.ExecContext(ctx, "CALL DOLT_COMMIT('-Am', 'migration: remove fk_dep_depends_on for external references')") // Best effort: migration commit is advisory; schema change already applied
+		// GH#2455: Stage only the affected table, not all dirty tables.
+		_, _ = db.ExecContext(ctx, "CALL DOLT_ADD('dependencies')")
+		_, _ = db.ExecContext(ctx, "CALL DOLT_COMMIT('-m', 'migration: remove fk_dep_depends_on for external references')") // Best effort: migration commit is advisory; schema change already applied
 	} else if !strings.Contains(strings.ToLower(err.Error()), "can't drop") &&
 		!strings.Contains(strings.ToLower(err.Error()), "doesn't exist") &&
 		!strings.Contains(strings.ToLower(err.Error()), "check that it exists") &&
@@ -1205,51 +1206,92 @@ func (s *DoltStore) commitAuthorString() string {
 
 // Commit creates a Dolt commit with the given message.
 //
-// Uses DOLT_COMMIT('-Am') to stage all dirty tables and commit. This is the
-// general-purpose commit used by maybeAutoCommit and callers that want to
-// commit all pending working set changes. Defense-in-depth for GH#2455:
-// snapshots issue_prefix before committing and restores it if corrupted
-// by stale working set changes from concurrent operations.
+// GH#2455: Stages all dirty tables EXCEPT config, then commits with '-m'.
+// The old '-Am' approach staged ALL dirty tables including config, which
+// swept up stale issue_prefix changes from concurrent operations. By
+// excluding config from automatic staging, we prevent the corruption.
+//
+// Callers that intentionally modify config (e.g., CommitPending after
+// 'bd config set') must call CommitWithConfig instead.
 func (s *DoltStore) Commit(ctx context.Context, message string) (retErr error) {
 	ctx, span := doltTracer.Start(ctx, "dolt.commit",
 		trace.WithSpanKind(trace.SpanKindClient),
 		trace.WithAttributes(s.doltSpanAttrs()...),
 	)
 	defer func() { endSpan(span, retErr) }()
 
-	// Pin a single connection so snapshot, commit, and restore all run
-	// on the same Dolt session. (GH#2455)
+	// Pin a single connection so all operations run on the same Dolt session.
 	conn, err := s.db.Conn(ctx)
 	if err != nil {
 		return fmt.Errorf("failed to acquire connection: %w", err)
 	}
 	defer conn.Close()
 
-	// Snapshot issue_prefix from HEAD before commit to detect corruption.
-	var prefixBefore string
-	_ = conn.QueryRowContext(ctx, "SELECT value FROM config WHERE `key` = 'issue_prefix'").Scan(&prefixBefore)
+	// GH#2455: Stage all dirty tables EXCEPT config. Query dolt_status for
+	// dirty tables and stage each one individually, skipping config to avoid
+	// sweeping up stale issue_prefix changes from concurrent operations.
+	rows, err := conn.QueryContext(ctx, "SELECT table_name FROM dolt_status")
+	if err != nil {
+		// If dolt_status fails, fall back to nothing (rare edge case).
+		return fmt.Errorf("failed to query dolt_status: %w", err)
+	}
+	var tables []string
+	for rows.Next() {
+		var table string
+		if err := rows.Scan(&table); err != nil {
+			_ = rows.Close()
+			return fmt.Errorf("failed to scan dolt_status: %w", err)
+		}
+		if table != "config" {
+			tables = append(tables, table)
+		}
+	}
+	_ = rows.Close()
+	if err := rows.Err(); err != nil {
+		return fmt.Errorf("failed to iterate dolt_status: %w", err)
+	}
+
+	if len(tables) == 0 {
+		return nil // Nothing to commit (all changes were config-only or dolt_ignore'd)
+	}
+
+	for _, table := range tables {
+		if _, err := conn.ExecContext(ctx, "CALL DOLT_ADD(?)", table); err != nil {
+			// Best effort: some tables may be dolt_ignore'd (e.g., wisps).
+			// DOLT_ADD fails for ignored tables; skip silently.
+			continue
+		}
+	}
 
 	// NOTE: In SQL procedure mode, Dolt defaults author to the authenticated SQL user
 	// (e.g. root@localhost). Always pass an explicit author for deterministic history.
-	if _, err := conn.ExecContext(ctx, "CALL DOLT_COMMIT('-Am', ?, '--author', ?)", message, s.commitAuthorString()); err != nil {
+	if _, err := conn.ExecContext(ctx, "CALL DOLT_COMMIT('-m', ?, '--author', ?)", message, s.commitAuthorString()); err != nil {
 		if isDoltNothingToCommit(err) {
 			return nil
 		}
 		return fmt.Errorf("failed to commit: %w", err)
 	}
 
-	// Verify issue_prefix wasn't corrupted by sweeping up stale working set changes.
-	if prefixBefore != "" {
-		var prefixAfter string
-		_ = conn.QueryRowContext(ctx, "SELECT value FROM config WHERE `key` = 'issue_prefix'").Scan(&prefixAfter)
-		if prefixAfter != prefixBefore {
-			// Restore the correct prefix and create a fix-up commit.
-			log.Printf("WARNING: issue_prefix corrupted by DOLT_COMMIT (%q -> %q), restoring (GH#2455)", prefixBefore, prefixAfter)
-			_, _ = conn.ExecContext(ctx, "UPDATE config SET value = ? WHERE `key` = 'issue_prefix'", prefixBefore)
-			_, _ = conn.ExecContext(ctx, "CALL DOLT_COMMIT('-Am', 'fix: restore issue_prefix corrupted by stale working set (GH#2455)', '--author', ?)", s.commitAuthorString())
-		}
+	return nil
+}
+
+// CommitWithConfig creates a Dolt commit that includes the config table.
+// Use this instead of Commit when the caller intentionally modified config
+// (e.g., CommitPending after 'bd config set', 'bd init', or 'bd rename-prefix').
+// GH#2455: Commit() excludes config to prevent sweeping up stale changes.
+func (s *DoltStore) CommitWithConfig(ctx context.Context, message string) error {
+	conn, err := s.db.Conn(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to acquire connection: %w", err)
 	}
+	defer conn.Close()
 
+	if _, err := conn.ExecContext(ctx, "CALL DOLT_COMMIT('-Am', ?, '--author', ?)", message, s.commitAuthorString()); err != nil {
+		if isDoltNothingToCommit(err) {
+			return nil
+		}
+		return fmt.Errorf("failed to commit: %w", err)
+	}
 	return nil
 }
 
@@ -1301,7 +1343,10 @@ func (s *DoltStore) CommitPending(ctx context.Context, actor string) (bool, erro
 	}
 
 	msg := s.buildBatchCommitMessage(ctx, actor)
-	if err := s.Commit(ctx, msg); err != nil {
+	// GH#2455: CommitPending is an explicit user action (bd dolt commit) that
+	// should include ALL pending changes, including config. Use CommitWithConfig
+	// instead of Commit to ensure intentional config changes are committed.
+	if err := s.CommitWithConfig(ctx, msg); err != nil {
 		// Dolt may report "nothing to commit" even when Status() showed changes
 		// (e.g., system tables or schema-only diffs). Treat as no-op.
 		errLower := strings.ToLower(err.Error())
@@ -1711,8 +1756,11 @@ func (s *DoltStore) tryAutoResolveMetadataConflicts(ctx context.Context, tx *sql
 		return false, fmt.Errorf("failed to resolve metadata conflicts: %w", err)
 	}
 
-	// Commit the merge with resolved conflicts.
-	if _, err := tx.ExecContext(ctx, "CALL DOLT_COMMIT('-Am', 'auto-resolve metadata merge conflicts (GH#2466)')"); err != nil {
+	// GH#2455: Stage only metadata (the table we resolved), not all dirty tables.
+	if _, err := tx.ExecContext(ctx, "CALL DOLT_ADD('metadata')"); err != nil {
+		return false, fmt.Errorf("failed to stage metadata: %w", err)
+	}
+	if _, err := tx.ExecContext(ctx, "CALL DOLT_COMMIT('-m', 'auto-resolve metadata merge conflicts (GH#2466)')"); err != nil {
 		return false, fmt.Errorf("failed to commit resolved conflicts: %w", err)
 	}
 

internal/storage/dolt/wisp_gc_test.go

@@ -279,11 +279,12 @@ func TestRunInTransaction_DoesNotCorruptConfig(t *testing.T) {
 
 	// Set the canonical issue_prefix and create an issue BEFORE corrupting config.
 	// CreateIssue has its own DOLT_COMMIT, so we must set up the issue first.
+	// Use CommitWithConfig since we're intentionally modifying config.
 	if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil {
 		t.Fatalf("SetConfig: %v", err)
 	}
-	if err := store.Commit(ctx, "set prefix"); err != nil {
-		t.Fatalf("Commit: %v", err)
+	if err := store.CommitWithConfig(ctx, "set prefix"); err != nil {
+		t.Fatalf("CommitWithConfig: %v", err)
 	}
 
 	issue := &types.Issue{
@@ -340,6 +341,90 @@ func TestRunInTransaction_DoesNotCorruptConfig(t *testing.T) {
 	}
 }
 
+// TestCommit_ExcludesConfig verifies that s.Commit() does NOT stage the config
+// table, preventing stale issue_prefix changes from being swept into commits.
+// This is the core fix for GH#2455: Commit() now stages all dirty tables
+// EXCEPT config. Config is only committed by CommitWithConfig (used by
+// CommitPending for explicit 'bd dolt commit').
+func TestCommit_ExcludesConfig(t *testing.T) {
+	store, cleanup := setupTestStore(t)
+	defer cleanup()
+
+	ctx, cancel := testContext(t)
+	defer cancel()
+
+	// Set and commit a known-good prefix via CommitWithConfig
+	if err := store.SetConfig(ctx, "issue_prefix", "correct"); err != nil {
+		t.Fatalf("SetConfig: %v", err)
+	}
+	if err := store.CommitWithConfig(ctx, "set correct prefix"); err != nil {
+		t.Fatalf("CommitWithConfig: %v", err)
+	}
+
+	// Simulate stale corruption: another operation modified issue_prefix in
+	// the working set without DOLT_COMMIT'ing. The working set now has a
+	// WRONG value while HEAD still has "correct".
+	_, err := store.db.ExecContext(ctx, "UPDATE config SET value = 'WRONG' WHERE `key` = 'issue_prefix'")
+	if err != nil {
+		t.Fatalf("simulate stale config change: %v", err)
+	}
+
+	// Also dirty a real table so Commit() has something to commit.
+	_, err = store.db.ExecContext(ctx, "INSERT INTO metadata (`key`, value) VALUES ('_gh2455_test', 'v') ON DUPLICATE KEY UPDATE value = 'v'")
+	if err != nil {
+		t.Fatalf("insert metadata: %v", err)
+	}
+
+	// Call s.Commit() — the general-purpose path. With the old '-Am'
+	// approach, this would stage ALL dirty tables including config,
+	// committing the "WRONG" value. With the fix, Commit() skips config
+	// entirely, so the committed issue_prefix remains "correct".
+	if err := store.Commit(ctx, "test commit that should skip config"); err != nil {
+		t.Fatalf("Commit: %v", err)
+	}
+
+	// Verify issue_prefix was NOT corrupted — HEAD should still have "correct".
+	var committedPrefix string
+	err = store.db.QueryRowContext(ctx,
+		"SELECT value FROM config AS OF 'HEAD' WHERE `key` = 'issue_prefix'").Scan(&committedPrefix)
+	if err != nil {
+		t.Fatalf("query HEAD prefix: %v", err)
+	}
+	if committedPrefix != "correct" {
+		t.Errorf("GH#2455 regression: HEAD issue_prefix = %q, want %q", committedPrefix, "correct")
+	}
+}
+
+// TestCommitWithConfig_IncludesConfig verifies that CommitWithConfig DOES
+// commit config changes. This is used by CommitPending (bd dolt commit)
+// when the user intentionally modified config.
+func TestCommitWithConfig_IncludesConfig(t *testing.T) {
+	store, cleanup := setupTestStore(t)
+	defer cleanup()
+
+	ctx, cancel := testContext(t)
+	defer cancel()
+
+	// Change issue_prefix and commit with CommitWithConfig (intentional change)
+	if err := store.SetConfig(ctx, "issue_prefix", "newprefix"); err != nil {
+		t.Fatalf("SetConfig: %v", err)
+	}
+	if err := store.CommitWithConfig(ctx, "intentional prefix change"); err != nil {
+		t.Fatalf("CommitWithConfig: %v", err)
+	}
+
+	// Verify the intentional change was committed
+	var committedPrefix string
+	err := store.db.QueryRowContext(ctx,
+		"SELECT value FROM config AS OF 'HEAD' WHERE `key` = 'issue_prefix'").Scan(&committedPrefix)
+	if err != nil {
+		t.Fatalf("query HEAD prefix: %v", err)
+	}
+	if committedPrefix != "newprefix" {
+		t.Errorf("CommitWithConfig should include config: HEAD issue_prefix = %q, want %q", committedPrefix, "newprefix")
+	}
+}
+
 // TestFindWispDependentsRecursive_NoDependents verifies wisps with no
 // dependents return an empty map.
 func TestFindWispDependentsRecursive_NoDependents(t *testing.T) {