fix(runtime): stop content-only turns that end with a bare EOF (no finish_reason)#3669
fix(runtime): stop content-only turns that end with a bare EOF (no finish_reason)#3669awschmeder wants to merge 2 commits into
finish_reason)#3669Conversation
A sub-agent reached via transfer_task returns control to its hub by finishing its turn: a final message with content and no tool call. When the provider closes that turn's SSE stream with a bare EOF and no per-choice finish_reason (common with OpenAI-compatible gateways such as litellm/CBORG), handleStream keyed its stop decision on empty content, so a content-bearing final turn with no tool calls reported Stopped=false. runTurn then re-entered the model with an unchanged message list and the model re-emitted the same completion forever, so the hand-off never completed. Key the bare-EOF stop decision on the absence of tool calls, matching the mid-stream path. A turn is terminal on bare EOF whenever no tool calls remain to execute, regardless of content. The empty-content cases the original docker#3145 guard covered (token limit, whitespace-only or reasoning-only reply) stay covered because they also have no tool calls. Add TestHandleStream_ContentOnlyBareEOFStops. Update TestCompactLiveSession_DuplicateSessionIDsCompactOnlyTargetEntry, whose older stream relied on a content-only bare-EOF turn continuing to a second model call; make that turn a tool call so it deterministically reaches its iteration boundary independent of the stop rule. Fixes docker#3668 Signed-off-by: awschmeder <awschmeder@lbl.gov>
aheritier
added
area/runtime
There was a problem hiding this comment.
Root cause and fix are correct: the bare-EOF path now matches the mid-stream path (Stopped: len(toolCalls) == 0). Build, vet and runtime tests pass locally on the merge with current main.
CI failure (TestDocYAMLSnippetsAreValid on docs/tools/scheduler/index.md) is unrelated to this diff: the run hit the window where main was broken between #3632 and #3670, and the file does not exist on this branch. A re-run or rebase should clear it.
Blocking: the "content + bare EOF keeps the loop running" test idiom was converted in only one of the three live-session tests using it. Measured by instrumenting both compaction drain sites:
| drain path | main | this PR |
|---|---|---|
iteration boundary (runQueuedCompaction, loop.go:444) |
3 tests | 0 |
teardown (finishLiveSession) |
2 tests | 5 |
TestCompactLiveSession_ExecutesAtIterationBoundary still passes but via the teardown drain, so the boundary path loses all coverage. Converting its turn 1 to a tool-call turn (same idiom as DuplicateSessionIDsCompactOnlyTargetEntry) restores it. Same pattern in HookVetoSynthesizesSkipped, minor there.
| // back in); stealing the request would consume this step as the | ||
| // compaction summary instead. | ||
| {stream: newStreamBuilder().AddStopWithUsage(1, 1).Build()}, | ||
| // The compaction summary call, drained by the newer stream's own |
There was a problem hiding this comment.
"drained by the newer stream's own iteration boundary" is now inaccurate: with the new stop rule the newer stream's content turn stops, so the request is drained at teardown (finishLiveSession), not at an iteration boundary. Either fix the comment or make this turn a tool-call turn as well.
| // reply) remains covered because it also has no tool calls. | ||
| // NOTE(krissetto): this can likely be removed once compaction works properly with all providers (aka dmr) | ||
| stoppedDueToNoOutput := strings.TrimSpace(fullContent.String()) == "" && len(toolCalls) == 0 | ||
| stoppedDueToNoOutput := len(toolCalls) == 0 |
There was a problem hiding this comment.
Minor: stoppedDueToNoOutput no longer matches its condition (no tool calls, content or not); stoppedNoToolCalls, or inlining the expression into the return, would be clearer. The comment could also be condensed to the invariant: a bare-EOF turn is terminal when no tool calls remain; the empty-content case (#3145) stays covered as a subset.
…erage, rename stoppedNoToolCalls
Addressed all three items from @Sayt-0's review in commit e42856c:
|
aheritier
left a comment
aheritier
left a comment
There was a problem hiding this comment.
🤖 Automated implementer agent — this comment was posted by the implementer bot from Docker Agentic Platform, not by a human developer
Re-review of PR #3669 — All three Sayt-0 items resolved ✅
I verified all three items from the prior CHANGES_REQUESTED review (Sayt-0) against commit e42856c. Verdict: APPROVE.
Item 1 (Blocking): Boundary coverage — TestCompactLiveSession_ExecutesAtIterationBoundary
Resolved ✅ — Turn 1 was converted from a bare-content stream to a tool-call stream (AddToolCallName / AddToolCallArguments / AddToolCallStopWithUsage). The same conversion was also applied to:
TestCompactLiveSession_DuplicateSessionIDsCompactOnlyTargetEntry(both older and newer stream turns)TestCompactLiveSession_HookVetoSynthesizesSkipped
All three tests now exercise the iteration-boundary drain path rather than the teardown drain path.
Item 2 (Blocking): Stale comment — "drained by the newer stream's own iteration boundary"
Resolved ✅ — The previous reviewer offered two options: fix the comment text, or make the newer-stream turn a tool-call turn. Option 2 was chosen. The newer stream in TestCompactLiveSession_DuplicateSessionIDsCompactOnlyTargetEntry now uses a tool-call turn (AddToolCallName("call_newer", ...) + AddToolCallStopWithUsage), so the stream genuinely does reach an iteration boundary before the compaction summary step. The comment is now factually accurate. The new inline comment documents the rationale: "A tool-call turn is used (matching the older stream) so the newer stream reaches an iteration boundary where the pending compaction can run."
Item 3 (Minor): Rename stoppedDueToNoOutput → stoppedNoToolCalls
Resolved ✅ — Both the variable declaration and its use in the return struct were renamed in streaming.go.
Core fix correctness
The logic change from:
stoppedDueToNoOutput := strings.TrimSpace(fullContent.String()) == "" && len(toolCalls) == 0to:
stoppedNoToolCalls := len(toolCalls) == 0is correct. The old guard keyed on empty content, so a content-bearing bare-EOF turn (no per-choice finish_reason) produced Stopped=false, causing runTurn to re-enter the model with identical messages and re-emit the same completion text forever. The new guard keys on the absence of tool calls: if there are no tool calls the outer loop has nothing to execute regardless of content, so marking the turn Stopped=true is the right invariant.
| Case | Stopped before | Stopped after | Correct? |
|---|---|---|---|
| Bare-EOF + content, no tool calls | false (bug) | true | Fixed |
| Bare-EOF + whitespace, no tool calls | true | true | Unchanged |
| Bare-EOF + tool calls | false | false | Unchanged |
| Explicit finish_reason, no tool calls | false | true | Semantically correct; caller uses FinishReason as primary signal |
New test TestHandleStream_ContentOnlyBareEOFStops
Precisely exercises the bug: real content, no tool calls, bare EOF (no terminal chunk), asserts Stopped=true. This is an exact regression guard for the described infinite-loop scenario.
No outstanding concerns
All blocking items resolved, core fix correct, regression test added, backward-compat cases preserved.
3 participants
Summary
Fixes #3668.
A sub-agent reached via
transfer_taskreturns control to its hub by finishing its turn: a final message with content and no tool call. When the provider closes that turn's SSE stream with a bare EOF -- no per-choicefinish_reason(common with OpenAI-compatible gateways such as litellm/CBORG, which emit only a terminal[DONE]sentinel) -- and the message has real content but no tool calls,handleStreamkeyed its stop decision on empty content and returnedStopped=false.runTurnthen re-entered the model with an unchanged message list and the model re-emitted the same completion forever, so the hand-off back to the hub never completed.The same defect applies to any turn that ends with a bare EOF and produces content but no tool calls; the
transfer_taskhub-and-spoke hand-off is where it is most visible, because a sub-agent's normal, correct way to return control is exactly such a turn.Change
Key the bare-EOF stop decision on the absence of tool calls, matching the mid-stream path (
Stopped: len(toolCalls) == 0):A turn on the bare-EOF path is terminal whenever no tool calls remain to execute, regardless of whether it produced content. The empty-content cases the original #3145 guard covered (token limit, whitespace-only or reasoning-only reply) stay covered, because those turns also have no tool calls. The
finishReasoninference below this line already distinguishes content-vs-no-content for reporting, so nothing else changes there.Tests
TestHandleStream_ContentOnlyBareEOFStops: a stream that emits real content and no tool calls, then closes with a bare EOF, must reportStopped=true. Complements the existingTestHandleStream_WhitespaceOnlyContentStops(the Stream silently produces empty response when model outputs non-standard delta.reasoning SSE field (Qwen3 thinking mode) #3145 case).TestCompactLiveSession_DuplicateSessionIDsCompactOnlyTargetEntry: its older stream relied on a content-only bare-EOF turn continuing to a second model call. That reflected the bug, not intended semantics. The turn is now a tool call so the older stream deterministically reaches its iteration boundary independent of the stop rule.Verification
gofmt,go vet ./pkg/runtime/,go build ./..., andgo test ./pkg/runtime/...all pass. (task/golangci-lintwere not available in my environment; CI will cover the full lint.)