Skip to content

fix(review): suppress findings inside Markdown fenced code blocks (#329)#343

Merged
ajianaz merged 1 commit into
developfrom
fix/markdown-code-block-fp
Jul 16, 2026
Merged

fix(review): suppress findings inside Markdown fenced code blocks (#329)#343
ajianaz merged 1 commit into
developfrom
fix/markdown-code-block-fp

Conversation

@ajianaz

@ajianaz ajianaz commented Jul 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixes the root cause of #329: Markdown fenced code blocks (documentation examples) were treated like executable code, producing false positives — e.g. a git push inside a ```bash block flagged as SQL injection.

Approach

Issue #329 offered three options (skip code blocks / reduce severity / language-hint matching). Skipping code blocks entirely subsumes language-hint matching for the common case and is the cleanest, so that's what this implements.

New module engine::markdown:

  • is_markdown(path) — detects .md / .mdx / .markdown
  • lines_inside_code_blocks(chunk) — returns the set of new-file line numbers inside ``` / ~~~ fences. Tracks fence state 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 is often pre-existing context).

New filter apply_markdown_code_block_filter() in review.rs:

  • Applied at the single choke point where all findings (security/secrets/rules scanners + LLM) are unified as Vec<ReviewIssue> — right next to apply_ignore_rules, in both the success path and the LLM-failure fallback path.
  • Drops findings whose file is markdown and whose line falls inside a code block.
  • Safe defaults: findings with no resolvable line number are kept; non-markdown files are unaffected. Fast-paths out when the diff has no markdown code blocks.

Why this design

  • One filter covers all sources. Deterministic findings get merged into response.issues after the LLM call, so filtering at the unified ReviewIssue stage catches regex-scanner and LLM false positives in one place.
  • Hunk context is already available (DiffLineType::Context lines are parsed), so fence tracking is reliable without extra file I/O.
  • Skips Removed lines so deletions can't corrupt fence state (e.g. a removed closing fence won't leave the tracker stuck "inside").

Scope

This addresses the systematic markdown code-block root cause (#329 item 1). The issue's other two observations (CHANGELOG future-date timezone; Cargo.lock format) are not regex-scanner artifacts — they come from LLM/rule paths and are noted for separate handling.

Testing

Refs #329

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
@ajianaz
ajianaz merged commit bd89814 into develop Jul 16, 2026
10 checks passed
@ajianaz
ajianaz deleted the fix/markdown-code-block-fp branch July 16, 2026 02:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant