diff --git a/nanobot/agent/tools/filesystem.py b/nanobot/agent/tools/filesystem.py index d0639902..1ada82fb 100644 --- a/nanobot/agent/tools/filesystem.py +++ b/nanobot/agent/tools/filesystem.py @@ -621,6 +621,89 @@ class _MatchSpan: 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]: matches: list[_MatchSpan] = [] start = 0 @@ -794,7 +877,46 @@ def _find_match(content: str, old_text: str) -> tuple[str | None, int]: ), line_hint=IntegerSchema( 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, nullable=True, ), @@ -813,6 +935,7 @@ class EditFileTool(_FsTool): _MAX_EDIT_FILE_SIZE = 1024 * 1024 * 1024 # 1 GiB _MARKDOWN_EXTS = frozenset({".md", ".mdx", ".markdown"}) + _DEFAULT_LINE_HINT_WINDOW = 5 @property def name(self) -> str: @@ -826,8 +949,10 @@ class EditFileTool(_FsTool): "with old_text copied from read_file. For multi-file, structural, " "or generated code edits, prefer apply_patch. If old_text matches " "multiple times, provide more context or set occurrence, line_hint, " - "replace_all, and expected_replacements. Shows closest-match " - "diagnostics on failure." + "target_line, target_start_line, replace_all, and expected_replacements. " + "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 @@ -839,7 +964,10 @@ class EditFileTool(_FsTool): self, path: str | None = None, old_text: str | None = None, new_text: str | 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: try: if not path: @@ -852,6 +980,22 @@ class EditFileTool(_FsTool): return ToolResult.error("Error: occurrence must be >= 1.") if line_hint is not None and line_hint < 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: 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.") if replace_all and line_hint is not None: 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: 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 occurrence is not None: if occurrence > count: @@ -910,13 +1062,46 @@ class EditFileTool(_FsTool): f"old_text appears {count} times." ) elif line_hint is not None: - nearest = min(matches, key=lambda match: abs(match.line - line_hint)) - distance = abs(nearest.line - line_hint) - if sum(1 for match in matches if abs(match.line - line_hint) == distance) > 1: + nearest = min(matches, key=lambda match: _line_distance_to_match(match, line_hint)) + distance = _line_distance_to_match(nearest, line_hint) + if sum( + 1 for match in matches + if _line_distance_to_match(match, line_hint) == distance + ) > 1: return ToolResult.error( f"Error: line_hint {line_hint} is ambiguous; " 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: line_numbers = [match.line for match in matches] preview = ", ".join(f"line {n}" for n in line_numbers[:3]) @@ -942,8 +1127,46 @@ class EditFileTool(_FsTool): if replace_all: 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: - 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: selected = [matches[occurrence - 1 if occurrence else 0]] if expected_replacements is not None and len(selected) != expected_replacements: diff --git a/nanobot/templates/agent/tool_contract.md b/nanobot/templates/agent/tool_contract.md index a95a2353..41075806 100644 --- a/nanobot/templates/agent/tool_contract.md +++ b/nanobot/templates/agent/tool_contract.md @@ -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). - 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 `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. - 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`. diff --git a/tests/tools/test_file_edit_coding_enhancements.py b/tests/tools/test_file_edit_coding_enhancements.py index d361d88a..a03f5fe8 100644 --- a/tests/tools/test_file_edit_coding_enhancements.py +++ b/tests/tools/test_file_edit_coding_enhancements.py @@ -86,6 +86,123 @@ def test_edit_file_can_select_nearest_line_hint(tmp_path): 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): target = tmp_path / "analysis.ipynb" 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" +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): target = tmp_path / "duplicate.txt" 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 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"