fix(merge): validate input sort order to prevent silent corruption (MERGE3-01) - #519
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesMerge now validates BAM header declarations and streamed per-source ordering before exposing output. Regular-file destinations use staged atomic persistence with Unix mode and symlink handling. Tests cover failures, cleanup, ties, permissions, and successful interleaving. Merge order enforcement and output finalization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant MergeCommand
participant HeaderValidation
participant MergeLoop
participant MergeOutputTarget
MergeCommand->>HeaderValidation: validate input declarations against --order
HeaderValidation-->>MergeCommand: compatible headers or error
MergeCommand->>MergeLoop: merge records with input paths and key extractor
MergeLoop->>MergeOutputTarget: write staged output
MergeLoop->>MergeLoop: verify source monotonicity
MergeLoop->>MergeOutputTarget: persist on success
MergeLoop-->>MergeCommand: error without output on violation
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #519 +/- ##
==========================================
+ Coverage 91.12% 92.73% +1.61%
==========================================
Files 78 166 +88
Lines 51540 101260 +49720
==========================================
+ Hits 46964 93905 +46941
- Misses 4576 7355 +2779 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
2612f62 to
67435d3
Compare
67435d3 to
a65340f
Compare
|
@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.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@crates/fgumi-sort/src/external.rs`:
- Around line 306-308: Update the output finalization flow around temp.persist
in the external sort implementation to preserve symlink destinations: detect
when output is a symlink and resolve it to the linked target before staging or
persisting, or explicitly reject symlink outputs. Ensure normal non-symlink
output handling and existing error context remain unchanged.
In `@tests/integration/test_merge_command.rs`:
- Around line 141-169: Add an integration test alongside
test_merge_streaming_verify_rejects_missorted_bare_header_no_partial_output that
supplies consecutive records with equal coordinates and verifies
coordinate-sorted merge succeeds. Keep the existing strictly decreasing-position
rejection test unchanged, and assert that equal adjacent positions are accepted
rather than treated as mis-sorted.
- Around line 79-94: Strengthen the assertions in
test_merge_rejects_coordinate_input_into_template_coordinate_default so the
error message must identify the first offending input, a.bam, in addition to
containing the existing coordinate-order text. Match the sibling streaming-check
test’s file-name assertion pattern and retain the no-output assertion.
- Around line 99-135: Extend test_merge_valid_coordinate_succeeds after the
successful merge and output-content assertions to independently scan the output
directory for entries matching the .fgumi-merge-*.tmp temporary-file pattern,
and assert that none remain after the atomic rename.
🪄 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: 9464aa92-8600-43d6-99ec-9bb15cad0299
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (6)
CLAUDE.mdcrates/fgumi-sort/Cargo.tomlcrates/fgumi-sort/src/external.rssrc/lib/commands/merge.rstests/integration/main.rstests/integration/test_merge_command.rs
a65340f to
8b55ec4
Compare
|
@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.
|
✅ 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: 2
🤖 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 `@crates/fgumi-sort/src/external.rs`:
- Around line 354-411: Update process_umask in crates/fgumi-sort/src/external.rs
to use nix::sys::stat::umask with Mode::empty() while retaining UMASK_LOCK
serialization, and remove the unsafe block, safety note, and unsafe allow.
Remove the unix-target libc dependency from crates/fgumi-sort/Cargo.toml and
delete the corresponding libc::umask approved-FFI exception from CLAUDE.md.
In `@tests/integration/test_merge_command.rs`:
- Around line 270-297: Add a Unix test alongside
test_merge_output_preserves_existing_destination_mode that merges into a
destination path that does not already exist, then compares its permission bits
with a control file created via File::create under the same conditions. Assert
the new merged output matches the control file’s mode, covering regressions
where the staging NamedTempFile’s 0600 permissions leak into new outputs.
🪄 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: 52a7bc75-c879-4a06-b70c-1f66bc615dde
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (6)
CLAUDE.mdcrates/fgumi-sort/Cargo.tomlcrates/fgumi-sort/src/external.rssrc/lib/commands/merge.rstests/integration/main.rstests/integration/test_merge_command.rs
9f60916 to
56ba829
Compare
|
@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: 2
🤖 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 `@crates/fgumi-sort/src/external.rs`:
- Around line 266-288: Update `OutputFile::create` to inspect the resolved
destination before staging: permit only missing paths or existing regular files,
and for FIFOs, devices, sockets, or other non-regular destinations either reject
them or use the direct-write path. Ensure `persist` cannot rename a regular
temporary file over a special output destination, while preserving stdout and
symlink-resolution behavior.
In `@tests/integration/test_merge_command.rs`:
- Around line 174-200: Extend the streaming merge validation tests beyond
test_merge_streaming_verify_rejects_missorted_bare_header_no_partial_output by
adding rstest cases for queryname, queryname-natural, and template-coordinate.
Generate records in both valid and invalid orders, validate expected ordering
with an independent comparator/baseline, and assert merged record identities and
ordering rather than only counts; retain rejection and no-partial-output
assertions for invalid inputs.
🪄 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: 47e3db30-4a79-459e-b121-8ddaa2228f33
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (6)
CLAUDE.mdcrates/fgumi-sort/Cargo.tomlcrates/fgumi-sort/src/external.rssrc/lib/commands/merge.rstests/integration/main.rstests/integration/test_merge_command.rs
…ERGE3-01) fgumi merge is a k-way loser-tree merge that assumes each input is already monotonic in the --order key; a mis-sorted input silently produces out-of-order output stamped with the requested SO. Neither reference tool guards this the way the audit assumed: samtools merge trusts the caller identically (its docs only warn output "may not be sorted"), and Picard MergeSamFiles re-sorts rather than rejecting. fgumi is a merge (not a sorter), so it adds an explicit guard, converting silent S1 corruption into a clear error. This also defuses the sharper fgumi-specific footgun: the --order template-coordinate default means `fgumi merge a.bam b.bam` on ordinary coordinate-sorted BAMs would otherwise corrupt. Two complementary mechanisms (hybrid): - Fast header check (merge.rs): reject an input whose header declares an order conflicting with --order, before any records are read. Inputs that declare no usable order (bare/unsorted) pass here and are verified below. - Streaming monotonicity verify (fgumi-sort run_merge_loop): the merge already extracts a key per record; compare each newly pulled key against the just-emitted key from the same source (tree.winner_key()), erroring if it goes backward. Reuses `fgumi sort --verify` semantics, ~one extra Ord comparison per record on an already comparison-heavy loop, and catches actual disorder regardless of what the header claims. The merge writes to a sibling temp and atomically renames on success, so a mid-merge rejection leaves no partial output (streamed stdout excepted). Real-tool evidence (--order coordinate; coordinate BAM + a queryname-sorted BAM whose record order differs): before, fgumi/samtools both emit mis-ordered records with a success exit and an SO:coordinate header; after, fgumi errors naming the offending input and record. Valid merges are unaffected. Tracker: reports/2026-07-09-fgumi-final-audit-burndown-tracker.md (W2b); audit premise corrected in the decisions log.
56ba829 to
d1b9167
Compare
This branch was previously deployed
1 participant
What & why (MERGE3-01, S1)
fgumi mergeis a k-way loser-tree merge that assumes every input is already monotonic in the--orderkey. A mis-sorted input silently produces out-of-order output stamped with the requestedSO— an S1 silent corruption.The audit's premise was that samtools/Picard reject this and fgumi should match them. Real-tool repro shows that's not true:
--order coordinate, inputs = 1 coordinate-sorted + 1 queryname-sorted (differing record order)SO:coordinateheader, mis-ordered records — silent S1So fgumi already matches
samtools merge(its true analog); Picard is a different operation (fgumi has a separatesort). fgumi therefore adds an explicit guard — converting silent corruption into a clear error — which is stricter than both reference tools but is the correctness-first choice. It also defuses the sharper fgumi-specific footgun: the--order template-coordinatedefault meansfgumi merge a.bam b.bamon ordinary coordinate BAMs would otherwise corrupt.Disposition was confirmed with the maintainer (tracker §2 decisions log).
The fix — hybrid guard
Fast header check (
merge.rs): reject an input whose header declares an order conflicting with--order, before any records are read. The error names both the input's order and how to fix it (--order <declared>to merge as-is, or sort to the requested order). Inputs that declare no usable order (bare /SO:unsorted) pass here and are verified below. Folded intomerge_headersso it reuses the single header read.Streaming monotonicity verify (
fgumi-sortrun_merge_loop): the merge already extracts a key per record; each newly pulled key is compared against the just-emitted key from the same source (LoserTree::winner_key()), erroring if it goes backward. Reusesfgumi sort --verifysemantics, catches actual disorder regardless of what the header claims (bare/lying headers), and costs ~one extraOrdcomparison per record on an already comparison-heavy loop. The merge writes to a sibling temp and atomically renames on success, so a mid-merge rejection leaves no partial output (streamed stdout excepted, where rename is impossible).The default
--orderis left unchanged (no CLI break).Evidence (after)
--order coordinate→ errors.--order coordinate→ passes the header check, then the streaming verify errors ("record 2 … sorts before a preceding record"), and no partial/temp file is left.Tests
test_check_input_declared_order(rstest, 11 cases: the full declared×requested matrix + undeclared pass-through) andtests/integration/test_merge_command.rs(declared-conflict reject, valid merge order, streaming-verify reject + no-partial-output assertion).Main-based (non-overlapping
merge.rs/external.rs; no in-flight merge PR). Tracker:reports/2026-07-09-fgumi-final-audit-burndown-tracker.md(W2b).Summary by CodeRabbit
mergenow validates each input BAM header’s declared sort order against the requested--order, failing fast on incompatible inputs.