mirror of
https://github.com/coder/coder.git
synced 2026-06-02 20:48:20 +00:00
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.
This commit is contained in:
committed by
GitHub
parent
c801dcbbc8
commit
32ed9f1f39
@@ -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 {
|
||||
|
||||
@@ -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()
|
||||
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<typeof searchReplaceSchema>;
|
||||
// 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<string, unknown>;
|
||||
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.
|
||||
*/
|
||||
|
||||
Reference in New Issue
Block a user