From df61bb802969b898318aad8f1bcfe41f4a1ac45f Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Sat, 9 May 2026 07:22:26 -0400 Subject: [PATCH 1/4] feat(tui): hide auto-design-router noise from queue by default Adds a show_classify_jobs config (off by default) that hides auto-design-router byproducts from the TUI queue: rows with job_type='classify' (the routing decision in flight) and rows with status='skipped' (the terminal state for skipped design reviews). Both predicates are scoped to source='auto_design' so a future pipeline that adopts the same job_type or status for unrelated reasons isn't silently swallowed. Configuration: - Global Config.ShowClassifyJobs (bool, default false). - RepoConfig.ShowClassifyJobs (*bool override). - Per-repo override applies when the queue is filtered to a single repo, otherwise the global value is used. This avoids one repo's preference leaking into a multi-repo view. - ResolveShowClassifyJobs mirrors ResolveAutoClosePassingReviews. Server-side filtering: - New /api/jobs?hide_classify_jobs=true query param. - storage.WithHideClassifyJobs adds a single SQL clause that hides rows where source='auto_design' AND (job_type='classify' OR status='skipped'), with COALESCE on j.source so rows with source IS NULL aren't dropped by three-valued-logic NULL. TUI: - Session-only 's' hotkey toggles visibility on the fly. Footer label tracks state ("show classify" vs "hide classify"), and the '?' help view lists the shortcut under Filtering. - Hitting 'l' on an auto-design classify row or terminal skipped row shows the classifier's verdict, skip_reason, and any internal error in a header band above the (typically empty) streamed log. Reasoning lines are width-truncated and have embedded newlines/tabs folded to spaces so each occupies exactly one terminal row, keeping logVisibleLines accurate for long classifier error blobs. Both classify and skipped triggers require source='auto_design'. - Effective show-classify value is cached on the model and recomputed only on filter or session-toggle changes, keeping render and fetch paths free of LoadRepoConfig disk I/O. Tests cover resolver precedence, the SQL filter (with auto_design, empty source, NULL source, and non-auto_design classify cases), the /api/jobs endpoint, classify reasoning header rendering, width truncation, and newline folding. --- cmd/roborev/tui/control_handlers.go | 2 + cmd/roborev/tui/fetch.go | 14 ++ cmd/roborev/tui/filter.go | 2 + cmd/roborev/tui/handlers.go | 2 + cmd/roborev/tui/handlers_modal.go | 4 + cmd/roborev/tui/handlers_queue.go | 17 ++ cmd/roborev/tui/helpers_test.go | 2 +- cmd/roborev/tui/nav.go | 6 +- cmd/roborev/tui/render_log.go | 104 ++++++++++++ cmd/roborev/tui/render_log_classify_test.go | 176 ++++++++++++++++++++ cmd/roborev/tui/render_queue.go | 8 +- cmd/roborev/tui/tui.go | 59 ++++++- internal/config/config.go | 19 +++ internal/config/config_test.go | 39 +++++ internal/daemon/server.go | 3 + internal/daemon/server_jobs_test.go | 46 +++++ internal/daemon/types.go | 1 + internal/daemon_client/client.gen.go | 58 ++++++- internal/daemon_client/openapi-3.0.json | 18 ++ internal/storage/db_filter_test.go | 107 ++++++++++++ internal/storage/jobs.go | 25 +++ 21 files changed, 699 insertions(+), 13 deletions(-) create mode 100644 cmd/roborev/tui/render_log_classify_test.go diff --git a/cmd/roborev/tui/control_handlers.go b/cmd/roborev/tui/control_handlers.go index 22c2fe935..53210ab80 100644 --- a/cmd/roborev/tui/control_handlers.go +++ b/cmd/roborev/tui/control_handlers.go @@ -254,6 +254,7 @@ func (m model) handleCtrlSetFilter( m.fetchSeq++ m.queueColGen++ m.loadingJobs = true + m.recomputeClassifyEffective() return m, controlResponse{OK: true}, m.fetchJobs() } @@ -299,6 +300,7 @@ func (m model) handleCtrlClearFilter( m.fetchSeq++ m.queueColGen++ m.loadingJobs = true + m.recomputeClassifyEffective() return m, controlResponse{OK: true}, m.fetchJobs() } diff --git a/cmd/roborev/tui/fetch.go b/cmd/roborev/tui/fetch.go index 967b2985a..357cdae1c 100644 --- a/cmd/roborev/tui/fetch.go +++ b/cmd/roborev/tui/fetch.go @@ -149,6 +149,10 @@ func listJobsParams(values neturl.Values) daemonclient.ListJobsParams { } setStringParam("job_type", ¶ms.JobType) setStringParam("exclude_job_type", ¶ms.ExcludeJobType) + if value := values.Get("hide_classify_jobs"); value != "" { + typed := daemonclient.ListJobsParamsHideClassifyJobs(value) + params.HideClassifyJobs = &typed + } setStringParam("repo_prefix", ¶ms.RepoPrefix) setIntParam("limit", ¶ms.Limit) setIntParam("offset", ¶ms.Offset) @@ -198,6 +202,13 @@ func (m model) fetchJobs() tea.Cmd { // Exclude fix jobs — they belong in the Tasks view, not the queue params.Set("exclude_job_type", "fix") + // Hide auto-design-router byproducts (classify rows + skipped design + // rows) unless the user opted in via show_classify_jobs. Resolved at + // fetch time so single-repo filters honor that repo's override. + if !m.shouldShowClassifyJobs() { + params.Set("hide_classify_jobs", "true") + } + // Set limit: use pagination unless we need client-side filtering (multi-repo) if needsAllJobs { params.Set("limit", "0") @@ -240,6 +251,9 @@ func (m model) fetchMoreJobs() tea.Cmd { params.Set("closed", "false") } params.Set("exclude_job_type", "fix") + if !m.shouldShowClassifyJobs() { + params.Set("hide_classify_jobs", "true") + } result, err := m.loadJobsPage(params) if err != nil { return paginationErrMsg{ diff --git a/cmd/roborev/tui/filter.go b/cmd/roborev/tui/filter.go index 3172fdff9..fcf4c3b16 100644 --- a/cmd/roborev/tui/filter.go +++ b/cmd/roborev/tui/filter.go @@ -255,6 +255,7 @@ func (m *model) reconcileAutoRepoFilter() bool { m.fetchSeq++ m.queueColGen++ m.loadingJobs = true + m.recomputeClassifyEffective() return true } @@ -319,6 +320,7 @@ func (m *model) popFilter() string { case filterTypeRepo: m.activeRepoFilter = nil m.autoRepoFilter = false + m.recomputeClassifyEffective() case filterTypeBranch: m.activeBranchFilter = "" } diff --git a/cmd/roborev/tui/handlers.go b/cmd/roborev/tui/handlers.go index f397bbf45..267d46c63 100644 --- a/cmd/roborev/tui/handlers.go +++ b/cmd/roborev/tui/handlers.go @@ -129,6 +129,8 @@ func (m model) handleGlobalKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { return m.handleBranchFilterOpenKey() case "h": return m.handleHideClosedKey() + case "s": + return m.handleToggleClassifyKey() case "c": return m.handleCommentOpenKey() case "y": diff --git a/cmd/roborev/tui/handlers_modal.go b/cmd/roborev/tui/handlers_modal.go index 1b0768265..6bc9ab092 100644 --- a/cmd/roborev/tui/handlers_modal.go +++ b/cmd/roborev/tui/handlers_modal.go @@ -172,6 +172,10 @@ func (m model) handleFilterKey(msg tea.KeyMsg) (tea.Model, tea.Cmd) { m.fetchSeq++ m.queueColGen++ m.loadingJobs = true + // activeRepoFilter may have changed; refresh the cached + // classify visibility so the next render/fetch sees the + // repo-scoped value without hitting disk on the hot path. + m.recomputeClassifyEffective() return m, m.fetchJobs() case "backspace": if len(m.filterSearch) > 0 { diff --git a/cmd/roborev/tui/handlers_queue.go b/cmd/roborev/tui/handlers_queue.go index 83cf18c54..f75d7aa50 100644 --- a/cmd/roborev/tui/handlers_queue.go +++ b/cmd/roborev/tui/handlers_queue.go @@ -372,3 +372,20 @@ func (m model) handleHideClosedKey() (tea.Model, tea.Cmd) { m.loadingJobs = true return m, m.fetchJobs() } + +// handleToggleClassifyKey flips visibility of auto-design-router +// classifier rows and skipped design rows in the queue. The toggle +// is session-only; it overrides show_classify_jobs config until the +// TUI is restarted. +func (m model) handleToggleClassifyKey() (tea.Model, tea.Cmd) { + if m.currentView != viewQueue { + return m, nil + } + next := !m.shouldShowClassifyJobs() + m.classifyOverride = &next + m.recomputeClassifyEffective() + m.queueColGen++ + m.fetchSeq++ + m.loadingJobs = true + return m, m.fetchJobs() +} diff --git a/cmd/roborev/tui/helpers_test.go b/cmd/roborev/tui/helpers_test.go index 59dd97034..de3908f1b 100644 --- a/cmd/roborev/tui/helpers_test.go +++ b/cmd/roborev/tui/helpers_test.go @@ -495,7 +495,7 @@ func TestRenderHelpTableLinesWithinWidth(t *testing.T) { helpSets := map[string][][]helpItem{ "queue": { {{"x", "cancel"}, {"r", "rerun"}, {"l", "log"}, {"p", "prompt"}, {"c", "comment"}, {"y", "copy"}, {"m", "commit"}, {"F", "fix"}}, - {{"↑/↓", "nav"}, {"enter", "review"}, {"a", "closed"}, {"f", "filter"}, {"h", "hide"}, {"T", "tasks"}, {"?", "help"}, {"q", "quit"}}, + {{"↑/↓", "nav"}, {"enter", "review"}, {"a", "closed"}, {"f", "filter"}, {"h", "hide"}, {"s", "show classify"}, {"T", "tasks"}, {"?", "help"}, {"q", "quit"}}, }, "review": { {{"p", "prompt"}, {"c", "comment"}, {"m", "commit"}, {"a", "closed"}, {"y", "copy"}, {"F", "fix"}}, diff --git a/cmd/roborev/tui/nav.go b/cmd/roborev/tui/nav.go index a14ff979e..32013b671 100644 --- a/cmd/roborev/tui/nav.go +++ b/cmd/roborev/tui/nav.go @@ -144,17 +144,17 @@ func (m *model) logViewLookupJob() *storage.ReviewJob { } // logVisibleLines returns the number of content lines visible in the -// log view, accounting for title, optional command line, separator, -// status, and help bar. +// log view, accounting for title, optional command line, optional +// classify-reasoning lines, separator, status, and help bar. func (m *model) logVisibleLines() int { // title + separator + status + help(N) helpRows := m.logHelpRows() reserved := 3 + len(reflowHelpRows(helpRows, m.width)) - // Check if command line header is shown if job := m.logViewLookupJob(); job != nil { if commandLineForJob(job) != "" { reserved++ } + reserved += len(classifyReasoningLines(job, m.width)) } return max(m.height-reserved, 1) } diff --git a/cmd/roborev/tui/render_log.go b/cmd/roborev/tui/render_log.go index c9179dd79..d2b57767b 100644 --- a/cmd/roborev/tui/render_log.go +++ b/cmd/roborev/tui/render_log.go @@ -5,6 +5,7 @@ import ( "strings" "github.com/mattn/go-runewidth" + "github.com/roborev-dev/roborev/internal/storage" ) func (m model) renderLogView() string { @@ -42,6 +43,19 @@ func (m model) renderLogView() string { headerLines++ } + // For auto-design-router rows (classify-typed, or terminal skipped + // design reviews) the streamed agent log is empty — the classifier + // runs as a one-shot SchemaAgent.Decide call, not a streaming chat. + // Surface the classifier's verdict and reasoning here so 'l' on a + // classify/skipped row actually shows something useful. Truncated + // to m.width so each returned string occupies exactly one terminal + // row, keeping the log content area aligned with logVisibleLines. + for _, line := range classifyReasoningLines(job, m.width) { + b.WriteString(line) + b.WriteString("\x1b[K\n") + headerLines++ + } + // Calculate visible area (must match logVisibleLines()) logHelp := m.logHelpRows() logHelpLines := len(reflowHelpRows(logHelp, m.width)) @@ -105,6 +119,95 @@ func (m model) renderLogView() string { return b.String() } +// classifyReasoningLines returns pre-styled header lines describing the +// auto-design-router classifier's verdict for a classify-typed row or a +// terminal skipped design row. Returns nil for jobs that aren't part of +// the auto-design pipeline so the log view leaves their layout alone. +// +// Both classify and skipped triggers require source='auto_design'; a +// future pipeline that adopts the classify job_type or the skipped +// status for unrelated reasons should not be mislabeled as +// auto-design output. +// +// Each returned string is truncated to fit width so it occupies exactly +// one terminal row. width <= 0 disables truncation (useful for tests or +// when the caller doesn't know the terminal width yet); the styled +// output reflects the raw text in that case. +func classifyReasoningLines(job *storage.ReviewJob, width int) []string { + if job == nil || job.Source != "auto_design" { + return nil + } + isClassify := job.JobType == storage.JobTypeClassify + isAutoDesignSkipped := job.Status == storage.JobStatusSkipped + if !isClassify && !isAutoDesignSkipped { + return nil + } + + var verdict string + switch { + case isClassify && job.Status != storage.JobStatusSkipped: + // classify row that hasn't transitioned yet (queued/running) + verdict = "Auto-design classifier in progress" + case isAutoDesignSkipped: + verdict = "Auto-design verdict: no design review needed" + default: + verdict = "Auto-design classifier" + } + + render := func(text string) string { + // Fold embedded newlines/tabs to single spaces so each + // rendered line occupies one terminal row. job.Error stores + // raw classifier stderr/error text and routinely contains + // '\n'; without this, multiline blobs would wrap across + // terminal rows while logVisibleLines reserves only one row + // per returned line, pushing status/help off the screen. + text = foldWhitespace(text) + if width > 0 && runewidth.StringWidth(text) > width { + text = runewidth.Truncate(text, width, "…") + } + return statusStyle.Render(text) + } + + var lines []string + lines = append(lines, render(verdict)) + if job.SkipReason != "" { + lines = append(lines, render("Reason: "+job.SkipReason)) + } + if job.Error != "" { + lines = append(lines, render("Detail: "+job.Error)) + } + return lines +} + +// foldWhitespace replaces embedded newlines, carriage returns, and +// tabs with a single space and collapses runs of resulting whitespace. +// Used to keep one-row-per-line invariants when rendering raw error or +// reason text into the log view header. +func foldWhitespace(s string) string { + if !strings.ContainsAny(s, "\n\r\t") { + return s + } + var b strings.Builder + b.Grow(len(s)) + prevSpace := false + for _, r := range s { + switch r { + case '\n', '\r', '\t': + r = ' ' + } + if r == ' ' { + if prevSpace { + continue + } + prevSpace = true + } else { + prevSpace = false + } + b.WriteRune(r) + } + return b.String() +} + func formatHelpLine(key, desc string) string { return fmt.Sprintf(" %-14s %s", key, desc) } @@ -147,6 +250,7 @@ func helpLines(tasksEnabled, noQuit bool) []string { keys: []struct{ key, desc string }{ {"f", "Filter by repository/branch"}, {"h", "Toggle hide closed/failed"}, + {"s", "Toggle classify rows (auto-design router)"}, {"esc", "Clear filters (one at a time)"}, }, }, diff --git a/cmd/roborev/tui/render_log_classify_test.go b/cmd/roborev/tui/render_log_classify_test.go new file mode 100644 index 000000000..562767b32 --- /dev/null +++ b/cmd/roborev/tui/render_log_classify_test.go @@ -0,0 +1,176 @@ +package tui + +import ( + "strings" + "testing" + + "github.com/mattn/go-runewidth" + "github.com/muesli/ansi" + "github.com/roborev-dev/roborev/internal/storage" + "github.com/stretchr/testify/assert" +) + +func TestClassifyReasoningLines(t *testing.T) { + tests := []struct { + name string + job *storage.ReviewJob + want []string + wantEmpty bool + }{ + { + name: "nil job", + job: nil, + wantEmpty: true, + }, + { + name: "plain review job", + job: &storage.ReviewJob{ + ID: 1, + JobType: storage.JobTypeReview, + Status: storage.JobStatusDone, + }, + wantEmpty: true, + }, + { + name: "running classify", + job: &storage.ReviewJob{ + ID: 2, + JobType: storage.JobTypeClassify, + Status: storage.JobStatusRunning, + Source: "auto_design", + }, + want: []string{"in progress"}, + }, + { + name: "skipped auto_design with reason", + job: &storage.ReviewJob{ + ID: 3, + JobType: storage.JobTypeReview, + Status: storage.JobStatusSkipped, + Source: "auto_design", + SkipReason: "trivial diff", + }, + want: []string{"no design review needed", "trivial diff"}, + }, + { + name: "skipped auto_design with classifier error", + job: &storage.ReviewJob{ + ID: 4, + JobType: storage.JobTypeReview, + Status: storage.JobStatusSkipped, + Source: "auto_design", + SkipReason: "classifier unavailable", + Error: "classify_agent \"codex\" not registered", + }, + want: []string{ + "no design review needed", + "classifier unavailable", + "not registered", + }, + }, + { + name: "skipped non-auto_design row leaves log alone", + job: &storage.ReviewJob{ + ID: 5, + JobType: storage.JobTypeReview, + Status: storage.JobStatusSkipped, + Source: "", + SkipReason: "unrelated reason", + }, + wantEmpty: true, + }, + { + // Future pipeline could use classify job_type for unrelated + // routing; mislabeling those as auto-design output would be + // wrong. + name: "non-auto_design classify row leaves log alone", + job: &storage.ReviewJob{ + ID: 6, + JobType: storage.JobTypeClassify, + Status: storage.JobStatusRunning, + Source: "", + }, + wantEmpty: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + // Width 0 = no truncation; tests use raw content + // substrings. + got := classifyReasoningLines(tt.job, 0) + if tt.wantEmpty { + assert.Empty(t, got, + "expected no header lines for non-classify job") + return + } + joined := strings.Join(got, "\n") + for _, want := range tt.want { + assert.Contains(t, joined, want, + "reasoning header should mention %q", want) + } + }) + } +} + +// TestClassifyReasoningLinesFoldsNewlines locks in that embedded +// newlines/tabs in raw classifier error text are folded to spaces so +// each returned line stays on a single terminal row. job.Error is +// stored raw (sanitization only applies to skip_reason at storage +// entry), and a multiline blob would break the row-reservation +// invariant in logVisibleLines. +func TestClassifyReasoningLinesFoldsNewlines(t *testing.T) { + job := &storage.ReviewJob{ + ID: 7, + JobType: storage.JobTypeReview, + Status: storage.JobStatusSkipped, + Source: "auto_design", + SkipReason: "line one\nline two", + Error: "exec: classifier\n\tstderr=timeout\n\tcode=124", + } + got := classifyReasoningLines(job, 0) + assert.Len(t, got, 3) + for i, line := range got { + assert.NotContains(t, line, "\n", + "line %d contains a newline: %q", i, line) + assert.NotContains(t, line, "\r", + "line %d contains a carriage return: %q", i, line) + assert.NotContains(t, line, "\t", + "line %d contains a tab: %q", i, line) + } +} + +// TestClassifyReasoningLinesTruncation locks in the contract that +// renderLogView and logVisibleLines both rely on: each returned line +// occupies exactly one terminal row. If the lines could wrap, the +// reserved-space calculation would be wrong and the log content area +// would misalign. +func TestClassifyReasoningLinesTruncation(t *testing.T) { + longReason := strings.Repeat("very long classifier reason ", 8) + longError := strings.Repeat("internal error blob ", 12) + job := &storage.ReviewJob{ + ID: 6, + JobType: storage.JobTypeReview, + Status: storage.JobStatusSkipped, + Source: "auto_design", + SkipReason: longReason, + Error: longError, + } + const width = 80 + + got := classifyReasoningLines(job, width) + assert.Len(t, got, 3, "expected verdict + reason + detail rows") + for i, line := range got { + // Strip ANSI styling to measure printable width. + stripped := ansi.PrintableRuneWidth(line) + // runewidth.StringWidth on the rendered line includes ANSI + // codes so use ansi.PrintableRuneWidth for the assertion; + // fall back to a runewidth check on a stripped copy if + // PrintableRuneWidth returns 0 (older versions). + if stripped == 0 { + stripped = runewidth.StringWidth(line) + } + assert.LessOrEqual(t, stripped, width, + "line %d (%q) exceeds width %d", i, line, width) + } +} diff --git a/cmd/roborev/tui/render_queue.go b/cmd/roborev/tui/render_queue.go index a2ff68211..26896e9eb 100644 --- a/cmd/roborev/tui/render_queue.go +++ b/cmd/roborev/tui/render_queue.go @@ -43,7 +43,13 @@ func (m model) queueHelpRows() [][]helpItem { if !m.lockedRepoFilter || !m.lockedBranchFilter { row2 = append(row2, helpItem{"f", "filter"}) } - row2 = append(row2, helpItem{"h", "hide"}, helpItem{"D", "focus"}) + row2 = append(row2, helpItem{"h", "hide"}) + if m.shouldShowClassifyJobs() { + row2 = append(row2, helpItem{"s", "hide classify"}) + } else { + row2 = append(row2, helpItem{"s", "show classify"}) + } + row2 = append(row2, helpItem{"D", "focus"}) if m.tasksWorkflowEnabled() { row2 = append(row2, helpItem{"T", "tasks"}) } diff --git a/cmd/roborev/tui/tui.go b/cmd/roborev/tui/tui.go index 156b3ca95..39b74ede5 100644 --- a/cmd/roborev/tui/tui.go +++ b/cmd/roborev/tui/tui.go @@ -316,6 +316,20 @@ type model struct { activeBranchFilter string // Empty = show all, otherwise branch name to filter by filterStack []string // Order of applied filters: "repo", "branch" - for escape to pop in order hideClosed bool // When true, hide jobs with closed reviews + // globalCfg is cached at startup and consulted at fetch time so that + // show_classify_jobs can be resolved against whichever repo is the + // currently active single-repo filter (rather than baked in once + // from the cwd at launch). + globalCfg *config.Config + // classifyOverride is a session-level toggle that overrides the + // config-derived show_classify_jobs value. nil = follow config; + // non-nil = use the override regardless of config. + classifyOverride *bool + // classifyEffective caches the resolved show-classify-jobs value + // so render and fetch paths don't hit disk via LoadRepoConfig on + // every paint or event. Recomputed by recomputeClassifyEffective + // whenever classifyOverride or activeRepoFilter changes. + classifyEffective bool // Display name cache (keyed by repo path) displayNames map[string]string @@ -482,6 +496,7 @@ func newModel(ep daemon.DaemonEndpoint, opts ...option) model { colOrder := parseColumnOrder(nil) taskColOrder := parseTaskColumnOrder(nil) var cwdRepoRoot, cwdRepoIdentity, cwdBranch string + var globalCfg *config.Config if !opt.disableExternalIO { // Read daemon version from runtime file @@ -491,6 +506,7 @@ func newModel(ep daemon.DaemonEndpoint, opts ...option) model { // Load preferences from config if cfg, err := config.LoadGlobal(); err == nil { + globalCfg = cfg hideClosed = cfg.HideClosedByDefault autoFilterRepo = cfg.AutoFilterRepo autoFilterBranch = cfg.AutoFilterBranch @@ -583,7 +599,7 @@ func newModel(ep daemon.DaemonEndpoint, opts ...option) model { } httpClient := ep.HTTPClient(10 * time.Second) - return model{ + m := model{ endpoint: ep, daemonVersion: daemonVersion, client: httpClient, @@ -596,6 +612,7 @@ func newModel(ep daemon.DaemonEndpoint, opts ...option) model { loadingJobs: true, // Init() calls fetchJobs, so mark as loading loadingStatus: true, // Init() calls fetchStatus, so mark as loading hideClosed: hideClosed, + globalCfg: globalCfg, activeRepoFilter: activeRepoFilter, autoRepoFilter: autoRepoFilterActive, activeBranchFilter: activeBranchFilter, @@ -624,6 +641,10 @@ func newModel(ep daemon.DaemonEndpoint, opts ...option) model { queueColCache: &colWidthCache{gen: -1}, taskColCache: &colWidthCache{gen: -1}, } + // Seed the cached classify-visibility decision once so render and + // fetch can read m.classifyEffective without hitting disk. + m.recomputeClassifyEffective() + return m } func (m model) Init() tea.Cmd { @@ -650,6 +671,42 @@ func (m model) tasksDisabledMessage() string { return "Tasks workflow disabled. Set advanced.tasks_enabled=true in global config to enable it." } +// shouldShowClassifyJobs returns the cached effective value computed +// by recomputeClassifyEffective. Hot path: callable from render and +// fetch without filesystem I/O. +func (m model) shouldShowClassifyJobs() bool { + return m.classifyEffective +} + +// recomputeClassifyEffective resolves whether the queue should include +// auto-design-router classifier rows and skipped design rows, and +// caches the result in m.classifyEffective. +// +// Precedence: a session-level toggle (the 's' key) takes priority over +// config. When unset, falls back to the per-repo override if the queue +// is filtered to a single repo, otherwise the global setting. This +// avoids the surprise of one repo's override leaking into a multi-repo +// view while still letting the user flip the toggle on the fly. +// +// Call this from any handler that mutates classifyOverride or +// activeRepoFilter. It only loads repo config from disk under the +// single-repo-filter branch, so amortizing it to filter/toggle changes +// keeps the render and fetch paths I/O-free. +func (m *model) recomputeClassifyEffective() { + switch { + case m.classifyOverride != nil: + m.classifyEffective = *m.classifyOverride + case len(m.activeRepoFilter) == 1: + m.classifyEffective = config.ResolveShowClassifyJobs( + m.activeRepoFilter[0], m.globalCfg, + ) + case m.globalCfg != nil: + m.classifyEffective = m.globalCfg.ShowClassifyJobs + default: + m.classifyEffective = false + } +} + // getDisplayName returns the display name for a repo, using the cache. // Falls back to loading from config on cache miss, then to the provided default name. func (m *model) getDisplayName(repoPath, defaultName string) string { diff --git a/internal/config/config.go b/internal/config/config.go index 875c4977e..b4defdb82 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -197,6 +197,7 @@ type Config struct { HideAddressedByDefault bool `toml:"hide_addressed_by_default"` // deprecated: use hide_closed_by_default AutoFilterRepo bool `toml:"auto_filter_repo" comment:"Automatically filter the TUI queue to the current repo."` AutoFilterBranch bool `toml:"auto_filter_branch" comment:"Automatically filter the TUI queue to the current branch."` + ShowClassifyJobs bool `toml:"show_classify_jobs" comment:"Show auto-design-review classifier rows (and skipped design rows) in the TUI queue. Off by default to reduce noise."` MouseEnabled bool `toml:"mouse_enabled" comment:"Enable mouse support in the TUI."` // Enable mouse capture and mouse-driven TUI interactions TabWidth int `toml:"tab_width"` // Tab expansion width for TUI rendering (default: 2) HiddenColumns []string `toml:"hidden_columns" comment:"Queue columns to hide in the TUI."` // Column names to hide in queue table (e.g. ["branch", "agent"]) @@ -1070,6 +1071,7 @@ type RepoConfig struct { // Behavior AutoClosePassingReviews *bool `toml:"auto_close_passing_reviews" comment:"Automatically close reviews that pass with no findings in this repo."` + ShowClassifyJobs *bool `toml:"show_classify_jobs" comment:"Override whether the TUI queue shows auto-design-review classifier rows for this repo. Omit to inherit."` // Hooks configuration (per-repo) Hooks []HookConfig `toml:"hooks"` @@ -1542,6 +1544,23 @@ func ResolveAutoClosePassingReviews(repoPath string, globalCfg *Config) bool { return resolveBool(globalVal, repoVal) } +// ResolveShowClassifyJobs returns whether the TUI queue should display +// auto-design-review classifier rows (job_type=classify) and skipped +// design rows (status=skipped). Off by default to reduce queue noise +// when the auto-design router is enabled. Per-repo config overrides +// global. +func ResolveShowClassifyJobs(repoPath string, globalCfg *Config) bool { + var repoVal *bool + if repoCfg, err := LoadRepoConfig(repoPath); err == nil && repoCfg != nil { + repoVal = repoCfg.ShowClassifyJobs + } + var globalVal bool + if globalCfg != nil { + globalVal = globalCfg.ShowClassifyJobs + } + return resolveBool(globalVal, repoVal) +} + // ResolveExcludePatterns returns the merged exclude patterns from // repo config and global config. Repo patterns are read from the // default branch (like review guidelines) to prevent untrusted diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 69460a734..6f84c09bb 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -387,6 +387,45 @@ func TestResolveAutoClosePassingReviews(t *testing.T) { } } +func TestResolveShowClassifyJobs(t *testing.T) { + tests := []struct { + name string + repoConfig string + globalConfig *Config + want bool + }{ + { + name: "default false", + want: false, + }, + { + name: "global enabled", + globalConfig: &Config{ShowClassifyJobs: true}, + want: true, + }, + { + name: "repo overrides global to true", + repoConfig: `show_classify_jobs = true`, + globalConfig: &Config{ShowClassifyJobs: false}, + want: true, + }, + { + name: "repo overrides global to false", + repoConfig: `show_classify_jobs = false`, + globalConfig: &Config{ShowClassifyJobs: true}, + want: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tmpDir := newTempRepo(t, tt.repoConfig) + got := ResolveShowClassifyJobs(tmpDir, tt.globalConfig) + assert.Equal(t, tt.want, got) + }) + } +} + func TestResolveReasoning(t *testing.T) { type resolverFunc func(explicit string, dir string, globalCfg *Config) (string, error) diff --git a/internal/daemon/server.go b/internal/daemon/server.go index ad4196af0..2657a8b25 100644 --- a/internal/daemon/server.go +++ b/internal/daemon/server.go @@ -889,6 +889,9 @@ func (s *Server) humaListJobs( storage.WithExcludeJobType(input.ExcludeJobType), ) } + if input.HideClassifyJobs == "true" { + listOpts = append(listOpts, storage.WithHideClassifyJobs()) + } if repoPrefix != "" && repo == "" { listOpts = append( listOpts, storage.WithRepoPrefix(repoPrefix), diff --git a/internal/daemon/server_jobs_test.go b/internal/daemon/server_jobs_test.go index 92c61b098..c1ab746fe 100644 --- a/internal/daemon/server_jobs_test.go +++ b/internal/daemon/server_jobs_test.go @@ -2367,6 +2367,52 @@ func TestHandleListJobsJobTypeFilter(t *testing.T) { }, "Expected non-fix job, got fix") } }) + + t.Run("hide_classify_jobs=true hides classify and skipped rows", func(t *testing.T) { + // Add a running classify row and a skipped design row alongside + // the existing review + fix jobs. The (repo_id, commit_id, + // review_type='design') partial unique index on auto_design rows + // means each auto_design row needs a distinct commit. + classifyCommit, err := db.GetOrCreateCommit(repo.ID, "jt-classify", "Author", "Subject", time.Now()) + require.NoError(t, err) + _, err = db.Exec(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, agent, status, job_type, review_type, source, worker_id, started_at, enqueued_at, updated_at) + VALUES (?, ?, 'jt-classify', 'test', 'running', 'classify', 'design', 'auto_design', 'w1', datetime('now'), datetime('now'), datetime('now')) + `, repo.ID, classifyCommit.ID) + require.NoError(t, err) + + skippedCommit, err := db.GetOrCreateCommit(repo.ID, "jt-skipped", "Author", "Subject", time.Now()) + require.NoError(t, err) + _, err = db.Exec(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, agent, status, job_type, review_type, source, skip_reason, enqueued_at, finished_at, updated_at) + VALUES (?, ?, 'jt-skipped', 'test', 'skipped', 'review', 'design', 'auto_design', 'trivial diff', datetime('now'), datetime('now'), datetime('now')) + `, repo.ID, skippedCommit.ID) + require.NoError(t, err) + + req := httptest.NewRequest( + http.MethodGet, "/api/jobs?hide_classify_jobs=true", nil, + ) + w := httptest.NewRecorder() + server.httpServer.Handler.ServeHTTP(w, req) + testutil.AssertStatusCode(t, w, http.StatusOK) + + var resp struct { + Jobs []storage.ReviewJob `json:"jobs"` + } + testutil.DecodeJSON(t, w, &resp) + + // Should see the original review + fix (2 jobs), not the classify + // or skipped rows. + assert.Len(t, resp.Jobs, 2, "expected only review and fix jobs") + for _, j := range resp.Jobs { + assert.NotEqual(t, storage.JobTypeClassify, j.JobType, + "classify rows should be hidden") + assert.NotEqual(t, storage.JobStatusSkipped, j.Status, + "skipped rows should be hidden") + } + }) } func TestHandleListJobsRepoPrefixFilter(t *testing.T) { diff --git a/internal/daemon/types.go b/internal/daemon/types.go index 857334cfe..31642a740 100644 --- a/internal/daemon/types.go +++ b/internal/daemon/types.go @@ -65,6 +65,7 @@ type ListJobsInput struct { Closed string `query:"closed" doc:"Filter by review closed state" enum:"true,false,"` JobType string `query:"job_type" doc:"Filter by job type"` ExcludeJobType string `query:"exclude_job_type" doc:"Exclude jobs of this type"` + HideClassifyJobs string `query:"hide_classify_jobs" doc:"Hide auto-design-router rows (job_type=classify and status=skipped)" enum:"true,false,"` RepoPrefix string `query:"repo_prefix" doc:"Filter repos by path prefix"` Limit int `query:"limit" default:"-999999" doc:"Max results (default 50, 0=unlimited, max 10000)"` Offset int `query:"offset" default:"-1" doc:"Skip N results (requires limit>0)"` diff --git a/internal/daemon_client/client.gen.go b/internal/daemon_client/client.gen.go index ddb919660..523a58b4e 100644 --- a/internal/daemon_client/client.gen.go +++ b/internal/daemon_client/client.gen.go @@ -59,18 +59,39 @@ func (e ListJobsParamsClosed) Valid() bool { } } +// Defines values for ListJobsParamsHideClassifyJobs. +const ( + ListJobsParamsHideClassifyJobsEmpty ListJobsParamsHideClassifyJobs = "" + ListJobsParamsHideClassifyJobsFalse ListJobsParamsHideClassifyJobs = "false" + ListJobsParamsHideClassifyJobsTrue ListJobsParamsHideClassifyJobs = "true" +) + +// Valid indicates whether the value is a known member of the ListJobsParamsHideClassifyJobs enum. +func (e ListJobsParamsHideClassifyJobs) Valid() bool { + switch e { + case ListJobsParamsHideClassifyJobsEmpty: + return true + case ListJobsParamsHideClassifyJobsFalse: + return true + case ListJobsParamsHideClassifyJobsTrue: + return true + default: + return false + } +} + // Defines values for GetSummaryParamsAll. const ( - False GetSummaryParamsAll = "false" - True GetSummaryParamsAll = "true" + GetSummaryParamsAllFalse GetSummaryParamsAll = "false" + GetSummaryParamsAllTrue GetSummaryParamsAll = "true" ) // Valid indicates whether the value is a known member of the GetSummaryParamsAll enum. func (e GetSummaryParamsAll) Valid() bool { switch e { - case False: + case GetSummaryParamsAllFalse: return true - case True: + case GetSummaryParamsAllTrue: return true default: return false @@ -465,9 +486,10 @@ type RepoSummary struct { // RepoWithCount defines model for RepoWithCount. type RepoWithCount struct { - Count int64 `json:"count"` - Name string `json:"name"` - RootPath string `json:"root_path"` + Count int64 `json:"count"` + Identity *string `json:"identity,omitempty"` + Name string `json:"name"` + RootPath string `json:"root_path"` } // RerunJobOutputBody defines model for RerunJobOutputBody. @@ -696,6 +718,9 @@ type ListJobsParams struct { // ExcludeJobType Exclude jobs of this type ExcludeJobType *string `form:"exclude_job_type,omitempty" json:"exclude_job_type,omitempty"` + // HideClassifyJobs Hide auto-design-router rows (job_type=classify and status=skipped) + HideClassifyJobs *ListJobsParamsHideClassifyJobs `form:"hide_classify_jobs,omitempty" json:"hide_classify_jobs,omitempty"` + // RepoPrefix Filter repos by path prefix RepoPrefix *string `form:"repo_prefix,omitempty" json:"repo_prefix,omitempty"` @@ -715,6 +740,9 @@ type ListJobsParamsBranchIncludeEmpty string // ListJobsParamsClosed defines parameters for ListJobs. type ListJobsParamsClosed string +// ListJobsParamsHideClassifyJobs defines parameters for ListJobs. +type ListJobsParamsHideClassifyJobs string + // ListReposParams defines parameters for ListRepos. type ListReposParams struct { // Branch Filter to repos with jobs on this branch @@ -2332,6 +2360,22 @@ func NewListJobsRequest(server string, params *ListJobsParams) (*http.Request, e } + if params.HideClassifyJobs != nil { + + if queryFrag, err := runtime.StyleParamWithOptions("form", false, "hide_classify_jobs", *params.HideClassifyJobs, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationQuery, Type: "string", Format: ""}); err != nil { + return nil, err + } else if parsed, err := url.ParseQuery(queryFrag); err != nil { + return nil, err + } else { + for k, v := range parsed { + for _, v2 := range v { + queryValues.Add(k, v2) + } + } + } + + } + if params.RepoPrefix != nil { if queryFrag, err := runtime.StyleParamWithOptions("form", false, "repo_prefix", *params.RepoPrefix, runtime.StyleParamOptions{ParamLocation: runtime.ParamLocationQuery, Type: "string", Format: ""}); err != nil { diff --git a/internal/daemon_client/openapi-3.0.json b/internal/daemon_client/openapi-3.0.json index b45531537..40af8f453 100644 --- a/internal/daemon_client/openapi-3.0.json +++ b/internal/daemon_client/openapi-3.0.json @@ -1221,6 +1221,9 @@ "format": "int64", "type": "integer" }, + "identity": { + "type": "string" + }, "name": { "type": "string" }, @@ -2569,6 +2572,21 @@ "type": "string" } }, + { + "description": "Hide auto-design-router rows (job_type=classify and status=skipped)", + "explode": false, + "in": "query", + "name": "hide_classify_jobs", + "schema": { + "description": "Hide auto-design-router rows (job_type=classify and status=skipped)", + "enum": [ + "true", + "false", + "" + ], + "type": "string" + } + }, { "description": "Filter repos by path prefix", "explode": false, diff --git a/internal/storage/db_filter_test.go b/internal/storage/db_filter_test.go index 035eb6cb2..e1f82bb48 100644 --- a/internal/storage/db_filter_test.go +++ b/internal/storage/db_filter_test.go @@ -741,6 +741,113 @@ func TestListJobsWithJobTypeFilter(t *testing.T) { } } +func TestListJobsWithHideClassifyJobs(t *testing.T) { + db := openTestDB(t) + defer db.Close() + + // Plain queued review — should always be visible. + repo, _, _ := createJobChain(t, db, "/tmp/repo-hide-classify", "review-sha") + + // Running classify row (auto-design router decision in flight). The + // (repo_id, commit_id, review_type='design') partial unique index on + // source='auto_design' rows means each auto_design row needs a + // distinct commit. + classifyCommit := createCommit(t, db, repo.ID, "classify-sha") + var classifyID int64 + require.NoError(t, db.QueryRow(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, status, job_type, review_type, source, worker_id, started_at, enqueued_at, updated_at) + VALUES (?, ?, 'classify-sha', 'running', 'classify', 'design', 'auto_design', 'w1', datetime('now'), datetime('now'), datetime('now')) + RETURNING id + `, repo.ID, classifyCommit.ID).Scan(&classifyID)) + + // Skipped design row (classifier or heuristic decided no design + // review needed). + skippedCommit := createCommit(t, db, repo.ID, "skipped-sha") + var skippedID int64 + require.NoError(t, db.QueryRow(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, status, job_type, review_type, source, skip_reason, enqueued_at, finished_at, updated_at) + VALUES (?, ?, 'skipped-sha', 'skipped', 'review', 'design', 'auto_design', 'trivial diff', datetime('now'), datetime('now'), datetime('now')) + RETURNING id + `, repo.ID, skippedCommit.ID).Scan(&skippedID)) + + // Skipped row that did NOT come from the auto-design router (source + // set to empty string). A future pipeline could adopt + // status='skipped' for some other reason; this filter must leave + // such rows alone. + otherSkippedCommit := createCommit(t, db, repo.ID, "other-skipped-sha") + var otherSkippedID int64 + require.NoError(t, db.QueryRow(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, status, job_type, review_type, source, skip_reason, enqueued_at, finished_at, updated_at) + VALUES (?, ?, 'other-skipped-sha', 'skipped', 'review', '', '', 'unrelated reason', datetime('now'), datetime('now'), datetime('now')) + RETURNING id + `, repo.ID, otherSkippedCommit.ID).Scan(&otherSkippedID)) + + // Skipped row with source IS NULL (the column is nullable, and + // EnqueueJob never sets it for plain reviews). This case is + // load-bearing: a NULL-naive predicate would silently drop these + // rows from results. + nullSkippedCommit := createCommit(t, db, repo.ID, "null-skipped-sha") + var nullSkippedID int64 + require.NoError(t, db.QueryRow(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, status, job_type, review_type, skip_reason, enqueued_at, finished_at, updated_at) + VALUES (?, ?, 'null-skipped-sha', 'skipped', 'review', '', 'unrelated reason', datetime('now'), datetime('now'), datetime('now')) + RETURNING id + `, repo.ID, nullSkippedCommit.ID).Scan(&nullSkippedID)) + + // Classify-typed row that did NOT come from the auto-design router + // (source=''). A future pipeline could use the classify job_type + // for unrelated routing; the filter must leave such rows alone. + otherClassifyCommit := createCommit(t, db, repo.ID, "other-classify-sha") + var otherClassifyID int64 + require.NoError(t, db.QueryRow(` + INSERT INTO review_jobs + (repo_id, commit_id, git_ref, status, job_type, review_type, source, worker_id, started_at, enqueued_at, updated_at) + VALUES (?, ?, 'other-classify-sha', 'running', 'classify', 'review', '', 'w2', datetime('now'), datetime('now'), datetime('now')) + RETURNING id + `, repo.ID, otherClassifyCommit.ID).Scan(&otherClassifyID)) + + t.Run("default returns all rows", func(t *testing.T) { + jobs, err := db.ListJobs("", "", 50, 0) + require.NoError(t, err) + assert.Len(t, jobs, 6, + "default should include both classify variants, all three skipped variants, and the plain review row") + }) + + t.Run("hide_classify_jobs only hides auto_design rows", func(t *testing.T) { + jobs, err := db.ListJobs("", "", 50, 0, WithHideClassifyJobs()) + require.NoError(t, err) + // Should keep: plain review, source='' skipped, source IS NULL + // skipped, non-auto_design classify. Should hide: classify row + // with source='auto_design', skipped row with source='auto_design'. + assert.Len(t, jobs, 4, "should hide only auto-design router rows") + var sawOtherSkipped, sawNullSkipped, sawOtherClassify bool + for _, j := range jobs { + assert.NotEqual(t, classifyID, j.ID, + "auto_design classify row should be hidden") + assert.NotEqual(t, skippedID, j.ID, + "auto_design skipped row should be hidden") + switch j.ID { + case otherSkippedID: + sawOtherSkipped = true + case nullSkippedID: + sawNullSkipped = true + case otherClassifyID: + sawOtherClassify = true + } + } + assert.True(t, sawOtherSkipped, + "skipped rows with source='' must remain visible") + assert.True(t, sawNullSkipped, + "skipped rows with source IS NULL must remain visible") + assert.True(t, sawOtherClassify, + "classify rows with source != 'auto_design' must remain visible") + }) +} + func TestEscapeLike(t *testing.T) { tests := []struct { input string diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index 758b99263..f9d913752 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -686,6 +686,7 @@ type listJobsOptions struct { closed *bool jobType string excludeJobType string + hideClassifyJobs bool repoPrefix string beforeCursor *int64 } @@ -724,6 +725,16 @@ func WithExcludeJobType(jobType string) ListJobsOption { return func(o *listJobsOptions) { o.excludeJobType = jobType } } +// WithHideClassifyJobs excludes auto-design-router byproducts: rows +// with job_type='classify' (the routing decision itself) and rows +// with status='skipped' (the terminal state for skipped design +// reviews). Both clauses are scoped to source='auto_design' so a +// future pipeline that creates classify jobs or skipped jobs for a +// different reason is not silently swallowed by this filter. +func WithHideClassifyJobs() ListJobsOption { + return func(o *listJobsOptions) { o.hideClassifyJobs = true } +} + // WithBeforeCursor filters jobs to those with ID < cursor (for cursor pagination). func WithBeforeCursor(id int64) ListJobsOption { return func(o *listJobsOptions) { o.beforeCursor = &id } @@ -799,6 +810,20 @@ func buildJobFilterClause(statusFilter, repoFilter string, o listJobsOptions) (s conditions = append(conditions, "j.job_type != ?") args = append(args, o.excludeJobType) } + if o.hideClassifyJobs { + // source is nullable, so guard the comparison with COALESCE. + // Without it, j.source = 'auto_design' returns NULL for rows + // where source IS NULL, NOT (TRUE AND NULL) is also NULL, and + // the row gets silently dropped from results. + // + // Both predicates are scoped to source='auto_design' so a + // future pipeline that adopts classify jobs or status='skipped' + // for an unrelated reason isn't silently hidden. + conditions = append(conditions, + "NOT (COALESCE(j.source, '') = 'auto_design' AND (j.job_type = ? OR j.status = ?))") + args = append(args, + JobTypeClassify, string(JobStatusSkipped)) + } if o.beforeCursor != nil { conditions = append(conditions, "j.id < ?") args = append(args, *o.beforeCursor) From 2a5fce664962f17f605fa458852de460c5f9cdf7 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Sat, 9 May 2026 07:26:31 -0400 Subject: [PATCH 2/4] fix(tui): label terminal classify rows with their actual status Caught by standard review 2217: classifyReasoningLines treated every classify row whose status wasn't 'skipped' as 'in progress', so a failed or canceled classifier showed an in-progress verdict alongside the error detail. That misleads operators looking at why a classifier didn't run. Branch on the row's status: queued/running stays 'in progress', while failed and canceled get explicit verdict text. Anything unexpected falls through to a verbatim '...: ' so a future status value can't silently regress to a misleading default. Tests cover queued, running, failed, and canceled classify rows; the failed and canceled cases also assert the header does NOT contain 'in progress'. --- cmd/roborev/tui/render_log.go | 19 +++++++-- cmd/roborev/tui/render_log_classify_test.go | 43 +++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/tui/render_log.go b/cmd/roborev/tui/render_log.go index d2b57767b..6b04042c4 100644 --- a/cmd/roborev/tui/render_log.go +++ b/cmd/roborev/tui/render_log.go @@ -145,11 +145,24 @@ func classifyReasoningLines(job *storage.ReviewJob, width int) []string { var verdict string switch { - case isClassify && job.Status != storage.JobStatusSkipped: - // classify row that hasn't transitioned yet (queued/running) - verdict = "Auto-design classifier in progress" case isAutoDesignSkipped: verdict = "Auto-design verdict: no design review needed" + case isClassify: + // classify rows can sit in queued/running, or end in + // failed/canceled when something went wrong before the + // classifier could MarkClassifyAsSkippedDesign or + // PromoteClassifyToDesignReview the row. Distinguish these so + // 'l' on a failed classifier doesn't claim it's still running. + switch job.Status { + case storage.JobStatusQueued, storage.JobStatusRunning: + verdict = "Auto-design classifier in progress" + case storage.JobStatusFailed: + verdict = "Auto-design classifier failed" + case storage.JobStatusCanceled: + verdict = "Auto-design classifier canceled" + default: + verdict = "Auto-design classifier: " + string(job.Status) + } default: verdict = "Auto-design classifier" } diff --git a/cmd/roborev/tui/render_log_classify_test.go b/cmd/roborev/tui/render_log_classify_test.go index 562767b32..1ccd8e155 100644 --- a/cmd/roborev/tui/render_log_classify_test.go +++ b/cmd/roborev/tui/render_log_classify_test.go @@ -15,6 +15,7 @@ func TestClassifyReasoningLines(t *testing.T) { name string job *storage.ReviewJob want []string + notWant []string wantEmpty bool }{ { @@ -41,6 +42,44 @@ func TestClassifyReasoningLines(t *testing.T) { }, want: []string{"in progress"}, }, + { + name: "queued classify", + job: &storage.ReviewJob{ + ID: 20, + JobType: storage.JobTypeClassify, + Status: storage.JobStatusQueued, + Source: "auto_design", + }, + want: []string{"in progress"}, + }, + { + // Terminal classify failures must not claim "in progress"; + // the operator pressing 'l' needs the actual status. + name: "failed classify", + job: &storage.ReviewJob{ + ID: 21, + JobType: storage.JobTypeClassify, + Status: storage.JobStatusFailed, + Source: "auto_design", + Error: "exec: classifier: timeout after 30s", + }, + want: []string{"failed", "timeout"}, + notWant: []string{ + "in progress", + "no design review needed", + }, + }, + { + name: "canceled classify", + job: &storage.ReviewJob{ + ID: 22, + JobType: storage.JobTypeClassify, + Status: storage.JobStatusCanceled, + Source: "auto_design", + }, + want: []string{"canceled"}, + notWant: []string{"in progress"}, + }, { name: "skipped auto_design with reason", job: &storage.ReviewJob{ @@ -109,6 +148,10 @@ func TestClassifyReasoningLines(t *testing.T) { assert.Contains(t, joined, want, "reasoning header should mention %q", want) } + for _, notWant := range tt.notWant { + assert.NotContains(t, joined, notWant, + "reasoning header should NOT mention %q", notWant) + } }) } } From feda678100bba91d78e29cfa59509080e8b21e79 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Sat, 9 May 2026 09:39:13 -0400 Subject: [PATCH 3/4] fix: surface classify reasoning end-to-end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two bugs that hid the new classify reasoning header from view, found by running the TUI against seeded auto-design rows: 1. ListJobs didn't select j.skip_reason or j.source — the columns were added by the auto-design router PR (#661) but the list query was not updated. Result: every job loaded by the TUI's queue or /api/jobs caller had Source="" and SkipReason="". The classifyReasoningLines helper requires Source='auto_design', so it always returned nil for list-fetched jobs and the reasoning header never rendered. GetJobByID already selects both columns; ListJobs is now in sync. 2. The TUI's log view bounced back to the queue with a "No log available for this job" flash whenever the on-disk log file was missing. That fires for every classify/skipped row because the classifier runs as a one-shot SchemaAgent.Decide call and never writes to the streaming log. So even with the data fixed, the user couldn't see the reasoning header — the view exited before it rendered. Now: when the job has classifyReasoningLines output, stay in the log view with an empty body so renderLogView's header band shows the verdict, skip reason, and any classifier error. Verified manually with an isolated daemon + seeded auto_design rows; captures show the verdict + reason + detail rendering correctly for both terminal skipped and failed classify states. --- cmd/roborev/tui/handlers_msg.go | 16 +++++++++++++++- internal/storage/jobs.go | 6 ++++-- 2 files changed, 19 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/tui/handlers_msg.go b/cmd/roborev/tui/handlers_msg.go index 85feb88df..dd18d6b26 100644 --- a/cmd/roborev/tui/handlers_msg.go +++ b/cmd/roborev/tui/handlers_msg.go @@ -546,8 +546,22 @@ func (m model) handleLogOutputMsg( } if msg.err != nil { if errors.Is(msg.err, errNoLog) { + // Auto-design-router rows (running classify, terminal + // skipped, failed/canceled classify) never produce a + // streamed agent log because the classifier is a + // one-shot SchemaAgent.Decide call. Stay in the log + // view so renderLogView's classifyReasoningLines + // header can show the verdict, skip reason, and any + // classifier error — bouncing back to the queue + // would hide that information behind a flash. + job := m.logViewLookupJob() + if job != nil && len(classifyReasoningLines(job, m.width)) > 0 { + m.logLines = []logLine{} + m.logStreaming = false + return m, nil + } flash := "No log available for this job" - if job := m.logViewLookupJob(); job != nil && + if job != nil && job.Status == storage.JobStatusFailed && job.Error != "" { flash = fmt.Sprintf( diff --git a/internal/storage/jobs.go b/internal/storage/jobs.go index f9d913752..e89685a01 100644 --- a/internal/storage/jobs.go +++ b/internal/storage/jobs.go @@ -843,7 +843,8 @@ func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int COALESCE(j.agentic, 0), r.root_path, r.name, c.subject, rv.closed, rv.output, rv.verdict_bool, j.source_machine_id, j.uuid, j.model, j.job_type, j.review_type, j.patch_id, j.parent_job_id, j.provider, j.requested_model, j.requested_provider, j.token_usage, COALESCE(j.worktree_path, ''), - j.command_line, COALESCE(j.min_severity, '') + j.command_line, COALESCE(j.min_severity, ''), + j.skip_reason, j.source FROM review_jobs j JOIN repos r ON r.id = j.repo_id LEFT JOIN commits c ON c.id = j.commit_id @@ -882,7 +883,8 @@ func (db *DB) ListJobs(statusFilter string, repoFilter string, limit, offset int &fields.Agentic, &j.RepoPath, &j.RepoName, &fields.CommitSubject, &fields.Closed, &output, &verdictBool, &fields.SourceMachineID, &fields.UUID, &fields.Model, &fields.JobType, &fields.ReviewType, &fields.PatchID, &fields.ParentJobID, &fields.Provider, &fields.RequestedModel, &fields.RequestedProvider, &fields.TokenUsage, &fields.WorktreePath, - &fields.CommandLine, &fields.MinSeverity) + &fields.CommandLine, &fields.MinSeverity, + &fields.SkipReason, &fields.Source) if err != nil { return nil, err } From 3a86be2d4a5a6f9c5ae04b27db693feb67bffe07 Mon Sep 17 00:00:00 2001 From: Phillip Cloud <417981+cpcloud@users.noreply.github.com> Date: Sat, 9 May 2026 09:57:53 -0400 Subject: [PATCH 4/4] fix: address roborev-ci review findings MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - render_log.go: distinguish degraded auto-design skips from clean ones. completeClassifyAsSkip writes status=skipped with job.Error populated when the classifier execution fails, but the previous branch labeled both kinds of skipped rows as "no design review needed" — misleading operators about a classifier failure. Now branches on job.Error: empty → "no design review needed", non-empty → "Auto-design classifier failed (skipped)". - render_log_classify_test.go: rename the failure-path case to assert it now contains "classifier failed" and does NOT contain "no design review needed"; add notWant to the clean-skip case to lock in that it does NOT contain "failed". --- cmd/roborev/tui/render_log.go | 13 ++++++++++++- cmd/roborev/tui/render_log_classify_test.go | 15 +++++++++++++-- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/cmd/roborev/tui/render_log.go b/cmd/roborev/tui/render_log.go index 6b04042c4..02789022d 100644 --- a/cmd/roborev/tui/render_log.go +++ b/cmd/roborev/tui/render_log.go @@ -146,7 +146,18 @@ func classifyReasoningLines(job *storage.ReviewJob, width int) []string { var verdict string switch { case isAutoDesignSkipped: - verdict = "Auto-design verdict: no design review needed" + // completeClassifyAsSkip converts a classifier execution + // failure into a skipped row with job.Error populated. A + // clean "no design review needed" verdict comes from + // applyClassifyVerdict and leaves job.Error empty. Don't + // label a degraded skip as a clean verdict — operators + // reading the log header need to see that the classifier + // errored. + if job.Error != "" { + verdict = "Auto-design classifier failed (skipped)" + } else { + verdict = "Auto-design verdict: no design review needed" + } case isClassify: // classify rows can sit in queued/running, or end in // failed/canceled when something went wrong before the diff --git a/cmd/roborev/tui/render_log_classify_test.go b/cmd/roborev/tui/render_log_classify_test.go index 1ccd8e155..f3f8d2bfe 100644 --- a/cmd/roborev/tui/render_log_classify_test.go +++ b/cmd/roborev/tui/render_log_classify_test.go @@ -81,6 +81,8 @@ func TestClassifyReasoningLines(t *testing.T) { notWant: []string{"in progress"}, }, { + // Clean classifier verdict (applyClassifyVerdict path): + // MarkClassifyAsSkippedDesign with empty errorDetail. name: "skipped auto_design with reason", job: &storage.ReviewJob{ ID: 3, @@ -89,9 +91,15 @@ func TestClassifyReasoningLines(t *testing.T) { Source: "auto_design", SkipReason: "trivial diff", }, - want: []string{"no design review needed", "trivial diff"}, + want: []string{"no design review needed", "trivial diff"}, + notWant: []string{"failed"}, }, { + // Daemon path: classifier execution fails → + // completeClassifyAsSkip → MarkClassifyAsSkippedDesign + // produces a row with status=skipped, source=auto_design, + // AND a non-empty Error. Must NOT be labeled as a clean + // "no design review needed" verdict. name: "skipped auto_design with classifier error", job: &storage.ReviewJob{ ID: 4, @@ -102,10 +110,13 @@ func TestClassifyReasoningLines(t *testing.T) { Error: "classify_agent \"codex\" not registered", }, want: []string{ - "no design review needed", + "classifier failed", "classifier unavailable", "not registered", }, + notWant: []string{ + "no design review needed", + }, }, { name: "skipped non-auto_design row leaves log alone",