Eliminate duplicate code patterns in JSON-RPC parsing and config initialization#1022
Merged
Eliminate duplicate code patterns in JSON-RPC parsing and config initialization#1022
Conversation
…sing logic Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…ncing Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
…tion Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix duplicate code issues in Go codebase
Eliminate duplicate code patterns in JSON-RPC parsing and config initialization
Feb 17, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors duplicated logic across MCP HTTP JSON-RPC handling and configuration loading by extracting small helper functions, reducing repeated code paths and centralizing defaulting/parsing behavior.
Changes:
- Added
parseJSONRPCResponseWithSSE()to consolidate JSON-RPC response parsing with SSE fallback, used by bothinitializeHTTPSession()andsendHTTPRequest(). - Added
intPtrOrDefault()to simplify optional integer field handling in stdin JSON config conversion. - Added
applyGatewayDefaults()to centralize gateway default initialization across TOML and stdin JSON config loading.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| internal/mcp/connection.go | Extracts shared JSON-RPC parsing (with SSE fallback) and updates HTTP call sites to use it. |
| internal/config/config_stdin.go | Introduces intPtrOrDefault() and uses applyGatewayDefaults() to reduce duplication in stdin config conversion. |
| internal/config/config_core.go | Introduces applyGatewayDefaults() and uses it during TOML config loading. |
Comments suppressed due to low confidence (1)
internal/mcp/connection.go:111
- Same issue as above: in the non-OK status branch after SSE extraction succeeds but JSON unmarshal fails, ResponseError.Data is set to json.RawMessage(body) even though body may be SSE/plain text. Ensure Data is always valid JSON (e.g., JSON-string/object wrapper) so encoding/marshalling the Response cannot fail.
if statusCode != http.StatusOK {
logConn.Printf("HTTP error status=%d, SSE data cannot be parsed as JSON-RPC", statusCode)
return &Response{
JSONRPC: "2.0",
Error: &ResponseError{
Code: -32603, // Internal error
Message: fmt.Sprintf("HTTP %d: %s", statusCode, http.StatusText(statusCode)),
Data: json.RawMessage(body),
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+82
to
+87
| return &Response{ | ||
| JSONRPC: "2.0", | ||
| Error: &ResponseError{ | ||
| Code: -32603, // Internal error | ||
| Message: fmt.Sprintf("HTTP %d: %s", statusCode, http.StatusText(statusCode)), | ||
| Data: json.RawMessage(body), |
There was a problem hiding this comment.
ResponseError.Data is assigned json.RawMessage(body) in the branch where the body is known to be non-JSON (json.Unmarshal failed and SSE extraction failed). json.RawMessage must contain valid JSON bytes; otherwise json.Marshal / json.Encoder will fail (e.g., when proxying this Response back to clients or logging it). Consider wrapping the raw body as a JSON string (or an object with a base64/string field) so the response can always be encoded safely.
This issue also appears on line 103 of the same file.
Suggested change
| return &Response{ | |
| JSONRPC: "2.0", | |
| Error: &ResponseError{ | |
| Code: -32603, // Internal error | |
| Message: fmt.Sprintf("HTTP %d: %s", statusCode, http.StatusText(statusCode)), | |
| Data: json.RawMessage(body), | |
| rawBodyJSON, marshalErr := json.Marshal(string(body)) | |
| if marshalErr != nil { | |
| // Fall back to an empty JSON string to ensure ResponseError.Data is always valid JSON. | |
| logConn.Printf("failed to marshal HTTP error body as JSON string: %v", marshalErr) | |
| rawBodyJSON = []byte(`""`) | |
| } | |
| return &Response{ | |
| JSONRPC: "2.0", | |
| Error: &ResponseError{ | |
| Code: -32603, // Internal error | |
| Message: fmt.Sprintf("HTTP %d: %s", statusCode, http.StatusText(statusCode)), | |
| Data: json.RawMessage(rawBodyJSON), |
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.
Automated analysis identified 3 duplication patterns totaling ~150 lines across critical MCP protocol handling and configuration code. This refactoring extracts helper functions to eliminate the duplication.
Changes
JSON-RPC Response Parsing (118+ lines → 1 helper)
parseJSONRPCResponseWithSSE()consolidating SSE format fallback logicinitializeHTTPSession()andsendHTTPRequest()Optional Config Field Assignment (9 lines → 1 helper)
intPtrOrDefault()for pointer dereferencing with defaultsif ptr != nil { cfg.Field = *ptr }patternGateway Default Initialization (duplicate assignments → 1 helper)
applyGatewayDefaults()centralizing Port/StartupTimeout/ToolTimeout defaultsExample
Before:
After:
No behavioral changes. All tests passing.
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/tmp/go-build4017383299/b275/launcher.test /tmp/go-build4017383299/b275/launcher.test -test.testlogfile=/tmp/go-build4017383299/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go pull.rebase 64/pkg/tool/linu-o(dns block)/tmp/go-build3792116991/b275/launcher.test /tmp/go-build3792116991/b275/launcher.test -test.testlogfile=/tmp/go-build3792116991/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true p/go-build kP-l/WrPb7ILNcQOx2pc8kP-l x_amd64/vet(dns block)/tmp/go-build2064399090/b275/launcher.test /tmp/go-build2064399090/b275/launcher.test -test.testlogfile=/tmp/go-build2064399090/b275/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true submodules | head -n 10 thored-by: lpcox <15877973+lpcox@users.noreply.github.com> ker/cli-plugins/docker-buildx go /exp.go 64/pkg/tool/linu--64 ker/cli-plugins/docker-buildx n-me�� (create|run) 64/pkg/tool/linu/tmp/go-build4017383299/b065/vet.cfg ker/cli-plugins/docker-compose go +lpcox@users.nor-c 64/pkg/tool/linugit status --porcelain --ignore-submodules | head -n 10 ker/cli-plugins/docker-compose(dns block)invalid-host-that-does-not-exist-12345.com/tmp/go-build4017383299/b260/config.test /tmp/go-build4017383299/b260/config.test -test.testlogfile=/tmp/go-build4017383299/b260/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true go ternal/randutil/--64 64/pkg/tool/linu-o(dns block)/tmp/go-build807840348/b001/config.test /tmp/go-build807840348/b001/config.test -test.testlogfile=/tmp/go-build807840348/b001/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net -trimpath 64/pkg/tool/linux_amd64/vet -p ions =0 64/pkg/tool/linux_amd64/vet(dns block)Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com> -W -I /tmp/go-build4017383299/b165/ test.test . --gdwarf2 --64 test.test` (dns block)
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.