feat(zendesk): enhance test coverage and retry logic

Added extensive unit tests across various components, including health checks, client instrumentation, and tool invocations. Achieved increased reliability by incorporating retry logic for rate-limited API responses.

Author: winromulus

src/ES.FX.Ignite.Zendesk/ES.FX.Ignite.Zendesk.csproj

@@ -11,6 +11,10 @@
 		<PackageReference Include="OpenTelemetry.Extensions.Hosting" />
 	</ItemGroup>
 
+	<ItemGroup>
+		<InternalsVisibleTo Include="ES.FX.Ignite.Zendesk.Tests" />
+	</ItemGroup>
+
 	<ItemGroup>
 		<ProjectReference Include="..\ES.FX.Ignite.Spark\ES.FX.Ignite.Spark.csproj" />
 		<ProjectReference Include="..\ES.FX.Zendesk\ES.FX.Zendesk.csproj" />

src/ES.FX.Zendesk.MCP.Host/Tools/ZendeskToolInvoker.cs

@@ -13,7 +13,9 @@ namespace ES.FX.Zendesk.MCP.Host.Tools;
 ///     status code and the Zendesk error body carried by <see cref="ZendeskApiException" />. Routing tool calls
 ///     through here re-throws a <see cref="ZendeskApiException" /> as an <see cref="McpException" />, whose message
 ///     the SDK surfaces verbatim, so the agent can distinguish (for example) <c>404 Not Found</c> from
-///     <c>403 Forbidden</c> from <c>422</c> and self-correct. Only the typed <see cref="ZendeskApiException" /> is
+///     <c>403 Forbidden</c> from <c>422</c> and self-correct. When the exception carries a
+///     <see cref="ZendeskApiException.RetryAfter" /> delay (typically a <c>429</c>), the message includes an
+///     explicit <c>"Retry after N seconds."</c> hint. Only the typed <see cref="ZendeskApiException" /> is
 ///     translated; other exceptions keep their default (generic) SDK handling.
 /// </remarks>
 internal static class ZendeskToolInvoker
@@ -86,6 +88,8 @@ private static string Describe(ZendeskApiException exception)
     {
         var message =
             $"The Zendesk API request failed with status {(int)exception.StatusCode} ({exception.StatusCode}).";
+        if (exception.RetryAfter is { } retryAfter)
+            message = $"{message} Retry after {(long)Math.Ceiling(retryAfter.TotalSeconds)} seconds.";
         return string.IsNullOrWhiteSpace(exception.ResponseBody)
             ? message
             : $"{message} Zendesk response: {exception.ResponseBody}";

tests/ES.FX.Ignite.Zendesk.Tests/HealthChecks/ZendeskClientHealthCheckTests.cs

@@ -0,0 +1,136 @@
+using System.Net;
+using System.Text;
+using ES.FX.Ignite.Zendesk.HealthChecks;
+using ES.FX.Zendesk.Abstractions;
+using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Diagnostics.HealthChecks;
+
+namespace ES.FX.Ignite.Zendesk.Tests.HealthChecks;
+
+public class ZendeskClientHealthCheckTests
+{
+    private const string AccessToken = "stub-access-token";
+
+    private const string TokenJson =
+        $$"""
+          { "access_token": "{{AccessToken}}", "token_type": "bearer", "expires_in": 3600 }
+          """;
+
+    private const string CurrentUserJson =
+        """
+        {
+          "user": {
+            "id": 12345,
+            "name": "Support Bot",
+            "email": "bot@unit-test.example",
+            "role": "admin"
+          }
+        }
+        """;
+
+    private static ServiceProvider BuildProvider(HttpMessageHandler handler)
+    {
+        var services = new ServiceCollection();
+        // Every factory-created client — including the dedicated token client — gets the stub as its
+        // primary handler, so no request in these tests can reach a real Zendesk host.
+        services.ConfigureHttpClientDefaults(b => b.ConfigurePrimaryHttpMessageHandler(() => handler));
+        services.AddZendeskClient(options =>
+        {
+            options.Subdomain = "unit-test-offline";
+            options.OAuth.ClientId = "cid";
+            options.OAuth.ClientSecret = "unit-test-client-secret";
+        });
+        return services.BuildServiceProvider();
+    }
+
+    private static HealthCheckContext CreateContext(IHealthCheck healthCheck, HealthStatus failureStatus) => new()
+    {
+        // A NON-default failure status, so a regression that hardcodes Unhealthy fails the assertion.
+        Registration = new HealthCheckRegistration("ZendeskClient", healthCheck, failureStatus, null)
+    };
+
+    [Fact]
+    public async Task CheckHealthAsync_Calls_The_Authenticated_Users_Me_Endpoint_And_Reports_Healthy()
+    {
+        var stub = new RoutingStubHandler();
+        await using var provider = BuildProvider(stub);
+        var healthCheck = new ZendeskClientHealthCheck(provider.GetRequiredService<IZendeskClient>());
+
+        var result = await healthCheck.CheckHealthAsync(CreateContext(healthCheck, HealthStatus.Degraded),
+            TestContext.Current.CancellationToken);
+
+        // Exactly two outbound requests, both intercepted by the stub: the token fetch, then the probe.
+        Assert.Equal(2, stub.Requests.Count);
+        Assert.Equal("/oauth/tokens", stub.TokenRequest?.RequestUri?.AbsolutePath);
+        // The dedicated token client has no auth handler attached, so the token request is unauthenticated.
+        Assert.Null(stub.TokenRequest?.Headers.Authorization);
+        Assert.Equal("/api/v2/users/me.json", stub.UsersMeRequest?.RequestUri?.AbsolutePath);
+        Assert.Equal("Bearer", stub.UsersMeRequest?.Headers.Authorization?.Scheme);
+        Assert.Equal(AccessToken, stub.UsersMeRequest?.Headers.Authorization?.Parameter);
+
+        Assert.Equal(HealthStatus.Healthy, result.Status);
+        Assert.Contains("12345", result.Description);
+        Assert.Contains("admin", result.Description);
+        // The description can surface on an unauthenticated /health endpoint — no PII, no secrets.
+        Assert.DoesNotContain("bot@unit-test.example", result.Description);
+        Assert.DoesNotContain("unit-test-client-secret", result.Description);
+        Assert.DoesNotContain(AccessToken, result.Description);
+    }
+
+    [Fact]
+    public async Task CheckHealthAsync_On_Failure_Returns_The_Registration_FailureStatus_With_The_Exception()
+    {
+        var stub = new RoutingStubHandler { UsersMeStatusCode = HttpStatusCode.InternalServerError };
+        await using var provider = BuildProvider(stub);
+        var healthCheck = new ZendeskClientHealthCheck(provider.GetRequiredService<IZendeskClient>());
+
+        var result = await healthCheck.CheckHealthAsync(CreateContext(healthCheck, HealthStatus.Degraded),
+            TestContext.Current.CancellationToken);
+
+        // Must honor context.Registration.FailureStatus, not hardcode Unhealthy.
+        Assert.Equal(HealthStatus.Degraded, result.Status);
+        Assert.NotNull(result.Exception);
+    }
+
+    /// <summary>
+    ///     Serves the OAuth token endpoint and <c>users/me.json</c> with canned responses, records the requests
+    ///     for assertions, and fails the test on any other outbound request.
+    /// </summary>
+    private sealed class RoutingStubHandler : HttpMessageHandler
+    {
+        public List<HttpRequestMessage> Requests { get; } = [];
+
+        public HttpRequestMessage? TokenRequest { get; private set; }
+
+        public HttpRequestMessage? UsersMeRequest { get; private set; }
+
+        public HttpStatusCode UsersMeStatusCode { get; init; } = HttpStatusCode.OK;
+
+        protected override Task<HttpResponseMessage> SendAsync(HttpRequestMessage request,
+            CancellationToken cancellationToken)
+        {
+            Requests.Add(request);
+            switch (request.RequestUri?.AbsolutePath)
+            {
+                case "/oauth/tokens":
+                    TokenRequest = request;
+                    return Task.FromResult(Json(HttpStatusCode.OK, TokenJson));
+                case "/api/v2/users/me.json":
+                    UsersMeRequest = request;
+                    return Task.FromResult(Json(UsersMeStatusCode,
+                        UsersMeStatusCode == HttpStatusCode.OK
+                            ? CurrentUserJson
+                            : """{ "error": "InternalServerError" }"""));
+                default:
+                    throw new InvalidOperationException(
+                        $"Unexpected outbound request {request.Method} {request.RequestUri} — " +
+                        "health check tests must never leave the stub.");
+            }
+        }
+
+        private static HttpResponseMessage Json(HttpStatusCode statusCode, string body) => new(statusCode)
+        {
+            Content = new StringContent(body, Encoding.UTF8, "application/json")
+        };
+    }
+}

tests/ES.FX.Ignite.Zendesk.Tests/HostingTests.cs

@@ -9,6 +9,7 @@
 using Microsoft.Extensions.Diagnostics.HealthChecks;
 using Microsoft.Extensions.Hosting;
 using Microsoft.Extensions.Options;
+using OpenTelemetry.Trace;
 
 namespace ES.FX.Ignite.Zendesk.Tests;
 
@@ -80,6 +81,49 @@ public void HealthCheck_Can_Be_Disabled_Via_Settings()
         Assert.DoesNotContain(registrations, registration => registration.Name == ZendeskClientSpark.Name);
     }
 
+    [Fact]
+    public void Binds_Settings_From_Configuration()
+    {
+        var builder = CreateBuilder(
+            Setting($"{SparkConfig.Settings}:HealthChecks:Enabled", "false"),
+            Setting($"{SparkConfig.Settings}:Tracing:Enabled", "false"));
+        builder.IgniteZendeskClient();
+
+        var app = builder.Build();
+
+        var settings = app.Services.GetRequiredService<ZendeskClientSparkSettings>();
+        Assert.False(settings.HealthChecks.Enabled);
+        Assert.False(settings.Tracing.Enabled);
+    }
+
+    [Fact]
+    public void ConfigureSettings_Delegate_Overrides_Configuration()
+    {
+        var builder = CreateBuilder(Setting($"{SparkConfig.Settings}:HealthChecks:Enabled", "false"));
+        builder.IgniteZendeskClient(configureSettings: settings => settings.HealthChecks.Enabled = true);
+
+        var app = builder.Build();
+
+        var settings = app.Services.GetRequiredService<ZendeskClientSparkSettings>();
+        Assert.True(settings.HealthChecks.Enabled);
+
+        var registrations = app.Services.GetRequiredService<IOptions<HealthCheckServiceOptions>>().Value.Registrations;
+        Assert.Contains(registrations, registration => registration.Name == ZendeskClientSpark.Name);
+    }
+
+    [Fact]
+    public void ConfigureOptions_Delegate_Overrides_Configuration()
+    {
+        var builder = CreateBuilder(ValidCredentials());
+        builder.IgniteZendeskClient(configureOptions: options => options.Subdomain = "overridden");
+
+        var app = builder.Build();
+
+        var options = app.Services.GetRequiredService<IOptions<ZendeskClientOptions>>().Value;
+        Assert.Equal("overridden", options.Subdomain);
+        Assert.Equal("cid", options.OAuth.ClientId);
+    }
+
     [Fact]
     public void Does_Not_Allow_Reconfiguration_Of_Same_Instance()
     {
@@ -89,6 +133,15 @@ public void Does_Not_Allow_Reconfiguration_Of_Same_Instance()
         Assert.Throws<ReconfigurationNotSupportedException>(() => builder.IgniteZendeskClient());
     }
 
+    [Fact]
+    public void Does_Not_Allow_Reconfiguration_Of_Same_ServiceKey()
+    {
+        var builder = CreateBuilder(ValidCredentials());
+        builder.IgniteZendeskClient(serviceKey: "a");
+
+        Assert.Throws<ReconfigurationNotSupportedException>(() => builder.IgniteZendeskClient(serviceKey: "a"));
+    }
+
     [Fact]
     public void Supports_Multiple_Named_Keyed_Instances()
     {
@@ -106,6 +159,7 @@ public void Supports_Multiple_Named_Keyed_Instances()
 
         Assert.NotNull(app.Services.GetRequiredService<IZendeskClient>());
         Assert.NotNull(app.Services.GetRequiredKeyedService<IZendeskClient>("sandbox"));
+        Assert.NotNull(app.Services.GetRequiredKeyedService<ZendeskClientSparkSettings>("sandbox"));
 
         var monitor = app.Services.GetRequiredService<IOptionsMonitor<ZendeskClientOptions>>();
         Assert.Equal("acme", monitor.Get(string.Empty).Subdomain);
@@ -128,4 +182,30 @@ public void Different_ServiceKeys_Do_Not_Conflict()
         Assert.NotNull(app.Services.GetRequiredKeyedService<IZendeskClient>("a"));
         Assert.NotNull(app.Services.GetRequiredKeyedService<IZendeskClient>("b"));
     }
+
+    [Fact]
+    public void Tracing_Enabled_Registers_TracerProvider()
+    {
+        var builder = CreateBuilder(ValidCredentials());
+        builder.IgniteZendeskClient();
+
+        var app = builder.Build();
+
+        // When tracing is enabled the Spark calls AddOpenTelemetry().WithTracing(...), which registers a
+        // TracerProvider in the container. Resolving it confirms the tracing branch ran.
+        Assert.NotNull(app.Services.GetService<TracerProvider>());
+    }
+
+    [Fact]
+    public void Tracing_Can_Be_Disabled_Via_Settings()
+    {
+        var builder = CreateBuilder(Setting($"{SparkConfig.Settings}:Tracing:Enabled", "false"));
+        builder.IgniteZendeskClient();
+
+        var app = builder.Build();
+
+        // With tracing disabled the Spark never calls AddOpenTelemetry().WithTracing(...), so no
+        // TracerProvider is registered.
+        Assert.Null(app.Services.GetService<TracerProvider>());
+    }
 }

tests/ES.FX.NousResearch.HermesAgent.Tests/Instrumentation/HermesAgentClientInstrumentationTests.cs

@@ -0,0 +1,82 @@
+using System.Collections.Concurrent;
+using System.Diagnostics;
+using System.Net;
+using ES.FX.NousResearch.HermesAgent.Server;
+using ES.FX.NousResearch.HermesAgent.Tests.Testing;
+using Microsoft.Extensions.Logging.Abstractions;
+
+namespace ES.FX.NousResearch.HermesAgent.Tests.Instrumentation;
+
+/// <summary>
+///     Behavioral coverage of the client's <see cref="Activity" /> emission
+///     (<see cref="HermesAgentClientInstrumentation" />): one Client-kind span per operation named
+///     <c>HermesAgent.{Area}.{Operation}</c> with the <c>hermesagent.operation</c> tag, <c>Ok</c> on success
+///     and <c>Error</c> (with the exception attached) on failure.
+/// </summary>
+public class HermesAgentClientInstrumentationTests
+{
+    private static HermesAgentServerApi CreateApi(HttpMessageHandler handler) =>
+        new(new HttpClient(handler) { BaseAddress = new Uri("http://localhost:8642/") },
+            NullLogger<HermesAgentServerApi>.Instance);
+
+    /// <summary>
+    ///     Subscribes to the client's <see cref="ActivitySource" /> and collects stopped activities. The source
+    ///     is process-global and other tests may emit on it concurrently, so callers isolate their own spans by
+    ///     filtering on a parent activity they start themselves.
+    /// </summary>
+    private static ActivityListener Subscribe(ConcurrentQueue<Activity> stopped)
+    {
+        var listener = new ActivityListener
+        {
+            ShouldListenTo = source => source.Name == HermesAgentClientInstrumentation.ActivitySourceName,
+            Sample = (ref ActivityCreationOptions<ActivityContext> _) =>
+                ActivitySamplingResult.AllDataAndRecorded,
+            ActivityStopped = stopped.Enqueue
+        };
+        ActivitySource.AddActivityListener(listener);
+        return listener;
+    }
+
+    [Fact]
+    public void ActivitySourceName_Is_Pinned_To_The_Documented_Value()
+    {
+        // OpenTelemetry subscriptions (the Ignite Spark, user AddSource calls) reference this exact string —
+        // a rename is a silent tracing outage, so pin the literal.
+        Assert.Equal("ES.FX.NousResearch.HermesAgent", HermesAgentClientInstrumentation.ActivitySourceName);
+    }
+
+    [Fact]
+    public async Task Successful_Operation_Emits_One_Client_Activity_With_Operation_Tag_And_Ok_Status()
+    {
+        var stopped = new ConcurrentQueue<Activity>();
+        using var listener = Subscribe(stopped);
+        using var parent = new Activity("test-parent").Start();
+
+        var api = CreateApi(new StubHttpMessageHandler("""{ "status": "healthy" }"""));
+        await api.GetHealthAsync(TestContext.Current.CancellationToken);
+
+        var activity = Assert.Single(stopped, a => a.ParentId == parent.Id);
+        Assert.Equal("HermesAgent.Server.GetHealth", activity.OperationName); // HermesAgent.{Area}.{Operation}
+        Assert.Equal(ActivityKind.Client, activity.Kind);
+        Assert.Equal("HermesAgent.Server.GetHealth", activity.GetTagItem("hermesagent.operation"));
+        Assert.Equal(ActivityStatusCode.Ok, activity.Status);
+    }
+
+    [Fact]
+    public async Task Failing_Operation_Sets_Error_Status_And_Attaches_The_Exception()
+    {
+        var stopped = new ConcurrentQueue<Activity>();
+        using var listener = Subscribe(stopped);
+        using var parent = new Activity("test-parent").Start();
+
+        var api = CreateApi(new StubHttpMessageHandler("""{ "error": "boom" }""",
+            HttpStatusCode.InternalServerError));
+        await Assert.ThrowsAsync<HermesAgentApiException>(async () =>
+            await api.GetHealthAsync(TestContext.Current.CancellationToken));
+
+        var activity = Assert.Single(stopped, a => a.ParentId == parent.Id);
+        Assert.Equal(ActivityStatusCode.Error, activity.Status);
+        Assert.False(string.IsNullOrEmpty(activity.StatusDescription));
+        Assert.Contains(activity.Events, e => e.Name == "exception"); // Activity.AddException event
+    }
+}