fix: report a workflow action that the GitHub API refused as failed [patch]

[matt-edmondson]

Fixes #286

What was wrong

MakeGitHubRequestAsync absorbs the failures this provider knows how to handle — AuthorizationException, a 403 for rate limiting, a 403 for authorization, a 429, and a connection error. It sets the status and logs, but does not rethrow. That is right for a polling update, which should not tear down over a rate limit.

RerunWorkflowAsync, CancelWorkflowAsync and TriggerWorkflowAsync each read reaching the next line as success:

await MakeGitHubRequestAsync(...);
return true;

Their own catch (NotFoundException) / catch (ApiException) blocks never fired, because those exceptions had already been swallowed one level down.

So clicking "Cancel Running Workflow" while rate limited, or just after the token was revoked, returned true. ExecuteGitHubApiAction took that as success and force-refreshed the build — the UI told the user the cancel had worked while nothing had happened server-side.

The fix

MakeGitHubRequestAsync now answers whether the request completed, and the three actions return that answer.

An unhandled ApiException status still propagates exactly as before, so a caller that genuinely cannot continue is not quietly handed a false instead — AnUnhandledStatusStillPropagates pins that.

The other five callers ignore the result, as they did before. They are polling updates (DiscoverOwnersAsync, UpdateRepositoriesAsync and siblings) that already surface a failure through the provider status, and widening them is not what this issue describes.

The second commit: one workflow action, not three

The guard and both catch blocks were copied between all three methods, and all three made the same mistake with the result. RunWorkflowActionAsync is that shape once, returning what the request returned.

It takes the API call as a delegate, which is what makes the path testable at all — four further tests drive a refused request, a successful one, a NotFound, and the no-credentials guard, none of them reaching the network. The owner carries its own token, which gets past the credential guard without the provider needing one. That path had no coverage before.

Tests

BuildMonitor.Test/GitHubRequestOutcomeTests.cs.

The request body is a Func<Task> the caller supplies, so each failure is injected directly and the real method is driven — no rule had to be lifted out of it, and the assertions cover both halves of each case: the returned answer and the provider status the user actually sees (RateLimited, AuthFailed, Error).

Octokit's own Response is internal, so the tests use a small IResponse fake carrying the status code and headers, which is all IsRateLimitResponse and ParseRateLimitResetTime read.

Verified by substituting the original behaviour back in (every swallowed failure reporting success, as await …; return true; did): ARateLimited403ReportsFailure, APlain403ReportsFailure, A429ReportsFailure, AnAuthorizationExceptionReportsFailure and AConnectionErrorReportsFailure all fail against it and pass against the fix.

Full suite on this branch: 43 total, 43 passed, 0 failed. dotnet build --configuration Release is clean. Tests pass on Windows, macOS and Ubuntu.

Known: the coverage gate is red, and needs a decision

SonarCloud fails this PR on Coverage on New Code at 66.7%, against a required 80%. The refactor above took it from 57.1% to 66.7%, and 66.7% is the ceiling without changes I do not think belong in this PR.

The three remaining uncovered lines are one per method, each nothing but the Octokit call construction. They cannot be kept out of the diff, because they are exactly the lines that must change to return the result rather than true. Covering them needs either a live request to api.github.com from a unit test, or an injectable client factory plus a fake IConnection.

I did not build that double: it is well outside what #286 describes, and it cuts against the approach CLAUDE.md sets out for this repository — ColumnStrideTests deliberately pulls the decidable part out into a plain method rather than faking the untestable layer underneath, and RunWorkflowActionAsync is that same move.

See the full note for the three ways out: accept on the new tests' merits, exclude the provider-to-Octokit glue from coverage, or land the injectable client factory as its own change.

🤖 Generated with Claude Code

https://claude.ai/code/session_01EdV5iCFkUxFqkLZQAGLAVT

[matt-edmondson]

SonarCloud Code Analysis is failing this PR on Coverage on New Code: 57.1% against a required 80%. Flagging what I have and have not been able to do about it.

What I did. I folded the three workflow actions into one RunWorkflowActionAsync. The guard and both catch blocks were copied between them, and all three made the same mistake with the result, so there is one copy now. More to the point, it takes the API call as a delegate, which makes the path testable without reaching the network — four new tests drive a refused request, a successful one, a NotFound, and the no-credentials guard. That path had no coverage before.

That takes new-code coverage from 57% to about 67%. Still under the gate.

What is left, and why I stopped. The three remaining uncovered lines are one per method, and each is nothing but the Octokit call construction:

internal async Task<bool> RerunWorkflowAsync(Run run) => await RunWorkflowActionAsync(run.Owner, $"…/rerun/{run.Id}", async () => await GitHubRuns.Rerun(…));

GitHubRuns resolves to CurrentClient!.Actions.Workflows.Runs, and MakeGitHubRequestAsync rebuilds CurrentClient from the owner's token on every call. Covering those lines needs one of:

1. a live request to api.github.com from a unit test — not acceptable; or
2. an injectable client factory on the provider plus a fake IConnection, so Runs.Rerun resolves against a double.

Option 2 is real work: a mutable test-only seam in production code, plus a fake implementing the whole of IConnection, plus constructing a full Run graph. I did not do it, for two reasons. It is well outside what #286 describes, and it cuts against the testing approach CLAUDE.md sets out for this repository — ColumnStrideTests deliberately pulls the decidable part out into a plain method rather than faking the untestable layer underneath, and RunWorkflowActionAsync is that same move. Building an Octokit double here would be the opposite.

I did not re-run the check: the gate is deterministic, so a re-run returns the same number.

Suggested resolutions, in the order I would pick them:

1. Accept the PR on the strength of what the new tests cover — the bug is in the result handling, and that is now fully covered.
2. Exclude the thin provider-to-Octokit glue from coverage, since no test can reach it without a double.
3. Land the injectable client factory as its own change, and let coverage here rise behind it.

Happy to do 3 as a separate PR if you want it — it would pay off across the provider, not just these three lines.

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate failed

Failed conditions
66.7% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud