Conversation
…sponse logging The three identical if/else blocks in SendRequestWithServerID that dispatch to LogRPCResponseWithAgentSnapshot vs LogRPCResponse are replaced with a single unexported logInboundRPCResponse helper. No functional change. Closes #3828 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors internal/mcp/connection.go to remove a duplicated inbound RPC response logging block in SendRequestWithServerID by extracting the shared logic into a small unexported helper, improving maintainability without changing behavior.
Changes:
- Added
logInboundRPCResponsehelper to centralize inbound RPC response logging (with optional agent DIFC tag snapshots). - Replaced three identical
if shouldAttachAgentTags { ... } else { ... }blocks with single-line helper calls across the HTTP/plain JSON, HTTP/SDK (streamable/SSE), and stdio code paths.
Show a summary per file
| File | Description |
|---|---|
| internal/mcp/connection.go | Extracts duplicated inbound response logging into logInboundRPCResponse and updates all three call sites. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 1/1 changed files
- Comments generated: 0
This was referenced Apr 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 This is a Repo Assist automated pull request.
Summary
Addresses the duplicate code pattern identified in #3828.
The three identical
if shouldAttachAgentTags { ... } else { ... }blocks inSendRequestWithServerIDare replaced with calls to a new unexported helperlogInboundRPCResponse. No functional change.Root cause
SendRequestWithServerIDhandles three code paths (plain JSON-RPC HTTP, SDK streamable/SSE HTTP, and stdio), each ending with the same 5-line conditional to dispatch logging. Any future change to the response logging signature — adding a tag type, changing the direction argument — had to be applied to three identical sites.Fix
Added
logInboundRPCResponse(serverID string, payload []byte, err error, shouldAttachTags bool, snapshot *AgentTagsSnapshot)just beforeSendRequest. Each of the three call sites is now a single line.When
shouldAttachTagsis true,snapshotis guaranteed non-nil: the caller setsshouldAttachAgentTags := hasSnapshot && difc.IsSinkServerID(serverID), soshouldAttachAgentTagscan only be true whenGetAgentTagsSnapshotFromContextreturned a non-nil result.Trade-offs
None. This is a pure mechanical refactor — the emitted log calls are byte-for-byte identical to before.
Test Status
The change is a pure refactor with no functional impact. Build and tests could not be run locally due to
proxy.golang.orgbeing blocked by the firewall in this environment. The CI pipeline will confirm correctness. The diff reducesconnection.goby 2 net lines while removing the maintenance hazard.Closes #3828
Warning
The following domains were blocked by the firewall during workflow execution:
go.opentelemetry.iogo.yaml.ingolang.orggoogle.golang.orggopkg.inproxy.golang.orgTo allow these domains, add them to the
network.allowedlist in your workflow frontmatter:See Network Configuration for more information.