Skip to content

fix(extract): support non-terminal + and reject over-long reads in read structures (R2-RS-01, R2-RS-02) - #496

Merged
nh13 merged 1 commit into
mainfrom
nh/fix-extract-read-structure
Jul 10, 2026
Merged

nh13 merged 1 commit into
mainfrom
nh/fix-extract-read-structure

Conversation

@nh13

@nh13 nh13 commented Jul 8, 2026 •

Copy link
Copy Markdown
Member

Summary

Two extract read-structure behaviors diverge from fgbio 4.1.0 (the parity target, ReadStructure #1157). Both stem from the read-structure = 0.2.0 crate:

  • R2-RS-01 (S3): fgbio accepts a + (any-length) segment in any position (e.g. 8B+M10T, +M70T) and extracts segments after the + by counting back from the read end. The crate rejects all non-terminal + at parse time, so fgumi extract erred before it did any work.
  • R2-RS-02 (S1/S3): for a fully-fixed structure, fgbio requires the read length to match exactly — an over-long read is an error. The crate only guards too-few bases and silently truncates the trailing bases of an over-long read.

Fix

Add a small local read-structure implementation (src/lib/read_structure.rs) that mirrors fgbio's model — a single + in any position, post-+ segments anchored to the read end, and exact-length validation for fully-fixed structures (over-long ⇒ error, never a silent truncation). The read-structure crate stays pinned at 0.2.0 (its SegmentType is reused); bumping to 0.3.0 would fix R2-RS-01 but change trailing-+ from zero-or-more to one-or-more (a new parity break) and force a breaking API migration. Wired into extract and the FASTQ segmenter (extraction is now structure-level).

Parity evidence (§0 step 2 → step 4)

Oracle: fgbio FastqToBam vs fgumi extract. Read structure 8B+M10T on a 30bp read; 8M2T on a 12bp read.

finding before (fgumi) after (fgumi) fgbio
R2-RS-01 non-terminal + exit=2 (parse error) exit=0 RX=ACGTACGTACGT out_seq=ACGTACGTAC identical
R2-RS-02 over-long fixed exit=0 (truncated to AC) exit=1 (rejected) identical

Repro script: reports/fgbio-parity-fixtures/extract-read-structure-parity.sh (+ .before.txt / .after.txt).

Tests / CI

  • New unit tests in read_structure (parse of non-terminal +, multiple-+ / zero-length rejection, span_of walk-back, exact-length vs zero-or-more validation) and fastq (structure-level extraction of 8B+M10T, over-long rejection incl. under the TooFewBases skip reason).
  • cargo ci-fmt / ci-lint / ci-test all clean (2233 passed).

Stacking

Stacked on #489 (nh/fix-extract-readname-umi-strict) — shares extract.rs. Auto-retargets to main when #489 merges. Merge order: #489 → this.

Tracker: fgbio↔fgumi behavioral parity round 2 — R2-RS-01, R2-RS-02 (§R2.4, PR 2 addendum).

Summary by CodeRabbit

  • New Features
    • Added a new read-structure implementation supporting a variable-length (+) segment in any position.
    • Includes fgbio-compatible span semantics and length checking.
  • Bug Fixes
    • Enforced sequence/quality length matching during FASTQ parsing.
    • Segment extraction now uses computed spans, including correct handling of non-terminal +.
    • Fixed-structure over-long reads are rejected consistently, independent of skip settings.
  • Documentation
    • Updated “Read Structure” documentation and examples for + positioning behavior.
  • Tests
    • Added coverage for non-terminal + boundaries and fixed-structure over-long rejection.

@nh13
nh13 temporarily deployed to github-actions July 8, 2026 23:53 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Jul 8, 2026 •

Copy link
Copy Markdown

Review Change Stack

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: c26fc556-e150-4196-ba54-412cd213f4bb

📥 Commits

Reviewing files that changed from the base of the PR and between 402370d and 8332e23.

📒 Files selected for processing (4)
  • src/lib/commands/extract.rs
  • src/lib/fastq.rs
  • src/lib/mod.rs
  • src/lib/read_structure.rs

Walkthrough

Adds a public local ReadStructure implementation supporting non-terminal + segments, fixed-length validation, and read-end-anchored spans. FASTQ parsing validates sequence/quality lengths, rejects over-long fixed reads, and extracts segments using computed spans.

Changes

Read structure and FASTQ extraction

Layer / File(s) Summary
Read structure parsing contract
src/lib/mod.rs, src/lib/read_structure.rs
Adds public read-structure types, parsing and formatting support, parse errors, and tests for valid and malformed structures.
Length validation and segment spans
src/lib/read_structure.rs
Computes offsets around a single +, resolves spans from the read end, and distinguishes valid, too-short, and over-long reads.
FASTQ validation and extraction
src/lib/fastq.rs, src/lib/commands/extract.rs
Uses length checks and computed spans for slicing, adds non-terminal + and fixed-structure rejection tests, and updates imports and documentation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant FASTQRecord
  participant FastqSet
  participant ReadStructure
  FASTQRecord->>FastqSet: provide sequence and quality
  FastqSet->>ReadStructure: check_read_length(read_len)
  ReadStructure-->>FastqSet: LengthCheck result
  FastqSet->>ReadStructure: span_of(segment_index, read_len)
  ReadStructure-->>FastqSet: segment span
  FastqSet-->>FASTQRecord: extracted segments
Loading

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 clearly captures the main behavior change: non-terminal + support and over-long read rejection in read-structure handling.
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-extract-read-structure

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

@codecov

codecov Bot commented Jul 8, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 92.12598% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.18%. Comparing base (0371ce2) to head (8332e23).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/lib/read_structure.rs 91.32% 17 Missing ⚠️
src/lib/fastq.rs 94.82% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #496      +/-   ##
==========================================
+ Coverage   91.08%   91.18%   +0.09%     
==========================================
  Files          78       79       +1     
  Lines       51726    52418     +692     
==========================================
+ Hits        47116    47798     +682     
- Misses       4610     4620      +10     

☔ 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
nh13 force-pushed the nh/fix-extract-readname-umi-strict branch from a9e4a9a to 50f36c4 Compare July 9, 2026 17:00
@nh13
nh13 force-pushed the nh/fix-extract-read-structure branch from c8fba87 to ab2eaf3 Compare July 9, 2026 20:32
@nh13
nh13 temporarily deployed to github-actions July 9, 2026 20:32 — with GitHub Actions Inactive
@nh13
nh13 force-pushed the nh/fix-extract-readname-umi-strict branch from 50f36c4 to f165260 Compare July 9, 2026 21:10
@nh13
nh13 force-pushed the nh/fix-extract-read-structure branch from ab2eaf3 to 247488d Compare July 9, 2026 21:11
@nh13
nh13 temporarily deployed to github-actions July 9, 2026 21:11 — with GitHub Actions Inactive
@nh13
nh13 force-pushed the nh/fix-extract-readname-umi-strict branch from f165260 to 9879c95 Compare July 9, 2026 22:20
@nh13
nh13 force-pushed the nh/fix-extract-read-structure branch from 247488d to 194e224 Compare July 9, 2026 22:21
@nh13
nh13 temporarily deployed to github-actions July 9, 2026 22:21 — with GitHub Actions Inactive
Base automatically changed from nh/fix-extract-readname-umi-strict to main July 10, 2026 00:45
@nh13
nh13 force-pushed the nh/fix-extract-read-structure branch from 194e224 to 3b7125d Compare July 10, 2026 00:48
@nh13
nh13 temporarily deployed to github-actions July 10, 2026 00:48 — with GitHub Actions Inactive
@nh13

nh13 commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
✅ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 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.rs`:
- Around line 235-275: Update the public # Errors documentation for the affected
read-parsing method to document sequence/quality length mismatches and over-long
reads against fixed structures, and remove references to obsolete per-segment
extraction errors. Locate the documentation adjacent to the method containing
the LengthCheck match and ensure it accurately lists all errors returned by that
method.

In `@src/lib/read_structure.rs`:
- Around line 167-190: Reject arithmetic overflow throughout ReadStructure
parsing: update length parsing, fixed-length summation, and offset accumulation
in the structure parser (including the post-`+` reverse pass) to use
checked_mul/checked_add and propagate a dedicated ReadStructureError instead of
panicking or wrapping. Ensure overflow is detected before validation completes
so span_of cannot receive wrapped offsets.
- Around line 15-20: Correct the module documentation describing end-anchored
segment offsets: update the statement that offsets after the indefinite-length
segment are stored as negative numbers to state that they are represented by
positive distances from the read end via SegOffset::FromEnd(usize).
- Around line 341-349: Convert rejects_multiple_plus_and_bad_tokens into an
rstest parameterized test, adding one #[case] for each malformed structure and
passing the case value to ReadStructure::from_str so failures identify the exact
input; retain the separate assertion for the MissingOperator error on "8B+".
🪄 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: 42dd6d51-30ca-4d38-82f7-b45d42bc34bc

📥 Commits

Reviewing files that changed from the base of the PR and between 0371ce2 and 3b7125d.

📒 Files selected for processing (4)
  • src/lib/commands/extract.rs
  • src/lib/fastq.rs
  • src/lib/mod.rs
  • src/lib/read_structure.rs

Comment thread src/lib/fastq.rs
Comment thread src/lib/read_structure.rs
Comment thread src/lib/read_structure.rs Outdated
Comment thread src/lib/read_structure.rs Outdated
@nh13

nh13 commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
✅ 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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/commands/extract.rs`:
- Line 310: Correct the user-facing CLI help text in the read-structure
documentation near the comment line by replacing “monotemplate” with “template”;
preserve the surrounding wording and formatting.

In `@src/lib/read_structure.rs`:
- Around line 348-447: Add proptest-based tests alongside the existing
read-structure tests, covering normalized parsing round trips through
ReadStructure::from_str and to_string, plus span_of invariants that each span
satisfies start <= end <= read_len and spans provide contiguous coverage.
Generate valid structures and read lengths, and ensure the properties exercise
variable and fixed segments beyond the current examples.
🪄 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: 7a781ba7-c626-4381-b5b8-ead4333e12aa

📥 Commits

Reviewing files that changed from the base of the PR and between 3b7125d and 402370d.

📒 Files selected for processing (4)
  • src/lib/commands/extract.rs
  • src/lib/fastq.rs
  • src/lib/mod.rs
  • src/lib/read_structure.rs

Comment thread src/lib/commands/extract.rs Outdated
Comment thread src/lib/read_structure.rs
…d structures (R2-RS-01, R2-RS-02)

fgumi consumes the read-structure 0.2.0 crate, which (a) rejects a `+` (any-length) segment in any position but the last, and (b) silently truncates trailing bases when a read is longer than a fully-fixed structure. Both diverge from fgbio 4.1.0 (ReadStructure #1157), the parity target.

Add a local read-structure implementation (the crate stays pinned at 0.2.0, reusing its SegmentType) that mirrors fgbio's model: a single `+` may appear in any position, with segments after it resolved by counting back from the read end; a fully-fixed structure requires an exact read length, so an over-long read is an error rather than a silent truncation. Wire it into `extract` and the FASTQ segmenter (structure-level extraction).

R2-RS-01 (non-terminal +): before, fgumi errored at parse ("non-terminal segment that has an indefinite length"); after, fgumi extracts identically to fgbio.
R2-RS-02 (over-long, fully-fixed): before, fgumi silently dropped trailing bases; after, fgumi errors, matching fgbio.

Parity evidence (fgbio FastqToBam oracle vs fgumi extract; read structure 8B+M10T on a 30bp read, 8M2T on a 12bp read):
  R2-RS-01  before: fgumi exit=2 (parse error)     after: fgumi exit=0 RX=ACGTACGTACGT out_seq=ACGTACGTAC (== fgbio)
  R2-RS-02  before: fgumi exit=0 (truncated to AC)  after: fgumi exit=1 rejected (== fgbio)
@nh13

nh13 commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
✅ 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

nh13 commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
✅ 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

nh13 commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
✅ 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

nh13 commented Jul 10, 2026

Copy link
Copy Markdown
Member Author

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jul 10, 2026 •

Copy link
Copy Markdown
✅ 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
nh13 merged commit 11c8765 into main Jul 10, 2026
11 checks passed
@nh13
nh13 deleted the nh/fix-extract-read-structure branch July 10, 2026 23:59
@nh13 nh13 mentioned this pull request Jul 10, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 8332e239 Deployed Jul 10, 2026 by nh13 via coverage #2202
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant