Skip to content

.NET: Make default-approval harness features configurable + customizable shell tool#6880

Merged
westey-m merged 2 commits into
microsoft:mainfrom
westey-m:dotnet-file-access-approval-opt-out
Jul 2, 2026
Merged

.NET: Make default-approval harness features configurable + customizable shell tool#6880
westey-m merged 2 commits into
microsoft:mainfrom
westey-m:dotnet-file-access-approval-opt-out

Conversation

@westey-m

@westey-m westey-m commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Motivation & Context

Several harness features register their function tools as approval-required and provide no way to opt out at the source. FileAccessProvider wraps every tool in an ApprovalRequiredAIFunction, and the HarnessAgent shell tool is always approval-required. Today the only way to avoid these prompts is to add auto-approval rules at the ToolApprovalAgent level, 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, but HarnessAgent called it with defaults only, so the shell tool was always exposed as run_shell with mandatory approval.

This makes both configurable at the source.

Fixes #6875
Fixes #6789

Description & Review Guide

  • What are the major changes?

    • FileAccessProviderOptions gains DisableReadOnlyToolApproval and DisableWriteToolApproval (both default false = approval required), matching the existing read-only/write split used by ReadOnlyToolsAutoApprovalRule / AllToolsAutoApprovalRule. FileAccessProvider now wraps each tool via a WrapWithApprovalIfRequired helper (mirroring AgentSkillsProvider) instead of hardcoding ApprovalRequiredAIFunction.
    • HarnessAgentOptions gains ShellToolName, ShellToolDescription, and ShellToolRequireApproval (default true). HarnessAgent forwards these to ShellExecutor.AsAIFunction(...), falling back to the executor's own default name (run_shell) when ShellToolName is null.
  • What is the impact of these changes?

    • Non-breaking and additive: all new options default to preserving today's behavior (all file-access tools and the shell tool still require approval; the shell tool is still named run_shell).
    • Python parity is intentionally out of scope — these are .NET-only issues; the Python providers currently also hardcode file-access approval and lack the harness shell-tool passthrough. Aligning Python can be done in a follow-up.
  • What do you want reviewers to focus on?

    • The read-only vs. write approval granularity 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 for FileAccessProvider). #6793 can be closed as superseded once this merges.

Related Issue

Fixes #6875
Fixes #6789

Contribution Checklist

  • The code builds clean without any errors or warnings
  • All unit tests pass, and I have added new tests where possible
  • The PR follows the Contribution Guidelines
  • This PR is linked to an issue and there is no other open PR for this issue (see Related Issue above).
  • This is not a breaking change. If it is a breaking change, add the breaking change label (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.

Copilot AI review requested due to automatic review settings July 2, 2026 11:24
@giles17 giles17 added the .NET Usage: [Issues, PRs], Target: .Net label Jul 2, 2026
@westey-m westey-m marked this pull request as ready for review July 2, 2026 11:26

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 / DisableWriteToolApproval to FileAccessProviderOptions and wrap file-access tools with approval only when required.
  • Add ShellToolName, ShellToolDescription, and ShellToolRequireApproval to HarnessAgentOptions and forward them into ShellExecutor.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(...).

@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Flagged issue

HarnessAgentOptions.ShellToolRequireApproval = false is not a standalone opt-out for LocalShellExecutor: HarnessAgent forwards it directly, but LocalShellExecutor.AsAIFunction(requireApproval: false) throws unless the executor was also created with AcknowledgeUnsafe = 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).


Source: automated DevFlow PR review

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 name parameter 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_DefaultsToApprovalAndDefaultNameAsync test claims to verify the default name (per its name) but only asserts requireApproval, missing an assertion on the name parameter actually received by the executor. There's also no test combining DisableWriteTools=true with DisableWriteToolApproval=true to 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 ShellToolRequireApproval option 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 with AcknowledgeUnsafe = 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 = false is not a standalone opt-out for LocalShellExecutor: HarnessAgent forwards it directly, but LocalShellExecutor.AsAIFunction(requireApproval: false) throws unless the executor was also created with AcknowledgeUnsafe = 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

@westey-m westey-m added this pull request to the merge queue Jul 2, 2026
Merged via the queue into microsoft:main with commit 48436f8 Jul 2, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

.NET Usage: [Issues, PRs], Target: .Net

Projects

None yet

5 participants