test(codec): assert the bases the CODEC consensus tests are named for

[nh13]

Closes #769.

Seven tests in the CODEC caller suite asserted less than the behavior they are named for. All seven passed against a consensus whose bases were wrong — #763 is the demonstration: the shared fixture emitted a consensus with 25 of 30 positions no-called and not one of them failed. They were written while create_fr_pair produced that mostly-N consensus, so none of them could look at the bases. #768 fixes the fixture and adds test_fixture_consensus_reproduces_the_reference_bases, which establishes what a family should produce; this makes the seven assert it.

Base branch

Based on #768 (763/nhomer/fix-codec-fixture-read-orientation), not on main. The strengthened assertions are only true once the fixture stores the reverse read in reference orientation, so this cannot stand alone.

What each test now asserts

For each, the question was what the test's name and setup mean it to prove — not "add a base comparison".

Test	What it is meant to prove	What it now asserts
test_make_consensus_r1_deletion	A deletion shifts the bases. R1's 5M2D25M excises reference 6-7.	The consensus is reference 1-5 joined to reference 8-42. Both the shifted and unshifted spans are 40bp, so a length cannot separate them; the sequence can.
test_make_consensus_r2_deletion	Same, for R2's 25M5D5M excising reference 36-40.	Reference 1-35 joined to reference 41-45.
test_make_consensus_soft_clipping	Soft-clipped bases are retained, shifting the aligned reference span inward.	Five placeholder bases, then reference 1-35, then five more. 45 bases in any arrangement satisfied the old length check.
test_make_consensus_both_soft_clipped_same_end	The shared clip is retained, and the pair is fully overlapped.	The clip ahead of reference 1-25, plus a single uniform quality across every position — which is what "no single-strand tail" means, and the half of the name the old !is_empty() ignored entirely.
test_emit_consensus_in_r1_orientation	The consensus comes out in R1's orientation — a relationship, so it takes two families to state.	The same reference span consensed twice, differing only in which mate is on the minus strand; the R1-reverse result must be the reverse complement of the R1-forward one, qualities included.
test_mask_single_stranded_regions	--single-strand-qual masks the single-strand positions and only those.	Per offset: the two tails at the masked quality, the duplex middle above it, and the bases untouched throughout.
test_rejected_reads_tracking_enabled	With tracking on, a rejected family's records reach the --rejects output.	The records are retained byte-for-byte in input order — the same contract test_high_duplex_disagreement_tracks_rejects pins on the other reject path — plus the counters.

Two of these needed more than a stronger assertion.

test_emit_consensus_in_r1_orientation could not state its claim at all with one family. It also asserted flag & REVERSE == 0, which is a tautology: build_output_record_into writes a constant flags::UNMAPPED for every CODEC consensus, so that check holds no matter which strand R1 was on. The orientation lives in the bases. The relationship is structural rather than a coincidence of this reference span — consensus_reads_raw builds one single-strand consensus per mate and folds them in a fixed order, so swapping which mate is R1 leaves the folded consensus untouched and changes only the final if r1_is_negative { reverse_complement_ss(..) }. The flag assertion is kept, reframed as documenting why the bases have to carry it.

Neither disjunction had a genuinely reachable second branch. test_mask_single_stranded_regions' has_low_qual || quals.is_empty() second disjunct is unreachable on a fixture that asserts output.count == 1 two lines earlier, and the first is satisfied by masking every position — so the interesting failure was inside the branch that "passed". test_rejected_reads_tracking_enabled' !rejected_reads().is_empty() || reads_filtered > 0 had both branches true, but only the first is the test's subject: reads_filtered is bumped by reject_records_count, which runs whether or not tracking is enabled, so the disjunction was satisfiable by a caller that never wrote the rejects buffer at all. Both are asserted unconditionally on the correct branch. test_reject_records_tracking gains assert!(rejected_reads().is_empty()) so the two form a real A/B on the tracking flag.

The inaccurate doc claim

test_emit_consensus_in_r1_orientation carried Note: Positions covered by only one strand get N bases (correct duplex behavior). That is not what the caller does: build_duplex_consensus_from_padded's single-strand branches keep the covering strand's base unless that strand's quality is MIN_PHRED, which none of these fixtures reach. The comment is gone, and the new assertions encode the real behavior — every strengthened expectation spans the single-strand tails, not just the duplex middle. A second copy of the same falsehood inside test_mask_single_stranded_regions ("single-stranded bases (where abConsensus has N/qual=2) get masked") is corrected too: maskCodecConsensusQuals rewrites qualities only.

