feat(approval): generic config-driven tool-call approval gate - #56811
feat(approval): generic config-driven tool-call approval gate#56811alonre wants to merge 2 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tackling configurable tool-call approvals. Current main does not provide this feature, but this version needs substantial rework before it can enforce the promised workflow.
Problems
tools/tool_gate.py:403documentshermes action approve <id>, but this PR changes no CLI or Kanban-worker integration. Current workers only runhermes ... chat -q "work kanban task <id>"(hermes_cli/kanban_db.py:8175-8202), and no current code consumes the replay marker, so deferred actions cannot be approved and replayed.- The new checks are only in AIAgent dispatch.
tools/code_execution_tool.py:588and:870, plusagent/transports/hermes_tools_mcp_server.py:214, callmodel_tools.handle_function_calldirectly, bypassing the proposed gate. - The replay claim is not atomic:
tools/tool_gate.py:579-599reads then updates status, while the newtools/write_approval.py:211-225has no lock or compare-and-swap. Concurrent replays can execute the same approved action twice.
Suggested changes
- Wire a supported approve/reject surface and deterministic worker replay, with an end-to-end lifecycle test.
- Gate at the common dispatch boundary or cover all direct dispatch callers.
- Implement a cross-process atomic pending-action claim and test concurrent replay.
This is an automated hermes-sweeper review.
| f"expires: {expiry}\n\n" | ||
| f"A tool call is awaiting human approval before it runs:\n\n" | ||
| f" {summary}\n\n" | ||
| f"Approve it with `hermes action approve {pending_id}` (or the " |
There was a problem hiding this comment.
This command is not registered by this PR, and no changed Kanban worker/dispatcher code invokes approve_action or replay_pending_action. As written, a deferred card has no supported path to approval or execution; please wire the public action surface and worker replay before advertising this command.
| # Atomic-ish claim: flip to executing immediately so a concurrent replay | ||
| # bails on the status check above. (File store is single-writer per id; | ||
| # the kanban exec card's idempotency_key is the cross-process guard.) | ||
| claimed = wa.update_pending(SUBSYSTEM, pending_id, {"status": "executing"}) |
There was a problem hiding this comment.
This is not an atomic claim: two processes can both read status == "pending" above, both replace the record with executing, and both reach handle_function_call. Atomic rename only prevents partial-file reads; use a cross-process compare-and-swap/lock and add a simultaneous-replay regression test.
Adds a human-in-the-loop approval gate for designated tool calls (inline blocking or deferred Kanban staging). Default OFF. Addresses review feedback on the original submission: - The gate now lives inside model_tools.handle_function_call itself, the single choke point every dispatch path funnels through (sequential/concurrent agent-loop dispatch, the execute_code sandbox IPC handler, and the MCP tool server all call it to actually run a tool). Previously the gate only ran at the agent-loop dispatch choke points, so a sandboxed script or an MCP client could call a gated tool directly and bypass approval entirely. Callers that already gated at their own choke point pass the new skip_tool_approval_gate=True so a consumed one-shot replay token isn't evaluated a second time. - write_approval.claim_pending() replaces the read-then-write "pending" -> "executing"/"approved" transition with a lock-guarded atomic check-and-set. The prior pattern let two concurrent callers (a button-click racing the kanban dispatcher, or two dispatcher ticks racing each other) both observe the pre-claim status and both proceed, double-executing the approved tool call.
Closes the gap where an approved action's exec card had no code path
that actually replayed it. The dispatcher spawns every task — exec
cards included — as a plain `hermes chat -q "work kanban task <id>"`
subprocess, so an exec card's replay marker was just handed to the
model as prose ("replay the staged tool call"), which is neither
deterministic nor guaranteed to invoke the tool.
tools/tool_gate.maybe_replay_kanban_task() is the new entry point:
given a task id, it reads the task body, and if it carries a replay
marker, calls replay_pending_action() directly and terminates the task
via the same kanban_complete/kanban_block tools a normal worker turn
would call — never handing an exec card to the LLM. cli.py's
single-query worker path calls this before touching the agent loop at
all, short-circuiting exec cards entirely.
0cf7d08 to
c975c6f
Compare
|
Reworked per review — rebased onto current `upstream/main` (the old branch had diverged). All three concerns addressed: 1. No CLI/kanban-worker integration to consume approvals — deferred actions couldn't actually run. Added `tool_gate.maybe_replay_kanban_task(task_id)`: given a task id, it reads the task body and, if it carries the replay marker, calls `replay_pending_action()` directly and terminates the task via the same `kanban_complete`/`kanban_block` tools a normal worker turn would call — never handing the exec card to the model. Wired into `cli.py`'s single-query worker entry point (the same place `HERMES_KANBAN_TASK` is already read for image-ref extraction), short-circuiting before the agent loop starts at all. 2. Gate only checked at the AIAgent dispatch layer — `code_execution_tool.py`, `hermes_tools_mcp_server.py` bypass it via direct `handle_function_call`. 3. Replay claim not atomic — `tool_gate.py:579-599` reads then updates status, `write_approval.py:211-225` has no lock/CAS. Concurrent replays could double-execute. Tests: 3 new unit tests for the CLI/worker wiring, 4 new tests for the dispatch choke point (direct `handle_function_call` calls are gated; the skip flag bypasses correctly), 1 concurrency regression test for the atomic claim. Full `tests/tools/test_tool_gate.py` (42 tests), `tests/run_agent/test_tool_gate_dispatch.py` (3 tests), `tests/tools/test_write_approval.py`, and the full `tests/run_agent/` suite (2329 tests) all pass. |
Summary
Adds a human-in-the-loop approval gate for designated tool calls. Default OFF — with no config the behaviour is unchanged everywhere.
Why
Some deployments need certain tools (e.g.
send_email,delete_*, API write-back tools) to require human sign-off before executing, without blocking every call and without a custom wrapper per tool. The existing dangerous-command engine has all the right primitives (inline ask, session/permanent allowlist, Kanban staging); this wires those to named or glob-matched tool names.Two approval modes
force_deferredpending/actions/<id>.json, opens a Kanban approval card, returns a non-error"staged"result so the agent continuesExecution-on-approval is Hermes-native: approving the card mints an agent-assigned execution card; the dispatcher wakes a worker, which replays the staged call with a one-shot per-pending-id token (consumed on first match so it passes through exactly once; TTL + status state machine guard against double-execution).
Decision ladder in
check_tool_approvalapprovals.mode == off→ allow.pending_id→ allow + consume.Config example
Files
tools/tool_gate.pysummarize_tool_call,approve_action,replay_pending_action, Kanban card helpers. Optional Mattermost notification is no-op unless credentials are set.tools/approval.pycheck_tool_approval+ helpers beforecheck_execute_code_guard.tools/write_approval.pyACTIONSsubsystem +update_pending()atomic helper.agent/tool_executor.pyagent/agent_runtime_helpers.pyinvoke_tool).gateway/run.pykind == "tool"prompts.tests/tools/test_tool_gate.pytests/run_agent/test_tool_gate_dispatch.pyTest plan
tests/tools/test_tool_gate.py— 27 unit tests passtests/run_agent/test_tool_gate_dispatch.py— 3 integration tests pass (sequential + concurrent)upstream/main(not the fork's feature branches)🤖 Generated with Claude Code