fix(consensus): clamp consensus depth at i16::MAX instead of wrapping u16 by nh13 · Pull Request #448 · fulcrumgenomics/fgumi

nh13 · 2026-06-20T03:48:16Z

Summary

On very deep UMI families, fgumi silently overflowed the consensus depth counter, corrupting the cD/cM/cE consensus tags and — because the error rate is errors / depth — changing a real filter keep/drop decision relative to fgbio. This makes fgumi clamp consensus depth at i16::MAX (32_767) exactly as fgbio does.

How it was found

Benchmarking fgumi against fgbio 4.0.0 on a deep cfDNA panel (idt-cfdna), the filter-stage BAM equivalency checks differed for every consensus strategy (simplex edit/identity/adjacency and duplex). The molecule MI:40915 has 277,312 raw reads (~138,656 per read-end). fgumi reported cD = 7_584, which is exactly 138_656 mod 65_536 — a u16 wraparound. fgbio reported cD = 32_767 (Short.MaxValue).

Root cause

ConsensusBaseBuilder::observations was [u16; 4], incremented with += 1, and the per-base depth/error vectors were Vec<u16>. A single position deeper than 65_535 wrapped the counter (a debug build panics with "attempt to add with overflow"; the release build that runs in production wraps silently). The wrapped depth then:

corrupts the per-read cD/cM (max/min depth) and the per-base cd array;
corrupts cE (errors / depth), which in the MI:40915 case inflated the error rate past --max-read-error-rate, so fgumi dropped a consensus read that fgbio kept (the 52_756 vs 52_758 record-count difference observed in the benchmark).

This is not a v0.3.1 regression — the u16 types predate v0.3.0; the path was simply never exercised until a family exceeded 65_535 reads.

Fix

fgbio's VanillaUmiConsensusCaller counts depth in Int and clamps the per-base depth/error to Short.MaxValue when storing, deriving cD/cM/cE from the clamped arrays (VanillaUmiConsensusCaller.scala:324-340,380-382). This mirrors it:

Widen observations / contributions() / observations_for_base() / all_observations() from u16 to u32 so the counter never wraps.
Clamp the per-base depth and error to i16::MAX when storing, so cD/cM/cE and the per-base cd/ce arrays all match fgbio. The unclamped depth is still used for the min-reads threshold, exactly as fgbio does (VanillaUmiConsensusCaller.scala:329).

Duplex needs no change. It builds per-strand consensus through the (now-corrected) vanilla caller and sums the two clamped strand depths as i32 without re-clamping — which is exactly what fgbio does (DuplexConsensusCaller.scala:480-489). Its error accumulation already clamps to i16::MAX.

Test-helper fix

The change also completes the test-only aux-tag scanner find_i16_array_tag_in_aux (in fgumi-raw-bam/testutil) to skip the unsigned C/S/I (and A/H) BAM aux types. It previously handled only c/s/i/f/Z, so when cD serializes as S (which a value of 32_767 now does) the scanner hit _ => break and never reached the per-base array tag. The production BAM reader handles all aux types; this only affected the test helper, and the new test surfaced it.

Tests

base_builder: 70_000 single-base observations no longer wrap contributions() (was 7_584 via mod 65_536).
vanilla_caller: a 40_000-read family (one disagreeing observation) reports cD = cM = 32_767 and cE = 1 / (32_767 * read_len) — i.e. computed from the clamped depth, matching fgbio.

cargo ci-test (2,214 tests), cargo ci-fmt, and cargo ci-lint (clippy pedantic, -D warnings) all pass.

Summary by CodeRabbit

Bug Fixes
- Prevented wraparound when counting observations in very deep UMI families.
- Updated consensus per-base depth/error handling to saturate at the expected i16-compatible limit, matching fgbio semantics.
Changes
- Widened per-base observation counters returned by the consensus base builder from u16 to u32.
Tests
- Added coverage for saturation/overflow boundaries.
- Expanded test utilities to handle more array tag element types.

coderabbitai · 2026-06-20T03:48:22Z

Important

Review skipped

No new commits to review since the last review.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8ed3d9c0-d40f-442b-9eff-2842da6b340d

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

Walkthrough

