fix(acp): isolate per-session approval callback via ContextVar#13471
Open
fix(acp): isolate per-session approval callback via ContextVar#13471
Conversation
Concurrent ACP sessions in one Hermes process previously shared tools.terminal_tool._approval_callback as a module-global, so session B overwriting the slot could route session A's dangerous-command prompt through B's callback (and vice versa). Within a single OS user this was UX confusion rather than a cross-principal boundary break, but the shared state is genuine concurrency sloppiness worth fixing. Store the callback (and the sibling sudo password callback) in ContextVars. Each asyncio task gets its own copy, so per-session set_approval_callback calls no longer stomp on each other. ACP's prompt handler now wraps loop.run_in_executor in contextvars.copy_context().run so the per-session callback survives the hop into the worker thread — asyncio does not propagate contextvars across the executor boundary on its own, and this was verified empirically. Regression tests reproduce the original primitive (two overlapping sessions, each asserts it observes its own callback) and document the run_in_executor contextvar contract the ACP fix relies on. Reported by @xeloxa in GHSA-qg5c-hvr5-hjgr.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Store the ACP approval callback in a
ContextVarso concurrent sessions in one Hermes process each see their own, instead of sharing a module-global that later-arriving sessions overwrite.Changes
tools/terminal_tool.py:_approval_callback/_sudo_password_callbackare nowContextVars;set_approval_callback/set_sudo_password_callbackstill work unchanged for CLI callers (single context holds a single callback).acp_adapter/server.py: drop the save/restore-global dance; wraploop.run_in_executor(_executor, _run_agent)withcontextvars.copy_context().run(...)so the per-session callback propagates into the worker thread.asynciodoes not copy contextvars into executors on its own — empirically verified.tests/acp/test_concurrent_approval_isolation.py: regression test that reproduces the original primitive (two overlapping "sessions," each asserts it observes its own callback) plus a guard on therun_in_executorcontextvar contract the fix relies on.Context
Reported in GHSA-qg5c-hvr5-hjgr by @xeloxa. Within a single OS user this was UX confusion (a dangerous-command prompt could land in the wrong editor tab) rather than a cross-principal boundary break, but the shared-state primitive is real concurrency sloppiness worth fixing. Same pattern we applied to gateway session vars in
e8034e2f.Validation
tests/acp/test_concurrent_approval_isolation.pytests/acp/ tests/cli/test_cli_approval_ui.py tests/tools/test_approval.pytests/tools/ -k "terminal or approval"