fix(openai): handle Ollama reasoning stream deltas#486
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes a compatibility gap in the OpenAI-compatible streaming adapter so Ollama “thinking” models that stream deltas in delta.reasoning are treated as active and their reasoning deltas are surfaced as reasoning stream events, preventing “no output” stalls.
Changes:
- Extend streamed
deltaparsing to acceptreasoningas an alias forreasoning_content. - Emit reasoning events using a helper that preserves
reasoning_contentprecedence when both fields are present. - Add unit tests covering alias emission and precedence behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| internal/providers/openai/types.go | Adds delta.reasoning to the streaming delta struct to support Ollama’s alias field. |
| internal/providers/openai/provider.go | Emits reasoning using a precedence helper so alias deltas produce StreamEventReasoning events. |
| internal/providers/openai/provider_test.go | Adds tests verifying alias deltas emit reasoning events (and not text) and that reasoning_content wins when both exist. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds reasoning alias handling in OpenAI streaming, propagates reasoning presence through collected streams, emits reasoning in CLI output, and treats reasoning-only turns as progress in agent guardrails. ChangesReasoning delta flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
The parsing change is correct — reasoning as an alias for reasoning_content, right precedence when both are present, emitted as a reasoning event so it's not mistaken for output. Tests are good.
But "Fixes #439" claims too much. This makes the thinking visible, which handles the "looks idle" half. The other half people hit — runs dying after 3 empty turns — comes from observeTurn in guardrails.go, which this doesn't touch: reasoning deltas go to OnReasoning and never get folded into the turn's text, so a reasoning-only turn still counts as empty, and three of them still trip maxEmptyTurns and stop with "no output". So merging this as-is would auto-close #439 while the main symptom's still there.
Either change "Fixes" to "Refs" so the issue stays open, or add the guardrail half (count a reasoning-bearing turn as live). The code itself is fine to merge — just don't let it close #439 yet.
Ashwinhegde19
force-pushed
the
fix-ollama-reasoning-alias
branch
from
815f744 to
27e1d0f
Compare
|
Updated, thanks for catching that. This now includes the guardrail half too: streamed reasoning deltas are recorded on the collected turn as liveness, and observeTurn resets the empty-turn counter when a turn has reasoning even if it has no visible answer text or tool call. I kept reasoning out of collected.Text, so it still does not become final-answer content. Added regression coverage for three reasoning-only turns followed by a final answer, plus a collector assertion that reasoning marks the turn as reasoning-bearing. Validated locally:
|
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P2] Surface reasoning deltas through headless stream-json output
internal/cli/exec.go:506
The linked repro for #439 useszero exec --output-format stream-json, but this PR only converts Ollama'sdelta.reasoninginto the runtimeStreamEventReasoning. The headless exec path still wires onlyOnTextinto the writer,execEventWriteronly emitstextevents, and the stream-json event enum has no reasoning event type, so stream-json clients will still see no live output while the model is thinking. Please add a stream-json reasoning/thinking event (and wireOnReasoningin the exec paths) or otherwise surface these deltas in headless output so the reported path stops looking idle. -
[P2] Get the linked issue approved before merging this community PR
CONTRIBUTING.md:19
The contribution policy says community PRs must be tied to an existing issue that has already been reviewed and approved by the core team, with approval shown by theissue-approvedlabel. This PR linksFixes #439, but #439 currently has no labels, so merging this would bypass the repository's issue-first gate for outside contributions. Please get #439 marked withissue-approvedor have a maintainer explicitly confirm an exception before this PR is merged.
Ashwinhegde19
force-pushed
the
fix-ollama-reasoning-alias
branch
from
27e1d0f to
26df701
Compare
|
Updated for the stream-json finding. The headless exec path now wires OnReasoning into execEventWriter, emits stream-json events with type reasoning and delta, and documents the event in docs/STREAM_JSON_PROTOCOL.md. The event is separate from text/final output, so reasoning stays liveness/progress only. Added coverage in TestRunExecStreamJSONEmitsReasoningEvents to verify stream-json receives reasoning before text and that reasoning is not folded into the final answer. Validated locally:
I also started make test-quick, but it produced no output for several minutes after the early packages and I interrupted it rather than leaving a hung process. The earlier full quick suite had passed before this stream-json-only follow-up. |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/cli/exec_protocol_test.go (1)
553-566: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winTest doesn't assert reasoning arrives before text.
findJSONEventjust scans for the first event of a given type — it can't detect ifreasoningandtextwere emitted out of order. Since the PR's whole premise is that reasoning deltas must be observed live (before/alongside text) to keep the turn "active," a test that only checks presence + delta value wouldn't catch an ordering regression (e.g., reasoning buffered and flushed only at the end). Consider asserting index positions ineventsdirectly.✅ Suggested ordering assertion
events := decodeJSONLines(t, stdout.String()) - reasoning := findJSONEvent(t, events, "reasoning") + reasoningIdx, reasoning := findJSONEventIndex(t, events, "reasoning") if reasoning["delta"] != "Thinking. " { t.Fatalf("unexpected reasoning event: %#v", reasoning) } - text := findJSONEvent(t, events, "text") + textIdx, text := findJSONEventIndex(t, events, "text") if text["delta"] != "done" { t.Fatalf("unexpected text event: %#v", text) } + if reasoningIdx >= textIdx { + t.Fatalf("expected reasoning event before text event, got indices %d, %d", reasoningIdx, textIdx) + } final := findJSONEvent(t, events, "final")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_protocol_test.go` around lines 553 - 566, The exec protocol test currently only verifies that reasoning and text events exist, but not that reasoning is emitted before text. Update the test around findJSONEvent/decodeJSONLines to assert the ordering directly from the events slice, using the reasoning and text event positions to ensure reasoning is observed first and not buffered until the end.internal/cli/exec_writer.go (1)
83-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPlain-text format silently swallows reasoning deltas.
text()falls through towriter.writeStdout(delta)for the default (non-JSON, non-stream-json) format, butreasoning()has no such fallback — reasoning deltas simply vanish for plainexecoutput. Given the whole point of this PR is to keep long-thinking models visibly "alive," plain-format users get zero indication anything is happening until the final answer lands. Worth at least a lightweight passthrough (or an explicit comment noting this is deliberate) so the omission doesn't look like an oversight.♻️ Optional: surface reasoning in plain mode
func (writer *execEventWriter) reasoning(delta string) { if writer.format == execOutputJSON { writer.writeJSON(map[string]any{"type": "reasoning", "delta": delta}) return } if writer.format == execOutputStreamJSON { writer.writeStreamJSON(streamjson.Event{Type: streamjson.EventReasoning, RunID: writer.runID, Delta: delta}) + return } + writer.writeStdout(delta) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/exec_writer.go` around lines 83 - 91, The execEventWriter.reasoning path only handles execOutputJSON and execOutputStreamJSON, so plain-text exec output drops reasoning deltas entirely. Update reasoning() to mirror text() by adding a non-JSON fallback (for example, forwarding delta to stdout through writeStdout) or add an explicit comment if the omission is intentional, and keep the behavior aligned with execEventWriter.text and execEventWriter.writeStdout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@internal/cli/exec_protocol_test.go`:
- Around line 553-566: The exec protocol test currently only verifies that
reasoning and text events exist, but not that reasoning is emitted before text.
Update the test around findJSONEvent/decodeJSONLines to assert the ordering
directly from the events slice, using the reasoning and text event positions to
ensure reasoning is observed first and not buffered until the end.
In `@internal/cli/exec_writer.go`:
- Around line 83-91: The execEventWriter.reasoning path only handles
execOutputJSON and execOutputStreamJSON, so plain-text exec output drops
reasoning deltas entirely. Update reasoning() to mirror text() by adding a
non-JSON fallback (for example, forwarding delta to stdout through writeStdout)
or add an explicit comment if the omission is intentional, and keep the behavior
aligned with execEventWriter.text and execEventWriter.writeStdout.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0a506f43-b2f6-407a-8f05-9390688a7c6b
📒 Files selected for processing (12)
docs/STREAM_JSON_PROTOCOL.mdinternal/agent/guardrails.gointernal/agent/guardrails_test.gointernal/cli/exec.gointernal/cli/exec_protocol_test.gointernal/cli/exec_writer.gointernal/providers/openai/provider.gointernal/providers/openai/provider_test.gointernal/providers/openai/types.gointernal/streamjson/streamjson.gointernal/zeroruntime/helpers.gointernal/zeroruntime/provider_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/agent/guardrails_test.go
- internal/zeroruntime/provider_test.go
- internal/providers/openai/provider.go
- internal/agent/guardrails.go
- internal/providers/openai/provider_test.go
- internal/providers/openai/types.go
- internal/zeroruntime/helpers.go
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
Thanks for the update. I rechecked the changed paths and found one issue that still needs to be addressed before this is ready.
Findings
- [P2] Complete the linked-issue approval before merging this community PR
CONTRIBUTING.md:19
The stream-json and guardrail fixes are now present, but the remaining contribution-policy blocker from the previous review is still unresolved: this PR linksFixes #439, and #439 still has noissue-approvedlabel. The policy requires community PRs to be tied to an existing issue that has already been reviewed and approved by the core team, with approval shown by that label. Please get #439 marked withissue-approved, or have a maintainer explicitly confirm an exception, before this PR is merged.
|
Updated in afd5d00 for CodeRabbit's ordering nitpick: TestRunExecStreamJSONEmitsReasoningEvents now asserts the reasoning event index is before the text event index, so the test catches buffering/order regressions instead of only checking presence.\n\nValidated locally:\n- go test ./internal/cli -count=1\n- git diff --check\n\nI intentionally left plain-text exec output silent for reasoning deltas. The existing behavior keeps reasoning separate from final/user-visible answer text; this PR's reported path is stream-json, and stream-json now receives live reasoning events. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving.
I flagged earlier that "Fixes #439" over-claimed — the parsing made thinking visible, but reasoning-only turns still counted as empty and tripped maxEmptyTurns, so the runs-dying-after-3-turns half was untouched. That's handled now: guardrails.go counts a reasoning-bearing turn as live, with a test that runs three thinking-only turns before a final answer and stays alive. So Fixes #439 holds up.
jatmn's stream-json point is covered too — there's a real reasoning event on the headless exec path now, emitted before text, kept out of the final answer, and documented as liveness-only.
I read the changed paths: reasoning_content precedence is preserved (the reasoning alias only wins when reasoning_content is empty), and the tests cover the alias, the precedence, the guardrail, and the exec stream-json ordering. Clean.
On the policy blocker: #439 is a real bug, so I've marked it issue-approved. The label didn't actually exist in the repo yet even though CONTRIBUTING.md references it — created it, so the gate is now satisfiable for the rest of the queue too.
Good to merge — over to kevin.
Fixes #439
Summary
delta.reasoningas an alias fordelta.reasoning_contentreasoning_contentprecedence when both fields are presentTests
go test ./internal/providers/openai -count=1make lintmake test-quickSummary by CodeRabbit
reasoningevents, and the CLI routes reasoning output during headlessexecruns.reasoning_content.reasoningevent behavior and examples.execStream-JSON reasoning ordering.