Skip to content

fix(tools): reuse canonical container config across sandbox creators - #90050

Open
kentimsit wants to merge 3 commits into
NousResearch:mainfrom
kentimsit:fix/centralize-container-config
Open

fix(tools): reuse canonical container config across sandbox creators#90050
kentimsit wants to merge 3 commits into
NousResearch:mainfrom
kentimsit:fix/centralize-container-config

Conversation

@kentimsit

@kentimsit kentimsit commented Aug 19, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

Hermes has four production paths that can create a container-backed environment: terminal commands, file tools, execute_code, and the system-prompt backend probe. Terminal creation now uses the canonical _container_config_from_config() projection, but the other three paths still hand-copy different subsets of the same configuration.

That makes behavior depend on which path creates or recreates the environment first. In production, a file-tool first creation reused a stale cross-process container without the current mount policy, while an execute_code recreation after idle cleanup lost its forwarded-variable policy.

This change routes the remaining three callers through the existing canonical projection. It preserves all current defaults and keeps local and SSH behavior unchanged with container_config=None.

The shared-projection design was proposed in #60815 by @NaMinhyeok, but that PR is now conflicting. My August 7 coordination question about replacing it from current main received no reply. Current main now already contains _container_config_from_config() for terminal and lazy task-environment creation, so this refresh reuses that helper.

Prior reports and partial implementations informed this refresh: #30097 by @elozadaf, #35660 by @temalo, #35937 by @jsplec, #65367 by @0112358DEUS, #75296 by @MichaelHuangXiaofei, and #80744 by @tanjelly. The prompt-network work in #62023 by @sagitario-jpn, #76911 by @webtecnica, and #76926 by @a-yeyang also informed the behavior coverage. Thanks as well to issue reporters @Shaihaan, @nikolay-bratanov, @MichaelHuangXiaofei, @smfed, and @ariv374.

Related Issue

Fixes #84027
Fixes #16214
Fixes #75291
Fixes #76906

Related to #87995. Its separate working-directory and duplicate-network-flag concerns are outside this focused change.

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • ✨ New feature (non-breaking change that adds functionality)
  • 🔒 Security fix
  • 📝 Documentation update
  • ✅ Tests (adding or improving test coverage)
  • ♻️ Refactor (no behavior change)
  • 🎯 New skill (bundled or hub)

Changes Made

  • tools/file_tools.py now derives container settings from the canonical projection when file tools create the shared environment first.
  • tools/code_execution_tool.py now preserves forwarded variables, static environment values, mounts, Docker arguments, shared memory, and lifecycle policy when recreating an environment after cleanup.
  • agent/prompt_builder.py now uses the same projection for backend probes, including docker_network and vercel_runtime.
  • Behavior tests compare each real caller boundary with the canonical projection.
  • The former AST/source-shape network assertion is replaced with an in-process TERMINAL_* parser → projection → environment factory → fake Docker constructor test. It covers forwarded names, static values, extra arguments, shared memory, network lockdown, cross-process persistence, and orphan-reaper policy without starting Docker.

How to Test

  1. Run the focused caller and Docker-constructor regressions:

    scripts/run_tests.sh tests/tools/test_file_tools_container_config.py tests/tools/test_code_execution_container_config.py tests/tools/test_docker_network_config.py -q

    Result: 8 passed.

  2. Run the prompt-probe regression:

    scripts/run_tests.sh tests/agent/test_prompt_builder.py -k probe_remote_backend_forwards_canonical_container_config -q

    Result: 1 passed.

  3. Run the socket-dependent execute_code suites in an environment that permits Unix-domain sockets:

    scripts/run_tests.sh tests/tools/test_code_execution.py tests/tools/test_code_execution_modes.py -q

    Result: 79 passed.

  4. Run static checks:

    uv run ruff check tools/file_tools.py tools/code_execution_tool.py agent/prompt_builder.py tests/tools/test_file_tools_container_config.py tests/tools/test_code_execution_container_config.py tests/tools/test_docker_network_config.py tests/agent/test_prompt_builder.py
    uv run python scripts/check-windows-footguns.py --all

    Results: Ruff passed; the Windows-footgun scan passed across 985 files.

The broader affected terminal/config group passed after rerunning socket-dependent cases with the required VM permission. The complete prompt-builder file has 64 passes and one unrelated existing failure caused by this executor treating /tmp as a Git worktree; the new prompt regression passes independently.

A full repository run is not claimed as green in this minimal dev environment. It was stopped after 3,953 passes because unrelated suites require optional acp and Anthropic SDK dependencies that are not installed here, and several existing tests assume /tmp is outside a Git worktree. No live Docker container was started.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: Linux VM with real parsing/factory boundaries and fake container constructors; no live Docker acceptance run

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — N/A; no public behavior or configuration surface changed
  • I've updated cli-config.yaml.example if I added/changed config keys — N/A; no config keys changed
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — N/A; this reuses an existing private projection
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide
  • I've updated tool descriptions/schemas if I changed tool behavior — N/A; no tool schema changed

Screenshots / Logs

N/A.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists tool/terminal Terminal execution and process management tool/file File tools (read, write, patch, search) tool/code-exec execute_code sandbox backend/docker Docker container execution area/config Config system, migrations, profiles sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data duplicate This issue or pull request already exists labels Aug 19, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Duplicate of #60815 — both route prompt probing, file tools, and execute_code through one canonical container-configuration projection so sandbox policy cannot diverge.

@webtecnica

Copy link
Copy Markdown
Contributor

Thanks for the credit and for the careful cross-caller consolidation — glad #76911 could inform the coverage. Happy to help review or adapt if needed. 🚀

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

Labels

area/config Config system, migrations, profiles backend/docker Docker container execution duplicate This issue or pull request already exists P2 Medium — degraded but workaround exists sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/code-exec execute_code sandbox tool/file File tools (read, write, patch, search) tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

3 participants