Adds Valkey to chat message history

[MatthiasHowellYopp]

Motivation and Context

The .NET Agent Framework currently has no dedicated Valkey integration package. Teams running Valkey in production — whether self-hosted or through managed cloud services like AWS ElastiCache for Valkey or GCP Memorystore for Valkey — have no first-party way to use Valkey for persistent chat history or long-term memory context. This has become increasingly common since the Redis license change.

The Microsoft.Agents.AI.Valkey package was scaffolded with working ValkeyChatHistoryProvider and ValkeyContextProvider implementations. This PR completes the package by adding missing pieces identified during review, fixing bugs in the existing sample, and adding a Bedrock-powered sample for AWS users.

Fixes issue microsoft#5445

Description

###Valkey package improvements:

• Added ValkeyProviderScope copy constructor matching the Mem0ProviderScope pattern, with null-guard via Throw.IfNull
• Added XML doc tags to all ValkeyProviderScope properties for consistency with Mem0ProviderScope
• Kept the package dependency lean — only references Microsoft.Agents.AI.Abstractions (no dependency on Microsoft.Agents.AI)

###Bug fix in existing sample:

• Fixed Program.cs — the stateInitializer referenced session?.Id which doesn't exist on AgentSession. Replaced with Guid.NewGuid():N using a discard parameter to make intent clear

###New Bedrock sample (AgentWithMemory_Step03_MemoryUsingValkey_Bedrock):

• Demonstrates both ValkeyChatHistoryProvider and ValkeyContextProvider powered by Amazon Bedrock via AWSSDK.Extensions.Bedrock.MEAI
• Uses IAmazonBedrockRuntime.AsIChatClient() which provides an IChatClient that plugs directly into the Agent Framework's ChatClientAgent
• Uses the standard AWS credential chain (env vars, profile, IAM role)
• Includes README with prerequisites, environment variables, and run instructions

###Infrastructure:

Added AWSSDK.Extensions.Bedrock.MEAI v4.0.6.10 to Directory.Packages.props
Registered the Bedrock sample in agent-framework-dotnet.slnx
Unit tests:

• Added tests for ValkeyProviderScope copy constructor (clone all properties, null source throws)
• 23 tests total, all passing

Contribution Checklist

• The code builds clean without any errors or warnings
• The PR follows the Contribution Guidelines
• All unit tests pass, and I have added new tests where possible
• Is this a breaking change? If yes, add "[BREAKING]" prefix to the title of the PR.

[Jonathan-Improving]

Consolidated Code Review: PR #2 — Adds Valkey to Chat Message History

PR: issue-5445 → main | Reviewed: 2026-04-27 | Scope: 16 changed files, +1,427 lines
Reviewers: GLIDE/Domain, DRY/Abstraction, Security (×2 rounds, consolidated)

---

🔒 Security Review

[VULN-001] Cross-Scope Data Leak — ThreadId Stored but Not Filtered (High) — See inline comment on ValkeyContextProvider.cs:178-193.

[VULN-002] Incomplete Query/Tag Escaping (Medium) — See inline comment on ValkeyContextProvider.cs:330-340.

Needs Verification:

• [VERIFY-001] No TTL on stored data — messages persist indefinitely. PII accumulates without bound. Intentional?
• [VERIFY-002] No try/catch around JsonSerializer.Deserialize<ChatMessage> in ValkeyChatHistoryProvider.ProvideChatHistoryAsync — see inline comment. Malformed JSON crashes the session.

Cleared: No arbitrary Valkey command injection (RESP parameterization). Indirect prompt injection risk documented accurately. No hardcoded secrets in samples. Connection security is a deployment concern, correctly documented.

---

🔴 Critical — Must Fix

1. Unit tests cover ~20% of public API surface
All 23 tests are synchronous constructor/property checks. Zero tests for any async method (ProvideChatHistoryAsync, StoreChatHistoryAsync, ProvideMessagesAsync, StoreAIContextAsync, EnsureIndexAsync, ClearMessagesAsync, GetMessageCountAsync, DisposeAsync), zero tests for security-critical methods (EscapeTag, EscapeQuery, ParseSearchResults), and zero tests for business logic (MaxMessages trimming, MaxMessagesToRetrieve limiting). InternalsVisibleTo is configured but unused. The Mem0 unit tests in this repo set a clear standard these tests fall far short of. Additionally, StateInitializer_NoScopeFields_Throws never triggers the lazy validation — it just asserts Assert.NotNull(provider).

2. CancellationToken never forwarded
Both providers accept CancellationToken in every async method but never use it. At minimum add cancellationToken.ThrowIfCancellationRequested() before I/O calls and between loop iterations. The Mem0 provider forwards CT to its HTTP calls.

3. EnsureIndexAsync race condition — See inline comment on ValkeyContextProvider.cs:268-275.

---

🟡 Medium

4. Constructor/DisposeAsync duplication — extract ValkeyConnectionManager
Both providers have 2 constructors each with ~80% identical bodies and character-for-character identical DisposeAsync implementations. An internal ValkeyConnectionManager (composition, not inheritance) would eliminate 4 duplicated constructor bodies and 2 identical DisposeAsync methods.

5. Error handling inconsistency between providers
ValkeyContextProvider wraps operations in try/catch (matching Mem0). ValkeyChatHistoryProvider does not — exceptions propagate unhandled. Either add try/catch to match, or document the intentional difference.

6. Synchronous ConnectionMultiplexer.Connect() in constructors
Blocking network call in constructor. Can cause thread-pool starvation in ASP.NET Core / DI scenarios. Consider ConnectAsync() via lazy init, or document that callers should prefer the IConnectionMultiplexer overload.

