[fix] Drain approval events before done

[mmabrouk]

Context

When a tool requested human approval, the runner signaled the pause immediately and began cancelling the managed session. It could emit the terminal done event before cancellation and already-queued ACP tool updates finished, leaving a late sibling tool call without its deterministic terminal result.

Changes

The pause controller now tracks a separate event-drain promise. The human-facing pause signal remains immediate, while terminalization waits for managed cancellation plus one event-loop turn. After the drain, the runner re-sweeps paused sibling tool calls before emitting done.

Cancellation failures remain best-effort. A rejected or synchronously thrown destroy operation cannot block the pause.

Tests / notes

• Added focused tests proving the pause signal is immediate, queued updates drain before completion, and destroy failures are absorbed.
• Focused tests: 3 passed.
• Runner typecheck passed.
• Full runner suite during extraction: 1062 of 1065 passed. The three failures were pre-existing shared-lane failures in model-credential and replay tests, outside this PR.

How to review

1. Read PendingApprovalPauseController.pause() and waitForEventDrain().
2. Read the terminalization block in sandbox_agent.ts that awaits the drain and re-sweeps siblings.
3. Read pending-approval-pause.test.ts for timing and failure behavior.

What to QA

• Trigger a tool approval while another tool update is queued. The approval prompt should appear immediately, the sibling receives one terminal result, and done remains the final event.

[vercel]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project	Deployment	Actions	Updated (UTC)
agenta-documentation	Ready	Preview, Comment	Jul 13, 2026 12:34am

[coderabbitai]

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 0bebfa97-5a0d-46db-ab17-db4f31027583

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Paused turns now await cancellation-related event draining before performing a final sweep that settles remaining paused tool calls. The pause controller tracks session-destruction completion and handles rejection or synchronous errors without rejecting the pause signal.

Changes

Paused turn lifecycle

Layer / File(s)	Summary
Pause event-drain lifecycle services/runner/src/engines/sandbox_agent/pause.ts, services/runner/tests/unit/pending-approval-pause.test.ts	PendingApprovalPauseController tracks session-destruction and queued event completion, exposes waitForEventDrain(), swallows cancellation failures, and tests ordering and failure handling.
Post-drain paused tool settlement services/runner/src/engines/sandbox_agent.ts	Paused runTurn flows await event draining before settling remaining paused tool calls with TOOL_NOT_EXECUTED_PAUSED.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant runTurn
  participant PauseController
  participant Session
  participant ToolCalls
  runTurn->>PauseController: waitForEventDrain()
  PauseController->>Session: finish destroySession and queued updates
  Session-->>PauseController: drain complete
  PauseController-->>runTurn: resolve event drain
  runTurn->>ToolCalls: settle paused calls as not executed

Loading

Possibly related PRs

• Agenta-AI/agenta#5066: Updates the permission-relay and pending-approval pause handshake.
• Agenta-AI/agenta#5102: Adjusts pause cancellation and session-destruction timing.
• Agenta-AI/agenta#5158: Changes paused-turn handling and approval park/resume teardown.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly captures the main change: draining approval events before emitting done.
Description check	✅ Passed	The description accurately explains the pause-drain fix, re-sweep behavior, and related tests.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/approval-event-drain

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[mmabrouk]

🤖 The AI agent says:

This is the complete approval event-drain fix extracted from #5242, including the caller hunk that was initially hidden in the mixed sandbox_agent.ts commit. Review in this order: pause controller, terminalization wait/re-sweep, then the focused timing tests. The pause signal must stay immediate, while done must wait for queued ACP updates.

@coderabbitai review

[coderabbitai]

@mmabrouk I'll review the PR now, following the order you outlined: PendingApprovalPauseController.pause() and waitForEventDrain(), then the terminalization/re-sweep block in sandbox_agent.ts, and finally the timing tests in pending-approval-pause.test.ts.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh pr view 5267 --repo Agenta-AI/agenta --json title,body,files -q '.files[].path'
echo "---DIFF---"
gh pr diff 5267 --repo Agenta-AI/agenta > /tmp/pr5267.diff
wc -l /tmp/pr5267.diff
grep -n "PendingApprovalPauseController\|waitForEventDrain\|pending-approval-pause" -r . --include="*.ts" || true

Length of output: 2903

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== pause.ts ==="
cat -n services/runner/src/engines/sandbox_agent/pause.ts
echo "=== sandbox_agent.ts around 1500-1560 (pause construction) ==="
sed -n '1480,1560p' services/runner/src/engines/sandbox_agent.ts
echo "=== sandbox_agent.ts around 1800-1900 (terminalization) ==="
sed -n '1780,1900p' services/runner/src/engines/sandbox_agent.ts
echo "=== pending-approval-pause.test.ts ==="
cat -n services/runner/tests/unit/pending-approval-pause.test.ts
echo "=== full diff ==="
cat /tmp/pr5267.diff

Length of output: 20549

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== settleOpenToolCalls definition ==="
rg -n "settleOpenToolCalls" -A 25 services/runner/src/engines/sandbox_agent.ts | head -100
echo "=== search other definition files ==="
rg -rn "settleOpenToolCalls" services/runner/src --type ts
echo "=== TOOL_NOT_EXECUTED_PAUSED ==="
rg -n "TOOL_NOT_EXECUTED_PAUSED" services/runner/src

Length of output: 5504

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n "settleOpenToolCalls" -B5 -A 30 services/runner/src/tracing/otel.ts

Length of output: 3703

---

@mmabrouk Reviewed the three files in the suggested order. The fix is sound.

PendingApprovalPauseController (services/runner/src/engines/sandbox_agent/pause.ts)

• pause() now separates the immediate human-facing signal (resolvePause) from the drain promise (eventDrain). The synchronous try/catch around destroySession() plus the .catch(() => {}) on the resulting promise correctly absorbs both synchronous throws and async rejections from managed cancellation, so a failing destroySession can never block the pause signal or hang waitForEventDrain().
• The extra setImmediate hop after cancellation settles is a reasonable way to let already-scheduled ACP update callbacks flush before terminalization proceeds.

Terminalization block (sandbox_agent.ts, Line 1838)

• Gating the await pause.waitForEventDrain() + re-sweep on stopReason === "paused" is correct — it only runs when pause.pause() was actually invoked, so eventDrain is guaranteed to be initialized.
• The re-sweep calling run.settleOpenToolCalls(...) a second time is safe: I checked settleOpenToolCalls in services/runner/src/tracing/otel.ts (Line 1272) — it deletes each tool span from the map as it settles it, so a second call only touches genuinely new/late entries and is idempotent with the settle already done inside the destroySession callback.

Tests (pending-approval-pause.test.ts)

• Traced through the ordering in the first test manually: the setImmediate scheduled inside finishDestroy (pushing "session update") is enqueued before the drain's own setImmediate (which is only scheduled later, via the microtask chain after resolve() fires), so the assertion that queuedUpdates is populated before drainFinished becomes true is guaranteed by Node's event-loop ordering, not flaky timing.
• The reject/throw tests correctly confirm pause.signal and waitForEventDrain() both resolve without rejecting.

One minor note for awareness rather than a blocker: the drain is a single extra event-loop turn (one setImmediate) past cancellation settling. If a harness/ACP client ever queues sibling updates across more than one macrotask hop (nested timers/immediates), a very late update could still slip past the re-sweep. That seems like an accepted, intentional scope boundary given the PR description and tests, but worth keeping in mind if similar races surface later.

No blocking issues found — approving from a code-review standpoint.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.