Refactor SendRequestWithServerID to use a single inbound response logging path

[Copilot]

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.

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

Firewall rules blocked me from connecting to one or more addresses (expand for details)

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/grpc@v1.80.0/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 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 (admins only)

[Copilot]

Pull request overview

Refactors SendRequestWithServerID response logging to eliminate duplicated “marshal + log inbound response” blocks across transport branches by routing through a single helper, aiming to keep inbound response logging consistent and easier to maintain.

Changes:

• Added logInboundRPCResponseFromResult(...) helper in internal/mcp/connection.go to marshal (when non-nil) and log inbound responses while returning (result, err) unchanged.
• Updated SendRequestWithServerID to return via the helper rather than duplicating response logging logic.
• Added targeted tests in internal/mcp/connection_logging_test.go to validate helper pass-through behavior.

Show a summary per file

File	Description
internal/mcp/connection.go	Introduces a single helper for inbound response logging and wires SendRequestWithServerID to use it.
internal/mcp/connection_logging_test.go	Adds unit tests covering helper return-value pass-through for nil/non-nil result and error combinations.

Copilot's findings

Tip

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

• Files reviewed: 2/2 changed files
• Comments generated: 2

[Copilot]

The doc comment says this helper “marshals a response payload”, but the implementation intentionally ignores json.Marshal errors (best-effort payload). Consider updating the comment to reflect that marshaling is attempted and failures are silently ignored, so readers don’t assume payload logging is guaranteed.

Suggested change

	// logInboundRPCResponseFromResult marshals a response payload, logs the inbound response, and
	// returns the original result and error unchanged.
	// 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.

[Copilot]

These tests only assert that (result, err) are passed through unchanged; they don’t validate the new helper’s core behavior (that it actually logs and that a non-nil result is marshaled into the logged payload). Consider initializing the RPC loggers to a temp dir (e.g., JSONL) and asserting the emitted log entry contains the expected response fields, while cleaning up global logger state to avoid cross-test interference.

[lpcox]

@copilot update the PR with this review feedback #4054 (review)