docs(rank_io): note OVFS metadata-probe deferral (#232)#242
Conversation
) A stop-gate review noted that `.ovfs` (RankQuantFastscan) persistence "bypasses the metadata path": `probe_index_metadata` dispatches the four OV*/TV* formats but `OVFS` falls through to the unknown-magic error, with no in-code note that this is deliberate. It IS deliberate: surfacing OVFS in the probe requires a new `IndexKind` variant, and `IndexKind` is not `#[non_exhaustive]`, so adding one is a breaking change — deferred to the 0.8.0 API re-architecture (#232). `.ovfs` files still round-trip via `RankQuantFastscan::{write,load}`; only the metadata-probe path is pending. Add a comment in the dispatch so the deferral reads as intentional, not an oversight. No behaviour change. Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Code Review by Qodo🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)
Great, no issues found!Qodo reviewed your code and found no material issues that require review |
PR Summary by QodoDocument intentional OVFS deferral in index metadata probe WalkthroughsDescription• Add an in-code note explaining why OVFS is not handled by probe_index_metadata.
• Clarify that adding OVFS would require a breaking IndexKind expansion, deferred to 0.8.0.
• Document that .ovfs still round-trips via RankQuantFastscan::{write,load}.
High-Level AssessmentThe following are alternative approaches to this PR: 1. Add `IndexKind::Ovfs` now (breaking)
2. Introduce a non-breaking probe result type
Recommendation: For a non-behavioral stop-gate PR, the chosen approach (documenting the intentional OVFS deferral and why it falls through) is optimal. The meaningful functional alternatives either require a breaking File ChangesDocumentation (1)
|
![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 adds a detailed comment in src/rank_io.rs explaining why OVFS (RankQuantFastscan) is intentionally omitted from the metadata probing path to avoid a breaking API change. The reviewer suggested explicitly matching OVFS_MAGIC and returning a specific error indicating that metadata probing is unsupported in this version, rather than letting it fall through to an "unknown magic" error.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Address gemini review on #242: instead of letting an .ovfs (OVFS / RankQuantFastscan) magic fall through to the generic "unknown ordvec index magic" error in probe_index_metadata — misleading, since the magic *is* recognized — explicitly match OVFS_MAGIC and return an actionable error pointing the caller at RankQuantFastscan::load and the #232 probe deferral. Add a regression test pinning the deferral contract: probing a valid .ovfs file must return the specific unsupported-probe error and must NOT report it as an unknown magic. Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
|
Addressed the gemini-code-assist suggestion in f0808ad:
— instead of letting |
2 participants
Stop-gate follow-up to #233. A review flagged that
.ovfs(RankQuantFastscan) persistence "bypasses the metadata/manifest path" —probe_index_metadatahandles the four OV*/TV* formats butOVFSfalls through to the unknown-magic error with no note that this is intentional.It is intentional: surfacing OVFS in the probe requires a new
IndexKindvariant, andIndexKindis not#[non_exhaustive], so adding one is a breaking change — deferred to the 0.8.0 API re-architecture (#232)..ovfsfiles still round-trip viaRankQuantFastscan::{write,load}; only the metadata-probe path is pending. This adds a comment so the deferral reads as deliberate.No behaviour change (comment only). Builds + fmt clean.