Fix: Missing default connection config propagation#1179
Fix: Missing default connection config propagation#1179ben-fornefeld wants to merge 6 commits intomainfrom
Conversation
ben-fornefeld
requested review from
ValentaTomas,
jakubno and
mishushakov
as code owners
🦋 Changeset detectedLatest commit: 3284d34 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 be2c748. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.14.2-fix-default-config-propagation.0.tgzCLI ( npm install ./e2b-cli-2.8.2-fix-default-config-propagation.0.tgzPython SDK ( pip install ./e2b-2.15.2+fix.default.config.propagation-py3-none-any.whl |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
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 |
ben-fornefeld
force-pushed
the
fix/default-config-propagation
branch
from
17f0fbd to
b80f0e5
Compare
|
also please fix Sandbox as any, if it's not necessary |
3 participants
Note
Low Risk
Small, targeted change to request option wiring; behavior is now more consistent and covered by new regression tests, with minimal risk beyond potential differences for callers relying on missing defaults.
Overview
Fixes missing propagation of a sandbox instance’s default connection config (API key/domain/timeout/headers) when calling instance methods like
connect()andpause().In the JS SDK, multiple sandbox instance methods now consistently merge
connectionConfigwith per-call overrides via a newresolveApiOptshelper, and new vitest coverage asserts correct forwarding and override behavior. In the Python SDK (sync + async),connect()andpause()now forward params throughconnection_config.get_api_params(**opts), with new tests validating base params and override/header merging.Written by Cursor Bugbot for commit 3284d34. This will update automatically on new commits. Configure here.