feat: redundant match arm preprocessing

.hermes/conveyor/work-59dfbec8/initial_plan.md

@@ -0,0 +1,47 @@
+# Initial Plan: Remove redundant `Language::Json` match arms in preprocess.rs
+
+## Approach
+Remove the redundant `Language::Json` match arms in `comment_syntax()` and `string_syntax()` methods in `crates/diffguard-domain/src/preprocess.rs`.
+
+The fix consolidates language coverage by letting the catch-all `_ =>` handle what was being redundantly matched explicitly.
+
+### Step 1: Fix `comment_syntax()` (line 83)
+Remove the redundant arm:
+```rust
+// REMOVE this line:
+Language::Json => CommentSyntax::CStyle,
+```
+The catch-all `_ => CommentSyntax::CStyle` already covers `Json`.
+
+### Step 2: Fix `string_syntax()` (line 108)
+Change the grouped arm to exclude `Json`:
+```rust
+// BEFORE:
+Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle,
+
+// AFTER:
+Language::Yaml | Language::Toml => StringSyntax::CStyle,
+```
+`Yaml` and `Toml` must remain explicit because they are NOT covered by the catch-all `_ => StringSyntax::CStyle` (which only covers the remaining languages: C, Cpp, CSharp, Java, Kotlin, JavaScript, TypeScript, Go, Ruby, Unknown).
+
+## Risks
+
+1. **Rustc warning vs error**: Removing the arm may change a warning into an error in strict builds or with `#[deny(dead_code))]` or `#[deny(unreachable_patterns)]` — verify the crate compiles cleanly after changes.
+
+2. **Mutation tests**: The `mutants.out.old` files suggest mutation testing was run. Removing dead code patterns may trigger different mutation coverage or new warnings from rustc.
+
+3. **Behavioral documentation**: The explicit `Language::Json` arm served as documentation that JSON intentionally uses C-style syntax even though JSON technically has no comments in its base spec. The comment `// JSON supports comments in jsonc/json5 dialects` on line 82 explains the intent. Removing the arm loses this documentation signal within the match.
+
+## Task Breakdown
+
+1. **Edit `comment_syntax()`**: Remove `Language::Json => CommentSyntax::CStyle,` at line 83
+2. **Edit `string_syntax()`**: Change `Language::Yaml | Language::Toml | Language::Json` to `Language::Yaml | Language::Toml` at line 108
+3. **Verify compilation**: `cargo build -p diffguard-domain` to ensure no new errors/warnings
+4. **Run tests**: `cargo test -p diffguard-domain` to confirm existing tests still pass
+5. **Optional**: Add a `#[allow(dead_code)]` or clarify the comment if the explicit `Json` arm was intentionally documented behavior
+
+## Verification Checklist
+- [ ] `cargo build -p diffguard-domain` succeeds with no new warnings
+- [ ] `cargo test -p diffguard-domain` passes
+- [ ] `cargo clippy -p diffguard-domain` passes (if applicable)
+- [ ] Mutation tests unchanged (existing test coverage for `Language::Json` remains)

.hermes/conveyor/work-59dfbec8/research_analysis.md

