{{ message }}
Extract shared HTTP response body read helpers to httputil package#8346
Merged
Conversation
Deduplicate the read-body-and-check-status pattern that was independently implemented in three files: - internal/proxy/proxy.go (>= 400 check) - internal/githubhttp/collaborator.go (>= 400 check) - internal/oidc/provider.go (exact 200 check with body in error) Add two helpers in internal/httputil/response.go: - ReadResponseBody: reads body, closes it, errors on status >= 400 - ReadResponseBodyStrict: reads body, closes it, errors on status != expected (includes body in error message for diagnostics) Refactor all three call sites to use the shared helpers and update corresponding tests to match the new error message format. Fixes #8320 Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses issue #8320 by deduplicating repeated “read HTTP response body + close + status check” logic across the proxy, GitHub collaborator permission client, and OIDC provider, consolidating it into shared helpers under internal/httputil.
Changes:
- Added shared response body helpers:
ReadResponseBody(status >= 400) andReadResponseBodyStrict(exact expected status, includes body in error). - Refactored proxy, collaborator permission fetching, and OIDC token fetching to use the shared helpers.
- Added/updated unit tests to cover the new helpers and align existing tests with the new error formats.
Show a summary per file
| File | Description |
|---|---|
| internal/httputil/response.go | Introduces shared response-body read + status-check helpers. |
| internal/httputil/response_test.go | Adds unit coverage for both new helper variants and read-error cases. |
| internal/proxy/proxy.go | Switches REST enrichment call path to use the shared helper. |
| internal/githubhttp/collaborator.go | Switches collaborator permission fetch to use the shared helper and updates logging. |
| internal/githubhttp/collaborator_test.go | Updates assertions for new shared-helper error message formats. |
| internal/oidc/provider.go | Switches OIDC response reading/status validation to strict shared helper. |
| internal/oidc/provider_test.go | Updates assertions to match shared-helper error message wording. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 7/7 changed files
- Comments generated: 4
- Review effort level: Low
Comment on lines
13
to
17
| "net/http" | ||
|
|
||
| "github.com/github/gh-aw-mcpg/internal/httputil" | ||
| "net/url" | ||
| "strings" |
Comment on lines
+124
to
127
| body, err := httputil.ReadResponseBodyStrict(resp, http.StatusOK, "OIDC token request") | ||
| if err != nil { | ||
| return "", time.Time{}, fmt.Errorf("failed to read OIDC token response: %w", err) | ||
| return "", time.Time{}, err | ||
| } |
Collaborator
Author
|
@copilot address review feedback |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Contributor
This was referenced Jun 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #8320
Deduplicates the HTTP response body read + status check pattern that was independently implemented in three files.
Changes
New helpers (
internal/httputil/response.go)ReadResponseBody(resp, context)— Reads the full body, closes it, and returns an error if status ≥ 400. Used by proxy and collaborator call sites.ReadResponseBodyStrict(resp, expectedStatus, context)— Reads the full body, closes it, and returns an error (including the body text) if the status doesn't match exactly. Used by the OIDC call site which checks!= 200and includes the body in error messages.Refactored call sites
internal/proxy/proxy.godefer Close+ReadAll+>= 400checkhttputil.ReadResponseBodyinternal/githubhttp/collaborator.godefer Close+ReadAll+>= 400checkhttputil.ReadResponseBodyinternal/oidc/provider.godefer Close+ReadAll+!= 200check (body in error)httputil.ReadResponseBodyStrictTest updates
internal/httputil/response_test.gocollaborator_test.goandprovider_test.goto match the slightly different error message format from the shared helpers