fix(coderd/x/chatd/chattool): make edit_files schema and errors actionable for models (#28121)

## Problem Chat `45b87e40-ffe7-49e5-8932-5fd0bdb9e542` on dev.coder.com failed 57 of 75 `edit_files` tool calls. Every failure was the same: the model omitted `files[].path` (it batched edits per file but only filled in `edits`), and the error relayed back to the model was: ``` POST http://[fd7a:115c:...]:4/api/v0/edit-files: unexpected status code 400: "path" is required ``` The model retried the identical malformed call dozens of times. Two gaps made this sticky: 1. The `edit_files` input schema had no field descriptions, so `path` was only a bare required property. 2. The agent API error reached the model wrapped in HTTP transport noise (method, internal tailnet URL, status code) with no indication of which `files` entry was broken. ## Changes - Add `description` tags to every `edit_files` schema field and state the path requirement in the tool description. - Validate `files` entries in the tool before plan-turn checks and the workspace connection lookup, returning entry-indexed errors such as `files[1].path is required; provide the absolute path of the file to edit; no files in this batch were applied`. - Relay agent API failures with `Message`, `Helper`, `Detail`, and `Validations` from `codersdk.Error` instead of the raw transport-prefixed string. ## Validation - `go test ./coderd/x/chatd/chattool` passes; new tests cover the schema description, entry-indexed validation errors, and transport-noise stripping (each verified red-green by toggling the fix off). - `go build ./...`, `go vet`, and pre-commit (fmt + lint) pass. > Mux created this PR on Mike's behalf.
2026-09-24 15:04:27 +08:00 · 2026-08-13 19:57:58 +02:00
parent e1fa247e59
commit 48e1e28638
2 changed files with 138 additions and 24 deletions
@@ -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()
+}
@@ -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)