fix: pass apiKey from connectionConfig to pause() method#1218
fix: pass apiKey from connectionConfig to pause() method#1218passionworkeer wants to merge 1 commit intoe2b-dev:mainfrom
Conversation
When using Sandbox.connect() with an apiKey, the pause() method was not passing the apiKey to the API call, causing auth failures. This fix ensures pause() (and deprecated betaPause()) use the same pattern as kill() - merging connectionConfig with provided opts. Fixes: e2b-dev#1215
passionworkeer
requested review from
ValentaTomas,
jakubno and
mishushakov
as code owners
|
|
@e2b-dev/copilot Ready for review! |
There was a problem hiding this comment.
Pull request overview
Fixes authentication failures when calling Sandbox.pause() (and deprecated betaPause()) on a sandbox created via Sandbox.connect() with an apiKey, by ensuring the instance’s connectionConfig is merged into the pause call options (matching the existing kill() pattern).
Changes:
- Update
Sandbox.pause()to pass{ ...this.connectionConfig, ...opts }intoSandboxApi.pause(). - Update
Sandbox.betaPause()to pass{ ...this.connectionConfig, ...opts }intoSandboxApi.betaPause().
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async pause(opts?: ConnectionOpts): Promise<boolean> { | ||
| return await SandboxApi.pause(this.sandboxId, opts) | ||
| return await SandboxApi.pause(this.sandboxId, { ...this.connectionConfig, ...opts }) |
There was a problem hiding this comment.
The JSDoc for pause() says it returns a sandbox ID that can be used to resume, but the method signature and implementation return Promise<boolean> (matching SandboxApi.pause). Please update the @returns description to reflect the boolean semantics (true=paused, false=already paused).
| */ | ||
| async pause(opts?: ConnectionOpts): Promise<boolean> { | ||
| return await SandboxApi.pause(this.sandboxId, opts) | ||
| return await SandboxApi.pause(this.sandboxId, { ...this.connectionConfig, ...opts }) |
There was a problem hiding this comment.
This change fixes a regression scenario where Sandbox.connect(id, { apiKey }) should allow calling sandbox.pause() without re-supplying the key (especially when E2B_API_KEY is not set). There are existing integration tests around pause/connect, but none cover this credential-propagation case; please add a regression test that temporarily unsets process.env.E2B_API_KEY, creates/connects using an explicit apiKey, and asserts pause() succeeds.
…tion - Update @returns JSDoc: pause() returns boolean (true=paused, false=already paused) matching SandboxApi.pause semantics, not a sandbox ID - Add regression tests for apiKey propagation in Sandbox.connect(): - pause() succeeds when E2B_API_KEY is unset but apiKey is passed to connect() - pause() returns false when sandbox is already paused - pause() works on connected sandbox with apiKey in connectionConfig Refs: Copilot PR e2b-dev#1218 review comments
|
Hi, checking in - this PR adds apiKey propagation to the pause() method. Copilot reviewed with no issues. Is there anything needed to move forward? |
When using Sandbox.connect() with an apiKey, the pause() method was not passing the apiKey to the API call, causing auth failures. This fix ensures pause() (and deprecated betaPause()) use the same pattern as kill() - merging connectionConfig with provided opts.
Fixes: #1215