Refactor SendRequestWithServerID to use a single inbound response logging path (#4054)

`SendRequestWithServerID` had repeated marshal+log response blocks
across transport branches, creating a maintainability hotspot and risk
of branch-level drift in logging behavior. This change centralizes that
logic into one helper while preserving existing call-site behavior.

- **What changed**
- Added `logInboundRPCResponseFromResult(...)` in
`internal/mcp/connection.go` to:
    - marshal `*Response` (when non-nil),
    - call `logInboundRPCResponse(...)`,
    - return `(result, err)` unchanged.
- Updated `SendRequestWithServerID` to use the helper for its return
path instead of duplicating response logging logic inline.

- **Why this matters**
  - Removes copy-pasted logging logic from transport handling flow.
  - Makes future changes to inbound response logging single-point edits.

- **Targeted coverage added**
- New tests in `internal/mcp/connection_logging_test.go` validate helper
pass-through behavior for:
    - non-nil response + non-nil error,
    - nil response + nil error.

```go
func logInboundRPCResponseFromResult(serverID string, result *Response, err error, shouldAttachTags bool, snapshot *AgentTagsSnapshot) (*Response, error) {
	var responsePayload []byte
	if result != nil {
		responsePayload, _ = json.Marshal(result)
	}
	logInboundRPCResponse(serverID, responsePayload, err, shouldAttachTags, snapshot)
	return result, err
}
```

> [!WARNING]
>
> <details>
> <summary>Firewall rules blocked me from connecting to one or more
addresses (expand for details)</summary>
>
> #### I tried to connect to the following addresses, but was blocked by
firewall rules:
>
> - `example.com`
> - Triggering command: `/tmp/go-build2522761549/b510/launcher.test
/tmp/go-build2522761549/b510/launcher.test
-test.testlogfile=/tmp/go-build2522761549/b510/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true
ache/go/1.25.8/x-errorsas .cfg 64/pkg/tool/linu-nilfunc -I
/tmp/go-build252-atomic -I 64/pkg/tool/linu-buildtags -o idi.go
racket.go x_amd64/compile -p flag -lang=go1.25 x_amd64/compile` (dns
block)
> - Triggering command: `/tmp/go-build3233101705/b514/launcher.test
/tmp/go-build3233101705/b514/launcher.test
-test.testlogfile=/tmp/go-build3233101705/b514/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -uns�� -unreachable=false
/tmp/go-build2522761549/b245/vet.cfg x_amd64/vet -c=4 -nolocalimports
-importcfg x_amd64/vet -ato�� -bool -buildtags x_amd64/vet -errorsas
0832 -nilfunc x_amd64/vet` (dns block)
> - `invalid-host-that-does-not-exist-12345.com`
> - Triggering command: `/tmp/go-build2522761549/b492/config.test
/tmp/go-build2522761549/b492/config.test
-test.testlogfile=/tmp/go-build2522761549/b492/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true` (dns block)
> - Triggering command: `/tmp/go-build3233101705/b496/config.test
/tmp/go-build3233101705/b496/config.test
-test.testlogfile=/tmp/go-build3233101705/b496/testlog.txt
-test.paniconexit0 -test.timeout=10m0s ortc��
pkg/mod/google.golang.org/[email protected]/internal/proxyattributes/-errorsas
stmain.go x_amd64/vet --gdwarf-5 --64 -o x_amd64/vet -o
2761549/b492/config.test -trimpath x_amd64/vet -p golang.org/x/tex--norc
-lang=go1.25 x_amd64/vet` (dns block)
> - `nonexistent.local`
> - Triggering command: `/tmp/go-build2522761549/b510/launcher.test
/tmp/go-build2522761549/b510/launcher.test
-test.testlogfile=/tmp/go-build2522761549/b510/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true
ache/go/1.25.8/x-errorsas .cfg 64/pkg/tool/linu-nilfunc -I
/tmp/go-build252-atomic -I 64/pkg/tool/linu-buildtags -o idi.go
racket.go x_amd64/compile -p flag -lang=go1.25 x_amd64/compile` (dns
block)
> - Triggering command: `/tmp/go-build3233101705/b514/launcher.test
/tmp/go-build3233101705/b514/launcher.test
-test.testlogfile=/tmp/go-build3233101705/b514/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -uns�� -unreachable=false
/tmp/go-build2522761549/b245/vet.cfg x_amd64/vet -c=4 -nolocalimports
-importcfg x_amd64/vet -ato�� -bool -buildtags x_amd64/vet -errorsas
0832 -nilfunc x_amd64/vet` (dns block)
> - `slow.example.com`
> - Triggering command: `/tmp/go-build2522761549/b510/launcher.test
/tmp/go-build2522761549/b510/launcher.test
-test.testlogfile=/tmp/go-build2522761549/b510/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true
ache/go/1.25.8/x-errorsas .cfg 64/pkg/tool/linu-nilfunc -I
/tmp/go-build252-atomic -I 64/pkg/tool/linu-buildtags -o idi.go
racket.go x_amd64/compile -p flag -lang=go1.25 x_amd64/compile` (dns
block)
> - Triggering command: `/tmp/go-build3233101705/b514/launcher.test
/tmp/go-build3233101705/b514/launcher.test
-test.testlogfile=/tmp/go-build3233101705/b514/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -uns�� -unreachable=false
/tmp/go-build2522761549/b245/vet.cfg x_amd64/vet -c=4 -nolocalimports
-importcfg x_amd64/vet -ato�� -bool -buildtags x_amd64/vet -errorsas
0832 -nilfunc x_amd64/vet` (dns block)
> - `this-host-does-not-exist-12345.com`
> - Triggering command: `/tmp/go-build2522761549/b519/mcp.test
/tmp/go-build2522761549/b519/mcp.test
-test.testlogfile=/tmp/go-build2522761549/b519/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -test.v=true
1.5.0/internal/xcontext/xcontext.go 4BJUeiabknHIHNszrg x_amd64/compile
-p e/otlptracehttp//usr/bin/runc -lang=go1.25 x_amd64/compile -o g_.a
2761549/b165/ x_amd64/vet -p hash/fnv -lang=go1.25 x_amd64/vet` (dns
block)
> - Triggering command: `/tmp/go-build1064564734/b001/mcp.test
/tmp/go-build1064564734/b001/mcp.test
-test.testlogfile=/tmp/go-build1064564734/b001/testlog.txt
-test.paniconexit0 -test.timeout=10m0s -uns�� 2761549/b402/_pkg_.a
/tmp/go-build2522761549/b126/vet-ifaceassert .cfg --global g/grpc/mem
64/bin/bash ache/go/1.25.8/x64/pkg/tool/linu-tests -qui��
ppFW/Fi3uUr-n6b91NPffppFW
k/gh-aw-mcpg/gh-aw-mcpg/internal/oidc/provider.gmain x_amd64/link .
-imultiarch x86_64-linux-gnu x_amd64/link` (dns block)
>
> If you need me to access, download, or install something from one of
these locations, you can either:
>
> - Configure [Actions setup
steps](https://gh.io/copilot/actions-setup-steps) to set up my
environment, which run before the firewall is enabled
> - Add the appropriate URLs or hosts to the custom allowlist in this
repository's [Copilot coding agent
settings](https://github.com/github/gh-aw-mcpg/settings/copilot/coding_agent)
(admins only)
>
> </details>

Author: lpcox

internal/mcp/connection.go

@@ -444,6 +444,18 @@ func logInboundRPCResponse(serverID string, payload []byte, err error, shouldAtt
 	}
 }
 
+// logInboundRPCResponseFromResult attempts to marshal a response payload for logging,
+// silently ignores marshal failures, logs the inbound response, and returns the
+// original result and error unchanged.
+func logInboundRPCResponseFromResult(serverID string, result *Response, err error, shouldAttachTags bool, snapshot *AgentTagsSnapshot) (*Response, error) {
+	var responsePayload []byte
+	if result != nil {
+		responsePayload, _ = json.Marshal(result)
+	}
+	logInboundRPCResponse(serverID, responsePayload, err, shouldAttachTags, snapshot)
+	return result, err
+}
+
 // SendRequest sends a JSON-RPC request and waits for the response
 // The serverID parameter is used for logging to associate the request with a backend server
 func (c *Connection) SendRequest(method string, params interface{}) (*Response, error) {
@@ -481,14 +493,7 @@ func (c *Connection) SendRequestWithServerID(ctx context.Context, method string,
 		result, err = c.callSDKMethod(method, params)
 	}
 
-	// Log the response from backend server
-	var responsePayload []byte
-	if result != nil {
-		responsePayload, _ = json.Marshal(result)
-	}
-	logInboundRPCResponse(serverID, responsePayload, err, shouldAttachAgentTags, snapshot)
-
-	return result, err
+	return logInboundRPCResponseFromResult(serverID, result, err, shouldAttachAgentTags, snapshot)
 }
 
 // callSDKMethod calls the appropriate SDK method based on the method name

internal/mcp/connection_logging_test.go

@@ -0,0 +1,64 @@
+package mcp
+
+import (
+	"bufio"
+	"encoding/json"
+	"errors"
+	"os"
+	"path/filepath"
+	"testing"
+
+	"github.com/github/gh-aw-mcpg/internal/logger"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func TestLogInboundRPCResponseFromResult_LogsMarshaledResponseAndReturnsResultAndError(t *testing.T) {
+	require := require.New(t)
+	assert := assert.New(t)
+	tmpDir := t.TempDir()
+	logDir := filepath.Join(tmpDir, "logs")
+
+	require.NoError(logger.InitJSONLLogger(logDir, "rpc-messages.jsonl"))
+	t.Cleanup(func() {
+		require.NoError(logger.CloseJSONLLogger())
+	})
+
+	expectedErr := errors.New("expected error")
+	expectedResult := &Response{
+		JSONRPC: "2.0",
+		ID:      1,
+		Result:  []byte(`{"ok":true}`),
+	}
+
+	result, err := logInboundRPCResponseFromResult("test-server", expectedResult, expectedErr, false, nil)
+
+	assert.Same(expectedResult, result)
+	assert.ErrorIs(err, expectedErr)
+
+	logFile, err := os.Open(filepath.Join(logDir, "rpc-messages.jsonl"))
+	require.NoError(err)
+	defer logFile.Close()
+
+	scanner := bufio.NewScanner(logFile)
+	require.True(scanner.Scan(), "expected a JSONL entry to be logged")
+
+	var entry logger.JSONLRPCMessage
+	require.NoError(json.Unmarshal([]byte(scanner.Text()), &entry))
+	assert.Equal(string(logger.RPCDirectionInbound), entry.Direction)
+	assert.Equal(string(logger.RPCMessageResponse), entry.Type)
+	assert.Equal("test-server", entry.ServerID)
+	assert.Equal(expectedErr.Error(), entry.Error)
+	assert.JSONEq(`{"jsonrpc":"2.0","id":1,"result":{"ok":true}}`, string(entry.Payload))
+	assert.False(scanner.Scan(), "expected exactly one JSONL entry")
+	require.NoError(scanner.Err())
+}
+
+func TestLogInboundRPCResponseFromResult_AllowsNilResult(t *testing.T) {
+	assert := assert.New(t)
+
+	result, err := logInboundRPCResponseFromResult("test-server", nil, nil, false, nil)
+
+	assert.Nil(result)
+	assert.NoError(err)
+}