Gitignore handling and read-edit tools - important fixes and improvments

1. Use all gitignore files in SerenaAgent, not just top level
2. Listing and file finding tools now can find all non-ignored files (previously they would only find code files)
3. Read and edit tools will raise an error on attempting to operate on an ignored file or a path outside the project root

src/multilspy/language_server.py

@@ -105,7 +105,7 @@ class LanguageServer:
         return dirname.startswith('.')
 
     @classmethod
-    def create(cls, config: MultilspyConfig, logger: MultilspyLogger, repository_root_path: str, add_gitignore_content_to_config: bool = True) -> "LanguageServer":
+    def create(cls, config: MultilspyConfig, logger: MultilspyLogger, repository_root_path: str) -> "LanguageServer":
         """
         Creates a language specific LanguageServer instance based on the given configuration, and appropriate settings for the programming language.
 
@@ -115,30 +115,8 @@ class LanguageServer:
         :param repository_root_path: The root path of the repository.
         :param config: The Multilspy configuration.
         :param logger: The logger to use.
-        :param add_gitignore_content_to_config: whether to add the content of the .gitignore file (if any found) to the config, so that
-            the paths ignored there are also ignored by the language server
-
         :return LanguageServer: A language specific LanguageServer instance.
         """
-        config = copy(config)  # prevent mutation
-        if add_gitignore_content_to_config:
-            gitignore_path = os.path.join(repository_root_path, ".gitignore")
-            if not os.path.exists(gitignore_path):
-                logger.log(
-                    f"Should ignore all files in gitignore but no .gitignore found at {gitignore_path}. Skipping.",
-                    logging.WARNING
-                )
-                gitignore_file_content = None
-            else:
-                if config.gitignore_file_content is not None:
-                    raise ValueError(
-                        f"Asked to add gitignore content to the config for {repository_root_path=} but there already is a non-empty entry there:\n{config.gitignore_file_content=}"
-                    )
-                logger.log(f"Reading gitignore file from {gitignore_path}", logging.DEBUG)
-                with open(gitignore_path, "r", encoding="utf-8") as f:
-                    gitignore_file_content = f.read()
-            config.gitignore_file_content = gitignore_file_content
-
         if config.code_language == Language.PYTHON:
             from multilspy.language_servers.pyright_language_server.pyright_server import (
                 PyrightServer,
@@ -270,12 +248,7 @@ class LanguageServer:
             # Normalize separators (pathspec expects forward slashes)
             pattern = pattern.replace(os.path.sep, '/')
             processed_patterns.append(pattern)
-        # Combine explicitly passed patterns with the content of the .gitignore file
-        self.logger.log(f"Processing gitignore content", logging.DEBUG)
-        if config.gitignore_file_content is not None:
-            for line in config.gitignore_file_content.splitlines():
-                if not line.startswith('#') and line.strip() != '':
-                    processed_patterns.append(line.strip())
+        self.logger.log(f"Processing {len(processed_patterns)} ignored paths from the config", logging.DEBUG)
 
         # Create a pathspec matcher from the processed patterns
         self._ignore_spec = pathspec.PathSpec.from_lines(
@@ -285,7 +258,7 @@ class LanguageServer:
 
     def get_ignore_spec(self) -> pathspec.PathSpec:
         """Returns the pathspec matcher for the paths that were configured to be ignored through
-        the multilspy config file and the .gitignore file.
+        the multilspy config.
 
         This is is a subset of the full language-specific ignore spec that determines
         which files are relevant for the language server.
@@ -339,7 +312,7 @@ class LanguageServer:
             normalized_path = normalized_path + '/'
 
         # Use the pathspec matcher to check if the path matches any ignore pattern
-        if self._ignore_spec.match_file(normalized_path):
+        if self.get_ignore_spec().match_file(normalized_path):
             return True
 
         return False
@@ -1697,7 +1670,7 @@ class SyncLanguageServer:
     It is used to communicate with Language Servers of different programming languages.
     """
 
-    def __init__(self, language_server: LanguageServer, timeout: Optional[int] = None):
+    def __init__(self, language_server: LanguageServer, timeout: Optional[float] = None):
         """
         :param language_server: the async language server being wrapped
         :param timeout: the timeout, in seconds, to use for requests to the language server.
@@ -1714,8 +1687,8 @@ class SyncLanguageServer:
 
     @classmethod
     def create(
-        cls, config: MultilspyConfig, logger: MultilspyLogger, repository_root_path: str, add_gitignore_content_to_config=True,
-        timeout: Optional[int] = None
+        cls, config: MultilspyConfig, logger: MultilspyLogger, repository_root_path: str,
+        timeout: Optional[float] = None
     ) -> "SyncLanguageServer":
         """
         Creates a language specific LanguageServer instance based on the given configuration, and appropriate settings for the programming language.
@@ -1725,13 +1698,11 @@ class SyncLanguageServer:
         :param repository_root_path: The root path of the repository (must be absolute).
         :param config: The Multilspy configuration.
         :param logger: The logger to use.
-        :param add_gitignore_content_to_config: whether to add the content of the .gitignore file (if any found) to the config, so that
-            the paths ignored there are also ignored by the language server
         :param timeout: the timeout, in seconds, to use for requests; if None, use no timeout
 
         :return SyncLanguageServer: A language specific LanguageServer instance.
         """
-        return SyncLanguageServer(LanguageServer.create(config, logger, repository_root_path, add_gitignore_content_to_config=add_gitignore_content_to_config), timeout=timeout)
+        return SyncLanguageServer(LanguageServer.create(config, logger, repository_root_path), timeout=timeout)
 
     @property
     def repository_root_path(self) -> str:
@@ -2313,7 +2284,7 @@ class SyncLanguageServer:
 
     def get_ignore_spec(self) -> pathspec.PathSpec:
         """Returns the pathspec matcher for the paths that were configured to be ignored through
-        the multilspy config file and the .gitignore file.
+        the multilspy config.
 
         This is is a subset of the full language-specific ignore spec that determines
         which files are relevant for the language server.

src/multilspy/multilspy_config.py

@@ -82,8 +82,6 @@ class MultilspyConfig:
     start_independent_lsp_process: bool = True
     ignored_paths: list[str] = field(default_factory=list)
     """Paths, dirs or glob-like patterns. The matching will follow the same logic as for .gitignore entries"""
-    gitignore_file_content: str | None = None
-    """Optional content of the gitignore file. If passed, will be used in addition to the explicitly passed ignored_paths for deciding which paths to ignore."""
 
     @classmethod
     def from_dict(cls, env: dict):

src/serena/agent.py

@@ -25,6 +25,7 @@ from typing import TYPE_CHECKING, Any, Literal, Self, TypeVar, Union, cast
 
 import yaml
 from overrides import override
+from pathspec import PathSpec
 from ruamel.yaml.comments import CommentedMap
 from sensai.util import logging
 from sensai.util.logging import FallbackHandler
@@ -41,7 +42,7 @@ from serena.dashboard import MemoryLogHandler, SerenaDashboardAPI
 from serena.prompt_factory import PromptFactory, SerenaPromptFactory
 from serena.symbol import SymbolManager
 from serena.text_utils import search_files
-from serena.util.file_system import scan_directory
+from serena.util.file_system import GitignoreParser, match_path, scan_directory
 from serena.util.general import load_yaml, save_yaml
 from serena.util.inspection import determine_programming_language_composition, iter_subclasses
 from serena.util.shell import execute_shell_command
@@ -583,6 +584,8 @@ class SerenaAgent:
         self.symbol_manager: SymbolManager | None = None
         self.memories_manager: MemoriesManager | None = None
         self.lines_read: LinesRead | None = None
+        self.ignore_spec: PathSpec  # not set to None to avoid assert statements
+        """Ignore spec, extracted from the project's gitignore files and the explicitly configured ignored paths."""
 
         # Apply context and mode tool configurations
         if context is None:
@@ -615,6 +618,46 @@ class SerenaAgent:
             raise ValueError("Cannot get project root if no project is active.")
         return project.project_root
 
+    def path_is_inside_project(self, path: str | Path) -> bool:
+        """
+        Checks if the given (absolute or relative) path is inside the project directory.
+        Note that even relative paths may be outside if the contain ".." or point to symlinks.
+        """
+        path = Path(path)
+        _proj_root = Path(self.get_project_root())
+        if not path.is_absolute():
+            path = _proj_root / path
+
+        path = path.resolve()
+        return path.is_relative_to(_proj_root)
+
+    def path_is_gitignored(self, path: str | Path) -> bool:
+        """
+        Checks if the given path is ignored by git. Non absolute paths are assumed to be relative to the project root.
+        """
+        path = Path(path)
+        if path.is_absolute():
+            relative_path = path.relative_to(self.get_project_root())
+        else:
+            relative_path = path
+
+        # always ignore paths inside .git
+        if len(relative_path.parts) > 0 and relative_path.parts[0] == ".git":
+            return True
+
+        return match_path(str(relative_path), self.ignore_spec)
+
+    def validate_relative_path(self, relative_path: str) -> None:
+        """
+        Validates that the given relative path is safe to read or edit,
+        meaning it's inside the project directory and is not ignored by git.
+        """
+        if not self.path_is_inside_project(relative_path):
+            raise ValueError(f"{relative_path=} points to path outside of the repository root, can't use it for safety reasons")
+
+        if self.path_is_gitignored(relative_path):
+            raise ValueError(f"File {relative_path} is gitignored, can't read or edit it for safety reasons")
+
     def get_exposed_tool_instances(self) -> list["Tool"]:
         """
         :return: all tool instances, including the non-active ones. For MCP clients, we need to expose them all since typical
@@ -685,6 +728,7 @@ class SerenaAgent:
         # start the language server
         self.reset_language_server()
         assert self.language_server is not None
+        self.ignore_spec = self.language_server.get_ignore_spec()
 
         # initialize project-specific instances
         self.symbol_manager = SymbolManager(self.language_server, self)
@@ -805,9 +849,21 @@ class SerenaAgent:
 
         # instantiate and start the language server
         assert self._active_project is not None
+        ignored_paths = self._active_project.project_config.ignored_paths
+        if len(ignored_paths) > 0:
+            log.info(f"Using {len(ignored_paths)} ignored paths from the explicit project configuration.")
+            log.debug(f"Ignored paths: {ignored_paths}")
+        if self._active_project.project_config.ignore_all_files_in_gitignore:
+            log.info(f"Parsing all gitignore files in {self._active_project.project_root}")
+            gitignore_parser = GitignoreParser(self._active_project.project_root)
+            log.info(f"Found {len(gitignore_parser.get_ignore_specs())} gitignore files.")
+            for spec in gitignore_parser.get_ignore_specs():
+                log.debug(f"Adding {len(spec.patterns)} patterns from {spec.file_path} to the ignored paths.")
+                ignored_paths.extend(spec.patterns)
+        log.debug(f"Using {len(ignored_paths)} ignored paths in total.")
         multilspy_config = MultilspyConfig(
             code_language=self._active_project.project_config.language,
-            ignored_paths=self._active_project.project_config.ignored_paths,
+            ignored_paths=ignored_paths,
             trace_lsp_communication=self.serena_config.trace_lsp_communication,
         )
         ls_logger = MultilspyLogger(log_level=self.serena_config.log_level)
@@ -816,7 +872,6 @@ class SerenaAgent:
             multilspy_config,
             ls_logger,
             self._active_project.project_root,
-            add_gitignore_content_to_config=self._active_project.project_config.ignore_all_files_in_gitignore,
         )
         self.language_server.start()
         if not self.language_server.is_running():
@@ -1061,6 +1116,8 @@ class ReadFileTool(Tool):
             required for the task.
         :return: the full text of the file at the given relative path
         """
+        self.agent.validate_relative_path(relative_path)
+
         result = self.language_server.retrieve_full_file_content(relative_path)
         result_lines = result.splitlines()
         if end_line is None:
@@ -1093,11 +1150,17 @@ class CreateTextFileTool(Tool, ToolMarkerCanEdit):
         :param content: the (utf-8-encoded) content to write to the file
         :return: a message indicating success or failure
         """
-        absolute_path = os.path.join(self.get_project_root(), relative_path)
-        os.makedirs(os.path.dirname(absolute_path), exist_ok=True)
-        with open(absolute_path, "w", encoding="utf-8") as f:
-            f.write(content)
-        return f"File created: {relative_path}"
+        self.agent.validate_relative_path(relative_path)
+
+        abs_path = (Path(self.get_project_root()) / relative_path).resolve()
+        will_overwrite_existing = abs_path.exists()
+
+        abs_path.parent.mkdir(parents=True, exist_ok=True)
+        abs_path.write_text(content, encoding="utf-8")
+        answer = f"File created: {relative_path}."
+        if will_overwrite_existing:
+            answer += " Overwrote existing file."
+        return answer
 
 
 class ListDirTool(Tool):
@@ -1107,7 +1170,7 @@ class ListDirTool(Tool):
 
     def apply(self, relative_path: str, recursive: bool, max_answer_chars: int = _DEFAULT_MAX_ANSWER_LENGTH) -> str:
         """
-        Lists files and directories in the given directory (optionally with recursion).
+        Lists all non-gitignored files and directories in the given directory (optionally with recursion).
 
         :param relative_path: the relative path to the directory to list; pass "." to scan the project root
         :param recursive: whether to scan subdirectories recursively
@@ -1116,17 +1179,14 @@ class ListDirTool(Tool):
             required for the task.
         :return: a JSON object with the names of directories and files within the given directory
         """
-
-        def is_ignored_path(abs_path: str) -> bool:
-            rel_path = os.path.relpath(abs_path, self.get_project_root())
-            return self.language_server.is_ignored_path(rel_path, ignore_unsupported_files=False)
+        self.agent.validate_relative_path(relative_path)
 
         dirs, files = scan_directory(
             os.path.join(self.get_project_root(), relative_path),
             relative_to=self.get_project_root(),
             recursive=recursive,
-            is_ignored_dir=is_ignored_path,
-            is_ignored_file=is_ignored_path,
+            is_ignored_dir=self.agent.path_is_gitignored,
+            is_ignored_file=self.agent.path_is_gitignored,
         )
 
         result = json.dumps({"dirs": dirs, "files": files})
@@ -1140,31 +1200,27 @@ class FindFileTool(Tool):
 
     def apply(self, file_mask: str, relative_path: str) -> str:
         """
-        Finds files matching the given file mask within the given relative path
+        Finds non-gitignored files matching the given file mask within the given relative path
 
         :param file_mask: the filename or file mask (using the wildcards * or ?) to search for
         :param relative_path: the relative path to the directory to search in; pass "." to scan the project root
         :return: a JSON object with the list of matching files
         """
+        self.agent.validate_relative_path(relative_path)
 
-        def is_ignored_path(abs_path: str) -> bool:
-            rel_path = os.path.relpath(abs_path, self.get_project_root())
-            return self.language_server.is_ignored_path(rel_path, ignore_unsupported_files=False)
+        dir_to_scan = os.path.join(self.get_project_root(), relative_path)
 
+        # find the files by ignoring everything that doesn't match
         def is_ignored_file(abs_path: str) -> bool:
-            if is_ignored_path(abs_path):
+            if self.agent.path_is_gitignored(abs_path):
                 return True
             filename = os.path.basename(abs_path)
-            is_ignored = not fnmatch(filename, file_mask)
-            if not is_ignored:
-                is_ignored = not fnmatch(filename, file_mask)
-            return is_ignored
+            return not fnmatch(filename, file_mask)
 
         dirs, files = scan_directory(
-            os.path.join(self.get_project_root(), relative_path),
-            relative_to=self.get_project_root(),
+            path=dir_to_scan,
             recursive=True,
-            is_ignored_dir=is_ignored_path,
+            is_ignored_dir=self.agent.path_is_gitignored,
             is_ignored_file=is_ignored_file,
         )
 
@@ -1512,6 +1568,7 @@ class ReplaceRegexTool(Tool, ToolMarkerCanEdit):
             If this is set to False and the regex matches multiple occurrences, an error will be returned
             (and you may retry with a revised, more specific regex).
         """
+        self.agent.validate_relative_path(relative_path)
         with EditedFileContext(relative_path, self.agent) as context:
             original_content = context.get_original_content()
             updated_content, n = re.subn(regex, repl, original_content, flags=re.DOTALL | re.MULTILINE)
@@ -1833,16 +1890,16 @@ class SearchForPatternTool(Tool):
         else:
             # we walk through all files in the project starting from the root
             files_to_search = []
-            ignore_spec = self.language_server.get_ignore_spec()
             for root, dirs, files in os.walk(self.get_project_root()):
                 # Don't go into directories that are ignored by modifying dirs inplace
                 # Explanation for the  + "/" part:
                 # pathspec can't handle the matching of directories if they don't end with a slash!
                 # see https://github.com/cpburnz/python-pathspec/issues/89
-                dirs[:] = [d for d in dirs if not ignore_spec.match_file(d + "/")]
+                dirs[:] = [d for d in dirs if not self.agent.path_is_gitignored(os.path.join(root, d))]
                 for file in files:
-                    if not ignore_spec.match_file(os.path.join(root, file)):
-                        files_to_search.append(os.path.join(root, file))
+                    file_path = os.path.join(root, file)
+                    if not self.agent.path_is_gitignored(file_path):
+                        files_to_search.append(file_path)
             # TODO (maybe): not super efficient to walk through the files again and filter if glob patterns are provided
             #   but it probably never matters and this version required no further refactoring
             matches = search_files(

src/serena/util/file_system.py

@@ -78,17 +78,14 @@ def find_all_non_ignored_files(repo_root: str) -> list[str]:
 
 @dataclass
 class GitignoreSpec:
-    """
-    Represents a single gitignore file and its parsed patterns.
-
-    :param file_path: Path to the gitignore file
-    :param patterns: List of adjusted patterns from the gitignore file
-    :param pathspec: Compiled PathSpec object for pattern matching
-    """
-
     file_path: str
+    """Path to the gitignore file."""
     patterns: list[str] = field(default_factory=list)
+    """List of patterns from the gitignore file.
+    The patterns are adjusted based on the gitignore file location.
+    """
     pathspec: PathSpec = field(init=False)
+    """Compiled PathSpec object for pattern matching."""
 
     def __post_init__(self) -> None:
         """Initialize the PathSpec from patterns."""
@@ -276,3 +273,14 @@ class GitignoreParser:
         """Reload all gitignore files from the repository."""
         self.ignore_specs.clear()
         self._load_gitignore_files()
+
+
+def match_path(path: str, path_spec: PathSpec) -> bool:
+    path = os.path.abspath(path)
+    normalized_path = str(path).replace(os.path.sep, "/")
+
+    # pathspec can't handle the matching of directories if they don't end with a slash!
+    # see https://github.com/cpburnz/python-pathspec/issues/89
+    if os.path.isdir(normalized_path) and not normalized_path.endswith("/"):
+        normalized_path = normalized_path + "/"
+    return path_spec.match_file(normalized_path)