feat(work-e55f69ed): add # Errors sections to core public APIs

CHANGELOG.md

@@ -25,6 +25,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 - **`--color` flag for CI log output control** — Controls ANSI color output with `--color <never|always|auto>`. Use `--color=never` to suppress ANSI codes in CI logs (GitHub Actions, GitLab CI), `--color=always` to force colors in piped output, `--color=auto` (default) to auto-detect based on terminal. Respects `NO_COLOR=1` environment variable.
 
+- **`# Errors` sections for core public APIs** — Added `# Errors` sections to documentation for core public APIs per Rust API Guidelines C409:
+  - `parse_unified_diff`
+  - `compile_rules`
+  - `RuleOverrideMatcher::compile`
+  - `run_check`
+
 - **`bench` crate for performance benchmarking** — Criterion-based benchmark infrastructure:
   - Parsing benchmarks: measures `parse_unified_diff()` at 0, 100, 1K, 10K, 100K lines
   - Evaluation benchmarks: measures `evaluate_lines()` at 0, 1, 10, 100, 500 rules
@@ -56,6 +62,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/check.rs

@@ -81,6 +81,15 @@ pub enum PathFilterError {
     },
 }
 
+/// Run a policy check over a unified diff text.
+///
+/// # Errors
+///
+/// Returns an error if:
+/// - The diff text cannot be parsed ([`diffguard_diff::DiffParseError`])
+/// - Path filter globs are invalid ([`PathFilterError`])
+/// - Rule compilation fails ([`diffguard_domain::RuleCompileError`])
+/// - Override compilation fails ([`diffguard_domain::OverrideCompileError`])
 pub fn run_check(
     plan: &CheckPlan,
     config: &diffguard_types::ConfigFile,

crates/diffguard-core/src/checkstyle.rs

@@ -7,6 +7,7 @@
 
 use std::collections::BTreeMap;
 
+use super::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::*;
@@ -290,17 +273,4 @@ mod tests {
         assert!(xml.contains("<checkstyle version=\"5.0\">"));
         assert!(xml.contains("</checkstyle>"));
     }
-
-    #[test]
-    fn escape_xml_handles_all_special_chars() {
-        assert_eq!(escape_xml("&"), "&amp;");
-        assert_eq!(escape_xml("<"), "&lt;");
-        assert_eq!(escape_xml(">"), "&gt;");
-        assert_eq!(escape_xml("\""), "&quot;");
-        assert_eq!(escape_xml("'"), "&apos;");
-        assert_eq!(
-            escape_xml("a&b<c>d\"e'f"),
-            "a&amp;b&lt;c&gt;d&quot;e&apos;f"
-        );
-    }
 }

crates/diffguard-core/src/junit.rs

@@ -5,6 +5,7 @@
 
 use std::collections::BTreeMap;
 
+use super::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::*;
@@ -327,15 +312,4 @@ mod tests {
         let xml = render_junit_for_receipt(&receipt);
         insta::assert_snapshot!(xml);
     }
-
-    #[test]
-    fn escape_xml_handles_all_special_chars() {
-        assert_eq!(escape_xml("&"), "&amp;");
-        assert_eq!(escape_xml("<"), "&lt;");
-        assert_eq!(escape_xml(">"), "&gt;");
-        assert_eq!(escape_xml("\""), "&quot;");
-        assert_eq!(escape_xml("'"), "&apos;");
-        assert_eq!(escape_xml("normal text"), "normal text");
-        assert_eq!(escape_xml("<a & b>"), "&lt;a &amp; b&gt;");
-    }
 }

crates/diffguard-core/src/lib.rs

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

crates/diffguard-core/src/xml_utils.rs

@@ -0,0 +1,86 @@
+//! XML utility functions for diffguard output formatters.
+//!
+//! Provides shared XML escaping functionality used by JUnit, Checkstyle,
+//! and other XML-based output formats.
+
+/// Escapes special XML characters and illegal control characters in a string.
+///
+/// Handles:
+/// - 5 named XML entities: `&`, `<`, `>`, `"`, `'`
+/// - Illegal control characters (0x00-0x1F except tab/LF/CR) as `&#xNN;` entities
+///
+/// Legal control characters (tab=0x09, LF=0x0A, CR=0x0D) are preserved as-is
+/// since they are allowed in XML character content.
+pub fn escape_xml(s: &str) -> String {
+    let mut out = String::with_capacity(s.len());
+    for c in s.chars() {
+        match c {
+            '&' => out.push_str("&amp;"),
+            '<' => out.push_str("&lt;"),
+            '>' => out.push_str("&gt;"),
+            '"' => out.push_str("&quot;"),
+            '\'' => out.push_str("&apos;"),
+            // Illegal XML control characters (0x00-0x1F except tab/LF/CR)
+            c if c <= '\u{001F}' && c != '\t' && c != '\n' && c != '\r' => {
+                out.push_str(&format!("&#x{:X};", c as u32));
+            }
+            _ => out.push(c),
+        }
+    }
+    out
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    #[test]
+    fn escape_xml_handles_all_special_chars() {
+        assert_eq!(escape_xml("&"), "&amp;");
+        assert_eq!(escape_xml("<"), "&lt;");
+        assert_eq!(escape_xml(">"), "&gt;");
+        assert_eq!(escape_xml("\""), "&quot;");
+        assert_eq!(escape_xml("'"), "&apos;");
+        assert_eq!(escape_xml("normal text"), "normal text");
+        assert_eq!(escape_xml("<a & b>"), "&lt;a &amp; b&gt;");
+    }
+
+    #[test]
+    fn escape_xml_escapes_illegal_control_chars() {
+        // NUL
+        let result = escape_xml("a\x00b");
+        assert!(result.contains("&#x0;"));
+        assert!(!result.contains('\x00'));
+
+        // BEL (0x07)
+        let result = escape_xml("a\x07b");
+        assert!(result.contains("&#x7;"));
+
+        // ESC (0x1B)
+        let result = escape_xml("a\x1Bb");
+        assert!(result.contains("&#x1B;"));
+    }
+
+    #[test]
+    fn escape_xml_preserves_legal_control_chars() {
+        // Tab
+        let result = escape_xml("a\tb");
+        assert!(result.contains('\t'));
+        assert!(!result.contains("&#x9;"));
+
+        // LF
+        let result = escape_xml("a\nb");
+        assert!(result.contains('\n'));
+        assert!(!result.contains("&#xA;"));
+
+        // CR
+        let result = escape_xml("a\rb");
+        assert!(result.contains('\r'));
+        assert!(!result.contains("&#xD;"));
+    }
+
+    #[test]
+    fn escape_xml_empty_string() {
+        assert_eq!(escape_xml(""), "");
+    }
+}