From 1fbfe58ba755aee33053a2d418e504afd7648593 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 30 Oct 2025 16:50:38 -0700 Subject: [PATCH] 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. --- .beads/beads.jsonl | 2 + cmd/bd/migrate.go | 16 +++ cmd/bd/renumber_test.go | 5 +- internal/storage/sqlite/hash_id_test.go | 10 +- internal/storage/sqlite/sqlite.go | 167 +++++++++++++++++------- 5 files changed, 145 insertions(+), 55 deletions(-) diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index 48b10543..ca67baa6 100644 --- 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"} diff --git a/cmd/bd/migrate.go b/cmd/bd/migrate.go index fb0bacab..13d1ec2e 100644 --- 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 { diff --git a/cmd/bd/renumber_test.go b/cmd/bd/renumber_test.go index d16be68f..11f2b97f 100644 --- 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 diff --git a/internal/storage/sqlite/hash_id_test.go b/internal/storage/sqlite/hash_id_test.go index 9a7cc868..7ce20eaf 100644 --- 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{ diff --git a/internal/storage/sqlite/sqlite.go b/internal/storage/sqlite/sqlite.go index 0f48298b..90dc67d0 100644 --- 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() }