Repository navigation
fix(correct,filter,clip,review): synthesize @HD when input lacks one (match fgbio) - #527
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (8)
WalkthroughBAM header handling now guarantees an ChangesHeader normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #527 +/- ##
==========================================
- Coverage 92.71% 92.69% -0.03%
==========================================
Files 166 166
Lines 100982 101060 +78
==========================================
+ Hits 93630 93677 +47
- Misses 7352 7383 +31 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
fb62064 to
b8b66e5
Compare
b8b66e5 to
f052dc1
Compare
f052dc1 to
6de00f2
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 (1)
src/lib/commands/clip.rs (1)
236-260: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winInconsistent guard ordering produces different rejection diagnostics for the same input depending on
--threads.In
execute()(threads path),require_query_groupedruns on the raw header beforeensure_hd_record, so a header-less input reportsSO:<default>in the error. Inexecute_single_threaded(),ensure_hd_recordruns first, so the same header-less input now hasSO:unsortedbaked in before the check, producing a differentfound: SO:... GO:...message for the identical failure. Both paths still reject (tests only check for the "queryname sorted or query grouped" substring), but the diagnostic detail silently diverges by execution mode. Moveensure_hd_recordbeforerequire_query_groupedinexecute()too (matchingexecute_single_threaded()) for consistent diagnostics.🔧 Suggested fix
let (reader, header) = create_bam_reader_for_pipeline(&self.io.input)?; + // Synthesize `@HD` VN:1.6 SO:unsorted when the input lacks one (match fgbio). + let header = crate::commands::common::ensure_hd_record(header)?; + // CLIP3-05: fgbio's ClipBam calls Bams.requireQueryGrouped. Clipping is // template-based (pair clip, overlap, past-mate, mate-fix), so coordinate- // sorted input silently mis-groups mates. Guard the *input* header. crate::commands::common::require_query_grouped( &header, &self.io.input.display().to_string(), )?; // Load reference (always required for clip) let reference = Arc::new(ReferenceReader::new(&self.reference)?); - // Synthesize `@HD` VN:1.6 SO:unsorted when the input lacks one (match fgbio). - let header = crate::commands::common::ensure_hd_record(header)?; - // Add `@PG` record with PP chaining to input's last program let header = crate::commands::common::add_pg_record(header, command_line)?;Also applies to: 289-298
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lib/commands/clip.rs` around lines 236 - 260, In the threaded branch of execute, call ensure_hd_record immediately after create_bam_reader_for_pipeline and before require_query_grouped, then use the normalized header for the guard and subsequent add_pg_record call. Keep the single-threaded ordering unchanged so both execution modes produce consistent rejection diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/lib/commands/clip.rs`:
- Around line 236-260: In the threaded branch of execute, call ensure_hd_record
immediately after create_bam_reader_for_pipeline and before
require_query_grouped, then use the normalized header for the guard and
subsequent add_pg_record call. Keep the single-threaded ordering unchanged so
both execution modes produce consistent rejection diagnostics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dd5420ae-f59a-454e-aa6d-65eb85fd69ef
📒 Files selected for processing (8)
crates/fgumi-bam-io/src/header.rssrc/lib/commands/clip.rssrc/lib/commands/common.rssrc/lib/commands/correct.rssrc/lib/commands/filter.rssrc/lib/commands/review.rstests/integration/test_clip_command.rstests/integration/test_filter_command.rs
6de00f2 to
6b7ab52
Compare
|
Addressed the outside-diff finding on Note on the stated rationale: the diagnostics did not actually diverge today — for header-less input, Also parameterized |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lib/commands/review.rs`:
- Around line 720-722: Add an end-to-end `review` test covering input without an
`@HD` record, then inspect the generated `.consensus.bam` and `.grouped.bam`
files to assert each contains the synthesized `@HD VN:1.6 SO:unsorted` header.
Reuse the existing review test setup and output-reading helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5c50177b-bb7c-4f92-b101-54ab7b3f1877
📒 Files selected for processing (8)
crates/fgumi-bam-io/src/header.rssrc/lib/commands/clip.rssrc/lib/commands/common.rssrc/lib/commands/correct.rssrc/lib/commands/filter.rssrc/lib/commands/review.rstests/integration/test_clip_command.rstests/integration/test_filter_command.rs
fgumi commands that read an input header and pass it through (adding only @pg) propagated a missing @hd line, producing a BAM whose header starts at @pg. fgbio synthesizes @hd VN:1.6 SO:unsorted for header-less input; fgumi now matches. Add fgumi_bam_io::header::ensure_hd_record, which inserts @hd VN:1.6 SO:unsorted only when the header lacks one (an existing @hd and its sort order are left untouched), and wire it into the passthrough commands that can otherwise emit a header-less BAM: correct, filter, clip, and review. Commands that already cannot emit a header-less BAM are intentionally not touched: dedup, downsample, and group reject non-template-coordinate input at their sort-order guard; simplex/duplex/codec build their output header via create_unmapped_consensus_header; and zipper builds @hd from the sequence dictionary. Verified on real data: fgumi correct on the header-less data/raw/correct_small.bam (from an old 0.1.3 simulate) now emits @hd VN:1.6 SO:unsorted, byte-identical to the fgbio baseline, and a content comparison vs fgbio is IDENTICAL (0 core-field and 0 tag-value diffs; the only residue is 10,134 records with tags in a different order, values equal).
6b7ab52 to
6131db9
Compare
This branch was previously deployed
1 participant
Summary
fgumi commands that read an input header and pass it through (adding only
@PG) propagate a missing@HDline, producing a BAM whose header starts at@PG. fgbio synthesizes@HD VN:1.6 SO:unsortedfor header-less input, and most downstream tooling expects an@HD. This makes fgumi match.New shared helper
fgumi_bam_io::header::ensure_hd_recordinserts@HD VN:1.6 SO:unsortedonly when the header lacks one — an existing@HD(and its sort order) is left untouched — wired into the passthrough commands that can otherwise emit a header-less BAM:correct,filter,clip,review.Scope: which commands, and why
Fixed (passthrough → written header, no guard):
correct,filter,clip,reviewIntentionally not touched, because they already cannot emit a header-less BAM:
dedup,downsample,group— reject non-template-coordinate input at their sort-order guard before the writer.simplex/duplex/codec— build their output header viacreate_unmapped_consensus_header(always carries@HD).zipper— builds@HDfrom the sequence dictionary.Evidence (real data)
The bug surfaced on
data/raw/correct_small.bam, generated by an oldfgumi 0.1.3 simulate correct-readsthat emitted no@HD(header starts at@PG). Running the fixedfgumi correcton it (matching the campaign's--max-mismatches 2 --min-distance 2):@HD VN:1.6 SO:unsorted— byte-identical to the fgbioCorrectUmisbaseline.Testing
ensure_hd_record(synthesizes when absent; preserves an existing@HD/SO).correct,filter, andclipon header-less input and assert the output carries@HD VN:1.6 SO:unsorted.cargo nextest2244 passed,cargo clippy --workspace --all-targets --features compare,simulate,profile-adjacency -- -D warnings -W clippy::pedanticclean,cargo fmt --checkclean.Summary by CodeRabbit
@HDheader record when missing.@HDrecords are preserved exactly (including version and all fields).@HDsynthesis and headerless-input rejection for clip and filter.