From ef59dfaeff9b63edf39e100b4eacdaac1a3b91c8 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 17:47:47 +0000 Subject: [PATCH 1/2] Initial plan From 43f8bd3e499c09f4cb1aed82b8fd624b9b59026f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 15 Apr 2026 18:09:38 +0000 Subject: [PATCH 2/2] refactor: consolidate near-duplicate rate-limit header parsers and clean up outlier functions - Add shared httputil.ParseRateLimitResetHeader (with TrimSpace); remove duplicate parseRateLimitResetHeader (server) and parseRateLimitReset (proxy) - Move validateGuardPolicies from config/guard_policy.go to config/validation.go alongside all other top-level config validators; replace logGuardPolicy with logValidation - Remove trivial truncateForLog wrapper in proxy/graphql.go; inline strutil.TruncateRunes directly at the two call sites - Update/remove tests accordingly Agent-Logs-Url: https://github.com/github/gh-aw-mcpg/sessions/2e461a0f-7bcc-4bd7-b521-c39a4af9ad09 Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> --- internal/config/guard_policy.go | 12 --- internal/config/validation.go | 14 +++ internal/httputil/httputil.go | 17 +++ internal/httputil/httputil_test.go | 60 +++++++++++ internal/proxy/graphql.go | 9 +- internal/proxy/graphql_test.go | 134 ------------------------ internal/proxy/handler.go | 15 +-- internal/proxy/rate_limit_test.go | 23 ---- internal/server/circuit_breaker.go | 14 --- internal/server/circuit_breaker_test.go | 53 ---------- 10 files changed, 94 insertions(+), 257 deletions(-) delete mode 100644 internal/proxy/graphql_test.go diff --git a/internal/config/guard_policy.go b/internal/config/guard_policy.go index 7da22e60..edebfa5b 100644 --- a/internal/config/guard_policy.go +++ b/internal/config/guard_policy.go @@ -777,18 +777,6 @@ func ParseGuardPolicyJSON(policyJSON string) (*GuardPolicy, error) { return policy, nil } -func validateGuardPolicies(cfg *Config) error { - logGuardPolicy.Printf("Validating guard policies: count=%d", len(cfg.Guards)) - for name, guardCfg := range cfg.Guards { - if guardCfg != nil && guardCfg.Policy != nil { - if err := ValidateGuardPolicy(guardCfg.Policy); err != nil { - return fmt.Errorf("invalid policy for guard '%s': %w", name, err) - } - } - } - return nil -} - // NormalizeScopeKind returns a copy of the policy map with the scope_kind field // normalized to lowercase trimmed string form. Other fields are preserved as-is. func NormalizeScopeKind(policy map[string]interface{}) map[string]interface{} { diff --git a/internal/config/validation.go b/internal/config/validation.go index b0b7971a..e3c7b551 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -634,3 +634,17 @@ func validateOpenTelemetryConfig(cfg *TracingConfig, enforceHTTPS bool) error { logValidation.Print("OpenTelemetry config validation passed") return nil } + +// validateGuardPolicies validates all per-server guard policies in the config. +// It iterates over cfg.Guards and calls ValidateGuardPolicy for each non-nil policy. +func validateGuardPolicies(cfg *Config) error { + logValidation.Printf("Validating guard policies: count=%d", len(cfg.Guards)) + for name, guardCfg := range cfg.Guards { + if guardCfg != nil && guardCfg.Policy != nil { + if err := ValidateGuardPolicy(guardCfg.Policy); err != nil { + return fmt.Errorf("invalid policy for guard '%s': %w", name, err) + } + } + } + return nil +} diff --git a/internal/httputil/httputil.go b/internal/httputil/httputil.go index ba004e91..0b665649 100644 --- a/internal/httputil/httputil.go +++ b/internal/httputil/httputil.go @@ -5,6 +5,9 @@ package httputil import ( "encoding/json" "net/http" + "strconv" + "strings" + "time" ) // WriteJSONResponse sets the Content-Type header, writes the status code, and encodes @@ -18,3 +21,17 @@ func WriteJSONResponse(w http.ResponseWriter, statusCode int, body interface{}) } w.Write(data) } + +// ParseRateLimitResetHeader parses the Unix-timestamp value of the +// X-RateLimit-Reset HTTP header into a time.Time. +// Returns zero time when the header value is absent or malformed. +func ParseRateLimitResetHeader(value string) time.Time { + if value == "" { + return time.Time{} + } + unix, err := strconv.ParseInt(strings.TrimSpace(value), 10, 64) + if err != nil { + return time.Time{} + } + return time.Unix(unix, 0) +} diff --git a/internal/httputil/httputil_test.go b/internal/httputil/httputil_test.go index 40ecc87a..10900ae2 100644 --- a/internal/httputil/httputil_test.go +++ b/internal/httputil/httputil_test.go @@ -4,7 +4,9 @@ import ( "encoding/json" "net/http" "net/http/httptest" + "strconv" "testing" + "time" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" @@ -151,3 +153,61 @@ func TestWriteJSONResponse(t *testing.T) { assert.Empty(t, rec.Body.String()) }) } + +// TestParseRateLimitResetHeader verifies the shared Unix-timestamp header parser. +func TestParseRateLimitResetHeader(t *testing.T) { + t.Parallel() + + now := time.Now() + future := now.Add(60 * time.Second) + + tests := []struct { + name string + value string + wantZero bool + wantTime time.Time + }{ + { + name: "empty", + value: "", + wantZero: true, + }, + { + name: "invalid", + value: "not-a-number", + wantZero: true, + }, + { + name: "valid unix timestamp", + value: "1000000000", + wantZero: false, + wantTime: time.Unix(1000000000, 0), + }, + { + name: "future timestamp", + value: strconv.FormatInt(future.Unix(), 10), + wantZero: false, + }, + { + name: "value with surrounding whitespace", + value: " 1000000000 ", + wantZero: false, + wantTime: time.Unix(1000000000, 0), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + got := ParseRateLimitResetHeader(tt.value) + if tt.wantZero { + assert.True(t, got.IsZero(), "expected zero time") + } else { + assert.False(t, got.IsZero(), "expected non-zero time") + if !tt.wantTime.IsZero() { + assert.Equal(t, tt.wantTime.Unix(), got.Unix()) + } + } + }) + } +} diff --git a/internal/proxy/graphql.go b/internal/proxy/graphql.go index 21490b59..c26ba70c 100644 --- a/internal/proxy/graphql.go +++ b/internal/proxy/graphql.go @@ -190,24 +190,19 @@ func extractOwnerRepo(variables map[string]interface{}, query string) (string, s // searchQueryArgPattern extracts the literal query string from search(query:"...", ...) var searchQueryArgPattern = regexp.MustCompile(`(?i)\bsearch\s*\(\s*query\s*:\s*"([^"]+)"`) -// truncateForLog truncates s to at most maxRunes runes, for safe debug logging. -func truncateForLog(s string, maxRunes int) string { - return strutil.TruncateRunes(s, maxRunes) -} - // extractSearchQuery returns the search query argument from a GraphQL search // query. It checks variables ($query) first, then inline query text. func extractSearchQuery(query string, variables map[string]interface{}) string { // Check variables for $query if variables != nil { if v, ok := variables["query"].(string); ok && v != "" { - logGraphQL.Printf("extractSearchQuery: found in variables: %q", truncateForLog(v, 80)) + logGraphQL.Printf("extractSearchQuery: found in variables: %q", strutil.TruncateRunes(v, 80)) return v } } // Parse inline: search(query:"repo:owner/name is:issue", ...) if m := searchQueryArgPattern.FindStringSubmatch(query); m != nil { - logGraphQL.Printf("extractSearchQuery: found inline: %q", truncateForLog(m[1], 80)) + logGraphQL.Printf("extractSearchQuery: found inline: %q", strutil.TruncateRunes(m[1], 80)) return m[1] } logGraphQL.Print("extractSearchQuery: no search query found") diff --git a/internal/proxy/graphql_test.go b/internal/proxy/graphql_test.go deleted file mode 100644 index f0180a89..00000000 --- a/internal/proxy/graphql_test.go +++ /dev/null @@ -1,134 +0,0 @@ -package proxy - -import ( - "testing" - - "github.com/stretchr/testify/assert" -) - -// TestTruncateForLog verifies all three branches of truncateForLog: -// the early-exit for non-positive maxRunes, the no-op for short strings, -// and the actual truncation path — including correct Unicode rune handling. -func TestTruncateForLog(t *testing.T) { - tests := []struct { - name string - s string - maxRunes int - expected string - }{ - // ── maxRunes ≤ 0 always returns "" ─────────────────────────────────── - { - name: "maxRunes zero returns empty string", - s: "hello", - maxRunes: 0, - expected: "", - }, - { - name: "maxRunes negative returns empty string", - s: "hello", - maxRunes: -1, - expected: "", - }, - { - name: "maxRunes very negative returns empty string", - s: "hello", - maxRunes: -100, - expected: "", - }, - { - name: "empty string with zero maxRunes returns empty string", - s: "", - maxRunes: 0, - expected: "", - }, - - // ── string fits within maxRunes (returned unchanged) ───────────────── - { - name: "empty string with positive maxRunes returns empty string", - s: "", - maxRunes: 10, - expected: "", - }, - { - name: "string shorter than maxRunes returned unchanged", - s: "hello", - maxRunes: 10, - expected: "hello", - }, - { - name: "string exactly equal to maxRunes returned unchanged", - s: "hello", - maxRunes: 5, - expected: "hello", - }, - { - name: "single character string within limit", - s: "a", - maxRunes: 1, - expected: "a", - }, - { - name: "unicode string within limit unchanged", - s: "日本語", - maxRunes: 10, - expected: "日本語", - }, - { - name: "unicode string exactly at limit unchanged", - s: "日本語", - maxRunes: 3, - expected: "日本語", - }, - - // ── truncation path ─────────────────────────────────────────────────── - { - name: "ASCII string truncated to first N chars", - s: "hello world", - maxRunes: 5, - expected: "hello", - }, - { - name: "truncate to single rune", - s: "hello", - maxRunes: 1, - expected: "h", - }, - { - name: "unicode string truncated by rune count not byte count", - s: "日本語テスト", - maxRunes: 3, - expected: "日本語", - }, - { - name: "mixed ASCII and unicode truncated at rune boundary", - s: "hello日本語", - maxRunes: 7, - expected: "hello日本", - }, - { - name: "multibyte euro sign truncated correctly", - s: "€€€€€", - maxRunes: 3, - expected: "€€€", - }, - { - name: "4-byte emoji runes truncated correctly", - s: "😀😃😄😁", - maxRunes: 2, - expected: "😀😃", - }, - { - name: "long string truncated at exactly maxRunes runes", - s: "abcdefghij", - maxRunes: 7, - expected: "abcdefg", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := truncateForLog(tt.s, tt.maxRunes) - assert.Equal(t, tt.expected, got) - }) - } -} diff --git a/internal/proxy/handler.go b/internal/proxy/handler.go index 79bd375d..956c9b2c 100644 --- a/internal/proxy/handler.go +++ b/internal/proxy/handler.go @@ -447,7 +447,7 @@ func injectRetryAfterIfRateLimited(w http.ResponseWriter, resp *http.Response) { return } - resetAt := parseRateLimitReset(resetHeader) + resetAt := httputil.ParseRateLimitResetHeader(resetHeader) retryAfter := computeRetryAfter(resetAt) w.Header().Set("Retry-After", strconv.Itoa(retryAfter)) @@ -457,19 +457,6 @@ func injectRetryAfterIfRateLimited(w http.ResponseWriter, resp *http.Response) { resp.StatusCode, remaining, resetHeader, retryAfter) } -// parseRateLimitReset parses the X-RateLimit-Reset Unix-timestamp header. -// Returns zero time when absent or malformed. -func parseRateLimitReset(value string) time.Time { - if value == "" { - return time.Time{} - } - unix, err := strconv.ParseInt(value, 10, 64) - if err != nil { - return time.Time{} - } - return time.Unix(unix, 0) -} - // computeRetryAfter returns the number of seconds to wait before retrying. // When resetAt is in the future the delay is clamped to [1, 3600] seconds. // When resetAt is zero or in the past a default of 60 seconds is returned. diff --git a/internal/proxy/rate_limit_test.go b/internal/proxy/rate_limit_test.go index b05a19d1..73020137 100644 --- a/internal/proxy/rate_limit_test.go +++ b/internal/proxy/rate_limit_test.go @@ -85,29 +85,6 @@ func TestInjectRetryAfterIfRateLimited(t *testing.T) { }) } -// TestParseRateLimitReset verifies the Unix-timestamp header parser. -func TestParseRateLimitReset(t *testing.T) { - t.Parallel() - - t.Run("empty string returns zero", func(t *testing.T) { - t.Parallel() - assert.True(t, parseRateLimitReset("").IsZero()) - }) - - t.Run("invalid string returns zero", func(t *testing.T) { - t.Parallel() - assert.True(t, parseRateLimitReset("not-a-number").IsZero()) - }) - - t.Run("valid unix timestamp parses correctly", func(t *testing.T) { - t.Parallel() - ts := time.Now().Add(60 * time.Second) - got := parseRateLimitReset(strconv.FormatInt(ts.Unix(), 10)) - assert.False(t, got.IsZero()) - assert.Equal(t, ts.Unix(), got.Unix()) - }) -} - // TestComputeRetryAfter verifies the retry-after calculation. func TestComputeRetryAfter(t *testing.T) { t.Parallel() diff --git a/internal/server/circuit_breaker.go b/internal/server/circuit_breaker.go index 4e67f8a3..7916879d 100644 --- a/internal/server/circuit_breaker.go +++ b/internal/server/circuit_breaker.go @@ -315,17 +315,3 @@ func parseRateLimitResetFromText(text string) time.Time { } return time.Now().Add(time.Duration(secs) * time.Second) } - -// parseRateLimitResetHeader parses the Unix-timestamp value of the -// X-RateLimit-Reset HTTP header into a time.Time. -// Returns zero time when the header is absent or malformed. -func parseRateLimitResetHeader(value string) time.Time { - if value == "" { - return time.Time{} - } - unix, err := strconv.ParseInt(strings.TrimSpace(value), 10, 64) - if err != nil { - return time.Time{} - } - return time.Unix(unix, 0) -} diff --git a/internal/server/circuit_breaker_test.go b/internal/server/circuit_breaker_test.go index 99c5c325..db53e585 100644 --- a/internal/server/circuit_breaker_test.go +++ b/internal/server/circuit_breaker_test.go @@ -1,7 +1,6 @@ package server import ( - "strconv" "testing" "time" @@ -352,58 +351,6 @@ func TestParseRateLimitResetFromText(t *testing.T) { } } -// TestParseRateLimitResetHeader verifies the Unix-timestamp header parsing. -func TestParseRateLimitResetHeader(t *testing.T) { - t.Parallel() - - now := time.Now() - future := now.Add(60 * time.Second) - - tests := []struct { - name string - value string - wantZero bool - wantTime time.Time - }{ - { - name: "empty", - value: "", - wantZero: true, - }, - { - name: "invalid", - value: "not-a-number", - wantZero: true, - }, - { - name: "valid unix timestamp", - value: "1000000000", - wantZero: false, - wantTime: time.Unix(1000000000, 0), - }, - { - name: "future timestamp", - value: strconv.FormatInt(future.Unix(), 10), - wantZero: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - got := parseRateLimitResetHeader(tt.value) - if tt.wantZero { - assert.True(t, got.IsZero(), "expected zero time") - } else { - assert.False(t, got.IsZero(), "expected non-zero time") - if !tt.wantTime.IsZero() { - assert.Equal(t, tt.wantTime.Unix(), got.Unix()) - } - } - }) - } -} - // TestExtractRateLimitErrorText verifies extraction of error text from backend results. func TestExtractRateLimitErrorText(t *testing.T) { t.Parallel()