Support mcp.json environment variable expansion in load_mcp_servers()
#3380
Conversation
wgillett
force-pushed
the
mcp-json-env-vars
branch
from
4c87ce3 to
d96f0f0
Compare
…de Code; compile a regex only once; use re.sub.
|
@DouweM Thanks for the feedback. I have addressed all of it. Please let me know if there is more to do here, or if you are ready to remove the "awaiting author revision" label. |
tests/test_mcp.py
Outdated
| async def test_is_running(): | ||
| """Test the is_running property.""" | ||
| server = MCPServerStdio('python', ['-m', 'tests.mcp_server']) | ||
|
|
||
| # Server should not be running initially | ||
| assert not server.is_running | ||
|
|
||
| # Server should be running inside the context manager | ||
| async with server: | ||
| assert server.is_running | ||
|
|
||
| # Server should not be running after exiting the context manager | ||
| assert not server.is_running |
There was a problem hiding this comment.
@Kludex I added this for test coverage. When I ran MCP tests, that showed is_running as not covered. Looking at this again, I see that is_running is actually exercised by test_agent.py, but it's all indirect. Having a test specifically for this function seems like a good idea to me.
There was a problem hiding this comment.
Please drop this test.
I don't agree that we should be testing private attributes/behavior.
There was a problem hiding this comment.
@Kludex I have removed the test as requested. Agree with not testing private attributes/behavior. For future reference, how would I tell that is_running is private? I expect private members to be named with a leading underscore.
|
@Kludex Thanks for the feedback. I have responded to all of it. Please let me know if more changes are needed. |
|
After #3380 (comment), we can merge it. Thanks! 🙏 |
|
@Kludex wrote "After #3380 (comment), we can merge it." |
DouweM
changed the title
mcp.json environment variable expansion in load_mcp_servers()
3 participants
Using environment variables is helpful to be able to check mcp.json into source control without embedding secrets, and to be able to vary configuration values dynamically.
In load_mcp_servers, expand any environment variables. Throw an exception if any variables don't have values.
Add tests and document the use of environment variables.