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 1 commit 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.
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.)