fix(fastq): narrow read-name sync suffix strip to fgbio parity by nh13 · Pull Request #512 · fulcrumgenomics/fgumi

nh13 · 2026-07-09T05:52:53Z

Problem

fastq_parse::strip_read_suffix canonicalizes read names before validating that paired/interleaved FASTQ streams are in sync — i.e. that the reads at the same position across streams share a name. Its rule was too lenient: after stripping a trailing space-separated comment, it also stripped a trailing separator + digit where the separator was one of /, ., _, : and the digit was 1 or 2. That collapses a genuinely mismatched pair such as read_1 (stream 0) and read_2 (stream 1) to the same base name read, so two different reads are silently accepted as "in sync" instead of being rejected.

Fix

Narrow the rule to match fgbio's FastqSource read-name canonicalization in com/fulcrumgenomics/fastq/FastqSource.scala, which strips only a trailing / followed by a single ASCII digit (0-9):

val suffix = fullName.takeRight(2)
val (name, readNumber) = suffix.length == 2 && suffix(0) == '/' && suffix(1).isDigit match {
  case true  => (fullName.dropRight(2), Some(fullName.last.asDigit))
  case false => (fullName, None)
}

The trailing space-comment strip (a separate, correct concern) is unchanged. After it, strip_read_suffix now strips a suffix only when it is / + a single ASCII digit. It no longer strips ./_/: separators, and it no longer strips multi-digit runs (read/12 is left intact). This is the same rule PR #489 established for the extract path (strip_read_number_suffix), now applied to the shared FASTQ-sync helper.

New one-line spec: strip a trailing space comment, then strip a trailing / + single ASCII digit; leave everything else (./_/: separators, multi-digit runs, non-digit suffixes) intact.

Affected callers

All three production callers are paired-FASTQ name-sync validation and benefit uniformly with no call-site changes:

FastqGrouper::drain_complete_templates (src/lib/grouper.rs)
zip_fastq source step
parse_zip_fastq source step

