Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 36 additions & 6 deletions cmd/opencodereview/config_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.<name> and mcp_servers.<name>")
}

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 = ""
Comment thread
aalhadxx marked this conversation as resolved.
case "model":
cfg.Model = ""
default:
return fmt.Errorf("unset supports provider, model, custom_providers.<name> and mcp_servers.<name>")
}
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.<name> and mcp_servers.<name>")
}

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.<name> and mcp_servers.<name>")
return fmt.Errorf("unset supports provider, model, custom_providers.<name> and mcp_servers.<name>")
}
}

Expand Down
175 changes: 175 additions & 0 deletions cmd/opencodereview/config_cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ package main
import (
"os"
"strconv"
"strings"
"testing"

"github.com/alibaba/open-code-review/internal/llm"
Expand Down Expand Up @@ -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")
}
}
2 changes: 1 addition & 1 deletion extensions/vscode/src/extension/services/CliService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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(); });
Expand Down
Original file line number Diff line number Diff line change
@@ -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 () => {
Expand All @@ -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
Expand Down
12 changes: 12 additions & 0 deletions internal/llm/providers.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
21 changes: 20 additions & 1 deletion internal/llm/providers_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
Expand Down Expand Up @@ -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))
Expand Down