7. No disposed-state guard — Neither provider has a _disposed flag. After DisposeAsync(), subsequent calls throw opaque errors.

8. StoreChatHistoryAsync pushes messages one-by-one — N round-trips. ListRightPushAsync accepts RedisValue[] for batch push.

9. Missing [JsonConstructor] on State classes — Unlike the Mem0 pattern. Could break session state serialization.

10. No PII redaction in log messages — ConversationId, UserId, AgentId logged directly. Mem0 uses a Redactor pattern.

---

🟢 Low

11. Duplicate <NoWarn> line in csproj (see inline)
12. NuGet <Description> says "vector search" — should say "full-text search" (see inline)
13. EscapeQuery allocates char[] array per call — make special a static readonly field
14. ProvideChatHistoryAsync fetches all messages then trims — use ListRangeAsync(key, -N, -1)
15. Missing Throw.IfNull(context) inconsistency across methods
16. Sample default model "gpt-5.4-mini" — not a known Azure OpenAI model name
17. MaxResults.ToString() uses current culture — use CultureInfo.InvariantCulture
18. PR claims bug fix not visible in diff — both sample files are new, not modified

---

✅ Positives

• Excellent pattern consistency with Mem0Provider — ValkeyProviderScope mirrors Mem0ProviderScope exactly
• Clean separation — ChatHistoryProvider (lists, no search module) vs ContextProvider (FT.SEARCH)
• Ownership semantics — _ownsConnection correctly tracks disposal responsibility
• Thorough XML documentation with security considerations (PII, compromised store, indirect prompt injection)
• Scope validation — ValidateStateInitializer prevents unbounded queries
• Idiomatic Valkey usage — FT.CREATE with "Index already exists" catch, RPUSH+LTRIM, TAG filters
• Two high-quality samples (Azure OpenAI + Bedrock) with clear READMEs
• ConfigureAwait(false) used consistently — correct for library code
• Lean dependencies — only Abstractions, StackExchange.Redis, Logging.Abstractions

Checklist Assessment

Area	Status	Notes
Functionality	✅	Providers implement claimed features correctly
Code Quality	🟡	Good structure; thread safety, disposal, DRY issues
Testing	🔴	~20% API coverage, no async/behavioral tests
Security	🟡	ThreadId leak, incomplete escaping, no TTL
Performance	🟡	One-by-one push, fetch-all-then-trim, per-call allocations
Documentation	✅	Excellent XML docs, READMEs; minor NuGet description issue

[Jonathan-Improving]

🟢 NuGet description inaccuracy

The <Description> says "context provider with vector search" but ValkeyContextProvider uses full-text search (FT.SEARCH with TEXT fields). There are no VECTOR fields in the schema. Should say "full-text search" instead.

[Jonathan-Improving]

🔴 Race condition — _indexCreated not thread-safe

_indexCreated is a plain bool read/written without synchronization. Under concurrent requests, multiple threads can see false and all attempt FT.CREATE. The catch handles "Index already exists", but the flag itself isn't volatile — reads can be stale on other cores.

Fix: Use SemaphoreSlim for full correctness, or at minimum Volatile.Read/Volatile.Write:

private readonly SemaphoreSlim _indexLock = new(1, 1);

private async Task EnsureIndexAsync()
{
    if (Volatile.Read(ref _indexCreated)) return;
    await _indexLock.WaitAsync().ConfigureAwait(false);
    try
    {
        if (_indexCreated) return;
        // ... FT.CREATE ...
        Volatile.Write(ref _indexCreated, true);
    }
    finally { _indexLock.Release(); }
}

[Jonathan-Improving]

🟢 Duplicate <NoWarn> line

<NoWarn>$(NoWarn);CA1873</NoWarn> appears on both line 7 and line 8. Copy-paste error — remove the duplicate.

[Jonathan-Improving]

🟡 Inconsistent error handling — no try/catch on deserialization

JsonSerializer.Deserialize<ChatMessage> is called here with no try/catch. If the Valkey store contains malformed JSON (corruption, tampering), this throws JsonException unhandled and crashes the session.

ValkeyContextProvider wraps both ProvideMessagesAsync and StoreAIContextAsync in try/catch (matching the Mem0 pattern). This provider does not — neither ProvideChatHistoryAsync nor StoreChatHistoryAsync have error handling.

Either add try/catch to match, or document the intentional difference in error handling strategy.

[Jonathan-Improving]

🔒 [VULN-001] Cross-Scope Data Leak — ThreadId stored but not filtered (High)

StoreAIContextAsync writes thread_id to every HASH document (line 243), but the search filter here only checks ApplicationId, AgentId, and UserId — ThreadId is never included. A developer who sets ThreadId expecting thread-level isolation gets none — all threads for the same user/agent/app share memories.

In multi-thread scenarios (e.g., a support agent handling multiple customer conversations), memories from one thread bleed into another.

Fix:

if (!string.IsNullOrEmpty(scope.ThreadId))
{
    filterParts.Add($"@thread_id:{{{EscapeTag(scope.ThreadId)}}}");
}

[Jonathan-Improving]

🔒 [VULN-002] Incomplete Query/Tag Escaping (Medium)

EscapeQuery is missing <, % (fuzzy match operator), and , from the special characters array. > is escaped but < is not.

EscapeTag (line 330) is also missing | (tag value separator) and ,. A crafted scope value containing | (e.g., UserId = "user1|user2") could match multiple tag values, potentially retrieving another user's data.

No arbitrary Valkey command injection risk (StackExchange.Redis uses parameterized RESP), but query manipulation within FT.SEARCH is possible.

Fix: Add missing characters to both methods. Consider an allowlist approach for tag values.