Make hash IDs opt-in via id_mode config (bd-5404)

- Add id_mode config (sequential|hash), defaults to sequential - Update CreateIssue/CreateIssues to check id_mode and generate appropriate IDs - Implement lazy counter initialization from existing issues - Update migrate --to-hash-ids to set id_mode=hash after migration - Fix hash ID tests to set id_mode=hash - Fix renumber test to use explicit IDs - All 183 test packages pass This makes hash IDs backward-compatible opt-in rather than forced default.
2025-10-30 16:50:38 -07:00
parent 0858bdb7ed
commit 1fbfe58ba7
5 changed files with 145 additions and 55 deletions
--- a/.beads/beads.jsonl
+++ b/.beads/beads.jsonl
@@ -130,6 +130,8 @@
 {"id":"bd-5403defc.1","content_hash":"d1ffe0d966939abf9449b6157cf9fcf42342b3056bfa65aeffbfa913ff722928","title":"Child test","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-30T15:46:47.841563-07:00","updated_at":"2025-10-30T15:46:59.618715-07:00","closed_at":"2025-10-30T15:46:59.618715-07:00"}
 {"id":"bd-5403defc.1.1","content_hash":"f835dedf00bec5edfac81de035e4b5af1490afa7008bdf74683041c44d33d830","title":"Nested child","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-30T15:46:51.064625-07:00","updated_at":"2025-10-30T15:46:59.618994-07:00","closed_at":"2025-10-30T15:46:59.618994-07:00"}
 {"id":"bd-5403defc.1.1.1","content_hash":"0c150383d4c2ce7aeecf70fe53f2599e9720eccffc7ab717a2abfef8e37f9dcc","title":"Deep nested","description":"","status":"closed","priority":2,"issue_type":"task","created_at":"2025-10-30T15:46:53.570315-07:00","updated_at":"2025-10-30T15:46:59.619236-07:00","closed_at":"2025-10-30T15:46:59.619236-07:00"}
+{"id":"bd-5404","content_hash":"71db1274320e28acd735902b9c07c2683b0c7dcbd7808637457dcb492273f19f","title":"Hash IDs as default: testing, release, and migration","description":"Drive hash IDs to be the default for Beads. Currently on feature/hash-ids branch, opt-in via --to-hash-ids flag, requires explicit migration. Target repos: ~/src/fred/beads, ~/wyvern, ~/src/wyvern. Deployment: homebrew/bin, MCP servers, all daemons at 0.19.1","design":"1. Checkout feature/hash-ids 2. Bump to 0.19.1 on branch 3. Build/install to homebrew 4. Update MCP servers 5. Restart daemons 6. Migrate all repos 7. Test workers 8. Make hash IDs default 9. Verify","acceptance_criteria":"All 3 repos migrated, all daemons at 0.19.1, workers functional, hash IDs default","status":"open","priority":1,"issue_type":"epic","created_at":"2025-10-30T16:33:07.965639-07:00","updated_at":"2025-10-30T16:33:07.965639-07:00"}
+{"id":"bd-5405","content_hash":"4c12ad67469db8ac3b9a9d6199c78521202b54a5cb4c0571ea1fb5e4cd8d42bc","title":"Test mixed ID system","description":"","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-30T16:36:18.414852-07:00","updated_at":"2025-10-30T16:36:18.414852-07:00"}
 {"id":"bd-55","content_hash":"d4d20e71bbf5c08f1fe1ed07f67b7554167aa165d4972ea51b5cacc1b256c4c1","title":"Split internal/rpc/server.go into focused modules","description":"The file `internal/rpc/server.go` is 2,273 lines with 50+ methods, making it difficult to navigate and prone to merge conflicts. Split into 8 focused files with clear responsibilities.\n\nCurrent structure: Single 2,273-line file with:\n- Connection handling\n- Request routing\n- All 40+ RPC method implementations\n- Storage caching\n- Health checks \u0026 metrics\n- Cleanup loops\n\nTarget structure:\n```\ninternal/rpc/\n├── server.go          # Core server, connection handling (~300 lines)\n├── methods_issue.go   # Issue operations (~400 lines)\n├── methods_deps.go    # Dependency operations (~200 lines)\n├── methods_labels.go  # Label operations (~150 lines)\n├── methods_ready.go   # Ready work queries (~150 lines)\n├── methods_compact.go # Compaction operations (~200 lines)\n├── methods_comments.go # Comment operations (~150 lines)\n├── storage_cache.go   # Storage caching logic (~300 lines)\n└── health.go          # Health \u0026 metrics (~200 lines)\n```\n\nMigration strategy:\n1. Create new files with appropriate methods\n2. Keep `server.go` as main file with core server logic\n3. Test incrementally after each file split\n4. Final verification with full test suite","acceptance_criteria":"- All 50 methods split into appropriate files\n- Each file \u003c500 LOC\n- All methods remain on `*Server` receiver (no behavior change)\n- All tests pass: `go test ./internal/rpc/...`\n- Verify daemon works: start daemon, run operations, check health\n- Update internal documentation if needed\n- No change to public API","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-28T14:21:37.51524-07:00","updated_at":"2025-10-28T14:21:37.51524-07:00","closed_at":"2025-10-28T14:11:04.399811-07:00"}
 {"id":"bd-57","content_hash":"3ab290915c117ec902bda1761e8c27850512f3fd4b494a93546c44b397d573a3","title":"bd resolve-conflicts - Git merge conflict resolver","description":"Automatically resolve JSONL merge conflicts.\n\nModes:\n- Mechanical: ID remapping (no AI)\n- AI-assisted: Smart merge/keep decisions\n- Interactive: Review each conflict\n\nHandles \u003c\u003c\u003c\u003c\u003c\u003c\u003c conflict markers in .beads/beads.jsonl\n\nFiles: cmd/bd/resolve_conflicts.go (new)","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-28T14:48:17.457619-07:00","updated_at":"2025-10-28T15:47:33.037021-07:00","closed_at":"2025-10-28T15:47:33.037021-07:00"}
 {"id":"bd-58","content_hash":"04b157cdc3fb162be6695517c10365c91ed14f69fad56a7bfc2b88d6b742ac38","title":"bd repair-deps - Orphaned dependency cleaner","description":"Find and fix orphaned dependency references.\n\nImplementation:\n- Scan all issues for dependencies pointing to non-existent issues\n- Report orphaned refs\n- Auto-fix with --fix flag\n- Interactive mode with --interactive\n\nFiles: cmd/bd/repair_deps.go (new)","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-29T19:42:29.852745-07:00","updated_at":"2025-10-29T19:42:29.852745-07:00"}
--- a/cmd/bd/migrate.go
+++ b/cmd/bd/migrate.go
@@ -356,6 +356,22 @@ This command:
 						color.Green("✓ Migrated %d issues to hash-based IDs\n", len(mapping))
 					}
 				}
+				
+				// Set id_mode=hash after successful migration (not in dry-run)
+				if !dryRun {
+					store, err := sqlite.New(targetPath)
+					if err == nil {
+						ctx := context.Background()
+						if err := store.SetConfig(ctx, "id_mode", "hash"); err != nil {
+							if !jsonOutput {
+								fmt.Fprintf(os.Stderr, "Warning: failed to set id_mode=hash: %v\n", err)
+							}
+						} else if !jsonOutput {
+							color.Green("✓ Switched database to hash ID mode\n")
+						}
+						store.Close()
+					}
+				}
 			} else {
 				store.Close()
 				if !jsonOutput {
--- a/cmd/bd/renumber_test.go
+++ b/cmd/bd/renumber_test.go
@@ -44,6 +44,7 @@ func TestRenumberWithGaps(t *testing.T) {

 	for _, tc := range testIssues {
 		issue := &types.Issue{
+			ID:          tc.id, // Set explicit ID to simulate gaps
 			Title:       tc.title,
 			Description: "Test issue for renumbering",
 			Priority:    1,
@@ -53,10 +54,6 @@ func TestRenumberWithGaps(t *testing.T) {
 		if err := testStore.CreateIssue(ctx, issue, "test"); err != nil {
 			t.Fatalf("failed to create issue: %v", err)
 		}
-		// Manually update ID to simulate gaps
-		if err := testStore.UpdateIssueID(ctx, issue.ID, tc.id, issue, "test"); err != nil {
-			t.Fatalf("failed to set issue ID to %s: %v", tc.id, err)
-		}
 	}

 	// Add a dependency to test that it gets updated
--- a/internal/storage/sqlite/hash_id_test.go
+++ b/internal/storage/sqlite/hash_id_test.go
@@ -17,10 +17,13 @@ func TestHashIDGeneration(t *testing.T) {

 	ctx := context.Background()

-	// Set up database with prefix
+	// Set up database with prefix and hash mode
 	if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil {
 		t.Fatalf("Failed to set prefix: %v", err)
 	}
+	if err := store.SetConfig(ctx, "id_mode", "hash"); err != nil {
+		t.Fatalf("Failed to set id_mode: %v", err)
+	}

 	// Create an issue - should get a hash ID
 	issue := &types.Issue{
@@ -139,10 +142,13 @@ func TestHashIDBatchCreation(t *testing.T) {

 	ctx := context.Background()

-	// Set up database with prefix
+	// Set up database with prefix and hash mode
 	if err := store.SetConfig(ctx, "issue_prefix", "bd"); err != nil {
 		t.Fatalf("Failed to set prefix: %v", err)
 	}
+	if err := store.SetConfig(ctx, "id_mode", "hash"); err != nil {
+		t.Fatalf("Failed to set id_mode: %v", err)
+	}

 	// Create multiple issues with similar content
 	issues := []*types.Issue{
--- a/internal/storage/sqlite/sqlite.go
+++ b/internal/storage/sqlite/sqlite.go
@@ -737,6 +737,44 @@ func (s *SQLiteStorage) SyncAllCounters(ctx context.Context) error {
 // The database should ALWAYS have issue_prefix config set explicitly (by 'bd init' or auto-import)
 // Never derive prefix from filename - it leads to silent data corruption

+// getIDMode returns the ID generation mode from config (sequential or hash).
+// Defaults to "sequential" for backward compatibility if not set.
+func getIDMode(ctx context.Context, conn *sql.Conn) string {
+	var mode string
+	err := conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "id_mode").Scan(&mode)
+	if err != nil || mode == "" {
+		return "sequential" // Default to sequential for backward compatibility
+	}
+	return mode
+}
+
+// nextSequentialID atomically increments and returns the next sequential ID number.
+// Must be called inside an IMMEDIATE transaction on the same connection.
+// Implements lazy initialization: if counter doesn't exist, initializes from existing issues.
+func nextSequentialID(ctx context.Context, conn *sql.Conn, prefix string) (int, error) {
+	var nextID int
+	
+	// The query handles three cases atomically:
+	// 1. Counter doesn't exist: initialize from MAX(existing IDs) or 1, then return that + 1
+	// 2. Counter exists but lower than max ID: update to max and return max + 1
+	// 3. Counter exists and correct: just increment and return next ID
+	err := conn.QueryRowContext(ctx, `
+		INSERT INTO issue_counters (prefix, last_id)
+		SELECT ?, COALESCE(MAX(CAST(substr(id, LENGTH(?) + 2) AS INTEGER)), 0) + 1
+		FROM issues
+		WHERE id LIKE ? || '-%'
+		  AND substr(id, LENGTH(?) + 2) GLOB '[0-9]*'
+		  AND instr(substr(id, LENGTH(?) + 2), '.') = 0
+		ON CONFLICT(prefix) DO UPDATE SET
+			last_id = last_id + 1
+		RETURNING last_id
+	`, prefix, prefix, prefix, prefix, prefix).Scan(&nextID)
+	if err != nil {
+		return 0, fmt.Errorf("failed to generate next sequential ID for prefix %s: %w", prefix, err)
+	}
+	return nextID, nil
+}
+
 // generateHashID creates a hash-based ID for a top-level issue.
 // For child issues, use the parent ID with a numeric suffix (e.g., "bd-a3f8e9a2.1").
 // Includes a nonce parameter to handle collisions.
@@ -813,27 +851,39 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act

 	// Generate ID if not set (inside transaction to prevent race conditions)
 	if issue.ID == "" {
-		// Generate hash-based ID with collision detection (bd-168)
-		// Try up to 10 times with different nonces to avoid collisions
-		var err error
-		for nonce := 0; nonce < 10; nonce++ {
-			candidate := generateHashID(prefix, issue.Title, issue.Description, actor, issue.CreatedAt, nonce)
-			
-			// Check if this ID already exists
-			var count int
-			err = conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count)
-			if err != nil {
-				return fmt.Errorf("failed to check for ID collision: %w", err)
-			}
-			
-			if count == 0 {
-				issue.ID = candidate
-				break
-			}
-		}
+		// Check id_mode config to determine ID generation strategy
+		idMode := getIDMode(ctx, conn)
 		
-		if issue.ID == "" {
-			return fmt.Errorf("failed to generate unique ID after 10 attempts")
+		if idMode == "hash" {
+			// Generate hash-based ID with collision detection (bd-168)
+			// Try up to 10 times with different nonces to avoid collisions
+			var err error
+			for nonce := 0; nonce < 10; nonce++ {
+				candidate := generateHashID(prefix, issue.Title, issue.Description, actor, issue.CreatedAt, nonce)
+				
+				// Check if this ID already exists
+				var count int
+				err = conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count)
+				if err != nil {
+					return fmt.Errorf("failed to check for ID collision: %w", err)
+				}
+				
+				if count == 0 {
+					issue.ID = candidate
+					break
+				}
+			}
+			
+			if issue.ID == "" {
+				return fmt.Errorf("failed to generate unique ID after 10 attempts")
+			}
+		} else {
+			// Default: generate sequential ID using counter
+			nextID, err := nextSequentialID(ctx, conn, prefix)
+			if err != nil {
+				return err
+			}
+			issue.ID = fmt.Sprintf("%s-%d", prefix, nextID)
 		}
 	} else {
 		// Validate that explicitly provided ID matches the configured prefix (bd-177)
@@ -947,7 +997,10 @@ func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue
 		return fmt.Errorf("failed to get config: %w", err)
 	}

-	// Validate explicitly provided IDs and generate hash IDs for those that need them
+	// Check id_mode config to determine ID generation strategy
+	idMode := getIDMode(ctx, conn)
+
+	// Validate explicitly provided IDs and generate IDs for those that need them
 	expectedPrefix := prefix + "-"
 	usedIDs := make(map[string]bool)
 	
@@ -962,39 +1015,55 @@ func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue
 		}
 	}
 	
-	// Second pass: generate IDs for issues that need them, with collision detection
-	for i := range issues {
-		if issues[i].ID == "" {
-			// Generate hash-based ID with collision detection (bd-168)
-			var generated bool
-			for nonce := 0; nonce < 10; nonce++ {
-				candidate := generateHashID(prefix, issues[i].Title, issues[i].Description, actor, issues[i].CreatedAt, nonce)
-				
-				// Check if this ID is already used in this batch or in the database
-				if usedIDs[candidate] {
-					continue
+	// Second pass: generate IDs for issues that need them
+	if idMode == "hash" {
+		// Hash mode: generate with collision detection
+		for i := range issues {
+			if issues[i].ID == "" {
+				var generated bool
+				for nonce := 0; nonce < 10; nonce++ {
+					candidate := generateHashID(prefix, issues[i].Title, issues[i].Description, actor, issues[i].CreatedAt, nonce)
+					
+					// Check if this ID is already used in this batch or in the database
+					if usedIDs[candidate] {
+						continue
+					}
+					
+					var count int
+					err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count)
+					if err != nil {
+						return fmt.Errorf("failed to check for ID collision: %w", err)
+					}
+					
+					if count == 0 {
+						issues[i].ID = candidate
+						usedIDs[candidate] = true
+						generated = true
+						break
+					}
 				}
 				
-				var count int
-				err := conn.QueryRowContext(ctx, `SELECT COUNT(*) FROM issues WHERE id = ?`, candidate).Scan(&count)
-				if err != nil {
-					return fmt.Errorf("failed to check for ID collision: %w", err)
+				if !generated {
+					return fmt.Errorf("failed to generate unique ID for issue %d after 10 attempts", i)
 				}
-				
-				if count == 0 {
-					issues[i].ID = candidate
-					usedIDs[candidate] = true
-					generated = true
-					break
-				}
-			}
-			
-			if !generated {
-				return fmt.Errorf("failed to generate unique ID for issue %d after 10 attempts", i)
 			}
 		}
-		
-		// Compute content hash if not already set (bd-95)
+	} else {
+		// Sequential mode: allocate sequential IDs for all issues that need them
+		for i := range issues {
+			if issues[i].ID == "" {
+				nextID, err := nextSequentialID(ctx, conn, prefix)
+				if err != nil {
+					return fmt.Errorf("failed to generate sequential ID for issue %d: %w", i, err)
+				}
+				issues[i].ID = fmt.Sprintf("%s-%d", prefix, nextID)
+				usedIDs[issues[i].ID] = true
+			}
+		}
+	}
+	
+	// Compute content hashes
+	for i := range issues {
 		if issues[i].ContentHash == "" {
 			issues[i].ContentHash = issues[i].ComputeContentHash()
 		}