From c05891ede3f40d2202020ededf18e41328c63704 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20D=2E?= Date: Fri, 19 Jun 2026 18:27:09 -0300 Subject: [PATCH 1/2] feat(tui): add 4R review agents to model picker MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add RRPhases() function with 4 review agents (risk, readability, reliability, resilience) - Add RRSeparatorRowIdx() for 4R separator row navigation - Update ModelPickerRows() to include 4R section after JD agents - Update applyAssignment() and applyAssignmentPreservingMatchingEffort() to handle 4R rows - Update renderPhaseList() to render 4R separator and agent rows - Update title to mention 4R reviewers - Update ModelPickerRowsForProfile() comment to clarify 4R exclusion - Update ConfigurableAgentPhases() to include 4R agents - Update TestModelPickerRows_Count (16 → 21 rows) - Add 5 new tests for 4R navigation and assignment - Update ConfigurableAgentPhases() comment to mention 4R reviewers Closes #928 --- internal/opencode/models.go | 20 +++- internal/tui/screens/model_picker.go | 69 +++++++++++- internal/tui/screens/model_picker_test.go | 129 +++++++++++++++++++++- 3 files changed, 209 insertions(+), 9 deletions(-) diff --git a/internal/opencode/models.go b/internal/opencode/models.go index 2a50663fb..ccf251e92 100644 --- a/internal/opencode/models.go +++ b/internal/opencode/models.go @@ -426,13 +426,27 @@ func JDPhases() []string { } } +// RRPhases returns the ordered list of 4R review sub-agent names. +// These are workflow-level agents (not SDD phases) used for multi-dimensional +// PR review: risk assessment, readability, reliability, and resilience. +// They support independent model configuration for diversity of perspective. +func RRPhases() []string { + return []string{ + "review-risk", + "review-readability", + "review-reliability", + "review-resilience", + } +} + // ConfigurableAgentPhases returns all agent names that support per-agent -// model configuration. This includes SDD phases + JD agents. +// model configuration. This includes SDD phases + JD agents + 4R reviewers. // Used by the inject model assignment table builder and the configurable agent set -// in ReadCurrentModelAssignments. The TUI model picker uses SDDPhases() and -// JDPhases() separately for row layout control. +// in ReadCurrentModelAssignments. The TUI model picker uses SDDPhases(), JDPhases(), +// and RRPhases() separately for row layout control. func ConfigurableAgentPhases() []string { phases := SDDPhases() phases = append(phases, JDPhases()...) + phases = append(phases, RRPhases()...) return phases } diff --git a/internal/tui/screens/model_picker.go b/internal/tui/screens/model_picker.go index 3d4586762..916b9cde7 100644 --- a/internal/tui/screens/model_picker.go +++ b/internal/tui/screens/model_picker.go @@ -123,9 +123,10 @@ const SDDOrchestratorPhase = "gentle-orchestrator" // ModelPickerRows returns the row labels for the model picker screen. // Row 0 is "gentle-orchestrator" (coordinator), row 1 is "Set all phases", -// rows 2-11 are the 10 SDD sub-agent phases, then a separator and JD agents. +// rows 2-11 are the 10 SDD sub-agent phases, then a separator and JD agents, +// then another separator and 4R review agents. func ModelPickerRows() []string { - rows := make([]string, 0, 2+len(opencode.SDDPhases())+1+len(opencode.JDPhases())) + rows := make([]string, 0, 2+len(opencode.SDDPhases())+1+len(opencode.JDPhases())+1+len(opencode.RRPhases())) rows = append(rows, SDDOrchestratorPhase) rows = append(rows, "Set all SDD phases") rows = append(rows, opencode.SDDPhases()...) @@ -133,11 +134,15 @@ func ModelPickerRows() []string { rows = append(rows, "--- Judgment Day ---") rows = append(rows, opencode.JDPhases()...) } + if len(opencode.RRPhases()) > 0 { + rows = append(rows, "--- 4R Review ---") + rows = append(rows, opencode.RRPhases()...) + } return rows } // ModelPickerRowsForProfile returns model picker rows for profile creation. -// JD agents are excluded because they are global (not profile-scoped). +// JD agents and 4R reviewers are excluded because they are global (not profile-scoped). func ModelPickerRowsForProfile() []string { rows := make([]string, 0, 2+len(opencode.SDDPhases())) rows = append(rows, SDDOrchestratorPhase) @@ -158,6 +163,19 @@ func SeparatorRowIdx() int { return 2 + len(opencode.SDDPhases()) } +// RRSeparatorRowIdx returns the index of the "--- 4R Review ---" separator +// row in ModelPickerRows(). Returns -1 if there are no 4R phases (and thus +// no separator). This is used by the TUI to skip the separator during +// cursor navigation and model selection. +func RRSeparatorRowIdx() int { + rr := opencode.RRPhases() + if len(rr) == 0 { + return -1 + } + // After orchestrator + "Set all" + SDD phases + JD separator + JD phases + return 2 + len(opencode.SDDPhases()) + 1 + len(opencode.JDPhases()) +} + // ProviderEntries returns sorted provider entries with display names and model counts. func ProviderEntries(state ModelPickerState) []ProviderEntry { entries := make([]ProviderEntry, 0, len(state.AvailableIDs)) @@ -424,7 +442,9 @@ func compareVersionKeys(left, right []int) int { func applyAssignmentPreservingMatchingEffort(state ModelPickerState, assignments map[string]model.ModelAssignment, assignment model.ModelAssignment, preserveEffort bool) map[string]model.ModelAssignment { phases := opencode.SDDPhases() jdPhases := opencode.JDPhases() + rrPhases := opencode.RRPhases() separatorIdx := SeparatorRowIdx() + rrSeparatorIdx := RRSeparatorRowIdx() switch { case state.SelectedPhaseIdx == 0: assignments[SDDOrchestratorPhase] = preserveMatchingEffort(assignments[SDDOrchestratorPhase], assignment, preserveEffort) @@ -434,6 +454,14 @@ func applyAssignmentPreservingMatchingEffort(state ModelPickerState, assignments } case state.SelectedPhaseIdx == separatorIdx: // Separator row ("--- Judgment Day ---") — no action, skip. + case state.SelectedPhaseIdx == rrSeparatorIdx: + // 4R separator row ("--- 4R Review ---") — no action, skip. + case state.SelectedPhaseIdx > rrSeparatorIdx: + // 4R agent rows: map to RRPhases() after separator. + rrIdx := state.SelectedPhaseIdx - rrSeparatorIdx - 1 + if rrIdx < len(rrPhases) { + assignments[rrPhases[rrIdx]] = preserveMatchingEffort(assignments[rrPhases[rrIdx]], assignment, preserveEffort) + } case state.SelectedPhaseIdx > separatorIdx: // JD agent rows: map to JDPhases() after separator. jdIdx := state.SelectedPhaseIdx - separatorIdx - 1 @@ -473,7 +501,9 @@ func formatAssignmentLabel(row, provName, modelName, effort string) string { func applyAssignment(state ModelPickerState, assignments map[string]model.ModelAssignment, assignment model.ModelAssignment) map[string]model.ModelAssignment { phases := opencode.SDDPhases() jdPhases := opencode.JDPhases() + rrPhases := opencode.RRPhases() separatorIdx := SeparatorRowIdx() + rrSeparatorIdx := RRSeparatorRowIdx() switch { case state.SelectedPhaseIdx == 0: assignments[SDDOrchestratorPhase] = assignment @@ -483,6 +513,14 @@ func applyAssignment(state ModelPickerState, assignments map[string]model.ModelA } case state.SelectedPhaseIdx == separatorIdx: // Separator row ("--- Judgment Day ---") — no action, skip. + case state.SelectedPhaseIdx == rrSeparatorIdx: + // 4R separator row ("--- 4R Review ---") — no action, skip. + case state.SelectedPhaseIdx > rrSeparatorIdx: + // 4R agent rows: map to RRPhases() after separator. + rrIdx := state.SelectedPhaseIdx - rrSeparatorIdx - 1 + if rrIdx < len(rrPhases) { + assignments[rrPhases[rrIdx]] = assignment + } case state.SelectedPhaseIdx > separatorIdx: // JD agent rows: map to JDPhases() after separator. jdIdx := state.SelectedPhaseIdx - separatorIdx - 1 @@ -633,7 +671,7 @@ func renderPhaseList( ) string { var b strings.Builder - title := "Assign Models to SDD Phases & JD Agents" + title := "Assign Models to SDD Phases, JD Agents & 4R Reviewers" if state.ForProfile { title = "Assign Models to SDD Phases" } @@ -668,7 +706,9 @@ func renderPhaseList( } phases := opencode.SDDPhases() jdPhases := opencode.JDPhases() + rrPhases := opencode.RRPhases() separatorIdx := SeparatorRowIdx() + rrSeparatorIdx := RRSeparatorRowIdx() for idx, row := range rows { focused := idx == cursor @@ -702,6 +742,27 @@ func renderPhaseList( b.WriteString(styles.SubtextStyle.Render(" " + row) + "\n") } continue + case idx == rrSeparatorIdx: + // 4R separator row — render as a visual divider with subtle indicator when focused. + if focused { + b.WriteString(styles.SubtextStyle.Render("▸ " + row) + "\n") + } else { + b.WriteString(styles.SubtextStyle.Render(" " + row) + "\n") + } + continue + case idx > rrSeparatorIdx: + // 4R agent rows + rrIdx := idx - rrSeparatorIdx - 1 + if rrIdx < len(rrPhases) { + phase := rrPhases[rrIdx] + assignment, ok := assignments[phase] + if ok && assignment.ProviderID != "" { + provName, modelName := resolveNames(assignment, state) + label = fmt.Sprintf("%-20s %s / %s", row, provName, modelName) + } else { + label = fmt.Sprintf("%-20s (default)", row) + } + } case idx > separatorIdx: // JD agent rows jdIdx := idx - separatorIdx - 1 diff --git a/internal/tui/screens/model_picker_test.go b/internal/tui/screens/model_picker_test.go index a28675e89..4ce1f14da 100644 --- a/internal/tui/screens/model_picker_test.go +++ b/internal/tui/screens/model_picker_test.go @@ -31,8 +31,8 @@ func makeTestState(phaseIdx int) *ModelPickerState { func TestModelPickerRows_Count(t *testing.T) { rows := ModelPickerRows() - // 1 orchestrator + 1 "Set all" + 10 sub-agents + 1 separator + 3 JD agents = 16 - want := 16 + // 1 orchestrator + 1 "Set all" + 10 sub-agents + 1 JD separator + 3 JD agents + 1 4R separator + 4 4R agents = 21 + want := 21 if len(rows) != want { t.Fatalf("ModelPickerRows() len = %d, want %d; rows = %v", len(rows), want, rows) } @@ -1243,6 +1243,119 @@ func TestHandleModelNav_JDLastRow(t *testing.T) { } } +// ─── handleModelNav: 4R review agent rows ───────────────────────────────── + +func TestHandleModelNav_RRAgentRows_AssignCorrectly(t *testing.T) { + rrPhases := opencode.RRPhases() + rrSepIdx := RRSeparatorRowIdx() + + for i, expectedPhase := range rrPhases { + t.Run(expectedPhase, func(t *testing.T) { + state := makeTestState(rrSepIdx + 1 + i) // 4R rows start after 4R separator + assignments := make(map[string]model.ModelAssignment) + + handled, updated := handleModelNav("enter", state, assignments) + + if !handled { + t.Fatal("handleModelNav should return handled=true on enter") + } + + // The target 4R phase must be assigned. + a, ok := updated[expectedPhase] + if !ok || a.ProviderID == "" { + t.Errorf("4R phase %q should be assigned; assignments: %v", expectedPhase, updated) + } + if a.ProviderID != "test-provider" { + t.Errorf("4R phase %q ProviderID = %q, want %q", expectedPhase, a.ProviderID, "test-provider") + } + if a.ModelID != "model-alpha" { + t.Errorf("4R phase %q ModelID = %q, want %q", expectedPhase, a.ModelID, "model-alpha") + } + + // No other 4R phase must be assigned. + for _, other := range rrPhases { + if other == expectedPhase { + continue + } + if _, exists := updated[other]; exists { + t.Errorf("unrelated 4R phase %q should not be assigned; assignments: %v", other, updated) + } + } + + // No SDD phase, JD agent, or orchestrator must be assigned. + for _, sdd := range opencode.SDDPhases() { + if _, exists := updated[sdd]; exists { + t.Errorf("SDD phase %q should not be assigned by 4R row; assignments: %v", sdd, updated) + } + } + for _, jd := range opencode.JDPhases() { + if _, exists := updated[jd]; exists { + t.Errorf("JD agent %q should not be assigned by 4R row; assignments: %v", jd, updated) + } + } + if _, exists := updated[SDDOrchestratorPhase]; exists { + t.Errorf("orchestrator should not be assigned by 4R row; assignments: %v", updated) + } + }) + } +} + +func TestHandleModelNav_RRFirstRow(t *testing.T) { + // Verify the FIRST 4R row (right after 4R separator) maps to review-risk. + rrPhases := opencode.RRPhases() + if len(rrPhases) == 0 { + t.Skip("no 4R phases defined") + } + rrSepIdx := RRSeparatorRowIdx() + state := makeTestState(rrSepIdx + 1) + assignments := make(map[string]model.ModelAssignment) + + _, updated := handleModelNav("enter", state, assignments) + + if _, ok := updated[rrPhases[0]]; !ok { + t.Fatalf("first 4R row should assign %q; got: %v", rrPhases[0], updated) + } +} + +func TestHandleModelNav_RRLastRow(t *testing.T) { + // Verify the LAST 4R row maps to the last 4R phase. + rrPhases := opencode.RRPhases() + if len(rrPhases) == 0 { + t.Skip("no 4R phases defined") + } + rrSepIdx := RRSeparatorRowIdx() + state := makeTestState(rrSepIdx + len(rrPhases)) + assignments := make(map[string]model.ModelAssignment) + + _, updated := handleModelNav("enter", state, assignments) + + lastPhase := rrPhases[len(rrPhases)-1] + if _, ok := updated[lastPhase]; !ok { + t.Fatalf("last 4R row should assign %q; got: %v", lastPhase, updated) + } +} + +func TestHandleModelNav_RRSeparatorRow_NoAssignment(t *testing.T) { + // Verify that selecting the 4R separator row does not assign any phase. + rrSepIdx := RRSeparatorRowIdx() + if rrSepIdx < 0 { + t.Skip("no 4R separator defined") + } + state := makeTestState(rrSepIdx) + assignments := make(map[string]model.ModelAssignment) + + handled, updated := handleModelNav("enter", state, assignments) + + if !handled { + t.Fatal("handleModelNav should return handled=true on separator enter") + } + + // No phase should be assigned when separator is selected. + if len(updated) != 0 { + t.Errorf("separator row should not assign any phase; got: %v", updated) + } +} + // ─── ModelPickerRowsForProfile ────────────────────────────────────────── func TestModelPickerRowsForProfile_Count(t *testing.T) { @@ -1274,3 +1387,15 @@ func TestModelPickerRowsForProfile_NoJDAgents(t *testing.T) { } } } + +func TestModelPickerRowsForProfile_NoRRAgents(t *testing.T) { + rows := ModelPickerRowsForProfile() + rrPhases := opencode.RRPhases() + for _, rr := range rrPhases { + for _, row := range rows { + if row == rr { + t.Fatalf("ModelPickerRowsForProfile() should not contain 4R agent %q; got: %v", rr, rows) + } + } + } +} From 2aaf1020b8c56f41202925fe97a375009e2327f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Mat=C3=ADas=20D=2E?= Date: Fri, 19 Jun 2026 23:45:59 -0300 Subject: [PATCH 2/2] fix(tui): address CodeRabbit review feedback for 4R model picker - Add defensive check in RRSeparatorRowIdx() to match ModelPickerRows() logic - Add test assertion to verify 4R separator is excluded from profile rows Addresses CodeRabbit review comments on PR #936 --- internal/tui/screens/model_picker.go | 8 ++++++-- internal/tui/screens/model_picker_test.go | 5 +++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/tui/screens/model_picker.go b/internal/tui/screens/model_picker.go index 916b9cde7..14468f2d4 100644 --- a/internal/tui/screens/model_picker.go +++ b/internal/tui/screens/model_picker.go @@ -172,8 +172,12 @@ func RRSeparatorRowIdx() int { if len(rr) == 0 { return -1 } - // After orchestrator + "Set all" + SDD phases + JD separator + JD phases - return 2 + len(opencode.SDDPhases()) + 1 + len(opencode.JDPhases()) + // After orchestrator + "Set all" + SDD phases + (optional JD separator + JD phases) + base := 2 + len(opencode.SDDPhases()) + if jd := opencode.JDPhases(); len(jd) > 0 { + base += 1 + len(jd) + } + return base } // ProviderEntries returns sorted provider entries with display names and model counts. diff --git a/internal/tui/screens/model_picker_test.go b/internal/tui/screens/model_picker_test.go index 4ce1f14da..91ca354ad 100644 --- a/internal/tui/screens/model_picker_test.go +++ b/internal/tui/screens/model_picker_test.go @@ -1398,4 +1398,9 @@ func TestModelPickerRowsForProfile_NoRRAgents(t *testing.T) { } } } + for _, row := range rows { + if row == "--- 4R Review ---" { + t.Fatalf("ModelPickerRowsForProfile() should not contain 4R separator; got: %v", rows) + } + } }