fix: ensure exec approval is registered before returning (#2402) by ramin-shirali · Pull Request #3357 · openclaw/openclaw

ramin-shirali · 2026-01-28T13:59:07Z

Summary

Fixes the exec approval fire-and-forget issue where the tool returned before registration
Implements two-phase approval: registration confirmation, then async decision waiting
Ensures approval ID is valid immediately after tool returns 'approval-pending'

Changes

ExecApprovalManager: Added register() and awaitDecision() methods
exec-approval handler: Two-phase response pattern + new waitDecision endpoint
bash-tools.exec: Await registration before returning (both node and gateway hosts)
Tests: Updated mocks to handle new internal call pattern

Greptile Overview

Greptile Summary

This PR fixes an exec-approval race where bash-tools.exec could return approval-pending before the approval was actually registered on the gateway.

Key changes:

Adds ExecApprovalManager.register() and awaitDecision() to separate “register approval” from “wait for decision”, and retains resolved entries briefly to support late waiters.
Updates exec.approval.request to optionally use a two-phase response (immediate {status:"accepted"} when twoPhase is true, then a final response with {decision}), and adds an exec.approval.waitDecision handler for the second phase.
Updates bash-tools.exec (node + gateway hosts) to first call exec.approval.request with { twoPhase: true } and expectFinal:false to confirm the approval ID exists, then asynchronously calls exec.approval.waitDecision to get the decision.
Adjusts tests/mocks to match the new call pattern.

Overall, the changes fit into the existing gateway protocol by leveraging expectFinal:false for early-return semantics while preserving the default single-result behavior for existing callers.

Confidence Score: 4/5

This PR is reasonably safe to merge, with one semantic edge case around timeout retention and late waiters to consider.
Core changes are localized and tested, and prior concurrency issues discussed in the thread appear addressed. Remaining concern is that timed-out approvals are retained and still discoverable via awaitDecision() for a grace window, which may blur the distinction between "expired/not found" vs "timed out" for late callers.
src/gateway/exec-approval-manager.ts (timeout/retention semantics), src/gateway/server-methods/exec-approval.ts (waitDecision contract)

…Manager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions.

…ec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals.

Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402

Add assertion for immediate 'accepted' response before final decision.

Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation.

greptile-apps

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/gateway/exec-approval-manager.ts

greptile-apps · 2026-02-03T03:54:47Z

Additional Comments (1)

src/gateway/exec-approval-manager.ts
[P1] register() can silently overwrite an existing pending entry with the same record.id, breaking the original request’s promise/timer and making resolve() apply to the wrong caller.

Because register() does this.pending.set(record.id, …) unconditionally, a second registration with the same id will replace the first PendingEntry. That can happen if a client retries exec.approval.request with the same explicit id (or if some other codepath calls register() directly). In that case the first request will hang/timeout unexpectedly, and the decision may be delivered to the newer entry.

Consider guarding inside register() (e.g., return existing promise or throw) to make this invariant local to the manager, rather than relying on caller-side checks.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/gateway/exec-approval-manager.ts
Line: 46:85

Comment:
[P1] `register()` can silently overwrite an existing pending entry with the same `record.id`, breaking the original request’s promise/timer and making `resolve()` apply to the wrong caller.

Because `register()` does `this.pending.set(record.id, …)` unconditionally, a second registration with the same id will replace the first `PendingEntry`. That can happen if a client retries `exec.approval.request` with the same explicit id (or if some other codepath calls `register()` directly). In that case the first request will hang/timeout unexpectedly, and the decision may be delivered to the newer entry.

Consider guarding inside `register()` (e.g., return existing `promise` or throw) to make this invariant local to the manager, rather than relying on caller-side checks.


How can I resolve this? If you propose a fix, please make it concise.

ramin-shirali · 2026-02-04T09:51:44Z

@greptileai Please re-evaluate this PR considering the changes.

greptile-apps

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/gateway/server-methods/exec-approval.ts

ramin-shirali · 2026-02-04T10:04:41Z

@greptileai Please re-evaluate this PR considering the changes.

greptile-apps

_{2 files reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/gateway/exec-approval-manager.ts

src/gateway/server-methods/exec-approval.ts

ramin-shirali · 2026-02-04T10:18:16Z

@greptileai Please re-evaluate this PR considering the changes.

greptile-apps

_{3 files reviewed, 3 comments}

_{Edit Code Review Agent Settings | Greptile}

src/gateway/exec-approval-manager.ts

src/gateway/server-methods/exec-approval.ts

greptile-apps · 2026-02-04T10:20:37Z

Additional Comments (1)

src/agents/bash-tools.exec.approval-id.test.ts
[P1] Test mock still returns { decision: "allow-once" } for exec.approval.request, but the implementation now uses exec.approval.waitDecision for decisions.

In skips approval when node allowlist is satisfied, the mock has an exec.approval.request branch returning a decision. That branch should now be unreachable, and if it becomes reachable again it likely indicates a regression back to the old flow. Removing/adjusting this branch would make the test fail in the right place if the call pattern changes.

Prompt To Fix With AI

This is a comment left during a code review.
Path: src/agents/bash-tools.exec.approval-id.test.ts
Line: 107:120

Comment:
[P1] Test mock still returns `{ decision: "allow-once" }` for `exec.approval.request`, but the implementation now uses `exec.approval.waitDecision` for decisions.

In `skips approval when node allowlist is satisfied`, the mock has an `exec.approval.request` branch returning a decision. That branch should now be unreachable, and if it becomes reachable again it likely indicates a regression back to the old flow. Removing/adjusting this branch would make the test fail in the right place if the call pattern changes.

How can I resolve this? If you propose a fix, please make it concise.

ramin-shirali · 2026-02-04T10:45:10Z

@greptileai Please re-evaluate this PR considering the changes.

greptile-apps

_{1 file reviewed, 2 comments}

_{Edit Code Review Agent Settings | Greptile}

src/gateway/server-methods/exec-approval.ts

ramin-shirali · 2026-02-04T11:45:23Z

@greptileai Please re-evaluate this PR considering the changes.

greptile-apps

_{1 file reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

src/gateway/exec-approval-manager.ts

…irali

…irali

@ramin-shirali

… thanks @ramin-shirali

# Conflicts: # CHANGELOG.md

steipete · 2026-02-13T18:57:16Z

Landed. Thanks @ramin-shirali.

Landed head SHA: d8e922d2838c3a4137e68c75c611f5101ee3e4ad
Merge commit: 1af0edf7ff222f6784a1ffa45cdacc89fa383feb

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]> (cherry picked from commit 1af0edf) # Conflicts: # src/discord/monitor/threading.test.ts

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]> (cherry picked from commit 1af0edf) # Conflicts: # CHANGELOG.md # src/agents/bash-tools.exec.ts # src/agents/pi-tools.workspace-paths.test.ts # src/discord/monitor/threading.test.ts # src/gateway/exec-approval-manager.ts

@ramin-shirali

…) (openclaw#3357) * feat(gateway): add register and awaitDecision methods to ExecApprovalManager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions. * feat(gateway): add two-phase response and waitDecision handler for exec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals. * fix(exec): await approval registration before returning approval-pending Ensures the approval ID is registered in the gateway before the tool returns. Uses exec.approval.request with expectFinal:false for registration, then fire-and-forget exec.approval.waitDecision for the decision phase. Fixes openclaw#2402 * test(gateway): update exec-approval test for two-phase response Add assertion for immediate 'accepted' response before final decision. * test(exec): update approval-id test mocks for new two-phase flow Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation. * fix(lint): add cause to errors, use generics instead of type assertions * fix(exec-approval): guard register() against duplicate IDs * fix: remove unused timeoutMs param, guard register() against duplicates * fix(exec-approval): throw on duplicate ID, capture entry in closure * fix: return error on timeout, remove stale test mock branch * fix: wrap register() in try/catch, make timeout handling consistent * fix: update snapshot on timeout, make two-phase response opt-in * fix: extend grace period to 15s, return 'expired' status * fix: prevent double-resolve after timeout * fix: make register() idempotent, capture snapshot before await * fix(gateway): complete two-phase exec approval wiring * fix: finalize exec approval race fix (openclaw#3357) thanks @ramin-shirali * fix(protocol): regenerate exec approval request models (openclaw#3357) thanks @ramin-shirali * fix(test): remove unused callCount in discord threading test --------- Co-authored-by: rshirali <[email protected]> Co-authored-by: rshirali <[email protected]> Co-authored-by: Peter Steinberger <[email protected]>

…irali

…irali

…irali

…irali

openclaw-barnacle bot added gateway Gateway runtime agents Agent runtime and tooling labels Jan 28, 2026

rshirali added 5 commits January 31, 2026 10:00

feat(gateway): add register and awaitDecision methods to ExecApproval…

2e59fa1

…Manager Separates registration (synchronous) from waiting (async) to allow callers to confirm registration before the decision is made. Adds grace period for resolved entries to prevent race conditions.

feat(gateway): add two-phase response and waitDecision handler for ex…

8d7addb

…ec approvals Send immediate 'accepted' response after registration so callers can confirm the approval ID is valid. Add exec.approval.waitDecision endpoint to wait for decision on already-registered approvals.

test(gateway): update exec-approval test for two-phase response

df9e2ad

Add assertion for immediate 'accepted' response before final decision.

test(exec): update approval-id test mocks for new two-phase flow

8936455

Mock both exec.approval.request (registration) and exec.approval.waitDecision (decision) calls to match the new internal implementation.

ramin-shirali force-pushed the fix/exec-approval-race-condition branch from ff66581 to 8936455 Compare January 31, 2026 09:07

fix(lint): add cause to errors, use generics instead of type assertions

03e5fdd

greptile-apps bot reviewed Feb 3, 2026

View reviewed changes

src/gateway/exec-approval-manager.ts Show resolved Hide resolved

rshirali and others added 2 commits February 3, 2026 18:34

fix(exec-approval): guard register() against duplicate IDs

4e1f0ee

Merge branch 'main' into fix/exec-approval-race-condition

7ee4a8f