@@ -0,0 +1,83 @@
+# Research Analysis: redundant match arm in preprocess.rs
+
+## Issue Summary
+**Issue**: GitHub issue #136 — "preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard"
+
+In `crates/diffguard-domain/src/preprocess.rs`, the `Language` enum match arms in `comment_syntax()` and `string_syntax()` methods have redundant arms where `Language::Json` is explicitly matched but then also caught by the catch-all `_ =>` wildcard pattern.
+
+## Issue Details
+
+### Location 1: `comment_syntax()` (lines 69–86)
+
+```rust
+pub fn comment_syntax(self) -> CommentSyntax {
+    match self {
+        Language::Python | Language::Ruby | Language::Shell => CommentSyntax::Hash,
+        Language::Rust | Language::Swift | Language::Scala => CommentSyntax::CStyleNested,
+        Language::Sql => CommentSyntax::Sql,
+        Language::Xml => CommentSyntax::Xml,
+        Language::Php => CommentSyntax::Php,
+        Language::Yaml | Language::Toml => CommentSyntax::Hash,
+        Language::Json => CommentSyntax::CStyle,  // ← REDUNDANT
+        _ => CommentSyntax::CStyle,
+    }
+}
+```
+
+### Location 2: `string_syntax()` (lines 89–111)
+
+```rust
+pub fn string_syntax(self) -> CommentSyntax {
+    match self {
+        Language::Rust => StringSyntax::Rust,
+        Language::Python => StringSyntax::Python,
+        Language::JavaScript | Language::TypeScript | Language::Ruby => StringSyntax::JavaScript,
+        Language::Go => StringSyntax::Go,
+        Language::Shell => StringSyntax::Shell,
+        Language::Swift | Language::Scala => StringSyntax::SwiftScala,
+        Language::Sql => StringSyntax::Sql,
+        Language::Xml => StringSyntax::Xml,
+        Language::Php => StringSyntax::Php,
+        Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle,  // ← GROUP PARTIALLY REDUNDANT
+        _ => StringSyntax::CStyle,
+    }
+}
+```
+
+In `string_syntax()`, the line `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle` is partially redundant because the catch-all `_ => StringSyntax::CStyle` already covers `Language::Json`. However, keeping `YAML` and `TOML` explicitly listed may be intentional for documentation purposes since they are not covered by the catch-all (the catch-all covers C, Cpp, CSharp, Java, Kotlin, JavaScript, TypeScript, Go, Ruby, and Unknown).
+
+## Relevant Codebase Areas
+
+### File: `crates/diffguard-domain/src/preprocess.rs`
+- **Lines 9–31**: `Language` enum definition with 21 variants (Rust, Python, JavaScript, TypeScript, Go, Ruby, C, Cpp, CSharp, Java, Kotlin, Shell, Swift, Scala, Sql, Xml, Php, Yaml, Toml, Json, Unknown)
+- **Lines 69–86**: `comment_syntax()` method — `Language::Json` arm (line 83) is fully redundant
+- **Lines 89–111**: `string_syntax()` method — `Language::Json` within the group arm (line 108) is redundant (caught by catch-all)
+
+### Other Files
+- `fuzz/fuzz_targets/preprocess.rs` — fuzzing target for the preprocessor
+- `crates/diffguard-domain/src/rules.rs` — uses `Language` for rule matching
+- `crates/diffguard-domain/src/preprocess.rs` tests (lines ~1015, ~1039, ~1083, ~2647)
+
+## Dependencies and Constraints
+- **No I/O constraint**: diffguard-domain must not use `std::process`, `std::fs`, or `std::env`
+- **Pure functions required**: All logic testable without mocks
+- **Best-effort preprocessing**: Uses C-like syntax heuristics, not full language parsers
+- **Mutation testing**: The crate runs `cargo mutants` for mutation testing
+
+## Key Findings
+
+1. **`comment_syntax()` redundancy**: The `Language::Json => CommentSyntax::CStyle` arm at line 83 is fully redundant — it produces the same output (`CStyle`) as the catch-all `_ => CommentSyntax::CStyle` at line 84.
+
+2. **`string_syntax()` partial redundancy**: In the arm `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle`, only `Json` is redundant since the catch-all already returns `CStyle` for `Json`. `Yaml` and `Toml` are NOT covered by the catch-all.
+
+3. **Rustc warning**: This generates E0017 (redundant match arm) or similar warning — the arms are unreachable or provably redundant.
+
+4. **Existing test coverage**: Tests exist for `Language::Json.comment_syntax()` (line 1083) and `jsonc_double_slash_comment_ignored` (line 2647), confirming `Json` should return `CStyle`.
+
+## Recommended Fix
+- In `comment_syntax()`: Remove the `Language::Json => CommentSyntax::CStyle,` line (83)
+- In `string_syntax()`: Change `Language::Yaml | Language::Toml | Language::Json` to `Language::Yaml | Language::Toml` (removing `Json` from the group)
+
+## Risk Assessment
+- **Low risk**: The fix is a pure refactoring — no functional change since `Json` already returns `CStyle` via the catch-all
+- **Test impact**: Existing tests should continue to pass since behavior is unchanged

.hermes/plans/2026-04-11_001536-issue-136-plan.md

@@ -0,0 +1,55 @@
+# Plan: Fix Issue #136 — Remove Redundant `Language::Json` Match Arms
+
+## Goal
+
+Fix clippy warning: `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard` in `crates/diffguard-domain/src/preprocess.rs`.
+
+## Current Context
+
+- **Issue:** #136 — `preprocess.rs: redundant match arm — Language::Json is shadowed by wildcard`
+- **Location:** `crates/diffguard-domain/src/preprocess.rs`
+- **Branch:** `main` with uncommitted working change
+- **Build status:** Passing
+- **Tests:** Passing (0 failures)
+- **Related work item:** `work-59dfbec8` exists in conveyor with initial plan already written
+
+## Proposed Approach
+
+Remove the redundant `Language::Json` match arms in `comment_syntax()` and `string_syntax()` methods. The wildcard `_ =>` already covers `Language::Json`, so the explicit arms are dead code.
+
+**Changes:**
+
+1. `comment_syntax()` (line ~82-83): Remove `Language::Json => CommentSyntax::CStyle,` — covered by `_ =>`
+2. `string_syntax()` (line ~107): Change `Language::Yaml | Language::Toml | Language::Json => StringSyntax::CStyle,` → `Language::Yaml | Language::Toml => StringSyntax::CStyle,` — `Json` covered by `_ =>`
+
+Note: The working tree already shows this diff (1 insertion, 2 deletions). This plan verifies the correct approach and defines verification steps.
+
+## Step-by-Step Plan
+
+1. **Verify current diff** matches the intended fix
+2. **Run full workspace build** (`cargo build --workspace`) — ensure no new errors
+3. **Run tests** (`cargo test --workspace`) — confirm no regressions
+4. **Run clippy** (`cargo clippy --workspace --all-targets`) — verify warning is resolved
+5. **Commit** with message referencing issue #136
+
+## Files Likely to Change
+
+- `crates/diffguard-domain/src/preprocess.rs` (1 file, ~3 lines changed)
+
+## Tests / Validation
+
+- `cargo build -p diffguard-domain` — compiles cleanly
+- `cargo test -p diffguard-domain` — all tests pass
+- `cargo clippy -p diffguard-domain` — no redundant match arm warning
+
+## Risks
+
+1. **Strict compile mode:** If crate uses `#[deny(dead_code)]` or `#[deny(unreachable_patterns)]`, removing the arm could trigger an error. Verified: no such attributes present.
+2. **Documentation signal:** The removed arm comment (`// JSON supports comments in jsonc/json5 dialects`) served as documentation. The updated comment on line 82 (`// JSON handled by wildcard (jsonc/json5 dialects use C-style comments)`) preserves intent.
+
+## Verification Checklist
+
+- [ ] `cargo build --workspace` succeeds
+- [ ] `cargo test --workspace` passes
+- [ ] `cargo clippy --all-targets` shows no redundant match arm warning
+- [ ] `git diff` shows expected 1 insertion, 2 deletions

CHANGELOG.md

@@ -56,6 +56,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   - Affects all output formats (markdown, SARIF, GitLab Quality JSON, JUnit, CSV)
   - Rationale: enterprises need to onboard existing codebases without flagging pre-existing issues
 
+### Internal
+
+- **Extracted duplicated `escape_xml` function** from `checkstyle.rs` and `junit.rs` into shared `xml_utils.rs` module
+
 ## [0.2.0] - 2026-04-06
 
 ### Added

adr-011-parse-blame-porcelain-result.md

