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.githubusercontent.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
…tes only) Slice 2 of the session-keepalive plan (#5153), stacked on the pool slice. A Claude ACP permission gate now parks the live session (awaiting_approval, 10-minute TTL) holding the pending permission id and the suspended prompt. The human's approve or deny answers the parked request directly via respondPermission, so the original tool call executes with its original byte-exact arguments; no model re-issues anything. This removes the argument-drift and task-restart failure classes from the live path. - Pi relay gates, Pi builtin gates, and client-tool MCP pauses never park (cold as today, asserted by tests); the cold decision-map fallback is untouched and covers TTL expiry, restarts, and every mismatch - the F-024 latch-loser sibling settle runs unconditionally on every pause - checkoutApproval removes the session from the map so a racing request can never destroy an in-flight resume; respondPermission is at-most-once - a parked prompt rejection evicts the session immediately Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
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
…o match Every tool approval on the dev box needed approving twice. Root cause: the awaiting_approval dispatch branch required the resume request's config fingerprint and credential epoch to equal the parked session's. But each approval reply is a fresh /run the backend mints with freshly minted short-lived material (gateway/Composio secret values, a per-turn tool-callback bearer), so the credential epoch -- and often the config fingerprint that can embed those tokens -- practically never matched. The parked live session was evicted on every approval, the turn ran cold, and cold-replay argument drift re-prompted the user. Fix: the awaiting_approval branch keeps the approval-decision match, the history fingerprint, and a hard mount-expiry bound (an expired parked mount can no longer write its cwd, so it still evicts to cold), but drops the config and credential-epoch equality checks. The parked process already holds its own credentials baked at acquire time; the resume only delivers the human yes/no. The idle-continuation branch keeps its full validation, with the ambiguous credentials mismatch reason split into credentials-expired vs credentials-rotated for log diagnosis. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
Restore time-based run limits in the split turn path (P1), skip the cold retry after a failed live turn already streamed to the client (P2), and await a replaced same-key session's teardown before parking (P2). Rides the stacked approvals lane because the streaming-retry fix edits the slice-2 approval-resume branch; lands with the stack bottom-up into big-agents. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
…ct helpers) Readability-only, no behavior change. Renames the SessionEnvironment `env2` to `environment` and extracts the two session-lifetime listeners into documented `routeSessionEventToActiveTurn` / `routePermissionRequestToActiveTurn` helpers, so `acquireEnvironment` reads as clear steps and the demux data flow is obvious. Addresses Mahmoud's #5156 review note that the event handling was hard to read. 723 tests unchanged, typecheck clean, reviewer confirmed no semantic drift. Lands on the stacked approvals lane (top of the keep-alive stack) because the working tree is the stack's applied state and GitButler attributes the edit to the top lane; it rides into big-agents with the stack. Claude-Session: https://claude.ai/code/session_01AumZJ9xRd4XYNHThqy4rTv
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