mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-09-25 14:06:18 +00:00
fix(memory): report malformed backend_config values by key name (#5555)
* fix(memory): report non-mapping Honcho backend_config values as ValueError
failure_policy, workspace_overrides and user_peer_overrides were read with a
falsy-only `or {}` fallback, so a truthy non-mapping (a bare string, a YAML
list) reached .get/.items and escaped as a bare AttributeError from inside
backend construction. Route all three through one _mapping helper that keeps
falsy values meaning "unset" and names the offending key as a ValueError, the
posture the mem0 and OpenViking backends already take.
* fix(memory): name the Honcho numeric knobs that cannot be cast
timeout_seconds / connect_timeout_seconds / message_char_limit /
max_injection_chars still reached float() / int() with a YAML null or a
mapping, so the operator got a TypeError naming neither the key nor the
config file. Narrow all four through one _number helper that keeps falsy
values meaning "unset" the way the sibling api_key / storage_path scalars
already do, and reports a value that cannot be cast as a ValueError on the
key. Numeric strings keep parsing, since that is what float/int accept.
* fix(memory): report non-numeric backend_config knobs by name in mem0 and OpenViking
mem0 casts top_k, score_threshold, max_injection_chars and timeout_seconds, and
OpenViking casts timeout_seconds, max_seen_message_ids, retrieval.top_k and
retrieval.max_injection_chars, straight through int()/float(). A knob written
without a value in YAML therefore escapes as "TypeError: int() argument must be
a string..." from inside backend construction, naming neither which knob nor
which file is wrong, and a non-numeric value escapes as the equally anonymous
"could not convert string to float".
Both now resolve numeric knobs through a helper that keeps a value-less key at
its default, the way the same dicts already treat failure_policy and
allow_insecure_http, and turns an uncastable value into a ValueError that names
the knob. OpenViking's score_threshold keeps None as a meaningful value rather
than a default to fall back to.
This commit is contained in:
parent
1f437f86c6
commit
479d2f10c8
@ -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 ""),
|
||||
|
||||
@ -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")),
|
||||
|
||||
@ -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:
|
||||
|
||||
@ -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"),
|
||||
[
|
||||
|
||||
@ -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):
|
||||
|
||||
@ -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
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user