fix(sdk): let callers omit lifecycle.onTimeout - #1703
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 |
PR SummaryLow Risk Overview Nullish
Reviewed by Cursor Bugbot for commit b46326a. Bugbot is set up for automated code reviews on this repo. Configure here. |
Package ArtifactsBuilt from c23edbe. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.41.1-fix-lifecycle-optional-on-timeout.0.tgzCLI ( npm install ./e2b-cli-2.16.3-fix-lifecycle-optional-on-timeout.0.tgzPython SDK ( pip install ./e2b-2.41.0+fix.lifecycle.optional.on.timeout-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".
|
|
||
| # 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.
When users copy this new example into a type-checked Python project, Pyright rejects it because SandboxOnTimeoutPause.keep_memory remains NotRequired[bool], so None is not accepted even though the runtime now treats it as unset. Either widen the public type to accept None, matching the advertised JS/Python behavior, or avoid documenting None as a supported typed call.
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 — a clean, consistent follow-up to the onTimeout/autoResume changes in #1693/#1694, and no bugs were found.
Extended reasoning...
Overview
This PR relaxes SandboxLifecycle.onTimeout (on_timeout in Python) from required to optional in both the JS and Python SDKs, and applies the same "nullish means unconfigured" treatment that #1693/#1694 established for onTimeout/autoResume to the nested keepMemory/keep_memory field. It also tweaks the autoResume-without-a-pause-action error message to point at the knob to fix rather than naming a default the SDK no longer decides, and corrects a changeset wording inaccuracy from #1693 (not yet released). Touches: sandboxApi.ts and sandbox_api.py (the request-building logic), the two SDKs' main.py docstrings, and both request-level test suites.
Security risks
None. This is purely SDK-side request-shaping logic (which fields get included in the create-sandbox payload) — no auth, crypto, or permission logic involved, and no user input is parsed or executed differently.
Level of scrutiny
Low-to-moderate is appropriate: it's a type-relaxation plus a bug fix (nullish keepMemory previously behaved inconsistently with the sibling onTimeout nullish-handling), not a new capability or behavior change for typed callers who were already setting these fields explicitly. The JS and Python implementations were changed in parallel and use the same reasoning (requestedKeepMemory ?? undefined / keep_memory is not None), so the two SDKs stay in sync as required by this repo's conventions.
Other factors
I traced the hasKeepMemory/keep_memory_provided guard logic against each of the PR's documented before/after table entries (bare pause with keepMemory: undefined, kill with keepMemory: undefined, autoResume alongside an unconfigured keepMemory) and the code matches the described behavior in both languages. I also grepped for other call sites relying on onTimeout/on_timeout being required and found none outside the generated API-response models (which are a separate, unrelated type for server responses, not this input type). Test coverage is thorough and directly exercises the changed branches; CI (format/lint/typecheck, both test suites) is reported green in the PR description.
|
/sdk claim |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claimed. This PR has been cloned into #1711, which now carries the work.
- The commit
b46326ais applied unmodified, so your authorship and theCo-Authored-Bytrailer are preserved — only PR ownership moves. - The branch was already current with
main, so no merge was needed; the tree on the new branch is byte-identical to this PR's head. - Contents were neither reviewed nor modified, and the description was carried over verbatim.
Please close this PR in favour of #1711 — I don't have write access to close it myself.
Sent by Cursor Automation: /claim Claim SDK PR by SDK Factory
There was a problem hiding this comment.
SDK test coverage — base 43c28b15f → head b46326afe
Measured both SDKs at both ends with E2B_API_KEY present, so the live suites really ran. The quoted diff base is the true merge-base here, so the A/B is clean.
Diff coverage is 100%, and it is the least informative number in this report. Only 5 of the 219 added lines are executable (3 python, 2 JS) and all 5 execute. The PR's headline change — making onTimeout / on_timeout optional — contributes zero executable lines, so coverage cannot see it at all. I mutation-tested the PR's own suites instead.
Mutation results
Revert one piece of the change, run only the two request-level suites the PR extends (0.6 s JS / 0.11 s python), restore.
| mutant | PR's tests | typechecker | verdict |
|---|---|---|---|
JS onTimeout? → required again |
12/12 pass | tsc --noEmit clean |
survived |
py NotRequired[...] → required again |
31/31 pass | ty check clean |
survived |
JS hasKeepMemory back to the in operator |
2 fail | — | caught |
py keep_memory_provided back to the in operator |
3 fail | — | caught |
py hint appended unconditionally (if True:) |
31/31 pass | — | survived |
| JS hint appended unconditionally | 12/12 pass | — | survived |
py hint never appended (if False:) |
4 fail | — | caught |
JS hint never appended (const hint = '') |
1 fail | — | caught |
The nullish-keepMemory fix — the one genuinely behavioural change here — is well guarded on both sides, and the split is exactly right: reverting it fails only the new nullish tests and leaves the explicit-value tests green. Both new suites assert at wire level (msw request.json() in JS, to_dict() in python), which is the correct target.
The four survivors are two findings, both symmetric across the SDKs, both detailed inline:
-
The new conditional hint is half-tested. Every
autoResume: truein either repo is paired with'pause'or with an unset/Noneaction — nothing anywhere passes an explicit'kill'alongsideautoResume: true. So the arm that must not append the hint never runs. Coverage agrees precisely: python gains one uncovered branch arc (853 → 860) in a function that had none on base, and JS'shintternary reads[0, 2]. I wrote the missing test for each side, confirmed it passes on head and fails against the surviving mutant. -
The type change has no automated guard in either SDK — for two different reasons. Neither typechecker is incapable; both catch it when the value reaches a checked call site.
tscnever looks attests/, andtydoes look but the PR's values arrive throughpytest.parametrize, which erases the type.
Answering the "Open question for infra"
I probed the live API rather than reasoning about it — created sandboxes and read get_info().lifecycle:
lifecycle sent |
observed back |
|---|---|
| omitted entirely | on_timeout: "kill", auto_resume: false |
{} |
on_timeout: "kill", auto_resume: false |
{"auto_resume": False} |
on_timeout: "kill", auto_resume: false |
{"on_timeout": "kill"} |
on_timeout: "kill", auto_resume: false |
{"on_timeout": "pause"} |
on_timeout: "pause", auto_resume: false |
Two things follow. The new shapes work end to end — lifecycle: {} and lifecycle: {"auto_resume": False} are accepted, create a sandbox, and behave as kill/no-auto-resume, so nothing regresses. But omitting is currently observationally identical to sending the old explicit default, so this change buys future-proofing and type ergonomics, not a behaviour change today; the probe can't distinguish "server honors absence" from "server default happens to match the spec default". Worth stating plainly so nobody reads the green suite as confirmation that the premise holds.
Also worth flagging for whoever picks that question up: SandboxInfoLifecycle exposes only on_timeout and auto_resume — there is no auto_pause_memory on the read path. So the field this PR actually changes behaviour for is unobservable end to end; it can only ever be asserted at the request-body level, which is what the new tests do. That is the right call, and it is also the ceiling.
Numbers
| base | head | |
|---|---|---|
| python stmts | 69.44% (10303/14838) | 69.44% (10306/14841) |
| python branches | 46.69% (1328/2844) | 46.63% (1327/2846) |
| python hand-written | 83.56% (5267/6303) | 83.57% (5270/6306) |
| python generated | 59.00% (5036/8535) | 59.00% (5036/8535) |
| python tests | 960 pass / 2 fail / 57 skip | 965 / 2 / 57 |
| js-sdk stmts | 82.13% (2216/2698) | 82.14% (2218/2700) |
| js-sdk lines | 82.04% (2166/2640) | 82.05% (2168/2642) |
| js-sdk branches | 73.23% (1338/1827) | 73.23% (1338/1827) — identical |
| js-sdk funcs | 88.86% (463/521) | 88.86% (463/521) |
| js-sdk tests | 634 pass / 1 fail / 32 skip | 636 / 1 / 32 |
| cli | 17.05% lines (236/1384), 109 pass | byte-identical |
The two test_firewall_transform_injects_headers failures and the js httpbin sidecar failure reproduce identically on base — the test org lacks the httpbin template. Not regressions.
Inventory: +5 python, +2 JS, 0 removed, 0 status changes, and every added test runs (none arrived skipped). Note the inventory undercounts the work: several of the PR's best additions are new assertions inside existing tests (the {} case, the two message-regex assertions), which no test-name diff can see.
Two notes on why the totals are useless for this PR, beyond the 5-executable-lines point:
- js-sdk branch counts are byte-identical on both sides, which hides the newly-uncovered arm completely. The
keepMemoryrefactor removed exactly as many branch arms as it added (44 → 42 in the lifecycle block, 4 zero-count arms at both ends), so the new dead arm is masked by a coincidence. Only the per-arm comparison shows it. - The one per-file mover among untouched files is noise I can attribute:
e2b/envd/api.py+2 lines, andtemplate_{sync,async}/build_api.py−1 line/−1 branch each (the live template suite). That −2 against sandbox_api's +1 is the whole −1 in the python branch total.
One genuinely nice piece of evidence for the fix being real: on base, the ?? true fallback inside the old keepMemory ternary had counts [5, 0] — a present-but-nullish keepMemory was never once exercised. That dead arm is exactly why the bug this PR fixes went unnoticed, and the PR deletes it.
Verdict
Strong test PR for the behavioural half — the nullish-keepMemory change is properly pinned, on both the sync and async python paths (the three new keep_memory tests are sync-only, but build_lifecycle_config is shared and both sandbox_{sync,async}/sandbox_api.py call it, and the async twin covers the wiring, so that matches the file's existing convention rather than being a gap). The two real gaps are the untested no-hint arm and the entirely unguarded type change, both cheap to close and both with verified fixes inline.
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 conditional is half-tested, and it is the only uncovered thing the PR adds.
On base, build_lifecycle_config had zero uncovered lines and zero uncovered branch arcs. On head it has exactly one: arc 853 → 860, the false side of this if — i.e. auto_resume: True alongside an action the caller did configure but that isn't pause. The JS twin is identical: the hint ternary at sandboxApi.ts:1608 reads [0, 2].
The reason is that no test in either repo pairs autoResume: true with an explicit 'kill'. Every one of the 8 python and 7 JS occurrences uses 'pause', or an unset/None action. So making the hint unconditional passes the whole suite — I ran it: if True: here → 31/31 python still green, and the JS equivalent → 12/12 still green.
That matters because the failure mode is the exact confusion this PR set out to remove. The behaviour on head is correct:
{'on_timeout': 'kill', 'auto_resume': True} -> "...action is 'pause'."
{'on_timeout': {'action': 'kill'}, 'auto_resume': True} -> "...action is 'pause'."
{'auto_resume': True} -> "...action is 'pause'. Set lifecycle['on_timeout'] to 'pause': leaving it unset defers the action to the API."
But nothing holds it there, so a later edit could tell a caller who did set on_timeout to "leave it unset", with CI green.
Verified fix — passes on head, fails against the surviving mutant:
@pytest.mark.parametrize(
"on_timeout",
[
pytest.param("kill", id="bare-kill"),
pytest.param({"action": "kill"}, id="object-kill"),
],
)
def test_explicit_kill_auto_resume_error_does_not_suggest_leaving_on_timeout_unset(
test_api_key, on_timeout
):
with pytest.raises(InvalidArgumentException) as excinfo:
Sandbox.create(
api_key=test_api_key,
lifecycle={"on_timeout": on_timeout, "auto_resume": True},
)
assert "leaving it unset" not in str(excinfo.value)This also closes the arc, taking the function back to fully covered.
| 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 half-covered conditional as the python side (see the comment on build_lifecycle_config): istanbul records this ternary as [0, 2] — the '' arm, taken when the caller did configure a non-pause action, never runs.
It is invisible in the summary because this file's branch total is byte-identical on base and head (1338/1827 overall, 44 → 42 arms in this block, 4 zero-count arms at both ends): the keepMemory refactor happened to remove as many arms as the hint ternary added. Only a per-arm comparison surfaces it.
Making the hint unconditional keeps all 12 tests in lifecycleRequest.test.ts green. Verified fix — passes on head, fails against that mutant:
test('an explicit kill action is not told to leave onTimeout unset', 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 regex is what does the work — it fails if the hint is appended when an action was given.
| * (currently `kill`) in effect. | ||
| */ | ||
| onTimeout: SandboxOnTimeout | ||
| onTimeout?: SandboxOnTimeout |
There was a problem hiding this comment.
This line — the PR's headline change — has no automated guard whatsoever in the JS SDK. I reverted it to onTimeout: SandboxOnTimeout and got: lifecycleRequest.test.ts 12/12 still passing, pnpm run typecheck clean, pnpm run lint unaffected. Nothing in CI notices.
It is not that the contract is unexpressed — your tests express it perfectly, that is precisely what dropping the as never casts did. They are just never typechecked:
packages/js-sdk/tsconfig.jsonis the only tsconfig and sets"include": ["src"], andtypecheckis a baretsc --noEmit, sotests/**is excluded.vitest.config.mtsdeclares notypecheckblock, so vitest strips types via esbuild without checking them.oxlintisn't type-aware.
Adding tests to the include list makes the guard appear immediately — 4 errors, all in this PR's own test file, and nothing else:
tests/sandbox/lifecycleRequest.test.ts(100,5): error TS2741: Property 'onTimeout' is missing in type '{ autoResume: false; }' but required in type 'SandboxLifecycle'.
tests/sandbox/lifecycleRequest.test.ts(108,56): error TS2741: Property 'onTimeout' is missing in type '{}' but required in type 'SandboxLifecycle'.
tests/sandbox/lifecycleRequest.test.ts(132,7): error TS2741: ...
tests/sandbox/lifecycleRequest.test.ts(143,7): error TS2741: ...
The honest cost: flipping include is not free, because tests/ has 37 pre-existing errors across 18 files today (worst offender 5, in tests/sandbox/commands/commandHandle.test.ts) — bounded, but not a one-liner, and out of scope here. A tsconfig.tests.json plus a typecheck:tests script scoped to a growing allowlist would let this PR's file be guarded now without blocking on that cleanup.
Not blocking for this PR — but as long as it holds, "the tests cover the optional onTimeout" is true only of the source text, not of anything CI runs.
| 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.
Unlike the JS side, ty check does cover tests/ (python-sdk's typecheck is uv run make typecheck → bare ty check, no path filter, and the Typecheck workflow runs on this PR). But it still doesn't guard the NotRequired change, because pytest.parametrize erases the type before it reaches Sandbox.create — these params are consumed through an unannotated lifecycle argument, so no checked call site ever sees the new shape.
I verified both halves. Reverting on_timeout to required leaves this file at 31/31 passing and ty check reporting All checks passed!. But the same shape written as a direct call is caught immediately:
error[missing-typed-dict-key]: Missing required key 'on_timeout' in TypedDict `SandboxLifecycle` constructor
--> Sandbox.create(api_key="x", lifecycle={"auto_resume": False})
3 diagnostics with the revert, 0 on head. So one directly-written call anywhere under packages/python-sdk turns CI's existing ty check into a real guard for this PR's headline change, at zero runtime cost — something like:
def test_optional_on_timeout_typechecks(monkeypatch, test_api_key):
# Written inline rather than via parametrize so `ty` actually checks the shape.
body = _sync_request_body(monkeypatch, test_api_key, {"auto_resume": False})
assert "autoPause" not in body
assert body["autoResume"] == {"enabled": False}(The _sync_request_body signature would need lifecycle: SandboxLifecycle for the check to bite — it's currently unannotated.)
Dropping the casts here is still the right change; it just documents the contract rather than enforcing it.
There was a problem hiding this comment.
TASTE review
The design is compliant on the points that matter, and the premise this PR flagged as unresolved checks out — I verified it against the pinned infra server, so the "Open question for infra" can be closed. One finding worth fixing before merge (in the changeset, not the code) and four non-blocking notes; details inline.
What holds up under TASTE: the option type and the result type stay separate, so relaxing SandboxLifecycle cannot weaken what getInfo() guarantees (SandboxInfoLifecycle keeps both members required). Absence is spelled undefined rather than null in the JS types, with null normalized at the boundary for untyped callers — I confirmed with a compiled probe that onTimeout: null and keepMemory: null are both type errors while lifecycle: {}, onTimeout: undefined and keepMemory: someOptional all typecheck. The pre-operation guards throw InvalidArgumentError / InvalidArgumentException, which is what TASTE asks for validation that runs before any operation starts. All four lifecycle docstring sites were updated, and the two main.py :param lifecycle: lines are byte-identical.
The open question for infra: answered, and no spec change is needed
The PR body asks whether the server distinguishes an absent field from the spec default:, and warns that the default: keys may need to come out of spec/openapi.yml for #1693/#1694/#1703 to have teeth. They do not. At the pinned ref (spec/infra-ref = 0716edb9e8):
packages/api/internal/api/api.gen.godeclaresAutoPause *boolandAutoPauseMemory *boolwithomitempty. oapi-codegen emits a pointer for every non-required field and does not materializedefault:at decode time, so an omitted key reaches the handler as a nil pointer rather than as the spec default.packages/api/internal/handlers/sandbox_create.goapplies the default itself, from server-side constants: line 146 isautoPause := sharedUtils.DerefOrDefault(body.AutoPause, sandbox.AutoPauseDefault)withAutoPauseDefault = false(sandbox/sandboxtypes/states.go:104), and line 149 isautoPauseFilesystemOnly := !sharedUtils.DerefOrDefault(body.AutoPauseMemory, true).
So the server both distinguishes nil and resolves it to exactly the values the SDK used to send, which is the "distinguishes, and treats it the same today" test these three PRs need. The two 400s on this surface also fall on the safe side of the new omission: an omitted autoPauseMemory makes autoPauseFilesystemOnly false, so neither the filesystemOnly && !autoPause check (line 171) nor the filesystemOnly && autoResume check (line 179) can fire — which is what makes the newly-permitted { action: 'kill', keepMemory: undefined } case safe. And since sandboxLifecycleToAPI only maps onTimeout and autoResume, no autoPauseMemory value is handed back, so the result surface is untouched.
Worth noting for the record: AutoPauseDefault = false means the @default kill / "currently kill" wording in the docstrings is still accurate.
Non-blocking
keepMemoryis now a tri-state boolean. After this PR it means unset (API default),true(memory snapshot) andfalse(filesystem-only), and it takes three docstring paragraphs to explain that unset is notfalse. TASTE names this exact case as the anti-pattern to avoid —Sandbox.pause(sandboxId, { mode: 'memory' }), not{ keepMemory: true }. This is inherited, not introduced (SandboxPauseOpts.keepMemoryhas the same shape and predates the lifecycle option), so it is not this PR's to fix; but if the option is ever revisited, asnapshot?: 'memory' | 'filesystem'union would express the three states without the prose, and absence would be the natural spelling of "let the API decide".- The object form of
onTimeoutisn't exported. Python namesSandboxOnTimeoutPause/SandboxOnTimeoutKillbut omits them frome2b.__all__(hasattr(e2b, 'SandboxOnTimeoutPause')isFalse), and JS leaves both shapes anonymous inside the union. That matters more after this PR, because building the dict incrementally is the workaround for the typing gap below, and there is no importable name to annotate it with. Pre-existing. - An in-repo consumer can now drop dead code.
packages/cli/src/commands/sandbox/create.ts:187-191throws--lifecycle.ontimeout is required when using --lifecycle.autoresume, but that branch is unreachable: line 177 already returns when neither option is set, and line 181 already throws whenautoResumeis set with a non-pauseaction, so reaching 187 would requireautoResumetruthy,onTimeout === 'pause'and!onTimeoutsimultaneously. It exists only to satisfy the then-requiredonTimeoutwhen constructing the return value, and nothing tests that message (tests/commands/sandbox/create_lifecycle.test.tsasserts only the line-181 one). Nice follow-up that shows the change paying off. - Two prose nits. The changeset's closing line writes
autoResume: True, mixing the JS option name with Python casing — changeset text is copied verbatim into both published CHANGELOGs. Separately, thekeepMemory-on-killguard readsonTimeout.keepMemory is only allowed when action is 'pause'.in JS but barekeep_memory is only allowed when on_timeout action is 'pause'.in Python; TASTE's own example is the JS wording, so the Python mirror is missing the parent path. Both pre-existing, and this PR changes when that guard fires, so it is a cheap thing to bring in scope.
What I verified
- The PR's numbers reproduce. 12 JS tests in
lifecycleRequest.test.ts, 31 Python tests intest_lifecycle_request.py, and rootformat/lint/typecheckall clean (formatleaves no diff). Thetests/sharedfigure reconciles too: the body's "163 passed, 25 errors" is the no-credentials run, and with a key present the same directory is 188 passed / 1 skipped (163 + 25 = 188). - The new tests have teeth. Reverting only the two source files to the base commit and leaving the tests at HEAD fails 3 JS and 7 Python tests. The JS cases that still pass are the ones where only the type changed and #1693 already produced the right runtime behavior, which is the expected result rather than a gap.
- The wire behavior matches the table in the description, checked by capturing the request body directly rather than reading the ternaries: a nullish
keepMemorysendsautoPause: truewithautoPauseMemoryomitted, and{ action: 'kill', keepMemory: undefined }now sendsautoPause: falsewithautoPauseMemoryomitted instead of raising. - No sibling conflict. #1702 touches the same two files and merges into this branch with zero conflicts (different regions — network builders versus lifecycle).
- One unrelated failure in the js-sdk
unitproject on my VM:network.test.ts"injected header is reflected by the httpbin sidecar" fails with404: template 'httpbin' not found. That is the test key's team lacking the fixture template, not anything from this PR (459 passed, 30 skipped otherwise).
Codex posted no suggestions and claude[bot] posted an LGTM at self-described "Low" scrutiny; I did not find a bug in the request-building logic either, so those verdicts hold as far as behavior goes. The finding below is about the typed surface the PR advertises rather than about what it puts on the wire.
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 changeset is the one artifact here that ships verbatim to users (into both packages/js-sdk/CHANGELOG.md and packages/python-sdk/CHANGELOG.md). Written to a file and run through the repo's own checker from packages/python-sdk:
$ uv run ty check --output-format concise /tmp/x.py
/tmp/x.py:3:16: error[invalid-argument-type] Argument to bound method `create` is incorrect: Expected `SandboxLifecycle | None`, found `dict[...]`
/tmp/x.py:3:41: error[invalid-argument-type] Invalid argument to key "on_timeout" with declared type `Literal["pause", "kill"] | SandboxOnTimeoutPause | SandboxOnTimeoutKill` on TypedDict `SandboxLifecycle`
The JS twin above it compiles clean, so this is the asymmetry described in the other comment rather than a typo. The read_from_config() variant in the PR description has the same problem when the function returns Optional[bool].
Since the recommendation is to leave the type strict, the sample is what should change. This version typechecks, and I confirmed with a request-capture probe that it produces exactly the wire behavior the changeset describes — keep_memory=None gives autoPause: true with autoPauseMemory omitted, True and False send autoPauseMemory accordingly:
# A keep_memory the caller does not have an opinion about is left out of the
# request entirely, so the API's default applies.
Sandbox.create(
lifecycle={
"on_timeout": {
"action": "pause",
**({"keep_memory": keep_memory} if keep_memory is not None else {}),
}
}
)One more prose nit further down: line 33 writes autoResume: True, mixing the JS option name with Python's capitalization.
| """ | ||
|
|
||
| on_timeout: SandboxOnTimeout | ||
| on_timeout: NotRequired[SandboxOnTimeout] |
There was a problem hiding this comment.
The JS half of this change fully delivers what the PR sets out to do; the Python half delivers only one of the two states, and I think the right fix is in the docs rather than here.
NotRequired[T] makes the key optional but still rejects None as a value, whereas the JS mirror onTimeout?: SandboxOnTimeout accepts undefined (exactOptionalPropertyTypes is off). All three lifecycle keys — on_timeout, keep_memory and auto_resume — are now runtime-lenient toward None but type-strict against it. That is exactly why the JS test file could drop its as never casts in this PR while the Python test file still routes every None row through cast(Any, ...): those calls remain unexpressible for a typed caller. Python has no undefined, so a caller holding an Optional[bool] from config has no direct spelling.
I checked that widening works mechanically — changing this line and keep_memory to NotRequired[Optional[...]] makes the changeset's sample and all of ty pass with 31 tests still green, and turns 5 of the 7 cast sites in the test file into plain typed calls. I am not recommending it, because TASTE's "absence is undefined, never null" rules out the T | null shape in an optional property, and the same conclusion was reached for NotRequired[str] on the egressProxy credentials in #1702 — worth keeping the two consistent.
So the code here looks right as it stands. What needs to change is the advertising: the changeset and the PR description both present keep_memory=None as the Python mirror of the JS keepMemory: undefined idiom, and it isn't one. Concrete alternative in the changeset comment.
One small thing that follows from this: nothing typed can reach the None branches, so the cast(Any, ...) in the new tests is correct and should stay — it is documenting untyped-caller tolerance, not a shape users are meant to write. A sentence to that effect above NO_ACTION_AUTO_RESUME_CASES would keep a future reader from "fixing" the casts away.
| // longer decides what an unset onTimeout means — so point at the knob. | ||
| const hint = onTimeoutConfigured | ||
| ? '' | ||
| : " Set lifecycle.onTimeout to 'pause': leaving it unset defers the action to the API." |
There was a problem hiding this comment.
Gating the hint is right — an unconditional hint that tells the user to do the thing they already did was a real defect on #1687 — but this is gated the less useful way round. The branch that gets the hint is the one where onTimeout is unset, and the branch that gets nothing is { onTimeout: 'kill', autoResume: true }, where the user has a concrete line in front of them to change. TASTE asks error messages to say what to do, so if either case deserves the pointer it is arguably the configured one.
A single message covers both without a branch, and states the rule in the option names the caller typed:
autoResume can only be true when lifecycle.onTimeout is 'pause'. Leaving onTimeout unset defers the action to the API, which does not enable auto-resume.
Non-blocking, and if you keep the branch, note the Python side spells the same knob lifecycle['on_timeout'] while this one says lifecycle.onTimeout — correct per-language idiom, just worth a glance to confirm it was deliberate.
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