test: add unit tests for output_helpers, config, model, stdout, and telemetry packages

cmd/opencodereview/output_helpers_test.go

@@ -0,0 +1,328 @@
+package main
+
+import (
+	"bytes"
+	"encoding/json"
+	"os"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/open-code-review/open-code-review/internal/agent"
+	"github.com/open-code-review/open-code-review/internal/model"
+)
+
+func TestHasSubtaskErrors(t *testing.T) {
+	tests := []struct {
+		name     string
+		warnings []agent.AgentWarning
+		want     bool
+	}{
+		{"nil warnings", nil, false},
+		{"empty", []agent.AgentWarning{}, false},
+		{"no subtask errors", []agent.AgentWarning{{Type: "other", Message: "msg"}}, false},
+		{"has subtask error", []agent.AgentWarning{{Type: "subtask_error", Message: "fail"}}, true},
+		{"mixed", []agent.AgentWarning{{Type: "warn"}, {Type: "subtask_error"}}, true},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := hasSubtaskErrors(tc.warnings)
+			if got != tc.want {
+				t.Errorf("hasSubtaskErrors() = %v, want %v", got, tc.want)
+			}
+		})
+	}
+}
+
+func TestWrapByRunes(t *testing.T) {
+	tests := []struct {
+		name  string
+		text  string
+		maxW  int
+		lines int
+	}{
+		{"empty", "", 80, 0},
+		{"short line", "hello", 80, 1},
+		{"exact width", strings.Repeat("a", 10), 10, 1},
+		{"wraps long line", strings.Repeat("word ", 25), 20, 7},
+		{"respects newlines", "line1\nline2\nline3", 80, 3},
+		{"wrap with newlines", "short\n" + strings.Repeat("x", 50), 20, 4},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := wrapByRunes(tc.text, tc.maxW)
+			if len(got) != tc.lines {
+				t.Errorf("wrapByRunes() got %d lines, want %d\nlines: %v", len(got), tc.lines, got)
+			}
+		})
+	}
+}
+
+func TestWrapSingleRuneLine(t *testing.T) {
+	tests := []struct {
+		name string
+		line string
+		maxW int
+		min  int
+	}{
+		{"short line unchanged", "hello", 100, 1},
+		{"wraps at space", "hello world foo bar baz", 12, 2},
+		{"no space to wrap", strings.Repeat("x", 30), 10, 3},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := wrapSingleRuneLine(tc.line, tc.maxW)
+			if len(got) < tc.min {
+				t.Errorf("got %d lines, want at least %d", len(got), tc.min)
+			}
+		})
+	}
+}
+
+func TestRuneWrapCut(t *testing.T) {
+	// Short line returns full length
+	runes := []rune("short")
+	cut := runeWrapCut(runes, 100)
+	if cut != len(runes) {
+		t.Errorf("expected %d, got %d", len(runes), cut)
+	}
+
+	// Cuts at space
+	runes = []rune("hello world test")
+	cut = runeWrapCut(runes, 11)
+	if runes[cut] != ' ' && cut != 11 {
+		t.Errorf("expected cut at space boundary, got %d (char=%c)", cut, runes[cut])
+	}
+}
+
+func TestVisibleRunesLen(t *testing.T) {
+	tests := []struct {
+		input string
+		want  int
+	}{
+		{"hello", 5},
+		{"", 0},
+		{"\x01\x02\x03", 0},
+		{"a\x01b", 2},
+		{"\x7f", 0},
+	}
+	for _, tc := range tests {
+		got := visibleRunesLen([]rune(tc.input))
+		if got != tc.want {
+			t.Errorf("visibleRunesLen(%q) = %d, want %d", tc.input, got, tc.want)
+		}
+	}
+}
+
+func TestSplitToLines(t *testing.T) {
+	tests := []struct {
+		input string
+		want  int
+	}{
+		{"a\nb\nc", 3},
+		{"a\nb\nc\n", 3},
+		{"single", 1},
+		{"crlf\r\nline", 2},
+		{"", 0},
+	}
+	for _, tc := range tests {
+		got := splitToLines(tc.input)
+		if len(got) != tc.want {
+			t.Errorf("splitToLines(%q) = %d lines, want %d", tc.input, len(got), tc.want)
+		}
+	}
+}
+
+func TestBuildDiffLines(t *testing.T) {
+	t.Run("empty suggestion returns nil", func(t *testing.T) {
+		c := model.LlmComment{ExistingCode: "old", SuggestionCode: ""}
+		got := buildDiffLines(c)
+		if got != nil {
+			t.Errorf("expected nil, got %v", got)
+		}
+	})
+
+	t.Run("empty existing returns nil", func(t *testing.T) {
+		c := model.LlmComment{ExistingCode: "", SuggestionCode: "new"}
+		got := buildDiffLines(c)
+		if got != nil {
+			t.Errorf("expected nil, got %v", got)
+		}
+	})
+
+	t.Run("diff computed", func(t *testing.T) {
+		c := model.LlmComment{
+			ExistingCode:   "line1\nline2\n",
+			SuggestionCode: "line1\nmodified\n",
+		}
+		got := buildDiffLines(c)
+		if len(got) == 0 {
+			t.Error("expected non-empty diff lines")
+		}
+	})
+}
+
+func TestStatusBadge(t *testing.T) {
+	tests := []struct {
+		status string
+		substr string
+	}{
+		{"added", "[A]"},
+		{"modified", "[M]"},
+		{"deleted", "[D]"},
+		{"renamed", "[R]"},
+		{"binary", "[B]"},
+		{"scan", "[S]"},
+		{"unknown", "[?]"},
+	}
+	for _, tc := range tests {
+		got := statusBadge(tc.status)
+		if !strings.Contains(got, tc.substr) {
+			t.Errorf("statusBadge(%q) = %q, expected to contain %q", tc.status, got, tc.substr)
+		}
+	}
+}
+
+func TestOutputJSON(t *testing.T) {
+	// Redirect stdout to capture output
+	old := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	comments := []model.LlmComment{
+		{Path: "a.go", Content: "fix bug", StartLine: 1, EndLine: 5},
+	}
+	err := outputJSON(comments)
+
+	w.Close()
+	os.Stdout = old
+
+	if err != nil {
+		t.Fatalf("outputJSON error: %v", err)
+	}
+
+	var buf bytes.Buffer
+	buf.ReadFrom(r)
+
+	var out jsonOutput
+	if err := json.Unmarshal(buf.Bytes(), &out); err != nil {
+		t.Fatalf("unmarshal output: %v", err)
+	}
+	if out.Status != "success" {
+		t.Errorf("status = %q, want success", out.Status)
+	}
+	if len(out.Comments) != 1 {
+		t.Errorf("expected 1 comment, got %d", len(out.Comments))
+	}
+}
+
+func TestOutputJSON_NoComments(t *testing.T) {
+	old := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	err := outputJSON(nil)
+
+	w.Close()
+	os.Stdout = old
+
+	if err != nil {
+		t.Fatalf("outputJSON error: %v", err)
+	}
+
+	var buf bytes.Buffer
+	buf.ReadFrom(r)
+
+	var out jsonOutput
+	json.Unmarshal(buf.Bytes(), &out)
+	if out.Message == "" {
+		t.Error("expected non-empty message when no comments")
+	}
+}
+
+func TestOutputJSONWithWarnings(t *testing.T) {
+	old := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	comments := []model.LlmComment{{Path: "b.go", Content: "test"}}
+	warnings := []agent.AgentWarning{{Type: "subtask_error", File: "c.go", Message: "failed"}}
+	err := outputJSONWithWarnings(comments, warnings, 5, 100, 50, 150, 10, 5, 3*time.Second, "summary", map[string]int64{"file_read": 3})
+
+	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 out.Summary == nil {
+		t.Fatal("expected non-nil summary")
+	}
+	if out.Summary.FilesReviewed != 5 {
+		t.Errorf("FilesReviewed = %d, want 5", out.Summary.FilesReviewed)
+	}
+	if out.ToolCalls == nil || out.ToolCalls.Total != 3 {
+		t.Errorf("ToolCalls.Total = %v", out.ToolCalls)
+	}
+}
+
+func TestOutputJSONWithWarnings_NoCommentsNoErrors(t *testing.T) {
+	old := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	warnings := []agent.AgentWarning{{Type: "warning", Message: "something"}}
+	err := outputJSONWithWarnings(nil, warnings, 2, 50, 20, 70, 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_warnings" {
+		t.Errorf("status = %q, want completed_with_warnings", out.Status)
+	}
+	if out.Message == "" {
+		t.Error("expected non-empty message")
+	}
+}
+
+func TestOutputJSONNoFiles(t *testing.T) {
+	old := os.Stdout
+	r, w, _ := os.Pipe()
+	os.Stdout = w
+
+	err := outputJSONNoFiles()
+
+	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 != "skipped" {
+		t.Errorf("status = %q, want skipped", out.Status)
+	}
+}

