state machine refactor - #62
Conversation
|
@CodeRabbit review |
📝 WalkthroughWalkthroughChangesEngine lifecycle and configuration
Sequence Diagram(s)sequenceDiagram
participant Caller
participant BareLoop
participant LoopMachine
participant Provider
participant Tools
Caller->>BareLoop: run(input, &RunConfig)
BareLoop->>LoopMachine: accept_input
LoopMachine-->>BareLoop: MachineStep
BareLoop->>Provider: stream request
Provider-->>BareLoop: model response
BareLoop->>LoopMachine: model_response
LoopMachine-->>BareLoop: CallTools or Done
BareLoop->>Tools: execute tool calls
Tools-->>BareLoop: tool results
BareLoop->>LoopMachine: tool_results
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: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/structured.rs (1)
343-358: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate the companion tests with the same feature predicates.
The
#[cfg(test)]module contains unconditionalparse_json_lenient_*tests that call theopenai/gemini-gated helpers, andtighten_*tests that callanthropic/grammar/openai/gemini-gated helpers. Without those provider features, the test target references undefined symbols and fails to compile.🤖 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/structured.rs` around lines 343 - 358, Gate the companion tests in the #[cfg(test)] module with feature predicates matching the helpers they exercise: apply openai/gemini gating to the parse_json_lenient_* tests, and anthropic/grammar/openai/gemini gating to the tighten_* tests. Keep unrelated tests unchanged.src/provider/openai.rs (1)
1171-1199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute tool-call deltas out of the text slot.
TEXT_PART_INDEXis0, and OpenAI tool-call indexes are also 0-based, so a response with streamed text followed bytool_calls[0]emitsIndexedDeltawithindex == 0while the text slot is still open.StreamAccumulatorthen delivers those tool argument fragments to the text lane instead of the toolcall slot and the tool input is lost. Use a disjoint OpenAI tool-call part range forPartStart/IndexedDelta, and handleid/nameemission even if the first fragment has no function fields.🤖 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/openai.rs` around lines 1171 - 1199, Update process_tool_call to map OpenAI tool-call indexes into a range disjoint from TEXT_PART_INDEX before emitting PartStart and IndexedDelta, using the mapped index consistently for both events. Ensure tool-call id and name metadata are emitted or updated when later fragments provide them, even if the first fragment lacks function fields, while preserving argument accumulation for every fragment.src/tool/health.rs (1)
572-591: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winLock-poisoning handling is inconsistent and can wedge every tool closed.
allow_requestreturnsfalseon a poisoned lock, so a single panic while the guard is held permanently blocks the tool — whilerecord_success/record_failuresilently no-op (never clearing it) andis_open/state_labelreport "closed". The rest of this file (e.g.get_stats,get_circuit_breaker,tool_count) already recovers viacrate::error::recover_guard; use the same helper here so the breaker degrades consistently instead of failing permanently closed.🔒 Proposed fix (pattern for each accessor)
- let Ok(mut state) = self.state.lock() else { - return false; - }; + let mut state = crate::error::recover_guard(self.state.lock());Also applies to: 598-602, 610-612, 634-687
🤖 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/tool/health.rs` around lines 572 - 591, Update the circuit-breaker state accessors, including allow_request, record_success, record_failure, is_open, state_label, and related methods in the referenced ranges, to recover poisoned mutex guards with crate::error::recover_guard instead of returning false, silently no-oping, or reporting stale state. Preserve each method’s existing behavior after obtaining the recovered guard so a poisoned lock does not permanently fail closed.
🟡 Minor comments (18)
src/hooks/builtin/auto_commit.rs-578-582 (1)
578-582: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDocument empty allow-lists as rejected.
with_files(Vec::new())now reachesstage_files, which returns an error; it no longer stages every working-tree change. Align this builder documentation with the safe behavior.🤖 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/builtin/auto_commit.rs` around lines 578 - 582, Update the documentation for the with_files builder near AutoCommitConfig::files to state that an empty file allow-list is rejected by stage_files, rather than staging every working-tree change. Remove the outdated claim that an empty vector stages all changes.src/hooks/executor.rs-190-199 (1)
190-199: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDescribe the actual abort short-circuit.
check_pre_compactreturns at the firstabortresult, so later hooks do not run. Update “all hooks run”/context accumulation wording to apply only until the first 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 `@src/hooks/executor.rs` around lines 190 - 199, Update the documentation for check_pre_compact to state that hooks execute in registration order only until the first CompactResult with abort: true, which is returned immediately; clarify that result merging and additional_context accumulation apply only before that abort.src/hooks/builtin/auto_commit.rs-489-493 (1)
489-493: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the tracked-path source.
The hook records
file_pathfromPostToolUseContext::input, as demonstrated byhook_tracks_tracked_tools; it does not inspect tool output. Update this documentation accordingly.🤖 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/builtin/auto_commit.rs` around lines 489 - 493, Update the documentation for the tracked-tools configuration near hook_tracks_tracked_tools to state that file_path is read from PostToolUseContext::input, not tool output; preserve the description that matching tools’ touched files are recorded and staged at session end.tests/constrained_decode.rs-49-63 (1)
49-63: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMisleading "not counted in rate" message — errors actually lower the rate.
totalis incremented before the stream attempt and never decremented on error, so errored attempts are included in therate = valid/totaldenominator. The subsequent log line claims errors are"not counted in rate", which is incorrect — they count as failures and can silently pull the strict-mode pass rate below the 90% threshold.🐛 Proposed fix: exclude errors from the denominator
None => { errors += 1; + total -= 1; println!(Also applies to: 69-81, 102-109
🤖 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 `@tests/constrained_decode.rs` around lines 49 - 63, Update the constrained decode accounting around the loop’s total, errors, and rate calculations so errored stream attempts are excluded from the rate denominator. Adjust total consistently in all corresponding paths (including the sections noted at 69–81 and 102–109), while preserving valid counting and error reporting; ensure the “not counted in rate” message matches the resulting behavior.src/presets.rs-25-27 (1)
25-27: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPass the constrained run config in the quick-start.
The example only applies
session_config(), so callers retainRunConfig::default()rather than the profile’s intendedmax_turns = 100. Showagent.run(..., &ConstrainedProfile::run_config())in the complete usage path.🤖 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/presets.rs` around lines 25 - 27, Update the BareLoop quick-start example to pass ConstrainedProfile::run_config() to agent.run(...) in the complete usage path, while retaining the existing session_config(), request_options(), and apply setup.CHANGELOG.md-24-24 (1)
24-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDuplicated
Runin the lifetime-type wording. Both entries listRuntwice where a distinct type name belongs, which makes the migration guide ambiguous about what replacesSessionResult.
CHANGELOG.md#L24-L24: changeSession`/`Run`/`Turn`/`RuntoSession`/`Run`/`Turn.CHANGELOG.md#L229-L231: replacethe new `Run`/`Run` (`engine::core`)with the single type that supersedesSessionResult.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@CHANGELOG.md` at line 24, The changelog repeats `Run` and must identify the distinct type replacing `SessionResult`. In `CHANGELOG.md` lines 24-24, remove the duplicated final `Run`; in lines 229-231, replace `the new Run/Run` with the single correct superseding type name.README.md-35-35 (1)
35-35: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale
runtimemodule link and label.runtimeis no longer a module undersrc/lib.rs, andLoopManagersis exposed viasrc/managers.rs; change the README label/link tomanagers.🤖 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` at line 35, Update the README table entry currently labeled runtime to use managers, linking to the managers documentation page instead, while retaining the LoopManagers description.src/stream.rs-1083-1113 (1)
1083-1113: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winError path leaves the slot half-consumed. When
serde_json::from_strfails,tool_inputhas already been moved out bymem::takeandself.open.remove(0)is skipped, so the slot survives with an empty buffer; a laterPartStopwould flush it as a tool call with{}input. Pop the slot before parsing (or before returning the error) so a failed flush cannot resurface as a bogus part.🐛 Proposed fix
StreamEvent::PartStop => { - let Some(slot) = self.open.first_mut() else { + if self.open.is_empty() { return Ok(()); - }; + } + let mut slot = self.open.remove(0); let flushed = match slot.kind { @@ - self.open.remove(0); if let Some(part) = flushed {🤖 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/stream.rs` around lines 1083 - 1113, Update the StreamEvent::PartStop branch to remove the first open slot before parsing tool input, while preserving access to that slot’s fields for flushing. Ensure serde_json::from_str failures return InvalidToolInputJson without leaving the consumed slot in self.open, and keep successful text/tool part completion behavior unchanged.src/engine/bare/emission.rs-66-77 (1)
66-77: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
MaxTurnscan shadow a normal completion, andContextOverflowis now unreachable.A run that legitimately finishes on its last allowed turn satisfies
turn_count() >= max_turnsand is reported asMaxTurnsrather thanComplete. The machine already knows the authoritative terminal outcome (MachineOutcome::MaxTurnsExceeded/Completed); deriving the reason from it would be exact. Also noteSessionEndReason::ContextOverflowis no longer produced by any branch.🤖 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 66 - 77, The session_end_reason method should derive the terminal reason from the machine’s authoritative outcome rather than comparing turn_count against max_turns. Map MachineOutcome::MaxTurnsExceeded to MaxTurns and MachineOutcome::Completed to Complete, while preserving cancellation and error precedence; ensure ContextOverflow remains handled if the outcome supports it.src/engine/core/lifecycle.rs-221-245 (1)
221-245: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc blocks left attached to the wrong items after the
Run/Turn/RunResultsplit. In both places the rustdoc for one item now precedes a different item, so the published docs describe the wrong type — including a dangling sentence referring toRunas "a type alias forRun".
src/engine/core/lifecycle.rs#L221-L245: drop the run-focused paragraphs and doctest sostruct Turnkeeps only its own description (238-243).src/engine/core/lifecycle.rs#L572-L614: move the "core agent lifecycle trait" prose and theimpl Loop for MyAgentexample offpub type RunResultand ontopub trait Loop, leaving the one-line alias doc in place.🤖 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/core/lifecycle.rs` around lines 221 - 245, The documentation at src/engine/core/lifecycle.rs lines 221-245 should remove the misplaced Run-focused paragraphs and doctest, leaving Turn’s own description attached to struct Turn. At src/engine/core/lifecycle.rs lines 572-614, keep only the one-line alias documentation on RunResult and move the core agent lifecycle prose and impl Loop for MyAgent example onto the pub trait Loop documentation.src/engine/bare/emission.rs-42-56 (1)
42-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winObservers lose the failure detail.
erroris now the constant"run failed"for every failure, soon_session_endconsumers can no longer distinguish cancellation from an API/tool failure. Consider threading the terminalLoopError(or its string) intonotify_session_endinstead of a boolean-derived placeholder.🤖 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 42 - 56, The notify_session_end flow currently replaces every failure with the placeholder "run failed"; update its callers and signature to thread the terminal LoopError or its string into SessionEndContext.error. Preserve None for successful sessions and pass the actual failure detail to observers, while keeping notify_session_end_hook behavior intact.src/config.rs-158-169 (1)
158-169: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThis "side-effect divergence" section contradicts the unified dispatch path in this PR.
dispatch.rsnow routes both modes throughexecute_tool_call, which firesnotify_tool_pre/notify_tool_postand pre/post detection on every attempt in both modes — its own doc states there is "no divergence in side-effect granularity, observer event counts, or recovery behaviour between modes". This section claims the opposite forParallel, so callers will mis-model observer and detector semantics.🤖 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/config.rs` around lines 158 - 169, Update the “Side-effect divergence from Sequential” documentation in the relevant configuration comments to reflect the unified execute_tool_call path: both Sequential and Parallel modes trigger detection and observer pre/post events on every retry attempt, with no side-effect granularity or recovery-behavior divergence. Retain the distinction for health tracking only if it remains accurate.src/engine/bare/dispatch.rs-325-337 (1)
325-337: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winFallback result loses the
tool_call_idcorrelation.If a slot is ever unfilled, the synthesized soft error carries an empty
tool_call_id/resolved_tool_name, which cannot be paired back to the model's tool call. Sinceidxis known here, populate it from the originating call.🛠️ Proposed fix
- Ok(results - .into_iter() - .map(|r| { - r.unwrap_or_else(|| ToolDispatchResult { - tool_call_id: String::new(), - output: ToolContent::Text("dispatch produced no result".to_string()), - is_error: true, - duration: Duration::ZERO, - resolved_tool_name: String::new(), - display_hint: None, - }) - }) - .collect()) + Ok(results + .into_iter() + .enumerate() + .map(|(idx, r)| { + r.unwrap_or_else(|| { + let call = tool_calls.get(idx); + ToolDispatchResult { + tool_call_id: call.map(|c| c.id.clone()).unwrap_or_default(), + output: ToolContent::Text("dispatch produced no result".to_string()), + is_error: true, + duration: Duration::ZERO, + resolved_tool_name: call.map(|c| c.tool.clone()).unwrap_or_default(), + display_hint: None, + } + }) + }) + .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/engine/bare/dispatch.rs` around lines 325 - 337, Update the fallback ToolDispatchResult construction in the result-mapping closure to use the originating call identified by idx, preserving its tool_call_id and resolved_tool_name instead of empty strings. Keep the existing soft-error output and other fallback fields unchanged.src/engine/bare/stream.rs-50-59 (1)
50-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAppend contributor messages after full history before streaming.
context contributormessages are described as turn-boundary context added at the top of a turn, but the current stream request starts with those messages before any existing history. Anthropic/Gemini foldRole::Systeminto top-level system fields, but OpenAI emitsRole::Systemas an inline message in array order, so an OpenAI-backed turn can receive the reminder after prior history.🔧 Proposed fix: append contributor messages after history
- let mut messages = contributor_messages; - messages.extend(self.machine.full_history()); + let mut messages = self.machine.full_history(); + messages.extend(contributor_messages);🤖 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/stream.rs` around lines 50 - 59, Update stream_turn so the existing machine.full_history() is placed into messages before contributor_messages are appended, ensuring context contributor messages occur at the end of the turn-boundary request. Preserve the existing StreamRequest construction and tool/system configuration.src/engine/bare.rs-334-338 (1)
334-338: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPlaceholder
Runis never consumed, sosession.runscarries a bogus empty entry.Constructors push
Run::default()andrun()unconditionally pushes anotherRun, so after the first runsession.runs.len()is 2 for one real run, and cross-run accounting (Session::total_turns,total_input_tokens, per-run iteration) includes the placeholder. Consider replacing the placeholder on the firstrun()(e.g. pop it when it is the untouched default) or dropping the placeholder entirely and makingcurrent_run()genuinely optional.Also applies to: 1699-1699
🤖 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 334 - 338, The session constructor’s initial Run::default() is retained as a bogus entry while run() unconditionally appends real runs. Remove the placeholder initialization and update current_run() and related session logic to handle an empty runs collection as optional, or otherwise ensure the first run replaces the untouched placeholder so totals and per-run iteration include only real runs.src/compact.rs-188-192 (1)
188-192: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winDoc formulas still use the old fixed-point scale (
/ 10_000) and omit the divisor.The code now divides by
100throughout. Concretely: Line 188 sayscompact_threshold_tokens × compact_target_pct / 10_000, Line 374 sayscontext_window * threshold / 10_000, Line 408 says "Equal tocontext_window * threshold", and Lines 421-422 say× pctwith no divisor.📝 Suggested doc corrections
- /// `target = compact_threshold_tokens × compact_target_pct / 10_000` + /// `target = compact_threshold_tokens × compact_target_pct / 100`/// Compaction triggers when estimated tokens reach - /// `context_window * threshold / 10_000`. See [`with_threshold`](Self::with_threshold). + /// `context_window * threshold / 100`. See [`with_threshold`](Self::with_threshold).- /// Equal to `context_window * threshold`. + /// Equal to `context_window * threshold / 100`.- /// - [`CompactBase::Threshold`]: `compact_threshold_tokens × pct` - /// - [`CompactBase::Context`]: `context_window × pct` + /// - [`CompactBase::Threshold`]: `compact_threshold_tokens × pct / 100` + /// - [`CompactBase::Context`]: `context_window × pct / 100`Also applies to: 371-374, 406-413, 416-422
🤖 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 188 - 192, Update the documentation formulas and explanatory text in the compact configuration documentation to match the implementation’s percent scale: use /100 wherever threshold or target percentage calculations are shown, including the formulas near the default description, context-window calculation, and target-token documentation. Ensure the stated default example remains mathematically consistent with the corrected formulas.src/engine/bare/compact.rs-51-58 (1)
51-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse a real token estimate for no-op compaction branches.
On the no
ContextManager, pre-compact abort, andNoActionpaths,run_compactionreturns zero, andLoopMachine::compaction_resultadopts that ascontext_tokens. That leaves the machine thinking the context is empty whilehistorystill contains the original messages, so normal auto-compaction can stop emitting compact requests until the next model response refreshes it; an aborting pre-compact hook can also report as a successful compaction. Preserve the pre-compaction estimate for these branches, or pass through the original token 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/engine/bare/compact.rs` around lines 51 - 58, Update run_compaction’s no-op branches—missing context_manager, pre_compact_hook_aborts, and NoAction—to return the pre-compaction token estimate instead of zero. Preserve the original history and ensure the result communicates an aborting hook as unsuccessful rather than a successful zero-token compaction.src/cancel.rs-251-257 (1)
251-257: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
interval.tick()returns immediately on the first call, so this test barely waits.
tokio::time::intervalfires its first tick instantly, sopending.tick().awaitadds ~no delay before theis_finished()assertion — the test would pass even ifnotified()resolved a moment later. Usetokio::time::sleep(Duration::from_millis(5)).await(or tick twice) to actually give the spawned task a window.🧪 Proposed fix
- let mut pending = tokio::time::interval(std::time::Duration::from_millis(5)); - pending.tick().await; + tokio::time::sleep(std::time::Duration::from_millis(5)).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/cancel.rs` around lines 251 - 257, Replace the first-tick wait in the cancellation test with an actual 5 ms delay, using tokio::time::sleep before the handle.is_finished() assertion. Remove the unnecessary interval setup while preserving the assertion that reset prevents notification until a new cancellation.
🧹 Nitpick comments (12)
Cargo.toml (2)
31-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider gating
bytesbehindproviders.bytes::Bytesis only used on the SSE/provider path; as an unconditional dependency it lands in every build, including feature-less consumers.bytes = { version = "1", optional = true }plusproviders = [..., "dep:bytes"]keeps the base dependency set unchanged.🤖 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` at line 31, Make the bytes dependency optional in Cargo.toml and add dep:bytes to the providers feature, ensuring bytes::Bytes remains available on the SSE/provider path while feature-less builds do not include it.
54-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNew
xaifeature isn't documented. The README feature table listsgrokbut not this alias; add a row so the two names don't drift.🤖 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` at line 54, Document the new xai feature alias in the README feature table by adding a row alongside grok, ensuring both feature names and their descriptions remain aligned.src/testing.rs (1)
635-643: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale parameter names in the doc above. The rustdoc for this method still refers to
_messages,_system, and_tools; the signature now takes a single_request.🤖 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/testing.rs` around lines 635 - 643, Update the rustdoc example above stream_messages to remove references to the obsolete _messages, _system, and _tools parameters, and document the current single _request argument consistently with the method signature.src/compact/truncating.rs (1)
56-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueWording is off by one. Both
compactandTokenSplitter::splitskip whenmessages.len() <= min_messages, so a conversation with exactlymin_messagesis also left untouched — "fewer than this" reads as exclusive.Also applies to: 106-108
🤖 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 56 - 61, Update the documentation for min_messages and the corresponding compact and TokenSplitter::split behavior to state that compaction is skipped when the conversation has min_messages or fewer messages, including exactly the configured threshold.src/provider/openai.rs (1)
1584-1737: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a text-then-tool-call case. The new tests exercise tool indices in isolation; none stream
contentbeforetool_calls, which is exactly the shape that exposes the part-index overlap flagged onprocess_tool_call.🤖 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/openai.rs` around lines 1584 - 1737, Add a regression test covering content emitted before a tool call in the same streamed response, using StreamEmitter and StreamAccumulator. Ensure the sequence includes a text/content delta followed by tool-call chunks and completion, then assert the accumulated message preserves the text part and correctly creates the tool-call part without index overlap. Place the test alongside the existing emitter tool-call tests and exercise process_tool_call through process_chunk.src/engine/core/machine.rs (1)
797-799: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
small_machineignores itsmax_turnsargument.Every call site passes a number that has no effect (the budget comes from
test_policy), which is misleading when the value at the call site disagrees with the policy — e.g.small_machine(2)inmax_turns_enforced_by_machine. Drop the parameter.🤖 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/core/machine.rs` around lines 797 - 799, Remove the unused max_turns parameter from small_machine and update every call site to invoke it without an argument, keeping the test_policy-controlled budget behavior unchanged.src/engine/bare/dispatch.rs (1)
479-488: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
pre_detectioncan no longer returnSome(blocked), but its doc still claims it can.The new flow yields only
Ok(None)orErr(LoopError), so theOptionin the return type is now vestigial and the doc above is stale. Either drop theOptionor restore the soft-block path.🤖 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 479 - 488, Update pre_detection and its callers to return Result<(), LoopError> rather than Result<Option<...>, LoopError>, since decide_detected_pattern now only produces errors or success. Remove the stale documentation describing Some(blocked), and adjust the success path and any matching call sites to use unit results while preserving Err(LoopError) propagation.src/provider/gemini.rs (1)
223-257: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider delegating
stream_messages/create_messageto their_with_optionscounterparts.Anthropic's impl does
self.stream_messages_with_options(request, RequestOptions::default()); Gemini instead duplicates the request-building/SSE logic across both method pairs. Not a bug today (the hardcodedNone/ToolConstraint::NonematchRequestOptions::default()), but it's duplicate logic that can silently diverge on future changes.♻️ Proposed refactor sketch
fn stream_messages( &self, request: &crate::api::StreamRequest, ) -> Pin<Box<dyn Stream<Item = Result<StreamEvent, ApiError>> + Send + 'static>> { - let system = request.system.clone(); - let tools = request.tools.clone(); - let body = build_request_body( - &request.messages, - system.as_deref(), - tools.as_deref(), - None, - &ToolConstraint::None, - self.include_thoughts, - ); - let url = self.stream_url(); - ... + self.stream_messages_with_options(request, crate::structured::RequestOptions::default()) }Similarly for
create_message→create_message_with_options(request, RequestOptions::default()).Also applies to: 259-284, 286-350
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/provider/gemini.rs` around lines 223 - 257, Refactor Gemini’s stream_messages and create_message methods to delegate to stream_messages_with_options and create_message_with_options respectively, passing RequestOptions::default(). Remove the duplicated request-building, SSE, and response logic from the non-options methods while preserving their existing behavior.src/compact.rs (2)
656-670: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale doc:
build_telemetryis no longer a method.Lines 662-663 still say "is a method by design to allow future configuration-aware telemetry", but the signature has dropped
&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/compact.rs` around lines 656 - 670, Update the documentation for CompactTelemetry::build_telemetry to remove the stale claim that it is a method by design, since the function no longer takes &self. Keep the remaining description accurate and focused on its current static function behavior.
1074-1085: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the now-unused
_manager/_outcomebindings.Neither is passed to
build_telemetryanymore; constructing them only adds noise (and the_-prefix hides that the setup is dead).🤖 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 1074 - 1085, Remove the unused ContextManager construction bound to _manager and the CompactionOutcome bound to _outcome from this test setup. Keep the required pre and post conversation values and any code still used by build_telemetry unchanged.src/engine/bare.rs (1)
449-462: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc says config is taken from the machine, but it is not.
"The session/run config is taken from the machine" contradicts the signature —
session_configis a parameter and the run config isRunConfig::default().🤖 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 449 - 462, Correct the documentation for BareLoop::from_machine to match its actual behavior: state that the session configuration is supplied via the session_config parameter and the run configuration uses RunConfig::default(), rather than claiming configuration is taken from the machine.src/observer.rs (1)
286-296: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the panic message, not just whether it downcasts.
payload_downcast = payload.is::<&str>()throws away the only actionable information. Extract the payload string so operators can diagnose which observer failed and why.🪵 Proposed change
if let Err(payload) = catch_unwind(AssertUnwindSafe(|| f(obs))) { + let msg = payload + .downcast_ref::<&str>() + .copied() + .or_else(|| payload.downcast_ref::<String>().map(String::as_str)) + .unwrap_or("<non-string panic payload>"); tracing::error!( observer = obs.name(), - payload_downcast = payload.is::<&str>(), + panic = msg, "observer panicked; continuing with remaining observers" ); }Worth noting in the doc, too, that a panicking observer may leave its own interior-mutable state inconsistent (
AssertUnwindSafesuppresses that check) and that this isolation is a no-op underpanic = "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 `@src/observer.rs` around lines 286 - 296, Update the panic handling in the observer iteration around catch_unwind to extract and log the panic payload’s message instead of only recording whether it downcasts to &str. Preserve the existing observer name and continuation behavior, and provide a useful fallback for non-string payloads.
🤖 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 `@README.md`:
- Line 82: Update the README Quick Start example to match examples/hello-cli.rs:
replace the removed LoopConfig usage with SessionConfig::default(), pass
&RunConfig::default() to agent.run, and use result.turn_count() instead of
result.total_turns, while preserving the example’s existing flow.
In `@src/engine/bare.rs`:
- Around line 1692-1701: Align session lifecycle handling in the run flow around
session_start, notify_session_start, managers.reset_all, and finalize: either
emit both start and end notifications for every run while resetting per run, or
defer reset and session-end notification until an explicit session teardown.
Preserve the documented session-scoped manager state and ensure start/end events
occur symmetrically for multiple runs.
In `@src/engine/core/machine.rs`:
- Around line 494-512: Prevent infinite compaction cycles across request_model
and compaction_result by tracking whether the current compaction attempt reduces
context_tokens (or recording one attempt per run). When compaction makes no
progress and the machine still requires compaction, transition to a terminal
failure/context-overflow outcome instead of emitting Compact again; preserve
normal compaction and turn-limit behavior when progress is made.
In `@src/hooks/builtin/auto_commit.rs`:
- Around line 24-29: Update run_git and its timeout handling so a recv_timeout
expiration terminates the worker’s Child process and waits for it to be reaped
before returning GitExecutorError::Timeout. Preserve normal completion behavior
and ensure the timeout path cannot leave the git process running or holding
repository locks.
---
Outside diff comments:
In `@src/provider/openai.rs`:
- Around line 1171-1199: Update process_tool_call to map OpenAI tool-call
indexes into a range disjoint from TEXT_PART_INDEX before emitting PartStart and
IndexedDelta, using the mapped index consistently for both events. Ensure
tool-call id and name metadata are emitted or updated when later fragments
provide them, even if the first fragment lacks function fields, while preserving
argument accumulation for every fragment.
In `@src/structured.rs`:
- Around line 343-358: Gate the companion tests in the #[cfg(test)] module with
feature predicates matching the helpers they exercise: apply openai/gemini
gating to the parse_json_lenient_* tests, and anthropic/grammar/openai/gemini
gating to the tighten_* tests. Keep unrelated tests unchanged.
In `@src/tool/health.rs`:
- Around line 572-591: Update the circuit-breaker state accessors, including
allow_request, record_success, record_failure, is_open, state_label, and related
methods in the referenced ranges, to recover poisoned mutex guards with
crate::error::recover_guard instead of returning false, silently no-oping, or
reporting stale state. Preserve each method’s existing behavior after obtaining
the recovered guard so a poisoned lock does not permanently fail closed.
---
Minor comments:
In `@CHANGELOG.md`:
- Line 24: The changelog repeats `Run` and must identify the distinct type
replacing `SessionResult`. In `CHANGELOG.md` lines 24-24, remove the duplicated
final `Run`; in lines 229-231, replace `the new Run/Run` with the single correct
superseding type name.
In `@README.md`:
- Line 35: Update the README table entry currently labeled runtime to use
managers, linking to the managers documentation page instead, while retaining
the LoopManagers description.
In `@src/cancel.rs`:
- Around line 251-257: Replace the first-tick wait in the cancellation test with
an actual 5 ms delay, using tokio::time::sleep before the handle.is_finished()
assertion. Remove the unnecessary interval setup while preserving the assertion
that reset prevents notification until a new cancellation.
In `@src/compact.rs`:
- Around line 188-192: Update the documentation formulas and explanatory text in
the compact configuration documentation to match the implementation’s percent
scale: use /100 wherever threshold or target percentage calculations are shown,
including the formulas near the default description, context-window calculation,
and target-token documentation. Ensure the stated default example remains
mathematically consistent with the corrected formulas.
In `@src/config.rs`:
- Around line 158-169: Update the “Side-effect divergence from Sequential”
documentation in the relevant configuration comments to reflect the unified
execute_tool_call path: both Sequential and Parallel modes trigger detection and
observer pre/post events on every retry attempt, with no side-effect granularity
or recovery-behavior divergence. Retain the distinction for health tracking only
if it remains accurate.
In `@src/engine/bare.rs`:
- Around line 334-338: The session constructor’s initial Run::default() is
retained as a bogus entry while run() unconditionally appends real runs. Remove
the placeholder initialization and update current_run() and related session
logic to handle an empty runs collection as optional, or otherwise ensure the
first run replaces the untouched placeholder so totals and per-run iteration
include only real runs.
In `@src/engine/bare/compact.rs`:
- Around line 51-58: Update run_compaction’s no-op branches—missing
context_manager, pre_compact_hook_aborts, and NoAction—to return the
pre-compaction token estimate instead of zero. Preserve the original history and
ensure the result communicates an aborting hook as unsuccessful rather than a
successful zero-token compaction.
In `@src/engine/bare/dispatch.rs`:
- Around line 325-337: Update the fallback ToolDispatchResult construction in
the result-mapping closure to use the originating call identified by idx,
preserving its tool_call_id and resolved_tool_name instead of empty strings.
Keep the existing soft-error output and other fallback fields unchanged.
In `@src/engine/bare/emission.rs`:
- Around line 66-77: The session_end_reason method should derive the terminal
reason from the machine’s authoritative outcome rather than comparing turn_count
against max_turns. Map MachineOutcome::MaxTurnsExceeded to MaxTurns and
MachineOutcome::Completed to Complete, while preserving cancellation and error
precedence; ensure ContextOverflow remains handled if the outcome supports it.
- Around line 42-56: The notify_session_end flow currently replaces every
failure with the placeholder "run failed"; update its callers and signature to
thread the terminal LoopError or its string into SessionEndContext.error.
Preserve None for successful sessions and pass the actual failure detail to
observers, while keeping notify_session_end_hook behavior intact.
In `@src/engine/bare/stream.rs`:
- Around line 50-59: Update stream_turn so the existing machine.full_history()
is placed into messages before contributor_messages are appended, ensuring
context contributor messages occur at the end of the turn-boundary request.
Preserve the existing StreamRequest construction and tool/system configuration.
In `@src/engine/core/lifecycle.rs`:
- Around line 221-245: The documentation at src/engine/core/lifecycle.rs lines
221-245 should remove the misplaced Run-focused paragraphs and doctest, leaving
Turn’s own description attached to struct Turn. At src/engine/core/lifecycle.rs
lines 572-614, keep only the one-line alias documentation on RunResult and move
the core agent lifecycle prose and impl Loop for MyAgent example onto the pub
trait Loop documentation.
In `@src/hooks/builtin/auto_commit.rs`:
- Around line 578-582: Update the documentation for the with_files builder near
AutoCommitConfig::files to state that an empty file allow-list is rejected by
stage_files, rather than staging every working-tree change. Remove the outdated
claim that an empty vector stages all changes.
- Around line 489-493: Update the documentation for the tracked-tools
configuration near hook_tracks_tracked_tools to state that file_path is read
from PostToolUseContext::input, not tool output; preserve the description that
matching tools’ touched files are recorded and staged at session end.
In `@src/hooks/executor.rs`:
- Around line 190-199: Update the documentation for check_pre_compact to state
that hooks execute in registration order only until the first CompactResult with
abort: true, which is returned immediately; clarify that result merging and
additional_context accumulation apply only before that abort.
In `@src/presets.rs`:
- Around line 25-27: Update the BareLoop quick-start example to pass
ConstrainedProfile::run_config() to agent.run(...) in the complete usage path,
while retaining the existing session_config(), request_options(), and apply
setup.
In `@src/stream.rs`:
- Around line 1083-1113: Update the StreamEvent::PartStop branch to remove the
first open slot before parsing tool input, while preserving access to that
slot’s fields for flushing. Ensure serde_json::from_str failures return
InvalidToolInputJson without leaving the consumed slot in self.open, and keep
successful text/tool part completion behavior unchanged.
In `@tests/constrained_decode.rs`:
- Around line 49-63: Update the constrained decode accounting around the loop’s
total, errors, and rate calculations so errored stream attempts are excluded
from the rate denominator. Adjust total consistently in all corresponding paths
(including the sections noted at 69–81 and 102–109), while preserving valid
counting and error reporting; ensure the “not counted in rate” message matches
the resulting behavior.
---
Nitpick comments:
In `@Cargo.toml`:
- Line 31: Make the bytes dependency optional in Cargo.toml and add dep:bytes to
the providers feature, ensuring bytes::Bytes remains available on the
SSE/provider path while feature-less builds do not include it.
- Line 54: Document the new xai feature alias in the README feature table by
adding a row alongside grok, ensuring both feature names and their descriptions
remain aligned.
In `@src/compact.rs`:
- Around line 656-670: Update the documentation for
CompactTelemetry::build_telemetry to remove the stale claim that it is a method
by design, since the function no longer takes &self. Keep the remaining
description accurate and focused on its current static function behavior.
- Around line 1074-1085: Remove the unused ContextManager construction bound to
_manager and the CompactionOutcome bound to _outcome from this test setup. Keep
the required pre and post conversation values and any code still used by
build_telemetry unchanged.
In `@src/compact/truncating.rs`:
- Around line 56-61: Update the documentation for min_messages and the
corresponding compact and TokenSplitter::split behavior to state that compaction
is skipped when the conversation has min_messages or fewer messages, including
exactly the configured threshold.
In `@src/engine/bare.rs`:
- Around line 449-462: Correct the documentation for BareLoop::from_machine to
match its actual behavior: state that the session configuration is supplied via
the session_config parameter and the run configuration uses
RunConfig::default(), rather than claiming configuration is taken from the
machine.
In `@src/engine/bare/dispatch.rs`:
- Around line 479-488: Update pre_detection and its callers to return Result<(),
LoopError> rather than Result<Option<...>, LoopError>, since
decide_detected_pattern now only produces errors or success. Remove the stale
documentation describing Some(blocked), and adjust the success path and any
matching call sites to use unit results while preserving Err(LoopError)
propagation.
In `@src/engine/core/machine.rs`:
- Around line 797-799: Remove the unused max_turns parameter from small_machine
and update every call site to invoke it without an argument, keeping the
test_policy-controlled budget behavior unchanged.
In `@src/observer.rs`:
- Around line 286-296: Update the panic handling in the observer iteration
around catch_unwind to extract and log the panic payload’s message instead of
only recording whether it downcasts to &str. Preserve the existing observer name
and continuation behavior, and provide a useful fallback for non-string
payloads.
In `@src/provider/gemini.rs`:
- Around line 223-257: Refactor Gemini’s stream_messages and create_message
methods to delegate to stream_messages_with_options and
create_message_with_options respectively, passing RequestOptions::default().
Remove the duplicated request-building, SSE, and response logic from the
non-options methods while preserving their existing behavior.
In `@src/provider/openai.rs`:
- Around line 1584-1737: Add a regression test covering content emitted before a
tool call in the same streamed response, using StreamEmitter and
StreamAccumulator. Ensure the sequence includes a text/content delta followed by
tool-call chunks and completion, then assert the accumulated message preserves
the text part and correctly creates the tool-call part without index overlap.
Place the test alongside the existing emitter tool-call tests and exercise
process_tool_call through process_chunk.
In `@src/testing.rs`:
- Around line 635-643: Update the rustdoc example above stream_messages to
remove references to the obsolete _messages, _system, and _tools parameters, and
document the current single _request argument consistently with the method
signature.
🪄 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: f23c3efb-3253-4f8c-967e-17467b35bb13
📒 Files selected for processing (77)
.gitignoreCHANGELOG.mdCargo.tomlMakefileREADME.mdexamples/chat.rsexamples/echo-tool-cli.rsexamples/hello-cli.rsexamples/repl-cli.rssrc/api.rssrc/api/error.rssrc/cancel.rssrc/capabilities.rssrc/compact.rssrc/compact/truncating.rssrc/compact/types.rssrc/config.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/message.rssrc/engine/bare/stream.rssrc/engine/core.rssrc/engine/core/lifecycle.rssrc/engine/core/machine.rssrc/engine/loop_core.rssrc/error.rssrc/fallback.rssrc/hooks.rssrc/hooks/builtin/auto_commit.rssrc/hooks/context.rssrc/hooks/executor.rssrc/lib.rssrc/managers.rssrc/memory/builtin.rssrc/memory/entry.rssrc/message.rssrc/middleware.rssrc/middleware/memoize.rssrc/middleware/timeout.rssrc/middleware/unknown_tool.rssrc/middleware/verify.rssrc/observer.rssrc/observer/context.rssrc/presets.rssrc/provider.rssrc/provider/anthropic.rssrc/provider/gemini.rssrc/provider/grammar.rssrc/provider/openai.rssrc/provider/sse.rssrc/reflection.rssrc/reflection/backoff.rssrc/reflection/llm.rssrc/stream.rssrc/stream/handler.rssrc/stream/heartbeat.rssrc/stream/rate_limit.rssrc/structured.rssrc/testing.rssrc/tool.rssrc/tool/health.rssrc/tool/permission.rssrc/tool/registry.rssrc/tool/shield.rstests/constrained_decode.rstests/examples_e2e.rstests/helpers.rstests/provider_e2e.rstests/provider_survival.rstests/structured_output.rs
💤 Files with no reviewable changes (8)
- src/memory/builtin.rs
- src/middleware/verify.rs
- src/middleware.rs
- src/engine/loop_core.rs
- src/stream/heartbeat.rs
- src/reflection.rs
- src/tool/shield.rs
- src/middleware/memoize.rs
| ```rust,no_run | ||
| use loopctl::engine::BareLoop; | ||
| use loopctl::engine::loop_core::Loop; | ||
| use loopctl::engine::core::Loop; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Quick Start block is only half-migrated. The import path was updated, but the same example still builds a LoopConfig (removed per CHANGELOG lines 225-235) and calls agent.run("…") without the new &RunConfig, and reads result.total_turns. Since it's a rust,no_run doctest, this will fail cargo test --doc. Mirror examples/hello-cli.rs: SessionConfig::default(), .run(input, &RunConfig::default()), result.turn_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 `@README.md` at line 82, Update the README Quick Start example to match
examples/hello-cli.rs: replace the removed LoopConfig usage with
SessionConfig::default(), pass &RunConfig::default() to agent.run, and use
result.turn_count() instead of result.total_turns, while preserving the
example’s existing flow.
| Box::pin(async move { | ||
| let turn_start = Instant::now(); | ||
| let current_turn = self.budget.total_turns; | ||
|
|
||
| self.record_user_input(input); | ||
| self.fire_turn_start(current_turn, input); | ||
|
|
||
| if !self.contributors.is_empty() { | ||
| let ctx = ContributorContext::new(current_turn, &self.conversation); | ||
| let injected: Vec<Message> = self | ||
| .contributors | ||
| .iter() | ||
| .filter_map(|contributor| contributor.contribute(&ctx)) | ||
| .collect(); | ||
| self.conversation.extend(injected); | ||
| let session_is_new = self.session.session_start.is_none(); | ||
| if session_is_new { | ||
| self.session.session_start = Some(Instant::now()); | ||
| self.notify_session_start(); | ||
| } | ||
|
|
||
| let cancel = Arc::clone(&self.cancelled); | ||
| tokio::select! { | ||
| biased; | ||
| () = cancel.notified() => { | ||
| self.state = LoopState::Cancelled; | ||
| // TODO: fire_turn_cancelled | ||
| self.managers.observers().on_turn_end(&TurnEndContext { | ||
| turn: current_turn, | ||
| success: false, | ||
| error: Some("cancelled".into()), | ||
| duration_ms: Self::millis_u64(turn_start.elapsed()), | ||
| input_tokens: 0, | ||
| output_tokens: 0, | ||
| }); | ||
| Err(LoopError::Cancelled) | ||
| self.session.runs.push(Run::new(input, run_config)); | ||
| self.managers.reset_all(); | ||
| self.machine.accept_input(input); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Session-scoped events fire asymmetrically across multiple runs.
notify_session_start fires only when session_start is None (once per session), but finalize fires notify_session_end on every run() exit. A host doing N runs on one loop sees 1 start and N ends. Also managers.reset_all() runs per-run(), which discards the fallback circuit-breaker and detection state that the field docs (Line 199) describe as session-scoped.
Either make both notifications per-run, or move the reset/end to a real session teardown.
🤖 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 1692 - 1701, Align session lifecycle
handling in the run flow around session_start, notify_session_start,
managers.reset_all, and finalize: either emit both start and end notifications
for every run while resetting per run, or defer reset and session-end
notification until an explicit session teardown. Preserve the documented
session-scoped manager state and ensure start/end events occur symmetrically for
multiple runs.
| fn request_model(&mut self, turn: usize, policy: MachinePolicy) -> MachineStep { | ||
| if self.turns_taken >= policy.max_turns { | ||
| let outcome = MachineOutcome::MaxTurnsExceeded; | ||
| self.state = MachineState::Terminal(outcome.clone()); | ||
| return MachineStep::Done(outcome); | ||
| } | ||
| if self.is_emergency(policy) { | ||
| let reason = CompactReason::Emergency; | ||
| self.state = MachineState::AwaitingCompaction { reason }; | ||
| return MachineStep::Compact { reason }; | ||
| } | ||
| if policy.auto_compact && self.should_compact(policy) { | ||
| let reason = CompactReason::ThresholdExceeded; | ||
| self.state = MachineState::AwaitingCompaction { reason }; | ||
| return MachineStep::Compact { reason }; | ||
| } | ||
| self.state = MachineState::AwaitingModel { turn }; | ||
| MachineStep::CallLLM { turn } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
No progress guard around compaction — the machine can emit Compact forever.
request_model re-emits Compact whenever context_tokens is still over the line, and compaction_result adopts whatever tokens_after the driver reports. If compaction cannot shrink the history below the emergency threshold (a single oversized message, a no-op compactor), the machine loops Compact → compaction_result → Compact indefinitely without ever consuming a turn, so max_turns never bites.
Consider tracking a per-run compaction attempt (or comparing tokens_after against the pre-compaction value) and going terminal — e.g. MachineOutcome::Failed / a context-overflow outcome — when compaction makes no progress.
Also applies to: 632-640
🤖 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/core/machine.rs` around lines 494 - 512, Prevent infinite
compaction cycles across request_model and compaction_result by tracking whether
the current compaction attempt reduces context_tokens (or recording one attempt
per run). When compaction makes no progress and the machine still requires
compaction, transition to a terminal failure/context-overflow outcome instead of
emitting Compact again; preserve normal compaction and turn-limit behavior when
progress is made.
| /// | ||
| /// Applied to every `git` call made by [`GitExecutor`] so a stuck git | ||
| /// process (for example a hung GPG-signing prompt or an unreachable | ||
| /// remote during a fetch-adjacent operation) cannot block the agent | ||
| /// loop indefinitely. After this elapses the child is killed and the | ||
| /// call surfaces as [`GitExecutorError::Timeout`]. |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Actually terminate timed-out git processes.
run_git moves Child into the worker thread, then returns Timeout after recv_timeout without a kill or reap path. A hung git process can therefore continue running and hold repository locks after the agent proceeds.
🤖 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/builtin/auto_commit.rs` around lines 24 - 29, Update run_git and
its timeout handling so a recv_timeout expiration terminates the worker’s Child
process and waits for it to be reaped before returning
GitExecutorError::Timeout. Preserve normal completion behavior and ensure the
timeout path cannot leave the git process running or holding repository locks.
✅ Action performedReview finished.
|
1 participant
No description provided.