From 349f58ed58edbe1cfa1b281d41a5a560d8ac1e83 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 11:49:25 -0500 Subject: [PATCH 01/23] Add commit message viewer (m) and keyboard help modal (?) - m key: Shows commit message(s) for selected job - Single commits: full message with author/date - Commit ranges: all commits with numbered list - Dirty reviews: flash "No commit message for uncommitted changes" - Run/prompt jobs: flash "No commit message for run tasks" - ? key: Toggles keyboard help modal with all shortcuts - Returns to previous view (queue or review) on close - Updated help lines to be more concise, moved less common shortcuts (r, c, p) to help modal only Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 344 +++++++++++++++++++++++++++++++++++++++- cmd/roborev/tui_test.go | 14 +- 2 files changed, 348 insertions(+), 10 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index e1033e0..a6abff5 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -73,6 +73,8 @@ const ( tuiViewPrompt tuiViewFilter tuiViewRespond + tuiViewCommitMsg + tuiViewHelp ) // repoFilterItem represents a repo (or group of repos with same display name) in the filter modal @@ -147,6 +149,14 @@ type tuiModel struct { // Daemon reconnection state consecutiveErrors int // Count of consecutive connection failures reconnecting bool // True if currently attempting reconnection + + // Commit message view state + commitMsgContent string // Formatted commit message(s) content + commitMsgScroll int // Scroll position in commit message view + commitMsgJobID int64 // Job ID for the commit message being viewed + + // Help view state + helpFromView tuiView // View to return to after closing help } // pendingState tracks a pending addressed toggle with sequence number @@ -212,6 +222,11 @@ type tuiClipboardResultMsg struct { err error view tuiView // The view where copy was triggered (for flash attribution) } +type tuiCommitMsgMsg struct { + jobID int64 + content string + err error +} type tuiReconnectMsg struct { newAddr string // New daemon address if found, empty if not found version string // Daemon version (to avoid sync call in Update) @@ -695,6 +710,89 @@ func (m tuiModel) fetchReviewAndCopy(jobID int64, job *storage.ReviewJob) tea.Cm } } +// fetchCommitMsg fetches commit message(s) for a job. +// For single commits, returns the commit message. +// For ranges, returns all commit messages in the range. +// For dirty reviews or prompt jobs, returns an error. +func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { + jobID := job.ID + return func() tea.Msg { + // Handle dirty reviews (uncommitted changes) + if job.DiffContent != nil { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commit message for uncommitted changes"), + } + } + + // Handle prompt/run jobs + if job.Prompt != "" { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commit message for run tasks"), + } + } + + // Check if this is a range (contains "..") + if strings.Contains(job.GitRef, "..") { + // Fetch all commits in range + commits, err := git.GetRangeCommits(job.RepoPath, job.GitRef) + if err != nil { + return tuiCommitMsgMsg{jobID: jobID, err: err} + } + if len(commits) == 0 { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commits in range %s", job.GitRef), + } + } + + // Fetch info for each commit + var content strings.Builder + content.WriteString(fmt.Sprintf("Commits in %s (%d commits):\n", job.GitRef, len(commits))) + content.WriteString(strings.Repeat("─", 60) + "\n\n") + + for i, sha := range commits { + info, err := git.GetCommitInfo(job.RepoPath, sha) + if err != nil { + content.WriteString(fmt.Sprintf("%d. %s: (error: %v)\n\n", i+1, sha[:7], err)) + continue + } + content.WriteString(fmt.Sprintf("%d. %s %s\n", i+1, info.SHA[:7], info.Subject)) + content.WriteString(fmt.Sprintf(" Author: %s | %s\n", info.Author, info.Timestamp.Format("2006-01-02 15:04"))) + if info.Body != "" { + // Indent body + bodyLines := strings.Split(info.Body, "\n") + for _, line := range bodyLines { + content.WriteString(" " + line + "\n") + } + } + content.WriteString("\n") + } + + return tuiCommitMsgMsg{jobID: jobID, content: content.String()} + } + + // Single commit + info, err := git.GetCommitInfo(job.RepoPath, job.GitRef) + if err != nil { + return tuiCommitMsgMsg{jobID: jobID, err: err} + } + + var content strings.Builder + content.WriteString(fmt.Sprintf("Commit: %s\n", info.SHA)) + content.WriteString(fmt.Sprintf("Author: %s\n", info.Author)) + content.WriteString(fmt.Sprintf("Date: %s\n", info.Timestamp.Format("2006-01-02 15:04:05 -0700"))) + content.WriteString(strings.Repeat("─", 60) + "\n\n") + content.WriteString(info.Subject + "\n") + if info.Body != "" { + content.WriteString("\n" + info.Body + "\n") + } + + return tuiCommitMsgMsg{jobID: jobID, content: content.String()} + } +} + func (m tuiModel) addressReview(reviewID, jobID int64, newState, oldState bool, seq uint64) tea.Cmd { return func() tea.Msg { reqBody, err := json.Marshal(map[string]interface{}{ @@ -1250,6 +1348,16 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } return m, nil } + if m.currentView == tuiViewCommitMsg { + m.currentView = tuiViewQueue + m.commitMsgContent = "" + m.commitMsgScroll = 0 + return m, nil + } + if m.currentView == tuiViewHelp { + m.currentView = m.helpFromView + return m, nil + } return m, tea.Quit case "up": @@ -1268,6 +1376,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if m.promptScroll > 0 { m.promptScroll-- } + } else if m.currentView == tuiViewCommitMsg { + if m.commitMsgScroll > 0 { + m.commitMsgScroll-- + } } case "k", "right": @@ -1319,6 +1431,8 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.reviewScroll++ } else if m.currentView == tuiViewPrompt { m.promptScroll++ + } else if m.currentView == tuiViewCommitMsg { + m.commitMsgScroll++ } case "j", "left": @@ -1652,6 +1766,34 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } } + case "m": + // Show commit message(s) for the selected job + if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { + job := m.jobs[m.selectedIdx] + m.commitMsgJobID = job.ID + m.commitMsgContent = "" + m.commitMsgScroll = 0 + return m, m.fetchCommitMsg(&job) + } else if m.currentView == tuiViewReview && m.currentReview != nil && m.currentReview.Job != nil { + job := m.currentReview.Job + m.commitMsgJobID = job.ID + m.commitMsgContent = "" + m.commitMsgScroll = 0 + return m, m.fetchCommitMsg(job) + } + + case "?": + // Toggle help modal + if m.currentView == tuiViewHelp { + m.currentView = m.helpFromView + return m, nil + } + if m.currentView == tuiViewQueue || m.currentView == tuiViewReview { + m.helpFromView = m.currentView + m.currentView = tuiViewHelp + return m, nil + } + case "esc": if m.currentView == tuiViewQueue && len(m.activeRepoFilter) > 0 { // Clear project filter first (keep hide-addressed if active) @@ -1704,6 +1846,14 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.currentView = tuiViewReview m.promptScroll = 0 } + } else if m.currentView == tuiViewCommitMsg { + // Go back to queue view + m.currentView = tuiViewQueue + m.commitMsgContent = "" + m.commitMsgScroll = 0 + } else if m.currentView == tuiViewHelp { + // Go back to previous view + m.currentView = m.helpFromView } } @@ -2027,6 +2177,21 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.flashView = msg.view // Use view from trigger time, not current view } + case tuiCommitMsgMsg: + // Ignore stale messages (job changed while fetching) + if msg.jobID != m.commitMsgJobID { + return m, nil + } + if msg.err != nil { + m.flashMessage = msg.err.Error() + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = m.currentView + return m, nil + } + m.commitMsgContent = msg.content + m.commitMsgScroll = 0 + m.currentView = tuiViewCommitMsg + case tuiJobsErrMsg: m.err = msg.err m.loadingJobs = false // Clear loading state so refreshes can resume @@ -2111,6 +2276,12 @@ func (m tuiModel) View() string { if m.currentView == tuiViewFilter { return m.renderFilterView() } + if m.currentView == tuiViewCommitMsg { + return m.renderCommitMsgView() + } + if m.currentView == tuiViewHelp { + return m.renderHelpView() + } if m.currentView == tuiViewPrompt && m.currentReview != nil { return m.renderPromptView() } @@ -2299,8 +2470,8 @@ func (m tuiModel) renderQueueView() string { b.WriteString("\x1b[K\n") // Clear to end of line // Help (two lines) - helpLine1 := "up/down/pgup/pgdn: navigate | enter: review | p: prompt | c: respond | y: copy | q: quit" - helpLine2 := "f: filter | h: hide | a: toggle addressed | x: cancel | r: rerun" + helpLine1 := "↑/↓: navigate | enter: review | y: copy | m: commit msg | q: quit | ?: help" + helpLine2 := "f: filter | h: hide addressed | a: toggle addressed | x: cancel" if len(m.activeRepoFilter) > 0 || m.hideAddressed { helpLine2 += " | esc: clear filters" } @@ -2591,7 +2762,7 @@ func (m tuiModel) renderReviewView() string { } // Help text wraps at narrow terminals - const helpText = "up/down: scroll | j/k: prev/next | a: addressed | c: respond | y: copy | p: prompt | esc/q: back" + const helpText = "↑/↓: scroll | j/k: prev/next | a: addressed | y: copy | m: commit msg | ?: help | esc/q: back" helpLines := 1 if m.width > 0 && m.width < len(helpText) { helpLines = (len(helpText) + m.width - 1) / m.width @@ -2947,6 +3118,173 @@ func (m tuiModel) submitResponse(jobID int64, text string) tea.Cmd { } } +func (m tuiModel) renderCommitMsgView() string { + var b strings.Builder + + b.WriteString(tuiTitleStyle.Render("Commit Message")) + b.WriteString("\x1b[K\n") // Clear to end of line + + if m.commitMsgContent == "" { + b.WriteString(tuiStatusStyle.Render("Loading commit message...")) + b.WriteString("\x1b[K\n") + // Pad to fill terminal + linesWritten := 2 + for linesWritten < m.height-1 { + b.WriteString("\x1b[K\n") + linesWritten++ + } + b.WriteString(tuiHelpStyle.Render("esc/q: back")) + b.WriteString("\x1b[K") + b.WriteString("\x1b[J") + return b.String() + } + + // Wrap text to terminal width minus padding + wrapWidth := max(20, min(m.width-4, 200)) + lines := wrapText(m.commitMsgContent, wrapWidth) + + // Reserve: title(1) + scroll indicator(1) + help(1) + margin(1) + visibleLines := m.height - 4 + if visibleLines < 1 { + visibleLines = 1 + } + + // Clamp scroll position to valid range + maxScroll := len(lines) - visibleLines + if maxScroll < 0 { + maxScroll = 0 + } + start := m.commitMsgScroll + if start > maxScroll { + start = maxScroll + } + if start < 0 { + start = 0 + } + end := min(start+visibleLines, len(lines)) + + linesWritten := 0 + for i := start; i < end; i++ { + b.WriteString(lines[i]) + b.WriteString("\x1b[K\n") // Clear to end of line before newline + linesWritten++ + } + + // Pad with clear-to-end-of-line sequences to prevent ghost text + for linesWritten < visibleLines { + b.WriteString("\x1b[K\n") + linesWritten++ + } + + // Scroll indicator + if len(lines) > visibleLines { + scrollInfo := fmt.Sprintf("[%d-%d of %d lines]", start+1, end, len(lines)) + b.WriteString(tuiStatusStyle.Render(scrollInfo)) + } + b.WriteString("\x1b[K\n") // Clear scroll indicator line + + b.WriteString(tuiHelpStyle.Render("up/down: scroll | esc/q: back")) + b.WriteString("\x1b[K") // Clear help line + b.WriteString("\x1b[J") // Clear to end of screen to prevent artifacts + + return b.String() +} + +func (m tuiModel) renderHelpView() string { + var b strings.Builder + + b.WriteString(tuiTitleStyle.Render("Keyboard Shortcuts")) + b.WriteString("\x1b[K\n\x1b[K\n") + + // Define shortcuts in groups + shortcuts := []struct { + group string + keys []struct{ key, desc string } + }{ + { + group: "Navigation", + keys: []struct{ key, desc string }{ + {"↑/k", "Move up / previous review"}, + {"↓/j", "Move down / next review"}, + {"enter", "View review details"}, + {"esc", "Go back / clear filter"}, + {"q", "Quit"}, + }, + }, + { + group: "Actions", + keys: []struct{ key, desc string }{ + {"a", "Mark as addressed"}, + {"c", "Respond to review"}, + {"x", "Cancel job"}, + {"r", "Re-run job"}, + {"y", "Copy review to clipboard"}, + {"m", "Show commit message(s)"}, + }, + }, + { + group: "Filtering", + keys: []struct{ key, desc string }{ + {"f", "Filter by repository"}, + {"h", "Toggle hide addressed"}, + }, + }, + { + group: "Review View", + keys: []struct{ key, desc string }{ + {"p", "View prompt"}, + {"↑/↓", "Scroll content"}, + }, + }, + } + + // Calculate visible area + // Reserve: title(1) + blank(1) + padding + help(1) + reservedLines := 3 + visibleLines := m.height - reservedLines + if visibleLines < 5 { + visibleLines = 5 + } + + linesWritten := 0 + for _, g := range shortcuts { + if linesWritten >= visibleLines-2 { + break + } + // Group header + b.WriteString(tuiSelectedStyle.Render(g.group)) + b.WriteString("\x1b[K\n") + linesWritten++ + + for _, k := range g.keys { + if linesWritten >= visibleLines { + break + } + line := fmt.Sprintf(" %-12s %s", k.key, k.desc) + b.WriteString(line) + b.WriteString("\x1b[K\n") + linesWritten++ + } + // Blank line between groups + if linesWritten < visibleLines { + b.WriteString("\x1b[K\n") + linesWritten++ + } + } + + // Pad remaining space + for linesWritten < visibleLines { + b.WriteString("\x1b[K\n") + linesWritten++ + } + + b.WriteString(tuiHelpStyle.Render("esc/q/?: close")) + b.WriteString("\x1b[K") + b.WriteString("\x1b[J") // Clear to end of screen + + return b.String() +} + func tuiCmd() *cobra.Command { var addr string diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index cce9e75..fe63783 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -4035,9 +4035,9 @@ func TestTUIRenderFailedJobNoBranchShown(t *testing.T) { func TestTUIVisibleLinesCalculationNoVerdict(t *testing.T) { // Test that visibleLines = height - 3 when no verdict (title + status + help) - // Help text is 87 chars, so use width >= 87 to avoid wrapping + // Help text is ~106 chars, so use width >= 110 to avoid wrapping m := newTuiModel("http://localhost") - m.width = 100 + m.width = 120 m.height = 10 // Small height to test calculation m.currentView = tuiViewReview // Create 20 lines of content to ensure scrolling @@ -4077,10 +4077,10 @@ func TestTUIVisibleLinesCalculationNoVerdict(t *testing.T) { func TestTUIVisibleLinesCalculationWithVerdict(t *testing.T) { // Test that visibleLines = height - 4 when verdict present (title + verdict + status + help) - // Help text is 87 chars, so use width >= 87 to avoid wrapping + // Help text is ~106 chars, so use width >= 110 to avoid wrapping verdictPass := "P" m := newTuiModel("http://localhost") - m.width = 100 + m.width = 120 m.height = 10 // Small height to test calculation m.currentView = tuiViewReview // Create 20 lines of content to ensure scrolling @@ -4119,7 +4119,7 @@ func TestTUIVisibleLinesCalculationWithVerdict(t *testing.T) { func TestTUIVisibleLinesCalculationNarrowTerminal(t *testing.T) { // Test that visibleLines accounts for help text wrapping at narrow terminals - // Help text is 87 chars, at width=50 it wraps to 2 lines: ceil(87/50) = 2 + // Help text is ~91 chars, at width=50 it wraps to 2 lines: ceil(91/50) = 2 m := newTuiModel("http://localhost") m.width = 50 m.height = 10 @@ -4160,7 +4160,7 @@ func TestTUIVisibleLinesCalculationNarrowTerminal(t *testing.T) { func TestTUIVisibleLinesCalculationNarrowTerminalWithVerdict(t *testing.T) { // Test narrow terminal with verdict - validates extra header line branch - // Help text is 87 chars, at width=50 it wraps to 2 lines: ceil(87/50) = 2 + // Help text is ~91 chars, at width=50 it wraps to 2 lines: ceil(91/50) = 2 verdictFail := "F" m := newTuiModel("http://localhost") m.width = 50 @@ -4234,7 +4234,7 @@ func TestTUIVisibleLinesCalculationLongTitleWraps(t *testing.T) { // "Review #1 very-long-repository-name-here abc1234..def5678 (claude-code) on feature/very-long-branch-name [ADDRESSED]" // = 7 + 3 + 31 + 17 + 14 + 34 + 12 = ~118 chars // At width=50: ceil(118/50) = 3 title lines - // Help at width=50: ceil(87/50) = 2 help lines + // Help at width=50: ceil(91/50) = 2 help lines // Non-content: title (3) + status line (1) + help (2) = 6 // visibleLines = 12 - 6 = 6 contentCount := 0 From 27e28b41d52efdff1ef5e7df12886df154b770f1 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 11:55:16 -0500 Subject: [PATCH 02/23] Rename respond command to comment - CLI: `roborev comment` is the new command, `respond` is hidden alias - TUI: "Add Comment" modal, "Comments" section in review view - Help modal: "c: Add comment" - Skills: Renamed roborev-respond to roborev-comment, updated SKILL.md files - Address skill: Updated to use `roborev comment` command Co-Authored-By: Claude Opus 4.5 --- .../{respond_test.go => comment_test.go} | 8 +-- cmd/roborev/main.go | 39 +++++++----- cmd/roborev/tui.go | 12 ++-- .../skills/claude/roborev-address/SKILL.md | 8 +-- .../skills/claude/roborev-comment/SKILL.md | 61 +++++++++++++++++++ .../skills/claude/roborev-respond/SKILL.md | 61 ------------------- .../skills/codex/roborev-address/SKILL.md | 8 +-- .../skills/codex/roborev-comment/SKILL.md | 61 +++++++++++++++++++ .../skills/codex/roborev-respond/SKILL.md | 61 ------------------- internal/skills/skills.go | 4 +- internal/skills/skills_test.go | 22 +++---- 11 files changed, 177 insertions(+), 168 deletions(-) rename cmd/roborev/{respond_test.go => comment_test.go} (93%) create mode 100644 internal/skills/claude/roborev-comment/SKILL.md delete mode 100644 internal/skills/claude/roborev-respond/SKILL.md create mode 100644 internal/skills/codex/roborev-comment/SKILL.md delete mode 100644 internal/skills/codex/roborev-respond/SKILL.md diff --git a/cmd/roborev/respond_test.go b/cmd/roborev/comment_test.go similarity index 93% rename from cmd/roborev/respond_test.go rename to cmd/roborev/comment_test.go index d481bfd..b15b268 100644 --- a/cmd/roborev/respond_test.go +++ b/cmd/roborev/comment_test.go @@ -1,6 +1,6 @@ package main -// Tests for the respond command +// Tests for the comment command import ( "encoding/json" @@ -12,7 +12,7 @@ import ( "github.com/roborev-dev/roborev/internal/version" ) -func TestRespondJobFlag(t *testing.T) { +func TestCommentJobFlag(t *testing.T) { t.Run("--job forces job ID interpretation", func(t *testing.T) { var receivedJobID int64 _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -34,7 +34,7 @@ func TestRespondJobFlag(t *testing.T) { defer cleanup() // "1234567" could be a SHA, but --job forces job ID interpretation - cmd := respondCmd() + cmd := commentCmd() cmd.SetArgs([]string{"--job", "1234567", "-m", "test message"}) err := cmd.Execute() if err != nil { @@ -52,7 +52,7 @@ func TestRespondJobFlag(t *testing.T) { })) defer cleanup() - cmd := respondCmd() + cmd := commentCmd() cmd.SetArgs([]string{"--job", "abc123", "-m", "test"}) err := cmd.Execute() if err == nil { diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index f12b2ce..849a774 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -54,7 +54,8 @@ func main() { rootCmd.AddCommand(reviewCmd()) rootCmd.AddCommand(statusCmd()) rootCmd.AddCommand(showCmd()) - rootCmd.AddCommand(respondCmd()) + rootCmd.AddCommand(commentCmd()) + rootCmd.AddCommand(respondCmd()) // hidden alias for backward compatibility rootCmd.AddCommand(addressCmd()) rootCmd.AddCommand(installHookCmd()) rootCmd.AddCommand(uninstallHookCmd()) @@ -1199,7 +1200,7 @@ Examples: return cmd } -func respondCmd() *cobra.Command { +func commentCmd() *cobra.Command { var ( responder string message string @@ -1207,19 +1208,19 @@ func respondCmd() *cobra.Command { ) cmd := &cobra.Command{ - Use: "respond [message]", - Short: "Add a response to a review", - Long: `Add a response or note to a review. + Use: "comment [message]", + Short: "Add a comment to a review", + Long: `Add a comment or note to a review. The first argument can be either a job ID (numeric) or a commit SHA. Using job IDs is recommended since they are displayed in the TUI. Examples: - roborev respond 42 "Fixed the null pointer issue" - roborev respond 42 -m "Added missing error handling" - roborev respond abc123 "Addressed by refactoring" - roborev respond 42 # Opens editor for message - roborev respond --job 1234567 "msg" # Force numeric arg as job ID`, + roborev comment 42 "Fixed the null pointer issue" + roborev comment 42 -m "Added missing error handling" + roborev comment abc123 "Addressed by refactoring" + roborev comment 42 # Opens editor for message + roborev comment --job 1234567 "msg" # Force numeric arg as job ID`, Args: cobra.RangeArgs(1, 2), RunE: func(cmd *cobra.Command, args []string) error { // Ensure daemon is running @@ -1295,7 +1296,7 @@ Examples: } if message == "" { - return fmt.Errorf("empty response, aborting") + return fmt.Errorf("empty comment, aborting") } if responder == "" { @@ -1327,21 +1328,29 @@ Examples: if resp.StatusCode != http.StatusCreated { body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("failed to add response: %s", body) + return fmt.Errorf("failed to add comment: %s", body) } - fmt.Println("Response added successfully") + fmt.Println("Comment added successfully") return nil }, } - cmd.Flags().StringVar(&responder, "responder", "", "responder name (default: $USER)") - cmd.Flags().StringVarP(&message, "message", "m", "", "response message (opens editor if not provided)") + cmd.Flags().StringVar(&responder, "commenter", "", "commenter name (default: $USER)") + cmd.Flags().StringVarP(&message, "message", "m", "", "comment message (opens editor if not provided)") cmd.Flags().BoolVar(&forceJobID, "job", false, "force argument to be treated as job ID (not SHA)") return cmd } +// respondCmd returns a hidden alias for backward compatibility +func respondCmd() *cobra.Command { + cmd := commentCmd() + cmd.Use = "respond [message]" + cmd.Hidden = true + return cmd +} + func addressCmd() *cobra.Command { var unaddress bool diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index a6abff5..617f604 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -2742,7 +2742,7 @@ func (m tuiModel) renderReviewView() string { // Append responses if any if len(m.currentResponses) > 0 { - content.WriteString("\n\n--- Responses ---\n") + content.WriteString("\n\n--- Comments ---\n") for _, r := range m.currentResponses { timestamp := r.CreatedAt.Format("Jan 02 15:04") content.WriteString(fmt.Sprintf("\n[%s] %s:\n", timestamp, r.Responder)) @@ -3009,14 +3009,14 @@ func (m tuiModel) renderFilterView() string { func (m tuiModel) renderRespondView() string { var b strings.Builder - title := "Respond to Review" + title := "Add Comment" if m.respondCommit != "" { - title = fmt.Sprintf("Respond to Review (%s)", m.respondCommit) + title = fmt.Sprintf("Add Comment (%s)", m.respondCommit) } b.WriteString(tuiTitleStyle.Render(title)) b.WriteString("\x1b[K\n\x1b[K\n") // Clear title and blank line - b.WriteString(tuiStatusStyle.Render("Enter your response (e.g., \"This is a known issue, can be ignored\")")) + b.WriteString(tuiStatusStyle.Render("Enter your comment (e.g., \"This is a known issue, can be ignored\")")) b.WriteString("\x1b[K\n\x1b[K\n") // Simple text box with border @@ -3036,7 +3036,7 @@ func (m tuiModel) renderRespondView() string { if m.respondText == "" { // Show placeholder (styled, but we pad manually to avoid ANSI issues) - placeholder := "Type your response..." + placeholder := "Type your comment..." padded := placeholder + strings.Repeat(" ", boxWidth-2-len(placeholder)) b.WriteString("| " + tuiStatusStyle.Render(padded) + " |\x1b[K\n") textLinesWritten++ @@ -3215,7 +3215,7 @@ func (m tuiModel) renderHelpView() string { group: "Actions", keys: []struct{ key, desc string }{ {"a", "Mark as addressed"}, - {"c", "Respond to review"}, + {"c", "Add comment"}, {"x", "Cancel job"}, {"r", "Re-run job"}, {"y", "Copy review to clipboard"}, diff --git a/internal/skills/claude/roborev-address/SKILL.md b/internal/skills/claude/roborev-address/SKILL.md index 664a10e..e7a5a8a 100644 --- a/internal/skills/claude/roborev-address/SKILL.md +++ b/internal/skills/claude/roborev-address/SKILL.md @@ -41,10 +41,10 @@ Parse the findings from the output (severity, file paths, line numbers), then: After fixing, **record what was done** by executing: ```bash -roborev respond --job "" +roborev comment --job "" ``` -This records your response in roborev so the review shows it was addressed. +This records your comment in roborev so the review shows it was addressed. Then ask the user if they want to commit the changes. @@ -56,5 +56,5 @@ Agent: 1. Executes `roborev show --job 1019` 2. Sees verdict is Fail with 2 findings 3. Reads files, fixes the issues, runs tests -4. Executes `roborev respond --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` -5. Asks: "I've addressed both findings and recorded the response. Tests pass. Would you like me to commit these changes?" +4. Executes `roborev comment --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` +5. Asks: "I've addressed both findings and recorded a comment. Tests pass. Would you like me to commit these changes?" diff --git a/internal/skills/claude/roborev-comment/SKILL.md b/internal/skills/claude/roborev-comment/SKILL.md new file mode 100644 index 0000000..5e7cf18 --- /dev/null +++ b/internal/skills/claude/roborev-comment/SKILL.md @@ -0,0 +1,61 @@ +--- +name: roborev:comment +description: Add a comment or note to a roborev code review to document how findings were addressed +--- + +# roborev:comment + +Record a comment on a roborev code review. + +## Usage + +``` +/roborev:comment [message] +``` + +## IMPORTANT + +This skill requires you to **execute a bash command** to record the comment in roborev. The task is not complete until you run the `roborev comment` command and see confirmation output. + +## Instructions + +When the user invokes `/roborev:comment [message]`: + +1. **If a message is provided**, immediately execute: + ```bash + roborev comment --job "" + ``` + +2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their comment. + +3. **Verify success** - the command will output confirmation. If it fails, report the error. + +The comment is recorded in roborev's database and will appear when viewing the review with `roborev show`. + +## Examples + +**With message provided:** + +User: `/roborev:comment 1019 Fixed all issues` + +Agent action: +```bash +roborev comment --job 1019 "Fixed all issues" +``` +Then confirm: "Comment recorded for review #1019." + +--- + +**Without message:** + +User: `/roborev:comment 1019` + +Agent: "What would you like to say about review #1019?" + +User: "The null check was a false positive" + +Agent action: +```bash +roborev comment --job 1019 "The null check was a false positive" +``` +Then confirm: "Comment recorded for review #1019." diff --git a/internal/skills/claude/roborev-respond/SKILL.md b/internal/skills/claude/roborev-respond/SKILL.md deleted file mode 100644 index 9272567..0000000 --- a/internal/skills/claude/roborev-respond/SKILL.md +++ /dev/null @@ -1,61 +0,0 @@ ---- -name: roborev:respond -description: Add a response or note to a roborev code review to document how findings were addressed ---- - -# roborev:respond - -Record a response to a roborev code review. - -## Usage - -``` -/roborev:respond [message] -``` - -## IMPORTANT - -This skill requires you to **execute a bash command** to record the response in roborev. The task is not complete until you run the `roborev respond` command and see confirmation output. - -## Instructions - -When the user invokes `/roborev:respond [message]`: - -1. **If a message is provided**, immediately execute: - ```bash - roborev respond --job "" - ``` - -2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their response. - -3. **Verify success** - the command will output confirmation. If it fails, report the error. - -The response is recorded in roborev's database and will appear when viewing the review with `roborev show`. - -## Examples - -**With message provided:** - -User: `/roborev:respond 1019 Fixed all issues` - -Agent action: -```bash -roborev respond --job 1019 "Fixed all issues" -``` -Then confirm: "Response recorded for review #1019." - ---- - -**Without message:** - -User: `/roborev:respond 1019` - -Agent: "What would you like to say in response to review #1019?" - -User: "The null check was a false positive" - -Agent action: -```bash -roborev respond --job 1019 "The null check was a false positive" -``` -Then confirm: "Response recorded for review #1019." diff --git a/internal/skills/codex/roborev-address/SKILL.md b/internal/skills/codex/roborev-address/SKILL.md index 8380998..a05a25e 100644 --- a/internal/skills/codex/roborev-address/SKILL.md +++ b/internal/skills/codex/roborev-address/SKILL.md @@ -41,10 +41,10 @@ Parse the findings from the output (severity, file paths, line numbers), then: After fixing, **record what was done** by executing: ```bash -roborev respond --job "" +roborev comment --job "" ``` -This records your response in roborev so the review shows it was addressed. +This records your comment in roborev so the review shows it was addressed. Then ask the user if they want to commit the changes. @@ -56,5 +56,5 @@ Agent: 1. Executes `roborev show --job 1019` 2. Sees verdict is Fail with 2 findings 3. Reads files, fixes the issues, runs tests -4. Executes `roborev respond --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` -5. Asks: "I've addressed both findings and recorded the response. Tests pass. Would you like me to commit these changes?" +4. Executes `roborev comment --job 1019 "Fixed null check in foo.go and added error handling in bar.go"` +5. Asks: "I've addressed both findings and recorded a comment. Tests pass. Would you like me to commit these changes?" diff --git a/internal/skills/codex/roborev-comment/SKILL.md b/internal/skills/codex/roborev-comment/SKILL.md new file mode 100644 index 0000000..4c1553a --- /dev/null +++ b/internal/skills/codex/roborev-comment/SKILL.md @@ -0,0 +1,61 @@ +--- +name: roborev:comment +description: Add a comment or note to a roborev code review to document how findings were addressed +--- + +# roborev:comment + +Record a comment on a roborev code review. + +## Usage + +``` +$roborev:comment [message] +``` + +## IMPORTANT + +This skill requires you to **execute a bash command** to record the comment in roborev. The task is not complete until you run the `roborev comment` command and see confirmation output. + +## Instructions + +When the user invokes `$roborev:comment [message]`: + +1. **If a message is provided**, immediately execute: + ```bash + roborev comment --job "" + ``` + +2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their comment. + +3. **Verify success** - the command will output confirmation. If it fails, report the error. + +The comment is recorded in roborev's database and will appear when viewing the review with `roborev show`. + +## Examples + +**With message provided:** + +User: `$roborev:comment 1019 Fixed all issues` + +Agent action: +```bash +roborev comment --job 1019 "Fixed all issues" +``` +Then confirm: "Comment recorded for review #1019." + +--- + +**Without message:** + +User: `$roborev:comment 1019` + +Agent: "What would you like to say about review #1019?" + +User: "The null check was a false positive" + +Agent action: +```bash +roborev comment --job 1019 "The null check was a false positive" +``` +Then confirm: "Comment recorded for review #1019." diff --git a/internal/skills/codex/roborev-respond/SKILL.md b/internal/skills/codex/roborev-respond/SKILL.md deleted file mode 100644 index 367b621..0000000 --- a/internal/skills/codex/roborev-respond/SKILL.md +++ /dev/null @@ -1,61 +0,0 @@ ---- -name: roborev:respond -description: Add a response or note to a roborev code review to document how findings were addressed ---- - -# roborev:respond - -Record a response to a roborev code review. - -## Usage - -``` -$roborev:respond [message] -``` - -## IMPORTANT - -This skill requires you to **execute a bash command** to record the response in roborev. The task is not complete until you run the `roborev respond` command and see confirmation output. - -## Instructions - -When the user invokes `$roborev:respond [message]`: - -1. **If a message is provided**, immediately execute: - ```bash - roborev respond --job "" - ``` - -2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their response. - -3. **Verify success** - the command will output confirmation. If it fails, report the error. - -The response is recorded in roborev's database and will appear when viewing the review with `roborev show`. - -## Examples - -**With message provided:** - -User: `$roborev:respond 1019 Fixed all issues` - -Agent action: -```bash -roborev respond --job 1019 "Fixed all issues" -``` -Then confirm: "Response recorded for review #1019." - ---- - -**Without message:** - -User: `$roborev:respond 1019` - -Agent: "What would you like to say in response to review #1019?" - -User: "The null check was a false positive" - -Agent action: -```bash -roborev respond --job 1019 "The null check was a false positive" -``` -Then confirm: "Response recorded for review #1019." diff --git a/internal/skills/skills.go b/internal/skills/skills.go index b4f024d..aa975d8 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -69,13 +69,13 @@ func IsInstalled(agent Agent) bool { skillsDir := filepath.Join(home, ".claude", "skills") checkFiles = []string{ filepath.Join(skillsDir, "roborev-address", "SKILL.md"), - filepath.Join(skillsDir, "roborev-respond", "SKILL.md"), + filepath.Join(skillsDir, "roborev-comment", "SKILL.md"), } case AgentCodex: skillsDir := filepath.Join(home, ".codex", "skills") checkFiles = []string{ filepath.Join(skillsDir, "roborev-address", "SKILL.md"), - filepath.Join(skillsDir, "roborev-respond", "SKILL.md"), + filepath.Join(skillsDir, "roborev-comment", "SKILL.md"), } default: return false diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index d0a8a7a..99b03d4 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -99,8 +99,8 @@ func TestInstallClaudeWhenDirExists(t *testing.T) { if _, err := os.Stat(filepath.Join(skillsDir, "roborev-address", "SKILL.md")); err != nil { t.Error("expected roborev-address/SKILL.md to exist") } - if _, err := os.Stat(filepath.Join(skillsDir, "roborev-respond", "SKILL.md")); err != nil { - t.Error("expected roborev-respond/SKILL.md to exist") + if _, err := os.Stat(filepath.Join(skillsDir, "roborev-comment", "SKILL.md")); err != nil { + t.Error("expected roborev-comment/SKILL.md to exist") } } @@ -143,8 +143,8 @@ func TestInstallCodexWhenDirExists(t *testing.T) { if _, err := os.Stat(filepath.Join(skillsDir, "roborev-address", "SKILL.md")); err != nil { t.Error("expected roborev-address/SKILL.md to exist") } - if _, err := os.Stat(filepath.Join(skillsDir, "roborev-respond", "SKILL.md")); err != nil { - t.Error("expected roborev-respond/SKILL.md to exist") + if _, err := os.Stat(filepath.Join(skillsDir, "roborev-comment", "SKILL.md")); err != nil { + t.Error("expected roborev-comment/SKILL.md to exist") } } @@ -220,7 +220,7 @@ func TestIsInstalledClaude(t *testing.T) { // Create only respond skill (not address) skillsDir := filepath.Join(claudeDir, "skills") - respondDir := filepath.Join(skillsDir, "roborev-respond") + respondDir := filepath.Join(skillsDir, "roborev-comment") if err := os.MkdirAll(respondDir, 0755); err != nil { t.Fatal(err) } @@ -228,7 +228,7 @@ func TestIsInstalledClaude(t *testing.T) { t.Fatal(err) } if !IsInstalled(AgentClaude) { - t.Error("expected IsInstalled=true when roborev-respond/SKILL.md exists") + t.Error("expected IsInstalled=true when roborev-comment/SKILL.md exists") } // Remove respond, add address @@ -265,7 +265,7 @@ func TestIsInstalledCodex(t *testing.T) { } // Create only respond skill - respondDir := filepath.Join(codexDir, "skills", "roborev-respond") + respondDir := filepath.Join(codexDir, "skills", "roborev-comment") if err := os.MkdirAll(respondDir, 0755); err != nil { t.Fatal(err) } @@ -273,7 +273,7 @@ func TestIsInstalledCodex(t *testing.T) { t.Fatal(err) } if !IsInstalled(AgentCodex) { - t.Error("expected IsInstalled=true when roborev-respond/SKILL.md exists") + t.Error("expected IsInstalled=true when roborev-comment/SKILL.md exists") } } @@ -318,7 +318,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { defer cleanup() // Create .claude with only respond skill installed - claudeSkillsDir := filepath.Join(tmpHome, ".claude", "skills", "roborev-respond") + claudeSkillsDir := filepath.Join(tmpHome, ".claude", "skills", "roborev-comment") if err := os.MkdirAll(claudeSkillsDir, 0755); err != nil { t.Fatal(err) } @@ -381,7 +381,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { defer cleanup() // Create .codex with only respond skill installed - codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-respond") + codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-comment") if err := os.MkdirAll(codexSkillsDir, 0755); err != nil { t.Fatal(err) } @@ -417,7 +417,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { } // Create .codex with skills - codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-respond") + codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-comment") if err := os.MkdirAll(codexSkillsDir, 0755); err != nil { t.Fatal(err) } From f6b465b245c00a2e0d86fe604839e7d325fcc5df Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 12:07:02 -0500 Subject: [PATCH 03/23] Normalize comment terminology and revert skill to roborev:respond - Rename TUI internal variables from respond* to comment* - Revert skill directories to roborev-respond (skill name roborev:respond) - Update skill to also mark review as addressed - Make 'roborev respond' visible as alias for 'comment' Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main.go | 4 +- cmd/roborev/tui.go | 110 +++++++++--------- cmd/roborev/tui_test.go | 86 +++++++------- .../skills/claude/roborev-comment/SKILL.md | 61 ---------- .../skills/claude/roborev-respond/SKILL.md | 61 ++++++++++ .../skills/codex/roborev-comment/SKILL.md | 61 ---------- .../skills/codex/roborev-respond/SKILL.md | 61 ++++++++++ internal/skills/skills.go | 4 +- internal/skills/skills_test.go | 22 ++-- 9 files changed, 235 insertions(+), 235 deletions(-) delete mode 100644 internal/skills/claude/roborev-comment/SKILL.md create mode 100644 internal/skills/claude/roborev-respond/SKILL.md delete mode 100644 internal/skills/codex/roborev-comment/SKILL.md create mode 100644 internal/skills/codex/roborev-respond/SKILL.md diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 849a774..9ed9666 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1343,11 +1343,11 @@ Examples: return cmd } -// respondCmd returns a hidden alias for backward compatibility +// respondCmd returns an alias for commentCmd func respondCmd() *cobra.Command { cmd := commentCmd() cmd.Use = "respond [message]" - cmd.Hidden = true + cmd.Short = "Alias for 'comment' - add a comment to a review" return cmd } diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 617f604..39a3d0c 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -72,7 +72,7 @@ const ( tuiViewReview tuiViewPrompt tuiViewFilter - tuiViewRespond + tuiViewComment tuiViewCommitMsg tuiViewHelp ) @@ -117,11 +117,11 @@ type tuiModel struct { filterSelectedIdx int // Currently highlighted repo in filter list filterSearch string // Search/filter text typed by user - // Respond modal state - respondText string // The response text being typed - respondJobID int64 // Job ID we're responding to - respondCommit string // Short commit SHA for display - respondFromView tuiView // View to return to after respond modal closes + // Comment modal state + commentText string // The response text being typed + commentJobID int64 // Job ID we're responding to + commentCommit string // Short commit SHA for display + commentFromView tuiView // View to return to after comment modal closes // Active filter (applied to queue view) activeRepoFilter []string // Empty = show all, otherwise repo root_paths to filter by @@ -214,7 +214,7 @@ type tuiReposMsg struct { repos []repoFilterItem totalCount int } -type tuiRespondResultMsg struct { +type tuiCommentResultMsg struct { jobID int64 err error } @@ -1236,39 +1236,39 @@ func (m tuiModel) findLastVisibleJob() int { func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { switch msg := msg.(type) { case tea.KeyMsg: - // Handle respond view first (it captures most keys for typing) - if m.currentView == tuiViewRespond { + // Handle comment view first (it captures most keys for typing) + if m.currentView == tuiViewComment { switch msg.String() { case "ctrl+c": return m, tea.Quit case "esc": - m.currentView = m.respondFromView - m.respondText = "" - m.respondJobID = 0 + m.currentView = m.commentFromView + m.commentText = "" + m.commentJobID = 0 return m, nil case "enter": - if strings.TrimSpace(m.respondText) != "" { - text := m.respondText - jobID := m.respondJobID + if strings.TrimSpace(m.commentText) != "" { + text := m.commentText + jobID := m.commentJobID // Return to previous view but keep text until submit succeeds - m.currentView = m.respondFromView - return m, m.submitResponse(jobID, text) + m.currentView = m.commentFromView + return m, m.submitComment(jobID, text) } return m, nil case "backspace": - if len(m.respondText) > 0 { - runes := []rune(m.respondText) - m.respondText = string(runes[:len(runes)-1]) + if len(m.commentText) > 0 { + runes := []rune(m.commentText) + m.commentText = string(runes[:len(runes)-1]) } return m, nil default: // Handle typing (supports non-ASCII runes and newlines) if msg.String() == "shift+enter" || msg.String() == "ctrl+j" { - m.respondText += "\n" + m.commentText += "\n" } else if len(msg.Runes) > 0 { for _, r := range msg.Runes { if unicode.IsPrint(r) || r == '\n' || r == '\t' { - m.respondText += string(r) + m.commentText += string(r) } } } @@ -1699,39 +1699,39 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } case "c": - // Open respond modal (from queue or review view) + // Open comment modal (from queue or review view) if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { job := m.jobs[m.selectedIdx] // Only allow responding to completed or failed reviews if job.Status == storage.JobStatusDone || job.Status == storage.JobStatusFailed { // Only clear text if opening for a different job (preserve for retry) - if m.respondJobID != job.ID { - m.respondText = "" + if m.commentJobID != job.ID { + m.commentText = "" } - m.respondJobID = job.ID - m.respondCommit = job.GitRef - if len(m.respondCommit) > 7 { - m.respondCommit = m.respondCommit[:7] + m.commentJobID = job.ID + m.commentCommit = job.GitRef + if len(m.commentCommit) > 7 { + m.commentCommit = m.commentCommit[:7] } - m.respondFromView = tuiViewQueue - m.currentView = tuiViewRespond + m.commentFromView = tuiViewQueue + m.currentView = tuiViewComment } return m, nil } else if m.currentView == tuiViewReview && m.currentReview != nil { // Only clear text if opening for a different job (preserve for retry) - if m.respondJobID != m.currentReview.JobID { - m.respondText = "" + if m.commentJobID != m.currentReview.JobID { + m.commentText = "" } - m.respondJobID = m.currentReview.JobID - m.respondCommit = "" + m.commentJobID = m.currentReview.JobID + m.commentCommit = "" if m.currentReview.Job != nil { - m.respondCommit = m.currentReview.Job.GitRef - if len(m.respondCommit) > 7 { - m.respondCommit = m.respondCommit[:7] + m.commentCommit = m.currentReview.Job.GitRef + if len(m.commentCommit) > 7 { + m.commentCommit = m.commentCommit[:7] } } - m.respondFromView = tuiViewReview - m.currentView = tuiViewRespond + m.commentFromView = tuiViewReview + m.currentView = tuiViewComment return m, nil } @@ -2151,16 +2151,16 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } } - case tuiRespondResultMsg: + case tuiCommentResultMsg: if msg.err != nil { m.err = msg.err - // Keep respondText and respondJobID so user can retry + // Keep commentText and commentJobID so user can retry } else { // Success - clear the response state only if still for the same job // (user may have started a new draft for a different job while this was in flight) - if m.respondJobID == msg.jobID { - m.respondText = "" - m.respondJobID = 0 + if m.commentJobID == msg.jobID { + m.commentText = "" + m.commentJobID = 0 } // Refresh the review to show the new response (if viewing a review) if m.currentView == tuiViewReview && m.currentReview != nil && m.currentReview.JobID == msg.jobID { @@ -2270,7 +2270,7 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { } func (m tuiModel) View() string { - if m.currentView == tuiViewRespond { + if m.currentView == tuiViewComment { return m.renderRespondView() } if m.currentView == tuiViewFilter { @@ -3010,8 +3010,8 @@ func (m tuiModel) renderRespondView() string { var b strings.Builder title := "Add Comment" - if m.respondCommit != "" { - title = fmt.Sprintf("Add Comment (%s)", m.respondCommit) + if m.commentCommit != "" { + title = fmt.Sprintf("Add Comment (%s)", m.commentCommit) } b.WriteString(tuiTitleStyle.Render(title)) b.WriteString("\x1b[K\n\x1b[K\n") // Clear title and blank line @@ -3034,14 +3034,14 @@ func (m tuiModel) renderRespondView() string { maxTextLines = 3 } - if m.respondText == "" { + if m.commentText == "" { // Show placeholder (styled, but we pad manually to avoid ANSI issues) placeholder := "Type your comment..." padded := placeholder + strings.Repeat(" ", boxWidth-2-len(placeholder)) b.WriteString("| " + tuiStatusStyle.Render(padded) + " |\x1b[K\n") textLinesWritten++ } else { - lines := strings.Split(m.respondText, "\n") + lines := strings.Split(m.commentText, "\n") for _, line := range lines { if textLinesWritten >= maxTextLines { break @@ -3082,7 +3082,7 @@ func (m tuiModel) renderRespondView() string { return b.String() } -func (m tuiModel) submitResponse(jobID int64, text string) tea.Cmd { +func (m tuiModel) submitComment(jobID int64, text string) tea.Cmd { return func() tea.Msg { responder := os.Getenv("USER") if responder == "" { @@ -3097,7 +3097,7 @@ func (m tuiModel) submitResponse(jobID int64, text string) tea.Cmd { body, err := json.Marshal(payload) if err != nil { - return tuiRespondResultMsg{jobID: jobID, err: fmt.Errorf("marshal request: %w", err)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("marshal request: %w", err)} } resp, err := m.client.Post( @@ -3106,15 +3106,15 @@ func (m tuiModel) submitResponse(jobID int64, text string) tea.Cmd { bytes.NewReader(body), ) if err != nil { - return tuiRespondResultMsg{jobID: jobID, err: fmt.Errorf("submit response: %w", err)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit response: %w", err)} } defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - return tuiRespondResultMsg{jobID: jobID, err: fmt.Errorf("submit response: HTTP %d", resp.StatusCode)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit response: HTTP %d", resp.StatusCode)} } - return tuiRespondResultMsg{jobID: jobID, err: nil} + return tuiCommentResultMsg{jobID: jobID, err: nil} } } diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index fe63783..5e28bc9 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -5148,28 +5148,28 @@ func TestTUIRespondTextPreservation(t *testing.T) { updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) m = updated.(tuiModel) - if m.currentView != tuiViewRespond { - t.Fatalf("Expected tuiViewRespond, got %v", m.currentView) + if m.currentView != tuiViewComment { + t.Fatalf("Expected tuiViewComment, got %v", m.currentView) } - if m.respondJobID != 1 { - t.Fatalf("Expected respondJobID=1, got %d", m.respondJobID) + if m.commentJobID != 1 { + t.Fatalf("Expected commentJobID=1, got %d", m.commentJobID) } // 2. Type some text - m.respondText = "My draft response" + m.commentText = "My draft response" // 3. Simulate failed submission - press enter then receive error - m.currentView = m.respondFromView // Simulate what happens on enter - errMsg := tuiRespondResultMsg{jobID: 1, err: fmt.Errorf("network error")} + m.currentView = m.commentFromView // Simulate what happens on enter + errMsg := tuiCommentResultMsg{jobID: 1, err: fmt.Errorf("network error")} updated, _ = m.Update(errMsg) m = updated.(tuiModel) // Text should be preserved after error - if m.respondText != "My draft response" { - t.Errorf("Expected text preserved after error, got %q", m.respondText) + if m.commentText != "My draft response" { + t.Errorf("Expected text preserved after error, got %q", m.commentText) } - if m.respondJobID != 1 { - t.Errorf("Expected respondJobID preserved after error, got %d", m.respondJobID) + if m.commentJobID != 1 { + t.Errorf("Expected commentJobID preserved after error, got %d", m.commentJobID) } // 4. Re-open respond for Job 1 (Retry) - text should still be there @@ -5178,8 +5178,8 @@ func TestTUIRespondTextPreservation(t *testing.T) { updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) m = updated.(tuiModel) - if m.respondText != "My draft response" { - t.Errorf("Expected text preserved on retry for same job, got %q", m.respondText) + if m.commentText != "My draft response" { + t.Errorf("Expected text preserved on retry for same job, got %q", m.commentText) } // 5. Go back to queue and switch to Job 2 - text should be cleared @@ -5189,11 +5189,11 @@ func TestTUIRespondTextPreservation(t *testing.T) { updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'c'}}) m = updated.(tuiModel) - if m.respondText != "" { - t.Errorf("Expected text cleared for different job, got %q", m.respondText) + if m.commentText != "" { + t.Errorf("Expected text cleared for different job, got %q", m.commentText) } - if m.respondJobID != 2 { - t.Errorf("Expected respondJobID=2, got %d", m.respondJobID) + if m.commentJobID != 2 { + t.Errorf("Expected commentJobID=2, got %d", m.commentJobID) } } @@ -5205,32 +5205,32 @@ func TestTUIRespondSuccessClearsOnlyMatchingJob(t *testing.T) { } // User submitted response for job 1, then started drafting for job 2 - m.respondJobID = 2 - m.respondText = "New draft for job 2" + m.commentJobID = 2 + m.commentText = "New draft for job 2" // Success message arrives for job 1 (the old submission) - successMsg := tuiRespondResultMsg{jobID: 1, err: nil} + successMsg := tuiCommentResultMsg{jobID: 1, err: nil} updated, _ := m.Update(successMsg) m = updated.(tuiModel) // Draft for job 2 should NOT be cleared - if m.respondText != "New draft for job 2" { - t.Errorf("Expected draft preserved for different job, got %q", m.respondText) + if m.commentText != "New draft for job 2" { + t.Errorf("Expected draft preserved for different job, got %q", m.commentText) } - if m.respondJobID != 2 { - t.Errorf("Expected respondJobID=2 preserved, got %d", m.respondJobID) + if m.commentJobID != 2 { + t.Errorf("Expected commentJobID=2 preserved, got %d", m.commentJobID) } // Now success for job 2 should clear - successMsg = tuiRespondResultMsg{jobID: 2, err: nil} + successMsg = tuiCommentResultMsg{jobID: 2, err: nil} updated, _ = m.Update(successMsg) m = updated.(tuiModel) - if m.respondText != "" { - t.Errorf("Expected text cleared for matching job, got %q", m.respondText) + if m.commentText != "" { + t.Errorf("Expected text cleared for matching job, got %q", m.commentText) } - if m.respondJobID != 0 { - t.Errorf("Expected respondJobID=0 after success, got %d", m.respondJobID) + if m.commentJobID != 0 { + t.Errorf("Expected commentJobID=0 after success, got %d", m.commentJobID) } } @@ -5268,42 +5268,42 @@ func TestTUIFilterBackspaceMultiByte(t *testing.T) { func TestTUIRespondBackspaceMultiByte(t *testing.T) { m := newTuiModel("http://localhost") - m.currentView = tuiViewRespond - m.respondJobID = 1 + m.currentView = tuiViewComment + m.commentJobID = 1 // Type text with multi-byte characters updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune("Hello 世界")}) m = updated.(tuiModel) - if m.respondText != "Hello 世界" { - t.Errorf("Expected respondText='Hello 世界', got %q", m.respondText) + if m.commentText != "Hello 世界" { + t.Errorf("Expected commentText='Hello 世界', got %q", m.commentText) } // Backspace should remove '界' (one character), not corrupt it updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyBackspace}) m = updated.(tuiModel) - if m.respondText != "Hello 世" { - t.Errorf("Expected respondText='Hello 世' after backspace, got %q", m.respondText) + if m.commentText != "Hello 世" { + t.Errorf("Expected commentText='Hello 世' after backspace, got %q", m.commentText) } // Backspace should remove '世' updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyBackspace}) m = updated.(tuiModel) - if m.respondText != "Hello " { - t.Errorf("Expected respondText='Hello ' after second backspace, got %q", m.respondText) + if m.commentText != "Hello " { + t.Errorf("Expected commentText='Hello ' after second backspace, got %q", m.commentText) } } func TestTUIRespondViewTruncationMultiByte(t *testing.T) { m := newTuiModel("http://localhost") - m.currentView = tuiViewRespond - m.respondJobID = 1 + m.currentView = tuiViewComment + m.commentJobID = 1 m.width = 30 m.height = 20 // Set text with multi-byte characters that would be truncated // The box has boxWidth-2 available space for text - m.respondText = "あいうえおかきくけこさしすせそ" // 15 Japanese characters (30 cells wide) + m.commentText = "あいうえおかきくけこさしすせそ" // 15 Japanese characters (30 cells wide) // Render should not panic or corrupt characters output := m.renderRespondView() @@ -5342,13 +5342,13 @@ func TestTUIRespondViewTruncationMultiByte(t *testing.T) { func TestTUIRespondViewTabExpansion(t *testing.T) { m := newTuiModel("http://localhost") - m.currentView = tuiViewRespond - m.respondJobID = 1 + m.currentView = tuiViewComment + m.commentJobID = 1 m.width = 40 m.height = 20 // Set text with tabs - m.respondText = "a\tb\tc" + m.commentText = "a\tb\tc" output := m.renderRespondView() plainOutput := stripANSI(output) diff --git a/internal/skills/claude/roborev-comment/SKILL.md b/internal/skills/claude/roborev-comment/SKILL.md deleted file mode 100644 index 5e7cf18..0000000 --- a/internal/skills/claude/roborev-comment/SKILL.md +++ /dev/null @@ -1,61 +0,0 @@ ---- -name: roborev:comment -description: Add a comment or note to a roborev code review to document how findings were addressed ---- - -# roborev:comment - -Record a comment on a roborev code review. - -## Usage - -``` -/roborev:comment [message] -``` - -## IMPORTANT - -This skill requires you to **execute a bash command** to record the comment in roborev. The task is not complete until you run the `roborev comment` command and see confirmation output. - -## Instructions - -When the user invokes `/roborev:comment [message]`: - -1. **If a message is provided**, immediately execute: - ```bash - roborev comment --job "" - ``` - -2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their comment. - -3. **Verify success** - the command will output confirmation. If it fails, report the error. - -The comment is recorded in roborev's database and will appear when viewing the review with `roborev show`. - -## Examples - -**With message provided:** - -User: `/roborev:comment 1019 Fixed all issues` - -Agent action: -```bash -roborev comment --job 1019 "Fixed all issues" -``` -Then confirm: "Comment recorded for review #1019." - ---- - -**Without message:** - -User: `/roborev:comment 1019` - -Agent: "What would you like to say about review #1019?" - -User: "The null check was a false positive" - -Agent action: -```bash -roborev comment --job 1019 "The null check was a false positive" -``` -Then confirm: "Comment recorded for review #1019." diff --git a/internal/skills/claude/roborev-respond/SKILL.md b/internal/skills/claude/roborev-respond/SKILL.md new file mode 100644 index 0000000..8f34b57 --- /dev/null +++ b/internal/skills/claude/roborev-respond/SKILL.md @@ -0,0 +1,61 @@ +--- +name: roborev:respond +description: Add a comment to a roborev code review and mark it as addressed +--- + +# roborev:respond + +Record a comment on a roborev code review and mark it as addressed. + +## Usage + +``` +/roborev:respond [message] +``` + +## IMPORTANT + +This skill requires you to **execute bash commands** to record the comment and mark the review addressed. The task is not complete until you run both commands and see confirmation output. + +## Instructions + +When the user invokes `/roborev:respond [message]`: + +1. **If a message is provided**, immediately execute: + ```bash + roborev comment --job "" && roborev address + ``` + +2. **If no message is provided**, ask the user what they'd like to say, then execute the commands with their comment. + +3. **Verify success** - both commands will output confirmation. If either fails, report the error. + +The comment is recorded in roborev's database and the review is marked as addressed. View results with `roborev show`. + +## Examples + +**With message provided:** + +User: `/roborev:respond 1019 Fixed all issues` + +Agent action: +```bash +roborev comment --job 1019 "Fixed all issues" && roborev address 1019 +``` +Then confirm: "Comment recorded and review #1019 marked as addressed." + +--- + +**Without message:** + +User: `/roborev:respond 1019` + +Agent: "What would you like to say about review #1019?" + +User: "The null check was a false positive" + +Agent action: +```bash +roborev comment --job 1019 "The null check was a false positive" && roborev address 1019 +``` +Then confirm: "Comment recorded and review #1019 marked as addressed." diff --git a/internal/skills/codex/roborev-comment/SKILL.md b/internal/skills/codex/roborev-comment/SKILL.md deleted file mode 100644 index 4c1553a..0000000 --- a/internal/skills/codex/roborev-comment/SKILL.md +++ /dev/null @@ -1,61 +0,0 @@ ---- -name: roborev:comment -description: Add a comment or note to a roborev code review to document how findings were addressed ---- - -# roborev:comment - -Record a comment on a roborev code review. - -## Usage - -``` -$roborev:comment [message] -``` - -## IMPORTANT - -This skill requires you to **execute a bash command** to record the comment in roborev. The task is not complete until you run the `roborev comment` command and see confirmation output. - -## Instructions - -When the user invokes `$roborev:comment [message]`: - -1. **If a message is provided**, immediately execute: - ```bash - roborev comment --job "" - ``` - -2. **If no message is provided**, ask the user what they'd like to say, then execute the command with their comment. - -3. **Verify success** - the command will output confirmation. If it fails, report the error. - -The comment is recorded in roborev's database and will appear when viewing the review with `roborev show`. - -## Examples - -**With message provided:** - -User: `$roborev:comment 1019 Fixed all issues` - -Agent action: -```bash -roborev comment --job 1019 "Fixed all issues" -``` -Then confirm: "Comment recorded for review #1019." - ---- - -**Without message:** - -User: `$roborev:comment 1019` - -Agent: "What would you like to say about review #1019?" - -User: "The null check was a false positive" - -Agent action: -```bash -roborev comment --job 1019 "The null check was a false positive" -``` -Then confirm: "Comment recorded for review #1019." diff --git a/internal/skills/codex/roborev-respond/SKILL.md b/internal/skills/codex/roborev-respond/SKILL.md new file mode 100644 index 0000000..3854e5e --- /dev/null +++ b/internal/skills/codex/roborev-respond/SKILL.md @@ -0,0 +1,61 @@ +--- +name: roborev:respond +description: Add a comment to a roborev code review and mark it as addressed +--- + +# roborev:respond + +Record a comment on a roborev code review and mark it as addressed. + +## Usage + +``` +$roborev:respond [message] +``` + +## IMPORTANT + +This skill requires you to **execute bash commands** to record the comment and mark the review addressed. The task is not complete until you run both commands and see confirmation output. + +## Instructions + +When the user invokes `$roborev:respond [message]`: + +1. **If a message is provided**, immediately execute: + ```bash + roborev comment --job "" && roborev address + ``` + +2. **If no message is provided**, ask the user what they'd like to say, then execute the commands with their comment. + +3. **Verify success** - both commands will output confirmation. If either fails, report the error. + +The comment is recorded in roborev's database and the review is marked as addressed. View results with `roborev show`. + +## Examples + +**With message provided:** + +User: `$roborev:respond 1019 Fixed all issues` + +Agent action: +```bash +roborev comment --job 1019 "Fixed all issues" && roborev address 1019 +``` +Then confirm: "Comment recorded and review #1019 marked as addressed." + +--- + +**Without message:** + +User: `$roborev:respond 1019` + +Agent: "What would you like to say about review #1019?" + +User: "The null check was a false positive" + +Agent action: +```bash +roborev comment --job 1019 "The null check was a false positive" && roborev address 1019 +``` +Then confirm: "Comment recorded and review #1019 marked as addressed." diff --git a/internal/skills/skills.go b/internal/skills/skills.go index aa975d8..b4f024d 100644 --- a/internal/skills/skills.go +++ b/internal/skills/skills.go @@ -69,13 +69,13 @@ func IsInstalled(agent Agent) bool { skillsDir := filepath.Join(home, ".claude", "skills") checkFiles = []string{ filepath.Join(skillsDir, "roborev-address", "SKILL.md"), - filepath.Join(skillsDir, "roborev-comment", "SKILL.md"), + filepath.Join(skillsDir, "roborev-respond", "SKILL.md"), } case AgentCodex: skillsDir := filepath.Join(home, ".codex", "skills") checkFiles = []string{ filepath.Join(skillsDir, "roborev-address", "SKILL.md"), - filepath.Join(skillsDir, "roborev-comment", "SKILL.md"), + filepath.Join(skillsDir, "roborev-respond", "SKILL.md"), } default: return false diff --git a/internal/skills/skills_test.go b/internal/skills/skills_test.go index 99b03d4..d0a8a7a 100644 --- a/internal/skills/skills_test.go +++ b/internal/skills/skills_test.go @@ -99,8 +99,8 @@ func TestInstallClaudeWhenDirExists(t *testing.T) { if _, err := os.Stat(filepath.Join(skillsDir, "roborev-address", "SKILL.md")); err != nil { t.Error("expected roborev-address/SKILL.md to exist") } - if _, err := os.Stat(filepath.Join(skillsDir, "roborev-comment", "SKILL.md")); err != nil { - t.Error("expected roborev-comment/SKILL.md to exist") + if _, err := os.Stat(filepath.Join(skillsDir, "roborev-respond", "SKILL.md")); err != nil { + t.Error("expected roborev-respond/SKILL.md to exist") } } @@ -143,8 +143,8 @@ func TestInstallCodexWhenDirExists(t *testing.T) { if _, err := os.Stat(filepath.Join(skillsDir, "roborev-address", "SKILL.md")); err != nil { t.Error("expected roborev-address/SKILL.md to exist") } - if _, err := os.Stat(filepath.Join(skillsDir, "roborev-comment", "SKILL.md")); err != nil { - t.Error("expected roborev-comment/SKILL.md to exist") + if _, err := os.Stat(filepath.Join(skillsDir, "roborev-respond", "SKILL.md")); err != nil { + t.Error("expected roborev-respond/SKILL.md to exist") } } @@ -220,7 +220,7 @@ func TestIsInstalledClaude(t *testing.T) { // Create only respond skill (not address) skillsDir := filepath.Join(claudeDir, "skills") - respondDir := filepath.Join(skillsDir, "roborev-comment") + respondDir := filepath.Join(skillsDir, "roborev-respond") if err := os.MkdirAll(respondDir, 0755); err != nil { t.Fatal(err) } @@ -228,7 +228,7 @@ func TestIsInstalledClaude(t *testing.T) { t.Fatal(err) } if !IsInstalled(AgentClaude) { - t.Error("expected IsInstalled=true when roborev-comment/SKILL.md exists") + t.Error("expected IsInstalled=true when roborev-respond/SKILL.md exists") } // Remove respond, add address @@ -265,7 +265,7 @@ func TestIsInstalledCodex(t *testing.T) { } // Create only respond skill - respondDir := filepath.Join(codexDir, "skills", "roborev-comment") + respondDir := filepath.Join(codexDir, "skills", "roborev-respond") if err := os.MkdirAll(respondDir, 0755); err != nil { t.Fatal(err) } @@ -273,7 +273,7 @@ func TestIsInstalledCodex(t *testing.T) { t.Fatal(err) } if !IsInstalled(AgentCodex) { - t.Error("expected IsInstalled=true when roborev-comment/SKILL.md exists") + t.Error("expected IsInstalled=true when roborev-respond/SKILL.md exists") } } @@ -318,7 +318,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { defer cleanup() // Create .claude with only respond skill installed - claudeSkillsDir := filepath.Join(tmpHome, ".claude", "skills", "roborev-comment") + claudeSkillsDir := filepath.Join(tmpHome, ".claude", "skills", "roborev-respond") if err := os.MkdirAll(claudeSkillsDir, 0755); err != nil { t.Fatal(err) } @@ -381,7 +381,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { defer cleanup() // Create .codex with only respond skill installed - codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-comment") + codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-respond") if err := os.MkdirAll(codexSkillsDir, 0755); err != nil { t.Fatal(err) } @@ -417,7 +417,7 @@ func TestUpdateOnlyUpdatesInstalled(t *testing.T) { } // Create .codex with skills - codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-comment") + codexSkillsDir := filepath.Join(tmpHome, ".codex", "skills", "roborev-respond") if err := os.MkdirAll(codexSkillsDir, 0755); err != nil { t.Fatal(err) } From 3fea8f8240ac8cb964327a710d3fe64369cdc340 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 12:10:41 -0500 Subject: [PATCH 04/23] Normalize user-facing terminology to use "comment" instead of "response" - Prompt: "Comments on this review:" instead of "Responses" - Sync output: "comments" instead of "responses" - Temp file: roborev-comment-*.md Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main.go | 14 +++++++------- internal/prompt/prompt.go | 4 ++-- internal/prompt/prompt_test.go | 24 ++++++++++++------------ 3 files changed, 21 insertions(+), 21 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 9ed9666..70878d6 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1273,7 +1273,7 @@ Examples: editor = "vim" } - tmpfile, err := os.CreateTemp("", "roborev-response-*.md") + tmpfile, err := os.CreateTemp("", "roborev-comment-*.md") if err != nil { return fmt.Errorf("create temp file: %w", err) } @@ -1290,7 +1290,7 @@ Examples: content, err := os.ReadFile(tmpfile.Name()) if err != nil { - return fmt.Errorf("read response: %w", err) + return fmt.Errorf("read comment: %w", err) } message = strings.TrimSpace(string(content)) } @@ -2162,7 +2162,7 @@ func syncStatusCmd() *cobra.Command { } return fmt.Sprintf("%d", count) } - fmt.Printf("Pending push: %s jobs, %s reviews, %s responses\n", + fmt.Printf("Pending push: %s jobs, %s reviews, %s comments\n", formatCount(len(jobs)), formatCount(len(reviews)), formatCount(len(responses))) // Try to connect to PostgreSQL @@ -2266,13 +2266,13 @@ func syncNowCmd() *cobra.Command { totalJobs := getInt(msg, "total_jobs") totalRevs := getInt(msg, "total_revs") totalResps := getInt(msg, "total_resps") - fmt.Printf("\rPushing: batch %d (total: %d jobs, %d reviews, %d responses) ", + fmt.Printf("\rPushing: batch %d (total: %d jobs, %d reviews, %d comments) ", batch, totalJobs, totalRevs, totalResps) } else if phase == "pull" { totalJobs := getInt(msg, "total_jobs") totalRevs := getInt(msg, "total_revs") totalResps := getInt(msg, "total_resps") - fmt.Printf("\rPulled: %d jobs, %d reviews, %d responses \n", + fmt.Printf("\rPulled: %d jobs, %d reviews, %d comments \n", totalJobs, totalRevs, totalResps) } case "error": @@ -2298,9 +2298,9 @@ func syncNowCmd() *cobra.Command { } fmt.Println("Sync completed") - fmt.Printf("Pushed: %d jobs, %d reviews, %d responses\n", + fmt.Printf("Pushed: %d jobs, %d reviews, %d comments\n", finalPushed.Jobs, finalPushed.Reviews, finalPushed.Responses) - fmt.Printf("Pulled: %d jobs, %d reviews, %d responses\n", + fmt.Printf("Pulled: %d jobs, %d reviews, %d comments\n", finalPulled.Jobs, finalPulled.Reviews, finalPulled.Responses) return nil diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 6963219..2e31e7c 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -370,7 +370,7 @@ func (b *Builder) writePreviousReviews(sb *strings.Builder, contexts []ReviewCon // Include responses to this review if len(ctx.Responses) > 0 { - sb.WriteString("\nResponses to this review:\n") + sb.WriteString("\nComments on this review:\n") for _, resp := range ctx.Responses { sb.WriteString(fmt.Sprintf("- %s: %q\n", resp.Responder, resp.Response)) } @@ -415,7 +415,7 @@ func (b *Builder) writePreviousAttemptsForGitRef(sb *strings.Builder, gitRef str if review.JobID > 0 { responses, err := b.db.GetResponsesForJob(review.JobID) if err == nil && len(responses) > 0 { - sb.WriteString("\nResponses to this review:\n") + sb.WriteString("\nComments on this review:\n") for _, resp := range responses { sb.WriteString(fmt.Sprintf("- %s: %q\n", resp.Responder, resp.Response)) } diff --git a/internal/prompt/prompt_test.go b/internal/prompt/prompt_test.go index 18be53b..1e1eb14 100644 --- a/internal/prompt/prompt_test.go +++ b/internal/prompt/prompt_test.go @@ -244,25 +244,25 @@ func TestBuildPromptWithPreviousReviewsAndResponses(t *testing.T) { t.Error("Prompt should contain the previous review text") } - // Should contain responses to the previous review - if !strings.Contains(prompt, "Responses to this review:") { - t.Error("Prompt should contain responses section for previous review") + // Should contain comments on the previous review + if !strings.Contains(prompt, "Comments on this review:") { + t.Error("Prompt should contain comments section for previous review") } if !strings.Contains(prompt, "alice") { - t.Error("Prompt should contain responder 'alice'") + t.Error("Prompt should contain commenter 'alice'") } if !strings.Contains(prompt, "Known issue, will fix in next sprint") { - t.Error("Prompt should contain alice's response text") + t.Error("Prompt should contain alice's comment text") } if !strings.Contains(prompt, "bob") { - t.Error("Prompt should contain responder 'bob'") + t.Error("Prompt should contain commenter 'bob'") } if !strings.Contains(prompt, "Added to tech debt backlog") { - t.Error("Prompt should contain bob's response text") + t.Error("Prompt should contain bob's comment text") } } @@ -607,17 +607,17 @@ func TestBuildPromptWithPreviousAttemptsAndResponses(t *testing.T) { t.Error("Prompt should contain the previous review text") } - // Should contain the response - if !strings.Contains(prompt, "Responses to this review:") { - t.Error("Prompt should contain responses section") + // Should contain the comment + if !strings.Contains(prompt, "Comments on this review:") { + t.Error("Prompt should contain comments section") } if !strings.Contains(prompt, "This is intentional") { - t.Error("Prompt should contain the response text") + t.Error("Prompt should contain the comment text") } if !strings.Contains(prompt, "developer") { - t.Error("Prompt should contain the responder name") + t.Error("Prompt should contain the commenter name") } } From 50b7c7f151d15d5ff9a251267dbf496a195ff35d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 12:20:50 -0500 Subject: [PATCH 05/23] Rename AddResponse/GetResponses functions to AddComment/GetComments Internal function/method renames for terminology consistency: - AddResponse -> AddComment - AddResponseToJob -> AddCommentToJob - GetResponsesForCommit -> GetCommentsForCommit - GetResponsesForJob -> GetCommentsForJob - GetResponsesForCommitSHA -> GetCommentsForCommitSHA - GetResponsesToSync -> GetCommentsToSync - MarkResponseSynced -> MarkCommentSynced - MarkResponsesSynced -> MarkCommentsSynced The database schema (responses table) and API endpoints (/api/respond) remain unchanged - this is purely internal code terminology. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/client_test.go | 8 ++--- cmd/roborev/main.go | 8 ++--- cmd/roborev/main_test.go | 8 ++--- cmd/roborev/refine.go | 12 +++---- cmd/roborev/refine_test.go | 4 +-- e2e_test.go | 18 +++++----- internal/daemon/client.go | 14 ++++---- internal/daemon/client_test.go | 6 ++-- internal/daemon/server.go | 24 ++++++------- internal/prompt/prompt.go | 8 ++--- internal/prompt/prompt_test.go | 14 ++++---- internal/storage/db.go | 2 +- internal/storage/db_test.go | 18 +++++----- internal/storage/repos_test.go | 12 +++---- internal/storage/reviews.go | 22 ++++++------ internal/storage/sync.go | 14 ++++---- internal/storage/sync_test.go | 64 +++++++++++++++++----------------- internal/storage/syncworker.go | 10 +++--- 18 files changed, 133 insertions(+), 133 deletions(-) diff --git a/cmd/roborev/client_test.go b/cmd/roborev/client_test.go index 9e4980a..1f2e44b 100644 --- a/cmd/roborev/client_test.go +++ b/cmd/roborev/client_test.go @@ -1,6 +1,6 @@ package main -// Tests for daemon client functions (getResponsesForJob, waitForReview, findJobForCommit) +// Tests for daemon client functions (getCommentsForJob, waitForReview, findJobForCommit) import ( "encoding/json" @@ -14,7 +14,7 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) -func TestGetResponsesForJob(t *testing.T) { +func TestGetCommentsForJob(t *testing.T) { t.Run("returns responses for job", func(t *testing.T) { _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { if r.URL.Path != "/api/responses" || r.Method != "GET" { @@ -36,7 +36,7 @@ func TestGetResponsesForJob(t *testing.T) { })) defer cleanup() - responses, err := getResponsesForJob(42) + responses, err := getCommentsForJob(42) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -51,7 +51,7 @@ func TestGetResponsesForJob(t *testing.T) { })) defer cleanup() - _, err := getResponsesForJob(42) + _, err := getCommentsForJob(42) if err == nil { t.Fatal("expected error, got nil") } diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 70878d6..640ae9d 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1570,8 +1570,8 @@ func enqueueReview(repoPath, gitRef, agentName string) (int64, error) { return job.ID, nil } -// getResponsesForJob fetches responses for a job -func getResponsesForJob(jobID int64) ([]storage.Response, error) { +// getCommentsForJob fetches comments for a job +func getCommentsForJob(jobID int64) ([]storage.Response, error) { addr := getDaemonAddr() client := &http.Client{Timeout: 5 * time.Second} @@ -1582,7 +1582,7 @@ func getResponsesForJob(jobID int64) ([]storage.Response, error) { defer resp.Body.Close() if resp.StatusCode != http.StatusOK { - return nil, fmt.Errorf("fetch responses: %s", resp.Status) + return nil, fmt.Errorf("fetch comments: %s", resp.Status) } var result struct { @@ -2148,7 +2148,7 @@ func syncStatusCmd() *cobra.Command { const maxPending = 1000 jobs, jobsErr := db.GetJobsToSync(machineID, maxPending) reviews, reviewsErr := db.GetReviewsToSync(machineID, maxPending) - responses, responsesErr := db.GetResponsesToSync(machineID, maxPending) + responses, responsesErr := db.GetCommentsToSync(machineID, maxPending) fmt.Println() if jobsErr != nil || reviewsErr != nil || responsesErr != nil { diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index c495989..7b1e662 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -293,7 +293,7 @@ func TestRefineNoChangeRetryLogic(t *testing.T) { })) defer cleanup() - responses, _ := getResponsesForJob(1) + responses, _ := getCommentsForJob(1) // Count no-change attempts noChangeAttempts := 0 @@ -807,7 +807,7 @@ func TestRefineLoopNoChangeRetryScenario(t *testing.T) { _, cleanup := setupMockDaemon(t, createMockRefineHandler(state)) defer cleanup() - responses, err := getResponsesForJob(42) + responses, err := getCommentsForJob(42) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -834,7 +834,7 @@ func TestRefineLoopNoChangeRetryScenario(t *testing.T) { _, cleanup := setupMockDaemon(t, createMockRefineHandler(state)) defer cleanup() - responses, _ := getResponsesForJob(42) + responses, _ := getCommentsForJob(42) noChangeAttempts := countNoChangeAttempts(responses) // Should not give up yet (first attempt) @@ -856,7 +856,7 @@ func TestRefineLoopNoChangeRetryScenario(t *testing.T) { _, cleanup := setupMockDaemon(t, createMockRefineHandler(state)) defer cleanup() - responses, _ := getResponsesForJob(42) + responses, _ := getCommentsForJob(42) noChangeAttempts := countNoChangeAttempts(responses) // Should only count the 1 response from roborev-refine, not the others diff --git a/cmd/roborev/refine.go b/cmd/roborev/refine.go index 76a06f7..514c7c6 100644 --- a/cmd/roborev/refine.go +++ b/cmd/roborev/refine.go @@ -351,9 +351,9 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al } // Get previous attempts for context - previousAttempts, err := client.GetResponsesForJob(currentFailedReview.JobID) + previousAttempts, err := client.GetCommentsForJob(currentFailedReview.JobID) if err != nil { - return fmt.Errorf("fetch previous responses: %w", err) + return fmt.Errorf("fetch previous comments: %w", err) } // Build address prompt @@ -434,7 +434,7 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al cleanupWorktree() fmt.Println("Agent made no changes") // Check how many times we've tried this review (only count our own attempts) - attempts, err := client.GetResponsesForJob(currentFailedReview.JobID) + attempts, err := client.GetCommentsForJob(currentFailedReview.JobID) if err != nil { return fmt.Errorf("fetch attempts: %w", err) } @@ -448,12 +448,12 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al // Tried 3 times (including this one), give up on this review // Do NOT mark as addressed - the review still needs attention fmt.Println("Giving up after multiple failed attempts (review remains unaddressed)") - client.AddResponse(currentFailedReview.JobID, "roborev-refine", "Agent could not determine how to address findings (attempt 3, giving up)") + client.AddComment(currentFailedReview.JobID, "roborev-refine", "Agent could not determine how to address findings (attempt 3, giving up)") skippedReviews[currentFailedReview.ID] = true // Don't re-select this run currentFailedReview = nil // Move on to next oldest failed commit } else { // Record attempt but don't mark addressed - might work on retry with different context - client.AddResponse(currentFailedReview.JobID, "roborev-refine", fmt.Sprintf("Agent could not determine how to address findings (attempt %d)", noChangeAttempts+1)) + client.AddComment(currentFailedReview.JobID, "roborev-refine", fmt.Sprintf("Agent could not determine how to address findings (attempt %d)", noChangeAttempts+1)) fmt.Printf("Attempt %d failed, will retry\n", noChangeAttempts+1) } continue @@ -476,7 +476,7 @@ func runRefine(agentName, reasoningStr string, maxIterations int, quiet bool, al // Add response recording what was done (include full agent output for database) responseText := fmt.Sprintf("Created commit %s to address findings\n\n%s", newCommit[:7], output) - client.AddResponse(currentFailedReview.JobID, "roborev-refine", responseText) + client.AddComment(currentFailedReview.JobID, "roborev-refine", responseText) // Mark old review as addressed if err := client.MarkReviewAddressed(currentFailedReview.ID); err != nil { diff --git a/cmd/roborev/refine_test.go b/cmd/roborev/refine_test.go index 43939da..b405b40 100644 --- a/cmd/roborev/refine_test.go +++ b/cmd/roborev/refine_test.go @@ -78,7 +78,7 @@ func (m *mockDaemonClient) MarkReviewAddressed(reviewID int64) error { return nil } -func (m *mockDaemonClient) AddResponse(jobID int64, responder, response string) error { +func (m *mockDaemonClient) AddComment(jobID int64, responder, response string) error { m.addedResponses = append(m.addedResponses, addedResponse{jobID, responder, response}) return nil } @@ -116,7 +116,7 @@ func (m *mockDaemonClient) FindPendingJobForRef(repoPath, gitRef string) (*stora return nil, nil } -func (m *mockDaemonClient) GetResponsesForJob(jobID int64) ([]storage.Response, error) { +func (m *mockDaemonClient) GetCommentsForJob(jobID int64) ([]storage.Response, error) { return m.responses[jobID], nil } diff --git a/e2e_test.go b/e2e_test.go index f22a371..fcc0dbc 100644 --- a/e2e_test.go +++ b/e2e_test.go @@ -157,22 +157,22 @@ func TestDatabaseIntegration(t *testing.T) { t.Errorf("Review output doesn't contain expected text: %s", review.Output) } - // Add a response - resp, err := db.AddResponse(commit.ID, "human-reviewer", "Agreed, LGTM!") + // Add a comment + resp, err := db.AddComment(commit.ID, "human-reviewer", "Agreed, LGTM!") if err != nil { - t.Fatalf("AddResponse failed: %v", err) + t.Fatalf("AddComment failed: %v", err) } if resp.Response != "Agreed, LGTM!" { - t.Errorf("Response not saved correctly") + t.Errorf("Comment not saved correctly") } - // Verify response can be fetched - responses, err := db.GetResponsesForCommitSHA("abc123") + // Verify comment can be fetched + comments, err := db.GetCommentsForCommitSHA("abc123") if err != nil { - t.Fatalf("GetResponsesForCommitSHA failed: %v", err) + t.Fatalf("GetCommentsForCommitSHA failed: %v", err) } - if len(responses) != 1 { - t.Errorf("Expected 1 response, got %d", len(responses)) + if len(comments) != 1 { + t.Errorf("Expected 1 comment, got %d", len(comments)) } } diff --git a/internal/daemon/client.go b/internal/daemon/client.go index aa7318a..8c756f9 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -26,8 +26,8 @@ type Client interface { // MarkReviewAddressed marks a review as addressed MarkReviewAddressed(reviewID int64) error - // AddResponse adds a response to a job - AddResponse(jobID int64, responder, response string) error + // AddComment adds a comment to a job + AddComment(jobID int64, responder, response string) error // EnqueueReview enqueues a review job and returns the job ID EnqueueReview(repoPath, gitRef, agentName string) (int64, error) @@ -41,8 +41,8 @@ type Client interface { // FindPendingJobForRef finds a queued or running job for any git ref FindPendingJobForRef(repoPath, gitRef string) (*storage.ReviewJob, error) - // GetResponsesForJob fetches responses for a job - GetResponsesForJob(jobID int64) ([]storage.Response, error) + // GetCommentsForJob fetches comments for a job + GetCommentsForJob(jobID int64) ([]storage.Response, error) } // DefaultPollInterval is the default polling interval for WaitForReview. @@ -150,7 +150,7 @@ func (c *HTTPClient) MarkReviewAddressed(reviewID int64) error { return nil } -func (c *HTTPClient) AddResponse(jobID int64, responder, response string) error { +func (c *HTTPClient) AddComment(jobID int64, responder, response string) error { reqBody, _ := json.Marshal(map[string]interface{}{ "job_id": jobID, "responder": responder, @@ -165,7 +165,7 @@ func (c *HTTPClient) AddResponse(jobID int64, responder, response string) error if resp.StatusCode != http.StatusOK && resp.StatusCode != http.StatusCreated { body, _ := io.ReadAll(resp.Body) - return fmt.Errorf("add response: %s: %s", resp.Status, body) + return fmt.Errorf("add comment: %s: %s", resp.Status, body) } return nil @@ -373,7 +373,7 @@ func (c *HTTPClient) FindPendingJobForRef(repoPath, gitRef string) (*storage.Rev return nil, nil } -func (c *HTTPClient) GetResponsesForJob(jobID int64) ([]storage.Response, error) { +func (c *HTTPClient) GetCommentsForJob(jobID int64) ([]storage.Response, error) { resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/responses?job_id=%d", c.addr, jobID)) if err != nil { return nil, err diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index 65d08da..0973119 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -15,7 +15,7 @@ import ( "github.com/roborev-dev/roborev/internal/storage" ) -func TestHTTPClientAddResponse(t *testing.T) { +func TestHTTPClientAddComment(t *testing.T) { var received map[string]interface{} server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { @@ -32,8 +32,8 @@ func TestHTTPClientAddResponse(t *testing.T) { defer server.Close() client := NewHTTPClient(server.URL) - if err := client.AddResponse(42, "test-agent", "Fixed the issue"); err != nil { - t.Fatalf("AddResponse failed: %v", err) + if err := client.AddComment(42, "test-agent", "Fixed the issue"); err != nil { + t.Fatalf("AddComment failed: %v", err) } if received["job_id"].(float64) != 42 { diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 993b933..4a567b9 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -69,8 +69,8 @@ func NewServer(db *storage.DB, cfg *config.Config, configPath string) *Server { mux.HandleFunc("/api/repos", s.handleListRepos) mux.HandleFunc("/api/review", s.handleGetReview) mux.HandleFunc("/api/review/address", s.handleAddressReview) - mux.HandleFunc("/api/respond", s.handleAddResponse) - mux.HandleFunc("/api/responses", s.handleListResponses) + mux.HandleFunc("/api/respond", s.handleAddComment) + mux.HandleFunc("/api/responses", s.handleListComments) mux.HandleFunc("/api/status", s.handleStatus) mux.HandleFunc("/api/stream/events", s.handleStreamEvents) mux.HandleFunc("/api/sync/now", s.handleSyncNow) @@ -707,20 +707,20 @@ func (s *Server) handleGetReview(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, review) } -type AddResponseRequest struct { +type AddCommentRequest struct { SHA string `json:"sha,omitempty"` // Legacy: link to commit by SHA JobID int64 `json:"job_id,omitempty"` // Preferred: link to job Responder string `json:"responder"` Response string `json:"response"` } -func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { +func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodPost { writeError(w, http.StatusMethodNotAllowed, "method not allowed") return } - var req AddResponseRequest + var req AddCommentRequest if err := json.NewDecoder(r.Body).Decode(&req); err != nil { writeError(w, http.StatusBadRequest, "invalid request body") return @@ -742,13 +742,13 @@ func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { if req.JobID != 0 { // Link to job (preferred method) - resp, err = s.db.AddResponseToJob(req.JobID, req.Responder, req.Response) + resp, err = s.db.AddCommentToJob(req.JobID, req.Responder, req.Response) if err != nil { if errors.Is(err, sql.ErrNoRows) { writeError(w, http.StatusNotFound, "job not found") return } - writeError(w, http.StatusInternalServerError, fmt.Sprintf("add response: %v", err)) + writeError(w, http.StatusInternalServerError, fmt.Sprintf("add comment: %v", err)) return } } else { @@ -759,9 +759,9 @@ func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { return } - resp, err = s.db.AddResponse(commit.ID, req.Responder, req.Response) + resp, err = s.db.AddComment(commit.ID, req.Responder, req.Response) if err != nil { - writeError(w, http.StatusInternalServerError, fmt.Sprintf("add response: %v", err)) + writeError(w, http.StatusInternalServerError, fmt.Sprintf("add comment: %v", err)) return } } @@ -769,7 +769,7 @@ func (s *Server) handleAddResponse(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusCreated, resp) } -func (s *Server) handleListResponses(w http.ResponseWriter, r *http.Request) { +func (s *Server) handleListComments(w http.ResponseWriter, r *http.Request) { if r.Method != http.MethodGet { writeError(w, http.StatusMethodNotAllowed, "method not allowed") return @@ -785,13 +785,13 @@ func (s *Server) handleListResponses(w http.ResponseWriter, r *http.Request) { writeError(w, http.StatusBadRequest, "invalid job_id") return } - responses, err = s.db.GetResponsesForJob(jobID) + responses, err = s.db.GetCommentsForJob(jobID) if err != nil { writeError(w, http.StatusInternalServerError, fmt.Sprintf("get responses: %v", err)) return } } else if sha := r.URL.Query().Get("sha"); sha != "" { - responses, err = s.db.GetResponsesForCommitSHA(sha) + responses, err = s.db.GetCommentsForCommitSHA(sha) if err != nil { writeError(w, http.StatusNotFound, "commit not found") return diff --git a/internal/prompt/prompt.go b/internal/prompt/prompt.go index 2e31e7c..9b4eba5 100644 --- a/internal/prompt/prompt.go +++ b/internal/prompt/prompt.go @@ -411,9 +411,9 @@ func (b *Builder) writePreviousAttemptsForGitRef(sb *strings.Builder, gitRef str sb.WriteString(review.Output) sb.WriteString("\n") - // Fetch and include responses for this review + // Fetch and include comments for this review if review.JobID > 0 { - responses, err := b.db.GetResponsesForJob(review.JobID) + responses, err := b.db.GetCommentsForJob(review.JobID) if err == nil && len(responses) > 0 { sb.WriteString("\nComments on this review:\n") for _, resp := range responses { @@ -442,9 +442,9 @@ func (b *Builder) getPreviousReviewContexts(repoPath, sha string, count int) ([] if err == nil { ctx.Review = review - // Also fetch responses for this review's job + // Also fetch comments for this review's job if review.JobID > 0 { - responses, err := b.db.GetResponsesForJob(review.JobID) + responses, err := b.db.GetCommentsForJob(review.JobID) if err == nil { ctx.Responses = responses } diff --git a/internal/prompt/prompt_test.go b/internal/prompt/prompt_test.go index 1e1eb14..2ce907d 100644 --- a/internal/prompt/prompt_test.go +++ b/internal/prompt/prompt_test.go @@ -212,14 +212,14 @@ func TestBuildPromptWithPreviousReviewsAndResponses(t *testing.T) { db.ClaimJob("test-worker") db.CompleteJob(job.ID, "test", "prompt", "Found potential memory leak in connection pool") - // Add responses to the previous review - _, err = db.AddResponseToJob(job.ID, "alice", "Known issue, will fix in next sprint") + // Add comments to the previous review + _, err = db.AddCommentToJob(job.ID, "alice", "Known issue, will fix in next sprint") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } - _, err = db.AddResponseToJob(job.ID, "bob", "Added to tech debt backlog") + _, err = db.AddCommentToJob(job.ID, "bob", "Added to tech debt backlog") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Also add commits 4 and 5 to DB @@ -586,9 +586,9 @@ func TestBuildPromptWithPreviousAttemptsAndResponses(t *testing.T) { db.CompleteJob(job.ID, "test", "prompt", "Found issue: missing null check") // Add a response to the previous review - _, err = db.AddResponseToJob(job.ID, "developer", "This is intentional, the value is never null here") + _, err = db.AddCommentToJob(job.ID, "developer", "This is intentional, the value is never null here") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Build prompt for a new review of the same commit diff --git a/internal/storage/db.go b/internal/storage/db.go index 61b797f..96d567f 100644 --- a/internal/storage/db.go +++ b/internal/storage/db.go @@ -618,7 +618,7 @@ func (db *DB) migrateSyncColumns() error { return fmt.Errorf("create idx_responses_uuid: %w", err) } - // Create index for GetResponsesToSync query pattern (source_machine_id + synced_at) + // Create index for GetCommentsToSync query pattern (source_machine_id + synced_at) _, err = db.Exec(`CREATE INDEX IF NOT EXISTS idx_responses_sync ON responses(source_machine_id, synced_at)`) if err != nil { return fmt.Errorf("create idx_responses_sync: %w", err) diff --git a/internal/storage/db_test.go b/internal/storage/db_test.go index f572f16..bf54d9c 100644 --- a/internal/storage/db_test.go +++ b/internal/storage/db_test.go @@ -311,24 +311,24 @@ func TestResponseOperations(t *testing.T) { repo, _ := db.GetOrCreateRepo("/tmp/test-repo") commit, _ := db.GetOrCreateCommit(repo.ID, "resp123", "Author", "Subject", time.Now()) - // Add response - resp, err := db.AddResponse(commit.ID, "test-user", "LGTM!") + // Add comment + resp, err := db.AddComment(commit.ID, "test-user", "LGTM!") if err != nil { - t.Fatalf("AddResponse failed: %v", err) + t.Fatalf("AddComment failed: %v", err) } if resp.Response != "LGTM!" { - t.Errorf("Expected response 'LGTM!', got '%s'", resp.Response) + t.Errorf("Expected comment 'LGTM!', got '%s'", resp.Response) } - // Get responses - responses, err := db.GetResponsesForCommit(commit.ID) + // Get comments + comments, err := db.GetCommentsForCommit(commit.ID) if err != nil { - t.Fatalf("GetResponsesForCommit failed: %v", err) + t.Fatalf("GetCommentsForCommit failed: %v", err) } - if len(responses) != 1 { - t.Errorf("Expected 1 response, got %d", len(responses)) + if len(comments) != 1 { + t.Errorf("Expected 1 comment, got %d", len(comments)) } } diff --git a/internal/storage/repos_test.go b/internal/storage/repos_test.go index 1b1543a..16613f1 100644 --- a/internal/storage/repos_test.go +++ b/internal/storage/repos_test.go @@ -523,8 +523,8 @@ func TestDeleteRepo(t *testing.T) { db.ClaimJob("worker-1") db.CompleteJob(job.ID, "codex", "prompt", "output") - // Add a response - db.AddResponseToJob(job.ID, "user", "comment") + // Add a comment + db.AddCommentToJob(job.ID, "user", "comment") err := db.DeleteRepo(repo.ID, true) if err != nil { @@ -720,13 +720,13 @@ func TestDeleteRepoCascadeDeletesLegacyCommitResponses(t *testing.T) { repo, _ := db.GetOrCreateRepo("/tmp/delete-legacy-resp-test") commit, _ := db.GetOrCreateCommit(repo.ID, "legacy-resp-commit", "A", "S", time.Now()) - // Add legacy commit-based response (not job-based) - _, err := db.AddResponse(commit.ID, "reviewer", "Legacy comment on commit") + // Add legacy commit-based comment (not job-based) + _, err := db.AddComment(commit.ID, "reviewer", "Legacy comment on commit") if err != nil { - t.Fatalf("AddResponse failed: %v", err) + t.Fatalf("AddComment failed: %v", err) } - // Verify response exists + // Verify comment exists var beforeCount int db.QueryRow(`SELECT COUNT(*) FROM responses WHERE commit_id = ?`, commit.ID).Scan(&beforeCount) if beforeCount != 1 { diff --git a/internal/storage/reviews.go b/internal/storage/reviews.go index eae80a8..f767b50 100644 --- a/internal/storage/reviews.go +++ b/internal/storage/reviews.go @@ -252,8 +252,8 @@ func (db *DB) GetReviewByID(reviewID int64) (*Review, error) { return &r, nil } -// AddResponse adds a response to a commit (legacy - use AddResponseToJob for new code) -func (db *DB) AddResponse(commitID int64, responder, response string) (*Response, error) { +// AddComment adds a comment to a commit (legacy - use AddCommentToJob for new code) +func (db *DB) AddComment(commitID int64, responder, response string) (*Response, error) { uuid := GenerateUUID() machineID, _ := db.GetMachineID() now := time.Now() @@ -277,8 +277,8 @@ func (db *DB) AddResponse(commitID int64, responder, response string) (*Response }, nil } -// AddResponseToJob adds a response linked to a job/review -func (db *DB) AddResponseToJob(jobID int64, responder, response string) (*Response, error) { +// AddCommentToJob adds a comment linked to a job/review +func (db *DB) AddCommentToJob(jobID int64, responder, response string) (*Response, error) { // Verify job exists first to return proper 404 instead of FK violation or orphaned row var exists int err := db.QueryRow(`SELECT 1 FROM review_jobs WHERE id = ?`, jobID).Scan(&exists) @@ -312,8 +312,8 @@ func (db *DB) AddResponseToJob(jobID int64, responder, response string) (*Respon }, nil } -// GetResponsesForCommit returns all responses for a commit -func (db *DB) GetResponsesForCommit(commitID int64) ([]Response, error) { +// GetCommentsForCommit returns all comments for a commit +func (db *DB) GetCommentsForCommit(commitID int64) ([]Response, error) { rows, err := db.Query(` SELECT id, commit_id, job_id, responder, response, created_at FROM responses @@ -346,8 +346,8 @@ func (db *DB) GetResponsesForCommit(commitID int64) ([]Response, error) { return responses, rows.Err() } -// GetResponsesForJob returns all responses linked to a job -func (db *DB) GetResponsesForJob(jobID int64) ([]Response, error) { +// GetCommentsForJob returns all comments linked to a job +func (db *DB) GetCommentsForJob(jobID int64) ([]Response, error) { rows, err := db.Query(` SELECT id, commit_id, job_id, responder, response, created_at FROM responses @@ -380,11 +380,11 @@ func (db *DB) GetResponsesForJob(jobID int64) ([]Response, error) { return responses, rows.Err() } -// GetResponsesForCommitSHA returns all responses for a commit by SHA -func (db *DB) GetResponsesForCommitSHA(sha string) ([]Response, error) { +// GetCommentsForCommitSHA returns all comments for a commit by SHA +func (db *DB) GetCommentsForCommitSHA(sha string) ([]Response, error) { commit, err := db.GetCommitBySHA(sha) if err != nil { return nil, err } - return db.GetResponsesForCommit(commit.ID) + return db.GetCommentsForCommit(commit.ID) } diff --git a/internal/storage/sync.go b/internal/storage/sync.go index 964f106..7a9e401 100644 --- a/internal/storage/sync.go +++ b/internal/storage/sync.go @@ -452,9 +452,9 @@ type SyncableResponse struct { CreatedAt time.Time } -// GetResponsesToSync returns responses created locally that need to be pushed. -// Only returns responses whose parent job has already been synced. -func (db *DB) GetResponsesToSync(machineID string, limit int) ([]SyncableResponse, error) { +// GetCommentsToSync returns comments created locally that need to be pushed. +// Only returns comments whose parent job has already been synced. +func (db *DB) GetCommentsToSync(machineID string, limit int) ([]SyncableResponse, error) { rows, err := db.Query(` SELECT r.id, r.uuid, r.job_id, j.uuid, @@ -497,15 +497,15 @@ func (db *DB) GetResponsesToSync(machineID string, limit int) ([]SyncableRespons return responses, rows.Err() } -// MarkResponseSynced updates the synced_at timestamp for a response -func (db *DB) MarkResponseSynced(responseID int64) error { +// MarkCommentSynced updates the synced_at timestamp for a comment +func (db *DB) MarkCommentSynced(responseID int64) error { now := time.Now().UTC().Format(time.RFC3339) _, err := db.Exec(`UPDATE responses SET synced_at = ? WHERE id = ?`, now, responseID) return err } -// MarkResponsesSynced updates the synced_at timestamp for multiple responses -func (db *DB) MarkResponsesSynced(responseIDs []int64) error { +// MarkCommentsSynced updates the synced_at timestamp for multiple comments +func (db *DB) MarkCommentsSynced(responseIDs []int64) error { if len(responseIDs) == 0 { return nil } diff --git a/internal/storage/sync_test.go b/internal/storage/sync_test.go index a9742f6..817205e 100644 --- a/internal/storage/sync_test.go +++ b/internal/storage/sync_test.go @@ -1476,7 +1476,7 @@ func TestGetReviewsToSync_TimestampComparison(t *testing.T) { }) } -func TestGetResponsesToSync_LegacyResponsesExcluded(t *testing.T) { +func TestGetCommentsToSync_LegacyCommentsExcluded(t *testing.T) { // This test verifies that legacy responses with job_id IS NULL (tied only to commit_id) // are excluded from sync since they cannot be synced via job_uuid. dbPath := filepath.Join(t.TempDir(), "test.db") @@ -1521,9 +1521,9 @@ func TestGetResponsesToSync_LegacyResponsesExcluded(t *testing.T) { t.Fatalf("MarkJobSynced failed: %v", err) } - jobResp, err := db.AddResponseToJob(job.ID, "human", "This is a job response") + jobResp, err := db.AddCommentToJob(job.ID, "human", "This is a job response") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Create a legacy commit-only response by directly inserting with job_id IS NULL @@ -1537,9 +1537,9 @@ func TestGetResponsesToSync_LegacyResponsesExcluded(t *testing.T) { legacyRespID, _ := result.LastInsertId() // Get responses to sync - should only include the job-based response - responses, err := db.GetResponsesToSync(machineID, 100) + responses, err := db.GetCommentsToSync(machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } foundJobResp := false @@ -1702,9 +1702,9 @@ func TestClearAllSyncedAt(t *testing.T) { job := h.createCompletedJob("clear-test-sha") // Add a response - _, err := h.db.AddResponseToJob(job.ID, "user", "test response") + _, err := h.db.AddCommentToJob(job.ID, "user", "test response") if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } // Mark everything as synced @@ -1756,9 +1756,9 @@ func TestClearAllSyncedAt(t *testing.T) { t.Errorf("Expected 1 review to sync after clear, got %d", len(reviews)) } - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 1 { t.Errorf("Expected 1 response to sync after clear, got %d", len(responses)) @@ -1774,9 +1774,9 @@ func TestBatchMarkSynced(t *testing.T) { for i := 0; i < 5; i++ { job := h.createCompletedJob(fmt.Sprintf("batch-test-sha-%d", i)) jobs = append(jobs, job) - _, err := h.db.AddResponseToJob(job.ID, "user", fmt.Sprintf("response %d", i)) + _, err := h.db.AddCommentToJob(job.ID, "user", fmt.Sprintf("response %d", i)) if err != nil { - t.Fatalf("AddResponseToJob failed: %v", err) + t.Fatalf("AddCommentToJob failed: %v", err) } } @@ -1837,9 +1837,9 @@ func TestBatchMarkSynced(t *testing.T) { t.Run("MarkResponsesSynced marks multiple responses", func(t *testing.T) { // Get responses for synced jobs - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 3 { t.Fatalf("Expected 3 responses to sync (jobs 0-2 synced), got %d", len(responses)) @@ -1850,14 +1850,14 @@ func TestBatchMarkSynced(t *testing.T) { for i, r := range responses { responseIDs[i] = r.ID } - if err := h.db.MarkResponsesSynced(responseIDs); err != nil { - t.Fatalf("MarkResponsesSynced failed: %v", err) + if err := h.db.MarkCommentsSynced(responseIDs); err != nil { + t.Fatalf("MarkCommentsSynced failed: %v", err) } // Verify no responses left to sync (for synced jobs) - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 0 { t.Errorf("Expected 0 responses to sync after batch mark, got %d", len(responses)) @@ -1872,7 +1872,7 @@ func TestBatchMarkSynced(t *testing.T) { if err := h.db.MarkReviewsSynced([]int64{}); err != nil { t.Errorf("MarkReviewsSynced with empty slice failed: %v", err) } - if err := h.db.MarkResponsesSynced([]int64{}); err != nil { + if err := h.db.MarkCommentsSynced([]int64{}); err != nil { t.Errorf("MarkResponsesSynced with empty slice failed: %v", err) } }) @@ -1960,24 +1960,24 @@ func TestGetReviewsToSync_RequiresJobSynced(t *testing.T) { } } -// TestGetResponsesToSync_RequiresJobSynced verifies that responses are only +// TestGetCommentsToSync_RequiresJobSynced verifies that responses are only // returned when their parent job has been synced (j.synced_at IS NOT NULL). -func TestGetResponsesToSync_RequiresJobSynced(t *testing.T) { +func TestGetCommentsToSync_RequiresJobSynced(t *testing.T) { h := newSyncTestHelper(t) // Create a completed job (not synced yet) job := h.createCompletedJob("response-sync-sha") // Add a response to the job - _, err := h.db.AddResponseToJob(job.ID, "test-user", "test response") + _, err := h.db.AddCommentToJob(job.ID, "test-user", "test response") if err != nil { t.Fatalf("Failed to add response: %v", err) } // Before job is synced, GetResponsesToSync should return nothing - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 0 { t.Errorf("Expected 0 responses before job is synced, got %d", len(responses)) @@ -1989,9 +1989,9 @@ func TestGetResponsesToSync_RequiresJobSynced(t *testing.T) { } // Now GetResponsesToSync should return the response - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 1 { t.Errorf("Expected 1 response after job is synced, got %d", len(responses)) @@ -2064,7 +2064,7 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { job := h.createCompletedJob("workflow-sha-" + string(rune('a'+i))) createdJobs = append(createdJobs, job) // Add a response - _, err := h.db.AddResponseToJob(job.ID, "user", "response") + _, err := h.db.AddCommentToJob(job.ID, "user", "response") if err != nil { t.Fatalf("Failed to add response %d: %v", i, err) } @@ -2087,9 +2087,9 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { t.Errorf("Expected 0 reviews to sync (jobs not synced), got %d", len(reviews)) } - responses, err := h.db.GetResponsesToSync(h.machineID, 100) + responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 0 { t.Errorf("Expected 0 responses to sync (jobs not synced), got %d", len(responses)) @@ -2117,9 +2117,9 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { t.Errorf("Expected 1 review to sync, got %d", len(reviews)) } - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 1 { t.Errorf("Expected 1 response to sync, got %d", len(responses)) @@ -2149,9 +2149,9 @@ func TestSyncOrder_FullWorkflow(t *testing.T) { t.Errorf("Expected 3 reviews to sync, got %d", len(reviews)) } - responses, err = h.db.GetResponsesToSync(h.machineID, 100) + responses, err = h.db.GetCommentsToSync(h.machineID, 100) if err != nil { - t.Fatalf("GetResponsesToSync failed: %v", err) + t.Fatalf("GetCommentsToSync failed: %v", err) } if len(responses) != 3 { t.Errorf("Expected 3 responses to sync, got %d", len(responses)) diff --git a/internal/storage/syncworker.go b/internal/storage/syncworker.go index c5e23e3..0b0acba 100644 --- a/internal/storage/syncworker.go +++ b/internal/storage/syncworker.go @@ -639,10 +639,10 @@ func (w *SyncWorker) pushChangesWithStats(ctx context.Context, pool *PgPool) (pu } } - // Push responses - batch operation - responses, err := w.db.GetResponsesToSync(machineID, syncBatchSize) + // Push comments - batch operation + responses, err := w.db.GetCommentsToSync(machineID, syncBatchSize) if err != nil { - return stats, fmt.Errorf("get responses to sync: %w", err) + return stats, fmt.Errorf("get comments to sync: %w", err) } if len(responses) > 0 { @@ -660,8 +660,8 @@ func (w *SyncWorker) pushChangesWithStats(ctx context.Context, pool *PgPool) (pu } } if len(syncedResponseIDs) > 0 { - if err := w.db.MarkResponsesSynced(syncedResponseIDs); err != nil { - log.Printf("Sync: failed to mark responses synced: %v", err) + if err := w.db.MarkCommentsSynced(syncedResponseIDs); err != nil { + log.Printf("Sync: failed to mark comments synced: %v", err) } } } From 6b875964a12475c68fd1a5464f46062d867417d7 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 12:31:57 -0500 Subject: [PATCH 06/23] Rename API endpoints from /api/respond to /api/comment - /api/respond -> /api/comment (POST: add comment) - /api/responses -> /api/comments (GET: list comments) - Keep backward compat aliases for /api/respond and /api/responses Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/client_test.go | 2 +- cmd/roborev/comment_test.go | 2 +- cmd/roborev/main.go | 4 ++-- cmd/roborev/main_test.go | 10 +++++----- cmd/roborev/tui.go | 10 +++++----- cmd/roborev/tui_test.go | 4 ++-- internal/daemon/client.go | 4 ++-- internal/daemon/client_test.go | 2 +- internal/daemon/server.go | 6 ++++-- 9 files changed, 23 insertions(+), 21 deletions(-) diff --git a/cmd/roborev/client_test.go b/cmd/roborev/client_test.go index 1f2e44b..abb442c 100644 --- a/cmd/roborev/client_test.go +++ b/cmd/roborev/client_test.go @@ -17,7 +17,7 @@ import ( func TestGetCommentsForJob(t *testing.T) { t.Run("returns responses for job", func(t *testing.T) { _, cleanup := setupMockDaemon(t, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/responses" || r.Method != "GET" { + if r.URL.Path != "/api/comments" || r.Method != "GET" { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusNotFound) return diff --git a/cmd/roborev/comment_test.go b/cmd/roborev/comment_test.go index b15b268..2a1f34f 100644 --- a/cmd/roborev/comment_test.go +++ b/cmd/roborev/comment_test.go @@ -20,7 +20,7 @@ func TestCommentJobFlag(t *testing.T) { json.NewEncoder(w).Encode(map[string]interface{}{"version": version.Version}) return } - if r.URL.Path == "/api/respond" && r.Method == "POST" { + if r.URL.Path == "/api/comment" && r.Method == "POST" { var req struct { JobID int64 `json:"job_id"` } diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 640ae9d..1f5bf6d 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1320,7 +1320,7 @@ Examples: reqBody, _ := json.Marshal(reqData) addr := getDaemonAddr() - resp, err := http.Post(addr+"/api/respond", "application/json", bytes.NewReader(reqBody)) + resp, err := http.Post(addr+"/api/comment", "application/json", bytes.NewReader(reqBody)) if err != nil { return fmt.Errorf("failed to connect to daemon: %w", err) } @@ -1575,7 +1575,7 @@ func getCommentsForJob(jobID int64) ([]storage.Response, error) { addr := getDaemonAddr() client := &http.Client{Timeout: 5 * time.Second} - resp, err := client.Get(fmt.Sprintf("%s/api/responses?job_id=%d", addr, jobID)) + resp, err := client.Get(fmt.Sprintf("%s/api/comments?job_id=%d", addr, jobID)) if err != nil { return nil, err } diff --git a/cmd/roborev/main_test.go b/cmd/roborev/main_test.go index 7b1e662..bca7105 100644 --- a/cmd/roborev/main_test.go +++ b/cmd/roborev/main_test.go @@ -350,7 +350,7 @@ func TestRunRefineSurfacesResponseErrors(t *testing.T) { json.NewEncoder(w).Encode(storage.Review{ ID: 1, JobID: 1, Output: "**Bug found**: fail", Addressed: false, }) - case r.URL.Path == "/api/responses": + case r.URL.Path == "/api/comments": w.WriteHeader(http.StatusInternalServerError) default: w.WriteHeader(http.StatusNotFound) @@ -634,7 +634,7 @@ func createMockRefineHandler(state *mockRefineState) http.Handler { } json.NewEncoder(w).Encode(reviewCopy) - case r.URL.Path == "/api/responses" && r.Method == "GET": + case r.URL.Path == "/api/comments" && r.Method == "GET": jobIDStr := r.URL.Query().Get("job_id") var jobID int64 fmt.Sscanf(jobIDStr, "%d", &jobID) @@ -648,7 +648,7 @@ func createMockRefineHandler(state *mockRefineState) http.Handler { "responses": responses, }) - case r.URL.Path == "/api/respond" && r.Method == "POST": + case r.URL.Path == "/api/comment" && r.Method == "POST": var req struct { JobID int64 `json:"job_id"` Responder string `json:"responder"` @@ -1068,7 +1068,7 @@ func TestRefineLoopStaysOnFailedFixChain(t *testing.T) { } json.NewEncoder(w).Encode(review) - case r.URL.Path == "/api/responses" && r.Method == http.MethodGet: + case r.URL.Path == "/api/comments" && r.Method == http.MethodGet: jobIDStr := r.URL.Query().Get("job_id") var jobID int64 fmt.Sscanf(jobIDStr, "%d", &jobID) @@ -1080,7 +1080,7 @@ func TestRefineLoopStaysOnFailedFixChain(t *testing.T) { "responses": responses, }) - case r.URL.Path == "/api/respond" && r.Method == http.MethodPost: + case r.URL.Path == "/api/comment" && r.Method == http.MethodPost: var req struct { JobID int64 `json:"job_id"` Responder string `json:"responder"` diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 39a3d0c..e8d6763 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -551,7 +551,7 @@ func (m tuiModel) fetchReview(jobID int64) tea.Cmd { // Fetch responses for this job var responses []storage.Response - respResp, err := m.client.Get(fmt.Sprintf("%s/api/responses?job_id=%d", m.serverAddr, jobID)) + respResp, err := m.client.Get(fmt.Sprintf("%s/api/comments?job_id=%d", m.serverAddr, jobID)) if err == nil { defer respResp.Body.Close() if respResp.StatusCode == http.StatusOK { @@ -566,7 +566,7 @@ func (m tuiModel) fetchReview(jobID int64) tea.Cmd { // Also fetch legacy responses by SHA for single commits (not ranges or dirty reviews) // and merge with job responses to preserve full history during migration if review.Job != nil && !strings.Contains(review.Job.GitRef, "..") && review.Job.GitRef != "dirty" { - shaResp, err := m.client.Get(fmt.Sprintf("%s/api/responses?sha=%s", m.serverAddr, review.Job.GitRef)) + shaResp, err := m.client.Get(fmt.Sprintf("%s/api/comments?sha=%s", m.serverAddr, review.Job.GitRef)) if err == nil { defer shaResp.Body.Close() if shaResp.StatusCode == http.StatusOK { @@ -3101,17 +3101,17 @@ func (m tuiModel) submitComment(jobID int64, text string) tea.Cmd { } resp, err := m.client.Post( - fmt.Sprintf("%s/api/respond", m.serverAddr), + fmt.Sprintf("%s/api/comment", m.serverAddr), "application/json", bytes.NewReader(body), ) if err != nil { - return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit response: %w", err)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit comment: %w", err)} } defer resp.Body.Close() if resp.StatusCode != http.StatusCreated { - return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit response: HTTP %d", resp.StatusCode)} + return tuiCommentResultMsg{jobID: jobID, err: fmt.Errorf("submit comment: HTTP %d", resp.StatusCode)} } return tuiCommentResultMsg{jobID: jobID, err: nil} diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 5e28bc9..ef0fbc4 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -4289,7 +4289,7 @@ func TestTUIFetchReviewFallbackSHAResponses(t *testing.T) { return } - if r.URL.Path == "/api/responses" { + if r.URL.Path == "/api/comments" { jobID := r.URL.Query().Get("job_id") sha := r.URL.Query().Get("sha") @@ -4375,7 +4375,7 @@ func TestTUIFetchReviewNoFallbackForRangeReview(t *testing.T) { return } - if r.URL.Path == "/api/responses" { + if r.URL.Path == "/api/comments" { // Return empty responses for job_id json.NewEncoder(w).Encode(map[string]interface{}{ "responses": []storage.Response{}, diff --git a/internal/daemon/client.go b/internal/daemon/client.go index 8c756f9..1fe5840 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -157,7 +157,7 @@ func (c *HTTPClient) AddComment(jobID int64, responder, response string) error { "response": response, }) - resp, err := c.httpClient.Post(c.addr+"/api/respond", "application/json", bytes.NewReader(reqBody)) + resp, err := c.httpClient.Post(c.addr+"/api/comment", "application/json", bytes.NewReader(reqBody)) if err != nil { return err } @@ -374,7 +374,7 @@ func (c *HTTPClient) FindPendingJobForRef(repoPath, gitRef string) (*storage.Rev } func (c *HTTPClient) GetCommentsForJob(jobID int64) ([]storage.Response, error) { - resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/responses?job_id=%d", c.addr, jobID)) + resp, err := c.httpClient.Get(fmt.Sprintf("%s/api/comments?job_id=%d", c.addr, jobID)) if err != nil { return nil, err } diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index 0973119..539fb24 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -19,7 +19,7 @@ func TestHTTPClientAddComment(t *testing.T) { var received map[string]interface{} server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - if r.URL.Path != "/api/respond" || r.Method != http.MethodPost { + if r.URL.Path != "/api/comment" || r.Method != http.MethodPost { t.Errorf("unexpected request: %s %s", r.Method, r.URL.Path) w.WriteHeader(http.StatusNotFound) return diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 4a567b9..00a816c 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -69,8 +69,10 @@ func NewServer(db *storage.DB, cfg *config.Config, configPath string) *Server { mux.HandleFunc("/api/repos", s.handleListRepos) mux.HandleFunc("/api/review", s.handleGetReview) mux.HandleFunc("/api/review/address", s.handleAddressReview) - mux.HandleFunc("/api/respond", s.handleAddComment) - mux.HandleFunc("/api/responses", s.handleListComments) + mux.HandleFunc("/api/comment", s.handleAddComment) + mux.HandleFunc("/api/comments", s.handleListComments) + mux.HandleFunc("/api/respond", s.handleAddComment) // backward compat + mux.HandleFunc("/api/responses", s.handleListComments) // backward compat mux.HandleFunc("/api/status", s.handleStatus) mux.HandleFunc("/api/stream/events", s.handleStreamEvents) mux.HandleFunc("/api/sync/now", s.handleSyncNow) From 4912673e3577ed84167b9e750d0dd94ccc4a15f0 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 12:39:47 -0500 Subject: [PATCH 07/23] Rename API fields from responder/response to commenter/comment - Remove backward compat /api/respond and /api/responses routes - Rename API JSON keys: responder -> commenter, response -> comment - Rename related variables and parameters throughout Only the database schema (responses table with responder/response columns) retains the old terminology. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/main.go | 16 ++++++++-------- cmd/roborev/refine_test.go | 12 ++++++------ cmd/roborev/tui.go | 10 +++++----- internal/daemon/client.go | 8 ++++---- internal/daemon/client_test.go | 8 ++++---- internal/daemon/server.go | 14 ++++++-------- 6 files changed, 33 insertions(+), 35 deletions(-) diff --git a/cmd/roborev/main.go b/cmd/roborev/main.go index 1f5bf6d..e84ad3e 100644 --- a/cmd/roborev/main.go +++ b/cmd/roborev/main.go @@ -1202,7 +1202,7 @@ Examples: func commentCmd() *cobra.Command { var ( - responder string + commenter string message string forceJobID bool ) @@ -1299,17 +1299,17 @@ Examples: return fmt.Errorf("empty comment, aborting") } - if responder == "" { - responder = os.Getenv("USER") - if responder == "" { - responder = "anonymous" + if commenter == "" { + commenter = os.Getenv("USER") + if commenter == "" { + commenter = "anonymous" } } // Build request with either job_id or sha reqData := map[string]interface{}{ - "responder": responder, - "response": message, + "commenter": commenter, + "comment": message, } if jobID != 0 { reqData["job_id"] = jobID @@ -1336,7 +1336,7 @@ Examples: }, } - cmd.Flags().StringVar(&responder, "commenter", "", "commenter name (default: $USER)") + cmd.Flags().StringVar(&commenter, "commenter", "", "commenter name (default: $USER)") cmd.Flags().StringVarP(&message, "message", "m", "", "comment message (opens editor if not provided)") cmd.Flags().BoolVar(&forceJobID, "job", false, "force argument to be treated as job ID (not SHA)") diff --git a/cmd/roborev/refine_test.go b/cmd/roborev/refine_test.go index b405b40..4975c59 100644 --- a/cmd/roborev/refine_test.go +++ b/cmd/roborev/refine_test.go @@ -23,7 +23,7 @@ type mockDaemonClient struct { // Track calls for assertions addressedReviews []int64 - addedResponses []addedResponse + addedComments []addedComment enqueuedReviews []enqueuedReview // Configurable errors for testing error paths @@ -31,10 +31,10 @@ type mockDaemonClient struct { getReviewBySHAErr error } -type addedResponse struct { +type addedComment struct { JobID int64 - Responder string - Response string + Commenter string + Comment string } type enqueuedReview struct { @@ -78,8 +78,8 @@ func (m *mockDaemonClient) MarkReviewAddressed(reviewID int64) error { return nil } -func (m *mockDaemonClient) AddComment(jobID int64, responder, response string) error { - m.addedResponses = append(m.addedResponses, addedResponse{jobID, responder, response}) +func (m *mockDaemonClient) AddComment(jobID int64, commenter, comment string) error { + m.addedComments = append(m.addedComments, addedComment{jobID, commenter, comment}) return nil } diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index e8d6763..d6435f0 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -3084,15 +3084,15 @@ func (m tuiModel) renderRespondView() string { func (m tuiModel) submitComment(jobID int64, text string) tea.Cmd { return func() tea.Msg { - responder := os.Getenv("USER") - if responder == "" { - responder = "anonymous" + commenter := os.Getenv("USER") + if commenter == "" { + commenter = "anonymous" } payload := map[string]interface{}{ "job_id": jobID, - "responder": responder, - "response": strings.TrimSpace(text), + "commenter": commenter, + "comment": strings.TrimSpace(text), } body, err := json.Marshal(payload) diff --git a/internal/daemon/client.go b/internal/daemon/client.go index 1fe5840..8a93239 100644 --- a/internal/daemon/client.go +++ b/internal/daemon/client.go @@ -27,7 +27,7 @@ type Client interface { MarkReviewAddressed(reviewID int64) error // AddComment adds a comment to a job - AddComment(jobID int64, responder, response string) error + AddComment(jobID int64, commenter, comment string) error // EnqueueReview enqueues a review job and returns the job ID EnqueueReview(repoPath, gitRef, agentName string) (int64, error) @@ -150,11 +150,11 @@ func (c *HTTPClient) MarkReviewAddressed(reviewID int64) error { return nil } -func (c *HTTPClient) AddComment(jobID int64, responder, response string) error { +func (c *HTTPClient) AddComment(jobID int64, commenter, comment string) error { reqBody, _ := json.Marshal(map[string]interface{}{ "job_id": jobID, - "responder": responder, - "response": response, + "commenter": commenter, + "comment": comment, }) resp, err := c.httpClient.Post(c.addr+"/api/comment", "application/json", bytes.NewReader(reqBody)) diff --git a/internal/daemon/client_test.go b/internal/daemon/client_test.go index 539fb24..fafbc8c 100644 --- a/internal/daemon/client_test.go +++ b/internal/daemon/client_test.go @@ -39,11 +39,11 @@ func TestHTTPClientAddComment(t *testing.T) { if received["job_id"].(float64) != 42 { t.Errorf("expected job_id 42, got %v", received["job_id"]) } - if received["responder"] != "test-agent" { - t.Errorf("expected responder test-agent, got %v", received["responder"]) + if received["commenter"] != "test-agent" { + t.Errorf("expected commenter test-agent, got %v", received["commenter"]) } - if received["response"] != "Fixed the issue" { - t.Errorf("expected response to match, got %v", received["response"]) + if received["comment"] != "Fixed the issue" { + t.Errorf("expected comment to match, got %v", received["comment"]) } } diff --git a/internal/daemon/server.go b/internal/daemon/server.go index 00a816c..5d1db97 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -71,8 +71,6 @@ func NewServer(db *storage.DB, cfg *config.Config, configPath string) *Server { mux.HandleFunc("/api/review/address", s.handleAddressReview) mux.HandleFunc("/api/comment", s.handleAddComment) mux.HandleFunc("/api/comments", s.handleListComments) - mux.HandleFunc("/api/respond", s.handleAddComment) // backward compat - mux.HandleFunc("/api/responses", s.handleListComments) // backward compat mux.HandleFunc("/api/status", s.handleStatus) mux.HandleFunc("/api/stream/events", s.handleStreamEvents) mux.HandleFunc("/api/sync/now", s.handleSyncNow) @@ -712,8 +710,8 @@ func (s *Server) handleGetReview(w http.ResponseWriter, r *http.Request) { type AddCommentRequest struct { SHA string `json:"sha,omitempty"` // Legacy: link to commit by SHA JobID int64 `json:"job_id,omitempty"` // Preferred: link to job - Responder string `json:"responder"` - Response string `json:"response"` + Commenter string `json:"commenter"` + Comment string `json:"comment"` } func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request) { @@ -728,8 +726,8 @@ func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request) { return } - if req.Responder == "" || req.Response == "" { - writeError(w, http.StatusBadRequest, "responder and response are required") + if req.Commenter == "" || req.Comment == "" { + writeError(w, http.StatusBadRequest, "commenter and comment are required") return } @@ -744,7 +742,7 @@ func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request) { if req.JobID != 0 { // Link to job (preferred method) - resp, err = s.db.AddCommentToJob(req.JobID, req.Responder, req.Response) + resp, err = s.db.AddCommentToJob(req.JobID, req.Commenter, req.Comment) if err != nil { if errors.Is(err, sql.ErrNoRows) { writeError(w, http.StatusNotFound, "job not found") @@ -761,7 +759,7 @@ func (s *Server) handleAddComment(w http.ResponseWriter, r *http.Request) { return } - resp, err = s.db.AddComment(commit.ID, req.Responder, req.Response) + resp, err = s.db.AddComment(commit.ID, req.Commenter, req.Comment) if err != nil { writeError(w, http.StatusInternalServerError, fmt.Sprintf("add comment: %v", err)) return From b2310885ec47de0973c80b03d04b6ad3bf429e3c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 12:52:52 -0500 Subject: [PATCH 08/23] Fix commit message view navigation and sanitize display content - Fix escape handler in commit message view to return to the originating view (queue or review) instead of always returning to queue - Add sanitizeForDisplay() to strip ANSI escape sequences and control characters from commit messages before display, preventing terminal injection when viewing untrusted repos - Add tests for commit message view navigation from both queue and review - Add tests for help modal toggle/back behavior - Add comprehensive tests for sanitization function Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 38 +++++++-- cmd/roborev/tui_test.go | 176 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 206 insertions(+), 8 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index d6435f0..77370ff 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -65,6 +65,25 @@ var ( // fullSHAPattern matches a 40-character hex git SHA (not ranges or branch names) var fullSHAPattern = regexp.MustCompile(`(?i)^[0-9a-f]{40}$`) +// ansiEscapePattern matches ANSI escape sequences (colors, cursor movement, etc.) +var ansiEscapePattern = regexp.MustCompile(`\x1b\[[0-9;?]*[a-zA-Z]|\x1b\][^\x07]*\x07`) + +// sanitizeForDisplay strips ANSI escape sequences and control characters from text +// to prevent terminal injection when displaying untrusted content (e.g., commit messages). +func sanitizeForDisplay(s string) string { + // Strip ANSI escape sequences + s = ansiEscapePattern.ReplaceAllString(s, "") + // Strip control characters except newline (\n) and tab (\t) + var result strings.Builder + result.Grow(len(s)) + for _, r := range s { + if r == '\n' || r == '\t' || !unicode.IsControl(r) { + result.WriteRune(r) + } + } + return result.String() +} + type tuiView int const ( @@ -151,9 +170,10 @@ type tuiModel struct { reconnecting bool // True if currently attempting reconnection // Commit message view state - commitMsgContent string // Formatted commit message(s) content - commitMsgScroll int // Scroll position in commit message view - commitMsgJobID int64 // Job ID for the commit message being viewed + commitMsgContent string // Formatted commit message(s) content + commitMsgScroll int // Scroll position in commit message view + commitMsgJobID int64 // Job ID for the commit message being viewed + commitMsgFromView tuiView // View to return to after closing commit message view // Help view state helpFromView tuiView // View to return to after closing help @@ -770,7 +790,7 @@ func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { content.WriteString("\n") } - return tuiCommitMsgMsg{jobID: jobID, content: content.String()} + return tuiCommitMsgMsg{jobID: jobID, content: sanitizeForDisplay(content.String())} } // Single commit @@ -789,7 +809,7 @@ func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { content.WriteString("\n" + info.Body + "\n") } - return tuiCommitMsgMsg{jobID: jobID, content: content.String()} + return tuiCommitMsgMsg{jobID: jobID, content: sanitizeForDisplay(content.String())} } } @@ -1349,7 +1369,7 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { return m, nil } if m.currentView == tuiViewCommitMsg { - m.currentView = tuiViewQueue + m.currentView = m.commitMsgFromView m.commitMsgContent = "" m.commitMsgScroll = 0 return m, nil @@ -1770,12 +1790,14 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // Show commit message(s) for the selected job if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { job := m.jobs[m.selectedIdx] + m.commitMsgFromView = m.currentView m.commitMsgJobID = job.ID m.commitMsgContent = "" m.commitMsgScroll = 0 return m, m.fetchCommitMsg(&job) } else if m.currentView == tuiViewReview && m.currentReview != nil && m.currentReview.Job != nil { job := m.currentReview.Job + m.commitMsgFromView = m.currentView m.commitMsgJobID = job.ID m.commitMsgContent = "" m.commitMsgScroll = 0 @@ -1847,8 +1869,8 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { m.promptScroll = 0 } } else if m.currentView == tuiViewCommitMsg { - // Go back to queue view - m.currentView = tuiViewQueue + // Go back to originating view + m.currentView = m.commitMsgFromView m.commitMsgContent = "" m.commitMsgScroll = 0 } else if m.currentView == tuiViewHelp { diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index ef0fbc4..1f69d85 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -6285,3 +6285,179 @@ func TestTUIReconnectOnConsecutiveErrors(t *testing.T) { } }) } + +func TestTUICommitMsgViewNavigationFromQueue(t *testing.T) { + // Test that pressing escape in commit message view returns to the originating view (queue) + m := newTuiModel("http://localhost") + m.jobs = []storage.ReviewJob{{ID: 1, GitRef: "abc123", Status: storage.JobStatusDone}} + m.selectedIdx = 0 + m.selectedJobID = 1 + m.currentView = tuiViewQueue + m.commitMsgJobID = 1 // Set to match incoming message (normally set by 'm' key handler) + m.commitMsgFromView = tuiViewQueue // Track where we came from + + // Simulate receiving commit message content (sets view to CommitMsg) + updated, _ := m.Update(tuiCommitMsgMsg{jobID: 1, content: "test message"}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewCommitMsg { + t.Errorf("Expected tuiViewCommitMsg, got %d", m2.currentView) + } + + // Press escape to go back + updated, _ = m2.Update(tea.KeyMsg{Type: tea.KeyEscape}) + m3 := updated.(tuiModel) + + if m3.currentView != tuiViewQueue { + t.Errorf("Expected to return to tuiViewQueue, got %d", m3.currentView) + } + if m3.commitMsgContent != "" { + t.Error("Expected commitMsgContent to be cleared") + } +} + +func TestTUICommitMsgViewNavigationFromReview(t *testing.T) { + // Test that pressing escape in commit message view returns to the originating view (review) + m := newTuiModel("http://localhost") + job := &storage.ReviewJob{ID: 1, GitRef: "abc123", Status: storage.JobStatusDone} + m.jobs = []storage.ReviewJob{*job} + m.currentReview = &storage.Review{ID: 1, JobID: 1, Job: job} + m.currentView = tuiViewReview + m.commitMsgFromView = tuiViewReview + m.commitMsgContent = "test message" + m.currentView = tuiViewCommitMsg + + // Press escape to go back + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyEscape}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewReview { + t.Errorf("Expected to return to tuiViewReview, got %d", m2.currentView) + } +} + +func TestTUICommitMsgViewNavigationWithQ(t *testing.T) { + // Test that pressing 'q' in commit message view also returns to originating view + m := newTuiModel("http://localhost") + m.currentView = tuiViewCommitMsg + m.commitMsgFromView = tuiViewReview + m.commitMsgContent = "test message" + + // Press 'q' to go back + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'q'}}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewReview { + t.Errorf("Expected to return to tuiViewReview after 'q', got %d", m2.currentView) + } +} + +func TestTUIHelpViewToggleFromQueue(t *testing.T) { + // Test that '?' opens help from queue and pressing '?' again returns to queue + m := newTuiModel("http://localhost") + m.currentView = tuiViewQueue + + // Press '?' to open help + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewHelp { + t.Errorf("Expected tuiViewHelp, got %d", m2.currentView) + } + if m2.helpFromView != tuiViewQueue { + t.Errorf("Expected helpFromView to be tuiViewQueue, got %d", m2.helpFromView) + } + + // Press '?' again to close help + updated, _ = m2.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m3 := updated.(tuiModel) + + if m3.currentView != tuiViewQueue { + t.Errorf("Expected to return to tuiViewQueue, got %d", m3.currentView) + } +} + +func TestTUIHelpViewToggleFromReview(t *testing.T) { + // Test that '?' opens help from review and escape returns to review + m := newTuiModel("http://localhost") + job := &storage.ReviewJob{ID: 1, GitRef: "abc123", Status: storage.JobStatusDone} + m.currentReview = &storage.Review{ID: 1, JobID: 1, Job: job} + m.currentView = tuiViewReview + + // Press '?' to open help + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'?'}}) + m2 := updated.(tuiModel) + + if m2.currentView != tuiViewHelp { + t.Errorf("Expected tuiViewHelp, got %d", m2.currentView) + } + if m2.helpFromView != tuiViewReview { + t.Errorf("Expected helpFromView to be tuiViewReview, got %d", m2.helpFromView) + } + + // Press escape to close help + updated, _ = m2.Update(tea.KeyMsg{Type: tea.KeyEscape}) + m3 := updated.(tuiModel) + + if m3.currentView != tuiViewReview { + t.Errorf("Expected to return to tuiViewReview, got %d", m3.currentView) + } +} + +func TestSanitizeForDisplay(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "plain text unchanged", + input: "Hello, world!", + expected: "Hello, world!", + }, + { + name: "preserves newlines and tabs", + input: "Line1\n\tIndented", + expected: "Line1\n\tIndented", + }, + { + name: "strips ANSI color codes", + input: "\x1b[31mred text\x1b[0m", + expected: "red text", + }, + { + name: "strips cursor movement", + input: "\x1b[2Jhello\x1b[H", + expected: "hello", + }, + { + name: "strips OSC sequences (title set)", + input: "\x1b]0;Evil Title\x07normal text", + expected: "normal text", + }, + { + name: "strips control characters", + input: "hello\x00world\x07\x08test", + expected: "helloworldtest", + }, + { + name: "handles complex escape sequence", + input: "\x1b[1;32mBold Green\x1b[0m and \x1b[4munderline\x1b[24m", + expected: "Bold Green and underline", + }, + { + name: "empty string unchanged", + input: "", + expected: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := sanitizeForDisplay(tt.input) + if got != tt.expected { + t.Errorf("sanitizeForDisplay(%q) = %q, want %q", tt.input, got, tt.expected) + } + }) + } +} From d991fa8a8dddbe4c66ad59e195aae49178cc053f Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 13:05:18 -0500 Subject: [PATCH 09/23] Fix GetSystemPrompt to return empty for run without template Previously, GetSystemPrompt("agent", "run") fell through to the default case and returned SystemPromptSingle (review instructions) for agents without a run template. This incorrectly prepended review-style output instructions to roborev run jobs for non-Gemini agents. Now returns empty string for "run" when no template exists, so raw prompts are used without preamble. Gemini still gets its run template. Co-Authored-By: Claude Opus 4.5 --- internal/prompt/templates.go | 3 +++ internal/prompt/templates_test.go | 13 +++++++++++++ 2 files changed, 16 insertions(+) diff --git a/internal/prompt/templates.go b/internal/prompt/templates.go index d170a23..7d1c049 100644 --- a/internal/prompt/templates.go +++ b/internal/prompt/templates.go @@ -43,6 +43,9 @@ func GetSystemPrompt(agentName string, promptType string) string { return SystemPromptRange case "address": return SystemPromptAddress + case "run": + // No default run preamble - return empty so raw prompts are used + return "" default: return SystemPromptSingle } diff --git a/internal/prompt/templates_test.go b/internal/prompt/templates_test.go index 39e078b..7675ed6 100644 --- a/internal/prompt/templates_test.go +++ b/internal/prompt/templates_test.go @@ -38,3 +38,16 @@ func TestGeminiRunTemplateLoads(t *testing.T) { t.Errorf("Expected Gemini run template content, got: %s", result[:min(100, len(result))]) } } + +func TestNonGeminiRunReturnsEmpty(t *testing.T) { + // Non-Gemini agents without a run template should return empty string, + // NOT the review prompt. This ensures roborev run uses raw prompts + // without review-style preambles for agents that don't have one. + agents := []string{"claude-code", "claude", "unknown-agent"} + for _, agent := range agents { + result := GetSystemPrompt(agent, "run") + if result != "" { + t.Errorf("GetSystemPrompt(%q, \"run\") = %q, want empty string", agent, result[:min(50, len(result))]) + } + } +} From 399b5b2e5eaa7837da2df67877f554fc59af65ec Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 13:51:38 -0500 Subject: [PATCH 10/23] Fix gitDescribePattern to match -dirty suffix from git describe The pattern only matched versions ending in -N-gHASH, so dirty dev builds like v0.16.1-2-g75d300a-dirty were incorrectly treated as release builds. Now allows optional -dirty suffix. Co-Authored-By: Claude Opus 4.5 --- internal/update/update.go | 6 +++--- internal/update/update_test.go | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/internal/update/update.go b/internal/update/update.go index 1459725..b13bf06 100644 --- a/internal/update/update.go +++ b/internal/update/update.go @@ -561,9 +561,9 @@ func extractBaseSemver(v string) string { return v } -// gitDescribePattern matches git describe format: v0.16.1-2-gabcdef -// The -N-gHASH suffix indicates N commits after the tag -var gitDescribePattern = regexp.MustCompile(`-\d+-g[0-9a-f]+$`) +// gitDescribePattern matches git describe format: v0.16.1-2-gabcdef or v0.16.1-2-gabcdef-dirty +// The -N-gHASH suffix indicates N commits after the tag, with optional -dirty suffix +var gitDescribePattern = regexp.MustCompile(`-\d+-g[0-9a-f]+(-dirty)?$`) // isDevBuildVersion returns true if the version is a dev build. // Dev builds are either: diff --git a/internal/update/update_test.go b/internal/update/update_test.go index abff1a3..b0ff22f 100644 --- a/internal/update/update_test.go +++ b/internal/update/update_test.go @@ -274,6 +274,11 @@ func TestIsDevBuildVersion(t *testing.T) { {"0.4.0-5-gabcdef", true}, {"1.2.3-100-gdeadbeef", true}, + // Git describe with -dirty suffix - ARE dev builds + {"0.16.1-2-g75d300a-dirty", true}, + {"v0.16.1-2-g75d300a-dirty", true}, + {"0.4.0-5-gabcdef-dirty", true}, + // Pure hash/dev - ARE dev builds {"dev", true}, {"abc1234", true}, From 683c004d2bbdc25912c9dfd7b18adad63653a793 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:03:00 -0500 Subject: [PATCH 11/23] Fix dirty job detection, OSC escape handling, and test messages - Handle job.GitRef == "dirty" and empty refs in fetchCommitMsg since job lists may not populate DiffContent field - Fix ansiEscapePattern to match OSC sequences terminated by ST (\x1b\\) in addition to BEL (\x07) - Fix copy-paste errors in sync_test.go test names and error messages Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 6 ++++-- cmd/roborev/tui_test.go | 7 ++++++- internal/storage/sync_test.go | 4 ++-- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 77370ff..eb82d62 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -66,7 +66,8 @@ var ( var fullSHAPattern = regexp.MustCompile(`(?i)^[0-9a-f]{40}$`) // ansiEscapePattern matches ANSI escape sequences (colors, cursor movement, etc.) -var ansiEscapePattern = regexp.MustCompile(`\x1b\[[0-9;?]*[a-zA-Z]|\x1b\][^\x07]*\x07`) +// Handles CSI sequences (\x1b[...X) and OSC sequences terminated by BEL (\x07) or ST (\x1b\\) +var ansiEscapePattern = regexp.MustCompile(`\x1b\[[0-9;?]*[a-zA-Z]|\x1b\]([^\x07\x1b]|\x1b[^\\])*(\x07|\x1b\\)`) // sanitizeForDisplay strips ANSI escape sequences and control characters from text // to prevent terminal injection when displaying untrusted content (e.g., commit messages). @@ -738,7 +739,8 @@ func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { jobID := job.ID return func() tea.Msg { // Handle dirty reviews (uncommitted changes) - if job.DiffContent != nil { + // Check both DiffContent and GitRef since job lists may not populate DiffContent + if job.DiffContent != nil || job.GitRef == "dirty" || job.GitRef == "" { return tuiCommitMsgMsg{ jobID: jobID, err: fmt.Errorf("no commit message for uncommitted changes"), diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 1f69d85..10465df 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -6431,10 +6431,15 @@ func TestSanitizeForDisplay(t *testing.T) { expected: "hello", }, { - name: "strips OSC sequences (title set)", + name: "strips OSC sequences (title set with BEL)", input: "\x1b]0;Evil Title\x07normal text", expected: "normal text", }, + { + name: "strips OSC sequences (title set with ST)", + input: "\x1b]0;Evil Title\x1b\\normal text", + expected: "normal text", + }, { name: "strips control characters", input: "hello\x00world\x07\x08test", diff --git a/internal/storage/sync_test.go b/internal/storage/sync_test.go index 817205e..03bf5e3 100644 --- a/internal/storage/sync_test.go +++ b/internal/storage/sync_test.go @@ -1835,7 +1835,7 @@ func TestBatchMarkSynced(t *testing.T) { } }) - t.Run("MarkResponsesSynced marks multiple responses", func(t *testing.T) { + t.Run("MarkCommentsSynced marks multiple comments", func(t *testing.T) { // Get responses for synced jobs responses, err := h.db.GetCommentsToSync(h.machineID, 100) if err != nil { @@ -1873,7 +1873,7 @@ func TestBatchMarkSynced(t *testing.T) { t.Errorf("MarkReviewsSynced with empty slice failed: %v", err) } if err := h.db.MarkCommentsSynced([]int64{}); err != nil { - t.Errorf("MarkResponsesSynced with empty slice failed: %v", err) + t.Errorf("MarkCommentsSynced with empty slice failed: %v", err) } }) } From 054036a436e128d658c807768a6b07510e23bc5f Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:08:40 -0500 Subject: [PATCH 12/23] Fix run job detection in fetchCommitMsg Check job.GitRef == "prompt" instead of job.Prompt != "" to identify run tasks. The Prompt field is populated for all jobs (storing the review prompt used), not just run tasks. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index eb82d62..55a210f 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -747,8 +747,8 @@ func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { } } - // Handle prompt/run jobs - if job.Prompt != "" { + // Handle prompt/run jobs (GitRef == "prompt" indicates a run task, not a commit review) + if job.GitRef == "prompt" { return tuiCommitMsgMsg{ jobID: jobID, err: fmt.Errorf("no commit message for run tasks"), From a8181083f8c4e5adf6f5755a8be89ec9a57ee389 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:11:37 -0500 Subject: [PATCH 13/23] Add tests for fetchCommitMsg job type detection Tests verify that: - Regular commits with Prompt populated are NOT detected as run tasks - Run tasks (GitRef == "prompt") return appropriate error - Dirty jobs (GitRef == "dirty" or DiffContent set) return appropriate error - Empty GitRef returns appropriate error The first case would have caught the bug where job.Prompt != "" was incorrectly used instead of job.GitRef == "prompt". Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui_test.go | 91 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 91 insertions(+) diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 10465df..a1fd840 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -6352,6 +6352,97 @@ func TestTUICommitMsgViewNavigationWithQ(t *testing.T) { } } +func TestFetchCommitMsgJobTypeDetection(t *testing.T) { + // Test that fetchCommitMsg correctly identifies job types and returns appropriate errors + // This is critical: Prompt field is populated for ALL jobs (stores review prompt), + // so we must check GitRef == "prompt" to identify run tasks, not Prompt != "" + + m := newTuiModel("http://localhost") + + tests := []struct { + name string + job storage.ReviewJob + expectError string // empty means no early error (will try git lookup) + }{ + { + name: "regular commit with Prompt populated should not error early", + job: storage.ReviewJob{ + ID: 1, + GitRef: "abc123def456", // valid commit SHA + Prompt: "You are a code reviewer...", // review prompt is stored for all jobs + }, + expectError: "", // should attempt git lookup, not return "run tasks" error + }, + { + name: "run task (GitRef=prompt) should error", + job: storage.ReviewJob{ + ID: 2, + GitRef: "prompt", + Prompt: "Explain this codebase", + }, + expectError: "no commit message for run tasks", + }, + { + name: "dirty job (GitRef=dirty) should error", + job: storage.ReviewJob{ + ID: 3, + GitRef: "dirty", + }, + expectError: "no commit message for uncommitted changes", + }, + { + name: "dirty job with DiffContent should error", + job: storage.ReviewJob{ + ID: 4, + GitRef: "some-ref", + DiffContent: func() *string { s := "diff content"; return &s }(), + }, + expectError: "no commit message for uncommitted changes", + }, + { + name: "empty GitRef should error", + job: storage.ReviewJob{ + ID: 5, + GitRef: "", + }, + expectError: "no commit message for uncommitted changes", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmd := m.fetchCommitMsg(&tt.job) + msg := cmd() + + result, ok := msg.(tuiCommitMsgMsg) + if !ok { + t.Fatalf("Expected tuiCommitMsgMsg, got %T", msg) + } + + if tt.expectError != "" { + if result.err == nil { + t.Errorf("Expected error %q, got nil", tt.expectError) + } else if result.err.Error() != tt.expectError { + t.Errorf("Expected error %q, got %q", tt.expectError, result.err.Error()) + } + } else { + // For valid commits, we expect a git error (repo doesn't exist in test) + // but NOT the "run tasks" or "uncommitted changes" error + if result.err != nil { + errMsg := result.err.Error() + if errMsg == "no commit message for run tasks" { + t.Errorf("Regular commit with Prompt should not be detected as run task") + } + if errMsg == "no commit message for uncommitted changes" { + t.Errorf("Regular commit should not be detected as uncommitted changes") + } + // Other errors (like git errors) are expected in test environment + } + } + }) + } +} + func TestTUIHelpViewToggleFromQueue(t *testing.T) { // Test that '?' opens help from queue and pressing '?' again returns to queue m := newTuiModel("http://localhost") From f76088c654448b0fe7fc6d6f41ff50856f7fb5f3 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:14:17 -0500 Subject: [PATCH 14/23] Remove separator lines from commit message view The 60-char separator lines wrapped awkwardly on narrow terminals. Use blank lines instead for cleaner formatting at any width. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 55a210f..be6d1c8 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -771,8 +771,7 @@ func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { // Fetch info for each commit var content strings.Builder - content.WriteString(fmt.Sprintf("Commits in %s (%d commits):\n", job.GitRef, len(commits))) - content.WriteString(strings.Repeat("─", 60) + "\n\n") + content.WriteString(fmt.Sprintf("Commits in %s (%d commits):\n\n", job.GitRef, len(commits))) for i, sha := range commits { info, err := git.GetCommitInfo(job.RepoPath, sha) @@ -804,8 +803,7 @@ func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { var content strings.Builder content.WriteString(fmt.Sprintf("Commit: %s\n", info.SHA)) content.WriteString(fmt.Sprintf("Author: %s\n", info.Author)) - content.WriteString(fmt.Sprintf("Date: %s\n", info.Timestamp.Format("2006-01-02 15:04:05 -0700"))) - content.WriteString(strings.Repeat("─", 60) + "\n\n") + content.WriteString(fmt.Sprintf("Date: %s\n\n", info.Timestamp.Format("2006-01-02 15:04:05 -0700"))) content.WriteString(info.Subject + "\n") if info.Body != "" { content.WriteString("\n" + info.Body + "\n") From 6fc66d91500ba0c3d7e83217ba213915936c5053 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:15:05 -0500 Subject: [PATCH 15/23] Document left/right arrow keys in help modal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add ←/→ for previous/next review navigation in Review View section. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index be6d1c8..1c51076 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -3256,6 +3256,7 @@ func (m tuiModel) renderHelpView() string { keys: []struct{ key, desc string }{ {"p", "View prompt"}, {"↑/↓", "Scroll content"}, + {"←/→", "Previous / next review"}, }, }, } From 846d033a2f4afe5d6bf60845ffd3d3ba93791a69 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:17:49 -0500 Subject: [PATCH 16/23] Add Page Up/Down to help modal Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 1 + 1 file changed, 1 insertion(+) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 1c51076..3f2b3e6 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -3228,6 +3228,7 @@ func (m tuiModel) renderHelpView() string { keys: []struct{ key, desc string }{ {"↑/k", "Move up / previous review"}, {"↓/j", "Move down / next review"}, + {"PgUp/PgDn", "Scroll by page"}, {"enter", "View review details"}, {"esc", "Go back / clear filter"}, {"q", "Quit"}, From face0b8680b528a52d411ac6b0f3ea9059d7a2e5 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:19:53 -0500 Subject: [PATCH 17/23] Order job list by job ID instead of enqueue time Job IDs provide more predictable ordering since they're monotonically increasing. Previously ordered by enqueued_at which could result in IDs appearing out of order if jobs were enqueued at similar times. Co-Authored-By: Claude Opus 4.5 --- internal/storage/jobs.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index 228f488..f13ebbf 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -940,7 +940,7 @@ func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int query += " WHERE " + strings.Join(conditions, " AND ") } - query += " ORDER BY j.enqueued_at DESC" + query += " ORDER BY j.id DESC" if limit > 0 { query += " LIMIT ?" From 518142f75bbdc29adba6c43955fe0bd0cc89c38d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:28:46 -0500 Subject: [PATCH 18/23] Revert incorrect selection update when marking review addressed When hideAddressed is on and user marks a review as addressed from the review view, do NOT update selectedIdx. The findNextViewableJob and findPrevViewableJob functions start searching from selectedIdx +/- 1, so left/right navigation naturally finds the correct adjacent visible jobs. Moving selectedIdx would cause navigation to skip a job. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 4 ++++ cmd/roborev/tui_test.go | 20 ++++++++++++-------- 2 files changed, 16 insertions(+), 8 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 3f2b3e6..8acc460 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -1626,6 +1626,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // No job associated - track by review ID instead m.pendingReviewAddressed[m.currentReview.ID] = pendingState{newState: newState, seq: seq} } + // Don't update selectedIdx here - keep it pointing at the current (now hidden) job. + // The findNextViewableJob/findPrevViewableJob functions start searching from + // selectedIdx +/- 1, so left/right navigation will naturally find the correct + // adjacent visible jobs. Moving selectedIdx would cause navigation to skip a job. return m, m.addressReview(m.currentReview.ID, jobID, newState, oldState, seq) } else if m.currentView == tuiViewQueue && len(m.jobs) > 0 && m.selectedIdx >= 0 && m.selectedIdx < len(m.jobs) { job := &m.jobs[m.selectedIdx] diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index a1fd840..a487490 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -208,8 +208,9 @@ func TestTUIToggleAddressedNoReview(t *testing.T) { func TestTUIAddressFromReviewViewWithHideAddressed(t *testing.T) { // When hideAddressed is on and user marks a review as addressed from review view, - // selection should move to next visible job when returning to queue (on escape), - // not immediately on 'a' press (so j/k navigation still works in review view) + // selectedIdx should NOT change immediately. The findNextViewableJob/findPrevViewableJob + // functions start from selectedIdx +/- 1, so left/right navigation will naturally + // find the correct adjacent visible jobs. Selection only moves on escape. m := newTuiModel("http://localhost") m.currentView = tuiViewReview m.hideAddressed = true @@ -228,26 +229,29 @@ func TestTUIAddressFromReviewViewWithHideAddressed(t *testing.T) { Job: &m.jobs[1], } - // Press 'a' to mark as addressed - selection should NOT move yet + // Press 'a' to mark as addressed - selection stays at current position updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'a'}}) m2 := updated.(tuiModel) - // Selection stays at index 1 so j/k navigation still works relative to current review + // Selection stays at index 1 so left/right navigation works correctly from current position if m2.selectedIdx != 1 { t.Errorf("After 'a': expected selectedIdx=1 (unchanged), got %d", m2.selectedIdx) } if m2.selectedJobID != 2 { t.Errorf("After 'a': expected selectedJobID=2 (unchanged), got %d", m2.selectedJobID) } + // Still viewing the current review + if m2.currentView != tuiViewReview { + t.Errorf("After 'a': expected view=review (unchanged), got %d", m2.currentView) + } - // Press escape to return to queue - NOW selection should move + // Press escape to return to queue - NOW selection moves via normalizeSelectionIfHidden updated2, _ := m2.Update(tea.KeyMsg{Type: tea.KeyEscape}) m3 := updated2.(tuiModel) - // Selection should have moved to next visible job (job 3, index 2) - // since job 2 is now addressed and hidden + // Selection moved to next visible job (job 3, index 2) if m3.selectedIdx != 2 { - t.Errorf("After escape: expected selectedIdx=2 (next visible job), got %d", m3.selectedIdx) + t.Errorf("After escape: expected selectedIdx=2, got %d", m3.selectedIdx) } if m3.selectedJobID != 3 { t.Errorf("After escape: expected selectedJobID=3, got %d", m3.selectedJobID) From d92d1380048c963824f7e6c212ef86d4c28336a9 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:39:35 -0500 Subject: [PATCH 19/23] Show flash notification when no adjacent review exists Display "No newer review" or "No older review" when pressing right/left arrows in review view and there's no adjacent review to navigate to. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 8acc460..0f5dd08 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -1428,6 +1428,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { Job: &job, } } + } else { + m.flashMessage = "No newer review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewReview } } else if m.currentView == tuiViewPrompt { if m.promptScroll > 0 { @@ -1485,6 +1489,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { Job: &job, } } + } else { + m.flashMessage = "No older review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewReview } } else if m.currentView == tuiViewPrompt { m.promptScroll++ From 96bfa073bd142e903567a1d73b0b672a4fe540a2 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:43:29 -0500 Subject: [PATCH 20/23] Move update notification to line 3 above queue table Display the sticky update notification on the third line of the TUI (above the job table) instead of at the bottom. This ensures it's only visible in the queue view, not in the review view. Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 0f5dd08..e3d6f54 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -2369,7 +2369,20 @@ func (m tuiModel) renderQueueView() string { m.status.CanceledJobs) } b.WriteString(tuiStatusStyle.Render(statusLine)) - b.WriteString("\x1b[K\n\x1b[K\n") // Clear status line and blank line + b.WriteString("\x1b[K\n") // Clear status line + + // Update notification on line 3 (above the table) + if m.updateAvailable != "" { + updateStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("226")).Bold(true) + var updateMsg string + if m.updateIsDevBuild { + updateMsg = fmt.Sprintf("Dev build - latest release: %s - run 'roborev update --force'", m.updateAvailable) + } else { + updateMsg = fmt.Sprintf("Update available: %s - run 'roborev update'", m.updateAvailable) + } + b.WriteString(updateStyle.Render(updateMsg)) + } + b.WriteString("\x1b[K\n") // Clear line 3 visibleJobList := m.getVisibleJobs() visibleSelectedIdx := m.getVisibleSelectedIdx() @@ -2487,19 +2500,10 @@ func (m tuiModel) renderQueueView() string { } b.WriteString("\x1b[K\n") // Clear scroll indicator line - // Status line: flash message (temporary) takes priority, then update notification, then blank + // Status line: flash message (temporary) if m.flashMessage != "" && time.Now().Before(m.flashExpiresAt) && m.flashView == tuiViewQueue { flashStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("46")) // Green b.WriteString(flashStyle.Render(m.flashMessage)) - } else if m.updateAvailable != "" { - updateStyle := lipgloss.NewStyle().Foreground(lipgloss.Color("226")).Bold(true) - var updateMsg string - if m.updateIsDevBuild { - updateMsg = fmt.Sprintf("Dev build - latest release: %s - run 'roborev update --force'", m.updateAvailable) - } else { - updateMsg = fmt.Sprintf("Update available: %s - run 'roborev update'", m.updateAvailable) - } - b.WriteString(updateStyle.Render(updateMsg)) } b.WriteString("\x1b[K\n") // Clear to end of line From fa6d70d9defcf661c60201b0d8355279a0c0f75d Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:46:30 -0500 Subject: [PATCH 21/23] Show flash notification at queue navigation bounds Display "No newer review" when pressing up at the top of the queue, and "No older review" when pressing down at the bottom (when there are no more jobs to load). Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index e3d6f54..2f6d906 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -1387,6 +1387,10 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { if prevIdx >= 0 { m.selectedIdx = prevIdx m.updateSelectedJobID() + } else { + m.flashMessage = "No newer review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewQueue } } else if m.currentView == tuiViewReview { if m.reviewScroll > 0 { @@ -1450,6 +1454,11 @@ func (m tuiModel) Update(msg tea.Msg) (tea.Model, tea.Cmd) { // At bottom with more jobs available - load them m.loadingMore = true return m, m.fetchMoreJobs() + } else if !m.hasMore || len(m.activeRepoFilter) > 0 { + // Truly at the bottom - no more to load or filter prevents auto-load + m.flashMessage = "No older review" + m.flashExpiresAt = time.Now().Add(2 * time.Second) + m.flashView = tuiViewQueue } } else if m.currentView == tuiViewReview { m.reviewScroll++ From 79d61361e6c05fdcf7b03cb1deba981c4127251a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 14:51:56 -0500 Subject: [PATCH 22/23] Improve fetchCommitMsg job detection and add test coverage - Check for prompt jobs before dirty jobs to handle backward compat - Return clearer error for empty GitRef instead of claiming dirty - Add tests for empty GitRef, backward compat run jobs, dirty edge cases - Add tests for review navigation bounds flash messages - Add tests for queue navigation bounds flash messages - Add tests for update notification placement in queue view Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui.go | 18 +++-- cmd/roborev/tui_test.go | 158 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 168 insertions(+), 8 deletions(-) diff --git a/cmd/roborev/tui.go b/cmd/roborev/tui.go index 2f6d906..9dc671c 100644 --- a/cmd/roborev/tui.go +++ b/cmd/roborev/tui.go @@ -738,20 +738,28 @@ func (m tuiModel) fetchReviewAndCopy(jobID int64, job *storage.ReviewJob) tea.Cm func (m tuiModel) fetchCommitMsg(job *storage.ReviewJob) tea.Cmd { jobID := job.ID return func() tea.Msg { + // Handle prompt/run jobs first (GitRef == "prompt" indicates a run task, not a commit review) + // Check this before dirty to handle backward compatibility with older run jobs + if job.GitRef == "prompt" { + return tuiCommitMsgMsg{ + jobID: jobID, + err: fmt.Errorf("no commit message for run tasks"), + } + } + // Handle dirty reviews (uncommitted changes) - // Check both DiffContent and GitRef since job lists may not populate DiffContent - if job.DiffContent != nil || job.GitRef == "dirty" || job.GitRef == "" { + if job.DiffContent != nil || job.GitRef == "dirty" { return tuiCommitMsgMsg{ jobID: jobID, err: fmt.Errorf("no commit message for uncommitted changes"), } } - // Handle prompt/run jobs (GitRef == "prompt" indicates a run task, not a commit review) - if job.GitRef == "prompt" { + // Handle missing GitRef (could be from incomplete job data or older versions) + if job.GitRef == "" { return tuiCommitMsgMsg{ jobID: jobID, - err: fmt.Errorf("no commit message for run tasks"), + err: fmt.Errorf("no git reference available for this job"), } } diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index a487490..4652ec5 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -2383,7 +2383,7 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { m.currentView = tuiViewReview m.currentReview = &storage.Review{ID: 10, Job: &storage.ReviewJob{ID: 1}} - // Press 'k' at first viewable job - should be no-op + // Press 'k' (right) at first viewable job - should show flash message updated, cmd := m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'k'}}) m2 := updated.(tuiModel) @@ -2396,13 +2396,19 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { if cmd != nil { t.Error("Expected no command at boundary") } + if m2.flashMessage != "No newer review" { + t.Errorf("Expected flash message 'No newer review', got %q", m2.flashMessage) + } + if m2.flashView != tuiViewReview { + t.Errorf("Expected flashView to be tuiViewReview, got %d", m2.flashView) + } // Now at last viewable job m.selectedIdx = 2 m.selectedJobID = 3 m.currentReview = &storage.Review{ID: 30, Job: &storage.ReviewJob{ID: 3}} - // Press 'j' at last viewable job - should be no-op + // Press 'j' (left) at last viewable job - should show flash message updated, cmd = m.Update(tea.KeyMsg{Type: tea.KeyRunes, Runes: []rune{'j'}}) m3 := updated.(tuiModel) @@ -2415,6 +2421,12 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { if cmd != nil { t.Error("Expected no command at boundary") } + if m3.flashMessage != "No older review" { + t.Errorf("Expected flash message 'No older review', got %q", m3.flashMessage) + } + if m3.flashView != tuiViewReview { + t.Errorf("Expected flashView to be tuiViewReview, got %d", m3.flashView) + } } func TestTUIReviewNavigationFailedJobInline(t *testing.T) { @@ -2616,6 +2628,78 @@ func TestTUIQueueViewArrowsMatchUpDown(t *testing.T) { } } +func TestTUIQueueNavigationBoundaries(t *testing.T) { + // Test flash messages when navigating at queue boundaries + m := newTuiModel("http://localhost") + + m.jobs = []storage.ReviewJob{ + {ID: 1, Status: storage.JobStatusDone}, + {ID: 2, Status: storage.JobStatusDone}, + {ID: 3, Status: storage.JobStatusDone}, + } + m.selectedIdx = 0 + m.selectedJobID = 1 + m.currentView = tuiViewQueue + m.hasMore = false // No more jobs to load + + // Press 'up' at top of queue - should show flash message + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyUp}) + m2 := updated.(tuiModel) + + if m2.selectedIdx != 0 { + t.Errorf("Expected selectedIdx to remain 0 at top, got %d", m2.selectedIdx) + } + if m2.flashMessage != "No newer review" { + t.Errorf("Expected flash message 'No newer review', got %q", m2.flashMessage) + } + if m2.flashView != tuiViewQueue { + t.Errorf("Expected flashView to be tuiViewQueue, got %d", m2.flashView) + } + + // Now at bottom of queue + m.selectedIdx = 2 + m.selectedJobID = 3 + m.flashMessage = "" // Clear + + // Press 'down' at bottom of queue - should show flash message + updated, _ = m.Update(tea.KeyMsg{Type: tea.KeyDown}) + m3 := updated.(tuiModel) + + if m3.selectedIdx != 2 { + t.Errorf("Expected selectedIdx to remain 2 at bottom, got %d", m3.selectedIdx) + } + if m3.flashMessage != "No older review" { + t.Errorf("Expected flash message 'No older review', got %q", m3.flashMessage) + } + if m3.flashView != tuiViewQueue { + t.Errorf("Expected flashView to be tuiViewQueue, got %d", m3.flashView) + } +} + +func TestTUIQueueNavigationBoundariesWithFilter(t *testing.T) { + // Test flash messages at bottom when filter is active (prevents auto-load) + m := newTuiModel("http://localhost") + + m.jobs = []storage.ReviewJob{ + {ID: 1, Status: storage.JobStatusDone, RepoPath: "/repo1"}, + {ID: 2, Status: storage.JobStatusDone, RepoPath: "/repo2"}, + } + m.selectedIdx = 0 + m.selectedJobID = 1 + m.currentView = tuiViewQueue + m.hasMore = true // More jobs available but... + m.activeRepoFilter = []string{"/repo1"} // Filter is active, prevents auto-load + + // Press 'down' - only job 1 matches filter, so we're at bottom + updated, _ := m.Update(tea.KeyMsg{Type: tea.KeyDown}) + m2 := updated.(tuiModel) + + // Should show flash since filter prevents loading more + if m2.flashMessage != "No older review" { + t.Errorf("Expected flash message 'No older review' with filter active, got %q", m2.flashMessage) + } +} + func TestTUIJobsRefreshDuringReviewNavigation(t *testing.T) { // Test that jobs refresh during review navigation doesn't reset selection // This tests the race condition fix: user navigates to job 3, but jobs refresh @@ -5587,6 +5671,56 @@ func TestTUIFlashMessageNotShownInDifferentView(t *testing.T) { } } +func TestTUIUpdateNotificationInQueueView(t *testing.T) { + m := newTuiModel("http://localhost") + m.currentView = tuiViewQueue + m.width = 80 + m.height = 24 + m.updateAvailable = "1.2.3" + + output := m.renderQueueView() + if !strings.Contains(output, "Update available: 1.2.3") { + t.Error("Expected update notification in queue view") + } + if !strings.Contains(output, "run 'roborev update'") { + t.Error("Expected update instructions in queue view") + } +} + +func TestTUIUpdateNotificationDevBuild(t *testing.T) { + m := newTuiModel("http://localhost") + m.currentView = tuiViewQueue + m.width = 80 + m.height = 24 + m.updateAvailable = "1.2.3" + m.updateIsDevBuild = true + + output := m.renderQueueView() + if !strings.Contains(output, "Dev build") { + t.Error("Expected 'Dev build' in notification for dev builds") + } + if !strings.Contains(output, "roborev update --force") { + t.Error("Expected --force flag in update instructions for dev builds") + } +} + +func TestTUIUpdateNotificationNotInReviewView(t *testing.T) { + m := newTuiModel("http://localhost") + m.currentView = tuiViewReview + m.width = 80 + m.height = 24 + m.updateAvailable = "1.2.3" + m.currentReview = &storage.Review{ + ID: 1, + Output: "Test review content", + } + + output := m.renderReviewView() + if strings.Contains(output, "Update available") { + t.Error("Update notification should not appear in review view") + } +} + func TestTUIFetchReviewAndCopySuccess(t *testing.T) { // Save original clipboard writer and restore after test originalClipboard := clipboardWriter @@ -6404,11 +6538,29 @@ func TestFetchCommitMsgJobTypeDetection(t *testing.T) { expectError: "no commit message for uncommitted changes", }, { - name: "empty GitRef should error", + name: "empty GitRef should error with missing ref message", job: storage.ReviewJob{ ID: 5, GitRef: "", }, + expectError: "no git reference available for this job", + }, + { + name: "empty GitRef with Prompt (backward compat run job) should error with missing ref", + job: storage.ReviewJob{ + ID: 6, + GitRef: "", + Prompt: "Explain this codebase", // older run job without GitRef=prompt + }, + expectError: "no git reference available for this job", + }, + { + name: "dirty job with nil DiffContent but GitRef=dirty should error", + job: storage.ReviewJob{ + ID: 7, + GitRef: "dirty", + DiffContent: nil, + }, expectError: "no commit message for uncommitted changes", }, } From edc79141d03f4f0850f2b454bd255644dba9f094 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Sun, 25 Jan 2026 15:00:13 -0500 Subject: [PATCH 23/23] Add flashExpiresAt assertions and verify update notification line position - Add flashExpiresAt assertions to all boundary navigation tests - Verify update notification appears on line 3 (above table) in queue view Co-Authored-By: Claude Opus 4.5 --- cmd/roborev/tui_test.go | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/cmd/roborev/tui_test.go b/cmd/roborev/tui_test.go index 4652ec5..5c2b4ca 100644 --- a/cmd/roborev/tui_test.go +++ b/cmd/roborev/tui_test.go @@ -2402,6 +2402,9 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { if m2.flashView != tuiViewReview { t.Errorf("Expected flashView to be tuiViewReview, got %d", m2.flashView) } + if m2.flashExpiresAt.IsZero() || m2.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m2.flashExpiresAt) + } // Now at last viewable job m.selectedIdx = 2 @@ -2427,6 +2430,9 @@ func TestTUIReviewNavigationBoundaries(t *testing.T) { if m3.flashView != tuiViewReview { t.Errorf("Expected flashView to be tuiViewReview, got %d", m3.flashView) } + if m3.flashExpiresAt.IsZero() || m3.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m3.flashExpiresAt) + } } func TestTUIReviewNavigationFailedJobInline(t *testing.T) { @@ -2655,6 +2661,9 @@ func TestTUIQueueNavigationBoundaries(t *testing.T) { if m2.flashView != tuiViewQueue { t.Errorf("Expected flashView to be tuiViewQueue, got %d", m2.flashView) } + if m2.flashExpiresAt.IsZero() || m2.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m2.flashExpiresAt) + } // Now at bottom of queue m.selectedIdx = 2 @@ -2674,6 +2683,9 @@ func TestTUIQueueNavigationBoundaries(t *testing.T) { if m3.flashView != tuiViewQueue { t.Errorf("Expected flashView to be tuiViewQueue, got %d", m3.flashView) } + if m3.flashExpiresAt.IsZero() || m3.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m3.flashExpiresAt) + } } func TestTUIQueueNavigationBoundariesWithFilter(t *testing.T) { @@ -2698,6 +2710,12 @@ func TestTUIQueueNavigationBoundariesWithFilter(t *testing.T) { if m2.flashMessage != "No older review" { t.Errorf("Expected flash message 'No older review' with filter active, got %q", m2.flashMessage) } + if m2.flashView != tuiViewQueue { + t.Errorf("Expected flashView to be tuiViewQueue, got %d", m2.flashView) + } + if m2.flashExpiresAt.IsZero() || m2.flashExpiresAt.Before(time.Now()) { + t.Errorf("Expected flashExpiresAt to be set in the future, got %v", m2.flashExpiresAt) + } } func TestTUIJobsRefreshDuringReviewNavigation(t *testing.T) { @@ -5685,6 +5703,17 @@ func TestTUIUpdateNotificationInQueueView(t *testing.T) { if !strings.Contains(output, "run 'roborev update'") { t.Error("Expected update instructions in queue view") } + + // Verify update notification appears on line 3 (index 2) - above the table + // Layout: line 0 = title, line 1 = status, line 2 = update notification + lines := strings.Split(output, "\n") + if len(lines) < 3 { + t.Fatalf("Expected at least 3 lines, got %d", len(lines)) + } + // Line 2 (third line) should contain the update notification + if !strings.Contains(lines[2], "Update available") { + t.Errorf("Expected update notification on line 3 (index 2), got: %q", lines[2]) + } } func TestTUIUpdateNotificationDevBuild(t *testing.T) {