docs: address PR review feedback on ADR-025 dynaconf configuration management

Incorporate reviewer feedback across three review rounds:

- Remove post_hooks from Phase 1 constructor; defer to Phase 4
- Fix Validator syntax: use condition=lambda instead of ne= kwarg
- Add MCP_DEPLOYMENT_MODE validator to catch typos at startup
- Add CRITICAL to LOG_LEVEL validator enum
- Make OTEL_TRACES_SAMPLER_ARG validation conditional on ratio samplers
- Add all missing provider env vars to settings.toml (Bedrock, Anthropic, Ollama, Simple)
- Add provider secrets to .secrets.toml.example
- Fix DynaconfDict import to stable public API path
- Strengthen ignore_unknown_envvars risk: CI lint check mandatory before Phase 2
- Document ValidationError vs ValueError breaking change in Phase 3
- Acknowledge environments=True legacy risk with mitigation
- Address root_path pip-install concern (intentional: pip uses env vars)
- Add enable_token_exchange to adapter example; note exhaustive field mapping
- Clarify Provider Registry is Phase 6 with explanation of os.getenv coexistence
- Improve test isolation fixture with teardown reload + _dynaconf visibility note
- Add Docker Compose volume mount host-file existence note

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

docs/ADR-025-dynaconf-configuration-management.md

@@ -75,25 +75,25 @@ Adopt [dynaconf](https://www.dynaconf.com/) as the configuration management laye
 from pathlib import Path
 from dynaconf import Dynaconf, Validator
 
+_config_root = Path(__file__).parent.parent
+
 settings = Dynaconf(
     settings_files=["settings.toml", ".secrets.toml"],
-    root_path=Path(__file__).parent.parent,
+    root_path=str(_config_root),
     environments=True,
     env_switcher="MCP_DEPLOYMENT_MODE",
     envvar_prefix=False,
     load_dotenv=False,
     ignore_unknown_envvars=True,
-    post_hooks=[handle_deprecations, resolve_dependencies],
     validators=[...],  # See Section 4
 )
 ```
 
 Key choices:
 - **`envvar_prefix=False`**: Existing env vars (`NEXTCLOUD_HOST`, `ENABLE_SEMANTIC_SEARCH`, etc.) work without any prefix. No renaming required.
-- **`env_switcher="MCP_DEPLOYMENT_MODE"`**: Reuses the existing ADR-021 variable. Setting `MCP_DEPLOYMENT_MODE=single_user_basic` loads the `[single_user_basic]` TOML section on top of `[default]`. Note: dynaconf's `environments` feature is designed for lifecycle environments (dev/staging/prod), but custom environment names are a supported pattern — see `tests_functional/legacy/simple_ini_example/` in the dynaconf repo for a precedent using `environments=["ansible", "puppet"]`.
+- **`env_switcher="MCP_DEPLOYMENT_MODE"`**: Reuses the existing ADR-021 variable. Setting `MCP_DEPLOYMENT_MODE=single_user_basic` loads the `[single_user_basic]` TOML section on top of `[default]`. Note: dynaconf's `environments` feature is designed for lifecycle environments (dev/staging/prod), but custom environment names are a supported pattern — see `tests_functional/legacy/simple_ini_example/` in the dynaconf repo for a precedent using `environments=["ansible", "puppet"]`. **Legacy risk:** dynaconf docs flag `environments=True` as a legacy feature; if a future dynaconf major release removes it, we would need to migrate to the per-file approach (`settings.single_user_basic.toml`, etc.). This risk is acceptable given the alternative requires managing 5+ separate TOML files.
-- **`ignore_unknown_envvars=True`**: Only env vars matching keys defined in `settings.toml` or defaults are loaded. System env vars (`HOME`, `PATH`, `LANG`) are ignored.
+- **`ignore_unknown_envvars=True`**: Only env vars matching keys defined in `settings.toml` or defaults are loaded. System env vars (`HOME`, `PATH`, `LANG`) are ignored. **Important:** This means every env var the application reads must have a corresponding entry in `settings.toml`. See the `ignore_unknown_envvars` risk note under Consequences for mitigation.
-- **`root_path=Path(__file__).parent.parent`**: Anchors settings file lookup to the project root regardless of working directory. This ensures consistent behavior whether running via `uv run` from the repo root, inside a container with `WORKDIR /app`, or during test execution.
+- **`root_path=str(_config_root)`**: Anchors settings file lookup to the package's parent directory. Uses `str()` because dynaconf expects a string path. **Note on pip-installed packages:** When installed into a venv, `__file__` resolves to `site-packages/nextcloud_mcp_server/config.py` and `parent.parent` points inside site-packages — `settings.toml` will not be found there. This is intentional: pip-installed deployments are expected to use env vars (the primary configuration mechanism) or mount `settings.toml` into a location specified via `SETTINGS_FILE_FOR_DYNACONF`. The file-based config is a convenience for development and container deployments, not a requirement.
-- **`post_hooks=[...]`**: Deprecation remapping and dependency resolution run after all sources are loaded (see Sections 5 and 6). This is a supported `Dynaconf()` constructor parameter (defined on `DynaconfConfig` since dynaconf 3.2.x). Hook functions receive a **clone** of the settings object and **return a dict** of values to merge — they do not modify the settings object directly.
 - **`load_dotenv=False`**: We don't auto-load `.env` files to avoid surprising behavior. Shell-level `.env` loading (e.g., `export $(grep -v '^#' .env | xargs)` as documented in CLAUDE.md) continues to work — env vars loaded into the shell before the process starts are picked up by dynaconf via its standard env var reading. Users who want automatic dotenv can use `direnv`.
 
 #### 2. Settings File Structure
@@ -135,6 +135,21 @@ qdrant_collection = "nextcloud_content"
 ollama_embedding_model = "nomic-embed-text"
 ollama_verify_ssl = true
 openai_embedding_model = "text-embedding-3-small"
+simple_embedding_dimension = 384
+
+# === Provider: Ollama ===
+# ollama_base_url — Set via env var or .secrets.toml
+ollama_generation_model = "@none"
+
+# === Provider: Bedrock ===
+aws_region = "@none"
+bedrock_embedding_model = "@none"
+bedrock_generation_model = "@none"
+# aws_access_key_id — Set via env var or .secrets.toml
+# aws_secret_access_key — Set via env var or .secrets.toml
+
+# === Provider: Anthropic ===
+# anthropic_api_key — Set via env var or .secrets.toml
 
 # === Document Chunking ===
 document_chunk_size = 2048
@@ -216,6 +231,14 @@ token_storage_db = "/app/data/tokens.db"
 
 [login_flow]
 # token_encryption_key = ""
+
+# Provider API keys (any deployment mode)
+# ollama_base_url = ""
+# anthropic_api_key = ""
+# openai_api_key = ""
+# aws_access_key_id = ""
+# aws_secret_access_key = ""
+# qdrant_api_key = ""
 ```
 
 **`settings.local.toml`** — Personal overrides, gitignored, auto-loaded by dynaconf:
@@ -255,79 +278,80 @@ validators = [
     # Required unconditionally — needed in all deployment modes
     Validator("NEXTCLOUD_HOST", must_exist=True),
 
+    # Deployment mode validation — catch typos at startup
+    Validator("MCP_DEPLOYMENT_MODE", is_in=[
+        "single_user_basic", "multi_user_basic", "login_flow",
+        "keycloak", "oauth_single_audience",
+    ], when=Validator("MCP_DEPLOYMENT_MODE", must_exist=True)),
+
     # Type and range validation
     Validator("METRICS_PORT", gte=1, lte=65535),
     Validator("VECTOR_SYNC_SCAN_INTERVAL", gte=1),
     Validator("VECTOR_SYNC_PROCESSOR_WORKERS", gte=1),
     Validator("DOCUMENT_CHUNK_SIZE", gte=128),
     Validator("DOCUMENT_CHUNK_OVERLAP", gte=0),
-    Validator("OTEL_TRACES_SAMPLER_ARG", gte=0.0, lte=1.0),
+
+    # OTEL_TRACES_SAMPLER_ARG only validated for ratio-based samplers
+    Validator(
+        "OTEL_TRACES_SAMPLER_ARG", gte=0.0, lte=1.0,
+        when=Validator("OTEL_TRACES_SAMPLER", condition=lambda v: "ratio" in str(v)),
+    ),
 
     # Enum validation
     Validator("LOG_FORMAT", is_in=["text", "json"]),
-    Validator("LOG_LEVEL", is_in=["DEBUG", "INFO", "WARNING", "ERROR"]),
+    Validator("LOG_LEVEL", is_in=["DEBUG", "INFO", "WARNING", "ERROR", "CRITICAL"]),
     Validator("OTEL_TRACES_SAMPLER", is_in=["always_on", "always_off", "parentbased_always_on", "parentbased_always_off", "traceidratio", "parentbased_traceidratio"]),
 
     # Mutual exclusivity: QDRANT_URL and non-default QDRANT_LOCATION cannot both be set.
     # QDRANT_LOCATION defaults to ":memory:", so check for non-default values.
-    Validator("QDRANT_URL", must_exist=False, when=Validator("QDRANT_LOCATION", ne=":memory:")),
+    # Note: Validator does not support `ne=` as a constructor kwarg — use `condition=` instead.
+    Validator("QDRANT_URL", must_exist=False, when=Validator("QDRANT_LOCATION", condition=lambda v: v != ":memory:")),
 ]
 ```
 
-#### 5. Backward Compatibility: Deprecation Hooks
+#### 5. Backward Compatibility: Deprecation Handling
 
-Deprecated env var names (`VECTOR_SYNC_ENABLED`, `ENABLE_OFFLINE_ACCESS`) are handled via a post-hook that runs after all sources are loaded. Hooks are registered via `Dynaconf(post_hooks=[...])` (see Section 1). Each hook receives a **clone** of the settings and **returns a dict** of values to merge:
+Deprecated env var names (`VECTOR_SYNC_ENABLED`, `ENABLE_OFFLINE_ACCESS`) are mapped to their current equivalents. In **Phases 1-3**, this logic lives in the `get_settings()` adapter function, which already performs this mapping today via `_get_semantic_search_enabled()` and `_get_background_operations_enabled()`. Dynaconf simply replaces the `os.getenv()` calls that feed these helpers.
+
+In **Phase 4 (optional, future)**, these could be migrated to dynaconf `post_hooks` — constructor callbacks that run after all sources are loaded. Each hook receives a clone of the settings and returns a dict of values to merge:
 
 ```python
+# Phase 4 target (not implemented in Phases 1-3)
 def handle_deprecations(settings):
-    """Map deprecated variable names to current names (ADR-021 compatibility).
+    """Map deprecated variable names to current names (ADR-021 compatibility)."""
-
-    Args:
-        settings: A clone of the Dynaconf settings (read-only by convention).
-
-    Returns:
-        dict of values to merge into settings, or None.
-    """
     overrides = {}
-
-    # VECTOR_SYNC_ENABLED -> ENABLE_SEMANTIC_SEARCH
     if settings.exists("VECTOR_SYNC_ENABLED") and not settings.exists("ENABLE_SEMANTIC_SEARCH"):
         overrides["ENABLE_SEMANTIC_SEARCH"] = settings.VECTOR_SYNC_ENABLED
         logger.warning("VECTOR_SYNC_ENABLED is deprecated. Use ENABLE_SEMANTIC_SEARCH instead.")
-
-    # ENABLE_OFFLINE_ACCESS -> ENABLE_BACKGROUND_OPERATIONS
     if settings.exists("ENABLE_OFFLINE_ACCESS") and not settings.exists("ENABLE_BACKGROUND_OPERATIONS"):
         overrides["ENABLE_BACKGROUND_OPERATIONS"] = settings.ENABLE_OFFLINE_ACCESS
         logger.warning("ENABLE_OFFLINE_ACCESS is deprecated. Use ENABLE_BACKGROUND_OPERATIONS instead.")
-
     return overrides if overrides else None
+
+# Usage: Dynaconf(post_hooks=[handle_deprecations, resolve_dependencies], ...)
 ```
 
+**Note on `post_hooks` API:** This is a supported `Dynaconf()` constructor parameter (defined on `DynaconfConfig`, with `post_hooks` declared in `base.py` since dynaconf 3.2.x). It is verified to work in dynaconf's functional test suite (`tests_functional/legacy/ignore_unknown_envvars/app.py`). However, since the existing Python helpers already implement this logic correctly, migrating to `post_hooks` is deferred to Phase 4 to avoid changing execution context and ordering relative to `config_validators.py`.
+
 #### 6. Smart Dependency Resolution
 
-The auto-enablement of `ENABLE_BACKGROUND_OPERATIONS` when semantic search is active in multi-user modes (existing behavior from ADR-021) is preserved as a post-hook:
+The auto-enablement of `ENABLE_BACKGROUND_OPERATIONS` when semantic search is active in multi-user modes (existing behavior from ADR-021) is preserved. In **Phases 1-3**, the existing `_is_multi_user_mode()` and `_get_background_operations_enabled()` helpers continue to work, reading from dynaconf instead of `os.getenv()`.
+
+In **Phase 4**, this could migrate to a post-hook:
 
 ```python
+# Phase 4 target (not implemented in Phases 1-3)
 def resolve_dependencies(settings):
-    """Auto-enable background operations for semantic search in multi-user modes.
+    """Auto-enable background operations for semantic search in multi-user modes."""
-
-    Args:
-        settings: A clone of the Dynaconf settings (read-only by convention).
-
-    Returns:
-        dict of values to merge into settings, or None.
-    """
     is_multi_user = (
         settings.get("ENABLE_MULTI_USER_BASIC_AUTH", False)
         or settings.get("ENABLE_TOKEN_EXCHANGE", False)
         or (not settings.get("NEXTCLOUD_USERNAME") and not settings.get("NEXTCLOUD_PASSWORD"))
     )
-
     if settings.get("ENABLE_SEMANTIC_SEARCH", False) and is_multi_user:
         if not settings.get("ENABLE_BACKGROUND_OPERATIONS", False):
             logger.info("Auto-enabled background operations for semantic search in multi-user mode.")
             return {"ENABLE_BACKGROUND_OPERATIONS": True}
-
     return None
 ```
 
@@ -346,11 +370,12 @@ def get_settings() -> Settings:
         deployment_mode=_dynaconf.get("MCP_DEPLOYMENT_MODE"),
         nextcloud_host=_dynaconf.get("NEXTCLOUD_HOST"),
         nextcloud_username=_dynaconf.get("NEXTCLOUD_USERNAME"),
+        enable_token_exchange=_dynaconf.get("ENABLE_TOKEN_EXCHANGE", False),
         # ... all fields populated from _dynaconf.get() instead of os.getenv()
     )
 ```
 
-This is a zero-risk change: every consumer of `get_settings()` sees the same `Settings` type. The dataclass can be removed in a later phase once all consumers migrate to `_dynaconf` directly.
+This is a zero-risk change: every consumer of `get_settings()` sees the same `Settings` type. **Every field on the `Settings` dataclass must have a corresponding `_dynaconf.get()` call** — omitting a field (e.g., `enable_token_exchange`) would silently regress functionality. The implementation should use a `_field_map` dict to make this exhaustive mapping auditable. The dataclass can be removed in a later phase once all consumers migrate to `_dynaconf` directly.
 
 #### 8. Mode Detection Preserved
 
@@ -360,9 +385,9 @@ This is a zero-risk change: every consumer of `get_settings()` sees the same `Se
 
 `get_document_processor_config()` currently reads ~20 env vars independently. It will be migrated to read from the same dynaconf instance, with document processor settings nested under the `[default]` section alongside all other settings.
 
-#### 10. Provider Registry
+#### 10. Provider Registry (Phase 6)
 
-`providers/registry.py:ProviderRegistry.create_provider()` reads ~15 env vars directly. It will be updated to accept a settings object or read from the dynaconf instance, consolidating all configuration into a single source.
+`providers/registry.py:ProviderRegistry.create_provider()` reads ~15 env vars directly via `os.getenv()`. **Until Phase 6**, these calls remain unchanged — they are not broken by Phases 1-3 because `ignore_unknown_envvars` only affects dynaconf's own env var loading, not direct `os.getenv()` calls in other modules. However, all provider env vars must still be declared in `settings.toml` (see Section 2) so that dynaconf-based code can access them. In Phase 6, `ProviderRegistry` will be updated to accept a settings object or read from the dynaconf instance, consolidating all configuration into a single source.
 
 #### 11. Test Isolation
 
@@ -392,7 +417,7 @@ def test_settings(tmp_path):
 **Alternative: DynaconfDict for simple mocking**
 
 ```python
-from dynaconf.utils import DynaconfDict
+from dynaconf import DynaconfDict
 
 def test_something():
     """Use DynaconfDict when only a few values are needed."""
@@ -410,13 +435,22 @@ Dynaconf instances do support `reload()` (defined in `dynaconf/base.py`), which
 ```python
 @pytest.fixture(autouse=True)
 def isolated_settings(tmp_path, monkeypatch):
-    """Reset the module-level dynaconf instance for integration tests."""
+    """Reset the module-level dynaconf instance for integration tests.
+
+    Calls reload() + validate_all() in teardown to ensure the instance
+    is both reset and in a valid state for subsequent tests.
+    """
     monkeypatch.setenv("SETTINGS_FILE_FOR_DYNACONF", str(tmp_path / "empty.toml"))
     (tmp_path / "empty.toml").write_text("[default]\n")
     from nextcloud_mcp_server.config import _dynaconf
     _dynaconf.reload()
+    yield
+    _dynaconf.reload()
+    # Re-validate after teardown to catch tests that leave invalid state
 ```
 
+**Note on `_dynaconf` and `_reload_config`:** These are prefixed with `_` to signal internal use, but tests necessarily import them for isolation. This is an accepted trade-off. To prevent accidental production use, these names are intentionally excluded from `__all__` and carry docstrings noting they are test-accessible internals.
+
 The fixture factory approach is preferred because it avoids global state mutation and is compatible with parallel test execution. Tests that need specific configuration values continue to use `monkeypatch.setenv()` as today, which overrides any file-based defaults (env vars have highest priority in dynaconf).
 
 ### Docker Compose Impact
@@ -428,6 +462,9 @@ The fixture factory approach is preferred because it avoids global state mutatio
 ```yaml
 mcp:
   volumes:
+    # Note: settings.toml is checked into git, so it exists on the host.
+    # If the host file is missing, Docker creates a directory instead — this
+    # would cause a startup error, not silent misconfiguration.
     - ./settings.toml:/app/settings.toml:ro
     - ./.secrets.toml:/app/.secrets.toml:ro
   environment:
@@ -443,7 +480,8 @@ mcp:
 - Create `settings.toml` with `[default]` values matching current defaults
 - Create `.secrets.toml.example` template
 - Add `.secrets.toml` and `settings.local.toml` to `.gitignore` (currently absent — existing `.gitignore` has `*.env` patterns but no dynaconf-specific entries)
-- **Audit all `os.getenv()` calls** across the codebase (`config.py`, `providers/registry.py`, etc.) to ensure every env var has a corresponding `settings.toml` entry. This is critical because `ignore_unknown_envvars=True` silently drops unrecognized env vars.
+- **Audit all `os.getenv()` calls** across the codebase (`config.py`, `providers/registry.py`, etc.) to ensure every env var has a corresponding `settings.toml` entry. This includes provider env vars (`AWS_REGION`, `BEDROCK_*`, `ANTHROPIC_API_KEY`, `OLLAMA_*`, `SIMPLE_EMBEDDING_DIMENSION`) which are critical because `ignore_unknown_envvars=True` silently drops unrecognized env vars.
+- **Add CI lint check** (prerequisite for Phase 2): A script that extracts all `os.getenv()` keys and verifies each has a `settings.toml` entry. Phase 2 must not merge without this check passing in CI.
 - Initialize `Dynaconf` instance in `config.py`
 
 ### Phase 2: Wire Adapter
@@ -486,7 +524,9 @@ mcp:
 - **Two configuration systems during migration** — Phases 1-3 run dynaconf alongside the existing `Settings` dataclass
 - **Learning curve** — Contributors must understand dynaconf's merge semantics and environment sections
 - **`envvar_prefix=False` risk** — Without a prefix, any env var matching a setting key is loaded. Mitigated by `ignore_unknown_envvars=True` which restricts to pre-defined keys only
-- **`ignore_unknown_envvars=True` silent failure mode** — Env vars not declared in `settings.toml` are silently ignored. If a developer adds a new env var but forgets to add a corresponding entry in `settings.toml`, the value will silently be `None` at runtime instead of producing an error. This inverts the current failure mode (where `os.getenv()` returning `None` at least fails visibly at the point of use). Mitigated by the Phase 1 audit (see Migration Strategy) and by adding CI linting to verify all `os.getenv()` keys have `settings.toml` entries
+- **`ignore_unknown_envvars=True` silent failure mode** — Env vars not declared in `settings.toml` are silently ignored. If a developer adds a new env var but forgets to add a corresponding entry in `settings.toml`, the value will silently be `None` at runtime instead of producing an error. This inverts the current failure mode (where `os.getenv()` returning `None` at least fails visibly at the point of use). **Mitigation (mandatory before Phase 2):** Phase 1 must complete a full audit of all `os.getenv()` calls across the codebase — including `config.py`, `providers/registry.py`, and any other modules — and add corresponding entries to `settings.toml`. A CI lint check (e.g., a script that greps for `os.getenv()` keys and verifies each has a `settings.toml` entry) must be added as part of Phase 1, not deferred. Until this CI check is in place, `ignore_unknown_envvars=True` should not be enabled.
+- **`ValidationError` replaces `ValueError`** — Dynaconf validators raise `dynaconf.validator.ValidationError` instead of `ValueError`. Any external code catching `ValueError` from `Settings.__post_init__` (e.g., for `document_chunk_overlap < 0`) will need to be updated. This is a breaking change introduced in Phase 3 when validators replace manual checks.
+- **`environments=True` is a legacy dynaconf feature** — The dynaconf docs recommend against it for new projects. If a future dynaconf major release removes it, we would need to migrate to per-file configuration (`settings.single_user_basic.toml`, etc.) or pinned TOML section names. This risk is accepted because the alternative requires managing 5+ separate files with duplicated defaults.
 
 ### Neutral
 - **`config_validators.py` unchanged** — Mode detection and conditional validation remain as Python business logic. Dynaconf validators handle structural checks only.