fix(plugins): rank pre_tool_call block above approve

[jackulau]

What does this PR do?

_get_pre_tool_call_directive_details() aggregated plugin pre_tool_call results with first-valid-wins semantics, so hook registration order — not severity — decided the outcome. A plugin returning {"action": "approve"} registered ahead of a security plugin returning {"action": "block"} made the block unreachable: the loop returned on the first valid directive of either kind.

That is backwards for the one case the directive exists to serve. An approve only escalates to the human-approval gate, but under approvals.mode: off (yolo-equivalent) an approve directive means no prompt at all — so a Safe Mode / startup-integrity / policy plugin's veto was silently discarded and the tool call ran unchallenged.

The fix holds the first approve back until the whole result list has been scanned for a veto, making precedence block > approve > no directive regardless of registration order. This mirrors how the approval layer already treats a deny from any source as decisive, and how the thread tool-whitelist block short-circuits ahead of the hooks.

Deliberately narrow — no change to hook invocation, to the directive shape, or to what counts as a valid directive:

• Within a single action the first valid directive still wins, including its rule_key for approve.
• A message-less block is still invalid and still ignored — and now correctly falls through to a later approve rather than swallowing it.
• Invalid/irrelevant return values are still silently ignored, so observer-only hooks are unaffected.

Related Issue

Fixes #87420

Type of Change

• 🔒 Security fix

(Also a bug fix; filing it under security because the failure mode is a silently-dropped plugin veto on a tool call.)

Changes Made

• hermes_cli/plugins.py — _get_pre_tool_call_directive_details(): return immediately on the first valid block; buffer the first valid approve and return it only after the full result list has been scanned. Docstring updated to state the precedence rule instead of "the first valid directive wins".
• tests/hermes_cli/test_plugins.py — four regression tests in TestPreToolCallDirective:
  • test_later_block_outranks_earlier_approve — the issue's exact repro (fails before this change, passes after)
  • test_earlier_block_still_wins — the reverse order is unchanged
  • test_message_less_block_does_not_suppress_approve — an invalid block does not swallow a valid approve
  • test_first_approve_wins_among_approves — precedence only reorders block vs approve; among approves the first still wins, rule_key included
• website/docs/user-guide/features/hooks.md — documented the precedence rule in the pre_tool_call directive section, which previously stated only "The first valid directive wins."

How to Test

Before the change, the reporter's repro returns ('approve', 'earlier plugin approves'); after it, ('block', 'later security plugin blocks'):

python -c "
from unittest.mock import patch
from hermes_cli.plugins import get_pre_tool_call_directive
results = [
    {'action': 'approve', 'message': 'earlier plugin approves'},
    {'action': 'block',   'message': 'later security plugin blocks'},
]
with patch('hermes_cli.lifecycle.invoke_hook', return_value=results):
    print(get_pre_tool_call_directive('write_file', {}))
"

Regression tests:

pytest tests/hermes_cli/test_plugins.py -q          # 60 passed
pytest tests/hermes_cli/test_plugins.py -q -k "PreToolCall or ResolvePreTool"
python scripts/check-windows-footguns.py --all      # clean, 972 files
ruff check hermes_cli/plugins.py tests/hermes_cli/test_plugins.py

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 — searched both the issue number and the fix area (get_pre_tool_call_directive, pre_tool_call precedence); the only prior work is feat(plugins): pre_tool_call approve action escalates to human gate (closes #51221) #58698 (merged, added the approve action) and feat(plugins): add pre_tool_call approve action and plugin mode #11816 (open, unrelated to precedence)
• My PR contains only changes related to this fix/feature (no unrelated commits)
• I've run pytest tests/ -q and all tests pass — ran the relevant suites (tests/hermes_cli/, tests/tools/test_approval.py) rather than the full tree. One pre-existing, unrelated collection error is present on Windows: tests/hermes_cli/test_doctor_journal_modes.py fails at import with AttributeError: module 'os' has no attribute 'geteuid'. That file is untouched by this PR and fails identically on a clean upstream/main.
• I've added tests for my changes (required for bug fixes, strongly encouraged for features)
• I've tested on my platform: Windows 11 (Python 3.11 venv)

Documentation & Housekeeping

• I've updated relevant documentation (README, docs/, docstrings) — docstring + website/docs/user-guide/features/hooks.md
• N/A — no config keys added or changed
• N/A — no architecture or workflow change
• I've considered cross-platform impact (Windows, macOS) — pure control-flow change, no platform-specific primitives; check-windows-footguns.py --all is clean
• N/A — no tool descriptions or schemas changed

Credit to @yangc222 for the report, the precise root-cause location, and the repro — the semantics implemented here are the ones proposed in the issue.

[jackulau]

Rebased onto current main — the branch had gone CONFLICTING after the modify action landed in _get_pre_tool_call_directive_details().

Both conflicts were in the function this PR touches, resolved on merit rather than by taking a side:

• modify accumulation is preserved exactly. modified_args still accumulates across hooks and is still attached to whatever directive is returned, block included.
• The one real interaction is that deferring approve to scan for a later block could have retroactively pulled in modify directives that come after the approve. It doesn't: the approve directive snapshots modified_args as it stood when that approve was seen, so accumulation semantics are byte-for-byte what they are on main.
• Two regression tests pin this — test_deferred_approve_does_not_absorb_later_modify and test_block_still_carries_accumulated_modify.

The docs merge keeps main's "(Python plugins registered first, then shell hooks)" and observer-only clauses alongside the new precedence sentence.

70 tests pass in tests/hermes_cli/test_plugins.py, ruff clean, check-windows-footguns.py --all clean (972 files). No scope change: still only the block > approve precedence fix.

[Enough1122]

AI code review — automated review for reference, author can ignore or act on any point.

fix(plugins): rank pre_tool_call block above approve

• hermes_cli/plugins.py (~6055-6062): the deferred-approve snapshot uses dict(modified_args), a shallow copy. If a modify directive appearing after the approve mutates a nested structure (e.g. a list inside args), the approve's snapshot is retroactively altered because the nested objects are shared. Either deep-copy the args at snapshot time or document the shallow-copy contract explicitly.
• This is a security-relevant precedence change: any existing hook pair where an earlier plugin's approve preceded a later plugin's block will now be vetoed. Intended behavior, and the hooks.md update is good — a changelog/upgrade note for plugin authors would help since this silently changes runtime semantics.
• rule_key is now read for every directive (result.get("rule_key")) even for block/modify results where it is never used — harmless but slightly misleading; could be moved under the approve branch for clarity.
• The six new tests (block-over-approve, approve-over-invalid-block, first-approve rule_key, deferred-approve modify snapshot, block carrying accumulated modify) are solid coverage of the precedence matrix.

[jackulau]

Thanks, useful pass. Acted on two of the four, and one does not reproduce.

Shallow copy at the deferred-approve snapshot: documented rather than deepened

You are right that dict(modified_args) is shallow. I do not think the failure you describe is reachable through it, though, because of how a later modify reaches the accumulator:

modified_args.update(partial)

update rebinds top-level keys to the objects the hook returned. It does not mutate the values already behind those keys, so a modify after the approve cannot reach through the snapshot's shared references, and the snapshot keeps whatever the accumulator held at that moment. The scenario needs someone to mutate a nested value in place, which no hook in this path does and which would corrupt modified_args itself just as readily.

I took your second option rather than the deep copy. Deep-copying would diverge from the shallow-merge contract modify already documents, and it would break hooks that pass unpicklable or identity-significant objects through args, which is a real cost for a case the code cannot currently reach. 3524b9b records the reasoning at the snapshot itself and adds the rule to the user-facing docs: a modify replaces a top-level key rather than merging into the value under it, so return a new value instead of mutating a nested object you received.

If you can construct a hook sequence that does defeat the snapshot, I would rather see it than argue from the code, and I will take the deep copy.

Upgrade note: added

Agreed that this changes runtime semantics with no API change to signal it. There is no CHANGELOG in the repo, so it went to hooks.md as a warning admonition, matching the existing "Breaking change" note in api-server.md. It names what changes (an earlier approve no longer suppresses a later block, regardless of registration order) and what does not (hooks returning a single kind of directive, observer-only hooks, modify).

rule_key read for every directive: does not reproduce

It is only read on the approve path. By the time that line runs, modify results have already hit continue at the top of the loop, block has returned, and a second approve has hit continue at the first_approve is not None guard. So the only directive that reaches result.get("rule_key") is the first valid approve, which is the one that uses it. I left it where it is.

Test coverage

The deferred-approve modify snapshot case you list is the one that pins the first point above, so it is doing real work rather than restating the implementation. Nothing added there this round.