Per-base observation counters in ConsensusBaseBuilder are widened from u16 to u32 to prevent wraparound in very deep UMI families. The multi-read consensus path in vanilla_caller now clamps depth and error counts to 32,767 (i16::MAX) before writing cD/cM tags, matching fgbio's saturation behavior. A test utility cursor fix enables correct parsing of additional BAM B-array subtypes.

Changes

Deep UMI family overflow fix

Layer / File(s)	Summary
Widen ConsensusBaseBuilder counters to u32 `crates/fgumi-consensus/src/base_builder.rs`	Internal `obs` array changed from `[u16; 4]` to `[u32; 4]`; `contributions()`, `observations_for_base()`, and `all_observations()` return `u32`; new test asserts no overflow at 70,000 observations.
Clamp depth/errors to i16::MAX in tag emission `crates/fgumi-consensus/src/vanilla_caller.rs`	Multi-read loop clamps `depth` and `error_count` to 32,767 via `try_from(...).unwrap_or(...).min(...)` before pushing to `depths`/`errors`; new test verifies `CD`/`CM`/`cd`/`CE` saturate at fgbio's boundary.
Fix aux byte-type cursor arithmetic `crates/fgumi-raw-bam/src/testutil.rs`	`find_i16_array_tag_in_aux` extended to handle `A`/`C` (1-byte), `S` (2-byte), `I` (4-byte), and `H` subtypes, correcting cursor advancement when skipping non-matching `B`-array tags.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

fulcrumgenomics/fgumi#148: Modifies the same contributions()/observations_for_base() callsites in vanilla_caller.rs for deriving error_count, directly interacting with the counter types widened in this PR.
fulcrumgenomics/fgumi#213: Changes how consensus integer tag values (including depth/error) are encoded into BAM, overlapping with this PR's cD/cM saturation semantics.
fulcrumgenomics/fgumi#423: Aligns per-base CD_*/CE_* tag generation and i16-style depth saturation for fgumi filter, compatible with this PR's consensus record emission.

Suggested labels

bug

🚥 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 captures the main change: widening observation counters from u16 to u32 and clamping consensus depth at i16::MAX to prevent wraparound overflow in deep UMI families.
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.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch nh/fix-consensus-depth-u16-overflow

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

codecov · 2026-06-20T03:51:05Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.05%. Comparing base (feebca3) to head (4702609).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #448      +/-   ##
==========================================
- Coverage   91.12%   91.05%   -0.08%     
==========================================
  Files          78       78              
  Lines       51414    51324      -90     
==========================================
- Hits        46851    46732     -119     
- Misses       4563     4592      +29

☔ 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-06-20T13:47:11Z

@coderabbitai review

coderabbitai · 2026-06-20T13:47:17Z

✅ 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-06-20T14:42:03Z

@coderabbitai review

coderabbitai · 2026-06-20T14:42:10Z

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

… u16 The per-base observation counter in `ConsensusBaseBuilder` was a `u16`, and the per-base consensus depth and error counts were stored as `u16`. On very deep UMI families — high-duplication cfDNA can exceed 65_535 reads at a single position — the observation counter wrapped modulo 65_536 (e.g. a 138_656-read family reported depth 7_584). The wrapped depth corrupts the cD/cM/cE consensus tags and, because cE = errors / depth, can push a read's error rate past the filter threshold and silently change a keep/drop decision relative to fgbio. fgbio's VanillaUmiConsensusCaller counts depth in `Int` and clamps the per-base depth/error to `Short.MaxValue` (32_767), deriving cD/cM/cE from the clamped arrays. Mirror that exactly: - Widen `ConsensusBaseBuilder::observations` (and `contributions()`, `observations_for_base()`, `all_observations()`) from `u16` to `u32` so the counter never wraps. - Clamp the per-base depth and error to `i16::MAX` when storing them, so cD/cM (max/min depth), cE (errors / depth) and the per-base cd/ce arrays all match fgbio. The unclamped depth is still used for the min-reads threshold, as fgbio does (VanillaUmiConsensusCaller.scala:329). Duplex needs no change: it builds per-strand consensus via the vanilla caller (now corrected) and sums the two clamped strand depths exactly as fgbio does (DuplexConsensusCaller.scala:480), without re-clamping the combined value. Also completes the test-only aux-tag scanner in `testutil` (`find_i16_array_tag_in_aux`) to skip the unsigned `C`/`S`/`I` (and `A`/`H`) BAM aux types. It previously handled only `c`/`s`/`i`, so a preceding tag written as `S` (which is how `cD`=32_767 now serializes) made the scanner stop before reaching the per-base array tag. Surfaced by the new test. Tests: a deep family (70_000 observations) no longer wraps `contributions()`, and a 40_000-read family reports cD=cM=32_767 with cE computed from the clamped depth — matching fgbio.

nh13 · 2026-06-20T17:19:11Z

@coderabbitai review

coderabbitai · 2026-06-20T17:19:20Z

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

…den #448) feat-runall already avoids the u16 wrap by saturating the observation counter at u16::MAX (rather than #448's u32 widening) and clamps the per-base cd/ce arrays to i16::MAX. But it computes the SUMMARY tags cD/cM (max/min depth) and cE (errors/depth) from the UNCLAMPED depths/errors vectors, so on a UMI family deeper than i16::MAX (32_767) at a position the summary tags diverge from fgbio (which stores depths/errors as `Short` and derives cD/cM/cE from those clamped arrays). High-duplication cfDNA can exceed 32_767 reads at a single position, and because cE = errors / depth a divergent denominator can push a read's error rate past the filter threshold and silently change a keep/drop decision relative to fgbio. Clamp at push so the depths/errors vectors — and everything derived from them (the summary cD/cM/cE tags, the per-base cd/ce tags, and duplex's per-strand tags + combined strand-sum) — match fgbio. The unclamped `depth` local is still used for the min-reads threshold (as fgbio does, VanillaUmiConsensusCaller.scala:329); min-reads keys off source_reads.len(), so it is unaffected either way. Duplex needs no separate change: it derives its per-strand aD/aM/bD/bM and cd/ce arrays from the vanilla `ConsensusResult` depths (now clamped) and sums the two clamped strand depths into the combined cD/cM without re-clamping, exactly as fgbio's DuplexConsensusCaller.scala:480. Also completes the test-only aux-tag scanner in `testutil` (`find_i16_array_tag_in_aux`) to skip the unsigned `C`/`S`/`I` (and `A`/`H`) BAM aux scalar types — it previously handled only `c`/`s`/`i`/`f`, so a preceding tag written as an unsigned scalar made the scanner stop before reaching the per-base array tag. Surfaced by the new test. Adds a deep-family (40_000-read) regression test asserting cD=cM=32_767 with cE computed from the clamped depth — verified to fail before the fix ("cD must saturate at i16::MAX"). Ported from the merged #448 test.

…den #448) feat-runall already avoids the u16 wrap by saturating the observation counter at u16::MAX (rather than #448's u32 widening) and clamps the per-base cd/ce arrays to i16::MAX. But it computes the SUMMARY tags cD/cM (max/min depth) and cE (errors/depth) from the UNCLAMPED depths/errors vectors, so on a UMI family deeper than i16::MAX (32_767) at a position the summary tags diverge from fgbio (which stores depths/errors as `Short` and derives cD/cM/cE from those clamped arrays). High-duplication cfDNA can exceed 32_767 reads at a single position, and because cE = errors / depth a divergent denominator can push a read's error rate past the filter threshold and silently change a keep/drop decision relative to fgbio. Clamp at push so the depths/errors vectors — and everything derived from them (the summary cD/cM/cE tags, the per-base cd/ce tags, and duplex's per-strand tags + combined strand-sum) — match fgbio. The unclamped `depth` local is still used for the min-reads threshold (as fgbio does, VanillaUmiConsensusCaller.scala:329); min-reads keys off source_reads.len(), so it is unaffected either way. Duplex needs no separate change: it derives its per-strand aD/aM/bD/bM and cd/ce arrays from the vanilla `ConsensusResult` depths (now clamped) and sums the two clamped strand depths into the combined cD/cM without re-clamping, exactly as fgbio's DuplexConsensusCaller.scala:480. Also completes the test-only aux-tag scanner in `testutil` (`find_i16_array_tag_in_aux`) to skip the unsigned `C`/`S`/`I` (and `A`/`H`) BAM aux scalar types — it previously handled only `c`/`s`/`i`/`f`, so a preceding tag written as an unsigned scalar made the scanner stop before reaching the per-base array tag. Surfaced by the new test. Adds a deep-family (40_000-read) regression test asserting cD=cM=32_767 with cE computed from the clamped depth — verified to fail before the fix ("cD must saturate at i16::MAX"). Ported from the merged #448 test. Cover the error half of the clamp. The depth test has one disagreeing observation, so errors.push(error_count.min(max_short)) was never exercised -- reverting just that .min() left it green. Saturating errors needs depth above 2 * i16::MAX, since the error count is depth minus the majority base's observations: 70,000 reads split 36,000 A / 34,000 C at position 0 puts 34,000 errors there. The oracle is fgbio's algorithm rather than fgumi's output -- summing the clamped arrays gives cE = 32,767 / (32,767 * 4) = 0.25 exactly, where an unclamped stored error gives 34,000 / 131,068 = 0.2594. The per-base ce array is clamped again when the tag is written, so the summary cE is the only tag that distinguishes the two; verified the test fails at 0.2594 with the error clamp reverted. Bounds-check the aux scan in testutil. find_i16_array_tag_in_aux advanced its cursor and indexed scalar, B-array, and Z/H payloads without checking them against the end of the block, so truncated or malformed aux bytes panicked instead of returning None. Every step now goes through a checked advance, an unknown B subtype fails closed rather than resyncing the cursor on a guessed element width, and an unterminated Z/H string is malformed rather than an implicit end-of-block. Adds a case table over the truncation shapes.

…den #448) feat-runall already avoids the u16 wrap by saturating the observation counter at u16::MAX (rather than #448's u32 widening) and clamps the per-base cd/ce arrays to i16::MAX. But it computes the SUMMARY tags cD/cM (max/min depth) and cE (errors/depth) from the UNCLAMPED depths/errors vectors, so on a UMI family deeper than i16::MAX (32_767) at a position the summary tags diverge from fgbio (which stores depths/errors as `Short` and derives cD/cM/cE from those clamped arrays). High-duplication cfDNA can exceed 32_767 reads at a single position, and because cE = errors / depth a divergent denominator can push a read's error rate past the filter threshold and silently change a keep/drop decision relative to fgbio. Clamp at push so the depths/errors vectors — and everything derived from them (the summary cD/cM/cE tags, the per-base cd/ce tags, and duplex's per-strand tags + combined strand-sum) — match fgbio. The unclamped `depth` local is still used for the min-reads threshold (as fgbio does, VanillaUmiConsensusCaller.scala:329); min-reads keys off source_reads.len(), so it is unaffected either way. Duplex needs no separate change: it derives its per-strand aD/aM/bD/bM and cd/ce arrays from the vanilla `ConsensusResult` depths (now clamped) and sums the two clamped strand depths into the combined cD/cM without re-clamping, exactly as fgbio's DuplexConsensusCaller.scala:480. Also completes the test-only aux-tag scanner in `testutil` (`find_i16_array_tag_in_aux`) to skip the unsigned `C`/`S`/`I` (and `A`/`H`) BAM aux scalar types — it previously handled only `c`/`s`/`i`/`f`, so a preceding tag written as an unsigned scalar made the scanner stop before reaching the per-base array tag. Surfaced by the new test. Adds a deep-family (40_000-read) regression test asserting cD=cM=32_767 with cE computed from the clamped depth — verified to fail before the fix ("cD must saturate at i16::MAX"). Ported from the merged #448 test. Cover the error half of the clamp. The depth test has one disagreeing observation, so errors.push(error_count.min(max_short)) was never exercised -- reverting just that .min() left it green. Saturating errors needs depth above 2 * i16::MAX, since the error count is depth minus the majority base's observations: 70,000 reads split 36,000 A / 34,000 C at position 0 puts 34,000 errors there. The oracle is fgbio's algorithm rather than fgumi's output -- summing the clamped arrays gives cE = 32,767 / (32,767 * 4) = 0.25 exactly, where an unclamped stored error gives 34,000 / 131,068 = 0.2594. The per-base ce array is clamped again when the tag is written, so the summary cE is the only tag that distinguishes the two; verified the test fails at 0.2594 with the error clamp reverted. Bounds-check the aux scan in testutil. find_i16_array_tag_in_aux advanced its cursor and indexed scalar, B-array, and Z/H payloads without checking them against the end of the block, so truncated or malformed aux bytes panicked instead of returning None. Every step now goes through a checked advance, an unknown B subtype fails closed rather than resyncing the cursor on a guessed element width, and an unterminated Z/H string is malformed rather than an implicit end-of-block. Adds a case table over the truncation shapes. Assert the saturation tests against a programmatic fgbio oracle rather than hand-computed constants. fgbio_summary_tags_baseline reimplements fgbio's derivation -- store each per-position depth and error as a Short, then take cD as the max depth, cM as the min, and cE as sum(errors)/sum(depths) over those clamped arrays -- with no fgbio dependency, the same shape as fgbio_edit_baseline_partition in fgumi-umi. Both saturation tests now compare against the oracle applied to their fixture's known pileup. Re-verified non-vacuous: reverting the error clamp still fails at 0.2594 against the oracle's 0.25. Reject --min-reads above the depth ceiling. The threshold is compared against a u16 per-position depth that saturates at u16::MAX, so any larger value can never be met and would silently no-call every position of an arbitrarily deep family as NotEnoughReads. Simplex is the only reachable path -- duplex and codec pin their single-strand caller's min_reads to 1, so their own min_reads is a strand-count filter rather than this threshold. u16::MAX itself stays accepted; it is exactly reachable. Add proptest coverage for the aux scanner. The case table pins the truncation shapes worth naming; the property tests cover the ones nobody thought of -- arbitrary byte sequences must never panic and must not yield an array larger than the block it came from, and well-formed B:s arrays must still round-trip, so the bounds checks cannot drift into rejecting valid input.

