Merge pull request #1731 from oraios/fup-ls-file-system-sync

F'up: LS file system sync

src/serena/ls_manager.py

@@ -280,12 +280,17 @@ class LanguageServerFileChangeNotifier:
     Detects changes to source files on disk and notifies language servers of those changes.
     """
 
-    def __init__(self, project: "Project", language_server_manager: LanguageServerManager) -> None:
+    def __init__(self, project: "Project", language_server_manager: LanguageServerManager, initial_poll: bool = True) -> None:
         self._project = project
         self._language_server_manager = language_server_manager
         self._freshness_last_seen_mtimes: dict[str, float] | None = None
         self._freshness_lock = threading.Lock()
 
+        if initial_poll:
+            # Establish the baseline for the first poll; no notifications are sent on the first call.
+            with LogTime("Initialising file change notifier (polling for baseline)"):
+                self.poll_and_notify()
+
     def poll_and_notify(self) -> int:
         """
         Detects source files that were changed, created or deleted on disk since the last call

src/serena/symbol.py

@@ -5,6 +5,7 @@ import os
 from abc import ABC, abstractmethod
 from collections import OrderedDict
 from collections.abc import Callable, Iterable, Iterator, Sequence
+from contextlib import contextmanager
 from dataclasses import asdict, dataclass
 from time import perf_counter
 from typing import Any, Generic, Literal, NotRequired, Self, TypedDict, TypeVar
@@ -1261,6 +1262,7 @@ class SymbolDictGrouper(Generic[TSymbolDict], ABC):
         self._group_keys = group_keys
         self._group_children_keys = group_children_keys
         self._collapse_singleton = collapse_singleton
+        self._is_enabled = True
 
     def _group_by(self, l: list[dict], keys: list[str], children_keys: list[str], is_children: bool) -> dict[str, Any] | list[Any]:
         """
@@ -1321,8 +1323,24 @@ class SymbolDictGrouper(Generic[TSymbolDict], ABC):
         """
         # avoid side effects by working on a deep-copy
         symbols_copy = copy.deepcopy(symbols)
+
+        if not self._is_enabled:
+            return symbols_copy
+
         return self._group_by(symbols_copy, self._group_keys, self._group_children_keys, is_children=False)
 
