fix(streaming): rationalize thinking retrieval vs model budget by arimu1 · Pull Request #1853 · embabel/embabel-agent

arimu1 · 2026-08-01T07:00:11Z

Summary

Fixes #1799 — rationalize thinking in streaming mode.

Streaming previously gated thinking format instructions on LlmOptions.thinking.enabled (typically only set via Thinking.withTokenBudget(...)). That made createObjectStreamWithThinking feel like it required a token budget as a prerequisite, even though retrieving thinking events and configuring a model thinking budget are separate concerns (as discussed with @jorander on the issue).

Changes

createObjectStreamWithThinking always enables thinking format instructions so callers get reasoning blocks without pre-configuring a budget.
createObjectStream never injects thinking format instructions (object-only path; avoids pushing models to think and then discarding the content — consistent with StreamingJacksonOutputConverter pushes models to think even when thinking is disabled #1685/StreamingJacksonOutputConverter.getFormat() respects disabled thinking #1687 intent for non-thinking streams).
API layer (OperationContextDelegate): enable extraction for the with-thinking stream while preserving any existing token budget via Thinking.applyExtraction().
Docs (streaming/page.adoc): document retrieval vs model budget, with optional Anthropic-style budget example.

Model thinking budget remains optional and independent:

// Optional — only when the provider needs a budget (e.g. Anthropic extended thinking)
LlmOptions options = new LlmOptions().withThinking(Thinking.withTokenBudget(8000));

No auto-default token budget and no warning log (per maintainer feedback on #1799).

AI assistance

Assisted by AI (Grok). Human author: arimu1.

Test plan

StreamingChatClientOperationsTest (25 tests) including:
- createObjectStreamWithThinking includes thinking format without LlmOptions thinking config
- createObjectStream omits thinking format instructions
JDK 21

export JAVA_HOME=.../jdk-21.0.12+8/Contents/Home
mvn -pl embabel-agent-api -am -Dkotlin.compiler.daemon=false \
  -Dmaven.gitcommitid.skip=true -Dsurefire.failIfNoSpecifiedTests=false \
  -Dtest=StreamingChatClientOperationsTest test

jorander

@arimu1 Thanks for the contribution. My comments mainly concern the documentation.

igordayen

@arimu1 - thank you for contributing! few comments to consider. best regrds

igordayen · 2026-08-01T21:24:18Z

That made createObjectStreamWithThinking feel like it required a token budget as a prerequisite, even though retrieving thinking events and configuring a model thinking budget are separate concerns (as discussed with @jorander on the issue).

agree 100%

arimu1 · 2026-08-02T02:35:28Z

Addressed review feedback and rebased onto current main:

Docs: distinguish application-level thinking (prompt format instructions → StreamingEvent.Thinking) from LLM-native reasoning (provider channels, not extracted today — Make thinking-content available in both blocking and streaming calls through ThinkingResponse/StreamingEvent #1716).
Renamed streamingInteractionForThinking → streamingInteractionForThinkingIfNecessary.
Clarified thinking-format comments in StreamingChatClientOperations.
Test helper uses CapturingSlot without FQCN.
Kept upstream “Streaming with Tools” section and resolved the docs conflict.

Happy to iterate further if anything still looks off.

igordayen

@arimu1 - thank you. started with larger design, please take a look and provide your comments.

And please confirm whether you are planning to execute IT test.
Also: please mark as "resolved" this outstanding inquiries - im referring to only ones that are actully resolved.

Thanks

arimu1 · 2026-08-02T10:10:04Z

Thanks @jorander and @igordayen — summarizing how the latest tip addresses the review threads:

Docs (jorander / igordayen)

The streaming docs now explicitly separate:

Application-level thinking — prompt format instructions + StreamingEvent.Thinking extraction (what this PR covers)
LLM-native reasoning — provider channels (Anthropic extended thinking, etc.), not extracted by Embabel today → tracked in Make thinking-content available in both blocking and streaming calls through ThinkingResponse/StreamingEvent #1716

Model thinking budget (LlmOptions.withThinking(Thinking.withTokenBudget(...))) is documented as optional and independent of retrieving thinking events.

Naming (igordayen)

Helper renamed to streamingInteractionForThinkingIfNecessary()
Code comments clarify prompt-instructed / application-level thinking vs native provider channels

IT tests

I am not planning a full multi-provider IT in this PR — coverage is unit-level (ThinkingFormatInstructionTests / stream path tests) plus the docs clarification. Happy to add a focused IT if you want a specific provider fixture called out.

Review threads

Please take another look at tip 9c14b2908. I will mark threads resolved once you confirm the docs wording matches the intended design split with #1716.

arimu1 · 2026-08-02T10:42:39Z

@jorander @igordayen Follow-up: all review threads are now resolved (including the FYI ping). Tip remains de94e383d with:

app-level vs LLM-native thinking split in streaming docs (Make thinking-content available in both blocking and streaming calls through ThinkingResponse/StreamingEvent #1716 for native)
streamingInteractionForThinkingIfNecessary rename
no FQCN in the test helper
unit coverage only (no multi-provider IT in this PR)

Happy to adjust further if anything still looks off after re-review.

arimu1 · 2026-08-02T15:54:45Z

Fixed DCO: tip commit 613fd1568 now has Signed-off-by. Both commits on the PR are signed off. Ready for re-review when convenient.

igordayen · 2026-08-02T17:57:22Z

@arimu1 - thank you for contributing. May I suggest? Regarding the introduction of the "thinking format" parameter.
The intent is to auto-enable prompt-level thinking in thinking APIs, correct?
Would not Thinking. enabled turning on Interaction be sufficient?
Concerned with:

attribute name, should it reflect semantics more clearly?
propagation across the chain of calls
Thank you

Remove the separate includeThinkingFormat SPI parameter. Application-level prompt format now follows Thinking.extractThinking on the Interaction (enabled via withExtraction/applyExtraction); provider tokenBudget remains independent (Thinking.enabled). WithThinking paths ensure extractThinking is set so format propagates through Interaction only. Addresses igordayen design feedback on embabel#1853 / embabel#1799. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>

arimu1 · 2026-08-03T01:05:33Z

@igordayen Thanks for the design pushback — you were right that a separate SPI includeThinkingFormat parameter was the wrong shape.

Answer to your questions

Intent: yes — auto-enable application-level (prompt-instructed) thinking for *WithThinking streaming APIs, without requiring a provider token budget.

Is Thinking on the Interaction enough? Yes. Pushed tip 4d2dca12a (rebased on current main):

Removed the separate includeThinkingFormat parameter.
Format instructions now follow Thinking.extractThinking on the Interaction (set via Thinking.withExtraction() / applyExtraction()).
createObjectStreamWithThinking (API + SPI) ensures extractThinking is set on the Interaction so the flag propagates through the call chain — no parallel SPI switch.
Provider budget stays on Thinking.enabled + tokenBudget (Thinking.withTokenBudget(...)) and is independent of application-level extraction.

Why `extractThinking` rather than `Thinking.enabled`?

On Thinking:

withExtraction() → extractThinking=true, enabled stays false
withTokenBudget(n) → enabled=true + budget, extractThinking stays false

So if format were gated on enabled alone, budget-only config would inject prompt format, and extraction-only config (what non-streaming thinking APIs already use) would not. Gating format on extractThinking matches the existing non-streaming thinkingInteraction() pattern and keeps provider budget orthogonal.

Propagation

createObjectStreamWithThinking
  → streamingInteractionForThinkingIfNecessary()  // API: withExtraction / applyExtraction
  → SPI withApplicationLevelThinkingIfNecessary() // same if SPI called directly
  → StreamingJacksonOutputConverter(thinkingEnabled = interaction.llm.thinking?.extractThinking)

Tests: StreamingChatClientOperationsTest 25/25 green (JDK 21). Happy to rename helpers if you prefer a different name.

Ready for re-review when convenient.

igordayen · 2026-08-03T02:46:34Z

@arimu1 - thank you for moving this forward.

Could you please consider adding a test for this into LLMOllamaStreamingBuilderIT.java.
Thanks

createObjectStreamWithThinking always enables thinking format instructions and extraction, independent of Thinking.withTokenBudget. Object-only streams omit thinking format. Document the split. Fixes embabel#1799. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>

…nsTest Address review feedback to use imports instead of fully qualified names. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>

Remove the separate includeThinkingFormat SPI parameter. Application-level prompt format now follows Thinking.extractThinking on the Interaction (enabled via withExtraction/applyExtraction); provider tokenBudget remains independent (Thinking.enabled). WithThinking paths ensure extractThinking is set so format propagates through Interaction only. Addresses igordayen design feedback on embabel#1853 / embabel#1799. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>

Add LLMOllamaStreamingBuilderIT coverage for createObjectStreamWithThinking without tokenBudget, with tokenBudget (applyExtraction), and object-only stream with budget alone — validates application-level extractThinking is independent of provider Thinking.withTokenBudget (embabel#1799 / embabel#1853). Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>

arimu1 · 2026-08-03T23:33:52Z

@igordayen Thanks — added the IT coverage in LLMOllamaStreamingBuilderIT.java (tip b497c7de3, rebased on current main).

New test: `createObjectStreamWithThinkingRetrievalIndependentOfTokenBudget`

Mirrors the existing reactive IT style and exercises the retrieval-vs-budget split from this PR:

Path A — createObjectStreamWithThinking without any Thinking / tokenBudget
Application-level extractThinking (and format instructions) come from the *WithThinking path alone; stream completes and yields object events.
Path B — createObjectStreamWithThinking with Thinking.withTokenBudget(100)
Provider budget is preserved; applyExtraction still enables thinking retrieval. Stream completes with object events.
Path C — object-only createObjectStream with tokenBudget alone
Budget does not require thinking events; typed object stream completes (budget orthogonal to application-level extraction).

Unit coverage remains in StreamingChatClientOperationsTest (25/25, JDK 21) for prompt-format injection (<think> with extractThinking / omitted for budget-only). Live Ollama IT matches neighbor patterns (same streaming-test profile / no env gate).

Ready for re-review when convenient.

igordayen · 2026-08-04T02:18:32Z

3. Path C — object-only createObjectStream with tokenBudget alone
Budget does not require thinking events; typed object stream completes (budget orthogonal to application-level extraction).

==> so path C is the only negative test then, thanks

igordayen

@guanxuc - getting better, thank you, some code maintenance would be required.

igordayen · 2026-08-04T02:19:59Z

+    private fun streamingInteractionForThinkingIfNecessary(): LlmInteraction {
+        val base = streamingInteraction()
+        val thinking = when (val existing = llm.thinking) {
+            null, Thinking.NONE -> Thinking.withExtraction()


Could you please explain the logic and how it actually behaves

igordayen · 2026-08-04T02:21:02Z

+     */
+    private fun withApplicationLevelThinkingIfNecessary(interaction: LlmInteraction): LlmInteraction {
+        val existing = interaction.llm.thinking
+        val thinking = when (existing) {


Can dup code be avoided?

igordayen · 2026-08-04T02:22:39Z

        val chatOptions = requireSpringAiLlm(llm).convertOptions(interaction.llm)

+        // Application-level thinking format: Thinking.extractThinking (not provider tokenBudget / enabled).
+        val includeApplicationLevelThinking = interaction.llm.thinking?.extractThinking == true


do you need this "val" , or just interaction.llm.thinking?.extractThinking == true use as is

igordayen · 2026-08-04T02:23:30Z

+    private fun withApplicationLevelThinkingIfNecessary(interaction: LlmInteraction): LlmInteraction {
+        val existing = interaction.llm.thinking
+        val thinking = when (existing) {
+            null, Thinking.NONE -> Thinking.withExtraction()


3rd occurrence of dup code

arimu1 · 2026-08-04T05:44:22Z

@guanxuc @igordayen Thanks — please go ahead with the shared withApplicationLevelThinkingIfNecessary extraction / inline / comment cleanup. I will not push overlapping edits on this PR so you can own the maintenance pass without conflicts.

guanxuc · 2026-08-04T05:51:18Z

@guanxuc - getting better, thank you, some code maintenance would be required.

Thanks @igordayen — I'll pick up the remaining code maintenance items:

Extracted Thinking.ensureExtraction() to eliminate 3 duplicate implementations
Added LlmInteraction.withApplicationLevelThinking() extension function
Inlined unnecessary includeApplicationLevelThinking val

Hi @igordayen , @arimu1 ,

I've pushed commit guanxuc@6a4434f to address the code maintenance items raised in the review.

Unfortunately, I don't have push access to @arimu1's fork, so I've pushed it to my own fork instead:
https://github.com/guanxuc/embabel-agent/commits/fix/1799-streaming-thinking-rationalize
Cloud you please take a look? Thank you!

guanxuc · 2026-08-04T06:04:53Z

@guanxuc @igordayen Thanks — please go ahead with the shared withApplicationLevelThinkingIfNecessary extraction / inline / comment cleanup. I will not push overlapping edits on this PR so you can own the maintenance pass without conflicts.

Hi @arimu1,

Since I don't have push access to your fork, I'll open a PR from my fork targeting your branch if the commit guanxuc@6a4434f looks good.

igordayen · 2026-08-04T14:32:58Z

@arimu1 @guanxuc I'm a bit confused about ownership from your end.
Please keep it on hold this week, and I appreciate your patience and cooperation. will resume tracking after release.
Addressing the items related to the upgrade this week, thanks for your understanding.

guanxuc · 2026-08-04T14:59:01Z

@arimu1 @guanxuc I'm a bit confused about ownership from your end. Please keep it on hold this week, and I appreciate your patience and cooperation. will resume tracking after release. Addressing the items related to the upgrade this week, thanks for your understanding.

Hi @igordayen ,

Got it, thanks for letting us know! We’ll pause this week and resume after the release. Good luck with the upgrade 🚀!

For clarity, please note that ownership of this PR rests with @arimu1.

jorander requested changes Aug 1, 2026

View reviewed changes

Comment thread ...kotlin/com/embabel/agent/spi/support/springai/streaming/StreamingChatClientOperationsTest.kt Outdated

Comment thread embabel-agent-docs/src/main/asciidoc/reference/streaming/page.adoc Outdated

jorander requested a review from igordayen August 1, 2026 08:15

igordayen reviewed Aug 1, 2026

View reviewed changes

arimu1 force-pushed the fix/1799-streaming-thinking-rationalize branch from ace908d to 9c14b29 Compare August 2, 2026 02:33

igordayen reviewed Aug 2, 2026

View reviewed changes

arimu1 force-pushed the fix/1799-streaming-thinking-rationalize branch from de94e38 to 613fd15 Compare August 2, 2026 15:46

arimu1 force-pushed the fix/1799-streaming-thinking-rationalize branch from 613fd15 to 4d2dca1 Compare August 3, 2026 01:05

arimu1 added 4 commits August 4, 2026 06:30

test(streaming): drop unnecessary FQNs in StreamingChatClientOperatio…

2dc25c0

…nsTest Address review feedback to use imports instead of fully qualified names. Signed-off-by: arimu1 <19286898+arimu1@users.noreply.github.com>

arimu1 force-pushed the fix/1799-streaming-thinking-rationalize branch from 4d2dca1 to b497c7d Compare August 3, 2026 23:33

igordayen reviewed Aug 4, 2026

View reviewed changes

igordayen mentioned this pull request Aug 8, 2026

feat(thinking): add tag selection API with include/exclude filtering (#1790) #1795

Open

Uh oh!

Conversation

arimu1 commented Aug 1, 2026

Summary

Changes

AI assistance

Test plan

Uh oh!

jorander left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

igordayen left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

igordayen commented Aug 1, 2026

Uh oh!

arimu1 commented Aug 2, 2026

Uh oh!

igordayen left a comment

Choose a reason for hiding this comment

Uh oh!

arimu1 commented Aug 2, 2026

Docs (jorander / igordayen)

Naming (igordayen)

IT tests

Review threads

Uh oh!

arimu1 commented Aug 2, 2026

Uh oh!

arimu1 commented Aug 2, 2026

Uh oh!

igordayen commented Aug 2, 2026

Uh oh!

arimu1 commented Aug 3, 2026

Answer to your questions

Why extractThinking rather than Thinking.enabled?

Propagation

Uh oh!

igordayen commented Aug 3, 2026

Uh oh!

arimu1 commented Aug 3, 2026

New test: createObjectStreamWithThinkingRetrievalIndependentOfTokenBudget

Uh oh!

igordayen commented Aug 4, 2026

Uh oh!

igordayen left a comment

Choose a reason for hiding this comment

Uh oh!

igordayen Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

igordayen Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

igordayen Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

igordayen Aug 4, 2026

Choose a reason for hiding this comment

Uh oh!

arimu1 commented Aug 4, 2026

Uh oh!

guanxuc commented Aug 4, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guanxuc commented Aug 4, 2026

Uh oh!

igordayen commented Aug 4, 2026

Uh oh!

guanxuc commented Aug 4, 2026

Uh oh!

Reviewers

Assignees

Why `extractThinking` rather than `Thinking.enabled`?

New test: `createObjectStreamWithThinkingRetrievalIndependentOfTokenBudget`

igordayen Aug 4, 2026 •

edited

Loading

guanxuc commented Aug 4, 2026 •

edited

Loading