Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 `<dotted.location>: <message> (got <input>)` 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).
Expand Down
27 changes: 26 additions & 1 deletion tests/test_integration.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]],
Expand Down Expand Up @@ -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:
Expand All @@ -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)
Expand Down
48 changes: 48 additions & 0 deletions tests/vdsm/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -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 <path>` 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]],
Expand Down
61 changes: 24 additions & 37 deletions tests/vdsm/setup_dsm.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,6 @@
from __future__ import annotations

import logging
import os
import subprocess
import time
from pathlib import Path
from typing import Any
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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(
Expand Down Expand Up @@ -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 <path>` 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}")
Expand Down
64 changes: 64 additions & 0 deletions tests/vdsm/ssh.py
Original file line number Diff line number Diff line change
@@ -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()