fix(honcho): preserve memory when backend rejects conclusions - #26484
fix(honcho): preserve memory when backend rejects conclusions#26484atmigtnca wants to merge 1 commit into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating two real Honcho reliability concerns. The conclusion fallback remains relevant, but this branch needs targeted salvage against current main.
Problems
- The multimodal portion is already handled before every provider sync:
run_agent.py:3396-3407flattens both payloads, andtests/run_agent/test_memory_sync_interrupted.py:245-268covers an image-bearing turn. The new plugin-local formatter would duplicate that behavior with different markers. plugins/memory/honcho/session.py:1123in this diff enters the peer-card fallback for everyconclusions_scope.create()exception. The PR describes a missing backend LLM credential specifically; an unknown or ambiguous write failure should not be converted into a successful durable write.- The new test's ordered
_get_or_create_peerside effect asserts writes touser_peer, but current card writes use the shared observer/target contract inplugins/memory/honcho/session.py:1247-1256.
Suggested changes
- Keep the conclusion fallback only; use
get_peer_card()/set_peer_card()and a peer-ID-aware test asserting the assistant observer and user target. - Limit fallback to the verified unsupported conclusion/credential rejection mode, and retain failure for other exceptions.
Automated hermes-sweeper review.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| def _content_to_text(content: Any) -> str: |
There was a problem hiding this comment.
Current main already normalizes normal completed turns before any provider receives them (run_agent.py:3396-3407), with a shared regression test. Please drop this plugin-local formatter and its duplicate test rather than maintain a second, differently formatted multimodal boundary.
| return False | ||
| # Self-hosted Honcho deployments may store/search raw messages but | ||
| # reject conclusion writes when the backend LLM/OpenAI key is not | ||
| # configured. In that case, preserve the user's explicit durable |
There was a problem hiding this comment.
This fallback runs for every conclusion-create exception, including transport and ambiguous post-write failures, but the stated use case is the known missing-backend-LLM-credential rejection. Please gate the fallback to that confirmed failure mode; otherwise return False so an uncertain conclusion is not reported as saved.
| mgr._get_or_create_peer = MagicMock(side_effect=[assistant_peer, user_peer, user_peer]) | ||
|
|
||
| ok = mgr.create_conclusion(session.key, "User prefers Korean casual tone") | ||
|
|
There was a problem hiding this comment.
The ordered mock returns user_peer for the fallback lookup/write even though this mode uses the assistant as observer. Use a peer-ID-aware side effect and assert assistant_peer.get_card(target=user_peer_id) plus assistant_peer.set_card(..., target=user_peer_id) to match the existing observer-target write contract.
What does this PR do?
Fixes two Honcho memory reliability failures seen in normal Hermes sessions:
sync_turnnow normalizes structured/multimodal message content before callingsanitize_context, so image/audio/list-style provider payloads do not raiseTypeError: expected string or bytes-like object, got 'list'and drop the whole turn.honcho_concludenow preserves explicit durable facts in the target peer card when a self-hosted Honcho backend can store/search raw messages but rejects conclusion creation because backend LLM credentials are missing.The fallback keeps the same observer/target relationship used by the normal conclusion path and deduplicates the fact before appending it to the peer card.
Related Issue
No linked GitHub issue.
Related open PRs found while checking for duplicates:
sync_turncrash only.Type of Change
Changes Made
plugins/memory/honcho/__init__.py_content_to_text()to coerce strings, dicts, and OpenAI-style content lists into safe text before Honcho ingest.[image attachment]and[audio attachment].plugins/memory/honcho/session.pytests/honcho_plugin/test_session.pysync_turnnormalization.How to Test
python -m pytest tests/honcho_plugin/test_session.py -q -o 'addopts='git diff --checkpython -m compileall -q plugins/memory/honcho tests/honcho_pluginpython -m ruff check plugins/memory/honcho/__init__.py plugins/memory/honcho/session.py tests/honcho_plugin/test_session.pyFull-suite note: I also attempted
scripts/run_tests.shfor CI parity. It reached 93% before the 600s local command timeout and had unrelated failures outside this Honcho change area, so this PR reports the focused checks above rather than claiming a full-suite pass.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs