Part of duplicate code analysis: #2628
Summary
internal/proxy/handler.go repeats the same "call forwardToGitHub, check error, defer close, io.ReadAll body, check error" sequence at 3 separate locations — in ServeHTTP (GraphQL introspection passthrough), handleWithDIFC (Phase 3 upstream forward), and passthrough.
Duplication Details
Pattern: forwardToGitHub + defer close + io.ReadAll body
- Severity: Medium
- Occurrences: 3 sites in one file
- Locations:
internal/proxy/handler.go (lines 83–90) — GraphQL introspection passthrough
internal/proxy/handler.go (lines 173–189) — DIFC Phase 3 upstream forward
internal/proxy/handler.go (lines 332–343) — write passthrough
- Code Sample:
// Site 1 (lines 83-90, slightly abbreviated):
resp, err := h.server.forwardToGitHub(r.Context(), http.MethodPost, "/graphql", bytes.NewReader(graphQLBody), "application/json", clientAuth)
if err != nil {
http.Error(w, "upstream request failed", http.StatusBadGateway)
return
}
defer resp.Body.Close()
respBody, _ := io.ReadAll(resp.Body)
// Site 2 (lines 177-189):
if err != nil {
logHandler.Printf("[DIFC] Phase 3 failed: %v", err)
http.Error(w, "upstream request failed", http.StatusBadGateway)
return
}
defer resp.Body.Close()
respBody, err := io.ReadAll(resp.Body)
if err != nil {
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
return
}
// Site 3 (lines 332-343): identical to site 2 without DIFC log line
resp, err := h.server.forwardToGitHub(r.Context(), r.Method, path, body, ...)
if err != nil {
http.Error(w, "upstream request failed", http.StatusBadGateway)
return
}
defer resp.Body.Close()
respBody, err := io.ReadAll(resp.Body)
if err != nil {
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
return
}
Impact Analysis
- Maintainability: Three independent copies of the same 8-line sequence. A future change (e.g., adding a timeout, changing the error message, adding tracing) must be applied in all three places.
- Bug Risk: Site 1 silently ignores
io.ReadAll errors (respBody, _ := io.ReadAll) while Sites 2 and 3 handle them — inconsistent behavior that could hide upstream body truncation.
- Code Bloat: ~24 lines that could be replaced by a helper + 3 call sites.
Refactoring Recommendations
-
Extract a forwardAndReadBody helper method on proxyHandler:
// forwardAndReadBody forwards a request to the upstream GitHub API and reads the
// entire response body. It writes a 502 error to w and returns nil, nil on failure.
func (h *proxyHandler) forwardAndReadBody(
w http.ResponseWriter, ctx context.Context,
method, path string, body io.Reader, contentType, clientAuth string,
) (*http.Response, []byte) {
resp, err := h.server.forwardToGitHub(ctx, method, path, body, contentType, clientAuth)
if err != nil {
http.Error(w, "upstream request failed", http.StatusBadGateway)
return nil, nil
}
defer resp.Body.Close()
respBody, err := io.ReadAll(resp.Body)
if err != nil {
http.Error(w, "failed to read upstream response", http.StatusBadGateway)
return nil, nil
}
return resp, respBody
}
Then callers check if resp == nil { return }.
-
Fix the silent error discard at line 89 (respBody, _ := io.ReadAll(...)) to be consistent with the other sites.
Estimated effort: ~45 min
Benefits: consistent error handling, single place for tracing/timeout logic, fixes inconsistency bug at line 89.
Implementation Checklist
Parent Issue
See parent analysis report: #2628
Related to #2628
Generated by Duplicate Code Detector · ◷
Part of duplicate code analysis: #2628
Summary
internal/proxy/handler.gorepeats the same "callforwardToGitHub, check error, defer close,io.ReadAllbody, check error" sequence at 3 separate locations — inServeHTTP(GraphQL introspection passthrough),handleWithDIFC(Phase 3 upstream forward), andpassthrough.Duplication Details
Pattern: forwardToGitHub + defer close + io.ReadAll body
internal/proxy/handler.go(lines 83–90) — GraphQL introspection passthroughinternal/proxy/handler.go(lines 173–189) — DIFC Phase 3 upstream forwardinternal/proxy/handler.go(lines 332–343) — write passthroughImpact Analysis
io.ReadAllerrors (respBody, _ := io.ReadAll) while Sites 2 and 3 handle them — inconsistent behavior that could hide upstream body truncation.Refactoring Recommendations
Extract a
forwardAndReadBodyhelper method onproxyHandler:Then callers check
if resp == nil { return }.Fix the silent error discard at line 89 (
respBody, _ := io.ReadAll(...)) to be consistent with the other sites.Estimated effort: ~45 min
Benefits: consistent error handling, single place for tracing/timeout logic, fixes inconsistency bug at line 89.
Implementation Checklist
forwardAndReadBodyhelper toproxyHandlerio.ReadAllerror at line 89make agent-finishedto verify no regressionsParent Issue
See parent analysis report: #2628
Related to #2628