.NET: fix: Require explicit TokenCredential in AddFoundryToolboxes#6877
Conversation
The AddFoundryToolboxes extension methods now require callers to pass a TokenCredential explicitly rather than relying on an internally-created default credential. This makes the credential choice intentional and avoids non-deterministic credential probing in production environments. Breaking change (experimental API): - AddFoundryToolboxes(IServiceCollection, params string[]) becomes AddFoundryToolboxes(IServiceCollection, TokenCredential, params string[]) - AddFoundryToolboxes(IServiceCollection, Action?, params string[]) becomes AddFoundryToolboxes(IServiceCollection, TokenCredential, Action?, params string[]) - Azure.Identity package dependency removed from Foundry.Hosting library. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
temporarily deployed
to
integration
GitHub Actions
Inactive
SergeyMenshykh
temporarily deployed
to
integration
GitHub Actions
Inactive
giles17
added
documentation
github-actions
Bot
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the .NET Foundry hosting extensions so AddFoundryToolboxes requires an explicit TokenCredential from callers, eliminating implicit/default credential selection and removing the Azure.Identity dependency from the hosting library.
Changes:
- Updated
AddFoundryToolboxesoverloads to require aTokenCredentialparameter and removed internal default-credential creation. - Removed the
Azure.Identitypackage reference fromMicrosoft.Agents.AI.Foundry.Hosting. - Updated docs/samples/tests/cref references to pass a credential explicitly.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| dotnet/tests/Foundry.Hosting.IntegrationTests/Fixtures/ToolboxOAuthConsentHostedAgentFixture.cs | Updates fixture comment to reflect new AddFoundryToolboxes(credential, ...) signature. |
| dotnet/tests/Foundry.Hosting.IntegrationTests.TestContainer/Program.cs | Extracts credential and passes it into AIProjectClient and AddFoundryToolboxes. |
| dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/ServiceCollectionExtensions.cs | Changes the AddFoundryToolboxes API to require TokenCredential and adjusts DI registration. |
| dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/Microsoft.Agents.AI.Foundry.Hosting.csproj | Removes Azure.Identity package reference from the hosting library. |
| dotnet/src/Microsoft.Agents.AI.Foundry.Hosting/FoundryToolboxHealthCheck.cs | Updates XML cref to match the new method signature and adds Azure.Core import. |
| dotnet/samples/04-hosting/FoundryHostedAgents/responses/Hosted-Toolbox/README.md | Updates documentation to show AddFoundryToolboxes(credential, name). |
| dotnet/samples/04-hosting/FoundryHostedAgents/responses/Hosted-Toolbox/Program.cs | Passes the existing credential into AddFoundryToolboxes. |
| dotnet/samples/04-hosting/FoundryHostedAgents/responses/Hosted-Toolbox-AuthPaths/README.md | Updates toolbox wiring guidance to the new signature. |
| dotnet/samples/04-hosting/FoundryHostedAgents/responses/Hosted-Toolbox-AuthPaths/Program.cs | Updates comments and call site to pass the credential explicitly. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 82%
✓ Correctness
The PR correctly implements the stated goal of requiring an explicit TokenCredential. The credential flows through DI to FoundryToolboxService and its handler correctly. Null guards are present. The Azure.Identity removal is safe as Azure.Core is still available transitively via Azure.AI.Projects. All call sites (samples, tests) are consistently updated. No correctness issues found.
✓ Security Reliability
This PR is a clear security improvement—removing implicit DefaultAzureCredential probing from library internals and requiring calers to supply an explicit TokenCredential. The input validation (ArgumentNullException.ThrowIfNull) is correct. One reliability concern: the switch from TryAddSingleton to AddSingleton for TokenCredential registration means repeated calls to AddFoundryToolboxes (an explicitly supported scenario per line 174's comment) will register duplicate TokenCredential singletons, and the credential bleds into the global DI container where it could affect other services resolving TokenCredential.
✓ Test Coverage
The PR adds a required TokenCredential parameter to AddFoundryToolboxes and validates it with ArgumentNullException.ThrowIfNull, but there are zero unit tests for AddFoundryToolboxes in the existing ServiceCollectionExtensionsTests.cs. The null-credential validation, the DI registration behavior (notably the switch from TryAddSingleton to AddSingleton for TokenCredential), and the options configuration all lack test coverage.
✓ Failure Modes
The PR correctly removes implicit DefaultAzureCredential probing. However, the switch from TryAddSingleton to AddSingleton for the TokenCredential registration introduces a silent last-wins behavior when AddFoundryToolboxes is called multiple times (a scenario the code explicitly supports per comments at lines 174 and 182-183). This can silently overwrite a credential from an earlier call or conflict with other components that register TokenCredential.
✓ Design Approach
I found one design-level issue in the DI wiring for
AddFoundryToolboxes: the new API requires an explicit credential, but the implementation still collapses all toolbox registrations onto a singleTokenCredential. That means repeatedAddFoundryToolboxes(...)calls can silently authenticate every toolbox with the wrong credential, which is broader behavior than the PR rationale describes.
Suggestions
- Consider using
services.TryAddSingleton<TokenCredential>(credential)at ServiceCollectionExtensions.cs:169 to match the first-wins semantics used forFoundryToolboxService(line 172) and avoid silent credential replacement whenAddFoundryToolboxesis called multiple times. Alternatively, scope the credential more narrowly (e.g., pass it directly toFoundryToolboxServicevia a factory registration) to support per-toolbox credential isolation.
Automated review by SergeyMenshykh's agents
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
temporarily deployed
to
integration
GitHub Actions
Inactive
SergeyMenshykh
temporarily deployed
to
integration
GitHub Actions
Inactive
Inject the AddFoundryToolboxes credential directly into the FoundryToolboxService factory and fail early if the service was already registered. This avoids registering TokenCredential in the host DI container while preserving a single toolbox service instance for both request handling and hosted startup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
SergeyMenshykh
temporarily deployed
to
integration
GitHub Actions
Inactive
SergeyMenshykh
temporarily deployed
to
integration
GitHub Actions
Inactive
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Summary
AddFoundryToolboxesnow requires callers to pass aTokenCredentialexplicitly. The toolbox service uses that credential directly instead of resolving a default credential from DI.Notes
AddFoundryToolboxes(credential, name).FoundryToolboxServiceregistration.Azure.Identitydependency from the hosting library.This is a breaking change to an experimental API.