fix(ollama): surface thinking output as a ThinkingBlock by CryoThrust · Pull Request #3146 · agentscope-ai/agentscope-java

CryoThrust · 2026-09-14T10:49:15Z

Summary

bind message.thinking on OllamaMessage (response-only; Ollama's request schema has no such field)
emit a ThinkingBlock from OllamaResponseParser before the text block, matching the OpenAI and Anthropic modules

Both stream(...) and the blocking call go through the same parseResponse, so this covers streaming and non-streaming, and ReActAgent already turns a ThinkingBlock into the ThinkingBlock*Events. No core changes.

Tests

Added to OllamaResponseParserTest:

a raw Ollama JSON payload with message.thinking deserializes and produces ThinkingBlock then TextBlock (this is the end-to-end shape of the reported case)
a thinking-only chunk produces just the ThinkingBlock
a response without thinking is unchanged

Without the DTO/parser change these do not even compile against the new getter and the JSON case fails, so they are a real regression guard.

agentscope-extensions-model-ollama: 169 run, 0 failures, 1 skipped
spotless:check clean

Closes #3140.

Ollama's /api/chat returns a thinking model's reasoning in message.thinking, but OllamaMessage had no such field and OllamaResponseParser only built TextBlock and ToolUseBlock. The reasoning was silently discarded for both streaming and non-streaming requests, even with the think option enabled. Bind message.thinking on OllamaMessage and emit a ThinkingBlock before the text block, matching the OpenAI and Anthropic modules. Both response paths share the same parser, so a single change covers them; Ollama's request message schema has no thinking field, so the converter is left alone.

codecov · 2026-09-14T11:06:22Z

Codecov Report

❌ Patch coverage is 60.00000% with 6 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
...el/ollama/formatter/OllamaMultiAgentFormatter.java	40.00%	1 Missing and 2 partials ⚠️
...ns/model/ollama/formatter/OllamaChatFormatter.java	60.00%	0 Missing and 2 partials ⚠️
...s/model/ollama/formatter/OllamaResponseParser.java	50.00%	0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

oss-maintainer

Summary

OllamaMessage.thinking + a ThinkingBlock emitted ahead of the text block, mirroring the OpenAI/Anthropic modules. The change is correctly scoped to the Ollama extension, the block ordering matches the other providers, and stream(...)/blocking both go through parseResponse, so the fix covers both paths. No API breakage: the new DTO field is additive and @JsonInclude(NON_NULL) keeps it out of outbound payloads. CI is green on 9e1cd877 (build ubuntu/windows, Check License, Check Module Sync, codecov/patch). Verdict: good to go apart from the field-name coverage question below.

Findings

[Warning] OllamaMessage.java:42 — only thinking is bound; Ollama uses message.reasoning on some models/paths, which would keep the reported symptom alive for those users.
[Info] OllamaMessage.java:43 — response-only field on a DTO shared with the request path; safe today, worth a guard/note so it cannot leak outbound later.
[Info] OllamaResponseParser.java:54 — streaming yields one ThinkingBlock per chunk; the new tests only cover single-response parsing, so the delta-concatenation path is unguarded.

Suggestions

If reasoning fallback is out of scope, mentioning it in the PR description (or in a follow-up issue) is enough — the Closes #3140 claim should not be read as "all thinking-capable Ollama models now surface thinking".

oss-maintainer · 2026-09-14T11:42:56Z

+     * {@code think} option. Response-only: Ollama's request message schema does not accept it.
+     */
+    @JsonProperty("thinking")
+    private String thinking;


[Info] OllamaMessage is shared by the request and response paths, so a response-only field is easy to leak back outbound. @JsonInclude(NON_NULL) on the class hides it while it stays null, which is why this is safe today. Could you add a short note (or a guard) in the request formatter so a future change that copies a whole OllamaMessage into an outbound turn cannot start sending thinking back to /api/chat? That would make the "response-only" claim in the javadoc enforced rather than conventional.

Good catch. In a63902d2, I added an explicit sanitization pass in OllamaChatFormatter.buildRequest and OllamaMultiAgentFormatter.buildRequest that clears thinking (sets it to null) across all messages before the request payload is constructed. So even if an OllamaMessage is copied or reused from a prior response, thinking is never leaked outbound to /api/chat. Also added testBuildRequestClearsThinkingFromOutboundMessages in OllamaChatFormatterTest to verify this behavior.

oss-maintainer · 2026-09-14T11:42:56Z

+     * The model's reasoning content, returned by thinking models when the request enables the
+     * {@code think} option. Response-only: Ollama's request message schema does not accept it.
+     */
+    @JsonProperty("thinking")


[Warning] Ollama exposes reasoning text under two field names depending on the surface: message.thinking for /api/chat with think: true, and message.reasoning for some reasoning models (and in the OpenAI-compatible layer). Only thinking is bound here, so for those models the reasoning text is still silently dropped — the same symptom #3140 reports. Consider falling back to reasoning when thinking is absent, or state the supported model/API set explicitly in the PR description so the issue can be closed with a documented scope.

Added @JsonAlias({"reasoning", "reasoning_content"}) to OllamaMessage.thinking in a63902d2. This handles native /api/chat thinking as well as OpenAI-compatible proxies or models exposing reasoning or reasoning_content, surfacing all of them as a ThinkingBlock. Verified with testReasoningAliasesSurviveJsonDeserialization in OllamaResponseParserTest.

oss-maintainer · 2026-09-14T11:42:56Z

-        // 1. Handle Text Content
+        // 1. Handle Thinking Content (thinking models with the `think` option enabled)
+        if (msg != null && msg.getThinking() != null && !msg.getThinking().isEmpty()) {
+            contentBlocks.add(ThinkingBlock.builder().thinking(msg.getThinking()).build());


[Info] In streaming mode every chunk goes through parseResponse, so N thinking deltas become N separate ThinkingBlocks in N ChatResponses. Please confirm the downstream ReActAgent → ThinkingBlock*Event path concatenates them (the OpenAI/Anthropic modules have the same shape, so this is likely already handled) and consider adding one streaming-level test that feeds a multi-chunk thinking sequence — the three new tests are all single-response, so the fragmentation case is currently unguarded.

Traced the downstream path: in ReActAgent, streaming chunks go through ReasoningContext, which uses ThinkingAccumulator to concatenate incoming ThinkingBlock.getThinking() deltas into the full reasoning content (while emitting each block immediately for real-time events).

To guard against fragmentation regressions, I added testStreamingThinkingAccumulation in OllamaResponseParserTest (a63902d2), which simulates an Ollama multi-chunk delta sequence and verifies ThinkingAccumulator stitches the deltas into the complete string.

…test streaming thinking Address maintainer review feedback: - Bind 'reasoning' and 'reasoning_content' via @JsonAlias on OllamaMessage.thinking so proxies and reasoning models exposing reasoning text are captured. - Enforce that 'thinking' is cleared in buildRequest across chat and multi-agent formatters so response-only thinking content is never sent outbound. - Add multi-chunk streaming accumulation regression test using ThinkingAccumulator, matching the ReActAgent downstream event aggregation path.

jujn · 2026-09-19T04:09:22Z

bind message.thinking on OllamaMessage (response-only; Ollama's request schema has no such field)

请求测 OllamaMessage 不支持 thinking，这个结论是怎么得出的？查了下貌似 Ollama 的文档和源码不一致

oss-maintainer

Summary

Delta re-review of a63902d2 (previous round was approved at 9e1cd877). The three items from that round are genuinely addressed — @JsonAlias({"reasoning", "reasoning_content"}) closes the field-name gap, and testStreamingThinkingAccumulation covers the delta-concatenation path. CI is green on this head (build ubuntu/windows, Check License, Check Module Sync, codecov/patch) and license/cla is success.

What I want to slow down is the new outbound sanitization added in this commit. Its stated premise — "thinking is response-only; Ollama's request schema does not accept it" — does not hold against upstream, and that is the same question @jujn raised on the PR at 04:09 which is still unanswered:

api/types.go (ollama/ollama@main) declares Thinking string \json:"thinking,omitempty"`on theMessagestruct used forChatRequest.Messages— inbound history accepts it,omitempty` makes it optional rather than illegal.
docs/api.md → Generate a chat completion → Parameters → "The message object has the following fields" lists thinking.
docs/capabilities/tool-calling.md composes the follow-up request as messages.append({'role': 'assistant', 'thinking': thinking, 'content': content, 'tool_calls': tool_calls}) — thinking is deliberately echoed back for think: true + tools.

On top of that, the guard cannot bite today: request messages come from OllamaMessageConverter, which maps only TextBlock/ImageBlock/ToolUseBlock and never ThinkingBlock, so an outbound OllamaMessage.thinking is always null — which is exactly why @JsonInclude(NON_NULL) was already sufficient. Net effect: two new copies of a loop that changes nothing now, and encodes a rule that will discard reasoning content the moment thinking is threaded through history.

Findings

[Warning] OllamaMessage.java:43 — "request schema does not accept it" is contradicted by api/types.go and docs/api.md; reword, since the guard's rationale depends on it.
[Warning] OllamaChatFormatter.java:455 — no-op today, wrong direction against the documented tool-call continuation flow; also mutates the caller's message list in place.
[Info] OllamaMultiAgentFormatter.java:303 — second copy of the same loop; one helper if it stays.
[Info] OllamaChatFormatterTest.java:543 — asserts the in-place mutation, not the serialized payload.

Suggestions

Either drop the guard and reword the DTO note, or keep it scoped (skip assistant messages carrying tool_calls) with a comment citing the upstream doc. Then pin the behaviour in the test that actually matters — serialize the built request and assert no "thinking" key appears — so the intent survives future edits. Nothing here touches the parser change itself, which looks right: ThinkingBlock before TextBlock, shared by streaming and blocking paths, no core impact.

Holding this as a comment rather than a re-approval until @jujn's question is resolved — the reasoning-block emission is fine, but the commit's stated invariant is not, and it is the part a future reader will trust.

Automated review by github-manager-bot

oss-maintainer · 2026-09-19T05:51:44Z

+     * The model's reasoning content, returned by thinking models when the request enables the
+     * {@code think} option. In native Ollama {@code /api/chat} this is returned in {@code thinking};
+     * some reasoning models or proxies expose it under {@code reasoning} or {@code reasoning_content}.
+     * Response-only: Ollama's request message schema does not accept it.


[Warning] Response-only: Ollama's request message schema does not accept it is not accurate, and it is the premise behind the new outbound strip in a63902d2 — it is also exactly the question @jujn left on this PR, which is still open.

Upstream accepts thinking inbound:

api/types.go (ollama/ollama@main): Thinking string json:"thinking,omitempty"`` sits on the same Message struct used for `ChatRequest.Messages`, i.e. request history — `omitempty` means optional, not rejected.

docs/api.md → Generate a chat completion → Parameters → "The message object has the following fields": thinking: (for thinking models) the model's thinking process.

docs/capabilities/tool-calling.md builds the follow-up request as messages.append({'role': 'assistant', 'thinking': thinking, 'content': content, 'tool_calls': tool_calls}) — the accumulated reasoning is deliberately echoed back.

Suggested wording: the Java request path does not populate it today, rather than the wire schema rejecting it.

oss-maintainer · 2026-09-19T05:51:44Z

+        if (messages != null) {
+            for (OllamaMessage msg : messages) {
+                if (msg != null && msg.getThinking() != null) {
+                    msg.setThinking(null);


[Warning] This is a guard against a case that cannot happen today, and it blocks the case that upstream recommends.

Today it is a no-op on the real path: request messages are built by OllamaMessageConverter, which maps only TextBlock / ImageBlock / ToolUseBlock — a ThinkingBlock from a prior turn is already dropped during conversion, so an outbound OllamaMessage never carries thinking (which is also why @JsonInclude(NON_NULL) alone was enough). So the guard adds a rule, not a fix.

The rule it encodes is the wrong direction: per docs/capabilities/tool-calling.md, for think: true + tools the assistant thinking should travel back with tool_calls in the follow-up request. If someone later wires ThinkingBlock into the history (the natural next step for thinking models), this loop will silently discard it and the model loses its own reasoning across the tool round-trip.

Options, any is fine:

Remove the loop and instead make the intent explicit where it belongs — keep ThinkingBlock out of OllamaMessageConverter, or map it and let it go outbound — plus reword the OllamaMessage note (see the comment on that line);

Keep it but scope it (skip assistant messages that carry tool_calls) and reference the upstream doc so the next reader does not widen it.

Non-blocking either way: setThinking(null) mutates the caller's OllamaMessage objects in place, so a list reused across a retry or another provider is silently rewritten. Building sanitized copies keeps buildRequest pure.

oss-maintainer · 2026-09-19T05:51:44Z

+        if (messages != null) {
+            for (OllamaMessage msg : messages) {
+                if (msg != null && msg.getThinking() != null) {
+                    msg.setThinking(null);


[Info] Same loop, second copy. If the guard stays at all, one shared helper (or a single call in OllamaChatModel before buildRequest) is enough — the two formatters otherwise drift, and a future fix (e.g. scoping it for tool-call continuations) has to be applied twice.

oss-maintainer · 2026-09-19T05:51:44Z

+
+        assertNotNull(request);
+        assertEquals(2, request.getMessages().size());
+        assertNull(request.getMessages().get(1).getThinking());


[Info] request.getMessages() is the very list that was handed to buildRequest, so assertNull(...) pins the in-place mutation instead of the wire format — it would still pass if the serializer emitted thinking. For the intent stated in the comment, assert on the payload: serialise the built request (the same codec the transport uses) and assert the JSON has no "thinking" key. If the tool-call continuation case is meant to keep thinking, that positive case needs an assertion too — right now no test distinguishes "never sent" from "dropped by mistake".

CryoThrust mentioned this pull request Sep 14, 2026

[Bug]: OllamaChatModel drops thinking output — OllamaMessage DTO has no "thinking" field, so reasoning is silently discarded #3140

Open

oss-maintainer reviewed Sep 14, 2026

View reviewed changes

This comment was marked as abuse.

Sign in to view

oss-maintainer reviewed Sep 19, 2026

View reviewed changes

Conversation

CryoThrust commented Sep 14, 2026

Summary

Tests

Uh oh!

codecov Bot commented Sep 14, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oss-maintainer left a comment

Choose a reason for hiding this comment

Summary

Findings

Suggestions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment was marked as abuse.

Uh oh!

jujn commented Sep 19, 2026

Uh oh!

oss-maintainer left a comment

Choose a reason for hiding this comment

Summary

Findings

Suggestions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Sep 14, 2026 •

edited

Loading