From 2d9db1b60d8dfa6089695ca86908119aa1364c7e Mon Sep 17 00:00:00 2001 From: Alex Date: Thu, 8 Oct 2026 11:52:20 +0800 Subject: [PATCH] edit: malformed JSON configs stay untouched when agents connect (#1242) Validate the complete comment-stripped document before object and array edits, including previews. Regression tests fail on the old implementation because edits and VS Code Connect succeed and alter malformed files. Targeted edit and VS Code tests pass in an isolated HOME. --- docs/subsystems/agent-wiring.md | 1 + internal/agent/vscode_invalid_test.go | 28 ++++++++++ internal/edit/json.go | 6 +++ internal/edit/json_invalid_test.go | 74 +++++++++++++++++++++++++++ internal/edit/jsonarray.go | 6 +++ 5 files changed, 115 insertions(+) create mode 100644 internal/agent/vscode_invalid_test.go create mode 100644 internal/edit/json_invalid_test.go diff --git a/docs/subsystems/agent-wiring.md b/docs/subsystems/agent-wiring.md index ee59f3a2..d3113713 100644 --- a/docs/subsystems/agent-wiring.md +++ b/docs/subsystems/agent-wiring.md @@ -35,6 +35,7 @@ something else shows up as drift. - magpie only removes what it wrote. Anything else in an agent's config stays as it was. - JetBrains Air's `magpie-opencode.json` keeps user-added fields in `provider.magpie`, including nested options, on sync and model picks (`air` in [`air.go`](../../internal/agent/air.go), using `theirsKept` in [`sync.go`](../../internal/agent/sync.go)). The gateway fields and complete model list are still replaced, so removed models leave the list. +- JSON/JSONC config edits and their previews reject malformed input rather than editing a partially parsed document. Object edits and VS Code's model-group array edits leave the original file untouched on a parse error; valid comments and trailing commas remain supported (`internal/edit`). - Codex signed in to ChatGPT stays beside its sign-in (`openai_base_url`) unless the user picks magpie API. magpie becomes its provider only while the Codex app holds the active account, which it sends nothing for: `/wham/usage` says it isn't allowed, and it has no credits and isn't a workspace still within its overage (`codexHeld`, read by `provider.CodexUsedUp`). Where the app goes by what usage can't show (an experiment's gate, a reserve, a Team plan's overage under the reserve experiment), it counts as held: a wrong move costs ChatGPT extras, a missed one every turn. A saved reading kept through a failed read stops saying held once its used-up window has started again; a hold with no window used up stays till a fresh read. The app then loses its ChatGPT state (durable threads, remote control), so a window at 100% alone, which credits get past, doesn't move it (`codex.out` marks the move, and Sync undoes it once the account has room). - Moving a field from one of magpie's models to another (the agent's own picker) is not drift. Moving it off magpie is. - A field that follows another while empty (`Follows`: Claude Code's tiers and subagents on its main model) is read by `Drift` as on that field's model. The main model moved, in Claude Code's own `/model`, onto the model magpie had set a tier to is not drift (#1050). diff --git a/internal/agent/vscode_invalid_test.go b/internal/agent/vscode_invalid_test.go new file mode 100644 index 00000000..2bcf5070 --- /dev/null +++ b/internal/agent/vscode_invalid_test.go @@ -0,0 +1,28 @@ +package agent + +import ( + "path/filepath" + "testing" +) + +func TestVSCodeConnectPreservesInvalidConfig(t *testing.T) { + for _, file := range []string{"settings.json", "chatLanguageModels.json"} { + t.Run(file, func(t *testing.T) { + a := vscodeAt(filepath.Join(syncHome(t), "vscode", "User")) + writeFile(t, a.Path, `{"chat.defaultModel":"auto"}`) + models := filepath.Join(a.Dir, "chatLanguageModels.json") + writeFile(t, models, `[{"name":"mine","vendor":"ollama"}]`) + path, bad := a.Path, `{"chat.defaultModel":"auto" "editor.fontSize":14}` + if file == "chatLanguageModels.json" { + path, bad = models, `[{"name":"mine","vendor":"ollama"} {"name":"other"}]` + } + writeFile(t, path, bad) + if err := a.Connect(); err == nil { + t.Error("connecting VS Code with malformed config succeeded") + } + if got := readFile(path); got != bad { + t.Errorf("connecting VS Code changed malformed %s: %q", file, got) + } + }) + } +} diff --git a/internal/edit/json.go b/internal/edit/json.go index 400b74d4..883d511d 100644 --- a/internal/edit/json.go +++ b/internal/edit/json.go @@ -87,6 +87,9 @@ func setJSONBytes(raw []byte, keyPath string, value any) ([]byte, error) { // Comments are blanked out in a copy of identical length so gjson offsets // map 1:1 back onto the original bytes. stripped := jsonc.ToJSONInPlace(append([]byte(nil), raw...)) + if !json.Valid(stripped) { + return nil, fmt.Errorf("invalid JSON/JSONC") + } root := gjson.ParseBytes(stripped) if !root.IsObject() { return nil, fmt.Errorf("top level is not a JSON object") @@ -266,6 +269,9 @@ func DelJSON(path string, keyPaths ...string) error { func delJSONBytes(raw []byte, keyPath string) ([]byte, bool, error) { stripped := jsonc.ToJSONInPlace(append([]byte(nil), raw...)) + if !json.Valid(stripped) { + return nil, false, fmt.Errorf("invalid JSON/JSONC") + } r := gjson.GetBytes(stripped, keyPath) if !r.Exists() || r.Index == 0 { return raw, false, nil diff --git a/internal/edit/json_invalid_test.go b/internal/edit/json_invalid_test.go new file mode 100644 index 00000000..8011573b --- /dev/null +++ b/internal/edit/json_invalid_test.go @@ -0,0 +1,74 @@ +package edit + +import ( + "bytes" + "os" + "testing" +) + +func TestJSONEditsPreserveInvalidFiles(t *testing.T) { + for _, input := range []string{ + `{"model":"old", "keep":`, + `{"model":"old" "keep":true}`, + `{"model":"old"} {"keep":true}`, + } { + for name, edit := range map[string]func(string) error{ + "set": func(p string) error { return SetJSON(p, KV{"model", "new"}) }, + "delete": func(p string) error { return DelJSON(p, "model") }, + "delete absent": func(p string) error { return DelJSON(p, "missing") }, + } { + t.Run(name+"/"+input, func(t *testing.T) { + p := tmpFile(t, "settings.json", input) + if err := edit(p); err == nil { + t.Error("editing invalid JSON succeeded") + } + if got, err := os.ReadFile(p); err != nil || string(got) != input { + t.Errorf("invalid config was changed: %q, %v", got, err) + } + }) + } + for name, changes := range map[string]struct { + set []KV + del []string + }{ + "set": {set: []KV{{"model", "new"}}}, + "delete": {del: []string{"model"}}, + } { + t.Run("preview/"+name+"/"+input, func(t *testing.T) { + raw := []byte(input) + if _, err := PatchJSON(raw, changes.set, changes.del); err == nil { + t.Error("previewing an edit of invalid JSON succeeded") + } + if !bytes.Equal(raw, []byte(input)) { + t.Fatal("preview changed input") + } + }) + } + } +} + +func TestJSONItemEditsPreserveInvalidFiles(t *testing.T) { + where := map[string]string{"name": "magpie"} + for _, input := range []string{ + `[{"name":"magpie"}, {"name":]`, + `[{"name":"magpie"} {"name":"keep"}]`, + `[{"name":"magpie"}] [{"name":"keep"}]`, + } { + for name, edit := range map[string]func(string) error{ + "set": func(p string) error { + return SetJSONItem(p, where, map[string]string{"name": "magpie", "model": "new"}) + }, + "delete": func(p string) error { return DelJSONItem(p, where) }, + } { + t.Run(name+"/"+input, func(t *testing.T) { + p := tmpFile(t, "chatLanguageModels.json", input) + if err := edit(p); err == nil { + t.Error("editing invalid JSON array succeeded") + } + if got, err := os.ReadFile(p); err != nil || string(got) != input { + t.Errorf("invalid config was changed: %q, %v", got, err) + } + }) + } + } +} diff --git a/internal/edit/jsonarray.go b/internal/edit/jsonarray.go index 3a101d15..ca76124b 100644 --- a/internal/edit/jsonarray.go +++ b/internal/edit/jsonarray.go @@ -42,6 +42,9 @@ func SetJSONItem(path string, where map[string]string, value any) error { return WriteAtomic(path, []byte("[\n\t"+string(v)+"\n]\n")) } stripped := jsonc.ToJSONInPlace(append([]byte(nil), raw...)) + if !json.Valid(stripped) { + return fmt.Errorf("%s: invalid JSON/JSONC", path) + } root, open, close := topArray(stripped) if close < 0 { return fmt.Errorf("%s: top level is not a JSON array", path) @@ -85,6 +88,9 @@ func DelJSONItem(path string, where map[string]string) error { changed := false for { stripped := jsonc.ToJSONInPlace(append([]byte(nil), raw...)) + if !json.Valid(stripped) { + return fmt.Errorf("%s: invalid JSON/JSONC", path) + } r, ok := findItem(stripped, where) if !ok { break