Make Foundry MemoryProvider store request/response message filters configurable - #680
Conversation
efdb5bb to
82c763e
Compare
This comment has been minimized.
This comment has been minimized.
Add StoreInputRequestFilter and StoreInputResponseFilter to MemoryProviderConfig and wire them into the underlying ContextProviderConfig. Previously the request store filter was hardcoded to ExternalOnly and the response store filter was left unset, so callers could not customize which request or response messages are persisted as memories. Defaults are preserved to match .NET/Python parity: request messages default to ExternalOnly and response messages default to PassThrough.
82c763e to
dded81a
Compare
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
parity-approved
There was a problem hiding this comment.
🟢 Ready to approve
The changes are small, preserve existing defaults, correctly wire through existing filtering hooks, and include targeted tests for both customization and default behavior.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR extends the Foundry MemoryProviderConfig to allow callers to configure which request and response messages are persisted as memories, aligning Foundry provider behavior with the already-supported filtering capabilities in agent.ContextProviderConfig.
Changes:
- Add configurable
StoreInputRequestFilterandStoreInputResponseFilterfields toMemoryProviderConfig, with defaultsExternalOnlyandPassThrough. - Wire the new filters into
agent.ContextProviderConfig(StoreInputRequestMessageFilter/StoreInputResponseMessageFilter) innewMemoryProvider. - Add unit tests verifying both custom filter invocation and default filter behavior.
File summaries
| File | Description |
|---|---|
| provider/foundryprovider/memory.go | Adds store filter config fields, sets defaults, and passes them into the underlying context provider config. |
| provider/foundryprovider/memory_test.go | Adds tests ensuring custom store filters are called and default request/response store filtering matches expected behavior. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
This comment has been minimized.
This comment has been minimized.
| if config.StoreInputRequestFilter == nil { | ||
| config.StoreInputRequestFilter = messagefilter.ExternalOnly | ||
| } | ||
| if config.StoreInputResponseFilter == nil { | ||
| config.StoreInputResponseFilter = messagefilter.PassThrough | ||
| } |
There was a problem hiding this comment.
These are already the ContextProviderConfig defaaults, no? No need to set them when nil.
github-actions
Bot
added
area:provider
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
failed-auto-risk
…Config Per review: ContextProviderConfig already defaults a nil StoreInput request filter to messagefilter.ExternalOnly and a nil response filter to messagefilter.PassThrough in Invoked, so defaulting them again in newMemoryProvider was redundant. Leave them nil when unset and let the context provider apply the defaults. Behavior is unchanged (the default-behavior test still passes). Signed-off-by: PratikDhanave <i.pratikdhanave@gmail.com>
|
Good catch, thanks Quim Muntal (@qmuntal) — you're right. |
github-actions
Bot
added
pending-auto-risk
Go API Consistency Review — PR #680Scope: In scope — this PR adds two new exported fields to What changed
Semantic / behavior parity ✅The defaults and semantics match the .NET implementation exactly:
Naming parity
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by Go API Consistency Review Agent · sonnet46 · 32.5 AIC · ⌖ 11.2 AIC · ⊞ 6K
| // memories. The default is [messagefilter.ExternalOnly]. | ||
| StoreInputRequestFilter messagefilter.Filter | ||
|
|
||
| // StoreInputResponseFilter filters response messages before they are stored as |
There was a problem hiding this comment.
Cross-SDK naming parity note: The upstream .NET FoundryMemoryProviderOptions uses StorageInputRequestMessageFilter and StorageInputResponseMessageFilter (with Storage prefix and Message suffix), while these Go fields are named StoreInputRequestFilter / StoreInputResponseFilter (with Store prefix, no Message suffix).
The PR description calls this out as an open design question. It is worth resolving intentionally:
- If the shorter Go names are preferred for idiom reasons, record that as a deliberate decision (e.g., in a changelog or inline comment) so future maintainers understand the divergence.
- If alignment with .NET is preferred, renaming to
StorageInputRequestMessageFilter/StorageInputResponseMessageFilterwould make cross-SDK documentation and porting easier.
Upstream reference: dotnet/src/Microsoft.Agents.AI.Foundry/Memory/FoundryMemoryProviderOptions.cs
github-actions
Bot
added
failed-auto-risk
What
Adds two fields to
MemoryProviderConfiginprovider/foundryprovider/memory.go:StoreInputRequestFilter messagefilter.Filter— filters request messages before they are stored as memories (defaultExternalOnly).StoreInputResponseFilter messagefilter.Filter— filters response messages before they are stored as memories (defaultPassThrough).newMemoryProvidernow defaults these toExternalOnly/PassThroughwhen nil and passes both into the underlyingagent.ContextProviderConfig(StoreInputRequestMessageFilter/StoreInputResponseMessageFilter).Why
Previously the request store filter was hardcoded to
ExternalOnlyand the response store filter was left unset, so callers had no way to customize which request or response messages get persisted as memories — even though the baseagent/context.goalready supports all three store/provide filters.The chosen defaults preserve current behavior and match the .NET / Python SDKs, where an unset request filter defaults to external-only and an unset response filter defaults to pass-through. This closes a missing-configurability parity gap without changing observable defaults.
Tests
provider/foundryprovider/memory_test.go:TestNewMemoryProviderUsesCustomStoreFilters— supplies custom request and response store filters and asserts both are invoked duringInvoked(mirrors the existingTestNewMemoryProviderUsesCustomSearchInputFilter).TestNewMemoryProviderStoreFiltersDefaultToExternalOnlyRequestAndPassThroughResponse— with no filters set, a non-external request message is dropped (defaultExternalOnly) while a non-external response message is kept (defaultPassThrough), verified through the recorded update request.go build ./...,go vet ./provider/foundryprovider/..., andgo test ./provider/foundryprovider/...all pass.Open design questions
ProvideInputMessageFilteris already exposed viaSearchInputFilter, so no rename is proposed here.StoreInputRequestFilter/StoreInputResponseFiltermirrorSearchInputFilterin this config rather than the longer...MessageFilternames used on the baseContextProviderConfig. Happy to align to either convention.