feat(io): accept uncompressed SAM and stdin anywhere BAM is accepted

[nh13]

Closes #642.

Every fgumi reader was a BGZF reader, so no command accepted uncompressed SAM. -i in.sam failed with invalid BGZF header, which tells the user nothing about what to do, and it failed that way regardless of --threads — verified against the built binary on main before writing any code:

$ fgumi simplex -i in.sam -o out.bam --min-reads 1
Error: Failed to read header from: in.sam
Caused by:  invalid BGZF header

$ fgumi simplex -i in.sam -o out.bam --min-reads 1 --threads 4      # same

Input format is a property of the data, not of the command, and it must not depend on which reader a flag happens to select. The same argument applies to where the data comes from, so this PR holds stdin to the same standard: every command that streams records accepts -.

Approach

Normalize at the boundary rather than teaching every consumer a second record source.

fgumi-bam-io's new sam_input module sniffs the input's leading bytes and, for SAM text, parses and re-encodes it on the fly into the BGZF stream the consumers already expect — so -i in.sam and -i in.bam are the same stream by the time any consumer sees them. Nothing downstream of the reader changed.

The alternative — a second, text record source threaded through the single-threaded fast paths, the pipelines and the sort engine — is what produces per-command and per-flag divergence in the first place. One conversion point cannot drift.

The transcode uses CompressionLevel::NONE: the bytes are decoded again microseconds later in the same process, so there is nothing to gain from deflating them.

Detection is by content, never by extension, so a .bam holding SAM text and a .sam holding BGZF both read correctly. Only a regular file is sniffed by path and re-opened; stdin, FIFOs and process substitution cannot be re-opened, so they are opened once and the peeked bytes replayed.

One classifier, not three

BGZF, plain gzip and text were previously distinguished by 2-, 4- and 18-byte checks in three different places, which disagreed with each other. A plain-gzipped SAM was called BGZF by one and SAM text by another: sort failed with Failed to read header, zipper with invalid flags — the exact confusion the content sniff exists to prevent. fgumi-bam-io::format::classify_input (Bgzf / Gzip / Text / Empty) now answers the question everywhere. Gzip-but-not-BGZF is named as such, with the bgzip fix in the message, instead of failing as a corrupt BAM. fgumi extract keeps gzip as a supported input, since FASTQ legitimately arrives that way.

Input that is neither BGZF nor SAM is rejected rather than accepted as headerless SAM — a SAM header parse consumes only @ lines, so arbitrary text would otherwise yield an empty header and transcode into a valid-looking BAM carrying no reference sequences at all. Empty input is reported as empty rather than as a malformed header.

Reader surfaces

Reader	Used by
create_bam_reader_with_opts	typed whole-record consumers
create_raw_bam_reader_with_opts	single-threaded fast paths
create_bam_reader_for_pipeline_with_opts	--threads N pipelines
create_raw_bam_reader_pool_integrated	the sort engine
open_raw_bam_record_reader_with_header	sort --verify, compare
create_bam_reader_for_pipeline	both unified_pipeline entry points
the shared normalizing reader	zipper's mapped input

The pipeline and pool readers parsed their header by rewinding, which cannot work for a transcoded stream. Both now parse through a TeeReader and replay the consumed bytes — the same trick the stdin branch already used. Besides enabling SAM, that means the input is opened exactly once: the file branches previously opened, seeked to 0 and re-read, which never worked for FIFOs (fgumi -i <(samtools view …)).

sort --verify and compare read their header through a format-aware reader and then re-opened the path with a bare File, which put them back on the BGZF-only path and made them reject the very files sort accepted. zipper chose its mapped reader by file extension and so was the one command a misnamed file could still fool. Both are fixed; without them the PR's central claim was simply false.

stdin

Every command that reads records streams stdin. Three deliberately do not:

Command	Why not
review	requires BAI indexes and does query() random access
merge	takes N pre-sorted inputs via a -b list; one stream cannot supply N
compare	compares two named inputs against each other

extract streams when there is exactly one --input; - with two or more is rejected outright.

Getting there removed three separate double-reads:

