From 504e9806697e45f7b2914c0c95dbdd36ca47042e Mon Sep 17 00:00:00 2001 From: aaron-sangster Date: Tue, 6 Jan 2026 03:11:38 +0000 Subject: [PATCH] fix(doctor): align duplicate detection with bd duplicates (bd-sali) (#917) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Exclude closed issues from duplicate detection (previously only excluded tombstones) - Use full content hash: title + description + design + acceptanceCriteria + status - Add comprehensive test coverage for duplicate detection edge cases 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Opus 4.5 --- cmd/bd/doctor/validation.go | 8 +- cmd/bd/doctor/validation_test.go | 276 +++++++++++++++++++++++++++++++ 2 files changed, 281 insertions(+), 3 deletions(-) create mode 100644 cmd/bd/doctor/validation_test.go diff --git a/cmd/bd/doctor/validation.go b/cmd/bd/doctor/validation.go index ca4a132c..0806d9a4 100644 --- a/cmd/bd/doctor/validation.go +++ b/cmd/bd/doctor/validation.go @@ -215,13 +215,15 @@ func CheckDuplicateIssues(path string) DoctorCheck { } } - // Find duplicates by title+description hash + // Find duplicates by content hash (matching bd duplicates algorithm) + // Only check open issues - closed issues are done, no point flagging duplicates seen := make(map[string][]string) // hash -> list of IDs for _, issue := range issues { - if issue.Status == types.StatusTombstone { + if issue.Status == types.StatusTombstone || issue.Status == types.StatusClosed { continue } - key := issue.Title + "|" + issue.Description + // Content key matches bd duplicates: title + description + design + acceptanceCriteria + status + key := issue.Title + "|" + issue.Description + "|" + issue.Design + "|" + issue.AcceptanceCriteria + "|" + string(issue.Status) seen[key] = append(seen[key], issue.ID) } diff --git a/cmd/bd/doctor/validation_test.go b/cmd/bd/doctor/validation_test.go new file mode 100644 index 00000000..d04cb4fa --- /dev/null +++ b/cmd/bd/doctor/validation_test.go @@ -0,0 +1,276 @@ +package doctor + +import ( + "context" + "os" + "path/filepath" + "testing" + + "github.com/steveyegge/beads/internal/beads" + "github.com/steveyegge/beads/internal/storage/sqlite" + "github.com/steveyegge/beads/internal/types" +) + +// TestCheckDuplicateIssues_ClosedIssuesExcluded verifies that closed issues +// are not flagged as duplicates (bug fix: bd-sali). +// Previously, doctor used title+description only and included closed issues, +// while bd duplicates excluded closed issues and used full content hash. +func TestCheckDuplicateIssues_ClosedIssuesExcluded(t *testing.T) { + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.Mkdir(beadsDir, 0755); err != nil { + t.Fatal(err) + } + + dbPath := filepath.Join(beadsDir, beads.CanonicalDatabaseName) + ctx := context.Background() + + store, err := sqlite.New(ctx, dbPath) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Initialize database with prefix + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set issue_prefix: %v", err) + } + + // Create closed issues with same title+description + // These should NOT be flagged as duplicates + issues := []*types.Issue{ + {Title: "mol-feature-dev", Description: "Molecule for feature", Status: types.StatusClosed, Priority: 2, IssueType: types.TypeTask}, + {Title: "mol-feature-dev", Description: "Molecule for feature", Status: types.StatusClosed, Priority: 2, IssueType: types.TypeTask}, + {Title: "mol-feature-dev", Description: "Molecule for feature", Status: types.StatusClosed, Priority: 2, IssueType: types.TypeTask}, + } + + for _, issue := range issues { + if err := store.CreateIssue(ctx, issue, "test"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + } + + // Close the store so CheckDuplicateIssues can open it + store.Close() + + check := CheckDuplicateIssues(tmpDir) + + // Should NOT report duplicates because all are closed + if check.Status != StatusOK { + t.Errorf("Status = %q, want %q (closed issues should be excluded from duplicate detection)", check.Status, StatusOK) + t.Logf("Message: %s", check.Message) + } +} + +// TestCheckDuplicateIssues_OpenDuplicatesDetected verifies that open issues +// with identical content ARE flagged as duplicates. +func TestCheckDuplicateIssues_OpenDuplicatesDetected(t *testing.T) { + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.Mkdir(beadsDir, 0755); err != nil { + t.Fatal(err) + } + + dbPath := filepath.Join(beadsDir, beads.CanonicalDatabaseName) + ctx := context.Background() + + store, err := sqlite.New(ctx, dbPath) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Initialize database with prefix + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set issue_prefix: %v", err) + } + + // Create open issues with same content - these SHOULD be flagged + issues := []*types.Issue{ + {Title: "Fix auth bug", Description: "Users cannot login", Design: "Use OAuth", AcceptanceCriteria: "User can login", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeBug}, + {Title: "Fix auth bug", Description: "Users cannot login", Design: "Use OAuth", AcceptanceCriteria: "User can login", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeBug}, + } + + for _, issue := range issues { + if err := store.CreateIssue(ctx, issue, "test"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + } + + store.Close() + + check := CheckDuplicateIssues(tmpDir) + + if check.Status != StatusWarning { + t.Errorf("Status = %q, want %q (open duplicates should be detected)", check.Status, StatusWarning) + } + if check.Message != "1 duplicate issue(s) in 1 group(s)" { + t.Errorf("Message = %q, want '1 duplicate issue(s) in 1 group(s)'", check.Message) + } +} + +// TestCheckDuplicateIssues_DifferentDesignNotDuplicate verifies that issues +// with same title+description but different design are NOT duplicates. +// This tests the full content hash (title+description+design+acceptanceCriteria+status). +func TestCheckDuplicateIssues_DifferentDesignNotDuplicate(t *testing.T) { + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.Mkdir(beadsDir, 0755); err != nil { + t.Fatal(err) + } + + dbPath := filepath.Join(beadsDir, beads.CanonicalDatabaseName) + ctx := context.Background() + + store, err := sqlite.New(ctx, dbPath) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Initialize database with prefix + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set issue_prefix: %v", err) + } + + // Create open issues with same title+description but DIFFERENT design + // These should NOT be flagged as duplicates + issues := []*types.Issue{ + {Title: "Fix auth bug", Description: "Users cannot login", Design: "Use OAuth", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeBug}, + {Title: "Fix auth bug", Description: "Users cannot login", Design: "Use SAML", Status: types.StatusOpen, Priority: 1, IssueType: types.TypeBug}, + } + + for _, issue := range issues { + if err := store.CreateIssue(ctx, issue, "test"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + } + + store.Close() + + check := CheckDuplicateIssues(tmpDir) + + if check.Status != StatusOK { + t.Errorf("Status = %q, want %q (different design = not duplicates)", check.Status, StatusOK) + t.Logf("Message: %s", check.Message) + } +} + +// TestCheckDuplicateIssues_MixedOpenClosed verifies correct behavior when +// there are both open and closed issues with same content. +// Only open duplicates should be flagged. +func TestCheckDuplicateIssues_MixedOpenClosed(t *testing.T) { + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.Mkdir(beadsDir, 0755); err != nil { + t.Fatal(err) + } + + dbPath := filepath.Join(beadsDir, beads.CanonicalDatabaseName) + ctx := context.Background() + + store, err := sqlite.New(ctx, dbPath) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Initialize database with prefix + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set issue_prefix: %v", err) + } + + // Create open issues first (will be duplicates of each other) + openIssues := []*types.Issue{ + {Title: "Task A", Description: "Do something", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask}, + {Title: "Task A", Description: "Do something", Status: types.StatusOpen, Priority: 2, IssueType: types.TypeTask}, + } + + for _, issue := range openIssues { + if err := store.CreateIssue(ctx, issue, "test"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + } + + // Create a closed issue with same content (should NOT be part of duplicate group) + closedIssue := &types.Issue{Title: "Task A", Description: "Do something", Status: types.StatusClosed, Priority: 2, IssueType: types.TypeTask} + if err := store.CreateIssue(ctx, closedIssue, "test"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + + store.Close() + + check := CheckDuplicateIssues(tmpDir) + + // Should detect 1 duplicate (the pair of open issues) + if check.Status != StatusWarning { + t.Errorf("Status = %q, want %q", check.Status, StatusWarning) + } + if check.Message != "1 duplicate issue(s) in 1 group(s)" { + t.Errorf("Message = %q, want '1 duplicate issue(s) in 1 group(s)'", check.Message) + } +} + +// TestCheckDuplicateIssues_TombstonesExcluded verifies tombstoned issues +// are excluded from duplicate detection. +func TestCheckDuplicateIssues_TombstonesExcluded(t *testing.T) { + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.Mkdir(beadsDir, 0755); err != nil { + t.Fatal(err) + } + + dbPath := filepath.Join(beadsDir, beads.CanonicalDatabaseName) + ctx := context.Background() + + store, err := sqlite.New(ctx, dbPath) + if err != nil { + t.Fatalf("Failed to create store: %v", err) + } + defer store.Close() + + // Initialize database with prefix + if err := store.SetConfig(ctx, "issue_prefix", "test"); err != nil { + t.Fatalf("Failed to set issue_prefix: %v", err) + } + + // Create tombstoned issues - these should NOT be flagged + issues := []*types.Issue{ + {Title: "Deleted issue", Description: "Was deleted", Status: types.StatusTombstone, Priority: 2, IssueType: types.TypeTask}, + {Title: "Deleted issue", Description: "Was deleted", Status: types.StatusTombstone, Priority: 2, IssueType: types.TypeTask}, + } + + for _, issue := range issues { + if err := store.CreateIssue(ctx, issue, "test"); err != nil { + t.Fatalf("Failed to create issue: %v", err) + } + } + + store.Close() + + check := CheckDuplicateIssues(tmpDir) + + if check.Status != StatusOK { + t.Errorf("Status = %q, want %q (tombstones should be excluded)", check.Status, StatusOK) + } +} + +// TestCheckDuplicateIssues_NoDatabase verifies graceful handling when no database exists. +func TestCheckDuplicateIssues_NoDatabase(t *testing.T) { + tmpDir := t.TempDir() + beadsDir := filepath.Join(tmpDir, ".beads") + if err := os.Mkdir(beadsDir, 0755); err != nil { + t.Fatal(err) + } + + // No database file created + + check := CheckDuplicateIssues(tmpDir) + + if check.Status != StatusOK { + t.Errorf("Status = %q, want %q", check.Status, StatusOK) + } + if check.Message != "N/A (no database)" { + t.Errorf("Message = %q, want 'N/A (no database)'", check.Message) + } +}