Add AgentCore Memory Support for Short Term Memory - #14
Conversation
d785f7c to
4075851
Compare
Code ReviewPR #14: Add AgentCore Memory Support for Short Term Memory SummaryThis PR is well-scoped, well-tested, and ships a clean integration of AgentCore Memory as a ChatHistoryProvider for the Microsoft Agent Framework pipeline. The AsyncLocal-based session-context flow is elegant: users get session-scoped memory for free with a single Memory ID, and the provider degrades to a true no-op when none is configured. Test coverage is unusually thorough - 79 unit tests including 10 FsCheck property-based tests, plus 6 end-to-end memory recall integration tests across every sample app. Before merging, please confirm the chronological ordering of ListEvents (the loaded history feeds directly into the LLM), and address a few smaller correctness/maintainability points: the user-supplied ChatHistoryProvider is silently overwritten, FilterMessagesForStorage collapses all non-User roles to ASSISTANT (including System), some property tests are declared async void which can mask assertion failures, and the env-var-based unit tests can race under xunit parallelization. Important Issues - Should Fix
Minor Issues
Positive Notes
RecommendationRequest changes - please address the important issues listed above. |
Thanks for the thorough review. All important issues have been addressed: Important Issues1. User-supplied ChatHistoryProvider silently overwrittenFixed. Changed to 2. FilterMessagesForStorage maps every non-User role to ASSISTANTFixed. Added an explicit allow-list: only 3. Verify ListEvents returns events in chronological orderConfirmed. AgentCore Memory's ListEvents returns events in chronological order (oldest first). Added a comment in 4. Property-based tests declared as async voidAlready 5. Environment-variable-based tests are not parallel-safeFixed. Added 6. ActorId and SessionId both set to the same session IDIntentional — documented with inline comments. The AgentCore Runtime doesn't provide a separate user/actor identity header. The only identity available at request time is the session ID. Added comments in both Minor Issues
|
| catch (Exception ex) when (nextToken is not null) | ||
| { | ||
| // Partial pagination failure — return what we have | ||
| logger.LogWarning(ex, "Error fetching page during pagination. Returning {Count} messages loaded so far.", messages.Count); |
There was a problem hiding this comment.
Should there be an option whether to treat exceptions reading and writing the history as fatal or was there some precedence in the community you were following to say these should be treated as just log messages.
There was a problem hiding this comment.
The "log and continue" approach follows the same pattern used by Strands Agents SDK (Python) and LangChain's memory implementations — memory is treated as a non-essential enhancement, not a critical path dependency. The reasoning:
- An agent without memory is still useful — it just doesn't remember prior turns. A failed memory load shouldn't prevent the agent from answering the current question.
- Memory save failures are even less critical — the current response has already been generated and returned to the user. Failing to persist it means the next turn won't have that context, but the current request succeeds.
- The alternative (fatal) is worse for production — if the Memory service has a transient outage, every agent invocation would fail with a 500, even though the agent could still answer questions perfectly well without history.
That said, some users might want strict mode where memory failures are fatal (e.g., compliance scenarios where incomplete history is unacceptable). We could add a MemoryErrorBehavior option (LogAndContinue vs ThrowOnError) in a follow-up if there's demand. For now, the safe default is graceful degradation.
| IEnumerable<ChatMessage>? requestMessages, | ||
| IEnumerable<ChatMessage>? responseMessages) | ||
| { | ||
| var allMessages = (requestMessages ?? []).Concat(responseMessages ?? []); |
There was a problem hiding this comment.
Probably my ignorance here but won't some the requestMessages already be saved when they were loaded from the chat history or is requestMessages only the new messages brought in for the request?
There was a problem hiding this comment.
MAF handles this. The base InvokedCoreAsync implementation filters out messages that were produced by a ChatHistoryProvider (stamped with AgentRequestMessageSourceType.ChatHistory) before passing them to StoreChatHistoryAsync. So context.RequestMessages only contains new messages from the current turn, not previously loaded history.
Issue #, if available:
DOTNET-8640
Description of changes
Integrates Amazon Bedrock AgentCore Memory as a
ChatHistoryProviderto give agents persistent conversation history within a session. This is session-scoped short-term memory — the agent remembers what was said earlier in the same conversation across multiple request/response cycles, surviving container restarts and scaling events. This is not long-term/semantic memory (that's a separate future feature).What it does
When a Memory ID is configured (via
options.MemoryIdor theAWS_AGENTCORE_MEMORY_IDenvironment variable), the provider automatically:When no Memory ID is configured, the provider is a no-op — zero overhead, no API calls.
How session identity flows (AsyncLocal)
The memory provider needs the session ID (from the
X-Amzn-Bedrock-AgentCore-Runtime-Session-IdHTTP header) to scope memory operations. The challenge: the provider runs deep inside the MS Agent Framework pipeline and doesn't have access to the HTTP request.We solve this with an
AsyncLocal<AgentCoreRuntimeContext>onAgentCoreRuntimeContextProvider.Current. TheMapAgentCoreendpoint handlers set this value after extracting the runtime context from HTTP headers. SinceAsyncLocalflows through the async call chain, the memory provider can read it later duringRunAsyncwithout the user needing to pass anything manually. Each concurrent request gets its own isolated value — no cross-request leakage.This means users get memory for free — no session management code, no StateBag wiring, just set a Memory ID and it works.
Usage
Changes
AgentCoreMemoryProvider— newChatHistoryProviderimplementation (load via ListEvents, save via CreateEvent, pagination, error handling)AgentCoreRuntimeContextProvider— addedAsyncLocal<AgentCoreRuntimeContext>for ambient session contextAgentCoreBuilderExtensions— registersIAmazonBedrockAgentCoreandAgentCoreMemoryProviderin DIAgentCoreEndpointExtensions/ParameterBindingPlan— sets the AsyncLocal in all endpoint handlersAgentCoreOptions— addedMemoryIdpropertyConstants.cs— centralizedAWS_AGENTCORE_MEMORY_IDenv var nameAWS::BedrockAgentCore::Memoryresource and IAM permissions for integration testsWhat was tested
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.