Refactor/bare loop - #70
Conversation
|
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe refactor moves ChangesBareLoop execution refactor
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/engine/bare/dispatch.rs (1)
414-424: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAbort the wave on the first hard error.
futures::future::join_all(tasks)waits for every in-wave future and only collects results after all complete. If one call reachesLoopError::ToolRecoveryExhaustedwhile another call is still running long work, the parallel batch delays the hard error and violates the documented first-hard-error-abort semantics. Use a short-circuit collector such asfutures::future::try_join_allso the firstErrimmediately stops pending sibling futures.🤖 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/dispatch.rs` around lines 414 - 424, Update the wave collection in the dispatch loop around `join_all(tasks)` to use a short-circuiting collector such as `try_join_all`, so the first hard error aborts pending sibling futures immediately. Preserve handling for cancellation and successful results, and continue storing completed results in the correct `idx` slots before returning the error.Source: MCP tools
🧹 Nitpick comments (5)
src/engine/bare.rs (3)
1075-1081: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the
matchwith?.The
Errarm returns the error unchanged, so the?operator expresses the same control flow.♻️ Proposed refactor
- let dispatched_parts: Vec<MessagePart> = match self - .dispatch_and_record(&dispatch_calls, turn, &accounting) - .await - { - Ok(parts) => parts, - Err(e) => return Err(e), - }; + let dispatched_parts: Vec<MessagePart> = self + .dispatch_and_record(&dispatch_calls, turn, &accounting) + .await?;🤖 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 1075 - 1081, Replace the match handling around dispatch_and_record in the current method with the ? operator, preserving the existing successful Vec<MessagePart> assignment and unchanged error propagation.
941-946: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRead
memory_top_kthroughcurrent_run().The rest of the refactor reads the in-flight run through
self.session.current_run()/current_run_mut(). This site still indexesself.session.runs.last()directly. The result is the same today, but the direct field access bypasses the accessor and will drift if the "current run" definition changes.♻️ Proposed refactor
let memory_top_k = self .session - .runs - .last() + .current_run() .map_or(RunConfig::default().memory_top_k, |r| r.config.memory_top_k);🤖 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 941 - 946, Update collect_memories to obtain memory_top_k through self.session.current_run() instead of directly accessing self.session.runs.last(). Preserve the existing default fallback when no current run exists and continue reading the current run’s config.memory_top_k.
1169-1185: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider delegating the error arms to
MachineOutcome::to_loop_error.
stop_reasonat Line 1252 already converts a terminal outcome into aLoopErrorthroughMachineOutcome::to_loop_error(max_turns). This arm re-implements the same three mappings inline. Only theCompletedarm is special here, because it storesfinal_textand breaks. Delegating the remaining arms keeps one mapping definition.🤖 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 1169 - 1185, The MachineStep::Done handling duplicates MachineOutcome-to-LoopError mappings. Keep the Completed arm’s output storage and break behavior, but replace the MaxTurnsExceeded, Cancelled, and Failed arms with MachineOutcome::to_loop_error using run_config.max_turns, matching the existing stop_reason conversion.src/engine/bare/emission.rs (1)
249-258: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe stream events derive
turnfrom a different source than the rest of the refactor.
record_turn_successandrecord_turn_failurecomputeturnasself.session.current_run().map_or(0, Run::turn_count). Every other notification in this layer now carries the machine-suppliedturnfromMachineStep::CallLLM. The two agree today, becausehandle_call_llmpushes theTurnonly after the provider call returns, soturn_count()still equals the 0-indexed current turn. The equality is incidental, not enforced.Passing the machine
turndown into these helpers would makeon_stream_success/on_stream_failurecorrelate withon_turn_startby construction.Also applies to: 293-299
🤖 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/emission.rs` around lines 249 - 258, Update record_turn_success and record_turn_failure to accept the machine-supplied turn as an argument and use it when constructing StreamContext, replacing the current session.current_run().map_or(0, Run::turn_count) lookup. Propagate the turn from MachineStep::CallLLM through both helper call sites so stream success and failure notifications correlate with on_turn_start.src/engine/bare/tests.rs (1)
1273-1281: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState the "no previous event" case explicitly.
idx.wrapping_sub(1)relies onusizewraparound tousize::MAXand then onVec::getreturningNonefor that index. The intent is "there is no event before index 0".checked_subexpresses it directly.♻️ Proposed refactor
- let before = events.get(idx.wrapping_sub(1)); + let before = idx.checked_sub(1).and_then(|i| events.get(i));🤖 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/tests.rs` around lines 1273 - 1281, Update the event-boundary assertion around the on_compaction search to use checked_sub when computing the preceding index, explicitly representing the absence of a previous event at index 0 instead of relying on wrapping subtraction. Preserve the existing before/after boundary checks and assertion 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 `@src/engine/bare.rs`:
- Around line 990-1009: Update turn_input to take &self instead of &mut self,
and use self.machine.full_history().last() for non-initial turns so pending
user, assistant, and tool messages are included. Preserve the existing
first-turn current_run input behavior and text-part aggregation.
- Around line 828-852: Ensure every error path after notify_turn_start in the
turn-processing flow emits notify_turn_end before returning. Update the
non-cancelled do_turn error arm and the apply_loop_detection error branch to
send a failed TurnEnd with the relevant error details, duration, and token
counts, matching the existing LoopError::Cancelled handling while preserving
their original error returns.
In `@src/engine/bare/dispatch.rs`:
- Around line 771-774: Update the tokio::select! in recovery_wait_or_return to
use biased selection, placing self.cancelled.notified() before the recovery
sleep branch. Preserve RecoveryDecision::Cancelled when cancellation is ready,
ensuring cancellation wins over a simultaneously ready Retry.
- Around line 553-557: Preserve the initial tool name across recovery retries in
the dispatch flow containing the recovery loop: capture the original `ToolCall`
name before retries can mutate `tc.tool`, then use that saved value for
`LoopError::ToolRecoveryExhausted` instead of the current corrected name.
In `@src/engine/bare/llm_turn.rs`:
- Around line 98-120: Update do_create_message to race
client.create_message_with_options against a configurable timeout in addition to
cancellation, using the existing timeout configuration used by the streaming
path where available. Map timeout expiration to LoopError::Api (or the
established dedicated timeout variant) before passing it through
record_turn_failure, while preserving the existing success and cancellation
behavior.
In `@src/engine/bare/model_switch.rs`:
- Around line 93-114: Handle the boolean result from ApiClient::set_model in
apply instead of unconditionally reporting success: when it returns false,
either emit a tracing warning while preserving the existing best-effort fallback
and observer behavior, or return LoopError::Config and skip those updates,
consistent with the intended design and switch_model_unsupported_client. Also
revise apply’s “atomically” documentation to reflect that no rollback occurs
when model switching fails.
In `@src/engine/core/lifecycle.rs`:
- Around line 100-106: Update BareLoop::collect_memories to return immediately
when memory_top_k is zero, before accessing or invoking the LoopMemory backend;
preserve normal retrieval for positive values. Add a test using a tracking/mock
backend to verify retrieve is not called when memory_top_k is zero.
---
Outside diff comments:
In `@src/engine/bare/dispatch.rs`:
- Around line 414-424: Update the wave collection in the dispatch loop around
`join_all(tasks)` to use a short-circuiting collector such as `try_join_all`, so
the first hard error aborts pending sibling futures immediately. Preserve
handling for cancellation and successful results, and continue storing completed
results in the correct `idx` slots before returning the error.
---
Nitpick comments:
In `@src/engine/bare.rs`:
- Around line 1075-1081: Replace the match handling around dispatch_and_record
in the current method with the ? operator, preserving the existing successful
Vec<MessagePart> assignment and unchanged error propagation.
- Around line 941-946: Update collect_memories to obtain memory_top_k through
self.session.current_run() instead of directly accessing
self.session.runs.last(). Preserve the existing default fallback when no current
run exists and continue reading the current run’s config.memory_top_k.
- Around line 1169-1185: The MachineStep::Done handling duplicates
MachineOutcome-to-LoopError mappings. Keep the Completed arm’s output storage
and break behavior, but replace the MaxTurnsExceeded, Cancelled, and Failed arms
with MachineOutcome::to_loop_error using run_config.max_turns, matching the
existing stop_reason conversion.
In `@src/engine/bare/emission.rs`:
- Around line 249-258: Update record_turn_success and record_turn_failure to
accept the machine-supplied turn as an argument and use it when constructing
StreamContext, replacing the current session.current_run().map_or(0,
Run::turn_count) lookup. Propagate the turn from MachineStep::CallLLM through
both helper call sites so stream success and failure notifications correlate
with on_turn_start.
In `@src/engine/bare/tests.rs`:
- Around line 1273-1281: Update the event-boundary assertion around the
on_compaction search to use checked_sub when computing the preceding index,
explicitly representing the absence of a previous event at index 0 instead of
relying on wrapping subtraction. Preserve the existing before/after boundary
checks and assertion behavior.
🪄 Autofix
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: 23a20ab2-fb17-4287-90ab-df359c7a89ac
📒 Files selected for processing (27)
.github/workflows/ci.ymlCHANGELOG.mdMakefilesrc/contributor.rssrc/engine.rssrc/engine/bare.rssrc/engine/bare/compact.rssrc/engine/bare/config.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/bare/llm_turn.rssrc/engine/bare/message.rssrc/engine/bare/model_switch.rssrc/engine/bare/stream.rssrc/engine/bare/tests.rssrc/engine/core.rssrc/engine/core/lifecycle.rssrc/engine/core/machine.rssrc/engine/core/outcome.rssrc/error.rssrc/lib.rssrc/memory/builtin.rssrc/message.rssrc/presets.rssrc/reflection/llm.rssrc/tool.rstests/provider_e2e.rs
💤 Files with no reviewable changes (4)
- src/engine.rs
- src/engine/bare/message.rs
- src/engine/bare/stream.rs
- src/memory/builtin.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/engine/bare/dispatch.rs (2)
768-776: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMake recovery analysis cancellation-aware.
recover_tool_error(...).awaitrunsReflector::analyze()andRecoveryStrategy::decide()before the cancellation-awareselect!. If either await blocks a cancellable path, returnRecoveryDecision::Cancelledonly after that work finishes. Wrap this await in a biasedselect!withself.cancelled.notified()first, then keep the existing backoffselect!as-is.🤖 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/dispatch.rs` around lines 768 - 776, Make the await of recover_tool_error cancellation-aware by wrapping it in a biased tokio::select!, placing self.cancelled.notified() first and returning RecoveryDecision::Cancelled when cancellation fires. Preserve the resulting recovery_action/correction handling and leave the existing backoff select! unchanged.
414-424: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftAbort sibling futures when a wave gets a hard error.
join_allwaits for a wave’s futures to complete. If one call returnsLoopError::LoopDetectedorLoopError::ToolRecoveryExhausted, a sibling call can continue untiljoin_allfinishes, delaying the batch result and allowing sibling tool side effects after the hard stop.Use a fail-fast or cancellable parallel-collection pattern for each wave, then preserve result order for successful waves. Update
parallel_hard_error_discards_sibling_resultsto usemax_concurrency: 2, a non-cancellation hard error, and a delayed sibling.🤖 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/dispatch.rs` around lines 414 - 424, Replace the `join_all(tasks).await` collection in the wave-dispatch flow with a fail-fast, cancellable pattern that aborts sibling futures when a non-cancellation hard error such as `LoopDetected` or `ToolRecoveryExhausted` occurs. Preserve ordered result placement for successful outcomes via the existing `wave` indices and keep cancellation handling intact. Update `parallel_hard_error_discards_sibling_results` to configure `max_concurrency: 2`, use a non-cancellation hard error, and include a delayed sibling verifying it is cancelled.
🧹 Nitpick comments (1)
src/engine/bare/tests.rs (1)
1005-1032: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive control so the test cannot pass for the wrong reason.
TrackingMemory::len()returns0. If the engine skips retrieval whenever memory is empty,retrieve_calls == 0holds even whenmemory_top_k > 0. The assertion then proves nothing aboutmemory_top_k.Return a non-zero
len(), or add a paired case withmemory_top_k: 3that assertsretrieve_calls == 1.♻️ Proposed change
fn len(&self) -> usize { - 0 + 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 `@src/engine/bare/tests.rs` around lines 1005 - 1032, Strengthen the memory_top_k retrieval test around TrackingMemory by making len() report available memory or adding a paired positive-control run with memory_top_k set to 3. Assert that the positive case invokes retrieve exactly once, while preserving the existing assertion that memory_top_k == 0 skips retrieval.
🤖 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/llm_turn.rs`:
- Around line 112-118: Update the timeout handling in do_create_message and
turn_timeout so an unconfigured handler never passes Duration::MAX to
tokio::time::sleep. When no stream timeout is configured, use a finite per-turn
timeout or bypass this timeout path for default passthrough handlers, while
preserving cancellation behavior.
In `@src/stream/handler.rs`:
- Around line 114-116: Adjust the default timeout values in the stream handler
configuration so per_event_timeout is strictly shorter than
total_stream_timeout, preserving total_stream_timeout at five minutes and
enabling the per-event timeout path used by event_deadline and next_event.
---
Outside diff comments:
In `@src/engine/bare/dispatch.rs`:
- Around line 768-776: Make the await of recover_tool_error cancellation-aware
by wrapping it in a biased tokio::select!, placing self.cancelled.notified()
first and returning RecoveryDecision::Cancelled when cancellation fires.
Preserve the resulting recovery_action/correction handling and leave the
existing backoff select! unchanged.
- Around line 414-424: Replace the `join_all(tasks).await` collection in the
wave-dispatch flow with a fail-fast, cancellable pattern that aborts sibling
futures when a non-cancellation hard error such as `LoopDetected` or
`ToolRecoveryExhausted` occurs. Preserve ordered result placement for successful
outcomes via the existing `wave` indices and keep cancellation handling intact.
Update `parallel_hard_error_discards_sibling_results` to configure
`max_concurrency: 2`, use a non-cancellation hard error, and include a delayed
sibling verifying it is cancelled.
---
Nitpick comments:
In `@src/engine/bare/tests.rs`:
- Around line 1005-1032: Strengthen the memory_top_k retrieval test around
TrackingMemory by making len() report available memory or adding a paired
positive-control run with memory_top_k set to 3. Assert that the positive case
invokes retrieve exactly once, while preserving the existing assertion that
memory_top_k == 0 skips retrieval.
🪄 Autofix
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: 7f995505-1e81-496b-8358-e03004653723
📒 Files selected for processing (8)
src/engine/bare.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/bare/llm_turn.rssrc/engine/bare/model_switch.rssrc/engine/bare/tests.rssrc/managers.rssrc/stream/handler.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/engine/bare/emission.rs
- src/engine/bare.rs
1 participant
No description provided.