clawhdf5/research/VERIFICATION_BRIEF.md

# Verification Brief — branch `verify/v3-plus-v6`

Independent audit of three already-implemented fixes:

- **P1** — `clawhdf5-ann::hnsw::compute_distance` now delegates to `clawhdf5-accel`'s
  runtime-dispatched SIMD kernels (`l2_distance`, `cosine_similarity`) instead of
  scalar loops.
- **P2** — `clawhdf5-io::async_read::AsyncFileReader` now opens the file handle
  once and caches it + its length behind a `tokio::sync::Mutex`.
- **PR1** — `clawhdf5-migrate` writes SHINES provenance (`hdf5_writer.rs`) and
  verifies it on read-back (`validate.rs`).

Branch state audited: `verify/v3-plus-v6` @ `07b7301` (merge of the v3 ann/io/migrate
work and v6 agent/format work). All three areas' existing test suites
(`cargo test -p clawhdf5-accel -p clawhdf5-ann -p clawhdf5-io --features async
-p clawhdf5-migrate --release`) pass — 41 + 23 + 89 + 26 tests green. That is
expected: the defect below is a numerical edge case none of the existing tests
exercise.

---

## P1 — SIMD distance in `clawhdf5-ann` — DEFECT FOUND

**File:** `crates/clawhdf5-accel/src/scalar.rs`, `avx2.rs`, `avx512.rs`, `neon.rs`
(all four backends share the bug identically; it surfaces in callers through
`crates/clawhdf5-ann/src/hnsw.rs:54`, `compute_distance`'s
`1.0 - clawhdf5_accel::cosine_similarity(a, b)`).

**Problem:** The near-zero-norm guard in `cosine_similarity` changed threshold
during the SIMD migration, and the new threshold is wrong.

Old scalar loop (pre-SIMD, `hnsw.rs` @ `55959b4`):

```rust
let denom = norm_a.sqrt() * norm_b.sqrt();
if denom < f32::EPSILON {
    1.0
} else {
    1.0 - (dot / denom)
}
```

New code, identical in all four `clawhdf5-accel` backends (e.g.
`scalar.rs:23-24`):

```rust
let denom = (norm_a * norm_b).sqrt();
if denom == 0.0 { 0.0 } else { dot / denom }
```

The old code clamped *any* near-zero denominator (anything under
`f32::EPSILON ≈ 1.19e-7`, not just exact zero) to a safe "maximally
dissimilar" result. The new code only special-cases an **exact** `0.0`
denominator; anything smaller but nonzero falls through to `dot / denom`.

For genuinely-zero vectors the two are equivalent (`denom == 0.0` in both, and
`1.0 - 0.0 == 1.0` matches the old `1.0`), and the existing test
(`hnsw.rs::cosine_zero_vector`, `clawhdf5-accel::test_cosine_zero_vector`)
only covers that case — which is why it didn't catch this.

But for vectors with a small (not exactly zero) norm, the two diverge sharply.
Concrete repro (values confirmed via a standalone build of both functions):

```
a = b = [1e-4]        // tiny but nonzero, identical vectors
old cosine distance = 1.0   // "unreliable direction" fallback, correctly
                             // caps degenerate near-zero vectors at max distance
new cosine distance = 0.0   // computed as fully identical
```

`denom` here is `1e-8`, comfortably below `f32::EPSILON` (`1.19e-7`) but not
`== 0.0`, so the old guard fired and the new one doesn't. This is not a
narrow floating-point-rounding footgun — the divergence spans roughly three
orders of magnitude of vector norm (anything with `denom` in
`(0, 1.19e-7)`), and it flips the result from "maximally dissimilar" to
"identical," the two opposite ends of the distance range. Any HNSW cosine
index that indexes or queries a near-zero-magnitude embedding (e.g. an
embedder's output for empty/masked/degenerate input, or a soft-deleted/
zeroed-out placeholder vector) will silently rank it as a near-duplicate of
other near-zero vectors instead of correctly pushing it to the bottom of
results.

Mismatched-length and truly-empty inputs were also checked: empty vectors
(`a.len() == b.len() == 0`) behave identically old vs. new (both hit the
zero-denominator path → distance `1.0`). Mismatched lengths now panic via
`assert_eq!` in every backend, versus the old code's `for i in 0..a.len()`
(which panicked on OOB if `b` was shorter, or silently truncated to `a`'s
length if `b` was longer). No caller reaches this: `HnswIndex::build_with_metric`
and `insert` both assert equal dimensions before any `compute_distance` call,
so mismatched lengths are unreachable in practice — not flagging as a
separate defect.

**Proposed fix:** Restore the epsilon-threshold guard in all four
`clawhdf5-accel` cosine_similarity backends (`scalar.rs`, `avx2.rs`,
`avx512.rs`, `neon.rs`), replacing `if denom == 0.0 { 0.0 }` with
`if denom < f32::EPSILON { 0.0 }`, so `1.0 - cosine_similarity(...)` in
`hnsw.rs` reproduces the old `denom < f32::EPSILON → 1.0` fallback exactly.
Add a regression test in `clawhdf5-accel` (e.g.
`test_cosine_near_zero_norm_clamped`) asserting `cosine_similarity(&[1e-4],
&[1e-4])` returns `0.0` (so `1.0 - sim == 1.0`, matching the old HNSW
fallback) rather than `1.0`, and a matching test in `hnsw.rs`
(`cosine_near_zero_vector`, alongside the existing `cosine_zero_vector`) using
a tiny-but-nonzero vector pair to lock in `compute_distance == 1.0`.

