From f0276c9f5ae82e292b946457cdd4b2cf0c827a24 Mon Sep 17 00:00:00 2001 From: starslittle <110606805+starslittle@users.noreply.github.com> Date: Mon, 17 Aug 2026 08:22:11 +0800 Subject: [PATCH] fix(memory): validate Honcho timeout and character limits (#4783) * fix(memory): validate Honcho timeout and character limits * fix(memory): enforce HonchoConfig invariants --- .../harness/deerflow/agents/memory/AGENTS.md | 1 + .../deerflow/agents/memory/backends/README.md | 8 +-- .../agents/memory/backends/honcho/config.py | 13 ++++ backend/tests/test_honcho_memory_backend.py | 69 +++++++++++++++++++ 4 files changed, 87 insertions(+), 4 deletions(-) diff --git a/backend/packages/harness/deerflow/agents/memory/AGENTS.md b/backend/packages/harness/deerflow/agents/memory/AGENTS.md index d014508b5..53f546f91 100644 --- a/backend/packages/harness/deerflow/agents/memory/AGENTS.md +++ b/backend/packages/harness/deerflow/agents/memory/AGENTS.md @@ -53,6 +53,7 @@ query-aware refresh policy and new lifecycle scheduling are separate changes, not part of this backend. - The optional `honcho` backend under `packages/harness/deerflow/agents/memory/backends/honcho/` is a remote-only HTTP adapter for user-model memory (RFC #1898's user-dimension option). Select with `memory.manager_class: honcho`, keep `memory.mode: middleware` (tool mode also supported — it implements `search`). It writes filtered turns as Honcho messages (no local LLM calls; Honcho's deriver builds representations server-side), resolves one workspace per `user_id` (`workspace_overrides` else `workspace_prefix + collision-resistant sanitized id`; missing user fails closed to no memory), offloads sync HTTP in its `a*` overrides via `asyncio.to_thread`, and tool mode retains passive writes via MemoryMiddleware, mirroring mem0. `failure_policy.read: fail_closed` rethrows recall failures; default is log-and-empty. +- Honcho configuration objects reject non-finite or non-positive timeout values and non-positive character budgets during construction, including direct dataclass construction, before an HTTP client can use them. - `memory.mode: tool` skips `MemoryMiddleware` and registers `memory_search`, `memory_add`, `memory_update`, and `memory_delete` on the agent. The model decides when to search, add, update, or delete facts; this is opt-in/experimental and should not be described as better than middleware mode without eval evidence. - Both modes share `FileMemoryStorage`, per-user/per-agent isolation, manual CRUD primitives, and the updater backend. Injection is mode-aware: middleware mode injects global `user`/`history` summaries plus the selected agent's facts, while tool mode injects only the global summaries and leaves every agent fact behind `memory_search` to avoid duplicating automatically injected and retrieval-returned context. `memory.injection_enabled: false` suppresses the complete block in either mode. - Middleware extraction classifies proposed facts with extraction-only `scope`/`durability`/`authority` labels. `_apply_updates` accepts only `user` + `durable` + `descriptive` new/consolidated facts, accepts only wholly user-scoped summary prose with `authority=descriptive`, and rejects missing labels per item without aborting unrelated updates. Contradiction removals use object entries with `id`, `scope`, `reason`, and optional zero-based `replacementFactIndex`; task/project removals fail closed, and a paired removal runs only when the referenced replacement survives the scope/confidence gates, deduplication, and max-fact trim under another fact ID. The labels are not persisted, so no storage migration is required. Staleness removals retain their independent candidate/cap guardrails, while tool-mode CRUD remains outside this extraction gate. Custom `memory.backend_config.prompts_dir` templates (including per-agent overrides) must carry the same classification fields; an un-migrated template makes the fail-closed gate reject every extraction-driven write, observable only through `rejected_by_scope_gate` and the >60% fact-rejection warning. diff --git a/backend/packages/harness/deerflow/agents/memory/backends/README.md b/backend/packages/harness/deerflow/agents/memory/backends/README.md index e1aaa72fc..4d7b21048 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/README.md +++ b/backend/packages/harness/deerflow/agents/memory/backends/README.md @@ -139,14 +139,14 @@ The optional `honcho/` backend is a remote-only HTTP adapter for user-model memo | `base_url` | str | `http://localhost:8000` | Honcho instance URL (e.g., `http://localhost:8000` or `https://api.honcho.dev`) | | `api_key` | str | optional | API key for hosted Honcho; required if `base_url` is `api.honcho.dev`. Can use `$HONCHO_API_KEY` env var syntax. Requires `allow_insecure_http: true` when using plain HTTP | | `allow_insecure_http` | bool | false | Allow HTTP (non-HTTPS) connections; needed for localhost development with api_key | -| `timeout_seconds` | float | `10.0` | HTTP client timeout (seconds) for calls to Honcho — read/write/pool; see `connect_timeout_seconds` for the connect phase | -| `connect_timeout_seconds` | float | `3.0` | HTTP connect timeout (seconds) for establishing the connection to Honcho | +| `timeout_seconds` | float | `10.0` | HTTP client timeout (seconds) for calls to Honcho — read/write/pool; see `connect_timeout_seconds` for the connect phase. Must be finite and `> 0` | +| `connect_timeout_seconds` | float | `3.0` | HTTP connect timeout (seconds) for establishing the connection to Honcho. Must be finite and `> 0` | | `workspace_prefix` | str | `deerflow-u-` | Prefix for isolated workspaces; each user gets one workspace named `{prefix}{sanitized_id}` | | `workspace_overrides` | dict | `{}` | Map specific user ids to custom workspace names; overrides the prefix-based derivation. Values must be non-empty (parse error otherwise). Mapping several users to one workspace shares its search index across them (see Workspace Resolution) | | `user_peer_overrides` | dict | `{}` | Map specific user ids to custom names for the user's own peer; overrides the stable-id derivation. Values must be non-empty (parse error otherwise) | | `assistant_peer` | str | `deerflow` | Default peer name for the assistant when storing messages | -| `message_char_limit` | int | `8000` | Character limit per message; longer messages are truncated | -| `max_injection_chars` | int | `6000` | Character limit for injected memory into the system prompt | +| `message_char_limit` | int | `8000` | Character limit per message; longer messages are truncated. Must be `> 0` (zero empties the write; a negative value is a Python suffix slice, not a cap) | +| `max_injection_chars` | int | `6000` | Character limit for injected memory into the system prompt. Must be `> 0` | | `failure_policy.read` | str | `fail_open` | Recall failure handling: `fail_open` (log and return empty) or `fail_closed` (rethrow) | **Workspace Resolution**: Each DeerFlow user maps to one Honcho workspace. The workspace name is derived as: `workspace_overrides[user_id]` (if present) else `workspace_prefix + sanitized_id`, where `sanitized_id` is a collision-resistant hash suffix (sanitize[:48]-sha256[:8]). Missing user fails closed to no memory. The default derivation is isolated per user; a `workspace_overrides` entry that maps several users to one workspace deliberately shares that workspace's **search index** across them (`search` uses Honcho's workspace-scoped `/search`, which has no peer filter), while `get_context` / `get_memory` remain peer-scoped. 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 5bbc726de..b6e983e38 100644 --- a/backend/packages/harness/deerflow/agents/memory/backends/honcho/config.py +++ b/backend/packages/harness/deerflow/agents/memory/backends/honcho/config.py @@ -10,6 +10,7 @@ from __future__ import annotations import re from dataclasses import dataclass, field +from math import isfinite from typing import Any _ID_RE = re.compile(r"[^a-zA-Z0-9_-]+") @@ -49,6 +50,18 @@ class HonchoConfig: read_fail_closed: bool = False storage_path: str = "" + def __post_init__(self) -> None: + if not isfinite(self.timeout_seconds) or self.timeout_seconds <= 0: + raise ValueError("Honcho backend: timeout_seconds must be a finite value > 0") + if not isfinite(self.connect_timeout_seconds) or self.connect_timeout_seconds <= 0: + raise ValueError("Honcho backend: connect_timeout_seconds must be a finite value > 0") + # add()/get_context() truncate with text[:n]. n <= 0 is empty (n == 0) + # or a Python negative slice (n == -1 -> text[:-1]), not a length cap. + if self.message_char_limit <= 0: + raise ValueError("Honcho backend: message_char_limit must be > 0") + if self.max_injection_chars <= 0: + raise ValueError("Honcho backend: max_injection_chars must be > 0") + @classmethod def from_backend_config(cls, backend_config: dict[str, Any] | None) -> HonchoConfig: cfg = dict(backend_config or {}) diff --git a/backend/tests/test_honcho_memory_backend.py b/backend/tests/test_honcho_memory_backend.py index 0d21b4089..aa913e4eb 100644 --- a/backend/tests/test_honcho_memory_backend.py +++ b/backend/tests/test_honcho_memory_backend.py @@ -23,6 +23,8 @@ class TestHonchoConfig: assert cfg.workspace_overrides == {} assert cfg.user_peer_overrides == {} assert cfg.assistant_peer == "deerflow" + assert cfg.timeout_seconds == 10.0 + assert cfg.connect_timeout_seconds == 3.0 assert cfg.message_char_limit == 8000 assert cfg.max_injection_chars == 6000 assert cfg.read_fail_closed is False @@ -69,6 +71,58 @@ 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("timeout_seconds", 0, id="timeout-zero"), + pytest.param("timeout_seconds", -1, id="timeout-negative"), + pytest.param("timeout_seconds", float("nan"), id="timeout-nan"), + pytest.param("timeout_seconds", float("inf"), id="timeout-inf"), + pytest.param("timeout_seconds", float("-inf"), id="timeout-neg-inf"), + pytest.param("connect_timeout_seconds", 0, id="connect-timeout-zero"), + pytest.param("connect_timeout_seconds", -1, id="connect-timeout-negative"), + pytest.param("connect_timeout_seconds", float("nan"), id="connect-timeout-nan"), + pytest.param("connect_timeout_seconds", float("inf"), id="connect-timeout-inf"), + pytest.param("connect_timeout_seconds", float("-inf"), id="connect-timeout-neg-inf"), + ], + ) + def test_rejects_invalid_timeouts(self, key, value): + with pytest.raises(ValueError, match=key): + HonchoConfig.from_backend_config({key: value}) + + @pytest.mark.parametrize("key", ["message_char_limit", "max_injection_chars"]) + @pytest.mark.parametrize("value", [0, -1]) + def test_rejects_non_positive_character_limits(self, key, value): + with pytest.raises(ValueError, match=key): + HonchoConfig.from_backend_config({key: value}) + + @pytest.mark.parametrize( + ("key", "value"), + [ + pytest.param("timeout_seconds", float("inf"), id="timeout"), + pytest.param("connect_timeout_seconds", 0, id="connect-timeout"), + pytest.param("message_char_limit", -1, id="message-limit"), + pytest.param("max_injection_chars", 0, id="injection-limit"), + ], + ) + def test_direct_construction_enforces_limits(self, key, value): + with pytest.raises(ValueError, match=key): + HonchoConfig(**{key: value}) + + def test_accepts_custom_positive_timeouts_and_character_limits(self): + cfg = HonchoConfig.from_backend_config( + { + "timeout_seconds": 2.5, + "connect_timeout_seconds": 0.5, + "message_char_limit": 12, + "max_injection_chars": 8, + } + ) + assert cfg.timeout_seconds == 2.5 + assert cfg.connect_timeout_seconds == 0.5 + assert cfg.message_char_limit == 12 + assert cfg.max_injection_chars == 8 + class TestSanitizeId: def test_passthrough_and_cleanup(self): @@ -271,6 +325,21 @@ class TestHonchoManagerWrite: sent = [c for c in fake.calls if c[0] == "messages"][0][1][2][0][1] assert len(sent) == 10 + def test_from_config_rejects_negative_message_char_limit(self): + """add() uses ``text[:message_char_limit]``. A negative limit is a + Python negative slice (``text[:-1]``), which deletes a suffix instead + of capping length. Fail at config parse so the manager never sees it. + """ + with pytest.raises(ValueError, match="message_char_limit"): + HonchoMemoryManager.from_config({"base_url": "http://honcho.test", "message_char_limit": -1}) + + def test_from_config_rejects_zero_max_injection_chars(self): + """get_context() uses ``representation[:max_injection_chars]``. Zero + would inject an empty memory string; reject it at startup. + """ + with pytest.raises(ValueError, match="max_injection_chars"): + HonchoMemoryManager.from_config({"base_url": "http://honcho.test", "max_injection_chars": 0}) + def test_add_normalizes_list_content(self): mgr, fake = _manager() mgr.add("t-5", [_msg("human", [{"type": "text", "text": "part1"}, {"type": "text", "text": "part2"}])], user_id="u1")