diff --git a/coderd/x/chatd/chattool/editfiles.go b/coderd/x/chatd/chattool/editfiles.go index 4ebe07c626170..91e66aef0542c 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:"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."` } // 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,21 +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. 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. 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 } } @@ -142,20 +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] - + for _, file := range args.Files { hasPlanFileName := looksLikePlanFileName(file.Path) if hasPlanFileName && !isAbsolutePath(file.Path) { return fantasy.NewTextErrorResponse( @@ -181,10 +193,32 @@ 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 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() + } + var sb strings.Builder + _, _ = sb.WriteString(sdkErr.Message) + if sdkErr.Helper != "" { + _, _ = sb.WriteString(": " + sdkErr.Helper) + } + if sdkErr.Detail != "" { + _, _ = sb.WriteString(": " + sdkErr.Detail) + } + for _, v := range sdkErr.Validations { + _, _ = sb.WriteString("\n- " + v.Field + ": " + v.Detail) + } + return sb.String() +} diff --git a/coderd/x/chatd/chattool/editfiles_test.go b/coderd/x/chatd/chattool/editfiles_test.go index a9a0a43a3a838..38097f6ac14fa 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" ) @@ -50,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) @@ -58,6 +66,78 @@ func TestEditFiles(t *testing.T) { assert.NotContains(t, editRequired, "replace_all", "replace_all should be optional") }) + t.Run("MalformedEntriesReturnEntryIndexedErrors", func(t *testing.T) { + t.Parallel() + 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", + }, + { + 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: 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) { + 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"` + 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)) + + 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\": Use an absolute path.: some detail\n- path: must be absolute", resp.Content) + }) + t.Run("PlanTurnRejectsNonPlanPath", func(t *testing.T) { t.Parallel() ctrl := gomock.NewController(t)