fix(detection): contain unidiff panic on orphaned +++ target line (headroomlabs-ai#1548)

tenderdeve · web-flow · commit e386c097d6d5 · 2026-07-01T17:10:52.000-05:00
## Description `headroom._core.detect_content_type()` panics with `pyo3_runtime.PanicException: called Option::unwrap() on a None value` on any text containing a `+++ ` target line with no preceding `--- ` source line — e.g. `set -x` xtrace output or a partial `git diff` quoted out of context. The panic originates in the bundled `unidiff` 0.4.0 parser (`lib.rs:665`): on a target-file header it does `source_file.clone().unwrap()`, but `source_file` is still `None` when no source header was seen. The crate's only guard there checks `current_file`, not `source_file`, so it falls through and unwraps `None` instead of returning `Err`. Because detection runs inside a `ThreadPoolExecutor` worker on the Python side, the native panic surfaces as an uncaught `PanicException`, bypasses the compression error handling, and returns **HTTP 500** for the whole request. The failure is deterministic on payload content, so client retries fail until the offending text leaves the context window. `is_diff()` in `unidiff_detector.rs` is the single entry point that drives `PatchSet::parse`, so the fix is contained there: wrap the parse in `catch_unwind` and treat an unparseable fragment as "not a diff". This matches the workspace's deliberate no-`panic = "abort"` policy (Cargo.toml) of surviving bad input rather than taking the long-lived proxy down. Closes headroomlabs-ai#1547 ## Type of Change - [x] Bug fix (non-breaking change that fixes an issue) ## Changes Made - `crates/headroom-core/src/transforms/unidiff_detector.rs`: contain any `unidiff` parser panic inside `is_diff()` via `catch_unwind`, returning `false` (not a diff) on panic. Added regression test `orphaned_target_line_does_not_panic`. - `CHANGELOG.md`: note under Unreleased → Fixed. ## Testing - [x] Unit tests pass (`cargo test -p headroom-core`) - [x] Linting passes (`cargo fmt --check`, `cargo clippy`) - [x] New tests added for new functionality - [x] Manual testing performed ### Test Output Before the fix (regression test reproduces the exact panic): ```text running 1 test test transforms::unidiff_detector::tests::orphaned_target_line_does_not_panic ... FAILED ---- transforms::unidiff_detector::tests::orphaned_target_line_does_not_panic stdout ---- thread '...' panicked at unidiff-0.4.0/src/lib.rs:665:54: called `Option::unwrap()` on a `None` value test result: FAILED. 0 passed; 1 failed; ... ``` After the fix: ```text running 15 tests test transforms::unidiff_detector::tests::orphaned_target_line_does_not_panic ... ok test transforms::unidiff_detector::tests::standard_git_diff_detected ... ok ... test result: ok. 15 passed; 0 failed; 0 ignored # whole transforms suite test result: ok. 700 passed; 0 failed; 0 ignored ``` ## Real Behavior Proof - Environment: macOS (arm64), Rust stable, `cargo test -p headroom-core`. - Exact command / steps: `cargo test -p headroom-core --lib unidiff_detector` then `cargo test -p headroom-core`. (1) Added a test calling `is_diff("+++ x")` / `detect_diff("+++ x")` and ran it → reproduced the panic at `unidiff-0.4.0/src/lib.rs:665:54` (output above), confirming the same crash path as the report. (2) Applied the `catch_unwind` containment in `is_diff()`. (3) Re-ran the test and the full transforms suite → all green (output above). - Observed result: the orphaned-`+++ ` input is now classified as "not a diff" (plain text) and returns normally instead of panicking. Real diffs (`standard_git_diff_detected`, `naked_hunk_without_git_header_detected`, multi-file, added/removed-only) still detect correctly, so the containment does not weaken detection. - Not tested: I exercised the Rust layer directly (the sole `unidiff` caller, which the `headroom._core.detect_content_type` binding routes through) rather than rebuilding the Python wheel; I did not run the live proxy against a real provider. ## Review Readiness - [x] I have performed a self-review - [x] This PR is ready for human review ## Checklist - [x] My code follows the project's style guidelines - [x] I have performed a self-review of my code - [x] I have commented my code, particularly in hard-to-understand areas - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective - [x] New and existing unit tests pass locally with my changes - [x] I have updated the CHANGELOG.md
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,6 +9,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ## Unreleased
 
 ### Fixed
+- Content detection no longer crashes the proxy on text containing an
+  orphaned `+++ ` target line with no preceding `--- ` source line (common in
+  `set -x` xtrace output and partial diffs). The bundled `unidiff` 0.4.0 parser
+  panics on that input instead of returning an error; the Rust diff detector now
+  contains the panic and treats the fragment as plain text, so the request is
+  compressed and forwarded normally instead of returning HTTP 500
+  ([#1547](https://github.com/headroomlabs-ai/headroom/issues/1547)).
 - Proactive expansion blocks injected into user turns are now wrapped in
   `<headroom_proactive_expansion>` XML tags, giving downstream consumers
   (LLMs, loggers, attribution parsers) a machine-readable provenance
diff --git a/crates/headroom-core/src/transforms/unidiff_detector.rs b/crates/headroom-core/src/transforms/unidiff_detector.rs
@@ -61,16 +61,28 @@ pub fn is_diff(content: &str) -> bool {
         return false;
     }
 
-    let mut patch = PatchSet::new();
-    if patch.parse(content).is_err() {
-        return false;
-    }
+    // `unidiff` 0.4.0 does not return `Err` on every malformed input — a
+    // `+++ ` target line with no preceding `--- ` source line makes it
+    // `unwrap()` a `None` and panic (lib.rs:665). Inputs of that shape are
+    // common (`set -x` xtrace, partial diffs quoted out of context). This
+    // detector runs inside a thread-pool worker on the Python side, where a
+    // native panic surfaces as an uncaught `PanicException` and 500s the whole
+    // request. Contain any parser panic here and treat the fragment as "not a
+    // diff" — consistent with the workspace's no-`panic = "abort"` policy of
+    // surviving bad input rather than taking the process down.
+    std::panic::catch_unwind(std::panic::AssertUnwindSafe(|| {
+        let mut patch = PatchSet::new();
+        if patch.parse(content).is_err() {
+            return false;
+        }
 
-    // `PatchSet::is_empty()` covers "found zero files"; the inner
-    // loop covers "found a file but with zero hunks" (e.g. mode-only
-    // changes). For diff-compressor routing we want at least one
-    // hunk — that's where the actual line-level change content lives.
-    !patch.is_empty() && patch.files().iter().any(|f| !f.is_empty())
+        // `PatchSet::is_empty()` covers "found zero files"; the inner
+        // loop covers "found a file but with zero hunks" (e.g. mode-only
+        // changes). For diff-compressor routing we want at least one
+        // hunk — that's where the actual line-level change content lives.
+        !patch.is_empty() && patch.files().iter().any(|f| !f.is_empty())
+    }))
+    .unwrap_or(false)
 }
 
 /// [`ContentType`]-typed wrapper. Returns `Some(ContentType::GitDiff)`
@@ -229,4 +241,16 @@ mod tests {
         assert_eq!(detect_diff("{}"), None);
         assert_eq!(detect_diff(""), None);
     }
+
+    #[test]
+    fn orphaned_target_line_does_not_panic() {
+        // `unidiff` 0.4.0 panics (unwrap on `None`) when it meets a
+        // `+++ ` target line with no preceding `--- ` source line.
+        // That shape is common in `set -x` xtrace output and partial
+        // diffs quoted out of context. It must degrade to "not a diff",
+        // never abort the caller.
+        assert!(!is_diff("+++ x"));
+        assert_eq!(detect_diff("+++ x"), None);
+        assert!(!is_diff("some prose\n+++ target without a source\nmore"));
+    }
 }
