fix(clip): unmap fully-clipped reads and match fgbio unmap/mate fidelity (CLIP-01)

[nh13]

What & why

clip (and every internal caller of SamRecordClipper / RawRecordClipper) diverged from fgbio when clipping removes all of a read's aligned bases. This PR closes the gap and, in doing so, ports the full fgbio/htsjdk fidelity for the unmap and mate-info paths.

fgbio SamRecordClipper.clip{Start,End}OfAlignment computes numberOfClippableBases (the sum of alignment M/=/X and insertion I CIGAR op lengths) and, when the requested clip is >= that count, calls htsjdk SAMUtils.makeReadUnmapped and returns — the read becomes unmapped with an empty CIGAR, keeping its bases/qualities. fgumi instead clipped every aligned base, leaving the read mapped at a stale POS with an all-clip CIGAR (e.g. 50H) and an empty sequence — silent corruption. This is tracker finding CLIP-01.

Fix

1. Unmap when the whole alignment is clipped (CLIP-01).

• Add the numberOfClippableBases guard to both SamRecordClipper and RawRecordClipper, in clip_start_of_alignment and clip_end_of_alignment: when clippable <= requested, unmap the read and return the clippable count, rather than proceeding to clip.
• make_read_unmapped[_raw] mirrors htsjdk SAMUtils.makeReadUnmapped: clear reference / pos / mapq / CIGAR / TLEN and the duplicate, secondary, supplementary, and proper-pair flags; set UNMAPPED; reverse-complement bases + reverse quals (then clear the reverse flag) for reverse-strand reads; bases/quals are otherwise preserved.
• Two fgumi unit tests that pinned the old all-soft-clip behavior (10S10M clip 10, 10M clip 10) are updated to assert the unmap.

2. Reorient strand-sensitive aux tags on unmap (htsjdk reverseComplement(true) fidelity).

htsjdk makeReadUnmapped reverse-complements a reverse-strand read via reverseComplement(true), which does more than SEQ/QUAL: it also reverse-complements the base-oriented tags in SAMRecord.TAGS_TO_REVERSE_COMPLEMENT (E2, SQ) and reverses the quality-oriented tags in TAGS_TO_REVERSE (OQ, U2). fgumi's make_read_unmapped[_raw] previously left these forward-oriented, so a reverse-strand read carrying e.g. OQ that clipping unmapped diverged from fgbio — squarely in the CLIP-01 path.

• make_read_unmapped (typed) and make_read_unmapped_raw now reorient these tags for reverse-strand reads, driven by shared TAGS_TO_REVERSE_COMPLEMENT / TAGS_TO_REVERSE constants so the two paths stay in lockstep.
• Only the Z-string encoding is handled (the sole form these tags take in practice); htsjdk additionally reverses array-encoded forms, but none of these tags is emitted as an array by real tooling. Documented inline.
• Adds E2 / U2 / SQ to the SamTag catalog.

3. Full mate-info parity, including the one-mapped mate-reverse flag.

Because fgbio ClipBam then calls SamPairUtil.setMateInfo(r1, r2, true) on every pair, the clipper unmap surfaced a paired-read gap: fgumi's clip only refreshed the MC/MQ tags. This PR replaces that with set_mate_info_raw, a faithful port of htsjdk setMateInfo (all three branches — both mapped, both unmapped, one of each) covering mate ref/pos/strand, the mate-unmapped flag, MQ/MC tags, and TLEN. This makes paired output match fgbio when a mate unmaps, and also fixes stale mate-pos/TLEN in the ordinary both-mapped case (previously never updated after a clip shifted a start).

• In the one-mapped branch, the mapped read's mate-reverse flag now reflects the unmapped read's actual strand (SamPairUtil.java:267) rather than an assumed false. The clipper's own unmap clears REVERSE, but a read that arrived unmapped-on-input may still carry it, and set_mate_info_raw runs on every pair, so such a read reaches this branch.

Parity evidence (fgbio 4.1.0 ClipBam vs fgumi clip, compare bams --mode content)

Reproducer: reports/fgbio-parity-fixtures/clip-01-unmap-fully-clipped.sh.

Scenario A — mapped fragment, 50M@chr1:100, --read-one-five-prime 60:

	before	after
fgumi	FLAG=0 POS=150 CIGAR=50H (mapped, corrupt)	FLAG=4 POS=0 CIGAR=* (unmapped)
vs fgbio	DIFFER	IDENTICAL

