Explain why whole-contig CRAM reference lookup failed

[tfenne]

Closes #998.

The whole-contig reference lookup reported:

A reference must be supplied (reference sequence SAMSequenceRecord(name=1,
length=200,dict_index=0,assembly=null,alternate_names=[]) not found).

which is misleading in the common case — a reference was supplied, it just had not been indexed — and it dumped the entire SAMSequenceRecord at the user.

This is the sibling of the by-region lookup fixed in #1790 (for #1732). Both fail for the same reason, so I factored the diagnostic into one private helper used by both, rather than letting two copies of the same message drift apart. It names the sequence and points at the companion .fai/.gzi/.dict files a CRAM needs.

Message text only — no change to which inputs are accepted.

Tests cover the whole-contig path, assert the two paths report an identical diagnostic, and assert the message no longer contains a raw SAMSequenceRecord( dump.

Test methods follow the house testXxx camelCase convention.

Summary by CodeRabbit

• Bug Fixes

  • Standardized error messages when reference bases cannot be retrieved through whole-contig or regional lookups.
  • Diagnostics now consistently identify the affected sequence and provide guidance about missing FASTA indexes or sequence dictionaries.
  • Error messages no longer include unnecessary raw sequence-record details.

• Tests

  • Added coverage confirming consistent diagnostics across both lookup methods.

[coderabbitai]

📝 Walkthrough

Walkthrough

CRAMReferenceRegion now uses one standardized diagnostic when reference bases are unavailable. Tests cover whole-contig and by-region failures, required .fai and .dict guidance, matching messages, and exclusion of raw sequence-record output.

Changes

CRAM reference error diagnostics

Layer / File(s)	Summary
Shared error diagnostic src/main/java/htsjdk/samtools/cram/build/CRAMReferenceRegion.java	Whole-contig and by-region null-reference failures now use the shared noReferenceBasesMessage helper.
Diagnostic regression coverage src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java	Tests verify sequence naming, .fai and .dict guidance, identical diagnostics across lookup paths, and omission of raw SAMSequenceRecord output.

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

Possibly related PRs

• samtools/htsjdk#1790: Standardizes the same CRAMReferenceRegion null-reference error paths and their diagnostic tests.

🚥 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 is concise and clearly describes the main diagnostic change for whole-contig CRAM reference lookup failures.
Linked Issues check	✅ Passed	The PR updates the CRAM reference error to explain missing indexing and required companion files, matching the issue's main read-path complaint.
Out of Scope Changes check	✅ Passed	The changes stay focused on CRAM reference diagnostics and tests, with no unrelated scope added.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch tf_fix_cram_998

---

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.}

[coderabbitai]

🧹 Nitpick comments (1)

src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java (1)

91-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Assert the .gzi guidance too.

The production message includes a .gzi requirement for bgzipped FASTA files, and the PR objective explicitly includes it, but this test only checks .fai and .dict. Add a .gzi assertion to prevent that guidance from regressing.

Proposed test addition

         Assert.assertTrue(
                 message.contains(".dict"),
                 "the error should mention the required sequence dictionary, but was: " + message);
+        Assert.assertTrue(
+                message.contains(".gzi"),
+                "the error should mention the bgzipped FASTA index, but was: " + message);

🤖 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/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java`
around lines 91 - 99, Update testWholeContigFetchExplainsLikelyCause to also
assert that the failed whole-contig fetch message contains ".gzi", alongside the
existing sequence, ".fai", and ".dict" guidance checks.

🤖 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.

Nitpick comments:
In
`@src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java`:
- Around line 91-99: Update testWholeContigFetchExplainsLikelyCause to also
assert that the failed whole-contig fetch message contains ".gzi", alongside the
existing sequence, ".fai", and ".dict" guidance checks.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 7c6333bb-7c66-4589-afd5-60a1935fc012

📥 Commits

Reviewing files that changed from the base of the PR and between a431e97 and d41c2ec.

📒 Files selected for processing (2)

• src/main/java/htsjdk/samtools/cram/build/CRAMReferenceRegion.java
• src/test/java/htsjdk/samtools/cram/build/CRAMReferenceRegionErrorMessageTest.java