feat(runner): reliable multi-gate approvals: truthful results, persisted answers, per-card dispatch#5382
feat(runner): reliable multi-gate approvals: truthful results, persisted answers, per-card dispatch#5382mmabrouk wants to merge 11 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
@coderabbitai review |
📝 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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ 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. |
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. |
|
Live QA exposed a step-2 regression: Pi executes a turn's parallel tool calls as one batch, so the bounded wait for an approved call's result deadlocked while a sibling gate was pending (five-minute hang, watchdog kill). The resume now parks immediately in that shape, carries the approved call ids, and the final resume seeds them so real results replace the interim sentinel in the records; Claude keeps the bounded wait, where it is correct. Root-cause report: docs/design/agent-workflows/scratch/debug-warm-resume-execution-failure.md. |
|
Live QA round two isolated the last two defects to the frontend and both are fixed with live-observed root causes: the auto-resume predicate's already-resumed guard misread a freshly clicked second card because the resume turn appends its step marker to the same message (the guard now applies only without a live click marker), and hydration treated the runner's park sentinels as final results, sealing a re-raised gate shut (a later approval request now reopens sentinel-only results, never real ones). The unknown-result sentinel also renders neutrally instead of as a failure. Root-cause report: docs/design/agent-workflows/scratch/debug-frontend-approval-dispatch.md. Next: the full QA re-run. |
QA cycle 4 — concurrent approvals (EE dev stack) — ALL GREENRe-ran the full Recording note: an MP4 of the passing run was captured (32s, step-by-step screenshots stitched with ffmpeg) but could not be uploaded here — the automation browser driving this run was not logged into GitHub, so the browser attachment flow was unavailable. The file is saved on the dev box at Checkpoints
Runner log excerpt (session
|
| Cell | Harness / sandbox | chat | approve | deny | warm | Note |
|---|---|---|---|---|---|---|
| C1 | claude / local (subscription) | PASS | PASS | PASS | — | approve executes the gated command; deny cleanly declines |
| C3 | pi_core / local (OpenAI) | BLOCKED | BLOCKED | BLOCKED | BLOCKED | Not an approval regression — the project's OpenAI vault key returns insufficient credit, so the model errors before any tool call (frames: start, start-step, error, finish-step, finish) |
| P1 | pi_core / local (OpenRouter) | BLOCKED | BLOCKED | BLOCKED | BLOCKED | openrouter/deepseek/deepseek-v4-flash not in this project's model allowlist |
The Pi gate cells are blocked by provider credit/allowlist on this dev project, not by the approval path. The pi_core human-approval park → resume → re-park path is proven directly by Scenario A above (the incident shape), with full wire-level evidence. The Claude approval and deny-path cells pass on the gate.
Conclusion: cycle 4 is fully green. Ready to merge pending a human review.
| settleBufferedPausedCompletions(); | ||
| const openAllowedExecutions = openToolCallIds() | ||
| .filter((id) => pause.isAllowedExecution(id)); | ||
| const piBatchBlockedByApproval = Boolean( |
There was a problem hiding this comment.
Reviewer focus: this is the Pi-batching park rule. Pi runs a turn's parallel tool calls as one batch and will not start an approved call until every sibling gate is also answered, so the bounded closure wait must be skipped here or the resume turn deadlocks for the full per-call limit (the five-minute hang QA hit). Check that the skip is gated to Pi only (Claude still closes in-turn) and that the carried approved-call ids are what let the final resume replace the interim APPROVED_EXECUTION_RESULT_UNKNOWN placeholder with the real result, so records end truthful.
| // Only park if still unsettled — a later `tool_result` overwrites this. | ||
| if (part.state === "input-available") { | ||
| index.approvals.set(str(payload.id), part) | ||
| const canRequestApproval = |
There was a problem hiding this comment.
Reviewer focus: the sentinel-reopen guard. On reload, a re-raised gate arrives as an interaction_request after the sibling was already stamped with a park sentinel (DEFERRED_NOT_EXECUTED / APPROVED_EXECUTION_RESULT_UNKNOWN). This must reopen the card only when the prior result is one of those non-final sentinels; verify a genuine result (output-available, output-denied, or a non-sentinel output-error) can never be reopened, otherwise an executed call would be resurrected as pending.
| return allSettled | ||
| if (!liveInteraction) { | ||
| // Restored tails need this guard; exact live markers are single-use and may target a part before `step-start`. | ||
| const resumedAlready = parts |
There was a problem hiding this comment.
Reviewer focus: the already-resumed guard is now bypassed on the live-marker path. The positional check ("did a step-start follow this card") misfires on a re-parked card, because a warm resume appends its step-start to the same message while the re-parked gate keeps its earlier array position, so the freshly clicked second card always looks "already resumed" and never dispatches. Confirm the guard runs only in the marker-less branch, and that the resend loop it originally prevented is still blocked because the live marker is cleared the moment a dispatch fires (so a marker-matched card dispatches at most once per click).
| const priorFp = historyFingerprint(priorConversation(request)); | ||
| const parkedList = [...existing.environment.parkedApprovals.values()]; | ||
| const resumeDecisions: ResumeApprovalInput[] = []; | ||
| const carriedForward: ParkedApproval[] = []; |
There was a problem hiding this comment.
Reviewer focus: the answered / carriedForward split for partial resumes. Check that a resume answering zero gates still takes the evict-to-cold path exactly as before (a non-empty answered set is what makes it resume live), so an empty answer set cannot silently strand the parked session. Then check the paired stale-sweep logic (inBandAnswerTokens, below): once any answer arrives, every parked token must be spared, including the carried-forward gates the resume re-parks, or the sweep would cancel gates the live process still holds pending.
mmabrouk
changed the title
1 participant
The symptom
During live QA, a user asked the agent to run two permission-gated shell commands in one turn (append a line to two files). The user approved the first command, and the playground reported it as "not executed" even though it had run. After the user approved the second command the whole conversation went silent and never replied, the already-approved first card flipped back to "waiting for approval", and a later question was answered with a stale "Done" instead of being read. Both files were in fact written exactly once, each only after its approval. What broke was the reporting, the rebuilt state, and the delivery of the final approval to the runner.
The runner is the Node/TypeScript sidecar under
services/runner/that drives a coding-agent harness (Pi or Claude) inside a sandbox. A gate is a human-approval request the harness raises before a policy-gated tool runs. When a gate has no answer the runner ends the turn and parks the live harness session in a keep-alive pool; when the answer arrives it checks the session back out and continues in place (a warm resume).What this PR changes
Truthful results when a turn pauses. Before: the post-pause cleanup swept every open tool call and stamped a "not executed, retry the same call" sentinel onto calls that had actually been approved and were mid-execution, so their real result was dropped and a side-effecting command was invited to run twice. After: an approved, executing call is excluded from the sweep and keeps its real executor result; a call that never started is recorded as deferred, never as a fake success.
Persisted approval answers. Before: the session record stored every approval request but no answer, so any state rebuild (a reload) resurrected answered gates as pending. That is why the first card flipped back to "waiting". After: resolving a gate emits a durable
interaction_responserecord and writes the allow/deny verdict into the interaction row'sresolutionfield. Rebuilt conversations overlay answers onto requests and render answered cards as answered.Per-card dispatch with partial answer sets. Before: the frontend sent approvals only once every visible card looked settled, a condition that could never hold after a rebuild, so the final answer sat unsent in browser memory and the conversation died. After: one click sends one answer. The runner accepts a resume that answers a subset of the parked gates, answers those, streams their results, and re-parks on the rest.
The Pi-batching park rule. Pi does not execute a parallel tool call the moment its own approval resolves; it waits until every sibling call in the batch has been answered, then runs them together. Before: the runner held the resume turn open waiting for the approved call's result, which under Pi could never arrive while a sibling gate was still pending, so the turn hung for the full five-minute per-call limit and the watchdog killed the stream. After: under Pi, when a fresh gate is still parked, the runner skips that bounded wait and parks immediately, carrying the approved call ids so the final resume replaces the interim placeholder with the real result. Claude keeps the bounded wait, where the approved call does finish inside the same turn.
Cross-turn rebuild correctness. Before: on reload, hydration treated the runner's park placeholders as final tool results, so a re-raised gate was sealed shut and the card could never be answered again. After: a later approval request reopens a placeholder-only result but never a real one, and answers are resolved across turn boundaries through a transcript-wide index, so a rebuilt conversation is answerable and matches the live one.
What this PR deliberately does not do
Merge order
This branch is stacked on the sessions rebase: PR #5375 (backend) and PR #5376 (runner) carry the shared
session_turnscounter fix and its 409 mapping, and merge first. This PR's base issessions-rebase/runner; merge it after those two.How this was verified
resolutionverdict.debug/qa-concurrent-approvals/qa-concurrent-approvals-pass.mp4) and is pending a manual browser upload to this thread.