feat(sandbox-e2b): E2B microVM sandbox provider worker#92
Open
rohitg00 wants to merge 5 commits into
Open
Conversation
Registers the canonical sandbox::e2b::* trigger family (create, exec, stop, list, snapshot, expose_port, fs::read, fs::write) with capability negotiation, per-worker concurrency tracking, S-code error mapping, and a stubbed REST client awaiting integration against the live E2B API. Tests cover image allowlist enforcement, concurrency rollback on failure, input validation, and the list capacity envelope. Part of the sandbox-as-worker family in iii-hq/workers.
Three behavioural fixes proven against the live E2B API and the new
wiremock suite:
1. POST /sandboxes now forwards `idle_timeout_secs` as the upstream
`timeout` field. Without it E2B's default ~15s reaper kills sandboxes
before any caller can use them, so the worker effectively never
produced a usable sandbox in production.
2. DELETE /sandboxes/{id} treats 404 as success. `stop` is idempotent —
the desired post-state is 'sandbox is not running', which 404 already
satisfies. The prior behaviour leaked the in-flight counter on every
sandbox that auto-died before an explicit stop, and surfaced
confusing S502 errors for an operation that semantically succeeded.
3. `do_list` reconciles `in_flight` against the upstream count when
the GET succeeds. E2B reaps idle sandboxes silently; the
locally-incremented counter would otherwise drift upward forever and
eventually wedge `create` at S400. The reconciliation makes `list`
self-healing. The response gains a `reconciled` boolean so callers
can tell apart authoritative answers from local-only fallbacks.
Tests: 10 wiremock-backed cases now cover happy create, allowlist
reject, 401/429/5xx mapping, 404-stop idempotency + counter release,
5xx-stop, list reconciliation, and list fallback when upstream is
down. The earlier 'stub returns S502' tests were retired; they were
proxies for behaviour we now exercise against a real-shape mock.
Live verification (against api.e2b.dev): create + list + stop +
double-stop all return correct shapes; in_flight drains to zero;
upstream account ends with no leaked sandboxes.
Drops the hand-rolled RFC3339 parser in favour of pass-through string
`started_at`. The numeric-epoch derivation isn't worth its lint
debt.
…ngine Verified the register_function/register_function_with handler signatures are unchanged between 0.11.3 and 0.11.6 (handler closure stays `Fn(R) -> Result<O,E>` — single-arg, no engine-supplied ctx; our HandlerCtx is captured by the closure, not an SDK parameter). All wiremock tests still pass. Pin moves to whatever the engine actually ships.
…ssion
Captures 10 decisions resolved in conversation:
1. Sandbox = caller-facing handle; vendor terminology normalized at
the worker boundary. One noun across the family.
2. Image is provider-specific opaque string; cross-provider portability
of the same value is not guaranteed.
3. "iii primitives" = trigger ABI is the contract. Workers may use
any iii primitive (registerFunction, registerTrigger http/cron/
pubsub/queue, iii.trigger, state::*). External transports are
implementation detail.
4. ABI is iii-sandbox plus extensions. Provider workers register
under sandbox::<provider>::* and MUST NOT shadow the bare
sandbox::* namespace.
5. stop is idempotent w.r.t. observed post-state. 404/409 from
upstream count as success. iii-sandbox's missing-from-registry
error is a known divergence to fix.
6. capabilities[] is advisory. Callers inspect before invoking
optional functions; calling an unregistered function falls
through to the engine's "function not found" error.
7. Snapshot semantics differ per provider; snapshot_id is opaque and
worker-scoped. Restore is not uniform in v0.
8. Namespace mixed: lifecycle flat, sub-resources nested.
9. idle_timeout_secs is the canonical lifetime knob; semantics are
provider-shaped (hard cap on most, idle-reset on CF + Daytona).
max_lifetime_secs reserved for a future iteration.
10. Lifecycle floor (create/exec/stop/list) is a hard requirement.
Extensions are optional and capability-gated. iii-sandbox is the
reference; provider workers track its lifecycle additions
loosely, with capability gating giving reasonable excuse for
incomplete coverage. Each worker is expected to grow toward full
provider-native parity over time.
Lives at sandbox-CONTEXT.md (family-scoped). Will move to /CONTEXT.md
or /docs/sandbox/CONTEXT.md if/when the iii-hq/workers monorepo gets
a CONTEXT-MAP.md and the multi-context structure crystallizes.
This was referenced May 7, 2026
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughThis pull request introduces ChangesE2B Sandbox Worker Implementation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
sandbox-e2b/tests/integration.rs (1)
133-174: ⚡ Quick winAdd a regression test for idempotent
DELETE 409handling.Line 133+ validates 404 idempotency, but 409 (“deletion in progress”) is also part of the documented success post-state contract. A dedicated test would prevent regressions.
🤖 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 `@sandbox-e2b/tests/integration.rs` around lines 133 - 174, Add a regression test mirroring stop_treats_404_as_success to assert that a DELETE returning 409 is treated as a successful idempotent stop: create a new async test (e.g., stop_treats_409_as_success) that starts a MockServer, mounts a Mock responding with status 409 for DELETE /sandboxes/sbx-gone (or similar), initializes ctx via ctx(&server, ...), pre-set ctx.in_flight.store(1, Ordering::SeqCst), call do_stop(&ctx, json!({ "sandbox_id": "..."})).await.unwrap(), assert the response is {} and assert ctx.in_flight.load(Ordering::SeqCst) == 0 to ensure the in-flight slot is released.sandbox-e2b/src/client.rs (1)
197-209: ⚡ Quick win
started_attype inconsistency betweenCreatedSandbox(i64 unix epoch) andSandboxRecord(RFC3339 string).Both structs model the same "sandbox start time" concept, but
CreatedSandbox.started_atis ani64computed locally viaunix_now_secs(), whileSandboxRecord.started_at(used bylist) is aStringpassed through from E2B. Handler code that unifies these two paths will need to convert between them, and callers of the worker ABI receive different types for the same field depending on the code path.Consider unifying on one representation — the
Stringpass-through is already the documented strategy inSandboxRecord's doc-comment. If a string epoch is acceptable forcreate, swapCreatedSandbox.started_attoStringas well and useunix_now_secs().to_string().🤖 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 `@sandbox-e2b/src/client.rs` around lines 197 - 209, CreatedSandbox.started_at currently uses an i64 from unix_now_secs(), which is inconsistent with SandboxRecord.started_at (an RFC3339/String); change CreatedSandbox.started_at to String and set it using unix_now_secs().to_string() (update the struct definition for CreatedSandbox and any place constructing it to call unix_now_secs().to_string()), keeping unix_now_secs() and SandboxRecord.started_at unchanged so both creation and listing paths return the same String timestamp representation.
🤖 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 `@sandbox-e2b/README.md`:
- Line 52: Update the stale README sentence that claims HTTP call bodies are
stubbed and return `S502`: change the description to state that v0.1 now wires
the `create`, `list`, and `stop` endpoints to the real E2B REST endpoints (no
longer returning `S502`), and remove or rephrase any implication that those
three functions are still stubbed; keep mention that other parts remain stubbed
if applicable and ensure `create`, `list`, and `stop` are called out by name.
In `@sandbox-e2b/src/client.rs`:
- Around line 19-28: The HTTP client currently built in pub fn new(...)
(Client::builder() -> .user_agent(...).build()) has no timeout and can hang;
update the builder in new() to set a sensible client-level timeout (e.g.,
Duration::from_secs(30) or a configurable value) so all requests from the http
field (used by create, stop, and list methods) will fail fast on
slow/unresponsive api.e2b.dev; ensure the Duration is imported and consider
exposing the timeout as a parameter if configurability is desired.
- Around line 11-28: E2bClient currently exposes api_key as a public field and
derives Debug, which will leak secrets when formatted; make api_key private
(remove pub on the api_key field) and either remove the Debug derive from
E2bClient or implement a custom Debug for E2bClient that redacts the api_key
(e.g., show "<redacted>" or omit it) while keeping other fields (api_base, http)
visible; update the constructor E2bClient::new (and any callers) to set the
now-private api_key via the struct initializer and provide an explicit safe
accessor if needed (e.g., a method that does not return the raw secret).
In `@sandbox-e2b/src/handler.rs`:
- Around line 151-157: The code currently accepts 0 as a valid port; update the
port validation around the `port` variable (the block using
`u16::try_from(port)` and before calling `ctx.client.expose_port(&sandbox_id,
port).await`) to reject 0 and only allow 1..=65535 by returning a
`WorkerError::BadInput` (e.g., "port must be 1..=65535") when the value is zero
or out of range; implement the check either before `u16::try_from` (if `port ==
0` then error) or immediately after conversion (if `port == 0u16` then error) so
`ctx.client.expose_port(&sandbox_id, port)` never receives 0.
- Around line 175-178: The current extraction of mode silently maps invalid
conversions to 0o644; instead change the logic around the mode variable (the
code using input.get("mode"), Value::as_u64 and u32::try_from) to treat a
present but non-convertible value as a validation error: if "mode" is missing,
you may keep the default; if "mode" exists but u64->u32 conversion via
u32::try_from fails (or Value::as_u64 is None), return an explicit
Err/validation error (with a clear message) from the enclosing handler function
rather than falling back to 0o644 so callers receive a proper error.
- Around line 84-92: The current parsing of args uses filter_map on
input.get("args") -> Value::as_array which silently drops non-string entries;
change this so malformed entries cause an error instead of being ignored: when
extracting "args" from input (the args variable, input.get("args"),
Value::as_array), iterate and verify each element is a string and if any element
is not, return an Err (or propagate a parsing error) rather than skipping it;
replace the filter_map-based collection with an explicit validation step that
fails on the first non-string element and only constructs Vec<String> when all
entries are valid.
In `@sandbox-e2b/src/lib.rs`:
- Around line 62-75: BadInput currently maps to SCode::ProviderUnavailable and
its error message lacks an [Sxxx] prefix; fix by adding a dedicated
SCode::BadInput = "S400" variant in the SCode enum, update WorkerError::code()
to return SCode::BadInput for the BadInput variant, and update the BadInput
#[error(...)] string on the WorkerError::BadInput variant to include a “[S400]”
prefix so its message format matches other variants.
---
Nitpick comments:
In `@sandbox-e2b/src/client.rs`:
- Around line 197-209: CreatedSandbox.started_at currently uses an i64 from
unix_now_secs(), which is inconsistent with SandboxRecord.started_at (an
RFC3339/String); change CreatedSandbox.started_at to String and set it using
unix_now_secs().to_string() (update the struct definition for CreatedSandbox and
any place constructing it to call unix_now_secs().to_string()), keeping
unix_now_secs() and SandboxRecord.started_at unchanged so both creation and
listing paths return the same String timestamp representation.
In `@sandbox-e2b/tests/integration.rs`:
- Around line 133-174: Add a regression test mirroring
stop_treats_404_as_success to assert that a DELETE returning 409 is treated as a
successful idempotent stop: create a new async test (e.g.,
stop_treats_409_as_success) that starts a MockServer, mounts a Mock responding
with status 409 for DELETE /sandboxes/sbx-gone (or similar), initializes ctx via
ctx(&server, ...), pre-set ctx.in_flight.store(1, Ordering::SeqCst), call
do_stop(&ctx, json!({ "sandbox_id": "..."})).await.unwrap(), assert the response
is {} and assert ctx.in_flight.load(Ordering::SeqCst) == 0 to ensure the
in-flight slot is released.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d68bbbf1-af14-4b77-845e-76c2ba10a4c8
⛔ Files ignored due to path filters (1)
sandbox-e2b/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
sandbox-CONTEXT.mdsandbox-e2b/Cargo.tomlsandbox-e2b/README.mdsandbox-e2b/iii.worker.yamlsandbox-e2b/src/client.rssandbox-e2b/src/config.rssandbox-e2b/src/handler.rssandbox-e2b/src/lib.rssandbox-e2b/src/main.rssandbox-e2b/tests/integration.rs
Comment on lines
+11
to
+28
| #[derive(Debug, Clone)] | ||
| pub struct E2bClient { | ||
| pub api_base: String, | ||
| pub api_key: String, | ||
| pub http: Client, | ||
| } | ||
|
|
||
| impl E2bClient { | ||
| pub fn new(api_base: impl Into<String>, api_key: impl Into<String>) -> Self { | ||
| Self { | ||
| api_base: api_base.into(), | ||
| api_key: api_key.into(), | ||
| http: Client::builder() | ||
| .user_agent("iii-sandbox-e2b/0.1") | ||
| .build() | ||
| .expect("reqwest client"), | ||
| } | ||
| } |
There was a problem hiding this comment.
api_key is pub and Debug-derived — API key will leak into logs.
E2bClient derives Debug and exposes api_key as a public field. Any {:?} or {:#?} print of the struct — in a panic, a tracing span, or a test failure — will emit the raw API key. Secrets should not appear in debug output.
-#[derive(Debug, Clone)]
+#[derive(Clone)]
pub struct E2bClient {
- pub api_base: String,
- pub api_key: String,
+ pub api_base: String,
+ api_key: String, // keep private; access via methods only
pub http: Client,
}
+
+impl std::fmt::Debug for E2bClient {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ f.debug_struct("E2bClient")
+ .field("api_base", &self.api_base)
+ .field("api_key", &"[redacted]")
+ .finish()
+ }
+}🤖 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 `@sandbox-e2b/src/client.rs` around lines 11 - 28, E2bClient currently exposes
api_key as a public field and derives Debug, which will leak secrets when
formatted; make api_key private (remove pub on the api_key field) and either
remove the Debug derive from E2bClient or implement a custom Debug for E2bClient
that redacts the api_key (e.g., show "<redacted>" or omit it) while keeping
other fields (api_base, http) visible; update the constructor E2bClient::new
(and any callers) to set the now-private api_key via the struct initializer and
provide an explicit safe accessor if needed (e.g., a method that does not return
the raw secret).
Comment on lines
+19
to
+28
| pub fn new(api_base: impl Into<String>, api_key: impl Into<String>) -> Self { | ||
| Self { | ||
| api_base: api_base.into(), | ||
| api_key: api_key.into(), | ||
| http: Client::builder() | ||
| .user_agent("iii-sandbox-e2b/0.1") | ||
| .build() | ||
| .expect("reqwest client"), | ||
| } | ||
| } |
There was a problem hiding this comment.
No HTTP timeout configured — any hung E2B call will block the worker indefinitely.
Client::builder() only sets user_agent; there is no .timeout() on the client or on any of the three live request paths (create, stop, list). A slow or unresponsive api.e2b.dev will park the async task forever, eventually exhausting the runtime's worker threads under concurrent load.
🛡️ Proposed fix: add a client-level timeout
+use std::time::Duration;
...
http: Client::builder()
.user_agent("iii-sandbox-e2b/0.1")
+ .timeout(Duration::from_secs(30))
.build()
.expect("reqwest client"),📝 Committable suggestion
‼️ 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.
Suggested change
| pub fn new(api_base: impl Into<String>, api_key: impl Into<String>) -> Self { | |
| Self { | |
| api_base: api_base.into(), | |
| api_key: api_key.into(), | |
| http: Client::builder() | |
| .user_agent("iii-sandbox-e2b/0.1") | |
| .build() | |
| .expect("reqwest client"), | |
| } | |
| } | |
| pub fn new(api_base: impl Into<String>, api_key: impl Into<String>) -> Self { | |
| Self { | |
| api_base: api_base.into(), | |
| api_key: api_key.into(), | |
| http: Client::builder() | |
| .user_agent("iii-sandbox-e2b/0.1") | |
| .timeout(Duration::from_secs(30)) | |
| .build() | |
| .expect("reqwest client"), | |
| } | |
| } |
🤖 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 `@sandbox-e2b/src/client.rs` around lines 19 - 28, The HTTP client currently
built in pub fn new(...) (Client::builder() -> .user_agent(...).build()) has no
timeout and can hang; update the builder in new() to set a sensible client-level
timeout (e.g., Duration::from_secs(30) or a configurable value) so all requests
from the http field (used by create, stop, and list methods) will fail fast on
slow/unresponsive api.e2b.dev; ensure the Duration is imported and consider
exposing the timeout as a parameter if configurability is desired.
Comment on lines
+84
to
+92
| let args: Vec<String> = input | ||
| .get("args") | ||
| .and_then(Value::as_array) | ||
| .map(|arr| { | ||
| arr.iter() | ||
| .filter_map(|v| v.as_str().map(ToString::to_string)) | ||
| .collect() | ||
| }) | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
Reject non-string args entries instead of silently dropping them.
Line 84-92 currently uses filter_map, so malformed elements are ignored and command semantics can change without error.
Suggested fix
- let args: Vec<String> = input
- .get("args")
- .and_then(Value::as_array)
- .map(|arr| {
- arr.iter()
- .filter_map(|v| v.as_str().map(ToString::to_string))
- .collect()
- })
- .unwrap_or_default();
+ let args: Vec<String> = match input.get("args") {
+ None => Vec::new(),
+ Some(Value::Array(arr)) => arr
+ .iter()
+ .map(|v| {
+ v.as_str()
+ .map(ToString::to_string)
+ .ok_or_else(|| WorkerError::BadInput("`args` must be an array of strings".into()))
+ })
+ .collect::<Result<_, _>>()?,
+ Some(_) => return Err(WorkerError::BadInput("`args` must be an array of strings".into())),
+ };🤖 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 `@sandbox-e2b/src/handler.rs` around lines 84 - 92, The current parsing of args
uses filter_map on input.get("args") -> Value::as_array which silently drops
non-string entries; change this so malformed entries cause an error instead of
being ignored: when extracting "args" from input (the args variable,
input.get("args"), Value::as_array), iterate and verify each element is a string
and if any element is not, return an Err (or propagate a parsing error) rather
than skipping it; replace the filter_map-based collection with an explicit
validation step that fails on the first non-string element and only constructs
Vec<String> when all entries are valid.
Comment on lines
+151
to
+157
| let port = input | ||
| .get("port") | ||
| .and_then(Value::as_u64) | ||
| .ok_or_else(|| WorkerError::BadInput("missing u16 port".into()))?; | ||
| let port = | ||
| u16::try_from(port).map_err(|_| WorkerError::BadInput("port out of range".into()))?; | ||
| let url = ctx.client.expose_port(&sandbox_id, port).await?; |
There was a problem hiding this comment.
Validate port as 1..=65535 (reject 0).
Line 151-157 allows 0, which is not a meaningful externally exposed service port.
Suggested fix
let port =
u16::try_from(port).map_err(|_| WorkerError::BadInput("port out of range".into()))?;
+ if port == 0 {
+ return Err(WorkerError::BadInput("port must be in 1..=65535".into()));
+ }📝 Committable suggestion
‼️ 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.
Suggested change
| let port = input | |
| .get("port") | |
| .and_then(Value::as_u64) | |
| .ok_or_else(|| WorkerError::BadInput("missing u16 port".into()))?; | |
| let port = | |
| u16::try_from(port).map_err(|_| WorkerError::BadInput("port out of range".into()))?; | |
| let url = ctx.client.expose_port(&sandbox_id, port).await?; | |
| let port = input | |
| .get("port") | |
| .and_then(Value::as_u64) | |
| .ok_or_else(|| WorkerError::BadInput("missing u16 port".into()))?; | |
| let port = | |
| u16::try_from(port).map_err(|_| WorkerError::BadInput("port out of range".into()))?; | |
| if port == 0 { | |
| return Err(WorkerError::BadInput("port must be in 1..=65535".into())); | |
| } | |
| let url = ctx.client.expose_port(&sandbox_id, port).await?; |
🤖 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 `@sandbox-e2b/src/handler.rs` around lines 151 - 157, The code currently
accepts 0 as a valid port; update the port validation around the `port` variable
(the block using `u16::try_from(port)` and before calling
`ctx.client.expose_port(&sandbox_id, port).await`) to reject 0 and only allow
1..=65535 by returning a `WorkerError::BadInput` (e.g., "port must be
1..=65535") when the value is zero or out of range; implement the check either
before `u16::try_from` (if `port == 0` then error) or immediately after
conversion (if `port == 0u16` then error) so
`ctx.client.expose_port(&sandbox_id, port)` never receives 0.
Comment on lines
+175
to
+178
| let mode = input | ||
| .get("mode") | ||
| .and_then(Value::as_u64) | ||
| .map(|m| u32::try_from(m).unwrap_or(0o644)); |
There was a problem hiding this comment.
Do not silently coerce invalid mode to 0o644.
Line 175-178 masks invalid input by defaulting on conversion failure; callers should get explicit validation errors.
Suggested fix
- let mode = input
- .get("mode")
- .and_then(Value::as_u64)
- .map(|m| u32::try_from(m).unwrap_or(0o644));
+ let mode = match input.get("mode").and_then(Value::as_u64) {
+ None => None,
+ Some(m) => Some(
+ u32::try_from(m)
+ .map_err(|_| WorkerError::BadInput("`mode` out of range for u32".into()))?,
+ ),
+ };📝 Committable suggestion
‼️ 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.
Suggested change
| let mode = input | |
| .get("mode") | |
| .and_then(Value::as_u64) | |
| .map(|m| u32::try_from(m).unwrap_or(0o644)); | |
| let mode = match input.get("mode").and_then(Value::as_u64) { | |
| None => None, | |
| Some(m) => Some( | |
| u32::try_from(m) | |
| .map_err(|_| WorkerError::BadInput("`mode` out of range for u32".into()))?, | |
| ), | |
| }; |
🤖 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 `@sandbox-e2b/src/handler.rs` around lines 175 - 178, The current extraction of
mode silently maps invalid conversions to 0o644; instead change the logic around
the mode variable (the code using input.get("mode"), Value::as_u64 and
u32::try_from) to treat a present but non-convertible value as a validation
error: if "mode" is missing, you may keep the default; if "mode" exists but
u64->u32 conversion via u32::try_from fails (or Value::as_u64 is None), return
an explicit Err/validation error (with a clear message) from the enclosing
handler function rather than falling back to 0o644 so callers receive a proper
error.
Comment on lines
+62
to
+75
| impl WorkerError { | ||
| pub fn code(&self) -> SCode { | ||
| match self { | ||
| Self::ImageNotAllowed(_) => SCode::ImageNotAllowed, | ||
| Self::ConcurrencyCapReached(_) => SCode::ConcurrencyCapReached, | ||
| Self::CapabilityUnsupported(_) => SCode::CapabilityUnsupported, | ||
| Self::RateLimited => SCode::RateLimited, | ||
| Self::QuotaExhausted => SCode::QuotaExhausted, | ||
| Self::ProviderUnavailable(_) => SCode::ProviderUnavailable, | ||
| Self::AuthInvalid => SCode::AuthInvalid, | ||
| Self::BadInput(_) => SCode::ProviderUnavailable, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
BadInput incorrectly maps to SCode::ProviderUnavailable.
BadInput represents a caller-side validation failure, but code() returns SCode::ProviderUnavailable (S502) for it. Any caller that inspects the S-code to decide whether to retry will treat a bad-input response as a transient provider outage and keep retrying — the opposite of the correct behaviour.
Additionally, BadInput's #[error(...)] message (line 58) lacks a [Sxxx] prefix, making it inconsistent with every other variant.
Two options: add a dedicated SCode::BadInput → "S400" variant, or re-use an existing non-retryable code such as SCode::ImageNotAllowed (also a caller error) as a fallback.
🐛 Minimal fix: add a dedicated SCode variant
pub enum SCode {
ImageNotAllowed,
ConcurrencyCapReached,
CapabilityUnsupported,
RateLimited,
QuotaExhausted,
ProviderUnavailable,
AuthInvalid,
+ BadInput,
}
impl SCode {
pub fn as_str(self) -> &'static str {
match self {
Self::ImageNotAllowed => "S100",
Self::ConcurrencyCapReached => "S400",
Self::CapabilityUnsupported => "S404",
Self::RateLimited => "S500",
Self::QuotaExhausted => "S501",
Self::ProviderUnavailable => "S502",
Self::AuthInvalid => "S503",
+ Self::BadInput => "S300", // caller-error bucket; pick the next free code
}
}
}
- #[error("bad input: {0}")]
+ #[error("[{}] bad input: {0}", SCode::BadInput.as_str())]
BadInput(String),
Self::BadInput(_) => SCode::ProviderUnavailable,
+ Self::BadInput(_) => SCode::BadInput,🤖 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 `@sandbox-e2b/src/lib.rs` around lines 62 - 75, BadInput currently maps to
SCode::ProviderUnavailable and its error message lacks an [Sxxx] prefix; fix by
adding a dedicated SCode::BadInput = "S400" variant in the SCode enum, update
WorkerError::code() to return SCode::BadInput for the BadInput variant, and
update the BadInput #[error(...)] string on the WorkerError::BadInput variant to
include a “[S400]” prefix so its message format matches other variants.
Stops shadowing the bare sandbox::* namespace. Routes through the new sandbox router worker (provider="e2b"); direct invocation of the sandbox::provider::e2b::* ids stays supported and stable. Handler logic, tests, and S-code mapping unchanged.
Contributor
Author
|
Refactored to register only Validation: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First member of the sandbox worker family + the family contract doc.
What ships
sandbox-e2b/— narrow Rust iii worker that exposes E2B microVM sandboxes viasandbox::e2b::*triggers. Live verified end-to-end againstapi.e2b.dev.sandbox-CONTEXT.md— family-scoped CONTEXT.md captured from a grill-with-docs session. 10 resolved decisions covering terminology, ABI shape, idempotency contract, and evolution policy. See the file for details.Functions
sandbox::e2b::createPOST /sandboxes(forwardsidle_timeout_secsastimeout)sandbox::e2b::exec/sandboxes/{id}/processesSSE; streaming parser is a follow-upsandbox::e2b::stopsandbox::e2b::listin_flightto upstream countsandbox::e2b::snapshotsandbox::e2b::expose_portsandbox::e2b::fs::read/fs::writeCapabilities advertised:
["snapshot","expose_port","fs"].Live verification
Cycle run against
api.e2b.devwith a realE2B_API_KEY:in_flight=1, sandbox visible{}success{}(the idempotency fix this PR captures)0 concurrent sandboxesat endTests
tests/integration.rs— 10 wiremock-backed cases:in_flightstays 0)in_flightto upstream countcargo test,cargo clippy --all-targets --all-features -- -D warnings,cargo fmt --all -- --checkall clean.ABI
Per
sandbox-CONTEXT.md:create,exec,stop,list) is the family-wide hard requirementcapabilities[]iii-sandboxis the reference; provider workers register undersandbox::<provider>::*stopis idempotent w.r.t. observed post-state (404/409 from upstream count as success)Pin
iii-sdk = "=0.11.6"to match the running engine.Summary by CodeRabbit
New Features
Documentation