Skip to content

fix(e2b): support e2b api url and CubeSandbox#1765

Open
chengjoey wants to merge 1 commit into
trpc-group:mainfrom
chengjoey:fix/e2b-apiurl
Open

fix(e2b): support e2b api url and CubeSandbox#1765
chengjoey wants to merge 1 commit into
trpc-group:mainfrom
chengjoey:fix/e2b-apiurl

Conversation

@chengjoey
Copy link
Copy Markdown
Contributor

run e2b with cubesandbox:
image

cubesandbox log:
image

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 8, 2026

Review Change Stack

📝 Walkthrough

Change overview

This PR adds explicit E2B management API URL support and CubeSandbox-friendly behavior to the e2b executor, plus TLS/HTTP-client improvements and test coverage:

  • New executor option WithAPIURL(apiURL string) and CodeExecutor.apiURL field; API URL is propagated into SandboxOpts so sandbox create/connect use a provided management API base URL instead of deriving one from domain settings.
  • ConnectionConfig.APIURL (with E2B_API_URL fallback) and SandboxOpts.APIURL: APIBase() returns a trimmed APIURL when provided; otherwise it falls back to E2B_API_URL or the legacy scheme+domain construction.
  • SandboxInfo.Domain and cached Sandbox.sandboxDomain (mutex-protected): sandbox create/connect/GetInfo populate or refresh the resolved domain; host/Jupyter URL generation prefers sandboxDomain and generates hostnames as .<sandbox_domain> (omitting clientID) when available, with legacy fallback when empty.
  • Default HTTP client initialization when none supplied: builds an *http.Client with a cloned Transport and TLSClientConfig whose RootCAs are extended from SSL_CERT_FILE and/or SSL_CERT_DIR (falls back to system pool when available). Existing custom HTTPClient values are not modified.
  • Tests added for APIURL precedence/normalization, default HTTP client / RootCAs loading, host & jupyter URL formatting, concurrency, and TLS helper behaviors. README example updated with CubeSandbox run instructions and env var notes.

Compatibility and behavioral risks

  • Hostname format change: exposed service hostnames shift from -. to .<sandbox_domain> when the API returns Domain. Any automation, DNS rules, ingress, firewall rules, or monitoring that constructs or expects the old hostname format may break.
  • API URL precedence: WithAPIURL explicitly overrides E2B_API_URL and legacy domain-derived URLs. A misconfigured WithAPIURL or E2B_API_URL may redirect all sandbox operations to an unintended management endpoint.
  • Dependency on SandboxInfo.Domain: if the E2B API omits Domain, returns stale values, or if Domain changes after creation, URL resolution may be incorrect until Create/Connect/GetInfo refreshes the cached sandboxDomain; this may cause temporary routing failures or require additional fetches.
  • Global TLS impact for internally-created clients: SSL_CERT_FILE/SSL_CERT_DIR affect any default HTTP client the code creates. Callers that previously relied on system trusts may see different trust behavior; callers providing their own HTTPClient are unaffected, which can create inconsistent behavior across call sites.
  • Behavioral divergence: callers that relied on implicit URL building or previous hostname composition might observe different endpoints; CI/CD scripts, automated tooling, or tests that hardcode old hostnames must be updated.

Recommended validation steps

  1. API URL behavior

    • Create CodeExecutor with WithAPIURL("https://example/api/") and verify APIBase() and sandbox create/connect use the trimmed URL (no duplicate/trailing slashes).
    • Set E2B_API_URL env var and confirm WithAPIURL overrides it; test behavior when WithAPIURL is empty so env var is used.
  2. Sandbox host, domain and relocation

    • Create a sandbox against a CubeSandbox endpoint that returns SandboxInfo.Domain; confirm GetHost/GetJupyter produce .<sandbox_domain> and that port mappings resolve.
    • Simulate or mock domain changes (relocation) and verify Create/Connect/GetInfo refresh cached sandboxDomain and URLs update correctly.
    • Verify legacy fallback behavior when Domain is absent: ensure predictable composition and document differences.
  3. TLS and HTTP client handling

    • No custom client: unset HTTPClient and set SSL_CERT_FILE to a test CA PEM; confirm the default transport’s TLSClientConfig.RootCAs includes the CA (tests mirror this).
    • SSL_CERT_DIR: populate with multiple certs and confirm they are loaded into RootCAs.
    • Custom client: supply an http.Client in ConnectionConfig and confirm init() leaves it unchanged.
  4. End-to-end CubeSandbox checks

    • Run the updated example/demo with -executor e2b, E2B_API_URL pointing at CubeSandbox, perform create/connect, run commands, check artifact download URLs, disk/volume behavior and container logs (compare to provided PR logs).
    • Validate exposed services (Jupyter, custom ports) are reachable via constructed URLs through any configured ingress/load-balancer.
  5. Backwards compatibility & operational audits

    • Confirm callers that rely on domain-based URL construction still work when neither WithAPIURL nor E2B_API_URL are set.
    • Audit and update automation, firewall/ingress rules, DNS, monitoring, and tests that hardcode or derive the old - hostnames.
    • Run the new unit tests and the repo’s test suite to ensure no regressions.

Priority fixes if validations fail

  • Restore a compatibility shim for hostname composition or require API to always supply SandboxInfo.Domain.
  • Correct configured APIURL in CI/prod (or fall back to E2B_API_URL) to avoid pointing to wrong management endpoints.
  • Document SSL_CERT_FILE/SSL_CERT_DIR expectations and recommend supplying custom HTTPClient where per-call certificate control is required.
中文

变更概览

此 PR 为 e2b 执行器引入显式的 E2B 管理 API URL 支持并增强对 CubeSandbox 的兼容性,同时改进了 TLS/HTTP 客户端初始化并增加了测试覆盖:

  • 新增执行器选项 WithAPIURL(apiURL string) 并在 CodeExecutor 中保存 apiURL;该值传入 SandboxOpts,使 sandbox create/connect 使用显式管理 API 基础 URL 而不是从 domain 派生。
  • ConnectionConfig.APIURL(优先于 E2B_API_URL 环境变量)与 SandboxOpts.APIURL:当传入时 APIBase() 返回已修剪的 APIURL;否则回退到 E2B_API_URL,再回退到旧的 scheme+domain 构造。
  • SandboxInfo.Domain 与带互斥保护的缓存字段 Sandbox.sandboxDomain:Create/Connect/GetInfo 会填充或刷新解析到的域名;URL 生成优先使用 sandboxDomain,主机名在可用时生成为 .<sandbox_domain>(不含 clientID),domain 为空时保留旧回退逻辑。
  • 默认 HTTP 客户端:当未提供 HTTPClient 时,构造 *http.Client,克隆 Transport 并设置 TLSClientConfig,其 RootCAs 会从 SSL_CERT_FILE/SSL_CERT_DIR 扩展(在可用时回退到系统池)。若用户提供了自定义 HTTPClient,则不做修改。
  • 新增单元测试覆盖 APIURL 优先级/规范化、默认 HTTP 客户端与 RootCAs 加载、host/jupyter URL 格式、并发场景与 TLS 辅助函数。README 示例更新,包含 CubeSandbox 运行提示与所需环境变量说明。

兼容性与行为风险

  • 主机名格式变更:当 API 返回 Domain 时,暴露服务主机名由 -. 变为 .<sandbox_domain>。任何构造或依赖旧格式的自动化、DNS、ingress、防火墙或监控规则可能失效。
  • API URL 优先级:WithAPIURL 显式优先于 E2B_API_URL 与基于 domain 的构造。错误配置的 APIURL 会导致所有 sandbox 操作指向错误的管理端点。
  • 依赖 SandboxInfo.Domain:如果 E2B API 未返回 Domain、返回过时值或域在创建后变更,直到 Create/Connect/GetInfo 刷新缓存前,URL 解析可能不正确,从而产生路由问题。
  • 全局 TLS 影响:SSL_CERT_FILE/SSL_CERT_DIR 会影响所有内部创建的默认 HTTP 客户端。依赖系统信任池的调用可能观察到不同的信任行为;提供自定义 HTTPClient 的调用不受影响,可能导致行为不一致。
  • 行为分歧:依赖旧有隐式 URL 构造的调用可能访问不同端点;需要更新 CI/CD 脚本、自动化和测试中的硬编码主机名。

推荐验证步骤

1)API URL 行为

  • 使用 WithAPIURL("https://example/api/") 创建 CodeExecutor,验证 APIBase() 与 sandbox create/connect 使用已修剪的 URL(无重复/尾部斜杠)。
  • 设置 E2B_API_URL 环境变量并确认 WithAPIURL 覆盖它;当 WithAPIURL 为空时验证 env var 生效。

2)Sandbox 域与迁移行为

  • 在返回 SandboxInfo.Domain 的 CubeSandbox 上创建 sandbox,确认 GetHost/GetJupyter 生成 .<sandbox_domain> 并且端口映射可访问。
  • 模拟或 mock 域变更(例如 sandbox 迁移),验证 Create/Connect/GetInfo 刷新缓存后 URL 能正确更新。
  • 验证 Domain 缺失时的回退逻辑并确保行为可预测。

3)TLS 与 HTTP 客户端处理

  • 无自定义客户端场景:不提供 HTTPClient, 设置 SSL_CERT_FILE 为测试 CA PEM,确认默认 transport 的 TLSClientConfig.RootCAs 包含该 CA(单元测试已覆盖此检查)。
  • SSL_CERT_DIR 场景:放置多个证书并确认它们已加载到 RootCAs 中。
  • 自定义客户端:在 ConnectionConfig 中提供 http.Client,确认 init() 不会替换或修改该客户端。

4)端到端 CubeSandbox 检查

  • 使用 -executor e2b 并将 E2B_API_URL 指向 CubeSandbox,执行 create/connect、运行命令,检查构建/下载 artifact 的 URL、磁盘/卷行为空及容器日志(对比 PR 附图日志)。
  • 验证通过构造的 URL 能够访问暴露的服务(Jupyter 与自定义端口),以及端口映射是否按预期解析。

5)向后兼容与运维检查

  • 在既无 WithAPIURL 也无 E2B_API_URL 情况下确认基于 domain 的默认 URL 构造继续有效。
  • 审计并更新自动化、firewall/ingress、DNS、监控和测试中硬编码或基于旧格式的主机名生成逻辑。
  • 运行新增单元测试与代码库测试套件,确保无回归。

若验证失败应优先处理

  • 添加主机名兼容层或要求 API 始终返回 SandboxInfo.Domain 以维持新行为可靠性。
  • 修正 CI/生产环境中的 APIURL 配置(或回退至 env var)以避免指向错误管理端点。
  • 明确 SSL_CERT_FILE/SSL_CERT_DIR 的期望并在需要按调用控制证书时建议使用自定义 http.Client。

Walkthrough

This PR adds an explicit APIURL override to the E2B executor and connection types, builds a TLS-aware default HTTP client that loads roots from SSL_CERT_FILE/SSL_CERT_DIR, switches sandbox hostname construction to prefer an API-resolved cached domain, and wires the APIURL through executor options and sandbox lifecycle; tests and README updates included.

Changes

E2B Sandbox API URL and TLS Configuration

Layer / File(s) Summary
Data Contracts
codeexecutor/e2b/internal/codeinterpreter/client.go, codeexecutor/e2b/internal/codeinterpreter/sandbox.go, codeexecutor/e2b/e2b.go
Adds ConnectionConfig.APIURL, SandboxOpts.APIURL, SandboxInfo.Domain, and Sandbox.sandboxDomain (with mutex).
API Base URL Resolution
codeexecutor/e2b/internal/codeinterpreter/client.go
ConnectionConfig.init() reads E2B_API_URL when APIURL is unset; APIBase() returns trimmed APIURL when present, else falls back to domain/scheme construction.
TLS and Default HTTP Client
codeexecutor/e2b/internal/codeinterpreter/client.go
Introduces newDefaultHTTPClient() and buildTLSConfigFromEnv() to create a TLS-aware default *http.Client that loads root CAs from SSL_CERT_FILE/SSL_CERT_DIR and attaches them to the transport.
Sandbox Domain and URL Construction
codeexecutor/e2b/internal/codeinterpreter/sandbox.go
Adds cached sandboxDomain and prefers it over connection.Domain for getHost()/jupyterURL, changing host composition to omit clientID when sandbox domain is resolved.
Sandbox Lifecycle Integration
codeexecutor/e2b/internal/codeinterpreter/sandbox.go
Create(), Connect(), and List() forward opts.APIURL into ConnectionConfig; Create/Connect cache API domain; GetInfo() refreshes cached sandboxDomain.
Executor Option Wiring
codeexecutor/e2b/e2b.go
Adds WithAPIURL(apiURL string) option and CodeExecutor.apiURL field; apiURL is passed into ci.SandboxOpts.APIURL during Create/Connect.
Test Support
codeexecutor/e2b/internal/codeinterpreter/testutil_test.go
Adds generateTestCAPEM() to emit self-signed ECDSA P-256 CA PEM for tests.
Test Coverage
codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go, .../client_test.go
Adds tests for APIBase() precedence and trimming, buildTLSConfigFromEnv() behaviors for file/dir/missing/invalid inputs, default HTTP client initialization, preservation of user-supplied HTTPClient, updated sandbox host/jupyter URL expectations, and a concurrency test.
Documentation
examples/tool/codeexec/README.md
Quick Start updated with CubeSandbox run example and env var notes (E2B_API_KEY, E2B_API_URL, SSL_CERT_FILE).

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes


  • Possibly related PRs

  • Suggested reviewers

    • sandyskies
    • WineChord
    • liuzengh

中文摘要(折叠): 此 PR 为 E2B 沙箱执行器添加可配置的 API URL 与 TLS 证书支持:在 ConnectionConfig/SandboxOpts 中添加 APIURL,支持从 E2B_API_URL 环境变量回退;构建可从 SSL_CERT_FILE/SSL_CERT_DIR 加载根证书的默认 HTTP 客户端;将沙箱主机名生成从 fullID 改为 API 返回的 domain 并缓存;通过 CodeExecutor 的 WithAPIURL 将配置传入 Create/Connect/List/GetInfo;新增测试用例与 README 示例。

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.30% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main changes: adding E2B API URL support and CubeSandbox integration.
Description check ✅ Passed The description relates to the changeset by showing execution results with CubeSandbox, demonstrating the feature works as intended.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Tip

💬 Introducing Slack Agent: The best way for teams to turn conversations into code.

Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.

  • Generate code and open pull requests
  • Plan features and break down work
  • Investigate incidents and troubleshoot customer tickets together
  • Automate recurring tasks and respond to alerts with triggers
  • Summarize progress and report instantly

Built for teams:

  • Shared memory across your entire org—no repeating context
  • Per-thread sandboxes to safely plan and execute work
  • Governance built-in—scoped access, auditability, and budget controls

One agent for your entire SDLC. Right inside Slack.

👉 Get started


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

// getHost returns the public host for a port exposed by the sandbox.
func (s *Sandbox) getHost(port int) string {
return fmt.Sprintf("%d-%s.%s", port, s.fullID(), s.connection.Domain)
return fmt.Sprintf("%d-%s.%s", port, s.id, s.sandboxHostDomain())
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

When domain is absent, this drops clientID from sandbox hostnames and changes existing GetHost and Jupyter URLs. Keep the old sandboxID-clientID fallback.

中文 当 `domain` 缺失时,这里会从 sandbox 主机名中丢掉 `clientID`,并改变现有 `GetHost` 和 Jupyter URL。请保留旧的 `sandboxID-clientID` 回退。

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.

done

// this keeps the jupyter/envd URLs correct even if the sandbox was
// relocated to a different host.
if info.Domain != "" {
s.sandboxDomain = info.Domain
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

GetInfo now writes sandboxDomain while Jupyter requests read it, so concurrent info refresh and execution races. Guard the cached domain or avoid mutating it.

中文 `GetInfo` 现在会在 Jupyter 请求读取 `sandboxDomain` 的同时写入它,因此并发刷新信息和执行代码会产生数据竞争。请为缓存域名加锁,或避免修改它。

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.

done

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 4

🧹 Nitpick comments (1)
codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go (1)

443-472: ⚡ Quick win

TestGetInfo_ThroughMockAPI does not exercise the new sandboxDomain refresh logic.

The mock response omits the domain field, so the if info.Domain != "" branch in GetInfo is never taken. The primary new behavior — that GetHost/jupyterURL uses the API-returned domain after a GetInfo call — is unverified.

🧪 Suggested test extension
 func TestGetInfo_ThroughMockAPI(t *testing.T) {
 	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		...
-		_, _ = w.Write([]byte(`{"sandboxID":"abc","clientID":"cid","state":"running","metadata":{"k":"v"}}`))
+		_, _ = w.Write([]byte(`{"sandboxID":"abc","clientID":"cid","state":"running","metadata":{"k":"v"},"domain":"custom.host.local"}`))
 	}))
 	...
 	info, err := sbx.GetInfo(context.Background())
 	...
+	// Verify that sandboxDomain is refreshed from the API response.
+	if sbx.sandboxDomain != "custom.host.local" {
+		t.Errorf("sandboxDomain not refreshed: %q", sbx.sandboxDomain)
+	}
+	if got := sbx.GetHost(3000); got != "3000-abc.custom.host.local" {
+		t.Errorf("GetHost after domain refresh: %q", got)
+	}
 }
中文

TestGetInfo_ThroughMockAPI 的 mock 响应中未包含 domain 字段,导致 GetInfo 中的 sandboxDomain 刷新分支从未被执行。GetInfo 返回 domain 后 GetHost/jupyterURL 使用新域名这一核心新行为缺乏测试覆盖。建议在 mock 响应中添加 "domain" 字段并断言 sandboxDomainGetHost 的结果。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go` around lines 443 -
472, Update the TestGetInfo_ThroughMockAPI test to include a "domain" field in
the mock JSON response so the sandboxDomain refresh branch in Sandbox.GetInfo is
exercised; after calling sbx.GetInfo assert that info.Domain (or
sbx.sandboxDomain) equals the returned domain and that sbx.GetHost (and/or
jupyterURL) now uses that domain (e.g., returns host/jupyter URL containing the
new domain) to verify the new behavior is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@codeexecutor/e2b/internal/codeinterpreter/client.go`:
- Around line 162-163: The TLS config returned currently only sets RootCAs which
leaves the minimum TLS version implicit; update the returned tls.Config (in the
function that currently returns &tls.Config{RootCAs: pool}) to explicitly set
MinVersion: tls.VersionTLS12 so the client enforces a TLS 1.2+ floor (i.e.,
return &tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12}).

In `@codeexecutor/e2b/internal/codeinterpreter/sandbox.go`:
- Around line 262-267: GetInfo currently writes s.sandboxDomain without
synchronization while sandboxHostDomain (used by jupyterURL → RunCode) reads it,
causing a data race; add a sync.RWMutex field (e.g., domainMu) to the Sandbox
struct and use domainMu.Lock()/Unlock() around writes in GetInfo and
domainMu.RLock()/RUnlock() around reads in sandboxHostDomain (and any other
readers like jupyterURL/RunCode) to ensure safe concurrent access to
sandboxDomain.

In `@examples/tool/codeexec/README.md`:
- Around line 52-59: Add the missing working-directory step to the CubeSandbox
snippet in the README so the commands run reproducibly: insert a `cd
tool/codeexec` step immediately before exporting E2B_ENV vars and running
`./codeexec-demo` in the CubeSandbox example block (the same placement used in
the other executor snippets) so users copying the Quick Start from `examples/`
land in the correct directory before invoking `codeexec-demo` with the `-model
deepseek-v4-flash -executor e2b` flags.
- Around line 55-57: Update the example env block in README.md to clearly mark
E2B_API_URL and SSL_CERT_FILE as optional overrides: change the comments or
variable descriptions around E2B_API_URL and SSL_CERT_FILE (while keeping
E2B_API_KEY shown as required) so the snippet communicates that E2B_API_URL and
SSL_CERT_FILE are optional overrides rather than mandatory configuration values
(reference the E2B_API_URL and SSL_CERT_FILE identifiers in the example and add
“(optional)” or similar wording next to them).

---

Nitpick comments:
In `@codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go`:
- Around line 443-472: Update the TestGetInfo_ThroughMockAPI test to include a
"domain" field in the mock JSON response so the sandboxDomain refresh branch in
Sandbox.GetInfo is exercised; after calling sbx.GetInfo assert that info.Domain
(or sbx.sandboxDomain) equals the returned domain and that sbx.GetHost (and/or
jupyterURL) now uses that domain (e.g., returns host/jupyter URL containing the
new domain) to verify the new behavior is covered.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 16922e9b-7b87-41e7-8464-3b2e95a23df5

📥 Commits

Reviewing files that changed from the base of the PR and between 432e09b and c16aad6.

📒 Files selected for processing (6)
  • codeexecutor/e2b/e2b.go
  • codeexecutor/e2b/internal/codeinterpreter/client.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go
  • codeexecutor/e2b/internal/codeinterpreter/testutil_test.go
  • examples/tool/codeexec/README.md

Comment thread codeexecutor/e2b/internal/codeinterpreter/client.go Outdated
Comment thread codeexecutor/e2b/internal/codeinterpreter/sandbox.go
Comment on lines +52 to +59
Run with CubeSandbox:

```bash
export E2B_API_KEY=dummy
export E2B_API_URL=http://<your-sandbox-server>:3000
export SSL_CERT_FILE=<caPATH>/rootCA.pem
./codeexec-demo -model deepseek-v4-flash -executor e2b
```
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Add the missing working-directory step for reproducible run commands.

The CubeSandbox snippet omits cd tool/codeexec, while the other executor snippets include it. Copy-pasting from the Quick Start flow can fail if the user is still in examples/.

Proposed doc patch
 Run with CubeSandbox:
 
 ```bash
+cd tool/codeexec
 export E2B_API_KEY=dummy
 export E2B_API_URL=http://<your-sandbox-server>:3000
 export SSL_CERT_FILE=<caPATH>/rootCA.pem
 ./codeexec-demo -model deepseek-v4-flash -executor e2b
</details>

<details><summary>中文</summary>
请补充工作目录切换步骤以保证可复现性。当前 CubeSandbox 片段缺少 `cd tool/codeexec`,而其他执行器都有;用户按 Quick Start 直接复制时,若仍在 `examples/` 目录可能执行失败。
</details>

As per coding guidelines, “Examples represent the recommended usage and the user's first impression. Review them for reproducibility, clarity, and safe execution,” and “Whether runtime steps, dependencies, environment variables, and side effects are clear.”

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion
Run with CubeSandbox:

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/tool/codeexec/README.md` around lines 52 - 59, Add the missing
working-directory step to the CubeSandbox snippet in the README so the commands
run reproducibly: insert a `cd tool/codeexec` step immediately before exporting
E2B_ENV vars and running `./codeexec-demo` in the CubeSandbox example block (the
same placement used in the other executor snippets) so users copying the Quick
Start from `examples/` land in the correct directory before invoking
`codeexec-demo` with the `-model deepseek-v4-flash -executor e2b` flags.

Comment on lines +55 to +57
export E2B_API_KEY=dummy
export E2B_API_URL=http://<your-sandbox-server>:3000
export SSL_CERT_FILE=<caPATH>/rootCA.pem
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Mark E2B_API_URL and SSL_CERT_FILE as optional in the example block.

The PR objective and code path indicate these are overrides, but the snippet currently makes them look mandatory. Please label them optional to match the public contract and reduce setup friction.

Proposed doc patch
 export E2B_API_KEY=dummy
-export E2B_API_URL=http://<your-sandbox-server>:3000
-export SSL_CERT_FILE=<caPATH>/rootCA.pem
+# Optional: override API endpoint (if not using the default E2B endpoint)
+export E2B_API_URL=http://<your-sandbox-server>:3000
+# Optional: custom CA bundle for self-hosted TLS
+export SSL_CERT_FILE=<caPATH>/rootCA.pem
 ./codeexec-demo -model deepseek-v4-flash -executor e2b
</details>

<details><summary>中文</summary>
建议在示例中明确 `E2B_API_URL` 和 `SSL_CERT_FILE` 为可选项。当前写法看起来像必填,会与实际“覆盖默认值”的语义不一致,也会增加上手门槛。
</details>

As per coding guidelines, “The goal of documentation is to accurately communicate the current public contract,” and “Whether commands, import paths, configuration keys, defaults, and return behavior match the current code.”

<!-- suggestion_start -->

<details>
<summary>📝 Committable suggestion</summary>

> ‼️ **IMPORTANT**
> Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

```suggestion
export E2B_API_KEY=dummy
# Optional: override API endpoint (if not using the default E2B endpoint)
export E2B_API_URL=http://<your-sandbox-server>:3000
# Optional: custom CA bundle for self-hosted TLS
export SSL_CERT_FILE=<caPATH>/rootCA.pem
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@examples/tool/codeexec/README.md` around lines 55 - 57, Update the example
env block in README.md to clearly mark E2B_API_URL and SSL_CERT_FILE as optional
overrides: change the comments or variable descriptions around E2B_API_URL and
SSL_CERT_FILE (while keeping E2B_API_KEY shown as required) so the snippet
communicates that E2B_API_URL and SSL_CERT_FILE are optional overrides rather
than mandatory configuration values (reference the E2B_API_URL and SSL_CERT_FILE
identifiers in the example and add “(optional)” or similar wording next to
them).

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (1)
codeexecutor/e2b/internal/codeinterpreter/client.go (1)

162-163: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Set explicit MinVersion: tls.VersionTLS12 on the returned tls.Config.

Static analysis (CWE-327) still flags this line, and the prior review thread marked the same finding as addressed in commit 495760e, but the current state shows &tls.Config{RootCAs: pool} without an explicit floor. For a framework-level default client, please make the policy explicit so it doesn't silently drift with future Go releases or config reuse.

🔒 Proposed fix
-	return &tls.Config{RootCAs: pool}
+	return &tls.Config{
+		RootCAs:    pool,
+		MinVersion: tls.VersionTLS12,
+	}
中文

静态分析(CWE-327)仍在此处告警;上一轮评审标记已在 495760e 中修复,但当前代码仍为 &tls.Config{RootCAs: pool},未显式设置最低 TLS 版本。框架级默认客户端建议显式指定 MinVersion: tls.VersionTLS12,避免随 Go 版本或配置复用产生隐式策略漂移。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@codeexecutor/e2b/internal/codeinterpreter/client.go` around lines 162 - 163,
The returned TLS configuration created at the return statement "return
&tls.Config{RootCAs: pool}" must explicitly set a minimum TLS version; update
the returned value to include MinVersion: tls.VersionTLS12 so the config becomes
&tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12} ensuring the
framework-level client enforces TLS1.2 as a floor and avoids future default
drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Duplicate comments:
In `@codeexecutor/e2b/internal/codeinterpreter/client.go`:
- Around line 162-163: The returned TLS configuration created at the return
statement "return &tls.Config{RootCAs: pool}" must explicitly set a minimum TLS
version; update the returned value to include MinVersion: tls.VersionTLS12 so
the config becomes &tls.Config{RootCAs: pool, MinVersion: tls.VersionTLS12}
ensuring the framework-level client enforces TLS1.2 as a floor and avoids future
default drift.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4eb5c0fc-9756-45e1-92ee-841e14a8722a

📥 Commits

Reviewing files that changed from the base of the PR and between c16aad6 and 495760e.

📒 Files selected for processing (6)
  • codeexecutor/e2b/e2b.go
  • codeexecutor/e2b/internal/codeinterpreter/client.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go
  • codeexecutor/e2b/internal/codeinterpreter/testutil_test.go
  • examples/tool/codeexec/README.md
✅ Files skipped from review due to trivial changes (1)
  • examples/tool/codeexec/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
  • codeexecutor/e2b/e2b.go
  • codeexecutor/e2b/internal/codeinterpreter/testutil_test.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox.go

@codecov
Copy link
Copy Markdown

codecov Bot commented May 8, 2026

Codecov Report

❌ Patch coverage is 83.51648% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.80029%. Comparing base (c0172c8) to head (f19ccb1).

Files with missing lines Patch % Lines
...odeexecutor/e2b/internal/codeinterpreter/client.go 82.00000% 5 Missing and 4 partials ⚠️
...deexecutor/e2b/internal/codeinterpreter/sandbox.go 89.47368% 4 Missing ⚠️
codeexecutor/e2b/e2b.go 33.33333% 2 Missing ⚠️

❌ Your patch check has failed because the patch coverage (83.51648%) is below the target coverage (85.00000%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@                 Coverage Diff                 @@
##                main       #1765         +/-   ##
===================================================
- Coverage   89.80970%   89.80029%   -0.00942%     
===================================================
  Files            935         935                 
  Lines         151291      151367         +76     
===================================================
+ Hits          135874      135928         +54     
- Misses          9703        9718         +15     
- Partials        5714        5721          +7     
Flag Coverage Δ
unittests 89.80029% <83.51648%> (-0.00942%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Signed-off-by: joey <zchengjoey@gmail.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
codeexecutor/e2b/internal/codeinterpreter/client_test.go (2)

150-161: ⚡ Quick win

Add RootCAs assertion for consistency.

Unlike TestBuildTLSConfigFromEnv_MissingCertFile (lines 145-147), this test doesn't verify the RootCAs field. For consistency and completeness, consider adding an assertion that RootCAs is non-nil, similar to the missing cert file test.

Suggested addition
 	if cfg.MinVersion != tls.VersionTLS12 {
 		t.Errorf("MinVersion: got %d, want TLS1.2", cfg.MinVersion)
 	}
+	if cfg.RootCAs == nil {
+		t.Error("RootCAs should fall back to the system pool, not be nil")
+	}
 }
中文

为保持一致性添加 RootCAs 断言。

TestBuildTLSConfigFromEnv_MissingCertFile(第 145-147 行)不同,此测试未验证 RootCAs 字段。为保持一致性和完整性,考虑添加断言验证 RootCAs 非空,类似于缺失证书文件测试。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@codeexecutor/e2b/internal/codeinterpreter/client_test.go` around lines 150 -
161, The test TestBuildTLSConfigFromEnv_MissingCertDir calls
buildTLSConfigFromEnv but doesn't assert RootCAs; add a check to ensure
cfg.RootCAs is non-nil (same style as TestBuildTLSConfigFromEnv_MissingCertFile)
so the test verifies that RootCAs is populated even when SSL_CERT_DIR is
missing.

