[test-improver] Improve tests for server/circuit_breaker (#3837)

## File Analyzed

- **Test File**: `internal/server/circuit_breaker_test.go`
- **Package**: `internal/server`
- **Lines Added**: +129

## Improvements Made

### 1. Better Testing Patterns
- ✅ All new tests use table-driven structure with `t.Parallel()`
subtests
- ✅ Descriptive test names following `TestType_Scenario` convention
- ✅ Uses `require` for fatal preconditions, `assert` for non-fatal
checks

### 2. Increased Coverage

Four previously untested code paths now have direct tests:

| New Test | Code Path Covered |
|---|---|
| `TestCircuitBreakerState_String` | `String()` method for all 4 states
including defensive `default: "UNKNOWN"` case |
| `TestFormatResetAt` | `formatResetAt()` helper — zero time and
non-zero RFC3339+duration output |
| `TestIsRateLimitText_Direct` | `isRateLimitText()` — all 5 match
patterns plus edge cases (case insensitivity, empty string, `"rate
limit"` without qualifier) |
| `TestCircuitBreaker_RecordRateLimitWhenAlreadyOpen` |
`RecordRateLimit()` OPEN→OPEN path — verifies `resetAt` is updated on
subsequent errors while circuit stays OPEN |

Previously these were either:
- **Never tested**: `String()` default case, `formatResetAt()` directly
- **Only tested indirectly**: `isRateLimitText()` via
`isRateLimitToolResult()`, the OPEN→OPEN path via logging only

### 3. No Regressions
- All existing tests are preserved unchanged
- No test helpers or fixtures modified

## Why These Changes?

The circuit breaker is security-critical infrastructure (rate-limit
protection). The four gaps found were:

1. The `String()` method's `default` case is defensive code that can
never occur with valid iota values — but it should be documented via a
test so future readers know it exists and won't get dropped in
refactoring.
2. `formatResetAt` is called in `ErrCircuitOpen.Error()` and several log
lines but had no isolated test — a regression in its formatting would
only surface through integration test failures.
3. `isRateLimitText` has 5 OR-combined patterns. The indirect tests
covered only 4 of them through `isRateLimitToolResult`. The "rate limit
combined with 403" branch (`strings.Contains(lower, "rate limit") &&
strings.Contains(lower, "403")`) had no test for a string that contains
`"rate limit"` without `"403"` being present — confirming the boundary
between true/false is correctly placed.
4. The OPEN→OPEN `RecordRateLimit` path (updating `resetAt` while
already open) is a normal operational scenario (multiple rate-limit
errors in a row) that had no test at all.

---
*Generated by Test Improver Workflow*
*Focuses on better patterns, increased coverage, and more stable tests*




> Generated by [Test
Improver](https://github.com/github/gh-aw-mcpg/actions/runs/24452358045/agentic_workflow)
· ● 14.7M ·
[◷](https://github.com/search?q=repo%3Agithub%2Fgh-aw-mcpg+%22gh-aw-workflow-id%3A+test-improver%22&type=pullrequests)

<!-- gh-aw-agentic-workflow: Test Improver, engine: copilot, model:
auto, id: 24452358045, workflow_id: test-improver, run:
https://github.com/github/gh-aw-mcpg/actions/runs/24452358045 -->

<!-- gh-aw-workflow-id: test-improver -->

Author: lpcox

internal/server/circuit_breaker_test.go

@@ -433,3 +433,132 @@ func TestExtractRateLimitErrorText(t *testing.T) {
 		assert.Equal(t, "rate limit exceeded", extractRateLimitErrorText(result))
 	})
 }
+
+// TestCircuitBreakerState_String verifies the string representation of each circuit breaker state.
+func TestCircuitBreakerState_String(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		state circuitBreakerState
+		want  string
+	}{
+		{circuitClosed, "CLOSED"},
+		{circuitOpen, "OPEN"},
+		{circuitHalfOpen, "HALF-OPEN"},
+		{circuitBreakerState(99), "UNKNOWN"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.want, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tt.want, tt.state.String())
+		})
+	}
+}
+
+// TestFormatResetAt verifies the formatResetAt helper function.
+func TestFormatResetAt(t *testing.T) {
+	t.Parallel()
+
+	t.Run("zero time returns 'unknown'", func(t *testing.T) {
+		t.Parallel()
+		assert.Equal(t, "unknown", formatResetAt(time.Time{}))
+	})
+
+	t.Run("non-zero time includes RFC3339 timestamp and duration hint", func(t *testing.T) {
+		t.Parallel()
+		resetTime := time.Date(2026, 1, 1, 12, 0, 0, 0, time.UTC)
+		result := formatResetAt(resetTime)
+		assert.Contains(t, result, "2026-01-01T12:00:00Z", "should contain RFC3339-formatted time")
+		assert.Contains(t, result, "in ", "should contain duration hint")
+	})
+}
+
+// TestIsRateLimitText_Direct directly verifies isRateLimitText with each pattern and edge cases.
+func TestIsRateLimitText_Direct(t *testing.T) {
+	t.Parallel()
+
+	tests := []struct {
+		name string
+		text string
+		want bool
+	}{
+		{
+			name: "rate limit exceeded",
+			text: "403 API rate limit exceeded",
+			want: true,
+		},
+		{
+			name: "rate limit combined with 403 status",
+			text: "error: rate limit triggered, status 403",
+			want: true,
+		},
+		{
+			name: "api rate limit phrase",
+			text: "api rate limit reached",
+			want: true,
+		},
+		{
+			name: "secondary rate limit phrase",
+			text: "secondary rate limit triggered",
+			want: true,
+		},
+		{
+			name: "too many requests phrase",
+			text: "too many requests, please slow down",
+			want: true,
+		},
+		{
+			name: "case insensitive match",
+			text: "RATE LIMIT EXCEEDED",
+			want: true,
+		},
+		{
+			name: "rate limit phrase without 403 or qualifier",
+			text: "rate limit information page",
+			want: false,
+		},
+		{
+			name: "unrelated error",
+			text: "repository not found",
+			want: false,
+		},
+		{
+			name: "empty string",
+			text: "",
+			want: false,
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()
+			assert.Equal(t, tt.want, isRateLimitText(tt.text))
+		})
+	}
+}
+
+// TestCircuitBreaker_RecordRateLimitWhenAlreadyOpen verifies that calling RecordRateLimit
+// on an already-OPEN circuit keeps it OPEN and updates the reset time.
+func TestCircuitBreaker_RecordRateLimitWhenAlreadyOpen(t *testing.T) {
+	t.Parallel()
+
+	fakeNow := time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC)
+	cb := newCircuitBreaker("test", 1, time.Hour)
+	cb.nowFunc = func() time.Time { return fakeNow }
+
+	initialReset := fakeNow.Add(30 * time.Second)
+	cb.RecordRateLimit(initialReset)
+	require.Equal(t, circuitOpen, cb.State(), "should be OPEN after threshold errors")
+
+	// Record another rate limit while already OPEN with a later reset time.
+	laterReset := fakeNow.Add(90 * time.Second)
+	cb.RecordRateLimit(laterReset)
+	assert.Equal(t, circuitOpen, cb.State(), "should remain OPEN")
+
+	// The reset time should be updated to the later value.
+	cb.mu.Lock()
+	gotReset := cb.resetAt
+	cb.mu.Unlock()
+	assert.Equal(t, laterReset, gotReset, "resetAt should be updated to the later reset time")
+}