Skip to content

fix(tools): scope the terminal env bridge one-shot to the Hermes home - #94206

Closed
liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-94200
Closed

liuhao1024 wants to merge 2 commits into
NousResearch:mainfrom
liuhao1024:liuhao/cron-bugfix-94200

Conversation

@liuhao1024

@liuhao1024 liuhao1024 commented Aug 24, 2026 •

Copy link
Copy Markdown

What does this PR do?

Under profile multiplexing (gateway.multiplex_profiles: true), _ensure_terminal_env_bridged()'s process-global one-shot flag let the first profile to run a terminal call permanently pin its TERMINAL_* selection into os.environ. With a docker-backend profile (e.g. coder) bridging before a local-backend one (e.g. board-leonard), every later profile inherited Docker — intermittent "sandbox flapping" depending on cron tick order at gateway boot.

This scopes the one-shot to the Hermes home (the reporter's suggested direction, verified in their fork): the last-bridged home is tracked alongside the flag, and a scope change re-bridges. On top of the reporter's patch it also unsets the TERMINAL_* keys the previous bridge introduced before re-bridging — without that, a profile whose config has no terminal section keeps the previous profile's backend forever through the elif "TERMINAL_ENV" not in os.environ branch, which is the leak's worst shape. Shell/.env-exported TERMINAL_* values are never bridge-owned and survive handoffs; within a single profile the one-shot semantics (and its optimization purpose) are unchanged.

Related Issue

Fixes #94200
Fixes #98581

Type of Change

  • 🐛 Bug fix

Changes Made

  • tools/terminal_tool.py (_ensure_terminal_env_bridged):
    • New module globals _terminal_config_bridge_scope (last-bridged str(get_hermes_home())) and _terminal_config_bridge_keys (the TERMINAL_* keys the last bridge introduced).
    • The one-shot guard now compares scope; on a scope change the bridge-introduced keys are popped from os.environ and the bridge re-runs for the incoming profile's config. Docstring documents the multiplexing failure mode.
  • tests/tools/test_terminal_env_bridge.py:
    • The autouse reset fixture also resets the two new globals.
    • Four new tests: docker→local handoff re-bridges; a section-less profile no longer inherits the previous profile's docker selection (the elif branch path); one-shot within a single scope is preserved (single apply_terminal_config_to_env call across three _get_env_config() runs); shell-exported TERMINAL_* keys survive handoffs.

How to Test

  • Run: .venv/bin/python -m pytest tests/tools/test_terminal_env_bridge.py tests/tools/test_docker_session_isolation.py tests/tools/test_terminal_degraded_mode.py -q
  • Observed result: 54 passed, 1 failed — the single failure (test_terminal_degraded_mode.py::TestConfigBridging::test_degraded_mode_is_bridged_everywhere) reproduces identically on unpatched main (stash-compared), i.e. pre-existing in this environment. Within test_terminal_env_bridge.py: 13 passed. Fail-on-main check (git stash of the source change): both scope-handoff tests fail against unpatched main (profile B inherits docker); the one-shot and shell-key tests pass both ways.

Checklist

  • Code follows the project's style guidelines
  • Self-review of the completed code performed
  • New and existing unit tests pass locally
  • Changes are backward-compatible (single-profile behavior unchanged; only cross-profile handoff gains the re-bridge)

@alt-glitch alt-glitch added type/bug Something isn't working comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management area/profiles Multi-profile isolation, HERMES_HOME scoping P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades labels Aug 24, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference, author can ignore or act on any point.

Good fix shape: keying the one-shot to the context-local home override plus tracking bridge-introduced keys (set-difference against a pre-bridge snapshot, so shell/.env exports survive handoffs — nicely covered by test_handoff_never_unsets_shell_exported_terminal_vars) is exactly right for #94200. Two issues:

  1. Failure path defeats the cleanup — tools/terminal_tool.py:1634-1635 flip _terminal_config_bridge_attempted / _terminal_config_bridge_scope to the new profile before the purge at :1643-1645, which sits inside the try after the hermes_cli.config import. If that import (or anything up to the purge) raises on the incoming profile, the except swallows it, the stale previous-profile TERMINAL_* values stay in os.environ, and every later call short-circuits at :1632 — the docker-pin leak this PR fixes, reintroduced precisely when the config machinery is broken. Move the key purge + _terminal_config_bridge_keys = set() above the flag flips (or reset _terminal_config_bridge_attempted = False in the except), so a failed re-bridge retries instead of latching.

  2. Thread safety under multiplexing: these module globals (tools/terminal_tool.py:1586-1592) are read/checked/written non-atomically from :1632-1635 and :1643-1665. If multiplexed profiles serve agents on different threads, two simultaneous first-calls can interleave the snapshot/purge and produce a wrong ownership set (e.g. dropping user-exported keys). A simple threading.Lock around the body would close it; worth doing since multiplexing is the stated motivation.

Minor: current_scope = None in the except at :1631 silently treats an override-API failure as "single-home"; fine, but a debug log there would help future forensics.

@liuhao1024

Copy link
Copy Markdown
Author

Excellent catch on point 1 — that failure path did reintroduce the leak exactly when the config machinery is broken. Fixed in 8cdcea25, along with point 2:

  1. The key purge and _terminal_config_bridge_keys = set() now run before the attempt flags latch, so an import failure (or anything below) still clears the previous profile's bridged keys. The failed bridge degrades to the ambient env — the historical local default — and the latch still holds, keeping the one-shot semantics: a broken config is re-attempted once per scope change, not on every call. test_failed_rebridge_purges_previous_profile_before_latching pins this: profile A pins docker, then profile B's bridge hits a poisoned hermes_cli.config import, and TERMINAL_ENV is asserted gone afterwards.
  2. Added _terminal_config_bridge_lock — the whole check/purge/re-bridge sequence now runs under one threading.Lock(), so two racing first-calls under multiplexing can't interleave the snapshot/purge and misattribute the ownership set. The bridge runs once per scope, so the lock's serial window is a one-time startup cost.
  3. On the minor: left current_scope = None silent — the override API failing is a hermes_constants-level anomaly, and a debug log there would fire for every terminal call in that (already broken) state since the one-shot can't latch a meaningful scope from it.

@liuhao1024

Copy link
Copy Markdown
Author

Consolidated the ownership/keying improvements from #98589 here, per the triage note — and rebased the branch onto current main (the terminal tool had moved under it).

What the new head adds on top of the previous one:

With this the PR covers both #94200 (gateway multiplexing) and #98581 (dashboard profile switcher); body updated to close both. Local runs: tests/tools/ -k terminal → 300 passed / 3 skipped, plus the docker-session-isolation, degraded-mode and web-server profile-unification suites — all green.

muhifni added a commit to muhifni/hermes-agent that referenced this pull request Sep 2, 2026
A multiplexed Hermes process (gateway.multiplex_profiles, unified
dashboard/TUI, or cron) can serve several profiles at once. Terminal
settings used to resolve through process-global TERMINAL_* env vars plus
the one-shot _ensure_terminal_env_bridged() guard. The first profile to
touch a terminal tool after startup therefore pinned its backend and
other policy (mounts, SSH target, network, cwd, resources) onto every
later profile until restart.

This is a correctness and sandbox-boundary bug: a local profile can run
inside another profile's docker sandbox, and a docker/ssh profile can be
dropped onto the launch host. Repro lineages: canonical NousResearch#68559, gateway
backend latch NousResearch#94200, dashboard/container symptoms NousResearch#98581/NousResearch#96992.

Fix with an authoritative profile terminal policy seam, analogous to
agent/secret_scope.py:

- tools/terminal_scope.py adds a ContextVar that holds the active
  profile's complete effective terminal policy. Projection order is
  defined defaults + supplemental tool defaults <- profile .env
  TERMINAL_* values <- explicit terminal: config.yaml keys. Once a scope
  is bound, missing values never fall through to ambient os.environ.
- install_profile_terminal_scope() fail-closes: unreadable/malformed
  policy installs a refusal scope; terminal_tool / execute_code refuse
  execution instead of inheriting launch-process policy.
- tools/terminal_tool.py routes TERMINAL_* reads through the scope-aware
  _tenv() helper and suppresses the process-env bridge while scoped.
- gateway/run.py, tui_gateway/server.py and cron/scheduler.py install the
  same profile terminal scope at their in-process profile boundaries.
- Other scoped readers from the first patch (prompt/cwd/media/footer)
  remain routed through the same seam.

Tests now cover the review-requested matrix: polluted launch profile A
with sensitive mounts/SSH/CWD/network/resource policy; profile B with
backend-only, empty terminal config, or .env-only selections cannot
observe A's values. They also cover malformed/unreadable policy refusal,
terminal_tool refusal, gateway cleanup including error paths,
dashboard/TUI session scope, and cron install/reset lifecycles.

Prior art / lineage: x7peeps NousResearch#68611 (original ContextVar direction),
100yenadmin NousResearch#79117/NousResearch#78030, liuhao1024 NousResearch#94206/NousResearch#98589, and complementary
Bergmann89 NousResearch#97014 (env_loader re-bridge containment).

Fixes NousResearch#68559
Refs NousResearch#94200, NousResearch#98581, NousResearch#96992
…ection

Rebased onto current main (terminal_tool.py was heavily refactored since
the branch's last rebase): the NousResearch#68559 per-turn-scope suppression of the
bridge is kept as the first check, and the home-keyed one-shot from
NousResearch#94200/NousResearch#98581 now sits under it.

- The one-shot latch is keyed to hermes_home_key() (context-local profile
  override -> HERMES_HOME env -> platform default); a home change
  re-bridges instead of pinning the first home's TERMINAL_* for everyone.
- Keys owned by the previous home's terminal section (computed via
  terminal_config_owned_env_vars, so launcher-bridged writes are covered
  too) are purged before the flags latch, so a failed re-bridge degrades
  to the local default instead of re-leaking the previous selection.
- The purge/keys/scope sequence is serialized: racing first terminal
  calls under multiplexing could otherwise attribute the wrong ownership
  set.

Fixes NousResearch#94200, fixes NousResearch#98581
…scoped bridge

Regression for the multiplexed-dashboard residual of NousResearch#68559 (NousResearch#107422):
the launch profile runs without a home override, so when a secondary
profile's unscoped path (agent-build probe, execute_code) latches its
docker policy into the process env, the launch profile's own tool call
must re-bridge from its own config instead of inheriting the secondary
profile's container, image, and volumes.
@liuhao1024
liuhao1024 force-pushed the liuhao/cron-bugfix-94200 branch from f87ba8d to d98dc2e Compare September 10, 2026 15:01
@liuhao1024

Copy link
Copy Markdown
Author

Rebased onto current main (terminal_tool.py was heavily refactored since the last rebase) and added a regression for the multiplexed-dashboard residual reported in #107422.

What changed in the rebase:

Verification: 18/18 in tests/tools/test_terminal_env_bridge.py, 131 passed across the bridge's reference surface (test_file_tools_plugin_container_cwd, test_docker_session_isolation, test_container_cwd_sanitize, test_terminal_task_cwd, etc.); the new test fails on unmodified main.

@teknium1

Copy link
Copy Markdown
Collaborator

Superseded by #108440 (on main as a5c801c, from #107442). You keyed the one-shot TERMINAL_* bridge to the Hermes home so a scope change re-bridges; what shipped removes the write altogether under a profile home override — a routed profile binds a terminal scope instead, and only the launch profile's terminal.* ever reaches process env. That avoids the re-bridge race between two secondaries as well. Thanks for the analysis, @liuhao1024.

@teknium1 teknium1 closed this Sep 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/profiles Multi-profile isolation, HERMES_HOME scoping comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades tool/terminal Terminal execution and process management type/bug Something isn't working

Projects

None yet

4 participants