Repository navigation
refactor: replace all unwrap() calls in src/lib/ with expect() - #226
Merged
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 23 minutes and 49 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (40)
📝 WalkthroughWalkthroughThe PR replaces many 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #226 +/- ##
==========================================
+ Coverage 88.88% 88.97% +0.09%
==========================================
Files 113 113
Lines 53521 54107 +586
==========================================
+ Hits 47574 48144 +570
- Misses 5947 5963 +16 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
src/lib/unified_pipeline/scheduler/ucb.rs (1)
137-137: Use a step-specific failure message here.
"should find expected element"is vague in a test failure. Prefer naming the missing step explicitly (Line 137).Proposed patch
- .expect("should find expected element"); + .expect("PipelineStep::Read should be present in priorities");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/unified_pipeline/scheduler/ucb.rs` at line 137, Replace the generic expect message "should find expected element" with a step-specific message that identifies which step or element was expected; locate the expect call (the .expect("should find expected element") in ucb.rs) and change it to include the step identifier or name (for example using format! to include step.id, step_name, or the variable used in the lookup) so test failures state exactly which step was missing.src/lib/sort/keys.rs (1)
997-997: Tighten expect wording for Option paths.These are
Optionvalues, so"result should be Ok"is imprecise. Use messages reflectingSome(...)expectation.Also applies to: 1033-1033, 1066-1066
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` at line 997, The expect message uses "Ok" but the value is an Option; update the assertion messages to reference Some(...) instead of Ok. Locate the three occurrences where the local variable named result is unwrapped with expect (the occurrences shown in keys.rs around the existing result.expect("result should be Ok") and the similar calls near the other two spots) and change the string to something like "expected Some(result)" or "expected Some(...)" so the panic text correctly reflects an Option being unwrapped.src/lib/fastq.rs (1)
571-571: Use Option-accurate expect messages.
resultis anOption, so"result should be Ok"is misleading in failures. Prefer wording like"iterator should yield a record".Also applies to: 607-607
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/fastq.rs` at line 571, The expect call uses an Option named result but the message says "should be Ok" which is for Results; update the expect message to be Option-accurate (e.g., change result.expect("result should be Ok") to something like result.expect("iterator should yield a record" or "expected Some(record) but got None") for the occurrence assigning fastq_set and the other occurrence around the second mention (line ~607) so the error text correctly reflects an Option None case; locate usages by the variable name result and the assignment to fastq_set in src/lib/fastq.rs to apply the change.
🤖 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/lib/reference.rs`:
- Around line 426-427: The expect messages used on the Position::try_from(...)
calls are misleading (they reference "fetching reference sequence"); update each
Position::try_from(1) and Position::try_from(4) (and the other occurrences of
Position::try_from in this file) to use conversion-specific messages such as
"Position::try_from failed for value X" or "failed to convert integer to
Position" so test failures clearly point to a position conversion error rather
than a fetch operation.
In `@src/lib/simulate/quality.rs`:
- Around line 122-123: The call to Normal::new can panic if self.noise_stddev is
negative/NaN/infinite because the CLI does not enforce the invariant; update
validation so the public API cannot panic: add a check in
PositionQualityModel::new() that validates noise_stddev (finite and >= 0.0) and
change the constructor to return Result<PositionQualityModel, Error> (or a
custom error) on invalid values; alternatively, if you prefer validating
earlier, implement the same finite/non-negative check in
QualityArgs::to_quality_model() or use clap's value_parser to reject bad inputs
at parse time — reference symbols: noise_stddev, PositionQualityModel::new(),
QualityArgs::to_quality_model(), and Normal::new.
In `@src/lib/unified_pipeline/bam.rs`:
- Around line 4473-4476: The expect message on the Option unwrap is misleading:
change the first expect on fns.secondary_serialize_fn.as_ref() to a message that
says the Option must be present (e.g., "secondary serialize function should be
set") so it reflects the Option check, and keep the second expect on the Result
(the serialization call) with "serialize should succeed"; locate the call using
fns.secondary_serialize_fn, the closure invocation with &test_data and &mut buf,
and the subsequent .expect that assigns to count and update the first expect
message accordingly.
In `@src/lib/unified_pipeline/scheduler/balanced_chase_drain.rs`:
- Around line 362-365: The two assertions comparing compress_pos and
serialize_pos use the wrong expect message for serialize_pos; update the
serialize_pos.expect calls (the ones paired with compress_pos.expect(...)) to
use a correct message like "serialize position should be Some" instead of
duplicating the compress message so that compress_pos.expect keeps "compress
position should be Some" and serialize_pos.expect reads "serialize position
should be Some" in both occurrences that reference these variables.
---
Nitpick comments:
In `@src/lib/fastq.rs`:
- Line 571: The expect call uses an Option named result but the message says
"should be Ok" which is for Results; update the expect message to be
Option-accurate (e.g., change result.expect("result should be Ok") to something
like result.expect("iterator should yield a record" or "expected Some(record)
but got None") for the occurrence assigning fastq_set and the other occurrence
around the second mention (line ~607) so the error text correctly reflects an
Option None case; locate usages by the variable name result and the assignment
to fastq_set in src/lib/fastq.rs to apply the change.
In `@src/lib/sort/keys.rs`:
- Line 997: The expect message uses "Ok" but the value is an Option; update the
assertion messages to reference Some(...) instead of Ok. Locate the three
occurrences where the local variable named result is unwrapped with expect (the
occurrences shown in keys.rs around the existing result.expect("result should be
Ok") and the similar calls near the other two spots) and change the string to
something like "expected Some(result)" or "expected Some(...)" so the panic text
correctly reflects an Option being unwrapped.
In `@src/lib/unified_pipeline/scheduler/ucb.rs`:
- Line 137: Replace the generic expect message "should find expected element"
with a step-specific message that identifies which step or element was expected;
locate the expect call (the .expect("should find expected element") in ucb.rs)
and change it to include the step identifier or name (for example using format!
to include step.id, step_name, or the variable used in the lookup) so test
failures state exactly which step was missing.
🪄 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: c1cd4f0e-b756-4645-b605-248001de4565
📒 Files selected for processing (39)
src/lib/bam_io.rssrc/lib/batched_sam_reader.rssrc/lib/fastq.rssrc/lib/grouper.rssrc/lib/header.rssrc/lib/mi_group.rssrc/lib/progress.rssrc/lib/read_info.rssrc/lib/reference.rssrc/lib/reorder_buffer.rssrc/lib/simulate/family_size.rssrc/lib/simulate/insert_size.rssrc/lib/simulate/quality.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/pipeline.rssrc/lib/sort/raw.rssrc/lib/sort/raw_bam_reader.rssrc/lib/sort/read_ahead.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/umi/parallel_assigner.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/unified_pipeline/fastq.rssrc/lib/unified_pipeline/queue.rssrc/lib/unified_pipeline/rebalancer.rssrc/lib/unified_pipeline/scheduler/balanced_chase.rssrc/lib/unified_pipeline/scheduler/balanced_chase_drain.rssrc/lib/unified_pipeline/scheduler/mod.rssrc/lib/unified_pipeline/scheduler/optimized_chase.rssrc/lib/unified_pipeline/scheduler/ucb.rssrc/lib/validation.rssrc/lib/variant_review.rssrc/lib/vendored/bam_codec/encoder/mod.rssrc/lib/vendored/bam_codec/encoder/quality_scores.rssrc/lib/vendored/bam_codec/encoder/reference_sequence_id.rssrc/lib/vendored/bam_codec/encoder/sequence.rssrc/lib/vendored/bgzf_multithreaded.rs
nh13
force-pushed
the
nh/unwrap-audit-lib
branch
from
April 4, 2026 17:51
a011968 to
ac3489c
Compare
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Member
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/lib/sort/raw_bam_reader.rs (1)
490-503:⚠️ Potential issue | 🟡 MinorRange indexing panics before your
expectmessageLines 491 and 502:
header_bytes[0..4]andheader_bytes[offset..offset + 4]panic out-of-bounds beforetry_into().expect(...)runs, losing the custom error context.Use
.get(0..4).expect(...)instead to preserve the message:Suggested fix
- let l_text = u32::from_le_bytes( - header_bytes[0..4].try_into().expect("header_bytes too short for l_text"), - ) as usize; + let l_text = u32::from_le_bytes( + header_bytes + .get(0..4) + .expect("header_bytes too short for l_text") + .try_into() + .expect("l_text slice must be exactly 4 bytes"), + ) as usize; @@ - let n_ref = u32::from_le_bytes( - header_bytes[offset..offset + 4].try_into().expect("header_bytes too short for n_ref"), - ); + let n_ref = u32::from_le_bytes( + header_bytes + .get(offset..offset + 4) + .expect("header_bytes too short for n_ref") + .try_into() + .expect("n_ref slice must be exactly 4 bytes"), + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/raw_bam_reader.rs` around lines 490 - 503, The slices header_bytes[0..4] and header_bytes[offset..offset + 4] can panic on range error before your custom expect runs; update the accesses in the raw_bam_reader code (the l_text and n_ref parsing blocks referencing header_bytes, l_text, parsed_text, n_ref) to use header_bytes.get(0..4).expect("header_bytes too short for l_text") and header_bytes.get(offset..offset+4).expect("header_bytes too short for n_ref") (then call try_into() on the returned slice) so the specified expect messages are used instead of a raw range panic.
🧹 Nitpick comments (2)
src/commands/simulate/common.rs (1)
60-67: Add focused tests for parser edge inputs.This function now owns critical validation (
NaN,inf, negatives). Add dedicated tests to lock behavior.Suggested test diff
@@ #[test] fn test_quality_args_zero_noise() { @@ assert!((model.noise_stddev - 0.0).abs() < f64::EPSILON); } + + #[rstest] + #[case("0.0", 0.0)] + #[case("2.5", 2.5)] + fn test_parse_noise_stddev_accepts_valid(#[case] input: &str, #[case] expected: f64) { + let parsed = parse_noise_stddev(input).expect("valid noise stddev should parse"); + assert!((parsed - expected).abs() < f64::EPSILON); + } + + #[rstest] + #[case("-0.1")] + #[case("NaN")] + #[case("inf")] + #[case("-inf")] + #[case("abc")] + fn test_parse_noise_stddev_rejects_invalid(#[case] input: &str) { + assert!(parse_noise_stddev(input).is_err(), "input should be rejected: {input}"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/commands/simulate/common.rs` around lines 60 - 67, Add unit tests for parse_noise_stddev to lock its validation behavior: create tests that call parse_noise_stddev with "NaN", "inf", "-inf", a negative number like "-1.0", and a valid number like "0.5" (and optionally "0" and a large finite value) asserting that NaN/inf/-inf and negatives return Err with the expected message pattern while valid inputs return Ok with the parsed f64; place tests near other command tests or in a new tests module in the same file and reference the parse_noise_stddev function name so CI will catch regressions.src/lib/sort/keys.rs (1)
997-997: Nitpick: "Ok" vs "Some" terminology.
from_recordreturnsOption, notResult. "result should be Some" or "should exist" would be more precise. Same applies to lines 1033 and 1066.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib/sort/keys.rs` at line 997, The expect() message is misleading because from_record returns Option, not Result; update the assertion message in keys.rs where result.expect("result should be Ok") appears (and the similar messages at the other occurrences referenced around lines 1033 and 1066) to use "result should be Some" or "expected Some value" / "should exist" so the wording accurately reflects Option semantics for the variables produced by from_record.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/lib/sort/raw_bam_reader.rs`:
- Around line 490-503: The slices header_bytes[0..4] and
header_bytes[offset..offset + 4] can panic on range error before your custom
expect runs; update the accesses in the raw_bam_reader code (the l_text and
n_ref parsing blocks referencing header_bytes, l_text, parsed_text, n_ref) to
use header_bytes.get(0..4).expect("header_bytes too short for l_text") and
header_bytes.get(offset..offset+4).expect("header_bytes too short for n_ref")
(then call try_into() on the returned slice) so the specified expect messages
are used instead of a raw range panic.
---
Nitpick comments:
In `@src/commands/simulate/common.rs`:
- Around line 60-67: Add unit tests for parse_noise_stddev to lock its
validation behavior: create tests that call parse_noise_stddev with "NaN",
"inf", "-inf", a negative number like "-1.0", and a valid number like "0.5" (and
optionally "0" and a large finite value) asserting that NaN/inf/-inf and
negatives return Err with the expected message pattern while valid inputs return
Ok with the parsed f64; place tests near other command tests or in a new tests
module in the same file and reference the parse_noise_stddev function name so CI
will catch regressions.
In `@src/lib/sort/keys.rs`:
- Line 997: The expect() message is misleading because from_record returns
Option, not Result; update the assertion message in keys.rs where
result.expect("result should be Ok") appears (and the similar messages at the
other occurrences referenced around lines 1033 and 1066) to use "result should
be Some" or "expected Some value" / "should exist" so the wording accurately
reflects Option semantics for the variables produced by from_record.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 228d9765-5cc2-4962-bdb7-e65b6aaa230f
📒 Files selected for processing (40)
src/commands/simulate/common.rssrc/lib/bam_io.rssrc/lib/batched_sam_reader.rssrc/lib/fastq.rssrc/lib/grouper.rssrc/lib/header.rssrc/lib/mi_group.rssrc/lib/progress.rssrc/lib/read_info.rssrc/lib/reference.rssrc/lib/reorder_buffer.rssrc/lib/simulate/family_size.rssrc/lib/simulate/insert_size.rssrc/lib/simulate/quality.rssrc/lib/sort/keys.rssrc/lib/sort/mod.rssrc/lib/sort/pipeline.rssrc/lib/sort/raw.rssrc/lib/sort/raw_bam_reader.rssrc/lib/sort/read_ahead.rssrc/lib/tag_reversal.rssrc/lib/template.rssrc/lib/umi/parallel_assigner.rssrc/lib/unified_pipeline/bam.rssrc/lib/unified_pipeline/base.rssrc/lib/unified_pipeline/fastq.rssrc/lib/unified_pipeline/queue.rssrc/lib/unified_pipeline/rebalancer.rssrc/lib/unified_pipeline/scheduler/balanced_chase.rssrc/lib/unified_pipeline/scheduler/balanced_chase_drain.rssrc/lib/unified_pipeline/scheduler/mod.rssrc/lib/unified_pipeline/scheduler/optimized_chase.rssrc/lib/unified_pipeline/scheduler/ucb.rssrc/lib/validation.rssrc/lib/variant_review.rssrc/lib/vendored/bam_codec/encoder/mod.rssrc/lib/vendored/bam_codec/encoder/quality_scores.rssrc/lib/vendored/bam_codec/encoder/reference_sequence_id.rssrc/lib/vendored/bam_codec/encoder/sequence.rssrc/lib/vendored/bgzf_multithreaded.rs
✅ Files skipped from review due to trivial changes (28)
- src/lib/progress.rs
- src/lib/unified_pipeline/queue.rs
- src/lib/read_info.rs
- src/lib/reorder_buffer.rs
- src/lib/sort/mod.rs
- src/lib/simulate/insert_size.rs
- src/lib/vendored/bam_codec/encoder/sequence.rs
- src/lib/fastq.rs
- src/lib/sort/read_ahead.rs
- src/lib/sort/pipeline.rs
- src/lib/batched_sam_reader.rs
- src/lib/variant_review.rs
- src/lib/simulate/quality.rs
- src/lib/validation.rs
- src/lib/vendored/bam_codec/encoder/quality_scores.rs
- src/lib/unified_pipeline/scheduler/mod.rs
- src/lib/reference.rs
- src/lib/simulate/family_size.rs
- src/lib/unified_pipeline/scheduler/balanced_chase_drain.rs
- src/lib/vendored/bgzf_multithreaded.rs
- src/lib/grouper.rs
- src/lib/header.rs
- src/lib/vendored/bam_codec/encoder/reference_sequence_id.rs
- src/lib/sort/raw.rs
- src/lib/unified_pipeline/base.rs
- src/lib/template.rs
- src/lib/unified_pipeline/fastq.rs
- src/lib/umi/parallel_assigner.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- src/lib/unified_pipeline/scheduler/optimized_chase.rs
- src/lib/tag_reversal.rs
- src/lib/mi_group.rs
- src/lib/bam_io.rs
- src/lib/unified_pipeline/scheduler/ucb.rs
Audit and replace all 624 unwrap() calls across 40 files in src/lib/. Production code (~133 calls) converted to expect() with invariant messages. Test code (~491 calls) converted to expect() with context-specific failure messages. Only 22 unwrap() calls remain, all in doc comments where they are standard Rust convention. Also adds a parse_noise_stddev value parser for the --quality-noise CLI option to validate finite, non-negative f64 inputs at parse time, and uses .get() for header byte slicing in raw_bam_reader to preserve custom error messages on out-of-bounds access.
nh13
force-pushed
the
nh/unwrap-audit-lib
branch
from
April 4, 2026 21:00
ac3489c to
5d5ef2d
Compare
Merged
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
unwrap()calls across 40 files insrc/lib/expect()with invariant-documenting messagesexpect()with context-specific failure messagesunwrap()calls remain, all in doc comments (standard Rust convention)Test plan
cargo ci-test— all 2,213 tests passcargo ci-fmt— formatting cleancargo ci-lint— no warnings