From f2f9e6f6fe7607c59d07bb6ed578cf85988dcf83 Mon Sep 17 00:00:00 2001 From: kuse <3133746534@qq.com> Date: Wed, 30 Sep 2026 10:48:26 +0800 Subject: [PATCH] fix(config): guard numeric config fields that accepted silently-broken values (#6026) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(config): guard numeric config fields that accepted silently-broken values Ten numeric fields across six config models admitted zero, negatives, or booleans (YAML/JSON true coerces to 1) and only failed at runtime — or kept running with a nonsense value: - CircuitBreakerConfig: failure_threshold >= 1, recovery_timeout_sec > 0 - McpServerConfig: tool_call_timeout > 0, session_init_timeout > 0 - McpOAuthConfig.refresh_skew_seconds >= 0 (0 stays valid) - ModelConfig.stream_chunk_timeout > 0; context_window keeps gt=0 and gains boolean rejection (review follow-up) - RunEventsConfig.max_trace_content >= 1 - SandboxConfig.port 1..65535, idle_timeout >= 0 (0 = documented disable) - SummarizationConfig.trim_tokens_to_summarize >= 1 Review follow-ups: boolean rejection extended to every bounded numeric sibling in sandbox_config (replicas, acquire_timeout, burst_limit, health_check_skip_seconds, bash_output_max_chars, read_file_output_max_chars, ls_output_max_chars, bash_command_timeout, temporary_grant_ttl, renewal_interval_seconds, ttl_multiplier) so the file is uniform, and to model_config.context_window. Tests cover allow_inf_nan=False rejection (float('inf')/'nan') and the refresh_skew_seconds=0 boundary. Generated-by: ZCode (GLM, coding agent) Authored-by: liwenjie200543 * style(config): apply Ruff formatting to numeric guards Signed-off-by: Willem Jiang --------- Signed-off-by: Willem Jiang Co-authored-by: liwenjie200543 Co-authored-by: Willem Jiang --- .../harness/deerflow/config/app_config.py | 21 ++- .../deerflow/config/extensions_config.py | 26 ++- .../harness/deerflow/config/model_config.py | 10 ++ .../deerflow/config/run_events_config.py | 11 +- .../harness/deerflow/config/sandbox_config.py | 39 ++++ .../deerflow/config/summarization_config.py | 11 +- backend/tests/test_numeric_config_guards.py | 166 ++++++++++++++++++ 7 files changed, 278 insertions(+), 6 deletions(-) create mode 100644 backend/tests/test_numeric_config_guards.py diff --git a/backend/packages/harness/deerflow/config/app_config.py b/backend/packages/harness/deerflow/config/app_config.py index d3b7589801..9ba0308625 100644 --- a/backend/packages/harness/deerflow/config/app_config.py +++ b/backend/packages/harness/deerflow/config/app_config.py @@ -7,7 +7,7 @@ from typing import Any, Literal, Self import yaml from dotenv import load_dotenv -from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, model_validator +from pydantic import BaseModel, ConfigDict, Field, PrivateAttr, field_validator, model_validator from deerflow.config.acp_config import ACPAgentConfig, load_acp_config_from_dict from deerflow.config.agent_storage_config import AgentStorageConfig @@ -77,8 +77,23 @@ CONFIG_FILE_DATABASE_DEFAULTS = { class CircuitBreakerConfig(BaseModel): """Configuration for the LLM Circuit Breaker.""" - failure_threshold: int = Field(default=5, description="Number of consecutive failures before tripping the circuit") - recovery_timeout_sec: int = Field(default=60, description="Time in seconds before attempting to recover the circuit") + failure_threshold: int = Field( + default=5, + ge=1, + description="Number of consecutive failures before tripping the circuit", + ) + recovery_timeout_sec: int = Field( + default=60, + gt=0, + description="Time in seconds before attempting to recover the circuit", + ) + + @field_validator("failure_threshold", "recovery_timeout_sec", mode="before") + @classmethod + def _reject_boolean_circuit_settings(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be an integer, not a boolean") + return value class LlmCallConfig(BaseModel): diff --git a/backend/packages/harness/deerflow/config/extensions_config.py b/backend/packages/harness/deerflow/config/extensions_config.py index 3a116e8726..1cd63d8ab9 100644 --- a/backend/packages/harness/deerflow/config/extensions_config.py +++ b/backend/packages/harness/deerflow/config/extensions_config.py @@ -198,7 +198,19 @@ class McpOAuthConfig(BaseModel): token_type_field: str = Field(default="token_type", description="Field name containing token type in token response") expires_in_field: str = Field(default="expires_in", description="Field name containing expiry (seconds) in token response") default_token_type: str = Field(default="Bearer", description="Default token type when missing in token response") - refresh_skew_seconds: int = Field(default=60, description="Refresh token this many seconds before expiry") + refresh_skew_seconds: int = Field( + default=60, + ge=0, + description="Refresh token this many seconds before expiry", + ) + + @field_validator("refresh_skew_seconds", mode="before") + @classmethod + def _reject_boolean_refresh_skew(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be an integer, not a boolean") + return value + extra_token_params: dict[str, str] = Field(default_factory=dict, description="Additional form params sent to token endpoint") model_config = ConfigDict(extra="allow") @@ -232,10 +244,14 @@ class McpServerConfig(BaseModel): ) tool_call_timeout: float | None = Field( default=None, + gt=0, + allow_inf_nan=False, description=("Timeout in seconds for individual stdio MCP tool calls and durable-task calls on every transport. Other HTTP/SSE tools use transport-level timeouts. None means no call-level timeout."), ) session_init_timeout: float | None = Field( default=DEFAULT_MCP_SESSION_INIT_TIMEOUT, + gt=0, + allow_inf_nan=False, description=( "Timeout in seconds for MCP server bring-up: tool discovery (subprocess spawn + initialize + tools/list) " "and persistent stdio session initialization, plus ephemeral HTTP/SSE durable-task session " @@ -243,6 +259,14 @@ class McpServerConfig(BaseModel): "construction or the task poller indefinitely. None means no timeout." ), ) + + @field_validator("tool_call_timeout", "session_init_timeout", mode="before") + @classmethod + def _reject_boolean_mcp_timeouts(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be a number, not a boolean") + return value + task_toolsets: list[McpTaskToolsetConfig] = Field( default_factory=list, description="Ordinary submit/status/cancel tool groups managed by the durable MCP task runtime", diff --git a/backend/packages/harness/deerflow/config/model_config.py b/backend/packages/harness/deerflow/config/model_config.py index ba956cae62..0e18682ccb 100644 --- a/backend/packages/harness/deerflow/config/model_config.py +++ b/backend/packages/harness/deerflow/config/model_config.py @@ -184,6 +184,8 @@ class ModelConfig(BaseModel): ) stream_chunk_timeout: float | None = Field( default=None, + gt=0, + allow_inf_nan=False, description=( "Maximum seconds to wait between successive streaming chunks before " "langchain-openai raises StreamChunkTimeoutError. None means use the " @@ -192,6 +194,14 @@ class ModelConfig(BaseModel): "interactive endpoints. Has no effect on non-OpenAI-compatible providers." ), ) + + @field_validator("stream_chunk_timeout", "context_window", mode="before") + @classmethod + def _reject_boolean_numeric_settings(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be a number, not a boolean") + return value + thinking: dict | None = Field( default_factory=lambda: None, description=( diff --git a/backend/packages/harness/deerflow/config/run_events_config.py b/backend/packages/harness/deerflow/config/run_events_config.py index cddd9061fd..b71f6e310e 100644 --- a/backend/packages/harness/deerflow/config/run_events_config.py +++ b/backend/packages/harness/deerflow/config/run_events_config.py @@ -15,7 +15,7 @@ from __future__ import annotations from typing import Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, field_validator class RunEventsConfig(BaseModel): @@ -25,8 +25,17 @@ class RunEventsConfig(BaseModel): ) max_trace_content: int = Field( default=10240, + ge=1, description="Maximum trace content size in bytes before truncation (db backend only).", ) + + @field_validator("max_trace_content", mode="before") + @classmethod + def _reject_boolean_max_trace_content(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be an integer, not a boolean") + return value + track_token_usage: bool = Field( default=True, description="Whether RunJournal should accumulate token counts to RunRow.", diff --git a/backend/packages/harness/deerflow/config/sandbox_config.py b/backend/packages/harness/deerflow/config/sandbox_config.py index cc42e380a1..d012ce727f 100644 --- a/backend/packages/harness/deerflow/config/sandbox_config.py +++ b/backend/packages/harness/deerflow/config/sandbox_config.py @@ -36,6 +36,14 @@ class SandboxNetworkConfig(BaseModel): le=3600, description="Lifetime in seconds for the temporary approval choice.", ) + + @field_validator("temporary_grant_ttl", mode="before") + @classmethod + def _reject_boolean_temporary_grant_ttl(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be an integer, not a boolean") + return value + proxy_image: str = Field( default="ghcr.io/bytedance/deer-flow-sandbox-network-proxy:latest", min_length=1, @@ -112,6 +120,14 @@ class SandboxOwnershipConfig(BaseModel): allow_inf_nan=False, description="Lease TTL as a multiple of renewal_interval_seconds. At least 2, so a single missed renewal (slow host, brief Redis blip) cannot expire a live owner's lease. Default 4 tolerates three consecutive misses.", ) + + @field_validator("renewal_interval_seconds", "ttl_multiplier", mode="before") + @classmethod + def _reject_boolean_ownership_settings(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be a number, not a boolean") + return value + key_prefix: str = Field( default="deerflow:sandbox:owner", description="Redis key prefix for ownership leases. Only applies to the redis ownership type.", @@ -204,6 +220,8 @@ class SandboxConfig(BaseModel): ) port: int | None = Field( default=None, + ge=1, + le=65535, description="Base port for sandbox containers", ) replicas: int | None = Field( @@ -231,8 +249,29 @@ class SandboxConfig(BaseModel): ) idle_timeout: int | None = Field( default=None, + ge=0, description="Idle timeout in seconds before released warm sandboxes/VMs are stopped (default: 600 = 10 minutes). Set to 0 to disable.", ) + + @field_validator( + "port", + "replicas", + "acquire_timeout", + "burst_limit", + "idle_timeout", + "health_check_skip_seconds", + "bash_output_max_chars", + "read_file_output_max_chars", + "ls_output_max_chars", + "bash_command_timeout", + mode="before", + ) + @classmethod + def _reject_boolean_numeric_settings(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be a number, not a boolean") + return value + health_check_skip_seconds: float | None = Field( default=None, ge=0, diff --git a/backend/packages/harness/deerflow/config/summarization_config.py b/backend/packages/harness/deerflow/config/summarization_config.py index 965cd2ac12..043e26cbca 100644 --- a/backend/packages/harness/deerflow/config/summarization_config.py +++ b/backend/packages/harness/deerflow/config/summarization_config.py @@ -3,7 +3,7 @@ import math from typing import Literal -from pydantic import BaseModel, Field, model_validator +from pydantic import BaseModel, Field, field_validator, model_validator ContextSizeType = Literal["fraction", "tokens", "messages"] DEFAULT_SKILL_FILE_READ_TOOL_NAMES: tuple[str, ...] = ("read_file", "read", "view", "cat") @@ -82,8 +82,17 @@ class SummarizationConfig(BaseModel): ) trim_tokens_to_summarize: int | None = Field( default=4000, + ge=1, description="Maximum tokens to keep when preparing messages for summarization. Pass null to skip trimming.", ) + + @field_validator("trim_tokens_to_summarize", mode="before") + @classmethod + def _reject_boolean_trim_tokens(cls, value: object) -> object: + if isinstance(value, bool): + raise ValueError("must be an integer, not a boolean") + return value + summary_prompt: str | None = Field( default=None, description="Custom prompt template for generating summaries. If not provided, uses the default LangChain prompt.", diff --git a/backend/tests/test_numeric_config_guards.py b/backend/tests/test_numeric_config_guards.py new file mode 100644 index 0000000000..efe550b97e --- /dev/null +++ b/backend/tests/test_numeric_config_guards.py @@ -0,0 +1,166 @@ +# SPDX-License-Identifier: MIT +"""Numeric config fields that previously accepted silently-broken values. + +Each guarded field used to admit zero, negatives, or booleans (YAML/JSON +``true`` coerces to ``1``) and only failed later at runtime — or worse, kept +running with a nonsense value. Bounds and the boolean rejection mirror the +guards already shipped for ``stream_bridge_config`` (heartbeat interval) and +``database_config`` (pool settings). +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from deerflow.config.app_config import CircuitBreakerConfig +from deerflow.config.extensions_config import McpOAuthConfig, McpServerConfig +from deerflow.config.model_config import ModelConfig +from deerflow.config.run_events_config import RunEventsConfig +from deerflow.config.sandbox_config import SandboxConfig +from deerflow.config.summarization_config import SummarizationConfig + + +class TestCircuitBreakerGuards: + def test_defaults_pass(self) -> None: + config = CircuitBreakerConfig() + assert config.failure_threshold == 5 + assert config.recovery_timeout_sec == 60 + + @pytest.mark.parametrize("field", ["failure_threshold", "recovery_timeout_sec"]) + @pytest.mark.parametrize("bad", [0, -1, True]) + def test_rejects_non_positive_and_boolean(self, field: str, bad: int | bool) -> None: + with pytest.raises(ValidationError): + CircuitBreakerConfig(**{field: bad}) + + +class TestMcpTimeoutGuards: + def test_defaults_pass(self) -> None: + config = McpServerConfig() + assert config.tool_call_timeout is None + assert config.session_init_timeout is not None + + @pytest.mark.parametrize("field", ["tool_call_timeout", "session_init_timeout"]) + @pytest.mark.parametrize("bad", [0, -1.0, True]) + def test_rejects_non_positive_and_boolean(self, field: str, bad: float | bool) -> None: + with pytest.raises(ValidationError): + McpServerConfig(**{field: bad}) + + def test_zero_point_five_and_none_are_valid(self) -> None: + config = McpServerConfig(tool_call_timeout=0.5, session_init_timeout=None) + assert config.tool_call_timeout == 0.5 + assert config.session_init_timeout is None + + @pytest.mark.parametrize("bad", [-1, True]) + def test_refresh_skew_rejects_negative_and_boolean(self, bad: int | bool) -> None: + with pytest.raises(ValidationError): + McpOAuthConfig(refresh_skew_seconds=bad) + + +class TestStreamChunkTimeoutGuard: + def test_none_default_passes(self) -> None: + config = ModelConfig(name="m", use="default", model="gpt-x", stream_chunk_timeout=None) + assert config.stream_chunk_timeout is None + + @pytest.mark.parametrize("bad", [0, -1.0, True]) + def test_rejects_non_positive_and_boolean(self, bad: float | bool) -> None: + with pytest.raises(ValidationError): + ModelConfig(name="m", use="default", model="gpt-x", stream_chunk_timeout=bad) + + +class TestMaxTraceContentGuard: + def test_default_passes(self) -> None: + assert RunEventsConfig().max_trace_content == 10240 + + @pytest.mark.parametrize("bad", [0, -1, True]) + def test_rejects_non_positive_and_boolean(self, bad: int | bool) -> None: + with pytest.raises(ValidationError): + RunEventsConfig(**{"max_trace_content": bad}) + + +class TestSandboxPortAndIdleTimeoutGuards: + def test_unset_defaults_pass(self) -> None: + config = SandboxConfig(use="local") + assert config.port is None + assert config.idle_timeout is None + + def test_zero_idle_timeout_is_documented_valid(self) -> None: + assert SandboxConfig(use="local", idle_timeout=0).idle_timeout == 0 + + @pytest.mark.parametrize("bad", [0, 65536, -1, True]) + def test_port_rejects_out_of_range_and_boolean(self, bad: int | bool) -> None: + with pytest.raises(ValidationError): + SandboxConfig(use="local", port=bad) + + @pytest.mark.parametrize("bad", [-1, True]) + def test_idle_timeout_rejects_negative_and_boolean(self, bad: int | bool) -> None: + with pytest.raises(ValidationError): + SandboxConfig(use="local", idle_timeout=bad) + + +class TestSiblingBoolRejection: + """Same-class siblings with bounds but no boolean rejection, flagged in review.""" + + def test_sandbox_numeric_fields_reject_booleans(self) -> None: + import pytest + + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + with pytest.raises(ValidationError): + SandboxConfig(use="local", replicas=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", acquire_timeout=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", burst_limit=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", health_check_skip_seconds=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", bash_output_max_chars=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", read_file_output_max_chars=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", ls_output_max_chars=True) + with pytest.raises(ValidationError): + SandboxConfig(use="local", bash_command_timeout=True) + with pytest.raises(ValidationError): + SandboxOwnershipConfig(renewal_interval_seconds=True) + with pytest.raises(ValidationError): + SandboxOwnershipConfig(ttl_multiplier=True) + + def test_ownership_defaults_still_pass(self) -> None: + from deerflow.config.sandbox_config import SandboxOwnershipConfig + + config = SandboxOwnershipConfig() + assert config.renewal_interval_seconds == 30.0 + assert config.ttl_multiplier == 4.0 + + def test_context_window_rejects_booleans_and_nonpositive(self) -> None: + with pytest.raises(ValidationError): + ModelConfig(name="m", use="default", model="gpt-x", context_window=True) + with pytest.raises(ValidationError): + ModelConfig(name="m", use="default", model="gpt-x", context_window=0) + with pytest.raises(ValidationError): + ModelConfig(name="m", use="default", model="gpt-x", context_window=-1) + + def test_allow_inf_nan_rejection(self) -> None: + with pytest.raises(ValidationError): + McpServerConfig(tool_call_timeout=float("inf")) + with pytest.raises(ValidationError): + ModelConfig(name="m", use="default", model="gpt-x", stream_chunk_timeout=float("inf")) + with pytest.raises(ValidationError): + ModelConfig(name="m", use="default", model="gpt-x", stream_chunk_timeout=float("nan")) + + def test_refresh_skew_zero_is_a_valid_boundary(self) -> None: + config = McpOAuthConfig(token_url="https://sso.example.org/token", refresh_skew_seconds=0) + assert config.refresh_skew_seconds == 0 + + +class TestTrimTokensToSummarizeGuard: + def test_default_and_none_pass(self) -> None: + assert SummarizationConfig().trim_tokens_to_summarize == 4000 + assert SummarizationConfig(trim_tokens_to_summarize=None).trim_tokens_to_summarize is None + + @pytest.mark.parametrize("bad", [0, -1, True]) + def test_rejects_non_positive_and_boolean(self, bad: int | bool) -> None: + with pytest.raises(ValidationError): + SummarizationConfig(**{"trim_tokens_to_summarize": bad})