From f3d12ebf801f65374a2ff168098447349ac02c56 Mon Sep 17 00:00:00 2001 From: "Anaz S. Aji" Date: Thu, 16 Jul 2026 09:07:18 +0700 Subject: [PATCH] fix(review): suppress findings inside Markdown fenced code blocks (#329) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Markdown files (.md/.mdx/.markdown) frequently contain fenced code blocks that are documentation examples, not executable code. Treating their contents like real source code produced false positives — e.g. a `git push` inside a fenced bash block flagged as SQL injection (#329). New module `engine::markdown` detects fenced code blocks (triple-backtick / triple-tilde) within a diff chunk and exposes which new-file line numbers fall inside them. A new filter `apply_markdown_code_block_filter()` drops findings (from all sources: security/secrets/rules scanners + LLM) whose file:line lands inside a code block of a markdown file. Fence state is tracked across full hunk context (Add + Context lines, skipping Removed lines) so it works even when only the body of a code block was edited — the opening fence does not need to be part of the diff. Findings without a resolvable line number are kept (safe default). 11 new tests (7 unit for fence detection + 4 integration for the filter, including the exact #329 git-push/SQL-injection scenario). All 648 tests pass; clippy + fmt + audit clean. Refs #329 --- CHANGELOG.md | 4 + docs/changelog.md | 4 + src/engine/markdown.rs | 208 +++++++++++++++++++++++++++++++++++ src/engine/mod.rs | 1 + src/engine/review.rs | 241 +++++++++++++++++++++++++++++++++++++++++ 5 files changed, 458 insertions(+) create mode 100644 src/engine/markdown.rs diff --git a/CHANGELOG.md b/CHANGELOG.md index 7c0e9e8..4e451d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Markdown False Positives (#329) + +- **Findings inside Markdown fenced code blocks are now suppressed.** Code blocks (\`\`\` / `~~~`) in `.md`/`.mdx`/`.markdown` files are documentation examples, not executable code. A `git push` inside a ` ```bash ` block is no longer flagged as SQL injection. The filter covers all finding sources (security/secrets/rules scanners + LLM) and uses full hunk context (Add + Context lines) to track fence state, so it works even when only the code-block body was edited. + ### Fixed — Config Validation & Best Practices (#334) - **Quality gate no longer fails when disabled (#58).** `evaluate()` forced `GateResult::Pass` when `enabled: false`; it now short-circuits before applying thresholds so a disabled gate never blocks a merge. diff --git a/docs/changelog.md b/docs/changelog.md index 081f804..d2115f2 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -7,6 +7,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Fixed — Markdown False Positives (#329) + +- **Findings inside Markdown fenced code blocks are now suppressed.** A `git push` inside a ` ```bash ` block in an `.md` file is no longer flagged as SQL injection. Covers all finding sources (security/secrets/rules + LLM); tracks fence state across full hunk context (Add + Context lines). + ### Fixed — Config Validation & Best Practices (#334) - **Quality gate no longer fails when disabled (#58).** `evaluate()` now forces `GateResult::Pass` when `enabled: false` instead of applying thresholds. diff --git a/src/engine/markdown.rs b/src/engine/markdown.rs new file mode 100644 index 0000000..1b5aa96 --- /dev/null +++ b/src/engine/markdown.rs @@ -0,0 +1,208 @@ +//! Markdown awareness for the review/scan pipeline. +//! +//! Markdown files (`.md`, `.mdx`, `.markdown`) frequently contain fenced code +//! blocks that are **documentation examples**, not executable code. Treating +//! their contents like real source code produces false positives — e.g. a +//! `git push` line inside a ` ```bash ` block flagged as SQL injection (#329). +//! +//! This module detects fenced code blocks (` ``` ` / `~~~`) within a diff chunk +//! and exposes which new-file line numbers fall inside them, so findings can be +//! filtered out. + +use std::collections::HashSet; + +use crate::engine::diff_parser::{DiffLineType, FileChunk}; + +/// True if the path looks like a Markdown documentation file. +pub fn is_markdown(path: &str) -> bool { + let lower = path.to_lowercase(); + lower.ends_with(".md") || lower.ends_with(".mdx") || lower.ends_with(".markdown") +} + +/// Compute the set of new-file line numbers that fall **inside** fenced code +/// blocks for a single diff chunk. +/// +/// Only lines present in the *new* file are considered (Add + Context). Removed +/// lines are skipped so they cannot corrupt fence state. The opening fence does +/// not need to be part of the diff itself — context lines carry it too, which +/// keeps this reliable when only the body of a code block was edited. +/// +/// Fence state is tracked across all hunks in the chunk in document order. This +/// is exact when a code block lives within contiguous lines and a best-effort +/// approximation when a fence spans non-contiguous hunks. +pub fn lines_inside_code_blocks(chunk: &FileChunk) -> HashSet { + let mut inside = HashSet::new(); + let mut in_fence = false; + + for hunk in &chunk.chunks { + for line in &hunk.lines { + // Removed lines don't exist in the new file — skip to keep fence + // state faithful to the post-change document. + if line.line_type == DiffLineType::Remove { + continue; + } + + let trimmed = line.content.trim_start(); + let is_fence = trimmed.starts_with("```") || trimmed.starts_with("~~~"); + if is_fence { + in_fence = !in_fence; + continue; + } + + if in_fence { + if let Some(ln) = line.new_line_no { + if ln > 0 { + inside.insert(ln); + } + } + } + } + } + + inside +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::engine::diff_parser::{DiffHunk, DiffLine}; + + fn chunk(file: &str, lines: &[(DiffLineType, &str, u32)]) -> FileChunk { + FileChunk { + old_path: None, + new_path: Some(file.to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: lines.len() as u32, + header: String::new(), + lines: lines + .iter() + .map(|(lt, content, ln)| DiffLine { + line_type: *lt, + content: content.to_string(), + old_line_no: None, + new_line_no: Some(*ln), + }) + .collect(), + }], + is_binary: false, + is_deleted: false, + is_new: false, + } + } + + #[test] + fn detects_markdown_extensions() { + assert!(is_markdown("README.md")); + assert!(is_markdown("docs/guide.MDX")); + assert!(is_markdown("notes.markdown")); + assert!(!is_markdown("src/main.rs")); + assert!(!is_markdown("config.yaml")); + } + + #[test] + fn lines_inside_backtick_fence() { + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "Intro", 1), + (DiffLineType::Add, "```bash", 2), + (DiffLineType::Add, "git push origin vX.Y.Z", 3), + (DiffLineType::Add, "git tag vX.Y.Z", 4), + (DiffLineType::Add, "```", 5), + (DiffLineType::Add, "Done.", 6), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&3)); + assert!(inside.contains(&4)); + assert!(!inside.contains(&1)); + assert!(!inside.contains(&6)); + // Fence lines themselves are not "inside". + assert!(!inside.contains(&2)); + assert!(!inside.contains(&5)); + } + + #[test] + fn lines_inside_tilde_fence() { + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "~~~sql", 1), + (DiffLineType::Add, "SELECT 1;", 2), + (DiffLineType::Add, "~~~", 3), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&2)); + } + + #[test] + fn no_fence_yields_empty() { + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "Just prose.", 1), + (DiffLineType::Add, "No code here.", 2), + ], + ); + assert!(lines_inside_code_blocks(&c).is_empty()); + } + + #[test] + fn context_line_fence_covers_added_body() { + // Only the body line is added; the fences are pre-existing context. + let c = chunk( + "doc.md", + &[ + (DiffLineType::Context, "```bash", 2), + (DiffLineType::Add, "git push origin vX.Y.Z", 3), + (DiffLineType::Context, "```", 4), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!( + inside.contains(&3), + "added body inside context fence must be detected" + ); + } + + #[test] + fn removed_lines_do_not_corrupt_fence_state() { + // A removed closing fence must not leave the tracker stuck "inside". + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, "```bash", 1), + (DiffLineType::Remove, "old line", 0), // removed; old file only + (DiffLineType::Add, "git push", 2), + (DiffLineType::Add, "```", 3), + (DiffLineType::Add, "prose after", 4), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&2)); + assert!( + !inside.contains(&4), + "prose after a closed fence must not be flagged" + ); + } + + #[test] + fn indented_fence_marker_works() { + // Some templates indent fences; the marker is detected after trim. + let c = chunk( + "doc.md", + &[ + (DiffLineType::Add, " ```", 1), + (DiffLineType::Add, " code", 2), + (DiffLineType::Add, " ```", 3), + ], + ); + let inside = lines_inside_code_blocks(&c); + assert!(inside.contains(&2)); + } +} diff --git a/src/engine/mod.rs b/src/engine/mod.rs index 1efd6b5..5f8aa38 100644 --- a/src/engine/mod.rs +++ b/src/engine/mod.rs @@ -6,6 +6,7 @@ pub mod debt_tracker; pub mod diff_parser; pub mod language_analyzer; pub mod llm; +pub mod markdown; pub mod memory; pub mod profiles; pub mod quality_gate; diff --git a/src/engine/review.rs b/src/engine/review.rs index 623bd6d..8820ec0 100644 --- a/src/engine/review.rs +++ b/src/engine/review.rs @@ -292,6 +292,7 @@ async fn review_diff_inner( tokens_used: None, should_block: false, }; + fallback.issues = apply_markdown_code_block_filter(fallback.issues, &diff_chunks); fallback.issues = apply_ignore_rules(fallback.issues, &config.ignore.rules); let min_sev = config.hook.min_severity_level(); fallback.should_block = fallback @@ -342,6 +343,11 @@ async fn review_diff_inner( // sec-hardcoded-secret regex. response.issues = apply_llm_secret_fp_filter(response.issues, &diff_chunks); + // Drop findings inside Markdown fenced code blocks (#329). Code blocks in + // `.md` files are documentation examples, not executable code — a `git push` + // inside a ```bash block is not SQL injection. + response.issues = apply_markdown_code_block_filter(response.issues, &diff_chunks); + // Apply ignore rules: filter out issues matching ignored patterns response.issues = apply_ignore_rules(response.issues, &config.ignore.rules); @@ -477,6 +483,67 @@ fn apply_llm_secret_fp_filter( issues } +/// Drop findings located inside Markdown fenced code blocks (#329). +/// +/// Code blocks in `.md`/`.mdx`/`.markdown` files are documentation examples, +/// not executable code — e.g. a `git push` inside a ```bash block must not be +/// flagged as SQL injection. Findings without a resolvable line, or in files +/// without any code block, are kept unchanged (safe default). +fn apply_markdown_code_block_filter( + mut issues: Vec, + diff_chunks: &[crate::engine::diff_parser::FileChunk], +) -> Vec { + use crate::engine::markdown::{is_markdown, lines_inside_code_blocks}; + use std::collections::HashSet; + + // Build file path -> set of code-block line numbers, for markdown files only. + let mut code_block_lines: std::collections::HashMap> = + std::collections::HashMap::new(); + for chunk in diff_chunks { + let path = chunk + .new_path + .as_deref() + .or(chunk.old_path.as_deref()) + .unwrap_or(""); + if !is_markdown(path) { + continue; + } + let set = lines_inside_code_blocks(chunk); + if !set.is_empty() { + code_block_lines + .entry(path.to_string()) + .or_default() + .extend(set); + } + } + + if code_block_lines.is_empty() { + return issues; // no markdown code blocks in this diff — fast path + } + + let before = issues.len(); + issues.retain(|issue| { + let Some(ln) = issue.line else { + return true; // keep findings without a concrete line number + }; + match code_block_lines.get(&issue.file) { + Some(lines) => !lines.contains(&ln), // drop if inside a code block + None => true, + } + }); + + let dropped = before - issues.len(); + if dropped > 0 { + debug!( + dropped, + remaining = issues.len(), + "removed markdown code-block false positives" + ); + } + + issues +} + /// Filter out issues whose `issue_type` matches any ignored rule pattern. fn apply_ignore_rules(mut issues: Vec, ignore_rules: &[String]) -> Vec { if ignore_rules.is_empty() { @@ -789,4 +856,178 @@ mod tests { let result = apply_ignore_rules(issues, &rules); assert!(result.is_empty()); } + + // ─── #329: markdown fenced code-block false positives ─── + + #[test] + fn markdown_fp_filter_drops_finding_inside_code_block() { + use crate::engine::diff_parser::*; + + // The exact #329 scenario: a `git push` inside a ```bash block in a + // markdown doc, flagged as SQL injection. + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("AGENT.md".to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 4, + header: String::new(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: "```bash".to_string(), + old_line_no: None, + new_line_no: Some(167), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "git push origin vX.Y.Z".to_string(), + old_line_no: None, + new_line_no: Some(168), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "```".to_string(), + old_line_no: None, + new_line_no: Some(169), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "AGENT.md".to_string(), + line: Some(168), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "SQL injection via string concatenation".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert!( + result.is_empty(), + "finding inside a markdown code block must be dropped" + ); + } + + #[test] + fn markdown_fp_filter_keeps_finding_outside_code_block() { + use crate::engine::diff_parser::*; + + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("doc.md".to_string()), + language: "markdown".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 3, + header: String::new(), + lines: vec![ + DiffLine { + line_type: DiffLineType::Add, + content: "```bash".to_string(), + old_line_no: None, + new_line_no: Some(1), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "echo hi".to_string(), + old_line_no: None, + new_line_no: Some(2), + }, + DiffLine { + line_type: DiffLineType::Add, + content: "```".to_string(), + old_line_no: None, + new_line_no: Some(3), + }, + ], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + // Finding on line 5 (outside the block, in prose) must survive. + let issues = vec![ReviewIssue { + file: "doc.md".to_string(), + line: Some(5), + severity: Severity::Minor, + issue_type: Some("style".to_string()), + title: "typo".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "finding outside a code block must be kept"); + } + + #[test] + fn markdown_fp_filter_keeps_findings_in_non_markdown_files() { + use crate::engine::diff_parser::*; + + // A real .py file is never treated as markdown, even if it has ``` text. + let diff_chunks = vec![FileChunk { + old_path: None, + new_path: Some("src/app.py".to_string()), + language: "python".to_string(), + chunks: vec![DiffHunk { + old_start: 1, + old_count: 1, + new_start: 1, + new_count: 2, + header: String::new(), + lines: vec![DiffLine { + line_type: DiffLineType::Add, + content: "eval(request.body.code)".to_string(), + old_line_no: None, + new_line_no: Some(42), + }], + }], + is_binary: false, + is_deleted: false, + is_new: false, + }]; + + let issues = vec![ReviewIssue { + file: "src/app.py".to_string(), + line: Some(42), + severity: Severity::Critical, + issue_type: Some("security".to_string()), + title: "eval injection".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &diff_chunks); + assert_eq!(result.len(), 1, "non-markdown files are unaffected"); + } + + #[test] + fn markdown_fp_filter_keeps_findings_without_line_number() { + // Findings with no resolvable line are kept (safe default). + let issues = vec![ReviewIssue { + file: "doc.md".to_string(), + line: None, + severity: Severity::Info, + issue_type: None, + title: "vague".to_string(), + body: "...".to_string(), + suggested_fix: None, + }]; + + let result = apply_markdown_code_block_filter(issues, &[]); + assert_eq!(result.len(), 1); + } }