feat(collect): ISA reseed-LEFT/RC warm-start (PRMI_ISA), byte-identical - #42
Conversation
|
Caution Review failedPull request was closed or merged during review Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough
ChangesRC Backward Span Warm-Start
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
3413e30 to
ee2e936
Compare
c18ee47 to
f9bde10
Compare
ee2e936 to
3e7ab3e
Compare
f9bde10 to
a18bec3
Compare
323ed4e to
d9185db
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
prmi/src/index/spectrum.rs (1)
2008-2066:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd a packed-PAC fail-closed guard in
span_rc_walk.
mem_search_backward_span_rc_warmstartis public and routes here; with an undersizedPacEncoding::Packed, this path can reach packed decode.unwrap()and panic. Guard once at entry and return the anchor floor on invalid packed input.Suggested fix
fn span_rc_walk( &self, occ_count: u64, anchor_len: u64, read: &[u8], pivot: usize, min_intv: u64, seed_win: Option<(u64, u64)>, pac: &[u8], enc: PacEncoding, ) -> u64 { if occ_count == 0 { return 0; } + if let PacEncoding::Packed { num_bases } = enc { + if validate_packed_pac(pac, num_bases, "mem_search_backward_span_rc_warmstart").is_err() { + return anchor_len; + } + } let sa_num = self.sa_num(); let l_pac = self.l_pac();As per coding guidelines, “flag … any public primitive that slices a caller-supplied buffer … without failing closed on malformed input.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prmi/src/index/spectrum.rs` around lines 2008 - 2066, The `span_rc_walk` function accepts a public-facing call path through `mem_search_backward_span_rc_warmstart` but lacks validation for the `PacEncoding::Packed` parameter, which can cause a panic during packed decode operations if the encoding is undersized. Add a guard at the entry of the `span_rc_walk` function to check if the `enc` parameter is `PacEncoding::Packed` with invalid dimensions, and return the `anchor_len` value immediately if validation fails, matching the fail-closed pattern already used for the anchor_end bounds check in this function.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/src/index/collect.rs`:
- Line 533: The reseed_rc_hint function is missing the #[inline] attribute for
consistency with the structurally identical reseed_isa_hint function which
already has this attribute. Both functions are called from the same hot reseed
loop and should have matching inline hints for performance. Add the #[inline]
attribute directly before the fn keyword of the reseed_rc_hint function
definition to match the pattern used in reseed_isa_hint.
In `@prmi/src/index/spectrum.rs`:
- Around line 4952-4966: The property test
mem_search_backward_warmstart_equals_cold currently modifies hint_raw using
modulo before passing it to mem_search_backward_span_rc_warmstart, which
prevents testing the documented contract that the function handles ANY hint
value including None and out-of-range values. Add additional test cases to
exercise both the None hint case and the unmodified Some(hint_raw) case with raw
out-of-range values, in addition to the existing modulo-clamped case, to ensure
the warmstart implementation correctly handles all valid hint scenarios.
---
Outside diff comments:
In `@prmi/src/index/spectrum.rs`:
- Around line 2008-2066: The `span_rc_walk` function accepts a public-facing
call path through `mem_search_backward_span_rc_warmstart` but lacks validation
for the `PacEncoding::Packed` parameter, which can cause a panic during packed
decode operations if the encoding is undersized. Add a guard at the entry of the
`span_rc_walk` function to check if the `enc` parameter is `PacEncoding::Packed`
with invalid dimensions, and return the `anchor_len` value immediately if
validation fails, matching the fail-closed pattern already used for the
anchor_end bounds check in this function.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2d4b3de-fc20-4fc5-bce6-3f1e6ca123b2
📒 Files selected for processing (2)
prmi/src/index/collect.rsprmi/src/index/spectrum.rs
d9185db to
df00adb
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/src/index/collect.rs`:
- Around line 544-547: The unchecked multiplication of `2 * self.l_pac()` can
overflow for large values before the subsequent checked_sub operations,
violating the checked arithmetic requirement for usize operations in the search
core. Replace the unchecked multiplication with a checked operation by calling
`self.l_pac().checked_mul(2)?` instead, which will properly propagate an error
if the multiplication would overflow, ensuring safe arithmetic throughout the rc
calculation chain.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97102027-5405-402d-b054-f10b777a7fa3
📒 Files selected for processing (2)
prmi/src/index/collect.rsprmi/src/index/spectrum.rs
df00adb to
216bc0f
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/src/index/collect.rs`:
- Around line 1703-1714: The test fixture in the build_mode2_with_isa test uses
a 4-periodic reference pattern that causes long exact blocks to repeat
frequently. This makes the filter at Lines 259-280 drop seed matches before
reaching the RC left-warmstart reseed path, so the test fails to exercise the
new code path. Replace the 4-periodic reference generation with a low-occurrence
pattern that has sufficient entropy to avoid excessive repetition, ensuring that
exact blocks are sufficiently distinct so the test reliably reaches the reseed
warmstart logic. Alternatively, add an assertion after running the reseed step
to verify that a reseed candidate was actually encountered and processed during
the test.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2d42b7b3-97fa-47a0-a4be-85c93921f342
📒 Files selected for processing (2)
prmi/src/index/collect.rsprmi/src/index/spectrum.rs
216bc0f to
a489141
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
a489141 to
ea63904
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi/src/index/spectrum.rs`:
- Around line 2008-2021: In the span_rc_walk method, change the early return
when occ_count == 0 from returning 0 to returning anchor_len instead, as the
API's documented floor is anchor_len and the _interval_rc path already assumes
every floor case preserves the anchor span. Additionally, add a direct zero-occ
span test to verify this behavior, ensuring the method fails closed on malformed
caller input as required for this byte-identity-critical public primitive.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5d986d62-92b7-4451-b6c8-b4ae0eae75bd
📒 Files selected for processing (2)
prmi/src/index/collect.rsprmi/src/index/spectrum.rs
Extends the forward ISA reseed to the LEFT (reverse-complement) extension. The reseed-left RC walk warm-starts from the RC-strand projection of the parent SMEM's cached locus instead of a cold model launch. Stacks on the forward-reseed change: reuses the same ReseedHint and PRMI_ISA gate, adding the RC projection + RC warm-start lever. spectrum.rs: - Factor the RC walk out of mem_search_backward_truncated_span_rc into a private span_rc_walk(seed_win) (None == the cold walk, verbatim); the public fn delegates with None. - mem_search_backward_span_rc_warmstart(seed_hint): the warm-started twin, delegating with Some. Seeds the internal forward_maximal_len insertion search. - mem_search_backward_warmstart_equals_cold proptest: RC warm == cold for ANY hint (the left twin of mem_search_warmstart_equals_cold). collect.rs: - reseed_rc_hint: RC-strand projection 2*l_pac - refpos - off - 1 (doubled-text complement-mirror; base 2*l_pac, NOT sa_num() == 2*l_pac+1 — the +1 sentinel is the off-by-one). - Thread the hint into zz_left_span_reseed; warm-start the RC walk from the projection (None/garbage is byte-id-safe, just costs probes). - collect_smems_isa_left_warmstart_equals_cold_partial: ISA-on == ISA-off on a partial read that reseeds with left context (RC path exercised). - reseed_rc_hint_reduces_probes (spectrum-probe-count): the load-bearing projection-correctness gate — the CORRECT RC projection must touch strictly fewer probes than cold; a wrong formula (e.g. sa_num() base) seeds far from the interval and fails this, which neither byte-id gate can catch.
ea63904 to
0073126
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
What
Extends the forward ISA reseed (#40) to the LEFT (reverse-complement) extension. The reseed-left RC walk warm-starts from the RC-strand projection of the parent SMEM's cached locus instead of a cold model launch, behind the same
PRMI_ISAgate.Stacked on #40 — base is
feat/v0.3-isa-reseed-warmstart, notmain. It reuses theReseedHintandPRMI_ISAmachinery that #40 introduces; review/merge #40 first. (The diff shown here is only the RC delta.)Why it's byte-identical
Same contract as the forward path: the RC hint only seeds the internal
forward_maximal_leninsertion search, which expands on a miss to the true boundary, so the span is identical to the cold RC walk for any hint (None/stale/garbage just cost probes).PRMI_ISAunset → the cold RC path, unchanged.Changes
spectrum.rsmem_search_backward_truncated_span_rcinto a privatespan_rc_walk(seed_win)(None== the cold walk, verbatim); the public fn delegates withNone. The existing…_equals_oraclegate still passes, confirming the refactor is behavior-preserving.mem_search_backward_span_rc_warmstart(seed_hint)— the warm-started twin, delegating withSome.mem_search_backward_warmstart_equals_coldproptest — RC warm == cold for ANY hint (the left twin ofmem_search_warmstart_equals_cold).collect.rsreseed_rc_hint— RC-strand projection2*l_pac − refpos − off − 1(doubled-text complement-mirror, matching BWA-MEMELearnedIndex_seeding.cpp:1535). Base is2*l_pac, notsa_num() == 2*l_pac+1— the+1sentinel is the off-by-one.zz_left_span_reseed; warm-start the RC walk from the projection.collect_smems_isa_left_warmstart_equals_cold_partial— ISA-on == ISA-off on a partial read that reseeds with left context.reseed_rc_hint_reduces_probes(featurespectrum-probe-count) — the load-bearing projection-correctness gate: the correct RC projection must touch strictly fewer probes than cold. A wrong formula (e.g.sa_num()base) seeds far from the interval and fails this — which neither the byte-id proptest nor the on==off gate can catch (a garbage projection passes both).Validation
cargo test --release -p prmi --lib— all pass except the pre-existing release-onlysa::doubled_text_tests::doubled_text_rejects_out_of_range_base(#[should_panic]on adebug_assert!; untouched here).cargo test --release -p prmi --lib --features spectrum-probe-count reseed_rc_hint_reduces_probes— passes.cargo clippy --release -p prmi --lib --all-features— clean.Summary by CodeRabbit