feat: natural language review rules (#12) and optional skip deletion-only (#29) (#44)

- Config: review_rules_prose (YAML list), triage_skip_deletion_only (bool)
- Guidance: prose_rules_section() injects custom rules into review prompt
- Triage: TriageOptions.skip_deletion_only, SkipDeletionOnly result
- Pipeline: prepare_diff_analysis uses triage options from config
- ROADMAP: shipped section updated

Made-with: Cursor

Author: haasonsaas

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.

src/config.rs

@@ -269,6 +269,10 @@ pub struct Config {
     #[serde(default)]
     pub review_instructions: Option<String>,
 
+    /// Natural language review rules (one per item); injected into prompt as bullets (#12).
+    #[serde(default)]
+    pub review_rules_prose: Option<Vec<String>>,
+
     #[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<String, String>,
 
+    /// 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();

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"));
+    }
 }

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<String> {
     }
 }
 
+fn prose_rules_section(config: &config::Config) -> Option<String> {
+    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::<Vec<_>>()
+            .join("\n");
+        Some(format!("Custom rules (natural language):\n{bullets}"))
+    }
+}
+
 fn path_instructions_section(path_config: Option<&config::PathConfig>) -> Option<String> {
     let instructions = path_config?.review_instructions.as_deref()?.trim();
     if instructions.is_empty() {

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<PathBuf, PreAnalysis>,
+    triage_skip_deletion_only: bool,
 ) -> Result<DiffPreparationDecision> {
     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!(

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();
             }

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]

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",
         }
     }
 }

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;
     }