Skip to content

Refactor shared pagination/map-extraction paths and isolate server rate-limit helpers#6799

Merged
lpcox merged 2 commits into
mainfrom
copilot/refactor-semantic-function-clustering
May 31, 2026
Merged

Refactor shared pagination/map-extraction paths and isolate server rate-limit helpers#6799
lpcox merged 2 commits into
mainfrom
copilot/refactor-semantic-function-clustering

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented May 31, 2026

This change addresses the semantic-clustering refactor findings by reducing repeated helper logic and improving locality of concern. It keeps behavior stable while making shared invariants and extraction paths explicit.

  • Rate-limit logic isolation (internal/server)

    • Moved rate-limit parsing/detection helpers out of circuit_breaker.go into rate_limit.go:
      • isRateLimitToolResult
      • isRateLimitText
      • parseRateLimitResetFromText
      • extractRateLimitErrorText
    • Leaves circuit breaker flow focused on state transitions and timing decisions.
  • Shared string extraction for map[string]interface{} (internal/strutil, internal/proxy)

    • Added strutil.GetStringFromMap(m, key) and unit tests.
    • Replaced inline string assertions in proxy argument extraction paths:
      • REST-style owner/repo/number extraction
      • collaborator permission arg parsing
      • GraphQL owner/repo variable extraction
  • Pagination invariant coupling made explicit

    • Added cross-reference comments between:
      • internal/mcp/pagination.go:paginateAll
      • internal/testutil/mcptest/validator.go:paginate
    • Documents intentional duplication of loop/cursor safety invariants so future edits stay aligned.
// before
owner, _ := argsMap["owner"].(string)
repo, _ := argsMap["repo"].(string)

// after
owner := strutil.GetStringFromMap(argsMap, "owner")
repo := strutil.GetStringFromMap(argsMap, "repo")

Copilot AI changed the title [WIP] Refactor identified opportunities in semantic function clustering analysis Refactor shared pagination/map-extraction paths and isolate server rate-limit helpers May 31, 2026
Copilot finished work on behalf of lpcox May 31, 2026 13:34
Copilot AI requested a review from lpcox May 31, 2026 13:34
@lpcox lpcox marked this pull request as ready for review May 31, 2026 13:46
Copilot AI review requested due to automatic review settings May 31, 2026 13:46
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR refactors repeated helper logic without changing behavior, improving locality for server rate-limit handling and shared proxy map-string extraction while documenting pagination invariants.

Changes:

  • Moved server-side rate-limit result parsing helpers into internal/server/rate_limit.go.
  • Added strutil.GetStringFromMap with unit coverage and adopted it in selected proxy argument extraction paths.
  • Added cross-reference comments between production and test pagination helpers.
Show a summary per file
File Description
internal/server/rate_limit.go New home for unexported rate-limit parsing/detection helpers.
internal/server/circuit_breaker.go Removes moved helper implementations and unused imports.
internal/strutil/map.go Adds shared map string extraction helper.
internal/strutil/map_test.go Covers helper behavior for present, missing, non-string, and nil map cases.
internal/proxy/proxy.go Uses shared helper for REST owner/repo/number string extraction.
internal/proxy/graphql.go Uses shared helper for GraphQL owner/repo variable extraction.
internal/proxy/collaborator_permission.go Uses shared helper for collaborator permission arguments.
internal/mcp/pagination.go Documents alignment with test pagination loop-protection invariants.
internal/testutil/mcptest/validator.go Documents alignment with production pagination loop-protection invariants.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 9/9 changed files
  • Comments generated: 0

@lpcox lpcox merged commit b295ab6 into main May 31, 2026
29 checks passed
@lpcox lpcox deleted the copilot/refactor-semantic-function-clustering branch May 31, 2026 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[refactor] Semantic function clustering analysis: 4 refactoring opportunities identified

3 participants