feat(tools): guard edit_file target lines

This commit is contained in:
chengyongru
2026-07-11 01:14:50 +08:00
committed by Xubin Ren
parent 47d498bab0
commit 052fdc132d
3 changed files with 415 additions and 9 deletions
+231 -8
View File
@@ -621,6 +621,89 @@ class _MatchSpan:
line: int line: int
def _match_end_line(match: _MatchSpan) -> int:
comparable = match.text[:-1] if match.text.endswith("\n") else match.text
return match.line + comparable.count("\n")
def _line_distance_to_match(match: _MatchSpan, line: int) -> int:
end_line = _match_end_line(match)
if match.line <= line <= end_line:
return 0
if line < match.line:
return match.line - line
return line - end_line
def _format_match_locations(matches: list[_MatchSpan], *, max_items: int = 5) -> str:
locations: list[str] = []
for match in matches[:max_items]:
end_line = _match_end_line(match)
if end_line == match.line:
locations.append(f"line {match.line}")
else:
locations.append(f"lines {match.line}-{end_line}")
if len(matches) > max_items:
locations.append("...")
return ", ".join(locations)
def _line_guard_error(
*,
guard_name: str,
guard_line: int,
matches: list[_MatchSpan],
max_distance: int,
) -> str:
nearest = min(matches, key=lambda match: _line_distance_to_match(match, guard_line))
distance = _line_distance_to_match(nearest, guard_line)
return ToolResult.error(
f"Error: {guard_name} {guard_line} does not match the old_text location. "
f"old_text appears at {_format_match_locations(matches)}. "
f"Nearest distance is {distance} line(s); allowed distance is {max_distance}. "
"Re-read the intended region and copy old_text from the target line, "
"or adjust the line guard."
)
def _start_line_guard_error(
*,
guard_line: int,
matches: list[_MatchSpan],
) -> str:
nearest = min(matches, key=lambda match: abs(match.line - guard_line))
distance = abs(nearest.line - guard_line)
return ToolResult.error(
f"Error: target_start_line {guard_line} does not match the old_text start. "
f"old_text appears at {_format_match_locations(matches)}. "
f"Nearest start-line distance is {distance} line(s). "
"Re-read the intended region and copy old_text starting at the target block line."
)
def _matches_for_line_guards(
matches: list[_MatchSpan],
*,
target_line: int | None,
target_line_window: int,
target_start_line: int | None,
) -> list[_MatchSpan]:
candidates = matches
if target_line is not None:
candidates = [
match
for match in candidates
if _line_distance_to_match(match, target_line) <= target_line_window
]
if target_start_line is not None:
candidates = [
match
for match in candidates
if match.line == target_start_line
]
return candidates
def _find_exact_matches(content: str, old_text: str) -> list[_MatchSpan]: def _find_exact_matches(content: str, old_text: str) -> list[_MatchSpan]:
matches: list[_MatchSpan] = [] matches: list[_MatchSpan] = []
start = 0 start = 0
@@ -794,7 +877,46 @@ def _find_match(content: str, old_text: str) -> tuple[str | None, int]:
), ),
line_hint=IntegerSchema( line_hint=IntegerSchema(
1, 1,
description="Optional 1-based line hint used to choose the nearest match.", description=(
"Optional 1-based line hint copied from read_file. Used to choose "
"the nearest match and reject matches outside line_hint_window."
),
minimum=1,
nullable=True,
),
line_hint_window=IntegerSchema(
5,
description=(
"Maximum line distance allowed between line_hint and the selected "
"old_text match (default 5)."
),
minimum=0,
nullable=True,
),
target_line=IntegerSchema(
1,
description=(
"Optional 1-based line from read_file that the selected old_text "
"match must cover. Prefer this when editing a specific numbered line."
),
minimum=1,
nullable=True,
),
target_line_window=IntegerSchema(
0,
description=(
"Maximum line distance allowed around target_line (default 0, "
"meaning old_text must cover target_line)."
),
minimum=0,
nullable=True,
),
target_start_line=IntegerSchema(
1,
description=(
"Optional 1-based line where the selected old_text match must start. "
"Use this for block edits that must begin at a specific numbered line."
),
minimum=1, minimum=1,
nullable=True, nullable=True,
), ),
@@ -813,6 +935,7 @@ class EditFileTool(_FsTool):
_MAX_EDIT_FILE_SIZE = 1024 * 1024 * 1024 # 1 GiB _MAX_EDIT_FILE_SIZE = 1024 * 1024 * 1024 # 1 GiB
_MARKDOWN_EXTS = frozenset({".md", ".mdx", ".markdown"}) _MARKDOWN_EXTS = frozenset({".md", ".mdx", ".markdown"})
_DEFAULT_LINE_HINT_WINDOW = 5
@property @property
def name(self) -> str: def name(self) -> str:
@@ -826,8 +949,10 @@ class EditFileTool(_FsTool):
"with old_text copied from read_file. For multi-file, structural, " "with old_text copied from read_file. For multi-file, structural, "
"or generated code edits, prefer apply_patch. If old_text matches " "or generated code edits, prefer apply_patch. If old_text matches "
"multiple times, provide more context or set occurrence, line_hint, " "multiple times, provide more context or set occurrence, line_hint, "
"replace_all, and expected_replacements. Shows closest-match " "target_line, target_start_line, replace_all, and expected_replacements. "
"diagnostics on failure." "Prefer target_line when editing from numbered read_file output. Use "
"target_start_line when a block edit must start at a specific line. "
"Shows closest-match diagnostics on failure."
) )
@staticmethod @staticmethod
@@ -839,7 +964,10 @@ class EditFileTool(_FsTool):
self, path: str | None = None, old_text: str | None = None, self, path: str | None = None, old_text: str | None = None,
new_text: str | None = None, new_text: str | None = None,
replace_all: bool = False, occurrence: int | None = None, replace_all: bool = False, occurrence: int | None = None,
line_hint: int | None = None, expected_replacements: int | None = None, **kwargs: Any, line_hint: int | None = None, line_hint_window: int | None = None,
target_line: int | None = None, target_line_window: int | None = None,
target_start_line: int | None = None,
expected_replacements: int | None = None, **kwargs: Any,
) -> str: ) -> str:
try: try:
if not path: if not path:
@@ -852,6 +980,22 @@ class EditFileTool(_FsTool):
return ToolResult.error("Error: occurrence must be >= 1.") return ToolResult.error("Error: occurrence must be >= 1.")
if line_hint is not None and line_hint < 1: if line_hint is not None and line_hint < 1:
return ToolResult.error("Error: line_hint must be >= 1.") return ToolResult.error("Error: line_hint must be >= 1.")
if line_hint_window is not None and line_hint_window < 0:
return ToolResult.error("Error: line_hint_window must be >= 0.")
if target_line is not None and target_line < 1:
return ToolResult.error("Error: target_line must be >= 1.")
if target_line_window is not None and target_line_window < 0:
return ToolResult.error("Error: target_line_window must be >= 0.")
if target_start_line is not None and target_start_line < 1:
return ToolResult.error("Error: target_start_line must be >= 1.")
if line_hint_window is not None and line_hint is None:
return ToolResult.error("Error: line_hint_window requires line_hint.")
if target_line_window is not None and target_line is None:
return ToolResult.error("Error: target_line_window requires target_line.")
if line_hint is not None and target_line is not None:
return ToolResult.error("Error: line_hint cannot be used with target_line.")
if line_hint is not None and target_start_line is not None:
return ToolResult.error("Error: line_hint cannot be used with target_start_line.")
if expected_replacements is not None and expected_replacements < 1: if expected_replacements is not None and expected_replacements < 1:
return ToolResult.error("Error: expected_replacements must be >= 1.") return ToolResult.error("Error: expected_replacements must be >= 1.")
@@ -900,8 +1044,16 @@ class EditFileTool(_FsTool):
return ToolResult.error("Error: occurrence cannot be used with replace_all=true.") return ToolResult.error("Error: occurrence cannot be used with replace_all=true.")
if replace_all and line_hint is not None: if replace_all and line_hint is not None:
return ToolResult.error("Error: line_hint cannot be used with replace_all=true.") return ToolResult.error("Error: line_hint cannot be used with replace_all=true.")
if replace_all and target_line is not None:
return ToolResult.error("Error: target_line cannot be used with replace_all=true.")
if replace_all and target_start_line is not None:
return ToolResult.error("Error: target_start_line cannot be used with replace_all=true.")
if occurrence is not None and line_hint is not None: if occurrence is not None and line_hint is not None:
return ToolResult.error("Error: line_hint cannot be used with occurrence.") return ToolResult.error("Error: line_hint cannot be used with occurrence.")
if occurrence is not None and target_line is not None:
return ToolResult.error("Error: target_line cannot be used with occurrence.")
if occurrence is not None and target_start_line is not None:
return ToolResult.error("Error: target_start_line cannot be used with occurrence.")
if count > 1 and not replace_all: if count > 1 and not replace_all:
if occurrence is not None: if occurrence is not None:
if occurrence > count: if occurrence > count:
@@ -910,13 +1062,46 @@ class EditFileTool(_FsTool):
f"old_text appears {count} times." f"old_text appears {count} times."
) )
elif line_hint is not None: elif line_hint is not None:
nearest = min(matches, key=lambda match: abs(match.line - line_hint)) nearest = min(matches, key=lambda match: _line_distance_to_match(match, line_hint))
distance = abs(nearest.line - line_hint) distance = _line_distance_to_match(nearest, line_hint)
if sum(1 for match in matches if abs(match.line - line_hint) == distance) > 1: if sum(
1 for match in matches
if _line_distance_to_match(match, line_hint) == distance
) > 1:
return ToolResult.error( return ToolResult.error(
f"Error: line_hint {line_hint} is ambiguous; " f"Error: line_hint {line_hint} is ambiguous; "
f"old_text appears {count} times." f"old_text appears {count} times."
) )
elif target_line is not None or target_start_line is not None:
target_window = target_line_window if target_line_window is not None else 0
candidates = _matches_for_line_guards(
matches,
target_line=target_line,
target_line_window=target_window,
target_start_line=target_start_line,
)
if not candidates:
if target_start_line is not None:
start_candidates = [
match for match in matches if match.line == target_start_line
]
if not start_candidates:
return _start_line_guard_error(
guard_line=target_start_line,
matches=matches,
)
return _line_guard_error(
guard_name="target_line",
guard_line=target_line or 1,
matches=matches,
max_distance=target_window,
)
if len(candidates) > 1:
return ToolResult.error(
f"Error: target_line {target_line} is ambiguous; "
f"old_text appears {count} times at "
f"{_format_match_locations(candidates)}."
)
else: else:
line_numbers = [match.line for match in matches] line_numbers = [match.line for match in matches]
preview = ", ".join(f"line {n}" for n in line_numbers[:3]) preview = ", ".join(f"line {n}" for n in line_numbers[:3])
@@ -942,8 +1127,46 @@ class EditFileTool(_FsTool):
if replace_all: if replace_all:
selected = matches selected = matches
elif target_line is not None or target_start_line is not None:
target_window = target_line_window if target_line_window is not None else 0
candidates = _matches_for_line_guards(
matches,
target_line=target_line,
target_line_window=target_window,
target_start_line=target_start_line,
)
if not candidates:
if target_start_line is not None:
start_candidates = [
match for match in matches if match.line == target_start_line
]
if not start_candidates:
return _start_line_guard_error(
guard_line=target_start_line,
matches=matches,
)
return _line_guard_error(
guard_name="target_line",
guard_line=target_line or 1,
matches=matches,
max_distance=target_window,
)
selected = candidates
elif line_hint is not None: elif line_hint is not None:
selected = [min(matches, key=lambda match: abs(match.line - line_hint))] nearest = min(matches, key=lambda match: _line_distance_to_match(match, line_hint))
window = (
self._DEFAULT_LINE_HINT_WINDOW
if line_hint_window is None
else line_hint_window
)
if _line_distance_to_match(nearest, line_hint) > window:
return _line_guard_error(
guard_name="line_hint",
guard_line=line_hint,
matches=matches,
max_distance=window,
)
selected = [nearest]
else: else:
selected = [matches[occurrence - 1 if occurrence else 0]] selected = [matches[occurrence - 1 if occurrence else 0]]
if expected_replacements is not None and len(selected) != expected_replacements: if expected_replacements is not None and len(selected) != expected_replacements:
+1 -1
View File
@@ -26,7 +26,7 @@ Tool signatures are provided automatically via function calling. This section do
- For code or config changes, the default loop is: locate (`find_files`/`grep`), inspect (`read_file`), edit (`apply_patch`), then verify (`exec` or re-read). - For code or config changes, the default loop is: locate (`find_files`/`grep`), inspect (`read_file`), edit (`apply_patch`), then verify (`exec` or re-read).
- Use `apply_patch` as the default code editing tool, especially for multi-file changes, structural edits, generated code, moves, adds, or deletes. - Use `apply_patch` as the default code editing tool, especially for multi-file changes, structural edits, generated code, moves, adds, or deletes.
- Use `apply_patch dry_run=true` when the patch is uncertain and you want validation plus a change summary before writing. - Use `apply_patch dry_run=true` when the patch is uncertain and you want validation plus a change summary before writing.
- Use `edit_file` only for small exact replacements in one file, with `old_text` copied from `read_file`; add `occurrence`, `line_hint`, or `expected_replacements` when ambiguity matters. - Use `edit_file` only for small exact replacements in one file, with `old_text` copied from `read_file`; when editing a specific numbered line, pass `target_line`; when a block must start at a specific line, also pass `target_start_line`; add `occurrence`, `line_hint`, or `expected_replacements` when ambiguity matters.
- Use `write_file` for new files or intentional full-file rewrites, not routine partial edits. - Use `write_file` for new files or intentional full-file rewrites, not routine partial edits.
- If `apply_patch` or `edit_file` fails, re-read with `force=true`, narrow the context, and try a smaller patch rather than switching to shell `sed` or `echo`. - If `apply_patch` or `edit_file` fails, re-read with `force=true`, narrow the context, and try a smaller patch rather than switching to shell `sed` or `echo`.
@@ -86,6 +86,123 @@ def test_edit_file_can_select_nearest_line_hint(tmp_path):
assert target.read_text() == "one\nsame\ntwo\nchanged\n" assert target.read_text() == "one\nsame\ntwo\nchanged\n"
def test_edit_file_rejects_unique_match_far_from_line_hint(tmp_path):
target = tmp_path / "wrong-line.txt"
target.write_text("one\nsame\ntwo\nother\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
line_hint=20,
))
assert "line_hint 20 does not match the old_text location" in result
assert "old_text appears at line 2" in result
assert target.read_text() == "one\nsame\ntwo\nother\n"
def test_edit_file_target_line_selects_matching_occurrence(tmp_path):
target = tmp_path / "duplicate.txt"
target.write_text("one\nsame\ntwo\nsame\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
target_line=4,
))
assert "Successfully edited" in result
assert target.read_text() == "one\nsame\ntwo\nchanged\n"
def test_edit_file_target_line_rejects_unique_match_on_wrong_line(tmp_path):
target = tmp_path / "wrong-line.txt"
target.write_text("one\nsame\ntwo\nother\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
target_line=4,
))
assert "target_line 4 does not match the old_text location" in result
assert "old_text appears at line 2" in result
assert target.read_text() == "one\nsame\ntwo\nother\n"
def test_edit_file_target_line_can_cover_multiline_match(tmp_path):
target = tmp_path / "block.txt"
target.write_text("before\nstart\nmiddle\nend\nafter\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="start\nmiddle\nend",
new_text="start\nchanged\nend",
target_line=3,
))
assert "Successfully edited" in result
assert target.read_text() == "before\nstart\nchanged\nend\nafter\n"
def test_edit_file_target_start_line_rejects_context_that_starts_too_early(tmp_path):
target = tmp_path / "block.txt"
target.write_text("before\nstart\nmiddle\nend\nafter\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="before\nstart\nmiddle",
new_text="before\nchanged\nmiddle",
target_line=2,
target_start_line=2,
))
assert "target_start_line 2 does not match the old_text start" in result
assert target.read_text() == "before\nstart\nmiddle\nend\nafter\n"
def test_edit_file_target_start_line_allows_exact_block_start(tmp_path):
target = tmp_path / "block.txt"
target.write_text("before\nstart\nmiddle\nend\nafter\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="start\nmiddle\nend",
new_text="start\nchanged\nend",
target_line=3,
target_start_line=2,
))
assert "Successfully edited" in result
assert target.read_text() == "before\nstart\nchanged\nend\nafter\n"
def test_edit_file_target_line_error_wins_when_start_line_matches(tmp_path):
target = tmp_path / "block.txt"
target.write_text("before\nstart\nmiddle\nend\nafter\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="start\nmiddle",
new_text="changed\nmiddle",
target_line=4,
target_start_line=2,
))
assert "target_line 4 does not match the old_text location" in result
assert target.read_text() == "before\nstart\nmiddle\nend\nafter\n"
def test_edit_file_can_edit_ipynb_as_json(tmp_path): def test_edit_file_can_edit_ipynb_as_json(tmp_path):
target = tmp_path / "analysis.ipynb" target = tmp_path / "analysis.ipynb"
target.write_text('{"cells": []}') target.write_text('{"cells": []}')
@@ -184,6 +301,40 @@ def test_edit_file_rejects_line_hint_with_occurrence(tmp_path):
assert target.read_text() == "same\nsame\n" assert target.read_text() == "same\nsame\n"
def test_edit_file_rejects_target_line_with_occurrence(tmp_path):
target = tmp_path / "duplicate.txt"
target.write_text("same\nsame\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
occurrence=1,
target_line=1,
))
assert "target_line cannot be used with occurrence" in result
assert target.read_text() == "same\nsame\n"
def test_edit_file_rejects_target_start_line_with_occurrence(tmp_path):
target = tmp_path / "duplicate.txt"
target.write_text("same\nsame\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
occurrence=1,
target_start_line=1,
))
assert "target_start_line cannot be used with occurrence" in result
assert target.read_text() == "same\nsame\n"
def test_edit_file_rejects_zero_occurrence(tmp_path): def test_edit_file_rejects_zero_occurrence(tmp_path):
target = tmp_path / "duplicate.txt" target = tmp_path / "duplicate.txt"
target.write_text("same\n") target.write_text("same\n")
@@ -214,3 +365,35 @@ def test_edit_file_rejects_zero_line_hint(tmp_path):
assert "line_hint must be >= 1" in result assert "line_hint must be >= 1" in result
assert target.read_text() == "same\n" assert target.read_text() == "same\n"
def test_edit_file_rejects_zero_target_line(tmp_path):
target = tmp_path / "duplicate.txt"
target.write_text("same\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
target_line=0,
))
assert "target_line must be >= 1" in result
assert target.read_text() == "same\n"
def test_edit_file_rejects_zero_target_start_line(tmp_path):
target = tmp_path / "duplicate.txt"
target.write_text("same\n")
tool = EditFileTool(workspace=tmp_path)
result = asyncio.run(tool.execute(
path=str(target),
old_text="same",
new_text="changed",
target_start_line=0,
))
assert "target_start_line must be >= 1" in result
assert target.read_text() == "same\n"