Skip to content

feat(plugins): add hermes-approval-guard dual-agent supervision plugin - #34699

Open
kkangg1 wants to merge 27 commits into
NousResearch:mainfrom
kkangg1:feat/approval-guard-plugin
Open

feat(plugins): add hermes-approval-guard dual-agent supervision plugin#34699
kkangg1 wants to merge 27 commits into
NousResearch:mainfrom
kkangg1:feat/approval-guard-plugin

Conversation

@kkangg1

@kkangg1 kkangg1 commented May 29, 2026

Copy link
Copy Markdown

What

hermes-approval-guard — a two-stage semantic approval plugin via pre_tool_call hook covering ALL tools, not just terminal.

Architecture:

Tool call → pre_tool_call hook
  ├─ SAFE_TOOLS (21 read/query tools) → ALLOW (0ms)
  ├─ extract_context — reuses system detect_dangerous/hardline_command
  ├─ Terminal fast-path: no DANGEROUS signal → ALLOW (0ms)
  │   • HARDLINE-only signals (rm -rf /) → skip LLM, let system handle it
  ├─ Stage 1: LLM fast-classify → ALLOW / ESCALATE (~500ms)
  │   • Uses call_llm(task="approval") with context-aware prompt
  │   • NEVER outputs DENY — DENY reserved for Stage 2 / system HARDLINE
  │   • LLM config: provider/model optional; unset → defaults to main Agent LLM
  │   • On terminal ALLOW: calls approve_session() to pre-mark patterns
  │     → system's check_all_command_guards skips redundant LLM
  └─ Stage 2: hermes chat -q --profile approval deep review (3-8s, optional)
      • Stateless: no persistent session; all context injected in prompt
      • Context from SessionDB: conversation + full tool call chain
      • Hindsight-backed: session-level + cross-session pattern memory
      • Toolsets restricted to file,memory,session_search (no terminal)
      • Timeout kills entire process group (Popen + start_new_session + killpg)
      • Outputs: ALLOW / DENY / MODIFY with structured JSON feedback

System HARDLINE (rm -rf /, mkfs, dd, shutdown, etc.) always active — cannot be bypassed.

Terminal Handling (detailed)

terminal command
  ├─ extract_context()
  │   ├─ detect_hardline_command() → HARDLINE signal (descriptive only)
  │   └─ detect_dangerous_command() → DANGEROUS signal + pattern_key
  ├─ fast-path check
  │   has_real_risk = any("WARNING" in s AND "HARDLINE" not in s)
  │   ├─ False (git status, ls, echo) → 0ms pass
  │   ├─ False BUT HARDLINE-only (rm -rf /) → pass, system blocks
  │   └─ True (rm -rf node_modules, chmod 777 /etc, curl | bash) → Stage 1
  ├─ Stage 1 LLM (ALLOW? ESCALATE?)
  │   ├─ ALLOW → approve_session(pattern_keys) → pre-mark → system skips DANGEROUS
  │   └─ ESCALATE → Stage 2 ACP
  └─ Stage 2 ACP → ALLOW / DENY / MODIFY
Command Plugin path System Outcome
git status fast-path (0ms) Execute
pip install pkg fast-path → Stage1 ALLOW → pre-mark Execute
rm -rf node_modules Stage1 → ALLOW → pre-mark skipped Execute
rm -rf / fast-path (HARDLINE-only) BLOCKED Denied
curl url | bash Stage1 → ESCALATE → Stage2 ACP decides

Design Principles

Principle Explanation
No hardcoded DENY in Stage 1 Risk signals are LLM context only — never hard-block
LLM: ALLOW / ESCALATE only Stage 1 mirrors system's _smart_approve; DENY reserved for Stage 2 / HARDLINE
Stateless ACP No --resume; all context from SessionDB + Hindsight; concurrent-safe
Fail-open Broken guard is never more restrictive than no guard
Terminal fast-path No DANGEROUS regex match → 0ms skip; HARDLINE-only also skips
approve_session pre-marking Stage 1 ALLOW → pre-mark patterns → system skips redundant DANGEROUS LLM
Explicit config only Memory backend config fully explicit in plugin_guard.memory; no cross-plugin cascade

Why

Aspect Built-in approvals.mode This plugin
Coverage terminal only ALL 25+ tools (write_file, patch, delegate_task, execute_code, etc.)
Decision Regex + zero-context LLM (16 tokens) Semantic LLM + ACP agent with full session context
Memory Session-level (_session_approved) Hindsight cross-session pattern bank
Context Command string only Conversation + tool chain + historical patterns
Denial feedback "BLOCKED: xxx" Structured: reason + alternatives + approval_id

Changes

11 files, 1,918 insertions:

File Purpose
plugin.yaml Manifest (standalone, pre_tool_call hook)
__init__.py PluginContext.register_hook entry point
guard.py Dispatcher, SessionDB context query, terminal fast-path, SAFE_TOOLS bypass
stage1_rules.py Risk signal extraction, reuses system detect_dangerous/hardline_command — never blocks
stage1_llm.py LLM classify: ALLOW/ESCALATE only, _smart_approve-style prompt
stage2_acp.py Stateless ACP: 5-section prompt, SessionDB + Hindsight injection, process-group timeout cleanup
hindsight_store.py Hindsight/Honcho/none memory backends, pattern key generation, session/pattern queries
feedback.py Structured denial: reason + alternative suggestions + approval_id + override paths
test_integration.py 7 scenarios, 41 test cases
README.md Architecture, terminal handling, design principles, failure modes, config reference
recommended-config.yaml Annotated config template with all options

Test Plan

cd plugins/hermes-approval-guard
python3 test_integration.py

7 scenarios, 41 test cases. Covers: SAFE_TOOLS bypass (9 of 21 tools sampled), context extraction (no hard DENY — signals only), write_file path/signal tests, delegate_task danger keywords, terminal HARDLINE vs DANGEROUS separation, structured feedback messages, pattern key generation (write_file/terminal/delegate_task), and Stage 1 LLM prompt structure (ALLOW/ESCALATE only, false-positive examples).

Config

plugin_guard:
  enabled: true
  # provider: your_provider_name     # optional, defaults to main Agent LLM
  # model: your_model_name           # optional, defaults to main Agent LLM
  fail_open: true

  stage2:
    enabled: false                 # ACP deep review (test first)
    profile: approval              # hermes --profile name

  memory:
    backend: hindsight             # hindsight | honcho | none
    bank: approval
    hindsight_url: "http://localhost:8888"

approvals:
  mode: off   # plugin handles all tools; HARDLINE remains active

Configuration Reference

Key Type Default Description
plugin_guard.enabled bool false Master switch
plugin_guard.provider str LLM provider (optional, defaults to main LLM)
plugin_guard.model str Model name (optional, defaults to main LLM)
plugin_guard.fail_open bool true LLM failure → allow (safe default)
plugin_guard.stage1.timeout int 5 Seconds for LLM classification
plugin_guard.stage2.enabled bool false Enable ACP deep review
plugin_guard.stage2.profile str "approval" Hermes profile for review agent
plugin_guard.stage2.timeout int 15 Seconds for deep review
plugin_guard.memory.backend str "hindsight" hindsight, honcho, or none
plugin_guard.memory.bank str "approval" Hindsight bank or Honcho user_id
plugin_guard.memory.hindsight_url str "http://localhost:8888" Hindsight server address
plugin_guard.memory.honcho_url str "http://localhost:1819" Honcho server address

Also set approvals.mode: off when plugin is enabled — system DANGEROUS check is redundant; system HARDLINE remains active regardless.

Failure Modes

Failure Behavior
plugin_guard.enabled: false Handler returns None immediately (~0.1ms)
Config missing/corrupt Plugin self-disables; all tools pass
Stage 1 LLM unavailable fail_open:true → ALLOW; fail_open:false → ESCALATE
Stage 2 ACP crash/timeout fail_open:true → ALLOW; fail_open:false → DENY
Hindsight backend down Silent skip; tool execution unaffected
Module import failure Caught by Hermes plugin loader; not registered

Compatibility

  • Hermes ≥ 0.14.0
  • Uses pre_tool_call plugin hook (hermes_cli/plugins.py)
  • Imports tools.approval for detect_dangerous/hardline_command
  • Imports hermes_state.SessionDB for conversation context
  • Optional: Hindsight HTTP API for approval memory

Checklist

  • Plugin manifest (plugin.yaml) present
  • README.md with architecture, design principles, failure modes, config reference
  • Integration tests (41 pass, exit code 0)
  • All code, comments, docstrings, and LLM prompts in English
  • No privacy-sensitive data in docs or config templates
  • Explicit config only — no cross-plugin config cascade
  • System HARDLINE protection confirmed independent and non-interfering
  • Verified with GitNexus: 19 functions, zero external callers (hook-only), 2 cross-module deps (call_llm, load_config)

@alt-glitch alt-glitch added type/feature New feature or request P3 Low — cosmetic, nice to have comp/plugins Plugin system and bundled plugins type/security Security vulnerability or hardening labels May 29, 2026
@kkangg1
kkangg1 force-pushed the feat/approval-guard-plugin branch from 1049873 to d303b43 Compare May 30, 2026 08:02
kkangg1 added 19 commits June 24, 2026 17:16
## What does this PR do?

Adds **hermes-approval-guard** — a `pre_tool_call` hook plugin that provides two-stage approval for tool calls without modifying any Hermes source code. It fills the gap left by the built-in `approvals.mode`: that system only covers `terminal` commands; this plugin protects `write_file`, `patch`, `delegate_task`, and `execute_code`.

The plugin is **disabled by default** (`plugin_guard.enabled: false`). When disabled, the handler returns `None` after a single dict lookup + boolean check (~0.1ms), so there is no measurable impact on agent operations.

### Architecture

```
Tool call → pre_tool_call hook
  ├─ SAFE_TOOLS (20+ read/query tools) → ALLOW (0ms)
  │   read_file, search_files, web_search, session_search,
  │   skill_view, vision_analyze, clarify, hindsight_recall, ...
  ├─ terminal / process            → delegate to approvals.mode
  ├─ HARDLINE rules               → BLOCK (<1ms)
  │   ├─ Sensitive paths: /etc, /boot, /sys, /proc, /dev, ~/.ssh, ~/.gnupg
  │   ├─ Sensitive files: .env, config.yaml, id_rsa, id_ed25519, authorized_keys
  │   └─ Delegate danger: "delete all", "rm -rf /", "format disk",
  │       "wipe system", "destroy everything"
  ├─ Stage 1: LLM classifier      → ALLOW / DENY / ESCALATE (~500ms)
  │   Uses call_llm(task="approval") with structured prompt.
  │   Returns structured verdict; ESCALATE triggers Stage 2.
  └─ Stage 2: ACP Agent (opt)     → deep review (3-8s)
      Spawns hermes chat -q as subprocess with --toolsets terminal,file,web.
      Returns structured verdict with reason and evidence.
```

### Relationship with Existing `approvals.mode`

This plugin **extends**, not replaces. The division of responsibility:

```
terminal commands      → approvals.mode (smart/manual) — unchanged
write_file / patch     → approval-guard (newly protected)
delegate_task          → approval-guard (newly protected)
execute_code           → approval-guard (newly protected)
read/search/query      → bypass both (always safe)
```

| Aspect | `approvals.mode` (built-in) | `approval-guard` (this plugin) |
|--------|---------------------------|-------------------------------|
| Coverage | `terminal` only | `write_file`, `patch`, `delegate_task`, `execute_code` |
| Decision | Regex + zero-context LLM (16 tokens) | Full-context LLM + optional ACP Agent |
| Memory | None (stateless) | Hindsight / Honcho — isolated bank |
| Tool access | No | Stage 2 ACP Agent: terminal, file, web |
| Risk model | Binary safe/dangerous | Gradient: score = base × intent × context |

### Failure Modes (Fault Tolerance)

The plugin defaults to **fail-open** for all newly-protected tools. The guiding principle: a broken guard should **never** be more restrictive than having no guard at all.

| Failure Scenario | Behavior | Recoverable? |
|-----------------|----------|:---:|
| `plugin_guard.enabled: false` | Handler returns `None` immediately (~0.1ms) | N/A (by design) |
| Config file missing/corrupted | Plugin disables itself (`_config_disable = True`), all tools pass | ✅ Auto-recover on config fix + restart |
| Stage 1 LLM call fails | `fail_open: true` → ALLOW; `fail_open: false` → DENY | ✅ Retry on next call |
| Stage 2 ACP subprocess fails | `fail_open: true` → ALLOW; `fail_open: false` → DENY | ✅ Retry on next call |
| Memory backend (Hindsight) down | Silently skips audit logging; tool execution unaffected | ✅ Auto-resume when backend recovers |
| Plugin module import fails | Caught by Hermes plugin loader; plugin not registered | ✅ No impact on agent |

All LLM failures are logged at WARNING level with the verdict outcome, making post-mortem analysis possible.

### Why ACP over A2A (for Stage 2)

