diff --git a/docs/ROADMAP.md b/docs/ROADMAP.md index 8f94b35..bb676a4 100644 --- a/docs/ROADMAP.md +++ b/docs/ROADMAP.md @@ -72,6 +72,8 @@ Create labels once: `priority: high`, `priority: medium`, `priority: low`, `area ## Shipped (recent) +- **Natural language rules (#12):** `review_rules_prose: [ "Rule one", "Rule two" ]` in config; injected as "Custom rules (natural language)" bullets into review guidance. Tests: `test_config_deserialize_review_rules_prose_from_yaml`, `build_review_guidance_includes_prose_rules`. +- **Triage skip deletion-only (#29):** `triage_skip_deletion_only: true` in config; when true, deletion-only diffs get `SkipDeletionOnly` and skip expensive review. Default false. Tests: `test_triage_deletion_only_with_skip_true_returns_skip_deletion_only`, config deserialize. - **LLM parsing (#28):** Repair candidate for diff-style line prefixes (`+` on each line) in `repair_json_candidates`; test `parse_json_with_diff_prefix_artifact`. - **Secrets (#20):** Built-in secret scanner in `plugins/builtin/secret_scanner.rs`. - **Verification (#23):** Verification pass and config (verification.*) in pipeline. diff --git a/src/config.rs b/src/config.rs index 3980d94..f49c792 100644 --- a/src/config.rs +++ b/src/config.rs @@ -269,6 +269,10 @@ pub struct Config { #[serde(default)] pub review_instructions: Option, + /// Natural language review rules (one per item); injected into prompt as bullets (#12). + #[serde(default)] + pub review_rules_prose: Option>, + #[serde(default = "default_true")] pub smart_review_summary: bool, @@ -302,6 +306,10 @@ pub struct Config { #[serde(default = "default_symbol_index_lsp_languages")] pub symbol_index_lsp_languages: HashMap, + /// When true, triage skips deletion-only diffs (#29). Default false (deletions get review). + #[serde(default)] + pub triage_skip_deletion_only: bool, + #[serde(default = "default_feedback_path")] pub feedback_path: PathBuf, @@ -557,6 +565,7 @@ impl Default for Config { comment_types: default_comment_types(), review_profile: None, review_instructions: None, + review_rules_prose: None, smart_review_summary: true, smart_review_diagram: false, symbol_index: true, @@ -568,6 +577,7 @@ impl Default for Config { symbol_index_graph_max_files: default_symbol_index_graph_max_files(), symbol_index_lsp_command: None, symbol_index_lsp_languages: default_symbol_index_lsp_languages(), + triage_skip_deletion_only: false, feedback_path: default_feedback_path(), eval_trend_path: default_eval_trend_path(), feedback_eval_trend_path: default_feedback_eval_trend_path(), @@ -2147,6 +2157,33 @@ verification_consensus_mode: all ); } + #[test] + fn test_config_deserialize_review_rules_prose_from_yaml() { + // #12: natural language rules — list of strings in YAML + let yaml = r#" +model: claude-sonnet-4-6 +review_rules_prose: + - Always use parameterized queries. + - No direct SQL string concatenation. +"#; + let config: Config = serde_yaml::from_str(yaml).unwrap(); + let rules = config.review_rules_prose.as_ref().unwrap(); + assert_eq!(rules.len(), 2); + assert!(rules[0].contains("parameterized")); + assert!(rules[1].contains("SQL")); + } + + #[test] + fn test_config_deserialize_triage_skip_deletion_only() { + // #29: optional skip deletion-only diffs + let yaml = r#" +model: claude-sonnet-4-6 +triage_skip_deletion_only: true +"#; + let config: Config = serde_yaml::from_str(yaml).unwrap(); + assert!(config.triage_skip_deletion_only); + } + #[test] fn test_default_frontier_role_models_match_requested_pair() { let config = Config::default(); diff --git a/src/review/pipeline/guidance.rs b/src/review/pipeline/guidance.rs index 0c51029..98faff1 100644 --- a/src/review/pipeline/guidance.rs +++ b/src/review/pipeline/guidance.rs @@ -96,4 +96,23 @@ mod tests { let guidance = build_review_guidance(&config, None).unwrap(); assert!(guidance.contains("Do not include code fix suggestions")); } + + #[test] + fn build_review_guidance_includes_prose_rules() { + // #12: natural language custom rules — injected as bullets into guidance + let config = config::Config { + review_rules_prose: Some(vec![ + "Always use parameterized queries.".to_string(), + "No direct SQL string concatenation.".to_string(), + ]), + ..config::Config::default() + }; + let guidance = build_review_guidance(&config, None).unwrap(); + assert!( + guidance.contains("Custom rules (natural language)"), + "guidance should include prose rules section" + ); + assert!(guidance.contains("Always use parameterized queries")); + assert!(guidance.contains("No direct SQL string concatenation")); + } } diff --git a/src/review/pipeline/guidance/sections.rs b/src/review/pipeline/guidance/sections.rs index 903b2a8..61d984d 100644 --- a/src/review/pipeline/guidance/sections.rs +++ b/src/review/pipeline/guidance/sections.rs @@ -9,6 +9,7 @@ pub(super) fn collect_guidance_sections( push_section(&mut sections, comment_types_section(config)); push_section(&mut sections, review_profile_section(config)); push_section(&mut sections, global_instructions_section(config)); + push_section(&mut sections, prose_rules_section(config)); push_section(&mut sections, path_instructions_section(path_config)); push_section(&mut sections, output_language_section(config)); sections.push(fix_suggestion_section(config)); @@ -66,6 +67,25 @@ fn global_instructions_section(config: &config::Config) -> Option { } } +fn prose_rules_section(config: &config::Config) -> Option { + let rules = config.review_rules_prose.as_deref()?; + let rules: Vec<&str> = rules + .iter() + .map(String::as_str) + .filter(|s| !s.trim().is_empty()) + .collect(); + if rules.is_empty() { + None + } else { + let bullets = rules + .iter() + .map(|r| format!("- {}", r.trim())) + .collect::>() + .join("\n"); + Some(format!("Custom rules (natural language):\n{bullets}")) + } +} + fn path_instructions_section(path_config: Option<&config::PathConfig>) -> Option { let instructions = path_config?.review_instructions.as_deref()?.trim(); if instructions.is_empty() { diff --git a/src/review/pipeline/prepare/runner/analysis.rs b/src/review/pipeline/prepare/runner/analysis.rs index e88792e..a76f821 100644 --- a/src/review/pipeline/prepare/runner/analysis.rs +++ b/src/review/pipeline/prepare/runner/analysis.rs @@ -4,7 +4,7 @@ use std::path::PathBuf; use crate::core; use crate::plugins::PreAnalysis; -use crate::review::triage::triage_diff; +use crate::review::triage::{triage_diff_with_options, TriageOptions}; use super::super::super::comments::{filter_comments_for_diff, synthesize_analyzer_comments}; @@ -22,6 +22,7 @@ pub(super) struct PreparedDiffAnalysis { pub(super) fn prepare_diff_analysis( diff: &core::UnifiedDiff, batched_pre_analysis: &mut HashMap, + triage_skip_deletion_only: bool, ) -> Result { let pre_analysis = batched_pre_analysis .remove(&diff.file_path) @@ -31,7 +32,12 @@ pub(super) fn prepare_diff_analysis( synthesize_analyzer_comments(pre_analysis.findings.clone())?, ); - let triage_result = triage_diff(diff); + let triage_result = triage_diff_with_options( + diff, + TriageOptions { + skip_deletion_only: triage_skip_deletion_only, + }, + ); if triage_result.should_skip() { if deterministic_comments.is_empty() { tracing::info!( diff --git a/src/review/pipeline/prepare/runner/run.rs b/src/review/pipeline/prepare/runner/run.rs index 22099b7..588a6ee 100644 --- a/src/review/pipeline/prepare/runner/run.rs +++ b/src/review/pipeline/prepare/runner/run.rs @@ -29,7 +29,11 @@ pub(in super::super::super) async fn prepare_file_review_jobs( continue; } - match prepare_diff_analysis(&diff, &mut batched_pre_analysis)? { + match prepare_diff_analysis( + &diff, + &mut batched_pre_analysis, + services.config.triage_skip_deletion_only, + )? { DiffPreparationDecision::Skip => { progress.skip_file(); } diff --git a/src/review/triage.rs b/src/review/triage.rs index 061e733..57a1514 100644 --- a/src/review/triage.rs +++ b/src/review/triage.rs @@ -11,7 +11,8 @@ mod run; #[allow(unused_imports)] pub use result::TriageResult; -pub use run::triage_diff; +#[allow(unused_imports)] +pub use run::{triage_diff, triage_diff_with_options, TriageOptions}; #[cfg(test)] mod tests { @@ -657,6 +658,7 @@ mod tests { assert!(TriageResult::SkipWhitespaceOnly.should_skip()); assert!(TriageResult::SkipGenerated.should_skip()); assert!(TriageResult::SkipCommentOnly.should_skip()); + assert!(TriageResult::SkipDeletionOnly.should_skip()); } #[test] @@ -671,6 +673,7 @@ mod tests { assert!(!TriageResult::SkipWhitespaceOnly.reason().is_empty()); assert!(!TriageResult::SkipGenerated.reason().is_empty()); assert!(!TriageResult::SkipCommentOnly.reason().is_empty()); + assert!(!TriageResult::SkipDeletionOnly.reason().is_empty()); } #[test] @@ -681,6 +684,7 @@ mod tests { TriageResult::SkipWhitespaceOnly.reason(), TriageResult::SkipGenerated.reason(), TriageResult::SkipCommentOnly.reason(), + TriageResult::SkipDeletionOnly.reason(), ]; // All should be unique let unique: std::collections::HashSet<&str> = reasons.iter().copied().collect(); @@ -691,6 +695,42 @@ mod tests { ); } + // ── #29 optional skip deletion-only ────────────────────────────────── + + #[test] + fn test_triage_deletion_only_with_skip_true_returns_skip_deletion_only() { + let diff = make_diff( + "src/lib.rs", + vec![ + make_line(1, ChangeType::Removed, "let x = 1;"), + make_line(2, ChangeType::Removed, "let y = 2;"), + ], + ); + let options = TriageOptions { + skip_deletion_only: true, + }; + assert_eq!( + triage_diff_with_options(&diff, options), + TriageResult::SkipDeletionOnly + ); + } + + #[test] + fn test_triage_deletion_only_with_skip_false_returns_needs_review() { + let diff = make_diff( + "src/lib.rs", + vec![make_line(1, ChangeType::Removed, "removed line")], + ); + assert_eq!(triage_diff(&diff), TriageResult::NeedsReview); + let options = TriageOptions { + skip_deletion_only: false, + }; + assert_eq!( + triage_diff_with_options(&diff, options), + TriageResult::NeedsReview + ); + } + // ── Adversarial edge cases ────────────────────────────────────────── #[test] diff --git a/src/review/triage/result.rs b/src/review/triage/result.rs index 73b8221..88534bd 100644 --- a/src/review/triage/result.rs +++ b/src/review/triage/result.rs @@ -5,6 +5,8 @@ pub enum TriageResult { SkipWhitespaceOnly, SkipGenerated, SkipCommentOnly, + /// File has only removal hunks; skip when config triage_skip_deletion_only is true (#29). + SkipDeletionOnly, } impl TriageResult { @@ -19,6 +21,7 @@ impl TriageResult { TriageResult::SkipWhitespaceOnly => "whitespace-only changes", TriageResult::SkipGenerated => "generated file", TriageResult::SkipCommentOnly => "comment-only changes", + TriageResult::SkipDeletionOnly => "deletion-only changes", } } } diff --git a/src/review/triage/run.rs b/src/review/triage/run.rs index dc94d15..fc4b234 100644 --- a/src/review/triage/run.rs +++ b/src/review/triage/run.rs @@ -7,7 +7,20 @@ use super::comments::is_comment_line; use super::files::{is_generated_file, is_lock_file}; use super::result::TriageResult; +/// Options for triage behavior (e.g. from config). +#[derive(Debug, Clone, Copy, Default)] +pub struct TriageOptions { + /// When true, treat deletion-only diffs as skip (#29). Default false (deletions still get review). + pub skip_deletion_only: bool, +} + +/// Convenience: triage with default options (skip_deletion_only = false). Used by tests and callers that do not need config. +#[allow(dead_code)] pub fn triage_diff(diff: &UnifiedDiff) -> TriageResult { + triage_diff_with_options(diff, TriageOptions::default()) +} + +pub fn triage_diff_with_options(diff: &UnifiedDiff, options: TriageOptions) -> TriageResult { if is_lock_file(&diff.file_path) { return TriageResult::SkipLockFile; } @@ -34,6 +47,9 @@ pub fn triage_diff(diff: &UnifiedDiff) -> TriageResult { } if is_deletion_only_change(&all_changes) { + if options.skip_deletion_only { + return TriageResult::SkipDeletionOnly; + } // Pure deletions can still remove required fields, checks, or error handling. return TriageResult::NeedsReview; }