fix(agent-framework): report dropped memory injections instead of failing silently - #1416
Open
inahus99 wants to merge 2 commits into
Open
Conversation
`_inject_memories` did not work against Agent Framework `Message` objects, in two separate ways. 1. `Message.text` is a read-only property derived from `Message.contents`, so assigning to it raises `AttributeError`. Every request that found an existing system message hit this path, and `_inject_memories` is called from `process()` without a guard, so the exception propagated and failed the whole chat call. Memories are now appended as an extra text content item, and the dict branch is checked first so plain-dict messages keep working. 2. When no system message was present, the prepended message carried the raw memories instead of the `wrap_memory_injection` output. That is the fence which marks retrieved memories as data and tells the model not to follow instructions inside them, so untrusted memory content reached the model unfenced. This path is common, since it covers any agent built without instructions. Both branches now inject the same wrapped text. Adds coverage for `_inject_memories`, which previously had none.
…ling silently `_inject_memories` had two paths that discarded memories without any signal, so a misbehaving context looked identical to one with no memories to inject. The fallback branch only prepended a system message when `context.messages` was a `list`, and silently did nothing otherwise. It also wrapped the insert in `except Exception: pass`, commented "log a warning" but with nothing to log through, since the function had no logger in scope. The function now takes the middleware's logger and warns on both paths: one for a non-list container, including the type it actually got, and one for an insert that raised, including the exception type. Neither raises, since injection is best-effort and must not fail the chat request. `_inject_memories` is private with a single call site, so the logger is a required argument rather than an optional one that could reintroduce the silent path.
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.
What and why
Follow-up to the two issues noted at the end of #1415.
_inject_memorieshad two paths that discarded memories with no signal at all, so a misbehaving context was indistinguishable from one that simply had no memories to inject.1. A non-
listcontainer was skipped silently. Theisinstancecheck sits inside thetrywith noelse, so any other sequence type fell straight through and the memories were dropped.2. A failing
insertwas swallowed. The comment says "log a warning", but the function had no logger in scope, so there was nothing to log through and the body was left aspass.Demonstrating both against the current code:
In both cases the model is silently answering without the memories it was supposed to have, and there is no way to tell from the logs — with
verbose=Truethe middleware still reports "Memory content preview" just before, so the logs positively suggest injection succeeded.The change
_inject_memoriesnow takes the middleware's logger and warns on each path — one for a non-list container, carrying the type actually received, and one for an insert that raised, carrying the exception type. Neither re-raises: injection is best-effort and must not fail the chat request. That intent is now stated in the docstring, since it is the reason the exception is caught at all.The logger is a required argument rather than optional. The function is private with exactly one call site, so there is no compatibility reason to default it, and a default of
Nonewould reintroduce the silent path this PR removes.Tests
Three tests added: a non-list container warns with
messages_type, a list whoseinsertraises warns with the exception type and leaves the messages untouched, and the normal existing-system-message path warns about nothing.Because the fix changes the signature, these cannot run against the old function — the pre-fix behaviour is the manual reproduction above. Full suite: 58 passing before, 61 after.
Verified against
agent-framework-core1.13.0:mypyclean,blackandflake8report nothing on the changed lines.No breaking changes;
_inject_memoriesis private and its only caller is updated here.