feat: bare loop, tokio dep added, clean up - #19
Conversation
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces ChangesAgentic Loop and Tool Content Refactoring
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/cancel.rs`:
- Around line 89-94: The notified() method has a lost-wakeup race: create the
Notify::notified() future before checking the cancellation flag so a concurrent
cancel() cannot wake before we register; specifically, call let notified =
self.notify.notified() first, then if self.flag.load(Ordering::Acquire) {
return; } else await notified.await; keep using the same Ordering on self.flag
and the Notified future to ensure the wakeup is not missed.
In `@src/engine/bare.rs`:
- Around line 491-497: The cancellation branch in bare.rs currently returns
Ok(SessionResult::failed(...)) which is inconsistent with cancellations from
stream_turn()/dispatch_tools() that return Err(AgentError::Cancelled); change
the branch that checks self.is_cancelled() to notify_session_end(...) and then
return Err(AgentError::Cancelled) instead of Ok(SessionResult::failed(...));
apply the same change to the corresponding cancellation branch around the other
occurrence (the block referenced at 541-544) so all cancellations surface as
Err(AgentError::Cancelled).
- Around line 652-667: The branch handling tool.call currently calls
result.text_content() which discards multipart/image payloads and flattens
ToolOutput; update the logic in the tool call handling (the match arm that
builds ToolCallResult and calls notify_tool_complete) to preserve the full
ToolOutput.payload (or the ToolContent model) instead of converting to text,
pass that preserved payload into notify_tool_complete and into
ToolCallResult.output, and ensure the "success" flag passed to
notify_tool_complete is derived from !result.is_error (so observers see failures
when result.is_error is true); apply the same change to the other similar
branches that use result.text_content() (the blocks around the other occurrences
referenced).
🪄 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: 1430b44b-5bc8-433b-a29c-ef7115aca410
📒 Files selected for processing (12)
Cargo.tomlsrc/api_client.rssrc/api_error.rssrc/cancel.rssrc/engine.rssrc/engine/bare.rssrc/lib.rssrc/loop_control/convergence.rssrc/loop_control/loop_detector.rssrc/message.rssrc/stream.rssrc/tool.rs
💤 Files with no reviewable changes (5)
- src/stream.rs
- src/api_client.rs
- src/api_error.rs
- src/loop_control/convergence.rs
- src/loop_control/loop_detector.rs
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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`:
- Around line 18-31: The README's module list uses reference-style names like
`[`api_client`]`, `[`api_error`]`, `[`builder`]`, `[`builtin`]`, `[`cancel`]`,
`[`core`]`, `[`engine`]`, `[`loop_control`]`, `[`message`]`, `[`stream`]`,
`[`tool`]`, and `[`testing`]` but no reference link definitions exist; fix this
by either converting each bracketed module token to an inline link (e.g.,
replace `[`api_client`]` with `[api_client](<target>)`) or by adding matching
reference definitions for each symbol at the bottom of the README (e.g.,
`[api_client]: <target>`), ensuring every listed module name resolves to its
documentation or crate/module path so the links become clickable.
- Line 132: The markdown code fence containing the ASCII architecture diagram is
missing a language specifier causing markdownlint MD040; update the opening
fence from ``` to ```text so the diagram block (the fence that contains the
ASCII art showing ApiClient and related components) is explicitly marked as
text, leaving the closing ``` unchanged.
In `@src/engine/bare.rs`:
- Around line 431-438: The doc comment listing loop termination conditions is
out of date: update the cancellation bullet to reflect the current contract by
stating that calling BareLoop::cancel results in an Err(AgentError::Cancelled)
being returned (rather than producing Ok(SessionResult::failed(...))); locate
the doc block in src/engine/bare.rs that documents the loop termination cases
and replace the sentence about cancellation so it mentions
Err(AgentError::Cancelled) and the caller should handle that error variant.
- Around line 505-506: The branch handling Ok((assistant_msg, usage,
_stop_reason)) treats any non-tool response as a successful end_turn; change it
to inspect the StreamStopReason returned by stream_turn() (e.g., the
`_stop_reason` variable) and only mark success true when that reason equals
StreamStopReason::EndTurn (or equivalent), otherwise set success to false (or
mark as truncated/partial) and propagate the actual stop reason into the
outgoing response/metadata; apply the same fix to the similar handling around
the other branch (lines ~523-538) so truncated/token-exhaustion stops are not
reported as clean successes.
- Around line 517-522: When calling self.dispatch_tools(&tool_calls).await in
the has_tool_calls branch, capture the Result instead of using ? so you can
detect AgentError::Cancelled and still emit end events before returning: call
self.notify_turn_end(true, None) and self.notify_session_end(...) as appropriate
when Err(AgentError::Cancelled) is returned, then return the error; for other
Errs propagate as before; only build_tool_result_message, push to conversation,
and increment total_tool_calls when dispatch_tools returns Ok. Ensure you
reference dispatch_tools, run, notify_turn_end, notify_session_end, and
AgentError::Cancelled in your changes.
🪄 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: 3d41e2ee-ef33-45c2-b226-c260c12b63c4
📒 Files selected for processing (3)
README.mdsrc/cancel.rssrc/engine/bare.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cancel.rs
No description provided.