Repository navigation
feat(sam): raw-byte record_utils + RawRecordClipper - #289
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
=======================================
Coverage 90.07% 90.07%
=======================================
Files 124 124
Lines 62247 62247
=======================================
+ Hits 56070 56071 +1
+ Misses 6177 6176 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
nh13
marked this pull request as ready for review
April 17, 2026 22:28
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughThis PR adds low-level BAM auxiliary array tag encoders/writers ( 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nh13
force-pushed
the
nh/raw-bam-api-7-additive-api
branch
from
April 18, 2026 08:32
3ff61b1 to
6a93923
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/fgumi-sam/src/alignment_tags.rs (1)
98-104:⚠️ Potential issue | 🟡 MinorReturn
truewhen tags are written.These mapped no-reference-span records still get
NM/MD/UQupdated, soOk(false)contradicts the documented “false if unmapped” contract.Proposed fix
record.data_mut().insert(nm_tag(), Value::from(0u32)); record.data_mut().insert(md_tag(), Value::String("0".to_owned().into())); record.data_mut().insert(uq_tag(), Value::from(0u32)); - return Ok(false); + return Ok(true);editor.update_int(b"NM", 0); editor.update_int(b"UQ", 0); editor.update_string(b"MD", b"0"); - return Ok(false); + return Ok(true);Also applies to: 299-305
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-sam/src/alignment_tags.rs` around lines 98 - 104, The code treats CIGARs with zero reference-consuming span by writing NM/MD/UQ tags but then returns Ok(false) which contradicts the documented contract that false means unmapped; update the return to Ok(true) after inserting tags so callers see tags were written. Locate the branch that checks ref_span == 0 (and the identical branch at the second occurrence around the later block) and change the final return from Ok(false) to Ok(true) so mapped-but-zero-ref-span records are signaled as handled.crates/fgumi-raw-bam/tests/tags_editor_invariants.rs (1)
1-1:⚠️ Potential issue | 🟡 MinorRestore the unsafe-code deny at this test crate root.
This integration test file is its own crate root, so the lint needs to live here.
Proposed fix
#![cfg(feature = "test-utils")] +#![deny(unsafe_code)]As per coding guidelines,
**/*.rs: Enforce#![deny(unsafe_code)]- only standard library unsafe code is permitted in Rust files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs` at line 1, This test crate root is missing the crate-level deny for unsafe code; add the attribute #![deny(unsafe_code)] at the top of tags_editor_invariants.rs (alongside the existing #![cfg(feature = "test-utils")] attribute) so the test crate enforces the deny(unsafe_code) lint for all Rust files in the crate.crates/fgumi-raw-bam/src/tags.rs (1)
1256-1307:⚠️ Potential issue | 🟠 MajorRestore payload-length checks before in-place writes.
find_tag_positioncan return a matching truncated tag before validating its value bytes. These setters then index or slice past the aux buffer and panic instead of returningfalse.Proposed fix shape
pub fn set_int_in_place(&mut self, tag: &[u8; 2], value: i64) -> bool { let Some((p, val_type)) = find_tag_position(self.0, *tag) else { return false; }; + let needed = match val_type { + b'c' | b'C' => 1, + b's' | b'S' => 2, + b'i' | b'I' => 4, + _ => return false, + }; + if p + 3 + needed > self.0.len() { + return false; + } match val_type {- if val_type != b'f' { + if val_type != b'f' || p + 7 > self.0.len() { return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/tags.rs` around lines 1256 - 1307, find_tag_position may return a tag whose value bytes are truncated, so set_int_in_place and set_float_in_place must verify the aux payload is long enough before any indexing/slicing; update set_int_in_place (matching on b'c','C','s','S','i','I') to check that self.0 has at least p+4 for 1-byte types, p+5 for 2-byte types, and p+7 for 4-byte types and return false if not, and update set_float_in_place to ensure p+7 <= self.0.len() before copying the f32 bytes, avoiding any out-of-bounds indexing into self.0 after calling find_tag_position.crates/fgumi-raw-bam/src/raw_bam_record.rs (1)
707-718:⚠️ Potential issue | 🟡 MinorReject embedded NULs in read names.
set_read_name(b"a\0b")creates a BAM name with multiple terminators; downstream C-string-style readers can see a truncated QNAME whilel_read_namestill spans the full field.Proposed fix
pub fn set_read_name(&mut self, new_name: &[u8]) { + assert!( + !new_name.contains(&0), + "read name must not contain embedded NUL bytes" + ); let new_l = new_name.len() + 1; // include NUL🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/raw_bam_record.rs` around lines 707 - 718, The set_read_name function must reject input containing embedded NUL bytes to avoid creating multiple terminators; add a check in set_read_name that scans new_name for any 0 byte and panic (or return an error consistent with project style) with a clear message like "read name contains embedded NUL" before computing new_l or performing the splice; reference the set_read_name function and the new_name/new_l variables and perform the validation right after entering the function (before constructing replacement and before splice).
🧹 Nitpick comments (5)
crates/fgumi-raw-bam/tests/noodles_roundtrip.rs (1)
61-64: Keep noodles content assertions in these round-trips.These are cross-decoder tests. Counting CIGAR ops and “parse succeeds” would miss raw encoding regressions that preserve shape but corrupt op lengths/kinds, bases, or qualities. Please compare the decoded noodles CIGAR contents and decoded sequence/qualities to the generated inputs.
Also applies to: 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/tests/noodles_roundtrip.rs` around lines 61 - 64, The test currently only checks the number of CIGAR ops; instead decode_with_noodles(&rec) into buf and assert full equality of decoded contents against the generated inputs: compare buf.cigar() (as a Vec or iterator of ops) to the original ops (e.g., proptest::prop_assert_eq!(buf.cigar().iter().cloned().collect::<Vec<_>>(), ops)); compare buf.sequence() (or buf.sequence().to_string()) to the generated seq string (proptest::prop_assert_eq!(buf.sequence(), seq)) and compare buf.qualities() (or converted Vec<u8>) to the generated quals (proptest::prop_assert_eq!(buf.qualities().iter().cloned().collect::<Vec<_>>(), quals)); apply the same detailed comparisons for the other round-trip assertion block that also uses decode_with_noodles/buf so raw encoding changes in op kinds/lengths, bases, or qualities are detected.crates/fgumi-raw-bam/src/noodles_compat.rs (1)
106-112: Single-record decode does a full BAM-stream build per call.
raw_record_to_record_bufdelegates toraw_records_to_record_bufs, which synthesizes a BAM magic + header + block-size wrapper for every call and spins up a freshnoodles::bam::io::Reader— just to decode one record. Fine for occasional use, but if this becomes the hot path for any downstream caller, the overhead stacks up fast. Worth adding a note to the doc comment that this isn't amortized (mirroring theRecordBufEncoderpattern) or building an amortized decoder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/noodles_compat.rs` around lines 106 - 112, raw_record_to_record_buf currently calls raw_records_to_record_bufs which rebuilds a full BAM stream and creates a new noodles::bam::io::Reader for each call, causing heavy per-call overhead; either document this in raw_record_to_record_buf's doc comment (explain it's not amortized and reference RecordBufEncoder) or implement an amortized decoder that reuses the stream/buffer/Reader across calls (e.g., introduce a RawRecordDecoder struct with methods like new(header: &noodles::sam::Header) and decode(&mut self, rec: &crate::RawRecord) -> Result<noodles::sam::alignment::RecordBuf>, which internally reuses the buffer and noodles::bam::io::Reader instead of rebuilding per record). Ensure tests or callers are updated to use the new decoder or the function’s docs are clear about the performance caveat.src/lib/commands/zipper.rs (1)
1280-1280: Replacecigar_ops_vec()allocation withcigar_ops_iter()for single-iteration use.The
cigar_opsvector is allocated but iterated only once in the loop. Userec.cigar_ops_iter()directly in theforloop to avoid the per-record allocation on the hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/zipper.rs` at line 1280, The code currently allocates a Vec via rec.cigar_ops_vec() into cigar_ops but only iterates it once; replace that allocation by iterating the iterator directly using rec.cigar_ops_iter() in the for loop (i.e., remove the let cigar_ops = rec.cigar_ops_vec() binding and use rec.cigar_ops_iter() where the loop over cigar_ops occurs) to avoid per-record allocation on the hot path while keeping the existing loop body and variable names for each operation.crates/fgumi-raw-bam/src/tags.rs (1)
1042-1047: Decode iterator entries directly.
iter_typed()callsget()for every entry, which rescans from the start and can return the first duplicate tag’s value for later duplicate entries. Decode from the currentTagEntry/offset instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/tags.rs` around lines 1042 - 1047, iter_typed currently calls RawTagsView::get(&entry.tag) which rescans and returns the first duplicate tag’s value; fix it by decoding the value directly from each TagEntry yielded by RawTagsView::iter() instead of calling get. Replace the filter_map so it maps each entry -> (entry.tag, decoded_value) by using the entry's offset/bytes and the existing decoding logic (add or reuse a helper such as RawTagsView::decode_value_from_entry or a decode_at_entry method) to produce the TagValue for that specific entry; ensure you use the TagEntry from RawTagsView::iter() and its offset/length to decode rather than RawTagsView::get.crates/fgumi-raw-bam/src/builder.rs (1)
1039-1039: Typo:multple→multiple.✏️ Fix
- fn test_mapped_builder_cigar_multple_ops() { + fn test_mapped_builder_cigar_multiple_ops() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-raw-bam/src/builder.rs` at line 1039, Rename the test function identifier test_mapped_builder_cigar_multple_ops to test_mapped_builder_cigar_multiple_ops to fix the typo; update any references or invocations of that function (e.g., in mod tests or #[test] attribute blocks) to the new name so the test still runs and compiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fgumi-raw-bam/src/builder.rs`:
- Around line 306-331: The clear() method resets bin to 0 which diverges from
the SAM spec for unmapped records; update clear() to set self.bin = 4680 (the
same constant used by UnmappedSamBuilder) and likewise change the
SamBuilder/Builder constructor default (where bin: 0 is currently set) to use
4680 so unmapped-like builders emit the spec-compliant bin by default;
alternatively, if you prefer not to change defaults, add a prominent doc comment
on the bin() setter and the constructor explaining callers should set bin to
4680 for unmapped records referencing the UnmappedSamBuilder constant.
In `@crates/fgumi-raw-bam/src/cigar.rs`:
- Around line 56-64: The functions consumes_ref and consumes_query (and the
similar functions around 1183-1187) can overflow when shifting op_type << 1 for
op_type >= 16; add an explicit bounds check at the start of each public const fn
(e.g., consumes_ref, consumes_query) to return false (or otherwise handle
invalid op_type) when op_type >= 16 before performing the shift, so the shift
never receives >=32 and debug builds won't panic; update the doc/comments to
mention valid op_type range if needed.
In `@crates/fgumi-raw-bam/src/indexed_reader.rs`:
- Around line 107-115: query_unmapped() currently falls back to the current BGZF
cursor when last_first_record_start_position() is None, which can miss unmapped
records after region queries; change the code to cache the first-record virtual
offset during read_header() (e.g. store an Option<VirtualPosition> field like
first_record_start_pos on the IndexedReader) and then in query_unmapped() use
that cached first_record_start_pos as the deterministic fallback before
returning UnmappedIter; update read_header() to set first_record_start_pos and
use inner.get_mut().seek_to_virtual_position(first_record_start_pos) when
last_first_record_start_position() returns None.
In `@crates/fgumi-raw-bam/src/raw_bam_record.rs`:
- Around line 220-223: In tags_mut, aux_data_offset_from_record can return an
offset > self.0.len() causing &mut self.0[off..] to panic; clamp the computed
offset before slicing (similar to RawTagsEditor). Replace the current off
calculation (which uses unwrap_or(len)) with code that takes the returned offset
and bounds it to self.0.len() (e.g., off = returned_offset.min(len) or
usize::min), then call RawTagsMut::new(&mut self.0[off..]) so slicing is always
within the record.
In `@crates/fgumi-raw-bam/src/sequence.rs`:
- Line 228: Replace the conditional debug-only check with an unconditional
precondition by changing the debug_assert!(dst.len() >= packed_len) to an
assert!(dst.len() >= packed_len) so the function (which documents that dst must
be >= bases.len().div_ceil(2)) fails immediately in release builds before any
writes to dst occur; ensure this assert is evaluated before any mutation of dst
or calls that write into it.
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 437-452: The match arm in TagValue::type_byte incorrectly returns
the array element subtype for Self::Array, misclassifying BAM B tags; update
TagValue::type_byte so the Self::Array(_) branch returns the canonical array
type byte b'B' (instead of using a.elem_type), preserving existing branches for
Char, Int, Float, String, and Hex.
- Around line 1318-1320: The slice start offset from
aux_data_offset_from_record(self.as_bytes()) can be beyond the buffer and cause
a panic when calling self.as_bytes_mut()[off..]; clamp the computed off to the
record length before slicing (e.g. let off = off.min(len) or set off = len if
out-of-range) so that RawTagsMut::new(&mut self.as_bytes_mut()[off..]) yields an
empty aux view for truncated records; update the block using
aux_data_offset_from_record, as_bytes(), as_bytes_mut(), and RawTagsMut::new to
perform this clamp.
- Around line 1006-1012: In get(), after calling find_tag_position(self.0, *tag)
and computing start = p + 3, guard that start < aux.len() before indexing
aux[start] for the b'A' branch; if the byte is out of range return None (like
other malformed cases) so TagValue::Char(aux[start]) is only constructed when
the A value byte exists; update the b'A' match arm in the get() method to
perform this bounds check and return None on truncation.
- Around line 3081-3095: The test test_dedup_pa_tag_check_fails_on_b_array
should not assert the known-bad dedup predicate (the OR of find_string_tag and
find_int_tag); instead update it to assert the corrected presence check by
calling find_tag_type(&aux, &pa_tag_bytes).is_some() (or replicate the fixed
dedup logic that recognizes B:i arrays), keeping use of make_b_int_array_tag to
construct the aux. Replace the assertion asserting !found_by_dedup with a
positive assertion that the tag is found (using find_tag_type) and remove the
brittle dedup predicate that mirrors the old logic in dedup.rs.
In `@crates/fgumi-raw-bam/tests/noodles_roundtrip_comprehensive.rs`:
- Around line 44-49: The test helper make_bam_stream currently writes n_ref = 0
but tests then create mapped records with ref_id(0), producing an inconsistent
BAM stream; update make_bam_stream (or add an overload) so it includes a proper
reference dictionary: compute required n_ref from the records (e.g. max ref_id +
1) or accept a references parameter and then write that many reference entries
(names and lengths) into the BAM header and adjust the header text length
accordingly before emitting records; make sure to update the header-writing
logic in make_bam_stream so ref_id used by RawRecord matches the written
reference entries.
In `@crates/fgumi-sam/src/clipper.rs`:
- Around line 1463-1465: The typed-path clipper is inconsistent with the raw
clipper: update the consumes_read/consumes_ref logic to count the CIGAR N (op
kind 3) the same way as the raw clipper/SamRecordClipper so clipped bases and
alignment starts match; specifically, in the block where kind is matched
(variables consumes_read and consumes_ref) add kind 3 to the consumes_ref set
(and likewise update the analogous logic at the other occurrence around the
second block mentioned) so both helpers treat N as reference-consuming; locate
the matches!(kind, ...) expressions and include 3 in the reference-consuming
match arm to align behavior across raw and typed clippers.
- Around line 1762-1768: The FR-pair gate currently checks only r1 using
fgumi_raw_bam::is_fr_pair_raw before clipping; change the validation to require
both records pass is_fr_pair_raw (i.e., call is_fr_pair_raw on r1.as_ref() &&
is_fr_pair_raw on r2.as_ref()) and return (0,0) if either fails inside
clip_overlapping_reads (and the other similar raw-pair clipping block around the
later occurrence), so the function only proceeds when both records are valid FR
mates.
- Around line 2275-2308: The match in raw_clip_update_array_tag is missing
handlers for the BAM array subtypes 'c' (Int8) and 'I' (UInt32), causing
per-base B:... arrays to be left untrimmed; add match arms for b'c' that parse
raw_bytes in 1-byte chunks into Vec<i8> and call editor.update_array_i8(&tag,
&vals), and for b'I' that parse raw_bytes in 4-byte little-endian chunks into
Vec<u32> and call editor.update_array_u32(&tag, &vals); also update the trailing
comment to reflect which types are intentionally skipped.
In `@crates/fgumi-sam/src/record_utils.rs`:
- Around line 697-735: The raw mate bounds paths (mate_unclipped_start_raw and
mate_unclipped_end_raw) currently accept empty/invalid MC CIGARs while the typed
counterparts return None; fix by parsing the MC CIGAR into ops with
parse_cigar_string(mc_cigar), check if the resulting ops is empty or otherwise
invalid and return None in that case, then compute leading/trailing and ref_len
from those ops (use existing helpers like parse_leading_clips /
trailing_clipping / cigar_reference_length on the parsed ops) instead of
operating on the raw MC string so both raw and typed paths consistently reject
empty/invalid MCs.
In `@src/lib/commands/zipper.rs`:
- Around line 1186-1190: The loop currently does std::mem::take(bytes) then
calls restore_unconverted_bases_in_raw_record which can Err and leave the slot
empty; to fix, stash the original bytes (e.g., let original =
std::mem::take(bytes) or let original = bytes.clone()), construct RawRecord from
that stash, call restore_unconverted_bases_in_raw_record, and on Err restore the
original Vec into *bytes before propagating the error; on success assign *bytes
= rec.into_inner(). Ensure references to raw_records, RawRecord::from,
restore_unconverted_bases_in_raw_record, std::mem::take, and rec.into_inner()
are used to locate the change.
---
Outside diff comments:
In `@crates/fgumi-raw-bam/src/raw_bam_record.rs`:
- Around line 707-718: The set_read_name function must reject input containing
embedded NUL bytes to avoid creating multiple terminators; add a check in
set_read_name that scans new_name for any 0 byte and panic (or return an error
consistent with project style) with a clear message like "read name contains
embedded NUL" before computing new_l or performing the splice; reference the
set_read_name function and the new_name/new_l variables and perform the
validation right after entering the function (before constructing replacement
and before splice).
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 1256-1307: find_tag_position may return a tag whose value bytes
are truncated, so set_int_in_place and set_float_in_place must verify the aux
payload is long enough before any indexing/slicing; update set_int_in_place
(matching on b'c','C','s','S','i','I') to check that self.0 has at least p+4 for
1-byte types, p+5 for 2-byte types, and p+7 for 4-byte types and return false if
not, and update set_float_in_place to ensure p+7 <= self.0.len() before copying
the f32 bytes, avoiding any out-of-bounds indexing into self.0 after calling
find_tag_position.
In `@crates/fgumi-raw-bam/tests/tags_editor_invariants.rs`:
- Line 1: This test crate root is missing the crate-level deny for unsafe code;
add the attribute #![deny(unsafe_code)] at the top of tags_editor_invariants.rs
(alongside the existing #![cfg(feature = "test-utils")] attribute) so the test
crate enforces the deny(unsafe_code) lint for all Rust files in the crate.
In `@crates/fgumi-sam/src/alignment_tags.rs`:
- Around line 98-104: The code treats CIGARs with zero reference-consuming span
by writing NM/MD/UQ tags but then returns Ok(false) which contradicts the
documented contract that false means unmapped; update the return to Ok(true)
after inserting tags so callers see tags were written. Locate the branch that
checks ref_span == 0 (and the identical branch at the second occurrence around
the later block) and change the final return from Ok(false) to Ok(true) so
mapped-but-zero-ref-span records are signaled as handled.
---
Nitpick comments:
In `@crates/fgumi-raw-bam/src/builder.rs`:
- Line 1039: Rename the test function identifier
test_mapped_builder_cigar_multple_ops to test_mapped_builder_cigar_multiple_ops
to fix the typo; update any references or invocations of that function (e.g., in
mod tests or #[test] attribute blocks) to the new name so the test still runs
and compiles.
In `@crates/fgumi-raw-bam/src/noodles_compat.rs`:
- Around line 106-112: raw_record_to_record_buf currently calls
raw_records_to_record_bufs which rebuilds a full BAM stream and creates a new
noodles::bam::io::Reader for each call, causing heavy per-call overhead; either
document this in raw_record_to_record_buf's doc comment (explain it's not
amortized and reference RecordBufEncoder) or implement an amortized decoder that
reuses the stream/buffer/Reader across calls (e.g., introduce a RawRecordDecoder
struct with methods like new(header: &noodles::sam::Header) and decode(&mut
self, rec: &crate::RawRecord) -> Result<noodles::sam::alignment::RecordBuf>,
which internally reuses the buffer and noodles::bam::io::Reader instead of
rebuilding per record). Ensure tests or callers are updated to use the new
decoder or the function’s docs are clear about the performance caveat.
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 1042-1047: iter_typed currently calls RawTagsView::get(&entry.tag)
which rescans and returns the first duplicate tag’s value; fix it by decoding
the value directly from each TagEntry yielded by RawTagsView::iter() instead of
calling get. Replace the filter_map so it maps each entry -> (entry.tag,
decoded_value) by using the entry's offset/bytes and the existing decoding logic
(add or reuse a helper such as RawTagsView::decode_value_from_entry or a
decode_at_entry method) to produce the TagValue for that specific entry; ensure
you use the TagEntry from RawTagsView::iter() and its offset/length to decode
rather than RawTagsView::get.
In `@crates/fgumi-raw-bam/tests/noodles_roundtrip.rs`:
- Around line 61-64: The test currently only checks the number of CIGAR ops;
instead decode_with_noodles(&rec) into buf and assert full equality of decoded
contents against the generated inputs: compare buf.cigar() (as a Vec or iterator
of ops) to the original ops (e.g.,
proptest::prop_assert_eq!(buf.cigar().iter().cloned().collect::<Vec<_>>(),
ops)); compare buf.sequence() (or buf.sequence().to_string()) to the generated
seq string (proptest::prop_assert_eq!(buf.sequence(), seq)) and compare
buf.qualities() (or converted Vec<u8>) to the generated quals
(proptest::prop_assert_eq!(buf.qualities().iter().cloned().collect::<Vec<_>>(),
quals)); apply the same detailed comparisons for the other round-trip assertion
block that also uses decode_with_noodles/buf so raw encoding changes in op
kinds/lengths, bases, or qualities are detected.
In `@src/lib/commands/zipper.rs`:
- Line 1280: The code currently allocates a Vec via rec.cigar_ops_vec() into
cigar_ops but only iterates it once; replace that allocation by iterating the
iterator directly using rec.cigar_ops_iter() in the for loop (i.e., remove the
let cigar_ops = rec.cigar_ops_vec() binding and use rec.cigar_ops_iter() where
the loop over cigar_ops occurs) to avoid per-record allocation on the hot path
while keeping the existing loop body and variable names for each operation.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8340b24d-7b16-4b7b-884f-939fd26fe6a7
📒 Files selected for processing (24)
crates/fgumi-consensus/src/codec_caller.rscrates/fgumi-consensus/src/duplex_caller.rscrates/fgumi-consensus/src/vanilla_caller.rscrates/fgumi-raw-bam/Cargo.tomlcrates/fgumi-raw-bam/src/builder.rscrates/fgumi-raw-bam/src/cigar.rscrates/fgumi-raw-bam/src/indexed_reader.rscrates/fgumi-raw-bam/src/lib.rscrates/fgumi-raw-bam/src/noodles_compat.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/sequence.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-raw-bam/src/testutil.rscrates/fgumi-raw-bam/tests/noodles_roundtrip.rscrates/fgumi-raw-bam/tests/noodles_roundtrip_comprehensive.rscrates/fgumi-raw-bam/tests/tags_editor_invariants.proptest-regressionscrates/fgumi-raw-bam/tests/tags_editor_invariants.rscrates/fgumi-sam/src/alignment_tags.rscrates/fgumi-sam/src/builder.rscrates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/lib.rscrates/fgumi-sam/src/record_utils.rssrc/lib/commands/extract.rssrc/lib/commands/zipper.rs
nh13
force-pushed
the
nh/raw-bam-api-7-additive-api
branch
from
April 18, 2026 16:55
6a93923 to
1d47104
Compare
nh13
force-pushed
the
nh/raw-bam-api-8-sam-raw-helpers
branch
from
April 18, 2026 17:20
d7c3057 to
ee84e7d
Compare
nh13
force-pushed
the
nh/raw-bam-api-8-sam-raw-helpers
branch
from
April 18, 2026 18:28
ee84e7d to
60c8cb6
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
crates/fgumi-sam/src/clipper.rs (1)
1771-1774:⚠️ Potential issue | 🟠 MajorReject non-reciprocal raw pairs too.
Checking each record with
is_fr_pair_rawstill allows two unrelated FR records through. Add a reciprocal mate check before clipping:r1.mate_* == r2.*andr2.mate_* == r1.*.Verify
is_fr_pair_rawis single-record only, then add a test where both records pass individually but point at different mates:#!/bin/bash rg -n -C4 'pub fn is_fr_pair_raw|fn is_fr_pair_raw' rg -n -C4 'clip_overlapping_reads\(|clip_extending_past_mate_ends\(' crates/fgumi-sam/src/clipper.rsAlso applies to: 1906-1910
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-sam/src/clipper.rs` around lines 1771 - 1774, The current check uses fgumi_raw_bam::is_fr_pair_raw on r1 and r2 individually, which still lets non-reciprocal FR records through; confirm is_fr_pair_raw is single-record-only, then add a reciprocal mate check wherever r1 and r2 are validated (e.g., the clipping functions referencing r1/r2 such as clip_overlapping_reads / clip_extending_past_mate_ends): ensure r1.mate_* fields (mate_tid/mate_pos/mate_is_reverse or the exact mate field names used) match r2's corresponding fields AND r2.mate_* match r1's, and if they do not, return (0, 0) as you already do for the other failure case. This same reciprocal check must be added at the other location noted (~1906-1910).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fgumi-sam/src/clipper.rs`:
- Around line 2107-2115: The hard-clip upgrade path currently shrinks
sequence/qualities via record.set_sequence_and_qualities but does not trim
per-base auxiliary tags, leaving them at the old length; update the same
branches (the from_start true and false paths using seq, qual,
length_to_upgrade, new_seq/new_qual and record.set_sequence_and_qualities) to
also trim any per-base tags to match the new sequence length: locate per-base
tags on the record (the auxiliary fields that are arrays/strings indexed per
base) and slice/truncate them to new_seq.len() (or new_len) before or after
calling set_sequence_and_qualities so all per-base tags remain consistent with
the reduced sequence; apply this for both the front-trim (from_start) and
back-trim branches.
- Around line 1518-1523: The update of BAM POS uses direct cast and addition
which can overflow: when adjusting record.pos() by ref_bases_clipped, convert
ref_bases_clipped to i32 safely and use checked_add on old_pos (the value
returned by record.pos()) and the casted delta, then handle the None case
instead of unwrapping/panicking; specifically update the logic around
record.pos(), ref_bases_clipped, and record.set_pos(...) to use checked_add and
propagate or return an error if the addition overflows rather than calling
expect().
---
Duplicate comments:
In `@crates/fgumi-sam/src/clipper.rs`:
- Around line 1771-1774: The current check uses fgumi_raw_bam::is_fr_pair_raw on
r1 and r2 individually, which still lets non-reciprocal FR records through;
confirm is_fr_pair_raw is single-record-only, then add a reciprocal mate check
wherever r1 and r2 are validated (e.g., the clipping functions referencing r1/r2
such as clip_overlapping_reads / clip_extending_past_mate_ends): ensure
r1.mate_* fields (mate_tid/mate_pos/mate_is_reverse or the exact mate field
names used) match r2's corresponding fields AND r2.mate_* match r1's, and if
they do not, return (0, 0) as you already do for the other failure case. This
same reciprocal check must be added at the other location noted (~1906-1910).
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 547a5eb0-ab34-4545-8ce4-5cf5d715b22d
📒 Files selected for processing (6)
crates/fgumi-raw-bam/src/tags.rscrates/fgumi-sam/src/alignment_tags.rscrates/fgumi-sam/src/builder.rscrates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/lib.rscrates/fgumi-sam/src/record_utils.rs
✅ Files skipped from review due to trivial changes (3)
- crates/fgumi-sam/src/lib.rs
- crates/fgumi-sam/src/builder.rs
- crates/fgumi-sam/src/record_utils.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/fgumi-sam/src/alignment_tags.rs
nh13
force-pushed
the
nh/raw-bam-api-8-sam-raw-helpers
branch
from
April 18, 2026 22:19
60c8cb6 to
1a32810
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/fgumi-sam/src/clipper.rs (1)
1778-1781:⚠️ Potential issue | 🟠 MajorValidate that the two raw records are the same mate pair.
Calling unary
is_fr_pair_rawon both records can still accept two unrelated FR-looking records. Add a pairwise check matching the typedrecord_utils::is_fr_pair(r1, r2)behavior: reciprocal mate positions/reference IDs, same reference expectations, and any required pair identity checks.#!/bin/bash # Inspect typed vs raw FR-pair validation. Expected: typed validation is pairwise; # raw gates here only call the unary raw predicate unless a pairwise helper exists. rg -n -C5 'fn\s+is_fr_pair|pub fn\s+is_fr_pair|is_fr_pair_raw|clip_overlapping_reads|clip_extending_past_mate_ends' \ crates/fgumi-sam/src/record_utils.rs \ crates/fgumi-sam/src/clipper.rs \ crates/fgumi-raw-bam/srcAlso applies to: 1913-1917
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-sam/src/clipper.rs` around lines 1778 - 1781, The current check uses unary fgumi_raw_bam::is_fr_pair_raw on r1 and r2 independently which can allow unrelated FR-like records; replace this with a pairwise validation mirroring record_utils::is_fr_pair by verifying reciprocal mate flags/positions and matching reference IDs for the pair (i.e., ensure r1.mate_ref == r2.ref && r2.mate_ref == r1.ref and mate positions are reciprocal and any same-reference expectations), so where you currently call fgumi_raw_bam::is_fr_pair_raw(r1) || fgumi_raw_bam::is_fr_pair_raw(r2) (and the similar block at the later location around the 1913–1917 region), change to a single pairwise guard that enforces the reciprocal mate relationship using the raw record fields or add/use a fgumi_raw_bam::is_fr_pair_raw_pair(r1, r2) helper that implements the same checks as record_utils::is_fr_pair.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/fgumi-sam/src/clipper.rs`:
- Around line 2224-2238: The code incorrectly advances seq_pos for
reference-only CIGAR ops (D, N, P), causing query bases/qualities to be dropped;
in the block handling other ops (the match on kind where new_cigar_ops is
pushed), only advance seq_pos when the operation consumes query bases (the
existing consumes_query = matches!(kind, 0 | 1 | 7 | 8) check) — move the
seq_pos += len inside that consumes_query branch (and only loop-copy
sequence/qualities when consumes_query is true) so ref-only ops do not change
seq_pos; update the logic around seq_pos, consumes_query, new_sequence, and
new_qualities accordingly (references: seq_pos, consumes_query, new_cigar_ops,
new_sequence, new_qualities).
---
Duplicate comments:
In `@crates/fgumi-sam/src/clipper.rs`:
- Around line 1778-1781: The current check uses unary
fgumi_raw_bam::is_fr_pair_raw on r1 and r2 independently which can allow
unrelated FR-like records; replace this with a pairwise validation mirroring
record_utils::is_fr_pair by verifying reciprocal mate flags/positions and
matching reference IDs for the pair (i.e., ensure r1.mate_ref == r2.ref &&
r2.mate_ref == r1.ref and mate positions are reciprocal and any same-reference
expectations), so where you currently call fgumi_raw_bam::is_fr_pair_raw(r1) ||
fgumi_raw_bam::is_fr_pair_raw(r2) (and the similar block at the later location
around the 1913–1917 region), change to a single pairwise guard that enforces
the reciprocal mate relationship using the raw record fields or add/use a
fgumi_raw_bam::is_fr_pair_raw_pair(r1, r2) helper that implements the same
checks as record_utils::is_fr_pair.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 36fa7a7d-918a-44aa-85a2-873d05f7a735
📒 Files selected for processing (8)
crates/fgumi-raw-bam/src/builder.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-sam/src/alignment_tags.rscrates/fgumi-sam/src/builder.rscrates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/lib.rscrates/fgumi-sam/src/record_utils.rs
✅ Files skipped from review due to trivial changes (2)
- crates/fgumi-sam/src/builder.rs
- crates/fgumi-raw-bam/src/builder.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/fgumi-sam/src/alignment_tags.rs
- crates/fgumi-sam/src/lib.rs
- crates/fgumi-sam/src/record_utils.rs
Adds two new capabilities to fgumi-sam: - Raw-byte record_utils helpers (unclipped 5' position, mate unclipped start/end, etc.) that operate directly on RawRecordView without inflating to RecordBuf. Kept alongside the RecordBuf variants for incremental adoption. - RawRecordClipper for raw-byte CIGAR surgery. Same clipping semantics as SamRecordClipper but edits packed BAM bytes in place via fgumi-raw-bam's length-changing edits, eliminating the decode-mutate- reencode round-trip. Also threads the renamed UnmappedSamBuilder through alignment_tags and keeps builder.rs comments in sync with the new API layout. Part 3 of 7 in the follow-up series closing #272.
nh13
force-pushed
the
nh/raw-bam-api-8-sam-raw-helpers
branch
from
April 18, 2026 23:09
1a32810 to
2ef037b
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (2)
crates/fgumi-sam/src/clipper.rs (2)
1466-1470:⚠️ Potential issue | 🟠 MajorCount
Nas reference-consuming when clipping crosses skips.Excluding BAM op
3means clipping acrossNdrops the skip but does not advancePOS; e.g.10M100N40Mclipped by 15 starts 100 bp too early. Update raw and typed parity together, and add a cross-Ntest that clips past the skip.Proposed fix
- // Match SamRecordClipper parity: count M/I/=/X as read-consuming and M/D/=/X - // as ref-consuming. N is deliberately excluded from ref here so the raw and - // typed clippers agree on bases_to_clip and alignment_start for spliced reads. + // Match BAM CIGAR semantics: M/I/=/X consume read; M/D/N/=/X consume ref. let consumes_read = matches!(kind, 0 | 1 | 7 | 8); // M, I, =, X - let consumes_ref = matches!(kind, 0 | 2 | 7 | 8); // M, D, =, X + let consumes_ref = matches!(kind, 0 | 2 | 3 | 7 | 8); // M, D, N, =, X @@ - // Match SamRecordClipper parity (see calculate_query_bases_for_ref_region): - // M/D/=/X consume ref; N is excluded so the raw and typed paths agree. - let consumes_ref = matches!(kind, 0 | 2 | 7 | 8); + // Match BAM CIGAR semantics: M/D/N/=/X consume ref. + let consumes_ref = matches!(kind, 0 | 2 | 3 | 7 | 8);Also update the typed
Kind::Skipbranches inSamRecordClipperso the cross-checks still compare equivalent behavior.Also applies to: 1750-1753
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-sam/src/clipper.rs` around lines 1466 - 1470, The current raw/typed parity excludes BAM op `N` (code 3) from ref-consuming, causing clips that cross skips to not advance POS; update the consumes_ref match to include 3 (change matches!(kind, 0 | 2 | 7 | 8) to include 3) and make the corresponding changes in the typed SamRecordClipper by treating Kind::Skip as ref-consuming when computing bases_to_clip/alignment_start so both raw and typed clippers advance across N; also add a unit test that clips across an N (e.g., 10M100N40M clipped by 15) to assert the clipped start is advanced past the skip.
1778-1780:⚠️ Potential issue | 🟠 MajorValidate that the raw records are reciprocal mates.
Calling
is_fr_pair_rawon each record only proves each record is individually plausible; two unrelated FR records can still be clipped together. Add a pair-level raw check before both pair-clipping paths.Proposed fix
+fn is_reciprocal_raw_pair(r1: &fgumi_raw_bam::RawRecord, r2: &fgumi_raw_bam::RawRecord) -> bool { + fgumi_raw_bam::read_name(r1.as_ref()) == fgumi_raw_bam::read_name(r2.as_ref()) + && fgumi_raw_bam::ref_id(r1.as_ref()) == fgumi_raw_bam::mate_ref_id(r2.as_ref()) + && fgumi_raw_bam::ref_id(r2.as_ref()) == fgumi_raw_bam::mate_ref_id(r1.as_ref()) + && fgumi_raw_bam::pos(r1.as_ref()) == fgumi_raw_bam::mate_pos(r2.as_ref()) + && fgumi_raw_bam::pos(r2.as_ref()) == fgumi_raw_bam::mate_pos(r1.as_ref()) +} + - if !fgumi_raw_bam::is_fr_pair_raw(r1.as_ref()) - || !fgumi_raw_bam::is_fr_pair_raw(r2.as_ref()) + if !fgumi_raw_bam::is_fr_pair_raw(r1.as_ref()) + || !fgumi_raw_bam::is_fr_pair_raw(r2.as_ref()) + || !is_reciprocal_raw_pair(r1, r2) { return (0, 0); }Also applies to: 1913-1915
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/fgumi-sam/src/clipper.rs` around lines 1778 - 1780, The current checks call fgumi_raw_bam::is_fr_pair_raw(r1.as_ref()) and is_fr_pair_raw(r2.as_ref()) separately which doesn't ensure r1 and r2 are reciprocal mates; add a pair-level validation (e.g., a new or existing function like fgumi_raw_bam::are_reciprocal_mates(r1.as_ref(), r2.as_ref()) or inline logic comparing read names, flags and mate fields) before entering either pair-clipping path so only true mate pairs are processed; apply the same pair-level validation to the other similar block (the one at the 1913-1915 location) to prevent clipping unrelated FR reads together.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@crates/fgumi-sam/src/clipper.rs`:
- Around line 1466-1470: The current raw/typed parity excludes BAM op `N` (code
3) from ref-consuming, causing clips that cross skips to not advance POS; update
the consumes_ref match to include 3 (change matches!(kind, 0 | 2 | 7 | 8) to
include 3) and make the corresponding changes in the typed SamRecordClipper by
treating Kind::Skip as ref-consuming when computing
bases_to_clip/alignment_start so both raw and typed clippers advance across N;
also add a unit test that clips across an N (e.g., 10M100N40M clipped by 15) to
assert the clipped start is advanced past the skip.
- Around line 1778-1780: The current checks call
fgumi_raw_bam::is_fr_pair_raw(r1.as_ref()) and is_fr_pair_raw(r2.as_ref())
separately which doesn't ensure r1 and r2 are reciprocal mates; add a pair-level
validation (e.g., a new or existing function like
fgumi_raw_bam::are_reciprocal_mates(r1.as_ref(), r2.as_ref()) or inline logic
comparing read names, flags and mate fields) before entering either
pair-clipping path so only true mate pairs are processed; apply the same
pair-level validation to the other similar block (the one at the 1913-1915
location) to prevent clipping unrelated FR reads together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3a52ef58-ad38-4c89-85e6-c7bf4d098da2
📒 Files selected for processing (8)
crates/fgumi-raw-bam/src/builder.rscrates/fgumi-raw-bam/src/raw_bam_record.rscrates/fgumi-raw-bam/src/tags.rscrates/fgumi-sam/src/alignment_tags.rscrates/fgumi-sam/src/builder.rscrates/fgumi-sam/src/clipper.rscrates/fgumi-sam/src/lib.rscrates/fgumi-sam/src/record_utils.rs
✅ Files skipped from review due to trivial changes (1)
- crates/fgumi-raw-bam/src/builder.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/fgumi-sam/src/builder.rs
- crates/fgumi-sam/src/alignment_tags.rs
- crates/fgumi-raw-bam/src/raw_bam_record.rs
- crates/fgumi-raw-bam/src/tags.rs
- crates/fgumi-sam/src/record_utils.rs
This was referenced Apr 18, 2026
Merged
Merged
This was referenced Jul 9, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two new capabilities to `fgumi-sam`:
Also threads the renamed `UnmappedSamBuilder` through `alignment_tags` and keeps `builder.rs` comments in sync with the new API layout.
Part 3 of 7 in the follow-up series closing #272. Stacks on #288.
Test plan
Stack