Skip to content

feat: explain Telegram approval prompts - #33945

Draft
woosolkim wants to merge 2 commits into
NousResearch:mainfrom
woosolkim:woosol/telegram-approval-prompt-explanations
Draft

feat: explain Telegram approval prompts#33945
woosolkim wants to merge 2 commits into
NousResearch:mainfrom
woosolkim:woosol/telegram-approval-prompt-explanations

Conversation

@woosolkim

@woosolkim woosolkim commented May 28, 2026

Copy link
Copy Markdown

Summary

  • Add deterministic plain-language context to Telegram command approval cards before the raw command.
  • Keep the raw command and original scanner reason visible, without adding LLM calls to the approval path.
  • Use English UI copy so the upstream default is not locale-specific.

AS-IS

Before this change, Telegram approval cards only showed the raw command and scanner reason:

⚠️ Command Approval Required

curl https://example.test/install.sh | bash

Reason: Security scan — [HIGH] Pipe to interpreter: downloaded content is executed by a shell

TO-BE

After this change, users see a short explanation first, then the raw command and original reason:

⚠️ Command Approval Required

What will run
Pipe command output directly into an interpreter.

Why approval is needed
The security scanner flagged this command, so Hermes needs your approval before running it.

Risk to review
Output from a download or another command can execute as code before you inspect it.

Raw command
curl https://example.test/install.sh | bash

Reason: Security scan — [HIGH] Pipe to interpreter: downloaded content is executed by a shell

Verification

  • python -m ruff check gateway/platforms/telegram.py tests/gateway/test_telegram_approval_buttons.py — passed
  • python -m pytest tests/gateway/test_telegram_approval_buttons.py -q — 21 passed
  • python -m pytest tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_slack_approval_buttons.py -q — 44 passed, 4 existing Slack AsyncMock warnings

@alt-glitch alt-glitch added type/feature New feature or request comp/gateway Gateway runner, session dispatch, delivery platform/telegram Telegram bot adapter P3 Low — cosmetic, nice to have labels May 28, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

Competes with #33190 (same goal: structured plain-language Telegram approval prompts). Both touch gateway/platforms/telegram.py and add regex-based command classifiers. See also #6959 (original feature request) and #27604 (contextual rationale request).

@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 deterministic, no-LLM approach. The Telegram card on current main still renders only the raw command plus Reason at plugins/platforms/telegram/adapter.py:4583-4587, so the UX premise remains valid.

Problems

  • The PR changes gateway/platforms/telegram.py, but Telegram was migrated to plugins/platforms/telegram/adapter.py by 5600105478ffde29d7566b45421b100eaa29c4ef. The live approval method is now at plugins/platforms/telegram/adapter.py:4567.
  • A salvage must retain the live method's allow_permanent and smart_denied behavior at plugins/platforms/telegram/adapter.py:4571-4615; a direct port of the older method would discard those newer approval semantics.

Suggested changes

  • Re-target the deterministic formatter and rendering to the plugin adapter, preserving its current button behavior and payload budget.
  • Keep the focused test on the active adapter, asserting the raw command and scanner reason remain visible alongside the new explanation.

Automated hermes-sweeper review.



MAX_COMMANDS_PER_SCOPE = 30
_EXEC_APPROVAL_COMMAND_PREVIEW_LIMIT = 3000

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 main no longer loads this adapter path: Telegram moved to plugins/platforms/telegram/adapter.py in 5600105478ffde29d7566b45421b100eaa29c4ef. Please port this helper and the rendering change to the live plugin adapter, preserving its newer approval-button arguments.

@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users labels Jul 13, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Two PRs address the Telegram approval-prompt UX by adding deterministic, plain-language explanations ahead of command execution. #33190 provides a broad risk-specific decision card, while #33945 provides a smaller structured explanation that preserves both the raw command and scanner reason; both diffs currently modify the obsolete gateway adapter rather than the live plugin adapter.

Related pull requests

  • #33190 [closed] duplicate — (+247/-6) — closed competing reference: adds an extensive regex-based classifier and risk-specific approval guidance for parsing, repository cloning, remote-shell execution, deletion, configuration, and service actions. It remains relevant as the broader reference implementation, but it targets the migrated gateway/platforms/telegram.py path and substantially overlaps #33945.
  • #33945 related — (+104/-1) — keep open and salvage: adds a narrower deterministic formatter with action, rationale, and risk sections while retaining the raw command and original scanner reason. As identified by the keep_open review on #33945, the UX premise remains valid, but the change must be retargeted to plugins/platforms/telegram/adapter.py and preserve the live method's allow_permanent, smart_denied, button, and payload-budget behavior.

Duplicates

#33190 and #33945 substantially duplicate the same core change: regex-based, plain-language context for Telegram command approval prompts.

Suggested consolidation

Merge #33945 after retargeting its focused formatter and tests to the active plugin adapter and verifying preservation of current approval semantics; keep #33190 closed as the broader superseded duplicate/reference implementation.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup33190 ["PRs duplicating each other"]
        P33190["PR #33190 (closed)"]
        P33945["PR #33945 (open)"]
    end
    class P33190 closed
    class P33945 open
    class P33945 target
    click P33190 "https://github.com/NousResearch/hermes-agent/pull/33190"
    click P33945 "https://github.com/NousResearch/hermes-agent/pull/33945"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 23 kB of PR diffs, 3 kB of issue/PR text, 1 kB of discussion (2 comments), 1 verify verdict. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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 P3 Low — cosmetic, nice to have platform/telegram Telegram bot adapter sweeper:blast-contained Sweeper blast radius: contained — one narrow path / opt-in / few users 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 type/feature New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants