feat(mattermost): add interactive approval buttons via send_exec_approval - #29373
feat(mattermost): add interactive approval buttons via send_exec_approval#29373clean-logic wants to merge 3 commits into
Conversation
|
Competing with #26537 (by @shawnfeng0), which is broader in scope (covers |
1679ca0 to
7717c3d
Compare
|
hermes mattermost integration is dead without this! please approve this PR with a cherry on top |
1edb3fe to
9350833
Compare
teknium1
left a comment
There was a problem hiding this comment.
Thanks for carrying the Mattermost implementation into the bundled-plugin layout. The interactive gap is real on current main, but this needs correction before salvage.
Problems
plugins/platforms/mattermost/adapter.py:1059can returnSendResult(success=False)when the callback listener is unavailable. Currentgateway/run.py:14655-14664ignores that result, setssent_buttons = True, and suppresses the text fallback; the prompt is then marked pending atgateway/run.py:14678-14683.adapter.py:930resolves by session only.tools/approval.py:1488-1521resolves the oldest pending entry for that session, so a stale button can resolve a newer command approval.gateway/platforms/webhook.py:152-200adds an extra-route API with no PR caller. The adapter instead creates its own app atadapter.py:329-338; please remove the unused core hook.
Suggested changes
- Check
SendResult.successin the update watcher and add an end-to-end regression for listener startup failure. - Bind approval clicks to an exact approval ID, with a stale-button/newer-approval regression test.
- Keep the plugin-owned listener and drop the unused webhook change.
Automated hermes-sweeper review.
| if choice not in {"once", "session", "always", "deny"}: | ||
| raise ValueError(f"invalid approval choice: {choice!r}") | ||
| from tools.approval import resolve_gateway_approval | ||
| resolve_gateway_approval(payload["session_key"], choice, resolve_all=False) |
There was a problem hiding this comment.
This resolves only by session_key, but tools.approval.resolve_gateway_approval() resolves the session FIFO. A click on an old button can therefore resolve a newer command approval in the same session. Please plumb and resolve an exact approval ID before enabling this command-approval path.
There was a problem hiding this comment.
@.git_pr_reply_approval_id.md
| # Extra-route facility (used by sibling adapters, e.g. Mattermost) | ||
| # ------------------------------------------------------------------ | ||
|
|
||
| def register_extra_route(self, method: str, path: str, handler) -> bool: |
There was a problem hiding this comment.
The PR-head Mattermost adapter never calls register_extra_route; it starts a separate aiohttp app instead. Please remove this unused core hook rather than adding speculative infrastructure and middleware to every webhook request.
resolve_gateway_approval() resolves the oldest pending approval for a session (FIFO). With more than one approval pending in the same session, a stale button click can resolve the wrong one. Add _ApprovalEntry.approval_id (generated per entry, surfaced to notify callbacks via approval_data["approval_id"]) and resolve_gateway_approval_by_id(), which resolves only the exact matching entry and never falls back to FIFO on a stale/unknown id. Plumb approval_id through gateway/run.py's _approval_notify_sync into send_exec_approval, and accept (currently unused) approval_id on every adapter implementing that method -- Telegram, Teams, Slack, Matrix, Feishu, Discord, WhatsApp Cloud, QQ. Text /approve and /deny keep resolving FIFO unchanged; adopting by-id resolution per adapter is a separate, additive step (Mattermost adopts it next). Flagged in PR NousResearch#29373 review (teknium1): plugins/platforms/mattermost/ adapter.py:930 resolved by session only, which could let a stale button resolve a newer command approval. Co-authored-by: Cursor <cursoragent@cursor.com>
The update watcher set sent_buttons = True unconditionally whenever adapter.send_update_prompt() didn't raise, even when it returned a non-raising SendResult(success=False) (e.g. the adapter's interactive- button listener isn't running). That suppressed the plain-text fallback and left the prompt marked pending with no way for the user to answer it. Check the returned SendResult.success, matching the pattern already used for send_slash_confirm. Only the update-prompt path was missing this check -- the exec-approval notify path already handles it. Flagged in PR NousResearch#29373 review (teknium1): plugins/platforms/mattermost/ adapter.py:1059 can return SendResult(success=False) when its callback listener is unavailable, and gateway/run.py ignored that result. Co-authored-by: Cursor <cursoragent@cursor.com>
…oval Adds full interactive-button support to the Mattermost gateway plugin -- exec approval, slash confirm, update prompt, and clarify -- via a plugin-owned aiohttp callback server (the Teams/Line pattern). Zero core edits. Mattermost intercepts messages starting with "/", so the plain-text /approve fallback is unusable; buttons are the only working approval path (NousResearch#27587). - send_exec_approval accepts allow_permanent/allow_session/smart_denied (matching every other adapter's current contract) and hides Allow Session / Always Allow accordingly, mirroring Discord/Slack. - Exact approval-ID resolution: send_exec_approval accepts approval_id and stores it on the pending entry; _resolve_approval resolves via resolve_gateway_approval_by_id so a stale button can never resolve a different, newer pending approval in the same session. Falls back to legacy session-FIFO resolve for prompts registered before this adapter started stamping approval_id (e.g. across a gateway restart). Shows an honest "Approval expired" message on a stale tap. - Clarify renders Telegram-style: every option's full text is listed in the post body (numbered), with short numeric buttons (1, 2, ...) plus "Other (type answer)" -- Mattermost attachment button labels were getting cut off for long options. - Mass-mention hardening (matching the equivalent upstream commit) extended to the new prompt-posting paths: approval/slash/update/ clarify prompts and the slash follow-up post all disable_mentions, since the posted text can echo back agent- or user-influenced content that may contain "@channel"/"@ALL". - Reconciled against the 3 adapter.py commits upstream landed since this branch was cut: leading-space slash commands, per-platform multiplex config validation (validate_mattermost_config), and the mass-mention hardening above. - Dropped: the unused gateway/platforms/webhook.py register_extra_route hook (the adapter uses its own aiohttp app and never called it) and a latent bug in this branch's env-enablement seeding that silently discarded MATTERMOST_HOME_CHANNEL_NAME by omitting "name" from its home_channel seed dict -- neither is needed for this feature. Local HTTP callback server (three-tier callback_host/callback_port, default 127.0.0.1:18065), MATTERMOST_ALLOWED_USERS auth, double-click guard, original-message updates on click, bounded _pending_actions (oldest-evicted at 500 entries). Fixes NousResearch#27587. Supersedes NousResearch#26537 (plugin-native vs. the now-deleted core adapter it targeted). Addresses PR NousResearch#29373 review (teknium1): - adapter.py:930 resolved by session only -> exact approval_id above. - webhook.py register_extra_route had no caller -> removed. (SendResult.success handling for the exec-approval path was already correct upstream; the update-prompt watcher gap is fixed separately.) Co-authored-by: Cursor <cursoragent@cursor.com>
9350833 to
cad414c
Compare
|
Thanks for the review — rebased onto current RebaseUpstream advanced ~2,900 commits since this branch was cut, but only 3 touched
Review point 1 —
|
Summary
Adds full interactive-button support to the Mattermost gateway plugin — exec
approval, slash confirm, update prompt, and clarify — bringing it to parity with
Discord and beyond.
Re-architected onto the bundled-plugin model introduced in
af973e407(Mattermostmoved from
gateway/platforms/mattermost.pytoplugins/platforms/mattermost/).Interactive callbacks are served by a plugin-owned aiohttp server (the Teams/Line
pattern) — zero core edits.
What it does
send_exec_approval— Allow Once / Allow Session / Always Allow / Deny →resolve_gateway_approvalsend_slash_confirm— Approve Once / Always / Cancel →tools.slash_confirm.resolvesend_update_prompt— Yes / No update decisionssend_clarify— per-option buttons →resolve_gateway_clarifycallback_host/callback_port, default127.0.0.1:18065),MATTERMOST_ALLOWED_USERSauth, double-click guard,original-message updates on click.
Why buttons (not text)
Mattermost intercepts messages starting with
/, so the plain-text/approvefallback in #27587 is impossible to action. Buttons are the only working approval
path.
Relation to #26537
Supersedes @shawnfeng0's #26537 (same goal). This PR is plugin-native (his targets
the now-deleted core adapter), adds
send_clarify, full registration hooks, andtests + docs. Credit to his callback-server design and review exchange.
Fixes #27587 (primary item; the slash-command-registration secondary item is out of
scope).