fix(reflect): pair each expanded memory_id with its own memory - #2759
Merged
benfrank241 merged 1 commit intoJul 17, 2026
Merged
Conversation
tool_expand zipped memory_ids against valid_uuids, which only collects the ids that parsed as UUIDs. One invalid id shifts every later pair by one, so a memory comes back stamped with a different memory's id, and zip truncates the tail so the last requested id gets no entry at all. Key each id to its own UUID and iterate memory_ids directly, so an invalid id can only affect its own entry.
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
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.
Root cause
tool_expandvalidatesmemory_idsinto a second, shorter list and then zips the two back together:valid_uuidsis compacted, sovalid_uuids[i]lines up withmemory_ids[i]only while every preceding id parsed. One invalid id shifts every later pair by one, andzipthen truncates to the shorter list, dropping the tail.Two things follow, and the first is the one that worries me:
item["memory_id"]anditem["memory"]["id"]disagree inside the same response item, so nothing raises and nothing logs: the reflect agent is handed B's text under A's id and cites it as A.errorsbranch that exists to report a bad id silently does not fire for it.Invariant
tool_expandreturns one entry per requestedmemory_id, in request order, and each entry'smemorypayload is the row whose id equals that entry's ownmemory_id.Measured on HEAD (9676fc1)
Case 3 is the control: with every id valid the pairing is correct, which is why the compaction is the trigger rather than the lookup.
Reachability
_execute_tool(agent.py:1420-1425) readsargs.get("memory_ids", []), checks only that it is non-empty, and passes the raw model-supplied array toexpand_fn;tools_schema.py:121-125declaresmemory_idsas a free-formarray of string. So one hallucinated or truncated id among valid ones is enough, and theerrorsdict plus theif mid in errorsbranch already in the function are the existing acknowledgement that such input arrives.The fix
Key each id to its own UUID and iterate
memory_idsdirectly, so an invalid id can only affect its own entry. Nine lines added and six removed intools.py, no signature or API change.One incidental, measured rather than assumed:
valid_uuidsis now built from the dict's values, so a repeated id is sent toWHERE id = ANY($1)once instead of once per occurrence. The returned results are unchanged (a duplicate id still gets one entry per occurrence, verified both before and after).How I verified
Clean
python:3.12-slimcontainer, package installed non-editable, HEAD 9676fc1. Provenance was pinned by sha256 identity between the installed module and the checkout, both sides of the differential:6f05a294f95600e6ad80573b949b78b90b070f93e35fa564589a254b185684b482d80e5f771d4399c918768b752be037b8257f45030827bded716eca502b89c5The two regression tests fail on main and pass on the branch (run both ways in the container, against the printed sha256 above):
The tests use this file's existing fake-connection idiom, so they need no database and no LLM. The connection is the only thing faked, and it supplies rows; the pairing under test happens entirely inside
tool_expandand runs for real.ruff checkandruff format --checkpass on both changed files.ty check hindsight_apireports 128 diagnostics on my branch and the same 128 on main, none inreflect/tools.py.Each of the two behaviors named above has its own test:
test_tool_expand_pairs_each_memory_id_with_its_own_memory(case 1) andtest_tool_expand_reports_a_trailing_invalid_memory_id(case 2).What I did not verify
zip()call sites in the package for the same shape; this PR is onlytool_expand.git log -S 'zip(memory_ids'on an unshallowed clone returns exactly one commit, 4f28338 (#132), which introduced the tool. This is an original defect rather than a regression, and there is no earlier attempt or revert to answer.Disclosure: I am an AI agent (Claude) working on Ehsan Barkhordar's behalf. He is accountable for this PR. Every number above came from a run in the container described, not from inspection.