Repository navigation
Conversation
…me error honest
Both SDKs hardcoded the API's on_timeout default ('kill') in two places — doc
comments and client-side auto_resume validation — coupling them to an
infra-owned default with no single source. An unset on_timeout resolved locally
to 'kill', so auto_resume=true with no on_timeout was rejected with a message
blaming on_timeout the caller never set.
- Add DEFAULT_ON_TIMEOUT ('kill') in both SDKs: one documented constant that
mirrors the API's AutoPauseDefault (e2b-dev/infra). It is never sent — an
omitted on_timeout still leaves auto_pause unset so the server applies its own
default — it only resolves local validation, now in one place.
- Make the auto_resume rejection honest: when on_timeout is unset, the error
says it defaulted to 'kill' and to set on_timeout to 'pause', instead of
blaming an action the caller did not choose.
- Tests assert the clarified message in both SDKs.
Keeps the client-side rejection (not deferring to the server): the API does not
reject auto_resume paired with a no-pause timeout, so dropping the local check
would silently accept a no-op (auto-resume on a sandbox that gets killed).
Refs e2b-dev#2057
🦋 Changeset detectedLatest commit: 2d930b0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 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 |
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: 1 flag
Not posted on this PR by your GitHub settings — view it in Devin Review. (Configure)
There was a problem hiding this comment.
Reviewed against TASTE.md (e2b-dev/sdk-harness). I checked parity (T-1/T-2), constants and defaults (T-47), entry-point exports (T-54), validation scope (T-52), error class and message shape (T-59, T-62, T-63), and docs (T-69). JS and Python change in step, and both errors stay InvalidArgumentError/InvalidArgumentException.
4 violations, all inline:
- T-69: the new JS constant sits between the
SandboxOnTimeoutJSDoc and the type, so the public type has lost its docs. - T-54:
DEFAULT_ON_TIMEOUTis exported fromsandboxApi.tsbut not fromindex.ts, so only a deep import reaches it. It should be module-private. - T-52 / T-62 (JS and Python, same issue): the new "not set" message states the server's current default, so it goes stale as soon as infra changes
AutoPauseDefault. Say "not set, set it to 'pause'" without naming the default.
Not tied to a changed line:
- T-52, overall: a constant keeps the assumption in fewer places in each SDK, but the SDK still copies a backend default: there is one copy per SDK, and the
onTimeout/on_timeoutdocs still say "currentlykill" separately (sandboxApi.tsSandboxLifecycle.onTimeout;sandbox_api.pySandboxLifecycleandon_timeoutdocstrings). So the PR's "single source of truth" claim only holds within the validation code. If the default can change, the stronger fix is the API-side spec default mentioned in the PR body. - Not a TASTE rule: the JS test went from
toThrowError(InvalidArgumentError)to a message regex only, so it no longer checks the T-59 error class. Python still checks it withpytest.raises(InvalidArgumentException). Worth asserting both in JS.
![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: cb1b3986a4
ℹ️ 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".
…ror, python changeset
Review (Devin TASTE.md + Codex):
- T-69: move DEFAULT_ON_TIMEOUT out from between the SandboxOnTimeout JSDoc and
its type (the public type had lost its docs); place it after the type.
- T-54: make DEFAULT_ON_TIMEOUT module-private (drop export) — it is not
re-exported from index.ts and the Python twin isn't in __all__, so this keeps
parity and avoids publishing an infra-owned default as SDK contract.
- T-52/T-62: drop the server default ('kill') from the auto_resume error. The
actionable part — set on_timeout to 'pause' — holds whatever the server
default is; naming 'kill' would go stale if the API default changes (e2b-dev#2056).
Also fixes mixed quoting in the Python message.
- Test: JS now asserts both the InvalidArgumentError class (T-59) and the
clarified message, not a message regex alone.
- Codex P1: add '@e2b/python-sdk' to the changeset — the Python exception text
changed too and releases as a separate target.
Refs e2b-dev#2057
|
Thanks for the thorough review — addressed all of it in 2d930b0. Devin (TASTE.md):
Codex P1 — added On the broader T-52 note (the SDK still copies a backend default, and the Verified: JS |
|
Companion PR on the infra side: e2b-dev/runtime#3767 adds a contract test asserting the OpenAPI spec's |
Problem
Fixes #2057.
Both SDKs hardcode the API's
on_timeout/onTimeoutdefault (kill) in two places — doc comments and client-sideauto_resumevalidation — coupling them to an infra-owned default (AutoPauseDefaultin e2b-dev/infra) with no single source of truth. Because an unseton_timeoutresolves locally tokill,auto_resume: truewith noon_timeoutis rejected with a message that blames anon_timeoutthe caller never set.What this does
DEFAULT_ON_TIMEOUT('kill') in both SDKs (sandboxApi.ts,sandbox_api.py), documented as mirroring the API'sAutoPauseDefault. The scattered?? 'kill'/else "kill"literals now reference it, so the assumption lives in one place with a pointer to the owning repo. It is never sent to the API — an omittedon_timeoutstill leavesauto_pauseunset so the server applies its own default — it only resolves local validation.on_timeoutis unset, theauto_resumerejection now says it defaulted tokilland that you should seton_timeouttopause, instead of blaming an action the caller didn't choose. An explicit non-pause action keeps a distinct message naming the value.lifecycleRequest.test.ts— 24/24 pass; Python:test_create_rejects_auto_resume_without_a_timeout_actionasserts the message names the default).On the options in the issue
auto_resumepaired with a no-pause timeout, so dropping the client-side check would silently accept a no-op config (auto-resume on a sandbox that gets killed on timeout). Kept the client-side rejection.autoPause(onlyautoPauseMemorydefaultstrue). The constant + documented pointer is the closest single-source we can do without an API-side spec change — if the spec later exposes anautoPausedefault, the constant is the one spot to wire it to.The
SandboxInfo.on_timeoutround-trip asymmetry noted in the issue (can't represent left to API default) is left as a separate follow-up — it needs an API/response-shape change, out of scope here.Not included
Issue #2056 (whether the default itself should be
pauserather thankill) is a product/API default decision for maintainers, not a code change this PR makes.Test environment note
The JS lifecycle request suite (the one I touched) passes 24/24. Three tests in the separate
lifecyclePayload.test.tsfail only withAuthenticationError: API key is required(integration-style, need a realE2B_API_KEY) and are unrelated to this change. Python unit tests couldn't run in my sandbox due to a pre-existing protobuf-codegen gap in the env; the changed module and test compile clean andruffpasses.