fix: make gateway approvals human-readable - #46682
Conversation
alt-glitch
added
type/bug
teknium1
left a comment
teknium1
left a comment
There was a problem hiding this comment.
Thanks for preserving the approval UX work. The human-first goal is still needed on current main: Telegram still renders the command before its reason at plugins/platforms/telegram/adapter.py:4581-4585, and the fallback does the same at gateway/run.py:18668-18674.
Problems
- The adapter edits target paths removed by the bundled-platform migration in
5600105478ff; the active Telegram/Slack/Discord/Feishu/Matrix code is now inplugins/platforms/*/adapter.py. The PR test still imports the removed Telegram module attests/gateway/test_telegram_approval_buttons.py:50on the PR head. gateway/platforms/slack.py:2266retains a 2900-character preview while adding summary text to the same Block Kit section. That can exceed Slack's 3000-character section limit. Current main budgets this atplugins/platforms/slack/adapter.py:3259-3269.tools/approval.py:819labels curl/wget requests read-only unless-X/--requestis present.curl -d,-F, and--upload-filecan mutate without either flag.
Suggested changes
- Port the renderer changes and tests to the active plugin adapter paths, plus QQBot, WhatsApp Cloud, and
gateway/run.pyfallback. - Use per-surface output budgets and conservative HTTP classification with mutation-flag tests.
Automated hermes-sweeper review.
| @@ -2263,8 +2265,9 @@ async def send_exec_approval( | |||
| "type": "mrkdwn", | |||
| "text": ( | |||
There was a problem hiding this comment.
This section keeps the existing 2900-character preview and adds header/explanation/reason framing, so it can exceed Slack's 3000-character section-text limit and fail with invalid_blocks. Budget cmd_preview against the complete fixed framing; current main now does this in plugins/platforms/slack/adapter.py:3259-3269.
| if first in {"curl", "wget"} or " curl " in f" {lowered} ": | ||
| if re.search(r'\b(?:-X|--request)\s*(?:POST|PUT|PATCH|DELETE)\b', command, re.IGNORECASE): | ||
| return f"This will send a mutating HTTP request to {target or 'a network endpoint'}." | ||
| return f"This will make a read-only network request to {target or 'a network endpoint'}." |
There was a problem hiding this comment.
curl -d/--data, -F, and --upload-file can mutate a remote endpoint without -X or --request; this would tell the approver the request is read-only. Classify those flags as potentially mutating (or leave the effect unclear) and add regression coverage.
teknium1
added
sweeper:risk-security-boundary
GottZ
left a comment
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Two PRs touch approval-related behavior but address different root causes: #46682 restructures command approval prompts into human-readable summaries, while #63869 fixes QQBot direct-message routing by treating dm like c2c across outbound and authorization paths.
Related pull requests
- #46682
related— (+309/-16) — author action required: the diff adds a reusable approval-summary classifier and places explanations before raw commands across gateway adapters, directly addressing unreadable approval prompts. Despite thekeep_openreview on #46682, the current diff targets adapter paths removed by the bundled-platform migration in5600105478ff, imports the removed Telegram module in its test, can exceed Slack's 3000-character section limit, and incorrectly classifies mutatingcurl -d,-F, and--upload-filerequests as read-only; the renderer and tests remain salvageable if ported to the activeplugins/platforms/*/adapter.pypaths with those correctness issues fixed. - #63869 [closed]
related— (+36/-9) — duplicate fix: the diff consistently acceptsdmalongsidec2cin QQBot send, upload, keyboard, and interaction-authorization paths, matching the reported direct-message failure. Although closed, it remains relevant as evidence of the broad fix and as a duplicate of #40705, which covers the same paths through a sharedis_private_chat_type()helper rather than nine repeated checks.
Duplicates
#63869 is a broad-scope duplicate of #40705; #31593 is the narrower authorization-only predecessor and #41801 is another broad-scope sibling. #46682 is not a duplicate of #63869 because it changes approval-prompt presentation rather than QQBot DM routing.
Suggested consolidation
For #46682, author action: rebase onto main, or split out the part that can merge—specifically preserve the shared human-first renderer and regression coverage, port them to the active plugins/platforms/*/adapter.py implementations, budget Slack text within its section limit, and classify body/upload forms of curl as mutating. This explicitly narrows the keep_open review's salvage path into required author work rather than recommending the current stale, high-risk diff. Keep #63869 closed as duplicate of #40705, with the duplicate chain recorded as #63869 → #40705; the two target PRs should not be consolidated into one patch because they address different causes.
Cross-PR triage: Reviewed 2 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 29 kB of PR diffs, 2 kB of issue/PR text, 4 kB of discussion (3 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
4 participants
Summary
Context
This preserves the Hermes/ReBot approval UX hotfix that was verified in the running runtime at /opt/hermes. The goal is to make raw shell/code approval cards human-readable before the user sees the raw command.
Validation
Targeted pytest was attempted but the active local Python environment is missing PyYAML, so collection failed before tests ran.