Repository navigation
fix(fastq): accept CRLF and unterminated final records (W9b: EXT3-06/07) - #554
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughFASTQ parsing now normalizes CRLF fields, accepts complete final records without a trailing newline, preserves incomplete chunks for reassembly, and rejects malformed or quality-less records with ChangesFASTQ parsing behavior
Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #554 +/- ##
==========================================
+ Coverage 92.84% 92.91% +0.06%
==========================================
Files 166 166
Lines 102064 102468 +404
==========================================
+ Hits 94765 95207 +442
+ Misses 7299 7261 -38 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
d3355ff to
dfc4ec5
Compare
dfc4ec5 to
08b875a
Compare
08b875a to
d24d594
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
d24d594 to
f9cdafc
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/fastq_parse.rs`:
- Around line 277-284: The no-trailing-newline branch in parse_fastq_records
must not treat an incremental chunk’s remaining quality bytes as a complete
record. Update the parser and its interaction with add_bytes_for_stream so this
path is accepted only when the input is known to be final, otherwise preserve
and return the bytes as leftover for the next chunk; retain normal
newline-terminated parsing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90428a63-fceb-4564-a646-f66002f3fb44
📒 Files selected for processing (3)
crates/fgumi-simd-fastq/src/parser.rscrates/fgumi-simd-fastq/src/reader.rssrc/lib/fastq_parse.rs
… (EXT3-06/07) Both FASTQ parsers (`fgumi-simd-fastq` — extract's path — and `fastq_parse`, the grouper/unified-pipeline path) split records on `\n` only: - **EXT3-06 (CRLF):** on a `\r\n`-terminated file the `\r` before each `\n` was left as the last byte of the name, sequence, and quality. A `\r` (byte 13) in the quality is below the printable-ASCII floor, so `extract` rejected the whole file with "Invalid quality scores range [13, …]". Strip one trailing `\r` per field (matching fgbio's `Source.getLines`), and compare CRLF-stripped lengths in the seq/qual validation so an unterminated final CRLF record is not spuriously rejected. - **EXT3-07 (no trailing newline):** `SimdFastqReader` errored "Truncated FASTQ record at EOF" on a final record lacking its terminating `\n`, even though `from_slice` already accepted it — the two paths disagreed. Yield the leftover when it is a complete record (starts with `@`, has the three internal newlines); only a genuinely short leftover (< 3 newlines) still errors. Matches fgbio's `lines.take(4)`. Real-tool repro: a CRLF FASTQ and a newline-less-final-record FASTQ that `fgumi extract` before rejected (error, 0 records) now yield 2 records each, name/seq/qual byte-identical to `fgbio FastqToBam`. RED-first unit tests in both crates.
f9cdafc to
8a79547
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
What / why
Both FASTQ parsers split records on
\nonly, so two valid inputs thatfgbio FastqToBamaccepts were rejected by fgumi:\r\n-terminated file the\rbefore each\nwas left as the last byte of the name, sequence, and quality. A\r(byte 13) in the quality is below the printable-ASCII floor, sofgumi extractrejected the whole file:Invalid quality scores detected: range [13, …].SimdFastqReadererroredTruncated FASTQ record at EOFon a final record lacking its terminating\n, even though the siblingfrom_sliceparser already accepted it — the two paths disagreed (this is the exact inconsistency the audit flagged:reader.rsvsfastq_parse.rs).The fix, applied to both the
fgumi-simd-fastqcrate (extract's path) andsrc/lib/fastq_parse.rs(the grouper / unified-pipeline path):\rper field (strip_trailing_cr), matching fgbio'sSource.getLines, and compare CRLF-stripped lengths in the seq/qual validation so an unterminated final CRLF record isn't spuriously rejected.@, does not end in\n, has exactly the three internal newlines) instead of erroring; a genuinely short/malformed leftover still errors. Matches fgbio'slines.take(4).Verification (§0)
Real-tool repro — a CRLF FASTQ and a newline-less-final-record FASTQ:
\nfgumi-after name/sequence/quality are byte-identical to
fgbio FastqToBamon the CRLF input. RED-first unit tests in both crates.Reviewers
Both ran. In-diff fixes folded by amend:
@n\nseq\n+\n— a newline-terminated+line with the quality line absent — which has three internal newlines but an empty quality span, panickingparse_single_recordwithslice index starts at 11 but ends at 10. Addedleftover.last() != Some(&b'\n')(a genuine unterminated record ends with quality bytes, never\n) + a RED-first regression test that reproduces the panic.from_slicedoc — the trailing newline is optional and CRLF is accepted.cargo ci-fmt && ci-lint && ci-testgreen (2234 tests).Scope
W9b of the extract/FASTQ residuals. Stacked on #512 (
nh/fix-fastqgrouper-readname-sync, which also editsfastq_parse.rs) — auto-retargets to main on its merge. Remaining W9: FASTQ3-04 (suffix strip, main-based) and the FASTQ3-02/03 product decisions; EXT3-03 deferred on #496.Summary by CodeRabbit
at_eofflag to control how an unterminated final quality line is handled.\rno longer appears in parsed name, sequence, or quality fields.