From 10c140833f714086c00b42b17bf1de57e9ea84db Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:37:38 +0000 Subject: [PATCH 1/2] fix: report a workflow action that the GitHub API refused as failed [patch] MakeGitHubRequestAsync absorbs the failures this provider knows how to handle -- AuthorizationException, a 403 for rate limiting or for authorization, a 429, and a connection error. It sets the status and logs, but does not rethrow, which is right for a polling update that should not tear down over a rate limit. RerunWorkflowAsync, CancelWorkflowAsync and TriggerWorkflowAsync read reaching the next line as success and returned true unconditionally. Their own catch blocks never fired, because the exceptions had already been swallowed one level down. So cancelling a workflow while rate limited, or just after the token was revoked, returned true. ExecuteGitHubApiAction took that as success and force-refreshed the build, telling the user the cancel had worked while nothing had happened server-side. Have MakeGitHubRequestAsync answer whether the request completed, and have the three actions return that answer. An unhandled ApiException status still propagates, so a caller that genuinely cannot continue is not quietly handed a false instead. The other five callers ignore the result, as they did before: they are polling updates that already surface a failure through the provider status. Fixes #286 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- .../GitHubRequestOutcomeTests.cs | 142 ++++++++++++++++++ BuildMonitor/Providers/GitHub.cs | 33 +++- 2 files changed, 168 insertions(+), 7 deletions(-) create mode 100644 BuildMonitor.Test/GitHubRequestOutcomeTests.cs diff --git a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs new file mode 100644 index 0000000..aa6c260 --- /dev/null +++ b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs @@ -0,0 +1,142 @@ +// Copyright (c) 2023-2026 ktsu-dev contributors + +namespace ktsu.BuildMonitor.Test; + +using System; +using System.Collections.Generic; +using System.Net; +using System.Net.Http; +using System.Threading.Tasks; + +using Microsoft.VisualStudio.TestTools.UnitTesting; + +using Octokit; + +/// +/// Tests whether one GitHub API request reports having succeeded. +/// +/// +/// MakeGitHubRequestAsync absorbs the failures this provider knows how to handle: it sets the +/// status and logs, but does not rethrow. That is deliberate -- a polling update should not tear +/// down over a rate limit -- but it means reaching the line after the call says nothing about +/// whether the call happened. The three workflow actions assumed it did and returned +/// unconditionally, so cancelling a workflow while rate limited, or after the +/// token was revoked, reported success and force-refreshed the build while nothing had happened +/// server-side. +/// +/// The request body is a the caller supplies, so the failures can be +/// injected directly and the real method driven, rather than a rule being lifted out of it. +/// +[TestClass] +public sealed class GitHubRequestOutcomeTests +{ + private const string RequestName = "test/request"; + + /// + /// A response built to order. Octokit's own Response is internal, and the provider reads + /// only the status code and the headers off it. + /// + private sealed class FakeResponse(HttpStatusCode statusCode, IReadOnlyDictionary headers) : IResponse + { + public object Body => "{}"; + public IReadOnlyDictionary Headers { get; } = headers; + public ApiInfo ApiInfo => null!; + public HttpStatusCode StatusCode { get; } = statusCode; + public string ContentType => "application/json"; + } + + private static ApiException ApiFailure(HttpStatusCode statusCode, IReadOnlyDictionary? headers = null) => + new(new FakeResponse(statusCode, headers ?? new Dictionary())); + + private static Task RunFailing(GitHub provider, Exception failure) => + provider.MakeGitHubRequestAsync(RequestName, () => Task.FromException(failure)); + + [TestMethod] + public async Task ASuccessfulRequestReportsSuccess() + { + GitHub provider = new(); + + bool succeeded = await provider.MakeGitHubRequestAsync(RequestName, () => Task.CompletedTask).ConfigureAwait(false); + + Assert.IsTrue(succeeded); + Assert.AreEqual(ProviderStatus.OK, provider.Status); + } + + /// + /// The headline case from the issue: rate limited, so the request never reached GitHub. + /// + [TestMethod] + public async Task ARateLimited403ReportsFailure() + { + GitHub provider = new(); + ApiException rateLimited = ApiFailure( + HttpStatusCode.Forbidden, + new Dictionary { ["X-RateLimit-Remaining"] = "0" }); + + bool succeeded = await RunFailing(provider, rateLimited).ConfigureAwait(false); + + Assert.IsFalse(succeeded, "A request refused for rate limiting did not happen, and must not report success."); + Assert.AreEqual(ProviderStatus.RateLimited, provider.Status); + } + + /// + /// The other headline case: a plain 403, which is what a just-revoked token looks like. + /// + [TestMethod] + public async Task APlain403ReportsFailure() + { + GitHub provider = new(); + + bool succeeded = await RunFailing(provider, ApiFailure(HttpStatusCode.Forbidden)).ConfigureAwait(false); + + Assert.IsFalse(succeeded, "A request refused for lack of authorization did not happen, and must not report success."); + Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status); + } + + [TestMethod] + public async Task A429ReportsFailure() + { + GitHub provider = new(); + + bool succeeded = await RunFailing(provider, ApiFailure(HttpStatusCode.TooManyRequests)).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual(ProviderStatus.RateLimited, provider.Status); + } + + [TestMethod] + public async Task AnAuthorizationExceptionReportsFailure() + { + GitHub provider = new(); + AuthorizationException unauthorized = new(new FakeResponse(HttpStatusCode.Unauthorized, new Dictionary())); + + bool succeeded = await RunFailing(provider, unauthorized).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual(ProviderStatus.AuthFailed, provider.Status); + } + + [TestMethod] + public async Task AConnectionErrorReportsFailure() + { + GitHub provider = new(); + + bool succeeded = await RunFailing(provider, new HttpRequestException("connection reset")).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.AreEqual(ProviderStatus.Error, provider.Status); + } + + /// + /// A status this provider has no handling for still propagates, so a caller that genuinely + /// cannot continue is not quietly handed a instead. + /// + [TestMethod] + public async Task AnUnhandledStatusStillPropagates() + { + GitHub provider = new(); + + await Assert.ThrowsExactlyAsync( + () => RunFailing(provider, ApiFailure(HttpStatusCode.InternalServerError))).ConfigureAwait(false); + } +} diff --git a/BuildMonitor/Providers/GitHub.cs b/BuildMonitor/Providers/GitHub.cs index 8d5ffed..2208582 100644 --- a/BuildMonitor/Providers/GitHub.cs +++ b/BuildMonitor/Providers/GitHub.cs @@ -652,8 +652,25 @@ private static List ParseLogForErrors(string logs) return [.. errors.Take(10)]; } + /// + /// Runs one GitHub API request, handling the failures this provider knows how to absorb. + /// + /// The request name, used for logging and in-flight tracking. + /// The API call to make. + /// The owner whose token the call should use, if any. + /// + /// if the request completed, if it failed in one + /// of the ways handled here. An unhandled status still propagates. + /// + /// + /// The answer matters because the failures handled here are handled silently: the status is set + /// and the failure is logged, but nothing is rethrown. A caller that assumed reaching the next + /// line meant success therefore reported success for a request that never happened -- which is + /// what the workflow actions did, telling the user a re-run or cancel had worked while the + /// provider was rate limited or its token had just been revoked. + /// [System.Diagnostics.CodeAnalysis.SuppressMessage("Style", "IDE0010:Add missing cases", Justification = "")] - internal async Task MakeGitHubRequestAsync(string name, Func action, Owner? owner = null) + internal async Task MakeGitHubRequestAsync(string name, Func action, Owner? owner = null) { await RequestSemaphore.WaitAsync().ConfigureAwait(false); try @@ -679,11 +696,13 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, Owner UpdateRateLimitFromApiInfo(); ClearStatus(); + return true; } catch (AuthorizationException) { Log.Error($"{Name}: AuthorizationException for request '{name}'"); OnAuthenticationFailure(); + return false; } catch (ApiException e) { @@ -711,11 +730,14 @@ internal async Task MakeGitHubRequestAsync(string name, Func action, Owner Log.Error($"{Name}: ApiException ({e.HttpResponse?.StatusCode}) for request '{name}' - {e.Message}"); throw; } + + return false; } catch (HttpRequestException ex) { Log.Error($"{Name}: Connection error - {ex.Message}"); SetStatus(ProviderStatus.Error, $"{Strings.ConnectionErrorMessage} {ex.Message}"); + return false; } } finally @@ -789,8 +811,7 @@ internal async Task RerunWorkflowAsync(Run run) try { - await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/rerun/{run.Id}", async () => await GitHubRuns.Rerun(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false); - return true; + return await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/rerun/{run.Id}", async () => await GitHubRuns.Rerun(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false); } catch (NotFoundException) { @@ -816,8 +837,7 @@ internal async Task CancelWorkflowAsync(Run run) try { - await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/cancel/{run.Id}", async () => await GitHubRuns.Cancel(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false); - return true; + return await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/cancel/{run.Id}", async () => await GitHubRuns.Cancel(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false); } catch (NotFoundException) { @@ -844,7 +864,7 @@ internal async Task TriggerWorkflowAsync(Build build, BranchName branch) try { - await MakeGitHubRequestAsync($"{Name}/{build.Owner.Name}/{build.Repository.Name}/dispatch/{build.Name}", async () => + return await MakeGitHubRequestAsync($"{Name}/{build.Owner.Name}/{build.Repository.Name}/dispatch/{build.Name}", async () => { CreateWorkflowDispatch createWorkflowDispatch = new(branch) { @@ -852,7 +872,6 @@ await MakeGitHubRequestAsync($"{Name}/{build.Owner.Name}/{build.Repository.Name} }; await GitHubActions.Workflows.CreateDispatch(build.Owner.Name, build.Repository.Name, long.Parse(build.Id, CultureInfo.InvariantCulture), createWorkflowDispatch).ConfigureAwait(false); }, build.Owner).ConfigureAwait(false); - return true; } catch (NotFoundException) { From 08a35caa79d8bc43dac2d93aca9a6065688f042a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 21:49:23 +0000 Subject: [PATCH 2/2] refactor: fold the three workflow actions into one, and cover it [patch] The guard and the two catch blocks were copied between RerunWorkflowAsync, CancelWorkflowAsync and TriggerWorkflowAsync, and all three made the same mistake with the result. RunWorkflowActionAsync is that shape once, returning what the request returned. Taking the API call as a delegate is what makes the path testable at all: the four new tests drive a refused request, a successful one, a NotFound and the no-credentials guard without reaching the network. The owner carries its own token, which gets past the credential guard without the provider needing one. Coverage on new code goes from 57% to 67%. The three remaining uncovered lines are each method's single line of Octokit call construction, which cannot run without either a live request or an IConnection test double; see the note on the pull request. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT --- .../GitHubRequestOutcomeTests.cs | 75 ++++++++++++++++++ BuildMonitor/Providers/GitHub.cs | 78 ++++++------------- 2 files changed, 100 insertions(+), 53 deletions(-) diff --git a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs index aa6c260..005944c 100644 --- a/BuildMonitor.Test/GitHubRequestOutcomeTests.cs +++ b/BuildMonitor.Test/GitHubRequestOutcomeTests.cs @@ -127,6 +127,81 @@ public async Task AConnectionErrorReportsFailure() Assert.AreEqual(ProviderStatus.Error, provider.Status); } + private static Owner OwnerWithToken() => new() + { + Name = OwnerName.Create("alpha"), + Token = BuildProviderToken.Create("alpha-pat"), + }; + + /// + /// The workflow actions themselves: a refused request must come back as a failure, not as the + /// unconditional success they used to report. + /// + /// + /// The API call is a delegate, so the refusal is injected and nothing reaches the network. The + /// owner carries its own token, which is what gets past the credential guard without the + /// provider needing one. + /// + [TestMethod] + public async Task AWorkflowActionRefusedByTheApiReportsFailure() + { + GitHub provider = new(); + ApiException rateLimited = ApiFailure( + HttpStatusCode.Forbidden, + new Dictionary { ["X-RateLimit-Remaining"] = "0" }); + + bool succeeded = await provider.RunWorkflowActionAsync( + OwnerWithToken(), RequestName, () => Task.FromException(rateLimited)).ConfigureAwait(false); + + Assert.IsFalse(succeeded, "A cancel or re-run the API refused must not be reported as having worked."); + Assert.AreEqual(ProviderStatus.RateLimited, provider.Status); + } + + [TestMethod] + public async Task AWorkflowActionThatSucceedsReportsSuccess() + { + GitHub provider = new(); + + bool succeeded = await provider.RunWorkflowActionAsync( + OwnerWithToken(), RequestName, () => Task.CompletedTask).ConfigureAwait(false); + + Assert.IsTrue(succeeded); + } + + /// + /// A missing NotFound is still a failure, and is caught by the action rather than escaping. + /// + [TestMethod] + public async Task AWorkflowActionOnAMissingRunReportsFailure() + { + GitHub provider = new(); + NotFoundException missing = new(new FakeResponse(HttpStatusCode.NotFound, new Dictionary())); + + bool succeeded = await provider.RunWorkflowActionAsync( + OwnerWithToken(), RequestName, () => Task.FromException(missing)).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + } + + [TestMethod] + public async Task AWorkflowActionWithoutCredentialsReportsFailureWithoutCalling() + { + GitHub provider = new(); + bool called = false; + + bool succeeded = await provider.RunWorkflowActionAsync( + new Owner { Name = OwnerName.Create("beta") }, + RequestName, + () => + { + called = true; + return Task.CompletedTask; + }).ConfigureAwait(false); + + Assert.IsFalse(succeeded); + Assert.IsFalse(called, "With no credentials there is nothing to ask GitHub."); + } + /// /// A status this provider has no handling for still propagates, so a caller that genuinely /// cannot continue is not quietly handed a instead. diff --git a/BuildMonitor/Providers/GitHub.cs b/BuildMonitor/Providers/GitHub.cs index 2208582..b0dd2ab 100644 --- a/BuildMonitor/Providers/GitHub.cs +++ b/BuildMonitor/Providers/GitHub.cs @@ -798,20 +798,30 @@ private static bool IsRateLimitResponse(ApiException exception) } /// - /// Re-runs a workflow run. + /// Runs one workflow action and answers whether it actually happened. /// - /// The workflow run to re-run. - /// True if the operation was successful, false otherwise. - internal async Task RerunWorkflowAsync(Run run) + /// The owner whose credentials the action runs under. + /// The request name, used for logging and in-flight tracking. + /// The API call to make. + /// only if the request reached GitHub and succeeded. + /// + /// The three workflow actions had this guard and these catch blocks copied between them, and all + /// three read reaching the line after the request as success. They did not: the failures this + /// provider handles are swallowed by without rethrowing, so + /// the catch blocks never fired for them and the action reported a success that never happened. + /// One copy, returning what the request returned, is what keeps that answer honest -- and taking + /// the call as a delegate is what lets a test drive the refusal without reaching the network. + /// + internal async Task RunWorkflowActionAsync(Owner owner, string name, Func action) { - if (!HasValidCredentials(run.Owner)) + if (!HasValidCredentials(owner)) { return false; } try { - return await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/rerun/{run.Id}", async () => await GitHubRuns.Rerun(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false); + return await MakeGitHubRequestAsync(name, action, owner).ConfigureAwait(false); } catch (NotFoundException) { @@ -823,31 +833,19 @@ internal async Task RerunWorkflowAsync(Run run) } } + /// + /// Re-runs a workflow run. + /// + /// The workflow run to re-run. + /// True if the operation was successful, false otherwise. + internal async Task RerunWorkflowAsync(Run run) => await RunWorkflowActionAsync(run.Owner, $"{Name}/{run.Owner.Name}/{run.Repository.Name}/rerun/{run.Id}", async () => await GitHubRuns.Rerun(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false)).ConfigureAwait(false); + /// /// Cancels a running workflow. /// /// The workflow run to cancel. /// True if the operation was successful, false otherwise. - internal async Task CancelWorkflowAsync(Run run) - { - if (!HasValidCredentials(run.Owner)) - { - return false; - } - - try - { - return await MakeGitHubRequestAsync($"{Name}/{run.Owner.Name}/{run.Repository.Name}/cancel/{run.Id}", async () => await GitHubRuns.Cancel(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false), run.Owner).ConfigureAwait(false); - } - catch (NotFoundException) - { - return false; - } - catch (ApiException) - { - return false; - } - } + internal async Task CancelWorkflowAsync(Run run) => await RunWorkflowActionAsync(run.Owner, $"{Name}/{run.Owner.Name}/{run.Repository.Name}/cancel/{run.Id}", async () => await GitHubRuns.Cancel(run.Owner.Name, run.Repository.Name, long.Parse(run.Id, CultureInfo.InvariantCulture)).ConfigureAwait(false)).ConfigureAwait(false); /// /// Triggers a workflow dispatch event. @@ -855,31 +853,5 @@ internal async Task CancelWorkflowAsync(Run run) /// The workflow build to trigger. /// The branch to run the workflow on. /// True if the operation was successful, false otherwise. - internal async Task TriggerWorkflowAsync(Build build, BranchName branch) - { - if (!HasValidCredentials(build.Owner)) - { - return false; - } - - try - { - return await MakeGitHubRequestAsync($"{Name}/{build.Owner.Name}/{build.Repository.Name}/dispatch/{build.Name}", async () => - { - CreateWorkflowDispatch createWorkflowDispatch = new(branch) - { - Inputs = new Dictionary() - }; - await GitHubActions.Workflows.CreateDispatch(build.Owner.Name, build.Repository.Name, long.Parse(build.Id, CultureInfo.InvariantCulture), createWorkflowDispatch).ConfigureAwait(false); - }, build.Owner).ConfigureAwait(false); - } - catch (NotFoundException) - { - return false; - } - catch (ApiException) - { - return false; - } - } + internal async Task TriggerWorkflowAsync(Build build, BranchName branch) => await RunWorkflowActionAsync(build.Owner, $"{Name}/{build.Owner.Name}/{build.Repository.Name}/dispatch/{build.Name}", async () => await GitHubActions.Workflows.CreateDispatch(build.Owner.Name, build.Repository.Name, long.Parse(build.Id, CultureInfo.InvariantCulture), new CreateWorkflowDispatch(branch) { Inputs = new Dictionary() }).ConfigureAwait(false)).ConfigureAwait(false); }