refactor - #36
Merged
Merged
Conversation
Contributor
|
Warning Review limit reached
Next review available in: 30 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe crate is reorganized around loop-centric modules and types, with new runtime, observer, memory, detection, reflection, compaction, and middleware subsystems. ChangesCore surface and runtime types
Runtime, detection, and dispatch
Examples and validation
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 18
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
src/engine/bare.rs (1)
1083-1097:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid emitting a successful turn-end before compaction can fail.
dispatch_and_recordemits a successfulon_turn_end, thenmaybe_compact_contextcan fail andabort_turn_and_sessionemits a failedon_turn_endfor the same logical turn. Delay the success notification until after post-turn compaction succeeds, or make the compaction failure path session-only once a turn-end has already been emitted.Also applies to: 1269-1276
🤖 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 1083 - 1097, The current code flow emits a successful on_turn_end event from dispatch_and_record, then if maybe_compact_context fails afterward, abort_turn_and_session emits a failed on_turn_end for the same logical turn, resulting in conflicting turn-end notifications. Fix this by either: (1) moving the success notification in dispatch_and_record to occur only after maybe_compact_context completes successfully, or (2) modifying the error handling path in abort_turn_and_session to emit session-level errors only (not turn-level) when a turn-end has already been successfully emitted. Apply the same fix pattern at both occurrences of this code (around line 1083-1097 and line 1269-1276).src/compact.rs (2)
668-674:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTelemetry currently undercounts tool-involved messages.
Line 673 only checks
is_tool_call, butPreCompactStats.tool_messagesis documented as counting tool calls or results. This skews compaction telemetry.Proposed fix
tool_messages: pre_messages .iter() .filter(|m| { m.parts .iter() - .any(crate::message::MessagePart::is_tool_call) + .any(|p| p.is_tool_call() || p.is_tool_result()) }) .count(),🤖 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/compact.rs` around lines 668 - 674, The filter for tool_messages in the compact operation at line 673 only checks for is_tool_call but according to the PreCompactStats.tool_messages documentation, it should count both tool calls and tool results. Update the filter condition in the any() call to check for both is_tool_call and is_tool_result (or whatever the method name is for checking tool results) to ensure tool-involved messages are correctly counted in the compaction telemetry.
585-622:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
compact_manualmisses the post-compaction overflow check promised by its contract.The docs on Line 585 state this method errors when compaction still exceeds the context window, but the implementation returns
Compacted(outcome)without validatingtokens_after. That can propagate an oversized conversation as a success.Proposed fix
if !outcome.success { return Err(ContextOverflow { tokens_used: tokens_before, context_window: self.context_window, message_count, trigger: CompactReason::Manual, compactor_error: outcome.error, }); } + let tokens_after = Self::estimate_tokens(&outcome.messages); + if tokens_after > self.context_window { + return Err(ContextOverflow { + tokens_used: tokens_after, + context_window: self.context_window, + message_count: outcome.messages.len(), + trigger: CompactReason::Manual, + compactor_error: Some("compactor failed to reduce context below window".into()), + }); + } + Ok(EnsureContextResult::Compacted(outcome)) }🤖 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/compact.rs` around lines 585 - 622, The compact_manual method documentation states it should return a ContextOverflow error when the result still exceeds the context window, but the current implementation only checks if the compaction succeeded via outcome.success without validating whether the compacted result fits within the context window. Add a second validation check after the existing !outcome.success check to verify that outcome.tokens_after does not exceed self.context_window, and return an error with ContextOverflow containing the actual tokens_after value if this post-compaction overflow condition is detected.src/detection/convergence.rs (1)
537-575:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix streak counting semantics in
add_response.On Line 549 and Line 566, streak is updated once per prior window item, then compared directly to
window_size. This causes incorrect behavior (e.g.,window_size = 2with two identical responses does not converge). Update streak once per new response (based on prior-response relationship), then evaluate detection.💡 Suggested direction
- for prev_response in &self.window { + let mut similar_to_previous = false; + for (idx, prev_response) in self.window.iter().enumerate() { let similarity = Self::compute_similarity(response, prev_response); if similarity > max_similarity { max_similarity = similarity; } - - if similarity >= self.config.similarity_threshold { - self.consecutive_count = self.consecutive_count.saturating_add(1); - if !self.similar_responses.contains(&response.to_string()) { - self.similar_responses.push(response.to_string()); - } - } else { - self.consecutive_count = 1; - self.similar_responses.clear(); - self.similar_responses.push(response.to_string()); - } + if idx + 1 == self.window.len() { + similar_to_previous = similarity >= self.config.similarity_threshold; + } } + + if self.window.is_empty() { + self.consecutive_count = 1; + } else if similar_to_previous { + self.consecutive_count = self.consecutive_count.saturating_add(1); + } else { + self.consecutive_count = 1; + self.similar_responses.clear(); + }🤖 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/detection/convergence.rs` around lines 537 - 575, The consecutive_count streak is being updated inside the loop that iterates through all previous responses in self.window, causing it to be incremented or reset multiple times for a single new response. This breaks the streak semantics since the count should increase by 1 per new response added (if it matches the previous response pattern), not once per prior window item. Refactor the add_response method to determine similarity once per new response (by checking against the appropriate previous response, likely the most recent one in the window) and then update self.consecutive_count exactly once based on that single similarity check, before evaluating the convergence detection condition against self.config.window_size.src/fallback.rs (3)
1430-1470:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTrip the circuit inside
record_api_failure().BareLoop calls
record_api_failure()on stream failures and does not calltransition_to_fallback()afterward, so this path returnstruebut leaves the manager inPrimarywithfallback_activated == false. That makes fallback activation repeat on later failures instead of opening the circuit.Proposed fix
if failures >= self.fallback_threshold && !self.fallback_activated.load(Ordering::Relaxed) { warn!( consecutive_failures = failures, threshold = self.fallback_threshold, "Fallback threshold reached" ); + self.transition_to_fallback(); true } else { false }🤖 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/fallback.rs` around lines 1430 - 1470, In the record_api_failure method, when the failure count reaches the fallback threshold, you need to atomically set the fallback_activated flag to true in addition to returning true. Currently the method returns true to signal that fallback should be triggered, but it does not actually set fallback_activated to true, which causes the circuit to never be opened and allows the method to return true again on subsequent failures instead of returning false. Add a store operation to fallback_activated with Ordering::Relaxed before returning true in the threshold-reached condition.
305-329:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve
new_failed()when changing the threshold.
FallbackEntry::new_failed("x").with_max_fail_count(3)currently becomes non-failed becausenew_failed()seeds two attempts, then the threshold is raised to three. Preserve the prior failed state when adjusting the threshold.Proposed fix
pub fn with_max_fail_count(mut self, max_fail_count: usize) -> Self { + let was_failed = self.failed(); self.max_fail_count = max_fail_count.max(1); + if was_failed { + while self.attempts.len() < self.max_fail_count { + self.attempts.push(AttemptRecord::anonymous()); + } + } self }🤖 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/fallback.rs` around lines 305 - 329, The `new_failed()` method initializes a FallbackEntry with 2 attempts and max_fail_count of 2, marking it as failed. However, when `with_max_fail_count()` is called with a higher value (e.g., 3), the entry becomes non-failed because now it needs 3 attempts to be considered failed. Modify the `with_max_fail_count()` method to preserve the failed state by ensuring that if the entry was previously failed (attempts >= current max_fail_count), it remains failed after the threshold adjustment by adding additional attempts as needed to maintain that failed status with the new max_fail_count threshold.
686-710:⚠️ Potential issue | 🟠 Major | ⚡ Quick winApply all config fields or stop advertising config-driven recovery.
FallbackConfig::recovery_timeoutis part of the config API, butwith_config()discards it, so callers using this method do not actually configure the recovery cooldown. Either store/apply the timeout in the manager API or make the docs explicit that callers must passconfig.recovery_timeoutseparately toshould_try_resume_primary().🤖 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/fallback.rs` around lines 686 - 710, The with_config() method is missing the assignment for the recovery_timeout field from FallbackConfig. Add a line to assign config.recovery_timeout to the corresponding field in the FallbackManager instance (likely self.recovery_timeout or similar based on the pattern of the other three assignments), ensuring that all fields advertised in the FallbackConfig are actually applied when callers use this convenience method.src/detection/manager.rs (2)
269-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWire
max_response_historyinto convergence config or remove it.Line 269 documents
max_response_historyas controlling retained responses, butto_convergence_config()never passes it to theConvergenceDetector, so changing this public field has no effect throughDetectionManager.🤖 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/detection/manager.rs` around lines 269 - 307, The field max_response_history is defined in DetectionConfig and initialized in the Default implementation, but it is never passed to ConvergenceConfig in the to_convergence_config() method. Either wire this field into the convergence config by adding it to the ConvergenceConfig struct initialization (if ConvergenceConfig has a corresponding field for max_response_history), or remove the max_response_history field entirely from DetectionConfig along with its documentation comment if it is not used by the downstream ConvergenceDetector.
527-552:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep injected detector thresholds consistent with
config().This constructor says the loop-related
configfields are ignored, but the manager still exposes that sameconfig; downstreamBareLoopusesdetection.config().stop_thresholdto decide when to stop. An injectedLoopDetectorwith a different stop threshold can warn according to one configuration and stop according to another. Either synchronize the stored config with the injected detector or change the runtime stop check to useLoopStatus::should_stop.🤖 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/detection/manager.rs` around lines 527 - 552, The new_with_loop_detector constructor accepts a pre-built LoopDetector but still stores and exposes the original config via the config() method. This creates an inconsistency because downstream code (in BareLoop) uses detection.config().stop_threshold to make stopping decisions, while the injected LoopDetector may have different thresholds baked in. Either extract the actual threshold values from the injected loop_detector and update the stored config to match those values (ensuring consistency), or update the runtime stop check in BareLoop to use LoopStatus::should_stop instead of directly accessing config().stop_threshold, so it respects the detector's actual configuration.
🧹 Nitpick comments (2)
src/reflection/backoff.rs (1)
129-139: ⚡ Quick winUse
max_attemptsindecideto avoid policy drift with the caller.Line 133 receives
max_attemptsbut ignores it. Since the caller also enforces a cap insrc/engine/bare/dispatch.rs(Lines 306-331), the strategy can emitRetrydecisions that are immediately discarded. Consider combining both limits in one place.Suggested change
- fn decide( + fn decide( &self, analysis: &FailureAnalysis, attempt: u32, - _max_attempts: u32, + max_attempts: u32, ) -> Pin<Box<dyn Future<Output = RecoveryAction> + Send + '_>> { + let retry_cap = self.max_retries.min(max_attempts); let action = if !analysis.is_recoverable { RecoveryAction::Fail(analysis.root_cause.clone()) - } else if attempt >= self.max_retries { - RecoveryAction::Fail(format!("max retries ({}) exceeded", self.max_retries)) + } else if attempt >= retry_cap { + RecoveryAction::Fail(format!("max retries ({retry_cap}) exceeded")) } else if analysis.severity >= FailureSeverity::High && analysis.correction.is_some() {🤖 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/reflection/backoff.rs` around lines 129 - 139, The `decide` method receives the `max_attempts` parameter but ignores it when making recovery decisions, while the caller in `src/engine/bare/dispatch.rs` also enforces a separate cap, causing potential policy drift. In the decision logic where you check `attempt >= self.max_retries`, extend this condition to also account for the `max_attempts` parameter so that both limits are considered in one place, preventing the strategy from emitting Retry decisions that would be immediately discarded by the caller.src/hooks/executor.rs (1)
81-101: 💤 Low valueRedundant builder methods with identical semantics.
Both
with_interactivityandinteractivitynow takemut self, set the same field, and returnSelf. They are functionally identical.Additionally, the doc comment on
with_interactivity(line 83) suggests usinginteractivity"to change the mode after construction", butinteractivityalso consumesself, so this guidance is misleading.Consider keeping only one method (e.g.,
with_interactivityto matchwith_hook), or clarifying the intended distinction if there is one.🤖 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/hooks/executor.rs` around lines 81 - 101, Remove the redundant builder method to eliminate duplication in HookExecutor. Both with_interactivity and interactivity methods are functionally identical - they both take mut self, set the same interactivity field, and return Self for chaining. Keep the with_interactivity method since it aligns with the naming pattern of with_hook, and delete the interactivity method entirely. Update the doc comment for with_interactivity to remove the misleading reference to calling interactivity "after construction" since both methods consume self via mut self and are used in the builder pattern, not for post-construction mutation.
🤖 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/compact.rs`:
- Around line 201-220: The `#[default]` attribute in the `CompactBase` enum is
currently applied to the `Context` variant, but the documentation and
`ContextManager::new` indicate that `Threshold` should be the default behavior.
Move the `#[default]` attribute from the `Context` variant to the `Threshold`
variant to ensure that `CompactBase::default()` returns the same default variant
that is documented and used throughout the codebase.
In `@src/detection/loop_detector.rs`:
- Around line 1369-1434: The find_repeated function collects operations from a
HashMap which has non-deterministic iteration order, causing the repeated vector
to have varying order across calls. When multiple operations have the same max
count (tie cases), build_warning's first() call will select different operations
non-deterministically, breaking the warning suppression logic. Sort the repeated
operations vector in a stable, deterministic order (such as by tool name or
operation name) before returning from find_repeated to ensure build_warning
consistently selects the same operation when ties occur.
- Around line 1506-1512: The check_file_reads method has asymmetrical
normalization where o.primary_param is normalized using
sig.normalize_param_for_comparison but is compared against raw file_path.
Normalize file_path using the same sig.normalize_param_for_comparison method
before the comparison to ensure consistency. Store the normalized file_path in a
variable before the ops.iter() call and use that normalized value in the filter
comparison, matching the pattern used in clear_warnings_for_recoverable_edit.
In `@src/detection/manager.rs`:
- Around line 411-414: The documentation comment describing the manager
lifecycle still references the old constructor name with_config() which has been
renamed to new_with_config(). Update the lifecycle documentation in the manager
to replace the reference from with_config() to new_with_config() to accurately
reflect the current public API.
In `@src/engine/bare.rs`:
- Around line 1269-1276: The on_turn_end notifications are using
budget.turn_count which gets incremented before the notification is sent,
causing a mismatch with on_turn_start which uses the turn index before
incrementing. Replace budget.turn_count with turn.idx in all TurnEndContext
calls to on_turn_end (both at the shown location and at lines 1281-1288) to
ensure consistent turn indices between on_turn_start and on_turn_end
notifications and maintain the pairing contract.
- Line 55: The documentation examples in the bare.rs file contain references to
the removed `core` module, specifically the commented-out import statement
showing `loopctl::core::LoopConfig`. Update all three instances of stale module
paths (at the locations indicated: 55, 158, and 917) by replacing references to
`loopctl::core` with the correct paths from the new public API surface. Ensure
all documentation examples and imports are updated to use the current module
structure to prevent users from encountering errors when following the docs and
to ensure doctests pass.
In `@src/engine/bare/dispatch.rs`:
- Around line 105-116: The `on_tool_pre` callback fires before the
`check_pre_tool_hooks` and `pre_detection` blocking checks, but early returns
from these checks skip the corresponding `on_tool_post` callback, creating
unpaired observer lifecycle events. Fix this by either moving the `on_tool_pre`
call after both blocking checks so it only fires for non-blocked paths, or add
`on_tool_post` with an error ToolPostContext when `check_pre_tool_hooks` or
`pre_detection` return a blocked result before the early return.
- Around line 466-472: After the tokio::select! block where pipeline.invoke(ctx)
completes and dispatch_result is obtained, add a check to see if the
cancellation signal has been set by calling cancel.is_cancelled(). If the
cancellation signal is set after pipeline invocation completes, return
Err(LoopError::Cancelled) immediately instead of continuing to construct and
return the ToolDispatchResult. This ensures that late-arriving cancellations are
properly propagated rather than being masked as soft tool errors by
ToolCallMiddleware, matching the behavior of ToolPipeline::dispatch_all.
- Around line 177-186: The `map` operation on the Result returned by
`handle_detected_pattern` is converting the error case into a soft tool result,
which discards the hard loop-detection error and bypasses the configured stop
policy. Instead of mapping the success case to a ToolDispatchResult, you should
use `match` or similar error propagation to handle both the Ok and Err cases of
`handle_detected_pattern`. When `Err(LoopError::LoopDetected)` is returned,
propagate that error up to the caller rather than converting it to a soft
ToolDispatchResult, allowing the configured stop policy to be properly enforced.
Only create a ToolDispatchResult for the success case or for actual tool
execution paths.
In `@src/memory.rs`:
- Around line 1-12: Update the module-level documentation comment in memory.rs
to reflect the current public API. Replace the outdated "Agent memory" wording
in the initial description and update the "Provided Implementations" section to
list LoopMemory and InMemoryStore instead of TrajectoryMemory, ensuring the
documentation accurately describes the actual public exports of the module.
In `@src/memory/builtin.rs`:
- Around line 141-143: The rustdoc comment for the with_entries method contains
incomplete and disconnected sentences that make the documentation unclear. The
first line "Useful for setting up test fixtures or seeding an agent with" is a
sentence fragment that doesn't complete its thought, and it's awkwardly
separated from the following line. Revise the rustdoc comment above the
with_entries method to form complete, coherent sentences that clearly explain
what the method does and its use cases, ensuring all sentences are grammatically
complete and logically connected.
In `@src/middleware/permission.rs`:
- Around line 39-45: The Rustdoc example for PermissionMiddleware contains two
API inconsistencies that prevent the code from compiling. First, verify the
correct method or constructor for creating a PermissionMiddleware with a
permission check (the example currently shows
`PermissionMiddleware::with_check(...)` which may not match the actual API
shape). Second, update the `PermissionCheck::Deny` variant to include its
required `reason` field with an appropriate string value explaining why the
permission was denied. Update the example code block within the documentation
comment to reflect the correct, working API so readers can use it as a valid
reference.
In `@src/middleware/timeout.rs`:
- Around line 68-69: The documentation comment for the `from_secs` method
incorrectly describes the default retry settings as including one retry with
double timeout. Update this comment to accurately reflect the actual default
values: `retry_on_timeout: false` and `max_retries: 0`, which means retries are
disabled by default. The corrected documentation should clearly state that no
retries occur with default settings, allowing users to understand the actual
reliability behavior without confusion.
In `@src/middleware/unknown_tool.rs`:
- Around line 25-27: Update the Rustdoc example for the UnknownToolMiddleware
struct to reflect the current signature of the new method. The example currently
shows UnknownToolMiddleware::new() being called without arguments, but the
method now requires an Arc<ToolRegistry> parameter. Modify the example to
properly instantiate a ToolRegistry, wrap it in an Arc, and pass it to the new
method call.
- Around line 163-178: The is_tool_not_found function is too broad in its check
by simply looking for "not found" anywhere in the error message, which will
incorrectly match unrelated errors like file or resource not found errors. Make
the pattern matching more specific to tool-related errors by checking for error
patterns that specifically indicate a tool was not found (such as checking for
"tool" in the error message alongside "not found", or looking for other
tool-specific keywords) instead of just matching any error containing "not
found".
- Around line 191-203: The tool_name is captured before the next.dispatch(ctx)
call, but middleware can mutate ctx.tool_name during dispatch to redirect to a
different tool. When generating suggestions in the is_tool_not_found block, you
are using the stale tool_name instead of the potentially updated one. Move the
tool_name capture to after the dispatch completes and use the resolved tool name
from the result object (or updated context) when calling
Self::find_best_match_inner to ensure suggestions are generated for the correct
redirected tool name.
In `@src/observer.rs`:
- Around line 142-146: The runtime call order in src/engine/bare.rs violates the
documented lifecycle contract for the reset method in the Observer trait. The
current implementation calls on_session_start before reset, which causes
per-session observer state to be wiped immediately after initialization. Reorder
the method calls in the runtime so that reset is invoked before on_session_start
to match the trait's documented contract and preserve observer state during
session start notification.
In `@src/observer/context.rs`:
- Around line 97-106: The TurnEndContext fields input_tokens and output_tokens
are documented as per-turn token counts, but in src/engine/bare.rs when
constructing TurnEndContext during the pattern short-circuit path, cumulative
session totals from budget.input_tokens and budget.output_tokens are being
passed instead. To fix this, locate where TurnEndContext is emitted in
src/engine/bare.rs (particularly in the early exit path) and replace the direct
assignment of budget.input_tokens and budget.output_tokens with calculations
that produce only the tokens consumed in the current turn. You may need to track
the token state at the turn's start and calculate the delta, or structure the
code to only accumulate tokens during the current turn's operations.
---
Outside diff comments:
In `@src/compact.rs`:
- Around line 668-674: The filter for tool_messages in the compact operation at
line 673 only checks for is_tool_call but according to the
PreCompactStats.tool_messages documentation, it should count both tool calls and
tool results. Update the filter condition in the any() call to check for both
is_tool_call and is_tool_result (or whatever the method name is for checking
tool results) to ensure tool-involved messages are correctly counted in the
compaction telemetry.
- Around line 585-622: The compact_manual method documentation states it should
return a ContextOverflow error when the result still exceeds the context window,
but the current implementation only checks if the compaction succeeded via
outcome.success without validating whether the compacted result fits within the
context window. Add a second validation check after the existing
!outcome.success check to verify that outcome.tokens_after does not exceed
self.context_window, and return an error with ContextOverflow containing the
actual tokens_after value if this post-compaction overflow condition is
detected.
In `@src/detection/convergence.rs`:
- Around line 537-575: The consecutive_count streak is being updated inside the
loop that iterates through all previous responses in self.window, causing it to
be incremented or reset multiple times for a single new response. This breaks
the streak semantics since the count should increase by 1 per new response added
(if it matches the previous response pattern), not once per prior window item.
Refactor the add_response method to determine similarity once per new response
(by checking against the appropriate previous response, likely the most recent
one in the window) and then update self.consecutive_count exactly once based on
that single similarity check, before evaluating the convergence detection
condition against self.config.window_size.
In `@src/detection/manager.rs`:
- Around line 269-307: The field max_response_history is defined in
DetectionConfig and initialized in the Default implementation, but it is never
passed to ConvergenceConfig in the to_convergence_config() method. Either wire
this field into the convergence config by adding it to the ConvergenceConfig
struct initialization (if ConvergenceConfig has a corresponding field for
max_response_history), or remove the max_response_history field entirely from
DetectionConfig along with its documentation comment if it is not used by the
downstream ConvergenceDetector.
- Around line 527-552: The new_with_loop_detector constructor accepts a
pre-built LoopDetector but still stores and exposes the original config via the
config() method. This creates an inconsistency because downstream code (in
BareLoop) uses detection.config().stop_threshold to make stopping decisions,
while the injected LoopDetector may have different thresholds baked in. Either
extract the actual threshold values from the injected loop_detector and update
the stored config to match those values (ensuring consistency), or update the
runtime stop check in BareLoop to use LoopStatus::should_stop instead of
directly accessing config().stop_threshold, so it respects the detector's actual
configuration.
In `@src/engine/bare.rs`:
- Around line 1083-1097: The current code flow emits a successful on_turn_end
event from dispatch_and_record, then if maybe_compact_context fails afterward,
abort_turn_and_session emits a failed on_turn_end for the same logical turn,
resulting in conflicting turn-end notifications. Fix this by either: (1) moving
the success notification in dispatch_and_record to occur only after
maybe_compact_context completes successfully, or (2) modifying the error
handling path in abort_turn_and_session to emit session-level errors only (not
turn-level) when a turn-end has already been successfully emitted. Apply the
same fix pattern at both occurrences of this code (around line 1083-1097 and
line 1269-1276).
In `@src/fallback.rs`:
- Around line 1430-1470: In the record_api_failure method, when the failure
count reaches the fallback threshold, you need to atomically set the
fallback_activated flag to true in addition to returning true. Currently the
method returns true to signal that fallback should be triggered, but it does not
actually set fallback_activated to true, which causes the circuit to never be
opened and allows the method to return true again on subsequent failures instead
of returning false. Add a store operation to fallback_activated with
Ordering::Relaxed before returning true in the threshold-reached condition.
- Around line 305-329: The `new_failed()` method initializes a FallbackEntry
with 2 attempts and max_fail_count of 2, marking it as failed. However, when
`with_max_fail_count()` is called with a higher value (e.g., 3), the entry
becomes non-failed because now it needs 3 attempts to be considered failed.
Modify the `with_max_fail_count()` method to preserve the failed state by
ensuring that if the entry was previously failed (attempts >= current
max_fail_count), it remains failed after the threshold adjustment by adding
additional attempts as needed to maintain that failed status with the new
max_fail_count threshold.
- Around line 686-710: The with_config() method is missing the assignment for
the recovery_timeout field from FallbackConfig. Add a line to assign
config.recovery_timeout to the corresponding field in the FallbackManager
instance (likely self.recovery_timeout or similar based on the pattern of the
other three assignments), ensuring that all fields advertised in the
FallbackConfig are actually applied when callers use this convenience method.
---
Nitpick comments:
In `@src/hooks/executor.rs`:
- Around line 81-101: Remove the redundant builder method to eliminate
duplication in HookExecutor. Both with_interactivity and interactivity methods
are functionally identical - they both take mut self, set the same interactivity
field, and return Self for chaining. Keep the with_interactivity method since it
aligns with the naming pattern of with_hook, and delete the interactivity method
entirely. Update the doc comment for with_interactivity to remove the misleading
reference to calling interactivity "after construction" since both methods
consume self via mut self and are used in the builder pattern, not for
post-construction mutation.
In `@src/reflection/backoff.rs`:
- Around line 129-139: The `decide` method receives the `max_attempts` parameter
but ignores it when making recovery decisions, while the caller in
`src/engine/bare/dispatch.rs` also enforces a separate cap, causing potential
policy drift. In the decision logic where you check `attempt >=
self.max_retries`, extend this condition to also account for the `max_attempts`
parameter so that both limits are considered in one place, preventing the
strategy from emitting Retry decisions that would be immediately discarded by
the caller.
🪄 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: b61b92e1-628b-4bc9-bf39-cc9cd7a1a0c1
📒 Files selected for processing (65)
README.mdsrc/api.rssrc/api/error.rssrc/builder/error.rssrc/builtin.rssrc/builtin/observer.rssrc/cancel.rssrc/capabilities.rssrc/compact.rssrc/compact/truncating.rssrc/compact/types.rssrc/config.rssrc/core.rssrc/core/agent_core.rssrc/core/agent_memory.rssrc/core/agent_observer.rssrc/core/types.rssrc/detection.rssrc/detection/convergence.rssrc/detection/loop_detector.rssrc/detection/manager.rssrc/engine.rssrc/engine/bare.rssrc/engine/bare/compact.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/bare/stream.rssrc/engine/loop_core.rssrc/error.rssrc/fallback.rssrc/hooks.rssrc/hooks/builtin/auto_commit.rssrc/hooks/context.rssrc/hooks/executor.rssrc/lib.rssrc/loop_control.rssrc/loop_control/bundle.rssrc/memory.rssrc/memory/builtin.rssrc/memory/entry.rssrc/message.rssrc/middleware.rssrc/middleware/output_limit.rssrc/middleware/permission.rssrc/middleware/timeout.rssrc/middleware/tool_call.rssrc/middleware/unknown_tool.rssrc/observability.rssrc/observability/console.rssrc/observability/event.rssrc/observability/sink.rssrc/observer.rssrc/observer/context.rssrc/reflection.rssrc/reflection/backoff.rssrc/runtime.rssrc/stream.rssrc/stream/handler.rssrc/stream/heartbeat.rssrc/testing.rssrc/tool.rssrc/tool/health.rssrc/tool/permission.rssrc/tool/registry.rssrc/tool/shield.rs
💤 Files with no reviewable changes (14)
- src/core/types.rs
- src/observability.rs
- README.md
- src/builtin.rs
- src/observability/event.rs
- src/observability/sink.rs
- src/core/agent_memory.rs
- src/core.rs
- src/observability/console.rs
- src/builtin/observer.rs
- src/core/agent_core.rs
- src/loop_control/bundle.rs
- src/core/agent_observer.rs
- src/loop_control.rs
…n edge cases, fix clippy pedantic warnings, update ci
…d config validation, remove dead recovery limit, clean up docs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/compact/truncating.rs (1)
118-163: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftHonor the requested token budget before reporting success.
compact()never usestarget_tokens, and this implementation always returnssuccess: trueeven whenpreserve_recentplus the preserved first message still exceeds the caller’s budget. With the new tool-pair backtracking, that preserved slice can grow further, so the loop can keep sending oversized contexts while compaction claims it succeeded.🤖 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/compact/truncating.rs` around lines 118 - 163, The compact() implementation in Truncating compaction ignores the requested target_tokens and can return success even when the preserved messages still exceed the budget. Update compact() to compare the estimated tokens for the final preserved slice against target_tokens, and only report success when the slice fits; otherwise continue trimming or return a failed CompactionOutcome as appropriate. Keep the existing adjust_for_tool_pairs and first-message preservation logic, but ensure the final CompactionOutcome fields tokens_after, tokens_saved, and success reflect the actual budget result.src/memory/builtin.rs (1)
248-283: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFilter out non-matching memories before scoring.
retrieve("unrelated", limit)currently returns entries anyway because every entry gets the baseline score; an empty query can also tag-match viacontains(""). Return an empty result for empty/no-match queries so callers only receive relevant memory.Suggested fix
- let query = query.to_string(); + let query = query.trim().to_string(); Box::pin(async move { let query_lower = query.to_lowercase(); let query_words: Vec<&str> = query_lower.split_whitespace().collect(); + if limit == 0 || query_words.is_empty() { + return Ok(Vec::new()); + } let entries = self.entries.read().unwrap_or_else(PoisonError::into_inner); let mut scored: Vec<(f32, MemoryEntry)> = entries .iter() - .map(|entry| { + .filter_map(|entry| { let memory_lower = entry.memory.to_lowercase(); let tag_match = entry .tags .iter() .any(|t| t.to_lowercase().contains(&query_lower)); let word_matches = query_words .iter() .filter(|w| memory_lower.contains(*w)) .count(); + if word_matches == 0 && !tag_match { + return None; + } let base_score = entry.relevance; #[allow(clippy::cast_precision_loss)] let query_bonus = if word_matches > 0 { word_matches as f32 / query_words.len().max(1) as f32 } else { 0.0 }; let tag_bonus = if tag_match { 0.3 } else { 0.0 }; - ( + Some(( base_score * 0.5 + query_bonus * 0.4 + tag_bonus + 0.1, entry.clone(), - ) + )) }) .collect();🤖 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/memory/builtin.rs` around lines 248 - 283, The retrieval logic in builtin memory scoring is too permissive because every entry gets a baseline score in the `retrieve` flow, and `contains("")` can make tag matching succeed for empty queries. Update the `retrieve` implementation to short-circuit empty queries and filter out entries that have neither a tag match nor any word match before scoring, so `MemoryEntry` results are only returned from relevant matches.
🤖 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 `@examples/chat.rs`:
- Around line 252-258: Reject malformed arithmetic in simple_eval by ensuring
the full token stream is consumed after parse_expr, and treat any leftover
tokens as an error instead of formatting the parsed prefix. Also tighten
parse_factor so an opening parenthesis must be matched by a closing parenthesis
before returning a value; if it is missing, return an error. Update the
simple_eval flow to surface these parse failures clearly using the existing
parse_expr and parse_factor paths.
In `@README.md`:
- Around line 80-112: The runnable README examples using BareLoop currently call
run without bringing the Loop trait into scope, so uncomments will fail to
compile. Update the example imports near BareLoop and ToolRegistry to include
loopctl::engine::Loop, matching the explicit trait import used in
examples/repl-cli.rs, so the agent.run(...) calls resolve correctly.
- Around line 142-150: The Feature Flags table in README.md is outdated and is
missing the newly added provider-related flags. Update the table to include the
new flags defined in Cargo.toml—providers, openai, anthropic, ollama, deepseek,
grok, gemini, and zai—so readers can discover the corresponding modules and
examples; keep the existing entries and align the new rows with the style used
by hooks, testing, tool_health, and tool_shield.
In `@src/api/error.rs`:
- Around line 1309-1314: The test_result_type helper currently returns String,
so it no longer validates the crate’s public Result<T> alias. Update
test_result_type in src/api/error.rs to use the Result<T> alias from the module
being tested, with a path that keeps the alias in the function signature and
return value so the test actually covers it. Keep the assertion behavior the
same, but make sure the referenced type is the public Result<T> alias rather
than std::string::String.
In `@src/config.rs`:
- Around line 132-135: The config validation in the model check still lets
whitespace-only values through because `is_empty()` is used directly. Update the
validation in `src/config.rs` around the `self.model` guard to trim the value
before checking so `" "` is rejected the same as an empty string. Also add a
regression test covering a whitespace-only model name to verify the `Config`
validation fails as expected.
In `@src/detection/convergence.rs`:
- Around line 67-74: The doc comment in convergence detection is inaccurate
about how Jaccard works in `compute_similarity`/`ConvergenceDetector`: word
reordering does not change set-based similarity, and punctuation sensitivity is
overstated because non-alphanumeric characters are normalized before
tokenization. Update the bullet list in `src/detection/convergence.rs` to keep
the semantic-blindness note, remove or reword the word-order limitation, and
adjust the punctuation/whitespace note to reflect the actual normalization
behavior.
- Around line 567-593: The streak logic in the response handling path is using
any_similar across the entire window, so non-consecutive matches can incorrectly
increment the consecutive count. Update the add_response logic in the
convergence detector to base streak extension only on the immediately previous
response (using the existing similarity check and window ordering), and keep
resetting similar_responses/consecutive_count when the last response is not
similar rather than when any earlier item in the window matches.
In `@src/engine/bare/emission.rs`:
- Around line 60-64: The terminal reason mapping in the emission path is too
coarse because `SessionEndReason` can be `Cancelled`, `MaxTurns`, or
`ContextOverflow`, but the current logic in the session-ending hook context
reduces every failure to `Error`. Update the `emit`/hook-context flow in
`bare::emission` so it receives and forwards the actual terminal reason from the
session result instead of deriving it from `result.success`, and make sure the
`SessionEndReason` value is preserved when constructing the hook context.
In `@src/engine/loop_core.rs`:
- Around line 365-377: The InputFix handling in `CorrectionType::InputFix`
currently accepts any `modified_input` value, but `ToolCall::input` must remain
a JSON object. Update this branch in `loop_core.rs` to validate
`correction.modified_input` is an object before assigning it to `self.input`,
and return `crate::reflection::CorrectionResult::Failed` when it is a scalar,
array, or missing. Keep the existing `tracing::debug!` and `self.input =
modified.clone()` path only for valid object payloads.
In `@src/middleware.rs`:
- Around line 1202-1209: The multipart test currently uses if let on
ToolContentPart::Text, which can silently succeed when a part has the wrong
variant. Update the multipart assertion block in the test around
ToolContent::Multipart to use match on parts[0] and parts[1] so any non-Text
variant causes an immediate test failure, while still asserting the expected
text values for the Text case.
In `@src/middleware/output_limit.rs`:
- Around line 65-74: The multipart handling in OutputLimit middleware currently
truncates each Text part independently, so the total tool output can still
exceed max_chars. Update ToolContent::Multipart processing in output_limit.rs to
track a shared remaining character budget across all parts, decrementing it as
you append/truncate each ToolContentPart::Text. Use the existing
ToolContent::Multipart and Text part loop to enforce one global limit for the
entire multipart payload, not per-part truncation.
---
Outside diff comments:
In `@src/compact/truncating.rs`:
- Around line 118-163: The compact() implementation in Truncating compaction
ignores the requested target_tokens and can return success even when the
preserved messages still exceed the budget. Update compact() to compare the
estimated tokens for the final preserved slice against target_tokens, and only
report success when the slice fits; otherwise continue trimming or return a
failed CompactionOutcome as appropriate. Keep the existing adjust_for_tool_pairs
and first-message preservation logic, but ensure the final CompactionOutcome
fields tokens_after, tokens_saved, and success reflect the actual budget result.
In `@src/memory/builtin.rs`:
- Around line 248-283: The retrieval logic in builtin memory scoring is too
permissive because every entry gets a baseline score in the `retrieve` flow, and
`contains("")` can make tag matching succeed for empty queries. Update the
`retrieve` implementation to short-circuit empty queries and filter out entries
that have neither a tag match nor any word match before scoring, so
`MemoryEntry` results are only returned from relevant matches.
🪄 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: 490d276b-0f94-4411-9ebe-0f7f45eaf3ed
📒 Files selected for processing (54)
.github/workflows/ci.ymlCargo.tomlMakefileREADME.mdexamples/chat.rsexamples/echo-tool-cli.rsexamples/hello-cli.rsexamples/repl-cli.rssrc/api.rssrc/api/error.rssrc/builder.rssrc/builder/error.rssrc/builder/features.rssrc/cancel.rssrc/capabilities.rssrc/compact.rssrc/compact/truncating.rssrc/config.rssrc/detection/convergence.rssrc/detection/loop_detector.rssrc/detection/manager.rssrc/engine/bare.rssrc/engine/bare/compact.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/bare/message.rssrc/engine/bare/stream.rssrc/engine/loop_core.rssrc/fallback.rssrc/hooks/executor.rssrc/lib.rssrc/memory.rssrc/memory/builtin.rssrc/message.rssrc/middleware.rssrc/middleware/output_limit.rssrc/middleware/permission.rssrc/middleware/timeout.rssrc/middleware/unknown_tool.rssrc/provider.rssrc/provider/anthropic.rssrc/provider/gemini.rssrc/provider/openai.rssrc/reflection.rssrc/reflection/backoff.rssrc/runtime.rssrc/stream.rssrc/stream/handler.rssrc/stream/heartbeat.rssrc/testing.rssrc/tool.rssrc/tool/health.rssrc/tool/registry.rssrc/tool/shield.rs
💤 Files with no reviewable changes (3)
- src/builder/features.rs
- src/builder.rs
- src/builder/error.rs
🚧 Files skipped from review as they are similar to previous changes (13)
- src/engine/bare/compact.rs
- src/message.rs
- src/middleware/timeout.rs
- src/engine/bare/stream.rs
- src/capabilities.rs
- src/api.rs
- src/middleware/permission.rs
- src/middleware/unknown_tool.rs
- src/fallback.rs
- src/compact.rs
- src/detection/manager.rs
- src/engine/bare/dispatch.rs
- src/detection/loop_detector.rs
…atch, fallback attempt padding, docs in tests
Contributor
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 (2)
src/api.rs (1)
160-163: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the
messagesdoc oncreate_message. The'staticstream note is copied fromstream_messages;create_messagereturns a+ '_future, so this should describe taking ownership for an owned request body instead.🤖 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/api.rs` around lines 160 - 163, The `create_message` doc comment in `api::create_message` is using the wrong ownership rationale: remove the `stream_messages`-specific `'static` stream note and rewrite it to explain that `messages` is taken by ownership because the request body must be built from owned data for the returned `+ '_` future. Keep the focus on `create_message` and its `messages` parameter, and make sure the wording no longer mentions the stream lifetime requirement.src/memory.rs (1)
144-172: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the
Arc<dyn LoopMemory>claim or switch back to boxed futures
impl Futureis fine on rust-version 1.85, but it makesLoopMemorynon-dyn-compatible, so theArc<dyn LoopMemory>wording insrc/memory/builtin.rsis misleading. If trait objects are required, these methods need boxed futures; otherwise update the docs/examples toArc<InMemoryStore>orArc<impl LoopMemory>.🤖 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/memory.rs` around lines 144 - 172, The `LoopMemory` trait methods `store`, `retrieve`, and `consolidate` use `impl Future`, which makes the trait not dyn-compatible, so the `Arc<dyn LoopMemory>` wording is misleading. Either switch the trait methods back to boxed futures if trait objects are required, or update the `LoopMemory` docs/examples (including the references in `memory/builtin.rs`) to use `Arc<impl LoopMemory>` or a concrete type like `Arc<InMemoryStore>`.
🧹 Nitpick comments (1)
src/memory/builtin.rs (1)
250-280: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winKeep the read lock scoped to the snapshot only.
retrievedoes O(n) scoring plus O(n log n) sorting while the read guard remains in scope. Snapshot the entries under the lock, then score/sort outside it sostore/consolidateare blocked for less time.Suggested refactor
- let entries = self.entries.read().unwrap_or_else(PoisonError::into_inner); + let entries = { + let entries = self.entries.read().unwrap_or_else(PoisonError::into_inner); + entries.clone() + }; let mut scored: Vec<(f32, MemoryEntry)> = entries .iter()🤖 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/memory/builtin.rs` around lines 250 - 280, The `retrieve` logic in `builtin.rs` holds the `self.entries` read guard while doing the full scoring and sorting work, which unnecessarily blocks writers. Update the `retrieve` path to first take a snapshot of the entries from `self.entries` under the lock, then drop the guard before the O(n) scoring and `scored.sort_by` work; keep the rest of the ranking and `take(limit)` behavior unchanged.
🤖 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/api.rs`:
- Around line 160-163: The `create_message` doc comment in `api::create_message`
is using the wrong ownership rationale: remove the `stream_messages`-specific
`'static` stream note and rewrite it to explain that `messages` is taken by
ownership because the request body must be built from owned data for the
returned `+ '_` future. Keep the focus on `create_message` and its `messages`
parameter, and make sure the wording no longer mentions the stream lifetime
requirement.
In `@src/memory.rs`:
- Around line 144-172: The `LoopMemory` trait methods `store`, `retrieve`, and
`consolidate` use `impl Future`, which makes the trait not dyn-compatible, so
the `Arc<dyn LoopMemory>` wording is misleading. Either switch the trait methods
back to boxed futures if trait objects are required, or update the `LoopMemory`
docs/examples (including the references in `memory/builtin.rs`) to use `Arc<impl
LoopMemory>` or a concrete type like `Arc<InMemoryStore>`.
---
Nitpick comments:
In `@src/memory/builtin.rs`:
- Around line 250-280: The `retrieve` logic in `builtin.rs` holds the
`self.entries` read guard while doing the full scoring and sorting work, which
unnecessarily blocks writers. Update the `retrieve` path to first take a
snapshot of the entries from `self.entries` under the lock, then drop the guard
before the O(n) scoring and `scored.sort_by` work; keep the rest of the ranking
and `take(limit)` behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fb053996-9172-4ac9-8a11-ea0b9da608fd
📒 Files selected for processing (27)
Cargo.tomlsrc/api.rssrc/api/error.rssrc/compact/truncating.rssrc/detection/loop_detector.rssrc/detection/manager.rssrc/engine/bare.rssrc/engine/bare/dispatch.rssrc/engine/loop_core.rssrc/fallback.rssrc/memory.rssrc/memory/builtin.rssrc/message.rssrc/middleware.rssrc/middleware/unknown_tool.rssrc/observer.rssrc/observer/context.rssrc/provider.rssrc/provider/anthropic.rssrc/provider/gemini.rssrc/provider/openai.rssrc/reflection.rssrc/stream/handler.rssrc/testing.rssrc/tool/health.rssrc/tool/registry.rssrc/tool/shield.rs
💤 Files with no reviewable changes (5)
- src/compact/truncating.rs
- src/message.rs
- src/middleware.rs
- src/detection/loop_detector.rs
- src/api/error.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- Cargo.toml
- src/middleware/unknown_tool.rs
- src/engine/loop_core.rs
- src/engine/bare/dispatch.rs
- src/fallback.rs
Contributor
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/detection/convergence.rs (1)
533-536: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc no longer matches the new streak logic.
The updated
add_responsederives the streak from the immediately previous response (window.back()), not "any comparison" across the whole window, and it uses>=(not "exceeds"). This doc describes the old behavior and will mislead callers.📝 Proposed doc fix
- /// Primary entry point. The response is compared against - /// every prior response in the window. If any comparison exceeds - /// [`ConvergenceConfig::similarity_threshold`], the consecutive count - /// is incremented; otherwise it resets to `1`. + /// Primary entry point. The response is compared against the + /// immediately previous response in the window. If that comparison + /// meets or exceeds [`ConvergenceConfig::similarity_threshold`], the + /// consecutive count is incremented; otherwise it resets to `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/detection/convergence.rs` around lines 533 - 536, Update the doc comment on the response-streak logic in convergence.rs to match the current add_response behavior: it should describe comparing only against the immediately previous response via window.back(), and state that the streak increments when similarity is at or above ConvergenceConfig::similarity_threshold rather than “any comparison” or “exceeds.” Keep the wording aligned with the actual streak reset/increment behavior so the documentation for add_response and the surrounding convergence window logic is accurate.
🤖 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/detection/convergence.rs`:
- Around line 533-536: Update the doc comment on the response-streak logic in
convergence.rs to match the current add_response behavior: it should describe
comparing only against the immediately previous response via window.back(), and
state that the streak increments when similarity is at or above
ConvergenceConfig::similarity_threshold rather than “any comparison” or
“exceeds.” Keep the wording aligned with the actual streak reset/increment
behavior so the documentation for add_response and the surrounding convergence
window logic is accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 68559c09-6ab9-4876-957c-589668963aa2
📒 Files selected for processing (16)
README.mdexamples/chat.rssrc/api/error.rssrc/config.rssrc/detection/convergence.rssrc/engine/bare.rssrc/engine/bare/dispatch.rssrc/engine/bare/emission.rssrc/engine/loop_core.rssrc/lib.rssrc/middleware.rssrc/middleware/output_limit.rssrc/middleware/tool_call.rssrc/provider/anthropic.rssrc/provider/gemini.rssrc/provider/openai.rs
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/config.rs
- src/engine/bare/emission.rs
- src/lib.rs
- examples/chat.rs
- src/engine/loop_core.rs
- src/engine/bare/dispatch.rs
- src/api/error.rs
- src/middleware.rs
This was referenced Jul 17, 2026
Merged
bobrykov
added a commit
that referenced
this pull request
Aug 18, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.