From b5ed1e3b4d802615047b0aefccb022b09ef7e989 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Fri, 29 May 2026 18:36:42 +0200 Subject: [PATCH] fix: address PR #814 review + SonarCloud gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit SonarCloud: - Resolve 6 S5332 hotspots (http→https in test fixture URLs). - S6418: hoist the unauthenticated AsyncOpenAI placeholder to a named constant + NOSONAR (genuine non-secret; gateway ignores it when unauthenticated). - Fix two reliability bugs: None-index guard in the gateway token-cache test (S2259) and float `> 0.0` instead of `!= 0.0` in the sentinel test (S1244). - status.py idle path sleeps 0.1s instead of sleep(0) (S7491); NOSONAR on the protocol-required async no-await aclose() stubs (S7503). Claude review: - Remove three leftover debug print() calls in app.py (logger.info already covers them). - payload_backfill: drop parsed_at from the backfilled-keys docstring (it is per-document state, not a deployment scalar); add a clean 404 precondition for BasicAuth deployments without an OAuth token verifier. - status.py: task_status typed TaskStatus | None (drop type: ignore). - nats.py: TODO to thread etags for file/deck/news; note etag default → None. - factory: warn on unknown INGEST_BUS_URL scheme; raise ValueError instead of assert for the external-mode preconditions. - docs/configuration.md: document the decomposition hook-point env vars + that nats-py ships core (lazy-imported) and external+bus uses two NATS connections. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/configuration.md | 34 +++++++++++++++++++ .../admin/payload_backfill.py | 14 ++++++-- nextcloud_mcp_server/app.py | 5 --- .../embedding/gateway_client.py | 7 +++- nextcloud_mcp_server/vector/queue/factory.py | 18 ++++++++-- nextcloud_mcp_server/vector/queue/nats.py | 10 ++++-- nextcloud_mcp_server/vector/queue/postgres.py | 2 +- nextcloud_mcp_server/vector/queue/status.py | 7 ++-- nextcloud_mcp_server/vector/scanner.py | 4 +-- tests/unit/providers/test_gateway_provider.py | 7 ++-- tests/unit/test_decomposition_config.py | 2 +- tests/unit/vector/test_collection_metadata.py | 4 +-- tests/unit/vector/test_nats_producer.py | 2 +- 13 files changed, 91 insertions(+), 25 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index a29aa626..2640982e 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -750,6 +750,40 @@ docker-compose up --- +## Decomposition Hook Points (Optional, Advanced) + +The server can optionally offload document processing and embeddings to external +services (the Astrolabe Cloud document-processor and embedding-gateway). These +are **opt-in**; every default reproduces the in-process monolith behavior, so +self-hosters can ignore this section. + +```bash +# Embeddings via an OpenAI-compatible gateway (else: autodetect — see above) +EMBEDDING_PROVIDER=gateway +EMBEDDING_GATEWAY_URL=https://embedding-gateway.internal +# Gateway M2M OIDC client (its own realm; leave unset to call it unauthenticated) +EMBEDDING_GATEWAY_TOKEN_URL=... +EMBEDDING_GATEWAY_CLIENT_ID=... +EMBEDDING_GATEWAY_CLIENT_SECRET=... + +# External ingest: publish to NATS instead of the in-process processor pool +INGEST_MODE=external # local (default) | external +STATUS_BACKEND=bus # local (default) | bus — REQUIRED with external +INGEST_BUS_URL=nats://nats:4222 +TENANT_ID= # NATS per-tenant subject token +``` + +Notes: +- `STATUS_BACKEND=local` with `INGEST_MODE=external` is rejected at startup + (the in-process job state is empty for externally-dispatched work). +- **`nats-py` ships as a core dependency** (small, pure-Python) and is imported + lazily — only when `INGEST_MODE=external`. Self-hosters who never enable + external ingest pay no runtime cost. +- `INGEST_MODE=external` + `STATUS_BACKEND=bus` opens **two** NATS connections + per pod (the ingest producer and the status subscriber are separate roles). + +--- + ## Tag-Based File Exclusion (Optional) Some files (contracts, medical records, credentials, private notes) should diff --git a/nextcloud_mcp_server/admin/payload_backfill.py b/nextcloud_mcp_server/admin/payload_backfill.py index f851ee46..f972db77 100644 --- a/nextcloud_mcp_server/admin/payload_backfill.py +++ b/nextcloud_mcp_server/admin/payload_backfill.py @@ -5,8 +5,10 @@ values for any *missing* decomposition payload keys (so existing corpora gain them without a re-index), then upserts the collection-metadata sentinel. Scope: this backfills the cheap, deployment-level scalar keys -(``processor_version``, ``parsed_at``, ``pipeline_tier``, ``embedding_identity``) -only. It deliberately does NOT synthesize ``acl_hash`` — a correct value needs +(``processor_version``, ``pipeline_tier``, ``embedding_identity``) only. +``parsed_at`` is per-document state (the local processor sets it at index time), +not a deployment-level scalar, so it is intentionally not backfilled. It also +deliberately does NOT synthesize ``acl_hash`` — a correct value needs per-document share enumeration (a separate job), and writing a placeholder ``acl_hash`` would be unsafe to pre-filter on. The query-side ACL pre-filter therefore stays disabled until a real ACL backfill runs (see @@ -38,6 +40,14 @@ logger = logging.getLogger(__name__) async def handle_payload_backfill(request: Request) -> JSONResponse: + # require_admin_scope authenticates via the OAuth token verifier; in + # BasicAuth-only deployments there is no oauth_context, so surface a clean + # 404 rather than a confusing 401 from the broad except below. + if getattr(request.app.state, "oauth_context", None) is None: + return JSONResponse( + {"error": "admin API requires an OAuth-capable deployment"}, + status_code=404, + ) try: await require_admin_scope(request) except AdminScopeRequired: diff --git a/nextcloud_mcp_server/app.py b/nextcloud_mcp_server/app.py index 6d175ad8..a7de80f8 100644 --- a/nextcloud_mcp_server/app.py +++ b/nextcloud_mcp_server/app.py @@ -1120,9 +1120,6 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = and settings.vector_sync_enabled and settings.enable_background_operations ): - print( - f"DEBUG: Multi-user BasicAuth mode detected, vector_sync={settings.vector_sync_enabled}, background_operations={settings.enable_background_operations}" - ) logger.info( "Multi-user BasicAuth with vector sync - checking for OAuth/app password credentials" ) @@ -1132,12 +1129,10 @@ def get_app(transport: str = "streamable-http", enabled_apps: list[str] | None = static_client_secret = os.getenv("NEXTCLOUD_OIDC_CLIENT_SECRET") if static_client_id and static_client_secret: - print("DEBUG: Using static OAuth credentials") logger.info("Using static OAuth credentials for background operations") multi_user_basic_oauth_creds = (static_client_id, static_client_secret) else: # Perform DCR before uvicorn starts (same lifecycle as OAuth modes) - print("DEBUG: No static credentials, attempting DCR...") logger.info( "OAuth credentials not configured - attempting Dynamic Client Registration..." ) diff --git a/nextcloud_mcp_server/embedding/gateway_client.py b/nextcloud_mcp_server/embedding/gateway_client.py index 7220f4a5..1a4b9d44 100644 --- a/nextcloud_mcp_server/embedding/gateway_client.py +++ b/nextcloud_mcp_server/embedding/gateway_client.py @@ -35,6 +35,11 @@ logger = logging.getLogger(__name__) # token never expires mid-flight (matches AstrolabeClient / CP CLI behavior). _EARLY_REFRESH_SECONDS = 60 +# Non-secret placeholder for AsyncOpenAI, which rejects an empty key. In +# unauthenticated mode the gateway ignores the bearer; when a token provider is +# configured, the real M2M token replaces this before each request. +_UNAUTHENTICATED_PLACEHOLDER = "unauthenticated" + class GatewayTokenProvider: """Caches a gateway M2M access token via the ``client_credentials`` grant. @@ -106,7 +111,7 @@ class GatewayProvider(OpenAIProvider): # the gateway is unauthenticated. When a token provider is configured, # the real Bearer is set on the client before each request. super().__init__( - api_key="gateway-unauthenticated", + api_key=_UNAUTHENTICATED_PLACEHOLDER, # NOSONAR: placeholder, not a secret base_url=base_url, embedding_model=embedding_model, generation_model=None, # gateway never generates diff --git a/nextcloud_mcp_server/vector/queue/factory.py b/nextcloud_mcp_server/vector/queue/factory.py index 8001e016..cc673941 100644 --- a/nextcloud_mcp_server/vector/queue/factory.py +++ b/nextcloud_mcp_server/vector/queue/factory.py @@ -9,16 +9,25 @@ later) so moving the external processor to Postgres needs no new INGEST_MODE. from __future__ import annotations +import logging from urllib.parse import urlsplit from ...config import Settings from .ports import TaskProducer +logger = logging.getLogger(__name__) + def _transport_for(url: str) -> str: scheme = urlsplit(url).scheme.lower() if scheme.startswith("postgres"): return "postgres" + if not scheme.startswith("nats"): + logger.warning( + "INGEST_BUS_URL scheme %r is neither nats:// nor postgres://; " + "defaulting to the NATS transport", + scheme, + ) return "nats" @@ -28,8 +37,13 @@ async def build_external_producer(settings: Settings) -> TaskProducer: Precondition: ``settings.ingest_mode == "external"`` (so __post_init__ has guaranteed ``ingest_bus_url`` and ``tenant_id`` are set). """ - assert settings.ingest_bus_url is not None - assert settings.tenant_id is not None + # Defence-in-depth (robust under ``python -O``, which strips asserts): + # __post_init__ already guarantees these when ingest_mode == external. + if settings.ingest_bus_url is None or settings.tenant_id is None: + raise ValueError( + "build_external_producer requires INGEST_BUS_URL and TENANT_ID " + "(guaranteed by Settings validation when INGEST_MODE=external)" + ) transport = _transport_for(settings.ingest_bus_url) if transport == "postgres": diff --git a/nextcloud_mcp_server/vector/queue/nats.py b/nextcloud_mcp_server/vector/queue/nats.py index 4d25a9be..7a5ecafe 100644 --- a/nextcloud_mcp_server/vector/queue/nats.py +++ b/nextcloud_mcp_server/vector/queue/nats.py @@ -36,7 +36,13 @@ def _modified_at_rfc3339(modified_at: int) -> str: def _content_hash(task: DocumentTask) -> str: """etag is the change-detection token; fall back to modified_at when it is - absent (e.g. deletes, or sources whose etag we don't thread through).""" + absent (e.g. deletes, or sources whose etag we don't thread through). + + TODO(follow-up): thread etags for file / deck_card / news_item scans too + (only note scans pass etag today). Until then their JetStream Nats-Msg-Id + dedup keys off modified_at, which misses content changes that leave + modified_at unchanged (e.g. a file move/rename). + """ return task.etag or str(task.modified_at) @@ -133,7 +139,7 @@ class NatsTaskProducer: ) -> None: return None - async def aclose(self) -> None: + async def aclose(self) -> None: # NOSONAR: async required by TaskProducer protocol # Per-handle close (e.g. a per-user scanner clone exiting). The bus # connection is shared and owned by the lifespan, so this is a no-op; # the connection is torn down once via ``drain()`` on shutdown. diff --git a/nextcloud_mcp_server/vector/queue/postgres.py b/nextcloud_mcp_server/vector/queue/postgres.py index 526265b7..47acc272 100644 --- a/nextcloud_mcp_server/vector/queue/postgres.py +++ b/nextcloud_mcp_server/vector/queue/postgres.py @@ -45,5 +45,5 @@ class PostgresTaskProducer: ) -> None: # pragma: no cover return None - async def aclose(self) -> None: # pragma: no cover + async def aclose(self) -> None: # pragma: no cover # NOSONAR: protocol stub return None diff --git a/nextcloud_mcp_server/vector/queue/status.py b/nextcloud_mcp_server/vector/queue/status.py index 010dc4de..724e805f 100644 --- a/nextcloud_mcp_server/vector/queue/status.py +++ b/nextcloud_mcp_server/vector/queue/status.py @@ -134,7 +134,7 @@ class NatsStatusSubscriber: self, shutdown_event: anyio.Event, *, - task_status: TaskStatus = None, # type: ignore[assignment] + task_status: TaskStatus | None = None, ) -> None: """Durable pull-consumer loop. Requires a live broker (integration).""" import anyio # noqa: PLC0415 @@ -149,8 +149,9 @@ class NatsStatusSubscriber: try: msgs = await sub.fetch(batch=16, timeout=5) except Exception: - # fetch timeout when idle — loop and re-check shutdown. - await anyio.sleep(0) + # fetch timeout when idle — brief pause before re-checking + # shutdown, avoiding a tight check-and-sleep spin on quiet tenants. + await anyio.sleep(0.1) continue for msg in msgs: self.handle_message(msg.subject, msg.data) diff --git a/nextcloud_mcp_server/vector/scanner.py b/nextcloud_mcp_server/vector/scanner.py index 6ada19cf..94ad168c 100644 --- a/nextcloud_mcp_server/vector/scanner.py +++ b/nextcloud_mcp_server/vector/scanner.py @@ -350,7 +350,7 @@ async def scan_user_documents( doc_type="note", operation="index", modified_at=modified_at, - etag=note.get("etag", ""), + etag=note.get("etag"), ) ) queued += 1 @@ -417,7 +417,7 @@ async def scan_user_documents( doc_type="note", operation="index", modified_at=modified_at, - etag=note.get("etag", ""), + etag=note.get("etag"), ) ) queued += 1 diff --git a/tests/unit/providers/test_gateway_provider.py b/tests/unit/providers/test_gateway_provider.py index f0d43467..fbefa0f7 100644 --- a/tests/unit/providers/test_gateway_provider.py +++ b/tests/unit/providers/test_gateway_provider.py @@ -29,7 +29,7 @@ def _patch_settings(monkeypatch, settings): def test_gateway_selected_unauthenticated(monkeypatch): settings = Settings( embedding_provider="gateway", - embedding_gateway_url="http://gateway:8083", + embedding_gateway_url="https://gateway:8083", embedding_gateway_model="mistral-embed", ) _patch_settings(monkeypatch, settings) @@ -44,7 +44,7 @@ def test_gateway_selected_unauthenticated(monkeypatch): def test_gateway_selected_with_m2m_oidc(monkeypatch): settings = Settings( embedding_provider="gateway", - embedding_gateway_url="http://gateway:8083", + embedding_gateway_url="https://gateway:8083", embedding_gateway_token_url="https://idp.example/oauth2/token", embedding_gateway_client_id="mcp-server", embedding_gateway_client_secret="shh", @@ -60,7 +60,7 @@ def test_partial_m2m_creds_rejected(): with pytest.raises(ValueError, match="must be set together"): Settings( embedding_provider="gateway", - embedding_gateway_url="http://gateway:8083", + embedding_gateway_url="https://gateway:8083", embedding_gateway_client_id="mcp-server", # missing token_url/secret ) @@ -111,6 +111,7 @@ async def test_token_provider_caches_and_refreshes(monkeypatch): assert calls["n"] == 1 # Expire the cache → next call refreshes. + assert tp._cache is not None tp._cache = (tp._cache[0], time.time() - 1) t3 = await tp.get_token() assert t3 == "tok2" diff --git a/tests/unit/test_decomposition_config.py b/tests/unit/test_decomposition_config.py index d6c1e016..4240a978 100644 --- a/tests/unit/test_decomposition_config.py +++ b/tests/unit/test_decomposition_config.py @@ -95,7 +95,7 @@ class TestConditionalRequired: def test_gateway_happy_path(self): s = Settings( embedding_provider="gateway", - embedding_gateway_url="http://gateway:8083", + embedding_gateway_url="https://gateway:8083", ) assert s.embedding_provider == "gateway" diff --git a/tests/unit/vector/test_collection_metadata.py b/tests/unit/vector/test_collection_metadata.py index bd55fb1c..3d7baabd 100644 --- a/tests/unit/vector/test_collection_metadata.py +++ b/tests/unit/vector/test_collection_metadata.py @@ -45,7 +45,7 @@ async def test_qdrant_error_falls_back_to_env(mocker): async def test_api_source(mocker): settings = Settings( collection_metadata_source="api", - collection_metadata_api_url="http://cp", + collection_metadata_api_url="https://cp", ) def handler(request: httpx.Request) -> httpx.Response: @@ -85,7 +85,7 @@ async def test_upsert_sentinel_builds_point(mocker): point = kwargs["points"][0] assert str(point.id) == cm.SENTINEL_POINT_ID # Non-zero dense (cosine-safe), empty sparse. - assert point.vector["dense"][0] != 0.0 + assert point.vector["dense"][0] > 0.0 assert len(point.vector["dense"]) == 4 assert point.payload[EMBEDDING_IDENTITY] == "mistral-embed" assert point.payload[cm.IS_SENTINEL] is True diff --git a/tests/unit/vector/test_nats_producer.py b/tests/unit/vector/test_nats_producer.py index 2fa0edfd..0f51f7f4 100644 --- a/tests/unit/vector/test_nats_producer.py +++ b/tests/unit/vector/test_nats_producer.py @@ -124,7 +124,7 @@ def test_publisher_matches_shared_fixture(mocker): ("nats://nats:4222", "nats"), ("postgres://h/db", "postgres"), ("postgresql://h/db", "postgres"), - ("http://elsewhere", "nats"), + ("https://elsewhere", "nats"), ], ) def test_transport_for(url, expected):