Non-vacuity

Every strengthened assertion was verified by perturbing the behavior it now pins and confirming the test fails. Each perturbation was also run against the pre-change tree to establish what the suite used to catch.

Perturbation	Fails now	Failed before
Fixture deletions stop skipping reference bases (reads become the contiguous span)	r1_deletion, r2_deletion	nothing
Fixture soft clips pull reference bases instead of placeholders	soft_clipping, both_soft_clipped_same_end	nothing
Duplex agreement keeps one strand's quality at the first position instead of summing	both_soft_clipped_same_end	nothing
Drop the if r1_is_negative { reverse_complement_ss(..) } re-orientation	emit_consensus_in_r1_orientation	nothing
Single-strand quality mask applied to every position, not just uncovered ones	mask_single_stranded_regions + 3 unit tests	the 3 unit tests only
Rejects buffer retains only the first record of a rejected group	rejected_reads_tracking_enabled	nothing
Single-strand branches emit N — i.e. the behavior the deleted comment described	7 tests	2

Six of the seven perturbations were entirely undetected before this change. The last row is the doc correction stated as a test: the false claim now costs seven failures instead of two.

Fixture change

create_fr_pair's fill byte for query-only CIGAR operations is now the named PLACEHOLDER_BASE (same value, b'A'), with a compile-time assertion that it differs from the first reference base. Without that, a leading soft clip would be indistinguishable from the reference span it precedes and test_make_consensus_soft_clipping would silently lose its subject. This is the only non-test-body change and it is not output-changing.

Verification

cargo ci-fmt, cargo ci-lint, cargo ci-test (7495 passed), cargo ci-doctest — all green.

Audit: other assertions in this module that cannot fail

Swept while in these tests, as #769 asks. Not changed here — the diff stays on the seven tests the issue names. The first two are worth a follow-up issue; the rest are listed so they are on the record.

Verified by perturbation:

• test_not_emit_consensus_chimeric_pair passes with the chimeric mutation deleted. Its fixture is create_fr_pair("read1", 100, 135, 30, …) — R1 spans 100-129 and R2 spans 135-164, so the two do not overlap at all and the family is rejected as InsufficientOverlap before is_primary_fr_pair_raw's cross-chromosome check is ever consulted. I removed both set_ref_id/set_mate_ref_id calls and the test still passed. It needs an overlapping fixture and an assertion on the reason, the way its sibling test_not_emit_consensus_for_rf_pair already does.
• test_check_overlap_phase_indel_mismatch discards its result: let _result = caller.check_overlap_phase(&r1, &r2, 100, 129);, under a comment saying the outcome is indeterminate. It is not — the fixture yields 29 != 24 and returns false deterministically, which the sibling test_check_overlap_phase_deletion_mismatch already asserts.

Asserted by the test harness rather than by production:

• codec_downsampling_retains_lowest_hashing_pairs' "R2 must keep the same templates as R1" is guaranteed by the #[cfg(test)] downsample_pairs wrapper, which derives one index vector from R1 and maps it over R2. Production caps each strand on its own ranks (codec_caps_each_strand_from_its_own_filtered_set documents exactly that), so the assertion states the opposite of the real contract and still cannot fail.
• test_downsample_pairs_no_limit returns at the wrapper's let ... else before any production code runs.
• test_clip_overlap_failed_counted_and_labeled's negative guard — that the reject did not leak into IndelErrorBetweenStrands — cannot fail, because the test's only interaction with the caller is a direct reject_records_count(2, ClipOverlapFailed); nothing in it can insert the other reason.

Post-condition already held / no state exercised:

• test_clear_rejected_reads clears an already-empty buffer, so a clear_rejected_reads body of {} passes.
• test_codec_statistics_tracking feeds no records and asserts three Default-derived zeroes.

Weaker than the contract the name claims:

