Repository navigation
[MCP] Allow specifying cwd and additional env vars - #5246
Conversation
96d1d25 to
5175155
Compare
5175155 to
16c3867
Compare
| env, | ||
| env_vars, | ||
| cwd, | ||
| } => serde_json::json!({ |
There was a problem hiding this comment.
Why aren't we just using serde for this again? Because this is UI, I guess?
There was a problem hiding this comment.
I'm just adding a couple of fields and didn't want to expand the scope of this PR to refactor all of the code around it since this PR is big enough already.
| fn set_env_var(key: &str, value: &str) { | ||
| unsafe { | ||
| std::env::set_var(key, value); | ||
| } | ||
| } | ||
|
|
||
| fn remove_env_var(key: &str) { | ||
| unsafe { | ||
| std::env::remove_var(key); | ||
| } | ||
| } |
There was a problem hiding this comment.
Do we have confidence this use case is sufficiently covered through some other means?
I do not want this in the codebase: I can't personally police all the places this happens and the risk of copy/paste here is very high.
If you think it is important to have a unit test for create_env_for_mcp_server(), then please pass in an abstraction around std::env::var.
| } | ||
|
|
||
| #[test] | ||
| #[serial(extra_rmcp_env)] |
There was a problem hiding this comment.
If you insist on continuing to use this pattern, please add detailed information to AGENTS.md so that we have some chance of catching when it is being misused/abused.
| } | ||
|
|
||
| if let Some(cwd) = cwd { | ||
| entry["cwd"] = toml_edit::value(cwd.to_string_lossy().to_string()); |
There was a problem hiding this comment.
Is cwd guaranteed to be an absolute, normalized path?
There was a problem hiding this comment.
We write the same PathBuf that we read without modifying it.
| .assert() | ||
| .success(); | ||
|
|
||
| let runtime = tokio::runtime::Runtime::new()?; |
There was a problem hiding this comment.
This seems unusual: is there a reason why you need to introduce a Runtime instead of making this an async test? Also, I see the runtime is explicitly dropped. Can you change to async or add a comment why this is being done?
2 participants
This makes stdio mcp servers more flexible by allowing users to specify the cwd to run the server command from and adding additional environment variables to be passed through to the server.
Example config using the test server in this repo:
@bolinfest I know you hate these env var tests but let's roll with this for now. I may take a stab at the env guard + serial macro at some point.