fix(approval): fire approval hooks on smart-mode auto approve/deny - #62021
fix(approval): fire approval hooks on smart-mode auto approve/deny#62021hellno wants to merge 2 commits into
Conversation
In approvals.mode=smart, the auxiliary LLM (_smart_approve) can return "approve"/"deny"/"escalate". Previously only the "escalate" path (falling through to the manual/gateway prompt) fired the pre_approval_request / post_approval_response plugin hooks. The auto-approve and auto-deny branches returned BEFORE firing any hook, so every approval observer (e.g. the bundled nemo_relay plugin, notifiers, audit sinks) silently missed the majority of permission decisions made on smart-mode surfaces. Fire both hooks inside the smart approve/deny branches at BOTH _smart_approve call sites: - check_all_command_guards (terminal command guard) - check_execute_code_guard (execute_code whole-script guard) The hooks are pure observers: _fire_approval_hook already swallows all errors, and the verdict, returned dict, and approve_session ordering are unchanged. New signals are additive: surface="smart", choice="smart_approve"|"smart_deny", decided_by="aux_llm" — existing observers keep working. The observer-facing payload is redacted at both sites (smart mode also runs in gateway/Discord/Slack sessions that can forward the payload off-box), matching the gateway path. The raw command is still what _smart_approve assesses and what executes; redaction is display-only and the tool-facing return dicts are unchanged. Regression tests assert both hooks fire with surface="smart" and the correct choice for auto-approve and auto-deny at both sites; that escalate still reaches the manual prompt (surface="cli", not "smart"); that a crashing observer never flips the verdict; and that the payload is redacted.
teknium1
left a comment
There was a problem hiding this comment.
Thanks for tracing both smart-approval call sites; the premise is confirmed on current main: tools/approval.py:2744-2763 and tools/approval.py:3049-3067 return auto decisions before the existing hook emitters (tools/approval.py:2453, 2524, 2898, 2910).
Problems
tools/approval.py:2754invokesredact_sensitive_text()outside the observer error boundary and before checking the verdict. A redactor/import failure can therefore alter the smart approve/deny/escalate flow, contrary to the stated observer-only guarantee. Add a fail-safe redacted sentinel and a regression test that forces redaction to raise.- The public hook contract also needs updating.
website/docs/user-guide/features/hooks.md:1063-1089documents only CLI/gateway prompt surfaces, and:1136omits the new smart choices.
Suggested changes
- Make the new terminal smart-hook payload construction failure-isolated without ever falling back to raw secret-bearing text.
- Document
surface="smart",smart_approve/smart_deny, anddecided_by="aux_llm"in the hooks guide.
Automated hermes-sweeper review.
| # unchanged. surface="smart" and the smart_* choices are additive. | ||
| # Redact the payload like the gateway path: smart mode also runs in | ||
| # gateway sessions that forward it off-box. The raw command still runs. | ||
| from agent.redact import redact_sensitive_text |
There was a problem hiding this comment.
This redaction call is outside _fire_approval_hook()'s error boundary and runs before the approve/deny/escalate branches. If its import or execution fails, it can abort the decision or manual fallback. Please catch this here, preserve the verdict, and use a non-secret sentinel rather than raw command text; add a forced-redactor-failure regression test.
There was a problem hiding this comment.
Thanks, both are fair. Fixed in 9d951e4.
-
Redaction was running before the verdict branch, so it also ran on escalate and could abort the defer-to-human fall-through. Moved the observer payload prep inside the
if verdict in ("approve","deny")gate, so escalate never touches observer code now. I didn't add the sentinel/try-except: on approve/deny a redact failure is consistent with how the rest of this file treatsredact_sensitive_text(called unguarded wherever its output is load-bearing), and the escalate path is now fully isolated. The execute_code site already reuses the existing display copies, so it needed no change. Added a differential regression test: on escalate the smart branch adds zero redaction over the manual-prompt baseline (two extra before). -
Documented
surface="smart",choicesmart_approve/smart_deny, anddecided_by="aux_llm"infeatures/hooks.md.
…ent smart surface
Addresses review feedback on the smart-mode approval hooks:
- check_all_command_guards: move the observer-only payload redaction inside the
`if verdict in ("approve","deny")` gate. It previously ran before the gate, so
it also executed on the escalate path; if redact raised there the exception
would propagate and abort the fall-through to the manual prompt, violating the
observer-only contract. Escalate now never touches observer code. No try/except
is added: on approve/deny a redact failure is consistent with the file's
existing trust model for redact_sensitive_text, and the safety-critical
defer-to-human path is fully isolated. check_execute_code_guard already reuses
the load-bearing display copies, so it needs no change.
- Add a differential regression test: on escalate the smart branch adds no
redaction over the manual-prompt baseline (pre-fix it redacted twice more,
up front).
- Document surface="smart", choice="smart_approve"/"smart_deny", and
decided_by="aux_llm" in the public hooks guide (features/hooks.md).
|
Merged via #63503 with your authorship preserved. Smart approve/deny decisions now emit force-redacted pre/post observer hooks across terminal and execute_code, with hook/redactor failures isolated from verdicts. Thank you for identifying and implementing the missing observability path. |
Summary
Before Hermes runs a risky command, it goes through an approval step. Plugins
can subscribe to that step (via the
pre_approval_request/post_approval_responsehooks from #16776) to send a notification, write anaudit log, or whatever else they need.
approvals.mode=smartadds an option where a small "guard" LLM decidesapprove-or-deny automatically instead of asking the human. The problem: when the
guard auto-decides, none of those subscription hooks fire. They fire only on the
leftover case, where the guard is unsure and hands the decision back to the
human.
Auto-deciding is the whole point of smart mode, so a subscribed plugin misses
almost every decision. It hears about the handful the guard punts on and nothing
else. This PR fires the hooks on the auto approve/deny decisions too.
Design notes
Approval is safety-critical, so the hooks are pure observers: they run alongside
the decision and never change it. The verdict, the value handed back to the
agent, and the order approvals get recorded are all the same as before. If a
subscribed plugin throws, the error is swallowed and the command still approves
or denies exactly as the guard decided. The new fields (
surface, thechoice,who decided) are added, not renamed, so plugins written against the old
CLI/gateway events keep working with no changes.
The one real tradeoff is what the command text looks like in the event. Plugins
forward these events off the machine (to Slack, a webhook, a log service), and a
command can contain a secret. Sending the raw command already caused a
credential leak once (#48456), so the event carries the redacted copy instead,
the same one the gateway prompt already shows. The guard still reads and runs
the real command; only the observer's copy is masked. And because that masking
step exists only to build the event, it is wrapped so that even if it failed
somehow, it could never break the actual approve/deny.
Two things I left out on purpose. The guard knows why it approved or denied, but
that reason isn't part of what it currently returns, so the event can't carry it
yet. Threading it through means changing what the guard returns, which is a
bigger, separate change. Also, once the guard approves a pattern, later reuses
skip the check completely. I don't fire on those, so you get one event when the
decision is actually made, not a duplicate every time the same command runs
again.
Changes
tools/approval.py: firepre_approval_request+post_approval_responseinthe approve/deny branches of both
_smart_approvecall sites:check_all_command_guards(terminal commands) andcheck_execute_code_guard(execute_code scripts).
surface="smart",choice="smart_approve"|"smart_deny",decided_by="aux_llm".hermes_cli/plugins.py: document the newsurface/choice/decided_byvalues in the
VALID_HOOKSreference.Testing
TestSmartModeFiresHooks: both hooks fire withsurface="smart"and thecorrect
choiceon auto-approve and auto-deny, at both sites;escalatestill reaches the manual prompt; a crashing observer never changes the
verdict; the payload is redacted; a redact failure can't break the decision.
approval and plugin suites pass unchanged.