Skip to content

fix(sam): use safe conversions for CIGAR ref span and revcomp string - #158

Merged
nh13 merged 1 commit into
mainfrom
147/nh/fix-sam-safety
Mar 4, 2026
Merged

nh13 merged 1 commit into
mainfrom
147/nh/fix-sam-safety

Conversation

@nh13

@nh13 nh13 commented Mar 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Use usize::try_from() instead of as usize cast for CIGAR-derived reference span in regenerate_alignment_tags_raw to avoid silent wrapping on negative values
  • Replace String::from_utf8().expect() with from_utf8_lossy() in revcomp_buf_value to match reverse_buf_value pattern and prevent potential panics
  • Remove stale # Panics doc section from revcomp_buf_value

Follow-up to #147 addressing CodeRabbitAI review feedback.

Test plan

  • All 1852 tests pass (cargo ci-test)
  • Formatting clean (cargo ci-fmt)
  • Linting clean (cargo ci-lint)

Use usize::try_from() instead of bare `as usize` cast for the
CIGAR-derived reference span to avoid silent wrapping on negative values.
Replace String::from_utf8().expect() with from_utf8_lossy() in
revcomp_buf_value to match reverse_buf_value pattern and prevent
potential panics on non-ASCII input.
@nh13
nh13 temporarily deployed to github-actions March 3, 2026 19:16 — with GitHub Actions Inactive
@coderabbitai

coderabbitai Bot commented Mar 3, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between bf4b24f and 455205d.

📒 Files selected for processing (2)
  • crates/fgumi-sam/src/alignment_tags.rs
  • crates/fgumi-sam/src/lib.rs

📝 Walkthrough

Walkthrough

Two functions received defensive improvements. regenerate_alignment_tags_raw now uses usize::try_from() with explicit error handling instead of casting when computing reference span from CIGAR data, rejecting negative values. revcomp_buf_value switched from from_utf8 with expect() to from_utf8_lossy(), removing a panic path and returning valid UTF-8 regardless. Documentation was updated to reflect these changes. No public APIs were modified.

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and accurately reflects the main changes: replacing unsafe casts with safe conversions for CIGAR reference span and revcomp string operations.
Description check ✅ Passed The description clearly relates to the changeset, detailing the three specific improvements made and referencing the follow-up context.
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 (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch 147/nh/fix-sam-safety

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


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

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Mar 3, 2026 •

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.81%. Comparing base (bf4b24f) to head (455205d).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
- Coverage   83.81%   83.81%   -0.01%     
==========================================
  Files         126      126              
  Lines       51211    51211              
==========================================
- Hits        42924    42923       -1     
- Misses       8287     8288       +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.

@nh13
nh13 merged commit bebaea7 into main Mar 4, 2026
7 checks passed
@nh13
nh13 deleted the 147/nh/fix-sam-safety branch March 4, 2026 06:23
@nh13 nh13 mentioned this pull request Mar 4, 2026

This branch was previously deployed

1 inactive deployment
github-actions — 455205de Deployed Mar 3, 2026 by nh13 via coverage #550
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant