fix(sdk): let callers omit lifecycle.onTimeout - #1711
Conversation
#1693 stopped sending `autoPause` when no timeout action was configured and #1694 did the same for `autoResume`, but `onTimeout` stayed a required member of `SandboxLifecycle`. The "not configured" state the SDKs now put on the wire was therefore unreachable for a typed caller except by omitting `lifecycle` entirely, and every test for it had to cast through the type. Make `onTimeout` optional in both SDKs, apply the same nullish-is-unconfigured rule to `keepMemory` / `keep_memory`, and stop phrasing the auto-resume guard in terms of a default the SDK no longer decides. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: b46326a 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 5dbcc55. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.41.1-cursor-author-pr-claiming-fdd6.0.tgzCLI ( npm install ./e2b-cli-2.16.3-cursor-author-pr-claiming-fdd6.0.tgzPython SDK ( pip install ./e2b-2.41.0+cursor.author.pr.claiming.fdd6-py3-none-any.whl |
![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: b46326afe5
ℹ️ 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".
| # None on_timeout is not a choice of action: the field is left out of | ||
| # the request and the API's default applies. | ||
| keep_memory = on_timeout_raw.get("keep_memory") | ||
| keep_memory_provided = keep_memory is not None |
There was a problem hiding this comment.
When keep_memory comes from an Optional[bool] configuration value and is None, this normalization now supports it at runtime, but the public SandboxOnTimeoutPause.keep_memory field remains NotRequired[bool]. Python type checkers therefore reject the newly documented call shape unless users cast to Any, unlike the equivalent JS field, which accepts an optional value. Widen the Python field to accept None so typed callers can use this behavior.
AGENTS.md reference: AGENTS.md:L3-L3
Useful? React with 👍 / 👎.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — approving despite the inline nit, which is a static-typing-only issue with no runtime impact.
The nit flags that SandboxOnTimeoutPause.keep_memory is still typed NotRequired[bool] instead of NotRequired[Optional[bool]], so a typed Optional[bool] spread fails pyright even though the runtime correctly treats None as unconfigured. This is a real inconsistency with the PR's stated goal and with the JS side, but it's cosmetic — it doesn't cause incorrect behavior at runtime, and fixing it is a one-line, low-risk change.
Extended reasoning...
Overview
This PR makes lifecycle.onTimeout/lifecycle["on_timeout"] optional in both the JS and Python SDKs, and treats a nullish keepMemory/keep_memory as "unconfigured" rather than an explicit choice. It touches packages/js-sdk/src/sandbox/sandboxApi.ts, packages/python-sdk/e2b/sandbox/sandbox_api.py, and the corresponding sync/async main.py docstrings, plus two changesets and their test suites. It's a direct, small follow-up to already-merged PRs #1693/#1694, narrowing an existing type/runtime mismatch rather than introducing new capability.
Security risks
None. This is client-side SDK request-building logic controlling which optional fields are included in the sandbox-create payload (autoPause/autoPauseMemory/autoResume). No auth, crypto, or permission logic is touched, and no user-controlled data flows into anything sensitive.
Level of scrutiny
Low-to-moderate is appropriate: this is SDK typing/validation logic, not a production-critical runtime path, and the change is a faithful mirror of the JS and Python logic with parallel guard clauses in both. I read through both sandboxApi.ts's createSandbox and Python's build_lifecycle_config to confirm the JS/Python behavior stays symmetric (per this repo's CLAUDE.md requirement that SDK changes land on both), and the guard/error-message logic (keepMemory-only-on-pause, autoResume-only-on-pause) is unchanged in substance, only the messaging and optionality shifted.
Other factors
Test coverage is thorough and directly exercises the new behavior (empty lifecycle, nullish keepMemory on both pause and kill, autoResume with unconfigured keepMemory) on both JS (12 tests) and Python (31 tests) sides, matching what the PR description claims. The one inline finding is a nit about a stale type annotation that doesn't match the new runtime semantics — worth fixing, but it doesn't block approval since it's a pure typing gap with no behavioral consequence.
| # string, or an unexpected value from an untyped caller) passes through as | ||
| # the action, so a non-"pause" value resolves to kill instead of crashing. | ||
| on_timeout_raw = lifecycle.get("on_timeout") if lifecycle else None | ||
| # A missing on_timeout — or an explicit None from an untyped caller — is not | ||
| # a choice of kill. It only resolves to kill semantics locally, for the | ||
| # validation below and for keep_memory. | ||
| # A missing on_timeout — or an explicit None — is not a choice of kill. The | ||
| # guards below therefore never assume what the API would pick; they only | ||
| # constrain what the caller said. | ||
| on_timeout_configured = on_timeout_raw is not None | ||
| if isinstance(on_timeout_raw, dict): | ||
| on_timeout = on_timeout_raw.get("action", "kill") | ||
| keep_memory_provided = "keep_memory" in on_timeout_raw | ||
| # A None keep_memory is not a choice of snapshot kind, the same way a | ||
| # None on_timeout is not a choice of action: the field is left out of | ||
| # the request and the API's default applies. | ||
| keep_memory = on_timeout_raw.get("keep_memory") | ||
| keep_memory_provided = keep_memory is not None | ||
| else: | ||
| # Only fall back when unconfigured, not on other falsy-but-present | ||
| # values an untyped caller might pass. |
There was a problem hiding this comment.
🟡 SandboxOnTimeoutPause.keep_memory (line 492) is still typed NotRequired[bool], but build_lifecycle_config now treats keep_memory: None as "unconfigured" and both the PR description and .changeset/lifecycle-optional-on-timeout.md advertise passing an Optional[bool] (e.g. keep_memory: read_from_config()) straight into Sandbox.create(lifecycle=...). Under pyright that call fails type checking (None is not assignable to bool), forcing every None-keep_memory case — even for action: "pause" — into cast(Any, ...) in the PR's own tests, unlike the JS side where keepMemory?: boolean widens to boolean | undefined with no cast. Fix: widen to NotRequired[Optional[bool]].
Extended reasoning...
This PR's runtime code in build_lifecycle_config (packages/python-sdk/e2b/sandbox/sandbox_api.py:815-831) explicitly treats a None keep_memory as "not configured" — it reads on_timeout_raw.get("keep_memory") and sets keep_memory_provided = keep_memory is not None, so the field is simply omitted from the wire request. That's the whole point of this PR: let a typed caller spread an Optional[bool] in without tripping validation. But the static type this flows through, SandboxOnTimeoutPause.keep_memory at line 492, is unchanged and still declared NotRequired[bool]. NotRequired only controls whether the key may be absent — it says nothing about the value being nullable. So a dict literal or variable typed Optional[bool] assigned to keep_memory is rejected by a static type checker even though the runtime happily accepts it.
I verified this empirically with pyright against the real package. Both of the exact patterns this PR's own description and changeset advertise fail to typecheck:
Sandbox.create(lifecycle={"on_timeout": {"action": "pause", "keep_memory": read_from_config()}})whereread_from_config() -> Optional[bool]Sandbox.create(lifecycle={"on_timeout": {"action": "pause", "keep_memory": None}})— the literal example from.changeset/lifecycle-optional-on-timeout.md
Both produce reportArgumentType/reportAssignmentType: "None is not assignable to bool". Meanwhile {"action": "pause", "keep_memory": True} and {"action": "pause"} (key omitted) both typecheck cleanly, confirming the problem is specifically the nullability of the value, not the presence of the key.
This is why the PR's own new Python tests (test_create_rejects_only_a_real_keep_memory_on_a_kill_action, test_create_treats_a_none_keep_memory_as_unconfigured, test_create_allows_auto_resume_with_an_unconfigured_keep_memory) all wrap their keep_memory: None dicts in cast(Any, ...), even for the action: "pause" case where the discriminated union should otherwise accept keep_memory directly without a cast. Compare this to the JS side: keepMemory?: boolean widens to boolean | undefined, so lifecycleRequest.test.ts's 'a nullish keepMemory is not a choice of snapshot kind' test passes an unset: boolean | undefined value with no cast at all for the pause case. That's a real JS/Python asymmetry in a PR whose explicit premise is parity between the SDKs (CLAUDE.md requires equivalent JS/Python SDK changes), and the PR description itself claims "the cases that used to need a cast are now written as typed calls" — which isn't true for this one.
Impact is limited to static type checking, not runtime: dicts still pass through and behave correctly regardless of the annotation, so nothing crashes and no data is lost. That's why this is a nit rather than a normal-severity finding — but it does directly undercut the stated purpose of the PR, which is to make these states expressible to typed callers without a cast.
Step-by-step proof:
- Open
packages/python-sdk/e2b/sandbox/sandbox_api.py:488-503—SandboxOnTimeoutPausedeclareskeep_memory: NotRequired[bool]. - Write
km: Optional[bool] = None; Sandbox.create(lifecycle={"on_timeout": {"action": "pause", "keep_memory": km}})in a.pyfile and runpyrightagainst the installed package. - Pyright reports
reportArgumentType: Argument of type "dict[str, dict[str, str | bool | None]]" cannot be assigned to parameter "lifecycle" ... "None" is not assignable to "bool". - Change the annotation to
keep_memory: NotRequired[Optional[bool]]and rerun — the same call now typechecks with zero errors, and no other type in the file needs to change (the discriminated union,build_lifecycle_config, andSandboxOnTimeoutKillare all unaffected).
Fix: change line 492 to keep_memory: NotRequired[Optional[bool]], matching the runtime "None means unconfigured" semantics this PR implements and the JS SDK's boolean | undefined widening.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
TASTE.md review — the direction is right; one carried-over defect and one verification gap
Making onTimeout / on_timeout optional is what TASTE asks for. The "no timeout action configured" state that #1693 and #1694 taught the wire to express is now expressible by a typed caller; absence stays absence instead of becoming a null sentinel; the relaxation is mirrored 1:1 across JS, sync Python and async Python; and the new guard message states the rule in the option names the user typed (lifecycle.onTimeout / lifecycle['on_timeout']) rather than in internal vocabulary. Pre-operation argument validation correctly raises InvalidArgumentError / InvalidArgumentException rather than a domain error. Everything below is non-blocking, but I would fix the changeset's Python sample before merge, because changeset prose is copied verbatim into the published CHANGELOG.
Provenance
#1711's head is literally the same commit as #1703's (b46326a), so "the tree is byte-identical" is exact rather than approximate. Two consequences: #1703 was closed by @mishushakov at 18:04 UTC, so the body's "please close #1703 in favour of this PR" is already stale; and the review history #1703 accumulated stayed behind on the closed PR — claude[bot]'s LGTM and the original thread are only visible there. Codex has since re-filed its P2 here, and I disagree with its remedy for reasons in the inline comment on the changeset.
Every number in the description reproduces
| check | result |
|---|---|
tests/sandbox/lifecycleRequest.test.ts |
12 passed, 0.6s |
tests/shared/sandbox/test_lifecycle_request.py |
31 passed, 0.11s |
python-sdk make lint / format / typecheck |
clean (414 files already formatted, ty: all checks passed) |
mutation test: source reverted to 43c28b1, tests left at HEAD |
3 JS + 7 Python failures |
git merge-tree against the open siblings on the same files (#1712, #1710) |
zero conflicts either way |
The mutation result is worth reading closely: the three JS tests that fail are the two nullish-keepMemory ones and the new error-message assertion. The lifecycle: {} and lifecycle: { autoResume: false } rows pass against the base source, because #1693 already omitted autoPause for a missing onTimeout — only the type changed there. That is the gap the inline comments on the two test files are about.
The open question for infra is answered: the default: keys do not need to come out of the spec
At the pinned infra commit (spec/infra-ref = 0716edb9e8), the generated request type is AutoPause *bool / AutoPauseMemory *bool (packages/api/internal/api/api.gen.go:554-557) — oapi-codegen emits a pointer per non-required field and does not materialize the spec default: at decode time — and the handler applies the default itself from a Go constant: autoPause := DerefOrDefault(body.AutoPause, sandbox.AutoPauseDefault) and autoPauseFilesystemOnly := !DerefOrDefault(body.AutoPauseMemory, true) (sandbox_create.go:146,149, AutoPauseDefault = false). So the server genuinely distinguishes absent from an explicit choice, and today treats absent the same as the value the SDK used to send. An omitted autoPauseMemory also leaves filesystemOnly false, so neither 400 gate (:171 filesystemOnly && !autoPause, :179 filesystemOnly && autoResume.Policy == Any) can fire — which is precisely why the newly-accepted { action: 'kill', keepMemory: undefined } body is safe.
I also created a real sandbox for each newly-legal shape against production, and all four behave as the description says:
| lifecycle | result |
|---|---|
{} |
created, on_timeout='kill', auto_resume=False |
{'auto_resume': False} |
created, same |
{'on_timeout': {'action': 'kill', 'keep_memory': None}} |
created (used to raise locally) |
{'on_timeout': {'action': 'pause', 'keep_memory': None}, 'auto_resume': True} |
created, on_timeout='pause', auto_resume=True |
(All four sandboxes were killed afterwards.)
Two notes that don't fit on a diff line
The change pays off in-repo, in a place the PR could claim. packages/cli/src/commands/sandbox/create.ts:15 hand-copies a narrowed type SandboxLifecycle = { onTimeout: 'pause' | 'kill'; autoResume?: boolean } instead of importing the one e2b exports, and that copy's required onTimeout is the only reason the throw at :187 exists — it is unreachable, since :177 already returns when neither flag is set and :181 already throws for auto-resume with a non-pause action. With onTimeout optional, the CLI can import the real type and drop the dead branch; I checked the copy is assignable to the SDK type, so the swap is mechanical. No test covers the :187 message, which corroborates that it never fires.
One inherited design note, deepened rather than introduced here. keepMemory / keep_memory is now a tri-state carried by a boolean: unset means "API decides", true means memory snapshot, false means filesystem-only, and it takes three paragraphs of docstring to say that unset is not false. TASTE names this exact anti-pattern, with this exact surface as its example ("Sandbox.pause(sandboxId, { mode: 'memory' }), not Sandbox.pause(sandboxId, { keepMemory: true })"), and the repo has a second instance in SandboxPauseOpts.keepMemory. A snapshot?: 'memory' | 'filesystem' would fold the three states into one enum and leave room for a third kind without a breaking change. Not something to fix in this PR — just the direction to take when this field is next touched. Also inherited: SandboxOnTimeoutPause / SandboxOnTimeoutKill are named in Python and appear verbatim in the type-checker error a user reads, but neither is in e2b.__all__, so the annotation that would let someone build the dict incrementally isn't importable.
Credit where it's due
The hint appended to the auto-resume error is gated on the unconfigured branch (onTimeoutConfigured ? '' : …) rather than appended unconditionally, so a caller who did pass onTimeout: 'kill' isn't told to set the knob they already set — that is a trap this repo has fallen into before. build_lifecycle_config being shared means one Python fix covers both mirrors, and the two messages mirror each other in their own language's option syntax. Correcting #1693's unreleased changeset in place is the right call, and I checked its sibling .changeset/omit-unset-auto-resume.md — nothing in #1694's prose is invalidated by this change, so the narrow scope is correct.
Sent by Cursor Automation: /check SDK complies with TASTE.md
|
|
||
| # A keep_memory that is None is no longer sent as True, and no longer trips the | ||
| # pause-only guard on a kill action. | ||
| Sandbox.create(lifecycle={"on_timeout": {"action": "pause", "keep_memory": None}}) |
There was a problem hiding this comment.
This sample does not typecheck, and the realistic version of it doesn't either. Written verbatim into a file and checked with the repo's own checker from packages/python-sdk, ty reports two diagnostics for this line (invalid-argument-type on the create call, plus one naming the on_timeout key), because NotRequired[bool] means "may be absent", not "may be None". The same is true of the call shape the PR is actually motivated by — a keep_memory: Optional[bool] variable that happens to be None — which adds two more. The JS twin twelve lines above compiles clean, so the fix this PR ships is reachable by a typed JS caller and not by a typed Python one. All three lifecycle keys share the trait: {"on_timeout": None}, {"auto_resume": None} and {"keep_memory": None} are each runtime-tolerated and statically rejected (6 diagnostics across those three calls).
Codex's P2 suggests widening the field to accept None. I'd push back on that, on two grounds. TASTE's spelling of absence is omission, not a null sentinel; and the surface is currently uniform about it — of the ~330 NotRequired[...] fields in the option TypedDicts (sandbox_api.py 17, mcp.py 302), zero accept None. The only nine NotRequired[Optional[...]] fields in the package are in CopyItem / Instruction (e2b/template/types.py:112-130), which are the camelCase serialized wire shapes rather than the option surface. Widening one key would also just move the asymmetry, since this PR made all three runtime-lenient — you'd need on_timeout and auto_resume too.
The smaller fix is the sample. Either spelling below typechecks clean, and I verified both produce a request body byte-identical to what the current sample produces at runtime (autoPause: true, autoPauseMemory absent):
# when there is genuinely nothing to say about the snapshot kind
Sandbox.create(lifecycle={"on_timeout": "pause"})
# when the value is conditional
Sandbox.create(
lifecycle={
"on_timeout": {
"action": "pause",
**({"keep_memory": keep_memory} if keep_memory is not None else {}),
}
}
)Worth keeping the sentence about None being treated as unconfigured — the runtime leniency is still the right defensive behavior for untyped callers — but the published sample shouldn't be a snippet CI would reject.
| expect(lastCreateBody?.autoResume).toEqual({ enabled: false }) | ||
|
|
||
| // An empty lifecycle expresses nothing at all. | ||
| await Sandbox.create('base', { apiKey: TEST_API_KEY, lifecycle: {} }) |
There was a problem hiding this comment.
Nothing typechecks this file, so dropping the as never casts asserts nothing. packages/js-sdk/tsconfig.json has "include": ["src"], so pnpm run typecheck (tsc --noEmit) never sees tests/; I appended const _deliberate: number = "not a number" to this file and tsc --noEmit still exited 0. vitest transpiles without typechecking and oxlint has no type information, so no CI leg would notice either.
That matters because the type relaxation is this PR's headline change, and the runtime assertions can't see it: with the source reverted to base, this test — including the lifecycle: {} row on this line — still passes, since #1693 already omitted autoPause for a missing onTimeout. So the JS half of the change currently has no automated verification at all.
Cheapest options, in order of intrusiveness: a *.test-d.ts file under vitest's typecheck mode (no precedent in the repo yet), or a second tsconfig that includes tests for the typecheck script. Either one would also start pinning the negatives the file already relies on — { onTimeout: null } and { action: 'kill', keepMemory: undefined } are both still type errors today, which I confirmed with a @ts-expect-error table in src, but only by hand.
| pytest.param(cast(Any, {"auto_resume": False}), None, id="no-on-timeout-key"), | ||
| # on_timeout is optional, so a lifecycle can leave it out entirely; untyped | ||
| # callers can also pass it as None. Neither selects an action. | ||
| pytest.param({}, None, id="empty-lifecycle"), |
There was a problem hiding this comment.
The Python mirror has the same hole, for a different reason. ty does cover tests/, but these literals travel through pytest.param(...) into an untyped lifecycle parameter, so they arrive as Any and are never checked against SandboxLifecycle — removing the cast(Any, ...) is cosmetic here. Measured: with e2b/sandbox/sandbox_api.py reverted to the base commit, where on_timeout is still required, ty check tests/shared/sandbox/test_lifecycle_request.py still reports "All checks passed!".
Two module-level bindings close it, and unlike the JS side they need no config change since make typecheck already covers this directory:
_TYPED_EMPTY: SandboxLifecycle = {}
_TYPED_NO_ACTION: SandboxLifecycle = {"auto_resume": False}I ran that both ways: on this PR's source ty passes, and against the base source it fails with two missing-typed-dict-key diagnostics naming on_timeout. That is the assertion the cast(Any, ...) removal is standing in for.
| * (currently `kill`) in effect. | ||
| */ | ||
| onTimeout: SandboxOnTimeout | ||
| onTimeout?: SandboxOnTimeout |
There was a problem hiding this comment.
TASTE treats docstrings as part of the API, and the newly accepted spelling is documented only in the changeset. For a typed JS caller, { action: 'pause', keepMemory: undefined } changed behavior in this PR — it used to send autoPauseMemory: true and now omits it — but keepMemory's JSDoc (line 442) still says only "Left unset, the flag is omitted from the create request", which reads as "key absent" rather than "absent or explicitly undefined, e.g. spread in from an optional value". Same for this property: "Omitted from the create request when unset" doesn't tell the reader that an explicit undefined counts as unset. One clause on each would put the PR's own headline example in the published API docs, where the changeset won't be after the release.
While you're in this neighbourhood: this type has no type-level JSDoc at all, and SandboxOpts.lifecycle (line 662) is documented as "Sandbox lifecycle configuration." — whereas the Python mirror carries both a SandboxLifecycle class docstring stating the omitted-on_timeout semantics and a full :param lifecycle: paragraph in sandbox_sync/main.py and sandbox_async/main.py. Since the PR updates the Python paragraph in both mirrors, the JS side is the one that ends up saying less about the exact semantics this PR introduces.
There was a problem hiding this comment.
SDK test coverage — base 43c28b15f → head b46326afe
The runtime half of this PR is genuinely well guarded: every mutant I made to the keepMemory narrowing and the new message hint was caught, by exactly the tests this PR adds. The type half has no automated guard at all — the TASTE review already found this and I confirm it below with the blast radius. And there is one gap nobody has flagged: the new hint's onTimeoutConfigured discriminator is only ever exercised in one state, in both SDKs, so a mutant that makes the hint unconditional survives the whole lifecycle suite. Details inline.
Mutation results (the strongest evidence here; percentages are context)
These suites are offline and run in under a second, so each mutant is cheap to check.
| mutant | result |
|---|---|
M1 — revert onTimeout? / NotRequired[...] back to required (the PR's headline change) |
survives: tsc --noEmit 0 errors, ty check clean, 12/12 JS and 31/31 python tests pass |
M2 — revert the JS keepMemory nullish narrowing |
caught, by exactly the 2 new JS tests |
| M3 — delete the JS hint | caught, by 1 JS test |
M4 — delete the JS keepMemory-on-kill guard |
caught, but only by a pre-existing test in lifecyclePayload.test.ts, a file this PR does not touch |
M5 — revert python keep_memory_provided to the in check |
caught, by exactly the 3 new python tests |
| M6 — delete the python hint | caught, by the 4 parametrized sync+async cases |
| M7 — make the hint unconditional | survives: 65 python and 17 JS lifecycle tests all pass |
The pass/fail split on M2 and M5 is the good news: the new tests fail on the reverted source while the explicit-value tests stay green, which is what you want from tests that are not over-coupled to the implementation.
Coverage A/B (E2B_API_KEY set, so the live suites really ran)
base 43c28b15f |
head b46326afe |
|
|---|---|---|
| python statements | 69.44% (10304/14838) | 69.45% (10307/14841) |
| python branches | 46.69% (1328/2844) | 46.70% (1329/2846) |
| python hand-written | 83.58% (5268/6303) | 83.59% (5271/6306) |
| python generated | 59.00% (5036/8535) | unchanged |
| python tests | 960 pass / 2 fail / 57 skip | 965 / 2 / 57 |
| js-sdk statements | 82.13% (2216/2698) | 82.14% (2218/2700) |
| js-sdk branches | 73.23% (1338/1827) | 73.28% (1339/1827) |
| js-sdk lines | 82.04% (2166/2640) | 82.05% (2168/2642) |
| js-sdk functions | 88.86% (463/521) | unchanged |
| js-sdk tests | 634 pass / 1 fail / 32 skip | 636 / 1 / 32 |
| cli | 17.05% lines (236/1384), 109 pass | byte-identical |
Diff coverage is 100% on both sides (python adds 3 executable statements, all covered; JS adds 2, both covered). Exactly one file moved in each SDK — e2b/sandbox/sandbox_api.py and src/sandbox/sandboxApi.ts — and no file lost line or branch coverage; none were added or removed. Test inventory: +5 python, +2 JS, 0 removed, 0 status changes, and all seven added tests arrive passed rather than skipped.
The quoted base in the trigger was the true merge-base this time, so the A/B is against the right tree. The 2 python and 1 JS failures are the long-standing environmental test_firewall_transform_injects_headers / httpbin failures — identical on base and head, not caused by this PR.
One number worth explaining: python gained 2 branch arms but only 1 covered arm. That uncovered arm is the M7 gap, and it is the only missing branch left in build_lifecycle_config. The JS branch total is unchanged at 238 because the duplicated typeof onTimeout !== 'string' && 'keepMemory' in onTimeout expression this PR removes offsets the new hint ternary.
Confirming the type hole, and sizing it
The TASTE review's two findings both reproduce exactly, so I will not restate them. What I can add is the scale and a working fix:
- 34
@ts-expect-error/@ts-ignoredirectives across 7 js-sdk test files are all inert, for the sameinclude: ["src"]reason — not just the casts in this PR.lifecyclePayload.test.ts:33is the one that matters here: its comment says the discriminated union is "asserted by@ts-expect-error", and that assertion has never been evaluated. Encouragingly, zero of the 34 are currently unused (noTS2578), so every one of them would be meaningful the moment tests are typechecked. - Including all of
tests/is not free: it surfaces 37 pre-existing errors across 18 files (mostly mock and generic shapes incommands/commandHandle.test.ts,envd/http2.test.ts,setup.ts). - A scoped config covering just the two lifecycle test files is free, with one non-obvious catch — see the inline comment on the JS test file.
The type hole is why the keep_memory nit shipped
Codex's P2 and Claude's nit about SandboxOnTimeoutPause.keep_memory being NotRequired[bool] are both correct, and they connect directly to the missing type-test. I annotated the changeset's own documented python example and the Optional[bool]-from-config shape the PR is motivated by:
_TYPED_NONE_KEEP: SandboxLifecycle = {"on_timeout": {"action": "pause", "keep_memory": None}}
_from_config: bool | None = None
_TYPED_OPTIONAL_KEEP: SandboxLifecycle = {"on_timeout": {"action": "pause", "keep_memory": _from_config}}At head that is 2 invalid-argument-type diagnostics from ty, with no source change — while the two on_timeout-omitting literals annotate cleanly. So the annotation the TASTE review proposes does double duty: it guards this PR's NotRequired relaxation and turns the keep_memory defect from something three reviewers caught by hand into a permanent CI check. I then applied the suggested remedy, keep_memory: NotRequired[Optional[bool]], and confirmed ty check returns "All checks passed!" with all four annotated literals present and the 31 lifecycle tests still green.
For context on the family: the autoResume && action !== 'pause' guard that had zero coverage when I measured #1693 ([0, 241] in JS) is now [2, 256], and this PR strengthens it further with regex assertions on the message. That gap is closed; M7 is the same gap having moved one level inward.
No repo changes were needed for this report, so nothing was committed.
Sent by Cursor Automation: /coverage SDK Test Coverage Report
| "auto_resume can only be True when on_timeout action is 'pause'." | ||
| ) | ||
| message = "auto_resume can only be True when on_timeout action is 'pause'." | ||
| if not on_timeout_configured: |
There was a problem hiding this comment.
This is the one new gap: only the True arm of this if ever runs.
853 -> 860 (the false arm) is the sole missing branch left in build_lifecycle_config — coverage.py has every other arc in the function marked executed. Reaching it needs auto_resume truthy alongside an explicitly configured non-pause action, and no test in the suite does that: every auto_resume: True case either omits on_timeout, passes it as None, or pairs it with "pause".
Confirmed by mutation rather than by reading the percentage. Replacing this line with if True: — i.e. appending the hint unconditionally, which is exactly the bug the discriminator exists to prevent — leaves 65 python tests and 17 JS tests all passing. The match= assertions this PR adds only ever check that the hint is present; nothing checks it is absent.
Worth closing because the hint makes a claim about the caller's input ("you left it unset"), so getting it wrong would tell someone who wrote on_timeout: "kill" to go set on_timeout. Verified behaviour at head:
{'on_timeout': 'kill', 'auto_resume': True} -> "...action is 'pause'." (no hint)
{'on_timeout': {'action': 'kill'}, 'auto_resume': True} -> "...action is 'pause'." (no hint)
{'auto_resume': True} -> "...action is 'pause'. Set lifecycle[...]" (hint)
A drop-in test is in my comment on test_lifecycle_request.py.
| if (autoResume && action !== 'pause') { | ||
| // Without a configured action there is no `kill` to name — the SDK no | ||
| // longer decides what an unset onTimeout means — so point at the knob. | ||
| const hint = onTimeoutConfigured |
There was a problem hiding this comment.
Same gap as the python twin, and istanbul shows it precisely: this cond-expr has counts [0, 2] — the hint arm runs twice, the '' arm never runs. It is the only uncovered branch arm in the block this PR touches (the neighbouring guards are healthy: hasKeepMemory && action !== 'pause' is [1, 258], autoResume && action !== 'pause' is [2, 256], !keepMemory && autoResume is [1, 255]).
Mutating this to a plain const hint = " Set lifecycle.onTimeout to 'pause': ..." keeps all 17 tests in lifecycleRequest.test.ts + lifecyclePayload.test.ts green.
One test closes it. I verified this passes at head and fails against that mutant:
test('the unset-onTimeout hint is omitted once an action was configured', async () => {
await expect(
Sandbox.create('base', {
apiKey: TEST_API_KEY,
lifecycle: { onTimeout: 'kill', autoResume: true },
})
).rejects.toThrowError(
/^autoResume can only be true when onTimeout action is 'pause'\.$/
)
})The anchored $ is what does the work — it is what makes the assertion about the hint's absence rather than just the base message.
|
|
||
| assert body["autoPause"] is True | ||
| assert "autoPauseMemory" not in body | ||
| assert body["autoResume"] == {"enabled": True} |
There was a problem hiding this comment.
Drop-in for the python side of the hint gap (see my comment on sandbox_api.py:853). I appended exactly this, ran it at head — 33 passed — and then re-applied the if True: mutant, where both cases fail:
CONFIGURED_ACTION_AUTO_RESUME_CASES = [
pytest.param({"on_timeout": "kill", "auto_resume": True}, id="explicit-kill"),
pytest.param(
cast(Any, {"on_timeout": {"action": "kill"}, "auto_resume": True}),
id="explicit-kill-object",
),
]
@pytest.mark.parametrize("lifecycle", CONFIGURED_ACTION_AUTO_RESUME_CASES)
def test_create_omits_the_unset_hint_once_an_action_was_configured(
test_api_key, lifecycle
):
# The hint exists to avoid naming a default the SDK no longer decides. When
# the caller *did* choose an action, there is nothing to point at.
with pytest.raises(InvalidArgumentException) as excinfo:
Sandbox.create(api_key=test_api_key, lifecycle=lifecycle)
assert "Set lifecycle" not in str(excinfo.value)Both the bare-string and object forms are worth keeping: they reach on_timeout_configured through the two different arms of the isinstance(on_timeout_raw, dict) split above.
These need no SandboxLifecycle annotation to do their job, since they assert runtime behaviour — but if you take the annotated-binding suggestion elsewhere in this file, note the second case genuinely does need its cast, because SandboxOnTimeoutKill has no auto_resume-adjacent problem but {"action": "kill"} with auto_resume: True is a combination the union is designed to reject.
| expect(lastCreateBody).not.toHaveProperty('autoResume') | ||
| }) | ||
|
|
||
| test('a nullish keepMemory is not a choice of snapshot kind', async () => { |
There was a problem hiding this comment.
Adding to the TASTE review's finding on this file (which reproduces — I confirmed tsc --noEmit never reads tests/): here is a config that actually works, plus the trap in the obvious version.
The naive fix, adding these files to include, reports 3 TS4023 errors in tests/setup.ts that have nothing to do with the tests — volumeTest's inferred type references unnameable vitest internals. They appear only because tsconfig.json sets "declaration": true, which a --noEmit type-check pass does not need. Turning it off clears them:
{
"extends": "./tsconfig.json",
"compilerOptions": { "declaration": false },
"include": [
"src",
"tests/sandbox/lifecycleRequest.test.ts",
"tests/sandbox/lifecyclePayload.test.ts"
]
}Measured both ways: 0 errors at head, and 4 TS2741 errors (lines 100, 108, 132, 143 of this file) once onTimeout is made required again. So it is a real guard on this PR's headline change, and it opts in only files that are already clean — unlike including all of tests/, which surfaces 37 pre-existing errors across 18 files.
It would also revive lifecyclePayload.test.ts:33, whose comment claims the discriminated union is "asserted by @ts-expect-error" — one of 34 such directives in js-sdk tests that are currently never evaluated.
|
@devin-ai-integration please check |
|
superseded by #1749 |
1 participant
Follow-up to #1693 and #1694.
Problem
#1693 stopped sending
autoPausewhen no timeout action was configured, and #1694 did the same forautoResume. Both describe an "onTimeoutnot configured" state in their docs — butonTimeoutstayed a required member ofSandboxLifecyclein both SDKs, so that state is unreachable for a typed caller except by omittinglifecycleentirely. Every test the two PRs added for it has to cast through the type (as never,cast(Any, ...)).Concretely,
lifecycle: { autoResume: false }— "no preference about the timeout action, but keep auto-resume off" — is exactly the shape #1694 made meaningful on the wire, and it does not typecheck.Three related cleanups fall out of that:
keepMemory/keep_memorywas still treated as an explicit choice ('keepMemory' in onTimeout), so it sentautoPauseMemory: trueand tripped the pause-only guard on a kill action — the one place the two PRs' own "nullish means not configured" rule didn't apply. Spreading an optional value in ({ action: 'pause', keepMemory: maybeUndefined }) is a normal call shape.autoPauseMemoryfor a barepauseafter that PR. Corrected in place, since it hasn't been released yet.Change
lifecycle: { autoResume: false }autoPauseomitted,autoResume: { enabled: false }lifecycle: {}{ action: 'pause', keepMemory: undefined }autoPauseMemory: trueautoPauseMemoryomitted{ action: 'kill', keepMemory: undefined }InvalidArgumentErrorautoPause: false{ action: 'kill', keepMemory: false }InvalidArgumentError{ autoResume: true }with no actionInvalidArgumentErrorautoResume: truestill requires an explicitonTimeout: 'pause'— auto-resume only has meaning for a sandbox that pauses, and sending it alongside an unset action would ask the API for a combination it has no way to honor today. The error now points at the knob to turn instead of asserting what an unsetonTimeoutresolves to:Usage
AsyncSandbox.createbehaves identically.Tests
Extended the two request-level suites the previous PRs added; the cases that used to need a cast are now written as typed calls, and the new nullish-
keepMemorybehavior is covered on both sides.packages/js-sdk/tests/sandbox/lifecycleRequest.test.ts— 12 passed (msw, no credentials)packages/python-sdk/tests/shared/sandbox/test_lifecycle_request.py— 31 passed (no credentials)Also run:
pnpm run format,pnpm run lint,pnpm run typecheckclean from the repo root;packages/python-sdktests/shared163 passed (the 25 errors aretests/shared/git, which needsE2B_API_KEYto create sandboxes);packages/cli108 passed (the one failing suite is the template-create backend test, which also needsE2B_API_KEY).No live-API coverage was added: nothing here changes what an explicitly-configured lifecycle puts on the wire, and the existing
lifecyclePayload.test.tssuite still covers the real pause/resume behavior.Open question for infra
spec/openapi.ymldeclaresautoPause: default: falseandautoPauseMemory: default: trueonNewSandbox. The premise of #1693/#1694 — that the API can now tell "unset" from an explicit choice and own its own default — only holds if the server distinguishes an absent field from the spec default rather than filling it in at decode time. Worth confirming on the infra side; if it doesn't, thosedefault:keys need to come out of the spec for the change to have teeth.🤖 Generated with Claude Code