Update Cargo.toml, add Gemini example, refine error handling #6
Conversation
… to help show how to use the library
WalkthroughThe pull request updates dependency configurations in Changes
Sequence Diagram(s)sequenceDiagram
participant Main as Main (main)
participant AskAI as ask_ai()
participant Client as GeminiClient
participant Provider as LlmProvider
participant Parser as GeminiResponseParser
participant Out as StdOut
Main->>AskAI: Invoke ask_ai()
AskAI->>Client: Initialize and send request
Client-->>AskAI: Return streaming response chunks
loop For each chunk
AskAI->>Parser: Call parse_chunk(chunk)
Parser-->>AskAI: Return parsed text (if available)
end
AskAI->>Out: Write parsed text to stdout
Note right of AskAI: Errors are handled with context
Assessment against linked issues
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (8)
examples/gemini.rs (3)
9-15: Consider unifying error types
Defining a separate localErrorenum is perfectly valid for this example. However, if you want to reduce duplication, consider merging with or extending the mainLlmErrortype to maintain consistency throughout the codebase.
33-69: Potential partial JSON chunk handling
The method assumes each chunk is a valid JSON object or bracket token. If the remote endpoint sends partial JSON objects across chunk boundaries, this could cause parse errors. Consider buffering incomplete data between calls toparse_chunkso you can handle streaming splits more robustly.
107-110: Handle top-level errors
Yourmainfunction is succinct. You may want to add user-friendly logging or retry logic here, in case of unhandled errors.src/error.rs (2)
33-44: Custom context vs. backtrace
ErrorContextstoring file and line data is useful. Depending on needs, you could consider Rust’sBacktraceor other introspection features for more detailed debugging information.
210-226:error_context!macro
Excellent convenience macro for creating structured error contexts. Future expansions might include more metadata or specialized formatting without complicating calling patterns.src/utils/doc.rs (2)
32-34: Consider making the mock response configurable.The current implementation returns a static "response" string. Consider making this configurable to support different test scenarios.
async fn chat_completion(&self, input: Self::Input) -> Result<Self::Output, Self::Error> { - Ok(SimpleOutput("response".to_string())) + Ok(SimpleOutput(format!("Mock response for input: {}", input.0))) }
36-39: Consider implementing streaming functionality.The current implementation returns an empty stream. Consider implementing basic streaming functionality to better simulate real LLM behavior.
async fn stream_chat_completion(&self, input: Self::Input) -> Result<impl ::futures_util::Stream<Item = Result<Self::StreamedOutput, Self::Error>>, Self::Error> { - Ok(::futures_util::stream::empty()) + use ::futures_util::stream; + Ok(stream::iter(vec![ + Ok("Mock ".to_string()), + Ok("streaming ".to_string()), + Ok(format!("response for: {}", input.0)) + ])) }Cargo.toml (1)
37-39: Consider pinning the exact version for self-dependency.When using path dependencies in dev-dependencies, it's recommended to pin the exact version to ensure reproducible builds.
[dev-dependencies] -tosic-llm = { path = ".", features = ["doc-utils"] } +tosic-llm = { path = ".", version = "=0.1.0", features = ["doc-utils"] } tokio = { version = "1.43.0", features = ["full"] }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(2 hunks)examples/gemini.rs(1 hunks)src/error.rs(1 hunks)src/gemini/mod.rs(4 hunks)src/lib.rs(1 hunks)src/provider.rs(2 hunks)src/utils/doc.rs(1 hunks)src/utils/mod.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/utils/mod.rs
🔇 Additional comments (14)
examples/gemini.rs (3)
1-8: Imports look good
No issues found with these import statements.
20-23: Struct definition is clear
Using flags likehas_startedandhas_finishedis a straightforward approach for controlling partial parsing flow.
71-99: Good usage of streaming
The logic for streaming responses is clear and well-structured. If the response isn't a stream, you currently return an error. Consider logging or an alternate fallback for a better user experience in production.src/error.rs (6)
8-31: Broadened error coverage
Supporting multiple error variants such asReqwest,Parse,Json,Io, andGenericmakes error handling more precise. TheContextvariant further enriches diagnostic capabilities.
46-78: Layered context approach
add_contextandnew_contextbuild upon existing errors nicely, but be cautious about repeatedly nesting contexts. Excessive recursion can make errors more verbose and harder to parse.
80-123: Strong implementation details
CombiningFrom<E>conversions, standard traits (Display,Error), and a consistentsource()method ensures a clean error chain. This design will yield clear error messages and enable easy debugging.
125-208: Error context extensions
YourWithContexttrait provides a convenient interface for attaching contextual information to bothResultandOption. This is a powerful and clean approach to error reporting.
228-234:bail!macro
A straightforward way to short-circuit on errors. This aligns well with typical early-return patterns in Rust.
235-242:ensure!macro
Enforcing runtime invariants withensure!is beneficial. It keeps error handling consistent and self-documenting.src/lib.rs (1)
12-12: Exposing the library-specificResult
Making thisResulttype public eases cross-crate usage and keeps error types more consistent throughout your ecosystem.src/provider.rs (2)
52-56: LGTM! Clean and efficient constructor implementation.The
#[inline(always)]attribute is appropriate here as this is a simple constructor.
78-94: LGTM! Clear and comprehensive example usage.The example effectively demonstrates both static and streaming response handling using the mock client.
src/gemini/mod.rs (1)
71-71: LGTM! Improved error handling with context.The addition of error context using the
contextmethod anderror_context!macro improves error messages and debugging capabilities.Also applies to: 86-86
Cargo.toml (1)
6-9: Verify the docs.rs cargo arguments.The
-Zunstable-optionsand-Zrustdoc-scrape-examplesare unstable features. Ensure they are supported by the current docs.rs build environment.✅ Verification successful
🌐 Web query:
Are -Zunstable-options and -Zrustdoc-scrape-examples supported by docs.rs?💡 Result:
Yes,
-Zunstable-optionsand-Zrustdoc-scrape-examplesare supported by Docs.rs when properly configured. Here's how it works:
Enabling through Cargo.toml metadata
You must add these flags to your crate'sCargo.tomlunder[package.metadata.docs.rs]:[package.metadata.docs.rs] cargo-args = ["-Zunstable-options", "-Zrustdoc-scrape-examples"]This tells Docs.rs to pass these flags during documentation builds[2][3].
Implementation details
- Docs.rs uses nightly Rust and accepts unstable features through explicit configuration[4]
- Examples are scraped from
examples/directory using Rustdoc's code analysis[2][5]- Shows up to 5 examples per item with toggleable visibility[2]
Handling dev-dependencies
If your examples require dev-dependencies, you must explicitly enable scraping per example:[[example]] name = "your_example" doc-scrape-examples = trueThis signals acceptance of dev-dependency builds[8].
A real-world example can be seen in the
tello-educrate where scraped examples appear in documentation after proper configuration[8]. The feature works through Docs.rs' nightly toolchain but requires explicit opt-in due to its unstable nature[2][5].Citations:
- 1: https://docs.rs/about
- 2: https://doc.rust-lang.org/rustdoc/scraped-examples.html
- 3: https://docs.rs/about/metadata
- 4: https://github.com/rust-lang/docs.rs/
- 5: https://rust-lang.github.io/rfcs/3123-rustdoc-scrape-examples.html
- 6: https://docs.rs/anyhow/latest/anyhow/
- 7: https://docs.rs/reqwest/latest/reqwest/
- 8: https://users.rust-lang.org/t/docs-rs-scraped-examples-with-dev-dependencies/92903
- 9: https://docs.rs/serde/latest/serde/
- 10: https://docs.rs/ffi-support/latest/ffi_support/
Docs.rs Flag Verification
Both the
-Zunstable-optionsand-Zrustdoc-scrape-examplesflags are supported by docs.rs when configured as shown. The current[package.metadata.docs.rs]configuration in your Cargo.toml is valid and aligns with docs.rs requirements.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 8
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
Cargo.toml(2 hunks)examples/gemini.rs(1 hunks)src/error.rs(1 hunks)src/gemini/mod.rs(4 hunks)src/lib.rs(1 hunks)src/provider.rs(2 hunks)src/utils/doc.rs(1 hunks)src/utils/mod.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/utils/doc.rs (1)
Learnt from: retrokiller543
PR: retrokiller543/tosic-llm#6
File: src/utils/doc.rs:12-16
Timestamp: 2025-02-11T20:50:08.362Z
Learning: The `mocked_llm_client!` macro in `src/utils/doc.rs` is a mock implementation specifically for doctests and is not used in production code. It's conditionally compiled only for documentation and testing purposes.
🔇 Additional comments (18)
examples/gemini.rs (4)
1-8: Imports look fine.
They correctly bring in the necessary dependencies and traits (likethiserror::ErrorandStreamExt).
25-32: Constructor logic is straightforward and clear.
Self-explanatory usage of booleans. No issues here.
71-105: Consider robust upstream data validation.
The function ensuresres.is_stream(), but if the stream contains corrupted JSON or unexpected chunk formatting,parse_chunkwill raise errors. This is acceptable for a sample, but you might want fallback or logging for chunk parse failures in a production scenario.
107-110: Main function usage is appropriate.
The top-level async function returnsResult; error handling is concise. Overall, it’s a clear entry point example.src/error.rs (3)
46-78: CustomLlmErrorstruct is consistent, but be mindful of recursive context.
StoringBox<LlmErrorType>within aContextcould lead to deep recursion if chaining becomes extensive. Currently, it seems safe, but be aware of potential complexity in repeated nested contexts.
176-208: Helpful handling ofOption<T>withEmptyvariant.
This extension viaWithContextis handy for situations whereNonearises. Good approach for encoding expectation failures into typed errors.
210-242: Macros are well-structured and convenient.
They cover common error patterns (error_context!,bail!,ensure!) with minimal ceremony. Great job keeping them flexible for usage across the codebase.src/lib.rs (1)
12-12: Inconsistency with the AI-generated summary.
The summary states thatResultusesLlmErrorTypeby default, but the code still referencesLlmError. Verify that tyingResulttoLlmErrorhere is intentional. If you intended to referenceLlmErrorType, this line needs updating.Likely an incorrect or invalid review comment.
src/utils/mod.rs (1)
3-3: LGTM!The addition of the
docmodule is clean and doesn't interfere with existing functionality.src/utils/doc.rs (2)
1-2: LGTM! Well-structured conditional compilation.The mock implementation is properly gated behind documentation and test features, and hidden from documentation.
12-16: Implementation uses todo!() as discussed.Based on the retrieved learning, this is intentional as the mock is specifically for doctests and won't be used in production.
src/provider.rs (2)
52-56: LGTM! Clean constructor implementation.The new constructor is well-documented and properly marked with inline optimization.
78-80: LGTM! Clear example usage.The example effectively demonstrates the usage of the mock client for both static and streaming responses.
src/gemini/mod.rs (2)
6-6: LGTM! Enhanced error handling.Good addition of WithContext and error_context macro for more informative error messages.
Also applies to: 19-19
71-71: LGTM! Improved error context.The error messages now provide clear context about where and why errors occur.
Also applies to: 86-86
Cargo.toml (3)
17-17: Verify the custom registry configuration.The
tosic-utilsdependency uses a custom registry "gitea". Ensure this registry is properly configured in your environment.❌ Verification inconclusive
🏁 Script executed:
#!/bin/bash # Check if the gitea registry is configured cargo config get registries.giteaLength of output: 131
It appears that the
cargocommand isn’t available in the current environment (as indicated by the “cargo: command not found” error), so we couldn’t automatically retrieve the custom registry configuration. Please manually verify that your custom registry"gitea"is correctly configured—typically by checking your local.cargo/configor.cargo/config.toml(or any relevant global Cargo configuration) to ensure that the registry details for"gitea"are properly set up.
6-9: Verify the unstable docs.rs options.The docs.rs metadata uses unstable cargo options. Please ensure these are required and work as expected.
✅ Verification successful
🌐 Web query:
What are the current stable options for docs.rs metadata in Cargo.toml?💡 Result:
The current stable configuration options for customizing docs.rs builds through
[package.metadata.docs.rs]inCargo.tomlinclude:Core Configuration Flags
features: Array of features to enable (default: empty)features = ["serde", "async"]all-features: Boolean to enable all features (default: false)no-default-features: Boolean to disable default features (default: false)[3][7]Target Configuration
default-target: Primary target for documentation landing pagedefault-target = "x86_64-pc-windows-msvc"targets: Additional targets to build documentation for[3][8]Build Flags
rustc-args: Extra compiler flagsrustc-args = ["--cfg", "special_feature"]rustdocflags: Custom rustdoc argumentsrustdocflags = ["--cfg", "docsrs"]Advanced Configuration
cargo-args: Unstable cargo flags (requires nightly toolchain)cargo-args = ["-Zbuild-std"]While some configurations like
cargo-argswith-Zflags require nightly Rust internally on docs.rs[1][4], these metadata keys themselves are part of stable Rust's Cargo manifest format[7]. Recent changes have stabilized common patterns like#[cfg(docsrs)]detection without manual configuration[5].For full compatibility, limit usage to non-nightly-dependent options unless explicitly requiring unstable features through cargo args[10].
Citations:
- 1: https://internals.rust-lang.org/t/why-use-nightly-for-docs-rs/20409
- 2: https://doc.rust-lang.org/cargo/commands/cargo-metadata.html
- 3: https://docs.rs/about/metadata
- 4: Consider recommending `cargo +nightly docs-rs --open` for local documentation builds tokio-rs/tokio#6469
- 5: Remove manual
--cfg docsrsinCargo.tomlmetadata bevyengine/bevy#14132- 6: https://rustwiki.org/en/cargo/commands/cargo-metadata.html
- 7: https://doc.rust-lang.org/cargo/reference/manifest.html
- 8: Crate failed to build rust-lang/docs.rs#23
- 9: https://docs.rs/cargo_metadata/latest/cargo_metadata/struct.Package.html
- 10: https://crates.io/crates/cargo-docs-rs
Re: Unstable docs.rs options in Cargo.toml
The current stable docs.rs configuration supports the usage of unstable flags via the advancedcargo-argsoption—but only when a nightly toolchain is used on docs.rs. Since your configuration specifiescargo-argswith-Zunstable-optionsand-Zrustdoc-scrape-examples, please confirm that your crate intentionally depends on these unstable features and that you are prepared to use the nightly environment on docs.rs.
- Note: The unstable flags are valid when using nightly, but for maximum compatibility, consider if stable alternatives (like
rustc-argsorrustdocflags) might suffice if nightly isn’t required.
24-24: Verify reqwest version for security updates.Since reqwest is used for HTTP requests, it's critical to ensure you're using a secure version.
✅ Verification successful
🌐 Web query:
What is the latest version of reqwest crate and are there any security advisories?💡 Result:
The latest stable version of the reqwest crate as of February 2025 is 0.12.9, based on its changelog entries showing recent updates to decompression handling and TLS revocation list support[3][4].
Security Advisories
RUSTSEC-2025-0001 (Critical):
Affectsrust-gst-plugin-reqwestthrough its OpenSSL dependency:
- Requires updating
opensslto v0.10.70 andopenssl-systo v0.9.105[9]- Impacts Fedora 41 packages until patched
Indirect Vulnerabilities:
- Reqwest depends on
hyper,tokio, andopenssl, which occasionally have security updates[9]- No direct vulnerabilities reported for reqwest itself in the RustSec database[2][8]
Recommended Actions
- Update using:
[dependencies] reqwest = "0.12"- Run
cargo update -p reqwestand rebuild[1]- Monitor RustSec Advisory Database for updates[2][8]
The crate maintains active development with recent fixes for connection reuse and TLS enhancements[3]. For WASM targets, use
reqwest_wasi(latest: 0.1.0)[4].Citations:
- 1: https://webscraping.ai/faq/reqwest/how-do-i-update-reqwest-to-the-latest-version
- 2: https://rustsec.org/contributing.html
- 3: https://github.com/seanmonstar/reqwest/blob/master/CHANGELOG.md
- 4: https://docs.rs/reqwest_wasi/latest/reqwest/
- 5: https://users.rust-lang.org/t/how-safe-is-crates-io/91290
- 6: https://ranchermanager.docs.rancher.com/reference-guides/rancher-security/security-advisories-and-cves
- 7: https://users.rust-lang.org/t/why-am-i-getting-this-error-with-reqwest-api/66887
- 8: https://github.com/rustsec/advisory-db
- 9: https://linuxsecurity.com/advisories/fedora/fedora-41-rust-gst-plugin-reqwest-2025-f8be7978e3-security-advisory-updates-ofnqzfbvtpkn
- 10: https://crates.io/crates/reqwest
Reqwest Dependency Security Verified
- The dependency is set to version 0.12.12 with
default-features = falseand uses therustls-tlsfeature, ensuring it does not pull in the OpenSSL stack and its associated vulnerabilities.- Although the web query indicates that the latest stable version as of February 2025 is 0.12.9, no direct vulnerabilities for reqwest have been reported, and using a version above that baseline (0.12.12) appears to be safe.
- Continue monitoring indirect advisories (such as those affecting hyper, tokio, and other dependencies) via the RustSec Advisory Database.
| #[derive(Debug, Error)] | ||
| enum Error { | ||
| #[error(transparent)] | ||
| Llm(#[from] LlmError), | ||
| #[error("{0}")] | ||
| Generic(String), | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider making the Error enum public if used across modules.
Currently, the Error enum is private to this file. If you anticipate needing it elsewhere, adding pub here can help maintain clarity and consistency across your project and keep your error types consolidated.
| #[derive(Debug)] | ||
| pub struct GeminiResponseParser { | ||
| has_started: bool, | ||
| has_finished: bool, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider adding comments or docstrings.
GeminiResponseParser is self-explanatory, but docstrings can clarify the reasoning and usage patterns.
| pub fn parse_chunk(&mut self, chunk: &[u8]) -> JsonResult<Option<String>> { | ||
| // Convert bytes to string | ||
| let chunk_str = String::from_utf8_lossy(chunk); | ||
|
|
||
| // Handle the start and end markers | ||
| if chunk_str == "[" { | ||
| self.has_started = true; | ||
| return Ok(None); | ||
| } else if chunk_str == "]" || chunk_str.is_empty() { | ||
| self.has_finished = true; | ||
| return Ok(None); | ||
| } | ||
|
|
||
| // Remove leading comma if present (subsequent chunks start with ,\r\n) | ||
| let cleaned_chunk = if chunk_str.starts_with(",") { | ||
| chunk_str.trim_start_matches(",").trim_start() | ||
| } else { | ||
| &chunk_str | ||
| }; | ||
|
|
||
| // Parse the JSON object | ||
| let v: Value = serde_json::from_str(cleaned_chunk)?; | ||
|
|
||
| // Extract the text from the nested structure | ||
| let text = v | ||
| .get("candidates") | ||
| .and_then(|c| c.get(0)) | ||
| .and_then(|c| c.get("content")) | ||
| .and_then(|c| c.get("parts")) | ||
| .and_then(|p| p.get(0)) | ||
| .and_then(|p| p.get("text")) | ||
| .and_then(|t| t.as_str()) | ||
| .map(String::from); | ||
|
|
||
| Ok(text) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Verify handling of chunked JSON.
Because the code handles [, ], and partial data, confirm you never receive a chunk that splits a JSON object across boundaries. Otherwise, serde_json::from_str will fail. If partial objects are possible, buffering logic might be necessary to reassemble JSON.
Could you confirm how GeminiClient streams JSON to ensure each chunk is a complete object (or bracket) for this parser?
| #[derive(Debug, Error, IsVariant)] | ||
| pub enum LlmErrorType { | ||
| #[error(transparent)] | ||
| Reqwest(#[from] reqwest::Error), | ||
| #[error(transparent)] | ||
| Parse(#[from] ParseError), | ||
| #[error(transparent)] | ||
| Json(#[from] serde_json::Error), | ||
| #[error(transparent)] | ||
| Io(#[from] std::io::Error), | ||
| #[error("An error occurred: {0}")] | ||
| Generic(#[from] Box<dyn std::error::Error + Send>), | ||
| #[error("{message}")] | ||
| Context { | ||
| message: String, | ||
| #[source] | ||
| source: Box<LlmErrorType>, | ||
| }, | ||
| #[error("Value was empty. {value}, expected one of: {expected:?}")] | ||
| Empty { | ||
| value: String, | ||
| expected: &'static [&'static str], | ||
| }, | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Robust error variant definitions.
The choice to wrap external errors (reqwest::Error, ParseError, etc.) and introduce a context variant is good for layering context. Consider adding doc comments for each variant to improve maintainability.
| [lib] | ||
| test = true | ||
| doctest = true |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider removing redundant library settings.
The test and doctest settings are enabled by default for libraries. These explicit settings are unnecessary unless you have a specific reason to include them.
|
|
||
| [dev-dependencies] | ||
| tosic-llm = { path = ".", features = ["doc-utils"] } | ||
| tokio = { version = "1.43.0", features = ["full"] } |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider limiting tokio features in dev-dependencies.
Using features = ["full"] for tokio includes all features, which might be unnecessary for testing. Consider specifying only the features you need (e.g., "rt", "macros", "rt-multi-thread").
-tokio = { version = "1.43.0", features = ["full"] }
+tokio = { version = "1.43.0", features = ["rt", "macros", "rt-multi-thread"] }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| tokio = { version = "1.43.0", features = ["full"] } | |
| tokio = { version = "1.43.0", features = ["rt", "macros", "rt-multi-thread"] } |
Resolves #5
Summary by CodeRabbit
New Features
Refactor
Chores
Resulttype alias public for better accessibility across modules.