test(tools): assert full valid-range output in read_file recovery test (#779 review)

anand + CodeRabbit: TestReadFileToolValidRangeUnchangedByRecovery claimed
byte-for-byte preservation but only checked substrings, so an altered
header/separator or a stray out-of-range line would still pass. Compare the
whole result.Output against the exact expected rendering.

internal/tools/file_tools_test.go

@@ -829,3 +829,61 @@ func TestGlobSkipsWorkspaceExcludedDirectoriesAndBinaryFiles(t *testing.T) {
 		t.Fatalf("expected only keep.txt, got:\n%s", res.Output)
 	}
 }
+
+// A backwards range (end_line < start_line) is a caller-arithmetic slip, not a
+// fatal error: read_file recovers by reading just start_line and surfaces a note
+// explaining the adjustment, matching how an end_line past EOF is auto-clamped.
+// It must never hard-error the way it used to.
+func TestReadFileToolRecoversBackwardsRange(t *testing.T) {
+	root := t.TempDir()
+	writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta\nepsilon")
+
+	result := NewReadFileTool(root).Run(context.Background(), map[string]any{
+		"path":       "notes.txt",
+		"start_line": 3,
+		"end_line":   2, // backwards
+	})
+
+	if result.Status != StatusOK {
+		t.Fatalf("backwards range must recover, not error; got %s: %s", result.Status, result.Output)
+	}
+	if strings.Contains(result.Output, "must be greater than or equal to start_line") {
+		t.Fatalf("backwards range still hard-errors: %q", result.Output)
+	}
+	for _, want := range []string{
+		"end_line 2 was before start_line 3", // the note
+		"only line 3 was read",
+		"3 | gamma",
+	} {
+		if !strings.Contains(result.Output, want) {
+			t.Fatalf("expected %q in output, got %q", want, result.Output)
+		}
+	}
+	// It read exactly start_line, nothing adjacent.
+	if strings.Contains(result.Output, "beta") || strings.Contains(result.Output, "delta") {
+		t.Fatalf("recovery read the wrong lines: %q", result.Output)
+	}
+}
+
+// A valid range with end_line >= start_line is byte-for-byte unchanged: the
+// recovery path must not touch normal reads (no stray note, exact same window).
+func TestReadFileToolValidRangeUnchangedByRecovery(t *testing.T) {
+	root := t.TempDir()
+	writeTestFile(t, filepath.Join(root, "notes.txt"), "alpha\nbeta\ngamma\ndelta\nepsilon")
+
+	result := NewReadFileTool(root).Run(context.Background(), map[string]any{
+		"path":       "notes.txt",
+		"start_line": 2,
+		"end_line":   4,
+	})
+	if result.Status != StatusOK {
+		t.Fatalf("valid range failed: %s: %s", result.Status, result.Output)
+	}
+	// Byte-for-byte: header, blank separator, the exact selected lines, and no
+	// recovery note or trailing content. An altered header/separator or a stray
+	// out-of-range line would fail this where a substring check would not.
+	const want = "File: notes.txt (lines 2-4 of 5)\n\n2 | beta\n3 | gamma\n4 | delta"
+	if result.Output != want {
+		t.Fatalf("valid-range output changed by the recovery path:\n got: %q\nwant: %q", result.Output, want)
+	}
+}

internal/tools/read_file.go

@@ -106,8 +106,18 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
 	if endLine == 0 || endLine > total {
 		endLine = total
 	}
+	// A backwards range (end_line < start_line) is a caller slip, not a fatal
+	// error. Recover the way an end_line past EOF is already recovered a few lines
+	// up (clamped down to the last line): clamp end_line UP to start_line and read
+	// just that one line. Erroring here instead cost the caller a whole retry for
+	// an off-by-arithmetic mistake, and it was inconsistent — start_line past EOF
+	// and end_line past EOF both recover gracefully; only this case hard-failed.
+	// The recovery is surfaced in the output (rangeNote) so it is never silent: the
+	// caller sees it read a different range than it asked for and why.
+	rangeNote := ""
 	if endLine < startLine {
-		return errorResult("Error: Invalid arguments for read_file: end_line must be greater than or equal to start_line")
+		rangeNote = fmt.Sprintf("(note: end_line %d was before start_line %d, so only line %d was read — pass end_line >= start_line to read a wider range)", endLine, startLine, startLine)
+		endLine = startLine
 	}
 
 	truncated := false
@@ -135,7 +145,12 @@ func renderReadFileRange(absolutePath string, relativePath string, total int, st
 		budgetedOutput = newHeadTailOutputBudgetBuilder(readOutputBudgetBytes, "use start_line/end_line or max_lines to continue with a smaller range")
 	}
 	budgetedOutput.WriteString(header)
