diff --git a/notes/bf-303t6.md b/notes/bf-303t6.md new file mode 100644 index 0000000..b639ba9 --- /dev/null +++ b/notes/bf-303t6.md @@ -0,0 +1,300 @@ +# Test Coverage Analysis: Truncated-Flate Recovery + +**Bead:** bf-303t6 +**Date:** 2026-07-05 +**Analysis:** Current test assertions and coverage gaps for truncated-flate error recovery + +## Current Test Landscape + +### 1. Integration-Level Test: `error_recovery_integration.rs::test_truncated_mid_stream()` + +**Location:** `crates/pdftract-core/tests/error_recovery_integration.rs:165-188` + +**Current Assertions:** +```rust +fn test_truncated_mid_stream() { + let fixture_path = fixture_path("truncated_mid_stream.pdf"); + let expected = load_expected_diagnostics(&fixture_path); + + let result = assert_no_panic("test_truncated_mid_stream", || { + let pdf_data = fs::read(&fixture_path).expect("fixture should exist"); + assert!(pdf_data.starts_with(b"%PDF-"), "Should be a valid PDF"); + + // Verify expected: STREAM_DECODE_ERROR + let stream_diags: Vec<_> = expected + .expected_diagnostics + .iter() + .filter(|d| d.code.contains("STREAM_DECODE")) + .collect(); + + assert!( + !stream_diags.is_empty(), + "Should expect STREAM_DECODE_ERROR diagnostic" + ); + }); + + assert!(result.is_ok(), "Test should not panic"); +} +``` + +**What's Validated:** +- ✅ Fixture file exists and is valid PDF structure +- ✅ Expected diagnostics JSON file exists +- ✅ Expected diagnostics includes `STREAM_DECODE_ERROR` +- ✅ Test doesn't panic (INV-8 compliance) + +**What's NOT Validated:** +- ❌ Actual extraction is never run +- ❌ No diagnostics are actually emitted or verified +- ❌ No verification of partial output behavior +- ❌ No verification that extraction succeeds despite truncation + +### 2. Stream Decoder Test: `stream_decoder_fixtures.rs` + +**Location:** `crates/pdftract-core/tests/stream_decoder_fixtures.rs:61-65` + +**Fixture Configuration:** +```rust +FixtureInfo { + name: "flate_truncated", + filter: FixtureFilter::Single("FlateDecode", None), + expected_diags: vec![], // ⚠️ EMPTY - no diagnostics expected! + bomb_limit: None, +}, +``` + +**Current Assertions:** +```rust +// Decode the fixture +let result = decode_fixture(&fixture, &input); +let decoded = match result { + Ok(data) => data, + Err(e) => { + failures.push(format!("{}: {}", fixture.name, e)); + continue; + } +}; + +// Compare against expected +if &decoded[..expected_bytes.len().min(decoded.len())] != expected_bytes { + failures.push(format!( + "{}: output mismatch (expected {} bytes, got {} bytes)", + fixture.name, + expected.len(), + decoded.len() + )); + continue; +} +``` + +**What's Validated:** +- ✅ Decoder doesn't return `Err` (returns `Ok` with partial bytes) +- ✅ Output matches expected file (0 bytes for truncated fixture) +- ✅ No panic occurs + +**What's NOT Validated:** +- ❌ `STREAM_DECODE_ERROR` diagnostic is NOT verified (expected_diags is empty!) +- ❌ Metadata says "expects partial bytes + STREAM_DECODE_ERROR" but test doesn't verify +- ❌ No verification that error is properly propagated +- ❌ No distinction between success and partial recovery + +### 3. Implementation: `FlateDecoder::decode_impl()` + +**Location:** `crates/pdftract-core/src/parser/stream.rs:524-558` + +**Error Handling:** +```rust +fn decode_impl( + mut decoder: R, + doc_counter: &mut u64, + max_bytes: u64, +) -> Vec { + let mut output = Vec::new(); + let mut chunk = vec![0u8; BOMB_CHECK_CHUNK]; + + loop { + match decoder.read(&mut chunk) { + Ok(0) => break, + Ok(n) => { + // Check bomb limit BEFORE adding bytes to output + if *doc_counter + output.len() as u64 + n as u64 > max_bytes { + // Bomb limit exceeded - return partial bytes + let remaining = (max_bytes - *doc_counter - output.len() as u64) as usize; + let to_add = remaining.min(n); + output.extend_from_slice(&chunk[..to_add]); + return output; + } + output.extend_from_slice(&chunk[..n]); + } + Err(e) if e.kind() == std::io::ErrorKind::UnexpectedEof => { + // Truncated stream - return partial bytes (INV-8) + break; // ⚠️ SILENT - no diagnostic emitted + } + Err(_) => { + // Other decoder errors - return partial bytes decoded so far + break; // ⚠️ SILENT - no diagnostic emitted + } + } + } + + output // ⚠️ Returns Vec - caller can't detect error occurred +} +``` + +**Critical Gaps:** +- ❌ `UnexpectedEof` is caught but no diagnostic is emitted +- ❌ Other decoder errors are silently caught +- ❌ Return type is `Vec`, not `Result, FilterError>` +- ❌ Caller has no way to distinguish success from partial recovery + +## Coverage Gap Summary + +### Gap 1: Missing Diagnostic Emission +**Severity:** HIGH +**Issue:** FlateDecoder catches `UnexpectedEof` but never emits `STREAM_DECODE_ERROR` diagnostic + +**Evidence:** +- Metadata file says: "expects partial bytes + STREAM_DECODE_ERROR" +- Implementation catches error but doesn't emit diagnostic +- Test has `expected_diags: vec![]` (empty) + +**Impact:** +- Diagnostics system is bypassed +- Users can't detect truncated streams in their PDFs +- INV-8 requires graceful recovery, but silent recovery hides issues + +### Gap 2: No Error Indication in Return Type +**Severity:** HIGH +**Issue:** `decode_impl()` returns `Vec` directly, not `Result` + +**Evidence:** +- Method signature: `fn decode_impl(...) -> Vec` +- Both success and error return `Vec` +- Caller can't distinguish partial recovery from clean decode + +**Impact:** +- No way for callers to know truncation occurred +- Violates principle of least surprise +- Makes debugging harder + +### Gap 3: Integration Test Doesn't Extract +**Severity:** MEDIUM +**Issue:** `test_truncated_mid_stream()` validates JSON but doesn't run extraction + +**Evidence:** +- Test reads PDF data but doesn't call `pdftract::extract()` +- Only validates fixture structure and expected diagnostics JSON +- Comment says: `// TODO: Extract with pdftract once API is available` + +**Impact:** +- End-to-end behavior is untested +- Integration between stream decoder and extraction pipeline is unverified +- Real-world behavior with actual PDF extraction is unknown + +### Gap 4: No Verification of Partial Output Quality +**Severity:** MEDIUM +**Issue:** Tests don't verify partial output is usable or meaningful + +**Evidence:** +- `flate_truncated.expected` is 0 bytes (trivially passes) +- No test verifies partial data represents valid prefix +- No test verifies remaining bytes are sensible + +**Impact:** +- Can't detect if partial output is garbage +- Can't verify recovery produces useful data +- Quality of partial recovery is untested + +### Gap 5: Stream Decoder Test Mismatch with Metadata +**Severity:** LOW +**Issue:** Test expects no diagnostics, metadata says STREAM_DECODE_ERROR expected + +**Evidence:** +- `stream_decoder_fixtures.rs`: `expected_diags: vec![]` +- `flate_truncated.meta`: "expects partial bytes + STREAM_DECODE_ERROR" + +**Impact:** +- Conflicting documentation +- Unclear what the actual expectation is +- Test may be out of sync with requirements + +## Test Files Examined + +1. **`crates/pdftract-core/tests/error_recovery_integration.rs`** (315 lines) + - Integration-level adversarial test corpus + - Tests 7 fixtures including `truncated_mid_stream.pdf` + +2. **`crates/pdftract-core/tests/stream_decoder_fixtures.rs`** (394 lines) + - Unit-level stream decoder tests + - Tests 15+ fixtures including `flate_truncated` + +3. **`crates/pdftract-core/src/parser/stream.rs`** (600+ lines) + - FlateDecoder implementation + - Error handling in `decode_impl()` + +4. **Fixture files:** + - `/tests/stream_decoder/fixtures/flate_truncated.bin` (10 bytes, truncated zlib) + - `/tests/stream_decoder/fixtures/flate_truncated.expected` (0 bytes) + - `/tests/stream_decoder/fixtures/flate_truncated.meta` ("expects partial bytes + STREAM_DECODE_ERROR") + - `/tests/error_recovery/fixtures/truncated_mid_stream.pdf` + - `/tests/error_recovery/fixtures/truncated_mid_stream.expected_diagnostics.json` + +## Recommendations for New Assertions + +### High Priority (Required for Complete Coverage) + +1. **Verify diagnostic emission in FlateDecoder** + - Assert `STREAM_DECODE_ERROR` is emitted on `UnexpectedEof` + - Update test: `expected_diags: vec![DiagCode::StreamDecodeError]` + +2. **Add integration test that actually extracts** + - Call `pdftract::extract()` on `truncated_mid_stream.pdf` + - Verify extraction succeeds despite truncation + - Verify diagnostics contain `STREAM_DECODE_ERROR` + +3. **Verify partial output is meaningful** + - Create fixture with known valid prefix + - Assert decoded bytes match expected prefix + - Verify partial data isn't garbage + +### Medium Priority (Improves Robustness) + +4. **Test error propagation through extraction pipeline** + - Verify stream decoder errors are properly reported + - Test diagnostics are collected and surfaced + - Verify no panic occurs (INV-8) + +5. **Add test for multiple truncation scenarios** + - Truncated at different points (early, mid, late) + - Truncated with different filter types (Flate, LZW, etc.) + - Truncated in filter arrays (multi-stage decoding) + +### Low Priority (Nice to Have) + +6. **Test bomb limit interaction with truncation** + - Verify truncation before bomb limit + - Verify truncation after bomb limit + - Verify both diagnostics can co-occur + +7. **Add regression test for specific real-world PDFs** + - Collect corpus of PDFs with truncated streams + - Test each produces reasonable output + - Track quality of partial recovery over time + +## Conclusion + +The current test coverage for truncated-flate recovery has **significant gaps**: + +- ✅ **Basic INV-8 compliance is tested**: No panics occur +- ❌ **Diagnostic emission is NOT tested**: Stream decoder doesn't emit diagnostics +- ❌ **Integration behavior is NOT tested**: PDF extraction is never called +- ❌ **Output quality is NOT verified**: Partial data quality is unchecked +- ❌ **Error detection is impossible**: Return type can't indicate errors + +**Most Critical Gap:** The FlateDecoder silently catches `UnexpectedEof` but never emits `STREAM_DECODE_ERROR` diagnostic, making it impossible for users or downstream code to detect that truncation occurred. + +**Path Forward:** +1. Fix FlateDecoder to emit diagnostics +2. Update stream decoder test to expect diagnostics +3. Make integration test actually run extraction +4. Add assertions for partial output quality