fix(config): preserve unknown JSON fields during config updates - #1508
Conversation
|
🔍 OpenCodeReview found 1 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| 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"} | ||
| ) |
There was a problem hiding this comment.
Bug-prone design: the known-field lists (configJSONFields, providerEntryJSONFields, etc.) are maintained separately from the struct definitions. If a developer adds a new field to a struct but forgets to update the corresponding list, the new field will be silently captured as "unknown". While mergeUnknownJSONFields prevents duplication when the struct field has a non-zero value, if the field is later cleared (zero value → omitted via omitempty), the stale unknown copy will resurrect it on the next marshal, producing incorrect config output.
Consider deriving the known-field list from the struct at init time using reflection (iterating over struct fields and reading their json tags). This eliminates the synchronization risk entirely:
func jsonFieldNames(v any) []string {
t := reflect.TypeOf(v)
var names []string
for i := 0; i < t.NumField(); i++ {
tag := t.Field(i).Tag.Get("json")
if tag == "" || tag == "-" { continue }
name, _, _ := strings.Cut(tag, ",")
names = append(names, name)
}
return names
}Alternatively, add a test that uses reflection to verify each list matches its struct.
|
Thanks for the contribution. The unknown-field round-trip looks correct, and deriving known JSON fields directly from struct tags addresses the maintenance concern from the previous review. Could you please squash the two commits into a single commit before merging? Since both commits are part of the same fix, keeping the PR as one atomic commit would make the history cleaner. |
|
Isn't this expected behavior @dvd233 ? What is the point of preserving a key that is never going to be used? (Please answer without AI/LLM) |
|
I think the chance of hitting this in practice is relatively low, but I still see value in it as a defensive fix. One concrete case is a temporary downgrade. Suppose a newer OCR version introduces a new config field, e.g. the older binary will unmarshal the config into its typed struct and write the whole file back, silently dropping So the point is not that the older binary needs to use the unknown key; it is that changing an unrelated setting should ideally not destroy configuration owned by a newer version. |
|
But older versions were archived and they overwrites jsons anyway because your fix cannot change the behavior of older releases .....
How are these related? |
Yeah, I think you're right. My downgrade example doesn't really justify this, since the fix can't change already released versions anyway. And if The So I don't think we need a generic unknown-field preservation layer here. |
4 participants
Description
ocr config set loads config.json into typed structs and writes the whole file back. Fields introduced by a newer binary, hand-added integrations, or nested provider/MCP sections were silently discarded when an older binary performed an unrelated config update.
This change preserves unknown JSON fields at the top level and in provider, LLM, telemetry, and MCP server sections while keeping known-field validation and config unset behavior unchanged. Provider TUI rollback cloning also deep-copies the preserved raw fields.
Type of Change
How Has This Been Tested?
GNU Make is not installed in the Windows environment, so the Makefile-equivalent commands were run directly. The race suite was not runnable because the available Windows Go toolchain has cgo disabled. The repository ocr review --audience agent self-review command was attempted but could not resolve an LLM endpoint because no test credentials/endpoint are configured.
Checklist
AI/LLM disclosure: OpenAI Codex (GPT-5) in the Codex desktop environment assisted with repository inspection and drafting. The submitted code and tests were manually reviewed.
Related Issues
Fixes #1485