From 32ed9f1f39d263b2ba730d8ebbbff1397c1595f2 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Tue, 26 May 2026 11:11:47 +0300 Subject: [PATCH] fix: use old_text/new_text in edit_files tool schema (#25658) Models frequently confuse the search and replace fields in the edit_files tool (CODAGT-312). Rename the model-facing JSON fields to old_text/new_text so the intent is unambiguous. Backend: custom UnmarshalJSON on editFileEdit falls back to deprecated search/replace when old_text/new_text are empty. The workspace agent API is unchanged; toSDKFiles maps old_text/new_text back to search/replace for agent/agentfiles. Frontend: normalizeEdit in parseEditFilesArgs accepts both old_text/new_text and search/replace, normalizing to the internal { search, replace } representation so streaming diff rendering works with either field naming convention. --- coderd/x/chatd/chattool/editfiles.go | 78 ++++++++- coderd/x/chatd/chattool/editfiles_test.go | 148 ++++++++++++++++++ .../ChatElements/tools/utils.test.ts | 51 ++++++ .../components/ChatElements/tools/utils.ts | 39 +++-- 4 files changed, 297 insertions(+), 19 deletions(-) diff --git a/coderd/x/chatd/chattool/editfiles.go b/coderd/x/chatd/chattool/editfiles.go index 51518c1c9c..1c1c584c40 100644 --- a/coderd/x/chatd/chattool/editfiles.go +++ b/coderd/x/chatd/chattool/editfiles.go @@ -2,6 +2,7 @@ package chattool import ( "context" + "encoding/json" "strings" "charm.land/fantasy" @@ -15,19 +16,80 @@ type EditFilesOptions struct { IsPlanTurn bool } +// EditFilesArgs is the tool input schema, auto-generated by the +// fantasy framework from these struct tags. type EditFilesArgs struct { - Files []workspacesdk.FileEdits `json:"files"` + Files []editFileEdits `json:"files"` +} + +type editFileEdits struct { + Path string `json:"path"` + Edits []editFileEdit `json:"edits"` +} + +// editFileEdit uses "old_text"/"new_text" instead of "search"/"replace" +// because models confused the direction (CODAGT-312). Deprecated +// "search"/"replace" accepted via UnmarshalJSON; toSDKFiles maps back +// to "search"/"replace" for agent/agentfiles. +type editFileEdit struct { + OldText string `json:"old_text"` + NewText string `json:"new_text"` + ReplaceAll bool `json:"replace_all,omitempty"` +} + +// UnmarshalJSON falls back to deprecated "search"/"replace" when +// "old_text"/"new_text" are empty. +func (e *editFileEdit) UnmarshalJSON(data []byte) error { + var raw struct { + OldText string `json:"old_text"` + Search string `json:"search"` + NewText string `json:"new_text"` + Replace string `json:"replace"` + ReplaceAll bool `json:"replace_all"` + } + if err := json.Unmarshal(data, &raw); err != nil { + return err + } + e.OldText = raw.OldText + if e.OldText == "" { + e.OldText = raw.Search + } + e.NewText = raw.NewText + if e.NewText == "" { + e.NewText = raw.Replace + } + e.ReplaceAll = raw.ReplaceAll + return nil +} + +func (a EditFilesArgs) toSDKFiles() []workspacesdk.FileEdits { + files := make([]workspacesdk.FileEdits, len(a.Files)) + for i, f := range a.Files { + edits := make([]workspacesdk.FileEdit, len(f.Edits)) + for j, e := range f.Edits { + edits[j] = workspacesdk.FileEdit{ + Search: e.OldText, + Replace: e.NewText, + ReplaceAll: e.ReplaceAll, + } + } + files[i] = workspacesdk.FileEdits{ + Path: f.Path, + Edits: edits, + } + } + return files } func EditFiles(options EditFilesOptions) fantasy.AgentTool { return fantasy.NewAgentTool( "edit_files", - "Perform search-and-replace edits on one or more files. Matching"+ - " is fuzzy (tolerates whitespace and indentation differences) and"+ - " preserves the file's existing indentation and line endings."+ - " Errors if search matches zero locations, or more than one unless"+ - " replace_all is set. All edits in a batch are validated before any"+ - " file is written.", + "Perform edits on one or more files by replacing old_text with"+ + " new_text. Matching is fuzzy (tolerates whitespace and indentation"+ + " differences) and preserves the file's existing indentation and"+ + " line endings. Errors if old_text matches zero locations, or more"+ + " than one unless replace_all is set. All edits in a batch are"+ + " validated before any file is written.", func(ctx context.Context, args EditFilesArgs, _ fantasy.ToolCall) (fantasy.ToolResponse, error) { var planPath string if options.IsPlanTurn && len(args.Files) > 0 { @@ -101,7 +163,7 @@ func executeEditFilesTool( } resp, err := conn.EditFiles(ctx, workspacesdk.FileEditRequest{ - Files: args.Files, + Files: args.toSDKFiles(), IncludeDiff: true, }) if err != nil { diff --git a/coderd/x/chatd/chattool/editfiles_test.go b/coderd/x/chatd/chattool/editfiles_test.go index d025d1ca4b..2cafdfe796 100644 --- a/coderd/x/chatd/chattool/editfiles_test.go +++ b/coderd/x/chatd/chattool/editfiles_test.go @@ -20,6 +20,44 @@ import ( func TestEditFiles(t *testing.T) { t.Parallel() + // Verify the generated tool schema exposes old_text/new_text + // (not the deprecated search/replace) so the rename is + // auditable without running a separate program. + t.Run("SchemaUsesOldTextNewText", func(t *testing.T) { + t.Parallel() + tool := chattool.EditFiles(chattool.EditFilesOptions{}) + info := tool.Info() + + // Dig into: files -> items -> properties -> edits -> items -> properties + filesSchema := info.Parameters["files"] + require.NotNil(t, filesSchema, "missing files parameter") + filesMap, ok := filesSchema.(map[string]any) + require.True(t, ok) + items, ok := filesMap["items"].(map[string]any) + require.True(t, ok) + props, ok := items["properties"].(map[string]any) + require.True(t, ok) + editsSchema, ok := props["edits"].(map[string]any) + require.True(t, ok) + editItems, ok := editsSchema["items"].(map[string]any) + require.True(t, ok) + editProps, ok := editItems["properties"].(map[string]any) + require.True(t, ok) + + assert.Contains(t, editProps, "old_text", "schema should expose old_text") + assert.Contains(t, editProps, "new_text", "schema should expose new_text") + assert.Contains(t, editProps, "replace_all", "schema should expose replace_all") + assert.NotContains(t, editProps, "search", "schema should not expose deprecated search") + assert.NotContains(t, editProps, "replace", "schema should not expose deprecated replace") + + // Verify required fields. + editRequired, ok := editItems["required"].([]string) + require.True(t, ok) + assert.Contains(t, editRequired, "old_text") + assert.Contains(t, editRequired, "new_text") + assert.NotContains(t, editRequired, "replace_all", "replace_all should be optional") + }) + t.Run("PlanTurnRejectsNonPlanPath", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) @@ -470,6 +508,116 @@ func TestEditFiles(t *testing.T) { }) } +func TestEditFiles_OldTextNewTextFieldsPreferred(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + targetPath := "/home/coder/main.go" + + // The agent API should map old_text->Search and new_text->Replace. + mockConn.EXPECT(). + EditFiles(gomock.Any(), workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: targetPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old content", + Replace: "new content", + }}, + }}, + IncludeDiff: true, + }). + Return(workspacesdk.FileEditResponse{}, nil) + + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"` + targetPath + `","edits":[{"old_text":"old content","new_text":"new content"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) +} + +func TestEditFiles_DeprecatedSearchReplaceFieldsStillWork(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + targetPath := "/home/coder/main.go" + + // Agents with cached schemas may still send "search"/"replace". + // Also exercises replace_all through the new unmarshal+convert path. + mockConn.EXPECT(). + EditFiles(gomock.Any(), workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: targetPath, + Edits: []workspacesdk.FileEdit{{ + Search: "old", + Replace: "replacement", + ReplaceAll: true, + }}, + }}, + IncludeDiff: true, + }). + Return(workspacesdk.FileEditResponse{}, nil) + + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"` + targetPath + `","edits":[{"search":"old","replace":"replacement","replace_all":true}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) +} + +func TestEditFiles_NewFieldNamesTakePrecedenceOverOld(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + targetPath := "/home/coder/main.go" + + // If both old and new field names are present, new names win. + mockConn.EXPECT(). + EditFiles(gomock.Any(), workspacesdk.FileEditRequest{ + Files: []workspacesdk.FileEdits{{ + Path: targetPath, + Edits: []workspacesdk.FileEdit{{ + Search: "from-oldText", + Replace: "from-newText", + }}, + }}, + IncludeDiff: true, + }). + Return(workspacesdk.FileEditResponse{}, nil) + + tool := chattool.EditFiles(chattool.EditFilesOptions{ + GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { + return mockConn, nil + }, + }) + + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: `{"files":[{"path":"` + targetPath + `","edits":[{"old_text":"from-oldText","search":"from-search","new_text":"from-newText","replace":"from-replace"}]}]}`, + }) + require.NoError(t, err) + assert.False(t, resp.IsError) +} + func TestEditFiles_ToolResponseCarriesFileResults(t *testing.T) { t.Parallel() diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts index 2d7dd80228..b0dd5384d8 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.test.ts @@ -763,6 +763,57 @@ describe("parseEditFilesArgs", () => { expect(parsed[0].edits[0].replace).toBe(""); }); + it("accepts old_text/new_text field names", () => { + const args = { + files: [ + { + path: "a.ts", + edits: [{ old_text: "before", new_text: "after" }], + }, + ], + }; + const result = parseEditFilesArgs(args); + expect(result).toHaveLength(1); + expect(result[0].edits).toHaveLength(1); + expect(result[0].edits[0]).toEqual({ search: "before", replace: "after" }); + }); + + it("prefers old_text/new_text over search/replace when both present", () => { + const args = { + files: [ + { + path: "a.ts", + edits: [ + { + old_text: "from-old-text", + new_text: "from-new-text", + search: "from-search", + replace: "from-replace", + }, + ], + }, + ], + }; + const result = parseEditFilesArgs(args); + expect(result[0].edits[0]).toEqual({ + search: "from-old-text", + replace: "from-new-text", + }); + }); + + it("preserves deletion via old_text/new_text (empty new_text)", () => { + const args = { + files: [ + { + path: "a.ts", + edits: [{ old_text: "remove me", new_text: "" }], + }, + ], + }; + const result = parseEditFilesArgs(args); + expect(result[0].edits[0]).toEqual({ search: "remove me", replace: "" }); + }); + // During streaming the model may emit a file entry before any // edit is complete. Every edit has a missing replace, so all are // filtered out. The file entry survives with an empty edits diff --git a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts index 8c74d5874a..8e5595f15f 100644 --- a/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts +++ b/site/src/pages/AgentsPage/components/ChatElements/tools/utils.ts @@ -12,12 +12,29 @@ export interface EditFilesFileEntry { edits: Array<{ search: string; replace: string }>; } -const searchReplaceSchema = Yup.object({ - search: Yup.string().required(), - replace: Yup.string().defined(), -}).required(); - -type SearchReplace = Yup.InferType; +// Validates that the edit has at least the shape of an object with +// string-typed text fields. Accepts both current field names +// (old_text/new_text) and deprecated names (search/replace). +const normalizeEdit = ( + e: unknown, +): { search: string; replace: string } | null => { + if (typeof e !== "object" || e === null) return null; + const raw = e as Record; + const search = + typeof raw.old_text === "string" + ? raw.old_text + : typeof raw.search === "string" + ? raw.search + : null; + const replace = + typeof raw.new_text === "string" + ? raw.new_text + : typeof raw.replace === "string" + ? raw.replace + : null; + if (!search || replace === null) return null; + return { search, replace }; +}; const fileEntrySchema = Yup.object({ path: Yup.string().required(), @@ -688,15 +705,15 @@ export const parseEditFilesArgs = (args: unknown): EditFilesFileEntry[] => { .filter((f): f is FileEntry => isValid(fileEntrySchema, f)) .map((f) => ({ path: f.path, - edits: f.edits.filter((e): e is SearchReplace => - isValid(searchReplaceSchema, e), - ), + edits: f.edits + .map(normalizeEdit) + .filter((e): e is { search: string; replace: string } => e !== null), })); }; /** - * Builds a synthetic unified diff from search/replace edit pairs - * for a single file. Each edit becomes a separate + * Builds a synthetic unified diff from edit pairs (normalized to + * search/replace) for a single file. Each edit becomes a separate * `Diff.createPatch` call; the patches are concatenated and * parsed into a single FileDiffMetadata. */