Repository navigation
refactor(lib)!: collapse Template/DecodedRecord to raw-only; drop dead RecordBuf helpers - #294
Merged
Merged
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
+ Coverage 89.95% 90.18% +0.23%
==========================================
Files 124 124
Lines 62261 60524 -1737
==========================================
- Hits 56005 54584 -1421
+ Misses 6256 5940 -316 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
3 tasks done
nh13
marked this pull request as ready for review
April 17, 2026 22:28
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 31 minutes and 19 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThe codebase was migrated to a raw-byte model: pipelines, groupers, MI grouping, templates, DecodedRecord, and many commands now operate solely on 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
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: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/lib/commands/simplex.rs (1)
611-623:⚠️ Potential issue | 🟠 MajorPropagate consensus failures in threaded mode.
This branch logs and drops the MI group, so
simplex --threadscan exit successfully with missing consensus output. The single-threaded path treats the same failure as fatal.Proposed fix
match caller.consensus_reads(raw_records) { Ok(batch_output) => { all_output.merge(batch_output); batch_stats.merge(&caller.statistics()); if track_rejects { all_rejects.extend(caller.take_rejected_reads()); } } Err(e) => { - log::warn!("Consensus error for MI {mi}: {e}"); + return Err(io::Error::other(format!( + "Consensus error for MI {mi}: {e}" + ))); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/simplex.rs` around lines 611 - 623, The threaded worker currently swallows consensus failures (match on caller.consensus_reads(raw_records) logs and continues), causing missing output under --threads; instead propagate the error so the threaded path fails like the single-threaded path. Replace the Err(e) arm so it forwards the error to the caller (e.g. return Err(e) from the worker function or send the Err through the thread result channel) rather than just logging; ensure the surrounding worker function or thread join expects a Result and that the error from caller.consensus_reads is converted/propagated through that path (references: caller.consensus_reads, caller.take_rejected_reads, all_output.merge, batch_stats.merge, and the MI identifier mi).src/lib/read_info.rs (1)
441-470:⚠️ Potential issue | 🟠 MajorHandle unmapped reads before normalizing positions.
get_unclipped_position_raw()returns0for unmapped reads, so an unmapped mate withref_id = -1can sort before the mapped mate instead of using theUNKNOWN_*sentinels.Proposed fix
- if let (Some(r1), Some(r2)) = (r1, r2) { - let ref_index1 = bam_fields::ref_id(r1.as_ref()); - let start1 = get_unclipped_position_raw(r1)?; - let strand1 = - u8::from((RawRecordView::new(r1).flags() & bam_fields::flags::REVERSE) != 0); - - let ref_index2 = bam_fields::ref_id(r2.as_ref()); - let start2 = get_unclipped_position_raw(r2)?; - let strand2 = - u8::from((RawRecordView::new(r2).flags() & bam_fields::flags::REVERSE) != 0); + let is_unmapped = |r: &RawRecord| { + (RawRecordView::new(r).flags() & bam_fields::flags::UNMAPPED) != 0 + }; + let raw_info = |r: &RawRecord| -> Result<(i32, i32, u8)> { + Ok(( + bam_fields::ref_id(r.as_ref()), + get_unclipped_position_raw(r)?, + u8::from((RawRecordView::new(r).flags() & bam_fields::flags::REVERSE) != 0), + )) + }; + + if let (Some(r1), Some(r2)) = (r1, r2) { + match (is_unmapped(r1), is_unmapped(r2)) { + (true, true) => return Ok(ReadInfo::unmapped(library, cell_barcode)), + (true, false) => { + let (ref_index, start, strand) = raw_info(r2)?; + return Ok(ReadInfo::single(ref_index, start, strand, library, cell_barcode)); + } + (false, true) => { + let (ref_index, start, strand) = raw_info(r1)?; + return Ok(ReadInfo::single(ref_index, start, strand, library, cell_barcode)); + } + (false, false) => {} + } + + let (ref_index1, start1, strand1) = raw_info(r1)?; + let (ref_index2, start2, strand2) = raw_info(r2)?; @@ - let ref_index = bam_fields::ref_id(record.as_ref()); - let start = get_unclipped_position_raw(record)?; - let strand = - u8::from((RawRecordView::new(record).flags() & bam_fields::flags::REVERSE) != 0); + if is_unmapped(record) { + return Ok(ReadInfo::unmapped(library, cell_barcode)); + } + let (ref_index, start, strand) = raw_info(record)?; Ok(ReadInfo::single(ref_index, start, strand, library, cell_barcode))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/read_info.rs` around lines 441 - 470, Before calling get_unclipped_position_raw() and using its 0 value, check for unmapped reads by inspecting bam_fields::ref_id(...) == -1 for each record/mate (r1, r2, record) and when unmapped use the UNKNOWN sentinels expected by ReadInfo (the same values used by ReadInfo::single for R2/missing mates) instead of start=0; only call get_unclipped_position_raw() and compute strand via RawRecordView::new(...).flags() when ref_id != -1, then pass ref_index/start/strand into ReadInfo::new or ReadInfo::single accordingly so unmapped mates sort using UNKNOWN_* rather than position 0.src/lib/commands/dedup.rs (1)
56-56:⚠️ Potential issue | 🟠 MajorReplace TC_TAG with dedicated PA_TAG constant, or verify the tag name matches "pa".
The check for missing tags uses
TC_TAG, which is hardcoded to the legacy"tc"tag (line 132 intag.rs). The error message at lines 1122–1133 explicitly requires the"pa"tag, and comments elsewhere (e.g., zipper.rs:2716) also reference"pa". If the requirement changed to"pa", this code will reject valid inputs. Either define a separatePA_TAGconstant and use it here, or confirm the tag was meant to remain"tc"and update the error message accordingly.Also applies to: 753–772, 1122–1133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/dedup.rs` at line 56, The check in src/lib/commands/dedup.rs is using the legacy TC_TAG constant but the code and error text expect a "pa" tag; either introduce and use a dedicated PA_TAG constant or make the tag usage and error text consistent. Update the logic in the function that references TC_TAG (search for TC_TAG in dedup.rs and related checks) to use a new PA_TAG constant defined alongside other tags (or change the error message to reference "tc" if that was intended), and ensure tag.rs exports PA_TAG (or adjust tag.rs if retaining TC_TAG) so all callers (dedup.rs, zipper.rs references) validate against the same tag symbol.src/lib/mi_group.rs (1)
263-264:⚠️ Potential issue | 🟠 MajorDivergent missing-MI semantics between
MiGrouperandMiGroupIterator.
MiGrouper::add_recordsgroups no-MI records under an empty key (unwrap_or_default()at line 264, codified bytest_grouper_groups_records_without_mi_under_empty_key), whileMiGroupIterator::nextsilently drops them (continueat line 447, codified bytest_skips_records_without_mi_tag). These types serve the same purpose in single- vs multi-threaded paths — the same input BAM will yield different outputs depending on--threads. Pick one policy and apply it to both, or at minimum document the divergence prominently on the public types.Also applies to: 446-448
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mi_group.rs` around lines 263 - 264, MiGroupIterator::next and MiGrouper::add_records have divergent semantics for missing MI tags (MiGrouper uses self.get_mi_tag(...).unwrap_or_default(), while MiGroupIterator::next currently skips records with no MI); pick one policy and make both match — simplest fix: change MiGroupIterator::next to use the same get_mi_tag(&raw).unwrap_or_default() behavior instead of continue so records without MI are grouped under the empty key, and add/adjust a short doc comment on both MiGroupIterator and MiGrouper explaining that missing MI is treated as the empty string (or, if you prefer the opposite policy, apply the reverse change to MiGrouper::add_records and update tests accordingly).src/lib/commands/clip.rs (1)
684-702:⚠️ Potential issue | 🟠 MajorThreaded path silently swallows
upgrade_all_clipping_rawerrors.Single-threaded
clip_fragment/clip_pairpropagate errors fromupgrade_all_clipping_rawwith?, but this threadedprocess_fndiscards them vialet _ = .... Malformed CIGARs or upgrade failures will silently no-op in--threadsmode and produce different output than the fast path.♻️ Propagate the error
- if upgrade_clipping { - let _ = clipper.upgrade_all_clipping_raw(record); - } + if upgrade_clipping { + clipper.upgrade_all_clipping_raw(record).map_err(io::Error::other)?; + } ... - if upgrade_clipping { - let _ = clipper.upgrade_all_clipping_raw(r1); - let _ = clipper.upgrade_all_clipping_raw(r2); - } + if upgrade_clipping { + clipper.upgrade_all_clipping_raw(r1).map_err(io::Error::other)?; + clipper.upgrade_all_clipping_raw(r2).map_err(io::Error::other)?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/clip.rs` around lines 684 - 702, The threaded processing branch is swallowing errors from upgrade_all_clipping_raw (using `let _ = ...`) causing silent no-ops in threaded mode; change the calls in the threaded `process_fn` to propagate errors like the single-threaded paths (use `?` or otherwise return/forward the Result) when calling `clipper.upgrade_all_clipping_raw(r1)` and `clipper.upgrade_all_clipping_raw(r2)` (and the single-call earlier for `record`), ensuring `process_fn` returns a Result that bubbles errors up consistently with `clip_fragment` / `clip_pair`; update function signatures or error channel handling as needed so the propagated errors are handled the same as the non-threaded path.
🧹 Nitpick comments (4)
src/lib/commands/review.rs (2)
1152-1177:parse_cigar_to_opssilently maps unknown CIGAR chars toM.The
_ => 0branch turns any typo in a test CIGAR into a match op — tests would still "pass" but against the wrong alignment. Prefer panicking so bad inputs surface immediately.🩹 Proposed fix
- let op_code: u32 = match ch { - 'M' => 0, - 'I' => 1, - 'D' => 2, - 'N' => 3, - 'S' => 4, - 'H' => 5, - 'P' => 6, - '=' => 7, - 'X' => 8, - _ => 0, - }; + let op_code: u32 = match ch { + 'M' => 0, + 'I' => 1, + 'D' => 2, + 'N' => 3, + 'S' => 4, + 'H' => 5, + 'P' => 6, + '=' => 7, + 'X' => 8, + other => panic!("unexpected CIGAR op '{other}'"), + };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/review.rs` around lines 1152 - 1177, The parse_cigar_to_ops function currently maps unknown CIGAR chars to 'M' via the `_ => 0` branch which hides bad inputs; change the match in parse_cigar_to_ops (the branch that computes op_code before calling encode_op) to explicitly panic with a clear message including the offending character (e.g., "unknown CIGAR op: {}") instead of returning 0, so typos surface immediately; keep using encode_op(op_code, len) unchanged.
687-697: Usewrite_raw_recordhere for consistency.
extract_consensus_reads(Line 630) writes viawrite_raw_record, but this hot loop open-codesblock_size + bytes. Same outcome, less drift.♻️ Proposed refactor
- if mi_set.contains(mi_base) { - // Write raw record: BAM block_size prefix + record bytes. - let block_size = raw_rec.len() as u32; - writer.get_mut().write_all(&block_size.to_le_bytes())?; - writer.get_mut().write_all(&raw_rec)?; - } + if mi_set.contains(mi_base) { + write_raw_record(writer.get_mut(), &raw_rec)?; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/review.rs` around lines 687 - 697, This loop is manually writing the BAM block_size + raw_rec bytes rather than using the existing helper; replace the open‑coded write in the block that checks mi_set (inside the branch using find_string_tag_in_record, SamTag::MI, extract_mi_base and mi_set) with a call to the existing write_raw_record helper (use the same writer variable you already have) so the write logic is consistent with extract_consensus_reads and centralizes encoding/format behavior.src/lib/mi_group.rs (1)
43-51:MemoryEstimateunderestimatesRawRecordallocations.
RawRecordwrapsVec<u8>; usingRawRecord::len()ignores unused capacity of the underlying byte buffer. Since these estimates drive pipeline back-pressure, chronic underestimation can let queues hold more memory than configured. IfRawRecordexposes (or can expose) abyte_capacity()/heap_size()accessor, prefer that here and inClipProcessedBatch::estimate_heap_sizeinclip.rs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mi_group.rs` around lines 43 - 51, The MemoryEstimate for MiGroup undercounts RawRecord heap usage because it sums RawRecord::len() (used bytes) but ignores the underlying Vec<u8> capacity; update MiGroup::estimate_heap_size to use a RawRecord heap-size accessor (e.g., RawRecord::byte_capacity() or heap_size()) instead of RawRecord::len(), and if that accessor doesn't exist add it to RawRecord to return the underlying Vec<u8> capacity in bytes; also update the related estimator ClipProcessedBatch::estimate_heap_size in clip.rs to use the same RawRecord accessor so both places account for unused Vec capacity rather than only .len().src/lib/commands/clip.rs (1)
826-830: Metrics file silently ignored in--threadsmode.When
--metricsis provided with--threads, the pipeline logs a note and discards the request. Consider failing fast (or at leastlog::warn!) so users don't mistakenly believe metrics were written.♻️ Suggested tweak
- if self.metrics.is_some() { - info!("Note: Detailed metrics file not supported in --threads mode"); - } + if self.metrics.is_some() { + log::warn!( + "--metrics is not supported with --threads; no metrics file will be written" + ); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/clip.rs` around lines 826 - 830, The code silently ignores a provided metrics file when running in threads mode by logging an info message; update the check that looks at self.metrics (where it currently calls info!("Note: Detailed metrics file not supported in --threads mode")) to either (A) fail fast by returning an Err with a clear message when metrics are supplied together with --threads, or (B) at minimum change the log level to warn! and include concrete wording that the metrics file will be ignored (e.g., include the metrics path via self.metrics and mention "--threads mode"); make this change in the function that contains the self.metrics.is_some() check so callers get a proper warning or an early error instead of a silent drop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/codec.rs`:
- Around line 9-11: The calls to the opts-free helper functions (notably
create_bam_reader_for_pipeline and create_raw_bam_reader) are using
PipelineReaderOpts::default(), so self.io.async_reader is being ignored; fix
this by constructing or forwarding a PipelineReaderOpts that sets async_reader =
self.io.async_reader (or otherwise pass the existing reader opts) when calling
create_bam_reader_for_pipeline and create_raw_bam_reader (and any other helper
that currently defaults opts), ensuring the same option is used in both pipeline
and single-threaded code paths; update the call sites in codec.rs where
create_bam_reader_for_pipeline/create_raw_bam_reader are invoked to pass the
proper PipelineReaderOpts instead of relying on the opts-free helpers.
In `@src/lib/commands/correct.rs`:
- Around line 19-22: Doctest examples for BamIoOptions used in module docs and
the CorrectUmis doctest are missing the required async_reader field and
therefore fail to compile; update the struct literals (BamIoOptions { input,
output }) to include async_reader: false (or true as appropriate) so they match
the real tests and compile under rustdoc, and ensure both occurrences referenced
by CorrectUmis and the module-level example are changed.
In `@src/lib/commands/dedup.rs`:
- Line 25: The dedup command is ignoring self.io.async_reader because
create_bam_reader_for_pipeline() is called with default reader options; update
the call in src/lib/commands/dedup.rs to propagate the async flag by passing the
async_reader value (self.io.async_reader) into create_bam_reader_for_pipeline or
by building ReaderOptions with async_reader set before calling it (so the dedup
code uses the same async reader setting as other commands). Ensure you reference
the same parameter name used by create_bam_reader_for_pipeline and the dedup
code so the async behavior is wired through end-to-end.
In `@src/lib/commands/duplex.rs`:
- Around line 249-252: The doctest showing a BamIoOptions literal is missing the
required async_reader field which prevents compilation; update the example
literal used in the duplex.rs doc snippet to include async_reader: false,
matching the pattern used in tests (see BamIoOptions initialization near the
test examples and the doctest block in duplex.rs) so the example compiles as-is.
In `@src/lib/commands/filter.rs`:
- Line 347: The filter command is ignoring the user's --async-reader flag
because BamIoOptions (flattened into the command) isn't passed into
create_bam_reader_for_pipeline; update the filter invocation to pass the
pipeline reader options (e.g., replace the call to
create_bam_reader_for_pipeline(&self.io.input) with the variant that accepts
options using self.io.pipeline_reader_opts(), or alternatively remove/hide the
flag from BamIoOptions for the filter command); specifically modify the code
that calls create_bam_reader_for_pipeline in the filter implementation to call
create_bam_reader_for_pipeline_with_opts (or the equivalent API) and supply
self.io.pipeline_reader_opts() so the --async-reader setting from BamIoOptions
is honored.
In `@src/lib/grouper.rs`:
- Around line 225-235: The code currently coalesces records using only the
stored hash (self.current_name_hash) which risks hash-collision merging; update
the grouping logic (e.g., in TemplateGrouper where current_name_hash,
current_raw_records, flush_current_template, and current_name are used) to
perform a two-step check: first the fast hash equality, then read the actual
QNAME bytes via bam_fields::read_name(&raw) and compare those bytes against the
stored self.current_name (or its bytes) before pushing into current_raw_records;
if the bytes differ (collision), treat as a new group by calling
flush_current_template(), setting self.current_name = Some(name_bytes.to_vec())
and self.current_name_hash = Some(name_hash) and then pushing raw; apply the
same hash+byte equality fix in RecordPositionGrouper and
group_by_name_and_build.
In `@tests/integration/test_pipeline_concurrency.rs`:
- Around line 560-565: The test uses noodles::sam::Header::default() which can
cause mapped test records to fail conversion prematurely; instead capture the
actual pipeline header in the FailAfterNGrouper and use that real header when
calling fgumi_raw_bam::raw_record_to_record_buf. Update FailAfterNGrouper to
store the pipeline-provided Header (from the pipeline callback where the header
is available), expose or move it into scope for the test, and replace the
Header::default() usage in the map closure with the captured header so
raw_record_to_record_buf uses the real BAM header during decoding.
---
Outside diff comments:
In `@src/lib/commands/clip.rs`:
- Around line 684-702: The threaded processing branch is swallowing errors from
upgrade_all_clipping_raw (using `let _ = ...`) causing silent no-ops in threaded
mode; change the calls in the threaded `process_fn` to propagate errors like the
single-threaded paths (use `?` or otherwise return/forward the Result) when
calling `clipper.upgrade_all_clipping_raw(r1)` and
`clipper.upgrade_all_clipping_raw(r2)` (and the single-call earlier for
`record`), ensuring `process_fn` returns a Result that bubbles errors up
consistently with `clip_fragment` / `clip_pair`; update function signatures or
error channel handling as needed so the propagated errors are handled the same
as the non-threaded path.
In `@src/lib/commands/dedup.rs`:
- Line 56: The check in src/lib/commands/dedup.rs is using the legacy TC_TAG
constant but the code and error text expect a "pa" tag; either introduce and use
a dedicated PA_TAG constant or make the tag usage and error text consistent.
Update the logic in the function that references TC_TAG (search for TC_TAG in
dedup.rs and related checks) to use a new PA_TAG constant defined alongside
other tags (or change the error message to reference "tc" if that was intended),
and ensure tag.rs exports PA_TAG (or adjust tag.rs if retaining TC_TAG) so all
callers (dedup.rs, zipper.rs references) validate against the same tag symbol.
In `@src/lib/commands/simplex.rs`:
- Around line 611-623: The threaded worker currently swallows consensus failures
(match on caller.consensus_reads(raw_records) logs and continues), causing
missing output under --threads; instead propagate the error so the threaded path
fails like the single-threaded path. Replace the Err(e) arm so it forwards the
error to the caller (e.g. return Err(e) from the worker function or send the Err
through the thread result channel) rather than just logging; ensure the
surrounding worker function or thread join expects a Result and that the error
from caller.consensus_reads is converted/propagated through that path
(references: caller.consensus_reads, caller.take_rejected_reads,
all_output.merge, batch_stats.merge, and the MI identifier mi).
In `@src/lib/mi_group.rs`:
- Around line 263-264: MiGroupIterator::next and MiGrouper::add_records have
divergent semantics for missing MI tags (MiGrouper uses
self.get_mi_tag(...).unwrap_or_default(), while MiGroupIterator::next currently
skips records with no MI); pick one policy and make both match — simplest fix:
change MiGroupIterator::next to use the same
get_mi_tag(&raw).unwrap_or_default() behavior instead of continue so records
without MI are grouped under the empty key, and add/adjust a short doc comment
on both MiGroupIterator and MiGrouper explaining that missing MI is treated as
the empty string (or, if you prefer the opposite policy, apply the reverse
change to MiGrouper::add_records and update tests accordingly).
In `@src/lib/read_info.rs`:
- Around line 441-470: Before calling get_unclipped_position_raw() and using its
0 value, check for unmapped reads by inspecting bam_fields::ref_id(...) == -1
for each record/mate (r1, r2, record) and when unmapped use the UNKNOWN
sentinels expected by ReadInfo (the same values used by ReadInfo::single for
R2/missing mates) instead of start=0; only call get_unclipped_position_raw() and
compute strand via RawRecordView::new(...).flags() when ref_id != -1, then pass
ref_index/start/strand into ReadInfo::new or ReadInfo::single accordingly so
unmapped mates sort using UNKNOWN_* rather than position 0.
---
Nitpick comments:
In `@src/lib/commands/clip.rs`:
- Around line 826-830: The code silently ignores a provided metrics file when
running in threads mode by logging an info message; update the check that looks
at self.metrics (where it currently calls info!("Note: Detailed metrics file not
supported in --threads mode")) to either (A) fail fast by returning an Err with
a clear message when metrics are supplied together with --threads, or (B) at
minimum change the log level to warn! and include concrete wording that the
metrics file will be ignored (e.g., include the metrics path via self.metrics
and mention "--threads mode"); make this change in the function that contains
the self.metrics.is_some() check so callers get a proper warning or an early
error instead of a silent drop.
In `@src/lib/commands/review.rs`:
- Around line 1152-1177: The parse_cigar_to_ops function currently maps unknown
CIGAR chars to 'M' via the `_ => 0` branch which hides bad inputs; change the
match in parse_cigar_to_ops (the branch that computes op_code before calling
encode_op) to explicitly panic with a clear message including the offending
character (e.g., "unknown CIGAR op: {}") instead of returning 0, so typos
surface immediately; keep using encode_op(op_code, len) unchanged.
- Around line 687-697: This loop is manually writing the BAM block_size +
raw_rec bytes rather than using the existing helper; replace the open‑coded
write in the block that checks mi_set (inside the branch using
find_string_tag_in_record, SamTag::MI, extract_mi_base and mi_set) with a call
to the existing write_raw_record helper (use the same writer variable you
already have) so the write logic is consistent with extract_consensus_reads and
centralizes encoding/format behavior.
In `@src/lib/mi_group.rs`:
- Around line 43-51: The MemoryEstimate for MiGroup undercounts RawRecord heap
usage because it sums RawRecord::len() (used bytes) but ignores the underlying
Vec<u8> capacity; update MiGroup::estimate_heap_size to use a RawRecord
heap-size accessor (e.g., RawRecord::byte_capacity() or heap_size()) instead of
RawRecord::len(), and if that accessor doesn't exist add it to RawRecord to
return the underlying Vec<u8> capacity in bytes; also update the related
estimator ClipProcessedBatch::estimate_heap_size in clip.rs to use the same
RawRecord accessor so both places account for unused Vec capacity rather than
only .len().
🪄 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: 3ce7a262-9e90-420a-bc88-441202e7020a
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_pipeline_concurrency.rs
nh13
force-pushed
the
nh/raw-bam-api-10-storage-pipeline-collapse
branch
4 times, most recently
from
April 19, 2026 20:45
a8c1f61 to
fd3b1ab
Compare
Base automatically changed from
nh/raw-bam-api-10-storage-pipeline-collapse
to
main
April 19, 2026 20:47
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 19, 2026 22:37
f8dfe31 to
d9d8f5d
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/commands/codec.rs (1)
570-590:⚠️ Potential issue | 🟠 MajorPropagate unexpected consensus failures in threaded mode.
Single-threaded mode returns non-disagreement errors, but threaded mode only logs them and writes partial results.
Proposed fix
Err(e) => { // Handle duplex disagreement errors by merging stats if e.to_string().contains("duplex disagreement") { batch_stats.merge(caller.statistics()); } else { - log::warn!("CODEC consensus error for MI {mi}: {e}"); + return Err(io::Error::other(format!( + "CODEC consensus error for MI {mi}: {e}" + ))); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/codec.rs` around lines 570 - 590, In the loop over "for MiGroup { mi, records } in batch.groups" where you call "caller.consensus_reads(records)" and match the Result, do not swallow non-"duplex disagreement" errors in the Err(e) arm; instead propagate the error back to the caller (return Err(e) or convert and propagate with context including the MI) so threaded mode fails the same way as single-threaded mode. Keep the existing behavior for duplex disagreement by still merging "batch_stats.merge(caller.statistics())" in that branch and only propagate unexpected errors rather than logging and continuing; references: caller.consensus_reads, batch_stats, all_output, all_rejects, track_rejects, and MiGroup { mi, records }.src/lib/commands/dedup.rs (1)
754-776:⚠️ Potential issue | 🟠 MajorTag mismatch: docs now say
pa, code still checkstc.The docstrings at lines 5, 805, 813, 818 and the field-level comment at line 86 claim the required tag is
pa, but the actual missing-tag detection here usesTC_TAG(line 754) and the user-facing error at lines 1125-1137 still names thetctag. Either the doc rename is premature or the check should move topa. Pick one and align all three spots (doc, check, bail message) — right now users will be told "missingpa" in help text but "missingtc" in the error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/dedup.rs` around lines 754 - 776, The code checks TC_TAG but docs and comments say the required tag is "pa"; update the code to consistently use the "pa" tag: replace uses of TC_TAG (in the loop where tc_tag_bytes is set and any find_tag_type calls) with the PA_TAG constant (or equivalent "pa" tag bytes), increment a clearly named metric (rename dedup_metrics.missing_tc_tag to dedup_metrics.missing_pa_tag and update all references), and update the user-facing bail/error message that currently mentions "tc" to mention "pa" so docs, runtime check, metric, and error text all match. Ensure you also update any other occurrences of "tc" in related variable names, logs, or comments to "pa".
🧹 Nitpick comments (3)
src/lib/commands/group.rs (2)
1238-1246: Tiny inconsistency: clippy allow foras u32only in one of the two twin blocks.The single-threaded twin at Line 1574 carries
#[allow(clippy::cast_possible_truncation)]over thescratch.len() as u32/raw.len() as u32casts, but this pipeline copy doesn't. Not a bug (BAM records never approachu32::MAX), but the pair should look the same so a future lint tightening doesn't fire only here.📝 Proposed tweak
for raw in [template.r1(), template.r2()].into_iter().flatten() { + #[allow(clippy::cast_possible_truncation)] if has_mi { scratch.clear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/group.rs` around lines 1238 - 1246, Add the same clippy allowance used in the single-threaded twin to this pipeline copy: annotate the block that performs the casts scratch.len() as u32 and raw.len() as u32 with #[allow(clippy::cast_possible_truncation)] (placing the attribute immediately above the block or the smallest enclosing function/statement that contains those casts) so the two twin blocks are consistent and future lint tightening won't trigger here.
1442-1445: Stale doc comment: no more raw-vs-noodles dispatch.This docblock still advertises dispatch and a noodles fallback, but after the collapse to raw-only there is exactly one path. Trim it so future readers don't go hunting for a branch that no longer exists.
📝 Proposed doc tidy
/// Process a single position group: build templates, filter, assign UMIs, and write output. /// Used by `execute_single_threaded` for streaming processing. - /// - /// Dispatches on raw-byte vs noodles mode based on the template storage format. - /// In raw-byte mode (single-threaded path after migration), uses zero-allocation filter - /// and direct raw-byte MI-tag injection. The noodles path is kept as a fallback for - /// any future code path that supplies parsed templates (e.g. tests or external callers). + /// + /// Operates on raw-byte records end-to-end: zero-allocation filter and direct + /// raw-byte MI-tag injection, no noodles decode/encode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/group.rs` around lines 1442 - 1445, The doc comment that starts with "Dispatches on raw-byte vs noodles mode based on the template storage format." is stale — remove the mention of a noodles fallback and dispatch logic and replace with a short, accurate note that this code now only uses the raw-byte (single-threaded) path; trim the block to a single sentence describing raw-only behavior and its zero-allocation/raw-byte MI-tag injection so future readers aren't looking for a nonexistent branch.src/lib/commands/clip.rs (1)
2362-2365: Trivial wrapper adds little.
build_raw_recordjust forwards toSamBuilder::build(). Call.build()directly at the test sites to cut indirection.Proposed diff
- /// Build a `RawRecord` from `fgumi_raw_bam::SamBuilder` for test use. - fn build_raw_record(b: &mut fgumi_raw_bam::SamBuilder) -> RawRecord { - b.build() - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/clip.rs` around lines 2362 - 2365, The helper function build_raw_record simply forwards to fgumi_raw_bam::SamBuilder::build() and adds unnecessary indirection; remove the build_raw_record function and update test sites to call .build() directly on the SamBuilder instances (references: function build_raw_record and method SamBuilder::build) so tests use b.build() in place of build_raw_record(b).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/clip.rs`:
- Around line 827-834: The code silently drops user-requested metrics when both
--threads and --metrics are provided (see self.metrics and the current
log::warn! block); fix by either making the CLI fail early via clap
conflicts_with between the --metrics and --threads args or by converting the
runtime warn into a hard error (e.g., return Err/anyhow::bail!) where this
branch executes so the run/execute path refuses to continue with incompatible
flags; alternatively, if keeping the non-fatal behavior, update the --metrics
clap help text to clearly document that metrics are unsupported with --threads
so users are not surprised.
In `@src/lib/commands/codec.rs`:
- Around line 550-553: The pipeline grouper closure currently constructs
MiGrouper with only the "MI" key (MiGrouper::new("MI", batch_size)), which omits
cell-tag (CB) grouping and will merge reads from different cells; change the
closure to construct MiGrouper with the same grouping keys/config used in the
single-threaded path (the call around line 370) so it groups by both MI and CB
(i.e., match the constructor/arguments used there), keeping the return type
Box<dyn Grouper<Group = MiGroupBatch> + Send>.
In `@src/lib/commands/duplex.rs`:
- Line 306: The duplex command currently calls create_bam_reader_for_pipeline
and create_raw_bam_reader which ignore the --async-reader flag; replace those
calls with create_bam_reader_for_pipeline_with_opts and
create_raw_bam_reader_with_opts and pass the reader options produced by
pipeline_reader_opts() (or the equivalent opts used by sibling commands like
dedup) so that self.io.async_reader is respected when creating the reader for
self.io.input (and the raw BAM reader at the other location).
In `@src/lib/commands/review.rs`:
- Around line 985-1004: The loop over record.cigar_ops_typed() currently treats
CigarKind::Skip the same as Deletion which makes skipped (N) regions be reported
as deletions; change the match so CigarKind::Deletion alone performs the "check
if ref_pos is within this deletion and return Ok(true)" logic, while
CigarKind::Skip only advances current_ref_pos by len (i.e., current_ref_pos +=
len) and does not trigger a deletion match. Update the match arms around
CigarKind::Deletion and CigarKind::Skip in the loop that references ref_pos and
current_ref_pos accordingly.
In `@src/lib/grouper.rs`:
- Around line 452-472: The code currently short-circuits validation with the
mc_validated flag, allowing later paired primary records to bypass MC tag
checks; remove the early return that returns Ok when mc_validated is true and
instead always run the MC-tag check for every eligible paired primary (i.e.,
where RawRecordView::new(decoded.data).flags() indicates PAIRED and not
SECONDARY/SUPPLEMENTARY/UNMAPPED/MATE_UNMAPPED), calling
bam_fields::find_mc_tag_in_record(raw) and returning the same io::Error if None;
you may still keep mc_validated as a “seen at least one valid MC” marker (set it
after a successful check) but do not use it to skip validation in
RecordPositionGrouper.
In `@src/lib/variant_review.rs`:
- Around line 159-198: Reject opposite-strand pairs that are RF (not FR) by
checking TLEN sign: when is_reverse and mate_is_reverse differ, fetch tlen via
record.template_length() and verify its sign matches an FR layout (if is_reverse
== false && mate_is_reverse == true then require tlen > 0; if is_reverse == true
&& mate_is_reverse == false then require tlen < 0); if the sign check fails
return "NA". Place this check after computing tlen (template_length) and before
computing other_coord/start/end so functions/vars record, is_reverse,
mate_is_reverse, tlen/template_length, outer, other_coord are used to locate
where to add the guard.
---
Outside diff comments:
In `@src/lib/commands/codec.rs`:
- Around line 570-590: In the loop over "for MiGroup { mi, records } in
batch.groups" where you call "caller.consensus_reads(records)" and match the
Result, do not swallow non-"duplex disagreement" errors in the Err(e) arm;
instead propagate the error back to the caller (return Err(e) or convert and
propagate with context including the MI) so threaded mode fails the same way as
single-threaded mode. Keep the existing behavior for duplex disagreement by
still merging "batch_stats.merge(caller.statistics())" in that branch and only
propagate unexpected errors rather than logging and continuing; references:
caller.consensus_reads, batch_stats, all_output, all_rejects, track_rejects, and
MiGroup { mi, records }.
In `@src/lib/commands/dedup.rs`:
- Around line 754-776: The code checks TC_TAG but docs and comments say the
required tag is "pa"; update the code to consistently use the "pa" tag: replace
uses of TC_TAG (in the loop where tc_tag_bytes is set and any find_tag_type
calls) with the PA_TAG constant (or equivalent "pa" tag bytes), increment a
clearly named metric (rename dedup_metrics.missing_tc_tag to
dedup_metrics.missing_pa_tag and update all references), and update the
user-facing bail/error message that currently mentions "tc" to mention "pa" so
docs, runtime check, metric, and error text all match. Ensure you also update
any other occurrences of "tc" in related variable names, logs, or comments to
"pa".
---
Nitpick comments:
In `@src/lib/commands/clip.rs`:
- Around line 2362-2365: The helper function build_raw_record simply forwards to
fgumi_raw_bam::SamBuilder::build() and adds unnecessary indirection; remove the
build_raw_record function and update test sites to call .build() directly on the
SamBuilder instances (references: function build_raw_record and method
SamBuilder::build) so tests use b.build() in place of build_raw_record(b).
In `@src/lib/commands/group.rs`:
- Around line 1238-1246: Add the same clippy allowance used in the
single-threaded twin to this pipeline copy: annotate the block that performs the
casts scratch.len() as u32 and raw.len() as u32 with
#[allow(clippy::cast_possible_truncation)] (placing the attribute immediately
above the block or the smallest enclosing function/statement that contains those
casts) so the two twin blocks are consistent and future lint tightening won't
trigger here.
- Around line 1442-1445: The doc comment that starts with "Dispatches on
raw-byte vs noodles mode based on the template storage format." is stale —
remove the mention of a noodles fallback and dispatch logic and replace with a
short, accurate note that this code now only uses the raw-byte (single-threaded)
path; trim the block to a single sentence describing raw-only behavior and its
zero-allocation/raw-byte MI-tag injection so future readers aren't looking for a
nonexistent branch.
🪄 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: 3ff44ac1-7e68-4630-bda7-6a050c921b10
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_pipeline_concurrency.rs
✅ Files skipped from review due to trivial changes (1)
- crates/fgumi-metrics/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/unified_pipeline/bam.rs
- src/lib/read_info.rs
- src/lib/commands/simplex.rs
- src/lib/commands/correct.rs
- crates/fgumi-metrics/src/clip.rs
- src/lib/unified_pipeline/base.rs
- src/lib/mi_group.rs
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 20, 2026 02:58
d9d8f5d to
d08c884
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/lib/commands/duplex.rs (1)
702-714:⚠️ Potential issue | 🟡 MinorThreaded error path diverges from single-threaded (and from simplex/codec).
Single-threaded duplex propagates
consensus_readserrors via?(line 479), and the companionsimplex+codecclosures in this PR were explicitly converted to returnio::Error::other(...)on non-tolerated errors. Here the pipeline closure swallows every error aslog::warn!, so real failures (OOM, malformed records, methylation lookup errors) become silent output gaps in--threadsmode only.If duplex errors are genuinely expected-but-skippable, consider matching codec's shape — classify tolerated cases (e.g. "duplex disagreement"/strand missing) explicitly and propagate everything else.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/duplex.rs` around lines 702 - 714, The threaded duplex pipeline is swallowing errors from caller.consensus_reads (used alongside caller.take_rejected_reads and caller.statistics), causing silent failures; change the closure to mirror the single-threaded behavior by propagating non-tolerated errors instead of only logging them: explicitly match/identify tolerated conditions (e.g., duplex disagreement or missing strand) and convert them to recoverable results, but for other errors return an io::Error (use io::Error::new or io::Error::other) so the error bubbles out of the pipeline rather than being suppressed; ensure the code paths that currently call all_output.merge, batch_stats.merge, and all_rejects.extend still run only on Ok(batch_output).src/lib/commands/review.rs (1)
753-755:⚠️ Potential issue | 🟠 MajorNormalize BAM
=bases before allele comparisons/counts.
BAM_BASE_TO_ASCII[0]isb'=', so reads encoded with “same as reference” bases are currently treated as non-reference and reported/counted as=. Map=tovariant.ref_basebeforeadd_base, comparison, and reporting.Proposed fix
+ fn normalize_base_for_variant(base: u8, ref_base: char) -> u8 { + if base == b'=' { + ref_base.to_ascii_uppercase() as u8 + } else { + base.to_ascii_uppercase() + } + } + for record in &consensus_reads { if let Some(base) = self.get_base_at_position(record, variant.pos)? { - consensus_counts.add_base(base); + consensus_counts.add_base(Self::normalize_base_for_variant( + base, + variant.ref_base, + )); } } @@ let read_base = match self.get_base_at_position(&record, variant.pos)? { - Some(b) => (b as char).to_ascii_uppercase(), // b is already ASCII + Some(b) => { + Self::normalize_base_for_variant(b, variant.ref_base) as char + } None => { @@ if let Some(base) = self.get_base_at_position(&rec, variant.pos)? { - counts.add_base(base); + counts.add_base(Self::normalize_base_for_variant( + base, + variant.ref_base, + )); } @@ let code = record.get_base(offset) as usize; - let base_char = (BAM_BASE_TO_ASCII[code] as char).to_ascii_uppercase(); + let base_char = Self::normalize_base_for_variant( + BAM_BASE_TO_ASCII[code], + variant.ref_base, + ) as char;Also applies to: 762-776, 843-845, 894-900, 942-950
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/review.rs` around lines 753 - 755, The code currently treats BAM encoded '=' bases as distinct instead of mapping them to the reference; update get_base_at_position usage so any returned BAM_BASE_TO_ASCII[0] (b'=') is replaced with variant.ref_base before calling consensus_counts.add_base, before any allele comparisons, and before reporting; specifically, normalize the base returned by get_base_at_position (and any local variables used in the allele checks around the consensus_counts.add_base call and the allele comparison/reporting blocks) by mapping '=' to variant.ref_base using BAM_BASE_TO_ASCII and use that normalized byte for add_base, comparisons, and logs.
🧹 Nitpick comments (1)
src/lib/commands/group.rs (1)
1363-1399: Per-recordvec![decoded]allocation in the streaming hot loop is unnecessary overhead.Each iteration wraps a single
DecodedRecordin a fresh vec just to callgrouper.add_records(), which immediately unpacks it in a loop. Theraw_recclone is unavoidable (grouper takes ownership), but the vec allocation can be eliminated by adding a singularadd_record()convenience method toRecordPositionGrouperwithout trait changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/group.rs` around lines 1363 - 1399, The loop is allocating a one-element Vec every iteration to call grouper.add_records(vec![decoded]); add a convenience method on RecordPositionGrouper (e.g., add_record(&mut self, record: DecodedRecord) -> Result<Vec<PositionGroup>, Error>) which performs the same logic as add_records for a single item, keep RawRecord cloning as-is, then change the loop to call grouper.add_record(decoded) and handle the returned completed groups exactly as before (feeding them to Self::process_and_write_position_group). This removes the per-iteration Vec allocation while preserving existing behavior of compute_group_key_from_raw, DecodedRecord::from_raw_bytes, and process_and_write_position_group.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/simplex.rs`:
- Around line 7-10: The --async-reader option is being ignored because
simplex.rs calls the opts-free constructors (create_bam_reader_for_pipeline and
create_raw_bam_reader) and drops self.io.async_reader; update both pipeline and
single-threaded MI-iterator code paths to call the variants that accept options
(e.g., create_bam_reader_for_pipeline_with_opts and
create_raw_bam_reader_with_opts) and pass self.io.pipeline_reader_opts() (or the
equivalent reader opts) so the async_reader flag is honored; locate the calls to
create_bam_reader_for_pipeline and create_raw_bam_reader in simplex.rs (pipeline
path and MI-iterator path) and replace them to thread through the opts from
self.io.pipeline_reader_opts().
In `@src/lib/grouper.rs`:
- Around line 81-89: The add_records implementation in SingleRecordGrouper
currently uses noodles::sam::Header::default(), which breaks decoding of mapped
records; change SingleRecordGrouper to store and use the real BAM header passed
from the pipeline factory and call raw_record_to_record_buf(&d.into_raw_bytes(),
&self.header) in add_records instead of the default header, add a constructor
SingleRecordGrouper::with_header(header: noodles::sam::Header) (or similar) to
initialize the grouper with the provided header, and update pipeline factory
closures to construct groupers via |header|
Box::new(SingleRecordGrouper::with_header(header.clone())) so the real header is
threaded into add_records.
---
Outside diff comments:
In `@src/lib/commands/duplex.rs`:
- Around line 702-714: The threaded duplex pipeline is swallowing errors from
caller.consensus_reads (used alongside caller.take_rejected_reads and
caller.statistics), causing silent failures; change the closure to mirror the
single-threaded behavior by propagating non-tolerated errors instead of only
logging them: explicitly match/identify tolerated conditions (e.g., duplex
disagreement or missing strand) and convert them to recoverable results, but for
other errors return an io::Error (use io::Error::new or io::Error::other) so the
error bubbles out of the pipeline rather than being suppressed; ensure the code
paths that currently call all_output.merge, batch_stats.merge, and
all_rejects.extend still run only on Ok(batch_output).
In `@src/lib/commands/review.rs`:
- Around line 753-755: The code currently treats BAM encoded '=' bases as
distinct instead of mapping them to the reference; update get_base_at_position
usage so any returned BAM_BASE_TO_ASCII[0] (b'=') is replaced with
variant.ref_base before calling consensus_counts.add_base, before any allele
comparisons, and before reporting; specifically, normalize the base returned by
get_base_at_position (and any local variables used in the allele checks around
the consensus_counts.add_base call and the allele comparison/reporting blocks)
by mapping '=' to variant.ref_base using BAM_BASE_TO_ASCII and use that
normalized byte for add_base, comparisons, and logs.
---
Nitpick comments:
In `@src/lib/commands/group.rs`:
- Around line 1363-1399: The loop is allocating a one-element Vec every
iteration to call grouper.add_records(vec![decoded]); add a convenience method
on RecordPositionGrouper (e.g., add_record(&mut self, record: DecodedRecord) ->
Result<Vec<PositionGroup>, Error>) which performs the same logic as add_records
for a single item, keep RawRecord cloning as-is, then change the loop to call
grouper.add_record(decoded) and handle the returned completed groups exactly as
before (feeding them to Self::process_and_write_position_group). This removes
the per-iteration Vec allocation while preserving existing behavior of
compute_group_key_from_raw, DecodedRecord::from_raw_bytes, and
process_and_write_position_group.
🪄 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: f0eb8e47-b84a-4f71-95d2-c37471e39ef5
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_pipeline_concurrency.rs
✅ Files skipped from review due to trivial changes (2)
- src/lib/commands/clip.rs
- src/lib/unified_pipeline/base.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/fgumi-metrics/Cargo.toml
- tests/integration/test_pipeline_concurrency.rs
- crates/fgumi-metrics/src/clip.rs
- src/lib/commands/shared_metrics.rs
- src/lib/variant_review.rs
- src/lib/commands/correct.rs
- src/lib/unified_pipeline/bam.rs
- src/lib/read_info.rs
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 20, 2026 04:28
d08c884 to
578043b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/commands/correct.rs (1)
10-40:⚠️ Potential issue | 🟡 MinorFix the remaining doctest compile errors.
The module doctest at line 40 still calls
execute()with zero arguments instead ofexecute("..."). The third method doctest (around line 371) is incomplete: imports at line 374 omitCompressionOptions,QueueMemoryOptions, andSchedulerOptions, and the struct literal ending at line 390 is missing those same three fields. The first two doctests also need to importCommandto match the method signature.Proposed doctest fixes
//! use std::path::PathBuf; //! use crate::commands::correct::CorrectUmis; +//! use crate::commands::command::Command; //! use crate::commands::common::{ //! BamIoOptions, CompressionOptions, QueueMemoryOptions, RejectsOptions, //! SchedulerOptions, ThreadingOptions, //! }; //! -//! corrector.execute().expect("Failed to correct UMIs"); +//! corrector.execute("fgumi correct ...").expect("Failed to correct UMIs"); //! ``` /// # use crate::commands::correct::CorrectUmis; +/// # use crate::commands::command::Command; /// # use crate::commands::common::{ /// # BamIoOptions, CompressionOptions, QueueMemoryOptions, RejectsOptions, /// # SchedulerOptions, ThreadingOptions, /// # }; /// # use crate::commands::correct::CorrectUmis; + /// # use crate::commands::command::Command; /// # use crate::commands::common::{ + /// # BamIoOptions, CompressionOptions, QueueMemoryOptions, RejectsOptions, + /// # SchedulerOptions, ThreadingOptions, /// # }; /// # use std::path::PathBuf; /// let corrector = CorrectUmis { /// # revcomp: false, /// # threading: ThreadingOptions::new(4), + /// # compression: CompressionOptions::default(), + /// # scheduler_opts: SchedulerOptions::default(), + /// # queue_memory: QueueMemoryOptions::default(), /// };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/correct.rs` around lines 10 - 40, Update the doctests to match the actual API: import the Command trait (use crate::commands::command::Command) in the first two doctests, change any calls of CorrectUmis.execute() with no args to call execute with the required string argument (e.g. execute("...") matching the real signature), and in the third doctest add the missing imports (CompressionOptions, QueueMemoryOptions, SchedulerOptions) and include the corresponding fields (compression, scheduler_opts, queue_memory) in the CorrectUmis struct literal so the example compiles against CorrectUmis, execute, CompressionOptions, QueueMemoryOptions, and SchedulerOptions.src/lib/commands/group.rs (1)
157-160:⚠️ Potential issue | 🟠 MajorGate mate-MAPQ checks on paired records.
Unpaired reads usually have
MATE_UNMAPPEDunset, socheck_mqbecomes true and anyMQaux tag can incorrectly discard a single-end read by mate MAPQ. RequirePAIREDbefore interpretingMQ.Proposed fix
- let check_mq = (flg & bam_fields::flags::MATE_UNMAPPED) == 0; + let check_mq = (flg & bam_fields::flags::PAIRED) != 0 + && (flg & bam_fields::flags::MATE_UNMAPPED) == 0;Also applies to: 221-228
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/group.rs` around lines 157 - 160, The mate-MAPQ gating currently uses only MATE_UNMAPPED to set check_mq, which lets unpaired reads be misinterpreted; update the check to require the PAIRED flag as well. In the block that reads flags via RawRecordView::new(raw).flags() and computes check_mq, change the logic to set check_mq = (flg & bam_fields::flags::PAIRED) != 0 && (flg & bam_fields::flags::MATE_UNMAPPED) == 0 (or equivalent) so MQ is only considered for paired reads; apply the same change to the other occurrence doing the same computation later (the block around the second check_mq assignment).src/lib/commands/review.rs (1)
657-698:⚠️ Potential issue | 🟠 MajorMissing explicit BGZF stream finalization — call
try_finish()before returning.Both
extract_grouped_reads()(lines 657–698) and the similar function at line 629 write a noodles BAM header then mix in raw record bytes viawrite_raw_record(writer.get_mut(), ...). The writer is then dropped implicitly, relying on noodles'Dropto flush the BGZF EOF block. However, errors during drop are silently ignored. Throughout the codebase (e.g.,zipper.rs:941,duplex_metrics.rs:1604,correct.rs:1768), BAM writers are explicitly finalized withwriter.try_finish()?;before scope exit to catch serialization errors and ensure proper file termination. Addwriter.try_finish()?;before theOk(())return in both functions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/review.rs` around lines 657 - 698, The function extract_grouped_reads currently returns after writing raw records but never explicitly finalizes the BGZF stream; before returning Ok(()), call writer.try_finish()? to flush and surface any BGZF serialization errors (mirror the pattern used in zipper.rs/duplex_metrics.rs/correct.rs). Locate the bam::io::writer::Builder-built writer in extract_grouped_reads (used with write_header and write_raw_record(writer.get_mut(), ...)) and add the try_finish() call on that writer instance (and do the same in the similar function at the other site) so the BGZF EOF block is written and any errors are propagated.
♻️ Duplicate comments (1)
src/lib/commands/codec.rs (1)
786-789:⚠️ Potential issue | 🟠 MajorUse a real header when decoding mapped raw test records.
create_codec_fr_pair_overlappingbuilds mapped records withref_id(0), butHeader::default()has no reference sequences.raw_record_to_record_bufcan fail here before the command is exercised. Reuse the samechr1test header used for writing.Proposed test fix
- fn to_record_buf(raw: fgumi_raw_bam::RawRecord) -> RecordBuf { - raw_record_to_record_buf(&raw, &Header::default()) + fn codec_test_header() -> Header { + use noodles::sam::header::record::value::Map; + use noodles::sam::header::record::value::map::{ReferenceSequence, header::Version}; + use std::num::NonZeroUsize; + + let mut header = Header::builder() + .set_header(Map::<noodles::sam::header::record::value::map::Header>::new( + Version::new(1, 6), + )) + .build(); + let rs = Map::<ReferenceSequence>::new( + NonZeroUsize::new(1000).expect("non-zero chromosome length"), + ); + header.reference_sequences_mut().insert(bstr::BString::from("chr1"), rs); + let rg = Map::<noodles::sam::header::record::value::map::ReadGroup>::default(); + header.read_groups_mut().insert(bstr::BString::from("A"), rg); + header + } + + fn to_record_buf(raw: fgumi_raw_bam::RawRecord) -> RecordBuf { + raw_record_to_record_buf(&raw, &codec_test_header()) .expect("raw_record_to_record_buf failed in test") }Then have
write_codec_bamcalllet header = codec_test_header();instead of rebuilding a separate header.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/codec.rs` around lines 786 - 789, The test helper to_record_buf uses Header::default() which lacks reference sequences, causing raw_record_to_record_buf to fail for mapped records created by create_codec_fr_pair_overlapping (they use ref_id(0)); change to_record_buf to use the real test header (codec_test_header()) when decoding mapped raw records, and update write_codec_bam to obtain its header via let header = codec_test_header() instead of rebuilding a separate header so the same chr1 test header is reused for both writing and decoding.
🧹 Nitpick comments (2)
src/lib/commands/review.rs (1)
1025-1067: Guard againstref_pos < startincalculate_read_offset.Minor: if
ref_pos < start, the branchcurrent_ref_pos + len as i32 > ref_posis true immediately and(ref_pos - current_ref_pos) as usizewraps to a huge value, returning a bogus offset. Callers fromgenerate_review_filerely on the region-query filter to avoid this, andhas_non_reference_basepre-checks bounds, butget_base_at_position/get_quality_at_positiondon't. A defensive early-return whenref_pos < start(or usingchecked_sub) would harden the helper.Proposed fix
fn calculate_read_offset(&self, record: &RawRecord, ref_pos: i32) -> Result<Option<usize>> { let start = match record.alignment_start_1based() { Some(pos) => pos as i32, None => return Ok(None), }; + if ref_pos < start { + return Ok(None); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/review.rs` around lines 1025 - 1067, The calculate_read_offset helper can return a bogus huge offset when ref_pos < start because (ref_pos - current_ref_pos) underflows; fix by adding a defensive early return at the top of calculate_read_offset: after obtaining start via alignment_start_1based(), check if ref_pos < start and return Ok(None) (or alternatively use checked_sub before computing offset_in_op) so callers like get_base_at_position/get_quality_at_position won't receive invalid offsets; reference calculate_read_offset, alignment_start_1based(), record.cigar_ops_typed(), and the offset_in_op computation to locate the change.src/lib/commands/dedup.rs (1)
1061-1093: Serialize loop: MI rewrite path LGTM; consider reusingblock_sizeframing helper.The raw-path serialize correctly (a) skips duplicates in remove mode, (b) applies
update_string_tagon a scratch copy when MI is assigned, and (c) falls through to a zero-copy write otherwise. Optional:write_raw_record-style framing (4-byte LE block_size + payload) is duplicated here — consider extending a small helper to reduce the risk of future drift between call sites (seecrates/fgumi-raw-bam/src/raw_bam_record.rs:1-5).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/dedup.rs` around lines 1061 - 1093, The loop duplicates the 4-byte LE block-size framing logic; extract a small helper (e.g., write_block_frame or write_raw_record) that takes a mutable output Vec<u8> and a payload (&[u8]) and appends block_size.to_le_bytes() then the payload, then replace the two duplicate blocks in this loop (the branch that uses scratch after bam_fields::update_string_tag and the else branch that writes raw) to call that helper; keep the existing duplicate-skipping check (remove_duplicates + RawRecordView::new(raw).flags() & DUPLICATE_FLAG) and ensure the helper is used in this file (src/lib/commands/dedup.rs) and can be shared or mirrored with the existing helper in crates/fgumi-raw-bam/src/raw_bam_record.rs if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/correct.rs`:
- Around line 43-45: Reader creation calls use PipelineReaderOpts::default()
which ignores the command-line flag self.io.async_reader; update each call to
create_bam_reader_for_pipeline and create_raw_bam_reader so they build
PipelineReaderOpts honoring self.io.async_reader (e.g. PipelineReaderOpts {
async_reader: self.io.async_reader, ..Default::default() }) instead of
PipelineReaderOpts::default(); apply the same change to every occurrence (the
two calls in the current diff and the other occurrences around
create_bam_reader_for_pipeline/create_raw_bam_reader at the other noted spots).
In `@src/lib/commands/group.rs`:
- Around line 1827-1856: The helper that builds paired reads currently ignores
the second MAPQ parameter and sets R2's MAPQ to mapq1; update the RawSamBuilder
for the second record (variable b2 in this function) to call .mapq(mapq2)
instead of .mapq(mapq1) so R2 honors the provided mapq2 value (ensure the
parameter name mapq2 is used, not the unused _mapq2).
---
Outside diff comments:
In `@src/lib/commands/correct.rs`:
- Around line 10-40: Update the doctests to match the actual API: import the
Command trait (use crate::commands::command::Command) in the first two doctests,
change any calls of CorrectUmis.execute() with no args to call execute with the
required string argument (e.g. execute("...") matching the real signature), and
in the third doctest add the missing imports (CompressionOptions,
QueueMemoryOptions, SchedulerOptions) and include the corresponding fields
(compression, scheduler_opts, queue_memory) in the CorrectUmis struct literal so
the example compiles against CorrectUmis, execute, CompressionOptions,
QueueMemoryOptions, and SchedulerOptions.
In `@src/lib/commands/group.rs`:
- Around line 157-160: The mate-MAPQ gating currently uses only MATE_UNMAPPED to
set check_mq, which lets unpaired reads be misinterpreted; update the check to
require the PAIRED flag as well. In the block that reads flags via
RawRecordView::new(raw).flags() and computes check_mq, change the logic to set
check_mq = (flg & bam_fields::flags::PAIRED) != 0 && (flg &
bam_fields::flags::MATE_UNMAPPED) == 0 (or equivalent) so MQ is only considered
for paired reads; apply the same change to the other occurrence doing the same
computation later (the block around the second check_mq assignment).
In `@src/lib/commands/review.rs`:
- Around line 657-698: The function extract_grouped_reads currently returns
after writing raw records but never explicitly finalizes the BGZF stream; before
returning Ok(()), call writer.try_finish()? to flush and surface any BGZF
serialization errors (mirror the pattern used in
zipper.rs/duplex_metrics.rs/correct.rs). Locate the
bam::io::writer::Builder-built writer in extract_grouped_reads (used with
write_header and write_raw_record(writer.get_mut(), ...)) and add the
try_finish() call on that writer instance (and do the same in the similar
function at the other site) so the BGZF EOF block is written and any errors are
propagated.
---
Duplicate comments:
In `@src/lib/commands/codec.rs`:
- Around line 786-789: The test helper to_record_buf uses Header::default()
which lacks reference sequences, causing raw_record_to_record_buf to fail for
mapped records created by create_codec_fr_pair_overlapping (they use ref_id(0));
change to_record_buf to use the real test header (codec_test_header()) when
decoding mapped raw records, and update write_codec_bam to obtain its header via
let header = codec_test_header() instead of rebuilding a separate header so the
same chr1 test header is reused for both writing and decoding.
---
Nitpick comments:
In `@src/lib/commands/dedup.rs`:
- Around line 1061-1093: The loop duplicates the 4-byte LE block-size framing
logic; extract a small helper (e.g., write_block_frame or write_raw_record) that
takes a mutable output Vec<u8> and a payload (&[u8]) and appends
block_size.to_le_bytes() then the payload, then replace the two duplicate blocks
in this loop (the branch that uses scratch after bam_fields::update_string_tag
and the else branch that writes raw) to call that helper; keep the existing
duplicate-skipping check (remove_duplicates + RawRecordView::new(raw).flags() &
DUPLICATE_FLAG) and ensure the helper is used in this file
(src/lib/commands/dedup.rs) and can be shared or mirrored with the existing
helper in crates/fgumi-raw-bam/src/raw_bam_record.rs if desired.
In `@src/lib/commands/review.rs`:
- Around line 1025-1067: The calculate_read_offset helper can return a bogus
huge offset when ref_pos < start because (ref_pos - current_ref_pos) underflows;
fix by adding a defensive early return at the top of calculate_read_offset:
after obtaining start via alignment_start_1based(), check if ref_pos < start and
return Ok(None) (or alternatively use checked_sub before computing offset_in_op)
so callers like get_base_at_position/get_quality_at_position won't receive
invalid offsets; reference calculate_read_offset, alignment_start_1based(),
record.cigar_ops_typed(), and the offset_in_op computation to locate the change.
🪄 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: bff6c495-629a-4ebe-b86a-ca6d26870b1d
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_pipeline_concurrency.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/fgumi-metrics/Cargo.toml
- crates/fgumi-metrics/src/clip.rs
- src/lib/tag_reversal.rs
- src/lib/unified_pipeline/bam.rs
- src/lib/grouper.rs
- src/lib/mi_group.rs
- src/lib/commands/clip.rs
- src/lib/read_info.rs
- src/lib/unified_pipeline/base.rs
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/grouper.rs (1)
402-411:⚠️ Potential issue | 🟡 MinorRemove the stale
compute_group_keydoc link.This PR removes
read_info::compute_group_key, but the rustdoc still links to it here. Point this at decode-stage group-key construction orGroupKeyinstead.Suggested doc fix
-/// **Requirement:** Paired-end reads must have MC tags so that [`compute_group_key`] -/// produces complete [`GroupKey::paired`] values. Without MC tags, R1 and R2 would +/// **Requirement:** Paired-end reads must have MC tags so that decode-stage +/// group-key construction produces complete [`GroupKey::paired`] values. +/// Without MC tags, R1 and R2 would /// get different `position_key()` values and be incorrectly split. @@ -/// [`compute_group_key`]: crate::read_info::compute_group_key🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/grouper.rs` around lines 402 - 411, The doc comment references the removed function `compute_group_key`; update the link so it points to the current group-key implementation instead (e.g., refer to `GroupKey` or the decode-stage group-key construction) — edit the comment block containing the sentence linking [`compute_group_key`] in `src/lib/grouper.rs` and replace that link with a proper reference to `GroupKey` (or a crate path to the decode-stage group-key construction) so rustdoc resolves to an existing symbol while leaving nearby symbols like `with_secondary_supplementary` unchanged.src/lib/commands/simplex.rs (1)
255-316:⚠️ Potential issue | 🟠 MajorAvoid opening the input twice in single-threaded mode.
The fast path opens
create_bam_reader_for_pipeline_with_opts, drops it, then reopens viacreate_raw_bam_reader_with_opts. That can break stdin/streaming inputs and needlessly starts the async reader. Pick the mode first; for single-threaded mode, create the raw reader once and use its header for output setup.Also applies to: 373-375
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/simplex.rs` around lines 255 - 316, The code currently opens a pipeline reader via create_bam_reader_for_pipeline_with_opts and then drops it and reopens a raw reader for single-threaded fast path, which re-opens input/starts async readers and breaks streaming; fix by checking threading first (inspect self.threading.threads) before creating any reader: if threads.is_some() call create_bam_reader_for_pipeline_with_opts and pass its reader/header into execute_threads_mode, otherwise for single-threaded create a single raw reader via create_raw_bam_reader_with_opts, use its header to build output_header (create_unmapped_consensus_header), set read_name_prefix from that header, and proceed with MiGroupIterator fast path without opening the input twice or calling drop(reader).src/lib/commands/duplex.rs (1)
690-708:⚠️ Potential issue | 🟠 MajorPropagate overlapping-consensus errors in threaded mode.
Line 695 silently drops the whole MI group on overlap failure, while single-thread mode returns the error. This can create
--threads-only output gaps.Suggested fix
- if apply_overlapping_consensus(&mut group_reads, oc).is_err() { - continue; - } + apply_overlapping_consensus(&mut group_reads, oc).map_err(|e| { + io::Error::other(format!( + "Overlapping consensus error for MI {mi}: {e}" + )) + })?; batch_overlapping.merge(oc.stats());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/duplex.rs` around lines 690 - 708, The code currently swallows errors from apply_overlapping_consensus in the overlapping_caller block (inside overlapping_caller, group_reads), causing threaded mode to drop MI groups instead of failing like single-threaded mode; change the logic so that if apply_overlapping_consensus returns Err, you propagate an io::Error (similar to how caller.consensus_reads errors are mapped) instead of continuing. Specifically, in the overlapping_caller branch (symbols: overlapping_caller, oc.reset_stats, apply_overlapping_consensus, batch_overlapping, group_reads), replace the silent continue with returning Err(io::Error::other(format!(...))) including the MI and original error, and only call batch_overlapping.merge(oc.stats()) when apply_overlapping_consensus succeeds.
♻️ Duplicate comments (1)
src/lib/commands/correct.rs (1)
43-45:⚠️ Potential issue | 🟠 MajorPreserve
--async-readerwhen opening BAM readers.These helpers use default reader options, so
self.io.async_readeris ignored in both threaded and single-thread modes.Suggested fix
use crate::bam_io::{ - BamWriter, create_bam_reader_for_pipeline, create_bam_writer, create_optional_bam_writer, - create_raw_bam_reader, + BamWriter, create_bam_reader_for_pipeline_with_opts, create_bam_writer, + create_optional_bam_writer, create_raw_bam_reader_with_opts, }; @@ - let (reader, header) = create_bam_reader_for_pipeline(&self.io.input)?; + let (reader, header) = create_bam_reader_for_pipeline_with_opts( + &self.io.input, + self.io.pipeline_reader_opts(), + )?; @@ - let (mut bam_reader, _) = create_raw_bam_reader(&self.io.input, 1)?; + let (mut bam_reader, _) = + create_raw_bam_reader_with_opts(&self.io.input, 1, self.io.pipeline_reader_opts())?;Also applies to: 408-409, 1112-1113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/correct.rs` around lines 43 - 45, The BAM reader helper calls (create_bam_reader_for_pipeline and create_raw_bam_reader) are currently using default options and ignoring self.io.async_reader; update the call sites to pass the async-reader flag from self.io (e.g., pass self.io.async_reader or wrap it into the ReaderOptions/Async flag parameter) so the created readers honor the --async-reader setting, and do the same for any other reader helper usages (including the duplicate occurrences referenced around create_bam_reader_for_pipeline and create_raw_bam_reader) to ensure both threaded and single-threaded modes preserve the async_reader option.
🧹 Nitpick comments (1)
src/lib/tag_reversal.rs (1)
104-126: Add raw coverage forac/bcreverse-complementing.Current raw tests cover Z-string reversal and array reversal, but not the
per_base::tags_to_reverse_complement()path.Suggested test coverage
fn test_reverse_per_base_tags_raw_string_tag_aq() { // Verify aq Z-type tag is reversed via the raw path too use crate::sort::bam_fields; use crate::vendored::bam_codec::encoder::encode_record_buf; let mut b = RawSamBuilder::new(); b.sequence(b"ACGT").qualities(&[30; 4]).flags(raw_flags::REVERSE); b.add_string_tag(b"aq", b"IIHG"); let record_buf = to_record_buf(&b.build()); let header = Header::default(); let mut raw = Vec::new(); encode_record_buf(&mut raw, &header, &record_buf).expect("encoding record should succeed"); let result = reverse_per_base_tags_raw(&mut raw).expect("reverse_per_base_tags_raw should succeed"); assert!(result); let aux = bam_fields::aux_data_slice(&raw); let s = bam_fields::find_string_tag(aux, b"aq").expect("aq tag should exist"); assert_eq!(s, b"GHII"); } + + #[rstest] + #[case(b"ac")] + #[case(b"bc")] + fn test_reverse_per_base_tags_raw_revcomp_string_tags(#[case] tag: &'static [u8; 2]) { + use crate::sort::bam_fields; + use crate::vendored::bam_codec::encoder::encode_record_buf; + + let mut b = RawSamBuilder::new(); + b.sequence(b"ACGT").qualities(&[30; 4]).flags(raw_flags::REVERSE); + b.add_string_tag(tag, b"ACGA"); + let record_buf = to_record_buf(&b.build()); + + let header = Header::default(); + let mut raw = Vec::new(); + encode_record_buf(&mut raw, &header, &record_buf).expect("encoding record should succeed"); + + let result = + reverse_per_base_tags_raw(&mut raw).expect("reverse_per_base_tags_raw should succeed"); + assert!(result); + + let aux = bam_fields::aux_data_slice(&raw); + let s = bam_fields::find_string_tag(aux, tag).expect("tag should exist"); + assert_eq!(s, b"TCGT"); + }As per coding guidelines, Use
rstestfor parameterized tests in Rust.Also applies to: 154-179
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tag_reversal.rs` around lines 104 - 126, Add raw-path tests that exercise per_base::tags_to_reverse_complement() (the AC/BC per-base reverse-complement tags) similar to the existing Z-string and array tests: create new rstest-parameterized test(s) (or convert the existing single tests) that build a RawSamBuilder record with flags raw_flags::REVERSE, attach AC and BC tags via add_string_tag (and appropriate array forms), encode via encode_record_buf, call reverse_per_base_tags_raw and assert true, then verify the tag values in the encoded buffer using bam_fields::find_string_tag/aux_data_slice to confirm reverse-complement behavior; update the existing test functions (e.g., test_reverse_per_base_tags_raw_string_tag_aq) or add new tests to also cover AC and BC cases so the per_base::tags_to_reverse_complement() path is exercised for raw encoding.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/review.rs`:
- Around line 598-599: The code currently reads the BAM index using a hard-coded
"<prefix>.bam.bai" path (calling bam::bai::fs::read and then
IndexedRawBamReader::from_path with
self.consensus_bam.with_extension("bam.bai")), which fails for standard
"<prefix>.bai" sidecars; add a small helper that first tries
bam::bai::fs::read(self.consensus_bam.with_extension("bam.bai")) and if that
returns Err tries bam::bai::fs::read(self.consensus_bam.with_extension("bai")),
then pass the successful index into
IndexedRawBamReader::from_path(&self.consensus_bam, index). Apply the same
fallback helper/change for the other two places that currently use
with_extension("bam.bai") (the occurrences around where
IndexedRawBamReader::from_path is used).
In `@src/lib/commands/shared_metrics.rs`:
- Around line 283-295: The current filter in RawRecordView::new(&raw).flags()
prematurely skips records with the UNMAPPED flag, which prevents decoding
unmapped consensus BAM candidates; update the condition used around
RawRecordView::new(&raw).flags() to remove the check that excludes
bam_fields::flags::UNMAPPED so paired/R1/primary filtering still applies but
unmapped records are not continued over, and ensure
raw_record_to_record_buf(&raw, &header) is still called for unmapped candidates
so the consensus-tag check and name extraction can run.
---
Outside diff comments:
In `@src/lib/commands/duplex.rs`:
- Around line 690-708: The code currently swallows errors from
apply_overlapping_consensus in the overlapping_caller block (inside
overlapping_caller, group_reads), causing threaded mode to drop MI groups
instead of failing like single-threaded mode; change the logic so that if
apply_overlapping_consensus returns Err, you propagate an io::Error (similar to
how caller.consensus_reads errors are mapped) instead of continuing.
Specifically, in the overlapping_caller branch (symbols: overlapping_caller,
oc.reset_stats, apply_overlapping_consensus, batch_overlapping, group_reads),
replace the silent continue with returning Err(io::Error::other(format!(...)))
including the MI and original error, and only call
batch_overlapping.merge(oc.stats()) when apply_overlapping_consensus succeeds.
In `@src/lib/commands/simplex.rs`:
- Around line 255-316: The code currently opens a pipeline reader via
create_bam_reader_for_pipeline_with_opts and then drops it and reopens a raw
reader for single-threaded fast path, which re-opens input/starts async readers
and breaks streaming; fix by checking threading first (inspect
self.threading.threads) before creating any reader: if threads.is_some() call
create_bam_reader_for_pipeline_with_opts and pass its reader/header into
execute_threads_mode, otherwise for single-threaded create a single raw reader
via create_raw_bam_reader_with_opts, use its header to build output_header
(create_unmapped_consensus_header), set read_name_prefix from that header, and
proceed with MiGroupIterator fast path without opening the input twice or
calling drop(reader).
In `@src/lib/grouper.rs`:
- Around line 402-411: The doc comment references the removed function
`compute_group_key`; update the link so it points to the current group-key
implementation instead (e.g., refer to `GroupKey` or the decode-stage group-key
construction) — edit the comment block containing the sentence linking
[`compute_group_key`] in `src/lib/grouper.rs` and replace that link with a
proper reference to `GroupKey` (or a crate path to the decode-stage group-key
construction) so rustdoc resolves to an existing symbol while leaving nearby
symbols like `with_secondary_supplementary` unchanged.
---
Duplicate comments:
In `@src/lib/commands/correct.rs`:
- Around line 43-45: The BAM reader helper calls (create_bam_reader_for_pipeline
and create_raw_bam_reader) are currently using default options and ignoring
self.io.async_reader; update the call sites to pass the async-reader flag from
self.io (e.g., pass self.io.async_reader or wrap it into the ReaderOptions/Async
flag parameter) so the created readers honor the --async-reader setting, and do
the same for any other reader helper usages (including the duplicate occurrences
referenced around create_bam_reader_for_pipeline and create_raw_bam_reader) to
ensure both threaded and single-threaded modes preserve the async_reader option.
---
Nitpick comments:
In `@src/lib/tag_reversal.rs`:
- Around line 104-126: Add raw-path tests that exercise
per_base::tags_to_reverse_complement() (the AC/BC per-base reverse-complement
tags) similar to the existing Z-string and array tests: create new
rstest-parameterized test(s) (or convert the existing single tests) that build a
RawSamBuilder record with flags raw_flags::REVERSE, attach AC and BC tags via
add_string_tag (and appropriate array forms), encode via encode_record_buf, call
reverse_per_base_tags_raw and assert true, then verify the tag values in the
encoded buffer using bam_fields::find_string_tag/aux_data_slice to confirm
reverse-complement behavior; update the existing test functions (e.g.,
test_reverse_per_base_tags_raw_string_tag_aq) or add new tests to also cover AC
and BC cases so the per_base::tags_to_reverse_complement() path is exercised for
raw encoding.
🪄 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: 97e8c924-e720-472a-93d3-17dd9e39fbee
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_pipeline_concurrency.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/fgumi-metrics/Cargo.toml
- crates/fgumi-metrics/src/clip.rs
- src/lib/unified_pipeline/bam.rs
- src/lib/commands/codec.rs
- src/lib/read_info.rs
- src/lib/commands/clip.rs
- src/lib/commands/group.rs
- src/lib/unified_pipeline/base.rs
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 20, 2026 05:43
955c22e to
98c4b9b
Compare
Member
Author
|
@coderabbitai resume |
Member
Author
|
@coderabbitai review |
✅ Actions performedReviews resumed. |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/lib/tag_reversal.rs (1)
14-16:⚠️ Potential issue | 🟡 MinorDoc comment lists a stale, incomplete tag set.
Per
tags_to_reverse()(14 tags incl. raw-read counts, AB/BA quals, (un)converted counts) andtags_to_reverse_complement()(ac,bc), the doc here under-counts reality. Either enumerate fully or point atper_base::tags_to_reverse()/tags_to_reverse_complement()as the source of truth.📝 Suggested wording
-/// Operates directly on the binary record without parsing into `RecordBuf`. -/// Array tags (cd, ce, ad, ae, bd, be, aq, bq) are reversed in-place. -/// String tags (ac, bc) are reverse-complemented in-place. +/// Operates directly on the binary record without parsing into `RecordBuf`. +/// Tags in `per_base::tags_to_reverse()` are reversed in-place (B-arrays) or +/// byte-reversed (Z-strings, e.g. `aq`/`bq`). Tags in +/// `per_base::tags_to_reverse_complement()` (`ac`, `bc`) are +/// reverse-complemented in-place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/tag_reversal.rs` around lines 14 - 16, The doc comment above the function in tag_reversal.rs currently lists an incomplete/stale set of tags; update that comment to either enumerate the full set exactly as returned by per_base::tags_to_reverse() (all 14 array tags including raw-read counts, AB/BA quals, (un)converted counts) and per_base::tags_to_reverse_complement() (ac, bc), or replace the list with a single sentence pointing readers to per_base::tags_to_reverse() and per_base::tags_to_reverse_complement() as the source of truth; make the change near the existing comment so it accurately reflects the behavior of the reversal code that operates on the binary record.src/lib/commands/codec.rs (1)
583-587:⚠️ Potential issue | 🟠 MajorPreserve rejects for tolerated duplex-disagreement errors.
The single-threaded path still writes rejects after this tolerated error, but the threaded path only drains rejects on
Ok. With--rejects, those failed groups can be missing from the rejects BAM.Proposed fix
Err(e) => { // Handle duplex disagreement errors by merging stats if e.to_string().contains("duplex disagreement") { batch_stats.merge(caller.statistics()); + if track_rejects { + all_rejects.extend(caller.take_rejected_reads()); + } } else { return Err(io::Error::other(format!( "CODEC consensus error for MI {mi}: {e}" )));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/codec.rs` around lines 583 - 587, The Err(e) branch that checks for "duplex disagreement" currently only merges stats via batch_stats.merge(caller.statistics()) but does not perform the rejects draining/write that the Ok path does, causing missing rejects with --rejects; modify the Err(e) handling so that when e.to_string().contains("duplex disagreement") you both merge statistics (batch_stats.merge(caller.statistics())) and invoke the same rejects-drain/write operation used in the Ok branch (i.e., call the function that drains or writes caller's rejects to the rejects writer exactly as done for Ok), ensuring the threaded and single-threaded paths preserve rejects consistently.src/lib/commands/duplex.rs (1)
306-309:⚠️ Potential issue | 🔴 CriticalDon't reopen the input stream in single-threaded mode.
The first reader (lines 306–309) handles stdin properly via
create_bam_reader_for_pipeline_with_opts, which buffers consumed header bytes. Single-threaded mode then drops that reader and reopens withcreate_raw_bam_reader_with_opts(line 416), which callsFile::open()directly—this fails on non-seekable inputs like/dev/stdinor pipes.Check threading mode before opening the reader, then branch to create exactly one reader per path, similar to
simplex.rs(lines 276–280).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/duplex.rs` around lines 306 - 309, The code currently opens a BAM reader via create_bam_reader_for_pipeline_with_opts and then, in single-threaded mode, discards it and reopens via create_raw_bam_reader_with_opts which fails on non-seekable inputs; fix by checking the threading mode on self (single-threaded vs multi-threaded) before opening any reader and only create the appropriate reader once: if single-threaded, call create_bam_reader_for_pipeline_with_opts (using self.io.pipeline_reader_opts()) so stdin/pipes are handled; if multi-threaded, use create_raw_bam_reader_with_opts for reopenable files; update duplex.rs to mirror the branching logic used in simplex.rs and remove the second unconditional File::open path so you never call create_raw_bam_reader_with_opts on non-seekable inputs.src/lib/commands/dedup.rs (1)
360-364:⚠️ Potential issue | 🟡 MinorCompare mate MQ before narrowing.
MQ:c:-1or another negative signed aux value wraps to255here, bypassing the minimum MAPQ filter.Proposed fix
if check_mq { if let Some(mq) = found_mq { - #[allow(clippy::cast_sign_loss, clippy::cast_possible_truncation)] - if (mq as u8) < config.min_mapq { + if mq < i64::from(config.min_mapq) { metrics.discarded_poor_alignment += 1; return false; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/dedup.rs` around lines 360 - 364, The current MAPQ check casts a potentially-negative auxiliary MQ value (found_mq -> mq) to u8, which wraps negatives to 255 and bypasses the filter; instead ensure you first detect negative MQs and treat them as failing the minimum MAPQ check (or explicitly compare as a signed value) before casting. Update the condition around check_mq / found_mq / mq so that negatives are handled (e.g., treat mq < 0 as below threshold) or convert config.min_mapq to a signed type and compare signed-to-signed, and keep incrementing metrics.discarded_poor_alignment when the MQ is negative or below config.min_mapq.
♻️ Duplicate comments (1)
src/lib/grouper.rs (1)
65-83:⚠️ Potential issue | 🟠 MajorFinish wiring real headers into
SingleRecordGrouper.
with_headeris correct, butnew()/Defaultstill decode withHeader::default(). The provided integration snippets still callSingleRecordGrouper::new()even when a realHeaderis available, so mapped records can still fail or decode incorrectly. This is the same header-threading issue previously raised.Verification
#!/bin/bash set -euo pipefail # Expect: mapped pass-through factory call sites use SingleRecordGrouper::with_header(header.clone()), # not SingleRecordGrouper::new() or ::default(). rg -n -C4 'SingleRecordGrouper::(new|default)\s*\(' --type=rustSuggested direction
- |_header: &Header| Box::new(SingleRecordGrouper::new()), + |header: &Header| Box::new(SingleRecordGrouper::with_header(header.clone())),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/grouper.rs` around lines 65 - 83, SingleRecordGrouper currently decodes mapped records against Header::default() via SingleRecordGrouper::new() / Default which causes incorrect decoding; update all call sites to construct groupers with the real header by using SingleRecordGrouper::with_header(header.clone()) instead of SingleRecordGrouper::new() or SingleRecordGrouper::default(), and consider removing or deprecating the public Default/new constructor (or making it only for unmapped-only tests) so header-threading is explicit; target uses of SingleRecordGrouper::new, SingleRecordGrouper::default, and places that pass unmapped records to ensure they now call SingleRecordGrouper::with_header(header.clone()) and propagate the noodles::sam::Header instance.
🧹 Nitpick comments (2)
src/lib/commands/dedup.rs (2)
2828-3009: Move this full-command BAM I/O regression to integration tests.This writes BAMs, runs
MarkDuplicates::execute(), then reads output BAMs; that is integration coverage rather than a source-local unit test. As per coding guidelines, Place unit tests alongside source code insrc/and integration tests intests/integration/.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/dedup.rs` around lines 2828 - 3009, The test test_dedup_no_umi_large_position_group performs end-to-end BAM file I/O and runs MarkDuplicates::execute(), so move it out of the source-local unit tests into the integration test suite: remove the entire test function from src/lib/commands/dedup.rs and create a new integration test file (e.g., tests/integration/dedup_no_umi.rs) containing the same test body; keep references to MarkDuplicates::try_parse_from and MarkDuplicates::execute unchanged so the behavior is identical, and ensure the new file imports the same helper types (RawSamBuilder, encode_op, flags, SamTag, etc.) or re-exports them from the crate for integration tests.
2432-2585: Collapse these table-style cases withrstest.These raw filter tests vary only flag/MAPQ/UMI/config/expected metric; a single parameterized test would be easier to maintain. As per coding guidelines, Use
rstestfor parameterized tests in Rust.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/dedup.rs` around lines 2432 - 2585, Multiple near-duplicate unit tests (test_filter_template_raw_accepts_valid, test_filter_template_raw_rejects_low_mapq, test_filter_template_raw_rejects_qc_fail, test_filter_template_raw_rejects_umi_with_n, test_filter_template_raw_rejects_short_umi, test_filter_template_raw_rejects_unmapped) exercise the same logic around filter_template, DedupFilterConfig, make_raw_bam_for_dedup and FilterMetrics; collapse them into a single parameterized rstest to reduce duplication. Create one #[rstest] parameterized test (e.g., test_filter_template_raw_param) that takes inputs for flags, mapq, umi_seq, DedupFilterConfig overrides and the expected FilterMetrics field name/expected increment, build the raw record with make_raw_bam_for_dedup, construct Template::from_records, create a fresh FilterMetrics per case, call filter_template(&template, &config, &mut metrics) and assert the boolean result and that the specific metrics field (e.g., accepted_templates, discarded_poor_alignment, discarded_non_pf, discarded_ns_in_umi, discarded_umi_too_short) equals the expected value; ensure you add the rstest attribute and imports and keep semantics identical to the original individual tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/commands/codec.rs`:
- Around line 583-587: The Err(e) branch that checks for "duplex disagreement"
currently only merges stats via batch_stats.merge(caller.statistics()) but does
not perform the rejects draining/write that the Ok path does, causing missing
rejects with --rejects; modify the Err(e) handling so that when
e.to_string().contains("duplex disagreement") you both merge statistics
(batch_stats.merge(caller.statistics())) and invoke the same rejects-drain/write
operation used in the Ok branch (i.e., call the function that drains or writes
caller's rejects to the rejects writer exactly as done for Ok), ensuring the
threaded and single-threaded paths preserve rejects consistently.
In `@src/lib/commands/dedup.rs`:
- Around line 360-364: The current MAPQ check casts a potentially-negative
auxiliary MQ value (found_mq -> mq) to u8, which wraps negatives to 255 and
bypasses the filter; instead ensure you first detect negative MQs and treat them
as failing the minimum MAPQ check (or explicitly compare as a signed value)
before casting. Update the condition around check_mq / found_mq / mq so that
negatives are handled (e.g., treat mq < 0 as below threshold) or convert
config.min_mapq to a signed type and compare signed-to-signed, and keep
incrementing metrics.discarded_poor_alignment when the MQ is negative or below
config.min_mapq.
In `@src/lib/commands/duplex.rs`:
- Around line 306-309: The code currently opens a BAM reader via
create_bam_reader_for_pipeline_with_opts and then, in single-threaded mode,
discards it and reopens via create_raw_bam_reader_with_opts which fails on
non-seekable inputs; fix by checking the threading mode on self (single-threaded
vs multi-threaded) before opening any reader and only create the appropriate
reader once: if single-threaded, call create_bam_reader_for_pipeline_with_opts
(using self.io.pipeline_reader_opts()) so stdin/pipes are handled; if
multi-threaded, use create_raw_bam_reader_with_opts for reopenable files; update
duplex.rs to mirror the branching logic used in simplex.rs and remove the second
unconditional File::open path so you never call create_raw_bam_reader_with_opts
on non-seekable inputs.
In `@src/lib/tag_reversal.rs`:
- Around line 14-16: The doc comment above the function in tag_reversal.rs
currently lists an incomplete/stale set of tags; update that comment to either
enumerate the full set exactly as returned by per_base::tags_to_reverse() (all
14 array tags including raw-read counts, AB/BA quals, (un)converted counts) and
per_base::tags_to_reverse_complement() (ac, bc), or replace the list with a
single sentence pointing readers to per_base::tags_to_reverse() and
per_base::tags_to_reverse_complement() as the source of truth; make the change
near the existing comment so it accurately reflects the behavior of the reversal
code that operates on the binary record.
---
Duplicate comments:
In `@src/lib/grouper.rs`:
- Around line 65-83: SingleRecordGrouper currently decodes mapped records
against Header::default() via SingleRecordGrouper::new() / Default which causes
incorrect decoding; update all call sites to construct groupers with the real
header by using SingleRecordGrouper::with_header(header.clone()) instead of
SingleRecordGrouper::new() or SingleRecordGrouper::default(), and consider
removing or deprecating the public Default/new constructor (or making it only
for unmapped-only tests) so header-threading is explicit; target uses of
SingleRecordGrouper::new, SingleRecordGrouper::default, and places that pass
unmapped records to ensure they now call
SingleRecordGrouper::with_header(header.clone()) and propagate the
noodles::sam::Header instance.
---
Nitpick comments:
In `@src/lib/commands/dedup.rs`:
- Around line 2828-3009: The test test_dedup_no_umi_large_position_group
performs end-to-end BAM file I/O and runs MarkDuplicates::execute(), so move it
out of the source-local unit tests into the integration test suite: remove the
entire test function from src/lib/commands/dedup.rs and create a new integration
test file (e.g., tests/integration/dedup_no_umi.rs) containing the same test
body; keep references to MarkDuplicates::try_parse_from and
MarkDuplicates::execute unchanged so the behavior is identical, and ensure the
new file imports the same helper types (RawSamBuilder, encode_op, flags, SamTag,
etc.) or re-exports them from the crate for integration tests.
- Around line 2432-2585: Multiple near-duplicate unit tests
(test_filter_template_raw_accepts_valid,
test_filter_template_raw_rejects_low_mapq,
test_filter_template_raw_rejects_qc_fail,
test_filter_template_raw_rejects_umi_with_n,
test_filter_template_raw_rejects_short_umi,
test_filter_template_raw_rejects_unmapped) exercise the same logic around
filter_template, DedupFilterConfig, make_raw_bam_for_dedup and FilterMetrics;
collapse them into a single parameterized rstest to reduce duplication. Create
one #[rstest] parameterized test (e.g., test_filter_template_raw_param) that
takes inputs for flags, mapq, umi_seq, DedupFilterConfig overrides and the
expected FilterMetrics field name/expected increment, build the raw record with
make_raw_bam_for_dedup, construct Template::from_records, create a fresh
FilterMetrics per case, call filter_template(&template, &config, &mut metrics)
and assert the boolean result and that the specific metrics field (e.g.,
accepted_templates, discarded_poor_alignment, discarded_non_pf,
discarded_ns_in_umi, discarded_umi_too_short) equals the expected value; ensure
you add the rstest attribute and imports and keep semantics identical to the
original individual tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8a449577-7990-45cf-b89f-81137369b872
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_pipeline_concurrency.rs
✅ Files skipped from review due to trivial changes (2)
- src/lib/commands/group.rs
- src/lib/unified_pipeline/base.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/fgumi-metrics/Cargo.toml
- src/lib/commands/shared_metrics.rs
- src/lib/commands/simplex.rs
- src/lib/commands/clip.rs
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 20, 2026 06:50
98c4b9b to
88f947f
Compare
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 20, 2026 07:09
88f947f to
dac71b7
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/lib/unified_pipeline/bam.rs (1)
337-372:⚠️ Potential issue | 🟡 MinorValidate offsets before indexing the buffer.
decode_recordscan panic on a malformedBoundaryBatchwherestart/endare out of bounds. Since this public function documents malformed BAM records asio::Error, check offset ordering andend <= buffer.len()before readingbatch.buffer[start].Suggested fix
let start = batch.offsets[i]; let end = batch.offsets[i + 1]; + let buffer_len = batch.buffer.len(); // Validate: end > start + 4 (need at least block_size prefix) - if end <= start + 4 { + if start >= end || end > buffer_len || end - start <= 4 { return Err(io::Error::new( io::ErrorKind::InvalidData, format!( "Invalid record bounds: start={start}, end={end}, record_index={i}, \ - num_records={num_records}, buffer_len={}", - batch.buffer.len() + num_records={num_records}, buffer_len={buffer_len}" ), )); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/unified_pipeline/bam.rs` around lines 337 - 372, The code in decode_records reads batch.buffer at indices derived from BoundaryBatch (batch.offsets) without validating that those offsets are in range; add explicit bounds checks before any indexing: verify start < end, start + 4 <= end, end <= batch.buffer.len(), and start < batch.buffer.len() (or start + 4 <= batch.buffer.len()) and return an io::Error::new(io::ErrorKind::InvalidData, ...) when these fail. Place these checks immediately after computing start/end (before reading batch.buffer[start] or computing stored_block_size) so stored_block_size and the slice for raw = batch.buffer[start + 4..end].to_vec() cannot panic. Ensure error messages reference record_index/i, start, end, and buffer_len for diagnostics.src/lib/mi_group.rs (1)
310-314:⚠️ Potential issue | 🟡 MinorUpdate the stale iterator doc.
After the rename, this no longer has a separate parsed
MiGroupIteratorto be “equivalent” to.Suggested wording
-/// This is the raw-byte equivalent of `MiGroupIterator` for the single-threaded path. +/// This is the single-threaded raw-byte MI grouping path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/mi_group.rs` around lines 310 - 314, Update the stale doc comment on the MiGroupIterator struct: remove the phrase claiming it is the "raw-byte equivalent of `MiGroupIterator` for the single-threaded path" and replace with a concise description stating that MiGroupIterator groups consecutive raw BAM records by their MI tag (i.e., it yields groups of records sharing the same MI tag from a stream of raw BAM records); keep the allow attribute and type-complexity note intact and ensure the top-line summary reflects the current single iterator implementation.src/lib/commands/codec.rs (1)
265-311:⚠️ Potential issue | 🟠 MajorAvoid opening CODEC input twice in single-threaded mode.
The single-threaded path still opens the pipeline reader to read the header, drops it, then reopens a raw reader. This wastes I/O and breaks stdin/non-seekable input after the first reader consumes the BAM header. Mirror the simplex structure: branch on
threadsbefore opening a reader, and derive the single-threaded header fromcreate_raw_bam_reader_with_opts.Proposed restructure
- // Open input BAM using streaming-capable reader for pipeline use - let (reader, header) = create_bam_reader_for_pipeline_with_opts( - &self.io.input, - self.io.pipeline_reader_opts(), - )?; - - // Create output header for unmapped consensus reads - let output_header = create_unmapped_consensus_header( - &header, - &self.read_group.read_group_id, - "Read group", - command_line, - )?; - - // Use library name from header if no prefix is specified (like fgbio) - let read_name_prefix = self.read_group.prefix_or_from_header(&header); - // Parse cell tag let cell_tag = Tag::from(SamTag::CB); // Enable rejects tracking if rejects file is specified let track_rejects = self.rejects_opts.is_enabled(); @@ // IMPORTANT: Check this BEFORE creating any output file handles to avoid // file handle conflicts that can corrupt the output. if let Some(threads) = self.threading.threads { + let (reader, header) = create_bam_reader_for_pipeline_with_opts( + &self.io.input, + self.io.pipeline_reader_opts(), + )?; + let output_header = create_unmapped_consensus_header( + &header, + &self.read_group.read_group_id, + "Read group", + command_line, + )?; + let read_name_prefix = self.read_group.prefix_or_from_header(&header); let result = self.execute_threads_mode( threads, reader, - header.clone(), + header, output_header, - read_name_prefix.clone(), + read_name_prefix, track_rejects, ); timer.log_completion(0); // Completion logged in execute_threads_mode return result; } - // Drop the reader for single-threaded mode - we use MiGroupIterator which needs its own reader - drop(reader); + let (mut raw_reader, header) = + create_raw_bam_reader_with_opts(&self.io.input, 1, self.io.pipeline_reader_opts())?; + let output_header = create_unmapped_consensus_header( + &header, + &self.read_group.read_group_id, + "Read group", + command_line, + )?; + let read_name_prefix = self.read_group.prefix_or_from_header(&header); @@ - let (mut raw_reader, _) = - create_raw_bam_reader_with_opts(&self.io.input, 1, self.io.pipeline_reader_opts())?; let raw_record_iter = std::iter::from_fn(move || {Also applies to: 359-370
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/codec.rs` around lines 265 - 311, The code currently opens the pipeline reader (create_bam_reader_for_pipeline_with_opts) unconditionally then drops it for single-threaded mode, which consumes the BAM header and breaks non-seekable/STDIN inputs; change the control flow to branch on self.threading.threads before creating any readers: if threads.is_some() keep the pipeline reader flow and call execute_threads_mode(...) with the pipeline reader and header as now; else (single-threaded) call create_raw_bam_reader_with_opts to obtain a raw reader and header for MiGroupIterator/consensus fast path (remove the unnecessary create_bam_reader_for_pipeline_with_opts and the drop(reader) in that branch) so the input is opened only once and non-seekable streams remain intact. Ensure read_name_prefix, output_header (create_unmapped_consensus_header), cell_tag, and track_rejects are computed in the appropriate branches or before branching if they don't require consuming the reader.
🧹 Nitpick comments (1)
src/lib/commands/group.rs (1)
1216-1258: Factor out the duplicated raw-byte MI-injection writer.Lines 1220-1250 and 1557-1588 implement the same byte-level loop (scratch copy →
update_string_tag→block_size+ payload emit) against two different sinks (&mut Vec<u8>vswriter.get_mut()). Any future fix (e.g., per-record MI, stripping stale MI) risks drifting between the two paths. Consider extracting a helper such as:Sketch
fn emit_template_raw( template: &Template, base_mi: u64, assign_tag_bytes: &[u8; 2], scratch: &mut Vec<u8>, mi_buf: &mut String, mut emit: impl FnMut(&[u8]) -> std::io::Result<()>, ) -> std::io::Result<()> { let mi = template.mi; let has_mi = mi.is_assigned(); if has_mi { mi.write_with_offset(base_mi, mi_buf); } for raw in [template.r1(), template.r2()].into_iter().flatten() { if has_mi { scratch.clear(); scratch.extend_from_slice(raw); bam_fields::update_string_tag(scratch, assign_tag_bytes, mi_buf.as_bytes()); let bs = u32::try_from(scratch.len()).unwrap(); emit(&bs.to_le_bytes())?; emit(scratch)?; } else { let bs = u32::try_from(raw.len()).unwrap(); emit(&bs.to_le_bytes())?; emit(raw)?; } } Ok(()) }Callers pass
|b| { output.extend_from_slice(b); Ok(()) }or|b| writer.get_mut().write_all(b).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/commands/group.rs` around lines 1216 - 1258, The two duplicated byte-level loops that inject/emit MI (the one using output.extend_from_slice and the one writing to writer.get_mut()) should be factored into a single helper (e.g., emit_template_raw) that accepts &Template, base_mi, &assign_tag_bytes, &mut scratch, &mut mi_buf and an emit: impl FnMut(&[u8]) -> std::io::Result<()>; inside it reuse mi.write_with_offset, call bam_fields::update_string_tag when has_mi, compute block size as a u32 from scratch.len() or raw.len(), and call emit for the size bytes then payload; replace both call sites to pass emit closures like |b| { output.extend_from_slice(b); Ok(()) } and |b| writer.get_mut().write_all(b). Ensure the helper returns std::io::Result<()> and use safe conversions (u32::try_from or cast with checked bounds) for block size.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib/commands/duplex.rs`:
- Around line 447-450: The MI-transform closure must strip trailing NUL bytes
from the raw Z-tag before extracting the base MI: inside the
MiGroupIterator::with_transform closure that currently does let mi_str =
String::from_utf8_lossy(mi_bytes); call trim_end_matches('\0') (or otherwise
trim any trailing 0 bytes from the bytes/string) and pass that trimmed string
into extract_mi_base(). Apply the same change to the other identical transform
usage (the transform around the other MiGroupIterator::with_transform occurrence
referenced in the comment) so raw tags like "1/A\0" and "1/B\0" collapse to "1"
consistently; this mirrors the behavior in has_both_strands_raw.
In `@src/lib/tag_reversal.rs`:
- Around line 15-16: The doc comment for per_base::tags_to_reverse currently
says "byte-wise" which implies corrupting multi-byte B-array elements; update
the comment in src/lib/tag_reversal.rs to clarify that B-array reversal is
element-wise (i.e., the order of elements along the raw path is reversed, not
the internal byte-order of multi-byte elements), and replace "byte-wise" with
wording like "element-wise (raw-path) reversal" and mention that multi-byte
element contents are preserved; reference the per_base::tags_to_reverse
description and any associated tag reversal function in tag_reversal.rs when
making the wording change.
In `@tests/integration/test_dedup_command.rs`:
- Around line 223-225: The synthetic SAM flag bits for the paired records are
inconsistent: R2 is marked REVERSE but R1 does not have the corresponding
MATE_REVERSE bit, which can fail is_fr_pair_raw self-consistency checks; update
the test fixture in tests/integration/test_dedup_command.rs (around the
constants NUM_TEMPLATES and PROPERLY_PAIRED and the related synthetic-record
construction at lines ~259-281) so that the pair flags are
self-consistent—ensure the properly paired flag is set for both records and that
when one record has the REVERSE bit the mate record has the MATE_REVERSE bit
(and vice versa) so the R1/R2 flag combination matches the expectations of
is_fr_pair_raw.
- Around line 231-247: The test constructs `@HD` tags by matching
HeaderTag::from(...) to HeaderTag::Other which panics because SO, GO, SS map to
typed variants; update the code in test_dedup_command.rs (and the helper
build_header_with_tags in bam_generator.rs and other occurrences in group.rs,
extract.rs) to set header fields using the typed header API or by inserting
HeaderTag::SortOrder / HeaderTag::GroupOrder / HeaderTag::SubsortOrder directly
instead of pattern-matching to HeaderTag::Other; ensure you call the noodles
Header builder methods (or construct HeaderRecordMap with the correct typed
HeaderTag variants) so SO/GO/SS are created as the proper typed variants rather
than Other.
---
Outside diff comments:
In `@src/lib/commands/codec.rs`:
- Around line 265-311: The code currently opens the pipeline reader
(create_bam_reader_for_pipeline_with_opts) unconditionally then drops it for
single-threaded mode, which consumes the BAM header and breaks
non-seekable/STDIN inputs; change the control flow to branch on
self.threading.threads before creating any readers: if threads.is_some() keep
the pipeline reader flow and call execute_threads_mode(...) with the pipeline
reader and header as now; else (single-threaded) call
create_raw_bam_reader_with_opts to obtain a raw reader and header for
MiGroupIterator/consensus fast path (remove the unnecessary
create_bam_reader_for_pipeline_with_opts and the drop(reader) in that branch) so
the input is opened only once and non-seekable streams remain intact. Ensure
read_name_prefix, output_header (create_unmapped_consensus_header), cell_tag,
and track_rejects are computed in the appropriate branches or before branching
if they don't require consuming the reader.
In `@src/lib/mi_group.rs`:
- Around line 310-314: Update the stale doc comment on the MiGroupIterator
struct: remove the phrase claiming it is the "raw-byte equivalent of
`MiGroupIterator` for the single-threaded path" and replace with a concise
description stating that MiGroupIterator groups consecutive raw BAM records by
their MI tag (i.e., it yields groups of records sharing the same MI tag from a
stream of raw BAM records); keep the allow attribute and type-complexity note
intact and ensure the top-line summary reflects the current single iterator
implementation.
In `@src/lib/unified_pipeline/bam.rs`:
- Around line 337-372: The code in decode_records reads batch.buffer at indices
derived from BoundaryBatch (batch.offsets) without validating that those offsets
are in range; add explicit bounds checks before any indexing: verify start <
end, start + 4 <= end, end <= batch.buffer.len(), and start < batch.buffer.len()
(or start + 4 <= batch.buffer.len()) and return an
io::Error::new(io::ErrorKind::InvalidData, ...) when these fail. Place these
checks immediately after computing start/end (before reading batch.buffer[start]
or computing stored_block_size) so stored_block_size and the slice for raw =
batch.buffer[start + 4..end].to_vec() cannot panic. Ensure error messages
reference record_index/i, start, end, and buffer_len for diagnostics.
---
Nitpick comments:
In `@src/lib/commands/group.rs`:
- Around line 1216-1258: The two duplicated byte-level loops that inject/emit MI
(the one using output.extend_from_slice and the one writing to writer.get_mut())
should be factored into a single helper (e.g., emit_template_raw) that accepts
&Template, base_mi, &assign_tag_bytes, &mut scratch, &mut mi_buf and an emit:
impl FnMut(&[u8]) -> std::io::Result<()>; inside it reuse mi.write_with_offset,
call bam_fields::update_string_tag when has_mi, compute block size as a u32 from
scratch.len() or raw.len(), and call emit for the size bytes then payload;
replace both call sites to pass emit closures like |b| {
output.extend_from_slice(b); Ok(()) } and |b| writer.get_mut().write_all(b).
Ensure the helper returns std::io::Result<()> and use safe conversions
(u32::try_from or cast with checked bounds) for block size.
🪄 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: 32d5249e-9867-4f91-a69a-23baf19114e3
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (24)
crates/fgumi-metrics/Cargo.tomlcrates/fgumi-metrics/src/clip.rssrc/lib/commands/clip.rssrc/lib/commands/codec.rssrc/lib/commands/correct.rssrc/lib/commands/dedup.rssrc/lib/commands/duplex.rssrc/lib/commands/filter.rssrc/lib/commands/group.rssrc/lib/commands/review.rssrc/lib/commands/shared_metrics.rssrc/lib/commands/simplex.rssrc/lib/commands/zipper.rssrc/lib/grouper.rssrc/lib/mi_group.rssrc/lib/read_info.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/variant_review.rstests/integration/test_bam_pipeline.rstests/integration/test_dedup_command.rstests/integration/test_pipeline_concurrency.rs
✅ Files skipped from review due to trivial changes (3)
- crates/fgumi-metrics/Cargo.toml
- tests/integration/test_pipeline_concurrency.rs
- src/lib/commands/review.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- src/lib/commands/shared_metrics.rs
- src/lib/commands/correct.rs
- src/lib/commands/clip.rs
- src/lib/grouper.rs
- src/lib/commands/dedup.rs
- src/lib/commands/filter.rs
- src/lib/unified_pipeline/base.rs
…d RecordBuf helpers
Final pipeline collapse that removes the dual-storage Template and
DecodedRecord enums in favor of raw-byte-only storage, deletes newly-dead
RecordBuf helpers, and migrates clip/review to raw-byte APIs. Mostly
deletions -- see diff shape.
- Template dual storage collapsed to raw-only; in-lib unit tests ported
alongside the code they test (src/lib/template.rs, src/lib/mi_group.rs,
src/lib/grouper.rs, src/lib/read_info.rs, src/lib/tag_reversal.rs).
- DecodedRecord collapsed to a single raw variant; drops
record()/into_record() accessors and GroupKeyConfig::raw_byte_mode flag
(src/lib/unified_pipeline/bam.rs, src/lib/unified_pipeline/base.rs).
- Dead RecordBuf helpers removed: read_info::compute_group_key,
get_unclipped_position_for_groupkey, and tag_reversal::reverse_per_base_tags
along with their tests.
- clip command migrated to RawRecordClipper; clip metrics moved to the
shared fgumi-metrics crate (src/lib/commands/clip.rs,
crates/fgumi-metrics/src/clip.rs).
- review.rs migrated to RawRecord via IndexedRawBamReader, with
variant_review.rs updated to consume RawRecord.
- zipper: unmapped reader swapped to RawBamReader + RawTemplateIterator,
followed by deletion of the cfg(test) RecordBuf reference implementation
of restore_unconverted_bases and its oracle tests (raw-byte mirrors in
main already cover every scenario). Preserves main's tc/TC_TAG naming
and YD-based restoration semantics.
- Cascade updates across commands/{codec,dedup,duplex,group,simplex,
correct,filter,shared_metrics} and grouper/mi_group/unified_pipeline
to drop the dual paths and match the collapsed Template API.
Part 6 of 8 in the series closing #272. Stacks on #292.
nh13
force-pushed
the
nh/raw-bam-api-11-storage-pipeline-final
branch
from
April 20, 2026 07:59
dac71b7 to
636ae2c
Compare
This was referenced Apr 20, 2026
Merged
nh13
added a commit
that referenced
this pull request
Apr 21, 2026
Replaces the remaining production RecordBuf decode paths with raw-byte primitives from fgumi_raw_bam, eliminating per-record noodles decode allocations in the metrics, downsample, and zipper commands. shared_metrics::process_templates_from_bam (fixes the simplex-metrics / duplex-metrics regression introduced by #294): drops the per-template raw_record_to_record_buf round-trip -- which rebuilt a full synthetic BAM stream (prelude + @sq dictionary + framing) for every record -- and reads flags, tid, alignment positions, unclipped 5' positions, MI and RX tags directly from RawRecord bytes via RawRecordView, unclipped_5prime_from_raw_bam, alignment_end_from_raw, and find_string_tag_in_record. Local unclipped_five_prime_position helper dropped in favor of the raw-byte primitive. shared_metrics::validate_not_consensus_bam: replaces the raw_record_to_record_buf + consensus_tags::is_consensus path with a raw aux scan for cD / aD / bD via find_tag_type, inlining the simplex / duplex detection rules the consensus_tags module enforces. downsample::execute: swaps create_bam_reader / create_bam_writer / create_optional_bam_writer for the raw variants; rewrites FamilyIterator to operate on RawRecord; rewrites get_mi_tag to scan aux bytes directly (supports both Z-typed and integer MI encodings); drops the reader.record_bufs + write_alignment_record calls for write_raw_record on both the kept and rejected writers. zipper::execute (mapped side): changes MappedReader::Bam and MappedReader::StdinBam to hold RawBamReader variants, feeds each directly to TemplateIterator in the reader thread, and skips the RecordBufEncoder round-trip that record_bufs_to_templates previously required. The SAM arm retains record_bufs_to_templates because text-mode SAM has no raw-byte path. sort::execute, merge::merge_headers: the header-only callers swap create_bam_reader for create_raw_bam_reader (test helpers unchanged). compare::bams::deserialize_raw_record: documents that this noodles-decode-per-record is intentional and only runs off the comparison hot path when rendering typed diff output. Deletes the dead sort::keys::SortKey trait and its CoordinateKey / QuerynameKey impls -- zero production or test callers; the live sort path uses RawSortKey, RawCoordinateKey, and RawQuerynameKey / RawQuerynameLexKey exclusively.
nh13
added a commit
that referenced
this pull request
Apr 21, 2026
Replaces the remaining production RecordBuf decode paths with raw-byte primitives from fgumi_raw_bam, eliminating per-record noodles decode allocations in the metrics, downsample, and zipper commands. shared_metrics::process_templates_from_bam (fixes the simplex-metrics / duplex-metrics regression introduced by #294): drops the per-template raw_record_to_record_buf round-trip -- which rebuilt a full synthetic BAM stream (prelude + @sq dictionary + framing) for every record -- and reads flags, tid, alignment positions, unclipped 5' positions, MI and RX tags directly from RawRecord bytes via RawRecordView, unclipped_5prime_from_raw_bam, alignment_end_from_raw, and find_string_tag_in_record. Local unclipped_five_prime_position helper dropped in favor of the raw-byte primitive. shared_metrics::validate_not_consensus_bam: replaces the raw_record_to_record_buf + consensus_tags::is_consensus path with a raw aux scan for cD / aD / bD via find_tag_type, inlining the simplex / duplex detection rules the consensus_tags module enforces. downsample::execute: swaps create_bam_reader / create_bam_writer / create_optional_bam_writer for the raw variants; rewrites FamilyIterator to operate on RawRecord; rewrites get_mi_tag to scan aux bytes directly (supports both Z-typed and integer MI encodings); drops the reader.record_bufs + write_alignment_record calls for write_raw_record on both the kept and rejected writers. zipper::execute (mapped side): changes MappedReader::Bam and MappedReader::StdinBam to hold RawBamReader variants, feeds each directly to TemplateIterator in the reader thread, and skips the RecordBufEncoder round-trip that record_bufs_to_templates previously required. The SAM arm retains record_bufs_to_templates because text-mode SAM has no raw-byte path. sort::execute, merge::merge_headers: the header-only callers swap create_bam_reader for create_raw_bam_reader (test helpers unchanged). compare::bams::deserialize_raw_record: documents that this noodles-decode-per-record is intentional and only runs off the comparison hot path when rendering typed diff output. Deletes the dead sort::keys::SortKey trait and its CoordinateKey / QuerynameKey impls -- zero production or test callers; the live sort path uses RawSortKey, RawCoordinateKey, and RawQuerynameKey / RawQuerynameLexKey exclusively.
Merged
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
Final pipeline collapse that removes the dual-storage
TemplateandDecodedRecordenums in favor of raw-byte-only storage, deletes newly-deadRecordBufhelpers, and migrates theclip/reviewcommands to raw-byte APIs. Mostly deletions — see diff shape (~5.9K insertions, ~9.8K deletions).src/lib/template.rs,src/lib/mi_group.rs,src/lib/grouper.rs,src/lib/read_info.rs,src/lib/tag_reversal.rs).record()/into_record()accessors andGroupKeyConfig::raw_byte_modeflag (src/lib/unified_pipeline/bam.rs,src/lib/unified_pipeline/base.rs).read_info::compute_group_key,get_unclipped_position_for_groupkey, andtag_reversal::reverse_per_base_tagsalong with their tests.clipcommand migrated toRawRecordClipper.clipmetrics moved to the sharedfgumi-metricscrate (src/lib/commands/clip.rs,crates/fgumi-metrics/src/clip.rs).review.rsmigrated toRawRecordviaIndexedRawBamReader.variant_review.rsupdated to consumeRawRecord.zipper: unmapped reader swapped toRawBamReader+RawTemplateIterator, followed by deletion of thecfg(test)RecordBufreference implementation ofrestore_unconverted_basesand its oracle tests (raw-byte mirrors already cover every scenario). Preserves main'stc/TC_TAGnaming (fix(zipper)!: rename sort-key tag from pa to tc to avoid bwa-mem pa:f collision #270) andYD-based restoration semantics (perf(zipper): restore unconverted bases on raw BAM bytes #274).commands/{codec,dedup,duplex,group,simplex,correct,filter,shared_metrics}andgrouper/mi_group/unified_pipelineto drop the dual paths and match the collapsedTemplateAPI.Part 6 of 8 in the series closing #272. Stacks on #292. (Expanded from 7 to 8 PRs — see series comments.)
Test plan
cargo ci-test— 2450 tests passcargo ci-fmt— cleancargo ci-lint— clean