feat(telegram): handle plugin approval events in TelegramExecApprovalHandler#57340
feat(telegram): handle plugin approval events in TelegramExecApprovalHandler#57340nk3750 wants to merge 6 commits into
Conversation
Greptile SummaryThis PR extends Key changes:
Confidence Score: 5/5Safe to merge — no functional defects found; only a minor type-cast style note. The implementation is functionally correct. All fields accessed on PluginApprovalRequest and PluginApprovalRequestPayload (expiresAtMs, sessionKey, agentId, turnSource*) are confirmed to exist. The startsWith("plugin:") discrimination is an established codebase convention. The resolveExecApprovalSessionTarget cast is safe at runtime. The only remaining note is a P2 style suggestion about the type assertion, which does not affect correctness. No files require special attention.
|
| Filename | Overview |
|---|---|
| extensions/telegram/src/exec-approvals-handler.ts | Extends handler to subscribe to plugin approval gateway events, widening method signatures to accept union types and dispatching to the correct payload builder. Functionally correct; one minor type assertion worth noting. |
Prompt To Fix All With AI
This is a comment left during a code review.
Path: extensions/telegram/src/exec-approvals-handler.ts
Line: 128
Comment:
**Type assertion bypasses TS safety on the request parameter**
`resolveExecApprovalSessionTarget` is typed to accept `ExecApprovalRequest`, but receives a `PluginApprovalRequest` via a cast. This is safe today because the function only reads `.request.sessionKey` and `.request.agentId` — fields present in both payload types — but the cast will silently suppress any future type errors if the function's internal field access ever broadens.
Consider expanding `resolveExecApprovalSessionTarget` upstream to accept a shared interface, or extracting a minimal compatible shim object so TypeScript continues to guard this call site.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "feat(telegram): handle plugin approval e..." | Re-trigger Greptile
|
Thank you for putting this up. This looks directionally correct and matches the Discord-side behavior, but I am not merging it blind without the Telegram regression coverage. What still needs to happen before landing:
The remaining review note about the cast is not the blocker here; the missing test coverage is. Issue thread for the bug: #57339 |
There was a problem hiding this comment.
💡 Codex Review
https://github.com/openclaw/openclaw/blob/e9061c7f2220ef7285b0e8dc0395e58b39a9c4d8/extensions/telegram/src/bot-handlers.runtime.ts#L1367
Route
always approval callbacks through direct resolver
The new direct-resolution flow only runs inside if (isApprovalCallback), but that predicate comes from APPROVE_CALLBACK_DATA_RE, which matches allow-once|allow-always|deny while Telegram approval buttons emit /approve <id> always for the “Allow Always” action (extensions/telegram/src/approval-buttons.ts). As a result, “Allow Always” callbacks skip resolveApprovalDirect and fall back to processMessage; for plugin approvals this re-enters the session pipeline path that this change explicitly says deadlocks, so the callback can hang instead of resolving.
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 381167dce1
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 078dabebdc
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Approval callback_queries must bypass the per-chat sequential queue. The agent turn waiting for approval holds the chat lock; queuing the callback behind it causes a deadlock. Changes: - sequential-key.ts: approval callback_queries now route to a separate ':approval' key, running in parallel with the main agent pipeline (same pattern as ':control') - bot-handlers.media.ts: accept 'always' as a valid decision value alongside 'allow-always' in APPROVE_CALLBACK_DATA_RE This is part 2 of the Telegram plugin approval fix (Issue 1 and the sequentializer bypass). Part 1 (handler plugin.approval.* support) is addressed separately in PR openclaw#57340. Fixes openclaw#57339 (partial).
Added in 078dabe — 7 new plugin approval tests, all 3 existing exec approval tests remain green (10/10 passing). Tests cover:
|
…Handler TelegramExecApprovalHandler only listened for exec.approval.requested and exec.approval.resolved gateway events, silently ignoring plugin approval events. This caused plugin approval buttons on Telegram to deadlock — the approval prompt rendered inline in the session, and clicking the button routed through the same blocked session pipeline. The Discord handler (DiscordExecApprovalHandler) already supports both event types. This commit brings Telegram to parity by: - Subscribing to plugin.approval.requested and plugin.approval.resolved - Accepting ExecApprovalRequest | PluginApprovalRequest in handleRequested - Using buildPluginApprovalPendingReplyPayload for plugin approval messages - Accepting ExecApprovalResolved | PluginApprovalResolved in handleResolved The existing button builder (buildTelegramExecApprovalButtons) and callback_data parser already handle plugin:<uuid> approval IDs correctly.
…y client When a user clicks an approval button on Telegram, the callback_query is processed by the Grammy bot's update pipeline. However, the Grammy runner sequentializes all updates per chat — and the agent turn that triggered the approval is still "processing" (waiting for the approval decision). This means the callback_query update is queued behind the blocked agent turn, creating a deadlock at the runner level. This commit adds direct approval resolution in the callback handler: when an approval callback is detected, it resolves the approval via a lazily-initialized gateway client (calling plugin.approval.resolve or exec.approval.resolve), bypassing processMessage entirely. NOTE: This alone does not fully fix the deadlock — the Grammy runner sequentializer blocks the callback_query handler from executing at all. A pre-sequentializer middleware is needed to intercept approval callback_queries before they enter the per-chat sequential queue. This is noted as a TODO for follow-up.
078dabe to
c75b5a2
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 896fff44c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
b993dd3 to
59d6886
Compare
…pproval-followup.md
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 768ea99ad3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Plugin approvals block the session pipeline while waiting for a decision, so routing | ||
| // through processMessage would deadlock. Instead, call the gateway resolve method directly. | ||
| const parsed = parseApprovalCallbackDecision(data); | ||
| if (parsed) { | ||
| const ok = await resolveApprovalDirect(runtimeCfg, parsed.id, parsed.decision, senderId); |
There was a problem hiding this comment.
Route approval callbacks to a non-chat sequential key
This new direct-resolve path still depends on the callback handler running, but callback updates are still sequentialized on the same telegram:<chatId>[:topic] key as the in-flight turn waiting for approval (I checked extensions/telegram/src/sequential-key.ts). In same-chat approval flows (for example DM or channel-target approvals), the callback query stays queued behind the blocked turn, so resolveApprovalDirect(...) is never reached and the approval can deadlock until expiry.
Useful? React with 👍 / 👎.
| gatewayRuntimeHoisted.createOperatorApprovalsGatewayClient.mockResolvedValue({ | ||
| connect: gatewayRuntimeHoisted.connectSpy, | ||
| request: gatewayRuntimeHoisted.requestSpy, | ||
| start: gatewayRuntimeHoisted.startSpy, | ||
| stop: gatewayRuntimeHoisted.stopSpy, |
There was a problem hiding this comment.
Preserve onHello handshake in approval gateway test mock
The beforeEach override replaces the hoisted createOperatorApprovalsGatewayClient mock with a plain mockResolvedValue(...), which drops the start() behavior that triggers onHelloOk. Since ensureApprovalGatewayClient now waits on that hello signal before resolving approvals, callback-approval tests can hang indefinitely on first use instead of reaching assertions.
Useful? React with 👍 / 👎.
|
Closing this as implemented after Codex review. Current What I checked:
So I’m closing this as already implemented rather than keeping a duplicate issue open. Review notes: reviewed against 5dfc1b90e176; fix evidence: release v2026.4.12, commit 5dfc1b90e176. |
Summary
Fixes #57339 — adds full plugin approval support to Telegram, bringing it to parity with Discord. Tested end-to-end: approval prompt appears, button click resolves immediately, tool proceeds.
Commit 1: Handler subscription (
exec-approvals-handler.ts)TelegramExecApprovalHandlernow handlesplugin.approval.requestedandplugin.approval.resolvedgateway events.plugin.approval.requestedandplugin.approval.resolvedinhandleGatewayEventExecApprovalRequest | PluginApprovalRequestinhandleRequested,shouldHandle, and helpersbuildPluginApprovalPendingReplyPayloadfor plugin approval messagesExecApprovalResolved | PluginApprovalResolvedinhandleResolvedCommit 2: Direct callback resolution (
bot-handlers.runtime.ts)Resolve approval callbacks directly via a lazily-initialized gateway client instead of routing through
processMessage(which deadlocks because the session pipeline is blocked waiting for the approval decision).plugin.approval.resolveorexec.approval.resolvevia gateway clientprocessMessageCommit 3: Bypass sequentializer (
sequential-key.ts)The
@grammyjs/runnersequentializer processes all updates per chat sequentially. When the agent turn is waiting for approval, the callback_query is queued behind it — deadlock at the runner level.Approval callback_queries now get a separate sequential key (
telegram:{chatId}:approval), allowing them to be processed in parallel with the blocked agent turn. Same pattern as abort requests (telegram:{chatId}:control).Commit 4: Dedup + param fix (
exec-approvals-handler.ts,bot-handlers.runtime.ts)telegram:prefix normalization.resolvedByproperty fromplugin.approval.resolvecall (gateway only accepts{ id, decision }).Commit 5: Review feedback (
bot-handlers.media.ts,bot-handlers.runtime.ts,exec-approvals-handler.ts)APPROVE_CALLBACK_DATA_REnow matchesalwaysalias (used by "Allow Always" button to fit Telegram's 64-byte callback_data limit)parseApprovalCallbackDecisioncorrectly mapsalways→allow-always(wasallow-once)resolveRequestSessionTargetCommit 6: Tests (
exec-approvals-handler.test.ts)7 new plugin approval tests, all 3 existing exec approval tests remain green (10/10).
Test plan
Unit tests (10/10 passing)
Manual e2e (Telegram DM with ClawClip plugin)
plugin.approval.requestedevents