fix(memory_manager): sanitize_context handles multimodal list content - #68072
fix(memory_manager): sanitize_context handles multimodal list content#68072mgcstudios86 wants to merge 2 commits into
Conversation
…tent The sanitize_context() function used re.sub which expects a string. When called with multimodal content (a list of typed parts), it raised TypeError: expected string or bytes-like object, got 'list'. This bug killed long-running QA workers after ~89 API calls when the memory provider passed back multimodal content. The fix: detect non-string input and flatten via _summarize_user_message_for_log before regex sanitization. All 10 tests in TestFlattenMessageContent pass.
Related: merged #44738 normalizes multimodal content at the external-memory sync boundary and covers the reported provider-sync path. This patch adds a defensive guard directly in sanitize_context(). |
Bryntly
left a comment
There was a problem hiding this comment.
LGTM. The logic correctly handles multimodal list contents avoiding a regex TypeError crash.
A couple of suggestions for improvement:
- Since type hints are used throughout the file, changing
text: strto justtextloses typing information. Consider usingtext: Anyortext: str | list(sinceAnyis already imported). - The nested
except Exception:blocks are quite broad and silently swallow errors (likeImportErroror potential bugs in_summarize_user_message_for_log). It might be better to catch specific exceptions or log a warning when falling back tostr(text). - Consider adding a quick unit test for
sanitize_contextintests/agent/test_memory_provider.pyto verify it correctly processes a list input to prevent future regressions.
Thanks for fixing this!
Per Bryntly's review (PR NousResearch#68072): - Use `Any` type hint instead of bare `text` (preserves type info) - Replace broad `except Exception` with `logger.warning(...)` so failures are observable instead of silently swallowed - Add 8 unit tests covering: string passthrough, empty, None, list of text parts, list with fence tags, list with image, scalar fallback, and a direct guard against the original TypeError-on-list regression Per alt-glitch comment (PR NousResearch#68072): - Docstring now references PR NousResearch#44738 (boundary normalization already merged in upstream) and frames this PR as a defensive guard for callers that forget the boundary normalization
|
Thanks for the review @Bryntly — addressed all three suggestions. What changed1. Type hint — def sanitize_context(text: Any) -> str:2. Replaced broad
3. Added 8 unit tests (
All 109 tests in On @alt-glitch's noteGood catch on #44738 — the boundary normalization there is the right primary fix. Updated the docstring to call this out: If you'd prefer, I can:
Happy to go with whichever you prefer. |
|
Thanks for adding defensive coverage around multimodal content. Problems
Suggested changes
Automated hermes-sweeper review. |
Bug
sanitize_context(text)inagent/memory_manager.pydoestext = re.sub(...)which expects a string.When called with multimodal content (a list of typed parts, e.g.
[{type: text, text: ...}, {type: image_url, ...}]), it raises:This bug kills long-running QA workers after ~89 API calls when the memory provider passes back multimodal content. The model itself is fine — the error is in the post-call handler.
Fix
Detect non-string input and flatten via
_summarize_user_message_for_logbefore regex sanitization. The test classTestFlattenMessageContentalready exists — the implementation just wasn't using it.Tests
All 10 tests in
TestFlattenMessageContentpass.Repro
Run a long-running agent with multimodal content + memory provider enabled. After ~89 API calls the worker dies with the exact error above.
Reported by
@matiasgonzalocalvo (mgcstudios user)