Skip to content

fix(approval): show human gateway approval reasons - #34629

Open
viggomeesters wants to merge 5 commits into
NousResearch:mainfrom
viggomeesters:main
Open

fix(approval): show human gateway approval reasons#34629
viggomeesters wants to merge 5 commits into
NousResearch:mainfrom
viggomeesters:main

Conversation

@viggomeesters

Copy link
Copy Markdown

Summary

  • expand dangerous-command approval labels into human-readable gateway reasons
  • show visible fallback reason for empty descriptions in Telegram approval prompts
  • preserve repo-local go-workflow ignore entries while merging current upstream main

Test plan

  • HERMES_CRON_SESSION= python3 -m pytest tests/tools/test_approval.py tests/gateway/test_telegram_approval_buttons.py -q -o 'addopts='

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

@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 addressing an approval prompt that currently exposes terse detector labels.

Problems

  • Current main now constructs gateway approval text through the combined guard at tools/approval.py:2790-2825; the patch predates that route. A salvage needs to expand the presentation description there while retaining the stable approval pattern_key.
  • The patch hard-codes Dutch strings in tools/approval.py and gateway/platforms/telegram.py, while language selection is configured through agent/i18n.py:212-252. This would change the default English prompt regardless of display.language.
  • The approval fix is bundled with 68 changed files / +3,835 lines, including .go-workflow/runtime/go_workflow/__main__.py:1-1407, which is unrelated to this behavior.

Suggested changes

  • Keep only the focused approval change; exclude the workflow files and unrelated ignore rules.
  • Port reason formatting to the current combined gateway path and add an end-to-end gateway-payload test.
  • Use English defaults or agent.i18n.t() for localized static UI text.

Automated hermes-sweeper review.

Comment thread tools/approval.py
return (False, None, None)


def format_approval_description(command: str, description: str | None) -> str:

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.

These hard-coded Dutch strings are used beyond Telegram, but current language selection is centralized in agent/i18n.py. Please keep the default English or add translation keys and render through t() so display.language remains authoritative.

@@ -0,0 +1,1407 @@

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 1,407-line workflow runtime is unrelated to the approval-reason fix. Please remove it from this PR so the approval change can be reviewed and salvaged independently.

@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-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026
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-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades 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.

3 participants