refactor(search): consolidate the keyword-mode query-path gate (ADR-030)

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

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.
 

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)."""

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":

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()

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