Apply review suggestions

Author: kacpersaw

bridge.go

@@ -10,8 +10,8 @@ import (
 	"time"
 
 	"cdr.dev/slog/v3"
-	aibcontext "github.com/coder/aibridge/context"
 	"github.com/coder/aibridge/circuitbreaker"
+	aibcontext "github.com/coder/aibridge/context"
 	"github.com/coder/aibridge/mcp"
 	"github.com/coder/aibridge/metrics"
 	"github.com/coder/aibridge/provider"
@@ -77,7 +77,7 @@ func NewRequestBridge(ctx context.Context, providers []provider.Provider, rec re
 				slog.F("from", from.String()),
 				slog.F("to", to.String()),
 			)
-			if cfg != nil && m != nil {
+			if m != nil {
 				m.CircuitBreakerState.WithLabelValues(providerName, endpoint).Set(circuitbreaker.StateToGaugeValue(to))
 				if to == gobreaker.StateOpen {
 					m.CircuitBreakerTrips.WithLabelValues(providerName, endpoint).Inc()
@@ -90,7 +90,7 @@ func NewRequestBridge(ctx context.Context, providers []provider.Provider, rec re
 		for _, path := range prov.BridgedRoutes() {
 			handler := newInterceptionProcessor(prov, rec, mcpProxy, logger, m, tracer)
 			// Wrap with circuit breaker middleware (nil cbs passes through)
-			wrapped := circuitbreaker.Middleware(cbs, m)(handler)
+			wrapped := circuitbreaker.Middleware(cbs, m, logger)(handler)
 			mux.Handle(path, wrapped)
 		}
 

circuit_breaker_integration_test.go

@@ -16,17 +16,19 @@ import (
 	"github.com/coder/aibridge"
 	"github.com/coder/aibridge/circuitbreaker"
 	"github.com/coder/aibridge/config"
+	"github.com/coder/aibridge/mcp"
 	"github.com/coder/aibridge/metrics"
 	"github.com/coder/aibridge/provider"
-	"github.com/coder/aibridge/mcp"
 	"github.com/prometheus/client_golang/prometheus"
 	promtest "github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"go.opentelemetry.io/otel"
 )
 
-func TestCircuitBreaker_WithNewRequestBridge(t *testing.T) {
+// TestCircuitBreaker_FullRecoveryCycle tests the complete circuit breaker lifecycle:
+// closed → open (after consecutive failures) → half-open (after timeout) → closed (after successful request)
+func TestCircuitBreaker_FullRecoveryCycle(t *testing.T) {
 	t.Parallel()
 
 	type testCase struct {
@@ -136,12 +138,14 @@ func TestCircuitBreaker_WithNewRequestBridge(t *testing.T) {
 			mockSrv.Start()
 
 			makeRequest := func() *http.Response {
-				req, _ := http.NewRequest("POST", mockSrv.URL+"/"+tc.providerName+tc.endpoint, strings.NewReader(tc.requestBody))
+				req, err := http.NewRequest("POST", mockSrv.URL+"/"+tc.providerName+tc.endpoint, strings.NewReader(tc.requestBody))
+				require.NoError(t, err)
 				req.Header.Set("Content-Type", "application/json")
 				tc.setupHeaders(req)
 				resp, err := http.DefaultClient.Do(req)
 				require.NoError(t, err)
-				_, _ = io.ReadAll(resp.Body)
+				_, err = io.ReadAll(resp.Body)
+				require.NoError(t, err)
 				resp.Body.Close()
 				return resp
 			}
@@ -203,6 +207,8 @@ func TestCircuitBreaker_WithNewRequestBridge(t *testing.T) {
 	}
 }
 
+// TestCircuitBreaker_HalfOpenFailure tests that a failed request in half-open state
+// returns the circuit to open: closed → open → half-open → open
 func TestCircuitBreaker_HalfOpenFailure(t *testing.T) {
 	t.Parallel()
 
@@ -253,13 +259,15 @@ func TestCircuitBreaker_HalfOpenFailure(t *testing.T) {
 	mockSrv.Start()
 
 	makeRequest := func() *http.Response {
-		req, _ := http.NewRequest("POST", mockSrv.URL+"/openai/v1/chat/completions",
+		req, err := http.NewRequest("POST", mockSrv.URL+"/openai/v1/chat/completions",
 			strings.NewReader(`{"model":"gpt-4o","messages":[{"role":"user","content":"hi"}]}`))
+		require.NoError(t, err)
 		req.Header.Set("Content-Type", "application/json")
 		req.Header.Set("Authorization", "Bearer test-key")
 		resp, err := http.DefaultClient.Do(req)
 		require.NoError(t, err)
-		_, _ = io.ReadAll(resp.Body)
+		_, err = io.ReadAll(resp.Body)
+		require.NoError(t, err)
 		resp.Body.Close()
 		return resp
 	}

circuitbreaker/circuitbreaker.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 	"time"
 
+	"cdr.dev/slog/v3"
 	"github.com/coder/aibridge/metrics"
 	"github.com/sony/gobreaker/v2"
 )
@@ -39,20 +40,26 @@ func DefaultConfig() Config {
 	}
 }
 
-// DefaultIsFailure returns true for status codes that typically indicate
-// upstream overload: 429 (Too Many Requests), 503 (Service Unavailable),
-// and 529 (Anthropic Overloaded).
+// DefaultIsFailure returns true for standard HTTP status codes that typically
+// indicate upstream overload: 429 (Too Many Requests) and 503 (Service Unavailable).
 func DefaultIsFailure(statusCode int) bool {
 	switch statusCode {
 	case http.StatusTooManyRequests, // 429
-		http.StatusServiceUnavailable, // 503
-		529:                           // Anthropic "Overloaded"
+		http.StatusServiceUnavailable: // 503
 		return true
 	default:
 		return false
 	}
 }
 
+// AnthropicIsFailure extends DefaultIsFailure with Anthropic's custom 529 "Overloaded" status.
+func AnthropicIsFailure(statusCode int) bool {
+	if statusCode == 529 {
+		return true
+	}
+	return DefaultIsFailure(statusCode)
+}
+
 // ProviderCircuitBreakers manages per-endpoint circuit breakers for a single provider.
 type ProviderCircuitBreakers struct {
 	provider string
@@ -141,7 +148,7 @@ func (w *statusCapturingWriter) Unwrap() http.ResponseWriter {
 // Middleware returns middleware that wraps handlers with circuit breaker protection.
 // It captures the response status code to determine success/failure without provider-specific logic.
 // If cbs is nil, requests pass through without circuit breaker protection.
-func Middleware(cbs *ProviderCircuitBreakers, m *metrics.Metrics) func(http.Handler) http.Handler {
+func Middleware(cbs *ProviderCircuitBreakers, m *metrics.Metrics, logger slog.Logger) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		// No circuit breaker configured - pass through
 		if cbs == nil {
@@ -170,6 +177,8 @@ func Middleware(cbs *ProviderCircuitBreakers, m *metrics.Metrics) func(http.Hand
 				w.Header().Set("Content-Type", "application/json")
 				w.WriteHeader(http.StatusServiceUnavailable)
 				w.Write([]byte(`{"type":"error","error":{"type":"circuit_breaker_open","message":"circuit breaker is open"}}`))
+			} else if err != nil {
+				logger.Warn(r.Context(), "unexpected circuit breaker error", slog.Error(err))
 			}
 		})
 	}

circuitbreaker/circuitbreaker_test.go

@@ -7,6 +7,9 @@ import (
 	"testing"
 	"time"
 
+	"cdr.dev/slog/v3"
+	"cdr.dev/slog/v3/sloggers/slogtest"
+
 	"github.com/sony/gobreaker/v2"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -36,7 +39,8 @@ func TestMiddleware_PerEndpointIsolation(t *testing.T) {
 		MaxRequests:      1,
 	}, func(endpoint string, from, to gobreaker.State) {})
 
-	handler := Middleware(cbs, nil)(upstream)
+	logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug)
+	handler := Middleware(cbs, nil, logger)(upstream)
 	server := httptest.NewServer(handler)
 	defer server.Close()
 
@@ -71,7 +75,8 @@ func TestMiddleware_NotConfigured(t *testing.T) {
 	})
 
 	// No circuit breaker configured (nil)
-	handler := Middleware(nil, nil)(upstream)
+	logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug)
+	handler := Middleware(nil, nil, logger)(upstream)
 	server := httptest.NewServer(handler)
 	defer server.Close()
 
@@ -103,7 +108,8 @@ func TestMiddleware_CustomIsFailure(t *testing.T) {
 		},
 	}, func(endpoint string, from, to gobreaker.State) {})
 
-	handler := Middleware(cbs, nil)(upstream)
+	logger := slogtest.Make(t, &slogtest.Options{IgnoreErrors: false}).Leveled(slog.LevelDebug)
+	handler := Middleware(cbs, nil, logger)(upstream)
 	server := httptest.NewServer(handler)
 	defer server.Close()
 

metrics/metrics.go

@@ -120,7 +120,7 @@ func NewMetrics(reg prometheus.Registerer) *Metrics {
 		CircuitBreakerTrips: promauto.With(reg).NewCounterVec(prometheus.CounterOpts{
 			Subsystem: "circuit_breaker",
 			Name:      "trips_total",
-			Help:      "Total number of times the circuit breaker has tripped open.",
+			Help:      "Total number of times the circuit breaker transitioned to open state.",
 		}, []string{"provider", "endpoint"}),
 		// Pessimistic cardinality: 2 providers, 5 endpoints = up to 10.
 		CircuitBreakerRejects: promauto.With(reg).NewCounterVec(prometheus.CounterOpts{