fix(tools): guide recovery from ambiguous edits
- commit
- 623e758da25a4b145e99adb1a47c21fb28838721
- author
- brice · · 2026-08-18 19:27 UTC
- committer
- brice · · 2026-08-18 19:57 UTC
- parent
- e791659
- signature
- GPG Verified
CD88F9E564E0102646FB6077A1A07C7C7C3E349D· arch-gpg Verified for the committer email
2 files · +144 -29
M packages/agent/tools/edit.go +80 -20
| @@ -31,11 +31,11 @@ type editArgs struct { | |||
| 31 | 31 | Edits []editOp `json:"edits"` | |
| 32 | 32 | } | |
| 33 | 33 | ||
| 34 | - | 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"]}` | |
| 34 | + | 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 with 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"]}` | |
| 35 | 35 | ||
| 36 | 36 | func (t *EditTool) Name() string { return "edit" } | |
| 37 | 37 | func (t *EditTool) Description() string { | |
| 38 | - | 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." | |
| 38 | + | return "Apply exact substitutions to an existing file. Inspect that file before editing and take every oldText directly from its current contents. Use a verbatim excerpt with enough surrounding text to identify exactly one location; choose write when replacing most or all of a file." | |
| 39 | 39 | } | |
| 40 | 40 | func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) } | |
| 41 | 41 | ||
| @@ -98,36 +98,41 @@ func (t *EditTool) plan(raw json.RawMessage) (editPlan, error) { | |||
| 98 | 98 | orig = orig[3:] | |
| 99 | 99 | } | |
| 100 | 100 | nl := detectLineEnding(orig) | |
| 101 | - | // Normalize to \n for matching. | |
| 102 | - | body := string(bytes.ReplaceAll(orig, []byte("\r\n"), []byte("\n"))) | |
| 101 | + | // Normalize the file and edit operands to \n for matching and replacement. | |
| 102 | + | body := normalizeLineEndings(string(orig)) | |
| 103 | 103 | ||
| 104 | - | // Validate all edits first (against original content, not sequentially). | |
| 104 | + | // Validate all edits against the original content and build their spans in | |
| 105 | + | // the same pass. This keeps validation and application anchored to the same | |
| 106 | + | // match instead of locating each oldText a second time. | |
| 107 | + | type span struct { | |
| 108 | + | start, end int | |
| 109 | + | replacement string | |
| 110 | + | } | |
| 111 | + | spans := make([]span, 0, len(a.Edits)) | |
| 105 | 112 | for i, e := range a.Edits { | |
| 106 | - | if e.OldText == "" { | |
| 113 | + | oldText := normalizeLineEndings(e.OldText) | |
| 114 | + | newText := normalizeLineEndings(e.NewText) | |
| 115 | + | if oldText == "" { | |
| 107 | 116 | return editPlan{}, fmt.Errorf("edit %d: oldText must not be empty", i+1) | |
| 108 | 117 | } | |
| 109 | - | if e.OldText == e.NewText { | |
| 118 | + | if oldText == newText { | |
| 110 | 119 | return editPlan{}, fmt.Errorf("edit %d: oldText equals newText", i+1) | |
| 111 | 120 | } | |
| 112 | - | count := strings.Count(body, e.OldText) | |
| 113 | - | if count == 0 { | |
| 121 | + | matches := summarizeMatches(body, oldText) | |
| 122 | + | if matches.count == 0 { | |
| 114 | 123 | return editPlan{}, 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) | |
| 115 | 124 | } | |
| 116 | - | if count > 1 { | |
| 117 | - | return editPlan{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path) | |
| 125 | + | if matches.count > 1 { | |
| 126 | + | return editPlan{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s at %s; read %s with a bounded context window around those lines, then retry with a verbatim excerpt containing enough surrounding text to identify exactly one location", i+1, matches.count, a.Path, formatMatchLines(matches.lines, matches.count), a.Path) | |
| 118 | 127 | } | |
| 128 | + | spans = append(spans, span{ | |
| 129 | + | start: matches.start, | |
| 130 | + | end: matches.start + len(oldText), | |
| 131 | + | replacement: newText, | |
| 132 | + | }) | |
| 119 | 133 | } | |
| 120 | 134 | ||
| 121 | 135 | // Apply atomically (sorted by position so offsets stay valid as we splice). | |
| 122 | - | type span struct { | |
| 123 | - | start, end int | |
| 124 | - | replacement string | |
| 125 | - | } | |
| 126 | - | spans := make([]span, 0, len(a.Edits)) | |
| 127 | - | for _, e := range a.Edits { | |
| 128 | - | idx := strings.Index(body, e.OldText) | |
| 129 | - | spans = append(spans, span{start: idx, end: idx + len(e.OldText), replacement: e.NewText}) | |
| 130 | - | } | |
| 131 | 136 | // Check for overlaps. | |
| 132 | 137 | for i := 0; i < len(spans); i++ { | |
| 133 | 138 | for j := i + 1; j < len(spans); j++ { | |
| @@ -170,6 +175,61 @@ func (t *EditTool) plan(raw json.RawMessage) (editPlan, error) { | |||
| 170 | 175 | return editPlan{path: path, final: final, result: result}, nil | |
| 171 | 176 | } | |
| 172 | 177 | ||
| 178 | + | const maxReportedMatchLines = 5 | |
| 179 | + | ||
| 180 | + | type matchSummary struct { | |
| 181 | + | count int | |
| 182 | + | start int | |
| 183 | + | lines []int | |
| 184 | + | } | |
| 185 | + | ||
| 186 | + | // summarizeMatches counts every possible match start, including overlaps, | |
| 187 | + | // while retaining only the line numbers useful for recovery guidance. | |
| 188 | + | func summarizeMatches(body, oldText string) matchSummary { | |
| 189 | + | var summary matchSummary | |
| 190 | + | for searchFrom := 0; searchFrom < len(body); { | |
| 191 | + | relative := strings.Index(body[searchFrom:], oldText) | |
| 192 | + | if relative < 0 { | |
| 193 | + | break | |
| 194 | + | } | |
| 195 | + | start := searchFrom + relative | |
| 196 | + | if summary.count == 0 { | |
| 197 | + | summary.start = start | |
| 198 | + | } | |
| 199 | + | summary.count++ | |
| 200 | + | if len(summary.lines) < maxReportedMatchLines { | |
| 201 | + | summary.lines = append(summary.lines, 1+strings.Count(body[:start], "\n")) | |
| 202 | + | } | |
| 203 | + | searchFrom = start + 1 | |
| 204 | + | } | |
| 205 | + | return summary | |
| 206 | + | } | |
| 207 | + | ||
| 208 | + | func formatMatchLines(lines []int, count int) string { | |
| 209 | + | parts := make([]string, len(lines)) | |
| 210 | + | for i, line := range lines { | |
| 211 | + | parts[i] = fmt.Sprintf("%d", line) | |
| 212 | + | } | |
| 213 | + | ||
| 214 | + | var list string | |
| 215 | + | switch len(parts) { | |
| 216 | + | case 1: | |
| 217 | + | list = parts[0] | |
| 218 | + | case 2: | |
| 219 | + | list = parts[0] + " and " + parts[1] | |
| 220 | + | default: | |
| 221 | + | list = strings.Join(parts[:len(parts)-1], ", ") + ", and " + parts[len(parts)-1] | |
| 222 | + | } | |
| 223 | + | if omitted := count - len(lines); omitted > 0 { | |
| 224 | + | list += fmt.Sprintf(" (and %d more)", omitted) | |
| 225 | + | } | |
| 226 | + | return "lines " + list | |
| 227 | + | } | |
| 228 | + | ||
| 229 | + | func normalizeLineEndings(s string) string { | |
| 230 | + | return strings.ReplaceAll(s, "\r\n", "\n") | |
| 231 | + | } | |
| 232 | + | ||
| 173 | 233 | func detectLineEnding(b []byte) string { | |
| 174 | 234 | if bytes.Contains(b, []byte("\r\n")) { | |
| 175 | 235 | return "\r\n" | |
| @@ -31,11 +31,11 @@ type editArgs struct { | |||||
| 31 | Edits []editOp `json:"edits"` | 31 | Edits []editOp `json:"edits"` | ||
| 32 | } | 32 | } | ||
| 33 | 33 | ||||
| 34 | - | 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"]}` | 34 | + | 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 with 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"]}` |
| 35 | 35 | ||||
| 36 | func (t *EditTool) Name() string { return "edit" } | 36 | func (t *EditTool) Name() string { return "edit" } | ||
| 37 | func (t *EditTool) Description() string { | 37 | func (t *EditTool) Description() string { | ||
| 38 | - | 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." | 38 | + | return "Apply exact substitutions to an existing file. Inspect that file before editing and take every oldText directly from its current contents. Use a verbatim excerpt with enough surrounding text to identify exactly one location; choose write when replacing most or all of a file." |
| 39 | } | 39 | } | ||
| 40 | func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) } | 40 | func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) } | ||
| 41 | 41 | ||||
| @@ -98,36 +98,41 @@ func (t *EditTool) plan(raw json.RawMessage) (editPlan, error) { | |||||
| 98 | orig = orig[3:] | 98 | orig = orig[3:] | ||
| 99 | } | 99 | } | ||
| 100 | nl := detectLineEnding(orig) | 100 | nl := detectLineEnding(orig) | ||
| 101 | - | // Normalize to \n for matching. | 101 | + | // Normalize the file and edit operands to \n for matching and replacement. |
| 102 | - | body := string(bytes.ReplaceAll(orig, []byte("\r\n"), []byte("\n"))) | 102 | + | body := normalizeLineEndings(string(orig)) |
| 103 | 103 | ||||
| 104 | - | // Validate all edits first (against original content, not sequentially). | 104 | + | // Validate all edits against the original content and build their spans in |
| 105 | + | // the same pass. This keeps validation and application anchored to the same | |||
| 106 | + | // match instead of locating each oldText a second time. | |||
| 107 | + | type span struct { | |||
| 108 | + | start, end int | |||
| 109 | + | replacement string | |||
| 110 | + | } | |||
| 111 | + | spans := make([]span, 0, len(a.Edits)) | |||
| 105 | for i, e := range a.Edits { | 112 | for i, e := range a.Edits { | ||
| 106 | - | if e.OldText == "" { | 113 | + | oldText := normalizeLineEndings(e.OldText) |
| 114 | + | newText := normalizeLineEndings(e.NewText) | |||
| 115 | + | if oldText == "" { | |||
| 107 | return editPlan{}, fmt.Errorf("edit %d: oldText must not be empty", i+1) | 116 | return editPlan{}, fmt.Errorf("edit %d: oldText must not be empty", i+1) | ||
| 108 | } | 117 | } | ||
| 109 | - | if e.OldText == e.NewText { | 118 | + | if oldText == newText { |
| 110 | return editPlan{}, fmt.Errorf("edit %d: oldText equals newText", i+1) | 119 | return editPlan{}, fmt.Errorf("edit %d: oldText equals newText", i+1) | ||
| 111 | } | 120 | } | ||
| 112 | - | count := strings.Count(body, e.OldText) | 121 | + | matches := summarizeMatches(body, oldText) |
| 113 | - | if count == 0 { | 122 | + | if matches.count == 0 { |
| 114 | return editPlan{}, 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) | 123 | return editPlan{}, 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) | ||
| 115 | } | 124 | } | ||
| 116 | - | if count > 1 { | 125 | + | if matches.count > 1 { |
| 117 | - | return editPlan{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path) | 126 | + | return editPlan{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s at %s; read %s with a bounded context window around those lines, then retry with a verbatim excerpt containing enough surrounding text to identify exactly one location", i+1, matches.count, a.Path, formatMatchLines(matches.lines, matches.count), a.Path) |
| 118 | } | 127 | } | ||
| 128 | + | spans = append(spans, span{ | |||
| 129 | + | start: matches.start, | |||
| 130 | + | end: matches.start + len(oldText), | |||
| 131 | + | replacement: newText, | |||
| 132 | + | }) | |||
| 119 | } | 133 | } | ||
| 120 | 134 | ||||
| 121 | // Apply atomically (sorted by position so offsets stay valid as we splice). | 135 | // Apply atomically (sorted by position so offsets stay valid as we splice). | ||
| 122 | - | type span struct { | |||
| 123 | - | start, end int | |||
| 124 | - | replacement string | |||
| 125 | - | } | |||
| 126 | - | spans := make([]span, 0, len(a.Edits)) | |||
| 127 | - | for _, e := range a.Edits { | |||
| 128 | - | idx := strings.Index(body, e.OldText) | |||
| 129 | - | spans = append(spans, span{start: idx, end: idx + len(e.OldText), replacement: e.NewText}) | |||
| 130 | - | } | |||
| 131 | // Check for overlaps. | 136 | // Check for overlaps. | ||
| 132 | for i := 0; i < len(spans); i++ { | 137 | for i := 0; i < len(spans); i++ { | ||
| 133 | for j := i + 1; j < len(spans); j++ { | 138 | for j := i + 1; j < len(spans); j++ { | ||
| @@ -170,6 +175,61 @@ func (t *EditTool) plan(raw json.RawMessage) (editPlan, error) { | |||||
| 170 | return editPlan{path: path, final: final, result: result}, nil | 175 | return editPlan{path: path, final: final, result: result}, nil | ||
| 171 | } | 176 | } | ||
| 172 | 177 | ||||
| 178 | + | const maxReportedMatchLines = 5 | |||
| 179 | + | ||||
| 180 | + | type matchSummary struct { | |||
| 181 | + | count int | |||
| 182 | + | start int | |||
| 183 | + | lines []int | |||
| 184 | + | } | |||
| 185 | + | ||||
| 186 | + | // summarizeMatches counts every possible match start, including overlaps, | |||
| 187 | + | // while retaining only the line numbers useful for recovery guidance. | |||
| 188 | + | func summarizeMatches(body, oldText string) matchSummary { | |||
| 189 | + | var summary matchSummary | |||
| 190 | + | for searchFrom := 0; searchFrom < len(body); { | |||
| 191 | + | relative := strings.Index(body[searchFrom:], oldText) | |||
| 192 | + | if relative < 0 { | |||
| 193 | + | break | |||
| 194 | + | } | |||
| 195 | + | start := searchFrom + relative | |||
| 196 | + | if summary.count == 0 { | |||
| 197 | + | summary.start = start | |||
| 198 | + | } | |||
| 199 | + | summary.count++ | |||
| 200 | + | if len(summary.lines) < maxReportedMatchLines { | |||
| 201 | + | summary.lines = append(summary.lines, 1+strings.Count(body[:start], "\n")) | |||
| 202 | + | } | |||
| 203 | + | searchFrom = start + 1 | |||
| 204 | + | } | |||
| 205 | + | return summary | |||
| 206 | + | } | |||
| 207 | + | ||||
| 208 | + | func formatMatchLines(lines []int, count int) string { | |||
| 209 | + | parts := make([]string, len(lines)) | |||
| 210 | + | for i, line := range lines { | |||
| 211 | + | parts[i] = fmt.Sprintf("%d", line) | |||
| 212 | + | } | |||
| 213 | + | ||||
| 214 | + | var list string | |||
| 215 | + | switch len(parts) { | |||
| 216 | + | case 1: | |||
| 217 | + | list = parts[0] | |||
| 218 | + | case 2: | |||
| 219 | + | list = parts[0] + " and " + parts[1] | |||
| 220 | + | default: | |||
| 221 | + | list = strings.Join(parts[:len(parts)-1], ", ") + ", and " + parts[len(parts)-1] | |||
| 222 | + | } | |||
| 223 | + | if omitted := count - len(lines); omitted > 0 { | |||
| 224 | + | list += fmt.Sprintf(" (and %d more)", omitted) | |||
| 225 | + | } | |||
| 226 | + | return "lines " + list | |||
| 227 | + | } | |||
| 228 | + | ||||
| 229 | + | func normalizeLineEndings(s string) string { | |||
| 230 | + | return strings.ReplaceAll(s, "\r\n", "\n") | |||
| 231 | + | } | |||
| 232 | + | ||||
| 173 | func detectLineEnding(b []byte) string { | 233 | func detectLineEnding(b []byte) string { | ||
| 174 | if bytes.Contains(b, []byte("\r\n")) { | 234 | if bytes.Contains(b, []byte("\r\n")) { | ||
| 175 | return "\r\n" | 235 | return "\r\n" | ||
M packages/agent/tools/tools_test.go +64 -9
| @@ -184,10 +184,13 @@ func TestEditMultiple(t *testing.T) { | |||
| 184 | 184 | } | |
| 185 | 185 | } | |
| 186 | 186 | ||
| 187 | - | func TestEditAmbiguous(t *testing.T) { | |
| 187 | + | func TestEditAmbiguousGuidesRecovery(t *testing.T) { | |
| 188 | 188 | dir := t.TempDir() | |
| 189 | 189 | p := filepath.Join(dir, "a.txt") | |
| 190 | - | os.WriteFile(p, []byte("x\nx\n"), 0o644) | |
| 190 | + | original := []byte("x\nx\nx\nx\nx\nx\nx\n") | |
| 191 | + | if err := os.WriteFile(p, original, 0o644); err != nil { | |
| 192 | + | t.Fatal(err) | |
| 193 | + | } | |
| 191 | 194 | tool := &EditTool{CWD: dir} | |
| 192 | 195 | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | |
| 193 | 196 | "path": "a.txt", | |
| @@ -196,11 +199,52 @@ func TestEditAmbiguous(t *testing.T) { | |||
| 196 | 199 | if err == nil { | |
| 197 | 200 | t.Fatal("want ambiguous error") | |
| 198 | 201 | } | |
| 202 | + | for _, want := range []string{ | |
| 203 | + | "oldText matches 7 times", | |
| 204 | + | "lines 1, 2, 3, 4, and 5 (and 2 more)", | |
| 205 | + | "read a.txt with a bounded context window around those lines", | |
| 206 | + | "retry with a verbatim excerpt containing enough surrounding text to identify exactly one location", | |
| 207 | + | } { | |
| 208 | + | if !strings.Contains(err.Error(), want) { | |
| 209 | + | t.Errorf("error missing %q: %q", want, err) | |
| 210 | + | } | |
| 211 | + | } | |
| 212 | + | got, readErr := os.ReadFile(p) | |
| 213 | + | if readErr != nil { | |
| 214 | + | t.Fatal(readErr) | |
| 215 | + | } | |
| 216 | + | if string(got) != string(original) { | |
| 217 | + | t.Fatalf("ambiguous edit modified file: got %q, want %q", got, original) | |
| 218 | + | } | |
| 219 | + | } | |
| 220 | + | ||
| 221 | + | func TestEditRejectsOverlappingMatches(t *testing.T) { | |
| 222 | + | dir := t.TempDir() | |
| 223 | + | p := filepath.Join(dir, "a.txt") | |
| 224 | + | original := []byte("aaa\n") | |
| 225 | + | if err := os.WriteFile(p, original, 0o644); err != nil { | |
| 226 | + | t.Fatal(err) | |
| 227 | + | } | |
| 228 | + | tool := &EditTool{CWD: dir} | |
| 229 | + | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | |
| 230 | + | "path": "a.txt", | |
| 231 | + | "edits": []map[string]any{{"oldText": "aa", "newText": "b"}}, | |
| 232 | + | }), nil) | |
| 233 | + | if err == nil || !strings.Contains(err.Error(), "oldText matches 2 times") { | |
| 234 | + | t.Fatalf("want overlapping-match error, got %v", err) | |
| 235 | + | } | |
| 236 | + | got, readErr := os.ReadFile(p) | |
| 237 | + | if readErr != nil { | |
| 238 | + | t.Fatal(readErr) | |
| 239 | + | } | |
| 240 | + | if string(got) != string(original) { | |
| 241 | + | t.Fatalf("ambiguous edit modified file: got %q, want %q", got, original) | |
| 242 | + | } | |
| 199 | 243 | } | |
| 200 | 244 | ||
| 201 | 245 | func TestEditGuidance(t *testing.T) { | |
| 202 | 246 | desc := (&EditTool{}).Description() | |
| 203 | - | for _, want := range []string{"Inspect that file", "directly from its current contents", "short excerpts", "write"} { | |
| 247 | + | for _, want := range []string{"Inspect that file", "directly from its current contents", "verbatim excerpt", "enough surrounding text", "write"} { | |
| 204 | 248 | if !strings.Contains(desc, want) { | |
| 205 | 249 | t.Errorf("description missing %q: %q", want, desc) | |
| 206 | 250 | } | |
| @@ -215,8 +259,11 @@ func TestEditGuidance(t *testing.T) { | |||
| 215 | 259 | items := edits["items"].(map[string]any) | |
| 216 | 260 | editProperties := items["properties"].(map[string]any) | |
| 217 | 261 | oldText := editProperties["oldText"].(map[string]any) | |
| 218 | - | if got, _ := oldText["description"].(string); !strings.Contains(got, "file being modified") { | |
| 219 | - | t.Fatalf("oldText schema description missing target-file guidance: %q", got) | |
| 262 | + | oldTextDescription, _ := oldText["description"].(string) | |
| 263 | + | for _, want := range []string{"file being modified", "verbatim excerpt", "enough surrounding text", "exactly one location"} { | |
| 264 | + | if !strings.Contains(oldTextDescription, want) { | |
| 265 | + | t.Errorf("oldText schema description missing %q: %q", want, oldTextDescription) | |
| 266 | + | } | |
| 220 | 267 | } | |
| 221 | 268 | } | |
| 222 | 269 | ||
| @@ -244,16 +291,24 @@ func TestEditNotFoundGuidesRecovery(t *testing.T) { | |||
| 244 | 291 | func TestEditPreservesCRLF(t *testing.T) { | |
| 245 | 292 | dir := t.TempDir() | |
| 246 | 293 | p := filepath.Join(dir, "a.txt") | |
| 247 | - | os.WriteFile(p, []byte("hello\r\nworld\r\n"), 0o644) | |
| 294 | + | if err := os.WriteFile(p, []byte("hello\r\nworld\r\n"), 0o644); err != nil { | |
| 295 | + | t.Fatal(err) | |
| 296 | + | } | |
| 248 | 297 | tool := &EditTool{CWD: dir} | |
| 249 | 298 | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | |
| 250 | - | "path": "a.txt", | |
| 251 | - | "edits": []map[string]any{{"oldText": "world", "newText": "gopher"}}, | |
| 299 | + | "path": "a.txt", | |
| 300 | + | "edits": []map[string]any{{ | |
| 301 | + | "oldText": "hello\r\nworld", | |
| 302 | + | "newText": "hello\r\ngopher", | |
| 303 | + | }}, | |
| 252 | 304 | }), nil) | |
| 253 | 305 | if err != nil { | |
| 254 | 306 | t.Fatal(err) | |
| 255 | 307 | } | |
| 256 | - | b, _ := os.ReadFile(p) | |
| 308 | + | b, err := os.ReadFile(p) | |
| 309 | + | if err != nil { | |
| 310 | + | t.Fatal(err) | |
| 311 | + | } | |
| 257 | 312 | if string(b) != "hello\r\ngopher\r\n" { | |
| 258 | 313 | t.Fatalf("got %q", string(b)) | |
| 259 | 314 | } | |
| @@ -184,10 +184,13 @@ func TestEditMultiple(t *testing.T) { | |||||
| 184 | } | 184 | } | ||
| 185 | } | 185 | } | ||
| 186 | 186 | ||||
| 187 | - | func TestEditAmbiguous(t *testing.T) { | 187 | + | func TestEditAmbiguousGuidesRecovery(t *testing.T) { |
| 188 | dir := t.TempDir() | 188 | dir := t.TempDir() | ||
| 189 | p := filepath.Join(dir, "a.txt") | 189 | p := filepath.Join(dir, "a.txt") | ||
| 190 | - | os.WriteFile(p, []byte("x\nx\n"), 0o644) | 190 | + | original := []byte("x\nx\nx\nx\nx\nx\nx\n") |
| 191 | + | if err := os.WriteFile(p, original, 0o644); err != nil { | |||
| 192 | + | t.Fatal(err) | |||
| 193 | + | } | |||
| 191 | tool := &EditTool{CWD: dir} | 194 | tool := &EditTool{CWD: dir} | ||
| 192 | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | 195 | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | ||
| 193 | "path": "a.txt", | 196 | "path": "a.txt", | ||
| @@ -196,11 +199,52 @@ func TestEditAmbiguous(t *testing.T) { | |||||
| 196 | if err == nil { | 199 | if err == nil { | ||
| 197 | t.Fatal("want ambiguous error") | 200 | t.Fatal("want ambiguous error") | ||
| 198 | } | 201 | } | ||
| 202 | + | for _, want := range []string{ | |||
| 203 | + | "oldText matches 7 times", | |||
| 204 | + | "lines 1, 2, 3, 4, and 5 (and 2 more)", | |||
| 205 | + | "read a.txt with a bounded context window around those lines", | |||
| 206 | + | "retry with a verbatim excerpt containing enough surrounding text to identify exactly one location", | |||
| 207 | + | } { | |||
| 208 | + | if !strings.Contains(err.Error(), want) { | |||
| 209 | + | t.Errorf("error missing %q: %q", want, err) | |||
| 210 | + | } | |||
| 211 | + | } | |||
| 212 | + | got, readErr := os.ReadFile(p) | |||
| 213 | + | if readErr != nil { | |||
| 214 | + | t.Fatal(readErr) | |||
| 215 | + | } | |||
| 216 | + | if string(got) != string(original) { | |||
| 217 | + | t.Fatalf("ambiguous edit modified file: got %q, want %q", got, original) | |||
| 218 | + | } | |||
| 219 | + | } | |||
| 220 | + | ||||
| 221 | + | func TestEditRejectsOverlappingMatches(t *testing.T) { | |||
| 222 | + | dir := t.TempDir() | |||
| 223 | + | p := filepath.Join(dir, "a.txt") | |||
| 224 | + | original := []byte("aaa\n") | |||
| 225 | + | if err := os.WriteFile(p, original, 0o644); err != nil { | |||
| 226 | + | t.Fatal(err) | |||
| 227 | + | } | |||
| 228 | + | tool := &EditTool{CWD: dir} | |||
| 229 | + | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | |||
| 230 | + | "path": "a.txt", | |||
| 231 | + | "edits": []map[string]any{{"oldText": "aa", "newText": "b"}}, | |||
| 232 | + | }), nil) | |||
| 233 | + | if err == nil || !strings.Contains(err.Error(), "oldText matches 2 times") { | |||
| 234 | + | t.Fatalf("want overlapping-match error, got %v", err) | |||
| 235 | + | } | |||
| 236 | + | got, readErr := os.ReadFile(p) | |||
| 237 | + | if readErr != nil { | |||
| 238 | + | t.Fatal(readErr) | |||
| 239 | + | } | |||
| 240 | + | if string(got) != string(original) { | |||
| 241 | + | t.Fatalf("ambiguous edit modified file: got %q, want %q", got, original) | |||
| 242 | + | } | |||
| 199 | } | 243 | } | ||
| 200 | 244 | ||||
| 201 | func TestEditGuidance(t *testing.T) { | 245 | func TestEditGuidance(t *testing.T) { | ||
| 202 | desc := (&EditTool{}).Description() | 246 | desc := (&EditTool{}).Description() | ||
| 203 | - | for _, want := range []string{"Inspect that file", "directly from its current contents", "short excerpts", "write"} { | 247 | + | for _, want := range []string{"Inspect that file", "directly from its current contents", "verbatim excerpt", "enough surrounding text", "write"} { |
| 204 | if !strings.Contains(desc, want) { | 248 | if !strings.Contains(desc, want) { | ||
| 205 | t.Errorf("description missing %q: %q", want, desc) | 249 | t.Errorf("description missing %q: %q", want, desc) | ||
| 206 | } | 250 | } | ||
| @@ -215,8 +259,11 @@ func TestEditGuidance(t *testing.T) { | |||||
| 215 | items := edits["items"].(map[string]any) | 259 | items := edits["items"].(map[string]any) | ||
| 216 | editProperties := items["properties"].(map[string]any) | 260 | editProperties := items["properties"].(map[string]any) | ||
| 217 | oldText := editProperties["oldText"].(map[string]any) | 261 | oldText := editProperties["oldText"].(map[string]any) | ||
| 218 | - | if got, _ := oldText["description"].(string); !strings.Contains(got, "file being modified") { | 262 | + | oldTextDescription, _ := oldText["description"].(string) |
| 219 | - | t.Fatalf("oldText schema description missing target-file guidance: %q", got) | 263 | + | for _, want := range []string{"file being modified", "verbatim excerpt", "enough surrounding text", "exactly one location"} { |
| 264 | + | if !strings.Contains(oldTextDescription, want) { | |||
| 265 | + | t.Errorf("oldText schema description missing %q: %q", want, oldTextDescription) | |||
| 266 | + | } | |||
| 220 | } | 267 | } | ||
| 221 | } | 268 | } | ||
| 222 | 269 | ||||
| @@ -244,16 +291,24 @@ func TestEditNotFoundGuidesRecovery(t *testing.T) { | |||||
| 244 | func TestEditPreservesCRLF(t *testing.T) { | 291 | func TestEditPreservesCRLF(t *testing.T) { | ||
| 245 | dir := t.TempDir() | 292 | dir := t.TempDir() | ||
| 246 | p := filepath.Join(dir, "a.txt") | 293 | p := filepath.Join(dir, "a.txt") | ||
| 247 | - | os.WriteFile(p, []byte("hello\r\nworld\r\n"), 0o644) | 294 | + | if err := os.WriteFile(p, []byte("hello\r\nworld\r\n"), 0o644); err != nil { |
| 295 | + | t.Fatal(err) | |||
| 296 | + | } | |||
| 248 | tool := &EditTool{CWD: dir} | 297 | tool := &EditTool{CWD: dir} | ||
| 249 | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | 298 | _, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{ | ||
| 250 | - | "path": "a.txt", | 299 | + | "path": "a.txt", |
| 251 | - | "edits": []map[string]any{{"oldText": "world", "newText": "gopher"}}, | 300 | + | "edits": []map[string]any{{ |
| 301 | + | "oldText": "hello\r\nworld", | |||
| 302 | + | "newText": "hello\r\ngopher", | |||
| 303 | + | }}, | |||
| 252 | }), nil) | 304 | }), nil) | ||
| 253 | if err != nil { | 305 | if err != nil { | ||
| 254 | t.Fatal(err) | 306 | t.Fatal(err) | ||
| 255 | } | 307 | } | ||
| 256 | - | b, _ := os.ReadFile(p) | 308 | + | b, err := os.ReadFile(p) |
| 309 | + | if err != nil { | |||
| 310 | + | t.Fatal(err) | |||
| 311 | + | } | |||
| 257 | if string(b) != "hello\r\ngopher\r\n" { | 312 | if string(b) != "hello\r\ngopher\r\n" { | ||
| 258 | t.Fatalf("got %q", string(b)) | 313 | t.Fatalf("got %q", string(b)) | ||
| 259 | } | 314 | } | ||