Edit tool bug - ambigous edits can lead to file corrupting #1261
Description
I think there's a bug with the edit tool.
Overall I think the first first occurence of } was found and the last of return mainLayout and it kinda grepped almost the whole file? Either that of the ambiguity of the grep? cause there were several instances of this "oldstring"
Not really sure of the cause.
Session to checkout: https://opencode.ai/s/KleBY92l
Here's the call the LLM did on tui.go
<invoke name="edit">
<parameter name="filePath">packages/tui/internal/tui/tui.go</parameter>
<parameter name="oldString"> }
return mainLayout</parameter>
<parameter name="newString"> }
// Add minimap overlay
if a.minimap.IsVisible() {
minimapView := a.minimap.View()
minimapX := a.width - a.minimap.Width() - 2 // Position on the right
minimapY := 0
mainLayout = layout.PlaceOverlay(
minimapX,
minimapY,
minimapView,
mainLayout,
)
}
return mainLayout</parameter>
</invoke>
How opencode analyzed it
Looking at the edit.ts file and the example from tui.go, yes, there is a potential for file corruption when replaceAll: false (or not specified) and the string is ambiguous.
The issue is in the replace function at lines 449-477. When replaceAll is false, the code has this logic:
const lastIndex = content.lastIndexOf(search)
if (index !== lastIndex) continue // Skip if multiple occurrences found
However, this check only works for the SimpleReplacer. The other replacers (like LineTrimmedReplacer, BlockAnchorReplacer, etc.) can yield the same match multiple times or yield different variations of
the same logical match.
For your example string " }\n\n return mainLayout" in the Go file:
- Multiple replacers could match: Different replacers might find slightly different versions of this text (with different whitespace handling)
- Same replacer, multiple matches: If this exact pattern appears multiple times in the file, some replacers might not detect the ambiguity properly
- Inconsistent ambiguity detection: The index !== lastIndex check only works when the exact same string is found multiple times, but replacers can yield different string variations that represent the
same logical content
Potential corruption scenarios:
• If two similar but slightly different blocks exist, a replacer might match one variation, pass the ambiguity check, but replace the wrong occurrence
• The replacers iterate in order, so the first successful match wins, which might not be the intended target
• Whitespace differences could cause the wrong block to be selected
The safer approach would be:
- Always validate that exactly one logical match exists across all replacers before making any replacement
- Or require users to provide more specific/unique strings when replaceAll: false
- Or improve the ambiguity detection to work across all replacer types, not just exact string matches
claude-sonnet-4-20250514 (11:48 PM)
Activity