fix(raw-bam): use minimal unsigned BAM integer types for non-negative tag values

[nh13]

Summary

• append_int_tag now uses unsigned types (C/S) for non-negative values that don't fit the smaller signed type, matching htsjdk and samtools behavior
• Encoding order: i8 → u8 → u16 → i16 → i32
• Saves 1 byte per tag for values 128-255 (common in high-depth consensus cD/cM tags)

Context

Observed on the schmitt-abl1 vendor sample in fgumi-benchmarks: 13,943 cD and 13,172 cM tag values in the range 128-255 were encoded as s (i16, 2 bytes) instead of C (u8, 1 byte).

Test plan

• test_append_int_tag_i8 — value 42 → type c
• test_append_int_tag_negative_i8 — value -5 → type c
• test_append_int_tag_u8 — value 200 → type C (was s)
• test_append_int_tag_negative_i16 — value -200 → type s
• test_append_int_tag_u16 — value 1000 → type S (was s)
• test_append_int_tag_i32 — value 100000 → type i
• Full test suite passes (cargo test --all --lib --tests)
• cargo ci-fmt and cargo ci-lint pass

Fixes #211

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.05%. Comparing base (7989ad0) to head (8fef1ca).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #213   +/-   ##
=======================================
  Coverage   88.05%   88.05%           
=======================================
  Files         113      113           
  Lines       52762    52762           
=======================================
  Hits        46460    46460           
  Misses       6302     6302           

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

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: c11488f0-d8dc-44c6-aa18-e319e6e0dec0

📥 Commits

Reviewing files that changed from the base of the PR and between 6fe898b and 8fef1ca.

📒 Files selected for processing (2)

• crates/fgumi-raw-bam/src/builder.rs
• crates/fgumi-raw-bam/src/tags.rs

✅ Files skipped from review due to trivial changes (1)

• crates/fgumi-raw-bam/src/builder.rs

---

📝 Walkthrough

Walkthrough

The PR changes integer-tag encoding to pick the smallest BAM integer type that fits an i32 value, preferring unsigned encodings for non‑negative values: c (i8) for -128..127, C (u8) for 128..255, s (i16) for -32768..-129, S (u16) for 256..65535, and i (i32) otherwise. The UnmappedBamRecordBuilder::append_int_tag doc comment was updated and unit tests were expanded to cover unsigned encodings and boundary transitions. No public APIs or runtime behavior beyond tag encoding choices were changed.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title accurately describes the main change: adopting minimal unsigned BAM integer types for non-negative values, matching the core modification in tags.rs.
Description check	✅ Passed	Description covers the key changes, rationale (space savings on 128-255 range), context (schmitt-abl1 sample), and test coverage—directly related to the changeset.
Linked Issues check	✅ Passed	Code changes fully implement issue #211 requirements: append_int_tag now selects unsigned types for non-negative values, with the exact encoding order (i8→u8→u16→i16→i32) and test coverage specified.
Out of Scope Changes check	✅ Passed	All changes are in-scope: builder.rs doc update and tags.rs logic/tests directly address issue #211 unsigned type selection; no unrelated modifications present.
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-unsigned-int-tags

---

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 (2)

crates/fgumi-raw-bam/src/tags.rs (2)

490-491: Stale comment. References "smallest signed type" but append_int_tag now prefers unsigned types.

♻️ Suggested fix

-/// (smallest signed type that fits).
+/// (smallest type that fits).

🤖 Prompt for AI Agents

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

In `@crates/fgumi-raw-bam/src/tags.rs` around lines 490 - 491, The comment above
the tag-rewrite logic is outdated: it mentions "smallest signed type" but
append_int_tag now prefers unsigned types; update the comment near the tag
removal/re-append logic in tags.rs to say the tag is removed and re-appended
using append_int_tag which chooses the smallest unsigned integer type that fits
(or otherwise document the current selection rules used by append_int_tag), and
ensure any references to "signed" are changed to "unsigned" and reflect the
actual behavior of the append_int_tag function.

---

1584-1604: Tests look good. Cover the key encoding transitions.

Consider adding boundary tests (127/128, 255/256, -128/-129) via rstest parameterization for completeness:

♻️ Optional: rstest parameterization for boundary coverage

#[rstest]
#[case::max_i8(127, b'c')]
#[case::min_u8(128, b'C')]
#[case::max_u8(255, b'C')]
#[case::min_u16(256, b'S')]
#[case::max_u16(65535, b'S')]
#[case::min_i32(65536, b'i')]
#[case::max_neg_i8(-128, b'c')]
#[case::min_neg_i16(-129, b's')]
#[case::min_i16(-32768, b's')]
#[case::neg_i32(-32769, b'i')]
fn test_append_int_tag_boundaries(#[case] value: i32, #[case] expected_type: u8) {
    let mut rec = Vec::new();
    append_int_tag(&mut rec, b"XX", value);
    assert_eq!(rec[2], expected_type);
}

🤖 Prompt for AI Agents

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

In `@crates/fgumi-raw-bam/src/tags.rs` around lines 1584 - 1604, Add parameterized
boundary tests for append_int_tag to cover transitions at 127/128, 255/256,
-128/-129, etc.; create an rstest like test_append_int_tag_boundaries that calls
append_int_tag(&mut rec, b"XX", value) for each case and asserts rec[2] ==
expected_type so you validate only the encoded type byte; include cases for
max_i8(127,b'c'), min_u8(128,b'C'), max_u8(255,b'C'), min_u16(256,b'S'),
max_u16(65535,b'S'), min_i32(65536,b'i'), max_neg_i8(-128,b'c'),
min_neg_i16(-129,b's'), min_i16(-32768,b's') and neg_i32(-32769,b'i') to ensure
all boundaries are covered.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 490-491: The comment above the tag-rewrite logic is outdated: it
mentions "smallest signed type" but append_int_tag now prefers unsigned types;
update the comment near the tag removal/re-append logic in tags.rs to say the
tag is removed and re-appended using append_int_tag which chooses the smallest
unsigned integer type that fits (or otherwise document the current selection
rules used by append_int_tag), and ensure any references to "signed" are changed
to "unsigned" and reflect the actual behavior of the append_int_tag function.
- Around line 1584-1604: Add parameterized boundary tests for append_int_tag to
cover transitions at 127/128, 255/256, -128/-129, etc.; create an rstest like
test_append_int_tag_boundaries that calls append_int_tag(&mut rec, b"XX", value)
for each case and asserts rec[2] == expected_type so you validate only the
encoded type byte; include cases for max_i8(127,b'c'), min_u8(128,b'C'),
max_u8(255,b'C'), min_u16(256,b'S'), max_u16(65535,b'S'), min_i32(65536,b'i'),
max_neg_i8(-128,b'c'), min_neg_i16(-129,b's'), min_i16(-32768,b's') and
neg_i32(-32769,b'i') to ensure all boundaries are covered.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 528ccf7e-da64-440b-b2a7-cdf4d5a9c21b

📥 Commits

Reviewing files that changed from the base of the PR and between 7989ad0 and 2893f5c.

📒 Files selected for processing (2)

• crates/fgumi-raw-bam/src/builder.rs
• crates/fgumi-raw-bam/src/tags.rs

[nh13]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

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]

🧹 Nitpick comments (1)

crates/fgumi-raw-bam/src/tags.rs (1)

1614-1630: Nice boundary matrix; consider adding extreme i32 sentinels.

Line 1614–1630 covers transition points well. Optional hardening: add i32::MIN and i32::MAX cases to explicitly pin fallback behavior.

🤖 Prompt for AI Agents

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

In `@crates/fgumi-raw-bam/src/tags.rs` around lines 1614 - 1630, Add explicit
sentinel cases to the test_append_int_tag_boundaries rstest to cover full i32
extremes: add #[case::min_i32(i32::MIN, b'i')] and #[case::max_i32(i32::MAX,
b'i')] so append_int_tag is exercised for the i32::MIN and i32::MAX paths;
locate and update the test function test_append_int_tag_boundaries (which calls
append_int_tag) to include these two new cases.

🤖 Prompt for all review comments with AI agents

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

Nitpick comments:
In `@crates/fgumi-raw-bam/src/tags.rs`:
- Around line 1614-1630: Add explicit sentinel cases to the
test_append_int_tag_boundaries rstest to cover full i32 extremes: add
#[case::min_i32(i32::MIN, b'i')] and #[case::max_i32(i32::MAX, b'i')] so
append_int_tag is exercised for the i32::MIN and i32::MAX paths; locate and
update the test function test_append_int_tag_boundaries (which calls
append_int_tag) to include these two new cases.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d243d4a3-ccff-403e-8e0f-912f604f049c

📥 Commits

Reviewing files that changed from the base of the PR and between 2893f5c and 6fe898b.

📒 Files selected for processing (2)

• crates/fgumi-raw-bam/src/builder.rs
• crates/fgumi-raw-bam/src/tags.rs

✅ Files skipped from review due to trivial changes (1)

• crates/fgumi-raw-bam/src/builder.rs