[tfenne]

VALIDATE_CRC_CHECKSUMS never checked the BGZF blocks that hold a BAM's header; it now does.

• SamReaderFactory.open made the BAMFileReader, which reads the header as it is made, and only then applied the enabled options, so CRC checking reached the stream after the header blocks had been inflated. A header block with a wrong CRC, and any records sharing it, went unchecked.
• The option is now passed to BAMFileReader's (package-private) constructors that take an InflaterFactory, which turn checking on before reading the header; the other constructors pass false.
• Tests corrupt the CRC of the first block of a small in-memory BAM and open it as a stream and as a file.
• Left alone: HtsgetBAMFileReader has the same ordering, and the option is ignored for bgzipped SAM (SAMTextReader.enableCrcChecking does nothing).

Found while reading broadinstitute/picard#2063.

Fixes #1949

Summary by CodeRabbit

• Bug Fixes
  • Enabling CRC validation now checks BAM header blocks as well as the remaining file blocks, detecting corrupted header checksums when reading from streams or files.

[coderabbitai]

Walkthrough

BAM reader construction now receives the CRC validation setting before reading the header. Factory input paths pass the setting to the reader. Tests cover corrupted header CRCs for stream and file inputs.

Changes: BAM CRC validation

Layer / File(s)	Summary
Configure CRC validation before header reads src/main/java/htsjdk/samtools/BAMFileReader.java	BAMFileReader constructors accept a CRC setting and apply it to the compressed input stream before reading the header. Convenience constructors keep CRC checking disabled by default.
Pass the setting through factory paths and test corrupted headers src/main/java/htsjdk/samtools/SamReaderFactory.java, src/test/java/htsjdk/samtools/SamReaderFactoryTest.java, CHANGELOG.md	SamReaderFactory passes the enabled setting to BAMFileReader across its input paths. Tests verify that corrupted header CRCs cause SAMFormatException for stream and file inputs. The changelog records the change.

Priority: Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix · Severity of issue fixed: Low

Possibly related PRs

• samtools/htsjdk#1943: Changes asynchronous BGZF reading to propagate CRC and decompression failures to the reading thread, connecting to the asynchronous CRC validation path.

Merge Risk: Minimal · up to 64ba7

CRC validation is configured before header reads; no merge-blocking risk was identified.

Pre-merge checks: 4 · 1

Failed checks: 1 warning

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documenta…	Write docstrings for the functions missing them to satisfy the coverage threshold.

Passed checks: 4 passed

Check name	Status	Explanation
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title states the main change: when VALIDATE_CRC_CHECKSUMS is enabled, the CRCs of BAM header blocks are now checked. The raw summary and PR objectives confirm this is the core change. The option i…
Linked Issues check		Issue #1949 requires that VALIDATE_CRC_CHECKSUMS check the BGZF blocks that hold a BAM header, including records in the last header block, before those blocks are inflated. The diff at head 64ba77b s…
Out of Scope Changes check		Every changed area supports #1949. BAMFileReader.java adds a checkCrcs parameter to the package-private constructors and a helper, openBlockCompressed, which the constructors share. This implements th…

Full details: Title check

Explanation

The title states the main change: when VALIDATE_CRC_CHECKSUMS is enabled, the CRCs of BAM header blocks are now checked. The raw summary and PR objectives confirm this is the core change. The option is passed to the BAMFileReader constructors so header blocks are checked before they are read. The title is specific, concise, and free of noise.

Full details: Linked Issues check

Explanation

Issue #1949 requires that VALIDATE_CRC_CHECKSUMS check the BGZF blocks that hold a BAM header, including records in the last header block, before those blocks are inflated. The diff at head 64ba77b shows the fix on the requested path. SamReaderFactoryImpl.open computes checkCrcs from enabledOptions before it builds the reader and passes it into every BAMFileReader construction path (seekable stream, non-seekable stream, and file path). BAMFileReader's package-private constructors call setCheckCrcs(checkCrcs) on the BlockCompressedInputStream before the header is read. The setting is applied through the new openBlockCompressed helper, which uses the synchronous or asynchronous stream type as before. The public constructors pass false, so default behavior is unchanged. The issue's stated fix matches this design. Residual uncertainty: the reviewed evidence did not include the internals of AsyncBlockCompressedInputStream. I did not confirm whether it starts reading or inflating blocks inside its constructor, before setCheckCrcs runs. The added tests cover the default path only. The main synchronous path is verified by the code diff. The issue's explicit exclusions (HtsgetBAMFileReader ordering and bgzipped SAM handling) are left unchanged, as the issue requests.

Full details: Out of Scope Changes check

Explanation

Every changed area supports #1949. BAMFileReader.java adds a checkCrcs parameter to the package-private constructors and a helper, openBlockCompressed, which the constructors share. This implements the requested fix. SamReaderFactory.java computes the option once and passes it to each BAMFileReader construction path, which is the required wiring. SamReaderFactoryTest.java adds tests for the corrupted header-block CRC on a byte-array stream and on a temporary file. These tests verify the fix. CHANGELOG.md adds a patch-release note for the behavior change. This is documentation of the same change. The PR does not change HtsgetBAMFileReader or SAMTextReader, which the issue and PR description explicitly leave alone. No unrelated functional change appears in the reviewed summary or diffs.

Full details: Docstring Coverage

Explanation

Docstring coverage is 75.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff with cyclomatic complexity of at least 2. Documentation-only edits and deletions within surviving functions are included; unchanged diff context and fully deleted functions are excluded. Analyzed 4 functions across 3 files. (1 skipped: 1 unsupported.)

Resolution

Write docstrings for the functions missing them to satisfy the coverage threshold.

Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

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