chore: address review-round-4 nits — stale delenv, upgrade hint, in-sync notes
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) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
ade42b55dc
commit
1fa4c82fd2
@@ -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**:
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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]
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user