fix(hooks): stop shell hook approval requests from bypassing confirmation

[fangliquanflq]

What does this PR do?

Shell pre_tool_call hooks can now require human confirmation with the documented {"action":"approve"} response instead of silently allowing the tool call. Unsupported directive values are reported and honor fail_closed, and hermes hooks doctor now rejects syntactically valid JSON that the runtime cannot honor.

Symptom

A configured shell hook that returns {"action":"approve"} is reported as healthy by hermes hooks doctor, but its response is discarded and the tool executes without an approval prompt.

Impact

Operators relying on shell hooks as approval policy can have protected tool calls proceed without the requested confirmation. The affected population is limited to users with pre_tool_call shell hooks that emit approval directives.

Bug Cause

Trigger: agent/shell_hooks.py / _parse_response() when a pre_tool_call hook returns an approval directive.

Causal chain:

1. A shell hook emits valid JSON with action set to approve.
2. _parse_response() recognizes only block and modify, so it returns no directive.
3. The existing plugin approval dispatcher receives nothing and lets the tool call proceed.

Why it is wrong: The shell bridge silently drops a documented control directive even though the downstream plugin hook path already supports approval escalation.

Working sibling / contrast: Python pre_tool_call hooks returning the same canonical approval shape already reach resolve_pre_tool_block() and the shared approval gate. Shell block and modify responses are also normalized correctly.

Ruled out: The approval gate itself is not bypassing the request. An end-to-end regression test proves that once the shell response reaches the existing directive dispatcher, request_tool_approval() receives the expected tool, reason, and rule key.

Fix

The shell response parser now preserves canonical approval directives and their optional message and rule_key. A shared semantic validator reports malformed or unsupported directives, lets valid no-op objects remain no-ops, blocks invalid directives when fail_closed is enabled, and powers the doctor smoke test so runtime and diagnostics agree.

Related Issue

Fixes #92553

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Changes Made

• agent/shell_hooks.py - parse shell approval directives and centralize semantic response validation.
• hermes_cli/hooks.py - make hooks doctor reject unsupported directive JSON.
• tests/agent/test_shell_hooks.py - cover parsing, approval escalation, and fail-closed behavior.
• tests/hermes_cli/test_hooks_cli.py - cover doctor diagnostics for unsupported directives.
• website/docs/user-guide/features/hooks.md - document the shell approval response and unsupported-directive failure semantics.

How to Test

1. Configure an allowlisted pre_tool_call shell hook that prints {"action":"approve","message":"confirm"} and verify the normal approval gate is invoked.
2. Configure a hook that prints {"action":"permit"} and verify hermes hooks doctor reports an unsupported response; with fail_closed: true, verify the runtime blocks the call.
3. Run the focused regression suites:

scripts/run_tests.sh tests/agent/test_shell_hooks.py -k 'ParseResponse or EvaluateResult or approve_escalates_through_plugin_manager' -q
scripts/run_tests.sh tests/hermes_cli/test_hooks_cli.py -k unsupported_pre_tool_call -q
scripts/run_tests.sh tests/hermes_cli/test_plugins.py -k 'PreToolCall or ResolvePreToolBlock' -q

All 35 selected tests pass on Windows 11.

Checklist

Code

• I've read the Contributing Guide
• My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
• I searched for existing PRs to make sure this isn't a duplicate
• My PR contains only changes related to this fix
• I've run the repo test entry on relevant tests and all selected tests pass
• I've added tests for my changes
• I've tested on my platform: Windows 11

Documentation & Housekeeping

• I've updated relevant documentation and docstrings
• cli-config.yaml.example update is not applicable because no config key changed
• CONTRIBUTING.md and AGENTS.md updates are not applicable because no architecture or workflow changed
• I've considered cross-platform impact and used Python subprocess fixtures for the new end-to-end tests
• Tool description/schema updates are not applicable because no model tool contract changed

Screenshots / Logs

Not applicable. The focused automated regression suites above exercise the shell response bridge, approval escalation, fail-closed behavior, and doctor diagnostic path.

[andrexibiza]

Blocking review (GitHub will not permit this identity to set REQUEST_CHANGES without explicit repository review access).

The shell bridge itself is now doing the right thing: the exact approve shape from #92553 survives parsing, malformed/unsupported pre_tool_call directives become visible and honor fail_closed, doctor and runtime share one semantic validator, and the one-hook production path reaches the existing human gate with the supplied reason/rule key. Exact-head CI/Docker/Nix are all green.

