Repository navigation
fix(sdk): use camelCase keys installCmd/runCmd for GitHub MCP server config (#1899) - #2031
TINGyu123644 wants to merge 1 commit into
Conversation
|
We require contributors to sign our Contributor License Agreement, and we don't have @TINGyu123644 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' |
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Devin Review found 2 potential issues.
1 flag not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
| Configuration for a GitHub-based MCP server. | ||
| """ | ||
|
|
||
| runCmd: str |
There was a problem hiding this comment.
🟡 Public MCP contract lacks changeset
Changing GitHubMcpServerConfig without a changeset leaves this public SDK contract change out of the release record.
Learn more
The SDK exports this type through the package exports. The repository requires a changeset whenever the public surface of the Python SDK changes. Renaming the required command key changes that surface, but this PR contains no changeset.
Example: A release containing this change has no changeset entry describing why callers must now supply runCmd instead of run_cmd.
Recommended fix: Add a Python SDK changeset with pnpm changeset, describing the updated GitHub MCP command keys.
Was this helpful? React with 👍 or 👎 to provide feedback.
| """ | ||
| Command to run the MCP server. Must start a stdio-compatible server. | ||
| """ | ||
| installCmd: NotRequired[str] |
There was a problem hiding this comment.
🟡 MCP key rename lacks required tests
Changing GitHubMcpServerConfig without updating tests leaves the affected configuration contract uncovered, contrary to the repository's test requirement.
Learn more
The new typed keys are sent as-is through the sync request builder and async request builder. The repository requires new or updated tests covering affected codepaths, and the PR changes no tests.
Example: A typed config with runCmd and installCmd can reach both sandbox creation paths, but neither path has a new regression test for this contract.
Recommended fix: Add coverage for the camelCase GitHub MCP configuration in sync and async Python sandbox creation, including serialized keys; run pnpm run test.
Was this helpful? React with 👍 or 👎 to provide feedback.
There was a problem hiding this comment.
TASTE.md review — checked parity (T-1/T-1a/T-2), wire-format casing (T-40), client-side validation and actionable errors (T-52, T-62), deprecation/compatibility (T-65/T-66), and docstrings (T-69/T-71).
1 violation found (inline). It's a gap the change leaves open, not a problem with the rename itself.
The rename to runCmd / installCmd complies with TASTE.md. The McpServer dict goes to mcp-gateway unchanged through json.dumps(mcp), so it is the serialized shape, and T-40 says that shape uses camelCase in both SDKs. It also brings Python in line with JS GitHubMcpServer (runCmd / installCmd? in packages/js-sdk/src/sandbox/sandboxApi.ts), which T-1 requires, and with the generated Python MCP configs in e2b/sandbox/mcp.py (airtableApiKey, awsRegion, …). T-65 does not apply: the snake_case keys never worked at runtime, so there is no working identifier to keep as a deprecated alias.
Not tied to a specific line:
- Line endings: the PR rewrites all of
sandbox_api.pyfrom LF to CRLF. That turns the 2-line change into a 1396/1396-line diff and buries the real change. Please convert the file back to LF so only the two key lines differ. - Missing changeset: this changes the public typing of
GitHubMcpServerConfig(anyone who type-checks code written againstrun_cmdwill now get errors), so the repo'sAGENTS.mdasks for apnpm changesetentry forpackages/python-sdk.
| Configuration for a GitHub-based MCP server. | ||
| """ | ||
|
|
||
| runCmd: str |
There was a problem hiding this comment.
T-52 / T-62 — client-side validation exists to turn opaque failures into actionable errors, and error messages should say what to do.
Renaming the type hint fixes type-checked callers. But anyone who already followed the old docs and passes {"github/owner/repo": {"run_cmd": ...}} will still have the key silently ignored, and the server still won't start. That silent failure is the bug in #1899. This is exactly the opaque-failure case T-52(b) covers.
Consider a small check where the config is serialized (Sandbox.create / AsyncSandbox.create, before json.dumps(mcp)). It should raise InvalidArgumentException and name the correct key, for example:
for name, cfg in (mcp or {}).items():
if name.startswith("github/") and isinstance(cfg, dict):
for old, new in (("run_cmd", "runCmd"), ("install_cmd", "installCmd")):
if old in cfg:
raise InvalidArgumentException(
f"MCP server '{name}': use '{new}' instead of '{old}'."
)Do this in both the sync and async paths (T-2). It is optional: the rename on its own already complies.
5131586 to
3efc9bd
Compare
|
We require contributors to sign our Contributor License Agreement, and we don't have @TINGyu123644 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' |
Summary
Fixes #1899.
The Python SDK's
GitHubMcpServerConfigtype hints (and the documented Pythonform) spell the custom MCP server config keys
install_cmd/run_cmd, but thesandbox
mcp-gatewayonly recognizes the camelCase keysinstallCmd/runCmd. Because the SDK passes the dict through verbatim, the documentedsnake_case form is silently ignored and the server never starts.
This updates the type hints to the camelCase keys the gateway actually accepts.
Changes
packages/python-sdk/e2b/sandbox/sandbox_api.py: renamerun_cmd->runCmdandinstall_cmd->installCmdinGitHubMcpServerConfig.Notes
hints / docs to the wire names the gateway expects). The external
"Custom MCP servers" documentation page should be updated to match.
dict verbatim, so runtime behaviour is unchanged and no internal code reads
these keys by the old names.
Testing
grepand aPython
astsyntax check on the modified file.dependency install; the change does not alter control flow.
Human Involvement
Generated entirely by AI agents without human participation in the loop.