Skip to content

fix(approval): bind interactive approvals to exact requests - #6105

Open
mrshu wants to merge 1 commit into
NousResearch:mainfrom
mrshu:mrshu/fix-approvals
Open

fix(approval): bind interactive approvals to exact requests#6105
mrshu wants to merge 1 commit into
NousResearch:mainfrom
mrshu:mrshu/fix-approvals

Conversation

@mrshu

@mrshu mrshu commented Apr 8, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Previously the interactive exec approval adapters reused the session-wide FIFO /approve resolver too literally. When a session had multiple pending approvals, a button click could resolve the wrong queued request, and some adapters bypassed the gateway's approval authorization check. This change binds each interactive prompt to a stable approval request id and reuses the gateway auth check before resolving the queue entry, so the button UI now matches the request the user actually clicked.

Related Issue

Related to #3915 and #503.

Hardens interactive approval integrity in the gateway button flows and also addresses the same general approval-routing/auth concerns seen in #1888.

Type of Change

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

Changes Made

  • Add request-bound approval entries in tools/approval.py and targeted resolve_gateway_approval(..., request_id=...) support
  • Thread approval request IDs and the runner's approval authorizer through gateway/run.py and gateway/platforms/base.py
  • Update Slack, Telegram, and Feishu approval handlers to store request-bound state and reject unauthorized button clicks
  • Extend the interactive approval tests to cover exact request binding and auth failures

How to Test

  1. Run uv run --with pytest --with pytest-asyncio python -m pytest -o addopts='' tests/gateway/test_approve_deny_commands.py tests/gateway/test_slack_approval_buttons.py tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_feishu_approval_buttons.py -q
  2. Confirm the suite passes (70 passed)
  3. Verify that button clicks resolve the intended pending approval and unauthorized button clicks are rejected

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've run pytest tests/ -q and all tests pass
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)
  • I've tested on my platform: macOS (Darwin 25.4.0)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) — or N/A
  • I've updated cli-config.yaml.example if I added/changed config keys — or N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows — or N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide — or N/A
  • I've updated tool descriptions/schemas if I changed tool behavior — or N/A

Screenshots / Logs

N/A

@mrshu
mrshu marked this pull request as ready for review April 8, 2026 10:05
@mrshu

mrshu commented Apr 8, 2026

Copy link
Copy Markdown
Contributor Author

@teknium1 another thing I came across -- feel free to let me know if this works

@mrshu
mrshu force-pushed the mrshu/fix-approvals branch from 2abe94a to a493093 Compare April 8, 2026 10:08
@alt-glitch alt-glitch added type/security Security vulnerability or hardening P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery labels Apr 30, 2026
@teknium1

teknium1 commented Jul 12, 2026

Copy link
Copy Markdown
Collaborator

Thanks for identifying the FIFO/request-binding issue. The underlying premise is still present on current main: tools/approval.py:1463-1489 resolves a session queue with queue.pop(0), while Slack sends session_key as its button value at plugins/platforms/slack/adapter.py:3287 and resolves it at :3669.

Problems

  • The PR's modified adapter paths no longer exist on current main. Commit 560010547 migrated Slack, Telegram, and Feishu to plugins/platforms/*/adapter.py, so this needs a substantive port rather than a clean cherry-pick.
  • A current-main port limited to the three original adapters would leave the same queue-targeting class in live interactive surfaces: Discord (plugins/platforms/discord/adapter.py:6920), Matrix (plugins/platforms/matrix/adapter.py:3268), QQBot (gateway/platforms/qqbot/adapter.py:1151), WhatsApp Cloud (gateway/platforms/whatsapp_cloud.py:1754), and Teams (plugins/platforms/teams/adapter.py:1069) all resolve with only a session key.

Suggested changes

  • Generate the request id when enqueuing the _ApprovalEntry, include it in the notify payload, and have every current interactive adapter resolve that exact entry.
  • Preserve the current adapter-local authorization checks while porting, and add exact-request tests for all supported button/reaction transports.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state 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 12, 2026
@mrshu
mrshu force-pushed the mrshu/fix-approvals branch from a493093 to b9daaed Compare July 12, 2026 18:03
@mrshu

mrshu commented Jul 12, 2026

Copy link
Copy Markdown
Contributor Author

@teknium1 Thanks for the detailed salvage guidance. I rebuilt this on current main and force-updated the PR as one focused commit.

  • Request IDs are created with the pending approval entry and propagated through gateway metadata.
  • Slack, Telegram, Feishu, Discord, Matrix, QQBot, WhatsApp Cloud, and Teams now resolve the exact request after their existing authorization checks.
  • Text and legacy approval paths retain FIFO behavior.
  • 480 targeted approval/adapter tests pass; lint and syntax checks pass.

The branch is ready for another look.

@mrshu
fix(approval): bind interactive approvals to exact requests
f72d7c0 
Previously interactive controls resolved the oldest queued approval; this
commit binds each control to its pending request without changing text FIFO.
Existing per-platform authorization checks remain ahead of resolution.

- Add stable request IDs and targeted gateway queue resolution
- Thread request IDs through all live interactive gateway adapters
- Cover exact queue and adapter payload routing in gateway tests
@mrshu
mrshu force-pushed the mrshu/fix-approvals branch from b9daaed to f72d7c0 Compare July 12, 2026 18:08
@hazeion hazeion mentioned this pull request Jul 20, 2026
Open
@andrexibiza andrexibiza mentioned this pull request Sep 7, 2026
Open
13 tasks
liuhao1024 added a commit to liuhao1024/hermes-agent that referenced this pull request Sep 7, 2026
@liuhao1024
fix(gateway): converge exec-approval request binding across all adapt…
6cceb5a 
…ers (NousResearch#104915)

Bind every interactive exec-approval card to the request generation it was
issued for, so a stale or overlapping control can no longer settle a different
pending approval:

- the entry side stamps an immutable request_id (uuid) onto every approval
  entry (tools/approval_gateway_wait.py) and the gateway notify path forwards
  it (gateway/run_turn_runner.py);
- all nine native interactive adapters (Discord, Slack, Teams, Matrix, Feishu,
  Telegram, QQBot, WhatsApp Cloud, relay) thread the keyword through
  send_exec_approval and resolve by request id, fail-closed when unbound;
- external/plugin adapters predating the converged signature fail closed to
  the typed-text prompt instead of rendering an unbound interactive control;
- WhatsApp Cloud keeps (session_key, request_id) as ONE bounded FIFO record
  (single-record tuple shape per the older NousResearch#87554 carrier), so eviction
  beyond INTERACTIVE_STATE_CACHE_SIZE drops both halves together.

Authorship lineage: exact-request-binding design credit goes to NousResearch#6105 (first
implementation) and NousResearch#87554 (native-adapter carrier incl. the bounded tuple
shape adopted here); the broader non-native surfaces rewrite in NousResearch#68080 stays
complementary.
liuhao1024 added a commit to liuhao1024/hermes-agent that referenced this pull request Sep 7, 2026
@liuhao1024
fix(gateway): converge exec-approval request binding across all adapt…
8931170 
…ers (NousResearch#104915)

Bind every interactive exec-approval card to the request generation it was
issued for, so a stale or overlapping control can no longer settle a different
pending approval:

- the entry side stamps an immutable request_id (uuid) onto every approval
  entry (tools/approval_gateway_wait.py) and the gateway notify path forwards
  it (gateway/run_turn_runner.py);
- all nine native interactive adapters (Discord, Slack, Teams, Matrix, Feishu,
  Telegram, QQBot, WhatsApp Cloud, relay) thread the keyword through
  send_exec_approval and resolve by request id, fail-closed when unbound;
- external/plugin adapters predating the converged signature fail closed to
  the typed-text prompt instead of rendering an unbound interactive control;
- WhatsApp Cloud keeps (session_key, request_id) as ONE bounded FIFO record
  (single-record tuple shape per the older NousResearch#87554 carrier), so eviction
  beyond INTERACTIVE_STATE_CACHE_SIZE drops both halves together;
- the Discord request-bound approval-card owner lives in its topical module
  plugins/platforms/discord/exec_approval.py (admin gate, card payload, and
  view factory on the adapter's shared component base), leaving the >6K-line
  transport adapter as projection/wiring; the Discord binding regressions
  moved with it to tests/plugins/platforms/.

Authorship lineage: exact-request-binding design credit goes to NousResearch#6105 (first
implementation) and NousResearch#87554 (native-adapter carrier incl. the bounded tuple
shape adopted here); the broader non-native surfaces rewrite in NousResearch#68080 stays
complementary.
@cresslank cresslank mentioned this pull request Sep 20, 2026
Open
19 tasks

This branch has not been deployed

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

Reviewers

No reviews

Assignees

No one assigned

Labels

comp/gateway Gateway runner, session dispatch, delivery P2 Medium — degraded but workaround exists 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 sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/security Security vulnerability or hardening

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mrshu @teknium1 @alt-glitch