Merge pull request #74 from frkr/fix/editproperly
Edit file does not work properly #73
3 files · +47 -4
M packages/agent/systemprompt.go +1 -1
| @@ -86,4 +86,4 @@ const defaultIdentity = `You are an expert coding assistant operating inside zot | |||
| 86 | 86 | ||
| 87 | 87 | Your output renders in a TUI that understands markdown for prose and plain text for tool output. Use markdown freely, keep answers concise, and let tool calls speak for themselves rather than narrating them in prose before you invoke them. Act first, then summarise what you did. | |
| 88 | 88 | ||
| 89 | - | When changing file contents, prefer the edit tool for in-place changes and the write tool for creating or fully replacing files. Do not use bash with cat/echo/sed/tee redirections to mutate files; those changes render as opaque shell output while edit renders as a readable diff.` | |
| 89 | + | For focused changes to an existing file, inspect its current contents and use edit with verbatim oldText taken from that same file. Include only enough context to make each match unambiguous. Use write when creating a file or replacing it wholesale. Do not mutate files through bash redirections or commands such as cat, echo, sed, or tee, because those changes appear as opaque shell output instead of a readable edit diff.` | |
| @@ -86,4 +86,4 @@ const defaultIdentity = `You are an expert coding assistant operating inside zot | |||||
| 86 | 86 | ||||
| 87 | Your output renders in a TUI that understands markdown for prose and plain text for tool output. Use markdown freely, keep answers concise, and let tool calls speak for themselves rather than narrating them in prose before you invoke them. Act first, then summarise what you did. | 87 | Your output renders in a TUI that understands markdown for prose and plain text for tool output. Use markdown freely, keep answers concise, and let tool calls speak for themselves rather than narrating them in prose before you invoke them. Act first, then summarise what you did. | ||
| 88 | 88 | ||||
| 89 | - | When changing file contents, prefer the edit tool for in-place changes and the write tool for creating or fully replacing files. Do not use bash with cat/echo/sed/tee redirections to mutate files; those changes render as opaque shell output while edit renders as a readable diff.` | 89 | + | For focused changes to an existing file, inspect its current contents and use edit with verbatim oldText taken from that same file. Include only enough context to make each match unambiguous. Use write when creating a file or replacing it wholesale. Do not mutate files through bash redirections or commands such as cat, echo, sed, or tee, because those changes appear as opaque shell output instead of a readable edit diff.` |
M packages/agent/tools/edit.go +3 -3
| @@ -28,11 +28,11 @@ type editArgs struct { | |||
| 28 | 28 | Edits []editOp `json:"edits"` | |
| 29 | 29 | } | |
| 30 | 30 | ||
| 31 | - | const editSchema = `{"type":"object","properties":{"path":{"type":"string"},"edits":{"type":"array","items":{"type":"object","properties":{"oldText":{"type":"string"},"newText":{"type":"string"}},"required":["oldText","newText"]}}},"required":["path","edits"]}` | |
| 31 | + | const editSchema = `{"type":"object","properties":{"path":{"type":"string","description":"File to modify, given as an absolute path or one relative to the working directory."},"edits":{"type":"array","description":"A set of non-overlapping substitutions, all located in the file content as it existed before this call.","items":{"type":"object","properties":{"oldText":{"type":"string","description":"A verbatim excerpt from the file being modified. Include enough surrounding text to identify exactly one location, without intersecting another edit."},"newText":{"type":"string","description":"Content that will replace the matched excerpt."}},"required":["oldText","newText"]}}},"required":["path","edits"]}` | |
| 32 | 32 | ||
| 33 | 33 | func (t *EditTool) Name() string { return "edit" } | |
| 34 | 34 | func (t *EditTool) Description() string { | |
| 35 | - | return "Edit a file via exact-match replacements. Each oldText must be unique in the file." | |
| 35 | + | return "Apply exact substitutions to an existing file. Inspect that file before editing and take every oldText directly from its current contents. Use short excerpts that identify one location; choose write when replacing most or all of a file." | |
| 36 | 36 | } | |
| 37 | 37 | func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) } | |
| 38 | 38 | ||
| @@ -77,7 +77,7 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu | |||
| 77 | 77 | } | |
| 78 | 78 | count := strings.Count(body, e.OldText) | |
| 79 | 79 | if count == 0 { | |
| 80 | - | return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s", i+1, a.Path) | |
| 80 | + | return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s; inspect that file again and use a verbatim excerpt with matching spaces and line breaks", i+1, a.Path) | |
| 81 | 81 | } | |
| 82 | 82 | if count > 1 { | |
| 83 | 83 | return core.ToolResult{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path) | |
| @@ -28,11 +28,11 @@ type editArgs struct { | |||||
| 28 | Edits []editOp `json:"edits"` | 28 | Edits []editOp `json:"edits"` | ||
| 29 | } | 29 | } | ||
| 30 | 30 | ||||
| 31 | - | const editSchema = `{"type":"object","properties":{"path":{"type":"string"},"edits":{"type":"array","items":{"type":"object","properties":{"oldText":{"type":"string"},"newText":{"type":"string"}},"required":["oldText","newText"]}}},"required":["path","edits"]}` | 31 | + | const editSchema = `{"type":"object","properties":{"path":{"type":"string","description":"File to modify, given as an absolute path or one relative to the working directory."},"edits":{"type":"array","description":"A set of non-overlapping substitutions, all located in the file content as it existed before this call.","items":{"type":"object","properties":{"oldText":{"type":"string","description":"A verbatim excerpt from the file being modified. Include enough surrounding text to identify exactly one location, without intersecting another edit."},"newText":{"type":"string","description":"Content that will replace the matched excerpt."}},"required":["oldText","newText"]}}},"required":["path","edits"]}` |
| 32 | 32 | ||||
| 33 | func (t *EditTool) Name() string { return "edit" } | 33 | func (t *EditTool) Name() string { return "edit" } | ||
| 34 | func (t *EditTool) Description() string { | 34 | func (t *EditTool) Description() string { | ||
| 35 | - | return "Edit a file via exact-match replacements. Each oldText must be unique in the file." | 35 | + | return "Apply exact substitutions to an existing file. Inspect that file before editing and take every oldText directly from its current contents. Use short excerpts that identify one location; choose write when replacing most or all of a file." |
| 36 | } | 36 | } | ||
| 37 | func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) } | 37 | func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) } | ||
| 38 | 38 | ||||
| @@ -77,7 +77,7 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu | |||||
| 77 | } | 77 | } | ||
| 78 | count := strings.Count(body, e.OldText) | 78 | count := strings.Count(body, e.OldText) | ||
| 79 | if count == 0 { | 79 | if count == 0 { | ||
| 80 | - | return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s", i+1, a.Path) | 80 | + | return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s; inspect that file again and use a verbatim excerpt with matching spaces and line breaks", i+1, a.Path) |
| 81 | } | 81 | } | ||
| 82 | if count > 1 { | 82 | if count > 1 { | ||
| 83 | return core.ToolResult{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path) | 83 | return core.ToolResult{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path) | ||
M packages/agent/tools/tools_test.go +43 -0
| @@ -158,6 +158,49 @@ func TestEditAmbiguous(t *testing.T) { | |||
| 158 | 158 | } | |
| 159 | 159 | } | |
| 160 | 160 | ||
| 161 | + | func TestEditGuidance(t *testing.T) { | |
| 162 | + | desc := (&EditTool{}).Description() | |
| 163 | + | for _, want := range []string{"Inspect that file", "directly from its current contents", "short excerpts", "write"} { | |
| 164 | + | if !strings.Contains(desc, want) { | |
| 165 | + | t.Errorf("description missing %q: %q", want, desc) | |
| 166 | + | } | |
| 167 | + | } | |
| 168 | + | ||
| 169 | + | var schema map[string]any | |
| 170 | + | if err := json.Unmarshal((&EditTool{}).Schema(), &schema); err != nil { | |
| 171 | + | t.Fatal(err) | |
| 172 | + | } | |
| 173 | + | properties := schema["properties"].(map[string]any) | |
| 174 | + | edits := properties["edits"].(map[string]any) | |
| 175 | + | items := edits["items"].(map[string]any) | |
| 176 | + | editProperties := items["properties"].(map[string]any) | |
| 177 | + | oldText := editProperties["oldText"].(map[string]any) | |
| 178 | + | if got, _ := oldText["description"].(string); !strings.Contains(got, "file being modified") { | |
| 179 | + | t.Fatalf("oldText schema description missing target-file guidance: %q", got) | |
| 180 | + | } | |
| 181 | + | } | |
| 182 | + | ||
| 183 | + | func TestEditNotFoundGuidesRecovery(t *testing.T) { | |
| 184 | + | dir := t.TempDir() | |
| 185 | + | p := filepath.Join(dir, "destination.txt") | |
| 186 | + | if err := os.WriteFile(p, []byte("destination content\n"), 0o644); err != nil { | |
| 187 | + | t.Fatal(err) | |
| 188 | + | } | |
| 189 | + | tool := &EditTool{CWD: dir} | |
| 190 | + | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | |
| 191 | + | "path": "destination.txt", | |
| 192 | + | "edits": []map[string]any{{"oldText": "content from another file", "newText": "replacement"}}, | |
| 193 | + | }), nil) | |
| 194 | + | if err == nil { | |
| 195 | + | t.Fatal("want oldText not found error") | |
| 196 | + | } | |
| 197 | + | for _, want := range []string{"oldText not found", "destination.txt", "inspect that file again", "matching spaces and line breaks"} { | |
| 198 | + | if !strings.Contains(err.Error(), want) { | |
| 199 | + | t.Errorf("error missing %q: %q", want, err) | |
| 200 | + | } | |
| 201 | + | } | |
| 202 | + | } | |
| 203 | + | ||
| 161 | 204 | func TestEditPreservesCRLF(t *testing.T) { | |
| 162 | 205 | dir := t.TempDir() | |
| 163 | 206 | p := filepath.Join(dir, "a.txt") | |
| @@ -158,6 +158,49 @@ func TestEditAmbiguous(t *testing.T) { | |||||
| 158 | } | 158 | } | ||
| 159 | } | 159 | } | ||
| 160 | 160 | ||||
| 161 | + | func TestEditGuidance(t *testing.T) { | |||
| 162 | + | desc := (&EditTool{}).Description() | |||
| 163 | + | for _, want := range []string{"Inspect that file", "directly from its current contents", "short excerpts", "write"} { | |||
| 164 | + | if !strings.Contains(desc, want) { | |||
| 165 | + | t.Errorf("description missing %q: %q", want, desc) | |||
| 166 | + | } | |||
| 167 | + | } | |||
| 168 | + | ||||
| 169 | + | var schema map[string]any | |||
| 170 | + | if err := json.Unmarshal((&EditTool{}).Schema(), &schema); err != nil { | |||
| 171 | + | t.Fatal(err) | |||
| 172 | + | } | |||
| 173 | + | properties := schema["properties"].(map[string]any) | |||
| 174 | + | edits := properties["edits"].(map[string]any) | |||
| 175 | + | items := edits["items"].(map[string]any) | |||
| 176 | + | editProperties := items["properties"].(map[string]any) | |||
| 177 | + | oldText := editProperties["oldText"].(map[string]any) | |||
| 178 | + | if got, _ := oldText["description"].(string); !strings.Contains(got, "file being modified") { | |||
| 179 | + | t.Fatalf("oldText schema description missing target-file guidance: %q", got) | |||
| 180 | + | } | |||
| 181 | + | } | |||
| 182 | + | ||||
| 183 | + | func TestEditNotFoundGuidesRecovery(t *testing.T) { | |||
| 184 | + | dir := t.TempDir() | |||
| 185 | + | p := filepath.Join(dir, "destination.txt") | |||
| 186 | + | if err := os.WriteFile(p, []byte("destination content\n"), 0o644); err != nil { | |||
| 187 | + | t.Fatal(err) | |||
| 188 | + | } | |||
| 189 | + | tool := &EditTool{CWD: dir} | |||
| 190 | + | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | |||
| 191 | + | "path": "destination.txt", | |||
| 192 | + | "edits": []map[string]any{{"oldText": "content from another file", "newText": "replacement"}}, | |||
| 193 | + | }), nil) | |||
| 194 | + | if err == nil { | |||
| 195 | + | t.Fatal("want oldText not found error") | |||
| 196 | + | } | |||
| 197 | + | for _, want := range []string{"oldText not found", "destination.txt", "inspect that file again", "matching spaces and line breaks"} { | |||
| 198 | + | if !strings.Contains(err.Error(), want) { | |||
| 199 | + | t.Errorf("error missing %q: %q", want, err) | |||
| 200 | + | } | |||
| 201 | + | } | |||
| 202 | + | } | |||
| 203 | + | ||||
| 161 | func TestEditPreservesCRLF(t *testing.T) { | 204 | func TestEditPreservesCRLF(t *testing.T) { | ||
| 162 | dir := t.TempDir() | 205 | dir := t.TempDir() | ||
| 163 | p := filepath.Join(dir, "a.txt") | 206 | p := filepath.Join(dir, "a.txt") | ||