-
Notifications
You must be signed in to change notification settings - Fork 8
Fix agentId parameter in foundry sdk #139
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…nto users/mrunalhirve/FixSDK
There was a problem hiding this comment.
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 fixes the agentId parameter handling in the Azure AI Foundry SDK integration by introducing an explicit agentInstanceId parameter to replace the implicit retrieval from turnContext.Activity.Recipient.AgenticAppId. This change improves the API design by making the agent instance identification explicit and decoupling it from the turn context.
Key Changes:
- Added
agentInstanceIdparameter to theAddToolServersToAgentAsyncmethod in both the interface and implementation - Replaced all usages of
turnContext.Activity.Recipient.AgenticAppIdwith the newagentInstanceIdparameter - Enhanced XML documentation from
<inheritdoc />to a complete parameter description
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/Tooling/Extensions/AzureAIFoundry/Services/IMcpToolRegistrationService.cs |
Added agentInstanceId parameter to the AddToolServersToAgentAsync method signature with updated documentation |
src/Tooling/Extensions/AzureAIFoundry/Services/McpToolRegistrationService.cs |
Implemented the new parameter, replaced AgenticAppId extraction logic with direct parameter usage, updated documentation, and corrected logging template |
Comments suppressed due to low confidence (1)
src/Tooling/Extensions/AzureAIFoundry/Services/McpToolRegistrationService.cs:111
- The newly added agentInstanceId parameter should be validated before use. Consider adding a null/empty check similar to the agentClient validation, such as:
if (string.IsNullOrWhiteSpace(agentInstanceId)) { throw new ArgumentException("Value cannot be null or whitespace.", nameof(agentInstanceId)); }
string agentInstanceId,
UserAuthorization userAuthorization,
string authHandlerName,
ITurnContext turnContext,
string? authToken = null)
| { | ||
| // Perform the (potentially async) work in a dedicated task to keep this synchronous signature. | ||
| var (toolDefinitions, toolResources) = GetMcpToolDefinitionsAndResourcesAsync(agenticAppId, authToken ?? string.Empty, turnContext).GetAwaiter().GetResult(); | ||
| var (toolDefinitions, toolResources) = GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).GetAwaiter().GetResult(); |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using GetAwaiter().GetResult() in an async method defeats the purpose of async/await and can lead to potential deadlocks. Since this method is already async, consider using await instead: var (toolDefinitions, toolResources) = await GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).ConfigureAwait(false);
| var (toolDefinitions, toolResources) = GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).GetAwaiter().GetResult(); | |
| var (toolDefinitions, toolResources) = await GetMcpToolDefinitionsAndResourcesAsync(agentInstanceId, authToken ?? string.Empty, turnContext).ConfigureAwait(false); |
|
|
||
| try | ||
| { | ||
| // Perform the (potentially async) work in a dedicated task to keep this synchronous signature. |
Copilot
AI
Dec 12, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment states "keep this synchronous signature" but the method signature is actually async (AddToolServersToAgentAsync). This comment appears to be outdated or incorrect. Consider removing or updating the comment to accurately reflect that this is an async method that is calling GetAwaiter().GetResult() on another async operation.
| // Perform the (potentially async) work in a dedicated task to keep this synchronous signature. |
| /// <exception cref="ArgumentNullException"></exception> | ||
| public async Task AddToolServersToAgentAsync( | ||
| PersistentAgentsClient agentClient, | ||
| string agentInstanceId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You have both TurnContext and auth token here, you should use this instead of adding another parameter and letting the caller handle it:
// resolve agent identity from context or token.
string agenticAppId = Runtime.Utils.Utility.ResolveAgentIdentity(turnContext, authToken);
No description provided.