Skip to content

fix(runtime): stop content-only turns that end with a bare EOF (no finish_reason) #3669

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
awschmeder wants to merge 2 commits into
base: main
Choose a base branch
from
+68 −19
Open

fix(runtime): stop content-only turns that end with a bare EOF (no finish_reason) #3669

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b82d83
fix(runtime): stop content-only turns that end with a bare EOF
awschmeder Jul 15, 2026
76cd025
fix(runtime): address Sayt-0 review: tool-call turns for boundary cov…
aheritier Jul 20, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 36 additions & 11 deletions pkg/runtime/live_sessions_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,9 +328,13 @@ func TestCompactLiveSession_ExecutesAtIterationBoundary(t *testing.T) {
started := make(chan struct{})
release := make(chan struct{})
prov := &stepProvider{id: "test/mock-model", steps: []providerStep{
// Turn 1: content without a finish reason keeps the loop running,
// so the queued request executes at the next iteration boundary.
{stream: newStreamBuilder().AddContent("working on it").Build(), started: started, release: release},
// Turn 1: a tool call keeps the loop running (Stopped=false), so the
// queued request executes at the next iteration boundary.
{stream: newStreamBuilder().
AddToolCallName("call_1", "unknown_tool").
AddToolCallArguments("call_1", "{}").
AddToolCallStopWithUsage(1, 1).
Build(), started: started, release: release},
// The compaction summary call.
{stream: newStreamBuilder().AddContent("a compact summary").AddStopWithUsage(10, 5).Build()},
// Turn 2: natural stop ends the stream.
Expand Down Expand Up @@ -446,14 +450,28 @@ func TestCompactLiveSession_DuplicateSessionIDsCompactOnlyTargetEntry(t *testing
startedB := make(chan struct{})
releaseB := make(chan struct{})
prov := &stepProvider{id: "test/mock-model", steps: []providerStep{
// Older stream turn 1: kept in flight while the newer stream
// registers under the same session ID.
{stream: newStreamBuilder().AddContent("older working").Build(), started: startedA, release: releaseA},
// Newer stream turn 1, gated so it stays live throughout.
{stream: newStreamBuilder().AddContent("newer working").Build(), started: startedB, release: releaseB},
// Older stream turn 1: a tool call keeps the stream live (the loop
// continues to execute the call) while the newer stream registers
// under the same session ID. A tool-call turn is used rather than a
// bare content turn so the older stream deterministically reaches a
// second model call regardless of the bare-EOF stop rule.
{stream: newStreamBuilder().
AddToolCallName("call_older", "unknown_tool").
AddToolCallArguments("call_older", "{}").
AddToolCallStopWithUsage(1, 1).
Build(), started: startedA, release: releaseA},
// Newer stream turn 1, gated so it stays live throughout. A tool-call
// turn is used (matching the older stream) so the newer stream reaches
// an iteration boundary where the pending compaction can run.
{stream: newStreamBuilder().
AddToolCallName("call_newer", "unknown_tool").
AddToolCallArguments("call_newer", "{}").
AddToolCallStopWithUsage(1, 1).
Build(), started: startedB, release: releaseB},
// Older stream turn 2: natural stop. With the request left alone this
// is the older stream's next model call; stealing the request would
// consume this step as the compaction summary instead.
// is the older stream's next model call (after the tool result feeds
// 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

@Sayt-0 Sayt-0 Jul 16, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"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.

// iteration boundary.
Expand Down Expand Up @@ -551,7 +569,14 @@ func TestCompactLiveSession_HookVetoSynthesizesSkipped(t *testing.T) {
started := make(chan struct{})
release := make(chan struct{})
prov := &stepProvider{id: "test/mock-model", steps: []providerStep{
{stream: newStreamBuilder().AddContent("working on it").Build(), started: started, release: release},
// Turn 1: tool call keeps the loop running (Stopped=false) so the
// hook veto fires at the iteration boundary, not at teardown.
{stream: newStreamBuilder().
AddToolCallName("call_1", "unknown_tool").
AddToolCallArguments("call_1", "{}").
AddToolCallStopWithUsage(1, 1).
Build(), started: started, release: release},
// Turn 2: natural stop (model call after the tool result, compaction vetoed).
{stream: newStreamBuilder().AddStopWithUsage(1, 1).Build()},
}}
rt := newLiveSessionsRuntime(t, prov, mockModelStoreWithLimit{limit: 100_000},
Expand Down
13 changes: 5 additions & 8 deletions pkg/runtime/streaming.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -359,14 +359,11 @@ mainLoop:

applyXMLFallback()

// If the stream completed without producing any usable content or tool
// calls, likely because of a token limit, stop to avoid breaking the request
// loop. Whitespace-only content counts as no output: it carries no answer,
// and runTurn's empty-turn detection uses the same trimmed-empty test, so
// treating it as stopped here guarantees that an empty-turn warning is always
// followed by a turn exit rather than an identical-message re-entry (#3145).
// Invariant: a bare-EOF turn (no per-choice finish_reason) is terminal
// whenever there are no tool calls — the outer loop has nothing to continue
// on. Turns with tool calls keep Stopped=false so the loop executes them.
// NOTE(krissetto): this can likely be removed once compaction works properly with all providers (aka dmr)
stoppedDueToNoOutput := strings.TrimSpace(fullContent.String()) == "" && len(toolCalls) == 0
stoppedNoToolCalls := len(toolCalls) == 0

@docker-agent docker-agent Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] Stale NOTE comment: now implies a permanent invariant is a removable workaround

The retained // NOTE(krissetto): this can likely be removed once compaction works properly was written when the variable was stoppedDueToNoOutput — a narrower, defensive workaround that stopped only when content was also empty. The PR correctly widened the condition to stoppedNoToolCalls := len(toolCalls) == 0, which encodes the correct semantic invariant for all bare-EOF turns: "nothing for the outer loop to continue on." This is no longer a temporary patch; it is the right behaviour regardless of whether compaction improves. Leaving the NOTE as-is invites a future reader to remove this line when compaction stabilises, which would silently reintroduce the re-entry loop for all content-only bare-EOF turns.

Consider replacing the NOTE with a brief comment explaining why this invariant is permanent:

Suggested change
stoppedNoToolCalls := len(toolCalls) == 0
stoppedNoToolCalls := len(toolCalls) == 0

Or updating the NOTE to reflect that the condition changed meaning, e.g.:

// This encodes the correct invariant for bare-EOF turns, not a temporary workaround.

Confidence Score
🟢 strong 100/100


// Prefer the provider's explicit finish reason when available (e.g.
// tool_calls). Only fall back to inference when no explicit reason was
Expand Down Expand Up @@ -399,7 +396,7 @@ mainLoop:
ReasoningContent: fullReasoningContent.String(),
ThinkingSignature: thinkingSignature,
ThoughtSignature: thoughtSignature,
Stopped: stoppedDueToNoOutput,
Stopped: stoppedNoToolCalls,
FinishReason: finishReason,
Usage: messageUsage,
}, nil
Expand Down
27 changes: 27 additions & 0 deletions pkg/runtime/streaming_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,33 @@ func TestHandleStream_WhitespaceOnlyContentStops(t *testing.T) {
"a whitespace-only, bare-EOF turn must stop so the empty-turn warning is followed by a turn exit, not an identical re-entry (#3145)")
}

// TestHandleStream_ContentOnlyBareEOFStops guards the loop that OpenAI-compatible
// gateways (litellm/CBORG) trigger: they close the SSE stream with a bare EOF and
// never send a per-choice finish_reason. A turn that produced real content but no
// tool calls has nothing left for the run loop to execute, so it must report
// Stopped=true. Keying the stop decision on empty content instead of "no tool
// calls" left such a final message with Stopped=false, so runTurn re-entered the
// model with identical messages and re-emitted the same completion text forever.
func TestHandleStream_ContentOnlyBareEOFStops(t *testing.T) {

@docker-agent docker-agent Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[low] Missing test for the symmetric bare-EOF+tool-calls path (Stopped=false)

TestHandleStream_ContentOnlyBareEOFStops (added here) tests the Stopped=true side of the new invariant. The symmetric case — a bare-EOF stream that does contain tool calls should return Stopped=false — is not directly exercised on the bare-EOF code path.

The existing TestHandleStream_ToolCallsAreExtracted and TestHandleStream_ToolCallThenSeparateStop both use AddToolCallStopWithUsage / AddToolCallWithStop, which emit a chunk with an explicit FinishReason: chat.FinishReasonToolCalls. That triggers the mid-stream early-return at line ~299 (Stopped: len(toolCalls) == 0), not the bare-EOF path at line 399. So a future bug that flips stoppedNoToolCalls to true when tool calls are present would not be caught by any existing test.

A companion test like:

func TestHandleStream_ToolCallsBareEOFContinues(t *testing.T) {
    stream := newStreamBuilder().
        AddToolCallName("call_1", "some_tool").
        AddToolCallArguments("call_1", `{}`).
        Build() // bare EOF — no finish_reason chunk
    // ... assert res.Stopped == false
}

would close this gap and make the invariant's two sides explicit.

Confidence Score
🟡 moderate 57/100

stream := newStreamBuilder().
AddContent("Frontmatter ingestion complete."). // real content, no tool calls
Build() // no terminal chunk: bare EOF, no finish reason

a := agent.New("root", "test", agent.WithModel(&mockProvider{id: "test/mock-model", stream: stream}))
sess := session.New(session.WithUserMessage("go"))

evCh := make(chan Event, 64)
res, err := handleStream(
t.Context(), nil, stream, a, nil, sess, nil,
defaultTelemetry{}, NewChannelSink(evCh), defaultStreamIdleTimeout,
)
require.NoError(t, err)

assert.Empty(t, res.Calls)
assert.True(t, res.Stopped,
"a content-bearing, bare-EOF turn with no tool calls must stop so the run loop exits instead of re-entering with identical messages")
}

// stalledStream is a chat.MessageStream that blocks in Recv() until
// either unblocked or the stream is closed. It is used to simulate a
// half-open TCP connection where the remote side stops sending data.
Expand Down
Loading