Pre-flip: remediate release-gate audit (8 findings) + full Python parity#23
Merged
Conversation
- Bitmap::new caps dim at MAX_DIM (u16::MAX). A larger dim is still a multiple of 64 but cannot be rank-transformed (u16 ranks) or query-indexed (u16 ids), so it previously built an index that panicked on the first add/search and disagreed with the .tvbm loader (which already caps dim). Now consistent with the Rank/RankQuant constructors. - Bitmap::top_m_candidates_batched_chunked guards batch_size > 0 and uses checked_mul for batch_size * dim, so a zero or overflowing batch_size fails loud instead of panicking inside par_chunks. - rank_to_bucket computes the bucket in u64: `d as u32` could truncate a large d to zero and divide-by-zero. Bit-identical over the realistic d <= u16::MAX domain; removes a panic otherwise reachable where d is a free argument. Tests: should_panic guards for the dim cap and zero batch_size, plus a large-d rank_to_bucket regression (64-bit).
Loaders validated structure (magic/version/dim/n_vectors/payload length) but accepted well-shaped payloads that violate the type's semantic invariant, silently corrupting scores instead of failing. Add per-row checks at the loader boundary: - .tvr (Rank): each row must be a permutation of [0, dim), not merely bounded by dim — rank_norm assumes a permutation. - .tvrq (RankQuant): each document must have uniform bucket composition (dim / 2^bits per bucket), which rankquant_norm assumes. - .tvbm (Bitmap): each document must have exactly n_top bits set. - .tvsb (SignBitmap): documented as legitimately structural-only — any bit pattern is a valid sign bitmap, so there is nothing further to verify. Tests: a new loader_validation module pairs a valid-file positive control with a corrupted-but-well-shaped negative case per format.
Bring the ordvec Python bindings to parity with the Rust crate's public surface so anything callable in Rust is callable from Python: - All four classes gain is_empty. - Bitmap gains build_query_bitmap_fp32, top_m_candidates_batched, top_m_candidates_batched_chunked, and body_overlap_scores_subset. - SignBitmap gains build_query_bitmap. - The rank-math primitives (rank_transform, rank_to_bucket, bucket_ranks, pack_buckets, unpack_buckets, rankquant_bytes_per_vec, bucket_centre, rank_norm, rankquant_norm), the byte-LUT helper search_asymmetric_byte_lut, and the limit constants MAX_DIM / MAX_SIGN_BITMAP_DIM / MAX_VECTORS are exposed at module level. The low-level rank_io read/write functions are intentionally not duplicated — the class write()/load() methods cover them. Every new entry point validates inputs at the FFI boundary (typed ValueError / IndexError, never a PanicException): Bitmap rejects dim > u16::MAX; the chunked candidate path clamps batch_size to the query count to avoid batch_size * dim overflow; body_overlap_scores_subset checks q_bitmap length, doc-id bounds, and ascending order; the primitives validate bits and array lengths. Also flips the stale tie-break xfail in test_bitmap.py to a passing regression test — the composite-key (score desc, doc_id asc) determinism fix it described is in the core and Rust-tested.
- Add a cargo-deny job to ci.yml so deny.toml (advisories, license allow-list, banned/duplicate crates, source allow-list) is actually enforced rather than only checklisted in the PR template. - release-crate.yml and release-python.yml now require the ci (and, for the wheel, python) workflow to have concluded success for the exact release SHA before publishing, so a manual release dispatch can't ship a commit whose full matrix (lint, MSRV, the test configs, deps + cargo-deny, AVX-512-SDE, wasm-simd128) has not gone green — without duplicating that matrix.
- README license links use absolute repository URLs: PyPI renders the README on the project page, where relative links (LICENSE-MIT/LICENSE-APACHE) 404. - Add the Windows operating-system classifier — release-python.yml builds and tests a windows-latest wheel, but the classifier set listed only Linux/macOS.
Review Summary by QodoPre-flip: remediate release-gate audit (8 findings) + full Python parity
WalkthroughsDescription• Harden core constructors and kernels against out-of-domain inputs - Bitmap::new caps dim at MAX_DIM (u16::MAX); top_m_candidates_batched_chunked guards batch_size > 0 with checked_mul - rank_to_bucket uses u64 math to prevent divide-by-zero from large d values • Validate semantic invariants on index load (permutation, composition, popcount) - .tvr loader verifies each row is a permutation of [0, dim) - .tvrq loader checks uniform bucket composition (dim / 2^bits per bucket) - .tvbm loader validates exactly n_top bits set per document • Mirror full Rust public API in Python bindings for complete parity - Add 10 module-level rank-math primitives (rank_transform, rank_to_bucket, bucket_ranks, etc.) - Expose 3 loader limit constants (MAX_DIM, MAX_SIGN_BITMAP_DIM, MAX_VECTORS) - Add missing class methods (is_empty, build_query_bitmap*, body_overlap_scores_subset, top_m_candidates_batched*) • Strengthen release workflows with full CI matrix gate and cargo-deny supply-chain checks - Add cargo-deny job to ci.yml for advisories/licenses/bans/sources validation - Require both ci.yml and python.yml to pass before publishing to crates.io/PyPI Diagramflowchart LR
A["Core Hardening<br/>Constructors & Kernels"] --> B["Input Validation<br/>Typed Exceptions"]
C["Semantic Validation<br/>Loaders"] --> D["Reject Corrupted<br/>Payloads"]
E["Python API Expansion<br/>Rank Math + Constants"] --> F["Full Rust Parity<br/>10 Functions + 3 Constants"]
G["Release Workflow<br/>Strengthening"] --> H["cargo-deny +<br/>CI Matrix Gate"]
B --> I["Safe Public API"]
D --> I
F --> I
H --> I
File Changes2. src/rank.rs
|
Code Review by Qodo
1.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 923cd15f9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Pre-release hardening across the Rust core, persistence loaders, Python bindings, and release/CI workflows to address the stated release-gate audit findings and bring Python up to full parity with the Rust public API surface.
Changes:
- Strengthen loader semantic validation for Rank / RankQuant / Bitmap formats and add targeted regression tests.
- Prevent previously reachable panics (e.g.,
rank_to_bucketlarge-d,Bitmap::newoversizedim, chunked batching withbatch_size=0/overflow). - Expand Python bindings to include missing methods, module-level rank primitives, and exposed constants; tighten release workflows to require green CI before publishing.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/index/main.rs | Registers new loader semantic-validation test module. |
| tests/index/loader_validation.rs | Adds round-trip + “well-shaped but semantically invalid” loader rejection/acceptance tests. |
| tests/index/bitmap.rs | Adds regression guards for oversize dim and zero batch_size panics. |
| src/rank.rs | Switches rank_to_bucket to u64 math and adds a large-d regression test. |
| src/rank_io.rs | Enforces semantic invariants at load time (Rank permutation, RankQuant constant composition, Bitmap popcount), and documents SignBitmap structural-only validation. |
| src/bitmap.rs | Adds dim <= MAX_DIM constructor guard; guards chunked batching against zero/overflow. |
| ordvec-python/tests/test_sign_bitmap.py | Adds Python-level is_empty and build_query_bitmap tests for parity. |
| ordvec-python/tests/test_rank.py | Adds Python-level is_empty parity test. |
| ordvec-python/tests/test_rank_quant.py | Adds Python-level is_empty parity test. |
| ordvec-python/tests/test_primitives.py | Adds coverage for newly exposed module-level primitives and constants. |
| ordvec-python/tests/test_bitmap.py | Converts prior xfail to regression coverage; adds parity tests for new Bitmap APIs and argument guards. |
| ordvec-python/src/lib.rs | Implements Python API parity: new methods, module-level primitives, input guards to avoid PanicException, and exports constants. |
| ordvec-python/README.md | Fixes license links to non-404 absolute URLs. |
| ordvec-python/python/ordvec/init.py | Re-exports new public primitives/constants and updates __all__. |
| ordvec-python/pyproject.toml | Adds Windows classifier. |
| .github/workflows/release-python.yml | Adds a “require CI green for SHA” gate before publishing wheels/sdist. |
| .github/workflows/release-crate.yml | Adds a “require CI green for SHA” gate before publishing crate. |
| .github/workflows/ci.yml | Adds cargo-deny supply-chain policy job. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly expands the Python API for the ordvec package by exposing module-level rank-math primitives, loader limit constants, and advanced Bitmap search methods such as batched and chunked candidate generation. In the Rust core, it introduces semantic validation during index loading to ensure data integrity (e.g., verifying permutations and constant-composition invariants) and improves numerical robustness by using u64 arithmetic for bucket calculations. Feedback was provided regarding potential usize overflows on 32-bit architectures when calculating the capacity for result vectors in the Python bindings.
Bot-review remediation (qodo / gemini / copilot / Codex) on PR #23: - pack_buckets rejects bucket codes outside [0, 1<<bits) with a ValueError instead of letting the core silently mask them (`b & mask`) to a different bucket (qodo, Correctness). - Bitmap::top_m_candidates_batched / _batched_chunked and SignBitmap::top_m_candidates_batched use checked_mul for the result-vector capacity, so an oversized batch*m can't overflow usize and panic in Array2::from_shape_vec (gemini). - rank_transform test uses a stable inner argsort to match the core's "ties broken by index" contract (robust to a float32 tie), plus an explicit tie-break test (copilot). - Regression test pinning bucket_ranks([]) -> []: the empty-input panic flagged by Codex/qodo is not reachable — the core maps over the empty slice and never calls rank_to_bucket, so its `d > 0` assert is unreached, and non-empty input always has d >= 1 (verified empirically). pytest 147 -> 150, all green.
The require-ci-green gate queried ci.yml/python.yml runs by head_sha and took the most recent, so a green run for the same commit SHA on an unrelated branch could satisfy it (qodo). Filter on `branch=main` and require a run with `head_branch == "main" and conclusion == "success"`, matching the gate's "push to main" error message.
Owner
Author
Bot review — cycle 1 triage (resolved)Investigated every finding from copilot / gemini / qodo / Codex and remediated or resolved each. Fixes in
On the empty-
|
The cycle-1 checked_mul guards sized the output buffer (batch * m_eff) and ran *after* top_m_candidates_batched, but the core allocates `scores = vec![0u32; batch * n]` *inside* that call — a larger quantity (n >= m_eff) that would overflow before the guard ran (Codex stop-review). Guard `batch * max(n, qpv)` (the core's scores + query-bitmap buffers) BEFORE the call in all three batched wrappers (Bitmap batched / chunked, SignBitmap batched), so an overflow is a clean ValueError instead of a wrap -> out-of-bounds panic. The output-buffer guard is kept (now provably safe).
bucket_ranks([]) already returned [] (the core maps over the empty slice and never calls rank_to_bucket, so its `d > 0` assert is unreachable — verified empirically), but the static analyzers (qodo Bug / Codex P1) keep flagging the empty path because they cannot see the iterator short-circuit. Add an explicit `slice.is_empty()` early-return so the empty -> empty contract is local to the boundary and the d == 0 case provably never reaches the core. Behavior unchanged; the existing test_bucket_ranks_empty_returns_empty regression still pins empty -> empty.
… APIs Bitmap::top_m_candidates_batched and SignBitmap::top_m_candidates_batched allocated `vec![0u32; batch * n]` and `vec![0u64; batch * qpv]` with unchecked multiplication. On a 32-bit target (wasm32, which this crate supports) a moderate corpus plus a large query batch can overflow usize, silently under-sizing those buffers and then indexing out of bounds. Compute the lengths with checked_mul + a clear expect() so the failure is loud on every target. The Python binding already guards these (it rejects/clamps before calling the core), so this closes the Rust public-API / wasm32 side. Behavior unchanged for non-overflowing inputs; gate green including the wasm32 +simd128 build.
Owner
Author
Merge-review follow-upClosed (Medium): unchecked
|
Fieldnote-Echo
added a commit
that referenced
this pull request
May 25, 2026
Addresses the PR #30 bot-review findings (gemini x5, copilot x1): - gemini (high): the add() growth paths could overflow usize when computing the resize length on 32-bit targets (wasm32 / armv7), where MAX_VECTORS (2^26) * a large dim / qpv / bytes-per-vec (up to ~2^16) exceeds usize::MAX. Centralized the guard in checked_new_len, which now also takes elems_per_vec and checked_mul's new_n * elems_per_vec (fail-loud), matching the crate's checked-allocation discipline (cf. #23 candidate APIs, result_buffer_len). Callers pass dim / packed-bytes-per-vec / qwords-per-vec. Bounded in practice by Rust's <= isize::MAX-bytes Vec invariant, but now explicit. +unit test checked_new_len_rejects_buffer_overflow. - copilot: the write-guard test's temp filenames used only the pid (which the OS reuses), so a leftover file from an aborted run could spuriously fail the !exists() checks. Added a per-run nanosecond nonce (matching `forge`). (copilot's other finding — pub(crate) breaking the fuzz crate — was already fixed in 9713f0d, before that review landed.) Gate green: fmt, clippy -D warnings, rustdoc -D warnings, test (default/experimental/no-default), MSRV 1.89, --locked, +nightly fuzz build.
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
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
Pre-public-flip hardening. This remediates an external release-gate static audit of the crate and bindings (8 findings), adds complete Python↔Rust API parity, and closes a Codex stop-review finding (a core panic reachable from the Python boundary). Every finding was triaged against the actual source and fixed at the source — no severity downgrades, no deferrals to issues.
Publish remains held: the release workflows are
workflow_dispatch-only and nothing here triggers a publish.Audit findings (all fixed)
Bitmap::newaccepteddim > u16::MAX→ later panic (Rust + Python)Bitmapcapdim ≤ MAX_DIM; matchesRank/RankQuantand the.tvbmloaderdeny.tomlnever runcargo-denyjob added toci.yml; both release workflows now require the fullci/pythonmatrices to be green for the release SHA before publish.tvrpermutation,.tvrqbucket-composition,.tvbmpopcount checks;.tvsbdocumented as legitimately structural-onlyxfailintest_bitmap.pysearch())top_m_candidates_batched_chunked(batch_size=0)panickedbatch_size > 0guard +checked_mul; Python clampsbatch_sizeto the query countOperating System :: Microsoft :: WindowsaddedCodex stop-review follow-up
rank_to_bucketdidd as u32, which truncates ad ≥ 2³²to zero → divide-by-zero. Newly reachable because the binding exposesrank_to_bucket/bucket_rankswithdas a free argument. Fixed in the core with u64 math (bit-identical over the realisticd ≤ u16::MAXdomain) so Rust callers are protected too, plus the chunked-batchbatch_sizeclamp above. Swept the rest of the new FFI surface: every entry point now turns a coreassert!/overflow into a typedValueError/IndexError, never aPanicException.Full Python parity (finding 7)
Anything callable in Rust is now callable from Python:
is_empty.Bitmap:build_query_bitmap_fp32,top_m_candidates_batched,top_m_candidates_batched_chunked,body_overlap_scores_subset.SignBitmap:build_query_bitmap.rank_transform,rank_to_bucket,bucket_ranks,pack_buckets,unpack_buckets,rankquant_bytes_per_vec,bucket_centre,rank_norm,rankquant_norm,search_asymmetric_byte_lut, and constantsMAX_DIM/MAX_SIGN_BITMAP_DIM/MAX_VECTORS.The low-level
rank_ioread/write functions are deliberately not duplicated — the classwrite()/load()methods already cover that capability, and exposing raw tuple-returning loaders would be parity of implementation detail, not API.Test plan (local gate — mirrors CI)
cargo fmt --all --check,cargo clippy --all-targets --all-features -- -D warningscargo test(default /--features experimental/--no-default-features) — index harness 39 → 45 (loader-validation module + 2should_panicguards), lib units 19 → 20cargo +1.89.0 build+ test (MSRV),cargo build --locked,RUSTFLAGS="-D warnings" cargo build,cargo +nightly fuzz buildcargo deny check→ advisories / bans / licenses / sources okcargo fmt -p ordvec-python --check,cargo clippy -p ordvec-python --all-targets -- -D warnings,maturin develop+pytest→ 147 passed, 0 failed, 0 xfail (108 + 1 xfail before)Note for releasing
The new
require-ci-greengate queriesci.yml/python.ymlruns byhead_sha, so release from a commit that went throughmain's CI (the normal flow). It fails loud with guidance if CI isn't green for the SHA.