feat(runner): hold approval gates open on the live session (Claude ACP gates) - #5158
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
| // Signal the parkable gate BEFORE the latch so a keep-alive resume can record the pending | ||
| // permission id and the multi-gate detector counts every pending gate (not just the first). | ||
| const gateToolCallId = stringValue(req?.toolCall?.toolCallId); | ||
| onUserApprovalGate?.({ |
There was a problem hiding this comment.
This callback is the single seam that can mark a gate as parkable, and it fires only on the harness ACP permission path. pauseClientTool never calls it, and Pi gates never reach this file, so the v1 scope (Claude ACP gates only) is enforced structurally rather than by checks scattered through the dispatch.
| // here would leave them as orphaned open parts whenever the dispatch later refuses the park | ||
| // (multi-gate, pool full) — `env.destroy()` does not re-run it. The exclusion keeps the | ||
| // gated (paused) call itself open, so the live resume is untouched. | ||
| run.settleOpenToolCalls( |
There was a problem hiding this comment.
The F-024 sibling settle runs unconditionally, park mode or not. Latch-loser tool calls announced before the winning gate can never execute this turn, and env.destroy() does not re-run this sweep, so skipping it when the dispatch later refuses the park (multi-gate, pool full) would orphan those parts. The exclusion keeps the gated call itself open for the live resume. This exact gap was the top finding of both review passes.
| // non-parkable pause (flag off, Pi relay/builtin gate, client tool) never records | ||
| // `parkedApproval`, so it still tears down here exactly as today. | ||
| if (opts.approvalParkMode && env.parkedApproval) return; | ||
| // Abort any in-flight loopback `tools/call` (a paused Claude client tool) BEFORE the |
There was a problem hiding this comment.
Park mode skips only the session-destroying steps (mcpAbort, destroySession). If the dispatch then decides not to park, env.destroy() still runs them, so nothing is lost; if it parks, the live session keeps the pending gate and the suspended prompt.
| }); | ||
| promptPromise = Promise.resolve(opts.resume.promptPromise); | ||
| promptPromise.catch(() => {}); | ||
| await env.session.respondPermission( |
There was a problem hiding this comment.
The whole point of the feature: the runner answers the gate the harness is still holding, so the ORIGINAL tool call executes with its original byte-exact arguments. No model re-issues the call, which removes the argument-drift and task-restart failures from the 2026-07-07 production turns at the root. The new turn only streams and traces; nothing is re-baked into the live session.
| * most once: only the checkout winner ever holds the parked permission id. After the resume | ||
| * turn, `repark` re-inserts only if the slot is still empty (see there). | ||
| */ | ||
| checkoutApproval(key: string): LiveSession<E> | undefined { |
There was a problem hiding this comment.
checkoutApproval removes the session from the map, so the resume turn owns the environment exclusively. A duplicate approval or a racing fresh message just misses the pool and runs cold (today's concurrent-request semantics) instead of destroying an environment that is mid-way through executing the approved tool. This also makes respondPermission at-most-once: only the checkout winner holds the parked permission id.
| * exactly the gate that parked. An incoming reply for a different id, or a plain user message, | ||
| * yields undefined and the dispatch degrades to cold. | ||
| */ | ||
| export function approvalDecisionForToolCall( |
There was a problem hiding this comment.
Decision matching reuses the cold path's envelope parsing (responder.approvalDecisionOf) and matches strictly by the parked gate's toolCallId. The cold decision-map machinery itself is untouched and remains the fallback for TTL expiry, restarts, misses, and non-Claude gates.
| // try/catch handles the failure) or after a supersede is not ours and does nothing. `evict` is | ||
| // idempotent through the session's one destroy, so no double-destroy is possible. The promise | ||
| // already carries runTurn's swallowing catch, so no unhandled rejection is introduced. | ||
| const watchParkedPrompt = (env: SessionEnvironment): void => { |
There was a problem hiding this comment.
A parked session whose held prompt rejects (sandbox died mid-park) is evicted immediately instead of occupying a pool slot for the full 10-minute approval TTL. The handler is identity-checked, so a rejection that arrives after a successful checkout does nothing; the resume turn's own error handling covers that case.
| return false; | ||
| } | ||
| if ((env.approvalGateCount ?? 0) > 1) { | ||
| klog(`multi-gate-no-park key=${key} gates=${env.approvalGateCount}`); |
There was a problem hiding this comment.
v1 parks only the single-gate case. Two or more unanswered gates at pause time refuse the park and tear down cold, exactly as today. The greppable multi-gate-no-park line is how QA distinguishes this deliberate refusal from a miss.
Adds AGENTA_RUNNER_SESSION_KEEPALIVE, _SESSION_TTL_MS, _SESSION_APPROVAL_TTL_MS, and _SESSION_POOL_MAX to the canonical agent env-var list. The flags land across the two keep-alive slices (#5156, #5158); this stacked lane carries the doc so both are in by the time it applies. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
Fix: approval resume no longer requires re-minted credentials to match (the "approve twice" bug)Live symptom. On the dev box every tool approval had to be clicked twice. The first approval never took effect; the turn restarted and asked again. Root cause. When a Claude ACP permission gate parks a live session in What the fix changes. On the Test evidence. Files: Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv |
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
services/runner/tests/unit/session-keepalive-approval.test.ts (1)
442-542: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMissing coverage for approval-resume failure paths.
The suite thoroughly exercises validation mismatches, TTL, disconnect, double-answer, and prompt rejection, but there is no case where the resume
runTurnitself throws (resume-threw, Lines 618-624 of server.ts) or returns!ok(resume-failed, Lines 625-629). Those paths callpool.evictIfCurrenton a session already removed from the map bycheckoutApproval; a test assertingenv1.destroyed === 1(andcalls.acquire === 2) after a throwing/failed resume would confirm the checked-out env is actually torn down and not leaked.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b60a7a66-0994-4741-9ad5-358b0c1aabd1
📒 Files selected for processing (7)
docs/design/agent-workflows/documentation/running-the-agent.mdservices/runner/src/engines/sandbox_agent.tsservices/runner/src/engines/sandbox_agent/acp-interactions.tsservices/runner/src/engines/sandbox_agent/session-pool.tsservices/runner/src/server.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-pool.test.ts
| * short-lived material every time, so they practically never match), but an expired mount means | ||
| * the parked cwd can no longer be written, so it must still evict to cold. | ||
| */ | ||
| export function mountCredentialsExpired( |
There was a problem hiding this comment.
On the mountCredentialsExpired check: this is a liveness check, not an authorization check, and that is the key difference from the project-scope case.
A parked session's cwd is a live geesefs (FUSE) mount backed by short-lived credentials minted at acquire time. If those credentials expire while the session sits parked, the cwd becomes a dead mount: the resumed tool would fail on its first write to the durable store. So before resuming, we verify the parked environment is still physically operable. Expiry means the environment is no longer usable (the mount's own concern), so we evict to cold and re-acquire with fresh credentials. That is unlike the project-scope case, which borrowed the mount for identity and is being moved off it (follow-up 5, in progress).
No code change needed. The structural-cleanup follow-up will rename this to make the intent read at the call site, e.g. parkedCwdMountExpired.
dosubot
Bot
added
size:XXL
8dd2ee5 to
2084078
Compare
Adds AGENTA_RUNNER_SESSION_KEEPALIVE, _SESSION_TTL_MS, _SESSION_APPROVAL_TTL_MS, and _SESSION_POOL_MAX to the canonical agent env-var list. The flags land across the two keep-alive slices (#5156, #5158); this stacked lane carries the doc so both are in by the time it applies. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
7551a87 to
2cac1dc
Compare
dosubot
Bot
added
size:XL
2084078 to
8d774de
Compare
2cac1dc to
7ef57be
Compare
|
Follow-up: the approval TTL default is now 5 minutes (300000 ms), landed in #5178. The design decision is recorded in session-keepalive/open-questions.md item 2. |
1 participant
Context
This is slice 2 of the session-keepalive plan (#5153), stacked on #5156. Slice 1 keeps a session alive across normal turn boundaries. This slice extends it to the approval pause, which is where both 2026-07-07 production failures happened: the runner destroyed the session at the moment a tool call needed approval, so a fresh model had to re-issue the call from a lossy text transcript. Its regenerated arguments drifted (three approval rounds for one curl) or it restarted the task entirely (five rounds, zero commits). Full analysis: cold-replay-failure-report.md.
With this slice, a Claude ACP permission gate parks the live session holding the still-pending permission request and the suspended prompt. When the human clicks approve or deny, the runner answers that parked request directly. The original tool call runs with its original byte-exact arguments. No model re-issues anything, so argument drift and task restart cannot happen on the live path.
What changed
acp-interactions.ts: a newonUserApprovalGatecallback fired only on the harness ACP permission path (never for client tools or Pi gates). This is the sole seam that marks a gate as parkable.sandbox_agent.ts: park mode in the pause callback. The F-024 sibling settle runs unconditionally on every pause; only the session-destroying steps are skipped when parking. The resume branch answers the parked gate viarespondPermission, seeds the parked tool call into the new turn's trace, resolves the durable interaction, and awaits the original suspended prompt.session-pool.ts: theawaiting_approvalstate with the 10-minute TTL,checkoutApproval(removes the session from the map so a racing request can never destroy an in-flight resume), and strict decision matching by the parked gate'stoolCallId(reusing the cold path's envelope parsing).server.ts: the resume dispatch branch (validate config, history, credential epoch, and the tool call id, then resume), park finalizers, and a watcher that evicts a parked session immediately if its held prompt rejects (a dead sandbox does not occupy a slot for the full TTL).Scope and risk
How to QA
Prerequisites: slice 1 QA passing (flag on, local sandbox, playground conversation), a Claude-harness agent with a gated tool (needs an anthropic provider key). Confirm mount signing works first (a
[keepalive] parkline proves it); without it keep-alive silently runs all-cold by design.AGENTA_RUNNER_SESSION_KEEPALIVE=true, ask the agent to run a gated tool. The approval prompt appears in the UI; the runner logs[keepalive] park-approval ...and the turn ends paused.AGENTA_RUNNER_SESSION_APPROVAL_TTL_MS). The runner logs[keepalive] resume-approve ...; the tool runs immediately with the original arguments; no second approval round appears. Compare with today: no[HITL] resume statedecision-map lines on this path.[keepalive] resume-reject ...; the model handles the rejection in the same live session.[keepalive] approval-ttl-expire ...; the click then resumes through today's cold decision-map path (the[HITL]lines reappear); behavior matches current production.[keepalive] non-claude-gate-no-park), cold behavior exactly as today.Test command:
cd services/runner && pnpm test(717 unit tests at this PR's head: everything from slice 1 and big-agents unchanged, plus the approval-parking and review-round tests) andpnpm run typecheck.Edge cases covered by tests: park-and-answer approve and deny, approval TTL expiry degrading to the cold decision map, non-Claude gates never parking, multi-gate pauses never parking, pre-pause sibling tool calls settled as not-executed on every pause path, resume mismatch (edited history, changed config, rotated credentials) evicting to cold, awaiting_approval sessions exempt from LRU eviction, a duplicate approval racing an in-flight resume (gate answered exactly once, resume never destroyed mid-flight), and a parked prompt rejection evicting promptly.
https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv