From 512de1f6b065f70fadaa2c943326919bb34cb212 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Tue, 14 Apr 2026 21:14:42 +0200 Subject: [PATCH] test: address PR #707 reviewer feedback on config path helpers - _resolve_settings_files() now raises FileNotFoundError when NEXTCLOUD_MCP_SETTINGS_FILE points to a missing file, instead of silently falling back to defaults (footgun on typos). - .secrets.toml is now looked for alongside the explicit settings file when NEXTCLOUD_MCP_SETTINGS_FILE is set, matching user expectation for /etc-style deployments. Unset behaviour (cwd lookup) is unchanged. - get_token_db_path() drops the redundant os.environ.get() short-circuit; TOKEN_STORAGE_DB is already bound through dynaconf because the key is declared in _DEFAULTS. - is_ephemeral_token_db() docstring documents the "must call get_token_db_path() first" precondition. - alembic.ini comment clarifies the ./tokens.db placeholder is cwd-relative by design and points readers at the -x database_url escape hatch. - New tests/unit/test_config_paths.py (12 tests) covering the ephemeral tempfile lifecycle, the TOKEN_STORAGE_DB override path, and all six _resolve_settings_files() cases including the two new behaviours. Full unit suite now at 476 passed (464 + 12 new). Ruff + ty clean. Co-Authored-By: Claude Opus 4.6 (1M context) --- alembic.ini | 6 +- nextcloud_mcp_server/config.py | 44 ++++++--- tests/unit/test_config_paths.py | 158 ++++++++++++++++++++++++++++++++ 3 files changed, 193 insertions(+), 15 deletions(-) create mode 100644 tests/unit/test_config_paths.py diff --git a/alembic.ini b/alembic.ini index 8f60afd9..ddf131c1 100644 --- a/alembic.ini +++ b/alembic.ini @@ -44,7 +44,11 @@ path_separator = os # Runtime code (migrations.get_alembic_config / alembic/env.get_database_url) # overrides this with config.get_token_db_path() which defaults to an # ephemeral per-process tempfile unless TOKEN_STORAGE_DB is set. -# This value only matters for manual `alembic` invocations from the repo root. +# This value only matters for manual `alembic` invocations (e.g. +# `uv run alembic current`). The `./tokens.db` path is intentionally +# resolved relative to the shell's current working directory at the time +# of invocation — run alembic from the repo root for predictable behaviour, +# or pass `-x database_url=sqlite+aiosqlite:///`. sqlalchemy.url = sqlite+aiosqlite:///./tokens.db [post_write_hooks] diff --git a/nextcloud_mcp_server/config.py b/nextcloud_mcp_server/config.py index 9e820b00..b84c989c 100644 --- a/nextcloud_mcp_server/config.py +++ b/nextcloud_mcp_server/config.py @@ -113,26 +113,34 @@ def _resolve_settings_files() -> list[str]: """Find optional external settings files. Priority: - 1. NEXTCLOUD_MCP_SETTINGS_FILE env var (absolute or relative path) - 2. ./settings.toml in cwd (for docker / dev workflows) - 3. .secrets.toml alongside whichever settings.toml was found + 1. NEXTCLOUD_MCP_SETTINGS_FILE env var (absolute or relative path). + If set but the file does not exist, raise FileNotFoundError — + silently falling back to defaults on a typo would be a footgun. + .secrets.toml is looked for alongside the explicit file. + 2. Otherwise ./settings.toml in cwd (for docker / dev workflows), + with .secrets.toml also looked for in cwd. - Returns an empty list if no files are present — that's fine, defaults and - env vars still apply. + Returns an empty list if nothing is configured — that's fine, defaults + and env vars still apply. """ files: list[str] = [] explicit = os.environ.get("NEXTCLOUD_MCP_SETTINGS_FILE") if explicit: p = Path(explicit) - if p.exists(): - files.append(str(p)) + if not p.exists(): + raise FileNotFoundError( + f"NEXTCLOUD_MCP_SETTINGS_FILE points to a file that does " + f"not exist: {explicit}" + ) + files.append(str(p)) + secrets = p.parent / ".secrets.toml" else: cwd_settings = Path.cwd() / "settings.toml" if cwd_settings.exists(): files.append(str(cwd_settings)) - cwd_secrets = Path.cwd() / ".secrets.toml" - if cwd_secrets.exists(): - files.append(str(cwd_secrets)) + secrets = Path.cwd() / ".secrets.toml" + if secrets.exists(): + files.append(str(secrets)) return files @@ -198,14 +206,15 @@ def get_token_db_path() -> str: """Resolve the token SQLite database path. Priority: - 1. TOKEN_STORAGE_DB env var / dynaconf setting if explicitly set — - docker-compose pins /app/data/tokens.db this way. + 1. TOKEN_STORAGE_DB if explicitly set — docker-compose pins + /app/data/tokens.db this way. Read via dynaconf, which picks up + the env var because TOKEN_STORAGE_DB is declared in _DEFAULTS. 2. Otherwise a per-process tempfile under tempfile.gettempdir(), allocated lazily and deleted at interpreter exit via atexit. Ephemeral: tokens are wiped on restart, matching the Qdrant ":memory:" default pattern used elsewhere in this project. """ - explicit = os.environ.get("TOKEN_STORAGE_DB") or _dynaconf.get("TOKEN_STORAGE_DB") + explicit = _dynaconf.get("TOKEN_STORAGE_DB") if explicit: return str(explicit) global _ephemeral_db_path @@ -228,7 +237,14 @@ def get_token_db_path() -> str: def is_ephemeral_token_db(path: str) -> bool: - """Return True if the given path is the process-local ephemeral tempfile.""" + """Return True if the given path is the process-local ephemeral tempfile. + + Precondition: `get_token_db_path()` must have been called at least once + in this process to allocate the tempfile. If called before allocation, + this returns False for any input (including the eventual tempfile path), + because there is nothing to compare against yet. In practice every call + site in this repo resolves the path via `get_token_db_path()` first. + """ return path == _ephemeral_db_path diff --git a/tests/unit/test_config_paths.py b/tests/unit/test_config_paths.py new file mode 100644 index 00000000..5e282aa0 --- /dev/null +++ b/tests/unit/test_config_paths.py @@ -0,0 +1,158 @@ +"""Tests for config.py path resolution helpers added in PR #707. + +Covers: +- get_token_db_path() / is_ephemeral_token_db() — ephemeral tempfile default + with TOKEN_STORAGE_DB override. +- _resolve_settings_files() — optional external settings file discovery, + including the NEXTCLOUD_MCP_SETTINGS_FILE env var and its colocation + semantics for .secrets.toml. +""" + +import os +import tempfile +from pathlib import Path + +import pytest + +import nextcloud_mcp_server.config as cfg +from nextcloud_mcp_server.config import ( + _reload_config, + _resolve_settings_files, + get_token_db_path, + is_ephemeral_token_db, +) + + +@pytest.fixture(autouse=True) +def _reset_ephemeral_state(monkeypatch): + """Reset module-global ephemeral tempfile state between tests. + + get_token_db_path() memoizes its result in a module-level global and + registers an atexit hook for cleanup. Tests need an isolated slate so + assertions about "already allocated" vs "not yet" are meaningful. + """ + old = cfg._ephemeral_db_path + cfg._ephemeral_db_path = None + monkeypatch.delenv("TOKEN_STORAGE_DB", raising=False) + monkeypatch.delenv("NEXTCLOUD_MCP_SETTINGS_FILE", raising=False) + _reload_config() + yield + if cfg._ephemeral_db_path and os.path.exists(cfg._ephemeral_db_path): + os.unlink(cfg._ephemeral_db_path) + cfg._ephemeral_db_path = old + _reload_config() + + +class TestGetTokenDbPath: + def test_explicit_env_var_returned(self, monkeypatch, tmp_path): + target = tmp_path / "explicit.db" + monkeypatch.setenv("TOKEN_STORAGE_DB", str(target)) + _reload_config() + + assert get_token_db_path() == str(target) + # No tempfile should have been allocated since we took the explicit + # branch. + assert cfg._ephemeral_db_path is None + + def test_ephemeral_tempfile_when_unset(self): + path = get_token_db_path() + + assert path.startswith(tempfile.gettempdir()) + assert f"nextcloud-mcp-tokens-{os.getpid()}-" in os.path.basename(path) + assert path.endswith(".db") + assert os.path.exists(path) + + def test_ephemeral_tempfile_is_memoized(self): + first = get_token_db_path() + second = get_token_db_path() + + assert first == second + # Only the memoized file should exist — no stray siblings. + parent = Path(first).parent + matches = list(parent.glob(f"nextcloud-mcp-tokens-{os.getpid()}-*.db")) + assert matches == [Path(first)] + + def test_is_ephemeral_token_db_detects_allocated_path(self): + path = get_token_db_path() + + assert is_ephemeral_token_db(path) is True + assert is_ephemeral_token_db("/some/other/path") is False + + def test_is_ephemeral_token_db_before_allocation(self): + # The autouse fixture reset _ephemeral_db_path to None; do not call + # get_token_db_path() first. Nothing is allocated yet. + assert cfg._ephemeral_db_path is None + assert is_ephemeral_token_db("/any/path") is False + assert is_ephemeral_token_db("") is False + + def test_explicit_path_does_not_trigger_tempfile(self, monkeypatch, tmp_path): + """Regression guard: the explicit branch must short-circuit cleanly.""" + monkeypatch.setenv("TOKEN_STORAGE_DB", str(tmp_path / "pinned.db")) + _reload_config() + + get_token_db_path() + # Nothing under the tempfile prefix should have been created. + matches = list( + Path(tempfile.gettempdir()).glob(f"nextcloud-mcp-tokens-{os.getpid()}-*.db") + ) + assert matches == [] + + +class TestResolveSettingsFiles: + def test_empty_list_when_nothing_present(self, monkeypatch, tmp_path): + monkeypatch.chdir(tmp_path) + assert _resolve_settings_files() == [] + + def test_picks_up_cwd_settings(self, monkeypatch, tmp_path): + (tmp_path / "settings.toml").write_text("[default]\n") + monkeypatch.chdir(tmp_path) + + result = _resolve_settings_files() + assert str(tmp_path / "settings.toml") in result + + def test_picks_up_cwd_secrets(self, monkeypatch, tmp_path): + (tmp_path / ".secrets.toml").write_text("[default]\n") + monkeypatch.chdir(tmp_path) + + result = _resolve_settings_files() + assert str(tmp_path / ".secrets.toml") in result + + def test_explicit_settings_file_included(self, monkeypatch, tmp_path): + explicit = tmp_path / "nested" / "my-settings.toml" + explicit.parent.mkdir() + explicit.write_text("[default]\n") + monkeypatch.setenv("NEXTCLOUD_MCP_SETTINGS_FILE", str(explicit)) + # cwd has no settings.toml / .secrets.toml + monkeypatch.chdir(tmp_path) + + result = _resolve_settings_files() + assert result == [str(explicit)] + + def test_explicit_settings_file_secrets_colocated(self, monkeypatch, tmp_path): + """PR #707 reviewer feedback: .secrets.toml should live beside + the explicit settings file, not always in cwd.""" + config_dir = tmp_path / "etc" + config_dir.mkdir() + explicit = config_dir / "settings.toml" + explicit.write_text("[default]\n") + secrets = config_dir / ".secrets.toml" + secrets.write_text("[default]\n") + + monkeypatch.setenv("NEXTCLOUD_MCP_SETTINGS_FILE", str(explicit)) + # cwd is elsewhere and deliberately contains *no* secrets file + monkeypatch.chdir(tmp_path) + + result = _resolve_settings_files() + assert str(explicit) in result + assert str(secrets) in result + + def test_explicit_missing_raises(self, monkeypatch, tmp_path): + """PR #707 reviewer feedback: missing explicit path must not be + silently ignored — users will think their config is applied when + it isn't.""" + missing = tmp_path / "does-not-exist.toml" + monkeypatch.setenv("NEXTCLOUD_MCP_SETTINGS_FILE", str(missing)) + monkeypatch.chdir(tmp_path) + + with pytest.raises(FileNotFoundError, match="does-not-exist.toml"): + _resolve_settings_files()