- Hermes already ships with `acp_adapter/edit_approval.py` — zero new infrastructure
- No external A2A server or container needed
- Single `hermes chat -q -p approval "review: ..."` subprocess call
- Approval agent shares the main agent's LiteLLM gateway and model config

## Related Issue

Related to existing `pre_tool_call` hook proposals and bugs:

- **NousResearch#11812** — `feat(plugins): add pre_tool_call "approve" action and plugin mode` — This PR implements a full two-stage approval system using the existing `pre_tool_call` hook infrastructure, avoiding the need for core changes to `get_pre_tool_call_block_message()`.
- **NousResearch#18988** — `Feature request: pre_tool_call rewrite action (rewrite tool args from a hook)` — Complementary feature; approval-guard could extend to argument rewriting in a future version.
- **NousResearch#34618** — `session_id not propagated to get_pre_tool_call_block_message` — Bug affecting this plugin; the handler accepts `session_id` as a kwarg but may receive empty string from some call sites.
- **NousResearch#28961** — `tool_executor pre_tool_call hook calls omit session_id and tool_call_id (v0.14.0)` — Related bug; plugin designed to be resilient to missing session/tool_call IDs.

Fixes # (new feature, no specific bug fix)

## Type of Change

- [x] ✨ New feature (non-breaking change that adds functionality)
- [x] ✅ Tests (adding or improving test coverage)

## Changes Made

- `plugins/hermes-approval-guard/plugin.yaml` — Plugin manifest (kind: standalone, hook: pre_tool_call)
- `plugins/hermes-approval-guard/__init__.py` — Registers hook via `PluginContext.register_hook("pre_tool_call", handler)`
- `plugins/hermes-approval-guard/guard.py` — Main dispatcher: SAFE_TOOLS → HARDLINE → LLM → ACP, terminal delegation
- `plugins/hermes-approval-guard/stage1_rules.py` — Pattern-based fast path (<1ms): sensitive paths, file names, delegate keywords
- `plugins/hermes-approval-guard/stage1_llm.py` — LLM semantic classifier using `call_llm(task="approval")` with structured prompt
- `plugins/hermes-approval-guard/stage2_acp.py` — ACP Agent deep review via `hermes chat -q` subprocess
- `plugins/hermes-approval-guard/feedback.py` — Structured denial messages with reason, alternatives, memory ID
- `plugins/hermes-approval-guard/hindsight_store.py` — Memory backend: Hindsight HTTP API / Honcho / none
- `plugins/hermes-approval-guard/recommended-config.yaml` — Annotated config template

## How to Test

### Unit Tests

```bash
cd plugins/hermes-approval-guard
python3 -m pytest test_unit.py -v
```

Covers: SAFE_TOOLS bypass, HARDLINE block, terminal delegation, process delegation, safe writes (5 tests).

### Integration Tests

```bash
cd plugins/hermes-approval-guard
python3 test_integration.py
```

44 tests covering 9 scenarios:

| # | Scenario | Tests | Key Edge Cases |
|---|----------|:---:|---------------|
| 1 | SAFE_TOOLS bypass | 7 | All 20+ read/query tools return None |
| 2 | HARDLINE path block | 5 | /etc, /boot, /sys, /proc, ~/.ssh, ~/.gnupg |
| 3 | HARDLINE filename block | 7 | .env, config.yaml, id_rsa, + safe paths like readme.md |
| 4 | Safe path → pass to LLM | 4 | User home dir, /tmp, relative paths |
| 5 | Delegate danger detection | 5 | "delete all", "format disk", "wipe system" → blocked; normal tasks → pass |
| 6 | Terminal delegation | 4 | Even dangerous commands like `sudo rm -rf /` passed to approvals.mode |
| 7 | Disabled → zero interference | 3 | /etc/passwd write, delegate_task with "rm -rf /", reboot command — all pass |
| 8 | Feedback message format | 4 | action field, message field, non-empty, CJK text present |
| 9 | LLM classification pipeline | 5 | ALLOW/DENY/ESCALATE mock, fail_open → ALLOW, fail_open=False → DENY |

### Live Test

1. Add `hermes-approval-guard` to `plugins.enabled` in `config.yaml`
2. Set `plugin_guard.enabled: true`
3. Restart Hermes — verify with `hermes plugins list`
4. Trigger a `write_file` call — confirm the LLM classification runs
5. Set `plugin_guard.enabled: false` — confirm all tools pass without LLM calls

## Checklist

### Code

