wip: switch to llms-sdk - #22
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR migrates agent generation to llms-sdk, introduces structured assistant message parts and token usage fields, updates tool and storage events, changes CLI provider configuration and rendering, and adds OpenAI and Anthropic integration coverage. ChangesLLM SDK and message migration
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant MicroAgent
participant LLM
participant Storage
participant ToolFunction
participant TUI
CLI->>MicroAgent: configure and start generation
MicroAgent->>LLM: stream LLMRequest
LLM-->>MicroAgent: deltas and completed assistant message
MicroAgent->>Storage: persist stream and session events
MicroAgent->>ToolFunction: execute tool calls
ToolFunction-->>MicroAgent: return tool results
MicroAgent->>Storage: store tool results and assistant message
MicroAgent-->>TUI: render structured content and usage
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
microagents-core/src/agent.rs (1)
995-996: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftA
tool_callcan be appended to history with no matching tool result. The assistant message (including itstool_callparts) is always pushed at Line 1038, but several paths produce noToolResultPart, leaving the next request with an unanswered tool call that OpenAI and Anthropic both reject.
microagents-core/src/agent.rs#L995-L996: add anelsebranch for thelocal_tools.get(&tc.name)miss that pushes a syntheticMessagePart::ToolResulterror fortc.id.microagents-core/src/agent.rs#L1029-L1034: in theOk(Err(e))and join-failure arms, also push aToolResultParterror for the corresponding tool call id (propagate the id into theJoinSetresult so it is available on failure) instead of only yielding the error.🤖 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 `@microagents-core/src/agent.rs` around lines 995 - 996, Ensure every assistant tool_call receives a matching ToolResult before history is appended: in the local_tools.get(&tc.name) miss branch, push a synthetic MessagePart::ToolResult error using tc.id; in the Ok(Err(e)) and join-failure arms, propagate each tool-call id through the JoinSet result and push the corresponding ToolResultPart error instead of only yielding the error. Apply both changes in microagents-core/src/agent.rs at lines 995-996 and 1029-1034.
🧹 Nitpick comments (8)
microagents-core/tests/integration_test.rs (2)
156-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the debug
println!.Left-over debug artifact; the OpenAI test doesn't do this.
🧹 Proposed cleanup
- Ok(ev) => { - println!("{:#?}", ev); - events.push(ev) - } + Ok(ev) => events.push(ev),🤖 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 `@microagents-core/tests/integration_test.rs` around lines 156 - 159, Remove the debug println! call from the Ok(ev) branch in the integration test while preserving the existing events.push(ev) behavior.
134-148: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFactor the shared assertion body out of the two provider tests.
The Anthropic test duplicates ~70 lines of the OpenAI test verbatim except for provider/model. Extract a
async fn assert_weather_run(provider: &str, model: &str)helper so future event-shape changes only need one edit. Also, both testsreturnsilently when the key is missing, so CI reports them as passing — considereprintln!("skipping: <VAR> unset")so a misconfiguredjake integration-corerun is visible.Also applies to: 163-211
🤖 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 `@microagents-core/tests/integration_test.rs` around lines 134 - 148, Extract the duplicated weather-run assertion logic from test_microagent_integration_anthropic and its OpenAI counterpart into an async assert_weather_run(provider, model) helper, passing each test’s provider and model while preserving all assertions. Update both missing-key early returns to emit an eprintln skip message naming the unset environment variable before returning.jakefile.toml (1)
7-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
--no-default-featurestargets are now dead weight.
microagents-coreno longer declares a[features]section (thetoken_estimation/tokiefeature was dropped in this PR), soclippy-nodefault,clippy-fix-nodefault,build-nodefault,build-rel-nodefaultandtest-nodefaultare just slower duplicates of the default targets. Remove them (and any CI jobs referencing them) unless a feature flag is coming back.🤖 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 `@jakefile.toml` around lines 7 - 14, Remove the obsolete no-default-features targets clippy-nodefault, clippy-fix-nodefault, build-nodefault, build-rel-nodefault, and test-nodefault from the Jake task definitions, and remove any CI jobs that invoke them. Keep the remaining default clippy, build, format, and test targets unchanged.microagents-core/src/agent.rs (2)
149-153: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueComments contradict the values they document.
Line 149 says "GPT-5.5 … as of May 2026" for
gpt-5.6-terra, and Line 152 mentions "Claude Opus 4.7" forclaude-sonnet-5(plus a "cuttig-edge" typo).📝 Suggested wording
- // GPT-5.5 is the current default ChatGPT model as of May 2026 + // gpt-5.6-terra is the current default OpenAI model SupportedProvider::OpenAI => Ok("gpt-5.6-terra"), - // Claude Opus 4.7 by Anthropic is cuttig-edge in the models market + // claude-sonnet-5 is Anthropic's current general-purpose model SupportedProvider::Anthropic => Ok("claude-sonnet-5"),🤖 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 `@microagents-core/src/agent.rs` around lines 149 - 153, Update the comments in the SupportedProvider model mapping to accurately describe the configured values: make the OpenAI comment match gpt-5.6-terra, make the Anthropic comment match claude-sonnet-5, and correct the “cuttig-edge” typo.
1293-1306: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnv-var mutation in parallel tests is racy — use the already-available
serial_test.Several tests in this module
unsafe { set_var("OPENAI_API_KEY", ...) }whilecargo testruns them on multiple threads; concurrentset_varis exactly why the call isunsafe, andtest_agent_fails_to_build_if_not_api_keydepends on key absence.serial_test = "3.5.0"is already a dev-dependency — annotate every env-mutating test with#[serial]. Restoring withunwrap_or_default()also leaves an empty-string var behind rather than unsetting it.♻️ Suggested pattern
#[test] + #[serial_test::serial] fn test_build_sets_empty_history() {🤖 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 `@microagents-core/src/agent.rs` around lines 1293 - 1306, Annotate every test in the module that mutates OPENAI_API_KEY with the existing serial_test #[serial] attribute, including the test building the agent. Restore the environment exactly: preserve and reinstate an existing value, but remove the variable when it was originally absent instead of restoring an empty string; apply the same cleanup to tests that require the key to be absent.microagents-events/src/lib.rs (1)
184-190: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
#[serde(untagged)]makes the persisted part format structurally ambiguous.Variant identity is inferred purely from which fields happen to deserialize, and serde ignores unknown fields, so the first variant whose required fields are present wins. Today the field sets are disjoint, but any future part sharing a field name (or an added optional field) will silently mis-deserialize persisted sessions, and errors surface as an unhelpful "data did not match any variant". Consider an internally-tagged representation (e.g.
#[serde(tag = "type", rename_all = "snake_case")]) plusdeny_unknown_fields, with a migration for existing JSONL/SQLite payloads.🤖 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 `@microagents-events/src/lib.rs` around lines 184 - 190, Replace the untagged serialization on AssistantMessagePart with an explicitly tagged representation using a stable type discriminator and snake_case variant names, and reject unknown fields on the part payload structs. Update JSONL/SQLite deserialization to migrate existing untagged persisted payloads into the new tagged format while preserving compatibility with current sessions.microagents-storage/src/jsonl.rs (1)
194-206: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winNeither persistence round-trip test verifies the new structured content. Both backends serialize to JSON and re-parse via
get_session, which is the only coverage that would catch#[serde(untagged)]AssistantMessagePartmis-resolution, yet both tests assert only JSON-RPC method names.
microagents-storage/src/jsonl.rs#L194-L206: assert that the round-trippedevents[2]content andevents[3]result equal the fixtures, and addToolCall/Thinkingparts to the fixture.microagents-storage/src/sqlite.rs#L267-L278: add the same content/result equality assertions afterget_session.🤖 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 `@microagents-storage/src/jsonl.rs` around lines 194 - 206, Extend the persistence round-trip tests so structured assistant content is verified after get_session: in microagents-storage/src/jsonl.rs lines 194-206, add ToolCall and Thinking parts to the fixtures and assert events[2].content and events[3].result equal those fixtures; apply the same content/result equality assertions in microagents-storage/src/sqlite.rs lines 267-278 after get_session.microagents-cli/src/tui/mod.rs (1)
443-461: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis tool-call derivation looks dead now, and its comment is stale.
microagents-corebuildsAssistantResponseEventonly in thetool_calls.is_empty()branch ofrun()and it does emitAgentEventAny::ToolCall(handled at Line 434), so thecontenthere should never carryToolCallparts. Either drop this block and the "the core does not currently emittool.callevents" comment, or keep it as a fallback and guard against double-rendering. TheOption<Vec<ToolCallPart>>accumulator can also just be aVecwithis_empty().♻️ Simplification if kept
- let mut tool_calls: Option<Vec<ToolCallPart>> = None; - for part in r.content { - match part { - AssistantMessagePart::ToolCall(t) => { - tool_calls.get_or_insert_with(Vec::new).push(t) - } - _ => continue, - } - } - if let Some(calls) = tool_calls { - for c in calls { + let tool_calls: Vec<ToolCallPart> = r + .content + .into_iter() + .filter_map(|p| match p { + AssistantMessagePart::ToolCall(t) => Some(t), + _ => None, + }) + .collect(); + for c in tool_calls {🤖 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 `@microagents-cli/src/tui/mod.rs` around lines 443 - 461, Remove the dead tool-call derivation loop from the AgentEventAny::AssistantResponse handler, including its stale comment and related accumulator/argument-normalization logic. Tool calls are already handled by the AgentEventAny::ToolCall branch, so preserve that existing rendering path without adding fallback behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@microagents-cli/src/tui/mod.rs`:
- Around line 417-422: Update the usage-line format string in the
session-stopped output to use Display formatting for token counts and
precision-limited Display formatting for latency, avoiding Debug formatting and
long floating-point tails. Keep the existing values and output structure
unchanged.
In `@microagents-core/src/agent.rs`:
- Around line 844-873: Both assistant_message.expect calls in the no-tool-calls
completion path can panic when streaming ends without a Complete chunk; add one
assistant_message.clone() guard in microagents-core/src/agent.rs lines 844-873,
yielding AgentError::RunError and returning when absent, then reuse the mapped
parts for AssistantResponseEvent and SessionStopEvent. Apply the same guard
before pushing the assistant message to self.history at
microagents-core/src/agent.rs line 1038; the sibling site should use the
existing final message rather than assume it is present.
- Around line 455-457: Update the SupportedProvider::Anthropic branch to report
ANTHROPIC_API_KEY in MicroAgentBuilderError::EnvVarNotFoundError when
check_env_var fails, while leaving the existing environment-variable validation
unchanged.
- Around line 913-916: Replace the panic-prone
serde_json::from_str(...).expect(...) in the tool_calls loop with explicit
malformed-JSON handling: when parsing tc.arguments fails, propagate a RunError
or return a ToolResult::Err to the model, and continue normal tool lookup only
for successfully parsed arguments.
---
Outside diff comments:
In `@microagents-core/src/agent.rs`:
- Around line 995-996: Ensure every assistant tool_call receives a matching
ToolResult before history is appended: in the local_tools.get(&tc.name) miss
branch, push a synthetic MessagePart::ToolResult error using tc.id; in the
Ok(Err(e)) and join-failure arms, propagate each tool-call id through the
JoinSet result and push the corresponding ToolResultPart error instead of only
yielding the error. Apply both changes in microagents-core/src/agent.rs at lines
995-996 and 1029-1034.
---
Nitpick comments:
In `@jakefile.toml`:
- Around line 7-14: Remove the obsolete no-default-features targets
clippy-nodefault, clippy-fix-nodefault, build-nodefault, build-rel-nodefault,
and test-nodefault from the Jake task definitions, and remove any CI jobs that
invoke them. Keep the remaining default clippy, build, format, and test targets
unchanged.
In `@microagents-cli/src/tui/mod.rs`:
- Around line 443-461: Remove the dead tool-call derivation loop from the
AgentEventAny::AssistantResponse handler, including its stale comment and
related accumulator/argument-normalization logic. Tool calls are already handled
by the AgentEventAny::ToolCall branch, so preserve that existing rendering path
without adding fallback behavior.
In `@microagents-core/src/agent.rs`:
- Around line 149-153: Update the comments in the SupportedProvider model
mapping to accurately describe the configured values: make the OpenAI comment
match gpt-5.6-terra, make the Anthropic comment match claude-sonnet-5, and
correct the “cuttig-edge” typo.
- Around line 1293-1306: Annotate every test in the module that mutates
OPENAI_API_KEY with the existing serial_test #[serial] attribute, including the
test building the agent. Restore the environment exactly: preserve and reinstate
an existing value, but remove the variable when it was originally absent instead
of restoring an empty string; apply the same cleanup to tests that require the
key to be absent.
In `@microagents-core/tests/integration_test.rs`:
- Around line 156-159: Remove the debug println! call from the Ok(ev) branch in
the integration test while preserving the existing events.push(ev) behavior.
- Around line 134-148: Extract the duplicated weather-run assertion logic from
test_microagent_integration_anthropic and its OpenAI counterpart into an async
assert_weather_run(provider, model) helper, passing each test’s provider and
model while preserving all assertions. Update both missing-key early returns to
emit an eprintln skip message naming the unset environment variable before
returning.
In `@microagents-events/src/lib.rs`:
- Around line 184-190: Replace the untagged serialization on
AssistantMessagePart with an explicitly tagged representation using a stable
type discriminator and snake_case variant names, and reject unknown fields on
the part payload structs. Update JSONL/SQLite deserialization to migrate
existing untagged persisted payloads into the new tagged format while preserving
compatibility with current sessions.
In `@microagents-storage/src/jsonl.rs`:
- Around line 194-206: Extend the persistence round-trip tests so structured
assistant content is verified after get_session: in
microagents-storage/src/jsonl.rs lines 194-206, add ToolCall and Thinking parts
to the fixtures and assert events[2].content and events[3].result equal those
fixtures; apply the same content/result equality assertions in
microagents-storage/src/sqlite.rs lines 267-278 after get_session.
🪄 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: f2378ebc-e173-409d-954e-626ff4350d52
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
jakefile.tomlmicroagents-cli/src/init_env.rsmicroagents-cli/src/tui/mod.rsmicroagents-core/Cargo.tomlmicroagents-core/src/agent.rsmicroagents-core/src/common.rsmicroagents-core/src/types.rsmicroagents-core/tests/integration_test.rsmicroagents-events/src/lib.rsmicroagents-events/src/types.rsmicroagents-storage/src/jsonl.rsmicroagents-storage/src/memory.rsmicroagents-storage/src/sqlite.rs
💤 Files with no reviewable changes (1)
- microagents-cli/src/init_env.rs
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
microagents-core/src/agent.rs (1)
659-666: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReset the terminal message for every generation pass.
After a tool call, a follow-up stream that ends without
Completereuses the previous iteration’sassistant_message; the guard then records that stale tool-call message as a successful response and session stop. Set it toNoneat the start of each loop iteration.🤖 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 `@microagents-core/src/agent.rs` around lines 659 - 666, Reset assistant_message to None at the beginning of every generation loop iteration, before processing the follow-up stream. Update the loop surrounding the existing assistant_message declaration/guard so each pass starts without a prior terminal message, while preserving the successful-response handling when the current stream produces one.
🤖 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 `@microagents-cli/src/init_env.rs`:
- Around line 41-43: Update the provider mapping for ANTHROPIC_API_KEY in the
provider inference configuration so it resolves to "anthropic" instead of
"openai". Leave the OPENAI_API_KEY mapping unchanged, ensuring Anthropic-only
configuration selects the Anthropic provider.
In `@microagents-cli/src/main.rs`:
- Around line 35-38: Update the help documentation for the provider field in the
CLI argument definition, removing the claim that omission unconditionally falls
back to OpenAI. Describe that the provider is inferred from configured
environment variables, may select Anthropic, and errors when no provider is
configured; leave the provider option and default behavior unchanged.
In `@microagents-events/src/lib.rs`:
- Around line 383-384: Add an explicit replay-safe no-op match arm for the
ToolAnyCall event in the TUI event handling logic in
microagents-cli/src/tui/mod.rs. Keep the existing ToolCall rendering behavior
unchanged, and ensure replaying persisted ToolAnyCall events does not reach
unreachable! or panic.
---
Outside diff comments:
In `@microagents-core/src/agent.rs`:
- Around line 659-666: Reset assistant_message to None at the beginning of every
generation loop iteration, before processing the follow-up stream. Update the
loop surrounding the existing assistant_message declaration/guard so each pass
starts without a prior terminal message, while preserving the
successful-response handling when the current stream produces one.
🪄 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: dd96b582-7c6c-4e3d-a008-309f7b0b996e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (13)
.github/workflows/lint.yamlmicroagents-cli/Cargo.tomlmicroagents-cli/src/init_env.rsmicroagents-cli/src/main.rsmicroagents-cli/src/tui/mod.rsmicroagents-core/Cargo.tomlmicroagents-core/src/agent.rsmicroagents-core/src/common.rsmicroagents-core/tests/integration_test.rsmicroagents-events/Cargo.tomlmicroagents-events/src/lib.rsmicroagents-storage/Cargo.tomlsearch-evals/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- microagents-cli/src/tui/mod.rs
1 participant
Summary by CodeRabbit
--no-prompt-cacheand defaults provider behavior to OpenAI.