• fastq, duplex-metrics and simplex-metrics rejected -i - at validation with File does not exist, because existence was checked before anything knew - names a stream. validate_input_exists now exempts stdin for streamable inputs while leaving references and include lists strictly checked.
• sort --verify opened its input twice, once for the header and once for the records; it now takes both from a single open.
• The metrics commands pre-scanned for a consensus BAM by opening the input a second time. They still reject consensus BAMs — the guard now runs against the first qualifying record of the pass they already make. Its predicate is deliberately looser than the main loop's and must stay that way: it must not skip unmapped reads, since consensus BAMs are unaligned and skipping them would hole the very check.
• fgumi fastq -i - failed outright when --output already existed. The guard that refuses to truncate the input BAM canonicalises both paths whenever the output exists, and canonicalize("-") is a NotFound error, so accepting stdin turned the common case of overwriting a prior run's output into an unexplained IO failure. reject_output_clobbering_input now exempts stdin — it names no file, so it can clobber nothing.
• extract opened each input three times — a quality-encoding sample, a BGZF probe, and the real pass — so stdin was drained before the records were read and -i - silently produced an empty BAM while exiting zero. The sample now reads through a tee and replays what it consumed.

zipper no longer has a text mode

zipper kept a second, text-mode orchestration because "text-mode SAM has no raw-byte path". With SAM transcoded at the boundary it does have one, so the mapped input goes through the shared normalizing reader and the command never learns which format arrived — there is no format branch left at all.

That turned out to be faster, which refuted the reason it had been deferred. On a 2.4M-record zip:

input	before	after
SAM file	2.9s	2.1s
SAM pipe	2.9s	2.28s
BAM file (control)	1.6s	1.6s

Output is byte-identical across all five input shapes. The transcode's encode replaces both the RecordBuf decode and the re-encode the merge used to pay, and framing at CompressionLevel::NONE is nearly free.

MappedReader collapsed from an enum to a raw reader and then disappeared; record_bufs_to_templates, file_is_bgzf and peek_bgzf became dead. zipper sheds ~255 lines.

SharedBuffer::is_empty no longer takes a mutex per record either. The transcode asks the sink whether the encoder has emitted a block once for every record it writes; that is now an atomic load against a length write and swap_into maintain inside the critical sections they already hold.

Separately, TemplateIterator no longer copies both read names into owned Vecs to decide whether two records belong to the same template — two heap allocations per record, on every zipper run, to answer a memcmp. It compares the name bytes in place (~4.8M allocations avoided on the 2.4M-record run). The remaining per-record allocation there — one RawRecord buffer per record — needs a pool with a cross-thread return path and is tracked separately in #654.

Deletions and API surface

• src/lib/batched_sam_reader.rs (662 lines) — deleted. It fed an adaptive buffer whose batch tracking was driven only by record_parsed(), which has zero production callers, so zipper --bwa-chunk-size was already inert before this PR. The buffer also cost a full-stream copy of the SAM text and 128 MiB of reservation, for buffering the normalizing reader already does. The flag is still parsed so existing command lines keep working, and hidden so new ones do not adopt it; removing it is a CLI break and belongs in its own change. (fgumi fastq has its own --bwa-chunk-size, which is live and untouched.)
• run_bam_pipeline_with_header (99 lines) — deleted; zero callers anywhere, including tests.
• run_bam_pipeline_with_grouper — moved behind a test-utils feature. Production commands all drive the pipeline through the *_from_reader family; this path-taking wrapper's only callers are integration tests, and leaving a generic with five type parameters in the default public API advertises it as a supported entry point that every future I/O change has to be kept in step with — a cost this PR already paid once. A self dev-dependency turns the feature on for the test targets, so cargo test needs no extra flag and cargo build never sees it. (.cargo/config.toml already had a cargo t = "test --features test-utils" alias naming a feature the crate did not define; it works now.)

review stays BAM-only, deliberately

fgumi review requires BAI indexes on --consensus-bam and --grouped-bam and does query() random access against them. Indexed random access is inherently BGZF-only — a plain SAM file cannot carry a BAI — so review is BAM-only by construction, not by omission.

It also has two sequential opens that I could have converted. I did not: making half of review's reads accept SAM while the indexed half still requires BGZF would reintroduce exactly the per-path inconsistency this PR removes. If review should accept SAM it needs a design decision (e.g. a non-indexed full-scan mode), not a reader swap.

Errors name the stage that actually failed

The transcode is lazy, so records were being pulled while the consumer was still reading the header — a malformed record on line 4 was reported as a header failure. The header is now flushed into its own BGZF block, so the header read completes before any record is touched.

Tests

Written test-first. The three reader-factory tests were watched failing with invalid BGZF header before the module existed, and sort failed the integration table exactly as predicted until its pool reader was converted.

• sam_input unit tests — transcoded stream is a readable BAM; byte-at-a-time reads match bulk reads (the staging buffer refills across read calls); header-only SAM yields a valid empty BAM; BGZF passes through byte-identical; empty input is rejected.
• format unit tests — a 7-case table over BGZF / plain gzip / SAM / empty / truncated gzip magic / short text / gzip-with-a-non-BC-extra.
• reader unit tests — the public factories accept uncompressed SAM.
• tests/integration/test_sam_input.rs — for each command, the same data as .bam and as .sam must both succeed and produce the same records. Cases run with and without --threads, so no future change can make format support depend on thread count. Plus misnamed extensions in both directions, piped input, a FIFO regression case, the rejection cases, and record-vs-header error attribution.
• tests/integration/test_input_source_matrix.rs — a CONTRACTS table pinning the whole contract: every command the binary advertises must declare whether it accepts SAM and whether it accepts stdin, and the declarations are checked against fgumi --help. A new command fails the suite until it states its stance rather than silently inheriting whichever reader it happened to pick. The SAM axis compares results, not exit codes: each run writes to its own directory (both previously wrote to one {command}.out, so the SAM run overwrote the BAM output it was meant to be checked against) and the BAM run is the oracle.
• sort --verify parity compares violations as well as records checked — a SAM-specific record-boundary bug could preserve the record total while corrupting order detection, and a count-only comparison would pass — with a case whose input is deliberately out of queryname order.

One stated limit: integer aux width

Comparing the two runs as bytes is not achievable and the matrix does not try. SAM's i type carries no width, so a cD:i:10 read from SAM text is re-encoded as Int32 while the same tag in the original BAM may have been stored as Int8. Every writer picks its own rule — fgumi narrows to the smallest signed type, htslib prefers unsigned, noodles does not narrow — so requiring identical aux encoding across a SAM round-trip would require something no BAM writer guarantees. The matrix therefore renders both outputs to SAM text and compares the values, which is the property that actually has to hold. Whether fgumi should narrow on the SAM path to match is tracked in #655.

cargo ci-test 6000 passed / 27 skipped; cargo ci-fmt, cargo ci-lint, and cargo check --workspace with both --no-default-features and --all-features all clean.

What changed since the first review

The first round of review (CodeRabbit plus an adversarial pass) found that the PR's headline claim did not hold, and the fixes cascaded well past the original scope. Summarising, since this description was first written:

• sort --verify, compare and zipper did not actually accept SAM, despite being listed as covered. Fixed; the "Reader surfaces" table above is now the real list.
• The whole stdin half is new — the streaming contract, the three double-reads, extract's tee-and-replay.
• The integration table is no longer a sample. The earlier text said extending it beyond sort/group/fastq needed per-command fixtures and was deferred to Every command that accepts BAM must accept SAM #642. It is done: CONTRACTS is exhaustive and self-enforcing against fgumi --help.
• zipper was rearchitected rather than given a format branch, and the benchmark above refuted the deferral.
• The three-way format sniff was unified into one classifier, and plain gzip is now rejected with an actionable message instead of being mistaken for BGZF. The earlier claim that "BGZF begins with the gzip magic and SAM text cannot" was true but insufficient — plain gzip also begins with the gzip magic, and used to be misread as BGZF.
• Deletions and the API-surface change in the section above.
• A second review round (this one) fixed the fastq stdin/clobber regression above, replaced the matrix's exit-code check with an output comparison, added the sort --verify violation parity and the duplex cases for the consensus guard, and removed the per-record mutex from the transcode.
• Test count moved from 5746/23 to 6000/27.

Note on feat-runall

That line has its own SAM support via the typed-step chain (InputSource::open_with_opts, suffix-then-magic detection), with #547 and #549 closing its last two gaps. This PR does not touch it — it targets main, which has no chain at all and is where the format gate is currently total. The two lines will need reconciling; the invariant should hold on whichever survives.

Summary by CodeRabbit

• New Features

  • Commands that accept BAM input now also accept uncompressed SAM directly (including stdin/FIFOs and content-based detection even with mismatched extensions), with automatic normalization to BGZF/BAM.
  • sort --verify now works correctly with streamed, non-seekable stdin inputs.
  • Enhanced streaming support for extract, zipper, and related pipelines.

• Bug Fixes

  • Improved handling of empty/malformed/non-SAM-or-BAM inputs, with clearer fault attribution.
  • Avoids hangs with non-reopenable streaming inputs (FIFO) during compare/verification-style engines.
  • Consensus handling in metrics is now record-based (instead of rejecting the whole input up front).

• Tests

  • Expanded coverage for stdin/FIFO behavior, format equivalence, and sort --verify result consistency.

[coderabbitai]

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

Walkthrough

The PR adds content-based SAM detection and streaming SAM-to-BAM normalization, integrates replayable readers into command and pipeline paths, broadens stdin support, centralizes input validation, moves consensus checks into streaming processing, and adds cross-command input-source coverage.

Changes

SAM input support

Layer / File(s)	Summary
SAM-to-BAM normalization crates/fgumi-bam-io/src/{format.rs,sam_input.rs,lib.rs}, Cargo.toml	Classifies content, lazily transcodes SAM to BGZF/BAM, passes through BGZF, rejects unsupported formats, and exposes normalization APIs.
BAM reader replay integration crates/fgumi-bam-io/src/reader.rs, crates/fgumi-sort/src/{external.rs,reader.rs,verify.rs,lib.rs}, src/lib/unified_pipeline/bam.rs	Opens inputs once, parses headers without seeking, replays consumed bytes, supports boxed readers, and removes reopen paths.
Command reader and validation wiring src/lib/commands/{common,extract,sort,zipper,compare,fastq,review,duplex_metrics,simplex_metrics,shared_metrics}.rs, src/lib/validation.rs	Adds stdin-aware validation, stream sniffing and replay, normalized raw readers, single-pass extraction, zipper handling for non-regular inputs, and lazy consensus checks.
Consensus and supporting APIs crates/fgumi-consensus/src/{filter.rs,lib.rs}, crates/fgumi-simd-fastq/src/reader.rs	Adds raw-aux consensus predicates and FASTQ reader unwrapping.
Input-source integration contracts tests/integration/*, src/lib/template.rs, src/lib/mod.rs	Tests BAM/SAM equivalence, stdin and FIFO flows, content detection, command contracts, verification, malformed input, invalid input, and template batching while removing the obsolete batched SAM reader.

Estimated code review effort: 5 (Critical) | ~90 minutes

Possibly related issues

• Every command that accepts BAM must accept SAM #642 — Covers the SAM-input invariant implemented by content-based normalization and cross-command tests.

Possibly related PRs

• fulcrumgenomics/fgumi#568 — Overlaps the single-open owned-reader plumbing used by sort verification.
• fulcrumgenomics/fgumi#594 — Also modifies the sort-verification comparison pipeline in sort_verify.rs.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: expanding BAM-accepted inputs to include uncompressed SAM and stdin.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch nh/feat-accept-sam-input

---

_{Comment @coderabbitai help to get the list of available commands.}

[codecov]

Codecov Report

❌ Patch coverage is 95.77836% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 93.82%. Comparing base (24c5466) to head (3d49b1b).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/fgumi-bam-io/src/sam_input.rs	94.57%	14 Missing ⚠️
crates/fgumi-sort/src/reader.rs	73.07%	7 Missing ⚠️
src/lib/commands/extract.rs	93.68%	6 Missing ⚠️
src/lib/commands/zipper.rs	91.17%	3 Missing ⚠️
crates/fgumi-bam-io/src/reader.rs	98.83%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #644      +/-   ##
==========================================
+ Coverage   93.57%   93.82%   +0.24%     
==========================================
  Files         175      176       +1     
  Lines      106821   107536     +715     
==========================================
+ Hits        99963   100896     +933     
+ Misses       6858     6640     -218     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[coderabbitai]

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-bam-io/src/sam_input.rs`:
- Around line 143-155: Update the batching loop in the reader method containing
self.reader.read_record and self.writer.write_alignment_record to stop as soon
as the writer has produced the first BGZF output bytes, preventing a single read
from buffering an entire 1,024-record batch; retain EOF finalization for
exhausted input. Add a regression test that performs a one-byte read with
long-read or sufficiently large SAM input and verifies it returns promptly
without unbounded buffering.

In `@tests/integration/test_sam_input.rs`:
- Around line 65-74: Update tests/integration/test_sam_input.rs:65-74 so
read_records returns each output header together with its records, normalizing
only path-dependent generated `@PG` data. At
tests/integration/test_sam_input.rs:123-127, assert normalized header and record
parity between BAM and SAM runs. At tests/integration/test_sam_input.rs:151-155,
run the canonical BAM sort and compare the misnamed file’s complete normalized
output, including headers and records, against it.

🪄 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: f14ace6c-8631-4dc6-94b6-6d33c5af280b

📥 Commits

Reviewing files that changed from the base of the PR and between 50372a6 and 6fcbaa1.

📒 Files selected for processing (7)

• crates/fgumi-bam-io/src/lib.rs
• crates/fgumi-bam-io/src/reader.rs
• crates/fgumi-bam-io/src/sam_input.rs
• crates/fgumi-sort/src/external.rs
• tests/integration/helpers/bam_generator.rs
• tests/integration/main.rs
• tests/integration/test_sam_input.rs

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

crates/fgumi-sort/src/external.rs (1)

3932-3948: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

advise_sequential skipped on the default (non-async) sort input path.

advise_sequential(&file) only runs inside the async_reader branch here, but the two equivalent call sites in crates/fgumi-bam-io/src/reader.rs (Lines 185, 391) now call it unconditionally as part of this PR. Sort's default synchronous file read misses the kernel readahead hint that the pipeline/BGZF readers get.

⚡ Proposed fix

         let file = std::fs::File::open(path_ref)
             .with_context(|| format!("Failed to open input BAM: {}", path_ref.display()))?;

+        fgumi_bam_io::os_hints::advise_sequential(&file);
+
         if async_reader {
-            fgumi_bam_io::os_hints::advise_sequential(&file);
             log::debug!(
                 "async sort reader enabled: spawning fgumi-prefetch thread for {}",
                 path_ref.display()
             );
             Box::new(fgumi_bam_io::prefetch_reader::PrefetchReader::from_file(file))
         } else {
             Box::new(io::BufReader::with_capacity(2 * 1024 * 1024, file))
         }

🤖 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 `@crates/fgumi-sort/src/external.rs` around lines 3932 - 3948, Update the
file-opening flow around the async_reader branch to call
fgumi_bam_io::os_hints::advise_sequential(&file) unconditionally after opening
the input BAM and before selecting PrefetchReader or BufReader. Keep the
async-specific debug logging and reader construction unchanged.

crates/fgumi-bam-io/src/reader.rs (1)

1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy lift

Extract shared open+normalize+replay helpers into fgumi-bam-io instead of re-implementing them per crate. The stdin/file-open-with-optional-prefetch pattern and the tee+BgzfReader+header-parse+replay pattern are each implemented independently at three sites; the advise_sequential placement drift already flagged in external.rs is a direct symptom of this duplication.

• crates/fgumi-bam-io/src/reader.rs#L369-434: promote this implementation (open + normalize_to_bgzf + read_header_and_replay) to a pub fn pair in fgumi-bam-io that both open_bgzf_reader and fgumi-sort's pool-integrated reader can call.
• crates/fgumi-bam-io/src/reader.rs#L160-198: switch open_bgzf_reader's stdin/file-opening block to reuse the same exported helper instead of its own copy.
• crates/fgumi-sort/src/external.rs#L3932-3966: replace the inline opened-building and tee/header-replay logic in create_raw_bam_reader_pool_integrated with the shared fgumi-bam-io helpers, eliminating this drift-prone duplicate.

🤖 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 `@crates/fgumi-bam-io/src/reader.rs` at line 1, Extract the shared
open/optional-prefetch and normalize-to-BGZF/header-replay logic into two public
helpers in fgumi-bam-io, based on the existing implementation near reader.rs
lines 369-434. Update open_bgzf_reader to use these helpers for stdin/file
input, and replace the duplicated opened-building and tee/header-replay logic in
create_raw_bam_reader_pool_integrated with them, preserving advise_sequential
placement and existing pool integration.

🤖 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 `@crates/fgumi-bam-io/src/reader.rs`:
- Line 1: Extract the shared open/optional-prefetch and
normalize-to-BGZF/header-replay logic into two public helpers in fgumi-bam-io,
based on the existing implementation near reader.rs lines 369-434. Update
open_bgzf_reader to use these helpers for stdin/file input, and replace the
duplicated opened-building and tee/header-replay logic in
create_raw_bam_reader_pool_integrated with them, preserving advise_sequential
placement and existing pool integration.

In `@crates/fgumi-sort/src/external.rs`:
- Around line 3932-3948: Update the file-opening flow around the async_reader
branch to call fgumi_bam_io::os_hints::advise_sequential(&file) unconditionally
after opening the input BAM and before selecting PrefetchReader or BufReader.
Keep the async-specific debug logging and reader construction unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 5ec2d38a-4b79-4bd4-b37a-777a1e8ab390

📥 Commits

Reviewing files that changed from the base of the PR and between 6fcbaa1 and 66465ad.

📒 Files selected for processing (7)

• crates/fgumi-bam-io/src/lib.rs
• crates/fgumi-bam-io/src/reader.rs
• crates/fgumi-bam-io/src/sam_input.rs
• crates/fgumi-sort/src/external.rs
• tests/integration/helpers/bam_generator.rs
• tests/integration/main.rs
• tests/integration/test_sam_input.rs

[nh13]

Edited to stay accurate. This comment originally described the branch as of 64e74d4c; three of its statements were overtaken by later work in the same PR. Those are corrected in place below and flagged with (updated). The full current state is in the PR description.

Addressed both outside-diff findings from the last review, plus a set of defects found while verifying them.

Outside-diff findings

• crates/fgumi-sort/src/external.rs — advise_sequential is now called unconditionally after opening the input, so the synchronous sort path gets the readahead hint the two reader.rs sites already got. The WILLNEED hints PrefetchReader issues remain async-only; conflating the two is what hid this.
• crates/fgumi-bam-io/src/reader.rs — deduped the half that was actually duplicated: read_header_and_replay is now pub in fgumi-bam-io and external.rs calls it instead of re-implementing the tee/header/replay block. I also added open_normalized_input there, which gave the shared opener a real second consumer. (updated) I initially declined to unify the open half, on the grounds that the three sites differ in buffering (2 MiB BufReader vs bare File), log level, and whether stdin honors --async-reader. That was wrong — those are parameters, not divergent behavior. The three near-identical copies are now one open_normalized_with_opts, the single place a normalized stream is minted: the async-stdin handling and the log label are arguments to it, and the buffering stays with the caller that wants it. Sort's runtime behavior is unchanged.

Defects found while verifying

• The PR's headline claim was false. sort --verify and compare read the header through a format-aware reader and then re-opened the path with a bare File::open, so both rejected the SAM files sort itself accepted. zipper dispatched on file extension, contradicting the "detection is by content" contract. All now go through open_normalized_input.
• zipper sniff-then-reopen broke FIFO input. Re-opening only rewinds for a regular file, so -i <(samtools view -h in.bam) lost the first four bytes and failed with invalid flags. Non-regular inputs are now opened once and the peeked bytes replayed. BAM over a FIFO now works too, which it did not before. (updated) zipper no longer sniffs at all: its mapped input goes through the shared normalizing reader, so there is no format branch left to get wrong.
• fgumi sort -i - silently lost its 2 MiB input buffer in this branch; restored.
• Record-level SAM parse errors were reported as header failures, because the lazy transcode pulled records while the consumer was still reading the header. The header is now flushed into its own BGZF block.
• Non-SAM text was accepted as headerless SAM and transcoded into a reference-less BAM. Now rejected.
• ChainedReader dropped its replay buffer on a zero-length read, and held the buffer for the stream's lifetime after exhaustion. Guarded and freed.
• fastq, duplex-metrics and simplex-metrics rejected -i - with File does not exist. (updated) All three now stream. The original fix only made fastq stream and had the two metrics commands declare their extra input scan explicitly; that scan is gone — the consensus-BAM guard now runs against the first qualifying record of the pass they already make, so they read the input exactly once. They still reject consensus BAMs.

Coverage

Added tests/integration/test_input_source_matrix.rs: every command the binary advertises must declare whether it accepts SAM and stdin, checked against fgumi --help, so a new command fails the suite until it states its stance. (updated) the count quoted here originally is superseded: cargo ci-test is now 5990 passed / 27 skipped. --workspace --no-default-features --all-targets and --all-features both check clean.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 3

🤖 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-bam-io/src/sam_input.rs`:
- Around line 207-220: Update SamToBamStream::read to return Ok(0) immediately
when buf.is_empty(), before checking staged data or calling produce(). Preserve
the existing EOF and normal-read behavior for non-empty buffers.

In `@crates/fgumi-sort/src/external.rs`:
- Around line 3941-3946: Update the stdin branch in the pool-integrated reader
to honor the async_reader option, creating the same PrefetchReader used by the
file-input async path when enabled while retaining the buffered stdin reader
when disabled. Ensure fgumi sort --async-reader - uses the prefetch thread
rather than silently falling back to BufReader.

In `@tests/integration/test_input_source_matrix.rs`:
- Around line 463-494: Strengthen the precondition in
declared_stdin_support_accepts_piped_sam by locating the matching CONTRACTS
entry and asserting its stdin support is Required, rather than only checking
that any entry exists. Keep feature-gated commands skipped, and ensure a stale
#[values(...)] entry fails as a contract-list mismatch instead of being reported
as a piped-SAM rejection.

🪄 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: 3f8be432-4c65-40f7-92ae-990a35274fbb

📥 Commits

Reviewing files that changed from the base of the PR and between 66465ad and 64e74d4.

📒 Files selected for processing (22)

• crates/fgumi-bam-io/src/lib.rs
• crates/fgumi-bam-io/src/reader.rs
• crates/fgumi-bam-io/src/sam_input.rs
• crates/fgumi-sort/src/external.rs
• crates/fgumi-sort/src/lib.rs
• crates/fgumi-sort/src/reader.rs
• crates/fgumi-sort/src/verify.rs
• src/lib/commands/compare/engines/mod.rs
• src/lib/commands/compare/engines/molecule_join.rs
• src/lib/commands/compare/engines/sort_verify.rs
• src/lib/commands/compare/metrics.rs
• src/lib/commands/duplex_metrics.rs
• src/lib/commands/fastq.rs
• src/lib/commands/review.rs
• src/lib/commands/simplex_metrics.rs
• src/lib/commands/sort.rs
• src/lib/commands/zipper.rs
• src/lib/validation.rs
• tests/integration/helpers/bam_generator.rs
• tests/integration/main.rs
• tests/integration/test_input_source_matrix.rs
• tests/integration/test_sam_input.rs

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[nh13]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/fgumi-bam-io/src/reader.rs (1)

486-498: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win

read_header_and_replay parses the BGZF header unbuffered — reintroduces the syscall-per-block issue already fixed elsewhere in this file.

open_bgzf_reader (lines 258–299) was specifically reworked to wrap the normalized stream in BufReader::with_capacity(BGZF_INPUT_BUFFER_SIZE, ...) before handing it to the BGZF frame reader, closing out the prior review comment on this exact pattern. read_header_and_replay builds BgzfReader::new(TeeReader::new(reader)) directly on its reader argument with no such wrap, and create_bam_reader_for_pipeline_with_opts (its main caller here) feeds it the raw, unbuffered open_normalized_with_opts output in the default (non-async) case — a plain File/io::stdin(). The BGZF frame reader issues small block-sized reads directly against that source during header parsing, so every BGZF block in the header pays a syscall instead of being served from a buffer.

This isn't confined to this file: fgumi_sort::open_raw_bam_record_reader_with_header (crates/fgumi-sort/src/reader.rs, lines 78-85) has the identical unbuffered open_normalized_input → read_header_and_replay pattern, and it now backs sort --verify and compare's molecule-join/sort-verify engines over stdin/FIFO inputs — exactly the paths this PR is adding streaming support for. RawBamRecordReader::new's internal 256 KiB buffer only kicks in for the replayed remainder, after the header has already been parsed unbuffered.

Simplest fix: wrap reader in a BufReader inside read_header_and_replay itself before constructing the TeeReader/BgzfReader, so every caller gets it for free (a BufReader's pre-buffered bytes survive being handed back out via into_parts()/ChainedReader, so no data is lost on replay).

🐛 Proposed fix

 pub fn read_header_and_replay(
     reader: Box<dyn Read + Send>,
     path: &Path,
 ) -> Result<(Header, Box<dyn Read + Send>)> {
-    let tee_reader = TeeReader::new(reader);
+    let buffered = BufReader::with_capacity(BGZF_INPUT_BUFFER_SIZE, reader);
+    let tee_reader = TeeReader::new(buffered);
     let bgzf_reader = BgzfReader::new(tee_reader);

Also applies to: 515-530

🤖 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 `@crates/fgumi-bam-io/src/reader.rs` around lines 486 - 498, Update
read_header_and_replay to wrap its reader argument in a BufReader with
BGZF_INPUT_BUFFER_SIZE before constructing TeeReader and BgzfReader, ensuring
header parsing is buffered for every caller while preserving all replayed bytes
through the existing into_parts/ChainedReader flow.

🤖 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 `@crates/fgumi-bam-io/src/reader.rs`:
- Around line 486-498: Update read_header_and_replay to wrap its reader argument
in a BufReader with BGZF_INPUT_BUFFER_SIZE before constructing TeeReader and
BgzfReader, ensuring header parsing is buffered for every caller while
preserving all replayed bytes through the existing into_parts/ChainedReader
flow.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 27bf4fd8-d4a1-4e5f-b651-57880c5b6c45

📥 Commits

Reviewing files that changed from the base of the PR and between b38c2bc and 8089b5e.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock, !**/*.lock

