Conversation
|
Warning Rate limit exceeded@arthrod has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe updates introduce a library crate configuration, add a new library file with utility functions for token truncation and model selection, and enhance error handling in the main executable. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MainRS
participant LibRS
participant Git
participant Env
User->>MainRS: Run auto-commit
MainRS->>Git: git diff --name-only --staged
Git-->>MainRS: List of staged files
MainRS->>Git: git diff --staged
Git-->>MainRS: Staged diff
MainRS->>LibRS: truncate_to_n_tokens(combined_diff, MAX_DIFF_TOKENS)
LibRS-->>MainRS: Truncated diff string
MainRS->>LibRS: get_model_from_env()
LibRS->>Env: Read AUTO_COMMIT_MODEL
Env-->>LibRS: Model name or default
LibRS-->>MainRS: Model name
MainRS->>User: Generate commit message using selected model and truncated diff
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hello @arthrod, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
Summary of Changes
Hello! Gemini here, providing a summary of this pull request to help everyone get up to speed quickly.
This PR focuses on improving the auto-commit tool in a few key areas. Firstly, it introduces the ability to configure the OpenAI model used for generating commit messages via the AUTO_COMMIT_MODEL environment variable, defaulting to gpt-4.1-nano if not set. This configuration is also documented in the README.
Secondly, it significantly enhances the handling of the Git diff input provided to the model. The tool now specifically uses the staged diff (git diff --staged), adds more robust error handling for Git command execution and UTF-8 conversion of output, and limits the size of the combined changed files list and diff output to a maximum number of 'tokens' (implemented as whitespace-delimited words) to prevent exceeding model context limits.
Finally, minor project structure changes include removing Cargo.lock from .gitignore and adding a placeholder Cargo.lock, and defining the core logic as a library in Cargo.toml.
Highlights
- Model Configuration: The OpenAI model used for generating commit messages can now be specified using the
AUTO_COMMIT_MODELenvironment variable, providing flexibility for users. - Improved Git Diff Handling: The tool now uses the staged diff (
git diff --staged), includes better error handling for Git command failures and output encoding issues, and fetches the list of changed files separately. - Diff Size Limiting: A new function
truncate_to_n_tokensis introduced and used to limit the size of the combined changed files list and diff output sent to the model, preventing potential context window issues. - Documentation Update: The README has been updated to include instructions on how to use the new
AUTO_COMMIT_MODELenvironment variable. - Project Structure: The project now explicitly defines its core logic as a library in
Cargo.toml, andCargo.lockis no longer ignored, with a placeholder added.
Changelog
Click here to see the changelog
- .gitignore
- Removed
Cargo.lockfrom the ignored files list.
- Removed
- Cargo.lock
- Added a placeholder
Cargo.lockfile with a comment explaining its purpose.
- Added a placeholder
- Cargo.toml
- Added a
[lib]section to define the crate as a library namedauto_commit.
- Added a
- README.md
- Added documentation for the
AUTO_COMMIT_MODELenvironment variable, explaining how to set it and its default value.
- Added documentation for the
- src/lib.rs
- Added
truncate_to_n_tokensfunction to limit string length by whitespace-delimited words. - Defined
DEFAULT_MODELconstant for the default OpenAI model name. - Added
get_model_from_envfunction to fetch the model name from the environment variable or use the default. - Included unit tests for the new
truncate_to_n_tokensandget_model_from_envfunctions.
- Added
- src/main.rs
- Imported
get_model_from_envandtruncate_to_n_tokensfrom theauto_commitlibrary. - Added
MAX_DIFF_TOKENSconstant to define the maximum size of the diff input. - Improved error handling for
git diff --stagedandgit rev-parsecommands usingmap_err. - Added error handling for UTF-8 conversion of Git command output.
- Changed the primary diff command from
git diff HEADtogit diff --staged. - Added a new command
git diff --name-only --stagedto get the list of changed files. - Combined the list of changed files and the diff output into a single string.
- Applied
truncate_to_n_tokensto the combined diff input before sending it to the model. - Updated the OpenAI API call to use the model name fetched by
get_model_from_env()instead of a hardcoded value.
- Imported
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Diffs show the change,
Staged files in a new range,
AI reads the code.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
Overall, this pull request introduces several valuable improvements, including better error handling for git commands, documenting the model environment variable, and setting up the project as a library. The addition of tests for the new library functions is also great. The changes align well with the stated goals of the pull request. I've reviewed the code based on common Rust practices, as no specific style guide was provided.
Summary of Findings
- Truncation logic vs. Tokenization: The
truncate_to_n_tokensfunction truncates by whitespace-delimited words, which may not align with the model's actual tokenization and could lead to unexpected behavior regarding token limits. Consider renaming or using a proper tokenization library. - Error Handling for
git rev-parse: Theunwrap_or("")in thegit rev-parseerror handling might mask UTF-8 errors and incorrectly proceed. Using amatchstatement to log the error and exit would be safer. - Redundant Git Commands: The
git diff --stagedcommand is executed multiple times. Running it once and reusing the output would improve efficiency. - Test Coverage: Tests were added for the new library functions, which is good. (Note: Per review settings, no direct comment was made on the test code itself).
- Documentation: Documentation for the
AUTO_COMMIT_MODELenvironment variable was added to the README. (Note: Per review settings, no direct comment was made on the README changes). - Code Structure: Moving functions to
src/lib.rsand adding the[lib]section inCargo.tomlimproves modularity. (Note: Per review settings, no direct comment was made on these changes). - Constant Usage: The
MAX_DIFF_TOKENSconstant was introduced, which is good practice. (Note: Per review settings, no direct comment was made on this change). - Placeholder Cargo.lock: A placeholder
Cargo.lockwas added and the.gitignoreupdated accordingly. (Note: Per review settings, no direct comment was made on these changes).
Merge Readiness
This pull request introduces good improvements, particularly in error handling and modularity. However, there is a high-severity issue regarding the redundant execution of a git command that should be addressed before merging. Additionally, consider the medium-severity issues related to the truncation logic and the git rev-parse error handling for further robustness and clarity. I am unable to approve this pull request; please have other reviewers review and approve this code before merging.
| let files_output = Command::new("git") | ||
| .arg("diff") | ||
| .arg("--name-only") | ||
| .arg("--staged") | ||
| .output() | ||
| .map_err(|e| { | ||
| error!("Couldn't get changed files: {}", e); | ||
| () | ||
| })? | ||
| .stdout; | ||
| let files_changed = match str::from_utf8(&files_output) { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| error!("Changed files output was not valid UTF-8: {}", e); | ||
| "" | ||
| } | ||
| }; | ||
|
|
||
| let diff_output = Command::new("git") | ||
| .arg("diff") | ||
| .arg("HEAD") | ||
| .arg("--staged") | ||
| .output() | ||
| .expect("Couldn't find diff.") | ||
| .map_err(|e| { | ||
| error!("Couldn't find diff: {}", e); | ||
| () | ||
| })? | ||
| .stdout; | ||
| let output = str::from_utf8(&output).unwrap(); | ||
| let diff_output = match str::from_utf8(&diff_output) { | ||
| Ok(v) => v, | ||
| Err(e) => { | ||
| error!("Diff output was not valid UTF-8: {}", e); | ||
| "" | ||
| } | ||
| }; |
There was a problem hiding this comment.
You are executing git diff --staged twice here (lines 138-153) after already executing it once earlier (lines 81-97). Running the same command multiple times is inefficient and could potentially lead to inconsistencies if the staged files change between the calls (though unlikely in this context).
Could you refactor this to run git diff --staged only once and reuse the result?
let files_output = Command::new("git")
.arg("diff")
.arg("--name-only")
.arg("--staged")
.output()
.map_err(|e| {
error!("Couldn't get changed files: {}", e);
()
})?
.stdout;
let files_changed = match str::from_utf8(&files_output) {
Ok(v) => v,
Err(e) => {
error!("Changed files output was not valid UTF-8: {}", e);
""
}
};
// Reuse the git_staged_cmd from earlier
let diff_output = git_staged_cmd;
let combined = format!("Changed files:\n{}\n\nDiff:\n{}", files_changed, diff_output);| /// Truncate a string to the first `limit` whitespace-delimited words. | ||
| pub fn truncate_to_n_tokens(text: &str, limit: usize) -> String { | ||
| text.split_whitespace().take(limit).collect::<Vec<_>>().join(" ") |
There was a problem hiding this comment.
The function truncate_to_n_tokens currently truncates based on whitespace-delimited words. While this might be a reasonable heuristic, it doesn't accurately reflect the tokenization used by large language models, which is typically based on subword units (like BPE). This could lead to the actual token count being significantly different from the limit provided, potentially exceeding the model's context window or truncating too aggressively.
Would it be more accurate to rename this function to reflect that it truncates by words (e.g., truncate_to_n_words) or consider using a proper tokenization library (like tiktoken-rs) if the intent is to strictly limit based on model tokens?
![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
🔭 Outside diff range comments (1)
README.md (1)
5-5: 🛠️ Refactor suggestionUpdate model reference in introduction
Line 5 mentions GPT-3.5, but the default model is now gpt-4.1-nano. This inconsistency could confuse users.
-A CLI tool that generates commit messages from your staged changes, built in Rust and using [OpenAI's GPT-3.5](https://platform.openai.com/overview). +A CLI tool that generates commit messages from your staged changes, built in Rust and using [OpenAI's models](https://platform.openai.com/overview).
🧹 Nitpick comments (3)
src/main.rs (3)
67-67: Consider documenting the MAX_DIFF_TOKENS constantAdding a comment explaining why 20,000 tokens was chosen as the limit would improve maintainability.
-const MAX_DIFF_TOKENS: usize = 20_000; +/// Maximum number of tokens to include in the diff sent to the AI model. +/// This helps prevent exceeding the model's context window limit. +const MAX_DIFF_TOKENS: usize = 20_000;
107-116: Great improvement to error handling, but consider refinementThe error handling is much better, but line 113 still uses
unwrap_or("")which could be improved to match the pattern used elsewhere.- if str::from_utf8(&is_repo).unwrap_or("").trim() != "true" { + let is_repo_str = match str::from_utf8(&is_repo) { + Ok(v) => v, + Err(e) => { + error!("Repository check output was not valid UTF-8: {}", e); + "" + } + }; + if is_repo_str.trim() != "true" {
155-156: Consider using a more descriptive variable name.The variable name
combinedis a bit generic. A more descriptive name would improve code readability.-let combined = format!("Changed files:\n{}\n\nDiff:\n{}", files_changed, diff_output); -let output = truncate_to_n_tokens(&combined, MAX_DIFF_TOKENS); +let git_content = format!("Changed files:\n{}\n\nDiff:\n{}", files_changed, diff_output); +let truncated_git_content = truncate_to_n_tokens(&git_content, MAX_DIFF_TOKENS);
📜 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 (5)
.gitignore(0 hunks)Cargo.toml(1 hunks)README.md(1 hunks)src/lib.rs(1 hunks)src/main.rs(5 hunks)
💤 Files with no reviewable changes (1)
- .gitignore
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/main.rs (1)
src/lib.rs (2)
get_model_from_env(10-12)truncate_to_n_tokens(2-4)
🔇 Additional comments (8)
Cargo.toml (1)
27-29: LGTM! Good library configuration.The addition of the library configuration is well-structured and follows Rust conventions. This modularization will help with code organization and reuse.
src/lib.rs (2)
6-12: LGTM! Good environment variable handling.The function and constant are well-documented and properly implemented. Using
unwrap_or_elsewith a default is a good pattern for handling environment variables.
14-36: LGTM! Well-structured tests.The tests are comprehensive and properly verify both functions. Good practice using environment variable cleanup in the tests to prevent interference between test cases.
src/main.rs (4)
24-24: LGTM! Proper imports from the new library module.Correctly imports the new utility functions from the library.
85-97: LGTM! Improved error handling.The change from
expectto proper error handling with logging is a significant improvement. This prevents the application from crashing on unexpected errors.
120-154: LGTM! Good separation of concerns.Fetching the file list separately from the diff content improves code clarity. The error handling is also well-implemented.
254-254: LGTM! Dynamic model selection.Using the environment variable for model selection improves configurability.
README.md (1)
31-35: LGTM! Clear documentation of the new environment variable.The documentation clearly explains the purpose of the
AUTO_COMMIT_MODELenvironment variable and its default value.
| /// Truncate a string to the first `limit` whitespace-delimited words. | ||
| pub fn truncate_to_n_tokens(text: &str, limit: usize) -> String { | ||
| text.split_whitespace().take(limit).collect::<Vec<_>>().join(" ") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Function name doesn't accurately reflect implementation
The function is named truncate_to_n_tokens but it's actually truncating based on whitespace-delimited words, not tokens in the NLP sense. OpenAI's tokenization is more complex and based on subword units, not just whitespace.
Either rename the function to better reflect its behavior:
-/// Truncate a string to the first `limit` whitespace-delimited words.
-pub fn truncate_to_n_tokens(text: &str, limit: usize) -> String {
+/// Truncate a string to the first `limit` whitespace-delimited words.
+pub fn truncate_to_n_words(text: &str, limit: usize) -> String {
text.split_whitespace().take(limit).collect::<Vec<_>>().join(" ")
}Or implement actual token-based truncation using a proper tokenizer if needed.
🤖 Prompt for AI Agents
In src/lib.rs lines 1 to 4, the function name truncate_to_n_tokens is misleading
because it truncates based on whitespace-delimited words, not true NLP tokens.
To fix this, either rename the function to something like truncate_to_n_words to
accurately reflect its behavior, or implement actual token-based truncation
using a proper tokenizer library that handles subword units as OpenAI
tokenization does.
…el-selection-bsavnq
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
1 participant
Summary
AUTO_COMMIT_MODELenvironment variable in the READMEDEFAULT_MODELTesting
cargo test --offline(fails: no matching package namedasync-openaifound)