fix: address review issues — dead code, swallowed errors, unclear messages, test duplication

- Remove unreachable `<error>` branches in `debug_bytes` (len > 6 is
  already proven by the early return, so direct indexing is safe)
- Propagate read errors in Node.js and Python file-read iterators
  instead of silently returning None on I/O failure
- Improve `into_datamap()` doc and error messages to explain that
  `chunks()` must be fully consumed before calling it
- Deduplicate 5 near-identical streaming roundtrip integration tests
  into 2 parameterized helpers

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Author: 2 people

nodejs/src/lib.rs

@@ -436,7 +436,11 @@ pub fn streaming_encrypt_from_file(
     let data_size = file.metadata().map_err(map_error)?.len() as usize;
     let mut reader = std::io::BufReader::new(file);
 
-    // Create iterator that reads file in chunks
+    // Track read errors so they aren't silently swallowed by the iterator
+    let read_error: std::rc::Rc<std::cell::RefCell<Option<std::io::Error>>> =
+        std::rc::Rc::new(std::cell::RefCell::new(None));
+    let read_error_clone = read_error.clone();
+
     let data_iter = std::iter::from_fn(move || {
         let mut buffer = vec![0u8; 8192];
         match reader.read(&mut buffer) {
@@ -445,7 +449,10 @@ pub fn streaming_encrypt_from_file(
                 buffer.truncate(n);
                 Some(Bytes::from(buffer))
             }
-            Err(_) => None,
+            Err(e) => {
+                *read_error_clone.borrow_mut() = Some(e);
+                None
+            }
         }
     });
 
@@ -474,10 +481,17 @@ pub fn streaming_encrypt_from_file(
             })?;
     }
 
+    if let Some(e) = read_error.borrow_mut().take() {
+        return Err(napi::Error::new(
+            Status::GenericFailure,
+            format!("Failed to read input file: {e}"),
+        ));
+    }
+
     let datamap = stream.into_datamap().ok_or_else(|| {
         napi::Error::new(
             Status::GenericFailure,
-            "Encryption did not produce a DataMap",
+            "Encryption did not produce a DataMap — ensure chunks() iterator was fully consumed",
         )
     })?;
 
@@ -499,6 +513,11 @@ pub fn encrypt_from_file(input_file: String, output_dir: String) -> Result<Encry
     let data_size = file.metadata().map_err(map_error)?.len() as usize;
     let mut reader = std::io::BufReader::new(file);
 
+    // Track read errors so they aren't silently swallowed by the iterator
+    let read_error: std::rc::Rc<std::cell::RefCell<Option<std::io::Error>>> =
+        std::rc::Rc::new(std::cell::RefCell::new(None));
+    let read_error_clone = read_error.clone();
+
     let data_iter = std::iter::from_fn(move || {
         let mut buffer = vec![0u8; 8192];
         match reader.read(&mut buffer) {
@@ -507,7 +526,10 @@ pub fn encrypt_from_file(input_file: String, output_dir: String) -> Result<Encry
                 buffer.truncate(n);
                 Some(Bytes::from(buffer))
             }
-            Err(_) => None,
+            Err(e) => {
+                *read_error_clone.borrow_mut() = Some(e);
+                None
+            }
         }
     });
 
@@ -533,10 +555,17 @@ pub fn encrypt_from_file(input_file: String, output_dir: String) -> Result<Encry
         })?;
     }
 
+    if let Some(e) = read_error.borrow_mut().take() {
+        return Err(napi::Error::new(
+            Status::GenericFailure,
+            format!("Failed to read input file: {e}"),
+        ));
+    }
+
     let data_map = stream.into_datamap().ok_or_else(|| {
         napi::Error::new(
             Status::GenericFailure,
-            "Encryption did not produce a DataMap",
+            "Encryption did not produce a DataMap — ensure chunks() iterator was fully consumed",
         )
     })?;
 

src/data_map.rs

@@ -246,22 +246,17 @@ fn debug_bytes<V: AsRef<[u8]>>(input: V) -> String {
         }
         return ret;
     }
-    match (input_ref.first(), input_ref.get(1), input_ref.get(2)) {
-        (Some(a), Some(b), Some(c)) => {
-            let len = input_ref.len();
-            match (
-                input_ref.get(len - 3),
-                input_ref.get(len - 2),
-                input_ref.get(len - 1),
-            ) {
-                (Some(x), Some(y), Some(z)) => {
-                    format!("{a:02x}{b:02x}{c:02x}..{x:02x}{y:02x}{z:02x}")
-                }
-                _ => "<error>".to_owned(),
-            }
-        }
-        _ => "<error>".to_owned(),
-    }
+    // Safety: len > 6 is guaranteed by the early return above, so all indices are valid.
+    let len = input_ref.len();
+    format!(
+        "{:02x}{:02x}{:02x}..{:02x}{:02x}{:02x}",
+        input_ref[0],
+        input_ref[1],
+        input_ref[2],
+        input_ref[len - 3],
+        input_ref[len - 2],
+        input_ref[len - 1]
+    )
 }
 
 impl Debug for ChunkInfo {

src/python.rs

@@ -345,6 +345,11 @@ pub fn encrypt_from_file(input_file: &str, output_dir: &str) -> PyResult<(PyData
         .len() as usize;
     let mut reader = std::io::BufReader::new(file);
 
+    // Track read errors so they aren't silently swallowed by the iterator
+    let read_error: std::rc::Rc<std::cell::RefCell<Option<std::io::Error>>> =
+        std::rc::Rc::new(std::cell::RefCell::new(None));
+    let read_error_clone = read_error.clone();
+
     let data_iter = std::iter::from_fn(move || {
         let mut buffer = vec![0u8; 8192];
         match reader.read(&mut buffer) {
@@ -353,7 +358,10 @@ pub fn encrypt_from_file(input_file: &str, output_dir: &str) -> PyResult<(PyData
                 buffer.truncate(n);
                 Some(Bytes::from(buffer))
             }
-            Err(_) => None,
+            Err(e) => {
+                *read_error_clone.borrow_mut() = Some(e);
+                None
+            }
         }
     });
 
@@ -375,8 +383,16 @@ pub fn encrypt_from_file(input_file: &str, output_dir: &str) -> PyResult<(PyData
         })?;
     }
 
+    if let Some(e) = read_error.borrow_mut().take() {
+        return Err(pyo3::exceptions::PyOSError::new_err(format!(
+            "Failed to read input file: {e}"
+        )));
+    }
+
     let data_map = stream.into_datamap().ok_or_else(|| {
-        pyo3::exceptions::PyValueError::new_err("Encryption did not produce a DataMap")
+        pyo3::exceptions::PyValueError::new_err(
+            "Encryption did not produce a DataMap — ensure chunks() iterator was fully consumed",
+        )
     })?;
 
     Ok((PyDataMap { inner: data_map }, chunk_names))

src/stream_encrypt.rs

@@ -602,7 +602,8 @@ impl<I> EncryptionStream<I> {
     /// # Ok(())
     /// # }
     /// ```
-    /// Returns the final DataMap, or `None` if encryption is not yet complete.
+    /// Returns the final DataMap, or `None` if encryption is not yet complete
+    /// (i.e., `chunks()` was not fully consumed).
     pub fn into_datamap(self) -> Option<DataMap> {
         self.final_datamap
     }

tests/integration_tests.rs

@@ -873,16 +873,15 @@ fn make_parallel_fetcher(
     }
 }
 
