From 813800def35b83946527694d65733b944a9836fd Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Tue, 1 Apr 2025 20:03:42 +0200 Subject: [PATCH 1/4] Force agent to read the lines it reads/deletes beforehand --- src/serena/agent.py | 66 ++++++++++++++++++++++++++++++++++++++------ src/serena/symbol.py | 9 ++++-- 2 files changed, 65 insertions(+), 10 deletions(-) diff --git a/src/serena/agent.py b/src/serena/agent.py index 725b665..ed80c1d 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -36,6 +36,21 @@ LOG_FORMAT = "%(levelname)-5s %(asctime)-15s %(name)s:%(funcName)s:%(lineno)d - TTool = TypeVar("TTool", bound="Tool") +class LinesRead: + def __init__(self) -> None: + self.files = defaultdict(lambda: set()) + + def add_lines_read(self, relative_path: str, lines: tuple[int, int]) -> None: + self.files[relative_path].add(lines) + + def were_lines_read(self, relative_path: str, lines: tuple[int, int]) -> bool: + return lines in self.files[relative_path] + + def invalidate_lines_read(self, relative_path: str) -> None: + if relative_path in self.files: + del self.files[relative_path] + + class SerenaAgent: def __init__(self, project_file_path: str, start_language_server: bool = False): """ @@ -81,10 +96,9 @@ class SerenaAgent: self.language_server = SyncLanguageServer.create(config, logger, self.project_root) self.prompt_factory = PromptFactory() - self.symbol_manager = SymbolManager(self.language_server) - - memories_dir = os.path.join(self.get_serena_managed_dir(), "memories") - self.memories_manager = MemoriesManager(memories_dir) + self.symbol_manager = SymbolManager(self.language_server, self) + self.memories_manager = MemoriesManager(os.path.join(self.get_serena_managed_dir(), "memories")) + self.lines_read = LinesRead() # find all tool classes and instantiate them self.tools: dict[type[Tool], Tool] = {} @@ -109,6 +123,9 @@ class SerenaAgent: def get_serena_managed_dir(self) -> str: return os.path.join(self.project_root, ".serena") + def mark_file_modified(self, relativ_path: str) -> None: + self.lines_read.invalidate_lines_read(relativ_path) + def __del__(self) -> None: """ Destructor to clean up the language server instance and GUI logger @@ -538,8 +555,9 @@ class InsertAfterSymbolTool(Tool): :param column: the column :param body: the body/content to be inserted """ + location = SymbolLocation(relative_path, line, column) self.symbol_manager.insert_after( - SymbolLocation(relative_path, line, column), + location, body=body, ) return "OK" @@ -593,10 +611,39 @@ class DeleteLinesTool(Tool): :param start_line: the 0-based index of the first line to be deleted :param end_line: the 0-based index of the last line to be deleted """ + if not self.agent.lines_read.were_lines_read(relative_path, (start_line, end_line)): + read_lines_tool = self.agent.get_tool(ReadFileTool) + return f"Error: Must call `{read_lines_tool.get_name()}` first to read exactly the affected lines." self.symbol_manager.delete_lines(relative_path, start_line, end_line) return "OK" +class ReplaceLinesTool(Tool): + """ + Replaces a range of lines within a file with new content. + """ + + def apply( + self, + relative_path: str, + start_line: int, + end_line: int, + content: str, + ) -> str: + """ + Deletes the given lines in the file. An editing operation, rarely used alone but can be useful in combination with + other tools. + + :param relative_path: the relative path to the file + :param start_line: the 0-based index of the first line to be deleted + :param end_line: the 0-based index of the last line to be deleted + :param content: the content to insert + """ + self.agent.get_tool(DeleteLinesTool).apply(relative_path, start_line, end_line) + self.agent.get_tool(InsertAtLineTool).apply(relative_path, start_line, content) + return "OK" + + class InsertAtLineTool(Tool): """ Inserts content at a given line in a file. @@ -609,14 +656,17 @@ class InsertAtLineTool(Tool): content: str, ) -> str: """ - Inserts the given content at the given line in the file. In general, symbolic insert operations like - insert_after_symbol or insert_before_symbol should be preferred if you know which symbol you are looking for. + Inserts the given content at the given line in the file, pushing existing content of the line down. + In general, symbolic insert operations like insert_after_symbol or insert_before_symbol should be preferred if you know which + symbol you are looking for. However, this can also be useful for small targeted edits of the body of a longer symbol (without replacing the entire body). :param relative_path: the relative path to the file :param line: the 0-based index of the line to insert content at - :param content: the body/content to be inserted + :param content: the content to be inserted """ + if not content.endswith("\n"): + content += "\n" self.symbol_manager.insert_at_line(relative_path, line, content) return "OK" diff --git a/src/serena/symbol.py b/src/serena/symbol.py index ec1f003..9c698f4 100644 --- a/src/serena/symbol.py +++ b/src/serena/symbol.py @@ -4,13 +4,16 @@ from collections.abc import Iterator, Sequence from contextlib import contextmanager from copy import copy from dataclasses import asdict, dataclass -from typing import Any, Self +from typing import TYPE_CHECKING, Any, Self from sensai.util.string import ToStringMixin from multilspy import SyncLanguageServer from multilspy.multilspy_types import Position, SymbolKind, UnifiedSymbolInformation +if TYPE_CHECKING: + from .agent import SerenaAgent + log = logging.getLogger(__name__) @@ -187,8 +190,9 @@ class Symbol(ToStringMixin): class SymbolManager: - def __init__(self, lang_server: SyncLanguageServer) -> None: + def __init__(self, lang_server: SyncLanguageServer, agent: "SerenaAgent") -> None: self.lang_server = lang_server + self.agent = agent def _to_symbols(self, items: list[UnifiedSymbolInformation]) -> list[Symbol]: return [Symbol(s) for s in items] @@ -289,6 +293,7 @@ class SymbolManager: abs_path = os.path.join(root_path, relative_path) with open(abs_path, "w") as f: f.write(file_buffer.contents) + self.agent.mark_file_modified(relative_path) @contextmanager def _edited_symbol_location(self, location: SymbolLocation) -> Iterator[Symbol]: From 382c8a4dbccc57f34a7d280fdc72512e9466e98b Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Tue, 1 Apr 2025 20:09:20 +0200 Subject: [PATCH 2/4] Allow tool to be retrieved from agent even if it is disabled (adding _all_tools) such that tools can call each other without referenced tools necessarily being enabled --- src/serena/agent.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/serena/agent.py b/src/serena/agent.py index dd10852..ce22947 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -103,12 +103,14 @@ class SerenaAgent: # find all tool classes and instantiate them excluded_tools = project_config.get("excluded_tools", []) + self._all_tools: dict[type[Tool], Tool] = {} self.tools: dict[type[Tool], Tool] = {} for tool_class in iter_tool_classes(): + tool_instance = tool_class(self) + self._all_tools[tool_class] = tool_instance if (tool_name := tool_class.get_name()) in excluded_tools: log.info(f"Skipping tool {tool_name} because it is in the exclude list") continue - tool_instance = tool_class(self) self.tools[tool_class] = tool_instance log.info(f"Loaded tools: {', '.join([tool.get_name() for tool in self.tools.values()])}") @@ -118,7 +120,7 @@ class SerenaAgent: self.language_server.start() def get_tool(self, tool_class: type[TTool]) -> TTool: - return self.tools[tool_class] # type: ignore + return self._all_tools[tool_class] # type: ignore def print_tool_overview(self) -> None: _print_tool_overview(self.tools.values()) From a9ea5a241b2b076115af51525afc298d942f1456 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Tue, 1 Apr 2025 20:48:59 +0200 Subject: [PATCH 3/4] Fix error not being returned by ReplaceLinesTool, improve docstrings --- src/serena/agent.py | 42 ++++++++++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 16 deletions(-) diff --git a/src/serena/agent.py b/src/serena/agent.py index ce22947..64c8ee2 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -35,6 +35,7 @@ from serena.util.shell import execute_shell_command log = logging.getLogger(__name__) LOG_FORMAT = "%(levelname)-5s %(asctime)-15s %(name)s:%(funcName)s:%(lineno)d - %(message)s" TTool = TypeVar("TTool", bound="Tool") +SUCCESS_RESULT = "OK" class LinesRead: @@ -45,7 +46,9 @@ class LinesRead: self.files[relative_path].add(lines) def were_lines_read(self, relative_path: str, lines: tuple[int, int]) -> bool: - return lines in self.files[relative_path] + lines_read_in_file = self.files[relative_path] + log.warning(f"Lines read in {relative_path}: {lines_read_in_file}; requested: {lines}") + return lines in lines_read_in_file def invalidate_lines_read(self, relative_path: str) -> None: if relative_path in self.files: @@ -284,13 +287,13 @@ class ReadFileTool(Tool): self, relative_path: str, start_line: int = 0, end_line: int | None = None, max_answer_chars: int = _DEFAULT_MAX_ANSWER_LENGTH ) -> str: """ - Reads the given file or a chunk of it (between start_line and end_line). Generally, symbolic operations + Reads the given file or a chunk of it. Generally, symbolic operations like find_symbol or find_referencing_symbols should be preferred if you know which symbols you are looking for. Reading the entire file is only recommended if there is no other way to get the content required for the task. :param relative_path: the relative path to the file to read - :param start_line: the start line of the range to read - :param end_line: the end line of the range to read. If None, the entire file will be read. + :param start_line: the 0-based index of the first line to be retrieved. + :param end_line: the 0-based index of the last line to be retrieved (inclusive). If None, read until the end of the file. :param max_answer_chars: if the file (chunk) is longer than this number of characters, no content will be returned. Don't adjust unless there is really no other way to get the content required for the task. @@ -301,7 +304,8 @@ class ReadFileTool(Tool): if end_line is None: result_lines = result_lines[start_line:] else: - result_lines = result_lines[start_line:end_line] + self.agent.lines_read.add_lines_read(relative_path, (start_line, end_line)) + result_lines = result_lines[start_line:end_line+1] result = "\n".join(result_lines) return self._limit_length(result, max_answer_chars) @@ -537,7 +541,7 @@ class ReplaceSymbolBodyTool(Tool): SymbolLocation(relative_path, line, column), body=body, ) - return "OK" + return SUCCESS_RESULT class InsertAfterSymbolTool(Tool): @@ -566,7 +570,7 @@ class InsertAfterSymbolTool(Tool): location, body=body, ) - return "OK" + return SUCCESS_RESULT class InsertBeforeSymbolTool(Tool): @@ -595,7 +599,7 @@ class InsertBeforeSymbolTool(Tool): SymbolLocation(relative_path, line, column), body=body, ) - return "OK" + return SUCCESS_RESULT class DeleteLinesTool(Tool): @@ -610,8 +614,9 @@ class DeleteLinesTool(Tool): end_line: int, ) -> str: """ - Deletes the given lines in the file. An editing operation, rarely used alone but can be useful in combination with - other tools. + Deletes the given lines in the file. + Requires that the same range of lines was previously read using the `read_file` tool to verify correctness + of the operation. :param relative_path: the relative path to the file :param start_line: the 0-based index of the first line to be deleted @@ -621,7 +626,7 @@ class DeleteLinesTool(Tool): read_lines_tool = self.agent.get_tool(ReadFileTool) return f"Error: Must call `{read_lines_tool.get_name()}` first to read exactly the affected lines." self.symbol_manager.delete_lines(relative_path, start_line, end_line) - return "OK" + return SUCCESS_RESULT class ReplaceLinesTool(Tool): @@ -637,17 +642,22 @@ class ReplaceLinesTool(Tool): content: str, ) -> str: """ - Deletes the given lines in the file. An editing operation, rarely used alone but can be useful in combination with - other tools. + Replaces the given range of lines in the given file. + Requires that the same range of lines was previously read using the `read_file` tool to verify correctness + of the operation. :param relative_path: the relative path to the file :param start_line: the 0-based index of the first line to be deleted :param end_line: the 0-based index of the last line to be deleted :param content: the content to insert """ - self.agent.get_tool(DeleteLinesTool).apply(relative_path, start_line, end_line) + if not content.endswith("\n"): + content += "\n" + result = self.agent.get_tool(DeleteLinesTool).apply(relative_path, start_line, end_line) + if result != SUCCESS_RESULT: + return result self.agent.get_tool(InsertAtLineTool).apply(relative_path, start_line, content) - return "OK" + return SUCCESS_RESULT class InsertAtLineTool(Tool): @@ -674,7 +684,7 @@ class InsertAtLineTool(Tool): if not content.endswith("\n"): content += "\n" self.symbol_manager.insert_at_line(relative_path, line, content) - return "OK" + return SUCCESS_RESULT class CheckOnboardingPerformedTool(Tool): From 7fb05ac244e738f518017045115783aa2b842133 Mon Sep 17 00:00:00 2001 From: Dominik Jain Date: Tue, 1 Apr 2025 21:55:47 +0200 Subject: [PATCH 4/4] Remove debug log message --- src/serena/agent.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/serena/agent.py b/src/serena/agent.py index 64c8ee2..6a4121a 100644 --- a/src/serena/agent.py +++ b/src/serena/agent.py @@ -47,7 +47,6 @@ class LinesRead: def were_lines_read(self, relative_path: str, lines: tuple[int, int]) -> bool: lines_read_in_file = self.files[relative_path] - log.warning(f"Lines read in {relative_path}: {lines_read_in_file}; requested: {lines}") return lines in lines_read_in_file def invalidate_lines_read(self, relative_path: str) -> None: