feat(runner): reliable multi-gate approvals: truthful results, persisted answers, per-card dispatch#5382
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR spans three main slices: (1) runner-side concurrent human-approval support, removing the single-gate latch and enabling multiple parked approval gates with plural resume decisions; (2) a backend sessions API rewrite introducing ChangesConcurrent approvals
Sessions API rewrite
HarnessType→HarnessKind rename
Runner config
Estimated code review effort: 5 (Critical) | ~150 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
✅ Action performedReview finished.
|
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Plan review: approved with decisions on the four open questions, binding for the implementation.
-
Yes, remove the client-tool side of the latch and delete
PendingApprovalLatchentirely, on the condition the implementer greps for every consumer first and the test suite pins that no other behavior leaned on it. Dead coordination primitives left in place get resurrected by accident. -
The watchdog watches the whole set. A turn with parked gates is pending until every gate is resolved; watching only the first gate recreates the width-one assumption we are removing.
-
Proceed as planned on the Claude concurrency unknown. Steps 1 and 2 fix #5373 regardless of whether the Claude adapter holds gates concurrently. The step-3 live warm-path test is the arbiter: if Claude serializes its permission requests, document that as the harness's own behavior, keep the plural machinery (Pi and future harnesses benefit), and do not build workarounds.
-
Yes, the live rendering check must explicitly cover two cards plus the #5078 dedup interaction, including the deny-one-approve-one combination now that #5381 gives denials their own frame.
The #5373 confirmation is the important find here: the implementation PR should carry 'Closes #5373'. Sequencing stays as agreed: client-tools-on-Daytona implements first, this second; step 3 (warm resume) lands after the sessions work settles to avoid churning against #5376. Implementation may start once CodeRabbit's pass is in and Mahmoud has had his look.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
docs/design/agent-workflows/projects/concurrent-approvals/PLAN.md (2)
205-216: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake the two-card rendering test mandatory and deterministic.
The core contract requires independently answerable cards keyed by distinct
toolCallIds, but the component test is marked “if feasible” and the remaining coverage is manual. Add a required test asserting distinct card identity and independent approve/deny behavior.
369-383: 🩺 Stability & Availability | 🔵 TrivialAdd an independent warm-path rollout gate.
The plan calls the multi-gate warm path the least-proven behavior, but its rollback mechanism is a code revert. Gate the warm multi-answer behavior separately, retain the cold fallback, and add metrics for fallback, partial response, rejection, and duplicate-answer cases.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9d25fd2-3e3c-4b18-a4c6-97821edb46d8
📒 Files selected for processing (2)
docs/design/agent-workflows/projects/concurrent-approvals/PLAN.mddocs/design/agent-workflows/projects/concurrent-approvals/README.md
| Order to prefer: deny-frame (#5381) and sessions (#5375/#5376) land, then concurrent-approvals | ||
| steps 1 to 2, then step 3 on top of the sessions machinery. If steps 1 to 2 are ready before | ||
| sessions lands, they can go first since they do not touch the keep-alive files, and step 3 | ||
| follows. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the inconsistent sessions PR reference.
The preceding section identifies the sessions work as PR #5376, but this sequence refers to #5375/#5376. Keep the PR number and branch reference consistent to avoid stacking the implementation on the wrong base.
Proposed wording
- Order to prefer: deny-frame (`#5381`) and sessions (`#5375/`#5376) land, then concurrent-approvals
+ Order to prefer: deny-frame (`#5381`) and sessions (`#5376`) land, then concurrent-approvals📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Order to prefer: deny-frame (#5381) and sessions (#5375/#5376) land, then concurrent-approvals | |
| steps 1 to 2, then step 3 on top of the sessions machinery. If steps 1 to 2 are ready before | |
| sessions lands, they can go first since they do not touch the keep-alive files, and step 3 | |
| follows. | |
| Order to prefer: deny-frame (`#5381`) and sessions (`#5376`) land, then concurrent-approvals | |
| steps 1 to 2, then step 3 on top of the sessions machinery. If steps 1 to 2 are ready before | |
| sessions lands, they can go first since they do not touch the keep-alive files, and step 3 | |
| follows. |
| Related prior work this plan builds on: [../hitl-fix/](../hitl-fix/) (the original | ||
| approve/deny round-trip fix) and [../cold-replay-stopgaps/](../cold-replay-stopgaps/) (the | ||
| text-replay hardening). The ground-truth code trace this plan is built on lives at | ||
| [../../scratch/research-client-tools-and-concurrent-hitl.md](../../scratch/research-client-tools-and-concurrent-hitl.md), | ||
| Question 2. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Complete the trailing research reference.
The README ends with a dangling Question 2. fragment. Fold it into the preceding sentence so readers know exactly which section to open.
Proposed wording
- The ground-truth code trace this plan is built on lives at
- [../../scratch/research-client-tools-and-concurrent-hitl.md](../../scratch/research-client-tools-and-concurrent-hitl.md),
- Question 2.
+ The ground-truth code trace this plan is built on lives at
+ [../../scratch/research-client-tools-and-concurrent-hitl.md](../../scratch/research-client-tools-and-concurrent-hitl.md),
+ specifically Question 2.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Related prior work this plan builds on: [../hitl-fix/](../hitl-fix/) (the original | |
| approve/deny round-trip fix) and [../cold-replay-stopgaps/](../cold-replay-stopgaps/) (the | |
| text-replay hardening). The ground-truth code trace this plan is built on lives at | |
| [../../scratch/research-client-tools-and-concurrent-hitl.md](../../scratch/research-client-tools-and-concurrent-hitl.md), | |
| Question 2. | |
| Related prior work this plan builds on: [../hitl-fix/](../hitl-fix/) (the original | |
| approve/deny round-trip fix) and [../cold-replay-stopgaps/](../cold-replay-stopgaps/) (the | |
| text-replay hardening). The ground-truth code trace this plan is built on lives at | |
| [../../scratch/research-client-tools-and-concurrent-hitl.md](../../scratch/research-client-tools-and-concurrent-hitl.md), | |
| specifically Question 2. |
6474509 to
0071f90
Compare
mmabrouk
changed the title
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Reviewer guide: the four riskiest hunks are flagged inline. The cold-path fix (latch removal + plural record) resolves #5373 on its own; the warm-resume machinery is the least-proven part and is what to scrutinize hardest. Live warm-path QA is the arbiter (evidence in a PR comment).
| // Do NOT force-settle open tool calls here, at first pause. With concurrent approvals a | ||
| // second gated call may still be in flight (its permission request lands a tick after the | ||
| // first gate pauses the turn), and settling it here would orphan a call that is about to | ||
| // emit its own approval card. The orphan settle is deferred to the post-drain sweep below |
There was a problem hiding this comment.
Scrutinize: the eager first-pause settle was removed. This is the subtle consequence of deleting the latch. With gates arriving as separate ACP messages, force-settling open siblings at the first pause used to close a second gate before its own permission request landed — the latch masked this by only ever pausing one gate. Orphan settling now happens once, in the post-drain sweep below (after waitForEventDrain lets every pending gate mark its call paused). Check: does every path that used to rely on the eager settle still settle genuine orphans? The post-drain sweep at ~line 500 and the in-handleUpdate re-sweep are the two remaining settle sites; confirm a late-announced non-gated sibling still settles (pinned by the orchestration test).
| env.parkedApprovals.set(info.toolCallId, record); | ||
| // The first recorded gate is the per-turn representative for logging and the | ||
| // per-turn-uniform validation reads (gate type, history, credentials). | ||
| env.parkedApproval ??= record; |
There was a problem hiding this comment.
Scrutinize: the plural park record. Every parkable gate is now recorded, keyed by its own toolCallId (was: only the first, guarded by approvalGateCount === 1). approvalGateCount still counts every gate, so parkedApprovals.size !== approvalGateCount means a gate lacked a resumable id — the dispatch treats that whole turn as unresumable and stays cold. Check the key choice: a gate with an empty toolCallId is counted but not recorded, which is what makes that inequality detectable.
| title: decision.toolName, | ||
| kind: decision.toolName, | ||
| rawInput: decision.args, | ||
| }); |
There was a problem hiding this comment.
Scrutinize: the warm resume settles each gate by its own tool-call id. One respondPermission per parked gate; all decisions share the one held prompt promise (one prompt per turn). This assumes the harness holds several pending permission requests concurrently and answers each independently — the open question for the Claude ACP adapter (#5373 strongly implies yes; the live warm-path test is the arbiter). Known gap called out for review: the loop is not atomic — if gate 1's respondPermission succeeds and gate 2's throws, gate 1 may already be executing. Today that surfaces as a turn error the dispatch handles; a first-reply-succeeds / second-reply-fails test and an explicit no-cold-retry-after-partial-success contract are follow-ups.
| // ANY parked prompt means the harness or sandbox died mid-park and the dead session must be | ||
| // evicted. Every parked gate shares the one turn prompt promise, so the catches are on the | ||
| // same promise; the eviction is identity-checked and idempotent, so repeated catches are safe. | ||
| for (const parked of env.parkedApprovals.values()) { |
There was a problem hiding this comment.
Scrutinize: watchdog set semantics. The parked-prompt eviction now watches the WHOLE parked set, not just the first gate — a turn is pending until every gate resolves, and a rejection of any parked prompt evicts the dead session. Every parked gate shares the one turn prompt promise, so these catches attach to the same promise; the eviction is identity-checked and idempotent, so repeated catches are safe (no double-evict).
| mismatch = "no-matching-approval"; | ||
| break; | ||
| } | ||
| resumeDecisions.push({ |
There was a problem hiding this comment.
Scrutinize: the all-or-cold resume rule. A multi-gate turn resumes live ONLY when the request answers EVERY parked gate; a partial answer degrades to cold (which re-raises and multiplexes the subset). The frontend's all-settled rule guarantees a resume /run is only sent once all cards are answered, so a partial set here means an edited/stale request. Paired fix: inBandAnswerTokens now spares stale-sweep tokens only when the same all-answered condition holds, so a partial answer never strands an interaction row as pending on the cold fallback.
| // the sweep runs; this narrow suppression only masks the cancel artifact.) Residual: a genuine | ||
| // mid-pause tool error also arrives as `failed` and reads as paused here — acceptable, since the | ||
| // paused result invites a retry and the runner has no adapter provenance to tell the two apart. | ||
| if (kind === "tool_call_update" && pause.active && frame?.status === "failed") { |
There was a problem hiding this comment.
Scrutinize: narrowed pause suppression (Codex fix). Only a failed update for a non-gated open call during a pause is suppressed (a managed-cancel artifact → the post-drain sweep settles it as paused). A completed update is NOT suppressed, so an auto-allowed sibling that legitimately finishes on the kept-alive warm session shows its real result. Residual, stated in the comment: a genuine mid-pause tool error also arrives as failed and reads as paused — accepted, since the runner has no adapter provenance to distinguish cancel from error and the paused result invites a retry.
|
@coderabbitai review |
✅ Action performedReview finished.
|
Live QA — wire-level walkthrough (fallback: browser recording unavailable)
Deployment: the EE dev runner container was rebuilt/restarted onto this branch's code ( Scenario: an agent calls two gated tools in one turn ("read file A and file B",
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
services/runner/tests/unit/client-tools.test.ts (1)
38-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExercise
onNonParkablePausein this seam.Capture the callback count and assert that pending verdicts increment it, while deny/fulfilled verdicts do not. Otherwise the mixed-gate cold-fallback signal is only tested through a fake counter.
As per coding guidelines, “Unit-test pure engine-internal logic directly, including
tools/*.”Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 343276b4-34ab-43c5-b903-89570cf88361
📒 Files selected for processing (19)
docs/design/agent-workflows/projects/concurrent-approvals/PLAN.mddocs/design/agent-workflows/projects/concurrent-approvals/README.mdsdks/python/oss/tests/pytest/unit/agents/adapters/test_vercel_stream_park.pysdks/python/oss/tests/pytest/unit/agents/test_ui_messages.pyservices/runner/src/engines/sandbox_agent/acp-interactions.tsservices/runner/src/engines/sandbox_agent/client-tools.tsservices/runner/src/engines/sandbox_agent/environment-setup.tsservices/runner/src/engines/sandbox_agent/run-turn.tsservices/runner/src/engines/sandbox_agent/runtime-contracts.tsservices/runner/src/engines/sandbox_agent/runtime-policy.tsservices/runner/src/permission-plan.tsservices/runner/src/server.tsservices/runner/tests/unit/client-tools.test.tsservices/runner/tests/unit/permission-plan.test.tsservices/runner/tests/unit/sandbox-agent-acp-interactions.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsweb/packages/agenta-playground/tests/unit/agentApprovalResume.test.ts
💤 Files with no reviewable changes (2)
- services/runner/src/permission-plan.ts
- services/runner/tests/unit/permission-plan.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/design/agent-workflows/projects/concurrent-approvals/README.md
| | File | Change | | ||
| | --- | --- | | ||
| | `services/runner/tests/unit/...` (new or extended) | Two-gates-one-turn on the cold path and the warm path; force-settle no longer fires for a second gate; multi-answer warm resume. | | ||
| | `web/packages/agenta-playground/src/state/execution/agentApprovalResume.test.ts` | Two pending cards: no resume until both settled. | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the frontend test path.
The plan names web/packages/agenta-playground/src/state/execution/agentApprovalResume.test.ts, but the supplied stack context places this unit test at web/packages/agenta-playground/tests/unit/agentApprovalResume.test.ts. Update the plan to prevent placing Vitest tests under src/.
As per coding guidelines, unit tests for @agenta/* packages belong under tests/unit/.
Source: Coding guidelines
| - **Plans #5373** ("HITL breaks on multi-file approval flow"). This is exactly the multi-gate | ||
| breakage: several gated tool calls in one turn, capped to one card, re-asked across turns. The | ||
| implementation pull request that follows this plan will close it. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Call #5373 an issue, not a plan.
This document consistently describes #5373 as a GitHub issue, so “Plans #5373” is misleading and may send readers to the wrong tracker.
🧰 Tools
🪛 LanguageTool
[style] ~424-~424: Consider an alternative for the overused word “exactly”.
Context: ... on multi-file approval flow"). This is exactly the multi-gate breakage: several gate...
(EXACTLY_PRECISELY)
| // the sweep runs; this narrow suppression only masks the cancel artifact.) Residual: a genuine | ||
| // mid-pause tool error also arrives as `failed` and reads as paused here — acceptable, since the | ||
| // paused result invites a retry and the runner has no adapter provenance to tell the two apart. | ||
| if (kind === "tool_call_update" && pause.active && frame?.status === "failed") { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
file="$(fd -a '^runtime-policy\.ts$' services/runner/src | head -n 1)"
sed -n '31,60p' "$file"Repository: Agenta-AI/agenta
Length of output: 1937
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== services/runner tsconfig files =="
fd -a 'tsconfig.*\.json$' services/runner -t f
echo
echo "== runtime-policy.ts excerpt =="
file="$(fd -a '^runtime-policy\.ts$' services/runner/src | head -n 1)"
sed -n '1,120p' "$file"
echo
echo "== search for status on the asserted frame/update shape =="
rg -n "sessionUpdate\?: unknown; toolCallId\?: unknown|frame\.status|status\?: unknown|tool_call_update" services/runner/src/engines/sandbox_agent -SRepository: Agenta-AI/agenta
Length of output: 6119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="services/runner/src/engines/sandbox_agent/run-turn.ts"
sed -n '180,220p' "$file"Repository: Agenta-AI/agenta
Length of output: 2154
Include status in the frame cast. frame.status is read below, but the asserted shape only declares sessionUpdate and toolCallId, which breaks strict TypeScript. The nearby run-turn.ts handler already treats status as part of the same update shape.
Proposed fix
const frame = update as
- | { sessionUpdate?: unknown; toolCallId?: unknown }
+ | { sessionUpdate?: unknown; toolCallId?: unknown; status?: unknown }
| undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (kind === "tool_call_update" && pause.active && frame?.status === "failed") { | |
| const frame = update as | |
| | { sessionUpdate?: unknown; toolCallId?: unknown; status?: unknown } | |
| | undefined; |
Source: Coding guidelines
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
My commit-level review of the feature commit (0071f90), after the plan review and Codex's pass. Verdict: architecture approved; final sign-off waits only on the live two-card run now in progress, which is also the plan's stated arbiter for the warm path.
What I verified in the diff:
- The latch is genuinely gone, class and all, not bypassed: permission-plan.ts simply no longer exports it, and the emit path has no width-one guard left. The repo-wide grep confirms no surviving code consumer.
- The parkability rule is the right conservative shape. A turn parks only when every pending gate is a parkable ACP permission gate with a resumable id; any mix with client-tool pauses or unresumable gates degrades to the cold path, which is the only path that can multiplex such a set. The three distinct no-park log lines (non-parkable, mixed, unresumable) will make production diagnosis trivial.
- The whole-set watchdog and the per-gate resume are correct where it counts: every parked prompt is watched with an identity-checked, idempotent eviction; the resume builds one decision per tool-call id and refuses to resume live unless the request answers every parked gate, matching the frontend's all-settled predicate exactly, with partial answers degrading to cold rather than half-resuming.
- The sibling-settle fix is a real pre-existing bug caught by this work: deferring orphan settling to the post-drain sweep, with suppression narrowed to managed-cancel failures only, removes the force-settle of a gate whose request had not yet arrived.
Noted as agreed follow-ups, not blockers: the singular parkedApproval kept as the logging representative (drop it in the warm-path hardening pass), partial-respondPermission-failure atomicity with its two failure-order tests, and the pause-registry refactor replacing the summary counters — all Codex items already documented inline.
Merge order stands: #5381 first (this PR uses its denied-frame marker), then this, and at merge time reconcile client-tools.ts and environment-setup.ts against #5383, which shares those two files.
Live QA: concurrent approvals (wire-level, fallback walkthrough)Wire-level verification of this PR against the EE dev stack, driving the real product endpoint ( This is the wire-walkthrough fallback, not a video. Two reasons the video was not possible, stated plainly:
Open question Q3 is settled: Claude raises the gates SERIALLY, not concurrentlyThis is the ship-gate the plan left open for step 3, and the live answer is serial. When the agent emits two parallel gated tool calls in one turn (parallel file reads, the exact #5373 shape, and separately two mutating bash calls), the runner sees exactly one permission request. Evidence, deterministic across three independent sessions and two tool types:
Turn 1 wire frames for "read /etc/hostname and /etc/os-release in parallel" ( The Claude ACP adapter ( Per-scenario result
What this means for the PRThe runner change is sound and does not regress the common path. The latch removal, the plural The gap is the PR's before/after claim. "After: two gated reads in one turn -> two approval cards, each answerable independently, and one resume runs both" does not reproduce on the live Claude harness in this stack, because Claude never delivers two concurrent permission requests. The user still sees one card at a time. This is exactly what the plan flagged as the risk under Q3 ("If the Claude ACP adapter turns out to serialize gates rather than hold them concurrently, step 3's benefit shrinks"), and the live run confirms that branch. Suggestions: temper the headline before/after to reflect serial Claude gating, and confirm the concurrent path against a harness that genuinely raises concurrent gates before trusting the two-card benefit. Pi builtin gating raises a separate gate per tool call and is the natural candidate to check (in these runs Pi auto-allowed, so it was not gated). The single-gate approve and deny paths, and the warm resume, are green and safe to ship. Evidence (JSON frame captures, driver, runner log correlation) under |
Live QA follow-up: the serial-chain value, and the concurrent path on PiA second live pass on the EE dev stack, driving the real product endpoint ( 1. Warm serial chain on Claude — verified, and it is replay-freeCell: Claude, local sandbox,
Three things prove this is the live harness continuing the original prompt, not a cold model replay between gates:
So on the warm path the user answers card 1, the approved tool runs, and card 2 arrives immediately on the same keep-alive session with no model round-trip between them. This is the honest "serial-chain" win. Note it is one card at a time (see §2), and this warm single-gate-per-turn park/resume behavior predates this PR — because the adapters serialize (each turn holds one gate), this PR's plural generalization is not what produces the chain here. 2. The genuinely-concurrent case: Pi also serializesCell: Pi ( Runner log for this session: exactly one So neither live harness (Claude Bottom line
|
dosubot
Bot
added
the
size:XL
Railway Preview Environment
Updated at 2026-07-21T16:15:55.756Z |
dosubot
Bot
added
size:XXL
|
The full incident root cause, the Zed ACP comparison, and the five-step execution plan now live in this PR under docs/design/agent-workflows/projects/approvals-incident-fixes/ (start with README.md for reading order). Implementation is starting per that plan; each step lands as its own commit here. |
3f71d40 to
a1468ec
Compare
1367100 to
fab413d
Compare
a1468ec to
689168a
Compare
Plan-only workspace for removing the one-approval-per-turn latch so each gated tool call in a turn emits its own approval card, pluralizing the parked-approval record and the warm/keep-alive resume path, and verifying the already-plural frontend and SDK behavior with tests. Plans #5373, assesses #5078 and #5097. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
Remove the one-approval-per-turn latch (PendingApprovalLatch) so each gated tool call in a turn emits its own approval card; pluralize the parked-approval record (a Map keyed by tool-call id) and the warm keep-alive resume input (a list of decisions), settling each parked gate by its own tool-call id on a live resume; defer orphan-sibling settling to the post-drain sweep so a concurrent gate is not force-settled before its own permission request arrives. Add runner unit tests (two cards emit, neither force-settled; multi-gate warm park + resume; deny-one-approve-one; mixed/unresumable sets stay cold), SDK vercel adapter plural egress/ingress tests, and a playground all-settled resume test. Closes #5373. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
Complete the plural approval path in run-turn: record every parkable gate into env.parkedApprovals (keyed by tool-call id), iterate opts.resume.decisions to answer each parked gate by its own permissionId on the live session, defer the orphan-sibling settle to the post-drain sweep (so a concurrent gate is not force-settled before its own permission request arrives), and wire the non-parkable-pause signal. Update the acp-interactions unit test to assert both concurrent gates emit their own card. Part of the concurrent-approvals change (#5373); sits on the deny-frame egress lane whose markToolCallDenied the live-resume deny path reuses. Claude-Session: https://claude.ai/code/session_01DnWRxU3dCJ11hgDidm26vW
689168a to
ca104f0
Compare
|
The interactions guard is relaxed per ruling: every approval gate now leaves a durable interaction row even when the run carries no workflow reference (attribution optional; such rows group under the session). In-band answering resolves reference-less rows, and the respond path was verified safe without references. The runner-side guard removal hunks ride the base PR #5436 (hunk attribution); the interactions client, tests, and the handoff rulings update land here. |
|
Live verification caught an integration gap the unit tests missed: the turns-ledger append and interaction rows read the request's execution id, but no caller sets it; the runner resolved the real id for locks and records without writing it back. The dispatch now threads the resolved id onto the request (minting one when absent), pinned by a dispatch-seam test. Runner suite 1244 green. |
Live verification — reference-less approval rows + concurrent-approval regressionEE dev stack, agent Local Claude (Pi harness · Suite baselines: runner D — reference-less approval row (session
|
188025a
into
feat/sessions-storage-rework
1 participant
The symptom
During live QA, a user asked the agent to run two permission-gated shell commands in one turn (append a line to two files). The user approved the first command, and the playground reported it as "not executed" even though it had run. After the user approved the second command the whole conversation went silent and never replied, the already-approved first card flipped back to "waiting for approval", and a later question was answered with a stale "Done" instead of being read. Both files were in fact written exactly once, each only after its approval. What broke was the reporting, the rebuilt state, and the delivery of the final approval to the runner.
The runner is the Node/TypeScript sidecar under
services/runner/that drives a coding-agent harness (Pi or Claude) inside a sandbox. A gate is a human-approval request the harness raises before a policy-gated tool runs. When a gate has no answer the runner ends the turn and parks the live harness session in a keep-alive pool; when the answer arrives it checks the session back out and continues in place (a warm resume).What this PR changes
Truthful results when a turn pauses. Before: the post-pause cleanup swept every open tool call and stamped a "not executed, retry the same call" sentinel onto calls that had actually been approved and were mid-execution, so their real result was dropped and a side-effecting command was invited to run twice. After: an approved, executing call is excluded from the sweep and keeps its real executor result; a call that never started is recorded as deferred, never as a fake success.
Persisted approval answers. Before: the session record stored every approval request but no answer, so any state rebuild (a reload) resurrected answered gates as pending. That is why the first card flipped back to "waiting". After: resolving a gate emits a durable
interaction_responserecord and writes the allow/deny verdict into the interaction row'sresolutionfield. Rebuilt conversations overlay answers onto requests and render answered cards as answered.Per-card dispatch with partial answer sets. Before: the frontend sent approvals only once every visible card looked settled, a condition that could never hold after a rebuild, so the final answer sat unsent in browser memory and the conversation died. After: one click sends one answer. The runner accepts a resume that answers a subset of the parked gates, answers those, streams their results, and re-parks on the rest.
The Pi-batching park rule. Pi does not execute a parallel tool call the moment its own approval resolves; it waits until every sibling call in the batch has been answered, then runs them together. Before: the runner held the resume turn open waiting for the approved call's result, which under Pi could never arrive while a sibling gate was still pending, so the turn hung for the full five-minute per-call limit and the watchdog killed the stream. After: under Pi, when a fresh gate is still parked, the runner skips that bounded wait and parks immediately, carrying the approved call ids so the final resume replaces the interim placeholder with the real result. Claude keeps the bounded wait, where the approved call does finish inside the same turn.
Cross-turn rebuild correctness. Before: on reload, hydration treated the runner's park placeholders as final tool results, so a re-raised gate was sealed shut and the card could never be answered again. After: a later approval request reopens a placeholder-only result but never a real one, and answers are resolved across turn boundaries through a transcript-wide index, so a rebuilt conversation is answerable and matches the live one.
What this PR deliberately does not do
Merge order
This branch is stacked on the sessions rebase: PR #5375 (backend) and PR #5376 (runner) carry the shared
session_turnscounter fix and its 409 mapping, and merge first. This PR's base issessions-rebase/runner; merge it after those two.How this was verified
resolutionverdict.debug/qa-concurrent-approvals/qa-concurrent-approvals-pass.mp4) and is pending a manual browser upload to this thread.