…den #448) feat-runall already avoids the u16 wrap by saturating the observation counter at u16::MAX (rather than #448's u32 widening) and clamps the per-base cd/ce arrays to i16::MAX. But it computes the SUMMARY tags cD/cM (max/min depth) and cE (errors/depth) from the UNCLAMPED depths/errors vectors, so on a UMI family deeper than i16::MAX (32_767) at a position the summary tags diverge from fgbio (which stores depths/errors as `Short` and derives cD/cM/cE from those clamped arrays). High-duplication cfDNA can exceed 32_767 reads at a single position, and because cE = errors / depth a divergent denominator can push a read's error rate past the filter threshold and silently change a keep/drop decision relative to fgbio. Clamp at push so the depths/errors vectors — and everything derived from them (the summary cD/cM/cE tags, the per-base cd/ce tags, and duplex's per-strand tags + combined strand-sum) — match fgbio. The unclamped `depth` local is still used for the min-reads threshold (as fgbio does, VanillaUmiConsensusCaller.scala:329); min-reads keys off source_reads.len(), so it is unaffected either way. Duplex needs no separate change: it derives its per-strand aD/aM/bD/bM and cd/ce arrays from the vanilla `ConsensusResult` depths (now clamped) and sums the two clamped strand depths into the combined cD/cM without re-clamping, exactly as fgbio's DuplexConsensusCaller.scala:480. Also completes the test-only aux-tag scanner in `testutil` (`find_i16_array_tag_in_aux`) to skip the unsigned `C`/`S`/`I` (and `A`/`H`) BAM aux scalar types — it previously handled only `c`/`s`/`i`/`f`, so a preceding tag written as an unsigned scalar made the scanner stop before reaching the per-base array tag. Surfaced by the new test. Adds a deep-family (40_000-read) regression test asserting cD=cM=32_767 with cE computed from the clamped depth — verified to fail before the fix ("cD must saturate at i16::MAX"). Ported from the merged #448 test. Cover the error half of the clamp. The depth test has one disagreeing observation, so errors.push(error_count.min(max_short)) was never exercised -- reverting just that .min() left it green. Saturating errors needs depth above 2 * i16::MAX, since the error count is depth minus the majority base's observations: 70,000 reads split 36,000 A / 34,000 C at position 0 puts 34,000 errors there. The oracle is fgbio's algorithm rather than fgumi's output -- summing the clamped arrays gives cE = 32,767 / (32,767 * 4) = 0.25 exactly, where an unclamped stored error gives 34,000 / 131,068 = 0.2594. The per-base ce array is clamped again when the tag is written, so the summary cE is the only tag that distinguishes the two; verified the test fails at 0.2594 with the error clamp reverted. Bounds-check the aux scan in testutil. find_i16_array_tag_in_aux advanced its cursor and indexed scalar, B-array, and Z/H payloads without checking them against the end of the block, so truncated or malformed aux bytes panicked instead of returning None. Every step now goes through a checked advance, an unknown B subtype fails closed rather than resyncing the cursor on a guessed element width, and an unterminated Z/H string is malformed rather than an implicit end-of-block. Adds a case table over the truncation shapes. Assert the saturation tests against a programmatic fgbio oracle rather than hand-computed constants. fgbio_summary_tags_baseline reimplements fgbio's derivation -- store each per-position depth and error as a Short, then take cD as the max depth, cM as the min, and cE as sum(errors)/sum(depths) over those clamped arrays -- with no fgbio dependency, the same shape as fgbio_edit_baseline_partition in fgumi-umi. Both saturation tests now compare against the oracle applied to their fixture's known pileup. Re-verified non-vacuous: reverting the error clamp still fails at 0.2594 against the oracle's 0.25. Reject --min-reads above the depth ceiling. The threshold is compared against a u16 per-position depth that saturates at u16::MAX, so any larger value can never be met and would silently no-call every position of an arbitrarily deep family as NotEnoughReads. Simplex is the only reachable path -- duplex and codec pin their single-strand caller's min_reads to 1, so their own min_reads is a strand-count filter rather than this threshold. u16::MAX itself stays accepted; it is exactly reachable. Add proptest coverage for the aux scanner. The case table pins the truncation shapes worth naming; the property tests cover the ones nobody thought of -- arbitrary byte sequences must never panic and must not yield an array larger than the block it came from, and well-formed B:s arrays must still round-trip, so the bounds checks cannot drift into rejecting valid input. # Conflicts: # src/lib/commands/simplex.rs

…den #448) (#555) feat-runall already avoids the u16 wrap by saturating the observation counter at u16::MAX (rather than #448's u32 widening) and clamps the per-base cd/ce arrays to i16::MAX. But it computes the SUMMARY tags cD/cM (max/min depth) and cE (errors/depth) from the UNCLAMPED depths/errors vectors, so on a UMI family deeper than i16::MAX (32_767) at a position the summary tags diverge from fgbio (which stores depths/errors as `Short` and derives cD/cM/cE from those clamped arrays). High-duplication cfDNA can exceed 32_767 reads at a single position, and because cE = errors / depth a divergent denominator can push a read's error rate past the filter threshold and silently change a keep/drop decision relative to fgbio. Clamp at push so the depths/errors vectors — and everything derived from them (the summary cD/cM/cE tags, the per-base cd/ce tags, and duplex's per-strand tags + combined strand-sum) — match fgbio. The unclamped `depth` local is still used for the min-reads threshold (as fgbio does, VanillaUmiConsensusCaller.scala:329); min-reads keys off source_reads.len(), so it is unaffected either way. Duplex needs no separate change: it derives its per-strand aD/aM/bD/bM and cd/ce arrays from the vanilla `ConsensusResult` depths (now clamped) and sums the two clamped strand depths into the combined cD/cM without re-clamping, exactly as fgbio's DuplexConsensusCaller.scala:480. Also completes the test-only aux-tag scanner in `testutil` (`find_i16_array_tag_in_aux`) to skip the unsigned `C`/`S`/`I` (and `A`/`H`) BAM aux scalar types — it previously handled only `c`/`s`/`i`/`f`, so a preceding tag written as an unsigned scalar made the scanner stop before reaching the per-base array tag. Surfaced by the new test. Adds a deep-family (40_000-read) regression test asserting cD=cM=32_767 with cE computed from the clamped depth — verified to fail before the fix ("cD must saturate at i16::MAX"). Ported from the merged #448 test. Cover the error half of the clamp. The depth test has one disagreeing observation, so errors.push(error_count.min(max_short)) was never exercised -- reverting just that .min() left it green. Saturating errors needs depth above 2 * i16::MAX, since the error count is depth minus the majority base's observations: 70,000 reads split 36,000 A / 34,000 C at position 0 puts 34,000 errors there. The oracle is fgbio's algorithm rather than fgumi's output -- summing the clamped arrays gives cE = 32,767 / (32,767 * 4) = 0.25 exactly, where an unclamped stored error gives 34,000 / 131,068 = 0.2594. The per-base ce array is clamped again when the tag is written, so the summary cE is the only tag that distinguishes the two; verified the test fails at 0.2594 with the error clamp reverted. Bounds-check the aux scan in testutil. find_i16_array_tag_in_aux advanced its cursor and indexed scalar, B-array, and Z/H payloads without checking them against the end of the block, so truncated or malformed aux bytes panicked instead of returning None. Every step now goes through a checked advance, an unknown B subtype fails closed rather than resyncing the cursor on a guessed element width, and an unterminated Z/H string is malformed rather than an implicit end-of-block. Adds a case table over the truncation shapes. Assert the saturation tests against a programmatic fgbio oracle rather than hand-computed constants. fgbio_summary_tags_baseline reimplements fgbio's derivation -- store each per-position depth and error as a Short, then take cD as the max depth, cM as the min, and cE as sum(errors)/sum(depths) over those clamped arrays -- with no fgbio dependency, the same shape as fgbio_edit_baseline_partition in fgumi-umi. Both saturation tests now compare against the oracle applied to their fixture's known pileup. Re-verified non-vacuous: reverting the error clamp still fails at 0.2594 against the oracle's 0.25. Reject --min-reads above the depth ceiling. The threshold is compared against a u16 per-position depth that saturates at u16::MAX, so any larger value can never be met and would silently no-call every position of an arbitrarily deep family as NotEnoughReads. Simplex is the only reachable path -- duplex and codec pin their single-strand caller's min_reads to 1, so their own min_reads is a strand-count filter rather than this threshold. u16::MAX itself stays accepted; it is exactly reachable. Add proptest coverage for the aux scanner. The case table pins the truncation shapes worth naming; the property tests cover the ones nobody thought of -- arbitrary byte sequences must never panic and must not yield an array larger than the block it came from, and well-formed B:s arrays must still round-trip, so the bounds checks cannot drift into rejecting valid input. # Conflicts: # src/lib/commands/simplex.rs

nh13 temporarily deployed to github-actions June 20, 2026 03:48 — with GitHub Actions Inactive

nh13 force-pushed the nh/fix-consensus-depth-u16-overflow branch from fe6f0c2 to 7f0d170 Compare June 20, 2026 03:51

nh13 temporarily deployed to github-actions June 20, 2026 03:51 — with GitHub Actions Inactive

nh13 force-pushed the nh/fix-consensus-depth-u16-overflow branch from 7f0d170 to 4702609 Compare June 20, 2026 17:18

nh13 temporarily deployed to github-actions June 20, 2026 17:18 — with GitHub Actions Inactive

nh13 merged commit 0882cd3 into main Jun 20, 2026
10 checks passed

nh13 deleted the nh/fix-consensus-depth-u16-overflow branch June 20, 2026 22:33

nh13 mentioned this pull request Jun 20, 2026

chore: release v0.4.0 #442

Merged

coderabbitai Bot mentioned this pull request Jun 25, 2026

fix(runall)!: CLI surface and validation parity; consensus depth/tie-break hardening (5/6) #462

Merged

nh13 mentioned this pull request Jul 10, 2026

fix(consensus): clamp summary cD/cM/cE at i16::MAX like fgbio (re-harden #448) #555

Merged

Navigation Menu

Repository navigation

fix(consensus): clamp consensus depth at i16::MAX instead of wrapping u16 - #448

fix(consensus): clamp consensus depth at i16::MAX instead of wrapping u16#448
nh13 merged 1 commit into
mainfrom
nh/fix-consensus-depth-u16-overflow

nh13 commented Jun 20, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Review skipped

Uh oh!

codecov Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

nh13 commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

nh13 commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

Uh oh!

nh13 commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 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 Jun 20, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

How it was found

Root cause

Fix

Test-helper fix

Tests

Summary by CodeRabbit

Uh oh!

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

Uh oh!

Review skipped

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested labels

Uh oh!

codecov Bot commented Jun 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

nh13 commented Jun 20, 2026

Uh oh!

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

Uh oh!

Uh oh!

nh13 commented Jun 20, 2026

Uh oh!

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

Uh oh!

Uh oh!

nh13 commented Jun 20, 2026

Uh oh!

coderabbitai Bot commented Jun 20, 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 Jun 20, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

codecov Bot commented Jun 20, 2026 •

edited

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading

coderabbitai Bot commented Jun 20, 2026 •

edited

Loading