From f301e7799cd7eb3e6d28a2f53887e60cb1ba0b62 Mon Sep 17 00:00:00 2001 From: ajianaz Date: Thu, 6 Aug 2026 10:15:07 +0700 Subject: [PATCH] fix: reduce false positives in SARIF review (#507) - Add context line filter: drop Minor/Info findings on unchanged diff lines (pre-existing code flagged as new issue) - Enhanced system prompt: in-place mutation awareness (Vec::retain etc.) and error path vs happy path distinction - Import Severity type for filter comparison --- src/engine/llm.rs | 5 +++ src/engine/review.rs | 76 +++++++++++++++++++++++++++++++++++++++++++- 2 files changed, 80 insertions(+), 1 deletion(-) diff --git a/src/engine/llm.rs b/src/engine/llm.rs index 74af42b..56d8525 100644 --- a/src/engine/llm.rs +++ b/src/engine/llm.rs @@ -198,6 +198,11 @@ CRITICAL CONSTRAINTS: When in doubt, downgrade severity rather than omitting — a borderline concern is a valid minor/info finding. 4. Common patterns to always check: unvalidated inputs, missing error handling, resource leaks, race conditions, off-by-one errors, unchecked edge cases. +LANGUAGE-SPECIFIC FALSE POSITIVE AWARENESS: +- In Rust, `Vec::retain()`, `Vec::append()`, `Vec::retain_mut()`, `Vec::splice()`, `Vec::dedup()`, `Vec::sort()`, `Vec::sort_by()` mutate the vector IN-PLACE. Do NOT flag code as "missing assignment" or "result ignored" when these methods are called — the mutation is the intended side effect. +- In Rust, `Err` arms that return early (e.g. `Err(e) => return error_response(...)`) are ERROR HANDLING paths. Do NOT flag them for missing post-conditions (like "filter not applied") — no data flows through error paths. +- In general, distinguish happy paths from error/early-return paths. Post-conditions (filters, transformations, validations) only need to hold on the happy path, not on every match arm. + SEVERITY LEVELS: - "critical": Security vulnerabilities, crashes, data loss, breaking bugs - "major": Bugs that affect functionality, logic errors, missing error handling, significant problems diff --git a/src/engine/review.rs b/src/engine/review.rs index cb83f7f..5121ff9 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -3,7 +3,7 @@ use tracing::{debug, instrument}; use crate::config::schema::Config; use crate::engine::llm; -use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse}; +use crate::engine::types::{LLMConfig, ReviewIssue, ReviewResponse, Severity}; /// Load a custom system prompt from a file path. /// Returns the file content, or None if the file doesn't exist, can't be read, @@ -433,6 +433,11 @@ async fn review_diff_inner( // Apply ignore rules: filter out issues matching ignored patterns response.issues = apply_ignore_rules(response.issues, &config.ignore.rules); + // Drop low-severity findings on unchanged (context) lines — these are + // pre-existing code that appeared in the diff due to surrounding changes, + // not new code introduced by the PR (#507 Pattern #3). + response.issues = apply_context_line_filter(response.issues, &diff_chunks); + // Calculate should_block based on min_severity let min_severity = config.hook.min_severity_level(); // Ord order is Critical(0) < Major(1) < Minor(2) < Info(3), so "at or above @@ -656,6 +661,75 @@ fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> issues } +/// Drop findings on unchanged (context) or removed lines (#507 Pattern #3). +/// +/// The LLM sometimes flags pre-existing code that appears in the diff purely +/// because surrounding lines changed. These findings are not about code the PR +/// introduces — they are noise. +/// +/// **Policy:** Only drop `Minor` and `Info` severity findings on context/removed +/// lines. `Critical` and `Major` findings are kept regardless, because they may +/// represent real risks worth surfacing even in pre-existing code. +fn apply_context_line_filter( + mut issues: Vec, + diff_chunks: &[crate::engine::diff_parser::FileChunk], +) -> Vec { + use crate::engine::diff_parser::DiffLineType; + + // Build lookup: (file, new_line_no) -> is_added + // Only includes lines present in the diff (Add or Context). Lines not in + // the diff at all are left alone (LLM line numbers can be imprecise). + let mut line_kinds: std::collections::HashMap<(String, u32), DiffLineType> = + std::collections::HashMap::new(); + for chunk in diff_chunks { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""); + for hunk in &chunk.chunks { + for line in &hunk.lines { + if let Some(ln) = line.new_line_no { + line_kinds.insert((path.to_string(), ln), line.line_type); + } + } + } + } + + let before = issues.len(); + issues.retain(|issue| { + // Keep findings without a concrete line number + let Some(ln) = issue.line else { + return true; + }; + + // Only filter if we can resolve this (file, line) to a diff line + let Some(kind) = line_kinds.get(&(issue.file.clone(), ln)) else { + return true; // not in diff — can't determine, keep + }; + + match kind { + DiffLineType::Add => true, // genuinely new code — always keep + DiffLineType::Context | DiffLineType::Remove => { + // Pre-existing code — only keep if severity is high enough + // Ord: Critical(0) < Major(1) < Minor(2) < Info(3) + issue.severity <= Severity::Major + } + } + }); + + let dropped = before - issues.len(); + if dropped > 0 { + debug!( + dropped, + remaining = issues.len(), + "removed low-severity findings on unchanged diff context lines (#507)" + ); + } + + issues +} + /// Check if a file path from an LLM issue matches any of the valid diff file paths. /// Uses exact match only — the LLM should report paths exactly as they appear in the diff. fn is_valid_file_path(issue_file: &str, valid_files: &[String]) -> bool {