fix(mcp): make keyring a true optional dependency in _resolve_keyring_refs#1028
Open
kovtcharov wants to merge 1 commit intomainfrom
Open
fix(mcp): make keyring a true optional dependency in _resolve_keyring_refs#1028kovtcharov wants to merge 1 commit intomainfrom
kovtcharov wants to merge 1 commit intomainfrom
Conversation
_resolve_keyring_refs imported keyring (an optional dependency) at the top of the function regardless of whether the env actually contained any $keyring references. On dev environments without keyring installed this caused tests/unit/mcp/client/test_mcp_client.py::TestMCPClient:: test_from_config_accepts_env to fail at collection with ModuleNotFoundError, even though the test only passes plain string env vars and never triggers the keyring path. Move the keyring + connectors imports inside the loop branch that actually needs them, so plain-env callers stay zero-dep on keyring while $keyring callers get the same behaviour as before.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why this matters
Before: `tests/unit/mcp/client/test_mcp_client.py::test_from_config_accepts_env` fails at import on any dev environment without the optional `keyring` package, even though that test only passes plain string env vars and never exercises the `$keyring` path. The unconditional `import keyring` at the top of `_resolve_keyring_refs` made keyring a hard dependency in practice for every config-driven MCP server load.
After: imports for `keyring`, `ConnectorsError`, and `SERVICE_NAME` are deferred to the branch that actually needs them. Plain-env callers stay zero-dep on keyring; the security invariants and error handling for real `$keyring` references are unchanged.
Caught while reviewing #1026 — pre-existing issue, not introduced by that PR.
Test plan