From a3685581c387cec2e420f92a403418b7b6c7001d Mon Sep 17 00:00:00 2001 From: Felix Krueger Date: Wed, 12 Aug 2026 18:24:14 +0200 Subject: [PATCH] fix(bam): refuse whitespace in read names and framing bytes in tag values (#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. --- CHANGELOG.md | 5 + docs/src/content/docs/quickstart.md | 14 ++ examples/mk_bam_fixture.rs | 131 ++++++++++ src/bam.rs | 160 ++++++++++++- test_files/README.md | 58 +++++ test_files/ubam_atag_ok.bam | Bin 0 -> 125 bytes test_files/ubam_lf_atag.bam | Bin 0 -> 125 bytes test_files/ubam_lf_qname.bam | Bin 0 -> 140 bytes test_files/ubam_lf_tagvalue.bam | Bin 0 -> 135 bytes test_files/ubam_tagvalue_space.bam | Bin 0 -> 157 bytes test_files/ubam_ws_qname.bam | Bin 0 -> 162 bytes test_files/ubam_ws_qname_late.bam | Bin 0 -> 181 bytes test_files/ubam_ws_qname_paired.bam | Bin 0 -> 184 bytes tests/integration_clump_only_ubam.rs | 31 +++ tests/integration_ubam.rs | 345 +++++++++++++++++++++++++++ 15 files changed, 742 insertions(+), 2 deletions(-) create mode 100644 examples/mk_bam_fixture.rs create mode 100644 test_files/ubam_atag_ok.bam create mode 100644 test_files/ubam_lf_atag.bam create mode 100644 test_files/ubam_lf_qname.bam create mode 100644 test_files/ubam_lf_tagvalue.bam create mode 100644 test_files/ubam_tagvalue_space.bam create mode 100644 test_files/ubam_ws_qname.bam create mode 100644 test_files/ubam_ws_qname_late.bam create mode 100644 test_files/ubam_ws_qname_paired.bam diff --git a/CHANGELOG.md b/CHANGELOG.md index 045433f5..cef467ed 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,11 @@ #### Changes +- **A BAM read name containing whitespace, or whitespace other than a space inside a preserved + aux-tag value, is now refused on every output format** + ([#415](https://github.com/FelixKrueger/TrimGalore/issues/415)) — see the uBAM section of the + docs for the samtools recipe that rewrites space-bearing names. + - **A paired run now writes every output to one directory** ([#398](https://github.com/FelixKrueger/TrimGalore/issues/398)). Without `--output_dir`, the validated FASTQs already went to Read 1's directory, but each diff --git a/docs/src/content/docs/quickstart.md b/docs/src/content/docs/quickstart.md index a9ab1d24..f044c960 100644 --- a/docs/src/content/docs/quickstart.md +++ b/docs/src/content/docs/quickstart.md @@ -67,6 +67,20 @@ trim_galore --output-format ubam --preserve-tags CB,UB sample.bam Paired uBAM output is a single interleaved BAM (`_val.bam`), matching samtools/Picard/fgbio convention. See [Output files](/guide/outputs/#ubam-output---output-format-ubam) for the full contract. +### Read names containing whitespace + +A read name containing whitespace is refused, **on every output format** — including FASTQ output, where a space or tab loses nothing today but a newline silently desynchronises the whole file. Trim Galore reads a FASTQ header as ending at the first whitespace, so an aux-tag tail after it cannot be carried, and a newline splits one record across five lines. Whitespace other than a space inside a preserved aux-tag value is refused for the same reason; a space in a tag value is legal and unaffected. + +The SAM specification forbids whitespace in QNAME, but samtools will build such a file. For names containing spaces only, samtools can rewrite them: + +```bash +samtools view -h in.bam \ + | awk 'BEGIN{FS=OFS="\t"} /^@/{print; next} {gsub(/ /,"_",$1); print}' \ + | samtools view -b -o fixed.bam - +``` + +`FS="\t"` is essential. Without it `$1` stops at the first space, the substitution matches nothing, and the file comes out unchanged — `samtools view -b` accepts it and Trim Galore still refuses it. A name containing a tab or newline cannot be represented in SAM text at all, so it has to be corrected at source. + ## Common combinations ```bash diff --git a/examples/mk_bam_fixture.rs b/examples/mk_bam_fixture.rs new file mode 100644 index 00000000..3d977cca --- /dev/null +++ b/examples/mk_bam_fixture.rs @@ -0,0 +1,131 @@ +//! Emit the uBAM fixtures that SAM text cannot express (#415). +//! +//! BAM stores QNAME and a `Z` tag value as NUL-terminated byte fields, so either can hold a +//! newline; SAM text is line-delimited and cannot, so samtools refuses to build these. noodles' +//! record encoder rejects any read name outside `[!-?A-~]{1,254}`, so the records are packed +//! here by hand and only the BGZF framing comes from `bgzf::Writer`. +//! +//! ```sh +//! cargo run --quiet --example mk_bam_fixture -- lf_qname > test_files/ubam_lf_qname.bam +//! ``` + +use std::io::Write; + +use noodles::bgzf; + +const SEQ: &str = "ACGTACGTACGTACGTACGTACGTACGTACGTACGTACGT"; +const QUAL_PHRED: u8 = 40; +const FLAG_UNMAPPED: u16 = 4; + +/// 4-bit sequence alphabet, indexed by the packed nibble value. +const SEQ_CODES: &[u8] = b"=ACMGRSVTWYHKDBN"; + +const CASES: &[&str] = &["lf_qname", "lf_tagvalue", "lf_atag", "atag_ok"]; + +/// Two bases per byte, high nibble first. +fn pack_seq(seq: &str) -> Vec { + let code = |b: u8| { + SEQ_CODES + .iter() + .position(|&c| c == b) + .unwrap_or_else(|| panic!("{} is not a IUPAC base", b as char)) as u8 + }; + seq.as_bytes() + .chunks(2) + .map(|pair| (code(pair[0]) << 4) | pair.get(1).map_or(0, |&b| code(b))) + .collect() +} + +/// One aux field: two-char tag, `Z`, NUL-terminated value. +fn z_tag(tag: &str, value: &[u8]) -> Vec { + let mut out = tag.as_bytes().to_vec(); + out.push(b'Z'); + out.extend_from_slice(value); + out.push(0); + out +} + +/// One aux field: two-char tag, `A`, a single unterminated byte. +fn a_tag(tag: &str, byte: u8) -> Vec { + let mut out = tag.as_bytes().to_vec(); + out.push(b'A'); + out.push(byte); + out +} + +/// One unmapped BAM record: `block_size` then the 32-byte fixed block, name, seq, qual, aux. +fn record(name: &[u8], aux: &[u8]) -> Vec { + let mut name_z = name.to_vec(); + name_z.push(0); + + let mut body = Vec::new(); + body.extend_from_slice(&(-1i32).to_le_bytes()); // ref_id + body.extend_from_slice(&(-1i32).to_le_bytes()); // pos + body.push(u8::try_from(name_z.len()).expect("read name fits in l_read_name")); + body.push(0); // mapq + body.extend_from_slice(&4680u16.to_le_bytes()); // bin + body.extend_from_slice(&0u16.to_le_bytes()); // n_cigar_op + body.extend_from_slice(&FLAG_UNMAPPED.to_le_bytes()); + body.extend_from_slice(&(SEQ.len() as i32).to_le_bytes()); // l_seq + body.extend_from_slice(&(-1i32).to_le_bytes()); // next_ref_id + body.extend_from_slice(&(-1i32).to_le_bytes()); // next_pos + body.extend_from_slice(&0i32.to_le_bytes()); // tlen + body.extend_from_slice(&name_z); + body.extend_from_slice(&pack_seq(SEQ)); + body.extend_from_slice(&vec![QUAL_PHRED; SEQ.len()]); + body.extend_from_slice(aux); + + let mut out = (body.len() as i32).to_le_bytes().to_vec(); + out.extend_from_slice(&body); + out +} + +/// The offending record sits second, so record 1 clears the sanity-check peek and the +/// failure lands in the trimming loop. +fn case_records(case: &str) -> Option, Vec)>> { + let clean_cb = || z_tag("CB", b"AAACCC"); + Some(match case { + "lf_qname" => vec![ + (b"readA".to_vec(), clean_cb()), + (b"readB\nEVIL".to_vec(), clean_cb()), + (b"readC".to_vec(), clean_cb()), + ], + "lf_tagvalue" => vec![ + (b"readA".to_vec(), clean_cb()), + (b"readB".to_vec(), z_tag("CB", b"AAA\nCCC")), + (b"readC".to_vec(), clean_cb()), + ], + "lf_atag" => vec![ + (b"readA".to_vec(), a_tag("XA", b'+')), + (b"readB".to_vec(), a_tag("XA", b'\n')), + (b"readC".to_vec(), a_tag("XA", b'+')), + ], + // The acceptance twin for the `A` arm: nothing here may be refused. + "atag_ok" => vec![ + (b"readA".to_vec(), a_tag("XA", b'+')), + (b"readB".to_vec(), a_tag("XA", b'-')), + (b"readC".to_vec(), a_tag("XA", b'+')), + ], + _ => return None, + }) +} + +fn main() -> std::io::Result<()> { + let case = std::env::args().nth(1).unwrap_or_default(); + let Some(records) = case_records(&case) else { + eprintln!("usage: mk_bam_fixture <{}>", CASES.join("|")); + std::process::exit(2); + }; + + let header_text = b"@HD\tVN:1.6\n"; + let mut writer = bgzf::Writer::new(std::io::stdout().lock()); + writer.write_all(b"BAM\x01")?; + writer.write_all(&(header_text.len() as i32).to_le_bytes())?; + writer.write_all(header_text)?; + writer.write_all(&0i32.to_le_bytes())?; // n_ref + for (name, aux) in &records { + writer.write_all(&record(name, aux))?; + } + let _stdout = writer.finish()?; + Ok(()) +} diff --git a/src/bam.rs b/src/bam.rs index af6dda19..b53ee134 100644 --- a/src/bam.rs +++ b/src/bam.rs @@ -862,6 +862,49 @@ pub fn peek_header(path: &Path) -> Result
{ Ok(header) } +/// Reject a BAM read name holding a byte that breaks the FASTQ id it becomes. +/// +/// The chosen set is `u8::is_ascii_whitespace` — what `parse_name_and_data`, +/// `fastq::read_id_prefix` and the id's one-line framing split on. VT sits deliberately +/// outside it; `append_to_id`'s `trim_end` does strip a trailing one. +fn reject_whitespace_in_qname(name: &[u8]) -> Result<()> { + if name.iter().any(u8::is_ascii_whitespace) { + let shown = String::from_utf8_lossy(name); + bail!( + "BAM read name contains whitespace: \"{}\". Trim Galore reads a FASTQ header as \ + ending at the first whitespace, so any --preserve-tags aux tags after it are lost \ + on uBAM output, and a newline would break the FASTQ record structure. The SAM \ + specification forbids whitespace in QNAME. Rename the reads before trimming — the \ + uBAM section of the docs gives a samtools recipe for space-only names; a tab or \ + newline in a name cannot be expressed in SAM text at all.", + shown.escape_debug() + ); + } + Ok(()) +} + +/// Reject bytes in an appended `TYPE:VALUE` that would break the FASTQ id holding it. +/// +/// The QNAME boundary set minus space, which `Z`'s `[ !-~]*` grammar makes legal: a tab +/// would forge a tag field and a newline would break the single-line id. +fn reject_framing_in_tag_value(text: &str) -> Result<()> { + if text.bytes().any(|b| b != b' ' && b.is_ascii_whitespace()) { + // `Z` values are unbounded, so borrow the 60-char cap the description notice uses. + let shown = if text.chars().count() > 60 { + format!("{}…", text.chars().take(60).collect::()) + } else { + text.to_string() + }; + bail!( + "aux tag value cannot pass through the FASTQ header: \"{}\". A tab would forge a \ + tag field and a newline would break the single-line read ID; spaces are legal and \ + unaffected. Fix the tag at source.", + shown.escape_debug() + ); + } + Ok(()) +} + /// Convert one BAM record to a `FastqRecord`. Per-record validation: /// - Must be unmapped (`is_unmapped()` true; per-record check resolves /// PLAN-REVIEW B-Crit-4 first-record-only contradiction). @@ -894,6 +937,7 @@ fn bam_record_to_fastq(rec: &bam::Record, tags: &[String]) -> Result Result = OnceLock::new(); SEEN.get_or_init(|| { @@ -1044,8 +1092,12 @@ fn emit_description_dropped_once(dropped: &str) { } /// Format `{TYPE}:{VALUE}` for one BAM aux field, matching `samtools fastq -T`. +/// +/// Every arm's output is scanned before returning, so no type code can route text into the +/// FASTQ id unchecked. fn append_tag_type_and_value(out: &mut String, value: &Value<'_>) -> Result<()> { use std::fmt::Write; + let start = out.len(); match value { Value::Character(c) => write!(out, "A:{}", *c as char).unwrap(), Value::Int8(v) => write!(out, "i:{}", v).unwrap(), @@ -1080,7 +1132,7 @@ fn append_tag_type_and_value(out: &mut String, value: &Value<'_>) -> Result<()> ); } } - Ok(()) + reject_framing_in_tag_value(&out[start..]) } #[cfg(test)] @@ -1406,6 +1458,110 @@ mod tests { ); } + #[test] + fn qname_boundary_bytes_rejected() { + for (label, name) in [ + ("space", b"read one".as_slice()), + ("tab", b"read\tone"), + ("LF", b"read\none"), + ("CR", b"read\rone"), + ("FF", b"read\x0Cone"), + ] { + assert!( + reject_whitespace_in_qname(name).is_err(), + "{} must be rejected in a QNAME", + label + ); + } + } + + #[test] + fn qname_non_boundary_bytes_accepted() { + // VT is accepted by decision, not by derivation: widening the set here would + // over-reject files that work today. + for (label, name) in [ + ("plain", b"read_one".as_slice()), + ("VT", b"read\x0Bone"), + ("at-sign", b"read@one"), + ("non-ASCII", "read\u{e9}one".as_bytes()), + ] { + assert!( + reject_whitespace_in_qname(name).is_ok(), + "{} must be accepted in a QNAME", + label + ); + } + } + + #[test] + fn qname_error_escapes_invisible_bytes() { + let err = reject_whitespace_in_qname(b"read\tone").expect_err("tab must be rejected"); + let msg = format!("{:#}", err); + assert!( + msg.contains("read\\tone"), + "error must show the offending byte escaped, got: {}", + msg + ); + } + + #[test] + fn tag_value_framing_bytes_rejected() { + for (label, value) in [ + ("tab", "AAA\tCCC"), + ("LF", "AAA\nCCC"), + ("CR", "AAA\rCCC"), + ("FF", "AAA\x0CCCC"), + ] { + assert!( + reject_framing_in_tag_value(value).is_err(), + "{} must be rejected in a Z tag value", + label + ); + } + } + + #[test] + fn tag_value_space_accepted() { + // `Z` is `[ !-~]*`, so a space is legal — and the tab introducing the tag + // precedes it, leaving `parse_name_and_data` able to split the tail. + assert!(reject_framing_in_tag_value("has a space").is_ok()); + } + + #[test] + fn read_side_emitter_rejects_newline_in_character_tag() { + // `A` writes its raw byte into the same id as `Z`, so it needs the same guard. + let value = Value::Character(b'\n'); + let mut buf = String::new(); + let err = append_tag_type_and_value(&mut buf, &value) + .expect_err("a newline in an A value must be rejected by the read-side emitter"); + assert!( + format!("{:#}", err).contains("cannot pass through the FASTQ header"), + "expected the framing rejection, got: {:#}", + err + ); + } + + #[test] + fn read_side_emitter_accepts_printable_character_tag() { + let mut buf = String::new(); + append_tag_type_and_value(&mut buf, &Value::Character(b'+')).unwrap(); + assert_eq!(buf, "A:+"); + } + + #[test] + fn read_side_emitter_rejects_newline_in_string_tag() { + let value = Value::String(bstr::BStr::new(b"AAA\nCCC")); + let mut buf = String::new(); + let err = append_tag_type_and_value(&mut buf, &value) + .expect_err("a newline in a Z value must be rejected by the read-side emitter"); + let msg = format!("{:#}", err); + assert!( + msg.contains("AAA\\nCCC"), + "expected the escaped value in the message, got: {}", + msg + ); + } + #[test] fn parse_unknown_type_code_rejected() { let err = parse_name_and_data("@read1\tXX:Q:foo").unwrap_err(); diff --git a/test_files/README.md b/test_files/README.md index df8351b8..6f877f94 100644 --- a/test_files/README.md +++ b/test_files/README.md @@ -52,6 +52,64 @@ samtools flagstat test_files/ubam_paired_test.bam # 20 total, 20 unmapped, 10 re Provenance: SRR24827378 (RRBS). See `BS-seq_10K_R{1,2}.fastq.gz` for the source. +## Malformed-read-name uBAM fixtures (#415) + +Eight fixtures for the whitespace refusal, used by `tests/integration_ubam.rs` +and `tests/integration_clump_only_ubam.rs`. In the multi-record fixtures the +offending record sits **after the first record or pair**, so record 1 clears the +sanity-check peek and the failure lands in the trimming loop. + +| Fixture | Shape | +|---|---| +| `ubam_ws_qname.bam` | 1 record, space in QNAME | +| `ubam_ws_qname_late.bam` | 3 records, space in record 2's QNAME | +| `ubam_ws_qname_paired.bam` | 2 interleaved pairs, space in the second pair's QNAME (record 3) | +| `ubam_tagvalue_space.bam` | 1 record, `CB:Z:has a space` — acceptance twin for the space exemption | +| `ubam_lf_qname.bam` | 3 records, newline in record 2's QNAME | +| `ubam_lf_tagvalue.bam` | 3 records, newline inside record 2's `CB:Z` value | +| `ubam_lf_atag.bam` | 3 records, newline inside record 2's `XA:A` value | +| `ubam_atag_ok.bam` | 3 records, printable `XA:A` values — acceptance twin for the `A` arm | + +The first four are samtools-built (requires `samtools`; not a runtime dep). +`--no-PG` keeps the header free of the build path, so the recipes below reproduce +the committed bytes exactly: + +```sh +S=ACGTACGTACGTACGTACGTACGTACGTACGTACGTACGT +Q=IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII +{ printf '@HD\tVN:1.6\n' + printf 'name with space\t4\t*\t0\t0\t*\t*\t0\t0\t%s\t%s\tCB:Z:AAACCC\n' "$S" "$Q" +} | samtools view --no-PG -b -o test_files/ubam_ws_qname.bam - +``` + +`ubam_ws_qname_late.bam` adds clean `readA` / `readC` records around +`readB with space`; `ubam_ws_qname_paired.bam` uses flags `77`/`141` for +`pairA` then `pairB with space`; `ubam_tagvalue_space.bam` is one `readA` with +`CB:Z:has a space`. + +The last four cannot be built with samtools. SAM text is line-delimited, so it +cannot express a newline in a QNAME or a tag value; BAM stores both as byte fields, +so the records are packed by hand. noodles' *record encoder* rejects any read name +outside `[!-?A-~]{1,254}`, but its `bgzf::Writer` is a plain sink, so only the +framing comes from noodles — see `examples/mk_bam_fixture.rs`: + +```sh +cargo run --quiet --example mk_bam_fixture -- lf_qname > test_files/ubam_lf_qname.bam +cargo run --quiet --example mk_bam_fixture -- lf_tagvalue > test_files/ubam_lf_tagvalue.bam +cargo run --quiet --example mk_bam_fixture -- lf_atag > test_files/ubam_lf_atag.bam +cargo run --quiet --example mk_bam_fixture -- atag_ok > test_files/ubam_atag_ok.bam +``` + +Verify — the record count is the probe that fails on the wrong input, because an +embedded newline makes one record occupy two output lines: + +```sh +samtools view test_files/ubam_lf_qname.bam | wc -l # 4 lines for 3 records +samtools view test_files/ubam_lf_qname.bam | sed -n '2,3p' # readB / EVIL, split +``` + +CI needs no samtools: all eight are committed. + ## uBAM output reference fixtures `ubam_out_se_REFERENCE.bam` (SE) and `ubam_out_pe_REFERENCE.bam` (PE, diff --git a/test_files/ubam_atag_ok.bam b/test_files/ubam_atag_ok.bam new file mode 100644 index 0000000000000000000000000000000000000000..c71ddfd44bc392f7ec956394a4da44a903625320 GIT binary patch literal 125 zcmb2|=3rp}f&Xj_PR>jW2@J(WUs9i>B_tGlD0s;8d9%?KDPWMGg-vp|}G J8EhYj005pcC4&F} literal 0 HcmV?d00001 diff --git a/test_files/ubam_lf_atag.bam b/test_files/ubam_lf_atag.bam new file mode 100644 index 0000000000000000000000000000000000000000..f02b6415a9c355070e4099f587376825b8c0cb4a GIT binary patch literal 125 zcmb2|=3rp}f&Xj_PR>jW2@J(WUs9i>B_tGlD0s;8d9%?K50RWV%C0zgj literal 0 HcmV?d00001 diff --git a/test_files/ubam_lf_qname.bam b/test_files/ubam_lf_qname.bam new file mode 100644 index 0000000000000000000000000000000000000000..73a0b94aec7f3c30bf428e651ef8db6be70640e3 GIT binary patch literal 140 zcmb2|=3rp}f&Xj_PR>jW`3%KHUs9i>B_tGlD0s;8d9%?Kp=tnq|PwV literal 0 HcmV?d00001 diff --git a/test_files/ubam_lf_tagvalue.bam b/test_files/ubam_lf_tagvalue.bam new file mode 100644 index 0000000000000000000000000000000000000000..a40be759cd1e89c8140c2f47438c2302685d47b4 GIT binary patch literal 135 zcmb2|=3rp}f&Xj_PR>jWSq#NRUs9i>B_tGlD0s;8d9%?KjWrVNbY3@`pWIr=hk14SJ?TsXu0tPJ(cxIl{h<<@4318I3Q zU0w{@DG3Qb_*rv!|Ae+Dd02rkqmjD0& literal 0 HcmV?d00001 diff --git a/test_files/ubam_ws_qname.bam b/test_files/ubam_ws_qname.bam new file mode 100644 index 0000000000000000000000000000000000000000..8313c5e56dffba9981f5de6707cb3167b12961e5 GIT binary patch literal 162 zcmb2|=3rp}f&Xj_PR>jWrVNbY3@`pWIr=hk14SJ?TsXu0tPJ(cxIl{h<<@4318I3Q zUH%N($q5NR_*t2FjWrVNbY3@`pWIr=hk14SJ?TsXu0tPJ(cxIl{h<<@4318I3Q zT}ce56B81C@U!Od$T{)w=m!8guX2i5Ynt>VQ0x$porMoV; literal 0 HcmV?d00001 diff --git a/test_files/ubam_ws_qname_paired.bam b/test_files/ubam_ws_qname_paired.bam new file mode 100644 index 0000000000000000000000000000000000000000..a7997de9db332a43be7ba5018d7cde94b459b379 GIT binary patch literal 184 zcmb2|=3rp}f&Xj_PR>jWrVNbY3@`pWIr=hk14SJ?TsXu0tPJ(cxIl{h<<@4318I3Q zU8xMG6B81C@U!Od$T{)w?BkL0NaAq_m@p&q$q9ugjhv<(Ea&_J`}+FMojb>4De>*= yr7Qc2R5r7I{BhoMq0MF$=Y=~I3@xXog?+ztI*Y;gdxY>cMoe3z8JIzi00RI7Q8NDk literal 0 HcmV?d00001 diff --git a/tests/integration_clump_only_ubam.rs b/tests/integration_clump_only_ubam.rs index 187cfb76..54bb177d 100644 --- a/tests/integration_clump_only_ubam.rs +++ b/tests/integration_clump_only_ubam.rs @@ -808,3 +808,34 @@ fn phred64_clump_only_bam_input_rejected() { "expected a --phred64 rejection" ); } + +/// `--clump_only` promises byte-identical preservation, so a dropped aux-tag tail +/// would break the contract `cli.rs` cites when refusing `--rename`. The refusal +/// reaches this path through the same `BamReader`. +#[test] +fn clump_only_ubam_refuses_whitespace_qname() { + let dir = fresh_tmpdir("ws_qname"); + let output = Command::new(binary()) + .args([ + "--clump_only", + "--output-format", + "ubam", + "--preserve-tags", + "CB", + ]) + .arg("-o") + .arg(&dir) + .arg(fixture("ubam_ws_qname.bam")) + .output() + .expect("trim_galore failed to run"); + assert!( + !output.status.success(), + "--clump_only must inherit the whitespace-QNAME refusal, not silently drop the tag" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("read name contains whitespace"), + "expected the whitespace message on the --clump_only path, got: {}", + stderr + ); +} diff --git a/tests/integration_ubam.rs b/tests/integration_ubam.rs index f9767c9f..5221d824 100644 --- a/tests/integration_ubam.rs +++ b/tests/integration_ubam.rs @@ -256,3 +256,348 @@ fn paired_with_single_fastq_rejected() { stderr ); } + +// --------------------------------------------------------------------------- +// #415 — whitespace in a BAM read name, and framing bytes in a Z tag value. +// +// Each case names the entry point it pins. Three entry points run before the +// trimming loop (sanity check, adapter detection, poly-G scan), so a fixture +// whose first record is the offender never reaches the writer; `-a` plus +// `--no_poly_g` skips both scans and is the arm a >1M-read file always takes. +// --------------------------------------------------------------------------- + +/// Skip adapter auto-detection and the poly-G scan so the trimming loop is reached. +const SKIP_PRESCANS: [&str; 3] = ["-a", "AGATCGGAAGAGC", "--no_poly_g"]; + +#[test] +fn ws_qname_refused_at_sanity_check_entry_point() { + let dir = fresh_tmpdir("tg_415_sanity"); + let output = Command::new(binary()) + .args(["--output-format", "ubam", "--preserve-tags", "CB"]) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_ws_qname.bam") + .output() + .expect("trim_galore failed to run"); + assert!(!output.status.success(), "whitespace QNAME must be refused"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("read name contains whitespace") && stderr.contains("name with space"), + "message must name the defect and the offending name, got: {}", + stderr + ); + // Record 1 is the offender, so the refusal precedes any writer. + let leftovers: Vec<_> = std::fs::read_dir(&dir) + .unwrap() + .filter_map(|e| e.ok().map(|e| e.file_name().to_string_lossy().into_owned())) + .filter(|n| n.contains("_trimmed") || n.contains("_val")) + .collect(); + assert!( + leftovers.is_empty(), + "refusal at the sanity check must write nothing, found: {:?}", + leftovers + ); +} + +#[test] +fn ws_qname_refused_in_trimming_loop_ubam_out_leaves_partial() { + let dir = fresh_tmpdir("tg_415_loop_ubam"); + let output = Command::new(binary()) + .args(["--output-format", "ubam", "--preserve-tags", "CB"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_ws_qname_late.bam") + .output() + .expect("trim_galore failed to run"); + assert!(!output.status.success(), "whitespace QNAME must be refused"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("BAM record 2"), + "must be caught in the trimming loop at record 2, not at the sanity check, got: {}", + stderr + ); + // The writer opens before the read loop, so record 1 is already on disk. + // Asserted, not desired — the cure spans all per-record bails and is tracked + // separately. + let partial = dir.join("ubam_ws_qname_late_trimmed.bam"); + let bytes = std::fs::read(&partial).expect("a mid-stream refusal leaves a partial uBAM behind"); + // What makes the residue a data-integrity problem is that it is indistinguishable + // from a complete BAM: bgzf's Drop finalises it, EOF marker included. + const BGZF_EOF: &[u8] = &[ + 0x1f, 0x8b, 0x08, 0x04, 0x00, 0x00, 0x00, 0x00, 0x00, 0xff, 0x06, 0x00, 0x42, 0x43, 0x02, + 0x00, 0x1b, 0x00, 0x03, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, + ]; + assert!( + bytes.ends_with(BGZF_EOF), + "the partial carries a valid BGZF EOF marker, so nothing downstream flags it ({} bytes)", + bytes.len() + ); +} + +#[test] +fn ws_qname_refused_in_trimming_loop_fastq_out() { + let dir = fresh_tmpdir("tg_415_loop_fastq"); + let output = Command::new(binary()) + .args(["--preserve-tags", "CB"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_ws_qname_late.bam") + .output() + .expect("trim_galore failed to run"); + // The widened scope: FASTQ output loses no tag text today, and is refused anyway. + assert!( + !output.status.success(), + "the refusal is read-side, so FASTQ output is refused too" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("read name contains whitespace"), + "expected the whitespace message on the FASTQ-output path, got: {}", + stderr + ); +} + +#[test] +fn ws_qname_refused_via_threaded_reader() { + let dir = fresh_tmpdir("tg_415_threaded"); + let output = Command::new(binary()) + .args(["--cores", "2"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_ws_qname_late.bam") + .output() + .expect("trim_galore failed to run"); + // `--cores 2` is the only route to the threaded single-stream reader. + assert!(!output.status.success(), "whitespace QNAME must be refused"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("read name contains whitespace"), + "expected the whitespace message from the threaded reader, got: {}", + stderr + ); +} + +#[test] +fn ws_qname_refused_via_interleaved_deinterleaver() { + let dir = fresh_tmpdir("tg_415_deinterleaved"); + let output = Command::new(binary()) + .arg("--paired") + .args(SKIP_PRESCANS) + .args(["-a2", "AGATCGGAAGAGC"]) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_ws_qname_paired.bam") + .output() + .expect("trim_galore failed to run"); + assert!(!output.status.success(), "whitespace QNAME must be refused"); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("read name contains whitespace"), + "expected the whitespace message, got: {}", + stderr + ); + // Record 3 is the first read of the second pair — proves the de-interleaver + // reported it, not the sanity check (which only ever sees record 1). + assert!( + stderr.contains("BAM record 3"), + "must come from the de-interleaver at record 3, got: {}", + stderr + ); +} + +#[test] +fn lf_in_qname_refused_end_to_end() { + let dir = fresh_tmpdir("tg_415_lf_qname"); + let output = Command::new(binary()) + .args(["--preserve-tags", "CB"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_lf_qname.bam") + .output() + .expect("trim_galore failed to run"); + // A newline in a read name would split one record across five lines, + // desynchronising every 4-line-block reader after it. + assert!(!output.status.success(), "LF in a QNAME must be refused"); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("read name contains whitespace") && stderr.contains("readB\\nEVIL"), + "message must show the newline escaped, got: {}", + stderr + ); +} + +#[test] +fn lf_in_tag_value_refused_end_to_end() { + let dir = fresh_tmpdir("tg_415_lf_tag"); + let output = Command::new(binary()) + .args(["--preserve-tags", "CB"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_lf_tagvalue.bam") + .output() + .expect("trim_galore failed to run"); + assert!( + !output.status.success(), + "a newline in a preserved Z value corrupts framing the same way a QNAME newline does" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + // `aux tag 'CB'` comes from the call site, not the predicate — asserting the bare + // prefix would also match the predicate's own wording and pin nothing. + assert!( + stderr.contains("aux tag 'CB'") && stderr.contains("AAA\\nCCC"), + "message must name the tag and show the escaped value, got: {}", + stderr + ); +} + +#[test] +fn lf_in_character_tag_value_refused_end_to_end() { + // `A` values reach the same id as `Z` values through a different match arm. + let dir = fresh_tmpdir("tg_415_lf_atag"); + let output = Command::new(binary()) + .args(["--preserve-tags", "XA"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_lf_atag.bam") + .output() + .expect("trim_galore failed to run"); + assert!( + !output.status.success(), + "a newline in an A tag value corrupts framing the same way a Z value does" + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("aux tag 'XA'") && stderr.contains("cannot pass through"), + "message must name the tag and the framing failure, got: {}", + stderr + ); +} + +#[test] +fn printable_character_tags_round_trip() { + // The acceptance twin for the A arm — green whether or not the guard is present, + // so it pins carriage rather than refusal. + let dir = fresh_tmpdir("tg_415_atag_ok"); + let output = Command::new(binary()) + .args(["--preserve-tags", "XA"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_atag_ok.bam") + .output() + .expect("trim_galore failed to run"); + assert!( + output.status.success(), + "printable A values must not be refused, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let tuples = read_fastq_tuples(&dir.join("ubam_atag_ok_trimmed.fq")); + assert_eq!(tuples.len(), 3); + let tags: Vec<&str> = tuples.iter().map(|t| t.0.as_str()).collect(); + assert!( + tags[0].ends_with("XA:A:+") && tags[1].ends_with("XA:A:-"), + "A values must reach the header intact, got: {:?}", + tags + ); +} + +#[test] +fn clean_tag_value_with_space_still_accepted() { + // A space is legal in a `Z` value, and the tab introducing the tag precedes + // it — so the tag tail still parses. Guards against reusing the QNAME set. + let dir = fresh_tmpdir("tg_415_tag_space"); + let output = Command::new(binary()) + .args(["--preserve-tags", "CB"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg("test_files/ubam_tagvalue_space.bam") + .output() + .expect("trim_galore failed to run"); + assert!( + output.status.success(), + "a space in a Z tag value must stay legal, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let tuples = read_fastq_tuples(&dir.join("ubam_tagvalue_space_trimmed.fq")); + assert_eq!(tuples.len(), 1); + assert!( + tuples[0].0.contains("CB:Z:has a space"), + "the space-bearing tag must survive, got id: {:?}", + tuples[0].0 + ); + + // uBAM out is the harder direction: `parse_name_and_data` has to re-split the tail + // around the space rather than treat it as a description boundary. + let ubam_dir = fresh_tmpdir("tg_415_tag_space_ubam"); + let output = Command::new(binary()) + .args(["--output-format", "ubam", "--preserve-tags", "CB"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&ubam_dir) + .arg("test_files/ubam_tagvalue_space.bam") + .output() + .expect("trim_galore failed to run"); + assert!( + output.status.success(), + "uBAM round-trip of a space-bearing Z value must succeed, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let mut decoded = Vec::new(); + std::io::Read::read_to_end( + &mut noodles::bgzf::Reader::new( + std::fs::File::open(ubam_dir.join("ubam_tagvalue_space_trimmed.bam")).unwrap(), + ), + &mut decoded, + ) + .unwrap(); + assert!( + decoded.windows(15).any(|w| w == b"CBZhas a space\0"), + "the space-bearing Z value must round-trip into the output BAM's aux data" + ); +} + +#[test] +fn description_notice_wording_is_fastq_input_only() { + // No space reaches `parse_name_and_data` from BAM input, so #406's "FASTQ + // header text" wording is unconditionally accurate; this pins the one + // direction that still reaches it. + let dir = fresh_tmpdir("tg_415_notice"); + let fq = dir.join("desc.fastq"); + std::fs::write( + &fq, + "@readA 1:N:0:CGATCG\n\ + ACGTACGTACGTACGTACGTACGTACGTACGTACGTACGT\n\ + +\n\ + IIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIIII\n", + ) + .unwrap(); + let output = Command::new(binary()) + .args(["--output-format", "ubam"]) + .args(SKIP_PRESCANS) + .arg("-o") + .arg(&dir) + .arg(&fq) + .output() + .expect("trim_galore failed to run"); + assert!( + output.status.success(), + "a FASTQ description is legitimate and must still be dropped with a notice, stderr: {}", + String::from_utf8_lossy(&output.stderr) + ); + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("FASTQ header text"), + "the #406 notice must still fire on FASTQ input, got: {}", + stderr + ); +}