From 62b9fb662346b630e23d13ded577be84b87f74ce Mon Sep 17 00:00:00 2001 From: georgex8001 Date: Tue, 30 Jun 2026 03:12:10 -0700 Subject: [PATCH] fix(acp): thread-safe interactive approval via contextvars MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Concurrent ACP sessions run on a shared ThreadPoolExecutor (max_workers=4). Each _run_agent mutated the process-global os.environ["HERMES_INTERACTIVE"] and restored it in finally, so one session's restore could clobber another's set mid-run — dropping the second session onto the non-interactive auto-approve path, executing a dangerous command without the approval callback firing (GHSA-96vc-wcxf-jjff). Replace the env-var flag with a thread/task-local contextvar in tools.approval. The two HERMES_INTERACTIVE read sites in approval.py now go through _is_interactive_cli() (contextvar-first, env fallback for legacy single-threaded CLI callers). The ACP executor sets the contextvar instead of os.environ; the existing contextvars.copy_context() wrapper isolates each session's write. Co-authored-by: Hermes Agent <127238744+teknium1@users.noreply.github.com> --- acp_adapter/server.py | 34 ++++++++++++--------- tests/acp/test_approval_isolation.py | 43 ++++++++++++++++++++++++++ tools/approval.py | 45 ++++++++++++++++++++++++++-- 3 files changed, 106 insertions(+), 16 deletions(-) diff --git a/acp_adapter/server.py b/acp_adapter/server.py index a51db91d4e8..df773297346 100644 --- a/acp_adapter/server.py +++ b/acp_adapter/server.py @@ -74,6 +74,10 @@ from acp_adapter.permissions import make_approval_callback from acp_adapter.provenance import session_provenance_meta from acp_adapter.session import SessionManager, SessionState, _expand_acp_enabled_toolsets from acp_adapter.tools import build_tool_complete, build_tool_start +from tools.approval import ( + reset_hermes_interactive_context, + set_hermes_interactive_context, +) logger = logging.getLogger(__name__) @@ -1446,20 +1450,23 @@ class HermesACPAgent(acp.Agent): # Approval callback is per-thread (thread-local, GHSA-qg5c-hvr5-hjgr). # Set it INSIDE _run_agent so the TLS write happens in the executor # thread — setting it here would write to the event-loop thread's TLS, - # not the executor's. Also set HERMES_INTERACTIVE so approval.py - # takes the CLI-interactive path (which calls the registered - # callback via prompt_dangerous_approval) instead of the - # non-interactive auto-approve branch (GHSA-96vc-wcxf-jjff). + # not the executor's. Interactive routing uses a contextvar in + # tools.approval (set_hermes_interactive_context) rather than + # os.environ["HERMES_INTERACTIVE"], so concurrent executor workers can't + # race on a process-global flag — one session's restore can't drop + # another onto the non-interactive auto-approve path mid-run + # (GHSA-96vc-wcxf-jjff). The contextvar write is isolated by the + # contextvars.copy_context() wrapper around the executor call below. # ACP's conn.request_permission maps cleanly to the interactive # callback shape — not the gateway-queue HERMES_EXEC_ASK path, # which requires a notify_cb registered in _gateway_notify_cbs. previous_approval_cb = None - previous_interactive = None + interactive_token = None edit_approval_token = None previous_session_id = None def _run_agent() -> dict: - nonlocal previous_approval_cb, previous_interactive, edit_approval_token, previous_session_id + nonlocal previous_approval_cb, interactive_token, edit_approval_token, previous_session_id # Bind HERMES_SESSION_KEY for this session so per-session caches # (e.g. the interactive sudo password cache in tools.terminal_tool) # scope to the ACP session rather than leaking across sessions @@ -1491,9 +1498,10 @@ class HermesACPAgent(acp.Agent): except Exception: logger.debug("Could not set ACP edit approval requester", exc_info=True) # Signal to tools.approval that we have an interactive callback - # and the non-interactive auto-approve path must not fire. - previous_interactive = os.environ.get("HERMES_INTERACTIVE") - os.environ["HERMES_INTERACTIVE"] = "1" + # and the non-interactive auto-approve path must not fire. Uses a + # contextvar (not os.environ) so concurrent executor workers don't + # race on the flag (GHSA-96vc-wcxf-jjff). + interactive_token = set_hermes_interactive_context(True) # Propagate the originating ACP session id to tools that want to # tag side-effects with it (e.g. ``kanban_create`` stamps it on # the new task so clients can render a per-session board). Save @@ -1513,11 +1521,9 @@ class HermesACPAgent(acp.Agent): logger.exception("Agent error in session %s", session_id) return {"final_response": f"Error: {e}", "messages": state.history} finally: - # Restore HERMES_INTERACTIVE. - if previous_interactive is None: - os.environ.pop("HERMES_INTERACTIVE", None) - else: - os.environ["HERMES_INTERACTIVE"] = previous_interactive + # Restore the interactive contextvar for this context. + if interactive_token is not None: + reset_hermes_interactive_context(interactive_token) # Restore HERMES_SESSION_ID symmetrically. if previous_session_id is None: os.environ.pop("HERMES_SESSION_ID", None) diff --git a/tests/acp/test_approval_isolation.py b/tests/acp/test_approval_isolation.py index e6d3f593f76..30d783f42e1 100644 --- a/tests/acp/test_approval_isolation.py +++ b/tests/acp/test_approval_isolation.py @@ -241,3 +241,46 @@ class TestAcpExecAskGate: "GHSA-96vc-wcxf-jjff" ) assert result["approved"] is True + + def test_interactive_context_var_routes_to_callback_without_env( + self, monkeypatch, + ): + """Context-local interactive flag must work without touching os.environ. + + Concurrent ACP sessions run on a shared ThreadPoolExecutor, so the + interactive flag is now a contextvar instead of a process-global env + var — one session can no longer clobber another's flag mid-run + (GHSA-96vc-wcxf-jjff). + """ + monkeypatch.delenv("HERMES_INTERACTIVE", raising=False) + monkeypatch.delenv("HERMES_GATEWAY_SESSION", raising=False) + monkeypatch.delenv("HERMES_EXEC_ASK", raising=False) + monkeypatch.delenv("HERMES_YOLO_MODE", raising=False) + + from tools.approval import ( + check_all_command_guards, + reset_hermes_interactive_context, + set_hermes_interactive_context, + ) + + called_with = [] + + def fake_cb(command, description, *, allow_permanent=True): + called_with.append((command, description)) + return "once" + + tok = set_hermes_interactive_context(True) + try: + result = check_all_command_guards( + "rm -rf /tmp/test-context-interactive", + "local", + approval_callback=fake_cb, + ) + finally: + reset_hermes_interactive_context(tok) + + assert called_with, ( + "set_hermes_interactive_context(True) should route dangerous " + "commands through the callback without HERMES_INTERACTIVE in env" + ) + assert result["approved"] is True diff --git a/tools/approval.py b/tools/approval.py index e7b59d19c21..ae8c82e1998 100644 --- a/tools/approval.py +++ b/tools/approval.py @@ -48,6 +48,47 @@ _approval_tool_call_id: contextvars.ContextVar[str] = contextvars.ContextVar( default="", ) +# Interactive-CLI flag. Concurrent ACP sessions run on a shared +# ThreadPoolExecutor (acp_adapter/server.py), so mutating the process-global +# os.environ["HERMES_INTERACTIVE"] races: one session's restore in `finally` +# can clobber another session's set mid-run, dropping it onto the +# non-interactive auto-approve path so a dangerous command executes without +# the approval callback firing (GHSA-96vc-wcxf-jjff). A contextvar is +# thread/task-local, so each executor worker (or asyncio task) sees only its +# own value. None = unset → fall back to the env var for legacy +# single-threaded CLI callers that still export HERMES_INTERACTIVE. +_hermes_interactive_ctx: contextvars.ContextVar[Optional[str]] = contextvars.ContextVar( + "hermes_interactive", + default=None, +) + + +def set_hermes_interactive_context(interactive: bool) -> contextvars.Token: + """Bind interactive mode for the current context (thread or asyncio task). + + Use this instead of mutating ``os.environ["HERMES_INTERACTIVE"]`` from + concurrent executor threads. When unset (default), interactive detection + falls back to the ``HERMES_INTERACTIVE`` env var for legacy callers. + """ + return _hermes_interactive_ctx.set("1" if interactive else "") + + +def reset_hermes_interactive_context(token: contextvars.Token) -> None: + """Restore the prior value from :func:`set_hermes_interactive_context`.""" + _hermes_interactive_ctx.reset(token) + + +def _is_interactive_cli() -> bool: + """True when running an interactive CLI/ACP session. + + Prefers the context-local flag (set by concurrent ACP sessions) and falls + back to the ``HERMES_INTERACTIVE`` env var for single-threaded callers. + """ + ctx_val = _hermes_interactive_ctx.get() + if ctx_val is not None: + return is_truthy_value(ctx_val) + return env_var_enabled("HERMES_INTERACTIVE") + def _fire_approval_hook(hook_name: str, **kwargs) -> None: """Invoke a plugin lifecycle hook for the approval system. @@ -1352,7 +1393,7 @@ def check_dangerous_command(command: str, env_type: str, if is_approved(session_key, pattern_key): return {"approved": True, "message": None} - is_cli = env_var_enabled("HERMES_INTERACTIVE") + is_cli = _is_interactive_cli() is_gateway = _is_gateway_approval_context() if not is_cli and not is_gateway: @@ -1612,7 +1653,7 @@ def check_all_command_guards(command: str, env_type: str, if _command_matches_permanent_allowlist(command): return {"approved": True, "message": None} - is_cli = env_var_enabled("HERMES_INTERACTIVE") + is_cli = _is_interactive_cli() is_gateway = _is_gateway_approval_context() is_ask = env_var_enabled("HERMES_EXEC_ASK")