.NET: Add always approve helpers, improve sample and fix bug#5451
.NET: Add always approve helpers, improve sample and fix bug#5451westey-m merged 2 commits intomicrosoft:feature-harnessfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds a .NET Harness middleware for “don’t ask again” tool approvals (tool-level or tool+arguments), updates the Harness research sample to use it, and fixes a streaming persistence bug impacting approvals.
Changes:
- Introduces
ToolApprovalAgentmiddleware + persisted session state (ToolApprovalState/ToolApprovalRule) and “always approve” helper content/extension methods. - Adds extensive unit test coverage for rule persistence, queueing behavior, and streaming/non-streaming flows.
- Updates the Harness research sample and shared console UX to support interactive tool approval prompts; fixes per-service-call streaming persistence by cloning updates.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgent.cs | Implements tool approval middleware, rule matching, queueing, and streaming logic. |
| dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalState.cs | Defines persisted session state for rules, queued requests, and collected responses. |
| dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalRule.cs | Defines persisted approval rules (tool-level vs tool+arguments). |
| dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/AlwaysApproveToolApprovalResponseContent.cs | Adds wrapper content carrying “always approve” intent. |
| dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalRequestContentExtensions.cs | Adds helper extension methods to create “always approve” responses. |
| dotnet/src/Microsoft.Agents.AI/Harness/ToolApproval/ToolApprovalAgentBuilderExtensions.cs | Adds UseToolApproval builder extension for agent pipelines. |
| dotnet/src/Microsoft.Agents.AI/AgentJsonUtilities.cs | Adds source-gen JSON type registration for ToolApproval state/rules. |
| dotnet/src/Microsoft.Agents.AI/ChatClient/PerServiceCallChatHistoryPersistingChatClient.cs | Fixes streaming persistence behavior by cloning updates before storing. |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentTests.cs | Comprehensive tests for middleware behavior (streaming + non-streaming). |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalRuleTests.cs | Tests rule defaults + JSON round-tripping. |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/ToolApprovalAgentBuilderExtensionsTests.cs | Tests builder extension wiring and null-arg behavior. |
| dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/ToolApproval/AlwaysApproveToolApprovalResponseContentTests.cs | Tests wrapper content + extension method behavior and invariants. |
| dotnet/samples/02-agents/Harness/Harness_Step01_Research/WebBrowsingTool.cs | Refactors web browsing tool into an AIFunction implementation. |
| dotnet/samples/02-agents/Harness/Harness_Step01_Research/Program.cs | Updates sample agent pipeline to use tool approval + clearer pipeline composition. |
| dotnet/samples/02-agents/Harness/Harness_Shared_Console/HarnessConsole.cs | Adds interactive console flow to collect/prompt tool approvals during streaming. |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 3 | Confidence: 77%
✓ Security Reliability
The PR introduces a well-structured tool approval middleware (ToolApprovalAgent) with standing rule support. The overall design is sound: cancellation tokens propagate to inner agent calls, the fail-safe pattern ensures mismatches result in re-prompting rather than unauthorized approvals, and state is properly scoped to sessions. The PerServiceCallChatHistoryPersistingChatClient Clone() fix correctly prevents shared-reference mutation bugs. Two reliability observations: (1) both RunCoreAsync and RunCoreStreamingAsync contain while(true) loops that re-invoke the inner agent whenever all approval requests are auto-approved — while practically bounded by LM behavior and CancellationToken propagation, there is no defensive iteration limit; (2) ArgumentsMatch calls SerializeArgumentValue without forwarding the instance's JsonSerializerOptions, which could cause rule matching to fail (fail-safe) when custom serializer options are provided.
✓ Test Coverage
The AlwaysApproveToolApprovalResponseContentTests and ToolApprovalAgentBuilderExtensionsTests provide solid coverage of the extension methods and builder wiring. The behavioral change in PerServiceCallChatHistoryPersistingChatClient.cs (adding .Clone() on streaming response updates at line 191) lacks a dedicated regression test — no existing or new test verifies that stored streaming updates are independent copies, meaning a revert of this fix would go unnoticed. The ToolApprovalAgent core middleware tests (ToolApprovalAgentTests.cs, ToolApprovalRuleTests.cs) are expected in the next chunk and cannot be evaluated here. The test files provide thorough coverage of the ToolApprovalAgent and ToolApprovalRule classes, including constructor validation, rule matching, auto-approve flows (deferred and immediate), queue behavior for both streaming and non-streaming, content ordering, unwrapping, and serialization round-trips. Notable gaps: no dedicated tests for the new ToolApprovalState class, no test for MatchesRule when FunctionCallContent has null arguments but the rule specifies arguments (potential NullReferenceException), and no CancellationToken propagation tests for the auto-approve re-call loop.
✗ Design Approach
The new tool-approval middleware is a solid direction, but two design mismatches make the feature narrower and less reliable than the surface API suggests. First, the implementation treats standing approvals as
FunctionCallContent-only even though the existing approval flow already carries other tool-call shapes such as MCP tool calls. Second, the middleware advertises customizableJsonSerializerOptions, but rule matching falls back to default serialization when comparing live arguments, so custom converters/policies can make persisted "always approve with these arguments" rules stop matching.
Flagged Issues
- Argument-specific approval rules are stored with the caller-provided JsonSerializerOptions but matched using default serialization. This makes the public customization hook only half-applied and causes persisted rules to silently fail under predictable configurations with custom converters or naming policies.
Suggestions
- No tests verify CancellationToken propagation or cancellation behavior during the auto-approve re-call loop. If a caller cancels mid-loop, the agent should stop promptly.
Automated review by westey-m's agents
* .NET: Add a TODO AIContextProvider (microsoft#5233) * Add a TODO AIContextProvider * Add unit tests * Address PR comments * Address PR comments * Fix test after removing one tool * .NET: Add a ModeProvider for managing agent modes (microsoft#5247) * Add a ModeProvider for managing agent modes * Fix typo * Fix typo * Fix typo * Address PR comments * .NET: Add sample to show how to build a harness (microsoft#5268) * Add sample to show how to build a harness * Improve sample * Sample max output tokens and model * Fix encoding * Fix model name in readme * Address PR comments * .NET: Add context window size compaction strategy for harness (microsoft#5304) * Add context window size compaction strategy for harness * Apply suggestions from code review Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * Address PR comments --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> * .NET: Add a file memory provider (microsoft#5315) * Add a file memory provider * Address PR comments * Fix review comments. * Add additional unit tests * Addressing PR comments. * .NET: Harness: Improve prompts and add FileSystem store (microsoft#5365) * Harness: Improve prompts and add FileSystem store * Address PR comments * .NET: Harness: Improve path validation (microsoft#5404) * Harness: Improve path validation * Address PR comments * .NET: Add always approve helpers, improve sample and fix bug (microsoft#5451) * Add always approve helpers, improve sample and fix bug * Address PR comments * .NET: Make Todo, Mode and FileMemory providers more configurable (microsoft#5477) * Make Todo, Mode and FileMemory providers more configurable * Address PR comments. * .NET: Add subagents provider and sample (microsoft#5518) * Add subagents provider and sample * Addressing PR comments. * .NET: Harness filememory index plus instructions consistency (microsoft#5540) * Add FileMemoryProvider index and improve instruction consistency * Address PR comments. * Address PR comments * Address PR comments. * Apply suggestion from @rogerbarreto Co-authored-by: Roger Barreto <19890735+rogerbarreto@users.noreply.github.com> --------- Co-authored-by: Roger Barreto <19890735+rogerbarreto@users.noreply.github.com> * .NET: Refactor harness console to be more extensible and easy to understand with better UX (microsoft#5573) * Refactor harness console to be more extensible and easy to understand with better UX. * Fix formatting issues. * Allow multiple clarifications in one response * Address PR comments * .NET: Add FileAccessProvdider and concurrency fix for FileMemoryProvider (microsoft#5583) * Add FileAccessProvdider and concurrency fix for FileMemoryProvider * Address PR comments --------- Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Co-authored-by: Roger Barreto <19890735+rogerbarreto@users.noreply.github.com>
Motivation and Context
Description
Contribution Checklist