Merge pull request #584 from rsnodgrass/repair-hashes

fix(rename-prefix): use hash IDs instead of sequential in --repair mode

cmd/bd/deletion_tracking.go

@@ -5,12 +5,30 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
+	"strings"
 
 	"github.com/steveyegge/beads/internal/config"
 	"github.com/steveyegge/beads/internal/merge"
 	"github.com/steveyegge/beads/internal/storage"
 )
 
+// isIssueNotFoundError checks if the error indicates the issue doesn't exist in the database.
+//
+// During 3-way merge, we try to delete issues that were removed remotely. However, the issue
+// may already be gone from the local database due to:
+//   - Already tombstoned by a previous sync/import
+//   - Never existed locally (multi-repo scenarios, partial clones)
+//   - Deleted by user between export and import phases
+//
+// In all these cases, "issue not found" is success from the merge's perspective - the goal
+// is to ensure the issue is deleted, and it already is. We only fail on actual database errors.
+func isIssueNotFoundError(err error) bool {
+	if err == nil {
+		return false
+	}
+	return strings.Contains(err.Error(), "issue not found:")
+}
+
 // getVersion returns the current bd version
 func getVersion() string {
 	return Version
@@ -74,11 +92,26 @@ func merge3WayAndPruneDeletions(ctx context.Context, store storage.Storage, json
 		return false, fmt.Errorf("failed to compute accepted deletions: %w", err)
 	}
 
-	// Prune accepted deletions from the database
-	// Collect all deletion errors - fail the operation if any delete fails
+	// Prune accepted deletions from the database.
+	//
+	// "Accepted deletions" are issues that:
+	//   1. Existed in the base snapshot (last successful import)
+	//   2. Were NOT modified locally (still in left snapshot, unchanged)
+	//   3. Are NOT in the merged result (deleted remotely)
+	//
+	// We tolerate "issue not found" errors because the issue may already be gone:
+	//   - Tombstoned by auto-import's git-history-backfill
+	//   - Deleted manually by the user
+	//   - Never existed in this clone (multi-repo, partial history)
+	// The goal is ensuring deletion, so already-deleted is success.
 	var deletionErrors []error
+	var alreadyGone int
 	for _, id := range acceptedDeletions {
 		if err := store.DeleteIssue(ctx, id); err != nil {
+			if isIssueNotFoundError(err) {
+				alreadyGone++
+				continue
+			}
 			deletionErrors = append(deletionErrors, fmt.Errorf("issue %s: %w", id, err))
 		}
 	}
@@ -89,9 +122,15 @@ func merge3WayAndPruneDeletions(ctx context.Context, store storage.Storage, json
 
 	// Print stats if deletions were found
 	stats := sm.GetStats()
-	if stats.DeletionsFound > 0 {
-		fmt.Fprintf(os.Stderr, "3-way merge: pruned %d deleted issue(s) from database (base: %d, left: %d, merged: %d)\n",
-			stats.DeletionsFound, stats.BaseCount, stats.LeftCount, stats.MergedCount)
+	actuallyDeleted := len(acceptedDeletions) - alreadyGone
+	if stats.DeletionsFound > 0 || alreadyGone > 0 {
+		if alreadyGone > 0 {
+			fmt.Fprintf(os.Stderr, "3-way merge: pruned %d deleted issue(s) from database, %d already gone (base: %d, left: %d, merged: %d)\n",
+				actuallyDeleted, alreadyGone, stats.BaseCount, stats.LeftCount, stats.MergedCount)
+		} else {
+			fmt.Fprintf(os.Stderr, "3-way merge: pruned %d deleted issue(s) from database (base: %d, left: %d, merged: %d)\n",
+				actuallyDeleted, stats.BaseCount, stats.LeftCount, stats.MergedCount)
+		}
 	}
 
 	return true, nil

cmd/bd/rename_prefix.go

@@ -2,16 +2,19 @@ package main
 
 import (
 	"context"
+	"database/sql"
 	"encoding/json"
 	"fmt"
 	"os"
 	"regexp"
 	"sort"
 	"strings"
+	"time"
 
 	"github.com/fatih/color"
 	"github.com/spf13/cobra"
 	"github.com/steveyegge/beads/internal/storage"
+	"github.com/steveyegge/beads/internal/storage/sqlite"
 	"github.com/steveyegge/beads/internal/types"
 	"github.com/steveyegge/beads/internal/utils"
 )
@@ -225,7 +228,7 @@ type issueSort struct {
 
 // repairPrefixes consolidates multiple prefixes into a single target prefix
 // Issues with the correct prefix are left unchanged.
-// Issues with incorrect prefixes are sorted and renumbered sequentially.
+// Issues with incorrect prefixes get new hash-based IDs.
 func repairPrefixes(ctx context.Context, st storage.Storage, actorName string, targetPrefix string, issues []*types.Issue, prefixes map[string]int, dryRun bool) error {
 	green := color.New(color.FgGreen).SprintFunc()
 	cyan := color.New(color.FgCyan).SprintFunc()
@@ -235,16 +238,12 @@ func repairPrefixes(ctx context.Context, st storage.Storage, actorName string, t
 	var correctIssues []*types.Issue
 	var incorrectIssues []issueSort
 
-	maxCorrectNumber := 0
 	for _, issue := range issues {
 		prefix := utils.ExtractIssuePrefix(issue.ID)
 		number := utils.ExtractIssueNumber(issue.ID)
 
 		if prefix == targetPrefix {
 			correctIssues = append(correctIssues, issue)
-			if number > maxCorrectNumber {
-				maxCorrectNumber = number
-			}
 		} else {
 			incorrectIssues = append(incorrectIssues, issueSort{
 				issue:  issue,
@@ -262,43 +261,58 @@ func repairPrefixes(ctx context.Context, st storage.Storage, actorName string, t
 		return incorrectIssues[i].number < incorrectIssues[j].number
 	})
 
+	// Get a database connection for ID generation
+	conn, err := st.UnderlyingConn(ctx)
+	if err != nil {
+		return fmt.Errorf("failed to get database connection: %w", err)
+	}
+	defer func() { _ = conn.Close() }()
+
+	// Build a map of all renames for text replacement using hash IDs
+	// Track used IDs to avoid collisions within the batch
+	renameMap := make(map[string]string)
+	usedIDs := make(map[string]bool)
+
+	// Mark existing correct IDs as used
+	for _, issue := range correctIssues {
+		usedIDs[issue.ID] = true
+	}
+
+	// Generate hash IDs for all incorrect issues
+	for _, is := range incorrectIssues {
+		newID, err := generateRepairHashID(ctx, conn, targetPrefix, is.issue, actorName, usedIDs)
+		if err != nil {
+			return fmt.Errorf("failed to generate hash ID for %s: %w", is.issue.ID, err)
+		}
+		renameMap[is.issue.ID] = newID
+		usedIDs[newID] = true
+	}
+
 	if dryRun {
 		fmt.Printf("DRY RUN: Would repair %d issues with incorrect prefixes\n\n", len(incorrectIssues))
-		fmt.Printf("Issues with correct prefix (%s): %d (highest number: %d)\n", cyan(targetPrefix), len(correctIssues), maxCorrectNumber)
+		fmt.Printf("Issues with correct prefix (%s): %d\n", cyan(targetPrefix), len(correctIssues))
 		fmt.Printf("Issues to repair: %d\n\n", len(incorrectIssues))
 
 		fmt.Printf("Planned renames (showing first 10):\n")
-		nextNumber := maxCorrectNumber + 1
 		for i, is := range incorrectIssues {
 			if i >= 10 {
 				fmt.Printf("... and %d more\n", len(incorrectIssues)-10)
 				break
 			}
 			oldID := is.issue.ID
-			newID := fmt.Sprintf("%s-%d", targetPrefix, nextNumber)
+			newID := renameMap[oldID]
 			fmt.Printf("  %s -> %s\n", yellow(oldID), cyan(newID))
-			nextNumber++
 		}
 		return nil
 	}
 
 	// Perform the repairs
 	fmt.Printf("Repairing database with multiple prefixes...\n")
-	fmt.Printf("  Issues with correct prefix (%s): %d (highest: %s-%d)\n",
-		cyan(targetPrefix), len(correctIssues), targetPrefix, maxCorrectNumber)
+	fmt.Printf("  Issues with correct prefix (%s): %d\n", cyan(targetPrefix), len(correctIssues))
 	fmt.Printf("  Issues to repair: %d\n\n", len(incorrectIssues))
 
-	oldPrefixPattern := regexp.MustCompile(`\b[a-z][a-z0-9-]*-(\d+)\b`)
-
-	// Build a map of all renames for text replacement
-	renameMap := make(map[string]string)
-	nextNumber := maxCorrectNumber + 1
-	for _, is := range incorrectIssues {
-		oldID := is.issue.ID
-		newID := fmt.Sprintf("%s-%d", targetPrefix, nextNumber)
-		renameMap[oldID] = newID
-		nextNumber++
-	}
+	// Pattern to match any issue ID reference in text (both hash and sequential IDs)
+	oldPrefixPattern := regexp.MustCompile(`\b[a-z][a-z0-9-]*-[a-z0-9]+\b`)
 
 	// Rename each issue
 	for _, is := range incorrectIssues {
@@ -369,11 +383,10 @@ func repairPrefixes(ctx context.Context, st storage.Storage, actorName string, t
 
 	if jsonOutput {
 		result := map[string]interface{}{
-			"target_prefix":     targetPrefix,
-			"prefixes_found":    len(prefixes),
-			"issues_repaired":   len(incorrectIssues),
-			"issues_unchanged":  len(correctIssues),
-			"highest_number":    nextNumber - 1,
+			"target_prefix":    targetPrefix,
+			"prefixes_found":   len(prefixes),
+			"issues_repaired":  len(incorrectIssues),
+			"issues_unchanged": len(correctIssues),
 		}
 		enc := json.NewEncoder(os.Stdout)
 		enc.SetIndent("", "  ")
@@ -434,6 +447,37 @@ func renamePrefixInDB(ctx context.Context, oldPrefix, newPrefix string, issues [
 	return nil
 }
 
+// generateRepairHashID generates a hash-based ID for an issue during repair
+// Uses the sqlite.GenerateIssueID function but also checks usedIDs for batch collision avoidance
+func generateRepairHashID(ctx context.Context, conn *sql.Conn, prefix string, issue *types.Issue, actor string, usedIDs map[string]bool) (string, error) {
+	// Try to generate a unique ID using the standard generation function
+	// This handles collision detection against existing database IDs
+	newID, err := sqlite.GenerateIssueID(ctx, conn, prefix, issue, actor)
+	if err != nil {
+		return "", err
+	}
+
+	// Check if this ID was already used in this batch
+	// If so, we need to generate a new one with a different timestamp
+	attempts := 0
+	for usedIDs[newID] && attempts < 100 {
+		// Slightly modify the creation time to get a different hash
+		modifiedIssue := *issue
+		modifiedIssue.CreatedAt = issue.CreatedAt.Add(time.Duration(attempts+1) * time.Nanosecond)
+		newID, err = sqlite.GenerateIssueID(ctx, conn, prefix, &modifiedIssue, actor)
+		if err != nil {
+			return "", err
+		}
+		attempts++
+	}
+
+	if usedIDs[newID] {
+		return "", fmt.Errorf("failed to generate unique ID after %d attempts", attempts)
+	}
+
+	return newID, nil
+}
+
 func init() {
 	renamePrefixCmd.Flags().Bool("dry-run", false, "Preview changes without applying them")
 	renamePrefixCmd.Flags().Bool("repair", false, "Repair database with multiple prefixes by consolidating them")

cmd/bd/rename_prefix_repair_test.go

@@ -93,19 +93,28 @@ func TestRepairMultiplePrefixes(t *testing.T) {
 		}
 	}
 
-	// Verify the others were renumbered
-	issue, err := store.GetIssue(ctx, "test-3")
-	if err != nil || issue == nil {
-		t.Fatalf("expected test-3 to exist (renamed from another-1)")
+	// Verify the others were renamed with hash IDs (not sequential)
+	// We have 5 total issues, 2 original (test-1, test-2), 3 renamed
+	if len(allIssues) != 5 {
+		t.Fatalf("expected 5 issues total, got %d", len(allIssues))
 	}
 
-	issue, err = store.GetIssue(ctx, "test-4")
-	if err != nil || issue == nil {
-		t.Fatalf("expected test-4 to exist (renamed from old-1)")
+	// Count issues with correct prefix and verify old IDs no longer exist
+	testPrefixCount := 0
+	for _, issue := range allIssues {
+		if len(issue.ID) > 5 && issue.ID[:5] == "test-" {
+			testPrefixCount++
+		}
+	}
+	if testPrefixCount != 5 {
+		t.Fatalf("expected all 5 issues to have 'test-' prefix, got %d", testPrefixCount)
 	}
 
-	issue, err = store.GetIssue(ctx, "test-5")
-	if err != nil || issue == nil {
-		t.Fatalf("expected test-5 to exist (renamed from old-2)")
+	// Verify old IDs no longer exist
+	for _, oldID := range []string{"old-1", "old-2", "another-1"} {
+		issue, err := store.GetIssue(ctx, oldID)
+		if err == nil && issue != nil {
+			t.Fatalf("expected old ID %s to no longer exist", oldID)
+		}
 	}
 }

internal/importer/importer.go

@@ -593,8 +593,18 @@ func upsertIssues(ctx context.Context, sqliteStore *sqlite.SQLiteStorage, issues
 				// Exact match (same content, same ID) - idempotent case
 				result.Unchanged++
 			} else {
-				// Same content, different ID - rename detected
-				if !opts.SkipUpdate {
+				// Same content, different ID - check if this is a rename or cross-prefix duplicate
+				existingPrefix := utils.ExtractIssuePrefix(existing.ID)
+				incomingPrefix := utils.ExtractIssuePrefix(incoming.ID)
+
+				if existingPrefix != incomingPrefix {
+					// Cross-prefix content match: same content but different projects/prefixes.
+					// This is NOT a rename - it's a duplicate from another project.
+					// Skip the incoming issue and keep the existing one unchanged.
+					// Calling handleRename would fail because CreateIssue validates prefix.
+					result.Skipped++
+				} else if !opts.SkipUpdate {
+					// Same prefix, different ID suffix - this is a true rename
 					deletedID, err := handleRename(ctx, sqliteStore, existing, incoming)
 					if err != nil {
 						return fmt.Errorf("failed to handle rename %s -> %s: %w", existing.ID, incoming.ID, err)

internal/importer/importer_test.go

@@ -524,7 +524,7 @@ func TestIsBoundary(t *testing.T) {
 	}
 }
 
-func TestIsNumeric(t *testing.T) {
+func TestIsValidIDSuffix(t *testing.T) {
 	tests := []struct {
 		s    string
 		want bool
@@ -538,18 +538,24 @@ func TestIsNumeric(t *testing.T) {
 		{"09ea", true},
 		{"abc123", true},
 		{"zzz", true},
+		// Hierarchical suffixes (hash.number format)
+		{"6we.2", true},
+		{"abc.1", true},
+		{"abc.1.2", true},
+		{"abc.1.2.3", true},
+		{"1.5", true},
 		// Invalid suffixes
-		{"", false},      // Empty string now returns false
-		{"1.5", false},   // Non-base36 characters
-		{"A3F8", false},  // Uppercase not allowed
-		{"@#$!", false},  // Special characters not allowed
+		{"", false},       // Empty string
+		{"A3F8", false},   // Uppercase not allowed
+		{"@#$!", false},   // Special characters not allowed
+		{"abc-def", false}, // Hyphens not allowed in suffix
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.s, func(t *testing.T) {
-			got := isNumeric(tt.s)
+			got := isValidIDSuffix(tt.s)
 			if got != tt.want {
-				t.Errorf("isNumeric(%q) = %v, want %v", tt.s, got, tt.want)
+				t.Errorf("isValidIDSuffix(%q) = %v, want %v", tt.s, got, tt.want)
 			}
 		})
 	}
@@ -1498,3 +1504,95 @@ func TestImportOrphanSkip_CountMismatch(t *testing.T) {
 		t.Errorf("Expected 2 normal issues in database, found %d", count)
 	}
 }
+
+// TestImportCrossPrefixContentMatch tests that importing an issue with a different prefix
+// but same content hash does NOT trigger a rename operation.
+//
+// Bug scenario:
+// 1. DB has issue "alpha-abc123" with prefix "alpha" configured
+// 2. Incoming JSONL has "beta-xyz789" with same content (same hash)
+// 3. Content hash match triggers rename detection (same content, different ID)
+// 4. handleRename tries to create "beta-xyz789" which fails prefix validation
+//
+// Expected behavior: Skip the cross-prefix "rename" and keep the existing issue unchanged.
+func TestImportCrossPrefixContentMatch(t *testing.T) {
+	ctx := context.Background()
+
+	tmpDB := t.TempDir() + "/test.db"
+	store, err := sqlite.New(context.Background(), tmpDB)
+	if err != nil {
+		t.Fatalf("Failed to create store: %v", err)
+	}
+	defer store.Close()
+
+	// Configure database with "alpha" prefix
+	if err := store.SetConfig(ctx, "issue_prefix", "alpha"); err != nil {
+		t.Fatalf("Failed to set prefix: %v", err)
+	}
+
+	// Create an issue with the configured prefix
+	existingIssue := &types.Issue{
+		ID:          "alpha-abc123",
+		Title:       "Shared Content Issue",
+		Description: "This issue has content that will match a cross-prefix import",
+		Status:      types.StatusOpen,
+		Priority:    2,
+		IssueType:   types.TypeTask,
+	}
+	if err := store.CreateIssue(ctx, existingIssue, "test-setup"); err != nil {
+		t.Fatalf("Failed to create existing issue: %v", err)
+	}
+
+	// Compute the content hash of the existing issue
+	existingHash := existingIssue.ComputeContentHash()
+
+	// Create an incoming issue with DIFFERENT prefix but SAME content
+	// This simulates importing from another project with same issue content
+	incomingIssue := &types.Issue{
+		ID:          "beta-xyz789", // Different prefix!
+		Title:       "Shared Content Issue",
+		Description: "This issue has content that will match a cross-prefix import",
+		Status:      types.StatusOpen,
+		Priority:    2,
+		IssueType:   types.TypeTask,
+	}
+
+	// Verify they have the same content hash (this is what triggers the bug)
+	incomingHash := incomingIssue.ComputeContentHash()
+	if existingHash != incomingHash {
+		t.Fatalf("Test setup error: content hashes should match. existing=%s incoming=%s", existingHash, incomingHash)
+	}
+
+	// Import the cross-prefix issue with SkipPrefixValidation (simulates auto-import behavior)
+	// This should NOT fail - cross-prefix content matches should be skipped, not renamed
+	result, err := ImportIssues(ctx, tmpDB, store, []*types.Issue{incomingIssue}, Options{
+		SkipPrefixValidation: true, // Auto-import typically sets this
+	})
+	if err != nil {
+		t.Fatalf("Import should not fail for cross-prefix content match: %v", err)
+	}
+
+	// The incoming issue should be skipped (not created, not updated)
+	// because it has a different prefix than configured
+	if result.Created != 0 {
+		t.Errorf("Expected 0 created (cross-prefix should be skipped), got %d", result.Created)
+	}
+
+	// The existing issue should remain unchanged
+	retrieved, err := store.GetIssue(ctx, "alpha-abc123")
+	if err != nil {
+		t.Fatalf("Failed to retrieve existing issue: %v", err)
+	}
+	if retrieved == nil {
+		t.Fatal("Existing issue alpha-abc123 should still exist after import")
+	}
+	if retrieved.Title != "Shared Content Issue" {
+		t.Errorf("Existing issue should be unchanged, got title: %s", retrieved.Title)
+	}
+
+	// The cross-prefix issue should NOT exist in the database
+	crossPrefix, err := store.GetIssue(ctx, "beta-xyz789")
+	if err == nil && crossPrefix != nil {
+		t.Error("Cross-prefix issue beta-xyz789 should NOT be created in the database")
+	}
+}

internal/importer/utils.go

@@ -139,7 +139,19 @@ func (fc *fieldComparator) checkFieldChanged(key string, existing *types.Issue,
 	}
 }
 
-// RenameImportedIssuePrefixes renames all issues and their references to match the target prefix
+// RenameImportedIssuePrefixes renames all issues and their references to match the target prefix.
+//
+// This function handles three ID formats:
+//   - Sequential numeric IDs: "old-123" → "new-123"
+//   - Hash-based IDs: "old-abc1" → "new-abc1"
+//   - Hierarchical IDs: "old-abc1.2.3" → "new-abc1.2.3"
+//
+// The suffix (everything after "prefix-") is preserved during rename, only the prefix changes.
+// This preserves issue identity across prefix renames while maintaining parent-child relationships
+// in hierarchical IDs (dots denote subtask nesting, e.g., bd-abc1.2 is child 2 of bd-abc1).
+//
+// All text references to old IDs in issue fields (title, description, notes, etc.) and
+// dependency relationships are updated to use the new IDs.
 func RenameImportedIssuePrefixes(issues []*types.Issue, targetPrefix string) error {
 	// Build a mapping of old IDs to new IDs
 	idMapping := make(map[string]string)
@@ -151,15 +163,15 @@ func RenameImportedIssuePrefixes(issues []*types.Issue, targetPrefix string) err
 		}
 
 		if oldPrefix != targetPrefix {
-			// Extract the numeric part
-			numPart := strings.TrimPrefix(issue.ID, oldPrefix+"-")
+			// Extract the suffix part (supports both numeric "123" and hash "abc1" and hierarchical "abc.1.2")
+			suffix := strings.TrimPrefix(issue.ID, oldPrefix+"-")
 
-			// Validate that the numeric part is actually numeric
-			if numPart == "" || !isNumeric(numPart) {
-				return fmt.Errorf("cannot rename issue %s: non-numeric suffix '%s'", issue.ID, numPart)
+			// Validate that the suffix is valid (alphanumeric + dots for hierarchy)
+			if suffix == "" || !isValidIDSuffix(suffix) {
+				return fmt.Errorf("cannot rename issue %s: invalid suffix '%s'", issue.ID, suffix)
 			}
 
-			newID := fmt.Sprintf("%s-%s", targetPrefix, numPart)
+			newID := fmt.Sprintf("%s-%s", targetPrefix, suffix)
 			idMapping[issue.ID] = newID
 		}
 	}
@@ -270,13 +282,24 @@ func isBoundary(c byte) bool {
 	return c == ' ' || c == '\t' || c == '\n' || c == '\r' || c == ',' || c == '.' || c == '!' || c == '?' || c == ':' || c == ';' || c == '(' || c == ')' || c == '[' || c == ']' || c == '{' || c == '}'
 }
 
-func isNumeric(s string) bool {
+// isValidIDSuffix validates the suffix portion of an issue ID (everything after "prefix-").
+//
+// Beads supports three ID formats, all of which this function must accept:
+//   - Sequential numeric: "123", "999" (legacy format)
+//   - Hash-based (base36): "abc1", "6we", "zzz" (current format, content-addressed)
+//   - Hierarchical: "abc1.2", "6we.2.3" (subtasks, dot-separated child counters)
+//
+// The dot separator in hierarchical IDs represents parent-child relationships:
+// "bd-abc1.2" means child #2 of parent "bd-abc1". Maximum depth is 3 levels.
+//
+// Rejected: uppercase letters, hyphens (would be confused with prefix separator),
+// and special characters.
+func isValidIDSuffix(s string) bool {
 	if len(s) == 0 {
 		return false
 	}
-	// Accept base36 characters (0-9, a-z) for hash-based suffixes
 	for _, c := range s {
-		if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z')) {
+		if !((c >= '0' && c <= '9') || (c >= 'a' && c <= 'z') || c == '.') {
 			return false
 		}
 	}