diff --git a/CHANGELOG.md b/CHANGELOG.md index 015754a..69dad33 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,7 @@ ### Fixed +- **vdsm: refresh DSM search index for both setup-time and runtime-created test data** (#67) — fixes the `tests/test_integration.py::TestSearch::test_search_keyword_finds_directory` flake on vdsm CI. DSM Universal Search doesn't crawl non-indexed shares promptly on a freshly-booted vdsm, so search calls returned `0 results found` for several minutes after the data was created — well past the test's 65-second retry budget. Two-part fix: (1) `tests/vdsm/setup_dsm.py` calls `synoindex -A -d` for `/testshare/Documents` and `/testshare/Media` at golden-image build time, registering the static test data with DSM's search index. (2) New `refresh_search_index` async fixture in `tests/test_integration.py` (no-op default for real-NAS runs) and a vdsm override in `tests/vdsm/conftest.py` that invokes `synoindex -A -d` via SSH for the runtime-created `Bambu Studio` subdirectory. Without (2), the round-1 partial fix shipped via this PR's earlier head improved the test from 6-attempt fail to 4-attempt success — still flaky. With (2), search registers the new path synchronously and the test passes on attempt 1. SSH plumbing was lifted from `setup_dsm.py` into a new shared `tests/vdsm/ssh.py` module so both image-build-time and test-fixture-time SSH go through the same helper. Best-effort: a non-zero `synoindex` return logs a warning and falls back to the existing retry loop. Modifying `setup_dsm.py` invalidates the vdsm-workflow golden-image cache key, so the next CI run rebuilds the image with the fix baked in. - **Auth ignores empty / whitespace-only credentials at every read site** (#66) — closes #35. `_resolve_credentials()` in `core/auth.py` previously trusted that env / config / keyring values, if present, contained real credentials. Empty strings already fell through (Python truthiness handled them), but **whitespace-only** values like `auth: {username: " "}` mid-edit slipped through and reached `login()` as `(' ', '\t', None)`, surfacing as a generic DSM 400 that pointed neither at the config nor at the empty values. New `_present_or_none()` helper returns the value unchanged when it has any non-whitespace content, otherwise `None`. Applied at all nine read sites (3 env vars × 3 storage tiers — env / plaintext config / keyring). Meaningful padding is preserved (e.g. a real password `" pwd "` keeps its spaces); only purely empty/whitespace-only inputs are filtered. Six regression tests cover whitespace at each strategy level (env, config, keyring), the empty-string regression, the whitespace-doesn't-shadow-valid-keyring case, and the preserve-internal-padding guarantee. - **CLI catches `pydantic.ValidationError` and emits a clean `Error:` line** (#65) — closes #34. The four `load_config()` call sites (`cli/check.py`, `cli/main.py` for `serve`, `cli/setup.py` discovery, `cli/setup.py:_setup_with_config` for `-c`) previously caught only `(FileNotFoundError, ValueError, yaml.YAMLError)`. Pydantic's `ValidationError` IS a `ValueError` subclass, so it WAS caught — but `str(e)` rendered Pydantic's raw multi-line traceback-style block, defeating the clean-error pattern PR #26 established for malformed YAML. New helper `format_validation_error()` in `core/config.py` renders `exc.errors()` as a one-line header (`Configuration validation failed (N error(s)):`) plus per-error `: (got )` lines. The catch order at each site is now `except ValidationError` BEFORE `except (FileNotFoundError, ValueError, yaml.YAMLError)` since pydantic's class subclasses ValueError. Four regression tests added (one per call site) reproducing the gap by triggering AppConfig's strict-top-level `extra="forbid"` with an unknown field, asserting `Error: Configuration validation failed` lands and no `Traceback` leaks. - **`search_files` MCP tool exposes `mtime_from` / `mtime_to`** (#64) — closes #33. The underlying `search_files()` handler in `modules/filestation/search.py` accepted both parameters and forwarded them to DSM, but the FastMCP tool registration in `modules/filestation/__init__.py:tool_search_files` omitted them entirely — the documented modification-date filter was inaccessible from MCP clients. The parameters now appear on the tool surface and the description names ISO-8601 / `YYYY-MM-DD` / Unix-epoch as accepted formats. While fixing the surface gap, also added a `parse_mtime()` helper in `modules/filestation/helpers.py` so the documented format actually round-trips: ISO-8601 datetimes (with or without offset), bare calendar dates, and numeric epoch strings now all convert to the integer epoch seconds DSM's `SYNO.FileStation.Search` API expects. Naive datetimes are treated as UTC for stable cross-host behavior. New tests: 8 `TestParseMtime` cases in `tests/modules/filestation/test_helpers.py` plus a `test_search_with_mtime_filter` round-trip test that asserts the converted epoch lands in the DSM `start` request via respx (508 tests pass total at 96.14% coverage). diff --git a/tests/test_integration.py b/tests/test_integration.py index 135311d..fcb2798 100644 --- a/tests/test_integration.py +++ b/tests/test_integration.py @@ -82,6 +82,23 @@ def integration_config() -> tuple[AppConfig, dict[str, str]]: return _load_integration_config() +@pytest.fixture +async def refresh_search_index() -> Any: + """Async callback to register a runtime-created path with the search index. + + Default no-op for real-NAS integration runs — Synology NAS shares are + typically indexed and the indexer picks up changes within seconds. + Overridden in tests/vdsm/conftest.py to invoke `synoindex -A -d` via + SSH on the vdsm container, where DSM Universal Search on non-indexed + shares can take several minutes to crawl runtime-created subdirectories. + """ + + async def _noop(_path: str) -> None: + return None + + return _noop + + @pytest.fixture async def nas_client( integration_config: tuple[AppConfig, dict[str, str]], @@ -325,7 +342,9 @@ class TestSearch: the delay or run fewer search tests at once. """ - async def test_search_keyword_finds_directory(self, nas_client: Any) -> None: + async def test_search_keyword_finds_directory( + self, nas_client: Any, refresh_search_index: Any + ) -> None: """A bare keyword should find matching directories via wildcard wrapping. Verifies three fixes at once: @@ -352,6 +371,12 @@ async def test_search_keyword_finds_directory(self, nas_client: Any) -> None: except ToolError as e: logger.info("Could not create search target (may already exist): %s", e) + # Register the (possibly just-created) directory with DSM's search + # index. No-op on real-NAS runs; on vdsm, calls synoindex via SSH + # so the runtime-created subdirectory is discoverable on the first + # search attempt instead of waiting for the periodic indexer. + await refresh_search_index(search_dir) + # Verify the target is visible to FileStation before searching listing = await list_files(client, path=folder) logger.info("Contents of %s before search:\n%s", folder, listing) diff --git a/tests/vdsm/conftest.py b/tests/vdsm/conftest.py index 186bd16..1d899e8 100644 --- a/tests/vdsm/conftest.py +++ b/tests/vdsm/conftest.py @@ -119,6 +119,54 @@ def integration_config( return vdsm_config +@pytest.fixture +async def refresh_search_index( + vdsm_container: VirtualDsmContainer, + dsm_version: str, +) -> Any: + """Override the no-op fixture from test_integration.py. + + Invokes `synoindex -A -d ` via SSH on the vdsm container so + runtime-created directories are immediately discoverable by DSM's + search service. Without this, DSM Universal Search on non-indexed + shares takes several minutes (sometimes longer than any reasonable + test retry budget) to crawl freshly-created subdirectories. + + Best-effort: a non-zero return from synoindex is logged but not + raised — the test's existing retry loop is the safety net for + indexer hiccups. + """ + import asyncio + + from tests.vdsm.ssh import ssh_exec + + meta = load_golden_meta(dsm_version) + admin_password = str(meta.get("admin_password", "")) + host = vdsm_container.host + port = vdsm_container.ssh_port + + async def _refresh(path: str) -> None: + # Translate the share-relative path the test uses (e.g. + # "/testshare/Documents/Bambu Studio") into the on-volume path + # synoindex needs (e.g. "/volume1/testshare/Documents/Bambu Studio"). + on_volume = f"/volume1{path}" + cmd = f"/usr/syno/bin/synoindex -A -d '{on_volume}'" + rc, out = await asyncio.to_thread( + ssh_exec, host, port, admin_password, cmd, sudo=True, timeout=20 + ) + if rc != 0: + logger.warning( + "synoindex -A -d %r returned rc=%d (output: %s) — relying on test retries", + on_volume, + rc, + out, + ) + else: + logger.info("synoindex registered %s with DSM search index", on_volume) + + return _refresh + + @pytest.fixture async def nas_client( vdsm_config: tuple[AppConfig, dict[str, str]], diff --git a/tests/vdsm/setup_dsm.py b/tests/vdsm/setup_dsm.py index 38f4d69..6301b15 100644 --- a/tests/vdsm/setup_dsm.py +++ b/tests/vdsm/setup_dsm.py @@ -18,8 +18,6 @@ from __future__ import annotations import logging -import os -import subprocess import time from pathlib import Path from typing import Any @@ -361,11 +359,7 @@ def _create_user_via_ui(page: Any, username: str, password: str) -> None: _SYNOSHARE = "/usr/syno/sbin/synoshare" -_SSH_OPTS = ( - "-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null " - "-o PreferredAuthentications=password -o PubkeyAuthentication=no " - "-o ConnectTimeout=10" -) +from tests.vdsm.ssh import ssh_exec as _ssh_exec # noqa: E402 def _enable_ssh(base_url: str, admin_user: str, admin_password: str) -> None: @@ -426,37 +420,10 @@ def _ssh( ) -> tuple[int, str]: """Run a command inside the DSM guest via SSH. - Password is passed via SSHPASS env var (for sshpass) and piped to - sudo -S via stdin when sudo=True. shlex.quote is used on the password - to prevent shell injection if the password contains special characters. + Thin wrapper for the shared helper in tests/vdsm/ssh.py — kept so + callers in this module don't have to change. """ - import shlex - - env = os.environ.copy() - env["SSHPASS"] = password - quoted_pw = shlex.quote(password) - remote_cmd = f"echo {quoted_pw} | sudo -S {cmd} 2>&1" if sudo else f"{cmd} 2>&1" - result = subprocess.run( - f'sshpass -e ssh {_SSH_OPTS} -p {port} mcpadmin@{host} "{remote_cmd}"', - shell=True, # noqa: S602 - capture_output=True, - text=True, - timeout=30, - env=env, - ) - # Strip sudo lecture and password prompt from output - lines = result.stdout.strip().split("\n") - filtered = [ - ln - for ln in lines - if not ln.startswith("Password:") - and "lecture" not in ln - and not ln.strip().startswith("#") - and "Respect the privacy" not in ln - and "Think before you type" not in ln - and "great power" not in ln - ] - return result.returncode, "\n".join(filtered).strip() + return _ssh_exec(host, port, password, cmd, sudo=sudo) def _create_shared_folders_via_ssh( @@ -523,6 +490,26 @@ def ssh(cmd: str, *, sudo: bool = True) -> tuple[int, str]: rc, out = ssh("ls -la /volume1/testshare/Documents/", sudo=False) print(f" Test data:\n{out}") + # Force DSM's file indexer to register the test data immediately. + # Without this, search tests against /testshare/Documents/Bambu Studio + # are flaky on freshly-booted vdsm — DSM Universal Search doesn't crawl + # non-indexed shares promptly, so search_files returns "0 results found" + # for up to several minutes after the data is created. Calling + # `synoindex -A -d ` adds the directory subtree to the search + # index immediately. Best-effort: if synoindex isn't available on this + # DSM build, log a warning and continue — the failure mode is just the + # pre-existing search-flake, not a setup blocker. + rc, out = ssh("/usr/syno/bin/synoindex -A -d /volume1/testshare/Documents") + if rc == 0: + print(" Search index: refreshed via synoindex -A -d /testshare/Documents") + else: + print(f" Warning: synoindex -A -d returned rc={rc}: {out}") + rc, out = ssh("/usr/syno/bin/synoindex -A -d /volume1/testshare/Media") + if rc == 0: + print(" Search index: refreshed via synoindex -A -d /testshare/Media") + else: + print(f" Warning: synoindex -A -d /testshare/Media returned rc={rc}: {out}") + # Set ACL permissions for test user (requires sudo) for name in ["testshare", "writable"]: rc, out = ssh(f"{_SYNOSHARE} --setuser {name} RW + {test_user}") diff --git a/tests/vdsm/ssh.py b/tests/vdsm/ssh.py new file mode 100644 index 0000000..cb69ba3 --- /dev/null +++ b/tests/vdsm/ssh.py @@ -0,0 +1,64 @@ +"""SSH-into-DSM-guest helper, shared by setup_dsm.py and conftest fixtures. + +Lifted from setup_dsm.py so the same SSH plumbing can be used at +golden-image-build time (one-shot, sudo for share/index admin) and at +test-fixture time (per-test, sudo for synoindex against runtime-created +directories). +""" + +from __future__ import annotations + +import os +import shlex +import subprocess + +SSH_OPTS = ( + "-o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null " + "-o PreferredAuthentications=password -o PubkeyAuthentication=no " + "-o ConnectTimeout=10" +) + + +def ssh_exec( + host: str, + port: int, + password: str, + cmd: str, + *, + sudo: bool = True, + timeout: int = 30, +) -> tuple[int, str]: + """Run a command inside the DSM guest via SSH. + + Password is passed via SSHPASS env var (for sshpass) and piped to + sudo -S via stdin when sudo=True. shlex.quote prevents shell injection + if the password contains special characters. + + Returns (returncode, filtered_output) — the sudo lecture / password + prompt boilerplate is stripped from stdout for cleaner test logs. + """ + env = os.environ.copy() + env["SSHPASS"] = password + quoted_pw = shlex.quote(password) + remote_cmd = f"echo {quoted_pw} | sudo -S {cmd} 2>&1" if sudo else f"{cmd} 2>&1" + result = subprocess.run( + f'sshpass -e ssh {SSH_OPTS} -p {port} mcpadmin@{host} "{remote_cmd}"', + shell=True, # noqa: S602 + capture_output=True, + text=True, + timeout=timeout, + env=env, + ) + # Strip sudo lecture and password prompt from output. + lines = result.stdout.strip().split("\n") + filtered = [ + ln + for ln in lines + if not ln.startswith("Password:") + and "lecture" not in ln + and not ln.strip().startswith("#") + and "Respect the privacy" not in ln + and "Think before you type" not in ln + and "great power" not in ln + ] + return result.returncode, "\n".join(filtered).strip()