Skip to content

fix(plugins): give block directives precedence - #68644

Closed
synscott wants to merge 1 commit into
NousResearch:mainfrom
synscott:fix/pre-tool-block-precedence
Closed

synscott wants to merge 1 commit into
NousResearch:mainfrom
synscott:fix/pre-tool-block-precedence

Conversation

@synscott

Copy link
Copy Markdown
Contributor

Summary

  • make valid block directives take precedence over valid approve directives regardless of plugin registration order
  • preserve first-wins ordering among directives with the same action
  • preserve existing validation, approval fail-closed behavior, callback invocation, and public helper signatures

Problem

_get_pre_tool_call_directive_details() returned the first valid directive from the collected pre_tool_call hook results. If an approval-policy plugin was registered before a blocking policy plugin, an earlier approve could mask a later block:

approve, block -> approve
block, approve -> block

That makes policy composition depend on plugin discovery order and allows adding an approval hook to weaken an existing veto.

Fix

Collect the first valid block and first valid approve from the already-materialized hook results, then return:

  1. the first valid block, when any block exists;
  2. otherwise the first valid approve;
  3. otherwise no directive.

All callbacks are still invoked once by invoke_hook() before selection. A selected block bypasses the human approval prompt; approve-only behavior remains unchanged and fail-closed.

Tests

Added regression coverage for:

  • block precedence in both registration orders
  • first block and first approve ordering
  • approve rule_key preservation
  • malformed block/approve results
  • single hook-result collection
  • suppression of the approval gate when a block exists

Focused canonical test runs on the submitted commit:

tests/hermes_cli/test_plugins.py: 122 passed
tests/tools/test_approval_plugin_hooks.py: 21 passed
tests/run_agent/ -k "tool or plugin or approval": 554 passed
tests/agent/ -k "tool or plugin or approval": 670 passed

Ruff lint, compileall, and git diff --check also pass.

Baseline limitation

The complete upstream suite is not green in this host environment. A prior full run produced unrelated dependency/environment failures that were reproduced by failure class on an unchanged worktree at the same base. This change does not touch those systems, and the affected plugin/approval/dispatch slices above pass on the submitted commit.

@alt-glitch alt-glitch added type/security Security vulnerability or hardening P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins area/auth Authentication, OAuth, credential pools labels Jul 21, 2026
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused policy-composition fix. Current main still returns the first valid directive in hermes_cli/plugins.py:2176-2194; an earlier approve is returned at hermes_cli/plugins.py:2192, before a later valid block can be evaluated. The returned approve then reaches the human gate at hermes_cli/plugins.py:2276-2295.

The proposed aggregation is consistent with the dispatcher: PluginManager.invoke_hook() already invokes every callback and materializes non-None results before selection (hermes_cli/plugins.py:1931-1946). The shared resolver is used by the current dispatch paths, including model_tools.py:1245-1278, agent/tool_executor.py:417-439, and agent/agent_runtime_helpers.py:2537-2554.

No blocking correctness or completeness issue found. This is an automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 30, 2026
teknium1 added a commit that referenced this pull request Sep 22, 2026
`_get_pre_tool_call_directive_details` returned the first valid block-or-approve
in registration order, so a plugin registered earlier that returned `approve`
hid a later security plugin's `block`; under `approvals.mode: off` an approve
means no prompt at all, so the veto was dropped silently. Precedence is now
`block` > `approve` > none: a valid block still returns immediately (modify
directives seen before it stay attached, as before), a valid approve is held
back until the whole result list has been scanned for a veto, and among approves
the first valid one (with its rule_key) still wins. Modify accumulation is
unchanged and now also keeps modify directives that follow the winning approve,
since the scan no longer stops there. Docstring and hooks.md no longer describe
"first valid directive wins".

Slim redo of #68644 (earliest) and #87449 against the modify-aware shape of the
function on main; both PRs predate it and could not be cherry-picked.

Fixes #87420
Supersedes #68644
Supersedes #87449
Co-authored-by: synscott <1563043+synscott@users.noreply.github.com>
Co-authored-by: Jack Lau <72348727+jackulau@users.noreply.github.com>
teknium1 added a commit that referenced this pull request Sep 22, 2026
`_get_pre_tool_call_directive_details` returned the first valid block-or-approve
in registration order, so a plugin registered earlier that returned `approve`
hid a later security plugin's `block`; under `approvals.mode: off` an approve
means no prompt at all, so the veto was dropped silently. Precedence is now
`block` > `approve` > none: a valid block still returns immediately (modify
directives seen before it stay attached, as before), a valid approve is held
back until the whole result list has been scanned for a veto, and among approves
the first valid one (with its rule_key) still wins. Modify accumulation is
unchanged and now also keeps modify directives that follow the winning approve,
since the scan no longer stops there. Docstring and hooks.md no longer describe
"first valid directive wins".

Slim redo of #68644 (earliest) and #87449 against the modify-aware shape of the
function on main; both PRs predate it and could not be cherry-picked.

Fixes #87420
Supersedes #68644
Supersedes #87449
Co-authored-by: synscott <1563043+synscott@users.noreply.github.com>
Co-authored-by: Jack Lau <72348727+jackulau@users.noreply.github.com>
teknium1 added a commit that referenced this pull request Sep 22, 2026
`_get_pre_tool_call_directive_details` returned the first valid block-or-approve
in registration order, so a plugin registered earlier that returned `approve`
hid a later security plugin's `block`; under `approvals.mode: off` an approve
means no prompt at all, so the veto was dropped silently. Precedence is now
`block` > `approve` > none: a valid block still returns immediately (modify
directives seen before it stay attached, as before), a valid approve is held
back until the whole result list has been scanned for a veto, and among approves
the first valid one (with its rule_key) still wins. Modify accumulation is
unchanged and now also keeps modify directives that follow the winning approve,
since the scan no longer stops there. Docstring and hooks.md no longer describe
"first valid directive wins".

Slim redo of #68644 (earliest) and #87449 against the modify-aware shape of the
function on main; both PRs predate it and could not be cherry-picked.

Fixes #87420
Supersedes #68644
Supersedes #87449
Co-authored-by: synscott <1563043+synscott@users.noreply.github.com>
Co-authored-by: Jack Lau <72348727+jackulau@users.noreply.github.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Closing as superseded by #118844 (#118844), commit 75e9567ca7, which fixes the same defect (you are credited there as a co-author / in the PR body).

Thanks for the contribution — the underlying problem this PR addresses has been resolved on current main. If you believe this was closed in error, please comment and we'll reopen.

@teknium1 teknium1 closed this Sep 22, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/auth Authentication, OAuth, credential pools comp/plugins Plugin system and bundled plugins P3 Low — cosmetic, nice to have sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants