scalesets: distinguish a GitHub 403 from a 401

[yhaliaw]

The problem

ScaleSetClient.Do maps both 401 and 403 onto the bare ErrUnauthorized sentinel:

case 401, 403:
    return nil, runnerErrors.ErrUnauthorized

GitHub answers 403 for a secondary rate limit and for SSO enforcement on an organization. Neither says anything about the credentials, and both clear on their own (or with an operator action that is not a credential rotation). Callers reading ErrUnauthorized therefore conclude the credentials are dead when they are usually fine and the refusal is temporary.

GARM does this to itself. In workers/scaleset/scaleset.go, a failed RemoveRunner is read as:

if errors.Is(err, runnerErrors.ErrUnauthorized) {
    // we don't have access to remove the runner. This implies that our
    // credentials may have expired or ar incorrect.
    // TODO(gabriel-samfira): we need to set the scale set as inactive and stop the listener (if any).

A secondary rate limit that would have cleared on the next pass is enough to reach that branch today, and enough to trigger the deactivation once the TODO is implemented.

It also affects API clients: handleError maps ErrUnauthorized to 401 and deliberately strips Details, so a consumer sees a bare 401 with no body for what may have been a transient 403.

The change

Return a ForbiddenError for 403, added to internal/errors alongside RunnerTransitionError and following its shape: it reports as a runnerErrors.UnauthorizedError via Is, exactly as RunnerTransitionError reports as a BadRequestError.

That keeps this backward compatible. Every existing caller that only asks "was this refused?" — scaleset.go, scaleset_listener.go, runner.go, pool.go — keeps working with no change. Callers that need to act on the difference can single it out with errors.Is(err, &internalErrors.ForbiddenError{}), checked before the broader unauthorized case.

handleError gains a ForbiddenError case mapping to HTTP 403, placed before the unauthorized case for that reason, so API clients can tell the two apart too. Response shape is unchanged (APIErrorResponse), and the swagger blocks document default: APIErrorResponse rather than enumerating statuses, so no docs or generated clients change.

Both branches now also keep the response body and URL, as every other status branch in Do already does. Previously they returned a bare sentinel, so an operator reading the log had nothing to tell a revoked credential from a rate limit.

Deliberately not included

Acting on the new distinction. scaleset.go's handler is the obvious consumer, but changing when a scale set gets parked or deactivated is a behaviour change that deserves its own PR — this one only makes the information available. Happy to follow up if you'd like it in the same change.

Notes

• I put ForbiddenError in internal/errors to match the existing precedent, which means external Go consumers of GARM as a library cannot reference the type (they get the HTTP 403 instead). If you would rather it were public, it could go to garm-provider-common/errors next to UnauthorizedError — happy to redo it that way.
• The Is ordering is load-bearing: because ForbiddenError reports as an UnauthorizedError, any switch/case chain that checks unauthorized first will swallow it. That is called out in the type's doc comment and in the handleError comment.

Testing

• make test equivalent (go test -race -mod=vendor -tags testing -timeout=60m -parallel=4 -count=1 ./...) passes.
• golangci-lint run --build-tags=testing,integration (v2.10.1, as pinned in the Makefile) reports 0 issues.
• New internal/errors/errors_test.go covers the identity and compatibility guarantee, including that it survives %w wrapping and that a plain 401 is not mistaken for a 403.
• New util/github/scalesets/client_test.go covers the status mapping through a real httptest server, that 403 stays ErrUnauthorized-compatible, that both refusals keep their body, and that 400/404/409 are unaffected.
• I verified the new tests fail against the old case 401, 403 behaviour rather than passing vacuously.