fix: Plug middleware to respect Response writer swaps

[ainsleyclark]

Summary

Fixed a bug in the Plug middleware system where response writer swaps made by plugs (e.g., wrapping with a recorder) were being discarded when calling the next handler. The middleware now correctly passes the potentially-modified ctx.Response to downstream handlers instead of always using the original outer writer.

Key Changes

• pkg/webkit/mux.go: Modified Plug to call next.ServeHTTP(c.Response, c.Request) instead of next.ServeHTTP(w, r), ensuring that any response writer modifications made by the plug are visible to downstream handlers
• pkg/util/httputil/recorder.go: Added automatic status code defaulting to 200 in Write() method to mirror standard net/http behavior when WriteHeader hasn't been explicitly called
• pkg/util/httputil/recorder_test.go: Added two test cases to verify the recorder correctly defaults status to 200 and preserves explicitly set status codes
• pkg/webkit/mux_test.go: Added regression test demonstrating that a plug's response writer swap (e.g., a recorder) now correctly captures the status code written by downstream handlers

Implementation Details

The core issue was that the plug middleware was capturing the outer response writer w in a closure and always passing it to the next handler, ignoring any ctx.Response modifications. By passing c.Response instead, plugs can now effectively wrap or replace the response writer for the entire handler chain. The recorder status defaulting ensures compatibility with standard HTTP handler behavior.

https://claude.ai/code/session_01SYHXLF9NZsd9AvunJ6az8v

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[claude]

Review summary

• Overall score: 8/10
• Critical issues: 0
• Warnings: 2
• Suggestions: 2
• Recommendation: ⚠️ Approve with comments

Clean, minimal bug fix with a clear root cause. The one-line change to mux.go is correct and the regression test properly documents the old behaviour. Two minor style issues worth addressing.

---

Critical issues 🔴

None

---

Warnings 🟡

1. New recorder tests missing t.Parallel()

TestResponseRecorder_Write_DefaultsStatusTo200 and TestResponseRecorder_Write_PreservesExplicitStatus don't call t.Parallel(). The package already uses parallel tests in httputil_test.go and the new mux subtest correctly calls t.Parallel(). The existing TestResponseRecorder_Write and TestResponseRecorder_WriteHeader have the same omission, but new tests should set the pattern rather than inherit it.

func TestResponseRecorder_Write_DefaultsStatusTo200(t *testing.T) {
    t.Parallel()
    // ...
}

2. Response written to swapped writer is lost from the real client connection

In the regression test the plug fully replaces ctx.Response with httptest.NewRecorder(). This means nothing reaches the original w passed to ServeHTTP — the actual HTTP client would receive a blank response. This is demonstrated (the outer recorder is discarded), but the more idiomatic and safer pattern wraps rather than replaces:

recorder := httputil.NewResponseRecorder(ctx.Response) // wraps real writer, not replaces
ctx.Response = recorder
err := next(ctx)
// inspect recorder.Status / recorder.Body

The existing httputil.ResponseRecorder already supports this pattern. The test as written does prove the fix works, but a wrapping-style test would also exercise ResponseRecorder and reflect real-world usage more faithfully. No production behaviour is wrong — this is a test design note.

---

Suggestions 🟢

1. The Status == 0 sentinel in Write() is implicit

Status is never explicitly documented as "zero means not yet written". If a caller were to set Status back to 0 after a WriteHeader call, the next Write would silently override it to 200. This is an edge case that can't occur in practice (HTTP statuses are never 0), but a brief comment on the struct field would make the invariant explicit:

Status int // 0 until WriteHeader or Write is called

2. Comment in regression test references PR/fix context

// Regression: Plug used to call next.ServeHTTP(w, r) with the captured outer
// writer, discarding any ctx.Response swap made by the plug (e.g. a recorder).

Per the project convention, comments should not reference the specific fix or issue number — those belong in commit/PR history. The test name already conveys the intent; the comment could be shortened to just document the invariant being asserted.

---

Generated with Claude Code