Remove client-side fork count validation - #1753
Conversation
Co-Authored-By: mish@e2b.dev <mish@e2b.dev>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
🦋 Changeset detectedLatest commit: 8b90a6d 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 a27591f. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.45.1-devin-1787329731-drop-fork-count-validation.0.tgzCLI ( npm install ./e2b-cli-2.17.2-devin-1787329731-drop-fork-count-validation.0.tgzPython SDK ( pip install ./e2b-2.45.1+devin.1787329731.drop.fork.count.validation-py3-none-any.whl |
There was a problem hiding this comment.
Checked against TASTE.md: T-52 (client-side validation must not mirror backend business rules), T-59 (InvalidArgumentError/InvalidArgumentException for pre-operation argument validation), T-1/T-2 (JS / sync Python / async Python parity), T-33 (per-item fork outcomes), T-69/T-70/T-71 (docs). No violations found.
The removed count < 1 check was exactly the duplicated backend rule T-52 forbids — SandboxForkRequest.count in spec/openapi.yml carries minimum: 1, maximum: 100, and the SDK never mirrored the maximum, so the local check only covered half the server rule and would drift. The removal is symmetric across all three surfaces (T-1, T-2), the corresponding tests are dropped in all three, the now-unused InvalidArgumentException imports are cleaned up in Python (the JS InvalidArgumentError import is still used elsewhere in sandboxApi.ts), and no docstring/JSDoc documented the removed error, so there is no doc drift. Changeset included.
There was a problem hiding this comment.
LGTM — mechanical removal of client-side fork count validation, consistently applied across JS and Python sync/async, per T-52.
Extended reasoning...
Overview
The PR removes the client-side count < 1 pre-validation from forkSandbox (JS) and Sandbox.fork/AsyncSandbox.fork (Python sync + async), letting the API's own validation reject invalid counts instead. Corresponding tests that asserted the client-side InvalidArgumentError/InvalidArgumentException were deleted, and a changeset was added for both packages.
Security risks
None. This narrows client-side validation in favor of relying on the backend, which is explicitly the intended trust boundary per the referenced TASTE T-52 principle. No new attack surface is introduced — an invalid count now simply reaches the API and gets rejected there instead of client-side.
Level of scrutiny
Low. This is a small, mechanical, symmetric change across three SDK implementations (JS, Python sync, Python async), with no logic changes beyond deleting a guard clause. I verified the now-unused InvalidArgumentException import was correctly dropped from both Python files (no other usages remain), while InvalidArgumentError in the JS file is still used elsewhere and correctly retained.
Other factors
Tests were updated in lockstep with the implementation change in all three SDKs, and a changeset is included for both affected packages. The change is a direct, explicit follow-up to a prior review discussion (#1749), so it reflects a deliberate design decision already discussed by maintainers.
|
Folded into #1749 per Mish's request. |
Summary
Per updated TASTE T-52 (backend is the only trust boundary; client validation must not mirror backend business rules), drop the SDK-side
count >= 1pre-validation inforkSandbox(JS) andSandbox.fork/AsyncSandbox.fork(Python sync + async). An invalid count now surfaces as the API's own 400 error instead of a client-sideInvalidArgumentError/InvalidArgumentException.Tests asserting the removed client-side rejection are deleted. Follow-up to #1749 review discussion.
Link to Devin session: https://app.devin.ai/sessions/1921bb3818604f3a95b0db1272cff3cd
Requested by: @mishushakov