diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 64d19190..19c46a4e 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -90,27 +90,57 @@ func runConfigSet(key, value string) error { displayValue = maskKey(value) } fmt.Printf("Set %s = %s\n", key, displayValue) + + // Warn when llm.* settings are shadowed by an active provider. + if cfg.Provider != "" && strings.HasPrefix(key, "llm.") { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: %q will have no effect while provider %q is active. The value is saved but will not be used until the provider is unset.\n", key, cfg.Provider) + } return nil } func runConfigUnset(key string) error { - parts := strings.SplitN(key, ".", 2) - if len(parts) != 2 || parts[1] == "" { - return fmt.Errorf("unset supports custom_providers. and mcp_servers.") - } - configPath, err := defaultConfigPath() if err != nil { return err } + // Top-level keys that do not contain a dot. + if !strings.Contains(key, ".") { + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + return fmt.Errorf("load config: %w", err) + } + switch key { + case "provider": + if cfg.Model != "" { + fmt.Fprintf(os.Stderr, "[ocr] WARNING: 'model' has also been cleared because it is tied to the provider.\n") + } + cfg.Provider = "" + cfg.Model = "" + case "model": + cfg.Model = "" + default: + return fmt.Errorf("unset supports provider, model, custom_providers. and mcp_servers.") + } + if err := saveConfig(configPath, cfg); err != nil { + return err + } + fmt.Printf("Unset %s\n", key) + return nil + } + + parts := strings.SplitN(key, ".", 2) + if len(parts) != 2 || parts[1] == "" { + return fmt.Errorf("unset supports provider, model, custom_providers. and mcp_servers.") + } + switch parts[0] { case "custom_providers": return unsetCustomProvider(configPath, parts[1]) case "mcp_servers": return unsetMCPServer(configPath, parts[1]) default: - return fmt.Errorf("unset supports custom_providers. and mcp_servers.") + return fmt.Errorf("unset supports provider, model, custom_providers. and mcp_servers.") } } diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 1cba3f99..7019cad9 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -3,6 +3,7 @@ package main import ( "os" "strconv" + "strings" "testing" "github.com/alibaba/open-code-review/internal/llm" @@ -1287,3 +1288,177 @@ func TestSetMCPServerValue_HeadersEmptyValue(t *testing.T) { t.Fatal("expected error for empty header value, got nil") } } + +// --- runConfigSet warning tests --- + +func TestRunConfigSetWarnsWhenLlmShadowedByProvider(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + // Create a config with an active provider. + configPath, _ := defaultConfigPath() + cfg := &Config{ + Provider: "anthropic", + Providers: map[string]ProviderEntry{ + "anthropic": {APIKey: "sk-test"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + // Capture stderr. + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := runConfigSet("llm.url", "https://custom.example.com") + + w.Close() + os.Stderr = oldStderr + if err != nil { + t.Fatalf("runConfigSet: %v", err) + } + + var buf strings.Builder + if _, err := buf.ReadFrom(r); err != nil { + t.Fatalf("read stderr: %v", err) + } + stderr := buf.String() + + if !strings.Contains(stderr, "WARNING") { + t.Errorf("stderr = %q, want WARNING about shadowed llm config", stderr) + } + if !strings.Contains(stderr, "anthropic") { + t.Errorf("stderr = %q, want provider name in warning", stderr) + } +} + +func TestRunConfigSetNoWarningWhenProviderNotActive(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + configPath, _ := defaultConfigPath() + cfg := &Config{} + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := runConfigSet("llm.url", "https://custom.example.com") + + w.Close() + os.Stderr = oldStderr + if err != nil { + t.Fatalf("runConfigSet: %v", err) + } + + var buf strings.Builder + if _, err := buf.ReadFrom(r); err != nil { + t.Fatalf("read stderr: %v", err) + } + stderr := buf.String() + + if strings.Contains(stderr, "WARNING") { + t.Errorf("stderr = %q, should NOT contain WARNING when no provider is active", stderr) + } +} + +// --- runConfigUnset tests --- + +func TestRunConfigUnsetProvider(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + configPath, _ := defaultConfigPath() + cfg := &Config{ + Provider: "anthropic", + Model: "claude-opus-4-6", + Providers: map[string]ProviderEntry{ + "anthropic": {APIKey: "sk-test"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + oldStderr := os.Stderr + r, w, _ := os.Pipe() + os.Stderr = w + + err := runConfigUnset("provider") + + w.Close() + os.Stderr = oldStderr + if err != nil { + t.Fatalf("runConfigUnset: %v", err) + } + + var buf strings.Builder + if _, err := buf.ReadFrom(r); err != nil { + t.Fatalf("read stderr: %v", err) + } + stderr := buf.String() + + if !strings.Contains(stderr, "WARNING") { + t.Errorf("stderr = %q, want WARNING about cleared model", stderr) + } + + cfg, err = loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload: %v", err) + } + if cfg.Provider != "" { + t.Errorf("Provider = %q, want empty", cfg.Provider) + } + if cfg.Model != "" { + t.Errorf("Model = %q, want empty", cfg.Model) + } +} + +func TestRunConfigUnsetModel(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + configPath, _ := defaultConfigPath() + cfg := &Config{ + Provider: "anthropic", + Model: "claude-opus-4-6", + Providers: map[string]ProviderEntry{ + "anthropic": {APIKey: "sk-test", Model: "claude-opus-4-6"}, + }, + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + if err := runConfigUnset("model"); err != nil { + t.Fatalf("runConfigUnset: %v", err) + } + + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("reload: %v", err) + } + if cfg.Provider != "anthropic" { + t.Errorf("Provider = %q, want anthropic", cfg.Provider) + } + if cfg.Model != "" { + t.Errorf("top-level Model = %q, want empty", cfg.Model) + } + if cfg.Providers["anthropic"].Model != "" { + t.Errorf("provider entry Model = %q, want empty", cfg.Providers["anthropic"].Model) + } +} + +func TestRunConfigUnsetInvalidKey(t *testing.T) { + dir := t.TempDir() + t.Setenv("HOME", dir) + + if err := runConfigUnset("invalid_key"); err == nil { + t.Fatal("expected error for invalid key") + } +} diff --git a/extensions/vscode/src/extension/services/CliService.ts b/extensions/vscode/src/extension/services/CliService.ts index 8de2994e..2089e7fb 100644 --- a/extensions/vscode/src/extension/services/CliService.ts +++ b/extensions/vscode/src/extension/services/CliService.ts @@ -32,7 +32,7 @@ export class CliService { private probeCommand(bin: string, args: string[]): Promise<{ ok: boolean; version?: string }> { return new Promise((resolve) => { - const proc = spawn(resolveBin(bin), args, { env: getShellEnv() }); + const proc = spawn(resolveBin(bin), args, { env: getShellEnv(), shell: process.platform === 'win32' }); let stdout = ''; let errored = false; proc.stdout?.on('data', (d) => { stdout += d.toString(); }); diff --git a/extensions/vscode/src/extension/services/__tests__/CliService.test.ts b/extensions/vscode/src/extension/services/__tests__/CliService.test.ts index 42ae24da..e63f3fce 100644 --- a/extensions/vscode/src/extension/services/__tests__/CliService.test.ts +++ b/extensions/vscode/src/extension/services/__tests__/CliService.test.ts @@ -1,6 +1,9 @@ // src/extension/services/__tests__/CliService.test.ts process.env.OCR_SKIP_SHELL_RESOLVE = '1'; import { CliService } from '../CliService'; +import { spawn } from 'child_process'; + +jest.mock('child_process'); describe('CliService.isAvailable', () => { it('node 一定存在 → true', async () => { @@ -13,6 +16,57 @@ describe('CliService.isAvailable', () => { }); }); +describe('CliService probe shell option', () => { + const originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + + afterEach(() => { + jest.resetAllMocks(); + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform); + } + }); + + it('Windows 上 probeCommand 应传入 shell: true', async () => { + Object.defineProperty(process, 'platform', { value: 'win32' }); + const mockProc = { + stdout: { on: jest.fn() }, + on: jest.fn((event: string, cb: Function) => { + if (event === 'close') cb(0); + }), + }; + (spawn as jest.Mock).mockReturnValue(mockProc); + + const svc = new CliService('node'); + await (svc as any).probeCommand('npm', ['--version']); + + expect(spawn).toHaveBeenCalledWith( + 'npm', + ['--version'], + expect.objectContaining({ shell: true }), + ); + }); + + it('非 Windows 上 probeCommand 不应传入 shell', async () => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + const mockProc = { + stdout: { on: jest.fn() }, + on: jest.fn((event: string, cb: Function) => { + if (event === 'close') cb(0); + }), + }; + (spawn as jest.Mock).mockReturnValue(mockProc); + + const svc = new CliService('node'); + await (svc as any).probeCommand('npm', ['--version']); + + expect(spawn).toHaveBeenCalledWith( + 'npm', + ['--version'], + expect.not.objectContaining({ shell: true }), + ); + }); +}); + describe('CliService.runRaw', () => { it('收集 stdout 并在结束时 resolve', async () => { // 用 node 打印一段 JSON 模拟 ocr diff --git a/internal/llm/providers.go b/internal/llm/providers.go index d80262ab..81cc496f 100644 --- a/internal/llm/providers.go +++ b/internal/llm/providers.go @@ -51,6 +51,18 @@ var registry = []Provider{ "gpt-5.4-mini", }, }, + { + Name: "openai-responses", + DisplayName: "OpenAI Responses API", + Protocol: ProtocolOpenAIResponses, + BaseURL: "https://api.openai.com/v1", + EnvVar: "OPENAI_API_KEY", + Models: []string{ + "gpt-5.6-luna", + "gpt-5.6-terra", + "gpt-5.6-sol", + }, + }, { Name: "edenai", DisplayName: "Eden AI", diff --git a/internal/llm/providers_test.go b/internal/llm/providers_test.go index 772d806d..dc99c9e4 100644 --- a/internal/llm/providers_test.go +++ b/internal/llm/providers_test.go @@ -40,7 +40,7 @@ func TestListProviders_Order(t *testing.T) { if len(providers) < 3 { t.Fatalf("expected at least 3 providers, got %d", len(providers)) } - expected := []string{"anthropic", "baidu-qianfan", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "hy-tokenplan", "iflytek", "kimi", "litellm", "mimo", "minimax", "ollama-cloud", "openai", "tencent-tokenhub", "volcengine", "z-ai", "z-ai-coding"} + expected := []string{"anthropic", "baidu-qianfan", "dashscope", "dashscope-tokenplan", "deepseek", "edenai", "hy-tokenplan", "iflytek", "kimi", "litellm", "mimo", "minimax", "ollama-cloud", "openai", "openai-responses", "tencent-tokenhub", "volcengine", "z-ai", "z-ai-coding"} if len(providers) != len(expected) { t.Fatalf("expected %d providers, got %d", len(expected), len(providers)) } @@ -87,6 +87,25 @@ func TestLookupProvider_PreservesModelOrder(t *testing.T) { } } +func TestLookupProvider_OpenAIResponsesModels(t *testing.T) { + p, ok := LookupProvider("openai-responses") + if !ok { + t.Fatal("openai-responses not found") + } + if p.Protocol != ProtocolOpenAIResponses { + t.Errorf("Protocol = %q, want %q", p.Protocol, ProtocolOpenAIResponses) + } + expected := []string{"gpt-5.6-luna", "gpt-5.6-terra", "gpt-5.6-sol"} + if len(p.Models) != len(expected) { + t.Fatalf("expected %d models, got %d", len(expected), len(p.Models)) + } + for i, model := range expected { + if p.Models[i] != model { + t.Errorf("Models[%d] = %q, want %q", i, p.Models[i], model) + } + } +} + func TestListProviders_ReturnsSortedProviders(t *testing.T) { providers := ListProviders() names := make([]string, len(providers))