[codex] address release audit findings#264
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
PR Summary by QodoRelease audit fixes: Python manifest limit parity + OVFS/FastScan doc refresh Description
Diagram
High-Level Assessment
Files changed (19)
|
Code Review by Qodo
1.
|
There was a problem hiding this comment.
Thorough review of the release-audit PR:
F1 — Format registry (src/format.rs): Well-designed. Centralizing magic, IndexKind, probe/manifest/FFI coverage into one lockstep table eliminates the scattered format knowledge that F1 set out to remove. The #[non_exhaustive] enums and explicit tracking-issue fields mean new formats must make deliberate stance decisions. ✅
F2 — Python GIL safety: All GIL-released paths now copy inputs before py.detach. Spot-checked Rank.add, RankQuant.search, RankQuant.search_asymmetric, RankQuant.search_asymmetric_subset, Bitmap.search, and Bitmap.search_subset — all use owned vectors before detach. The CandidateIds::into_owned() helper correctly handles both borrowed and already-converted paths. ✅
F3 — C ABI OVFS reporting: ordvec_index_load now correctly returns ORDVEC_STATUS_UNSUPPORTED_FORMAT for OVFS (and SignBitmap/Rank) rather than the previous CORRUPT_INDEX/UNKNOWN mishandling. The info_for_metadata path remains consistent with the registry-backed dispatch. ✅
Tests: The new test_search_asymmetric_snapshots_query_before_detach regression test is a good stress test for the copy-before-detach invariant. The manifest test_verify_manifest_enforces_calibration_profile_limit covers the new Python binding surface. ✅
Docs: Updated links from Fieldnote-Echo/ordvec-formalization to Project-Navi/ordvec-formalization are consistent across README, SECURITY, threat model, and experiments. ✅
No blocking issues found. This is a solid pre-v0.5.0 release-surface cleanup.
Was this automated review useful? React with 👍 or 👎 to this review to help us measure review quality.
Workflow run: https://github.com/Project-Navi/ordvec/actions/runs/27853967591
Summary
ordvec-manifestPython bindings: default constant,default_resource_limits(), verifier/create keyword arguments, exports, and regression coverage forcalibration_profile_too_large.src/format.rsas the lockstep source of truth for format magic,IndexKind, probe coverage, manifest coverage, and C ABI load support. The registry is#[doc(hidden)]in v0.5.0 because it supports workspace crates such asordvec-manifest; it is not a documented downstream API.probe_index_metadata,ordvec-manifest, andordvec-ffithrough that registry..ovfs/OVFSis now explicitly known-but-not-probeable and not manifest-covered in v1, and the C ABI reports it asUNSUPPORTED_FORMATinstead of corrupt.py.detach. This closes the safe-Python mutation race class; docs and changelog call out the intentional memory tradeoff for large calls.(candidate_count = 0, candidates = NULL), while(0, non-NULL)isBAD_ARGUMENT..ovfs, FastScan, b=8 support boundaries, two-approver release governance, formalization links, and theSubsetScratch::capacities_for_testtest-utilsgate.Why
This PR addresses the release-surface audit before v0.5.0. The substantive fixes are:
OVFSthrough the C ABI as a known unsupported format, not as corruption.It also closes the earlier manifest Python parity gap and stale release/governance documentation found during the same audit pass.
Benchmark Note
The Python copy-before-detach change affects the Python binding. The public BEIR latency figures in the README are generated by the Rust
benchmarks/beir-benchbinary; the BEIR harness explicitly avoids importing the Pythonordvecpackage and has a Makefile guardrail for that. No README BEIR numbers are refreshed in this PR because that benchmark hot path does not cross the Python/FFI boundary.Validation
Local validation run during this PR:
cargo fmt --checkgit diff --checkcargo test --lockedcargo test --locked --features test-utils batched_into_is_allocation_free_after_warmupcargo test -p ordvec-manifest --lockedcargo test -p ordvec-ffi --lockedcargo check -p ordvec-python --lockedcargo check -p ordvec-manifest-python --lockedcargo test -p ordvec-manifest-python --lockedcargo clippy --workspace --all-targets --all-features --locked -- -D warningscargo clippy -p ordvec-manifest-python --all-targets --locked -- -D warningscargo doc -p ordvec --no-deps --locked/tmp/ordvec-cbindgen-0.29.3/bin/cbindgen ordvec-ffi --config ordvec-ffi/cbindgen.toml --output ordvec-ffi/include/ordvec.h --verifymaturin build --release --manifest-path ordvec-python/Cargo.toml --out /tmp/ordvec-python-distuv run --with pytest --with /tmp/ordvec-python-dist/ordvec-0.5.0-cp310-abi3-manylinux_2_34_x86_64.whl python -m pytest ordvec-python/tests -q(511 passed)maturin build --release --manifest-path ordvec-manifest-python/Cargo.toml --out /tmp/ordvec-manifest-python-distuv run --with pytest --with /tmp/ordvec-manifest-python-dist/ordvec_manifest-0.5.0-cp310-abi3-manylinux_2_34_x86_64.whl python -m pytest ordvec-manifest-python/tests -qpython3 tests/release_publish_invariants.pyGitHub Actions on the PR head remain the merge gate; the weekly fuzz job is expected to skip on ordinary PR pushes.