Repository navigation
fix(filter,clip): require query-grouped input (FILT3-02, CLIP3-05) - #517
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
WalkthroughSAM header utilities now identify query-grouped inputs and rebuild headers while preserving metadata. Clip and filter reject non-query-grouped BAMs before processing, with builders and integration tests updated for accepted and rejected header orders. ChangesQuery-grouped input enforcement
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant ClipOrFilter
participant BAMReader
participant require_query_grouped
User->>ClipOrFilter: run command with BAM
ClipOrFilter->>BAMReader: open input
BAMReader-->>ClipOrFilter: header
ClipOrFilter->>require_query_grouped: validate SO/GO/SS
require_query_grouped-->>ClipOrFilter: accept or error
ClipOrFilter-->>User: process input or reject command
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #517 +/- ##
=======================================
Coverage 92.60% 92.60%
=======================================
Files 165 165
Lines 99636 99709 +73
=======================================
+ Hits 92265 92336 +71
- Misses 7371 7373 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
98be98d to
a24cb2a
Compare
bd77b55 to
8d07a40
Compare
a24cb2a to
e80c30b
Compare
8d07a40 to
33c1059
Compare
e80c30b to
37e2e83
Compare
33c1059 to
43fa76c
Compare
37e2e83 to
d52374b
Compare
43fa76c to
80744f5
Compare
ae3dee2 to
4332216
Compare
80744f5 to
72072c4
Compare
fgbio's FilterConsensusReads and ClipBam both call Bams.requireQueryGrouped
before processing; fgumi did no such check. On coordinate-sorted input a
template's mates scatter, every read degrades to its own single-read
"template", and pair/template logic is silently wrong with a success exit
(filter drops all reads; clip no-ops pair clipping).
Add a query-grouped guard matching fgbio's isQueryGrouped
(SO:queryname || GO:query) — weaker than the consensus template-coordinate
guard, so a plain queryname sort is accepted:
- fgumi-sam: is_query_grouped() predicate, header_as_queryname(), and a
SamBuilder::set_queryname_sort_order() test helper.
- commands::common: require_query_grouped() with fgbio-style diagnostics
that echo the found SO/GO.
- Wire it into filter::execute and both clip::execute paths, guarding the
input header before clip rewrites SO for the output.
Real-tool parity (adversarial coordinate-sorted fixture with scattered mates):
before: fgbio errors; fgumi exits 0 (filter "kept 0, rejected 4"; clip no-op).
after: fgumi errors like fgbio ("not queryname sorted or query grouped,
found: SO:coordinate GO:none"); both accept query-grouped input
(clip 4->4 byte parity).
Test fixtures that fed bare-header BAMs updated to stamp SO:queryname.
Tracker: reports/2026-07-09-fgumi-final-audit-burndown-tracker.md (W2)
72072c4 to
c676f1b
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
What & why
fgbio's
FilterConsensusReadsandClipBamboth callBams.requireQueryGroupedbefore processing (FilterConsensusReads.scala:184,ClipBam.scala:115). fgumi did no such check. Both commands are template-based (they group a template's reads by adjacency), so on coordinate-sorted input a template's mates scatter, every read degrades to its own single-read "template", and the pair/template logic is silently wrong with a success exit:filterdrops all reads (each lone read fails the both-primaries-pass logic).clipno-ops pair clipping / overlap / mate-info fix.This is the filter/clip instance of the same missing-input-order-guard class fixed for the consensus callers in #508. Tracker IDs FILT3-02 (S3→S1) and CLIP3-05 (S3→S1).
The fix
Add a query-grouped guard matching fgbio's
isQueryGrouped(SO:queryname || GO:query) — deliberately weaker than the consensus template-coordinate guard (check_consensus_sort_order), so a plain queryname sort is accepted:fgumi-sam: newis_query_grouped()predicate;header_as_queryname()+ aSamBuilder::set_queryname_sort_order()test helper (the threeheader_as_*stampers now share arebuild_header_with_hdhelper).commands::common:require_query_grouped()with fgbio-style diagnostics that echo the foundSO/GO.filter::executeand bothclip::executepaths (single-threaded +--threads), guarding the input header before clip rewritesSOfor the output.Test fixtures that fed bare-header BAMs were updated to stamp
SO:queryname(bare headers are non-query-grouped, which fgbio also rejects — they were pinning invalid input).Real-tool parity evidence
Adversarial fixture: coordinate-sorted BAM with scattered mates (a template's R1/R2 non-adjacent).
requireQueryGrouped)not queryname sorted or query grouped, found: SO:coordinate GO:noneStacking
Stacked on #508 (
nh/fix-consensus-sort-order-guards) — filter/clip already edited there, and it introduces the siblingcheck_consensus_sort_order. Merge order: #505 → #508 → this PR. GitHub auto-retargets tomainas the stack merges.The third W2 finding, MERGE3-01, ships as its own
main-based PR (different mechanism, non-overlappingmerge.rs, no in-flight merge PR).Tracker:
reports/2026-07-09-fgumi-final-audit-burndown-tracker.md(W2a).Summary by CodeRabbit
New Features
Bug Fixes
clipandfilternow reject coordinate-sorted or otherwise incompatible BAM files before processing.