fix(delegate): resolve subagent approval prompts without deadlocking parent TUI (#15491)
Subagents run inside a ThreadPoolExecutor. The CLI's interactive approval callback lives in tools/terminal_tool.py's threading.local(), which worker threads do not inherit. When a subagent hits a dangerous-command guard, prompt_dangerous_approval() falls back to input() from the worker thread, deadlocking against the parent's prompt_toolkit TUI that owns stdin. Fix: install a non-interactive callback into every subagent worker thread via ThreadPoolExecutor(initializer=set_approval_callback, initargs=(cb,)). The callback is config-gated by delegation.subagent_auto_approve: false (default) -> _subagent_auto_deny (safe; matches leaf tool blocklist) true -> _subagent_auto_approve (opt-in YOLO for cron/batch) Both emit a logger.warning audit line. Gateway sessions are unaffected because they resolve approvals via tools/approval.py's per-session queue, not through these TLS callbacks. Diagnosis credit: @MorAlekss (#14685). - hermes_cli/config.py: DEFAULT_CONFIG.delegation.subagent_auto_approve: False - cli-config.yaml.example: documented, commented (default) - tools/delegate_tool.py: _subagent_auto_deny, _subagent_auto_approve, _get_subagent_approval_callback, wired into the child timeout executor - tests/tools/test_delegate.py: 7 tests covering defaults, truthy coercion, and TLS scoping in the worker thread
This commit is contained in:
@@ -796,6 +796,10 @@ delegation:
|
|||||||
# Raise to 2 to allow workers to spawn their own subagents.
|
# Raise to 2 to allow workers to spawn their own subagents.
|
||||||
# Requires role="orchestrator" on intermediate agents.
|
# Requires role="orchestrator" on intermediate agents.
|
||||||
# orchestrator_enabled: true # Kill switch for role="orchestrator" children (default: true).
|
# orchestrator_enabled: true # Kill switch for role="orchestrator" children (default: true).
|
||||||
|
# subagent_auto_approve: false # When a subagent hits a dangerous-command approval prompt, auto-deny (default: false)
|
||||||
|
# or auto-approve "once" (true) instead of blocking on stdin.
|
||||||
|
# The parent TUI owns stdin, so blocking would deadlock; non-interactive resolution is required.
|
||||||
|
# Both choices emit a logger.warning audit line. Flip to true only for cron/batch pipelines.
|
||||||
# inherit_mcp_toolsets: true # When explicit child toolsets are narrowed, also keep the parent's MCP toolsets (default: true). Set false for strict intersection.
|
# inherit_mcp_toolsets: true # When explicit child toolsets are narrowed, also keep the parent's MCP toolsets (default: true). Set false for strict intersection.
|
||||||
# model: "google/gemini-3-flash-preview" # Override model for subagents (empty = inherit parent)
|
# model: "google/gemini-3-flash-preview" # Override model for subagents (empty = inherit parent)
|
||||||
# provider: "openrouter" # Override provider for subagents (empty = inherit parent)
|
# provider: "openrouter" # Override provider for subagents (empty = inherit parent)
|
||||||
|
|||||||
@@ -783,6 +783,15 @@ DEFAULT_CONFIG = {
|
|||||||
# warning log if out of range.
|
# warning log if out of range.
|
||||||
"max_spawn_depth": 1, # depth cap (1 = flat [default], 2 = orchestrator→leaf, 3 = three-level)
|
"max_spawn_depth": 1, # depth cap (1 = flat [default], 2 = orchestrator→leaf, 3 = three-level)
|
||||||
"orchestrator_enabled": True, # kill switch for role="orchestrator"
|
"orchestrator_enabled": True, # kill switch for role="orchestrator"
|
||||||
|
# When a subagent hits a dangerous-command approval prompt, the parent's
|
||||||
|
# prompt_toolkit TUI owns stdin — a thread-local input() call from the
|
||||||
|
# subagent worker would deadlock the parent UI. To avoid the deadlock,
|
||||||
|
# subagent threads ALWAYS resolve approvals non-interactively:
|
||||||
|
# false (default) → auto-deny with a logger.warning audit line (safe)
|
||||||
|
# true → auto-approve "once" with a logger.warning audit line
|
||||||
|
# Flip to true only if you trust delegated work to run dangerous cmds
|
||||||
|
# without human review (cron pipelines, batch automation, etc.).
|
||||||
|
"subagent_auto_approve": False,
|
||||||
},
|
},
|
||||||
|
|
||||||
# Ephemeral prefill messages file — JSON list of {role, content} dicts
|
# Ephemeral prefill messages file — JSON list of {role, content} dicts
|
||||||
|
|||||||
@@ -2128,5 +2128,103 @@ class TestOrchestratorEndToEnd(unittest.TestCase):
|
|||||||
self.assertFalse(built_agents[2]["is_orchestrator_prompt"])
|
self.assertFalse(built_agents[2]["is_orchestrator_prompt"])
|
||||||
|
|
||||||
|
|
||||||
|
class TestSubagentApprovalCallback(unittest.TestCase):
|
||||||
|
"""Subagent worker threads must have a non-interactive approval callback
|
||||||
|
installed so dangerous-command prompts don't fall back to input() and
|
||||||
|
deadlock the parent's prompt_toolkit TUI.
|
||||||
|
|
||||||
|
Governed by delegation.subagent_auto_approve:
|
||||||
|
false (default) → _subagent_auto_deny
|
||||||
|
true → _subagent_auto_approve
|
||||||
|
"""
|
||||||
|
|
||||||
|
def test_auto_deny_returns_deny(self):
|
||||||
|
from tools.delegate_tool import _subagent_auto_deny
|
||||||
|
self.assertEqual(
|
||||||
|
_subagent_auto_deny("rm -rf /tmp/x", "dangerous"),
|
||||||
|
"deny",
|
||||||
|
)
|
||||||
|
|
||||||
|
def test_auto_approve_returns_once(self):
|
||||||
|
from tools.delegate_tool import _subagent_auto_approve
|
||||||
|
self.assertEqual(
|
||||||
|
_subagent_auto_approve("rm -rf /tmp/x", "dangerous"),
|
||||||
|
"once",
|
||||||
|
)
|
||||||
|
|
||||||
|
@patch("tools.delegate_tool._load_config", return_value={})
|
||||||
|
def test_getter_defaults_to_deny(self, _mock_cfg):
|
||||||
|
from tools.delegate_tool import (
|
||||||
|
_get_subagent_approval_callback,
|
||||||
|
_subagent_auto_deny,
|
||||||
|
)
|
||||||
|
self.assertIs(_get_subagent_approval_callback(), _subagent_auto_deny)
|
||||||
|
|
||||||
|
@patch(
|
||||||
|
"tools.delegate_tool._load_config",
|
||||||
|
return_value={"subagent_auto_approve": False},
|
||||||
|
)
|
||||||
|
def test_getter_explicit_false_is_deny(self, _mock_cfg):
|
||||||
|
from tools.delegate_tool import (
|
||||||
|
_get_subagent_approval_callback,
|
||||||
|
_subagent_auto_deny,
|
||||||
|
)
|
||||||
|
self.assertIs(_get_subagent_approval_callback(), _subagent_auto_deny)
|
||||||
|
|
||||||
|
@patch(
|
||||||
|
"tools.delegate_tool._load_config",
|
||||||
|
return_value={"subagent_auto_approve": True},
|
||||||
|
)
|
||||||
|
def test_getter_true_is_approve(self, _mock_cfg):
|
||||||
|
from tools.delegate_tool import (
|
||||||
|
_get_subagent_approval_callback,
|
||||||
|
_subagent_auto_approve,
|
||||||
|
)
|
||||||
|
self.assertIs(_get_subagent_approval_callback(), _subagent_auto_approve)
|
||||||
|
|
||||||
|
@patch(
|
||||||
|
"tools.delegate_tool._load_config",
|
||||||
|
return_value={"subagent_auto_approve": "yes"},
|
||||||
|
)
|
||||||
|
def test_getter_truthy_string_is_approve(self, _mock_cfg):
|
||||||
|
"""is_truthy_value accepts 'yes'/'1'/'true' as truthy."""
|
||||||
|
from tools.delegate_tool import (
|
||||||
|
_get_subagent_approval_callback,
|
||||||
|
_subagent_auto_approve,
|
||||||
|
)
|
||||||
|
self.assertIs(_get_subagent_approval_callback(), _subagent_auto_approve)
|
||||||
|
|
||||||
|
def test_executor_initializer_installs_callback_in_worker(self):
|
||||||
|
"""The initializer sets the callback on the worker thread's TLS,
|
||||||
|
not the parent's — verifies the fix actually scopes to workers.
|
||||||
|
"""
|
||||||
|
from concurrent.futures import ThreadPoolExecutor
|
||||||
|
from tools.terminal_tool import (
|
||||||
|
set_approval_callback as _set_cb,
|
||||||
|
_get_approval_callback,
|
||||||
|
)
|
||||||
|
from tools.delegate_tool import _subagent_auto_deny
|
||||||
|
|
||||||
|
# Parent thread has no callback.
|
||||||
|
_set_cb(None)
|
||||||
|
self.assertIsNone(_get_approval_callback())
|
||||||
|
|
||||||
|
seen = []
|
||||||
|
|
||||||
|
def worker():
|
||||||
|
seen.append(_get_approval_callback())
|
||||||
|
|
||||||
|
with ThreadPoolExecutor(
|
||||||
|
max_workers=1,
|
||||||
|
initializer=_set_cb,
|
||||||
|
initargs=(_subagent_auto_deny,),
|
||||||
|
) as executor:
|
||||||
|
executor.submit(worker).result()
|
||||||
|
|
||||||
|
self.assertEqual(seen, [_subagent_auto_deny])
|
||||||
|
# Parent's callback slot is still empty (TLS isolates threads).
|
||||||
|
self.assertIsNone(_get_approval_callback())
|
||||||
|
|
||||||
|
|
||||||
if __name__ == "__main__":
|
if __name__ == "__main__":
|
||||||
unittest.main()
|
unittest.main()
|
||||||
|
|||||||
@@ -33,6 +33,7 @@ from typing import Any, Dict, List, Optional
|
|||||||
|
|
||||||
from toolsets import TOOLSETS
|
from toolsets import TOOLSETS
|
||||||
from tools import file_state
|
from tools import file_state
|
||||||
|
from tools.terminal_tool import set_approval_callback as _set_subagent_approval_cb
|
||||||
from utils import base_url_hostname, is_truthy_value
|
from utils import base_url_hostname, is_truthy_value
|
||||||
|
|
||||||
|
|
||||||
@@ -47,6 +48,64 @@ DELEGATE_BLOCKED_TOOLS = frozenset(
|
|||||||
]
|
]
|
||||||
)
|
)
|
||||||
|
|
||||||
|
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Subagent approval callbacks
|
||||||
|
# ---------------------------------------------------------------------------
|
||||||
|
# Subagents run inside a ThreadPoolExecutor worker. The CLI's interactive
|
||||||
|
# approval callback is stored in tools/terminal_tool.py's threading.local(),
|
||||||
|
# so worker threads do NOT inherit it. Without a callback,
|
||||||
|
# prompt_dangerous_approval() falls back to input() from the worker thread,
|
||||||
|
# which deadlocks against the parent's prompt_toolkit TUI that owns stdin.
|
||||||
|
#
|
||||||
|
# Fix: install a non-interactive callback into every subagent worker thread
|
||||||
|
# via ThreadPoolExecutor(initializer=_set_subagent_approval_cb, initargs=(cb,)).
|
||||||
|
# The callback is chosen by the `delegation.subagent_auto_approve` config:
|
||||||
|
# false (default) → _subagent_auto_deny (safe; matches leaf tool blocklist)
|
||||||
|
# true → _subagent_auto_approve (opt-in YOLO for cron/batch)
|
||||||
|
# Both emit a logger.warning for audit; gateway sessions are unaffected
|
||||||
|
# because they resolve approvals via tools/approval.py's per-session queue,
|
||||||
|
# not through these TLS callbacks.
|
||||||
|
def _subagent_auto_deny(command: str, description: str, **kwargs) -> str:
|
||||||
|
"""Auto-deny dangerous commands in subagent threads (safe default).
|
||||||
|
|
||||||
|
Returns 'deny' so the subagent sees a refusal it can recover from, and
|
||||||
|
never calls input() (which would deadlock the parent TUI).
|
||||||
|
"""
|
||||||
|
logger.warning(
|
||||||
|
"Subagent auto-denied dangerous command: %s (%s). "
|
||||||
|
"Set delegation.subagent_auto_approve: true to allow.",
|
||||||
|
command, description,
|
||||||
|
)
|
||||||
|
return "deny"
|
||||||
|
|
||||||
|
|
||||||
|
def _subagent_auto_approve(command: str, description: str, **kwargs) -> str:
|
||||||
|
"""Auto-approve dangerous commands in subagent threads (opt-in YOLO).
|
||||||
|
|
||||||
|
Only installed when delegation.subagent_auto_approve=true. Returns 'once'
|
||||||
|
so the subagent proceeds without blocking the parent UI.
|
||||||
|
"""
|
||||||
|
logger.warning(
|
||||||
|
"Subagent auto-approved dangerous command: %s (%s)",
|
||||||
|
command, description,
|
||||||
|
)
|
||||||
|
return "once"
|
||||||
|
|
||||||
|
|
||||||
|
def _get_subagent_approval_callback():
|
||||||
|
"""Return the callback to install into subagent worker threads.
|
||||||
|
|
||||||
|
Config key: delegation.subagent_auto_approve (bool, default False).
|
||||||
|
Reads via the same _load_config() path as the rest of delegate_task so
|
||||||
|
priority is config.yaml > (no env override for this knob) > default.
|
||||||
|
"""
|
||||||
|
cfg = _load_config()
|
||||||
|
val = cfg.get("subagent_auto_approve", False)
|
||||||
|
if is_truthy_value(val):
|
||||||
|
return _subagent_auto_approve
|
||||||
|
return _subagent_auto_deny
|
||||||
|
|
||||||
# Build a description fragment listing toolsets available for subagents.
|
# Build a description fragment listing toolsets available for subagents.
|
||||||
# Excludes toolsets where ALL tools are blocked, composite/platform toolsets
|
# Excludes toolsets where ALL tools are blocked, composite/platform toolsets
|
||||||
# (hermes-* prefixed), and scenario toolsets.
|
# (hermes-* prefixed), and scenario toolsets.
|
||||||
@@ -1344,7 +1403,15 @@ def _run_single_child(
|
|||||||
# Run child with a hard timeout to prevent indefinite blocking
|
# Run child with a hard timeout to prevent indefinite blocking
|
||||||
# when the child's API call or tool-level HTTP request hangs.
|
# when the child's API call or tool-level HTTP request hangs.
|
||||||
child_timeout = _get_child_timeout()
|
child_timeout = _get_child_timeout()
|
||||||
_timeout_executor = ThreadPoolExecutor(max_workers=1)
|
_timeout_executor = ThreadPoolExecutor(
|
||||||
|
max_workers=1,
|
||||||
|
# Install a non-interactive approval callback in the worker thread
|
||||||
|
# so dangerous-command prompts from the subagent don't fall back to
|
||||||
|
# input() and deadlock the parent's prompt_toolkit TUI.
|
||||||
|
# Callback (deny vs approve) is governed by delegation.subagent_auto_approve.
|
||||||
|
initializer=_set_subagent_approval_cb,
|
||||||
|
initargs=(_get_subagent_approval_callback(),),
|
||||||
|
)
|
||||||
# Capture the worker thread so the timeout diagnostic can dump its
|
# Capture the worker thread so the timeout diagnostic can dump its
|
||||||
# Python stack (see #14726 — 0-API-call hangs are opaque without it).
|
# Python stack (see #14726 — 0-API-call hangs are opaque without it).
|
||||||
_worker_thread_holder: Dict[str, Optional[threading.Thread]] = {"t": None}
|
_worker_thread_holder: Dict[str, Optional[threading.Thread]] = {"t": None}
|
||||||
|
|||||||
Reference in New Issue
Block a user