Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
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
28 changes: 18 additions & 10 deletions cmd/opencodereview/config_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -176,7 +176,7 @@ func unsetActiveProvider(configPath string) error {
}

func legacyLLMShadowWarning(provider, key string) string {
if provider == "" || !strings.HasPrefix(key, "llm.") {
if provider == "" || !strings.HasPrefix(key, "llm.") || key == "llm.prompt_caching" || key == "llm.PromptCaching" {
return ""
}
section := "custom_providers"
Expand Down Expand Up @@ -300,15 +300,16 @@ type Config struct {
}

type LlmConfig struct {
URL string `json:"url,omitempty"`
AuthToken string `json:"auth_token,omitempty"`
AuthHeader string `json:"auth_header,omitempty"`
Model string `json:"model,omitempty"`
Protocol string `json:"protocol,omitempty"` // canonical protocol name; takes priority over UseAnthropic
UseAnthropic *bool `json:"use_anthropic,omitempty"` // nil = default true; false = OpenAI protocol (legacy fallback)
TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds
ExtraBody map[string]any `json:"extra_body,omitempty"`
ExtraHeaders map[string]string `json:"extra_headers,omitempty"`
URL string `json:"url,omitempty"`
AuthToken string `json:"auth_token,omitempty"`
AuthHeader string `json:"auth_header,omitempty"`
Model string `json:"model,omitempty"`
Protocol string `json:"protocol,omitempty"` // canonical protocol name; takes priority over UseAnthropic
UseAnthropic *bool `json:"use_anthropic,omitempty"` // nil = default true; false = OpenAI protocol (legacy fallback)
TimeoutSec int `json:"timeout_sec,omitempty"` // per-request HTTP timeout in seconds
ExtraBody map[string]any `json:"extra_body,omitempty"`
ExtraHeaders map[string]string `json:"extra_headers,omitempty"`
PromptCaching *bool `json:"prompt_caching,omitempty"`
}

// TelemetryConfig holds telemetry-specific settings.
Expand Down Expand Up @@ -365,6 +366,7 @@ var supportedConfigKeys = []string{
"llm.model",
"llm.protocol",
"llm.use_anthropic",
"llm.prompt_caching",
"llm.extra_body",
"llm.extra_headers",
"language",
Expand Down Expand Up @@ -476,6 +478,12 @@ func setConfigValue(cfg *Config, key, value string) error {
} else if cfg.Llm.Protocol == "" || cfg.Llm.Protocol == llm.ProtocolAnthropic || cfg.Llm.Protocol == llm.ProtocolOpenAIChatCompletions {
cfg.Llm.Protocol = llm.ProtocolOpenAIChatCompletions
}
case "llm.prompt_caching", "llm.PromptCaching":
b, err := strconv.ParseBool(value)
if err != nil {
return fmt.Errorf("invalid boolean for llm.prompt_caching: %w", err)
}
cfg.Llm.PromptCaching = &b
case "language", "Language":
cfg.Language = value
case "telemetry.enabled", "telemetry.Enabled":
Expand Down
21 changes: 20 additions & 1 deletion cmd/opencodereview/config_cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -830,6 +830,22 @@ func TestSetConfigValueLlmUseAnthropicInvalid(t *testing.T) {
}
}

func TestSetConfigValueLlmPromptCaching(t *testing.T) {
cfg := &Config{}
if err := setConfigValue(cfg, "llm.prompt_caching", "false"); err != nil {
t.Fatalf("setConfigValue: %v", err)
}
if cfg.Llm.PromptCaching == nil || *cfg.Llm.PromptCaching {
t.Fatalf("PromptCaching = %v, want false", cfg.Llm.PromptCaching)
}
}

func TestSetConfigValueLlmPromptCachingInvalid(t *testing.T) {
if err := setConfigValue(&Config{}, "llm.prompt_caching", "sometimes"); err == nil {
t.Fatal("expected error for invalid boolean")
}
}

func TestSetConfigValueLanguage(t *testing.T) {
cfg := &Config{}
if err := setConfigValue(cfg, "language", "English"); err != nil {
Expand Down Expand Up @@ -929,7 +945,7 @@ func TestSetConfigValueUnknownKeyMessage(t *testing.T) {
t.Fatal("expected error for unknown key")
}
want := "unknown config key: bogus.key\n" +
"Supported keys: provider, model, providers.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\n" +
"Supported keys: provider, model, providers.<name>.<field>, custom_providers.<name>.<field>, mcp_servers.<name>.<field>, llm.url, llm.auth_token, llm.auth_header, llm.model, llm.protocol, llm.use_anthropic, llm.prompt_caching, llm.extra_body, llm.extra_headers, language, telemetry.enabled, telemetry.exporter, telemetry.otlp_endpoint, telemetry.content_logging\n" +
"Provider fields: api_key, url, protocol, model, models, auth_header, extra_body, extra_headers\n" +
"Protocol values: anthropic, openai, openai-responses\n" +
"MCP server fields: type, command, args, env, url, headers, tools, setup"
Expand Down Expand Up @@ -998,6 +1014,9 @@ func TestLegacyLLMShadowWarning(t *testing.T) {
if got := legacyLLMShadowWarning("dashscope", "Llm.model"); got != "" {
t.Errorf("warning for invalid mixed-case legacy key = %q", got)
}
if got := legacyLLMShadowWarning("dashscope", "llm.prompt_caching"); got != "" {
t.Errorf("warning for global prompt caching setting = %q", got)
}
if got := legacyLLMShadowWarning("dashscope", "llm.model"); !strings.Contains(got, "providers.dashscope.<field>") {
t.Errorf("preset-provider warning = %q", got)
}
Expand Down
9 changes: 9 additions & 0 deletions cmd/opencodereview/provider_cmd.go
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ func applyManualConfig(configPath string, cfg *Config, result providerTUIResult)
return fmt.Errorf("invalid auth_header: %w", err)
}
cfg.Llm.AuthHeader = authHeader
if result.promptCaching != nil {
cfg.Llm.PromptCaching = result.promptCaching
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[maintainability · low]
Potential issue: When switching from Anthropic to a non-Anthropic protocol, the old PromptCaching value remains in the config file. While this doesn't cause functional problems (the setting is only used for Anthropic), it leaves stale configuration data. Consider clearing the field when the protocol is not Anthropic:

if result.promptCaching != nil {
    cfg.Llm.PromptCaching = result.promptCaching
} else if llm.NormalizeProtocol(result.protocol) != llm.ProtocolAnthropic {
    cfg.Llm.PromptCaching = nil
}

This same pattern appears in all three apply functions (applyManualConfig, applyCustomProviderConfig, applyOfficialProviderConfig) and should be updated consistently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in c07b2a7. The manual, custom, and official apply paths now clear stale prompt-caching state when the effective protocol is not Anthropic. Regression coverage includes all three switch paths and preserves the active Anthropic setting when an inactive OpenAI provider is edited.

// Write the canonical protocol so resolver picks it up directly. Also
// mirror use_anthropic so configs read correctly on older binaries that
// predate llm.protocol: anthropic -> true, the OpenAI family (including
Expand Down Expand Up @@ -186,6 +189,9 @@ func applyCustomProviderConfig(configPath string, cfg *Config, result providerTU
} else {
entry.APIKey = ""
}
if result.promptCaching != nil {
cfg.Llm.PromptCaching = result.promptCaching
}
cfg.CustomProviders[result.provider] = entry

if !result.isEdit {
Expand Down Expand Up @@ -260,6 +266,9 @@ func applyOfficialProviderConfig(configPath string, cfg *Config, result provider
// Confirmed empty key: clear saved api_key so resolver falls back to $ENV_VAR.
entry.APIKey = ""
}
if result.promptCaching != nil {
cfg.Llm.PromptCaching = result.promptCaching
}
cfg.Providers[result.provider] = entry

if cfg.Provider != result.provider {
Expand Down
41 changes: 41 additions & 0 deletions cmd/opencodereview/provider_cmd_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"os"
"path/filepath"
"testing"

"github.com/alibaba/open-code-review/internal/llm"
)

func TestMaskKey(t *testing.T) {
Expand Down Expand Up @@ -274,6 +276,45 @@ func TestApplyCustomProviderConfig_EmptyKeyClearsSavedAPIKey(t *testing.T) {
}
}

func TestApplyCustomProviderConfig_PersistsPromptCaching(t *testing.T) {
disabled := false
configPath := filepath.Join(t.TempDir(), "config.json")
cfg := &Config{
Provider: "gateway",
Model: "claude-test",
CustomProviders: map[string]ProviderEntry{
"gateway": {
URL: "https://gateway.example/v1",
Protocol: llm.ProtocolAnthropic,
APIKey: "test-key",
Model: "claude-test",
Models: []string{"claude-test"},
},
},
}

err := applyCustomProviderConfig(configPath, cfg, providerTUIResult{
provider: "gateway",
model: "claude-test",
models: []string{"claude-test"},
apiKey: "test-key",
isCustom: true,
url: "https://gateway.example/v1",
protocol: llm.ProtocolAnthropic,
promptCaching: &disabled,
})
if err != nil {
t.Fatalf("applyCustomProviderConfig: %v", err)
}
diskCfg, err := loadOrCreateConfig(configPath)
if err != nil {
t.Fatalf("load config: %v", err)
}
if diskCfg.Llm.PromptCaching == nil || *diskCfg.Llm.PromptCaching {
t.Fatalf("PromptCaching = %v, want false", diskCfg.Llm.PromptCaching)
}
}

func TestProviderTUIResult_ResolvedModel(t *testing.T) {
r := providerTUIResult{
provider: "baidu-qianfan",
Expand Down
Loading
Loading