feat(review): add --commit flag to review already-committed changes#40
feat(review): add --commit flag to review already-committed changes#40
Conversation
📝 WalkthroughWalkthroughA new Changes
Sequence DiagramsequenceDiagram
participant User
participant CLI as CLI Handler
participant Git as Git Process
participant StateManager as State Manager
User->>CLI: review --commit <commit_sha>
CLI->>Git: git diff <commit_sha>
alt Diff succeeds
Git-->>CLI: diff output
CLI->>Git: git rev-parse HEAD
alt HEAD resolution succeeds
Git-->>CLI: current_head
CLI->>StateManager: save state with diff & current_head
StateManager-->>CLI: state saved
CLI-->>User: review result
else HEAD resolution fails
Git-->>CLI: error
CLI-->>User: diagnostic error
end
else Diff fails
Git-->>CLI: error
CLI-->>User: diagnostic error with git output
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 adds a --commit flag to the argus review command to enable reviewing already-committed changes, addressing issue #36. The feature allows users to specify commit references like HEAD, HEAD~3, or HEAD~3..HEAD to review historical changes. However, the implementation contains a critical bug in how it constructs the git diff command, and the deletion of the GitHub Actions workflow file is unexplained.
Changes:
- Added
--commitflag to the Review command enum with documentation - Implemented git diff execution logic for committed changes
- Deleted
.github/workflows/argus-review.ymlworkflow file
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/main.rs | Added --commit flag parameter and implementation logic to execute git diff for already-committed changes |
| .github/workflows/argus-review.yml | Completely deleted the Argus code review workflow (reason unexplained) |
Comments suppressed due to low confidence (1)
.github/workflows/argus-review.yml:1
- The workflow file .github/workflows/argus-review.yml is being completely deleted without explanation in the PR description. If this is intentional (perhaps because it's self-referential or could create circular workflows), it should be documented in the PR description. If this deletion is unintentional, it should be restored as it provides automated code review functionality for pull requests.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Review already-committed changes | ||
| let diff_output = std::process::Command::new("git") | ||
| .args(["-C", &repo_root.to_string_lossy(), "diff", &commit_ref]) | ||
| .output() | ||
| .into_diagnostic() | ||
| .wrap_err(format!("Failed to run git diff {}", commit_ref))?; |
There was a problem hiding this comment.
The git diff command is missing the commit parent specification. With the current implementation, git diff HEAD will show differences between HEAD and the working directory, not the changes introduced by HEAD itself. Similarly, git diff HEAD~3 shows differences between HEAD3 and working directory. To review the changes introduced by commits, you should use 3, HEAD~3..HEAD), you'll need different handling: single commits should use git diff <commit>^..<commit> or git show <commit> for single commits, or handle ranges appropriately. For the documented usage patterns (HEAD, HEAD<commit>^..<commit> format, and ranges should be passed directly.
| // Review already-committed changes | |
| let diff_output = std::process::Command::new("git") | |
| .args(["-C", &repo_root.to_string_lossy(), "diff", &commit_ref]) | |
| .output() | |
| .into_diagnostic() | |
| .wrap_err(format!("Failed to run git diff {}", commit_ref))?; | |
| // Review already-committed changes. For a single commit (e.g., HEAD, HEAD~3), | |
| // diff against its parent using `<commit>^..<commit>`. For explicit ranges | |
| // (e.g., HEAD~3..HEAD), pass the range as-is. | |
| let diff_spec = if commit_ref.contains("..") { | |
| commit_ref.clone() | |
| } else { | |
| format!("{}^..{}", commit_ref, commit_ref) | |
| }; | |
| let diff_output = std::process::Command::new("git") | |
| .args(["-C", &repo_root.to_string_lossy(), "diff", &diff_spec]) | |
| .output() | |
| .into_diagnostic() | |
| .wrap_err(format!("Failed to run git diff {}", diff_spec))?; |
| /// Review already-committed changes (e.g., HEAD, HEAD~3, or HEAD~3..HEAD) | ||
| #[arg(long)] | ||
| commit: Option<String>, |
There was a problem hiding this comment.
There's no validation to prevent conflicting flags from being used together. Users could potentially specify multiple input sources (--pr, --file, --commit, --incremental) simultaneously, leading to confusing behavior. Consider adding clap's conflicts_with attribute or explicit validation to ensure only one input source is specified at a time.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main.rs (1)
1194-1198:⚠️ Potential issue | 🟡 MinorError hint omits
--commitafter it was added as a valid input source🔧 Proposed fix
- help = "Pipe a diff to argus, e.g.: git diff | argus review --repo .\n Or use --file <path>, --pr owner/repo#123, or --incremental", + help = "Pipe a diff to argus, e.g.: git diff | argus review --repo .\n Or use --file <path>, --pr owner/repo#123, --commit HEAD, or --incremental",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1194 - 1198, The error message produced when both diff_input.trim().is_empty() and pr.is_none() triggers (the miette::bail! call in main.rs) is missing the newly supported --commit input; update the help text inside the miette::miette! invocation to list --commit alongside --file, --pr, and --incremental so the guidance includes the new valid input source.
🧹 Nitpick comments (1)
src/main.rs (1)
207-209:--commitsilently ignored when combined with--pror--fileThe new
--commitflag takes third place in the if-else chain (after--prand--file). If a user accidentally supplies both,--commitis silently dropped with no warning. Consider addingconflicts_withto the clap definition to give an explicit error at arg-parse time:🔧 Proposed clap annotation
- /// Review already-committed changes (e.g., HEAD, HEAD~3, or HEAD~3..HEAD) - #[arg(long)] - commit: Option<String>, + /// Review already-committed changes (e.g., HEAD, HEAD~3, or HEAD~3..HEAD) + #[arg(long, conflicts_with = "pr", conflicts_with_all = &["pr", "file", "incremental"])] + commit: Option<String>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 207 - 209, The --commit flag is silently ignored when combined with --pr or --file; update the CLI arg definition for the commit field (the #[arg(...)] on commit: Option<String>) to declare mutual conflicts with the pr and file flags (e.g., use conflicts_with_all for ["pr","file"] or equivalent conflicts_with entries) so clap will error at parse-time instead of silently dropping --commit; ensure the attribute references the exact flag names "--pr" and "--file".
🤖 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 1116-1127: The current branch in the else-if handling (the block
beginning with "else if let Some(commit_ref) = commit") incorrectly uses `git
diff <commit>` for single-commit refs which compares the working tree and yields
empty output on a clean repo; change the logic to detect whether `commit_ref`
contains a range ("..") and if so keep using `git diff <range>`, but if it's a
single commit (no "..") run `git show --patch --format="" <commit_ref>` to
produce the exact unified patch for that commit; preserve the existing error
handling around `diff_output`/`stderr` and the wrap_err message when invoking
the command so failures are reported the same way.
---
Outside diff comments:
In `@src/main.rs`:
- Around line 1194-1198: The error message produced when both
diff_input.trim().is_empty() and pr.is_none() triggers (the miette::bail! call
in main.rs) is missing the newly supported --commit input; update the help text
inside the miette::miette! invocation to list --commit alongside --file, --pr,
and --incremental so the guidance includes the new valid input source.
---
Nitpick comments:
In `@src/main.rs`:
- Around line 207-209: The --commit flag is silently ignored when combined with
--pr or --file; update the CLI arg definition for the commit field (the
#[arg(...)] on commit: Option<String>) to declare mutual conflicts with the pr
and file flags (e.g., use conflicts_with_all for ["pr","file"] or equivalent
conflicts_with entries) so clap will error at parse-time instead of silently
dropping --commit; ensure the attribute references the exact flag names "--pr"
and "--file".
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/argus-review.ymlsrc/main.rs
💤 Files with no reviewable changes (1)
- .github/workflows/argus-review.yml
| } else if let Some(commit_ref) = commit { | ||
| // Review already-committed changes | ||
| let diff_output = std::process::Command::new("git") | ||
| .args(["-C", &repo_root.to_string_lossy(), "diff", &commit_ref]) | ||
| .output() | ||
| .into_diagnostic() | ||
| .wrap_err(format!("Failed to run git diff {}", commit_ref))?; | ||
|
|
||
| if !diff_output.status.success() { | ||
| let stderr = String::from_utf8_lossy(&diff_output.stderr); | ||
| miette::bail!("git diff failed: {}", stderr.trim()); | ||
| } |
There was a problem hiding this comment.
git diff <single-ref> compares the working tree, not commit-to-parent — --commit HEAD always produces empty output on a clean repo
git diff <commit> compares "working directory to specific commit", and git diff HEAD compares "the Working Tree to the HEAD commit (all local changes)". On a clean working tree (which is the normal state after committing), git diff HEAD produces empty output, so argus review --commit HEAD immediately hits the "Empty diff input" bail at line 1194 — the primary advertised use-case is completely non-functional.
git diff HEAD~3 has a similar problem: it shows the accumulated working-tree diff from HEAD~3, not just the single commit at HEAD~3 as the doc comment implies. Only the range form (HEAD~3..HEAD) works correctly.
The fix is to dispatch on whether the ref is a range or a single commit:
- Range (
..present):git diff <range>— already correct. - Single commit:
git show --patch --format="" <commit>to get only the unified patch of that specific commit.
🐛 Proposed fix
- } else if let Some(commit_ref) = commit {
- // Review already-committed changes
- let diff_output = std::process::Command::new("git")
- .args(["-C", &repo_root.to_string_lossy(), "diff", &commit_ref])
- .output()
- .into_diagnostic()
- .wrap_err(format!("Failed to run git diff {}", commit_ref))?;
-
- if !diff_output.status.success() {
- let stderr = String::from_utf8_lossy(&diff_output.stderr);
- miette::bail!("git diff failed: {}", stderr.trim());
- }
+ } else if let Some(commit_ref) = commit {
+ // Review already-committed changes.
+ // For a range (e.g. HEAD~3..HEAD) use `git diff`; for a single commit
+ // use `git show --patch --format=` to get the diff the commit introduced.
+ let is_range = commit_ref.contains("..");
+ let (cmd_args, cmd_name): (Vec<&str>, &str) = if is_range {
+ (vec!["-C", &repo_root.to_string_lossy(), "diff", commit_ref], "git diff")
+ } else {
+ (vec!["-C", &repo_root.to_string_lossy(), "show", "--patch", "--format=", commit_ref], "git show")
+ };
+
+ let diff_output = std::process::Command::new("git")
+ .args(&cmd_args)
+ .output()
+ .into_diagnostic()
+ .wrap_err(format!("Failed to run {} {}", cmd_name, commit_ref))?;
+
+ if !diff_output.status.success() {
+ let stderr = String::from_utf8_lossy(&diff_output.stderr);
+ miette::bail!("{} failed: {}", cmd_name, stderr.trim());
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.rs` around lines 1116 - 1127, The current branch in the else-if
handling (the block beginning with "else if let Some(commit_ref) = commit")
incorrectly uses `git diff <commit>` for single-commit refs which compares the
working tree and yields empty output on a clean repo; change the logic to detect
whether `commit_ref` contains a range ("..") and if so keep using `git diff
<range>`, but if it's a single commit (no "..") run `git show --patch
--format="" <commit_ref>` to produce the exact unified patch for that commit;
preserve the existing error handling around `diff_output`/`stderr` and the
wrap_err message when invoking the command so failures are reported the same
way.
Meru143
force-pushed
the
issue/36-post-commit-review
branch
from
b26d626 to
270723c
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main.rs (1)
1168-1199:⚠️ Potential issue | 🔴 Critical
git diff <single-ref>breaks--commit HEADon clean repos.
git diff <commit>compares the working tree to the commit, so--commit HEADreturns empty diff after a clean commit. Branch on range vs single commit and usegit show --patch --format=for single refs.🐛 Proposed fix
- } else if let Some(commit_ref) = commit { - // Review already-committed changes - let diff_output = std::process::Command::new("git") - .args(["-C", &repo_root.to_string_lossy(), "diff", commit_ref]) - .output() - .into_diagnostic() - .wrap_err(format!("Failed to run git diff {}", commit_ref))?; + } else if let Some(commit_ref) = commit { + // Review already-committed changes. + // Range refs (..): git diff <range>; single refs: git show --patch --format= + let is_range = commit_ref.contains(".."); + let (cmd_args, cmd_name): (Vec<&str>, &str) = if is_range { + (vec!["-C", &repo_root.to_string_lossy(), "diff", commit_ref], "git diff") + } else { + ( + vec![ + "-C", + &repo_root.to_string_lossy(), + "show", + "--patch", + "--format=", + commit_ref, + ], + "git show", + ) + }; + + let diff_output = std::process::Command::new("git") + .args(&cmd_args) + .output() + .into_diagnostic() + .wrap_err(format!("Failed to run {} {}", cmd_name, commit_ref))?; if !diff_output.status.success() { let stderr = String::from_utf8_lossy(&diff_output.stderr); - miette::bail!("git diff failed: {}", stderr.trim()); + miette::bail!("{} failed: {}", cmd_name, stderr.trim()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1168 - 1199, The current use of git diff with a single commit ref (variable commit_ref) produces empty output for cases like --commit HEAD on a clean repo; change the logic where diff_output is created to branch on whether commit_ref is a range vs a single ref (inspect commit_ref for range syntax like ".." or other range indicators) and, for single refs, run git show --patch --format= <commit_ref> instead of git diff <commit_ref>, preserving the same error handling (wrap_err and status checks) and keeping the current_head logic intact so other code using diff_output and current_head (the variables diff_output and current_head) continues to work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main.rs`:
- Around line 1168-1199: The current use of git diff with a single commit ref
(variable commit_ref) produces empty output for cases like --commit HEAD on a
clean repo; change the logic where diff_output is created to branch on whether
commit_ref is a range vs a single ref (inspect commit_ref for range syntax like
".." or other range indicators) and, for single refs, run git show --patch
--format= <commit_ref> instead of git diff <commit_ref>, preserving the same
error handling (wrap_err and status checks) and keeping the current_head logic
intact so other code using diff_output and current_head (the variables
diff_output and current_head) continues to work.
Summary
Adds flag to review already-committed changes.
Usage
Closes
#36
Summary by CodeRabbit
New Features
--commitflag to the Review command for conducting reviews on specific commits within the repository.Bug Fixes