Skip to content

fix(approvals): show purpose, effect, and risk in approval prompts - #75795

Open
troyechung wants to merge 2 commits into
NousResearch:mainfrom
troyechung:feat/approval-command-context
Open

fix(approvals): show purpose, effect, and risk in approval prompts#75795
troyechung wants to merge 2 commits into
NousResearch:mainfrom
troyechung:feat/approval-command-context

Conversation

@troyechung

Copy link
Copy Markdown

What does this PR do?

Rebuilds the intent of #22363 against the current gateway approval architecture.

Dangerous-command approval prompts currently expose the command and a guard reason, but not enough context for users to understand why the command is needed, what it changes, or what could go wrong. This PR surfaces model-provided Purpose / Effect / Risk in the same actionable approval prompt while keeping the system-generated risk assessment separate.

Model-provided context is explicitly labeled as unverified, length-limited, sanitized, and credential-redacted. The gateway also redacts again at the outbound delivery boundary. Delivery failures with unknown status fail closed and remove the pending approval entry.

Related Issue

Refs #22363

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 🔒 Security fix
  • ✅ Tests (adding or improving test coverage)

Changes Made

  • Added optional approval purpose, effect, and risk fields to the terminal tool schema and guard pipeline.
  • Added sanitized, unverified model context to CLI approval prompts.
  • Combined system risk and model context into the same gateway approval description.
  • Removed the separate gateway follow-up message, avoiding ordering and association problems.
  • Preserved button-based prompts and the single-message text fallback without changing adapter signatures.
  • Added defense-in-depth credential redaction at the outbound delivery boundary.
  • Added strict delivery-error propagation and pending-entry cleanup when approval delivery fails.
  • Preserved existing host-access propagation.
  • Kept request-ID binding and approval response routing out of scope for this PR.

How to Test

Run the related approval and gateway suites:

python -m pytest \
  tests/tools/test_command_guards.py \
  tests/gateway/test_approval_context_message.py \
  tests/gateway/test_approval_prompt_redaction.py \
  tests/tools/test_hardline_blocklist.py \
  tests/tools/test_approval_plugin_hooks.py \
  -q -n 0

Result: 249 passed.

Additional checks performed:

  • git diff --check
  • conflict-marker scan
  • credential-pattern scan
  • py_compile for tools/approval.py and gateway/run.py

Checklist

Code

Documentation & Housekeeping

  • I've updated relevant docstrings
  • cli-config.yaml.example — N/A, no config keys changed
  • CONTRIBUTING.md / AGENTS.md — N/A, no contributor workflow changed
  • I've considered cross-platform impact across button and text adapters
  • I've updated terminal tool descriptions and schema fields

Screenshots / Logs

The targeted test suites pass with 249 tests. No UI screenshots are included because the behavior is covered at the shared CLI and gateway delivery boundaries.

sqp-adicr and others added 2 commits July 31, 2026 21:55
Add optional purpose/effect/risk context fields to terminal tool calls and thread them through command approval requests. Gateway approval prompts keep the existing request message intact and send a follow-up context message when explanation data is available.
Enhance approval prompts with model-supplied Purpose/Effect/Risk context
co-located in a single message — no standalone follow-up.

- tools/approval.py: _sanitize_explanation, _build_enhanced_description_with_context,
  enhanced_desc replaces combined_desc on all user-facing surfaces
- gateway/run.py: extract _deliver_approval_message (module-level, testable);
  DeliveryError for fail-closed delivery; button success returns, button
  explicit failure falls through to text, unknown/None/timeout raises DeliveryError
- prompt_dangerous_approval: CLI displays unverified-context annotation
- tests: production-delivery, redaction, fail-closed, E2E, AST wiring
@alt-glitch alt-glitch added type/feature New feature or request comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets tool/terminal Terminal execution and process management P3 Low — cosmetic, nice to have needs-decision Awaiting maintainer decision before any implementation sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Aug 1, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related to #22363: both make dangerous-command approvals more understandable, but the current diff uses one sanitized/redacted approval message and fail-closed delivery cleanup rather than #22363's separate follow-up context message. This is a design choice for maintainers, not a duplicate.

@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 rebuilding the approval-context idea around the current single-message gateway path. Current main still sends only the command and detector reason (gateway/run.py:4817-4889), so the feature has a real target.

Problems

  • Blocking — tools/approval.py:3428-3434: the sanitizer removes every C0 control character before normalizing/splitting newlines. A value such as normal text\n/approve session becomes one line, so the anchored _FORGE_RE misses the forged approval instruction. The new test at tests/gateway/test_approval_context_message.py:385 covers only a forged first line.
  • Suggestion — tools/approval.py:3917-3921: enhanced_desc already embeds the context, then prompt_dangerous_approval() receives the same explanation separately. The direct renderer prints both (tools/approval.py:2502-2513), duplicating context in its no-callback fallback.

Suggested changes

  • Normalize CRLF first, retain LF while stripping other controls, then filter forged lines; add embedded-line and CRLF regression cases.
  • Render the context through one of the two prompt inputs, not both.

Automated hermes-sweeper review.

Comment thread tools/approval.py
if not value:
continue
value = redact_sensitive_text(value, force=True)
value = re.sub(r"[\x00-\x1f\x7f]", "", value)

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.

Blocking: this removes LF/CR before the normalization and line-based _FORGE_RE pass below. A value like normal text\n/approve session becomes one non-matching line, leaving a forged approval instruction in the user-visible context. Normalize line endings first and preserve LF while stripping the remaining controls; add a regression case with a benign first line.

Comment thread tools/approval.py
allow_permanent=has_permanent_capable and not smart_denied_for_owner,
smart_denied=smart_denied_for_owner,
approval_callback=approval_callback,
explanation=approval_explanation,

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.

enhanced_desc already embeds the model context, while the direct prompt renderer also prints explanation separately. In the no-callback fallback this duplicates the purpose/effect/risk block; pass only one representation to that renderer.

@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-broad Sweeper blast radius: broad — a core path most sessions hit labels Aug 1, 2026
@web3blind

Copy link
Copy Markdown
Contributor

This PR would also help with another approval-clarity case I just ran into: approvals generated by the pending write-approval helpers for skill/memory writes.

Example shape:

python - <<'PY'
from hermes_cli.write_approval_commands import handle_pending_subcommand
from tools import write_approval as wa
pid='...'
rec=wa.get_pending(wa.SKILLS, pid)
print('found', bool(rec), rec.get('summary') if rec else '')
if rec:
    print(handle_pending_subcommand(wa.SKILLS, ['approve', pid]))
print('remaining', wa.get_pending(wa.SKILLS, pid))
PY

Today the approval reason is technically correct (script execution via heredoc), but it is not the useful decision context for the user. The actual operation is something like:

  • approve pending skills write <id>
  • summary: patch '<skill-name>' SKILL.md (+N/-M lines) or write references/... in '<skill-name>'
  • effect: modifies persisted Hermes skill instructions/reference files
  • risk: changes future agent behavior / procedural memory

It would be great if the Purpose/Effect/Risk surface could either:

  1. recognize these pending write-approval wrappers and show the staged write summary, or
  2. let the write-approval path attach structured approval metadata before the dangerous-command guard renders the prompt.

That would keep the useful one-approval-per-staged-operation flow, but make the approval card human-readable: users approve the actual pending skill/memory change, not an opaque Python heredoc.

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

Labels

comp/gateway Gateway runner, session dispatch, delivery comp/tools Tool registry, model_tools, toolsets needs-decision Awaiting maintainer decision before any implementation P3 Low — cosmetic, nice to have sweeper:blast-broad Sweeper blast radius: broad — a core path most sessions hit sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data tool/terminal Terminal execution and process management type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants