.NET: Improve Todo multithreading and inject todos into message list#5655
Conversation
There was a problem hiding this comment.
Automated Code Review
Reviewers: 4 | Confidence: 87%
✓ Correctness
This PR converts TodoProvider to be thread-safe using per-session SemaphoreSlim locks, makes command handlers async, and adds todo list message injection into the AI context. The implementation is correct: snapshots are taken under the lock, user callbacks are invoked outside the lock, ConditionalWeakTable provides thread-safe lock-per-session management, and the async conversions (both sync ValueTask.FromResult in ModeCommandHandler and async/await in TodoCommandHandler) are appropriate. No correctness issues found.
✓ Security Reliability
The PR adds thread-safety via per-session SemaphoreSlim locks to TodoProvider, converting synchronous methods to async. The locking pattern is correct: all state reads/writes are performed under the lock, snapshots are returned via ToList(), and the ConditionalWeakTable ensures lock lifetime is tied to session lifetime. The IDisposable implementation only disposes _nullSessionLock (acceptable since ConditionalWeakTable entries are GC'd with their keys). One minor reliability concern: public async methods and tool lambdas don't accept CancellationToken for the semaphore wait, which could theoretically cause indefinite waits if a bug prevents lock release, though practically the operations under the lock are trivially fast in-memory operations making this low risk.
✓ Test Coverage
The PR adds solid test coverage for new features (message injection, suppression, custom builder, snapshot safety, and concurrency). However, there are a few gaps: the new
.Trim()behavior on Title/Description during add operations has no test verifying whitespace is actually trimmed, the description-formatting branch in FormatTodoListMessage is never exercised, and the null-session lock path is untested.
✗ Design Approach
I found two design-level problems in the new todo-context approach. First, the provider now injects the full todo list through
AIContext.Messageson every invocation, but that channel is part of the request/history pipeline, so this change solves state visibility by appending repeated snapshots into durable chat history rather than supplying transient context. Second, the new "snapshot" reads only clone the list container; they still expose live mutableTodoIteminstances, so callers and custom message builders can mutate provider state from outside the lock.
Flagged Issues
- The snapshot API (
state.Items.ToList()) only copies the list container, not theTodoItemobjects themselves. SinceTodoItemexposes public seters for Title, Description, and IsComplete, calers and custom message builders can mutate internal provider state from outside the synchronization lock. Items should be cloned or exposed as immutable projections.
Automated review by westey-m's agents
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET todo harness so todo state is serialized per session during concurrent tool calls, and so the todo list can be injected into the agent’s request context before each invocation. It also updates the shared console sample to use async command handling for the new async todo APIs.
Changes:
- Made
TodoProvideroperations async and added per-session locking around todo reads/writes. - Added configurable todo-list message injection via
TodoProviderOptions. - Updated tests and console command handlers to use the new async APIs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/Todo/TodoProviderTests.cs |
Expands coverage for async helper methods, todo message injection, and concurrent tool execution. |
dotnet/src/Microsoft.Agents.AI/Harness/Todo/TodoProviderOptions.cs |
Adds options for suppressing or customizing injected todo-list messages. |
dotnet/src/Microsoft.Agents.AI/Harness/Todo/TodoProvider.cs |
Implements per-session locking, async todo accessors, and injected todo-list context messages. |
dotnet/samples/02-agents/Harness/Harness_Shared_Console/HarnessConsole.cs |
Awaits async command handlers in the console loop. |
dotnet/samples/02-agents/Harness/Harness_Shared_Console/Commands/TodoCommandHandler.cs |
Switches /todos handling to async and uses GetAllTodosAsync. |
dotnet/samples/02-agents/Harness/Harness_Shared_Console/Commands/ModeCommandHandler.cs |
Adapts /mode handling to the async command-handler interface. |
dotnet/samples/02-agents/Harness/Harness_Shared_Console/Commands/ICommandHandler.cs |
Changes the command-handler contract from synchronous to async. |
Motivation and Context
Making sure that we have no issues when multiple Function Tools are called in parallel and ensuring that the LLM is kept up to date on any remaining todo items.
Description
Contribution Checklist