fix: Edit file does not work properly

#73

commit: 93967bc7cb22a2d22a8688cf4be025df62faa764
author: DaviSM · 3mo ago · 2026-07-11 03:07 UTC
parent: dbd0dfa
signature: unsigned

2 files · +121 -14

M packages/agent/tools/edit.go +25 -14


@@ -32,7 +32,7 @@ const editSchema = `{"type":"object","properties":{"path":{"type":"string"},"edi
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 "Edit a file via exact-match replacements. Each oldText must be unique in the file. NEVER pass text from a different file as oldText. Prefer small unique blocks; for large or highly divergent rewrites, use write instead."
36	36		}
37	37		func (t *EditTool) Schema() json.RawMessage { return json.RawMessage(editSchema) }
38	38

@@ -68,13 +68,19 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu
68	68		body := string(bytes.ReplaceAll(orig, []byte("\r\n"), []byte("\n")))
69	69
70	70		// Validate all edits first (against original content, not sequentially).
	71	+	// oldText == newText is a no-op (destination already has the desired
	72	+	// text) — skip those rather than erroring so equalize/port flows can
	73	+	// continue when a block is already in sync.
	74	+	type span struct {
	75	+	start, end int
	76	+	replacement string
	77	+	}
	78	+	spans := make([]span, 0, len(a.Edits))
	79	+	applied := 0
71	80		for i, e := range a.Edits {
72	81		if e.OldText == "" {
73	82		return core.ToolResult{}, fmt.Errorf("edit %d: oldText must not be empty", i+1)
74	83		}
75		-	if e.OldText == e.NewText {
76		-	return core.ToolResult{}, fmt.Errorf("edit %d: oldText equals newText", i+1)
77		-	}
78	84		count := strings.Count(body, e.OldText)
79	85		if count == 0 {
80	86		return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s", i+1, a.Path)

@@ -82,18 +88,23 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu
82	88		if count > 1 {
83	89		return core.ToolResult{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path)
84	90		}
85		-	}
86		-
87		-	// Apply atomically (sorted by position so offsets stay valid as we splice).
88		-	type span struct {
89		-	start, end int
90		-	replacement string
91		-	}
92		-	spans := make([]span, 0, len(a.Edits))
93		-	for _, e := range a.Edits {
	91	+	if e.OldText == e.NewText {
	92	+	continue
	93	+	}
94	94		idx := strings.Index(body, e.OldText)
95	95		spans = append(spans, span{start: idx, end: idx + len(e.OldText), replacement: e.NewText})
	96	+	applied++
	97	+	}
	98	+
	99	+	if applied == 0 {
	100	+	// Every edit was already a no-op; file is unchanged.
	101	+	return core.ToolResult{
	102	+	Content: []provider.Content{provider.TextBlock{Text: "already up to date\n"}},
	103	+	Details: map[string]any{"path": path, "edits": 0, "diff": ""},
	104	+	}, nil
96	105		}
	106	+
	107	+	// Apply atomically (sorted by position so offsets stay valid as we splice).
97	108		// Check for overlaps.
98	109		for i := 0; i < len(spans); i++ {
99	110		for j := i + 1; j < len(spans); j++ {

@@ -137,7 +148,7 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu
137	148		// want it.
138	149		return core.ToolResult{
139	150		Content: []provider.Content{provider.TextBlock{Text: diff}},
140		-	Details: map[string]any{"path": path, "edits": len(a.Edits), "diff": diff},
	151	+	Details: map[string]any{"path": path, "edits": applied, "diff": diff},
141	152		}, nil
142	153		}
143	154


@@ -32,7 +32,7 @@ const editSchema = `{"type":"object","properties":{"path":{"type":"string"},"edi
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 "Edit a file via exact-match replacements. Each oldText must be unique in the file. NEVER pass text from a different file as oldText. Prefer small unique blocks; for large or highly divergent rewrites, use write instead."
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

@@ -68,13 +68,19 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu
68		body := string(bytes.ReplaceAll(orig, []byte("\r\n"), []byte("\n")))	68		body := string(bytes.ReplaceAll(orig, []byte("\r\n"), []byte("\n")))
69			69
70		// Validate all edits first (against original content, not sequentially).	70		// Validate all edits first (against original content, not sequentially).
			71	+	// oldText == newText is a no-op (destination already has the desired
			72	+	// text) — skip those rather than erroring so equalize/port flows can
			73	+	// continue when a block is already in sync.
			74	+	type span struct {
			75	+	start, end int
			76	+	replacement string
			77	+	}
			78	+	spans := make([]span, 0, len(a.Edits))
			79	+	applied := 0
71		for i, e := range a.Edits {	80		for i, e := range a.Edits {
72		if e.OldText == "" {	81		if e.OldText == "" {
73		return core.ToolResult{}, fmt.Errorf("edit %d: oldText must not be empty", i+1)	82		return core.ToolResult{}, fmt.Errorf("edit %d: oldText must not be empty", i+1)
74		}	83		}
75	-	if e.OldText == e.NewText {
76	-	return core.ToolResult{}, fmt.Errorf("edit %d: oldText equals newText", i+1)
77	-	}
78		count := strings.Count(body, e.OldText)	84		count := strings.Count(body, e.OldText)
79		if count == 0 {	85		if count == 0 {
80		return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s", i+1, a.Path)	86		return core.ToolResult{}, fmt.Errorf("edit %d: oldText not found in %s", i+1, a.Path)

@@ -82,18 +88,23 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu
82		if count > 1 {	88		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)	89		return core.ToolResult{}, fmt.Errorf("edit %d: oldText matches %d times (must be unique) in %s", i+1, count, a.Path)
84		}	90		}
85	-	}	91	+	if e.OldText == e.NewText {
86	-		92	+	continue
87	-	// Apply atomically (sorted by position so offsets stay valid as we splice).	93	+	}
88	-	type span struct {
89	-	start, end int
90	-	replacement string
91	-	}
92	-	spans := make([]span, 0, len(a.Edits))
93	-	for _, e := range a.Edits {
94		idx := strings.Index(body, e.OldText)	94		idx := strings.Index(body, e.OldText)
95		spans = append(spans, span{start: idx, end: idx + len(e.OldText), replacement: e.NewText})	95		spans = append(spans, span{start: idx, end: idx + len(e.OldText), replacement: e.NewText})
			96	+	applied++
			97	+	}
			98	+
			99	+	if applied == 0 {
			100	+	// Every edit was already a no-op; file is unchanged.
			101	+	return core.ToolResult{
			102	+	Content: []provider.Content{provider.TextBlock{Text: "already up to date\n"}},
			103	+	Details: map[string]any{"path": path, "edits": 0, "diff": ""},
			104	+	}, nil
96		}	105		}
			106	+
			107	+	// Apply atomically (sorted by position so offsets stay valid as we splice).
97		// Check for overlaps.	108		// Check for overlaps.
98		for i := 0; i < len(spans); i++ {	109		for i := 0; i < len(spans); i++ {
99		for j := i + 1; j < len(spans); j++ {	110		for j := i + 1; j < len(spans); j++ {

@@ -137,7 +148,7 @@ func (t *EditTool) Execute(ctx context.Context, raw json.RawMessage, progress fu
137		// want it.	148		// want it.
138		return core.ToolResult{	149		return core.ToolResult{
139		Content: []provider.Content{provider.TextBlock{Text: diff}},	150		Content: []provider.Content{provider.TextBlock{Text: diff}},
140	-	Details: map[string]any{"path": path, "edits": len(a.Edits), "diff": diff},	151	+	Details: map[string]any{"path": path, "edits": applied, "diff": diff},
141		}, nil	152		}, nil
142		}	153		}
143			154

M packages/agent/tools/tools_test.go +96 -0


@@ -158,6 +158,102 @@ func TestEditAmbiguous(t *testing.T) {
158	158		}
159	159		}
160	160
	161	+	// TestEditNoOpWhenAlreadyInSync: oldText == newText means the destination
	162	+	// already has the desired block. That is success, not an error — equalize
	163	+	// flows must keep going.
	164	+	func TestEditNoOpWhenAlreadyInSync(t *testing.T) {
	165	+	dir := t.TempDir()
	166	+	p := filepath.Join(dir, "a.txt")
	167	+	orig := "hello world\n"
	168	+	if err := os.WriteFile(p, []byte(orig), 0o644); err != nil {
	169	+	t.Fatal(err)
	170	+	}
	171	+	tool := &EditTool{CWD: dir}
	172	+	res, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{
	173	+	"path": "a.txt",
	174	+	"edits": []map[string]any{{"oldText": "hello world", "newText": "hello world"}},
	175	+	}), nil)
	176	+	if err != nil {
	177	+	t.Fatalf("no-op edit should succeed: %v", err)
	178	+	}
	179	+	b, _ := os.ReadFile(p)
	180	+	if string(b) != orig {
	181	+	t.Fatalf("file changed on no-op: got %q", b)
	182	+	}
	183	+	got := res.Content[0].(provider.TextBlock).Text
	184	+	if !strings.Contains(got, "already up to date") {
	185	+	t.Fatalf("want already-up-to-date result, got %q", got)
	186	+	}
	187	+	}
	188	+
	189	+	func TestEditSkipsNoOpAmongRealEdits(t *testing.T) {
	190	+	dir := t.TempDir()
	191	+	p := filepath.Join(dir, "a.txt")
	192	+	if err := os.WriteFile(p, []byte("a\nb\nc\n"), 0o644); err != nil {
	193	+	t.Fatal(err)
	194	+	}
	195	+	tool := &EditTool{CWD: dir}
	196	+	_, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{
	197	+	"path": "a.txt",
	198	+	"edits": []map[string]any{
	199	+	{"oldText": "a", "newText": "a"}, // already in sync
	200	+	{"oldText": "c", "newText": "C"},
	201	+	},
	202	+	}), nil)
	203	+	if err != nil {
	204	+	t.Fatal(err)
	205	+	}
	206	+	b, _ := os.ReadFile(p)
	207	+	if string(b) != "a\nb\nC\n" {
	208	+	t.Fatalf("got %q", b)
	209	+	}
	210	+	}
	211	+
	212	+	// TestEditOldTextNotFoundGuidesRecovery covers the equalize-divergent-files
	213	+	// failure mode: oldText taken from a different file than path. The error
	214	+	// must tell the model to re-read the destination and edit in blocks (or write).
	215	+	func TestEditOldTextNotFoundGuidesRecovery(t *testing.T) {
	216	+	dir := t.TempDir()
	217	+	p := filepath.Join(dir, "desktop.ts")
	218	+	if err := os.WriteFile(p, []byte("export const timer: ReturnType<typeof setTimeout> \| null = null\n"), 0o644); err != nil {
	219	+	t.Fatal(err)
	220	+	}
	221	+	tool := &EditTool{CWD: dir}
	222	+	// Content shaped like a sibling "browser" file — not present in desktop.ts.
	223	+	_, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{
	224	+	"path": "desktop.ts",
	225	+	"edits": []map[string]any{{
	226	+	"oldText": "export const timer: number \| null = null;",
	227	+	"newText": "export const timer: ReturnType<typeof setTimeout> \| null = null",
	228	+	}},
	229	+	}), nil)
	230	+	if err == nil {
	231	+	t.Fatal("want oldText not found error")
	232	+	}
	233	+	msg := err.Error()
	234	+	for _, want := range []string{
	235	+	"oldText not found",
	236	+	"desktop.ts",
	237	+	} {
	238	+	if !strings.Contains(msg, want) {
	239	+	t.Errorf("error missing %q; got %q", want, msg)
	240	+	}
	241	+	}
	242	+	}
	243	+
	244	+	func TestEditDescriptionGuidesReadBeforeEdit(t *testing.T) {
	245	+	desc := (&EditTool{}).Description()
	246	+	for _, want := range []string{
	247	+	"different file",
	248	+	"write",
	249	+	"small unique blocks",
	250	+	} {
	251	+	if !strings.Contains(desc, want) {
	252	+	t.Errorf("Description missing %q; got %q", want, desc)
	253	+	}
	254	+	}
	255	+	}
	256	+
161	257		func TestEditPreservesCRLF(t *testing.T) {
162	258		dir := t.TempDir()
163	259		p := filepath.Join(dir, "a.txt")


@@ -158,6 +158,102 @@ func TestEditAmbiguous(t *testing.T) {
158	}	158		}
159	}	159		}
160		160
		161	+	// TestEditNoOpWhenAlreadyInSync: oldText == newText means the destination
		162	+	// already has the desired block. That is success, not an error — equalize
		163	+	// flows must keep going.
		164	+	func TestEditNoOpWhenAlreadyInSync(t *testing.T) {
		165	+	dir := t.TempDir()
		166	+	p := filepath.Join(dir, "a.txt")
		167	+	orig := "hello world\n"
		168	+	if err := os.WriteFile(p, []byte(orig), 0o644); err != nil {
		169	+	t.Fatal(err)
		170	+	}
		171	+	tool := &EditTool{CWD: dir}
		172	+	res, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{
		173	+	"path": "a.txt",
		174	+	"edits": []map[string]any{{"oldText": "hello world", "newText": "hello world"}},
		175	+	}), nil)
		176	+	if err != nil {
		177	+	t.Fatalf("no-op edit should succeed: %v", err)
		178	+	}
		179	+	b, _ := os.ReadFile(p)
		180	+	if string(b) != orig {
		181	+	t.Fatalf("file changed on no-op: got %q", b)
		182	+	}
		183	+	got := res.Content[0].(provider.TextBlock).Text
		184	+	if !strings.Contains(got, "already up to date") {
		185	+	t.Fatalf("want already-up-to-date result, got %q", got)
		186	+	}
		187	+	}
		188	+
		189	+	func TestEditSkipsNoOpAmongRealEdits(t *testing.T) {
		190	+	dir := t.TempDir()
		191	+	p := filepath.Join(dir, "a.txt")
		192	+	if err := os.WriteFile(p, []byte("a\nb\nc\n"), 0o644); err != nil {
		193	+	t.Fatal(err)
		194	+	}
		195	+	tool := &EditTool{CWD: dir}
		196	+	_, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{
		197	+	"path": "a.txt",
		198	+	"edits": []map[string]any{
		199	+	{"oldText": "a", "newText": "a"}, // already in sync
		200	+	{"oldText": "c", "newText": "C"},
		201	+	},
		202	+	}), nil)
		203	+	if err != nil {
		204	+	t.Fatal(err)
		205	+	}
		206	+	b, _ := os.ReadFile(p)
		207	+	if string(b) != "a\nb\nC\n" {
		208	+	t.Fatalf("got %q", b)
		209	+	}
		210	+	}
		211	+
		212	+	// TestEditOldTextNotFoundGuidesRecovery covers the equalize-divergent-files
		213	+	// failure mode: oldText taken from a different file than path. The error
		214	+	// must tell the model to re-read the destination and edit in blocks (or write).
		215	+	func TestEditOldTextNotFoundGuidesRecovery(t *testing.T) {
		216	+	dir := t.TempDir()
		217	+	p := filepath.Join(dir, "desktop.ts")
		218	+	if err := os.WriteFile(p, []byte("export const timer: ReturnType<typeof setTimeout> \| null = null\n"), 0o644); err != nil {
		219	+	t.Fatal(err)
		220	+	}
		221	+	tool := &EditTool{CWD: dir}
		222	+	// Content shaped like a sibling "browser" file — not present in desktop.ts.
		223	+	_, err := tool.Execute(context.Background(), mustJSON(t, map[string]any{
		224	+	"path": "desktop.ts",
		225	+	"edits": []map[string]any{{
		226	+	"oldText": "export const timer: number \| null = null;",
		227	+	"newText": "export const timer: ReturnType<typeof setTimeout> \| null = null",
		228	+	}},
		229	+	}), nil)
		230	+	if err == nil {
		231	+	t.Fatal("want oldText not found error")
		232	+	}
		233	+	msg := err.Error()
		234	+	for _, want := range []string{
		235	+	"oldText not found",
		236	+	"desktop.ts",
		237	+	} {
		238	+	if !strings.Contains(msg, want) {
		239	+	t.Errorf("error missing %q; got %q", want, msg)
		240	+	}
		241	+	}
		242	+	}
		243	+
		244	+	func TestEditDescriptionGuidesReadBeforeEdit(t *testing.T) {
		245	+	desc := (&EditTool{}).Description()
		246	+	for _, want := range []string{
		247	+	"different file",
		248	+	"write",
		249	+	"small unique blocks",
		250	+	} {
		251	+	if !strings.Contains(desc, want) {
		252	+	t.Errorf("Description missing %q; got %q", want, desc)
		253	+	}
		254	+	}
		255	+	}
		256	+
161	func TestEditPreservesCRLF(t *testing.T) {	257		func TestEditPreservesCRLF(t *testing.T) {
162	dir := t.TempDir()	258		dir := t.TempDir()
163	p := filepath.Join(dir, "a.txt")	259		p := filepath.Join(dir, "a.txt")