fix(gateway): bind Discord exec-approval cards to their own request (#104915) - #104960
liuhao1024 wants to merge 1 commit into
Conversation
alt-glitch
added
type/bug
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
I do not think this immutable head is landable yet, despite exact CI/Docker/Nix being green.
The Discord request-binding direction is correct: a rendered approval card must settle the exact request generation it represents, never the session FIFO. But this change widens the shared send_exec_approval(...) call site by passing request_id= to every adapter while only updating a subset of adapter signatures. On current main, Slack, Teams, Matrix, and Feishu still expose the old signature, so those approval-notification paths will raise TypeError before rendering any prompt. This is an interface/compatibility regression that the hosted graph is not exercising.
There is also a class-completeness problem: WhatsApp Cloud and QQ still resolve approval button taps with resolve_gateway_approval(session_key, choice) (session FIFO), and this patch explicitly discards the newly available request id on those adapters (relay also discards it). That leaves the original overlapping/stale-card misapproval class alive on other native-button surfaces. The invariant should be platform-independent: rendered approval control × request_id × session × principal must resolve exactly one still-pending request; stale/unbound controls fail closed. Text /approve may retain its intentional FIFO semantics as a separate projection.
Please converge the adapter interface first (including external-plugin compatibility), carry request identity through every native interactive approval surface that can outlive or overlap another request, and add cross-adapter regressions. Separately, the Discord approval-card state is still being extended inside a >6K physical owner; under the repository's bounded-owner acceptance work this should move to a dedicated approval-card owner rather than growing plugins/platforms/discord/adapter.py further.
Exact object reviewed: 5fa571773f2452c8cdb39b34172442910f477191.
| chat_id=ctx._status_chat_id, command=cmd, session_key=ctx.session_key or "", | ||
| description=desc, metadata=ctx._status_thread_metadata, **flags, | ||
| description=desc, metadata=ctx._status_thread_metadata, | ||
| request_id=approval_data.get("request_id"), **flags, |
There was a problem hiding this comment.
This widens the call ABI for every status adapter, but current-main Slack, Teams, Matrix, and Feishu send_exec_approval(...) methods do not accept request_id. Those paths will raise TypeError before an approval prompt is rendered. Please either migrate the complete adapter/plugin interface in this carrier (with compatibility coverage for external adapters) or capability-gate the new argument rather than unconditionally passing it.
| ) -> SendResult: | ||
| """Approve / Deny buttons; a tap resolves via ``tools.approval.resolve_gateway_approval``.""" | ||
| del allow_permanent, allow_session # This adapter already offers one-shot Approve / Deny only. | ||
| del request_id # Request-bound buttons are wired for Discord only so far (#104915). |
There was a problem hiding this comment.
Dropping request_id here leaves the same stale/overlapping-button bug alive on WhatsApp: its inbound callback still calls resolve_gateway_approval(session_key, choice) and therefore resolves the FIFO request, not the card's request. Persist the request id in the interactive state/payload and resolve by it; QQ/relay and the other native-button adapters need the same class-level treatment rather than a Discord-only fix.
| request_id: Optional[str] = None, | ||
| ): | ||
| super().__init__(allowed_user_ids, allowed_role_ids, timeout=_read_discord_prompt_timeout()) | ||
| self.session_key = session_key |
There was a problem hiding this comment.
The request-bound state is the right semantic fix, but this adds another approval-policy coordinate to an adapter file already far beyond the repository's 2K physical-owner ceiling. Please extract ExecApprovalView / approval-card state into a bounded Discord approval module and keep the transport adapter as the projection/wiring surface.
|
Thanks for the thorough review — all three landability blockers are addressed in 0638c85 (rebased onto current main, which had moved 1. Interface convergence + external-plugin compatibility. All five remaining built-in signatures ( 2. Request identity on every native interactive surface. No adapter discards the id anymore:
The invariant is now platform-independent as specified: a rendered control resolves exactly its 3. Cross-adapter regressions. 4. Approval-card owner extraction — treated as the separate work the review scoped it to ( |
5fa5717 to
0638c85
Compare
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 0638c857490f61016266c0fe662cb94ac2ea950e after the material response to the prior review. The built-in convergence is substantially stronger now: all nine native interactive approval surfaces carry request identity, stale/unbound built-in controls fail closed, the Teams precheck/ignored-return window is closed, and the exact-head CI / Docker / Nix workflows are green. I also checked the current core resolver/request generation, the per-adapter regressions, the prior review threads, and the overlapping repository history.
I still see two landing blockers on this head:
-
The external/plugin compatibility path fixes the Python ABI by silently weakening the authorization invariant.
_accepts_keyword(...)omitsrequest_idfor a legacy external adapter and then still renders that adapter's interactive approval. That adapter can only fall back to session FIFO, so the exact stale/overlapping-control class this PR closes for built-ins remains reachable for third-party interactive approval surfaces. At an authorization UI boundary, unsupported request binding needs to fail closed rather than degrade invisibly. Either refuse the interactive control, route to a clearly separate typed-text FIFO path, or provide a compatibility carrier that preserves opaque request identity. Please add a legacy-plugin regression proving a stale/unbound rendered control cannot settle another request. -
WhatsApp splits one approval authority record across two different lifetimes.
_exec_approval_stateis intentionally FIFO-capped by_bounded_put, while_exec_approval_request_idsis a plain dict that is only popped on a tap. Once unanswered approval state is evicted from the bounded cache, its request-id half remains indefinitely, so the two coordinates drift and the new map grows without bound. Store(session_key, request_id)in the same bounded record or evict both atomically, and cover eviction beyondINTERACTIVE_STATE_CACHE_SIZE. The older #87554 carrier already used the single bounded tuple shape here, so that implementation deserves explicit credit rather than losing the useful part of its design.
Current landing state: head workflows are green, but the first surviving commit 1fa3cb74c4759c14d952f10ee5c294f4580e1eea has no hosted workflow receipts, so this two-commit train is still 1/2 hosted-green rather than fully proven. Also, live main has advanced to 76af5ebf0978f9978ad01e9cc860647e02ffe5ae; this branch is now 2 ahead / 24 behind with merge base 0d08cd295fd73427833ee349eb858569d4d0dd3a, so the repaired carrier needs fresh current-main integration proof before landing. I am not duplicating the existing live Discord >2K physical-owner thread here; it remains unresolved on the current diff.
Interlock / provenance: #6105 is the earlier credited exact-request-binding implementation; #68080 is its broader rewrite spanning messaging plus TUI/Desktop/Runs, but is now stale/conflicting; #87554 is a direct older native-adapter carrier for the same stale-button invariant and is also stale/conflicting. If #104960 is selected, it is the current-main superseding carrier for the native messaging slice; the broader non-native surfaces in #68080 remain complementary rather than being erased, and the prior authorship/design lineage should stay visible.
The response to the first review did real work: especially the Teams correction, complete built-in sweep, and stale/unbound fail-closed coverage. Close the two remaining authority/lifetime boundaries, rebase onto live main, and get every surviving commit hosted-green; then this is much closer to a coherent repository-wide approval binding. 🚀
| # Built-in adapters accept request_id (#104915); external/plugin adapters that | ||
| # predate the converged signature must keep receiving the legacy call shape. | ||
| extra: Dict[str, Any] = {} | ||
| if _accepts_keyword(adapter.send_exec_approval, "request_id"): |
There was a problem hiding this comment.
Capability-gating the kwarg fixes the ABI crash, but this branch silently downgrades authorization semantics for any legacy external adapter: it still renders an interactive approval without request_id, so the stale/overlapping-control class remains reachable through that adapter's session-FIFO response path. For a rendered authorization control, unsupported request binding should fail closed (or fall back to the explicitly typed FIFO path), not compatibly drop the identity coordinate. Please add a legacy-plugin regression proving no stale/unbound rendered control can settle another request.
| self._clarify_state: "OrderedDict[str, str]" = OrderedDict() | ||
| self._exec_approval_state: "OrderedDict[str, str]" = OrderedDict() | ||
| # approval_id -> gateway request id for request-bound exec-approval cards (#104915). | ||
| self._exec_approval_request_ids: Dict[str, str] = {} |
There was a problem hiding this comment.
This splits one approval record across two lifetimes. _exec_approval_state is FIFO-capped at INTERACTIVE_STATE_CACHE_SIZE via _bounded_put, but this new plain dict is only popped on a tap; cache eviction leaves the request-id half behind forever. After enough unanswered approvals this grows unbounded and the correlation maps drift. Keep (session_key, request_id) in the same bounded entry (the older #87554 carrier already used that shape) or evict both atomically, and add an over-cap eviction regression.
0638c85 to
6cceb5a
Compare
|
Thank you for the re-review — both remaining authority/lifetime boundaries are closed in 6cceb5a (squashed to a single commit, rebased onto live 1. Legacy/plugin compatibility path now fails closed. 2. WhatsApp authority record unified into one bounded lifetime. Provenance is kept visible in both the commit message and the PR body: #6105 as the first exact-request-binding implementation, #87554 as the native-adapter carrier whose bounded tuple design is adopted here, and #68080 as the complementary broader rewrite for the non-native surfaces. Head CI is running on the squashed commit; local |
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6cceb5a363cdad28092aee8915848fe7582a16e9 against the two remaining authority/lifetime blockers from my prior review. Both are actually closed on this object.
-
Legacy adapter behavior now fails closed at the rendering boundary.
TurnRunner._approval_notify_sync()checks whether the adapter'ssend_exec_approvalacceptsrequest_id. A legacy external/plugin adapter is no longer invoked to render an unbound interactive control; execution falls through to the explicitly separate typed-text approval path. The new convergence tests exercise both sides of that branch: the legacy adapter's interactive method is not called, while a converged adapter receives the immutable request id. That closes the silent authorization downgrade I identified. -
WhatsApp Cloud now gives card state and request authority one bounded lifetime.
_exec_approval_stateis a single FIFO-cappedOrderedDictwhose value is(session_key, request_id); send stores that tuple atomically, tap handling pops it atomically, an absent/unbound record resolves zero rather than FIFO, and the over-cap regression proves that 1,003 issued cards leave exactly the 1,000-entry cap with the oldest complete record evicted and the newest binding intact. There is no separate request-id map left to drift or grow without bound.
I also rechecked the surrounding request-generation and resolution path: every queued approval receives an immutable request id, the interactive path forwards it, and WhatsApp calls resolve_gateway_approval(..., request_id=request_id) only when bound. The two old bypasses are no longer reachable through these paths.
The submitted series is now one commit, and the hosted CI, Docker Build, Test, and Publish, and Nix flake check runs all concluded successfully for this exact head:
- CI: https://github.com/NousResearch/hermes-agent/actions/runs/34129123687
- Docker: https://github.com/NousResearch/hermes-agent/actions/runs/34129122931
- Nix: https://github.com/NousResearch/hermes-agent/actions/runs/34129123019
So the prior two blockers are resolved, and I found no replacement blocker in those fixes.
There is one separate landing-state condition: live main is now 1735ccde444355e1be11a2519f2224764e3f287e; this head is 119 commits behind it and GitHub currently reports the PR non-mergeable. The next rebase/conflict resolution will create a new exact object and invalidate these green receipts, so it needs fresh CI/Docker/Nix proof before landing. The existing Discord bounded-owner thread remains separate from the two blockers resolved here.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-reviewed exact head 6cceb5a363cdad28092aee8915848fe7582a16e9 against the two remaining authority/lifetime blockers from my 0638c85 review. Both are actually closed.
-
Legacy external/plugin adapters now fail closed rather than silently weakening the authorization invariant. In
gateway/run_turn_runner.py, the call site separateshas_exec_approvalfrombinds_requests; an adapter whosesend_exec_approvaldoes not acceptrequest_idno longer enters the interactive path at all and falls through to the typed-text approval surface.TestLegacyAdapterFailsClosedexercises the behavior, not just the signature: the legacy interactive method raises if invoked and is asserted never to run, the typed-text prompt is delivered with approval metadata, and the converged counterpart still receives the exact request id. That closes the prior ABI-compatible-but-authority-unsafe downgrade. -
WhatsApp Cloud now has one bounded lifetime for card state and request identity.
_exec_approval_request_idsis gone._exec_approval_stateowns(session_key, request_id)as oneOrderedDictrecord inserted through the existing bounded state path; tap handling pops that same record, resolves only with the bound request id, and treats an unbound record as resolved-zero rather than FIFO.test_request_binding_evicts_with_state_beyond_cache_capdrives the cache pastINTERACTIVE_STATE_CACHE_SIZE, proves the oldest record is evicted whole, and proves the newest tuple remains intact. This closes both the stranded-side-map leak and the lifetime drift, while preserving the useful bounded tuple design from #87554 with explicit provenance.
The branch is now one surviving commit and all three hosted workflows are green on that exact object:
- CI: https://github.com/NousResearch/hermes-agent/actions/runs/34129123687
- Docker Build, Test, and Publish: https://github.com/NousResearch/hermes-agent/actions/runs/34129122931
- Nix flake check: https://github.com/NousResearch/hermes-agent/actions/runs/34129123019
Live main has moved since the squash, but neither repaired owner (gateway/run_turn_runner.py nor gateway/platforms/whatsapp_cloud.py) changed in that drift, and current-main resolve_gateway_approval(...) still selects an exact matching request_id before settlement. I do not see a reason to reopen either finding for churn-only rebasing.
Disposition for this re-review: both remaining authority/lifetime blockers are closed on 6cceb5a; I have no new inline finding on those repairs. The previously separate Discord physical-owner thread is not being marked resolved by this review.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Re-review of exact head 6cceb5a363cdad28092aee8915848fe7582a16e9.
The two semantic blockers from the earlier review are fixed on this immutable object: the shared adapter ABI is converged without silently preserving unbound interactive controls for legacy adapters, and request identity now survives across the native interactive approval surfaces with stale/unbound controls failing closed. The exact required-check gate is green, and this is one surviving commit.
One landing blocker remains from the original review: the Discord authorization state still grows plugins/platforms/discord/adapter.py, already a >6K physical owner. That is not a cosmetic decomposition request; this code decides which physical effect an operator authorizes. Under the repository's bounded-owner acceptance contract, the request-bound approval-card owner needs to move into a dedicated bounded module with the transport adapter left as projection/wiring, and the existing Discord binding regressions should move with that owner.
I would keep this carrier and its now-correct cross-adapter semantics, do the bounded-owner extraction here, rerun the exact one-commit hosted graph, then re-review the immutable result. No second approval mechanism is needed.
| # A card resolves only the request it was issued for; an unbound card or one | ||
| # whose request is gone must never settle a different queued command (#104915). | ||
| # Text /approve keeps its FIFO semantics inside resolve_gateway_approval. | ||
| count = ( |
There was a problem hiding this comment.
The request-specific resolution is now semantically correct, but this is still authorization policy/effect-binding behavior inside a >6K transport owner. Please move ExecApprovalView / request-bound approval-card state and resolution into a dedicated bounded Discord approval module, with adapter.py only constructing/wiring the projection. The request-ID/stale-card regressions should follow that owner so this fixes the class without extending the godfile.
…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.
6cceb5a to
8931170
Compare
|
Bounded-owner extraction done in 8931170 (still one surviving commit on top of the same base; the hosted graph re-runs on it now).
Verified locally: the migrated binding regressions, the convergence suite, component-auth, view-base parity, lazy-install re-registration, prompt-timeout, and card-content tests all pass (68 tests); ruff clean on all touched files. No behavior change intended anywhere — this is a pure ownership move on top of the already-closed semantic fixes. |
3 participants
What does this PR do?
Discord execution-approval cards resolved whatever request was oldest in the session's gateway approval queue, not the request shown on the card. With two pending approvals A then B, clicking Allow Once on card B approved A; clicking a stale card A after its request had been removed settled the newer request B. An operator could therefore authorize a different command than the one displayed.
The fix threads the immutable
request_id(already generated for every queued entry by_ApprovalEntry) through the gateway notify path intoExecApprovalView, and resolves only that bound request. A card whose request no longer exists — or one constructed without a request id — fails closed into the existing "Approval expired" state instead of falling back to FIFO. Text/approvekeeps its FIFO semantics and/approve allis unchanged.Per review feedback (0638c85), the binding is converged across every native interactive approval surface, platform-independently: a rendered approval control resolves exactly its
request_id × session × principal, and stale/unbound controls fail closed. The shared call site gates the new keyword through_accepts_keyword.Per the follow-up re-review (6cceb5a), the two remaining authority/lifetime boundaries are closed:
send_exec_approvalpredates the converged signature is no longer rendered an interactive approval at all — its taps could only resolve through the session FIFO, silently re-opening the stale/overlapping-control hole at an authorization boundary. It degrades to the clearly separate typed-text prompt (with a warning naming the adapter) instead of an unbound card.(session_key, request_id)as ONE bounded FIFO record (the single-record tuple shape of the older fix: bind gateway approval buttons to request ids #87554 carrier): the binding is stored exactly when the card state is, and eviction beyondINTERACTIVE_STATE_CACHE_SIZEdrops both halves together — no stranded request-id half, no unbounded side map.The branch is now a single commit rebased onto live
main(6927ff8), so every surviving commit is the head that CI runs on.Authorship lineage (kept visible per review): the exact-request-binding design credit goes to #6105 (first implementation) and #87554 (native-adapter carrier, including the bounded tuple shape adopted here); the broader non-native-surfaces rewrite in #68080 remains complementary rather than superseded.
Related Issue
Fixes #104915
Type of Change
Changes Made
gateway/run_turn_runner.py—_approval_notify_syncpassesrequest_id=approval_data.get("request_id")toadapter.send_exec_approval; adapters whose signature predates the converged keyword fail closed to the typed-text prompt (warning logged) instead of rendering an unbound interactive control.plugins/platforms/discord/adapter.py—send_exec_approvalandExecApprovalViewaccept and storerequest_id;_resolvecallsresolve_gateway_approval(..., request_id=self.request_id)and treats an unbound card as resolved-zero (fail closed).plugins/platforms/slack/adapter.py— button value carriessession_key|request_id(slash-confirm convention); the click handler partitions it; unbound legacy cards fail closed.plugins/platforms/teams/adapter.py— AdaptiveCard action data carriesrequest_id; the resolver honors the request id and its return value instead of a session-level precheck (also fixes a stale-card "Approved" render window).plugins/platforms/feishu/adapter.py— interactive-card prompt state carriesrequest_id;_resolve_approvalbinds to it.plugins/platforms/matrix/adapter.py—_MatrixApprovalPromptcarriesrequest_id; the reaction resolver binds to it.plugins/platforms/telegram/adapter.py— the request id rides out-of-band next to the short monotonic callback id (64-bytecallback_datacap); the callback resolver binds to it.gateway/platforms/whatsapp_cloud.py— the tap state stores(session_key, request_id)as one FIFO-capped record via_bounded_put(12-hex payload id); the tap resolver unpacks the record and binds to it, so eviction drops state and binding together.gateway/platforms/qqbot/keyboards.py+gateway/platforms/qqbot/adapter.py— button data appends the hex request id after the decision; legacy 3-segment data still parses but resolves fail-closed.gateway/relay/adapter.py— minted prompt state carriesrequest_id;_resolve_exec_approvalbinds to it.tests/gateway/test_exec_approval_request_binding_convergence.py— AST-level contract test pinning all nine adapter signatures + the_accepts_keywordgate, plus behavior tests proving a legacy adapter'ssend_exec_approvalis never called (typed-text prompt with the adapter's typed prefix fires instead) while a converged adapter still receivesrequest_id.test_qqbot.py,test_slack_approval_buttons.py,test_telegram_approval_buttons.py,test_whatsapp_cloud.py(including the beyond-cap eviction regression),relay/test_relay_interactive.py,test_feishu_approval_buttons.py,test_matrix_exec_approval.py, and the Discord binding suite.How to Test
pytest tests/gateway/test_exec_approval_request_binding_convergence.py tests/gateway/test_discord_exec_approval_request_binding.py tests/gateway/test_approval_prompt_redaction.py -q— Observed result: 29 passed (nine-signature contract + gate unit test + legacy fail-closed/converged counterpart behavior tests + Discord binding suite + redaction wiring), on the squashed head rebased ontomain@6927ff8.pytest tests/gateway/test_qqbot.py tests/gateway/test_slack_approval_buttons.py tests/gateway/test_telegram_approval_buttons.py tests/gateway/test_whatsapp_cloud.py tests/gateway/test_feishu_approval_buttons.py tests/gateway/test_matrix_exec_approval.py tests/gateway/relay/test_relay_interactive.py -q— Observed result: 138 passed, including the newtest_request_binding_evicts_with_state_beyond_cache_cap(1003 cards → capped at 1000, oldest evicted whole, newest record intact).pytest tests/gateway/relay/test_relay_slack_prompt_dm_root.py tests/gateway/test_discord_approval_mentions.py tests/gateway/test_discord_exec_approval_content.py tests/gateway/test_discord_prompt_content_siblings.py tests/tools/test_approval.py tests/gateway/test_approve_deny_commands.py tests/relay/test_relay_prompt_ack_stream_isolation.py tests/run_agent/test_authorization_gate.py -q— Observed result: 182 passed, 1 failed (TestDetectDangerousRm::test_nonrecursive_verification_artifact_cleanup_is_not_dangerous), which fails identically on a cleanupstream/maincheckout (pre-existing, unrelated to this change).ruff checkon all touched files — all checks passed.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs