mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-07-21 02:05:45 +00:00
fix(config): close out #4124 review follow-ups (shared signature helper, resolve_config_path None contract) (#4275)
* fix(config): close out #4124 review follow-ups Extracts the (mtime, size, sha256) content-signature helper that was duplicated between config/app_config.py and mcp/cache.py into a new config/file_signature.py, and fixes ExtensionsConfig.resolve_config_path() to return None instead of raising FileNotFoundError when an explicit config_path argument or DEER_FLOW_EXTENSIONS_CONFIG_PATH points at a file that has since been deleted -- the exact resolution mode Docker dev/prod uses per AGENTS.md, so the MCP tools-cache staleness check could raise instead of degrading to "not stale". Both were flagged by willem-bd in review on #4124 and explicitly deferred there ("flagging for visibility", "leaving the extraction as the follow-up you suggested"). * fix(config): keep explicit extensions-config paths fail-loud resolve_config_path() previously turned every missing-file case into a clean None, including an explicit config_path argument or DEER_FLOW_EXTENSIONS_CONFIG_PATH (the exact mode Docker dev/prod uses). That silently downgrades a bad Docker mount, typo, or deleted production config to "no extensions" instead of surfacing the misconfiguration, per fancyboi999's review [P1] and willem-bd's follow-up notes on this PR. Restores FileNotFoundError for the two explicit modes (config_path argument, DEER_FLOW_EXTENSIONS_CONFIG_PATH); only the fallback search mode (no explicit path/env var, nothing found in the usual locations) still returns None, since that is the legitimate "extensions were never configured" case. The one caller that needs the old fail-soft behavior -- the MCP tools-cache staleness check, which re-resolves the path on every get_cached_mcp_tools() call -- gets a narrow, local catch instead (deerflow.mcp.cache._resolve_config_path) so a config file going missing mid-run still degrades the cache to "not stale" rather than crashing a hot per-request path. Also hoists the double os.getenv() read in the env-var branch into a local, per willem-bd's nit. Adds resolver-level tests for both explicit-path and env-var raises, a dedicated search-mode None regression test, and updates the existing MCP-cache docstrings to describe the corrected split.
This commit is contained in:
Daoyuan Li
+3
-1
@@ -311,6 +311,8 @@ Configuration priority:
|
||||
3. `extensions_config.json` in current directory (backend/)
|
||||
4. `extensions_config.json` in parent directory (project root - **recommended location**)
|
||||
|
||||
Extensions are optional only in the fallback *search* mode (priority 3-4 above): `ExtensionsConfig.resolve_config_path()` returns `None` when neither an explicit `config_path` nor `DEER_FLOW_EXTENSIONS_CONFIG_PATH` is given and the search locations find nothing. An explicit `config_path` argument or a set `DEER_FLOW_EXTENSIONS_CONFIG_PATH` (priority 1-2) is an operator assertion that one particular file must be used, so a missing file in either of those modes raises `FileNotFoundError` instead — including when the file existed earlier and has since been deleted. The MCP tools cache's staleness check (`deerflow.mcp.cache._resolve_config_path`) is a narrow, deliberate exception to that rule: it catches that `FileNotFoundError` locally and treats it as "unconfigured" so a previously-valid config disappearing mid-run degrades the cache to serving its last-known-good tools instead of raising out of a per-request hot path (see the MCP System section below).
|
||||
|
||||
### Gateway API (`app/gateway/`)
|
||||
|
||||
FastAPI application on port 8001 with health check at `GET /health`. Set `GATEWAY_ENABLE_DOCS=false` to disable `/docs`, `/redoc`, and `/openapi.json` in production (default: enabled).
|
||||
@@ -442,7 +444,7 @@ Additional providers also live here (`boxlite`, `brave`, `browserless`, `crawl4a
|
||||
|
||||
- Uses `langchain-mcp-adapters` `MultiServerMCPClient` for multi-server management
|
||||
- **Lazy initialization**: Tools loaded on first use via `get_cached_mcp_tools()`
|
||||
- **Cache invalidation**: Detects extensions-config changes by comparing the resolved config path and a `(mtime, size, sha256)` content signature against the values recorded at initialization (mirrors `config/app_config.py::get_app_config()`), not a strict mtime `>` comparison. This catches same-second edits, mtime that stays put or moves backward (`git checkout`, `cp -p` / backup restore, `tar` / `rsync`, object-store / network mounts), and a switch to a different config file with an equal-or-older mtime
|
||||
- **Cache invalidation**: Detects extensions-config changes by comparing the resolved config path and a `(mtime, size, sha256)` content signature against the values recorded at initialization, not a strict mtime `>` comparison. This catches same-second edits, mtime that stays put or moves backward (`git checkout`, `cp -p` / backup restore, `tar` / `rsync`, object-store / network mounts), and a switch to a different config file with an equal-or-older mtime. The signature helper (`config/file_signature.py::get_config_signature`) is shared with `config/app_config.py::get_app_config()` for the sibling runtime-editable config file, rather than each maintaining its own copy. `ExtensionsConfig.resolve_config_path()` raises `FileNotFoundError` for an explicit `config_path`/`DEER_FLOW_EXTENSIONS_CONFIG_PATH` that points at a missing file — an operator-asserted path going missing is a real misconfiguration, so this is intentionally loud for callers that load the config for actual use (e.g. `from_file()` via `get_mcp_tools()`); only the fallback search mode returns `None`. The MCP cache's own path resolution (`mcp/cache.py::_resolve_config_path`) is narrower: it catches that specific `FileNotFoundError` locally and treats it the same as "unconfigured", so this staleness check degrades to "not stale" instead of propagating an exception when a previously-valid explicit/env-var config disappears mid-run
|
||||
- **Transports**: stdio (command-based), SSE, HTTP
|
||||
- **OAuth (HTTP/SSE)**: Supports token endpoint flows (`client_credentials`, `refresh_token`) with automatic token refresh + Authorization header injection
|
||||
- **Routing hints**: `extensions_config.json -> mcpServers.<server>.routing` and
|
||||
|
||||
@@ -1,4 +1,3 @@
|
||||
import hashlib
|
||||
import logging
|
||||
import os
|
||||
from collections.abc import Mapping
|
||||
@@ -18,6 +17,8 @@ from deerflow.config.channel_connections_config import ChannelConnectionsConfig
|
||||
from deerflow.config.checkpointer_config import CheckpointerConfig, load_checkpointer_config_from_dict
|
||||
from deerflow.config.database_config import DatabaseConfig
|
||||
from deerflow.config.extensions_config import ExtensionsConfig
|
||||
from deerflow.config.file_signature import ConfigSignature as _ConfigSignature
|
||||
from deerflow.config.file_signature import get_config_signature as _get_config_signature
|
||||
from deerflow.config.guardrails_config import GuardrailsConfig, load_guardrails_config_from_dict
|
||||
from deerflow.config.input_polish_config import InputPolishConfig
|
||||
from deerflow.config.loop_detection_config import LoopDetectionConfig
|
||||
@@ -516,7 +517,6 @@ class AppConfig(BaseModel):
|
||||
_app_config: AppConfig | None = None
|
||||
_app_config_path: Path | None = None
|
||||
_app_config_mtime: float | None = None
|
||||
_ConfigSignature = tuple[float | None, int | None, str | None]
|
||||
_app_config_signature: _ConfigSignature | None = None
|
||||
_app_config_is_custom = False
|
||||
_current_app_config: ContextVar[AppConfig | None] = ContextVar("deerflow_current_app_config", default=None)
|
||||
@@ -531,24 +531,6 @@ def _get_config_mtime(config_path: Path) -> float | None:
|
||||
return None
|
||||
|
||||
|
||||
def _get_config_signature(config_path: Path) -> _ConfigSignature | None:
|
||||
"""Get cache metadata for a config file, including a content digest."""
|
||||
try:
|
||||
stat_result = config_path.stat()
|
||||
except OSError:
|
||||
return None
|
||||
|
||||
digest = hashlib.sha256()
|
||||
try:
|
||||
with config_path.open("rb") as f:
|
||||
for chunk in iter(lambda: f.read(1024 * 1024), b""):
|
||||
digest.update(chunk)
|
||||
except OSError:
|
||||
return (stat_result.st_mtime, stat_result.st_size, None)
|
||||
|
||||
return (stat_result.st_mtime, stat_result.st_size, digest.hexdigest())
|
||||
|
||||
|
||||
def _load_and_cache_app_config(config_path: str | None = None) -> AppConfig:
|
||||
"""Load config from disk and refresh cache metadata."""
|
||||
global _app_config, _app_config_path, _app_config_mtime, _app_config_signature, _app_config_is_custom
|
||||
|
||||
@@ -152,7 +152,7 @@ class ExtensionsConfig(BaseModel):
|
||||
2. If provided `DEER_FLOW_EXTENSIONS_CONFIG_PATH` environment variable, use it.
|
||||
3. Otherwise, search the caller project root for `extensions_config.json`, then `mcp_config.json`.
|
||||
4. For backward compatibility, also search legacy backend/repository-root defaults.
|
||||
5. If not found, return None (extensions are optional).
|
||||
5. If not found via search, return None (extensions are optional).
|
||||
|
||||
Args:
|
||||
config_path: Optional path to extensions config file.
|
||||
@@ -165,15 +165,37 @@ class ExtensionsConfig(BaseModel):
|
||||
4. Finally, search backend/repository-root defaults for monorepo compatibility.
|
||||
|
||||
Returns:
|
||||
Path to the extensions config file if found, otherwise None.
|
||||
Path to the extensions config file if found via the resolution
|
||||
order above.
|
||||
|
||||
An explicit `config_path` argument or a set
|
||||
`DEER_FLOW_EXTENSIONS_CONFIG_PATH` is an operator assertion that
|
||||
one particular file must be used, so a missing file in either of
|
||||
those two modes raises ``FileNotFoundError`` (see Raises below)
|
||||
instead of degrading to "no config" — a bad Docker mount, typo,
|
||||
or deleted production config should surface as a loud, actionable
|
||||
error rather than silently starting with every MCP server and
|
||||
skill absent.
|
||||
|
||||
Only the fallback *search* mode (no explicit argument and no env
|
||||
var set) returns ``None`` when nothing is found: that case means
|
||||
extensions were never configured in the first place, which is the
|
||||
legitimate "extensions are optional" case some callers (e.g. the
|
||||
MCP tools-cache staleness check in `deerflow.mcp.cache`) rely on
|
||||
as a clean, expected signal.
|
||||
|
||||
Raises:
|
||||
FileNotFoundError: If `config_path` is given, or
|
||||
`DEER_FLOW_EXTENSIONS_CONFIG_PATH` is set, and the resolved
|
||||
path does not exist.
|
||||
"""
|
||||
if config_path:
|
||||
path = Path(config_path)
|
||||
if not path.exists():
|
||||
raise FileNotFoundError(f"Extensions config file specified by param `config_path` not found at {path}")
|
||||
return path
|
||||
elif os.getenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH"):
|
||||
path = Path(os.getenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH"))
|
||||
elif env_path := os.getenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH"):
|
||||
path = Path(env_path)
|
||||
if not path.exists():
|
||||
raise FileNotFoundError(f"Extensions config file specified by environment variable `DEER_FLOW_EXTENSIONS_CONFIG_PATH` not found at {path}")
|
||||
return path
|
||||
@@ -193,7 +215,9 @@ class ExtensionsConfig(BaseModel):
|
||||
if path.exists():
|
||||
return path
|
||||
|
||||
# Extensions are optional, so return None if not found
|
||||
# Extensions are optional: unlike the explicit config_path/env-var
|
||||
# branches above, finding nothing here is the expected case, so
|
||||
# return None rather than raising.
|
||||
return None
|
||||
|
||||
@classmethod
|
||||
|
||||
@@ -0,0 +1,55 @@
|
||||
"""Shared content-signature helper for runtime-editable config files.
|
||||
|
||||
Both ``config/app_config.py`` (``config.yaml``) and ``mcp/cache.py``
|
||||
(``extensions_config.json``) need to detect when a runtime-editable config
|
||||
file has actually changed, even under conditions a bare mtime comparison
|
||||
misses: same-second edits, mtime that stays put or moves backward
|
||||
(``git checkout``, ``cp -p`` / backup restore, ``tar`` / ``rsync`` that
|
||||
preserve timestamps, object-store / network mounts), or a switch to a
|
||||
different file whose mtime is <= the previously recorded one.
|
||||
|
||||
This module is the single implementation of that ``(mtime, size, sha256)``
|
||||
signature so the two call sites share one behavior instead of maintaining
|
||||
verbatim-duplicate copies that can silently drift apart over time.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import hashlib
|
||||
from pathlib import Path
|
||||
|
||||
# (mtime, size, sha256-hexdigest) recorded for a config file, or the current
|
||||
# values recomputed for comparison against a previously recorded one. A
|
||||
# ``None`` digest (third element) means the stat succeeded but the content
|
||||
# could not be read; the whole tuple is ``None`` when the file could not be
|
||||
# stat-ed at all (e.g. it does not exist).
|
||||
ConfigSignature = tuple[float | None, int | None, str | None]
|
||||
|
||||
|
||||
def get_config_signature(config_path: Path) -> ConfigSignature | None:
|
||||
"""Get cache metadata for *config_path*, including a content digest.
|
||||
|
||||
Returns ``None`` when the file cannot be stat-ed (e.g. it does not
|
||||
exist), so callers can treat "no file" as a distinct case from "file
|
||||
with unreadable content" (which still yields a partial signature below).
|
||||
"""
|
||||
try:
|
||||
stat_result = config_path.stat()
|
||||
except OSError:
|
||||
return None
|
||||
|
||||
# Always hash the full file here rather than short-circuiting when
|
||||
# mtime/size already match a previously recorded signature: swapping in
|
||||
# different content of identical byte length within the same second
|
||||
# leaves mtime *and* size unchanged, so only the sha256 catches that
|
||||
# swap. Skipping the hash on an mtime/size match would reopen the narrow
|
||||
# gap this signature was built to close.
|
||||
digest = hashlib.sha256()
|
||||
try:
|
||||
with config_path.open("rb") as f:
|
||||
for chunk in iter(lambda: f.read(1024 * 1024), b""):
|
||||
digest.update(chunk)
|
||||
except OSError:
|
||||
return (stat_result.st_mtime, stat_result.st_size, None)
|
||||
|
||||
return (stat_result.st_mtime, stat_result.st_size, digest.hexdigest())
|
||||
@@ -1,12 +1,14 @@
|
||||
"""Cache for MCP tools to avoid repeated loading."""
|
||||
|
||||
import asyncio
|
||||
import hashlib
|
||||
import logging
|
||||
from pathlib import Path
|
||||
|
||||
from langchain_core.tools import BaseTool
|
||||
|
||||
from deerflow.config.file_signature import ConfigSignature as _ConfigSignature
|
||||
from deerflow.config.file_signature import get_config_signature as _get_config_signature
|
||||
|
||||
logger = logging.getLogger(__name__)
|
||||
|
||||
_mcp_tools_cache: list[BaseTool] | None = None
|
||||
@@ -14,54 +16,53 @@ _cache_initialized = False
|
||||
_initialization_lock = asyncio.Lock()
|
||||
|
||||
# Cache-invalidation key for the resolved extensions config file. We track the
|
||||
# resolved path *and* a ``(mtime, size, sha256)`` content signature — mirroring
|
||||
# resolved path *and* a ``(mtime, size, sha256)`` content signature — via the
|
||||
# shared ``deerflow.config.file_signature`` helper also used by
|
||||
# ``deerflow.config.app_config`` for the sibling runtime-editable config file —
|
||||
# rather than only the mtime. A strict mtime ``>`` comparison misses same-second
|
||||
# edits and mtime that stays put or moves backward (object-store / network
|
||||
# mounts, ``git checkout``, ``cp -p`` / backup restore, ``tar`` / ``rsync`` that
|
||||
# preserve timestamps), and tracking no path at all makes a switch to a
|
||||
# different config file with an equal-or-older mtime structurally invisible.
|
||||
_ConfigSignature = tuple[float | None, int | None, str | None]
|
||||
_config_path: Path | None = None # Resolved extensions config path at init time
|
||||
_config_signature: _ConfigSignature | None = None # (mtime, size, sha256) at init time
|
||||
|
||||
|
||||
def _resolve_config_path() -> Path | None:
|
||||
"""Resolve the extensions config file path, or ``None`` when unconfigured."""
|
||||
"""Resolve the extensions config file path, or ``None`` when unconfigured.
|
||||
|
||||
``ExtensionsConfig.resolve_config_path()`` raises ``FileNotFoundError``
|
||||
when an explicit `config_path` or `DEER_FLOW_EXTENSIONS_CONFIG_PATH`
|
||||
points at a file that does not exist. That is deliberate for callers that
|
||||
load the config for actual use (e.g. ``ExtensionsConfig.from_file()`` via
|
||||
``get_mcp_tools()``): an operator-asserted explicit path going missing is
|
||||
a real misconfiguration and must be surfaced loudly.
|
||||
|
||||
This helper is not one of those callers — it only backs the cache's own
|
||||
staleness check (``_is_cache_stale``, via ``_current_config_state``),
|
||||
which runs on every ``get_cached_mcp_tools()`` call and just wants to know
|
||||
whether the previously loaded config is still current. If the file behind
|
||||
a previously-valid explicit/env-var path becomes unreadable later
|
||||
(deleted mid-run, a Docker mount hiccup, ...), raising here would crash
|
||||
every subsequent call to that hot per-request path instead of leaving the
|
||||
cache serving its last-known-good MCP tools. So this wrapper catches that
|
||||
specific failure and treats it the same as "unconfigured", matching
|
||||
``_is_cache_stale()``'s existing fail-soft handling of a ``None`` config
|
||||
state (see its docstring). Scoping the catch here — rather than making
|
||||
``resolve_config_path()`` itself return ``None`` for every caller — keeps
|
||||
the loud failure intact for callers that actually need the file.
|
||||
"""
|
||||
from deerflow.config.extensions_config import ExtensionsConfig
|
||||
|
||||
return ExtensionsConfig.resolve_config_path()
|
||||
|
||||
|
||||
def _get_config_signature(config_path: Path) -> _ConfigSignature | None:
|
||||
"""Get cache metadata for the extensions config file, including a content digest.
|
||||
|
||||
Mirrors ``deerflow.config.app_config._get_config_signature`` so both
|
||||
runtime-editable config files (``config.yaml`` and ``extensions_config.json``)
|
||||
share the same content-based staleness signal. Returns ``None`` when the
|
||||
file cannot be stat-ed (e.g. it does not exist).
|
||||
"""
|
||||
try:
|
||||
stat_result = config_path.stat()
|
||||
except OSError:
|
||||
return ExtensionsConfig.resolve_config_path()
|
||||
except FileNotFoundError:
|
||||
logger.debug(
|
||||
"Extensions config path could not be resolved while checking MCP cache staleness; treating as unconfigured for this check.",
|
||||
exc_info=True,
|
||||
)
|
||||
return None
|
||||
|
||||
# Always hash the full file here rather than short-circuiting when
|
||||
# mtime/size already match a previously recorded signature: swapping in a
|
||||
# different MCP server config of identical byte length within the same
|
||||
# second leaves mtime *and* size unchanged, so only the sha256 catches
|
||||
# that swap. Skipping the hash on an mtime/size match would reopen the
|
||||
# narrow gap this signature was built to close.
|
||||
digest = hashlib.sha256()
|
||||
try:
|
||||
with config_path.open("rb") as f:
|
||||
for chunk in iter(lambda: f.read(1024 * 1024), b""):
|
||||
digest.update(chunk)
|
||||
except OSError:
|
||||
return (stat_result.st_mtime, stat_result.st_size, None)
|
||||
|
||||
return (stat_result.st_mtime, stat_result.st_size, digest.hexdigest())
|
||||
|
||||
|
||||
def _current_config_state() -> tuple[Path | None, _ConfigSignature | None]:
|
||||
"""Return the currently resolved extensions config path and its signature."""
|
||||
|
||||
@@ -0,0 +1,77 @@
|
||||
"""Unit tests for the shared config-file content-signature helper.
|
||||
|
||||
``deerflow.config.file_signature.get_config_signature`` was extracted from
|
||||
verbatim-duplicate implementations that used to live independently in
|
||||
``deerflow.config.app_config`` and ``deerflow.mcp.cache`` (flagged in review
|
||||
on PR #4124: "now a verbatim duplicate of
|
||||
``deerflow/config/app_config.py::_get_config_signature`` / ``_ConfigSignature``
|
||||
... worth a follow-up to extract both into a small shared helper"). These
|
||||
tests cover the shared implementation directly, and pin that both former
|
||||
call sites now delegate to it instead of maintaining independent copies that
|
||||
can silently drift apart.
|
||||
"""
|
||||
|
||||
from __future__ import annotations
|
||||
|
||||
import os
|
||||
from pathlib import Path
|
||||
|
||||
from deerflow.config.file_signature import ConfigSignature, get_config_signature
|
||||
|
||||
|
||||
def test_missing_file_returns_none(tmp_path: Path):
|
||||
missing = tmp_path / "does-not-exist.json"
|
||||
assert get_config_signature(missing) is None
|
||||
|
||||
|
||||
def test_existing_file_returns_full_signature(tmp_path: Path):
|
||||
cfg = tmp_path / "config.json"
|
||||
cfg.write_text('{"a": 1}', encoding="utf-8")
|
||||
|
||||
signature = get_config_signature(cfg)
|
||||
|
||||
assert signature is not None
|
||||
mtime, size, digest = signature
|
||||
assert mtime == cfg.stat().st_mtime
|
||||
assert size == cfg.stat().st_size
|
||||
assert isinstance(digest, str) and len(digest) == 64 # sha256 hexdigest
|
||||
|
||||
|
||||
def test_content_change_changes_signature_even_with_same_mtime_and_size(tmp_path: Path):
|
||||
"""The digest -- not just mtime/size -- must catch a same-length content
|
||||
swap within the same second (the exact hole the sha256 exists to close)."""
|
||||
cfg = tmp_path / "config.json"
|
||||
cfg.write_text('{"server": "srv1"}', encoding="utf-8")
|
||||
before = get_config_signature(cfg)
|
||||
assert before is not None
|
||||
recorded_mtime, recorded_size = before[0], before[1]
|
||||
|
||||
cfg.write_text('{"server": "srv9"}', encoding="utf-8") # same length, different content
|
||||
os.utime(cfg, (recorded_mtime, recorded_mtime))
|
||||
assert cfg.stat().st_mtime == recorded_mtime # guard: mtime truly unchanged
|
||||
assert cfg.stat().st_size == recorded_size # guard: size truly unchanged too
|
||||
|
||||
after = get_config_signature(cfg)
|
||||
assert after is not None
|
||||
assert after[0] == before[0]
|
||||
assert after[1] == before[1]
|
||||
assert after[2] != before[2] # only the digest catches the swap
|
||||
|
||||
|
||||
def test_signature_type_alias_shape():
|
||||
"""ConfigSignature is the (mtime, size, sha256) tuple type both call sites share."""
|
||||
assert ConfigSignature == tuple[float | None, int | None, str | None]
|
||||
|
||||
|
||||
def test_app_config_and_mcp_cache_share_the_same_implementation():
|
||||
"""Regression guard for the PR #4124 review finding: both modules must
|
||||
delegate to this shared helper rather than maintaining independent
|
||||
verbatim copies that can silently drift apart over time.
|
||||
"""
|
||||
import deerflow.config.app_config as app_config_module
|
||||
import deerflow.mcp.cache as cache_module
|
||||
|
||||
assert app_config_module._get_config_signature is get_config_signature
|
||||
assert cache_module._get_config_signature is get_config_signature
|
||||
assert app_config_module._ConfigSignature is ConfigSignature
|
||||
assert cache_module._ConfigSignature is ConfigSignature
|
||||
@@ -225,10 +225,19 @@ def test_config_deleted_after_init_is_not_stale(cache_globals, monkeypatch, tmp_
|
||||
|
||||
The resolver is monkeypatched to keep pointing at the (now-missing) path,
|
||||
isolating ``_is_cache_stale``'s own stat-failure handling from
|
||||
``ExtensionsConfig.resolve_config_path``'s separate not-found contract for
|
||||
explicit path/env-var configuration (that function raises
|
||||
``FileNotFoundError`` in that mode instead of returning ``None`` — a
|
||||
distinct, pre-existing latent issue outside this module's scope).
|
||||
``ExtensionsConfig.resolve_config_path``'s own not-found contract for
|
||||
explicit path/env-var configuration, which raises ``FileNotFoundError``
|
||||
in that mode (an operator-asserted path going missing is a real
|
||||
misconfiguration and must be loud for callers that load the config for
|
||||
real use — PR #4275 review, fancyboi999 [P1]). ``_resolve_config_path``
|
||||
just above is the narrow exception: it catches that specific
|
||||
``FileNotFoundError`` and treats it as "unconfigured" so this staleness
|
||||
check keeps degrading to "not stale" instead of raising — see
|
||||
``test_extensions_config_env_var_missing_file_raises`` in
|
||||
``test_runtime_paths.py`` for the resolver-level raise contract, and
|
||||
``test_config_deleted_after_init_via_real_env_resolution_does_not_raise``
|
||||
below for the same scenario this test isolates against, exercised through
|
||||
the real resolver instead of a monkeypatch.
|
||||
"""
|
||||
cfg = tmp_path / "extensions_config.json"
|
||||
_write_extensions_config(cfg, {"srv1": _server()})
|
||||
@@ -243,3 +252,37 @@ def test_config_deleted_after_init_is_not_stale(cache_globals, monkeypatch, tmp_
|
||||
)
|
||||
|
||||
assert cache_module._is_cache_stale() is False
|
||||
|
||||
|
||||
def test_config_deleted_after_init_via_real_env_resolution_does_not_raise(cache_globals, monkeypatch, tmp_path):
|
||||
"""End-to-end regression for the explicit-vs-search distinction raised by
|
||||
fancyboi999 [P1] on PR #4275: when the extensions config path comes from
|
||||
``DEER_FLOW_EXTENSIONS_CONFIG_PATH`` (exactly how Docker dev/prod point at
|
||||
it, per backend/AGENTS.md) and the file is deleted after a successful
|
||||
init, ``_is_cache_stale()`` must not raise — even though
|
||||
``ExtensionsConfig.resolve_config_path()`` itself now (again) raises
|
||||
``FileNotFoundError`` for a missing explicit/env-var path, restoring loud
|
||||
failure for callers that load the config for real use.
|
||||
|
||||
Unlike ``test_config_deleted_after_init_is_not_stale`` (which monkeypatches
|
||||
``ExtensionsConfig.resolve_config_path`` to isolate ``_is_cache_stale``'s
|
||||
own None-handling from the resolver's own contract), this test exercises
|
||||
the REAL resolver end to end. ``_resolve_config_path`` in this module is
|
||||
the only thing standing between that raise and a crash here: it catches
|
||||
``FileNotFoundError`` locally and returns ``None``, so this hot,
|
||||
per-request staleness check keeps degrading to "not stale" (serving
|
||||
last-known-good cached tools) instead of propagating uncaught out of
|
||||
``get_cached_mcp_tools()``. Deleting the ``_resolve_config_path`` try/except
|
||||
reproduces the original crash this test guards against.
|
||||
"""
|
||||
cfg = tmp_path / "extensions_config.json"
|
||||
_write_extensions_config(cfg, {"srv1": _server()})
|
||||
_initialize_against(monkeypatch, cfg) # sets DEER_FLOW_EXTENSIONS_CONFIG_PATH=cfg
|
||||
assert cache_module._config_signature is not None # guard: had a real signature
|
||||
|
||||
cfg.unlink() # config deleted; env var still points at the now-missing path
|
||||
|
||||
# Must not raise, and must report "not stale" (fail-soft: keep serving the
|
||||
# last-known-good MCP tools), matching the deliberate contract in
|
||||
# test_config_deleted_after_init_is_not_stale above.
|
||||
assert cache_module._is_cache_stale() is False
|
||||
|
||||
@@ -179,3 +179,101 @@ def test_extensions_config_falls_back_to_legacy_when_project_root_lacks_file(tmp
|
||||
monkeypatch.setattr(extensions_config_module, "__file__", str(fake_paths_module_file))
|
||||
|
||||
assert ExtensionsConfig.resolve_config_path() == legacy_extensions
|
||||
|
||||
|
||||
def test_extensions_config_explicit_path_missing_file_raises(tmp_path: Path, monkeypatch):
|
||||
"""An explicit ``config_path`` argument pointing at a file that does not
|
||||
exist (e.g. it existed earlier and was deleted) must raise
|
||||
``FileNotFoundError`` identifying the explicit `config_path` argument as
|
||||
the culprit, not silently degrade to ``None``.
|
||||
|
||||
An explicit ``config_path`` is an operator assertion that one specific
|
||||
file must be used (PR #4275 review, fancyboi999 [P1]): a bad path, typo,
|
||||
or deleted file must surface as a loud, actionable error instead of
|
||||
silently constructing an empty extensions config (no MCP servers, no
|
||||
skills). Only the fallback *search* mode (no explicit argument, no env
|
||||
var) treats "nothing found" as the legitimate optional-extensions case
|
||||
and returns ``None`` — see
|
||||
``test_extensions_config_falls_back_to_legacy_when_project_root_lacks_file``
|
||||
and ``test_extensions_config_search_finds_nothing_returns_none`` below.
|
||||
"""
|
||||
_clear_path_env(monkeypatch)
|
||||
missing = tmp_path / "extensions_config.json"
|
||||
# Never created (equivalent to "existed, then got deleted" from the
|
||||
# resolver's point of view -- it only sees that the path doesn't exist).
|
||||
|
||||
with pytest.raises(FileNotFoundError, match="config_path"):
|
||||
ExtensionsConfig.resolve_config_path(config_path=str(missing))
|
||||
|
||||
|
||||
def test_extensions_config_env_var_missing_file_raises(tmp_path: Path, monkeypatch):
|
||||
"""``DEER_FLOW_EXTENSIONS_CONFIG_PATH`` pointing at a file that has since
|
||||
been deleted must raise ``FileNotFoundError`` identifying the environment
|
||||
variable as the culprit, not silently return ``None``.
|
||||
|
||||
This is the exact resolution mode Docker dev/prod uses (see
|
||||
backend/AGENTS.md: "Docker development ... points `DEER_FLOW_CONFIG_PATH`
|
||||
/ `DEER_FLOW_EXTENSIONS_CONFIG_PATH`" at the mounted config directory), so
|
||||
a bad mount or deleted file at this explicit, operator-configured path is
|
||||
a real misconfiguration that must surface loudly (PR #4275 review,
|
||||
fancyboi999 [P1]) instead of silently starting with every MCP server and
|
||||
skill absent.
|
||||
|
||||
``deerflow.mcp.cache._resolve_config_path`` calls
|
||||
``ExtensionsConfig.resolve_config_path()`` with no args and therefore
|
||||
hits this exact branch whenever the env var is set; that module has its
|
||||
own narrower catch around this specific exception so the MCP tools-cache
|
||||
staleness check does not crash on every request once the file goes
|
||||
missing after a successful init — see
|
||||
``test_config_deleted_after_init_via_real_env_resolution_does_not_raise``
|
||||
in ``test_mcp_cache.py`` for that regression.
|
||||
"""
|
||||
_clear_path_env(monkeypatch)
|
||||
cfg = tmp_path / "extensions_config.json"
|
||||
cfg.write_text('{"mcpServers": {}, "skills": {}}', encoding="utf-8")
|
||||
monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(cfg))
|
||||
|
||||
assert ExtensionsConfig.resolve_config_path() == cfg # sanity: resolves while present
|
||||
|
||||
cfg.unlink()
|
||||
|
||||
with pytest.raises(FileNotFoundError, match="DEER_FLOW_EXTENSIONS_CONFIG_PATH"):
|
||||
ExtensionsConfig.resolve_config_path()
|
||||
|
||||
|
||||
def test_extensions_config_search_finds_nothing_returns_none(tmp_path: Path, monkeypatch):
|
||||
"""The fallback *search* mode (no explicit ``config_path``, no
|
||||
``DEER_FLOW_EXTENSIONS_CONFIG_PATH``) must still return ``None`` — not
|
||||
raise — when none of the search locations (project root, legacy
|
||||
backend/repo-root) have an extensions config file.
|
||||
|
||||
This is the one resolution mode where "not found" is the expected,
|
||||
non-error case (extensions are entirely optional throughout the
|
||||
application), so it must keep its pre-existing ``None`` contract even
|
||||
though the explicit `config_path`/`DEER_FLOW_EXTENSIONS_CONFIG_PATH`
|
||||
branches now raise ``FileNotFoundError`` for the analogous "missing file"
|
||||
condition (see ``test_extensions_config_explicit_path_missing_file_raises``
|
||||
and ``test_extensions_config_env_var_missing_file_raises`` above).
|
||||
Regression guard for the original #4124 fix:
|
||||
``deerflow.mcp.cache._is_cache_stale`` depends on this fallback ``None``
|
||||
to treat "never configured" as "not stale".
|
||||
"""
|
||||
_clear_path_env(monkeypatch)
|
||||
cwd = tmp_path / "cwd"
|
||||
cwd.mkdir()
|
||||
monkeypatch.chdir(cwd)
|
||||
|
||||
fake_backend = tmp_path / "fake-backend-empty"
|
||||
fake_repo = tmp_path / "fake-repo-empty"
|
||||
fake_backend.mkdir()
|
||||
fake_repo.mkdir()
|
||||
# No extensions_config.json / mcp_config.json anywhere: not in cwd, not in
|
||||
# the legacy backend dir, not in the legacy repo root.
|
||||
|
||||
fake_paths_module_file = fake_backend / "packages" / "harness" / "deerflow" / "config" / "extensions_config.py"
|
||||
fake_paths_module_file.parent.mkdir(parents=True)
|
||||
fake_paths_module_file.write_text("", encoding="utf-8")
|
||||
|
||||
monkeypatch.setattr(extensions_config_module, "__file__", str(fake_paths_module_file))
|
||||
|
||||
assert ExtensionsConfig.resolve_config_path() is None
|
||||
|
||||
Reference in New Issue
Block a user