feat(feishu): add exec approval interactive card with in-place updates#58682
feat(feishu): add exec approval interactive card with in-place updates#58682cowboy129 wants to merge 7 commits into
Conversation
Greptile SummaryThis PR adds a Feishu Interactive Card (V2) approval flow for exec commands, with three action buttons (Allow Once / Allow Always / Deny), in-place card updates on resolution via direct Key concerns:
Confidence Score: 4/5The Feishu interactive card flow is correct and well-tested, but the refactoring in commands-approve.ts introduces two behavioral regressions that should be verified or intentionally acknowledged before merging. Two P1 findings in commands-approve.ts: (1) plugin:xxx IDs are now silently misrouted to exec.approval.resolve, and (2) the isAuthorizedSender gate is now a hard requirement where it previously was not. These are present-defect regressions in existing functionality, even though they fall outside the primary Feishu card scope of the PR. src/auto-reply/reply/commands-approve.ts — the plugin approval routing removal and the authorization gate simplification need verification
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| await callGateway({ | ||
| method, | ||
| method: "exec.approval.resolve", | ||
| params: { id: parsed.id, decision: parsed.decision }, | ||
| clientName: GATEWAY_CLIENT_NAMES.GATEWAY_CLIENT, | ||
| clientDisplayName: `Chat approval (${resolvedBy})`, | ||
| mode: GATEWAY_CLIENT_MODES.BACKEND, | ||
| }); |
There was a problem hiding this comment.
The refactoring removes the isPluginId branch that previously routed plugin:xxx IDs to plugin.approval.resolve. The new code unconditionally calls exec.approval.resolve for every ID, which means any /approve plugin:abc allow-once command will fail with a not-found error from the server instead of being handled correctly.
Previously the routing was:
parsed.id.startsWith("plugin:")→plugin.approval.resolve- exec IDs →
exec.approval.resolve(with fallback toplugin.approval.resolveonAPPROVAL_NOT_FOUND)
If plugin approvals are fully deprecated and removed from the server, this is fine — but if plugin.approval.resolve still exists on the gateway, existing workflows using /approve plugin:xxx are now broken.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-approve.ts
Line: 154-160
Comment:
**Plugin approval IDs now silently misrouted**
The refactoring removes the `isPluginId` branch that previously routed `plugin:xxx` IDs to `plugin.approval.resolve`. The new code unconditionally calls `exec.approval.resolve` for every ID, which means any `/approve plugin:abc allow-once` command will fail with a not-found error from the server instead of being handled correctly.
Previously the routing was:
- `parsed.id.startsWith("plugin:")` → `plugin.approval.resolve`
- exec IDs → `exec.approval.resolve` (with fallback to `plugin.approval.resolve` on `APPROVAL_NOT_FOUND`)
If plugin approvals are fully deprecated and removed from the server, this is fine — but if `plugin.approval.resolve` still exists on the gateway, existing workflows using `/approve plugin:xxx` are now broken.
How can I resolve this? If you propose a fix, please make it concise.| if (!params.command.isAuthorizedSender) { | ||
| logVerbose( | ||
| `Ignoring /approve from unauthorized sender: ${params.command.senderId || "<unknown>"}`, | ||
| ); | ||
| return { shouldContinue: false }; | ||
| } |
There was a problem hiding this comment.
The old code allowed approval if isAuthorizedSender || hasExplicitApprovalAuthorization. hasExplicitApprovalAuthorization was satisfied by the channel plugin's auth.authorizeActorAction returning authorized: true (e.g., a Telegram exec-approval approver configured in the channel config). The new code requires isAuthorizedSender to be true first, making the channel-specific checks (lines 99–141) unreachable for anyone who isn't already a general authorized sender.
In practice this means: if a deployment has Telegram exec-approval approvers who are NOT in the general authorized-sender list, their /approve commands now silently fail (the handler returns { shouldContinue: false } with no reply). The old code would have let them through via hasExplicitApprovalAuthorization.
If the intent is to require isAuthorizedSender for all approval commands going forward, please add a comment noting the deliberate tightening, and verify no existing deployments relied on the separate execApprovalAuthorization path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/auto-reply/reply/commands-approve.ts
Line: 88-93
Comment:
**Narrower authorization gate may break existing Telegram deployments**
The old code allowed approval if `isAuthorizedSender || hasExplicitApprovalAuthorization`. `hasExplicitApprovalAuthorization` was satisfied by the channel plugin's `auth.authorizeActorAction` returning `authorized: true` (e.g., a Telegram exec-approval approver configured in the channel config). The new code requires `isAuthorizedSender` to be true first, making the channel-specific checks (lines 99–141) unreachable for anyone who isn't already a general authorized sender.
In practice this means: if a deployment has Telegram exec-approval approvers who are NOT in the general authorized-sender list, their `/approve` commands now silently fail (the handler returns `{ shouldContinue: false }` with no reply). The old code would have let them through via `hasExplicitApprovalAuthorization`.
If the intent is to require `isAuthorizedSender` for all approval commands going forward, please add a comment noting the deliberate tightening, and verify no existing deployments relied on the separate `execApprovalAuthorization` path.
How can I resolve this? If you propose a fix, please make it concise.![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d0401eaca2
ℹ️ 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".
| await callGateway({ | ||
| method, | ||
| method: "exec.approval.resolve", | ||
| params: { id: parsed.id, decision: parsed.decision }, |
There was a problem hiding this comment.
/approve now always calls exec.approval.resolve, so plugin approvals (plugin:<id> and legacy plugin IDs that require fallback) can no longer be resolved through chat and will return unknown/expired errors. This regresses existing plugin-approval flows that still emit /approve commands for plugin requests.
Useful? React with 👍 / 👎.
| if (!params.command.isAuthorizedSender) { | ||
| logVerbose( | ||
| `Ignoring /approve from unauthorized sender: ${params.command.senderId || "<unknown>"}`, | ||
| ); | ||
| return { shouldContinue: false }; |
There was a problem hiding this comment.
The new early return on !isAuthorizedSender bypasses channel/plugin approval authorization logic, so configured approvers in gated chats can no longer run /approve unless they also pass the general chat authorization gate. This breaks approval-only access patterns where approvers are intentionally allowed to resolve requests without full command access.
Useful? React with 👍 / 👎.
| await callGateway({ | ||
| method: "exec.approval.resolve", | ||
| params: { id: approvalId, decision: execApprovalDecision }, | ||
| clientName: GATEWAY_CLIENT_NAMES.GATEWAY_CLIENT, |
There was a problem hiding this comment.
In the Feishu card-action exec path, the handler resolves approvals via callGateway without checking whether event.operator.open_id is a configured approver. Because these cards can be shown in shared conversations, any member who can click the button can approve or deny exec requests, bypassing intended approver restrictions.
Useful? React with 👍 / 👎.
| approvals: { | ||
| render: { | ||
| exec: { | ||
| buildPendingPayload: (params) => buildFeishuExecApprovalPendingPayload(params), | ||
| // Resolved notifications are handled by in-place card updates |
There was a problem hiding this comment.
buildPendingPayload can return null for excluded Feishu targets (dm/channel filtering), but this adapter block does not register delivery.shouldSuppressForwardingFallback. In src/infra/exec-approval-forwarder.ts, a null payload falls back to generic text, so approvals still leak to targets that were meant to be excluded by Feishu execApprovals.target.
Useful? React with 👍 / 👎.
3e378db to
613b6fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 613b6fdae9
ℹ️ 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".
| if ( | ||
| params.command.channel === "feishu" && | ||
| !isPluginId && | ||
| !isFeishuExecApprovalClientEnabled({ cfg: params.cfg, accountId: params.ctx.AccountId }) | ||
| ) { |
There was a problem hiding this comment.
This gate rejects every Feishu exec /approve when channels.feishu.execApprovals is not enabled, before execApprovalAuthorization.authorized is evaluated. That regresses valid text-based approval flows (for example users authorized via global approvals.exec.approvers, or fallback text prompts routed to Feishu) because they now always get "Feishu exec approvals are not enabled" even though they are legitimate approvers.
Useful? React with 👍 / 👎.
| // Resolved notifications are handled by in-place card updates | ||
| // in card-action.ts — suppress the default text message. | ||
| buildResolvedPayload: () => null, |
There was a problem hiding this comment.
Returning null unconditionally here suppresses all resolved notifications for Feishu, but in-place card updates only happen in handleFeishuCardAction when a card button is clicked and a message id is available. If an approval is resolved from another surface (terminal/Web/another channel) or the pending message was delivered as plain text fallback, Feishu recipients never receive a resolution update and are left with stale pending prompts.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10869770ea
ℹ️ 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".
| buildResolvedPayload: ({ resolved }) => | ||
| resolved.resolvedBy?.startsWith("feishu:") ? null : undefined, |
There was a problem hiding this comment.
buildResolvedPayload only suppresses the follow-up message when resolved.resolvedBy starts with "feishu:", but the card-action path resolves approvals with clientDisplayName set to "Feishu card approval (feishu:...)". Because gateway resolved events use that display name as resolvedBy, this predicate never matches, so Feishu still sends the default resolved text even after the card is updated in place, reintroducing duplicate resolution notifications for card-button approvals.
Useful? React with 👍 / 👎.
| approvalCommandId: params.request.id, | ||
| command: commandDisplay.commandText, | ||
| cwd: params.request.request.cwd ?? undefined, | ||
| host: params.request.request.host === "node" ? "node" : "gateway", |
There was a problem hiding this comment.
The Feishu pending renderer rewrites every non-"node" host value to "gateway", which drops valid host context from the approval request (host is an open string field and may be unset or values like sandbox). As a result, approvers can be shown the wrong execution host in Feishu approval prompts/cards, which can mislead approval decisions; this path should carry through the original host and only apply a fallback when host is truly missing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0054f2b535
ℹ️ 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".
| !isFeishuExecApprovalApprover({ | ||
| cfg, | ||
| accountId: account.accountId, | ||
| senderId: event.operator.open_id, | ||
| }) |
There was a problem hiding this comment.
The exec-button path authorizes only via isFeishuExecApprovalApprover and then immediately calls exec.approval.resolve, but it never checks isFeishuExecApprovalClientEnabled. If channels.feishu.execApprovals.enabled is turned off while approver IDs remain configured, previously delivered cards can still approve/deny requests, which bypasses the expected feature-disable behavior.
Useful? React with 👍 / 👎.
| cfg, | ||
| messageId: originalMessageId, | ||
| card: resolvedCard, | ||
| accountId, | ||
| }).catch(() => {}); |
There was a problem hiding this comment.
The in-place card update error is swallowed with .catch(() => {}), so this handler reports success even when the card was not updated. Fresh evidence in this commit: resolved fallback text is suppressed for resolvedBy containing Feishu card approval (extensions/feishu/src/channel.ts), so when patching fails (for example, message no longer editable), users can receive no visible resolution confirmation at all.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dbbc85f69
ℹ️ 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".
| buildResolvedPayload: ({ resolved }) => | ||
| resolved.resolvedBy?.includes("Feishu card approval") ? null : undefined, |
There was a problem hiding this comment.
This suppression predicate is global for the channel, so any resolution with resolvedBy containing "Feishu card approval" skips resolved notifications for all Feishu forward targets. In multi-target setups (for example, session + explicit targets), handleFeishuCardAction only patches the clicked message (event.context.open_message_id in extensions/feishu/src/card-action.ts), so other Feishu targets are left with stale pending prompts and no resolution update.
Useful? React with 👍 / 👎.
| const result = await sendCardFeishu({ | ||
| cfg, | ||
| to, | ||
| card: feishuData.card, | ||
| accountId: accountId ?? undefined, |
There was a problem hiding this comment.
sendPayload resolves threadId into replyToMessageId but does not pass replyInThread, unlike the existing sendText card path in the same file. When approvals are delivered through sendPayload in threaded contexts, cards will be posted as inline replies instead of thread replies, which breaks expected routing for thread-based conversations.
Useful? React with 👍 / 👎.
Add Feishu Interactive Card (V2) support for exec approval workflow: - Send pending approval cards with Allow Once / Allow Always / Deny buttons - Resolve approvals via direct gateway RPC (callGateway) instead of synthetic /approve commands, eliminating duplicate messages - Update cards in-place on resolution showing command, result, and operator - Display operator as Feishu user capsule (<at> mention) - Suppress default resolved text notification for Feishu channel - Support both DM and group chat contexts - Chinese localization for all card labels Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Revert commands-approve.ts to upstream, add only Feishu channel checks (preserves plugin routing, explicit authorization, and fallback logic) - Use static chunkTextForOutbound in feishu outbound (fixes chunker test) - Quote <at> tag id attribute in resolved card - Regenerate schema.base.generated.ts Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ssion - Verify event.operator.open_id is a configured approver before resolving exec approval via card button RPC (prevents unauthorized button clicks) - Only suppress resolved text notification when resolved via Feishu card action (resolvedBy starts with "feishu:"); fall through to default text notification when resolved from other surfaces (terminal, web, etc.) - Support undefined return from buildResolvedPayload to signal "use default" Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Fix resolvedBy check to match "Feishu card approval" (the gateway clientDisplayName) instead of "feishu:" prefix - Preserve "sandbox" host value in Feishu approval payloads instead of collapsing non-"node" hosts to "gateway" Co-Authored-By: Claude Opus 4.6 <[email protected]>
- Check isFeishuExecApprovalClientEnabled before processing card button clicks, so disabling the feature prevents stale cards from resolving - Log card patch failures and fall back to sending a new resolved card instead of silently swallowing the error Co-Authored-By: Claude Opus 4.6 <[email protected]>
…ayload Pass replyInThread to sendCardFeishu in the sendPayload path so card replies in threaded contexts are posted as thread replies instead of inline replies, matching the existing sendText behavior. Co-Authored-By: Claude Opus 4.6 <[email protected]>
Refactor Feishu exec approval to use the same ExecApprovalChannelRuntime + createApproverRestrictedNativeApprovalCapability architecture as Discord and Telegram. - Add FeishuExecApprovalHandler with gateway WebSocket subscription, pending message tracking, multi-target card updates on resolve, and timeout/expiry management - Add approval-native.ts adapter for proper authorization, target routing, and forwarding suppression - Rewrite exec-approvals.ts to use createChannelExecApprovalProfile - Simplify card-action.ts button handler to RPC-only (card updates delegated to handler) - Support expired card state and smart resolvedBy display - Fix sendOutboundText replyInThread passthrough Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
c840747 to
73539d1
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 73539d12b1
ℹ️ 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".
| dedupeKey: plannedTarget.target.to, | ||
| target: { to: plannedTarget.target.to }, | ||
| }), |
There was a problem hiding this comment.
prepareTarget collapses each planned destination down to { to }, so any plannedTarget.target.threadId is discarded before delivery. In thread/topic conversations this posts approval cards to the top-level chat instead of the originating thread, and distinct thread targets in the same chat can be deduped together by dedupeKey, so some intended recipients never get an in-thread approval prompt.
Useful? React with 👍 / 👎.
| const expiredCard = createExecApprovalResolvedCard({ | ||
| approvalId: "", | ||
| decision: "expired", |
There was a problem hiding this comment.
The expired-card path hardcodes approvalId: "", so every expired update shows an empty approval slug. When multiple approvals are pending in the same chat, operators cannot tell which request expired from the card they see, which makes reconciliation and follow-up actions error-prone.
Useful? React with 👍 / 👎.
|
Closing this as duplicate or superseded after Codex automated review. Close #58682 as superseded by open PR #60328. The Feishu interactive exec approval work is still not implemented on current main, but #60328 explicitly takes over the same feature with the Handler/capability pattern and incorporates the useful review feedback from #58682. Best possible solution: Close #58682 and continue the Feishu native exec approval implementation and review on #60328, which is the explicit successor PR using the shared Handler/capability architecture. Do not mark the feature implemented until the canonical PR or equivalent code lands on main with config, runtime, docs, and tests. What I checked:
So I’m closing this here and keeping the remaining discussion on the canonical linked item. Codex Review notes: model gpt-5.5, reasoning high; reviewed against 4ad8b613c9b6. |
2 participants
Summary
callGateway) instead of synthetic/approvecommands, eliminating duplicate messagesChanged files
extensions/feishu/src/card-ux-exec-approval.ts— New card templates for pending and resolved exec approval cardsextensions/feishu/src/card-action.ts— Handle exec approval button clicks viacallGatewayRPC + in-place card updateextensions/feishu/src/exec-approval-forwarding.ts— Feishu-specific exec approval forwarding payload builderextensions/feishu/src/exec-approvals.ts— Feishu exec approval helpersextensions/feishu/src/channel.ts— RegisterbuildPendingPayloadand suppressbuildResolvedPayloadextensions/feishu/src/monitor.account.ts— Parsecard.action.triggerevent with WebSocket SDK field fallbackssrc/infra/exec-approval-forwarder.ts— AllowbuildResolvedPayloadto returnnullto suppress notificationssrc/plugin-sdk/infra-runtime.ts— ExportcallGatewayand gateway constantsTest plan
card-ux-exec-approval.test.ts)exec-approval-forwarding.test.ts)exec-approvals.test.ts)🤖 Generated with Claude Code