Conversation
Signed-off-by: haoshan98 <haoshanw@gmail.com>
maralbahari
left a comment
maralbahari
left a comment
There was a problem hiding this comment.
Thanks for supporting this feature, it's really helpful and it follows the repo architecture nicely, cassettes included. Left some inline comments.
One general thing: the pluggable sandbox. In #308 (comment) we agreed this should be pluggable, but right now Eryx isn't a provider behind an interface, it is the implementation. A second sandbox (E2B, Firecracker, a container runner) means writing a whole new GatewayExecutor and redoing the OpenAI projection.
web_search already has the shape for this: the provider shapes the request and normalizes the response, the handler owns concurrency and the model-facing output. Same split here:
pub(crate) trait CodeInterpreterProvider: Debug + Send + Sync {
fn execute<'a>(
&'a self,
code: &'a str,
limits: &'a ExecutionLimits,
) -> Pin<Box<dyn Future<Output = Result<ExecutionOutput, ToolError>> + Send + 'a>>;
fn max_concurrent_executions(&self) -> Option<NonZeroUsize> { None }
fn check_ready(&self) -> Result<(), ToolError>;
}ExecutionOutput { status, stdout, stderr } already exists in the PR, so it's mostly moving code: plan_gateway_events / public_output / output_item onto the handler, Eryx into eryx.rs behind the feature.
This also cleans up the #[cfg]. Most of the 25 sites only exist because the field is typed on the concrete Eryx type. Use a dyn alias like WebSearchExecutor and 21 of them go away, leaving only mod embedded, its re-export, and the one construction site.
if you'd rather land the feature first. Your call.
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| #[cfg_attr(feature = "openapi", derive(utoipa::ToSchema))] | ||
| #[serde(tag = "type", rename_all = "snake_case")] | ||
| pub enum CodeInterpreterCallOutput { |
There was a problem hiding this comment.
This enum is missing OpenAI's image output variant, and because it is closed for deserialization the gap is not just a feature gap: it silently drops items on the native-upstream path.
OpenAI's code_interpreter_call.outputs is a union of {"type": "logs", "logs": ...} and {"type": "image", "url": ...}. Only Logs is modeled here, so any call that produces a plot or image artifact returns nothing visible. docs/design/tool-framework.md lists this as known future work, which is fair for a first cut given the Eryx executor only captures stdout/stderr.
The part that is not just future work is the ingestion path. docs/design/embedded-code-interpreter.md states:
If an upstream provider emits a native
code_interpreter_call, typed ingestion validates and assembles it before the dispatcher sends its frames through the ordinary wire-restoration path without suppressing them.
That claim does not hold when the upstream item carries an image output. Walking it through:
OutputItemhas#[serde(other)] Unknown, but with#[serde(tag = "type")]that only catches an unrecognized tag value. The tagcode_interpreter_callis recognized, so serde commits toCodeInterpreterCalland the inner failure is not caught byUnknown.slot.rs::completeparses viadeserialize_from_value_opt::<OutputItem>(raw_item), which returnsNoneon failure. The reasoning-specific fallback below it does not apply.ActiveItem::from_payloadcreatesCodeInterpreterCall { item: None }and never populates it, sincemerge_doneonly fires onSome(OutputItem::CodeInterpreterCall(done)).ActiveItem::finalizeisSelf::CodeInterpreterCall { item } => OutputItem::CodeInterpreterCall(item?), so it returnsNone.
Net effect: the output item is dropped from the response with no error and no log. The model's code call vanishes from output[]. This is a silent-data-loss failure mode rather than a rejection, which is the harder kind to debug in production.
Suggested change: add the Image { url: String } variant now. It is a two-line addition to a #[serde(tag = "type")] enum, it makes the wire contract complete, and it removes the drop path. Worth a test that a native upstream code_interpreter_call carrying {"type": "image", ...} survives ingestion and appears in output[].
Separately, consider whether this enum should stay closed at all. OutputItem itself carries an Unknown fallback precisely so an unmodeled upstream shape degrades instead of disappearing. Applying the same policy here, or at minimum logging when a known-tag output item fails to parse in slot.rs, would make future OpenAI additions to this union non-breaking.
| #[cfg(not(feature = "embedded-code-interpreter"))] | ||
| Self::CodeInterpreter(_) => { | ||
| tracing::debug!("code_interpreter tool skipped in normalize - handler not yet registered"); | ||
| tracing::debug!("code_interpreter tool cannot normalize without an available handler"); |
There was a problem hiding this comment.
This second arm is redundant. One tool type should have one entry in this match.
CodeInterpreterHandler and its ToolHandler impl live outside the #[cfg(feature = "embedded-code-interpreter")] mod embedded block in tool/code_interpreter.rs, so CodeInterpreterHandler.normalize(param) compiles in a default build too. The cfg split is not needed to make it build.
It is also unreachable. In a default build a code_interpreter declaration is already rejected before normalization, twice:
registry.rs::insert_code_interpreter_entryreturnscode_interpreter_unavailable_error()undercfg(not(...)).GatewayExecutors::validate_declarationsreturns the same error.
Both run before to_upstream_request calls to_function_tools, so this arm never executes and its tracing::debug! never fires.
Collapsing it also makes the arm consistent with web_search right above, which normalizes unconditionally and leaves availability to the executor (WebSearchHandler::unavailable()):
Self::WebSearch(_) => vec![web_search_function_tool()],
Self::CodeInterpreter(param) => CodeInterpreterHandler.normalize(param),Suggested change: delete lines 109-113 and drop the #[cfg(feature = "embedded-code-interpreter")] on line 107, leaving one arm.
Note this also affects the two tests at the bottom of the file. unavailable_code_interpreter_does_not_create_a_model_visible_function asserts to_function_tools().is_empty() and would no longer hold, so it should be dropped and enabled_code_interpreter_exposes_its_fixed_function_contract un-gated to run in both builds. The fail-closed behavior it was guarding is already covered by build_with_handlers_rejects_unavailable_code_interpreter_before_entry_creation and request_validation_rejects_code_interpreter_without_a_ready_executor, which test it at the layer that actually enforces it.
| } | ||
|
|
||
| #[cfg(feature = "embedded-code-interpreter")] | ||
| mod embedded { |
There was a problem hiding this comment.
Per #308 (comment), this needs to be pluggable. Right now Eryx is not a provider behind an interface, it is the implementation: there is no sandbox trait, so a second backend means writing a whole new GatewayExecutor and re-deriving the OpenAI projection by hand.
Suggest mirroring web_search, where WebSearchProvider shapes requests and normalizes responses while the handler owns concurrency and the model-facing output shape:
tool/code_interpreter/
mod.rs CodeInterpreterHandler: ToolHandler + GatewayExecutor.
Owns admission, output bounding, and the public
code_interpreter_call item + SSE lifecycle.
provider.rs The trait below.
eryx.rs EryxProvider, #[cfg(feature = "embedded-code-interpreter")].
/// A sandbox backend behind `code_interpreter`.
///
/// Implementations run one program under `limits` and normalize the result;
/// the handler owns admission, output bounding, and the public output shape.
/// Cancellation is signalled by dropping the returned future.
pub(crate) trait CodeInterpreterProvider: Debug + Send + Sync {
fn execute<'a>(
&'a self,
code: &'a str,
limits: &'a ExecutionLimits,
) -> Pin<Box<dyn Future<Output = Result<ExecutionOutput, ToolError>> + Send + 'a>>;
/// Backend-imposed ceiling on concurrent executions, if any.
fn max_concurrent_executions(&self) -> Option<NonZeroUsize> { None }
/// Startup readiness probe (Eryx builds one sandbox; a remote backend
/// would ping its control plane).
fn check_ready(&self) -> Result<(), ToolError>;
}ExecutionOutput { status, stdout, stderr } already exists in this PR and is the right neutral boundary type. Pin<Box<dyn Future>> rather than async_trait, to match WebSearchProvider and GatewayExecutor.
Two follow-ons this implies:
- Move
plan_gateway_events,public_output, andoutput_itemoffEryxCodeInterpreterExecutorontoCodeInterpreterHandler, so backends cannot drift from the OpenAI contract independently. - Add a
providerselector toCodeInterpreterRuntimeConfig(likeWebSearchProviderKind) and split the wasmtime-specific keys (max_fuel) into an Eryx sub-config, leaving the neutral limits at the top. Marking the struct#[non_exhaustive]asWebSearchProviderConfigalready is would keep later additions non-breaking.
The Cargo feature gate is a separate axis and should stay: keep eryx.rs gated, keep the trait and handler unconditional. That also removes the #[cfg(...)] branching currently spread across normalize.rs, registry.rs, and executors.rs, since availability becomes "is a provider registered", the way WebSearchHandler::unavailable() already works.
| if let Some( | ||
| mut item @ (OutputItem::Reasoning(_) | ||
| | OutputItem::FunctionCall(_) | ||
| | OutputItem::CodeInterpreterCall(_) |
There was a problem hiding this comment.
I left a comment in line 342 that is related to this.
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved timeout, fail-closed, sandbox ownership, native event preservation, and history rehydration issues remain.
Get a fresh assessment by requesting another Copilot review.
Review effort: Lite
Findings: 1
Open (3)
What changed in this PR
Adds an opt-in, Eryx-backed gateway code_interpreter with sandboxed execution, lifecycle events, streaming support, configuration, documentation, and tests.
Changes:
- Adds strict gateway tool normalization and execution controls.
- Integrates HTTP/SSE, WebSocket, scheduling, and native event handling.
- Adds runtime setup, OpenAPI coverage, CI, and characterization fixtures.
| File | Summary |
|---|---|
scripts/tests/setup-eryx-runtime-test.sh |
Tests runtime setup behavior. |
scripts/setup-eryx-runtime.sh |
Provisions the Eryx runtime. |
rust-toolchain.toml |
Pins the Rust toolchain. |
README.md |
Documents opt-in setup. |
mkdocs.yaml |
Registers design documentation. |
docs/design/tool-framework.md |
Updates tool architecture. |
docs/design/embedded-code-interpreter.md |
Documents implementation and limitations. |
docs/design/codex-integration.md |
Documents tool integration. |
crates/agentic-server/tests/responses_websocket_test.rs |
Tests WebSocket fail-closed behavior. |
crates/agentic-server/tests/responses_test.rs |
Tests HTTP validation. |
crates/agentic-server/src/openapi.rs |
Adds OpenAPI models. |
crates/agentic-server/src/main.rs |
Parses interpreter configuration. |
crates/agentic-server/src/config_file.rs |
Adds file-based configuration. |
crates/agentic-server/Cargo.toml |
Adds feature forwarding. |
crates/agentic-server-core/tests/support/mod.rs |
Supports new output items. |
crates/agentic-server-core/tests/code_interpreter_characterization_test.rs |
Validates recorded contracts. |
crates/agentic-server-core/tests/cassettes/record_code_interpreter_cassettes.sh |
Records characterization cassettes. |
crates/agentic-server-core/tests/cassettes/README.md |
Documents cassette workflows. |
crates/agentic-server-core/tests/cassettes/code_interpreter/prompts.txt |
Defines test prompts. |
crates/agentic-server-core/tests/cassettes/code_interpreter/openai_tools.json |
Defines OpenAI fixtures. |
crates/agentic-server-core/tests/cassettes/code_interpreter/gateway_tools.json |
Defines gateway fixtures. |
crates/agentic-server-core/tests/cassettes/code_interpreter/code-interpreter-openai-reference-gpt-5.6-streaming.yaml |
Stores streaming reference behavior. |
crates/agentic-server-core/tests/cassettes/code_interpreter/code-interpreter-openai-reference-gpt-5.6-nonstreaming.yaml |
Stores non-streaming reference behavior. |
crates/agentic-server-core/tests/cassettes/code_interpreter/code-interpreter-gateway-Qwen-Qwen3.6-35B-A3B-nonstreaming.yaml |
Stores gateway behavior. |
crates/agentic-server-core/src/types/tools/params.rs |
Defines strict request parameters. |
crates/agentic-server-core/src/types/tools/mod.rs |
Exports tool types. |
crates/agentic-server-core/src/types/tools/code_interpreter.rs |
Validates tool arguments. |
crates/agentic-server-core/src/types/request_response.rs |
Updates request fixtures. |
crates/agentic-server-core/src/types/mod.rs |
Re-exports new types. |
crates/agentic-server-core/src/types/io/output.rs |
Adds output-item integration. |
crates/agentic-server-core/src/types/io/mod.rs |
Exports IO models. |
crates/agentic-server-core/src/types/io/code_interpreter.rs |
Defines output and event models. |
crates/agentic-server-core/src/tool/registry.rs |
Registers gateway bindings. |
crates/agentic-server-core/src/tool/ownership.rs |
Updates ownership documentation. |
crates/agentic-server-core/src/tool/normalize.rs |
Normalizes gateway declarations. |
crates/agentic-server-core/src/tool/mod.rs |
Exposes tool modules. |
crates/agentic-server-core/src/tool/handler.rs |
Updates handler contracts. |
crates/agentic-server-core/src/tool/executors.rs |
Adds readiness-gated executors. |
crates/agentic-server-core/src/tool/code_interpreter.rs |
Implements Eryx execution and admission control. |
crates/agentic-server-core/src/lib.rs |
Re-exports public APIs. |
crates/agentic-server-core/src/executor/response_budget.rs |
Accounts for retained output. |
crates/agentic-server-core/src/executor/request.rs |
Updates configuration errors. |
crates/agentic-server-core/src/executor/rehydrate.rs |
Validates inherited declarations. |
crates/agentic-server-core/src/executor/pipeline/tests.rs |
Tests native lifecycle handling. |
crates/agentic-server-core/src/executor/gateway.rs |
Emits gateway lifecycle events. |
crates/agentic-server-core/src/executor/gateway_accumulator.rs |
Allocates event sequences. |
crates/agentic-server-core/src/executor/accumulator/tests.rs |
Tests output accumulation. |
crates/agentic-server-core/src/executor/accumulator/slot.rs |
Supports code-call slots. |
crates/agentic-server-core/src/executor/accumulator/completion.rs |
Merges completed calls. |
crates/agentic-server-core/src/executor/accumulator/active.rs |
Tracks active calls. |
crates/agentic-server-core/src/events/validate.rs |
Validates lifecycle events. |
crates/agentic-server-core/src/events/types.rs |
Adds event and item variants. |
crates/agentic-server-core/src/events/normalize.rs |
Normalizes event payloads. |
crates/agentic-server-core/src/config.rs |
Adds runtime limits. |
crates/agentic-server-core/Cargo.toml |
Adds optional Eryx dependencies. |
Cargo.toml |
Adds workspace dependencies. |
ARCHITECTURE.md |
Updates architecture guidance. |
.rust-file-sizes.json |
Records file-size allowances. |
.github/workflows/rust.yml |
Adds feature-enabled CI. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| let mode = std::fs::metadata(temp_dir) | ||
| .map_err(|_| ToolError::Config("code interpreter TMPDIR cannot be inspected".to_owned()))? | ||
| .permissions() | ||
| .mode() | ||
| & 0o777; | ||
| if mode != 0o700 { |
| Self::McpListTools(list_tools) => Some(InputItem::McpListTools(list_tools.clone())), | ||
| Self::Compaction(item) => Some(InputItem::Compaction(item.clone())), | ||
| Self::WebSearchCall(_) | Self::McpCall(_) | Self::Unknown => None, | ||
| Self::CodeInterpreterCall(_) | Self::WebSearchCall(_) | Self::McpCall(_) | Self::Unknown => None, |
| "crates/agentic-server-core/src/tool/code_interpreter.rs": { | ||
| "limit": 529, | ||
| "reason": "The dedicated Eryx executor keeps sandbox admission, cancellation, bounded output capture, and gateway execution lifecycle together." |
| .with_execution_timeout(config.execution_wall_time) | ||
| .with_max_memory_bytes(memory) | ||
| .with_max_fuel(config.max_fuel.get()); | ||
| let builder = Sandbox::embedded() |
There was a problem hiding this comment.
Sandbox::embedded() still permits raw WASI stdout/stderr and Eryx accumulates complete output internally, so OutputHandler limits do not bound host output or memory. a code-interpreter call can therefore exhaust the gateway or its log sink despite the configured limits. we need to keep this backend unavailable for production until output is bounded at the WASI boundary or by an equivalent external sandbox limit.
Signed-off-by: haoshan98 <haoshanw@gmail.com>
Signed-off-by: haoshan98 <haoshanw@gmail.com>
| Self::McpListTools(list_tools) => Some(InputItem::McpListTools(list_tools.clone())), | ||
| Self::Compaction(item) => Some(InputItem::Compaction(item.clone())), | ||
| Self::WebSearchCall(_) | Self::McpCall(_) | Self::Unknown => None, | ||
| Self::CodeInterpreterCall(_) | Self::WebSearchCall(_) | Self::McpCall(_) | Self::Unknown => None, |
There was a problem hiding this comment.
native code_interpreter_call items still disappear on continuation. OutputItem::to_input_item() returns None for every code-interpreter item, but only gateway-generated calls have the normalized function call and output persisted separately. an upstream-native call therefore loses its context on a later previous_response_id or conversation turn. we should preserve native items through explicit origin-aware history handling.
franciscojavierarceo
left a comment
franciscojavierarceo
left a comment
There was a problem hiding this comment.
the latest patch fixes the done-only ID and public ID-format issues, but the remaining blockers are still unresolved: Eryx can bypass the configured output limits through raw WASI stdout/stderr; native code_interpreter_call items are dropped from continuation history; native image outputs are not represented and can disappear during ingestion; and TMPDIR validation does not reject symlinks or verify ownership. we need to close those paths before this is ready to merge.
Summary
code_interpretertool backed by Eryx 0.8.{"type":"code_interpreter","execution":"gateway"}and normalize it to a strict model-facing function with onecodeargument.code_interpreter_calllifecycle for gateway execution while preserving native upstream code-interpreter events.embedded-code-interpreter, the operator enables the executor, and the embedded runtime passes startup checks.scripts/setup-eryx-runtime.shto install the lockederyx-precompileversion and prepare the platform-specific runtime required by feature-enabled source builds.The Cargo feature and operator setting remain disabled by default. Eryx 0.8.0 still accumulates complete output internally and inherits raw WASI stdout/stderr, so the gateway’s retained-output limits are not hard host-memory or host-output bounds. The design document records this containment limitation and the requirements for distributing feature-enabled production artifacts.
Test Plan
cargo fmt -- --checkbash scripts/tests/setup-eryx-runtime-test.shcargo test -p agentic-server-core code_interpreter --no-fail-fastcargo test -p agentic-server-core --test code_interpreter_characterization_testcargo test -p agentic-server code_interpreter --no-fail-fastcode_interpreter_callingestion and pass-through.runtime.cwasmartifact was not locally available.