fix(tools): centralize container_config across sandbox entry points - #60815
NaMinhyeok wants to merge 2 commits into
Conversation
…l parity The three sandbox entry points (terminal, execute_code, file tools) share one environment slot per task but hand-copy the container_config dict they pass to _create_environment. The copies drifted: execute_code's dict lacks docker_forward_env, docker_env, docker_extra_args, modal_mode, docker_mount_cwd_to_workspace, docker_persist_across_processes and docker_orphan_reaper; file_tools lacks docker_env, docker_extra_args, modal_mode and the persist/reaper flags. _create_environment silently defaults missing keys (cc.get(key, default)), so whenever execute_code is the tool that (re)creates the shared environment - e.g. after the idle-lifetime cleanup evicts it - the new environment is built with forward_env=[] and every secret listed in docker_forward_env vanishes from the session env snapshot for all tools until a terminal/file call happens to recreate it. Same mechanism for the other dropped keys. Bring both dicts to full terminal parity and add regression tests that pin every key with a non-default sentinel and assert execute_code and file_tools build identical configs.
Competing-PR cluster for container_config parity across the sandbox entry points: this PR is effectively a rebase of the stale OPEN #30097 (execute_code subset) and OPEN #35660 (file_tools + code_execution parity) onto current |
|
Thanks for the focused parity fix. The premise holds on current The added test captures both entry points' Automated hermes-sweeper review. |
Route terminal, execute_code, file tools, and the prompt backend probe through one canonical container_config builder. This prevents future caller drift and fixes the probe silently ignoring docker_network=false. Add behavior-level parity checks plus real env-config-to-Docker-factory regressions for secret forwarding and network lockdown. Co-authored-by: elozadaf <elozada@lozortfederal.com> Co-authored-by: Ted Malone <ted.malone@outlook.com>
2fbf92e to
2a7daf1
Compare
|
Follow-up pushed in While re-auditing every production Rather than adding another one-off key fix, the follow-up now:
Contributor credit from the prior implementations is preserved: Validation:
The PR title and description have also been updated to match the final four-caller implementation and reproducible test command. |
|
Independent confirmation from production (Hermes v0.19.0, Synology DSM / ContainerManager): We hit exactly this. The |
|
@teknium1 the bug still exists. Docker containers may not get the env variables depending on which tool created them. This makes Docker containers unusable unless the code is fixed locally. However, this PR is now stale and out of sync with main. Should I create replacement PR based on current |
What does this PR do?
Fixes container environment settings being silently dropped depending on which production path calls
_create_environment().There are four callers that need the same
container_config:terminal_toolexecute_codeThe first three share one collapsed environment slot per task, so whichever path creates or recreates the sandbox determines the configuration reused by all three. The prompt probe uses a separate slot, but it also creates a real container.
Before this PR:
execute_codeomitteddocker_forward_env,docker_env,docker_extra_args,modal_mode, mount, persistence, and orphan-reaper settings.docker_env,docker_extra_args,modal_mode, persistence, and orphan-reaper settings.docker_network, soterminal.docker_network: falsesilently fell back to a network-enabled probe container.This PR adds one private
_build_container_config(config)projection intools/terminal_tool.pyand routes all four callers through it. That fixes the current losses and removes the hand-copied dictionaries that repeatedly drifted.Why this is not already fixed
Earlier fixes covered only subsets of the call paths. #14235 fixed the terminal dictionary, and #12900 partially fixed file tools; the execute-code path remained incomplete. The prompt-probe
docker_networkomission was a fourth sibling path not covered by the existing three-module regression guard.Related work and contributor credit
This patch builds on prior reports and implementations:
malaiwah) identified the shared-slotdocker_forward_envloss across tool paths.elozadafsupplied the execute-code Docker env/extra-args fix.temalo) supplied full tool-path parity tests and explicitly proposed a shared helper as the follow-up.elozadafand Ted Malone are preserved as co-authors on follow-up commit2a7daf1d4. Related historical reports/attempts include #12534, #17620, #16214, and #5722.Changes made
tools/terminal_tool.py— add the canonical container-config builder and use it in the terminal path.tools/code_execution_tool.py— use the shared builder.tools/file_tools.py— use the shared builder.agent/prompt_builder.py— use the shared builder so the probe honorsdocker_networkand every other container setting.TERMINAL_*parsing reaches the real environment factory and final Docker constructor forforward_env, static env, extra args, and network lockdown;docker_network: falsethrough the real parser/builder/factory chain.How to test
Result on the PR branch: 321 passed, 1 skipped.
The complete two-commit PR was also replayed onto current
origin/main(e4ea0a0ed) in a temporary worktree: clean rebase, 321 passed, 1 skipped. Ruff, Python compilation, the Windows-footgun check, andgit diff --checkalso pass.Scope
container_config=None).Checklist
CONTRIBUTING.mdandAGENTS.md.main.