feat: deterministic manifest schema v2 — the manifest hash is the content address#287
Open
Nelson Spence (Fieldnote-Echo) wants to merge 5 commits into
Open
feat: deterministic manifest schema v2 — the manifest hash is the content address#287Nelson Spence (Fieldnote-Echo) wants to merge 5 commits into
Nelson Spence (Fieldnote-Echo) wants to merge 5 commits into
Conversation
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.
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.
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.
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.
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.
Nelson Spence (Fieldnote-Echo)
requested review from
Navi Bot (project-navi-bot) and
Todd Baur (toadkicker)
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 QodoDeterministic manifest schema v2 (hash = content address) + cache migration
AI Description
Diagram
High-Level Assessment
Files changed (10)
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Code Review by Qodo
1. Backslash path bypass
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+4187
to
+4196
| fn is_manifest_path_absolute(path: &str) -> bool { | ||
| if path.starts_with('/') || path.starts_with('\\') { | ||
| return true; | ||
| } | ||
| let bytes = path.as_bytes(); | ||
| bytes.len() >= 3 | ||
| && bytes[0].is_ascii_alphabetic() | ||
| && bytes[1] == b':' | ||
| && (bytes[2] == b'/' || bytes[2] == b'\\') | ||
| } |
There was a problem hiding this comment.
1. Backslash path bypass 🐞 Bug ≡ Correctness
validate_manifest_shape() skips canonical-path validation when is_manifest_path_absolute() returns true, but is_manifest_path_absolute() treats any leading \ as absolute, letting non-canonical backslash paths bypass v2’s forward-slash-only enforcement. On Unix this can then be resolved as a relative path (because resolution uses Path::is_absolute()), allowing manifests with backslash-containing bundle-relative paths to verify successfully and violating the canonical/deterministic path contract.
Agent Prompt
## Issue description
`is_manifest_path_absolute()` currently returns true for any path starting with a single backslash (`\`). Because canonicality checks are skipped for “absolute” paths, this allows backslash-containing paths (non-canonical in v2) to bypass `*_path_not_canonical` validation.
## Issue Context
Verification path resolution later uses `Path::is_absolute()` (platform semantics), so on Unix a leading-backslash path is treated as *relative* and can still be resolved within `base_dir`, meaning the manifest can pass verification with a non-canonical (backslash) path.
## Fix Focus Areas
- ordvec-manifest/src/lib.rs[4169-4196]
- ordvec-manifest/src/lib.rs[353-385]
- ordvec-manifest/src/lib.rs[2403-2444]
### Concrete fix direction
- Update `is_manifest_path_absolute()` to only treat UNC/verbatim forms as absolute for backslashes (e.g., `path.starts_with("\\\\")`), not a single leading `\`.
- Consider using the same absolute-path classification consistently in resolution (or explicitly rejecting Windows/UNC-looking absolute strings on non-Windows when `allow_absolute_paths` is false) so policy and canonicality decisions can’t diverge across OSes.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
150
to
154
| CREATE TABLE IF NOT EXISTS active_manifest( | ||
| id INTEGER PRIMARY KEY CHECK(id = 1), | ||
| manifest_id TEXT NOT NULL, | ||
| manifest_path TEXT NOT NULL, | ||
| activated_at TEXT NOT NULL, | ||
| forced INTEGER NOT NULL |
There was a problem hiding this comment.
2. Active_manifest not migrated 🐞 Bug ☼ Reliability
init() migrates only verification_reports and then uses CREATE TABLE IF NOT EXISTS for active_manifest, so existing databases with the legacy active_manifest schema are not updated. activate() now inserts into active_manifest without manifest_id, which can fail against pre-existing DBs due to schema mismatch / legacy NOT NULL constraints.
Agent Prompt
## Issue description
SQLite upgrade logic drops `manifest_id` from `active_manifest`, but `init()` never migrates existing `active_manifest` tables—only `verification_reports` is migrated. Since `CREATE TABLE IF NOT EXISTS` does not alter existing tables, upgraded installs can break at runtime when `activate()` runs its new INSERT statement.
## Issue Context
After upgrading, older databases may still have an `active_manifest` table that includes a `manifest_id` column (and potentially a NOT NULL constraint). The new code inserts only `(id, manifest_path, activated_at, forced)`, so activation can fail on upgraded DBs.
## Fix Focus Areas
- ordvec-manifest/src/sqlite.rs[94-158]
- ordvec-manifest/src/sqlite.rs[44-92]
### Concrete fix direction
- Add a migration check for `active_manifest` similar to `verification_reports_needs_migration()`.
- If legacy columns are detected, migrate via `ALTER TABLE ... RENAME TO ...; CREATE TABLE ...; INSERT INTO new_table(...) SELECT ... FROM old_table; DROP old_table;`.
- Ensure migration runs before any `activate()` insert and is safe/idempotent.
ⓘ 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 U1 of the OrdinalDB post-mortem hardening (builder post-mortem F3; reconciled plan + implementation spec in the OrdinalDB planning docs; trust co-design: the deterministic manifest is the future Ed25519 seal surface — see ordinaldb docs/roadmap/ordinaldb-trust-spec.md).
Breaking schema change (v2, zero back-compat by decision — nothing published):
manifest_idandcreated_atremoved fromIndexManifest; creation writesbuild: None(no more per-write UUIDs/timestamps) → two writes of identical content produce byte-identical manifest.json, sosha256(manifest.json)is a stable content address.ordvec.index_manifest.v2; old-schema manifests fail with a clear "older/newer manifest schema" error (version probed on parse failure).(name, path)at creation; canonical bundle-relative forward-slash paths enforced at both create and verify (red-team fix: creation previously could embed paths its own verification rejects); golden byte fixture + determinism tests are the merge gates.VerificationReport.manifest_idand the sqlite registry's manifest_id column removed (forced fallout; cache identity already keyed on manifest_sha256).Adversarial 3-lens review pass done pre-PR: 5 medium findings found and fixed (create/verify canonicality asymmetry, non-escaping
..acceptance, Windows absolute-path contract split, stale README/examples, missing CHANGELOG). Tests: 108 passed (-p ordvec-manifest --all-features), root suite 256 passed; clippy/fmt clean; CI matrix extended to run manifest tests across OSes.Note:
uuiddep retained (still used by row-identity db_id validation).Stacked:
feat/typed-verification-codes(U2) builds on this branch.🤖 Generated with Claude Code
https://claude.ai/code/session_01BQ1JzAocstWiqtCF5J2V7K