@@ -0,0 +1,110 @@
+# ADR-011: Remove Unnecessary Result Wrapper from parse_blame_porcelain
+
+**Status:** Accepted
+
+**Date:** 2026-04-11
+
+**Work Item:** work-430b0729
+
+---
+
+## Context
+
+Issue #141 reports that `parse_blame_porcelain` in `crates/diffguard/src/main.rs` (line 1768) is typed to return `Result<BTreeMap<u32, BlameLineMeta>>` but never actually returns `Err`. This is dead code — the function silently skips invalid entries via `continue` rather than propagating errors, and always reaches `Ok(out)` at line 1818.
+
+Clippy detects this pattern with the `unnecessary_result_bool` lint (or equivalent): *"this function's return value is unnecessarily wrapped by `Result`"*.
+
+---
+
+## Decision
+
+Change `parse_blame_porcelain` to return `BTreeMap<u32, BlameLineMeta>` directly, removing the `Result` wrapper.
+
+### Changes Required
+
+1. **Function signature (line 1768):**
+   ```rust
+   // Before
+   fn parse_blame_porcelain(blame_text: &str) -> Result<BTreeMap<u32, BlameLineMeta>>
+
+   // After
+   fn parse_blame_porcelain(blame_text: &str) -> BTreeMap<u32, BlameLineMeta>
+   ```
+
+2. **Return expression (line 1818):**
+   ```rust
+   // Before
+   Ok(out)
+
+   // After
+   out
+   ```
+
+3. **Caller in `collect_blame_allowed_lines` (lines 1861-1862):**
+   ```rust
+   // Before
+   let blame_map = parse_blame_porcelain(&blame_text)
+       .with_context(|| format!("parse git blame for {}", path))?;
+
+   // After
+   let blame_map = parse_blame_porcelain(&blame_text);
+   ```
+
+4. **Test at line 4068:**
+   ```rust
+   // Before
+   let map = parse_blame_porcelain(porcelain).expect("parse");
+
+   // After
+   let map = parse_blame_porcelain(porcelain);
+   ```
+
+### Rationale for Silent-Skip Behavior
+
+The parsing logic skips malformed entries rather than failing because:
+- Git blame output for files with unusual content (binary, untrusted encoding) may contain partial/invalid entries
+- The function is used to extract allowed-line metadata for diff checking — incomplete data is tolerable, hard failure is not
+- This behavior is established and users depend on it
+
+---
+
+## Alternatives Considered
+
+### 1. Keep Result and document the never-err case
+Adding a comment like `// SAFETY: this function never returns Err` would suppress the lint but leave unnecessary complexity for callers.
+
+### 2. Return Option instead of bare BTreeMap
+`Option<BTreeMap<u32, BlameLineMeta>>` would allow `None` for parse failures, but no call site checks for `Err` so `None` would be equally unused. The bare type is cleaner.
+
+### 3. Make the function return Result and propagate real errors
+Adding proper error propagation would be a breaking change to the call sites' logic and is out of scope for this fix.
+
+---
+
+## Consequences
+
+**Positive:**
+- Removes dead error-handling code from callers
+- Eliminates Clippy lint
+- Improves code clarity — readers know the function cannot fail
+- Removes `.expect()` from test, making test failure messages cleaner
+
+**Negative:**
+- None — this is purely a refactor with no behavioral change
+
+**Neutral:**
+- The `anyhow::Result` type alias used throughout the crate remains; this only affects one function's return type
+
+---
+
+## Files Affected
+
+- `crates/diffguard/src/main.rs` — function definition (line 1768), return (line 1818), caller (lines 1861-1862), test (line 4068)
+
+---
+
+## Verification
+
+After applying changes:
+1. Run `cargo clippy -p diffguard` — confirm no lint warnings related to `parse_blame_porcelain`
+2. Run `cargo test -p diffguard` — confirm all tests pass, especially `parse_blame_porcelain_extracts_line_metadata`

crates/diffguard-core/src/checkstyle.rs

@@ -7,6 +7,7 @@
 
 use std::collections::BTreeMap;
 
+use crate::xml_utils::escape_xml;
 use diffguard_types::{CheckReceipt, Finding, Severity};
 
 /// Renders a CheckReceipt as a Checkstyle XML report.
@@ -77,24 +78,6 @@ pub fn render_checkstyle_for_receipt(receipt: &CheckReceipt) -> String {
     out
 }
 
-/// Escape characters that have special meaning in XML.
-///
-/// Required for: description, message, path, rule_id, and any other text content.
-fn escape_xml(s: &str) -> String {
-    let mut out = String::with_capacity(s.len());
-    for c in s.chars() {
-        match c {
-            '&' => out.push_str("&"),
-            '<' => out.push_str("&lt;"),
-            '>' => out.push_str("&gt;"),
-            '"' => out.push_str("&quot;"),
-            '\'' => out.push_str("&apos;"),
-            _ => out.push(c),
-        }
-    }
-    out
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

crates/diffguard-core/src/junit.rs

@@ -5,6 +5,7 @@
 
 use std::collections::BTreeMap;
 
+use crate::xml_utils::escape_xml;
 use diffguard_types::{CheckReceipt, Finding, Severity};
 
 /// Renders a CheckReceipt as a JUnit XML report.
@@ -103,22 +104,6 @@ pub fn render_junit_for_receipt(receipt: &CheckReceipt) -> String {
     out
 }
 
-/// Escapes special XML characters in a string.
-fn escape_xml(s: &str) -> String {
-    let mut out = String::with_capacity(s.len());
-    for c in s.chars() {
-        match c {
-            '&' => out.push_str("&"),
-            '<' => out.push_str("&lt;"),
-            '>' => out.push_str("&gt;"),
-            '"' => out.push_str("&quot;"),
-            '\'' => out.push_str("&apos;"),
-            _ => out.push(c),
-        }
-    }
-    out
-}
-
 #[cfg(test)]
 mod tests {
     use super::*;

crates/diffguard-core/src/lib.rs

@@ -10,6 +10,7 @@ mod render;
 mod sarif;
 mod sensor;
 mod sensor_api;
+mod xml_utils;
 
 pub use check::{CheckPlan, CheckRun, PathFilterError, run_check};
 pub use checkstyle::render_checkstyle_for_receipt;