From 249d7c73636a21e133383d0952e7cb07338eb1bc Mon Sep 17 00:00:00 2001 From: Gordon Date: Tue, 17 Mar 2026 10:52:34 +0100 Subject: [PATCH 1/5] fix: handle double-serialized edits argument in edit_file tool LLMs sometimes send the edits parameter as a JSON string instead of a JSON array (e.g. "[{...}]" rather than [{...}]). Add a custom UnmarshalJSON on EditFileArgs that detects this and transparently double-deserializes the string into the expected []Edit slice. This also fixes the ACP filesystem handler since it reuses the builtin.EditFileArgs type. --- pkg/tools/builtin/filesystem.go | 40 ++++++++++++++++ pkg/tools/builtin/filesystem_test.go | 68 ++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+) diff --git a/pkg/tools/builtin/filesystem.go b/pkg/tools/builtin/filesystem.go index 1423985e2..e92a0dd3f 100644 --- a/pkg/tools/builtin/filesystem.go +++ b/pkg/tools/builtin/filesystem.go @@ -165,6 +165,46 @@ type EditFileArgs struct { Edits []Edit `json:"edits" jsonschema:"Array of edit operations"` } +// UnmarshalJSON handles LLM-generated arguments where "edits" may be +// a JSON string instead of a JSON array (double-serialized). +func (a *EditFileArgs) UnmarshalJSON(data []byte) error { + // First, try standard unmarshaling. + type Alias EditFileArgs + var std Alias + if err := json.Unmarshal(data, &std); err == nil { + *a = EditFileArgs(std) + return nil + } + + // If that failed, the "edits" field may be a JSON string. + // Parse it as a raw message and attempt double-deserialization. + var raw struct { + Path string `json:"path"` + Edits json.RawMessage `json:"edits"` + } + if err := json.Unmarshal(data, &raw); err != nil { + return fmt.Errorf("failed to parse edit_file arguments: %w", err) + } + + a.Path = raw.Path + + // Try parsing edits as an array first (normal case). + if err := json.Unmarshal(raw.Edits, &a.Edits); err == nil { + return nil + } + + // Try unwrapping a double-serialized JSON string. + var editsStr string + if err := json.Unmarshal(raw.Edits, &editsStr); err != nil { + return fmt.Errorf("edits field is neither an array nor a JSON string: %w", err) + } + if err := json.Unmarshal([]byte(editsStr), &a.Edits); err != nil { + return fmt.Errorf("failed to parse double-serialized edits string: %w", err) + } + + return nil +} + func (t *FilesystemTool) Tools(context.Context) ([]tools.Tool, error) { return []tools.Tool{ { diff --git a/pkg/tools/builtin/filesystem_test.go b/pkg/tools/builtin/filesystem_test.go index 668be54ad..a7f641bd5 100644 --- a/pkg/tools/builtin/filesystem_test.go +++ b/pkg/tools/builtin/filesystem_test.go @@ -260,6 +260,74 @@ func TestFilesystemTool_EditFile(t *testing.T) { assert.Contains(t, result.Output, "old text not found") } +func TestEditFileArgs_UnmarshalJSON(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + input string + wantPath string + wantEdits []Edit + wantErr bool + }{ + { + name: "normal array edits", + input: `{"path": "test.txt", "edits": [{"oldText": "hello", "newText": "world"}]}`, + wantPath: "test.txt", + wantEdits: []Edit{ + {OldText: "hello", NewText: "world"}, + }, + }, + { + name: "double-serialized string edits", + input: `{"path": "test.txt", "edits": "[{\"oldText\": \"hello\", \"newText\": \"world\"}]"}`, + wantPath: "test.txt", + wantEdits: []Edit{ + {OldText: "hello", NewText: "world"}, + }, + }, + { + name: "double-serialized multiple edits", + input: `{"path": "f.go", "edits": "[{\"oldText\": \"a\", \"newText\": \"b\"}, {\"oldText\": \"c\", \"newText\": \"d\"}]"}`, + wantPath: "f.go", + wantEdits: []Edit{ + {OldText: "a", NewText: "b"}, + {OldText: "c", NewText: "d"}, + }, + }, + { + name: "invalid JSON", + input: `not json at all`, + wantErr: true, + }, + { + name: "edits is neither array nor string", + input: `{"path": "test.txt", "edits": 42}`, + wantErr: true, + }, + { + name: "double-serialized but inner JSON is invalid", + input: `{"path": "test.txt", "edits": "not valid json"}`, + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + var args EditFileArgs + err := json.Unmarshal([]byte(tc.input), &args) + if tc.wantErr { + assert.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tc.wantPath, args.Path) + assert.Equal(t, tc.wantEdits, args.Edits) + }) + } +} + func TestFilesystemTool_SearchFilesContent(t *testing.T) { t.Parallel() tmpDir := t.TempDir() From 549fea0a11562bccf8565988d51ca4a07cb240cf Mon Sep 17 00:00:00 2001 From: Gordon Date: Tue, 17 Mar 2026 12:23:07 +0100 Subject: [PATCH 2/5] fix: validate path field in double-serialization fallback Add a check that the path field is non-empty when the UnmarshalJSON fallback path is taken, so that a missing path produces a clear error instead of a confusing downstream failure. --- pkg/tools/builtin/filesystem.go | 3 +++ pkg/tools/builtin/filesystem_test.go | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/pkg/tools/builtin/filesystem.go b/pkg/tools/builtin/filesystem.go index e92a0dd3f..b0139f007 100644 --- a/pkg/tools/builtin/filesystem.go +++ b/pkg/tools/builtin/filesystem.go @@ -187,6 +187,9 @@ func (a *EditFileArgs) UnmarshalJSON(data []byte) error { } a.Path = raw.Path + if a.Path == "" { + return errors.New("path field is required") + } // Try parsing edits as an array first (normal case). if err := json.Unmarshal(raw.Edits, &a.Edits); err == nil { diff --git a/pkg/tools/builtin/filesystem_test.go b/pkg/tools/builtin/filesystem_test.go index a7f641bd5..50a7b1074 100644 --- a/pkg/tools/builtin/filesystem_test.go +++ b/pkg/tools/builtin/filesystem_test.go @@ -310,6 +310,11 @@ func TestEditFileArgs_UnmarshalJSON(t *testing.T) { input: `{"path": "test.txt", "edits": "not valid json"}`, wantErr: true, }, + { + name: "missing path in double-serialized fallback", + input: `{"edits": "[{\"oldText\": \"a\", \"newText\": \"b\"}]"}`, + wantErr: true, + }, } for _, tc := range tests { From e8a0ae7d736150aee5d88b100a1b52f4a9a0d008 Mon Sep 17 00:00:00 2001 From: Gordon Date: Tue, 17 Mar 2026 13:15:58 +0100 Subject: [PATCH 3/5] fix: simplify UnmarshalJSON and validate path consistently Remove the type alias approach (standard unmarshal + fallback) and always parse through RawMessage. This simplifies the code and ensures path validation happens consistently for both normal and double-serialized edits. Add test cases for missing/empty path with normal array edits. --- pkg/tools/builtin/filesystem.go | 10 ---------- pkg/tools/builtin/filesystem_test.go | 10 ++++++++++ 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/tools/builtin/filesystem.go b/pkg/tools/builtin/filesystem.go index b0139f007..6b3555f9f 100644 --- a/pkg/tools/builtin/filesystem.go +++ b/pkg/tools/builtin/filesystem.go @@ -168,16 +168,6 @@ type EditFileArgs struct { // UnmarshalJSON handles LLM-generated arguments where "edits" may be // a JSON string instead of a JSON array (double-serialized). func (a *EditFileArgs) UnmarshalJSON(data []byte) error { - // First, try standard unmarshaling. - type Alias EditFileArgs - var std Alias - if err := json.Unmarshal(data, &std); err == nil { - *a = EditFileArgs(std) - return nil - } - - // If that failed, the "edits" field may be a JSON string. - // Parse it as a raw message and attempt double-deserialization. var raw struct { Path string `json:"path"` Edits json.RawMessage `json:"edits"` diff --git a/pkg/tools/builtin/filesystem_test.go b/pkg/tools/builtin/filesystem_test.go index 50a7b1074..dcf88c091 100644 --- a/pkg/tools/builtin/filesystem_test.go +++ b/pkg/tools/builtin/filesystem_test.go @@ -315,6 +315,16 @@ func TestEditFileArgs_UnmarshalJSON(t *testing.T) { input: `{"edits": "[{\"oldText\": \"a\", \"newText\": \"b\"}]"}`, wantErr: true, }, + { + name: "missing path with normal array edits", + input: `{"edits": [{"oldText": "a", "newText": "b"}]}`, + wantErr: true, + }, + { + name: "empty path with normal array edits", + input: `{"path": "", "edits": [{"oldText": "a", "newText": "b"}]}`, + wantErr: true, + }, } for _, tc := range tests { From f65a06545a02133aba0b46249c4f5d8b824062be Mon Sep 17 00:00:00 2001 From: Gordon Date: Tue, 17 Mar 2026 13:25:53 +0100 Subject: [PATCH 4/5] fix: accept partial arguments in UnmarshalJSON for TUI compatibility The TUI renders tool calls while arguments are still streaming, so it may unmarshal EditFileArgs with only a path and no edits field. Accept nil/missing edits gracefully instead of returning an error. Also remove the strict path-is-required validation from UnmarshalJSON since the handler already produces a clear error when the path is empty. This keeps UnmarshalJSON focused on parsing concerns. --- pkg/tools/builtin/filesystem.go | 7 +++++-- pkg/tools/builtin/filesystem_test.go | 27 ++++++++++++++++++--------- 2 files changed, 23 insertions(+), 11 deletions(-) diff --git a/pkg/tools/builtin/filesystem.go b/pkg/tools/builtin/filesystem.go index 6b3555f9f..6b05dfe9e 100644 --- a/pkg/tools/builtin/filesystem.go +++ b/pkg/tools/builtin/filesystem.go @@ -177,8 +177,11 @@ func (a *EditFileArgs) UnmarshalJSON(data []byte) error { } a.Path = raw.Path - if a.Path == "" { - return errors.New("path field is required") + + // When edits is missing or null (e.g. during argument streaming in + // the TUI, or partial tool calls), accept the partial result. + if len(raw.Edits) == 0 || string(raw.Edits) == "null" { + return nil } // Try parsing edits as an array first (normal case). diff --git a/pkg/tools/builtin/filesystem_test.go b/pkg/tools/builtin/filesystem_test.go index dcf88c091..a5100d417 100644 --- a/pkg/tools/builtin/filesystem_test.go +++ b/pkg/tools/builtin/filesystem_test.go @@ -311,19 +311,28 @@ func TestEditFileArgs_UnmarshalJSON(t *testing.T) { wantErr: true, }, { - name: "missing path in double-serialized fallback", - input: `{"edits": "[{\"oldText\": \"a\", \"newText\": \"b\"}]"}`, - wantErr: true, + name: "missing edits field (partial/streaming args)", + input: `{"path": "/tmp/test.txt"}`, + wantPath: "/tmp/test.txt", }, { - name: "missing path with normal array edits", - input: `{"edits": [{"oldText": "a", "newText": "b"}]}`, - wantErr: true, + name: "null edits field", + input: `{"path": "test.txt", "edits": null}`, + wantPath: "test.txt", }, { - name: "empty path with normal array edits", - input: `{"path": "", "edits": [{"oldText": "a", "newText": "b"}]}`, - wantErr: true, + name: "missing path with double-serialized edits", + input: `{"edits": "[{\"oldText\": \"a\", \"newText\": \"b\"}]"}`, + wantEdits: []Edit{ + {OldText: "a", NewText: "b"}, + }, + }, + { + name: "missing path with normal array edits", + input: `{"edits": [{"oldText": "a", "newText": "b"}]}`, + wantEdits: []Edit{ + {OldText: "a", NewText: "b"}, + }, }, } From 19c6b5b7e74d545b9f83aefdd04e4b4b8926ce0a Mon Sep 17 00:00:00 2001 From: Gordon Date: Tue, 17 Mar 2026 14:24:22 +0100 Subject: [PATCH 5/5] test: assert specific error messages in UnmarshalJSON tests Check that each error case returns the expected error message, not just that an error occurred. --- pkg/tools/builtin/filesystem_test.go | 35 ++++++++++++++++------------ 1 file changed, 20 insertions(+), 15 deletions(-) diff --git a/pkg/tools/builtin/filesystem_test.go b/pkg/tools/builtin/filesystem_test.go index a5100d417..84b0319de 100644 --- a/pkg/tools/builtin/filesystem_test.go +++ b/pkg/tools/builtin/filesystem_test.go @@ -264,11 +264,12 @@ func TestEditFileArgs_UnmarshalJSON(t *testing.T) { t.Parallel() tests := []struct { - name string - input string - wantPath string - wantEdits []Edit - wantErr bool + name string + input string + wantPath string + wantEdits []Edit + wantErr bool + wantErrMsg string }{ { name: "normal array edits", @@ -296,19 +297,22 @@ func TestEditFileArgs_UnmarshalJSON(t *testing.T) { }, }, { - name: "invalid JSON", - input: `not json at all`, - wantErr: true, + name: "invalid JSON", + input: `not json at all`, + wantErr: true, + wantErrMsg: "invalid character", }, { - name: "edits is neither array nor string", - input: `{"path": "test.txt", "edits": 42}`, - wantErr: true, + name: "edits is neither array nor string", + input: `{"path": "test.txt", "edits": 42}`, + wantErr: true, + wantErrMsg: "edits field is neither an array nor a JSON string", }, { - name: "double-serialized but inner JSON is invalid", - input: `{"path": "test.txt", "edits": "not valid json"}`, - wantErr: true, + name: "double-serialized but inner JSON is invalid", + input: `{"path": "test.txt", "edits": "not valid json"}`, + wantErr: true, + wantErrMsg: "failed to parse double-serialized edits string", }, { name: "missing edits field (partial/streaming args)", @@ -342,7 +346,8 @@ func TestEditFileArgs_UnmarshalJSON(t *testing.T) { var args EditFileArgs err := json.Unmarshal([]byte(tc.input), &args) if tc.wantErr { - assert.Error(t, err) + require.Error(t, err) + assert.Contains(t, err.Error(), tc.wantErrMsg) return } require.NoError(t, err)