From 8457c427a50cfb51aa92a2bf903c645971b31ba6 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 8 May 2026 20:50:27 +0200 Subject: [PATCH] =?UTF-8?q?fix(chunk-context):=20address=20PR=20#767=20rev?= =?UTF-8?q?iew=20=E2=80=94=20doc=5Ftype=20filter=20parity=20+=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Add `doc_type` FieldCondition to the chunk_index-path highlighted-image Qdrant filter in both `api/visualization.py` and `auth/viz_routes.py`, matching the shape of `_get_chunk_by_index_from_qdrant`. Safe today (the block is guarded by `doc_type == "file"` and Nextcloud file IDs are globally unique) but prevents a latent bug if other doc types start storing highlighted images. - Demote `viz_routes.py` `ValueError` log from `error` to `warning` (lazy %-style) — `_parse_int_param` raises on user-supplied bad input, which is a 400 not a server error and shouldn't pollute error logs. - Hoist `effective_chunk_index` to compute once at the top of `get_chunk_with_context`, removing two duplicate assignments. - Add `test_file_doc_type_qdrant_miss_yields_fast_404` to the management endpoint tests, locking in the proxy-timeout fix contract. - Add `tests/unit/test_viz_routes_chunk_context.py` mirroring management coverage for the OAuth-session route: param forwarding (chunk_index / total_chunks), `doc_type=file` fast 404, and 400 on invalid int params. Co-Authored-By: Claude Opus 4.7 (1M context) --- nextcloud_mcp_server/api/visualization.py | 4 + nextcloud_mcp_server/auth/viz_routes.py | 7 +- nextcloud_mcp_server/search/context.py | 11 +- .../test_management_chunk_context_endpoint.py | 43 ++++ tests/unit/test_viz_routes_chunk_context.py | 194 ++++++++++++++++++ 5 files changed, 252 insertions(+), 7 deletions(-) create mode 100644 tests/unit/test_viz_routes_chunk_context.py diff --git a/nextcloud_mcp_server/api/visualization.py b/nextcloud_mcp_server/api/visualization.py index 5c5951d6..949a3977 100644 --- a/nextcloud_mcp_server/api/visualization.py +++ b/nextcloud_mcp_server/api/visualization.py @@ -592,6 +592,10 @@ async def get_chunk_context(request: Request) -> JSONResponse: FieldCondition( key="user_id", match=MatchValue(value=user_id) ), + FieldCondition( + key="doc_type", + match=MatchValue(value=doc_type), + ), chunk_filter, ] ), diff --git a/nextcloud_mcp_server/auth/viz_routes.py b/nextcloud_mcp_server/auth/viz_routes.py index 95256286..5902c4d0 100644 --- a/nextcloud_mcp_server/auth/viz_routes.py +++ b/nextcloud_mcp_server/auth/viz_routes.py @@ -646,6 +646,10 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: FieldCondition( key="user_id", match=MatchValue(value=username) ), + FieldCondition( + key="doc_type", + match=MatchValue(value=doc_type), + ), FieldCondition( key="chunk_index", match=MatchValue(value=chunk_index), @@ -716,7 +720,8 @@ async def chunk_context_endpoint(request: Request) -> JSONResponse: return JSONResponse(response_data) except ValueError as e: - logger.error(f"Invalid parameter format: {e}") + # User-supplied bad input → 400, not a server error. + logger.warning("Invalid parameter format: %s", e) return JSONResponse( {"success": False, "error": f"Invalid parameter format: {e}"}, status_code=400, diff --git a/nextcloud_mcp_server/search/context.py b/nextcloud_mcp_server/search/context.py index 94cd7281..b9c1f301 100644 --- a/nextcloud_mcp_server/search/context.py +++ b/nextcloud_mcp_server/search/context.py @@ -271,6 +271,11 @@ async def get_chunk_with_context( else (doc_id if isinstance(doc_id, int) else None) ) + # Effective chunk_index for adjacent lookups, marker insertion, and the + # response payload — keep `chunk_index is not None` distinct from this so + # the gate at line ~280 still controls *whether* to take the indexed path. + effective_chunk_index = chunk_index if chunk_index is not None else 0 + # Try to get chunk from Qdrant (fast path). # Prefer chunk_index lookup (always-indexed field) when caller supplied it; # fall back to (chunk_start, chunk_end) lookup otherwise. @@ -296,9 +301,6 @@ async def get_chunk_with_context( settings = get_settings() chunk_overlap = settings.document_chunk_overlap - # Effective chunk_index for adjacent lookups and response (default to 0) - effective_chunk_index = chunk_index if chunk_index is not None else 0 - before_context = "" after_context = "" has_before_truncation = False @@ -420,9 +422,6 @@ async def get_chunk_with_context( has_before_truncation = context_start > 0 has_after_truncation = context_end < len(full_text) - # Effective chunk_index for response (default to 0 when caller didn't supply) - effective_chunk_index = chunk_index if chunk_index is not None else 0 - # Create marked text with position markers marked_text = _insert_position_markers( before_context=before_context, diff --git a/tests/unit/test_management_chunk_context_endpoint.py b/tests/unit/test_management_chunk_context_endpoint.py index c335fce0..7e5d7d6b 100644 --- a/tests/unit/test_management_chunk_context_endpoint.py +++ b/tests/unit/test_management_chunk_context_endpoint.py @@ -258,6 +258,49 @@ class TestChunkContextCredentialPath: assert data["success"] is False assert "failed to fetch chunk context" in data["error"].lower() + def test_file_doc_type_qdrant_miss_yields_fast_404(self): + """For doc_type=file, a Qdrant miss must surface as 404 immediately + (no slow PDF re-parse fallback). Locks the proxy-timeout fix in. + + At the unit level we only assert the response shape; the + no-fallback contract itself lives in `search/context.py` and is + exercised by chunk-context tests there. + """ + mock_nc_client = _make_mock_nc_client() + + with ( + patch( + "nextcloud_mcp_server.api.visualization.validate_token_and_get_user", + new_callable=AsyncMock, + return_value=("testuser", True), + ), + patch( + "nextcloud_mcp_server.api.visualization.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.api.visualization.get_chunk_with_context", + new_callable=AsyncMock, + return_value=None, + ) as mock_get_chunk, + ): + app = create_test_app() + client = TestClient(app) + response = client.get( + "/api/v1/chunk-context?doc_type=file&doc_id=12345" + "&start=0&end=10&chunk_index=3&total_chunks=20", + headers={"Authorization": "Bearer test-token"}, + ) + assert response.status_code == 404 + data = response.json() + assert data["success"] is False + # Confirm the handler called the resolver with doc_type=file + # (not a coerced/normalized value) so the fast-fail path engages. + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["doc_type"] == "file" + assert kwargs["chunk_index"] == 3 + class TestChunkContextParameterForwarding: """Verify new chunk_index / total_chunks query params reach the lookup. diff --git a/tests/unit/test_viz_routes_chunk_context.py b/tests/unit/test_viz_routes_chunk_context.py new file mode 100644 index 00000000..26e550cc --- /dev/null +++ b/tests/unit/test_viz_routes_chunk_context.py @@ -0,0 +1,194 @@ +"""Unit tests for the OAuth-session chunk-context endpoint +(`nextcloud_mcp_server.auth.viz_routes.chunk_context_endpoint`). + +Mirrors the regression coverage of +`tests/unit/test_management_chunk_context_endpoint.py` (which targets the +management API route in `nextcloud_mcp_server.api.visualization`). + +Both routes share the same purpose — fetch chunk text with surrounding +context for the viz pane — but live behind different auth surfaces: + +* Management API: OAuth bearer validated by `validate_token_and_get_user` +* Viz route: Starlette session auth via `@requires("authenticated")` + +Because of the auth-middleware difference, a separate file is cleaner than +mixing both styles into one test module. +""" + +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 chunk_context_endpoint + +pytestmark = pytest.mark.unit + + +class _AlwaysAuthBackend(AuthenticationBackend): + """Stub auth backend: every request is authenticated as `testuser`.""" + + async def authenticate(self, conn): + return AuthCredentials(["authenticated"]), SimpleUser("testuser") + + +def _make_app() -> Starlette: + return Starlette( + routes=[ + Route("/app/chunk-context", chunk_context_endpoint, methods=["GET"]), + ], + middleware=[ + Middleware(AuthenticationMiddleware, backend=_AlwaysAuthBackend()), + ], + ) + + +def _make_mock_chunk_context(chunk_text="chunk", before="before", after="after"): + """Mock a ChunkContext dataclass with enough fields for the handler.""" + ctx = MagicMock() + ctx.chunk_text = chunk_text + ctx.before_context = before + ctx.after_context = after + ctx.has_before_truncation = False + ctx.has_after_truncation = False + ctx.page_number = None + ctx.chunk_index = 0 + ctx.total_chunks = 1 + return ctx + + +def _make_mock_nc_client(): + """Mock NextcloudClient that supports `async with`.""" + mock_client = MagicMock() + mock_client.__aenter__ = AsyncMock(return_value=mock_client) + mock_client.__aexit__ = AsyncMock(return_value=None) + return mock_client + + +def _make_mock_settings(nextcloud_host: str = "http://localhost:8080") -> MagicMock: + """Mock get_settings() return value with the fields the handler reads.""" + settings = MagicMock() + settings.nextcloud_host = nextcloud_host + settings.get_collection_name.return_value = "test-collection" + return settings + + +class TestVizChunkContextParameterForwarding: + """Regression guard mirroring TestChunkContextParameterForwarding for the + management API: chunk_index / total_chunks must reach get_chunk_with_context. + """ + + def test_chunk_index_and_total_chunks_forwarded(self): + mock_nc_client = _make_mock_nc_client() + mock_ctx = _make_mock_chunk_context() + mock_ctx.chunk_index = 7 + mock_ctx.total_chunks = 10 + + with ( + patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_make_mock_settings(), + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_chunk_with_context", + new_callable=AsyncMock, + return_value=mock_ctx, + ) as mock_get_chunk, + ): + with TestClient(_make_app()) as client: + response = client.get( + "/app/chunk-context?doc_type=note&doc_id=42" + "&start=0&end=10&chunk_index=7&total_chunks=10" + ) + + assert response.status_code == 200 + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["chunk_index"] == 7 + assert kwargs["total_chunks"] == 10 + + data = response.json() + assert data["chunk_index"] == 7 + assert data["total_chunks"] == 10 + # page_number must be present even when None — the response shape + # is unconditional so the frontend can rely on the key existing. + assert "page_number" in data + + +class TestVizChunkContextFile404: + """When get_chunk_with_context returns None for doc_type=file, the route + must surface a fast 404 — no slow PDF re-parse fallback. This guards the + proxy-timeout fix from PR #767. + """ + + def test_file_doc_type_qdrant_miss_yields_fast_404(self): + mock_nc_client = _make_mock_nc_client() + + with ( + patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_make_mock_settings(), + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_user_client_basic_auth", + new_callable=AsyncMock, + return_value=mock_nc_client, + ), + patch( + "nextcloud_mcp_server.auth.viz_routes.get_chunk_with_context", + new_callable=AsyncMock, + return_value=None, + ) as mock_get_chunk, + ): + with TestClient(_make_app()) as client: + response = client.get( + "/app/chunk-context?doc_type=file&doc_id=12345" + "&start=0&end=10&chunk_index=3&total_chunks=20" + ) + + assert response.status_code == 404 + data = response.json() + assert data["success"] is False + assert "failed to fetch chunk context" in data["error"].lower() + + kwargs = mock_get_chunk.await_args.kwargs + assert kwargs["doc_type"] == "file" + assert kwargs["chunk_index"] == 3 + assert kwargs["total_chunks"] == 20 + + +class TestVizChunkContextValueErrorLogging: + """Verify the route returns 400 for malformed integer params (and does + not crash with a 500). The log-level demotion (logger.warning) is + asserted indirectly via response shape — log-level itself is not a + behaviour the user can observe through HTTP. + """ + + def test_invalid_int_param_returns_400(self): + with patch( + "nextcloud_mcp_server.auth.viz_routes.get_settings", + return_value=_make_mock_settings(), + ): + with TestClient(_make_app()) as client: + response = client.get( + "/app/chunk-context?doc_type=note&doc_id=1" + "&start=not-a-number&end=10" + ) + + assert response.status_code == 400 + data = response.json() + assert data["success"] is False + assert "invalid" in data["error"].lower()