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.
This commit is contained in:
5 files changed
+115
No files matched your search
@@ -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).
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
@@ -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)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -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
|
||||
|
||||
Reference in new issue
Block a user