From ea108140ab55dfa0f8f1ed697a52ed3aa9054863 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 3 Jun 2026 13:10:14 +0200 Subject: [PATCH] fix(search): cap path_prefixes at the MCP tool; widen path filter tests Round 2 review follow-ups: - Add Field(max_length=20) to the nc_semantic_search path_prefixes param so an LLM client can't build an unbounded OR-filter (mirrors the cap the Astrolabe PHP controller applies on the UI path). - Note in normalize_path_prefixes that the two-pass collect-then-strip is deliberate (the `if path_prefix:` guard is truthy for whitespace-only input; the strip pass is what drops it). - Tests: exercise build_base_filter_conditions with 3 folders (guards the list comprehension) and parametrize the no-path case over None, empty list, and blank-only inputs. Co-Authored-By: Claude Opus 4.8 (1M context) --- nextcloud_mcp_server/search/access_filter.py | 3 ++ nextcloud_mcp_server/server/semantic.py | 2 ++ tests/unit/search/test_access_filter.py | 30 ++++++++++++++++---- 3 files changed, 29 insertions(+), 6 deletions(-) diff --git a/nextcloud_mcp_server/search/access_filter.py b/nextcloud_mcp_server/search/access_filter.py index 05a0d76a..7f75f326 100644 --- a/nextcloud_mcp_server/search/access_filter.py +++ b/nextcloud_mcp_server/search/access_filter.py @@ -217,6 +217,9 @@ def normalize_path_prefixes( if path_prefixes: raw.extend(path_prefixes) + # Two-pass on purpose: the ``if path_prefix:`` guard above is truthy for a + # whitespace-only string like ``" "``, so the strip-and-drop pass below is + # what actually removes it — collecting first keeps the dedup order stable. seen: set[str] = set() cleaned: list[str] = [] for value in raw: diff --git a/nextcloud_mcp_server/server/semantic.py b/nextcloud_mcp_server/server/semantic.py index 6e5276ad..371f96cb 100644 --- a/nextcloud_mcp_server/server/semantic.py +++ b/nextcloud_mcp_server/server/semantic.py @@ -102,11 +102,13 @@ def configure_semantic_tools(mcp: FastMCP): path_prefixes: Annotated[ list[str] | None, Field( + max_length=20, description=( "Restrict to files under any of these folders/paths " "(e.g. ['/Projects/Reports', '/Shared/Specs']). Folders are " "OR-ed together. Matches the file_path of indexed files " "only, so setting it implicitly limits results to files. " + "Capped at 20 folders to bound the OR-filter width. " "None or empty = no path filter." ), ), diff --git a/tests/unit/search/test_access_filter.py b/tests/unit/search/test_access_filter.py index df29098c..6c9faa4c 100644 --- a/tests/unit/search/test_access_filter.py +++ b/tests/unit/search/test_access_filter.py @@ -265,11 +265,23 @@ class TestBuildBaseFilterConditions: assert match.text == prefix @pytest.mark.unit - def test_no_path_condition_when_prefix_absent(self) -> None: - conditions = build_base_filter_conditions("alice", None, path_prefix=None) + @pytest.mark.parametrize( + "kwargs", + [ + {"path_prefix": None}, + {"path_prefixes": None}, + {"path_prefixes": []}, + {"path_prefix": " ", "path_prefixes": ["", " "]}, + ], + ) + def test_no_path_condition_when_prefix_absent(self, kwargs) -> None: + # No folder filter (None, empty list, or blank-only) must add neither a + # flat file_path condition nor a nested path OR. + conditions = build_base_filter_conditions("alice", None, **kwargs) assert not any( isinstance(c, FieldCondition) and c.key == "file_path" for c in conditions ) + assert self._path_should_texts(conditions) is None @staticmethod def _path_should_texts(conditions) -> set[str] | None: @@ -292,12 +304,18 @@ class TestBuildBaseFilterConditions: @pytest.mark.unit def test_multiple_path_prefixes_or_in_nested_should(self) -> None: - # Two+ folders must OR together: a single nested Filter(should=[...]) is - # appended (not two must conditions, which would AND and match nothing). + # 3+ folders must OR together: a single nested Filter(should=[...]) is + # appended (not separate must conditions, which would AND and match + # nothing). 3 folders also guards the list comprehension against an + # off-by-one. conditions = build_base_filter_conditions( - "alice", None, path_prefixes=["/Projects", "/Archive"] + "alice", None, path_prefixes=["/Projects", "/Archive", "/Shared"] ) - assert self._path_should_texts(conditions) == {"/Projects", "/Archive"} + assert self._path_should_texts(conditions) == { + "/Projects", + "/Archive", + "/Shared", + } # No bare file_path FieldCondition in must for the multi-folder case. assert not any( isinstance(c, FieldCondition) and c.key == "file_path" for c in conditions