diff --git a/backend/packages/harness/deerflow/agents/memory/backends/honcho/config.py b/backend/packages/harness/deerflow/agents/memory/backends/honcho/config.py index b6e983e38..45dc7fb6a 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/honcho/config.py +++ b/backend/packages/harness/deerflow/agents/memory/backends/honcho/config.py @@ -9,6 +9,7 @@ plain HTTP; a configured ``api_key`` over plain HTTP requires the explicit from __future__ import annotations import re +from collections.abc import Callable from dataclasses import dataclass, field from math import isfinite from typing import Any @@ -21,13 +22,48 @@ def sanitize_id(raw: str) -> str: return _ID_RE.sub("-", str(raw)).strip("-")[:64] +def _mapping(value: Any, name: str) -> dict[str, Any]: + """Narrow an operator-supplied nested value to a mapping. + + Falsy values (absent key, YAML null, ``{}``, ``""``) mean "unset" and keep + the default. A truthy non-mapping — a bare string, a YAML list — is a + config mistake, and naming the key here beats the ``AttributeError`` that + ``.get``/``.items`` would otherwise raise inside backend construction. The + mem0 and OpenViking backends already report these keys as ``ValueError``. + """ + if not value: + return {} + if not isinstance(value, dict): + raise ValueError(f"Honcho backend: {name} must be a mapping, got {type(value).__name__}") + return value + + +def _number[T](cfg: dict[str, Any], key: str, default: T, cast: Callable[[Any], T]) -> T: + """Narrow an operator-supplied numeric knob. + + Falsy values (absent key, YAML null, empty string) mean "unset" and keep the + default — the same line ``_mapping`` draws above. Anything else that cannot be + cast is a config mistake, and naming the key here beats the ``TypeError`` that + ``float(None)`` or ``int({...})`` raise from inside backend construction + without mentioning which knob or which file is wrong. Numeric strings keep + working, because that is what ``float``/``int`` already accept. + """ + value = cfg.get(key, default) + if value is None or (isinstance(value, str) and not value.strip()): + return default + try: + return cast(value) + except (TypeError, ValueError): + raise ValueError(f"Honcho backend: {key} must be a number, got {type(value).__name__}") from None + + def _parse_override_map(cfg: dict[str, Any], key: str) -> dict[str, str]: """Overrides map raw user ids to explicit workspace/peer ids; an empty or null VALUE is always a config mistake (empty string is falsy and would silently fall through to the default derivation; YAML null would stringify into an id literally named "None"), so fail fast at parse time.""" out: dict[str, str] = {} - for k, v in (cfg.get(key) or {}).items(): + for k, v in _mapping(cfg.get(key), key).items(): if v is None or not str(v).strip(): raise ValueError(f"Honcho backend: {key}[{k!r}] has an empty value; remove the entry or set a non-empty id.") out[str(k)] = str(v) @@ -65,7 +101,7 @@ class HonchoConfig: @classmethod def from_backend_config(cls, backend_config: dict[str, Any] | None) -> HonchoConfig: cfg = dict(backend_config or {}) - failure_policy = cfg.get("failure_policy") or {} + failure_policy = _mapping(cfg.get("failure_policy"), "failure_policy") base_url = str(cfg.get("base_url", "http://localhost:8000")).rstrip("/") api_key = cfg.get("api_key") or None allow_insecure = bool(cfg.get("allow_insecure_http", False)) @@ -78,10 +114,10 @@ class HonchoConfig: workspace_overrides=_parse_override_map(cfg, "workspace_overrides"), user_peer_overrides=_parse_override_map(cfg, "user_peer_overrides"), assistant_peer=str(cfg.get("assistant_peer", "deerflow")), - timeout_seconds=float(cfg.get("timeout_seconds", 10.0)), - connect_timeout_seconds=float(cfg.get("connect_timeout_seconds", 3.0)), - message_char_limit=int(cfg.get("message_char_limit", 8000)), - max_injection_chars=int(cfg.get("max_injection_chars", 6000)), + timeout_seconds=_number(cfg, "timeout_seconds", 10.0, float), + connect_timeout_seconds=_number(cfg, "connect_timeout_seconds", 3.0, float), + message_char_limit=_number(cfg, "message_char_limit", 8000, int), + max_injection_chars=_number(cfg, "max_injection_chars", 6000, int), allow_insecure_http=allow_insecure, read_fail_closed=str(failure_policy.get("read", "")).lower() == "fail_closed", storage_path=str(cfg.get("storage_path") or ""), diff --git a/backend/packages/harness/deerflow/agents/memory/backends/mem0/config.py b/backend/packages/harness/deerflow/agents/memory/backends/mem0/config.py index d4f2be8b2..8b7712c02 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/mem0/config.py +++ b/backend/packages/harness/deerflow/agents/memory/backends/mem0/config.py @@ -10,6 +10,7 @@ fast, not silently fall back to defaults. from __future__ import annotations import os +from collections.abc import Callable from dataclasses import dataclass from math import isfinite from typing import Any @@ -23,6 +24,26 @@ _READ_POLICIES = frozenset({"fail_open", "fail_closed"}) _WRITE_POLICIES = frozenset({"log_and_drop", "raise"}) +def _number[T](cfg: dict[str, Any], key: str, default: T, cast: Callable[[Any], T]) -> T: + """Read a numeric knob, treating a value-less key as unset. + + ``top_k:`` with nothing after it in YAML arrives as ``None``, and the range + checks below never see it: ``int(None)`` raises ``TypeError`` from inside + backend construction without naming the knob or the file. A key carrying no + value keeps its default -- the same line ``failure_policy`` and + ``allow_insecure_http`` already draw -- and a value that cannot be cast is + reported as the config mistake it is. Numeric strings keep working, because + that is what ``int``/``float`` already accept. + """ + value = cfg.get(key, default) + if value is None or (isinstance(value, str) and not value.strip()): + return default + try: + return cast(value) + except (TypeError, ValueError): + raise ValueError(f"mem0 {key} must be a number, got {type(value).__name__}") from None + + @dataclass(frozen=True) class Mem0Config: """Validated knobs for the mem0 HTTP backend.""" @@ -86,10 +107,10 @@ class Mem0Config: api_key_env=str(cfg.get("api_key_env", "MEM0_API_KEY")), base_url=str(cfg.get("base_url", "https://api.mem0.ai")).rstrip("/"), allow_insecure_http=allow_insecure_http, - top_k=int(cfg.get("top_k", 8)), - score_threshold=float(cfg.get("score_threshold", 0.1)), - max_injection_chars=int(cfg.get("max_injection_chars", 12000)), - timeout_seconds=float(cfg.get("timeout_seconds", 10.0)), + top_k=_number(cfg, "top_k", 8, int), + score_threshold=_number(cfg, "score_threshold", 0.1, float), + max_injection_chars=_number(cfg, "max_injection_chars", 12000, int), + timeout_seconds=_number(cfg, "timeout_seconds", 10.0, float), startup_policy=str(cfg.get("startup_policy", "fail_fast")), read_policy=str(failure_policy.get("read", "fail_open")), write_policy=str(failure_policy.get("write", "log_and_drop")), diff --git a/backend/packages/harness/deerflow/agents/memory/backends/openviking/config.py b/backend/packages/harness/deerflow/agents/memory/backends/openviking/config.py index d29296c5b..5a4364269 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/openviking/config.py +++ b/backend/packages/harness/deerflow/agents/memory/backends/openviking/config.py @@ -4,6 +4,7 @@ from __future__ import annotations import os import re +from collections.abc import Callable from dataclasses import dataclass, field from math import isfinite from typing import Any, Literal @@ -74,10 +75,10 @@ class OpenVikingConfig: api_key=os.environ.get(api_key_env, "").strip(), api_key_env=api_key_env, default_peer_id=str(cfg.pop("default_peer_id", "deerflow")).strip(), - timeout_seconds=float(cfg.pop("timeout_seconds", 30.0)), - search_top_k=int(retrieval.pop("top_k", 8)), - score_threshold=_optional_float(retrieval.pop("score_threshold", None)), - max_injection_chars=int(retrieval.pop("max_injection_chars", 12_000)), + timeout_seconds=_number(cfg, "timeout_seconds", 30.0, float), + search_top_k=_number(retrieval, "top_k", 8, int), + score_threshold=_optional_float(retrieval.pop("score_threshold", None), "score_threshold"), + max_injection_chars=_number(retrieval, "max_injection_chars", 12_000, int), content_mode=str(retrieval.pop("content_mode", "overview")).lower(), # type: ignore[arg-type] injection_query=str( retrieval.pop( @@ -92,7 +93,7 @@ class OpenVikingConfig: cfg.pop("allow_insecure_http", False), "allow_insecure_http", ), - max_seen_message_ids=int(cfg.pop("max_seen_message_ids", 512)), + max_seen_message_ids=_number(cfg, "max_seen_message_ids", 512, int), ) unknown = sorted( @@ -157,8 +158,35 @@ def _mapping(value: Any, name: str) -> dict[str, Any]: return dict(value) -def _optional_float(value: Any) -> float | None: - return None if value is None else float(value) +def _number[T](cfg: dict[str, Any], key: str, default: T, cast: Callable[[Any], T]) -> T: + """Read a numeric knob, treating a value-less key as unset. + + ``timeout_seconds:`` with nothing after it in YAML arrives as ``None``, and + ``float(None)`` raises ``TypeError`` from inside backend construction without + naming the knob or the file. A key carrying no value keeps its default -- the + line ``_mapping`` and ``_boolean`` already draw -- and a value that cannot be + cast is reported as the config mistake it is. Numeric strings keep working, + because that is what ``int``/``float`` already accept. + """ + value = cfg.pop(key, default) + if value is None or (isinstance(value, str) and not value.strip()): + return default + try: + return cast(value) + except (TypeError, ValueError): + raise ValueError(f"OpenViking {key} must be a number, got {type(value).__name__}") from None + + +def _optional_float(value: Any, name: str) -> float | None: + """Read a numeric knob where ``None`` is a meaningful value of its own: an + unset score threshold means "apply none", so it stays ``None`` rather than + falling back to a default.""" + if value is None or (isinstance(value, str) and not value.strip()): + return None + try: + return float(value) + except (TypeError, ValueError): + raise ValueError(f"OpenViking {name} must be a number, got {type(value).__name__}") from None def _boolean(value: Any, name: str) -> bool: diff --git a/backend/tests/test_honcho_memory_backend.py b/backend/tests/test_honcho_memory_backend.py index 28119c99b..0c9c4d92b 100644 --- a/backend/tests/test_honcho_memory_backend.py +++ b/backend/tests/test_honcho_memory_backend.py @@ -71,6 +71,71 @@ class TestHonchoConfig: with pytest.raises(ValueError, match="user_peer_overrides"): HonchoConfig.from_backend_config({"user_peer_overrides": {"bob": " "}}) + @pytest.mark.parametrize( + ("key", "value"), + [ + pytest.param("failure_policy", "fail_closed", id="policy-string"), + pytest.param("failure_policy", ["fail_closed"], id="policy-list"), + pytest.param("workspace_overrides", "shared", id="overrides-string"), + pytest.param("workspace_overrides", ["alice"], id="overrides-list"), + pytest.param("user_peer_overrides", 5, id="peer-overrides-int"), + ], + ) + def test_non_mapping_nested_values_rejected_as_config_error(self, key, value): + """A truthy non-mapping is the operator's mistake, not an internal error: + mem0 and OpenViking raise ValueError for these same keys, so Honcho must + name the offending key instead of surfacing ``AttributeError`` from a + ``.get``/``.items`` call inside backend construction.""" + with pytest.raises(ValueError, match=f"{key} must be a mapping"): + HonchoConfig.from_backend_config({key: value}) + + @pytest.mark.parametrize("key", ["failure_policy", "workspace_overrides", "user_peer_overrides"]) + @pytest.mark.parametrize("value", [None, "", [], {}]) + def test_empty_nested_values_still_mean_unset(self, key, value): + cfg = HonchoConfig.from_backend_config({key: value}) + assert cfg.read_fail_closed is False + assert cfg.workspace_overrides == {} + assert cfg.user_peer_overrides == {} + + @pytest.mark.parametrize( + ("key", "value"), + [ + pytest.param("timeout_seconds", ["slow"], id="timeout-list"), + pytest.param("timeout_seconds", "slow", id="timeout-text"), + pytest.param("connect_timeout_seconds", {"seconds": 5}, id="connect-timeout-mapping"), + pytest.param("message_char_limit", "wide", id="message-limit-text"), + pytest.param("max_injection_chars", ["1000"], id="injection-limit-list"), + ], + ) + def test_unusable_numeric_values_rejected_as_config_error(self, key, value): + """``float([...])`` and ``int("wide")`` raise a TypeError that names neither + the knob nor the config file, so a mistyped scalar reaches the operator as + an internal traceback — the same symptom as the nested values above, one + block below them.""" + with pytest.raises(ValueError, match=f"{key} must be a number"): + HonchoConfig.from_backend_config({key: value}) + + @pytest.mark.parametrize( + ("key", "default"), + [ + ("timeout_seconds", 10.0), + ("connect_timeout_seconds", 3.0), + ("message_char_limit", 8000), + ("max_injection_chars", 6000), + ], + ) + @pytest.mark.parametrize("value", [None, "", " "]) + def test_empty_numeric_values_still_mean_unset(self, key, default, value): + cfg = HonchoConfig.from_backend_config({key: value}) + assert getattr(cfg, key) == default + + @pytest.mark.parametrize(("key", "value", "expected"), [("timeout_seconds", "2.5", 2.5), ("message_char_limit", "120", 120)]) + def test_numeric_strings_still_accepted(self, key, value, expected): + """float()/int() already accept numeric strings, so a quoted YAML scalar + must keep working; the guard is only for values that cannot be cast.""" + cfg = HonchoConfig.from_backend_config({key: value}) + assert getattr(cfg, key) == expected + @pytest.mark.parametrize( ("key", "value"), [ diff --git a/backend/tests/test_mem0_memory_backend.py b/backend/tests/test_mem0_memory_backend.py index 4ba1ba571..6a2254590 100644 --- a/backend/tests/test_mem0_memory_backend.py +++ b/backend/tests/test_mem0_memory_backend.py @@ -11,6 +11,14 @@ import pytest from deerflow.agents.memory.backends.mem0.client import Mem0APIError, Mem0AuthError, Mem0Client from deerflow.agents.memory.backends.mem0.config import Mem0Config +#: What each numeric knob holds when the operator does not set it. +_NUMERIC_DEFAULTS: dict[str, float] = { + "top_k": 8, + "score_threshold": 0.1, + "max_injection_chars": 12000, + "timeout_seconds": 10.0, +} + class TestMem0Config: def test_defaults(self) -> None: @@ -86,6 +94,30 @@ class TestMem0Config: with pytest.raises(ValueError): Mem0Config.from_backend_config({key: value}) + @pytest.mark.parametrize("key", ["top_k", "score_threshold", "max_injection_chars", "timeout_seconds"]) + @pytest.mark.parametrize("unset", [None, "", " "]) + def test_numeric_knob_written_without_a_value_keeps_its_default(self, key: str, unset: object) -> None: + """An unquoted ``top_k:`` in YAML parses as unset, not as a broken backend.""" + cfg = Mem0Config.from_backend_config({key: unset}) + + assert getattr(cfg, key) == _NUMERIC_DEFAULTS[key] + + @pytest.mark.parametrize( + ("key", "value"), + [ + ("top_k", "eight"), + ("top_k", []), + ("score_threshold", "high"), + ("score_threshold", {"min": 0.2}), + ("max_injection_chars", ["12000"]), + ("timeout_seconds", "soon"), + ], + ) + def test_non_numeric_knob_names_the_key(self, key: str, value: object) -> None: + """The report has to say which knob is wrong, not just that a cast failed.""" + with pytest.raises(ValueError, match=f"mem0 {key} must be a number"): + Mem0Config.from_backend_config({key: value}) + @pytest.mark.parametrize("policy", ["read", "write"]) def test_invalid_failure_policy_rejected(self, policy: str) -> None: with pytest.raises(ValueError, match=policy): diff --git a/backend/tests/test_openviking_memory_backend.py b/backend/tests/test_openviking_memory_backend.py index c38ec91e4..267e65a56 100644 --- a/backend/tests/test_openviking_memory_backend.py +++ b/backend/tests/test_openviking_memory_backend.py @@ -248,6 +248,65 @@ def test_config_uses_single_user_key_and_rejects_legacy_trusted_fields( OpenVikingConfig.from_backend_config(_backend_config(tmp_path, max_connections=10)) +@pytest.mark.parametrize( + ("section", "key", "attr", "default"), + [ + (None, "timeout_seconds", "timeout_seconds", 30.0), + (None, "max_seen_message_ids", "max_seen_message_ids", 512), + ("retrieval", "top_k", "search_top_k", 8), + ("retrieval", "max_injection_chars", "max_injection_chars", 12_000), + ("retrieval", "score_threshold", "score_threshold", None), + ], +) +@pytest.mark.parametrize("unset", [None, "", " "]) +def test_numeric_knob_written_without_a_value_keeps_its_default( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + section: str | None, + key: str, + attr: str, + default: Any, + unset: Any, +) -> None: + """An unquoted ``top_k:`` in YAML parses as unset, not as a broken backend.""" + monkeypatch.setenv("OPENVIKING_API_KEY", "user-key") + config = _backend_config(tmp_path) + target = config if section is None else config[section] + target[key] = unset + + cfg = OpenVikingConfig.from_backend_config(config) + + assert getattr(cfg, attr) == default + + +@pytest.mark.parametrize( + ("section", "key", "value"), + [ + (None, "timeout_seconds", "soon"), + (None, "timeout_seconds", [30]), + (None, "max_seen_message_ids", "many"), + ("retrieval", "top_k", "four"), + ("retrieval", "top_k", {"count": 4}), + ("retrieval", "score_threshold", "high"), + ], +) +def test_non_numeric_knob_names_the_key( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + section: str | None, + key: str, + value: Any, +) -> None: + """The report has to say which knob is wrong, not just that a cast failed.""" + monkeypatch.setenv("OPENVIKING_API_KEY", "user-key") + config = _backend_config(tmp_path) + target = config if section is None else config[section] + target[key] = value + + with pytest.raises(ValueError, match=f"OpenViking {key} must be a number"): + OpenVikingConfig.from_backend_config(config) + + def test_backend_is_discovered_by_registered_name() -> None: reset_memory_manager() assert _scan_backends()["openviking"] is OpenVikingMemoryManager