feat: typed verification codes + shared hash helpers#288
Conversation
Extract the EINTR-safe bounded hashing core from sha256_file_bounded and share it with a new public sha256_reader; add public sha256_bytes and dedupe the private sqlite.rs copy onto it.
Add ordvec_manifest::codes with one pub const per issue code (including the previously format!-built path, metric-spec, and row-id families, now selected via per-context const structs) and reference the consts at every emit site, internal compare, and bounded-read call. A source-scan test rejects new bare literals at emit sites; a value-lock test pins the security-relevant code strings so silent renames break tests.
Add non_exhaustive VerificationCode with ReportIssue::classification() over the named code consts so downstream security code branches on typed values. ReportIssue gains optional artifact_name and expected/actual sha256+size fields (skip_serializing_if None keeps existing report JSON byte-stable), populated at the artifact, auxiliary, and row-identity mismatch sites for downstream IntegrityError reconstruction.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
PR Summary by QodoAdd typed verification codes and shared SHA-256 helpers
AI Description
Diagram
High-Level Assessment
Files changed (5)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Artifact missing misclassified
|
The merge of feat/deterministic-manifest landed the new
{context}_absolute_path_unresolvable rejection while this branch had
replaced resolve_existing_path's context string with PathIssueCodes and
moved every emitted code into the pub codes registry. Wire the new
rejection through both: an absolute_path_unresolvable field on
PathIssueCodes, per-context consts (artifact, row_identity,
encoder_distortion_profile, calibration_profile, auxiliary_artifact),
and locked code values for the two security-relevant variants in
verification_codes.rs.
The 0.7.0 release section arrived via the deterministic-manifest merge with only the schema-v2 entry; add the Added entries for this branch's typed verification classification, structured mismatch detail, and shared hash helpers so the release notes cover the full stacked change. Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
|
Merged |
Audit remediation for the typed VerificationCode classification, from an adversarial re-review (confirms and extends the qodo finding on #288): resolve_existing_path emitted *_path_unavailable for every canonicalize error kind, and classification() mapped ARTIFACT_PATH_UNAVAILABLE to VerificationCode::ArtifactMissing. A present-but-unreadable primary artifact (permission denied, symlink loop, I/O error) was therefore reported to downstream integrity handling as a missing file. The auxiliary path already discriminated NotFound; the primary and row-identity paths did not. - Add codes ARTIFACT_MISSING / ROW_IDENTITY_MISSING, emitted only on an ErrorKind::NotFound canonicalize failure (via a new optional path_missing on PathIssueCodes; profiles keep a single path_unavailable code). - Classify ARTIFACT_MISSING -> ArtifactMissing and the new ROW_IDENTITY_MISSING -> RowIdentityMissing (closing a second gap: a missing rows.jsonl previously classified Unknown while its sha/row-count mismatches were typed). ARTIFACT_PATH_UNAVAILABLE and ROW_IDENTITY_PATH_UNAVAILABLE now classify Unknown like their siblings. - Correct the classification() doc: it over-claimed that all security-relevant families are typed, when path-policy rejections and diagnostic I/O failures deliberately map to Unknown. Tests: end-to-end artifact_missing and row_identity_missing emit+classify, both *_path_unavailable codes pinned to Unknown, locked code-string table extended; ordvec-manifest --all-features 114 passed. Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
|
Follow-up audit round (adversarial re-review, 3 independent reviewers). The qodo finding (ARTIFACT_PATH_UNAVAILABLE→ArtifactMissing) is confirmed and fixed at root cause in
qodo thread resolved with the evidence. |
903baa4
into
feat/deterministic-manifest
The GitHub merge of #288 into this branch left the last push attributed to project-navi-bot, which require_last_push_approval rejects — so the CODEOWNER approval no longer counts and the PR cannot merge. Push an empty commit as the branch owner (Fieldnote-Echo) to reset the last pusher; a re-approval then satisfies the rule. No content change: the stack (deterministic schema v2 + typed verification codes + the audit-round fixes) is complete, up to date with main, and green. Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
…tent address (#287) * feat: deterministic manifest schema v2 Remove manifest_id and created_at from IndexManifest so identical content serializes to identical bytes and sha256(manifest.json) becomes the bundle's content address. Creation writes build: None, sorts auxiliary artifact entries by (name, path), and validation rejects non-canonical embedded paths. Schema version bumps to ordvec.index_manifest.v2 with a targeted schema_version probe so old-schema manifests fail parse with a clear older/newer-schema error. VerificationReport and the sqlite registry drop manifest_id keying; a stale manifest_id column now forces the verification_reports migration. uuid dependency retained: still used for row-identity db_id validation. * test: determinism merge gates for manifest bytes Byte-identical double-create, checked-in golden bytes, artifact and auxiliary change sensitivity, auxiliary declaration-order invariance, old-schema rejection with a clear error, and canonical-path validation. * fix: enforce canonical manifest paths at create and verify Creation now runs the same canonical-form check on every embedded path (artifact, row identity, auxiliary artifacts) that verification applies, so create can no longer mint a manifest that fails its own default verification (e.g. a Unix filename containing a backslash). Non-escaping `..` segments (a/../index.ovrq) are now rejected as non-canonical by default: they pass lexical-escape and containment checks while aliasing the same bundle content under distinct verified manifest byte-identities. `..` remains available under the existing allow_path_escape opt-in, matching the documented policy split. Absolute path strings (POSIX, drive-letter, UNC) are excluded from the canonical-form check and governed solely by the allow_absolute_paths policy at resolution, restoring the retained absolute-path opt-in on Windows; path_to_manifest_string also strips Windows verbatim prefixes (\\?\) so created absolute paths round-trip through verification. Calibration and encoder-distortion profile ref paths gain the same canonical check (calibration_profile_path_not_canonical, encoder_distortion_profile_path_not_canonical) closing the remaining aliasing surface on hand-written manifests. * ci: run ordvec-manifest tests across the OS matrix The manifest crate embeds and resolves bundle paths, so its behavior is OS-sensitive, and it ships cross-platform (crates.io plus Windows/macOS wheels via ordvec-manifest-python) — but only the ubuntu manifest lane ran its tests. Add a default-features test step to the 3-OS matrix job; the dedicated ubuntu lane keeps covering the feature matrix and clippy. * docs: schema v2 README refresh and changelog entry The crate README still documented ordvec.index_manifest.v1 and showed verification-report examples carrying the removed manifest_id field. Document the v2 deterministic-bytes contract (content addressing, sorted auxiliary entries, canonical embedded paths), drop stale schema-version tags from the row-identity guidance, and add the new auxiliary_artifact_path_not_canonical reason code to the failure list. Record the breaking schema change under [Unreleased] in CHANGELOG.md as CONTRIBUTING.md requires for user-facing changes. * fix: close backslash-absolute manifest path bypass is_manifest_path_absolute() treated any leading backslash as absolute, which skipped the canonical-form check, while resolution used platform Path::is_absolute() semantics — so on Unix a manifest embedding \evil resolved inside the bundle and verified successfully under the default policy. Only UNC/verbatim (\\-prefixed) forms now count as backslash-absolute; a single leading backslash falls through to the canonical check, which rejects backslashes. Path resolution now classifies with the same platform-independent rule for the allow_absolute_paths gate and rejects outright (`{context}_absolute_path_unresolvable`) whenever the policy classification and the platform's resolution semantics disagree, so an absolute-for-policy path can never silently resolve relative to the manifest base on any OS. * fix: drop stale active_manifest schema on sqlite registry init Schema v2 dropped active_manifest's manifest_id column, but init() only migrated verification_reports; CREATE TABLE IF NOT EXISTS left a legacy active_manifest table in place, so activate()'s INSERT failed at runtime with a NOT NULL constraint on old registry DBs. init() now compares each registry table's live column set against the current schema via PRAGMA table_info and drops mismatched tables before recreating them empty. The registry is rebuildable cache/pointer state (cached verification reports and the active-manifest pointer, never source of truth), so stale schemas are dropped, not migrated — no back-compat machinery by design. The check runs on every init(), ahead of any activate() insert, and is idempotent. * chore(release): bump lockstep version to 0.7.0 for manifest schema v2 The deterministic manifest schema v2 is a breaking change on top of the release-shaped 0.6.0 tree, and the release-publish invariants require the current version to have a dated changelog section with an empty [Unreleased]. Bump every lockstep version field (core, manifest, ffi, both python bindings), the ordvec dep requirement in ordvec-manifest, the README quickstart requirement, and the THREAT_MODEL status line; move the schema-v2 changelog entry into a dated 0.7.0 section. Both lockfiles re-synced to the new member versions. Also bumps crossbeam-epoch 0.9.18 -> 0.9.20 in both lockfiles to clear RUSTSEC-2026-0204 (same fix as on main), so the branch's cargo-deny advisories job passes independently of the merge base. Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * fix: platform-proof the schema-v2 test suite (windows CRLF golden, macOS non-UTF-8) Two OS-matrix failures from running ordvec-manifest tests beyond Linux: - The checked-in golden manifest is byte-compared via include_bytes!, and Windows runners check it out through autocrlf, turning every LF into CRLF and failing the golden byte-identity test. Pin the golden dir -text via .gitattributes so checkout never rewrites fixture bytes. - verify_for_load_preserves_non_utf8_base_paths creates a directory named with an invalid UTF-8 byte; macOS APFS rejects that with EILSEQ at creation, so the scenario cannot exist there. Gate the test to Linux instead of cfg(unix). Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * fix: portable missing-file assertion for the windows io error wording Windows reports a missing file as 'The system cannot find the file specified. (os error 2)' — neither 'No such file' nor 'not found'. Accept the windows wording (and the platform-neutral 'os error 2' raw-io suffix), and print the actual message on failure. Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * fix: atomic sqlite migration, load-time schema check, audit doc fixes Audit remediation for schema-v2 (ordvec-manifest), all found by an adversarial re-review of the stack: - sqlite report-registry migration is now atomic. The v1->v2 RENAME/CREATE/INSERT/DROP ran as a bare execute_batch (each statement auto-committed), so an interruption after the RENAME left a stray verification_reports_old that wedged every future open (the RENAME target already existed), and two processes could race it. Run it in one IMMEDIATE transaction, DROP any leftover _old first, and re-check the need under the write lock so a lost race commits a no-op. - load_manifest_file now rejects an unsupported schema_version explicitly. A genuine v1 manifest already failed the parse (its manifest_id / created_at trip deny_unknown_fields), but a v2-shaped document that only mislabels schema_version would load and fail solely at verify; the loader now enforces the version as documented. - golden-bytes test message no longer misattributes every failure to a manifest schema change (an .ovrq index-format change or an editor newline-normalizing the fixture are the other causes). - CHANGELOG: creation omits the build field (not 'build: null'), and the registry migration line now distinguishes the migrated verification cache from the reset active_manifest pointer. Tests: +leftover-_old migration recovery, +wrong-schema-version load rejection; ordvec-manifest --all-features 102 passed. Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * feat: typed verification codes + shared hash helpers (#288) * feat: add sha256_bytes and bounded sha256_reader hash helpers Extract the EINTR-safe bounded hashing core from sha256_file_bounded and share it with a new public sha256_reader; add public sha256_bytes and dedupe the private sqlite.rs copy onto it. * refactor: name every verification issue code as a pub const Add ordvec_manifest::codes with one pub const per issue code (including the previously format!-built path, metric-spec, and row-id families, now selected via per-context const structs) and reference the consts at every emit site, internal compare, and bounded-read call. A source-scan test rejects new bare literals at emit sites; a value-lock test pins the security-relevant code strings so silent renames break tests. * feat: typed verification classification and structured mismatch detail Add non_exhaustive VerificationCode with ReportIssue::classification() over the named code consts so downstream security code branches on typed values. ReportIssue gains optional artifact_name and expected/actual sha256+size fields (skip_serializing_if None keeps existing report JSON byte-stable), populated at the artifact, auxiliary, and row-identity mismatch sites for downstream IntegrityError reconstruction. * fix: adapt absolute-path-unresolvable codes to typed const registry The merge of feat/deterministic-manifest landed the new {context}_absolute_path_unresolvable rejection while this branch had replaced resolve_existing_path's context string with PathIssueCodes and moved every emitted code into the pub codes registry. Wire the new rejection through both: an absolute_path_unresolvable field on PathIssueCodes, per-context consts (artifact, row_identity, encoder_distortion_profile, calibration_profile, auxiliary_artifact), and locked code values for the two security-relevant variants in verification_codes.rs. * docs: changelog entry for typed verification codes under 0.7.0 The 0.7.0 release section arrived via the deterministic-manifest merge with only the schema-v2 entry; add the Added entries for this branch's typed verification classification, structured mismatch detail, and shared hash helpers so the release notes cover the full stacked change. Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * fix: distinguish missing files from I/O failures in typed classification Audit remediation for the typed VerificationCode classification, from an adversarial re-review (confirms and extends the qodo finding on #288): resolve_existing_path emitted *_path_unavailable for every canonicalize error kind, and classification() mapped ARTIFACT_PATH_UNAVAILABLE to VerificationCode::ArtifactMissing. A present-but-unreadable primary artifact (permission denied, symlink loop, I/O error) was therefore reported to downstream integrity handling as a missing file. The auxiliary path already discriminated NotFound; the primary and row-identity paths did not. - Add codes ARTIFACT_MISSING / ROW_IDENTITY_MISSING, emitted only on an ErrorKind::NotFound canonicalize failure (via a new optional path_missing on PathIssueCodes; profiles keep a single path_unavailable code). - Classify ARTIFACT_MISSING -> ArtifactMissing and the new ROW_IDENTITY_MISSING -> RowIdentityMissing (closing a second gap: a missing rows.jsonl previously classified Unknown while its sha/row-count mismatches were typed). ARTIFACT_PATH_UNAVAILABLE and ROW_IDENTITY_PATH_UNAVAILABLE now classify Unknown like their siblings. - Correct the classification() doc: it over-claimed that all security-relevant families are typed, when path-policy rejections and diagnostic I/O failures deliberately map to Unknown. Tests: end-to-end artifact_missing and row_identity_missing emit+classify, both *_path_unavailable codes pinned to Unknown, locked code-string table extended; ordvec-manifest --all-features 114 passed. Signed-off-by: Nelson Spence <nelson@projectnavi.ai> --------- Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * chore: reset last-push attribution to branch owner The GitHub merge of #288 into this branch left the last push attributed to project-navi-bot, which require_last_push_approval rejects — so the CODEOWNER approval no longer counts and the PR cannot merge. Push an empty commit as the branch owner (Fieldnote-Echo) to reset the last pusher; a re-approval then satisfies the rule. No content change: the stack (deterministic schema v2 + typed verification codes + the audit-round fixes) is complete, up to date with main, and green. Signed-off-by: Nelson Spence <nelson@projectnavi.ai> * docs: note serde_json feature-unification risk on the manifest content address write_manifest_file's canonical bytes are the bundle's content address, and nested serde_json::Value maps (extensions / attestations) are key-sorted only while serde_json is built without preserve_order. Document that a downstream graph enabling serde_json/preserve_order (or arbitrary_precision) via feature unification would change the content address, and must not be used for content-addressed manifests. Documents the constraint half of #295. (Also serves as a reviewable Fieldnote-Echo push to reset last-push approval on this PR — the earlier empty commit was not a reviewable push.) Signed-off-by: Nelson Spence <nelson@projectnavi.ai> --------- Signed-off-by: Nelson Spence <nelson@projectnavi.ai>
Wave U2 of the OrdinalDB post-mortem hardening (post-mortem F2 upstream half + F14). Stacked on #287 (
feat/deterministic-manifest) — merge that first; GitHub will retarget this to main.pub constin acodesmodule — zero bare literals at emit sites; internal string-compares switched to consts.#[non_exhaustive] pub enum VerificationCode+ReportIssue::classification()so downstream security code (ordinaldbIntegrityErrormapping) branches on typed values, never strings. Serialized report JSON is byte-stable (regression-tested).sha256_bytes+ boundedsha256_readersharingsha256_file_bounded's EINTR/bounded core; private sqlite duplicate deduped onto the pub fn (F14 — brief-06 reimplemented this by hand, the drift risk this closes).Adversarial 3-lens review: clean (low-only findings). Tests: 95-108 passed across feature combos; clippy/fmt clean.
🤖 Generated with Claude Code
https://claude.ai/code/session_01BQ1JzAocstWiqtCF5J2V7K