From d7e09ede4a55b1f13a83022595148073b7930e08 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 3 Jul 2026 20:12:27 +0200 Subject: [PATCH] refactor(search): consolidate the keyword-mode query-path gate (ADR-030) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Behavior-preserving cleanup of the query-type gate merged in #1020 — no wire contract, status, or 422 change (existing unit + pact tests pass unchanged). - management.py: merge resolve_search_algorithm into select_search_algorithm (it was the sole caller, via the implicit-default branch). One function now expresses the whole contract: absent → graceful default; explicit supported → pass through; explicit unsupported → raise UnsupportedSearchType (422). - visualization.py: extract _build_search_algorithm() and use it in both unified_search and vector_search, removing the byte-identical select + try/except + if-semantic-else-bm25hybrid block from each endpoint. - viz_routes.py: the OAuth-session PCA-viz search selected the pure-dense SemanticSearchAlgorithm from a raw query param with no mode check, so in keyword mode it built a dense algorithm that fails against the sparse-only collection. Reject semantic when dense is disabled (bm25_hybrid stays valid in both modes). New unit test asserts the dense algo is never constructed. - Contract: parametrize the keyword-mode 422 pact over both /api/v1/search and /api/v1/vector-viz/search (they now share one gate). ADR-030 updated. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/ADR-030-keyword-only-search-mode.md | 7 +- nextcloud_mcp_server/api/management.py | 68 ++++--------- nextcloud_mcp_server/api/visualization.py | 86 +++++++++------- nextcloud_mcp_server/auth/viz_routes.py | 18 +++- .../test_mcp_status_search_types_consumer.py | 32 ++++-- tests/unit/test_management_status_endpoint.py | 46 +++------ .../test_viz_routes_search_algorithm_gate.py | 99 +++++++++++++++++++ 7 files changed, 220 insertions(+), 136 deletions(-) create mode 100644 tests/unit/test_viz_routes_search_algorithm_gate.py diff --git a/docs/ADR-030-keyword-only-search-mode.md b/docs/ADR-030-keyword-only-search-mode.md index 0524a5df..246565e2 100644 --- a/docs/ADR-030-keyword-only-search-mode.md +++ b/docs/ADR-030-keyword-only-search-mode.md @@ -140,9 +140,10 @@ This is deliberately stricter than the earlier behaviour (which silently coerced semantic answer; a 422 lets astrolabe *and* any other client fail loud and self-correct from the advertised set. The strictness applies only to an **explicit** request: a call that omits `algorithm` still defaults gracefully -across modes. `api/management.py` splits the two paths — `select_search_algorithm` -(explicit → raise `UnsupportedSearchType`; absent → default) wraps the retained -`resolve_search_algorithm` (the lenient default-coercion helper). +across modes. `api/management.py`'s `select_search_algorithm` is the single entry +point for both paths — explicit unsupported → raise `UnsupportedSearchType`; +absent → default to a serviceable type (`hybrid` when available, else the first +supported, i.e. `bm25` in keyword mode). Astrolabe is the **consumer** of `/api/v1/status` and `/api/v1/search`, and this server is the **provider** (ADR-029). The contract is pinned both ways: a diff --git a/nextcloud_mcp_server/api/management.py b/nextcloud_mcp_server/api/management.py index 3d7464c6..0dc7b38d 100644 --- a/nextcloud_mcp_server/api/management.py +++ b/nextcloud_mcp_server/api/management.py @@ -72,7 +72,7 @@ class UnsupportedSearchType(Exception): (ADR-030), so a client (astrolabe) that requests ``semantic`` against a keyword-only server gets a hard, self-correcting error instead of silent BM25 results. Only *explicit* requests are strict — an absent ``algorithm`` still - defaults gracefully via :func:`resolve_search_algorithm`. + defaults gracefully (see :func:`select_search_algorithm`). """ def __init__(self, requested: str, supported: list[str]) -> None: @@ -85,33 +85,32 @@ def __init__(self, requested: str, supported: list[str]) -> None: def select_search_algorithm(requested: str | None, settings: Settings) -> str: - """Pick the algorithm to run for a search request (ADR-030, strict variant). + """Pick the algorithm to run for a search request (ADR-030). - The search endpoints (`/api/v1/search`, `/api/v1/vector-viz/search`) call this - with the raw ``body.get("algorithm")`` — ``None`` when the client sent no - ``algorithm`` field: + Single source of truth for the two search endpoints (`/api/v1/search`, + `/api/v1/vector-viz/search`), called with the raw ``body.get("algorithm")`` — + ``None`` when the client sent no ``algorithm`` field: - - ``requested is None`` → graceful default via - :func:`resolve_search_algorithm` (coerces the historical ``hybrid`` default - to a serviceable type, e.g. ``bm25`` in keyword mode). Callers that don't - pin an algorithm keep working across modes. + - ``requested is None`` → graceful default: ``hybrid`` when available, else the + first supported type (``bm25`` in keyword mode). Never errors, so callers + that don't pin an algorithm keep working across modes. (An explicit JSON + ``"algorithm": null`` is indistinguishable from an omitted key — both surface + as ``None`` — so it takes this default path rather than being rejected.) - explicit ``requested`` in ``supported_search_types`` → returned unchanged. - explicit ``requested`` NOT in ``supported_search_types`` → raise :class:`UnsupportedSearchType` (→ 422). This is the strict half of the - contract that /api/v1/status advertises; the lenient - :func:`resolve_search_algorithm` remains for the implicit-default path. - - Caveat: an explicit JSON ``"algorithm": null`` is indistinguishable from an - omitted key (both surface as ``None`` from ``body.get``), so it takes the - graceful-default path rather than being rejected. + contract that /api/v1/status advertises: an explicit ``semantic`` while + ``SEARCH_MODE=keyword`` is rejected rather than silently coerced, so the + accepted set stays in lockstep with the advertised one. """ - if requested is None: - return resolve_search_algorithm("hybrid", settings) supported = supported_search_types(settings) + if requested is None: + # Empty (vector sync off) preserves the prior default of "hybrid". + return "hybrid" if "hybrid" in supported else (supported or ["hybrid"])[0] if requested in supported: return requested - # Log the hard reject (mirrors resolve_search_algorithm's coercion log) so - # operators can see why an explicit algorithm got a 422 rather than results. + # Log the hard reject so operators can see why an explicit algorithm got a 422 + # rather than results (the response search_method reflects what actually ran). logger.debug( "select_search_algorithm: rejecting explicit %r (supported=%r)", requested, @@ -120,37 +119,6 @@ def select_search_algorithm(requested: str | None, settings: Settings) -> str: raise UnsupportedSearchType(requested, supported) -def resolve_search_algorithm(algorithm: str, settings: Settings) -> str: - """Coerce a requested search algorithm to one this server can serve now. - - Falls back to a supported algorithm when the request is unknown OR - unavailable in the current SEARCH_MODE (ADR-030) — notably ``semantic`` while - ``SEARCH_MODE=keyword``, which would otherwise route a dense query at a - sparse-only index and fail. Prefers ``hybrid`` when available (the historical - default), else the first supported type. Backs the **implicit-default** path - (no ``algorithm`` in the request); an *explicit* unsupported algorithm is - rejected by :func:`select_search_algorithm` (422) rather than coerced, keeping - the accepted set in lockstep with the ``supported_search_types`` that - /api/v1/status advertises. - """ - supported = supported_search_types(settings) - if algorithm in supported: - return algorithm - # Empty (vector sync off) preserves the prior default of "hybrid". - resolved = "hybrid" if "hybrid" in supported else (supported or ["hybrid"])[0] - # Log the rewrite so operators debugging "why did I get bm25 results for an - # algorithm=semantic request" can see the coercion (the response - # search_method reflects the algorithm actually run, not the request). - logger.debug( - "resolve_search_algorithm: %r unavailable in current SEARCH_MODE " - "(supported=%r); coercing to %r", - algorithm, - supported, - resolved, - ) - return resolved - - def extract_bearer_token(request: Request) -> str | None: """Extract OAuth bearer token from Authorization header. diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 28dc7b65..a8430f21 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -27,10 +27,11 @@ select_search_algorithm, validate_token_and_get_user, ) -from nextcloud_mcp_server.config import get_settings +from nextcloud_mcp_server.config import Settings, get_settings from nextcloud_mcp_server.embedding.service import get_embedding_service from nextcloud_mcp_server.search import ( BM25HybridSearchAlgorithm, + SearchAlgorithm, SemanticSearchAlgorithm, ) from nextcloud_mcp_server.search.access_filter import ( @@ -73,6 +74,35 @@ def _unsupported_search_type_response(e: UnsupportedSearchType) -> JSONResponse: ) +def _build_search_algorithm( + requested_algorithm: str | None, + settings: Settings, + *, + score_threshold: float, + fusion: str, +) -> tuple[SearchAlgorithm, str]: + """Resolve + instantiate the search algorithm for a request (ADR-030). + + Shared by both search endpoints (`/api/v1/search`, `/api/v1/vector-viz/search`) + so their selection logic can't drift. Raises :class:`UnsupportedSearchType` + for an *explicit* unsupported algorithm (the caller maps it to a 422 via + :func:`_unsupported_search_type_response`); an absent algorithm defaults + gracefully. Invalid fusion is normalized to ``"rrf"``. Returns the + ``(algorithm instance, resolved algorithm name)``. + """ + algorithm = select_search_algorithm(requested_algorithm, settings) + if algorithm == "semantic": + return SemanticSearchAlgorithm(score_threshold=score_threshold), algorithm + # Both "bm25" and "hybrid" run BM25HybridSearchAlgorithm — it combines dense + # semantic + sparse BM25 in hybrid mode, and issues a sparse-only query in + # keyword mode (it branches on dense_enabled internally). + fusion = fusion if fusion in ("rrf", "dbsf") else "rrf" + return ( + BM25HybridSearchAlgorithm(score_threshold=score_threshold, fusion=fusion), + algorithm, + ) + + async def _search_with_acl( request: Request, user_id: str, @@ -265,29 +295,21 @@ async def unified_search(request: Request) -> JSONResponse: if not query: return JSONResponse({"results": [], "total_found": 0}) - # Pick the algorithm to run (ADR-030, strict variant): an *explicit* + # Resolve + build the search algorithm (ADR-030): an *explicit* # unsupported request (e.g. "semantic" while SEARCH_MODE=keyword) is - # rejected with 422 carrying the advertised supported_search_types, so - # the client can correct it rather than silently receive BM25 results. An + # rejected with 422 carrying the advertised supported_search_types, so the + # client can correct it rather than silently receive BM25 results. An # absent algorithm still defaults gracefully across modes. try: - algorithm = select_search_algorithm(requested_algorithm, settings) + search_algo, algorithm = _build_search_algorithm( + requested_algorithm, + settings, + score_threshold=score_threshold, + fusion=fusion, + ) except UnsupportedSearchType as e: return _unsupported_search_type_response(e) - # Validate fusion method - valid_fusions = {"rrf", "dbsf"} - if fusion not in valid_fusions: - fusion = "rrf" - - # Select search algorithm - if algorithm == "semantic": - search_algo = SemanticSearchAlgorithm(score_threshold=score_threshold) - else: - search_algo = BM25HybridSearchAlgorithm( - score_threshold=score_threshold, fusion=fusion - ) - # Request extra results to handle offset search_limit = limit + offset @@ -522,31 +544,21 @@ async def vector_search(request: Request) -> JSONResponse: status_code=400, ) - # Pick the algorithm to run (ADR-030, strict variant): an *explicit* + # Resolve + build the search algorithm (ADR-030): an *explicit* # unsupported request (e.g. "semantic" while SEARCH_MODE=keyword) is - # rejected with 422 carrying the advertised supported_search_types, so - # the client can correct it rather than silently receive BM25 results. An + # rejected with 422 carrying the advertised supported_search_types, so the + # client can correct it rather than silently receive BM25 results. An # absent algorithm still defaults gracefully across modes. try: - algorithm = select_search_algorithm(requested_algorithm, settings) + search_algo, algorithm = _build_search_algorithm( + requested_algorithm, + settings, + score_threshold=score_threshold, + fusion=fusion, + ) except UnsupportedSearchType as e: return _unsupported_search_type_response(e) - # Validate fusion method - valid_fusions = {"rrf", "dbsf"} - if fusion not in valid_fusions: - fusion = "rrf" - - # Select search algorithm - if algorithm == "semantic": - search_algo = SemanticSearchAlgorithm(score_threshold=score_threshold) - else: - # Both "hybrid" and "bm25" use the BM25HybridSearchAlgorithm - # which combines dense semantic and sparse BM25 vectors - search_algo = BM25HybridSearchAlgorithm( - score_threshold=score_threshold, fusion=fusion - ) - async def _execute(owners: list[str] | None) -> list: """Run the search across requested doc_types with the given owner scope (None ⇒ self-only).""" diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 7f14dabc..047b397b 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -204,7 +204,23 @@ async def vector_visualization_search(request: Request) -> JSONResponse: auth_client_ctx = await _get_authenticated_client_for_userinfo(request) async with auth_client_ctx as nc_client: - # Create search algorithm (no client needed - verification removed) + # Create search algorithm (no client needed - verification removed). + # ADR-030: the pure-dense "semantic" algorithm queries the dense + # vector slot, which a keyword-only collection (SEARCH_MODE=keyword) + # does not have — reject it here rather than letting the query fail + # against the sparse-only index. "bm25_hybrid" stays valid in both + # modes (it issues a sparse-only query in keyword mode internally). + if algorithm == "semantic" and not settings.dense_enabled: + return JSONResponse( + { + "success": False, + "error": ( + "semantic search is unavailable in keyword-only mode " + "(SEARCH_MODE=keyword)" + ), + }, + status_code=400, + ) if algorithm == "semantic": search_algo = SemanticSearchAlgorithm(score_threshold=score_threshold) elif algorithm == "bm25_hybrid": diff --git a/tests/contract/test_mcp_status_search_types_consumer.py b/tests/contract/test_mcp_status_search_types_consumer.py index e47702e8..b5c21f42 100644 --- a/tests/contract/test_mcp_status_search_types_consumer.py +++ b/tests/contract/test_mcp_status_search_types_consumer.py @@ -117,32 +117,44 @@ async def test_status_advertises_bm25_only_in_keyword_mode(status_pact): assert types == ["bm25"] -async def _post_search_algorithm(base_url: str, algorithm: str) -> httpx.Response: - """Stand-in for astrolabe's McpServerClient.search(): POST /api/v1/search with - an explicit ``algorithm``. Returns the raw response so the caller can assert - on the (strict, ADR-030) 422 the server sends for an unsupported algorithm.""" +async def _post_search_algorithm( + base_url: str, path: str, algorithm: str +) -> httpx.Response: + """Stand-in for astrolabe's McpServerClient search calls: POST ``path`` with an + explicit ``algorithm``. Returns the raw response so the caller can assert on + the (strict, ADR-030) 422 the server sends for an unsupported algorithm. + + Both astrolabe search entry points are exercised — ``searchForUnifiedSearch`` + (POST /api/v1/search) and ``search`` (POST /api/v1/vector-viz/search) — which + on the server share one ``_build_search_algorithm`` gate, so both must reject + an explicit unsupported algorithm identically. + """ async with httpx.AsyncClient(base_url=base_url) as client: return await client.post( - "/api/v1/search", + path, json={"query": "torch leadership award", "algorithm": algorithm}, ) -async def test_search_rejects_unsupported_algorithm_in_keyword_mode(status_pact): +@pytest.mark.parametrize("path", ["/api/v1/search", "/api/v1/vector-viz/search"]) +async def test_search_rejects_unsupported_algorithm_in_keyword_mode( + status_pact, path: str +): """Explicitly requesting ``semantic`` on a keyword-only server → 422 (ADR-030). This is the strict half of the contract: rather than silently degrading a ``semantic`` request to BM25, the server rejects it with the advertised ``supported_search_types`` so astrolabe can surface/guard the error. astrolabe also gates the request client-side from ``/api/v1/status``, but this pins the - server-side backstop the client relies on. + server-side backstop the client relies on — on **both** search endpoints, + which share one ``_build_search_algorithm`` gate. """ ( status_pact.upon_receiving( - "a semantic search request when the server is in keyword mode" + f"a semantic search request to {path} when the server is in keyword mode" ) .given("the server advertises keyword-only search support") - .with_request("POST", "/api/v1/search") + .with_request("POST", path) .with_body( {"query": "torch leadership award", "algorithm": "semantic"}, content_type="application/json", @@ -159,7 +171,7 @@ async def test_search_rejects_unsupported_algorithm_in_keyword_mode(status_pact) ) with status_pact.serve() as srv: - resp = await _post_search_algorithm(str(srv.url), "semantic") + resp = await _post_search_algorithm(str(srv.url), path, "semantic") assert resp.status_code == 422 body = resp.json() diff --git a/tests/unit/test_management_status_endpoint.py b/tests/unit/test_management_status_endpoint.py index 1ceff4be..ef4acf73 100644 --- a/tests/unit/test_management_status_endpoint.py +++ b/tests/unit/test_management_status_endpoint.py @@ -419,42 +419,11 @@ def test_keyword_mode_advertises_bm25_only(self): assert supported_search_types(s) == ["bm25"] -class TestResolveSearchAlgorithm: - """resolve_search_algorithm coerces requests to a mode-serviceable one.""" - - def test_hybrid_mode_passes_through_valid(self): - from nextcloud_mcp_server.api.management import resolve_search_algorithm - - s = create_mock_settings(vector_sync_enabled=True, dense_enabled=True) - for algo in ("semantic", "bm25", "hybrid"): - assert resolve_search_algorithm(algo, s) == algo - - def test_unknown_algorithm_falls_back_to_hybrid(self): - from nextcloud_mcp_server.api.management import resolve_search_algorithm - - s = create_mock_settings(vector_sync_enabled=True, dense_enabled=True) - assert resolve_search_algorithm("nonsense", s) == "hybrid" - - def test_keyword_mode_redirects_dense_requests_to_bm25(self): - from nextcloud_mcp_server.api.management import resolve_search_algorithm - - s = create_mock_settings(vector_sync_enabled=True, dense_enabled=False) - # "semantic" would route a dense query at a sparse-only index → bm25. - assert resolve_search_algorithm("semantic", s) == "bm25" - assert resolve_search_algorithm("hybrid", s) == "bm25" - assert resolve_search_algorithm("bm25", s) == "bm25" - - def test_vector_sync_off_preserves_hybrid_default(self): - from nextcloud_mcp_server.api.management import resolve_search_algorithm - - s = create_mock_settings(vector_sync_enabled=False) - assert resolve_search_algorithm("semantic", s) == "hybrid" - - class TestSelectSearchAlgorithm: - """select_search_algorithm is the strict variant backing the search - endpoints (ADR-030): an *explicit* unsupported algorithm raises - UnsupportedSearchType (→ 422); an absent algorithm defaults gracefully.""" + """select_search_algorithm backs the search endpoints (ADR-030): an absent + algorithm defaults gracefully across modes, an explicit supported algorithm + passes through, and an explicit unsupported one raises UnsupportedSearchType + (→ 422).""" def test_none_defaults_gracefully_in_hybrid(self): from nextcloud_mcp_server.api.management import select_search_algorithm @@ -470,6 +439,13 @@ def test_none_defaults_to_bm25_in_keyword_mode(self): # No explicit algorithm → coerced to the one serviceable type. assert select_search_algorithm(None, s) == "bm25" + def test_none_defaults_to_hybrid_when_vector_sync_off(self): + from nextcloud_mcp_server.api.management import select_search_algorithm + + # supported_search_types == [] ⇒ preserve the prior "hybrid" default. + s = create_mock_settings(vector_sync_enabled=False) + assert select_search_algorithm(None, s) == "hybrid" + def test_explicit_supported_passes_through(self): from nextcloud_mcp_server.api.management import select_search_algorithm diff --git a/tests/unit/test_viz_routes_search_algorithm_gate.py b/tests/unit/test_viz_routes_search_algorithm_gate.py new file mode 100644 index 00000000..436004ca --- /dev/null +++ b/tests/unit/test_viz_routes_search_algorithm_gate.py @@ -0,0 +1,99 @@ +"""Keyword-mode gate for the OAuth-session PCA-viz search endpoint +(`nextcloud_mcp_server.auth.viz_routes.vector_visualization_search`). + +The pure-dense ``semantic`` algorithm queries the dense vector slot, which a +keyword-only collection (SEARCH_MODE=keyword) does not have. Unlike the +management API endpoints, this route selects the algorithm from a raw query +param, so it must guard ``semantic`` explicitly (ADR-030) rather than letting the +query fail against the sparse-only index. ``bm25_hybrid`` stays valid in keyword +mode (it issues a sparse-only query internally), so it must NOT be rejected. + +Auth harness mirrors ``tests/unit/test_viz_routes_chunk_context.py``. +""" + +from unittest.mock import AsyncMock, MagicMock, patch + +import pytest +from starlette.applications import Starlette +from starlette.authentication import ( + AuthCredentials, + AuthenticationBackend, + SimpleUser, +) +from starlette.middleware import Middleware +from starlette.middleware.authentication import AuthenticationMiddleware +from starlette.routing import Route +from starlette.testclient import TestClient + +from nextcloud_mcp_server.auth.viz_routes import vector_visualization_search + +pytestmark = pytest.mark.unit + + +class _AlwaysAuthBackend(AuthenticationBackend): + async def authenticate(self, conn): + return AuthCredentials(["authenticated"]), SimpleUser("testuser") + + +def _make_app() -> Starlette: + return Starlette( + routes=[ + Route( + "/app/vector-viz/search", + vector_visualization_search, + methods=["GET"], + ), + ], + middleware=[ + # ty flags AuthenticationMiddleware against Middleware's _MiddlewareFactory + # generic (a starlette typing quirk); CI scopes ty to nextcloud_mcp_server/ + # so this only trips the local pre-commit hook on this test file. + Middleware(AuthenticationMiddleware, backend=_AlwaysAuthBackend()), # ty: ignore[invalid-argument-type] + ], + ) + + +def _keyword_mode_settings() -> MagicMock: + """Vector sync on, dense off → SEARCH_MODE=keyword (supported = ["bm25"]).""" + settings = MagicMock() + settings.vector_sync_enabled = True + settings.dense_enabled = False + settings.get_collection_name.return_value = "test-bm25-keyword" + return settings + + +def _mock_auth_client_ctx() -> MagicMock: + """An async-context-manager mock for _get_authenticated_client_for_userinfo.""" + client = MagicMock() + client.__aenter__ = AsyncMock(return_value=client) + client.__aexit__ = AsyncMock(return_value=None) + return client + + +def test_semantic_rejected_in_keyword_mode_without_building_dense_algo(): + """An explicit ``algorithm=semantic`` on a keyword-only server → 400, and the + dense SemanticSearchAlgorithm is never constructed (no dead dense query).""" + with ( + patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_keyword_mode_settings(), + ), + patch( + "nextcloud_mcp_server.auth.viz_routes._get_authenticated_client_for_userinfo", + new_callable=AsyncMock, + return_value=_mock_auth_client_ctx(), + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.SemanticSearchAlgorithm" + ) as mock_semantic, + ): + with TestClient(_make_app()) as client: + resp = client.get( + "/app/vector-viz/search?query=leadership&algorithm=semantic" + ) + + assert resp.status_code == 400 + body = resp.json() + assert body["success"] is False + assert "keyword-only" in body["error"] + mock_semantic.assert_not_called()