Fix: Missing default connection config propagation#1179
Conversation
🦋 Changeset detectedLatest commit: 60aa6ca The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
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 |
Package ArtifactsBuilt from 5b53c24. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.15.1-fix-default-config-propagation.0.tgzCLI ( npm install ./e2b-cli-2.8.3-fix-default-config-propagation.0.tgzPython SDK ( pip install ./e2b-2.16.0+fix.default.config.propagation-py3-none-any.whl |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90de8fc509
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| return { | ||
| ...this.connectionConfig, | ||
| ...opts, | ||
| }) | ||
| ...(opts ?? {}), |
There was a problem hiding this comment.
resolveApiOpts now forwards the full stored connectionConfig, which always includes a concrete apiUrl; if a caller overrides only domain on instance calls like connect()/pause(), the inherited apiUrl still takes precedence when ConnectionConfig is rebuilt, so the request is sent to the original API host instead of the override domain. This regression is specific to the newly migrated instance methods and makes per-call domain overrides ineffective unless callers also pass apiUrl explicitly.
Useful? React with 👍 / 👎.
| sandbox_id=self.sandbox_id, | ||
| timeout=timeout, | ||
| **opts, | ||
| **self.connection_config.get_api_params(**opts), |
There was a problem hiding this comment.
Using self.connection_config.get_api_params(**opts) here injects the stored api_url into every call; when users pass a per-call domain override without api_url, _cls_connect/_cls_pause rebuild ConnectionConfig from kwargs and keep the old api_url, so the override domain is ignored. This is a behavior change from the previous **opts path for these instance methods (and the same pattern is now present in both sync and async implementations).
Useful? React with 👍 / 👎.
| ) | ||
| } | ||
|
|
||
| private resolveApiOpts<T extends object>(opts?: T): ConnectionOpts & T { |
There was a problem hiding this comment.
you can just narrow it down to just extends ConnectionOpts
|
I am not sure this is a safe change, because what if I want to set things like requestTimeout on Sandbox.create but don't want it to be propagated to other class methods? Imo we should only inherit properties like apiUrl/domain, the rest looks fine |
17f0fbd to
b80f0e5
Compare
|
also please fix Sandbox as any, if it's not necessary |
Note
Medium Risk
Changes how instance methods merge and forward connection options (api key/domain/headers/timeouts) to API calls in both JS and Python SDKs, which can affect request routing and auth headers. Regression tests reduce risk but behavior changes could impact callers relying on previous (incorrect) defaults.
Overview
Fixes missing propagation of instance
connectionConfigwhen calling sandbox instance methods (notablypause/betaPause/connect, plus related methods) so default config is always forwarded and per-call overrides still win.In the JS SDK this centralizes option merging via a new
resolveApiOpts()helper and updates multipleSandboxApi.*calls to use it; in the Python SDK it updatesSandbox.connect()andSandbox.pause()(sync + async) to passself.connection_config.get_api_params(**opts).Adds regression tests in both SDKs to assert defaults are forwarded and overrides are applied correctly.
Written by Cursor Bugbot for commit 60aa6ca. This will update automatically on new commits. Configure here.