feat: add thinking delta - #59
Conversation
d2af863 to
8ce18f3
Compare
✅ Action performedReview finished.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds dedicated thinking deltas across Anthropic, Gemini, and OpenAI streams; exposes observer callbacks; refactors streaming into handler events; bundles API inputs in ChangesThinking Streaming and Request API
Sequence Diagram(s)sequenceDiagram
participant ProviderStreamEmitter
participant StreamHandler
participant BareLoop
participant ObserverHost
participant StreamAccumulator
ProviderStreamEmitter->>StreamHandler: emit DeltaPart::Thinking
StreamHandler->>BareLoop: yield HandlerEvent::Stream
BareLoop->>ObserverHost: on_thinking_delta(ThinkingDeltaContext)
BareLoop->>StreamAccumulator: ignore thinking in assembled Message
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/provider/anthropic.rs (1)
1242-1267: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
on_message_stopdoesn't close an open thinking part.The added
thinking_part_open/thinking_indexstate (lines 940-956) is closed symmetrically withtext_part_openinon_block_stop, buton_message_stop— which defensively closes any block still marked open before the terminalMessageStop— only checkstext_part_openandtool_parts_open. Ifmessage_stoparrives while a thinking block was never closed viacontent_block_stop(malformed/truncated stream), no matchingPartStopis emitted for that part index, leaving an unbalancedPartStart/PartStoppair for consumers.🔧 Proposed fix
fn on_message_stop(&mut self) { self.finished = true; if self.text_part_open { self.push(StreamEvent::PartStop); } + if self.thinking_part_open { + self.push(StreamEvent::PartStop); + } for _ in 0..self.tool_parts_open { self.push(StreamEvent::PartStop); } self.tool_parts_open = 0; self.text_part_open = false; + self.thinking_part_open = false; + self.thinking_index = None; self.push(StreamEvent::MessageStop); }🤖 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 `@src/provider/anthropic.rs` around lines 1242 - 1267, Update on_message_stop to defensively close an open thinking block using the existing thinking_part_open state, emitting its matching PartStop before closing tool blocks and sending MessageStop. Reset thinking_part_open consistently with text_part_open and preserve the documented PartStop ordering.
🤖 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.
Inline comments:
In `@src/engine/bare/stream.rs`:
- Around line 91-102: Update StreamHandler::stream_turn and its
stream_turn_via_handler caller to preserve observer callbacks when using a
StreamHandler, routing both text deltas and thinking deltas through the
configured ObserverHost/text streamer just like the inline path. Ensure
on_text_delta and on_thinking_delta are emitted without changing the existing
inline streaming behavior.
In `@src/provider/gemini.rs`:
- Around line 510-518: Update the generationConfig construction in the Gemini
request path to add thinkingConfig with includeThoughts enabled only when the
selected model supports thinking, rather than injecting it for every model.
Preserve the existing response_format and tool suppression behavior, and omit
thinkingConfig entirely for non-thinking models.
- Around line 817-892: The Gemini extract_parts and corresponding OpenAI
reasoning/text streaming paths must prevent interleaved lanes from clobbering
the single active accumulator state. Update both src/provider/gemini.rs lines
817-892 and src/provider/openai.rs lines 1082-1099 to emit a PartStop before
opening a PartStart for the other lane, or extend the shared accumulator to
track both lanes independently; preserve correct text, thinking, and tool-part
boundaries in both providers.
---
Outside diff comments:
In `@src/provider/anthropic.rs`:
- Around line 1242-1267: Update on_message_stop to defensively close an open
thinking block using the existing thinking_part_open state, emitting its
matching PartStop before closing tool blocks and sending MessageStop. Reset
thinking_part_open consistently with text_part_open and preserve the documented
PartStop ordering.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1ed809d3-71b7-44fd-b422-f49ef34e83f7
📒 Files selected for processing (9)
CHANGELOG.mdsrc/engine/bare.rssrc/engine/bare/stream.rssrc/observer.rssrc/observer/context.rssrc/provider/anthropic.rssrc/provider/gemini.rssrc/provider/openai.rssrc/stream.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/provider/gemini.rs (1)
963-1010: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winIterate all
parts[]when extracting a function call
extract_function_callstill hardcodes/candidates/0/content/parts/0/functionCall. If athought:truepart appears before the tool call in the same chunk, this silently misses the call and skips the new closing logic. Parse thepartsarray and find thefunctionCallentry instead of assuming index 0; there’s no test covering the mixedthought + functionCallshape.🤖 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 `@src/provider/gemini.rs` around lines 963 - 1010, Update extract_function_call to iterate candidates[0].content.parts and select the part containing functionCall instead of reading only parts[0]. Preserve the existing name/args extraction and lane-closing plus event-emission behavior once the function-call part is found, while doing nothing when no such part exists.
🤖 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.
Outside diff comments:
In `@src/provider/gemini.rs`:
- Around line 963-1010: Update extract_function_call to iterate
candidates[0].content.parts and select the part containing functionCall instead
of reading only parts[0]. Preserve the existing name/args extraction and
lane-closing plus event-emission behavior once the function-call part is found,
while doing nothing when no such part exists.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 744be8e7-30e3-4c75-b4ae-f96883d9c9ae
📒 Files selected for processing (19)
CHANGELOG.mdCargo.tomlsrc/api.rssrc/capabilities.rssrc/engine/bare.rssrc/engine/bare/dispatch.rssrc/engine/bare/stream.rssrc/observer.rssrc/observer/context.rssrc/provider/anthropic.rssrc/provider/gemini.rssrc/provider/grammar.rssrc/provider/openai.rssrc/reflection/llm.rssrc/runtime.rssrc/stream.rssrc/stream/handler.rssrc/structured.rssrc/testing.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- CHANGELOG.md
- src/stream.rs
- src/provider/openai.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CHANGELOG.md (1)
167-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRequire a wildcard arm for downstream
#[non_exhaustive]matches. Downstream crates still need_ =>when matchingDeltaPart; handlingThinkingexplicitly is optional, but it does not make the match exhaustive on its own.🤖 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 `@CHANGELOG.md` around lines 167 - 170, Update the CHANGELOG entry for stream::DeltaPart to state that downstream exhaustive matches require a wildcard (_) arm because the type is #[non_exhaustive]; clarify that explicitly handling Thinking is optional and does not replace the wildcard requirement.
🤖 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.
Outside diff comments:
In `@CHANGELOG.md`:
- Around line 167-170: Update the CHANGELOG entry for stream::DeltaPart to state
that downstream exhaustive matches require a wildcard (_) arm because the type
is #[non_exhaustive]; clarify that explicitly handling Thinking is optional and
does not replace the wildcard requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d14a9ced-ec43-4b14-8807-4e212a72a312
📒 Files selected for processing (2)
CHANGELOG.mdsrc/provider/gemini.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/provider/gemini.rs
1 participant
No description provided.