-// --- Task 5: stream_encrypt + streaming_decrypt roundtrip ---
-
-#[test]
-fn test_stream_encrypt_decrypt_roundtrip() -> Result<()> {
-    let data_size = 100_000;
+/// Helper: stream-encrypt data with a given iterator chunk size, then stream-decrypt and verify.
+fn assert_stream_roundtrip(data_size: usize, iter_chunk_size: usize) -> Result<()> {
     let original_data = random_bytes(data_size);
 
     let mut stream = stream_encrypt(
         data_size,
-        original_data.chunks(4096).map(|c| Bytes::from(c.to_vec())),
+        original_data
+            .chunks(iter_chunk_size)
+            .map(|c| Bytes::from(c.to_vec())),
     )?;
 
     let mut storage = HashMap::new();
@@ -893,82 +892,34 @@ fn test_stream_encrypt_decrypt_roundtrip() -> Result<()> {
 
     let data_map = stream
         .datamap()
-        .ok_or_else(|| Error::Generic("No DataMap after stream_encrypt".to_string()))?;
+        .ok_or_else(|| Error::Generic("No DataMap after stream_encrypt".to_string()))?
+        .clone();
 
     let fetcher = make_parallel_fetcher(&storage);
-    let decrypt_stream = streaming_decrypt(data_map, fetcher)?;
+    let decrypt_stream = streaming_decrypt(&data_map, fetcher)?;
     let decrypted = decrypt_stream.range_full()?;
 
     assert_eq!(decrypted.as_ref(), &original_data[..]);
     Ok(())
 }
 
-// --- Task 6: stream_encrypt + streaming_decrypt roundtrip ---
+#[test]
+fn test_stream_encrypt_decrypt_roundtrip() -> Result<()> {
+    assert_stream_roundtrip(100_000, 4096)
+}
 
 #[test]
 fn test_file_stream_encrypt_decrypt_roundtrip() -> Result<()> {
-    let data_size = 200_000;
-    let original_data = random_bytes(data_size);
-
-    let mut stream = stream_encrypt(
-        data_size,
-        original_data.chunks(8192).map(|c| Bytes::from(c.to_vec())),
-    )?;
-
-    let mut storage = HashMap::new();
-    for chunk_result in stream.chunks() {
-        let (hash, content) = chunk_result?;
-        let _ = storage.insert(hash, content.to_vec());
-    }
-
-    let data_map = stream
-        .datamap()
-        .ok_or_else(|| Error::Generic("No DataMap".to_string()))?
-        .clone();
-
-    let fetcher = make_parallel_fetcher(&storage);
-    let decrypt_stream = streaming_decrypt(&data_map, fetcher)?;
-    let decrypted = decrypt_stream.range_full()?;
-
-    assert_eq!(decrypted.as_ref(), &original_data[..]);
-    Ok(())
+    assert_stream_roundtrip(200_000, 8192)
 }
 
-// --- Task 7: Cross-compatibility between streaming APIs ---
-
 #[test]
 fn test_stream_encrypt_file_decrypt_storage_cross() -> Result<()> {
-    let data_size = 150_000;
-    let original_data = random_bytes(data_size);
-
-    let mut stream = stream_encrypt(
-        data_size,
-        original_data.chunks(8192).map(|c| Bytes::from(c.to_vec())),
-    )?;
-
-    let mut storage = HashMap::new();
-    for chunk_result in stream.chunks() {
-        let (hash, content) = chunk_result?;
-        let _ = storage.insert(hash, content.to_vec());
-    }
-
-    let data_map = stream
-        .datamap()
-        .ok_or_else(|| Error::Generic("No DataMap".to_string()))?
-        .clone();
-
-    let fetcher = make_parallel_fetcher(&storage);
-    let decrypt_stream = streaming_decrypt(&data_map, fetcher)?;
-    let decrypted = decrypt_stream.range_full()?;
-
-    assert_eq!(decrypted.as_ref(), &original_data[..]);
-    Ok(())
+    assert_stream_roundtrip(150_000, 8192)
 }
 
-#[test]
-fn test_file_encrypt_stream_decrypt_cross() -> Result<()> {
-    // Use standard encrypt() then streaming decrypt — cross-compatibility test
-    let data_size = 150_000;
+/// Helper: standard encrypt() then streaming_decrypt() cross-compatibility check.
+fn assert_encrypt_then_stream_decrypt(data_size: usize) -> Result<()> {
     let original_data = random_bytes(data_size);
 
     let (data_map, encrypted_chunks) = encrypt(original_data.clone())?;
@@ -982,22 +933,14 @@ fn test_file_encrypt_stream_decrypt_cross() -> Result<()> {
     Ok(())
 }
 
-// --- Task 8: In-memory encrypt ↔ streaming decrypt cross-compatibility ---
+#[test]
+fn test_file_encrypt_stream_decrypt_cross() -> Result<()> {
+    assert_encrypt_then_stream_decrypt(150_000)
+}
 
 #[test]
 fn test_memory_encrypt_stream_decrypt() -> Result<()> {
-    let data_size = 100_000;
-    let original_data = random_bytes(data_size);
-
-    let (data_map, encrypted_chunks) = encrypt(original_data.clone())?;
-    let storage = build_chunk_storage(&encrypted_chunks);
-    let fetcher = make_parallel_fetcher(&storage);
-
-    let decrypt_stream = streaming_decrypt(&data_map, fetcher)?;
-    let decrypted = decrypt_stream.range_full()?;
-
-    assert_eq!(decrypted.as_ref(), &original_data[..]);
-    Ok(())
+    assert_encrypt_then_stream_decrypt(100_000)
 }
 
 #[test]