fix(hooks): shell pre_tool_call hooks can escalate to the approval gate with "approve"
`agent/shell_hooks.py::_parse_pre_tool_call` translated only the block and
modify dialects, so a shell hook printing the documented
`{"action": "approve", ...}` parsed to None and the tool ran with no approval
prompt — silently, with exit 0, valid JSON and `hermes hooks doctor` green.
The Python-plugin side already accepts approve and routes it through
`_resolve_block_from_details` → `request_tool_approval`; the shell parser now
yields the same `{"action": "approve", "message"?, "rule_key"?}` shape (optional
fields kept only as non-empty stripped strings), so `hermes hooks test` prints
it under `parsed:` and the dispatcher escalates it. the `decision` dialect's
`{"decision": "approve"}` means auto-ALLOW, not "ask a human", so it is
deliberately not mapped; that dialect has no top-level ask dialect to mirror.
Slim redo with credit: #92562 (earliest) bundled a larger policy-authority
rework; #110325 carried the same parser change plus an unrelated rule_key
default change and 10+ tests.
Fixes #92553
Supersedes #92562
Supersedes #110325
Co-authored-by: fangliquanflq <fangliquan@qq.com>
This commit is contained in:
@@ -445,6 +445,15 @@ def _parse_pre_tool_call(data: Dict[str, Any]) -> Optional[Dict[str, Any]]:
|
||||
for verb, _, _, payload in _PRE_TOOL_DIALECTS:
|
||||
if data.get(verb) == "modify" and isinstance(data.get(payload), dict):
|
||||
return {"action": "modify", "args": data[payload]}
|
||||
# Hermes-only escalation to the human-approval gate (#92553). Claude-Code's ``decision:
|
||||
# approve`` means auto-ALLOW, so it is deliberately not mapped onto this.
|
||||
if data.get("action") == "approve":
|
||||
directive: Dict[str, Any] = {"action": "approve"}
|
||||
for key in ("message", "rule_key"):
|
||||
value = data.get(key)
|
||||
if isinstance(value, str) and value.strip():
|
||||
directive[key] = value.strip()
|
||||
return directive
|
||||
return None
|
||||
|
||||
|
||||
|
||||
@@ -49,7 +49,18 @@ class TestParseResponse:
|
||||
)
|
||||
assert r == {"action": "block", "message": "nope"}
|
||||
|
||||
|
||||
@pytest.mark.parametrize("stdout, expected", [
|
||||
('{"action": "approve", "message": " needs a human ", "rule_key": " terminal:rm "}',
|
||||
{"action": "approve", "message": "needs a human", "rule_key": "terminal:rm"}),
|
||||
('{"action": "approve", "message": "", "rule_key": 7}', {"action": "approve"}),
|
||||
# Claude-Code's ``decision: approve`` means auto-ALLOW, not "ask a human": never mapped.
|
||||
('{"decision": "approve", "reason": "ok"}', None),
|
||||
('{"action": "approve", "decision": "block", "reason": "no"}', {"action": "block", "message": "no"}),
|
||||
])
|
||||
def test_approve_is_parsed_like_the_plugin_directive(self, stdout, expected):
|
||||
"""The documented ``approve`` action used to parse to None, so the tool ran with no
|
||||
approval prompt (#92553). It now yields the same shape Python plugins return."""
|
||||
assert shell_hooks._parse_response("pre_tool_call", stdout) == expected
|
||||
|
||||
def test_empty_stdout_returns_none(self):
|
||||
assert shell_hooks._parse_response("pre_tool_call", "") is None
|
||||
@@ -201,6 +212,32 @@ class TestCallbackSubprocess:
|
||||
)
|
||||
assert msg == "blocked-by-shell"
|
||||
|
||||
def test_approve_reaches_the_human_gate_through_plugin_manager(self, tmp_path, monkeypatch):
|
||||
"""End to end: a shell hook's approve directive escalates to request_tool_approval with its
|
||||
message and rule_key, and the gate's denial blocks the tool (#92553)."""
|
||||
from hermes_cli import plugins
|
||||
|
||||
script = _write_script(
|
||||
tmp_path, "approve.sh",
|
||||
"#!/usr/bin/env bash\n"
|
||||
'printf \'{"action": "approve", "message": "risky", "rule_key": "terminal:rm"}\\n\'\n',
|
||||
)
|
||||
monkeypatch.setenv("HERMES_HOME", str(tmp_path / "home"))
|
||||
monkeypatch.setenv("HERMES_ACCEPT_HOOKS", "1")
|
||||
plugins._plugin_manager = plugins.PluginManager()
|
||||
cfg = {"hooks": {"pre_tool_call": [{"matcher": "terminal", "command": str(script)}]}}
|
||||
assert len(shell_hooks.register_from_config(cfg, accept_hooks=True)) == 1
|
||||
|
||||
seen = []
|
||||
|
||||
def _gate(tool_name, reason, **kwargs):
|
||||
seen.append((tool_name, reason, kwargs.get("rule_key")))
|
||||
return {"approved": False, "message": "denied by human"}
|
||||
|
||||
monkeypatch.setattr("tools.approval.request_tool_approval", _gate)
|
||||
assert plugins.resolve_pre_tool_block("terminal", {"command": "rm"}) == "denied by human"
|
||||
assert seen == [("terminal", "risky", "terminal:rm")]
|
||||
|
||||
def test_matcher_regex_filters_callback(self, tmp_path, monkeypatch):
|
||||
"""A matcher set to 'terminal' must not fire for 'web_search'."""
|
||||
calls = tmp_path / "calls.log"
|
||||
|
||||
@@ -1729,6 +1729,10 @@ profile's `HERMES_HOME`. `tool_name` and `tool_input` are `null` for non-tool ev
|
||||
{"action": "modify", "args": {"new_string": "fixed content"}} // Hermes-canonical
|
||||
{"decision": "modify", "tool_input": {"new_string": "fixed content"}} // Claude-Code style
|
||||
|
||||
// Escalate a pre_tool_call to the human-approval gate (Hermes-only; `message` and `rule_key`
|
||||
// are optional). Claude-Code's `{"decision": "approve"}` means auto-allow and is NOT mapped here:
|
||||
{"action": "approve", "message": "Why approval is required", "rule_key": "optional:scope"}
|
||||
|
||||
// Inject context for pre_llm_call:
|
||||
{"context": "Today is Friday, 2026-04-17"}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user