chore(sdk): de-duplicate file URL signing and stream start handling, drop unused exports - #1873
devin-ai-integration[bot] wants to merge 4 commits into
Conversation
…drop unused exports 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:
|
🦋 Changeset detectedLatest commit: 1cd53fd 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 |
Package ArtifactsBuilt from a8508ba. Download artifacts from this workflow run. JS SDK ( npm install ./e2b-2.49.2-devin-1789369459-sdk-dead-code.0.tgzCLI ( npm install ./e2b-cli-2.19.1-devin-1789369459-sdk-dead-code.0.tgzCode Interpreter JS SDK ( npm install ./e2b-code-interpreter-2.8.1-devin-1789369459-sdk-dead-code.0.tgzDesktop JS SDK ( npm install ./e2b-desktop-2.4.1-devin-1789369459-sdk-dead-code.0.tgzPython SDK ( pip install ./e2b-2.49.1+devin.1789369459.sdk.dead.code-py3-none-any.whlCode Interpreter Python SDK ( pip install ./e2b_code_interpreter-2.10.0+devin.1789369459.sdk.dead.code-py3-none-any.whlDesktop Python SDK ( pip install ./e2b_desktop-2.5.0+devin.1789369459.sdk.dead.code-py3-none-any.whl |
There was a problem hiding this comment.
TASTE.md review of #1873 (checked: T-1/T-2 parity, T-3/T-3a signatures, T-11 named types, T-15 enums, T-47 named constants, T-54 exports, T-55 envd gating, T-59/T-60 error mapping, T-69 docs).
The refactor is behaviour-preserving and the public surface (uploadUrl/downloadUrl, upload_url/download_url, their SandboxUrlOpts/kwargs and JSDoc/docstrings) is untouched, so no public-API violations. 2 minor internal findings, both inline:
- T-11: JS
signedFileUrlre-spells'read' | 'write'inline where the Python side already uses the namedOperationalias. - T-3a: Python
_signed_file_urlchains two optionals positionally (private, so low priority).
Not on a changed line (so noted here only): protected readonly mcpPort = 50005 sits directly under the now-ConnectionConfig.envdPort line — since this PR moved envdPort to a named constant, mcpPort could get the same treatment for T-47 consistency. readFirstEvent returns .value without checking done, but that is inherited from the two functions it replaces, not introduced here.
The CLI export removals are outside TASTE.md's scope (packages/js-sdk/packages/python-sdk); I verified none of the removed exports are imported elsewhere in the repo.
|
|
||
| private async signedFileUrl( | ||
| path: string, | ||
| operation: '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. The Python half of this refactor imports the named Operation alias from e2b/sandbox/signature.py; the JS half re-spells 'read' | 'write' inline (and signature.ts does too in SignatureOpts). Export a SignatureOperation = 'read' | 'write' type from sandbox/signature.ts, use it in SignatureOpts.operation, and reference it here so the set of operations has one home in each SDK (T-1 parity with Operation).
| operation: Operation, | ||
| user: Optional[str], |
There was a problem hiding this comment.
T-3a (minor, private helper) — optionals are keyword-only, separated from the required positionals by a bare *. user and use_signature_expiration are the optional half of download_url/upload_url's surface, but here they are bound positionally so the call sites read _signed_file_url(path, "read", user, use_signature_expiration) and adding a third option becomes an ordering hazard. Since this is new code it can get the * from day one (call sites then pass user=user, use_signature_expiration=use_signature_expiration):
| operation: Operation, | |
| user: Optional[str], | |
| operation: Operation, | |
| *, | |
| user: Optional[str], |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42ea22caec
ℹ️ 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".
| private async signedFileUrl( | ||
| path: string, | ||
| operation: 'read' | 'write', | ||
| opts: SandboxUrlOpts = {} |
There was a problem hiding this comment.
Preserve null URL option normalization
When plain JavaScript callers pass null as the second argument to uploadUrl or downloadUrl, both methods previously normalized it with opts = opts ?? {}. A default parameter only replaces undefined, so the shared helper now dereferences null at opts.useSignatureExpiration and throws a TypeError instead of returning a URL. Normalize nullish options in the helper and add regression coverage for both methods.
AGENTS.md reference: AGENTS.md:L6-L6
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Looks good — this is a mechanical de-duplication/dead-export cleanup with no behavior change. Verified: JS uploadUrl/downloadUrl now delegate to signedFileUrl with the correct 'write'/'read' operation per call site; envdPort now reads ConnectionConfig.envdPort (still 49983); readFirstEvent preserves the original Unavailable → SandboxNotFoundError mapping for both call sites; Python's _signed_file_url threads operation through correctly for both upload_url/download_url; the dropped self._envd_api_url attribute was unused (the local envd_api_url param is still used). Grepped the CLI package for all de-exported symbols (connectSandbox, listSandboxes, listSandboxLogs, transformTemplateData, apiKey, configName, etc.) and found no external imports, only in-module use.
Extended reasoning...
Overview
This PR is a pure refactor/dead-code-removal sweep across js-sdk, python-sdk, and the CLI. It collapses duplicated logic (uploadUrl/downloadUrl in JS and Python into shared private helpers, and the "read first gRPC stream event + map Unavailable" logic in envd/api.ts into readFirstEvent), replaces a hard-coded envd port literal with the existing ConnectionConfig.envdPort constant, drops an unused Python filesystem attribute, and strips export from CLI symbols that are only used within their defining module.
Security risks
None identified. No new external inputs, no auth/crypto logic changes — the signature-generation and URL-building code paths are byte-for-byte preserved, just restructured to avoid duplication. The operation parameter ('read'/'write') is correctly threaded through to match the original call sites in both JS and Python.
Level of scrutiny
I applied moderate-to-high scrutiny given this touches SDK sandbox file-signing code and CLI export surface across many files, but the diffs are small, mechanical, and easy to diff against the pre-refactor logic line by line, which I did for the highest-risk pieces (signedFileUrl/_signed_file_url, readFirstEvent, envdPort).
Other factors
I independently grepped the CLI codebase for every de-exported symbol (connectSandbox, listSandboxes, listSandboxLogs, transformTemplateData, apiKey, projectId, configName, configSchema, UserIdentity/UserOAuth/UserTokens, DASHBOARD_BASE, primaryColor, asRed, borderStyle, isTokenExpired, TokenRefreshError, refreshOAuthToken, Column) and confirmed none are imported from outside their defining file, matching the PR's claim. The changeset file correctly bumps e2b, @ e2b/python-sdk, and @ e2b/cli as patch versions. I was unable to execute pnpm run typecheck/lint/test in this sandboxed environment (Bash tool denied non-git commands), but the manual diff review gives high confidence given the mechanical nature of the change and the PR's own verification claims (unit/integration test runs, lint/typecheck pass).
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
Summary
Scheduled dead/duplicated-code sweep over
packages/js-sdk/src,packages/python-sdk/e2b,packages/cli/src(generated clients excluded). Public behaviour is unchanged; no exported symbol was removed.Removed / de-duplicated
Sandbox.uploadUrl/downloadUrlwere two ~40-line copies differing only in theoperationliteral and thepath ?? ''default. Both now delegate to a privatesignedFileUrl(path, 'read' | 'write', opts).SandboxBase.upload_url/download_urlhad the same duplication; both now call_signed_file_url(path, operation, user, use_signature_expiration)(shared by sync and async sandboxes viaSandboxBase).envd/api.ts:handleProcessStartEventandhandleWatchDirStartEventduplicated the "pull first stream event, mapCode.Unavailable→SandboxNotFoundError" block; extracted into a genericreadFirstEvent<T>(events).Sandbox.envdPorthard-coded49983a second time; it now readsConnectionConfig.envdPort(mirrors Python, which already usesConnectionConfig.envd_port).Filesystem.__init__(sync + async):self._envd_api_urlwas assigned and never read anywhere; dropped.exportkeyword from module-private symbols that nothing imports (verified with knip + grep across src/tests):apiKey,projectId,connectSandbox,listSandboxes,listSandboxLogs,transformTemplateData,configName,configSchema,DASHBOARD_BASE,primaryColor,asRed,borderStyle,isTokenExpired,TokenRefreshError,refreshOAuthToken,UserIdentity,UserOAuth,UserTokens,Column.Public-API candidates deliberately left in place
ConnectionConfig.envdPort(JS) /ConnectionConfig.envd_port(Py): public static on an exported class — only referenced internally, but removing it is breaking. Used as the single source of truth instead.SandboxApi.getFullInfo(JS): deprecated but exported viaSandbox; still in docs.Sandbox.getMcpUrl/getMcpToken,TemplateBase.toJSON,LogEntry.toString(JS) andSandbox.get_mcp_url(Py): public instance methods, no internal callers by design.PtyCreateOpts,PtyConnectOpts,FilesystemRequestOpts,FilesystemListOpts,WatchOpts,SandboxUrlOpts,McpServer,BasicBuildOptions,GenericDockerRegistry,AWSRegistry,GCPRegistry): they appear in public method signatures and in the generated SDK reference, so they are part of the public type surface even thoughindex.tsdoesn't re-export them by name.DockerfFileFinalParserInterface(Py): looks like an empty protocol, but it is the return type ofDockerfileParserInterface.set_start_cmd(mirrors the JSTemplateFinalmarker) and appears in the docs.Commands.kill/Pty.kill(JS) are byte-identicalsendSignal(SIGKILL)wrappers on two separate public classes; merging them would need a shared free function across classes and was left out to keep this PR mechanical.Verification
pnpm run format,pnpm run lint,pnpm run typecheck(js-sdk, python-sdkty, cli) all pass. Ran the affected integration tests against E2B: js-sdkurls,secure,files/signing,files/watch,commands/connect(25 passed); python-sdktest_sandbox_urls, sync/asyncfiles/test_secured,test_connect(28 passed); full@e2b/clisuite (128 passed).Link to Devin session: https://app.devin.ai/sessions/9ef3322918044538a155b99fbc8bc4e4
Open in Devin Desktop: https://app.devin.ai/desktop/session/9ef3322918044538a155b99fbc8bc4e4?variant=devin