feat: improve test coverage for cmd/review-bot

Partially addresses #32

- Tests for setupLogger, isPatternFile, evaluateCIStatus, envOrDefault*, validateReviewerName
- Subprocess tests for main() error paths (version, missing flags, invalid inputs)
- Integration test scaffold (build tag: integration)
- Makefile with build/test/lint/coverage targets
- Coverage: 16.7% → 42.3% for cmd/review-bot

cmd/review-bot/main_test.go

@@ -2,7 +2,10 @@ package main
 
 import (
 	"bytes"
+	"flag"
 	"log/slog"
+	"os"
+	"os/exec"
 	"strings"
 	"testing"
 
@@ -365,3 +368,480 @@ func TestSetupLogger_Integration(t *testing.T) {
 	setupLogger("text", "unknown") // should default to info
 	setupLogger("invalid", "info") // should default to text
 }
+
+func TestIsPatternFile(t *testing.T) {
+	tests := []struct {
+		path string
+		want bool
+	}{
+		{"README.md", true},
+		{"docs/GUIDE.MD", true},
+		{"config.yml", true},
+		{"config.yaml", true},
+		{"notes.txt", true},
+		{"NOTES.TXT", true},
+		{"main.go", false},
+		{"lib.rs", false},
+		{"index.js", false},
+		{"Makefile", false},
+		{"", false},
+		{"doc.pdf", false},
+		{"patterns.Yml", true},
+		{"deep/path/file.yaml", true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.path, func(t *testing.T) {
+			got := isPatternFile(tc.path)
+			if got != tc.want {
+				t.Errorf("isPatternFile(%q) = %v, want %v", tc.path, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestEvaluateCIStatus(t *testing.T) {
+	tests := []struct {
+		name       string
+		statuses   []gitea.CommitStatus
+		wantPassed bool
+		wantSubstr string
+	}{
+		{
+			name:       "empty statuses",
+			statuses:   nil,
+			wantPassed: true,
+			wantSubstr: "no CI statuses",
+		},
+		{
+			name: "all success",
+			statuses: []gitea.CommitStatus{
+				{Status: "success", Context: "ci/build", Description: "Build passed"},
+				{Status: "success", Context: "ci/test", Description: "Tests passed"},
+			},
+			wantPassed: true,
+			wantSubstr: "all checks passed",
+		},
+		{
+			name: "one failure",
+			statuses: []gitea.CommitStatus{
+				{Status: "success", Context: "ci/build", Description: "Build passed"},
+				{Status: "failure", Context: "ci/test", Description: "Tests failed"},
+			},
+			wantPassed: false,
+			wantSubstr: "ci/test",
+		},
+		{
+			name: "error status",
+			statuses: []gitea.CommitStatus{
+				{Status: "error", Context: "ci/lint", Description: "Lint error"},
+			},
+			wantPassed: false,
+			wantSubstr: "ci/lint",
+		},
+		{
+			name: "pending treated as not-failed",
+			statuses: []gitea.CommitStatus{
+				{Status: "pending", Context: "ci/build", Description: "In progress"},
+				{Status: "success", Context: "ci/test", Description: "Tests passed"},
+			},
+			wantPassed: true,
+			wantSubstr: "all checks passed",
+		},
+		{
+			name: "multiple failures",
+			statuses: []gitea.CommitStatus{
+				{Status: "failure", Context: "ci/build", Description: "Build failed"},
+				{Status: "failure", Context: "ci/test", Description: "Tests failed"},
+			},
+			wantPassed: false,
+			wantSubstr: "ci/build",
+		},
+		{
+			name: "mixed with pending and failure",
+			statuses: []gitea.CommitStatus{
+				{Status: "success", Context: "ci/build", Description: "Build passed"},
+				{Status: "pending", Context: "ci/deploy", Description: "Deploying"},
+				{Status: "failure", Context: "ci/test", Description: "Tests failed"},
+			},
+			wantPassed: false,
+			wantSubstr: "ci/test",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			passed, details := evaluateCIStatus(tc.statuses)
+			if passed != tc.wantPassed {
+				t.Errorf("evaluateCIStatus() passed = %v, want %v", passed, tc.wantPassed)
+			}
+			if !strings.Contains(details, tc.wantSubstr) {
+				t.Errorf("evaluateCIStatus() details = %q, want substring %q", details, tc.wantSubstr)
+			}
+		})
+	}
+}
+
+func TestEnvOrDefault(t *testing.T) {
+	// Test with unset env var
+	os.Unsetenv("TEST_ENV_OR_DEFAULT_UNSET")
+	got := envOrDefault("TEST_ENV_OR_DEFAULT_UNSET", "fallback")
+	if got != "fallback" {
+		t.Errorf("envOrDefault(unset) = %q, want %q", got, "fallback")
+	}
+
+	// Test with set env var
+	os.Setenv("TEST_ENV_OR_DEFAULT_SET", "custom")
+	defer os.Unsetenv("TEST_ENV_OR_DEFAULT_SET")
+	got = envOrDefault("TEST_ENV_OR_DEFAULT_SET", "fallback")
+	if got != "custom" {
+		t.Errorf("envOrDefault(set) = %q, want %q", got, "custom")
+	}
+
+	// Test with empty env var (should return default)
+	os.Setenv("TEST_ENV_OR_DEFAULT_EMPTY", "")
+	defer os.Unsetenv("TEST_ENV_OR_DEFAULT_EMPTY")
+	got = envOrDefault("TEST_ENV_OR_DEFAULT_EMPTY", "fallback")
+	if got != "fallback" {
+		t.Errorf("envOrDefault(empty) = %q, want %q", got, "fallback")
+	}
+}
+
+func TestEnvOrDefaultFloat(t *testing.T) {
+	// Test with unset env var
+	os.Unsetenv("TEST_ENV_FLOAT_UNSET")
+	got := envOrDefaultFloat("TEST_ENV_FLOAT_UNSET", 1.5)
+	if got != 1.5 {
+		t.Errorf("envOrDefaultFloat(unset) = %f, want %f", got, 1.5)
+	}
+
+	// Test with valid float
+	os.Setenv("TEST_ENV_FLOAT_SET", "2.7")
+	defer os.Unsetenv("TEST_ENV_FLOAT_SET")
+	got = envOrDefaultFloat("TEST_ENV_FLOAT_SET", 1.5)
+	if got != 2.7 {
+		t.Errorf("envOrDefaultFloat(set) = %f, want %f", got, 2.7)
+	}
+
+	// Test with invalid float (should return default)
+	os.Setenv("TEST_ENV_FLOAT_INVALID", "not-a-number")
+	defer os.Unsetenv("TEST_ENV_FLOAT_INVALID")
+	got = envOrDefaultFloat("TEST_ENV_FLOAT_INVALID", 3.14)
+	if got != 3.14 {
+		t.Errorf("envOrDefaultFloat(invalid) = %f, want %f", got, 3.14)
+	}
+
+	// Test with empty string (should return default)
+	os.Setenv("TEST_ENV_FLOAT_EMPTY", "")
+	defer os.Unsetenv("TEST_ENV_FLOAT_EMPTY")
+	got = envOrDefaultFloat("TEST_ENV_FLOAT_EMPTY", 0.5)
+	if got != 0.5 {
+		t.Errorf("envOrDefaultFloat(empty) = %f, want %f", got, 0.5)
+	}
+}
+
+func TestEnvOrDefaultInt(t *testing.T) {
+	// Test with unset env var
+	os.Unsetenv("TEST_ENV_INT_UNSET")
+	got := envOrDefaultInt("TEST_ENV_INT_UNSET", 42)
+	if got != 42 {
+		t.Errorf("envOrDefaultInt(unset) = %d, want %d", got, 42)
+	}
+
+	// Test with valid int
+	os.Setenv("TEST_ENV_INT_SET", "100")
+	defer os.Unsetenv("TEST_ENV_INT_SET")
+	got = envOrDefaultInt("TEST_ENV_INT_SET", 42)
+	if got != 100 {
+		t.Errorf("envOrDefaultInt(set) = %d, want %d", got, 100)
+	}
+
+	// Test with invalid int (should return default)
+	os.Setenv("TEST_ENV_INT_INVALID", "abc")
+	defer os.Unsetenv("TEST_ENV_INT_INVALID")
+	got = envOrDefaultInt("TEST_ENV_INT_INVALID", 42)
+	if got != 42 {
+		t.Errorf("envOrDefaultInt(invalid) = %d, want %d", got, 42)
+	}
+
+	// Test with empty string (should return default)
+	os.Setenv("TEST_ENV_INT_EMPTY", "")
+	defer os.Unsetenv("TEST_ENV_INT_EMPTY")
+	got = envOrDefaultInt("TEST_ENV_INT_EMPTY", 99)
+	if got != 99 {
+		t.Errorf("envOrDefaultInt(empty) = %d, want %d", got, 99)
+	}
+
+	// Test with negative int
+	os.Setenv("TEST_ENV_INT_NEG", "-5")
+	defer os.Unsetenv("TEST_ENV_INT_NEG")
+	got = envOrDefaultInt("TEST_ENV_INT_NEG", 42)
+	if got != -5 {
+		t.Errorf("envOrDefaultInt(negative) = %d, want %d", got, -5)
+	}
+}
+
+func TestEnvOrDefaultBool(t *testing.T) {
+	tests := []struct {
+		name       string
+		envVal     string
+		setEnv     bool
+		defaultVal bool
+		want       bool
+	}{
+		{"unset returns default true", "", false, true, true},
+		{"unset returns default false", "", false, false, false},
+		{"true", "true", true, false, true},
+		{"TRUE", "TRUE", true, false, true},
+		{"True", "True", true, false, true},
+		{"1", "1", true, false, true},
+		{"yes", "yes", true, false, true},
+		{"YES", "YES", true, false, true},
+		{"false", "false", true, true, false},
+		{"0", "0", true, true, false},
+		{"no", "no", true, true, false},
+		{"random string", "random", true, true, false},
+		{"empty string returns default", "", true, true, true},
+		{"whitespace true", "  true  ", true, false, true},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			envKey := "TEST_ENV_BOOL_" + strings.ReplaceAll(tc.name, " ", "_")
+			if tc.setEnv {
+				os.Setenv(envKey, tc.envVal)
+				defer os.Unsetenv(envKey)
+			} else {
+				os.Unsetenv(envKey)
+			}
+			got := envOrDefaultBool(envKey, tc.defaultVal)
+			if got != tc.want {
+				t.Errorf("envOrDefaultBool(%q, %v) = %v, want %v", tc.envVal, tc.defaultVal, got, tc.want)
+			}
+		})
+	}
+}
+
+func TestExtractSentinelName_EdgeCases(t *testing.T) {
+	tests := []struct {
+		body string
+		want string
+	}{
+		{"<!-- review-bot:sonnet --> rest", "sonnet"},
+		{"<!-- review-bot:gpt-review --> rest", "gpt-review"},
+		{"no sentinel here", "unknown"},
+		{"<!-- review-bot:", "unknown"},       // prefix but no suffix
+		{"prefix <!-- review-bot:abc --> end", "abc"}, // embedded in text
+	}
+
+	for _, tc := range tests {
+		got := extractSentinelName(tc.body)
+		if got != tc.want {
+			t.Errorf("extractSentinelName(%q) = %q, want %q", tc.body, got, tc.want)
+		}
+	}
+}
+
+// TestMainSubprocess runs main() as a subprocess using the test binary itself.
+// This allows coverage to be captured for main() code paths.
+func TestMainSubprocess_Version(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		// Reset flags for main()
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot", "--version"}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_Version")
+	cmd.Env = append(os.Environ(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		t.Fatalf("--version subprocess failed: %v\n%s", err, out)
+	}
+	if !strings.Contains(string(out), "review-bot") {
+		t.Errorf("--version output = %q, want to contain 'review-bot'", string(out))
+	}
+}
+
+func TestMainSubprocess_MissingFlags(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		// Reset flags for main()
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot"}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_MissingFlags")
+	cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit with no flags, got success")
+	}
+	if !strings.Contains(string(out), "missing required") {
+		t.Errorf("expected error about missing flags, got: %s", out)
+	}
+}
+
+func TestMainSubprocess_InvalidReviewerName(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot",
+			"--gitea-url", "http://localhost",
+			"--repo", "owner/repo",
+			"--pr", "1",
+			"--reviewer-name", "invalid name",
+			"--reviewer-token", "tok",
+			"--llm-base-url", "http://localhost",
+			"--llm-api-key", "key",
+			"--llm-model", "model",
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidReviewerName")
+	cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit with invalid reviewer-name, got success")
+	}
+	if !strings.Contains(string(out), "invalid reviewer name") {
+		t.Errorf("expected error about invalid reviewer name, got: %s", out)
+	}
+}
+
+func TestMainSubprocess_InvalidRepo(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot",
+			"--gitea-url", "http://localhost",
+			"--repo", "invalidrepo",
+			"--pr", "1",
+			"--reviewer-token", "tok",
+			"--llm-base-url", "http://localhost",
+			"--llm-api-key", "key",
+			"--llm-model", "model",
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidRepo")
+	cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit with invalid repo format")
+	}
+	if !strings.Contains(string(out), "invalid repo format") {
+		t.Errorf("expected error about invalid repo, got: %s", out)
+	}
+}
+
+func TestMainSubprocess_InvalidPRNumber(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot",
+			"--gitea-url", "http://localhost",
+			"--repo", "owner/repo",
+			"--pr", "notanumber",
+			"--reviewer-token", "tok",
+			"--llm-base-url", "http://localhost",
+			"--llm-api-key", "key",
+			"--llm-model", "model",
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidPRNumber")
+	cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit with invalid PR number")
+	}
+	if !strings.Contains(string(out), "invalid PR number") {
+		t.Errorf("expected error about invalid PR number, got: %s", out)
+	}
+}
+
+func TestMainSubprocess_InvalidTemperature(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot",
+			"--gitea-url", "http://localhost",
+			"--repo", "owner/repo",
+			"--pr", "1",
+			"--reviewer-token", "tok",
+			"--llm-base-url", "http://localhost",
+			"--llm-api-key", "key",
+			"--llm-model", "model",
+			"--llm-temperature", "5.0",
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidTemperature")
+	cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit with invalid temperature")
+	}
+	if !strings.Contains(string(out), "invalid LLM temperature") {
+		t.Errorf("expected error about invalid temperature, got: %s", out)
+	}
+}
+
+func TestMainSubprocess_InvalidProvider(t *testing.T) {
+	if os.Getenv("TEST_SUBPROCESS_MAIN") == "1" {
+		flag.CommandLine = flag.NewFlagSet(os.Args[0], flag.ExitOnError)
+		os.Args = []string{"review-bot",
+			"--gitea-url", "http://localhost",
+			"--repo", "owner/repo",
+			"--pr", "1",
+			"--reviewer-token", "tok",
+			"--llm-base-url", "http://localhost",
+			"--llm-api-key", "key",
+			"--llm-model", "model",
+			"--llm-provider", "invalid-provider",
+		}
+		main()
+		return
+	}
+
+	cmd := exec.Command(os.Args[0], "-test.run=TestMainSubprocess_InvalidProvider")
+	cmd.Env = append(cleanEnv(), "TEST_SUBPROCESS_MAIN=1")
+	out, err := cmd.CombinedOutput()
+	if err == nil {
+		t.Fatal("expected non-zero exit with invalid provider")
+	}
+	if !strings.Contains(string(out), "invalid LLM provider") {
+		t.Errorf("expected error about invalid provider, got: %s", out)
+	}
+}
+
+// cleanEnv returns environ without any GITEA/LLM/REVIEWER env vars that would
+// interfere with testing missing-flag scenarios.
+func cleanEnv() []string {
+	var env []string
+	for _, e := range os.Environ() {
+		key := strings.SplitN(e, "=", 2)[0]
+		switch {
+		case strings.HasPrefix(key, "GITEA_"),
+			strings.HasPrefix(key, "LLM_"),
+			strings.HasPrefix(key, "REVIEWER_"),
+			strings.HasPrefix(key, "PR_"),
+			strings.HasPrefix(key, "LOG_"),
+			strings.HasPrefix(key, "CONVENTIONS_"),
+			strings.HasPrefix(key, "SYSTEM_PROMPT_"),
+			strings.HasPrefix(key, "PATTERNS_"),
+			strings.HasPrefix(key, "UPDATE_"):
+			continue
+		default:
+			env = append(env, e)
+		}
+	}
+	return env
+}