mirror of
https://github.com/bytedance/deer-flow.git
synced 2026-11-09 04:20:34 +00:00
fix(memory): make doctor validates the selected backend config (#7965)
A stale memory.backend_config (DeerMem-only keys left in place after switching manager_class to openviking) surfaced only when MemoryMiddleware constructed the manager mid-conversation, after model tokens were already spent; make doctor reported Ready because MemoryConfig accepts backend_config as untyped data. doctor now runs the backend's own side-effect-free validator (OpenVikingConfig.from_backend_config) and reports the offending keys before an agent turn is accepted. Backends without a static validator are skipped, as is disabled memory. Fixes #7962 Signed-off-by: Batianxieshen1 <280706568+Batianxieshen1@users.noreply.github.com> Co-authored-by: Batianxieshen1 <280706568+Batianxieshen1@users.noreply.github.com> Co-authored-by: Totoro <msy626836554@gmail.com> Co-authored-by: Willem Jiang <willem.jiang@gmail.com>
This commit is contained in:
parent
961262a17e
commit
c69e9ef0bc
@ -94,3 +94,109 @@ def test_no_arguments_ignores_inherited_runtime_configuration(doctor_checkout: P
|
||||
assert external_config.read_bytes() == config_bytes
|
||||
assert external_env.read_bytes() == env_bytes
|
||||
assert not external_home.exists()
|
||||
|
||||
|
||||
class TestMemoryBackendConfigCheck:
|
||||
"""check_memory_backend_config validates the selected backend statically."""
|
||||
|
||||
@pytest.fixture
|
||||
def doctor(self):
|
||||
import importlib.util
|
||||
|
||||
spec = importlib.util.spec_from_file_location("doctor_module", REPO_ROOT / "scripts" / "doctor.py")
|
||||
module = importlib.util.module_from_spec(spec)
|
||||
spec.loader.exec_module(module)
|
||||
return module
|
||||
|
||||
@staticmethod
|
||||
def _config(manager_class: str, backend_config: dict | None = None, enabled: bool = True):
|
||||
from types import SimpleNamespace
|
||||
|
||||
return SimpleNamespace(
|
||||
memory=SimpleNamespace(
|
||||
enabled=enabled,
|
||||
manager_class=manager_class,
|
||||
backend_config=backend_config or {},
|
||||
)
|
||||
)
|
||||
|
||||
def test_stale_deermem_field_fails_and_names_the_key(self, doctor):
|
||||
result = doctor._check_memory_backend_config(self._config("openviking", {"debounce_seconds": 30}))
|
||||
|
||||
assert result.status == "fail"
|
||||
assert "debounce_seconds" in result.detail
|
||||
assert result.fix
|
||||
|
||||
def test_valid_config_passes(self, doctor, monkeypatch):
|
||||
monkeypatch.setenv("OPENVIKING_API_KEY", "test-key")
|
||||
config = self._config("openviking", {"base_url": "http://127.0.0.1:1933", "owner_user_id": "user-1"})
|
||||
|
||||
assert doctor._check_memory_backend_config(config).status == "ok"
|
||||
|
||||
def test_other_backends_are_skipped(self, doctor):
|
||||
result = doctor._check_memory_backend_config(self._config("deermem", {"debounce_seconds": 30}))
|
||||
|
||||
assert result.status == "skip"
|
||||
|
||||
def test_disabled_memory_is_skipped(self, doctor):
|
||||
result = doctor._check_memory_backend_config(self._config("openviking", {"debounce_seconds": 30}, enabled=False))
|
||||
|
||||
assert result.status == "skip"
|
||||
|
||||
def test_custom_openviking_subclass_path_is_skipped(self, doctor):
|
||||
# A user-defined subclass owns its backend_config shape; the built-in
|
||||
# validator must not reject it.
|
||||
result = doctor._check_memory_backend_config(self._config("myapp.memory.CustomOpenVikingManager", {"anything": True}))
|
||||
|
||||
assert result.status == "skip"
|
||||
|
||||
def test_dotted_manager_path_is_validated(self, doctor):
|
||||
result = doctor._check_memory_backend_config(
|
||||
self._config(
|
||||
"deerflow.agents.memory.backends.openviking.openviking_manager.OpenVikingMemoryManager",
|
||||
{"debounce_seconds": 30},
|
||||
)
|
||||
)
|
||||
|
||||
assert result.status == "fail"
|
||||
|
||||
def test_missing_api_key_advises_dotenv(self, doctor, monkeypatch):
|
||||
monkeypatch.delenv("OPENVIKING_API_KEY", raising=False)
|
||||
config = self._config("openviking", {"base_url": "http://127.0.0.1:1933", "owner_user_id": "user-1"})
|
||||
|
||||
result = doctor._check_memory_backend_config(config)
|
||||
|
||||
assert result.status == "fail"
|
||||
assert "USER API key is missing" in result.detail
|
||||
assert result.fix == "Add OPENVIKING_API_KEY=<your-key> to your .env file"
|
||||
|
||||
|
||||
def test_memory_backend_config_failure_is_reported_end_to_end(doctor_checkout: Path):
|
||||
(doctor_checkout / "config.yaml").write_text(
|
||||
"config_version: 5\nmodels: []\nsandbox:\n use: deerflow.sandbox.local:LocalSandboxProvider\nmemory:\n enabled: true\n manager_class: openviking\n backend_config:\n debounce_seconds: 30\n",
|
||||
encoding="utf-8",
|
||||
)
|
||||
|
||||
# This check needs the harness importable in the subprocess, so unlike the
|
||||
# disposable-checkout runs above, point PYTHONPATH at the repo's backend
|
||||
# packages (the harness is path-installed, not pip-installed).
|
||||
repo_packages = REPO_ROOT / "backend" / "packages"
|
||||
separator = ";" if os.name == "nt" else ":"
|
||||
env = {name: value for name, value in os.environ.items() if not name.startswith("DEER_FLOW_") and name not in {"PYTHONHOME"}}
|
||||
env["PYTHONPATH"] = separator.join([str(repo_packages / "harness"), str(repo_packages / "extension-api"), str(REPO_ROOT / "backend")])
|
||||
result = subprocess.run(
|
||||
[sys.executable, str(doctor_checkout / "scripts" / "doctor.py")],
|
||||
cwd=doctor_checkout,
|
||||
env=env,
|
||||
capture_output=True,
|
||||
text=True,
|
||||
encoding="utf-8",
|
||||
errors="replace",
|
||||
check=False,
|
||||
timeout=60,
|
||||
)
|
||||
|
||||
assert result.returncode == 1, result.stderr
|
||||
assert "memory backend config (openviking)" in result.stdout
|
||||
assert "Unknown OpenViking backend_config fields: debounce_seconds" in result.stdout
|
||||
assert "Remove the fields named in the error" in result.stdout
|
||||
|
||||
@ -413,6 +413,90 @@ def check_config_loadable(config_path: Path) -> CheckResult:
|
||||
)
|
||||
|
||||
|
||||
def check_memory_backend_config(config_path: Path) -> CheckResult:
|
||||
"""Statically validate the selected memory backend's backend_config.
|
||||
|
||||
A stale backend_config (DeerMem-only keys left in place after switching
|
||||
manager_class to openviking) used to surface only when MemoryMiddleware
|
||||
constructed the manager mid-conversation, after model tokens had already
|
||||
been spent. from_backend_config is side-effect-free, so the same errors
|
||||
can be reported here before an agent turn is accepted.
|
||||
"""
|
||||
if not config_path.exists():
|
||||
return CheckResult("memory backend config", "skip")
|
||||
|
||||
try:
|
||||
config = _load_app_config(config_path)
|
||||
except Exception:
|
||||
# check_config_loadable already reports load failures.
|
||||
return CheckResult("memory backend config", "skip")
|
||||
|
||||
return _check_memory_backend_config(config)
|
||||
|
||||
|
||||
def _check_memory_backend_config(config: object) -> CheckResult:
|
||||
memory = getattr(config, "memory", None)
|
||||
manager_class = str(getattr(memory, "manager_class", "") or "").strip()
|
||||
label = f"memory backend config ({manager_class or 'unspecified'})"
|
||||
if memory is None or not getattr(memory, "enabled", True):
|
||||
return CheckResult(label, "skip", "memory disabled")
|
||||
|
||||
backend_config = getattr(memory, "backend_config", None) or {}
|
||||
if not _resolves_to_builtin_openviking(manager_class):
|
||||
return CheckResult(label, "skip", "no static validator for this backend")
|
||||
|
||||
try:
|
||||
from deerflow.agents.memory.backends.openviking.config import OpenVikingConfig
|
||||
except Exception as exc:
|
||||
# Importing the backend package executes openviking/__init__.py, which
|
||||
# pulls pydantic and deerflow internals; a broken backend environment
|
||||
# degrades to a skip instead of crashing the report (same policy as
|
||||
# resolve_config_path).
|
||||
return CheckResult(label, "skip", str(exc))
|
||||
|
||||
try:
|
||||
OpenVikingConfig.from_backend_config(dict(backend_config))
|
||||
except Exception as exc:
|
||||
detail = str(exc).strip().splitlines()[0] if str(exc).strip() else repr(exc)
|
||||
return CheckResult(label, "fail", detail, fix=_memory_backend_config_fix(detail))
|
||||
return CheckResult(label, "ok")
|
||||
|
||||
|
||||
def _resolves_to_builtin_openviking(manager_class: str) -> bool:
|
||||
"""True when manager_class resolves to the built-in OpenViking manager.
|
||||
|
||||
Only the built-in backend's config contract is known here: a custom
|
||||
``OpenVikingMemoryManager`` subclass pointed at by a dotted path owns its
|
||||
backend_config shape, so doctor must not reject it with the built-in
|
||||
rules. Resolution failures are the factory's to report (it fails loudly
|
||||
in get_memory_manager), not this check's.
|
||||
"""
|
||||
try:
|
||||
from deerflow.agents.memory.backends.openviking.openviking_manager import OpenVikingMemoryManager
|
||||
from deerflow.agents.memory.manager import _resolve_manager_class
|
||||
except Exception:
|
||||
return False
|
||||
try:
|
||||
return _resolve_manager_class(manager_class) is OpenVikingMemoryManager
|
||||
except Exception:
|
||||
return False
|
||||
|
||||
|
||||
def _memory_backend_config_fix(detail: str) -> str | None:
|
||||
"""Remediation matched to the validator's failure modes; None lets the
|
||||
verbatim error speak for itself."""
|
||||
if "USER API key is missing; set" in detail:
|
||||
var = detail.rsplit("set ", 1)[-1].strip().rstrip(".")
|
||||
return f"Add {var}=<your-key> to your .env file"
|
||||
if "Unknown OpenViking backend_config fields" in detail:
|
||||
return "Remove the fields named in the error from memory.backend_config, or switch manager_class back"
|
||||
if "trusted mode is no longer supported" in detail:
|
||||
return "Use a USER API key (api_key_env) and configure owner_user_id instead of auth_mode/account"
|
||||
if "custom HTTP client fields are no longer supported" in detail:
|
||||
return "Remove the custom HTTP client fields from memory.backend_config"
|
||||
return None
|
||||
|
||||
|
||||
def check_llm_api_key(config_path: Path) -> list[CheckResult]:
|
||||
"""Check that each model's env var is set in the environment."""
|
||||
if not config_path.exists():
|
||||
@ -923,6 +1007,7 @@ def main() -> int:
|
||||
config_failure or check_config_exists(config_path),
|
||||
check_config_version(config_path, project_root),
|
||||
check_config_loadable(config_path),
|
||||
check_memory_backend_config(config_path),
|
||||
check_models_configured(config_path),
|
||||
]
|
||||
sections.append(("Configuration", cfg_checks))
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user