From 1fa4c82fd28d0f93c4cfebf03106535021a64c85 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 12 May 2026 22:35:50 +0200 Subject: [PATCH] =?UTF-8?q?chore:=20address=20review-round-4=20nits=20?= =?UTF-8?q?=E2=80=94=20stale=20delenv,=20upgrade=20hint,=20in-sync=20notes?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four small follow-ups from the reviewer's latest pass: - tests/unit/test_stdio.py:18: the single_user_env fixture used monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", ...). That env var is no longer read after the ADR-022 follow-up; switched to delenv of MCP_DEPLOYMENT_MODE which is the canonical mode-selection input today. Comment updated to match. - config_validators.py: when detect_auth_mode rejects an invalid MCP_DEPLOYMENT_MODE, surface a one-line ADR-022 migration hint if the rejected value is exactly "oauth_single_audience" (the most common upgrade pain — users carrying that value over from ADR-021 .env files). Other invalid values get the regular "Valid values: …" message unchanged. - config.py + config_validators.py: added cross-reference comments on both mode-resolution sites (Settings.__post_init__ and detect_auth_mode) noting that they each compute the canonical mode independently and must be kept in sync when a new mode is added. Surfaces the parallel-duplication intentionally so the next maintainer doesn't have to discover it. - docs/ADR-021-configuration-consolidation.md:92: appended a trailing comment to the historical "valid values" example, marking oauth_single_audience and oauth_token_exchange as removed in ADR-022. ADR-021 stays as the historical record; the trailer points future readers at the current state. No functional changes; 1009 unit tests still pass. Co-Authored-By: Claude Opus 4.7 (1M context) --- docs/ADR-021-configuration-consolidation.md | 1 + nextcloud_mcp_server/config.py | 8 ++++++++ nextcloud_mcp_server/config_validators.py | 16 +++++++++++++++- tests/unit/test_stdio.py | 4 ++-- 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/docs/ADR-021-configuration-consolidation.md b/docs/ADR-021-configuration-consolidation.md index 513f8448..6298a754 100644 --- a/docs/ADR-021-configuration-consolidation.md +++ b/docs/ADR-021-configuration-consolidation.md @@ -90,6 +90,7 @@ MCP_DEPLOYMENT_MODE=login_flow # Valid values: single_user_basic, multi_user_basic, # oauth_single_audience, oauth_token_exchange +# (both OAuth values removed in ADR-022 — current value: login_flow) ``` **Detection logic**: diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index ece2d641..62eca989 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -636,6 +636,14 @@ class Settings: "of truth for selecting an auth flow." ) + # NOTE: this block mirrors the resolution logic in + # `config_validators.detect_auth_mode` (which works on strings via a + # `mode_map`). Both call sites resolve the deployment mode + # independently — the canonical AuthMode enum in detect_auth_mode, + # and the boolean derived flags here. **Keep them in sync when + # adding a new mode**: a new entry must be added in both places, in + # addition to `mode_map` (`config_validators.py`) and any + # MODE_REQUIREMENTS entry. resolved_mode = (self.deployment_mode or "").strip().lower() if not resolved_mode: if self.nextcloud_username and self.nextcloud_password: diff --git a/nextcloud_mcp_server/config_validators.py b/nextcloud_mcp_server/config_validators.py index 78af22d7..0b190659 100644 --- a/nextcloud_mcp_server/config_validators.py +++ b/nextcloud_mcp_server/config_validators.py @@ -172,6 +172,11 @@ def detect_auth_mode(settings: Settings) -> AuthMode: `Settings.__post_init__` so every Settings instance carries correct flags regardless of how it was constructed. + Keep the resolution logic here in sync with `Settings.__post_init__`: + both compute the canonical mode from `deployment_mode` (+ credentials + as a fallback). When adding a new mode, update `mode_map` *and* the + `__post_init__` resolution block in `config.py`. + Args: settings: Application settings @@ -196,9 +201,18 @@ def detect_auth_mode(settings: Settings) -> AuthMode: if mode_str not in mode_map: valid_modes = ", ".join(mode_map.keys()) + # ADR-022 migration hint: the most common upgrade pain is users + # carrying MCP_DEPLOYMENT_MODE=oauth_single_audience over from + # ADR-021. Surface a one-liner so they don't have to grep the + # changelog. + hint = ( + " (Note: 'oauth_single_audience' was renamed to 'login_flow' in ADR-022.)" + if mode_str == "oauth_single_audience" + else "" + ) raise ValueError( f"Invalid MCP_DEPLOYMENT_MODE: '{settings.deployment_mode}'. " - f"Valid values: {valid_modes}" + f"Valid values: {valid_modes}.{hint}" ) explicit_mode = mode_map[mode_str] diff --git a/tests/unit/test_stdio.py b/tests/unit/test_stdio.py index a70053be..cd2418ae 100644 --- a/tests/unit/test_stdio.py +++ b/tests/unit/test_stdio.py @@ -14,8 +14,8 @@ def single_user_env(monkeypatch): monkeypatch.setenv("NEXTCLOUD_HOST", "https://cloud.example.com") monkeypatch.setenv("NEXTCLOUD_USERNAME", "admin") monkeypatch.setenv("NEXTCLOUD_PASSWORD", "secret") - # Ensure multi-user mode is off (may leak from other tests) - monkeypatch.delenv("ENABLE_MULTI_USER_BASIC_AUTH", raising=False) + # Ensure no explicit deployment mode leaks from other tests + monkeypatch.delenv("MCP_DEPLOYMENT_MODE", raising=False) _reload_config() yield _reload_config()