fix(runall): honor --extract::no-check-crc on BGZF FASTQ input - #996
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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: Repository: fulcrumgenomics/fgumi/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. 0 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 1 review per hour. WalkthroughFASTQ BGZF split decoding now uses CRC policy resolved from extract options rather than ChangesFASTQ CRC policy
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The runall FASTQ path follows the requested CRC policy, and the added tests cover both disabled-check acceptance and default rejection. No actionable merge-readiness risk remains. 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Comment |
1025773 to
948c232
Compare
|
@coderabbitai pause |
✅ Action performedReviews paused. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #996 +/- ##
==========================================
- Coverage 96.29% 96.27% -0.02%
==========================================
Files 296 296
Lines 149023 149032 +9
==========================================
- Hits 143496 143475 -21
- Misses 5527 5557 +30 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
948c232 to
16ac4c9
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
The all-file BGZF FASTQ parallel-decode split (build_bgzf_fastq_split) took its CRC32 policy from ChainSpec::verify_crc, while the fused FASTQ readers resolved theirs from the extract options' check_crc / no_check_crc. runall set verify_crc to a hardcoded true for a FASTQ source, believing it inert, so under --extract::no-check-crc the split aborted with a CRC mismatch while the run logged "CRC verify: off"; standalone extract only worked because it resolved the field a second time itself. Resolve the split's policy in the builder from the same extract options the fused readers use, so no FASTQ command can leave the two decode fronts out of sync. ChainSpec::verify_crc is now genuinely inert for a FASTQ source, and extract's own copy of the resolution is removed. The default policy (verify a file, skip stdin) is unchanged.
16ac4c9 to
fd74432
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
Summary
fgumi runall --start-from extract --extract::no-check-crcaborted with a BGZF CRC32 mismatch on all-BGZF FASTQ file input — while loggingCRC verify: off— where standalonefgumi extract --no-check-crcsucceeds.The all-file BGZF FASTQ parallel-decode split (
build_bgzf_fastq_split) took its CRC policy fromChainSpec::verify_crc, while the fused FASTQ readers resolved theirs from the extract options'check_crc/no_check_crc. runall setverify_crcto a hardcodedtruefor a FASTQ source (commented as inert); standalone extract only worked because it resolved the field a second time itself.The split's policy is now resolved once in
open_fastq_sourcefrom the same extract options the fused readers use (via the sharedresolve_check_crc) and carried inPendingSource::Fastq, so no FASTQ command can leave the two decode fronts out of sync.ChainSpec::verify_crcis now genuinely inert for FASTQ sources (doc corrected), extract's redundant resolution is removed, andopen_fastq_readeruses the sharedresolve_check_crc/check_crc_reasoninstead of inline copies. The settled default policy (verify a file, skip trusted stdin; #786/#798) is unchanged.Found by a sweep for siblings of #991. Blocks v0.8.0.
Test plan
--extract::no-check-crcwith every record present in order and identical to standalone extract, and rejected by default with the mismatch raised byFastqDecompress. Fails without the builder change, as does the existing standalone split test.-istill reports--input is required.cargo ci-fmt,ci-lint,ci-doc,ci-test(10474 passed),ci-doctest.Risk: Command output data: none; CRC rejection behavior changes for BGZF FASTQ, and integration tests pin record names, order, and parity with standalone
extract.unsafe: none added or modified; CLAUDE.md’s allowlist does not need an update. Memory bounds, queue capacity, and thread/backpressure policy: none.Fix: BGZF FASTQ split decoding now uses the CRC policy resolved from extract options. This aligns
runallwith fused FASTQ readers. The default CRC policy remains unchanged.Coverage: Added integration tests for corrupted BGZF FASTQ with CRC checks disabled and enabled, and for missing input when starting from
sort. Test execution results were not provided.