feat: streaming dep optional, non streaming path, streaming flag, rel… - #65
feat: streaming dep optional, non streaming path, streaming flag, rel…#65bobrykov wants to merge 1 commit into
Conversation
|
@CodeRabbit review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughChangesTurn execution and feature configuration
Sequence Diagram(s)sequenceDiagram
participant User
participant chat
participant BareLoop
participant ProviderClient
User->>chat: Set NO_STREAM=1
chat->>BareLoop: Set TurnMode::NonStreaming
BareLoop->>ProviderClient: Send create_message request
ProviderClient-->>BareLoop: Return assembled response
BareLoop-->>chat: Return completed turn
chat-->>User: Print assembled response
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 5
🧹 Nitpick comments (3)
src/engine/bare.rs (2)
2708-2716: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the test to match what it asserts.
turn_mode_default_is_nonstreaming_without_featureasserts both branches:NonStreamingwithout the feature andStreamingwith it. The name describes only one branch.📝 Proposed rename
- fn turn_mode_default_is_nonstreaming_without_feature() { + fn turn_mode_default_follows_streaming_feature() {🤖 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/engine/bare.rs` around lines 2708 - 2716, Rename the test function turn_mode_default_is_nonstreaming_without_feature to reflect that it verifies the default turn mode for both streaming and non-streaming feature configurations. Keep the assertions and test behavior unchanged.
2469-2494: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared mock response assembly.
MockClient::create_messagehere andRecordingClient::create_messageat lines 6062-6092 contain the same event-to-response assembly: pop queued events, track the lastMessageDeltastop reason, feed each event toStreamAccumulator, then build theNonStreamingResponse. The two copies will drift as the accumulator contract evolves.Extract one test helper, for example
fn assemble_response(events: Vec<StreamEvent>) -> Result<NonStreamingResponse, ApiError>, and call it from both clients.🤖 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/engine/bare.rs` around lines 2469 - 2494, Extract the duplicated event-to-response logic from MockClient::create_message and RecordingClient::create_message into a shared test helper such as assemble_response. The helper should accept queued StreamEvent values, track the latest MessageDelta stop reason, process events through StreamAccumulator, and return the constructed NonStreamingResponse or ApiError; update both client methods to call it after popping their events.src/provider.rs (1)
130-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCollapse the repeated provider gate into one named feature.
The predicate
any(feature = "openai", feature = "anthropic", feature = "gemini")now appears at lines 58, 62, 64, 130, 198, 213, 326, and 333. Each new provider requires editing every occurrence, and a missed site produces a feature-combination build break rather than a visible error.Add an internal aggregate feature in
Cargo.toml, for example_http_provider = [], haveopenai,anthropic, andgeminienable it, then gate on#[cfg(feature = "_http_provider")]at each site.Also applies to: 198-198, 213-213, 326-333
🤖 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.rs` at line 130, Replace the repeated provider cfg predicate in src/provider.rs, including the sites around provider definitions and HTTP-provider code, with #[cfg(feature = "_http_provider")]. Define the internal _http_provider aggregate feature in Cargo.toml and update the openai, anthropic, and gemini features to enable it, preserving their existing feature behavior.
🤖 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 `@Cargo.toml`:
- Around line 99-104: Remove panic = "abort" from the [profile.release]
configuration so dispatch_tool’s catch_unwind-based tool-panic isolation remains
effective in release builds. Preserve the other release profile settings
unchanged.
In `@examples/chat.rs`:
- Around line 336-353: Update the runtime mode setup around `no_stream` so it is
true whenever the `streaming` feature is unavailable, ensuring the later
result-printing logic displays non-streaming responses. In the feature-enabled
live-streaming branch, explicitly select `TurnMode::Streaming` before
configuring the text streamer, while preserving the existing `NO_STREAM=1`
non-streaming behavior.
In `@README.md`:
- Around line 170-175: Update the README paragraphs describing streaming so they
distinguish feature availability from runtime mode selection: state that the
streaming feature enables TurnMode::Streaming, but turns use it only when
selected, including via set_turn_mode, while TurnMode::NonStreaming remains
available. Apply the same clarification to the referenced paragraph.
In `@src/engine/bare.rs`:
- Around line 122-134: Resolve the conflicting defaults for TurnMode by removing
its derived Default implementation and relying solely on default_turn_mode() for
the feature-dependent constructor default. Update the associated public
documentation to describe this single default contract accurately, while
preserving the enum variants and mode-switching behavior.
- Around line 136-144: Update the documentation for the cfg-gated Streaming
variant to remove the claim that disabling the streaming feature produces a
LoopError::Config at turn time, while retaining only the accurate streaming
feature requirement.
---
Nitpick comments:
In `@src/engine/bare.rs`:
- Around line 2708-2716: Rename the test function
turn_mode_default_is_nonstreaming_without_feature to reflect that it verifies
the default turn mode for both streaming and non-streaming feature
configurations. Keep the assertions and test behavior unchanged.
- Around line 2469-2494: Extract the duplicated event-to-response logic from
MockClient::create_message and RecordingClient::create_message into a shared
test helper such as assemble_response. The helper should accept queued
StreamEvent values, track the latest MessageDelta stop reason, process events
through StreamAccumulator, and return the constructed NonStreamingResponse or
ApiError; update both client methods to call it after popping their events.
In `@src/provider.rs`:
- Line 130: Replace the repeated provider cfg predicate in src/provider.rs,
including the sites around provider definitions and HTTP-provider code, with
#[cfg(feature = "_http_provider")]. Define the internal _http_provider aggregate
feature in Cargo.toml and update the openai, anthropic, and gemini features to
enable it, preserving their existing feature behavior.
🪄 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: 6a114cd1-01ed-41f8-943b-b6b87b293fa5
📒 Files selected for processing (17)
CHANGELOG.mdCargo.tomlREADME.mdexamples/chat.rssrc/capabilities.rssrc/engine/bare.rssrc/engine/bare/compact.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/bare/stream.rssrc/managers.rssrc/observer.rssrc/provider.rssrc/reflection/llm.rssrc/stream.rssrc/stream/rate_limit.rssrc/structured.rs
| [profile.release] | ||
| lto = "fat" | ||
| codegen-units = 1 | ||
| strip = true | ||
| panic = "abort" | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
panic = "abort" disables the tool-panic isolation in dispatch_tool.
src/engine/bare/dispatch.rs wraps each tool invocation in AssertUnwindSafe(...).catch_unwind() and converts a panic into a soft error result (lines 552-601). With panic = "abort" a panicking tool terminates the process in release builds instead of returning a recoverable result, so the release binary loses a documented resilience guarantee that debug builds keep.
Consider dropping panic = "abort" from the release profile, or documenting that tool-panic recovery only holds under panic = "unwind".
♻️ Proposed change to keep unwind-based tool isolation
[profile.release]
lto = "fat"
codegen-units = 1
strip = true
-panic = "abort"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| [profile.release] | |
| lto = "fat" | |
| codegen-units = 1 | |
| strip = true | |
| panic = "abort" | |
| [profile.release] | |
| lto = "fat" | |
| codegen-units = 1 | |
| strip = true | |
| panic = "abort" |
🤖 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 `@Cargo.toml` around lines 99 - 104, Remove panic = "abort" from the
[profile.release] configuration so dispatch_tool’s catch_unwind-based tool-panic
isolation remains effective in release builds. Preserve the other release
profile settings unchanged.
| // Pick the engine turn mode at runtime. `NO_STREAM=1` drives each turn via | ||
| // the non-streaming `create_message` path (no per-delta callbacks, the | ||
| // assembled response is printed after the turn). Otherwise stream text | ||
| // deltas live as they arrive. | ||
| let no_stream = std::env::var("NO_STREAM").map_or(false, |v| v == "1"); | ||
| #[cfg(feature = "streaming")] | ||
| if no_stream { | ||
| agent.set_turn_mode(loopctl::engine::TurnMode::NonStreaming); | ||
| } else { | ||
| agent.set_text_streamer(Arc::new(|delta| { | ||
| print!("{delta}"); | ||
| let _ = std::io::stdout().flush(); | ||
| })); | ||
| } | ||
| #[cfg(not(feature = "streaming"))] | ||
| { | ||
| let _ = no_stream; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep no_stream aligned with the actual engine mode.
When the streaming feature is disabled and NO_STREAM is unset, no_stream remains false. The engine still uses the non-streaming path, but Lines 393-399 then skip printing a non-empty result.output. The default no-feature example can therefore complete without displaying the assistant response.
Set no_stream to true when streaming is unavailable. Select TurnMode::Streaming explicitly in the live-streaming branch instead of relying on the constructor default.
Suggested fix
- let no_stream = std::env::var("NO_STREAM").map_or(false, |v| v == "1");
+ let no_stream = cfg!(not(feature = "streaming"))
+ || std::env::var("NO_STREAM").map_or(false, |v| v == "1");
#[cfg(feature = "streaming")]
if no_stream {
agent.set_turn_mode(loopctl::engine::TurnMode::NonStreaming);
} else {
+ agent.set_turn_mode(loopctl::engine::TurnMode::Streaming);
agent.set_text_streamer(Arc::new(|delta| {
print!("{delta}");
let _ = std::io::stdout().flush();
}));
}
-
- #[cfg(not(feature = "streaming"))]
- {
- let _ = no_stream;
- }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Pick the engine turn mode at runtime. `NO_STREAM=1` drives each turn via | |
| // the non-streaming `create_message` path (no per-delta callbacks, the | |
| // assembled response is printed after the turn). Otherwise stream text | |
| // deltas live as they arrive. | |
| let no_stream = std::env::var("NO_STREAM").map_or(false, |v| v == "1"); | |
| #[cfg(feature = "streaming")] | |
| if no_stream { | |
| agent.set_turn_mode(loopctl::engine::TurnMode::NonStreaming); | |
| } else { | |
| agent.set_text_streamer(Arc::new(|delta| { | |
| print!("{delta}"); | |
| let _ = std::io::stdout().flush(); | |
| })); | |
| } | |
| #[cfg(not(feature = "streaming"))] | |
| { | |
| let _ = no_stream; | |
| } | |
| // Pick the engine turn mode at runtime. `NO_STREAM=1` drives each turn via | |
| // the non-streaming `create_message` path (no per-delta callbacks, the | |
| // assembled response is printed after the turn). Otherwise stream text | |
| // deltas live as they arrive. | |
| let no_stream = cfg!(not(feature = "streaming")) | |
| || std::env::var("NO_STREAM").map_or(false, |v| v == "1"); | |
| #[cfg(feature = "streaming")] | |
| if no_stream { | |
| agent.set_turn_mode(loopctl::engine::TurnMode::NonStreaming); | |
| } else { | |
| agent.set_turn_mode(loopctl::engine::TurnMode::Streaming); | |
| agent.set_text_streamer(Arc::new(|delta| { | |
| print!("{delta}"); | |
| let _ = std::io::stdout().flush(); | |
| })); | |
| } |
🤖 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 `@examples/chat.rs` around lines 336 - 353, Update the runtime mode setup
around `no_stream` so it is true whenever the `streaming` feature is
unavailable, ensuring the later result-printing logic displays non-streaming
responses. In the feature-enabled live-streaming branch, explicitly select
`TurnMode::Streaming` before configuring the text streamer, while preserving the
existing `NO_STREAM=1` non-streaming behavior.
| Enable the `streaming` feature (implied by every HTTP provider) to route turns | ||
| through [`StreamHandler`](https://docs.rs/loopctl/latest/loopctl/stream/handler/struct.StreamHandler.html) | ||
| with retry, timeout, rate-limit detection, and `on_text_delta` / | ||
| `on_thinking_delta` callbacks for real-time token display. Switch a constructed | ||
| loop explicitly with | ||
| [`set_turn_mode`](https://docs.rs/loopctl/latest/loopctl/engine/struct.BareLoop.html#method.set_turn_mode). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe runtime mode selection explicitly.
The streaming feature makes TurnMode::Streaming available. It does not force every turn to use that mode. A loop can select TurnMode::NonStreaming while the feature is enabled, so these paragraphs should refer to TurnMode::Streaming selection rather than feature availability.
Suggested wording
-Enable the `streaming` feature ... to route turns through `StreamHandler`.
+Enable the `streaming` feature to make `TurnMode::Streaming` available.
+When that mode is selected, turns use `StreamHandler`; otherwise they use
+`ApiClient::create_message`.
-Each turn ... uses the streaming path (`StreamHandler`) when `streaming`
-is enabled, or `create_message` otherwise.
+Each turn ... uses `StreamHandler` when `TurnMode::Streaming` is selected,
+or `create_message` otherwise.Also applies to: 179-184
🧰 Tools
🪛 LanguageTool
[grammar] ~174-~174: Ensure spelling is correct
Context: ...itch a constructed loop explicitly with set_turn_mode. #...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 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 `@README.md` around lines 170 - 175, Update the README paragraphs describing
streaming so they distinguish feature availability from runtime mode selection:
state that the streaming feature enables TurnMode::Streaming, but turns use it
only when selected, including via set_turn_mode, while TurnMode::NonStreaming
remains available. Apply the same clarification to the referenced paragraph.
| /// The default is `Streaming` when `streaming` is compiled in, otherwise | ||
| /// `NonStreaming`. Switch modes on a constructed loop with | ||
| /// [`set_turn_mode`](BareLoop::set_turn_mode). | ||
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | ||
| pub enum TurnMode { | ||
| /// Fulfil each turn via [`ApiClient::create_message`]. | ||
| /// | ||
| /// No streaming code is exercised; `on_text_delta`, | ||
| /// `on_thinking_delta`, and the text streamer never fire. The full | ||
| /// assistant text still surfaces through | ||
| /// [`on_response`](crate::observer::LoopObserver::on_response). | ||
| #[default] | ||
| NonStreaming, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
TurnMode::default() and the constructor default disagree.
The derived Default impl marks NonStreaming as the default, so TurnMode::default() returns NonStreaming even when the streaming feature is enabled. The doc on line 122 states the default is Streaming under that feature, and default_turn_mode() implements that. Two different defaults exist on one public type, which will surprise callers who use TurnMode::default().
Pick one contract. Either drop the Default derive and expose only the constructor default, or make the doc state that Default is always NonStreaming while the constructor resolves per feature.
♻️ Option: drop the `Default` derive
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum TurnMode {
/// Fulfil each turn via [`ApiClient::create_message`].
///
/// No streaming code is exercised; `on_text_delta`,
/// `on_thinking_delta`, and the text streamer never fire. The full
/// assistant text still surfaces through
/// [`on_response`](crate::observer::LoopObserver::on_response).
- #[default]
NonStreaming,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// The default is `Streaming` when `streaming` is compiled in, otherwise | |
| /// `NonStreaming`. Switch modes on a constructed loop with | |
| /// [`set_turn_mode`](BareLoop::set_turn_mode). | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] | |
| pub enum TurnMode { | |
| /// Fulfil each turn via [`ApiClient::create_message`]. | |
| /// | |
| /// No streaming code is exercised; `on_text_delta`, | |
| /// `on_thinking_delta`, and the text streamer never fire. The full | |
| /// assistant text still surfaces through | |
| /// [`on_response`](crate::observer::LoopObserver::on_response). | |
| #[default] | |
| NonStreaming, | |
| /// The default is `Streaming` when `streaming` is compiled in, otherwise | |
| /// `NonStreaming`. Switch modes on a constructed loop with | |
| /// [`set_turn_mode`](BareLoop::set_turn_mode). | |
| #[derive(Debug, Clone, Copy, PartialEq, Eq)] | |
| pub enum TurnMode { | |
| /// Fulfil each turn via [`ApiClient::create_message`]. | |
| /// | |
| /// No streaming code is exercised; `on_text_delta`, | |
| /// `on_thinking_delta`, and the text streamer never fire. The full | |
| /// assistant text still surfaces through | |
| /// [`on_response`](crate::observer::LoopObserver::on_response). | |
| NonStreaming, |
🤖 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/engine/bare.rs` around lines 122 - 134, Resolve the conflicting defaults
for TurnMode by removing its derived Default implementation and relying solely
on default_turn_mode() for the feature-dependent constructor default. Update the
associated public documentation to describe this single default contract
accurately, while preserving the enum variants and mode-switching behavior.
| /// Fulfil each turn via [`ApiClient::stream_messages`] wrapped in | ||
| /// [`StreamHandler`](crate::stream::handler::StreamHandler). | ||
| /// | ||
| /// Requires the `streaming` feature. Constructing or selecting this | ||
| /// mode without `streaming` enabled yields a | ||
| /// [`LoopError::Config`](crate::error::LoopError::Config) at turn time. | ||
| #[cfg(feature = "streaming")] | ||
| Streaming, | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the unreachable LoopError::Config claim.
The Streaming variant is gated by #[cfg(feature = "streaming")], so it does not exist when the feature is off. A caller cannot construct or select it, and do_turn has no Config error arm. The documented "yields a LoopError::Config at turn time" behavior is unreachable.
📝 Proposed doc fix
- /// Requires the `streaming` feature. Constructing or selecting this
- /// mode without `streaming` enabled yields a
- /// [`LoopError::Config`](crate::error::LoopError::Config) at turn time.
+ /// Requires the `streaming` feature. Without that feature this variant
+ /// is not compiled, so selecting it is a compile-time error rather than
+ /// a runtime one.
#[cfg(feature = "streaming")]
Streaming,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Fulfil each turn via [`ApiClient::stream_messages`] wrapped in | |
| /// [`StreamHandler`](crate::stream::handler::StreamHandler). | |
| /// | |
| /// Requires the `streaming` feature. Constructing or selecting this | |
| /// mode without `streaming` enabled yields a | |
| /// [`LoopError::Config`](crate::error::LoopError::Config) at turn time. | |
| #[cfg(feature = "streaming")] | |
| Streaming, | |
| } | |
| /// Fulfil each turn via [`ApiClient::stream_messages`] wrapped in | |
| /// [`StreamHandler`](crate::stream::handler::StreamHandler). | |
| /// | |
| /// Requires the `streaming` feature. Without that feature this variant | |
| /// is not compiled, so selecting it is a compile-time error rather than | |
| /// a runtime one. | |
| #[cfg(feature = "streaming")] | |
| Streaming, |
🤖 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/engine/bare.rs` around lines 136 - 144, Update the documentation for the
cfg-gated Streaming variant to remove the claim that disabling the streaming
feature produces a LoopError::Config at turn time, while retaining only the
accurate streaming feature requirement.
1 participant
…ease profile