Skip to content

McpRouting.handleMcp() is ~300 lines mixing auth, session lifecycle, and JSON-RPC dispatch; extract the auth+session prologue #541

Description

@monkopedia-coder

Code-health suggestion (proactive — staged for your review)

Category: complexity

Where: core/mcp/src/jvmMain/kotlin/com/rousecontext/mcp/core/McpRouting.kthandleMcp() spans lines 648-946 (~298 lines) and carries @Suppress("LongMethod", "ReturnCount", "CyclomaticComplexMethod"). (Its siblings handleAuthorize 449+, handleToken 563+, handleAuthorizeStatus 529+ carry the same triple-suppress — this is the worst offender.)

Current: handleMcp() inlines, in one function: security-alert gate → integration resolution → rate-limit → bearer extraction+validation → client-name/client-id resolution → Unknown (#N) label resolution with lazy #345 migration → request-body read with timeout → JSON parse → notification detection → session sweep → session create/lookup/evict with ownership check (#206) → JSON-RPC dispatch. ~10 distinct concerns, many with their own early-return-401 branches, which is why all three complexity rules are suppressed.

Proposed: Extract the request-prologue into a couple of focused, testable helpers, leaving handleMcp as the orchestration skeleton. Concretely:

  • authenticateMcpRequest(ri, request): AuthOutcome — bearer parse + validateToken + clientId/clientName/label resolution + the Unknown-client labeler: monotonic "Unknown (#N)" numbering #345 lazy-migration, returning either a 401 marker or a resolved (clientId, clientLabel, clientName). Removes ~4 duplicated WWW-Authenticate+401 blocks.
  • resolveOrCreateSession(ri, method, incomingSessionId, auth): SessionResolution — the create/lookup/evict + Security: MCP session-id not bound to the access token that created it #206 ownership branch (currently ~lines 774-830).
    Each is independently unit-testable (the auth-label migration and session-ownership rules are exactly the kind of logic worth isolating), and handleMcp drops back under the LongMethod/complexity thresholds so the suppressions can come off.

Why: These handlers are the security-critical hot path (auth, session ownership, rate-limit). A 300-line function with 3 suppressed complexity warnings is where subtle auth/session bugs hide (cf. the #206 ownership and #345 label fixes already threaded through here). Extraction improves reviewability and lets the auth/session invariants be tested directly rather than only through full-request integration tests.

Effort: small-to-medium refactor (mechanical extraction; existing McpRouting integration tests guard behavior). Judge on its own — the sibling handlers can follow the same pattern later.
Filed by the nightly code-health pass. To act on it: relabel agent-workable (it'll flow into triage→work). To decline: close it — it won't be suggested again.

Metadata

Metadata

Assignees

No one assigned

    Labels

    health-suggestionProactive code-health suggestion from the nightly analysis pass (staged for review)needs-decisionRequires user input on direction, scope, or design

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions