diff --git a/.beads/beads.jsonl b/.beads/beads.jsonl index a46f751e..12dce632 100644 --- a/.beads/beads.jsonl +++ b/.beads/beads.jsonl @@ -82,7 +82,7 @@ {"id":"bd-52","title":"Clean up linter errors (914 total issues)","description":"The codebase has 914 linter issues reported by golangci-lint. While many are documented as baseline in LINTING.md, we should clean these up systematically to improve code quality and maintainability.","design":"Break down by linter category, prioritizing high-impact issues:\n1. dupl (7) - Code duplication\n2. goconst (12) - Repeated strings\n3. gocyclo (11) - High complexity functions\n4. revive (78) - Style issues\n5. gosec (102) - Security warnings\n6. errcheck (683) - Unchecked errors (many in tests)","acceptance_criteria":"All linter categories reduced to acceptable levels, with remaining baseline documented in LINTING.md","notes":"Reduced from 56 to 41 issues locally, then to 0 issues.\n\n**Fixed in commits:**\n- c2c7eda: Fixed 15 actual errors (dupl, gosec, revive, staticcheck, unparam)\n- 963181d: Configured exclusions to get to 0 issues locally\n\n**Current status:**\n- ✅ Local: golangci-lint reports 0 issues\n- ❌ CI: Still failing (see bd-65)\n\n**Problem:**\nConfig v2 format or golangci-lint-action@v8 compatibility issue causing CI to fail despite local success.\n\n**Next:** Debug bd-65 to fix CI/local discrepancy","status":"in_progress","priority":2,"issue_type":"epic","created_at":"2025-10-24T01:01:12.997982-07:00","updated_at":"2025-10-24T13:51:54.439577-07:00"} {"id":"bd-53","title":"Fix code duplication in label.go (dupl)","description":"Lines 72-120 duplicate lines 122-170 in cmd/bd/label.go. The add and remove commands have nearly identical structure.","design":"Extract common batch operation logic into a shared helper function that takes the operation type as a parameter.","status":"closed","priority":1,"issue_type":"task","created_at":"2025-10-24T01:01:36.971666-07:00","updated_at":"2025-10-24T13:51:54.416434-07:00","closed_at":"2025-10-24T12:40:43.046348-07:00","dependencies":[{"issue_id":"bd-53","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.325899-07:00","created_by":"renumber"}]} {"id":"bd-54","title":"Convert repeated strings to constants (goconst)","description":"12 instances of repeated strings that should be constants: \"alice\", \"windows\", \"bd-114\", \"daemon\", \"import\", \"healthy\", \"unhealthy\", \"1.0.0\", \"custom-1\", \"custom-2\"","design":"Create package-level or test-level constants for frequently used test strings and command names.","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-24T01:01:36.9778-07:00","updated_at":"2025-10-24T13:51:54.439751-07:00","dependencies":[{"issue_id":"bd-54","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.326123-07:00","created_by":"renumber"}]} -{"id":"bd-55","title":"Refactor high complexity functions (gocyclo)","description":"11 functions exceed cyclomatic complexity threshold (\u003e30): runDaemonLoop (42), importIssuesCore (71), TestLabelCommands (67), issueDataChanged (39), etc.","design":"Break down complex functions into smaller, testable units. Extract validation, error handling, and business logic into separate functions.","status":"open","priority":1,"issue_type":"task","created_at":"2025-10-24T01:01:36.989066-07:00","updated_at":"2025-10-25T10:33:30.169063-07:00","dependencies":[{"issue_id":"bd-55","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.323992-07:00","created_by":"renumber"}]} +{"id":"bd-55","title":"Refactor high complexity functions (gocyclo)","description":"11 functions exceed cyclomatic complexity threshold (\u003e30): runDaemonLoop (42), importIssuesCore (71), TestLabelCommands (67), issueDataChanged (39), etc.","design":"Break down complex functions into smaller, testable units. Extract validation, error handling, and business logic into separate functions.","notes":"Refactored issueDataChanged from complexity 39 → 11 by extracting into fieldComparator struct with methods for each comparison type.\n\nRefactored runDaemonLoop from complexity 42 → 7 by extracting:\n- setupDaemonLogger: Logger initialization logic\n- setupDaemonLock: Lock and PID file management\n- startRPCServer: RPC server startup with error handling\n- runGlobalDaemon: Global daemon mode handling\n- createSyncFunc: Sync cycle logic (export, commit, pull, import, push)\n- runEventLoop: Signal handling and main event loop\n\nCode review fixes:\n- Fixed sync overlap: Changed initial sync from `go doSync()` to synchronous `doSync()` to prevent race with ticker\n- Fixed resource cleanup: Replaced `os.Exit(1)` with `return` after acquiring locks to ensure defers run and clean up PID files/locks\n- Added signal.Stop(sigChan) in runEventLoop and runGlobalDaemon to prevent lingering notifications\n- Added server.Stop() in serverErrChan case for consistent cleanup\n\nRefactored TestLabelCommands from complexity 67 → \u003c10 by extracting labelTestHelper with methods:\n- createIssue: Issue creation helper\n- addLabel/addLabels: Label addition helpers\n- removeLabel: Label removal helper\n- getLabels: Label retrieval helper\n- assertLabelCount/assertHasLabel/assertHasLabels/assertNotHasLabel: Assertion helpers\n- assertLabelEvent: Event verification helper\n\nRefactored TestReopenCommand from complexity 37 → \u003c10 by extracting reopenTestHelper with methods:\n- createIssue: Issue creation helper\n- closeIssue/reopenIssue: State transition helpers\n- getIssue: Issue retrieval helper\n- addComment: Comment addition helper\n- assertStatus/assertClosedAtSet/assertClosedAtNil: Status assertion helpers\n- assertCommentEvent: Event verification helper\n\nAll tests pass after refactoring.","status":"in_progress","priority":1,"issue_type":"task","created_at":"2025-10-24T01:01:36.989066-07:00","updated_at":"2025-10-25T11:22:17.056061-07:00","dependencies":[{"issue_id":"bd-55","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.323992-07:00","created_by":"renumber"}]} {"id":"bd-56","title":"Fix revive style issues (78 issues)","description":"Style violations: unused parameters (many cmd/args in cobra commands), missing exported comments, stuttering names (SQLiteStorage), indent-error-flow issues.","design":"Rename unused params to _, add godoc comments to exported types, fix stuttering names, simplify control flow.","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-24T01:01:36.99984-07:00","updated_at":"2025-10-24T13:51:54.417341-07:00","dependencies":[{"issue_id":"bd-56","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.322412-07:00","created_by":"renumber"}]} {"id":"bd-57","title":"Address gosec security warnings (102 issues)","description":"Security linter warnings: file permissions (0755 should be 0750), G304 file inclusion via variable, G204 subprocess launches. Many are false positives but should be reviewed.","design":"Review each gosec warning. Add exclusions for legitimate cases to .golangci.yml. Fix real security issues (overly permissive file modes).","status":"open","priority":2,"issue_type":"task","created_at":"2025-10-24T01:01:37.0139-07:00","updated_at":"2025-10-24T13:51:54.417632-07:00","dependencies":[{"issue_id":"bd-57","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.324202-07:00","created_by":"renumber"}]} {"id":"bd-58","title":"Handle unchecked errors (errcheck - 683 issues)","description":"683 unchecked error returns, mostly in tests (Close, Rollback, RemoveAll). Many already excluded in config but still showing up.","design":"Review .golangci.yml exclude-rules. Most defer Close/Rollback errors in tests can be ignored. Add systematic exclusions or explicit _ = assignments where appropriate.","status":"open","priority":3,"issue_type":"task","created_at":"2025-10-24T01:01:37.018404-07:00","updated_at":"2025-10-24T13:51:54.41793-07:00","dependencies":[{"issue_id":"bd-58","depends_on_id":"bd-52","type":"parent-child","created_at":"2025-10-24T13:17:40.324423-07:00","created_by":"renumber"}]} diff --git a/cmd/bd/daemon.go b/cmd/bd/daemon.go index f50694d9..468adde2 100644 --- a/cmd/bd/daemon.go +++ b/cmd/bd/daemon.go @@ -4,6 +4,7 @@ import ( "context" "encoding/json" "fmt" + "io" "os" "os/exec" "os/signal" @@ -781,8 +782,15 @@ func importToJSONLWithStore(ctx context.Context, store storage.Storage, jsonlPat return nil } -func runDaemonLoop(interval time.Duration, autoCommit, autoPush bool, logPath, pidFile string, global bool) { - // Configure log rotation with lumberjack +type daemonLogger struct { + logFunc func(string, ...interface{}) +} + +func (d *daemonLogger) log(format string, args ...interface{}) { + d.logFunc(format, args...) +} + +func setupDaemonLogger(logPath string) (*lumberjack.Logger, daemonLogger) { maxSizeMB := getEnvInt("BEADS_DAEMON_LOG_MAX_SIZE", 10) maxBackups := getEnvInt("BEADS_DAEMON_LOG_MAX_BACKUPS", 3) maxAgeDays := getEnvInt("BEADS_DAEMON_LOG_MAX_AGE", 7) @@ -790,221 +798,172 @@ func runDaemonLoop(interval time.Duration, autoCommit, autoPush bool, logPath, p logF := &lumberjack.Logger{ Filename: logPath, - MaxSize: maxSizeMB, // MB - MaxBackups: maxBackups, // number of rotated files - MaxAge: maxAgeDays, // days - Compress: compress, // compress old logs - } - defer func() { _ = logF.Close() }() - - log := func(format string, args ...interface{}) { - msg := fmt.Sprintf(format, args...) - timestamp := time.Now().Format("2006-01-02 15:04:05") - _, _ = fmt.Fprintf(logF, "[%s] %s\n", timestamp, msg) + MaxSize: maxSizeMB, + MaxBackups: maxBackups, + MaxAge: maxAgeDays, + Compress: compress, } - // Acquire daemon lock FIRST - this is the single source of truth for exclusivity + logger := daemonLogger{ + logFunc: func(format string, args ...interface{}) { + msg := fmt.Sprintf(format, args...) + timestamp := time.Now().Format("2006-01-02 15:04:05") + _, _ = fmt.Fprintf(logF, "[%s] %s\n", timestamp, msg) + }, + } + + return logF, logger +} + +func setupDaemonLock(pidFile string, global bool, log daemonLogger) (io.Closer, error) { beadsDir := filepath.Dir(pidFile) lock, err := acquireDaemonLock(beadsDir, global) if err != nil { if err == ErrDaemonLocked { - log("Daemon already running (lock held), exiting") - os.Exit(1) + log.log("Daemon already running (lock held), exiting") + } else { + log.log("Error acquiring daemon lock: %v", err) } - log("Error acquiring daemon lock: %v", err) - os.Exit(1) + return nil, err } - defer func() { _ = lock.Close() }() - // PID file was already written by acquireDaemonLock, but verify it has our PID myPID := os.Getpid() if data, err := os.ReadFile(pidFile); err == nil { if pid, err := strconv.Atoi(strings.TrimSpace(string(data))); err == nil && pid == myPID { // PID file is correct, continue } else { - log("PID file has wrong PID (expected %d, got %d), overwriting", myPID, pid) + log.log("PID file has wrong PID (expected %d, got %d), overwriting", myPID, pid) _ = os.WriteFile(pidFile, []byte(fmt.Sprintf("%d\n", myPID)), 0600) } } else { - // PID file missing (shouldn't happen since acquireDaemonLock writes it), create it - log("PID file missing after lock acquisition, creating") + log.log("PID file missing after lock acquisition, creating") _ = os.WriteFile(pidFile, []byte(fmt.Sprintf("%d\n", myPID)), 0600) } - defer func() { _ = os.Remove(pidFile) }() + return lock, nil +} - log("Daemon started (interval: %v, auto-commit: %v, auto-push: %v)", interval, autoCommit, autoPush) +func startRPCServer(ctx context.Context, socketPath string, store storage.Storage, log daemonLogger) (*rpc.Server, chan error, error) { + server := rpc.NewServer(socketPath, store) + serverErrChan := make(chan error, 1) - // Global daemon runs in routing mode without opening a database - if global { - globalDir, err := getGlobalBeadsDir() - if err != nil { - log("Error: cannot get global beads directory: %v", err) - os.Exit(1) + go func() { + log.log("Starting RPC server: %s", socketPath) + if err := server.Start(ctx); err != nil { + log.log("RPC server error: %v", err) + serverErrChan <- err } - socketPath := filepath.Join(globalDir, "bd.sock") + }() - // Create server with nil storage - uses per-request routing - server := rpc.NewServer(socketPath, nil) - - ctx, cancel := context.WithCancel(context.Background()) - defer cancel() - - // Start RPC server in background - serverErrChan := make(chan error, 1) - go func() { - log("Starting global RPC server: %s", socketPath) - if err := server.Start(ctx); err != nil { - log("RPC server error: %v", err) - serverErrChan <- err - } - }() - - // Wait for server to be ready or fail - select { - case err := <-serverErrChan: - log("RPC server failed to start: %v", err) - os.Exit(1) - case <-server.WaitReady(): - log("Global RPC server ready (socket listening)") - case <-time.After(5 * time.Second): - log("WARNING: Server didn't signal ready after 5 seconds (may still be starting)") - } - - // Wait for shutdown signal - sigChan := make(chan os.Signal, 1) - signal.Notify(sigChan, daemonSignals...) - - sig := <-sigChan - log("Received signal: %v", sig) - log("Shutting down global daemon...") - - cancel() - if err := server.Stop(); err != nil { - log("Error stopping server: %v", err) - } - - log("Global daemon stopped") - return + select { + case err := <-serverErrChan: + log.log("RPC server failed to start: %v", err) + return nil, nil, err + case <-server.WaitReady(): + log.log("RPC server ready (socket listening)") + case <-time.After(5 * time.Second): + log.log("WARNING: Server didn't signal ready after 5 seconds (may still be starting)") } - // Local daemon mode - open database and run sync loop - daemonDBPath := dbPath - if daemonDBPath == "" { - // Try to find database in current repo - if foundDB := beads.FindDatabasePath(); foundDB != "" { - daemonDBPath = foundDB - } else { - // No database found - error out instead of falling back to ~/.beads - log("Error: no beads database found") - log("Hint: run 'bd init' to create a database or set BEADS_DB environment variable") - os.Exit(1) - } - } + return server, serverErrChan, nil +} - log("Using database: %s", daemonDBPath) - - store, err := sqlite.New(daemonDBPath) +func runGlobalDaemon(log daemonLogger) { + globalDir, err := getGlobalBeadsDir() if err != nil { - log("Error: cannot open database: %v", err) + log.log("Error: cannot get global beads directory: %v", err) os.Exit(1) } - defer func() { _ = store.Close() }() - log("Database opened: %s", daemonDBPath) - - // Start RPC server - socketPath := filepath.Join(filepath.Dir(daemonDBPath), "bd.sock") - server := rpc.NewServer(socketPath, store) + socketPath := filepath.Join(globalDir, "bd.sock") ctx, cancel := context.WithCancel(context.Background()) defer cancel() - // Start RPC server in background - serverErrChan := make(chan error, 1) - go func() { - log("Starting RPC server: %s", socketPath) - if err := server.Start(ctx); err != nil { - log("RPC server error: %v", err) - serverErrChan <- err - } - }() - // Wait for server to be ready or fail - select { - case err := <-serverErrChan: - log("RPC server failed to start: %v", err) - os.Exit(1) - case <-server.WaitReady(): - log("RPC server ready (socket listening)") - case <-time.After(5 * time.Second): - log("WARNING: Server didn't signal ready after 5 seconds (may still be starting)") + server, _, err := startRPCServer(ctx, socketPath, nil, log) + if err != nil { + return } sigChan := make(chan os.Signal, 1) signal.Notify(sigChan, daemonSignals...) + defer signal.Stop(sigChan) - ticker := time.NewTicker(interval) - defer ticker.Stop() + sig := <-sigChan + log.log("Received signal: %v", sig) + log.log("Shutting down global daemon...") - doSync := func() { + cancel() + if err := server.Stop(); err != nil { + log.log("Error stopping server: %v", err) + } + + log.log("Global daemon stopped") +} + +func createSyncFunc(ctx context.Context, store storage.Storage, autoCommit, autoPush bool, log daemonLogger) func() { + return func() { syncCtx, syncCancel := context.WithTimeout(ctx, 2*time.Minute) defer syncCancel() - log("Starting sync cycle...") + log.log("Starting sync cycle...") jsonlPath := findJSONLPath() if jsonlPath == "" { - log("Error: JSONL path not found") + log.log("Error: JSONL path not found") return } if err := exportToJSONLWithStore(syncCtx, store, jsonlPath); err != nil { - log("Export failed: %v", err) + log.log("Export failed: %v", err) return } - log("Exported to JSONL") + log.log("Exported to JSONL") if autoCommit { hasChanges, err := gitHasChanges(syncCtx, jsonlPath) if err != nil { - log("Error checking git status: %v", err) + log.log("Error checking git status: %v", err) return } if hasChanges { message := fmt.Sprintf("bd daemon sync: %s", time.Now().Format("2006-01-02 15:04:05")) if err := gitCommit(syncCtx, jsonlPath, message); err != nil { - log("Commit failed: %v", err) + log.log("Commit failed: %v", err) return } - log("Committed changes") + log.log("Committed changes") } } if err := gitPull(syncCtx); err != nil { - log("Pull failed: %v", err) + log.log("Pull failed: %v", err) return } - log("Pulled from remote") + log.log("Pulled from remote") if err := importToJSONLWithStore(syncCtx, store, jsonlPath); err != nil { - log("Import failed: %v", err) + log.log("Import failed: %v", err) return } - log("Imported from JSONL") + log.log("Imported from JSONL") if autoPush && autoCommit { if err := gitPush(syncCtx); err != nil { - log("Push failed: %v", err) + log.log("Push failed: %v", err) return } - log("Pushed to remote") + log.log("Pushed to remote") } - log("Sync cycle complete") + log.log("Sync cycle complete") } +} - // Run initial sync in background so daemon becomes responsive immediately - go doSync() +func runEventLoop(ctx context.Context, cancel context.CancelFunc, ticker *time.Ticker, doSync func(), server *rpc.Server, serverErrChan chan error, log daemonLogger) { + sigChan := make(chan os.Signal, 1) + signal.Notify(sigChan, daemonSignals...) + defer signal.Stop(sigChan) for { select { @@ -1015,25 +974,85 @@ func runDaemonLoop(interval time.Duration, autoCommit, autoPush bool, logPath, p doSync() case sig := <-sigChan: if isReloadSignal(sig) { - log("Received reload signal, ignoring (daemon continues running)") + log.log("Received reload signal, ignoring (daemon continues running)") continue } - log("Received signal %v, shutting down gracefully...", sig) + log.log("Received signal %v, shutting down gracefully...", sig) cancel() if err := server.Stop(); err != nil { - log("Error stopping RPC server: %v", err) + log.log("Error stopping RPC server: %v", err) } return case <-ctx.Done(): - log("Context canceled, shutting down") + log.log("Context canceled, shutting down") if err := server.Stop(); err != nil { - log("Error stopping RPC server: %v", err) + log.log("Error stopping RPC server: %v", err) } return case err := <-serverErrChan: - log("RPC server failed: %v", err) + log.log("RPC server failed: %v", err) cancel() + if err := server.Stop(); err != nil { + log.log("Error stopping RPC server: %v", err) + } return } } } + +func runDaemonLoop(interval time.Duration, autoCommit, autoPush bool, logPath, pidFile string, global bool) { + logF, log := setupDaemonLogger(logPath) + defer func() { _ = logF.Close() }() + + lock, err := setupDaemonLock(pidFile, global, log) + if err != nil { + os.Exit(1) + } + defer func() { _ = lock.Close() }() + defer func() { _ = os.Remove(pidFile) }() + + log.log("Daemon started (interval: %v, auto-commit: %v, auto-push: %v)", interval, autoCommit, autoPush) + + if global { + runGlobalDaemon(log) + return + } + + daemonDBPath := dbPath + if daemonDBPath == "" { + if foundDB := beads.FindDatabasePath(); foundDB != "" { + daemonDBPath = foundDB + } else { + log.log("Error: no beads database found") + log.log("Hint: run 'bd init' to create a database or set BEADS_DB environment variable") + os.Exit(1) + } + } + + log.log("Using database: %s", daemonDBPath) + + store, err := sqlite.New(daemonDBPath) + if err != nil { + log.log("Error: cannot open database: %v", err) + os.Exit(1) + } + defer func() { _ = store.Close() }() + log.log("Database opened: %s", daemonDBPath) + + socketPath := filepath.Join(filepath.Dir(daemonDBPath), "bd.sock") + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + server, serverErrChan, err := startRPCServer(ctx, socketPath, store, log) + if err != nil { + return + } + + ticker := time.NewTicker(interval) + defer ticker.Stop() + + doSync := createSyncFunc(ctx, store, autoCommit, autoPush, log) + doSync() + + runEventLoop(ctx, cancel, ticker, doSync, server, serverErrChan, log) +} diff --git a/cmd/bd/label_test.go b/cmd/bd/label_test.go index 91b75bce..59135639 100644 --- a/cmd/bd/label_test.go +++ b/cmd/bd/label_test.go @@ -10,6 +10,111 @@ import ( "github.com/steveyegge/beads/internal/types" ) +type labelTestHelper struct { + s *sqlite.SQLiteStorage + ctx context.Context + t *testing.T +} + +func (h *labelTestHelper) createIssue(title string, issueType types.IssueType, priority int) *types.Issue { + issue := &types.Issue{ + Title: title, + Priority: priority, + IssueType: issueType, + Status: types.StatusOpen, + } + if err := h.s.CreateIssue(h.ctx, issue, "test-user"); err != nil { + h.t.Fatalf("Failed to create issue: %v", err) + } + return issue +} + +func (h *labelTestHelper) addLabel(issueID, label string) { + if err := h.s.AddLabel(h.ctx, issueID, label, "test-user"); err != nil { + h.t.Fatalf("Failed to add label '%s': %v", label, err) + } +} + +func (h *labelTestHelper) addLabels(issueID string, labels []string) { + for _, label := range labels { + h.addLabel(issueID, label) + } +} + +func (h *labelTestHelper) removeLabel(issueID, label string) { + if err := h.s.RemoveLabel(h.ctx, issueID, label, "test-user"); err != nil { + h.t.Fatalf("Failed to remove label '%s': %v", label, err) + } +} + +func (h *labelTestHelper) getLabels(issueID string) []string { + labels, err := h.s.GetLabels(h.ctx, issueID) + if err != nil { + h.t.Fatalf("Failed to get labels: %v", err) + } + return labels +} + +func (h *labelTestHelper) assertLabelCount(issueID string, expected int) { + labels := h.getLabels(issueID) + if len(labels) != expected { + h.t.Errorf("Expected %d labels, got %d", expected, len(labels)) + } +} + +func (h *labelTestHelper) assertHasLabel(issueID, expected string) { + labels := h.getLabels(issueID) + for _, l := range labels { + if l == expected { + return + } + } + h.t.Errorf("Expected label '%s' not found", expected) +} + +func (h *labelTestHelper) assertHasLabels(issueID string, expected []string) { + labels := h.getLabels(issueID) + labelMap := make(map[string]bool) + for _, l := range labels { + labelMap[l] = true + } + for _, exp := range expected { + if !labelMap[exp] { + h.t.Errorf("Expected label '%s' not found", exp) + } + } +} + +func (h *labelTestHelper) assertNotHasLabel(issueID, label string) { + labels := h.getLabels(issueID) + for _, l := range labels { + if l == label { + h.t.Errorf("Did not expect label '%s' but found it", label) + } + } +} + +func (h *labelTestHelper) assertLabelEvent(issueID string, eventType types.EventType, labelName string) { + events, err := h.s.GetEvents(h.ctx, issueID, 100) + if err != nil { + h.t.Fatalf("Failed to get events: %v", err) + } + + expectedComment := "" + if eventType == types.EventLabelAdded { + expectedComment = "Added label: " + labelName + } else if eventType == types.EventLabelRemoved { + expectedComment = "Removed label: " + labelName + } + + for _, e := range events { + if e.EventType == eventType && e.Comment != nil && *e.Comment == expectedComment { + return + } + } + h.t.Errorf("Expected to find event %s for label %s", eventType, labelName) +} + func TestLabelCommands(t *testing.T) { tmpDir, err := os.MkdirTemp("", "bd-test-label-*") if err != nil { @@ -25,290 +130,69 @@ func TestLabelCommands(t *testing.T) { defer s.Close() ctx := context.Background() + h := &labelTestHelper{s: s, ctx: ctx, t: t} t.Run("add label to issue", func(t *testing.T) { - issue := &types.Issue{ - Title: "Test Issue", - Description: "Test description", - Priority: 1, - IssueType: types.TypeBug, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "bug", "test-user"); err != nil { - t.Fatalf("Failed to add label: %v", err) - } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(labels) != 1 { - t.Errorf("Expected 1 label, got %d", len(labels)) - } - if labels[0] != "bug" { - t.Errorf("Expected label 'bug', got '%s'", labels[0]) - } + issue := h.createIssue("Test Issue", types.TypeBug, 1) + h.addLabel(issue.ID, "bug") + h.assertLabelCount(issue.ID, 1) + h.assertHasLabel(issue.ID, "bug") }) t.Run("add multiple labels", func(t *testing.T) { - issue := &types.Issue{ - Title: "Multi Label Issue", - Description: "Test description", - Priority: 1, - IssueType: types.TypeFeature, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - + issue := h.createIssue("Multi Label Issue", types.TypeFeature, 1) labels := []string{"feature", "high-priority", "needs-review"} - for _, label := range labels { - if err := s.AddLabel(ctx, issue.ID, label, "test-user"); err != nil { - t.Fatalf("Failed to add label '%s': %v", label, err) - } - } - - gotLabels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(gotLabels) != 3 { - t.Errorf("Expected 3 labels, got %d", len(gotLabels)) - } - - labelMap := make(map[string]bool) - for _, l := range gotLabels { - labelMap[l] = true - } - - for _, expected := range labels { - if !labelMap[expected] { - t.Errorf("Expected label '%s' not found", expected) - } - } + h.addLabels(issue.ID, labels) + h.assertLabelCount(issue.ID, 3) + h.assertHasLabels(issue.ID, labels) }) t.Run("add duplicate label is idempotent", func(t *testing.T) { - issue := &types.Issue{ - Title: "Duplicate Label Test", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "duplicate", "test-user"); err != nil { - t.Fatalf("Failed to add label first time: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "duplicate", "test-user"); err != nil { - t.Fatalf("Failed to add label second time: %v", err) - } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(labels) != 1 { - t.Errorf("Expected 1 label after duplicate add, got %d", len(labels)) - } + issue := h.createIssue("Duplicate Label Test", types.TypeTask, 1) + h.addLabel(issue.ID, "duplicate") + h.addLabel(issue.ID, "duplicate") + h.assertLabelCount(issue.ID, 1) }) t.Run("remove label from issue", func(t *testing.T) { - issue := &types.Issue{ - Title: "Remove Label Test", - Priority: 1, - IssueType: types.TypeBug, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "temporary", "test-user"); err != nil { - t.Fatalf("Failed to add label: %v", err) - } - - if err := s.RemoveLabel(ctx, issue.ID, "temporary", "test-user"); err != nil { - t.Fatalf("Failed to remove label: %v", err) - } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(labels) != 0 { - t.Errorf("Expected 0 labels after removal, got %d", len(labels)) - } + issue := h.createIssue("Remove Label Test", types.TypeBug, 1) + h.addLabel(issue.ID, "temporary") + h.removeLabel(issue.ID, "temporary") + h.assertLabelCount(issue.ID, 0) }) t.Run("remove one of multiple labels", func(t *testing.T) { - issue := &types.Issue{ - Title: "Multi Remove Test", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - + issue := h.createIssue("Multi Remove Test", types.TypeTask, 1) labels := []string{"label1", "label2", "label3"} - for _, label := range labels { - if err := s.AddLabel(ctx, issue.ID, label, "test-user"); err != nil { - t.Fatalf("Failed to add label '%s': %v", label, err) - } - } - - if err := s.RemoveLabel(ctx, issue.ID, "label2", "test-user"); err != nil { - t.Fatalf("Failed to remove label: %v", err) - } - - gotLabels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(gotLabels) != 2 { - t.Errorf("Expected 2 labels, got %d", len(gotLabels)) - } - - for _, l := range gotLabels { - if l == "label2" { - t.Error("Expected label2 to be removed, but it's still there") - } - } + h.addLabels(issue.ID, labels) + h.removeLabel(issue.ID, "label2") + h.assertLabelCount(issue.ID, 2) + h.assertNotHasLabel(issue.ID, "label2") }) t.Run("remove non-existent label is no-op", func(t *testing.T) { - issue := &types.Issue{ - Title: "Remove Non-Existent Test", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "exists", "test-user"); err != nil { - t.Fatalf("Failed to add label: %v", err) - } - - if err := s.RemoveLabel(ctx, issue.ID, "does-not-exist", "test-user"); err != nil { - t.Fatalf("Failed to remove non-existent label: %v", err) - } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(labels) != 1 { - t.Errorf("Expected 1 label to remain, got %d", len(labels)) - } + issue := h.createIssue("Remove Non-Existent Test", types.TypeTask, 1) + h.addLabel(issue.ID, "exists") + h.removeLabel(issue.ID, "does-not-exist") + h.assertLabelCount(issue.ID, 1) }) t.Run("get labels for issue with no labels", func(t *testing.T) { - issue := &types.Issue{ - Title: "No Labels Test", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels: %v", err) - } - - if len(labels) != 0 { - t.Errorf("Expected 0 labels, got %d", len(labels)) - } + issue := h.createIssue("No Labels Test", types.TypeTask, 1) + h.assertLabelCount(issue.ID, 0) }) t.Run("label operations create events", func(t *testing.T) { - issue := &types.Issue{ - Title: "Event Test", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "test-label", "test-user"); err != nil { - t.Fatalf("Failed to add label: %v", err) - } - - if err := s.RemoveLabel(ctx, issue.ID, "test-label", "test-user"); err != nil { - t.Fatalf("Failed to remove label: %v", err) - } - - events, err := s.GetEvents(ctx, issue.ID, 100) - if err != nil { - t.Fatalf("Failed to get events: %v", err) - } - - foundAdd := false - foundRemove := false - for _, e := range events { - if e.EventType == types.EventLabelAdded && e.Comment != nil && *e.Comment == "Added label: test-label" { - foundAdd = true - } - if e.EventType == types.EventLabelRemoved && e.Comment != nil && *e.Comment == "Removed label: test-label" { - foundRemove = true - } - } - - if !foundAdd { - t.Error("Expected to find label_added event") - } - if !foundRemove { - t.Error("Expected to find label_removed event") - } + issue := h.createIssue("Event Test", types.TypeTask, 1) + h.addLabel(issue.ID, "test-label") + h.removeLabel(issue.ID, "test-label") + h.assertLabelEvent(issue.ID, types.EventLabelAdded, "test-label") + h.assertLabelEvent(issue.ID, types.EventLabelRemoved, "test-label") }) t.Run("labels persist after issue update", func(t *testing.T) { - issue := &types.Issue{ - Title: "Persistence Test", - Description: "Original description", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.AddLabel(ctx, issue.ID, "persistent", "test-user"); err != nil { - t.Fatalf("Failed to add label: %v", err) - } - + issue := h.createIssue("Persistence Test", types.TypeTask, 1) + h.addLabel(issue.ID, "persistent") updates := map[string]interface{}{ "description": "Updated description", "priority": 2, @@ -316,18 +200,8 @@ func TestLabelCommands(t *testing.T) { if err := s.UpdateIssue(ctx, issue.ID, updates, "test-user"); err != nil { t.Fatalf("Failed to update issue: %v", err) } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels after update: %v", err) - } - - if len(labels) != 1 { - t.Errorf("Expected 1 label after update, got %d", len(labels)) - } - if labels[0] != "persistent" { - t.Errorf("Expected label 'persistent', got '%s'", labels[0]) - } + h.assertLabelCount(issue.ID, 1) + h.assertHasLabel(issue.ID, "persistent") }) t.Run("labels work with different issue types", func(t *testing.T) { @@ -340,30 +214,11 @@ func TestLabelCommands(t *testing.T) { } for _, issueType := range issueTypes { - issue := &types.Issue{ - Title: "Type Test: " + string(issueType), - Priority: 1, - IssueType: issueType, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create %s issue: %v", issueType, err) - } - + issue := h.createIssue("Type Test: "+string(issueType), issueType, 1) labelName := "type-" + string(issueType) - if err := s.AddLabel(ctx, issue.ID, labelName, "test-user"); err != nil { - t.Fatalf("Failed to add label to %s issue: %v", issueType, err) - } - - labels, err := s.GetLabels(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get labels for %s issue: %v", issueType, err) - } - - if len(labels) != 1 || labels[0] != labelName { - t.Errorf("Label mismatch for %s issue: expected [%s], got %v", issueType, labelName, labels) - } + h.addLabel(issue.ID, labelName) + h.assertLabelCount(issue.ID, 1) + h.assertHasLabel(issue.ID, labelName) } }) } diff --git a/cmd/bd/reopen_test.go b/cmd/bd/reopen_test.go index d18f329a..a5b3def8 100644 --- a/cmd/bd/reopen_test.go +++ b/cmd/bd/reopen_test.go @@ -10,6 +10,89 @@ import ( "github.com/steveyegge/beads/internal/types" ) +type reopenTestHelper struct { + s *sqlite.SQLiteStorage + ctx context.Context + t *testing.T +} + +func (h *reopenTestHelper) createIssue(title string, issueType types.IssueType, priority int) *types.Issue { + issue := &types.Issue{ + Title: title, + Priority: priority, + IssueType: issueType, + Status: types.StatusOpen, + } + if err := h.s.CreateIssue(h.ctx, issue, "test-user"); err != nil { + h.t.Fatalf("Failed to create issue: %v", err) + } + return issue +} + +func (h *reopenTestHelper) closeIssue(issueID, reason string) { + if err := h.s.CloseIssue(h.ctx, issueID, "test-user", reason); err != nil { + h.t.Fatalf("Failed to close issue: %v", err) + } +} + +func (h *reopenTestHelper) reopenIssue(issueID string) { + updates := map[string]interface{}{ + "status": string(types.StatusOpen), + } + if err := h.s.UpdateIssue(h.ctx, issueID, updates, "test-user"); err != nil { + h.t.Fatalf("Failed to reopen issue: %v", err) + } +} + +func (h *reopenTestHelper) getIssue(issueID string) *types.Issue { + issue, err := h.s.GetIssue(h.ctx, issueID) + if err != nil { + h.t.Fatalf("Failed to get issue: %v", err) + } + return issue +} + +func (h *reopenTestHelper) addComment(issueID, comment string) { + if err := h.s.AddComment(h.ctx, issueID, "test-user", comment); err != nil { + h.t.Fatalf("Failed to add comment: %v", err) + } +} + +func (h *reopenTestHelper) assertStatus(issueID string, expected types.Status) { + issue := h.getIssue(issueID) + if issue.Status != expected { + h.t.Errorf("Expected status %s, got %s", expected, issue.Status) + } +} + +func (h *reopenTestHelper) assertClosedAtSet(issueID string) { + issue := h.getIssue(issueID) + if issue.ClosedAt == nil { + h.t.Error("Expected ClosedAt to be set") + } +} + +func (h *reopenTestHelper) assertClosedAtNil(issueID string) { + issue := h.getIssue(issueID) + if issue.ClosedAt != nil { + h.t.Errorf("Expected ClosedAt to be nil, got %v", issue.ClosedAt) + } +} + +func (h *reopenTestHelper) assertCommentEvent(issueID, comment string) { + events, err := h.s.GetEvents(h.ctx, issueID, 100) + if err != nil { + h.t.Fatalf("Failed to get events: %v", err) + } + + for _, e := range events { + if e.EventType == types.EventCommented && e.Comment != nil && *e.Comment == comment { + return + } + } + h.t.Errorf("Expected to find comment event with reason '%s'", comment) +} + func TestReopenCommand(t *testing.T) { tmpDir, err := os.MkdirTemp("", "bd-test-reopen-*") if err != nil { @@ -25,187 +108,42 @@ func TestReopenCommand(t *testing.T) { defer s.Close() ctx := context.Background() + h := &reopenTestHelper{s: s, ctx: ctx, t: t} t.Run("reopen closed issue", func(t *testing.T) { - issue := &types.Issue{ - Title: "Test Issue", - Description: "Test description", - Priority: 1, - IssueType: types.TypeBug, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.CloseIssue(ctx, issue.ID, "test-user", "Closing for test"); err != nil { - t.Fatalf("Failed to close issue: %v", err) - } - - closed, err := s.GetIssue(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get closed issue: %v", err) - } - if closed.Status != types.StatusClosed { - t.Errorf("Expected status to be closed, got %s", closed.Status) - } - if closed.ClosedAt == nil { - t.Error("Expected ClosedAt to be set") - } - - updates := map[string]interface{}{ - "status": string(types.StatusOpen), - } - if err := s.UpdateIssue(ctx, issue.ID, updates, "test-user"); err != nil { - t.Fatalf("Failed to reopen issue: %v", err) - } - - reopened, err := s.GetIssue(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get reopened issue: %v", err) - } - if reopened.Status != types.StatusOpen { - t.Errorf("Expected status to be open, got %s", reopened.Status) - } - if reopened.ClosedAt != nil { - t.Errorf("Expected ClosedAt to be nil, got %v", reopened.ClosedAt) - } + issue := h.createIssue("Test Issue", types.TypeBug, 1) + h.closeIssue(issue.ID, "Closing for test") + h.assertStatus(issue.ID, types.StatusClosed) + h.assertClosedAtSet(issue.ID) + h.reopenIssue(issue.ID) + h.assertStatus(issue.ID, types.StatusOpen) + h.assertClosedAtNil(issue.ID) }) t.Run("reopen with reason adds comment", func(t *testing.T) { - issue := &types.Issue{ - Title: "Test Issue 2", - Description: "Test description", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - if err := s.CloseIssue(ctx, issue.ID, "test-user", "Done"); err != nil { - t.Fatalf("Failed to close issue: %v", err) - } - - updates := map[string]interface{}{ - "status": string(types.StatusOpen), - } - if err := s.UpdateIssue(ctx, issue.ID, updates, "test-user"); err != nil { - t.Fatalf("Failed to reopen issue: %v", err) - } - + issue := h.createIssue("Test Issue 2", types.TypeTask, 1) + h.closeIssue(issue.ID, "Done") + h.reopenIssue(issue.ID) reason := "Found a regression" - if err := s.AddComment(ctx, issue.ID, "test-user", reason); err != nil { - t.Fatalf("Failed to add comment: %v", err) - } - - events, err := s.GetEvents(ctx, issue.ID, 100) - if err != nil { - t.Fatalf("Failed to get events: %v", err) - } - - found := false - for _, e := range events { - if e.EventType == types.EventCommented && e.Comment != nil && *e.Comment == reason { - found = true - break - } - } - if !found { - t.Errorf("Expected to find comment event with reason '%s'", reason) - } + h.addComment(issue.ID, reason) + h.assertCommentEvent(issue.ID, reason) }) t.Run("reopen multiple issues", func(t *testing.T) { - issue1 := &types.Issue{ - Title: "Multi Test 1", - Priority: 1, - IssueType: types.TypeBug, - Status: types.StatusOpen, - } - issue2 := &types.Issue{ - Title: "Multi Test 2", - Priority: 1, - IssueType: types.TypeBug, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue1, "test-user"); err != nil { - t.Fatalf("Failed to create issue1: %v", err) - } - if err := s.CreateIssue(ctx, issue2, "test-user"); err != nil { - t.Fatalf("Failed to create issue2: %v", err) - } - - if err := s.CloseIssue(ctx, issue1.ID, "test-user", "Done"); err != nil { - t.Fatalf("Failed to close issue1: %v", err) - } - if err := s.CloseIssue(ctx, issue2.ID, "test-user", "Done"); err != nil { - t.Fatalf("Failed to close issue2: %v", err) - } - - updates1 := map[string]interface{}{ - "status": string(types.StatusOpen), - } - if err := s.UpdateIssue(ctx, issue1.ID, updates1, "test-user"); err != nil { - t.Fatalf("Failed to reopen issue1: %v", err) - } - - updates2 := map[string]interface{}{ - "status": string(types.StatusOpen), - } - if err := s.UpdateIssue(ctx, issue2.ID, updates2, "test-user"); err != nil { - t.Fatalf("Failed to reopen issue2: %v", err) - } - - reopened1, err := s.GetIssue(ctx, issue1.ID) - if err != nil { - t.Fatalf("Failed to get issue1: %v", err) - } - reopened2, err := s.GetIssue(ctx, issue2.ID) - if err != nil { - t.Fatalf("Failed to get issue2: %v", err) - } - - if reopened1.Status != types.StatusOpen { - t.Errorf("Expected issue1 status to be open, got %s", reopened1.Status) - } - if reopened2.Status != types.StatusOpen { - t.Errorf("Expected issue2 status to be open, got %s", reopened2.Status) - } + issue1 := h.createIssue("Multi Test 1", types.TypeBug, 1) + issue2 := h.createIssue("Multi Test 2", types.TypeBug, 1) + h.closeIssue(issue1.ID, "Done") + h.closeIssue(issue2.ID, "Done") + h.reopenIssue(issue1.ID) + h.reopenIssue(issue2.ID) + h.assertStatus(issue1.ID, types.StatusOpen) + h.assertStatus(issue2.ID, types.StatusOpen) }) t.Run("reopen already open issue is no-op", func(t *testing.T) { - issue := &types.Issue{ - Title: "Already Open", - Priority: 1, - IssueType: types.TypeTask, - Status: types.StatusOpen, - } - - if err := s.CreateIssue(ctx, issue, "test-user"); err != nil { - t.Fatalf("Failed to create issue: %v", err) - } - - updates := map[string]interface{}{ - "status": string(types.StatusOpen), - } - if err := s.UpdateIssue(ctx, issue.ID, updates, "test-user"); err != nil { - t.Fatalf("Failed to update issue: %v", err) - } - - updated, err := s.GetIssue(ctx, issue.ID) - if err != nil { - t.Fatalf("Failed to get issue: %v", err) - } - if updated.Status != types.StatusOpen { - t.Errorf("Expected status to remain open, got %s", updated.Status) - } - if updated.ClosedAt != nil { - t.Errorf("Expected ClosedAt to remain nil, got %v", updated.ClosedAt) - } + issue := h.createIssue("Already Open", types.TypeTask, 1) + h.reopenIssue(issue.ID) + h.assertStatus(issue.ID, types.StatusOpen) + h.assertClosedAtNil(issue.ID) }) }