From 22cf66a4164d853be13e65824461d47355561202 Mon Sep 17 00:00:00 2001 From: Steve Yegge Date: Thu, 23 Oct 2025 18:41:54 -0700 Subject: [PATCH] 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 --- cmd/bd/import_idempotent_test.go | 204 +++++++++++++++++++++++++++++++ cmd/bd/import_shared.go | 134 +++++++++++++------- 2 files changed, 295 insertions(+), 43 deletions(-) diff --git a/cmd/bd/import_idempotent_test.go b/cmd/bd/import_idempotent_test.go index aec0e46d..157398f3 100644 --- a/cmd/bd/import_idempotent_test.go +++ b/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) { diff --git a/cmd/bd/import_shared.go b/cmd/bd/import_shared.go index d86b02f4..0ff4e28c 100644 --- a/cmd/bd/import_shared.go +++ b/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 } }