From e1f71f6c28cd985250b5f21948567047b5285040 Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:33:33 +0000 Subject: [PATCH 1/2] fix(coderd/x/chatd/chattool): make edit_files schema and errors actionable for models A dev.coder.com chat failed 57 consecutive edit_files calls because the model omitted files[].path and the relayed error was an opaque agent API transport error. Describe every schema field, validate entries before calling the agent with entry-indexed messages, and strip HTTP method/URL/status noise from agent API errors. --- coderd/x/chatd/chattool/editfiles.go | 55 +++++++++++--- coderd/x/chatd/chattool/editfiles_test.go | 89 +++++++++++++++++++++++ 2 files changed, 132 insertions(+), 12 deletions(-) diff --git a/coderd/x/chatd/chattool/editfiles.go b/coderd/x/chatd/chattool/editfiles.go index 4ebe07c6261..a790444e20c 100644 --- a/coderd/x/chatd/chattool/editfiles.go +++ b/coderd/x/chatd/chattool/editfiles.go @@ -3,10 +3,12 @@ package chattool import ( "context" "encoding/json" + "fmt" "strings" "charm.land/fantasy" + "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" ) @@ -19,12 +21,12 @@ type EditFilesOptions struct { // EditFilesArgs is the tool input schema, auto-generated by the // fantasy framework from these struct tags. type EditFilesArgs struct { - Files []editFileEdits `json:"files"` + Files []editFileEdits `json:"files" description:"Files to edit. Every entry must include path and at least one edit."` } type editFileEdits struct { - Path string `json:"path"` - Edits []editFileEdit `json:"edits"` + Path string `json:"path" description:"Absolute path of the file to edit (for example /home/coder/project/main.go). Required in every entry, including repeated edits to the same file."` + Edits []editFileEdit `json:"edits" description:"Search and replace operations applied to this file in order."` } // editFileEdit uses "old_text"/"new_text" instead of "search"/"replace" @@ -32,9 +34,9 @@ type editFileEdits struct { // "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"` + OldText string `json:"old_text" description:"Existing text in the file to replace. Matching is fuzzy: whitespace and indentation differences are tolerated."` + NewText string `json:"new_text" description:"Text that replaces old_text."` + ReplaceAll bool `json:"replace_all,omitempty" description:"Replace every match of old_text instead of erroring when it matches more than once."` } // UnmarshalJSON falls back to deprecated "search"/"replace" when @@ -99,11 +101,13 @@ func EditFiles(options EditFilesOptions) fantasy.AgentTool { return fantasy.NewAgentTool( "edit_files", "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.", + " new_text. Every files entry must include the absolute path of"+ + " the file to edit and at least one edit. 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 { @@ -156,6 +160,17 @@ func executeEditFilesTool( args.Files[i].Path = strings.TrimSpace(args.Files[i].Path) file := args.Files[i] + if file.Path == "" { + return fantasy.NewTextErrorResponse(fmt.Sprintf( + "files[%d].path is missing; every files entry must include the absolute path of the file to edit; no files in this batch were applied", i, + )), nil + } + if len(file.Edits) == 0 { + return fantasy.NewTextErrorResponse(fmt.Sprintf( + "files[%d].edits is empty; every files entry must include at least one old_text/new_text edit; no files in this batch were applied", i, + )), nil + } + hasPlanFileName := looksLikePlanFileName(file.Path) if hasPlanFileName && !isAbsolutePath(file.Path) { return fantasy.NewTextErrorResponse( @@ -181,10 +196,26 @@ func executeEditFilesTool( IncludeDiff: true, }) if err != nil { - return fantasy.NewTextErrorResponse(err.Error()), nil + return fantasy.NewTextErrorResponse(agentAPIErrorMessage(err)), nil } return toolResponse(map[string]any{ "ok": true, "files": resp.Files, }), nil } + +// agentAPIErrorMessage extracts the message the model should see from a +// workspace agent API error. codersdk.Error.Error() prefixes the HTTP +// method, internal agent URL, and status code, which the model cannot +// act on and which buries the actual problem. +func agentAPIErrorMessage(err error) string { + sdkErr, ok := codersdk.AsError(err) + if !ok || sdkErr.Message == "" { + return err.Error() + } + msg := sdkErr.Message + if sdkErr.Detail != "" { + msg += ": " + sdkErr.Detail + } + return msg +} diff --git a/coderd/x/chatd/chattool/editfiles_test.go b/coderd/x/chatd/chattool/editfiles_test.go index a9a0a43a3a8..6fa37119704 100644 --- a/coderd/x/chatd/chattool/editfiles_test.go +++ b/coderd/x/chatd/chattool/editfiles_test.go @@ -13,6 +13,7 @@ import ( "golang.org/x/xerrors" "github.com/coder/coder/v2/coderd/x/chatd/chattool" + "github.com/coder/coder/v2/codersdk" "github.com/coder/coder/v2/codersdk/workspacesdk" "github.com/coder/coder/v2/codersdk/workspacesdk/agentconnmock" ) @@ -58,6 +59,94 @@ func TestEditFiles(t *testing.T) { assert.NotContains(t, editRequired, "replace_all", "replace_all should be optional") }) + // Models kept omitting files[].path (dev chat 45b87e40 failed 57 + // consecutive edit_files calls), so the schema must describe the + // field, not just mark it required. + t.Run("SchemaDescribesPath", func(t *testing.T) { + t.Parallel() + tool := chattool.EditFiles(chattool.EditFilesOptions{}) + info := tool.Info() + + filesMap, ok := info.Parameters["files"].(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) + pathSchema, ok := props["path"].(map[string]any) + require.True(t, ok) + desc, _ := pathSchema["description"].(string) + assert.Contains(t, desc, "Absolute path") + }) + + t.Run("MissingPathReturnsEntryIndexedError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + 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":"/home/coder/a.txt","edits":[{"old_text":"old","new_text":"new"}]},` + + `{"edits":[{"old_text":"old","new_text":"new"}]}` + + `]}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, "files[1].path is missing; every files entry must include the absolute path of the file to edit; no files in this batch were applied", resp.Content) + }) + + t.Run("EmptyEditsReturnsEntryIndexedError", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + 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":"/home/coder/a.txt","edits":[]}]}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, "files[0].edits is empty; every files entry must include at least one old_text/new_text edit; no files in this batch were applied", resp.Content) + }) + + t.Run("AgentAPIErrorOmitsTransportNoise", func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + sdkErr := codersdk.NewTestError(http.StatusBadRequest, "POST", "http://[fd7a::1]:4/api/v0/edit-files") + sdkErr.Message = `file path must be absolute: "a.txt"` + mockConn.EXPECT().EditFiles(gomock.Any(), gomock.Any()). + Return(workspacesdk.FileEditResponse{}, xerrors.Errorf("do request: %w", sdkErr)) + + 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":"a.txt","edits":[{"old_text":"old","new_text":"new"}]}]}`, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, `file path must be absolute: "a.txt"`, resp.Content) + }) + t.Run("PlanTurnRejectsNonPlanPath", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t) From 25f1eb405ebb6bbdb037ad039a5e19c6a443c05e Mon Sep 17 00:00:00 2001 From: Michael Suchacz <203725896+ibetitsmike@users.noreply.github.com> Date: Thu, 13 Aug 2026 11:43:45 +0000 Subject: [PATCH 2/2] refactor(coderd/x/chatd/chattool): apply cleanup-gate findings to edit_files Validate entries before plan-turn checks and connection lookup, keep Helper and Validations in relayed agent API errors, and deduplicate the schema and malformed-input tests. --- coderd/x/chatd/chattool/editfiles.go | 69 +++++++------- coderd/x/chatd/chattool/editfiles_test.go | 107 ++++++++++------------ 2 files changed, 85 insertions(+), 91 deletions(-) diff --git a/coderd/x/chatd/chattool/editfiles.go b/coderd/x/chatd/chattool/editfiles.go index a790444e20c..91e66aef054 100644 --- a/coderd/x/chatd/chattool/editfiles.go +++ b/coderd/x/chatd/chattool/editfiles.go @@ -25,7 +25,7 @@ type EditFilesArgs struct { } type editFileEdits struct { - Path string `json:"path" description:"Absolute path of the file to edit (for example /home/coder/project/main.go). Required in every entry, including repeated edits to the same file."` + Path string `json:"path" description:"The absolute path of the file to edit, for example /home/coder/project/main.go."` Edits []editFileEdit `json:"edits" description:"Search and replace operations applied to this file in order."` } @@ -101,23 +101,38 @@ func EditFiles(options EditFilesOptions) fantasy.AgentTool { return fantasy.NewAgentTool( "edit_files", "Perform edits on one or more files by replacing old_text with"+ - " new_text. Every files entry must include the absolute path of"+ - " the file to edit and at least one edit. Matching is fuzzy"+ + " new_text. Each entry in files must include the absolute path"+ + " of the file to edit and at least one edit. 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) { + if len(args.Files) == 0 { + return fantasy.NewTextErrorResponse("files is required"), nil + } + for i := range args.Files { + args.Files[i].Path = strings.TrimSpace(args.Files[i].Path) + if args.Files[i].Path == "" { + return fantasy.NewTextErrorResponse(fmt.Sprintf( + "files[%d].path is required; provide the absolute path of the file to edit; no files in this batch were applied", i, + )), nil + } + if len(args.Files[i].Edits) == 0 { + return fantasy.NewTextErrorResponse(fmt.Sprintf( + "files[%d].edits must contain at least one edit; no files in this batch were applied", i, + )), nil + } + } var planPath string - if options.IsPlanTurn && len(args.Files) > 0 { + if options.IsPlanTurn { resolvedPlanPath, err := resolvePlanTurnPath(ctx, options.ResolvePlanPath) if err != nil { return fantasy.NewTextErrorResponse(err.Error()), nil } - for i := range args.Files { - args.Files[i].Path = strings.TrimSpace(args.Files[i].Path) - if args.Files[i].Path != resolvedPlanPath { + for _, f := range args.Files { + if f.Path != resolvedPlanPath { return fantasy.NewTextErrorResponse("during plan turns, edit_files is restricted to " + resolvedPlanPath), nil } } @@ -146,31 +161,13 @@ func executeEditFilesTool( args EditFilesArgs, resolvePlanPath func(context.Context) (chatPath string, home string, err error), ) (fantasy.ToolResponse, error) { - if len(args.Files) == 0 { - return fantasy.NewTextErrorResponse("files is required"), nil - } - var ( chatPath string home string planPathErr error planPathLoaded bool ) - for i := range args.Files { - args.Files[i].Path = strings.TrimSpace(args.Files[i].Path) - file := args.Files[i] - - if file.Path == "" { - return fantasy.NewTextErrorResponse(fmt.Sprintf( - "files[%d].path is missing; every files entry must include the absolute path of the file to edit; no files in this batch were applied", i, - )), nil - } - if len(file.Edits) == 0 { - return fantasy.NewTextErrorResponse(fmt.Sprintf( - "files[%d].edits is empty; every files entry must include at least one old_text/new_text edit; no files in this batch were applied", i, - )), nil - } - + for _, file := range args.Files { hasPlanFileName := looksLikePlanFileName(file.Path) if hasPlanFileName && !isAbsolutePath(file.Path) { return fantasy.NewTextErrorResponse( @@ -204,18 +201,24 @@ func executeEditFilesTool( }), nil } -// agentAPIErrorMessage extracts the message the model should see from a -// workspace agent API error. codersdk.Error.Error() prefixes the HTTP -// method, internal agent URL, and status code, which the model cannot -// act on and which buries the actual problem. +// agentAPIErrorMessage preserves the agent's actionable message while +// dropping the transport metadata (HTTP method, URL, status code) that +// codersdk.Error.Error() prefixes. func agentAPIErrorMessage(err error) string { sdkErr, ok := codersdk.AsError(err) if !ok || sdkErr.Message == "" { return err.Error() } - msg := sdkErr.Message + var sb strings.Builder + _, _ = sb.WriteString(sdkErr.Message) + if sdkErr.Helper != "" { + _, _ = sb.WriteString(": " + sdkErr.Helper) + } if sdkErr.Detail != "" { - msg += ": " + sdkErr.Detail + _, _ = sb.WriteString(": " + sdkErr.Detail) + } + for _, v := range sdkErr.Validations { + _, _ = sb.WriteString("\n- " + v.Field + ": " + v.Detail) } - return msg + return sb.String() } diff --git a/coderd/x/chatd/chattool/editfiles_test.go b/coderd/x/chatd/chattool/editfiles_test.go index 6fa37119704..38097f6ac14 100644 --- a/coderd/x/chatd/chattool/editfiles_test.go +++ b/coderd/x/chatd/chattool/editfiles_test.go @@ -51,6 +51,13 @@ func TestEditFiles(t *testing.T) { assert.NotContains(t, editProps, "search", "schema should not expose deprecated search") assert.NotContains(t, editProps, "replace", "schema should not expose deprecated replace") + // Requiredness alone did not stop models from omitting path, + // so the schema must also describe it. + pathSchema, ok := props["path"].(map[string]any) + require.True(t, ok) + pathDesc, _ := pathSchema["description"].(string) + assert.Contains(t, pathDesc, "absolute path") + // Verify required fields. editRequired, ok := editItems["required"].([]string) require.True(t, ok) @@ -59,67 +66,48 @@ func TestEditFiles(t *testing.T) { assert.NotContains(t, editRequired, "replace_all", "replace_all should be optional") }) - // Models kept omitting files[].path (dev chat 45b87e40 failed 57 - // consecutive edit_files calls), so the schema must describe the - // field, not just mark it required. - t.Run("SchemaDescribesPath", func(t *testing.T) { + t.Run("MalformedEntriesReturnEntryIndexedErrors", func(t *testing.T) { t.Parallel() - tool := chattool.EditFiles(chattool.EditFilesOptions{}) - info := tool.Info() - - filesMap, ok := info.Parameters["files"].(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) - pathSchema, ok := props["path"].(map[string]any) - require.True(t, ok) - desc, _ := pathSchema["description"].(string) - assert.Contains(t, desc, "Absolute path") - }) - - t.Run("MissingPathReturnsEntryIndexedError", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - mockConn := agentconnmock.NewMockAgentConn(ctrl) - tool := chattool.EditFiles(chattool.EditFilesOptions{ - GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { - return mockConn, nil + cases := []struct { + name string + input string + wantErr string + }{ + { + name: "MissingPath", + input: `{"files":[` + + `{"path":"/home/coder/a.txt","edits":[{"old_text":"old","new_text":"new"}]},` + + `{"edits":[{"old_text":"old","new_text":"new"}]}` + + `]}`, + wantErr: "files[1].path is required; provide the absolute path of the file to edit; no files in this batch were applied", }, - }) - - resp, err := tool.Run(context.Background(), fantasy.ToolCall{ - ID: "call-1", - Name: "edit_files", - Input: `{"files":[` + - `{"path":"/home/coder/a.txt","edits":[{"old_text":"old","new_text":"new"}]},` + - `{"edits":[{"old_text":"old","new_text":"new"}]}` + - `]}`, - }) - require.NoError(t, err) - assert.True(t, resp.IsError) - assert.Equal(t, "files[1].path is missing; every files entry must include the absolute path of the file to edit; no files in this batch were applied", resp.Content) - }) - - t.Run("EmptyEditsReturnsEntryIndexedError", func(t *testing.T) { - t.Parallel() - ctrl := gomock.NewController(t) - mockConn := agentconnmock.NewMockAgentConn(ctrl) - tool := chattool.EditFiles(chattool.EditFilesOptions{ - GetWorkspaceConn: func(context.Context) (workspacesdk.AgentConn, error) { - return mockConn, nil + { + name: "EmptyEdits", + input: `{"files":[{"path":"/home/coder/a.txt","edits":[]}]}`, + wantErr: "files[0].edits must contain at least one edit; no files in this batch were applied", }, - }) + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + ctrl := gomock.NewController(t) + mockConn := agentconnmock.NewMockAgentConn(ctrl) + 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":"/home/coder/a.txt","edits":[]}]}`, - }) - require.NoError(t, err) - assert.True(t, resp.IsError) - assert.Equal(t, "files[0].edits is empty; every files entry must include at least one old_text/new_text edit; no files in this batch were applied", resp.Content) + resp, err := tool.Run(context.Background(), fantasy.ToolCall{ + ID: "call-1", + Name: "edit_files", + Input: tc.input, + }) + require.NoError(t, err) + assert.True(t, resp.IsError) + assert.Equal(t, tc.wantErr, resp.Content) + }) + } }) t.Run("AgentAPIErrorOmitsTransportNoise", func(t *testing.T) { @@ -128,6 +116,9 @@ func TestEditFiles(t *testing.T) { mockConn := agentconnmock.NewMockAgentConn(ctrl) sdkErr := codersdk.NewTestError(http.StatusBadRequest, "POST", "http://[fd7a::1]:4/api/v0/edit-files") sdkErr.Message = `file path must be absolute: "a.txt"` + sdkErr.Helper = "Use an absolute path." + sdkErr.Detail = "some detail" + sdkErr.Validations = []codersdk.ValidationError{{Field: "path", Detail: "must be absolute"}} mockConn.EXPECT().EditFiles(gomock.Any(), gomock.Any()). Return(workspacesdk.FileEditResponse{}, xerrors.Errorf("do request: %w", sdkErr)) @@ -144,7 +135,7 @@ func TestEditFiles(t *testing.T) { }) require.NoError(t, err) assert.True(t, resp.IsError) - assert.Equal(t, `file path must be absolute: "a.txt"`, resp.Content) + assert.Equal(t, "file path must be absolute: \"a.txt\": Use an absolute path.: some detail\n- path: must be absolute", resp.Content) }) t.Run("PlanTurnRejectsNonPlanPath", func(t *testing.T) {