From 704a537847aee9a17bda950b12a9e6d071a87ee3 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Wed, 3 Jun 2026 15:44:59 +0200 Subject: [PATCH] chore: round-4 polish + standardize on module-level loggers Round-4 review (non-blocking) items: - get_procrastinate_conninfo: warn on an empty connect_timeout= value (it falls back to the 10s default); preserve an explicit connect_timeout=0. - Document the _doc_queueing_lock user_id invariant (NC rejects ':' in usernames). - docs/configuration.md: note that `db downgrade` leaves procrastinate's tables in place and how to drop them on a full teardown. - reclaim_stalled_ingest_jobs: debug heartbeat log when nothing is stalled. - Drop the redundant list() wrap in the integration stalled-jobs assertion. Logging pattern: define a module-level `logger = logging.getLogger(__name__)` and use it instead of function-local or inline getLogger(__name__) calls (config.py, config_validators.py, tests/.../test_scope_authorization.py). The test file's dev-only `scripts.*` import gets a ty: ignore since it resolves via sys.path at runtime, not as an installed package. Co-Authored-By: Claude Opus 4.8 (1M context) --- docs/configuration.md | 7 +++++ nextcloud_mcp_server/config.py | 14 ++++++---- nextcloud_mcp_server/config_validators.py | 2 -- .../vector/queue/procrastinate.py | 19 +++++++++---- .../integration/test_ingest_queue_postgres.py | 8 ++---- .../login_flow/test_scope_authorization.py | 28 ++++++++----------- 6 files changed, 44 insertions(+), 34 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index fb0a38bc..ae6d229b 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -834,6 +834,13 @@ Notes: - KEDA scales the worker on `SELECT count(*) FROM procrastinate_jobs WHERE queue_name='ingest' AND status='todo'`. - `INGEST_QUEUE=postgres` with a SQLite `DATABASE_URL` is rejected at startup. +- **Teardown:** because procrastinate's schema is a separate lineage, + `nextcloud-mcp-server db downgrade` (Alembic) does **not** drop the + `procrastinate_*` tables. To fully revert (e.g. back to NATS or SQLite-only), + drop them manually after downgrading: + `DROP TABLE IF EXISTS procrastinate_jobs, procrastinate_events, + procrastinate_periodic_defers, procrastinate_workers CASCADE;` (plus the + `procrastinate_*` types/functions if removing the extension entirely). --- diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 3b44bd3d..37516c13 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -11,6 +11,8 @@ from typing import Any from dynaconf import Dynaconf, Validator +logger = logging.getLogger(__name__) + # Sentinel for "key not in dynaconf at all" vs "explicitly set to None". _UNSET = object() @@ -734,7 +736,6 @@ class Settings: def __post_init__(self): """Validate configuration and set defaults.""" - logger = logging.getLogger(__name__) # Validate SSL/TLS configuration if not self.nextcloud_verify_ssl: @@ -1064,8 +1065,6 @@ def _get_semantic_search_enabled() -> bool: Returns: True if semantic search should be enabled """ - logger = logging.getLogger(__name__) - new_value = _dynaconf.get("ENABLE_SEMANTIC_SEARCH", False) old_value = _dynaconf.get("VECTOR_SYNC_ENABLED", False) @@ -1146,7 +1145,6 @@ def _log_bg_ops_advisories_once( return _bg_ops_advisories_logged = True - logger = logging.getLogger(__name__) if explicit and legacy: logger.warning( "Both ENABLE_BACKGROUND_OPERATIONS and ENABLE_OFFLINE_ACCESS are set. " @@ -1473,7 +1471,7 @@ def get_procrastinate_conninfo(database_url: str | None = None) -> str: # query string is dropped with a warning. dropped = sorted(k for k in url.query if k != "connect_timeout") if dropped: - logging.getLogger(__name__).warning( + logger.warning( "Dropping DATABASE_URL query parameters not forwarded to the " "procrastinate connector: %s", ", ".join(dropped), @@ -1492,10 +1490,14 @@ def get_procrastinate_conninfo(database_url: str | None = None) -> str: params["password"] = url.password # Honor an operator-supplied connect_timeout, else default to 10s. (make_url # query values are str or a tuple of strs when repeated; take the last.) + # ``connect_timeout=0`` (disable) is preserved — only a missing/empty value + # falls back to the default, and an explicit-but-empty value is flagged. _ct = url.query.get("connect_timeout") if isinstance(_ct, (list, tuple)): _ct = _ct[-1] if _ct else None - params["connect_timeout"] = str(_ct) if _ct else "10" + if _ct == "": + logger.warning("DATABASE_URL has an empty connect_timeout=; using default 10s") + params["connect_timeout"] = _ct if _ct else "10" params.update(_pg_ssl_params()) return make_conninfo(**params) diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index b13f5050..a341013e 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -187,8 +187,6 @@ def detect_auth_mode(settings: Settings) -> AuthMode: ValueError: If explicit deployment_mode is unrecognised. """ - logger = logging.getLogger(__name__) - # ADR-021: explicit deployment mode wins if settings.deployment_mode: mode_str = settings.deployment_mode.lower().strip() diff --git a/nextcloud_mcp_server/vector/queue/procrastinate.py b/nextcloud_mcp_server/vector/queue/procrastinate.py index 049f3eaa..e7b13f92 100644 --- a/nextcloud_mcp_server/vector/queue/procrastinate.py +++ b/nextcloud_mcp_server/vector/queue/procrastinate.py @@ -136,6 +136,10 @@ async def reclaim_stalled_ingest_jobs(context: JobContext, timestamp: int) -> No reclaimed += 1 if reclaimed: logger.warning("ingest.reclaimed_stalled_jobs count=%d", reclaimed) + else: + # Visible heartbeat under verbose logging when debugging a suspected + # reclaim failure, without noising up production logs. + logger.debug("ingest.reclaim_check stalled=0") async def _resolve_client(user_id: str) -> NextcloudClient: @@ -293,11 +297,16 @@ def _doc_queueing_lock(task: DocumentTask) -> str: """Per-document enqueue-dedup key (partial-unique on ``status='todo'``). Collision-safe with a raw ``:`` delimiter because the first two segments can - never themselves contain ``:``: ``user_id`` is a Nextcloud username/UID (no - colons) and ``doc_type`` is a controlled enum (``note``/``file``/ - ``deck_card``/``news_item``). The trailing ``doc_id`` may contain anything — - it's the final unambiguous segment. A future ``doc_type`` containing ``:`` - would break this invariant, so keep doc_type colon-free. + never themselves contain ``:``: + + - ``user_id`` — a Nextcloud username/UID; Nextcloud rejects ``:`` in + usernames (Web UI + provisioning validation), so it is colon-free. + - ``doc_type`` — a controlled enum (``note``/``file``/``deck_card``/ + ``news_item``); a future ``doc_type`` containing ``:`` would break this + invariant, so keep doc_type colon-free. + + The trailing ``doc_id`` may contain anything — it's the final unambiguous + segment. """ return f"{task.user_id}:{task.doc_type}:{task.doc_id}" diff --git a/tests/integration/test_ingest_queue_postgres.py b/tests/integration/test_ingest_queue_postgres.py index 4b03dd24..6ac0027c 100644 --- a/tests/integration/test_ingest_queue_postgres.py +++ b/tests/integration/test_ingest_queue_postgres.py @@ -133,9 +133,7 @@ async def test_ingest_queue_end_to_end(fresh_app): assert counts.get("todo") == 2 # 4. Fresh todo jobs are not "doing", so none are stalled. - stalled = list( - await fresh_app.job_manager.get_stalled_jobs( - queue=INGEST_QUEUE_NAME, seconds_since_heartbeat=0 - ) + stalled = await fresh_app.job_manager.get_stalled_jobs( + queue=INGEST_QUEUE_NAME, seconds_since_heartbeat=0 ) - assert stalled == [] + assert list(stalled) == [] diff --git a/tests/server/login_flow/test_scope_authorization.py b/tests/server/login_flow/test_scope_authorization.py index 426bd06b..e68d7f28 100644 --- a/tests/server/login_flow/test_scope_authorization.py +++ b/tests/server/login_flow/test_scope_authorization.py @@ -16,6 +16,8 @@ import logging import httpx import pytest +logger = logging.getLogger(__name__) + @pytest.mark.integration @pytest.mark.login_flow @@ -65,8 +67,6 @@ async def test_basicauth_shows_all_tools(nc_mcp_client): async def test_read_only_token_filters_write_tools(nc_mcp_login_flow_client_read_only): """Test that a token with only read scopes filters out write tools.""" - logger = logging.getLogger(__name__) - # Connect with token that has only "notes.read" scope result = await nc_mcp_login_flow_client_read_only.list_tools() assert result is not None @@ -114,8 +114,6 @@ async def test_read_only_token_filters_write_tools(nc_mcp_login_flow_client_read async def test_write_only_token_filters_read_tools(nc_mcp_login_flow_client_write_only): """Test that a token with only write scopes filters out read tools.""" - logger = logging.getLogger(__name__) - # Connect with token that has only "notes.write" scope result = await nc_mcp_login_flow_client_write_only.list_tools() assert result is not None @@ -163,8 +161,6 @@ async def test_write_only_token_filters_read_tools(nc_mcp_login_flow_client_writ async def test_full_access_token_shows_all_tools(nc_mcp_login_flow_client_full_access): """Test that a token with both read and write scopes scopes can see all tools.""" - logger = logging.getLogger(__name__) - # Connect with token that has both "notes.read" and "notes.write" scopes result = await nc_mcp_login_flow_client_full_access.list_tools() assert result is not None @@ -305,7 +301,11 @@ async def test_tools_have_scope_decorators(nc_mcp_client): @pytest.mark.integration async def test_scope_classification(): """Test that our scope classification correctly identifies read vs write operations.""" - from scripts.add_scope_decorators_simple import classify_function + # `scripts/` is a dev-only helper dir (not an installed package); resolved + # at runtime via the repo root on sys.path, so ty can't see it. + from scripts.add_scope_decorators_simple import ( # ty: ignore[unresolved-import] + classify_function, + ) # Test read operations assert classify_function("nc_notes_get_note") == "notes.read" @@ -336,7 +336,11 @@ async def test_scope_classification(): @pytest.mark.integration async def test_all_tools_classified(): """Verify that all tools can be properly classified as read or write.""" - from scripts.add_scope_decorators_simple import classify_function + # `scripts/` is a dev-only helper dir (not an installed package); resolved + # at runtime via the repo root on sys.path, so ty can't see it. + from scripts.add_scope_decorators_simple import ( # ty: ignore[unresolved-import] + classify_function, + ) # List of all tool names (extracted from our implementation) all_tools = [ @@ -407,8 +411,6 @@ async def test_jwt_with_no_custom_scopes_returns_zero_tools( so users can provision Nextcloud access after authentication """ - logger = logging.getLogger(__name__) - # Connect with JWT token that has NO custom scopes (only openid, profile, email) result = await nc_mcp_login_flow_client_no_custom_scopes.list_tools() assert result is not None @@ -451,8 +453,6 @@ async def test_jwt_consent_scenarios_read_only(nc_mcp_login_flow_client_read_onl Expected: Should see read tools but not write tools. """ - logger = logging.getLogger(__name__) - result = await nc_mcp_login_flow_client_read_only.list_tools() assert result is not None assert len(result.tools) > 0 @@ -490,8 +490,6 @@ async def test_jwt_consent_scenarios_write_only(nc_mcp_login_flow_client_write_o Expected: Should see write tools but not read-only tools. """ - logger = logging.getLogger(__name__) - result = await nc_mcp_login_flow_client_write_only.list_tools() assert result is not None assert len(result.tools) > 0 @@ -529,8 +527,6 @@ async def test_jwt_consent_scenarios_full_access(nc_mcp_login_flow_client_full_a Expected: Should see all 90+ tools (both read and write). """ - logger = logging.getLogger(__name__) - result = await nc_mcp_login_flow_client_full_access.list_tools() assert result is not None assert len(result.tools) > 0