- [x] I've read the Contributing Guide
- [x] My commit messages follow Conventional Commits
- [x] I searched for existing PRs (none found for approval-guard concept)
- [x] My PR contains only changes related to this feature
- [x] I've run `pytest tests/ -q` — plugin has standalone unit + integration tests, all pass (49/49)
- [x] I've added tests for my changes (unit + integration, 49 total)
- [x] I've tested on my platform: WSL2 Ubuntu 26.04, Python 3.12.13, Hermes 0.14.0

### Documentation & Housekeeping

- [x] I've updated relevant documentation — N/A (self-contained plugin with recommended-config.yaml)
- [x] I've updated `cli-config.yaml.example` — N/A (config is read from `plugin_guard` key, not cli-config)
- [x] I've updated CONTRIBUTING.md or AGENTS.md — N/A (no architecture changes to core)
- [x] I've considered cross-platform impact — plugin is pure Python with optional `hermes chat -q` subprocess (Unix-only for Stage 2); Stage 1 is platform-agnostic
- [x] I've updated tool descriptions/schemas — N/A (no tool changes)

## For New Skills (Plugin)

- [x] This is broadly useful — dual-agent supervision is a common need for production Hermes deployments
- [x] Follows standard format — see `plugin.yaml` with kind, hooks, metadata
- [x] No external dependencies beyond Hermes core — `call_llm`, `PluginContext`, optional `agent.auxiliary_client`
- [x] Tested end-to-end — plugin loaded, hook registered, verified with `hermes plugins list`

## Screenshots / Logs

```bash
$ hermes plugins list | grep approval-guard
  hermes-approval-guard | enabled | user | v0.1.0

$ python3 test_integration.py
═══ 1. SAFE_TOOLS ═══
  ✅ read_file
  ✅ web_search
  ...
  ✅ vision_analyze
═══ 9. LLM classification ═══
  ✅ ALLOW
  ✅ DENY
  ✅ ESCALATE
  ✅ fail→ALLOW
  ✅ fail→DENY
==================================================
  44/44 passed, 0/44 failed
```
Replace hardcoded HARDLINE rejection with LLM+ACP semantic review:

