refactor(sdk): remove dead code and de-duplicate SDK/CLI helpers - #1766
refactor(sdk): remove dead code and de-duplicate SDK/CLI helpers#1766devin-ai-integration[bot] wants to merge 1 commit into
Conversation
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
🤖 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:
|
|
Package ArtifactsBuilt from 5c4c4b7. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.45.1-devin-1787555919-sdk-dead-code-cleanup.0.tgzCLI ( npm install ./e2b-cli-2.17.2-devin-1787555919-sdk-dead-code-cleanup.0.tgzPython SDK ( pip install ./e2b-2.45.1+devin.1787555919.sdk.dead.code.cleanup-py3-none-any.whl |
There was a problem hiding this comment.
Reviewed against TASTE.md from e2b-dev/sdk-harness, focused only on lines this PR touches. Checked: API shape (T-3, T-11, T-15), cross-language parity (T-1, T-2), errors (T-57, T-58, T-62), timeout constants (T-47), enums/named types (T-15), docstrings (T-69/T-71).
4 violations found, all in the newly extracted helpers (the de-duplication itself is a good change — REQUEST_TIMEOUT_MS moving into connectionConfig is exactly T-47, and _file_operation_url / fileOperationUrl keep JS and Python mirrors in step per T-1):
- 2× bare
Errorin the new response helpers instead of the domain error class (T-57/T-58, message also fails T-62). The lines are carried over from the call sites being collapsed, but centralizing them is the cheap moment to fix them once instead of five times. - Python helper re-inlines
Literal["read", "write"]instead of the existingOperationalias (T-11). - Chained optional positionals at the new helper call sites (T-3).
Nothing line-specific for the CLI changes: languageChoices is CLI surface, outside the SDK design rules, and the extraction preserves behavior. FetchResponse<any, any, any> in the new helpers matches the existing handleApiError signature, so it is consistent, not a new violation.
| } | ||
|
|
||
| if (!res.data) { | ||
| throw new Error('Response data is missing') |
There was a problem hiding this comment.
T-57/T-58 — one base class per domain, and prefer the specific error over a generic one: secret failures must be SecretError (already imported here), never a bare Error, or a caller's catch (e) { if (e instanceof SecretError) } misses this path entirely. T-62 also wants the message to say what to do, not just what failed.
| throw new Error('Response data is missing') | |
| throw new SecretError( | |
| 'The API returned a success status with an empty body. Retry the request, and contact support@e2b.dev if it keeps happening.' | |
| ) |
| throwOnVolumePathError(res, path) | ||
|
|
||
| if (!res.data) { | ||
| throw new Error('Response data is missing') |
There was a problem hiding this comment.
T-57/T-58 — same as in secret.ts: volume errors extend VolumeError, so the empty-body case should throw VolumeError rather than a bare Error that escapes every domain-scoped catch. T-62: name the recovery in the message.
| throw new Error('Response data is missing') | |
| throw new VolumeError( | |
| 'The API returned a success status with an empty body. Retry the request, and contact support@e2b.dev if it keeps happening.' | |
| ) |
| def _file_operation_url( | ||
| self, | ||
| path: str, | ||
| operation: Literal["read", "write"], |
There was a problem hiding this comment.
T-11 — when a concept already has a named type, use it instead of the primitive/literal it aliases. Operation = Literal["read", "write"] already exists in e2b/sandbox/signature.py and is what get_signature (called a few lines below) takes; re-inlining the literal here creates a second place the set is written down, which is exactly what the alias exists to prevent.
Add Operation to the existing from .signature import get_signature import; Literal then drops out of the typing import added at the top of the file.
| operation: Literal["read", "write"], | |
| operation: Operation, |
|
|
||
| :return: URL for downloading file | ||
| """ | ||
| return self._file_operation_url(path, "read", user, use_signature_expiration) |
There was a problem hiding this comment.
T-3 — the golden rule applies from the very first optional parameter: path and operation are required and positional, but user / use_signature_expiration are optional and belong in kwargs (the Python spelling of the trailing options object). As written the call site is a chain of optional positionals whose meaning is unreadable without jumping to the definition, and inserting a parameter later silently reorders callers. Same on the upload_url call below.
| return self._file_operation_url(path, "read", user, use_signature_expiration) | |
| return self._file_operation_url( | |
| path, | |
| "read", | |
| user=user, | |
| use_signature_expiration=use_signature_expiration, | |
| ) |
| */ | ||
| private async fileOperationUrl( | ||
| path: string, | ||
| operation: 'read' | 'write', |
There was a problem hiding this comment.
T-1/T-11 — the Python mirror of this helper takes the named Operation alias from signature.py, so the JS side should have the same named type rather than a third inline copy of 'read' | 'write' (the other copy is in signature.ts). Export export type Operation = 'read' | 'write' from src/sandbox/signature.ts, use it in getSignature, and reference it here so both SDKs write the vocabulary down once.
There was a problem hiding this comment.
Looks good, straightforward de-duplication refactor. I compared each extracted helper (JS fileOperationUrl, Python _file_operation_url, secretInfoFromResponse, throwOnVolumePathError/volumeEntryStatFromResponse, CLI languageChoices) line-by-line against the original inline code they replace, and the logic is preserved exactly (including the path ?? '' default only applying to upload, and optional-chaining on opts being equivalent to the prior opts = opts ?? {}). Confirmed REQUEST_TIMEOUT_MS moved to connectionConfig has the identical value (60_000) and that asBuildLogs/VolumeApiPaths have zero remaining references anywhere in the repo. Python's SandboxBase is the single shared sync/async base, so no separate async change was needed.
Extended reasoning...
Overview
This PR is a pure refactor/dead-code-removal pass across the JS SDK, Python SDK, and CLI: it extracts previously duplicated logic into shared helpers (Sandbox.fileOperationUrl / Python SandboxBase._file_operation_url, secretInfoFromResponse, throwOnVolumePathError/volumeEntryStatFromResponse, CLI languageChoices), removes an unused CLI helper (asBuildLogs) and an unused type alias (VolumeApiPaths), and de-duplicates a REQUEST_TIMEOUT_MS constant by importing it from connectionConfig instead of redefining it locally.
Security risks
None identified. No new external input handling, no auth/crypto logic changes — the signing (getSignature) and error-handling code paths are moved verbatim into shared helpers, not altered in behavior.
Level of scrutiny
I read every changed file's diff in full and compared old vs. new logic side-by-side for each extraction (sandbox URL signing, secret response handling, volume error/stat handling, CLI language choices, and the REQUEST_TIMEOUT_MS/VolumeApiPaths cleanups). I also grepped the repo to confirm the removed asBuildLogs and VolumeApiPaths symbols have zero remaining references, and confirmed the REQUEST_TIMEOUT_MS constant value is identical in both locations. This is a mechanical, low-risk refactor with no behavior change, matching the PR's own description, so a lighter level of scrutiny than a feature PR is appropriate — but I did verify each extraction line-by-line rather than trusting the description.
Other factors
A new regression test (urls.test.ts) covers the one behavior detail worth double-checking (upload URL with no path, for both unsecured and signed sandboxes), and it passes trivially given the code inspected. The Python side reuses the single shared SandboxBase for both sync and async, so CLAUDE.md's "apply equivalent changes to JS and both sync/async Python" requirement is satisfied without needing a second file. No changeset was added, which is consistent with CLAUDE.md's requirement only applying when public surface changes — here the removed/renamed items are private/unexported and behavior is preserved, so this looks correctly scoped.
approve
|
putting this in drafts right now - will be revisiting it later. |
Summary
Dead-code / duplication audit of the hand-written SDK sources (
packages/js-sdk/src,packages/python-sdk/e2b,packages/cli/src; generated clients, fixtures and build output excluded). Behavior-preserving: every candidate was grepped across the whole monorepo (source, tests, examples, docs) before touching it, and nothing exported from a package entry point was removed.Removed (verified unreferenced monorepo-wide)
asBuildLogs()inpackages/cli/src/utils/format.ts— zero references anywhere.VolumeApiPathstype alias inpackages/js-sdk/src/volume/client.ts— zero references; it is not re-exported fromvolume/index.tsor the package entry point, so it is not part of the public surface.REQUEST_TIMEOUT_MS = 60_000duplicate involume/client.ts— now imports the same constant fromconnectionConfig(identical value).De-duplicated
Sandbox.uploadUrl()/Sandbox.downloadUrl()(JS) were byte-for-byte identical except the'write'/'read'operation and upload'spath ?? ''default. Both now delegate to one privatefileOperationUrl(path, operation, opts):Same change applied to the Python SDK in
e2b/sandbox/main.pyviaSandboxBase._file_operation_url(path, operation, ...). It lives on the shared base, so sync and async both pick it up.packages/js-sdk/src/volume/index.ts: the repeated404 -> VolumePathNotFoundError+handleApiError+!res.data+convertVolumeEntryStatblocks collapse intothrowOnVolumePathError(res, path)andvolumeEntryStatFromResponse(res, path), used bylist,makeDir,getInfo,updateMetadata,writeFile,readFileandremove. The streamedreadFilepath keeps its own inline handling because it must cancel the unconsumed body and runcleanup()before throwing.packages/js-sdk/src/secret.ts:create,updateandgetInfoshared an identical error/empty-body/convert tail, nowsecretInfoFromResponse(res). The per-method 404 handling (SecretNotFoundErrorvs.exists()returningfalse) stays at the call sites since it differs.CLI:
initandmigrateeach hard-coded the same three-entry languageselectlist; both now uselanguageChoicesfromtemplate/generators/types.ts(prompt messages stay per-command).Public-API candidates deliberately left in place
validateApiKey,apiErrorFromCode,handleApiError,components,paths(js-sdk/src/api) — exported from the entry point; removal would be breaking even where internal usage is thin.VolumeMetadataOptions,VolumeWriteOptions) — explicitly kept for backwards compatibility.Column<T>inpackages/cli/src/utils/table.ts— only used inrenderTable's own signature, but it is the documented shape callers construct.ConnectionConfig.set_integration()(Python) andSupportsApiErrorResponse— flagged by static analysis but genuinely used (CLI/tests, andapi_exception_from_responserespectively).No changeset: no public surface changed (the two removals are not entry-point exports and all other changes are internal refactors).
Verification
pnpm run format,pnpm run lintandpnpm run typecheckclean across all three packages. CLI suite: 116 passed. JS SDK offline suites covering the touched code (sandbox/urls,sandbox/files/signing,sandbox/secure,utils, mockedvolume/file,secret): passed. Pythontests/test_sandbox_urls.py: passed. Added a regression test assertinguploadUrl()with no path still omits thepathquery param and signs the empty path, since that default now flows through the shared helper.Link to Devin session: https://app.devin.ai/sessions/c4b115a6dec24b8aad92fbd5d24c7610