From d083b85591f493be839c85e29bd1108df89e3dad Mon Sep 17 00:00:00 2001 From: Nikola Hristov Date: Sat, 15 Aug 2026 22:48:57 -0700 Subject: [PATCH] feat(hooks): pre_tool_call content transformation via `modify` directive Adds a `modify` response type to pre_tool_call hooks so a hook can transform tool arguments before the tool executes, instead of repairing results afterwards via post_tool_call. - hermes_cli/plugins.py: _dispatch_pre_tool_call_hooks() fires hooks once and returns (block_message, modified_args); modify directives shallow-merge into an accumulated dict built from the original args. - agent/shell_hooks.py: _parse_response() accepts both the canonical {"action": "modify", "args": {...}} and Claude Code-compatible {"decision": "modify", "tool_input": {...}} wire formats. - model_tools.py, agent/tool_executor.py, agent/agent_runtime_helpers.py: dispatch sites migrated; modified args applied before execution. - Docs + 10 new tests (merge semantics, precedence, block interplay). Salvaged from PR #28953. Best fix for #18988. --- agent/agent_runtime_helpers.py | 10 +- agent/shell_hooks.py | 21 ++++ agent/tool_executor.py | 9 +- docs/observability/README.md | 2 +- hermes_cli/plugins.py | 78 ++++++++++++++- model_tools.py | 16 +-- tests/agent/test_shell_hooks.py | 28 ++++++ tests/hermes_cli/test_plugins.py | 117 ++++++++++++++++++++++ website/docs/user-guide/features/hooks.md | 28 +++++- 9 files changed, 287 insertions(+), 22 deletions(-) diff --git a/agent/agent_runtime_helpers.py b/agent/agent_runtime_helpers.py index e0c51fb353..47a272c055 100644 --- a/agent/agent_runtime_helpers.py +++ b/agent/agent_runtime_helpers.py @@ -2992,17 +2992,17 @@ def invoke_tool(agent, function_name: str, function_args: dict, effective_task_i block_message: Optional[str] = None if not pre_tool_block_checked: try: - from hermes_cli.plugins import resolve_pre_tool_block - block_message = resolve_pre_tool_block( - function_name, - function_args, - task_id=effective_task_id or "", + from hermes_cli.plugins import _dispatch_pre_tool_call_hooks + block_message, modified_args = _dispatch_pre_tool_call_hooks( + function_name, function_args, task_id=effective_task_id or "", session_id=getattr(agent, "session_id", "") or "", tool_call_id=tool_call_id or "", turn_id=getattr(agent, "_current_turn_id", "") or "", api_request_id=getattr(agent, "_current_api_request_id", "") or "", middleware_trace=list(_tool_middleware_trace), ) + if modified_args is not None: + function_args = modified_args except Exception: block_message = None if block_message is not None: diff --git a/agent/shell_hooks.py b/agent/shell_hooks.py index 965bbcd3d0..8751aeb6fd 100644 --- a/agent/shell_hooks.py +++ b/agent/shell_hooks.py @@ -47,6 +47,12 @@ Wire protocol # Inject context for pre_llm_call: {"context": "Today is Friday"} + # Modify tool input for pre_tool_call (Hermes-canonical): + {"action": "modify", "args": {"new_string": "fixed content"}} + + # Modify tool input for pre_tool_call (Claude-Code-style): + {"decision": "modify", "tool_input": {"new_string": "fixed content"}} + # Silent no-op: @@ -774,6 +780,12 @@ def _parse_response(event: str, stdout: str) -> Optional[Dict[str, Any]]: skipping the translation silently breaks every ``pre_tool_call`` block directive. + For ``pre_tool_call`` the ``modify`` action (canonical: ``{"action": + "modify", "args": {...}}``, Claude-Code-style: ``{"decision": + "modify", "tool_input": {...}}``) is translated to + ``{"action": "modify", "args": {...}}`` so callers can merge the + returned fields into the tool's ``args`` before dispatch. + For ``pre_llm_call``, ``{"context": "..."}`` is passed through unchanged to match the existing plugin-hook contract. @@ -800,6 +812,15 @@ def _parse_response(event: str, stdout: str) -> Optional[Dict[str, Any]]: return {"action": "block", "message": _block_message(data.get("message"), data.get("reason"))} if data.get("decision") == "block": return {"action": "block", "message": _block_message(data.get("reason"), data.get("message"))} + # "modify" action — transform tool_input before dispatch + if data.get("action") == "modify": + new_args = data.get("args") + if isinstance(new_args, dict): + return {"action": "modify", "args": new_args} + if data.get("decision") == "modify": + new_args = data.get("tool_input") + if isinstance(new_args, dict): + return {"action": "modify", "args": new_args} return None if event == "pre_verify": diff --git a/agent/tool_executor.py b/agent/tool_executor.py index bdf4efc235..71a43f195a 100644 --- a/agent/tool_executor.py +++ b/agent/tool_executor.py @@ -590,10 +590,11 @@ def _run_agent_tool_execution_middleware( block_error_type = "plugin_block" def _resolve_pre_tool_block(): + nonlocal final_args try: - from hermes_cli.plugins import resolve_pre_tool_block + from hermes_cli.plugins import _dispatch_pre_tool_call_hooks - return resolve_pre_tool_block( + block_msg, modified_args = _dispatch_pre_tool_call_hooks( function_name, final_args, task_id=effective_task_id or "", @@ -604,6 +605,10 @@ def _run_agent_tool_execution_middleware( or "", middleware_trace=list(state["middleware_trace"]), ) + if modified_args is not None: + final_args = modified_args + state["args"] = modified_args + return block_msg except Exception: return None diff --git a/docs/observability/README.md b/docs/observability/README.md index 14847f7496..915b0d5e2a 100644 --- a/docs/observability/README.md +++ b/docs/observability/README.md @@ -55,7 +55,7 @@ behavior-affecting hooks: | Hook | Return behavior | | --- | --- | | `pre_llm_call` | May return a string or `{"context": "..."}` to inject ephemeral context into the current user message. | -| `pre_tool_call` | May return `{"action": "block", "message": "..."}` to block a tool before execution. | +| `pre_tool_call` | May return `{"action": "block", "message": "..."}` to block a tool before execution, or `{"action": "modify", "args": {...}}` to transform the tool's input arguments. | | `transform_tool_result` | May return a replacement tool result string after `post_tool_call`. | | `transform_llm_output` | May return a replacement final assistant text string. | diff --git a/hermes_cli/plugins.py b/hermes_cli/plugins.py index ac1246e810..89da6db541 100644 --- a/hermes_cli/plugins.py +++ b/hermes_cli/plugins.py @@ -51,7 +51,7 @@ from contextlib import contextmanager from dataclasses import dataclass, field from functools import wraps from pathlib import Path -from typing import Any, Callable, Dict, List, Mapping, Optional, Set, Union +from typing import (Any, Callable, Dict, Iterable, List, Mapping, Optional, Set, Tuple, Type, Union) from hermes_constants import ( get_hermes_home, @@ -5950,6 +5950,7 @@ class _PreToolCallDirective: action: Optional[str] = None message: Optional[str] = None rule_key: Optional[str] = None + modified_args: Optional[Dict[str, Any]] = None def set_thread_tool_whitelist( @@ -6022,9 +6023,24 @@ def _get_pre_tool_call_directive_details( middleware_trace=list(middleware_trace or []), ) + block_msg: Optional[str] = None + modified_args: Optional[Dict[str, Any]] = None + for result in hook_results: if not isinstance(result, dict): continue + # "modify" action — transform tool_input before dispatch. + # Processed before the block/approve gate so modify directives + # are visible even when a later hook blocks. Hooks accumulate: + # each modify directive shallow-merges its keys into one + # accumulated dict built from the original args on first hit. + if result.get("action") == "modify": + partial = result.get("args") + if isinstance(partial, dict) and partial: + if modified_args is None: + modified_args = dict(args) if isinstance(args, dict) else {} + modified_args.update(partial) + continue action = result.get("action") if action not in ("block", "approve"): continue @@ -6038,9 +6054,12 @@ def _get_pre_tool_call_directive_details( rule_key = rule_key.strip() if isinstance(rule_key, str) else None if not rule_key: rule_key = None - return _PreToolCallDirective(action=action, message=message, rule_key=rule_key) + return _PreToolCallDirective( + action=action, message=message, rule_key=rule_key, + modified_args=modified_args, + ) - return _PreToolCallDirective() + return _PreToolCallDirective(modified_args=modified_args) def get_pre_tool_call_directive( @@ -6165,6 +6184,59 @@ def resolve_pre_tool_block( return None +def _dispatch_pre_tool_call_hooks( + tool_name: str, + args: Optional[Dict[str, Any]], + task_id: str = "", + session_id: str = "", + tool_call_id: str = "", + turn_id: str = "", + api_request_id: str = "", + middleware_trace: Optional[List[Dict[str, Any]]] = None, +) -> Tuple[Optional[str], Optional[Dict[str, Any]]]: + """Invoke ``pre_tool_call`` hooks once and process all response types. + + Returns a ``(block_message, modified_args)`` tuple: + - ``block_message`` — the first block/approve directive's resolved message + (or ``None`` when the call may proceed). Uses the same approval-gate + logic as :func:`resolve_pre_tool_block`. + - ``modified_args`` — merged args from the first ``modify`` directive + (or ``None`` when no hook requested modification). + + This is the single invocation point for ``pre_tool_call`` hooks. + Callers that only need block detection should keep using + :func:`get_pre_tool_call_block_message` or + :func:`resolve_pre_tool_block` for backward compat. + Callers that also need input transformation should call this + function and apply ``modified_args`` if not ``None``. + """ + details = _get_pre_tool_call_directive_details( + tool_name, args, task_id=task_id, session_id=session_id, + tool_call_id=tool_call_id, turn_id=turn_id, + api_request_id=api_request_id, middleware_trace=middleware_trace, + ) + block_msg: Optional[str] = None + if details.action == "block": + block_msg = details.message + elif details.action == "approve": + try: + from tools.approval import request_tool_approval + result = request_tool_approval( + tool_name, + details.message or "", + rule_key=details.rule_key or tool_name, + ) + except Exception: + block_msg = f"BLOCKED: plugin approval gate failed for {tool_name}" + else: + if not result.get("approved"): + block_msg = str( + result.get("message") + or f"BLOCKED: plugin approval required for {tool_name}" + ) + return (block_msg, details.modified_args) + + def get_pre_verify_continue_message( *, session_id: str = "", diff --git a/model_tools.py b/model_tools.py index 241c862fae..0a5216bbb8 100644 --- a/model_tools.py +++ b/model_tools.py @@ -1370,22 +1370,22 @@ def handle_function_call( if function_name in _AGENT_LOOP_TOOLS: return tool_error(f"{function_name} must be handled by the agent loop") - # Check plugin hooks for a block/approve directive (unless caller + # Check plugin hooks for a block/approve/modify directive (unless caller # already checked — e.g. run_agent._invoke_tool passes skip=True to # avoid double-firing the hook). # # Single-fire contract: pre_tool_call fires exactly once per tool - # execution. resolve_pre_tool_block() internally calls - # invoke_hook("pre_tool_call", ...) once and returns the block message - # for a `block` directive OR for an `approve` directive whose human - # gate denied/timed-out/errored (fail-closed). Observer plugins see + # execution. _dispatch_pre_tool_call_hooks() internally calls + # invoke_hook("pre_tool_call", ...) once and returns both the block + # message (for `block`/`approve` directives) and any modified args + # (for `modify` directives). Observer plugins see # the hook on that same pass. When skip=True, the caller already # fired it — do nothing here. if not skip_pre_tool_call_hook: block_message: Optional[str] = None try: - from hermes_cli.plugins import resolve_pre_tool_block - block_message = resolve_pre_tool_block( + from hermes_cli.plugins import _dispatch_pre_tool_call_hooks + block_message, modified_args = _dispatch_pre_tool_call_hooks( function_name, function_args, task_id=task_id or "", @@ -1395,6 +1395,8 @@ def handle_function_call( api_request_id=api_request_id or "", middleware_trace=list(_tool_middleware_trace), ) + if modified_args is not None: + function_args = modified_args except Exception as _hook_err: logger.debug("pre_tool_call hook error: %s", _hook_err) diff --git a/tests/agent/test_shell_hooks.py b/tests/agent/test_shell_hooks.py index 95934ef2e7..ef7c45ebf7 100644 --- a/tests/agent/test_shell_hooks.py +++ b/tests/agent/test_shell_hooks.py @@ -251,6 +251,34 @@ class TestCallbackSubprocess: + def test_modify_canonical_parsing(self, tmp_path): + """Shell hook returning canonical modify is parsed correctly.""" + script = _write_script( + tmp_path, "mod_canon.sh", + "#!/usr/bin/env bash\n" + 'printf \'{"action": "modify", "args": {"path": "/safe"}}\\n\'', + ) + spec = shell_hooks.ShellHookSpec( + event="pre_tool_call", command=str(script), + ) + cb = shell_hooks._make_callback(spec) + result = cb(tool_name="write_file", args={"path": "/unsafe"}) + assert result == {"action": "modify", "args": {"path": "/safe"}} + + def test_modify_claude_code_parsing(self, tmp_path): + """Shell hook returning Claude-Code modify is normalised.""" + script = _write_script( + tmp_path, "mod_cc.sh", + "#!/usr/bin/env bash\n" + 'printf \'{"decision": "modify", "tool_input": {"content": "safe"}}\\n\'', + ) + spec = shell_hooks.ShellHookSpec( + event="pre_tool_call", command=str(script), + ) + cb = shell_hooks._make_callback(spec) + result = cb(tool_name="write_file", args={"content": "danger"}) + assert result == {"action": "modify", "args": {"content": "safe"}} + # ── config parsing ──────────────────────────────────────────────────────── diff --git a/tests/hermes_cli/test_plugins.py b/tests/hermes_cli/test_plugins.py index 608cbc4b21..c86fd73ed5 100644 --- a/tests/hermes_cli/test_plugins.py +++ b/tests/hermes_cli/test_plugins.py @@ -16,6 +16,7 @@ from hermes_cli.plugins import ( PluginContext, PluginManager, PluginManifest, + _dispatch_pre_tool_call_hooks, get_plugin_command_handler, get_plugin_commands, get_pre_tool_call_block_message, @@ -1122,6 +1123,122 @@ class TestResolvePreToolBlock: assert msg is not None and "gate failed" in msg # fail-closed +class TestPreToolCallModify: + """Tests for the modify action — transforming tool args before dispatch.""" + + def test_modify_returns_merged_args(self, monkeypatch): + """A single modify hook should return merged args.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": {"path": "/safe/dir"}} + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "write_file", {"path": "/unsafe/dir", "content": "x"} + ) + assert block_msg is None + assert modified == {"path": "/safe/dir", "content": "x"} + + def test_modify_accumulates_multiple_hooks(self, monkeypatch): + """Multiple modify hooks should accumulate — hook A + hook B both survive.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": {"path": "/safe"}}, + {"action": "modify", "args": {"content": "fixed"}}, + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "write_file", {"path": "/unsafe", "content": "original"} + ) + assert block_msg is None + assert modified == {"path": "/safe", "content": "fixed"} + + def test_modify_last_wins_on_same_key(self, monkeypatch): + """When two hooks modify the same key, the later hook wins.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": {"path": "/first"}}, + {"action": "modify", "args": {"path": "/second"}}, + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "write_file", {"path": "/original"} + ) + assert modified == {"path": "/second"} + + def test_modify_with_block_returns_both(self, monkeypatch): + """When a modify precedes a block, both are returned.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": {"path": "/safe"}}, + {"action": "block", "message": "still blocked"}, + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "write_file", {"path": "/unsafe"} + ) + assert block_msg == "still blocked" + assert modified == {"path": "/safe"} + + def test_modify_after_block_is_invisible(self, monkeypatch): + """A modify after a block is never reached — first block wins.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "block", "message": "stopped"}, + {"action": "modify", "args": {"path": "/invisible"}}, + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "write_file", {"path": "/original"} + ) + assert block_msg == "stopped" + assert modified is None + + def test_modify_with_none_args(self, monkeypatch): + """Modify should handle None args gracefully.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": {"path": "/safe"}} + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks("write_file", None) + assert block_msg is None + assert modified == {"path": "/safe"} + + def test_modify_none_when_no_hooks(self, monkeypatch): + """No hooks → both return values are None.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "terminal", {"cmd": "ls"} + ) + assert block_msg is None + assert modified is None + + def test_modify_invalid_args_ignored(self, monkeypatch): + """Non-dict args and empty dicts should be silently ignored.""" + monkeypatch.setattr( + "hermes_cli.plugins.invoke_hook", + lambda hook_name, **kwargs: [ + {"action": "modify", "args": "not a dict"}, + {"action": "modify", "args": {}}, # empty + {"action": "modify", "args": {"path": "/real"}}, + ], + ) + block_msg, modified = _dispatch_pre_tool_call_hooks( + "write_file", {"path": "/original"} + ) + assert modified == {"path": "/real"} + + class TestGetPreVerifyContinueMessage: """`pre_verify` directive aggregation — mirrors the pre_tool_call block path.""" diff --git a/website/docs/user-guide/features/hooks.md b/website/docs/user-guide/features/hooks.md index f52aec8030..ace2d2998c 100644 --- a/website/docs/user-guide/features/hooks.md +++ b/website/docs/user-guide/features/hooks.md @@ -437,7 +437,7 @@ Payload fields below are the exact event-specific fields supplied by each call s | Hook | Category | Exact timing and return behavior | Explicit payload fields | Privacy / sensitivity | |---|---|---|---|---| -| `pre_tool_call` | Directive/control | Once before execution; first valid `block` or `approve` directive wins. | `tool_name`, `args`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `middleware_trace` | Raw arguments may contain user content, paths, commands, or secrets. | +| [`pre_tool_call`](#pre_tool_call) | Directive/control | Once before execution; first valid `block` or `approve` directive wins, and `modify` returns are shallow-merged into the tool arguments. | `tool_name`, `args`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `middleware_trace` | Raw arguments may contain user content, paths, commands, or secrets. | | `post_tool_call` | Observer | After blocked, error, or successful result; return ignored. | `tool_name`, `args`, `result`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `duration_ms`, `status`, `error_type`, `error_message`, `middleware_trace` | Result/error text may contain arbitrary tool or user content and secrets. | | `transform_tool_result` | Transform | After `post_tool_call`, before conversation append; first string replaces the result. | `tool_name`, `args`, `result`, `task_id`, `session_id`, `tool_call_id`, `turn_id`, `api_request_id`, `duration_ms`, `status`, `error_type`, `error_message` | Exposes the full model-bound result and arguments. | | `transform_terminal_output` | Transform | After bounded foreground process capture, before final output limiting; first string replaces output. | `command`, `output`, `returncode`, `task_id`, `env_type` | Command/output may contain credentials. | @@ -551,9 +551,25 @@ return {"action": "block", "message": "Reason the tool call was blocked"} return {"action": "approve", "message": "Why approval is required", "rule_key": "optional:scope"} ``` -The first valid directive wins. `block` requires a non-empty `message` and short-circuits the tool with that text as the error returned to the model. `approve` escalates the call to the existing human-approval gate; `message` and `rule_key` are optional, and denial, timeout, or gate error fails closed. Other return values are ignored. +The first valid directive wins (Python plugins registered first, then shell hooks). `block` requires a non-empty `message` and short-circuits the tool with that text as the error returned to the model. `approve` escalates the call to the existing human-approval gate; `message` and `rule_key` are optional, and denial, timeout, or gate error fails closed. Other return values are ignored, so existing observer-only callbacks keep working unchanged. -**Use cases:** Logging, audit trails, tool call counters, blocking dangerous operations, rate limiting, per-user policy enforcement. +**Return value — rewrite the tool's arguments:** + +```python +return {"action": "modify", "args": {"new_string": "fixed content"}} +``` + +The returned `args` dictionary is shallow-merged over the original tool arguments before the tool executes. Multiple `modify` hooks accumulate — each hook's keys are merged into one accumulated dict built from the original args, so hook A changing `path` and hook B changing `content` both survive. If two hooks modify the same key, the later hook wins. + +Shell hooks also accept the Claude Code-compatible format: + +```json +{"decision": "modify", "tool_input": {"new_string": "fixed content"}} +``` + +Both formats are normalized internally to `{"action": "modify", "args": {...}}`. + +**Use cases:** Logging, audit trails, tool call counters, blocking dangerous operations, rate limiting, per-user policy enforcement, argument sanitization, path rewriting, injecting default parameters. **Example — tool call audit log:** @@ -1562,7 +1578,7 @@ Declare shell-script hooks in your `~/.hermes/config.yaml` and Hermes will run t Use shell hooks when you want a drop-in, single-file script (Bash, Python, anything with a shebang) to: -- **Block a tool call** — reject dangerous `terminal` commands, enforce per-directory policies, require approval for destructive `write_file` / `patch` operations. +- **Block or modify a tool call** — reject dangerous `terminal` commands, enforce per-directory policies, require approval for destructive `write_file` / `patch` operations, or rewrite arguments (sanitize paths, inject defaults) before the tool runs. - **Run after a tool call** — auto-format Python or TypeScript files that the agent just wrote, log API calls, trigger a CI workflow. - **Inject context into the next LLM turn** — prepend `git status` output, the current weekday, or retrieved documents to the user message (see [`pre_llm_call`](#pre_llm_call)). - **Observe lifecycle events** — write a log line when a subagent completes (`subagent_stop`) or a session starts (`on_session_start`). @@ -1625,6 +1641,10 @@ Each time the event fires, Hermes spawns a subprocess for every matching hook (m {"decision": "block", "reason": "Forbidden: rm -rf"} // Claude-Code style {"action": "block", "message": "Forbidden: rm -rf"} // Hermes-canonical +// Modify a pre_tool_call — rewrite tool args before dispatch: +{"action": "modify", "args": {"new_string": "fixed content"}} // Hermes-canonical +{"decision": "modify", "tool_input": {"new_string": "fixed content"}} // Claude-Code style + // Inject context for pre_llm_call: {"context": "Today is Friday, 2026-04-17"}