134-148: ⚡ Quick win

Strengthen the system pool fallback verification.

The test expects the TLS config to use the system certificate pool when SSL_CERT_FILE is missing, but only asserts that RootCAs != nil. While this ensures graceful degradation, it doesn't distinguish between the system pool and an empty fallback pool created when x509.SystemCertPool() fails (line 122–124 in the implementation).

Consider verifying that the pool contains certificates—for example, by attempting to append a test certificate or checking that system pool population succeeded—to confirm the actual fallback behavior matches the test's intent.

中文

加强系统证书池回退的验证。

测试期望在 SSL_CERT_FILE 缺失时使用系统证书池,但只断言了 RootCAs != nil。虽然这确保了优雅降级,但未能区分系统池和 x509.SystemCertPool() 失败时创建的空回退池(实现第 122–124 行)。

考虑验证池中确实包含证书——例如,通过尝试附加测试证书或检查系统池是否成功加载——以确认实际回退行为与测试意图相符。

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@codeexecutor/e2b/internal/codeinterpreter/client_test.go` around lines 134 -
148, The test TestBuildTLSConfigFromEnv_MissingCertFile currently only checks
cfg.RootCAs != nil which doesn't prove the system cert pool was used; update the
test to verify the pool actually contains certificates (e.g., assert
len(cfg.RootCAs.Subjects()) > 0 or otherwise confirm Subjects() returns a
non-empty slice) so it fails if x509.SystemCertPool() fell back to an empty
pool; reference buildTLSConfigFromEnv and the
TestBuildTLSConfigFromEnv_MissingCertFile test to locate where to add this
stronger assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@codeexecutor/e2b/internal/codeinterpreter/client_test.go`:
- Around line 150-161: The test TestBuildTLSConfigFromEnv_MissingCertDir calls
buildTLSConfigFromEnv but doesn't assert RootCAs; add a check to ensure
cfg.RootCAs is non-nil (same style as TestBuildTLSConfigFromEnv_MissingCertFile)
so the test verifies that RootCAs is populated even when SSL_CERT_DIR is
missing.
- Around line 134-148: The test TestBuildTLSConfigFromEnv_MissingCertFile
currently only checks cfg.RootCAs != nil which doesn't prove the system cert
pool was used; update the test to verify the pool actually contains certificates
(e.g., assert len(cfg.RootCAs.Subjects()) > 0 or otherwise confirm Subjects()
returns a non-empty slice) so it fails if x509.SystemCertPool() fell back to an
empty pool; reference buildTLSConfigFromEnv and the
TestBuildTLSConfigFromEnv_MissingCertFile test to locate where to add this
stronger assertion.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 8d595ed1-7bfb-453e-b522-092fce4123d9

📥 Commits

Reviewing files that changed from the base of the PR and between 495760e and f19ccb1.

📒 Files selected for processing (7)
  • codeexecutor/e2b/e2b.go
  • codeexecutor/e2b/internal/codeinterpreter/client.go
  • codeexecutor/e2b/internal/codeinterpreter/client_test.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go
  • codeexecutor/e2b/internal/codeinterpreter/testutil_test.go
  • examples/tool/codeexec/README.md
✅ Files skipped from review due to trivial changes (1)
  • examples/tool/codeexec/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
  • codeexecutor/e2b/internal/codeinterpreter/testutil_test.go
  • codeexecutor/e2b/e2b.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox.go
  • codeexecutor/e2b/internal/codeinterpreter/client.go
  • codeexecutor/e2b/internal/codeinterpreter/sandbox_test.go

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants