-
Notifications
You must be signed in to change notification settings - Fork 776
feat: Auto-detect transport type from config files #661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
🎭 Playwright E2E Test Results✅ 24 passed Details24 tests across 3 suites 📊 View Detailed HTML Report (download artifacts) |
f2dfe50
to
50b96a0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @felixweinberger, thanks for this!
-
There is a stalled PR that is working toward this. I'm mentioning it here so it can be closed if this one is merged. It is trying to modify
client/bin/start.js
though. -
I'm testing this locally, and I realized
start.js
is not going to be the right place butcli.js
is, since it's the entrypoint for the inspector package via npx. And placing this hook incli.js
, the common launching script, this should work with the CLI inspector as well, though I'm not certain that's the case. I saw you added tests incli-tests
and that gave me the impression it would be the case, but... -
CLI tests are failing. Turns out the CLI tests are launching the web client. Those tests are meant to test the CLI client. they should just run and be done, without having to interact with the browser. Tests of launching the web client would be more appropriate in
client/e2e
where the Playwright tests happen and can run in CI.

- I notice in your example
mcp_dev_config.json
you provide adefault-server
, so I tried that and found that you cannot launch with--config
and fail to specify a server name. It feels like this should be possible if there is adefault-server
configured. Maybe the if not present on the command line, the script can grep the file for 'default-server' and if present in the file, use it as the server arg.


- Verified that I could launch if I added a
--server
argument


In #643 there is a setup for Playwright testing of the command line arguments using I created that setup to write tests to catch the CLI options bug #649 . PR #664 fixes that bug. Note the Playwright CLI tests PR is currently on top of the bug fix so that I could verify the tests caught the bug, but passed after the fix. I think all of this should be ready for review, if it's helpful here. I thought a full end-to-end test of entry-point CLI arguments to UI would be helpful as a catch-all since the Inspector is several components. (I had experience with Playwright from #582 to test more of the UI.). Tighter tests around just the CLI would be helpful too. (I also noticed the other new "CLI tests" are not same CLI. 😂 ) |
50b96a0
to
9d8c2ec
Compare
When launching the inspector with --config, automatically set the transport dropdown and server URL based on the config file contents. This eliminates the need to manually switch between stdio/sse/streamable-http in the UI. - Use discriminated union for ServerConfig to properly type different transports - Detect transport type and URL from config, pass via query params - Maintain backwards compatibility for configs without explicit 'type' field - Add comprehensive tests for the new functionality Improves UX by making the config file the single source of truth for server settings. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add tests for config files with different transport types (stdio, sse, streamable-http) - Add test for backward compatibility with configs missing type field - Verify transport and serverUrl options are correctly parsed from config files - All 36 tests pass
Move transport configuration parsing to cli.js as suggested in PR review. The CLI entry point (cli.js) should handle config file parsing and pass parameters to start.js.
- Remove web client launch tests from CLI test suite - Add CLI-mode tests for SSE and streamable-http configs - Keep only tests that use --cli flag as intended
- Test that transport and serverUrl parameters are correctly passed via URL - Verify SSE and streamable-http transport types are handled - Test default STDIO behavior
- Auto-select server when --config is used without --server - Use 'default-server' if it exists in config - Use single server if only one is defined - Show helpful error if multiple servers exist without default - Add tests for all default-server scenarios
- Add examples for stdio, sse, and streamable-http transport types - Document automatic server selection behavior - Show how to use default-server in config files
d175c34
to
1e399b7
Compare
- Pass URL as command for SSE/HTTP configs instead of empty string - Update runCli to pass transport type flag when needed - Update tests to expect connection errors for non-existent SSE/HTTP servers
c288e03
to
195f790
Compare
Thanks for the reviews - I've updated the PR to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixweinberger Tests are looking good now.
I still had a problem launching without specifying the server.
mcp.json
{
"mcpServers" : {
"default-server": {
"command": "node",
"args": [
"/Users/cliffhall/Projects/mcp-c64/dist/server/index.js"
]
}
}
}
node cli/build/cli.js --config mcp.json
node cli/build/cli.js --config mcp.json --server default-server

- Add --transport and --server-url support to client/bin/start.js - Pass transport configuration through server to client via /config endpoint - Update client App.tsx to use defaultTransport and defaultServerUrl from server - Ensure SSE and HTTP transport configs work properly from CLI
@cliffhall thanks for taking a detailed look, good catch! I used your
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @felixweinberger, the config file is working great on my machine now, and I'm going to do a proper review of the code momentarily, but would you mind adding mcp.json
to the .gitignore
and .prettierignore
in this PR?
I am just realizing that I will have one present for testing, and I expect @olaservo and other maintainers and contributors will as well. We don't want anyone to accidentally commit their file. And running prettier-check
(automatically part of running tests) fails locally because of my mcp.json
, which is what triggered this suggestion.
Also, from the playwright tests, I tried this command line:
npx . --transport sse --server-url http://localhost:3000/sse
- Add --transport option to command line parser - Support --transport stdio/sse/http flags - Convert 'http' to 'streamable-http' internally - Fix transport type conversion for CLI mode
Add --server-url option to CLI parser so the flag is properly recognized and passed through to the server process via the /config endpoint.
🤦 forgot to actually parse Tested with multiple different combinations:
![]()
![]() ![]()
![]()
![]()
![]() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Felix!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@felixweinberger thanks for handling those things. It's all looking good to me so far. I see @ochafik has a few requests, but I'm onboard to approve as soon as that's settled. Also, I think you're inheriting the manual args parsing stuff from the original cli.ts author, and if you want to punt that to another PR, it would fully make sense; it could be a bigger can of worms than you bargained for in this PR.
Test 30 now expects an error when multiple servers exist, even with "default-server" present, aligning with the simplified logic that only auto-selects when there's exactly one server.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Motivation and Context
When launching the inspector with
--config
, automatically set the transportdropdown and server URL based on the config file contents. This eliminates
the need to manually switch between
stdio/sse/streamable-http
in the UI.type
fieldImproves UX by making the config file the single source of truth for
server settings.
How Has This Been Tested?
Tested manually with different configs:
E.g. with a
stdio
based config:E.g. with a
streamable-http
based config:Breaking Changes
No.
Types of changes
Checklist
Additional context