From c177d0e542fb08f584f453e8f0001637c7589f3e Mon Sep 17 00:00:00 2001 From: ly-wang19 <94427531+ly-wang19@users.noreply.github.com> Date: Sun, 21 Jun 2026 22:16:00 +0800 Subject: [PATCH] fix(config): coerce null object config sections to their defaults (#3573) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(config): coerce null object config sections to their defaults #3434 made commented-out list sections (models/tools/tool_groups) parse as [] instead of crashing, but the same scenario still crashes for object sections: commenting out every key under e.g. memory: / summarization: / guardrails: makes PyYAML parse the value as None, and AppConfig then raises "Input should be a valid dictionary" for that section — breaking the documented `cp config.example.yaml config.yaml` first-run flow. Generalize the handling: a model_validator(mode="before") drops None-valued sections so each field falls back to its default (list sections -> [], object sections -> their default config). This subsumes the previous list-only field_validator and the database special-case. Required sections without a default (sandbox) still error when null. Adds test_app_config_coerces_commented_out_object_sections; the existing list-section regression test still passes. * docs+test: address review on null-section coercion - Correct the _drop_null_config_sections docstring: it does not subsume the database special-case; _apply_database_defaults still owns `database` and applies concrete defaults beyond null-coercion. - Strengthen test_app_config_coerces_commented_out_object_sections to assert each null section falls back to its expected default config type, not just that it is non-None. - Add test_app_config_null_required_section_still_errors covering the required-section (sandbox) "still errors when null" claim. --------- Co-authored-by: ly-wang19 --- .../harness/deerflow/config/app_config.py | 32 +++++++---- backend/tests/test_app_config_reload.py | 53 +++++++++++++++++++ 2 files changed, 74 insertions(+), 11 deletions(-) diff --git a/backend/packages/harness/deerflow/config/app_config.py b/backend/packages/harness/deerflow/config/app_config.py index e3cc1f7b3..fcfa44fb3 100644 --- a/backend/packages/harness/deerflow/config/app_config.py +++ b/backend/packages/harness/deerflow/config/app_config.py @@ -8,7 +8,7 @@ from typing import Any, Self import yaml from dotenv import load_dotenv -from pydantic import BaseModel, ConfigDict, Field, field_validator +from pydantic import BaseModel, ConfigDict, Field, model_validator from deerflow.config.acp_config import ACPAgentConfig, load_acp_config_from_dict from deerflow.config.agents_api_config import AgentsApiConfig, load_agents_api_config_from_dict @@ -161,20 +161,30 @@ class AppConfig(BaseModel): ), ) - @field_validator("models", "tools", "tool_groups", mode="before") + @model_validator(mode="before") @classmethod - def _coerce_null_list_sections(cls, value: Any) -> Any: - """Treat a present-but-empty config section as an empty list. + def _drop_null_config_sections(cls, data: Any) -> Any: + """Treat a present-but-null config section as absent so its default applies. Commenting out every entry under a top-level YAML key — e.g. ``models:`` - with only comments beneath it, exactly as shipped in - ``config.example.yaml`` — makes PyYAML parse the value as ``None``. - Without this, the documented ``cp config.example.yaml config.yaml`` - first-run flow crashes with an opaque ``Input should be a valid list`` - pydantic error. Coercing ``None`` to ``[]`` keeps that flow working and - matches the field's own ``default_factory=list``. + (a list) or ``memory:`` (an object), with only comments beneath it as + shipped throughout ``config.example.yaml`` — makes PyYAML parse the value + as ``None``. Without this, the documented ``cp config.example.yaml + config.yaml`` first-run flow crashes with an opaque ``Input should be a + valid list`` / ``valid dictionary`` pydantic error for that section. + + Dropping the ``None`` lets each field fall back to its default: list + sections become ``[]`` via ``default_factory=list`` and object sections + get their default config. This generalizes the earlier list-only + handling to every section that defines a default. The ``database`` + section is independent and still owned by ``_apply_database_defaults`` + (in ``from_file``), which applies concrete defaults beyond null-coercion. + Required sections without a default (``sandbox``) intentionally still + error when null — there is nothing to fall back to. """ - return [] if value is None else value + if isinstance(data, dict): + return {key: value for key, value in data.items() if value is not None} + return data @classmethod def resolve_config_path(cls, config_path: str | None = None) -> Path: diff --git a/backend/tests/test_app_config_reload.py b/backend/tests/test_app_config_reload.py index ca98759bd..a5a8dd7a1 100644 --- a/backend/tests/test_app_config_reload.py +++ b/backend/tests/test_app_config_reload.py @@ -170,6 +170,59 @@ def test_app_config_coerces_commented_out_list_sections(tmp_path, monkeypatch): assert config.tool_groups == [] +def test_app_config_coerces_commented_out_object_sections(tmp_path, monkeypatch): + """Commenting out every entry under an object key makes PyYAML parse it as None. + + Same documented ``cp config.example.yaml config.yaml`` flow as the list + sections: object sections (memory, summarization, ...) must fall back to + their defaults instead of raising ``Input should be a valid dictionary``. + """ + config_path = tmp_path / "config.yaml" + extensions_path = tmp_path / "extensions_config.json" + _write_extensions_config(extensions_path) + config_path.write_text( + yaml.safe_dump( + { + "sandbox": {"use": "deerflow.sandbox.local:LocalSandboxProvider"}, + "memory": None, + "summarization": None, + "guardrails": None, + "tool_output": None, + "run_events": None, + } + ), + encoding="utf-8", + ) + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(extensions_path)) + + config = AppConfig.from_file(str(config_path)) + + # Each present-but-null object section falls back to a real default config + # object of the expected type (not merely non-None). + assert type(config.memory).__name__ == "MemoryConfig" + assert type(config.summarization).__name__ == "SummarizationConfig" + assert type(config.guardrails).__name__ == "GuardrailsConfig" + assert type(config.tool_output).__name__ == "ToolOutputConfig" + assert type(config.run_events).__name__ == "RunEventsConfig" + + +def test_app_config_null_required_section_still_errors(tmp_path, monkeypatch): + """A present-but-null *required* section still errors. + + ``sandbox`` has no default, so dropping a ``sandbox: null`` key leaves the + required field absent — there is nothing to fall back to (per + ``_drop_null_config_sections``), unlike the optional object sections above. + """ + config_path = tmp_path / "config.yaml" + extensions_path = tmp_path / "extensions_config.json" + _write_extensions_config(extensions_path) + config_path.write_text(yaml.safe_dump({"sandbox": None}), encoding="utf-8") + monkeypatch.setenv("DEER_FLOW_EXTENSIONS_CONFIG_PATH", str(extensions_path)) + + with pytest.raises(ValidationError): + AppConfig.from_file(str(config_path)) + + def test_app_config_warns_when_no_models_configured(tmp_path, monkeypatch, caplog): config_path = tmp_path / "config.yaml" extensions_path = tmp_path / "extensions_config.json"