Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
82 changes: 58 additions & 24 deletions coderd/x/chatd/chattool/editfiles.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)

Expand All @@ -19,22 +21,22 @@ 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"
// 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"`
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
Expand Down Expand Up @@ -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
}
}
Expand Down Expand Up @@ -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(
Expand All @@ -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()
}
80 changes: 80 additions & 0 deletions coderd/x/chatd/chattool/editfiles_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -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)
Expand All @@ -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)
Expand Down
Loading