Conversation
…rdAndReadBody These four methods in internal/proxy/handler.go had zero test coverage: - ServeHTTP: the main HTTP dispatch (health checks, write passthrough, REST/GraphQL routing, GH-host prefix stripping, unknown-endpoint 403) - handleWithDIFC: the 6-phase DIFC pipeline (guard not initialised, upstream errors, non-200 pass-through, non-JSON pass-through, JSON array responses, GraphQL forwarding, strict-mode behaviour) - passthrough: write-operation forwarding (success, nil body, upstream error) - forwardAndReadBody: HTTP forwarding helper (success, network error → 502) The new tests use a NoopGuard so no WASM binary is required, and a httptest.Server to stand in for the upstream GitHub API. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds first-pass unit test coverage for the core HTTP proxy handler paths in internal/proxy, aiming to exercise the DIFC pipeline and upstream forwarding behavior without requiring a WASM guard.
Changes:
- Introduces
internal/proxy/handler_test.gowith tests forServeHTTP,handleWithDIFC,passthrough, andforwardAndReadBody. - Covers health endpoints, GH-host prefix stripping, REST/GraphQL allow/block behavior, and upstream error/pass-through scenarios.
- Adds a
newTestServerhelper wiringNoopGuard+ DIFC evaluator for deterministic handler execution.
Comments suppressed due to low confidence (2)
internal/proxy/handler_test.go:378
- Same issue as other tests: relying on "http://127.0.0.1:1" to guarantee a network error can be flaky (port might be in use, or connect behavior differs). Use a closed ephemeral port or a stub Transport/RoundTripper to produce a deterministic upstream error.
func TestPassthrough_UpstreamError(t *testing.T) {
// Point at a URL that refuses connections
s := newTestServer(t, "http://127.0.0.1:1") // port 1 is never listening
h := &proxyHandler{server: s}
internal/proxy/handler_test.go:410
- Same hard-coded unreachable upstream issue: "http://127.0.0.1:1" isn’t guaranteed to fail quickly or consistently on all CI runners. Consider using a stubbed http.RoundTripper or a closed ephemeral port, and combine with a short client timeout from newTestServer to prevent hangs.
func TestForwardAndReadBody_NetworkError(t *testing.T) {
s := newTestServer(t, "http://127.0.0.1:1")
h := &proxyHandler{server: s}
w := httptest.NewRecorder()
resp, body := h.forwardAndReadBody(w, context.Background(), http.MethodGet, "/repos/org/repo/issues", nil, "", "")
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| func newTestServer(t *testing.T, upstreamURL string) *Server { | ||
| t.Helper() | ||
| return &Server{ | ||
| guard: guard.NewNoopGuard(), | ||
| evaluator: difc.NewEvaluatorWithMode(difc.EnforcementFilter), | ||
| agentRegistry: difc.NewAgentRegistryWithDefaults(nil, nil), | ||
| capabilities: difc.NewCapabilities(), | ||
| githubAPIURL: upstreamURL, | ||
| httpClient: &http.Client{}, | ||
| guardInitialized: true, | ||
| enforcementMode: difc.EnforcementFilter, | ||
| } |
There was a problem hiding this comment.
newTestServer sets httpClient to &http.Client{} with no Timeout. Several tests intentionally hit an unreachable upstream (127.0.0.1:1); without a client timeout this can hang in environments where the connect attempt is slow (e.g., SYN drops / sandboxed networking). Set a small Timeout (and/or a custom Transport with DialContext timeout) on the test client to keep these unit tests deterministic.
| func TestHandleWithDIFC_UpstreamError(t *testing.T) { | ||
| // Use an unreachable address to force a network error in forwardToGitHub. | ||
| s := newTestServer(t, "http://127.0.0.1:1") | ||
| h := &proxyHandler{server: s} |
There was a problem hiding this comment.
This test forces an upstream failure by hard-coding "http://127.0.0.1:1". That port can be unexpectedly open in some environments, and even when closed the connect may behave differently across platforms. Prefer a deterministic failure source (e.g., create a listener on 127.0.0.1:0, close it, then use that port; or use a custom http.RoundTripper that returns an error).
This issue also appears in the following locations of the same file:
- line 375
- line 405
| // ─── ServeHTTP: strict mode blocks when items filtered ─────────────────────── | ||
|
|
||
| func TestHandleWithDIFC_StrictModeBlocksFilteredItems(t *testing.T) { | ||
| upstream := mockUpstream(t, http.StatusOK, map[string]interface{}{ | ||
| "total_count": 1, | ||
| "items": []interface{}{map[string]interface{}{"id": 1}}, | ||
| }) | ||
| defer upstream.Close() | ||
|
|
||
| // strict mode with NoopGuard (no labels set) — evaluator allows, no items filtered | ||
| s := newTestServer(t, upstream.URL) | ||
| s.enforcementMode = difc.EnforcementStrict | ||
| s.evaluator = difc.NewEvaluatorWithMode(difc.EnforcementStrict) | ||
| h := &proxyHandler{server: s} | ||
|
|
||
| req := httptest.NewRequest(http.MethodGet, "/search/issues?q=test", nil) | ||
| w := httptest.NewRecorder() | ||
| h.ServeHTTP(w, req) | ||
|
|
||
| // NoopGuard returns nil LabelResponse, so no items are filtered → 200 OK | ||
| assert.Equal(t, http.StatusOK, w.Code) | ||
| } |
There was a problem hiding this comment.
The test name and inline comments don’t match the asserted behavior: the name says strict mode blocks filtered items, but the test asserts 200 OK and the comment says no items are filtered. Rename the test (and adjust the comment) to describe what it actually verifies, or change the fixture/guard setup to exercise the strict-mode blocking branch.
Test Coverage Improvement:
proxyhandler methodsFunction Analyzed
internal/proxyServeHTTP,handleWithDIFC,passthrough,forwardAndReadBodyhandleWithDIFCis a 190-line, 6-phase DIFC pipeline with many conditional branchesWhy These Functions?
internal/proxy/handler.gohad zero test coverage for its four core methods.handleWithDIFCis the most complex function in the file (6-phase DIFC pipeline: agent labeling → resource labeling → coarse access check → upstream forwarding → response labeling → fine-grained filtering → label propagation), yet none of its branches were exercised by any test. The other three methods (ServeHTTP,passthrough,forwardAndReadBody) are the primary entry and forwarding points for all proxy traffic.Tests Added
/healthand/healthzreturn{"status":"ok"}/api/v3/healthis treated as/health__schema/__typequeries forwarded?q=...reaches the upstream server/graphqltraverses pipelinepassthroughsuccess — body, method, and status forwarded correctlypassthroughnil body — DELETE without body handledpassthroughupstream error — network failure → 502forwardAndReadBodysuccess — returns response and body bytesforwardAndReadBodynetwork error — writes 502 and returnsnil, nilTest Approach
Tests use:
guard.NewNoopGuard()— no WASM binary needed; returns empty labels (all operations allowed)httptest.NewServer— mock GitHub API upstream, exercising the full forwarding pathhttptest.NewRecorder— captures HTTP response written by the handlerpackage proxy) to access unexportedServerfields andproxyHandlerCoverage Report
Generated by Test Coverage Improver
Next run should target:
internal/proxy/proxy.go→restBackendCaller.CallTool(multiple switch branches, currently untested)