diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index c7f19d685..3a1dea996 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -9,6 +9,7 @@ import ( "net/url" "os" "path/filepath" + "reflect" "strconv" "strings" @@ -332,6 +333,11 @@ type ProviderEntry struct { // first time any config command runs. AWSProfile string `json:"aws_profile,omitempty"` AWSRegion string `json:"aws_region,omitempty"` + + // unknownJSONFields keeps JSON keys with no matching struct field alive across + // a load/save cycle. Unexported: any struct-literal rebuild must copy it + // (see cloneProviderEntry) or the fields are dropped again + unknownJSONFields map[string]json.RawMessage } // MCPServerConfig holds configuration for a single MCP server. @@ -345,6 +351,8 @@ type MCPServerConfig struct { Headers map[string]string `json:"headers,omitempty"` Tools []string `json:"tools,omitempty"` Setup string `json:"setup,omitempty"` + + unknownJSONFields map[string]json.RawMessage } // Config represents the user-level configuration file (~/.opencodereview/config.json). @@ -359,6 +367,8 @@ type Config struct { Language string `json:"language,omitempty"` Telemetry *TelemetryConfig `json:"telemetry,omitempty"` MCPServers map[string]MCPServerConfig `json:"mcp_servers,omitempty"` + + unknownJSONFields map[string]json.RawMessage } type LlmConfig struct { @@ -373,6 +383,8 @@ type LlmConfig struct { ExtraBody map[string]any `json:"extra_body,omitempty"` ExtraHeaders map[string]string `json:"extra_headers,omitempty"` RetryCodes []int `json:"retry_codes,omitempty"` + + unknownJSONFields map[string]json.RawMessage } // TelemetryConfig holds telemetry-specific settings. @@ -381,6 +393,188 @@ type TelemetryConfig struct { Exporter string `json:"exporter,omitempty"` // "console" or "otlp" OTLPEndpoint string `json:"otlp_endpoint,omitempty"` // OTLP collector address ContentLog bool `json:"content_logging,omitempty"` // Include prompt/response content + + unknownJSONFields map[string]json.RawMessage +} + +func jsonFieldNames(value any) []string { + typeOf := reflect.TypeOf(value) + for typeOf.Kind() == reflect.Pointer { + typeOf = typeOf.Elem() + } + + fields := make([]string, 0, typeOf.NumField()) + for i := 0; i < typeOf.NumField(); i++ { + field := typeOf.Field(i) + if field.PkgPath != "" { + continue + } + tag := field.Tag.Get("json") + name, _, _ := strings.Cut(tag, ",") + if name != "" && name != "-" { + fields = append(fields, name) + } + } + return fields +} + +func collectUnknownJSONFields(data []byte, knownFields []string) (map[string]json.RawMessage, error) { + var fields map[string]json.RawMessage + if err := json.Unmarshal(data, &fields); err != nil { + return nil, err + } + + known := make(map[string]struct{}, len(knownFields)) + for _, field := range knownFields { + known[field] = struct{}{} + } + for field := range fields { + if _, ok := known[strings.ToLower(field)]; ok { + delete(fields, field) + } + } + return fields, nil +} + +func mergeUnknownJSONFields(data []byte, unknown map[string]json.RawMessage) ([]byte, error) { + if len(unknown) == 0 { + return data, nil + } + + var fields map[string]json.RawMessage + if err := json.Unmarshal(data, &fields); err != nil { + return nil, err + } + known := make(map[string]struct{}, len(fields)) + for field := range fields { + known[strings.ToLower(field)] = struct{}{} + } + for field, value := range unknown { + if _, exists := known[strings.ToLower(field)]; !exists { + fields[field] = value + } + } + return json.Marshal(fields) +} + +func (c *Config) UnmarshalJSON(data []byte) error { + type configAlias Config + var decoded configAlias + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(Config{})) + if err != nil { + return err + } + *c = Config(decoded) + c.unknownJSONFields = unknown + return nil +} + +func (c Config) MarshalJSON() ([]byte, error) { + type configAlias Config + data, err := json.Marshal(configAlias(c)) + if err != nil { + return nil, err + } + return mergeUnknownJSONFields(data, c.unknownJSONFields) +} + +func (e *ProviderEntry) UnmarshalJSON(data []byte) error { + type providerEntryAlias ProviderEntry + var decoded providerEntryAlias + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(ProviderEntry{})) + if err != nil { + return err + } + *e = ProviderEntry(decoded) + e.unknownJSONFields = unknown + return nil +} + +func (e ProviderEntry) MarshalJSON() ([]byte, error) { + type providerEntryAlias ProviderEntry + data, err := json.Marshal(providerEntryAlias(e)) + if err != nil { + return nil, err + } + return mergeUnknownJSONFields(data, e.unknownJSONFields) +} + +func (c *MCPServerConfig) UnmarshalJSON(data []byte) error { + type mcpServerConfigAlias MCPServerConfig + var decoded mcpServerConfigAlias + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(MCPServerConfig{})) + if err != nil { + return err + } + *c = MCPServerConfig(decoded) + c.unknownJSONFields = unknown + return nil +} + +func (c MCPServerConfig) MarshalJSON() ([]byte, error) { + type mcpServerConfigAlias MCPServerConfig + data, err := json.Marshal(mcpServerConfigAlias(c)) + if err != nil { + return nil, err + } + return mergeUnknownJSONFields(data, c.unknownJSONFields) +} + +func (c *LlmConfig) UnmarshalJSON(data []byte) error { + type llmConfigAlias LlmConfig + var decoded llmConfigAlias + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(LlmConfig{})) + if err != nil { + return err + } + *c = LlmConfig(decoded) + c.unknownJSONFields = unknown + return nil +} + +func (c LlmConfig) MarshalJSON() ([]byte, error) { + type llmConfigAlias LlmConfig + data, err := json.Marshal(llmConfigAlias(c)) + if err != nil { + return nil, err + } + return mergeUnknownJSONFields(data, c.unknownJSONFields) +} + +func (c *TelemetryConfig) UnmarshalJSON(data []byte) error { + type telemetryConfigAlias TelemetryConfig + var decoded telemetryConfigAlias + if err := json.Unmarshal(data, &decoded); err != nil { + return err + } + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(TelemetryConfig{})) + if err != nil { + return err + } + *c = TelemetryConfig(decoded) + c.unknownJSONFields = unknown + return nil +} + +func (c TelemetryConfig) MarshalJSON() ([]byte, error) { + type telemetryConfigAlias TelemetryConfig + data, err := json.Marshal(telemetryConfigAlias(c)) + if err != nil { + return nil, err + } + return mergeUnknownJSONFields(data, c.unknownJSONFields) } func loadOrCreateConfig(path string) (*Config, error) { diff --git a/cmd/opencodereview/config_cmd_test.go b/cmd/opencodereview/config_cmd_test.go index 35d4e9e72..91e2ad91f 100644 --- a/cmd/opencodereview/config_cmd_test.go +++ b/cmd/opencodereview/config_cmd_test.go @@ -4,9 +4,11 @@ package main import ( + "encoding/json" "io" "os" "path/filepath" + "reflect" "strconv" "strings" "testing" @@ -1534,6 +1536,80 @@ func TestConfigRoundTripPreservesTimeoutSec(t *testing.T) { } } +func TestConfigRoundTripPreservesUnknownFields(t *testing.T) { + configPath := filepath.Join(t.TempDir(), "config.json") + original := `{ + "provider": "anthropic", + "future_top_level": {"enabled": true}, + "providers": { + "anthropic": { + "model": "claude-opus-4-6", + "future_provider": {"value": "preserve-me"} + } + }, + "llm": { + "model": "claude-opus-4-6", + "future_llm": 7 + }, + "telemetry": { + "enabled": true, + "future_telemetry": "keep-me" + }, + "mcp_servers": { + "docs": { + "command": "docs-server", + "future_mcp": {"version": 2} + } + } +}` + if err := os.WriteFile(configPath, []byte(original), 0o600); err != nil { + t.Fatalf("write config: %v", err) + } + + cfg, err := loadOrCreateConfig(configPath) + if err != nil { + t.Fatalf("loadOrCreateConfig: %v", err) + } + if err := setConfigValue(cfg, "language", "English"); err != nil { + t.Fatalf("setConfigValue: %v", err) + } + if err := saveConfig(configPath, cfg); err != nil { + t.Fatalf("saveConfig: %v", err) + } + + data, err := os.ReadFile(configPath) + if err != nil { + t.Fatalf("read config: %v", err) + } + var got map[string]any + if err := json.Unmarshal(data, &got); err != nil { + t.Fatalf("parse saved config: %v", err) + } + assertJSONValue(t, got, []string{"future_top_level", "enabled"}, true) + assertJSONValue(t, got, []string{"providers", "anthropic", "future_provider", "value"}, "preserve-me") + assertJSONValue(t, got, []string{"llm", "future_llm"}, float64(7)) + assertJSONValue(t, got, []string{"telemetry", "future_telemetry"}, "keep-me") + assertJSONValue(t, got, []string{"mcp_servers", "docs", "future_mcp", "version"}, float64(2)) +} + +func assertJSONValue(t *testing.T, root map[string]any, path []string, want any) { + t.Helper() + var current any = root + for _, part := range path { + object, ok := current.(map[string]any) + if !ok { + t.Fatalf("JSON path %q entered %T at %q", strings.Join(path, "."), current, part) + } + current, ok = object[part] + if !ok { + t.Fatalf("JSON path %q is missing", strings.Join(path, ".")) + } + } + if !reflect.DeepEqual(current, want) { + t.Errorf("JSON path %q = %#v, want %#v", strings.Join(path, "."), current, want) + } +} + func TestSetMCPServerValue_Type(t *testing.T) { cfg := &Config{} if err := setMCPServerValue(cfg, "mcp_servers.gh.type", "remote"); err != nil { diff --git a/cmd/opencodereview/provider_tui.go b/cmd/opencodereview/provider_tui.go index fdd98013a..bdef802a9 100644 --- a/cmd/opencodereview/provider_tui.go +++ b/cmd/opencodereview/provider_tui.go @@ -4,6 +4,7 @@ package main import ( + "encoding/json" "fmt" "os" "sort" @@ -1300,6 +1301,7 @@ func cloneProviderEntry(v ProviderEntry) ProviderEntry { AWSProfile: v.AWSProfile, AWSRegion: v.AWSRegion, } + out.unknownJSONFields = cloneUnknownJSONFields(v.unknownJSONFields) if v.ExtraBody != nil { out.ExtraBody = make(map[string]any, len(v.ExtraBody)) for k, val := range v.ExtraBody { @@ -1315,6 +1317,17 @@ func cloneProviderEntry(v ProviderEntry) ProviderEntry { return out } +func cloneUnknownJSONFields(src map[string]json.RawMessage) map[string]json.RawMessage { + if src == nil { + return nil + } + out := make(map[string]json.RawMessage, len(src)) + for key, value := range src { + out[key] = append(json.RawMessage(nil), value...) + } + return out +} + func cloneCustomProvidersMap(src map[string]ProviderEntry) map[string]ProviderEntry { if src == nil { return nil diff --git a/cmd/opencodereview/provider_tui_funcs_test.go b/cmd/opencodereview/provider_tui_funcs_test.go index a961d43a9..8c5ea92f7 100644 --- a/cmd/opencodereview/provider_tui_funcs_test.go +++ b/cmd/opencodereview/provider_tui_funcs_test.go @@ -4,6 +4,7 @@ package main import ( + "encoding/json" "os" "path/filepath" "reflect" @@ -294,6 +295,9 @@ func TestCloneProviderEntry_CopiesEveryField(t *testing.T) { ExtraHeaders: map[string]string{"X-Trace": "on"}, AWSRegion: "us-west-2", AWSProfile: "example-profile", + unknownJSONFields: map[string]json.RawMessage{ + "future_provider": json.RawMessage(`{"value":"preserve-me"}`), + }, } rv := reflect.ValueOf(orig)