From 664619dcb9596c8ef9fa8e680b4eeea6040d4d5b Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 11:59:42 -0500 Subject: [PATCH 01/10] Initial pass on addressing review feedback --- cmd/roborev/main.go | 9 ++++++ cmd/roborev/tui.go | 12 ++----- cmd/roborev/tui_test.go | 54 ++++++++++++++++++++++++++++++++ internal/storage/reviews_test.go | 48 ++++++++++++++++++++++++++++ 4 files changed, 113 insertions(+), 10 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 6b70fd6..e9102bb 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -2352,6 +2352,15 @@ func shortJobRef(job storage.ReviewJob) string { return shortRef(job.GitRef) } +// formatAgentLabel returns the agent display string, including model if set. +// Format: "agent" or "agent: model" +func formatAgentLabel(agent string, model string) string { + if model != "" { + return fmt.Sprintf("%s: %s", agent, model) + } + return agent +} + // generateHookContent creates the post-commit hook script content. // It bakes the path to the currently running binary for consistency. // Falls back to PATH lookup if the baked path becomes unavailable. diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 6a7e14a..ad57e8f 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -2926,11 +2926,7 @@ func (m tuiModel) renderReviewView() string { } repoStr := m.getDisplayName(review.Job.RepoPath, defaultName) - // Build agent string, including model if explicitly set - agentStr := review.Agent - if review.Job.Model != "" { - agentStr = fmt.Sprintf("%s: %s", review.Agent, review.Job.Model) - } + agentStr := formatAgentLabel(review.Agent, review.Job.Model) title = fmt.Sprintf("Review %s%s (%s)", idStr, repoStr, agentStr) titleLen = len(title) @@ -3084,11 +3080,7 @@ func (m tuiModel) renderPromptView() string { review := m.currentReview if review.Job != nil { ref := shortJobRef(*review.Job) - // Build agent string, including model if explicitly set - agentStr := review.Agent - if review.Job.Model != "" { - agentStr = fmt.Sprintf("%s: %s", review.Agent, review.Job.Model) - } + agentStr := formatAgentLabel(review.Agent, review.Job.Model) title := fmt.Sprintf("Prompt: %s (%s)", ref, agentStr) b.WriteString(tuiTitleStyle.Render(title)) } else { diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 5299d4f..ec6a4ff 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -3925,6 +3925,60 @@ func TestTUIRenderReviewViewWithoutModel(t *testing.T) { } } +func TestTUIRenderPromptViewWithModel(t *testing.T) { + m := newTuiModel("http://localhost") + m.width = 100 + m.height = 30 + m.currentView = tuiViewPrompt + m.currentReview = &storage.Review{ + ID: 10, + Agent: "codex", + Prompt: "Review this code", + Job: &storage.ReviewJob{ + ID: 1, + GitRef: "abc1234", + Agent: "codex", + Model: "o3", + }, + } + + output := m.View() + + // Should contain agent with model in format "(codex: o3)" + if !strings.Contains(output, "(codex: o3)") { + t.Errorf("Expected Prompt view to contain '(codex: o3)', got:\n%s", output) + } +} + +func TestTUIRenderPromptViewWithoutModel(t *testing.T) { + m := newTuiModel("http://localhost") + m.width = 100 + m.height = 30 + m.currentView = tuiViewPrompt + m.currentReview = &storage.Review{ + ID: 10, + Agent: "codex", + Prompt: "Review this code", + Job: &storage.ReviewJob{ + ID: 1, + GitRef: "abc1234", + Agent: "codex", + Model: "", // No model set + }, + } + + output := m.View() + + // Should contain just the agent "(codex)" without model + if !strings.Contains(output, "(codex)") { + t.Errorf("Expected Prompt view to contain '(codex)', got:\n%s", output) + } + // Should NOT contain the colon separator that would indicate a model + if strings.Contains(output, "(codex:") { + t.Error("Expected Prompt view NOT to contain '(codex:' when no model is set") + } +} + func TestTUIRenderReviewViewNoBranchForRange(t *testing.T) { m := newTuiModel("http://localhost") m.width = 100 diff --git a/internal/storage/reviews_test.go b/internal/storage/reviews_test.go index 7b34820..acd3f99 100644 --- a/internal/storage/reviews_test.go +++ b/internal/storage/reviews_test.go @@ -234,6 +234,54 @@ func TestAddCommentToJobWithNoReview(t *testing.T) { } } +func TestGetReviewByJobIDIncludesModel(t *testing.T) { + db := openReviewsTestDB(t) + defer db.Close() + + repo, err := db.GetOrCreateRepo("/tmp/test-repo") + if err != nil { + t.Fatalf("GetOrCreateRepo failed: %v", err) + } + + tests := []struct { + name string + gitRef string + model string + expectedModel string + }{ + {"model is populated when set", "abc123", "o3", "o3"}, + {"model is empty when not set", "def456", "", ""}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + job, err := db.EnqueueRangeJob(repo.ID, tt.gitRef, "codex", tt.model, "thorough") + if err != nil { + t.Fatalf("EnqueueRangeJob failed: %v", err) + } + + // Claim job to move to running, then complete it + db.ClaimJob("test-worker") + err = db.CompleteJob(job.ID, "codex", "test prompt", "Test review output\n\n## Verdict: PASS") + if err != nil { + t.Fatalf("CompleteJob failed: %v", err) + } + + review, err := db.GetReviewByJobID(job.ID) + if err != nil { + t.Fatalf("GetReviewByJobID failed: %v", err) + } + + if review.Job == nil { + t.Fatal("Expected review.Job to be populated") + } + if review.Job.Model != tt.expectedModel { + t.Errorf("Expected model %q, got %q", tt.expectedModel, review.Job.Model) + } + }) + } +} + func openReviewsTestDB(t *testing.T) *DB { tmpDir := t.TempDir() dbPath := filepath.Join(tmpDir, "test.db") From d3c9de528db434dca36ef51b3e080f91396c7f8a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:05:30 -0500 Subject: [PATCH 02/10] Fix location line wrapping and add addressed-without-verdict test - Track locationLineLen and compute wrapped line count for header height calculation, fixing scroll indicator accuracy with long paths/branches - Add TestTUIRenderReviewViewAddressedWithoutVerdict for line ordering - Update TestTUIVisibleLinesCalculationLongTitleWraps for new wrapping Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 30 +++++++++++++-------- cmd/roborev/tui_test.go | 60 ++++++++++++++++++++++++++++++++++++----- 2 files changed, 72 insertions(+), 18 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index ad57e8f..760f122 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -2916,6 +2916,7 @@ func (m tuiModel) renderReviewView() string { // Build title string and compute its length for line calculation var title string var titleLen int + var locationLineLen int if review.Job != nil { ref := shortJobRef(*review.Job) idStr := fmt.Sprintf("#%d ", review.Job.ID) @@ -2936,20 +2937,21 @@ func (m tuiModel) renderReviewView() string { // Show location line: repo path (or identity/name), git ref, and branch b.WriteString("\n") - pathLine := review.Job.RepoPath - if pathLine == "" { + locationLine := review.Job.RepoPath + if locationLine == "" { // No local path - use repo name/identity as fallback - pathLine = review.Job.RepoName + locationLine = review.Job.RepoName } - if pathLine != "" { - pathLine += " " + ref + if locationLine != "" { + locationLine += " " + ref } else { - pathLine = ref + locationLine = ref } if m.currentBranch != "" { - pathLine += " on " + m.currentBranch + locationLine += " on " + m.currentBranch } - b.WriteString(tuiStatusStyle.Render(pathLine)) + locationLineLen = len(locationLine) + b.WriteString(tuiStatusStyle.Render(locationLine)) b.WriteString("\x1b[K") // Clear to end of line // Show verdict and addressed status on next line @@ -3013,11 +3015,17 @@ func (m tuiModel) renderReviewView() string { helpLines = (len(helpText) + m.width - 1) / m.width } - // headerHeight = title + location line (1) + status line (1) + help + verdict/addressed (0|1) - headerHeight := titleLines + 1 + helpLines + // Compute location line count (repo path + ref + branch can wrap) + locationLines := 0 if review.Job != nil { - headerHeight++ // Add 1 for location line (repo path + ref + branch) + locationLines = 1 + if m.width > 0 && locationLineLen > m.width { + locationLines = (locationLineLen + m.width - 1) / m.width + } } + + // headerHeight = title + location line + status line (1) + help + verdict/addressed (0|1) + headerHeight := titleLines + locationLines + 1 + helpLines hasVerdict := review.Job != nil && review.Job.Verdict != nil && *review.Job.Verdict != "" if hasVerdict || review.Addressed { headerHeight++ // Add 1 for verdict/addressed line diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index ec6a4ff..1e6531b 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -4087,6 +4087,52 @@ func TestTUIRenderReviewViewVerdictOnLine2(t *testing.T) { } } +func TestTUIRenderReviewViewAddressedWithoutVerdict(t *testing.T) { + // Test that [ADDRESSED] appears on line 2 when no verdict is present + m := newTuiModel("http://localhost") + m.width = 100 + m.height = 30 + m.currentView = tuiViewReview + m.currentReview = &storage.Review{ + ID: 10, + Output: "Line 1\nLine 2\nLine 3", + Addressed: true, + Job: &storage.ReviewJob{ + ID: 1, + GitRef: "abc1234", + RepoName: "myrepo", + Agent: "codex", + Verdict: nil, // No verdict + }, + } + + output := m.View() + lines := strings.Split(output, "\n") + + // Line 0: Title + if !strings.Contains(lines[0], "Review") { + t.Errorf("Line 0 should contain 'Review', got: %s", lines[0]) + } + + // Line 1: Location (ref) + if len(lines) > 1 && !strings.Contains(lines[1], "abc1234") { + t.Errorf("Line 1 should contain ref 'abc1234', got: %s", lines[1]) + } + + // Line 2: [ADDRESSED] (no verdict) + if len(lines) > 2 && !strings.Contains(lines[2], "[ADDRESSED]") { + t.Errorf("Line 2 should contain '[ADDRESSED]', got: %s", lines[2]) + } + if len(lines) > 2 && strings.Contains(lines[2], "Verdict") { + t.Errorf("Line 2 should not contain 'Verdict' when no verdict is set, got: %s", lines[2]) + } + + // Line 3: Content + if len(lines) > 3 && !strings.Contains(lines[3], "Line 1") { + t.Errorf("Line 3 should contain content 'Line 1', got: %s", lines[3]) + } +} + func TestTUIBranchClearedOnFailedJobNavigation(t *testing.T) { // Test that navigating from a successful review with branch to a failed job clears the branch m := newTuiModel("http://localhost") @@ -4355,15 +4401,15 @@ func TestTUIVisibleLinesCalculationNarrowTerminalWithVerdict(t *testing.T) { } func TestTUIVisibleLinesCalculationLongTitleWraps(t *testing.T) { - // Test that long titles correctly wrap and reduce visible lines + // Test that long titles and location lines correctly wrap and reduce visible lines // New layout: // - Title: "Review #1 very-long-repository-name-here (claude-code)" = ~54 chars, ceil(54/50) = 2 lines - // - Location line: repo + ref + branch = 1 line + // - Location line: "very-long-repository-name-here abc1234567890..de on feature/very-long-branch-name" = 81 chars, ceil(81/50) = 2 lines // - Addressed line: 1 line (since Addressed=true) // - Status line: 1 line // - Help: ceil(91/50) = 2 lines - // Non-content: 2 + 1 + 1 + 1 + 2 = 7 - // visibleLines = 12 - 7 = 5 + // Non-content: 2 + 2 + 1 + 1 + 2 = 8 + // visibleLines = 12 - 8 = 4 m := newTuiModel("http://localhost") m.width = 50 m.height = 12 @@ -4392,14 +4438,14 @@ func TestTUIVisibleLinesCalculationLongTitleWraps(t *testing.T) { } } - expectedContent := 5 + expectedContent := 4 if contentCount != expectedContent { t.Errorf("Expected %d content lines with long wrapping title, got %d", expectedContent, contentCount) } // Should show scroll indicator with correct range - if !strings.Contains(output, "[1-5 of 20 lines]") { - t.Errorf("Expected scroll indicator '[1-5 of 20 lines]', output: %s", output) + if !strings.Contains(output, "[1-4 of 20 lines]") { + t.Errorf("Expected scroll indicator '[1-4 of 20 lines]', output: %s", output) } // Should contain the long repo name and branch From 3d7fbb08f56a6faccee489a56ba8ddba5e63496e Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:11:28 -0500 Subject: [PATCH 03/10] Use runewidth for display width and improve test robustness - Use runewidth.StringWidth instead of len() for titleLen and locationLineLen to correctly handle non-ASCII characters - Add upfront line count check in TestTUIRenderReviewViewAddressedWithoutVerdict to fail fast instead of silently passing with too-short output Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 4 ++-- cmd/roborev/tui_test.go | 13 +++++++++---- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 760f122..1bc5c95 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -2930,7 +2930,7 @@ func (m tuiModel) renderReviewView() string { agentStr := formatAgentLabel(review.Agent, review.Job.Model) title = fmt.Sprintf("Review %s%s (%s)", idStr, repoStr, agentStr) - titleLen = len(title) + titleLen = runewidth.StringWidth(title) b.WriteString(tuiTitleStyle.Render(title)) b.WriteString("\x1b[K") // Clear to end of line @@ -2950,7 +2950,7 @@ func (m tuiModel) renderReviewView() string { if m.currentBranch != "" { locationLine += " on " + m.currentBranch } - locationLineLen = len(locationLine) + locationLineLen = runewidth.StringWidth(locationLine) b.WriteString(tuiStatusStyle.Render(locationLine)) b.WriteString("\x1b[K") // Clear to end of line diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 1e6531b..9eb1363 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -4109,26 +4109,31 @@ func TestTUIRenderReviewViewAddressedWithoutVerdict(t *testing.T) { output := m.View() lines := strings.Split(output, "\n") + // Require at least 4 lines: title, location, addressed, content + if len(lines) < 4 { + t.Fatalf("Expected at least 4 lines, got %d:\n%s", len(lines), output) + } + // Line 0: Title if !strings.Contains(lines[0], "Review") { t.Errorf("Line 0 should contain 'Review', got: %s", lines[0]) } // Line 1: Location (ref) - if len(lines) > 1 && !strings.Contains(lines[1], "abc1234") { + if !strings.Contains(lines[1], "abc1234") { t.Errorf("Line 1 should contain ref 'abc1234', got: %s", lines[1]) } // Line 2: [ADDRESSED] (no verdict) - if len(lines) > 2 && !strings.Contains(lines[2], "[ADDRESSED]") { + if !strings.Contains(lines[2], "[ADDRESSED]") { t.Errorf("Line 2 should contain '[ADDRESSED]', got: %s", lines[2]) } - if len(lines) > 2 && strings.Contains(lines[2], "Verdict") { + if strings.Contains(lines[2], "Verdict") { t.Errorf("Line 2 should not contain 'Verdict' when no verdict is set, got: %s", lines[2]) } // Line 3: Content - if len(lines) > 3 && !strings.Contains(lines[3], "Line 1") { + if !strings.Contains(lines[3], "Line 1") { t.Errorf("Line 3 should contain content 'Line 1', got: %s", lines[3]) } } From 6e3361bb950aae15f43e3a7b2359bfbe5abc303b Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:21:27 -0500 Subject: [PATCH 04/10] Isolate tests from production environment - Add setupTuiTestEnv helper to set ROBOREV_DATA_DIR to temp directory, preventing TUI tests from reading production daemon.json - Add testServerAddr constant to replace hardcoded localhost:7373 - Fix TestDaemonRunStartsAndShutdownsCleanly to properly stop daemon using SIGTERM via ListAllRuntimes() to find daemon PID - Remove os.Exit(0) from daemon shutdown handler to allow proper test cleanup (Go 1.25 panics on os.Exit during tests) This prevents test daemons from polluting production roborev environment and ensures tests don't leave zombie daemon processes. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main.go | 3 +- cmd/roborev/main_test.go | 39 ++++++++++++++++++++++++-- cmd/roborev/tui_test.go | 60 +++++++++++++++++++++++++++------------- 3 files changed, 79 insertions(+), 23 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index e9102bb..26ebd06 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -542,7 +542,8 @@ func daemonRunCmd() *cobra.Command { if err := server.Stop(); err != nil { log.Printf("Shutdown error: %v", err) } - os.Exit(0) + // Note: Don't call os.Exit here - let server.Start() return naturally + // after Stop() is called. This allows proper cleanup and testability. }() // Start server (blocks until shutdown) diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 76de432..83b0fbe 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -19,6 +19,7 @@ import ( "strings" "sync" "sync/atomic" + "syscall" "testing" "time" @@ -1510,9 +1511,41 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { // Daemon is still running - good } - // The daemon is blocking in server.Start(), so we can't easily stop it - // without sending a signal. For this unit test, we just verify it started. - // A full integration test would require a separate process. + // Find the daemon's runtime file (daemon..json) to get PID + var info *daemon.RuntimeInfo + deadline = time.Now().Add(2 * time.Second) + for time.Now().Before(deadline) { + runtimes, err := daemon.ListAllRuntimes() + if err == nil && len(runtimes) > 0 { + info = runtimes[0] + break + } + time.Sleep(50 * time.Millisecond) + } + if info == nil { + t.Fatal("daemon did not create runtime file") + } + + // Send SIGTERM to stop the daemon gracefully + if info.PID > 0 { + proc, err := os.FindProcess(info.PID) + if err == nil { + proc.Signal(syscall.SIGTERM) + } + } + + // Wait for daemon to exit + select { + case <-errCh: + // Daemon exited - good + case <-time.After(5 * time.Second): + // Force kill if still running + if info.PID > 0 { + if proc, err := os.FindProcess(info.PID); err == nil { + proc.Kill() + } + } + } } // TestDaemonStopNotRunning verifies daemon stop reports when no daemon is running diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 9eb1363..e47b733 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -7,6 +7,7 @@ import ( "net/http" "net/http/httptest" "net/url" + "os" "regexp" "strings" "testing" @@ -18,9 +19,30 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) +// testServerAddr is a placeholder address used in tests that don't make real HTTP calls. +// Tests that need actual HTTP should use httptest.NewServer and pass ts.URL. +const testServerAddr = "http://test.invalid:9999" + +// setupTuiTestEnv isolates the test from the production roborev environment +// by setting ROBOREV_DATA_DIR to a temp directory. This prevents tests from +// reading production daemon.json or affecting production state. +func setupTuiTestEnv(t *testing.T) { + t.Helper() + tmpDir := t.TempDir() + origDataDir := os.Getenv("ROBOREV_DATA_DIR") + os.Setenv("ROBOREV_DATA_DIR", tmpDir) + t.Cleanup(func() { + if origDataDir != "" { + os.Setenv("ROBOREV_DATA_DIR", origDataDir) + } else { + os.Unsetenv("ROBOREV_DATA_DIR") + } + }) +} + // mockConnError creates a connection error (url.Error) for testing func mockConnError(msg string) error { - return &url.Error{Op: "Get", URL: "http://localhost:7373", Err: errors.New(msg)} + return &url.Error{Op: "Get", URL: testServerAddr, Err: errors.New(msg)} } // stripANSI removes ANSI escape sequences from a string @@ -6240,7 +6262,7 @@ func TestFormatClipboardContent(t *testing.T) { } func TestTUIConfigReloadFlash(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) t.Run("no flash on first status fetch", func(t *testing.T) { // First status fetch with a ConfigReloadCounter should NOT flash @@ -6265,7 +6287,7 @@ func TestTUIConfigReloadFlash(t *testing.T) { t.Run("flash on config reload after first fetch", func(t *testing.T) { // Start with a model that has already fetched status once - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.statusFetchedOnce = true m.lastConfigReloadCounter = 1 @@ -6288,7 +6310,7 @@ func TestTUIConfigReloadFlash(t *testing.T) { t.Run("flash when ConfigReloadCounter changes from zero to non-zero", func(t *testing.T) { // Model has fetched status once but daemon hadn't reloaded yet - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.statusFetchedOnce = true m.lastConfigReloadCounter = 0 // No reload had occurred @@ -6307,7 +6329,7 @@ func TestTUIConfigReloadFlash(t *testing.T) { }) t.Run("no flash when ConfigReloadCounter unchanged", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.statusFetchedOnce = true m.lastConfigReloadCounter = 1 @@ -6328,7 +6350,7 @@ func TestTUIConfigReloadFlash(t *testing.T) { func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { t.Run("triggers reconnection after 3 consecutive connection errors", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // Already had 2 connection errors // Third connection error should trigger reconnection @@ -6347,7 +6369,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("does not trigger reconnection before 3 errors", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 1 // Only 1 error so far updated, cmd := m.Update(tuiJobsErrMsg{err: mockConnError("connection refused")}) @@ -6365,7 +6387,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("does not count application errors for reconnection", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // 2 connection errors // Application error (404, parse error, etc.) should not increment counter @@ -6384,7 +6406,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("does not count non-connection errors in jobs fetch", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // Non-connection error (like parse error) should not increment @@ -6397,7 +6419,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("pagination errors also trigger reconnection", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // Connection error in pagination should trigger reconnection @@ -6416,7 +6438,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("status/review connection errors trigger reconnection", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // Connection error via tuiErrMsg (from fetchStatus, fetchReview, etc.) @@ -6435,7 +6457,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("status/review application errors do not trigger reconnection", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // Application error via tuiErrMsg should not increment @@ -6454,7 +6476,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("resets error count on successful jobs fetch", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 5 updated, _ := m.Update(tuiJobsMsg{jobs: []storage.ReviewJob{}, hasMore: false}) @@ -6466,7 +6488,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("resets error count on successful status fetch", func(t *testing.T) { - m := newTuiModel("http://localhost:7373") + m := newTuiModel(testServerAddr) m.consecutiveErrors = 5 updated, _ := m.Update(tuiStatusMsg(storage.DaemonStatus{Version: "1.0.0"})) @@ -6478,7 +6500,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("updates server address on successful reconnection", func(t *testing.T) { - m := newTuiModel("http://127.0.0.1:7373") + m := newTuiModel(testServerAddr) m.reconnecting = true updated, cmd := m.Update(tuiReconnectMsg{newAddr: "http://127.0.0.1:7374", version: "2.0.0"}) @@ -6505,15 +6527,15 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("handles reconnection to same address", func(t *testing.T) { - m := newTuiModel("http://127.0.0.1:7373") + m := newTuiModel(testServerAddr) m.reconnecting = true m.consecutiveErrors = 3 // Same address - no change needed - updated, cmd := m.Update(tuiReconnectMsg{newAddr: "http://127.0.0.1:7373"}) + updated, cmd := m.Update(tuiReconnectMsg{newAddr: testServerAddr}) m2 := updated.(tuiModel) - if m2.serverAddr != "http://127.0.0.1:7373" { + if m2.serverAddr != testServerAddr { t.Errorf("Expected serverAddr unchanged, got %s", m2.serverAddr) } if m2.reconnecting { @@ -6526,7 +6548,7 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { }) t.Run("handles failed reconnection", func(t *testing.T) { - m := newTuiModel("http://127.0.0.1:7373") + m := newTuiModel(testServerAddr) m.reconnecting = true m.consecutiveErrors = 3 From 3fb075dfad98a160a4bc9412a1c1a3b40ff0f8ba Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:28:46 -0500 Subject: [PATCH 05/10] Fix daemon test self-signal and add setupTuiTestEnv calls - Use os.Interrupt instead of syscall.SIGTERM for cross-platform support - Guard against signaling non-self PIDs (daemon runs in test process) - Remove force-kill fallback that could kill the test process - Call setupTuiTestEnv(t) in TestTUIConfigReloadFlash and TestTUIReconnectOnConsecutiveErrors to isolate from production env Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main_test.go | 19 ++++++++----------- cmd/roborev/tui_test.go | 2 ++ 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 83b0fbe..2860fe4 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -19,7 +19,6 @@ import ( "strings" "sync" "sync/atomic" - "syscall" "testing" "time" @@ -1526,25 +1525,23 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { t.Fatal("daemon did not create runtime file") } - // Send SIGTERM to stop the daemon gracefully - if info.PID > 0 { + // The daemon runs in a goroutine within this test process, so info.PID + // will be os.Getpid(). Send os.Interrupt (works on all platforms) to + // trigger the daemon's signal handler for graceful shutdown. + if info.PID > 0 && info.PID == os.Getpid() { proc, err := os.FindProcess(info.PID) if err == nil { - proc.Signal(syscall.SIGTERM) + // Use os.Interrupt - registered on all platforms including Windows + proc.Signal(os.Interrupt) } } - // Wait for daemon to exit + // Wait for daemon to exit (don't force-kill our own process) select { case <-errCh: // Daemon exited - good case <-time.After(5 * time.Second): - // Force kill if still running - if info.PID > 0 { - if proc, err := os.FindProcess(info.PID); err == nil { - proc.Kill() - } - } + t.Log("Warning: daemon did not exit within timeout") } } diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index e47b733..42a0f27 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -6262,6 +6262,7 @@ func TestFormatClipboardContent(t *testing.T) { } func TestTUIConfigReloadFlash(t *testing.T) { + setupTuiTestEnv(t) m := newTuiModel(testServerAddr) t.Run("no flash on first status fetch", func(t *testing.T) { @@ -6349,6 +6350,7 @@ func TestTUIConfigReloadFlash(t *testing.T) { } func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { + setupTuiTestEnv(t) t.Run("triggers reconnection after 3 consecutive connection errors", func(t *testing.T) { m := newTuiModel(testServerAddr) m.consecutiveErrors = 2 // Already had 2 connection errors From dcddb1f2856e0d022a1df451e355e4f59d7696b9 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:33:08 -0500 Subject: [PATCH 06/10] Make daemon test signal any PID and fail on timeout - Remove self-PID check to handle future refactors where daemon may run as separate process - Change timeout from t.Log to t.Fatal to ensure test failures don't leak daemon goroutines into subsequent tests Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main_test.go | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 2860fe4..99a2bc1 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -1525,23 +1525,21 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { t.Fatal("daemon did not create runtime file") } - // The daemon runs in a goroutine within this test process, so info.PID - // will be os.Getpid(). Send os.Interrupt (works on all platforms) to - // trigger the daemon's signal handler for graceful shutdown. - if info.PID > 0 && info.PID == os.Getpid() { + // Send os.Interrupt to trigger the daemon's signal handler for graceful shutdown. + // Works whether daemon runs in-process (current design) or as separate process (future). + if info.PID > 0 { proc, err := os.FindProcess(info.PID) if err == nil { - // Use os.Interrupt - registered on all platforms including Windows proc.Signal(os.Interrupt) } } - // Wait for daemon to exit (don't force-kill our own process) + // Wait for daemon to exit select { case <-errCh: // Daemon exited - good case <-time.After(5 * time.Second): - t.Log("Warning: daemon did not exit within timeout") + t.Fatal("daemon did not exit within 5 second timeout") } } From 9ae476d1e3319b5b29de8df7a74aca931fbb573f Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:43:03 -0500 Subject: [PATCH 07/10] Add TUI version mismatch detection and warning banner When the TUI connects to a daemon running a different version, display a persistent red error banner: "VERSION MISMATCH: TUI X != Daemon Y - restart TUI or daemon". This prevents confusing behavior when stale binaries are used (e.g., from baked-in post-commit hook paths). - Add versionMismatch field to tuiModel - Check for mismatch when receiving daemon status - Display error banner in queue and review views - Add TestTUIVersionMismatchDetection with 4 test cases Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 17 +++++++++ cmd/roborev/tui_test.go | 83 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 1bc5c95..693bc3a 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -125,6 +125,7 @@ type tuiModel struct { err error updateAvailable string // Latest version if update available, empty if up to date updateIsDevBuild bool // True if running a dev build + versionMismatch bool // True if daemon version doesn't match TUI version // Pagination state hasMore bool // true if there are more jobs to load @@ -2223,6 +2224,8 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.consecutiveErrors = 0 // Reset on successful fetch if m.status.Version != "" { m.daemonVersion = m.status.Version + // Check for version mismatch between TUI and daemon + m.versionMismatch = m.daemonVersion != version.Version } // Show flash notification when config is reloaded // Use counter (not timestamp) to detect reloads that happen within the same second @@ -2710,6 +2713,13 @@ func (m tuiModel) renderQueueView() string { } b.WriteString("\x1b[K\n") // Clear to end of line + // Version mismatch error (persistent, red) + if m.versionMismatch { + errorStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("196")).Bold(true) // Bright red + b.WriteString(errorStyle.Render(fmt.Sprintf("VERSION MISMATCH: TUI %s != Daemon %s - restart TUI or daemon", version.Version, m.daemonVersion))) + b.WriteString("\x1b[K\n") + } + // Help (two lines) helpLine1 := "↑/↓: navigate | enter: review | y: copy | m: commit msg | q: quit | ?: help" helpLine2 := "f: filter | h: hide addressed | a: toggle addressed | x: cancel" @@ -3072,6 +3082,13 @@ func (m tuiModel) renderReviewView() string { } b.WriteString("\x1b[K\n") // Clear status line + // Version mismatch error (persistent, red) + if m.versionMismatch { + errorStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("196")).Bold(true) + b.WriteString(errorStyle.Render(fmt.Sprintf("VERSION MISMATCH: TUI %s != Daemon %s - restart TUI or daemon", version.Version, m.daemonVersion))) + b.WriteString("\x1b[K\n") + } + b.WriteString(tuiHelpStyle.Render(helpText)) b.WriteString("\x1b[K") // Clear help line b.WriteString("\x1b[J") // Clear to end of screen to prevent artifacts diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 42a0f27..759f358 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -17,6 +17,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "github.com/mattn/go-runewidth" "github.com/roborev-dev/roborev/internal/storage" + "github.com/roborev-dev/roborev/internal/version" ) // testServerAddr is a placeholder address used in tests that don't make real HTTP calls. @@ -6261,6 +6262,88 @@ func TestFormatClipboardContent(t *testing.T) { } } +func TestTUIVersionMismatchDetection(t *testing.T) { + setupTuiTestEnv(t) + + t.Run("detects version mismatch", func(t *testing.T) { + m := newTuiModel(testServerAddr) + + // Simulate receiving status with different version + status := tuiStatusMsg(storage.DaemonStatus{ + Version: "different-version", + }) + + updated, _ := m.Update(status) + m2 := updated.(tuiModel) + + if !m2.versionMismatch { + t.Error("Expected versionMismatch=true when daemon version differs") + } + if m2.daemonVersion != "different-version" { + t.Errorf("Expected daemonVersion='different-version', got %q", m2.daemonVersion) + } + }) + + t.Run("no mismatch when versions match", func(t *testing.T) { + m := newTuiModel(testServerAddr) + + // Simulate receiving status with same version as TUI + status := tuiStatusMsg(storage.DaemonStatus{ + Version: version.Version, + }) + + updated, _ := m.Update(status) + m2 := updated.(tuiModel) + + if m2.versionMismatch { + t.Error("Expected versionMismatch=false when versions match") + } + }) + + t.Run("displays error banner in queue view when mismatched", func(t *testing.T) { + m := newTuiModel(testServerAddr) + m.width = 100 + m.height = 30 + m.currentView = tuiViewQueue + m.versionMismatch = true + m.daemonVersion = "old-version" + + output := m.View() + + if !strings.Contains(output, "VERSION MISMATCH") { + t.Error("Expected queue view to show VERSION MISMATCH error") + } + if !strings.Contains(output, "old-version") { + t.Error("Expected error to show daemon version") + } + }) + + t.Run("displays error banner in review view when mismatched", func(t *testing.T) { + m := newTuiModel(testServerAddr) + m.width = 100 + m.height = 30 + m.currentView = tuiViewReview + m.versionMismatch = true + m.daemonVersion = "old-version" + m.currentReview = &storage.Review{ + ID: 1, + Output: "Test review", + Job: &storage.ReviewJob{ + ID: 1, + GitRef: "abc123", + RepoName: "test", + Agent: "test", + }, + } + + output := m.View() + + if !strings.Contains(output, "VERSION MISMATCH") { + t.Error("Expected review view to show VERSION MISMATCH error") + } + }) +} + func TestTUIConfigReloadFlash(t *testing.T) { setupTuiTestEnv(t) m := newTuiModel(testServerAddr) From b15dbd67f2c2e869ab4ecb01353b62c262d3d2bb Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:49:33 -0500 Subject: [PATCH 08/10] Improve daemon test: verify HTTP alive, signal own process explicitly - Use a specific port (17373) instead of port 0 which doesn't work correctly with FindAvailablePort - Wait for daemon to be HTTP-responsive before attempting shutdown - Explicitly signal os.Getpid() with comment explaining why PID reuse is not a concern (daemon runs in same process as test) Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main_test.go | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 99a2bc1..3e05e1e 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -1475,11 +1475,12 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { } // Create the daemon run command with custom flags + // Use a high port number to avoid conflicts (FindAvailablePort will increment if busy) cmd := daemonRunCmd() cmd.SetArgs([]string{ "--db", dbPath, "--config", configPath, - "--addr", "127.0.0.1:0", // Use port 0 to get a free port + "--addr", "127.0.0.1:17373", }) // Run daemon in goroutine @@ -1510,29 +1511,37 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { // Daemon is still running - good } - // Find the daemon's runtime file (daemon..json) to get PID + // Wait for daemon to be fully started and responsive + // The runtime file is written before ListenAndServe, so we need to verify + // the HTTP server is actually accepting connections var info *daemon.RuntimeInfo - deadline = time.Now().Add(2 * time.Second) + deadline = time.Now().Add(5 * time.Second) for time.Now().Before(deadline) { runtimes, err := daemon.ListAllRuntimes() if err == nil && len(runtimes) > 0 { info = runtimes[0] - break + // Verify daemon is actually responding to HTTP + if daemon.IsDaemonAlive(info.Addr) { + break + } } time.Sleep(50 * time.Millisecond) } if info == nil { t.Fatal("daemon did not create runtime file") } + if !daemon.IsDaemonAlive(info.Addr) { + t.Fatal("daemon is not responding to HTTP requests") + } - // Send os.Interrupt to trigger the daemon's signal handler for graceful shutdown. - // Works whether daemon runs in-process (current design) or as separate process (future). - if info.PID > 0 { - proc, err := os.FindProcess(info.PID) - if err == nil { - proc.Signal(os.Interrupt) - } + // The daemon runs in a goroutine within this test process, so info.PID is + // guaranteed to be os.Getpid(). Use os.Interrupt to trigger the signal handler. + // PID reuse is not a concern since we're signaling our own process. + proc, err := os.FindProcess(os.Getpid()) + if err != nil { + t.Fatalf("failed to find own process: %v", err) } + proc.Signal(os.Interrupt) // Wait for daemon to exit select { From f0e737245c844c890f17867971bb7dd21c65e8c5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:53:57 -0500 Subject: [PATCH 09/10] Fix daemon test: filter by PID, check signal error - Filter runtimes by PID == os.Getpid() to avoid matching stale runtime files from other processes - Check and fail on proc.Signal error - Clarify comment about FindAvailablePort auto-incrementing Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main_test.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 3e05e1e..35979e6 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -1475,7 +1475,8 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { } // Create the daemon run command with custom flags - // Use a high port number to avoid conflicts (FindAvailablePort will increment if busy) + // Use a high base port to avoid conflicts with production (7373). + // FindAvailablePort will auto-increment if 17373 is busy. cmd := daemonRunCmd() cmd.SetArgs([]string{ "--db", dbPath, @@ -1515,33 +1516,37 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { // The runtime file is written before ListenAndServe, so we need to verify // the HTTP server is actually accepting connections var info *daemon.RuntimeInfo + myPID := os.Getpid() deadline = time.Now().Add(5 * time.Second) for time.Now().Before(deadline) { runtimes, err := daemon.ListAllRuntimes() - if err == nil && len(runtimes) > 0 { - info = runtimes[0] - // Verify daemon is actually responding to HTTP - if daemon.IsDaemonAlive(info.Addr) { + if err == nil { + // Find the runtime for OUR daemon (matching our PID), not a stale one + for _, rt := range runtimes { + if rt.PID == myPID && daemon.IsDaemonAlive(rt.Addr) { + info = rt + break + } + } + if info != nil { break } } time.Sleep(50 * time.Millisecond) } if info == nil { - t.Fatal("daemon did not create runtime file") - } - if !daemon.IsDaemonAlive(info.Addr) { - t.Fatal("daemon is not responding to HTTP requests") + t.Fatal("daemon did not create runtime file or is not responding") } - // The daemon runs in a goroutine within this test process, so info.PID is - // guaranteed to be os.Getpid(). Use os.Interrupt to trigger the signal handler. - // PID reuse is not a concern since we're signaling our own process. - proc, err := os.FindProcess(os.Getpid()) + // The daemon runs in a goroutine within this test process. + // Use os.Interrupt to trigger the signal handler. + proc, err := os.FindProcess(myPID) if err != nil { t.Fatalf("failed to find own process: %v", err) } - proc.Signal(os.Interrupt) + if err := proc.Signal(os.Interrupt); err != nil { + t.Fatalf("failed to send interrupt signal: %v", err) + } // Wait for daemon to exit select { From dfa47c6ea56f7d47dde3291a7c22170d207d4d13 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Mon, 26 Jan 2026 12:55:18 -0500 Subject: [PATCH 10/10] Increase daemon test timeouts for race detector Race detector adds significant overhead which can cause timeout failures on slower CI machines. Increase timeouts from 5s to 10s and add more context to failure messages for debugging. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main_test.go | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 35979e6..7c4f68c 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -1514,10 +1514,11 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { // Wait for daemon to be fully started and responsive // The runtime file is written before ListenAndServe, so we need to verify - // the HTTP server is actually accepting connections + // the HTTP server is actually accepting connections. + // Use longer timeout for race detector which adds significant overhead. var info *daemon.RuntimeInfo myPID := os.Getpid() - deadline = time.Now().Add(5 * time.Second) + deadline = time.Now().Add(10 * time.Second) for time.Now().Before(deadline) { runtimes, err := daemon.ListAllRuntimes() if err == nil { @@ -1532,10 +1533,12 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { break } } - time.Sleep(50 * time.Millisecond) + time.Sleep(100 * time.Millisecond) } if info == nil { - t.Fatal("daemon did not create runtime file or is not responding") + // Provide more context for debugging CI failures + runtimes, _ := daemon.ListAllRuntimes() + t.Fatalf("daemon did not create runtime file or is not responding (myPID=%d, found %d runtimes)", myPID, len(runtimes)) } // The daemon runs in a goroutine within this test process. @@ -1548,12 +1551,12 @@ func TestDaemonRunStartsAndShutdownsCleanly(t *testing.T) { t.Fatalf("failed to send interrupt signal: %v", err) } - // Wait for daemon to exit + // Wait for daemon to exit (longer timeout for race detector) select { case <-errCh: // Daemon exited - good - case <-time.After(5 * time.Second): - t.Fatal("daemon did not exit within 5 second timeout") + case <-time.After(10 * time.Second): + t.Fatal("daemon did not exit within 10 second timeout") } }