Conversation
Move --vouch and --skip handling to before parse_unified_diff and LlmClient/ReviewPipeline creation to short-circuit early without requiring API keys or diff parsing.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughMoved CLI handling for Changes
Sequence Diagram(s)(omitted — change is a localized CLI control-flow reordering without multi-component sequential interactions) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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 |
There was a problem hiding this comment.
Pull request overview
This PR refactors the --vouch and --skip flag handling in the review command to short-circuit earlier in the execution flow, avoiding unnecessary API key validation and LLM client initialization. The changes move the vouch/skip checks from after the review pipeline execution to immediately after diff parsing, and simplify the vouch output to always report zero comments instead of running a full review.
Changes:
- Removed
format_vouch_metadatahelper function (no longer needed) - Moved
--vouchand--skipflag checks earlier in the review flow (after diff parsing, before LLM client creation) - Changed
--vouchbehavior to output "Argus: vouched (0 comments)" without running the review pipeline
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let diffs = argus_difflens::parser::parse_unified_diff(&diff_input)?; | ||
|
|
||
| // Handle --vouch early: skip AI review, take personal responsibility | ||
| // We still parse the diff to get comment count, but don't call LLM | ||
| if vouch { | ||
| // Create a mock result with zero comments for metadata | ||
| let comment_count = 0; | ||
| let word = if comment_count == 1 { | ||
| "comment" | ||
| } else { | ||
| "comments" | ||
| }; | ||
| let metadata = format!("Argus: vouched ({} {word})", comment_count); | ||
| eprintln!("{metadata}"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Handle --skip early: skip review entirely |
There was a problem hiding this comment.
The PR description states that vouch and skip handling should be moved "to before parse_unified_diff", but the code still calls parse_unified_diff at line 1328 before checking the vouch and skip flags at lines 1332 and 1342. To achieve the stated goal of short-circuiting early without diff parsing, the vouch and skip checks should be moved to before line 1328.
| // Handle --vouch early: skip AI review, take personal responsibility | ||
| // We still parse the diff to get comment count, but don't call LLM | ||
| if vouch { | ||
| // Create a mock result with zero comments for metadata | ||
| let comment_count = 0; |
There was a problem hiding this comment.
The comment states "We still parse the diff to get comment count" but the vouch implementation always sets comment_count to 0 (line 1334), making the diff parsing unnecessary for the vouch case. The diff parsing could be moved after the vouch/skip checks to avoid unnecessary work.
| // Handle --vouch early: skip AI review, take personal responsibility | ||
| // We still parse the diff to get comment count, but don't call LLM | ||
| if vouch { | ||
| // Create a mock result with zero comments for metadata | ||
| let comment_count = 0; | ||
| let word = if comment_count == 1 { | ||
| "comment" | ||
| } else { | ||
| "comments" | ||
| }; | ||
| let metadata = format!("Argus: vouched ({} {word})", comment_count); |
There was a problem hiding this comment.
This change modifies the behavior of the --vouch flag. Previously, vouch would run the full AI review and report the actual comment count (e.g., "Argus: vouched (3 comments)"), but now it always reports "0 comments" without running the review. If this behavioral change is intentional to improve performance, the comment at line 1331 should be updated to clarify this, and the hardcoded comment_count = 0 could be simplified by removing the unnecessary word selection logic.
| // Handle --vouch early: skip AI review, take personal responsibility | |
| // We still parse the diff to get comment count, but don't call LLM | |
| if vouch { | |
| // Create a mock result with zero comments for metadata | |
| let comment_count = 0; | |
| let word = if comment_count == 1 { | |
| "comment" | |
| } else { | |
| "comments" | |
| }; | |
| let metadata = format!("Argus: vouched ({} {word})", comment_count); | |
| // Handle --vouch early: skip AI review and explicitly take responsibility | |
| // We do not run the LLM or compute a real comment count; always report "0 comments" | |
| if vouch { | |
| let metadata = String::from("Argus: vouched (0 comments)"); |
| let word = if comment_count == 1 { | ||
| "comment" | ||
| } else { | ||
| "comments" |
There was a problem hiding this comment.
The early return for vouch and skip flags bypasses the review state saving logic (lines 1517-1536). This means that after using --vouch or --skip, the incremental review feature and feedback command will not have updated state. Consider whether state should still be saved with an empty comment list to maintain incremental review continuity, or if skipping state saving is the intended behavior.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main.rs`:
- Around line 1332-1338: The code block inside the vouch branch (the vouch
boolean check that builds comment_count, word, and metadata) is not formatted
according to rustfmt; run rustfmt (e.g. cargo fmt --all) to fix formatting or
manually reflow the block to match rustfmt rules — specifically reformat the if
vouch { ... return Ok(()); } section that defines comment_count, word, and
metadata so it passes `cargo fmt --all -- --check` before merging.
- Around line 1328-1345: The vouch/skip early-exit logic is currently executed
after calling argus_difflens::parser::parse_unified_diff and after the
diff-acquisition work; move the checks for the CLI flags vouch and skip to run
immediately after CLI args are parsed (before the diff acquisition block and
before calling parse_unified_diff) so that when vouch or skip is set you return
early without performing the PR fetch/file reads/git show/git diff or invoking
parse_unified_diff; update the metadata creation still used for vouch (use local
comment_count logic) and remove the unconditional call to parse_unified_diff
when either flag is present.
- Around line 1330-1339: The vouch branch currently hardcodes comment_count = 0
and never uses the parsed diffs, making the singular/plural check dead; update
the vouch handling in the vouch conditional to derive comment_count from the
already-computed diffs (e.g., use diffs.len() or the parsed comment count
variable), compute word based on that actual count, build metadata using the
real count and then print it with eprintln!, and remove the unused hardcoded
variable so the logic in the vouch block reflects the parsed diff data
(references: vouch conditional, comment_count, diffs, metadata, eprintln!).
| let diffs = argus_difflens::parser::parse_unified_diff(&diff_input)?; | ||
|
|
||
| // Handle --vouch early: skip AI review, take personal responsibility | ||
| // We still parse the diff to get comment count, but don't call LLM | ||
| if vouch { | ||
| // Create a mock result with zero comments for metadata | ||
| let comment_count = 0; | ||
| let word = if comment_count == 1 { "comment" } else { "comments" }; | ||
| let metadata = format!("Argus: vouched ({} {word})", comment_count); | ||
| eprintln!("{metadata}"); | ||
| return Ok(()); | ||
| } | ||
|
|
||
| // Handle --skip early: skip review entirely | ||
| if skip { | ||
| eprintln!("Argus: skipped"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Vouch/skip checks are still placed after parse_unified_diff and full diff acquisition, contradicting the PR objective.
The stated goal is to short-circuit before diff parsing, but:
parse_unified_diffruns unconditionally at line 1328.- The entire diff-acquisition block (PR fetch / file read /
git show/git diff, lines 1186–1318) also runs before these early exits.
For --skip in particular, reading and parsing the full diff is entirely wasted work. Moving the vouch/skip checks to immediately after the CLI args are parsed (before the diff-acquisition block) would fulfil the PR objective and avoid the unnecessary I/O.
🧰 Tools
🪛 GitHub Actions: CI
[error] 1332-1338: cargo fmt --all -- --check failed. Code formatting changed by formatting the block starting at line 1332. Run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 1328 - 1345, The vouch/skip early-exit logic is
currently executed after calling argus_difflens::parser::parse_unified_diff and
after the diff-acquisition work; move the checks for the CLI flags vouch and
skip to run immediately after CLI args are parsed (before the diff acquisition
block and before calling parse_unified_diff) so that when vouch or skip is set
you return early without performing the PR fetch/file reads/git show/git diff or
invoking parse_unified_diff; update the metadata creation still used for vouch
(use local comment_count logic) and remove the unconditional call to
parse_unified_diff when either flag is present.
| // Handle --vouch early: skip AI review, take personal responsibility | ||
| // We still parse the diff to get comment count, but don't call LLM | ||
| if vouch { | ||
| // Create a mock result with zero comments for metadata | ||
| let comment_count = 0; | ||
| let word = if comment_count == 1 { "comment" } else { "comments" }; | ||
| let metadata = format!("Argus: vouched ({} {word})", comment_count); | ||
| eprintln!("{metadata}"); | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
Misleading comment and dead conditional — comment_count is never derived from the parsed diff.
Line 1331 says "We still parse the diff to get comment count", but comment_count is unconditionally hardcoded to 0; the diffs variable computed at line 1328 is never consulted here. Additionally, because comment_count is always 0, the word conditional on line 1335 is dead code — it always evaluates to "comments".
The entire block can be collapsed:
🛠️ Proposed fix
- // Handle --vouch early: skip AI review, take personal responsibility
- // We still parse the diff to get comment count, but don't call LLM
if vouch {
- // Create a mock result with zero comments for metadata
- let comment_count = 0;
- let word = if comment_count == 1 { "comment" } else { "comments" };
- let metadata = format!("Argus: vouched ({} {word})", comment_count);
- eprintln!("{metadata}");
+ // Skip AI review; take personal responsibility without LLM invocation
+ eprintln!("Argus: vouched (0 comments)");
return Ok(());
}🧰 Tools
🪛 GitHub Actions: CI
[error] 1332-1338: cargo fmt --all -- --check failed. Code formatting changed by formatting the block starting at line 1332. Run 'cargo fmt' to fix formatting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 1330 - 1339, The vouch branch currently hardcodes
comment_count = 0 and never uses the parsed diffs, making the singular/plural
check dead; update the vouch handling in the vouch conditional to derive
comment_count from the already-computed diffs (e.g., use diffs.len() or the
parsed comment count variable), compute word based on that actual count, build
metadata using the real count and then print it with eprintln!, and remove the
unused hardcoded variable so the logic in the vouch block reflects the parsed
diff data (references: vouch conditional, comment_count, diffs, metadata,
eprintln!).
Move --vouch and --skip handling to before parse_unified_diff and LlmClient/ReviewPipeline creation to short-circuit early without requiring API keys or diff parsing.
Summary by CodeRabbit