research: add implementation brief with 17 numbered INT items

Covers performance, security, and provenance findings across clawhdf5-format, clawhdf5-migrate, and memory/query crates. Each item lists target file, problem, and proposed change for the coding phase.
2026-08-17 00:22:21 +00:00
parent b2dce41532
commit 122849b5a9
1 changed files with 345 additions and 0 deletions
@@ -0,0 +1,345 @@
 # Implementation Brief — Performance, Security & Provenance
 **Phase:** Research
 **Date:** 2026-08-17
 **Scope:** `clawhdf5` Rust workspace (`/mission/repo`)
 ## Method
 Read `ROADMAP.md`, `IMPROVEMENT_LOG.md`, `CLAUDE.md`, `CHANGELOG.md`, and recent
 `git log` before scoping this brief, to avoid re-proposing work already merged.
 The repo has already been through several hardening passes (Tier 1–4, see
 `CHANGELOG.md` "Unreleased" section and the `git log` entries tagged
 `security:`/`perf:`): bounds-check audits on `chunked_read.rs`/`data_read.rs`/
 `local_heap.rs`/`btree_v1.rs`, `MAX_DECOMPRESS_SIZE` output caps, WAL v2
 per-entry CRC32, Android JNI length validation, pyo3 bump, O(1) chunk-cache
 lookup with `Arc`-shared buffers, and optional rayon parallelism for HNSW
 `prune_connections`. None of that is re-proposed here.
 Four focused audits were run against the areas those passes did **not**
 cover: (1) the HDF5 binary parser files outside the already-audited set, plus
 `clawhdf5-accel`/`clawhdf5-gpu` unsafe code; (2) `clawhdf5-agent`'s
 query-time hot paths (search/rerank/consolidation/knowledge graph); (3) the
 provenance/anomaly-detection subsystem end-to-end; (4) error handling in
 `clawhdf5-io`, `clawhdf5-migrate`, `clawhdf5-py`, and the `clawhdf5` facade.
 `clawhdf5-accel` (SIMD dispatch), `clawhdf5-gpu` (no unsafe code, wgpu-mediated),
 `clawhdf5-io`, `clawhdf5-py`, and the `clawhdf5` facade crate were all found
 already sound for the failure modes investigated — no items proposed for
 those beyond what's listed below. Say so once here rather than padding the
 list with manufactured items.
 ---
 ## Section A — Parser crash safety (crafted-file DoS)
 These three files use raw `offset + N > file_data.len()` arithmetic instead
 of the `checked_add`-based `ensure_len` helper that every other parser in
 `clawhdf5-format` already uses (established pattern: `btree_v2.rs`,
 `global_heap.rs`, `fractal_heap.rs`, `shared_message.rs`, `local_heap.rs`'s
 own `ensure_len`, etc.). On a crafted file with an address field close to
 `u64::MAX`, the addition overflows — panicking in debug builds, silently
 wrapping in the release profile (no `overflow-checks` set anywhere in the
 workspace `Cargo.toml`), after which the bounds check passes falsely and the
 next slice operation panics anyway. Net effect either way: a crafted file
 crashes the parser instead of returning `Err`.
 ### INT-01 — `crates/clawhdf5-format/src/fixed_array.rs`, `crates/clawhdf5-format/src/extensible_array.rs`
 **Problem:** Six unguarded-addition bounds checks: `FixedArrayHeader::parse`
 (fixed_array.rs:69), the data-block header check in
 `read_fixed_array_chunks` (fixed_array.rs:129), `ExtensibleArrayHeader::parse`
 (extensible_array.rs:101), `read_extensible_array_data_block`
 (extensible_array.rs:278), the index-block parse (extensible_array.rs:429),
 and the super-block parse (extensible_array.rs:630). The offending offsets
 (`data_block_address`/`index_block_address`) come from `DataLayout::parse`
 (`data_layout.rs`, chunk_index_type 3/4 branches, ~lines 460–470), which only
 special-cases the exact all-`0xFF` sentinel via `is_undefined` — any other
 near-max value passes through unchanged.
 **Change:** Replace every raw `offset + N > file_data.len()` in both files
 with the `checked_add`-based `ensure_len` pattern already used elsewhere in
 the crate (e.g. mirror `local_heap.rs`'s `ensure_len`).
 ### INT-02 — `crates/clawhdf5-format/src/symbol_table.rs`
 **Problem:** `SymbolTableNode::parse` (line 83) uses raw
 `offset + 8 > file_data.len()`, unlike `read_offset` in the same file which
 already uses `checked_add`. `offset` is a SNOD address taken verbatim from a
 v1 B-tree leaf entry and passed straight through by `group_v1.rs:49` with no
 sentinel/range check — a crafted v1-group B-tree leaf with a near-`u64::MAX`
 child pointer overflows the check the same way as INT-01.
 **Change:** Use `offset.checked_add(8)` (`ensure_len` pattern) at line 83.
 Note: the `entries_start + num_symbols * entry_size` addition at line 106 has
 the same raw-arithmetic style, but `num_symbols` is `u16` so the multiply
 itself can't overflow — lower priority, but worth fixing for consistency in
 the same pass.
 ### INT-03 — `crates/clawhdf5-format/src/datatype.rs`
 **Problem:** `Datatype::parse` recurses into itself with no depth counter
 (`grep -n "depth" datatype.rs` — zero hits) for Compound members (lines 361,
 387), Enumeration base type (line 418), VariableLength base type (line 471),
 and Array base type (lines 497, 518). A message data size is capped at
 `u16::MAX` (65535 bytes; see `object_header.rs:141` v1, `object_header.rs:411`
 v2), so a crafted Compound-of-Compound-of-Compound... datatype message can
 nest ~8000 levels deep — enough to blow the stack, and materially worse on
 the project's documented no_std/embedded targets (`thumbv7em-none-eabihf`,
 per `CHANGELOG.md`) where available stack is a few KB. The changelog records
 this exact class of bug already fixed for the N-Bit filter's type tree, but
 that fix was never applied to the general `Datatype::parse` reader used for
 every Dataspace/Attribute/Dataset datatype message.
 **Change:** Thread a `depth: u16` counter through `Datatype::parse`'s
 recursive call sites (mirror `object_header.rs`'s continuation-depth guards)
 and return a new `FormatError::NestingDepthExceeded` past a fixed limit
 (suggest 64).
 ---
 ## Section B — Provenance & anomaly detection
 The most significant finding of this brief: **the provenance/anomaly
 subsystem exists and is tested, but is never invoked from the real save/load
 path.** It's a fully-built, unused API surface, not an active control.
 ### INT-04 — `crates/clawhdf5-agent/src/provenance.rs`, `crates/clawhdf5-agent/src/anomaly.rs`, `crates/clawhdf5-agent/src/lib.rs`
 **Problem:** `ProvenanceStore`, `MemoryProvenance::new`, `verify_integrity`,
 `mark_verified`, `WriteAnomalyDetector`, `record_write`,
 `check_pattern_anomaly`, `check_rate_anomaly`, `check_source_anomaly` have
 zero callers outside their own module/tests. `lib.rs` only declares
 `pub mod provenance;` / `pub mod anomaly;` (lines 22, 33) — neither is
 referenced from `HDF5Memory::save_or_update` (~line 495) or the WAL replay
 path (`wal.rs::replay_into_cache`, line 311). Concretely: the 15
 injection-pattern checks, rate limiting, and content-hash integrity
 verification described as shipped in `ROADMAP.md` Track 5 never execute
 during normal library usage today.
 **Change:** Call `ProvenanceStore::add` and
 `WriteAnomalyDetector::record_write` + the `check_*` methods from
 `HDF5Memory::save_or_update`, and call `verify_integrity` from the
 open/load path (surfacing a mismatch to the caller, not panicking). If the
 intent is genuinely opt-in-only, that's a legitimate design choice, but it
 must be documented prominently at the crate root / in `CLAUDE.md` — right
 now it reads as an active control and isn't one.
 ### INT-05 — `crates/clawhdf5-agent/src/lib.rs` (`MemoryEntry.source_channel`, ~line 167), `crates/clawhdf5-agent/src/consolidation.rs` (`ConsolidationEngine::add_memory`, ~line 205)
 **Problem:** `source_channel: String` is free text set entirely by the
 caller of `save`/`save_or_update` — nothing validates it against an
 allowlist, so a write can claim `source_channel = "system"` or any other
 privileged-looking label. Separately, `add_memory` takes `source:
 MemorySource` (User/System/Tool/Retrieval/Correction) as a plain parameter;
 `MemorySource::Correction`/`System` get elevated importance weighting in
 `score_correction` (~line 133), so any caller can claim a trust level the
 content doesn't warrant.
 **Change:** Derive `MemorySource`/`source_channel` at the actual trust
 boundary (the ingestion layer that knows the true origin), not as a
 caller-supplied argument to the storage API. At minimum, gate
 `MemorySource::System`/`Correction` construction behind a distinct
 constructor not exposed to the same call path as untrusted content.
 ### INT-06 — `crates/clawhdf5-agent/src/anomaly.rs` (`check_pattern_anomaly`, ~lines 192–195)
 **Problem:** Matching is `chunk.to_lowercase().contains(pattern.as_str())` —
 plain literal-substring test after case folding only. Inserting any
 character inside a pattern (extra whitespace, a zero-width character, `.`
 between letters) or substituting a homoglyph for one Latin letter defeats
 every one of the 15 injection patterns; there's no Unicode
 confusable-normalization or punctuation/whitespace stripping.
 **Change:** Normalize input before matching (strip zero-width characters and
 punctuation, apply NFKC + confusable-folding) or switch to fuzzy/token-based
 detection instead of raw `contains`.
 ### INT-07 — `crates/clawhdf5-agent/src/anomaly.rs` (`check_rate_anomaly`, ~lines 149–151)
 **Problem:** The per-minute rate check uses a single global sliding window
 (`self.window.len()`) across all sessions/sources combined. One noisy
 session can trip the shared window without the alert naming the offending
 session (unlike the separate cumulative `max_writes_per_session` check,
 which does name it); conversely, many distinct low-volume sessions can
 jointly flood the shared window without any individual one tripping its own
 per-session limit.
 **Change:** Key the sliding window by session/source (or add a per-source
 rolling count) so the rate check attributes to, and can throttle, the actual
 offender.
 ### INT-08 — `crates/clawhdf5-format/src/provenance.rs` (`verify_dataset`, ~line 126)
 **Problem:** The SHA-256 content hash is written automatically on save when
 `db.provenance` is set (`file_writer.rs` ~1061–1068, gated on the
 `provenance` feature), but `verify_dataset` is only ever called from test
 files — no reader/open path in `clawhdf5-io` or the `clawhdf5` facade calls
 it. A corrupted dataset is silently readable with no automatic integrity
 check; the write-side machinery exists but nothing consumes it. (Note:
 `CHANGELOG.md` already documents that this hash is unkeyed/tamper-*evident*
 not tamper-*proof* — that's accepted and not re-flagged here; this item is
 about it never being invoked at all, not about its cryptographic strength.)
 **Change:** Optionally call `verify_dataset` on dataset open (behind the
 `provenance` feature) and surface a mismatch as a typed error/warning to the
 caller instead of leaving verification purely opt-in/manual.
 ### INT-09 — `crates/clawhdf5-agent/src/wal.rs` (`WalFile::read_entries`, ~lines 219–272)
 **Problem:** Two related gaps. (a) WAL v2's per-entry CRC32 covers only each
 entry's own bytes — there's no sequence number or entry-chaining, so entries
 could be reordered, duplicated, or spliced (e.g. a `Tombstone` moved
 before/after its target `Save`) while every individual entry still passes
 its own CRC check, silently changing replayed cache state. (b) The
 `WAL_VERSION_LEGACY_NO_CRC` branch (~lines 260–266) does no CRC verification
 at all, and the version byte itself is a single unauthenticated byte — since
 `read_entries` is a public standalone API (not just reached via `open()`'s
 one-time migrate-on-read), flipping that byte from `2` to `1` silently
 downgrades every subsequent entry in the file to the fully-unverified
 pre-hardening parser.
 **Change:** Add a monotonic sequence number or entry-chaining (CRC/hash
 including the previous entry's CRC) to detect reordering/splicing. Restrict
 the legacy-no-CRC branch to the `open()` migration path only, or emit a
 warning when `read_entries` falls back to it via any other entry point.
 ---
 ## Section C — Correctness bug (panic on valid, untrusted input)
 ### INT-10 — `crates/clawhdf5-migrate/src/validate.rs` (`truncate`, lines 143–149)
 **Problem:**
 ```rust
 fn truncate(s: &str) -> String {
    if s.len() <= 40 {
        s.to_string()
    } else {
        format!("{}…", &s[..40])   // byte-index slice, not char-boundary safe
    }
 }
 ```
 `s` is `source.chunk` — arbitrary UTF-8 text read from the source SQLite
 database, called from the chunk-text mismatch branch of `validate_hdf5`
 (~line 58) whenever migrated text doesn't exactly match the source. This is
 the default (non-`--dry-run`) validation path, not test-only code — the file
 has no `#[cfg(test)]` block. If a multi-byte character (emoji, accented
 letter, CJK, etc.) straddles byte offset 40, `&s[..40]` panics with "byte
 index 40 is not a char boundary" instead of producing the diagnostic the
 code exists to report.
 **Change:** Truncate on a char boundary, e.g.
 `let cut = s.char_indices().nth(40).map(|(i, _)| i).unwrap_or(s.len()); format!("{}…", &s[..cut])`.
 ---
 ## Section D — Performance (query-time hot paths, `clawhdf5-agent`)
 `search.rs`, `vector_search.rs`, `hybrid.rs`, `reranker.rs`, `confidence.rs`,
 `temporal.rs`, `ivf.rs`, `pq.rs`, and `gpu_search.rs` were reviewed and found
 already efficient (temporal index uses `partition_point` binary search,
 hybrid merge uses `HashMap` accumulation not nested loops, no gratuitous
 clones in the batch vector paths) — no items proposed there.
 ### INT-11 — `crates/clawhdf5-agent/src/bm25.rs` (`BM25Index::search`, ~lines 118–141)
 **Problem:** The WAND top-k threshold update calls
 `top_k_scores.sort_by(...)` over the full `k`-sized buffer for every matching
 document that beats the running threshold (twice in the `>= k` branch), plus
 another full sort on reaching exactly `k` results. For `m` matching
 documents this is `O(m·k log k)` where a heap gives `O(m log k)`.
 **Change:** Replace `top_k_scores: Vec<f32>` with a min-heap
 (`BinaryHeap<Reverse<f32>>`) of size `k`; pop/push instead of sort-and-index.
 ### INT-12 — `crates/clawhdf5-agent/src/knowledge.rs` (`KnowledgeCache::resolve_or_create`, lines 304–330)
 **Problem:** `self.entities.iter().map(|e| levenshtein(&lower_name,
 &e.name.to_lowercase()))` allocates a fresh lowercased `String` for every
 entity on every resolution call (this runs per extracted mention during
 entity/relation extraction) and never short-circuits even on an exact
 `dist == 0` match — it scores every remaining entity regardless.
 **Change:** Cache a lowercased name on `Entity` to avoid the
 per-call allocation, and break out of the scan as soon as a `dist == 0`
 match is found.
 ### INT-13 — `crates/clawhdf5-agent/src/knowledge.rs` (`bfs_neighbors` lines 339–378, `spreading_activation` lines 435–495, `get_relations_from`/`get_relations_to` lines 247–254)
 **Problem:** All four functions filter/scan the *entire* `self.relations`
 list per node processed (`O(V·E)` for BFS instead of `O(V+E)`;
 `O(max_steps · active_nodes · relations)` for spreading activation), and
 `bfs_neighbors` additionally calls `self.get_entity(neighbour_id)` per
 discovered neighbor, itself an `O(n)` linear `.find()` over `self.entities`.
 **Change:** Build (or maintain incrementally on `add_entity`/`add_relation`)
 a `HashMap<u64, Vec<usize>>` adjacency index and a `HashMap<u64, usize>`
 id→index map, shared across all four functions, replacing the linear scans
 with O(1)/O(degree) lookups.
 ### INT-14 — `crates/clawhdf5-agent/src/consolidation.rs` (`ConsolidationEngine::add_memory`, lines 212–217)
 **Problem:**
 ```rust
 let working: Vec<MemoryRecord> = self.records.iter()
    .filter(|r| r.tier == MemoryTier::Working)
    .cloned()
    .collect();
 ```
 `score_surprise` (the only consumer) only reads `r.embedding` by reference —
 the full clone (chunk text + embedding `Vec<f32>`) of every working-tier
 record is discarded immediately after use.
 **Change:** Collect `Vec<&MemoryRecord>` (or iterate the filtered
 `self.records` directly, passing an iterator of `&[f32]`) instead of
 `.cloned()`.
 ### INT-15 — `crates/clawhdf5-agent/src/consolidation.rs` (`consolidate`, lines 284–291 and 345–351)
 **Problem:** `self.records.retain(|r| !evict_ids.contains(&r.id))` where
 `evict_ids: Vec<u64>` — `retain` calls `.contains()` (linear scan) for every
 record in `self.records`, giving `O(n·m)` cost (n = records, m = eviction
 count) on both the Working-tier eviction (line 289) and Episodic-tier
 eviction (line 350), on every consolidation tick.
 **Change:** Build `evict_ids` as a `HashSet<u64>` for O(1) membership checks.
 ### INT-16 — `crates/clawhdf5-agent/src/blas_search.rs` (`blas_cosine_batch`, lines 30–39), `crates/clawhdf5-agent/src/accelerate_search.rs` (`accelerate_cosine_batch_vecs`, lines 164–173)
 **Problem:** `cache.embeddings` is stored as `Vec<Vec<f32>>`; both functions
 re-flatten the entire corpus into a fresh `Vec<f32>`
 (`flat.extend_from_slice(&vectors[i])` per non-tombstoned vector) on *every
 single query* before running the actual BLAS/Accelerate matmul — an
 `O(N·dim)` copy paid per query when the `fast-math` feature is enabled. The
 fix pattern already exists in-file: `blas_cosine_batch_flat` (same file,
 lines 89–142) has an `all_active` fast path that skips this copy when
 reading from a pre-flattened buffer directly — it's just not used for the
 `Vec<Vec<f32>>` call sites.
 **Change:** Maintain a persistent flat embedding buffer alongside
 `cache.embeddings` (updated incrementally on insert/delete) and call
 `blas_cosine_batch_flat` instead of `blas_cosine_batch` from both files'
 query paths.
 ### INT-17 — `crates/clawhdf5-agent/src/entity_extract.rs` (`dedup_overlapping`, lines 302–313)
 **Problem:** `result.iter().any(|existing| ...)` checks every candidate
 entity against all already-accepted entities — `O(n²)` in
 entities-per-extraction-call. This runs at ingestion time (every memory
 save), not query time, and is bounded by entities-per-chunk (typically
 small), so it's lower priority than INT-11 through INT-16.
 **Change:** If profiling shows this matters in practice (large chunks with
 many extracted entities), replace with a spatial/interval-based overlap
 index; otherwise leave as-is — flagging for completeness, not urgency.
 ---
 ## Summary table
 | INT | Area | File(s) | Category |
 |-----|------|---------|----------|
 | INT-01 | Parser crash safety | `fixed_array.rs`, `extensible_array.rs` | Security |
 | INT-02 | Parser crash safety | `symbol_table.rs` | Security |
 | INT-03 | Parser crash safety | `datatype.rs` | Security |
 | INT-04 | Provenance wiring | `provenance.rs`, `anomaly.rs`, `lib.rs` | Provenance |
 | INT-05 | Source trust boundary | `lib.rs`, `consolidation.rs` | Provenance |
 | INT-06 | Anomaly pattern bypass | `anomaly.rs` | Provenance |
 | INT-07 | Rate-limit attribution | `anomaly.rs` | Provenance |
 | INT-08 | Integrity verification unwired | `clawhdf5-format/provenance.rs` | Provenance |
 | INT-09 | WAL ordering/legacy fallback | `wal.rs` | Provenance |
 | INT-10 | Char-boundary panic | `clawhdf5-migrate/validate.rs` | Correctness |
 | INT-11 | WAND top-k re-sort | `bm25.rs` | Performance |
 | INT-12 | Entity resolution scan | `knowledge.rs` | Performance |
 | INT-13 | Graph traversal scan | `knowledge.rs` | Performance |
 | INT-14 | Unneeded clone | `consolidation.rs` | Performance |
 | INT-15 | O(n·m) eviction | `consolidation.rs` | Performance |
 | INT-16 | Per-query re-flatten | `blas_search.rs`, `accelerate_search.rs` | Performance |
 | INT-17 | O(n²) dedup (low priority) | `entity_extract.rs` | Performance |
 ## Follow-ups for the coding phase
 TASK: INT-01 — Fix unchecked-overflow bounds checks in fixed_array.rs/extensible_array.rs
 TASK: INT-02 — Fix unchecked-overflow bounds check in symbol_table.rs
 TASK: INT-03 — Add recursion-depth guard to Datatype::parse
 TASK: INT-04 — Wire provenance.rs/anomaly.rs into save/load path
 TASK: INT-05 — Enforce source-of-truth for MemorySource/source_channel at trust boundary
 TASK: INT-06 — Harden anomaly pattern matching against whitespace/homoglyph bypass
 TASK: INT-07 — Make anomaly rate-limit window per-source
 TASK: INT-08 — Wire clawhdf5-format provenance verify_dataset into read path
 TASK: INT-09 — Add WAL entry ordering protection and restrict legacy no-CRC fallback
 TASK: INT-10 — Fix byte-index slice panic in clawhdf5-migrate validate.rs truncate()
 TASK: INT-11 — Replace BM25 top-k re-sort with a min-heap
 TASK: INT-12 — Cache lowercased entity names and early-exit in resolve_or_create
 TASK: INT-13 — Add adjacency index for knowledge graph traversal functions
 TASK: INT-14 — Avoid cloning working-tier records in consolidation add_memory
 TASK: INT-15 — Use HashSet for eviction ID membership checks in consolidation
 TASK: INT-16 — Use persistent flat embedding buffer in blas_search/accelerate_search
 TASK: INT-17 — (optional/low-priority) revisit entity_extract dedup_overlapping if profiling shows it matters