From 5200d1ea71e1b52795a274b9f8fb5f5242543b4a Mon Sep 17 00:00:00 2001 From: dvd233 <111864431+dvd233@users.noreply.github.com> Date: Sun, 20 Sep 2026 12:41:15 -0700 Subject: [PATCH 1/3] fix(config): preserve unknown JSON fields --- cmd/opencodereview/config_cmd.go | 180 ++++++++++++++++++ cmd/opencodereview/config_cmd_test.go | 76 ++++++++ cmd/opencodereview/provider_tui.go | 13 ++ cmd/opencodereview/provider_tui_funcs_test.go | 4 + 4 files changed, 273 insertions(+) diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index c7f19d685..07fce3fac 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -332,6 +332,8 @@ type ProviderEntry struct { // first time any config command runs. AWSProfile string `json:"aws_profile,omitempty"` AWSRegion string `json:"aws_region,omitempty"` + + unknownJSONFields map[string]json.RawMessage } // MCPServerConfig holds configuration for a single MCP server. @@ -345,6 +347,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 +363,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 +379,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 +389,178 @@ 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 +} + +var ( + providerEntryJSONFields = []string{ + "api_key", "api_key_cmd", "url", "protocol", "model", "models", "auth_header", + "timeout_sec", "extra_body", "extra_headers", "retry_codes", "aws_profile", "aws_region", + } + mcpServerConfigJSONFields = []string{"type", "command", "args", "env", "url", "headers", "tools", "setup"} + configJSONFields = []string{"provider", "model", "max_tokens", "effort", "providers", "custom_providers", "llm", "language", "telemetry", "mcp_servers"} + llmConfigJSONFields = []string{"url", "auth_token", "auth_token_cmd", "auth_header", "model", "protocol", "use_anthropic", "timeout_sec", "extra_body", "extra_headers", "retry_codes"} + telemetryConfigJSONFields = []string{"enabled", "exporter", "otlp_endpoint", "content_logging"} +) + +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, configJSONFields) + 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, providerEntryJSONFields) + 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, mcpServerConfigJSONFields) + 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, llmConfigJSONFields) + 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, telemetryConfigJSONFields) + 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) From 98e8c145d52e9e6e2417dbe03fa9fd7f98db0ca0 Mon Sep 17 00:00:00 2001 From: dvd233 <111864431+dvd233@users.noreply.github.com> Date: Sun, 20 Sep 2026 12:57:51 -0700 Subject: [PATCH 2/3] fix(config): derive JSON field names from tags --- cmd/opencodereview/config_cmd.go | 39 ++++++++++++++++++++------------ 1 file changed, 25 insertions(+), 14 deletions(-) diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 07fce3fac..13d2b82ce 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" @@ -393,16 +394,26 @@ type TelemetryConfig struct { unknownJSONFields map[string]json.RawMessage } -var ( - providerEntryJSONFields = []string{ - "api_key", "api_key_cmd", "url", "protocol", "model", "models", "auth_header", - "timeout_sec", "extra_body", "extra_headers", "retry_codes", "aws_profile", "aws_region", +func jsonFieldNames(value any) []string { + typeOf := reflect.TypeOf(value) + for typeOf.Kind() == reflect.Pointer { + typeOf = typeOf.Elem() } - mcpServerConfigJSONFields = []string{"type", "command", "args", "env", "url", "headers", "tools", "setup"} - configJSONFields = []string{"provider", "model", "max_tokens", "effort", "providers", "custom_providers", "llm", "language", "telemetry", "mcp_servers"} - llmConfigJSONFields = []string{"url", "auth_token", "auth_token_cmd", "auth_header", "model", "protocol", "use_anthropic", "timeout_sec", "extra_body", "extra_headers", "retry_codes"} - telemetryConfigJSONFields = []string{"enabled", "exporter", "otlp_endpoint", "content_logging"} -) + + 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 @@ -449,7 +460,7 @@ func (c *Config) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &decoded); err != nil { return err } - unknown, err := collectUnknownJSONFields(data, configJSONFields) + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(Config{})) if err != nil { return err } @@ -473,7 +484,7 @@ func (e *ProviderEntry) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &decoded); err != nil { return err } - unknown, err := collectUnknownJSONFields(data, providerEntryJSONFields) + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(ProviderEntry{})) if err != nil { return err } @@ -497,7 +508,7 @@ func (c *MCPServerConfig) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &decoded); err != nil { return err } - unknown, err := collectUnknownJSONFields(data, mcpServerConfigJSONFields) + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(MCPServerConfig{})) if err != nil { return err } @@ -521,7 +532,7 @@ func (c *LlmConfig) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &decoded); err != nil { return err } - unknown, err := collectUnknownJSONFields(data, llmConfigJSONFields) + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(LlmConfig{})) if err != nil { return err } @@ -545,7 +556,7 @@ func (c *TelemetryConfig) UnmarshalJSON(data []byte) error { if err := json.Unmarshal(data, &decoded); err != nil { return err } - unknown, err := collectUnknownJSONFields(data, telemetryConfigJSONFields) + unknown, err := collectUnknownJSONFields(data, jsonFieldNames(TelemetryConfig{})) if err != nil { return err } From 91a7b1532adaac2a95af790caaeeb2cf3773acb3 Mon Sep 17 00:00:00 2001 From: Kite <254839944+lizhengfeng101@users.noreply.github.com> Date: Tue, 22 Sep 2026 13:14:22 +0800 Subject: [PATCH 3/3] feat: Apply suggestion from @lizhengfeng101 --- cmd/opencodereview/config_cmd.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/cmd/opencodereview/config_cmd.go b/cmd/opencodereview/config_cmd.go index 13d2b82ce..3a1dea996 100644 --- a/cmd/opencodereview/config_cmd.go +++ b/cmd/opencodereview/config_cmd.go @@ -334,6 +334,9 @@ type ProviderEntry struct { 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 }