fix(zipper,clip): compute supplementary TLEN instead of copying the mate primary's

[nh13]

Closes #673.

Template::fix_mate_info (zipper) and set_supplemental_mate_info_raw (clip) set a supplementary alignment's TLEN by negating its mate primary's TLEN, a faithful port of htsjdk's SamPairUtil.setMateInformationOnSupplementalAlignment. That encodes an unstated assumption — that the supplementary occupies the same place in the template geometry as its own primary, same reference and same side of the mate — which is false by construction for the split alignments this path exists to handle.

Two failure modes

Cross-reference. A supplementary mapped to a different reference than its mate gets a non-zero TLEN, so the record carries RNAME != RNEXT with TLEN != 0.

Same reference, supplementary beyond its mate. Both sign and magnitude are wrong. With R1 at chr1:1,000 forward, R2 at chr1:1,350 reverse and an R1 supplementary at chr1:5,000, the supplementary received +450 — the primary pair's insert size — when it is the rightmost segment of the template and so must be negative.

Neither case had test coverage: every existing supplementary-TLEN test placed the supplementary on the same reference as its mate and asserted the copied value.

The fix

Compute TLEN from the supplementary's own alignment against the mate primary. compute_insert_size_raw already returns 0 for unmapped and cross-reference pairs, so both failure modes fall out of routing the supplementary path through it — the correct logic was already sitting in the same file, just not wired to this path.

In template.rs it is split into an InsertSizeEnd snapshot plus compute_insert_size_from_ends, so the supplementary loops can compute against the mate primary without holding a second borrow of self.records. No per-record allocation is added on either path.

This also removes a latent ordering dependency: the old code required the primary pair to be fixed first so the supplementary would read an updated TLEN (fgbio carries the same constraint, noted at Bams.scala:111). Computing from coordinates makes the two steps independent.

Why this basis

The SAM specification is silent on supplementary TLEN rather than violated by the old behaviour — hts-specs #522 scoped the definition to primary reads and left non-primary records undefined, and #842 leaves the computation aligner-defined. The case rests on two other grounds:

1. fgumi contradicted itself. compute_insert_size_raw guards cross-reference; the supplementary path did not.
2. Every independent implementation computes from real coordinates. bwa-mem (bwamem.c:887-892) and minibwa (format.c:243-262) compute per emitted record from that record's own 5′ position and emit 0 across references — including for supplementary records. samtools fixmate leaves supplementaries untouched. Only the htsjdk lineage copies the value from a different record.

The existing 5′-based pairwise basis is deliberately retained rather than htslib's chain-wide leftmost-to-rightmost basis: chain-wide extents would also change the primaries' TLEN whenever a supplementary falls outside the pair's span, diverging from bwa on records that all implementations currently agree on.

Scope and divergence

Three sites across two commands: template.rs:533 and :578 (zipper), clip.rs:1004 (clip). The issue as originally filed covered only zipper.

The existing supplementary-TLEN tests were written for fgbio parity and asserted the copied value; they now assert the computed value. This is an intentional divergence from fgbio and Picard MergeBamAlignment on these records until the upstream fix lands — the compare harness will report it.

Root cause is htsjdk SamPairUtil.java:354, introduced in 9e03608a (2014) with behaviour unchanged since. Filed upstream as samtools/htsjdk#1795, tracked for fgbio as fulcrumgenomics/fgbio#1165.

Tests

Eight existing assertions updated (seven unit, one integration), each previously asserting the copied value. Eight new rstest cases added across zipper and clip covering cross-reference, beyond-mate, before-mate, and coincident-5′-end geometries; the case tables seed a sentinel TLEN so a regression cannot pass by coincidence.

Worth noting that test_clip_command_threads_mode_supplementary_mate_repair already had exactly the geometry at issue — a supplementary 5 kb from its mate — and was asserting +298 where the correct value is -4604. Nothing was checking supplementary TLEN correctly.

cargo ci-fmt, cargo ci-lint, and cargo ci-test all pass (6898 tests).

---

Investigation and reproduction assisted by Claude Code (Anthropic). All code references and the htsjdk 5.0.0 reproduction cited in #673 were verified by hand.

Summary by CodeRabbit