internal/config/testconnection/testconnection_test.go

@@ -0,0 +1,92 @@
+package testconnection
+
+import (
+	"testing"
+)
+
+func TestLoadDefault(t *testing.T) {
+	conv, err := LoadDefault()
+	if err != nil {
+		t.Fatalf("LoadDefault: %v", err)
+	}
+	if conv == nil {
+		t.Fatal("expected non-nil conversation")
+	}
+	if conv.Timeout <= 0 {
+		t.Errorf("expected positive timeout, got %d", conv.Timeout)
+	}
+	if len(conv.Messages) == 0 {
+		t.Fatal("expected at least one message")
+	}
+	hasSystem := false
+	hasUser := false
+	for _, m := range conv.Messages {
+		switch m.Role {
+		case "system":
+			hasSystem = true
+		case "user":
+			hasUser = true
+		}
+	}
+	if !hasSystem {
+		t.Error("expected a system message")
+	}
+	if !hasUser {
+		t.Error("expected a user message")
+	}
+}
+
+func TestResolveLang(t *testing.T) {
+	tests := []struct {
+		input string
+		want  string
+	}{
+		{"", "English"},
+		{"Chinese", "Chinese"},
+		{"Japanese", "Japanese"},
+	}
+	for _, tc := range tests {
+		got := resolveLang(tc.input)
+		if got != tc.want {
+			t.Errorf("resolveLang(%q) = %q, want %q", tc.input, got, tc.want)
+		}
+	}
+}
+
+func TestApplyLanguage(t *testing.T) {
+	conv := &LlmConversation{
+		Messages: []ChatMessage{
+			{Role: "system", Content: "You are a bot."},
+			{Role: "user", Content: "Hello"},
+		},
+	}
+
+	conv.ApplyLanguage("Chinese")
+
+	if conv.Messages[0].Content == "You are a bot." {
+		t.Error("expected system message to be modified")
+	}
+	expected := "You are a bot.\n\nAlways respond in Chinese."
+	if conv.Messages[0].Content != expected {
+		t.Errorf("system content = %q, want %q", conv.Messages[0].Content, expected)
+	}
+	// User message should not be modified
+	if conv.Messages[1].Content != "Hello" {
+		t.Errorf("user content should not change, got %q", conv.Messages[1].Content)
+	}
+}
+
+func TestApplyLanguage_EmptyLang(t *testing.T) {
+	conv := &LlmConversation{
+		Messages: []ChatMessage{
+			{Role: "system", Content: "Base."},
+		},
+	}
+
+	conv.ApplyLanguage("")
+
+	expected := "Base.\n\nAlways respond in English."
+	if conv.Messages[0].Content != expected {
+		t.Errorf("content = %q, want %q", conv.Messages[0].Content, expected)
+	}
+}

internal/config/toolsconfig/toolsconfig_test.go

@@ -0,0 +1,102 @@
+package toolsconfig
+
+import (
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestLoad_Default(t *testing.T) {
+	tools, err := Load("")
+	if err != nil {
+		t.Fatalf("Load default tools: %v", err)
+	}
+	if len(tools) == 0 {
+		t.Fatal("expected at least one tool from embedded config")
+	}
+	// Verify first tool has required fields
+	first := tools[0]
+	if first.Name == "" {
+		t.Error("expected non-empty tool name")
+	}
+	if first.Definition == nil {
+		t.Error("expected non-nil definition")
+	}
+}
+
+func TestLoad_CustomFile(t *testing.T) {
+	tmp := t.TempDir()
+	path := filepath.Join(tmp, "tools.json")
+	data := `[
+		{"name": "test_tool", "plan_task": true, "main_task": false, "definition": {"name": "test_tool"}}
+	]`
+	os.WriteFile(path, []byte(data), 0644)
+
+	tools, err := Load(path)
+	if err != nil {
+		t.Fatalf("Load custom file: %v", err)
+	}
+	if len(tools) != 1 {
+		t.Fatalf("expected 1 tool, got %d", len(tools))
+	}
+	if tools[0].Name != "test_tool" {
+		t.Errorf("expected name=test_tool, got %s", tools[0].Name)
+	}
+	if !tools[0].PlanTask {
+		t.Error("expected PlanTask=true")
+	}
+	if tools[0].MainTask {
+		t.Error("expected MainTask=false")
+	}
+}
+
+func TestLoad_FileNotFound(t *testing.T) {
+	_, err := Load("/nonexistent/tools.json")
+	if err == nil {
+		t.Error("expected error for nonexistent file")
+	}
+}
+
+func TestLoad_InvalidJSON(t *testing.T) {
+	tmp := t.TempDir()
+	path := filepath.Join(tmp, "tools.json")
+	os.WriteFile(path, []byte("not json"), 0644)
+
+	_, err := Load(path)
+	if err == nil {
+		t.Error("expected error for invalid JSON")
+	}
+}
+
+func TestToolDefsByPhase(t *testing.T) {
+	def := json.RawMessage(`{"name": "test"}`)
+	tests := []struct {
+		name     string
+		entry    ToolConfigEntry
+		planOnly bool
+		wantOk   bool
+	}{
+		{"plan_task and planOnly=true", ToolConfigEntry{PlanTask: true, MainTask: false, Definition: def}, true, true},
+		{"plan_task and planOnly=false", ToolConfigEntry{PlanTask: true, MainTask: false, Definition: def}, false, false},
+		{"main_task and planOnly=false", ToolConfigEntry{PlanTask: false, MainTask: true, Definition: def}, false, true},
+		{"main_task and planOnly=true", ToolConfigEntry{PlanTask: false, MainTask: true, Definition: def}, true, false},
+		{"both and planOnly=true", ToolConfigEntry{PlanTask: true, MainTask: true, Definition: def}, true, true},
+		{"both and planOnly=false", ToolConfigEntry{PlanTask: true, MainTask: true, Definition: def}, false, true},
+		{"neither", ToolConfigEntry{PlanTask: false, MainTask: false, Definition: def}, true, false},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got, ok := tc.entry.ToolDefsByPhase(tc.planOnly)
+			if ok != tc.wantOk {
+				t.Errorf("ToolDefsByPhase(planOnly=%v) ok=%v, want %v", tc.planOnly, ok, tc.wantOk)
+			}
+			if tc.wantOk && got == nil {
+				t.Error("expected non-nil definition when ok=true")
+			}
+			if !tc.wantOk && got != nil {
+				t.Error("expected nil definition when ok=false")
+			}
+		})
+	}
+}

internal/model/model_test.go

@@ -0,0 +1,192 @@
+package model
+
+import (
+	"encoding/json"
+	"testing"
+)
+
+func TestDiff_JSONRoundTrip(t *testing.T) {
+	d := Diff{
+		OldPath:    "a.go",
+		NewPath:    "b.go",
+		Diff:       "@@ -1 +1 @@\n-old\n+new",
+		IsBinary:   false,
+		IsDeleted:  false,
+		IsNew:      true,
+		IsRenamed:  true,
+		Insertions: 5,
+		Deletions:  3,
+	}
+
+	data, err := json.Marshal(d)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+
+	var got Diff
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+
+	if got != d {
+		t.Errorf("roundtrip mismatch:\n  got  %+v\n  want %+v", got, d)
+	}
+}
+
+func TestPreviewEntry_JSONRoundTrip(t *testing.T) {
+	e := PreviewEntry{
+		Path:          "main.go",
+		Status:        "modified",
+		Insertions:    10,
+		Deletions:     2,
+		WillReview:    true,
+		ExcludeReason: ExcludeNone,
+	}
+
+	data, err := json.Marshal(e)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+
+	var got PreviewEntry
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+
+	if got != e {
+		t.Errorf("roundtrip mismatch:\n  got  %+v\n  want %+v", got, e)
+	}
+}
+
+func TestPreviewEntry_ExcludeReasonOmitEmpty(t *testing.T) {
+	e := PreviewEntry{
+		Path:          "a.go",
+		ExcludeReason: ExcludeNone,
+	}
+	data, _ := json.Marshal(e)
+	var m map[string]any
+	json.Unmarshal(data, &m)
+	if _, ok := m["exclude_reason"]; ok {
+		t.Error("expected exclude_reason to be omitted when empty")
+	}
+
+	e.ExcludeReason = ExcludeUserRule
+	data, _ = json.Marshal(e)
+	json.Unmarshal(data, &m)
+	if m["exclude_reason"] != "user_exclude" {
+		t.Errorf("expected exclude_reason=user_exclude, got %v", m["exclude_reason"])
+	}
+}
+
+func TestExcludeReasonConstants(t *testing.T) {
+	constants := map[ExcludeReason]string{
+		ExcludeNone:        "",
+		ExcludeUserRule:    "user_exclude",
+		ExcludeExtension:   "unsupported_ext",
+		ExcludeDefaultPath: "default_path",
+		ExcludeDeleted:     "deleted",
+		ExcludeBinary:      "binary",
+	}
+	for k, v := range constants {
+		if string(k) != v {
+			t.Errorf("ExcludeReason constant mismatch: got %q, want %q", string(k), v)
+		}
+	}
+}
+
+func TestScanItem_AsDiff(t *testing.T) {
+	item := &ScanItem{
+		Path:      "file.go",
+		Content:   "package main\n",
+		IsBinary:  false,
+		LineCount: 1,
+	}
+
+	d := item.AsDiff()
+	if d == nil {
+		t.Fatal("expected non-nil Diff")
+	}
+	if d.OldPath != "file.go" {
+		t.Errorf("OldPath = %q, want file.go", d.OldPath)
+	}
+	if d.NewPath != "file.go" {
+		t.Errorf("NewPath = %q, want file.go", d.NewPath)
+	}
+	if d.NewFileContent != "package main\n" {
+		t.Errorf("NewFileContent = %q", d.NewFileContent)
+	}
+	if d.IsBinary {
+		t.Error("expected IsBinary=false")
+	}
+	if d.Insertions != 1 {
+		t.Errorf("Insertions = %d, want 1", d.Insertions)
+	}
+}
+
+func TestScanItem_AsDiff_Nil(t *testing.T) {
+	var item *ScanItem
+	d := item.AsDiff()
+	if d != nil {
+		t.Error("expected nil Diff for nil ScanItem")
+	}
+}
+
+func TestScanItem_AsDiff_Binary(t *testing.T) {
+	item := &ScanItem{
+		Path:     "image.png",
+		IsBinary: true,
+	}
+	d := item.AsDiff()
+	if !d.IsBinary {
+		t.Error("expected IsBinary=true")
+	}
+}
+
+func TestLlmComment_JSON(t *testing.T) {
+	c := LlmComment{
+		Path:           "main.go",
+		Content:        "fix this",
+		SuggestionCode: "new code",
+		ExistingCode:   "old code",
+		StartLine:      10,
+		EndLine:        15,
+		Thinking:       "reasoning",
+	}
+
+	data, err := json.Marshal(c)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+
+	var got LlmComment
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+
+	if got != c {
+		t.Errorf("roundtrip mismatch:\n  got  %+v\n  want %+v", got, c)
+	}
+}
+
+func TestCodeReviewResult_JSON(t *testing.T) {
+	r := CodeReviewResult{
+		RelevantFile:      "api.go",
+		SuggestionContent: "suggestion",
+		ExistingCode:      "old",
+		SuggestionCode:    "new",
+	}
+
+	data, err := json.Marshal(r)
+	if err != nil {
+		t.Fatalf("marshal: %v", err)
+	}
+
+	var got CodeReviewResult
+	if err := json.Unmarshal(data, &got); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+
+	if got != r {
+		t.Errorf("roundtrip mismatch:\n  got  %+v\n  want %+v", got, r)
+	}
+}

internal/stdout/stdout_test.go

@@ -0,0 +1,30 @@
+package stdout
+
+import (
+	"io"
+	"os"
+	"testing"
+)
+
+func TestWriter_Default(t *testing.T) {
+	w := Writer()
+	if w != os.Stdout {
+		t.Error("expected default Writer to be os.Stdout")
+	}
+}
+
+func TestQuiet(t *testing.T) {
+	restore := Quiet()
+
+	w := Writer()
+	if w != io.Discard {
+		t.Error("expected Writer to be io.Discard after Quiet()")
+	}
+
+	restore()
+
+	w = Writer()
+	if w != os.Stdout {
+		t.Error("expected Writer to be os.Stdout after restore")
+	}
+}

internal/telemetry/config_test.go

@@ -0,0 +1,265 @@
+package telemetry
+
+import (
+	"os"
+	"path/filepath"
+	"testing"
+)
+
+func TestDefaultConfig(t *testing.T) {
+	cfg := DefaultConfig()
+	if cfg.Enabled {
+		t.Error("expected Enabled=false by default")
+	}
+	if cfg.ServiceName != "open-code-review" {
+		t.Errorf("expected ServiceName=open-code-review, got %s", cfg.ServiceName)
+	}
+	if cfg.Exporter != "console" {
+		t.Errorf("expected Exporter=console, got %s", cfg.Exporter)
+	}
+	if cfg.OTLPEndpoint != "" {
+		t.Errorf("expected empty OTLPEndpoint, got %s", cfg.OTLPEndpoint)
+	}
+	if cfg.OTLPProtocol != "grpc" {
+		t.Errorf("expected OTLPProtocol=grpc, got %s", cfg.OTLPProtocol)
+	}
+	if cfg.ContentLog {
+		t.Error("expected ContentLog=false by default")
+	}
+}
+
+func TestResolveEnv(t *testing.T) {
+	tests := []struct {
+		name  string
+		envs  map[string]string
+		check func(t *testing.T, cfg Config)
+	}{
+		{
+			name: "enable telemetry",
+			envs: map[string]string{"OCR_ENABLE_TELEMETRY": "1"},
+			check: func(t *testing.T, cfg Config) {
+				if !cfg.Enabled {
+					t.Error("expected Enabled=true")
+				}
+			},
+		},
+		{
+			name: "custom service name",
+			envs: map[string]string{"OTEL_SERVICE_NAME": "my-service"},
+			check: func(t *testing.T, cfg Config) {
+				if cfg.ServiceName != "my-service" {
+					t.Errorf("expected ServiceName=my-service, got %s", cfg.ServiceName)
+				}
+			},
+		},
+		{
+			name: "otlp endpoint sets exporter to otlp",
+			envs: map[string]string{"OTEL_EXPORTER_OTLP_ENDPOINT": "localhost:4317"},
+			check: func(t *testing.T, cfg Config) {
+				if cfg.Exporter != "otlp" {
+					t.Errorf("expected Exporter=otlp, got %s", cfg.Exporter)
+				}
+				if cfg.OTLPEndpoint != "localhost:4317" {
+					t.Errorf("expected OTLPEndpoint=localhost:4317, got %s", cfg.OTLPEndpoint)
+				}
+			},
+		},
+		{
+			name: "otlp protocol override",
+			envs: map[string]string{"OTEL_EXPORTER_OTLP_PROTOCOL": "http/json"},
+			check: func(t *testing.T, cfg Config) {
+				if cfg.OTLPProtocol != "http/json" {
+					t.Errorf("expected OTLPProtocol=http/json, got %s", cfg.OTLPProtocol)
+				}
+			},
+		},
+		{
+			name: "content logging enabled",
+			envs: map[string]string{"OCR_CONTENT_LOGGING": "1"},
+			check: func(t *testing.T, cfg Config) {
+				if !cfg.ContentLog {
+					t.Error("expected ContentLog=true")
+				}
+			},
+		},
+		{
+			name: "no env vars leaves defaults",
+			envs: map[string]string{},
+			check: func(t *testing.T, cfg Config) {
+				if cfg.Enabled {
+					t.Error("expected Enabled=false")
+				}
+			},
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Clear relevant env vars
+			envKeys := []string{
+				"OCR_ENABLE_TELEMETRY", "OTEL_SERVICE_NAME",
+				"OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL",
+				"OCR_CONTENT_LOGGING",
+			}
+			for _, k := range envKeys {
+				t.Setenv(k, "")
+				os.Unsetenv(k)
+			}
+			for k, v := range tc.envs {
+				t.Setenv(k, v)
+			}
+
+			cfg := DefaultConfig()
+			resolveEnv(&cfg)
+			tc.check(t, cfg)
+		})
+	}
+}
+
+func TestLoadFromJSON(t *testing.T) {
+	t.Run("nonexistent file returns nil error", func(t *testing.T) {
+		cfg := DefaultConfig()
+		err := LoadFromJSON(&cfg, "/nonexistent/path/config.json")
+		if err != nil {
+			t.Errorf("expected nil error for missing file, got %v", err)
+		}
+	})
+
+	t.Run("malformed json returns nil error", func(t *testing.T) {
+		tmp := t.TempDir()
+		path := filepath.Join(tmp, "config.json")
+		os.WriteFile(path, []byte("{invalid json"), 0644)
+
+		cfg := DefaultConfig()
+		err := LoadFromJSON(&cfg, path)
+		if err != nil {
+			t.Errorf("expected nil error for malformed JSON, got %v", err)
+		}
+	})
+
+	t.Run("no telemetry section", func(t *testing.T) {
+		tmp := t.TempDir()
+		path := filepath.Join(tmp, "config.json")
+		os.WriteFile(path, []byte(`{"other": "value"}`), 0644)
+
+		cfg := DefaultConfig()
+		err := LoadFromJSON(&cfg, path)
+		if err != nil {
+			t.Errorf("unexpected error: %v", err)
+		}
+		if cfg.Enabled {
+			t.Error("expected Enabled to remain false")
+		}
+	})
+
+	t.Run("all fields set", func(t *testing.T) {
+		tmp := t.TempDir()
+		path := filepath.Join(tmp, "config.json")
+		data := `{
+			"telemetry": {
+				"enabled": true,
+				"exporter": "otlp",
+				"otlp_endpoint": "collector:4317",
+				"content_logging": true
+			}
+		}`
+		os.WriteFile(path, []byte(data), 0644)
+
+		cfg := DefaultConfig()
+		err := LoadFromJSON(&cfg, path)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if !cfg.Enabled {
+			t.Error("expected Enabled=true")
+		}
+		if cfg.Exporter != "otlp" {
+			t.Errorf("expected Exporter=otlp, got %s", cfg.Exporter)
+		}
+		if cfg.OTLPEndpoint != "collector:4317" {
+			t.Errorf("expected OTLPEndpoint=collector:4317, got %s", cfg.OTLPEndpoint)
+		}
+		if !cfg.ContentLog {
+			t.Error("expected ContentLog=true")
+		}
+	})
+
+	t.Run("otlp_endpoint auto-sets exporter to otlp", func(t *testing.T) {
+		tmp := t.TempDir()
+		path := filepath.Join(tmp, "config.json")
+		data := `{"telemetry": {"otlp_endpoint": "localhost:4317"}}`
+		os.WriteFile(path, []byte(data), 0644)
+
+		cfg := DefaultConfig()
+		err := LoadFromJSON(&cfg, path)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if cfg.Exporter != "otlp" {
+			t.Errorf("expected Exporter=otlp, got %s", cfg.Exporter)
+		}
+	})
+
+	t.Run("exporter not overridden if already non-default", func(t *testing.T) {
+		tmp := t.TempDir()
+		path := filepath.Join(tmp, "config.json")
+		data := `{"telemetry": {"exporter": "otlp"}}`
+		os.WriteFile(path, []byte(data), 0644)
+
+		cfg := DefaultConfig()
+		cfg.Exporter = "custom"
+		err := LoadFromJSON(&cfg, path)
+		if err != nil {
+			t.Fatalf("unexpected error: %v", err)
+		}
+		if cfg.Exporter != "custom" {
+			t.Errorf("expected Exporter to remain custom, got %s", cfg.Exporter)
+		}
+	})
+}
+
+func TestResolveConfig(t *testing.T) {
+	t.Run("empty path uses only env and defaults", func(t *testing.T) {
+		envKeys := []string{
+			"OCR_ENABLE_TELEMETRY", "OTEL_SERVICE_NAME",
+			"OTEL_EXPORTER_OTLP_ENDPOINT", "OTEL_EXPORTER_OTLP_PROTOCOL",
+			"OCR_CONTENT_LOGGING",
+		}
+		for _, k := range envKeys {
+			t.Setenv(k, "")
+			os.Unsetenv(k)
+		}
+
+		cfg := ResolveConfig("")
+		if cfg.Enabled {
+			t.Error("expected disabled with no env")
+		}
+	})
+
+	t.Run("env overrides json", func(t *testing.T) {
+		tmp := t.TempDir()
+		path := filepath.Join(tmp, "config.json")
+		data := `{"telemetry": {"enabled": false}}`
+		os.WriteFile(path, []byte(data), 0644)
+
+		t.Setenv("OCR_ENABLE_TELEMETRY", "1")
+
+		cfg := ResolveConfig(path)
+		if !cfg.Enabled {
+			t.Error("expected env to override json: Enabled should be true")
+		}
+	})
+}
+
+func TestHomeConfigPath(t *testing.T) {
+	path := HomeConfigPath()
+	if path == "" {
+		t.Skip("could not determine home dir")
+	}
+	if !filepath.IsAbs(path) {
+		t.Errorf("expected absolute path, got %s", path)
+	}
+	if filepath.Base(path) != "config.json" {
+		t.Errorf("expected config.json at end, got %s", filepath.Base(path))
+	}
+}

internal/telemetry/events_test.go

@@ -0,0 +1,50 @@
+package telemetry
+
+import (
+	"testing"
+	"time"
+)
+
+func TestFormatDuration(t *testing.T) {
+	tests := []struct {
+		dur  time.Duration
+		want string
+	}{
+		{0, "0s"},
+		{1500 * time.Millisecond, "1.5s"},
+		{60 * time.Second, "1m0s"},
+		{123 * time.Millisecond, "123ms"},
+		{2*time.Minute + 30*time.Second, "2m30s"},
+	}
+	for _, tc := range tests {
+		got := FormatDuration(tc.dur)
+		if got != tc.want {
+			t.Errorf("FormatDuration(%v) = %q, want %q", tc.dur, got, tc.want)
+		}
+	}
+}
+
+func TestSummarizeArgs(t *testing.T) {
+	tests := []struct {
+		name string
+		args map[string]any
+		want string
+	}{
+		{"nil map", nil, ""},
+		{"empty map", map[string]any{}, ""},
+		{"path key returns quoted", map[string]any{"path": "foo/bar.go"}, `"foo/bar.go"`},
+		{"search key returns quoted", map[string]any{"search": "hello"}, `"hello"`},
+		{"query key returns quoted", map[string]any{"query": "world"}, `"world"`},
+		{"pattern key returns quoted", map[string]any{"pattern": "*.go"}, `"*.go"`},
+		{"generic short value", map[string]any{"foo": "bar"}, "foo=bar"},
+		{"long value skipped", map[string]any{"data": string(make([]byte, 60))}, ""},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := summarizeArgs(tc.args)
+			if got != tc.want {
+				t.Errorf("summarizeArgs(%v) = %q, want %q", tc.args, got, tc.want)
+			}
+		})
+	}
+}