📒 Files selected for processing (38)

• Cargo.toml
• crates/fgumi-bam-io/src/format.rs
• crates/fgumi-bam-io/src/lib.rs
• crates/fgumi-bam-io/src/reader.rs
• crates/fgumi-bam-io/src/sam_input.rs
• crates/fgumi-consensus/src/filter.rs
• crates/fgumi-consensus/src/lib.rs
• crates/fgumi-simd-fastq/src/reader.rs
• crates/fgumi-sort/src/external.rs
• crates/fgumi-sort/src/lib.rs
• crates/fgumi-sort/src/reader.rs
• crates/fgumi-sort/src/verify.rs
• src/lib/batched_sam_reader.rs
• src/lib/commands/common.rs
• src/lib/commands/compare/engines/mod.rs
• src/lib/commands/compare/engines/molecule_join.rs
• src/lib/commands/compare/engines/sort_verify.rs
• src/lib/commands/compare/metrics.rs
• src/lib/commands/duplex_metrics.rs
• src/lib/commands/extract.rs
• src/lib/commands/fastq.rs
• src/lib/commands/review.rs
• src/lib/commands/shared_metrics.rs
• src/lib/commands/simplex_metrics.rs
• src/lib/commands/sort.rs
• src/lib/commands/zipper.rs
• src/lib/mod.rs
• src/lib/template.rs
• src/lib/unified_pipeline/bam.rs
• src/lib/validation.rs
• tests/integration/helpers/bam_generator.rs
• tests/integration/helpers/mod.rs
• tests/integration/main.rs
• tests/integration/test_async_reader.rs
• tests/integration/test_compare_bams.rs
• tests/integration/test_input_source_matrix.rs
• tests/integration/test_sam_input.rs
• tests/integration/test_streaming_input.rs

💤 Files with no reviewable changes (3)

• src/lib/mod.rs
• src/lib/batched_sam_reader.rs
• src/lib/commands/compare/engines/mod.rs

[nh13]

Addressed the outside-diff finding from the latest review. It has no thread ID, so it cannot be resolved through the UI.

Outside diff range — crates/fgumi-bam-io/src/reader.rs, read_header_and_replay parses the BGZF header unbuffered. Correct, and confirmed against the current code. Two of the three callers hand it the raw open_normalized_* output: create_bam_reader_for_pipeline_with_opts here, and fgumi_sort::open_raw_bam_record_reader_with_header in crates/fgumi-sort/src/reader.rs. Both are a plain File or io::stdin(), so the frame reader's 18-byte block-header read and the block-body read each hit the source directly. The third caller, external.rs, already wraps its input in a 2 MiB BufReader and was never affected.

I did not apply the suggested fix as written. Leaving the BufReader resident under the returned stream fixes the header parse but puts a second buffer under the consumers that already have one — RawBamRecordReader wraps its input in a 256 KiB BufReader, and the sort worker pool reads via read_raw_blocks, so both pull BGZF block by block rather than in buffer-sized chunks. A resident layer would therefore copy every byte of the input a second time on the sort hot path, which is the cost open_bgzf_reader's doc comment explicitly avoids paying.

Instead the buffer is scoped to the parse: it sits under the TeeReader while the header is read, then its readahead is drained into the replay buffer and the original stream is handed back unwrapped. Every caller gets the syscall fix, and no extra layer survives into the record path. The drain costs one memcpy of at most 256 KiB, which ChainedReader frees as soon as it is delivered.

Two regression tests, both verified to fail without the change:

• test_read_header_and_replay_buffers_the_source — a spy reader records the largest request reaching the source. Before the fix it was 80 bytes; it now sees a full buffer-sized fill.
• test_read_header_and_replay_replays_every_byte — asserts the replayed stream is byte-identical to the input. Without the readahead drain the entire record block and the EOF block are silently dropped, which is the failure mode that would otherwise reach a pipe consumer with no error.

Also updated the doc on BGZF_INPUT_BUFFER_SIZE, which named only open_bgzf_reader as its user and now has a second one.