Scenario B — FR pair, both clipped past alignment (--read-one-five-prime 101 --read-two-five-prime 101, ClipBamTest.scala:621):

	before	after
fgumi	mate-flags stale (MATE_REVERSE set, RNEXT=chr1)	FLAG=77/141, RNEXT=* PNEXT=0 TLEN=0
vs fgbio	DIFFER	IDENTICAL

Also verified IDENTICAL to fgbio for normal paired clipping that does not unmap (overlap clip, fixed 3′ clip, clip-past-mate), confirming the setMateInfo change is safe on the common path.

Tests / CI

• Clipper unmap cross-checks (crosscheck_unmap_*): start/end, boundary (clip == clippable), no-unmap regression guard, and negative-strand revcomp — on both SamRecordClipper and RawRecordClipper.
• New tag-reorientation cross-checks: negative-strand reorients OQ/U2 (reversed) and E2 (reverse-complemented) identically on the typed and raw paths; positive-strand leaves them untouched.
• test_set_mate_info_raw_all_branches (all three htsjdk branches) plus a new one-mapped case asserting the mate-reverse flag follows the unmapped read's actual strand.
• cargo ci-fmt, cargo ci-lint, cargo ci-test (2217 tests) all green.

Refs: CLIP-01.

[coderabbitai]

Walkthrough

Fully clipped reads now become unmapped in typed and raw paths while preserving and normalizing sequence data. Paired clipping now synchronizes mate coordinates, flags, tags, and template lengths through set_mate_info_raw.

Changes

Clipper unmap behavior

Layer / File(s)	Summary
Typed unmap helpers and wiring crates/fgumi-sam/src/clipper.rs	Computes clippable bases, converts fully clipped typed records to unmapped state, and adds start/end early-unmap branches with reverse-strand tag handling.
Raw unmap helpers and wiring crates/fgumi-sam/src/clipper.rs	Adds equivalent raw-byte behavior, including reverse-complemented sequence, reversed qualities, cleared alignment fields, and empty CIGAR.
Unmap behavior validation crates/fgumi-sam/src/clipper.rs, crates/fgumi-sam/Cargo.toml, crates/fgumi-tag/src/tag.rs	Updates full-clip expectations and cross-checks typed/raw state, boundary counts, preserved bases and qualities, reverse-strand handling, and auxiliary tags.

Mate-pair info synchronization

Layer / File(s)	Summary
Mate synchronization implementation src/lib/commands/clip.rs	Replaces tag-only updates with mapped/mapped, mapped/unmapped, and unmapped/unmapped handling for coordinates, mate flags, MQ/MC, and TLEN.
Pipeline integration src/lib/commands/clip.rs	Uses set_mate_info_raw(r1, r2) in single-threaded and multi-threaded paired clipping paths.
Mate-state validation src/lib/commands/clip.rs	Tests all mate-state branches and their coordinate, flag, tag, and TLEN results.

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

Possibly related PRs

• fulcrumgenomics/fgumi#289: Adds related raw clipping APIs used by the raw-path clipping changes.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fully clipped reads are unmapped and mate handling is aligned with fgbio.

✨ 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-clip-unmap-when-fully-clipped

---

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

[codecov]

Codecov Report

❌ Patch coverage is 98.50000% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 91.17%. Comparing base (f638abc) to head (6794f10).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/lib/commands/clip.rs	98.50%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #492      +/-   ##
==========================================
+ Coverage   91.12%   91.17%   +0.05%     
==========================================
  Files          78       78              
  Lines       51540    51916     +376     
==========================================
+ Hits        46964    47334     +370     
- Misses       4576     4582       +6     

☔ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

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 `@crates/fgumi-sam/src/clipper.rs`:
- Around line 5969-6057: The new clipper tests are parameterized with runtime
`for` loops, which should be converted to `rstest` cases for per-case reporting.
Refactor the `crosscheck_unmap_*` tests in `clipper.rs` to use `#[rstest]` with
`#[case(...)]` for `ClippingMode` (and any clip/bases variations) instead of
looping inside the test body, while keeping the existing assertions and coverage
intact.

🪄 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: d27c0889-7069-4598-8416-07f25da78ae2

📥 Commits

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

📒 Files selected for processing (2)

• crates/fgumi-sam/src/clipper.rs
• src/lib/commands/clip.rs

[nh13]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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]

@coderabbitai review

[coderabbitai]

✅ 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.