Harden issueDataChanged with type-safe comparisons

Based on code review feedback:
- Remove unsafe type assertions that could panic
- Add robust type conversion helpers (strFrom, intFrom, equalStatus, equalIssueType)
- Support multiple numeric types for priority (int, int32, int64, float64)
- Treat empty string and nil as equal for optional fields
- Add default case to detect unknown fields (conservative)
- Add comprehensive unit tests for all edge cases
- Test coverage: enums as strings, numeric conversions, nil/empty handling

Amp-Thread-ID: https://ampcode.com/threads/T-225f8c56-0710-46e9-9db2-dbf90cf91911
Co-authored-by: Amp <amp@ampcode.com>

cmd/bd/import_idempotent_test.go

@@ -12,6 +12,210 @@ import (
 	"github.com/steveyegge/beads/internal/types"
 )
 
+// TestIssueDataChanged tests the issueDataChanged helper function
+func TestIssueDataChanged(t *testing.T) {
+	tests := []struct {
+		name     string
+		existing *types.Issue
+		updates  map[string]interface{}
+		want     bool // true if changed, false if unchanged
+	}{
+		{
+			name: "no changes",
+			existing: &types.Issue{
+				Title:       "Test",
+				Description: "Desc",
+				Status:      types.StatusOpen,
+				Priority:    2,
+				IssueType:   types.TypeTask,
+			},
+			updates: map[string]interface{}{
+				"title":       "Test",
+				"description": "Desc",
+				"status":      types.StatusOpen,
+				"priority":    2,
+				"issue_type":  types.TypeTask,
+			},
+			want: false,
+		},
+		{
+			name: "title changed",
+			existing: &types.Issue{
+				Title: "Old Title",
+			},
+			updates: map[string]interface{}{
+				"title": "New Title",
+			},
+			want: true,
+		},
+		{
+			name: "status as string vs enum - unchanged",
+			existing: &types.Issue{
+				Status: types.StatusOpen,
+			},
+			updates: map[string]interface{}{
+				"status": "open", // string instead of enum
+			},
+			want: false,
+		},
+		{
+			name: "status as enum - unchanged",
+			existing: &types.Issue{
+				Status: types.StatusInProgress,
+			},
+			updates: map[string]interface{}{
+				"status": types.StatusInProgress, // enum
+			},
+			want: false,
+		},
+		{
+			name: "issue_type as string vs enum - unchanged",
+			existing: &types.Issue{
+				IssueType: types.TypeBug,
+			},
+			updates: map[string]interface{}{
+				"issue_type": "bug", // string instead of enum
+			},
+			want: false,
+		},
+		{
+			name: "priority as int - unchanged",
+			existing: &types.Issue{
+				Priority: 3,
+			},
+			updates: map[string]interface{}{
+				"priority": 3,
+			},
+			want: false,
+		},
+		{
+			name: "priority as int64 - unchanged",
+			existing: &types.Issue{
+				Priority: 2,
+			},
+			updates: map[string]interface{}{
+				"priority": int64(2),
+			},
+			want: false,
+		},
+		{
+			name: "priority as float64 whole number - unchanged",
+			existing: &types.Issue{
+				Priority: 1,
+			},
+			updates: map[string]interface{}{
+				"priority": float64(1),
+			},
+			want: false,
+		},
+		{
+			name: "priority as float64 fractional - changed",
+			existing: &types.Issue{
+				Priority: 1,
+			},
+			updates: map[string]interface{}{
+				"priority": 1.5, // fractional not allowed
+			},
+			want: true,
+		},
+		{
+			name: "empty string vs empty - unchanged",
+			existing: &types.Issue{
+				Design: "",
+			},
+			updates: map[string]interface{}{
+				"design": "",
+			},
+			want: false,
+		},
+		{
+			name: "empty string vs nil - unchanged (treated as equal)",
+			existing: &types.Issue{
+				Assignee: "",
+			},
+			updates: map[string]interface{}{
+				"assignee": nil,
+			},
+			want: false,
+		},
+		{
+			name: "non-empty to empty - changed",
+			existing: &types.Issue{
+				Notes: "Some notes",
+			},
+			updates: map[string]interface{}{
+				"notes": "",
+			},
+			want: true,
+		},
+		{
+			name: "external_ref nil vs empty - unchanged",
+			existing: &types.Issue{
+				ExternalRef: nil,
+			},
+			updates: map[string]interface{}{
+				"external_ref": nil,
+			},
+			want: false,
+		},
+		{
+			name: "external_ref pointer to empty vs nil - unchanged",
+			existing: &types.Issue{
+				ExternalRef: strPtr(""),
+			},
+			updates: map[string]interface{}{
+				"external_ref": nil,
+			},
+			want: false,
+		},
+		{
+			name: "external_ref changed",
+			existing: &types.Issue{
+				ExternalRef: strPtr("gh-123"),
+			},
+			updates: map[string]interface{}{
+				"external_ref": "gh-456",
+			},
+			want: true,
+		},
+		{
+			name: "unknown field - treated as changed",
+			existing: &types.Issue{
+				Title: "Test",
+			},
+			updates: map[string]interface{}{
+				"title":        "Test",
+				"unknown_field": "value",
+			},
+			want: true,
+		},
+		{
+			name: "invalid type for title - treated as changed",
+			existing: &types.Issue{
+				Title: "Test",
+			},
+			updates: map[string]interface{}{
+				"title": 123, // wrong type
+			},
+			want: true,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := issueDataChanged(tt.existing, tt.updates)
+			if got != tt.want {
+				t.Errorf("issueDataChanged() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}
+
+// strPtr helper for tests
+func strPtr(s string) *string {
+	return &s
+}
+
 // TestIdempotentImportNoTimestampChurn verifies that importing unchanged issues
 // does not update their timestamps (bd-84)
 func TestIdempotentImportNoTimestampChurn(t *testing.T) {

cmd/bd/import_shared.go

@@ -15,88 +15,136 @@ import (
 // issueDataChanged checks if any fields in the updates map differ from the existing issue
 // Returns true if any field changed, false if all fields match
 func issueDataChanged(existing *types.Issue, updates map[string]interface{}) bool {
-	// Helper to safely compare string values (handles empty vs nil)
-	strMatch := func(existingVal string, newVal interface{}) bool {
-		if newVal == nil {
-			return existingVal == ""
+	// Helper to safely extract string from interface (handles string and *string)
+	strFrom := func(v interface{}) (string, bool) {
+		switch t := v.(type) {
+		case string:
+			return t, true
+		case *string:
+			if t == nil {
+				return "", true
+			}
+			return *t, true
+		case nil:
+			return "", true
+		default:
+			return "", false
 		}
-		valStr, ok := newVal.(string)
-		if !ok {
-			return false
-		}
-		return existingVal == valStr
 	}
 
-	// Helper to safely compare pointer string values
-	ptrStrMatch := func(ptr *string, val interface{}) bool {
-		if val == nil {
-			return ptr == nil || *ptr == ""
-		}
-		valStr, ok := val.(string)
+	// Helper to compare string field (treats empty and nil as equal)
+	equalStr := func(existingVal string, newVal interface{}) bool {
+		s, ok := strFrom(newVal)
 		if !ok {
-			return false
+			return false // Type mismatch means changed
 		}
-		if ptr == nil {
-			return valStr == ""
+		return existingVal == s
+	}
+
+	// Helper to compare *string field (treats empty and nil as equal)
+	equalPtrStr := func(existing *string, newVal interface{}) bool {
+		s, ok := strFrom(newVal)
+		if !ok {
+			return false // Type mismatch means changed
+		}
+		if existing == nil {
+			return s == ""
+		}
+		return *existing == s
+	}
+
+	// Helper to safely extract int from interface
+	intFrom := func(v interface{}) (int64, bool) {
+		switch t := v.(type) {
+		case int:
+			return int64(t), true
+		case int32:
+			return int64(t), true
+		case int64:
+			return t, true
+		case float64:
+			// Only accept whole numbers
+			if t == float64(int64(t)) {
+				return int64(t), true
+			}
+			return 0, false
+		default:
+			return 0, false
+		}
+	}
+
+	// Helper to compare Status field
+	equalStatus := func(existing types.Status, newVal interface{}) bool {
+		switch t := newVal.(type) {
+		case types.Status:
+			return existing == t
+		case string:
+			return string(existing) == t
+		default:
+			return false // Unknown type means changed
+		}
+	}
+
+	// Helper to compare IssueType field
+	equalIssueType := func(existing types.IssueType, newVal interface{}) bool {
+		switch t := newVal.(type) {
+		case types.IssueType:
+			return existing == t
+		case string:
+			return string(existing) == t
+		default:
+			return false // Unknown type means changed
 		}
-		return *ptr == valStr
 	}
 
 	// Check each field in updates map
 	for key, newVal := range updates {
 		switch key {
 		case "title":
-			if existing.Title != newVal.(string) {
+			if !equalStr(existing.Title, newVal) {
 				return true
 			}
 		case "description":
-			if existing.Description != newVal.(string) {
+			if !equalStr(existing.Description, newVal) {
 				return true
 			}
 		case "status":
-			if newStatus, ok := newVal.(types.Status); ok {
-				if existing.Status != newStatus {
-					return true
-				}
-			} else if newStatusStr, ok := newVal.(string); ok {
-				if string(existing.Status) != newStatusStr {
-					return true
-				}
+			if !equalStatus(existing.Status, newVal) {
+				return true
 			}
 		case "priority":
-			if existing.Priority != newVal.(int) {
+			p, ok := intFrom(newVal)
+			if !ok || existing.Priority != int(p) {
 				return true
 			}
 		case "issue_type":
-			if newType, ok := newVal.(types.IssueType); ok {
-				if existing.IssueType != newType {
-					return true
-				}
-			} else if newTypeStr, ok := newVal.(string); ok {
-				if string(existing.IssueType) != newTypeStr {
-					return true
-				}
+			if !equalIssueType(existing.IssueType, newVal) {
+				return true
 			}
 		case "design":
-			if !strMatch(existing.Design, newVal) {
+			if !equalStr(existing.Design, newVal) {
 				return true
 			}
 		case "acceptance_criteria":
-			if !strMatch(existing.AcceptanceCriteria, newVal) {
+			if !equalStr(existing.AcceptanceCriteria, newVal) {
 				return true
 			}
 		case "notes":
-			if !strMatch(existing.Notes, newVal) {
+			if !equalStr(existing.Notes, newVal) {
 				return true
 			}
 		case "assignee":
-			if !strMatch(existing.Assignee, newVal) {
+			if !equalStr(existing.Assignee, newVal) {
 				return true
 			}
 		case "external_ref":
-			if !ptrStrMatch(existing.ExternalRef, newVal) {
+			if !equalPtrStr(existing.ExternalRef, newVal) {
 				return true
 			}
+		default:
+			// Unknown field - treat as changed to be conservative
+			// This prevents skipping updates when new fields are added
+			return true
 		}
 	}