💥 Replace wire-scalar enums with byte-backed newtypes#3218
Conversation
📝 WalkthroughWalkthroughThe PR converts archive metadata enums to byte-backed types with uppercase constants and compatibility helpers, updates header and reader handling, migrates CLI paths, and revises tests, fuzz targets, benchmarks, examples, and documentation. ChangesArchive metadata migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the Compression, Encryption, CipherMode, and DataKind types from enums to struct-based representations wrapping a u8. It introduces uppercase constants (e.g., Compression::NO, Encryption::AES, DataKind::FILE) to replace the deprecated CamelCase variants, updates the codebase to use these new constants, and simplifies parsing logic using infallible byte conversions. Additionally, comprehensive unit tests are added to verify the new struct-based representations and ensure backward compatibility. I have no feedback to provide as there are no review comments.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
8ef55b4 to
da30e15
Compare
github-actions
Bot
added
cli
da30e15 to
26f55aa
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cli/src/command/extract.rs (1)
1314-1319: 🩺 Stability & Availability | 🔴 Critical | ⚡ Quick winGracefully handle unknown
DataKinds to prevent denial-of-service.Crafted archives containing a private or reserved
DataKind(e.g., a raw byte value>= 4) will bypass theSYMBOLIC_LINKandDIRECTORYchecks in the upstream extraction loops and fall through toextract_file_entry. This triggers theunreachable!macro, panicking the thread and aborting extraction.Replace the panic with a graceful warning and early return to handle unknown byte values safely.
🛡️ Proposed fix
- if item.header().data_kind() != DataKind::FILE { - unreachable!( - "extract_file_entry called with {:?}", - item.header().data_kind() - ); - } + if item.header().data_kind() != DataKind::FILE { + log::warn!( + "Skipping unsupported data kind for {}: {:?}", + item_path, + item.header().data_kind() + ); + return Ok(()); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/command/extract.rs` around lines 1314 - 1319, Update extract_file_entry so unexpected DataKind values no longer trigger unreachable! or panic. Log a warning containing the received kind, then return early; preserve normal file extraction for DataKind::FILE and existing handling for symbolic links and directories.
🧹 Nitpick comments (2)
lib/src/entry/options.rs (1)
1561-1567: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock the wire-byte ordering into a regression test.
Derived
Ordcurrently satisfies the requirement, but no test protects reserved byte3ordering beforeXZ(4).Proposed test
+ #[test] + fn compression_orders_by_wire_byte() { + let reserved_three = Compression::from_byte(3); + assert!(Compression::ZSTANDARD < reserved_three); + assert!(reserved_three < Compression::XZ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@lib/src/entry/options.rs` around lines 1561 - 1567, Extend compression_known_constants_map_to_spec_bytes to assert the ordering around the reserved byte: verify the value corresponding to reserved byte 3 sorts before Compression::XZ (byte 4). Preserve the existing assertions for the known constants and use the enum’s ordering behavior directly.cli/src/command/diff.rs (1)
319-345: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove redundant match arms.
Since
DataKindis now a byte-backed newtype, exhaustive matching is safely handled by the_ =>wildcard arm. The arms forDataKind::FILE | DataKind::DIRECTORY | DataKind::SYMBOLIC_LINKand the fallbackDataKind::HARD_LINKperform the exact same action as the wildcard arm and can be cleanly removed to simplify the logic.♻️ Proposed refactor
- DataKind::FILE | DataKind::DIRECTORY | DataKind::SYMBOLIC_LINK => { - println!("{}", DiffKind::TypeMismatch.display(path_str)); - } DataKind::HARD_LINK if meta.is_file() => { let EntryContent::HardLink(stored) = entry.content(read_options)? else { unreachable!("data_kind() returned HardLink"); }; match is_same_file(path, stored.as_str()) { Ok(true) => (), Ok(false) => { println!( "{}", DiffKind::NotLinked(stored.to_string()).display(path_str) ); } Err(e) if e.kind() == io::ErrorKind::NotFound => { println!("{}", DiffKind::Missing.display(path_str)); } Err(e) => return Err(e), } } - DataKind::HARD_LINK => { - println!("{}", DiffKind::TypeMismatch.display(path_str)); - } _ => { println!("{}", DiffKind::TypeMismatch.display(path_str)); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/src/command/diff.rs` around lines 319 - 345, In the DataKind match within the diff handling logic, remove the redundant DataKind::FILE | DataKind::DIRECTORY | DataKind::SYMBOLIC_LINK arm and the fallback DataKind::HARD_LINK arm. Retain the specialized DataKind::HARD_LINK if meta.is_file() handling and the existing _ wildcard arm for all remaining cases.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cli/src/command/extract.rs`:
- Around line 1314-1319: Update extract_file_entry so unexpected DataKind values
no longer trigger unreachable! or panic. Log a warning containing the received
kind, then return early; preserve normal file extraction for DataKind::FILE and
existing handling for symbolic links and directories.
---
Nitpick comments:
In `@cli/src/command/diff.rs`:
- Around line 319-345: In the DataKind match within the diff handling logic,
remove the redundant DataKind::FILE | DataKind::DIRECTORY |
DataKind::SYMBOLIC_LINK arm and the fallback DataKind::HARD_LINK arm. Retain the
specialized DataKind::HARD_LINK if meta.is_file() handling and the existing _
wildcard arm for all remaining cases.
In `@lib/src/entry/options.rs`:
- Around line 1561-1567: Extend compression_known_constants_map_to_spec_bytes to
assert the ordering around the reserved byte: verify the value corresponding to
reserved byte 3 sorts before Compression::XZ (byte 4). Preserve the existing
assertions for the known constants and use the enum’s ordering behavior
directly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02e0fa17-8092-4bf3-aa41-492d841b0b3a
📒 Files selected for processing (37)
cli/src/cli.rscli/src/command/bsdtar.rscli/src/command/chmod.rscli/src/command/core.rscli/src/command/diff.rscli/src/command/extract.rscli/src/command/list.rscli/src/command/verify.rscli/tests/cli/bsdtar/option_update_strip_components.rscli/tests/cli/bsdtar/symlink.rscli/tests/cli/create/empty_entries.rscli/tests/cli/create/symlink.rscli/tests/cli/create/symlink_metadata.rscli/tests/cli/delete/directory_entry.rscli/tests/cli/delete/symlink_entry.rscli/tests/cli/extract/option_keep_permission.rscli/tests/cli/update/symlink_entry.rscli/tests/cli/utils/archive.rsfuzz/fuzz_targets/aes_cbc.rsfuzz/fuzz_targets/aes_ctr.rsfuzz/fuzz_targets/camellia_cbc.rsfuzz/fuzz_targets/camellia_ctr.rsfuzz/fuzz_targets/split_archive.rslib/benches/create_extract.rslib/examples/change_compression_method.rslib/src/archive.rslib/src/entry.rslib/src/entry/builder.rslib/src/entry/content.rslib/src/entry/header.rslib/src/entry/options.rslib/src/entry/read.rslib/src/lib.rslib/tests/extract_compatibility.rslib/tests/extract_multipart_compatibility.rslib/tests/extract_solid_compatibility.rsxtask/src/main.rs
1 participant
Summary
Replace the
DataKind,Compression,Encryption, andCipherModeenums with byte-backed newtypes (pub struct X(u8)+ associated constants). The open-enum shape allowed constructing aliased values such asDataKind::Reserved(0)(same wire byte asFilebut unequal), and assigning a new known value would silently change the meaning of existingReserved(n)matches; with the newtype every byte value has a single canonical representation and future assignments are non-breaking.Notes
DataKind::FILE, …). Old variant spellings remain available as#[deprecated]constants where the spelling changed;CipherMode::CBC/CTRandCompression::XZare unchanged.TryFrom<u8>is kept (behavior unchanged, still infallible) and shadowed by a deprecated inherenttry_from, soX::try_from(b)call sites get a deprecation warning; both will be removed together in a future release.from_byteis the successor.Reserved(v)/Private(v)pattern matches no longer compile; use a wildcard arm withis_reserved()/is_private().Debugoutput is unchanged.Ordnow follows the byte value; the only observable difference isCompression's unassigned byte 3 now ordering beforeXZ(4).new_reserved(with "unassigned → Some" semantics its result would silently change across versions once a value is assigned; with "any value < 128 → Some" semanticsnew_reserved(0)would returnFILE— the reserved range is the specification's namespace) andunsafe new_unchecked(every byte is a valid canonical value, so there is no safety invariant forunsafeto guard; unchecked total construction is exactlyfrom_byte).Summary by CodeRabbit
New Features
Bug Fixes
Documentation