feat: safe-by-default SignLoadPolicy for verified loads#23
Open
Nelson Spence (Fieldnote-Echo) wants to merge 3 commits into
Open
feat: safe-by-default SignLoadPolicy for verified loads#23Nelson Spence (Fieldnote-Echo) wants to merge 3 commits into
Nelson Spence (Fieldnote-Echo) wants to merge 3 commits into
Conversation
DenseLoadOptions drops require_sign: bool for sign: SignLoadPolicy
{RequireIfSupported, Require, Any, Forbid}, defaulting to
RequireIfSupported — a bundle whose (dim, bits) can carry a sign
sidecar must have one to load. Forbid + declared sidecar surfaces as
the new DenseError::SignSidecarForbidden; Require + missing keeps
DenseError::MissingSignSidecar. io::load_bundle applies the same
default so the convenience load() entry points inherit the
strictness (typed errors for that surface arrive in PR2).
verify_for_load stays integrity-only: policy lives in open/load only.
Sign-capable bundle written with SignPolicy::Disabled fails the default load with MissingSignSidecar; Any loads it; Forbid rejects a signed bundle with SignSidecarForbidden and accepts an unsigned one; Require round-trips both outcomes; a sign-incapable bundle (dim=8) loads fine by default; the convenience load() matches open_verified defaults on all three bundle shapes.
downstream-smoke, supportsearch, and the arxiv-precomputed benchmark all pair a SignPolicy::Required write with a SignLoadPolicy::Require load. supportsearch still cannot build standalone until the ordvec 0.6.0 publish (pre-existing: its lockfile pins crates.io ordvec 0.5.0 and it carries no git patch, unlike the other two consumers).
Nelson Spence (Fieldnote-Echo)
requested review from
a team and
Navi Bot (project-navi-bot)
as code owners
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PR Summary by QodoSafe-by-default SignLoadPolicy for verified bundle loads
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Code Review by Qodo
1. Misleading sign-sidecar error
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
353
to
+358
| "invalid query value at query {query_index}, coord {coord_index}: {value}" | ||
| ), | ||
| Self::MissingSignSidecar => f.write_str("verified dense load requires a sign sidecar"), | ||
| Self::SignSidecarForbidden => f.write_str( | ||
| "verified dense load forbids a sign sidecar, but the bundle declares one", | ||
| ), |
There was a problem hiding this comment.
1. Misleading sign-sidecar error 🐞 Bug ◔ Observability
DenseError::MissingSignSidecar is emitted by both verified-load and SignTwoStage search paths, but its Display text claims it is a verified-load failure, which misleads callers when the error originates from search APIs.
Agent Prompt
## Issue description
`DenseError::MissingSignSidecar` is used in multiple contexts (verified load policy and SignTwoStage search), but its `Display` message is load-specific ("verified dense load requires a sign sidecar"). This makes error output misleading in search contexts and in any downstream code that logs `{err}`.
## Issue Context
- The enum variant is explicitly documented as used by search (`DenseSearchMode::SignTwoStage`) and now also by verified load.
- Tests print the error via `{err}` in a search context, so the message is user-visible.
## Fix Focus Areas
- ordinaldb/src/error.rs[332-370]
## Suggested fix
- Change the `Display` string for `DenseError::MissingSignSidecar` to be context-agnostic, e.g. "missing required sign sidecar" or "operation requires a sign sidecar".
- (Optional) If you want richer diagnostics, consider splitting into two variants (search vs load) or carrying context, but the minimal fix is to generalize the message.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+614
to
+626
| // This backs the convenience `load()` entry points, which take no | ||
| // `DenseLoadOptions`; they inherit the same default sign policy as | ||
| // `open_verified` (`SignLoadPolicy::RequireIfSupported`). | ||
| // TODO(PR2): type this error surface as `DenseError` instead of an | ||
| // `InvalidData` `io::Error`. | ||
| let sign = match auxiliary_path(&plan, SIGN_AUX_NAME)? { | ||
| Some(path) => Some(SignBitmap::load(path)?), | ||
| None if crate::ordinal::sign_compatible(metadata.dim, metadata_bits) => { | ||
| return Err(invalid_data( | ||
| "sign-capable bundle (bits=2, dim divisible by 64) has no sign sidecar; \ | ||
| open it with open_verified and SignLoadPolicy::Any to load without one", | ||
| )); | ||
| } |
There was a problem hiding this comment.
2. Stringly typed load error 🐞 Bug ⚙ Maintainability
The convenience load() path enforces the new default sign-sidecar policy but reports a missing sign sidecar as a generic io::Error with a formatted string, forcing downstream callers to string-match to distinguish it from other InvalidData failures.
Agent Prompt
## Issue description
`OrdinalIndex::load` / `IdMapIndex::load` fail a sign-capable bundle missing its sign sidecar, but the error is currently returned as `io::ErrorKind::InvalidData` with a string message. This prevents reliable programmatic handling (without brittle substring checks) and creates inconsistency with `open_verified`, which returns a typed `DenseError::MissingSignSidecar`.
## Issue Context
- This new failure mode is part of the PR’s safe-by-default behavior, so it is more likely to be encountered by callers using the convenience APIs.
- The repository already has a TODO noting the error surface should be typed.
## Fix Focus Areas
- ordinaldb/src/io.rs[577-636]
- ordinaldb/tests/sign_load_policy.rs[185-211]
## Suggested fix
- Instead of `invalid_data("...")`, construct an `io::Error` that carries a typed inner error, e.g.:
- `return Err(std::io::Error::new(std::io::ErrorKind::InvalidData, crate::DenseError::MissingSignSidecar));`
This keeps the public return type as `io::Result` while allowing callers to downcast via `err.get_ref().and_then(|e| e.downcast_ref::<DenseError>())`.
- Update the test to assert on the downcasted `DenseError::MissingSignSidecar` rather than substring-matching.
- If you prefer a dedicated typed error for convenience loads, introduce a small `LoadError` enum and wrap that instead.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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.
Wave 1 / PR4 of the post-mortem hardening (post-mortem F5). Stacked on the SignPolicy PR (
feat/sign-policy) — merge that first.DenseLoadOptions.require_sign: bool→sign: SignLoadPolicy { RequireIfSupported, Require, Any, Forbid }; hand-written Default = RequireIfSupported: a sign-capable bundle (bits=2, dim%64==0) missing its sidecar now FAILS by default instead of silently falling back to the slow exact scan.Anyrestores old behavior explicitly;Forbidgets a dedicatedDenseError::SignSidecarForbidden.expected_dim/expected_bitsstay None-default caller assertions — adjudicated with evidence: the load path already unconditionally cross-checks manifest-recorded dim/bits against artifact bytes; the sign policy was the only real default-safety hole.load()(io.rs) routes through the same strict default — no looser side door;verify_for_loadstays integrity-only (policy only in open/load, per the round-2 API-split edit).Adversarial 3-lens review: clean (low-only; SignLoadPolicy is deliberately a plain enum like SignPolicy — flagged deviation). Tests: 253 passed workspace all-features (+9 gate tests in
tests/sign_load_policy.rs); doctests + clippy/fmt clean.🤖 Generated with Claude Code
https://claude.ai/code/session_01BQ1JzAocstWiqtCF5J2V7K