Confusing parameter/property names in Azure.AI.Agents.Persistent.MCPApproval

[stephentoub]

The MCPApproval class has a constructor:

azure-sdk-for-net/sdk/ai/Azure.AI.Agents.Persistent/src/Custom/MCPApproval.cs

Lines 54 to 55 in ea4b6a1

	/// <param name="trust">The trust level, can be "always" or "never"</param>
	public MCPApproval(string trust)

I read that "trust" parameter being "always" or "never" as meaning whether to always trust a tool or never trust a tool. But it's the opposite. "always" here maps to approval always being required, and "never" maps to approval never being required, which is the opposite of the tool "always" being trusted or "never" being trusted, as highlighted by the subsequent properties:

azure-sdk-for-net/sdk/ai/Azure.AI.Agents.Persistent/src/Custom/MCPApproval.cs

Lines 61 to 68 in ea4b6a1

	/// <summary>
	/// Return true if we do not trust all tool and always need to ask for approval before sending data to server.
	/// </summary>
	public bool AlwaysApprove{get => string.Equals(_forAllToolsApproval, ALWAYS);}
	/// <summary>
	/// Return true if we trust all tools and do not need to ask for approval before before sending data to server.
	/// </summary>
	public bool NeverApprove { get => string.Equals(_forAllToolsApproval, NEVER); }

This parameter should be renamed, e.g. "requireApproval".

I suggest those properties should also be renamed. "AlwaysApprove" reads different to me than "AlwaysRequireApproval"; the former sounds like it's saying the system should always automatically approve of any tool calls, but that's the opposite of which it actually means, which is that all tool calls require approval.

[nick863]

@stephentoub, the new version of Azure.AI.Agent.Persistent, containing fix for this issue was released. Please upgrade using command

dotnet add package Azure.AI.Agents.Persistent --version 1.2.0-beta.6

[github-actions]

Hi @stephentoub. Thank you for opening this issue and giving us the opportunity to assist. We believe that this has been addressed. If you feel that further discussion is needed, please add a comment with the text "/unresolve" to remove the "issue-addressed" label and continue the conversation.

[stephentoub]

@nick863, thanks, though it doesn't look like that PR addressed the "trust" constructor parameter?