feat: implement incremental review (--incremental)#4
Conversation
Add 'argus describe' subcommand that generates PR titles, descriptions, and labels from diffs using LLM analysis. Features: - Conventional commit-style title generation (max 72 chars) - Structured description with summary, changes, and considerations - Label suggestions from common categories - Supports stdin, --file, and --pr input sources - Repository context via --repo for better descriptions - Progress spinner with elapsed time - All output formats: text, json, markdown Implementation: - PrDescription struct (Serialize + Deserialize) - build_describe_system_prompt() - LLM system instructions - build_describe_prompt() - user prompt with diff + context - parse_describe_response() - JSON response parser with fence stripping - Full CLI integration with error handling and spinners Also adds: - SPRINT-CONTROL and SPRINT-LOG.md for sprint tracking - SPRINT-TEMPLATE.md for sprint workflow documentation - Fixes clippy warnings (map_err -> inspect_err) Tests: 372 passing (10 new tests for describe feature) Entire-Checkpoint: b934bccc5ebd
Adds support for reviewing only new/changed code since the last review. Features: - New `--incremental` flag to review changes since last saved state - New `--base-sha` flag to manually specify a baseline commit - Persists review state (SHA + timestamp) to `.argus/review-state.json` - Automatically generates diffs using `git diff` (requires no stdin) - Fallback logic: defaults to reviewing uncommitted changes (HEAD) if no state exists - Robust error handling for git commands (exit status checks) Implementation: - `ReviewState` struct in `crates/argus-review/src/state.rs` - Loading/saving logic using `serde` + `chrono` - Integration in `src/main.rs` to override diff generation - Dependencies: `chrono` added to `argus-review` and `argus-ai` - `.gitignore` updated to exclude `.argus/` and artifacts Tests: - Manual verification of dirty tree review - State file creation verified - `cargo test` passing Entire-Checkpoint: b934bccc5ebd
|
Caution Review failedThe pull request is closed. 📝 WalkthroughWalkthroughThe pull request introduces incremental review capabilities with persistent state tracking and a new Changes
Sequence DiagramssequenceDiagram
participant User
participant CLI as Argus CLI
participant Git as Git Engine
participant State as Review State
participant LLM as LLM API
participant Output as Output Formatter
User->>CLI: review --incremental
CLI->>State: load(repo_root)
alt State exists
State-->>CLI: ReviewState with last_reviewed_sha
CLI->>Git: generate diff(last_reviewed_sha...HEAD)
else First run
CLI->>Git: generate diff(HEAD^...HEAD)
end
Git-->>CLI: diff content
CLI->>LLM: send diff + context
LLM-->>CLI: review response
CLI->>Output: format results
Output-->>User: display review
CLI->>State: save(current_head_sha, timestamp)
State-->>CLI: state persisted
sequenceDiagram
participant User
participant CLI as Argus CLI
participant Diff as Diff Source
participant LLM as LLM API
participant Parser as JSON Parser
participant Output as Output Formatter
User->>CLI: describe --pr <url> or --file <path>
alt PR source
CLI->>Diff: fetch GitHub PR diff
else File source
CLI->>Diff: read file
end
Diff-->>CLI: diff content
CLI->>LLM: build_describe_prompt(diff)
LLM-->>CLI: JSON response
CLI->>Parser: parse_describe_response()
Parser-->>CLI: PrDescription {title, description, labels}
CLI->>Output: format(Json|Markdown|Text)
Output-->>User: display PR description
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
✨ 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 pull request implements two major features for the Argus code review tool: incremental review mode and PR description generation. The incremental review feature allows users to review only changes made since the last review by persisting state in .argus/review-state.json, while the PR description generation feature adds a new argus describe subcommand that uses LLM to generate PR titles, descriptions, and labels from diffs.
Changes:
- Added
--incrementaland--base-shaflags to enable reviewing only new changes since a previous review or specified commit - Implemented state persistence in
.argus/review-state.jsonto track the last reviewed SHA and timestamp - Added new
argus describesubcommand for AI-powered PR description generation with conventional commit formatting and label suggestions - Extended prompt.rs with describe-specific prompts and response parsing, including comprehensive tests
Reviewed changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 13 comments.
Show a summary per file
| File | Description |
|---|---|
| src/main.rs | Added incremental review logic with git command execution, state management, and complete describe command implementation |
| crates/argus-review/src/state.rs | New module for ReviewState struct with load/save functionality for tracking review state |
| crates/argus-review/src/prompt.rs | Added describe-related prompt building and parsing functions with PrDescription struct and tests |
| crates/argus-review/src/lib.rs | Exported new state module |
| crates/argus-review/Cargo.toml | Added chrono and miette dependencies for state management |
| Cargo.toml | Added chrono and indicatif dependencies to main binary |
| Cargo.lock | Updated with new dependencies |
| .gitignore | Added .argus/ directory and review_output.txt to ignore list |
| SPRINT-TEMPLATE.md | New sprint planning template documenting feature backlog and process |
| SPRINT-LOG.md | New sprint log documenting completed features |
| SPRINT-CONTROL | New sprint control file for managing automated sprint chains |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| timestamp: Utc::now(), | ||
| }; | ||
| if let Err(e) = state.save(&repo_root) { | ||
| eprintln!("warning: failed to save review state: {e}"); |
There was a problem hiding this comment.
The state is saved after the review completes successfully, but if the state save fails (line 1324-1326), only a warning is printed. This could lead to confusion in subsequent incremental reviews where the state file doesn't reflect the last actual review. Consider whether a failed state save should be a hard error when using --incremental, or add a more prominent warning explaining the implications.
| eprintln!("warning: failed to save review state: {e}"); | |
| eprintln!("warning: failed to save review state: {e}"); | |
| eprintln!( | |
| "note: subsequent runs that rely on the review state (e.g. via --incremental) \ | |
| will not take this review into account; consider re-running once this \ | |
| issue is resolved." | |
| ); |
| let (diff_input, current_head_sha) = if let Some(pr_ref) = pr { | ||
| let (owner, repo, pr_number) = argus_review::github::parse_pr_reference(pr_ref)?; | ||
| let github = argus_review::github::GitHubClient::new(None)?; | ||
| github.get_pr_diff(&owner, &repo, pr_number).await? | ||
| (github.get_pr_diff(&owner, &repo, pr_number).await?, None) |
There was a problem hiding this comment.
State will be saved with current HEAD even when reviewing a PR (--pr flag). When reviewing a GitHub PR, the current_head_sha will be None (line 1099), so state won't be saved. However, if someone uses --incremental with --pr in the future, this could cause unexpected behavior. Consider documenting this interaction or explicitly validating that --incremental and --pr are mutually exclusive.
| s.last_reviewed_sha | ||
| } else { | ||
| eprintln!( | ||
| "No previous review state found. Reviewing uncommitted changes (HEAD)." |
There was a problem hiding this comment.
When no previous state exists, the fallback is to use "HEAD" as the diff base (line 1129). This means git diff HEAD will be executed, which shows uncommitted changes in the working tree. However, the message says "Reviewing uncommitted changes (HEAD)". This is correct but could be clearer - it's actually comparing the working tree against HEAD, not reviewing HEAD itself. Consider clarifying the message to "No previous review state found. Comparing working tree against HEAD."
| "No previous review state found. Reviewing uncommitted changes (HEAD)." | |
| "No previous review state found. Comparing working tree against HEAD." |
| /// Generate a PR title, description, and labels from a diff | ||
| #[command( | ||
| long_about = "Generate a PR title, description, and labels from a diff.\n\n\ | ||
| Analyzes code changes and uses an LLM to produce a well-formatted PR description\n\ | ||
| with conventional commit-style title, structured body, and suggested labels.\n\n\ | ||
| Examples:\n git diff main | argus describe\n argus describe --file changes.patch\n argus describe --pr owner/repo#123" | ||
| )] | ||
| Describe { | ||
| /// GitHub PR to describe (format: owner/repo#123) | ||
| #[arg(long)] | ||
| pr: Option<String>, | ||
| /// Read diff from file instead of stdin | ||
| #[arg(long)] | ||
| file: Option<PathBuf>, | ||
| /// Repository path for codebase context | ||
| #[arg(long)] | ||
| repo: Option<PathBuf>, | ||
| }, |
There was a problem hiding this comment.
The PR title says "feat: implement incremental review (--incremental)" but the PR includes two major features: 1) incremental review with --incremental and --base-sha flags, AND 2) a new argus describe subcommand for PR description generation. The describe command is a substantial addition (227 lines in main.rs + 108 lines in prompt.rs + tests) and should be mentioned in the title or split into a separate PR for clearer history. Consider updating the PR title to reflect both features, e.g., "feat: add incremental review and PR description generation".
| miette::bail!(miette::miette!( | ||
| help = "Pipe a diff to argus, e.g.: git diff | argus review --repo .\n Or use --file <path>, --pr owner/repo#123, or --incremental", | ||
| "Empty diff input" | ||
| )); |
There was a problem hiding this comment.
The empty diff check at line 1153 skips the check when pr.is_none() is false (i.e., when reviewing a PR). However, this logic doesn't account for incremental mode explicitly. If a user runs argus review --incremental and there are no changes since the last review, the diff will be empty, and this will bail with "Empty diff input". This is technically correct behavior, but the error message suggests using --incremental as a solution, which would be confusing since --incremental was already used. Consider adding a more specific message for the incremental case: "No changes since last review."
| miette::bail!(miette::miette!( | |
| help = "Pipe a diff to argus, e.g.: git diff | argus review --repo .\n Or use --file <path>, --pr owner/repo#123, or --incremental", | |
| "Empty diff input" | |
| )); | |
| if incremental || base_sha.is_some() { | |
| // In incremental/base-sha mode, an empty diff means there are no changes | |
| // relative to the last reviewed (or specified) baseline. | |
| miette::bail!(miette::miette!( | |
| help = "There are no changes to review since the last baseline. Make some changes or adjust --base-sha if you expected differences.", | |
| "No changes since last review" | |
| )); | |
| } else { | |
| miette::bail!(miette::miette!( | |
| help = "Pipe a diff to argus, e.g.: git diff | argus review --repo .\n Or use --file <path>, --pr owner/repo#123, or --incremental", | |
| "Empty diff input" | |
| )); | |
| } |
| let user = | ||
| argus_review::prompt::build_describe_prompt(&diff_input, repo_map.as_deref(), None); |
There was a problem hiding this comment.
The history_context parameter in build_describe_prompt is always passed as None (line 1411), even though the function supports it and there are tests for it. The codebase has gitpulse for analyzing git history and hotspots (mentioned in SPRINT-TEMPLATE.md line 18). Consider either: 1) removing the unused parameter if history context won't be used for PR descriptions, or 2) implementing history context extraction to provide richer context for PR description generation.
| - ~/argus/SPRINT-LOG.md (what's already shipped) | ||
|
|
||
| ## Feature Backlog (update as features ship) | ||
| 1. PR description generation — ~80% done in dirty tree |
There was a problem hiding this comment.
The SPRINT-TEMPLATE.md indicates "PR description generation — ~80% done in dirty tree" (line 16), but this PR appears to complete the feature. After merging this PR, consider updating the feature backlog in SPRINT-TEMPLATE.md to remove or mark as complete the PR description generation item, since it's now fully implemented and tested.
| 1. PR description generation — ~80% done in dirty tree | |
| 1. PR description generation — DONE |
|
|
||
| ## Feature Backlog (update as features ship) | ||
| 1. PR description generation — ~80% done in dirty tree | ||
| 2. Incremental review — only review NEW changes after force-push |
There was a problem hiding this comment.
The SPRINT-LOG.md entry for incremental review says "Incremental review — only review NEW changes after force-push" (line 17 in SPRINT-TEMPLATE and line 38 in SPRINT-LOG). However, the implementation doesn't specifically detect or handle force-pushes. It tracks the last reviewed SHA and diffs against it, which works for any changes (new commits, force pushes, or working tree changes). Consider updating this description to be more accurate: "Incremental review — only review changes since the last review".
| 2. Incremental review — only review NEW changes after force-push | |
| 2. Incremental review — only review changes since the last review |
| ) | ||
| } else { | ||
| read_diff_input(file)? | ||
| (read_diff_input(&None)?, None) |
There was a problem hiding this comment.
Inconsistent pattern for calling read_diff_input. This creates a temporary Option with &None, while other call sites like line 713 and 1356 pass the Option reference directly. For consistency and to avoid temporary allocations, consider refactoring to match the pattern used elsewhere in the file.
| (read_diff_input(&None)?, None) | |
| (read_diff_input(&file)?, None) |
| std::fs::write(state_path, content).into_diagnostic()?; | ||
| Ok(()) | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the new ReviewState module. The module handles critical functionality (loading/saving review state) but has no unit tests, unlike other modules in this crate (github.rs, llm.rs, patch.rs, pipeline.rs, prompt.rs, and sarif.rs all have test coverage). Consider adding tests for load/save operations, error cases, and edge conditions like invalid JSON or permission issues.
| } | |
| } | |
| #[cfg(test)] | |
| mod tests { | |
| use super::*; | |
| use std::fs; | |
| use std::io::Write; | |
| use std::path::PathBuf; | |
| fn create_temp_repo_root(test_name: &str) -> PathBuf { | |
| let mut dir = std::env::temp_dir(); | |
| let unique = format!( | |
| "argus_review_state_tests_{}_{}", | |
| test_name, | |
| std::process::id() | |
| ); | |
| dir.push(unique); | |
| if !dir.exists() { | |
| fs::create_dir_all(&dir).expect("failed to create temp repo root"); | |
| } | |
| dir | |
| } | |
| #[test] | |
| fn save_and_load_roundtrip() { | |
| let repo_root = create_temp_repo_root("roundtrip"); | |
| let original = ReviewState { | |
| last_reviewed_sha: "abc123".to_string(), | |
| timestamp: Utc::now(), | |
| }; | |
| // Save should succeed. | |
| original.save(&repo_root).expect("save failed"); | |
| // Load should return the same state. | |
| let loaded = ReviewState::load(&repo_root) | |
| .expect("load failed") | |
| .expect("expected Some(ReviewState), got None"); | |
| assert_eq!(original.last_reviewed_sha, loaded.last_reviewed_sha); | |
| // Timestamps may differ slightly if serialization has precision limits, | |
| // but for chrono + serde_json this should be exact. | |
| assert_eq!(original.timestamp, loaded.timestamp); | |
| } | |
| #[test] | |
| fn load_returns_none_when_state_missing() { | |
| let repo_root = create_temp_repo_root("missing_state"); | |
| // Ensure there is no .argus/review-state.json. | |
| let argus_dir = repo_root.join(".argus"); | |
| if argus_dir.exists() { | |
| fs::remove_dir_all(&argus_dir).expect("failed to clean .argus dir"); | |
| } | |
| let loaded = ReviewState::load(&repo_root).expect("load should not error"); | |
| assert!(loaded.is_none(), "expected None for missing state file"); | |
| } | |
| #[test] | |
| fn load_returns_error_on_invalid_json() { | |
| let repo_root = create_temp_repo_root("invalid_json"); | |
| let argus_dir = repo_root.join(".argus"); | |
| fs::create_dir_all(&argus_dir).expect("failed to create .argus dir"); | |
| let state_path = argus_dir.join("review-state.json"); | |
| let mut file = fs::File::create(&state_path).expect("failed to create state file"); | |
| // Write something that is not valid JSON. | |
| file.write_all(b"this is not json") | |
| .expect("failed to write invalid content"); | |
| drop(file); | |
| let result = ReviewState::load(&repo_root); | |
| assert!(result.is_err(), "expected error for invalid JSON"); | |
| } | |
| #[cfg(unix)] | |
| #[test] | |
| fn save_fails_on_permission_error_unix() { | |
| use std::os::unix::fs::PermissionsExt; | |
| let repo_root = create_temp_repo_root("permission_error_save"); | |
| let argus_dir = repo_root.join(".argus"); | |
| fs::create_dir_all(&argus_dir).expect("failed to create .argus dir"); | |
| // Remove write permissions from the .argus directory. | |
| let mut perms = fs::metadata(&argus_dir) | |
| .expect("metadata failed") | |
| .permissions(); | |
| perms.set_mode(0o555); // read and execute only | |
| fs::set_permissions(&argus_dir, perms).expect("set_permissions failed"); | |
| let state = ReviewState { | |
| last_reviewed_sha: "def456".to_string(), | |
| timestamp: Utc::now(), | |
| }; | |
| let result = state.save(&repo_root); | |
| assert!(result.is_err(), "expected error when directory is not writable"); | |
| } | |
| #[cfg(unix)] | |
| #[test] | |
| fn load_fails_on_permission_error_unix() { | |
| use std::os::unix::fs::PermissionsExt; | |
| let repo_root = create_temp_repo_root("permission_error_load"); | |
| let argus_dir = repo_root.join(".argus"); | |
| fs::create_dir_all(&argus_dir).expect("failed to create .argus dir"); | |
| let state_path = argus_dir.join("review-state.json"); | |
| fs::write(&state_path, b"{\"last_reviewed_sha\":\"xyz\",\"timestamp\":\"2020-01-01T00:00:00Z\"}") | |
| .expect("failed to write initial state file"); | |
| // Remove read permissions from the file. | |
| let mut perms = fs::metadata(&state_path) | |
| .expect("metadata failed") | |
| .permissions(); | |
| perms.set_mode(0o000); | |
| fs::set_permissions(&state_path, perms).expect("set_permissions failed"); | |
| let result = ReviewState::load(&repo_root); | |
| assert!(result.is_err(), "expected error when file is not readable"); | |
| } | |
| } |
feat: implement incremental review (--incremental)
Summary
Adds support for incremental code reviews. Use
argus review --incrementalto review only the changes made since the last review (or a specified base SHA). This feature makes iterative development loops faster and cleaner by hiding previously-reviewed issues unless they are touched again.Changes
--incremental: Review changes since the last saved review state.--base-sha <SHA>: Explicitly set the base commit for the review diff..argus/review-state.jsoninside the repo.git diff <base>..<HEAD>(orgit diff <base>for dirty trees).Usage
Testing
Summary by CodeRabbit
Release Notes
describesubcommand to generate PR titles, descriptions, and labels from diffs using AI.--incrementalflag and--base-shaoption to review only new changes since the last review with automatic state persistence.