From 840f85f9bc5be032c0d397de6aa57b196b41c554 Mon Sep 17 00:00:00 2001 From: kite <254839944+lizhengfeng101@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:55:44 +0800 Subject: [PATCH] test: raise statement coverage to 90% and enforce it in CI (#747) * test: raise statement coverage to 90% and enforce it in CI Add unit tests across the cmd and internal packages to bring total statement coverage above 90%, and gate future regressions. - Cover CLI helpers, provider TUI handlers, resume/manifest paths, and error branches in config, llm, llmloop, scan, session, agent, viewer, mcp, pathutil, and telemetry. - Raise the coverage threshold from 80% to 90% in the Makefile (COVERAGE_THRESHOLD) and in the CI "Check coverage threshold" step. - Ignore generated coverage.out and coverage.html artifacts. Total statement coverage is now 90.5%, measured consistently by both `make coverage` and the CI `go test ./...` scope. * test: widen statement coverage margin with environment-independent unit tests Add table-driven unit tests for pure, environment-independent functions to raise the statement-coverage safety margin above the 90% threshold: - session.ResumeState.ValidateScanOptions (70% -> 100%) - rules.SystemRule.UnmarshalJSON error branches (71% -> 82%) - llmloop.stripMarkdownFences no-newline branch (82% -> 100%) - diff.firstLine empty/blank-input branch - diff.extractCodeBlock missing-newline and no-closing-fence branches - main.truncate n<=1 and normalization branches - agent.Agent nil-receiver accessor guards --- .github/workflows/ci.yml | 6 +- .gitignore | 2 + Makefile | 2 +- .../apply_provider_field_test.go | 88 ++++ cmd/opencodereview/config_cmd_test.go | 15 + cmd/opencodereview/config_runset_test.go | 148 +++++++ cmd/opencodereview/config_unset_error_test.go | 42 ++ cmd/opencodereview/delegate_exec_test.go | 123 ++++++ cmd/opencodereview/delegate_helpers_test.go | 70 ++++ cmd/opencodereview/flag_suggest_test.go | 96 +++++ cmd/opencodereview/misc_helpers_test.go | 91 +++++ cmd/opencodereview/output_manifest_test.go | 137 +++++++ .../provider_config_apply_test.go | 61 +++ .../provider_tui_cpinput_test.go | 109 +++++ .../provider_tui_customform_test.go | 211 ++++++++++ .../provider_tui_deleteconfirm_test.go | 65 +++ .../provider_tui_editsave_test.go | 111 +++++ .../provider_tui_manualenter_test.go | 192 +++++++++ .../provider_tui_modeltui_test.go | 265 ++++++++++++ .../provider_tui_persist_test.go | 378 ++++++++++++++++++ .../provider_tui_rollback_test.go | 226 +++++++++++ .../provider_tui_savefail_test.go | 150 +++++++ cmd/opencodereview/review_helpers_test.go | 85 ++++ cmd/opencodereview/review_mcp_more_test.go | 81 ++++ cmd/opencodereview/review_resume_more_test.go | 71 ++++ cmd/opencodereview/rules_check_test.go | 36 ++ cmd/opencodereview/scan_helpers_test.go | 46 +++ cmd/opencodereview/scan_resume_more_test.go | 69 ++++ cmd/opencodereview/session_cmd_test.go | 24 ++ cmd/opencodereview/session_complete_test.go | 43 ++ .../session_display_more_test.go | 99 +++++ cmd/opencodereview/shared_llmruntime_test.go | 104 +++++ internal/agent/agent_test.go | 25 ++ internal/agent/getters_test.go | 28 ++ internal/agent/preview_run_test.go | 64 +++ internal/config/rules/system_rules_test.go | 97 +++++ .../rules/system_rules_unmarshal_test.go | 74 ++++ internal/diff/first_line_test.go | 29 ++ internal/diff/relocation_test.go | 2 + internal/llm/client_params_test.go | 195 +++++++++ internal/llm/resolver_norm_test.go | 70 ++++ internal/llm/resolver_shellrc_test.go | 63 +++ internal/llmloop/compression_test.go | 10 + internal/llmloop/loop_execute_more_test.go | 164 ++++++++ internal/llmloop/loop_execute_test.go | 146 +++++++ internal/mcp/client_test.go | 22 + internal/pathutil/path_test.go | 3 + internal/scan/getters_more_test.go | 85 ++++ internal/scan/getters_test.go | 29 ++ internal/scan/provider_more_test.go | 146 +++++++ internal/session/final_manifest_test.go | 33 ++ internal/session/list_error_test.go | 56 +++ internal/session/list_more_test.go | 92 +++++ internal/session/manifest_guards_test.go | 97 +++++ .../session/validate_scan_options_test.go | 72 ++++ internal/telemetry/traceid_test.go | 33 ++ internal/viewer/server_startserver_test.go | 74 ++++ internal/viewer/store_load_test.go | 63 +++ 58 files changed, 5014 insertions(+), 4 deletions(-) create mode 100644 cmd/opencodereview/apply_provider_field_test.go create mode 100644 cmd/opencodereview/config_runset_test.go create mode 100644 cmd/opencodereview/config_unset_error_test.go create mode 100644 cmd/opencodereview/delegate_exec_test.go create mode 100644 cmd/opencodereview/delegate_helpers_test.go create mode 100644 cmd/opencodereview/flag_suggest_test.go create mode 100644 cmd/opencodereview/misc_helpers_test.go create mode 100644 cmd/opencodereview/output_manifest_test.go create mode 100644 cmd/opencodereview/provider_config_apply_test.go create mode 100644 cmd/opencodereview/provider_tui_cpinput_test.go create mode 100644 cmd/opencodereview/provider_tui_customform_test.go create mode 100644 cmd/opencodereview/provider_tui_deleteconfirm_test.go create mode 100644 cmd/opencodereview/provider_tui_editsave_test.go create mode 100644 cmd/opencodereview/provider_tui_manualenter_test.go create mode 100644 cmd/opencodereview/provider_tui_modeltui_test.go create mode 100644 cmd/opencodereview/provider_tui_persist_test.go create mode 100644 cmd/opencodereview/provider_tui_rollback_test.go create mode 100644 cmd/opencodereview/provider_tui_savefail_test.go create mode 100644 cmd/opencodereview/review_helpers_test.go create mode 100644 cmd/opencodereview/review_mcp_more_test.go create mode 100644 cmd/opencodereview/review_resume_more_test.go create mode 100644 cmd/opencodereview/rules_check_test.go create mode 100644 cmd/opencodereview/scan_helpers_test.go create mode 100644 cmd/opencodereview/scan_resume_more_test.go create mode 100644 cmd/opencodereview/session_complete_test.go create mode 100644 cmd/opencodereview/session_display_more_test.go create mode 100644 cmd/opencodereview/shared_llmruntime_test.go create mode 100644 internal/agent/getters_test.go create mode 100644 internal/agent/preview_run_test.go create mode 100644 internal/config/rules/system_rules_unmarshal_test.go create mode 100644 internal/diff/first_line_test.go create mode 100644 internal/llm/client_params_test.go create mode 100644 internal/llm/resolver_norm_test.go create mode 100644 internal/llm/resolver_shellrc_test.go create mode 100644 internal/llmloop/loop_execute_more_test.go create mode 100644 internal/llmloop/loop_execute_test.go create mode 100644 internal/scan/getters_more_test.go create mode 100644 internal/scan/getters_test.go create mode 100644 internal/scan/provider_more_test.go create mode 100644 internal/session/final_manifest_test.go create mode 100644 internal/session/list_error_test.go create mode 100644 internal/session/list_more_test.go create mode 100644 internal/session/manifest_guards_test.go create mode 100644 internal/session/validate_scan_options_test.go create mode 100644 internal/telemetry/traceid_test.go create mode 100644 internal/viewer/server_startserver_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5b0a237..d6c13ae 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -64,11 +64,11 @@ jobs: run: | COVERAGE=$(go tool cover -func=coverage.out | grep total: | awk '{print $3}' | sed 's/%//') echo "Total coverage: ${COVERAGE}%" - if awk "BEGIN {exit !($COVERAGE < 80)}"; then - echo "FAIL: Coverage ${COVERAGE}% is below 80% threshold" + if awk "BEGIN {exit !($COVERAGE < 90)}"; then + echo "FAIL: Coverage ${COVERAGE}% is below 90% threshold" exit 1 fi - echo "PASS: Coverage ${COVERAGE}% meets 80% threshold" + echo "PASS: Coverage ${COVERAGE}% meets 90% threshold" - name: Build run: go build -o ./opencodereview ./cmd/opencodereview diff --git a/.gitignore b/.gitignore index e8cc9f2..291a15d 100644 --- a/.gitignore +++ b/.gitignore @@ -6,6 +6,8 @@ dist/ npm/*/bin/ +coverage.out +coverage.html .claude/plans .claude/commands/comment.md .claude/commands/read-issue.md diff --git a/Makefile b/Makefile index 1998310..9c75a73 100644 --- a/Makefile +++ b/Makefile @@ -37,7 +37,7 @@ PACKAGES := $(shell $(GO) list ./... | grep -v /extensions/) test: LC_ALL=C $(GO) test -v -race -count=1 $(PACKAGES) -COVERAGE_THRESHOLD := 80 +COVERAGE_THRESHOLD := 90 coverage: LC_ALL=C $(GO) test -count=1 -coverprofile=coverage.out $(PACKAGES) diff --git a/cmd/opencodereview/apply_provider_field_test.go b/cmd/opencodereview/apply_provider_field_test.go new file mode 100644 index 0000000..1b9a6e3 --- /dev/null +++ b/cmd/opencodereview/apply_provider_field_test.go @@ -0,0 +1,88 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" +) + +// TestApplyProviderField exercises every field branch of applyProviderField, +// including the JSON/parse error paths and the unknown-field default. +func TestApplyProviderField(t *testing.T) { + t.Run("success branches set the entry", func(t *testing.T) { + var e ProviderEntry + cases := []struct { + field, value string + check func(ProviderEntry) bool + }{ + {"api_key", "sk-x", func(e ProviderEntry) bool { return e.APIKey == "sk-x" }}, + {"url", "https://x.example", func(e ProviderEntry) bool { return e.URL == "https://x.example" }}, + {"model", "gpt-4", func(e ProviderEntry) bool { return e.Model == "gpt-4" }}, + {"models", "a,b,a", func(e ProviderEntry) bool { return len(e.Models) == 2 }}, + {"extra_body", `{"k":1}`, func(e ProviderEntry) bool { return e.ExtraBody["k"] != nil }}, + } + for _, c := range cases { + if err := applyProviderField(&e, c.field, "providers.p."+c.field, c.value); err != nil { + t.Fatalf("field %q: %v", c.field, err) + } + if !c.check(e) { + t.Errorf("field %q not applied: %+v", c.field, e) + } + } + }) + + t.Run("protocol validated and normalized", func(t *testing.T) { + var e ProviderEntry + if err := applyProviderField(&e, "protocol", "providers.p.protocol", "openai"); err != nil { + t.Fatalf("valid protocol: %v", err) + } + if e.Protocol == "" { + t.Error("protocol not set") + } + if err := applyProviderField(&e, "protocol", "providers.p.protocol", "not-a-protocol"); err == nil { + t.Error("expected error for invalid protocol") + } + }) + + t.Run("auth_header normalized", func(t *testing.T) { + var e ProviderEntry + if err := applyProviderField(&e, "auth_header", "providers.p.auth_header", "x-api-key"); err != nil { + t.Fatalf("valid auth header: %v", err) + } + if e.AuthHeader == "" { + t.Error("auth header not set") + } + }) + + t.Run("auth_header rejects unsupported value", func(t *testing.T) { + var e ProviderEntry + if err := applyProviderField(&e, "auth_header", "providers.p.auth_header", "cookie"); err == nil { + t.Error("expected error for unsupported auth header") + } + }) + + t.Run("extra_body rejects invalid JSON", func(t *testing.T) { + var e ProviderEntry + if err := applyProviderField(&e, "extra_body", "providers.p.extra_body", "{bad"); err == nil { + t.Error("expected JSON error") + } + }) + + t.Run("extra_headers parsed", func(t *testing.T) { + var e ProviderEntry + if err := applyProviderField(&e, "extra_headers", "providers.p.extra_headers", "X-A=1"); err != nil { + t.Fatalf("valid extra headers: %v", err) + } + if len(e.ExtraHeaders) == 0 { + t.Error("extra headers not set") + } + }) + + t.Run("unknown field returns error", func(t *testing.T) { + var e ProviderEntry + if err := applyProviderField(&e, "bogus", "providers.p.bogus", "x"); err == nil { + t.Error("expected error for unknown field") + } + }) +} diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index d4ee4a6..9498c74 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -1380,6 +1380,21 @@ func TestSetMCPServerValue_URLNoHost(t *testing.T) { } } +func TestSetMCPServerValue_URLParseError(t *testing.T) { + cfg := &Config{} + // "://bad" has no scheme, so url.Parse itself fails before the scheme check. + if err := setMCPServerValue(cfg, "mcp_servers.gh.url", "://bad"); err == nil { + t.Fatal("expected error for unparseable URL, got nil") + } +} + +func TestSetMCPServerValue_HeadersEmptyName(t *testing.T) { + cfg := &Config{} + if err := setMCPServerValue(cfg, "mcp_servers.gh.headers", `{"":"val"}`); err == nil { + t.Fatal("expected error for empty header name, got nil") + } +} + func TestSetMCPServerValue_HeadersInvalidJSON(t *testing.T) { cfg := &Config{} if err := setMCPServerValue(cfg, "mcp_servers.gh.headers", "not-json"); err == nil { diff --git a/cmd/opencodereview/config_runset_test.go b/cmd/opencodereview/config_runset_test.go new file mode 100644 index 0000000..37b2644 --- /dev/null +++ b/cmd/opencodereview/config_runset_test.go @@ -0,0 +1,148 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "strings" + "testing" +) + +// TestRunConfigSetPersists drives runConfigSet end-to-end (HOME points at a temp +// dir) covering the success path and the API-key masking branch. +func TestRunConfigSetPersists(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + + t.Run("plain value success", func(t *testing.T) { + out := captureStdout(t, func() { + if err := runConfigSet("language", "zh"); err != nil { + t.Fatalf("runConfigSet: %v", err) + } + }) + if !strings.Contains(out, "Set language = zh") { + t.Errorf("stdout = %q, want confirmation", out) + } + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload: %v", err) + } + if cfg.Language != "zh" { + t.Errorf("language = %q, want zh", cfg.Language) + } + }) + + t.Run("api key value is masked in output", func(t *testing.T) { + out := captureStdout(t, func() { + if err := runConfigSet("providers.openai.api_key", "sk-supersecretvalue"); err != nil { + t.Fatalf("runConfigSet: %v", err) + } + }) + if strings.Contains(out, "sk-supersecretvalue") { + t.Errorf("stdout leaks the raw API key: %q", out) + } + if !strings.Contains(out, "Set providers.openai.api_key") { + t.Errorf("stdout = %q, want confirmation line", out) + } + }) + + t.Run("invalid key returns error", func(t *testing.T) { + if err := runConfigSet("no_such_key", "x"); err == nil { + t.Error("expected error for unknown config key") + } + }) +} + +// TestRunConfigUnsetPaths drives runConfigUnset across its dispatch branches. +func TestRunConfigUnsetPaths(t *testing.T) { + t.Run("unset active provider clears provider and model", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + if err := saveConfig(configPath, &Config{Provider: "openai", Model: "gpt-4"}); err != nil { + t.Fatalf("save: %v", err) + } + out := captureStdout(t, func() { + if err := runConfigUnset("provider"); err != nil { + t.Fatalf("runConfigUnset: %v", err) + } + }) + if !strings.Contains(out, "Cleared active provider") { + t.Errorf("stdout = %q", out) + } + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload: %v", err) + } + if cfg.Provider != "" || cfg.Model != "" { + t.Errorf("provider/model not cleared: %q/%q", cfg.Provider, cfg.Model) + } + }) + + t.Run("unset custom provider", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "cp": {URL: "https://x.example", Protocol: "openai"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("save: %v", err) + } + out := captureStdout(t, func() { + if err := runConfigUnset("custom_providers.cp"); err != nil { + t.Fatalf("runConfigUnset: %v", err) + } + }) + if !strings.Contains(out, "Deleted custom provider") { + t.Errorf("stdout = %q", out) + } + }) + + t.Run("unset mcp server", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + configPath, err := defaultConfigPath() + if err != nil { + t.Fatal(err) + } + cfg := &Config{ + MCPServers: map[string]MCPServerConfig{ + "srv": {Type: "stdio", Command: "echo"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("save: %v", err) + } + out := captureStdout(t, func() { + if err := runConfigUnset("mcp_servers.srv"); err != nil { + t.Fatalf("runConfigUnset: %v", err) + } + }) + if !strings.Contains(out, "srv") { + t.Errorf("stdout = %q", out) + } + }) + + t.Run("malformed key returns error", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + if err := runConfigUnset("custom_providers"); err == nil { + t.Error("expected error for key without a name segment") + } + }) + + t.Run("unknown prefix returns error", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + if err := runConfigUnset("bogus.name"); err == nil { + t.Error("expected error for unknown unset prefix") + } + }) +} diff --git a/cmd/opencodereview/config_unset_error_test.go b/cmd/opencodereview/config_unset_error_test.go new file mode 100644 index 0000000..793e9f4 --- /dev/null +++ b/cmd/opencodereview/config_unset_error_test.go @@ -0,0 +1,42 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// TestUnset_LoadErrors covers the load-config error branch of each unset helper: +// when the config path holds invalid JSON, loadOrCreateConfig fails to parse it +// and the helper must surface the wrapped error rather than proceed. +func TestUnset_LoadErrors(t *testing.T) { + newBadConfig := func(t *testing.T) string { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + if err := os.WriteFile(path, []byte("{not valid json"), 0o600); err != nil { + t.Fatalf("write bad config: %v", err) + } + return path + } + + t.Run("unsetActiveProvider", func(t *testing.T) { + if err := unsetActiveProvider(newBadConfig(t)); err == nil { + t.Fatal("expected load error, got nil") + } + }) + + t.Run("unsetCustomProvider", func(t *testing.T) { + if err := unsetCustomProvider(newBadConfig(t), "any"); err == nil { + t.Fatal("expected load error, got nil") + } + }) + + t.Run("unsetMCPServer", func(t *testing.T) { + if err := unsetMCPServer(newBadConfig(t), "any"); err == nil { + t.Fatal("expected load error, got nil") + } + }) +} diff --git a/cmd/opencodereview/delegate_exec_test.go b/cmd/opencodereview/delegate_exec_test.go new file mode 100644 index 0000000..51330fd --- /dev/null +++ b/cmd/opencodereview/delegate_exec_test.go @@ -0,0 +1,123 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" +) + +// gitCommitFile writes a file and commits it, returning after the commit lands. +func gitCommitFile(t *testing.T, dir, name, content, msg string) { + t.Helper() + if err := os.WriteFile(filepath.Join(dir, name), []byte(content), 0o644); err != nil { + t.Fatalf("write %s: %v", name, err) + } + for _, args := range [][]string{{"add", "."}, {"commit", "-m", msg}} { + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } +} + +// silenceStdout redirects os.Stdout to /dev/null for the duration of fn so the +// delegate commands' Printf output does not clutter test logs. +func silenceStdout(t *testing.T, fn func()) { + t.Helper() + orig := os.Stdout + devnull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + t.Fatalf("open devnull: %v", err) + } + os.Stdout = devnull + defer func() { + os.Stdout = orig + _ = devnull.Close() + }() + fn() +} + +func TestExecuteDelegatePreview_Workspace(t *testing.T) { + dir := initTestGitRepo(t) + // Uncommitted change so the workspace preview has at least one entry. + if err := os.WriteFile(filepath.Join(dir, "app.go"), []byte("package app\n"), 0o644); err != nil { + t.Fatalf("write app.go: %v", err) + } + silenceStdout(t, func() { + if err := executeDelegatePreview(delegateOptions{repoDir: dir}); err != nil { + t.Fatalf("executeDelegatePreview(workspace) error: %v", err) + } + }) +} + +func TestExecuteDelegatePreview_Range(t *testing.T) { + dir := initTestGitRepo(t) + gitCommitFile(t, dir, "b.go", "package b\n", "second commit") + silenceStdout(t, func() { + err := executeDelegatePreview(delegateOptions{repoDir: dir, from: "HEAD~1", to: "HEAD"}) + if err != nil { + t.Fatalf("executeDelegatePreview(range) error: %v", err) + } + }) +} + +func TestExecuteDelegatePreview_Commit(t *testing.T) { + dir := initTestGitRepo(t) + gitCommitFile(t, dir, "c.go", "package c\n", "add c") + silenceStdout(t, func() { + // commit mode auto-fills background from the commit message. + err := executeDelegatePreview(delegateOptions{repoDir: dir, commit: "HEAD"}) + if err != nil { + t.Fatalf("executeDelegatePreview(commit) error: %v", err) + } + }) +} + +func TestExecuteDelegateRule(t *testing.T) { + dir := initTestGitRepo(t) + silenceStdout(t, func() { + err := executeDelegateRule(delegateOptions{repoDir: dir}, []string{"README.md"}) + if err != nil { + t.Fatalf("executeDelegateRule error: %v", err) + } + }) +} + +func TestLoadDelegateContext_BackgroundFile(t *testing.T) { + dir := initTestGitRepo(t) + bgPath := filepath.Join(dir, "bg.txt") + if err := os.WriteFile(bgPath, []byte("extra background"), 0o644); err != nil { + t.Fatalf("write bg: %v", err) + } + dc, err := loadDelegateContext(delegateOptions{repoDir: dir, backgroundFile: "bg.txt", background: "base"}) + if err != nil { + t.Fatalf("loadDelegateContext error: %v", err) + } + if dc.opts.background == "" { + t.Error("expected merged background, got empty") + } +} + +func TestLoadDelegateContext_NotGitRepo(t *testing.T) { + dir := t.TempDir() + if _, err := loadDelegateContext(delegateOptions{repoDir: dir}); err == nil { + t.Fatal("expected error for non-git dir") + } +} + +func TestDelegateContextMergeBase_Range(t *testing.T) { + dir := initTestGitRepo(t) + gitCommitFile(t, dir, "d.go", "package d\n", "add d") + dc, err := loadDelegateContext(delegateOptions{repoDir: dir, from: "HEAD~1", to: "HEAD"}) + if err != nil { + t.Fatalf("loadDelegateContext error: %v", err) + } + if got := dc.mergeBase(context.Background()); got == "" { + t.Error("expected non-empty merge base for range mode") + } +} diff --git a/cmd/opencodereview/delegate_helpers_test.go b/cmd/opencodereview/delegate_helpers_test.go new file mode 100644 index 0000000..10501a7 --- /dev/null +++ b/cmd/opencodereview/delegate_helpers_test.go @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "context" + "testing" +) + +func TestValidateDelegateOptions(t *testing.T) { + cases := []struct { + name string + opts delegateOptions + wantErr bool + }{ + {"workspace", delegateOptions{}, false}, + {"commit", delegateOptions{commit: "abc"}, false}, + {"range", delegateOptions{from: "main", to: "dev"}, false}, + {"from without to", delegateOptions{from: "main"}, true}, + {"to without from", delegateOptions{to: "dev"}, true}, + {"commit and range mixed", delegateOptions{commit: "abc", from: "main", to: "dev"}, true}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + err := validateDelegateOptions(&c.opts) + if (err != nil) != c.wantErr { + t.Errorf("validateDelegateOptions() err = %v, wantErr %v", err, c.wantErr) + } + }) + } +} + +func TestDelegateContextReviewMode(t *testing.T) { + cases := []struct { + name string + opts delegateOptions + want string + }{ + {"commit", delegateOptions{commit: "abc"}, "commit"}, + {"range", delegateOptions{from: "main", to: "dev"}, "range"}, + {"workspace", delegateOptions{}, "workspace"}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + dc := &delegateContext{cc: &commonContext{}, opts: c.opts} + if got := dc.reviewMode(); got != c.want { + t.Errorf("reviewMode() = %q, want %q", got, c.want) + } + }) + } +} + +func TestDelegateContextMergeBaseEmptyForNonRange(t *testing.T) { + dc := &delegateContext{cc: &commonContext{}, opts: delegateOptions{commit: "abc"}} + if got := dc.mergeBase(context.Background()); got != "" { + t.Errorf("mergeBase(commit mode) = %q, want empty", got) + } + dc = &delegateContext{cc: &commonContext{}, opts: delegateOptions{}} + if got := dc.mergeBase(context.Background()); got != "" { + t.Errorf("mergeBase(workspace mode) = %q, want empty", got) + } +} + +func TestDelegateContextResolver(t *testing.T) { + dc := &delegateContext{cc: &commonContext{}, opts: delegateOptions{}} + if got := dc.resolver(); got != nil { + t.Errorf("resolver() = %v, want nil", got) + } +} diff --git a/cmd/opencodereview/flag_suggest_test.go b/cmd/opencodereview/flag_suggest_test.go new file mode 100644 index 0000000..d8bfd68 --- /dev/null +++ b/cmd/opencodereview/flag_suggest_test.go @@ -0,0 +1,96 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "errors" + "strings" + "testing" + + "github.com/spf13/cobra" +) + +func TestLevenshtein(t *testing.T) { + cases := []struct { + a, b string + want int + }{ + {"", "", 0}, + {"", "abc", 3}, + {"abc", "", 3}, + {"abc", "abc", 0}, + {"format", "forma", 1}, + {"model", "modle", 2}, + {"kitten", "sitting", 3}, + } + for _, c := range cases { + if got := levenshtein(c.a, c.b); got != c.want { + t.Errorf("levenshtein(%q,%q) = %d, want %d", c.a, c.b, got, c.want) + } + } +} + +func TestSuggestFlag(t *testing.T) { + parent := &cobra.Command{Use: "parent"} + parent.PersistentFlags().String("repo", "", "") + child := &cobra.Command{Use: "child"} + child.Flags().String("format", "", "") + child.Flags().String("model", "", "") + parent.AddCommand(child) + + t.Run("close match on local flag", func(t *testing.T) { + if got := suggestFlag(child, "forma"); got == "" || !strings.Contains(got, "--format") { + t.Errorf("suggestFlag(forma) = %q, want suggestion for --format", got) + } + }) + t.Run("close match on inherited flag", func(t *testing.T) { + if got := suggestFlag(child, "rep"); got == "" || !strings.Contains(got, "--repo") { + t.Errorf("suggestFlag(rep) = %q, want suggestion for --repo", got) + } + }) + t.Run("no close match", func(t *testing.T) { + if got := suggestFlag(child, "zzzzzzzz"); got != "" { + t.Errorf("suggestFlag(zzzzzzzz) = %q, want empty", got) + } + }) + t.Run("empty after trimming dashes", func(t *testing.T) { + if got := suggestFlag(child, "--"); got != "" { + t.Errorf("suggestFlag(--) = %q, want empty", got) + } + }) +} + +func TestFlagErrorWithSuggestion(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + cmd.Flags().String("format", "", "") + + t.Run("unknown flag yields suggestion", func(t *testing.T) { + in := errors.New("unknown flag: --forma") + out := flagErrorWithSuggestion(cmd, in) + if !strings.Contains(out.Error(), "Did you mean") { + t.Errorf("expected suggestion, got %q", out.Error()) + } + }) + t.Run("unknown flag with no close match returns original", func(t *testing.T) { + in := errors.New("unknown flag: --zzzzzzzz") + out := flagErrorWithSuggestion(cmd, in) + if out != in { + t.Errorf("expected original error, got %q", out.Error()) + } + }) + t.Run("non-flag error returned unchanged", func(t *testing.T) { + in := errors.New("some other error") + out := flagErrorWithSuggestion(cmd, in) + if out != in { + t.Errorf("expected original error, got %q", out.Error()) + } + }) + t.Run("dashes-only unknown returned unchanged", func(t *testing.T) { + in := errors.New("unknown flag: ---") + out := flagErrorWithSuggestion(cmd, in) + if out != in { + t.Errorf("expected original error, got %q", out.Error()) + } + }) +} diff --git a/cmd/opencodereview/misc_helpers_test.go b/cmd/opencodereview/misc_helpers_test.go new file mode 100644 index 0000000..3fb3ee5 --- /dev/null +++ b/cmd/opencodereview/misc_helpers_test.go @@ -0,0 +1,91 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" + + "github.com/alibaba/open-code-review/internal/session" + "github.com/spf13/cobra" +) + +func TestReviewModeFromOptions(t *testing.T) { + cases := []struct { + name string + opts reviewOptions + want string + }{ + {"commit", reviewOptions{commit: "abc"}, session.ReviewModeCommit}, + {"range", reviewOptions{from: "main", to: "dev"}, session.ReviewModeRange}, + {"workspace", reviewOptions{}, session.ReviewModeWorkspace}, + {"from only falls back to workspace", reviewOptions{from: "main"}, session.ReviewModeWorkspace}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := reviewModeFromOptions(c.opts); got != c.want { + t.Errorf("reviewModeFromOptions() = %q, want %q", got, c.want) + } + }) + } +} + +func TestSanitizeEndpointHost(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"empty", "", ""}, + {"whitespace", " ", ""}, + {"strips credentials and path", "https://user:pass@API.example.com:8080/v1/chat?k=1#frag", "api.example.com:8080"}, + {"lowercases host", "https://Example.COM", "example.com"}, + {"no host yields empty", "mailto:foo@bar.com", ""}, + {"unparseable yields empty", "://:::", ""}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + if got := sanitizeEndpointHost(c.in); got != c.want { + t.Errorf("sanitizeEndpointHost(%q) = %q, want %q", c.in, got, c.want) + } + }) + } +} + +func TestShortSessionID(t *testing.T) { + if got := shortSessionID("0123456789abcdef"); got != "01234567" { + t.Errorf("shortSessionID(long) = %q, want %q", got, "01234567") + } + if got := shortSessionID("short"); got != "short" { + t.Errorf("shortSessionID(short) = %q, want %q", got, "short") + } + if got := shortSessionID("12345678"); got != "12345678" { + t.Errorf("shortSessionID(exactly8) = %q, want %q", got, "12345678") + } +} + +func TestCompleteSessionIDs(t *testing.T) { + t.Run("with args returns no completions", func(t *testing.T) { + cmd := &cobra.Command{Use: "x"} + comps, directive := completeSessionIDs(cmd, []string{"already"}, "") + if comps != nil { + t.Errorf("expected nil completions, got %v", comps) + } + if directive != cobra.ShellCompDirectiveNoFileComp { + t.Errorf("directive = %v, want NoFileComp", directive) + } + }) + + t.Run("fresh repo yields empty completions", func(t *testing.T) { + dir := initTestGitRepo(t) + cmd := &cobra.Command{Use: "x"} + cmd.Flags().String("repo", dir, "") + comps, directive := completeSessionIDs(cmd, nil, "") + if len(comps) != 0 { + t.Errorf("expected no completions for fresh repo, got %v", comps) + } + if directive != cobra.ShellCompDirectiveNoFileComp { + t.Errorf("directive = %v, want NoFileComp", directive) + } + }) +} diff --git a/cmd/opencodereview/output_manifest_test.go b/cmd/opencodereview/output_manifest_test.go new file mode 100644 index 0000000..63ae178 --- /dev/null +++ b/cmd/opencodereview/output_manifest_test.go @@ -0,0 +1,137 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/agent" + "github.com/alibaba/open-code-review/internal/session" +) + +// TestWarningsForOutput covers warningsForOutput: the early pass-through when +// there is no manifest, the all-filtered case that collapses to nil, and the +// mixed case that keeps only non-subtask warnings. +func TestWarningsForOutput(t *testing.T) { + warns := []agent.AgentWarning{ + {Type: "subtask_error"}, + {Type: "scan_subtask_error"}, + {Type: "token_budget_reached"}, + } + manifest := &session.RunManifest{TerminalState: session.StateComplete} + + t.Run("nil manifest passes through unchanged", func(t *testing.T) { + got := warningsForOutput(warns, nil) + if len(got) != len(warns) { + t.Errorf("got %d warnings, want %d", len(got), len(warns)) + } + }) + + t.Run("all subtask errors collapse to nil", func(t *testing.T) { + only := []agent.AgentWarning{{Type: "subtask_error"}, {Type: "scan_subtask_error"}} + if got := warningsForOutput(only, manifest); got != nil { + t.Errorf("expected nil, got %v", got) + } + }) + + t.Run("mixed keeps only non-subtask warnings", func(t *testing.T) { + got := warningsForOutput(warns, manifest) + if len(got) != 1 || got[0].Type != "token_budget_reached" { + t.Errorf("expected only token_budget_reached, got %v", got) + } + }) +} + +// TestManifestMessage covers every terminal-state branch of manifestMessage, +// including the waived-count variant of a complete run and the failed run with +// and without a recorded RunFailure classification. +func TestManifestMessage(t *testing.T) { + items := func(n int) []session.CoverageItem { + return make([]session.CoverageItem, n) + } + + t.Run("nil manifest is empty", func(t *testing.T) { + if got := manifestMessage(nil, 0); got != "" { + t.Errorf("nil manifest = %q, want empty", got) + } + }) + + cases := []struct { + name string + manifest *session.RunManifest + findings int + want string // substring the message must contain + }{ + { + name: "complete without waived", + manifest: &session.RunManifest{ + TerminalState: session.StateComplete, + Coverage: session.Coverage{Selected: items(3)}, + }, + findings: 2, + want: "Review complete: 2 finding(s) across 3 selected item(s).", + }, + { + name: "complete with waived", + manifest: &session.RunManifest{ + TerminalState: session.StateComplete, + Coverage: session.Coverage{Selected: items(3), Waived: items(1)}, + }, + findings: 2, + want: "including 1 waived", + }, + { + name: "partial", + manifest: &session.RunManifest{ + TerminalState: session.StatePartial, + Coverage: session.Coverage{Selected: items(4), Failed: items(1)}, + }, + findings: 1, + want: "partially complete", + }, + { + name: "failed with classification", + manifest: &session.RunManifest{ + TerminalState: session.StateFailed, + Coverage: session.Coverage{Selected: items(2), Failed: items(2)}, + RunFailure: &session.RunFailure{Classification: session.RunFailureInput}, + }, + findings: 0, + want: "Review failed (input)", + }, + { + name: "failed without classification", + manifest: &session.RunManifest{ + TerminalState: session.StateFailed, + Coverage: session.Coverage{Selected: items(2), Failed: items(2)}, + }, + findings: 0, + want: "Review failed: 0 finding(s)", + }, + { + name: "skipped", + manifest: &session.RunManifest{ + TerminalState: session.StateSkipped, + }, + want: "Review skipped", + }, + { + name: "unknown state falls through", + manifest: &session.RunManifest{ + TerminalState: session.TerminalState("bogus"), + }, + want: "unknown manifest state", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + got := manifestMessage(tc.manifest, tc.findings) + if !strings.Contains(got, tc.want) { + t.Errorf("manifestMessage = %q, want substring %q", got, tc.want) + } + }) + } +} diff --git a/cmd/opencodereview/provider_config_apply_test.go b/cmd/opencodereview/provider_config_apply_test.go new file mode 100644 index 0000000..c528986 --- /dev/null +++ b/cmd/opencodereview/provider_config_apply_test.go @@ -0,0 +1,61 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "path/filepath" + "strings" + "testing" +) + +// TestApplyOfficialProviderConfig_Validation covers the pre-save validation +// branches (empty provider, empty model, missing API key) that reject the +// request before any network connection test runs. +func TestApplyOfficialProviderConfig_Validation(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + + t.Run("empty provider rejected", func(t *testing.T) { + err := applyOfficialProviderConfig(configPath, &Config{}, providerTUIResult{}) + if err == nil || !strings.Contains(err.Error(), "required") { + t.Fatalf("got %v, want provider/model required error", err) + } + }) + + t.Run("empty model rejected", func(t *testing.T) { + err := applyOfficialProviderConfig(configPath, &Config{}, providerTUIResult{provider: "openai"}) + if err == nil || !strings.Contains(err.Error(), "required") { + t.Fatalf("got %v, want provider/model required error", err) + } + }) + + t.Run("missing API key for non-preset provider rejected", func(t *testing.T) { + err := applyOfficialProviderConfig(configPath, &Config{}, providerTUIResult{ + provider: "not-a-preset-provider", + model: "m", + }) + if err == nil || !strings.Contains(err.Error(), "API key is required") { + t.Fatalf("got %v, want API-key-required error", err) + } + }) +} + +// TestSetCustomProviderValue covers the malformed-key rejection and the +// success path that materializes a custom provider entry. +func TestSetCustomProviderValue(t *testing.T) { + t.Run("malformed key rejected", func(t *testing.T) { + if err := setCustomProviderValue(&Config{}, "custom_providers.onlyname", "v"); err == nil { + t.Error("expected error for key missing a field segment") + } + }) + + t.Run("success sets a custom provider field", func(t *testing.T) { + cfg := &Config{} + if err := setCustomProviderValue(cfg, "custom_providers.cp.url", "https://x.example"); err != nil { + t.Fatalf("setCustomProviderValue: %v", err) + } + if cfg.CustomProviders["cp"].URL != "https://x.example" { + t.Errorf("custom provider url not set: %+v", cfg.CustomProviders) + } + }) +} diff --git a/cmd/opencodereview/provider_tui_cpinput_test.go b/cmd/opencodereview/provider_tui_cpinput_test.go new file mode 100644 index 0000000..5afeaf6 --- /dev/null +++ b/cmd/opencodereview/provider_tui_cpinput_test.go @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "path/filepath" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/alibaba/open-code-review/internal/llm" +) + +// TestIsUserEditMsg covers the key/paste true branches and the default false. +func TestIsUserEditMsg(t *testing.T) { + if !isUserEditMsg(tea.KeyPressMsg{Code: 'a', Text: "a"}) { + t.Error("KeyPressMsg should be a user edit") + } + if !isUserEditMsg(tea.PasteMsg{Content: "x"}) { + t.Error("PasteMsg should be a user edit") + } + if isUserEditMsg(tea.WindowSizeMsg{Width: 10, Height: 10}) { + t.Error("WindowSizeMsg should not be a user edit") + } +} + +// TestOfficialAPIKeyRequiredError covers the with-EnvVar and without branches. +func TestOfficialAPIKeyRequiredError(t *testing.T) { + got := officialAPIKeyRequiredError(llm.Provider{EnvVar: "MY_KEY"}) + if got != "API key is required (or set $MY_KEY)" { + t.Errorf("got %q, want mention of $MY_KEY", got) + } + if got := officialAPIKeyRequiredError(llm.Provider{}); got != "API key is required" { + t.Errorf("got %q, want generic message", got) + } +} + +// TestOfficialProviderEnvKeySet covers empty EnvVar, unset var, and set var. +func TestOfficialProviderEnvKeySet(t *testing.T) { + if officialProviderEnvKeySet(llm.Provider{}) { + t.Error("empty EnvVar should report not set") + } + if officialProviderEnvKeySet(llm.Provider{EnvVar: "OCR_TEST_UNSET_ENVKEY_XYZ"}) { + t.Error("unset env var should report not set") + } + t.Setenv("OCR_TEST_SET_ENVKEY", "value") + if !officialProviderEnvKeySet(llm.Provider{EnvVar: "OCR_TEST_SET_ENVKEY"}) { + t.Error("set env var should report set") + } +} + +// TestFindCustomIdx covers a hit and a miss. +func TestFindCustomIdx(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "cp": {URL: "https://x.example", Protocol: "openai"}, + }, + } + m := newProviderTUI(cfg, filepath.Join(t.TempDir(), "config.json")) + m.customProviders = collectCustomProviders(cfg) + if idx := m.findCustomIdx("cp"); idx < 0 { + t.Error("existing provider should be found") + } + if idx := m.findCustomIdx("nope"); idx != -1 { + t.Errorf("missing provider should return -1, got %d", idx) + } +} + +// TestPassThroughCPInput drives each custom-provider input step and confirms a +// key press clears formError, including the masked-API-key replacement branch. +func TestPassThroughCPInput(t *testing.T) { + newModel := func() providerTUIModel { + cfg := &Config{} + m := newProviderTUI(cfg, filepath.Join(t.TempDir(), "config.json")) + m.formError = "stale error" + return m + } + key := tea.KeyPressMsg{Code: 'a', Text: "a"} + + for _, step := range []struct { + name string + step customProviderStep + }{ + {"name", cpStepName}, + {"baseURL", cpStepBaseURL}, + {"apiKey", cpStepAPIKey}, + {"authHeader", cpStepAuthHeader}, + } { + t.Run(step.name, func(t *testing.T) { + m := newModel() + m.cpStep = step.step + out, _ := m.passThroughCPInput(key) + if out.(providerTUIModel).formError != "" { + t.Error("a key press should clear formError") + } + }) + } + + t.Run("masked api key begins replace on edit", func(t *testing.T) { + m := newModel() + m.cpStep = cpStepAPIKey + m.apiKeyMasked = true + m.apiKeyOriginal = "secret" + out, _ := m.passThroughCPInput(key) + if out.(providerTUIModel).apiKeyMasked { + t.Error("editing a masked API key should begin replacement") + } + }) +} diff --git a/cmd/opencodereview/provider_tui_customform_test.go b/cmd/opencodereview/provider_tui_customform_test.go new file mode 100644 index 0000000..4ee9f1d --- /dev/null +++ b/cmd/opencodereview/provider_tui_customform_test.go @@ -0,0 +1,211 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "path/filepath" + "testing" + + tea "charm.land/bubbletea/v2" +) + +// TestHandleCustomFormEnter_Steps drives handleCustomFormEnter through the create +// flow's every cpStep, covering both guard and advance branches. +func TestHandleCustomFormEnter_Steps(t *testing.T) { + t.Run("name empty stays", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.creatingCustom = true + m.cpStep = cpStepName + m.cpNameInput.SetValue("") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepName { + t.Errorf("empty name should stay on name step, got %d", got.cpStep) + } + }) + + t.Run("name taken sets formError", func(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "dup": {URL: "https://x", Protocol: "openai", Models: []string{"m"}}, + }, + } + m := newProviderTUI(cfg, "") + m.creatingCustom = true + m.cpStep = cpStepName + m.cpNameInput.SetValue("dup") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.formError == "" { + t.Error("taken name should set formError") + } + if got.cpStep != cpStepName { + t.Errorf("taken name should stay on name step, got %d", got.cpStep) + } + }) + + t.Run("name valid advances to protocol", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.creatingCustom = true + m.cpStep = cpStepName + m.cpNameInput.SetValue("fresh") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepProtocol { + t.Errorf("cpStep = %d, want cpStepProtocol", got.cpStep) + } + }) + + t.Run("protocol advances to baseURL", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepProtocol + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepBaseURL { + t.Errorf("cpStep = %d, want cpStepBaseURL", got.cpStep) + } + }) + + t.Run("baseURL empty stays", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepBaseURL + m.cpURLInput.SetValue("") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepBaseURL { + t.Errorf("empty URL should stay on baseURL step, got %d", got.cpStep) + } + }) + + t.Run("baseURL valid advances to APIKey", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.creatingCustom = true + m.cpStep = cpStepBaseURL + m.cpURLInput.SetValue("https://api.example.com") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepAPIKey { + t.Errorf("cpStep = %d, want cpStepAPIKey", got.cpStep) + } + }) + + t.Run("APIKey advances to authHeader", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepAPIKey + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.cpStep != cpStepAuthHeader { + t.Errorf("cpStep = %d, want cpStepAuthHeader", got.cpStep) + } + }) + + t.Run("authHeader invalid sets formError", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepAuthHeader + m.cpAuthInput.SetValue("bogus") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if got.formError == "" { + t.Error("invalid auth header should set formError") + } + }) + + t.Run("authHeader valid creating saves and enters model step", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + m := newProviderTUI(&Config{}, path) + m.creatingCustom = true + m.cpStep = cpStepAuthHeader + m.cpNameInput.SetValue("newprov") + m.cpURLInput.SetValue("https://api.example.com") + m.cpAuthInput.SetValue("authorization") + out, _ := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if !got.savedInSession { + t.Errorf("create should mark savedInSession; formError=%q", got.formError) + } + if got.step != stepModel { + t.Errorf("after create should enter model step, got %d", got.step) + } + }) + + t.Run("authHeader valid non-create-non-edit confirms", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepAuthHeader + m.cpAuthInput.SetValue("") + out, cmd := m.handleCustomFormEnter() + got := out.(providerTUIModel) + if !got.confirmed { + t.Error("valid auth header (no create/edit) should confirm") + } + if cmd == nil { + t.Error("should return quit command") + } + }) +} + +// TestUpdateCustomProviderForm_Esc covers esc on the name step (full reset) and +// on a later step (decrement / edit APIKey→BaseURL), plus ctrl+c. +func TestUpdateCustomProviderForm_Esc(t *testing.T) { + t.Run("esc on name step resets", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.creatingCustom = true + m.cpStep = cpStepName + m.cpNameInput.SetValue("scratch") + out, _ := m.updateCustomProviderForm("esc", escKey()) + got := out.(providerTUIModel) + if got.creatingCustom { + t.Error("esc on name step should cancel creatingCustom") + } + if got.cpNameInput.Value() != "" { + t.Errorf("name input should be cleared, got %q", got.cpNameInput.Value()) + } + }) + + t.Run("esc on later step decrements", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepBaseURL + out, _ := m.updateCustomProviderForm("esc", escKey()) + got := out.(providerTUIModel) + if got.cpStep != cpStepProtocol { + t.Errorf("esc should decrement to protocol, got %d", got.cpStep) + } + }) + + t.Run("esc on editing APIKey jumps to baseURL", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.editingCustom = true + m.cpStep = cpStepAPIKey + out, _ := m.updateCustomProviderForm("esc", escKey()) + got := out.(providerTUIModel) + if got.cpStep != cpStepBaseURL { + t.Errorf("editing esc on APIKey should go to baseURL, got %d", got.cpStep) + } + }) + + t.Run("ctrl+c cancels", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + out, cmd := m.updateCustomProviderForm("ctrl+c", tea.KeyPressMsg{}) + got := out.(providerTUIModel) + if !got.cancelled || cmd == nil { + t.Error("ctrl+c should cancel and quit") + } + }) +} + +// TestUpdateCustomProviderForm_ProtocolNav covers up/down protocol selection. +func TestUpdateCustomProviderForm_ProtocolNav(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.cpStep = cpStepProtocol + m.cpProtocolIdx = 0 + out, _ := m.updateCustomProviderForm("down", tea.KeyPressMsg{Code: tea.KeyDown}) + got := out.(providerTUIModel) + if got.cpProtocolIdx != 1 { + t.Errorf("down should advance protocol idx, got %d", got.cpProtocolIdx) + } + out, _ = got.updateCustomProviderForm("up", tea.KeyPressMsg{Code: tea.KeyUp}) + got = out.(providerTUIModel) + if got.cpProtocolIdx != 0 { + t.Errorf("up should decrement protocol idx, got %d", got.cpProtocolIdx) + } +} diff --git a/cmd/opencodereview/provider_tui_deleteconfirm_test.go b/cmd/opencodereview/provider_tui_deleteconfirm_test.go new file mode 100644 index 0000000..c44dc3e --- /dev/null +++ b/cmd/opencodereview/provider_tui_deleteconfirm_test.go @@ -0,0 +1,65 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "path/filepath" + "testing" +) + +// TestUpdateDeleteModelConfirm covers the key branches of the delete-model +// confirmation handler: cancel (n/esc), ctrl+c quit, and the default-tab +// no-op. +func TestUpdateDeleteModelConfirm_ProviderTUI(t *testing.T) { + newModel := func() providerTUIModel { + cfg := &Config{} + m := newProviderTUI(cfg, filepath.Join(t.TempDir(), "config.json")) + m.confirmingDeleteModel = true + return m + } + + t.Run("n cancels", func(t *testing.T) { + m := newModel() + out, _ := m.updateDeleteModelConfirm("n") + if out.(providerTUIModel).confirmingDeleteModel { + t.Error("n should cancel the delete confirmation") + } + }) + + t.Run("esc cancels", func(t *testing.T) { + m := newModel() + out, _ := m.updateDeleteModelConfirm("esc") + if out.(providerTUIModel).confirmingDeleteModel { + t.Error("esc should cancel the delete confirmation") + } + }) + + t.Run("ctrl+c quits", func(t *testing.T) { + m := newModel() + out, cmd := m.updateDeleteModelConfirm("ctrl+c") + if !out.(providerTUIModel).cancelled { + t.Error("ctrl+c should mark the model cancelled") + } + if cmd == nil { + t.Error("ctrl+c should return a quit command") + } + }) + + t.Run("y on manual tab is a no-op", func(t *testing.T) { + m := newModel() + m.activeTab = tabManual + out, _ := m.updateDeleteModelConfirm("y") + if out.(providerTUIModel).confirmingDeleteModel { + t.Error("y on manual tab should clear the confirmation without deleting") + } + }) + + t.Run("unhandled key is ignored", func(t *testing.T) { + m := newModel() + out, _ := m.updateDeleteModelConfirm("z") + if !out.(providerTUIModel).confirmingDeleteModel { + t.Error("unhandled key should leave the confirmation open") + } + }) +} diff --git a/cmd/opencodereview/provider_tui_editsave_test.go b/cmd/opencodereview/provider_tui_editsave_test.go new file mode 100644 index 0000000..4ad79a5 --- /dev/null +++ b/cmd/opencodereview/provider_tui_editsave_test.go @@ -0,0 +1,111 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "os" + "path/filepath" + "testing" +) + +// TestApplyEditCustomProviderSave_Guards covers the two early-return guards: +// a nil config and an empty config path. +func TestApplyEditCustomProviderSave_Guards(t *testing.T) { + t.Run("nil config", func(t *testing.T) { + m := &providerTUIModel{} + if err := m.applyEditCustomProviderSave(); err == nil { + t.Fatal("expected error when config is nil") + } + if m.formError == "" { + t.Error("formError should be set when config is nil") + } + }) + + t.Run("empty config path", func(t *testing.T) { + m := &providerTUIModel{existingCfg: &Config{}} + if err := m.applyEditCustomProviderSave(); err == nil { + t.Fatal("expected error when config path is empty") + } + if m.formError == "" { + t.Error("formError should be set when config path is empty") + } + }) +} + +// TestApplyEditCustomProviderSave_RenameReassignsActiveProvider covers the +// name-change branch where the edited provider is also the active provider: the +// old key is deleted, and the active Provider/Model are re-pointed at the new +// name. +func TestApplyEditCustomProviderSave_RenameReassignsActiveProvider(t *testing.T) { + dir := t.TempDir() + configPath := filepath.Join(dir, "config.json") + cfg := &Config{ + Provider: "oldname", + Model: "some-model", + CustomProviders: map[string]ProviderEntry{ + "oldname": {URL: "https://example.com/v1", Protocol: "openai"}, + }, + } + m := newProviderTUI(cfg, configPath) + m.activeTab = tabCustom + m.editingCustom = true + m.editTargetName = "oldname" + m.cpProtocolIdx = 1 // openai + m.cpNameInput.SetValue("newname") + m.cpURLInput.SetValue("https://example.com/v1") + + if err := m.applyEditCustomProviderSave(); err != nil { + t.Fatalf("applyEditCustomProviderSave: %v", err) + } + if _, ok := cfg.CustomProviders["oldname"]; ok { + t.Error("old provider key should be deleted after rename") + } + if _, ok := cfg.CustomProviders["newname"]; !ok { + t.Error("new provider key should exist after rename") + } + if cfg.Provider != "newname" { + t.Errorf("active Provider = %q, want newname", cfg.Provider) + } + if cfg.Model != "" { + t.Errorf("active Model = %q, want cleared", cfg.Model) + } +} + +// TestApplyEditCustomProviderSave_SaveFailureRestoresBackup covers the +// save-failure path where the reload also fails (config path is a directory, so +// both save and reload error) and the in-memory backup is restored. +func TestApplyEditCustomProviderSave_SaveFailureRestoresBackup(t *testing.T) { + dir := t.TempDir() + // A directory path makes both saveConfig and the reload fallback fail. + blockPath := filepath.Join(dir, "blocked") + if err := os.Mkdir(blockPath, 0o755); err != nil { + t.Fatal(err) + } + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "aaa": {URL: "https://example.com/v1", Protocol: "openai", Models: []string{"m1"}}, + }, + } + m := newProviderTUI(cfg, blockPath) + m.activeTab = tabCustom + m.editingCustom = true + m.editTargetName = "aaa" + m.cpProtocolIdx = 1 + m.cpNameInput.SetValue("aaa") + m.cpURLInput.SetValue("https://changed.example.com/v1") + + if err := m.applyEditCustomProviderSave(); err == nil { + t.Fatal("expected save error when config path is a directory") + } + if m.formError == "" { + t.Error("formError should be set on save failure") + } + if m.savedInSession { + t.Error("savedInSession must stay false on save failure") + } + // Backup restored: the URL edit should not have stuck. + if got := cfg.CustomProviders["aaa"].URL; got != "https://example.com/v1" { + t.Errorf("URL = %q, want original restored", got) + } +} diff --git a/cmd/opencodereview/provider_tui_manualenter_test.go b/cmd/opencodereview/provider_tui_manualenter_test.go new file mode 100644 index 0000000..90dd770 --- /dev/null +++ b/cmd/opencodereview/provider_tui_manualenter_test.go @@ -0,0 +1,192 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" + + tea "charm.land/bubbletea/v2" +) + +// TestHandleManualFormEnter_Steps drives handleManualFormEnter through every +// manualStep, covering both the guard (empty required field) and advance +// branches that the existing tests leave at ~28%. +func TestHandleManualFormEnter_Steps(t *testing.T) { + t.Run("URL empty stays", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepURL + m.manualURLInput.SetValue("") + out, _ := m.handleManualFormEnter() + got := out.(providerTUIModel) + if got.manualStep != manualStepURL { + t.Errorf("empty URL should stay on URL step, got %d", got.manualStep) + } + }) + + t.Run("URL non-empty advances to protocol", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepURL + m.manualURLInput.SetValue("https://api.example.com/v1") + out, _ := m.handleManualFormEnter() + got := out.(providerTUIModel) + if got.manualStep != manualStepProtocol { + t.Errorf("manualStep = %d, want manualStepProtocol", got.manualStep) + } + }) + + t.Run("protocol advances to model", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepProtocol + out, _ := m.handleManualFormEnter() + got := out.(providerTUIModel) + if got.manualStep != manualStepModel { + t.Errorf("manualStep = %d, want manualStepModel", got.manualStep) + } + }) + + t.Run("model empty stays", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepModel + m.manualModelInput.SetValue("") + out, _ := m.handleManualFormEnter() + got := out.(providerTUIModel) + if got.manualStep != manualStepModel { + t.Errorf("empty model should stay on model step, got %d", got.manualStep) + } + }) + + t.Run("model non-empty advances to auth token", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepModel + m.manualModelInput.SetValue("gpt-4") + out, _ := m.handleManualFormEnter() + got := out.(providerTUIModel) + if got.manualStep != manualStepAuthToken { + t.Errorf("manualStep = %d, want manualStepAuthToken", got.manualStep) + } + }) + + t.Run("auth header invalid sets formError", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepAuthHeader + m.manualAuthHeaderInput.SetValue("bogus-header") + out, _ := m.handleManualFormEnter() + got := out.(providerTUIModel) + if got.formError == "" { + t.Error("invalid auth header should set formError") + } + if got.manualStep != manualStepAuthHeader { + t.Errorf("invalid header should stay on auth header step, got %d", got.manualStep) + } + if got.confirmed { + t.Error("invalid header must not confirm") + } + }) + + t.Run("auth header valid confirms and quits", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.manualStep = manualStepAuthHeader + m.manualAuthHeaderInput.SetValue("authorization") + out, cmd := m.handleManualFormEnter() + got := out.(providerTUIModel) + if !got.confirmed { + t.Error("valid auth header should confirm") + } + if cmd == nil { + t.Error("valid auth header should return a quit command") + } + }) +} + +// TestUpdateManualForm_Esc covers the esc branches: on the URL step the form is +// dismissed and inputs reset (with and without an existing config), while later +// steps decrement to the previous step. +func TestUpdateManualForm_Esc(t *testing.T) { + t.Run("esc on URL step with existingCfg restores values", func(t *testing.T) { + cfg := &Config{} + cfg.Llm.URL = "https://saved.example.com" + cfg.Llm.Model = "saved-model" + cfg.Llm.AuthToken = "secret" + m := newProviderTUI(cfg, "") + m.inManualForm = true + m.manualStep = manualStepURL + m.manualURLInput.SetValue("https://scratch") + out, _ := m.updateManualForm("esc", escKey()) + got := out.(providerTUIModel) + if got.inManualForm { + t.Error("esc on URL step should exit manual form") + } + if got.manualURLInput.Value() != "https://saved.example.com" { + t.Errorf("URL not restored: %q", got.manualURLInput.Value()) + } + if !got.manualTokenMasked { + t.Error("existing token should be masked after restore") + } + }) + + t.Run("esc on URL step without existingCfg clears values", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.existingCfg = nil + m.inManualForm = true + m.manualStep = manualStepURL + m.manualURLInput.SetValue("https://scratch") + out, _ := m.updateManualForm("esc", escKey()) + got := out.(providerTUIModel) + if got.inManualForm { + t.Error("esc on URL step should exit manual form") + } + if got.manualURLInput.Value() != "" { + t.Errorf("URL should be cleared, got %q", got.manualURLInput.Value()) + } + }) + + t.Run("esc on later step decrements", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.inManualForm = true + m.manualStep = manualStepModel + out, _ := m.updateManualForm("esc", escKey()) + got := out.(providerTUIModel) + if got.manualStep != manualStepProtocol { + t.Errorf("esc should decrement to protocol, got %d", got.manualStep) + } + if !got.inManualForm { + t.Error("esc on later step should stay in manual form") + } + }) +} + +// TestUpdateManualForm_ProtocolNav covers the up/down protocol selection on the +// protocol step. +func TestUpdateManualForm_ProtocolNav(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.inManualForm = true + m.manualStep = manualStepProtocol + m.manualProtocolIdx = 0 + + out, _ := m.updateManualForm("down", tea.KeyPressMsg{Code: tea.KeyDown}) + got := out.(providerTUIModel) + if got.manualProtocolIdx != 1 { + t.Errorf("down should advance protocol idx to 1, got %d", got.manualProtocolIdx) + } + + out, _ = got.updateManualForm("up", tea.KeyPressMsg{Code: tea.KeyUp}) + got = out.(providerTUIModel) + if got.manualProtocolIdx != 0 { + t.Errorf("up should return protocol idx to 0, got %d", got.manualProtocolIdx) + } +} + +// TestUpdateManualForm_CtrlC covers the ctrl+c cancel branch. +func TestUpdateManualForm_CtrlC(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.inManualForm = true + out, cmd := m.updateManualForm("ctrl+c", tea.KeyPressMsg{}) + got := out.(providerTUIModel) + if !got.cancelled { + t.Error("ctrl+c should mark cancelled") + } + if cmd == nil { + t.Error("ctrl+c should return quit command") + } +} diff --git a/cmd/opencodereview/provider_tui_modeltui_test.go b/cmd/opencodereview/provider_tui_modeltui_test.go new file mode 100644 index 0000000..33441eb --- /dev/null +++ b/cmd/opencodereview/provider_tui_modeltui_test.go @@ -0,0 +1,265 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "path/filepath" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/alibaba/open-code-review/internal/llm" +) + +// newCustomModelTUI builds a modelTUIModel backed by a custom provider with a +// writable config path, so add/delete/persist operations exercise the save path. +func newCustomModelTUI(t *testing.T, models []string) (modelTUIModel, *Config, string) { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "myprov": {URL: "https://api.example.com", Protocol: "openai", Models: models}, + }, + } + m := newModelTUIConfig(modelTUIConfig{ + Provider: llm.Provider{Name: "myprov", DisplayName: "My Prov", Models: models}, + ProviderName: "myprov", + ExistingCfg: cfg, + ConfigPath: path, + IsCustom: true, + }) + return m, cfg, path +} + +// TestModelTUIUpdate_Navigation covers window resize, up/down wrap-around, and +// enter on both a real model and the custom "add" item. +func TestModelTUIUpdate_Navigation(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1", "m2"}) + + // Window resize records dimensions. + out, _ := m.Update(tea.WindowSizeMsg{Width: 120, Height: 40}) + got := out.(modelTUIModel) + if got.width != 120 || got.height != 40 { + t.Errorf("resize not recorded: %dx%d", got.width, got.height) + } + + // down from 0 advances; up from 0 wraps to last item (the custom "add" row). + out, _ = m.Update(tea.KeyPressMsg{Code: tea.KeyDown}) + if out.(modelTUIModel).modelIdx != 1 { + t.Errorf("down modelIdx = %d, want 1", out.(modelTUIModel).modelIdx) + } + m.modelIdx = 0 + out, _ = m.Update(tea.KeyPressMsg{Code: tea.KeyUp}) + if out.(modelTUIModel).modelIdx != m.itemCount()-1 { + t.Errorf("up-wrap modelIdx = %d, want %d", out.(modelTUIModel).modelIdx, m.itemCount()-1) + } + + // enter on a real model confirms and quits. + m.modelIdx = 0 + out, cmd := m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + if !out.(modelTUIModel).confirmed || cmd == nil { + t.Error("enter on model should confirm and quit") + } + + // enter on the custom "add" item enters custom-model input mode. + m.modelIdx = m.itemCount() - 1 + out, _ = m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + if !out.(modelTUIModel).customModel { + t.Error("enter on custom item should enter customModel mode") + } +} + +// TestModelTUIUpdate_CancelAndDelete covers esc/ctrl+c cancel and the 'd' key +// entering delete-confirm on a user-added model. +func TestModelTUIUpdate_CancelAndDelete(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1", "m2"}) + + out, cmd := m.Update(tea.KeyPressMsg{Code: tea.KeyEscape}) + if !out.(modelTUIModel).cancelled || cmd == nil { + t.Error("esc should cancel and quit") + } + + m.modelIdx = 0 + out, _ = m.Update(dKey()) + got := out.(modelTUIModel) + if !got.confirmingDeleteModel { + t.Error("'d' on user-added model should begin delete confirm") + } + if got.deleteModelName != "m1" { + t.Errorf("deleteModelName = %q, want m1", got.deleteModelName) + } +} + +// TestModelTUIUpdate_CustomModelInput covers the customModel input sub-mode: +// esc exit, empty enter stays, duplicate enter sets formError, valid enter +// persists and adds, and default keys pass through to the text input. +func TestModelTUIUpdate_CustomModelInput(t *testing.T) { + t.Run("esc exits custom input", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.customModel = true + out, _ := m.Update(tea.KeyPressMsg{Code: tea.KeyEscape}) + if out.(modelTUIModel).customModel { + t.Error("esc should exit customModel mode") + } + }) + + t.Run("empty enter stays", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.customModel = true + m.modelInput.SetValue("") + out, _ := m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + if !out.(modelTUIModel).customModel { + t.Error("empty enter should stay in customModel mode") + } + }) + + t.Run("duplicate enter sets formError", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.customModel = true + m.modelInput.SetValue("m1") + out, _ := m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + if out.(modelTUIModel).formError == "" { + t.Error("duplicate model should set formError") + } + }) + + t.Run("valid enter persists and adds", func(t *testing.T) { + m, cfg, _ := newCustomModelTUI(t, []string{"m1"}) + m.customModel = true + m.modelInput.SetValue("m2") + out, _ := m.Update(tea.KeyPressMsg{Code: tea.KeyEnter}) + got := out.(modelTUIModel) + if got.customModel { + t.Error("valid enter should exit customModel mode") + } + if !got.savedInSession { + t.Errorf("valid enter should persist; formError=%q", got.formError) + } + if !llm.ModelListContains(cfg.CustomProviders["myprov"].Models, "m2") { + t.Error("new model should be added to config") + } + }) + + t.Run("default key passes through", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.customModel = true + out, _ := m.Update(tea.KeyPressMsg{Code: 'x', Text: "x"}) + if !out.(modelTUIModel).customModel { + t.Error("default key should stay in customModel mode") + } + }) +} + +// TestUpdateDeleteModelConfirm covers y (confirm), n/esc (cancel), and ctrl+c. +func TestUpdateDeleteModelConfirm(t *testing.T) { + t.Run("n cancels", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.confirmingDeleteModel = true + out, _ := m.updateDeleteModelConfirm("n") + if out.(modelTUIModel).confirmingDeleteModel { + t.Error("'n' should cancel delete confirm") + } + }) + + t.Run("ctrl+c quits", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.confirmingDeleteModel = true + out, cmd := m.updateDeleteModelConfirm("ctrl+c") + if !out.(modelTUIModel).cancelled || cmd == nil { + t.Error("ctrl+c should cancel and quit") + } + }) + + t.Run("y confirms delete", func(t *testing.T) { + m, cfg, _ := newCustomModelTUI(t, []string{"m1", "m2"}) + m.confirmingDeleteModel = true + m.deleteModelName = "m2" + out, _ := m.updateDeleteModelConfirm("y") + got := out.(modelTUIModel) + if got.confirmingDeleteModel { + t.Error("'y' should end delete confirm") + } + if !got.savedInSession { + t.Errorf("'y' should persist deletion; formError=%q", got.formError) + } + if llm.ModelListContains(cfg.CustomProviders["myprov"].Models, "m2") { + t.Error("deleted model should be gone from config") + } + }) +} + +// TestConfirmDeleteCustomProviderModel_Guards covers the not-user-added and +// nil-cfg early returns. +func TestConfirmDeleteCustomProviderModel_Guards(t *testing.T) { + t.Run("unknown model is a no-op", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.confirmingDeleteModel = true + m.deleteModelName = "not-in-list" + out, _ := m.confirmDeleteCustomProviderModel() + got := out.(modelTUIModel) + if got.confirmingDeleteModel { + t.Error("unknown model should end confirm without saving") + } + if got.savedInSession { + t.Error("unknown model must not persist") + } + }) + + t.Run("nil cfg is a no-op", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.deleteModelName = "m1" + m.existingCfg = nil + m.confirmingDeleteModel = true + out, _ := m.confirmDeleteCustomProviderModel() + if out.(modelTUIModel).savedInSession { + t.Error("nil cfg must not persist") + } + }) +} + +// TestConfirmDeleteOfficialModel covers deleting a user-added model from an +// official (preset) provider entry, plus the not-user-added guard. +func TestConfirmDeleteOfficialModel(t *testing.T) { + newOfficial := func(t *testing.T) (modelTUIModel, *Config) { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + Providers: map[string]ProviderEntry{ + "openai": {Models: []string{"user-added"}}, + }, + } + m := newModelTUIConfig(modelTUIConfig{ + Provider: llm.Provider{Name: "openai", DisplayName: "OpenAI", Models: []string{"gpt-4"}}, + ProviderName: "openai", + RegistryModels: []string{"gpt-4"}, + ExistingCfg: cfg, + ConfigPath: path, + IsCustom: false, + }) + return m, cfg + } + + t.Run("deletes user-added model", func(t *testing.T) { + m, cfg := newOfficial(t) + m.confirmingDeleteModel = true + m.deleteModelName = "user-added" + out, _ := m.confirmDeleteOfficialModel() + got := out.(modelTUIModel) + if !got.savedInSession { + t.Errorf("should persist deletion; formError=%q", got.formError) + } + if llm.ModelListContains(cfg.Providers["openai"].Models, "user-added") { + t.Error("deleted model should be gone from config") + } + }) + + t.Run("registry model is not deletable", func(t *testing.T) { + m, _ := newOfficial(t) + m.confirmingDeleteModel = true + m.deleteModelName = "gpt-4" // in registry, not user-added + out, _ := m.confirmDeleteOfficialModel() + if out.(modelTUIModel).savedInSession { + t.Error("registry model must not be deletable") + } + }) +} diff --git a/cmd/opencodereview/provider_tui_persist_test.go b/cmd/opencodereview/provider_tui_persist_test.go new file mode 100644 index 0000000..8174309 --- /dev/null +++ b/cmd/opencodereview/provider_tui_persist_test.go @@ -0,0 +1,378 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "path/filepath" + "testing" + + tea "charm.land/bubbletea/v2" + "github.com/alibaba/open-code-review/internal/llm" +) + +// TestPersistCustomModelName covers the providerTUIModel.persistCustomModelName +// branches: empty name, nil config, custom-tab success, official-tab success, +// and both save-failure rollbacks. +func TestPersistCustomModelName(t *testing.T) { + t.Run("empty name errors", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + if _, err := m.persistCustomModelName(""); err == nil { + t.Fatal("expected error for empty name") + } + }) + + t.Run("nil config not persisted", func(t *testing.T) { + m := newProviderTUI(&Config{}, "") + m.existingCfg = nil + persisted, err := m.persistCustomModelName("x") + if err != nil || persisted { + t.Errorf("got (%v, %v), want (false, nil)", persisted, err) + } + }) + + t.Run("custom tab success", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "cp": {URL: "https://x.example", Protocol: "openai", Models: []string{"m1"}}, + }, + } + m := newProviderTUI(cfg, path) + m.activeTab = tabCustom + m.customIdx = 0 + persisted, err := m.persistCustomModelName("m2") + if err != nil || !persisted { + t.Fatalf("got (%v, %v), want (true, nil)", persisted, err) + } + if !llm.ModelListContains(cfg.CustomProviders["cp"].Models, "m2") { + t.Error("m2 not appended to custom provider") + } + }) + + t.Run("official tab success", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{} + m := newProviderTUI(cfg, path) + m.activeTab = tabOfficial + provider := m.currentProvider() + if provider.Name == "" { + t.Skip("no official provider available") + } + persisted, err := m.persistCustomModelName("my-model") + if err != nil || !persisted { + t.Fatalf("got (%v, %v), want (true, nil)", persisted, err) + } + if !llm.ModelListContains(cfg.Providers[provider.Name].Models, "my-model") { + t.Error("my-model not appended to official provider entry") + } + }) + + t.Run("custom tab save failure rolls back", func(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "cp": {URL: "https://x.example", Protocol: "openai", Models: []string{"m1"}}, + }, + } + m := newProviderTUI(cfg, unwritableConfigPath(t)) + m.activeTab = tabCustom + m.customIdx = 0 + persisted, err := m.persistCustomModelName("m2") + if err == nil || persisted { + t.Fatalf("got (%v, %v), want (false, error)", persisted, err) + } + if llm.ModelListContains(cfg.CustomProviders["cp"].Models, "m2") { + t.Error("m2 should be rolled back after save failure") + } + }) + + t.Run("official tab save failure rolls back", func(t *testing.T) { + cfg := &Config{} + m := newProviderTUI(cfg, unwritableConfigPath(t)) + m.activeTab = tabOfficial + provider := m.currentProvider() + if provider.Name == "" { + t.Skip("no official provider available") + } + persisted, err := m.persistCustomModelName("my-model") + if err == nil || persisted { + t.Fatalf("got (%v, %v), want (false, error)", persisted, err) + } + if llm.ModelListContains(cfg.Providers[provider.Name].Models, "my-model") { + t.Error("my-model should be rolled back after save failure") + } + }) +} + +// TestUpdateCustomModelInput drives the providerTUIModel custom-model input +// sub-mode: esc exit, empty enter, duplicate name, valid enter persists, and +// default key passthrough. +func TestUpdateCustomModelInput(t *testing.T) { + newModel := func(t *testing.T) providerTUIModel { + t.Helper() + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "cp": {URL: "https://x.example", Protocol: "openai", Models: []string{"m1"}}, + }, + } + m := newProviderTUI(cfg, path) + m.activeTab = tabCustom + m.customIdx = 0 + m.customModel = true + return m + } + + t.Run("esc exits", func(t *testing.T) { + m := newModel(t) + out, _ := m.updateCustomModelInput("esc", tea.KeyPressMsg{Code: tea.KeyEscape}) + if out.(providerTUIModel).customModel { + t.Error("esc should exit custom model input") + } + }) + + t.Run("empty enter stays", func(t *testing.T) { + m := newModel(t) + m.modelInput.SetValue("") + out, _ := m.updateCustomModelInput("enter", enterKey()) + if !out.(providerTUIModel).customModel { + t.Error("empty enter should stay in custom model input") + } + }) + + t.Run("duplicate sets formError", func(t *testing.T) { + m := newModel(t) + m.modelInput.SetValue("m1") + out, _ := m.updateCustomModelInput("enter", enterKey()) + if out.(providerTUIModel).formError == "" { + t.Error("duplicate model should set formError") + } + }) + + t.Run("valid enter persists", func(t *testing.T) { + m := newModel(t) + m.modelInput.SetValue("m2") + out, _ := m.updateCustomModelInput("enter", enterKey()) + got := out.(providerTUIModel) + if got.customModel { + t.Error("valid enter should exit custom model input") + } + if !got.savedInSession { + t.Errorf("valid enter should persist; formError=%q", got.formError) + } + }) + + t.Run("default key passes through", func(t *testing.T) { + m := newModel(t) + out, _ := m.updateCustomModelInput("x", tea.KeyPressMsg{Code: 'x', Text: "x"}) + if !out.(providerTUIModel).customModel { + t.Error("default key should stay in custom model input") + } + }) +} + +// TestApplyCreateCustomProvider covers the create-custom-provider handler: +// nil config guard, empty config path guard, empty name, duplicate name, +// success, and save failure. +func TestApplyCreateCustomProvider(t *testing.T) { + setup := func(t *testing.T, cfg *Config, path string) providerTUIModel { + t.Helper() + m := newProviderTUI(cfg, path) + m.activeTab = tabCustom + m.creatingCustom = true + m.cpProtocolIdx = 0 + return m + } + + t.Run("nil config guard", func(t *testing.T) { + m := setup(t, &Config{}, filepath.Join(t.TempDir(), "c.json")) + m.existingCfg = nil + out, _ := m.applyCreateCustomProvider() + if out.(providerTUIModel).formError == "" { + t.Error("nil config should set formError") + } + }) + + t.Run("empty config path guard", func(t *testing.T) { + m := setup(t, &Config{}, "") + out, _ := m.applyCreateCustomProvider() + if out.(providerTUIModel).formError == "" { + t.Error("empty config path should set formError") + } + }) + + t.Run("empty name", func(t *testing.T) { + m := setup(t, &Config{}, filepath.Join(t.TempDir(), "c.json")) + m.cpNameInput.SetValue("") + out, _ := m.applyCreateCustomProvider() + if out.(providerTUIModel).formError == "" { + t.Error("empty name should set formError") + } + }) + + t.Run("duplicate name", func(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "dup": {URL: "https://x.example", Protocol: "openai"}, + }, + } + m := setup(t, cfg, filepath.Join(t.TempDir(), "c.json")) + m.customProviders = collectCustomProviders(cfg) + m.cpNameInput.SetValue("dup") + m.cpURLInput.SetValue("https://y.example") + out, _ := m.applyCreateCustomProvider() + if out.(providerTUIModel).formError == "" { + t.Error("duplicate name should set formError") + } + }) + + t.Run("success", func(t *testing.T) { + cfg := &Config{} + m := setup(t, cfg, filepath.Join(t.TempDir(), "c.json")) + m.cpNameInput.SetValue("brandnew") + m.cpURLInput.SetValue("https://new.example/v1") + out, _ := m.applyCreateCustomProvider() + got := out.(providerTUIModel) + if !got.savedInSession { + t.Errorf("success should set savedInSession; formError=%q", got.formError) + } + if _, ok := cfg.CustomProviders["brandnew"]; !ok { + t.Error("new provider not saved to config") + } + if got.step != stepModel { + t.Error("success should advance to stepModel") + } + }) + + t.Run("save failure", func(t *testing.T) { + m := setup(t, &Config{}, unwritableConfigPath(t)) + m.cpNameInput.SetValue("brandnew") + m.cpURLInput.SetValue("https://new.example/v1") + out, _ := m.applyCreateCustomProvider() + got := out.(providerTUIModel) + if got.formError == "" { + t.Error("save failure should set formError") + } + if got.savedInSession { + t.Error("save failure must not set savedInSession") + } + }) +} + +// TestPersistAddedModelName covers modelTUIModel.persistAddedModelName: empty +// name, nil config, custom success, official success, and both save failures. +func TestPersistAddedModelName(t *testing.T) { + t.Run("empty name errors", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + if err := m.persistAddedModelName(""); err == nil { + t.Fatal("expected error for empty name") + } + }) + + t.Run("nil config errors", func(t *testing.T) { + m, _, _ := newCustomModelTUI(t, []string{"m1"}) + m.existingCfg = nil + if err := m.persistAddedModelName("x"); err == nil { + t.Fatal("expected error for nil config") + } + }) + + t.Run("custom success", func(t *testing.T) { + m, cfg, _ := newCustomModelTUI(t, []string{"m1"}) + if err := m.persistAddedModelName("m2"); err != nil { + t.Fatalf("persistAddedModelName: %v", err) + } + if !llm.ModelListContains(cfg.CustomProviders["myprov"].Models, "m2") { + t.Error("m2 not added to custom provider") + } + }) + + t.Run("custom save failure rolls back", func(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{ + "myprov": {URL: "https://x.example", Protocol: "openai", Models: []string{"m1"}}, + }, + } + m := newModelTUIConfig(modelTUIConfig{ + Provider: llm.Provider{Name: "myprov", Models: []string{"m1"}}, + ProviderName: "myprov", + ExistingCfg: cfg, + ConfigPath: unwritableConfigPath(t), + IsCustom: true, + }) + if err := m.persistAddedModelName("m2"); err == nil { + t.Fatal("expected save failure error") + } + if llm.ModelListContains(cfg.CustomProviders["myprov"].Models, "m2") { + t.Error("m2 should be rolled back after save failure") + } + }) + + t.Run("official success", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{} + m := newModelTUIConfig(modelTUIConfig{ + Provider: llm.Provider{Name: "openai", Models: []string{"gpt-4"}}, + ProviderName: "openai", + ExistingCfg: cfg, + ConfigPath: path, + IsCustom: false, + }) + if err := m.persistAddedModelName("gpt-x"); err != nil { + t.Fatalf("persistAddedModelName: %v", err) + } + if !llm.ModelListContains(cfg.Providers["openai"].Models, "gpt-x") { + t.Error("gpt-x not added to official provider entry") + } + }) + + t.Run("official save failure rolls back", func(t *testing.T) { + cfg := &Config{} + m := newModelTUIConfig(modelTUIConfig{ + Provider: llm.Provider{Name: "openai", Models: []string{"gpt-4"}}, + ProviderName: "openai", + ExistingCfg: cfg, + ConfigPath: unwritableConfigPath(t), + IsCustom: false, + }) + if err := m.persistAddedModelName("gpt-x"); err == nil { + t.Fatal("expected save failure error") + } + if llm.ModelListContains(cfg.Providers["openai"].Models, "gpt-x") { + t.Error("gpt-x should be rolled back after save failure") + } + }) +} + +// TestConfirmDeleteOfficialModelActiveClear covers the modelTUIModel handler +// where the deleted model is also the active config model: deletion clears the +// active Model. +func TestConfirmDeleteOfficialModelActiveClear(t *testing.T) { + path := filepath.Join(t.TempDir(), "config.json") + cfg := &Config{ + Provider: "openai", + Model: "user-added", + Providers: map[string]ProviderEntry{ + "openai": {Models: []string{"user-added"}, Model: "user-added"}, + }, + } + m := newModelTUIConfig(modelTUIConfig{ + Provider: llm.Provider{Name: "openai", DisplayName: "OpenAI", Models: []string{"gpt-4"}}, + ProviderName: "openai", + RegistryModels: []string{"gpt-4"}, + ExistingCfg: cfg, + ConfigPath: path, + IsCustom: false, + }) + m.confirmingDeleteModel = true + m.deleteModelName = "user-added" + + out, _ := m.confirmDeleteOfficialModel() + got := out.(modelTUIModel) + if !got.savedInSession { + t.Fatalf("should persist deletion; formError=%q", got.formError) + } + if cfg.Model == "user-added" { + t.Error("active Model should be cleared after deleting the active model") + } +} diff --git a/cmd/opencodereview/provider_tui_rollback_test.go b/cmd/opencodereview/provider_tui_rollback_test.go new file mode 100644 index 0000000..1bc1395 --- /dev/null +++ b/cmd/opencodereview/provider_tui_rollback_test.go @@ -0,0 +1,226 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "os" + "path/filepath" + "testing" + + "github.com/alibaba/open-code-review/internal/llm" +) + +func TestRegistryModelsForProvider(t *testing.T) { + t.Run("known preset ignores fallback", func(t *testing.T) { + got := registryModelsForProvider("anthropic", []string{"junk"}) + if len(got) == 0 { + t.Fatal("expected preset models for anthropic") + } + for _, m := range got { + if m == "junk" { + t.Error("fallback leaked into preset result") + } + } + }) + t.Run("unknown uses fallback", func(t *testing.T) { + got := registryModelsForProvider("no-such-provider", []string{"a", "b"}) + if len(got) != 2 || got[0] != "a" || got[1] != "b" { + t.Errorf("got %v, want [a b]", got) + } + }) + t.Run("unknown without fallback is nil", func(t *testing.T) { + if got := registryModelsForProvider("no-such-provider", nil); got != nil { + t.Errorf("got %v, want nil", got) + } + }) +} + +func TestApplyModelDeleteToEntry(t *testing.T) { + entry := ProviderEntry{Models: []string{"m1", "m2"}, Model: "m2"} + got := applyModelDeleteToEntry(entry, "m2") + if llm.ModelListContains(got.Models, "m2") { + t.Error("deleted model still present") + } + if got.Model != "" { + t.Errorf("active model = %q, want cleared", got.Model) + } +} + +func TestClearCfgActiveModelIfDeleted(t *testing.T) { + t.Run("clears matching", func(t *testing.T) { + cfg := &Config{Provider: "p", Model: "m"} + clearCfgActiveModelIfDeleted(cfg, "p", "m") + if cfg.Model != "" { + t.Errorf("model = %q, want cleared", cfg.Model) + } + }) + t.Run("keeps non-matching provider", func(t *testing.T) { + cfg := &Config{Provider: "other", Model: "m"} + clearCfgActiveModelIfDeleted(cfg, "p", "m") + if cfg.Model != "m" { + t.Errorf("model = %q, want unchanged", cfg.Model) + } + }) + t.Run("nil cfg is a no-op", func(t *testing.T) { + clearCfgActiveModelIfDeleted(nil, "p", "m") // must not panic + }) +} + +func TestRollbackCfgActiveModel(t *testing.T) { + cfg := &Config{Provider: "p", Model: ""} + rollbackCfgActiveModel(cfg, "p", "prev") + if cfg.Model != "prev" { + t.Errorf("model = %q, want prev", cfg.Model) + } + // Different provider: unchanged. + cfg2 := &Config{Provider: "other", Model: "x"} + rollbackCfgActiveModel(cfg2, "p", "prev") + if cfg2.Model != "x" { + t.Errorf("model = %q, want x", cfg2.Model) + } + rollbackCfgActiveModel(nil, "p", "prev") // must not panic +} + +func TestRollbackModelDelete(t *testing.T) { + t.Run("nil cfg is a no-op", func(t *testing.T) { + m := &modelTUIModel{existingCfg: nil} + m.rollbackModelDelete(ProviderEntry{}, "") // must not panic + }) + + t.Run("custom provider restores entry and active model", func(t *testing.T) { + cfg := &Config{ + Provider: "cp", + Model: "", + CustomProviders: map[string]ProviderEntry{"cp": {Models: []string{"a"}}}, + } + m := &modelTUIModel{ + existingCfg: cfg, + providerName: "cp", + isCustomProvider: true, + } + prev := ProviderEntry{Models: []string{"a", "b"}, Model: "b"} + m.rollbackModelDelete(prev, "b") + if got := cfg.CustomProviders["cp"]; len(got.Models) != 2 { + t.Errorf("restored models = %v, want 2 entries", got.Models) + } + if cfg.Model != "b" { + t.Errorf("active model = %q, want b", cfg.Model) + } + }) + + t.Run("official provider restores entry", func(t *testing.T) { + cfg := &Config{ + Provider: "op", + Providers: map[string]ProviderEntry{"op": {}}, + } + m := &modelTUIModel{ + existingCfg: cfg, + providerName: "op", + isCustomProvider: false, + } + prev := ProviderEntry{Models: []string{"x"}, Model: "x"} + m.rollbackModelDelete(prev, "x") + if got := cfg.Providers["op"]; len(got.Models) != 1 { + t.Errorf("restored models = %v, want 1 entry", got.Models) + } + }) +} + +func TestModelReloadConfigAfterSaveFailure(t *testing.T) { + t.Run("empty path returns false", func(t *testing.T) { + m := &modelTUIModel{configPath: ""} + if m.reloadConfigAfterSaveFailure() { + t.Error("expected false for empty configPath") + } + }) + + t.Run("reloads from disk", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"provider":"cp","custom_providers":{"cp":{"models":["m1"]}}}`), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + m := &modelTUIModel{ + configPath: path, + providerName: "cp", + isCustomProvider: true, + } + if !m.reloadConfigAfterSaveFailure() { + t.Fatal("expected reload to succeed") + } + if m.existingCfg == nil || m.existingCfg.Provider != "cp" { + t.Errorf("reloaded cfg = %+v, want provider cp", m.existingCfg) + } + }) + + t.Run("parse error returns false", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "bad.json") + if err := os.WriteFile(path, []byte("{not json"), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + m := &modelTUIModel{configPath: path} + if m.reloadConfigAfterSaveFailure() { + t.Error("expected false on parse error") + } + }) +} + +func TestProviderReloadConfigAfterSaveFailure(t *testing.T) { + t.Run("empty path returns false", func(t *testing.T) { + m := &providerTUIModel{configPath: ""} + if m.reloadConfigAfterSaveFailure() { + t.Error("expected false for empty configPath") + } + }) + + t.Run("reloads and refreshes custom providers", func(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "config.json") + if err := os.WriteFile(path, []byte(`{"custom_providers":{"cp":{"url":"http://x","models":["m1"]}}}`), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + m := &providerTUIModel{configPath: path} + if !m.reloadConfigAfterSaveFailure() { + t.Fatal("expected reload to succeed") + } + if m.existingCfg == nil { + t.Fatal("existingCfg not set after reload") + } + }) +} + +func TestAdjustModelIdxAfterDelete(t *testing.T) { + cfg := &Config{ + CustomProviders: map[string]ProviderEntry{"cp": {Models: []string{"a"}}}, + } + m := &modelTUIModel{ + existingCfg: cfg, + providerName: "cp", + isCustomProvider: true, + models: []string{"a", "b"}, + modelIdx: 5, // out of range + } + m.adjustModelIdxAfterDelete() + if m.modelIdx != 0 { + t.Errorf("modelIdx = %d, want clamped to 0 (single remaining model)", m.modelIdx) + } +} + +func TestRefreshModelSelectionAfterAdd(t *testing.T) { + t.Run("selects matching name", func(t *testing.T) { + m := &modelTUIModel{isCustomProvider: true, models: []string{"a", "b", "c"}} + m.refreshModelSelectionAfterAdd("b") + if m.modelIdx != 1 { + t.Errorf("modelIdx = %d, want 1", m.modelIdx) + } + }) + t.Run("falls back to last when absent", func(t *testing.T) { + m := &modelTUIModel{isCustomProvider: true, models: []string{"a", "b"}} + m.refreshModelSelectionAfterAdd("missing") + if m.modelIdx != 1 { + t.Errorf("modelIdx = %d, want last index 1", m.modelIdx) + } + }) +} diff --git a/cmd/opencodereview/provider_tui_savefail_test.go b/cmd/opencodereview/provider_tui_savefail_test.go new file mode 100644 index 0000000..0a32e5b --- /dev/null +++ b/cmd/opencodereview/provider_tui_savefail_test.go @@ -0,0 +1,150 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// unwritableConfigPath returns a config path whose parent is a regular file, so +// any saveConfig / loadOrCreateConfig against it fails (ENOTDIR). This is the +// lever used to drive the save-failure + rollback branches of the TUI handlers. +func unwritableConfigPath(t *testing.T) string { + t.Helper() + dir := t.TempDir() + blocker := filepath.Join(dir, "blocker") + if err := os.WriteFile(blocker, []byte("x"), 0o644); err != nil { + t.Fatalf("write blocker: %v", err) + } + return filepath.Join(blocker, "config.json") +} + +// TestUpdateDeleteConfirm_SaveFailure drives the provider-delete confirm handler +// down its save-failure branch: the in-memory delete happens, saveConfig fails, +// and the handler surfaces a formError without marking savedInSession. +func TestUpdateDeleteConfirm_SaveFailure(t *testing.T) { + cfg := &Config{ + Provider: "cp", + Model: "m1", + CustomProviders: map[string]ProviderEntry{ + "cp": {URL: "https://x.example", Protocol: "openai", Models: []string{"m1"}}, + }, + } + m := newProviderTUI(cfg, unwritableConfigPath(t)) + m.activeTab = tabCustom + m.confirmingDelete = true + m.deleteTargetIdx = 0 + m.deleteTargetName = "cp" + + out, _ := m.updateDeleteConfirm("y") + got := out.(providerTUIModel) + + if !strings.Contains(got.formError, "failed to save") { + t.Errorf("formError = %q, want save-failure message", got.formError) + } + if got.savedInSession { + t.Error("savedInSession should be false after save failure") + } + if got.confirmingDelete { + t.Error("confirmingDelete should be cleared after handling") + } +} + +// TestConfirmDeleteCustomModel_SaveFailureRollback drives the custom-model delete +// handler down its save-failure branch and asserts the entry is rolled back to +// its pre-delete state (reload fails, so the in-memory rollback runs). +func TestConfirmDeleteCustomModel_SaveFailureRollback(t *testing.T) { + cfg := &Config{ + Provider: "cp", + Model: "m2", + CustomProviders: map[string]ProviderEntry{ + "cp": { + URL: "https://x.example", + Protocol: "openai", + Models: []string{"m1", "m2"}, + Model: "m2", + }, + }, + } + m := newProviderTUI(cfg, unwritableConfigPath(t)) + m.activeTab = tabCustom + m.customIdx = 0 + m.step = stepModel + m.modelIdx = 1 + m.deleteModelName = "m2" + m.confirmingDeleteModel = true + + out, _ := m.confirmDeleteCustomModel() + got := out.(providerTUIModel) + + if !strings.Contains(got.formError, "failed to save") { + t.Errorf("formError = %q, want save-failure message", got.formError) + } + if got.savedInSession { + t.Error("savedInSession should be false after save failure") + } + // Rollback restored the model list in the in-memory config. + entry := got.existingCfg.CustomProviders["cp"] + if len(entry.Models) != 2 { + t.Errorf("rolled-back models = %v, want [m1 m2]", entry.Models) + } + if entry.Model != "m2" { + t.Errorf("rolled-back active model = %q, want m2", entry.Model) + } +} + +// TestConfirmDeleteOfficialModel_SaveFailureRollback drives the official-model +// delete handler down its save-failure branch for a user-added model and asserts +// the provider entry is rolled back. +func TestConfirmDeleteOfficialModel_SaveFailureRollback(t *testing.T) { + m := newProviderTUI(&Config{}, unwritableConfigPath(t)) + m.activeTab = tabOfficial + provider := m.currentProvider() + if provider.Name == "" { + t.Skip("no official provider available") + } + userModel := "user-added-model-xyz" + cfg := &Config{ + Provider: provider.Name, + Model: userModel, + Providers: map[string]ProviderEntry{ + provider.Name: {Models: []string{userModel}, Model: userModel}, + }, + } + m.existingCfg = cfg + m.step = stepModel + m.deleteModelName = userModel + m.confirmingDeleteModel = true + + if !m.isUserAddedOfficialModel(userModel) { + t.Fatalf("test setup: %q not recognized as user-added official model", userModel) + } + + out, _ := m.confirmDeleteOfficialModel() + got := out.(providerTUIModel) + + if !strings.Contains(got.formError, "failed to save") { + t.Errorf("formError = %q, want save-failure message", got.formError) + } + if got.savedInSession { + t.Error("savedInSession should be false after save failure") + } + // Rollback restored the user-added model. + entry := got.existingCfg.Providers[provider.Name] + if !containsStr(entry.Models, userModel) { + t.Errorf("rolled-back models = %v, want to contain %q", entry.Models, userModel) + } +} + +func containsStr(list []string, want string) bool { + for _, s := range list { + if s == want { + return true + } + } + return false +} diff --git a/cmd/opencodereview/review_helpers_test.go b/cmd/opencodereview/review_helpers_test.go new file mode 100644 index 0000000..83cb96e --- /dev/null +++ b/cmd/opencodereview/review_helpers_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "context" + "testing" + + "github.com/alibaba/open-code-review/internal/tool" +) + +func TestRunPreview(t *testing.T) { + dir := initTestGitRepo(t) + gitCommitFile(t, dir, "x.go", "package x\n", "add x") + cc, err := loadCommonContext(dir, "", 0, 0, true) + if err != nil { + t.Fatalf("loadCommonContext: %v", err) + } + silenceStdout(t, func() { + if err := runPreview(cc, reviewOptions{commit: "HEAD"}); err != nil { + t.Fatalf("runPreview error: %v", err) + } + }) +} + +func TestLoadReviewResumeState(t *testing.T) { + dir := initTestGitRepo(t) + + t.Run("empty resume returns nil", func(t *testing.T) { + state, err := loadReviewResumeState(dir, reviewOptions{}) + if err != nil || state != nil { + t.Errorf("got state=%v err=%v, want nil,nil", state, err) + } + }) + + t.Run("workspace resume rejected", func(t *testing.T) { + _, err := loadReviewResumeState(dir, reviewOptions{resume: "sess-1"}) + if err == nil { + t.Fatal("expected error for workspace-mode resume") + } + }) + + t.Run("missing session load fails", func(t *testing.T) { + _, err := loadReviewResumeState(dir, reviewOptions{resume: "does-not-exist", commit: "HEAD"}) + if err == nil { + t.Fatal("expected error loading nonexistent resume session") + } + }) +} + +func TestInitMCPClients(t *testing.T) { + ctx := context.Background() + reg := tool.NewRegistry() + + t.Run("nil config", func(t *testing.T) { + if got := initMCPClients(ctx, nil, reg, "/tmp", "v"); got != nil { + t.Errorf("got %v, want nil", got) + } + }) + + t.Run("empty servers", func(t *testing.T) { + if got := initMCPClients(ctx, &Config{}, reg, "/tmp", "v"); got != nil { + t.Errorf("got %v, want nil", got) + } + }) + + t.Run("remote without url skipped", func(t *testing.T) { + cfg := &Config{MCPServers: map[string]MCPServerConfig{ + "r": {Type: "remote"}, + }} + if got := initMCPClients(ctx, cfg, reg, "/tmp", "v"); len(got) != 0 { + t.Errorf("got %d clients, want 0", len(got)) + } + }) + + t.Run("stdio without command skipped", func(t *testing.T) { + cfg := &Config{MCPServers: map[string]MCPServerConfig{ + "s": {Type: "stdio"}, + }} + if got := initMCPClients(ctx, cfg, reg, "/tmp", "v"); len(got) != 0 { + t.Errorf("got %d clients, want 0", len(got)) + } + }) +} diff --git a/cmd/opencodereview/review_mcp_more_test.go b/cmd/opencodereview/review_mcp_more_test.go new file mode 100644 index 0000000..e565565 --- /dev/null +++ b/cmd/opencodereview/review_mcp_more_test.go @@ -0,0 +1,81 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "context" + "os" + "testing" + + "github.com/alibaba/open-code-review/internal/tool" +) + +// silenceStderr redirects os.Stderr to /dev/null for the duration of fn so the +// MCP init warnings do not clutter test logs. +func silenceStderr(t *testing.T, fn func()) { + t.Helper() + orig := os.Stderr + devnull, err := os.OpenFile(os.DevNull, os.O_WRONLY, 0) + if err != nil { + t.Fatalf("open devnull: %v", err) + } + os.Stderr = devnull + defer func() { + os.Stderr = orig + _ = devnull.Close() + }() + fn() +} + +// TestInitMCPClients_ErrorBranches covers the connect/start/setup failure paths +// that end in a "skip this server" continue. Each server fails fast (refused +// connection, missing binary, non-zero setup) so no real MCP server is needed. +func TestInitMCPClients_ErrorBranches(t *testing.T) { + ctx := context.Background() + + t.Run("remote connect failure skipped", func(t *testing.T) { + reg := tool.NewRegistry() + cfg := &Config{MCPServers: map[string]MCPServerConfig{ + // Port 1 is reserved and refuses connections immediately. + "r": {Type: "remote", URL: "http://127.0.0.1:1/mcp"}, + }} + var clients []interface{} + silenceStderr(t, func() { + for _, c := range initMCPClients(ctx, cfg, reg, t.TempDir(), "v") { + clients = append(clients, c) + } + }) + if len(clients) != 0 { + t.Errorf("got %d clients, want 0 (connect should fail)", len(clients)) + } + }) + + t.Run("stdio start failure skipped", func(t *testing.T) { + reg := tool.NewRegistry() + cfg := &Config{MCPServers: map[string]MCPServerConfig{ + "s": {Type: "stdio", Command: "ocr-nonexistent-binary-xyz"}, + }} + var n int + silenceStderr(t, func() { + n = len(initMCPClients(ctx, cfg, reg, t.TempDir(), "v")) + }) + if n != 0 { + t.Errorf("got %d clients, want 0 (start should fail)", n) + } + }) + + t.Run("setup failure skips server", func(t *testing.T) { + reg := tool.NewRegistry() + cfg := &Config{MCPServers: map[string]MCPServerConfig{ + "s": {Type: "stdio", Command: "true", Setup: "exit 1"}, + }} + var n int + silenceStderr(t, func() { + n = len(initMCPClients(ctx, cfg, reg, t.TempDir(), "v")) + }) + if n != 0 { + t.Errorf("got %d clients, want 0 (setup should fail and skip)", n) + } + }) +} diff --git a/cmd/opencodereview/review_resume_more_test.go b/cmd/opencodereview/review_resume_more_test.go new file mode 100644 index 0000000..edeb59d --- /dev/null +++ b/cmd/opencodereview/review_resume_more_test.go @@ -0,0 +1,71 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/session" +) + +// writeRangeResumeSession persists a range-mode session with the given completed +// file checkpoints and returns its ID. HOME must already point at a temp dir. +func writeRangeResumeSession(t *testing.T, repoDir string, files ...string) string { + t.Helper() + sh := session.New(repoDir, "feature", "fake", session.SessionOptions{ + ReviewMode: session.ReviewModeRange, + DiffFrom: "main", + DiffTo: "feature", + }) + for _, f := range files { + sh.RecordReviewItemDone(f, "", f, "fp-"+f, nil) + } + if err := sh.Finalize(); err != nil { + t.Fatalf("finalize session: %v", err) + } + return sh.SessionID +} + +// TestLoadReviewResumeState_WithSession drives the fixture-backed branches of +// loadReviewResumeState: a successful resume, a review-mode mismatch, and a +// session that completed no items. +func TestLoadReviewResumeState_WithSession(t *testing.T) { + t.Run("success returns state with completed items", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + id := writeRangeResumeSession(t, repoDir, "a.go", "b.go") + + state, err := loadReviewResumeState(repoDir, reviewOptions{resume: id, from: "main", to: "feature"}) + if err != nil { + t.Fatalf("loadReviewResumeState: %v", err) + } + if state == nil || state.CompletedCount() != 2 { + t.Fatalf("got %v, want state with 2 completed items", state) + } + }) + + t.Run("review mode mismatch errors", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + id := writeRangeResumeSession(t, repoDir, "a.go") + + // Session was range-mode; request commit-mode resume. + _, err := loadReviewResumeState(repoDir, reviewOptions{resume: id, commit: "HEAD"}) + if err == nil { + t.Fatal("expected error for mode mismatch") + } + }) + + t.Run("no completed items errors", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + id := writeRangeResumeSession(t, repoDir) // no items recorded + + _, err := loadReviewResumeState(repoDir, reviewOptions{resume: id, from: "main", to: "feature"}) + if err == nil || !strings.Contains(err.Error(), "no completed review items") { + t.Fatalf("got %v, want no-completed-items error", err) + } + }) +} diff --git a/cmd/opencodereview/rules_check_test.go b/cmd/opencodereview/rules_check_test.go new file mode 100644 index 0000000..6896cfb --- /dev/null +++ b/cmd/opencodereview/rules_check_test.go @@ -0,0 +1,36 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" +) + +// TestRunRulesCheck drives the full `ocr rules check` helper against a real git +// repo so the resolver-build, DetailResolver assertion, and formatted print +// path all run. +func TestRunRulesCheck(t *testing.T) { + dir := initTestGitRepo(t) + + // runRulesCheck reads the package-level flag var; set and restore it. + prev := rulesCheckRepoDir + rulesCheckRepoDir = dir + t.Cleanup(func() { rulesCheckRepoDir = prev }) + + t.Run("resolves a rule for a Go file", func(t *testing.T) { + silenceStdout(t, func() { + if err := runRulesCheck("internal/foo/bar.go"); err != nil { + t.Fatalf("runRulesCheck error: %v", err) + } + }) + }) + + t.Run("non-git repo dir errors", func(t *testing.T) { + rulesCheckRepoDir = t.TempDir() + defer func() { rulesCheckRepoDir = dir }() + if err := runRulesCheck("x.go"); err == nil { + t.Fatal("expected error for non-git repo dir") + } + }) +} diff --git a/cmd/opencodereview/scan_helpers_test.go b/cmd/opencodereview/scan_helpers_test.go new file mode 100644 index 0000000..87bc878 --- /dev/null +++ b/cmd/opencodereview/scan_helpers_test.go @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" + + "github.com/alibaba/open-code-review/internal/config/template" +) + +func TestLoadScanResumeState(t *testing.T) { + dir := initTestGitRepo(t) + + t.Run("empty resume returns nil", func(t *testing.T) { + state, err := loadScanResumeState(dir, scanOptions{}, nil) + if err != nil || state != nil { + t.Errorf("got state=%v err=%v, want nil,nil", state, err) + } + }) + + t.Run("missing session load fails", func(t *testing.T) { + _, err := loadScanResumeState(dir, scanOptions{resume: "nope"}, nil) + if err == nil { + t.Fatal("expected error loading nonexistent resume session") + } + }) +} + +func TestRunScanPreview(t *testing.T) { + dir := initTestGitRepo(t) + gitCommitFile(t, dir, "y.go", "package y\n", "add y") + cc, err := loadCommonContext(dir, "", 0, 0, false) + if err != nil { + t.Fatalf("loadCommonContext: %v", err) + } + scanTpl, err := template.LoadScanDefault() + if err != nil { + t.Fatalf("LoadScanDefault: %v", err) + } + silenceStdout(t, func() { + if err := runScanPreview(cc, scanTpl, nil); err != nil { + t.Fatalf("runScanPreview error: %v", err) + } + }) +} diff --git a/cmd/opencodereview/scan_resume_more_test.go b/cmd/opencodereview/scan_resume_more_test.go new file mode 100644 index 0000000..221fc42 --- /dev/null +++ b/cmd/opencodereview/scan_resume_more_test.go @@ -0,0 +1,69 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/session" +) + +// writeScanResumeSession persists a full-scan session with the given completed +// file checkpoints and returns its ID. HOME must already point at a temp dir. +func writeScanResumeSession(t *testing.T, repoDir string, files ...string) string { + t.Helper() + sh := session.New(repoDir, "feature", "fake", session.SessionOptions{ + ReviewMode: session.ReviewModeFullScan, + }) + for _, f := range files { + sh.RecordReviewItemDone(f, "", f, "fp-"+f, nil) + } + if err := sh.Finalize(); err != nil { + t.Fatalf("finalize session: %v", err) + } + return sh.SessionID +} + +// TestLoadScanResumeState_WithSession drives the fixture-backed branches of +// loadScanResumeState: a successful resume, a scan-mode mismatch, and a session +// that completed no items. +func TestLoadScanResumeState_WithSession(t *testing.T) { + t.Run("success returns state with completed items", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + id := writeScanResumeSession(t, repoDir, "a.go", "b.go") + + state, err := loadScanResumeState(repoDir, scanOptions{resume: id}, nil) + if err != nil { + t.Fatalf("loadScanResumeState: %v", err) + } + if state == nil || state.CompletedCount() != 2 { + t.Fatalf("got %v, want state with 2 completed items", state) + } + }) + + t.Run("non-scan session rejected", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + // Persist a range-mode session, then try to resume it as a scan. + id := writeRangeResumeSession(t, repoDir, "a.go") + + _, err := loadScanResumeState(repoDir, scanOptions{resume: id}, nil) + if err == nil { + t.Fatal("expected error resuming a non-scan session") + } + }) + + t.Run("no completed items errors", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + id := writeScanResumeSession(t, repoDir) // no items recorded + + _, err := loadScanResumeState(repoDir, scanOptions{resume: id}, nil) + if err == nil || !strings.Contains(err.Error(), "no completed scan items") { + t.Fatalf("got %v, want no-completed-items error", err) + } + }) +} diff --git a/cmd/opencodereview/session_cmd_test.go b/cmd/opencodereview/session_cmd_test.go index 366a6e1..ce9a570 100644 --- a/cmd/opencodereview/session_cmd_test.go +++ b/cmd/opencodereview/session_cmd_test.go @@ -296,6 +296,30 @@ func TestTruncateUnicode(t *testing.T) { } } +// TestTruncate covers the remaining branches of truncate: newline/tab +// normalization, the short-enough pass-through, and the n<=1 ellipsis-only case. +func TestTruncate(t *testing.T) { + cases := []struct { + name string + s string + n int + want string + }{ + {"shorter than limit is unchanged", "abc", 10, "abc"}, + {"newlines and tabs become spaces", "a\nb\tc", 10, "a b c"}, + {"n of one collapses to ellipsis", "abcdef", 1, "…"}, + {"n of zero collapses to ellipsis", "abcdef", 0, "…"}, + {"exact length is unchanged", "abcd", 4, "abcd"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := truncate(tc.s, tc.n); got != tc.want { + t.Errorf("truncate(%q, %d) = %q, want %q", tc.s, tc.n, got, tc.want) + } + }) + } +} + func TestRunSession_UnknownSubcommand(t *testing.T) { err := runSession([]string{"bogus"}) if err == nil { diff --git a/cmd/opencodereview/session_complete_test.go b/cmd/opencodereview/session_complete_test.go new file mode 100644 index 0000000..7b86e57 --- /dev/null +++ b/cmd/opencodereview/session_complete_test.go @@ -0,0 +1,43 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" + + "github.com/spf13/cobra" +) + +// TestCompleteSessionIDs_WithSession drives the success loop of the shell +// completion helper against a real fixture session, covering the prefix-match +// and no-match branches that the fresh-repo test cannot reach. +func TestCompleteSessionIDs_WithSession(t *testing.T) { + newCmd := func(repo string) *cobra.Command { + c := &cobra.Command{} + c.Flags().String("repo", repo, "") + return c + } + + t.Run("lists matching session IDs", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + id := writeRangeResumeSession(t, repoDir, "a.go") + + got, _ := completeSessionIDs(newCmd(repoDir), nil, id[:4]) + if len(got) == 0 { + t.Fatalf("expected a completion for session %s, got none", id) + } + }) + + t.Run("prefix that matches nothing yields empty list", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + writeRangeResumeSession(t, repoDir, "a.go") + + got, _ := completeSessionIDs(newCmd(repoDir), nil, "zzzz-no-match") + if len(got) != 0 { + t.Errorf("got %v, want empty", got) + } + }) +} diff --git a/cmd/opencodereview/session_display_more_test.go b/cmd/opencodereview/session_display_more_test.go new file mode 100644 index 0000000..25e3387 --- /dev/null +++ b/cmd/opencodereview/session_display_more_test.go @@ -0,0 +1,99 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "testing" + + "github.com/alibaba/open-code-review/internal/session" +) + +// TestDisplayMode covers the empty and non-empty branches. +func TestDisplayMode(t *testing.T) { + if got := displayMode(""); got != "-" { + t.Errorf("displayMode(\"\") = %q, want -", got) + } + if got := displayMode("range"); got != "range" { + t.Errorf("displayMode(range) = %q, want range", got) + } +} + +// TestDescribeRange covers each review-mode branch plus the fallthrough. +func TestDescribeRange(t *testing.T) { + tests := []struct { + name string + summary session.Summary + want string + }{ + { + name: "range with endpoints", + summary: session.Summary{ReviewMode: session.ReviewModeRange, DiffFrom: "a", DiffTo: "b"}, + want: "a..b", + }, + { + name: "range without endpoints", + summary: session.Summary{ReviewMode: session.ReviewModeRange}, + want: "-", + }, + { + name: "commit", + summary: session.Summary{ReviewMode: session.ReviewModeCommit, DiffCommit: "abc123"}, + want: "abc123", + }, + { + name: "commit without value", + summary: session.Summary{ReviewMode: session.ReviewModeCommit}, + want: "-", + }, + { + name: "other mode", + summary: session.Summary{}, + want: "-", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := describeRange(tt.summary); got != tt.want { + t.Errorf("describeRange() = %q, want %q", got, tt.want) + } + }) + } +} + +// TestDescribeStart covers the zero-time and formatted branches. +func TestDescribeStart(t *testing.T) { + if got := describeStart(session.Summary{}); got != "-" { + t.Errorf("describeStart(zero) = %q, want -", got) + } + s := session.Summary{} + s.StartTime = s.StartTime.AddDate(2024, 0, 0) // any non-zero time + if got := describeStart(s); got == "-" { + t.Error("describeStart(non-zero) should not be -") + } +} + +// TestDescribeFilesNoManifest covers the branch where RunManifest is nil. +func TestDescribeFilesNoManifest(t *testing.T) { + s := session.Summary{CompletedFiles: 3} + if got := describeFiles(s); got != "3" { + t.Errorf("describeFiles(no manifest) = %q, want 3", got) + } + s.ReusedFiles = 2 + if got := describeFiles(s); got != "5 (reused 2)" { + t.Errorf("describeFiles(reused) = %q, want 5 (reused 2)", got) + } +} + +// TestCompleteEnum verifies the closure returns the provided values with the +// no-file-completion directive. +func TestCompleteEnum(t *testing.T) { + fn := completeEnum("a", "b", "c") + values, directive := fn(nil, nil, "") + if len(values) != 3 || values[0] != "a" { + t.Errorf("completeEnum values = %v, want [a b c]", values) + } + if directive == 0 { + t.Error("expected a non-zero shell completion directive") + } +} diff --git a/cmd/opencodereview/shared_llmruntime_test.go b/cmd/opencodereview/shared_llmruntime_test.go new file mode 100644 index 0000000..6f88434 --- /dev/null +++ b/cmd/opencodereview/shared_llmruntime_test.go @@ -0,0 +1,104 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package main + +import ( + "os" + "path/filepath" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/config/template" + "github.com/alibaba/open-code-review/internal/llm" +) + +// loadTestTemplate returns a validated default template for runtime tests. +func loadTestTemplate(t *testing.T) *template.Template { + t.Helper() + tpl, err := template.LoadDefault() + if err != nil { + t.Fatalf("LoadDefault: %v", err) + } + return tpl +} + +// TestLoadLLMRuntime_Success resolves an endpoint via OCR_LLM_* env vars (no +// config file on disk, so LoadAppConfig returns nil,nil) and asserts the +// runtime bundle is fully populated. +func TestLoadLLMRuntime_Success(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + t.Setenv("OCR_LLM_URL", "https://api.example.test/v1") + t.Setenv("OCR_LLM_TOKEN", "tok-123") + t.Setenv("OCR_LLM_MODEL", "test-model") + + tpl := loadTestTemplate(t) + rt, err := loadLLMRuntime(tpl, "", llm.ResolveOptions{}) + if err != nil { + t.Fatalf("loadLLMRuntime error: %v", err) + } + if rt.Model != "test-model" { + t.Errorf("model = %q, want test-model", rt.Model) + } + if rt.Client == nil { + t.Error("expected non-nil client") + } + if rt.Collector == nil { + t.Error("expected non-nil collector") + } + if len(rt.MainToolDefs) == 0 { + t.Error("expected main tool defs") + } + if rt.RuntimeConfig.EndpointHost != "api.example.test" { + t.Errorf("endpoint host = %q, want api.example.test", rt.RuntimeConfig.EndpointHost) + } +} + +// TestLoadLLMRuntime_BadToolConfig covers the toolsconfig.Load failure branch. +func TestLoadLLMRuntime_BadToolConfig(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + tpl := loadTestTemplate(t) + _, err := loadLLMRuntime(tpl, filepath.Join(t.TempDir(), "no-such-tools.json"), llm.ResolveOptions{}) + if err == nil || !strings.Contains(err.Error(), "load tools") { + t.Fatalf("err = %v, want load-tools failure", err) + } +} + +// TestLoadLLMRuntime_UnresolvableEndpoint covers the ResolveEndpointWithOptions +// failure branch: no config file and no env vars means no endpoint resolves. +func TestLoadLLMRuntime_UnresolvableEndpoint(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + // Clear any inherited resolution sources. + t.Setenv("OCR_LLM_URL", "") + t.Setenv("OCR_LLM_TOKEN", "") + t.Setenv("OCR_LLM_MODEL", "") + t.Setenv("ANTHROPIC_BASE_URL", "") + t.Setenv("ANTHROPIC_AUTH_TOKEN", "") + t.Setenv("ANTHROPIC_MODEL", "") + + tpl := loadTestTemplate(t) + _, err := loadLLMRuntime(tpl, "", llm.ResolveOptions{}) + if err == nil || !strings.Contains(err.Error(), "resolve LLM endpoint") { + t.Fatalf("err = %v, want resolve-endpoint failure", err) + } +} + +// TestLoadLLMRuntime_BadAppConfig covers the LoadAppConfig parse-failure branch +// by writing an invalid config.json at the default HOME-based path. +func TestLoadLLMRuntime_BadAppConfig(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + cfgDir := filepath.Join(home, ".opencodereview") + if err := os.MkdirAll(cfgDir, 0o755); err != nil { + t.Fatalf("mkdir: %v", err) + } + if err := os.WriteFile(filepath.Join(cfgDir, "config.json"), []byte("{not json"), 0o644); err != nil { + t.Fatalf("write config: %v", err) + } + + tpl := loadTestTemplate(t) + _, err := loadLLMRuntime(tpl, "", llm.ResolveOptions{}) + if err == nil || !strings.Contains(err.Error(), "load app config") { + t.Fatalf("err = %v, want load-app-config failure", err) + } +} diff --git a/internal/agent/agent_test.go b/internal/agent/agent_test.go index 1fd4a55..c069269 100644 --- a/internal/agent/agent_test.go +++ b/internal/agent/agent_test.go @@ -615,6 +615,31 @@ func TestApplyResumeReusesCompletedItemsAcrossModels(t *testing.T) { } } +// TestAgentGettersNil covers the defensive early returns in the accessor +// methods when the agent (or its session) was never fully constructed, so +// callers never advertise a resume target that does not exist. +func TestAgentGettersNil(t *testing.T) { + var nilAgent *Agent + if got := nilAgent.SessionID(); got != "" { + t.Errorf("nil agent SessionID = %q, want empty", got) + } + if got := nilAgent.RunManifest(); got != nil { + t.Errorf("nil agent RunManifest = %v, want nil", got) + } + + // An agent with no session must also return the empty/nil sentinels. + empty := &Agent{} + if got := empty.SessionID(); got != "" { + t.Errorf("sessionless SessionID = %q, want empty", got) + } + if got := empty.RunManifest(); got != nil { + t.Errorf("sessionless RunManifest = %v, want nil", got) + } + if got := empty.ResumeInfo(); got != nil { + t.Errorf("resumeless ResumeInfo = %v, want nil", got) + } +} + func TestCountReviewable(t *testing.T) { a := New(Args{}) diffs := []model.Diff{ diff --git a/internal/agent/getters_test.go b/internal/agent/getters_test.go new file mode 100644 index 0000000..83bf0f6 --- /dev/null +++ b/internal/agent/getters_test.go @@ -0,0 +1,28 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package agent + +import "testing" + +// TestAgentGettersNilSafe covers the nil-safe guard paths of the small +// ResultProvider getters, which the happy-path budget/manifest tests do not +// exercise. +func TestAgentGettersNilSafe(t *testing.T) { + a := &Agent{} + + if got := a.SessionID(); got != "" { + t.Errorf("SessionID() on session-less agent = %q, want empty", got) + } + if got := a.RunManifest(); got != nil { + t.Errorf("RunManifest() on session-less agent = %v, want nil", got) + } + + var nilAgent *Agent + if got := nilAgent.SessionID(); got != "" { + t.Errorf("nil-receiver SessionID() = %q, want empty", got) + } + if got := nilAgent.RunManifest(); got != nil { + t.Errorf("nil-receiver RunManifest() = %v, want nil", got) + } +} diff --git a/internal/agent/preview_run_test.go b/internal/agent/preview_run_test.go new file mode 100644 index 0000000..884702f --- /dev/null +++ b/internal/agent/preview_run_test.go @@ -0,0 +1,64 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package agent + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "testing" +) + +func initPreviewRepo(t *testing.T) string { + t.Helper() + dir := t.TempDir() + run := func(args ...string) { + cmd := exec.Command("git", append([]string{"-C", dir}, args...)...) + cmd.Env = append(os.Environ(), + "GIT_AUTHOR_NAME=t", "GIT_AUTHOR_EMAIL=t@t", + "GIT_COMMITTER_NAME=t", "GIT_COMMITTER_EMAIL=t@t") + if out, err := cmd.CombinedOutput(); err != nil { + t.Fatalf("git %v: %v: %s", args, err, out) + } + } + run("init") + run("config", "user.email", "t@t") + run("config", "user.name", "t") + if err := os.WriteFile(filepath.Join(dir, "README.md"), []byte("# r\n"), 0o644); err != nil { + t.Fatalf("write README: %v", err) + } + run("add", ".") + run("commit", "-m", "init") + return dir +} + +// TestPreview exercises Agent.Preview against a real workspace diff so the +// full preview-building path (loadDiffs + whyExcluded + entry assembly) runs. +func TestPreview(t *testing.T) { + dir := initPreviewRepo(t) + + // A reviewable Go file and an excluded binary-ish/extension file. + if err := os.WriteFile(filepath.Join(dir, "main.go"), []byte("package main\n"), 0o644); err != nil { + t.Fatalf("write main.go: %v", err) + } + if err := os.WriteFile(filepath.Join(dir, "data.bin"), []byte{0x00, 0x01, 0x02}, 0o644); err != nil { + t.Fatalf("write data.bin: %v", err) + } + + a := New(Args{RepoDir: dir}) + preview, err := a.Preview(context.Background()) + if err != nil { + t.Fatalf("Preview error: %v", err) + } + if preview.TotalFiles == 0 { + t.Fatal("Preview reported zero files despite workspace changes") + } + if preview.ReviewableCount == 0 { + t.Error("expected at least one reviewable entry (main.go)") + } + if len(preview.Entries) != preview.TotalFiles { + t.Errorf("entries=%d totalFiles=%d, want equal", len(preview.Entries), preview.TotalFiles) + } +} diff --git a/internal/config/rules/system_rules_test.go b/internal/config/rules/system_rules_test.go index 4eec0b6..919ff19 100644 --- a/internal/config/rules/system_rules_test.go +++ b/internal/config/rules/system_rules_test.go @@ -1592,3 +1592,100 @@ func TestSystemRulesIntegrity(t *testing.T) { } }) } + +// TestLoadRuleFile covers loadRuleFile's read-error and unmarshal-error +// branches plus the success path that resolves entries and returns the rule. +func TestLoadRuleFile(t *testing.T) { + t.Run("read error on missing path", func(t *testing.T) { + if _, err := loadRuleFile(filepath.Join(t.TempDir(), "nope.json")); err == nil { + t.Fatal("expected read error for missing rule file, got nil") + } + }) + + t.Run("unmarshal error on invalid JSON", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "rule.json") + if err := os.WriteFile(path, []byte("{not json"), 0o644); err != nil { + t.Fatalf("write invalid rule: %v", err) + } + if _, err := loadRuleFile(path); err == nil { + t.Fatal("expected unmarshal error for invalid JSON, got nil") + } + }) + + t.Run("valid file returns rule", func(t *testing.T) { + path := filepath.Join(t.TempDir(), "rule.json") + if err := os.WriteFile(path, []byte(`{"rules":[{"rule":"be careful"}]}`), 0o644); err != nil { + t.Fatalf("write valid rule: %v", err) + } + pr, err := loadRuleFile(path) + if err != nil { + t.Fatalf("loadRuleFile: %v", err) + } + if pr == nil || len(pr.Rules) != 1 || pr.Rules[0].Rule != "be careful" { + t.Errorf("unexpected rule: %+v", pr) + } + }) +} + +// TestLoadGlobalRule covers loadGlobalRule's non-NotExist read error, +// unmarshal error, and success branches by pointing HOME at a temp dir. +func TestLoadGlobalRule(t *testing.T) { + globalRulePath := func(home string) string { + return filepath.Join(home, ".opencodereview", "rule.json") + } + + t.Run("missing file is not an error", func(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + pr, err := loadGlobalRule() + if err != nil || pr != nil { + t.Fatalf("expected nil,nil for missing global rule: pr=%v err=%v", pr, err) + } + }) + + t.Run("read error when path is a directory", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + // Create the rule.json path as a directory so ReadFile fails with a + // non-NotExist error (EISDIR), exercising the wrapped-error branch. + if err := os.MkdirAll(globalRulePath(home), 0o755); err != nil { + t.Fatalf("mkdir rule path: %v", err) + } + if _, err := loadGlobalRule(); err == nil { + t.Fatal("expected read error when rule path is a directory, got nil") + } + }) + + t.Run("unmarshal error on invalid JSON", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := globalRulePath(home) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + if err := os.WriteFile(path, []byte("{bad"), 0o644); err != nil { + t.Fatalf("write invalid rule: %v", err) + } + if _, err := loadGlobalRule(); err == nil { + t.Fatal("expected unmarshal error for invalid global rule, got nil") + } + }) + + t.Run("valid file returns rule", func(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + path := globalRulePath(home) + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + if err := os.WriteFile(path, []byte(`{"rules":[{"rule":"global rule"}]}`), 0o644); err != nil { + t.Fatalf("write valid rule: %v", err) + } + pr, err := loadGlobalRule() + if err != nil { + t.Fatalf("loadGlobalRule: %v", err) + } + if pr == nil || len(pr.Rules) != 1 || pr.Rules[0].Rule != "global rule" { + t.Errorf("unexpected rule: %+v", pr) + } + }) +} diff --git a/internal/config/rules/system_rules_unmarshal_test.go b/internal/config/rules/system_rules_unmarshal_test.go new file mode 100644 index 0000000..6d0bbd9 --- /dev/null +++ b/internal/config/rules/system_rules_unmarshal_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package rules + +import ( + "encoding/json" + "strings" + "testing" +) + +// TestSystemRuleUnmarshalJSON exercises the custom order-preserving decoder in +// SystemRule.UnmarshalJSON: the happy path, the absent/null map short-circuits, +// and the malformed-input error branches. +func TestSystemRuleUnmarshalJSON(t *testing.T) { + t.Run("preserves path_rule_map key order", func(t *testing.T) { + var r SystemRule + in := `{"default_rule":"d.md","path_rule_map":{"*.go":"go.md","*.php":"php.md"}}` + if err := json.Unmarshal([]byte(in), &r); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if r.DefaultRule != "d.md" { + t.Errorf("DefaultRule = %q, want d.md", r.DefaultRule) + } + if len(r.PathRules) != 2 { + t.Fatalf("PathRules len = %d, want 2", len(r.PathRules)) + } + if r.PathRules[0].Pattern != "*.go" || r.PathRules[1].Pattern != "*.php" { + t.Errorf("order not preserved: %+v", r.PathRules) + } + }) + + t.Run("absent path_rule_map yields no rules", func(t *testing.T) { + var r SystemRule + if err := json.Unmarshal([]byte(`{"default_rule":"d.md"}`), &r); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(r.PathRules) != 0 { + t.Errorf("PathRules = %+v, want empty", r.PathRules) + } + }) + + t.Run("null path_rule_map yields no rules", func(t *testing.T) { + var r SystemRule + if err := json.Unmarshal([]byte(`{"default_rule":"d.md","path_rule_map":null}`), &r); err != nil { + t.Fatalf("unmarshal: %v", err) + } + if len(r.PathRules) != 0 { + t.Errorf("PathRules = %+v, want empty", r.PathRules) + } + }) + + errorCases := []struct { + name string + in string + want string + }{ + {"invalid top-level json", `{`, ""}, + {"path_rule_map is not an object", `{"default_rule":"d.md","path_rule_map":[1,2]}`, "expected '{'"}, + {"path_rule_map value is not a string", `{"default_rule":"d.md","path_rule_map":{"*.go":123}}`, "read path_rule_map value"}, + } + for _, tc := range errorCases { + t.Run(tc.name, func(t *testing.T) { + var r SystemRule + err := json.Unmarshal([]byte(tc.in), &r) + if err == nil { + t.Fatalf("in %q: expected error, got nil", tc.in) + } + if tc.want != "" && !strings.Contains(err.Error(), tc.want) { + t.Errorf("err = %q, want substring %q", err.Error(), tc.want) + } + }) + } +} diff --git a/internal/diff/first_line_test.go b/internal/diff/first_line_test.go new file mode 100644 index 0000000..bbec97f --- /dev/null +++ b/internal/diff/first_line_test.go @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package diff + +import "testing" + +// TestFirstLine covers firstLine: it returns the first non-empty trimmed line, +// skips leading blank lines, and returns "" when there is no non-empty line. +func TestFirstLine(t *testing.T) { + cases := []struct { + name string + in string + want string + }{ + {"single line with trailing newline", "abc123\n", "abc123"}, + {"trims surrounding whitespace", " deadbeef \n", "deadbeef"}, + {"skips leading blank lines", "\n\n sha\n", "sha"}, + {"empty input", "", ""}, + {"only whitespace and newlines", "\n \n\t\n", ""}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + if got := firstLine(tc.in); got != tc.want { + t.Errorf("firstLine(%q) = %q, want %q", tc.in, got, tc.want) + } + }) + } +} diff --git a/internal/diff/relocation_test.go b/internal/diff/relocation_test.go index 6600cc3..494dcef 100644 --- a/internal/diff/relocation_test.go +++ b/internal/diff/relocation_test.go @@ -207,6 +207,8 @@ func TestExtractCodeBlock(t *testing.T) { {"with surrounding text", "Here:\n```\ncode\n```\ndone", "code"}, {"no code block", "just text", ""}, {"empty block", "```\n```", ""}, + {"opening fence without newline", "```go", ""}, + {"no closing fence", "```\nfoo\nbar", ""}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/llm/client_params_test.go b/internal/llm/client_params_test.go new file mode 100644 index 0000000..c17be61 --- /dev/null +++ b/internal/llm/client_params_test.go @@ -0,0 +1,195 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "testing" +) + +// TestBuildOpenAIParams_AllRoles exercises every message-role branch plus the +// tools/max-tokens/temperature options in buildOpenAIParams. +func TestBuildOpenAIParams_AllRoles(t *testing.T) { + c := &OpenAIClient{} + temp := 0.5 + req := ChatRequest{ + Messages: []Message{ + {Role: "system", Content: "sys"}, + {Role: "user", Content: "hi"}, + {Role: "tool", Content: "result", ToolCallID: "call-1"}, + {Role: "assistant", Content: "plain"}, + {Role: "assistant", Content: "with tools", ToolCalls: []ToolCall{ + {ID: "call-2", Function: FunctionCall{Name: "f", Arguments: `{"a":1}`}}, + }}, + {Role: "assistant", Content: "", ToolCalls: []ToolCall{ + {ID: "call-3", Function: FunctionCall{Name: "g", Arguments: `{}`}}, + }}, + {Role: "unknown", Content: "fallback"}, + }, + Tools: []ToolDef{ + {Function: FunctionDef{Name: "f", Description: "d", Parameters: map[string]any{"type": "object"}}}, + }, + MaxTokens: 256, + Temperature: &temp, + } + + params := c.buildOpenAIParams("gpt-x", req) + + if string(params.Model) != "gpt-x" { + t.Errorf("model = %q, want gpt-x", params.Model) + } + if len(params.Messages) != len(req.Messages) { + t.Errorf("messages = %d, want %d", len(params.Messages), len(req.Messages)) + } + if len(params.Tools) != 1 { + t.Errorf("tools = %d, want 1", len(params.Tools)) + } + if params.MaxCompletionTokens.Value != 256 { + t.Errorf("max completion tokens = %d, want 256", params.MaxCompletionTokens.Value) + } + if params.Temperature.Value != 0.5 { + t.Errorf("temperature = %v, want 0.5", params.Temperature.Value) + } +} + +// TestBuildOpenAIParams_Minimal verifies that optional fields stay unset when +// the request omits tools, max tokens, and temperature. +func TestBuildOpenAIParams_Minimal(t *testing.T) { + c := &OpenAIClient{} + params := c.buildOpenAIParams("m", ChatRequest{ + Messages: []Message{{Role: "user", Content: "x"}}, + }) + if len(params.Tools) != 0 { + t.Errorf("tools = %d, want 0", len(params.Tools)) + } + if params.MaxCompletionTokens.Valid() { + t.Error("expected max completion tokens unset") + } + if params.Temperature.Valid() { + t.Error("expected temperature unset") + } +} + +// TestBuildAnthropicParams_AllRoles exercises every message-role branch (system, +// tool-result flushing, assistant with/without tool calls, user string and +// content-block content) plus tools/system cache-control and temperature. +func TestBuildAnthropicParams_AllRoles(t *testing.T) { + c := &AnthropicClient{} + temp := 0.3 + req := ChatRequest{ + Messages: []Message{ + {Role: "system", Content: "sys"}, + {Role: "tool", Content: "tool-result", ToolCallID: "call-1"}, + {Role: "assistant", Content: "with tools", ToolCalls: []ToolCall{ + {ID: "call-2", Function: FunctionCall{Name: "f", Arguments: `{"a":1}`}}, + }}, + {Role: "assistant", Content: "plain-no-tools"}, + {Role: "user", Content: "hi"}, + {Role: "user", Content: []ContentBlock{ + {Type: "text", Text: "block-text"}, + {Type: "tool_result", ToolUseID: "call-3", Text: "tr"}, + }}, + }, + Tools: []ToolDef{ + {Function: FunctionDef{Name: "f", Description: "d", Parameters: map[string]any{"type": "object"}}}, + }, + MaxTokens: 512, + Temperature: &temp, + } + + params, err := c.buildAnthropicParams("claude-x", req) + if err != nil { + t.Fatalf("buildAnthropicParams returned error: %v", err) + } + if string(params.Model) != "claude-x" { + t.Errorf("model = %q, want claude-x", params.Model) + } + if params.MaxTokens != 512 { + t.Errorf("max tokens = %d, want 512", params.MaxTokens) + } + if len(params.System) == 0 { + t.Error("expected system blocks to be set") + } + if len(params.Tools) != 1 { + t.Errorf("tools = %d, want 1", len(params.Tools)) + } + if !params.Temperature.Valid() || params.Temperature.Value != 0.3 { + t.Errorf("temperature = %v, want 0.3", params.Temperature) + } + if len(params.Messages) == 0 { + t.Error("expected messages to be built") + } +} + +// TestBuildAnthropicParams_InvalidToolArgs covers the error branch where an +// assistant tool call carries malformed JSON arguments. +func TestBuildAnthropicParams_InvalidToolArgs(t *testing.T) { + c := &AnthropicClient{} + _, err := c.buildAnthropicParams("claude-x", ChatRequest{ + Messages: []Message{ + {Role: "assistant", ToolCalls: []ToolCall{ + {ID: "call-1", Function: FunctionCall{Name: "bad", Arguments: `{not-json`}}, + }}, + }, + }) + if err == nil { + t.Fatal("expected error for invalid tool call arguments") + } +} + +// TestBuildAnthropicParams_DefaultMaxTokens verifies the fallback to 8192 when +// the request omits MaxTokens, and that optional fields stay unset. +func TestBuildAnthropicParams_DefaultMaxTokens(t *testing.T) { + c := &AnthropicClient{} + params, err := c.buildAnthropicParams("m", ChatRequest{ + Messages: []Message{{Role: "user", Content: "x"}}, + }) + if err != nil { + t.Fatalf("buildAnthropicParams returned error: %v", err) + } + if params.MaxTokens != 8192 { + t.Errorf("max tokens = %d, want default 8192", params.MaxTokens) + } + if len(params.Tools) != 0 { + t.Errorf("tools = %d, want 0", len(params.Tools)) + } + if len(params.System) != 0 { + t.Errorf("system = %d, want 0", len(params.System)) + } + if params.Temperature.Valid() { + t.Error("expected temperature unset") + } +} + +// TestBuildToolInputSchema covers properties, required filtering (non-string +// entries dropped), and extra-field passthrough. +func TestBuildToolInputSchema(t *testing.T) { + props := map[string]any{"a": map[string]any{"type": "string"}} + schema := buildToolInputSchema(map[string]any{ + "type": "object", + "properties": props, + "required": []any{"a", 42}, + "additionalProperties": false, + }) + + if schema.Properties == nil { + t.Error("properties not set") + } + if len(schema.Required) != 1 || schema.Required[0] != "a" { + t.Errorf("required = %v, want [a] (non-string dropped)", schema.Required) + } + if schema.ExtraFields == nil || schema.ExtraFields["additionalProperties"] != false { + t.Errorf("extra field additionalProperties not preserved: %v", schema.ExtraFields) + } + if _, ok := schema.ExtraFields["type"]; ok { + t.Error("reserved key 'type' should not appear in ExtraFields") + } +} + +// TestBuildToolInputSchema_Empty ensures a bare schema stays empty. +func TestBuildToolInputSchema_Empty(t *testing.T) { + schema := buildToolInputSchema(map[string]any{}) + if schema.Properties != nil || len(schema.Required) != 0 || schema.ExtraFields != nil { + t.Errorf("empty input produced non-empty schema: %+v", schema) + } +} diff --git a/internal/llm/resolver_norm_test.go b/internal/llm/resolver_norm_test.go new file mode 100644 index 0000000..7b4347e --- /dev/null +++ b/internal/llm/resolver_norm_test.go @@ -0,0 +1,70 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "testing" +) + +// TestNormalizeAuthHeader covers every branch of NormalizeAuthHeader, +// including the empty pass-through, the two canonical forms, the "bearer" +// alias, and the unsupported-value error. +func TestNormalizeAuthHeader(t *testing.T) { + cases := []struct { + in string + want string + wantErr bool + }{ + {"", "", false}, + {" ", "", false}, + {"x-api-key", "x-api-key", false}, + {"X-API-KEY", "x-api-key", false}, + {"authorization", "authorization", false}, + {"Bearer", "authorization", false}, + {" Authorization ", "authorization", false}, + {"cookie", "", true}, + } + for _, c := range cases { + got, err := NormalizeAuthHeader(c.in) + if (err != nil) != c.wantErr { + t.Errorf("NormalizeAuthHeader(%q) err=%v, wantErr=%v", c.in, err, c.wantErr) + continue + } + if got != c.want { + t.Errorf("NormalizeAuthHeader(%q) = %q, want %q", c.in, got, c.want) + } + } +} + +// TestTryCCEnv covers tryCCEnv: the model-override branch, a successful resolve +// from the ANTHROPIC_* environment, and the incomplete-environment miss. +func TestTryCCEnv(t *testing.T) { + t.Run("model override wins over env model", func(t *testing.T) { + t.Setenv(envCCBaseURL, "https://cc.example") + t.Setenv(envCCToken, "tok") + t.Setenv(envCCModel, "env-model") + + ep, ok, err := tryCCEnv("override-model") + if err != nil || !ok { + t.Fatalf("tryCCEnv: ok=%v err=%v", ok, err) + } + if ep.Model != "override-model" { + t.Errorf("model = %q, want override-model", ep.Model) + } + if ep.Protocol != ProtocolAnthropic || ep.AuthHeader != "authorization" { + t.Errorf("unexpected protocol/auth: %q %q", ep.Protocol, ep.AuthHeader) + } + }) + + t.Run("incomplete environment is a miss", func(t *testing.T) { + t.Setenv(envCCBaseURL, "https://cc.example") + t.Setenv(envCCToken, "") + t.Setenv(envCCModel, "m") + + _, ok, err := tryCCEnv("") + if err != nil || ok { + t.Fatalf("tryCCEnv should miss on empty token: ok=%v err=%v", ok, err) + } + }) +} diff --git a/internal/llm/resolver_shellrc_test.go b/internal/llm/resolver_shellrc_test.go new file mode 100644 index 0000000..e2a084d --- /dev/null +++ b/internal/llm/resolver_shellrc_test.go @@ -0,0 +1,63 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llm + +import ( + "os" + "path/filepath" + "testing" +) + +func TestShellRCFiles(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + if got := shellRCFiles(); len(got) != 0 { + t.Errorf("shellRCFiles() with no rc files = %v, want empty", got) + } + + zshrc := filepath.Join(home, ".zshrc") + if err := os.WriteFile(zshrc, []byte("# empty\n"), 0o644); err != nil { + t.Fatalf("write .zshrc: %v", err) + } + got := shellRCFiles() + if len(got) != 1 || got[0] != zshrc { + t.Errorf("shellRCFiles() = %v, want [%s]", got, zshrc) + } +} + +func TestTryShellRC(t *testing.T) { + home := t.TempDir() + t.Setenv("HOME", home) + + // No rc files: not found, no error. + if _, ok, err := tryShellRC(""); ok || err != nil { + t.Fatalf("tryShellRC() with no files = ok:%v err:%v, want false,nil", ok, err) + } + + // A complete rc file yields a resolved endpoint. + rc := "export ANTHROPIC_BASE_URL=\"https://example.test\"\n" + + "export ANTHROPIC_AUTH_TOKEN='tok-123'\n" + + "export ANTHROPIC_MODEL=claude-x\n" + if err := os.WriteFile(filepath.Join(home, ".zshrc"), []byte(rc), 0o644); err != nil { + t.Fatalf("write .zshrc: %v", err) + } + + ep, ok, err := tryShellRC("") + if err != nil || !ok { + t.Fatalf("tryShellRC() = ok:%v err:%v, want true,nil", ok, err) + } + if ep.Token != "tok-123" || ep.Model != "claude-x" { + t.Errorf("resolved endpoint = %+v, want token tok-123 model claude-x", ep) + } + + // modelOverride takes precedence. + ep, ok, err = tryShellRC("override-model") + if err != nil || !ok { + t.Fatalf("tryShellRC(override) = ok:%v err:%v", ok, err) + } + if ep.Model != "override-model" { + t.Errorf("model override not applied: %q", ep.Model) + } +} diff --git a/internal/llmloop/compression_test.go b/internal/llmloop/compression_test.go index 6d4a0b3..b5c65de 100644 --- a/internal/llmloop/compression_test.go +++ b/internal/llmloop/compression_test.go @@ -139,6 +139,16 @@ func TestStripMarkdownFences(t *testing.T) { input: "```json\n```", want: "", }, + { + name: "single-line json fence without newline", + input: "```json", + want: "", + }, + { + name: "bare fence without newline", + input: "```", + want: "", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { diff --git a/internal/llmloop/loop_execute_more_test.go b/internal/llmloop/loop_execute_more_test.go new file mode 100644 index 0000000..a01624b --- /dev/null +++ b/internal/llmloop/loop_execute_more_test.go @@ -0,0 +1,164 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llmloop + +import ( + "context" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/model" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" +) + +// TestExecuteToolCall_TaskDone covers every branch of the task_done handling: +// argument parse error, missing state (implicit completion), non-string state, +// explicit DONE / FAILED, and an unrecognized state value. +func TestExecuteToolCall_TaskDone(t *testing.T) { + newRunner := func() *Runner { + reg := tool.NewRegistry() + reg.Freeze() + return NewRunner(Deps{Tools: reg, CommentCollector: tool.NewCommentCollector()}) + } + + call := func(args string) tool.TaskCheckpoint { + return newRunner().executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{Name: tool.TaskDone.Name(), Arguments: args}, + }, nil) + } + + t.Run("parse error", func(t *testing.T) { + cp := call(`{bad`) + if !strings.Contains(cp.Data, "Error parsing tool arguments") { + t.Errorf("cp.Data = %q, want parse-error message", cp.Data) + } + }) + + t.Run("missing state completes", func(t *testing.T) { + cp := call(`{}`) + if !cp.Completed || cp.Failed { + t.Errorf("cp = %+v, want Completed", cp) + } + }) + + t.Run("non-string state", func(t *testing.T) { + cp := call(`{"state":123}`) + if !strings.Contains(cp.Data, "must be DONE or FAILED") { + t.Errorf("cp.Data = %q, want non-string state message", cp.Data) + } + }) + + t.Run("DONE completes", func(t *testing.T) { + cp := call(`{"state":"DONE"}`) + if !cp.Completed || cp.Failed { + t.Errorf("cp = %+v, want Completed", cp) + } + }) + + t.Run("FAILED fails", func(t *testing.T) { + cp := call(`{"state":"FAILED"}`) + if !cp.Failed { + t.Errorf("cp = %+v, want Failed", cp) + } + }) + + t.Run("invalid state", func(t *testing.T) { + cp := call(`{"state":"MAYBE"}`) + if !strings.Contains(cp.Data, "invalid task_done state") { + t.Errorf("cp.Data = %q, want invalid-state message", cp.Data) + } + }) +} + +// TestExecuteToolCall_CodeCommentAsyncPool covers the async dispatch path where a +// CommentWorkerPool is present: the call returns immediately with a success +// checkpoint, records "(async)" on the task record, and the comment lands in the +// collector once the pool drains. +func TestExecuteToolCall_CodeCommentAsyncPool(t *testing.T) { + collector := tool.NewCommentCollector() + pool := NewCommentWorkerPool(2) + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + r := NewRunner(Deps{ + Tools: reg, + CommentCollector: collector, + CommentWorkerPool: pool, + }) + + rec := &session.TaskRecord{} + cp := r.executeToolCall(context.Background(), "async.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"comments":[{"content":"issue","existing_code":"foo"}]}`, + }, + }, rec) + + if cp.Data != tool.CommentSucceed { + t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data) + } + if len(rec.ToolResults) != 1 || rec.ToolResults[0].Result != "(async)" { + t.Errorf("recorded results = %+v, want one (async) entry", rec.ToolResults) + } + + // Drain the pool and confirm the comment was collected with the injected path. + comments := r.CollectPendingComments() + if len(comments) != 1 { + t.Fatalf("collected %d comments, want 1", len(comments)) + } + if comments[0].Path != "async.go" { + t.Errorf("comment path = %q, want async.go", comments[0].Path) + } +} + +// TestExecuteToolCall_CodeCommentDiffResolved covers the synchronous code_comment +// path where DiffLookup returns a diff and ResolveComment resolves the line +// numbers from file content (so the re-location LLM branch is skipped), with a +// non-nil task record so AddToolResult runs. +func TestExecuteToolCall_CodeCommentDiffResolved(t *testing.T) { + collector := tool.NewCommentCollector() + reg := tool.NewRegistry() + reg.Register(&tool.CodeCommentProvider{Collector: collector}) + reg.Freeze() + + diffLookup := func(path string) *model.Diff { + return &model.Diff{ + NewPath: path, + NewFileContent: "line one\nfoo bar\nline three\n", + } + } + + r := NewRunner(Deps{ + Tools: reg, + CommentCollector: collector, + DiffLookup: diffLookup, + }) + + rec := &session.TaskRecord{} + cp := r.executeToolCall(context.Background(), "resolved.go", llm.ToolCall{ + Function: llm.FunctionCall{ + Name: tool.CodeComment.Name(), + Arguments: `{"comments":[{"content":"issue","existing_code":"foo bar"}]}`, + }, + }, rec) + + if cp.Data != tool.CommentSucceed { + t.Fatalf("cp.Data = %q, want CommentSucceed", cp.Data) + } + if len(rec.ToolResults) != 1 || rec.ToolResults[0].Result != tool.CommentSucceed { + t.Errorf("recorded results = %+v, want one success entry", rec.ToolResults) + } + + comments := collector.Comments() + if len(comments) != 1 { + t.Fatalf("collected %d comments, want 1", len(comments)) + } + // ResolveComment should have located "foo bar" on line 2 of NewFileContent. + if comments[0].StartLine != 2 { + t.Errorf("comment StartLine = %d, want 2 (resolved from file content)", comments[0].StartLine) + } +} diff --git a/internal/llmloop/loop_execute_test.go b/internal/llmloop/loop_execute_test.go new file mode 100644 index 0000000..ab83097 --- /dev/null +++ b/internal/llmloop/loop_execute_test.go @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package llmloop + +import ( + "context" + "errors" + "strings" + "testing" + + "github.com/alibaba/open-code-review/internal/llm" + "github.com/alibaba/open-code-review/internal/model" + "github.com/alibaba/open-code-review/internal/session" + "github.com/alibaba/open-code-review/internal/tool" +) + +// erroringProvider is a dynamic tool provider whose Execute always fails, used +// to drive executeToolCall's dynamic-tool error branch. +type erroringProvider struct { + tool tool.Tool +} + +func (p *erroringProvider) Tool() tool.Tool { return p.tool } +func (p *erroringProvider) Execute(_ context.Context, _ map[string]any) (string, error) { + return "", errors.New("boom") +} + +// TestExecuteToolCall_DynamicNotRegistered covers the path where the LLM calls +// a name that is neither a built-in tool nor present in the registry. +func TestExecuteToolCall_DynamicNotRegistered(t *testing.T) { + reg := tool.NewRegistry() + reg.Freeze() + r := NewRunner(Deps{Tools: reg, CommentCollector: tool.NewCommentCollector()}) + + cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{Name: "totally_unknown", Arguments: `{}`}, + }, nil) + + if cp.Data != tool.NotAvailableMsg { + t.Errorf("cp.Data = %q, want NotAvailableMsg", cp.Data) + } +} + +// TestExecuteToolCall_DynamicExecuteError covers the dynamic-tool branch where +// the provider's Execute returns an error. +func TestExecuteToolCall_DynamicExecuteError(t *testing.T) { + reg := tool.NewRegistry() + reg.Register(&erroringProvider{tool: tool.Dynamic("dyn_fail")}) + reg.Freeze() + r := NewRunner(Deps{Tools: reg, CommentCollector: tool.NewCommentCollector()}) + + cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{Name: "dyn_fail", Arguments: `{}`}, + }, nil) + + if !strings.Contains(cp.Data, "Error executing tool dyn_fail") { + t.Errorf("cp.Data = %q, want execute-error message", cp.Data) + } +} + +// TestExecuteToolCall_DynamicSuccessRecordsResult covers the dynamic-tool +// success path with a non-nil TaskRecord so AddToolResult runs. +func TestExecuteToolCall_DynamicSuccessRecordsResult(t *testing.T) { + reg := tool.NewRegistry() + dyn := &argsCapturingProvider{tool: tool.Dynamic("dyn_ok")} + reg.Register(dyn) + reg.Freeze() + r := NewRunner(Deps{Tools: reg, CommentCollector: tool.NewCommentCollector()}) + + rec := &session.TaskRecord{} + cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{Name: "dyn_ok", Arguments: `{"k":"v"}`}, + }, rec) + + if cp.Data != "ok" { + t.Errorf("cp.Data = %q, want ok", cp.Data) + } + if len(rec.ToolResults) != 1 { + t.Fatalf("expected 1 recorded tool result, got %d", len(rec.ToolResults)) + } + if rec.ToolResults[0].ToolName != "dyn_ok" || rec.ToolResults[0].Result != "ok" { + t.Errorf("recorded result = %+v, want dyn_ok/ok", rec.ToolResults[0]) + } +} + +// TestExecuteToolCall_KnownToolNotRegistered covers the lookupTool-nil branch: +// a built-in tool the model may call but which is absent from the registry. +func TestExecuteToolCall_KnownToolNotRegistered(t *testing.T) { + reg := tool.NewRegistry() + reg.Freeze() + r := NewRunner(Deps{Tools: reg, CommentCollector: tool.NewCommentCollector()}) + + cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{Name: tool.FileRead.Name(), Arguments: `{"path":"x"}`}, + }, nil) + + if cp.Data != tool.NotAvailableMsg { + t.Errorf("cp.Data = %q, want NotAvailableMsg", cp.Data) + } +} + +// TestCollectPendingComments_AwaitsPool covers the worker-pool drain branch of +// CollectPendingComments. +func TestCollectPendingComments_AwaitsPool(t *testing.T) { + collector := tool.NewCommentCollector() + pool := NewCommentWorkerPool(2) + r := NewRunner(Deps{ + Tools: tool.NewRegistry(), + CommentCollector: collector, + CommentWorkerPool: pool, + }) + + done := make(chan struct{}) + pool.Submit(func() ([]model.LlmComment, error) { + close(done) + return nil, nil + }) + + got := r.CollectPendingComments() + select { + case <-done: + default: + t.Fatal("CollectPendingComments returned before pool work drained") + } + if len(got) != 0 { + t.Errorf("comments = %d, want 0", len(got)) + } +} + +// TestExecuteToolCall_DynamicParseError covers the dynamic-tool branch where +// the arguments string fails to parse. +func TestExecuteToolCall_DynamicParseError(t *testing.T) { + reg := tool.NewRegistry() + reg.Register(&argsCapturingProvider{tool: tool.Dynamic("dyn_ok")}) + reg.Freeze() + r := NewRunner(Deps{Tools: reg, CommentCollector: tool.NewCommentCollector()}) + + cp := r.executeToolCall(context.Background(), "file.go", llm.ToolCall{ + Function: llm.FunctionCall{Name: "dyn_ok", Arguments: `{bad`}, + }, nil) + + if !strings.Contains(cp.Data, "Error parsing tool arguments for dyn_ok") { + t.Errorf("cp.Data = %q, want parse-error message", cp.Data) + } +} diff --git a/internal/mcp/client_test.go b/internal/mcp/client_test.go index 7677f1f..a5a9327 100644 --- a/internal/mcp/client_test.go +++ b/internal/mcp/client_test.go @@ -110,6 +110,28 @@ func TestNewRemoteClient_HeaderExpandsToEmpty(t *testing.T) { } } +// TestNewRemoteClient_ConnectFailure exercises the success path of header +// expansion (non-empty value) plus HTTP client / transport construction, then +// the connect-failure return when the endpoint refuses the connection. +func TestNewRemoteClient_ConnectFailure(t *testing.T) { + t.Setenv("OCR_TEST_TOKEN", "secret-value") + + _, err := NewRemoteClient( + context.Background(), + "test-srv", + // Port 1 is reserved and refuses connections immediately. + "http://127.0.0.1:1/mcp", + map[string]string{"Authorization": "Bearer $OCR_TEST_TOKEN"}, + "v0.0.1-test", + ) + if err == nil { + t.Fatal("expected error when the endpoint refuses the connection, got nil") + } + if !strings.Contains(err.Error(), "connect to remote MCP server") { + t.Errorf("error = %q, want mention of 'connect to remote MCP server'", err.Error()) + } +} + func TestNewRemoteClient_HeaderExpandsUnsetVar(t *testing.T) { t.Setenv("OCR_TEST_UNSET_MARKER", "") // Ensure the variable is truly unset (Setenv("", "") sets it to empty; diff --git a/internal/pathutil/path_test.go b/internal/pathutil/path_test.go index d5fe6be..1c3210c 100644 --- a/internal/pathutil/path_test.go +++ b/internal/pathutil/path_test.go @@ -129,6 +129,9 @@ func TestWithinBase_AdditionalCases(t *testing.T) { {name: "dotdot only", base: "/a/b", target: "/a", want: false}, {name: "root base with child", base: "/", target: "/anything", want: true}, {name: "empty relative after clean", base: "/a/b", target: "/a/b/./c", want: true}, + // filepath.Rel cannot relate a relative base to an absolute target, + // so WithinBase must fall through its error branch and report false. + {name: "rel error on mixed abs/rel", base: "relative", target: "/absolute", want: false}, } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { diff --git a/internal/scan/getters_more_test.go b/internal/scan/getters_more_test.go new file mode 100644 index 0000000..462b5cd --- /dev/null +++ b/internal/scan/getters_more_test.go @@ -0,0 +1,85 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package scan + +import ( + "testing" + + "github.com/alibaba/open-code-review/internal/model" + "github.com/alibaba/open-code-review/internal/session" +) + +// TestScanAgent_SessionID_Persistent covers the non-empty return of SessionID: +// a session with a JSONL writer reports its persisted ID. +func TestScanAgent_SessionID_Persistent(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + sess := session.New(t.TempDir(), "", "model-x", session.SessionOptions{ + ReviewMode: session.ReviewModeFullScan, + }) + if !sess.HasPersistence() { + t.Skip("session persistence unavailable in this environment") + } + a := &Agent{session: sess} + if got := a.SessionID(); got != sess.SessionID { + t.Errorf("SessionID() = %q, want %q", got, sess.SessionID) + } +} + +// TestScanAgent_ResumeInfo covers both the nil and non-nil branches, and that +// the returned pointer is a defensive copy. +func TestScanAgent_ResumeInfo(t *testing.T) { + a := &Agent{} + if got := a.ResumeInfo(); got != nil { + t.Errorf("ResumeInfo() on fresh agent = %v, want nil", got) + } + a.resumeInfo = &session.ResumeInfo{ResumedFrom: "sess-1", ReusedFiles: 3} + got := a.ResumeInfo() + if got == nil || got.ResumedFrom != "sess-1" || got.ReusedFiles != 3 { + t.Fatalf("ResumeInfo() = %+v, want copy of resumeInfo", got) + } + if got == a.resumeInfo { + t.Error("ResumeInfo() must return a copy, not the internal pointer") + } +} + +// TestScanAgent_Fingerprints covers initScanFingerprints (map build) and +// scanItemFingerprint's cached-hit branch versus the fallback compute. +func TestScanAgent_Fingerprints(t *testing.T) { + a := &Agent{} + items := []model.ScanItem{ + {Path: "a.go", Content: "package a\n"}, + {Path: "b.go", Content: "package b\n"}, + } + a.initScanFingerprints(items) + if len(a.scanFingerprints) != 2 { + t.Fatalf("scanFingerprints size = %d, want 2", len(a.scanFingerprints)) + } + // Cached hit: matches the map value for a known path. + if got, want := a.scanItemFingerprint(items[0]), a.scanFingerprints["a.go"]; got != want { + t.Errorf("cached fingerprint = %q, want %q", got, want) + } + // Fallback compute: an item not in the map still gets a stable fingerprint. + other := model.ScanItem{Path: "c.go", Content: "package c\n"} + if got := a.scanItemFingerprint(other); got == "" || got != scanItemFingerprint(other) { + t.Errorf("fallback fingerprint = %q, want computed value", got) + } + + // initScanFingerprints with no items is a no-op (leaves map nil). + empty := &Agent{} + empty.initScanFingerprints(nil) + if empty.scanFingerprints != nil { + t.Errorf("scanFingerprints = %v, want nil for empty items", empty.scanFingerprints) + } +} + +// TestResumedFromSession covers the nil and non-nil resume-state branches. +func TestResumedFromSession(t *testing.T) { + if got := resumedFromSession(nil); got != "" { + t.Errorf("resumedFromSession(nil) = %q, want empty", got) + } + got := resumedFromSession(&session.ResumeState{SessionID: "prev-123"}) + if got != "prev-123" { + t.Errorf("resumedFromSession = %q, want prev-123", got) + } +} diff --git a/internal/scan/getters_test.go b/internal/scan/getters_test.go new file mode 100644 index 0000000..d4aeec8 --- /dev/null +++ b/internal/scan/getters_test.go @@ -0,0 +1,29 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package scan + +import "testing" + +// TestScanGettersOnEmptyAgent exercises the small ResultProvider getters that +// scan implements as constants or nil-safe guards, so review and scan can share +// the output pipeline. +func TestScanGettersOnEmptyAgent(t *testing.T) { + a := &Agent{} + + if got := a.SessionID(); got != "" { + t.Errorf("SessionID() on session-less agent = %q, want empty", got) + } + if got := a.RunManifest(); got != nil { + t.Errorf("RunManifest() = %v, want nil", got) + } + if a.BudgetExceeded() { + t.Errorf("BudgetExceeded() = true, want false") + } + + // Nil receiver must not panic for SessionID (guarded). + var nilAgent *Agent + if got := nilAgent.SessionID(); got != "" { + t.Errorf("nil-receiver SessionID() = %q, want empty", got) + } +} diff --git a/internal/scan/provider_more_test.go b/internal/scan/provider_more_test.go new file mode 100644 index 0000000..7d9ee36 --- /dev/null +++ b/internal/scan/provider_more_test.go @@ -0,0 +1,146 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package scan + +import ( + "context" + "os" + "path/filepath" + "sort" + "testing" + + "github.com/alibaba/open-code-review/internal/model" +) + +// TestProvider_Enumerate_OversizeSkip covers the size-cap branch: a file larger +// than maxFileSizeBytes is skipped with a warning while smaller files remain. +func TestProvider_Enumerate_OversizeSkip(t *testing.T) { + repo := initTestRepo(t) + writeFile(t, repo, "small.go", []byte("package s\n")) + writeFile(t, repo, "big.go", []byte("package big // "+string(make([]byte, 200))+"\n")) + gitCommit(t, repo, "init") + + got, err := NewProvider(repo, nil, nil, 32).Enumerate(context.Background()) + if err != nil { + t.Fatalf("Enumerate: %v", err) + } + paths := itemPaths(got) + if contains(paths, "big.go") { + t.Errorf("big.go should be skipped (exceeds size cap), got %v", paths) + } + if !contains(paths, "small.go") { + t.Errorf("small.go should be present, got %v", paths) + } +} + +// TestProvider_Enumerate_NonRegularSkip covers the !IsRegular branch: a symlink +// tracked by git is enumerated but skipped because it is not a regular file. +func TestProvider_Enumerate_NonRegularSkip(t *testing.T) { + repo := initTestRepo(t) + writeFile(t, repo, "real.go", []byte("package r\n")) + if err := os.Symlink("real.go", filepath.Join(repo, "link.go")); err != nil { + t.Skipf("symlink unsupported: %v", err) + } + gitCommit(t, repo, "init") + + got, err := NewProvider(repo, nil, nil, 0).Enumerate(context.Background()) + if err != nil { + t.Fatalf("Enumerate: %v", err) + } + paths := itemPaths(got) + if contains(paths, "link.go") { + t.Errorf("symlink link.go must be skipped (non-regular), got %v", paths) + } + if !contains(paths, "real.go") { + t.Errorf("real.go should be present, got %v", paths) + } +} + +// TestProvider_Enumerate_SniffError covers the binary-sniff error branch: a file +// that cannot be opened for sniffing is skipped with a warning. +func TestProvider_Enumerate_SniffError(t *testing.T) { + if os.Geteuid() == 0 { + t.Skip("root bypasses file permission checks") + } + repo := initTestRepo(t) + writeFile(t, repo, "ok.go", []byte("package ok\n")) + writeFile(t, repo, "locked.go", []byte("package locked\n")) + gitCommit(t, repo, "init") + // Make locked.go unreadable so isBinaryFile's os.Open fails. + if err := os.Chmod(filepath.Join(repo, "locked.go"), 0o000); err != nil { + t.Fatalf("chmod: %v", err) + } + t.Cleanup(func() { _ = os.Chmod(filepath.Join(repo, "locked.go"), 0o644) }) + + got, err := NewProvider(repo, nil, nil, 0).Enumerate(context.Background()) + if err != nil { + t.Fatalf("Enumerate: %v", err) + } + paths := itemPaths(got) + if contains(paths, "locked.go") { + t.Errorf("unreadable locked.go must be skipped, got %v", paths) + } + if !contains(paths, "ok.go") { + t.Errorf("ok.go should be present, got %v", paths) + } +} + +// TestIsBinaryFile covers detection plus the open-error branch. +func TestIsBinaryFile(t *testing.T) { + dir := t.TempDir() + text := filepath.Join(dir, "text.txt") + if err := os.WriteFile(text, []byte("hello world\n"), 0o644); err != nil { + t.Fatalf("write text: %v", err) + } + bin := filepath.Join(dir, "bin.dat") + if err := os.WriteFile(bin, []byte{'a', 0x00, 'b'}, 0o644); err != nil { + t.Fatalf("write bin: %v", err) + } + empty := filepath.Join(dir, "empty.txt") + if err := os.WriteFile(empty, nil, 0o644); err != nil { + t.Fatalf("write empty: %v", err) + } + + t.Run("text is not binary", func(t *testing.T) { + b, err := isBinaryFile(text) + if err != nil || b { + t.Errorf("isBinaryFile(text) = %v, %v; want false, nil", b, err) + } + }) + t.Run("NUL byte is binary", func(t *testing.T) { + b, err := isBinaryFile(bin) + if err != nil || !b { + t.Errorf("isBinaryFile(bin) = %v, %v; want true, nil", b, err) + } + }) + t.Run("empty is not binary", func(t *testing.T) { + b, err := isBinaryFile(empty) + if err != nil || b { + t.Errorf("isBinaryFile(empty) = %v, %v; want false, nil", b, err) + } + }) + t.Run("missing file errors", func(t *testing.T) { + if _, err := isBinaryFile(filepath.Join(dir, "nope")); err == nil { + t.Error("expected error for missing file") + } + }) +} + +func itemPaths(items []model.ScanItem) []string { + paths := make([]string, 0, len(items)) + for _, it := range items { + paths = append(paths, it.Path) + } + sort.Strings(paths) + return paths +} + +func contains(list []string, want string) bool { + for _, s := range list { + if s == want { + return true + } + } + return false +} diff --git a/internal/session/final_manifest_test.go b/internal/session/final_manifest_test.go new file mode 100644 index 0000000..8c2e113 --- /dev/null +++ b/internal/session/final_manifest_test.go @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import "testing" + +func TestFinalManifest(t *testing.T) { + // Nil receiver must not panic. + var nilSH *SessionHistory + if got := nilSH.FinalManifest(); got != nil { + t.Errorf("nil-receiver FinalManifest() = %v, want nil", got) + } + + // A session with no frozen manifest (legacy/scan) returns nil. + sh := &SessionHistory{} + if got := sh.FinalManifest(); got != nil { + t.Errorf("FinalManifest() with no manifest = %v, want nil", got) + } + + // SetFinalManifest is a no-op on a nil receiver. + nilSH.SetFinalManifest(&RunManifest{RunID: "ignored"}) + + // After storing, FinalManifest returns a cloned copy carrying the data. + sh.SetFinalManifest(&RunManifest{RunID: "run-123", Operation: "review"}) + got := sh.FinalManifest() + if got == nil { + t.Fatal("FinalManifest() after set = nil, want value") + } + if got.RunID != "run-123" || got.Operation != "review" { + t.Errorf("FinalManifest() = %+v, want RunID=run-123 Operation=review", got) + } +} diff --git a/internal/session/list_error_test.go b/internal/session/list_error_test.go new file mode 100644 index 0000000..5ba6cac --- /dev/null +++ b/internal/session/list_error_test.go @@ -0,0 +1,56 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "os" + "path/filepath" + "testing" +) + +// TestListSessions_DirIsFile covers the ReadDir error branch (a non-NotExist +// error): when the computed sessions dir path is occupied by a regular file, +// os.ReadDir fails with ENOTDIR and ListSessions must surface it. +func TestListSessions_DirIsFile(t *testing.T) { + t.Setenv("HOME", t.TempDir()) + repoDir := t.TempDir() + + dir, err := SessionsDir(repoDir) + if err != nil { + t.Fatalf("SessionsDir: %v", err) + } + if err := os.MkdirAll(filepath.Dir(dir), 0o755); err != nil { + t.Fatalf("mkdir parent: %v", err) + } + // Occupy the sessions-dir path with a file so ReadDir cannot treat it as a dir. + if err := os.WriteFile(dir, []byte("x"), 0o644); err != nil { + t.Fatalf("write file at dir path: %v", err) + } + + if _, err := ListSessions(repoDir); err == nil { + t.Fatal("ListSessions should error when the sessions path is a file") + } +} + +// TestRecordToItem covers the non-item type (returns false) and the +// empty-FilePath fallback to NewPath for a recognized item record. +func TestRecordToItem(t *testing.T) { + if _, ok := recordToItem(summaryRecord{Type: "session_start"}); ok { + t.Error("session_start should not convert to an item") + } + + item, ok := recordToItem(summaryRecord{ + Type: "review_item_done", + NewPath: "renamed.go", + }) + if !ok { + t.Fatal("review_item_done should convert to an item") + } + if item.FilePath != "renamed.go" { + t.Errorf("FilePath = %q, want NewPath fallback %q", item.FilePath, "renamed.go") + } + if item.Type != "done" { + t.Errorf("Type = %q, want %q", item.Type, "done") + } +} diff --git a/internal/session/list_more_test.go b/internal/session/list_more_test.go new file mode 100644 index 0000000..6ad882b --- /dev/null +++ b/internal/session/list_more_test.go @@ -0,0 +1,92 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "runtime" + "testing" + "time" + + "github.com/alibaba/open-code-review/internal/model" +) + +// TestParseRecordTime covers the empty, valid-RFC3339, and garbage branches. +func TestParseRecordTime(t *testing.T) { + if got := parseRecordTime(""); !got.IsZero() { + t.Errorf("parseRecordTime(\"\") = %v, want zero", got) + } + want := time.Date(2026, 8, 5, 10, 30, 0, 0, time.UTC) + if got := parseRecordTime("2026-08-05T10:30:00Z"); !got.Equal(want) { + t.Errorf("parseRecordTime(RFC3339) = %v, want %v", got, want) + } + if got := parseRecordTime("not-a-timestamp"); !got.IsZero() { + t.Errorf("parseRecordTime(garbage) = %v, want zero", got) + } +} + +// TestSessionsDir_HomeUnset covers the os.UserHomeDir error branch by clearing +// the HOME environment variable. +func TestSessionsDir_HomeUnset(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("HOME is not the home-dir source on Windows") + } + t.Setenv("HOME", "") + if _, err := SessionsDir(t.TempDir()); err == nil { + t.Fatal("SessionsDir with HOME unset should error") + } +} + +// TestLoadSummary_HomeUnset covers the SessionFilePath error branch in +// LoadSummary (and, by extension, LoadDetail) when the home dir cannot resolve. +func TestLoadSummary_HomeUnset(t *testing.T) { + if runtime.GOOS == "windows" { + t.Skip("HOME is not the home-dir source on Windows") + } + t.Setenv("HOME", "") + if _, err := LoadSummary(t.TempDir(), "sess-1"); err == nil { + t.Fatal("LoadSummary with HOME unset should error") + } + if _, _, err := LoadDetail(t.TempDir(), "sess-1"); err == nil { + t.Fatal("LoadDetail with HOME unset should error") + } +} + +// TestManifest_NilReceiver covers the sh == nil branch of Manifest. +func TestManifest_NilReceiver(t *testing.T) { + var sh *SessionHistory + if got := sh.Manifest(); got != nil { + t.Errorf("(nil).Manifest() = %v, want nil", got) + } +} + +// TestRecordReviewItem_NilReceiver covers the sh == nil guard on all three +// checkpoint recorders (no panic, no-op). +func TestRecordReviewItem_NilReceiver(t *testing.T) { + var sh *SessionHistory + sh.RecordReviewItemDone("a.go", "", "", "fp", nil) + sh.RecordReviewItemReused("a.go", "", "", "fp", "src", nil) + sh.RecordReviewItemFailed("a.go", "", "", "fp", "boom") +} + +// TestRecordReviewItem_EmptyFilePathUsesNewPath covers the filePath == "" → +// filePath = newPath branch of each checkpoint recorder. Without persistence the +// recorders still create the in-memory FileSession keyed by newPath. +func TestRecordReviewItem_EmptyFilePathUsesNewPath(t *testing.T) { + sh := New(t.TempDir(), "main", "test-model", SessionOptions{}) + + sh.RecordReviewItemDone("", "old.go", "done.go", "fp1", nil) + if _, ok := sh.FileSessions["done.go"]; !ok { + t.Error("RecordReviewItemDone with empty filePath should key FileSession by newPath") + } + + sh.RecordReviewItemReused("", "old.go", "reused.go", "fp2", "src", []model.LlmComment{{Content: "x"}}) + if _, ok := sh.FileSessions["reused.go"]; !ok { + t.Error("RecordReviewItemReused with empty filePath should key FileSession by newPath") + } + + sh.RecordReviewItemFailed("", "old.go", "failed.go", "fp3", "boom") + if _, ok := sh.FileSessions["failed.go"]; !ok { + t.Error("RecordReviewItemFailed with empty filePath should key FileSession by newPath") + } +} diff --git a/internal/session/manifest_guards_test.go b/internal/session/manifest_guards_test.go new file mode 100644 index 0000000..ad964ca --- /dev/null +++ b/internal/session/manifest_guards_test.go @@ -0,0 +1,97 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "errors" + "testing" +) + +// TestManifestBuilderNilReceiver covers the `if b == nil` guard on every +// exported ManifestBuilder method: a nil builder must never panic, the mutating +// error-returning methods return errNilBuilder, and the boolean/void methods +// degrade quietly. +func TestManifestBuilderNilReceiver(t *testing.T) { + var b *ManifestBuilder + + // Void setters: must not panic on a nil receiver. + b.SetParentRunID("parent") + b.SetRepository(ManifestRepository{}) + b.SetInput(ManifestInput{Mode: InputModeWorkspace}) + b.SetExecution(ManifestExecution{}) + + // Error-returning methods: must report errNilBuilder. + errReturners := map[string]error{ + "SetRunFailure": b.SetRunFailure(RunFailureBudget, "r"), + "SetPendingFailureCause": b.SetPendingFailureCause(FailureBudget, "r"), + "RegisterSelected": b.RegisterSelected(CoverageItem{ItemID: "a"}), + "SealSelected": b.SealSelected(), + "MarkCompleted": b.MarkCompleted("a"), + "MarkReused": b.MarkReused("a"), + "MarkFailed": b.MarkFailed("a", FailureProvider, "r"), + "MarkWaived": b.MarkWaived("a", "r"), + } + for name, err := range errReturners { + if !errors.Is(err, errNilBuilder) { + t.Errorf("%s on nil builder: got %v, want errNilBuilder", name, err) + } + } + + if _, err := b.Finalize(0); !errors.Is(err, errNilBuilder) { + t.Errorf("Finalize on nil builder: got %v, want errNilBuilder", err) + } + + // Boolean predicates: must report false on a nil receiver. + if b.Sealed() { + t.Error("Sealed on nil builder should be false") + } + if b.Frozen() { + t.Error("Frozen on nil builder should be false") + } +} + +// TestManifestBuilderFrozenNoOp covers the frozen-branch no-ops of the void +// setters and the errFrozen paths of the mutating methods after Finalize has +// frozen the builder. +func TestManifestBuilderFrozenNoOp(t *testing.T) { + b := NewManifestBuilder("run-frozen", "review") + b.SetInput(ManifestInput{Mode: InputModeWorkspace}) + b.SetParentRunID("orig-parent") + if _, err := b.Finalize(0); err != nil { + t.Fatalf("Finalize: %v", err) + } + if !b.Frozen() { + t.Fatal("builder should be frozen after Finalize") + } + + // Void setters must silently no-op once frozen (no panic, no mutation). + b.SetParentRunID("changed") + b.SetRepository(ManifestRepository{}) + b.SetInput(ManifestInput{Mode: InputModeCommit}) + b.SetExecution(ManifestExecution{}) + + // Mutating methods must report errFrozen. + frozenReturners := map[string]error{ + "SetRunFailure": b.SetRunFailure(RunFailureBudget, "r"), + "SetPendingFailureCause": b.SetPendingFailureCause(FailureBudget, "r"), + "RegisterSelected": b.RegisterSelected(CoverageItem{ItemID: "z"}), + "SealSelected": b.SealSelected(), + "MarkCompleted": b.MarkCompleted("a"), + } + for name, err := range frozenReturners { + if !errors.Is(err, errFrozen) { + t.Errorf("%s after freeze: got %v, want errFrozen", name, err) + } + } + + // The frozen manifest must still report the original parent, proving the + // post-freeze SetParentRunID was a no-op. + m, err := b.Finalize(0) + if err != nil { + t.Fatalf("idempotent Finalize: %v", err) + } + if m.ParentRunID != "orig-parent" { + t.Errorf("parent_run_id = %q, want unchanged %q", m.ParentRunID, "orig-parent") + } +} diff --git a/internal/session/validate_scan_options_test.go b/internal/session/validate_scan_options_test.go new file mode 100644 index 0000000..7a2d434 --- /dev/null +++ b/internal/session/validate_scan_options_test.go @@ -0,0 +1,72 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package session + +import ( + "strings" + "testing" +) + +// TestValidateScanOptions covers every branch of ResumeState.ValidateScanOptions: +// the nil receiver, the missing/mismatched review mode errors, the scan-path +// scope mismatch, and the two success paths (with and without a recorded scope). +func TestValidateScanOptions(t *testing.T) { + t.Run("nil state is a no-op", func(t *testing.T) { + var s *ResumeState + if err := s.ValidateScanOptions([]string{"a"}); err != nil { + t.Errorf("nil state = %v, want nil", err) + } + }) + + t.Run("missing review mode metadata errors", func(t *testing.T) { + s := &ResumeState{SessionID: "sess-1", ReviewMode: ""} + err := s.ValidateScanOptions(nil) + if err == nil || !strings.Contains(err.Error(), "missing review mode metadata") { + t.Errorf("err = %v, want missing review mode metadata", err) + } + }) + + t.Run("non-scan mode errors", func(t *testing.T) { + s := &ResumeState{SessionID: "sess-1", ReviewMode: ReviewModeRange} + err := s.ValidateScanOptions(nil) + if err == nil || !strings.Contains(err.Error(), "does not match current mode") { + t.Errorf("err = %v, want mode mismatch", err) + } + }) + + t.Run("scope mismatch errors", func(t *testing.T) { + s := &ResumeState{ + ReviewMode: ReviewModeFullScan, + HasScanPathScope: true, + ScanPaths: []string{"src"}, + } + err := s.ValidateScanOptions([]string{"docs"}) + if err == nil || !strings.Contains(err.Error(), "scan path scope") { + t.Errorf("err = %v, want scan path scope mismatch", err) + } + }) + + t.Run("matching scope succeeds after normalization", func(t *testing.T) { + s := &ResumeState{ + ReviewMode: ReviewModeFullScan, + HasScanPathScope: true, + ScanPaths: []string{"src"}, + } + // "./src/" normalizes to "src", so the scopes match. + if err := s.ValidateScanOptions([]string{"./src/"}); err != nil { + t.Errorf("matching scope = %v, want nil", err) + } + }) + + t.Run("no recorded scope skips the scope check", func(t *testing.T) { + s := &ResumeState{ + ReviewMode: ReviewModeFullScan, + HasScanPathScope: false, + ScanPaths: nil, + } + if err := s.ValidateScanOptions([]string{"anything"}); err != nil { + t.Errorf("no scope = %v, want nil", err) + } + }) +} diff --git a/internal/telemetry/traceid_test.go b/internal/telemetry/traceid_test.go new file mode 100644 index 0000000..bf20510 --- /dev/null +++ b/internal/telemetry/traceid_test.go @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package telemetry + +import ( + "context" + "testing" +) + +// TestTraceIDFromContext_Empty covers the invalid-span branch: a bare context +// carries no span, so an empty string is returned. +func TestTraceIDFromContext_Empty(t *testing.T) { + if got := TraceIDFromContext(context.Background()); got != "" { + t.Errorf("TraceIDFromContext(bare ctx) = %q, want empty", got) + } +} + +// TestTraceIDFromContext_Valid covers the valid-span branch: a context carrying +// an active span reports its hex-encoded trace ID. +func TestTraceIDFromContext_Valid(t *testing.T) { + setupEnabledTelemetry(t) + ctx, span := StartSpan(context.Background(), "test.traceid") + defer span.End() + + got := TraceIDFromContext(ctx) + if got == "" { + t.Fatal("TraceIDFromContext with active span returned empty") + } + if want := span.SpanContext().TraceID().String(); got != want { + t.Errorf("TraceIDFromContext = %q, want %q", got, want) + } +} diff --git a/internal/viewer/server_startserver_test.go b/internal/viewer/server_startserver_test.go new file mode 100644 index 0000000..1d94ef7 --- /dev/null +++ b/internal/viewer/server_startserver_test.go @@ -0,0 +1,74 @@ +// SPDX-License-Identifier: Apache-2.0 +// Copyright 2026 alibaba/open-code-review Contributors + +package viewer + +import ( + "net" + "net/http/httptest" + "strings" + "testing" +) + +// TestStartServer_SessionsRootError forces os.UserHomeDir to fail by clearing +// HOME so StartServer returns before binding a socket. +func TestStartServer_SessionsRootError(t *testing.T) { + t.Setenv("HOME", "") + // On unix os.UserHomeDir errors when HOME is empty. + if _, err := SessionsRoot(); err == nil { + t.Skip("home dir resolvable despite empty HOME; platform-specific") + } + if err := StartServer("127.0.0.1:0"); err == nil { + t.Fatal("expected StartServer to fail when sessions root cannot resolve") + } +} + +// TestStartServer_AddrInUse runs the full setup path (routes, host guard, +// security headers, server construction) and then fails fast on ListenAndServe +// because the port is already bound — no goroutine leak. +func TestStartServer_AddrInUse(t *testing.T) { + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("reserve port: %v", err) + } + defer ln.Close() + + err = StartServer(ln.Addr().String()) + if err == nil { + t.Fatal("expected StartServer to fail binding an in-use address") + } +} + +// TestParseTemplate_SessionWithComments renders session.html with review +// comments spanning every severity and category so the funcMap closures +// (severityCounts, severityClass, categoryClass, groupCommentsByFile) execute. +func TestParseTemplate_SessionWithComments(t *testing.T) { + tmpl, err := parseTemplate("session.html") + if err != nil { + t.Fatalf("parseTemplate: %v", err) + } + + comments := []*ReviewComment{ + {FilePath: "a.go", Content: "c1", Category: "bug", Severity: "critical", StartLine: 1, EndLine: 2}, + {FilePath: "a.go", Content: "c2", Category: "security", Severity: "high"}, + {FilePath: "b.go", Content: "c3", Category: "performance", Severity: "medium"}, + {FilePath: "b.go", Content: "c4", Category: "docs", Severity: "low"}, + } + vs := &ViewSession{ + Summary: SessionSummary{SessionID: "s", CWD: "/p"}, + Comments: comments, + Files: []*FileGroup{ + {FilePath: "a.go", Tasks: map[TaskType][]*TaskCard{ + MainTask: {{RequestNo: 1, ResponseContent: "ok", DurationMs: 1500, PromptTokens: 1200, CompletionTokens: 2_000_000}}, + }}, + }, + } + + rr := httptest.NewRecorder() + if err := tmpl.Execute(rr, sessionPageData{EncodedRepo: "r", RepoName: "R", Session: vs}); err != nil { + t.Fatalf("execute session.html with comments: %v", err) + } + if !strings.Contains(rr.Body.String(), "Review Comments") { + t.Error("rendered page missing Review Comments section") + } +} diff --git a/internal/viewer/store_load_test.go b/internal/viewer/store_load_test.go index f5ab267..bc0dc04 100644 --- a/internal/viewer/store_load_test.go +++ b/internal/viewer/store_load_test.go @@ -671,3 +671,66 @@ func TestLoadSession_MultipleTaskTypes(t *testing.T) { t.Errorf("memory_compression_task cards = %d", len(fg.Tasks[MemoryCompressionTask])) } } + +// TestLoadSession_ReviewComments covers the review_item_done / review_item_reused +// comment-parsing block: every ReviewComment field, the per-comment path +// override, and a reused item carrying comments. +func TestLoadSession_ReviewComments(t *testing.T) { + root := t.TempDir() + repoDir := filepath.Join(root, "repo") + if err := os.MkdirAll(repoDir, 0755); err != nil { + t.Fatal(err) + } + + writeJSONL(t, filepath.Join(repoDir, "comments.jsonl"), + `{"type":"session_start","timestamp":"2025-01-01T00:00:00Z","cwd":"/x","model":"m"}`, + `{"type":"review_item_done","filePath":"main.go","comments":[{"path":"override.go","content":"use a constant","suggestion_code":"const N = 3","existing_code":"3","start_line":10,"end_line":12,"category":"style","severity":"minor"}]}`, + `{"type":"review_item_reused","filePath":"util.go","comments":[{"content":"reused finding"}]}`, + `{"type":"session_end","duration_seconds":5,"files_reviewed":["main.go"]}`, + ) + + vs, err := LoadSession(root, "repo", "comments") + if err != nil { + t.Fatal(err) + } + + if len(vs.Comments) != 2 { + t.Fatalf("Comments = %d, want 2", len(vs.Comments)) + } + if vs.Summary.CommentCount != 2 { + t.Errorf("CommentCount = %d, want 2", vs.Summary.CommentCount) + } + + c := vs.Comments[0] + // path override replaces the record-level filePath. + if c.FilePath != "override.go" { + t.Errorf("FilePath = %q, want override.go", c.FilePath) + } + if c.Content != "use a constant" { + t.Errorf("Content = %q", c.Content) + } + if c.SuggestionCode != "const N = 3" { + t.Errorf("SuggestionCode = %q", c.SuggestionCode) + } + if c.ExistingCode != "3" { + t.Errorf("ExistingCode = %q", c.ExistingCode) + } + if c.StartLine != 10 || c.EndLine != 12 { + t.Errorf("lines = %d-%d, want 10-12", c.StartLine, c.EndLine) + } + if c.Category != "style" { + t.Errorf("Category = %q", c.Category) + } + if c.Severity != "minor" { + t.Errorf("Severity = %q", c.Severity) + } + + // A reused comment with no path falls back to the record-level filePath. + reused := vs.Comments[1] + if reused.FilePath != "util.go" { + t.Errorf("reused FilePath = %q, want util.go", reused.FilePath) + } + if reused.Content != "reused finding" { + t.Errorf("reused Content = %q", reused.Content) + } +}