Pull request overview

Adds opt-in configuration to the .NET AgentSkillsProvider so hosts can selectively disable the tool-approval wrapper for specific skill tools (load skill, read resource, run script), while preserving existing default behavior.

Changes:

• Introduced per-tool approval toggles on AgentSkillsProviderOptions (DisableLoadSkillApproval, DisableReadSkillResourceApproval, DisableRunSkillScriptApproval).
• Updated AgentSkillsProvider tool construction to conditionally wrap each tool in ApprovalRequiredAIFunction.
• Added unit tests validating wrapper behavior for each individual toggle and for disabling all approvals.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

Automated Code Review

Reviewers: 5 | Confidence: 90%

✓ Correctness

This PR adds three boolean options (DisableLoadSkillApproval, DisableReadSkillResourceApproval, DisableRunSkillScriptApproval) to AgentSkillsProviderOptions to allow selectively disabling approval wrapping for specific skill tools. The implementation is correct: the null-conditional pattern this._options?.DisableXAproval is not true properly handles the case where options is null (defaulting to requiring approval), and the helper method WrapWithApprovalIfRequired is straightforward. Default behavior is preserved (all tools require approval when options are not provided or flags are false). Tests comprehensively cover each option individually and in combination.

✓ Security Reliability

The PR adds per-tool options to disable approval gating for skill tools. The implementation is correct with safe defaults (approval required when options are null or properties are false). The is not true null-coalescing pattern correctly handles the nullable _options field. The only concern is that DisableRunSkillScriptApproval — which removes human-in-the-loop oversight for arbitrary script execution — lacks a security warning in its XML docs, unlike the analogous IncludeDetailedErrors property which has detailed guidance about trusted sources.

✓ Test Coverage

Test coverage for the new skill approval options is thorough. The PR includes: (1) an existing test verifying default behavior (all tools wrapped when no options passed, covering the null-safe _options?.Disable... path), (2) individual tests for each of the three disable options confirming the targeted tool is unwrapped while the others remain wrapped, and (3) a combined test disabling all approvals. Each test asserts both positive and negative conditions for all three tools, ensuring option isolation. No significant test coverage gaps identified.

✓ Failure Modes

The changes are clean from a failure-mode perspective. The null-conditional pattern on _options safely defaults to requiring approval when options are not provided. The new boolean properties default to false (approval required), preserving backward compatibility. The WrapWithApprovalIfRequired helper is straightforward. No silent failures, swallowed exceptions, or race conditions introduced.

✓ Design Approach

The per-tool approval flags are implemented at the provider level, but on the default ChatClientAgent path they do not fully deliver the advertised behavior when a response mixes approval-required and non-required skill tools. I found one design issue where the new options can still surface approval prompts unless callers also know to enable a separate chat-client decorator.

Automated review by SergeyMenshykh's agents

Automated Code Review

Reviewers: 5 | Confidence: 89% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach

Automated review by SergeyMenshykh's agents