Skip to content

fix(approval): explain requested action and permission - #51397

Open
TMShiroiko wants to merge 1 commit into
NousResearch:mainfrom
TMShiroiko:main
Open

fix(approval): explain requested action and permission#51397
TMShiroiko wants to merge 1 commit into
NousResearch:mainfrom
TMShiroiko:main

Conversation

@TMShiroiko

Copy link
Copy Markdown

Summary

  • Add shared approval explanation text for what Hermes is trying to do and what permission is requested
  • Surface that context in CLI, gateway fallback text, ACP, and approval-capable platform adapters
  • Add coverage for ACP and Telegram approval prompts

Test plan

  • uv run pytest tests/acp/test_permissions.py tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_slack_approval_buttons.py tests/gateway/test_matrix_exec_approval.py -q
  • uv run pytest tests/gateway/test_feishu_approval_buttons.py tests/gateway/test_qqbot.py -q
  • python -m py_compile tools/approval.py gateway/run.py plugins/platforms/discord/adapter.py plugins/platforms/slack/adapter.py plugins/platforms/telegram/adapter.py plugins/platforms/feishu/adapter.py plugins/platforms/matrix/adapter.py acp_adapter/permissions.py cli.py

@alt-glitch alt-glitch added type/feature New feature or request comp/tools Tool registry, model_tools, toolsets comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/acp Agent Communication Protocol adapter comp/plugins Plugin system and bundled plugins platform/discord Discord bot adapter platform/feishu Feishu / Lark adapter platform/matrix Matrix adapter (E2EE) platform/slack Slack app adapter platform/telegram Telegram bot adapter P3 Low — cosmetic, nice to have sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data labels Jun 23, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related / competing-PR cluster for plain-language approval explanations (all address #34625 / #6959 / #27604):

Not marking any as a duplicate — the platform-specific PRs could build on this shared layer. Flagging the cluster so a maintainer can pick a canonical direction.

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review Summary

Verdict: Approved

Excellent UX improvement: approval prompts now explain what Hermes is trying to do and what permission the approval grants. This is implemented consistently across all platform adapters (CLI, Discord, Feishu, Matrix, Slack, Telegram, ACP).

Looks Good

  • New build_approval_explanation() in tools/approval.py provides platform-neutral wording
  • All 7 platform adapters receive the explanation via metadata["approval_explanation"]
  • CLI panel correctly accounts for explanation height in layout calculations
  • Tests cover both the ACP bridge and Telegram adapter
  • The explanation text is clear and actionable for end users

Reviewed by Hermes Agent

@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 tackling a real approval-UX gap. Current main still renders only the command and detector reason in CLI (cli.py:11706-11812), gateway fallback (gateway/run.py:18669-18674), and ACP (acp_adapter/permissions.py:83-91).

Problems

  • tools/approval.py:1668 in this PR builds a user-visible explanation from raw command / combined_desc. The helper includes the description in its permission text, but current redaction is deliberately applied before callback and gateway rendering (tools/approval.py:1707-1718, gateway/run.py:18624-18630). This introduces an unredacted egress for secrets contained in a detector description; tests/tools/test_approval.py:2400-2418 covers that exact redaction contract.
  • The generic dispatch reaches every adapter with send_exec_approval (gateway/run.py:18635-18643), but WhatsApp (gateway/platforms/whatsapp_cloud.py:801), Teams (plugins/platforms/teams/adapter.py:1094), QQBot (gateway/platforms/qqbot/adapter.py:2664), and the TUI approval event/render path are not updated.

Suggested changes

  • Derive the explanation from redacted display values, or redact explanation fields at every egress boundary; add a secret-description regression test.
  • Carry the structured explanation through all existing approval adapters and the TUI/API approval payload contract, with sibling-path tests.

Automated hermes-sweeper review.

Comment thread tools/approval.py
"pattern_key": primary_key,
"pattern_keys": all_keys,
"description": combined_desc,
"explanation": build_approval_explanation(command, combined_desc),

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.

combined_desc is raw here, and build_approval_explanation() appends it to the externally rendered permission text. Current main deliberately redacts approval command and description before display (prompt_dangerous_approval, gateway egress); construct this from the same redacted fields or redact explanation before every send, then add a secret-in-description regression test.

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

Labels

comp/acp Agent Communication Protocol adapter comp/cli CLI entry point, hermes_cli/, setup wizard comp/gateway Gateway runner, session dispatch, delivery comp/plugins Plugin system and bundled plugins comp/tools Tool registry, model_tools, toolsets P3 Low — cosmetic, nice to have platform/discord Discord bot adapter platform/feishu Feishu / Lark adapter platform/matrix Matrix adapter (E2EE) platform/slack Slack app adapter platform/telegram Telegram bot adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants