Complete bd-175, bd-176, bd-177: Memory tests, corruption docs, prefix validation

- bd-175: Added comprehensive test coverage for internal/storage/memory backend - All CRUD operations, dependencies, labels, comments - Thread safety with race detection - LoadFromIssues and counter sync - Fixed batch duplicate detection - bd-176: Documented corruption vs collision distinction - Added FAQ entry explaining logical vs physical corruption - Updated TROUBLESHOOTING with clear guidance - Clarified when to use collision resolution vs reimport - bd-177: Added prefix validation in SQLite mode - Validates explicit IDs match configured prefix - Works in both CreateIssue and CreateIssues - Comprehensive tests for single and batch operations
2025-10-27 11:29:08 -07:00
parent f5e1a9811a
commit 68ffb9ed40
6 changed files with 1171 additions and 27 deletions
--- a/internal/storage/sqlite/prefix_validation_test.go
+++ b/internal/storage/sqlite/prefix_validation_test.go
@@ -0,0 +1,165 @@
+package sqlite
+
+import (
+	"context"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/steveyegge/beads/internal/types"
+)
+
+func TestPrefixValidation(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "beads-prefix-test-*")
+	if err != nil {
+		t.Fatalf("failed to create temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	dbPath := filepath.Join(tmpDir, "test.db")
+	store, err := New(dbPath)
+	if err != nil {
+		t.Fatalf("failed to create storage: %v", err)
+	}
+	defer store.Close()
+
+	ctx := context.Background()
+
+	// Set prefix to "test"
+	if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil {
+		t.Fatalf("failed to set prefix: %v", err)
+	}
+
+	tests := []struct {
+		name    string
+		issueID string
+		wantErr bool
+	}{
+		{
+			name:    "valid prefix - matches",
+			issueID: "test-123",
+			wantErr: false,
+		},
+		{
+			name:    "invalid prefix - wrong prefix",
+			issueID: "bd-456",
+			wantErr: true,
+		},
+		{
+			name:    "invalid prefix - no dash",
+			issueID: "test123",
+			wantErr: true,
+		},
+		{
+			name:    "invalid prefix - empty",
+			issueID: "",
+			wantErr: false, // Empty ID triggers auto-generation
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			issue := &types.Issue{
+				ID:        tt.issueID,
+				Title:     "Test issue",
+				Status:    types.StatusOpen,
+				Priority:  1,
+				IssueType: types.TypeTask,
+			}
+
+			err := store.CreateIssue(ctx, issue, "test-user")
+			if (err != nil) != tt.wantErr {
+				t.Errorf("CreateIssue() error = %v, wantErr %v", err, tt.wantErr)
+			}
+
+			// If we expected success and the ID was empty, verify it was generated with correct prefix
+			if err == nil && tt.issueID == "" {
+				if issue.ID == "" {
+					t.Error("ID should be generated")
+				}
+				if issue.ID[:5] != "test-" {
+					t.Errorf("Generated ID should have prefix 'test-', got %s", issue.ID)
+				}
+			}
+		})
+	}
+}
+
+func TestPrefixValidationBatch(t *testing.T) {
+	tmpDir, err := os.MkdirTemp("", "beads-prefix-batch-test-*")
+	if err != nil {
+		t.Fatalf("failed to create temp dir: %v", err)
+	}
+	defer os.RemoveAll(tmpDir)
+
+	dbPath := filepath.Join(tmpDir, "test.db")
+	store, err := New(dbPath)
+	if err != nil {
+		t.Fatalf("failed to create storage: %v", err)
+	}
+	defer store.Close()
+
+	ctx := context.Background()
+
+	// Set prefix to "batch"
+	if err := store.SetConfig(ctx, "issue_prefix", "batch"); err != nil {
+		t.Fatalf("failed to set prefix: %v", err)
+	}
+
+	tests := []struct {
+		name    string
+		issues  []*types.Issue
+		wantErr bool
+	}{
+		{
+			name: "all valid prefixes",
+			issues: []*types.Issue{
+				{ID: "batch-1", Title: "Test 1", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+				{ID: "batch-2", Title: "Test 2", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+			},
+			wantErr: false,
+		},
+		{
+			name: "one invalid prefix in batch",
+			issues: []*types.Issue{
+				{ID: "batch-10", Title: "Test 1", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+				{ID: "wrong-20", Title: "Test 2", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+			},
+			wantErr: true,
+		},
+		{
+			name: "mixed auto-generated and explicit",
+			issues: []*types.Issue{
+				{ID: "batch-100", Title: "Explicit ID", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+				{ID: "", Title: "Auto ID", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+			},
+			wantErr: false,
+		},
+		{
+			name: "mixed with invalid prefix",
+			issues: []*types.Issue{
+				{ID: "", Title: "Auto ID", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+				{ID: "invalid-500", Title: "Wrong prefix", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeTask},
+			},
+			wantErr: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := store.CreateIssues(ctx, tt.issues, "test-user")
+			if (err != nil) != tt.wantErr {
+				t.Errorf("CreateIssues() error = %v, wantErr %v", err, tt.wantErr)
+			}
+
+			// For successful batches, verify all IDs have correct prefix
+			if err == nil {
+				for i, issue := range tt.issues {
+					if issue.ID[:6] != "batch-" {
+						t.Errorf("Issue %d: ID should have prefix 'batch-', got %s", i, issue.ID)
+					}
+				}
+			}
+		})
+	}
+}
--- a/internal/storage/sqlite/sqlite.go
+++ b/internal/storage/sqlite/sqlite.go
@@ -617,19 +617,19 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
 		}
 	}()

+	// Get prefix from config (needed for both ID generation and validation)
+	var prefix string
+	err = conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "issue_prefix").Scan(&prefix)
+	if err == sql.ErrNoRows || prefix == "" {
+		// CRITICAL: Reject operation if issue_prefix config is missing (bd-166)
+		// This prevents duplicate issues with wrong prefix
+		return fmt.Errorf("database not initialized: issue_prefix config is missing (run 'bd init --prefix <prefix>' first)")
+	} else if err != nil {
+		return fmt.Errorf("failed to get config: %w", err)
+	}
+
 	// Generate ID if not set (inside transaction to prevent race conditions)
 	if issue.ID == "" {
-		// Get prefix from config
-		var prefix string
-		err := conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "issue_prefix").Scan(&prefix)
-		if err == sql.ErrNoRows || prefix == "" {
-			// CRITICAL: Reject operation if issue_prefix config is missing (bd-166)
-			// This prevents duplicate issues with wrong prefix
-			return fmt.Errorf("database not initialized: issue_prefix config is missing (run 'bd init --prefix <prefix>' first)")
-		} else if err != nil {
-			return fmt.Errorf("failed to get config: %w", err)
-		}
-
 		// Atomically initialize counter (if needed) and get next ID (within transaction)
 		// This ensures the counter starts from the max existing ID, not 1
 		// CRITICAL: We rely on BEGIN IMMEDIATE above to serialize this operation across processes
@@ -665,6 +665,13 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
 		}

 		issue.ID = fmt.Sprintf("%s-%d", prefix, nextID)
+	} else {
+		// Validate that explicitly provided ID matches the configured prefix (bd-177)
+		// This prevents wrong-prefix bugs when IDs are manually specified
+		expectedPrefix := prefix + "-"
+		if !strings.HasPrefix(issue.ID, expectedPrefix) {
+			return fmt.Errorf("issue ID '%s' does not match configured prefix '%s'", issue.ID, prefix)
+		}
 	}

 	// Insert issue
@@ -743,19 +750,7 @@ func validateBatchIssues(issues []*types.Issue) error {

 // generateBatchIDs generates IDs for all issues that need them atomically
 func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue, dbPath string) error {
-	// Count how many issues need IDs
-	needIDCount := 0
-	for _, issue := range issues {
-		if issue.ID == "" {
-			needIDCount++
-		}
-	}
-
-	if needIDCount == 0 {
-		return nil
-	}
-
-	// Get prefix from config
+	// Get prefix from config (needed for both generation and validation)
 	var prefix string
 	err := conn.QueryRowContext(ctx, `SELECT value FROM config WHERE key = ?`, "issue_prefix").Scan(&prefix)
 	if err == sql.ErrNoRows || prefix == "" {
@@ -765,6 +760,24 @@ func generateBatchIDs(ctx context.Context, conn *sql.Conn, issues []*types.Issue
 		return fmt.Errorf("failed to get config: %w", err)
 	}

+	// Count how many issues need IDs and validate explicitly provided IDs
+	needIDCount := 0
+	expectedPrefix := prefix + "-"
+	for _, issue := range issues {
+		if issue.ID == "" {
+			needIDCount++
+		} else {
+			// Validate that explicitly provided ID matches the configured prefix (bd-177)
+			if !strings.HasPrefix(issue.ID, expectedPrefix) {
+				return fmt.Errorf("issue ID '%s' does not match configured prefix '%s'", issue.ID, prefix)
+			}
+		}
+	}
+
+	if needIDCount == 0 {
+		return nil
+	}
+
 	// Atomically reserve ID range
 	var nextID int
 	err = conn.QueryRowContext(ctx, `