KrustyPlanet/hermes-agent
6
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

fix(acp): thread-safe interactive approval via contextvars

Browse source 
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>
This commit is contained in:
georgex8001 2026-06-30 03:12:10 -07:00 • committed by Teknium
parent f5eb4c307b
commit 62b9fb6623
3 changed files with 106 additions and 16 deletions
Download patch file Download diff file Expand all files Collapse all files

34
acp_adapter/server.py
View file

@ -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)

43
tests/acp/test_approval_isolation.py
View file

@ -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

45
tools/approval.py
View file

@ -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")