Fix #2448: prepare_reference_data crashes on dict entries with missing or non-string id - #2451
Open
Memtensor-AI wants to merge 2 commits into
Open
Memtensor-AI wants to merge 2 commits into
Memtensor-AI wants to merge 2 commits into
Conversation
…non-string id (MemTensor#2448) The dict branch of prepare_reference_data used to assume every entry carried a string "id" key, so cached search results / MCP payloads that had already been serialized would raise KeyError('id') or AttributeError on .split('-'). Both errors escaped the streaming pipeline. Normalize dict entries instead: create the metadata dict if missing, skip ref_id derivation when id is absent (keep id slot as None), and coerce non-string ids (int, uuid.UUID, ...) with str() before the prefix split. The original id value is preserved in metadata["id"] unchanged so downstream consumers can still round-trip it. Regression tests in tests/mem_os/utils/test_reference_utils.py cover both crash repros from the issue plus the TextualMemoryItem branch (baseline).
Memtensor-AI
added
ai:generated
Collaborator
Author
🤖 Open Code ReviewTarget: PR #2451 ✅ OpenCodeReview: Review complete: 0 finding(s) across 2 selected item(s). Generated by cloud-assistant via Open Code Review. |
Collaborator
Author
🔧 Open Code Review requested Agent fixOpen Code Review found 2 issue(s). I have resumed the development Agent to fix them.
The Agent will push a new commit to this PR branch. OCR will recheck after the commit is pushed. |
…#2448) OCR findings resolved: 1. src/memos/mem_os/utils/reference_utils.py: the dict-entry branch aliased ``memories_json = memories`` and then wrote ``ref_id``, ``embedding``, ``sources``, ``memory`` and ``id`` into the caller's ``metadata`` dict in place. Cached payloads (search cache, MCP messages reused across calls) would be silently corrupted. Now we shallow-copy the outer entry and the metadata dict before mutating. 2. tests/mem_os/utils/test_reference_utils.py: the class docstring documented ``test_pre_fix_would_have_raised`` as ``xfail(strict=False)`` but the decorator was missing, so the test ran as a plain passing test and the "pre-fix demonstration" gating story was silently lost. The decorator is now applied and the docstring rewritten to reflect the actual XPASS/XFAIL flip semantics. Also adds ``test_caller_dict_is_not_mutated`` as a regression guard for finding 1 (the pre-existing tests could not catch the aliasing bug). Verification: - pytest tests/mem_os/utils/test_reference_utils.py -v 8 passed, 2 xpassed (documented XPASS from the pre-fix demo cases) - ruff check src/memos/mem_os/utils/reference_utils.py tests/mem_os/utils/test_reference_utils.py All checks passed
Memtensor-AI
added
status:ready
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fix #2448: prepare_reference_data no longer crashes on dict memory entries with a missing or non-string
id.Root cause: the dict branch of
prepare_reference_datainsrc/memos/mem_os/utils/reference_utils.pyassumed every entry carried anidkey and that the id was a string, so cached search results / MCP payloads that had already been serialized would raiseKeyError('id')orAttributeError: 'int' object has no attribute 'split', and the exception escaped the streaming pipeline. TheTextualMemoryItembranch was unaffected because pydantic guarantees the invariant there.Fix: normalize dict entries defensively — auto-create
metadataif missing, skip ref_id derivation whenidis absent (keeping the id slot explicit asNone), and coerce non-string ids (int, uuid.UUID, ...) withstr()before the prefix split. The original id value is preserved inmetadata["id"]unchanged so downstream consumers can still round-trip it. TheTextualMemoryItembranch is untouched.Tests: added 9 regression cases in
tests/mem_os/utils/test_reference_utils.pycovering both crash repros from the issue, string / UUID / int id shapes, missing metadata, missing memory, and the TextualMemoryItem baseline. All 9 pass; the widertests/mem_os/suite (45 tests) also passes with no regressions.ruff formatandruff checkare clean on the touched files. Local commit created and branch pushed to origin asbugfix/autodev-2448-20260930155922241(commit22d8e980).Related Issue (Required): Fixes #2448
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Automated tests are pending.
Checklist
@WeiminLee please review this PR.
Reviewer Checklist