feat(memory): add Agent memory_manager param with configurable sync auto-flush#2795
Conversation
…uto-flush Wire a memory_manager param on Agent, mirroring the TS constructor: it accepts a MemoryManager instance or a MemoryManagerConfig (auto-wrapped), registers it as a plugin, and exposes it as agent.memory_manager. The synchronous Agent(...) entry point awaits MemoryManager.flush() after each invocation so background extraction persists across its per-invocation event loop. This is gated by flush_on_invocation_end (default True); set it False to opt out. The async invoke_async path never auto-flushes, so it does not affect trigger cadence.
Expose flush_on_invocation_end as a plain public attribute instead of a property, and tighten the Agent.memory_manager docstring.
|
Assessment: Comment Clean, well-tested change that gives Review Categories
Nice work porting the TS test suite case-for-case — the async/sync/opt-out matrix is well covered and all 81 affected tests pass locally. |
- Show the memory_manager= param (not plugins=) in the class docstring example - Raise a clear error when a MemoryManager is passed via both memory_manager= and plugins= instead of a raw 'already registered' ValueError
…istry change Keep the collision guard inline in Agent rather than adding a __contains__ to the shared plugin registry, which is out of scope for this PR.
… error - Import MemoryManager/MemoryManagerConfig directly (no circular dep, since memory imports Agent only under TYPE_CHECKING); drop the inline import - Reword the duplicate-registration error to point users at the memory_manager parameter rather than endorsing the plugins path
|
Assessment: Comment (re-review on Re-reviewed after the latest push. All three points from my initial review are resolved, and 82 affected tests pass locally. Resolved since first review
Open item — maintainer design discussion
Implementation is solid and the sync-only flush boundary is well-reasoned and well-tested; the remaining question is purely about API surface, not correctness. |
…nditional Per review, drop the flush_on_invocation_end flag. With a True default it only risked confusing users, and cadence control is already available via the async invoke_async path. The synchronous Agent(...) entry point now always flushes pending extraction after each invocation.
|
Assessment: Approve (re-review on The design discussion landed well — Verification on this commit
One open process note
Clean, well-tested change with a sensible sync-only flush boundary. Nice work iterating on the feedback. |
It does not use self, matching the sibling _resolve_context_manager.
|
Assessment: Approve (re-review on All review threads are now resolved. Verified the two new points from @opieter-aws on this commit: Verification on this commit
One open process note (unchanged)
Implementation is solid, the sync-only flush boundary is well-reasoned, and the iteration on feedback has been thorough. LGTM. |
Description
Adds a dedicated
memory_managerparameter toAgent, mirroring the TypeScript SDK'sAgentconstructor, and makes cross-session memory persistence work out of the box for the synchronous entry point.Previously a
MemoryManagercould only be attached viaplugins=[...], and persisting background extraction across the synchronousAgent(...)entry point required the caller to opt in (the originalflush_on_invocation_enddefaulted toFalse) or to callflush()manually. That left a quiet footgun: each synchronous invocation runs in its own event loop (run_async→asyncio.run), which cancels in-flight background saves on close, so the last turn's extraction was typically lost unless the user knew to enable the flag.This change gives
Agenta first-classmemory_managerparam (matchingconversation_manager/session_manager) and flushes pending extraction automatically — but only on the synchronous path, so it never forces a save every turn on the async path or interferes withIntervalTriggercadence.Public API Changes
Agentaccepts amemory_manager(aMemoryManagerinstance or aMemoryManagerConfig, auto-wrapped) and exposes it asagent.memory_manager:flush_on_invocation_end(onMemoryManager/MemoryManagerConfig) has been removed. The synchronousAgent(...)entry point now always awaits pending extraction after each invocation; the asyncinvoke_asyncpath never auto-flushes, so callers driving their own loop continue toawait memory_manager.flush()at a shutdown boundary.Auto-flush covers the
Agent(...)entry point only. The deprecatedstructured_output()sync path is not wired (it doesn't append messages, so extraction records nothing to flush there).Breaking Changes
The
flush_on_invocation_endparameter that shipped in the initial memory port has been removed (same unreleased development cycle). Persistence across the synchronous entry point is now automatic and unconditional, so there is no flag to migrate. Callers needing extraction-cadence control should useinvoke_asyncandflush()directly.Testing
How have you tested the change? Verify that the changes do not break functionality or introduce new warnings.
Ported the three TS test files (
memory-manager,extraction,model-extractor) as example-basedpytestsuites so coverage mirrors the TS suite case-for-case; runs green undertests/strands/memory. Adjacentplugins/toolssuites were run to confirm no regressions in plugin/tool discovery.hatch run prepareChecklist