tidaldb/docs/reviews/M0-M10-code-review-2026-06-08-pass2.md

# tidalDB — M0–M10 Seven-Dimension Code Review (fresh pass · 2026-06-08)

> **Scope:** the four database crates at `git HEAD` — `tidal/` (84.3k LOC), `tidal-net/` (1.9k), `tidal-server/` (5.4k), `tidalctl/` (1.4k) ≈ **93k LOC / 253 source files**, carved into **29 review zones**.
> The `applications/` consumers (`forage`, `iknowyou`) are companion products, **not** part of the M0–M10 database milestones, and were **excluded** by design.
> **Method:** the `code-reviewer` seven-dimension protocol (Completeness · Accuracy · Tech Debt · Maintainability · Extensibility · DRY · CLEAN) run as 29 parallel Opus zone reviewers, each applying all seven passes plus the Rust red-flag table and `CODING_GUIDELINES.md`, at the correctness-first "trust it at 3am" standard. **Every** BLOCKER/CRITICAL/WARNING was then handed to an independent **default-refute verifier** (Sonnet) that re-read the live source and tried to refute it; only survivors are reported. 85 agents, ~5.4M tokens.

This is a **post-remediation re-review**. Three remediation passes already landed (see `docs/reviews/M0-M10-code-review-2026-06-07.md`, `-2026-06-08.md`, and `M0-M10-seven-dimension-review.md`). Everything below was found in the **current committed code** and survived adversarial verification; refuted items are listed for transparency. The two BLOCKERs were additionally re-confirmed by the orchestrator reading the cited source directly.

---

## Remediation status: RESOLVED — all 91 actionable findings fixed (2026-06-09)

Every actionable finding — **2 BLOCKER · 17 CRITICAL · 32 WARNING · 3 verifier-downgraded SUGGESTION · 37 standalone SUGGESTION** — was remediated at root cause. The 2 refuted findings and 58 PRAISE items needed no action. Highlights:

- **Both BLOCKERs (session restore):** `restore_sessions()` now advances `next_session_id` past every durable snapshot/start id (no more archived-snapshot overwrite), and the durable `started_at_ns` wall clock drives both TTL-enforcement paths (sweeper + `PolicyEvaluator::check`) plus a back-dated monotonic `started_at`, so a session's age — and its TTL bound — survives a restart. Closed sessions can no longer restore as ACTIVE (storage-snapshot probe before active restore), orphaned start records are sealed into archived snapshots, and a `SessionState::new` constructor removes the dual-construction-site drift that let the bug hide.
- **CRITICAL durability/correctness:** backup now fsyncs files + dirs; the governance contributor checkpoint fsyncs per accept; USearch HNSW is wired onto the live path (size-gated, no config plumbing); cohort rescore honors the profile sort; the notification cap records only the delivered page **and** is idempotent per `(user, entity, day)`; Stage-2/creator-search id narrowing is checked; replication `finalize`/`enter_dual_write` are durable-before-observable with targeted rollback; the segment receiver and `recv_segment`/`server_terminated` surface a dead serve loop; `GrpcTransport::new` validates before building the channel; the cluster router gained timeout + concurrency limits with a bounded scatter-gather pool; and `recover()` no longer silently truncates a corrupt non-final WAL segment.
- **WARNINGs/SUGGESTIONs:** poison-recovery on the cohort cache; `is_dir`/writability probes in the builder; checked id narrowing everywhere; deterministic RRF + fusion tie-breaks; the ranking query-clock threaded through `read_*_at`; LWW merge made commutative (value tie-break); `encode_session_event` made fallible against >64 KiB fields; macOS `F_FULLFSYNC`; storage-zone tracing; and the dead `BatchConflict`/`SegmentFull`/`tenant_wal_dir`/`rustls-pemfile` removed or honestly documented.

One genuine regression was caught and corrected during verification: the "sort replaces boosts" (spec §11.9) fix was initially applied as a blanket skip, which silently flattened the **complementary** engagement boosts on `for_you`/`following`/`related`/`notification`. It was replaced with precise per-boost **subsumption** — only a boost whose `(signal, agg, window)` the active formula sort already reads (the `trending`/`cohort_trending` velocity boosts, kept as the cohort-rescore definition) is skipped.

**Verification (real, not wiring):** `cargo clippy -D warnings` clean across `tidaldb` + `tidal-net` + `tidal-server` + `tidalctl` (all targets, `test-utils`); **tidaldb 1997 tests / 31 binaries, tidal-server 67 / 8, tidal-net 59, tidalctl 41 — 0 failures.** ~100 regression tests were added alongside the fixes (each finding's prescribed test). Uncommitted.

---

## Verdict: REQUEST_CHANGES

The engine is in materially better shape than the prior passes — the dominant "silently-wrong-query-results" class is much reduced, durability discipline (atomic `WriteBatch`, fsync of WAL + parent dirs, recovery rebuilds) is real and consistently applied across the storage/WAL core, and the code is genuinely well-tested (1583+ lib tests, crash-property suites). But **two BLOCKERs in the session-restore path destroy or silently void durable state on a clean restart**, and the CRITICAL list is dominated by *silent* failure modes — wrong results with no error, background threads that die without a signal, and a production engine (USearch HNSW) wired into nothing. None are acceptable to ship.

| Severity | Raw raised | After verification |
|----------|-----------:|-------------------:|
| **BLOCKER** | 2 | **2** |
| **CRITICAL** | 17 | **17** |
| **WARNING** | 32 | **32** |
| SUGGESTION (verifier-downgraded from WARNING) | — | 3 |
| Standalone SUGGESTION | 37 | 37 |
| PRAISE | 58 | 58 |
| *Refuted / false-positive* | 2 | — |

56 non-trivial findings were raised; **54 survived** adversarial verification (2 refuted, 3 severity-downgraded to suggestion). Verifier verdicts on survivors: confirmed at reviewer severity or tightened — **0 upgrades**.

**Confirmed findings by dimension:** Accuracy 31 · Completeness 10 · Tech Debt 5 · DRY 4 · CLEAN 2 (+ 2 cross-dimension). Accuracy-class correctness/durability defects dominate, exactly the profile expected of a database whose hardest problems are crash recovery and silent wrong-results.

**Hotspot zones (BLOCKER+CRITICAL):** `query-executor` 3 · `tidal-net` 2 · `repl-cluster` 2 · `db-export` 2; then `db-sessions`, `db-recovery`, `session`, `governance`, `storage-vector`, `text`, `wal-recovery`, `query-search`, `repl-shipping`, `tidal-server` one each.

---

## Fix-first queue

The single highest-leverage move is to **rewrite session restore as one routine that re-derives every durable invariant** — it is the root of both BLOCKERs and two CRITICALs.

1. **[BLOCKER]** `db/session_restore.rs:29-78` — `restore_sessions()` never advances `next_session_id`; reseeded to 1 on every open, so a clean restart re-issues id 1 and **overwrites the archived session snapshot** (silent, permanent data loss). Reviewer reproduced it: `user=1 signals=5` → `user=2 signals=0`.
2. **[BLOCKER]** `session/policy.rs:75` + `db/sweeper.rs:27` — TTL/`max_session_duration` is enforced off the **monotonic** `started_at`, which restore reseeds to `Instant::now()` (`session_restore.rs:146`). Every restart resets session age to ~0; the abuse/safety bound is silently void and expired sessions never reap. `close_session_internal` already uses the durable `started_at_ns` and documents the hazard — the two *enforcement* paths were missed.
3. **[CRITICAL]** `db/sweeper.rs:27` / `db/recovery` — same monotonic-clock root: the startup sweep that promises to reap sessions "that expired while the process was down" cannot, and a **closed session can be restored as ACTIVE** (`db-recovery`), landing the same id in both `sessions` and `closed_sessions`.
4. **[CRITICAL]** `storage/vector` — **USearch, the mandated production HNSW engine, is wired into nothing**: every deployment silently runs O(n) brute-force scans. A correctness-adjacent performance cliff hidden behind a "production" label.
5. **[CRITICAL]** `query/executor` & `query/search` — three silent wrong-results paths survive: cohort rescore discards the profile sort mode; the notification cap is recorded before pagination (over-counts deliveries the caller never sees); creator-search Category/Format filters are intersected against the *item* index and drop **all** creators.
6. **[CRITICAL]** `tidal-net` / `repl-shipping` — a dead segment-receiver thread is indistinguishable from a healthy one (the `server_terminated` probe built to detect it is never called); a follower silently stops replicating and reports success. Also `channel_capacity==0` **panics** in `GrpcTransport::new` because `validate()` runs after the channel is built.
7. **[CRITICAL]** `db/export/backup.rs` & `governance/community_ledger` — backup copy never `fsync`s files or the dest dir yet is documented "crash-consistent"; the governance contributor checkpoint (the *sole* non-WAL-rebuildable durable copy) is committed-not-fsynced while claiming a "ZERO loss window."
8. **[CRITICAL]** `wal/reader.rs:70-104` — `recover()` truncates a corrupt **non-final** segment and replays past the resulting sequence **gap** with no error surfaced.
9. **[CRITICAL]** `tidal-server/cluster.rs` — the cluster router has no concurrency limit or timeout; scatter-gather spawns **unbounded detached OS threads** per request (carried over from the prior pass).

---

## Step back — structural themes (the cause under the symptoms)

Several findings are instances of one missing abstraction. Fix the cause, not the three instances.

- **A. Session restore does not reconstruct durable invariants.** Five findings (2 BLOCKER + 2 CRITICAL + 1 WARNING) trace to the same place: the restore path rebuilds *in-memory* session state without re-deriving what the durable record already implies — the id-allocator high-water mark, the true wall-clock age (`started_at_ns`), and closed-vs-active membership from the durable `Close` marker. The proper fix is a **single restore routine** that, after scanning all durable session sources, (a) advances `next_session_id` past every snapshot/start id, (b) back-dates `started_at` from `started_at_ns` so the existing monotonic math is correct, and (c) routes closed snapshots into `closed_sessions` *only*. Today these are three half-implemented behaviors across two functions.

- **B. Unchecked lossy integer casts at the storage/index boundary.** A CRITICAL (`query-executor` Stage-2 intersection truncating candidate IDs with raw `as u32`) and three WARNINGs (`db-entities` `signal_with_context`, `db-recovery` `rebuild_item_indexes`, `wal-format` `encode_session_event` u16 length) are all the same bug: a 64-bit id/length narrowed with `as` instead of `try_into()`, silently aliasing or truncating once values exceed the narrow type. Centralize id-narrowing in one fallible helper that *skips with a counter* (or errors) on overflow.

- **C. Background workers fail silently.** The replication segment-receiver and `tidal-net` `recv_segment` (CRITICAL) and the text syncer's blocking outbox under a held mutex (CRITICAL) share a gap: a worker death or stall is invisible to the foreground and to operators, violating "if you can't observe it, you can't operate it." Each long-lived worker needs a liveness latch surfaced in health/diagnostics, and a non-blocking backpressure policy.

- **D. Documented-as-production, not on the live path.** USearch HNSW (CRITICAL), per-tenant WAL/storage isolation (`tenant_wal_dir` dead code, WARNING), and `gc_source` reclamation (WARNING) are all asserted by docs/names but not actually wired. This is doc↔impl drift that reads as "done" — the most dangerous kind, because nobody re-checks it.

- **E. Durability claims stronger than the code.** Backup `fsync` (CRITICAL), governance contributor checkpoint (CRITICAL), `recover()` gap-on-corrupt-segment (CRITICAL), macOS `F_FULLFSYNC` and `truncate_before` dir-fsync (WARNING): the WAL/storage core fsyncs correctly, but peripheral durable writes don't, while their docs assert "crash-consistent" / "zero loss." Bring every durable-artifact path up to the WAL's own standard, and where a platform (macOS plain `fsync`) cannot meet it, scope the doc claim to where it's true.

---

## Note on prior passes (regressions still open)

The `M0-M10-code-review-2026-06-08.md` doc records "Remediation status: RESOLVED — all 142 findings remediated." This fresh pass finds that **at least three previously-reported issues are still unfixed at HEAD**, flagged as such by the reviewers:
- *Sweeper never TTL-expires restored sessions* — "raised and verified in `…2026-06-07.md` (lines 306-316) and is still unfixed at HEAD."
- *`recv_segment` cannot distinguish server death from clean shutdown* — prior CRITICAL #21.
- *Cluster router unbounded thread spawn / no timeout* — prior CRITICAL #22.

The "RESOLVED" claim should be treated as **not fully accurate**; these belong back on the queue. (The wins are real too — the windowed-count tier-overlap double-count, the deferred-filter silent drops in RETRIEVE, and the atomic-batch durability fixes from earlier passes all hold up under this pass.)

---

## How to read the findings

Each finding below carries its verified location, the dimension it came from, the zone, the verifier's verdict, the concrete impact, and a production-quality fix. The seven-dimension **forcing-gate proof** (every zone's one-line verdict on all seven dimensions) is in the appendix.

---

## BLOCKERs (2) — must fix before ship

### [BLOCKER] Session-id allocator is not advanced past archived closed sessions on reopen — reused id silently overwrites an archived session snapshot (data loss)

**Location:** `tidal/src/db/session_restore.rs:29-78 (restore_sessions) and tidal/src/db/mod.rs:344 / SharedDefaults seeding next_session_id = AtomicU64::new(1)`
**Dimension:** Accuracy · **Zone:** `db-sessions` · **Verifier:** CONFIRMED

**Issue:** next_session_id is seeded to 1 on every open and is advanced ONLY by restore_session_wal_events (session_restore.rs:226-248), which advances past sessions that have a WAL Start but no Close. Cleanly-closed sessions are removed from the partition (partition_session_events removes them on Close at session_restore.rs:316-319), so they never advance the allocator. restore_sessions() (the storage scan that loads archived snapshots into closed_sessions) inserts snapshots and logs orphans but NEVER advances next_session_id. There is no durable session-id counter. Result: after a clean run that opens+closes session id 1 (snapshot durably archived at Tag::Session/b"snapshot" key for id 1) and shuts down, a reopen resets next_session_id to 1 and the next start_session re-issues id 1. close_session_internal then writes a NEW snapshot at the same key, overwriting the prior archived session, and apply_session_preference_update runs against the wrong (newly reused) session. I verified this with two real tests against a persistent data dir: (1) start/close session for user 1 (5 signals), reopen, start a new session for user 2 — the new session's id is SessionId(1), identical to the archived one; (2) after the new (user 2, 0-signal) session closes, session_snapshot(SessionId(1)) returns user_id=2, signals_written=0 — the user-1 archived snapshot is destroyed. Both assertions failed exactly as predicted (printed 'before: user=1 signals=5, after: user=2 signals=0').

**Why it matters:** This is silent, permanent loss of durably-archived session state — the exact 3am-incident class the durability discipline elsewhere in this zone is built to prevent. The whole point of persisting snapshots and start records (sessions.rs:87-93, the 'detect a crash during an active session at next startup' comment) is undermined: a clean restart corrupts the session archive. It also poisons cross-session preference aggregation, which keys on user_id and signaled_entities from a reused-id session. Every long-lived embedded deployment that restarts will progressively overwrite its oldest archived sessions starting from id 1.

**Fix:**

Make next_session_id durable across reopen by seeding it from the max id observed in ALL durable session sources, not just open WAL sessions. In restore_sessions() track a max_id while scanning: for every b"snapshot" (deserialize and read snapshot.id) AND every b"start" record (deserialize_start_record -> session_id), compute max_id = max(max_id, id). After the scan, advance the allocator with the same saturating CAS loop used in restore_session_wal_events: loop { let cur = next_session_id.load(Acquire); if max_id < cur { break; } if compare_exchange(cur, max_id.saturating_add(1), Release, Relaxed).is_ok() { break; } }. Because restore_sessions() runs in open() AFTER from_parts() (which runs restore_session_wal_events), the final allocator value must be max over both paths — the CAS-with-load-compare handles that idempotently regardless of order. Add a regression test mirroring the two I ran: assert a new session after reopen gets an id strictly greater than any previously-closed id, and that session_snapshot of the old id still returns the original user_id/signals_written.

---

### [BLOCKER] Session TTL and policy-duration limits are NOT honored across a crash — restored sessions reset their age to ~zero

**Location:** `tidal/src/session/policy.rs:75 and tidal/src/db/sweeper.rs:27 (root cause: started_at re-seeded at tidal/src/db/session_restore.rs:146)`
**Dimension:** Accuracy · **Zone:** `session` · **Verifier:** CONFIRMED

**Issue:** Both TTL-enforcing paths measure session age from the monotonic Instant `state.started_at`: PolicyEvaluator::check computes `now.duration_since(state.started_at)` (policy.rs:75) to reject expired writes, and sweep_expired_sessions computes `now.duration_since(state.started_at)` (sweeper.rs:27) to auto-close. But on crash recovery, restore_session_wal_events re-seeds `started_at: std::time::Instant::now()` (session_restore.rs:146) because the monotonic clock is not durable — the comment there even says 'We lost the exact monotonic Instant -- approximate with now.' The durable wall-clock start, `started_at_ns`, IS restored correctly and IS used by close_session_internal (sessions.rs:163-170, which explicitly documents this exact hazard and fixes it there). So after a restart, a session that started 2 hours ago under a 1-hour max_session_duration has elapsed≈0 in BOTH the sweeper and the policy check: the sweeper will never reap it, and session_signal will keep accepting writes well past the policy limit. The team already diagnosed and fixed this monotonic-not-durable bug in close_session_internal but left the two paths that actually ENFORCE the limit on the broken clock.

**Why it matters:** This is the literal focus item for this zone: 'TTL/lease honored across crashes.' It is not. A policy that caps an agent session at N minutes (a safety/abuse bound) is silently voided by any restart — the session's clock restarts from zero on every crash, so a long-lived or adversarial agent can evade max_session_duration indefinitely by surviving across restarts, and expired sessions accumulate in memory because the sweeper never sees them as expired. Worse, the divergence is silent: close_session reports the true ~2h age via started_at_ns, but the enforcement paths think it is brand new. There is no test covering restored-session expiry, so this regressed unnoticed.

**Fix:**

Make both enforcement paths measure age from the durable `started_at_ns`, exactly as close_session_internal already does. In PolicyEvaluator::check, replace `now: Instant` / `now.duration_since(state.started_at)` with a wall-clock delta: pass `now_ns: u64` (Timestamp::now().as_nanos() at the call site in session_signal) and compute `elapsed = Duration::from_nanos(now_ns.saturating_sub(state.started_at_ns))`. In sweep_expired_sessions, replace `now.duration_since(state.started_at)` with `Duration::from_nanos(Timestamp::now().as_nanos().saturating_sub(state.started_at_ns))`. Keep `started_at: Instant` only for live latency/duration metrics where monotonicity is wanted, but never for the durable TTL bound. Add a regression test (mirroring sessions.rs:636 restored_session_duration_is_age_not_time_since_restore): construct a session with started_at_ns set ~2h in the past and a fresh started_at=Instant::now(), then assert (a) sweep_expired_sessions/force_sweep closes it and (b) session_signal returns SessionExpired — proving the TTL survives a simulated restore.

---


## CRITICAL (17) — should fix before merge

### [CRITICAL] Sweeper never TTL-expires restored sessions: age measured from a monotonic Instant that is reset to now() on restore

**Location:** `tidal/src/db/sweeper.rs:27-41 (now.duration_since(state.started_at)); tidal/src/db/session_restore.rs:146 (started_at: Instant::now())`
**Dimension:** Accuracy · **Zone:** `db-export` · **Verifier:** CONFIRMED

**Issue:** sweep_expired_sessions computes elapsed = now.duration_since(state.started_at). started_at is a std::time::Instant. On restart, restore_session_wal_events rebuilds each open session with started_at: Instant::now() (session_restore.rs:146, 'We lost the exact monotonic Instant -- approximate with now'), discarding the true age. The durable wall-clock anchor started_at_ns IS preserved on the restored struct (session_restore.rs:147) but is never read by the sweeper or any expiry path. Consequently a session that was already past max_session_duration at crash/shutdown time is handed a fresh full lease on every restart and is never reaped. The start_sweeper docstring (sweeper.rs:64-68, 104-106) explicitly promises the opposite: 'sessions that expired while the process was down ... are reaped promptly at startup.' That guarantee does not hold for restored sessions. This exact defect was raised and verified in docs/reviews/M0-M10-code-review-2026-06-07.md (lines 306-316) and is still unfixed at HEAD.

**Why it matters:** GC/TTL correctness is this zone's special focus. max_session_duration is a policy bound; a crash loop or routine restart cadence lets abandoned sessions live indefinitely, leaking memory (signaled_entities, signals maps) and violating the documented session lifecycle. The bug is invisible in single-run tests and only manifests across restarts — precisely the 3am-incident failure mode.

**Fix:**

Make expiry use the durable wall-clock anchor. Cleanest single-path fix: in session_restore.rs back-date started_at on restore so the existing monotonic math reflects true age — let elapsed = Timestamp::now().as_nanos().saturating_sub(started_at_ns); started_at: Instant::now().checked_sub(Duration::from_nanos(elapsed)).unwrap_or_else(Instant::now). Then the unchanged sweeper computes correct elapsed for restored sessions and an over-age session is reaped on the first startup sweep. Add a test: open a session, persist, reopen with a clock/started_at_ns far in the past, force_sweep, assert the session is closed.

---

### [CRITICAL] Backup copy never fsyncs files or the destination directory; a host crash can silently truncate a 'crash-consistent' backup

**Location:** `tidal/src/db/backup.rs:57-139 (copy_dir_recursive, std::fs::copy with no sync_all) and backup.rs:273 (copy call); docstring claims at backup.rs:11-13, 144-148`
**Dimension:** Accuracy · **Zone:** `db-export` · **Verifier:** CONFIRMED

**Issue:** copy_dir_recursive uses std::fs::copy for every file and std::fs::create_dir_all for directories, but never calls sync_all on the copied files nor fsyncs the destination directory entries. create_backup returns BackupInfo and logs 'backup completed' while the copied bytes and new dirents may still live only in the page cache. The module docstring and create_backup doc both assert the result is 'crash-consistent' and directly openable. That holds against a process crash but NOT against a host/power crash in the window after create_backup returns: files can be zero-length or missing, directory entries unlinked — a corrupt backup discovered only at restore. Note the WAL layer already fsyncs files and parent dirs everywhere (wal/segment.rs:178/281, wal/checkpoint.rs:44/54, wal/compaction.rs:150), so the backup path is the inconsistent outlier.

**Why it matters:** A backup whose durability you cannot trust is worse than no backup — you discover it is broken at the moment you need it. Durability of backup artifacts is exactly what this zone weights heavily. The 'crash-consistent' claim in the docs is currently false for the only crash that backups exist to survive.

**Fix:**

After copying each file, fsync it: open the dest_path and call sync_all() (or copy then File::open(dest).sync_all()). After the whole tree is copied, fsync every directory that received entries (open the dir and sync_all on the fd, as the WAL does). Reuse the existing dir-fsync helper pattern from wal/segment.rs. Then update the docstring only if a residual gap remains. Add a fault-injection-style test or at minimum assert files are sync'd; the existing backup_captures_all_acked_signals test only proves logical completeness, not durability.

---

### [CRITICAL] A closed session can be restored as ACTIVE — same session_id ends up in both self.sessions and closed_sessions (phantom state)

**Location:** `tidal/src/db/session_restore.rs:90-251 (restore_session_wal_events) and 29-78 (restore_sessions); reachable via close ordering in tidal/src/db/sessions.rs:178-202`
**Dimension:** Accuracy · **Zone:** `db-recovery` · **Verifier:** CONFIRMED

**Issue:** On restart there are two independent restore paths with no cross-check. restore_session_wal_events() restores every session that has a journal Start with no matching Close into self.sessions as an ACTIVE session (it never consults closed_sessions or the durable b"snapshot" row). restore_sessions() independently loads every durable b"snapshot" row into closed_sessions. close_session_internal commits the snapshot+delete-start batch FIRST (sessions.rs:186-191) and appends the journal Close SECOND (sessions.rs:197-202), explicitly tolerating a failed Close append with only a warning. The session journal (sessions.log) is append-only and NEVER compacted, so the original Start record is always still present at replay. Therefore a crash (or a failed Close append) between the snapshot batch and the Close append leaves: snapshot row present (no start row) AND Start-without-Close in the journal. Reopen then inserts the SAME session_id into closed_sessions (as ended) and into self.sessions (as active), and the active copy will accept new signals against a session the operator and the durable snapshot consider closed. CODING_GUIDELINES line 241 requires recovery to a consistent state with 'no phantom state'; this violates it.

**Why it matters:** A cleanly-or-nearly-cleanly closed session silently resurrects as live after a crash, diverging in-memory state from the durable record of truth. Signals can be written into a session that was reported closed (wrong summaries, double-counted engagement), active_sessions() lists a session that closed_sessions also reports as ended, and there is no log or error — exactly the 3am 'why is this closed session still alive' incident. There is no test exercising the snapshot-present + Start-without-Close combination, so the divergence is currently invisible.

**Fix:**

Make the durable snapshot authoritative over the journal during restore. Two changes: (1) Reorder so closed snapshots are known before active restore — call restore_sessions() (or at least load the snapshot key-set) BEFORE restore_session_wal_events(), and in restore_session_wal_events skip any session_id already in closed_sessions. (2) Independently of ordering, guard the active-restore insert with a direct storage probe in restore_session_wal_events: before building SessionState, check `storage.items_engine().get(&encode_key(EntityId::new(session_id), Tag::Session, b"snapshot"))`; if a snapshot exists, the session is closed — skip it (optionally log at info) instead of inserting into self.sessions. The function already reads the b"start" record there, so the probe is one extra point-lookup on a cold path. Add a regression test: open, start+signal a session, close it, then re-append a synthetic Start-without-Close to the journal (or crash between the snapshot batch and the Close append), reopen, and assert the session is in closed_sessions and NOT in active_sessions().

---

### [CRITICAL] Per-accept contributor persist is committed-not-fsynced, but claims a 'ZERO loss window' for the SOLE non-WAL-rebuildable durable copy

**Location:** `tidal/src/governance/community_ledger.rs:462-496 (persist_cell) and tidal/src/db/communities.rs:314-345; checkpoint at community_ledger.rs:517-565`
**Dimension:** Accuracy · **Zone:** `governance` · **Verifier:** CONFIRMED

**Issue:** persist_cell and checkpoint commit the contributor row via storage.write_batch(batch) and return Ok with no flush()/fsync. StorageEngine::write_batch's own contract (storage/engine.rs:44-50) states 'Atomicity is NOT durability ... the bytes may still sit in an in-process memtable / OS page cache and be lost on power loss. Callers that need the batch to survive a crash MUST pair it with flush()'. The Fjall backend confirms this: write_batch ends at wb.commit() (fjall.rs:183), and only flush() issues persist(SyncAll) (fjall.rs:186-199). Yet communities.rs:314-326 asserts a 'ZERO loss window for an accepted contribution' and persist_cell's rustdoc says the cell is 'durably written ... before the caller returns Ok(true)'. The community ledger is explicitly the SOLE durable copy of per-contributor identity (the WAL community event carries none and cannot rebuild it — community_ledger.rs:504-506, confirmed: WAL replay never touches the community ledger). The fsync-durable WAL event therefore does NOT protect the attribution. On power loss / kernel panic after Ok(true) but before the next fsync barrier (only shutdown lifecycle.rs:212 or backup backup.rs:265 — the periodic maintenance loop checkpoints WITHOUT flushing, state_rebuild.rs:601-606), the contributor row is permanently lost while the WAL still reports the aggregate event. The claimed guarantee is violated.

**Why it matters:** This is the one piece of state in the zone that cannot be rebuilt from the WAL. A power-loss crash silently drops contributor attribution — breaking M9p3 retroactive purge (the purged user's contributions can no longer be located) and M10p3 explainability/remove-by-agent, and contradicting the explicit 'zero loss' promise the API makes to callers who treated Ok(true) as durable.

**Fix:**

Make the per-accept durability match the claim. After the persist_cell write_batch succeeds, issue the fsync barrier on the items keyspace before returning Ok(true): follow storage.write_batch(batch)? with storage.flush()? (Fjall::flush already does rotate_only + persist(SyncAll)). To avoid an fsync per accept dominating latency, instead carry the contributor identity (user, writer_agent, epoch) in the community WAL event envelope so the ledger becomes WAL-rebuildable and the fsync-durable WAL append is the durability point — then drop persist_cell entirely. Either path closes the gap. If neither is adopted now, correct the rustdoc/comments to state the window is bounded by the next flush barrier (shutdown/backup), not zero, and add storage.flush() to the periodic maintenance loop so exposure is bounded by the checkpoint interval rather than by 'time since last clean shutdown'.

---

### [CRITICAL] Cohort rescore discards profile sort mode and reported scores; silently wrong ordering for any non-boost profile

**Location:** `tidal/src/query/executor/helpers.rs:182-267 (rescore_with_cohort), invoked from tidal/src/query/executor/mod.rs:452-468`
**Dimension:** Accuracy · **Zone:** `query-executor` · **Verifier:** CONFIRMED

**Issue:** The main scoring path bakes the profile's Sort into ScoredCandidate.score (scoring.rs:69-143 — e.g. Sort::New scores by entity_id, Sort::MostViewed by windowed count, Sort::AlphabeticalAsc by title). rescore_with_cohort then OVERWRITES every candidate.score with a sum derived ONLY from profile.boosts, normalizes, and re-sorts strictly by descending score — never consulting profile.sort. Cohort context is wired on EVERY retrieve (query_ops.rs:104-105) and the rescore fires whenever query.cohort is set, which the public builder (types.rs:358) allows with ANY profile. For a sort-driven profile with empty boosts (e.g. the built-in `new`), cohort_score collapses to 0.0 for all candidates, normalize maps them all to 1.0, and the descending sort is a no-op — so the query returns score=1.0 for every item and the profile's New/Alphabetical/Shortest ordering is destroyed, with no error. signal_snapshot is also left reflecting the global-ledger scores, not the cohort scores reported.

**Why it matters:** A well-formed query (cohort + any sort profile) silently returns wrong-ordered results and bogus uniform scores. This is exactly the 'cohort rescore must honor profile sort mode' failure the zone calls out — a 3am ranking-incident that looks like data corruption but is a logic gap.

**Fix:**

Make the cohort rescore signal-scoped, not a full re-derivation: read cohort signal values and feed them into the SAME score_by_sort / boost pipeline the main path uses (pass the cohort ledger as the signal source), so Sort modes and boosts both apply under cohort scope. At minimum, when profile.sort is metadata/ID-based or profile.boosts is empty, do not overwrite score from boosts — preserve the main-path score (or re-run score_by_sort against the cohort ledger) and keep signal_snapshot consistent. Add tests: cohort + `new` must keep descending-ID order; cohort + a boost-driven profile must rank by cohort signals.

---

### [CRITICAL] Notification-cap recording runs before pagination, over-recording deliveries the caller never sees

**Location:** `tidal/src/query/executor/pipeline.rs:430-461 (Stage 4.5) then pagination at pipeline.rs:467-479; recording in tidal/src/query/executor/helpers.rs:318-333 (apply_notification_caps notification path)`
**Dimension:** Accuracy · **Zone:** `query-executor` · **Verifier:** CONFIRMED

**Issue:** apply_notification_caps runs on the FULL post-diversity candidate set and, for the `notification` profile, calls tracker.check_and_record (an unconditional increment of persistent per-user/per-creator/day counters — notification_tracker.rs:95-129) for every surviving candidate. Pagination (offset/limit slicing) happens AFTER, in Stage 5. So a query with limit=5 and 50 surviving candidates records 50 deliveries while returning 5 — the daily cap is consumed by 45 items the user never received on this page. Re-issuing the same query (or fetching the next page, which re-runs the whole pipeline) records them all again; check_and_record has no entity-level idempotency.

**Why it matters:** The notification cap is persistent shared state that drives whether a user gets notified. Over-recording silently exhausts the daily budget far below the real delivery count, suppressing legitimate notifications — a correctness defect in the exact feature the cap exists to enforce, and it corrupts state that is hard to reason about after the fact.

**Fix:**

Record deliveries against only the items actually returned to the caller (the post-pagination page), not the full pre-slice set. Either move the recording side effect out of apply_notification_caps and apply it to the final `page` slice in Stage 5, or split check-vs-record so Stage 4.5 only filters and the record step is keyed to the delivered page. Add a test: limit smaller than surviving candidates must record exactly `limit` deliveries, and a second identical query must not double-count delivered items.

---

### [CRITICAL] Stage 2 index intersection truncates candidate IDs with raw `as u32`, can wrongly retain a >u32::MAX candidate

**Location:** `tidal/src/query/executor/pipeline.rs:291-295`
**Dimension:** Accuracy · **Zone:** `query-executor` · **Verifier:** CONFIRMED

**Issue:** `candidates.retain(|id| bitmap.contains(id.as_u64() as u32))` performs an INCLUSION test against the FilterEvaluator's match bitmap using a raw `as u32` truncation of the candidate id. The module's own canonical helper entity_as_u32 (mod.rs:498-510) exists precisely to forbid this: per its documented polarity rule, an included id that overflows u32 must be DROPPED, because truncating `2^32 + k` to `k` aliases it onto a low slot that may be present in the match bitmap and wrongly RETAIN it. Candidates from the SignalRanked strategy are sourced straight from the ledger, which keys on full-u64 EntityId (trimmer.rs:74), so an entity id > u32::MAX is valid input that reaches this line. The SAFETY comment ('truncation is intentional and correct throughout this module') is wrong for the candidate side — every other site in this zone routes through entity_as_u32.

**Why it matters:** A well-formed SignalRanked query with an index-backed filter (e.g. CategoryEq) can leak an item that does not satisfy the filter when its id exceeds 4.29B and aliases onto a matched low id — silently wrong results on hostile/large-id input, the same bug class the rest of the zone hardened against.

**Fix:**

Route the inclusion test through the checked conversion: `candidates.retain(|id| crate::query::executor::entity_as_u32(*id).is_some_and(|i| bitmap.contains(i)))`, matching the InCollection/SocialGraph arms. Correct the SAFETY comment. Add a regression test with a SignalRanked candidate at `(1u64<<32)+k` and a filter bitmap containing `k`, asserting the overflow id is dropped.

---

### [CRITICAL] Creator-search CategoryEq/FormatEq filter is intersected against the item index in Stage 2, silently dropping all creators

**Location:** `tidal/src/query/search/executor/pipeline.rs:181-182, 423-476 (apply_metadata_filter has no entity_kind guard); wired item-scoped category_index at tidal/src/db/query_ops.rs:224-229`
**Dimension:** Accuracy · **Zone:** `query-search` · **Verifier:** CONFIRMED

**Issue:** For a SEARCH with entity_kind = Creator and a CategoryEq/FormatEq filter, execute() runs apply_metadata_filter BEFORE apply_creator_metadata_filter. apply_metadata_filter builds a FilterEvaluator over the *item-scoped* bitmap indexes (category_index/format_index, always wired from query_ops.rs regardless of entity_kind) and does candidates.retain(|id| bitmap.contains(id)). The candidates here are CREATOR ids from creator BM25/ANN retrieval, but the bitmap holds ITEM ids in that category. So creators are intersected against the wrong namespace: in a creator-only DB the item category index is empty and EVERY creator is dropped; in a mixed DB only creators whose id coincidentally aliases an item-category id survive. The Stage 2b creator-metadata post-filter (apply_creator_metadata_filter, lines 479-515) then re-checks the same filter against stored creator metadata, but it is an AND-style retain and cannot resurrect candidates Stage 2 already removed. Net effect: a documented feature (filter creators by category/format) returns zero or wrong results with no error and nothing in the logs. The m5p4 step02 test masks this — it asserts only no-panic and comments 'Results may be 0 if bitmap not populated for creators' (tidal/tests/m5p4_creator_search.rs:144-156).

**Why it matters:** This is exactly the 3am silent-wrong-results class the review weights as CRITICAL: a caller asks for verified/jazz creators and gets an empty or arbitrary set, with no diagnostic. It also contradicts the project's own anti-pattern fix elsewhere (the W29 cross-field guard and try_eq UnknownField work were done specifically so creator metadata filters are correct) — the pre-filter stage undoes that intent for the Creator path.

**Fix:**

Make the Stage 2 index pre-filter item-only. In apply_metadata_filter, return early (retain all) when query.entity_kind == EntityKind::Creator, mirroring the existing guard style in apply_creator_metadata_filter (pipeline.rs:485). Thread the entity_kind into apply_metadata_filter (it already takes &Search-derived combined_filter; pass query or query.entity_kind). The creator path then relies solely on apply_creator_metadata_filter (Stage 2b) which reads the correct per-creator metadata. Add a regression test: build a DB with BOTH items (some in category 'jazz') and creators (with metadata category='news'), SEARCH creators with FilterExpr::eq('category','news'), and assert the matching creators are returned (not dropped by the item-index intersection). Note: for entity_kind = Creator, deferred variants (MinSignal/InCollection/etc.) are still no-ops in Stage 2 and handled correctly downstream, so the early-return is safe for them too.

---

### [CRITICAL] finalize() swallows its checkpoint error but no later transition re-persists — a silent finalize-checkpoint failure reverts the tenant to DualWrite on crash

**Location:** `tidal/src/replication/migration.rs:204-213 (finalize), 222-241 (gc_source), 83-97 (persist_routing)`
**Dimension:** Accuracy · **Zone:** `repl-cluster` · **Verifier:** CONFIRMED

**Issue:** finalize() updates the router in memory (finalize_migration: clears dual-write, sets the pin) then calls self.persist_routing() — the NON-strict variant (lines 83-97) that logs and SWALLOWS a storage error. Its own comment (lines 209-211) claims this persist means 'a crash after finalization cannot revert the tenant'. That guarantee is false. If the durable write fails, disk still holds the pre-finalize {DualWrite} routing while memory holds the pin, and finalize() still returns Ok(()). The only subsequent transition, gc_source (222-241), never calls persist_routing/persist_routing_strict. So nothing re-persists. On crash + restore_migration_routing, the tenant comes back in DualWrite mode (reads route to the SOURCE shard, writes fan out to both) — the exact 'revert finalized tenant' bug this slice was built to close. The justification in persist_routing's doc ('a missed checkpoint is recoverable on the next transition') does not hold for finalize, because finalize's only next transition (gc_source) does not persist.

**Why it matters:** finalize is the cutover that re-points every read for the tenant from source to target. A silent revert on crash means reads after recovery hit the SOURCE shard, which gc_source was about to garbage-collect — serving stale or (post-GC) empty results for that tenant with no error surfaced. This is the same class of silent mis-routing the strict-persist path in enter_dual_write was added to prevent; finalize is asymmetrically weaker on the more dangerous transition.

**Fix:**

Make finalize durable-before-observable like enter_dual_write: snapshot the router, apply finalize_migration in memory, then persist_routing_strict(); on Err, restore the snapshot and leave the state in DualWrite so the caller retries once storage recovers (mirror the enter_dual_write rollback at lines 164-176). If finalize must stay best-effort, then either (a) re-persist in gc_source via persist_routing_strict before flipping to Complete, or (b) correct the comment to state the revert risk explicitly. The strict-persist option is correct for a database — durability before the cutover is observable.

---

### [CRITICAL] enter_dual_write rollback restores a full-router snapshot, clobbering a concurrent migration's routing for a different tenant

**Location:** `tidal/src/replication/migration.rs:147-180 (enter_dual_write rollback), tidal/src/replication/tenant.rs:408-447 (to_checkpoint_bytes / restore_from_checkpoint_bytes)`
**Dimension:** Accuracy · **Zone:** `repl-cluster` · **Verifier:** CONFIRMED

**Issue:** All TenantMigration instances share one Arc<TenantRouter> (db/mod.rs:288 builds a single router; each migration has only its own per-tenant Mutex<MigrationState>). On a persist failure, enter_dual_write rolls back by calling restore_from_checkpoint_bytes(&pre_snapshot) (line 172-174), and restore_from_checkpoint_bytes (tenant.rs:433-445) does dual_write_map.clear() + shard_pin_map.clear() then reloads the WHOLE snapshot. If tenant A's enter_dual_write took its pre_snapshot, and tenant B's migration concurrently called set_dual_write / finalize_migration on the same router between A's snapshot and A's rollback, A's rollback wipes B's just-applied (and possibly already-persisted) entry. There is no global migration lock serializing distinct tenants' transitions — the only mutex is per-migration state, which does not cover the shared DashMaps.

**Why it matters:** Multi-tenant isolation is the central promise of this zone: one tenant's migration must never corrupt another tenant's routing. Here a storage hiccup during tenant A's dual-write entry can silently revert tenant B's finalized pin or dual-write, mis-routing B's writes/reads with no error on either path. It is timing-dependent, so it passes every single-migration test and surfaces only under concurrent migrations in production — the worst kind of latent durability bug.

**Fix:**

Do not use a whole-router snapshot/restore for rollback. Roll back precisely the entry this migration mutated: on persist failure, call a targeted dual_write_map.remove(self.tenant_id) (and, for symmetry, restore only this tenant's prior pin if one existed) instead of clear-and-reload. Capture just this tenant's prior (dual_write, pin) entries before set_dual_write and restore exactly those. Alternatively, serialize all router-mutating migration transitions under a single coordinator-level lock so snapshot/restore is atomic with respect to other tenants — but the targeted-remove fix is cheaper and avoids the global lock on a cold path.

---

### [CRITICAL] Segment-receiver thread death mid-life is silent: no health latch, indistinguishable from a still-running receiver until shutdown

**Location:** `tidal/src/replication/receiver.rs:87-137 (spawn_receiver, halt path 117-129); tidal/src/db/diagnostics.rs:48-90 (health_check); tidal/src/db/replication_ops.rs:23-40 (start_replication)`
**Dimension:** Accuracy · **Zone:** `repl-shipping` · **Verifier:** CONFIRMED

**Issue:** When the receiver loop hits a corrupt segment or a follower-WAL IO fault, apply_payload_with_leader_seq returns Err and the thread returns that Err and exits (receiver.rs:128). That error is ONLY retrievable via SegmentReceiverHandle::join() (receiver.rs:61), which the node calls exclusively at shutdown (db/lifecycle.rs:62-67). While the node stays open, nothing observes the death: health_check (diagnostics.rs:48-90) consults `closed`, `checkpoint_thread.is_finished()` (latching checkpoint_thread_died), `metrics.is_degraded()`, and `text_index_unhealthy()` — but never the receiver. SegmentReceiverHandle exposes no is_finished()/healthy() accessor at all (receiver.rs:49-70). Worse, the leader-HWM update lives INSIDE the same apply path that just died (receiver.rs:108-116 → apply_payload_with_leader_seq → gauge.update_leader_seqno at receiver.rs:287), so once the receiver is dead BOTH applied_seqno and leader_seqno freeze and lag_segments() (lag.rs:56-60) stays flat instead of growing. A partitioned/desynced follower whose receiver halted therefore reports zero lag and a healthy node while silently applying nothing — exactly the 'server-loop death must be distinguishable from clean shutdown' failure, invisible at 3am. recv_segment()==None (clean shutdown) and a halt-on-Err both terminate the thread; only the Err return distinguishes them, and only at join().

**Why it matters:** Replication exists so a follower can be promoted with the freshest state. A follower whose receiver died on one corrupt segment will be promoted believing it is caught up, silently losing every event the leader shipped after the halt. The whole point of multi-node durability is defeated by an unobservable stalled receiver. The codebase ALREADY proves the correct pattern is expected here: the checkpoint thread latches metrics.checkpoint_thread_died via a panic supervisor + JoinHandle::is_finished() and folds it into health_check (db/mod.rs:113-120, diagnostics.rs:57-70), and the text syncer latches text_index_unhealthy the same way. The receiver is the one durability-critical background loop without this.

**Fix:**

Give the receiver the same liveness latch as the checkpoint thread. (1) Add an Arc<AtomicBool> `replication_receiver_died` to MetricsState (or TidalDb), passed into spawn_receiver. On the Err halt path (receiver.rs:124-128) and at any unexpected thread exit, set it true before returning; on the clean recv_segment()==None path (receiver.rs:97-100) leave it false. (2) Alternatively/additionally expose SegmentReceiverHandle::is_finished(&self)->bool and have health_check inspect the receiver_handle guard with is_finished() exactly like checkpoint_thread (diagnostics.rs:60-70), latching the flag. (3) Fold the latch into health_check's degraded decision and the HTTP/metrics health surface so a dead receiver returns degraded. (4) Add a test: feed one corrupt segment, assert the receiver halts AND that health_check returns Err / the latch is set WITHOUT closing the DB (the existing receiver_thread_exits_on_corrupt_payload only asserts via join() at teardown). This makes a stalled follower loud and bounded instead of silently frozen.

---

### [CRITICAL] USearch (the mandated production HNSW engine) is wired into nothing — every deployment does O(n) brute-force scans

**Location:** `tidal/src/storage/vector/registry.rs:384 (rebuild), tidal/src/db/users.rs:144 (write auto-register), tidal/src/query/search/executor/pipeline.rs:803; UsearchIndex only constructed in tidal/src/storage/vector/usearch_index.rs tests and tidal/tests/vector_usearch.rs`
**Dimension:** Tech Debt / Extensibility · **Zone:** `storage-vector` · **Verifier:** CONFIRMED

**Issue:** CODING_GUIDELINES §4 states 'USearch is the HNSW engine ... 126K+ QPS' and §8 sets the benchmark target 'ANN retrieval at 1M vectors <10ms p99'. But grep confirms UsearchIndex::new appears only in test code. Every production embedding-slot instantiation — the rebuild-from-store path, the write-path auto-register in users.rs, and the query pipeline — hardcodes Box::new(BruteForceIndex::new(config)). There is no schema/config selector that ever picks UsearchIndex. The entire usearch_index.rs module (save/load/view/tombstone/total_slots) is dead in production: correct, fully tested, and unreachable.

**Why it matters:** BruteForceIndex computes l2_distance_sq against every stored vector under an RwLock read on every search. At the spec's 1M-vector / 1536D scale that is ~1.5e9 float ops plus a full O(n) collect per query — orders of magnitude past the <10ms p99 budget, and it serializes writes against searches on one RwLock. Results are exact (no correctness bug), but the database silently cannot meet its stated scale or latency contract; an operator reading the docs would believe HNSW is active. This is the kind of gap that surfaces as a production latency incident at 3am with no code-level signal that brute force is the cause.

**Fix:**

Add a backend selector so EmbeddingSlotState is built from UsearchIndex in persistent/production mode and BruteForceIndex only for tests/tiny slots. Concretely: introduce a `VectorBackend` enum (or a factory closure) on the registry/config, default it to USearch when a real data dir is configured, and route registry.rs:384, users.rs:144, and the query pipeline through that factory instead of the hardcoded `Box::new(BruteForceIndex::new(config))`. Gate the choice on slot size or an explicit schema flag if you want brute force for <N vectors. Until USearch is reachable, either change the guideline/docs to state brute-force is the shipped engine or open a tracked issue — the current state silently contradicts the spec.

---

### [CRITICAL] Blocking send on bounded outbox under a held mutex stalls all item writes during a text-index commit outage

**Location:** `tidal/src/db/items.rs:197-205 (producer) with tidal/src/db/text_syncer.rs:40,207 (bounded channel)`
**Dimension:** Accuracy · **Zone:** `text` · **Verifier:** CONFIRMED

**Issue:** `write_item_with_metadata` locks `self.text_tx` (a `Mutex<Option<Sender<PendingWrite>>>`) and then calls the BLOCKING `tx.send(...)` on a channel that `open_text_syncer` creates as `crossbeam::channel::bounded(WRITE_OUTBOX_CAPACITY=16_384)`. The syncer's `run()` is designed to retry a failing commit FOREVER (syncer.rs:124-128 'keeps retrying regardless'). So on a persistent commit outage (disk full, corrupt segment, fsync EIO) the syncer stops draining, the 16K buffer fills, and `send` blocks indefinitely — while the `text_tx` MutexGuard is still held. Every concurrent `write_item_with_metadata` then blocks on `self.text_tx.lock()` too. A degraded-text-search incident (which the design explicitly says must be tolerated: 'text index is derived state... best-effort') is thereby escalated into a full stall of the durable item write API.

**Why it matters:** This inverts the core design contract that the text index is best-effort derived state that must never gate durable writes (CODING_GUIDELINES Sec 5; spec 06 axiom 3). The durable entity-store write has already completed by line 144, so blocking here adds zero durability — it only removes availability. At 3am a single full disk on the Tantivy directory would freeze all item ingestion across the process, and the cause (a held mutex around a blocking send) is non-obvious from the call site. The bounded-channel doc comment even acknowledges backpressure 'stalls' the producer but does not account for that stall happening inside a global mutex critical section.

**Fix:**

Do not hold the mutex across a blocking send, and do not let a derived-index outage block durable writes. Two-part fix: (1) clone the `Sender` out of the guard and drop the guard before sending — `let tx = self.text_tx.lock().ok().and_then(|g| g.clone()); if let Some(tx) = tx { ... }` (crossbeam `Sender` is cheap to clone). (2) Use `try_send` (or `send_timeout` with a short bound) instead of blocking `send`, and on `Full`/`Timeout` log a warn and drop the enqueue — the unconditional rebuild-from-store at open re-indexes anything dropped, so correctness is preserved while the write path stays non-blocking. The text_syncer.rs comment already contemplates 'or, at the call site, dropping' — implement that drop rather than relying on a blocking send under lock.

---

### [CRITICAL] recv_segment cannot distinguish server death from clean shutdown; the probe built to detect it (server_terminated) is never called

**Location:** `tidal-net/src/transport.rs:202-213 (server_terminated), :244-286 (recv_segment); tidal/src/replication/receiver.rs:96-100 (consumer)`
**Dimension:** Completeness · **Zone:** `tidal-net` · **Verifier:** CONFIRMED

**Issue:** When the gRPC serve loop fails AFTER startup (listener dies, TLS handshake task panics, reactor torn down), the spawned server task ends, its inbound_tx sender drops, the inbound channel closes, and recv_segment returns None. The receiver thread (receiver.rs:97-100) treats None as a clean shutdown: it logs debug! 'transport closed, shutting down' and returns Ok(()). A follower whose receive side has silently died is therefore indistinguishable from one that was intentionally shut down — exactly the failure mode the zone's special-focus calls out. server_terminated() was built to surface this ('Health checks and the cluster control plane poll this so a follower whose receive side has silently died is observable, not a black hole') but grep across tidal-net, tidal-server, and tidal confirms it has ZERO callers. The serve task's terminal Err is logged at error! (server.rs:192-193), which helps a human reading logs, but no code path demotes the node or fails the receiver — at 3am the cluster control plane still believes the follower is healthy while replication has silently stopped.

**Why it matters:** Silent replication stall on a follower whose serve loop died: the leader keeps shipping (sends may even succeed against a half-open connection until keep-alive trips), the follower's receiver thread has exited Ok, and nothing observes it. This is a durability-relevant blind spot — divergence accumulates with no signal.

**Fix:**

Wire server_terminated() into the liveness story so the two None causes are distinguishable. Concretely: (1) have the cluster control plane / health endpoint poll transport.server_terminated() and demote/alert the node when it flips true (it is already #[must_use] and ready); OR (2) make the distinction explicit at the receiver boundary — e.g. on recv_segment returning None, the receiver checks server_terminated() and, if the server died WITHOUT a shutdown_receivers() request, returns an Err(WalError) via SegmentReceiverHandle::join instead of Ok(()) so a dead serve loop surfaces as a failed receiver rather than a clean exit. Add an integration test that fails the serve task (e.g. drop/abort it) and asserts the follower observes server_terminated()==true (and/or the receiver surfaces an error), distinct from the shutdown_receivers() clean-exit case already covered by shutdown_unparks_recv_segment_and_thread_exits.

---

### [CRITICAL] channel_capacity==0 panics in GrpcTransport::new because validate() runs after the mpsc channel is built

**Location:** `tidal-net/src/transport.rs:138 then :142-146; tidal-net/src/config.rs:101-106 (validate); tidal-net/src/client.rs:37 (only validate() callsite)`
**Dimension:** Accuracy · **Zone:** `tidal-net` · **Verifier:** CONFIRMED

**Issue:** GrpcTransport::new builds the inbound channel with `mpsc::channel(config.channel_capacity)` at transport.rs:138, then enters runtime.block_on where it calls start_server (:143) and PeerPool::new (:144). config.validate() — the only thing that rejects channel_capacity==0 — is invoked inside PeerPool::new (client.rs:37), i.e. AFTER the channel is already constructed. tokio's bounded channel asserts `buffer > 0` (tokio-1.49 bounded.rs:157), so GrpcTransport::new(cfg) with channel_capacity:0 panics with 'mpsc bounded channel requires buffer > 0' before validate() ever runs. The config doc at config.rs:82-85 explicitly promises the opposite: 'surface it as a typed error here instead of a panic deep in GrpcTransport::new.' The existing test rejects_invalid_config only exercises PeerPool::new directly, so it never hits the real constructor ordering and the panic is masked.

**Why it matters:** A buggy or operator-supplied config panics the database's transport constructor instead of returning the typed Internal error the design guarantees. CODING_GUIDELINES §7: 'Panics in a database corrupt state.' The whole point of validate() (a defensive guard against a known panic) is defeated by call ordering.

**Fix:**

Call config.validate() at the very top of GrpcTransport::new, before mpsc::channel and before entering the runtime — e.g. add `config.validate()?;` immediately after ensure_crypto_provider() at transport.rs:129. Then either keep the validate() in PeerPool::new (idempotent, cheap) or document that the transport validates up front. Add a test that calls GrpcTransport::new (not PeerPool::new) with channel_capacity:0 and asserts Err(Internal), not a panic — guarding the real constructor path the current test misses.

---

### [CRITICAL] Cluster router has no concurrency limit or timeout — scatter-gather spawns unbounded detached OS threads per request

**Location:** `tidal-server/src/cluster.rs:578-615 (build_cluster_router) and tidal-server/src/scatter_gather.rs:309-331 (dispatch_shards)`
**Dimension:** Accuracy · **Zone:** `tidal-server` · **Verifier:** CONFIRMED

**Issue:** The standalone router wraps protected routes in `TimeoutLayer` + `ConcurrencyLimitLayer::new(MAX_CONCURRENCY=100)` (router.rs:113-120), but `build_cluster_router` applies neither — it only adds a body limit and optional auth, then `public.merge(protected)` with no ServiceBuilder. So the cluster surface has no in-flight cap and no request timeout. Compounding this, every `/sharded/feed` and `/sharded/search` calls `dispatch_shards`, which spawns one *detached* `std::thread` per live shard per request (scatter_gather.rs:313-320). With no concurrency limit in front, a burst of sharded queries spawns shards×concurrent-requests detached OS threads with no global ceiling. The detached threads also outlive the request on a slow shard (by design), so threads accumulate faster than they retire. This is the exact 'no unbounded thread spawn' hazard called out for this zone, and it is on the un-throttled path.

**Why it matters:** At 3am under a query storm, the cluster node spawns threads without bound and never times out a wedged request — the node exhausts OS threads/memory and falls over instead of shedding load. The standalone node degrades cleanly (429/408) under the same load; the cluster node does not. The per-shard thread spawn already has a local degrade-on-spawn-failure path, but nothing caps the aggregate.

**Fix:**

Mirror the standalone middleware on the cluster protected router: wrap it in `ServiceBuilder::new().layer(TimeoutLayer::with_status_code(REQUEST_TIMEOUT, ...)).layer(ConcurrencyLimitLayer::new(MAX_CONCURRENCY))` before merging public routes. Promote `REQUEST_TIMEOUT_SECS`/`MAX_CONCURRENCY`/`BODY_LIMIT_BYTES` to a shared module so both routers consume the same constants. Additionally bound scatter-gather fan-out: replace the per-request detached `std::thread::spawn` with a shared bounded worker pool (the `ClusterWritePool` pattern already in offload.rs) or at minimum a semaphore so total concurrent shard workers are capped regardless of request concurrency.

---

### [CRITICAL] recover() silently truncates a corrupt non-final segment and replays past the resulting sequence gap with no error

**Location:** `tidal/src/wal/reader.rs:70-104 (the per-segment loop) and reader.rs:287-306 (recover_segment)`
**Dimension:** Accuracy · **Zone:** `wal-recovery` · **Verifier:** CONFIRMED

**Issue:** recover() calls recover_segment(seg_path)? for EVERY segment in seq order. recover_segment unconditionally truncates a torn/corrupt tail (last_valid_offset < total_len) on whatever segment it is handed, then recover() continues to the next segment and keeps appending its events into all_events / advancing next_seq. The doc-comment on recover_segment (lines 280-286) assumes only the active/last segment can be torn, but the loop applies it to every segment with no check that the corruption was in the final segment and no cross-segment sequence-continuity check. A sealed (already rotated+synced, per segment.rs:269-271) non-final segment that suffers silent bit-rot in the middle will: (1) have its tail truncated and discarded, (2) have all later segments replayed anyway, producing a state that is missing a contiguous run of sequences with a GAP, and (3) return Ok(RecoveryResult) from recover() with nothing surfaced — Wal::open (mod.rs:204) logs only 'recovery complete'. This is exactly the asymmetry the session journal already guards against (session_journal.rs:24-42, mod.rs:221-231 log corruption_detected), but the signal WAL — the actual source of truth for every entity/signal/relationship write — does not.

**Why it matters:** This is silent data loss on corrupt input in the durability-critical path. At 3am, an operator who lost the tail of a sealed segment to bit-rot gets a clean 'recovery complete' log, a database that is silently missing events, and a non-contiguous sequence space — with no signal that anything was dropped. The two-phase BLAKE3 validation correctly DETECTS the corruption; the bug is that recovery's RESPONSE (truncate + continue past it, return Ok) is only correct when the corruption is the torn tail of the final segment. For any earlier segment it converts detected corruption into invisible loss.

**Fix:**

Make recover() track whether each segment's scan was tail-corrupt and whether it is the final segment. Replace the blanket recover_segment(seg_path)? call with logic that: (a) for the LAST segment, truncates the torn tail as today (legitimate crash-mid-append); (b) for any NON-final segment that scans tail_corrupt, do NOT silently truncate-and-continue — instead either return WalError::Corruption (mirroring the sequence-overflow handling at lines 84-89) so open fails loudly and an operator can intervene, or at minimum record a corruption flag on RecoveryResult and have Wal::open emit a tracing::error! exactly as the session-journal path does (mod.rs:221-231). Concretely: use scan_segment_summary (which already exposes tail_corrupt without truncating) inside recover() to detect the condition before deciding to truncate, and add a cross-segment check that next_seq from segment N equals header.first_seq of segment N+1's first batch; surface a gap as Corruption. Add a recovery test that writes two segments, corrupts the FIRST one's tail, and asserts recover() either errors or sets a corruption signal — not that it silently returns the second segment's events.

---


## WARNING (32) — real issues, can merge but fix soon

### [WARNING] Cohort resolver cache mutex uses .expect() (panic) on the signal-write hot path instead of the codebase's poison-recovery pattern

**Location:** `tidal/src/cohort/resolver.rs:77, 95, 109, 132, 145, 154`
**Dimension:** Accuracy · **Zone:** `cohort-load-misc` · **Verifier:** CONFIRMED

**Issue:** Six production (non-test) call sites lock the LRU cache with `.lock().expect("cohort resolver cache poisoned")`. CohortResolver::resolve is reached on every cohort-attributed signal write (db/signals.rs:542). If any thread ever panics while holding this lock, the mutex is poisoned and every subsequent resolve/invalidate/cache_len call panics too — converting a one-off panic into a cascading, write-path-wide outage. This violates CODING_GUIDELINES §7 ("no expect() outside of tests and initialization; panics in a database corrupt state") and is inconsistent with the rest of the crate, which recovers from poison via unwrap_or_else(PoisonError::into_inner) (testing/faults.rs:54-55, testing/cluster.rs:308-309, and many db/ sites). The Debug impl here already does the right thing with try_lock; the hot-path methods do not.

**Why it matters:** At 3am, a single panic in any future cohort code path (or an allocation failure under memory pressure) would poison this lock and take down all cohort-attributed signal writes cluster-wide, not just the one request that tripped it. The cache is explicitly a pure accelerator (a miss is recomputed), so a poisoned lock has zero correctness value to defend — recovering into_inner is strictly safe and is already the house pattern.

**Fix:**

Replace each `.lock().expect("cohort resolver cache poisoned")` with `.lock().unwrap_or_else(std::sync::PoisonError::into_inner)`, matching testing/faults.rs and the db/ modules. The cache's critical sections are short and panic-free, so resuming with the recovered guard cannot observe a torn LRU; a future panic-during-resolve then degrades to a cache miss instead of a write-path outage. Update the `# Panics` doc sections to state the methods no longer panic on poison.

---

### [WARNING] write_signal reconstructs the WAL payload inline instead of calling the single_event_payload helper built to prevent exactly this drift

**Location:** `tidal/src/testing/cluster.rs:428-434 vs tidal/src/testing/cluster_transport.rs:95-107`
**Dimension:** DRY · **Zone:** `cohort-load-misc` · **Verifier:** CONFIRMED

**Issue:** single_event_payload (cluster_transport.rs:95) was extracted with a doc comment stating its purpose: "Both the eager ship path (SimulatedCluster::write_signal) and the recovery path (redeliver_missed) emit exactly the same payload shape ... Sharing the construction here keeps the two sites from drifting on the load-bearing event_count/leader_last_seq invariants (the receiver's monotonic advance relies on leader_last_seq == seqno)." redeliver_missed calls it; write_signal does not — it hand-builds WalSegmentPayload { id, bytes, event_count: 1, leader_last_seq: seqno } inline. The two payload constructions can now silently diverge on the very invariants the helper exists to lock together.

**Why it matters:** This is the harness that validates crash/replication correctness; if a future edit changes the payload shape in one site (e.g. multi-event batches, a different leader_last_seq derivation) and not the other, replication convergence in tests would break or — worse — pass against a subtly wrong payload, masking a real receiver bug. The helper's own doc claims both sites share it; the code contradicts the doc.

**Fix:**

In write_signal, replace the inline `let payload = WalSegmentPayload { id: WalSegmentId::new(crate::replication::RegionId::SINGLE, leader_shard, seqno), bytes: bytes.clone(), event_count: 1, leader_last_seq: seqno };` with `let payload = single_event_payload(leader_shard, seqno, bytes.clone());` (importing single_event_payload alongside the other cluster_transport imports). This makes the helper's drift-prevention guarantee actually true.

---

### [WARNING] validate() accepts a regular file (or any non-directory) as data_dir / wal_dir / cache_dir

**Location:** `tidal/src/db/builder.rs:237-252`
**Dimension:** Completeness · **Zone:** `db-core` · **Verifier:** CONFIRMED

**Issue:** The directory loop checks only `dir.exists()` and `permissions().readonly()`. If a caller passes a path that exists but is a regular file (e.g. `with_data_dir("/etc/hosts")`), validation passes; the failure surfaces much later and far less clearly when fjall/WAL try to create children under a non-directory, producing an opaque io error rather than a typed ConfigError at the open boundary.

**Why it matters:** The builder's stated contract is to return a specific typed ConfigError per failure at the single open boundary; a non-directory path defeats that and yields a confusing downstream error for an operator misconfiguration.

**Fix:**

In the dirs_to_check loop, after the exists() check add: `if !dir.is_dir() { return Err(ConfigError::DirectoryNotFound { path: dir.clone() }); }` (or a new `ConfigError::NotADirectory { path }` variant for an even clearer message). This keeps all path-shape validation at the same boundary as the existing checks.

---

### [WARNING] Writability validation via permissions().readonly() can pass for a directory the process cannot actually write

**Location:** `tidal/src/db/builder.rs:241-251`
**Dimension:** Accuracy · **Zone:** `db-core` · **Verifier:** CONFIRMED

**Issue:** `m.permissions().readonly()` on Unix reflects only the inode's write-permission bits, not whether THIS process's uid/gid can write. A directory owned by another user with mode 0755 reports readonly()==false, so validation passes, and the real EACCES only appears later when the WAL/fjall attempt to create files. The inline comment already concedes "a more robust check would attempt to create a temp file" but ships the weaker one.

**Why it matters:** The NotWritable error exists precisely so the operator learns at open() that the directory is unusable. As written it can pass validation and then fail opaquely during component init, defeating the intent for the most common real-world cause (wrong ownership).

**Fix:**

Probe writability for real: attempt to create+remove a uniquely-named temp file in the directory (e.g. `tempfile::Builder::new().tempfile_in(dir)`), mapping any failure to ConfigError::NotWritable { path }. This is the same approach the lock-file open already relies on and removes the false-positive. If the temp-file probe is considered too heavy for cache_dir (reserved/no-op today), at minimum apply it to data_dir and wal_dir, the two paths the engine actually writes.

---

### [WARNING] signal_with_context narrows item id with a bare `as u32`, silently aliasing over-range item IDs in hard-neg/seen/durable user-state rows

**Location:** `tidal/src/db/signals.rs:344`
**Dimension:** Accuracy · **Zone:** `db-entities` · **Verifier:** CONFIRMED

**Issue:** `let item_u32 = entity_id.as_u64() as u32;` truncates the item id with no warning, then feeds it to `hard_negatives.add`, `mark_seen`, `add_save_durable`/`add_like_durable`, and `hard_negatives.persist`. Every sibling path that narrows an item id to its u32 slot was hardened to be observable: `write_item_with_metadata` REJECTS an over-range id (items.rs:64-71), and the `Hide` relationship path routes through `narrow_item_slot` which emits a `tracing::warn!` on overflow (relationships.rs:53/105 -> user_state.rs:50-65). This one path is the last silent truncation, and the in-code comment at items.rs:59-63 explicitly flags it as a deferred gap. Because the truncated id is also written into the DURABLE Tag::HardNeg and Tag::UserState rows (hard_neg_key/user_state_key take the already-narrowed value), two distinct items whose ids share their low 32 bits collide permanently on disk and on restore — a wrong-results path (an item the user never rejected gets excluded, or a save/like attaches to the wrong item) with zero operator visibility.

**Why it matters:** Silent cross-id collision in user-correctness primitives (hard-negatives, seen, saved/liked) is exactly the class of bug that is invisible until a 3am 'why is this item missing from this user's feed' incident, and the durable rows make it survive restart. Consistency with the already-hardened Hide and item-write paths is the explicit project contract.

**Fix:**

Route the narrowing through the existing observable helper instead of a bare cast: `let item_u32 = crate::entities::user_state::narrow_item_slot(entity_id.as_u64());` (make it `pub(crate)` reachable from db). That logs a warn on overflow, matching items.rs and the Hide path, and removes the last silent truncation. Better still, since `signal_with_context` is a public entry point and the durable rows it writes are permanent, mirror `write_item_with_metadata` and reject an over-range item id up front with `TidalError::invalid_input` so a colliding row never lands on disk; then drop the per-call narrowing comment once all item-side write paths share the one guard.

---

### [WARNING] Export session-journal stream stops at the first frame that yields no events, silently dropping all later sessions

**Location:** `tidal/src/db/export.rs:614-619 (let events = decode_session_events(&frame); if events.is_empty() { break; })`
**Dimension:** Accuracy · **Zone:** `db-export` · **Verifier:** CONFIRMED

**Issue:** read_session_journal_signals streams one length-prefixed frame at a time and treats an empty decode result as corruption -> break. But decode_session_events returns an empty vec not only on corruption: a valid v2 frame carrying a forward-compatible unknown record type is intentionally Skipped by decode_typed_record (session.rs:424-429, RecordOutcome::Skipped, no event pushed), so a single-frame decode of such a record returns []. The whole-buffer decoder correctly skips that frame and continues; this streaming reader instead breaks and drops every subsequent session's signals from the export. Today only 3 record types exist (Start/Signal/Close), so this is latent — it bites the instant a newer writer adds a 4th type to the journal, exactly the forward-compat path the v2 framing was designed to tolerate. (Close frames are not affected — Close pushes an event.)

**Why it matters:** Silent, partial data loss in an RLHF/training export that downstream tooling assumes is complete. It is invisible until a schema/format evolution, and then it corrupts every export from older readers with no error. It also undermines the very purpose of the v2 forward-compat skip.

**Fix:**

Distinguish 'frame skipped' from 'frame corrupt'. Use decode_session_events_with_diagnostics(&frame) per frame: continue on a clean skip (events empty, corruption_detected false) and only break when outcome.corruption_detected is true. Add a test that writes a journal with a Start, an unknown-but-checksum-valid v2 frame, then a Signal, and asserts the trailing Signal is still exported.

---

### [WARNING] FOR SESSION missing-session handling diverges between retrieve and search

**Location:** `tidal/src/db/query_ops.rs:111-125 (retrieve) vs 272-289 (search)`
**Dimension:** Completeness · **Zone:** `db-ops` · **Verifier:** CONFIRMED

**Issue:** For the same user-facing clause (FOR SESSION <id>), retrieve hard-errors when the session is absent — returns Err(TidalError::Query(QueryError::SessionNotFound(...))) at lines 117-121 — while search logs tracing::warn! and continues without the session boost (lines 278-285). Two query types, identical clause, opposite failure modes.

**Why it matters:** CODING_GUIDELINES §6 'Graceful degradation, never failure' states the engine must 'never return an empty result set or an error for a well-formed query.' A RETRIEVE with FOR SESSION pointing at an expired/evicted session is a well-formed query; failing it hard contradicts the guideline AND surprises a caller who got degradation from the structurally-identical SEARCH path. A client that pages a feed with FOR SESSION will see SEARCH keep working but RETRIEVE start 500-ing the moment the session is swept — a confusing, surface-specific outage.

**Fix:**

Unify on the graceful-degradation behavior already implemented in search: in retrieve, replace the Err(SessionNotFound) arm (lines 117-121) with the same warn-and-continue path — on session_snapshot Err, log tracing::warn!(session_id, error, "FOR SESSION: session not found; executing without session boost") and fall through to base_executor. If a strict mode is genuinely wanted, make it an explicit query option that BOTH retrieve and search honor identically, rather than an accidental divergence.

---

### [WARNING] signal_for_tenant commits the local half then returns Err — a retry double-counts the local signal

**Location:** `tidal/src/db/replication_ops.rs:306-353`
**Dimension:** Accuracy · **Zone:** `db-ops` · **Verifier:** CONFIRMED

**Issue:** When a tenant is in dual-write and write_assignments resolves [local, remote] (source local, target remote), the loop writes the local half via self.signal(...) (line 324) and only then hits the remote assignment and returns Err::Internal (lines 341-349). The method is non-atomic: it returns an error to the caller after a durable, weight-accumulating signal write already landed. signal()/record_signal accumulates (it is not idempotent — confirmed at db/signals.rs:120-159), so a caller that treats the Err as 'nothing happened' and retries will apply the local weight twice. The test at tests/m8p5_multitenancy.rs:307-323 asserts this partial-write-then-Err is the intended contract.

**Why it matters:** A signal ledger is durability-critical state; a partial-success-reported-as-failure is a classic source of silent inflation. The standard caller reaction to Err is retry, and every retry adds another local view/like to the decay accumulator while the operation still never succeeds (remote stays unwired). The docstring documents the fail-closed intent but does not warn the caller that the local write is non-rollback-able, so the retry-amplification hazard is undocumented at the call site.

**Fix:**

Make the partial-write contract explicit and safe: (a) detect a non-local assignment BEFORE writing anything — iterate assignments once to check all are local, and if any is remote return Err immediately with zero writes (fully fail-closed, no partial mutation); the local write then only happens when the whole op can succeed. This removes the retry-double-count entirely. If the 'local half still applied' behavior is deliberately wanted, change the return to a typed PartialApplied { local_written: true } variant (not a bare Internal) so callers can distinguish 'retry is safe' from 'retry double-counts', and update the test to assert the typed outcome.

---

### [WARNING] rebuild_item_indexes indexes a truncated, colliding id for entity_id > u32::MAX instead of skipping it

**Location:** `tidal/src/db/state_rebuild.rs:382-396`
**Dimension:** Tech Debt · **Zone:** `db-recovery` · **Verifier:** CONFIRMED

**Issue:** When a scanned item row has entity_id > u32::MAX the rebuild logs a warning (lines 383-388) and then STILL truncates to u32 and inserts it into the universe bitmap and every index (lines 389-396) under a colliding lower id. The live write path takes the opposite, safer stance: it REJECTS any item id > u32::MAX BEFORE any durable write (tidal/src/db/items.rs:64-71, 'so a rejected item never lands in storage in an unindexable state'). Because the write path refuses such rows, the only way one reaches the rebuild is a corrupt or hostile durable store — and in exactly that case the rebuild silently corrupts the in-memory universe/indexes by aliasing a real lower id, producing phantom query hits, rather than skipping the bad row.

**Why it matters:** On corrupt/hostile on-disk input the rebuild manufactures cross-id collisions in candidate generation (a query for the colliding low id returns the wrong entity's metadata), which is a silently-wrong-results defect — the kind of corruption-driven misbehavior this database must fail safe against. The comment already admits 'universe bitmap entry will collide with a lower ID' yet proceeds anyway.

**Fix:**

Mirror the write path's refusal: after the warning, `continue;` to skip the row entirely rather than insert a collision. Replace lines 382-396 so the over-limit branch logs and skips:
```rust
if entity_id.as_u64() > u64::from(u32::MAX) {
    tracing::warn!(entity_id = entity_id.as_u64(),
        "entity ID exceeds u32::MAX during rebuild; skipping (write path never persists such ids)");
    continue;
}
let id_u32 = entity_id.as_u64() as u32;
```
This keeps the rebuild's invariant identical to the write path: an item that cannot be safely u32-indexed is never indexed, instead of being indexed under a colliding id.

---

### [WARNING] Orphaned session start records are detected on every open but never deleted or sealed into a snapshot — a permanent re-warned leak when the WAL no longer carries the session

**Location:** `tidal/src/db/session_restore.rs:60-70 (restore_sessions, b"start" arm)`
**Dimension:** Completeness · **Zone:** `db-sessions` · **Verifier:** CONFIRMED

**Issue:** When restore_sessions() finds a b"start" record (a session active at last shutdown), it logs a warning and increments `orphaned`, but takes no remediating action. If the same session is also present as an open session in the WAL, restore_session_wal_events rehydrates it and a later normal close deletes the start key (sessions.rs:184-188). But if the WAL session events are gone — the session journal is append-only with NO rotation/compaction (confirmed: session_journal.rs has no truncate/compact, and the prior review flagged its unbounded growth) and recover() stops at the first corrupt/truncated record (session_journal.rs:104-119), dropping all later events — the storage start record has no corresponding WAL Start and is never rehydrated, never closed, and never deleted. It is then re-detected and re-warned as orphaned on every subsequent open, forever.

**Why it matters:** Operationally this is a permanently-firing warning that trains operators to ignore session-restore warnings (alert fatigue at 3am), plus a small unbounded accumulation of dead start records in the items keyspace. With the BLOCKER above it compounds: the orphaned id is also not reflected in next_session_id, so a future session can reuse it.

**Fix:**

In restore_sessions(), after the full scan, for each orphaned start record whose session_id is NOT in the set restored as active from the WAL, seal it deterministically: build a frozen closed-session snapshot (duration computed from started_at_ns, zero in-memory signals — explicitly marking the lost-state condition) and write it via a WriteBatch that put()s the snapshot key and delete()s the start key atomically, exactly as close_session_internal does (sessions.rs:178-192). That converts the unrecoverable active session into an archived record once, stops the recurring warning, and reclaims the key. Advance next_session_id past its id as part of the BLOCKER fix.

---

### [WARNING] join_community / rejoin_community do a non-atomic read-then-put on the membership registry, racing a concurrent leave

**Location:** `tidal/src/db/communities.rs:42-78 (join_community) and 156-174 (rejoin_community)`
**Dimension:** Tech Debt · **Zone:** `db-sessions` · **Verifier:** CONFIRMED

**Issue:** join_community reads the prior row via membership_registry.with_row(...) (a DashMap::get + clone of (state,epoch,joined_at_ns)), drops the guard, builds a new Membership, persists it, then calls membership_registry.put(membership) which replaces the row. Between the read and the put, a concurrent leave_community can engage the stop-forward gate and put a Left row; join's subsequent put overwrites it with a fresh Joined/Rejoined row whose stop_forward_at_ns is 0 — silently re-admitting a member who just left, with no error to either caller. leave_community itself is careful (durability-before-mutation, engages the gate on a snapshot copy then persists then publishes), but it gives no protection against a join that read the pre-leave row. The DashMap entry is not held across the persist, so this is a genuine lost-update window, not just a theoretical one.

**Why it matters:** Community membership governs whether a user's contributions flow to a shared overlay (a privacy/consent boundary). A lost-update that re-admits a just-left member silently defeats the stop-forward guarantee the rest of communities.rs documents as '<1s, by construction'. The window is small but the failure is a privacy regression, not a transient count error.

**Fix:**

Serialize the read-modify-write per (user, community). Either (a) hold the DashMap entry across the whole transition by using membership_registry.entry((user,community)) / a with_row_mut that takes &mut Membership and performs persist inside the held guard (accepting persist-under-lock for this cold lifecycle path), or (b) add a per-key lifecycle mutex (a sharded Mutex keyed by (user,community)) that join/leave/rejoin all acquire before their read-modify-write. Document the single-writer-per-(user,community) invariant on MembershipRegistry either way. The signal write path (community_contribute) stays lock-free; only the rare lifecycle transitions take the lock.

---

### [WARNING] PreferenceVectors::restore neutralizes NaN but leaves a non-unit vector, defeating its own cosine-poisoning guard

**Location:** `tidal/src/entities/preference.rs:306-343 (NaN handling at 334; insert at 339) vs cosine_similarity at 191-205`
**Dimension:** Accuracy · **Zone:** `entities` · **Verifier:** CONFIRMED

**Issue:** restore() reads each f32 and does `vec.push(if f.is_nan() { 0.0 } else { f })`, then `self.inner.insert(user_id, vec)` WITHOUT re-normalizing. The comment at line 333-334 says it neutralizes NaN 'so a torn row cannot poison cosine scoring downstream.' But cosine_similarity (191-205) computes `dot / candidate_norm` and explicitly relies on the stored preference already being unit-length ('The stored preference is already unit-length, so we only need to normalize the candidate side'). When a torn/garbled checkpoint row has one or more NaN components, zeroing them produces a vector whose L2 norm is NOT 1.0, so cosine_similarity returns a mis-scaled similarity (can exceed 1.0 or be arbitrarily wrong) — exactly the poisoning the comment claims to prevent. Even a fully-finite-but-tampered row that is not unit-length is inserted verbatim.

**Why it matters:** This is the corrupt-input boundary the DATABASE-CRITICAL lens weights heavily. A single torn preference row silently distorts personalized ranking scores for that user with no error and no observability, and the code gives a false sense of safety via a comment that overstates what the NaN check buys.

**Fix:**

Re-establish the unit-length invariant at the load boundary instead of trusting the stored bytes. After building `vec` (with NaN already zeroed), call `l2_normalize(&mut vec)` before `self.inner.insert(...)` — l2_normalize already leaves an all-zero vector untouched, so a fully-corrupt row degrades to a zero pref (cosine returns 0.0) rather than a poisoned non-unit one. This matches set()'s contract (which normalizes) and makes the comment true.

---

### [WARNING] User-purge and agent-purge paths are asymmetric: one re-checkpoints eagerly, the other does not

**Location:** `tidal/src/db/communities.rs:457-486 (purge_prior_contributions) vs tidal/src/db/remove_scope.rs:60-87 (CommunityAgent)`
**Dimension:** Tech Debt · **Zone:** `governance` · **Verifier:** CONFIRMED

**Issue:** The agent-scope purge persists its tombstone then immediately re-checkpoints the community ledger (remove_scope.rs:80-87, 'belt-and-suspenders with the tombstone replay'). The user-scope purge (communities.rs:474-481) persists the tombstone and mutates memory but never re-checkpoints, relying solely on tombstone replay. Both are crash-correct (db/mod.rs:613-618 replays user tombstones on open and purge is idempotent), so this is not a durability defect — but two paths solving the identical problem two different ways invites a future editor to 'fix' one to match the other and accidentally remove a guarantee, or to assume the user path also re-checkpoints.

**Why it matters:** Divergent handling of the same crash window is a maintenance hazard in the most safety-critical code in the zone; the next person must re-derive that both are correct rather than reading one consistent pattern.

**Fix:**

Pick one policy and apply it to both. Preferred: drop the eager re-checkpoint from the agent path too (tombstone replay already covers the window for both), and add a one-line comment on each path stating 'durability is provided by tombstone replay on open; the periodic/lifecycle checkpoint GCs the stale rows'. If eager re-checkpoint is genuinely wanted, factor a shared helper that persists-tombstone + mutates + (optionally) re-checkpoints so the two callers cannot drift.

---

### [WARNING] RRF fuse() tie order is non-deterministic across process runs (no entity_id tie-break)

**Location:** `tidal/src/query/fusion.rs:92-97`
**Dimension:** Accuracy · **Zone:** `query-suggest-fusion` · **Verifier:** CONFIRMED

**Issue:** fuse() collects scores from a HashMap<u64, f64> (default RandomState, per-process randomized iteration order) into a Vec, then sorts with `results.sort_by(|a, b| b.1.partial_cmp(&a.1).unwrap_or(Ordering::Equal))`. There is no secondary tie-break on entity_id, and sort_by is stable, so any two entities with an equal fused score keep whatever relative order the randomized HashMap iteration produced — which changes between process runs. Downstream the fused order flows into `candidates` (search/executor/pipeline.rs:162), and the final score sort (query/executor/helpers.rs:260) is again `partial_cmp(...).unwrap_or(Equal)`, a stable sort that PRESERVES the random tie order into the returned results. So two identical Hybrid queries over identical data — e.g. before and after a restart — can return tied results in different order. This is the exact determinism class already hardened for in the BM25 sort (pipeline.rs:346 neutralizes NaN and uses total_cmp 'so the result is deterministic'), but fusion was missed.

**Why it matters:** A ranking database that returns different orderings for the same query on the same data breaks reproducibility, makes relevance/A-B debugging non-falsifiable, and can drop or duplicate rows under keyset pagination across restarts when adjacent rows tie on score and the unstable global tie order disagrees with the cursor's assumption of a fixed total order. RRF is documented as the robust, deterministic fusion baseline; silent run-to-run reordering violates that contract.

**Fix:**

Break ties on entity_id to make the order a true total order, mirroring the BM25 fix: `results.sort_by(|a, b| b.1.partial_cmp(&a.1).unwrap_or(std::cmp::Ordering::Equal).then_with(|| a.0.as_u64().cmp(&b.0.as_u64())));` (RRF terms are always finite positive, so partial_cmp never sees NaN here — the only missing piece is the id tie-break). Add a test fusing inputs crafted to produce equal scores asserting ascending-by-id tie order, plus a proptest that fusing identical inputs twice yields identical output. Apply the same `.then_with(id)` tie-break to the final score sorts in query/executor/helpers.rs:260 and search/executor/pipeline.rs so determinism holds end-to-end, not just inside fuse().

---

### [WARNING] Ranking still reads wall-clock Timestamp::now() per candidate instead of the threaded query clock; scores are not reproducible

**Location:** `tidal/src/ranking/executor/helpers.rs:54-102 (read_agg) and tidal/src/ranking/executor/scoring.rs:69-149 (score_by_sort); driven by the dead `now` param in tidal/src/ranking/executor/mod.rs:192-197, 501-509`
**Dimension:** Accuracy · **Zone:** `ranking` · **Verifier:** CONFIRMED

**Issue:** Every time-dependent read in the scoring pipeline — SignalAgg::Value (read_windowed_count), Velocity (read_velocity), DecayScore (read_decay_score), and the ProfileDecay recency factor — goes through the ledger's wall-clock wrappers, each of which calls Timestamp::now() internally (core.rs:214, 281, and read_velocity at core.rs:325 via read_windowed_count). The `now: Timestamp` argument threaded through score()/score_with_session()/score_personalized()/compute_raw_score()/score_by_sort() is never passed to any ledger read. So within a single query each candidate is decayed/aged to a slightly different clock, and re-running the identical query against an unchanged ledger yields different scores. This is the zone's special-focus item ('decay must read the query clock, not Timestamp::now()'). The ledger already grew the explicit-clock variants read_decay_score_at (core.rs:233) and read_windowed_count_at (core.rs:296) in a prior remediation, but they were never wired into the executor, and the mod.rs:181-184 doc comment still claims this 'requires a read_decay_score_at on the ledger (out of scope)' — which is now factually false.

**Why it matters:** For a ranking database, reproducibility of a scored result set is a correctness property: A/B replay, debugging 'why did this item rank here at 3am', and the prior review's own goal of making scoring a pure function of (ledger snapshot, now) all depend on it. The blast radius is bounded (sub-microsecond per-candidate clock skew rarely flips adjacent ranks, and ordering is otherwise correct), so this is a WARNING, not a CRITICAL — but the parameter is live, misleading dead weight and the remediation is now cheap because two of the three needed _at methods already exist.

**Fix:**

Thread `now` into the reads. In compute_raw_score/score_by_sort, capture now_ns = now.as_nanos() once and call read_decay_score_at / read_windowed_count_at instead of the wall-clock wrappers; add the missing read_velocity_at(entity, signal, window, now_ns) to SignalLedger (it is a one-line wrapper over read_windowed_count_at / duration_secs) and route SignalAgg::Velocity through it. Update read_agg/read_agg_for_sort/passes_gates/passes_excludes to take now_ns. Then correct the mod.rs:181-184 and scoring.rs:58-61 doc comments to state the clock is honored. Add a determinism test: score the same candidates twice against an unchanged ledger and assert byte-identical scores.

---

### [WARNING] gc_source transitions to Complete but never reclaims source-shard data — the name and 'GC window' docstring overstate behavior

**Location:** `tidal/src/replication/migration.rs:216-241 (gc_source), 327-335 (TenantRouter::finalize_migration doc: 'The old source shard can be GC'd')`
**Dimension:** Completeness · **Zone:** `repl-cluster` · **Verifier:** CONFIRMED

**Issue:** gc_source (222-241) checks the elapsed GC window then flips MigrationState::Finalizing -> Complete. It performs no storage delete, no shipper teardown, no source-shard reclamation — it is a pure in-memory state flip. finalize_migration's doc (tenant.rs:331) asserts 'The old source shard can be GC'd', and gc_source's name implies it does the GC. After a real migration the source shard's tenant data lives forever; nothing in this zone removes it.

**Why it matters:** An operator reading these names/docs will believe source data is reclaimed after the window, freeing disk and removing the stale copy that could otherwise be served if routing ever reverts (see the finalize finding). The unbounded retention is also a correctness hazard: a stale source copy + a routing-revert bug = serving old data. Silently doing nothing where the API name promises reclamation is the kind of gap that bites during a capacity incident.

**Fix:**

Either (a) implement the reclamation: on gc_source, after the window, delete the tenant's source-shard keyspace (WriteBatch delete-range + flush) and tear down any source-shard shipper/dual-write plumbing; or (b) rename to mark_complete / advance_to_complete and amend the docstrings to state explicitly that source-data reclamation is a separate, not-yet-wired step (with the spec task reference, per CODING_GUIDELINES §12). Given durability is paramount, (a) is the right end state; (b) is acceptable only as an interim with an honest doc.

---

### [WARNING] tenant_wal_dir is dead code — per-tenant storage/WAL isolation does not exist; signal_for_tenant writes all tenants into one shared ledger

**Location:** `tidal/src/replication/tenant.rs:538-549 (tenant_wal_dir, zero non-test callers), tidal/src/db/replication_ops.rs:306-353 (signal_for_tenant -> self.signal)`
**Dimension:** Completeness · **Zone:** `repl-cluster` · **Verifier:** CONFIRMED

**Issue:** tenant_wal_dir builds per-tenant WAL paths (/data/tenants/{id}/wal) but has no production caller (grep: only its own two unit tests). signal_for_tenant enforces the per-tenant rate limiter and resolves shard assignments, then for the local shard calls self.signal(...) (replication_ops.rs:324), which writes to the single shared SignalLedger/entity store with NO tenant namespace in the key. So on a single node hosting multiple tenants, all tenants' signals land in one keyspace; tenant_id influences only rate limiting and (multi-node) routing, not at-rest data isolation. The module header (mod.rs:3-4) frames replication as a no-op single-node, but tenant.rs presents tenant_wal_dir as if storage isolation were implemented.

**Why it matters:** 'Multi-tenant isolation' is the headline concern for this zone. Presenting a tenant_wal_dir helper while no write path uses it invites a maintainer to assume isolation holds, when a single-node multi-tenant deployment actually co-mingles every tenant's data and WAL. A noisy/abusive tenant is not storage-isolated from others on the same node, contradicting the spirit of TenantConfig's max_storage_bytes/max_entities (which are also never enforced).

**Fix:**

Pick one and make it true: (a) wire tenant_wal_dir and a tenant key-prefix into the write path so each tenant gets an isolated keyspace/WAL and the TenantConfig limits are enforced; or (b) delete tenant_wal_dir and the implied-isolation doc, and document in signal_for_tenant that on-node tenant isolation is logical (rate/routing) only, not physical, with the spec task that will add it. Do not leave a dead isolation helper that implies a guarantee the write path does not provide.

---

### [WARNING] LWWRegister::merge is non-commutative under an identical HlcTimestamp with differing values (unstated precondition)

**Location:** `tidal/src/replication/crdt/lww_register.rs:36-38, 102-109 (merge); property test lww_register.rs:462-472`
**Dimension:** Accuracy · **Zone:** `repl-crdt` · **Verifier:** CONFIRMED

**Issue:** `lww_other_wins(cur, inc)` returns true only when `*inc > cur`; equality keeps the current value. The ordering key is the full HlcTimestamp (wall+logical+node_id), which excludes the payload. So if two registers carry the SAME HlcTimestamp but DIFFERENT values, merge keeps whichever is `self`: merge(a,b) and merge(b,a) diverge. I confirmed this with a standalone repro of the exact merge logic — merge(a,b).value=Some(1), merge(b,a).value=Some(2). The `merge_is_commutative` proptest asserts this law for 'all registers' with a timestamp domain of only ~1.1M points (wall 0..=1e6, logical 0..=100, node 0..=10) and independent `any::<u8>()` values, so a collision seed would falsify the asserted law; it passes only because the sampler has not hit the exact same-key/different-value pair in 256 cases.

**Why it matters:** This is a state-based CRDT whose entire purpose is deterministic post-partition reconciliation by merging untrusted peer state over the wire. A CRDT merge that is order-dependent for any reachable input cannot guarantee replica convergence — two replicas applying the same set of registers in different orders would settle on different values, silently. For honest nodes the per-node HLC uniqueness invariant makes the collision unreachable (a node never re-emits a (wall,logical) pair; distinct nodes differ in node_id), so this is not a live data-loss bug today — but it is an unenforced, undocumented soundness precondition, and the proptest claims a stronger guarantee than the code actually provides. A corrupt/forged peer register or a future HLC change that weakens uniqueness turns this latent gap into real divergence.

**Fix:**

Make the tie-break total by folding the value into the decision, OR document+enforce the precondition. Cleanest: in `LWWRegister<T>` constrain `T: Ord` and break exact-timestamp ties on the value deterministically (e.g. keep the larger value), so equal keys resolve identically regardless of merge order — mirroring how `CrdtSignalState::merge` already appends the score to its key tuple `(last_update_ns, score)` and is therefore commutative. If a total `T` ordering is undesirable, instead (a) add a debug_assert/Internal-error guard that a same-key merge with a differing value is an invariant violation, and (b) harden the proptest generator to deliberately sample colliding timestamps with differing values so the asserted commutativity law matches the code's real guarantee rather than passing by sampling luck.

---

### [WARNING] Embedding-slot dimension validation enforces only != 0, not the documented [2, 4096] bound

**Location:** `tidal/src/schema/validation/builders.rs:317-322 (and SchemaError::InvalidEmbeddingDimensions at schema/error.rs:272-273)`
**Dimension:** Completeness · **Zone:** `schema` · **Verifier:** CONFIRMED

**Issue:** build() rejects dimensions == 0 but accepts every other usize, including 1 (the spec's EmbeddingSlot doc and 11-schema.md V-E04 require [2, 4096]) and arbitrarily huge values up to usize::MAX. The dimension flows untouched into the vector registry/index (storage/vector/registry.rs:227, brute/mod.rs:221 cast it to u64/u32) and becomes the contract every persisted vector must match. The spec claims validation is 'eager and complete -- a definition that passes validation is guaranteed to be self-consistent', which this violates.

**Why it matters:** A dimension of 1 is a degenerate vector index; an enormous dimension is accepted at define-time and only surfaces much later as an allocation failure or a per-vector insert error, far from the schema call that caused it. The whole point of define-time validation is to fail at the schema boundary, not deep in the ANN path on first insert. It is developer-supplied (not hostile wire input), so this is quality, not a crash-on-hostile-input BLOCKER.

**Fix:**

In the embedding-slot validation loop, after the `== 0` check add an explicit range gate matching the documented contract: `if !(2..=4096).contains(&slot.dimensions) { return Err(SchemaError::InvalidEmbeddingDimensions { slot: slot.name.clone(), dimensions: slot.dimensions }); }`. If 4096 is genuinely too low for some model, lift the ceiling deliberately and update the spec/doc — but pick a bound and enforce it eagerly. Add reject tests for dim=1 and dim=4097 alongside the existing rejects_zero_dimension_embedding_slot.

---

### [WARNING] Reserved text-field key duplicated as two independent string literals coupled only by comment

**Location:** `tidal/src/schema/error.rs:318 (RESERVED_TEXT_FIELD_KEY = "entity_id") vs tidal/src/text/index.rs:28 (ENTITY_ID_FIELD = "entity_id")`
**Dimension:** DRY · **Zone:** `schema` · **Verifier:** CONFIRMED

**Issue:** The schema builder rejects a user text field named `entity_id` (builders.rs:361 compares against RESERVED_TEXT_FIELD_KEY) precisely because it would collide with Tantivy's always-present internal u64 field, whose name is independently defined as ENTITY_ID_FIELD in text/index.rs:488. The two constants are kept equal only by the comment 'Kept in sync with the field name in text::index::build_tantivy_schema'. They are not the same symbol — error.rs cannot import the text-module const without a layering edge, so the literal is copied.

**Why it matters:** If a future change renames the internal Tantivy field via ENTITY_ID_FIELD but not RESERVED_TEXT_FIELD_KEY, the define-time guard would protect against the stale name while the new name silently becomes a user-declarable text field again — reintroducing the exact `add_field` panic inside Tantivy's SchemaBuilder::build() at DB open that this guard exists to prevent. A comment is not a compile-time coupling.

**Fix:**

Make text::index::ENTITY_ID_FIELD the single source of truth and have the schema guard reference it: `if field.key == crate::text::index::ENTITY_ID_FIELD`. If the schema->text dependency direction is undesirable, hoist one shared `pub(crate) const ENTITY_ID_FIELD` into a neutral low-level module (e.g. storage/keys or a small constants module) that both schema and text import. Add a compile-time assertion or a test that asserts the two are equal so a future rename fails the build, not production open.

---

### [WARNING] SessionState's 14-field literal is hand-duplicated across both production construction sites with no constructor

**Location:** `tidal/src/db/sessions.rs:66-81 and tidal/src/db/session_restore.rs:140-156 (struct def tidal/src/session/state.rs:24)`
**Dimension:** DRY · **Zone:** `session` · **Verifier:** CONFIRMED

**Issue:** SessionState has 14 fields and no constructor; start_session (sessions.rs:66) and restore_session_wal_events (session_restore.rs:140) each spell out the full literal, including the five 'always the same' defaults (signals/signaled_entities DashMap::new(), annotations Mutex::new(Vec::new()), signals_written/signals_rejected AtomicU64::new(0), audit_log Mutex::new(AuditLog::new())). The two literals must be kept in lock-step by hand: adding a field (this struct has grown across milestones) means editing every site, and a divergence in an atomic's initial value or a missing reset would be a silent state bug. The same literal is also repeated in 3 test helpers.

**Why it matters:** This is the precise structural smell that let the started_at/started_at_ns divergence in the BLOCKER persist: the two construction sites are independently maintained, so the restore path seeding a non-durable started_at went unnoticed against the live path. Centralizing construction makes the durable-vs-monotonic field handling reviewable in one place.

**Fix:**

Add `SessionState::new(id, user_id, agent_id, policy_name, started_at, started_at_ns, metadata) -> Self` that fills the five empty-state fields once, and have both production sites call it. Better, give it `started_at_ns` and derive `started_at: Instant::now()` internally with a doc note that the Instant is for live metrics only and started_at_ns is the durable age source — encoding the BLOCKER's invariant structurally. Tests can use a `#[cfg(test)]` builder default.

---

### [WARNING] No tracing instrumentation anywhere in the storage zone, violating CODING_GUIDELINES §11.2

**Location:** `tidal/src/storage/fjall.rs:120-200 (StorageEngine impl), tidal/src/storage/memory.rs:40-117`
**Dimension:** Completeness · **Zone:** `storage-engine` · **Verifier:** CONFIRMED

**Issue:** CODING_GUIDELINES §11.2 names this exact boundary as non-negotiable: "Instrument the public API and the major internal stage boundaries: EntityStore::{get, put, scan_prefix}" and §11.1 calls #[tracing::instrument] on subsystem-crossing functions "non-negotiable — it is how query latency, signal write throughput, and WAL sync times are measured in production." `grep -rn instrument tidal/src/storage/` returns nothing. Neither StorageEngine impl carries a single span. flush()/write_batch() — the fsync and atomic-commit barriers the spec says you measure WAL sync times on — are completely dark.

**Why it matters:** At 3am during a production incident you cannot answer "is the slow query stuck in a storage get, a scan_prefix over a hot entity, or a flush fsync?" without spans. The guideline explicitly exists so this instrumentation does not have to be retrofitted under pressure. A restart that hangs in scan_prefix(&[]) (used by every state_rebuild path) is currently unobservable from traces.

**Fix:**

Add #[tracing::instrument(skip(self, value), fields(key_len = key.len()))] (and analogous for get/scan_prefix/write_batch/flush) to the FjallBackend StorageEngine impl methods, or — if per-call span overhead on get is a hot-path concern — instrument at the storage_box.rs dispatch layer and emit a tracing::debug! with the operation + byte counts on flush()/write_batch() at minimum. The guideline permits instrumenting at the stage boundary; pick one and wire it. Do NOT add tracing-subscriber (the crate is a library per §11.3) — import tracing only.

---

### [WARNING] rebuild_from_store aborts the whole entity kind on a single un-indexable embedding

**Location:** `tidal/src/storage/vector/registry.rs:399 (`slot.index.insert(entity_id.as_u64(), &vector)?`) and the `?` on deserialize at line 343`
**Dimension:** Completeness / Accuracy · **Zone:** `storage-vector` · **Verifier:** CONFIRMED

**Issue:** The rebuild loop propagates `?` on the per-embedding `index.insert` (and on `deserialize_embedding`). If even one stored EMB: value yields a vector whose length differs from the slot's first-seen dimensionality — e.g. a slot dim changed across a schema revision, a partially-written/corrupt header that still passes deserialize_embedding's self-consistent length check, or a hand-edited store — `BruteForceIndex::insert` returns DimensionMismatch and the entire loop aborts. db/mod.rs:785 then logs 'rebuild failed' and leaves ANN search for that whole entity kind empty, even though hundreds of thousands of perfectly good embeddings for the same kind were skipped. One poison vector takes down the kind.

**Why it matters:** This is the recovery path after a crash/restart — exactly when robustness matters most. The guideline (CODING_GUIDELINES §5, mirrored in the vector docs) is 'unconditionally rebuild from the store; any entity in the store but missing is re-indexed.' A single bad row defeating the rebuild for an entire kind violates that deterministic-recovery contract and is a silent, hard-to-diagnose degradation (ANN returns nothing for all Items) with only one error line to go on.

**Fix:**

Make the per-embedding body of the loop fault-isolating: on a deserialize error or an index.insert error for one (entity_id, slot), `tracing::warn!` with the id/slot and `continue` rather than returning the error, accumulating a skipped-count that is logged at the end. Reserve hard `Err` returns for whole-scan failures (storage.scan_prefix entry errors), which are genuinely unrecoverable. Add a test: seed a store with two valid embeddings and one whose stored header declares a different dimension under the same slot, rebuild, and assert the two valid ones are indexed and searchable while the bad one is skipped — proving one poison row no longer bricks the kind.

---

### [WARNING] Doc comments reference a non-existent module path for the rebuild-at-open primitive

**Location:** `tidal/src/text/writer.rs:100; tidal/src/db/items.rs:196; tidal/src/db/creators.rs:31`
**Dimension:** Tech Debt · **Zone:** `text` · **Verifier:** CONFIRMED

**Issue:** Three doc comments point readers to `db::mod::rebuild_text_indexes_at_open` (and writer.rs:100 says `db::mod::rebuild_text_indexes_at_open`). The function actually lives in `db::state_rebuild::rebuild_text_indexes_at_open` (state_rebuild.rs:761); `db/mod.rs:751` only *calls* it. The `rustdoc` intra-doc style backtick path does not resolve.

**Why it matters:** This is the single most load-bearing recovery invariant in the zone (it is why losing un-committed Tantivy writes on crash is safe). A maintainer following the cited path during an incident lands in the wrong file and may wrongly conclude the rebuild is unwired — exactly the silent-regression failure this design was written to prevent. The writer.rs reference is inside the reviewed zone.

**Fix:**

Update all three comments to `crate::db::state_rebuild::rebuild_text_indexes_at_open`. Prefer a real intra-doc link (`[crate::db::state_rebuild::rebuild_text_indexes_at_open]`) so `cargo doc` / clippy `broken_intra_doc_links` keeps it from rotting again.

---

### [WARNING] BM25 retrieval returns the entire match set with no top_k cap on the non-degraded path — unbounded allocation on a broad query

**Location:** `tidal/src/text/collectors.rs:22-103 (AllScoresCollector) consumed at tidal/src/query/search/executor/pipeline.rs:335-348`
**Dimension:** Completeness · **Zone:** `text` · **Verifier:** CONFIRMED

**Issue:** `AllScoresCollector` collects EVERY matching document (by design, so the ranker does top-K). But on the normal path nothing bounds the result: `retrieve_bm25` returns the full Vec and truncation only happens under `degradation_level.reduces_candidates()` (pipeline.rs:138-141). The spec's `top_k_text` default of 200 (06-text-retrieval.md Sec 11.2) is never enforced at steady state. A broad/common term over a 10M-doc corpus collects millions of `(EntityId, f32)` pairs (24 bytes each → tens of MB) plus a full-size HashMap at pipeline.rs:145, per query.

**Why it matters:** A single broad or hostile query (e.g. a near-ubiquitous token) drives an allocation proportional to corpus match count, not to the requested limit — a memory-amplification / latency-spike vector under load, and a divergence from the documented candidate-sizing contract. The collector itself is correct for its stated purpose, but the contract ('caller does top-K') is only honored when degraded.

**Fix:**

Bound the candidate set on the normal path too: either give `AllScoresCollector` an optional cap (a bounded min-heap of size `top_k_text`, defaulting to the spec's 200/configurable per profile) so it never materializes more than K, or apply a `top_k_text` truncation in `retrieve_bm25` after the sort regardless of degradation level. A bounded collector is preferable since it also caps peak memory inside Tantivy rather than after the fact.

---

### [WARNING] rustls-pemfile is a declared but unused dependency

**Location:** `tidal-net/Cargo.toml:38 (rustls-pemfile = "2"); no usage in tidal-net/src/`
**Dimension:** Tech Debt · **Zone:** `tidal-net` · **Verifier:** CONFIRMED

**Issue:** rustls-pemfile is listed as a direct dependency but grep finds no rustls_pemfile usage anywhere in tidal-net/src — TLS material is parsed via tonic's Identity::from_pem / Certificate::from_pem in tls.rs, which do their own PEM decoding. The crate pulls and compiles a dependency it never calls.

**Why it matters:** CODING_GUIDELINES §10 mandates 'minimal, intentional, auditable' dependencies, each justified against 'could we write this in 200 lines?'. An unused dep is unjustifiable by definition: it grows the build, the audit surface, and the supply-chain footprint for zero benefit, and misleads a future reader into thinking PEM parsing happens here.

**Fix:**

Remove `rustls-pemfile = "2"` from tidal-net/Cargo.toml (and the corresponding crate_universe pin if Bazel tracks it). Run `cargo build -p tidal-net` to confirm nothing breaks. If a future raw-PEM path needs it, re-add it at that point with the actual usage.

---

### [WARNING] Sharded write handlers run blocking DB writes directly on the async reactor

**Location:** `tidal-server/src/cluster.rs:892-936 (sharded_create_item / sharded_write_embedding / sharded_write_signal)`
**Dimension:** Accuracy · **Zone:** `tidal-server` · **Verifier:** CONFIRMED

**Issue:** The non-sharded cluster read/write handlers carefully offload blocking `TidalDb` work (reads via `offload_cluster_read`, writes via `write_pool.submit`). The three `sharded_*` write handlers instead call `crate::scatter_gather::sharded_write_item/embedding/signal` synchronously inside the async handler. Those functions do a blocking `cluster.node(shard).db.write_*` call (storage + WAL fsync) on the reactor thread. The module docs (offload.rs:5-12) explicitly state reads/writes are blocking and must be moved off the reactor; these handlers violate that.

**Why it matters:** A reactor worker is pinned for the full duration of a storage write + fsync. Under concurrent sharded-write load this starves every other in-flight request on that worker (the precise failure offload.rs was built to prevent). It is less severe than the cluster writes (no gRPC, single shard) but still blocks the reactor — and it is on the same un-throttled cluster router as the finding above.

**Fix:**

Route the sharded single-shard writes through `offload_read` (they touch only the local store, no gRPC, so spawn_blocking is sufficient) — e.g. `offload_read(move || sharded_write_signal(&cluster, ...)).await`, taking an owned `cluster_arc()` clone as the other cluster handlers do. This makes all three write paths consistent with the documented offload contract.

---

### [WARNING] Multi-process cluster_e2e test verifies nothing across process boundaries — every node runs all regions in-process

**Location:** `tidal-server/tests/cluster_e2e.rs:91-115, 311-373`
**Dimension:** Completeness · **Zone:** `tidal-server` · **Verifier:** CONFIRMED

**Issue:** `ClusterHarness::start(n)` spawns n separate `tidal-server cluster` OS processes, each given the same full topology. Each process therefore builds its own `ClusterState`/`SimulatedCluster` containing ALL n regions internally. The test only ever writes to and reads from `nodes[0]` (e.g. line 366 reads `region=region-1` from node 0, which holds region-1 internally). A write to node 0 never reaches node 1's process. The test 'passes' purely on node 0's in-process replication; it would pass identically with a single process. The comment at lines 417-419 even acknowledges G1.

**Why it matters:** This is the headline 'multi-process E2E' harness (commit b16025b), but it does not test process isolation, cross-node routing, or real network partition — the things the cluster claims to need verification for. Per the project standard ('Wiring is not verification'), this is wiring presented as an E2E proof. An operator reading the test name trusts a guarantee the test does not provide.

**Fix:**

Either (a) make each spawned process own exactly ONE region and have them peer over gRPC to real sibling addresses, then assert a write on node 0 becomes visible on node 1's process — the actual multi-process contract; or (b) rename/re-scope the test honestly (e.g. `single_process_cluster_smoke`) and mark the true multi-process E2E as an explicit known gap until G1/m8p10 land. Do not leave a no-assertion-across-processes test under the `cluster_e2e` name.

---

### [WARNING] scope-stats reports `complete: true` when WAL integrity could NOT be verified (corrupt checkpoint case)

**Location:** `tidalctl/src/commands/scope_stats.rs:151-152, 174`
**Dimension:** Accuracy · **Zone:** `tidalctl` · **Verifier:** CONFIRMED

**Issue:** `inconsistency_count = diagnose_wal(base).map_or(0, |r| r.inconsistency_count)` collapses a diagnose_wal FAILURE to 0, and line 174 then sets `complete: inconsistency_count == 0` → `true`. read_all_events (line 132) only reads segment files and never touches checkpoint.meta, so with a corrupt `checkpoint.meta` + otherwise-valid segments the tally succeeds while diagnose_wal errors out. The output then asserts `complete: true` ("tally covers every WAL batch") even though integrity was never actually proven. This is the SAME corruption condition that makes `status` and `diagnostics` exit 2 (verified by status_corrupt_wal_exits_2 / diagnostics_corrupt_wal_exits_2). The in-band integrity signal — the operator's entire go/no-go for trusting the numbers — flips to a false positive in exactly the degraded case it exists to flag. The code comment at lines 147-150 admits 'we cannot prove completeness' but the emitted field claims the opposite.

**Why it matters:** At 3am an operator runs `scope-stats` to decide whether the per-scope share-eligible tally can be trusted (it gates what data ships). `complete: true` is a positive integrity assertion; producing it when integrity is unknown is silently-wrong reporting on a durability-adjacent surface. A consumer branching on `complete` (the field's own rustdoc invites this) will trust an unverified tally precisely when the store is corrupt.

**Fix:**

Model the third state honestly instead of folding diagnosis-failure into the clean path. Capture the Result and distinguish unknown from clean: change the cross-check to `let diag = diagnose_wal(base);` then `inconsistency_count = diag.as_ref().map(|r| r.inconsistency_count).unwrap_or(0);` and compute completeness as `let complete = matches!(&diag, Ok(r) if r.inconsistency_count == 0);` so a failed diagnosis yields `complete: false` (integrity not provable → do not claim whole). Optionally add an `integrity: "clean" | "torn" | "unverified"` enum field and, to match status/diagnostics, return EXIT_DEGRADED when diagnose_wal errors so a script's `scope-stats && ship` cannot treat a corrupt store as success. Add a test mirroring scope_stats_torn_tail_reports_inconsistency that corrupts checkpoint.meta and asserts `complete: false`.

---

### [WARNING] encode_session_event silently truncates string length fields > 65535 bytes (u16 cast), producing an unrecoverable journal record

**Location:** `tidal/src/wal/format/session.rs:189, 191, 209, 212`
**Dimension:** Accuracy · **Zone:** `wal-format` · **Verifier:** CONFIRMED

**Issue:** Every variable-length string is framed as `(s.len() as u16).to_le_bytes()` followed by the full `s.as_bytes()`. When `s.len() > u16::MAX` (65535) the cast wraps (e.g. 65536 -> 0, 65540 -> 4) while the encoder still appends the full-length byte payload. The record's len prefix and BLAKE3 are computed over the real bytes, so the frame is internally self-consistent and passes the checksum, but on decode `read_str_field` reads only the truncated count, leaving the remaining string bytes to be misinterpreted as the next field/record. `agent_id` is bounded to 64 chars upstream (session/types.rs:48), but `signal_name`, `policy_name`, and especially user-supplied `annotation` (which flows unbounded from the public signal API through sessions.rs:445-451 — MAX_ANNOTATIONS caps the count, not the byte length) have no length guard before reaching this encoder.

**Why it matters:** A single oversized annotation corrupts its own record and, because the byte stream is misframed from that point, the decoder either stops cleanly (silently dropping that write AND every later event in the journal — lost session state on recovery) or decodes a garbled string. This is a silent-data-loss path triggered by ordinary large user input, not just hostile bytes, and it violates the project rule that the WAL is the durable source of truth. Encoding must never produce a record it cannot faithfully decode.

**Fix:**

Make the encoder fallible (or validate at the API boundary) rather than casting. Either (a) change `encode_session_event` to return `Result<Vec<u8>, WalError>` and reject any field whose `len() > u16::MAX` with a typed error, or (b) enforce a documented max length (mirroring agent_id's 1..=64 rule) on `signal_name`/`policy_name`/`annotation` at the write API so the invariant holds before the WAL. Add a test that an annotation of length 70_000 is either rejected or round-trips losslessly — currently neither holds. Prefer (a) so the format layer is self-defending regardless of upstream.

---

### [WARNING] segment.sync() rustdoc overstates durability on macOS — fsync does not flush the drive write cache without F_FULLFSYNC

**Location:** `tidal/src/wal/segment.rs:208-235 (doc + sync_data call at 233)`
**Dimension:** CLEAN · **Zone:** `wal-write` · **Verifier:** CONFIRMED

**Issue:** The rustdoc states sync_data() 'flushes the file's data ... a single sync() makes the freshly appended batch fully durable' and is 'genuinely sufficient'. On Linux (fdatasync) this is correct. On macOS, std's File::sync_data/sync_all issues plain fsync(2), which Apple explicitly documents as NOT flushing the storage device's volatile write cache — true durability requires fcntl(F_FULLFSYNC). No F_FULLFSYNC handling exists anywhere in the crate (grep confirms zero hits). macOS is an active platform for this project (dev and test loop run on darwin per CLAUDE.md and the working env).

**Why it matters:** A power-loss/hard-crash on macOS can lose batches the WAL has 'fsynced' and acknowledged to callers, because the bytes are still in the drive cache. The doc tells a future maintainer the durability story is complete when on macOS it is not. The WAL is the source of truth (CODING_GUIDELINES §2) and §8 requires 'no lost events' — on macOS this is silently weaker than the doc claims. Severity is WARNING rather than CRITICAL only because the overwhelming production target is Linux and the gap is a known OS-level fsync semantic, not a logic bug.

**Fix:**

Either (a) on macOS, issue F_FULLFSYNC instead of fsync for segment, checkpoint, session-journal, and directory syncs (a small platform-gated helper: on target_os="macos" call libc::fcntl(fd, libc::F_FULLFSYNC); fall back to sync_data elsewhere), or (b) if macOS is explicitly dev/test-only and never a durable production target, soften the rustdoc to scope the 'fully durable' claim to Linux/fdatasync and add a one-line note that macOS needs F_FULLFSYNC for crash durability. Do not leave the doc asserting full durability while the implementation cannot provide it on a supported platform.

---

### [WARNING] WalHandle::truncate_before deletes segments without the directory fsync that the compaction path performs

**Location:** `tidal/src/wal/writer.rs:308-317 (TruncateBefore -> segment::delete_segments_before) and tidal/src/wal/segment.rs:296-306 (delete_segments_before has no dir sync), vs tidal/src/wal/compaction.rs:145-151 (compact_segments fsyncs the dir after unlinks)`
**Dimension:** Accuracy · **Zone:** `wal-write` · **Verifier:** CONFIRMED

**Issue:** compact_segments explicitly fsyncs the WAL directory after removing segments, with a comment that without it 'a crash after deletion but before directory metadata flush could resurrect deleted segment files.' The sibling deletion path used by the TruncateBefore command — segment::delete_segments_before — performs the same remove_file calls but never fsyncs the directory. So the two deletion paths disagree on durability of the unlink.

**Why it matters:** A resurrected old segment is benign for correctness here (it is covered by the checkpoint and replays idempotently), so this is not data loss. But it is an undocumented inconsistency between two near-identical delete loops: a reader comparing them will reasonably assume the missing fsync in one is a bug, and a future change that makes segment resurrection non-idempotent (e.g. if truncate ever advances the WAL checkpoint marker) would turn this into a real defect. truncate_before is currently only called from tests (production uses compact_wal_online), which is why this is WARNING not CRITICAL.

**Fix:**

Unify the two paths: route TruncateBefore through compaction::compact_segments (or have delete_segments_before fsync the directory after deleting, matching compact_segments) so there is exactly one durable-delete routine. While there, add a doc note on delete_segments_before stating whether the unlink is made durable; today the asymmetry is silent.

---


## Downgraded by verifier to SUGGESTION (3)

- **`db-export`** — Secondary-state checkpoint block duplicated four ways with divergent error handling  
  `tidal/src/db/backup.rs:325-348 (checkpoint_secondary_state) vs tidal/src/db/lifecycle.rs:173-198, tidal/src/db/state_rebuild.rs:591-646, tidal/src/db/remove_scope.rs:81` — The cohort / co-engagement / community / preference-vector checkpoint sequence is copy-pasted in at least four places. backup.rs logs-and-continues (returns ()); lifecycle.rs folds each failure into first_err; the guard conditions (entry_count() > 0, !is_empty()) differ subtly between copies. The backup.rs docstring even acknowledges 'the shutdown path has the same set inline.' Divergence risk is real: a future fifth derived ledger, or a changed guard, must be edited in four spots or one path silently skips a checkpoint.

- **`tidal-net`** — Circuit breaker module doc claims an in_flight flag the type does not have  
  `tidal-net/src/circuit_breaker.rs:10-12 (module doc) vs :43-45 (HalfOpen variant)` — The module doc states 'the half-open state carries an `in_flight` flag so concurrent callers cannot all probe a peer that just failed.' The actual CircuitState::HalfOpen is a fieldless variant; the single-probe invariant is enforced by the variant itself (check() returns Ok only on the Open->HalfOpen transition and Err while in HalfOpen), not by any in_flight flag. The behavior is correct and well-tested (half_open_admits_exactly_one_probe), but the doc describes a data member that does not exist.

- **`tidal-server`** — Cluster /signals returns 204 success while follower replication is best-effort and silently dropped  
  `tidal-server/src/cluster.rs:782-804 (write_signal) → tidal/src/testing/cluster.rs:424-437 (SimulatedCluster::write_signal send_segment)` — The cluster `write_signal` handler returns `204 No Content` once `SimulatedCluster::write_signal` returns Ok. But that engine call applies the write to the leader and then ships to followers with `let _ = transport.send_segment(...)` — the send result is discarded (cluster.rs:436 in the engine). A follower whose gRPC ship fails is never surfaced; the client sees 204 and assumes the signal is durably replicated. There is no acknowledgement, quorum, or even a logged warning on ship failure at the engine boundary.


## Standalone SUGGESTIONS (37)

- **`cohort-load-misc`** — BUILD_HASH doc comment names the wrong environment variable  
  `tidal/src/lib.rs:30-32` — The rustdoc says "Build hash compiled in from the `GIT_HASH` environment variable" and "Falls back to `dev` if `GIT_HASH` is unset", but the const actually reads `option_env!("TIDALDB_BUILD_HASH")`. build.rs:3-4 reads GIT_HASH at build time and re-exports it as TIDALDB_BUILD_HASH, so runtime behavior is correct, but a reader grepping for TIDALDB_BUILD_HASH (the var that actually gates the const) finds a doc that points them at the wrong name.

- **`cohort-load-misc`** — No test for the concurrent invalidate_all / slow-path-insert staleness window in the resolver  
  `tidal/src/cohort/resolver.rs:74-98` — resolve() computes membership against a registry snapshot outside the cache lock (lines 83-91), then re-acquires the lock to insert (95-96). If define_cohort + invalidate_all races between the snapshot and the insert, a membership computed against the OLD cohort set can be written into the freshly-cleared cache and served until the next per-user invalidate or LRU eviction. This is benign for a pure accelerator (and documented on invalidate_all), but there is no test pinning the behavior, so a future refactor could turn the benign window into a real lost-attribution bug undetected.

- **`db-core`** — Preference-vector dimensionality fallback duplicated across both open branches  
  `tidal/src/db/open.rs:113-116 and 246-249` — The block `let pref_dim = schema_def.embedding_slots().first().map_or(128, |s| s.dimensions);` appears verbatim in both the Ephemeral and Persistent arms of open_with_schema, and the magic default 128 also appears independently in from_config (mod.rs:447, PreferenceVectors::new(128)).

- **`db-entities`** — Finite-weight validation duplicated across signal and signal_scoped instead of shared like the backpressure guard  
  `tidal/src/db/signals.rs:128-132 and 190-194` — `check_write_backpressure` was correctly extracted into one helper so the two admission policies cannot drift (per the W10 fix and its regression test). The immediately-adjacent `if !weight.is_finite() { return Err(...) }` block is still copy-pasted verbatim into both `signal` and `signal_scoped`. The two admission preambles are otherwise identical; a future change to the weight rule (e.g. clamping, or a max magnitude) has to be made in two places and a missed one is a silent divergence between the two entry points — the same drift risk the backpressure extraction was meant to remove.

- **`db-ops`** — tidaldb_checkpoint_failures_total omits the partition_id label every other line carries  
  `tidal/src/db/metrics/mod.rs:493-503` — Every node-identity series is stamped with partition_id (uptime/health/info at 320-329, and all feature-gated gauges via the same render). The unconditional checkpoint_failures_total block emits 'tidaldb_checkpoint_failures_total {failures}' with no labels at all (line 501). The W13 cluster-scrape goal is that per-node series stay distinct when many nodes feed one Prometheus — this counter breaks that for itself.

- **`db-recovery`** — rebuild_entity_state and rebuild_item_indexes have no direct unit test for the rebuild-equals-pre-crash-state invariant  
  `tidal/src/db/state_rebuild.rs:148-222 and 367-414` — The zone's two central rebuild functions are exercised only indirectly through integration crash tests. There is no focused test asserting that, after writing relationships (block/hide/follow/interaction) and items (with category/format/creator/tags/duration/created_at), a rebuild reconstructs the identical UserStateIndex sets, interaction ledger, creator_items bitmap, universe, and created_at recency order. The shared-indexer design is the safety net, but a direct test would lock the contract and catch a future change to the shared path that the integration tests might not isolate.

- **`db-sessions`** — community_contribute mutates the in-memory aggregate before persisting the cell, inverting the durability-before-mutation discipline used elsewhere in the zone  
  `tidal/src/db/communities.rs:299-345 (community_contribute)` — The method records into the in-memory community_ledger (community_ledger.record at line 302) BEFORE persist_cell (line 328). On a persist_cell failure it returns Err(Durability) but the in-memory aggregate has already been incremented, so memory is ahead of disk for that contributor cell. This is the opposite order from add_to_collection (collections.rs:70-85) and leave_community (communities.rs:115-139), which both persist first and mutate memory only on success. The WAL community event IS written durably first (record_signal_scoped at line 289), so on crash the checkpoint-restore plus WAL replay reconverges and there is no permanent loss — the inconsistency is transient and lost on restart. But within a single live process after a persist_cell error, the caller sees Err while the aggregate reflects a contribution the caller believes failed.

- **`entities`** — Relationship key hardcodes the separator and Tag::Rel byte instead of going through encode_key/Tag  
  `tidal/src/entities/relationship.rs:72-97 (encode_relationship_key, relationship_prefix); parse offsets at 125-137` — encode_relationship_key and relationship_prefix build keys with literal `key.push(0x00)` (separator) and `key.push(0x04)` (Tag::Rel), and parse_relationship_to hardcodes the to-id at offset 11. The canonical contract lives in storage::keys (NUL = 0x00, Tag::Rel = 0x04, encode_key layout `[id BE][0x00][tag][suffix]`). The relationship key is exactly `encode_key(from, Tag::Rel, [rel_type_byte, to_be8])`, so it could be expressed through the shared helper. If Tag::Rel's byte or the separator ever changes, storage::keys has a drift-guard test but these hand-rolled functions silently diverge, corrupting every relationship key/prefix.

- **`governance`** — Redundant HashMap re-lookup in the sorted-fold over contributors  
  `tidal/src/governance/community_ledger.rs:219-226 (decay_score_at), 427-431 (explain), 658-663 (aggregate_digest)` — Each site does `let mut keys: Vec<&ContributorKey> = cell.contributors.keys().collect(); keys.sort_unstable();` then `for k in keys { if let Some(e) = cell.contributors.get(k) { ... } }`, performing a second HashMap lookup per contributor that is provably present (the key came from the same map under the same read guard). The `if let Some` also implies a fallibility that cannot occur.

- **`query-executor`** — creator_id backfill from item_metadata is duplicated verbatim across Stage 3 and Stage 4.5  
  `tidal/src/query/executor/mod.rs:480-490 and tidal/src/query/executor/pipeline.rs:442-452` — The identical block (for each candidate with creator_id None, look up meta, parse `creator_id`, set EntityId) appears twice. The comment at pipeline.rs:436-441 explains the second pass is a cheap re-backfill, but the logic is copy-pasted rather than extracted.

- **`query-executor`** — Preference-boost map keyed by truncated u32 can collide for >u32::MAX candidate ids  
  `tidal/src/query/executor/personalization.rs:112 and the consumer at tidal/src/ranking/executor/mod.rs:283,296` — compute_preference_boosts inserts `entity_id.as_u64() as u32` as the key, and the scorer looks it up with the same truncation. Because both sides truncate identically, the boost lands on the right candidate in the common case, but two candidates whose ids differ only above bit 32 collide on one map slot, cross-contaminating their additive boost.

- **`query-suggest-fusion`** — Suggest.for_user is a stored-but-never-read API knob  
  `tidal/src/query/suggest.rs:35,60-63 and tidal/src/db/query_ops.rs:334-337` — Suggest exposes a for_user field and a for_user(id) builder method, both documented 'reserved for future use', but SuggestionIndex::suggest takes only (prefix, limit) and the call site in query_ops.rs never forwards req.for_user. A caller that sets for_user expecting personalized suggestions silently gets the same global result.

- **`query-suggest-fusion`** — Trending and eviction sorts (suggest) also order ties by map iteration  
  `tidal/src/query/suggest.rs:241 and 269` — trending_suggestions does `pairs.sort_by(|a, b| b.1.cmp(&a.1))` and evict_low_frequency does `pairs.sort_by_key(|&(_, f)| f)` over pairs collected from a DashMap iterator. Entries with equal frequency are ordered by map iteration, so the returned top-N trending terms and which terms get evicted are non-deterministic among ties. The counts are inherently approximate and ephemeral, so impact is small, but it is the same latent inconsistency as the fusion finding.

- **`ranking`** — Trending / cohort_trending double-count velocity: the Trending sort base and the profile boosts read the same signals  
  `tidal/src/ranking/builtins.rs:130-154 (trending), 431-453 (cohort_trending); applied additively at tidal/src/ranking/executor/scoring.rs:205-274 (score_trending base) + tidal/src/ranking/executor/mod.rs:526-541 (boost loop)` — The trending() builtin sets sort = Sort::Trending AND boosts = [share.velocity(24h)*2, view.velocity(24h)*1]. compute_raw_score adds the Sort::Trending base (score_trending = view_vel + 2*share_vel over the same 24h window) to boost_sum (2*share_vel + view_vel), so the effective score is 2*view_vel + 4*share_vel. Spec §11.9 states the sort mode 'replaces stages 4-5 (boost and penalty application)', i.e. boosts should not also fire when a formula sort is active. Because the duplication is a uniform positive scalar over identical inputs, the resulting order is identical to the spec'd formula, so this is not a correctness bug — but it is redundant work and a latent trap: any future change that makes the boost window differ from the formula window (e.g. boost over 6h, sort over 24h) would silently produce a hybrid the spec says should not exist.

- **`ranking`** — Shuffle sort is a fixed global permutation, not the spec'd per-user, time-windowed, quality-weighted sampling  
  `tidal/src/ranking/executor/formulas.rs:72-84 (shuffle_score) and tidal/src/ranking/executor/scoring.rs:80` — Spec §11.6 defines shuffle_score(item) = random(seed) * quality_weight(item) with seed = hash(user_id, timestamp_minute) and quality_weight = sqrt(quality_score). The implementation is shuffle_score(entity_id) = hash(entity_id) only — no user_id, no timestamp_minute, and no quality weighting. The result is the same ordering for every user on every request forever, and low-quality items are as likely to surface as high-quality ones. The doc comment honestly says 'deterministic hash of entity ID', so this is an under-implementation rather than a hidden bug, but the advertised 'surprise me / mood-based discovery' behavior (per-user variety that refreshes per minute, quality-biased) does not exist.

- **`ranking`** — All-equal candidate scores normalize to 1.0, but spec §8.2 specifies 0.5  
  `tidal/src/ranking/executor/helpers.rs:273-274 (range < EPSILON -> 1.0) and the asserting test tidal/src/ranking/executor/tests.rs:73-88` — Spec §8.2 says: 'If max_score == min_score (all candidates scored equally), all normalized scores are set to 0.5.' normalize() sets them to 1.0 instead, and score_normalization_all_equal asserts 1.0. Ordering is unaffected (all equal), so this is purely a reported-score semantic mismatch, but the returned `score` field is what callers surface for explainability/thresholding, and a degenerate set reporting 1.0 ('maximally confident') rather than 0.5 ('neutral / no signal') is misleading — e.g. a Scan profile with no boosts on items that all have zero signals returns every item at score 1.0.

- **`repl-shipping`** — lag_segments() silently returns 0 for an unknown shard, masking a misconfigured gauge  
  `tidal/src/replication/lag.rs:56-60 and 76-78` — lag_segments() computes leader.saturating_sub(self.state.applied_seqno(self.shard_id).unwrap_or(0)). If the gauge's shard_id is not present in the shared ReplicationState (a wiring bug — e.g. a gauge built for ShardId::SINGLE against a state that only tracks other shards), applied_seqno returns None, unwrap_or(0) treats it as 'fully behind' or, combined with a 0 leader, as 'caught up'. Either way the unknown-shard case is indistinguishable from a legitimately-zero reading. applied_seqno() (lag.rs:76-78) has the same unwrap_or(0).

- **`schema`** — Embedding-slot names bypass the identifier validation applied to signal/policy names  
  `tidal/src/schema/validation/builders.rs:57-69 (embedding_slot) and 310-329 (validation loop)` — Signal names and policy names must pass is_valid_signal_name (non-empty, ASCII, lowercase [a-z0-9_], leading letter). Embedding-slot names are validated only for emptiness and (name,kind) uniqueness — any UTF-8 is accepted, including control bytes. In the fingerprint, a slot name is hashed as raw bytes followed by a fixed-width [kind:1][dims:8] trailer; because the kind byte (0..=2) can appear inside an unrestricted name, a maliciously/accidentally crafted pair of slot names could in principle produce the same concatenated byte stream under a different split, unlike signal names where the 0x00 kind separator after a guaranteed-non-control name makes collisions impossible.

- **`session`** — Session start record persists durably to storage but session restore depends only on the fire-and-forget WAL Start event  
  `tidal/src/db/sessions.rs:87-107 (durable start record + fire-and-forget WAL) vs tidal/src/db/session_restore.rs:60-72 and :90-105` — start_session writes a start record synchronously to items storage (sessions.rs:90, a durable put) AND fires a best-effort WAL session_start (sessions.rs:98, fire-and-forget per wal/mod.rs:329-356). On restart, active sessions are re-created ONLY from the WAL Start events (restore_session_wal_events, session_restore.rs:90); the durable storage start record is used merely to rehydrate metadata (session_restore.rs:129-138) and, when seen alone, is logged as 'orphaned' / 'in-memory signal state is lost' (restore_sessions, session_restore.rs:60-68). So if a crash lands after the durable storage put but before the fire-and-forget WAL Start fsyncs, the session was durably recorded as started yet is NOT restored as active — it only emits an orphan warning. The reverse asymmetry of two start-of-truth records is easy to misread.

- **`signals`** — Negative signal weight is accepted and can drive a hot decay score negative  
  `tidal/src/db/signals.rs:128-132 (guard) and tidal/src/signals/hot.rs:147-168` — db.signal() rejects only non-finite weights (NaN/Inf); a finite negative weight is accepted. Spec §8 states 'Negative weights are rejected (negative signals use separate signal types, not negative weights)'. A negative weight folds into the decay accumulator and can make the stored score negative, at which point the debug_assert!(step.new_score >= 0.0) in on_signal (hot.rs:168) would fire in debug/test builds and the INV-SIG-3 'decay scores are non-negative' invariant is violated. The warm-tier count is unaffected (it uses fetch_add(1)), so it is not a data-corruption bug, but the score path silently diverges from the spec and from its own asserted invariant.

- **`signals`** — warm.rs restore() does not validate last_*_rotation_ns ordering relative to bucket pointers  
  `tidal/src/signals/warm.rs:361-389` — restore() clamps current_minute/hour/day to valid array bounds (good, prevents OOB on next rotate), but accepts arbitrary last_minute/hour/day_rotation_ns from the snapshot with no cross-field sanity. A corrupt checkpoint with last_hour_rotation_ns > last_minute_rotation_ns (hour anchor ahead of minute anchor) makes sum_current_hour compute k via now_ns.saturating_sub(last_hour) — if now_ns < last_hour the in-progress-hour fold reads k=1 (saturates), under-counting rather than mis-indexing. So it cannot panic or corrupt, but it can silently produce a wrong windowed count from hostile input until the next real rotation re-anchors the timestamps.

- **`storage-engine`** — scan_prefix error mid-stream is untested on the Fjall backend  
  `tidal/src/storage/fjall.rs:148-152` — The streaming adapter maps each guard via `guard.into_inner().map_err(map_fjall_err)?`, so a torn read / decompress failure surfaces as a `Err(StorageError)` item partway through iteration. Every test (scan_prefix, scan_empty_prefix_enumerates_all, scan_prefix_is_lazy) and most production callers use `.collect::<Result<Vec<_>>>()` or `.flatten()` — the latter (e.g. governance/community_ledger.rs:570, db/mod.rs:613) *silently drops* an Err item. There is no test proving a mid-stream backend error is propagated rather than swallowed, and the `.flatten()` callers would swallow it if it were.

- **`storage-engine`** — FjallStorage::open uses a hand-rolled corruption error instead of map_fjall_err, losing error classification  
  `tidal/src/storage/fjall.rs:266-296` — open() and open_keyspace() map every fjall open failure to `StorageError::Corruption { message: format!("failed to open ...: {e}") }`. But map_fjall_err (lines 92-118) is the carefully-built router that distinguishes Io (ENOSPC, permissions) / Closed (Locked, Poisoned) / Corruption. A permission-denied or disk-full on open is now stringified into a *corruption* message — the exact mis-classification map_fjall_err was written to prevent (its own doc comment, lines 69-91, argues this is wrong for the get/put path).

- **`storage-engine`** — StorageError::BatchConflict is dead — never constructed by any backend  
  `tidal/src/storage/error.rs:16-18` — BatchConflict is defined, Display-tested, and referenced in trait docs (engine.rs:54 "batch conflict"), but neither FjallBackend::write_batch nor InMemoryBackend::write_batch can ever produce it — fjall's OwnedWriteBatch.commit() maps through map_fjall_err (which never yields BatchConflict), and the in-memory path is infallible under the write lock. It is a phantom variant.

- **`storage-indexes`** — total_count() and selectivity() recompute a full bitmap union on every call  
  `tidal/src/storage/indexes/range.rs:207-218 (total_count), 195-203 (selectivity); tidal/src/storage/indexes/bitmap.rs:196-207 (total_count)` — total_count() unions every value bitmap in the tree/map on each invocation, and RangeIndex::selectivity() materializes the entire range bitmap just to read its len(). The evaluator's selectivity() path calls range-index selectivity per predicate during AND/OR ordering, so a query with several range predicates over a large index repeatedly walks all value bitmaps. At the stated M2 scale (10K entities) this is negligible, but it is O(distinct_values * avg_bitmap_size) per call and will become a hot-path cost as the index grows.

- **`storage-indexes`** — RangeIndex::load_from_kv_pairs accepts trailing bytes after a fixed-width value  
  `tidal/src/storage/indexes/range.rs:355-362` — The loader checks value_raw.len() < V::WIDTH and errors on truncation, but does not check for EXTRA bytes: from_be_key_bytes reads only raw[..WIDTH] and silently discards any remainder. A well-formed key always carries exactly WIDTH value bytes, so this never fires on data this code wrote; but on corrupt or adversarial on-disk input, two distinct keys that share the first WIDTH value bytes but differ in trailing bytes would both decode to the same value (last-write-wins overwrites the tree entry), masking corruption rather than surfacing it. The bitmap loader, by contrast, validates the full structure via parse_suffix_value.

- **`storage-indexes`** — node_count() and eval_to_bitmap() recurse without an explicit depth bound  
  `tidal/src/storage/indexes/filter/expr.rs:156-164 (node_count) and tidal/src/storage/indexes/filter/evaluator.rs:67-101 (eval_to_bitmap)` — The 256-node guard in db/query_ops.rs:33-41 bounds the TOTAL node count, but it is computed by node_count(), which itself recurses the full tree before the guard can reject it — and eval_to_bitmap/evaluate_not also recurse with no separate depth cap. A pathologically deep filter (e.g. Not(Not(Not(...))) thousands deep) would recurse to compute node_count before the breadth guard ever runs. In practice FilterExpr is built programmatically by the embedder (combined_filter wraps a flat Vec in a single And; there is no text parser producing arbitrary nesting), so this is reachable only if a caller hand-builds an extreme AST, which bounds the severity.

- **`storage-vector`** — rebuild_from_store can register a zero-dimension slot from a corrupt store and pollute search results  
  `tidal/src/storage/vector/registry.rs:361-399 (`let dimensions = vector.len();` then lazy register + insert)` — The write path rejects empty vectors (l2_normalize returns ZeroNormVector before anything is stored), so dim=0 is unreachable through normal operation. But rebuild trusts whatever is on disk: serialize_embedding(&[]) is a valid 4-byte payload that deserialize_embedding happily accepts (expected_len == 4), so a corrupt or hand-crafted store containing an `EMB:slot` value of exactly 4 zero bytes makes rebuild register a 0-dimensional slot and insert a 0-length vector. l2_distance_sq over empty slices returns 0.0, so that entity would tie for nearest on every query against a 0-dim slot.

- **`tidal-net`** — Server-side payload validation rejects strictly-greater-than but the engine wire limit is inclusive at MAX_PAYLOAD_BYTES  
  `tidal-net/src/server.rs:46-52; tidal-net/src/transport.rs:232-237; consistency with config.rs:119` — send_segment (transport.rs:232) and ship_segment (server.rs:46) both reject `len() > max_payload_bytes`, i.e. a payload exactly equal to the configured max is accepted. config.validate (config.rs:119) caps max_payload_bytes at MAX_PAYLOAD_BYTES inclusive, and the codec limits are set to max_payload_bytes. This is internally consistent (a payload == max is accepted on both ends and within the codec), so it is not a bug. The note is only that the boundary semantics ('> max' vs '>= max') are asserted by prose comments rather than a shared test, so a future change to one guard could silently desync the two ends.

- **`tidal-server`** — 2MB body-limit literal duplicated instead of reusing the named constant  
  `tidal-server/src/cluster.rs:603 vs tidal-server/src/router.rs:41` — `build_cluster_router` hardcodes `DefaultBodyLimit::max(2 * 1024 * 1024)` while the standalone router defines `const BODY_LIMIT_BYTES: usize = 2 * 1024 * 1024` and uses it. The two can silently drift (raise one, forget the other) and a reader has to compute the magic number in cluster.rs.

- **`tidal-server`** — ScatterGatherInfo construction is copy-pasted between sharded_feed and sharded_search  
  `tidal-server/src/cluster.rs:1005-1011 and 1069-1075` — Both sharded handlers build an identical `ScatterGatherInfo { degraded, unavailable_shards, shards_queried, elapsed_ms, shard_deadline_ms }` from the `ScatterGatherMeta`. The five-field mapping is duplicated verbatim.

- **`tidalctl`** — scope-stats swallows a list_segments error and reports `wal_segments: 0` while having just tallied events from those segments  
  `tidalctl/src/commands/scope_stats.rs:154-156` — `list_segments(&wal_dir).map(|s| s.len()).unwrap_or(0)` discards any error and reports zero segments. read_all_events (line 132) internally calls the same list_segments and would have already returned Err via `?` if it failed, so in the common path this is unreachable — but if the directory contents change between the two scans (a segment rolled/removed by a concurrent live writer, which this lock-free inspector explicitly supports), the second call can fail or return a different count, yielding the internally-inconsistent output `wal_segments: 0` alongside a non-zero `total_events`. The `0` reads as 'no segments' rather than 'segment count unknown'.

- **`wal-format`** — Length-prefixed string write is hand-rolled 4x; no encode-side mirror of read_str_field  
  `tidal/src/wal/format/session.rs:189-192, 209-210, 212-213` — The decode side centralizes the `[len: u16][bytes]` framing in `read_str_field` (line 470), but the encode side hand-rolls `payload.extend_from_slice(&(x.len() as u16).to_le_bytes()); payload.extend_from_slice(x.as_bytes());` four separate times (agent_id, policy_name, signal_name, annotation). There is no single `push_str_field` owning the write-side framing.

- **`wal-format`** — Present-but-short optional fields (seqno/idempotency_key) in a checksum-valid v2 Signal frame silently decode to None instead of stopping  
  `tidal/src/wal/format/session.rs:548-591` — In `decode_signal_record`, a present optional (`has_seqno != 0` / `has_key != 0`) whose value runs past `end` falls through to `None` (lines 551-555, 565-587) and the function still returns `Some(Signal{..})`, i.e. RecordOutcome::Decoded — the flag byte is consumed but the absent value is not, and the partial field is silently dropped rather than treated as Malformed. The writer always emits the full value when the flag is set, and a checksum-valid v2 frame proves the bytes are intact, so a genuine writer can never produce this; it is reachable only via a hand-crafted frame with a recomputed valid BLAKE3. No test covers it.

- **`wal-recovery`** — Dedup window force_rotate inside record() resets the time-based rotation clock, slightly weakening the time window after a size-driven rotation  
  `tidal/src/wal/dedup.rs:104-110 and 152-156` — record() (called post-fsync for every kept event) calls force_rotate() when current reaches max_entries, and force_rotate() resets rotation_time = Instant::now(). Because record() does not itself drive the time window (only contains/is_duplicate call maybe_rotate), a burst that triggers size-driven rotations repeatedly pushes the time-based rotation deadline forward, so the effective time window can be longer than configured under sustained load. This is explicitly within the documented best-effort contract (dedup.rs:16-23), so it is not a correctness bug — but it is an undocumented interaction at the record() site.

- **`wal-write`** — WalError::SegmentFull is dead code — defined and unit-tested but never constructed in production  
  `tidal/src/wal/error.rs:13-15 (variant) and error.rs:62-64 (display test)` — SegmentFull is documented as an 'internal signal to trigger rotation', but rotation is now driven by SegmentWriter::needs_rotation() + rotate() inside flush_batch (writer.rs:224-226); the variant is never returned anywhere in non-test code (grep confirms the only references are the definition and its Display test).

- **`wal-write`** — No test covers a flush failure that occurs during segment rotation (rotate() failing mid-commit)  
  `tidal/src/wal/writer.rs:224-229 (flush_batch: needs_rotation -> rotate -> write -> sync) and the test-only fault hook at writer.rs:206-214 which fires before encode` — The FAIL_NEXT_FLUSH hook injects an error at the very top of the write closure (before encode/rotate/write/sync), so the resilience tests prove caller-notification and writer-survival for a failure on a non-rotating batch. There is no test where the failure lands specifically inside rotate() — i.e. the old segment was synced but the new segment file failed to open, or the directory fsync failed — even though that path is real (rotate does file open + dir sync_all, both fallible).


## Refuted by adversarial verification (2) — listed for transparency

- **`query-executor`** — ~~signal_ranked_candidates buffer-cap math can under-bound when limit is 0~~  
  `tidal/src/query/executor/candidate_gen.rs:68,85,101,108-111` — **REFUTED:** The claimed "index-underflow panic" cannot occur because the `.max(200)` floor in `candidate_gen.rs:68` means `max_candidates` is always >= 200, even when `limit == 0`. With `limit == 0`: `max_candidates = (0*4).max(200) = 200`, `buffer_cap = 200.saturating_mul(2).max(201) = 400`. The inline partition at line 101 fires only when `scored.len() >= buffer_cap = 400`, so the index `max_candidates - 1 = 199` is always valid (vec has at least 400 elements). The final partition at lines 108-111 fires only when `scored.len() > max_candidates = 200`, so index 199 is valid (vec has at least 201 elements). Similarly in `pipeline.rs:210`, `cap = (0*4).max(100) = 100`, and `select_nth_unstable_by(cap - 1 = 99)` is only called inside `if candidates.len() > cap`, guaranteeing at least 101 elements. The `.max()` floors make the arithmetic safe regardless of whether `limit == 0`. The upstream guard at `types.rs:188` is a real dependency, but the failure mode described — "index-underflow panic" — is arithmetically impossible because `max_candidates` can never be zero. The finding's core premise is wrong.

- **`repl-crdt`** — ~~CrdtSignalState carries a single lambda; multi-decay-rate signals lose fidelity on reconciliation~~  
  `tidal/src/replication/crdt/signal_state.rs:99-103 (lambda field doc); consumed at tidal/src/signals/ledger/core.rs:533-539 (apply_crdt_state) and core.rs:599-617 (crdt_contributions)` — **REFUTED:** The finding's core premise is false: the schema does not support multiple decay rates per signal type. `DecayModel` is a single-lambda enum (`Exponential { half_life, lambda }` stores exactly one f64), and `derive_signal_ids_and_lambdas` (tidal/src/signals/ledger/mod.rs:67-69) always builds `vec![*lambda]` — a one-element vector. The `signal_lambdas: HashMap<SignalTypeId, Vec<f64>>` is a forward-compatibility container but is never populated with more than one entry per signal type in the live codebase. Consequently `lambdas.len()` at core.rs:533 is always 1 for Exponential signals (or 0 for Permanent/Linear), so the loop `for i in 0..lambdas.len().min(MAX_DECAY_RATES)` iterates at most once. The "slots 1..N overwritten with wrong lambda" scenario cannot occur because no multi-rate signal can be constructed. The comment at core.rs:529-532 is an explicit, honest scope declaration ("CrdtSignalState uses a single lambda per signal type"), not a silent omission. The finding also misidentifies the CRDT snapshot path: `crdt_contributions` snapshots `stored_score(0)` with the signal's one lambda, which is exactly correct for the only decay rate that exists. No fidelity loss occurs for any currently constructible signal.


## PRAISE — what the code does well (58)

- **`cohort-load-misc`** — Cohort checkpoint restore is genuinely all-or-nothing and the caller contract matches the code

- **`cohort-load-misc`** — Hostile-input hardening on the checkpoint suffix codec and cohort-name length bound

- **`db-core`** — Shutdown captures the first durable-flush failure and still attempts every step

- **`db-core`** — Hostile/corrupt metadata input cannot panic, over-allocate, or mis-frame on the crash-recovery rebuild path

- **`db-core`** — Partial-init failure leaves no corrupt or locked state

- **`db-entities`** — Overwrite re-index scrubs stale in-memory index entries before re-inserting, through the same shared path the restart rebuild uses

- **`db-entities`** — Hostile-input metadata deserialize is bounds-checked, non-panicking, and saturating on the length prefix

- **`db-export`** — Bounded export scan eliminates the unbounded-Vec OOM on both WAL and session-journal paths

- **`db-export`** — JSON-Lines export delegates all escaping to serde_json, with a non-panicking fallback on the export hot path

- **`db-ops`** — HTTP accept-inheritance bug fixed at the root with a precise invariant comment

- **`db-ops`** — Cumulative histogram is correct and proven against the naive rule

- **`db-recovery`** — Write path and restart-rebuild share one indexing implementation, making cross-restart drift impossible by construction

- **`db-recovery`** — Session WAL replay reproduces live windowed-counter rotations by sorting events ascending and seeding from each signal's earliest timestamp

- **`db-sessions`** — Prior-review CRITICAL notification-tracker memory leak is genuinely fixed and wired into the periodic sweeper

- **`db-sessions`** — Session/snapshot deserialization is uniformly hardened against hostile and corrupt on-disk input

- **`entities`** — narrow_item_slot makes a previously-silent u64->u32 truncation observable and centralized

- **`entities`** — Corrupt-input determinism via total_cmp and bounds-checked deserialization across the zone

- **`governance`** — Checkpoint is a single atomic WriteBatch with watermark rows that survive total contributor purge

- **`governance`** — Empty-agent purge footgun made inert at both the API boundary and the engine

- **`query-executor`** — Negated and OR-nested deferred filters fail loud instead of silently returning the inverse/AND set

- **`query-executor`** — Unknown-signal degradation is surgical — only UnknownSignalType degrades, every other error propagates

- **`query-search`** — Deferred filters are now fully applied and shared verbatim with RETRIEVE — the prior silent-drop is genuinely closed

- **`query-search`** — Disciplined checked u64->u32 conversion and poison-lock recovery throughout the scope/filter path

- **`query-suggest-fusion`** — Cursor wire format is genuinely crash/hostile-input safe

- **`query-suggest-fusion`** — MAX_TRENDING_KEYS cap held atomically under concurrency via CAS-reserve

- **`ranking`** — NaN/Inf handling and the missing-metadata sentinel normalization are correct and well-defended

- **`ranking`** — DiversitySelector invariants INV-RANK-5/6 are explicit, enforced, and property-tested

- **`repl-cluster`** — enter_dual_write implements true durability-before-observable-mutation with a tested rollback

- **`repl-cluster`** — Rate limiter decouples refill-claim from consume so contended retries cannot double-grant tokens, with a concurrency test that proves it

- **`repl-crdt`** — HLC logical-overflow carry is now symmetric across now() and update(), with guarding tests

- **`repl-crdt`** — Double-decay fix and LWW-decision unification are both real and test-pinned

- **`repl-shipping`** — Community-overlay WAL filter preserves the authoritative last-seq boundary — idempotency and lag stay correct across event dropping

- **`schema`** — Schema fingerprint correctly protects on-disk integrity with a single canonical kind encoding

- **`schema`** — Zero panics in production schema code; every fallible path returns a typed SchemaError

- **`session`** — Restored window counts are made byte-identical to live via earliest-timestamp seeding and ascending replay

- **`session`** — Session record deserialization rejects hostile/corrupt input instead of best-effort patching or OOMing

- **`signals`** — Forward-decay kernel unified as single source of truth with out-of-order correctness

- **`signals`** — Windowed tier-overlap double-count eliminated with bounded in-progress-hour fold

- **`signals`** — Checkpoint codec is offset-derived with compile-time size pinning and self-checksummed meta

- **`storage-engine`** — write_batch atomicity is genuinely all-or-nothing and proven by fault injection

- **`storage-engine`** — map_fjall_err preserves io::Error identity and conservatively halts on unknown variants

- **`storage-indexes`** — Deferred-filter 'return-universe' contract is soundly guarded end-to-end

- **`storage-indexes`** — Inverted/empty range bounds are clamped instead of panicking

- **`storage-vector`** — BruteForceIndex::deserialize is a model of hostile-input hardening

- **`storage-vector`** — Durable-first ordering in lifecycle ops is correct and rigorously reasoned, with a regression test for the failure window

- **`text`** — Strict fast-field accessor refuses to alias entity 0 on a missing id value

- **`text`** — Open-failure and absent-vs-unreadable are deliberately not collapsed onto one silent path

- **`tidal-net`** — Permanent-vs-transient error classification is correct, exhaustive, and prevents an infinite silent replication stall

- **`tidal-net`** — mTLS security boundary is proven by negative tests, not just a happy path

- **`tidal-server`** — Scatter-gather partial-failure handling is genuinely robust — no silent shard drops, NaN-safe ordering, deadline-bounded

- **`tidal-server`** — Entity→shard routing delegates to the engine ShardRouter so write and read routing cannot diverge

- **`tidalctl`** — Refuses to fabricate a '0' for values it cannot honestly derive offline

- **`tidalctl`** — All JSON routed through one serde_json path, closing the hand-rolled control-char escaping hole

- **`wal-format`** — Length fields read from disk are bounds-checked before any slice, and the only length-driven allocation is hard-capped

- **`wal-format`** — Corruption-vs-torn-tail contract is precisely specified and tested, including the subtle checksum-valid-but-short-body case

- **`wal-recovery`** — Dedup is correctly split into contains/record so a failed flush is never suppressed as a phantom duplicate

- **`wal-recovery`** — Hostile-input deserialization never panics: bounds checked before BLAKE3, before allocation, and before every field read

- **`wal-write`** — Active-segment clamp is implemented identically in both the offline and online deletion paths and regression-tested from both entry points


---

## Appendix — seven-dimension coverage per zone (forcing-gate proof)

Every zone was examined against all seven dimensions; each bullet is the reviewer's one-line verdict.

### `cohort-load-misc` — Cohort agg, bulk load, test harness, lib.rs  _(12 files)_
- **Completeness:** Largely clean. Error/edge paths are handled: cohort checkpoint restore is all-or-nothing with a documented corruption policy that matches its caller in db/mod.rs:682-707; rate-limiter and degradation thresholds validate degenerate inputs; load detector has proptests; no orphan TODO/FIXME or commented-out code in the zone. One gap: cohort/resolver.rs has no test for the concurrent define+invalidate_all staleness window (SUGGESTION).
- **Accuracy:** One real defect: cohort/resolver.rs uses .expect("...poisoned") on the cache mutex at 6 production (non-test) call sites on the signal-write hot path, diverging from the codebase's own PoisonError::into_inner recovery pattern (faults.rs, cluster.rs) and violating CODING_GUIDELINES §7. write_signal seqno rollback (cluster.rs:399,411) cannot underflow (slot >= 1 after increment). deserialize/restore paths are length- and version-checked against hostile input. Atomic orderings in crash_injector and load detector are correct and justified.
- **Tech Debt:** Honestly documented: cohort/checkpoint.rs:153-159 (full-keyspace scan_prefix) and cluster.rs:700-709 (applied_count hardcodes ShardId(0), undercounts post-promotion) both carry explicit doc notes and caller contracts. relay_log_len Relaxed-ordering correction is justified in-comment. No hidden hacks.
- **Maintainability:** Strong. Names are intention-revealing, functions focused (<50 lines mostly), WHY-comments are dense and load-bearing (corruption policy, atomicity invariants, Arc-clone vs String rationale). lib.rs BUILD_HASH doc comment (lines 30-32) names the wrong env var (GIT_HASH vs the TIDALDB_BUILD_HASH the code actually reads) — minor doc drift.
- **Extensibility:** Appropriate. Pluggable Transport in the cluster harness, configurable thresholds/cache-capacity, Predicate enum composes cleanly. No over-engineering; no YAGNI abstractions.
- **DRY:** One concrete violation: single_event_payload (cluster_transport.rs:95) was extracted specifically to keep write_signal and redeliver_missed from drifting on event_count/leader_last_seq, but write_signal (cluster.rs:428-434) still builds the payload inline instead of calling it — the exact drift the helper exists to prevent. resolver.rs:55 has redundant-but-safe defensive clamp code.
- **CLEAN:** Clear and neat overall. Hot-path allocation discipline is deliberate (Arc::clone keys in CohortSignalLedger, enabled fast-path in RateLimiter, borrowed-&str registry lookups). The inline payload duplication and the BUILD_HASH doc are the only blemishes; no dead code or misleading names found.

### `db-core` — TidalDb core / builder / open / lifecycle  _(11 files)_
- **Completeness:** Open/close/Drop, builder validation, capability gating, partial-init, and crash-recovery rebuild paths all have tests; no orphan TODO/FIXME or commented-out code in zone. Gaps: validate() omits is_dir() checks (WARNING) and writability is metadata-only (WARNING).
- **Accuracy:** Durability ordering in shutdown_inner is correct (persist derived state -> storage.flush -> WAL marker -> compact); WAL replay vs checkpoint boundary uses strict greater-than so no double-count; first-failure-wins error capture surfaces lost durable state. Lock acquired before construction, released on every error path via Drop. Only one non-test expect() (mod.rs:258), a locally-guaranteed invariant. Clean.
- **Tech Debt:** cache_dir is reserved/no-op (documented W1); no magic numbers beyond the documented 128-dim preference fallback. resolved_wal_dir() is a single source of truth. No copy-paste smells; SharedDefaults + build_control_plane already factor the two constructors. Clean.
- **Maintainability:** Names are intention-revealing; the heavy from_parts/open_with_dir functions carry detailed WHY comments tying each step to a ticket (BLOCKER 6/7, obs-REPL-1, CONCURRENCY-1/2/3, SHUTDOWN-2). from_parts is long (>500 lines) but linearly ordered and annotated — acceptable given construction is inherently sequential.
- **Extensibility:** StorageBox trait-routes Memory vs Fjall; NodeConfig/NodeRole cleanly gate single/leader/follower; runtime-validated builder is the right call over typestate (justified in-code). No over-engineering.
- **DRY:** Default field initializers centralized in SharedDefaults; control-plane wiring shared via build_control_plane/install_metrics_wiring; WAL dir resolved through one method. The two `let pref_dim = ... map_or(128 ...)` blocks in open.rs are the only minor duplication (SUGGESTION).
- **CLEAN:** Bounds-checked, checked_add deserialize on the rebuild path; saturating length encode; hostile metadata input degrades to empty map with no panic/OOM. Lock-error path distinguishes WouldBlock from real I/O. Genuinely tidy.

### `db-entities` — Entity write ops (items/users/creators/signals)  _(11 files)_
- **Completeness:** Write paths handle validation, defaults, overwrite-scrub, and durable restore; one known gap (signal_with_context item-id narrowing) is documented-but-deferred. Tests cover oversized metadata, u32 boundary, overwrite scrub, backpressure parity.
- **Accuracy:** WAL-first ordering correct; durability-before-mutation in remove_scope; checked_add bounds on deserialize; no unwrap/expect/panic in non-test code; lock-poison recovery on universe bitmap. One silent-aliasing narrowing remains (signals.rs:344).
- **Tech Debt:** Bare `as u32` in signal_with_context is the last un-unified narrowing — the in-code comment (items.rs:59-63) acknowledges it as deferred. metrics only on `signal`, not `signal_scoped` (minor parity gap).
- **Maintainability:** Names intention-revealing; functions focused; extensive WHY-comments tie each side-effect to its recovery path. signal_with_context is a long dispatcher but each arm is a one-liner delegating to a named helper.
- **Extensibility:** Shared helpers (write_entity_meta, write_entity_embedding, entity_embedding_slot, persist_to_items, check_write_backpressure) factor the right seams. narrow_item_slot exists as the intended shared guard but is not yet wired into the signal path.
- **DRY:** Strong: user/creator meta + embedding + persist paths share one body; signal/signal_scoped share backpressure+finite guard. Minor: finite-weight check duplicated inline in both signal entry points rather than shared like backpressure is.
- **CLEAN:** Clear and neat; no dead code; EMB: row skip prevents garbage deserialization; explicit no-op arms (InteractionWeight|Mute) are greppable not catch-all.

### `db-export` — Export / backup / text-sync / sweeper  _(5 files)_
- **Completeness:** Gaps found: no test/handling for forward-compat session frames in the streaming export (the stream stops early); no test asserting restored-session TTL expiry; backup has no test for host-crash durability of the copy. Validation of export limit/time-range is otherwise complete and tested.
- **Accuracy:** Three real defects: (1) export session-journal streaming break-on-empty silently drops the rest of the journal on a forward-compat/Close-less frame; (2) sweeper measures session age from a monotonic Instant reset on restore, so restored sessions never TTL-expire — already flagged in the 2026-06-07 review and still unfixed; (3) backup copy never fsyncs files or the dest dir, so a host crash can leave a 'crash-consistent' backup truncated. Batch-WAL scan, u8 signal-type guard, JSON escaping, and CAS backup guard are correct.
- **Tech Debt:** Secondary-checkpoint block is duplicated across backup.rs, lifecycle.rs, remove_scope.rs, state_rebuild.rs with subtly different error folding — a known, documented split. MAX_SESSION_FRAME_BYTES and NANOS_PER_DAY are reasonable named constants.
- **Maintainability:** Strong: intention-revealing names, thorough WHY comments, focused functions. The export module's heap/bounded-retainer abstraction is well documented. start_sweeper docstring overstates the down-time-reaping guarantee (contradicted by the Instant reset).
- **Extensibility:** render_signals match-on-format and the ExportFormat enum make adding formats a compile-error-driven change — good. BoundedEarliest is shared across both scan paths (no duplication of the OOM fix). No over-engineering.
- **DRY:** in_time_window and BoundedEarliest are correctly centralized across both export scan paths. The only real DRY violation is the 4-way duplicated secondary-checkpoint block (cohort/co-engagement/community/preference) noted under tech_debt.
- **CLEAN:** Mostly clean and neat. The one correctness smell is the `if events.is_empty() { break }` heuristic in read_session_journal_signals conflating 'corrupt frame' with 'frame produced no events for us', which is the root of the forward-compat drop bug.

### `db-ops` — Query/replication ops, http, metrics  _(7 files)_
- **Completeness:** Checked error/edge paths: filter-node cap (retrieve:33-41, search:166-174), bounded HTTP request line + header drain (http:178-219), histogram overflow-above-bounds (histogram:72-81 + proptest 194-210). New behavior is tested (partition_id, degraded-derivation, fail-closed dual-write, well-formed-exposition parse). One real gap: FOR SESSION has divergent missing-session handling between retrieve and search — see finding.
- **Accuracy:** Histogram cumulative semantics correct (partition_point suffix-increment proven against naive rule; +Inf=count; no finite bucket on over-range). Atomics use Relaxed for stats, Release/Acquire at the checkpoint-staleness boundary correctly (mod.rs:277 pairs state_rebuild:537). No unwrap/expect/panic/unsafe in non-test code. No lock held across await (sync code). One partial-write-then-Err hazard in signal_for_tenant — see finding.
- **Tech Debt:** signal_for_tenant remote dispatch is explicitly unwired and fails closed with a tracking note to docs/specs/14 (CODING_GUIDELINES §12 honored). No orphan TODOs, no commented-out code, no magic numbers (MAX_FILTER_NODES, MAX_REQUEST_LINE_BYTES, CHECKPOINT_STALENESS_LIMIT_NS all named + documented).
- **Maintainability:** Names intention-revealing; functions focused; WHY-comments are strong (the macOS accept-inheritance comment at http:221-236 is exemplary). render_prometheus is long but flat and #[allow(too_many_lines)]-annotated. Clean.
- **Extensibility:** Histogram bounds are &'static parameters; metrics-add recipe documented at mod.rs:6-10; executor wiring is builder-chained. Appropriate abstraction, no YAGNI over-engineering.
- **DRY:** now_unix_nanos shared between staleness + render. write_metric_line dedups gauge/counter lines. as_micros()->u64 truncation note repeated verbatim in 3 hot paths (acceptable, each is local context). creator_embedding_slot unified via entity_embedding_slot. No harmful duplication.
- **CLEAN:** Clear/logical/neat. extract_path avoids a Vec alloc; bounded reads avoid OOM. One minor neatness gap: checkpoint_failures_total omits the partition_id label every other node-identity line carries — see finding.

### `db-recovery` — State rebuild & session restore on restart  _(14 files)_
- **Completeness:** WARNING — rebuild paths cover all four relationship types, item indexes, text/suggestion/embedding indexes, and replication HWM; but the closed-vs-active session reconciliation on restart is missing (no dedup, no test). Schema-fingerprint and WAL-lag have thorough unit tests; rebuild_entity_state / rebuild_item_indexes have no direct unit test (covered indirectly by integration crash tests).
- **Accuracy:** CRITICAL — session restore can place one session_id in BOTH self.sessions (active) and closed_sessions when a snapshot committed but its Close journal record was lost/torn. Otherwise logic is correct: shared ItemIndexes::index eliminates write/rebuild drift, ascending-ts session replay reproduces live BucketedCounter rotations, saturating next_session_id advance, checked_add bounds on all deserializers, Release/Acquire seq publication is sound.
- **Tech Debt:** WARNING — rebuild_item_indexes indexes a truncated/colliding id when entity_id > u32::MAX instead of skipping (write path rejects such ids before any durable write); the rebuild's divergent 'warn-then-index-collision' handling is a latent corruption-on-corrupt-input path.
- **Maintainability:** Clean — intention-revealing names, focused functions, load-bearing WHY-comments (item_meta_entity_id factoring, created_at materialization rationale, marker-not-advanced-on-periodic-checkpoint). run_checkpoint_thread is long but cohesive and annotated.
- **Extensibility:** Clean — ItemIndexes bundle and item_meta_entity_id are the right shared abstraction for the likely change (new item index); RelationshipType::from_byte forces every match to be revisited on a new variant. No over-engineering.
- **DRY:** Clean — the write path and rebuild path were deliberately unified through ItemIndexes::index/scrub; item_meta_entity_id is the single 'which rows are item metadata' rule reused by every item scan. No meaningful duplication remains.
- **CLEAN:** Clean — no dead code, no needless clones in hot paths (Cow used in the write path), one documented-invariant expect() in non-test code (AgentId::new("restored")). No unsafe.

### `db-sessions` — Sessions / collections / communities / notifications  _(11 files)_
- **Completeness:** One real gap: orphaned session start records are detected at open but never cleaned up or sealed into a snapshot (session_restore.rs:60-70). Notification-tracker eviction (prior CRITICAL) is now genuinely wired into the sweeper loop. Collection/community CRUD cover idempotency, u32-universe rejection, and reopen-durability with tests.
- **Accuracy:** BLOCKER: next_session_id is not advanced past durably-archived closed sessions on reopen, so a reused id silently overwrites a prior session's archived snapshot (verified with a real failing test). Membership stop-forward gate and notification check_and_record concurrency are correct (atomic CAS + record_lock). community_contribute records in-memory before persist_cell — minor memory-ahead-of-disk window, but WAL-backed so no data loss.
- **Tech Debt:** join_community/rejoin_community use a read-then-put on the membership registry that is not atomic against a concurrent leave (with_row read, then put); single-writer-per-(user,community) assumption is undocumented. No magic-number abuse; constants (MAX_CLOSED_SESSIONS, EVICT_BATCH_SIZE, NOTIFICATION_KEEP_DAYS) are named.
- **Maintainability:** Strong: intention-revealing names, thorough WHY-comments on every durability-ordering decision (durability-before-mutation rationale is explicit in collections.rs and communities.rs). session_signal is long (~160 lines) but linearly readable with clear stages.
- **Extensibility:** Snapshot serde is cleanly versioned (0x01/0x02/0x03) with shared bounds-checked cursor helpers; membership is JSON serde (flexible). Storage namespacing (COLLECTION_NAMESPACE sentinel, Tag-scoped prefixes) supports prefix-scoped rebuilds without over-engineering.
- **DRY:** Cursor helpers (read_u32/read_u64/read_len_prefixed_utf8/capped_capacity) are single-sourced across snapshot, start-record, audit, and saved-search codecs — a genuinely good de-duplication. narrow_collection_item_id centralizes the u32-universe guard. Minor: orphaned-start-key encode_key is repeated in 3 files but trivial.
- **CLEAN:** Clear and neat. Corrupt-input deserialization is uniformly rejected (None) with capped pre-allocation — no OOM-on-hostile-input vector. No unwrap/expect in non-test code (the one expect in session_restore.rs:122 is on a provably-valid literal AgentId).

### `entities` — Entity model (Item/User/Creator)  _(11 files)_
- **Completeness:** Validation and edge paths are handled: dim checks (preference), per-item bounds checks on deserialize (collection/mod), from_byte returns None on unknown bytes (Visibility/RelationshipType/Tag), and Mute is documented PERSISTED-BUT-INERT with a tracking note. Tests include regressions + proptests; all 96 entity tests pass. Clean.
- **Accuracy:** One real correctness gap: PreferenceVectors::restore neutralizes NaN to 0.0 but does NOT re-normalize, so a torn checkpoint row yields a non-unit pref vector while cosine_similarity divides only by the candidate norm (assumes unit pref) — the function's own comment claims to prevent cosine poisoning but only handles NaN, not the broken unit-length invariant. No unwrap/expect/panic/unsafe in non-test code; DashMap snapshots avoid held shard locks. Otherwise accurate.
- **Tech Debt:** relationship.rs hand-encodes keys with magic 0x00/0x04 instead of encode_key + Tag::Rel, duplicating the storage key contract. Caller db/signals.rs:344 uses a bare `as u32` truncation that bypasses the observable narrow_item_slot helper this zone added (out-of-zone but adjacent). Low overall.
- **Maintainability:** Strong: intention-revealing names, focused <50-line functions, thorough WHY-comments (out-of-order decay, seen/hidden orthogonality, eviction policy rationale).
- **Extensibility:** Shared serialize_entity codec for user+creator, trait-abstracted StorageEngine for checkpoint/restore, adding a RelationshipType/Tag forces exhaustive match revisits. Right-sized, not over-engineered.
- **DRY:** serialize_entity unifies user/creator; collection bitmap is single source of truth (item_ids derived). One duplication: relationship key tag byte hardcoded vs Tag::Rel.
- **CLEAN:** Clear, logical, neat. narrow_item_slot observability helper and the total_cmp/NaN-determinism handling in co_engagement are exemplary. No dead code, no misleading names.

### `governance` — Governance / community ledger  _(13 files)_
- **Completeness:** Checked: tombstone replay on open (db/mod.rs:613-636) covers both user+agent purge crash windows; per-accept persist + periodic + lifecycle checkpoint all wired; multi-entity/multi-signal collision regression has a test (community_ledger.rs:1038). Gap: no test or barrier asserting the per-accept contributor row is fsync-durable, while the rustdoc claims a 'ZERO loss window'.
- **Accuracy:** Checkpoint is a single atomic WriteBatch (community_ledger.rs:517-565) — the delete-then-put-outside-batch BLOCKER is fixed. purge_writer empty-agent guard correct (line 368). Watermark vs contributor suffix disambiguation by length is sound (8 vs >=30 bytes). One real defect: persist_cell/checkpoint never fsync, so the SOLE durable copy of contributor identity is committed-not-durable on power loss despite the 'durably written' claim.
- **Tech Debt:** Asymmetry: the agent-purge path re-checkpoints eagerly (remove_scope.rs:80) but the user-purge path does not (communities.rs:476) — both are correct via tombstone replay, but the inconsistency is a latent confusion. No hardcoded-config or copy-paste blocks of concern.
- **Maintainability:** Strong: intention-revealing names, thorough WHY-comments tying each guard to its CRITICAL/BLOCKER history, focused functions. purge_contributor/purge_writer share one purge_matching core (good). decode/encode codec is a single shared pair.
- **Extensibility:** Good boundaries: ContributorKey, RemoveScope enum, versioned GovernancePolicy/SharePolicy. WriterTag = String type alias is an appropriate seam. No over-engineering observed.
- **DRY:** to_durable_bytes centralizes tombstone serialize-or-error; atomic_u64 serde adapter shared by Membership+CapabilityToken; one suffix codec used by both checkpoint and restore. Minor: the 'collect keys -> sort -> re-get(k)' fold is repeated in decay_score_at/explain/aggregate_digest.
- **CLEAN:** Clear and neat. Minor needless re-lookup (HashMap::get after already holding the key set) in three fold sites; from_utf8_lossy on the writer tag silently mutates a corrupted tag rather than rejecting the row.

### `query-executor` — RETRIEVE query executor & filters  _(11 files)_
- **Completeness:** Filter coverage is thorough — deferred filters (Saved/Liked/InProgress/SocialGraph/Min-MaxSignal/NearLocation/InCollection) are all extracted and applied, with loud rejection of negated and OR-nested deferred shapes. Gap: cohort rescore ignores profile.sort and is reachable with any profile, and notification-cap recording runs pre-pagination (over-records). Both lack negative tests.
- **Accuracy:** Strong on the suppression/inclusion u32-overflow invariant via entity_as_u32 — except Stage 2 index intersection (pipeline.rs:294) still uses a raw `as u32` truncation on the candidate side, which can wrongly RETAIN a >u32::MAX SignalRanked candidate that aliases onto a matched low slot. read_agg correctly propagates UnknownSignalType (loud). No locks held across await; no unwrap in non-test code.
- **Tech Debt:** Cohort rescore re-sort is hardcoded to descending-score, duplicating intent of the main sort path instead of reusing it. ANN strategy in RETRIEVE is a logged scan fallback (acceptable, tracked in comments). Offset pagination acknowledged as M3 replacement target.
- **Maintainability:** Excellent — intention-revealing names, generous WHY-comments explaining each guard's failure mode, functions mostly focused. stage3_score is long but `#[allow(too_many_lines)]` and well-sectioned.
- **Extensibility:** collect_matching centralizes the And/Or-recurse, skip-Not walker (W27) — good single point of change. PostFilterCtx cleanly shares deferred filters between RETRIEVE and SEARCH. Cohort rescore is the one spot not abstracted against the sort path.
- **DRY:** Good. Suppression block, deferred post-filters, and traversal rule are each single-sourced and shared with SEARCH. entity_as_u32 is the one canonical conversion — but pipeline.rs:294 bypasses it.
- **CLEAN:** Clear and neat overall. One misleading SAFETY comment at pipeline.rs:293-294 asserts truncation is 'correct throughout this module' while the rest of the module deliberately avoids exactly that truncation.

### `query-search` — SEARCH pipeline & scope  _(12 files)_
- **Completeness:** All declared deferred filters (MinSignal/MaxSignal/NearLocation/InCollection/SocialGraph) are now applied via the shared post_filter module, with loud errors on unknown signals and OR/NOT-of-deferred rejection — verified against current source and regression tests. GAP: creator-search metadata filtering (CategoryEq/FormatEq on EntityKind::Creator) is processed against the wrong (item) index in Stage 2 and silently drops all creators; the m5p4 test tolerates 0 results so the gap is uncovered.
- **Accuracy:** No unchecked unwrap/expect/panic in non-test code; all u64->u32 bitmap conversions go through checked entity_as_u32 with documented polarity; cursor offset uses saturating_add; BM25 NaN neutralized with total_cmp; poisoned-lock recovery via into_inner. The one accuracy defect is the creator-namespace Stage-2 intersection (F1).
- **Tech Debt:** Minor: stats.filters_applied = query.filters.len() reports declared not applied count; exclude list applied after candidates_considered is set. No temporary hacks or hardcoded config found.
- **Maintainability:** Excellent — stage helpers are focused (<50 lines mostly), names intention-revealing, WHY-comments dense and accurate. The 8-stage execute() reads linearly as documented.
- **Extensibility:** Shared user_filter/post_filter modules make RETRIEVE/SEARCH drift impossible; ScopeResolver builder cleanly extensible. creator_embedding_slot threaded for future schema-driven resolution. No YAGNI over-engineering observed.
- **DRY:** Strong — collect_matching centralizes the four deferred extractors (W27), suppression/post-filter logic shared verbatim across both pipelines. entity_as_u32 is duplicated (pipeline.rs:75 and executor/mod.rs:508) but each is documented and tiny; the pipeline one only converts, the canonical applier lives in executor/mod.
- **CLEAN:** Clean and neat; no dead code, no needless clones on the hot path (combined_filter computed once — a fix from a prior pass). DEFAULT_EMBEDDING_SLOT constant centralizes the literal.

### `query-suggest-fusion` — SUGGEST, RRF fusion, retrieve types/cursor  _(9 files)_
- **Completeness:** Edge/error paths are well covered: cursor rejects wrong-length, unknown-kind, non-finite scores, and overflowing offsets; retrieve validates limit + u32 creator-ID ceiling; fusion handles empty/single/one-sided lists. No orphan TODOs or commented-out code. One gap: no test asserting deterministic fused ordering on tied scores.
- **Accuracy:** RRF math is correct (1/(k+rank), 1-based ranks, union semantics verified by proptest). Cursor encode/decode is byte-stable and round-trips. The one real defect: fuse() orders tied scores by randomized HashMap iteration with no entity_id tie-break, so identical queries across process restarts can return tied results in different order — a determinism bug the team already guarded against in the BM25 sort but missed here.
- **Tech Debt:** Suggest.for_user is accepted, stored, and builder-exposed but never read ('reserved for future use') — a public no-op knob. NotificationCaps lives in retrieve/types.rs but is documented as only meaningful for one profile and not enforced to it. Both minor.
- **Maintainability:** Names are intention-revealing, functions are focused and well under 50 lines, and the WHY-comments (cursor discriminant byte, key_count CAS reservation, NaN neutralization rationale elsewhere) are genuinely load-bearing and excellent. Clean module boundaries.
- **Extensibility:** Cursor's tagged-discriminant wire format and RetrievalMode enum are the right abstractions for likely change. rrf_term extraction removes duplication cleanly. for_user reserved field is mild speculative generality (YAGNI) but harmless.
- **DRY:** rrf_term centralizes the rank-score formula across fuse() and route_results (verified by rrf_term_matches_inline_formula). Limit-range check [1,500] is duplicated between RetrieveBuilder::build and Retrieve::validate, but that duplication is intentional defense-in-depth and documented. No problematic duplication found.
- **CLEAN:** Code is neat and efficient. fuse() pre-sizes the HashMap; cursor uses a fixed 17-byte stack buffer. The only CLEAN smell is the inconsistent tie-handling: BM25 sort uses total_cmp for a true total order, but fuse() and trending sort use partial_cmp/cmp with no secondary key, leaving tied order to randomized map iteration.

### `ranking` — Ranking: profile engine, scoring, diversity  _(16 files)_
- **Completeness:** Pipeline stages 2/4/5/6 (exclude/boost/penalty/gate), decay, sort modes, and diversity relaxation are all implemented with tests. Gaps are intentional/documented: per-user penalty multiplier (spec §6 USER_PENALTY_MULTIPLIER=3.0) and topic_diversity/category_min are not wired (reserved fields). Shuffle and Hot are honestly documented as reduced (no user/time seed; uniform age). No untracked TODO/FIXME or empty-catch found.
- **Accuracy:** Scoring math is correct and order-preserving; NaN is neutralized at construction + total_cmp sort + sentinel-aware normalize (verified against the prior Shortest/DateSaved regression — now fixed). No data races (executor is stateless over an immutable &SignalLedger). Genuine issue: the threaded query clock `now` is never used for any ledger read, so per-candidate decay/window reads each call Timestamp::now() — non-deterministic within a query.
- **Tech Debt:** Trending/cohort_trending double-apply velocity (sort formula base + redundant boosts), order-neutral but redundant. Social-graph trending path bypasses degradation substitution (documented no-op). Stale doc comment claims read_decay_score_at is 'out of scope' though it now exists in the ledger.
- **Maintainability:** Strong: intention-revealing names, focused functions, heavy WHY-comments tying each decision to spec sections and prior-review findings. finalize/normalize/read_agg are each well under 50 lines. One doc comment (mod.rs:181-184) is now factually wrong about ledger capabilities.
- **Extensibility:** Right abstraction: profiles are data (registered/validated/versioned), Sort/SignalAgg/Boost are open enums, DiversityConstraints uses a builder with reserved fields for future constraints. extra_boost closure cleanly factors the personalized vs base overlay. No over-engineering.
- **DRY:** read_agg/read_agg_for_sort share one path; the COARSE_VELOCITY_WINDOW constant is shared between read_agg and the social-graph path to prevent drift; score_candidates collapses the prior duplicated base/personalized loops. No material duplication in production code.
- **CLEAN:** Clear and neat. Session-keyword lowercasing and alphabetical-title reads are hoisted out of hot loops. Pure formulas are isolated and unit-tested. Minor: spec-vs-code score-on-tie semantics (0.5 vs 1.0) and shuffle/Hot reductions are the only rough edges, all order-neutral.

### `repl-cluster` — Replication: tenant/shard/migration/reconcile/control  _(11 files)_
- **Completeness:** Mostly complete with edge/error paths and strong tests (crash-survival, fault-injection, proptests). Gaps: gc_source is a state-only flip that never reclaims source-shard data; tenant_wal_dir is dead code (no production caller) so per-tenant storage isolation does not actually exist beyond rate-limit/routing metadata. Both are documented-scope-adjacent but the GC naming overstates behavior.
- **Accuracy:** Two real correctness/durability holes found: (1) finalize() persists with the non-strict swallow-error path yet its own comment claims a crash cannot revert the tenant — but gc_source never re-persists, so a swallowed finalize-checkpoint error silently reverts a finalized tenant to DualWrite on crash; (2) enter_dual_write rollback restores a FULL-router snapshot, clobbering any concurrent migration's entry for a different tenant. Rate-limiter CAS, advance_atomic_max, jump-hash, range validation, reconcile LWW/CRDT merge are all correctly reasoned.
- **Tech Debt:** Low-moderate. control.lag_for is single-shard-only (documented, debug_assert-guarded). gc_source carries a misleading name for a no-op-on-data transition. Migration timing/GC is a standalone primitive not wired into a production coordinator (with_persistence only exercised in tests).
- **Maintainability:** Strong. Intention-revealing names, focused functions under 50 lines, thorough WHY-comments on every concurrency primitive. The two durability comments (finalize line 209-211, migration struct doc) overstate guarantees relative to the code — a maintainer would trust them.
- **Extensibility:** Good. Transport trait-abstracted, routing strategy enum is open for Hash/Range/Single, RegionId/ShardId newtypes. Migration state machine is a clean primitive. No obvious YAGNI over-engineering.
- **DRY:** Good. find_shard_assignment shared between pinned and dual-write paths; advance_atomic_max centralized across the two HWMs; build_channel_endpoints reused for session transport. No significant duplication.
- **CLEAN:** Clean and efficient. Lock-free hot paths (atomics, DashMap), no needless hot-path allocation, consistent abstraction levels. One dead helper (tenant_wal_dir) and one misleadingly-named transition (gc_source) are the only smells.

### `repl-crdt` — Replication CRDTs (HLC/LWW/PN-counter/signal-state)  _(7 files)_
- **Completeness:** Strong. Every CRDT type carries example + proptest law coverage (commutativity/associativity/idempotency), HLC overflow/clamp edges are tested, and from_node_contribution's score/timestamp and bucket round-trip contracts are now pinned by tests. Gap: the LWWRegister commutativity proptest asserts a law that does not hold universally (equal-timestamp/different-value), and apply_crdt_state's multi-lambda reconciliation has no test.
- **Accuracy:** Mostly sound. Both prior CRITICALs are genuinely fixed: Hlc::update() now carries logical overflow into the wall (verified hlc.rs:310-316 + test update_strictly_advances_when_logical_saturated), and crdt_contributions passes hot.stored_score(0) not a decayed score (core.rs:608), eliminating the double-decay. One real residual: LWWRegister::merge is non-commutative under an identical full HlcTimestamp with differing values (verified by standalone repro) — unreachable for honest HLC-stamped writes, but an unstated/unenforced precondition. Plus a multi-decay-rate fidelity gap when CRDT state reconciles into multi-lambda hot slots.
- **Tech Debt:** Low. No magic numbers (LOGICAL_BITS/MASK/MAX, MAX_WALL_MS, NANOS_PER_SEC all named). node_buckets stores a full PNCounter per single-key node — over-allocation is documented and deliberately accepted to reuse the audited merge. No temporary hacks.
- **Maintainability:** Excellent. Intention-revealing names, dense WHY-comments on every non-obvious branch (overflow carry, key-aligned lookup, LWW tiebreak, cross-map invariant). All functions <50 lines and single-purpose. A reader can follow the convergence argument from the doc comments alone.
- **Extensibility:** Good. Shared lww_other_wins helper is the right seam for the two LWW call sites; forward_decay_step kernel is reused not re-implemented; storage engine untouched. CrdtSignalState's single-lambda model is the one place a likely future change (multi-rate reconciliation) is not yet abstracted.
- **DRY:** Good. The prior DRY drift (LWWRegister::write reimplementing the decision) is fixed — write and merge both route through lww_other_wins (lww_register.rs:91,104) with a test asserting they agree. Decay math centralized in forward_decay_step. No duplicated constants.
- **CLEAN:** Clean. force-set/merge paths do no needless clones (clone_from on the LWW value, Copy inserts in signal_state, per-node-max in PNCounter). No dead code, no mixed abstraction levels, no panics/unsafe in non-test code (all unwrap/expect/clone hits are test-only; the lone 'unsafe' token is a doc-comment word).

### `repl-shipping` — Replication: shipper / receiver / transport  _(7 files)_
- **Completeness:** Mostly complete. Shipper, receiver, transport, lag, idempotency, segment_id all have edge/error paths and extensive tests (fault injection, proptests, overflow, atomicity). Gap: when the receiver thread halts mid-life (corrupt segment or follower-WAL IO fault) there is no test or wiring that makes that death observable while the node stays open — the existing tests only observe the halt via join() at shutdown.
- **Accuracy:** Strong. Verified the BLOCKER-6 durability fix is real (apply_replicated_event appends to the follower WAL before in-memory mutation, core.rs:451-476), mid-payload all-or-nothing staging (receiver.rs:210-321), seq-overflow rejection (receiver.rs:240-253), idempotency floor (receiver.rs:261), and the WAL-position-preserving community filter (shipper.rs:450-510). One accuracy/operability defect: receiver death is not surfaced to health_check.
- **Tech Debt:** Low. control.rs lag_for is single-shard-only but documented and debug-asserted. MAX_PAYLOAD_BYTES is a named const. No copy-paste or hardcoded magic on the hot path in this zone.
- **Maintainability:** High. Functions are focused (<50 lines except the documented spawn_shipper loop), names are intention-revealing, and the WHY-comments are unusually thorough and accurate (they match the code).
- **Extensibility:** Good. Transport is a clean trait with a blanket Arc<dyn Transport> impl; TransportError's transient/permanent/too-large taxonomy is the right seam. Not over-engineered.
- **DRY:** Good. advance_atomic_max is shared by ReplicationState and the lag gauge; build_channel_endpoints is shared by WAL and session transports; last_seq_in_segment / count_events_in_segment / filter_segment_drop_local share the same decode-scan loop shape (mild, acceptable).
- **CLEAN:** Clean and efficient. Lock-free atomics on the read path, DashMap for operator-visible shipper state, staging Vec only on the cold replication apply path. One needless-clone note: segment_bytes.clone() per peer per send in the shipper (shipper.rs:330) is on the cold ship path, acceptable.

### `schema` — Schema builder / validation / migrations  _(10 files)_
- **Completeness:** Validation is thorough for what exists (signals, decay, windows, velocity, embedding slots, text fields, policies), each rejection path has a unit test. Two gaps: embedding dimensions enforce only `!= 0`, not the documented [2,4096] bound; the spec's migration/profile/cohort surface lives outside this module or is absent, and the spec header still says 'Implemented (M0-M8)' while flagging itself a pre-API sketch.
- **Accuracy:** Clean. Fingerprint hashes name+kind+decay+slot-dims; the 0x00..0x02 kind byte is a reliable delimiter after each non-empty lowercase signal name, so signal-name boundary collisions are impossible. Lambda precompute, WindowSet empty-check (runs on pre-dedup Vec, correct), and the saturating clock-anomaly Timestamp::now() are all correct. No data races (Schema is immutable post-build).
- **Tech Debt:** Low. One duplicated string literal (`entity_id`) split across two crate-internal constants coupled by comment only. No magic numbers in hot paths, no commented-out code, no orphan TODOs.
- **Maintainability:** Strong. Intention-revealing names, focused functions, and unusually good WHY-comments (the fingerprint-exclusion rationale and the `target` field's load-bearing-for-durability note are exemplary). build() is long but explicitly annotated and is a flat sequence of independent validation passes.
- **Extensibility:** Right-sized. Closed Window/EntityKind enums are deliberate (fixed storage layout) and documented as such. The builder pattern cleanly admits new signal attributes (positive_engagement was added without churn). Not over-engineered.
- **DRY:** One real violation: RESERVED_TEXT_FIELD_KEY (schema/error.rs:318) and ENTITY_ID_FIELD (text/index.rs:28) are independent `"entity_id"` literals. The prior EntityKind->byte 3-way duplication is now correctly collapsed to a single fingerprint_byte().
- **CLEAN:** Neat and consistent. No dead code (the seemingly-unused `target` field is documented as fingerprint-load-bearing), no needless allocation in any path, no misleading names. Production schema code contains zero unwrap/expect/panic/unsafe — every fallible path returns a typed SchemaError.

### `session` — Session & agent context / policies  _(14 files)_
- **Completeness:** Serde/journal codecs are exhaustively corruption- and roundtrip-tested; torn-tail vs corruption is distinguished with observability. Real gap: no test asserts a RESTORED session is expired by the sweeper or rejected by policy on its true age — the exact 'TTL honored across crashes' focus item is uncovered (see BLOCKER).
- **Accuracy:** Decay math correctly delegates to the canonical forward_decay_step kernel; out-of-order pre-decay and timestamp non-regression are correct; bounds checks and capped allocations are sound. The one real correctness defect: TTL/lease enforcement reads the non-durable monotonic Instant (started_at), not the durable started_at_ns, so a restored session's age resets to ~0.
- **Tech Debt:** SessionState's 14-field literal is hand-constructed at 2 production sites (start_session, restore_session_wal_events) with no constructor — they must be kept in lock-step manually.
- **Maintainability:** Strong: nearly every ordering/durability decision carries a WHY comment (e.g. sessions.rs:163-170 on started_at_ns, snapshot.rs:112 on deterministic ordering). Functions are focused; names are intention-revealing.
- **Extensibility:** Good: session-journal frames (v1/v2) and snapshots (v0x01-v0x03) are explicitly versioned with forward/backward-compatible decode; optional Signal fields use one shared push_optional framing.
- **DRY:** Byte-cursor helpers (read_u32/u64/utf8/len_prefixed, write_len_prefixed, capped_capacity) are correctly single-sourced across the whole session slice incl. saved_search; write_audit_entry shared by snapshot+audit codecs. Only duplication is the SessionState literal.
- **CLEAN:** Clean overall. Minor: SessionHotState::frozen_score uses Relaxed while every live read uses Acquire (benign — archive time, single-threaded close), and current_score recomputes wall-clock dt; no dead code, no needless hot-path allocation in this slice.

### `signals` — Signals: types, decay, velocity, windowed agg  _(14 files)_
- **Completeness:** Strong. Decay (in-order/out-of-order), velocity-as-count/duration, and all five windows (incl. real 30d day-bucket tier) are implemented; EWMA velocity and RelativeVelocity/Ratio are documented-not-implemented and explicitly rejected at profile registration (spec §5 note matches code), so there is no silent gap. NaN/Inf weight rejected at the public db.signal() boundary. Crash-injection points wired on the write and checkpoint paths. No orphan TODO/FIXME found.
- **Accuracy:** Sound. forward_decay_step kernel is mathematically exact and is the single source of truth; the hot-tier CAS-retry re-reads last_update_ns inside the loop (regression-tested for the stale-timestamp over-add). maybe_rotate serializes the whole minute→hour→day cascade behind the minute-CAS, so concurrent rotation cannot mis-apply an aggregate. Read-time rotation on &self via atomics is sound under DashMap read refs. No unwrap/expect/panic in non-test code; no lock held across await (no async here).
- **Tech Debt:** Low. NANOS_PER_SEC and NS_PER_{MIN,HOUR,DAY} are centralized; bucket sizes are named consts; the reverse-sum idiom is extracted to one helper. Restore's full-keyspace scan is documented as a known perf (not correctness) limitation tied to the key layout.
- **Maintainability:** Excellent. Dense but accurate WHY-comments on every non-obvious boundary (in-progress-hour fold, write-side vs read-side overlap bounds, 7-day seam slack). Field-offset codec is derived from named constants with compile-time size asserts. Functions are focused and under the size limit.
- **Extensibility:** Good. Checkpoint format is versioned (V1/V2 entry, V1/V2/V3 meta) with forward/backward-compatible decode. Decay kernel is a pure function reused by every tier. Window is a closed enum by design (documented rationale). No obvious over-engineering.
- **DRY:** Good. derive_signal_ids_and_lambdas is the single id/lambda derivation shared by global+cohort ledgers; get_or_create_entry is the single entry construction site; apply_event_local is the shared incremental-update core; sum_last_n_buckets is the one ring-sum. Field offsets defined once for both serialize and deserialize.
- **CLEAN:** Clean. Intention-revealing names, no dead code, Relaxed/Acquire/Release orderings each justified in module docs, allocation-free hot path. The clone-free integrity-hash iter path is a genuinely good efficiency choice.

### `storage-engine` — Storage KV engine (fjall/memory/keys)  _(8 files)_
- **Completeness:** Strong. Edge/error paths handled; round-trip, ordering, atomicity, crash, isolation, reopen all tested. Gap: no tracing spans despite CODING_GUIDELINES §11.2 naming storage ops non-negotiable; no test asserting scan_prefix surfaces a backend get-error mid-stream (the FjallBackend per-guard into_inner error path is untested).
- **Accuracy:** Correct. Verified against fjall 3.0.2 source: prefix() snapshots at SeqNo::MAX (streaming claim holds), OwnedWriteBatch commits under one seqno (atomicity real), insert() returns Err (no panic) on oversized/closed. map_fjall_err preserves io::Error kind + conservatively halts on unknown variants. No unwrap/expect/panic in non-test code; no unsafe; no lock held across await (sync API).
- **Tech Debt:** Low/acknowledged. scan_prefix(&[]) full-keyspace cost is documented as a known perf limitation at every restart caller; InMemoryBackend eager-materialize ceiling is documented and test-scoped. StorageError::BatchConflict is defined but never produced by either backend (dead variant, minor).
- **Maintainability:** Excellent. Intention-revealing names, every non-obvious decision carries a WHY comment grounded in the durability invariant, functions are small and focused, Tag::ALL is a single source of truth with a drift-guard test.
- **Extensibility:** Right-sized. StorageEngine trait is the clean boundary; FjallAtomicBatch deliberately NOT trait-abstracted with a documented rationale (cross-keyspace is fjall-specific, InMemory has no keyspaces) — correct YAGNI call, not over-engineering.
- **DRY:** Good. open_keyspace dedupes the three-keyspace open path; rotate_only/flush_all share one fsync; MAX_TAG_BYTE derived from Tag::ALL removed the prior dual-range proptest drift. as_byte/from_byte parallel tables are guarded by all_byte_mappings_consistent.
- **CLEAN:** Clean, logical, neat. No dead code beyond the unused BatchConflict variant; allocations are intentional (Vec capacity pre-hinted in encode_key); abstraction levels are consistent within each file.

### `storage-indexes` — Secondary indexes (range/bitmap/filter)  _(8 files)_
- **Completeness:** Edge/error paths are well covered: inverted bounds, empty intervals, truncated/corrupt persisted keys, unknown equality fields, and the predicate-vs-bitmap split all have explicit handling and tests. 80 zone tests pass. The deferred-filter 'return-universe' contract is fully guarded by reject_negated/reject_or validators wired into both pipelines. Gap: RangeIndex::load_from_kv_pairs does not reject trailing bytes after a fixed-width value (W).
- **Accuracy:** Range, bitmap, AND/OR/NOT, and selectivity logic are correct. is_empty_interval correctly clamps every BTreeMap::range panic case (lo>hi, equal-excluded). into_predicate uses checked u32::try_from (no truncation). Lock poisoning is recovered via into_inner rather than panicking. No unwrap/expect/unsafe in non-test code (the one unwrap_or_default in evaluate_and is provably non-empty). NOT-over-universe contract is sound given the universe only grows (no live item-delete path). Clean.
- **Tech Debt:** Minimal. RANGE_KEY_PREFIX / SUFFIX_TAG / ENCODED_KEY_PREFIX_LEN are centralized so write and read sides cannot drift. No magic numbers, no copy-paste blocks, no temporary hacks. The RangeKeyCodec trait removes the only duplication between u32/u64 persistence paths.
- **Maintainability:** Names are intention-revealing, functions are small and focused (<50 lines), and the WHY-comments (W22 scrub, NOT-over-universe contract, u32 bound enforcement) explain non-obvious invariants well. Consistent structure across bitmap and range indexes.
- **Extensibility:** RangeKeyCodec is the right abstraction for adding new numeric value widths. FilterExpr's deferred-variant design cleanly separates index-backed from post-filter evaluation. try_eq returns a typed error for unknown fields, leaving room for a generic field-equality variant. Not over-engineered.
- **DRY:** Strong. The key-prefix constants, the collect_matching walker (single shared traversal rule), and the RangeKeyCodec trait all eliminate would-be duplication. selectivity's range() and total_count()'s union share the same union pattern but that is acceptable.
- **CLEAN:** Clear and neat. One efficiency note: RangeIndex::total_count() and BitmapIndex::total_count() compute a full union of every value bitmap on each call, and RangeIndex::selectivity() materializes the whole range bitmap — O(n) per call (SUGGESTION). No dead code, no misleading names, no needless clones on a hot path.

### `storage-vector` — Vector index (usearch/brute/registry/lifecycle)  _(10 files)_
- **Completeness:** Mostly complete: deserialize hostile-input guards, soft-delete tombstones, and rebuild-from-store are all present and tested. Gap: rebuild aborts on the FIRST bad embedding (one corrupt/mismatched vector drops every embedding for that entity kind from ANN); no test for that path. UsearchIndex has tests but no production wiring.
- **Accuracy:** Logic is correct. No unwrap/expect/panic in non-test code; RwLock poison is mapped to Backend errors, not unwrapped. Durable-first ordering in lifecycle ops is sound and crash-window-reasoned. BruteForceIndex::deserialize uses fully-checked arithmetic against the real buffer length — no OOM/panic on hostile count/dims (verified by tests). No locks held across await (sync code).
- **Tech Debt:** UsearchIndex is dead in production — every path (registry rebuild, write auto-register, query pipeline, users.rs) hardcodes BruteForceIndex. The 'production HNSW' engine and its save/load/view/tombstone machinery are exercised only by tests. ef_search per-query override is a documented permanent no-op.
- **Maintainability:** Strong. Intention-revealing names, focused functions, excellent WHY-comments (SECURITY block in deserialize, durability-invariant docs on lifecycle ops). Single-source-of-truth consts for HNSW defaults and the ARCHIVED_PREFIX marker.
- **Extensibility:** VectorIndex trait is the right boundary and is clean. But there is no backend selector to actually choose UsearchIndex vs BruteForceIndex from schema/config — the abstraction exists yet only one impl is reachable, so the extensibility is theoretical.
- **DRY:** Good. read_*_le, validate_dimensions, ef_search_unsupported_warn, ARCHIVED_PREFIX, and DEFAULT_* consts are all centralized. select_top_k and cmp_distance are shared. No notable duplication.
- **CLEAN:** Clean and neat. Distance comparator, top-k selection, and tombstone handling are clear and efficient (select_nth_unstable_by for k<<n). Minor: doc strings on the index trait still describe USearch as the production engine that the code never instantiates outside tests.

### `text` — Tantivy full-text indexing  _(11 files)_
- **Completeness:** Strong. Crash recovery is unconditional rebuild-at-open for BOTH item and creator indexes (state_rebuild.rs:761), transient-commit-failure resilience with backoff + health flag, open-failure logs loudly before degrading, bounded outbox. Edge cases (genuine entity 0, absent-vs-unreadable dir, empty field set, delete-of-nonexistent, update-removes-field) are all tested; 46/46 text tests pass.
- **Accuracy:** No unwrap/expect/panic and no unsafe in non-test code; delete-then-add update is atomic per-commit; strict Column::first avoids aliasing entity 0; atomic Acquire/Release orderings are correct and documented. One real defect: producer uses a BLOCKING send on a bounded channel while holding the text_tx mutex (items.rs:197-205 + text_syncer.rs:40/207), so a persistent commit outage stalls the whole item write path.
- **Tech Debt:** Three doc comments cite a non-existent module path `db::mod::rebuild_text_indexes_at_open` (writer.rs:100, items.rs:196, creators.rs:31) — the function lives in db::state_rebuild. Spec/Cargo version drift (spec says Tantivy 0.25+, Cargo pins 0.22) is outside the zone but worth a note.
- **Maintainability:** Excellent. Intention-revealing names, load-bearing WHY-comments on every non-obvious decision (merge constants, health-flag orderings, absent-vs-unreadable distinction), constants are named and documented. Functions are focused and well under 50 lines.
- **Extensibility:** The shipped `TextIndex` is a concrete struct, not the `TextIndex` trait + `MockTextIndex` the spec (Sec 13/1.2) describes; tests use `ephemeral()` instead of a mock. Reasonable YAGNI for single backend, but it diverges from the documented abstraction and means no module boundary actually enforces 'no Tantivy types leak out' — callers in query/pipeline.rs touch tantivy::Searcher directly.
- **DRY:** Good. searcher_stats() centralizes the (segments,docs) shape for both read paths; steady_state_merge_policy() centralizes merge config for open() and ephemeral() (a prior duplication explicitly called out as fixed). ENTITY_ID_FIELD literal defined once.
- **CLEAN:** Clean throughout. NaN-neutralizing total_cmp sort downstream (pipeline.rs:346) is a genuinely careful touch; no dead code, no needless hot-path allocation in the collector or syncer apply loop.

### `tidal-net` — tidal-net: gRPC transport / TLS / mTLS / circuit breaker  _(14 files)_
- **Completeness:** Mostly complete: TLS/mTLS positive AND negative paths tested (untrusted CA, absent client cert, plaintext-without-opt-in all rejected); config invariants tested; circuit-breaker state machine exhaustively tested. GAP: the documented server-death-vs-clean-shutdown distinction is not actually wired — server_terminated() exists but has zero callers, so a follower whose serve loop dies exits its receiver thread Ok(()) indistinguishably from a clean shutdown.
- **Accuracy:** One real defect: config.validate() runs in PeerPool::new AFTER mpsc::channel(config.channel_capacity) is built in GrpcTransport::new, so channel_capacity==0 panics (tokio asserts buffer>0) instead of returning the typed Internal error the doc promises. Otherwise correct: circuit breaker single-probe invariant sound; no std Mutex/RwLock guard held across .await (the inbound_rx guard is held across a synchronous block_on by design, single-consumer); TLS server-cert verification is NOT disabled (no danger_accept_invalid_certs); overflow-checked u32->u16 conversions on the wire; #![forbid(unsafe_code)].
- **Tech Debt:** rustls-pemfile is declared in Cargo.toml but unused in src (TLS material is read via Identity/Certificate::from_pem) — violates the 'minimal, intentional, auditable deps' guideline. GrpcTransportConfig::default sets insecure=true; tidal-server's cluster path hardcodes insecure=true with no TLS knob (out-of-zone but relevant to 'no insecure defaults').
- **Maintainability:** Strong. Intention-revealing names, focused functions (<50 lines), and unusually thorough WHY-comments on every subtle decision (shutdown latch, codec limits, backpressure classification, permanent-vs-transient mapping).
- **Extensibility:** Appropriate. stream_segments and heartbeat are honestly documented deferrals returning explicit Status::unimplemented / minimal-real-behavior rather than silent stubs. Transport trait boundary keeps network code out of the core crate.
- **DRY:** Acceptable. The codec max-size setting and the TLS-asymmetry rejection are duplicated across client/server, but each duplication is intentional (both ends must enforce independently) and documented. MAX_PAYLOAD_BYTES is mirrored from the engine with a compile-time cross-check.
- **CLEAN:** Clean overall. Minor: server_terminated is effectively dead code (no callers); CircuitState::HalfOpen carries no in_flight field despite the module doc referencing one (the invariant is enforced by the variant itself, so the doc is slightly misleading).

### `tidal-server` — tidal-server: cluster server / router / scatter-gather  _(11 files)_
- **Completeness:** Cluster routes, scatter-gather merge, partial-failure, and config loading are well-covered with tests. GAP: cluster router omits the timeout + concurrency limits the standalone router enforces; sharded write handlers run blocking DB work directly on the reactor with no offload; the multi-process cluster_e2e test asserts nothing across process boundaries (every node runs all regions in-process, queries only hit node 0).
- **Accuracy:** Scatter-gather deadline/degradation logic, NaN-safe sort, dedup, and per-creator reconciliation are correct and well-reasoned. Two real correctness gaps: cluster /signals returns 204 while follower ship is best-effort (send errors swallowed in the engine), and scatter-gather spawns one unbounded detached OS thread per shard per request with no global cap on the un-throttled cluster router.
- **Tech Debt:** cluster.rs (1112 lines) and scatter_gather.rs (1678 lines) each carry an acknowledged, deferred split with a tracking note — reasonable. Hardcoded 2MB body limit literal duplicated instead of reusing router::BODY_LIMIT_BYTES.
- **Maintainability:** Strong: intention-revealing names, thorough WHY-comments, shared dto/health modules prevent standalone/cluster drift. Functions mostly focused; a few handlers exceed 50 lines but read clearly.
- **Extensibility:** offload abstraction (read pool vs runtime-free write pool) is the right boundary. ShardRouter delegation prevents write/read hash divergence. Topology YAML is data-driven. No over-engineering observed.
- **DRY:** Good factoring via merge_and_assemble (shared RETRIEVE/SEARCH tail), dto.rs, health.rs, offload_read. Minor: 2MB body-limit constant and ScatterGatherInfo construction block are duplicated.
- **CLEAN:** Clean and logical overall. The duplicated 2MB literal and the cluster router's missing middleware stack are the main blemishes; dead-ish ShardedFeedQuery.deadline_ms clamp path is fine.

### `tidalctl` — tidalctl: CLI  _(11 files)_
- **Completeness:** Largely complete — every command has integration tests, all five subcommands honor the documented exit-code contract (status/diagnostics exit 2 on degraded WAL, verified by tests). One gap: scope-stats' in-band integrity signal (`complete`) does not cover the case where diagnose_wal itself fails on a corrupt checkpoint (W1). recover's `--verify-only=false` rejection is correct (no silent no-op).
- **Accuracy:** Strong. No unwrap/expect/panic in non-test code (crate forbids unsafe, denies unwrap_used). Saturating arithmetic on checkpoint-age nanos (diagnostics.rs:41-48) and uncheckpointed-bytes (diagnostics.rs:192) is correct against overflow/hostile timestamps. CliError::to_json has an infallible fallback. One accuracy defect: scope-stats reports `complete: true` when integrity could not actually be verified (W1). One minor swallow: list_segments error → 0 (S1).
- **Tech Debt:** Low. No magic numbers (REASON_* and OFFLINE_UNAVAILABLE are single-sourced constants). recover's `verify_only` is a documented forward-compat stub, not dead code. The prior review's three Accuracy items (status exit code, fabricated checkpoint_age 0, text-index 0) and the main.rs-too-large item are all genuinely remediated at HEAD.
- **Maintainability:** Excellent. main.rs split into commands/ + json + wal_state modules; every function under 50 lines except the deliberately-flat format_pretty (annotated). Names are intention-revealing; WHY-comments are abundant and accurate (verified against engine behavior).
- **Extensibility:** Appropriate. The five-command enum + per-module run() is the right shape; DirsOutput is shared between status and paths so the projection cannot drift (guarded by a test). recover's reserved flag is YAGNI-conscious (rejected, not half-built). No over-engineering.
- **DRY:** Good. WalState::from_report vs gather_wal_state are two intentional IO paths (one reuses a held report, one scans) and the divergence is documented. DirsOutput is the single source for the six-path projection. Offline-unavailability reasons are single-sourced and a test asserts JSON/pretty agreement. The two pretty-formatters (recover, diagnostics, scope-stats) repeat the section-heading idiom but are not worth extracting.
- **CLEAN:** Clean and neat. All JSON flows through one serde_json path (control-char escaping hole closed, test-guarded). BTreeMap keeps offline_unavailable output order stable. No needless clones in any hot path (this is a one-shot CLI). The `unwrap_or(0)` on list_segments (S1) is the one slightly-misleading construct.

### `wal-format` — WAL record/format (de)serialization  _(4 files)_
- **Completeness:** Strong. Both formats cover encode/decode, version compat (v1/v2/v3 batch; legacy-v1/v2 session), torn-tail vs corruption distinction, and have property tests + per-byte corruption tests. Gap: no encode-side validation of variable-length string lengths against the u16 prefix (see finding 1); no test exercises a present-but-short v2 optional field (finding 3).
- **Accuracy:** Logic is correct on the read path. Length fields read from disk are bounds-checked (session.rs:293, reader.rs:197) before any slice, and the only length-driven allocation in decode_batch is Vec::with_capacity(event_count) capped at MAX_EVENTS_PER_BATCH=256 — no OOM vector from a hostile length. BLAKE3 covers header[0..32]+events (batch) and [marker,version,type]+payload (session). No unwrap/expect/panic in non-test code except one provably-infallible documented expect (batch.rs:574). One encode-side silent-truncation defect (finding 1).
- **Tech Debt:** Low. Offsets/sizes are named constants (HEADER_PAYLOAD_LEN_OFFSET, SESSION_*), owned in one place to prevent drift between scanner and decoder. No magic numbers of concern. The u16-length encode without a guard is the one latent debt item.
- **Maintainability:** Excellent. Intention-revealing names, focused helpers (read_u64_le/read_str_field/decode_typed_record), and unusually thorough WHY-comments explaining the corruption-vs-torn-tail contract. Functions are <50 lines except decode_signal_record (~80) which is linear and readable.
- **Extensibility:** Good and appropriately scoped. Version dispatch (event_size_for_version, v2 forward-compat unknown-type skip, reserved-byte rejection) is the right abstraction for the likely change (new format version) without YAGNI over-engineering.
- **DRY:** Good. parse_base shared by legacy/v3 decode; push_optional centralizes the [flag][payload] framing; compute_checksum/session_record_checksum each single-sourced. Minor: the u16 length-prefix write idiom (len as u16).to_le_bytes() + bytes is repeated 4x in encode_session_event without a push_str_field helper mirroring read_str_field (finding 2).
- **CLEAN:** Clear, logical, neat. No dead code, no needless clones in these files (ann_for_wal clone lives in sessions.rs, out of zone). Reserved-byte handling and the layered corruption semantics are precise.

### `wal-recovery` — WAL read / replay / dedup  _(8 files)_
- **Completeness:** Strong. Edge/error paths (empty dir, empty segment, torn tail, mid-batch corruption, sequence overflow, checkpoint filter) all have tests. Gap: no test or detection for corruption in a NON-final segment when a later intact segment exists (see CRITICAL finding). No TODO/FIXME, no commented-out code, no empty catches.
- **Accuracy:** Mostly correct. Two-phase validation (bounds then BLAKE3) is sound; checked_add guards corrupt first_seq overflow; saturating_add guards next_seq wrap; dedup record-after-fsync ordering is correct. One real defect: recover() truncates a torn/corrupt tail on ANY segment and keeps replaying later segments with no gap detection -> silent data loss + sequence gap on a corrupt sealed segment.
- **Tech Debt:** Low. REPLAY_EVENTS_PER_SEC / CHECKPOINT_RESTORE_SECS are documented coarse constants, not magic numbers. Session-format offset constants are named and centralized. No copy-paste; scan logic shared via scan_segment_inner.
- **Maintainability:** Excellent. Functions are focused (<50 lines except the necessarily-long decode_session_events_with_diagnostics and diagnose_wal, both flagged with allow attributes and well-commented). Names are intention-revealing. WHY-comments are dense and load-bearing (e.g. why record-after-fsync, why > vs >= replay predicate).
- **Extensibility:** Good. Format versioning (v1/v2/v3) handled cleanly; session frame v1/v2 forward-compat is explicit. scan_segment_inner is the single shared scanner feeding readonly/summary/recover, preventing drift. No YAGNI over-engineering.
- **DRY:** Strong. SegmentScan/scan_segment_inner is the one decode loop reused by recover, read_all_events, diagnostics, and readonly scan. payload_len_from_header is the single owner of the header offset. push_optional centralizes optional-field framing. Diagnostics and recover deliberately share scan_segment_summary so the >/>= predicate can't diverge again.
- **CLEAN:** Clean. No dead code, no needless allocation in the decode path (read_to_end once per segment is necessary). Abstraction levels are consistent. The only blemish is the asymmetry where the session journal surfaces corruption_detected but the signal WAL recover() surfaces nothing for a corrupt non-final segment.

### `wal-write` — WAL write path & compaction  _(11 files)_
- **Completeness:** Solid. Every error path in the commit routine resolves caller replies (success, dedup sentinel, and error). Crash-recovery, rotation, oversized-batch, clock-anomaly, sequence-overflow, and active-segment-clamp cases all have dedicated tests. Gap: no test exercises a flush failure that occurs DURING segment rotation (rotate() opening a new file failing), though the error is propagated correctly. No TODO/FIXME or commented-out code found.
- **Accuracy:** Strong. fsync ordering is correct (data via sync_data; dir entry via sync_all on create/rotate/delete/checkpoint-rename). The active-segment clamp in both compact_wal_online and handle_aux_command genuinely prevents unlinking the live inode. Dedup is recorded only after durable flush, so retries of a failed batch are accepted. Sequence overflow on corruption-controlled first_seq is checked. No unwrap/expect/panic in non-test code; no unsafe; no lock held across await (writer is a dedicated thread, no async). One real durability caveat on macOS (see findings).
- **Tech Debt:** Two items: WalError::SegmentFull is dead (defined+tested, never constructed). Magic-number 0 dedup sentinel is well-documented. No hardcoded config beyond named consts.
- **Maintainability:** Excellent. Intention-revealing names, the flush_batch/partition_dedup/handle_aux_command extraction keeps the steady-state and shutdown-drain paths from diverging, and the rustdoc explains the WHY (hazard analysis) at every durability-critical site. run_writer is over 50 lines but is cohesive and annotated with #[allow(clippy::too_many_lines)] with justification.
- **Extensibility:** Right-sized. Shard/region identity is threaded through the writer config and filename format (v1/v2) for the cluster path without over-engineering. The CommandOutcome enum centralizes per-command dispatch across three loops. No speculative abstraction.
- **DRY:** Good. The two error-notify loops (flush_batch err arm and the shutdown-drain already-failed arm) duplicate the 'send synthetic Io error to each reply' pattern, but the duplication is small and the contexts differ. compact_wal/compact_wal_online correctly share compact_segments. delete_segments_before (segment.rs) and compact_segments (compaction.rs) are two near-identical delete loops that differ in dir-fsync — see findings.
- **CLEAN:** Clear and neat. One doc-accuracy issue: segment.rs sync() rustdoc overstates durability on macOS ('genuinely sufficient', 'fully durable') where fsync does not flush the drive cache without F_FULLFSYNC.


---

_Generated 2026-06-08 from workflow run `wf_79dbc004-39f` (29 Opus zone reviewers + 56 Sonnet adversarial verifiers). BLOCKERs re-confirmed against source by the orchestrator._