docs(agent-workflows): streaming invoke for reliable multi-tool output - #5003
docs(agent-workflows): streaming invoke for reliable multi-tool output#5003mmabrouk wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThis PR adds five design documents about streaming invoke reliability, describing the batch-versus-streaming output gap, the approval-boundary failure mode, and the proposed platform-side draining path. It also records rollout order, test coverage, and current review status. ChangesStreaming invoke design documentation
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Design-doc-only, no code. Please review the decisions in
Live evidence is in @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
repo="Agenta-AI/agenta"
pr=5003
echo "=== PR files ==="
gh pr view $pr --repo $repo --json files --jq '.files[].path'
echo "=== PR diff (doc files) ==="
gh pr diff $pr --repo $repoLength of output: 27341 🏁 Script executed: #!/bin/bash
set -euo pipefail
git fetch origin big-agents --depth=50 2>&1 | tail -5
git show origin/big-agents:services/oss/src/agent/app.py 2>/dev/null | sed -n '260,325p' || echo "FILE_NOT_FOUND_app.py"
echo "---dtos.py---"
git show origin/big-agents:sdks/python/agenta/sdk/agents/dtos.py 2>/dev/null | sed -n '320,350p;495,520p' || echo "FILE_NOT_FOUND_dtos.py"
echo "---wire.py---"
git show origin/big-agents:sdks/python/agenta/sdk/agents/utils/wire.py 2>/dev/null | sed -n '130,190p' || echo "FILE_NOT_FOUND_wire.py"
echo "---interfaces.py stream/result---"
git show origin/big-agents:sdks/python/agenta/sdk/agents/interfaces.py 2>/dev/null | rg -n 'def stream|def result|class .*Run|messages|events' | head -60Length of output: 1245 🏁 Script executed: #!/bin/bash
set -uo pipefail
git fetch origin big-agents --depth=50 2>&1 | tail -3
echo "=== find files ==="
git ls-tree -r --name-only FETCH_HEAD | rg -i 'agent/app\.py$|agents/dtos\.py$|agents/utils/wire\.py$|agents/interfaces\.py$'Length of output: 639 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== app.py 260-325 ==="
git show FETCH_HEAD:services/oss/src/agent/app.py | sed -n '260,325p'
echo "=== dtos.py Event & AgentResult ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/dtos.py | sed -n '320,350p;495,525p'
echo "=== wire.py result_from_wire ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/utils/wire.py | sed -n '130,190p'
echo "=== interfaces.py stream/result ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/interfaces.py | rg -n 'def stream|def result|class |messages|events|stop_reason'Length of output: 8561 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== search AgentStream class ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/dtos.py | rg -n 'class AgentStream|def result|on_result|on_cleanup'
echo "=== find AgentStream definition file ==="
git grep -n "class AgentStream" FETCH_HEAD -- 'sdks/**' 2>/dev/null || git show FETCH_HEAD -- sdks | true
git ls-tree -r --name-only FETCH_HEAD | rg -i 'sdks/python/agenta/sdk/agents'Length of output: 3479 🏁 Script executed: #!/bin/bash
set -uo pipefail
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/streaming.py | sed -n '1,140p'Length of output: 4226 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== interfaces.py stream() context ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/interfaces.py | sed -n '240,296p'
echo "=== search for messages/events keys in runner wire building (rust/ts side) ==="
git ls-tree -r --name-only FETCH_HEAD | rg -i 'runner' | rg -i '\.rs$|\.ts$' | head -20Length of output: 3296 🏁 Script executed: #!/bin/bash
set -uo pipefail
git show FETCH_HEAD:services/runner/src/engines/sandbox_agent/transcript.ts | sed -n '1,200p'Length of output: 3441 🏁 Script executed: #!/bin/bash
set -uo pipefail
git grep -n "stopReason\|messages:\|events:" FETCH_HEAD -- services/runner/src/engines/sandbox_agent/run-plan.ts services/runner/src/engines/sandbox_agent/daemon.ts services/runner/src/engines/sandbox_agent.ts 2>/dev/null | head -60Length of output: 1112 🏁 Script executed: #!/bin/bash
set -uo pipefail
git show FETCH_HEAD:services/runner/src/engines/sandbox_agent.ts | sed -n '580,830p'Length of output: 11281 🏁 Script executed: #!/bin/bash
set -uo pipefail
git grep -n "emit" FETCH_HEAD -- services/runner/src/engines/sandbox_agent.ts | head -40Length of output: 1440 🏁 Script executed: #!/bin/bash
set -uo pipefail
git grep -n "emit" FETCH_HEAD -- services/runner/src/entry.ts services/runner/src/cli.ts services/runner/src/engines/skills.ts 2>/dev/null | head -30
echo "---"
git grep -rn "runSandboxAgent\|deliver_.*stream\|delivery" FETCH_HEAD -- services/runner/src 2>/dev/null | head -40Length of output: 3305 🏁 Script executed: #!/bin/bash
set -uo pipefail
git show FETCH_HEAD:services/runner/src/server.ts | sed -n '100,200p'Length of output: 4484 🏁 Script executed: #!/bin/bash
set -uo pipefail
git grep -n "x-ndjson\|application/x-ndjson\|/run\b" FETCH_HEAD -- sdks/python/agenta/sdk/agents/adapters/sandbox_agent.py 2>/dev/null | head -30
echo "---utils dir---"
git ls-tree -r --name-only FETCH_HEAD | rg -i 'sdks/python/agenta/sdk/agents/utils'Length of output: 1062 🏁 Script executed: #!/bin/bash
set -uo pipefail
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/adapters/sandbox_agent.py | sed -n '1,170p'Length of output: 6144
3. Source of the full list — resolved, and it's the pessimistic branch.
So On the other three: 1. Batch history default: Agree with framing 1 (default 2. 4. Option C scope: Agree, ship A+B now and track C separately. The approval-boundary gap looks like it needs the runner-side HITL/park lifecycle understood first ( Overall: plan is sound, the wire-compat framing-1 lean is right, and the one code fact you asked me to verify confirms the harder implementation path — good to flag that explicitly before the implementation PR starts. ✏️ Learnings added
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a23edf5e-59e3-4b16-b088-2d398f609586
📒 Files selected for processing (5)
docs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/README.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/context.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/plan.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/research.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/status.md
| ``` | ||
| I have all the data. Picking `team-testing` (C085TMJJ0PK) as the test channel and posting the | ||
| digest now. | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Label the example fence.
This bare fence trips markdownlint (MD040). Mark it as plain text so the example renders cleanly and the doc stays lintable.
♻️ Suggested wording
-```
+```text
I have all the data. Picking `team-testing` (C085TMJJ0PK) as the test channel and posting the
digest now.
-```
+```📝 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.
| ``` | |
| I have all the data. Picking `team-testing` (C085TMJJ0PK) as the test channel and posting the | |
| digest now. | |
| ``` |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 10-10: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
Source: Linters/SAST tools
| - Source of the list: `result.messages` if the streaming-drained terminal result populates it | ||
| (research §2 verification item); otherwise assemble it in `_agent_batch` from the events it | ||
| already iterates and currently discards (`app.py:315`). | ||
| - Also surface `result.stop_reason` on the response (e.g. as `outputs.stop_reason` or a status | ||
| field), so a caller can distinguish "ended on a tool turn" from "completed its answer" without | ||
| inspecting the message list. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Collapse the already-settled branches.
The review direction here is no longer conditional: _agent_batch must assemble the full turn itself, and stop_reason should be exposed on outputs. Keeping result.messages / workflow status as live options will steer the implementation toward the wrong contract.
Suggested wording
-- Source of the list: `result.messages` if the streaming-drained terminal result populates it
- (research §2 verification item); otherwise assemble it in `_agent_batch` from the events it
- already iterates and currently discards (`app.py:315`).
-- Also surface `result.stop_reason` on the response (e.g. as `outputs.stop_reason` or a status
- field), so a caller can distinguish "ended on a tool turn" from "completed its answer" without
- inspecting the message list.
++ `_agent_batch` assembles the full turn itself from the drained events it already iterates
++ (`app.py:315`).
++ Surface `result.stop_reason` on `outputs.stop_reason` so callers can distinguish "ended on a
++ tool turn" from "completed its answer" without inspecting the message list.Also applies to: 71-75
| turn. The full turn — every tool call, every tool result, the stop reason — is already produced by | ||
| the runner and is fully visible over a **streaming invoke** (`Accept: application/x-ndjson`), | ||
| confirmed live. So the caller *can* already get a reliable, self-describing result today; the batch | ||
| default just throws it away. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Tighten the streaming claim.
The README says streaming already exposes every tool result and the stop reason, but the approval-boundary gap documented below means the terminal tool result still does not appear in the stream. Rephrase this as the pre-approval turn so the top-level summary matches the rest of the design.
♻️ Suggested wording
- The full turn — every tool call, every tool result, the stop reason — is already produced by the runner and is fully visible over a **streaming invoke** (`Accept: application/x-ndjson`), confirmed live. So the caller *can* already get a reliable, self-describing result today; the batch default just throws it away.
+ The pre-approval turn — every tool call and tool result before the approval boundary — is already produced by the runner and is fully visible over a **streaming invoke** (`Accept: application/x-ndjson`), confirmed live. The terminal approval-boundary result is tracked separately.📝 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.
| turn. The full turn — every tool call, every tool result, the stop reason — is already produced by | |
| the runner and is fully visible over a **streaming invoke** (`Accept: application/x-ndjson`), | |
| confirmed live. So the caller *can* already get a reliable, self-describing result today; the batch | |
| default just throws it away. | |
| turn. The pre-approval turn — every tool call and tool result before the approval boundary — is already produced by the runner and is fully visible over a **streaming invoke** (`Accept: application/x-ndjson`), confirmed live. The terminal approval-boundary result is tracked separately. |
| `result_from_wire` (`sdks/python/agenta/sdk/agents/utils/wire.py:149-183`) parses the runner's | ||
| `/run` result and **populates `messages`, `events`, and `stop_reason`** from the wire | ||
| (`data["messages"]`, `data["events"]`, `data["stopReason"]`). So the runner emits them and the SDK | ||
| already models them. Verification item: confirm the **streaming-drained** terminal result | ||
| (`session.stream(...).result()`, `interfaces.py:278-296`) populates `.messages`/`.events` the same | ||
| way the one-shot `/run` result does. If it does, Option A is a handler-local change. If the | ||
| streaming result leaves them empty, `_agent_batch` can build the list from the events it already | ||
| iterates (it currently discards them at `app.py:315`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
State the resolved finding here.
This still reads like an open verification item, but the PR findings already settle it: the streaming-drained terminal result does not populate .messages/.events the way the one-shot /run result does, so _agent_batch has to assemble the list from the streamed events it already iterates. Update the text to reflect the concluded behavior.
♻️ Suggested wording
- Verification item: confirm the **streaming-drained** terminal result (`session.stream(...).result()`, `interfaces.py:278-296`) populates `.messages`/`.events` the same way the one-shot `/run` result does. If it does, Option A is a handler-local change. If the streaming result leaves them empty, `_agent_batch` can build the list from the events it already iterates (it currently discards them at `app.py:315`).
+ The **streaming-drained** terminal result (`session.stream(...).result()`, `interfaces.py:278-296`) does not populate `.messages`/`.events` the same way the one-shot `/run` result does, so `_agent_batch` must build the list from the events it already iterates.📝 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.
| `result_from_wire` (`sdks/python/agenta/sdk/agents/utils/wire.py:149-183`) parses the runner's | |
| `/run` result and **populates `messages`, `events`, and `stop_reason`** from the wire | |
| (`data["messages"]`, `data["events"]`, `data["stopReason"]`). So the runner emits them and the SDK | |
| already models them. Verification item: confirm the **streaming-drained** terminal result | |
| (`session.stream(...).result()`, `interfaces.py:278-296`) populates `.messages`/`.events` the same | |
| way the one-shot `/run` result does. If it does, Option A is a handler-local change. If the | |
| streaming result leaves them empty, `_agent_batch` can build the list from the events it already | |
| iterates (it currently discards them at `app.py:315`). | |
| `result_from_wire` (`sdks/python/agenta/sdk/agents/utils/wire.py:149-183`) parses the runner's | |
| `/run` result and **populates `messages`, `events`, and `stop_reason`** from the wire | |
| (`data["messages"]`, `data["events"]`, `data["stopReason"]`). So the runner emits them and the SDK | |
| already models them. The **streaming-drained** terminal result (`session.stream(...).result()`, | |
| `interfaces.py:278-296`) does not populate `.messages`/`.events` the same way the one-shot `/run` | |
| result does, so `_agent_batch` must build the list from the events it already iterates. |
|
|
||
| The live streaming run exposed a related but distinct gap. The terminal `SEND_MESSAGE` tripped a | ||
| `user_approval` interaction. Over a one-shot HTTP invoke there is no reply channel, so the stream | ||
| emitted the `interaction_request` and then `done` — it did **not** stream a `SEND_MESSAGE` |
There was a problem hiding this comment.
I don't understand this. I want to determine first if this is a bug. From what I understand from your explanation, if a tool is set to auto-approve and you make a call with streaming, although the tool is called, you don't get the full message. It stops only at the tool call. Now, that is strange behavior, and I wonder if this is a bug or something else.
How does the frontend deal with this? We don't see this issue in the frontend. Does the frontend wait for this user approval interaction and send another message so that it gets the new stream, or how does it work from its side? Why does it happen like that, returning an interaction request instead of finishing? Can you also do a git blame and see when this behavior started happening? From my understanding, the right behavior is that it continues working. If auto-approval happens, it means it happens within the sidecar. We did not need that; that's not a real interaction.
I'd love to add a new page in this PR with more description about this, with much, much more detail: when was this introduced, is it a bug, how the frontends deal with it, etc.
| separately close the approval-boundary gap so an auto-approved terminal tool's result is observable | ||
| (Option C). | ||
|
|
||
| ## Option A — batch returns the full `outputs.messages` (recommended) |
There was a problem hiding this comment.
No, we are not going to change the Bach Enfoc. It will stay as is.
| the single-message shape. Decide against the actual consumers (product API invoke path, playground | ||
| FE, evaluators) — see open question 1. | ||
|
|
||
| ## Option B — streaming is the documented client path (done client-side) |
There was a problem hiding this comment.
Now this is exactly the route that we want to go. From our side, we will not use patch invoke. We will only use streaming. This means in the lab kit we need to use kind of streaming, but also the platform operation needs to use streaming, because otherwise it will have the same problem if that invoke tool is called from within the application.
| runs should use a streaming Accept type (or `history: full` once Option A lands), and that the batch | ||
| default returns only the final text. | ||
|
|
||
| ## Option C — close the approval-boundary gap (separate, harder) |
There was a problem hiding this comment.
This might be a bug. See above comment. Let's first research it before going much more in detail here.
| do not depend on it. Until it lands, `check-tools.sh <trace> <WRITE_TOOL>` stays the way to confirm | ||
| a gated write returned `ok:true`. | ||
|
|
||
| ## Decisions needed |
There was a problem hiding this comment.
I've answered before, so let's go over that before going into details. I don't think any of what you said here is really a real question.
mmabrouk
left a comment
mmabrouk
left a comment
There was a problem hiding this comment.
Please read the comments and create a second version of this.
Let's focus on:
- The issue with human approvals: is this a bug? What is it?
- The main problem, which is how we make the platform tool actually use streaming so that when an agent uses it to test something, it works.
… untouched, approval-boundary bug Second-round revision per PR #5003 review. Batch invoke stays as-is (single final message). Direction is streaming everywhere the full turn matters: the client (lab kit, shipped) and the platform's own internal invoke paths (workflow/agent-as-tool at tools/router.py:1306 and evaluations run batch today; proposal is a draining invoke_workflow_streaming variant). Adds approval-boundary.md: an auto-approved run stops at the tool gate because HITLResponder parks on any session id before consulting the auto policy (responder.ts:257); introduced in b109cc5; the playground hides it via useChat resume; verdict is a bug with a fix at responder.ts:254-259. Claude-Session: https://claude.ai/code/session_01WSp2LqKrEtXnm2fsPWuQWa
572aa89 to
403e31b
Compare
Second round — revised per your reviewApplied your decisions. Batch is untouched; the direction is streaming everywhere; the approval boundary is now a dedicated page with a bug verdict. 1. Batch invoke stays as-is. Dropped the batch-coalescing proposal entirely. 2. Streaming everywhere, including the platform's own invoke. The lab kit client already streams. New: where Agenta invokes an agent from inside the platform, it sends
3. Approval boundary — new page
What feedback I need
@coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -uo pipefail
git fetch origin big-agents --depth=100 2>&1 | tail -3
echo "=== tools/router.py around 1306 ==="
git show FETCH_HEAD:api/oss/src/apis/fastapi/tools/router.py | sed -n '1270,1330p'
echo "=== evaluations adapters.py around 104,508 ==="
git show FETCH_HEAD:api/oss/src/core/evaluations/runtime/adapters.py | sed -n '90,120p;495,520p'
echo "=== entrypoints/routers.py around 790-820 ==="
git show FETCH_HEAD:api/entrypoints/routers.py | sed -n '780,825p'
echo "=== responder.ts around 240-270 ==="
git show FETCH_HEAD:services/agent/src/responder.ts | sed -n '230,275p'
echo "=== normalizer.py around 290-315 ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/agents/utils/normalizer.py | sed -n '285,315p' 2>/dev/null || echo "check file path"
git ls-tree -r --name-only FETCH_HEAD | rg -i 'normalizer\.py$'
echo "=== relay.ts around 100-115 ==="
git ls-tree -r --name-only FETCH_HEAD | rg -i 'relay\.ts$'Length of output: 7466 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== find responder.ts ==="
git ls-tree -r --name-only FETCH_HEAD | rg -i 'responder\.ts$'
echo "=== find HITLResponder / onPermission ==="
git grep -n "onPermission\|hasHumanSurface\|class HITLResponder" FETCH_HEAD -- '*.ts' | head -30
echo "=== normalizer.py sessionId ==="
git show FETCH_HEAD:sdks/python/agenta/sdk/middlewares/running/normalizer.py | sed -n '280,320p'
echo "=== relay.ts around 95-120 ==="
git show FETCH_HEAD:services/runner/src/tools/relay.ts | sed -n '90,120p'
echo "=== AgentChatPanel.tsx ==="
git ls-tree -r --name-only FETCH_HEAD | rg -i 'AgentChatPanel\.tsx$'Length of output: 7209 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== responder.ts 180-225 ==="
git show FETCH_HEAD:services/runner/src/responder.ts | sed -n '180,225p'
echo "=== resolve_session_id ==="
git grep -n "def resolve_session_id" FETCH_HEAD -- '*.py'
git show FETCH_HEAD:sdks/python/agenta/sdk/middlewares/running/normalizer.py | rg -n "def resolve_session_id" -A 15
echo "=== TODO(S5) in relay.ts ==="
git grep -n "TODO(S5)\|TODO(S5" FETCH_HEAD -- services/runner/src/tools/relay.ts
echo "=== commit b109cc51ef ==="
git log --all --oneline | grep -i b109cc51 || echo "not in shallow history"
git show b109cc51ef --stat 2>&1 | head -30Length of output: 5524 🏁 Script executed: #!/bin/bash
set -uo pipefail
echo "=== diff for permissions.ts in b109cc51ef ==="
git show b109cc51ef -- '*permissions.ts' | head -100
echo "=== check if services/agent existed before rename ==="
git log --all --diff-filter=A --name-only -- '*services/agent/src/responder.ts' 2>&1 | head -10
git log --all --oneline -- 'services/runner/src/responder.ts' 2>&1 | tail -5Length of output: 3897 🏁 Script executed: #!/bin/bash
set -uo pipefail
git show FETCH_HEAD:sdks/python/agenta/sdk/models/shared.py | sed -n '1,40p'Length of output: 1601
Verified independently:
Two citation corrections for
On your open questions:
Nice diagnosis on the dead-code branch — that's a subtle one to catch from a trace alone. ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: cad8fa80-8aef-4a27-8a3f-145689adf171
📒 Files selected for processing (6)
docs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/README.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/approval-boundary.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/context.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/plan.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/research.mddocs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/status.md
✅ Files skipped from review due to trivial changes (1)
- docs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/status.md
| (`services/agent/src/engines/sandbox_agent/permissions.ts:63-77`). This is the only emitter of | ||
| `kind: user_approval` in the codebase, so the event in the trace fires for every gate the harness | ||
| raises, regardless of policy. | ||
|
|
||
| 2. **The responder parks on a "human surface" before it checks the policy.** | ||
| `HITLResponder.onPermission` (`services/agent/src/responder.ts:254-259`): | ||
|
|
||
| ```ts | ||
| async onPermission(request: PermissionRequest): Promise<ResponderOutcome> { | ||
| const stored = this.lookupPermission(request); | ||
| if (stored) return stored; | ||
| if (this.hasHumanSurface) return "park"; // line 257 — parks REGARDLESS of policy | ||
| return this.basePolicy === "deny" ? "deny" : "allow"; // headless-only, unreachable in practice | ||
| } | ||
| ``` | ||
|
|
||
| Line 257 returns `park` whenever `hasHumanSurface` is true, *before* it ever looks at | ||
| `basePolicy`. The auto-allow branch on line 258 is only reachable when there is no human surface. | ||
| `PermissionPolicy` is only `"auto" | "deny"` (`responder.ts:33`) — there is no `ask` value — so the | ||
| responder cannot tell "this specific tool needs a human" from "the policy auto-approves | ||
| everything." It parks on all of them. | ||
|
|
||
| 3. **`hasHumanSurface` is just "is there a sessionId".** | ||
| `services/agent/src/engines/sandbox_agent.ts:511`: | ||
| `const hasHumanSurface = !!(request.sessionId && request.sessionId.trim());` | ||
| The responder is built with that flag (`sandbox_agent.ts:537-544`). | ||
|
|
||
| 4. **The SDK mints a sessionId for every invoke, headless or not.** The running-middleware normalizer | ||
| resolves a session id once before the handler runs and mints one when it is absent | ||
| (`sdks/python/agenta/sdk/middlewares/running/normalizer.py:302-308`, | ||
| `sdks/python/agenta/sdk/models/shared.py:13-22` — `resolve_session_id` returns `uuid4().hex`). It | ||
| flows into `SessionConfig.session_id` (`services/oss/src/agent/app.py:216, 254`) and becomes the | ||
| runner's `AgentRunRequest.sessionId`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Refresh the stale code-path citations.
These references still point at services/agent/src/..., but the current runner-side implementation is under services/runner/src/responder.ts, and the relay.ts TODO line has moved. Since this page is meant to pin exact locations, please update the root-cause section and the key-files list together.
Also applies to: 103-107, 218-224
🧰 Tools
🪛 LanguageTool
[grammar] ~62-~62: Ensure spelling is correct
Context: ...ent.ts:537-544`). 4. The SDK mints a sessionId for every invoke, headless or not. Th...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
| Streaming is not a workaround bolted on for this test. It is a first-class negotiated format the SDK | ||
| routing already supports (`Accept: text/event-stream | application/x-ndjson | application/jsonl`, or | ||
| `flags.stream: true`). All three return 200 with the full event stream. | ||
|
|
||
| ## The direction: stream everywhere, leave batch alone | ||
|
|
||
| Batch returns only the final assistant text by design, and it stays that way. It is the right shape for | ||
| a caller that wants one final message. We are **not** reshaping batch or coalescing the full turn into | ||
| its response. | ||
|
|
||
| The answer to "the multi-tool output is unreliable" is to use **streaming** on every path that needs | ||
| the whole turn. There are two such paths: | ||
|
|
||
| 1. **The external client.** A client asks for a stream (`Accept: application/x-ndjson`) and reads the | ||
| full turn directly. This is already done in the lab kit (`test-agent.sh` streams and prints the | ||
| ordered tool list; see `plan.md`). | ||
| 2. **The platform's own invoke.** When Agenta invokes an agent from *inside* the platform — an agent | ||
| invoking another app/agent as a tool, an evaluation running a workflow-under-test, a trigger firing | ||
| a workflow — the same batch-vs-stream choice applies, set by the `Accept` header the platform sends. | ||
| Today those paths send `Accept: application/json` and read a single batch body, so they hit the | ||
| identical empty/mid-sentence problem, now server-side. The platform-side change is to make the | ||
| result-consuming invoke paths stream and drain the event stream. Where the platform invokes agents, | ||
| which paths are batch today, and the proposed change are in `plan.md` and `research.md`. | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clarify the stream-negotiation rule.
The later 406 example shows that flags.stream: true is not a standalone override when Accept is application/json. As written, “or flags.stream: true” / “All three return 200” reads too broadly and can mislead anyone implementing the platform-side drain.
Suggested wording
- Streaming is a first-class negotiated format the SDK routing already supports (`Accept: text/event-stream | application/x-ndjson | application/jsonl`, or `flags.stream: true`). All three return 200 with the full event stream.
+ Streaming is a first-class negotiated format the SDK routing already supports (`Accept: text/event-stream | application/x-ndjson | application/jsonl`); `flags.stream: true` still requires a stream-compatible `Accept` header.📝 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.
| Streaming is not a workaround bolted on for this test. It is a first-class negotiated format the SDK | |
| routing already supports (`Accept: text/event-stream | application/x-ndjson | application/jsonl`, or | |
| `flags.stream: true`). All three return 200 with the full event stream. | |
| ## The direction: stream everywhere, leave batch alone | |
| Batch returns only the final assistant text by design, and it stays that way. It is the right shape for | |
| a caller that wants one final message. We are **not** reshaping batch or coalescing the full turn into | |
| its response. | |
| The answer to "the multi-tool output is unreliable" is to use **streaming** on every path that needs | |
| the whole turn. There are two such paths: | |
| 1. **The external client.** A client asks for a stream (`Accept: application/x-ndjson`) and reads the | |
| full turn directly. This is already done in the lab kit (`test-agent.sh` streams and prints the | |
| ordered tool list; see `plan.md`). | |
| 2. **The platform's own invoke.** When Agenta invokes an agent from *inside* the platform — an agent | |
| invoking another app/agent as a tool, an evaluation running a workflow-under-test, a trigger firing | |
| a workflow — the same batch-vs-stream choice applies, set by the `Accept` header the platform sends. | |
| Today those paths send `Accept: application/json` and read a single batch body, so they hit the | |
| identical empty/mid-sentence problem, now server-side. The platform-side change is to make the | |
| result-consuming invoke paths stream and drain the event stream. Where the platform invokes agents, | |
| which paths are batch today, and the proposed change are in `plan.md` and `research.md`. | |
| Streaming is not a workaround bolted on for this test. It is a first-class negotiated format the SDK | |
| routing already supports (`Accept: text/event-stream | application/x-ndjson | application/jsonl`); | |
| `flags.stream: true` still requires a stream-compatible `Accept` header. All three return 200 with the | |
| full event stream. | |
| ## The direction: stream everywhere, leave batch alone | |
| Batch returns only the final assistant text by design, and it stays that way. It is the right shape for | |
| a caller that wants one final message. We are **not** reshaping batch or coalescing the full turn into | |
| its response. | |
| The answer to "the multi-tool output is unreliable" is to use **streaming** on every path that needs | |
| the whole turn. There are two such paths: | |
| 1. **The external client.** A client asks for a stream (`Accept: application/x-ndjson`) and reads the | |
| full turn directly. This is already done in the lab kit (`test-agent.sh` streams and prints the | |
| ordered tool list; see `plan.md`). | |
| 2. **The platform's own invoke.** When Agenta invokes an agent from *inside* the platform — an agent | |
| invoking another app/agent as a tool, an evaluation running a workflow-under-test, a trigger firing | |
| a workflow — the same batch-vs-stream choice applies, set by the `Accept` header the platform sends. | |
| Today those paths send `Accept: application/json` and read a single batch body, so they hit the | |
| identical empty/mid-sentence problem, now server-side. The platform-side change is to make the | |
| result-consuming invoke paths stream and drain the event stream. Where the platform invokes agents, | |
| which paths are batch today, and the proposed change are in `plan.md` and `research.md`. |
| - **The platform's own invoke must stream too.** When Agenta invokes an agent from inside the | ||
| platform — an agent invoking another app/agent as a tool, an evaluation running a | ||
| workflow-under-test, a trigger firing a workflow — it sends `Accept: application/json` today and | ||
| gets the same partial output. The proposed change is a draining `invoke_workflow_streaming` variant | ||
| on `WorkflowsService`, applied first to the workflow/agent-as-tool path | ||
| (`api/oss/src/apis/fastapi/tools/router.py:1306`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Narrow the internal-invoke scope.
plan.md only treats workflow/agent-as-tool and evaluations as synchronous result consumers. Triggers/schedules and session-respond are detached in production, so this sentence currently overstates which paths see the partial-output failure.
Suggested wording
- When Agenta invokes an agent from inside the platform — an agent invoking another app/agent as a tool, an evaluation running a workflow-under-test, a trigger firing a workflow — it sends Accept: application/json today and gets the same partial output.
+ When Agenta invokes an agent from inside the platform — especially workflow/agent-as-tool and evaluations, which consume the result synchronously — it sends `Accept: application/json` today and gets the same partial output.📝 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.
| - **The platform's own invoke must stream too.** When Agenta invokes an agent from inside the | |
| platform — an agent invoking another app/agent as a tool, an evaluation running a | |
| workflow-under-test, a trigger firing a workflow — it sends `Accept: application/json` today and | |
| gets the same partial output. The proposed change is a draining `invoke_workflow_streaming` variant | |
| on `WorkflowsService`, applied first to the workflow/agent-as-tool path | |
| (`api/oss/src/apis/fastapi/tools/router.py:1306`). | |
| When Agenta invokes an agent from inside the platform — especially workflow/agent-as-tool and evaluations, which consume the result synchronously — it sends `Accept: application/json` today and gets the same partial output. |
403e31b to
ac99f2e
Compare
|
Rebased onto big-agents |
|
This seems ton impact |
…untouched, approval-boundary bug Rebased onto big-agents f8765a9. Claude-Session: https://claude.ai/code/session_01WSp2LqKrEtXnm2fsPWuQWa
ac99f2e to
ec47387
Compare
2 participants
What
Design docs only (no code). Second-round revision of the streaming-invoke workspace under
docs/design/agent-workflows/projects/builder-agent-reliability/streaming-invoke/, per the review on this PR. Now six files (addsapproval-boundary.md).The direction (revised)
Use streaming everywhere the full turn matters, and leave batch alone.
_agent_batchkeeps returning one final assistant message (services/oss/src/agent/app.py:303-321). The earlier batch-coalescing proposal is dropped.test-agent.sh).Accept: application/jsontoday and gets the same partial output. The result-consuming batch paths are the workflow/agent-as-tool path (api/oss/src/apis/fastapi/tools/router.py:1306) and the evaluations runtime (api/oss/src/core/evaluations/runtime/adapters.py:104,508). Triggers/schedules and session-respond run detached in production, not batch. Proposal: a draininginvoke_workflow_streamingvariant onWorkflowsService, applied to workflow-as-tool first.New page:
approval-boundary.mdAn auto-approved run stops at the tool gate.
HITLResponder.onPermissionreturnsparkwhenever a session id is present, before it consults theautopolicy (services/agent/src/responder.ts:257), and the SDK mints asessionIdfor every invoke, soautonever auto-approves in-band. The turn ends at the gate withstopReason: "paused"; the terminal tool'stool_resultnever streams. The playground hides this by auto-resending on the park (useChat+sendAutomaticallyWhen). Introduced in commitb109cc51ef(2026-06-25). Verdict: bug. Recommended fix atresponder.ts:254-259, with anask-disposition follow-up so the playground still prompts.Built via temp-index
commit-treeonorigin/big-agents(zero working-tree touch). Diff is exactly the six doc files.https://claude.ai/code/session_01WSp2LqKrEtXnm2fsPWuQWa