feat(hooks): add async requireApproval to before_tool_call#55339
feat(hooks): add async requireApproval to before_tool_call#55339joshavant merged 29 commits intoopenclaw:mainfrom
Conversation
🔒 Aisle Security AnalysisWe found 3 potential security issue(s) in this PR:
Vulnerabilities1. 🟠 Plugin approval can be bypassed when no approval route exists and timeoutBehavior is set to "allow"
DescriptionThe This creates an approval-bypass condition:
Vulnerable flow:
This undermines the “native platform approval” guarantee documented for RecommendationTreat missing approval routes / immediate-null decisions as a hard failure (deny), regardless of Suggested fix (agent-side): distinguish between Example patch (conceptual): // If gateway reports no route (decision is null), do not treat as timeout-allow.
if (hasImmediateDecision && decision === null) {
safeOnResolution(PluginApprovalResolutions.CANCELLED);
return { blocked: true, reason: "Plugin approval unavailable (no approval route)" };
}
// Only allow timeoutBehavior handling when a real wait was performed and timed out.Additionally (gateway-side), prefer returning an error (or a distinct status) when no approval clients/routes exist instead of If you intentionally want plugins to be able to fail-open, gate it behind an explicit trusted plugin allowlist or a server config flag, not plugin-controlled input. 2. 🟠 Plugin approvals can be self-resolved due to shared operator.approvals scope for request and resolve
DescriptionThe new plugin approval RPC methods are all grouped under the same If an untrusted plugin (or any non-human automation client) is granted the approvals scope so it can request human approval, it can immediately bypass the approval process by calling Key issues:
Vulnerable code (scope mapping): [APPROVALS_SCOPE]: [
"exec.approval.request",
"exec.approval.waitDecision",
"exec.approval.resolve",
"plugin.approval.request",
"plugin.approval.waitDecision",
"plugin.approval.resolve",
],And the resolve handler performs no approver verification: const ok = manager.resolve(approvalId, decision, resolvedBy ?? null);RecommendationSeparate requesting a plugin approval from resolving it. Minimum change:
Additionally (defense-in-depth):
Example scope split: // approvals scope: only resolution
[APPROVALS_SCOPE]: [
"plugin.approval.resolve",
"plugin.approval.waitDecision", // if needed for approver UIs
],
// write scope (or a new dedicated scope): request
[WRITE_SCOPE]: [
// ...
"plugin.approval.request",
],Example defense-in-depth check: // when creating
record.requestedByConnId = client?.connId ?? null;
// when resolving
if (snapshot?.requestedByConnId && snapshot.requestedByConnId === client?.connId) {
respond(false, undefined, errorShape(ErrorCodes.INVALID_REQUEST, "requester cannot resolve"));
return;
}3. 🟡 Discord mention injection via unescaped plugin approval title/description forwarded to Discord messages/components
DescriptionUntrusted plugin-controlled fields (
Impact:
Vulnerable code (examples): // src/infra/plugin-approvals.ts
lines.push(`Title: ${request.request.title}`);
lines.push(`Description: ${request.request.description}`);// extensions/discord/src/channel.ts
text: `### Description\n${formatDiscordApprovalPreview(request.description, 1000)}`,RecommendationSuppress mentions for plugin-provided text in Discord outbound sends. Preferred fix (Discord API-level): set Example (when posting messages): rest.post(Routes.channelMessages(channelId), {
body: {
content: text,
allowed_mentions: { parse: [] }, // blocks @everyone/@here/user/role mentions
},
});If using a higher-level payload builder, ensure the generated request body includes Defense-in-depth (text-level): additionally neutralize reserved mentions by inserting a zero-width character: function neutralizeDiscordMentions(s: string) {
return s.replace(/@everyone/g, '@\u200beveryone').replace(/@here/g, '@\u200bhere');
}Apply the mention suppression/neutralization to both:
Analyzed PR: #55339 at commit Last updated on: 2026-03-26T23:42:09Z |
openclaw-barnacle
bot
added
docs
![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: 360030a3d3
ℹ️ 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".
src/plugins/hooks.ts
Outdated
| return acc; | ||
| } | ||
| return { | ||
| params: lastDefined(acc?.params, next.params), |
There was a problem hiding this comment.
This merge path still allows lower-priority hooks to overwrite params after an earlier hook has already selected requireApproval. Because the approval prompt comes from the first hook but execution later uses the merged params, a user can approve one plugin-described action while a different parameter set is actually executed. This undermines the approval boundary for multi-plugin setups.
Useful? React with 👍 / 👎.
| args.signal!.addEventListener("abort", () => reject(args.signal!.reason), { | ||
| once: true, | ||
| }); |
There was a problem hiding this comment.
The abort listener added here is never removed when waitPromise wins the race. If a run reuses the same AbortSignal across many approved tool calls, these listeners accumulate until the signal aborts, which can produce MaxListenersExceededWarning and unnecessary retained closures. Add explicit listener cleanup after Promise.race resolves.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR adds async human-in-the-loop approval to the Confidence Score: 4/5Safe to merge — core approval flow is correctly implemented, fail-closed, and well-tested; three P2 polish items remain but none affect correctness or security. The PR introduces a substantial new async approval surface but builds entirely on existing tested infrastructure. Security invariants (server-generated IDs, scope-gated resolve, timeout-defaults-to-deny, abort unblocking) are all correct. The identified issues are code-quality improvements rather than bugs. src/gateway/server-methods/plugin-approval.ts (ID mutation + no-route early-termination), src/agents/pi-tools.before-tool-call.ts (catch-block error message).
|
| Filename | Overview |
|---|---|
| src/plugins/hooks.ts | Extends mergeResults to receive the hook registration for pluginId stamping; block correctly takes precedence over requireApproval. |
| src/agents/pi-tools.before-tool-call.ts | Adds async approval flow via two-phase gateway RPC with abort-signal racing; catch block conflates abort with gateway failure in block reason. |
| src/gateway/server-methods/plugin-approval.ts | New gateway handlers mirroring exec-approval pattern; fragile record.id mutation and unnecessary waitDecision round-trip on no-route path noted. |
| src/gateway/exec-approval-manager.ts | Generalized to ExecApprovalManager; new awaitDecision method correctly returns the existing promise for already-registered IDs. |
| src/auto-reply/reply/commands-approve.ts | Adds plugin: prefix routing and exec→plugin fallback with robust multi-signal error detection for mixed server/client version compatibility. |
| src/infra/exec-approval-forwarder.ts | Adds plugin approval forwarding with independent pluginPending map; stop() correctly clears both maps. |
| src/plugins/types.ts | Defines requireApproval on PluginHookBeforeToolCallResult and PluginApprovalResolutions enum; onResolution clearly documented as informational-only. |
| ui/src/ui/controllers/exec-approval.ts | Adds kind discriminator and plugin-specific fields to ExecApprovalRequest; both approval types feed the same queue. |
Comments Outside Diff (1)
-
src/agents/pi-tools.before-tool-call.ts, line 313-319 (link)Misleading block reason when abort signal fires
The catch block handles two distinct cases — a gateway RPC failure and an
AbortSignalcancellation — with the same error message. When the agent run is cancelled mid-approval the reason shown to the model will say "Plugin approval required (gateway unavailable)" (orapproval.description), even though the gateway was fine and the run was intentionally aborted. SincesafeOnResolutionalready correctly maps this toCANCELLED, the block reason should distinguish the two:Prompt To Fix With AI
This is a comment left during a code review. Path: src/agents/pi-tools.before-tool-call.ts Line: 313-319 Comment: **Misleading block reason when abort signal fires** The catch block handles two distinct cases — a gateway RPC failure and an `AbortSignal` cancellation — with the same error message. When the agent run is cancelled mid-approval the reason shown to the model will say "Plugin approval required (gateway unavailable)" (or `approval.description`), even though the gateway was fine and the run was intentionally aborted. Since `safeOnResolution` already correctly maps this to `CANCELLED`, the block reason should distinguish the two: How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/plugin-approval.ts
Line: 83-84
Comment:
**Fragile post-creation ID mutation**
`record.id` is mutated after `create()` returns but before `register()` stores it. This works today because `create()` only allocates the object without storing it, but it relies on that internal invariant staying true. A more robust approach would generate the prefixed ID up front and pass it directly to `create()`, making the intent clearer and eliminating the mutation:
```ts
import { randomUUID } from "node:crypto";
// ...
const record = manager.create(request, timeoutMs, `plugin:${randomUUID()}`);
```
This removes the mutation and makes the prefixing strategy explicit at the call site.
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/agents/pi-tools.before-tool-call.ts
Line: 313-319
Comment:
**Misleading block reason when abort signal fires**
The catch block handles two distinct cases — a gateway RPC failure and an `AbortSignal` cancellation — with the same error message. When the agent run is cancelled mid-approval the reason shown to the model will say "Plugin approval required (gateway unavailable)" (or `approval.description`), even though the gateway was fine and the run was intentionally aborted. Since `safeOnResolution` already correctly maps this to `CANCELLED`, the block reason should distinguish the two:
```suggestion
} catch (err) {
// Gateway error or abort signal — fall back to soft block
const isAbort = args.signal?.aborted;
await safeOnResolution(PluginApprovalResolutions.CANCELLED);
log.warn(`plugin approval gateway request failed, falling back to block: ${String(err)}`);
return {
blocked: true,
reason: isAbort
? "Approval cancelled (agent run aborted)"
: approval.description || "Plugin approval required (gateway unavailable)",
};
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: src/gateway/server-methods/plugin-approval.ts
Line: 148-161
Comment:
**Unnecessary `waitDecision` round-trip in the no-route early-termination path**
When `!hasApprovalClients && !forwarded`, the gateway expires the approval immediately and returns `{ id, decision: null }`. Because the client only checks for a non-empty `id`, it proceeds to fire a `plugin.approval.waitDecision` RPC — which correctly gets `null` back from the grace-window entry but adds an extra network round-trip for every no-route call.
The client could short-circuit by also checking whether `requestResult.decision` is already present (non-undefined), or the early-termination response could use a distinct marker (e.g. `status: "no-route"`) so the client can detect it without a second RPC. Not blocking, but worth noting for high-frequency tool calls on setups without an approval UI.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Approvals: complete Discord parity and c..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const record = manager.create(request, timeoutMs, null); | ||
| record.id = `plugin:${record.id}`; |
There was a problem hiding this comment.
record.id is mutated after create() returns but before register() stores it. This works today because create() only allocates the object without storing it, but it relies on that internal invariant staying true. A more robust approach would generate the prefixed ID up front and pass it directly to create(), making the intent clearer and eliminating the mutation:
import { randomUUID } from "node:crypto";
// ...
const record = manager.create(request, timeoutMs, `plugin:${randomUUID()}`);This removes the mutation and makes the prefixing strategy explicit at the call site.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/plugin-approval.ts
Line: 83-84
Comment:
**Fragile post-creation ID mutation**
`record.id` is mutated after `create()` returns but before `register()` stores it. This works today because `create()` only allocates the object without storing it, but it relies on that internal invariant staying true. A more robust approach would generate the prefixed ID up front and pass it directly to `create()`, making the intent clearer and eliminating the mutation:
```ts
import { randomUUID } from "node:crypto";
// ...
const record = manager.create(request, timeoutMs, `plugin:${randomUUID()}`);
```
This removes the mutation and makes the prefixing strategy explicit at the call site.
How can I resolve this? If you propose a fix, please make it concise.| undefined, | ||
| ); | ||
| } | ||
|
|
||
| const decision = await decisionPromise; | ||
| respond( | ||
| true, | ||
| { | ||
| id: record.id, | ||
| decision, | ||
| createdAtMs: record.createdAtMs, | ||
| expiresAtMs: record.expiresAtMs, | ||
| }, | ||
| undefined, |
There was a problem hiding this comment.
waitDecision round-trip in the no-route early-termination path
When !hasApprovalClients && !forwarded, the gateway expires the approval immediately and returns { id, decision: null }. Because the client only checks for a non-empty id, it proceeds to fire a plugin.approval.waitDecision RPC — which correctly gets null back from the grace-window entry but adds an extra network round-trip for every no-route call.
The client could short-circuit by also checking whether requestResult.decision is already present (non-undefined), or the early-termination response could use a distinct marker (e.g. status: "no-route") so the client can detect it without a second RPC. Not blocking, but worth noting for high-frequency tool calls on setups without an approval UI.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/gateway/server-methods/plugin-approval.ts
Line: 148-161
Comment:
**Unnecessary `waitDecision` round-trip in the no-route early-termination path**
When `!hasApprovalClients && !forwarded`, the gateway expires the approval immediately and returns `{ id, decision: null }`. Because the client only checks for a non-empty `id`, it proceeds to fire a `plugin.approval.waitDecision` RPC — which correctly gets `null` back from the grace-window entry but adds an extra network round-trip for every no-route call.
The client could short-circuit by also checking whether `requestResult.decision` is already present (non-undefined), or the early-termination response could use a distinct marker (e.g. `status: "no-route"`) so the client can detect it without a second RPC. Not blocking, but worth noting for high-frequency tool calls on setups without an approval UI.
How can I resolve this? If you propose a fix, please make it concise.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
360030a to
4144d0b
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aba858d6c1
ℹ️ 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".
| const waitPromise = callGatewayTool<{ | ||
| id?: string; | ||
| decision?: string | null; | ||
| }>( | ||
| "plugin.approval.waitDecision", |
There was a problem hiding this comment.
runBeforeToolCallHook unconditionally calls plugin.approval.waitDecision as soon as plugin.approval.request returns an id, but plugin.approval.request can legitimately return a final { decision: null } immediately when there is no approval route (src/gateway/server-methods/plugin-approval.ts, no-route branch). In that case this extra wait call returns “expired or not found”, which is caught as a gateway failure, so the run is treated as cancelled/gateway-unavailable instead of a timeout. That bypasses timeoutBehavior: "allow" and reports the wrong resolution for no-route environments.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
528d4c4 to
31fcae9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 31fcae984f
ℹ️ 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".
| }, | ||
| buildPluginPendingPayload: ({ request, nowMs }) => { | ||
| const text = buildPluginApprovalRequestMessage(request, nowMs); | ||
| const buttons = buildTelegramExecApprovalButtons(request.id); |
There was a problem hiding this comment.
This path passes the full plugin approval id into buildTelegramExecApprovalButtons, but plugin ids are now plugin:<uuid>, which pushes the callback payload /approve <id> allow-always to 65 bytes. buildTelegramExecApprovalButtons enforces Telegram’s 64-byte callback limit, so the Allow Always button is silently dropped for plugin approvals, leaving Telegram users unable to choose the persistent allow decision from buttons even though the backend supports it.
Useful? React with 👍 / 👎.
extensions/discord/src/channel.ts
Outdated
| allowedDecisions: ["allow-once", "allow-always", "deny"], | ||
| }, | ||
| discord: { | ||
| components: buildDiscordPluginPendingComponentSpec({ request }), | ||
| }, |
There was a problem hiding this comment.
Plugin pending payloads always include execapproval Discord components here, but the corresponding interaction handler is only registered when discord.execApprovals.enabled in extensions/discord/src/monitor/provider.ts. In configurations where approvals.plugin forwarding is enabled but Discord exec approvals are disabled, messages render approval buttons that have no handler, so users see interactive controls that cannot resolve the approval.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
8719312 to
d626955
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6269552b3
ℹ️ 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".
extensions/telegram/src/channel.ts
Outdated
| }, | ||
| buildPluginPendingPayload: ({ request, nowMs }) => { | ||
| const text = buildPluginApprovalRequestMessage(request, nowMs); | ||
| const approvalSlug = request.id.slice(0, 8); |
There was a problem hiding this comment.
Building Telegram plugin-approval buttons from request.id.slice(0, 8) reduces the actionable ID to values like plugin:a, leaving only one hex character of entropy after the fixed prefix. If multiple pending plugin approvals share that first UUID nibble, /approve plugin:a ... becomes an ambiguous prefix in ExecApprovalManager.lookupPendingId, so the button press fails even though the approval is still pending. This truncation is unnecessarily aggressive for the 64-byte callback cap (a 42-character prefix still fits allow-always).
Useful? React with 👍 / 👎.
src/infra/exec-approval-forwarder.ts
Outdated
| const targets = entry?.targets; | ||
| if (!targets || targets.length === 0) { | ||
| return; |
There was a problem hiding this comment.
handlePluginApprovalResolved bails out when pluginPending has no entry, so resolved notifications are lost whenever this process did not observe (or no longer remembers) the corresponding requested event. Unlike the exec path, this branch never recomputes targets from resolved.request, even though that payload is provided on resolve broadcasts, which can leave forwarded chats showing a stale pending approval with no final state update after restart/reconnect races.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
d626955 to
dc8fa94
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dc8fa94c6b
ℹ️ 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".
| // Plugin approval IDs are kind-prefixed (`plugin:<uuid>`); route directly when detected. | ||
| // Unprefixed IDs try exec first, then fall back to plugin for backward compat. | ||
| const isPluginId = parsed.id.startsWith("plugin:"); | ||
| if (isPluginId) { |
There was a problem hiding this comment.
This new plugin-ID routing path is unreachable for Telegram accounts that have approvals.plugin forwarding enabled but channels.telegram.execApprovals disabled, because handleApproveCommand still exits earlier with "Telegram exec approvals are not enabled" before it reaches the plugin.approval.resolve branch. In that configuration, forwarded plugin approvals in Telegram cannot be resolved from chat and will always time out/deny, which breaks the advertised independent plugin-approval flow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7006197e9c
ℹ️ 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".
| const isPluginId = parsed.id.startsWith("plugin:"); | ||
|
|
||
| if (params.command.channel === "telegram") { | ||
| if (params.command.channel === "telegram" && !isPluginId) { |
There was a problem hiding this comment.
The Telegram guard now skips approver validation for plugin: IDs, so plugin approvals can be resolved by any sender who is merely command-authorized, not necessarily in channels.telegram.*.execApprovals.approvers. In group/chat setups where isAuthorizedSender is broader than the approver list, this lets non-approvers approve or deny plugin-gated tool calls via /approve plugin:<id> ..., which weakens the approval boundary for Telegram-routed approvals.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
bbba561 to
1afec53
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1afec53d28
ℹ️ 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".
| // Plugin approval IDs are kind-prefixed (`plugin:<uuid>`); route directly when detected. | ||
| // Unprefixed IDs try exec first, then fall back to plugin for backward compat. | ||
| if (isPluginId) { | ||
| try { | ||
| await callApprovalMethod("plugin.approval.resolve"); |
There was a problem hiding this comment.
This branch treats any ID that starts with plugin: as a plugin approval and skips exec.approval.resolve, but exec.approval.request still accepts caller-provided IDs without reserving that prefix. If an integration creates an exec approval with an ID like plugin:job-42, /approve will be sent to plugin.approval.resolve and fail as “unknown or expired,” leaving a valid exec approval unresolved. Please keep the exec-first fallback path for prefixed IDs (or otherwise disambiguate by source metadata instead of prefix alone).
Useful? React with 👍 / 👎.
| const method = | ||
| resolveApprovalKindFromId(approvalId) === "plugin" | ||
| ? "plugin.approval.resolve" | ||
| : "exec.approval.resolve"; |
There was a problem hiding this comment.
The Discord button handler chooses plugin.approval.resolve solely from the approval ID prefix. Because exec approvals can be created with explicit IDs, any exec ID that starts with plugin: will be misrouted and fail to resolve when a user clicks the button. This creates stuck approvals for valid exec requests; the handler should use cached request kind (from the original requested event) rather than inferring kind from the ID string.
Useful? React with 👍 / 👎.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
aac49f8 to
268da43
Compare
Extend the before_tool_call plugin hook with a requireApproval return field that pauses agent execution and waits for real user approval via channels (Telegram, Discord, /approve command) instead of relying on the agent to cooperate with a soft block. - Add requireApproval field to PluginHookBeforeToolCallResult with id, title, description, severity, timeout, and timeoutBehavior options - Extend runModifyingHook merge callback to receive hook registration so mergers can stamp pluginId; always invoke merger even for the first result - Make ExecApprovalManager generic so it can be reused for plugin approvals - Add plugin.approval.request/waitDecision/resolve gateway methods with schemas, scope guards, and broadcast events - Handle requireApproval in pi-tools via two-phase gateway RPC with fallback to soft block when the gateway is unavailable - Extend the exec approval forwarder with plugin approval message builders and forwarding methods - Update /approve command to fall back to plugin.approval.resolve when exec approval lookup fails - Document before_tool_call requireApproval in hooks docs and unified /approve behavior in exec-approvals docs
- Extract mergeParamsWithApprovalOverrides helper to deduplicate param merge logic in before_tool_call hook handling - Use idiomatic conditional spread syntax in toolContext construction - Extract callApprovalMethod helper in /approve command to eliminate duplicated callGateway calls - Simplify plugin approval schema by removing unnecessary Type.Union with Type.Null on optional fields - Extract normalizeTrimmedString helper for turn source field trimming
Fix 3 broken assertions expecting old "Exec approval" message text. Add tests for the /approve command's exec→plugin fallback path, plugin approval method registration and scope authorization, and handler factory key verification.
Handle plugin.approval.requested and plugin.approval.resolved gateway events by extending the existing exec approval queue with a kind discriminator. Plugin approvals reuse the same overlay, queue management, and expiry timer, with branched rendering for plugin-specific content (title, description, severity). The decision handler routes resolve calls to the correct gateway method based on kind.
The gateway broadcasts plugin approval payloads with title, description, severity, pluginId, agentId, and sessionKey nested inside the request object (PluginApprovalRequestPayload), not at the top level. Fix the parser to read from the correct location so the overlay actually appears.
- Add APPROVAL_NOT_FOUND error code so /approve fallback uses structured matching instead of fragile string comparison - Check block before requireApproval so higher-priority plugin blocks cannot be overridden by a lower-priority approval - Race waitDecision against abort signal so users are not stuck waiting for the full approval timeout after cancelling a run - Use null consistently for missing pluginDescription instead of converting to undefined - Add comments explaining the +10s timeout buffer on gateway RPCs
…hooks - Fix timeout-allow param bug: return merged hook params instead of original params when timeoutBehavior is "allow", preventing security plugins from having their parameter rewrites silently discarded. - Host-generate approval IDs: remove plugin-provided id field from the requireApproval type, gateway request, and protocol schema. Server always generates IDs via randomUUID() to prevent forged/predictable ID attacks. - Define onResolution semantics: add PluginApprovalResolutions constants and PluginApprovalResolution type. onResolution callback now fires on every exit path (allow, deny, timeout, abort, gateway error, no-ID). Decision branching uses constants instead of hard-coded strings. - Fix pre-existing test infrastructure issues: bypass CJS mock cache for getGlobalHookRunner global singleton, reset gateway mock between tests, fix hook merger priority ordering in block+requireApproval test.
Harden the plugin approval request schema: restrict severity to enum (info|warning|critical), cap timeoutMs at 600s, limit title to 80 chars and description to 256 chars. Prefix plugin approval IDs with `plugin:` so /approve routing can distinguish them from exec approvals deterministically instead of relying on fallback.
…check hasExecApprovalClients() counted the backend connection that issued the plugin.approval.request RPC as an approval client, preventing the no-approval-route fast path from firing in headless setups and causing 120s stalls. Pass the caller's connId so it is skipped. Applied to both plugin and exec approval handlers.
joshavant
force-pushed
the
feat/ask-before-tool-copy-53768
branch
from
795663f to
2c78bae
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c78bae827
ℹ️ 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".
| const requestResult = await callGatewayTool<{ | ||
| id?: string; | ||
| status?: string; | ||
| decision?: string | null; | ||
| }>( | ||
| "plugin.approval.request", |
There was a problem hiding this comment.
This path sends plugin.approval.request before checking args.signal.aborted, so a tool call that has already been canceled can still create and forward a live approval request. In that scenario the subsequent wait path exits as "run aborted," but the approval prompt remains visible until timeout (or can even be resolved by a user despite the run being dead), which creates confusing stale approvals. Guarding before the request (or racing the request itself with abort) avoids creating orphaned plugin approvals.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 16198e2849
ℹ️ 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".
| }, | ||
| buildPluginPendingPayload: ({ request, nowMs }) => { | ||
| const text = buildPluginApprovalRequestMessage(request, nowMs); | ||
| const buttons = buildTelegramExecApprovalButtons(request.id); |
There was a problem hiding this comment.
buildPluginPendingPayload always adds /approve inline buttons for plugin approvals, but Telegram callback handling still rejects approval callbacks unless isTelegramExecApprovalClientEnabled is true (extensions/telegram/src/bot-handlers.runtime.ts, isApprovalCallback branch). In configs that enable approvals.plugin forwarding while keeping channels.telegram.execApprovals.enabled off, users are shown actionable plugin-approval buttons that can never submit a decision, so requests silently age out unless resolved elsewhere; this payload should only include buttons when the callback path is actually enabled for the target account.
Useful? React with 👍 / 👎.
2 participants
Summary
Add async human-in-the-loop approval for plugin-controlled tool calls via the
before_tool_callhook.Note: This PR was copied over from feat(hooks): add async requireApproval to before_tool_call #53768.
Plugins return
requireApprovalfrom theirbefore_tool_callhandler to pause execution and prompt the user for an explicit allow/deny decision.Approval surfaces: exec approval overlay (UI), Telegram buttons, Discord interactions,
/approveCLI command.The
/approvecommand now handles both exec and plugin approvals with automatic fallback.blocktakes precedence overrequireApprovalso a higher-priority plugin's block cannot be overridden by a lower-priority approval request.Approval wait races the abort signal so cancelling a run unblocks immediately instead of waiting for the full timeout.
Screen.Recording.2026-03-24.at.15.53.34.mov
Motivation
Security plugins like Sage (part of Agent Trust Hub) need a way to intercept and gate dangerous or suspicious tool calls before they execute. The existing synchronous
block: trueveto is binary — it can only allow or deny with no user interaction. Real-world security policies require presenting context to the user (what the tool will do, why it was flagged, severity) and letting them make an informed decision.This also aligns OpenClaw with AARTS (An Open Standard for AI Agent Runtime Safety), which defines an
askverdict for tool hooks — requiring the host to prompt the user interactively before proceeding. TherequireApprovalmechanism is the host-side implementation of that pattern.Sage already has OpenClaw users who currently lack the interactive approval-based protection available to Sage users on other supported agents (Cursor, Claude Code). This PR closes that gap.
While the immediate driver is security plugins, the
requireApprovalmechanism is generic — any plugin can use it to gate tool calls that warrant human oversight (cost controls, external API calls, destructive operations, etc.).Security Considerations
The approval flow gates tool execution on human consent. Key properties:
operator.approvalsoroperator.admin) can submit decisions.timeoutMs(default 120s), the tool call is blocked.Related
Competing / prior art
block: trueveto is already onmain; this branch adds the only net-new feature (async approval). Theafter_tool_callobservability from feat(hooks): wire before_tool_call/after_tool_call with veto support #18810 can land separately.needsApprovaltobefore_tool_callbut bundled vendor-specific gateway methods, retry store, and CLI commands. This PR takes the generic approach instead:requireApprovalis vendor-neutral, reuses the existingExecApprovalManager<T>, and exposesplugin.approval.*methods usable by any plugin.Related open issues
Test Plan
src/gateway/server-methods/plugin-approval.test.ts— gateway validation, state managementsrc/infra/plugin-approval-forwarder.test.ts— channel forwardingsrc/plugins/hooks.before-tool-call.test.ts— hook result merging with pluginId stampingsrc/auto-reply/reply/commands.test.ts—/approvefallback, scope enforcementui/src/ui/controllers/exec-approval.test.ts— UI controller wiringui/src/ui/app-gateway.node.test.ts— gateway event subscriptionsrc/gateway/method-scopes.test.ts— method scope registrationsrc/agents/pi-tools.before-tool-call.e2e.test.ts— approval RPC flow, timeouts, fallbacks