[nh13]

Companion to fulcrumgenomics/fgbio#1168, which fixes the same defect in the Scala implementation.

Problem

Unmapped reads contribute bases and depth to consensus reads, with no alignment supporting them.

fgumi group --allow-unmapped deliberately supports wholly unmapped templates (ribosome display and similar), but its gate is on templates with both reads unmapped, so a half-mapped pair is kept regardless of that flag. The consensus pre-group filter then kept any record that was mapped or had a mapped mate, which admitted the unmapped end too.

That end has no cigar, and is_cigar_prefix returns true for an empty left hand side — a.len() > b.len() is false, last_index = 0.saturating_sub(1), and the loop body never runs:

pub fn is_cigar_prefix(a: &[(Kind, usize)], b: &[(Kind, usize)]) -> bool {
    if a.len() > b.len() { return false; }
    let last_index = a.len().saturating_sub(1);
    for (i, &(op_a, len_a)) in a.iter().enumerate() { ... }
    true
}

So in select_most_common_alignment_group an empty cigar matches every alignment group instead of forming one of its own, and the read is retained rather than rejected. Whether it is retained at all depends on where it lands in the length-descending sort, so the outcome turns on a read-length tiebreak.

Observed for a molecule of three read pairs where one pair has an unmapped R2:

Command	Symptom
simplex	cD=3 for R2 where only two reads are eligible, and the unmapped read's bases reach the consensus
duplex	aD=3 for the AB single-strand consensus of R2 where only two AB-R2s are mapped
codec	Unaffected — its alignment grouping is reached only for templates forming a primary FR pair

Fix

Two layers.

1. consensus_pregroup_keep_flags now requires the record to be mapped (src/lib/commands/common.rs). This is fgumi's copy of fgbio's ConsensusCallingIterator filter and is already shared by simplex, duplex and codec, so all three are covered by construction — and it is already parameterised by --allow-unmapped, so the relaxation has a home. The mapped end of a half-mapped pair is unaffected; it is mapped in its own right.

2. Under --allow-unmapped, mapped reads still win. drop_unmapped_if_any_mapped removes unmapped reads from any set being consensus called that also holds a mapped read, and leaves a wholly unmapped set intact. Dropped reads are attributed to RejectionReason::Unmapped — which existed in the enum with no production call site until now — and reach the rejects output.

So: unmapped reads are not consensus input by default; with --allow-unmapped they are admitted but never displace mapped reads.

Behaviour change

This is breaking for input containing half-mapped pairs. A family of such pairs previously produced an R2 consensus built from unaligned reads. It now produces no consensus for that end, and therefore no read pair.

That is deliberate: it restores parity with fgbio, where the same family produces nothing at all. Gating the fallback behind --allow-unmapped rather than applying it unconditionally is what keeps the default in step with fgbio while preserving the wholly-unmapped capability fgbio does not have.

Tests

Written first, watched failing, then fixed — the two commits are test: then fix: in that order, so the first is red by design. Happy to squash if you would rather not carry a red commit.

At the test commit, verified: 3 failing, 2 passing guards.

Test	At test: commit	Purpose
test_simplex_ignores_unmapped_end_when_mapped_reads_present	FAIL	reproduces the defect
test_duplex_ignores_unmapped_end_when_mapped_reads_present	FAIL	reproduces the defect
test_simplex_prefers_mapped_reads_even_when_unmapped_are_allowed	FAIL	pins layer 2, under --allow-unmapped
test_codec_ignores_half_mapped_pair_when_mapped_reads_present	PASS	guards codec's existing FR-pair protection
test_simplex_allow_unmapped_still_consenses_a_wholly_unmapped_family	PASS	guards the --allow-unmapped capability against this change

The simplex tests assert R2's bases as well as its depth. Depth alone can look correct while the consensus is still contaminated — that is how the --max-reads interaction was caught in the fgbio equivalent.

test_consensus_pregroup_keep_flags's unmapped_paired_mate_mapped case is inverted to pin the new behaviour, and a case is added for the mapped end of a half-mapped pair, which is still kept.

After: cargo ci-test 7247 passed, 0 failed; cargo ci-fmt and cargo ci-lint clean.

Not addressed here

The root cause in is_cigar_prefix is untouched. An empty cigar is still a prefix of every cigar. This PR makes that unreachable from the consensus callers by default, and harmless under --allow-unmapped; any future path producing an empty cigar would hit it again.

Risk: output changes for consensus grouping and consensus depth/bases, pinned by mapped-read filtering; sort order, corrected UMIs, and metrics: none. unsafe changes: none, and CLAUDE.md allowlist updates: none. Memory bounds, queue capacity, and thread/backpressure policy: none.

Fix: Require mapped records by default. With --allow-unmapped, retain wholly unmapped families but prefer mapped records when both types exist. Attribute removed records to RejectionReason::Unmapped.

Tests cover simplex, duplex, and CODEC behavior, including half-mapped pairs and mapped-read depth.

[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: a1c5747e-0c6d-406c-9d49-56a0e7f618ad

📥 Commits

Reviewing files that changed from the base of the PR and between 1386d8b and 444e746.

📒 Files selected for processing (5)

• src/lib/commands/codec.rs
• src/lib/commands/common.rs
• src/lib/commands/duplex.rs
• src/lib/commands/simplex.rs
• tests/integration/test_duplex_command.rs

---

Walkthrough

Mixed mapped and unmapped records now retain mapped reads during filtering. Unmapped records receive rejection indices when mapped records exist. Wholly unmapped families remain available when --allow-unmapped is enabled. Integration tests cover CODEC, duplex, and simplex consensus output.

Changes

Mapped-read precedence

Layer / File(s)	Summary
Default pre-group filtering src/lib/commands/common.rs, src/lib/commands/duplex.rs, src/lib/commands/simplex.rs, src/lib/commands/codec.rs	The default filter drops unmapped records, including unmapped ends of half-mapped pairs. Mapped records remain eligible. --allow-unmapped relaxes only the mapped-record rule.
Mixed-group alignment filtering crates/fgumi-consensus/src/vanilla_caller.rs	Mixed groups discard unmapped reads and retain their rejection indices. Wholly unmapped groups remain unchanged.
Mixed-mapping consensus validation tests/integration/test_codec_command.rs, tests/integration/test_duplex_command.rs, tests/integration/test_simplex_command.rs, src/lib/commands/common.rs, src/lib/commands/codec.rs	Integration tests verify mapped-read precedence, per-end depths, --allow-unmapped, and CODEC behavior for wholly unmapped pairs.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: 🔵 Low · up to 444e7

The PR prevents unmapped reads from contaminating consensus while preserving wholly unmapped handling when explicitly enabled. It is mergeable with owner awareness that the shared --allow-unmapped documentation may not accurately describe codec behavior, which could mislead users without undermining the core consensus fix.

Sequence Diagram(s)

sequenceDiagram
  participant InputRecords
  participant PreGroupFilter
  participant AlignmentFilter
  participant ConsensusCaller
  InputRecords->>PreGroupFilter: provide mapped and unmapped records
  PreGroupFilter->>AlignmentFilter: pass records allowed for grouping
  AlignmentFilter->>AlignmentFilter: remove unmapped reads from mixed groups
  AlignmentFilter->>ConsensusCaller: return retained reads and rejection indices
  ConsensusCaller->>InputRecords: produce consensus output

Loading

Possibly related PRs

• fulcrumgenomics/fgumi#509: Refines the same consensus pre-group unmapped-read filtering behavior.
• fulcrumgenomics/fgumi#516: Uses the same Unmapped rejection reason in alignment filtering.
• fulcrumgenomics/fgumi#585: Updates the related CODEC --allow-unmapped integration test.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title uses the required Conventional Commit format and accurately describes the breaking consensus filtering change.
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.

---

_{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 94.22%. Comparing base (ade4853) to head (444e746).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #737      +/-   ##
==========================================
+ Coverage   94.12%   94.22%   +0.09%     
==========================================
  Files         181      186       +5     
  Lines      109415   110979    +1564     
==========================================
+ Hits       102987   104568    +1581     
+ Misses       6428     6411      -17     

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

[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/commands/common.rs`:
- Line 383: Update the comment at src/lib/commands/common.rs:383-383 to describe
that the default pre-group predicate keeps only mapped primary records, removing
references to the obsolete mapped-mate exception. Also update the test
documentation at src/lib/commands/common.rs:2222-2231 to match this default
behavior; no code changes are needed.

🪄 Autofix

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: 67f53720-5911-4c6a-9cb8-c3549484d212

📥 Commits

Reviewing files that changed from the base of the PR and between ade4853 and b41ce9e.

📒 Files selected for processing (5)

• crates/fgumi-consensus/src/vanilla_caller.rs
• src/lib/commands/common.rs
• tests/integration/test_codec_command.rs
• tests/integration/test_duplex_command.rs
• tests/integration/test_simplex_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.

[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/commands/common.rs`:
- Around line 362-363: Update the documentation near the `--allow-unmapped`
option in `common.rs` to remove the claim that fully unmapped input can be
consensus-called universally. Clarify that the unmapped-consensus behavior
applies only to callers that support it, while `CodecConsensusCaller` still
requires mapped primary FR pairs.

🪄 Autofix

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: 9ca745ed-b7d3-4fc7-b3af-e158d945a648

📥 Commits

Reviewing files that changed from the base of the PR and between b41ce9e and 1386d8b.

📒 Files selected for processing (2)

• src/lib/commands/common.rs
• src/lib/commands/duplex.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.