fix(bam): refuse whitespace in read names and framing bytes in tag values (#415) - #426
Merged
Conversation
…lues (#415) A uBAM whose QNAME contained whitespace was accepted, and the read-name tail plus every --preserve-tags aux tag were silently discarded: bam_record_to_fastq builds the FASTQ id as `@{QNAME}\tTAG:TYPE:VALUE`, and parse_name_and_data re-splits on the first whitespace, so a space in the name captured the tag tail. The check is read-side, at the single BAM->FASTQ conversion point, so it also refuses FASTQ-output runs where no tag is currently lost. Narrowing it to uBAM output was ruled out on evidence rather than declined: FastqRecord::write_to writes the id verbatim then a newline, so an LF in a read name emits a five-line record and desynchronises every subsequent record in the file. An output-format gate would have closed a lost-tag hole and left a lost-file hole. A character class split was priced and declined because input validity must not depend on an output flag. The reject set is u8::is_ascii_whitespace: the bytes parse_name_and_data, fastq::read_id_prefix and the id's one-line framing treat as boundaries. VT sits outside it by decision, not derivation -- append_to_id's trim_end uses Unicode White_Space and does strip a trailing one, but rejecting VT read-side would over-reject files that work today. `@` and non-ASCII are accepted for the same reason; both still fail at noodles' record encoder on uBAM output, which is pre-existing and out of scope here. Aux tag values get the same treatment through a second predicate -- the QNAME set minus space, which Z's `[ !-~]*` grammar makes legal. The scan runs over whatever append_tag_type_and_value emitted rather than inside one match arm, so the A (Character) type is covered too: an A value holding a newline reproduced the same corruption signature, and on uBAM output Trim Galore itself emitted a spec-invalid A value. Known and deliberately not cured here: a mid-stream refusal can leave a partial output file, and the partial uBAM is valid, BGZF-EOF-terminated and samtools-readable. The writer opens before the read loop, so this predates #415 and already applies to the four other per-record bails; a test asserts the residue rather than wishing it away. Tracked separately. Fixtures that SAM text cannot express -- a newline inside a QNAME or a tag value -- are packed by hand in examples/mk_bam_fixture.rs, with only the BGZF framing from noodles. noodles' record encoder rejects such names; its bgzf::Writer does not validate, so no external generator is needed. All eight fixtures regenerate byte-identically from the recipes in test_files/README.md. Tests: 643 -> 663 (8 unit, 12 integration), each naming the entry point it pins. Three of the four entry points into the conversion point run before the trimming loop, so fixtures place the offender past record 1; otherwise the refusal fires at the sanity check and the writer-residue path stays untested.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A uBAM whose read name contained whitespace was accepted, and the read-name tail plus every
--preserve-tagsaux tag were silently discarded — the FASTQ id is built as@{QNAME}\tTAG:TYPE:VALUEand re-split on the first whitespace, so a space in the name captured the tag tail. This refuses such records at the single BAM→FASTQ conversion point, which means it also refuses FASTQ-output runs that previously succeeded: a newline in a read name makes one record occupy five lines and desynchronises every record after it, so gating the check on output format would have closed a lost-tag hole and left a lost-file hole. Aux tag values get the same treatment through a second predicate (the read-name set minus space, whichZ's grammar makes legal), applied to whatever the tag emitter wrote so theAtype is covered too. A mid-stream refusal can still leave a partial output file — that predates this change and applies to four other per-record bails, so it is asserted by a test rather than cured here, and tracked separately. 643 → 663 tests.Addresses #415.
AI-assisted detail
Why the check is read-side rather than gated on
--output-format ubam. The issue describes tag loss, which only happens on the uBAM-output path. ButFastqRecord::write_towrites the id verbatim followed by a newline, so an LF inside a read name emits a five-line record — measured before the fix as 13 output lines for a 3-record input, at exit 0. That lands on the FASTQ-output path, where the reported defect does not reach. A character-class split (control bytes always, space/tab only for uBAM output) was priced and declined: input validity should not be discovered by changing an output flag.The reject set and why VT is excluded.
u8::is_ascii_whitespace— the bytesparse_name_and_data,fastq::read_id_prefixand the id's one-line framing treat as boundaries. VT (0x0B) is outside Rust's ASCII-whitespace set and is accepted by decision, not derivation:append_to_id'strim_enduses UnicodeWhite_Spaceand does strip a trailing one under--renameplus a clip flag. Rejecting VT read-side would over-reject files that work today, which is the same reasoning that admits@and non-ASCII. Those two still fail at noodles' record encoder on uBAM output with an opaque message — pre-existing, and worth its own issue.The
A(Character) tag arm. The scan runs over whateverappend_tag_type_and_valueappended rather than inside one match arm. AnAvalue holding a newline reproduced the same corruption signature as theZcase, and on uBAM output Trim Galore itself emitted a spec-invalidAvalue — so--preserve-tagscould produce malformed BAM, not merely malformed FASTQ.Why the fixtures place the offender past record 1. Four entry points feed the conversion point and three run before the trimming loop: the sanity-check peek, adapter auto-detection (≤1M records) and the poly-G scan (≤1M). A single-record fixture is therefore always caught before any writer exists, so "nothing was written" holds vacuously and the writer-residue path stays untested. The cases run with
-a … --no_poly_gto skip both pre-scans, which is the arm any file over 1M reads takes anyway. Each test names the entry point it pins.The partial-output residue. The writer is created before the read loop, so a refusal past the pre-scan window leaves a partial file — and for uBAM the partial is valid, BGZF-EOF-terminated and samtools-readable, so nothing downstream flags it. The test asserts the EOF marker rather than mere non-emptiness, because indistinguishability from a complete BAM is what makes it a data-integrity problem. Not cured here: it applies equally to the aligned / reverse / secondary / supplementary bails, so the fix belongs across all five.
Fixtures. A newline inside a QNAME or a tag value cannot be expressed in SAM text, so samtools cannot build these; noodles' record encoder rejects the names too. But
bgzf::Writerdoes no validation, so the records are packed by hand inexamples/mk_bam_fixture.rswith only the framing from noodles — no external generator needed. All eight fixtures regenerate byte-identically from the recipes intest_files/README.md, and samtools reads all of them back with the expected records.