-	budgetedOutput.WriteString("\n\n")
+	budgetedOutput.WriteString("\n")
+	if rangeNote != "" {
+		budgetedOutput.WriteString(rangeNote)
+		budgetedOutput.WriteString("\n")
+	}
+	budgetedOutput.WriteString("\n")
 	if err := appendReadFileRange(budgetedOutput, absolutePath, startLine, selectedLines, width); err != nil {
 		return errorResult("Error reading file " + relativePath + ": " + err.Error())
 	}

internal/tui/provider_wizard.go

@@ -1959,49 +1959,12 @@ func (wizard *providerWizardState) filteredModels() []providerWizardModel {
 	return models
 }
 
-// A model Description is only a good row label when it reads like a display
-// NAME — "Grok 4.3" for x-ai/grok-4.3 — which is what the field is for, and why
-// the row prefers it over an ugly raw ID.
-//
-// Providers do not all honour that. Aggregators commonly return a prose blurb
-// instead ("Fast DeepSeek model for efficient chat, coding help, and agent
-// loops"), which fails as a label twice over: it overflows the row, and because
-// such blurbs are written per model FAMILY rather than per model, several
-// DIFFERENT models collapse to byte-identical rows. A picker then offers two
-// visually identical entries with the distinguishing ID visible only in the
-// detail line of whichever one is highlighted.
-//
-// providerWizardGenericModelDescription cannot catch these: it rejects
-// descriptions that are vague ("live model", "catalog default"), whereas these
-// are specific — just shared and sentence-shaped. So the test is shape, not
-// wording: a name is short and few-worded; anything longer is prose, and the ID
-// (unique by construction) makes the better label.
-const (
-	providerWizardMaxModelNameChars = 28
-	providerWizardMaxModelNameWords = 4
-)
-
-func providerWizardDescriptionIsDisplayName(description string) bool {
-	trimmed := strings.TrimSpace(description)
-	if trimmed == "" || providerWizardGenericModelDescription(trimmed) {
-		return false
-	}
-	if len(trimmed) > providerWizardMaxModelNameChars {
-		return false
-	}
-	return len(strings.Fields(trimmed)) <= providerWizardMaxModelNameWords
-}
-
 func (model providerWizardModel) displayLabel() string {
-	if providerWizardDescriptionIsDisplayName(model.Description) {
-		return strings.TrimSpace(model.Description)
+	description := strings.TrimSpace(model.Description)
+	if description != "" && !providerWizardGenericModelDescription(description) {
+		return description
 	}
-	if id := strings.TrimSpace(model.ID); id != "" {
-		return id
-	}
-	// ID-less sentinel rows built in currentModel ("model name required",
-	// "no matching models") would otherwise render blank.
-	return strings.TrimSpace(model.Description)
+	return model.ID
 }
 
 func (model providerWizardModel) secondaryText() string {

internal/tui/provider_wizard_test.go

@@ -1787,37 +1787,3 @@ func providerWizardIDs(descriptors []providercatalog.Descriptor) []string {
 	}
 	return ids
 }
-
-// A provider that returns prose blurbs instead of display names must not
-// collapse distinct models into identical rows. Reproduces the reported picker
-// where two "Open-weight GPT model..." and two "Kimi multimodal agent model..."
-// rows were indistinguishable, with the ID visible only for the highlighted one.
-func TestProviderWizardModelRowsStayDistinctWithProseDescriptions(t *testing.T) {
-	wizard := &providerWizardState{
-		step:        providerWizardStepModel,
-		modelSource: "models.dev",
-		models: []providerWizardModel{
-			{ID: "gpt-oss-120b", Description: "Open-weight GPT model for self-hosted reasoning and instruction-following workloads"},
-			{ID: "gpt-oss-20b", Description: "Open-weight GPT model for self-hosted reasoning and instruction-following workloads"},
-			{ID: "kimi-k2-thinking", Description: "Kimi multimodal agent model for visual understanding, coding, and planning"},
-			{ID: "kimi-k2-instruct", Description: "Kimi multimodal agent model for visual understanding, coding, and planning"},
-			{ID: "x-ai/grok-4.3", Description: "Grok 4.3"},
-		},
-	}
-	view := plainRender(t, strings.Join(wizard.renderModelStep(96), "\n"))
-
-	// every prose-described model is identified by its unique ID
-	for _, id := range []string{"gpt-oss-120b", "gpt-oss-20b", "kimi-k2-thinking", "kimi-k2-instruct"} {
-		if !strings.Contains(view, id) {
-			t.Errorf("model %q is not identifiable in the list:\n%s", id, view)
-		}
-	}
-	// a genuine display name is still preferred over the raw id
-	if !strings.Contains(view, "Grok 4.3") {
-		t.Errorf("friendly display name was lost:\n%s", view)
-	}
-	// the prose blurb must not be a row label
-	if strings.Contains(view, "❯ Open-weight GPT model") {
-		t.Errorf("prose blurb still used as a row label:\n%s", view)
-	}
-}

internal/tui/sidebar.go

@@ -94,26 +94,12 @@ func (m model) sidebarAvailable() bool {
 	if widthTier(m.width) < tierMedium {
 		return false
 	}
-	// Full-screen overlays (setup, wizards, pickers) take over the chat column and
-	// render at full width; suppress the second column while any is active so
-	// their geometry and mouse hit-testing stay full-width as before.
-	//
-	// The `/` command palette is deliberately NOT in this list. It is not a
-	// full-screen overlay: suggestionOverlay draws it via centerRenderedBlock at
-	// minInt(width, suggestionPaletteMaxWidth) — a centred box capped at 76 cells
-	// — floating over the chat column rather than replacing it. Suppressing the
-	// sidebar for it collapsed the whole layout on every keystroke of `/`: the
-	// plan panel dropped out of the sidebar and re-rendered inline at the bottom
-	// and the transcript re-wrapped to full width, which is at its most jarring
-	// mid-run with a live plan on screen. Since the sidebar already requires the
-	// medium tier (>= 80 cells) the palette is always strictly narrower than the
-	// chat column here, so the two never contend for the same cells.
-	//
-	// Mouse hit-testing is unaffected: sidebarLineAtMouse carries its own
-	// suggestionsActive() guard, so clicks still go to the palette and not to the
-	// sidebar rows underneath it.
+	// Full-screen overlays (setup, wizards, pickers, the empty-state suggestion
+	// list) take over the chat column and render at full width; suppress the
+	// second column while any is active so their geometry and mouse hit-testing
+	// stay full-width as before.
 	if m.setup.visible || m.helpOverlay || m.leaderHelpOverlay || m.providerWizard != nil || m.mcpAddWizard != nil ||
-		m.mcpManager != nil || m.picker != nil {
+		m.mcpManager != nil || m.picker != nil || m.suggestionsActive() {
 		return false
 	}
 	// Home/welcome screen: stay single-column until there's real conversation, so

internal/tui/sidebar_test.go

@@ -631,58 +631,3 @@ func TestTwoColumnTranscriptViewWidth(t *testing.T) {
 func stripSidebar(lines []string) string {
 	return ansiPattern.ReplaceAllString(strings.Join(lines, "\n"), "")
 }
-
-// The `/` command palette must NOT collapse the sidebar. It is a centred box
-// capped at suggestionPaletteMaxWidth floating over the chat column, not a
-// full-screen overlay — suppressing the second column for it dropped the plan
-// out of the sidebar and re-rendered it inline at the bottom on every `/`,
-// which is at its most disruptive mid-run with a live plan on screen. The
-// genuinely full-width overlays must still suppress it.
-func TestSidebarSurvivesCommandPalette(t *testing.T) {
-	base := func() model {
-		m := runningPlanModel(t, 3)
-		m.altScreen = true
-		m.height = 40
-		m.headerPrinted = true
-		m.transcript = append(m.transcript, transcriptRow{kind: rowToolCall, tool: "read_file", detail: "main.go"})
-		return m
-	}
-
-	m := base()
-	if !m.sidebarActive() {
-		t.Fatal("precondition: sidebar should be active for a wide alt-screen model with a plan")
-	}
-
-	// `/` palette open: sidebar stays, so the plan keeps its home and the layout
-	// does not reflow.
-	m.suggestions = []commandSuggestion{{Name: "/model", Desc: "Pick a model."}, {Name: "/plan", Desc: "Show planning mode status."}}
-	if !m.suggestionsActive() {
-		t.Fatal("precondition: suggestions should be active")
-	}
-	if !m.sidebarActive() {
-		t.Error("command palette must not collapse the sidebar")
-	}
-	if got := m.renderPinnedPlanPanel(m.chatColumnWidth(), 10); got != "" {
-		t.Errorf("plan must stay in the sidebar, not fall back to the pinned panel:\n%s", got)
-	}
-	// The palette never contends for the sidebar's cells: the two-column path
-	// renders it at width = chatColumnWidth, so it is centred inside the chat
-	// column and cannot overlap the sidebar.
-	chatW := m.chatColumnWidth()
-	for _, line := range strings.Split(plainRender(t, m.suggestionOverlay(chatW)), "\n") {
-		if w := lipgloss.Width(line); w > chatW {
-			t.Errorf("palette line is %d wide, wider than the chat column %d — it would overlap the sidebar", w, chatW)
-		}
-	}
-	// Clicks still belong to the palette, not the sidebar rows beneath it.
-	if _, ok := m.sidebarLineAtMouse(tea.MouseClickMsg{Button: tea.MouseLeft}); ok {
-		t.Error("sidebar must not take mouse hits while the palette is open")
-	}
-
-	// A genuinely full-width overlay still suppresses the sidebar.
-	full := base()
-	full.picker = &commandPicker{}
-	if full.sidebarActive() {
-		t.Error("a full-screen picker must still collapse the sidebar")
-	}
-}