fix(docker): full container_config parity for file_tools and code_execution_tool - #35937
fix(docker): full container_config parity for file_tools and code_execution_tool#35937jsplec wants to merge 1 commit into
Conversation
4ee8a7d to
1419cda
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for identifying the order-dependent Docker configuration path. The two docker_env / docker_extra_args omissions are still present on current main in tools/file_tools.py:1141-1151 and tools/code_execution_tool.py:680-688.
Problems
- The PR's
cli.py,gateway/run.py, and bridge-test changes are already implemented by merged PR #50631 (de6b3ae37). - The Docker configs still would not be fully identical:
tools/terminal_tool.py:2202-2216forwards additional settings that the PR'scode_execution_tool.pyconfig does not carry. This matches the linked #35660 superset discussion. - The new unconditional agent halt bypasses main's explicit warning-first, opt-in hard-stop policy in
agent/tool_guardrails.py:64-79(0704589). Its per-error map also does not reset on a different failure, so it does not enforce a consecutive streak, and it is not wired into the concurrent executor.
Suggested changes
- Salvage the two remaining Docker keys with focused file-tools and execute-code coverage; drop the already-merged bridge hunks.
- Rework any loop-stop proposal through
ToolCallGuardrailControllerand cover sequential/concurrent behavior.
Automated hermes-sweeper review.
| if _is_error_result: | ||
| _err_key = (function_name, _err_text[:120]) | ||
| streak = agent._consecutive_tool_error_streak | ||
| streak[_err_key] = streak.get(_err_key, 0) + 1 |
There was a problem hiding this comment.
This counter is keyed by error text but only cleared after a success. A failure sequence A, B, A increments A twice even though the failures were not consecutive. Reset the prior fingerprint whenever a different result is observed, or use the existing guardrail controller.
| @@ -3987,6 +3989,20 @@ def _perform_api_call(next_api_kwargs): | |||
| pass | |||
| break | |||
|
|
|||
| if agent._tool_error_loop_halt is not None: | |||
There was a problem hiding this comment.
The new halt flag is only updated in the sequential executor. Current concurrent execution has its own path in agent/tool_executor.py, so this loop check cannot halt repeated failures produced there. Route any policy through the shared guardrail mechanism instead.
| @@ -602,6 +602,7 @@ def load_cli_config() -> Dict[str, Any]: | |||
| "container_persistent": "TERMINAL_CONTAINER_PERSISTENT", | |||
| "docker_volumes": "TERMINAL_DOCKER_VOLUMES", | |||
| "docker_env": "TERMINAL_DOCKER_ENV", | |||
| "docker_extra_args": "TERMINAL_DOCKER_EXTRA_ARGS", | |||
There was a problem hiding this comment.
This bridge is already on current main via merged PR #50631 (de6b3ae37). Please drop this duplicate hunk when salvaging the remaining container-config propagation fix.
| @@ -649,6 +649,8 @@ def _get_or_create_env(task_id: str): | |||
| "container_persistent": config.get("container_persistent", True), | |||
| "docker_volumes": config.get("docker_volumes", []), | |||
| "docker_run_as_host_user": config.get("docker_run_as_host_user", False), | |||
| "docker_env": config.get("docker_env", {}), | |||
| "docker_extra_args": config.get("docker_extra_args", []), | |||
There was a problem hiding this comment.
These two keys fix part of the divergence, but the resulting config still differs from terminal_tool's container config: it omits settings such as docker_forward_env and docker_mount_cwd_to_workspace. Either narrow the PR claim or align with the linked parity work (#35660).
…er_config parity with terminal_tool terminal_tool._create_environment() is the source of truth for docker container_config: modal_mode, docker_volumes, docker_mount_cwd_to_workspace, docker_forward_env, docker_env, docker_run_as_host_user, docker_extra_args, docker_network, docker_persist_across_processes, docker_orphan_reaper. The container is created lazily by whichever tool wins the race to touch it first (file ops, code execution, or terminal), via _resolve_container_task_id. file_tools._get_file_ops() and code_execution_tool._get_or_create_env() built their own container_config dicts that omitted several of these keys, so a file or code-execution op winning the race silently dropped the user's configured proxy env, docker run flags, orphan reaping, and (for code_execution_tool specifically) cwd mounting and forwarded env too. The result was order-dependent and non-deterministic from the user's perspective. Bring both to full parity with terminal_tool's container_config dict, and pin each key with focused tests (test_file_tools_container_config.py, new test_code_execution_container_config.py) so a future divergence is caught the same way test_terminal_config_env_sync.py already catches CLI/gateway bridge drift. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1419cda to
e83c496
Compare
Docker: full container_config parity for file_tools and code_execution_tool
terminal_tool._create_environment()is the source of truth for the dockercontainer_configdict:modal_mode,docker_volumes,docker_mount_cwd_to_workspace,docker_forward_env,docker_env,docker_run_as_host_user,docker_extra_args,docker_network,docker_persist_across_processes,docker_orphan_reaper.The shared per-task container is created lazily by whichever tool wins the race to touch it first (
_resolve_container_task_id).file_tools._get_file_ops()andcode_execution_tool._get_or_create_env()each build their owncontainer_configdict, and both omitted several of the above keys — so a file or code-execution op winning the race silently dropped the user's configured proxy env, extradocker runflags, orphan reaping, and (forcode_execution_toolspecifically) cwd mounting and forwarded env too. Order-dependent and non-deterministic from the user's perspective.Fix
Bring both
file_tools.pyandcode_execution_tool.pyto full parity withterminal_tool.py'scontainer_configdict, not justdocker_env/docker_extra_args.Tests
tests/tools/test_file_tools_container_config.pywithdocker_env/docker_extra_argscoverage.tests/tools/test_code_execution_container_config.pypinning all container-config keys forcode_execution_tool._get_or_create_env, mirroring the file_tools test.Changes from the original PR, per review:
cli.py/gateway/run.pyTERMINAL_DOCKER_EXTRA_ARGSbridge hunks — already merged via fix(terminal): bridge docker_extra_args to TERMINAL_DOCKER_EXTRA_ARGS in CLI + gateway #50631 (de6b3ae3).docker_env,docker_extra_args) to full parity (7 more keys were still missing incode_execution_tool.py, 5 infile_tools.py) after @teknium1 pointed outterminal_tool.py:2202-2216forwards a wider set than either tool carried — matches the parity work referenced in fix(tools): bring file_tools + code_execution container_config to terminal_tool parity #35660.mainalready shipsagent/tool_guardrails.py'sToolCallGuardrailController, which is fully wired into bothexecute_tool_calls_sequentialandexecute_tool_calls_concurrent(tool_executor.py) and already haltsrun_conversation(conversation_loop.py:4690) via_tool_guardrail_halt_decision. It covers the same repeated-identical-failure case (exact_failure_warn_after/exact_failure_block_after) without the streak-key bug the original addition had (A,B,A double-counted A) and without the concurrent-path gap. The 44k-token memory-replace retry loop that motivated the original addition would be caught by this existing controller today iftool_loop_guardrails.hard_stop_enabledis turned on (it's opt-in, off by default) — a config change, not a code change, and outside this PR's scope.Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com