mirror of
https://github.com/microsoft/agent-framework.git
synced 2026-06-16 21:04:09 +08:00
Fix PR review issues: retry off-by-one, URI encoding, docs, tests, build
- Fix off-by-one in FoundryToolboxBearerTokenHandler retry loop (4 attempts → 3) - URI-encode version parameter in HostedMcpToolboxAITool.BuildAddress - Add XML doc clarifying version pinning is reserved for future use - Add comment clarifying AddHostedService deduplication safety - Fix DevTemporaryTokenCredential expiry to use DateTimeOffset.MaxValue - Fix AgentCard ambiguity in A2AServer sample with using alias - Add 18 new unit tests for retry handler and ReadMcpToolboxMarkers Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
alliscode
@@ -108,6 +108,6 @@ internal sealed class DevTemporaryTokenCredential : TokenCredential
|
||||
throw new CredentialUnavailableException($"{EnvironmentVariable} environment variable is not set.");
|
||||
}
|
||||
|
||||
return new AccessToken(this._token, DateTimeOffset.UtcNow.AddHours(1));
|
||||
return new AccessToken(this._token, DateTimeOffset.MaxValue);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -8,6 +8,7 @@ using Microsoft.Agents.AI;
|
||||
using Microsoft.Extensions.AI;
|
||||
using OpenAI;
|
||||
using OpenAI.Chat;
|
||||
using AgentCard = A2A.AgentCard;
|
||||
|
||||
namespace A2AServer;
|
||||
|
||||
|
||||
@@ -34,7 +34,11 @@ public sealed class HostedMcpToolboxAITool : HostedMcpServerTool
|
||||
/// Initializes a new instance of the <see cref="HostedMcpToolboxAITool"/> class.
|
||||
/// </summary>
|
||||
/// <param name="toolboxName">The Foundry toolbox name.</param>
|
||||
/// <param name="version">Optional pinned toolbox version. When <see langword="null"/>, the project's default version is used.</param>
|
||||
/// <param name="version">
|
||||
/// Optional pinned toolbox version. When <see langword="null"/>, the project's default version is used.
|
||||
/// Currently reserved for forward compatibility — version-specific routing is handled server-side by
|
||||
/// the Foundry proxy.
|
||||
/// </param>
|
||||
public HostedMcpToolboxAITool(string toolboxName, string? version = null)
|
||||
: base(
|
||||
serverName: NotNullOrWhitespace(toolboxName, nameof(toolboxName)),
|
||||
@@ -63,7 +67,7 @@ public sealed class HostedMcpToolboxAITool : HostedMcpServerTool
|
||||
|
||||
return string.IsNullOrEmpty(version)
|
||||
? $"{UriScheme}://{toolboxName}"
|
||||
: $"{UriScheme}://{toolboxName}?version={version}";
|
||||
: $"{UriScheme}://{toolboxName}?version={Uri.EscapeDataString(version)}";
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
|
||||
@@ -48,6 +48,7 @@ internal sealed class FoundryToolboxBearerTokenHandler : DelegatingHandler
|
||||
request.Headers.TryAddWithoutValidation("Foundry-Features", this._featuresHeaderValue);
|
||||
}
|
||||
|
||||
// MaxRetries is the total number of attempts (not additional retries after the first).
|
||||
for (int attempt = 0; attempt < MaxRetries; attempt++)
|
||||
{
|
||||
// Clone the request for retries (the original request cannot be sent twice)
|
||||
@@ -65,19 +66,20 @@ internal sealed class FoundryToolboxBearerTokenHandler : DelegatingHandler
|
||||
return response;
|
||||
}
|
||||
|
||||
// Last attempt exhausted — return the error response as-is.
|
||||
if (attempt == MaxRetries - 1)
|
||||
{
|
||||
return response;
|
||||
}
|
||||
|
||||
response.Dispose();
|
||||
|
||||
if (attempt < MaxRetries - 1)
|
||||
{
|
||||
await Task.Delay(TimeSpan.FromSeconds(Math.Pow(2, attempt)), cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
}
|
||||
await Task.Delay(TimeSpan.FromSeconds(Math.Pow(2, attempt)), cancellationToken)
|
||||
.ConfigureAwait(false);
|
||||
}
|
||||
|
||||
// Final attempt after backoff exhausted — return last response (already disposed above, so resend)
|
||||
return await base.SendAsync(
|
||||
await CloneRequestAsync(request, cancellationToken).ConfigureAwait(false),
|
||||
cancellationToken).ConfigureAwait(false);
|
||||
// Unreachable when MaxRetries > 0, but satisfies the compiler.
|
||||
throw new InvalidOperationException("Retry loop completed without returning a response.");
|
||||
}
|
||||
|
||||
private static async Task<HttpRequestMessage> CloneRequestAsync(
|
||||
|
||||
@@ -127,7 +127,11 @@ public sealed class FoundryToolboxService : IHostedService, IAsyncDisposable
|
||||
/// <see cref="FoundryToolboxOptions.StrictMode"/> to either reject or lazily open it.
|
||||
/// </summary>
|
||||
/// <param name="toolboxName">The Foundry toolbox name from the marker.</param>
|
||||
/// <param name="version">Optional pinned version; ignored when matching a pre-registered entry.</param>
|
||||
/// <param name="version">
|
||||
/// Optional pinned version. Currently reserved for future use — version-specific routing is
|
||||
/// handled server-side by the Foundry proxy. This parameter is accepted for forward compatibility
|
||||
/// but does not affect the proxy URL used to connect to the toolbox.
|
||||
/// </param>
|
||||
/// <param name="cancellationToken">The request cancellation token.</param>
|
||||
/// <exception cref="InvalidOperationException">
|
||||
/// Thrown when the toolbox is not pre-registered and <see cref="FoundryToolboxOptions.StrictMode"/>
|
||||
|
||||
@@ -161,7 +161,8 @@ public static class FoundryHostingExtensions
|
||||
// Register FoundryToolboxService as a singleton so it can be injected into the handler
|
||||
services.TryAddSingleton<FoundryToolboxService>();
|
||||
|
||||
// Add it as a hosted service so StartAsync is called before the app starts serving requests
|
||||
// AddHostedService uses TryAddEnumerable internally, so calling AddFoundryToolboxes
|
||||
// multiple times will not invoke StartAsync twice on the same singleton.
|
||||
services.AddHostedService(sp => sp.GetRequiredService<FoundryToolboxService>());
|
||||
|
||||
return services;
|
||||
|
||||
+185
@@ -0,0 +1,185 @@
|
||||
// Copyright (c) Microsoft. All rights reserved.
|
||||
|
||||
using System;
|
||||
using System.Net;
|
||||
using System.Net.Http;
|
||||
using System.Threading;
|
||||
using System.Threading.Tasks;
|
||||
using Azure.Core;
|
||||
using Microsoft.Agents.AI.Foundry.Hosting;
|
||||
using Moq;
|
||||
|
||||
namespace Microsoft.Agents.AI.Foundry.UnitTests.Hosting;
|
||||
|
||||
public class FoundryToolboxBearerTokenHandlerTests
|
||||
{
|
||||
private const string FakeToken = "test-bearer-token";
|
||||
|
||||
private static Mock<TokenCredential> CreateMockCredential()
|
||||
{
|
||||
var mock = new Mock<TokenCredential>();
|
||||
mock.Setup(c => c.GetTokenAsync(It.IsAny<TokenRequestContext>(), It.IsAny<CancellationToken>()))
|
||||
.ReturnsAsync(new AccessToken(FakeToken, DateTimeOffset.UtcNow.AddHours(1)));
|
||||
return mock;
|
||||
}
|
||||
|
||||
private static (FoundryToolboxBearerTokenHandler Handler, CountingHandler Inner) CreateHandlerPair(
|
||||
Mock<TokenCredential>? credential = null,
|
||||
string? featuresHeader = null,
|
||||
HttpStatusCode statusCode = HttpStatusCode.OK)
|
||||
{
|
||||
credential ??= CreateMockCredential();
|
||||
var inner = new CountingHandler(statusCode);
|
||||
var handler = new FoundryToolboxBearerTokenHandler(credential.Object, featuresHeader)
|
||||
{
|
||||
InnerHandler = inner
|
||||
};
|
||||
return (handler, inner);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SendAsync_InjectsBearerTokenAsync()
|
||||
{
|
||||
var (handler, _) = CreateHandlerPair();
|
||||
using var invoker = new HttpMessageInvoker(handler);
|
||||
|
||||
using var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com/api");
|
||||
using var response = await invoker.SendAsync(request, CancellationToken.None);
|
||||
|
||||
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
|
||||
Assert.Equal("Bearer", request.Headers.Authorization?.Scheme);
|
||||
Assert.Equal(FakeToken, request.Headers.Authorization?.Parameter);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SendAsync_InjectsFoundryFeaturesHeaderAsync()
|
||||
{
|
||||
var (handler, _) = CreateHandlerPair(featuresHeader: "feature1,feature2");
|
||||
using var invoker = new HttpMessageInvoker(handler);
|
||||
|
||||
using var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com/api");
|
||||
using var response = await invoker.SendAsync(request, CancellationToken.None);
|
||||
|
||||
Assert.True(request.Headers.TryGetValues("Foundry-Features", out var values));
|
||||
Assert.Contains("feature1,feature2", values);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SendAsync_OmitsFeaturesHeaderWhenNullAsync()
|
||||
{
|
||||
var (handler, _) = CreateHandlerPair(featuresHeader: null);
|
||||
using var invoker = new HttpMessageInvoker(handler);
|
||||
|
||||
using var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com/api");
|
||||
using var response = await invoker.SendAsync(request, CancellationToken.None);
|
||||
|
||||
Assert.False(request.Headers.Contains("Foundry-Features"));
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(HttpStatusCode.OK)]
|
||||
[InlineData(HttpStatusCode.Created)]
|
||||
[InlineData(HttpStatusCode.BadRequest)]
|
||||
[InlineData(HttpStatusCode.NotFound)]
|
||||
public async Task SendAsync_NonRetryableStatusCode_ReturnsImmediatelyAsync(HttpStatusCode statusCode)
|
||||
{
|
||||
var (handler, inner) = CreateHandlerPair(statusCode: statusCode);
|
||||
using var invoker = new HttpMessageInvoker(handler);
|
||||
|
||||
using var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com/api");
|
||||
using var response = await invoker.SendAsync(request, CancellationToken.None);
|
||||
|
||||
Assert.Equal(statusCode, response.StatusCode);
|
||||
Assert.Equal(1, inner.CallCount);
|
||||
}
|
||||
|
||||
[Theory]
|
||||
[InlineData(HttpStatusCode.TooManyRequests)]
|
||||
[InlineData(HttpStatusCode.InternalServerError)]
|
||||
[InlineData(HttpStatusCode.BadGateway)]
|
||||
[InlineData(HttpStatusCode.ServiceUnavailable)]
|
||||
public async Task SendAsync_RetryableStatusCode_RetriesMaxTimesAsync(HttpStatusCode statusCode)
|
||||
{
|
||||
var (handler, inner) = CreateHandlerPair(statusCode: statusCode);
|
||||
using var invoker = new HttpMessageInvoker(handler);
|
||||
|
||||
using var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com/api");
|
||||
using var response = await invoker.SendAsync(request, CancellationToken.None);
|
||||
|
||||
// MaxRetries is 3, so exactly 3 total attempts (not 4).
|
||||
Assert.Equal(3, inner.CallCount);
|
||||
Assert.Equal(statusCode, response.StatusCode);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SendAsync_RetryableStatusCode_SucceedsOnSecondAttemptAsync()
|
||||
{
|
||||
// First call returns 503, second returns 200.
|
||||
var inner = new SequenceHandler(
|
||||
HttpStatusCode.ServiceUnavailable,
|
||||
HttpStatusCode.OK);
|
||||
|
||||
var handler = new FoundryToolboxBearerTokenHandler(CreateMockCredential().Object, null)
|
||||
{
|
||||
InnerHandler = inner
|
||||
};
|
||||
using var invoker = new HttpMessageInvoker(handler);
|
||||
|
||||
using var request = new HttpRequestMessage(HttpMethod.Get, "https://example.com/api");
|
||||
using var response = await invoker.SendAsync(request, CancellationToken.None);
|
||||
|
||||
Assert.Equal(HttpStatusCode.OK, response.StatusCode);
|
||||
Assert.Equal(2, inner.CallCount);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A test handler that always returns the configured status code and counts how many times it was called.
|
||||
/// </summary>
|
||||
private sealed class CountingHandler : HttpMessageHandler
|
||||
{
|
||||
private readonly HttpStatusCode _statusCode;
|
||||
private int _callCount;
|
||||
|
||||
public int CallCount => this._callCount;
|
||||
|
||||
public CountingHandler(HttpStatusCode statusCode)
|
||||
{
|
||||
this._statusCode = statusCode;
|
||||
}
|
||||
|
||||
protected override Task<HttpResponseMessage> SendAsync(
|
||||
HttpRequestMessage request,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
Interlocked.Increment(ref this._callCount);
|
||||
return Task.FromResult(new HttpResponseMessage(this._statusCode));
|
||||
}
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// A test handler that returns status codes from a sequence, cycling through them.
|
||||
/// </summary>
|
||||
private sealed class SequenceHandler : HttpMessageHandler
|
||||
{
|
||||
private readonly HttpStatusCode[] _statusCodes;
|
||||
private int _callCount;
|
||||
|
||||
public int CallCount => this._callCount;
|
||||
|
||||
public SequenceHandler(params HttpStatusCode[] statusCodes)
|
||||
{
|
||||
this._statusCodes = statusCodes;
|
||||
}
|
||||
|
||||
protected override Task<HttpResponseMessage> SendAsync(
|
||||
HttpRequestMessage request,
|
||||
CancellationToken cancellationToken)
|
||||
{
|
||||
var index = Interlocked.Increment(ref this._callCount) - 1;
|
||||
var statusCode = index < this._statusCodes.Length
|
||||
? this._statusCodes[index]
|
||||
: this._statusCodes[^1];
|
||||
return Task.FromResult(new HttpResponseMessage(statusCode));
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -668,4 +668,100 @@ public class InputConverterTests
|
||||
// Model from the request is intentionally NOT propagated — the hosted agent uses its own model.
|
||||
Assert.Null(options.ModelId);
|
||||
}
|
||||
|
||||
// ── ReadMcpToolboxMarkers tests ──────────────────────────────────────────────
|
||||
|
||||
[Fact]
|
||||
public void ReadMcpToolboxMarkers_NullTools_ReturnsEmpty()
|
||||
{
|
||||
var request = new CreateResponse();
|
||||
// Tools defaults to null when not set via JSON deserialization.
|
||||
|
||||
var markers = InputConverter.ReadMcpToolboxMarkers(request);
|
||||
|
||||
Assert.Empty(markers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReadMcpToolboxMarkers_McpToolWithToolboxAddress_ReturnsMarker()
|
||||
{
|
||||
var request = new CreateResponse();
|
||||
request.Tools.Add(new MCPTool("test-toolbox")
|
||||
{
|
||||
ServerUrl = new Uri("foundry-toolbox://my-toolbox")
|
||||
});
|
||||
|
||||
var markers = InputConverter.ReadMcpToolboxMarkers(request);
|
||||
|
||||
Assert.Single(markers);
|
||||
Assert.Equal("my-toolbox", markers[0].Name);
|
||||
Assert.Null(markers[0].Version);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReadMcpToolboxMarkers_McpToolWithVersionedAddress_ReturnsNameAndVersion()
|
||||
{
|
||||
var request = new CreateResponse();
|
||||
request.Tools.Add(new MCPTool("test-toolbox")
|
||||
{
|
||||
ServerUrl = new Uri("foundry-toolbox://my-toolbox?version=v3")
|
||||
});
|
||||
|
||||
var markers = InputConverter.ReadMcpToolboxMarkers(request);
|
||||
|
||||
Assert.Single(markers);
|
||||
Assert.Equal("my-toolbox", markers[0].Name);
|
||||
Assert.Equal("v3", markers[0].Version);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReadMcpToolboxMarkers_McpToolWithNonToolboxUrl_SkipsIt()
|
||||
{
|
||||
var request = new CreateResponse();
|
||||
request.Tools.Add(new MCPTool("external-mcp")
|
||||
{
|
||||
ServerUrl = new Uri("https://example.com/mcp")
|
||||
});
|
||||
|
||||
var markers = InputConverter.ReadMcpToolboxMarkers(request);
|
||||
|
||||
Assert.Empty(markers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReadMcpToolboxMarkers_McpToolWithNullServerUrl_SkipsIt()
|
||||
{
|
||||
var request = new CreateResponse();
|
||||
request.Tools.Add(new MCPTool("test") { ServerUrl = null });
|
||||
|
||||
var markers = InputConverter.ReadMcpToolboxMarkers(request);
|
||||
|
||||
Assert.Empty(markers);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void ReadMcpToolboxMarkers_MixedTools_ReturnsOnlyToolboxMarkers()
|
||||
{
|
||||
var request = new CreateResponse();
|
||||
request.Tools.Add(new MCPTool("external")
|
||||
{
|
||||
ServerUrl = new Uri("https://example.com/mcp")
|
||||
});
|
||||
request.Tools.Add(new MCPTool("toolbox-1")
|
||||
{
|
||||
ServerUrl = new Uri("foundry-toolbox://box-a")
|
||||
});
|
||||
request.Tools.Add(new MCPTool("toolbox-2")
|
||||
{
|
||||
ServerUrl = new Uri("foundry-toolbox://box-b?version=2025-01")
|
||||
});
|
||||
|
||||
var markers = InputConverter.ReadMcpToolboxMarkers(request);
|
||||
|
||||
Assert.Equal(2, markers.Count);
|
||||
Assert.Equal("box-a", markers[0].Name);
|
||||
Assert.Null(markers[0].Version);
|
||||
Assert.Equal("box-b", markers[1].Name);
|
||||
Assert.Equal("2025-01", markers[1].Version);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user