ci: add golangci-lint #282
Conversation
## Walkthrough
A new GitHub Actions workflow was added to automate Go code linting using golangci-lint on pushes to the main branch and pull requests. The SSE server example was updated to use a more general HTTP context function. Minor internal improvements and test fixes were made in JSON marshaling, SSE server handling, HTTP error formatting, and test code.
## Changes
| File(s) | Change Summary |
|-----------------------------------------|------------------------------------------------------------------------------------------------------------------------------|
| .github/workflows/golangci-lint.yml, .golangci.yml | Added GitHub Actions workflow and configuration file to run golangci-lint with a preset for standard error handling exclusions.|
| examples/custom_context/main.go | Replaced `server.WithSSEContextFunc` with `server.WithHTTPContextFunc` in `ServeSSE` method of `MCPServer`. |
| mcp/types.go | Simplified `URITemplate.MarshalJSON` by calling `t.Raw()` instead of `t.Template.Raw()`. |
| server/sse.go | Updated deprecation comment for `WithSSEContextFunc` to recommend `WithHTTPContextFunc`; reformatted `http.Error` calls; added error check on JSON encoding in `writeJSONRPCError`. |
| server/server_test.go | Removed redundant type assertion on `toolsList` in `TestMCPServer_Tools` test function. |
| server/sse_test.go | Added early return after nil check in `TestSSEServer` subtest; fixed assignment target in `TestServerResponseMarshalError`. |
| client/transport/streamable_http_test.go | Added error handling for JSON encoding failures in mock HTTP server responses. |
| examples/everything/main.go | Added error handling for notification sending in `handleLongRunningOperationTool`. |
| server/session_test.go | Added explicit error check and assertion for notification send in `TestMCPServer_NotificationChannelBlocked`. |
## Possibly related PRs
- mark3labs/mcp-go#32: Introduced `WithSSEContextFunc` and context customization on transport servers, related to the SSE context function change in the example.
- mark3labs/mcp-go#29: Added `ServeHTTP` method to `SSEServer` in `server/sse.go`, related to SSE server implementation and error handling improvements.
## Suggested reviewers
- robert-jackson-gleanTip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
.github/workflows/golangci-lint.yml (2)
1-7: Consider scoping pull_request triggers
Currently the workflow runs on any pull request. To mirror the push trigger and avoid unnecessary CI runs (e.g., on long-lived branch PRs), you may want to restrict pull requests to themainbranch:on: push: branches: - main pull_request: - + branches: + - main
16-19: Pin Go version for reproducibility
Usingstablewill always pick the latest Go release, which can introduce breaking changes. For a reproducible CI environment, consider pinning to a known major/minor version (e.g.,1.20.x):- with: - go-version: stable + with: + go-version: '1.20.x'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/golangci-lint.yml(1 hunks)
🔇 Additional comments (1)
.github/workflows/golangci-lint.yml (1)
8-10: Minimal permissions are correctly scoped
Granting onlycontents: readfollows the principle of least privilege for a lint job. Nice!
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
examples/custom_context/main.go(1 hunks)mcp/types.go(1 hunks)server/server_test.go(1 hunks)server/sse.go(3 hunks)server/sse_test.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- server/server_test.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/sse.go (1)
server/errors.go (1)
ErrDynamicPathConfig(27-29)
examples/custom_context/main.go (1)
server/http_transport_options.go (1)
WithHTTPContextFunc(148-154)
🔇 Additional comments (5)
server/sse.go (3)
188-188: Accurate deprecation notice update.The deprecation comment now correctly points users to
WithHTTPContextFuncas the recommended replacement forWithSSEContextFunc, which aligns with the broader shift towards a more general HTTP context function across the codebase.
300-305: Improved error formatting for better readability.The multi-line formatting for
http.Errorimproves code readability while maintaining the same functionality.
628-633: Improved error formatting in ServeHTTP.The multi-line formatting for
http.Errorenhances readability without changing the behavior.mcp/types.go (1)
70-70: Simplified JSON marshaling logic.The code now calls
t.Raw()directly instead oft.Template.Raw(), using the embedded receiver's method rather than accessing the underlying field explicitly. This is a clean simplification that maintains the same behavior.examples/custom_context/main.go (1)
128-128: Updated to use recommended HTTP context function.The change replaces the deprecated
WithSSEContextFuncwith the recommendedWithHTTPContextFunc, aligning with the guidance in the updated deprecation notice inserver/sse.go.
| @@ -29,6 +29,7 @@ func TestSSEServer(t *testing.T) { | |||
|
|
|||
| if sseServer == nil { | |||
| t.Error("SSEServer should not be nil") | |||
| return | |||
There was a problem hiding this comment.
Added critical early return to prevent nil dereference.
This change adds a necessary return statement after detecting that sseServer is nil, which prevents potential nil pointer dereference errors in subsequent test assertions.
There was a problem hiding this comment.
Yes, thanks to the linter in the CI, we won't miss such issues in the future.
There was a problem hiding this comment.
@pottekkat Absolutely! Integrating golangci-lint into your CI pipeline is a great move that will help catch these kinds of issues automatically before they make it into the codebase. Early detection of potential nil dereferences and similar bugs will save debugging time and improve code quality. Looking forward to seeing the linter in action across the project!
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @@ -1247,7 +1248,7 @@ | |||
| WithHooks(&Hooks{ | |||
| OnAfterInitialize: []OnAfterInitializeFunc{ | |||
| func(ctx context.Context, id any, message *mcp.InitializeRequest, result *mcp.InitializeResult) { | |||
| result.Result.Meta = map[string]any{"invalid": func() {}} // marshal will fail | |||
| result.Meta = map[string]any{"invalid": func() {}} // marshal will fail | |||
There was a problem hiding this comment.
Fixed test marshal error simulation.
The assignment now correctly sets result.Meta directly instead of using an incorrect nested field reference (result.Result.Meta), properly simulating a marshal failure in the test.
There was a problem hiding this comment.
@pottekkat Thanks for confirming! The fix ensures that your test correctly simulates a JSON marshal error by directly setting result.Meta instead of the non-existent nested field. This makes the test more accurate and reliable. 👍
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.golangci.yml (1)
5-5: Add a newline at the end of the fileThe YAML linter is reporting that there's no newline at the end of the file, which is a common linting rule.
linters: exclusions: presets: - std-error-handling +🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.golangci.yml(1 hunks)client/transport/streamable_http_test.go(3 hunks)examples/everything/main.go(2 hunks)server/session_test.go(1 hunks)server/sse.go(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/sse.go
🧰 Additional context used
🪛 YAMLlint (1.35.1)
.golangci.yml
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (6)
.golangci.yml (1)
1-5: LGTM! Good linter configuration for a starting point.This configuration is a good baseline for adding golangci-lint to your CI. Using the "std-error-handling" preset is a reasonable choice to start with as it excludes common standard error handling patterns from linting, reducing false positives.
🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 5-5: no new line character at the end of file
(new-line-at-end-of-file)
examples/everything/main.go (1)
395-407: Excellent error handling improvementAdding error handling for the notification send operation is a good practice. The error is properly checked and wrapped with context about what operation failed.
server/session_test.go (1)
576-577: Good test improvement with explicit error checkingAdding explicit error checking with
require.NoErrorfor the notification send operation improves the test robustness by ensuring the operation succeeds before continuing with the test.client/transport/streamable_http_test.go (3)
49-56: Good error handling for JSON encodingAdding error checking for JSON encoding operations is a good practice. The error is properly handled by returning an appropriate HTTP error response.
68-75: Consistent error handling pattern appliedThe same robust error handling pattern has been consistently applied to this JSON encoding operation.
113-123: Consistent error handling throughoutThe error handling pattern has been consistently applied throughout the mock server implementation, which improves robustness and reliability of the tests.
| linters: | ||
| exclusions: | ||
| presets: | ||
| - std-error-handling |
There was a problem hiding this comment.
This avoids false positives on .Close() and other common cases.
| @@ -185,7 +185,7 @@ func WithSSEEndpoint(endpoint string) SSEOption { | |||
| // WithSSEContextFunc sets a function that will be called to customise the context | |||
| // to the server using the incoming request. | |||
| // | |||
| // Deprecated: Use WithContextFunc instead. This will be removed in a future version. | |||
| // Deprecated: Use WithHTTPContextFunc instead. This will be removed in a future version. | |||
| server.WithSSEContextFunc(authFromRequest), | ||
| server.WithHTTPContextFunc(authFromRequest), |
There was a problem hiding this comment.
Deprecated, examples should always use the recommended way.
Add golangci-lint to the CI.
Fix the errors reported by golangci-lint.
See this workflow run for all the errors reported by golangci-lint before they were fixed: https://github.com/mark3labs/mcp-go/actions/runs/14992236103/job/42118080591?pr=282
Only the default linters are enabled now. We can enable more in the future as needed unless anyone has strong opinions on enabling any of the other linters now. See: https://golangci-lint.run/usage/linters/
Summary by CodeRabbit