There is still one authority blocker before this should become the documented shell-hook contract, though: the shared pre-tool aggregator lets an earlier approve erase a later hard block.

agent.shell_hooks.register_from_config() appends configured shell callbacks to manager._hooks[event] in config order. The lifecycle dispatcher runs the callbacks and returns their results in that order. _get_pre_tool_call_directive_details() then walks those results and immediately returns on the first valid block or approve directive. That means this deterministic policy stack is unsafe:

1. shell hook A returns {"action":"approve","message":"confirm sensitive write","rule_key":"write_file:ssh"};
2. later shell hook B returns {"action":"block","message":"this exact target is forbidden"};
3. the aggregator returns A's approve and discards B's block result;
4. _resolve_block_from_details() invokes request_tool_approval(); a once decision — or an already-cached session/permanent grant for A's rule key — returns None;
5. the tool executes even though another configured policy produced an unconditional veto.

The callbacks all ran, so this is not an execution-order theory; it is the result-selection logic in hermes_cli/plugins.py. approve is an additive request for consent, not authority to cancel an independent deny. Current check_all_command_guards() already uses the correct repository-wide shape for this class: gather every independent finding once, then decide, with a hard block retaining dominance and approval keys remaining independently authoritative.

Please repair the shared aggregator rather than special-casing shell hooks:

• invoke every pre_tool_call hook exactly once and preserve #87482's accumulated modify behavior;
• gather all valid control directives before resolving them;
• if any valid block exists, block regardless of registration order or any cached/interactive approval;
• otherwise aggregate the approval reasons and rule keys into one explicit request whose persistence records every approved key, or require independent decisions. A grant for one rule must not satisfy another rule;
• add deterministic regressions for approve → block, cached-approved approve → block, and two independent approve keys, alongside the existing single-invocation/modify tests.

This is materially the same authority lesson as the terminal gather-then-decide machinery: status as “approved” cannot collapse independent policy findings. #92562 expands the existing Python-plugin surface into operator-authored shell policy, so leaving first-valid-wins here turns the newly documented feature into a way for hook ordering to weaken policy.

Topology/provenance:

• #92553 by @hubbadubbadubdab owns the concrete shell-parser/doctor bug; this PR by @fangliquanflq is the correct owner of that bridge repair.
• Merged #60504 re-landed plugin approval authority while explicitly preserving @kshitijk4poor's #58698 and @doncazper's #59163 rule-key work. Those are antecedent architecture, not duplicates. In fact, the later-block bypass defeats #59163's invariant that one approval key must not authorize a different rule.
• Merged #87482 preserves @NikolaRHristov's #28953 (and credits @elasticdotventures' #19305) for single-fire modify aggregation. The repair here must compose with that implementation rather than re-invoke hooks or discard accumulated modifications.

Exact object reviewed: head 944b9d340b08273d10b68105f7afbaad88de51aa, merge-base/PR base 987064caa4f8845f605ac7346fed5b72fddfb21c, current main 530028c213ae9eed5d7f1a826451e0edf24a11d2. The branch is 1 commit ahead / 4 behind current main; those four main commits touch the gateway control-socket/update lane rather than these hook files. Hosted receipts are genuinely green: CI 32605161706, Docker 32605161110, Nix 32605161158. They verify the submitted object, but the present tests cover only one approval hook and therefore do not exercise the cross-hook veto boundary above.

[fangliquanflq]

Addressed the authority issue in d82e616.

• pre_tool_call results are now gathered before resolution, so any valid hard block dominates approvals regardless of registration order or cached grants.
• Every approval directive is resolved independently with its own reason and rule_key; a grant for one rule cannot authorize another.
• All hooks still run exactly once, and modify results continue to accumulate even when they appear after a block.
• Added deterministic regressions for approve-before-block, cached approval before a later block, two independent approval keys, and post-block modification accumulation.

Verification: 68/68 tests passed in tests/hermes_cli/test_plugins.py; focused shell parsing/evaluation tests passed (19), the unsupported-directive doctor test passed (1), focused pre-tool aggregation/resolution tests passed (18), and Ruff passed on the changed implementation and tests. The broader Windows run also confirmed the plugin suite is green; its 11 shell-script failures are the existing native-Windows .sh execution limitation (WinError 193), not failures in this change.