TASK: INT-01 — Restore f32::EPSILON near-zero-denom guard in clawhdf5-accel cosine_similarity (all 4 backends) + regression tests

---

## P2 — Cached async file handle in `clawhdf5-io` — SOUND, no defect

**File:** `crates/clawhdf5-io/src/async_read.rs`, `AsyncFileReader::read_at` /
`::len` (lines 96-126).

Checked against the pre-fix version (diff in `b08df7b`, which per-call opened
a fresh `tokio::fs::File` and re-stat'd the length):

- **No seek/read interleaving across tasks.** `read_at` takes
  `let mut guard = self.handle.lock().await` once at the top and then borrows
  `file` from that guard (`guard.as_mut()`) for the rest of the function,
  including both the `seek(...).await` and `read_exact(...).await` calls.
  Because `file` is a live borrow of `guard`, the Rust borrow checker forces
  `guard` (and therefore the lock) to stay held across both await points —
  it cannot be dropped until the whole function returns. `tokio::sync::Mutex`
  is specifically designed to be held across `.await` (unlike `std::sync::Mutex`),
  so a second task's `read_at` call blocks at `.lock().await` until the first
  task's seek+read pair has fully completed. A seek from one task can never be
  followed by a read from another task on the same descriptor.
- **Lazy-init race is also covered by the same lock.** The `if guard.is_none()`
  open-and-populate branch runs under the same guard acquired at the top, so
  two concurrent first-callers can't both open+overwrite the cached handle;
  the second one to acquire the lock sees `guard.is_some()` and reuses it.
- **Cached length staleness.** The length is cached forever once populated —
  intentional and documented in the struct's doc comment ("cached for the
  lifetime of this reader"). Grepped the whole workspace
  (`AsyncFileReader` outside `async_read.rs` itself): zero other callers exist
  yet, so there's no current code path where a caller observes a stale length
  against a file that changed size mid-lifetime. If the backing file were
  truncated externally during the reader's life, the stale (larger) cached
  length would make `read_at` attempt to read more than remains on disk —
  but that fails loudly via `read_exact`'s `UnexpectedEof` rather than
  silently returning corrupted/truncated data, which is a safe failure mode,
  not a correctness bug.
- **Short-read/truncation semantics.** The `offset >= file_len → empty`,
  `to_read = len.min(available)` logic is byte-for-byte unchanged from the
  pre-fix version; only the source of `file_len` changed (cached vs.
  freshly stat'd). For the current, only-consumer-is-itself usage pattern
  (open once, read many times, file not mutated externsally during the
  reader's life) the observable behavior is identical to before.

No item raised for P2.

---

## PR1 — SHINES provenance in `clawhdf5-migrate` — SOUND, no defect

**Files:** `crates/clawhdf5-migrate/src/hdf5_writer.rs`,
`crates/clawhdf5-migrate/src/main.rs`, `crates/clawhdf5-migrate/src/validate.rs`,
`crates/clawhdf5-migrate/src/hdf5_reader.rs`.

- **Current-run source path / timestamp on `--incremental` merges.**
  `write_hdf5` (`hdf5_writer.rs:23`) computes `timestamp = iso8601_now()`
  fresh on every call — it is never read from the merged `data` struct, so
  the top-level `migrated_at` attribute and the per-dataset
  `.with_provenance("clawhdf5-migrate", timestamp, source_opt)` calls
  (`hdf5_writer.rs:147,177,189`) always carry the current run's wall-clock
  time, incremental or not. For `source_path`: `hdf5_reader::read_hdf5`
  (used to load the incremental base) explicitly returns
  `source_path: String::new()` with a comment noting the caller must carry
  the real path forward (`hdf5_reader.rs:52-56`); `main.rs:160`
  (`base.source_path = source.source_path`) does exactly that — it
  overwrites the re-read base's placeholder with the *freshly re-read SQLite
  source's* path before calling `write_hdf5`, not a previous run's path.
  Traced through: on an `--incremental` run, both the top-level attributes
  and every per-dataset provenance attribute reflect the current run, not a
  stale one. `test_incremental_migration` (`main.rs`) exercises the merge
  path and passes, though it doesn't assert on `source_path`/`migrated_at`
  specifically — the coding phase could add that assertion as cheap
  extra insurance, but it's not fixing a defect, just tightening coverage.
- **Hash-mismatch vs. absent-attribute handling.**
  `verify_chunk_provenance` (`validate.rs:161-184`) returns `Err(...)`
  (fails loudly, wired through `validate_hdf5`'s `?`) only on
  `VerifyResult::Mismatch`, i.e. an actual recomputed-vs-stored SHA-256
  disagreement. `VerifyResult::NoHash` (attribute absent, e.g. an
  older output file) is handled separately — it sets `all_present = false`
  and continues, returning `Ok(false)` from `verify_chunk_provenance`
  (surfaced as `ValidationSummary::provenance_verified == false`, not an
  error). This is correctly asymmetric: real corruption is a hard error,
  merely-missing provenance metadata is a soft "unverified" signal, matching
  the documented contract in the function's doc comment.

No item raised for PR1.

---

## Summary

| Item | Verdict | Follow-up |
|------|---------|-----------|
| P1 SIMD distance | **Defect** — cosine near-zero-norm guard weakened from `< f32::EPSILON` to `== 0.0` across all 4 backends | INT-01 |
| P2 async file handle | Sound | none |
| PR1 migrate provenance | Sound | none |