Skip to content

chore(sdk): de-duplicate file URL signing and stream start handling, drop unused exports - #1873

Draft
devin-ai-integration[bot] wants to merge 4 commits into
mainfrom
devin/1789369459-sdk-dead-code
Draft

devin-ai-integration[bot] wants to merge 4 commits into
mainfrom
devin/1789369459-sdk-dead-code

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

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

  • JS Sandbox.uploadUrl / downloadUrl were two ~40-line copies differing only in the operation literal and the path ?? '' default. Both now delegate to a private signedFileUrl(path, 'read' | 'write', opts).
  • Python SandboxBase.upload_url / download_url had the same duplication; both now call _signed_file_url(path, operation, user, use_signature_expiration) (shared by sync and async sandboxes via SandboxBase).
  • JS envd/api.ts: handleProcessStartEvent and handleWatchDirStartEvent duplicated the "pull first stream event, map Code.UnavailableSandboxNotFoundError" block; extracted into a generic readFirstEvent<T>(events).
  • JS Sandbox.envdPort hard-coded 49983 a second time; it now reads ConnectionConfig.envdPort (mirrors Python, which already uses ConnectionConfig.envd_port).
  • Python Filesystem.__init__ (sync + async): self._envd_api_url was assigned and never read anywhere; dropped.
  • CLI: removed the export keyword 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 via Sandbox; still in docs.
  • Sandbox.getMcpUrl / getMcpToken, TemplateBase.toJSON, LogEntry.toString (JS) and Sandbox.get_mcp_url (Py): public instance methods, no internal callers by design.
  • Option/registry types flagged as "unused exports" by knip (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 though index.ts doesn't re-export them by name.
  • DockerfFileFinalParserInterface (Py): looks like an empty protocol, but it is the return type of DockerfileParserInterface.set_start_cmd (mirrors the JS TemplateFinal marker) and appears in the docs.
  • Commands.kill / Pty.kill (JS) are byte-identical sendSignal(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-sdk ty, cli) all pass. Ran the affected integration tests against E2B: js-sdk urls, secure, files/signing, files/watch, commands/connect (25 passed); python-sdk test_sandbox_urls, sync/async files/test_secured, test_connect (28 passed); full @e2b/cli suite (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

…drop unused exports

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@cla-bot cla-bot Bot added the cla-signed label Sep 14, 2026
@changeset-bot

changeset-bot Bot commented Sep 14, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 1cd53fd

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
e2b Patch
@e2b/python-sdk Patch
@e2b/cli Patch

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

@github-actions

github-actions Bot commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Package Artifacts

Built from a8508ba. Download artifacts from this workflow run.

JS SDK (e2b@2.49.2-devin-1789369459-sdk-dead-code.0):

npm install ./e2b-2.49.2-devin-1789369459-sdk-dead-code.0.tgz

CLI (@e2b/cli@2.19.1-devin-1789369459-sdk-dead-code.0):

npm install ./e2b-cli-2.19.1-devin-1789369459-sdk-dead-code.0.tgz

Code Interpreter JS SDK (@e2b/code-interpreter@2.8.1-devin-1789369459-sdk-dead-code.0):

npm install ./e2b-code-interpreter-2.8.1-devin-1789369459-sdk-dead-code.0.tgz

Desktop JS SDK (@e2b/desktop@2.4.1-devin-1789369459-sdk-dead-code.0):

npm install ./e2b-desktop-2.4.1-devin-1789369459-sdk-dead-code.0.tgz

Python SDK (e2b==2.49.1+devin.1789369459.sdk.dead.code):

pip install ./e2b-2.49.1+devin.1789369459.sdk.dead.code-py3-none-any.whl

Code Interpreter Python SDK (e2b-code-interpreter==2.10.0+devin.1789369459.sdk.dead.code):

pip install ./e2b_code_interpreter-2.10.0+devin.1789369459.sdk.dead.code-py3-none-any.whl

Desktop Python SDK (e2b-desktop==2.5.0+devin.1789369459.sdk.dead.code):

pip install ./e2b_desktop-2.5.0+devin.1789369459.sdk.dead.code-py3-none-any.whl

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 signedFileUrl re-spells 'read' | 'write' inline where the Python side already uses the named Operation alias.
  • T-3a: Python _signed_file_url chains 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',

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment on lines +170 to +171
operation: Operation,
user: Optional[str],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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):

Suggested change
operation: Operation,
user: Optional[str],
operation: Operation,
*,
user: Optional[str],

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 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 = {}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge 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 👍 / 👎.

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 UnavailableSandboxNotFoundError 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>
@mishushakov
mishushakov marked this pull request as draft September 14, 2026 16:11

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

0 participants