From fb722770e424795d83e4eaf572591f17c7c6fcac Mon Sep 17 00:00:00 2001 From: wutongyuonce <147830929+wutongyuonce@users.noreply.github.com> Date: Wed, 2 Sep 2026 12:34:09 +0800 Subject: [PATCH] fix(agents): do not hide invalid config with file fallback (#4952) * fix(agents): do not hide invalid config with file fallback * test(agents): cover invalid on-disk config fallback * fix(agents): resolve stores off the event loop * fix(agents): distinguish missing nested config from main config * fix(agents): reject missing explicit config path * test(agents): isolate config fallback test * test(agents): isolate router blocking IO coverage * test(agents): pin malformed config.yaml parse-error propagation An unparseable config.yaml used to be swallowed by the broad except Exception and silently downgrade to FileAgentStore. The narrowed except FileNotFoundError already propagates yaml.ParserError/ScannerError; pin that contract with a real on-disk config instead of monkeypatched get_app_config. --- backend/app/gateway/routers/agents.py | 25 ++++--- .../deerflow/persistence/agents/__init__.py | 26 +++++-- .../tests/blocking_io/test_agents_router.py | 34 +++++++-- backend/tests/test_agent_storage_backend.py | 75 ++++++++++++++++++- 4 files changed, 137 insertions(+), 23 deletions(-) diff --git a/backend/app/gateway/routers/agents.py b/backend/app/gateway/routers/agents.py index 20da60ea9..49840bac2 100644 --- a/backend/app/gateway/routers/agents.py +++ b/backend/app/gateway/routers/agents.py @@ -19,7 +19,7 @@ from deerflow.config.agents_config import ( ) from deerflow.config.app_config import get_app_config from deerflow.config.paths import get_paths -from deerflow.persistence.agents import AgentExistsError, get_agent_store +from deerflow.persistence.agents import AgentDeleteOutcome, AgentExistsError, get_agent_store from deerflow.runtime.user_context import get_effective_user_id logger = logging.getLogger(__name__) @@ -251,10 +251,14 @@ async def check_agent_name(name: str) -> dict: _validate_agent_name(name) normalized = _normalize_agent_name(name) user_id = get_effective_user_id() + # Availability is defined by the active backend and stays consistent with # create()'s conflict rule (file: per-user or legacy dir; db: a row). The # exists() probe is filesystem IO / a DB round trip, so keep it off the loop. - exists = await asyncio.to_thread(get_agent_store().exists, normalized, user_id=user_id) + def _exists() -> bool: + return get_agent_store().exists(normalized, user_id=user_id) + + exists = await asyncio.to_thread(_exists) return {"available": not exists, "name": normalized} @@ -334,11 +338,10 @@ async def create_agent_endpoint(request: AgentCreateRequest) -> AgentResponse: # model / model_settings / thinking_enabled / reasoning_effort (issue #4336). _apply_model_behavior(config_data, request) - store = get_agent_store() - def _create_agent() -> AgentResponse: # Worker thread: existence checks + persistence (file IO or a DB round # trip) must stay off the event loop. + store = get_agent_store() store.create(normalized_name, config_data, request.soul, user_id=user_id) logger.info("Created agent '%s'", normalized_name) agent_cfg = load_agent_config(normalized_name, user_id=user_id) @@ -453,11 +456,14 @@ async def update_agent(name: str, request: AgentUpdateRequest) -> AgentResponse: for key, value in preserve_non_managed_fields(agent_cfg).items(): updated.setdefault(key, value) - store = get_agent_store() # Persist config (when changed) and/or soul (when provided) off the # event loop. A no-change PATCH commits nothing and re-reads current state. if updated is not None or request.soul is not None: - await asyncio.to_thread(store.update, name, updated, request.soul, user_id=user_id) + + def _update_agent() -> None: + get_agent_store().update(name, updated, request.soul, user_id=user_id) + + await asyncio.to_thread(_update_agent) logger.info(f"Updated agent '{name}'") @@ -560,11 +566,12 @@ async def delete_agent(name: str) -> None: _validate_agent_name(name) name = _normalize_agent_name(name) user_id = get_effective_user_id() - store = get_agent_store() - try: # Off the event loop: file rmtree or a DB delete plus memory cleanup. - outcome = await asyncio.to_thread(store.delete, name, user_id=user_id) + def _delete_agent() -> AgentDeleteOutcome: + return get_agent_store().delete(name, user_id=user_id) + + outcome = await asyncio.to_thread(_delete_agent) except Exception as e: logger.error(f"Failed to delete agent '{name}': {e}", exc_info=True) raise HTTPException(status_code=500, detail=f"Failed to delete agent: {str(e)}") diff --git a/backend/packages/harness/deerflow/persistence/agents/__init__.py b/backend/packages/harness/deerflow/persistence/agents/__init__.py index 4ab1f1797..dd10bd385 100644 --- a/backend/packages/harness/deerflow/persistence/agents/__init__.py +++ b/backend/packages/harness/deerflow/persistence/agents/__init__.py @@ -8,6 +8,7 @@ persistence layer. from __future__ import annotations +import os from typing import TYPE_CHECKING from deerflow.persistence.agents.base import ( @@ -60,10 +61,13 @@ def make_agent_store(config: AppConfig) -> AgentStore: def get_agent_store() -> AgentStore: """Return the store for the current process's configuration. - Defaults to the file backend when no app config can be resolved — the free - functions in ``agents_config`` must keep working in lightweight contexts - (CLI, tests, tools) that never load a full ``config.yaml``. Only an - explicit ``agent_storage.backend: db`` diverges from the file default. + Defaults to the file backend only when search mode cannot find the main app + config — the free functions in ``agents_config`` must keep working in + lightweight contexts (CLI, tests, tools) that never load a full + ``config.yaml``. A missing explicit ``DEER_FLOW_CONFIG_PATH`` or a missing + nested config is an operator error and propagates instead of falling back. + Only an explicit ``agent_storage.backend: db`` diverges from the file + default once configuration resolves successfully. Cross-process invariant (the ``db`` backend's whole point): the per-run agent build runs in the **graph subprocess**, a different process from the @@ -76,12 +80,20 @@ def get_agent_store() -> AgentStore: downgraded to node-local ``file``. Pinned by ``test_get_agent_store_resolves_db_backend_from_on_disk_config``. """ - from deerflow.config.app_config import get_app_config + from deerflow.config.app_config import AppConfig, get_app_config try: config = get_app_config() - except Exception: # noqa: BLE001 — no resolvable config → file default - return _file_store() + except FileNotFoundError: + if os.getenv("DEER_FLOW_CONFIG_PATH"): + raise + # ``get_app_config()`` also loads optional nested config files. Only + # fall back when the main config itself is absent. + try: + AppConfig.resolve_config_path() + except FileNotFoundError: + return _file_store() + raise return make_agent_store(config) diff --git a/backend/tests/blocking_io/test_agents_router.py b/backend/tests/blocking_io/test_agents_router.py index a05a0afbd..3903f2c6d 100644 --- a/backend/tests/blocking_io/test_agents_router.py +++ b/backend/tests/blocking_io/test_agents_router.py @@ -1,11 +1,10 @@ """Regression anchors: the custom-agent router must not block the event loop. -``app.gateway.routers.agents.create_agent_endpoint`` and ``delete_agent`` are -async route handlers that resolve the agent directory (``Paths.base_dir`` calls -``Path.resolve``), probe it (``Path.exists``), and create/remove it (``mkdir``, -config/SOUL writes, ``shutil.rmtree``) — all blocking IO. Both offload that work -via ``asyncio.to_thread``; if any of it regresses back onto the event loop, the -strict Blockbuster gate raises ``BlockingError`` and these tests fail. +The custom-agent handlers resolve the active store, access agent directories, +and may perform filesystem or database IO. Create/read/check/update/delete all +offload their complete blocking operations via ``asyncio.to_thread``; if any of +that work regresses onto the event loop, the strict Blockbuster gate raises +``BlockingError`` and these tests fail. Imports live at module scope so the one-time FastAPI app construction (which reads files while building OpenAPI schemas) happens at collection time, not on @@ -23,11 +22,13 @@ import pytest from app.gateway.routers.agents import ( AgentCreateRequest, + AgentUpdateRequest, check_agent_name, create_agent_endpoint, delete_agent, get_agent, list_agents, + update_agent, ) from deerflow.config.agents_api_config import load_agents_api_config_from_dict from deerflow.config.paths import get_paths @@ -36,6 +37,13 @@ from deerflow.runtime.user_context import get_effective_user_id pytestmark = pytest.mark.asyncio +@pytest.fixture(autouse=True) +def _isolate_agent_store_config(tmp_path: Path, monkeypatch) -> None: + monkeypatch.delenv("DEER_FLOW_CONFIG_PATH", raising=False) + monkeypatch.setenv("DEER_FLOW_PROJECT_ROOT", str(tmp_path)) + monkeypatch.setattr("deerflow.config.app_config._legacy_config_candidates", lambda: ()) + + async def test_create_agent_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None: monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path)) monkeypatch.setattr("deerflow.config.paths._paths", None) @@ -92,3 +100,17 @@ async def test_read_endpoints_do_not_block_event_loop(tmp_path: Path, monkeypatc assert (await check_agent_name("never-created-agent"))["available"] is True finally: load_agents_api_config_from_dict({}) + + +async def test_update_agent_does_not_block_event_loop(tmp_path: Path, monkeypatch) -> None: + monkeypatch.setenv("DEER_FLOW_HOME", str(tmp_path)) + monkeypatch.setattr("deerflow.config.paths._paths", None) + load_agents_api_config_from_dict({"enabled": True}) + try: + await create_agent_endpoint(AgentCreateRequest(name="loop-update-agent", soul="Original soul")) + + response = await update_agent("loop-update-agent", AgentUpdateRequest(description="Updated description")) + + assert response.description == "Updated description" + finally: + load_agents_api_config_from_dict({}) diff --git a/backend/tests/test_agent_storage_backend.py b/backend/tests/test_agent_storage_backend.py index d4b98ae42..f2ace812a 100644 --- a/backend/tests/test_agent_storage_backend.py +++ b/backend/tests/test_agent_storage_backend.py @@ -263,9 +263,82 @@ def test_get_agent_store_falls_back_to_file_without_config(tmp_path, monkeypatch """The ``except -> file`` fallback is for genuinely unresolvable config only (CLI/tests); it must not fire when a config exists — that asymmetry is what keeps a misconfigured graph process from silently downgrading db to file.""" - monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(tmp_path / "does-not-exist.yaml")) + monkeypatch.delenv("DEER_FLOW_CONFIG_PATH", raising=False) + monkeypatch.setenv("DEER_FLOW_PROJECT_ROOT", str(tmp_path)) + from deerflow.config import app_config + + monkeypatch.setattr(app_config, "_legacy_config_candidates", lambda: ()) try: reset_app_config() assert isinstance(get_agent_store(), FileAgentStore) finally: reset_app_config() + + +def test_get_agent_store_does_not_fallback_when_explicit_config_is_missing(tmp_path, monkeypatch): + """An explicit config path is an operator assertion and must fail closed.""" + monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(tmp_path / "does-not-exist.yaml")) + try: + reset_app_config() + with pytest.raises(FileNotFoundError, match="DEER_FLOW_CONFIG_PATH"): + get_agent_store() + finally: + reset_app_config() + + +def test_get_agent_store_does_not_hide_invalid_config(monkeypatch): + """Only missing config falls back; config errors must reach the caller.""" + from deerflow.config import app_config + + def raise_invalid_config(): + raise ValueError("invalid config") + + monkeypatch.setattr(app_config, "get_app_config", raise_invalid_config) + with pytest.raises(ValueError, match="invalid config"): + get_agent_store() + + +def test_get_agent_store_propagates_invalid_on_disk_config(tmp_path, monkeypatch): + """A present config with an invalid backend must fail instead of falling back.""" + cfg_path = tmp_path / "config.yaml" + _write_min_config(cfg_path, {"agent_storage": {"backend": "invalid"}}) + monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(cfg_path)) + try: + reset_app_config() + with pytest.raises(ValueError, match="agent_storage.backend"): + get_agent_store() + finally: + reset_app_config() + + +def test_get_agent_store_propagates_malformed_yaml(tmp_path, monkeypatch): + """An unparseable config.yaml must surface the parse error, not fall back to file.""" + cfg_path = tmp_path / "config.yaml" + cfg_path.write_text("agent_storage: [oops\n", encoding="utf-8") + monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(cfg_path)) + try: + reset_app_config() + with pytest.raises(yaml.YAMLError): # ParserError/ScannerError, previously swallowed + get_agent_store() + finally: + reset_app_config() + + +def test_get_agent_store_does_not_fallback_when_extensions_config_is_missing(tmp_path, monkeypatch): + """A missing nested config must not look like a missing main config.""" + cfg_path = tmp_path / "config.yaml" + _write_min_config( + cfg_path, + { + "agent_storage": {"backend": "db"}, + "database": {"backend": "sqlite", "sqlite_dir": str(tmp_path / "db")}, + }, + ) + monkeypatch.setenv("DEER_FLOW_CONFIG_PATH", str(cfg_path)) + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(tmp_path / "missing-extensions.json")) + try: + reset_app_config() + with pytest.raises(FileNotFoundError, match="Extensions config"): + get_agent_store() + finally: + reset_app_config()