- Stage 1 (LLM): never outputs DENY, only ALLOW/ESCALATE
  • Prompt mirrors system _smart_approve style ("many flagged
    commands are false positives")
  • Terminal fast-path: no DANGEROUS regex matches 0ms bypass
  • On ALLOW: calls approve_session() to pre-mark patterns,
    preventing redundant system check_all_command_guards

- Stage 2 (ACP): stateless deep review with full session context
  • No persistent ACP session (stateless, concurrent-safe)
  • Injects conversation context + tool chain from SessionDB
  • Hindsight-backed session and cross-session pattern memory

- guard.py: session DB query replaces in-memory log
- stage1_rules.py: extract_context() replaces fast_path() HARDLINE
- stage2_acp.py: five-section structured prompt
- hindsight_store.py: session/pattern query + pattern key generation
- recommended-config.yaml: approvals.mode: off recommended
- README: describe new architecture (no hardcoded HARDLINE, ALLOW/ESCALATE
  only, stateless ACP, terminal fast-path, approve_session pre-marking)
- tests: rewrite 41 integration tests matching new extract_context() and
  prompt structure (7 scenarios)
Hindsight Docker container exposes port 8888, not 8421. The hardcoded
default was causing "Connection refused" on all recall/retain calls,
leaving ACP without session history or cross-session pattern memory.
- Remove --provider flag from ACP subprocess; let approval profile
  handle provider/model selection (was hardcoded to "zjic")
- Fix _parse_acp_output to strip "Query:" prefix and error lines
  before parsing, preventing false DENY from matching prompt keywords
- Validate JSON verdict values (ALLOW/DENY/MODIFY only)
…chain

- hindsight_url/honcho_url read from plugin_guard.memory config first,
  then HINDSIGHT_URL/HONCHO_URL env vars, then defaults (8888/1819)
- _get_hindsight_url(cfg) and _get_honcho_url(cfg) helpers
- Updated all API functions to pass cfg through for URL resolution
… hardening

- .gitignore: shield GitNexus auto-generated .claude/ and CLAUDE.md
- guard.py: remove redundant fail_open branch in stage2-disabled path
- hindsight_store.py: reuse official Hindsight plugin config chain
  (plugin_guard.memory > official config.json > env vars > defaults)
  instead of duplicating URL/bank resolution
- hindsight_store.py: remove dead code (unreachable cfg fallback)
- stage2_acp.py: harden LLM prompt with anti-chat instructions
  ('你是自动化安全检查程序,不是对话助手')
  and fail-open guidance ('宁可放过不可误杀')

Risk: low — all changes are within approval-guard plugin, no
API or architecture changes.
Remove official Hindsight config.json lookup from URL/bank
resolution. All memory config is now explicitly declared in
plugin_guard.memory section:

  plugin_guard.memory.hindsight_url → HINDSIGHT_URL env → default
  plugin_guard.memory.bank          → 'approval'

No more cross-plugin config cascade — the approval-guard owns
its own config entirely.
…config

- Add detailed terminal handling section with decision tree
- Add common command scenario table
- Document explicit config policy (no cross-plugin config cascade)
- Add hindsight_url/honcho_url to config reference
- Update files table with current descriptions
All comments, docstrings, LLM prompts, user-facing denial messages,
and test strings converted from Chinese to English. This makes the
plugin ready for upstream PR submission.

- stage1_llm/stage2_acp: LLM prompts fully in English
- feedback: denial messages and override instructions in English
- stage1_rules: signal descriptions (HARDLINE, DANGEROUS patterns)
- guard/hindsight_store: docstrings and section comments
- recommended-config.yaml: all comments in English
- test_integration: updated string matchers to match new English text
- README: removed last Chinese citation

Tests: 41/41 passed, zero Chinese characters remain.
C1 (CRITICAL): pass provider/model from plugin_guard config to call_llm
  - call_llm resolution: explicit args > auxiliary.{task} > auto
  - Without explicit args, plugin_guard.provider/model was dead config
  - Now: cfg.get('provider')/cfg.get('model') → explicit args → priority 1
  - Unset → None → falls through to main Agent LLM (auto-detection)

C2 (CRITICAL): stage2 default enabled=False to match recommended-config.yaml
  - guard.py:237 cfg_stage2.get('enabled', True) → False
  - Prevents accidental Stage 2 launch when config is absent

H1 (HIGH): kill entire process group on ACP timeout
  - Switched subprocess.run → Popen with start_new_session=True
  - Timeout → os.killpg(SIGTERM) → 3s grace → SIGKILL
  - Prevents orphaned grandchild processes from hermes chat subprocesses

H2 (HIGH): remove terminal from ACP agent toolsets
  - -t 'file,terminal,memory,session_search' → 'file,memory,session_search'
  - ACP review agent must not execute commands

H3 (HIGH): remove hardcoded path in test_integration.py
  - ~/gitea/hermes-agent/... → os.path.dirname(__file__)
  - Tests now run from any location

Docs: generalize provider/model in recommended-config.yaml and README
  - Replace specific provider names with your_provider_name placeholders
  - Comment out by default; uncomment to override from main Agent LLM
…ty, SAFE_TOOLS count

- Fix SAFE_TOOLS count: 13 → 21 (actual frozenset size)
- Document LLM config chain: optional provider/model, defaults to main LLM
- Document stage2 subprocess safety: restricted toolsets, process-group cleanup
- Fix Quick Start: stage2.enabled: true → false (match recommended-config)
- Add LLM config note to Architecture diagram
…config TTL

- stage1_rules: add tirith security scan for terminal commands
  (catches pipe-to-interpreter, sudo abuse, etc. beyond DANGEROUS regex)
- stage2_acp: short-circuit when session context is empty (skip subprocess)
- stage2_acp: prompt hardening — CRITICAL RULE defaults to ALLOW on sparse context
- guard: _load_config() 30s TTL — config changes take effect without restart
- guard: add diagnostic logging for _get_session_context (session_id tracing)
Upstream invoke_tool() does not pass session_id to the pre_tool_call
hook, so plugin handlers always receive an empty session_id and
cannot query SessionDB for conversation context.

Workaround (active until upstream fix is merged):
- __init__.py: monkey-patch AIAgent._invoke_tool to stash
  agent.session_id in thread-local storage before the hook fires
- guard.py: fall back to thread-local when hook session_id is empty

This is self-contained in the plugin — no hot-patch to installed
Hermes files required.
…rmanent lock

- guard.py: when session_id is empty, walk call stack (inspect.stack())
  to find AIAgent._invoke_tool's self.session_id — no monkey-patching,
  no system file modifications
- guard.py: fix _config_disable bug — once set True (from config
  enabled=false or load exception), it permanently froze the cache.
  Now resets on each TTL refresh so config changes take effect.
- guard.py: remove test toggle from prior debugging session
- __init__.py: revert to clean (remove monkey-patch, only register hook)

@teknium1 teknium1 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the substantial plugin implementation. Current main has moved since this branch: hermes_cli/plugins.py:2114-2128 now supports a plugin pre_tool_call approve directive that sends any tool to the native human approval gate, so the terminal-only premise needs re-scoping rather than duplication.

Problems

  • guard.py:56-71 latches _config_disable; after one disabled/error read, the documented 30-second reload cannot run.
  • guard.py:40 bypasses browser_console, although tools/browser_tool.py:3270-3290 evaluates supplied JavaScript expressions.
  • guard.py:273-278 returns None even with fail_open: false; that permits the call after a Stage 2 exception.
  • stage2_acp.py:251-259 launches with the writable file toolset (toolsets.py:191-194) and HERMES_YOLO_MODE=1, so the reviewer is not read-only.
  • test_integration.py:77-80 counts a failed guard import as passing tests.

Suggested changes

  • Make config reload retryable, make Stage 2 failure honor fail_open, and remove non-read-only tools from the bypass/reviewer paths.
  • Add standard tests/plugins/ coverage for discovery and actual hook dispatch, including failure branches and Windows behavior.

Automated hermes-sweeper review.

Comment thread plugins/hermes-approval-guard/guard.py Outdated
# ── Always-safe tools (read-only, skip all review) ─────────────────
_SAFE_TOOLS = frozenset({
"read_file", "search_files", "web_search", "web_extract",
"session_search", "browser_snapshot", "browser_console",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

browser_console is not read-only when expression is supplied: current tools/browser_tool.py evaluates JavaScript in the page context. Keeping it in this unconditional bypass contradicts the all-tools guard claim; distinguish console-read mode from expression evaluation or remove it from _SAFE_TOOLS.

Comment thread plugins/hermes-approval-guard/guard.py Outdated

def _load_config() -> Dict[str, Any]:
global _config_cache, _config_disable, _config_cache_time
if _config_disable:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once a missing, invalid, or disabled config sets _config_disable, this early return prevents every later TTL refresh. The comment below promises live config refresh, but enabling the plugin after startup will never take effect.

Comment thread plugins/hermes-approval-guard/guard.py Outdated
exc, cfg.get("fail_open", True))
if cfg.get("fail_open", True):
return None
return None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This returns None for a Stage 2 exception regardless of fail_open, which permits the tool call. Return a block directive when fail_open is false and add a regression test.

[
"hermes", "chat", "-q", review_prompt,
"--profile", profile,
"-t", "file,memory,session_search",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current file includes write_file and patch (toolsets.py:191-194), so this reviewer is not read-only. Combined with HERMES_YOLO_MODE=1 below, a prompt-injected reviewer can mutate files without the approval layer; use a genuinely read-only boundary.

except Exception as e:
print(f" WARN guard.py load failed: {e} (expected — needs Hermes runtime)")
for t in safe_tools:
check(f"SAFE_TOOLS should contain {t}", True) # known-safe, assume pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A failure to import guard.py is converted into passing checks. This masks plugin-loader and runtime import regressions; fail the test instead and add pytest coverage that exercises real plugin discovery and pre-tool dispatch.

@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:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 13, 2026
@teknium1 teknium1 added the sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users label Jul 13, 2026
Hermes Agent and others added 8 commits July 17, 2026 14:35
…d fail-closed paths

- Remove browser_console from _SAFE_TOOLS: it evaluates arbitrary JS
  when expression is supplied (tools/browser_tool.py), so it is not
  read-only.
- Drop _config_disable short-circuit: it permanently blocked TTL
  refreshes, contradicting the live-config-reload comment. Config now
  reloads on every TTL expiry; load failures return disabled without
  caching the failure state.
- Return a block directive when Stage 2 ACP raises and fail_open=False
  (previously both branches returned None, permitting the call). Add a
  regression test for fail-closed behavior.
- Replace ACP reviewer toolset with a read-only whitelist and drop
  HERMES_YOLO_MODE=1: the previous setup let a prompt-injected reviewer
  mutate files without the approval layer.
- test_integration: fail checks on guard.py import errors instead of
  converting them to passes, so plugin-loader regressions surface.

Addresses review comments from @teknium1 on PR NousResearch#34699.
- Add repo root to sys.path so stage1_rules can import tools.approval
  for dangerous/hardline command detection. Previously the import
  silently fell back to a sentinel message, causing the terminal
  danger tests to fail spuriously with 'unable to load module'.
- Rewrite Stage 2 fail-closed regression as a source-structure
  assertion. A full mock-based test requires a package import context
  (guard.py uses function-level relative imports) and is covered by
  integration tests in the Hermes test suite; here we assert the
  exception handler returns build_deny_message when fail_open=False
  instead of returning None unconditionally.

Result: 43 passed, 0 failed (was 39 passed, 2 failed).
Strip HERMES_YOLO_MODE from the reviewer child env — tools/approval
freezes it at import time, so inheriting it from a --yolo parent gateway
would let the stage2 reviewer bypass every approval gate. The reviewer
is already restricted to the read-only session_search toolset; this
closes the env-var half of the boundary.

Also extract the timeout kill path into _kill_review_process() with a
Windows fallback: os.killpg/signal.SIGKILL do not exist there, so the
direct child is terminate()d/kill()ed instead of raising AttributeError
out of the timeout handler.
The core dispatcher passes turn_id/api_request_id/middleware_trace/
telemetry_schema_version to pre_tool_call callbacks. The handler only
accepted five kwargs, so invoke_hook caught the resulting TypeError and
the plugin was silently neutered at runtime. Add **_extra, matching the
bundled langfuse/security-guidance hook convention.
test_integration.py converted hindsight_store/stage1_llm import failures
from warn-and-skip to hard failures, closing the remaining
'import regression reads green' holes (guard.py itself was fixed in the
previous commit on this branch).

New tests/plugins/test_approval_guard_plugin.py covers what the
standalone runner cannot:
  * real bundled-plugin discovery via PluginManager.discover_and_load
  * pre_tool_call hook dispatch through hermes_cli.plugins
    (block directive end-to-end, extra-kwargs tolerance regression)
  * failure branches: fail_open=false blocks on stage2 exception,
    config-load failure is retried instead of latching disabled
  * stage2 reviewer boundary: HERMES_YOLO_MODE stripped, session_search
    toolset restriction
  * browser_console is reviewed, not bypassed
  * Windows behavior: kill fallback without killpg/SIGKILL
Add a 'Relationship with the Native approve Directive' section:
upstream pre_tool_call now supports {"action": "approve"} to escalate
any tool to the built-in human approval gate; document that this plugin
complements rather than duplicates it (autonomous dual-agent semantic
review for unattended runs vs human-in-the-loop veto).

Also sync stale facts: SAFE_TOOLS count (21 -> 20 after browser_console
removal), stage2 reviewer now gets session_search only (not
file,memory,session_search) with HERMES_YOLO_MODE stripped, and the new
tests/plugins pytest entry point.
@kkangg1

kkangg1 commented Jul 31, 2026

Copy link
Copy Markdown
Author

Rebased onto current main and addressed the sweeper review point by point:

  • Config reload latching (guard.py:56-71): _config_disable was already removed — a failed/disabled read returns {"enabled": False} without caching, so the next TTL cycle retries. Added TestConfigReload regression coverage (enable-after-startup takes effect).
  • browser_console bypass (guard.py:40): removed from _SAFE_TOOLS. Added dispatch-level tests: browser_console(expression=...) now goes through Stage 1 LLM review while browser_snapshot stays bypassed.
  • Stage 2 exception honoring fail_open (guard.py:273-278): the exception branch returns a block directive when fail_open: false. Replaced the source-regex check with a behavior-level regression test.
  • Reviewer not read-only (stage2_acp.py): toolset restricted to -t session_search only; additionally the subprocess env now explicitly strips HERMES_YOLO_MODE so a parent gateway --yolo cannot leak past the approval layer. Also fixed the Windows timeout-kill path (os.killpg/SIGKILL fallback).
  • Import failure counted as pass (test_integration.py): import failures now fail the test. Added tests/plugins/test_approval_guard_plugin.py (18 cases): real plugin discovery, end-to-end pre_tool_call hook dispatch, failure branches, Windows behavior. Writing the dispatch test surfaced a real bug — the core dispatcher passes extra kwargs (turn_id etc.) that made the handler a silent no-op; fixed by accepting **_extra.
  • Native approve directive overlap: README now has a "Relationship with the Native approve Directive" section positioning this plugin as unattended dual-agent semantic review, complementary to the interactive human gate.

Tests: plugin suite 43 passed, new pytest suite 18 passed, ruff clean.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request type/security Security vulnerability or hardening

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants