fix(compare): use semantic integer comparison for BAM tag values

[nh13]

Summary

• raw_tags_equal_order_independent now decodes integer tag values semantically when type bytes differ
• When both tags are integer types (c/C/s/S/i/I), values are decoded to i64 for comparison
• Fast path preserved: byte-identical tags skip decoding entirely
• Non-integer types still use exact byte comparison

Context

After #197 rewrote compare bams to use raw byte comparison, it lost semantic integer normalization. BAMs from different tools (fgumi vs fgbio) may encode the same integer value with different BAM types (e.g., C/u8 vs s/i16 for value 158), causing false tag_diff reports.

Observed on schmitt-abl1: 14,164 records reported as tag_diff with 0 actual value differences — all diffs were cD/cM tags with identical values but different integer encodings.

Test plan

• test_order_independent_same_value_different_int_types_u8_vs_i16 — u8 158 == i16 158
• test_order_independent_same_value_different_int_types_u8_vs_i32 — u8 200 == i32 200
• test_order_independent_same_value_different_int_types_i16_vs_i32 — i16 1000 == i32 1000
• test_order_independent_different_values_different_int_types — different values still detected
• Full test suite passes (cargo test --all --lib --tests)
• cargo ci-fmt and cargo ci-lint pass

Fixes #212

[codecov]

Codecov Report

❌ Patch coverage is 99.02913% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 88.07%. Comparing base (7989ad0) to head (33dfbca).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
src/commands/compare/raw_compare.rs	99.02%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #214      +/-   ##
==========================================
+ Coverage   88.05%   88.07%   +0.02%     
==========================================
  Files         113      113              
  Lines       52762    52863     +101     
==========================================
+ Hits        46460    46560     +100     
- Misses       6302     6303       +1     

☔ View full report in Codecov by Sentry.
📢 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]

📝 Walkthrough

Walkthrough

Added two internal helpers, is_int_type and decode_int_tag, to recognize BAM integer aux types and decode their value bytes to i64. raw_tags_equal_order_independent now uses a byte-identical fast path (including the type byte); if that fails and both tags are integer types, it decodes both values and compares the resulting i64s. Non-integer tags retain previous behavior. Tests include builders (make_s_tag, make_upper_s_tag, make_upper_i_tag) and cover cross-type integer equality, semantic inequalities, integer-vs-non-integer mismatches, reordered tags, negative values, and sign-interpretation cases.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly describes the main change: fixing BAM tag comparison to use semantic integer comparison instead of raw bytes.
Description check	✅ Passed	Description is directly related to the changeset, detailing the semantic integer comparison implementation and test coverage.
Linked Issues check	✅ Passed	Changes fully implement issue #212 objectives: semantic integer comparison for BAM tags, preserving fast-path, and handling all integer types (c/C/s/S/i/I).
Out of Scope Changes check	✅ Passed	All changes are scoped to the raw_tags_equal_order_independent function and related test helpers, directly addressing issue #212 requirements.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 nh/fix-semantic-int-compare

---

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 and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

src/commands/compare/raw_compare.rs (1)

547-619: Consider adding signed integer edge case tests.

Good coverage of the core scenarios. Missing tests for signed values:

1. Negative values across types (e.g., -100 as i8 vs -100 as i16 → should match)
2. Same byte, different sign interpretation (e.g., i8(-1) vs u8(255) → should NOT match)

The second case validates that the b as i8 cast in decode_int_tag works correctly.

🧪 Suggested additional tests

+    #[test]
+    fn test_order_independent_negative_value_across_types() {
+        // -100 as i8 (c) vs -100 as i16 (s) => should match
+        let r1 = base_record(&vec![b'X', b'N', b'c', (-100i8) as u8]);
+        let r2 = base_record(&make_s_tag(*b"XN", -100));
+        assert!(raw_tags_equal_order_independent(&r1, &r2));
+    }
+
+    #[test]
+    fn test_order_independent_same_bytes_different_sign_interpretation() {
+        // i8(-1) = 0xFF vs u8(255) = 0xFF => semantically different, should NOT match
+        let r1 = base_record(&vec![b'X', b'V', b'c', 0xFF]); // -1 as i8
+        let r2 = base_record(&make_c_tag(*b"XV", 255));       // 255 as u8
+        assert!(!raw_tags_equal_order_independent(&r1, &r2));
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/compare/raw_compare.rs` around lines 547 - 619, Add two unit
tests to cover signed-integer edge cases: one that asserts semantic equality for
negative values across signed types (e.g., create records with
make_i8/make_s_tag or equivalent i8 vs i16 -100 and assert
raw_tags_equal_order_independent returns true and raw_compare_structured shows
tag_order_match true but tags_match false), and one that asserts non-equality
for same bytes with different sign interpretation (e.g., i8(-1) vs u8(255) using
make_i_tag/make_c_tag or the existing make_* helpers) and assert
raw_tags_equal_order_independent returns false; reference decode_int_tag in the
test names/comments so it’s clear these verify the b as i8 cast behavior.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/commands/compare/raw_compare.rs`:
- Around line 547-619: Add two unit tests to cover signed-integer edge cases:
one that asserts semantic equality for negative values across signed types
(e.g., create records with make_i8/make_s_tag or equivalent i8 vs i16 -100 and
assert raw_tags_equal_order_independent returns true and raw_compare_structured
shows tag_order_match true but tags_match false), and one that asserts
non-equality for same bytes with different sign interpretation (e.g., i8(-1) vs
u8(255) using make_i_tag/make_c_tag or the existing make_* helpers) and assert
raw_tags_equal_order_independent returns false; reference decode_int_tag in the
test names/comments so it’s clear these verify the b as i8 cast behavior.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 1c5c7891-4503-4b2c-9dd1-d2739e8333e6

📥 Commits

Reviewing files that changed from the base of the PR and between 7989ad0 and 0895c36.

📒 Files selected for processing (1)

• src/commands/compare/raw_compare.rs

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/compare/raw_compare.rs`:
- Around line 204-216: Add coverage for the u32 tag by implementing a helper
like make_I_tag(tag: [u8;2], value: u32) that builds aux bytes with b'I' and
value.to_le_bytes(), then update the integer-type test matrix to include the I/i
pair and convert the integer comparison tests to rstest parameterized cases
(c/C/s/S/i/I) and use proptest for value ranges; change references to existing
helpers (make_s_tag, make_upper_s_tag) to add and use the new make_I_tag helper
in the parameterized tests so decode_int_tag(b'I') is exercised with u32 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: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 51aa50fb-92ee-4f30-b896-048de713e05a

📥 Commits

Reviewing files that changed from the base of the PR and between 0895c36 and 0bab68e.

📒 Files selected for processing (1)

• src/commands/compare/raw_compare.rs

[coderabbitai]

🧹 Nitpick comments (1)

src/commands/compare/raw_compare.rs (1)

644-650: Consider adding a make_lower_c_tag helper for i8 tags.

Test manually constructs [b'X', b'N', b'c', (-100i8) as u8]. A helper like the others would improve consistency:

✨ Suggested helper

+    /// Helper to build aux bytes for a c-type (i8) integer tag.
+    fn make_lower_c_tag(tag: [u8; 2], value: i8) -> Vec<u8> {
+        vec![tag[0], tag[1], b'c', value as u8]
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/compare/raw_compare.rs` around lines 644 - 650, The test
test_order_independent_negative_value_across_types manually builds an i8 tag
byte sequence ([b'X', b'N', b'c', (-100i8) as u8]) which is inconsistent with
other tests that use helpers like make_s_tag; add a make_lower_c_tag function
(mirroring naming/behavior of existing tag helpers) that accepts an i8 and
returns the proper byte vector, then update the test to use make_lower_c_tag
instead of constructing the bytes inline; locate usages around base_record,
make_s_tag, and raw_tags_equal_order_independent to ensure the new helper
integrates consistently with other tests.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/commands/compare/raw_compare.rs`:
- Around line 644-650: The test
test_order_independent_negative_value_across_types manually builds an i8 tag
byte sequence ([b'X', b'N', b'c', (-100i8) as u8]) which is inconsistent with
other tests that use helpers like make_s_tag; add a make_lower_c_tag function
(mirroring naming/behavior of existing tag helpers) that accepts an i8 and
returns the proper byte vector, then update the test to use make_lower_c_tag
instead of constructing the bytes inline; locate usages around base_record,
make_s_tag, and raw_tags_equal_order_independent to ensure the new helper
integrates consistently with other tests.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: b37dee06-8ce9-4226-9c0e-e55ac8a12787

📥 Commits

Reviewing files that changed from the base of the PR and between 0bab68e and 33dfbca.

📒 Files selected for processing (1)

• src/commands/compare/raw_compare.rs