test: improve coverage for cmd/opencodereview package from 42% to 70%

Add comprehensive unit tests covering config dispatch, emit run result,
git helpers, provider commands, provider TUI pure functions and View
rendering, shared utilities, flags parsing, scan command flags, and
small file entry points (version, viewer, llm, rules commands).

cmd/opencodereview/config_cmd_test.go

@@ -1,6 +1,7 @@
 package main
 
 import (
+	"os"
 	"testing"
 )
 
@@ -417,6 +418,379 @@ func TestUnsetInvalidKey(t *testing.T) {
 	}
 }
 
+func TestMergeModelLists(t *testing.T) {
+	tests := []struct {
+		name  string
+		lists [][]string
+		want  []string
+	}{
+		{"empty", nil, nil},
+		{"single list", [][]string{{"a", "b"}}, []string{"a", "b"}},
+		{"merge with dedup", [][]string{{"a", "b"}, {"b", "c"}}, []string{"a", "b", "c"}},
+		{"three lists", [][]string{{"x"}, {"y"}, {"x", "z"}}, []string{"x", "y", "z"}},
+		{"empty strings filtered", [][]string{{"a", "", "b"}}, []string{"a", "b"}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := mergeModelLists(tc.lists...)
+			if len(got) != len(tc.want) {
+				t.Fatalf("mergeModelLists() = %v, want %v", got, tc.want)
+			}
+			for i := range tc.want {
+				if got[i] != tc.want[i] {
+					t.Errorf("[%d] = %q, want %q", i, got[i], tc.want[i])
+				}
+			}
+		})
+	}
+}
+
+func TestEnsureTelemetry(t *testing.T) {
+	cfg := &Config{}
+	if cfg.Telemetry != nil {
+		t.Fatal("Telemetry should be nil initially")
+	}
+	cfg.ensureTelemetry()
+	if cfg.Telemetry == nil {
+		t.Fatal("Telemetry should be non-nil after ensureTelemetry()")
+	}
+	cfg.ensureTelemetry()
+	if cfg.Telemetry == nil {
+		t.Fatal("Telemetry should remain non-nil on second call")
+	}
+}
+
+func TestSetConfigValueLlmURL(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.url", "https://example.com/v1"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Llm.URL != "https://example.com/v1" {
+		t.Errorf("URL = %q", cfg.Llm.URL)
+	}
+}
+
+func TestSetConfigValueLlmAuthToken(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.auth_token", "tok-123"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Llm.AuthToken != "tok-123" {
+		t.Errorf("AuthToken = %q", cfg.Llm.AuthToken)
+	}
+}
+
+func TestSetConfigValueLlmModel(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.model", "my-model"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Llm.Model != "my-model" {
+		t.Errorf("Model = %q", cfg.Llm.Model)
+	}
+}
+
+func TestSetConfigValueLlmUseAnthropic(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.use_anthropic", "false"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Llm.UseAnthropic == nil || *cfg.Llm.UseAnthropic != false {
+		t.Errorf("UseAnthropic = %v", cfg.Llm.UseAnthropic)
+	}
+}
+
+func TestSetConfigValueLlmUseAnthropicInvalid(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.use_anthropic", "notbool"); err == nil {
+		t.Fatal("expected error for invalid boolean")
+	}
+}
+
+func TestSetConfigValueLanguage(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "language", "English"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Language != "English" {
+		t.Errorf("Language = %q", cfg.Language)
+	}
+}
+
+func TestSetConfigValueTelemetryEnabled(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "telemetry.enabled", "true"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Telemetry == nil || !cfg.Telemetry.Enabled {
+		t.Error("Telemetry.Enabled should be true")
+	}
+}
+
+func TestSetConfigValueTelemetryEnabledInvalid(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "telemetry.enabled", "notbool"); err == nil {
+		t.Fatal("expected error for invalid boolean")
+	}
+}
+
+func TestSetConfigValueTelemetryExporter(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "telemetry.exporter", "otlp"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Telemetry.Exporter != "otlp" {
+		t.Errorf("Exporter = %q", cfg.Telemetry.Exporter)
+	}
+}
+
+func TestSetConfigValueTelemetryOTLPEndpoint(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "telemetry.otlp_endpoint", "localhost:4317"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Telemetry.OTLPEndpoint != "localhost:4317" {
+		t.Errorf("OTLPEndpoint = %q", cfg.Telemetry.OTLPEndpoint)
+	}
+}
+
+func TestSetConfigValueTelemetryContentLogging(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "telemetry.content_logging", "true"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if !cfg.Telemetry.ContentLog {
+		t.Error("ContentLog should be true")
+	}
+}
+
+func TestSetConfigValueTelemetryContentLoggingInvalid(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "telemetry.content_logging", "notbool"); err == nil {
+		t.Fatal("expected error for invalid boolean")
+	}
+}
+
+func TestSetConfigValueLlmExtraBody(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.extra_body", `{"key":"val"}`); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Llm.ExtraBody == nil {
+		t.Fatal("ExtraBody should not be nil")
+	}
+	if cfg.Llm.ExtraBody["key"] != "val" {
+		t.Errorf("ExtraBody[\"key\"] = %v", cfg.Llm.ExtraBody["key"])
+	}
+}
+
+func TestSetConfigValueLlmExtraBodyInvalid(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "llm.extra_body", "not-json"); err == nil {
+		t.Fatal("expected error for invalid JSON")
+	}
+}
+
+func TestSetConfigValueUnknownKey(t *testing.T) {
+	cfg := &Config{}
+	if err := setConfigValue(cfg, "unknown.key", "val"); err == nil {
+		t.Fatal("expected error for unknown key")
+	}
+}
+
+func TestSetConfigValueProviderClearsModel(t *testing.T) {
+	cfg := &Config{Provider: "old-provider", Model: "old-model"}
+	if err := setConfigValue(cfg, "provider", "new-provider"); err != nil {
+		t.Fatalf("setConfigValue: %v", err)
+	}
+	if cfg.Model != "" {
+		t.Errorf("Model should be cleared on provider change, got %q", cfg.Model)
+	}
+}
+
+func TestRunConfigUnset_InvalidKey(t *testing.T) {
+	if err := runConfigUnset("provider"); err == nil {
+		t.Fatal("expected error for non custom_providers key")
+	}
+	if err := runConfigUnset("custom_providers."); err == nil {
+		t.Fatal("expected error for empty provider name")
+	}
+}
+
+func TestRunConfig_EmptyArgs(t *testing.T) {
+	err := runConfig(nil)
+	if err != nil {
+		t.Fatalf("runConfig with nil args should print usage, got error: %v", err)
+	}
+}
+
+func TestRunConfig_ProviderWithArgs(t *testing.T) {
+	err := runConfig([]string{"provider", "extra"})
+	if err == nil {
+		t.Fatal("expected error when provider has args")
+	}
+}
+
+func TestRunConfig_ModelWithArgs(t *testing.T) {
+	err := runConfig([]string{"model", "extra"})
+	if err == nil {
+		t.Fatal("expected error when model has args")
+	}
+}
+
+func TestDeleteCustomProvider_NotFound(t *testing.T) {
+	cfg := &Config{}
+	_, err := deleteCustomProvider(cfg, "nonexistent")
+	if err == nil {
+		t.Fatal("expected error for nil CustomProviders")
+	}
+
+	cfg.CustomProviders = map[string]ProviderEntry{"other": {}}
+	_, err = deleteCustomProvider(cfg, "nonexistent")
+	if err == nil {
+		t.Fatal("expected error for missing provider")
+	}
+}
+
+func TestActiveModelForProvider(t *testing.T) {
+	tests := []struct {
+		name     string
+		cfg      *Config
+		provider string
+		entry    ProviderEntry
+		want     string
+	}{
+		{"entry model", nil, "p", ProviderEntry{Model: "m1"}, "m1"},
+		{"cfg model", &Config{Provider: "p", Model: "m2"}, "p", ProviderEntry{}, "m2"},
+		{"entry takes precedence", &Config{Provider: "p", Model: "m2"}, "p", ProviderEntry{Model: "m1"}, "m1"},
+		{"different provider", &Config{Provider: "other", Model: "m2"}, "p", ProviderEntry{}, ""},
+		{"no model", &Config{Provider: "p"}, "p", ProviderEntry{}, ""},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := activeModelForProvider(tc.cfg, tc.provider, tc.entry)
+			if got != tc.want {
+				t.Errorf("got %q, want %q", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestNormalizeModelList(t *testing.T) {
+	tests := []struct {
+		name   string
+		models []string
+		want   []string
+	}{
+		{"dedup", []string{"a", "b", "a"}, []string{"a", "b"}},
+		{"trim spaces", []string{" a ", " b "}, []string{"a", "b"}},
+		{"filter empty", []string{"a", "", "b"}, []string{"a", "b"}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := normalizeModelList(tc.models)
+			if len(got) != len(tc.want) {
+				t.Fatalf("got %v, want %v", got, tc.want)
+			}
+			for i := range tc.want {
+				if got[i] != tc.want[i] {
+					t.Errorf("[%d] = %q, want %q", i, got[i], tc.want[i])
+				}
+			}
+		})
+	}
+}
+
+func TestParseModelListValue(t *testing.T) {
+	tests := []struct {
+		name  string
+		value string
+		want  int
+	}{
+		{"empty", "", 0},
+		{"json array", `["a","b"]`, 2},
+		{"comma separated", "a,b,c", 3},
+		{"bracket unquoted", "[a,b]", 2},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, err := parseModelListValue(tc.value)
+			if err != nil {
+				t.Fatalf("parseModelListValue: %v", err)
+			}
+			if len(got) != tc.want {
+				t.Errorf("got %d models, want %d: %v", len(got), tc.want, got)
+			}
+		})
+	}
+}
+
+func TestResolveConfigPath_Default(t *testing.T) {
+	t.Setenv("OCR_CONFIG_PATH", "")
+	p, err := resolveConfigPath()
+	if err != nil {
+		t.Fatalf("resolveConfigPath: %v", err)
+	}
+	if p == "" {
+		t.Fatal("expected non-empty default config path")
+	}
+}
+
+func TestResolveConfigPath_Env(t *testing.T) {
+	t.Setenv("OCR_CONFIG_PATH", "/tmp/test-config.json")
+	p, err := resolveConfigPath()
+	if err != nil {
+		t.Fatalf("resolveConfigPath: %v", err)
+	}
+	if p != "/tmp/test-config.json" {
+		t.Errorf("path = %q, want /tmp/test-config.json", p)
+	}
+}
+
+func TestLoadOrCreateConfig_NewFile(t *testing.T) {
+	cfg, err := loadOrCreateConfig(t.TempDir() + "/nonexistent.json")
+	if err != nil {
+		t.Fatalf("loadOrCreateConfig: %v", err)
+	}
+	if cfg == nil {
+		t.Fatal("expected non-nil config")
+	}
+}
+
+func TestLoadOrCreateConfig_InvalidJSON(t *testing.T) {
+	dir := t.TempDir()
+	path := dir + "/bad.json"
+	if err := os.WriteFile(path, []byte("{invalid"), 0644); err != nil {
+		t.Fatal(err)
+	}
+	_, err := loadOrCreateConfig(path)
+	if err == nil {
+		t.Fatal("expected error for invalid JSON")
+	}
+}
+
+func TestLoadAppConfig_NotExist(t *testing.T) {
+	cfg, err := LoadAppConfig(t.TempDir() + "/none.json")
+	if err != nil {
+		t.Fatalf("LoadAppConfig: %v", err)
+	}
+	if cfg != nil {
+		t.Fatal("expected nil config for non-existent file")
+	}
+}
+
+func TestLoadAppConfig_InvalidJSON(t *testing.T) {
+	dir := t.TempDir()
+	path := dir + "/bad.json"
+	if err := os.WriteFile(path, []byte("not json"), 0644); err != nil {
+		t.Fatal(err)
+	}
+	_, err := LoadAppConfig(path)
+	if err == nil {
+		t.Fatal("expected error for invalid JSON")
+	}
+}
+
 func TestEnsureModelInList(t *testing.T) {
 	models := []string{"test-model", "test-model-2", "bbb", "aaa", "test-model-3"}
 

cmd/opencodereview/config_dispatch_test.go

@@ -0,0 +1,30 @@
+package main
+
+import (
+	"strings"
+	"testing"
+)
+
+func TestRunConfig_UnknownSubcommand(t *testing.T) {
+	err := runConfig([]string{"delete", "foo"})
+	if err == nil {
+		t.Fatal("expected error for unknown subcommand")
+	}
+	if !strings.Contains(err.Error(), "unknown") {
+		t.Errorf("error = %q, expected to contain 'unknown'", err.Error())
+	}
+}
+
+func TestRunConfig_InvalidSetMissingValue(t *testing.T) {
+	err := runConfig([]string{"set", "provider"})
+	if err == nil {
+		t.Fatal("expected error for set without value")
+	}
+}
+
+func TestRunConfig_InvalidUnsetMissingKey(t *testing.T) {
+	err := runConfig([]string{"unset"})
+	if err == nil {
+		t.Fatal("expected error for unset without key")
+	}
+}

cmd/opencodereview/emit_run_result_test.go

@@ -0,0 +1,177 @@
+package main
+
+import (
+	"context"
+	"encoding/json"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/open-code-review/open-code-review/internal/agent"
+	"github.com/open-code-review/open-code-review/internal/model"
+)
+
+type mockResultProvider struct {
+	diffs            []model.Diff
+	filesReviewed    int64
+	inputTokens      int64
+	outputTokens     int64
+	totalTokens      int64
+	cacheReadTokens  int64
+	cacheWriteTokens int64
+	warnings         []agent.AgentWarning
+	projectSummary   string
+	toolCalls        map[string]int64
+}
+
+func (m *mockResultProvider) Diffs() []model.Diff            { return m.diffs }
+func (m *mockResultProvider) FilesReviewed() int64           { return m.filesReviewed }
+func (m *mockResultProvider) TotalInputTokens() int64        { return m.inputTokens }
+func (m *mockResultProvider) TotalOutputTokens() int64       { return m.outputTokens }
+func (m *mockResultProvider) TotalTokensUsed() int64         { return m.totalTokens }
+func (m *mockResultProvider) TotalCacheReadTokens() int64    { return m.cacheReadTokens }
+func (m *mockResultProvider) TotalCacheWriteTokens() int64   { return m.cacheWriteTokens }
+func (m *mockResultProvider) Warnings() []agent.AgentWarning { return m.warnings }
+func (m *mockResultProvider) ProjectSummary() string         { return m.projectSummary }
+func (m *mockResultProvider) ToolCalls() map[string]int64    { return m.toolCalls }
+
+func TestEmitRunResult_JSONNoFiles(t *testing.T) {
+	ag := &mockResultProvider{filesReviewed: 0}
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "developer", nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	var out jsonOutput
+	if err := json.Unmarshal([]byte(got), &out); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	if out.Status != "skipped" {
+		t.Errorf("status = %q, want skipped", out.Status)
+	}
+}
+
+func TestEmitRunResult_JSONWithComments(t *testing.T) {
+	ag := &mockResultProvider{
+		filesReviewed: 3,
+		inputTokens:   100,
+		outputTokens:  50,
+		totalTokens:   150,
+		warnings:      []agent.AgentWarning{{Type: "info", Message: "note"}},
+		toolCalls:     map[string]int64{"file_read": 2},
+	}
+	comments := []model.LlmComment{{Path: "main.go", Content: "fix", StartLine: 1, EndLine: 2}}
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, comments, time.Now(), "json", "developer", nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	var out jsonOutput
+	if err := json.Unmarshal([]byte(got), &out); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	if len(out.Comments) != 1 {
+		t.Errorf("expected 1 comment, got %d", len(out.Comments))
+	}
+	if out.Summary == nil || out.Summary.FilesReviewed != 3 {
+		t.Errorf("summary.FilesReviewed = %v", out.Summary)
+	}
+}
+
+func TestEmitRunResult_TextNoComments(t *testing.T) {
+	ag := &mockResultProvider{filesReviewed: 2}
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "Looks good to me") {
+		t.Errorf("expected 'Looks good to me', got %q", got)
+	}
+}
+
+func TestEmitRunResult_TextWithComments(t *testing.T) {
+	ag := &mockResultProvider{filesReviewed: 1}
+	comments := []model.LlmComment{{Path: "a.go", Content: "rename", StartLine: 5, EndLine: 10}}
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, comments, time.Now(), "text", "developer", nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "a.go") {
+		t.Errorf("expected path, got %q", got)
+	}
+	if !strings.Contains(got, "rename") {
+		t.Errorf("expected comment content, got %q", got)
+	}
+}
+
+func TestEmitRunResult_TextWithProjectSummary(t *testing.T) {
+	ag := &mockResultProvider{
+		filesReviewed:  5,
+		projectSummary: "All tests pass, code quality is good.",
+	}
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "developer", nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "Project Summary") {
+		t.Errorf("expected 'Project Summary', got %q", got)
+	}
+	if !strings.Contains(got, "All tests pass") {
+		t.Errorf("expected summary content, got %q", got)
+	}
+}
+
+func TestEmitRunResult_AgentTextRestoresQuiet(t *testing.T) {
+	ag := &mockResultProvider{filesReviewed: 1}
+	q := newQuietHandle("text", "agent")
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", q)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if q.fn != nil {
+		t.Error("expected quiet handle to be restored")
+	}
+	_ = got
+}
+
+func TestEmitRunResult_AgentJSONDoesNotRestore(t *testing.T) {
+	ag := &mockResultProvider{
+		filesReviewed: 1,
+		inputTokens:   10,
+		outputTokens:  5,
+		totalTokens:   15,
+	}
+	q := newQuietHandle("json", "agent")
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, nil, time.Now(), "json", "agent", q)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	var out jsonOutput
+	if err := json.Unmarshal([]byte(got), &out); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	q.Restore()
+}
+
+func TestEmitRunResult_NilQuietHandle(t *testing.T) {
+	ag := &mockResultProvider{filesReviewed: 1}
+	got := captureStdout(t, func() {
+		err := emitRunResult(context.Background(), ag, nil, time.Now(), "text", "agent", nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	_ = got
+}

cmd/opencodereview/flags_test.go

@@ -1,6 +1,9 @@
 package main
 
-import "testing"
+import (
+	"testing"
+	"time"
+)
 
 func TestParseReviewFlagsModelOverride(t *testing.T) {
 	opts, err := parseReviewFlags([]string{"--model", "claude-opus-4-6"})
@@ -18,3 +21,174 @@ func TestParseReviewFlagsModelOverride(t *testing.T) {
 		t.Errorf("audience = %q, want %q", opts.audience, "human")
 	}
 }
+
+func TestParseReviewFlags_InvalidAudience(t *testing.T) {
+	_, err := parseReviewFlags([]string{"--audience", "robot"})
+	if err == nil {
+		t.Fatal("expected error for invalid audience")
+	}
+}
+
+func TestParseReviewFlags_NegativeMaxTools(t *testing.T) {
+	_, err := parseReviewFlags([]string{"--max-tools", "-1"})
+	if err == nil {
+		t.Fatal("expected error for negative max-tools")
+	}
+}
+
+func TestParseReviewFlags_MaxToolsBelowMin(t *testing.T) {
+	opts, err := parseReviewFlags([]string{"--max-tools", "5"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.maxTools != 10 {
+		t.Errorf("maxTools = %d, want 10 (clamped to min)", opts.maxTools)
+	}
+}
+
+func TestParseReviewFlags_NegativeMaxGitProcs(t *testing.T) {
+	_, err := parseReviewFlags([]string{"--max-git-procs", "-1"})
+	if err == nil {
+		t.Fatal("expected error for negative max-git-procs")
+	}
+}
+
+func TestParseReviewFlags_ConflictingModes(t *testing.T) {
+	_, err := parseReviewFlags([]string{"--from", "main", "--to", "dev", "--commit", "abc"})
+	if err == nil {
+		t.Fatal("expected error for conflicting modes")
+	}
+}
+
+func TestParseReviewFlags_FromWithoutTo(t *testing.T) {
+	_, err := parseReviewFlags([]string{"--from", "main"})
+	if err == nil {
+		t.Fatal("expected error for --from without --to")
+	}
+}
+
+func TestParseReviewFlags_ToWithoutFrom(t *testing.T) {
+	_, err := parseReviewFlags([]string{"--to", "dev"})
+	if err == nil {
+		t.Fatal("expected error for --to without --from")
+	}
+}
+
+func TestParseReviewFlags_Help(t *testing.T) {
+	opts, err := parseReviewFlags([]string{"-h"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !opts.showHelp {
+		t.Error("expected showHelp=true")
+	}
+}
+
+func TestParseReviewFlags_ShortFlags(t *testing.T) {
+	opts, err := parseReviewFlags([]string{"-c", "abc123", "-f", "json", "-p"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.commit != "abc123" {
+		t.Errorf("commit = %q, want abc123", opts.commit)
+	}
+	if opts.outputFormat != "json" {
+		t.Errorf("outputFormat = %q, want json", opts.outputFormat)
+	}
+	if !opts.preview {
+		t.Error("expected preview=true")
+	}
+}
+
+func TestParseConfigArgs_Empty(t *testing.T) {
+	_, err := parseConfigArgs(nil)
+	if err == nil {
+		t.Fatal("expected error for empty args")
+	}
+}
+
+func TestParseConfigArgs_Set(t *testing.T) {
+	act, err := parseConfigArgs([]string{"set", "llm.model", "gpt-4"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if act.subCmd != "set" || act.key != "llm.model" || act.value != "gpt-4" {
+		t.Errorf("got %+v", act)
+	}
+}
+
+func TestParseConfigArgs_SetMissingValue(t *testing.T) {
+	_, err := parseConfigArgs([]string{"set", "llm.model"})
+	if err == nil {
+		t.Fatal("expected error for missing value")
+	}
+}
+
+func TestParseConfigArgs_Unset(t *testing.T) {
+	act, err := parseConfigArgs([]string{"unset", "custom_providers.foo"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if act.subCmd != "unset" || act.key != "custom_providers.foo" {
+		t.Errorf("got %+v", act)
+	}
+}
+
+func TestParseConfigArgs_UnsetMissingKey(t *testing.T) {
+	_, err := parseConfigArgs([]string{"unset"})
+	if err == nil {
+		t.Fatal("expected error for missing key")
+	}
+}
+
+func TestParseConfigArgs_UnknownSubCmd(t *testing.T) {
+	_, err := parseConfigArgs([]string{"delete", "foo"})
+	if err == nil {
+		t.Fatal("expected error for unknown subcommand")
+	}
+}
+
+func TestDurationVar(t *testing.T) {
+	fs := newOcrFlagSet("test")
+	var d time.Duration
+	fs.DurationVar(&d, "timeout", 5*time.Second, "max duration")
+	if err := fs.Parse([]string{"--timeout", "10s"}); err != nil {
+		t.Fatalf("parse: %v", err)
+	}
+	if d != 10*time.Second {
+		t.Errorf("d = %v, want 10s", d)
+	}
+}
+
+func TestPrintDefaults(t *testing.T) {
+	fs := newOcrFlagSet("test")
+	var s string
+	fs.StringVar(&s, "name", "default", "a name")
+	fs.PrintDefaults()
+}
+
+func TestExpandShortFlags(t *testing.T) {
+	m := map[string]string{"c": "commit", "f": "format"}
+	tests := []struct {
+		name string
+		args []string
+		want []string
+	}{
+		{"expands short", []string{"-c", "abc"}, []string{"--commit", "abc"}},
+		{"keeps long", []string{"--format", "json"}, []string{"--format", "json"}},
+		{"unknown short kept", []string{"-x", "val"}, []string{"-x", "val"}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := expandShortFlags(tc.args, m)
+			if len(got) != len(tc.want) {
+				t.Fatalf("got %v, want %v", got, tc.want)
+			}
+			for i := range tc.want {
+				if got[i] != tc.want[i] {
+					t.Errorf("[%d] = %q, want %q", i, got[i], tc.want[i])
+				}
+			}
+		})
+	}
+}

cmd/opencodereview/git_test.go

@@ -0,0 +1,153 @@
+package main
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"strings"
+	"testing"
+)
+
+func initTestGitRepo(t *testing.T) string {
+	t.Helper()
+	dir := t.TempDir()
+	cmds := [][]string{
+		{"git", "-C", dir, "init"},
+		{"git", "-C", dir, "config", "user.email", "test@test.com"},
+		{"git", "-C", dir, "config", "user.name", "Test"},
+	}
+	for _, args := range cmds {
+		cmd := exec.Command(args[0], args[1:]...)
+		if out, err := cmd.CombinedOutput(); err != nil {
+			t.Fatalf("git init: %v: %s", err, out)
+		}
+	}
+	f := filepath.Join(dir, "README.md")
+	os.WriteFile(f, []byte("hello"), 0o644)
+	cmd := exec.Command("git", "-C", dir, "add", ".")
+	cmd.CombinedOutput()
+	cmd = exec.Command("git", "-C", dir, "commit", "-m", "initial commit")
+	cmd.CombinedOutput()
+	return dir
+}
+
+func TestRunGitCmd_Success(t *testing.T) {
+	dir := initTestGitRepo(t)
+	out, err := runGitCmd(dir, "rev-parse", "--git-dir")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if len(out) == 0 {
+		t.Error("expected non-empty output")
+	}
+}
+
+func TestRunGitCmd_Failure(t *testing.T) {
+	dir := t.TempDir()
+	_, err := runGitCmd(dir, "rev-parse", "--git-dir")
+	if err == nil {
+		t.Error("expected error for non-git dir")
+	}
+}
+
+func TestGetCommitMessage(t *testing.T) {
+	dir := initTestGitRepo(t)
+	msg, err := getCommitMessage(dir, "HEAD")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if msg != "initial commit" {
+		t.Errorf("msg = %q, want 'initial commit'", msg)
+	}
+}
+
+func TestGetCommitMessage_InvalidCommit(t *testing.T) {
+	dir := initTestGitRepo(t)
+	_, err := getCommitMessage(dir, "nonexistent-ref-xyz")
+	if err == nil {
+		t.Fatal("expected error for invalid commit")
+	}
+}
+
+func TestResolveRepoDir_ValidGitRepo(t *testing.T) {
+	dir := initTestGitRepo(t)
+	resolved, err := resolveRepoDir(dir)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if resolved == "" {
+		t.Error("expected non-empty resolved path")
+	}
+}
+
+func TestResolveRepoDir_NotGitRepo(t *testing.T) {
+	dir := t.TempDir()
+	_, err := resolveRepoDir(dir)
+	if err == nil {
+		t.Fatal("expected error for non-git dir")
+	}
+	if !strings.Contains(err.Error(), "not a git repository") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+func TestResolveRepoDir_EmptyUsesWd(t *testing.T) {
+	dir := initTestGitRepo(t)
+	origDir, _ := os.Getwd()
+	defer os.Chdir(origDir)
+	os.Chdir(dir)
+
+	resolved, err := resolveRepoDir("")
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if resolved == "" {
+		t.Error("expected non-empty resolved path")
+	}
+}
+
+func TestRequireGitRepo_Valid(t *testing.T) {
+	dir := initTestGitRepo(t)
+	if err := requireGitRepo(dir); err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestRequireGitRepo_Invalid(t *testing.T) {
+	dir := t.TempDir()
+	err := requireGitRepo(dir)
+	if err == nil {
+		t.Fatal("expected error for non-git dir")
+	}
+}
+
+func TestValidateReviewRefs_ValidCommit(t *testing.T) {
+	dir := initTestGitRepo(t)
+	err := validateReviewRefs(dir, reviewOptions{commit: "HEAD"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestValidateReviewRefs_InvalidCommit(t *testing.T) {
+	dir := initTestGitRepo(t)
+	err := validateReviewRefs(dir, reviewOptions{commit: "nonexistent-ref-xyz"})
+	if err == nil {
+		t.Fatal("expected error for invalid commit ref")
+	}
+}
+
+func TestValidateReviewRefs_EmptySkipped(t *testing.T) {
+	dir := initTestGitRepo(t)
+	err := validateReviewRefs(dir, reviewOptions{})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+}
+
+func TestBuildToolRegistry(t *testing.T) {
+	reg := buildToolRegistry(nil, nil)
+	if reg == nil {
+		t.Fatal("expected non-nil registry")
+	}
+}

cmd/opencodereview/output_helpers_test.go

@@ -162,6 +162,34 @@ func TestBuildDiffLines(t *testing.T) {
 	})
 }
 
+func TestOutputJSONWithWarnings_NoCommentsSubtaskError(t *testing.T) {
+	old := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	warnings := []agent.AgentWarning{{Type: "subtask_error", File: "x.go", Message: "fail"}}
+	err := outputJSONWithWarnings(nil, warnings, 1, 10, 5, 15, 0, 0, time.Second, "", nil)
+
+	w.Close()
+	os.Stdout = old
+
+	if err != nil {
+		t.Fatalf("error: %v", err)
+	}
+
+	var buf bytes.Buffer
+	buf.ReadFrom(r)
+
+	var out jsonOutput
+	json.Unmarshal(buf.Bytes(), &out)
+	if out.Status != "completed_with_errors" {
+		t.Errorf("status = %q, want completed_with_errors", out.Status)
+	}
+	if !strings.Contains(out.Message, "errors") {
+		t.Errorf("message = %q, expected to mention errors", out.Message)
+	}
+}
+
 func TestStatusBadge(t *testing.T) {
 	tests := []struct {
 		status string
@@ -326,3 +354,209 @@ func TestOutputJSONNoFiles(t *testing.T) {
 		t.Errorf("status = %q, want skipped", out.Status)
 	}
 }
+
+func captureStdout(t *testing.T, fn func()) string {
+	t.Helper()
+	old := os.Stdout
+	r, w, err := os.Pipe()
+	if err != nil {
+		t.Fatalf("os.Pipe: %v", err)
+	}
+	os.Stdout = w
+	fn()
+	w.Close()
+	os.Stdout = old
+	var buf bytes.Buffer
+	buf.ReadFrom(r)
+	return buf.String()
+}
+
+func TestOutputText_NoComments(t *testing.T) {
+	got := captureStdout(t, func() {
+		outputText(nil)
+	})
+	if !strings.Contains(got, "Looks good to me") {
+		t.Errorf("expected 'Looks good to me', got %q", got)
+	}
+}
+
+func TestOutputText_WithComments(t *testing.T) {
+	comments := []model.LlmComment{
+		{Path: "main.go", StartLine: 10, EndLine: 15, Content: "potential nil dereference"},
+	}
+	got := captureStdout(t, func() {
+		outputText(comments)
+	})
+	if !strings.Contains(got, "main.go") {
+		t.Errorf("expected path in output, got %q", got)
+	}
+	if !strings.Contains(got, "potential nil dereference") {
+		t.Errorf("expected comment content in output, got %q", got)
+	}
+}
+
+func TestOutputTextWithWarnings_NoCommentsNoErrors(t *testing.T) {
+	warnings := []agent.AgentWarning{{Type: "warning", File: "x.go", Message: "slow"}}
+	got := captureStdout(t, func() {
+		outputTextWithWarnings(nil, warnings)
+	})
+	if !strings.Contains(got, "Looks good to me") {
+		t.Errorf("expected 'Looks good to me', got %q", got)
+	}
+}
+
+func TestOutputTextWithWarnings_NoCommentsWithSubtaskError(t *testing.T) {
+	warnings := []agent.AgentWarning{{Type: "subtask_error", File: "y.go", Message: "failed"}}
+	got := captureStdout(t, func() {
+		outputTextWithWarnings(nil, warnings)
+	})
+	if !strings.Contains(got, "could not be reviewed") {
+		t.Errorf("expected subtask error message, got %q", got)
+	}
+}
+
+func TestOutputTextWithWarnings_WithComments(t *testing.T) {
+	comments := []model.LlmComment{
+		{Path: "a.go", StartLine: 1, EndLine: 3, Content: "fix this"},
+	}
+	warnings := []agent.AgentWarning{{Type: "info", File: "b.go", Message: "note"}}
+	got := captureStdout(t, func() {
+		outputTextWithWarnings(comments, warnings)
+	})
+	if !strings.Contains(got, "a.go") {
+		t.Errorf("expected comment path, got %q", got)
+	}
+	if !strings.Contains(got, "fix this") {
+		t.Errorf("expected comment content, got %q", got)
+	}
+}
+
+func TestRenderComment_EmptyContentNoDiff(t *testing.T) {
+	got := captureStdout(t, func() {
+		renderComment(model.LlmComment{Path: "skip.go", StartLine: 1, EndLine: 1, Content: "", ExistingCode: "", SuggestionCode: ""})
+	})
+	if got != "" {
+		t.Errorf("expected empty output for empty comment, got %q", got)
+	}
+}
+
+func TestRenderComment_ContentOnly(t *testing.T) {
+	got := captureStdout(t, func() {
+		renderComment(model.LlmComment{Path: "file.go", StartLine: 5, EndLine: 10, Content: "consider renaming"})
+	})
+	if !strings.Contains(got, "file.go:5-10") {
+		t.Errorf("expected path:line range, got %q", got)
+	}
+	if !strings.Contains(got, "consider renaming") {
+		t.Errorf("expected content, got %q", got)
+	}
+}
+
+func TestRenderComment_WithDiff(t *testing.T) {
+	got := captureStdout(t, func() {
+		renderComment(model.LlmComment{
+			Path:           "diff.go",
+			StartLine:      1,
+			EndLine:        2,
+			Content:        "rename var",
+			ExistingCode:   "old := 1\n",
+			SuggestionCode: "new := 1\n",
+		})
+	})
+	if !strings.Contains(got, "diff.go:1-2") {
+		t.Errorf("expected path:line range, got %q", got)
+	}
+	if !strings.Contains(got, "rename var") {
+		t.Errorf("expected content, got %q", got)
+	}
+}
+
+func TestPrintDiffLine(t *testing.T) {
+	tests := []struct {
+		name    string
+		prefix  string
+		content string
+	}{
+		{"added", "+", "new line"},
+		{"deleted", "-", "old line"},
+		{"context", " ", "context line"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := captureStdout(t, func() {
+				printDiffLine(tc.prefix, tc.content, "\033[92m", "\033[48;2;0;60;0m")
+			})
+			if !strings.Contains(got, tc.prefix) {
+				t.Errorf("expected prefix %q in output, got %q", tc.prefix, got)
+			}
+			if !strings.Contains(got, tc.content) {
+				t.Errorf("expected content %q in output, got %q", tc.content, got)
+			}
+		})
+	}
+}
+
+func TestOutputPreviewText_NoFiles(t *testing.T) {
+	p := &agent.DiffPreview{TotalFiles: 0}
+	got := captureStdout(t, func() {
+		outputPreviewText(p)
+	})
+	if !strings.Contains(got, "No files changed") {
+		t.Errorf("expected 'No files changed', got %q", got)
+	}
+}
+
+func TestOutputPreviewText_WithReviewableFiles(t *testing.T) {
+	p := &agent.DiffPreview{
+		Entries: []agent.DiffPreviewEntry{
+			{Path: "main.go", Status: "modified", Insertions: 10, Deletions: 3, WillReview: true},
+			{Path: "util.go", Status: "added", Insertions: 20, Deletions: 0, WillReview: true},
+		},
+		TotalInsertions: 30,
+		TotalDeletions:  3,
+		TotalFiles:      2,
+		ReviewableCount: 2,
+		ExcludedCount:   0,
+	}
+	got := captureStdout(t, func() {
+		outputPreviewText(p)
+	})
+	if !strings.Contains(got, "2 file(s) changed") {
+		t.Errorf("expected file count, got %q", got)
+	}
+	if !strings.Contains(got, "Will review (2)") {
+		t.Errorf("expected 'Will review' section, got %q", got)
+	}
+	if !strings.Contains(got, "main.go") || !strings.Contains(got, "util.go") {
+		t.Errorf("expected file paths, got %q", got)
+	}
+}
+
+func TestOutputPreviewText_WithExcludedFiles(t *testing.T) {
+	p := &agent.DiffPreview{
+		Entries: []agent.DiffPreviewEntry{
+			{Path: "src.go", Status: "modified", Insertions: 5, Deletions: 1, WillReview: true},
+			{Path: "vendor/lib.go", Status: "added", Insertions: 100, Deletions: 0, WillReview: false, ExcludeReason: model.ExcludeDefaultPath},
+		},
+		TotalInsertions: 105,
+		TotalDeletions:  1,
+		TotalFiles:      2,
+		ReviewableCount: 1,
+		ExcludedCount:   1,
+	}
+	got := captureStdout(t, func() {
+		outputPreviewText(p)
+	})
+	if !strings.Contains(got, "Will review (1)") {
+		t.Errorf("expected 'Will review (1)', got %q", got)
+	}
+	if !strings.Contains(got, "Excluded from review (1)") {
+		t.Errorf("expected 'Excluded from review (1)', got %q", got)
+	}
+	if !strings.Contains(got, "vendor/lib.go") {
+		t.Errorf("expected excluded file path, got %q", got)
+	}
+	if !strings.Contains(got, "default_path") {
+		t.Errorf("expected exclude reason, got %q", got)
+	}
+}

cmd/opencodereview/provider_cmd_test.go

@@ -0,0 +1,205 @@
+package main
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestMaskKey(t *testing.T) {
+	tests := []struct {
+		name string
+		key  string
+		want string
+	}{
+		{"empty", "", "(not set)"},
+		{"short", "abcd", "***"},
+		{"exactly 8", "12345678", "***"},
+		{"normal", "sk-ant-secret-key-1234", "sk-a***1234"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := maskKey(tc.key)
+			if got != tc.want {
+				t.Errorf("maskKey(%q) = %q, want %q", tc.key, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestSaveConfig(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "sub", "config.json")
+
+	cfg := &Config{
+		Provider: "anthropic",
+		Model:    "claude-opus-4-6",
+		Language: "English",
+	}
+
+	if err := saveConfig(path, cfg); err != nil {
+		t.Fatalf("saveConfig: %v", err)
+	}
+
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatalf("stat: %v", err)
+	}
+	if perm := info.Mode().Perm(); perm != 0o600 {
+		t.Errorf("perm = %o, want 600", perm)
+	}
+
+	data, err := os.ReadFile(path)
+	if err != nil {
+		t.Fatalf("read: %v", err)
+	}
+	var loaded Config
+	if err := json.Unmarshal(data, &loaded); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	if loaded.Provider != "anthropic" {
+		t.Errorf("Provider = %q", loaded.Provider)
+	}
+	if loaded.Language != "English" {
+		t.Errorf("Language = %q", loaded.Language)
+	}
+}
+
+func TestApplyProviderDeletions(t *testing.T) {
+	dir := t.TempDir()
+	configPath := filepath.Join(dir, "config.json")
+
+	cfg := &Config{
+		Provider: "keep",
+		CustomProviders: map[string]ProviderEntry{
+			"del1": {URL: "https://a.example.com"},
+			"del2": {URL: "https://b.example.com"},
+			"keep": {URL: "https://c.example.com"},
+		},
+	}
+	if err := saveConfig(configPath, cfg); err != nil {
+		t.Fatalf("saveConfig: %v", err)
+	}
+
+	clearedActive, err := applyProviderDeletions(configPath, cfg, []string{"del1", "del2"})
+	if err != nil {
+		t.Fatalf("applyProviderDeletions: %v", err)
+	}
+	if clearedActive {
+		t.Error("should not have cleared active provider")
+	}
+	if _, exists := cfg.CustomProviders["del1"]; exists {
+		t.Error("del1 should have been deleted")
+	}
+	if _, exists := cfg.CustomProviders["del2"]; exists {
+		t.Error("del2 should have been deleted")
+	}
+	if _, exists := cfg.CustomProviders["keep"]; !exists {
+		t.Error("keep should still exist")
+	}
+}
+
+func TestApplyProviderDeletions_ActiveCleared(t *testing.T) {
+	dir := t.TempDir()
+	configPath := filepath.Join(dir, "config.json")
+
+	cfg := &Config{
+		Provider: "active-one",
+		CustomProviders: map[string]ProviderEntry{
+			"active-one": {URL: "https://x.example.com"},
+		},
+	}
+	if err := saveConfig(configPath, cfg); err != nil {
+		t.Fatalf("saveConfig: %v", err)
+	}
+
+	clearedActive, err := applyProviderDeletions(configPath, cfg, []string{"active-one"})
+	if err != nil {
+		t.Fatalf("applyProviderDeletions: %v", err)
+	}
+	if !clearedActive {
+		t.Error("should have cleared active provider")
+	}
+}
+
+func TestApplyProviderDeletions_SkipsNotFound(t *testing.T) {
+	dir := t.TempDir()
+	configPath := filepath.Join(dir, "config.json")
+
+	cfg := &Config{
+		CustomProviders: map[string]ProviderEntry{
+			"exists": {URL: "https://a.example.com"},
+		},
+	}
+	if err := saveConfig(configPath, cfg); err != nil {
+		t.Fatalf("saveConfig: %v", err)
+	}
+
+	_, err := applyProviderDeletions(configPath, cfg, []string{"nonexistent"})
+	if err != nil {
+		t.Fatalf("applyProviderDeletions should not fail, got: %v", err)
+	}
+}
+
+func TestRemoveModels(t *testing.T) {
+	tests := []struct {
+		name     string
+		existing []string
+		remove   []string
+		want     []string
+	}{
+		{"remove one", []string{"a", "b", "c"}, []string{"b"}, []string{"a", "c"}},
+		{"remove none", []string{"a", "b"}, []string{"x"}, []string{"a", "b"}},
+		{"remove all", []string{"a", "b"}, []string{"a", "b"}, []string{}},
+		{"empty existing", nil, []string{"a"}, []string{}},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := removeModels(tc.existing, tc.remove)
+			if len(got) != len(tc.want) {
+				t.Fatalf("removeModels() = %v, want %v", got, tc.want)
+			}
+			for i := range tc.want {
+				if got[i] != tc.want[i] {
+					t.Errorf("[%d] = %q, want %q", i, got[i], tc.want[i])
+				}
+			}
+		})
+	}
+}
+
+func TestApplyManualConfig_MissingURL(t *testing.T) {
+	err := applyManualConfig("", &Config{}, providerTUIResult{url: "", model: "m"})
+	if err == nil {
+		t.Fatal("expected error for missing URL")
+	}
+}
+
+func TestApplyManualConfig_MissingModel(t *testing.T) {
+	err := applyManualConfig("", &Config{}, providerTUIResult{url: "https://example.com", model: ""})
+	if err == nil {
+		t.Fatal("expected error for missing model")
+	}
+}
+
+func TestApplyCustomProviderConfig_MissingProvider(t *testing.T) {
+	err := applyCustomProviderConfig("", &Config{}, providerTUIResult{provider: "", model: "m"})
+	if err == nil {
+		t.Fatal("expected error for missing provider")
+	}
+}
+
+func TestApplyCustomProviderConfig_MissingModel(t *testing.T) {
+	err := applyCustomProviderConfig("", &Config{}, providerTUIResult{provider: "p", model: ""})
+	if err == nil {
+		t.Fatal("expected error for missing model")
+	}
+}
+
+func TestApplyOfficialProviderConfig_MissingFields(t *testing.T) {
+	err := applyOfficialProviderConfig("", &Config{}, providerTUIResult{provider: "", model: ""})
+	if err == nil {
+		t.Fatal("expected error for missing provider/model")
+	}
+}

cmd/opencodereview/scan_cmd_test.go

@@ -140,3 +140,110 @@ func TestParseScanFlags_HelpFlag(t *testing.T) {
 		t.Error("opts.showHelp should be true when -h is supplied")
 	}
 }
+
+func TestParseScanFlags_RejectsNegativeMaxTokensBudget(t *testing.T) {
+	_, err := parseScanFlags([]string{"--max-tokens-budget", "-100"})
+	if err == nil {
+		t.Fatal("expected error for negative --max-tokens-budget")
+	}
+	if !strings.Contains(err.Error(), "--max-tokens-budget") {
+		t.Errorf("error message = %q; want it to mention --max-tokens-budget", err.Error())
+	}
+}
+
+func TestParseScanFlags_BooleanFlags(t *testing.T) {
+	opts, err := parseScanFlags([]string{"--no-plan", "--no-dedup", "--no-summary", "--preview"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !opts.noPlan {
+		t.Error("noPlan should be true")
+	}
+	if !opts.noDedup {
+		t.Error("noDedup should be true")
+	}
+	if !opts.noSummary {
+		t.Error("noSummary should be true")
+	}
+	if !opts.preview {
+		t.Error("preview should be true")
+	}
+}
+
+func TestParseScanFlags_ModelOverride(t *testing.T) {
+	opts, err := parseScanFlags([]string{"--model", "claude-opus-4-6"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.model != "claude-opus-4-6" {
+		t.Errorf("model = %q, want claude-opus-4-6", opts.model)
+	}
+}
+
+func TestParseScanFlags_AllStringFlags(t *testing.T) {
+	opts, err := parseScanFlags([]string{
+		"--tools", "/tmp/tools.json",
+		"--rule", "/tmp/rule.json",
+		"--repo", "/tmp/repo",
+		"--exclude", "*.md,*.txt",
+		"--batch", "by-language",
+		"--background", "test context",
+		"--audience", "agent",
+		"-f", "json",
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.toolConfigPath != "/tmp/tools.json" {
+		t.Errorf("toolConfigPath = %q", opts.toolConfigPath)
+	}
+	if opts.rulePath != "/tmp/rule.json" {
+		t.Errorf("rulePath = %q", opts.rulePath)
+	}
+	if opts.repoDir != "/tmp/repo" {
+		t.Errorf("repoDir = %q", opts.repoDir)
+	}
+	if opts.excludes != "*.md,*.txt" {
+		t.Errorf("excludes = %q", opts.excludes)
+	}
+	if opts.batch != "by-language" {
+		t.Errorf("batch = %q", opts.batch)
+	}
+	if opts.background != "test context" {
+		t.Errorf("background = %q", opts.background)
+	}
+	if opts.audience != "agent" {
+		t.Errorf("audience = %q", opts.audience)
+	}
+	if opts.outputFormat != "json" {
+		t.Errorf("outputFormat = %q", opts.outputFormat)
+	}
+}
+
+func TestParseScanFlags_IntFlags(t *testing.T) {
+	opts, err := parseScanFlags([]string{
+		"--concurrency", "16",
+		"--timeout", "20",
+		"--max-tools", "50",
+		"--max-git-procs", "32",
+		"--max-tokens-budget", "100000",
+	})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.concurrency != 16 {
+		t.Errorf("concurrency = %d", opts.concurrency)
+	}
+	if opts.perFileTimeout != 20 {
+		t.Errorf("perFileTimeout = %d", opts.perFileTimeout)
+	}
+	if opts.maxTools != 50 {
+		t.Errorf("maxTools = %d", opts.maxTools)
+	}
+	if opts.maxGitProcs != 32 {
+		t.Errorf("maxGitProcs = %d", opts.maxGitProcs)
+	}
+	if opts.maxTokensBudget != 100000 {
+		t.Errorf("maxTokensBudget = %d", opts.maxTokensBudget)
+	}
+}

cmd/opencodereview/shared_test.go

@@ -0,0 +1,127 @@
+package main
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/open-code-review/open-code-review/internal/config/rules"
+)
+
+func TestApplyCLIExcludes_Empty(t *testing.T) {
+	cc := &commonContext{FileFilter: &rules.FileFilter{Exclude: []string{"a"}}}
+	applyCLIExcludes(cc, nil)
+	if len(cc.FileFilter.Exclude) != 1 {
+		t.Errorf("expected 1 exclude, got %d", len(cc.FileFilter.Exclude))
+	}
+}
+
+func TestApplyCLIExcludes_AppendsPatterns(t *testing.T) {
+	cc := &commonContext{FileFilter: &rules.FileFilter{Exclude: []string{"a"}}}
+	applyCLIExcludes(cc, []string{"b", "c"})
+	if len(cc.FileFilter.Exclude) != 3 {
+		t.Errorf("expected 3 excludes, got %d", len(cc.FileFilter.Exclude))
+	}
+}
+
+func TestApplyCLIExcludes_NilFileFilter(t *testing.T) {
+	cc := &commonContext{}
+	applyCLIExcludes(cc, []string{"x"})
+	if cc.FileFilter == nil {
+		t.Fatal("expected FileFilter to be created")
+	}
+	if len(cc.FileFilter.Exclude) != 1 || cc.FileFilter.Exclude[0] != "x" {
+		t.Errorf("expected [x], got %v", cc.FileFilter.Exclude)
+	}
+}
+
+func TestNewQuietHandle_NoOp(t *testing.T) {
+	h := newQuietHandle("text", "developer")
+	if h.fn != nil {
+		t.Error("expected no-op handle for text/developer")
+	}
+	h.Restore()
+}
+
+func TestNewQuietHandle_JSON(t *testing.T) {
+	h := newQuietHandle("json", "developer")
+	if h.fn == nil {
+		t.Error("expected fn to be set for json format")
+	}
+	h.Restore()
+	if h.fn != nil {
+		t.Error("expected fn to be nil after Restore")
+	}
+}
+
+func TestNewQuietHandle_Agent(t *testing.T) {
+	h := newQuietHandle("text", "agent")
+	if h.fn == nil {
+		t.Error("expected fn to be set for agent audience")
+	}
+	h.Restore()
+}
+
+func TestQuietHandle_NilReceiver(t *testing.T) {
+	var h *quietHandle
+	h.Restore()
+}
+
+func TestQuietHandle_IdempotentRestore(t *testing.T) {
+	h := newQuietHandle("json", "developer")
+	h.Restore()
+	h.Restore()
+	if h.fn != nil {
+		t.Error("expected nil after double restore")
+	}
+}
+
+func TestResolveWorkingDir_CurrentDir(t *testing.T) {
+	dir := t.TempDir()
+	origDir, _ := os.Getwd()
+	defer os.Chdir(origDir)
+	os.Chdir(dir)
+
+	absPath, isGit, err := resolveWorkingDir("", false)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if absPath == "" {
+		t.Error("expected non-empty absPath")
+	}
+	if isGit {
+		t.Error("temp dir should not be a git repo")
+	}
+}
+
+func TestResolveWorkingDir_RequireGitFails(t *testing.T) {
+	dir := t.TempDir()
+	_, _, err := resolveWorkingDir(dir, true)
+	if err == nil {
+		t.Fatal("expected error for non-git dir with requireGit=true")
+	}
+}
+
+func TestResolveWorkingDir_NonExistent(t *testing.T) {
+	_, _, err := resolveWorkingDir(filepath.Join(t.TempDir(), "no-such-dir"), false)
+	if err == nil {
+		t.Fatal("expected error for non-existent path")
+	}
+}
+
+func TestResolveWorkingDir_GitRepo(t *testing.T) {
+	dir := t.TempDir()
+	gitDir := filepath.Join(dir, ".git")
+	if err := os.Mkdir(gitDir, 0o755); err != nil {
+		t.Fatal(err)
+	}
+
+	absPath, isGit, err := resolveWorkingDir(dir, false)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if absPath == "" {
+		t.Error("expected non-empty absPath")
+	}
+	_ = isGit
+}

cmd/opencodereview/smallfiles_test.go

@@ -0,0 +1,248 @@
+package main
+
+import (
+	"runtime"
+	"strings"
+	"testing"
+)
+
+func TestPrintVersion_Dev(t *testing.T) {
+	origVersion := Version
+	origCommit := GitCommit
+	origDate := BuildDate
+	defer func() {
+		Version = origVersion
+		GitCommit = origCommit
+		BuildDate = origDate
+	}()
+
+	Version = "dev"
+	GitCommit = ""
+	BuildDate = ""
+
+	got := captureStdout(t, func() {
+		printVersion()
+	})
+	if !strings.Contains(got, "open-code-review dev") {
+		t.Errorf("expected 'open-code-review dev', got %q", got)
+	}
+	if !strings.Contains(got, runtime.GOOS+"/"+runtime.GOARCH) {
+		t.Errorf("expected OS/ARCH, got %q", got)
+	}
+}
+
+func TestPrintVersion_WithCommitAndDate(t *testing.T) {
+	origVersion := Version
+	origCommit := GitCommit
+	origDate := BuildDate
+	defer func() {
+		Version = origVersion
+		GitCommit = origCommit
+		BuildDate = origDate
+	}()
+
+	Version = "1.2.3"
+	GitCommit = "abc1234"
+	BuildDate = "2026-01-01"
+
+	got := captureStdout(t, func() {
+		printVersion()
+	})
+	if !strings.Contains(got, "1.2.3") {
+		t.Errorf("expected version, got %q", got)
+	}
+	if !strings.Contains(got, "abc1234") {
+		t.Errorf("expected commit, got %q", got)
+	}
+	if !strings.Contains(got, "2026-01-01") {
+		t.Errorf("expected build date, got %q", got)
+	}
+}
+
+func TestParseViewerFlags_Defaults(t *testing.T) {
+	opts, err := parseViewerFlags(nil)
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.addr != "localhost:5483" {
+		t.Errorf("addr = %q, want localhost:5483", opts.addr)
+	}
+	if opts.showHelp {
+		t.Error("showHelp should be false")
+	}
+}
+
+func TestParseViewerFlags_CustomAddr(t *testing.T) {
+	opts, err := parseViewerFlags([]string{"--addr", ":3000"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if opts.addr != ":3000" {
+		t.Errorf("addr = %q, want :3000", opts.addr)
+	}
+}
+
+func TestParseViewerFlags_Help(t *testing.T) {
+	opts, err := parseViewerFlags([]string{"-h"})
+	if err != nil {
+		t.Fatalf("unexpected error: %v", err)
+	}
+	if !opts.showHelp {
+		t.Error("expected showHelp=true")
+	}
+}
+
+func TestRunLLM_NoArgs(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runLLM(nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "LLM utility") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestRunLLM_UnknownSubcommand(t *testing.T) {
+	err := runLLM([]string{"bogus"})
+	if err == nil {
+		t.Fatal("expected error for unknown subcommand")
+	}
+	if !strings.Contains(err.Error(), "unknown llm sub-command") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+func TestRunRules_NoArgs(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runRules(nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "ocr rules") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestRunRules_Help(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runRules([]string{"-h"})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "ocr rules") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestRunRules_UnknownSubcommand(t *testing.T) {
+	err := runRules([]string{"bogus"})
+	if err == nil {
+		t.Fatal("expected error for unknown subcommand")
+	}
+}
+
+func TestRunRules_HelpAltFlag(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runRules([]string{"--help"})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "ocr rules") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestRunRulesCheck_Help(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runRulesCheck([]string{"-h"})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "ocr rules check") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestRunRulesCheck_NoArgs(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runRulesCheck(nil)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "ocr rules check") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestRunLLMProviders(t *testing.T) {
+	got := captureStdout(t, func() {
+		runLLMProviders()
+	})
+	if !strings.Contains(got, "Built-in providers") {
+		t.Errorf("expected provider listing, got %q", got)
+	}
+}
+
+func TestRunViewer_Help(t *testing.T) {
+	got := captureStdout(t, func() {
+		err := runViewer([]string{"-h"})
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+	})
+	if !strings.Contains(got, "Session history") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestPrintReviewUsage(t *testing.T) {
+	got := captureStdout(t, func() {
+		printReviewUsage()
+	})
+	if !strings.Contains(got, "ocr review") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestPrintTopLevelUsage(t *testing.T) {
+	got := captureStdout(t, func() {
+		printTopLevelUsage()
+	})
+	if !strings.Contains(got, "OpenCodeReview") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestPrintViewerUsage(t *testing.T) {
+	got := captureStdout(t, func() {
+		printViewerUsage()
+	})
+	if !strings.Contains(got, "Session history") {
+		t.Errorf("expected viewer usage text, got %q", got)
+	}
+}
+
+func TestPrintRulesCheckUsage(t *testing.T) {
+	got := captureStdout(t, func() {
+		printRulesCheckUsage()
+	})
+	if !strings.Contains(got, "ocr rules check") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}
+
+func TestPrintScanUsage(t *testing.T) {
+	got := captureStdout(t, func() {
+		printScanUsage()
+	})
+	if !strings.Contains(got, "ocr scan") {
+		t.Errorf("expected usage text, got %q", got)
+	}
+}