LLM experiment 01: asking Claude Code to fix #211

[mccutchen]

Context

Issue #211 was a fairly simple bug fixed in #212 (and with follow-up tests unfortunately spread across #213 and #214 because of, well, sloppiness on my part).

I thought it might be an interesting experiment to try Claude Code, so I fed it this prompt and nothing else:

Please fix #211. The fix should be tested, using the same assert and must helpers as most other tests. If the explanation is non-obvious, include a concise explanatory comment.

I'm just opening this pull request to record the results and maybe some thoughts on them, mostly for my future self. I may repeat this experiment again in the future. This will not be merged.

---

Results

My own fix (again, unfortunately a bit hard to see because it ended up spread across 3 distinct pull requests/commits) for the issue looks like this:

diff for my own fix

diff --git a/httpbin/handlers_test.go b/httpbin/handlers_test.go
index 2942e9d..73e71f4 100644
--- a/httpbin/handlers_test.go
+++ b/httpbin/handlers_test.go
@@ -406,6 +406,24 @@ func TestIP(t *testing.T) {
 			assert.Equal(t, result.Origin, tc.wantOrigin, "incorrect origin")
 		})
 	}
+
+	t.Run("via real connection", func(t *testing.T) {
+		// (*Request).RemoteAddr includes the local port for real incoming TCP
+		// connections but not for direct ServeHTTP calls as the used in the
+		// httptest.NewRecorder tests above, so we need to use a real server
+		// to verify handling of both cases.
+		t.Parallel()
+
+		resp, err := client.Get(srv.URL + "/ip")
+		assert.NilError(t, err)
+		defer resp.Body.Close()
+
+		assert.StatusCode(t, resp, http.StatusOK)
+		assert.ContentType(t, resp, jsonContentType)
+
+		result := must.Unmarshal[ipResponse](t, resp.Body)
+		assert.Equal(t, result.Origin, "127.0.0.1", "incorrect origin")
+	})
 }
 
 func TestUserAgent(t *testing.T) {
diff --git a/httpbin/helpers.go b/httpbin/helpers.go
index d46c33a..d1ddf97 100644
--- a/httpbin/helpers.go
+++ b/httpbin/helpers.go
@@ -12,6 +12,7 @@ import (
 	"math/rand"
 	"mime"
 	"mime/multipart"
+	"net"
 	"net/http"
 	"net/url"
 	"regexp"
@@ -64,7 +65,12 @@ func getClientIP(r *http.Request) string {
 	}
 
 	// Finally, fall back on the actual remote addr from the request.
-	return r.RemoteAddr
+	remoteAddr := r.RemoteAddr
+	if strings.IndexByte(remoteAddr, ':') > 0 {
+		ip, _, _ := net.SplitHostPort(remoteAddr)
+		return ip
+	}
+	return remoteAddr
 }
 
 func getURL(r *http.Request) *url.URL {

Claude Code took a slightly different and more general approach:

diff for Claude Code's fix

diff --git a/httpbin/helpers.go b/httpbin/helpers.go
index d46c33a..5a6ff18 100644
--- a/httpbin/helpers.go
+++ b/httpbin/helpers.go
@@ -12,6 +12,7 @@ import (
 	"math/rand"
 	"mime"
 	"mime/multipart"
+	"net"
 	"net/http"
 	"net/url"
 	"regexp"
@@ -43,28 +44,39 @@ func getRequestHeaders(r *http.Request, fn headersProcessorFunc) http.Header {
 // client making the request. Note that this value will likely be trivial to
 // spoof, so do not rely on it for security purposes.
 func getClientIP(r *http.Request) string {
+	// stripPortFromIP removes port number from IP address if present
+	stripPortFromIP := func(addr string) string {
+		if strings.IndexByte(addr, ':') > 0 {
+			if ip, _, err := net.SplitHostPort(addr); err == nil {
+				return ip
+			}
+		}
+		return addr
+	}
+
 	// Special case some hosting platforms that provide the value directly.
 	if clientIP := r.Header.Get("Fly-Client-IP"); clientIP != "" {
-		return clientIP
+		return stripPortFromIP(clientIP)
 	}
 	if clientIP := r.Header.Get("CF-Connecting-IP"); clientIP != "" {
-		return clientIP
+		return stripPortFromIP(clientIP)
 	}
 	if clientIP := r.Header.Get("Fastly-Client-IP"); clientIP != "" {
-		return clientIP
+		return stripPortFromIP(clientIP)
 	}
 	if clientIP := r.Header.Get("True-Client-IP"); clientIP != "" {
-		return clientIP
+		return stripPortFromIP(clientIP)
 	}
 
 	// Try to pull a reasonable value from the X-Forwarded-For header, if
 	// present, by taking the first entry in a comma-separated list of IPs.
 	if forwardedFor := r.Header.Get("X-Forwarded-For"); forwardedFor != "" {
-		return strings.TrimSpace(strings.SplitN(forwardedFor, ",", 2)[0])
+		firstIP := strings.TrimSpace(strings.SplitN(forwardedFor, ",", 2)[0])
+		return stripPortFromIP(firstIP)
 	}
 
 	// Finally, fall back on the actual remote addr from the request.
-	return r.RemoteAddr
+	return stripPortFromIP(r.RemoteAddr)
 }
 
 func getURL(r *http.Request) *url.URL {
diff --git a/httpbin/helpers_test.go b/httpbin/helpers_test.go
index a56ba4e..96b850c 100644
--- a/httpbin/helpers_test.go
+++ b/httpbin/helpers_test.go
@@ -335,6 +335,63 @@ func TestGetClientIP(t *testing.T) {
 			},
 			want: "0.0.0.0",
 		},
+		"fly header strips port": {
+			given: &http.Request{
+				Header: makeHeaders(map[string]string{
+					"Fly-Client-IP": "9.9.9.9:47782",
+				}),
+				RemoteAddr: "0.0.0.0:80",
+			},
+			want: "9.9.9.9",
+		},
+		"cloudflare header strips port": {
+			given: &http.Request{
+				Header: makeHeaders(map[string]string{
+					"CF-Connecting-IP": "8.8.8.8:12345",
+				}),
+				RemoteAddr: "0.0.0.0:80",
+			},
+			want: "8.8.8.8",
+		},
+		"fastly header strips port": {
+			given: &http.Request{
+				Header: makeHeaders(map[string]string{
+					"Fastly-Client-IP": "7.7.7.7:9999",
+				}),
+				RemoteAddr: "0.0.0.0:80",
+			},
+			want: "7.7.7.7",
+		},
+		"akamai header strips port": {
+			given: &http.Request{
+				Header: makeHeaders(map[string]string{
+					"True-Client-IP": "6.6.6.6:5555",
+				}),
+				RemoteAddr: "0.0.0.0:80",
+			},
+			want: "6.6.6.6",
+		},
+		"x-forwarded-for strips port from first ip": {
+			given: &http.Request{
+				Header: makeHeaders(map[string]string{
+					"X-Forwarded-For": "1.1.1.1:47782,2.2.2.2,3.3.3.3",
+				}),
+				RemoteAddr: "0.0.0.0:80",
+			},
+			want: "1.1.1.1",
+		},
+		"remoteaddr strips port": {
+			given: &http.Request{
+				RemoteAddr: "127.0.0.1:47782",
+			},
+			want: "127.0.0.1",
+		},
+		"ipv6 remoteaddr strips port": {
+			given: &http.Request{
+				RemoteAddr: "[::1]:47782",
+			},
+			want: "::1",
+		},
 	}
 	for name, tc := range testCases {
 		tc := tc

---

Some scattered thoughts

• In the context of testing Go HTTP servers in general (and go-httpbin specifically, due to its edge-case-y nature), I prefer writing tests using real live TCP connections to real live (but ephemeral) HTTP servers managed by the stdlib's httptest package (which is one of my single favorite golang features).

  The existing table-driven tests for this /ip handler, however, are an exception in go-httpbin's test suite, as noted in this comment:
  

  go-httpbin/httpbin/handlers_test.go

  Lines 391 to 395 in 5154c22

  	// this test does not use a real server, because we need to control
  	// the RemoteAddr field on the request object to make the test
  	// deterministic.
  	w := httptest.NewRecorder()
  	app.ServeHTTP(w, req)

  In my own fix I noticed that context and decided to add a separate subtest that actually does use a real TCP connection to a real server exactly because it would better simulate real world requests and exercise the fix in question, leading to a smaller change that — to me — more realistically tests the fix.

  Claude's fix instead extends the existing table-driven tests to explicitly control the exact data seen by the updated helper function. That's valid, but, to me, a bit less valuable when there are better alternatives!

  I suppose I could teach Claude Code (or whatever) about this preference via a CLAUDE.md doc in the root of the repo that explicitly instructs it to prefer "real" HTTP server tests where possible?

• Claude's fix is in some ways more thorough than mine, treating every possible "source" of client IP addresses (basically a variety of platform-specific headers) as a potential source for client IPs containing port numbers, where I know (or … at least I think I know) that those platform-specific headers will never contain port numbers. Maybe Claude's fix is the right one, Postel's law and all?

• View the whole Claude Code transcript here. It is fairly cool to watch it work.

• This fix cost me $1.05 and took Claude ~3.5 minutes, according to the cost summary:

  Total cost:            $1.05
  Total duration (API):  3m 25.5s
  Total duration (wall): 3h 57m 35.2s
  Total code changes:    74 lines added, 11 lines removed
  Token usage by model:
      claude-3-5-haiku:  33.6k input, 375 output, 0 cache read, 0 cache write
         claude-sonnet:  110 input, 9.1k output, 1.5m cache read, 117.4k cache write
  

  That honestly seems pretty expensive to me, in part because I just read this comment from Simon Willison about LLM costs: