Skip to content

fix(approval): honor permanent approval gating - #61378

Open
lazydao wants to merge 2 commits into
NousResearch:mainfrom
lazydao:agent/approval-permanent-choice
Open

fix(approval): honor permanent approval gating#61378
lazydao wants to merge 2 commits into
NousResearch:mainfrom
lazydao:agent/approval-permanent-choice

Conversation

@lazydao

@lazydao lazydao commented Jul 9, 2026

Copy link
Copy Markdown

Summary

Honors allow_permanent=false across gateway approval delivery paths.

When Tirith or another pre-approval policy disallows permanent approval, gateway clients must not offer an always action in API events, button UIs, reactions, or text instructions.

Details

  • Builds API approval event choices dynamically.
  • Forwards allow_permanent through the gateway dispatcher while preserving compatibility with legacy adapters.
  • Omits approve always from the generic text fallback.
  • Hides permanent approval in Feishu, Telegram, Slack, Matrix, Teams, QQBot, and Discord.
  • Documents the optional adapter contract.
  • Adds direct dispatcher, API-choice, and fallback regression tests.
  • Adds platform-specific tests for every updated native approval surface.

WhatsApp Cloud is unchanged because its approval UI only exposes Approve and Deny and has no permanent action.

Validation

  • 8 changed-surface test files: 324 passed
  • 5 compatibility/API regression files: 148 passed
  • Total targeted tests: 472 passed
  • ruff check on all changed Python files
  • git diff --check

@lazydao
lazydao force-pushed the agent/approval-permanent-choice branch from d0a3f44 to c270ffa Compare July 9, 2026 08:45
@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/gateway Gateway runner, session dispatch, delivery platform/feishu Feishu / Lark adapter sweeper:risk-message-delivery Sweeper risk: may drop, duplicate, misroute, or suppress messages labels Jul 9, 2026
@alt-glitch

Copy link
Copy Markdown
Collaborator

This was generated by AI during triage.

Related: gateway-side companion of the merged #44534 (which carried allow_permanent to TUI + desktop approval prompts). This PR does the equivalent for the gateway transports (API server events, text fallback, Feishu interactive cards). Not a duplicate — different surface. Also relevant to the allow_permanent/permanent-approval discussion in #39187 / #39212.

@teknium1

Copy link
Copy Markdown
Contributor

Thanks for carrying the allow_permanent contract into gateway delivery paths. The reported mismatch is real: tools/approval.py:2801-2803 emits allow_permanent=False for Tirith findings, while current API events (gateway/platforms/api_server.py:4315), the generic fallback (gateway/run.py:18553), and Feishu cards (plugins/platforms/feishu/adapter.py:2021) still expose permanent approval.

Problems

  • This is incomplete across native gateway approval surfaces. Telegram (plugins/platforms/telegram/adapter.py:4574-4582), Slack (plugins/platforms/slack/adapter.py:3281-3307), Matrix (plugins/platforms/matrix/adapter.py:2014-2018), Teams (plugins/platforms/teams/adapter.py:1122-1145), QQBot (gateway/platforms/qqbot/adapter.py:2656-2687), and Discord's ExecApprovalView still expose or advertise an always action.
  • The new Feishu test verifies card construction, but not the gateway/run.py forwarding path, API event choices, or text fallback behavior changed here.

Suggested changes

  • Carry the optional allow_permanent argument through each native approval UI that presents permanent approval, and hide its corresponding action/instruction when false.
  • Add dispatcher/API/fallback regression tests in addition to the Feishu card test.

Automated hermes-sweeper review.

@lazydao
lazydao force-pushed the agent/approval-permanent-choice branch from c270ffa to d260efb Compare July 10, 2026 22:44
@lazydao
lazydao marked this pull request as ready for review July 10, 2026 22:44
@teknium1 teknium1 added sweeper:risk-security-boundary Sweeper risk: may affect sandboxing, auth, credentials, or sensitive data sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 11, 2026

lazydao commented Jul 11, 2026

Copy link
Copy Markdown
Author

Thanks for the review. The requested gateway coverage was added in d260efb20, which was pushed after the sweeper comment:

  • propagated allow_permanent through Telegram, Slack, Matrix, Teams, QQBot, Discord, Feishu, the API server, and the generic fallback;
  • hides each permanent action and instruction when allow_permanent is false;
  • added dispatcher/API/fallback and platform-specific regression tests.

Ruff and the approval-focused gateway test suites pass. Please re-review the updated head.

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 P2 Medium — degraded but workaround exists platform/feishu Feishu / Lark adapter sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform 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/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants