Part of duplicate code analysis: #3931

Summary

The SendRequestWithServerID function in internal/mcp/connection.go contains three identical 5-line blocks that marshal a response to JSON and call logInboundRPCResponse. This pattern is copy-pasted for each of the three transport branches (plain JSON-RPC, SDK/SSE+streamable, and stdio), meaning any future change to response logging must be made in three places.

Duplication Details

Pattern: Repeated response-logging block across transport branches

• Severity: Medium
• Occurrences: 3
• Locations:
  • internal/mcp/connection.go (lines 467–472) — plain JSON-RPC path
  • internal/mcp/connection.go (lines 478–483) — SDK streamable/SSE path
  • internal/mcp/connection.go (lines 490–496) — stdio path
• Code Sample (identical in all three branches):

  var responsePayload []byte
  if result != nil {
      responsePayload, _ = json.Marshal(result)
  }
  logInboundRPCResponse(serverID, responsePayload, err, shouldAttachAgentTags, snapshot)
  return result, err

Impact Analysis

• Maintainability: Any change to how inbound responses are logged (e.g., adding tracing, changing serialization) requires the same edit in three places with no compiler enforcement
• Bug Risk: Divergence between the three blocks over time (e.g., one block modified but not the others) could introduce subtle logging inconsistencies
• Code Bloat: Minor (~15 extra lines), but the function is already long and the repetition obscures the meaningful difference between branches (which dispatch call is made)

Refactoring Recommendations

1. Extract a shared helper or restructure to a single return path

   Option A — helper function:

   func logAndReturn(serverID string, result *Response, err error, attach bool, snap *AgentTagsSnapshot) (*Response, error) {
       var payload []byte
       if result != nil {
           payload, _ = json.Marshal(result)
       }
       logInboundRPCResponse(serverID, payload, err, attach, snap)
       return result, err
   }

   Then replace each block with:

   return logAndReturn(serverID, result, err, shouldAttachAgentTags, snapshot)

   Option B — single fall-through return:
   Restructure SendRequestWithServerID so all three branches assign result, err and fall through to a single logging+return block at the end of the function (removing early return statements inside the if c.isHTTP blocks).

2. Estimated effort: ~30 minutes; low risk change

3. Benefits: Single place to update response logging logic, shorter function, clearer control flow

Implementation Checklist

• Review duplication findings
• Choose refactoring approach (helper function vs. single return path)
• Implement changes in internal/mcp/connection.go
• Run make test to verify no functionality broken
• Verify logging output is unchanged

Parent Issue

See parent analysis report: #3931
Related to #3931

Generated by Duplicate Code Detector · ● 1.4M · ◷

• expires on Apr 23, 2026, 6:12 AM UTC