feat - incorporated mcp server request configs for cli mode #373

Kavyapriya-1804 · 2025-05-03T20:39:35Z

This PR implements the feature requested by #330

Motivation and Context

When using the Inspector in CLI mode, there's currently no way to configure Inspector-specific settings like MCP_SERVER_REQUEST_TIMEOUT. This limitation forces users to work with default timeout values, which may not be suitable for all use cases, especially when dealing with slower MCP servers or long-running operations. This PR adds support to accept the mcp server request's configs to be set from CLI in form of flags and values.

The below configs have been added

Setting	Description	Default
`MCP_SERVER_REQUEST_TIMEOUT`	Timeout for requests to the MCP server (ms)	10000
`MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS`	Reset timeout on progress notifications	true
`MCP_REQUEST_MAX_TOTAL_TIMEOUT`	Maximum total timeout for requests sent to the MCP server (ms) (Use with progress notifications)	60000

Sample usage for a remote everything-server:

mcp-inspector-cli --cli http://localhost:3001/sse --method tools/list --request-timeout 1 --reset-timeout-on-progress true --max-total-timeout 5000

How Has This Been Tested?

I have tested the MCP_SERVER_REQUEST_TIMEOUT with listing tools as below. Successfully passes or fails according to the timeout duration given.

mcp-inspector-cli --cli http://localhost:3001/sse --method tools/list --request-timeout 1 --reset-timeout-on-progress true --max-total-timeout 5000

Tested MCP_REQUEST_MAX_TOTAL_TIMEOUT with calling a tool (longRunningOperation) from everything-server as below. But not able to test the failure case for this.

mcp-inspector-cli \  --cli http://localhost:3001 \
  --method tools/call \
  --tool-name longRunningOperation \
  --tool-arg duration=10 \
  --tool-arg steps=10 \
  --request-timeout 1000 \
  --reset-timeout-on-progress true \
  --max-total-timeout 1

MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS has been implemented the same way but not able to test the failure scenarios

Breaking Changes

This PR does not introduce any breaking changes

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Documentation update

Checklist

I have read the MCP Documentation
My code follows the repository's style guidelines
New and existing tests pass locally
I have added appropriate error handling
I have added or updated documentation as needed

Additional context

The implementation of passing the 3 server configurations has been done like how its done for the mcp web mode. Checked the implementation of connect (class Protocol) and request (class Client) methods in typescript-sdk repo of mcp as well. In both cases RequestOptions are accepted the same way.

Could you please suggest further ways to test MCP_REQUEST_MAX_TOTAL_TIMEOUT and MCP_REQUEST_TIMEOUT_RESET_ON_PROGRESS ?

Kavyapriya-1804 · 2025-05-04T13:22:48Z

synced the PR @olaservo @cliffhall @pulkitsharma07

olaservo · 2025-05-05T12:04:54Z

Hi @Kavyapriya-1804 I think one way we can run tests on these changes is to add a script similar to this one: cli\scripts\cli-tests.js

If you look at the package.json for the cli folder, the test script is running that script. It runs specific testing scenarios through the CLI using the Everything server as a test server, which is one we use a lot for validation.

I created an example here. The only issue I had with this example is that I can't get the Test 5: Reset timeout on progress disabled - should fail with timeout scenario to fail as expected, but that could be an issue with the test script and not the implementation. Hopefully it gives you an idea of how to test this though.

I was running the above example script by adding it to the package.json in the cli folder, eg "test:timeouts": "node scripts/timeout-tests.js",

Kavyapriya-1804 · 2025-05-11T15:23:36Z

Hi @olaservo,
Thanks for the test cases !

I was able to run them successfully. But wasn't able to get 2/ 9 test cases right (Test 4: Reset timeout on progress enabled - should complete successfully and Max total timeout exceeded (should fail)).
I guess this is because we are not able to capture the progress notifications from the CLI. When I looked into the CLI implementation of tools/call too, I wasn't able to find any code related to capturing notifications of a progress.

Kindly suggest further..

cliffhall · 2025-05-12T15:58:01Z

Hi @olaservo, Thanks for the test cases !

I was able to run them successfully. But wasn't able to get 2/ 9 test cases right (Test 4: Reset timeout on progress enabled - should complete successfully and Max total timeout exceeded (should fail)). I guess this is because we are not able to capture the progress notifications from the CLI. When I looked into the CLI implementation of tools/call too, I wasn't able to find any code related to capturing notifications of a progress.

Kindly suggest further..

@Kavyapriya-1804 Maybe this suggests we need to add those features to the CLI? What would that look like?

Kavyapriya-1804 · 2025-05-12T17:49:52Z

Hi @cliffhall , @olaservo, @pulkitsharma07
I don't see any implementation in cli that captures and logs notifications when using CLI mode in terminal.
Though I could see server notifications being listened and shown in UI in inspector web as below.

Implementing this feature could be helpful to receive updates likes messages and progress on long running tool calls.

Would you like to have them implemented as part of this PR ?
What are your thoughts on this ?

cliffhall · 2025-05-12T19:21:45Z

Hi @cliffhall , @olaservo, @pulkitsharma07 I don't see any implementation in cli that captures and logs notifications when using CLI mode in terminal.
...
Would you like to have them implemented as part of this PR ? What are your thoughts on this ?

I think the CLI should handle logs and notifications in some way, but I'm not sure how. Presumably they would be well identifiable and parsable from the STDOUT/STDERR of the CLI, so that tests can verify that they appear there and are correct. Play with what they might look like in the output and make a proposal here for that. Let us see some mockups of an output stream with these notifications mixed in.

Kavyapriya-1804 · 2025-05-14T08:16:26Z

Sure @cliffhall.
Will analyze and extend the proposal for including notification in the output stream

cc: @olaservo

olaservo · 2025-05-28T12:48:54Z

Hi @Kavyapriya-1804 , just wanted to mention I think we can leave out the notifications piece and just focus on the supported features + configs for now since it probably made these changes a lot bigger. What do you think?

…uest-configs

Kavyapriya-1804 · 2025-05-28T17:27:27Z

Hi @olaservo, yeah sure. Would work on the notifications part in a separate issue.
For now, the code is good with passing 7/9 test cases you gave. Quoting the below detail from our previous conversation ..

I was able to run them successfully. But wasn't able to get 2/ 9 test cases right (Test 4: Reset timeout on progress enabled - should complete successfully and Max total timeout exceeded (should fail)).
I guess this is because we are not able to capture the progress notifications from the CLI. When I looked into the CLI implementation of tools/call too, I wasn't able to find any code related to capturing notifications of a progress.

Please merge if it looks good to you. Thanks !

Kavyapriya-1804 · 2025-05-30T16:42:44Z

Hi @olaservo @cliffhall
The PR is now in sync with the main branch. Please review and merge this.

cliffhall · 2025-05-30T22:55:18Z

@Kavyapriya-1804 With the everything server running in SSE mode, the command you included, I get the following:

Could you please give a clear and complete set of instructions for testing against server-everything ? I.e., setup and test cases for stdio, streamableHttp, and sse, with the various settings and whether failure or success is expected? I don't want to have to guess about this. It would be good if you included screenshots of the results of each test on your system, so that we can compare our results to yours in each case.

olaservo · 2025-06-01T20:18:42Z

@cliffhall here's the original suggestion I made for testing that we were referencing for this PR:

I think one way we can run tests on these changes is to add a script similar to this one: cli\scripts\cli-tests.js

If you look at the package.json for the cli folder, the test script is running that script. It runs specific testing scenarios through the CLI using the Everything server as a test server, which is one we use a lot for validation.

I created an example here. The only issue I had with this example is that I can't get the Test 5: Reset timeout on progress disabled - should fail with timeout scenario to fail as expected, but that could be an issue with the test script and not the implementation. Hopefully it gives you an idea of how to test this though.

I was running the above example script by adding it to the package.json in the cli folder, eg "test:timeouts": "node scripts/timeout-tests.js",

As long as we're OK with waiting on the timeout support for progress notifications, I think we could add a modified version of this ^^ script to the repo and/or modify the existing to include these cases?

cliffhall · 2025-06-02T16:19:10Z

@Kavyapriya-1804 could you look at incorporating the script @olaservo mentioned above into the existing tests?

…uest-configs

Kavyapriya-1804 · 2025-06-03T18:14:05Z

Sure, will add @cliffhall

Kavyapriya-1804 · 2025-06-21T12:28:50Z

Hi @cliffhall ,
That might be a node version issue or due to missing dependencies.
I just npm installed at root and cli levels.
It seems to run all the test cases fine.

olaservo · 2025-06-28T04:30:47Z

Hi @Kavyapriya-1804 , apologies but it looks like the other changes for http support created some conflicts here - let me know if you want me to resolve them, I know this has been pending a while and this probably should have gone first. Thanks!

Kavyapriya-1804 · 2025-06-28T14:28:25Z

Hi @olaservo,
I see the conflicts from the 2 files.
Yeah, I really appreciate if you could resolve those conflicts. Thanks !

…Kavyapriya-1804/inspector into feat-cli-server-request-configs

cliffhall · 2025-07-01T15:04:15Z

@Kavyapriya-1804 all the tests are running and I think the changes look good, but one outstanding thing is:

You removed the logging but left the TOTAL_TESTS var at line 27 and it's incrementing at what was lines 184 and 295. Since it was only used for the logging it should be removed as well.

Kavyapriya-1804 · 2025-07-05T16:34:26Z

@Kavyapriya-1804 all the tests are running and I think the changes look good, but one outstanding thing is:

You removed the logging but left the TOTAL_TESTS var at line 27 and it's incrementing at what was lines 184 and 295. Since it was only used for the logging it should be removed as well.

Hi @cliffhall,
The TOTAL_TESTS increment is not a part of my change as I have mentioned in the above conversation

Please let me know if we need something else in this regard.

Kavyapriya-1804 · 2025-07-07T14:13:16Z

Updated this branch with latest from main.
Guess its good to merge now @cliffhall, @olaservo

Not needed

cliffhall

We could use some documentation of how to set these request configs in the README.md.

Kavyapriya-1804 · 2025-07-14T17:37:45Z

Documentation updated @cliffhall 👍

Changes were applied

cliffhall

Documentation tweak and questions about missing tests.

cliffhall · 2025-07-15T19:12:38Z

README.md

+# With request timeout configs (in milliseconds) - max total timeout
+npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=15 steps=5 --max-total-timeout 30000
+


--max-total-timeout really only makes sense when combined with --request-timeout and --reset-timeout-on-progress

Suggested change

# With request timeout configs (in milliseconds) - max total timeout

npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=15 steps=5 --max-total-timeout 30000

# With request timeout configs (in milliseconds) - max total timeout (used with request timeout and reset timeout on progress)

npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=35 steps=5 --reset-timeout-on-progress true --request-timeout 7000 --max-total-timeout 35000

Agreed. This can be done.

cliffhall · 2025-07-15T19:15:08Z

cli/scripts/cli-tests.js

+  // console.log(
+  //   `\n${colors.YELLOW}=== Running Progress-Related Timeout Tests ===${colors.NC}`,
+  // );
+
+  // // Test 32: Reset timeout on progress disabled - should fail with timeout
+  // await runErrorTest(
+  //   "reset_timeout_disabled",
+  //   "should_timeout_quickly",
+  //   TEST_CMD,
+  //   ...TEST_ARGS,
+  //   "--cli",
+  //   "--method",
+  //   "tools/call",
+  //   "--tool-name",
+  //   "longRunningOperation",
+  //   "--tool-arg",
+  //   "duration=15", // Same configuration as above
+  //   "steps=5",
+  //   "--request-timeout",
+  //   "2000",
+  //   "--reset-timeout-on-progress",
+  //   "false", // Only difference is here
+  //   "--max-total-timeout",
+  //   "30000",
+  // );


What's the gap on getting this test to work? And will we have a test of --reset-timeout-on-progress true combined with --max-total-timeout? The PR description says you're not able to test the failure scenarios, but can you test for success?

I was not able to get the test implementations right for these 2 params which I have been informing right from the initial days of this PR. It has been 2 months and a lot of code changes, so I am not sure of the current progress for this change.

Really appreciate your hanging in there @Kavyapriya-1804!

It would be good to see a test that shows --request-timeout and --reset-timeout-on-progress true combined with --max-total-timeout actually works. It's the key to managing long running operations.

feat - incorporated mcp server request configs for cli mode

1aaf641

Kavyapriya-1804 mentioned this pull request May 3, 2025

Add support for Inspector configuration settings in CLI mode #330

Open

Merge branch 'main' into feat-cli-server-request-configs

b38b6fa

Merge branch 'main' into feat-cli-server-request-configs

b1110c4

Kavyapriya-1804 force-pushed the feat-cli-server-request-configs branch from ec4fb9d to b1110c4 Compare May 10, 2025 15:06

Merge branch 'main' into feat-cli-server-request-configs

90dc9d0

Merge branch 'main' into feat-cli-server-request-configs

a76776f

cliffhall added enhancement New feature or request waiting on submitter Waiting for the submitter to provide more info labels May 14, 2025

Merge branch 'main' into feat-cli-server-request-configs

a4d2ab2

Kavyapriya-1804 added 2 commits May 28, 2025 22:07

Merge branch 'main' into feat-cli-server-request-configs

003ea14

Merge remote-tracking branch 'upstream/main' into feat-cli-server-req…

bc62929

…uest-configs

Merge branch 'main' into feat-cli-server-request-configs

0ffcd81

Merge branch 'main' into feat-cli-server-request-configs

470db15

Merge remote-tracking branch 'upstream/main' into feat-cli-server-req…

79b1b10

…uest-configs

Merge branch 'main' into feat-cli-server-request-configs

bc470f6

Merge branch 'main' into feat-cli-server-request-configs

c97b5c9

Kavyapriya-1804 added 2 commits June 23, 2025 10:38

Merge branch 'main' into feat-cli-server-request-configs

02a0e3c

Merge branch 'main' into feat-cli-server-request-configs

5fb9790

Kavyapriya-1804 requested a review from cliffhall June 25, 2025 03:57

olaservo added 5 commits June 28, 2025 20:46

Merge from main

2ef0c0f

Merge branch 'main' into feat-cli-server-request-configs

e003134

Fix formatting

ff846b6

Update cli-tests.js

302257b

Merge branch 'feat-cli-server-request-configs' of https://github.com/…

d797947

…Kavyapriya-1804/inspector into feat-cli-server-request-configs

Merge branch 'main' into feat-cli-server-request-configs

f4f4779

olaservo added the cli label Jul 7, 2025

Merge branch 'main' into feat-cli-server-request-configs

72ac117

Merge branch 'main' into feat-cli-server-request-configs

94cd40a

cliffhall approved these changes Jul 7, 2025 •

edited

Loading

View reviewed changes

cliffhall previously requested changes Jul 7, 2025

View reviewed changes

Kavyapriya-1804 added 2 commits July 14, 2025 22:21

Merge branch 'main' into feat-cli-server-request-configs

ea6af4d

feat: updated documentation for CLI req timeout configs

29947ea

Merge branch 'main' into feat-cli-server-request-configs

470b19d

olaservo approved these changes Jul 15, 2025

View reviewed changes

olaservo requested a review from cliffhall July 15, 2025 03:21

cliffhall requested changes Jul 15, 2025

View reviewed changes

		# With request timeout configs (in milliseconds) - max total timeout
		npx modelcontextprotocol/inspector --cli node build/index.js --method tools/call --tool-name longRunningOperation --tool-arg duration=15 steps=5 --max-total-timeout 30000

feat - incorporated mcp server request configs for cli mode #373

Are you sure you want to change the base?

feat - incorporated mcp server request configs for cli mode #373

Conversation

Kavyapriya-1804 commented May 3, 2025

Motivation and Context

How Has This Been Tested?

Breaking Changes

Types of changes

Checklist

Additional context

Uh oh!

Kavyapriya-1804 commented May 4, 2025

Uh oh!

olaservo commented May 5, 2025

Uh oh!

Kavyapriya-1804 commented May 11, 2025

Uh oh!

cliffhall commented May 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Kavyapriya-1804 commented May 12, 2025

Uh oh!

cliffhall commented May 12, 2025

Uh oh!

Kavyapriya-1804 commented May 14, 2025

Uh oh!

olaservo commented May 28, 2025

Uh oh!

Kavyapriya-1804 commented May 28, 2025

Uh oh!

Kavyapriya-1804 commented May 30, 2025

Uh oh!

cliffhall commented May 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olaservo commented Jun 1, 2025

Uh oh!

cliffhall commented Jun 2, 2025

Uh oh!

Kavyapriya-1804 commented Jun 3, 2025

Uh oh!

Kavyapriya-1804 commented Jun 21, 2025

Uh oh!

olaservo commented Jun 28, 2025

Uh oh!

Kavyapriya-1804 commented Jun 28, 2025

Uh oh!

cliffhall commented Jul 1, 2025

Uh oh!

Kavyapriya-1804 commented Jul 5, 2025

Uh oh!

Kavyapriya-1804 commented Jul 7, 2025

Uh oh!

Uh oh!

cliffhall left a comment

Choose a reason for hiding this comment

Uh oh!

Kavyapriya-1804 commented Jul 14, 2025

Uh oh!

cliffhall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffhall Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Kavyapriya-1804 Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

cliffhall Jul 15, 2025

Choose a reason for hiding this comment

Uh oh!

Kavyapriya-1804 Jul 17, 2025

Choose a reason for hiding this comment

Uh oh!

cliffhall Jul 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cliffhall commented May 12, 2025 •

edited

Loading

cliffhall commented May 30, 2025 •

edited

Loading

cliffhall left a comment •

edited

Loading

cliffhall Jul 17, 2025 •

edited

Loading