• test_mask_end_qualities asserts q <= 5 where mask_consensus_quals_query_based assigns exactly 5, and never asserts the interior is untouched — so masking every base to 0 passes both loops, which is the one thing a test named for masking ends must exclude.
• test_build_clipped_info_clip_from_start_reverse asserts adjusted_pos > 100 where the value is exactly 103; every sibling assertion in the same test is an assert_eq!.
• test_reverse_complement_ss_depths_errors_reversed uses ACGT, whose reverse complement is itself, so the base handling passes for a no-op — and unlike to_source_read, reverse_complement_ss has no non-palindromic sibling test anywhere.
• test_pad_consensus_shorter_target asserts only a length where the claim is "returned unchanged".
• test_build_clipped_info_zero_clip_preserves_all asserts the CIGAR has one op, not which op.
• test_downsample_pairs asserts only that two reads survived, not which two.
• test_codec_caller_creation and test_vanilla_to_single_strand_negative_strand echo their own constructor arguments.
• test_consensus_reads_typed_disagreement_count's assert!(err.is_duplex_disagreement()) is implied by the matches! on the line above it.

Risk: command output changes none; unsafe changes none and CLAUDE.md allowlist changes none; memory bounds, queue capacity, and thread/backpressure policy changes none.

Fix: Strengthen seven CODEC consensus tests to assert exact bases, orientations, masking, qualities, and rejected-read records.

• Verify deletion shifts, soft-clip retention, complete overlap, and R1 reverse-complement orientation.
• Verify exact single-strand masking without changing bases or duplex quality.
• Verify byte-exact rejected-read tracking.
• Use a named placeholder base in the fixture.
• Correct documentation for unconditional single-strand and outer-base quality overrides.
• Formatting, linting, tests, and doctests pass.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 22f7e852-ad2b-4e76-8fbb-06edc48b8365

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

Note

Reviews paused

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

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 8275a860-f6f1-4eec-90ef-5978faa758f9

📥 Commits

Reviewing files that changed from the base of the PR and between 4025d70 and 4884585.

📒 Files selected for processing (2)

• crates/fgumi-consensus/src/codec_caller.rs
• src/lib/commands/codec.rs

Included review availability: 0 reviews are currently available. Based on recent review activity, included reviews refill at 1 per hour.

---

Walkthrough

The PR strengthens CODEC consensus tests with exact sequence, orientation, quality, masking, deletion, soft-clip, and rejection assertions. It also updates CLI documentation to describe unconditional quality overrides.

Changes

CODEC consensus validation

Layer / File(s)	Summary
Consensus fixtures and base assertions crates/fgumi-consensus/src/codec_caller.rs	The fixtures use distinct placeholder bases. Tests now verify exact deletion shifts, reverse-R1 output, retained soft clips, and duplex sequences and qualities.
Quality masking validation crates/fgumi-consensus/src/codec_caller.rs, src/lib/commands/codec.rs	Tests assert reference bases, consensus length, and per-position duplex or single-strand qualities. CLI documentation states that configured qualities are assigned unconditionally.
Reject tracking validation crates/fgumi-consensus/src/codec_caller.rs	Tests verify disabled tracking, byte-exact rejected records, input order, filtered counts, and rejection reasons.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 48845

The change strengthens CODEC consensus assertions and fixture behavior, with the reported formatting, lint, test, and doctest checks passing; no actionable merge-blocking risk remains beyond normal review.

Possibly related issues

• CODEC tests with assertions that cannot fail, including one that never reaches its subject #771 — Addresses the same gap in ineffective CODEC consensus assertions and remaining unreachable or incomplete coverage.

Possibly related PRs

• fulcrumgenomics/fgumi#533 — Implements CODEC quality-masking and overlap behavior covered by these tests.
• fulcrumgenomics/fgumi#566 — Introduces CODEC rejection and duplex-disagreement behavior covered by the added assertions.
• fulcrumgenomics/fgumi#756 — Adds the rejection-tracking behavior validated by the byte-exact record tests.

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	The changes strengthen the seven CODEC tests identified in issue #769 and correct the related single-strand quality documentation.
Out of Scope Changes check	✅ Passed	The changes are limited to CODEC consensus tests, fixtures, and documentation directly related to issue #769.
Title check	✅ Passed	The title uses the required conventional-commit format and accurately describes the strengthened CODEC consensus tests.

---

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

[nh13]

@coderabbitai pause

[coderabbitai]

✅ Action performed

Reviews paused.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.27%. Comparing base (2884061) to head (477dbbe).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #770      +/-   ##
==========================================
- Coverage   94.30%   94.27%   -0.03%     
==========================================
  Files         186      186              
  Lines      112416   112508      +92     
==========================================
+ Hits       106013   106071      +58     
- Misses       6403     6437      +34     

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

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