internal/telemetry/metrics_test.go

@@ -0,0 +1,30 @@
+package telemetry
+
+import (
+	"context"
+	"fmt"
+	"testing"
+	"time"
+)
+
+func TestRecordFunctions_DisabledTelemetry(t *testing.T) {
+	// Reset state so telemetry is disabled
+	initialized = false
+	shutdownFuncs = nil
+
+	ctx := context.Background()
+
+	// These should all be no-ops when telemetry is disabled
+	RecordReviewDuration(ctx, 5*time.Second)
+	RecordFilesReviewed(ctx, 10)
+	RecordCommentsGenerated(ctx, 3)
+	RecordLLMRequest(ctx, "gpt-4", 2*time.Second, 1000, "ok")
+	RecordToolCall(ctx, "file_read", 100*time.Millisecond, true)
+	RecordToolCall(ctx, "file_read", 100*time.Millisecond, false)
+}
+
+func TestCheckMetricErr(t *testing.T) {
+	// Should not panic
+	checkMetricErr(nil)
+	checkMetricErr(fmt.Errorf("some error"))
+}

internal/telemetry/provider_test.go

@@ -0,0 +1,52 @@
+package telemetry
+
+import (
+	"context"
+	"testing"
+)
+
+func TestIsEnabled_NotInitialized(t *testing.T) {
+	// Reset global state
+	initialized = false
+	shutdownFuncs = nil
+
+	if IsEnabled() {
+		t.Error("expected IsEnabled()=false when not initialized")
+	}
+}
+
+func TestIsEnabled_InitializedButNoShutdowns(t *testing.T) {
+	initialized = true
+	shutdownFuncs = nil
+	defer func() {
+		initialized = false
+	}()
+
+	if IsEnabled() {
+		t.Error("expected IsEnabled()=false when no shutdown funcs registered")
+	}
+}
+
+func TestIsEnabled_WithShutdowns(t *testing.T) {
+	initialized = true
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error { return nil },
+	}
+	defer func() {
+		initialized = false
+		shutdownFuncs = nil
+	}()
+
+	if !IsEnabled() {
+		t.Error("expected IsEnabled()=true when shutdown funcs registered")
+	}
+}
+
+func TestContentLogging_Disabled(t *testing.T) {
+	initialized = false
+	shutdownFuncs = nil
+
+	if ContentLogging() {
+		t.Error("expected ContentLogging()=false when telemetry disabled")
+	}
+}

internal/telemetry/shutdown_test.go

@@ -0,0 +1,71 @@
+package telemetry
+
+import (
+	"context"
+	"errors"
+	"testing"
+	"time"
+)
+
+func TestShutdown_NoFuncs(t *testing.T) {
+	shutdownFuncs = nil
+	err := Shutdown(context.Background())
+	if err != nil {
+		t.Errorf("expected nil error, got %v", err)
+	}
+}
+
+func TestShutdown_Success(t *testing.T) {
+	called := false
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error {
+			called = true
+			return nil
+		},
+	}
+	defer func() { shutdownFuncs = nil }()
+
+	err := Shutdown(context.Background())
+	if err != nil {
+		t.Errorf("expected nil error, got %v", err)
+	}
+	if !called {
+		t.Error("expected shutdown function to be called")
+	}
+	if shutdownFuncs != nil {
+		t.Error("expected shutdownFuncs to be cleared after shutdown")
+	}
+}
+
+func TestShutdown_WithErrors(t *testing.T) {
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error { return nil },
+		func(ctx context.Context) error { return errors.New("fail1") },
+		func(ctx context.Context) error { return errors.New("fail2") },
+	}
+	defer func() { shutdownFuncs = nil }()
+
+	err := Shutdown(context.Background())
+	if err == nil {
+		t.Fatal("expected error")
+	}
+	if shutdownFuncs != nil {
+		t.Error("expected shutdownFuncs to be cleared even on error")
+	}
+}
+
+func TestShutdownWithTimeout(t *testing.T) {
+	called := false
+	shutdownFuncs = []func(context.Context) error{
+		func(ctx context.Context) error {
+			called = true
+			return nil
+		},
+	}
+	defer func() { shutdownFuncs = nil }()
+
+	ShutdownWithTimeout(context.Background(), 5*time.Second)
+	if !called {
+		t.Error("expected shutdown function to be called via ShutdownWithTimeout")
+	}
+}

internal/telemetry/span_test.go

@@ -0,0 +1,43 @@
+package telemetry
+
+import (
+	"fmt"
+	"testing"
+
+	"go.opentelemetry.io/otel/attribute"
+)
+
+func TestAnyToAttr(t *testing.T) {
+	tests := []struct {
+		name string
+		key  string
+		val  interface{}
+		want attribute.KeyValue
+	}{
+		{"string", "k", "hello", attribute.String("k", "hello")},
+		{"int", "k", 42, attribute.Int64("k", 42)},
+		{"int64", "k", int64(100), attribute.Int64("k", 100)},
+		{"bool", "k", true, attribute.Bool("k", true)},
+		{"float64", "k", 3.14, attribute.Float64("k", 3.14)},
+		{"default fallback", "k", []int{1, 2}, attribute.String("k", fmt.Sprintf("%v", []int{1, 2}))},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			got := AnyToAttr(tc.key, tc.val)
+			if got != tc.want {
+				t.Errorf("AnyToAttr(%q, %v) = %v, want %v", tc.key, tc.val, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestSetAttr_NilSpan(t *testing.T) {
+	// Should not panic
+	SetAttr(nil, "key", "value")
+}
+
+func TestRecordToolResult_NilSpan(t *testing.T) {
+	// Should not panic
+	RecordToolResult(nil, "tool", 100, nil)
+	RecordToolResult(nil, "tool", 100, fmt.Errorf("err"))
+}