fix(deployment): surface GitHub approval failures instead of a generic 500 - #1198
fix(deployment): surface GitHub approval failures instead of a generic 500#1198krusche wants to merge 1 commit into
Conversation
…c 500 When GitHub rejected an in-app deployment approval/decline, the service threw a ResponseStatusException(502, "GitHub rejected the request: ...") — but the GlobalExceptionHandler's generic Exception handler caught it first (advice handlers run before Spring's ResponseStatusExceptionResolver) and flattened it to a 500 "internal server error", discarding the status and message. Every ResponseStatusException across the app was affected. - Add a dedicated @ExceptionHandler(ResponseStatusException.class) that preserves the intended status + reason (and keeps these handled outcomes out of Sentry; 5xx logged at WARN, 4xx at DEBUG). - Introduce GitHubReviewException (extends IOException) carrying GitHub's HTTP status so the review path can react to a 401 (expired brokered token) with an actionable message telling the reviewer to sign out and back in, while still returning 502 to the client (not 401, which would bounce them to login). Root cause of the 401s themselves (expired Keycloak-brokered GitHub token, not refreshed) is tracked separately. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | ✅ 0 (≤ 20 complexity) |
🟢 Coverage 91.67% diff coverage · +0.28% coverage variation
Metric Results Coverage variation ✅ +0.28% coverage variation (-1.00%) Diff coverage ✅ 91.67% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (8afae1a) Report Missing Report Missing Report Missing Head commit (a38d148) 15993 (+104) 8626 (+101) 53.94% (+0.28%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#1198) 24 22 91.67% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%1 Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct.
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
Claudia-Anthropica
left a comment
Claudia-Anthropica
left a comment
There was a problem hiding this comment.
@krusche This cleanly preserves intentional response statuses and gives expired GitHub authorization failures an actionable message while keeping the client-facing status at 502. Nice focused tests around both the controller response and audit-row behavior; approving as-is.
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
|
we should still work on this and get it fixed: not stale |
Motivation
Clicking Approve/Decline on a pending deployment showed a red toast reading "Error: An internal server error occurred" — with no hint of what actually went wrong. On prod the real cause was a GitHub HTTP 401 (the deployer's Keycloak-brokered GitHub token had expired), which the service correctly turns into a
502 "GitHub rejected the request: … HTTP 401".That message never reached the user:
GlobalExceptionHandler's generic@ExceptionHandler({Exception.class, IOException.class})catchesResponseStatusExceptiontoo (advice handlers run before Spring'sResponseStatusExceptionResolver), so everyResponseStatusExceptionin the app was being flattened to a flat500 "internal server error", discarding both the intended status and the message.Prod log for the reported failure:
Changes
GlobalExceptionHandler: add a dedicated@ExceptionHandler(ResponseStatusException.class)that preserves the intended status + reason. These are deliberate, handled outcomes, so they're kept out of Sentry (5xx logged at WARN, 4xx at DEBUG). This fixes the misleading error for all endpoints that throwResponseStatusException, not just approvals.GitHubReviewException(new, extendsIOException): carries GitHub's HTTP status. The review path uses it to detect a 401 and return an actionable reason — "your GitHub authorization has expired. Please sign out of Helios and sign in again, then retry." — while still returning 502 to the client (returning 401 would make the client treat the user's Helios session as expired and bounce them to login).Scope / follow-up
This makes the failure legible; it does not stop the 401s. The underlying cause — Keycloak stores the GitHub token at login and never refreshes it, and GitHub App user tokens expire after 8h, so auto- and in-app approval both fail once the token is stale — is being addressed separately (Keycloak refresh-token integration).
Testing
DeploymentApprovalControllerErrorHandlingTest(new,@WebMvcTest): a service-thrown502reaches the client as502with its reason intact; an unexpected exception still falls back to500.DeploymentReviewActionServiceTest(new case): a GitHub401yields502with the actionable "expired / sign in again" message and still records aFAILED_AT_GITHUBaudit row.GitHubServiceTestfailure case unchanged (GitHubReviewExceptionis anIOExceptionwith the same message).🤖 Generated with Claude Code