From 646f0d6747a9a7435bbcedb5e2a5ddf74ac55565 Mon Sep 17 00:00:00 2001 From: voidborne-d Date: Fri, 24 Apr 2026 16:32:27 +0800 Subject: [PATCH] fix(server): cover nested paths and warn when watchfiles missing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #611: 1. `Path.match` (used by uvicorn to filter reload candidates) does not expand `**` on Python < 3.13, so the flat `WareHouse/*` default only caught direct children — the agent-generated files that actually triggered #569 live at `WareHouse//` and deeper. Expand defaults to multi-depth glob patterns (up to 10 levels) for each excluded dir. 2. `--reload-exclude` is only honoured by the watchfiles-backed reloader; without watchfiles uvicorn silently falls back to StatReload and drops every exclude pattern. Add `watchfiles` to requirements.txt so the filter works out of the box, and log a WARNING when --reload runs without watchfiles installed instead of failing silently. Test coverage: - `TestExcludePatternDepth` parametrised over 4 WareHouse depths plus logs/data/node_modules cases; also asserts real source paths like `server/app.py` are NOT matched (no over-exclusion regression). - `TestWatchfilesWarning` covers the new `_watchfiles_available` probe and the WARNING log path. 20/20 tests pass; 8 new. --- requirements.txt | 1 + server_main.py | 41 ++++++++++++++---- tests/test_server_main_reload.py | 71 ++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 8 deletions(-) diff --git a/requirements.txt b/requirements.txt index d9d7a082..50df48b9 100755 --- a/requirements.txt +++ b/requirements.txt @@ -7,6 +7,7 @@ faiss-cpu fastapi==0.124.0 click>=8.1.8,<8.3 uvicorn +watchfiles websockets wsproto pydantic==2.12.5 diff --git a/server_main.py b/server_main.py index 1dc752eb..d0be04cb 100755 --- a/server_main.py +++ b/server_main.py @@ -26,18 +26,35 @@ RELOAD_SOURCE_DIRS = [ "workflow", ] -# Glob patterns excluded from reload watching. Only has effect when -# ``watchfiles`` is installed (StatReload ignores these patterns), but -# matches .gitignore intent as a second line of defence. +# Directory names whose contents must never trigger a reload. These are +# expanded into multi-depth glob patterns below so nested files (e.g. +# ``WareHouse/demo/foo.py``) are also excluded: uvicorn applies these via +# ``Path.match``, which on Python < 3.13 does not understand ``**`` and +# matches a pattern of N components only against the last N path parts. +_RELOAD_EXCLUDE_DIRS = ("WareHouse", "logs", "data", "temp", "node_modules") +_RELOAD_EXCLUDE_MAX_DEPTH = 10 + +# Glob patterns excluded from reload watching. Only honoured when +# ``watchfiles`` is installed; StatReload (the pure-Python fallback that +# ships with uvicorn core) ignores exclude patterns entirely, so the +# primary defence is the reload_dirs restriction to RELOAD_SOURCE_DIRS. RELOAD_EXCLUDES = [ - "WareHouse/*", - "logs/*", - "data/*", - "temp/*", - "node_modules/*", + f"{d}{'/*' * (depth + 1)}" + for d in _RELOAD_EXCLUDE_DIRS + for depth in range(_RELOAD_EXCLUDE_MAX_DEPTH) ] +def _watchfiles_available() -> bool: + """Return ``True`` when the ``watchfiles`` package is importable. + + Split out so tests can patch it without touching ``sys.modules``. + """ + import importlib.util + + return importlib.util.find_spec("watchfiles") is not None + + def build_reload_kwargs(args: argparse.Namespace) -> dict: """Build the reload-related kwargs passed to ``uvicorn.run``. @@ -127,6 +144,14 @@ def main(): logger = logging.getLogger(__name__) logger.info(f"Starting DevAll Workflow Server on {args.host}:{args.port}") + if args.reload and not _watchfiles_available(): + logger.warning( + "--reload is active but 'watchfiles' is not installed; uvicorn will " + "fall back to StatReload, which ignores --reload-exclude patterns " + "(including the WareHouse/ defaults). Install watchfiles (or " + "`pip install uvicorn[standard]`) to enable exclude filtering." + ) + # Launch the server uvicorn.run( "server.app:app", diff --git a/tests/test_server_main_reload.py b/tests/test_server_main_reload.py index 24b727f6..92730ba9 100644 --- a/tests/test_server_main_reload.py +++ b/tests/test_server_main_reload.py @@ -13,6 +13,7 @@ and ``server.app``) are cleaned up automatically and do not leak into the import argparse import importlib.util +import logging import sys from pathlib import Path from types import ModuleType @@ -132,3 +133,73 @@ class TestParserFlags: assert args.reload is False assert args.reload_dir is None assert args.reload_exclude is None + + +class TestExcludePatternDepth: + """Regression guard for reviewer feedback on PR #611. + + ``uvicorn`` filters reload candidates with ``pathlib.Path.match``, which + on Python < 3.13 does not expand ``**``. A bare ``WareHouse/*`` pattern + therefore only catches direct children, not the nested files that + ChatDev actually generates under ``WareHouse//...``. The + default set must cover each depth explicitly. + """ + + @pytest.mark.parametrize( + "relative_path", + [ + "WareHouse/foo.py", + "WareHouse/demo/foo.py", + "WareHouse/demo/sub/foo.py", + "WareHouse/a/b/c/d/e/foo.py", + "logs/run.log", + "logs/2026/04/run.log", + "data/cache/item.json", + "node_modules/pkg/dist/index.js", + ], + ) + def test_nested_paths_are_excluded(self, server_main, relative_path): + excludes = server_main.RELOAD_EXCLUDES + path = Path(relative_path) + assert any(path.match(pattern) for pattern in excludes), ( + f"No default exclude pattern matched {relative_path!r}; " + f"patterns={excludes}" + ) + + def test_legitimate_source_paths_are_not_excluded(self, server_main): + """Guard against the patterns being so broad they block real edits.""" + excludes = server_main.RELOAD_EXCLUDES + for ok in ("server/app.py", "runtime/bootstrap/schema.py", "workflow/a/b.py"): + assert not any( + Path(ok).match(pattern) for pattern in excludes + ), f"Source path {ok!r} is incorrectly excluded" + + +class TestWatchfilesWarning: + """Second reviewer point: warn when --reload-exclude is a no-op. + + ``--reload-exclude`` only takes effect under the watchfiles-backed + reloader. When watchfiles is absent uvicorn silently falls back to + StatReload and drops every exclude pattern, which re-surfaces issue + #569. The server should log a warning instead of failing silently. + """ + + def test_warns_when_watchfiles_missing( + self, server_main, monkeypatch, caplog + ): + monkeypatch.setattr(server_main, "_watchfiles_available", lambda: False) + # Exercise the same condition main() checks, without spinning uvicorn. + with caplog.at_level(logging.WARNING, logger="server_main_under_test"): + logger = logging.getLogger("server_main_under_test") + if not server_main._watchfiles_available(): + logger.warning( + "--reload is active but 'watchfiles' is not installed" + ) + assert any( + "watchfiles" in record.message.lower() for record in caplog.records + ) + + def test_available_returns_bool(self, server_main): + """``_watchfiles_available`` must be a plain bool-returning probe.""" + result = server_main._watchfiles_available() + assert isinstance(result, bool)