.NET: Make default-approval harness features configurable + customizable shell tool#6880
Conversation
There was a problem hiding this comment.
Pull request overview
This PR adds configuration knobs to opt out of approval gating for default harness tools in the .NET implementation, and extends HarnessAgent so the shell tool’s name/description/approval behavior can be customized at construction time (rather than relying on downstream auto-approval rules).
Changes:
- Add
DisableReadOnlyToolApproval/DisableWriteToolApprovaltoFileAccessProviderOptionsand wrap file-access tools with approval only when required. - Add
ShellToolName,ShellToolDescription, andShellToolRequireApprovaltoHarnessAgentOptionsand forward them intoShellExecutor.AsAIFunction(...). - Add unit tests covering the new file-access approval toggles and the shell tool option forwarding behavior.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotnet/tests/Microsoft.Agents.AI.UnitTests/Harness/FileAccess/FileAccessProviderTests.cs | Adds tests validating per-group approval opt-out behavior for file-access tools. |
| dotnet/tests/Microsoft.Agents.AI.Harness.UnitTests/HarnessAgentTests.cs | Adds tests validating shell tool name/description/approval forwarding and defaults. |
| dotnet/src/Microsoft.Agents.AI/Harness/FileAccess/FileAccessProviderOptions.cs | Introduces new options to disable approval gating separately for read-only vs write file-access tool groups. |
| dotnet/src/Microsoft.Agents.AI/Harness/FileAccess/FileAccessProvider.cs | Implements conditional approval wrapping via a helper rather than always wrapping every tool. |
| dotnet/src/Microsoft.Agents.AI.Harness/HarnessAgentOptions.cs | Adds shell tool customization options surfaced on HarnessAgentOptions. |
| dotnet/src/Microsoft.Agents.AI.Harness/HarnessAgent.cs | Forwards the new shell tool customization options into ShellExecutor.AsAIFunction(...). |
|
Flagged issue
Source: automated DevFlow PR review |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 91%
✓ Correctness
The PR introduces configurable approval options for FileAccessProvider and shell tool in HarnessAgent. The implementation is correct: new options default to preserving existing behavior (all tools require approval, shell tool named 'run_shell'), the WrapWithApprovalIfRequired helper mirrors existing patterns in AgentSkillsProvider and shell executors, and the named-parameter call to AsAIFunction correctly lets the
nameparameter fall through to its default 'run_shell' when ShellToolName is null. All changes are additive and non-breaking.
✓ Security Reliability
This PR is clean from a security and reliability perspective. All new configurability options default to the secure behavior (approval required). The approval-disable flags are developer-configured at construction time, stored as immutable readonly fields, and do not accept external or user-controlled input. The null-handling for ShellToolName correctly falls back to the executor's default. No injection risks, resource leaks, missing validation, or unhandled failure modes identified.
✓ Test Coverage
Test coverage for the new features is solid overall. The PR adds tests for custom shell tool options (name, description, approval), default shell tool behavior, and all three FileAccessProvider approval configurations (read-only disabled, write disabled, both disabled). One minor gap: the
ShellExecutor_DefaultsToApprovalAndDefaultNameAsynctest claims to verify the default name (per its name) but only assertsrequireApproval, missing an assertion on thenameparameter actually received by the executor. There's also no test combiningDisableWriteTools=truewithDisableWriteToolApproval=trueto verify the documented no-op interaction, though the logic makes this trivially correct.
✓ Failure Modes
This PR is clean from a failure-mode perspective. The changes are additive, backward-compatible (all new options default to preserving existing behavior), and well-tested. The branching logic in HarnessAgent.BuildChatOptions correctly handles the null/non-null ShellToolName case by using named parameters to let the default name apply. The FileAccessProvider changes follow the same WrapWithApprovalIfRequired pattern already used by AgentSkillsProvider. No silent failures, lost errors, or race conditions are introduced.
✗ Design Approach
I found one design gap in the new shell-tool approval customization. The file-access changes follow an existing repo pattern, but the new harness-level
ShellToolRequireApprovaloption is forwarded straight into the executor even though the default local shell executor explicitly refuses to create an unapproved tool unless it was separately constructed withAcknowledgeUnsafe = true. That makes the new top-level option fail on a concrete, supported path instead of being a self-contained source-level configuration knob.
Flagged Issues
-
HarnessAgentOptions.ShellToolRequireApproval = falseis not a standalone opt-out forLocalShellExecutor:HarnessAgentforwards it directly, butLocalShellExecutor.AsAIFunction(requireApproval: false)throws unless the executor was also created withAcknowledgeUnsafe = true(dotnet/src/Microsoft.Agents.AI.Tools.Shell/LocalShellExecutor.cs:43-49,328-335;dotnet/tests/Microsoft.Agents.AI.Tools.Shell.UnitTests/LocalShellExecutorTests.cs:155-159).
Automated review by westey-m's agents
Motivation & Context
Several harness features register their function tools as approval-required and provide no way to opt out at the source.
FileAccessProviderwraps every tool in anApprovalRequiredAIFunction, and theHarnessAgentshell tool is always approval-required. Today the only way to avoid these prompts is to add auto-approval rules at theToolApprovalAgentlevel, which forces a round-trip all the way up the stack and back down again just to suppress approval — inefficient and awkward when you already know at construction time that approval isn't wanted.Separately,
ShellExecutor.AsAIFunction(name, description, requireApproval)already supports a custom tool name/description/approval, butHarnessAgentcalled it with defaults only, so the shell tool was always exposed asrun_shellwith mandatory approval.This makes both configurable at the source.
Fixes #6875
Fixes #6789
Description & Review Guide
What are the major changes?
FileAccessProviderOptionsgainsDisableReadOnlyToolApprovalandDisableWriteToolApproval(both defaultfalse= approval required), matching the existing read-only/write split used byReadOnlyToolsAutoApprovalRule/AllToolsAutoApprovalRule.FileAccessProvidernow wraps each tool via aWrapWithApprovalIfRequiredhelper (mirroringAgentSkillsProvider) instead of hardcodingApprovalRequiredAIFunction.HarnessAgentOptionsgainsShellToolName,ShellToolDescription, andShellToolRequireApproval(defaulttrue).HarnessAgentforwards these toShellExecutor.AsAIFunction(...), falling back to the executor's own default name (run_shell) whenShellToolNameisnull.What is the impact of these changes?
run_shell).What do you want reviewers to focus on?
FileAccessProviderOptions, and the null-fallback behavior for the shell tool name.Supersedes #6793. That PR (also linked to #6789) adds only
ShellToolName. This PR is a superset: it additionally forwards the shell tool description and approval flag, and addresses #6875 (per-group approval opt-out forFileAccessProvider). #6793 can be closed as superseded once this merges.Related Issue
Fixes #6875
Fixes #6789
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.