Repository navigation
fix(extract): honor --check-crc/--no-check-crc on the BGZF FASTQ split decode - #959
Conversation
…t decode The all-file BGZF parallel-decode split for FASTQ input (build_bgzf_fastq_split -> FastqDecompress) reads its per-block CRC32 policy from ChainSpec.verify_crc, but Extract::execute_chain hardcoded that field to false. As a result --check-crc and the file-input default silently skipped CRC verification on that path, accepting corrupted FASTQ blocks. This path shipped to main with the StepK FASTQ decode work (via #952, merged into #951). Resolve verify_crc through the shared resolve_check_crc policy (--check-crc wins, --no-check-crc disables, file inputs verify by default). The spec construction is extracted into a testable Extract::build_extract_chain_spec, mirroring Sort::build_sort_chain_spec. test_bgzf_fastq_honors_check_crc did not catch this: its small input is read in full during quality-encoding detection (sample_detection_quals), which opens its own policy-honoring reader and rejects the corrupted trailing block before the split decoder runs. Add split_decoder_honors_check_crc_past_detection_window, which places the corruption past the detection window so only the split decode reaches it, exercising the decoder's own policy. Also align the parse_and_zip_two_streams_preserves_ordinal doc with what it actually covers (the primitives, not the step; step wiring is covered by the chain tests).
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: 0 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughThe extract path no longer hardcodes CRC verification off. It resolves the effective policy, passes it to ChangesExtract CRC policy
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant ExtractCLI
participant build_extract_chain_spec
participant ChainSpec
participant execute_chain
participant BGZF_split_decoder
ExtractCLI->>build_extract_chain_spec: Resolve CRC policy
build_extract_chain_spec->>ChainSpec: Set verify_crc
ExtractCLI->>execute_chain: Execute chain
execute_chain->>BGZF_split_decoder: Decode with CRC policy
Merge Risk: ⚪ Minimal · up to CRC verification is now applied according to the documented command policy for eligible file inputs, including blocks decoded after initial quality detection. No actionable merge risk remains. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #959 +/- ##
==========================================
+ Coverage 96.02% 96.05% +0.03%
==========================================
Files 290 290
Lines 143501 143507 +6
==========================================
+ Hits 137796 137851 +55
+ Misses 5705 5656 -49 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
This branch was successfully deployed
1 participant
Summary
The all-file BGZF parallel-decode split for FASTQ input (
build_bgzf_fastq_split→FastqDecompress) reads its per-block CRC32 policy fromChainSpec.verify_crc, butExtract::execute_chainhardcoded that field tofalse. As a result--check-crcand the file-input default silently skipped CRC verification on that path, accepting corrupted FASTQ blocks. This path shipped tomainwith the StepK FASTQ decode work (via #952, merged into #951).CodeRabbit flagged this on #951 (
builder.rs:889), but the thread was resolved as "stale / symbols removed" — the symbols do exist onmain, and the bug is real.Fix
Resolve
verify_crcthrough the sharedresolve_check_crcpolicy (--check-crcwins,--no-check-crcdisables, file inputs verify by default). The spec construction is extracted into a testableExtract::build_extract_chain_spec, mirroringSort::build_sort_chain_spec.Why the existing test missed it
test_bgzf_fastq_honors_check_crcuses a small input that is read in full during quality-encoding detection (sample_detection_quals), which opens its own policy-honoring reader and rejects the corrupted trailing block before the split decoder ever runs. The newsplit_decoder_honors_check_crc_past_detection_windowplaces the corruption past the detection window so only the full split decode reaches it, exercising the decoder's own policy (verified failing before the fix, passing after).Also
Aligns the
parse_and_zip_two_streams_preserves_ordinaldoc with what it actually covers (a second CodeRabbit finding on #951 resolved as stale — also still valid onmain): it exercises the parse/zip primitives, not the step; step wiring is covered by the chain tests.Verification
cargo ci-fmt,cargo ci-lint,cargo ci-tag-literals,cargo ci-publish-order, and the affected tests all pass locally.Risk: command output changes: none;
unsafechanges: none, so no CLAUDE.md allowlist update is required; memory bounds, queue capacity, and thread/backpressure policy: none.Fix: Resolve
ChainSpec.verify_crcthroughresolve_check_crc.--check-crcenables verification,--no-check-crcdisables it, and file inputs verify by default.Extract::build_extract_chain_spec.parse_and_zip_two_streams_preserves_ordinaldocumentation.