(On this branch's base only grouper.rs actually calls the shared helper; the zip_fastq/parse_zip_fastq source steps are not present on main yet, but they use the same helper and get the fix for free when they land.)

Behavior tradeoff (please note)

Narrowing correctly rejects mismatched read_1 / read_2 streams that the old rule silently accepted. But it also now rejects non-standard paired FASTQs that legitimately use .1/.2/_1/_2/:1/:2 read-number suffixes to distinguish R1/R2. This matches fgbio, which also rejects such names as out of sync, so it is correct fgbio parity — but it is a real, user-visible behavior change reviewers should be aware of. FASTQs using the standard /1//2 (or no suffix) are unaffected.

Tests

Rewrote test_strip_read_suffix (fastq_parse.rs) as an rstest table: /1, /2, /3, /0 strip to read; .1/.2/_1/_2/:1/:2 are preserved; read/12 (multi-digit) and read/x (non-digit) are preserved; a trailing space comment is still stripped first (with and without a following /+digit).
Added test_strip_read_suffix_rejects_mismatched_underscore_pair (fastq_parse.rs) asserting read_1 and read_2 no longer collide, with a comment documenting the tradeoff.
Added test_fastq_grouper_rejects_mismatched_underscore_pair (grouper.rs) driving FastqGrouper end-to-end to prove the sync-validation now rejects a mismatched read_1 / read_2 pair as "out of sync".
No other tests codified the old broad behavior; the existing grouper/fastq tests all use /1//2 suffixes, which remain valid.

Follow-up (out of scope here)

src/lib/commands/extract.rs still carries its own broad strip_read_suffix_extract. Consolidating both onto the narrow shared helper is a natural follow-up, intentionally left out to keep this fix focused on strip_read_suffix.

Summary by CodeRabbit

Bug Fixes
- Improved paired-end read-name handling to prevent distinct reads such as _1 and _2 from being incorrectly treated as identical.
- Preserved multi-digit, nonstandard, and underscore-based suffixes during read-name processing.
- Added validation for mismatched paired-end names, with a clear “out of sync” error.

`strip_read_suffix` canonicalizes read names before checking that paired/interleaved FASTQ streams are in sync (that the reads at the same position across streams share a name). Its old rule was too lenient: after stripping a trailing space-separated comment it also stripped a trailing `separator + digit` for separator in {`/`, `.`, `_`, `:`} and digit in {`1`, `2`}. That collapses a genuinely-mismatched pair such as `read_1` (stream 0) and `read_2` (stream 1) to the same base name `read`, so two different reads are silently accepted as "in sync". Narrow the rule to match fgbio's `FastqSource` read-name canonicalization (`com/fulcrumgenomics/fastq/FastqSource.scala`), which strips only a trailing `/` followed by a single ASCII digit (0-9). The space-comment strip is unchanged; `.`/`_`/`:` separators and multi-digit runs (`read/12`) are now preserved. This is the same rule PR #489 established for the extract path, applied here to the shared FASTQ-sync helper. All three production callers are paired-FASTQ name-sync validation and benefit uniformly with no call-site changes: `FastqGrouper` (`grouper.rs`), `zip_fastq`, and `parse_zip_fastq`. Behavior tradeoff: narrowing correctly rejects mismatched `read_1` / `read_2` streams, but it ALSO now rejects non-standard paired FASTQs that legitimately use `.1`/`.2`/`_1`/`_2`/`:1`/`:2` read-number suffixes. This matches fgbio (which rejects them too), so it is correct fgbio parity, but it is a real behavior change reviewers should note. Future cleanup: `extract.rs` still carries its own broad `strip_read_suffix_extract`; consolidating both onto the narrow shared helper is a follow-up left out of this focused fix.

coderabbitai · 2026-07-09T05:53:00Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 45bff5a4-53b2-4a9d-a535-48f061e60d95

📥 Commits

Reviewing files that changed from the base of the PR and between f638abc and a02b27c.

📒 Files selected for processing (2)

src/lib/fastq_parse.rs
src/lib/grouper.rs

Walkthrough

Risk of collapsing distinct paired reads is reduced by narrowing suffix stripping to trailing / plus one ASCII digit after comment removal. Underscore-suffixed names remain distinct, and grouper tests verify mismatches are rejected.

Changes

FASTQ suffix canonicalization

Layer / File(s)	Summary
Canonicalization rules and coverage `src/lib/fastq_parse.rs`	`strip_read_suffix` removes comments and only a trailing single-digit slash suffix; parameterized tests cover preserved separators, multi-digit suffixes, non-digits, and comment handling.
Grouper mismatch validation `src/lib/grouper.rs`	A paired-stream test confirms `_1` and `_2` names remain out of sync and cause `drain_complete_templates()` to return an error.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

fulcrumgenomics/fgumi#244: Introduced the shared FASTQ suffix-stripping utility used by this canonicalization flow.
fulcrumgenomics/fgumi#486: Also narrows FASTQ QNAME suffix handling and updates paired-end validation.

Suggested labels: fgumi extract

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: narrowing FASTQ read-name suffix stripping to match fgbio behavior.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

Create stacked PR
Commit on current branch

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch nh/fix-fastqgrouper-readname-sync

_{Comment @coderabbitai help to get the list of available commands.}

codecov · 2026-07-09T05:55:12Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.05%. Comparing base (f638abc) to head (a02b27c).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
- Coverage   91.12%   91.05%   -0.07%     
==========================================
  Files          78       78              
  Lines       51540    51541       +1     
==========================================
- Hits        46964    46933      -31     
- Misses       4576     4608      +32

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

nh13 · 2026-07-11T10:03:42Z

@coderabbitai review

coderabbitai · 2026-07-11T10:03:49Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

nh13 · 2026-07-11T12:41:52Z

@coderabbitai review

coderabbitai · 2026-07-11T12:41:56Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

nh13 · 2026-07-11T13:53:14Z

@coderabbitai review

coderabbitai · 2026-07-11T13:53:18Z

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

nh13 temporarily deployed to github-actions July 9, 2026 05:52 — with GitHub Actions Inactive

nh13 mentioned this pull request Jul 10, 2026

fix(fastq): accept CRLF and unterminated final records (W9b: EXT3-06/07) #554

Merged

nh13 merged commit 3699753 into main Jul 11, 2026
11 checks passed

nh13 deleted the nh/fix-fastqgrouper-readname-sync branch July 11, 2026 20:29

This was referenced Jul 11, 2026

chore: release v0.5.0 #454

Merged

refactor(fastq): consolidate read-name suffix stripping and assert cross-parser parity #572

Merged

Navigation Menu

Repository navigation

fix(fastq): narrow read-name sync suffix strip to fgbio parity - #512

fix(fastq): narrow read-name sync suffix strip to fgbio parity#512
nh13 merged 1 commit into
mainfrom
nh/fix-fastqgrouper-readname-sync

nh13 commented Jul 9, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

Uh oh!

codecov Bot commented Jul 9, 2026

Uh oh!

nh13 commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

nh13 commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

nh13 commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

nh13 commented Jul 9, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix

Affected callers

Behavior tradeoff (please note)

Tests

Follow-up (out of scope here)

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Jul 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

codecov Bot commented Jul 9, 2026

Codecov Report

Uh oh!

nh13 commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nh13 commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nh13 commented Jul 11, 2026

Uh oh!

coderabbitai Bot commented Jul 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

This branch was previously deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

nh13 commented Jul 9, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jul 9, 2026 •

edited

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading

coderabbitai Bot commented Jul 11, 2026 •

edited

Loading