Centralize error handling patterns in storage layer (bd-bwk2)

Standardized error handling across the SQLite storage layer by
consistently using wrapDBError() helper functions that were already
defined in errors.go.

Changes:
- config.go: Applied wrapDBError to all config/metadata functions
- queries.go: Fixed bare 'return err' in CreateIssue, UpdateIssue, DeleteIssues
- store.go: Changed %v to %w for proper error chain preservation
- errors_test.go: Added comprehensive test coverage for error wrapping

All error paths now:
- Wrap errors with operation context using %w
- Convert sql.ErrNoRows to ErrNotFound consistently
- Preserve error chains for unwrapping and type checking

This improves debugging by maintaining operation context throughout
the error chain and enables type-safe error checking with sentinel
errors.

All tests passing ✓

🤖 Generated with [Claude Code](https://claude.com/claude-code)

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

internal/storage/sqlite/config.go

@@ -11,7 +11,7 @@ func (s *SQLiteStorage) SetConfig(ctx context.Context, key, value string) error
 		INSERT INTO config (key, value) VALUES (?, ?)
 		ON CONFLICT (key) DO UPDATE SET value = excluded.value
 	`, key, value)
-	return err
+	return wrapDBError("set config", err)
 }
 
 // GetConfig gets a configuration value
@@ -21,14 +21,14 @@ func (s *SQLiteStorage) GetConfig(ctx context.Context, key string) (string, erro
 	if err == sql.ErrNoRows {
 		return "", nil
 	}
-	return value, err
+	return value, wrapDBError("get config", err)
 }
 
 // GetAllConfig gets all configuration key-value pairs
 func (s *SQLiteStorage) GetAllConfig(ctx context.Context) (map[string]string, error) {
 	rows, err := s.db.QueryContext(ctx, `SELECT key, value FROM config ORDER BY key`)
 	if err != nil {
-		return nil, err
+		return nil, wrapDBError("query all config", err)
 	}
 	defer func() { _ = rows.Close() }()
 
@@ -36,17 +36,17 @@ func (s *SQLiteStorage) GetAllConfig(ctx context.Context) (map[string]string, er
 	for rows.Next() {
 		var key, value string
 		if err := rows.Scan(&key, &value); err != nil {
-			return nil, err
+			return nil, wrapDBError("scan config row", err)
 		}
 		config[key] = value
 	}
-	return config, rows.Err()
+	return config, wrapDBError("iterate config rows", rows.Err())
 }
 
 // DeleteConfig deletes a configuration value
 func (s *SQLiteStorage) DeleteConfig(ctx context.Context, key string) error {
 	_, err := s.db.ExecContext(ctx, `DELETE FROM config WHERE key = ?`, key)
-	return err
+	return wrapDBError("delete config", err)
 }
 
 // OrphanHandling defines how to handle orphan issues during import
@@ -81,7 +81,7 @@ func (s *SQLiteStorage) SetMetadata(ctx context.Context, key, value string) erro
 		INSERT INTO metadata (key, value) VALUES (?, ?)
 		ON CONFLICT (key) DO UPDATE SET value = excluded.value
 	`, key, value)
-	return err
+	return wrapDBError("set metadata", err)
 }
 
 // GetMetadata gets a metadata value (for internal state like import hashes)
@@ -91,5 +91,5 @@ func (s *SQLiteStorage) GetMetadata(ctx context.Context, key string) (string, er
 	if err == sql.ErrNoRows {
 		return "", nil
 	}
-	return value, err
+	return value, wrapDBError("get metadata", err)
 }

internal/storage/sqlite/errors_test.go

@@ -0,0 +1,248 @@
+package sqlite
+
+import (
+	"database/sql"
+	"errors"
+	"fmt"
+	"testing"
+)
+
+// TestWrapDBError tests the wrapDBError function
+func TestWrapDBError(t *testing.T) {
+	tests := []struct {
+		name      string
+		op        string
+		err       error
+		wantNil   bool
+		wantError string
+		wantType  error
+	}{
+		{
+			name:    "nil error returns nil",
+			op:      "test operation",
+			err:     nil,
+			wantNil: true,
+		},
+		{
+			name:      "sql.ErrNoRows converted to ErrNotFound",
+			op:        "get issue",
+			err:       sql.ErrNoRows,
+			wantError: "get issue: not found",
+			wantType:  ErrNotFound,
+		},
+		{
+			name:      "generic error wrapped with context",
+			op:        "update issue",
+			err:       errors.New("database locked"),
+			wantError: "update issue: database locked",
+		},
+		{
+			name:      "already wrapped error preserved",
+			op:        "delete issue",
+			err:       fmt.Errorf("constraint violation: %w", ErrConflict),
+			wantError: "delete issue: constraint violation: conflict",
+			wantType:  ErrConflict,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := wrapDBError(tt.op, tt.err)
+
+			if tt.wantNil {
+				if result != nil {
+					t.Errorf("wrapDBError() = %v, want nil", result)
+				}
+				return
+			}
+
+			if result == nil {
+				t.Fatal("wrapDBError() returned nil, want error")
+			}
+
+			if tt.wantError != "" && result.Error() != tt.wantError {
+				t.Errorf("wrapDBError() error = %q, want %q", result.Error(), tt.wantError)
+			}
+
+			if tt.wantType != nil && !errors.Is(result, tt.wantType) {
+				t.Errorf("wrapDBError() error doesn't wrap %v", tt.wantType)
+			}
+		})
+	}
+}
+
+// TestWrapDBErrorf tests the wrapDBErrorf function
+func TestWrapDBErrorf(t *testing.T) {
+	tests := []struct {
+		name      string
+		err       error
+		format    string
+		args      []interface{}
+		wantNil   bool
+		wantError string
+		wantType  error
+	}{
+		{
+			name:    "nil error returns nil",
+			err:     nil,
+			format:  "operation %s on %s",
+			args:    []interface{}{"update", "issue-123"},
+			wantNil: true,
+		},
+		{
+			name:      "sql.ErrNoRows converted to ErrNotFound with formatting",
+			err:       sql.ErrNoRows,
+			format:    "get issue %s",
+			args:      []interface{}{"bd-abc"},
+			wantError: "get issue bd-abc: not found",
+			wantType:  ErrNotFound,
+		},
+		{
+			name:      "generic error with formatted context",
+			err:       errors.New("timeout"),
+			format:    "query %s with filter %s",
+			args:      []interface{}{"issues", "status=open"},
+			wantError: "query issues with filter status=open: timeout",
+		},
+		{
+			name:      "multiple format args",
+			err:       errors.New("invalid value"),
+			format:    "update %s field %s to %v",
+			args:      []interface{}{"issue-123", "priority", 1},
+			wantError: "update issue-123 field priority to 1: invalid value",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result := wrapDBErrorf(tt.err, tt.format, tt.args...)
+
+			if tt.wantNil {
+				if result != nil {
+					t.Errorf("wrapDBErrorf() = %v, want nil", result)
+				}
+				return
+			}
+
+			if result == nil {
+				t.Fatal("wrapDBErrorf() returned nil, want error")
+			}
+
+			if tt.wantError != "" && result.Error() != tt.wantError {
+				t.Errorf("wrapDBErrorf() error = %q, want %q", result.Error(), tt.wantError)
+			}
+
+			if tt.wantType != nil && !errors.Is(result, tt.wantType) {
+				t.Errorf("wrapDBErrorf() error doesn't wrap %v", tt.wantType)
+			}
+		})
+	}
+}
+
+// TestSentinelErrors tests the sentinel error constants
+func TestSentinelErrors(t *testing.T) {
+	tests := []struct {
+		name  string
+		err   error
+		check func(error) bool
+		want  bool
+	}{
+		{
+			name:  "ErrNotFound detected by IsNotFound",
+			err:   ErrNotFound,
+			check: IsNotFound,
+			want:  true,
+		},
+		{
+			name:  "wrapped ErrNotFound detected",
+			err:   fmt.Errorf("get issue: %w", ErrNotFound),
+			check: IsNotFound,
+			want:  true,
+		},
+		{
+			name:  "other error not detected as ErrNotFound",
+			err:   errors.New("other error"),
+			check: IsNotFound,
+			want:  false,
+		},
+		{
+			name:  "ErrConflict detected by IsConflict",
+			err:   ErrConflict,
+			check: IsConflict,
+			want:  true,
+		},
+		{
+			name:  "wrapped ErrConflict detected",
+			err:   fmt.Errorf("unique constraint: %w", ErrConflict),
+			check: IsConflict,
+			want:  true,
+		},
+		{
+			name:  "ErrCycle detected by IsCycle",
+			err:   ErrCycle,
+			check: IsCycle,
+			want:  true,
+		},
+		{
+			name:  "wrapped ErrCycle detected",
+			err:   fmt.Errorf("dependency check: %w", ErrCycle),
+			check: IsCycle,
+			want:  true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := tt.check(tt.err)
+			if got != tt.want {
+				t.Errorf("check(%v) = %v, want %v", tt.err, got, tt.want)
+			}
+		})
+	}
+}
+
+// TestErrorChaining tests that error chains are preserved through operations
+func TestErrorChaining(t *testing.T) {
+	// Create a chain: root -> middle -> top
+	root := errors.New("root cause")
+	middle := fmt.Errorf("middle layer: %w", root)
+	top := wrapDBError("top operation", middle)
+
+	// Verify we can unwrap to each level
+	if !errors.Is(top, middle) {
+		t.Error("top error doesn't wrap middle error")
+	}
+	if !errors.Is(top, root) {
+		t.Error("top error doesn't wrap root error")
+	}
+
+	// Verify error message includes all context
+	want := "top operation: middle layer: root cause"
+	if top.Error() != want {
+		t.Errorf("error message = %q, want %q", top.Error(), want)
+	}
+}
+
+// TestSQLErrNoRowsConversion tests that sql.ErrNoRows is consistently converted
+func TestSQLErrNoRowsConversion(t *testing.T) {
+	// Both wrapping functions should convert sql.ErrNoRows to ErrNotFound
+	err1 := wrapDBError("get config", sql.ErrNoRows)
+	err2 := wrapDBErrorf(sql.ErrNoRows, "get metadata %s", "key")
+
+	if !IsNotFound(err1) {
+		t.Error("wrapDBError didn't convert sql.ErrNoRows to ErrNotFound")
+	}
+	if !IsNotFound(err2) {
+		t.Error("wrapDBErrorf didn't convert sql.ErrNoRows to ErrNotFound")
+	}
+
+	// The conversion replaces sql.ErrNoRows with ErrNotFound (not wrapped together)
+	// This is intentional - we want a single, clean error type for "not found" conditions
+	// The error message should indicate the operation context
+	if err1.Error() != "get config: not found" {
+		t.Errorf("err1 message = %q, want %q", err1.Error(), "get config: not found")
+	}
+	if err2.Error() != "get metadata key: not found" {
+		t.Errorf("err2 message = %q, want %q", err2.Error(), "get metadata key: not found")
+	}
+}

internal/storage/sqlite/queries.go

@@ -87,13 +87,13 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
 		// Generate hash-based ID with adaptive length based on database size (bd-ea2a13)
 		generatedID, err := GenerateIssueID(ctx, conn, prefix, issue, actor)
 		if err != nil {
-			return err
+			return wrapDBError("generate issue ID", err)
 		}
 		issue.ID = generatedID
 	} else {
 		// Validate that explicitly provided ID matches the configured prefix (bd-177)
 		if err := ValidateIssueIDPrefix(issue.ID, prefix); err != nil {
-			return err
+			return wrapDBError("validate issue ID prefix", err)
 		}
 
 		// For hierarchical IDs (bd-a3f8e9.1), ensure parent exists
@@ -115,17 +115,17 @@ func (s *SQLiteStorage) CreateIssue(ctx context.Context, issue *types.Issue, act
 
 	// Insert issue
 	if err := insertIssue(ctx, conn, issue); err != nil {
-		return err
+		return wrapDBError("insert issue", err)
 	}
 
 	// Record creation event
 	if err := recordCreatedEvent(ctx, conn, issue, actor); err != nil {
-		return err
+		return wrapDBError("record creation event", err)
 	}
 
 	// Mark issue as dirty for incremental export
 	if err := markDirty(ctx, conn, issue.ID); err != nil {
-		return err
+		return wrapDBError("mark issue dirty", err)
 	}
 
 	// Commit the transaction
@@ -370,7 +370,7 @@ func (s *SQLiteStorage) UpdateIssue(ctx context.Context, id string, updates map[
 	// Get old issue for event
 	oldIssue, err := s.GetIssue(ctx, id)
 	if err != nil {
-		return err
+		return wrapDBError("get issue for update", err)
 	}
 	if oldIssue == nil {
 		return fmt.Errorf("issue %s not found", id)
@@ -388,7 +388,7 @@ func (s *SQLiteStorage) UpdateIssue(ctx context.Context, id string, updates map[
 
 		// Validate field values
 		if err := validateFieldUpdate(key, value); err != nil {
-			return err
+			return wrapDBError("validate field update", err)
 		}
 
 		setClauses = append(setClauses, fmt.Sprintf("%s = ?", key))
@@ -740,7 +740,7 @@ func (s *SQLiteStorage) DeleteIssue(ctx context.Context, id string) error {
 	}
 
 	if err := tx.Commit(); err != nil {
-		return err
+		return wrapDBError("commit delete transaction", err)
 	}
 
 	// REMOVED (bd-c7af): Counter sync after deletion - no longer needed with hash IDs
@@ -777,7 +777,7 @@ func (s *SQLiteStorage) DeleteIssues(ctx context.Context, ids []string, cascade
 
 	expandedIDs, err := s.resolveDeleteSet(ctx, tx, ids, idSet, cascade, force, result)
 	if err != nil {
-		return nil, err
+		return nil, wrapDBError("resolve delete set", err)
 	}
 
 	inClause, args := buildSQLInClause(expandedIDs)
@@ -835,7 +835,7 @@ func (s *SQLiteStorage) expandWithDependents(ctx context.Context, tx *sql.Tx, id
 func (s *SQLiteStorage) validateNoDependents(ctx context.Context, tx *sql.Tx, ids []string, idSet map[string]bool, result *DeleteIssuesResult) error {
 	for _, id := range ids {
 		if err := s.checkSingleIssueValidation(ctx, tx, id, idSet, result); err != nil {
-			return err
+			return wrapDBError("check dependents", err)
 		}
 	}
 	return nil
@@ -885,7 +885,7 @@ func (s *SQLiteStorage) trackOrphanedIssues(ctx context.Context, tx *sql.Tx, ids
 	orphanSet := make(map[string]bool)
 	for _, id := range ids {
 		if err := s.collectOrphansForID(ctx, tx, id, idSet, orphanSet); err != nil {
-			return err
+			return wrapDBError("collect orphans", err)
 		}
 	}
 	for orphanID := range orphanSet {

internal/storage/sqlite/store.go

@@ -149,7 +149,7 @@ func New(ctx context.Context, path string) (*SQLiteStorage, error) {
 	if err := verifySchemaCompatibility(db); err != nil {
 		// Schema probe failed - retry migrations once
 		if retryErr := RunMigrations(db); retryErr != nil {
-			return nil, fmt.Errorf("migration retry failed after schema probe failure: %w (original: %v)", retryErr, err)
+			return nil, fmt.Errorf("migration retry failed after schema probe failure: %w (original: %w)", retryErr, err)
 		}
 
 		// Probe again after retry