Fix multiple bugs and performance issues across the workspace#60
Merged
Fix multiple bugs and performance issues across the workspace#60
Conversation
…mples The previous `&ex[..200]` byte-slicing could panic when truncating multi-byte UTF-8 characters. Replaced with `ex.chars().take(200).collect()` which correctly handles character boundaries.
HashMap iteration order is non-deterministic, causing the same set of diffs to produce different LLM prompts across runs. Switching to BTreeMap ensures files are grouped in a consistent lexicographic order by directory.
The review subcommand previously required .argus.toml to exist, bailing with an error if missing. This was redundant since the config loader already falls back to ArgusConfig::default(). Changed the hard error to a stderr hint so users can run reviews without creating a config file.
feedback.rs and state.rs violated the project convention of using thiserror/ArgusError for library crate errors and reserving miette for the binary crate only. Replaced miette::Result with Result<T, ArgusError> and removed the now-unused miette dependency from argus-review.
…tries chat_gemini had its own 5-retry loop with exponential backoff for 429 responses, but the pipeline already handles retries via chat_with_rate_limit_retries. The duplicated retry logic caused up to 5*3=15 retries for Gemini while other providers got only 3. Now all providers retry consistently through the pipeline's retry mechanism.
… tokio generate_map, build_related_code_context, and build_history_insights perform synchronous file I/O and CPU-heavy parsing (tree-sitter, git2) that can block the tokio worker thread. Wrapped each in tokio::task::block_in_place so the runtime can schedule other tasks while these operations run. Uses block_in_place rather than spawn_blocking to avoid cloning referenced data.
1. Changed review() to take Vec<FileDiff> by value instead of &[FileDiff] to avoid cloning the entire diff vector for the filter. 2. Added estimate_diffs_tokens() that computes token estimates directly from diff components without building an intermediate String. The full diff_text is now only built when needed for self-reflection and summary rather than eagerly for the split decision.
…replacements The previous logic used the patch line count to determine how many original lines to remove, causing a 3-line patch targeting line 5 to delete lines 5-7 and replace them. This silently destroyed unrelated code when the patch expanded a single line into multiple lines. Fixed by always replacing only the single target line with the full patch content. Added a regression test verifying that a multi-line patch expanding one line does not overwrite subsequent lines.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
⌛ 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)
📝 WalkthroughWalkthroughThe PR removes the miette dependency and migrates error handling to ArgusError across argus-review, changes ReviewPipeline to take owned diffs and improve token estimation and grouping, simplifies Gemini request logic, fixes patch application to avoid overwriting subsequent lines, and makes missing config non-fatal. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 3
🧹 Nitpick comments (1)
crates/argus-review/src/pipeline.rs (1)
411-411: Avoid rebuilding full diff text in the single-call path.Line 411 already materializes
diff_textfor review, then Lines 476-479 rebuild it again unconditionally. This reintroduces avoidable large allocation work in non-split flows.Also applies to: 476-479
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/argus-review/src/pipeline.rs` at line 411, The code is rebuilding the full diff text twice: once at let diff_text = diffs_to_text(&kept_diffs) and again unconditionally later; remove the second diffs_to_text call and reuse the already-materialized diff_text (or pass a reference) in the single-call path. Update the block that currently calls diffs_to_text(&kept_diffs) again (the later unconditional allocation) to use the existing diff_text variable (or borrow &diff_text) so large allocations are avoided in the non-split flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/argus-review/src/pipeline.rs`:
- Around line 236-238: Format the closure passed to tokio::task::block_in_place
to satisfy rustfmt: replace the braced closure form with a single-expression
closure so it reads tokio::task::block_in_place(||
build_related_code_context(&kept_diffs, &index_path)); — locate the call to
tokio::task::block_in_place(...) that invokes build_related_code_context and
change the closure body to the concise single-expression form (and add a
trailing semicolon if appropriate for the surrounding expression).
In `@crates/argus-review/src/state.rs`:
- Around line 35-43: cargo fmt failures are caused by unformatted Rust code in
the block that reads/parses the review state (the read_to_string call and
serde_json::from_str call producing ArgusError::Config), so run rustfmt (cargo
fmt) or reflow the closure formatting to match rustfmt style: normalize spacing
and line breaks in the map_err closures around
read_to_string(&state_path).map_err(|e| { ArgusError::Config(format!(...)) })?
and serde_json::from_str(&content).map_err(|e|
ArgusError::Config(format!(...)))? so that the closures, format! calls, and ?
operators follow rustfmt conventions and CI will pass.
In `@src/main.rs`:
- Around line 1263-1266: The new hint block is failing rustfmt; reformat it (or
run cargo fmt) so it matches rustfmt style—ensure the if condition using
cli.config.is_none() and std::path::Path::new(".argus.toml").exists() and the
eprintln!(...) line are rustfmt-normalized (preserve the logic but adjust
spacing/indentation to rustfmt rules) and commit the resulting change.
---
Nitpick comments:
In `@crates/argus-review/src/pipeline.rs`:
- Line 411: The code is rebuilding the full diff text twice: once at let
diff_text = diffs_to_text(&kept_diffs) and again unconditionally later; remove
the second diffs_to_text call and reuse the already-materialized diff_text (or
pass a reference) in the single-call path. Update the block that currently calls
diffs_to_text(&kept_diffs) again (the later unconditional allocation) to use the
existing diff_text variable (or borrow &diff_text) so large allocations are
avoided in the non-split flow.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
crates/argus-review/Cargo.tomlcrates/argus-review/src/feedback.rscrates/argus-review/src/llm.rscrates/argus-review/src/patch.rscrates/argus-review/src/pipeline.rscrates/argus-review/src/prompt.rscrates/argus-review/src/state.rssrc/main.rs
💤 Files with no reviewable changes (1)
- crates/argus-review/Cargo.toml
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses multiple critical bugs and performance issues across the Argus codebase, focusing on patch application, UTF-8 handling, determinism, async runtime efficiency, error handling standardization, and allocation optimization.
Changes:
- Fixed patch application to replace only the target line instead of multiple lines, preventing file corruption when patches expand single lines into multiple lines
- Replaced byte-slicing with character-based truncation to prevent UTF-8 panics in negative example handling
- Switched from HashMap to BTreeMap for deterministic LLM prompt generation
- Wrapped synchronous blocking operations in
tokio::task::block_in_placeto prevent async runtime starvation - Removed internal retry logic from Gemini client to avoid double-retries with the existing pipeline retry mechanism
- Migrated argus-review library crate from
miette::ResulttoResult<T, ArgusError>for consistency with library error handling conventions - Changed review command to print a warning instead of failing when
.argus.tomlis missing, allowing operation with defaults - Optimized token estimation and diff handling to reduce allocations by passing ownership instead of cloning
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Changed config requirement from error to warning; passed diffs by value instead of reference |
| crates/argus-review/src/state.rs | Migrated from miette::Result to ArgusError with detailed error messages |
| crates/argus-review/src/prompt.rs | Fixed UTF-8 panic by using character-based truncation instead of byte-slicing |
| crates/argus-review/src/pipeline.rs | Added BTreeMap for determinism, block_in_place for async safety, optimized token estimation; deferred diff_text allocation |
| crates/argus-review/src/patch.rs | Fixed multi-line patch application to replace only the target line; added regression test |
| crates/argus-review/src/llm.rs | Removed internal Gemini retry loop to defer to pipeline-level retry mechanism |
| crates/argus-review/src/feedback.rs | Migrated from miette::Result to ArgusError |
| crates/argus-review/Cargo.toml | Removed miette dependency from library crate |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+719
to
+720
| total_bytes += 6 + diff.old_path.as_os_str().len() + 1; | ||
| total_bytes += 6 + diff.new_path.as_os_str().len() + 1; |
There was a problem hiding this comment.
The token estimation for file paths may be inaccurate. Using as_os_str().len() counts the platform-specific byte representation, but diffs_to_text uses display() which produces a UTF-8 string. On some platforms, these can differ. Consider using diff.old_path.to_string_lossy().len() and diff.new_path.to_string_lossy().len() to match the actual format used in diffs_to_text.
Suggested change
| total_bytes += 6 + diff.old_path.as_os_str().len() + 1; | |
| total_bytes += 6 + diff.new_path.as_os_str().len() + 1; | |
| total_bytes += 6 + diff.old_path.to_string_lossy().len() + 1; | |
| total_bytes += 6 + diff.new_path.to_string_lossy().len() + 1; |
Comment on lines
+100
to
+101
| let display_ex = if ex.chars().count() > 200 { | ||
| let truncated: String = ex.chars().take(200).collect(); |
There was a problem hiding this comment.
The character count check iterates through all characters twice for strings longer than 200 characters. Consider optimizing by collecting first 200 characters unconditionally and checking the length, or by using a single pass approach. However, this is a minor performance issue since negative_examples are typically small in number.
Suggested change
| let display_ex = if ex.chars().count() > 200 { | |
| let truncated: String = ex.chars().take(200).collect(); | |
| let mut truncated = String::with_capacity(200); | |
| let mut char_count = 0usize; | |
| let mut exceeded = false; | |
| for ch in ex.chars() { | |
| if char_count < 200 { | |
| truncated.push(ch); | |
| } | |
| char_count += 1; | |
| if char_count > 200 { | |
| exceeded = true; | |
| break; | |
| } | |
| } | |
| let display_ex = if exceeded { |
crates/argus-review/src/pipeline.rs
Outdated
|
|
||
| // Build the full diff text for self-reflection and summary | ||
| // (deferred from earlier to avoid allocation when splitting into groups) | ||
| let diff_text = diffs_to_text(&kept_diffs); |
There was a problem hiding this comment.
When the single LLM call path is taken, diff_text is allocated twice: once at line 411 for the review prompt, and again at line 478 for self-reflection. Consider reusing the diff_text variable or restructuring to avoid this double allocation. One approach: declare diff_text before the if/else and initialize it conditionally, or move the line 478 allocation inside the split path only.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/argus-review/src/pipeline.rs (1)
703-705:⚠️ Potential issue | 🟠 MajorFix CI blocker:
estimate_tokensis now dead code.The pipeline failure at Line 703 is valid. Since this helper is only used by tests, gate it to test builds and remove the intra-doc link dependency.
🛠️ Proposed fix
-fn estimate_tokens(text: &str) -> usize { +#[cfg(test)] +fn estimate_tokens(text: &str) -> usize { text.len() / 4 } /// Estimate tokens for a slice of diffs without building the full text string. /// -/// Uses the same `len / 4` heuristic as [`estimate_tokens`] but computes +/// Uses the same `len / 4` heuristic but computes /// the byte length directly from the diff components to avoid a large /// intermediate allocation.Also applies to: 709-710
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/argus-review/src/pipeline.rs` around lines 703 - 705, The helper function estimate_tokens is only used by tests and is causing a CI failure as dead code; mark it as test-only and remove any intra-doc link that references it: add #[cfg(test)] above fn estimate_tokens(text: &str) -> usize (or move the function into the tests module) so it is compiled only for test builds, and delete or update any intra-doc link pointing to estimate_tokens in docs/comments to avoid the dependency.
🧹 Nitpick comments (1)
crates/argus-review/src/pipeline.rs (1)
409-409: Avoid rebuilding full diff text in the single-call path.Line 409 builds
diff_text, then Lines 474-476 build it again. Reusing the first string avoids a large duplicate allocation on big diffs.♻️ Suggested refactor sketch
+ let mut cached_diff_text: Option<String> = None; ... - let diff_text = diffs_to_text(&kept_diffs); + let diff_text = diffs_to_text(&kept_diffs); + cached_diff_text = Some(diff_text.clone()); ... - let diff_text = diffs_to_text(&kept_diffs); + let diff_text = cached_diff_text.unwrap_or_else(|| diffs_to_text(&kept_diffs));Also applies to: 474-476
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/argus-review/src/pipeline.rs` at line 409, The code currently calls diffs_to_text(&kept_diffs) twice (once into diff_text at the top and again in the single-call path), causing a large duplicate allocation; change the single-call path to reuse the previously-created diff_text variable (or borrow it) instead of calling diffs_to_text a second time so the string is allocated once — update any branches that call diffs_to_text(&kept_diffs) (the second call in the single-call path) to use diff_text from the outer scope (or pass &diff_text) and remove the redundant invocation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/argus-review/src/pipeline.rs`:
- Around line 222-227: The calls to tokio::task::block_in_place inside
ReviewPipeline::review (used around argus_repomap::generate_map and two other
places) are unsafe on a current-thread Tokio runtime and can panic; update
ReviewPipeline::review to either (a) perform a runtime check before each
block_in_place invocation (use tokio::runtime::Handle::try_current() and
error/return if the handle indicates a current-thread runtime), (b) replace
block_in_place with tokio::task::spawn_blocking to run the blocking work off the
async thread, or (c) add a clear doc comment on ReviewPipeline::review requiring
a multi-threaded runtime and validate at runtime; choose one approach and apply
consistently for all three block_in_place usages (identify them by
tokio::task::block_in_place and the surrounding argus_repomap::generate_map call
sites) so the method no longer panics under a current-thread runtime.
---
Outside diff comments:
In `@crates/argus-review/src/pipeline.rs`:
- Around line 703-705: The helper function estimate_tokens is only used by tests
and is causing a CI failure as dead code; mark it as test-only and remove any
intra-doc link that references it: add #[cfg(test)] above fn
estimate_tokens(text: &str) -> usize (or move the function into the tests
module) so it is compiled only for test builds, and delete or update any
intra-doc link pointing to estimate_tokens in docs/comments to avoid the
dependency.
---
Nitpick comments:
In `@crates/argus-review/src/pipeline.rs`:
- Line 409: The code currently calls diffs_to_text(&kept_diffs) twice (once into
diff_text at the top and again in the single-call path), causing a large
duplicate allocation; change the single-call path to reuse the
previously-created diff_text variable (or borrow it) instead of calling
diffs_to_text a second time so the string is allocated once — update any
branches that call diffs_to_text(&kept_diffs) (the second call in the
single-call path) to use diff_text from the outer scope (or pass &diff_text) and
remove the redundant invocation.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/argus-review/src/pipeline.rscrates/argus-review/src/state.rsnpm/bin/argussrc/main.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main.rs
Comment on lines
+222
to
+227
| tokio::task::block_in_place(|| { | ||
| match argus_repomap::generate_map(root, 1024, &focus_files, OutputFormat::Text) { | ||
| Ok(map) if !map.is_empty() => Some(map), | ||
| _ => None, | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Tokio runtime flavor usage and where review() is invoked.
# 1) Find Tokio runtime entrypoints/flavors.
echo "=== Tokio runtime entrypoints/decorators ==="
rg -nP --type rust '#\[tokio::main(?:\([^)]*\))?\]|#\[tokio::test(?:\([^)]*\))?\]'
echo ""
echo "=== Tokio Builder usage ==="
rg -nP --type rust 'Builder::new_current_thread\(|Builder::new_multi_thread\(|new_current_thread\(|new_multi_thread\('
# 2) Find block_in_place usage sites.
echo ""
echo "=== block_in_place usage sites ==="
rg -nP --type rust '\bblock_in_place\s*\('
# 3) Find ReviewPipeline::review call sites to validate they run under multi-thread runtime paths.
echo ""
echo "=== ReviewPipeline::review call sites ==="
rg -nP --type rust '\.review\s*\('Repository: Meru143/argus
Length of output: 682
🏁 Script executed:
#!/bin/bash
echo "=== Check tokio::main at src/main.rs:813 ==="
sed -n '810,820p' src/main.rs
echo ""
echo "=== Check review() call at src/main.rs:1473 ==="
sed -n '1470,1480p' src/main.rs
echo ""
echo "=== Check Tokio version ==="
rg -A 5 '^\s*tokio\s*=' Cargo.toml | head -20
echo ""
echo "=== Check ReviewPipeline::review signature ==="
rg -B 5 -A 10 'fn review\s*\(' crates/argus-review/src/pipeline.rs | head -30Repository: Meru143/argus
Length of output: 2002
🏁 Script executed:
#!/bin/bash
echo "=== Check ReviewPipeline visibility ==="
rg -B 10 'pub.*struct ReviewPipeline' crates/argus-review/src/
echo ""
echo "=== Check if review() is public and could be called externally ==="
rg -B 2 'pub async fn review' crates/argus-review/src/
echo ""
echo "=== Check for tokio::test with current_thread flavor ==="
rg -n '#\[tokio::test.*current_thread' --type rust
echo ""
echo "=== Check all tokio::test decorators ==="
rg -n '#\[tokio::test' --type rust | head -20
echo ""
echo "=== Verify library/public API exports ==="
rg -n '^pub use.*ReviewPipeline|^pub.*ReviewPipeline' crates/argus-review/src/lib.rsRepository: Meru143/argus
Length of output: 1265
🏁 Script executed:
#!/bin/bash
echo "=== Check crates/argus-review directory structure ==="
ls -la crates/argus-review/src/ | head -20
echo ""
echo "=== Check if lib.rs exists ==="
ls -la crates/argus-review/src/lib.rs 2>&1
echo ""
echo "=== Check crates/argus-review/Cargo.toml ==="
cat crates/argus-review/Cargo.toml
echo ""
echo "=== Inspect crates/argus-review/src/lib.rs if it exists ==="
if [ -f crates/argus-review/src/lib.rs ]; then
cat crates/argus-review/src/lib.rs
fiRepository: Meru143/argus
Length of output: 2081
🏁 Script executed:
#!/bin/bash
echo "=== Inspect review() method and surrounding context ==="
sed -n '170,240p' crates/argus-review/src/pipeline.rs
echo ""
echo "=== Check for any runtime assertions or checks ==="
rg -n 'try_current\|current_thread\|is_finished\|Handle::current' crates/argus-review/src/pipeline.rsRepository: Meru143/argus
Length of output: 2824
Verify runtime safety: block_in_place will panic on current-thread Tokio runtime.
Lines 222, 236, and 246 unconditionally call tokio::task::block_in_place, which panics when invoked from a current-thread Tokio runtime. Since ReviewPipeline::review() is a public async method in the library crate, external callers could invoke it from a current-thread runtime context without protective documentation or runtime checks.
Either add runtime validation before each block_in_place call (e.g., using tokio::runtime::Handle::try_current()), document the multi-thread requirement explicitly in the method's doc comment, or refactor to avoid block_in_place.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@crates/argus-review/src/pipeline.rs` around lines 222 - 227, The calls to
tokio::task::block_in_place inside ReviewPipeline::review (used around
argus_repomap::generate_map and two other places) are unsafe on a current-thread
Tokio runtime and can panic; update ReviewPipeline::review to either (a) perform
a runtime check before each block_in_place invocation (use
tokio::runtime::Handle::try_current() and error/return if the handle indicates a
current-thread runtime), (b) replace block_in_place with
tokio::task::spawn_blocking to run the blocking work off the async thread, or
(c) add a clear doc comment on ReviewPipeline::review requiring a multi-threaded
runtime and validate at runtime; choose one approach and apply consistently for
all three block_in_place usages (identify them by tokio::task::block_in_place
and the surrounding argus_repomap::generate_map call sites) so the method no
longer panics under a current-thread runtime.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses several critical bugs, architectural flaws, and performance bottlenecks identified during a comprehensive codebase review.
Fixes include:
apply_patchesto accurately replace the targeted line without accidentally deleting adjacent original lines if the patch content is multi-line.&ex[..200]byte-slicing forex.chars().take(200)to safely truncate negative examples containing multi-byte UTF-8 characters.HashMaptoBTreeMapingroup_related_diffsso LLM prompts are consistently generated in alphabetical order, making tests and outputs reproducible.argus_repomap::generate_mapand SQLite queries) intokio::task::block_in_placeto prevent them from starving the Tokio worker threads.chat_gemini, deferring to the standard 3-retry pipeline used by other providers to avoid hanging the review process for minutes.feedback.rsandstate.rsaway frommiette::ResulttoResult<T, ArgusError>, aligning theargus-reviewlibrary crate with the established project conventions (thiserrorfor libraries,miettefor binaries).argus reviewto run gracefully with default settings when.argus.tomlis missing, improving the out-of-the-box user experience..to_vec()clones ofFileDiffstructures and redundantdiffs_to_textformatting for token estimation with lightweight iteration and ownership passing.Test plan
cargo checkandcargo testto verify no regressions in the pipeline, error handling, or patch application logic.argus reviewruns without.argus.tomlpresent using default settings.Summary by CodeRabbit
Bug Fixes
Improvements