Repository navigation
Bump mcp-go to v0.45.0 and update conformance tests - #682
Merged
Merged
Conversation
- Fix ElicitationCapability type change in mcp-go v0.45.0 - Update conformance server and runner to v0.1.15 - Refactor conformance workflow to use env vars and loop Signed-off-by: craig <cbrookes@redhat.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughBumps MCP Go and conformance server versions, changes elicitation capability payloads from a generic empty struct to a typed Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![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 (1)
.github/workflows/conformance.yaml (1)
64-65: Consider removing redundant sleep.The
make deploy-conformance-servertarget already includeskubectl waitcommands with 60s and 120s timeouts (see Makefile lines 353, 357). This additional 10s sleep may be unnecessary.Proposed removal
- name: Deploy conformance server run: make deploy-conformance-server - - name: Wait for conformance server to get ready - run: sleep 10 - - name: Run MCP conformance tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/conformance.yaml around lines 64 - 65, Remove the redundant sleep step in the GitHub Actions job step named "Wait for conformance server to get ready": delete the `run: sleep 10` step because `make deploy-conformance-server` already performs `kubectl wait` with sufficient timeouts (the existing `kubectl wait` commands in the Makefile invoked by `make deploy-conformance-server` cover readiness), so keep the deployment invocation and rely on its waits instead of adding an extra sleep.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/conformance.yaml:
- Around line 69-86: Remove the nonexistent "ping" entry from the SCENARIOS
array so the conformance workflow doesn't attempt to run a v0.1.15-missing
scenario; locate the SCENARIOS declaration (the array named SCENARIOS containing
server-initialize, tools-list, ping, ...) and delete the "ping" item so the for
loop over "for scenario in \"${SCENARIOS[@]}\"; do" only iterates valid
scenarios.
---
Nitpick comments:
In @.github/workflows/conformance.yaml:
- Around line 64-65: Remove the redundant sleep step in the GitHub Actions job
step named "Wait for conformance server to get ready": delete the `run: sleep
10` step because `make deploy-conformance-server` already performs `kubectl
wait` with sufficient timeouts (the existing `kubectl wait` commands in the
Makefile invoked by `make deploy-conformance-server` cover readiness), so keep
the deployment invocation and rely on its waits instead of adding an extra
sleep.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 05b7614d-c7f5-4f15-85cb-0fd70a9d37e9
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
.github/workflows/conformance.yamlgo.modinternal/broker/upstream/mcp.gointernal/clients/clients.gotests/servers/conformance-server/Dockerfile
Signed-off-by: craig <cbrookes@redhat.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/servers/conformance-server/start.ts (1)
1-5: trim the comment blocksMost of these comments restate the next line and don't match the repo's lowercase/terse convention. Drop the obvious ones or rewrite them as terse lowercase notes only where they add context.
As per coding guidelines "Minimal, terse comments (lowercase, only when necessary) with no emojis or AI-style formatting."
Also applies to: 9-9, 18-18, 26-29, 35-35, 86-86, 113-113, 135-135
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/servers/conformance-server/start.ts` around lines 1 - 5, The comment blocks in start.ts are overly verbose and not following the repo's lowercase/terse convention; edit or remove them so only minimal, lowercase notes remain where helpful (for example replace the multi-line explanation about DNS rebinding and Host header validation with a single-line lowercase note like "disable dns-rebinding/host header validation for in-cluster use" near the express/createMcpExpressApp() usage), and similarly trim or rewrite the other listed comments (lines around 9, 18, 26-29, 35, 86, 113, 135) to terse lowercase statements or remove them if they merely restate the next line.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/servers/conformance-server/start.ts`:
- Around line 11-15: The CORS configuration in the cors(...) call is missing the
MCP protocol header, causing browser preflight failures; update the
allowedHeaders array inside the cors(...) call (the entry that currently
contains ['Content-Type', 'mcp-session-id', 'last-event-id']) to include
'mcp-protocol-version' (matching header name case used by clients) so preflight
requests allow the MCP-Protocol-Version header to be sent to the /mcp endpoint.
- Around line 25-28: The POST/GET/DELETE handlers currently return 400 for any
session header problem; change logic so that when an MCP-Session-Id header is
missing you still return 400 (unless the POST is an initialize request via
isInitializeRequest(req.body)), but if a sessionId is provided and
transports[sessionId] is falsy you must return 404 to signal a stale/unknown
session. Locate the checks around transports and sessionId in the POST handler
(where isInitializeRequest is used) and the corresponding GET and DELETE
handlers, and replace the single 400 response for missing transport with a
conditional: if (sessionId) respond 404, else respond 400 (with the same
response body/message as appropriate).
- Around line 7-15: The CORS setup currently allows all origins (origin: '*')
and the app is bound to '0.0.0.0', violating the MCP Protocol requirement to
validate the Origin header and reject invalid origins with HTTP 403; change the
createMcpExpressApp host to 'localhost' for local deployments and replace the
permissive cors() call used in app.use(...) with a custom origin-check: read the
request Origin header, compare it against the allowed origins list, and if
invalid immediately respond with res.status(403).send(...) (ensuring
exposedHeaders/allowedHeaders still include 'Mcp-Session-Id' and
'mcp-session-id' and 'last-event-id'); keep using the existing cors middleware
only when the origin passes validation so requests from invalid origins are
rejected per spec.
---
Nitpick comments:
In `@tests/servers/conformance-server/start.ts`:
- Around line 1-5: The comment blocks in start.ts are overly verbose and not
following the repo's lowercase/terse convention; edit or remove them so only
minimal, lowercase notes remain where helpful (for example replace the
multi-line explanation about DNS rebinding and Host header validation with a
single-line lowercase note like "disable dns-rebinding/host header validation
for in-cluster use" near the express/createMcpExpressApp() usage), and similarly
trim or rewrite the other listed comments (lines around 9, 18, 26-29, 35, 86,
113, 135) to terse lowercase statements or remove them if they merely restate
the next line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b57bd092-8a16-45f7-9752-9b4f12941098
📒 Files selected for processing (2)
tests/servers/conformance-server/Dockerfiletests/servers/conformance-server/start.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/servers/conformance-server/Dockerfile
Signed-off-by: craig <cbrookes@redhat.com>
jasonmadigan
approved these changes
Mar 25, 2026
Member
|
build failure but other than that all good |
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
2 participants
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.
replaces #644
Summary by CodeRabbit
Tests
Chores