fix(manifest): harden v0.6.0 canonical writes and MSRV

[Fieldnote-Echo]

Stacked on #297 so every review surface consistently targets the corrected 0.6.0 train. Retarget to main after #297 merges.

What changed

• write manifests to a synced same-directory temporary file, then atomically replace the destination
• preserve File::create mode/umask behavior for new Unix files and portable permissions for existing files
• reject symlink and other non-regular destinations before replacement
• reject non-finite distortion values before touching an existing manifest
• recursively sort nested extension/attestation maps before serialization
• normalize exactly representable JSON numbers and fail closed on precision that would alias through f64
• reject non-UTF-8 artifact paths instead of embedding a lossy replacement character
• restore rusqlite 0.39 because the 0.40 build dependency does not compile on the declared Rust 1.89 floor
• run the manifest all-features suite permanently in the MSRV lane

Why

The manifest bytes are the bundle's content address. In-place truncation, feature-dependent JSON ordering, lossy paths, silent NaN-to-null conversion, and precision collisions all weaken that identity contract. Separately, the dependency bot's SQLite bump silently broke ordvec-manifest --all-features on the documented MSRV.

The numeric path deliberately rejects values outside the exact canonical i64/u64/f64 domain instead of collapsing distinct arbitrary-precision inputs to the same bytes.

Validation

• adversarial independent review: SHIP after collision and permission-contract fixes
• stable ordvec-manifest --all-features: 121 passed
• Rust 1.89 ordvec-manifest --all-features --locked: 121 passed
• no-default-features: 101 passed
• clippy all-targets/all-features with -D warnings
• release publish and signed-release invariant suites
• cargo deny, fmt, and diff checks

Closes #294.
Closes #295.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

PR Summary by Qodo

Harden ordvec-manifest v0.6.0 canonical writes and restore MSRV compatibility

🐞 Bug fix 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Canonicalize manifest JSON deterministically (sorted nested maps, normalized numbers, UTF-8 paths
 only).
• Write manifests via synced same-directory tempfiles and atomic replacement; reject
 symlinks/non-regular targets.
• Restore rusqlite 0.39 for Rust 1.89 MSRV and run all-features manifest tests in MSRV CI.

Diagram

graph TD
  A["Callers (CLI/tests)"] --> B["write_manifest_file"] --> C["Validate & canonicalize"] --> D["Tempfile (same dir)"] --> E["fsync + persist (atomic replace)"] --> F["Sync parent dir"] --> G["Manifest path on disk"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Adopt a standard canonical-JSON implementation (e.g., JCS RFC 8785)

• ➕ Delegates tricky number/string canonicalization rules to a well-specified standard
• ➕ Potentially easier to interoperate with non-Rust tooling
• ➖ May not match existing ordvec-manifest schema/pretty-printing expectations without migration
• ➖ Still needs explicit policy for rejecting non-finite/aliased numbers and non-UTF-8 paths

2. Stream canonicalization at serialization time (custom Serializer)

• ➕ Avoids cloning the full manifest into a canonical copy
• ➕ Centralizes ordering/number normalization logic in one output path
• ➖ More complex to implement and audit than mutating a cloned Value tree
• ➖ Harder to unit test in isolation; easy to miss edge cases

3. Use a dedicated atomic-write helper crate (e.g., atomicwrites)

• ➕ Reduces bespoke filesystem edge-case handling
• ➕ Potentially clearer portability semantics
• ➖ Less control over permission/umask contract and destination-type checks
• ➖ Another dependency in a security-sensitive write path

Recommendation: The PR’s approach is strong for the stated contract: it fails closed before touching existing manifests, explicitly rejects symlinks/non-regular targets, and makes bytes independent of downstream serde_json feature unification. Given the content-addressing sensitivity, keeping the canonicalization and atomic-write policy in-tree (with the new adversarial tests) is preferable to swapping in a generic helper unless the project explicitly wants to adopt a cross-language canonical-JSON standard (which would likely require a schema-version/migration decision).

Files changed (8) +508 / -48

Bug fix (1) +253 / -31

lib.rsImplement atomic manifest writes and feature-independent canonical JSON serialization +253/-31

Implement atomic manifest writes and feature-independent canonical JSON serialization

• Reworks 'write_manifest_file' to pre-validate distortion values, canonicalize nested JSON objects, normalize JSON numbers, and reject non-UTF-8 paths. Writes via a same-directory tempfile with fsync + atomic persist, preserves portable permissions, rejects symlink/non-regular targets, and syncs the parent directory on Unix for durability.

ordvec-manifest/src/lib.rs

Tests (2) +217 / -5

deterministic.rsAdd determinism, atomicity, symlink-safety, and UTF-8 path regression tests +187/-1

Add determinism, atomicity, symlink-safety, and UTF-8 path regression tests

• Adds tests ensuring nested JSON key order and numeric spelling do not affect manifest bytes, and that arbitrary-precision numbers that would alias via f64 are rejected. Verifies atomic replacement semantics via inode hard-links, checks mode/umask behavior and permission preservation on Unix, rejects symlink destinations, and adds a Linux-only non-UTF-8 path rejection test.

ordvec-manifest/tests/deterministic.rs

manifest.rsEnsure non-finite distortion values fail before overwriting existing manifests +30/-4

Ensure non-finite distortion values fail before overwriting existing manifests

• Introduces a regression test confirming that a NaN distortion value causes 'write_manifest_file' to error without replacing/truncating an existing destination file.

ordvec-manifest/tests/manifest.rs

Documentation (2) +25 / -0

CHANGELOG.mdDocument canonicalization hardening, atomic writes, and MSRV fix +18/-0

Document canonicalization hardening, atomic writes, and MSRV fix

• Adds a 0.6.0 Fixed section documenting deterministic serialization (nested map sorting + number normalization), atomic writes with permission preservation, and the rusqlite downgrade to maintain Rust 1.89 compatibility.

CHANGELOG.md

README.mdClarify canonical content-address contract and atomic write semantics +7/-0

Clarify canonical content-address contract and atomic write semantics

• Documents that nested extensions/attestations are recursively canonicalized independent of serde_json features. Also explains atomic write behavior, permission portability guarantees, and rejection of symlink/non-regular destinations.

ordvec-manifest/README.md

Other (3) +13 / -12

ci.ymlRun ordvec-manifest all-features tests in CI +2/-0

Run ordvec-manifest all-features tests in CI

• Adds a dedicated CI step to run 'cargo test -p ordvec-manifest --all-features' in the MSRV lane. This prevents feature-gated regressions (including optional SQLite support) from silently breaking the declared Rust floor.

.github/workflows/ci.yml

Cargo.lockDowngrade rusqlite dependency graph to 0.39 line +8/-10

Downgrade rusqlite dependency graph to 0.39 line

• Updates the lockfile to reflect 'rusqlite' 0.39 and matching transitive versions (e.g., libsqlite3-sys/hashlink). Also reflects serde_json dependency graph adjustments required by test feature coverage.

Cargo.lock

Cargo.tomlRestore rusqlite 0.39; move tempfile to runtime deps; harden serde_json tests +3/-2

Restore rusqlite 0.39; move tempfile to runtime deps; harden serde_json tests

• Pins optional 'rusqlite' back to 0.39 for MSRV compatibility. Promotes 'tempfile' to a regular dependency for atomic writes, and configures dev-dependency 'serde_json' with 'arbitrary_precision' + 'preserve_order' to ensure canonicalization is feature-unification-proof.

ordvec-manifest/Cargo.toml

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e4ed0e2dd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Racy destination type check ✓ Resolved 🐞 Bug ☼ Reliability

Description

write_manifest_file() validates the destination with symlink_metadata() and later replaces it via
tempfile::NamedTempFile::persist(), leaving a TOCTOU window where the destination can change type
between the check and the rename. Under concurrent modification of the parent directory, this can
violate the documented guarantee that symlink/non-regular destinations are rejected (even though
rename typically replaces the symlink itself rather than following it).

Code

ordvec-manifest/src/lib.rs[R4461-4498]

+    let inherited_permissions = match fs::symlink_metadata(path) {
+        Ok(metadata) if metadata.file_type().is_file() => Some(metadata.permissions()),
+        Ok(_) => {
+            return Err(ManifestError::invalid(format!(
+                "manifest destination {} exists and is not a regular file",
+                path.display()
+            )))
+        }
+        Err(error) if error.kind() == io::ErrorKind::NotFound => None,
+        Err(error) => return Err(ManifestError::Io(error)),
+    };
+
+    let mut builder = tempfile::Builder::new();
+    builder.prefix(".ordvec-manifest-").suffix(".tmp");
+    #[cfg(unix)]
+    {
+        use std::os::unix::fs::PermissionsExt;
+
+        builder.permissions(
+            inherited_permissions
+                .clone()
+                .unwrap_or_else(|| fs::Permissions::from_mode(0o666)),
+        );
+    }
+    #[cfg(not(unix))]
+    if let Some(permissions) = inherited_permissions.as_ref() {
+        builder.permissions(permissions.clone());
+    }
+    let mut temporary = builder.tempfile_in(parent)?;
+    serde_json::to_writer_pretty(temporary.as_file_mut(), &canonical)?;
+    temporary.as_file_mut().flush()?;
+    if let Some(permissions) = inherited_permissions {
+        temporary.as_file().set_permissions(permissions)?;
+    }
+    temporary.as_file().sync_all()?;
+    temporary
+        .persist(path)
+        .map_err(|error| ManifestError::Io(error.error))?;

Relevance

⭐⭐⭐ High

Team repeatedly accepted TOCTOU/race hardening in ordvec-manifest (verify_for_load, create/hash
snapshots, sqlite cache scoping).

PR-#163
PR-#178
PR-#283

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The implementation performs a one-time destination type check and then later persists (renames) the
temp file to the same path without re-validating the destination entry, so destination type can
change between validation and replacement.

ordvec-manifest/src/lib.rs[4438-4500]
PR-#283

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`write_manifest_file()` rejects non-regular destinations based on a single early `symlink_metadata()` check, but the actual replacement happens later via `temporary.persist(path)`. If another process changes `path` after the check (e.g., swaps in a symlink), the rename can proceed and replace that new entry, breaking the stated “reject symlink/non-regular destination” contract under races.

### Issue Context
This is a classic check-then-use gap on a pathname in a mutable directory. While a fully race-free solution is OS-specific, you can still (a) narrow the window and (b) ensure the documentation matches the strongest guarantee you can actually enforce.

### Fix Focus Areas
- ordvec-manifest/src/lib.rs[4461-4498]

### Suggested remediation
1. Re-run `fs::symlink_metadata(path)` immediately before `temporary.persist(path)` and reject if the destination exists and is not a regular file.
2. If you want a stronger guarantee, consider an OS-specific implementation (e.g., directory-fd + `openat`/`renameat`-style flows) or adjust the docs to explicitly scope the guarantee to the non-adversarial/no-concurrent-writer case.
3. Add a targeted regression test that simulates the race as far as practical (e.g., create file, pass check, swap to symlink just before persist using a hook/abstraction or by splitting the function for testability).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8cab2323d0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 285a4bbb5f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "Codex (@codex) review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "Codex (@codex) address that feedback".