| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
There was a problem hiding this comment.
I really appreciate the work to make windows work. I have too little understanding of windows to have a strong opinion. Some minor comments. Throwing it back at you, but happy to merge with the minor changes that I requested.
In general it might be worthwhile to start a src/mcp/client/stdio/ folder and move stdio.py to src/mcp/client/stdio/__init__.py or whatever, so that we can have a src/mcp/client/stdio/win32.py, or even just have a helper src/mcp/platform/win32/.
Sorry, something went wrong.
| except Exception: | ||
| return command |
There was a problem hiding this comment.
What exception could happen here? Can we add at least a comment or be more specific about the exception?
Sorry, something went wrong.
There was a problem hiding this comment.
The main one I was worried about is any exceptions thrown by shutil.which(). I never hit this clause, so I can also remove it if you prefer
Sorry, something went wrong.
There was a problem hiding this comment.
sorry meant to request changes to get it back to your queue.
Sorry, something went wrong.
|
@dsp-ant thanks for the prompt review! I believe I've addressed everything |
Sorry, something went wrong.
| if sys.platform == "win32": | ||
| await terminate_windows_process(process) | ||
| else: | ||
| process.terminate() |
There was a problem hiding this comment.
This still hangs on linux if the invoked process expects SIGINT, eg: https://github.com/modelcontextprotocol/servers/blob/main/src/everything/index.ts#L13
Shouldn't we try to force kill, same as we do on Windows?
Sorry, something went wrong.
Sorry, something went wrong.
There was a problem hiding this comment.
just as a note the fix in cristipufu's pr works on windows 11, the merged fix still hangs.
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
This change should fix most cases of #359, with issues spawning stdio MCP servers on Windows.
Motivation and Context
There is a somewhat longstanding issue with stdio_cleint that it doesn't work on Windows.
Example:
Servers running with "npx" have problems because of the way "npx" command is handled in Windows (it is typically stored as a ps1 script):
mcp: servers: todoist: command: "npx" args: [ "@abhiz123/todoist-mcp-server", "-y"] brave-search: command: "npx" args: ["@modelcontextprotocol/server-brave-search", "-y"] fetch: command: "uvx" args: ["mcp-server-fetch"]Replacing "npx" with "npx.cmd" usually mitigates this, but that makes the command platform-specific.
This change adds some of that handling in the stdio_client itself, and adds helpers for processing the command and spawning a process in platform-specific ways.
How Has This Been Tested?
Tested on Windows VM in Azure using all mcp-agent examples. All of them use stdio_client, and all of them now work.
Note: for using with streamlit, I had to add an additional change which I didn't add to this PR, since it may not belong to the SDK:
In addition:
Breaking Changes
None
Types of changes
Checklist
Additional context
Open to feedback on how to make this more elegant.