Repository navigation
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @Kylinny on file. You can sign our CLA at https://e2b.dev/docs/cla . Once you've signed, post a comment here that says '@cla-bot check' |
🦋 Changeset detectedLatest commit: c7313bb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review of #1901 (checked T-1/T-1a parity & naming, T-40 wire-format casing, T-43 shell escaping, T-52 client-side validation, T-54 public surface, T-71 docstrings).
The fix itself follows T-40 as intended: Python keeps the documented snake_case run_cmd/install_cmd in GitHubMcpServerConfig and converts to the camelCase wire form on the way in, and the gateway command still goes through shlex.quote (T-43). Applying the mapping once in create() so both the API payload and the mcp-gateway --config JSON agree is the right place.
2 violations, both inline in packages/python-sdk/e2b/sandbox/sandbox_api.py:
- The "explicit wire key wins over its snake_case alias" branch turns
runCmd/installCmdinto an accepted (undocumented) Python spelling — T-1a / T-40. build_mcp_wire_configis an internal helper exposed under a public name without being in__all__— T-54.
Not tied to a line: since the tests import the helper, they'd import the underscored name after the rename.
| for key, value in config.items(): | ||
| wire_key = _github_mcp_server_wire_names.get(key, key) | ||
| # An explicitly spelled wire key wins over its snake_case alias, | ||
| # regardless of dict order. | ||
| if wire_key == key or wire_key not in config: | ||
| mapped[wire_key] = value | ||
| wire[server] = mapped | ||
| else: |
There was a problem hiding this comment.
T-1a / T-40 — the Python surface is snake_case only; camelCase is confined to the serialized shape and the SDK converts on the way in. Making an explicitly spelled runCmd/installCmd "win" over run_cmd/install_cmd quietly turns the wire spelling into a second accepted Python spelling (one that GitHubMcpServerConfig doesn't document), and adds a client-side precedence rule the gateway never defined (T-52). Just rename and let the gateway validate:
| for key, value in config.items(): | |
| wire_key = _github_mcp_server_wire_names.get(key, key) | |
| # An explicitly spelled wire key wins over its snake_case alias, | |
| # regardless of dict order. | |
| if wire_key == key or wire_key not in config: | |
| mapped[wire_key] = value | |
| wire[server] = mapped | |
| else: | |
| wire[server] = { | |
| _github_mcp_server_wire_names.get(key, key): value | |
| for key, value in config.items() | |
| } |
| } | ||
|
|
||
|
|
||
| def build_mcp_wire_config(mcp: McpServer) -> Dict[str, Any]: |
There was a problem hiding this comment.
T-54 — everything public is listed in __init__.py's __all__; this helper is internal wiring for create() and isn't (and shouldn't be) exported, so give it the module-private prefix like the _github_mcp_server_wire_names table above it. The tests can import the underscored name.
| def build_mcp_wire_config(mcp: McpServer) -> Dict[str, Any]: | |
| def _build_mcp_wire_config(mcp: McpServer) -> Dict[str, Any]: |
|
@cla-bot check |
|
The cla-bot has been summoned, and re-checked this pull request! |
|
Hi @mishushakov — a gentle follow-up on this fix: it maps the Python SDK's snake_case GitHub MCP server keys ( |
4f30399 to
cdcf6f5
Compare
The Python SDK documents snake_case keys (run_cmd/install_cmd) for GitHubMcpServerConfig, but the mcp-gateway only recognizes camelCase (runCmd/installCmd). The SDK passed the dict through verbatim, so the documented form was silently ignored and the server never started (e2b-dev#1899). Map the keys to the wire form in Sandbox.create/AsyncSandbox.create via build_mcp_wire_config() before the config reaches the API payload and the `mcp-gateway --config` JSON. Base server entries and unknown keys pass through unchanged; an explicitly spelled wire key wins over its snake_case alias; the caller's dict is never mutated.
cdcf6f5 to
c7313bb
Compare
|
This is not a priority for us atm. I will come back to it once I have more headroom |
This branch has not been deployed
2 participants
Thank you @aimable100 for the clear report and repro — the gateway log line made the root cause easy to confirm.
Root cause
GitHubMcpServerConfigin the Python SDK documents snake_case keys (run_cmd/install_cmd), but the sandbox'smcp-gatewayonly recognizes camelCase (runCmd/installCmd).Sandbox.create/AsyncSandbox.createserialized the dict verbatim intomcp-gateway --config <json>, so the documented Python form was silently ignored and the server never started (the gateway loggedGitHub server missing run command).The JS SDK was already consistent (its
GitHubMcpServertype usesrunCmd/installCmd), so this only affected the Python SDK.Fix (option 1 from the issue)
Map the keys to the wire form once at the top of
create()in both the sync and async implementations, via a newbuild_mcp_wire_config()helper ine2b/sandbox/sandbox_api.py— matching how other snake_case fields are mapped to wire names elsewhere in the SDK. The documented Python form keeps working; both the API payload and the gateway--configJSON now agree on the wire names.Invariants of the mapping:
"github/owner/repo"are touched; base MCP server entries (already camelCase) pass through unchanged.envs, future gateway keys) pass through verbatim.runCmd) always wins over its snake_case alias.Usage example
The docs' "Custom MCP servers" Python example (snake_case) is now accurate as written, so no docs change is needed.
Tests
packages/python-sdk/tests/shared/sandbox/test_mcp_wire_names.py(9 tests): key mapping, partial configs,envs/unknown keys pass-through, base-server entries untouched, explicit wire key precedence, no input mutation, plus end-to-end sync/asynccreate()tests asserting both the API request body and the actualmcp-gateway --configcommand carryrunCmd/installCmd.tests/shared/suite: 223 passed, 32 skipped (pre-existing skips), no regressions.ruff check,ruff format --check, andty checkall pass.@e2b/python-sdk.Fixes #1899.