Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions pkg/acp/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,11 @@ func (t *FilesystemToolset) handleEditFile(ctx context.Context, toolCall tools.T
modifiedContent := resp.Content

for i, edit := range args.Edits {
if !strings.Contains(modifiedContent, edit.OldText) {
replaced, ok := builtin.FindAndReplace(modifiedContent, edit.OldText, edit.NewText)
if !ok {
return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil
}
modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1)
modifiedContent = replaced
}

_, err = t.agent.conn.WriteTextFile(ctx, acp.WriteTextFileRequest{
Expand Down
156 changes: 154 additions & 2 deletions pkg/tools/builtin/filesystem.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"regexp"
"strings"
"sync"
"unicode/utf8"

"github.com/docker/docker-agent/pkg/chat"
"github.com/docker/docker-agent/pkg/fsx"
Expand Down Expand Up @@ -441,10 +442,11 @@ func (t *FilesystemTool) handleEditFile(ctx context.Context, args EditFileArgs)

var changes []string
for i, edit := range args.Edits {
if !strings.Contains(modifiedContent, edit.OldText) {
replaced, ok := FindAndReplace(modifiedContent, edit.OldText, edit.NewText)
if !ok {
return tools.ResultError(fmt.Sprintf("Edit %d failed: old text not found", i+1)), nil
}
modifiedContent = strings.Replace(modifiedContent, edit.OldText, edit.NewText, 1)
modifiedContent = replaced
changes = append(changes, fmt.Sprintf("Edit %d: Replaced %d characters", i+1, len(edit.OldText)))
}

Expand All @@ -463,6 +465,156 @@ func (t *FilesystemTool) handleEditFile(ctx context.Context, args EditFileArgs)
return tools.ResultSuccess("File edited successfully. Changes:\n" + strings.Join(changes, "\n")), nil
}

// FindAndReplace tries to find oldText in content and replace the first
// occurrence with newText. It tries an exact match first, then falls back
// through progressively looser normalization strategies to handle common
// LLM mistakes (extra/missing whitespace, collapsed line continuations,
// escaped quotes).
//
// When a fuzzy strategy matches, the replacement is applied to the
// original (un-normalized) content so that surrounding text is preserved.
func FindAndReplace(content, oldText, newText string) (string, bool) {
// Strategy 1: exact match.
if strings.Contains(content, oldText) {
return strings.Replace(content, oldText, newText, 1), true
}

// Strategy 2: line-trimmed match (strip trailing whitespace per line).
if result, ok := normalizedReplace(content, oldText, newText, trimTrailingWhitespacePerLine); ok {
return result, true
}

// Strategy 3: indentation-flexible match (strip leading whitespace per line).
if result, ok := normalizedReplace(content, oldText, newText, stripLeadingWhitespacePerLine); ok {
return result, true
}

// Strategy 4: line-continuation normalization (collapse "\" + newline + spaces into a single space).
if result, ok := normalizedReplace(content, oldText, newText, collapseLineContinuations); ok {
return result, true
}

// Strategy 5: escape-normalized match (\" ↔ ").
if result, ok := normalizedReplace(content, oldText, newText, normalizeEscapedQuotes); ok {
return result, true
}

// Strategy 6: whitespace-collapsed match (collapse all runs of whitespace to single space).
if result, ok := normalizedReplace(content, oldText, newText, collapseWhitespace); ok {
return result, true
}

return content, false
}

// normalizedReplace applies a normalization function to both the content
// and the search text. If the normalized search text is found in the
// normalized content, it locates the corresponding range in the original
// content and performs the replacement there.
func normalizedReplace(content, oldText, newText string, normalize func(string) string) (string, bool) {
normContent := normalize(content)
normOld := normalize(oldText)

if normOld == "" {
return content, false
}

idx := strings.Index(normContent, normOld)
if idx == -1 {
return content, false
}

// Map the normalized indices back to the original content.
origStart := mapNormToOrig(content, idx, normalize)
origEnd := mapNormToOrigEnd(content, idx+len(normOld), normalize)

return content[:origStart] + newText + content[origEnd:], true
}

// mapNormToOrig finds the smallest rune-aligned position in the original
// string whose normalized prefix has at least normIdx characters.
func mapNormToOrig(original string, normIdx int, normalize func(string) string) int {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM: Binary search may overshoot with collapsing normalizations

The binary search finds "the smallest position where the normalized prefix has at least normIdx characters", but this can cause off-by-one errors when normalization collapses characters.

Example:
Normalizing "a b" (two spaces) to "a b" (one space):

  • Looking for position 2 in normalized string (after "a ")
  • Binary search might land at position 3 in original (after "a ")
  • Because normalize("a ") = "a " has length 2

Why LIKELY not CONFIRMED:
The binary search invariant appears correct in principle, but edge cases with collapsing normalizations could expose boundary issues. The real problem is that this function feeds into mapNormToOrigEnd, which has confirmed bugs.

Recommendation:
Add test cases specifically for boundary detection with:

  • Multiple consecutive collapsible characters
  • UTF-8 multi-byte characters adjacent to collapsible sequences

lo, hi := 0, len(original)
for lo < hi {
mid := (lo + hi) / 2
if len(normalize(original[:mid])) < normIdx {
lo = mid + 1
} else {
hi = mid
}
}
// Snap to the start of the rune at position lo to avoid splitting
// a multi-byte UTF-8 character. When lo == len(original) we are past
// the end of the string and no adjustment is needed.
for lo > 0 && lo < len(original) && !utf8.RuneStart(original[lo]) {
lo--
}
return lo
}

// mapNormToOrigEnd is like mapNormToOrig but then advances past any
// trailing characters that were consumed by the normalization (e.g. the
// quote in \" → "). This ensures the entire original text corresponding
// to the normalized match is included. Advances by whole runes to
// preserve UTF-8 integrity.
func mapNormToOrigEnd(original string, normIdx int, normalize func(string) string) int {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH: mapNormToOrigEnd boundary detection logic is incorrect

This function attempts to advance past trailing characters consumed by normalization, but the implementation is fundamentally incorrect for normalizations that map multiple characters to fewer.

Root Cause:
The function uses mapNormToOrig (binary search) to find the starting position, then advances by runes while checking if the normalized length changes. However, this approach doesn't correctly handle character-consuming normalizations like:

  • collapseWhitespace (multiple spaces → one space)
  • collapseLineContinuations (\\ + newline → space)
  • normalizeEscapedQuotes (\\"")

Impact:
Incorrect end boundary detection leads to wrong replacement ranges and potentially corrupted file content.

Recommendation:
Consider a different approach:

  1. Track character-to-character mapping during normalization
  2. Use a forward scan with explicit state tracking
  3. Add comprehensive tests with UTF-8 edge cases

pos := mapNormToOrig(original, normIdx, normalize)
for pos < len(original) {
_, size := utf8.DecodeRuneInString(original[pos:])
if len(normalize(original[:pos+size])) != len(normalize(original[:pos])) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 HIGH: Incorrect termination condition for consumed character detection

The condition len(normalize(original[:pos+size])) != len(normalize(original[:pos])) is fundamentally flawed for character-consuming normalizations.

The Problem:
When normalization collapses multiple characters to fewer (e.g., collapseWhitespace, collapseLineContinuations), advancing by one rune will almost always change the normalized length UNLESS that rune is completely consumed. This logic will fail to correctly identify the end boundary.

Impact:

  • Wrong replacement ranges
  • Potential file corruption when fuzzy matching triggers
  • Especially dangerous with multi-byte UTF-8 characters

Example:
Consider normalizing "a b" (two spaces) to "a b" (one space):

  • At position after 'a', normalized length is 2
  • Advancing one rune (first space), normalized length is still 2 (space collapsed)
  • Advancing another rune (second space), normalized length is still 2
  • The loop will incorrectly continue past the intended boundary

The test suite doesn't cover these edge cases with UTF-8 and character-consuming normalizations.

break
}
pos += size
}
return pos
}

func trimTrailingWhitespacePerLine(s string) string {
lines := strings.Split(s, "\n")
for i, line := range lines {
lines[i] = strings.TrimRight(line, " \t")
}
return strings.Join(lines, "\n")
}

func stripLeadingWhitespacePerLine(s string) string {
lines := strings.Split(s, "\n")
for i, line := range lines {
lines[i] = strings.TrimLeft(line, " \t")
}
return strings.Join(lines, "\n")
}

// lineContinuationRE matches shell-style line continuations: optional
// whitespace, a backslash, optional whitespace, a newline, and optional
// leading whitespace on the next line. This is intentionally
// context-unaware — it does not distinguish continuations inside string
// literals from those in code. This is acceptable because the fuzzy
// strategies are only attempted after an exact match has already failed,
// making a false positive unlikely in practice.
var lineContinuationRE = regexp.MustCompile(`\s*\\\s*\n\s*`)

func collapseLineContinuations(s string) string {
return lineContinuationRE.ReplaceAllString(s, " ")
}

// normalizeEscapedQuotes replaces escaped quotes (\" → ") so that the
// LLM's unescaped version can match the file's escaped version.
//
// This strategy is only reached after the exact match (Strategy 1)
// fails, meaning the literal oldText does not appear in the content.
// A false positive would require the content to contain both the escaped
// and unescaped forms of the same text, which is rare in practice.
func normalizeEscapedQuotes(s string) string {
return strings.ReplaceAll(s, `\"`, `"`)
}

var whitespaceRE = regexp.MustCompile(`\s+`)

func collapseWhitespace(s string) string {
return whitespaceRE.ReplaceAllString(s, " ")
}

func (t *FilesystemTool) handleListDirectory(_ context.Context, args ListDirectoryArgs) (*tools.ToolCallResult, error) {
resolvedPath := t.resolvePath(args.Path)

Expand Down
156 changes: 156 additions & 0 deletions pkg/tools/builtin/filesystem_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -260,6 +260,162 @@ func TestFilesystemTool_EditFile(t *testing.T) {
assert.Contains(t, result.Output, "old text not found")
}

func TestFindAndReplace(t *testing.T) {
t.Parallel()

tests := []struct {
name string
content string
oldText string
newText string
wantOK bool
want string
}{
{
name: "exact match",
content: "hello world",
oldText: "hello",
newText: "hi",
wantOK: true,
want: "hi world",
},
{
name: "no match at all",
content: "hello world",
oldText: "xyz",
newText: "abc",
wantOK: false,
want: "hello world",
},
{
name: "trailing whitespace mismatch",
content: "line one \nline two\n",
oldText: "line one\nline two",
newText: "replaced",
wantOK: true,
want: "replaced\n",
},
{
name: "leading whitespace (indentation) mismatch",
content: "# Install deps\nRUN yarn install",
oldText: " # Install deps\nRUN yarn install",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "line continuation collapsed by LLM",
content: "RUN apt-get update && \\\n apt-get install -y curl",
oldText: "RUN apt-get update && apt-get install -y curl",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "escaped quotes vs unescaped",
content: `echo \"hello world\"`,
oldText: `echo "hello world"`,
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "whitespace collapsed (multiple spaces to one)",
content: "foo bar\tbaz",
oldText: "foo bar baz",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "multi-line continuation Dockerfile",
content: "RUN --mount=type=cache,target=/var/cache/apt \\\n --mount=type=cache,target=/var/lib/apt \\\n apt-get update",
oldText: "RUN --mount=type=cache,target=/var/cache/apt --mount=type=cache,target=/var/lib/apt apt-get update",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "preserves surrounding content",
content: "before\nhello world\nafter",
oldText: "hello world",
newText: "REPLACED",
wantOK: true,
want: "before\nREPLACED\nafter",
},
{
name: "exact match preferred over fuzzy",
content: "hello world",
oldText: "hello world",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "overlapping collapsed sequences",
content: "cmd \\\n --flag1 \\\n --flag2 \\\n --flag3",
oldText: "cmd --flag1 --flag2 --flag3",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "utf8 content with whitespace normalization",
content: "greeting = \"héllo wörld\"",
oldText: "greeting = \"héllo wörld\"",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
{
name: "utf8 content with leading whitespace",
content: "msg := \"日本語テスト\"",
oldText: " msg := \"日本語テスト\"",
newText: "REPLACED",
wantOK: true,
want: "REPLACED",
},
}

for _, tc := range tests {
t.Run(tc.name, func(t *testing.T) {
t.Parallel()
got, ok := FindAndReplace(tc.content, tc.oldText, tc.newText)
assert.Equal(t, tc.wantOK, ok)
if tc.wantOK {
assert.Equal(t, tc.want, got)
}
})
}
}

func TestFilesystemTool_EditFile_FuzzyMatch(t *testing.T) {
t.Parallel()
tmpDir := t.TempDir()
tool := NewFilesystemTool(tmpDir)

// File has line continuations; LLM sends collapsed version.
original := "FROM node:22\n\nRUN apt-get update && \\\n apt-get install -y curl && \\\n rm -rf /var/lib/apt/lists/*\n"
testFile := "Dockerfile"
require.NoError(t, os.WriteFile(filepath.Join(tmpDir, testFile), []byte(original), 0o644))

result, err := tool.handleEditFile(t.Context(), EditFileArgs{
Path: testFile,
Edits: []Edit{
{
OldText: "RUN apt-get update && apt-get install -y curl && rm -rf /var/lib/apt/lists/*",
NewText: "RUN apt-get update && \\\n apt-get install -y curl wget && \\\n rm -rf /var/lib/apt/lists/*",
},
},
})
require.NoError(t, err)
assert.Contains(t, result.Output, "File edited successfully")

edited, err := os.ReadFile(filepath.Join(tmpDir, testFile))
require.NoError(t, err)
assert.Contains(t, string(edited), "curl wget")
}

func TestFilesystemTool_SearchFilesContent(t *testing.T) {
t.Parallel()
tmpDir := t.TempDir()
Expand Down
Loading