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

[cbcoutinho]

Summary

Follow-up to #1020: a behavior-preserving cleanup of the ADR-030 keyword-mode query-path gate. No wire contract, /api/v1/status, or 422 change — the existing unit and Pact tests pass unchanged, which is the proof this is a pure refactor.

Changes

• api/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).
• api/visualization.py — extract _build_search_algorithm() used by both unified_search and vector_search, removing the byte-identical select + try/except + if-semantic-else-bm25hybrid block duplicated in each endpoint.
• auth/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. Now rejects semantic when dense is disabled (bm25_hybrid stays valid in both modes). New unit test asserts the dense algorithm 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.

Note on status codes (intentional)

The rejected-semantic case returns 400 on the session-auth /app/vector-viz/search (matching that file's local {"success": false, "error": …} + 400 convention used throughout viz_routes.py) but 422 unsupported_search_type on the astrolabe-consumed /api/v1/search + /api/v1/vector-viz/search (the Pact-covered contract). The split is deliberate: only the two API endpoints share _build_search_algorithm and are part of astrolabe's consumer contract; the browser viz route keeps its own error shape.

Out of scope (intentionally kept)

Load-bearing dense_enabled branches in bm25_hybrid.py (sparse-only vs fusion) and server/semantic.py (method_label / score_threshold); all ingest-side conditionals. The strict 422 backstop is deliberately retained (dropping it would revert to the silent semantic→bm25 coercion that motivated #1020).

Test plan

• pytest tests/unit/test_management_status_endpoint.py tests/unit/test_search_endpoint_algorithm_gate.py tests/unit/test_viz_routes_search_algorithm_gate.py (+ search suite) — 159 passed
• pytest -m contract tests/contract/test_mcp_status_search_types_consumer.py — 4 interactions (status hybrid/keyword + 422 on both endpoints)
• ruff + ty clean (pre-commit)

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Claude finished @cbcoutinho's task in 2m 10s —— View job

---

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

• Gather context (diff, changed files, CLAUDE.md conventions)
• Review api/management.py gate consolidation
• Review api/visualization.py extraction
• Review auth/viz_routes.py mode-check fix
• Review test coverage (unit + contract)
• Review ADR-030 doc update

This is a clean, well-scoped refactor — I traced the logic carefully and the "behavior-preserving" claim holds up.

api/management.py — resolve_search_algorithm is fully inlined into select_search_algorithm (nextcloud_mcp_server/api/management.py:87-121). Confirmed no other references to the removed function remain anywhere in the tree (grep -rn "resolve_search_algorithm" is empty). The None default logic ("hybrid" if "hybrid" in supported else (supported or ["hybrid"])[0]) is identical to the old resolve_search_algorithm("hybrid", settings) call it replaces, and the new test_none_defaults_to_hybrid_when_vector_sync_off test covers the previously-implicit "empty supported list" edge case.

api/visualization.py — _build_search_algorithm() (nextcloud_mcp_server/api/visualization.py:77-104) is a faithful extraction: fusion normalization is still skipped on the semantic early-return, matching the original per-endpoint blocks (fusion was always irrelevant to SemanticSearchAlgorithm, which doesn't take a fusion kwarg). Both unified_search and vector_search still capture the resolved algorithm name for their algorithm_used response field (lines 425, 627) — that plumbing survived the extraction correctly.

auth/viz_routes.py — this is a genuine (small) bug fix riding along with the refactor: previously algorithm == "semantic" unconditionally built SemanticSearchAlgorithm with no dense_enabled check, so a keyword-mode server would construct a dense-query algorithm against a sparse-only collection. The new guard (lines 210-226) rejects it with 400 before construction, and settings is correctly in scope (assigned at line 120, same function). The new unit test asserts mock_semantic.assert_not_called(), which is exactly the right thing to pin.

One observation, not a blocker: this endpoint returns 400 for the rejected-semantic case, while the same conceptual rejection on /api/v1/search and /api/v1/vector-viz/search (via UnsupportedSearchType) returns 422. That's consistent with this file's existing local convention ({"success": False, "error": ...} + 400 is used throughout viz_routes.py, e.g. lines 175, 184, 222, 682, 702), and the PR correctly scopes the parametrized 422 pact test to only the two endpoints that share _build_search_algorithm. So this is likely intentional, but worth a one-line callout in the PR description or ADR if the status-code split between the two "same bug class" gates is meant to stay permanent, since a future reader may otherwise assume it's an oversight.

Tests — the new tests/unit/test_viz_routes_search_algorithm_gate.py mirrors the harness pattern from test_viz_routes_chunk_context.py as documented, mocks get_settings and the auth-client context correctly, and the removed TestResolveSearchAlgorithm class in test_management_status_endpoint.py was for the deleted function, not orphaned coverage. tests/unit/test_search_endpoint_algorithm_gate.py is untouched by this PR and doesn't reference the removed function, consistent with the "unchanged tests still pass" claim. The contract test parametrization over both endpoints (test_mcp_status_search_types_consumer.py) correctly documents that both astrolabe entry points now share one gate.

Docs — ADR-030 update accurately reflects the single-function contract, no stale references to resolve_search_algorithm left in prose.

No bugs, security issues, or missing test coverage found. Note: I was unable to execute ruff/ty/pytest directly in this sandboxed review environment (bash tool required approval that wasn't grantable here), so I verified correctness by static tracing of the diff and cross-referencing all call sites instead — the PR's own reported test run (159 passed) plus CI (in progress at the time of this review) are the authoritative signal for the automated checks.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[cbcoutinho]

Thanks — round 1 is a clean pass (no bugs/security/coverage findings). On your one observation:

400 (viz route) vs 422 (API endpoints) for rejected semantic: intentional, and I've documented it in the PR description under "Note on status codes." The 422 unsupported_search_type shape is astrolabe's Pact-covered consumer contract and is scoped to the two /api/v1/* endpoints that share _build_search_algorithm; the session-auth /app/vector-viz/search keeps this file's local {success:false,error} + 400 convention (lines 175/184/222/682/702). The split is meant to stay.

No code change this round. Treating as merge-ready pending the integration matrix going green.