+    @contextmanager
+    def disabled_context(self) -> Iterator[None]:
+        """
+        A context manager that temporarily disables grouping.
+        """
+        original_state = self._is_enabled
+        self._is_enabled = False
+        try:
+            yield
+        finally:
+            self._is_enabled = original_state
+
 
 class LanguageServerSymbolDictGrouper(SymbolDictGrouper[LanguageServerSymbol.OutputDict]):
     def __init__(

src/serena/tools/jetbrains_tools.py

@@ -259,7 +259,6 @@ class JetBrainsFindReferencingSymbolsTool(Tool, ToolMarkerSymbolicRead, ToolMark
 
     symbol_dict_grouper = JetBrainsSymbolDictGrouper(["relative_path", "type"], ["type"], collapse_singleton=True)
 
-    # TODO: (maybe) - add content snippets showing the references like in LS based version?
     def apply(
         self,
         name_path: str,
@@ -273,6 +272,8 @@ class JetBrainsFindReferencingSymbolsTool(Tool, ToolMarkerSymbolicRead, ToolMark
 
         :param name_path: name path of the symbol for which to find references
         :param relative_path: the relative path to the file containing the symbol (must be a file, not a directory)
+            Note: for external dependencies, this must be an identifier starting with `<ext` that you have received
+            earlier (don't try to guess!).
         :param max_answer_chars: max characters for the result (-1 for default). If exceeded, no content/a shortened result is returned.
         """
         relative_path = self._sanitize_input_param(relative_path)

src/serena/tools/symbol_tools.py

@@ -54,6 +54,8 @@ class GetSymbolsOverviewTool(Tool, ToolMarkerSymbolicRead):
             Don't adjust unless there is really no other way to get the content required for the task.
         :return: a JSON object containing symbols grouped by kind in a compact format.
         """
+        # Note: file system sync not required (relevant file is opened in the language server explicitly)
+
         if depth == -1:
             if relative_path.endswith((".java", ".kt")):
                 depth = 1
@@ -189,6 +191,8 @@ class FindSymbolTool(Tool, ToolMarkerSymbolicRead):
         :param max_answer_chars: max result length; -1 for default
         :return: symbols (with locations) matching the name.
         """
+        # Note: file system sync not required; the symbol finder opens all relevant source files explicitly in the case of changes
+
         if include_body:
             depth = 0  # ignore user-specified depth if include_body is True
         assert max_matches != 0, "max_matches must be > 0 or equal to -1."
@@ -267,14 +271,14 @@ class FindReferencingSymbolsTool(Tool, ToolMarkerSymbolicRead):
 
         :param name_path: name path of the symbol
         :param relative_path: the relative path to the file containing the symbol for which to find references.
-            Note: for external dependencies, this must be an identifier starting with `<ext` that you have received
-            earlier (don't try to guess!).
         :param include_kinds: (optional) limits results to the given LSP symbol kinds (integers)
         :param exclude_kinds: optional list of LSP symbol kinds (integers) to exclude.
         :param max_answer_chars: max result length; -1 for default
         :return: a list of JSON objects with the symbols referencing the requested symbol
         """
-        self.project.ls_sync_file_system_changes()
+        # file system sync needed for case where symbol finder does not perform a global search, updating everything
+        if relative_path:
+            self.project.ls_sync_file_system_changes()
 
         include_body = False  # It is probably never a good idea to include the body of the referencing symbols
         parsed_include_kinds: Sequence[SymbolKind] | None = [SymbolKind(k) for k in include_kinds] if include_kinds else None
@@ -685,6 +689,7 @@ class RenameSymbolTool(Tool, ToolMarkerSymbolicEdit):
         :param new_name: the new name for the symbol
         :return: result summary indicating success or failure
         """
+        self.project.ls_sync_file_system_changes()
         code_editor = self.create_ls_code_editor()
         status_message = code_editor.rename_symbol(name_path, relative_path=relative_path, new_name=new_name)
         return status_message

test/conftest.py

@@ -13,6 +13,7 @@ import pytest
 from _pytest.mark import Mark, MarkDecorator
 from sensai.util.logging import configure
 
+from serena.agent import SerenaAgent
 from serena.config.serena_config import SerenaConfig, SerenaPaths
 from serena.constants import SERENA_MANAGED_DIR_NAME
 from serena.project import Project
@@ -129,7 +130,7 @@ def start_default_ls_context(language: Language) -> Iterator[SolidLanguageServer
 
 
 def create_default_serena_config():
-    return SerenaConfig(gui_log_window=False, web_dashboard=False)
+    return SerenaConfig().with_headless_mode_overrides()
 
 
 def _create_default_project(language: Language, repo_root_override: str | None = None) -> Project:
@@ -238,6 +239,20 @@ def project_with_ls_context(language: Language, repo_root_override: str | None =
         yield project
 
 
+@contextmanager
+def agent_for_project_context(language: Language, repo_root_override: str | None = None) -> Iterator[SerenaAgent]:
+    project_root = str(get_repo_path(language)) if repo_root_override is None else repo_root_override
+    agent = SerenaAgent(project=project_root, serena_config=create_default_serena_config())
+
+    # wait for agent to be ready
+    agent.execute_task(lambda: None)
+
+    try:
+        yield agent
+    finally:
+        agent.on_shutdown()
+
+
 @pytest.fixture(scope="module")
 def project_with_ls(request: LanguageParamRequest) -> Iterator[Project]:
     if not hasattr(request, "param"):

test/serena/test_ls_file_sync.py

@@ -5,88 +5,130 @@ Exercises all three ``FileChangeType`` branches against a real pyright backend:
 Created (new caller file), Changed (append a second caller), Deleted (remove the caller file).
 """
 
+import json
 import os
 import shutil
 
 import pytest
 
-from solidlsp import SolidLanguageServer
+from serena.agent import SerenaAgent
+from serena.project import Project
+from serena.tools import FindReferencingSymbolsTool
 from solidlsp.ls_config import Language
-from test.conftest import get_repo_path, project_with_ls_context
+from test.conftest import agent_for_project_context, get_repo_path
 
 pytestmark = pytest.mark.python
 
-# A method that already has a call site in the fixture, so its baseline reference set is non-empty.
-_TARGET_FILE = os.path.join("test_repo", "services.py")
-_TARGET_SYMBOL = "create_user"
 
-_CALLER_REL_PATH = os.path.join("test_repo", "external_caller.py")
-_CALLER_ONE = "external_caller_one"
-_CALLER_TWO = "external_caller_two"
+class FileSystemSyncTestCase:
+    """
+    Tests the language server file system synchronisation behaviour, using a reference search
+    as an example of a feature that generally depends on the synchronisation.
+
+    It supports two modes:
+      * using Serena's tools (where an explicit sync is not needed because it builds on find_symbol)
+      * a low-level mode that uses LS methods directly and therefore needs the sync
+    """
+
+    # A method that already has a call site in the fixture, so its baseline reference set is non-empty.
+    _TARGET_FILE = os.path.join("test_repo", "services.py")
+    _TARGET_SYMBOL = "create_user"
+
+    _CALLER_REL_PATH = os.path.join("test_repo", "external_caller.py")
+    _CALLER_ONE = "external_caller_one"
+    _CALLER_TWO = "external_caller_two"
+
+    def __init__(self, use_serena_tool: bool, tool_use_relative_path: bool = True):
+        """
+        :param use_serena_tool: whether to use the Serena tool, which does not need an explicit FS sync
+        """
+        self._use_serena_tool = use_serena_tool
+        self._tool_use_relative_path = tool_use_relative_path
+
+    def _caller_source(self, *function_names: str) -> str:
+        body = "\n\n".join(
+            f"def {name}() -> User:\n    return UserService().{self._TARGET_SYMBOL}('x', 'y', 'z@example.com')" for name in function_names
+        )
+        return "from test_repo.models import User\nfrom test_repo.services import UserService\n\n\n" + body + "\n"
+
+    def _referencing_symbol_names(self, agent: SerenaAgent) -> list[str]:
+        """Names of the symbols that reference ``_TARGET_SYMBOL``, per the warm language server."""
+        if self._use_serena_tool:
+            tool = agent.get_tool(FindReferencingSymbolsTool)
+            with tool.symbol_dict_grouper.disabled_context():
+                relative_path = self._TARGET_FILE if self._tool_use_relative_path else ""
+                response = tool.apply(name_path=self._TARGET_SYMBOL, relative_path=relative_path)
+                ref_symbols = json.loads(response)
+                symbol_names = [ref["name_path"].split("/")[-1] for ref in ref_symbols]
+                return symbol_names
+        else:
+            ls = next(iter(agent.get_active_project_or_raise().language_server_manager.iter_language_servers()))
+            document_symbols = ls.request_document_symbols(self._TARGET_FILE).get_all_symbols_and_roots()
+            target = next((s for s in document_symbols[0] if s.get("name") == self._TARGET_SYMBOL), None)
+            assert target is not None and "selectionRange" in target, f"{self._TARGET_SYMBOL} not found in {self._TARGET_FILE}"
+            start = target["selectionRange"]["start"]
+            references = ls.request_referencing_symbols(self._TARGET_FILE, start["line"], start["character"])
+            return [ref.symbol["name"] for ref in references]
+
+    def run(self, tmp_path):
+        # Work on an isolated copy so we can freely create/edit/delete files under the project root.
+        repo_root = tmp_path / "repo"
+        shutil.copytree(get_repo_path(Language.PYTHON), repo_root)
+        caller_abs = repo_root / self._CALLER_REL_PATH
+
+        with agent_for_project_context(Language.PYTHON, str(repo_root)) as agent:
+            project = agent.get_active_project_or_raise()
+
+            # Warm the reference index, then establish the freshness baseline (first poll never notifies).
+            self._referencing_symbol_names(agent)
+            self._sync_fs(project, 0, "<baseline>")
+
+            # --- Created -------------------------------------------------------------------------------
+            caller_abs.write_text(self._caller_source(self._CALLER_ONE), encoding="utf-8")
+
+            if not self._use_serena_tool:
+                # Adversarial: without the poll, the warm server has never seen the new file -> stale.
+                assert self._CALLER_ONE not in self._referencing_symbol_names(agent), "expected the new caller to be invisible before poll"
+
+            self._sync_fs(project, 1, "Created")
+            assert self._CALLER_ONE in self._referencing_symbol_names(agent), "new caller must be visible after poll"
+
+            # --- Changed -------------------------------------------------------------------------------
+            caller_abs.write_text(self._caller_source(self._CALLER_ONE, self._CALLER_TWO), encoding="utf-8")
+            self._sync_fs(project, 1, "Changed")
+            names_after_change = self._referencing_symbol_names(agent)
+            assert self._CALLER_TWO in names_after_change, "appended caller must be visible after poll"
+            assert self._CALLER_ONE in names_after_change
+
+            # --- Deleted -------------------------------------------------------------------------------
+            caller_abs.unlink()
+            self._sync_fs(project, 1, "Deleted")
+            names_after_delete = self._referencing_symbol_names(agent)
+            assert self._CALLER_ONE not in names_after_delete, "deleted caller must disappear after poll"
+            assert self._CALLER_TWO not in names_after_delete
+
+    def _sync_fs(self, project: Project, num_expected_events: int, event_type: str):
+        # apply sync only if Serena's tool is not being used
+        if not self._use_serena_tool:
+            assert project.ls_sync_file_system_changes() == num_expected_events, f"expected {num_expected_events} {event_type} event(s)"
 
 
-def _caller_source(*function_names: str) -> str:
-    body = "\n\n".join(
-        f"def {name}() -> User:\n    return UserService().{_TARGET_SYMBOL}('x', 'y', 'z@example.com')" for name in function_names
-    )
-    return "from test_repo.models import User\nfrom test_repo.services import UserService\n\n\n" + body + "\n"
+def test_find_referencing_symbol_tool_handles_sync_with_relative_path(tmp_path):
+    """
+    Tests that the FindReferencingSymbolsTool handles file system synchronisation correctly when passed a relative path.
+    """
+    FileSystemSyncTestCase(use_serena_tool=True, tool_use_relative_path=True).run(tmp_path)
 
 
-def _referencing_symbol_names(ls: SolidLanguageServer) -> list[str]:
-    """Names of the symbols that reference ``_TARGET_SYMBOL``, per the warm language server."""
-    document_symbols = ls.request_document_symbols(_TARGET_FILE).get_all_symbols_and_roots()
-    target = next((s for s in document_symbols[0] if s.get("name") == _TARGET_SYMBOL), None)
-    assert target is not None and "selectionRange" in target, f"{_TARGET_SYMBOL} not found in {_TARGET_FILE}"
-    start = target["selectionRange"]["start"]
-    references = ls.request_referencing_symbols(_TARGET_FILE, start["line"], start["character"])
-    return [ref.symbol["name"] for ref in references]
+def test_find_referencing_symbol_tool_handles_sync_without_relative_path(tmp_path):
+    """
+    Tests that the FindReferencingSymbolsTool handles file system synchronisation correctly when no relative path is provided.
+    """
+    FileSystemSyncTestCase(use_serena_tool=True, tool_use_relative_path=False).run(tmp_path)
 
 
-def test_external_changes_become_visible_after_sync(tmp_path):
-    # Work on an isolated copy so we can freely create/edit/delete files under the project root.
-    repo_root = tmp_path / "repo"
-    shutil.copytree(get_repo_path(Language.PYTHON), repo_root)
-    caller_abs = repo_root / _CALLER_REL_PATH
-
-    with project_with_ls_context(Language.PYTHON, str(repo_root)) as project:
-        language_server = next(iter(project.language_server_manager.iter_language_servers()))
-
-        def sync_fs() -> int:
-            return project.language_server_manager.sync_file_system_changes()
-
-        # Warm the reference index, then establish the freshness baseline (first poll never notifies).
-        _referencing_symbol_names(language_server)
-        assert sync_fs() == 0, "first poll must only establish the baseline"
-
-        # --- Created -------------------------------------------------------------------------------
-        caller_abs.write_text(_caller_source(_CALLER_ONE), encoding="utf-8")
-
-        # Adversarial: without the poll, the warm server has never seen the new file -> stale.
-        assert _CALLER_ONE not in _referencing_symbol_names(language_server), "expected the new caller to be invisible before poll"
-
-        assert sync_fs() == 1, "one Created event expected"
-        assert _CALLER_ONE in _referencing_symbol_names(language_server), "new caller must be visible after poll"
-
-        # --- Changed -------------------------------------------------------------------------------
-        caller_abs.write_text(_caller_source(_CALLER_ONE, _CALLER_TWO), encoding="utf-8")
-        assert sync_fs() == 1, "one Changed event expected"
-        names_after_change = _referencing_symbol_names(language_server)
-        assert _CALLER_TWO in names_after_change, "appended caller must be visible after poll"
-        assert _CALLER_ONE in names_after_change
-
-        # --- Deleted -------------------------------------------------------------------------------
-        caller_abs.unlink()
-        assert sync_fs() == 1, "one Deleted event expected"
-        names_after_delete = _referencing_symbol_names(language_server)
-        assert _CALLER_ONE not in names_after_delete, "deleted caller must disappear after poll"
-        assert _CALLER_TWO not in names_after_delete
-
-
-def test_first_poll_is_baseline_only(tmp_path):
-    """A newly-loaded project must not report its entire pre-existing file set as changed."""
-    repo_root = tmp_path / "repo"
-    shutil.copytree(get_repo_path(Language.PYTHON), repo_root)
-    with project_with_ls_context(Language.PYTHON, str(repo_root)) as project:
-        assert project.ls_sync_file_system_changes() == 0
-        # No external edits between the two calls -> still nothing to report.
-        assert project.ls_sync_file_system_changes() == 0
+def test_ls_low_level_find_references_with_explicit_sync(tmp_path):
+    """
+    Tests that requesting references directly from the LS works if synchronisation is explicitly requested after external file changes.
+    """
+    FileSystemSyncTestCase(use_serena_tool=False).run(tmp_path)