feat(runner): allow multiple simultaneous approval requests in one turn#5382
feat(runner): allow multiple simultaneous approval requests in one turn#5382mmabrouk wants to merge 9 commits into
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
|
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. |
|
Plan step 2 landed on this lane: pause terminalization is now truthful — approved executions keep their real executor results, never-started sibling tool calls are recorded as deferred rather than fabricated, and a new non-retry sentinel marks approved results that were never observed by the executor. Steps 3 and 4 follow on this same lane (plan/concurrent-approvals). |
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
de3135f to
20dcb55
Compare
|
This branch now stacks on the sessions rebase (#5375 backend, then #5376 runner) rather than sitting directly on main. The approvals work persists the answer half of each gate by writing through the session interactions plane, and those two branches are the ones that rebuild that plane, so the DAO and router edits here depend on their commits and must merge first. Retargeting the base to |
|
Step 4 landed: each approval card now dispatches the moment it is answered, and the runner answers exactly the gates a given request carries while keeping the rest parked in the same live session. Next on this lane is the incident regression test and the recorded live QA. |
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (7)
api/oss/src/dbs/postgres/sessions/records/dbes.py (1)
30-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a partial index to save space for historical data.
Since
turn_idis only forward-filled and will remainNULLfor all historical records, indexing thoseNULLvalues bloats the index unnecessarily. Consider making this a partial index with aturn_id IS NOT NULLcondition, consistent with how nullable identifiers are indexed elsewhere in the codebase (e.g.,MountDBE).
api/oss/src/dbs/postgres/sessions/records/dbes.py#L30-L35: addpostgresql_where=text("turn_id IS NOT NULL")to theIndexdeclaration.api/oss/databases/postgres/migrations/tracing_oss/versions/oss000000004_add_records_turn_span.py#L29-L34: addpostgresql_where=sa.text("turn_id IS NOT NULL")to theop.create_indexcall.api/oss/src/apis/fastapi/sessions/models.py (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
SessionStreamResponseis missing thecountfield. Every other response envelope in this module (SessionResponse,SessionsResponse,SessionStreamsResponse,SessionTurnResponse) carriescountalongside its payload; this singular response is the only one that doesn't.As per coding guidelines: "include
countplus payload in response envelopes."Proposed change
class SessionStreamResponse(BaseModel): + count: int = 0 stream: Optional[SessionStream] = NoneSource: Coding guidelines
api/oss/src/apis/fastapi/otlp/extractors/span_data_builders.py (1)
150-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate the repeated
ag.{ns}.{k}update calls.The
session/user/agentattribute updates are now three near-identical lines; a small loop would reduce duplication if more identity namespaces are added later.♻️ Optional consolidation
- attributes.update(**{f"ag.session.{k}": v for k, v in features.session.items()}) - attributes.update(**{f"ag.user.{k}": v for k, v in features.user.items()}) - attributes.update(**{f"ag.agent.{k}": v for k, v in features.agent.items()}) + for _ns, _values in ( + ("session", features.session), + ("user", features.user), + ("agent", features.agent), + ): + attributes.update(**{f"ag.{_ns}.{k}": v for k, v in _values.items()})api/oss/tests/pytest/unit/sessions/test_command_matrix_inputs_data.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_FakeStreamsDAO/lock_enginetest doubles, already diverging. Three new test files copy-paste the same fake streams DAO,lock_enginefixture (patchingLockEngine._client), and_service()helper; the copy intest_kill_runner_teardown.pyhas already drifted by droppingturn_idfromupdate().
api/oss/tests/pytest/unit/sessions/test_command_matrix_inputs_data.py#L41-89: move_FakeStreamsDAO,lock_engine, and_service()into a sharedconftest.py/test-utils module forunit/sessions/, and import from there.api/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.py#L40-88: same — import the shared fixture instead of redefining it.api/oss/tests/pytest/unit/sessions/test_kill_runner_teardown.py#L30-71: same, and restoreturn_idpreservation inupdate()to match the other copies (or confirm it's intentionally irrelevant to this file's assertions) once consolidated.services/runner/src/server.ts (1)
494-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPark/resume logs still surface only one tool name for a multi-gate turn.
env.parkedApproval?.toolName(singular) is used for thepark-approvalandresumelog lines even though a turn can now park several gates (env.parkedApprovals). For a multi-gate turn this always logs only the first gate's tool, which weakens debuggability of exactly the new concurrent-approval scenario this PR introduces.Consider logging all parked tool names (or at least a count) alongside the existing
gates=/approve=/reject=counters.♻️ Example: surface all parked tool names
- klog( - `park-approval key=${key} tool=${env.parkedApproval?.toolName ?? "?"}`, - ); + klog( + `park-approval key=${key} tools=${[...env.parkedApprovals.values()] + .map((g) => g.toolName ?? "?") + .join(",")}`, + );Also applies to: 529-532, 687-688, 744-751
services/runner/tests/unit/server.test.ts (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
listen()test-server helper into a sharedtests/utils/module. All three files define and maintain, in lockstep, the samelisten(run, token)bootstrap helper (same signature, same "force the configured token unconditionally" rationale) — this PR had to edit all three together to keep the token semantics synchronized, which is direct evidence of the duplication cost. As per path instructions, shared test helpers should live intests/utils/where applicable.
services/runner/tests/unit/server.test.ts#L46-L52: replace this locallisten()with an import from a new shared helper (e.g.tests/utils/server-listen.ts).services/runner/tests/acceptance/server-contract.test.ts#L44-L49: replace this locallisten()with the same shared helper.services/runner/tests/integration/server-smoke.test.ts#L50-L55: replace this locallisten()with the same shared helper.Source: Path instructions
services/runner/tests/unit/session-alive-interrupt.test.ts (1)
86-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test doesn't actually exercise the guard it claims to —
release()never callshandleBeat.
release()callssendHeartbeatdirectly (not throughhandleBeat), so it can never triggeronInterruptedregardless of theinterruptedFiredguard. The assertion at Line 102 therefore passes trivially and doesn't validate de-dup across two real interruption-reporting beats. The genuinely untested path is two INTERVAL beats (both routed throughhandleBeat) each reportinginterrupted: true—session-alive.test.ts's fake-timer pattern (vi.useFakeTimers()+vi.advanceTimersByTimeAsync) could drive that directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9e97440-7a24-48dd-aac4-ec583afd5637
⛔ Files ignored due to path filters (8)
api/uv.lockis excluded by!**/*.lockweb/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/MountQueryRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/ArchiveSessionRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/DeleteSessionRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/FetchTurnRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/SessionDetachRequest.tsis excluded by!**/generated/**
📒 Files selected for processing (227)
api/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core/env.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000014_add_session_turns.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000015_add_session_streams_header.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000016_add_mounts_agent_id.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000017_drop_session_states.pyapi/oss/databases/postgres/migrations/tracing_oss/versions/oss000000003_add_span_identity_columns.pyapi/oss/databases/postgres/migrations/tracing_oss/versions/oss000000004_add_records_turn_span.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/mounts/utils.pyapi/oss/src/apis/fastapi/otlp/extractors/adapters/logfire_adapter.pyapi/oss/src/apis/fastapi/otlp/extractors/canonical_attributes.pyapi/oss/src/apis/fastapi/otlp/extractors/span_data_builders.pyapi/oss/src/apis/fastapi/otlp/utils/serialization.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/apis/fastapi/tracing/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/interfaces.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/sessions/dtos.pyapi/oss/src/core/sessions/interactions/dtos.pyapi/oss/src/core/sessions/interactions/interfaces.pyapi/oss/src/core/sessions/interactions/service.pyapi/oss/src/core/sessions/records/dtos.pyapi/oss/src/core/sessions/service.pyapi/oss/src/core/sessions/states/dtos.pyapi/oss/src/core/sessions/states/interfaces.pyapi/oss/src/core/sessions/states/service.pyapi/oss/src/core/sessions/streams/dtos.pyapi/oss/src/core/sessions/streams/interfaces.pyapi/oss/src/core/sessions/streams/runner_client.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/src/core/sessions/turns/__init__.pyapi/oss/src/core/sessions/turns/dtos.pyapi/oss/src/core/sessions/turns/interfaces.pyapi/oss/src/core/sessions/turns/service.pyapi/oss/src/core/sessions/turns/types.pyapi/oss/src/core/shared/dtos.pyapi/oss/src/core/tracing/dtos.pyapi/oss/src/core/tracing/service.pyapi/oss/src/core/tracing/utils/filtering.pyapi/oss/src/core/tracing/utils/trees.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/src/dbs/postgres/mounts/dbas.pyapi/oss/src/dbs/postgres/mounts/dbes.pyapi/oss/src/dbs/postgres/mounts/mappings.pyapi/oss/src/dbs/postgres/sessions/interactions/dao.pyapi/oss/src/dbs/postgres/sessions/records/dao.pyapi/oss/src/dbs/postgres/sessions/records/dbas.pyapi/oss/src/dbs/postgres/sessions/records/dbes.pyapi/oss/src/dbs/postgres/sessions/records/mappings.pyapi/oss/src/dbs/postgres/sessions/states/dao.pyapi/oss/src/dbs/postgres/sessions/states/dbes.pyapi/oss/src/dbs/postgres/sessions/states/mappings.pyapi/oss/src/dbs/postgres/sessions/streams/dao.pyapi/oss/src/dbs/postgres/sessions/streams/dbes.pyapi/oss/src/dbs/postgres/sessions/streams/mappings.pyapi/oss/src/dbs/postgres/sessions/turns/__init__.pyapi/oss/src/dbs/postgres/sessions/turns/dao.pyapi/oss/src/dbs/postgres/sessions/turns/dbas.pyapi/oss/src/dbs/postgres/sessions/turns/dbes.pyapi/oss/src/dbs/postgres/sessions/turns/mappings.pyapi/oss/src/dbs/postgres/sessions/turns/utils.pyapi/oss/src/dbs/postgres/tracing/dao.pyapi/oss/src/dbs/postgres/tracing/dbas.pyapi/oss/src/dbs/postgres/tracing/dbes.pyapi/oss/src/dbs/postgres/tracing/mappings.pyapi/oss/src/dbs/postgres/tracing/utils.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.pyapi/oss/tests/pytest/acceptance/session_states/test_session_states_basics.pyapi/oss/tests/pytest/acceptance/sessions/__init__.pyapi/oss/tests/pytest/acceptance/sessions/test_archive_unarchive.pyapi/oss/tests/pytest/acceptance/sessions/test_records_ingest_contract.pyapi/oss/tests/pytest/acceptance/sessions/test_stream_header_basics.pyapi/oss/tests/pytest/acceptance/sessions/test_stream_header_roundtrip.pyapi/oss/tests/pytest/unit/mounts/test_agent_id_backfill.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyapi/oss/tests/pytest/unit/otlp/test_logfire_adapter.pyapi/oss/tests/pytest/unit/session_states/__init__.pyapi/oss/tests/pytest/unit/session_states/test_harness_sessions_mapping.pyapi/oss/tests/pytest/unit/session_states/test_session_id_validation.pyapi/oss/tests/pytest/unit/sessions/conftest.pyapi/oss/tests/pytest/unit/sessions/test_command_matrix_inputs_data.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.pyapi/oss/tests/pytest/unit/sessions/test_interactions_transition_update.pyapi/oss/tests/pytest/unit/sessions/test_kill_runner_teardown.pyapi/oss/tests/pytest/unit/sessions/test_mounts_agent_id_backfill.pyapi/oss/tests/pytest/unit/sessions/test_record_ingest_endpoint.pyapi/oss/tests/pytest/unit/sessions/test_records_mapping_upsert.pyapi/oss/tests/pytest/unit/sessions/test_records_turn_span_dao.pyapi/oss/tests/pytest/unit/sessions/test_runner_client_kill.pyapi/oss/tests/pytest/unit/sessions/test_sessions_root_service.pyapi/oss/tests/pytest/unit/sessions/test_stream_header_merge.pyapi/oss/tests/pytest/unit/sessions/test_transition_interaction_resolution.pyapi/oss/tests/pytest/unit/sessions/test_turns_dao.pyapi/oss/tests/pytest/unit/sessions/test_turns_dao_conflict.pyapi/oss/tests/pytest/unit/sessions/test_wp5_dao_fanout.pyapi/oss/tests/pytest/unit/sessions/test_wp5_mount_teardown.pyapi/oss/tests/pytest/unit/sessions/test_wp5_root_router.pyapi/oss/tests/pytest/unit/tracing/test_dao_ingest_identity_columns.pyapi/oss/tests/pytest/unit/tracing/utils/test_trees.pyapi/pyproject.tomlclients/python/agenta_client/__init__.pyclients/python/agenta_client/mounts/client.pyclients/python/agenta_client/mounts/raw_client.pyclients/python/agenta_client/sessions/client.pyclients/python/agenta_client/sessions/raw_client.pyclients/python/agenta_client/types/__init__.pyclients/python/agenta_client/types/harness_kind.pyclients/python/agenta_client/types/harness_session_record.pyclients/python/agenta_client/types/mount.pyclients/python/agenta_client/types/mount_create.pyclients/python/agenta_client/types/mount_query.pyclients/python/agenta_client/types/session_heartbeat_result.pyclients/python/agenta_client/types/session_interaction.pyclients/python/agenta_client/types/session_mount.pyclients/python/agenta_client/types/session_mount_query.pyclients/python/agenta_client/types/session_record.pyclients/python/agenta_client/types/session_response.pyclients/python/agenta_client/types/session_state.pyclients/python/agenta_client/types/session_state_data.pyclients/python/agenta_client/types/session_state_upsert_request.pyclients/python/agenta_client/types/session_stream.pyclients/python/agenta_client/types/session_stream_command_response.pyclients/python/agenta_client/types/session_stream_header_edit.pyclients/python/agenta_client/types/session_stream_response.pyclients/python/agenta_client/types/session_streams_response.pyclients/python/agenta_client/types/session_turn.pyclients/python/agenta_client/types/session_turn_query.pyclients/python/agenta_client/types/session_turn_response.pyclients/python/agenta_client/types/session_turns_response.pyclients/python/agenta_client/types/sessions_response.pyclients/python/agenta_client/types/span_input.pyclients/python/agenta_client/types/span_output.pyclients/python/agenta_client/types/spans_node_input.pyclients/python/agenta_client/types/spans_node_output.pyclients/python/agenta_client/types/workflow_request_data.pydocs/design/agent-workflows/projects/approvals-incident-fixes/README.mddocs/design/agent-workflows/projects/approvals-incident-fixes/context.mddocs/design/agent-workflows/projects/approvals-incident-fixes/plan.mddocs/design/agent-workflows/projects/approvals-incident-fixes/qa.mddocs/design/agent-workflows/projects/approvals-incident-fixes/research.mddocs/design/agent-workflows/projects/approvals-incident-fixes/status.mddocs/design/agent-workflows/projects/concurrent-approvals/PLAN.mddocs/design/agent-workflows/projects/concurrent-approvals/README.mddocs/design/agent-workflows/scratch/debug-concurrent-approvals-db58551b.mddocs/design/agent-workflows/scratch/debug-session-turns-append-500.mddocs/design/agent-workflows/scratch/zed-acp-approvals-comparison.mddocs/docs/self-host/reference/01-configuration.mdxhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/kubernetes/helm/templates/api-deployment.yamlsdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/adapters/local.pysdks/python/agenta/sdk/agents/adapters/sandbox_agent.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/errors.pysdks/python/agenta/sdk/agents/interfaces.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/agenta/sdk/engines/tracing/tracing.pysdks/python/agenta/sdk/middlewares/running/normalizer.pysdks/python/agenta/sdk/models/tracing.pysdks/python/oss/tests/pytest/integration/agents/_fake_runner_backend.pysdks/python/oss/tests/pytest/integration/agents/_qa_transcripts.pysdks/python/oss/tests/pytest/unit/agents/adapters/test_vercel_stream_park.pysdks/python/oss/tests/pytest/unit/agents/conftest.pysdks/python/oss/tests/pytest/unit/agents/connections/test_dtos_model_ref.pysdks/python/oss/tests/pytest/unit/agents/skills/test_skills_e2e.pysdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_capabilities_events.pysdks/python/oss/tests/pytest/unit/agents/test_environment_lifecycle.pysdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_harness_identity.pysdks/python/oss/tests/pytest/unit/agents/test_ui_messages.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/test_batch_fold_stream_contract_routing.pysdks/python/oss/tests/pytest/unit/test_invoke_real_handlers_negotiation_routing.pysdks/python/oss/tests/pytest/unit/test_normalizer_passthrough.pysdks/python/oss/tests/pytest/unit/test_tracing_store_agent.pyservices/oss/tests/pytest/unit/agent/conftest.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/environment.tsservices/runner/src/engines/sandbox_agent/pause.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/engines/sandbox_agent/sandbox-reconnect.tsservices/runner/src/engines/sandbox_agent/session-continuity-durable.tsservices/runner/src/permission-plan.tsservices/runner/src/protocol.tsservices/runner/src/responder.tsservices/runner/src/server.tsservices/runner/src/sessions/alive.tsservices/runner/src/sessions/interactions.tsservices/runner/src/sessions/persist.tsservices/runner/src/tracing/otel.tsservices/runner/src/version.tsservices/runner/tests/acceptance/server-contract.test.tsservices/runner/tests/integration/server-smoke.test.tsservices/runner/tests/unit/client-tools.test.tsservices/runner/tests/unit/pending-approval-pause.test.tsservices/runner/tests/unit/permission-plan.test.tsservices/runner/tests/unit/responder.test.tsservices/runner/tests/unit/sandbox-agent-acp-interactions.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-lifecycle.test.tsservices/runner/tests/unit/sandbox-reconnect.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-alive-interrupt.test.tsservices/runner/tests/unit/session-alive.test.tsservices/runner/tests/unit/session-continuity-durable.test.tsservices/runner/tests/unit/session-interactions.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-keepalive-engine.test.tsservices/runner/tests/unit/session-persist.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.tsweb/oss/src/components/SessionInspector/api.ts
💤 Files with no reviewable changes (17)
- clients/python/agenta_client/types/session_state_upsert_request.py
- clients/python/agenta_client/types/harness_session_record.py
- api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py
- api/oss/src/dbs/postgres/sessions/states/dbes.py
- clients/python/agenta_client/types/session_state.py
- api/oss/src/core/sessions/states/dtos.py
- clients/python/agenta_client/types/session_state_data.py
- api/oss/src/core/sessions/states/service.py
- api/oss/src/core/sessions/states/interfaces.py
- api/oss/tests/pytest/unit/session_states/test_harness_sessions_mapping.py
- api/oss/tests/pytest/unit/session_states/test_session_id_validation.py
- api/oss/src/dbs/postgres/sessions/states/mappings.py
- api/oss/src/dbs/postgres/sessions/states/dao.py
- services/runner/tests/unit/permission-plan.test.ts
- api/oss/databases/postgres/migrations/core/env.py
- api/oss/tests/pytest/acceptance/session_states/test_session_states_basics.py
- services/runner/src/permission-plan.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/design/agent-workflows/projects/concurrent-approvals/README.md
- sdks/python/oss/tests/pytest/unit/agents/test_ui_messages.py
- services/runner/tests/unit/session-keepalive-dispatch.test.ts
- services/runner/src/engines/sandbox_agent/runtime-policy.ts
- services/runner/src/engines/sandbox_agent/environment-setup.ts
- sdks/python/oss/tests/pytest/unit/agents/adapters/test_vercel_stream_park.py
- services/runner/src/engines/sandbox_agent/runtime-contracts.ts
- services/runner/tests/unit/client-tools.test.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🧹 Nitpick comments (7)
api/oss/src/dbs/postgres/sessions/records/dbes.py (1)
30-35: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winUse a partial index to save space for historical data.
Since
turn_idis only forward-filled and will remainNULLfor all historical records, indexing thoseNULLvalues bloats the index unnecessarily. Consider making this a partial index with aturn_id IS NOT NULLcondition, consistent with how nullable identifiers are indexed elsewhere in the codebase (e.g.,MountDBE).
api/oss/src/dbs/postgres/sessions/records/dbes.py#L30-L35: addpostgresql_where=text("turn_id IS NOT NULL")to theIndexdeclaration.api/oss/databases/postgres/migrations/tracing_oss/versions/oss000000004_add_records_turn_span.py#L29-L34: addpostgresql_where=sa.text("turn_id IS NOT NULL")to theop.create_indexcall.api/oss/src/apis/fastapi/sessions/models.py (1)
60-61: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
SessionStreamResponseis missing thecountfield. Every other response envelope in this module (SessionResponse,SessionsResponse,SessionStreamsResponse,SessionTurnResponse) carriescountalongside its payload; this singular response is the only one that doesn't.As per coding guidelines: "include
countplus payload in response envelopes."Proposed change
class SessionStreamResponse(BaseModel): + count: int = 0 stream: Optional[SessionStream] = NoneSource: Coding guidelines
api/oss/src/apis/fastapi/otlp/extractors/span_data_builders.py (1)
150-152: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: consolidate the repeated
ag.{ns}.{k}update calls.The
session/user/agentattribute updates are now three near-identical lines; a small loop would reduce duplication if more identity namespaces are added later.♻️ Optional consolidation
- attributes.update(**{f"ag.session.{k}": v for k, v in features.session.items()}) - attributes.update(**{f"ag.user.{k}": v for k, v in features.user.items()}) - attributes.update(**{f"ag.agent.{k}": v for k, v in features.agent.items()}) + for _ns, _values in ( + ("session", features.session), + ("user", features.user), + ("agent", features.agent), + ): + attributes.update(**{f"ag.{_ns}.{k}": v for k, v in _values.items()})api/oss/tests/pytest/unit/sessions/test_command_matrix_inputs_data.py (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
_FakeStreamsDAO/lock_enginetest doubles, already diverging. Three new test files copy-paste the same fake streams DAO,lock_enginefixture (patchingLockEngine._client), and_service()helper; the copy intest_kill_runner_teardown.pyhas already drifted by droppingturn_idfromupdate().
api/oss/tests/pytest/unit/sessions/test_command_matrix_inputs_data.py#L41-89: move_FakeStreamsDAO,lock_engine, and_service()into a sharedconftest.py/test-utils module forunit/sessions/, and import from there.api/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.py#L40-88: same — import the shared fixture instead of redefining it.api/oss/tests/pytest/unit/sessions/test_kill_runner_teardown.py#L30-71: same, and restoreturn_idpreservation inupdate()to match the other copies (or confirm it's intentionally irrelevant to this file's assertions) once consolidated.services/runner/src/server.ts (1)
494-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPark/resume logs still surface only one tool name for a multi-gate turn.
env.parkedApproval?.toolName(singular) is used for thepark-approvalandresumelog lines even though a turn can now park several gates (env.parkedApprovals). For a multi-gate turn this always logs only the first gate's tool, which weakens debuggability of exactly the new concurrent-approval scenario this PR introduces.Consider logging all parked tool names (or at least a count) alongside the existing
gates=/approve=/reject=counters.♻️ Example: surface all parked tool names
- klog( - `park-approval key=${key} tool=${env.parkedApproval?.toolName ?? "?"}`, - ); + klog( + `park-approval key=${key} tools=${[...env.parkedApprovals.values()] + .map((g) => g.toolName ?? "?") + .join(",")}`, + );Also applies to: 529-532, 687-688, 744-751
services/runner/tests/unit/server.test.ts (1)
46-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
listen()test-server helper into a sharedtests/utils/module. All three files define and maintain, in lockstep, the samelisten(run, token)bootstrap helper (same signature, same "force the configured token unconditionally" rationale) — this PR had to edit all three together to keep the token semantics synchronized, which is direct evidence of the duplication cost. As per path instructions, shared test helpers should live intests/utils/where applicable.
services/runner/tests/unit/server.test.ts#L46-L52: replace this locallisten()with an import from a new shared helper (e.g.tests/utils/server-listen.ts).services/runner/tests/acceptance/server-contract.test.ts#L44-L49: replace this locallisten()with the same shared helper.services/runner/tests/integration/server-smoke.test.ts#L50-L55: replace this locallisten()with the same shared helper.Source: Path instructions
services/runner/tests/unit/session-alive-interrupt.test.ts (1)
86-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test doesn't actually exercise the guard it claims to —
release()never callshandleBeat.
release()callssendHeartbeatdirectly (not throughhandleBeat), so it can never triggeronInterruptedregardless of theinterruptedFiredguard. The assertion at Line 102 therefore passes trivially and doesn't validate de-dup across two real interruption-reporting beats. The genuinely untested path is two INTERVAL beats (both routed throughhandleBeat) each reportinginterrupted: true—session-alive.test.ts's fake-timer pattern (vi.useFakeTimers()+vi.advanceTimersByTimeAsync) could drive that directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e9e97440-7a24-48dd-aac4-ec583afd5637
⛔ Files ignored due to path filters (8)
api/uv.lockis excluded by!**/*.lockweb/packages/agenta-api-client/src/generated/api/resources/mounts/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/mounts/client/requests/MountQueryRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/Client.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/ArchiveSessionRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/DeleteSessionRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/FetchTurnRequest.tsis excluded by!**/generated/**web/packages/agenta-api-client/src/generated/api/resources/sessions/client/requests/SessionDetachRequest.tsis excluded by!**/generated/**
📒 Files selected for processing (227)
api/entrypoints/routers.pyapi/oss/databases/postgres/migrations/core/env.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000014_add_session_turns.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000015_add_session_streams_header.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000016_add_mounts_agent_id.pyapi/oss/databases/postgres/migrations/core_oss/versions/oss000000017_drop_session_states.pyapi/oss/databases/postgres/migrations/tracing_oss/versions/oss000000003_add_span_identity_columns.pyapi/oss/databases/postgres/migrations/tracing_oss/versions/oss000000004_add_records_turn_span.pyapi/oss/src/apis/fastapi/mounts/router.pyapi/oss/src/apis/fastapi/mounts/utils.pyapi/oss/src/apis/fastapi/otlp/extractors/adapters/logfire_adapter.pyapi/oss/src/apis/fastapi/otlp/extractors/canonical_attributes.pyapi/oss/src/apis/fastapi/otlp/extractors/span_data_builders.pyapi/oss/src/apis/fastapi/otlp/utils/serialization.pyapi/oss/src/apis/fastapi/sessions/models.pyapi/oss/src/apis/fastapi/sessions/router.pyapi/oss/src/apis/fastapi/tracing/router.pyapi/oss/src/core/mounts/dtos.pyapi/oss/src/core/mounts/interfaces.pyapi/oss/src/core/mounts/service.pyapi/oss/src/core/sessions/dtos.pyapi/oss/src/core/sessions/interactions/dtos.pyapi/oss/src/core/sessions/interactions/interfaces.pyapi/oss/src/core/sessions/interactions/service.pyapi/oss/src/core/sessions/records/dtos.pyapi/oss/src/core/sessions/service.pyapi/oss/src/core/sessions/states/dtos.pyapi/oss/src/core/sessions/states/interfaces.pyapi/oss/src/core/sessions/states/service.pyapi/oss/src/core/sessions/streams/dtos.pyapi/oss/src/core/sessions/streams/interfaces.pyapi/oss/src/core/sessions/streams/runner_client.pyapi/oss/src/core/sessions/streams/service.pyapi/oss/src/core/sessions/turns/__init__.pyapi/oss/src/core/sessions/turns/dtos.pyapi/oss/src/core/sessions/turns/interfaces.pyapi/oss/src/core/sessions/turns/service.pyapi/oss/src/core/sessions/turns/types.pyapi/oss/src/core/shared/dtos.pyapi/oss/src/core/tracing/dtos.pyapi/oss/src/core/tracing/service.pyapi/oss/src/core/tracing/utils/filtering.pyapi/oss/src/core/tracing/utils/trees.pyapi/oss/src/dbs/postgres/mounts/dao.pyapi/oss/src/dbs/postgres/mounts/dbas.pyapi/oss/src/dbs/postgres/mounts/dbes.pyapi/oss/src/dbs/postgres/mounts/mappings.pyapi/oss/src/dbs/postgres/sessions/interactions/dao.pyapi/oss/src/dbs/postgres/sessions/records/dao.pyapi/oss/src/dbs/postgres/sessions/records/dbas.pyapi/oss/src/dbs/postgres/sessions/records/dbes.pyapi/oss/src/dbs/postgres/sessions/records/mappings.pyapi/oss/src/dbs/postgres/sessions/states/dao.pyapi/oss/src/dbs/postgres/sessions/states/dbes.pyapi/oss/src/dbs/postgres/sessions/states/mappings.pyapi/oss/src/dbs/postgres/sessions/streams/dao.pyapi/oss/src/dbs/postgres/sessions/streams/dbes.pyapi/oss/src/dbs/postgres/sessions/streams/mappings.pyapi/oss/src/dbs/postgres/sessions/turns/__init__.pyapi/oss/src/dbs/postgres/sessions/turns/dao.pyapi/oss/src/dbs/postgres/sessions/turns/dbas.pyapi/oss/src/dbs/postgres/sessions/turns/dbes.pyapi/oss/src/dbs/postgres/sessions/turns/mappings.pyapi/oss/src/dbs/postgres/sessions/turns/utils.pyapi/oss/src/dbs/postgres/tracing/dao.pyapi/oss/src/dbs/postgres/tracing/dbas.pyapi/oss/src/dbs/postgres/tracing/dbes.pyapi/oss/src/dbs/postgres/tracing/mappings.pyapi/oss/src/dbs/postgres/tracing/utils.pyapi/oss/src/utils/env.pyapi/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.pyapi/oss/tests/pytest/acceptance/session_states/test_session_states_basics.pyapi/oss/tests/pytest/acceptance/sessions/__init__.pyapi/oss/tests/pytest/acceptance/sessions/test_archive_unarchive.pyapi/oss/tests/pytest/acceptance/sessions/test_records_ingest_contract.pyapi/oss/tests/pytest/acceptance/sessions/test_stream_header_basics.pyapi/oss/tests/pytest/acceptance/sessions/test_stream_header_roundtrip.pyapi/oss/tests/pytest/unit/mounts/test_agent_id_backfill.pyapi/oss/tests/pytest/unit/mounts/test_agent_mounts.pyapi/oss/tests/pytest/unit/otlp/test_logfire_adapter.pyapi/oss/tests/pytest/unit/session_states/__init__.pyapi/oss/tests/pytest/unit/session_states/test_harness_sessions_mapping.pyapi/oss/tests/pytest/unit/session_states/test_session_id_validation.pyapi/oss/tests/pytest/unit/sessions/conftest.pyapi/oss/tests/pytest/unit/sessions/test_command_matrix_inputs_data.pyapi/oss/tests/pytest/unit/sessions/test_heartbeat_is_current_turn.pyapi/oss/tests/pytest/unit/sessions/test_interactions_transition_update.pyapi/oss/tests/pytest/unit/sessions/test_kill_runner_teardown.pyapi/oss/tests/pytest/unit/sessions/test_mounts_agent_id_backfill.pyapi/oss/tests/pytest/unit/sessions/test_record_ingest_endpoint.pyapi/oss/tests/pytest/unit/sessions/test_records_mapping_upsert.pyapi/oss/tests/pytest/unit/sessions/test_records_turn_span_dao.pyapi/oss/tests/pytest/unit/sessions/test_runner_client_kill.pyapi/oss/tests/pytest/unit/sessions/test_sessions_root_service.pyapi/oss/tests/pytest/unit/sessions/test_stream_header_merge.pyapi/oss/tests/pytest/unit/sessions/test_transition_interaction_resolution.pyapi/oss/tests/pytest/unit/sessions/test_turns_dao.pyapi/oss/tests/pytest/unit/sessions/test_turns_dao_conflict.pyapi/oss/tests/pytest/unit/sessions/test_wp5_dao_fanout.pyapi/oss/tests/pytest/unit/sessions/test_wp5_mount_teardown.pyapi/oss/tests/pytest/unit/sessions/test_wp5_root_router.pyapi/oss/tests/pytest/unit/tracing/test_dao_ingest_identity_columns.pyapi/oss/tests/pytest/unit/tracing/utils/test_trees.pyapi/pyproject.tomlclients/python/agenta_client/__init__.pyclients/python/agenta_client/mounts/client.pyclients/python/agenta_client/mounts/raw_client.pyclients/python/agenta_client/sessions/client.pyclients/python/agenta_client/sessions/raw_client.pyclients/python/agenta_client/types/__init__.pyclients/python/agenta_client/types/harness_kind.pyclients/python/agenta_client/types/harness_session_record.pyclients/python/agenta_client/types/mount.pyclients/python/agenta_client/types/mount_create.pyclients/python/agenta_client/types/mount_query.pyclients/python/agenta_client/types/session_heartbeat_result.pyclients/python/agenta_client/types/session_interaction.pyclients/python/agenta_client/types/session_mount.pyclients/python/agenta_client/types/session_mount_query.pyclients/python/agenta_client/types/session_record.pyclients/python/agenta_client/types/session_response.pyclients/python/agenta_client/types/session_state.pyclients/python/agenta_client/types/session_state_data.pyclients/python/agenta_client/types/session_state_upsert_request.pyclients/python/agenta_client/types/session_stream.pyclients/python/agenta_client/types/session_stream_command_response.pyclients/python/agenta_client/types/session_stream_header_edit.pyclients/python/agenta_client/types/session_stream_response.pyclients/python/agenta_client/types/session_streams_response.pyclients/python/agenta_client/types/session_turn.pyclients/python/agenta_client/types/session_turn_query.pyclients/python/agenta_client/types/session_turn_response.pyclients/python/agenta_client/types/session_turns_response.pyclients/python/agenta_client/types/sessions_response.pyclients/python/agenta_client/types/span_input.pyclients/python/agenta_client/types/span_output.pyclients/python/agenta_client/types/spans_node_input.pyclients/python/agenta_client/types/spans_node_output.pyclients/python/agenta_client/types/workflow_request_data.pydocs/design/agent-workflows/projects/approvals-incident-fixes/README.mddocs/design/agent-workflows/projects/approvals-incident-fixes/context.mddocs/design/agent-workflows/projects/approvals-incident-fixes/plan.mddocs/design/agent-workflows/projects/approvals-incident-fixes/qa.mddocs/design/agent-workflows/projects/approvals-incident-fixes/research.mddocs/design/agent-workflows/projects/approvals-incident-fixes/status.mddocs/design/agent-workflows/projects/concurrent-approvals/PLAN.mddocs/design/agent-workflows/projects/concurrent-approvals/README.mddocs/design/agent-workflows/scratch/debug-concurrent-approvals-db58551b.mddocs/design/agent-workflows/scratch/debug-session-turns-append-500.mddocs/design/agent-workflows/scratch/zed-acp-approvals-comparison.mddocs/docs/self-host/reference/01-configuration.mdxhosting/docker-compose/ee/docker-compose.dev.ymlhosting/docker-compose/ee/docker-compose.gh.ymlhosting/docker-compose/oss/docker-compose.dev.ymlhosting/docker-compose/oss/docker-compose.gh.ymlhosting/kubernetes/helm/templates/api-deployment.yamlsdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/adapters/harnesses.pysdks/python/agenta/sdk/agents/adapters/local.pysdks/python/agenta/sdk/agents/adapters/sandbox_agent.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/agenta/sdk/agents/errors.pysdks/python/agenta/sdk/agents/interfaces.pysdks/python/agenta/sdk/agents/utils/wire.pysdks/python/agenta/sdk/agents/wire_models.pysdks/python/agenta/sdk/engines/tracing/tracing.pysdks/python/agenta/sdk/middlewares/running/normalizer.pysdks/python/agenta/sdk/models/tracing.pysdks/python/oss/tests/pytest/integration/agents/_fake_runner_backend.pysdks/python/oss/tests/pytest/integration/agents/_qa_transcripts.pysdks/python/oss/tests/pytest/unit/agents/adapters/test_vercel_stream_park.pysdks/python/oss/tests/pytest/unit/agents/conftest.pysdks/python/oss/tests/pytest/unit/agents/connections/test_dtos_model_ref.pysdks/python/oss/tests/pytest/unit/agents/skills/test_skills_e2e.pysdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_capabilities_events.pysdks/python/oss/tests/pytest/unit/agents/test_environment_lifecycle.pysdks/python/oss/tests/pytest/unit/agents/test_harness_adapters.pysdks/python/oss/tests/pytest/unit/agents/test_harness_identity.pysdks/python/oss/tests/pytest/unit/agents/test_ui_messages.pysdks/python/oss/tests/pytest/unit/agents/test_wire_contract.pysdks/python/oss/tests/pytest/unit/test_batch_fold_stream_contract_routing.pysdks/python/oss/tests/pytest/unit/test_invoke_real_handlers_negotiation_routing.pysdks/python/oss/tests/pytest/unit/test_normalizer_passthrough.pysdks/python/oss/tests/pytest/unit/test_tracing_store_agent.pyservices/oss/tests/pytest/unit/agent/conftest.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/environment.tsservices/runner/src/engines/sandbox_agent/pause.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/engines/sandbox_agent/sandbox-reconnect.tsservices/runner/src/engines/sandbox_agent/session-continuity-durable.tsservices/runner/src/permission-plan.tsservices/runner/src/protocol.tsservices/runner/src/responder.tsservices/runner/src/server.tsservices/runner/src/sessions/alive.tsservices/runner/src/sessions/interactions.tsservices/runner/src/sessions/persist.tsservices/runner/src/tracing/otel.tsservices/runner/src/version.tsservices/runner/tests/acceptance/server-contract.test.tsservices/runner/tests/integration/server-smoke.test.tsservices/runner/tests/unit/client-tools.test.tsservices/runner/tests/unit/pending-approval-pause.test.tsservices/runner/tests/unit/permission-plan.test.tsservices/runner/tests/unit/responder.test.tsservices/runner/tests/unit/sandbox-agent-acp-interactions.test.tsservices/runner/tests/unit/sandbox-agent-orchestration.test.tsservices/runner/tests/unit/sandbox-lifecycle.test.tsservices/runner/tests/unit/sandbox-reconnect.test.tsservices/runner/tests/unit/server.test.tsservices/runner/tests/unit/session-alive-interrupt.test.tsservices/runner/tests/unit/session-alive.test.tsservices/runner/tests/unit/session-continuity-durable.test.tsservices/runner/tests/unit/session-interactions.test.tsservices/runner/tests/unit/session-keepalive-approval.test.tsservices/runner/tests/unit/session-keepalive-dispatch.test.tsservices/runner/tests/unit/session-keepalive-engine.test.tsservices/runner/tests/unit/session-persist.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.tsweb/oss/src/components/AgentChatSlice/assets/transcriptToMessages.tsweb/oss/src/components/SessionInspector/api.ts
💤 Files with no reviewable changes (17)
- clients/python/agenta_client/types/session_state_upsert_request.py
- clients/python/agenta_client/types/harness_session_record.py
- api/oss/tests/pytest/acceptance/session_states/test_harness_sessions_roundtrip.py
- api/oss/src/dbs/postgres/sessions/states/dbes.py
- clients/python/agenta_client/types/session_state.py
- api/oss/src/core/sessions/states/dtos.py
- clients/python/agenta_client/types/session_state_data.py
- api/oss/src/core/sessions/states/service.py
- api/oss/src/core/sessions/states/interfaces.py
- api/oss/tests/pytest/unit/session_states/test_harness_sessions_mapping.py
- api/oss/tests/pytest/unit/session_states/test_session_id_validation.py
- api/oss/src/dbs/postgres/sessions/states/mappings.py
- api/oss/src/dbs/postgres/sessions/states/dao.py
- services/runner/tests/unit/permission-plan.test.ts
- api/oss/databases/postgres/migrations/core/env.py
- api/oss/tests/pytest/acceptance/session_states/test_session_states_basics.py
- services/runner/src/permission-plan.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- docs/design/agent-workflows/projects/concurrent-approvals/README.md
- sdks/python/oss/tests/pytest/unit/agents/test_ui_messages.py
- services/runner/tests/unit/session-keepalive-dispatch.test.ts
- services/runner/src/engines/sandbox_agent/runtime-policy.ts
- services/runner/src/engines/sandbox_agent/environment-setup.ts
- sdks/python/oss/tests/pytest/unit/agents/adapters/test_vercel_stream_park.py
- services/runner/src/engines/sandbox_agent/runtime-contracts.ts
- services/runner/tests/unit/client-tools.test.ts
🛑 Comments failed to post (12)
api/oss/src/apis/fastapi/sessions/router.py (1)
236-242: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
FastAPI add_api_route multiple methods single operation_id duplicate operationId OpenAPI💡 Result:
When you use add_api_route with multiple HTTP methods in FastAPI (e.g., methods=["POST", "DELETE"]), the framework historically generated the same operationId for all methods because the underlying unique_id was computed using only the first method from the set [1][2][3]. This resulted in duplicate operationId values in the OpenAPI schema and triggered UserWarning: Duplicate Operation ID [1][4][5]. Recent updates to FastAPI (notably integrated around June 2026) have addressed this by ensuring that for multi-method routes, a per-method operationId is derived [2][5][6]. The OpenAPI generator now creates a unique ID for each operation by incorporating the specific HTTP method into the ID string (e.g., clear_clear_post, clear_clear_delete) rather than reusing the same route-level ID [1][5]. If you are encountering this issue in your current environment: 1. Update FastAPI: Ensure your FastAPI version is up to date, as this behavior was specifically targeted for resolution in recent releases [2][5]. 2. Use Explicit operation_id: You can still manually override the operationId for any route by providing the operation_id parameter directly to add_api_route [1][7][8]. This remains the most reliable way to maintain full control over the OpenAPI schema: app.add_api_route("/items", my_handler, methods=["POST", "DELETE"], operation_id="my_custom_id") 3. Alternative Pattern: While multi-method support via add_api_route is available, an alternative, idiomatic approach in FastAPI is to use separate decorators for each HTTP method, which automatically assigns distinct operation IDs for each [9]:
@app.post("/items")@app.delete("/items") async def handle_items: return {"message": "success"} If you are implementing a custom generate_unique_id_function, ensure that your logic handles multiple methods correctly, as the framework's internal OpenAPI generator now expects to be able to distinguish between them [6][7][8].Citations:
- 1: fastapi/fastapi#15321
- 2: fastapi/fastapi#15698
- 3: fastapi/fastapi#15215
- 4: fastapi/fastapi#15194
- 5: fastapi/fastapi@707fd45
- 6: fastapi/fastapi#15222
- 7: https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/?h=operation_id
- 8: https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/
- 9: fastapi/fastapi#10694
🏁 Script executed:
python3 - <<'PY' from pathlib import Path import re # Find likely dependency files candidates = [ "api/oss/pyproject.toml", "api/oss/requirements.txt", "api/oss/poetry.lock", "api/oss/pdm.lock", "api/oss/uv.lock", ] for p in candidates: path = Path(p) if path.exists(): print(f"== {p} ==") text = path.read_text(errors="ignore") for pat in [r'fastapi\s*([=><!~]+)\s*["\']?([^"\',\s]+)', r'fastapi[^\n]*']: m = re.search(pat, text, re.I) if m: print(m.group(0)) break else: print("(no fastapi line found quickly)") PYRepository: Agenta-AI/agenta
Length of output: 154
🏁 Script executed:
rg -n "fastapi" api/oss/{pyproject.toml,requirements.txt,poetry.lock,pdm.lock,uv.lock} api/oss -g '!**/.venv/**'Repository: Agenta-AI/agenta
Length of output: 34062
🏁 Script executed:
python3 - <<'PY' from pathlib import Path for p in [ "api/oss/src/main.py", "api/oss/src/app.py", "api/oss/src/__init__.py", ]: path = Path(p) if path.exists(): print(f"== {p} ==") for i, line in enumerate(path.read_text(errors="ignore").splitlines(), 1): if "generate_unique_id" in line or "FastAPI(" in line or "openapi" in line: print(f"{i}: {line}") PYRepository: Agenta-AI/agenta
Length of output: 184
🌐 Web query:
FastAPI multi-method add_api_route explicit operation_id duplicate operationId current behavior💡 Result:
In FastAPI, the behavior of
operationIdgeneration for routes with multiple HTTP methods has been a known limitation, often resulting in duplicateoperationIdwarnings because the default generation logic historically used the sameunique_id(derived from the first method in the set) for all methods [1][2][3]. Explicitly setting anoperation_idremains the recommended way to handle this issue, as FastAPI respects user-provided identifiers and does not override them [1][2][4]. Regarding current behavior and recent updates: 1. Explicitoperation_id: If you provide anoperation_idparameter toadd_api_routeor path decorators, FastAPI uses it as-is for that operation. If you provide the sameoperation_idfor multiple methods, you will trigger a duplicate operation ID warning, as each operation in an OpenAPI schema must be unique [5][4][6]. 2. Default behavior (without explicitoperation_id): Recent improvements in the FastAPI codebase (addressing long-standing issues like#4740) have introduced logic to automatically generate uniqueoperationIdvalues for multi-method routes by appending the HTTP method to the base ID [1][2][7]. This replaces the previous behavior where all methods on a single route incorrectly shared the exact sameoperationId[2][3]. 3. Custom Generators: You can maintain full control overoperationIdgeneration by using thegenerate_unique_id_functionparameter in theFastAPIconstructor. This function receives theAPIRouteobject and allows you to define a naming scheme that avoids conflicts entirely [5][6]. For stable and predictable OpenAPI schemas, it is best practice to either rely on these recent automated generation improvements or define a customgenerate_unique_id_functionif your application requires a specific naming convention [5][7].Citations:
- 1: fastapi/fastapi#15321
- 2: fastapi/fastapi#15698
- 3: fastapi/fastapi#15215
- 4: fastapi/fastapi#15222
- 5: https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/
- 6: https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/?h=operation_id
- 7: fastapi/fastapi@707fd45
🌐 Web query:
FastAPI explicit operation_id multi-method route same operationId both methods💡 Result:
In FastAPI, you must ensure that every operationId is unique across your entire API [1][2]. When a single route is registered with multiple HTTP methods (e.g., using
methods=["GET", "POST"]), you face specific challenges regarding automatic operationId generation [3][4][5]. Explicit Operation IDs If you manually assign an operation_id (e.g.,@app.get("/path", operation_id="my_id")), FastAPI respects this value [3][6]. However, if you apply the same explicit operation_id to multiple methods on the same route, you will violate the OpenAPI specification, which requires each operationId to be unique [4][7]. You must provide distinct IDs for each method if you choose to set them manually [1][2]. Automatic Generation and Multi-Method Routes In recent versions of FastAPI, improvements have been made to handle automatic operationId generation for multi-method routes to prevent duplicate IDs [3][4][7]. - Previously, FastAPI generated a single unique_id for the route (often based on the first method found), causing all methods on that route to share that same ID [3][4]. - Newer updates to FastAPI ensure that for multi-method routes, the generated operationIds are automatically suffixed (e.g., _get, _post) to maintain uniqueness while remaining deterministic [3][4][6]. If you are using an older version of FastAPI that produces duplicate operationIds for multi-method routes, you have a few options: 1. Update FastAPI: Ensure you are using a recent version that includes the fixes for multi-method route operationId generation [4][7]. 2. Custom ID Generation: You can provide a custom generate_unique_id_function to the FastAPI app, which gives you full control over how operationIds are derived from the route object [1][8]. 3. Manual Override: You can programmatically iterate over app.routes after they have been registered and assign unique operation_ids to each route (and method) if necessary [9][10]. For consistent and maintainable API schemas, it is generally recommended to rely on the default unique ID generation or a custom generate_unique_id_function that ensures uniqueness programmatically, rather than manually setting IDs for every endpoint [1][8].Citations:
- 1: https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/
- 2: https://fastapi.tiangolo.com/reference/apirouter/
- 3: fastapi/fastapi#15321
- 4: fastapi/fastapi#15698
- 5: fastapi/fastapi#15194
- 6: fastapi/fastapi#15222
- 7: fastapi/fastapi@707fd45
- 8: https://fastapi.tiangolo.com/advanced/path-operation-advanced-configuration/?h=operation_id
- 9: https://github.com/FastAPI/FastAPI/blob/5ba94612/docs/en/docs/advanced/path-operation-advanced-configuration.md
- 10: https://github.com/tiangolo/fastapi/blob/master/docs_src/path_operation_advanced_configuration/tutorial002_py310.py
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== potential dependency files ==\n' git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|poetry\.lock|pdm\.lock|uv\.lock|lock\.txt)$' || true printf '\n== fastapi pin/search ==\n' git ls-files | while read -r f; do case "$f" in *pyproject.toml|*requirements*.txt|*poetry.lock|*pdm.lock|*uv.lock|*lock.txt) if rg -n "fastapi" "$f" >/dev/null 2>&1; then echo "-- $f" rg -n "fastapi" "$f" fi ;; esac done printf '\n== unique id generator search ==\n' rg -n "generate_unique_id_function|unique_id|operation_id=" api/oss/src api/oss/tests -g '!**/.venv/**' || trueRepository: Agenta-AI/agenta
Length of output: 50372
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== sessions/router.py around the header route ==\n' sed -n '220,255p' api/oss/src/apis/fastapi/sessions/router.py | cat -n printf '\n== Sessions router class / route registration ==\n' rg -n "class .*Router|add_api_route|set_session_stream_header" api/oss/src/apis/fastapi/sessions/router.pyRepository: Agenta-AI/agenta
Length of output: 3092
🏁 Script executed:
#!/bin/bash set -euo pipefail printf '== app construction search ==\n' rg -n "FastAPI\(|generate_unique_id_function|openapi_url|docs_url" api/oss/src -g '!**/.venv/**' || true printf '\n== top-level repo files related to python deps ==\n' git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|poetry\.lock|pdm\.lock|uv\.lock)$' || trueRepository: Agenta-AI/agenta
Length of output: 1753
Split the header route by verb
PUTandPOSTreuseoperation_id="set_session_stream_header", so FastAPI will generate duplicate OpenAPI operationIds for this route. Split them into separate route registrations with distinct IDs, or keep only the verb you need.api/oss/src/core/tracing/utils/trees.py (1)
168-170: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Safely cast extracted identifier attributes to string.
Telemetry attributes are arbitrary untyped JSON and may contain non-string types (e.g., integers, lists, or dicts) for the
idfield. Directly assigning a non-string value tospan_dtocould bypass Pydantic validation and result in serialization or database crashes downstream.🛡️ Proposed fix to safeguard the assignment
- span_dto.session_id = session.get("id") if isinstance(session, dict) else None - span_dto.user_id = user.get("id") if isinstance(user, dict) else None - span_dto.agent_id = agent.get("id") if isinstance(agent, dict) else None + session_id = session.get("id") if isinstance(session, dict) else None + span_dto.session_id = str(session_id) if isinstance(session_id, (str, int)) else None + + user_id = user.get("id") if isinstance(user, dict) else None + span_dto.user_id = str(user_id) if isinstance(user_id, (str, int)) else None + + agent_id = agent.get("id") if isinstance(agent, dict) else None + span_dto.agent_id = str(agent_id) if isinstance(agent_id, (str, int)) else None📝 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.session_id = session.get("id") if isinstance(session, dict) else None span_dto.session_id = str(session_id) if isinstance(session_id, (str, int)) else None user_id = user.get("id") if isinstance(user, dict) else None span_dto.user_id = str(user_id) if isinstance(user_id, (str, int)) else None agent_id = agent.get("id") if isinstance(agent, dict) else None span_dto.agent_id = str(agent_id) if isinstance(agent_id, (str, int)) else Noneapi/oss/src/dbs/postgres/mounts/dao.py (1)
229-258: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Prevent TOCTOU race condition and blob storage leaks with
DELETE ... RETURNING.The current implementation performs a
SELECTfollowed by aDELETE. If a mount is inserted concurrently for thissession_idbetween these two statements, theDELETEwill remove it from the database, but it won't be included in the returnedmountslist. Consequently, the caller will not tear down its object-store prefix, causing a resource leak.PostgreSQL and SQLAlchemy support
DELETE ... RETURNING. You can execute the delete and fetch the deleted rows atomically in a single statement.🐛 Proposed fix
- async with self.engine.session() as session: - stmt = select(MountDBE).where( - MountDBE.project_id == project_id, - MountDBE.session_id == session_id, - ) - result = await session.execute(stmt) - mount_dbes = list(result.scalars().all()) - mounts = [map_mount_dbe_to_dto(mount_dbe=dbe) for dbe in mount_dbes] - - if mount_dbes: - del_stmt = sa_delete(MountDBE).where( - MountDBE.project_id == project_id, - MountDBE.session_id == session_id, - ) - await session.execute(del_stmt) - await session.commit() + async with self.engine.session() as session: + del_stmt = sa_delete(MountDBE).where( + MountDBE.project_id == project_id, + MountDBE.session_id == session_id, + ).returning(MountDBE) + result = await session.execute(del_stmt) + mount_dbes = list(result.scalars().all()) + mounts = [map_mount_dbe_to_dto(mount_dbe=dbe) for dbe in mount_dbes] + + if mount_dbes: + await session.commit()📝 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.async def delete_by_session_id( self, *, project_id: UUID, session_id: str, ) -> List[Mount]: """Hard delete the mount rows bound to a session. Mounts are semi- independent (optional `session_id`, may outlive a session) — this is the explicit, session-scoped fan-out (S7/F1, WP5), not a blind cascade. Returns the deleted rows so the caller can tear down their object-store prefixes.""" async with self.engine.session() as session: del_stmt = sa_delete(MountDBE).where( MountDBE.project_id == project_id, MountDBE.session_id == session_id, ).returning(MountDBE) result = await session.execute(del_stmt) mount_dbes = list(result.scalars().all()) mounts = [map_mount_dbe_to_dto(mount_dbe=dbe) for dbe in mount_dbes] if mount_dbes: await session.commit() return mountsapi/oss/tests/pytest/unit/mounts/test_agent_id_backfill.py (1)
1-11: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring cites the wrong migration file.
The docstring references
oss000000014_add_mounts_agent_id, but per the migration file listing,oss000000014isadd_session_turns.py— the actualmounts.agent_idmigration isoss000000016_add_mounts_agent_id.py(correctly cited in the sibling integration testtest_mounts_agent_id_backfill.py).📝 Proposed fix
-"""Backfill parser for the `agent_id` migration (oss000000014_add_mounts_agent_id). +"""Backfill parser for the `agent_id` migration (oss000000016_add_mounts_agent_id).📝 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."""Backfill parser for the `agent_id` migration (oss000000016_add_mounts_agent_id). The migration's backfill is inline SQL (`split_part(substr(slug, N), '__', 1)`), matching the sibling core_oss data migrations (oss000000010, oss000000011), which are also plain SQL with no dedicated unit test. This file pins the parsing invariant in Python so any drift in the slug format (verified against `mint_agent_slug`, `core/mounts/service.py`) is caught without a live DB: the SQL and `mint_agent_slug`/`mint_agent_id` must derive the identical id from the identical slug. The SQL itself was validated against a real Postgres instance (ephemeral, local) during implementation. """api/oss/tests/pytest/unit/sessions/test_records_turn_span_dao.py (1)
1-14: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Docstring migration reference is off by one.
The docstring says the chain applies "through oss000000003_add_records_turn_span", but per the migration list,
oss000000003isadd_span_identity_columnsandoss000000004is the actualadd_records_turn_spanmigration this test depends on.📝 Proposed fix
-Requires the tracing_oss migration chain applied (through oss000000003_add_records_turn_span) +Requires the tracing_oss migration chain applied (through oss000000004_add_records_turn_span)📝 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."""Integration-style tests for records turn_id/span_id tagging against a real Postgres. Requires the tracing_oss migration chain applied (through oss000000004_add_records_turn_span) and POSTGRES_URI_TRACING pointed at that database. Records have no FK to any other table (cross-DB, plain columns), so no fixture chain is needed beyond project_id/session_id. Verifies: - a new record carries turn_id (and span_id when supplied); - records group by turn_id (client-side grouping over get_records, since there is no server-side aggregate endpoint); - span_id is populated when present on the event, null otherwise; - old records (turn_id/span_id both null, as if written before this column existed) are still readable under the session, alongside newer tagged ones. """api/oss/tests/pytest/unit/sessions/test_wp5_root_router.py (1)
140-153: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Unpatched
check_action_accesscall in an otherwise-mocked unit test.Every other test in this file wraps the router call in
_patched_access(...); this one does not, so its pass/fail implicitly depends on_validate_session_idrunning before the permission check inside the router. Wrap this in_patched_access(True)(orFalse) too, so the test isolates input validation from the real RBAC/DB call and stays correct if that ordering ever changes.🔧 Proposed fix
async def test_delete_session_rejects_invalid_session_id(): sessions_service = AsyncMock() router = SessionsRootRouter(sessions_service=sessions_service) project_id = uuid4() user_id = uuid4() app = FastAPI() request = _make_authed_request(app, project_id, user_id, method="DELETE") - with pytest.raises(HTTPException) as exc_info: - await router.delete_session(request=request, session_id="../etc/passwd") + with _patched_access(True): + with pytest.raises(HTTPException) as exc_info: + await router.delete_session(request=request, session_id="../etc/passwd")📝 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.async def test_delete_session_rejects_invalid_session_id(): sessions_service = AsyncMock() router = SessionsRootRouter(sessions_service=sessions_service) project_id = uuid4() user_id = uuid4() app = FastAPI() request = _make_authed_request(app, project_id, user_id, method="DELETE") with _patched_access(True): with pytest.raises(HTTPException) as exc_info: await router.delete_session(request=request, session_id="../etc/passwd") assert exc_info.value.status_code == 400 sessions_service.delete_session.assert_not_awaited()clients/python/agenta_client/__init__.py (1)
23-23: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix invalid
typing.Anyobjects in generated module exports.The generated
_dynamic_importsdictionaries and__all__lists contain unquotedtyping.Anyobjects instead of the correct string literals (e.g.,"FullJsonInput","FullJsonOutput","LabelJsonInput","LabelJsonOutput"). This is likely a bug in the client SDK generator (Fern) when evaluating and handling type aliases.In
_dynamic_imports, this evaluates to the exact same dictionary key, silently overwriting previous entries and breaking lazy loading attribute access. In__all__, wildcard imports (from module import *) will crash the runtime withTypeError: item in __all__ must be str.Please update the generator configuration or run a post-processing script to replace these unquoted type variables with their respective string identifiers.
clients/python/agenta_client/__init__.py#L23-L23: Replace unquotedtyping.Anykeys with string names ("FullJsonInput", etc.).clients/python/agenta_client/__init__.py#L41-L41: Replace unquotedtyping.Anyelements with string names.clients/python/agenta_client/types/__init__.py#L779-L779: Replace unquotedtyping.Anykeys with string names.clients/python/agenta_client/types/__init__.py#L797-L797: Replace unquotedtyping.Anyelements with string names.📍 Affects 2 files
clients/python/agenta_client/__init__.py#L23-L23(this comment)clients/python/agenta_client/__init__.py#L41-L41clients/python/agenta_client/types/__init__.py#L779-L779clients/python/agenta_client/types/__init__.py#L797-L797docs/design/agent-workflows/projects/concurrent-approvals/PLAN.md (2)
148-155: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Clarify parked-gate count semantics for mixed pauses.
The plan says every gate is stored in
parkedApprovalsand thatapprovalGateCount === parkedApprovals.size, but Step 3 also permits non-parkable gates that have nopermissionId. Define the map as containing only parkable gates, retain a separate total pending-gate count, and compare those counts when deciding whether warm parking is safe.
289-295: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Align live QA criteria with current adapter behavior.
The current QA findings state that Claude and Pi serialize permission requests, so the live two-card scenario described here is not currently reproducible. Keep concurrent coverage in the fake concurrent harness, and make live validation cover the warm serial chain while documenting adapter-level concurrency as the separate limitation tracked in
#5391.Also applies to: 409-414
sdks/python/agenta/sdk/engines/tracing/tracing.py (1)
261-280: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Support raw OpenTelemetry spans in
store_agent.Raw OpenTelemetry spans (such as a
NonRecordingSpan) do not support thenamespacekeyword argument and will raise aTypeErrorwhen called. Thewith suppress():context manager will silently swallow this exception, dropping theagent_idattribute entirely.Consider adding the same
try...except TypeErrorfallback here that is already used instore_sessionto ensure the attribute is recorded across all span types.🐛 Proposed fix
if agent_id: - span.set_attribute("id", agent_id, namespace="agent") + try: + span.set_attribute("id", agent_id, namespace="agent") + except TypeError: + span.set_attribute("agent.id", agent_id)📝 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.def store_agent( self, agent_id: Optional[str] = None, span: Optional[Span] = None, ): """Set agent attributes on the current span. Args: agent_id: Unique identifier for the running artifact (workflow / application / evaluator id) span: Optional span to set attributes on (defaults to current span) """ self._warn_if_not_initialized("store_agent") with suppress(): if span is None: span = self.get_current_span() if agent_id: try: span.set_attribute("id", agent_id, namespace="agent") except TypeError: span.set_attribute("agent.id", agent_id)services/runner/src/engines/sandbox_agent/run-turn.ts (1)
762-781: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Log the failure instead of silently swallowing it.
appendSessionTurnis fire-and-forget with.catch(() => {}), discarding any error with no trace. Given this is exactly the durable write path implicated in the recentsession_turns500 incident, a silent swallow here makes a recurrence invisible.🩹 Proposed fix
void (deps.appendSessionTurn ?? appendSessionTurn)( sessionId, plan.harness, env.continuityTurnIndex, { streamId: request.streamId, agentSessionId: env.session.agentSessionId, sandboxId: env.sandbox?.sandboxId, references: workflowRefs ? Object.values(workflowRefs) : undefined, traceId: run.traceId(), }, { authorization: syncCred, log: logger }, - ).catch(() => {}); + ).catch((err) => + logger(`[turns] appendSessionTurn failed: ${errorMessage(err)}`), + );📝 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.// Append this turn to the durable turns log; fire-and-forget (a plain INSERT, no race). const syncCred = runCredential(request); if (syncCred && request.streamId) { const workflowRefs = buildWorkflowReferences( request.runContext?.workflow, ); void (deps.appendSessionTurn ?? appendSessionTurn)( sessionId, plan.harness, env.continuityTurnIndex, { streamId: request.streamId, agentSessionId: env.session.agentSessionId, sandboxId: env.sandbox?.sandboxId, references: workflowRefs ? Object.values(workflowRefs) : undefined, traceId: run.traceId(), }, { authorization: syncCred, log: logger }, ).catch((err) => logger(`[turns] appendSessionTurn failed: ${errorMessage(err)}`), ); }web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts (1)
177-199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Check how "done" / stopReason "paused" relate in the turn lifecycle rg -n '"done"|stopReason|paused' services/runner/src/engines/sandbox_agent/run-turn.ts services/runner/src/server.ts services/runner/src/protocol.ts | head -80Repository: Agenta-AI/agenta
Length of output: 3176
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '--- transcriptToMessages outline ---' ast-grep outline web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts --view expanded || true echo echo '--- transcriptToMessages relevant slices ---' sed -n '1,260p' web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts echo echo '--- run-turn pause/done slices ---' sed -n '620,820p' services/runner/src/engines/sandbox_agent/run-turn.ts echo echo '--- protocol done type ---' sed -n '360,420p' services/runner/src/protocol.tsRepository: Agenta-AI/agenta
Length of output: 23321
🏁 Script executed:
#!/bin/bash set -euo pipefail # Focus on the parts that decide whether a response can arrive in a later turn. sed -n '1,260p' web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts printf '\n---\n' sed -n '620,820p' services/runner/src/engines/sandbox_agent/run-turn.ts printf '\n---\n' sed -n '360,420p' services/runner/src/protocol.tsRepository: Agenta-AI/agenta
Length of output: 22255
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== transcriptToMessages: request/response logic ===' nl -ba web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts | sed -n '120,230p' echo echo '=== transcriptToMessages: draft reset / done handling ===' nl -ba web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts | sed -n '230,290p' echo echo '=== run-turn: stopReason and pause handling ===' nl -ba services/runner/src/engines/sandbox_agent/run-turn.ts | sed -n '650,810p' echo echo '=== server: pause/parkability hooks ===' nl -ba services/runner/src/server.ts | sed -n '410,460p'Repository: Agenta-AI/agenta
Length of output: 247
🏁 Script executed:
#!/bin/bash set -euo pipefail rg -n 'interaction_response|interaction_request|user_approval|toolCallId' services/runner/src web/oss/src | sed -n '1,220p'Repository: Agenta-AI/agenta
Length of output: 23137
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '### transcriptToMessages done boundary' nl -ba web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts | sed -n '240,270p' echo echo '### run-turn pause branch' nl -ba services/runner/src/engines/sandbox_agent/run-turn.ts | sed -n '740,800p'Repository: Agenta-AI/agenta
Length of output: 233
🏁 Script executed:
#!/bin/bash set -euo pipefail echo '=== transcriptToMessages continuation ===' sed -n '240,340p' web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.ts echo echo '=== transcriptToMessages tests ===' sed -n '1,200p' web/oss/src/components/AgentChatSlice/assets/transcriptToMessages.test.ts echo echo '=== acp-interactions response/request emission ===' sed -n '150,280p' services/runner/src/engines/sandbox_agent/acp-interactions.ts echo echo '=== session persist around interaction events ===' sed -n '260,340p' services/runner/src/sessions/persist.tsRepository: Agenta-AI/agenta
Length of output: 12409
Keep approval responses linked across turns
interaction_responseis persisted separately and the protocol says it returns cross-turn, but this handler only searches the current draft. Once adoneboundary closes the draft, a later response can’t reach the originalapproval-requestedpart, so the card stays pending on reload. Carry the tool-part index across drafts, or resolve against earlier drafts bytoolCallId.
|
The incident regression tests are in: the engine-seam test replays session db58551b's exact shape (two parallel gated calls, a gate surfacing mid-resume, a cancellation-closure frame) and asserts exactly-once real results, and the hydration test rebuilds the persisted record order and asserts no card flips back to waiting. Next: recorded live QA. |
|
An adversarial review pass over the whole train produced seven findings (four blockers); all seven are fixed in this commit, each with a regression test encoding its failure scenario. The blockers: reload hydration now resolves answers across turn boundaries via a transcript-wide index, the cold path resolves and emits the original interaction token, a partial answer set no longer lets the stale sweep cancel carried-forward gates, and gate classification gets a bounded quiet period against the pause sweep. |
1 participant
What this changes
The runner's approval park-and-resume path was hard-capped at one gate per turn by the
PendingApprovalLatch, and its keep-alive resume remembered only one parked gate. This PR generalizes both to N gates: the latch is removed so each gate emits its own approval card keyed by its owntoolCallId; every parked gate is stored in aparkedApprovalsmap; and one warm resume answers each parked gate by its ownpermissionId, so an approve and a deny in the same turn each land on the right call. It also fixes a real, pre-existing bug in the sibling force-settle path (below).The wire contract does not change shape: one
interaction_request(kind: user_approval) per gate, onetool-approval-requestSDK frame per event, one{approved}tool_resultkeyed bytoolCallId. An older frontend paired with the new runner still works.What users actually get today — read this before the before/after
Live QA on the EE dev stack (driving the real product endpoint, asserting on the SSE frame stream and the runner log, never on model prose — evidence in the comments) established that both live harnesses raise permission gates serially, not concurrently:
claude-agent-acp0.58.1): two gated tool calls in one turn produce exactly onerequest_permission. The adapter blocks on it and issues the next only after it is answered.So the headline "two approval cards in one turn" does not reproduce on Claude or Pi. When an agent issues two gated calls at once (the #5373 "read file A and file B" shape), the user is shown one card, answers it, and the second card arrives on the next turn — one card at a time.
On the warm keep-alive path this next card arrives cleanly: answer card 1, the approved tool runs, and card 2 arrives on the same live session with no model re-plan between the gates (the adapter unblocks
respondPermission, executes the tool, then raises the next gate — verified: noreasoningframes on the resume, the sibling keeps its originaltoolCallId, and the runner log shows the next gate on the same session with no cold miss). That warm serial chain is carried by the single-gate-per-turn keep-alive machinery that predates this PR; because the adapters serialize (a turn only ever holds one live gate), this PR's plural generalization is not exercised by Claude or Pi.Before / after
On a serializing adapter (Claude, Pi) — every harness in this stack today: behavior is unchanged. One card per gate, one at a time; on the warm path the gates chain without a model re-plan between them.
On a harness that raises genuinely concurrent gates (none exists here yet):
Real bugfix (independent of the concurrency repro)
The eager first-pause sibling force-settle used to settle a second gated call before its own permission request arrived — with gates arriving as separate ACP messages it would orphan a call that was about to emit its own card. Orphan settling is moved to the post-drain sweep (after
waitForEventDrainlets every pending gate mark its call paused), plus an in-band re-sweep for a sibling announced after the pause. A managed-cancelfailedframe for a non-gated sibling is suppressed so the sweep's deterministic "paused" result stands, while a legitimatecompletedresult from an auto-allowed sibling on a warm session is kept.run-turn.ts,runtime-policy.ts.Machinery changes (runner only for behavior; SDK/frontend get tests)
PendingApprovalLatch(no remaining consumer) so each gate emits its owninteraction_request, keyed by itstoolCallId. The turn still ends once (the pause is idempotent) and each carded gate is marked paused so the force-settle sweep leaves it open.permission-plan.ts,acp-interactions.ts,client-tools.ts.SessionEnvironmentholdsparkedApprovals: Map<toolCallId, ParkedApproval>, not just the first, plus anonParkablePauseCount.run-turn.ts,runtime-contracts.ts,environment-setup.ts.RunTurnOptions.resumecarries a list of decisions; the live resume callsrespondPermissiononce per parked gate, by its own id.server.ts,run-turn.ts.Testing
tool-approval-requestperuser_approvalevent for two events; ingress emits twotool_resultblocks keyed by their owntoolCallId.agentShouldResumeAfterApprovaldoes not resume with a second card pending and resumes once both settle.debug/qa-concurrent-approvals/.Relationship to #5373
Relates to #5373. This PR puts the runner-side machinery a concurrent-approval fix needs in place, but it does not resolve #5373's user-visible pain on the current adapters: Claude and Pi serialize their permission requests, so a multi-file approval still costs one card and one turn per file. The warm keep-alive path already carries that serial chain without a model re-plan between gates, which keeps it from being slow, but the cards do not arrive together. Collapsing a multi-file approval into a single set of cards requires the adapter to raise the gates concurrently; that adapter-level serialization is tracked in #5391. Deliberately not using "Closes #5373" — the observable fix is blocked upstream of this change.
Composition with in-flight work
markToolCallDenied, so the base isfeat/deny-frame-egress; the diff here is only the concurrent-approvals change.