• Bug Fixes
  • Corrected template-length calculations for supplementary alignments.
  • Supplementary records now calculate lengths from their own coordinates and their mate’s primary alignment.
  • Improved handling for alignments on different references, varied positions, strand orientation, and multiple supplementary records.
  • Preserved existing mate flags and mapping metadata behavior.

[coderabbitai]

Note

Reviews paused

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

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: f1fd28b8-c6f6-4132-9424-ceaa0bbcc9d5

📥 Commits

Reviewing files that changed from the base of the PR and between f0669a1 and 563875b.

📒 Files selected for processing (4)

• src/lib/commands/clip.rs
• src/lib/commands/zipper.rs
• src/lib/template.rs
• tests/integration/test_clip_command.rs

---

Walkthrough

Supplementary TLEN is now computed from each supplementary alignment and its mate primary. Insert-size calculation handles strand orientation, unmapped mates, cross-reference pairs, overflow, and stale TLEN values.

Changes

Supplementary TLEN recalculation

Layer / File(s)	Summary
Coordinate-based insert-size computation src/lib/template.rs	Adds reusable alignment-end snapshots and computes TLEN from 5′ coordinates with unmapped, cross-reference, and overflow handling.
Clip supplementary mate repair src/lib/commands/clip.rs	Computes TLEN from each supplementary alignment and its mate snapshot while preserving existing mate metadata updates.
Supplementary TLEN validation src/lib/template.rs, src/lib/commands/clip.rs, src/lib/commands/zipper.rs, tests/integration/test_clip_command.rs	Expands tests for positions, strand orientation, reference mismatches, multiple supplementaries, stale values, and integration outputs.

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

Possibly related issues

• fulcrumgenomics/fgbio issue 1165: Tracks the same supplementary-alignment TLEN calculation defect.

Possibly related PRs

• fulcrumgenomics/fgumi#582: Modifies the clipping and supplementary mate-repair flow in clip.rs.

Sequence Diagram(s)

sequenceDiagram
  participant Alignment as Alignment record
  participant MateRepair as Template or clip mate repair
  participant InsertSize as compute_insert_size_from_ends
  participant MatePrimary as Mate primary

  Alignment->>MateRepair: Provide supplementary coordinates
  MateRepair->>MatePrimary: Read mate-primary coordinates
  MateRepair->>InsertSize: Compute strand-aware TLEN
  InsertSize-->>MateRepair: Return TLEN
  MateRepair-->>Alignment: Store independently computed TLEN

Loading

🚥 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 identifies the main change: computing supplementary TLEN in zipper and clip instead of copying the mate primary value.
Linked Issues check	✅ Passed	The changes satisfy issue #673 by computing supplementary TLEN from coordinates, handling unmapped and cross-reference pairs, preserving 5′ logic, and adding coverage.
Out of Scope Changes check	✅ Passed	All code and test changes support the linked issue by updating shared TLEN logic, zipper and clip paths, and related assertions.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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 673/nhomer/fix-supplementary-tlen

---

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

[nh13]

@coderabbitai pause

[coderabbitai]

✅ Action performed

Reviews paused.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.95%. Comparing base (3d27019) to head (563875b).
⚠️ Report is 10 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff            @@
##             main     #684    +/-   ##
========================================
  Coverage   93.94%   93.95%            
========================================
  Files         178      178            
  Lines      108058   108166   +108     
========================================
+ Hits       101518   101630   +112     
+ Misses       6540     6536     -4     

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

[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 `@src/lib/template.rs`:
- Around line 823-880: Extract the shared htsjdk-style insert-size calculation
over ref_id, five_prime, and is_unmapped into a common function reusable by
template.rs and clip.rs. In src/lib/template.rs:823-880, have
compute_insert_size_from_ends delegate to it while preserving snapshot
conversion; in src/lib/commands/clip.rs:856-933, replace
compute_insert_size_raw’s duplicated formula with the same helper and retain
MateSnap only for mapq and cigar data.

🪄 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: 18b2bceb-a307-4882-bbe5-aadd48246ca5

📥 Commits

Reviewing files that changed from the base of the PR and between 08458ba and f0669a1.

📒 Files selected for processing (4)

• src/lib/commands/clip.rs
• src/lib/commands/zipper.rs
• src/lib/template.rs
• tests/integration/test_clip_command.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.