tidaldb/docs/reviews/M0-M10-seven-dimension-review.md

# tidalDB M0–M10 — Seven-Dimension Code Review

**Scope:** Full engine across milestones M0–M10 — `tidal` (db, wal, signals, storage, query, ranking, entities, governance, replication, crdt, schema, session, text, cohort/load/testing), `tidal-net`, `tidal-server`, `tidalctl`. ~86K LOC of Rust source across 28 subsystem slices.

**Method:** Each slice reviewed against the seven-dimension protocol (Completeness · Accuracy · Tech Debt · Maintainability · Extensibility · DRY · CLEAN) plus the project's `CODING_GUIDELINES.md` and Rust red-flag tables, in the correctness-first "trust it at 3am" standard. Every BLOCKER/CRITICAL was then handed to an independent adversarial verifier instructed to *refute* it by reading the actual code; only survivors are reported below.

---

## Remediation status (implemented)

All findings below have been remediated (see the post-remediation re-review section after this list for six further fixes a later adversarial pass surfaced). Workspace is green: `cargo clippy -D warnings` on all four crates (incl. `tidaldb --features metrics`) clean, `cargo build --workspace --all-targets` clean, `cargo fmt` applied, `scripts/check-docs.sh` OK; engine lib 1672 tests / 0 failures (deterministic), engine integration + sibling suites green. ~170 regression tests were added.

- **Both BLOCKERs and all nine CRITICALs** are fixed with dedicated regression tests.
  - The BLOCKER #1 fix is read-side (`30d = last-7d hour tier + day buckets older than 7 days`); the report's suggested "clear the hour buckets on day rotation" would break the 7-day window (which legitimately reads all 168 hour buckets), so the read-side approach was used instead.
- The **WARNINGs and SUGGESTIONs** were remediated across the named slices (DRY extractions, validation, durability ordering, doc accuracy, perf, bounds). A handful were intentionally taken as documented-limitations where the reviewer offered that alternative (e.g. Rising/MostFollowed/CreatorEngagementRate remain custom-profile-only with a doc note; `lag_for`'s single-shard scope stays documented).
- **W1** (`wal_dir` override): now honored end-to-end via `WalConfig::wal_dir_override` + `Config::resolved_wal_dir`, threaded to the WAL writer and every reader; `cache_dir` remains a documented reserved no-op (no consumer, and the storage backend exposes no separate cache-dir knob).

**Previously-deferred item — now FIXED (both read AND write side):** a *related* 24h/7d/30d windowed double-count for **continuously-active** entities (events with sub-60-minute gaps crossing an hour boundary) — the same tier-overlap class as BLOCKER #1 but at the minute→hour boundary, which the original review did not flag. The minute tier is a rolling 60-minute window (it must be, to answer `OneHour`), aged only by the elapsed minutes on each rotation, so after a gap it still holds the previous hour's tail that the rollup already copied into the newest completed hour bucket.

There were **two** overlap sites — and a first remediation pass fixed only the read side, which an adversarial re-review correctly flagged as a half-fix (the durable hour tier was still corrupted on the pure write path: a 90-minute dense history read at 120 min reported 477 for 361 real events). The complete fix bounds **both** sites and the day tier:

- **Read side** (`windowed_count` → `sum_current_hour(now_ns)`): folds in only the last `k` minute buckets, `k = (now_ns − last_hour_rotation_ns)/60s + 1` (capped at 60) — the minutes since the last hour rotation, disjoint from every completed hour bucket.
- **Write side** (`maybe_rotate`'s `hour_agg`): the hour rollup snapshots only the in-progress hour, `k = (last_min − last_hour)/60s + 1` minute buckets walking back from `current_minute` — anchored at the rollup-time `last_min` (where `current_minute` still points), **not** `now`. (A naive `now`-anchored bound evaluates to the full 60 at the hour boundary and does not fix it — the verifier confirmed this.)
- **Day tier** (`maybe_rotate`'s `day_agg`): the same overlap one tier up. The day rollup now folds in only the completed hour buckets inside the completing day, `j = (last_hour − last_day)/3600s` (capped 23), plus `hour_agg` — not the rolling "23 most recent hour buckets".

No persisted-format change: all bounds derive from `last_minute/hour/day_rotation_ns`, already in `BucketedCounterSnapshot`. Six regression tests were added that read **after a gap** and on the **pure write path** (the cases the at-`end` tests structurally could not catch); each fails against the unbounded code (477/361, 440/343, day-tier inflation) and passes against the fix. See `signals/warm.rs`.

---

## Post-remediation re-review (2026-06-08) — additional fixes

An adversarial multi-agent re-review of the uncommitted remediation diff surfaced six further issues (one BLOCKER, two CRITICAL, two WARNING, plus the day-tier extension above), each verified against the code and now fixed with regression tests:

1. **[BLOCKER] warm-tier write-side double-count** — the half-fix above; now fully bounded on read + write + day tier.
2. **[CRITICAL] W22 stale-index half-fix** (`db/items.rs`, `db/state_rebuild.rs`) — `RangeIndex::delete_entity` was added and unit-tested but never wired into the live re-index path, so an item *overwrite* left it indexed under both the old and new category/format/creator/tag/duration/created_at value (phantom RETRIEVE/SEARCH hits; corrupted recency). Fixed: `write_item_with_metadata` now reads the prior metadata and `ItemIndexes::scrub`s every index (plus the creator-keyed `CreatorItemsBitmap`, which gained `remove_item`) before re-inserting. TidalDb-level regression test added.
3. **[CRITICAL] checkpoint `read()` mutated disk** (`wal/checkpoint.rs`) — a `remove_stale_temp` call had been added to the shared, documented-read-only `CheckpointManager::read()`, breaking read-only-mount forensic inspection (EROFS) and racing the writer's rename. Fixed: `read()` is side-effect-free again; the orphan-temp sweep moved to `sweep_stale_temp`, called from the writable WAL `recover()` path (best-effort).
4. **[WARNING] W13 partition_id never wired** (`db/mod.rs`) — `set_partition_id` existed but no construction site called it, so cluster nodes still emitted colliding `partition_id="0"`. Fixed: a shared `install_metrics_wiring` helper now stamps the real shard on both paths; `#[allow(dead_code)]` removed; construction-level test added.
5. **[WARNING] SocialGraph/InCollection u32 truncation** (`query/executor/post_filter.rs`) — the inclusion arms used `id.as_u64() as u32`, which *aliases* a `> u32::MAX` id onto a low slot and could wrongly retain it, contradicting the `entity_as_u32` invariant. Both arms now route through the checked conversion (dropping non-representable ids); `entity_as_u32` made `pub(crate)`; overflow-id regression test added.

Workspace re-verified green after these fixes (clippy `-D warnings` on all four crates incl. `--features metrics`, `build --workspace --all-targets`, `fmt`, `check-docs.sh`; engine lib + integration + sibling suites; `metrics_integration` stress 60/60 after the non-blocking-accept fix).

---

## Summary

| Severity | Count |
|---|---|
| BLOCKER | 2 |
| CRITICAL | 9 |
| WARNING | 50 |
| SUGGESTION | 80 |

**Adversarial verification:** 16 high-severity findings examined → **14 confirmed**, **2 refuted** as false positives (and 3 confirmed-but-downgraded to WARNING).

**Recommendation: REQUEST_CHANGES.** Two BLOCKERs are silent data-loss/correctness holes on documented, expected operations (signal-checkpoint trim, 30-day window). The nine CRITICALs are real, reachable bugs — none are happy-path-instant crashes, which is why they're CRITICAL not BLOCKER, but several are quadratic cliffs, OOM paths, unbounded leaks, or silent data deletion. The engine's durability *machinery* is genuinely strong (see What's Good); the defects are in the seams where derived state, limits, and lifecycle cleanup are not wired all the way through.

### Top action items

1. **Fix the signal-checkpoint trim hole (BLOCKER, `signals/checkpoint/mod.rs`)** — after the ledger ever exceeds the 5M trim threshold, the first restart silently loses *all* checkpoint-only signal state because checkpoint never deletes stale keys, the integrity hash mismatches, restore returns `Ok(None)`, and the covering WAL has already been compacted. Make checkpoint authoritative over the keyspace (emit deletes for evicted rows in the same atomic batch).
2. **Fix the 30-day windowed-count double-count (BLOCKER, `signals/warm.rs`)** — for ~24h after every day boundary, day-tier rollup double-counts the still-populated hour buckets; 116 real events report as 216. This directly inflates trending/velocity ranking. Clear the hour buckets on day rotation (mirror the minute→hour symmetry) and add a dense-in-progress-day regression test.
3. **Kill the two unbounded-growth / quadratic paths** — the community ledger does a full O(total-contributions) durable snapshot on *every* accepted contribution (`db/communities.rs` → `governance/community_ledger.rs`, O(N²) ingest), and `notification_tracker::evict_old_days` has zero callers so its maps leak forever. Both are simple wirings (incremental put; sweeper tick).

---

## BLOCKER & CRITICAL findings (verified)

### 1. [BLOCKER] 30d windowed count double-counts ~24h of events for a full day after each day boundary
**Slice:** `signals-decay-hotwarm`  **Dimension:** Accuracy  **Location:** `tidal/src/signals/warm.rs:213, 471-505`  **Confidence:** 97

**Issue.** windowed_count(ThirtyDays) returns sum_current_day() + sum_last_n_days(30). sum_current_day() reads the in-progress hour (minute buckets) plus the 23 most-recent hour buckets. But rotate_day/rotate_days roll those same 24 hours of hour-tier data into a new day bucket WITHOUT ever clearing the hour buckets (the only hour_buckets write is rotate_hour at line 294, which overwrites the next slot on hour rotation; day rotation never zeroes hours). For roughly a full day after each day boundary, the just-rolled-up events live in BOTH the still-populated hour buckets (read by sum_current_day) AND the new day bucket (read by sum_last_n_days(30)). Verified empirically by a temporary test: 116 real events reported as 216 (sum_current_day=101 + sum_last_n_days30=115). Compare the 24h path, which is correct precisely because minute->hour rotation DOES clear the minute buckets via 60 rotate_minute calls.

**Why it matters.** read_windowed_count and read_velocity for the 30d window roughly double (then drift) for any entity active around a day boundary. The 30d window directly feeds trending/velocity ranking, so candidate scores for the most-active items are systematically inflated by a derived-state bug — exactly the 3am production incident the project rules warn about. It also violates the documented invariant 'windowed aggregates equal the sum of events within the window' (CODING_GUIDELINES §3/§8). Existing tests (thirty_day_window_aggregates_across_days asserts only >=9; events_outside_30d_window_not_counted asserts only <=31) use one event per day and never exercise a dense in-progress day across a boundary, so the bug ships untested.

**Fix.**

The day tier must not re-count what the hour tier still holds. Two correct options: (1) After rolling day_agg into a day bucket in rotate_days, zero the 24 hour buckets that fed it (mirror the minute-buckets-cleared-on-hour-rotation symmetry) so sum_current_day no longer re-reads them; or (2) redefine the 30d read to NOT add sum_current_day on top of the day ring when those hours have already cascaded — e.g. make day bucket [current_day] the live in-progress day fed incrementally and sum exactly DAY_BUCKETS-1 completed days plus the in-progress day once. Option (1) is the smaller change and keeps the cascade model consistent across tiers. Add a regression test that records a dense in-progress day (e.g. 5 events/hour for 23h), crosses the day boundary with one more event, and asserts windowed_count(ThirtyDays) == all_time_count() (e.g. 116, not 216).

> _Verifier (97): Confirmed by reading the code and by a real test: rotate_day/rotate_days (warm.rs:304-310, 484-505) roll the 24h hour-tier aggregate into a new day bucket but never clear the hour buckets (rotate_hour only overwrites the next slot), so windowed_count(ThirtyDays) at line 213 counts the same events twice — empirically 122 real events reported as 238 (sum_current_day=118 + sum_last_n_days30=120), drifting from ~2x down over ~24h after each day boundary; this 30d count feeds ranking via read_windowed_count/read_velocity (ranking/executor/helpers.rs:74,77, builtins.rs:167) and violates the CODING_GUIDELINES.md:226 invariant "windowed aggregates equal the sum of events within the window," and existing tests (one event/day, loose >=9 / <=31 bounds) never exercise it._

---

### 2. [BLOCKER] checkpoint() never deletes stale signal keys; after a trim the integrity hash mismatches and restore silently drops all signal state
**Slice:** `signals-ledger-checkpoint`  **Dimension:** Accuracy  **Location:** `tidal/src/signals/checkpoint/mod.rs:68-125 (checkpoint), 142-208 (restore)`  **Confidence:** 90

**Issue.** checkpoint() builds a WriteBatch of ONLY put() ops for the entries currently live in the DashMap, plus the meta key. It never deletes storage keys for entities present in a previous checkpoint but no longer in the map. The LRU trimmer (signals/trimmer.rs, invoked every checkpoint cycle in db/state_rebuild.rs once the DashMap exceeds DEFAULT_MAX_SIGNAL_ENTRIES) removes entries from the DashMap but does NOT delete their storage keys. So after any trim-then-checkpoint, storage retains orphaned Tag::Sig rows. On restore() the scan at mod.rs:174-183 collects EVERY non-meta Tag::Sig value (including orphans) into raw_values, but the payload_hash in meta was computed in checkpoint() only over the live entries. verify_checkpoint_payload therefore fails, restore() returns Ok(None) (mod.rs:186-194), and the system falls back to full WAL replay.

**Why it matters.** This is a durability/data-loss hole, not just a perf fallback. The periodic checkpoint path runs compact_wal_online(dir, seq) immediately after a successful checkpoint (state_rebuild.rs:524), deleting WAL segments whose events are covered by the checkpoint. When the next restart's restore() fails the integrity check and falls back to WAL replay, the pre-checkpoint events needed to rebuild those aggregates have already been compacted away. Net: on the first restart after the ledger has ever exceeded the trim threshold (the entire reason the trimmer exists — 5M+ entries), ALL signal aggregate state reconstructable only from the checkpoint is silently lost and the ledger comes back effectively empty. Ranking quality collapses with no error surfaced. This is exactly the 3am-incident class the guidelines forbid: derived state that is NOT rebuildable from the WAL after a documented, expected operation.

**Fix.**

Make the checkpoint authoritative over the Tag::Sig keyspace so the on-disk set always equals the in-memory set. In checkpoint(), after building/sorting kv_pairs, enumerate existing Tag::Sig keys (excluding meta) and emit batch.delete() for any not in the live set, in the SAME atomic WriteBatch: let live: HashSet<&[u8]> = kv_pairs.iter().map(|(k,_)| k.as_slice()).collect(); for item in storage.scan_prefix(&[]) { let (k,_) = item?; if let Some((eid, Tag::Sig, suf)) = parse_key(&k) { if !(eid == EntityId::new(0) && suf == META_SUFFIX) && !live.contains(k.as_slice()) { batch.delete(k); } } } then add the puts. Because put+delete are one atomic write_batch, restore() scans exactly the live set and the hash matches. Add a regression test: record N entities, checkpoint, evict some via trim_cold_entries, checkpoint again, restore into a fresh ledger, assert Some(meta) and the surviving entry_count (not None).

> _Verifier (90): Confirmed all four links: trim_cold_entries (trimmer.rs:97) only removes from the in-memory DashMap, checkpoint() (mod.rs:98-100) emits only put() ops for live entries plus the meta key with no delete() for evicted rows, both write_batch impls apply only the batch's explicit ops (memory.rs:97-111, fjall.rs:155+), so orphaned Tag::Sig rows persist; restore() then scans EVERY non-meta Tag::Sig value including orphans into raw_values (mod.rs:174-183) while the hash covered only live entries, so verify_checkpoint_payload mismatches and returns Ok(None), which open.rs:173 treats as "no checkpoint" with no warning logged and an empty DashMap, and compact_wal_online (state_rebuild.rs:524) has already deleted the pre-checkpoint WAL segments (first_seq < seq, compaction.rs:122-134) so replay cannot rebuild them — a real silent total loss of signal aggregate state on the first restart after the ledger has ever exceeded the 5M trim threshold._

---

### 3. [CRITICAL] Full community-ledger checkpoint on every accepted contribution is O(total contributions) per write
**Slice:** `db-entity-ops`  **Dimension:** CLEAN  **Location:** `tidal/src/db/communities.rs:309-319`  **Confidence:** 90

**Issue.** community_contribute calls self.community_ledger.checkpoint(storage.items_engine()) synchronously for every accepted Ok(true). checkpoint() (community_ledger.rs:407) scan_prefix's the ENTIRE Tag::Community keyspace to stage a delete of every existing row, then re-serializes EVERY in-memory contributor cell across ALL communities into one WriteBatch, then commits. So a single signal_for_community write does work proportional to the total number of community contributions in the database, not to the one cell it touched.

**Why it matters.** Community contribution is a write path (the doc even calls the gate sub-microsecond). As community contribution volume grows, each new contribution rewrites the whole durable snapshot — quadratic ingestion cost. At 10^5 contributions a single new like restages 10^5 rows under WAL/fsync. This violates the hot-path allocation/efficiency discipline in CODING_GUIDELINES and will fall off the signal-write latency budget under any real community load.

**Fix.**

Make the per-accept durability incremental rather than a full snapshot swap. Persist only the one affected contributor cell on the write path (a single put of encode_contributor_suffix(community,user,epoch,entity,type_id,writer) -> serialize_entry), and keep the full checkpoint() for shutdown/periodic compaction (where the all-or-nothing prefix-swap belongs). E.g. add a community_ledger.persist_cell(storage, community, entity, type_id, user, writer, epoch, entry) that does one engine.put, and call THAT here instead of checkpoint(). The zero-loss-window guarantee is preserved because the single accepted cell is durably written before Ok(true); the periodic full checkpoint then garbage-collects purged rows.

> _Verifier (90): Confirmed in production (non-test) code: communities.rs:309-319 calls community_ledger.checkpoint() synchronously on every accepted contribution, and checkpoint() (community_ledger.rs:407-454) scan_prefix's the entire Tag::Community keyspace and re-serializes every contributor across the global `cells: DashMap<(CommunityId,EntityId,SignalTypeId),CommunityCell>` into one WriteBatch — so each write is O(total community contributions), making ingestion quadratic and violating the hot-path/allocation discipline; the synchronous-durability rationale is documented but justifies durability, not the full-snapshot rewrite (an incremental single-row put would satisfy the same Ok(true)="durably logged" promise), and the finding's only misstep is attributing the "sub-microsecond" rustdoc phrase, which actually describes the gate, not the contribution path._

---

### 4. [CRITICAL] Session-journal export reads the entire append-only journal into memory, defeating the export limit (OOM)
**Slice:** `db-export-backup-http`  **Dimension:** Accuracy  **Location:** `tidal/src/db/export.rs:280, 433-523`  **Confidence:** 88

**Issue.** `export_signals` carefully bounds the batch-WAL path with `scan_batch_wal_bounded` (a `limit`-capped heap) precisely to avoid OOM on a large pre-compaction WAL. But the session-journal path, `read_session_journal_signals`, slurps the whole file with `std::fs::read(&journal_path)`, decodes every event, and collects ALL matching events into `results` with NO limit applied — the `effective_limit` truncation only happens after the merge at export.rs:287. The session journal (src/wal/session_journal.rs:66) is a single `create(true).append(true)` file with no rotation, compaction, or size bound, so it grows without limit for the life of the database.

**Why it matters.** A `limit: Some(10)` export against a database with a multi-gigabyte session journal allocates the entire journal contents plus a fully-materialized `Vec<ExportedSignal>` of every user-attributed signal ever written, then throws all but 10 away. This is the exact OOM class the batch-WAL bounded scan was built to prevent — the fix was applied asymmetrically and the larger, never-compacted file is the one left unbounded. Export is documented as 'must never abort a batch' and runs concurrently with the live writer; an OOM here can take down the whole embedding process at 3am.

**Fix.**

Push the limit and filters down into the session-journal scan the same way the batch path does. Stream the file (or at minimum, after building the `session_user` map, retain only the `limit` earliest-timestamp matches via the same bounded `BinaryHeap<HeapEntry>` used in `scan_batch_wal_bounded`) so the working set is capped at `limit` regardless of journal size. Factor the heap-bounded retention into a shared helper that both `scan_batch_wal_bounded` and the session path call, taking a closure that yields `(timestamp, ExportedSignal)`. If true streaming of the session journal isn't yet exposed, at least cap retention: `if heap.len() > limit { heap.pop(); }` after each push, mirroring lines 405-415.

> _Verifier (88): Confirmed in code: export.rs:447 `read_session_journal_signals` does `std::fs::read(&journal_path)` and collects every matching Signal into an unbounded `results` Vec (lines 470-520) with the `effective_limit` truncation applied only after the merge at line 287, while the batch-WAL path uses the deliberately limit-capped `scan_batch_wal_bounded` (rustdoc at 298-318 explicitly documents the anti-OOM intent); session_journal.rs:66 is a single `create(true).append(true)` file with no rotation/compaction/truncation anywhere in the tree (Close events are merely appended, and unlike the batch WAL's `compact_wal_online`, the journal grows for the life of the DB), so a small-limit export against a large accumulated journal allocates the whole file plus a fully-materialized signal Vec — a real, reachable OOM on a public, concurrency-safe export path, gated on multi-GB journal accumulation rather than instant certain crash, which keeps it CRITICAL rather than BLOCKER._

---

### 5. [CRITICAL] evict_old_days has zero callers — notification maps grow unbounded forever
**Slice:** `db-signals-sessions`  **Dimension:** Completeness  **Location:** `tidal/src/db/notification_tracker.rs:171`  **Confidence:** 90

**Issue.** The type doc (lines 19-24) says "call evict_old_days periodically (e.g., on startup or daily) to remove stale entries. Without eviction the maps grow by O(users × creators) per day of operation." A grep across the whole crate (excluding this file's own tests) finds no caller of evict_old_days — not in open.rs, not in the sweeper, not on startup, not on a timer. The DashMaps `delivered` and `user_total` therefore accumulate a new key per (user, creator, day) and (user, day) for every day the process runs and are never trimmed.

**Why it matters.** For a long-lived notification-serving process this is a steady, unbounded memory leak proportional to active-users × creators × days-of-uptime. It exactly matches the project rule 'every entity has a lifecycle' and 'design for recovery' — here the lifecycle's cleanup transition is defined but never invoked, so a server that stays up for months OOMs. The mitigation exists and is tested; it is simply not wired.

**Fix.**

Wire eviction on a cadence the existing sweeper already provides. The DB has a background sweeper (src/db/sweeper.rs); add a daily tick that calls self.notification_tracker.evict_old_days(today, keep_days) with keep_days from config (e.g. 1–7), and also call it once during open() so a restart trims stale days. Concretely, in the sweeper loop compute `let today = (Timestamp::now().as_nanos() / 86_400_000_000_000) as u32;` and invoke `self.notification_tracker.evict_old_days(today, self.config.notification_keep_days);`. Without a caller the method and its doc are a false promise.

> _Verifier (90): Verified: grep across the whole workspace shows evict_old_days (notification_tracker.rs:171) has zero non-test callers, while check_and_record at helpers.rs:276 inserts a new (user,creator,day)/(user,day) key on every notification-profile RETRIEVE, and the only background thread (sweeper.rs:100) calls sweep_expired_sessions and never the tracker — so the DashMaps grow unbounded per day exactly as the type's own doc (lines 19-23) warns, a real but slow long-uptime leak warranting CRITICAL not BLOCKER._

---

### 6. [CRITICAL] Empty writer_agent in RemoveScope::CommunityAgent purges ALL direct user contributions
**Slice:** `governance`  **Dimension:** Accuracy  **Location:** `tidal/src/governance/community_ledger.rs:315-317 (purge_writer); reached via /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/remove_scope.rs:60-61`  **Confidence:** 88

**Issue.** purge_writer matches contributors whose writer tag equals writer_agent. A direct (non-agent) user write is stored with writer == "" (WriterTag empty string / WRITER_DIRECT). RemoveScope::CommunityAgent { writer_agent, community } is a public API (RemoveScope is re-exported from lib.rs:61, remove_from_personalization is pub) and passes writer_agent straight into purge_writer with no non-empty validation. An embedder (or a bug upstream) that calls remove_from_personalization with an empty agent id silently removes every direct user contribution to that community and stamps a purge watermark, then writes a durable AgentPurgeTombstone("") that re-purges them on every subsequent open.

**Why it matters.** This is a silent, durable, replay-persistent data-loss path for the most common contribution class (direct user writes). It is exactly the 3am footgun the durability machinery exists to prevent: 'revoke agent X' with X unset deletes real user data and the tombstone makes it permanent across restart.

**Fix.**

Reject an empty writer_agent before mutating. In community_ledger.rs make purge_writer guard the sentinel: `pub fn purge_writer(&self, community: CommunityId, writer_agent: &str) -> usize { if writer_agent.is_empty() { return 0; } self.purge_matching(community, |(_u, writer, _ep)| writer == writer_agent) }` — and, defensively, validate at the API boundary in remove_scope.rs CommunityAgent arm: `if writer_agent.is_empty() { return Err(TidalError::invalid_input("writer_agent must be non-empty; empty matches direct user writes")); }` before persisting the tombstone. Add a test asserting purge_writer(c, "") removes 0 and leaves direct writes intact.

> _Verifier (88): Confirmed end-to-end: direct user writes store writer_agent="" (communities.rs:207-215, the sole direct-write path; agent writes use a real agent_id at capabilities.rs:173), purge_writer matches writer==writer_agent (community_ledger.rs:316), and the public, freely-constructible RemoveScope::CommunityAgent{writer_agent:String} (lib.rs:61) flows straight into purge_writer with no is_empty guard (remove_scope.rs:60-61) plus a durable AgentPurgeTombstone("") that re-purges all direct contributions on every open (community_ledger.rs:525 via db/mod.rs:609), so an empty agent id silently and permanently deletes all direct user contributions — real and durable, but a public-API misuse footgun (requires passing "") rather than a happy-path corruption, so CRITICAL not BLOCKER._

---

### 7. [CRITICAL] Synchronous full-snapshot community checkpoint on every accepted contribution is O(N^2) ingest
**Slice:** `governance`  **Dimension:** CLEAN  **Location:** `tidal/src/governance/community_ledger.rs:407-455 (checkpoint), driven per-write from /Users/jordan.washburn/Workspace/orchard9/tidaldb/tidal/src/db/communities.rs:309-318`  **Confidence:** 90

**Issue.** checkpoint() scans the entire Tag::Community prefix, stages a delete for every existing durable row, then stages a put for every in-memory contributor cell, and commits one WriteBatch. signal_for_community calls this synchronously on the write path for EVERY accepted community contribution (communities.rs:310). For a community that has accumulated M contributor cells, the M-th write does O(M) deletes + O(M) puts + an O(M) durable scan, so ingesting N contributions is O(N^2) total work and O(N) write amplification per signal. The CODING_GUIDELINES signal-write budget is <100us; a community with tens of thousands of cells blows it by orders of magnitude.

**Why it matters.** Community write throughput collapses quadratically as a community grows — the larger and more active a community, the slower each new contribution, which is the opposite of the desired scaling and a latent production cliff. It also rewrites the whole durable snapshot on every write, multiplying I/O and write-amplification.

**Fix.**

Make per-contribution durability incremental instead of a full rewrite: on an accepted contribution put only that single (community,user,epoch,entity,signal_type,writer) row via encode_contributor_suffix, and on a purge delete only the affected rows. Keep the full scan-delete-rewrite snapshot for the lifecycle/periodic/backup checkpoint path (backup.rs, state_rebuild.rs) where an all-or-nothing swap is wanted. Alternatively, debounce: keep an in-memory dirty-set and flush a batched delta on a periodic tick, accepting a bounded loss window like the cohort ledger rather than a synchronous full snapshot per write. Benchmark community ingest before/after per CODING_GUIDELINES §8.

> _Verifier (90): Confirmed in source: communities.rs:309-318 synchronously calls community_ledger.checkpoint() on every accepted contribution (production path, not test), and checkpoint() (community_ledger.rs:418-453) scans the full Tag::Community prefix staging a delete per durable row then a put per in-memory contributor cell across all communities into one WriteBatch — so the M-th write is O(M) and N writes are O(N^2), blowing the CODING_GUIDELINES.md:246 "<100 microseconds" signal-write budget, and the rustdoc only justifies atomicity, never the quadratic cost._

---

### 8. [CRITICAL] Unchecked `id as u32` truncation in trending scope bitmap aliases high entity IDs in release builds
**Slice:** `query-retrieve-search`  **Dimension:** Accuracy  **Location:** `tidal/src/query/search/scope.rs:322-330`  **Confidence:** 88

**Issue.** percentile_bitmap inserts entity IDs into a RoaringBitmap via `bitmap.insert(id as u32)`, guarded only by `debug_assert!(u32::try_from(id).is_ok(), ...)`. debug_assert is compiled out in release. An entity id above u32::MAX (4.29e9) is silently truncated, so a high id wraps onto a low id's slot in the scope bitmap.

**Why it matters.** The scope bitmap is the Stage 0 pre-filter applied to BOTH BM25 and ANN retrieval (WithinScope::Trending / CohortTrending). A truncated/aliased id makes the trending scope admit an item that is NOT trending (whatever low id the high id aliased onto) and silently drop the real high-id item — a wrong-results leak with no diagnostic. Every other id->u32 conversion in this exact slice (entity_as_u32, retain_excluded, retrieve_ann's filtered_search predicate) uses checked `u32::try_from`; this is the one release-relevant exception, so the codebase already has the right pattern to copy.

**Fix.**

Drop the debug_assert and skip non-representable ids explicitly, matching entity_as_u32 elsewhere in the slice:
```rust
for &(id, vel) in &sorted {
    if vel >= cutoff_velocity && vel > 0 {
        match u32::try_from(id) {
            Ok(k) => { bitmap.insert(k); }
            Err(_) => {
                tracing::warn!(entity_id = id, "trending scope: id exceeds u32 bitmap key space; skipped");
            }
        }
    }
}
```
Skipping (rather than truncating) is the correct polarity for an inclusion bitmap: a >u32 id cannot be a member, so excluding it is sound, and it can never alias onto another item.

> _Verifier (88): In scope.rs:329 the producer `percentile_bitmap` inserts `id as u32` (truncating, debug_assert-only) into the trending scope bitmap, while both consumers — BM25 retain (pipeline.rs:132) and the ANN filtered_search predicate (pipeline.rs:396) — use checked `u32::try_from` via `entity_as_u32`, whose own doc comment warns that `id as u32` "would alias a high id onto a low one"; since `EntityId` wraps an arbitrary caller-supplied u64 with no u32 cap, an entity ID above u32::MAX is silently aliased onto a low slot in release, admitting a non-trending item and dropping the real one with no diagnostic._

---

### 9. [CRITICAL] Hlc::update() does not carry logical-counter overflow, so it can return a non-unique timestamp
**Slice:** `replication-crdt`  **Dimension:** Accuracy  **Location:** `tidal/src/replication/crdt/hlc.rs:288-301`  **Confidence:** 78

**Issue.** When the wall clock has not advanced (`pt == cur_wall`) and the logical counter is already saturated (`cur_logical == LOGICAL_MAX`), `new_logical = (LOGICAL_MAX + 1).min(LOGICAL_MAX) = LOGICAL_MAX`, so `new_packed == cur`. The CAS succeeds against the unchanged word and `update()` returns a timestamp byte-identical to the current state. `now()` explicitly handles this exact regime by carrying the overflow into the wall component (lines 226-231, with a warn!), but `update()` silently saturates. Two concurrent `update()` calls in this regime, or an `update()` followed by an equal-keyed event, can produce duplicate timestamps from the same node.

**Why it matters.** The module's central guarantee (doc lines 150-154, 271-272) is that every timestamp a node emits is strictly monotonic and unique within the node, because LWW conflict resolution depends on it. `update()` claims in its own docstring to return a value 'strictly > remote' and to make subsequent reads strictly greater, but in the saturated-counter regime it returns a value equal to the prior state. This is the same practically-unreachable (65536 events/ms) regime the authors deemed worth fixing in `now()` — leaving `update()` asymmetric is a latent uniqueness hole that contradicts both the module doc and `now()`'s own carefully-built invariant.

**Fix.**

Mirror the `now()` carry in `update()`: after computing `pt` and the candidate `new_logical`, detect saturation when the wall did not advance and carry into the wall component instead of pinning logical. e.g. replace the saturating `.min(LOGICAL_MAX)` cascade with a check: `if pt == cur_wall && new_logical_raw > LOGICAL_MAX { new_wall = cur_wall.saturating_add(1); new_logical = 0; emit the same warn! }` — reusing the exact overflow-handling branch `now()` already has, so the two methods share one rule and one tracing signal.

> _Verifier (78): Confirmed at hlc.rs:294/303-314: when pt==cur_wall and cur_logical==LOGICAL_MAX, `((cur_logical+1).min(LOGICAL_MAX))` returns LOGICAL_MAX unchanged, so new_packed==cur, the CAS(cur,cur) succeeds, and update() returns a byte-identical timestamp — whereas now() (lines 226-231) carries the same overflow into the wall component with a warn! and has a guarding test (now_strictly_advances_when_logical_saturated) that update() lacks, so update() silently violates the module's per-node uniqueness invariant (lines 152-154) and its own docstring promise (lines 268-272); the regime is astronomically rare (65,536 events/ms) but is the exact case the authors deemed worth fixing in now(), making the asymmetry a genuine latent uniqueness hole rather than test code, an unreachable path, or a documented design choice._

---

### 10. [CRITICAL] from_node_contribution score/last_update_ns contract is ambiguous and the production caller double-decays
**Slice:** `replication-crdt`  **Dimension:** Accuracy  **Location:** `tidal/src/replication/crdt/signal_state.rs:98-112`  **Confidence:** 90

**Issue.** `from_node_contribution(node, score, last_update_ns, ...)` stores `score` against `last_update_ns` and `decay_score(query_ns)` later applies `exp(-lambda*(query_ns - last_update_ns))` to it. The docstring describes `score` only as 'the current node's running decay score', without stating whether it must be the *stored accumulator at last_update_ns* (so re-decay is correct) or a value already decayed to some query time (so re-decay is wrong). The production caller `SignalLedger::crdt_contributions` (signals/ledger/core.rs:550-553) passes `score = hot.current_score(0, now_ns, lambda)` — the score decayed forward to `now_ns` — together with `last_update_ns = hot.last_update_ns()`, the original (earlier) event time. `decay_score(query_ns)` then decays that already-decayed score again across `[last_update_ns, query_ns]`, re-applying the `[last_update_ns, now_ns]` interval a second time.

**Why it matters.** Post-partition reconciliation is the whole reason this slice exists, and the merged signal score directly drives ranking. A systematic double-decay makes every reconciled contribution monotonically too small, so a viral item's merged engagement score is under-counted after a partition heals — silently degrading exactly the rankings the database is built to produce, with no error surfaced. The unit tests pass only because every test calls `from_node_contribution` with `last_update_ns == query_ns` (e.g. reconcile_tests.rs:546, signal_state.rs:321-327), which masks the production mismatch.

**Fix.**

Make the contract explicit and internally consistent. Document that `score` MUST be the stored accumulator value as of `last_update_ns` (NOT decayed to any later time), then fix the caller to pass `hot.stored_score(0)` (which exists, hot.rs:225) with the matching `last_update_ns`, letting `decay_score` apply the one and only decay. Alternatively, if the caller intends to snapshot at `now_ns`, pass `last_update_ns = now_ns` so the stored decayed value is consistent with its timestamp. Add a regression test where `now_ns > last_update_ns` and assert the reconciled `decay_score` equals the single-decay analytical value, so the round-trip can no longer silently double-decay.

> _Verifier (90): Confirmed in production code: crdt_contributions (core.rs:550-551) builds the contribution with score = current_score(0, now_ns, lambda) = stored*exp(-lambda*(now_ns-last_ns)) but pairs it with last_update_ns = the earlier hot.last_update_ns(), and the live reconcile-apply path apply_crdt_state (core.rs:474) calls state.decay_score(now_ns) which re-applies exp(-lambda*(now_ns-last_ns)) before force_set_score, so every reconciled contribution is silently decayed twice; the masking is also real (the m8_uat CRDT sub-test builds states via on_signal(...,now_ns) so last_update_ns==query_ns and the cluster check uses epsilon=1.0, while the m8p2 replication test exercises the WAL path, not this CRDT snapshot path)._

---

### 11. [CRITICAL] Shutdown-drain multi-chunk flush drops remaining callers' replies on a write error
**Slice:** `wal-writer-reader`  **Dimension:** Accuracy  **Location:** `tidal/src/wal/writer.rs:551-558`  **Confidence:** 88

**Issue.** In the shutdown drain, a drained batch larger than max_batch is flushed in chunks with `next_seq = flush_batch(...)?`. flush_batch notifies only the CURRENT chunk's callers before returning Err. The `?` then propagates immediately, so the `events`/`replies` iterators for every NOT-YET-PROCESSED chunk are dropped without sending anything. Those callers' reply channels close and they observe `WalError::SendFailed`/`Closed` instead of the real write error.

**Why it matters.** The module-level doc on QueuedAppend states it is 'critical that every queued append eventually resolves its reply' and that 'both the steady-state loop and the shutdown drain funnel through the same flush_batch routine' precisely to avoid dropping channels on error. This multi-chunk path violates that invariant. A caller blocked in WalSender::append on shutdown under a real I/O fault gets a misleading Closed, hiding the actual ENOSPC/EIO cause — exactly the 3am-incident confusion the consolidation was built to prevent. It only triggers when >256 appends queue during shutdown AND a write fault occurs, hence CRITICAL not BLOCKER.

**Fix.**

Do not early-return on chunk failure during the drain; notify the remaining chunks' callers before propagating. e.g. collect the error and drain the rest:
```rust
let mut drain_err = None;
loop {
    let chunk_events: Vec<EventRecord> = events.by_ref().take(max_batch).collect();
    if chunk_events.is_empty() { break; }
    let chunk_replies: Vec<_> = replies.by_ref().take(chunk_events.len()).collect();
    if drain_err.is_some() {
        // a prior chunk already failed; still notify these callers with an error
        for r in chunk_replies { let _ = r.send(Err(WalError::Closed)); }
        continue;
    }
    match flush_batch(&mut segment, config, next_seq, &chunk_events, chunk_replies) {
        Ok(seq) => next_seq = seq,
        Err(e) => drain_err = Some(e),
    }
}
if let Some(e) = drain_err { return Err(e); }
```
This keeps every queued append resolved (each chunk's callers were notified by flush_batch or the fallback) while still surfacing the error from run_writer.

> _Verifier (88): Confirmed at writer.rs:557 — `next_seq = flush_batch(...)?` propagates immediately while flush_batch (lines 244-254) notifies only the current chunk's replies, so the `replies` iterator still holding senders for not-yet-flushed chunks is dropped on `?`, surfacing as `Closed` instead of the real I/O error; this directly contradicts the module doc (lines 22-25) requiring every queued append to resolve its reply and diverges from the steady-state loop (lines 495-513) which logs-and-continues rather than propagating, but it requires the compound trigger of >max_batch (256) queued appends during shutdown plus a write fault on a non-final chunk and yields a misleading-error (not data-loss/hang) symptom, so CRITICAL is the defensible ceiling._

---

## WARNINGS

| # | Slice | Location | Dim | Issue |
|---|---|---|---|---|
| 1 | db-core-lifecycle | `tidal/src/db/builder.rs:171-183, 280-290; open.rs:153-156` | Completeness | wal_dir / cache_dir builder overrides are validated then silently ignored |
| 2 | db-core-lifecycle | `tidal/src/db/paths.rs:16-23, 60-105` | CLEAN | Paths::items_dir/users_dir/creators_dir/ensure_all describe a layout the engine never creates |
| 3 | db-core-lifecycle | `tidal/src/db/builder.rs:298-299` | Accuracy | open() doc claims storage 'will create them via Paths::ensure_all during initialization' — it never does |
| 4 | db-core-lifecycle | `tidal/src/db/lifecycle.rs:218-223` | Accuracy | Poisoned WAL mutex in shutdown_inner discards an already-captured first_err |
| 5 | db-entity-ops | `tidal/src/db/collections.rs:59, 95` | Accuracy | Collection item IDs above u32::MAX silently alias instead of being rejected |
| 6 | db-entity-ops | `tidal/src/db/collections.rs:60-74` | Accuracy | add_to_collection mutates in-memory index before persisting; persist failure leaves memory ahead of disk |
| 7 | db-entity-ops | `tidal/src/db/communities.rs:108-125` | Accuracy | leave_community engages the live stop-forward gate before persisting; persist failure leaves an un-durable gate |
| 8 | db-signals-sessions | `tidal/src/db/notification_tracker.rs:63` | DRY | Dead public surface: record / would_exceed_per_creator / would_exceed_total are only called from tests |
| 9 | db-signals-sessions | `tidal/src/db/signals.rs:297` | Completeness | signal_with_context '# Durability' docstring omits two side-effect indexes it mutates |
| 10 | db-signals-sessions | `tidal/src/db/signals.rs:93` | DRY | Backup-guard + WAL backpressure preamble duplicated verbatim between signal and signal_scoped |
| 11 | db-recovery-rebuild | `tidal/src/db/sweeper.rs:118` | Accuracy | start_sweeper panics on thread-spawn failure instead of degrading |
| 12 | db-export-backup-http | `tidal/src/db/export.rs:11-14, 30, 127-143, 185` | Completeness | `ExportFormat::JsonLines` and `to_json_line` are dead-ended — `export_signals` returns structs, never the wire format |
| 13 | db-metrics | `tidal/src/db/metrics/mod.rs:292-298, 470` | Extensibility | partition_id="0" hardcoded into every metric line — wrong in cluster mode |
| 14 | wal-writer-reader | `tidal/src/wal/checkpoint.rs:28-49` | Accuracy | Checkpoint temp file leaks on crash between write and rename; no recovery cleanup |
| 15 | wal-writer-reader | `tidal/src/wal/reader.rs:181-186` | Tech Debt | Reader hardcodes the payload_len byte offset instead of using the format module's owned layout |
| 16 | wal-format-recovery | `tidal/src/wal/format/session.rs:324-328` | Accuracy | v2 frame with a known type but short body is misreported as on-disk corruption |
| 17 | signals-decay-hotwarm | `tidal/src/signals/warm.rs:206, 525, 454-459` | DRY | 'last 24h = in-progress hour + 23 hour buckets' is expressed three times |
| 18 | signals-decay-hotwarm | `tidal/src/signals/warm.rs:388-412` | Accuracy | Concurrent rotation at an hour boundary can lose minute data from the hour rollup |
| 19 | signals-ledger-checkpoint | `tidal/src/signals/checkpoint/meta.rs:43-51, 64-124` | Accuracy | Integrity hash covers entry payloads but NOT the meta record (wal_sequence / checkpoint_time_ns) |
| 20 | signals-ledger-checkpoint | `tidal/src/signals/ledger/core.rs:207-260` | CLEAN | read_decay_score / read_windowed_count read Timestamp::now() internally — no consistent query clock, repeated syscalls per candidate |
| 21 | storage-core | `tidal/src/storage/engine.rs:38-45` | Completeness | Trait contract for write_batch promises atomicity but is silent on durability |
| 22 | storage-indexes | `tidal/src/storage/indexes/range.rs:66-81` | Completeness | RangeIndex has no delete_entity; re-indexed items leave stale range entries |
| 23 | storage-vector | `tidal/src/storage/vector/lifecycle/ops.rs:138-165` | Accuracy | update_embedding leaves entity store and ANN index divergent on partial failure with no recovery |
| 24 | storage-vector | `tidal/src/storage/vector/lifecycle/ops.rs:188-218` | Accuracy | Soft delete: ANN tombstone applied before durable archive tombstone is written (crash window resurrects the vector) |
| 25 | storage-vector | `tidal/src/storage/vector/registry.rs:300-352` | CLEAN | rebuild_from_store does two full O(N) store scans with a per-key String allocation on every embedding key |
| 26 | query-executor | `tidal/src/query/executor/helpers.rs:166-198` | Accuracy | Cohort rescore swallows the unknown-signal schema error the main path raises loudly |
| 27 | query-executor | `tidal/src/query/executor/user_filter.rs:99-120, 137-158, 242-256, 272-286` | DRY | Four near-identical filter-collector walkers duplicate a load-bearing traversal rule |
| 28 | query-retrieve-search | `tidal/src/query/executor/pipeline.rs:250-276` | Completeness | RETRIEVE Stage 2 did not receive the empty/poisoned-universe deferred-filter fix that SEARCH got |
| 29 | query-retrieve-search | `tidal/src/query/search/executor_helpers.rs:104-121` | Accuracy | Creator metadata filter matches a value against ANY metadata field, not the intended field |
| 30 | ranking | `tidal/src/ranking/executor/mod.rs:617-619` | CLEAN | Alphabetical tie-break allocates two Strings per equal-score comparison |
| 31 | ranking | `tidal/src/ranking/executor/mod.rs:393-408` | CLEAN | session_boost re-lowercases all session keywords for every candidate |
| 32 | entities | `tidal/src/entities/mod.rs:67-69` | CLEAN | Hot-path callers clone whole RoaringBitmap via get() when get_ref() exists to avoid it |
| 33 | entities | `tidal/src/entities/co_engagement.rs:84-90` | Accuracy | Co-engagement edge double-counted when an item recurs in the recent queue |
| 34 | entities | `tidal/src/entities/collection.rs:136-154` | Tech Debt | CollectionIndex tracks membership in both a bitmap and a Vec with O(n) per-mutation scans |
| 35 | governance | `tidal/src/governance/community_ledger.rs:235-255 (windowed_count)` | Accuracy | windowed_count reads its own wall clock while every sibling read takes now_ns |
| 36 | governance | `tidal/src/governance/capability.rs:366-373` | Completeness | Unticketed TODO(rehydrate) violates the no-orphan-TODO rule |
| 37 | replication-core | `tidal/src/replication/session_bridge.rs:313-360` | Accuracy | recv_and_apply: Layer-1 seqno HWM advances even when Layer-2 idempotency subsequently drops the event |
| 38 | replication-core | `tidal/src/replication/migration.rs:119-137` | Completeness | enter_dual_write mutates router routing before persisting; a crash in the gap loses an in-flight migration's dual-write state |
| 39 | schema | `tidal/src/schema/validation/builders.rs:255-285` | Completeness | AgentPolicy numeric fields are never validated at build(); Duration::ZERO silently expires every session |
| 40 | session | `tidal/src/session/saved_search.rs:107, 120, 141` | Accuracy | saved_search silently masks corruption with from_utf8_lossy while the rest of the slice rejects it |
| 41 | text | `tidal/src/db/text_syncer.rs:186` | Completeness | Unbounded text-syncer channel grows without bound during a sustained commit outage |
| 42 | cohort-load-testing | `tidal/src/cohort/ledger.rs:128-148` | Accuracy | Cohort read_decay_score returns an undecayed score for an out-of-range decay index instead of None |
| 43 | cohort-load-testing | `tidal/src/db/builder.rs:114-117` | Completeness | Builder accepts a rate-limiter config but never validates it; a degenerate config silently degrades to unlimited |
| 44 | cohort-load-testing | `tidal/src/testing/cluster.rs:513-524` | Accuracy | await_convergence calls blocking redeliver_missed while holding the batch_log lock |
| 45 | tidal-net | `tidal-net/src/client.rs:34-66` | Accuracy | Client builds a plaintext channel when insecure=false and tls=None, instead of erroring |
| 46 | tidal-net | `tidal-net/src/config.rs:70-90` | Completeness | No validation of GrpcTransportConfig invariants (zero capacity/timeouts, payload ceiling) |
| 47 | tidal-server-cluster | `tidal-server/src/scatter_gather.rs:640-708 vs 768-834` | DRY | scatter_gather_retrieve and scatter_gather_search duplicate the entire merge+assemble tail |
| 48 | tidal-server-core | `tidal-server/src/main.rs:264-280 (serve)` | Completeness | Standalone shutdown never observes final-flush durability failure |
| 49 | tidalctl | `tidalctl/src/commands/diagnostics.rs:146-167, 206-214` | DRY | Offline-unavailable field set maintained in two unsynchronized places |
| 50 | tidalctl | `tidalctl/src/commands/paths.rs:34-48` | DRY | Six-directory path projection duplicated between paths.rs and status.rs |


## SUGGESTIONS (by slice)

- **db-core-lifecycle** (2): Six near-identical Tag-prefix restore-scan loops in from_parts; fs4 try_lock_exclusive error is mapped unconditionally to DataDirLocked
- **db-entity-ops** (2): flush_text_index discards channel send/recv results; u32 item-id narrowing policy is implemented three different ways
- **db-signals-sessions** (3): item_embedding_slot and creator_embedding_slot are the same resolver parameterized by EntityKind; next_session_id advance uses unguarded session_id + 1; duration_ms derived from Instant::elapsed() on a restored session is wall-clock-since-restore, not session age
- **db-recovery-rebuild** (4): Four duplicated items/creators scan loops; items keyspace scanned twice at open; Fingerprint lacks length-prefix/domain-separation; collision-safe only by coincidence; run_checkpoint_thread mixes timing, trimming, checkpointing, compaction, and metrics; Recovery/checkpoint cadence constants are scattered inline literals
- **db-export-backup-http** (4): No validation that `since <= until` in `ExportRequest`; since-inclusive/until-exclusive window filter duplicated across both scan paths; Backup WAL-marker timestamp uses raw SystemTime while the rest of the method uses `Timestamp::now()`; `extract_path` allocates a Vec just to read the second whitespace token
- **db-metrics** (3): 'now in nanos' exists twice with divergent overflow semantics, contradicting the doc claim; Undocumented u128->u64 truncation of latency on the hot path; LatencyHistogram::new does not validate that bounds are sorted ascending
- **wal-writer-reader** (2): Active-segment selection can leave segment filename first_seq ahead of next_seq after a checkpoint-heavy reopen; handle_session_command has an unreachable `_ => return` arm that hides intent
- **wal-format-recovery** (3): Optional-field `[flag][payload]` framing is hand-rolled three times; Magic byte-offset literal `4 + 3` in corruption test instead of the named frame constants; v3 event reserved bytes [30..32] are written-zero but never asserted-zero on decode
- **signals-decay-hotwarm** (1): Decay factor in on_signal uses a single last_ns snapshot for all CAS retries
- **signals-ledger-checkpoint** (2): restore() returns CheckpointMeta whose wal_sequence is silently discarded by the sole production caller; V1 and V2 deserialize arms duplicate the checkpoint_time_ns/wal_sequence read+offset-error boilerplate
- **storage-core** (3): Tag byte mapping is a hand-maintained parallel table with no drift guard; Repeated keyspace-open block in FjallStorage::open; FjallAtomicBatch publicly re-exported but has no trait abstraction and no production caller
- **storage-indexes** (3): delete does a redundant second tree/map lookup after get_mut; Hard-coded 10-byte key-prefix skip duplicated across bitmap and range parsers; evaluate_not complement omits items absent from the universe bitmap (by design, but undocumented edge)
- **storage-vector** (3): Unticketed TODO for per-query ef_search override; Duplicated ef_search mismatch-warning block across search and filtered_search; read_*_le helpers documented as panic-on-OOB but are pub(super) used across modules without a debug_assert guard
- **query-executor** (2): Notification-cap creator resolution re-reads storage despite pre-loaded metadata; ANN candidate strategy silently falls back to scan with no tracking issue
- **query-retrieve-search** (2): User-context suppression block is duplicated between SEARCH and RETRIEVE pipelines; resolve_trending scans the entire global signal ledger per query with no candidate bound
- **ranking** (3): resolve_signal_type Result consumed via map_or silently swallows schema errors; score_personalized and score_inner duplicate the candidate-loop skeleton; Rising / MostFollowed / CreatorEngagementRate sort modes have no built-in profile
- **entities** (3): Collection item-ID truncation to u32 is silent, unlike the audited narrow_item_slot path; Field-by-field little-endian decode ritual repeated across four entity codecs; top_candidates does a full O(all-edges) scan per call
- **governance** (2): contributor_count scans every cell of every community on each call; .expect on infallible 16-byte BLAKE3 truncation in non-test code
- **replication-core** (3): IdempotencyStore::new panics on capacity 0 instead of returning a Result; Three near-identical batch-walk loops (filter_segment_drop_local, last_seq_in_segment, count_events_in_segment) share the same decode/advance skeleton; lag_for silently returns the single gauge for any shard in release builds (debug_assert only)
- **replication-crdt** (3): LWWRegister::write reimplements the LWW decision instead of routing through lww_other_wins; Undocumented cross-map invariant: node_decay_scores and node_last_update_ns must stay key-paired; node_buckets nests a full PNCounter per node to hold a single key
- **schema** (2): Empty embedding-slot name reported as DuplicateEmbeddingSlot is a misleading error; Item and creator text-field validation are two hand-written call sites
- **session** (3): saved_search re-implements length-prefixed LE decode instead of reusing the serde cursor helpers; build_frozen_snapshot samples wall-clock twice for one logical close instant; saved_search length fields cast usize->u32 without guarding >4GiB inputs
- **text** (3): Failed flush-path commit does not arm retry_not_before, delaying recovery to the timeout path; final_flush sleeps after the last attempt's failure before giving up; read_stats_from_dir conflates 'directory absent' with 'index unreadable'
- **cohort-load-testing** (4): parse_cohort_entry_suffix silently accepts trailing bytes after the cohort name; applied_count hardcodes ShardId(0) as 'the initial leader' and silently returns wrong counts after promotion; write_signal ship loop and redeliver_missed duplicate the WalSegmentPayload construction; total_signals stored with Relaxed but read (relay_log_len) with Acquire — mismatched ordering with no paired Release
- **tidal-net** (2): expect() on a production code path (runtime() helper); Heartbeat handler ignores the claimed shard/region identity
- **tidal-server-cluster** (3): Score sort treats NaN as equal, yielding an unstable/partial order if a score is ever NaN; Backpressure retry_after_ms=50 is a bare magic number; Routing index step bypasses the engine ShardRouter's region mapping, coupling server to hash-mod-len
- **tidal-server-core** (6): Config crate hand-rolls enum string tables that drift from the engine's canonical labels; `extract_string` is a pure pass-through to `extract_string_field`; User-supplied `limit` is unbounded at the network trust boundary; Crate doc references a non-existent 'Known Gaps' section; Shutdown-flag atomic ordering differs from ClusterState for the same concept; Two io-error variants with overlapping roles and a misleading name
- **tidalctl** (4): as_nanos() as u64 truncates wall-clock for post-2554 timestamps; recover threads a verify_only flag that has exactly one accepted value; scope-stats --pretty only indents JSON; no human-readable mode like recover/diagnostics; format_pretty is a 120-line function building the report inline


## What's Good

- **db-core-lifecycle** — Shutdown durability is genuinely 3am-grade: shutdown_inner attempts EVERY flush step (signal ledger, cohort, co-engagement, community, preference vectors, replication HWM, storage, WAL marker, compaction), captures the FIRST error, and surfaces it from close() while Drop logs it — the SHUTDOWN-2 reasoning at lines 142-262 of lifecycle.rs is exactly right and the rationale is in-code.
- **db-entity-ops** — Durability ordering is reasoned about explicitly and correctly in the strongest paths: write_relationship persists before mutating in-memory state, define_community_policy/define_cohort persist-before-register with a crash-between comment, and purge_prior_contributions writes the tombstone before mutating the aggregate (durability-before-mutation). This is exactly the 3am-incident-grade rigor the project demands.
- **db-signals-sessions** — The closed-session cap fix (sessions.rs:208-216, enforce_closed_session_cap) is exactly right: insert-then-trim makes `len() <= MAX` a post-condition instead of a racy pre-condition, the comment explains the concurrent-overshoot bug it replaces, and the low-water-mark batch eviction avoids re-sorting on every close. This is 3am-incident-grade reasoning.
- **db-recovery-rebuild** — ItemIndexes (state_rebuild.rs:234) is the standout decision: it forces the live write path and the restart-rebuild path through ONE indexing implementation, with a doc comment that names the exact prior drift (missing creator_items / defaulted created_at) it eliminates. This is correctness-by-construction, not by discipline.
- **db-export-backup-http** — The WAL u8 signal-type-ID truncation is handled with exemplary defense-in-depth: export.rs:208-226 surfaces a typed error instead of silently aliasing type #256 onto #0, and the write path (wal_bridge.rs:109) independently `u8::try_from`-guards the same boundary — the comment even documents the prior release-build aliasing bug that motivated it.
- **db-metrics** — The histogram observe() comment (lines 54-74) is exemplary: it explains WHY partition_point is used (hot path, O(log n) vs O(n)), acknowledges the inherent O(suffix) increment cost, and is backed by a proptest (observe_matches_naive_cumulative_rule) that checks the optimized path against the naive rule for every value — this is precisely the correctness-first discipline the project demands.
- **wal-writer-reader** — The durability rationale is documented at exactly the right depth: SegmentWriter::sync (segment.rs:208-235) explicitly states sync_data covers the data half only and points to the four sites (open, rotate, checkpoint, compaction) that fsync the directory entry — this is the kind of 3am-incident-grade note that prevents someone from 'simplifying' away a load-bearing dir fsync.
- **wal-format-recovery** — compact_wal_online (compaction.rs:91-103) closes a genuine 3am silent-data-loss hazard: clamping the deletion floor to min(checkpoint_seq, active_first_seq) so the writer's open FD is never unlinked. The hazard analysis in the doc comment (Linux unlinked-inode appends being lost on next open) is exactly the kind of reasoning that belongs at a durability boundary, and online_never_deletes_active_segment_even_when_fully_checkpointed is the regression test that locks it in.
- **signals-decay-hotwarm** — The forward_decay_step kernel (decay.rs) is exactly the right abstraction: a pure, allocation-free, lock-free scalar function extracted as the single source of truth, with a module doc that names the five prior drifted copies and the specific spec violation (session tier added late weight at full value) that motivated consolidation. This is textbook DRY-as-correctness.
- **signals-ledger-checkpoint** — The named-offset codec in format.rs (OFF_* derived from prior field sizes, used by BOTH serialize and deserialize) plus the compile-time const-asserts pinning ENTRY_SIZE_V1==983 and ENTRY_SIZE==1116 is exactly the right way to make a binary wire format un-desyncable — a miscount is a build break, not a silent roundtrip corruption.
- **storage-core** — map_fjall_err (fjall.rs:69-118) is exactly the kind of error routing you want at 3am: it preserves the io::Error kind/source by moving it into StorageError::Io, separates transient/unavailable (Poisoned/Locked/KeyspaceDeleted -> Closed) from genuine on-disk corruption, and the non_exhaustive wildcard conservatively halts on integrity ambiguity rather than masking a data problem as transient. The reasoning is documented in the doc comment.
- **storage-indexes** — Length-prefixed bitmap keys (bitmap.rs:53-61, encode_suffix/parse_suffix_value) are a genuinely correct fix for a real cross-index collision: field 'a:b'+value 'c' vs field 'a'+value 'b:c' both rendered identically under a naive 'BMP:{field}:{value}' scheme. The u16 length prefix removes the ambiguity, the field-name-too-long case returns a typed error instead of corrupting, and a dedicated test (key_format_disambiguates_field_value_boundary) proves cross-load isolation.
- **storage-vector** — Brute-force deserialize (brute/mod.rs:296-352) is exemplary hostile-input hardening: count is bounded against the actual buffer length BEFORE any allocation, all arithmetic is checked, the SECURITY comment explains the exact attack (wrap -> tiny expected_size -> exabyte with_capacity -> OOB read), and corruption surfaces as a recoverable CorruptedIndex rather than crashing the recovery path. This is precisely the 3am-incident discipline the project demands.
- **query-executor** — The separate-arms treatment of `for_creator` in Stage 1 (pipeline.rs:99-120) is exactly the 3am-correctness mindset: a single `if let && let` chain would have silently fallen through to a full-universe scan and returned every creator's items when the index was absent; the code instead warns and returns empty, with a comment explaining precisely why the obvious refactor is wrong.
- **query-retrieve-search** — Cursor encoding is genuinely 3am-proof: a discriminant byte distinguishes keyset vs offset so the two shapes can never be silently misread as each other, decode rejects non-finite keyset scores and unknown kind bytes, and offset() uses checked usize::try_from instead of a silent `as usize`. The proptests round-trip the full f64/usize ranges.
- **ranking** — finite_score + normalize is a genuinely careful piece of float engineering: NaN is neutralized at score construction (so it can never reach the sort and scramble the rank, fixing the old partial_cmp().unwrap_or(Equal) trap), while ±Inf 'sort last/first' sentinels are preserved through a total_cmp sort and then folded to the correct end of [0,1] by computing the min/max range over finite scores only. The negated-scale regression (Shortest's -duration making a missing item normalize to 1.0) is captured by an explicit test. This is exactly the 3am-incident-proof rigor the project demands.
- **entities** — Durability is genuinely complete and traced end-to-end: every derived index (user_state, hard_neg, preference, co_engagement) has restore wired into open.rs and checkpoint/persist wired into the periodic thread, shutdown, and backup. I verified the call sites rather than trusting the doc comments, and the non-durable add_save/add_like/record_completion variants are correctly reserved for ephemeral (no-storage) mode only.
- **governance** — The checkpoint key codec is exemplary defensive engineering: encode_contributor_suffix puts (entity, signal_type) in the KEY (not just the value) so one contributor spanning multiple cells produces distinct rows, the rustdoc names the exact BLOCKER it fixes, and checkpoint_restore_preserves_multi_entity_multi_signal_contributor regression-tests it directly.
- **replication-core** — receiver.rs durability is genuinely WAL-first and verified end-to-end: apply_replicated_event appends to the follower's own WAL via a group-commit writer whose append() blocks until segment.sync() (fsync) before the reply, and state.advance() only runs after every staged event is durably synced — the BLOCKER-6 fix is real, not wiring.
- **replication-crdt** — The convergence argument is genuinely well-engineered: switching per-node decay merge from addition to a last-writer-wins register versioned by last_update_ns (signal_state.rs:30-45) is the correct fix for replay-induced double-counting, and the doc comment explains exactly WHY addition is unsafe under at-least-once delivery. This is the hard part of the slice and it is right.
- **schema** — EntityKind::fingerprint_byte (entity.rs:68) is exemplary: one const fn is the single source of truth for the kind->byte mapping, its coupling to both schema_fingerprint and embedding_slot_fingerprint is documented at the definition, and a dedicated test (entity_kind_fingerprint_byte_is_canonical) locks the byte values so a refactor cannot silently change the on-disk fingerprint. This is precisely the 3am-incident-proofing the project asks for.
- **session** — The decay-math consolidation is exemplary: SessionHotState delegates to the canonical forward_decay_step kernel and documents the prior bug it fixed (out-of-order weight was added at full value, over-crediting stale activity). The two property-style tests (out_of_order_event_contributes_strictly_less_than_in_order, out_of_order_event_does_not_regress_timestamp) verify the exact CODING_GUIDELINES §3 invariant against an analytical pre-decay value — this is real correctness verification, not coverage theater.
- **text** — The syncer's resilience design is exactly the 3am-incident engineering the project charter demands: a failed Tantivy commit retains the buffered batch, retries with exponential backoff, flips a shared health flag after a threshold, and keeps draining the channel — with a thread-local fault-injection hook and a CRITICAL-tagged regression test (`syncer_survives_transient_commit_failure_and_recovers`) proving the dead-syncer-loses-all-writes failure mode stays fixed.
- **cohort-load-testing** — Crash-injection infrastructure is genuinely production-grade for a test harness: every CrashPoint variant is wired to a real write-path call site (signals/hot, signals/ledger/core, signals/checkpoint, cohort/ledger, db/collections, entities/co_engagement) and exercised by m7_crash_property.rs, the atomic Orderings (AcqRel on the crossing counter, Release/Acquire on armed/fired) are each justified with a precise comment, and one-shot + thread-local install keeps it catch_unwind-safe.
- **tidal-net** — circuit_breaker.rs: the half-open single-probe invariant is enforced by the state machine (in-flight HalfOpen state), not merely advertised, and the record_backpressure path that resolves a half-open probe without opening the breaker (avoiding a wedged-forever breaker the first time a probe draws accepted=false) shows genuine 3am-incident thinking. The 14 tests cover every transition including the subtle interleavings.
- **tidal-server-cluster** — dispatch_shards is a genuinely careful piece of concurrent engineering: detached (not scoped) workers so the coordinator can return its partial result the instant the budget expires without joining an in-flight blocking query; a sync_channel bounded to live_shards.len() so no late worker ever blocks on send into a dropped receiver; the coordinator's own tx dropped so recv_timeout observes Disconnected instead of burning the full budget; and a try_recv drain pass to catch race-window stragglers. Timed-out and failed-to-spawn shards are recorded as degraded by name, never silently dropped. This is the right answer to 'one slow shard must not stall the gather' and it is documented to match.
- **tidal-server-core** — Anti-drift design is excellent: `dto.rs` and `health.rs` are deliberately the single source of truth for the request/response shapes and probe bodies shared by the standalone and cluster routers, with a clear comment explaining that prior verbatim duplication let the two modes' APIs silently diverge. This is the right abstraction for a likely change.
- **tidalctl** — Exemplary null-vs-zero discipline: every field that cannot be honestly derived offline is typed Option<_> and serialized as JSON null with a self-describing offline_unavailable reason map, never a fabricated 0 that an operator would misread as 'healthy/idle'. This is exactly the 'if you can't observe it, you can't operate it' standard, and it is regression-tested (diagnostics_no_checkpoint_reports_null_age, diagnostics_unreadable_text_index_exits_2_and_reports_null).


## Appendix — per-slice seven-dimension verdicts

### db-core-lifecycle
- **Completeness:** Open/close/recovery paths are thorough and well-instrumented, but two configured builder fields (wal_dir, cache_dir) are validated yet never consumed — a field that does not flow through every layer.
- **Accuracy:** Concurrency, atomics ordering, CAS-idempotent shutdown, and lock-acquisition order are correct and carefully reasoned; the main defects are config-honoring (silent ignore of wal_dir/cache_dir) and a Paths layout that misreports fjall's real on-disk shape.
- **Tech Debt:** Acknowledged and documented: TidalDb ~60-field struct + two 100+-line constructors exceed §9; the debt is real but explicitly tracked with a safe incremental-extraction plan.
- **Maintainability:** Comments consistently explain WHY (obs-REPL-1, BLOCKER 6/7, CONCURRENCY-1/2/3, SHUTDOWN-2) with traceable invariants; the two giant constructors are the only genuine cognitive-load smell and they are flagged in-code.
- **Extensibility:** StorageBox trait boundary, NodeConfig/role abstraction, and Paths-as-single-source-of-layout are the right seams; runtime-validated builder over typestate is a defensible, documented choice.
- **Dry:** SharedDefaults + build_control_plane + make_single_node_session_bridge already extract the duplicated constructor wiring well; remaining duplication is the per-Tag restore-scan loop in from_parts (6 near-identical blocks) which is the missing abstraction.
- **Clean:** Names are intention-revealing and the hot path is untouched (this is cold-path lifecycle code), but Paths exposes items_dir/users_dir/creators_dir/ensure_all that describe a directory layout the engine never creates — dead and misleading API.

### db-entity-ops
- **Completeness:** Strong. Validation, error paths, ephemeral no-op contracts, and crash-recovery comments are present and explicit. Gaps: collection writes lack the u32-universe guard that items.rs enforces, and Mute relationships are durably stored but inert (acknowledged as tracked).
- **Accuracy:** Mostly sound, but two real correctness/durability concerns: collection item IDs > u32::MAX silently alias (no guard, unlike items.rs which rejects), and several persist-after-in-memory-mutation orderings diverge on persist failure (add_to_collection, leave_community) leaving in-memory ahead of durable state.
- **Tech Debt:** Moderate. The bare `as u32` narrowing is duplicated across collections/signals while items.rs rejects and relationships.rs routes through an observable helper — three different policies for the same invariant. items.rs explicitly flags this as out-of-scope debt.
- **Maintainability:** Good. Functions are focused, names intention-revealing, and the WHY comments are unusually thorough (durability ordering, lock-poison recovery, narrowing rationale). write_item_with_metadata is long but justified and annotated.
- **Extensibility:** Good. Shared persistence helpers (write_entity_meta/read_entity_meta/persist_to_items/write_entity_embedding) centralize the entity format; the namespaced collection key layout enables bounded prefix scans. The missing piece is a shared item-id narrowing guard the doc itself calls for.
- **Dry:** Good with one gap. Metadata persistence and embedding writes are well-factored into users.rs helpers. The unextracted abstraction is the u32 item-id narrowing/guard, hand-rolled differently in 3+ call sites.
- **Clean:** Clean and readable overall. One efficiency red flag: community_contribute synchronously re-checkpoints the entire community ledger on every accepted contribution (O(total contributions) per write).

### db-signals-sessions
- **Completeness:** Mostly complete; one real gap — NotificationTracker has an unbounded-growth mitigation (evict_old_days) that nothing ever calls, and the signal_with_context durability docstring omits two of the side-effect indexes it actually mutates.
- **Accuracy:** Strong. WAL-first durability holds, atomics use correct orderings, the check_and_record TOCTOU fix and closed-session cap insert-then-trim ordering are sound and concurrency-tested. No panics on normal paths; the one expect() in recovery is provably infallible and documented.
- **Tech Debt:** Low-moderate. A dead public surface in NotificationTracker (record/would_exceed_per_creator/would_exceed_total) duplicates logic the executor already inlines, and evict_old_days is wired to nothing.
- **Maintainability:** High. Intention-revealing names, focused functions, and unusually good WHY-comments on the non-obvious concurrency and WAL-replay-ordering decisions.
- **Extensibility:** Good. Executor wiring is a clean builder; embedding-slot resolution is centralized; scope/governance is threaded flatly but coherently. No over-engineering for hypotheticals.
- **Dry:** Mostly DRY. The backup-guard + backpressure block is duplicated verbatim between signal and signal_scoped; the embedding-slot resolver pattern is duplicated for item vs creator; the cap-enforcement logic exists both in NotificationTracker (dead) and inlined in the executor.
- **Clean:** Clean and efficient. Hot paths avoid per-candidate allocation, the load detector guard is RAII, and degradation level is threaded without branching the query body.

### db-recovery-rebuild
- **Completeness:** Strong. Crash-recovery rebuild is unconditional from the durable source of truth (text/vector pattern), partial-failure arms are non-fatal+logged, replication HWM is restored, and the dual-write remote path fails closed rather than silently dropping. One gap: only the item side of the text-health latch self-heals; creator-only unhealth never clears from the background thread (documented).
- **Accuracy:** Sound. Atomic orderings are deliberate and correct (Release/Acquire on last_seq for shutdown; Relaxed on the periodic read is safe because the WAL marker is intentionally NOT advanced periodically). DashMap iteration in the sweeper collects-then-closes, avoiding re-entrant deadlock. Schema fingerprint is collision-safe today only by coincidence of disjoint byte ranges (no length-prefix canonicalization).
- **Tech Debt:** Low-to-moderate. signal_for_tenant remote dispatch is a documented fail-closed stub tied to a spec task (acceptable per §12). The 30s/10s/500ms/60s timing constants are inline literals. Two full item-store scans at open is duplicated work.
- **Maintainability:** High. Doc comments consistently explain WHY (BLOCKER/obs references, durability ordering, deadlock rationale). Functions are focused. run_checkpoint_thread is long (~180 lines) and mixes timing, trimming, checkpointing, compaction, and metrics at several abstraction levels.
- **Extensibility:** Good. ItemIndexes bundles the shared write/rebuild indexing path so the two cannot drift — the strongest design decision in the slice. TextSyncerPending's two-phase open/spawn cleanly prevents the rebuild-vs-writer-lock deadlock. The schema fingerprint's excluded-fields trade-off is explicitly reasoned and versionable.
- **Dry:** One real missing abstraction: four near-identical scan_prefix(&[]) + parse_key + Tag::Meta + EMB-skip loops over the items engine (rebuild_item_indexes, rebuild_text_index_from_engine, rebuild_item_text_and_suggestions_at_open) and a creator variant. At open, the items keyspace is fully scanned twice.
- **Clean:** Clean and neat. Intention-revealing names, good tracing discipline (no println!), no dead code. Mixed abstraction levels inside run_checkpoint_thread are the only blemish.

### db-export-backup-http
- **Completeness:** Strong — export validates limits and unknown signal types up front, backup quiesces writes and handles partial-failure non-fatally, http surface is bounded and tested. Gap: the session-journal export path has no limit/streaming, and the JSON-Lines `ExportFormat` enum and `to_json_line` are never used by `export_signals` itself (callers must serialize), leaving the wire format half-wired.
- **Accuracy:** Mostly sound. WAL u8 signal-type truncation is correctly guarded at both write and export boundaries; backup ordering (ledger checkpoint -> flush -> copy -> WAL marker) is crash-consistent and well-reasoned; signal weights are finite-validated upstream so f32 export cannot emit JSON null. One real correctness/resource issue: unbounded session-journal read defeats the export limit.
- **Tech Debt:** Low. Duplicated since/until range-filter logic across the two scan paths; one inconsistent timestamp source in backup (raw SystemTime vs Timestamp::now elsewhere). No storage-engine type leakage, no magic-number sprawl.
- **Maintainability:** Excellent — comments explain WHY (the bounded-heap rationale, the in-flight-WAL consistency argument, the u8 truncation history) rather than WHAT. Functions are focused and well under the 50-line smell threshold.
- **Extensibility:** Good. `ExportFormat` is an enum ready for more formats, backup secondary-checkpoint set is factored into one helper shared with shutdown, http request bounding is constant-driven. `ExportRequest` is open to new filters without breaking callers.
- **Dry:** Minor duplication: the `since`-inclusive/`until`-exclusive time-window filter is hand-written in both `scan_batch_wal_bounded` and `read_session_journal_signals`; the secondary-checkpoint set is duplicated between backup and shutdown (documented, acceptable).
- **Clean:** Clean and neat. One needless allocation in `extract_path` (collects a Vec just to index [1]) but it is on the cold metrics endpoint, not a hot path. Naming is intention-revealing throughout.

### db-metrics
- **Completeness:** Solid — every rendered gauge/counter has a real producer (active_sessions in sessions.rs, last_checkpoint_ns/wal_lag_bytes/tantivy_indexed_docs in state_rebuild.rs, latency histograms in signals.rs/query_ops.rs), health derivation covers dead/stale checkpoint thread, and tests assert well-formed exposition. Gap: usearch_index_size_bytes, usearch_vector_count, tantivy_segment_count, signal_hot_entries, bitmap_index_cardinality, wal_compacted_segments_total appear rendered but I could not confirm a production writer for all of them in this slice's blast radius.
- **Accuracy:** Correct. Cumulative-bucket observe via partition_point is sound and proptested against the naive rule; +Inf=total_count is right; staleness saturating_sub is safe against backward clock jumps; Acquire/Release pairing between is_degraded and the checkpoint writer is correct; Relaxed on histogram counters is justified and appropriate. One real truncation: as_micros() (u128) -> u64 cast in callers, harmless in practice but undocumented.
- **Tech Debt:** Low. partition_id="0" is hardcoded into every always-on metric line, which will mislead in cluster/multi-partition mode. Two separate now-in-nanos implementations (gated helper vs inline writer) contradict the module's own DRY claim.
- **Maintainability:** Strong. Clear module doc with a 3-step recipe for adding counters, intention-revealing names, WHY-comments on ordering and gating, focused functions. render_prometheus is long but flat and uniform (the #[allow(too_many_lines)] is honest).
- **Extensibility:** Good for the stated growth path (add field, increment, add render line). LatencyHistogram is reusable with static bounds. Weak spot: partition_id is not parameterized, so cluster-aware labeling is a future rewrite of the constant block rather than a config knob.
- **Dry:** One real smell: the 'now in nanos' logic exists twice with divergent overflow semantics (metrics/mod.rs now_unix_nanos uses try_from/u64::MAX; state_rebuild.rs:489 uses as_nanos() as u64 with unwrap_or_default). The module doc claims it 'lives in one place' but it does not, because the helper is metrics-gated and the writer is not.
- **Clean:** Clean and neat. No dead code, no needless allocation on the observe hot path (no per-call String), no misleading names. render_prometheus builds one String with format!/write! — efficient. The histogram comment block on observe is exemplary.

### wal-writer-reader
- **Completeness:** Strong — recovery, torn-tail repair, dedup, checkpoint, rotation, and directory fsync are all present with fault-injection tests; one real gap is the shutdown-drain multi-chunk early-return that can drop later callers' replies, contradicting the module's stated 'every queued append eventually resolves' invariant.
- **Accuracy:** Mostly sound and unusually well-reasoned on durability (data sync + explicit dir fsync on create/rotate/checkpoint/delete, checked sequence arithmetic, active-segment clamp). Defects: a checkpoint tmp file can leak on a crash mid-write, the shutdown-drain `?` drops remaining reply channels, and a corrupted-but-checksum-valid batch whose first_seq is far below the checkpoint silently advances next_seq past a gap (acceptable but undocumented).
- **Tech Debt:** Low. The hardcoded `data[offset + 24..offset + 28]` payload_len slice in the reader duplicates the header layout that format::batch already owns — a magic-offset that will silently break if the header layout ever shifts.
- **Maintainability:** High. Rustdoc is exemplary — every non-obvious durability and concurrency decision is explained with the failure it prevents. `run_writer` is long but the extraction into flush_batch/partition_dedup/handle_aux_command keeps each piece focused.
- **Extensibility:** Good. Shard/region identity is threaded through cleanly, segment filename is versioned (v1/v2), and the storage surface stays behind WalError. No over-engineering for hypotheticals.
- **Dry:** Good. The shared flush_batch/partition_dedup/handle_aux_command routines deliberately collapse the three command loops so they cannot diverge. The one duplicated constant is the 24..28 header offset (owned by format::batch, re-hardcoded in reader).
- **Clean:** Clean. Names are intention-revealing, no dead code, no needless allocation in the commit path (batch Vec capacity is reused via drain). Comments explain WHY, not WHAT.

### wal-format-recovery
- **Completeness:** Strong. Both wire formats are versioned with documented backward-compat decode, corruption is surfaced (not swallowed) into the recovery path and diagnostics roll-up, and tests cover legacy/v3/torn-tail/checksum-mismatch/forward-compat. No orphan TODOs, no commented-out code, no empty error arms.
- **Accuracy:** Correct on the load-bearing paths: durability ordering (compact only after checkpoint+flush; dedup.record only post-fsync), the compact_wal_online live-segment clamp closes a real silent-data-loss hole, diagnostics use the same `>` replay predicate as recovery, BLAKE3 covers shard/region+governance bytes, and first_seq+i is checked_add. The one expect() is provably infallible and documented. Minor semantic conflation in v2 decode-failure handling (see findings), no blockers.
- **Tech Debt:** Low. A couple of magic numbers (frame-length literals in tests, has_* flag bytes) and a duplicated optional-field framing pattern, but nothing structurally rotting. Storage types do not leak; only RegionId/ShardId (replication domain types) cross in, which is intended.
- **Maintainability:** High. Wire layouts are documented byte-by-byte, named constants explain offsets, comments consistently explain WHY (the dedup contains/record split, the online-compaction hazard). Functions are focused; decode_session_events_with_diagnostics is the longest but reads linearly.
- **Extensibility:** Good and YAGNI-appropriate. The version byte + event_size_for_version dispatch makes a v4 event record a localized change; the SessionDecodeOutcome struct gives room to add more diagnostics. The v2 session-record forward-compat path (unknown checksummed types are skipped) is a genuine extensibility win.
- **Dry:** One real missing abstraction: the `[flag: u8][optional payload]` encode/decode pattern is hand-rolled three times in session.rs (annotation, session_seqno, idempotency_key). Otherwise the parse_base / read_*_le helpers already factor the common decoding well.
- **Clean:** Clean. Intention-revealing names, no dead code, error-path allocations (format!/into) are confined to the cold corruption path and never touch the hot encode/decode loop. Encode/decode loops are tight with pre-sized buffers.

### signals-decay-hotwarm
- **Completeness:** Mostly complete: decay/hot/trimmer are well-tested with property tests and crash-point hooks; but the warm tier's 30d window has a verified double-count that no existing test catches (loose >=9/<=31 bounds let it slip), so the 'windowed aggregates equal events in window' invariant is NOT actually covered.
- **Accuracy:** One BLOCKER: the 30d windowed count double-counts ~24h of events for a full day after every day boundary (verified empirically: 116 real events reported as 216). Decay math and hot-tier CAS concurrency are otherwise sound and well-reasoned.
- **Tech Debt:** Low. Magic ns constants are centralized (NANOS_PER_SEC, NS_PER_MIN/HOUR/DAY). The main debt is the triplicated 'last 24h = in-progress hour + 23 hour buckets' expression, which is the structural root of the double-count bug.
- **Maintainability:** Strong. Exceptional doc-comments explaining WHY (ordering rationale, fold-in reasoning, crash-point placement). Functions are focused and under the 50-line smell threshold. maybe_rotate is the densest but is heavily commented.
- **Extensibility:** Good. The forward_decay_step kernel is correctly extracted as the single source of truth across all tiers; MAX_DECAY_RATES and bucket counts are named constants; storage types do not leak. No over-engineering.
- **Dry:** One real smell: the 24h-rollup expression is written three times (windowed_count 24h, sum_current_day, and the day_agg snapshot in maybe_rotate). Extracting it would both DRY the code and force the day-boundary clearing question that the bug hinges on.
- **Clean:** Clean and efficient. Lock-free atomics on the hot path, no per-candidate allocation, mul_add for fused rounding, Relaxed where approximation is acceptable, Acquire/Release only at sync boundaries. Names are intention-revealing.

### signals-ledger-checkpoint
- **Completeness:** Mostly complete with strong test/proptest coverage, but the checkpoint write path is missing a delete-stale-keys step — after the trimmer evicts entries, orphaned storage keys are never removed, so a whole class of recovery is untested and broken.
- **Accuracy:** Serialization/deserialization, offset-chain const-asserts, and integrity hashing are correct in isolation, but the checkpoint+trim+restore interaction produces a guaranteed integrity-hash mismatch that escalates to silent data loss once WAL segments are compacted away — a real durability hole.
- **Tech Debt:** Low. Named-offset codec, shared id-derivation helper, and clone-free hash iterator are deliberate, well-justified choices. The known full-keyspace-scan limitation is documented with a clear rationale.
- **Maintainability:** High. Doc comments explain WHY (out-of-range decay None, local-profile-intact, hot/warm reconcile), functions are focused, names are intention-revealing. The offset table + const-assert make the wire format safe to evolve.
- **Extensibility:** Good. Versioned record formats (V1/V2 entry, V1/V2 meta) with backward-compatible deserialize, a WalWriter trait with a default-method governance extension point, and a single entry-construction site. No YAGNI over-engineering.
- **Dry:** Strong. derive_signal_ids_and_lambdas is the single id/lambda source for global+cohort ledgers; apply_event_local + get_or_create_entry centralize the mutation path; hash_checkpoint_payload_iter and the owned variant share one hashing core. One minor duplicated meta-parse block across V1/V2 arms.
- **Clean:** Clean and efficient. Lock-free hot path honored, no per-candidate allocation in serialize, clone-free hashing. One efficiency/determinism wart: read_decay_score/read_windowed_count each call Timestamp::now() internally, so a per-candidate scoring loop takes N independent clock reads and lacks a consistent query time.

### storage-core
- **Completeness:** Strong — every trait method is implemented on both backends, atomicity has both a crash test and a no-crash companion, and the streaming-iterator fix is covered. Gap: the trait contract is silent on write_batch durability, so the atomic-but-not-fsynced semantics live only in the impl comment, not the trait that callers code against.
- **Accuracy:** Correct. write_batch routes through a single fjall OwnedWriteBatch (one journal record, one seqno) so recovery sees all-or-none; commit is Buffer-durable and callers pair it with flush()/persist(SyncAll). map_fjall_err classification is sound and conservative on the non_exhaustive wildcard. No unwrap/expect/panic in production paths; no locks held across .await (sync code). Atomic ordering and byte encoding verified.
- **Tech Debt:** Low. Minor: triple-repeated keyspace-open block in FjallStorage::open; FjallAtomicBatch is a publicly re-exported fjall-specific primitive with no production caller (tests only) and no trait equivalent.
- **Maintainability:** Good — intentional names, focused functions (all <50 lines), comments explain WHY (durability, snapshot ownership, classification). Main drag: keys.rs Tag maintenance is spread across as_byte/from_byte/doc/3 test arrays with no single source of truth.
- **Extensibility:** Good. StorageEngine trait cleanly hides the backend; StorageBox composes it; nothing leaks fjall types upward. Cross-keyspace atomicity is only modeled by the concrete fjall type, so a future second engine (RocksDB) cannot satisfy the cross-keyspace transaction use case through the trait.
- **Dry:** One real missing abstraction: the Tag enum is a hand-maintained pair of parallel match tables (as_byte/from_byte) plus three verbatim 25-element arrays in tests, and the two proptests disagree on the tag-byte range (1..=25 vs 1..=23). Adding a tag silently risks drift with no compile-time guard.
- **Clean:** Clean. No dead code, no needless allocation in the streaming scan path (the prior eager .collect() was removed and documented), consistent abstraction levels, neat error mapping. with_capacity on the batch is appropriately non-const due to allocation.

### storage-indexes
- **Completeness:** Strong overall: persistence, deletion, cardinality, selectivity, and recursion guards all present and tested. One real gap — RangeIndex has no delete_entity counterpart to BitmapIndex, and the index update path (items.rs) only inserts on re-write, leaving stale old values; the missing scrub capability lives in this slice.
- **Accuracy:** Solid. RwLock poison handled defensively, inverted-range bounds clamp instead of panicking BTreeMap::range, u32-universe bound enforced defensively in the predicate, length-prefixed keys remove the field/value delimiter ambiguity. No torn writes, no panics on normal paths, no wrong atomic ordering (no atomics here). The recursion guard (256 nodes) is wired at both query entry points.
- **Tech Debt:** Low. Minor double-lookup (get_mut then get) in both delete paths; MAX_FILTER_NODES literal duplicated across query_ops.rs call sites (outside slice). No storage-engine types leak — clean trait-free Vec<(Vec<u8>,Vec<u8>)> KV boundary.
- **Maintainability:** Excellent. Intention-revealing names, focused functions, doc comments explain WHY (key disambiguation, complement semantics, predicate u32 bound). Comments are load-bearing, not noise.
- **Extensibility:** Good. RangeKeyCodec trait cleanly parameterizes the only per-type difference (BE width). FilterExpr enum is open for new variants. The bitmap/range delete asymmetry is the main extensibility wart.
- **Dry:** Mostly DRY. RANGE_KEY_PREFIX shared write/read side prevents drift. The 10-byte key-prefix skip and the get_mut-then-get delete dance are each duplicated across bitmap.rs and range.rs — a small missing shared helper.
- **Clean:** Clean and efficient. No per-candidate allocation in evaluate; AND intersects smallest-first with exact (not estimated) ordering and short-circuits on empty; OR/NOT minimal. Predicate avoids id truncation. Mixed abstraction is absent.

### storage-vector
- **Completeness:** Strong — trait, three backends, lifecycle, registry, and crash-recovery rebuild are all present with extensive unit + property + corruption tests; the only real gap is the per-query ef_search override carrying an unticketed TODO, and a documented soft-delete crash window.
- **Accuracy:** Mostly sound — corruption hardening in brute load is excellent and the soft-delete tombstone closes a real resurrection bug; the notable correctness gaps are a non-atomic insert/delete on the entity store (lost-update / orphaned-tombstone window) in the lifecycle ops, and a torn-write window during update_embedding/delete_embedding where the entity store and ANN index can diverge with no recovery short of full rebuild.
- **Tech Debt:** Low-moderate — unticketed TODO for per-query ef_search; the double full-store O(N) scan in rebuild_from_store with per-key String allocs is a startup-cost smell that will bite at scale.
- **Maintainability:** Excellent — intention-revealing names, WHY-focused comments, focused functions, single-source-of-truth constants for HNSW defaults and the ARCHIVED prefix.
- **Extensibility:** Excellent — the VectorIndex trait cleanly isolates usearch behind the boundary (CODING_GUIDELINES §2 honored), swappable backends, adaptive filtered-search strategy correctly deferred to the query planner.
- **Dry:** Very good — shared validate_dimensions / read_*_le helpers, single-source HNSW default consts, shared normalize_and_store prologue and ARCHIVED_PREFIX constant; minor duplicated ef_search-warning block across search/filtered_search.
- **Clean:** Very good — brute-force top-k uses select_nth then sorts only k (efficient), distances avoid sqrt, comments are accurate; the rebuild double-scan is the main efficiency blemish.

### query-executor
- **Completeness:** Strong. All 6 pipeline stages are implemented with graceful-degradation paths, warnings on every unsatisfiable-scope branch, and ~1260 lines of unit tests plus shared fixtures. Gap: cohort rescore claims loud-failure parity with the main scoring path but silently scores 0.0 on an unknown signal name; ANN candidate strategy is a documented scan-fallback stub.
- **Accuracy:** Mostly sound. Atomic check-and-record closes the notification TOCTOU; cursor-offset overflow is saturated; u32 truncation is validated at write time so the executor's `as u32` casts are provably safe. One real divergence: `rescore_with_cohort` swallows the schema error that the main path propagates. Off-by-pagination, ordering, and bitmap intersection logic all check out.
- **Tech Debt:** Low. ANN fallback is a tracked stub; offset-pagination is documented as a known M3 replacement target. No magic numbers without rationale; no storage types leak into the module (all access via the StorageEngine trait).
- **Maintainability:** Excellent. Every non-obvious branch carries a WHY comment (the separate for_creator arms, the Not-skip traversal discipline, the New-profile truncation partition). Functions are focused; `execute` is long but linear and stage-delineated. Names are intention-revealing.
- **Extensibility:** Good. Builder-style `with_*` setters make the executor additively configurable per milestone; the shared `PostFilterCtx` + `apply_deferred_post_filters` give RETRIEVE and SEARCH one drift-proof seam. Stage boundaries are clean.
- **Dry:** One real smell: four near-identical `collect_*_filters` AST walkers share a load-bearing And/Or-recurse-skip-Not traversal that should live in exactly one generic walker. Otherwise the shared post-filter module is exactly the right de-duplication.
- **Clean:** Clean and efficient. Bounded-buffer top-K avoids O(N) heap in signal-ranked gen; pre-allocated result buffers; no per-candidate allocation in scoring. Minor: notification-cap creator resolution re-reads storage when a pre-loaded metadata map is already in hand.

### query-retrieve-search
- **Completeness:** Strong. RETRIEVE/SEARCH/SUGGEST surfaces are fully wired with graceful degradation, deferred-post-filter coverage shared across both pipelines, and tests covering the historically-leaky cases (OR-of-deferred, negated-deferred, no-universe post-filter). One real gap: the empty/poisoned-universe degradation fix applied to SEARCH Stage 2 was NOT mirrored into the RETRIEVE Stage 2 evaluator path.
- **Accuracy:** Mostly sound, with careful u32-overflow handling almost everywhere (entity_as_u32, retain_excluded/included, cursor decode). One genuine correctness hole: percentile_bitmap in scope.rs uses an unchecked `id as u32` guarded only by debug_assert, so a >u32 entity id silently aliases onto a low id in release builds and corrupts the trending scope pre-filter. BM25 NaN ordering and cursor non-finite handling are correctly defended.
- **Tech Debt:** Low. Magic numbers are named constants with rationale (ANN/BM25 caps). The known u32 bitmap key-space ceiling for for_creator is documented and deferred explicitly. The creator-metadata filter's value-only matching (no field discriminant) is a documented limitation that will need revisiting.
- **Maintainability:** High. Stage helpers are focused and named to read linearly, the 8-stage pipeline is split out of the executor struct file to respect the 600-line rule, and comments consistently explain WHY (the poison-recovery, the u32 aliasing risk, the relevance-anchor design). Doc comments are unusually load-bearing and accurate.
- **Extensibility:** Good. Cursor is discriminant-tagged so keyset pagination can replace offset without wire breakage; ScopeResolver and the builder pattern compose cleanly; deferred-filter extraction is centralized so a polarity-aware evaluator can be slotted in later. Embedding-slot resolution is threaded rather than hardcoded.
- **Dry:** Strong. Deferred post-filters, OR/NOT rejection validators, user-state extraction, and the rrf_term formula are each defined once and shared by RETRIEVE and SEARCH. The DEFAULT_EMBEDDING_SLOT literal is centralized. Minor residual duplication: the Stage 5 pagination/assembly block and the user-context suppression block are near-identical between the two pipelines but not extracted.
- **Clean:** Clean. Hot-path scoring avoids per-candidate allocation, combined_filter is computed once to avoid repeated tree clones, and sorts use total_cmp where NaN could appear. Naming is intention-revealing throughout.

### ranking
- **Completeness:** Clean — every Sort variant is implemented and exercised by sort_tests.rs; gates/excludes/penalties/decay/exploration are all wired; degradation-window substitution is handled. Rising/MostFollowed/CreatorEngagementRate have executor + test coverage but no built-in profile (reachable only via custom schema profiles); Hot/New use an entity-id recency proxy, both explicitly documented as known limitations rather than silent gaps.
- **Accuracy:** Strong. The sort comparator uses f64::total_cmp and finite_score neutralizes NaN at construction, closing the old partial_cmp().unwrap_or(Equal) rank-scramble; normalize folds ±Inf sentinels to the correct end of [0,1] with a dedicated regression test; alphabetical f64 prefix collisions are broken on the full title. The executor is stateless and read-only over the ledger, so no concurrency/durability surface. One real correctness-adjacent issue: per-comparison String allocation in the alphabetical tie-break.
- **Tech Debt:** Minor and well-tracked. The `now` parameter is threaded through the whole scoring API but steers no read (decay ages forward to wall-clock inside the ledger) — documented as a clock-contract debt. `resolve_signal_type` is consumed as a Result via map_or, silently swallowing the schema error instead of routing through the read_agg_for_sort degradation path — inconsistent but behaviorally equivalent.
- **Maintainability:** Excellent. Doc comments consistently explain WHY (the finite_score/normalize sentinel reasoning, the co-engagement additive-vs-convex deviation, the Hot age-proxy rationale). Functions are focused and the formulas are isolated as pure functions in formulas.rs for independent testing.
- **Extensibility:** Very good. Profiles are data (Serialize/Deserialize), sort modes are a closed enum dispatched in one place, the registry versions and validates without leaking storage types, and degradation is a parameter rather than a fork. No over-engineering for hypotheticals.
- **Dry:** Good with one structural smell: score_inner and score_personalized re-implement the same exclude→gate→compute_raw_score→session_boost→finalize candidate loop, differing only in the personalization terms. test_fixtures.rs is an exemplary DRY consolidation of previously-duplicated ledger/profile builders.
- **Clean:** Clear and neat, but two needless hot-path allocations: the alphabetical tie-break re-reads + re-lowercases titles (String alloc) on every equal-score comparison, and session_boost re-lowercases all session keywords for every candidate instead of once. Both violate the 'no per-candidate allocation in scoring' guideline.

### entities
- **Completeness:** Strong. Every index has durable backing wired into open/checkpoint/shutdown (verified user_state.restore, hard_negatives.restore, preference_vectors.checkpoint, co_engagement.restore), the Mute relationship is explicitly tracked-but-inert with a tracking note, and edge/error paths (empty/torn bytes, unknown user, capacity=0) are handled. Gap: preference update_counts persistence is deferred to M7 (documented), and the in-memory bitmap+Vec dual tracking in CollectionIndex has no consistency guarantee under concurrent reads.
- **Accuracy:** Mostly sound. Forward-decay kernel is shared correctly (interaction.rs handles out-of-order via advance_timestamp flag), NaN is neutralized at load boundaries, total_cmp gives deterministic ordering, full-u64 follower IDs fixed a real aliasing bug. Two concerns: co_engagement double-counts an edge when prev_item appears multiple times in the recent VecDeque, and the collection caller truncates item IDs to u32 with a bare cast (the exact pattern narrow_item_slot was built to make observable).
- **Tech Debt:** Moderate. CollectionIndex maintains membership in BOTH a RoaringBitmap and a Vec<u32> with O(n) linear contains()/retain() per mutation — the Vec is only needed at serialize time. CreatorItemsBitmap exposes get_ref() to avoid clones but hot-path callers use the cloning get(). No magic-number sprawl; constants are named.
- **Maintainability:** High. Intention-revealing names, focused functions, comments explain WHY (the narrow_item_slot rationale, the orthogonal seen/hidden state note, the total_cmp NaN reasoning) rather than WHAT. Durability contracts are documented per-struct. Serialization codecs are hand-rolled but linear and readable.
- **Extensibility:** Good. RelationshipType::from_byte returning None forces every match to be revisited when a variant is added (no silent default), the storage-engine trait boundary is respected (no fjall types leak in), and checkpoint/restore follow one consistent sentinel-entity prefix pattern. No over-engineering for hypotheticals.
- **Dry:** Mostly DRY. decay_to centralizes the read-side decay so score/top_creators cannot drift; blend_and_normalize is the shared EMA body. The missing abstraction: four near-identical hand-rolled little-endian field-by-field decode loops (mod.rs, collection.rs, relationship.rs, user_state.rs durable rows) repeat the same bounds-check + from_le_bytes + advance-pos ritual; a small reader helper would remove the repetition and the chance of an off-by-one in one copy.
- **Clean:** Clean overall. The main efficiency smell is the per-creator RoaringBitmap clone on the candidate-generation hot path (CreatorItemsBitmap::get used where get_ref would avoid the alloc), and top_candidates doing a full O(all-edges) scan per call. Hot-path structs here are index containers (DashMap), not per-candidate scoring structs, so the repr(C, align(64)) rule does not apply to them.

### governance
- **Completeness:** Strong — every governance surface (scope/share/membership/policy/capability/tombstone/provenance/ledger) has lifecycle, durable persistence, crash-replay, and tests; the one gap is an unticketed TODO in capability.rs eviction and a Purge LeaveMode declared-but-rejected, both documented.
- **Accuracy:** Mostly sound — atomics use correct Acquire/Release at gate boundaries, CAS revoke/stop-forward are idempotent, checkpoint swap is atomic via WriteBatch, key codec collision is fixed and tested; one real data-integrity footgun (empty writer_agent purges all direct writes) and a determinism/consistency wrinkle in windowed_count's internal clock.
- **Tech Debt:** Low-to-moderate — clean codec abstractions and shared serde helper; the standout debt is the synchronous full-snapshot checkpoint on the community write path (an algorithmic O(N^2) ingest cost) and one unticketed TODO.
- **Maintainability:** Excellent — intention-revealing names, WHY-focused comments tying each guard to the failure it prevents, focused functions, consistent structure across all ten files.
- **Extensibility:** Good — ScopeClass↔SignalScope pinned by compile-time const asserts, versioned policies mirror the profile registry, RemoveScope enum and LeaveMode leave room for new variants without rewrites; no over-engineering.
- **Dry:** Strong — to_durable_bytes, encode/decode_contributor_suffix, and the atomic_u64 serde adapter are each single-sourced specifically to prevent drift between two call sites; no meaningful duplication found.
- **Clean:** High — no dead code, no misleading names, hot-path gates are O(1) atomic loads; the lone efficiency blemish is the per-write full-rewrite checkpoint and an O(all-cells) contributor_count scan.

### replication-core
- **Completeness:** Strong. WAL shipper, receiver, idempotency, reconcile, migration, tenant routing, lag, upgrade are all implemented with edge/error paths and extensive tests (crash-survival, fault-injection, proptests). Gaps: control.rs lag_for is single-shard-only (documented), and the receiver's atomic-payload claim is not honored on a mid-loop disk fault.
- **Accuracy:** Mostly correct; concurrency primitives (CAS loops, advance_atomic_max, rate limiter refill) are carefully reasoned. One real correctness hole: apply_payload Phase 2 applies events one-at-a-time and is NOT atomic on a mid-loop WAL-append failure, contradicting its own docstring and risking double-count on re-ship.
- **Tech Debt:** Low. Magic thresholds are named consts with spec references. control.rs lag_for carries an explicit single-shard debt marker tied to M8p6+. No storage types leak across module boundaries except the deliberate Arc<dyn StorageEngine> sink in migration (trait-abstracted, correct).
- **Maintainability:** High. Intention-revealing names, WHY-focused comments, focused functions. spawn_shipper is long (~190 lines) but is a documented linear setup + per-peer loop with #[allow(too_many_lines)].
- **Extensibility:** Good. Transport is a clean trait with a blanket Arc<dyn Transport> impl; in-process channel wiring is shared between WAL and session factories; routing strategies are an enum. ShipperState was deliberately hoisted to an Arc for operator visibility.
- **Dry:** Good. advance_atomic_max, build_channel_endpoints, find_shard_assignment, last_seq/count_events scan loops are each extracted to one home. Minor: the three near-identical batch-scan-loops in shipper.rs (filter/last_seq/count_events) share a walk pattern that could be one iterator.
- **Clean:** Clean and efficient. No needless hot-path allocation in the lag/state/rate-limiter paths (atomics, no locks). Receiver stages into a Vec (cold replication path, acceptable). Naming and structure are consistent throughout.

### replication-crdt
- **Completeness:** Strong. Every CRDT type carries example-based AND proptest-based law coverage (commutativity/associativity/idempotency), HLC overflow and clamp edge cases are tested, and the snapshot round-trip is verified. Gap: no test pins down the `from_node_contribution` score/timestamp contract against its production caller, which is where a convergence bug hides.
- **Accuracy:** Mostly sound — the per-node LWW + per-node-max merge math converges, and all 74 tests pass. Two real holes: (1) `Hlc::update()` does NOT carry logical-counter overflow the way `now()` does, so a saturated counter can return a non-unique timestamp, contradicting its own docstring; (2) `from_node_contribution`'s `score`/`last_update_ns` contract is ambiguous and the production caller feeds a decayed-to-now score with a stale timestamp, causing double-decay at read time.
- **Tech Debt:** Low. `node_buckets` nests a full HashMap-backed `PNCounter` to hold a single node's key — a minor structural over-allocation in the warm path. No magic numbers leak; constants are named and documented.
- **Maintainability:** Excellent. Intention-revealing names, dense WHY-comments on every non-obvious branch (overflow carry, key-aligned lookup, LWW tiebreak), and functions are short and focused. A reader can follow the convergence argument from the doc comments alone.
- **Extensibility:** Good. The shared `lww_other_wins` helper is the right seam for the two LWW call sites, the storage engine is not touched, and the kernel `forward_decay_step` is correctly reused instead of re-implemented. No over-engineering for hypotheticals.
- **Dry:** Good with one drift: `LWWRegister::write` re-implements the `ts > cur` decision inline instead of routing through `lww_other_wins`, whose own doc claims to be 'the single place the decision lives'. Decay math is correctly centralized in `forward_decay_step`.
- **Clean:** Clean. No dead code, no needless allocation in the genuinely hot path (this is cold-path reconciliation), consistent abstraction levels. `decay_score` uses a clear iterator-map-sum with a key-aligned lookup that is explicitly documented as load-bearing.

### schema
- **Completeness:** Strong — every validation rule (signal names, decay params, windows, velocity, embedding slots, text fields, policy signal references) has both a typed error and a test. The one real gap: AgentPolicy numeric fields (max_session_duration, max_signals_per_session) are accepted at build() with zero validation, so a Duration::ZERO policy silently expires every session.
- **Accuracy:** Sound. Score/Timestamp/EntityId encodings are correct and property-tested; total_cmp gives a real total order; now() degrades on clock anomaly instead of panicking; saturating subtraction in decay math is consistent. No torn-write/durability concerns here (cold-path declaration types). Only accuracy nit is the ZERO-duration policy footgun above.
- **Tech Debt:** Low. fingerprint_byte is correctly centralized as one source of truth. Minor: empty-embedding-slot-name is reported as DuplicateEmbeddingSlot (documented reuse, but a misleading variant), and the two text-field sets duplicate the same validate_text_field_set call site rather than iterating a collection.
- **Maintainability:** High. Intention-revealing names, doc comments that explain WHY (fingerprint coupling, reserved-key panic avoidance, epoch-saturation safety), focused functions. build() is long but flagged with a justified #[allow(too_many_lines)] and is a flat sequence of independent passes.
- **Extensibility:** Good. DecaySpec/DecayModel split (user spec vs computed lambda) and the WindowSet abstraction leave clean seams. The fingerprint exclusion of windows/velocity/positive-engagement is explicitly documented as a versioning decision, not an accident. No over-engineering.
- **Dry:** Good. EntityKind::fingerprint_byte is the single kind->byte mapping; RESERVED_TEXT_FIELD_KEY is one const; validate_text_field_set is shared across item/creator sets. No meaningful duplicated validation logic.
- **Clean:** Clean. No dead code (target field's load-bearing role is documented and verified against schema_fingerprint), no needless allocation on any hot path (all to_owned/clone are cold build-time), no misleading names beyond the empty-slot-name error variant.

### session
- **Completeness:** Strong — every record type has a serde round-trip, every cap has an enforcement path and a test, and active/frozen snapshot paths are both covered. One gap: saved_search corruption handling silently diverges from the (stricter) serde module.
- **Accuracy:** Mostly sound. Decay math is correctly delegated to the canonical kernel, atomics ordering matches the proven HotSignalState pattern, and bounds-checking in the serde cursor is rigorous. Two real correctness concerns: saved_search uses lossy UTF-8 (silent corruption) while the rest of the slice rejects it, and build_frozen_snapshot reads two distinct wall-clock timestamps for one logical close instant.
- **Tech Debt:** Low. Shared byte-cursor helpers retired prior copy-paste; audit-entry codec is shared between snapshot and standalone paths. saved_search is the one module that re-rolls its own hand-inlined LE decode instead of reusing the serde cursor helpers — a duplicated, less-safe decoder.
- **Maintainability:** High. Module-per-concern split is clean, names are intention-revealing, comments consistently explain WHY (cap-before-entry-lock deadlock, ascending replay seed, read-time window aging). Doc tables in mod.rs and the version-byte history are genuinely helpful.
- **Extensibility:** Good. Snapshot format is explicitly versioned (0x01/0x02/0x03) with forward-compatible defaulting; PolicyViolationKind is a typed enum for caller dispatch; lambda is captured per-signal-type. No over-engineering.
- **Dry:** Good after the cursor-helper extraction. The one missing abstraction: saved_search's serialize/deserialize should use write_len_prefixed / read_len_prefixed_utf8 / read_u64 from the serde module rather than re-implementing length-prefixed LE decode by hand.
- **Clean:** Clean. Hot-path session decay is a single-atomic CAS loop with no per-call allocation; cold-path snapshot/serde paths allocate freely as appropriate. No dead code, no misleading names.

### text
- **Completeness:** Strong — every write/delete/rebuild/flush/shutdown path is implemented, error-returning, and exercised by unit tests; crash recovery is correctly delegated to rebuild-from-store at open. Gap: an unbounded producer channel has no backpressure or depth observability when the syncer is stalled in a commit-failure backoff.
- **Accuracy:** Sound — atomic health flag uses correct Acquire/Release pairing, the syncer retains its batch and retries on commit failure (the 3am failure mode is explicitly defended), and the collector's strict fast-field accessor avoids aliasing legal entity 0. One latency gap: a failed flush-path commit does not arm retry_not_before, so it is only retried on the slower timeout path.
- **Tech Debt:** Low — constants are named and documented at module scope, the merge policy is centralized, and the entity_id field name lives in exactly one place. No storage-engine type leaks; Tantivy is correctly confined behind this module.
- **Maintainability:** Excellent — intention-revealing names, focused functions, and doc comments that explain WHY (durability rationale, single-writer lock lifetime, clamp reasoning) rather than restating WHAT. The syncer run() loop is the only dense spot but is well-sectioned.
- **Extensibility:** Good — TextFieldType drives schema construction and default-field selection, the collector is generic over any query, and preprocess_query is a clean hook for future query-syntax additions. RRF/hybrid fusion lives outside this slice as intended.
- **Dry:** Good — merge policy, searcher_stats, and the entity_id field name are each defined once and shared by both read and write sides; a prior open()/ephemeral() merge-policy duplication was already extracted. Only duplication is per-module test helpers (test code, not flagged).
- **Clean:** Clean — no needless allocation in the collect hot path (merge_fruits pre-sizes the Vec), no dead code, consistent abstraction levels. preprocess_query allocates one String per parse on the cold query-parse path, which is acceptable.

### cohort-load-testing
- **Completeness:** Strong. Every CrashPoint variant is wired to a real call site and exercised by m7_crash_property.rs; cohort checkpoint corruption policy is documented, all-or-nothing, and tested. Gap: RateLimiterConfig's fallible validation (try_limited/try_new) exists but the public db builder only ever calls the infallible RateLimiter::new, so a degenerate user config silently degrades to unlimited at open time rather than failing loudly.
- **Accuracy:** Mostly sound: atomic Orderings on the in-flight gauge and crash injector are correct and justified; seqno bump/encode/leader-write is held under one lock with rollback for atomicity; partition set uses poison-tolerant locking. One real divergence: cohort ledger read_decay_score still does unwrap_or(0.0) on an out-of-range decay index where the global ledger was hardened to return Ok(None) — a latent silent-wrong-answer that the current executor (idx=0 only) does not yet trigger.
- **Tech Debt:** Acknowledged and documented in place: cohort restore's scan_prefix(&[]) full-keyspace scan and applied_count's hardcoded ShardId(0) are both called out with rationale. The await_convergence blocking redeliver-under-lock is undocumented debt. No orphan TODOs, no commented-out code.
- **Maintainability:** Excellent. Intention-revealing names, focused functions, and unusually rich WHY-comments (lock-ordering, Arc-clone-vs-String rationale, corruption posture). cluster.rs at 766 lines is large but justified by a single cohesive harness concern and an explicit module-size note.
- **Extensibility:** Good. Transport is trait-abstracted and pluggable (channel vs gRPC); degradation levels and thresholds are configurable; rate limiter keyed for per-session growth. The fallible config constructors are built for a future that the builder does not yet wire up.
- **Dry:** Strong intent — cohort ledger explicitly shares derive_signal_ids_and_lambdas with the global ledger and reuses the global fixed-format record. The one DRY break is the read_decay_score lambda-range-guard: the global ledger has it, the cohort ledger does not, so the two read paths have silently diverged on an invariant they are documented to share.
- **Clean:** Clean and efficient. Unlimited rate-limiter fast path avoids allocation and DashMap touch; cohort record keys by Arc::clone not String on the fan-out hot path; lazy token-bucket refill is syscall-free. No dead code, no misleading names, consistent abstraction levels.

### tidal-net
- **Completeness:** Strong — payload validation, TLS/mTLS, circuit breaker, permanent-vs-transient classification, and deterministic receiver shutdown are all present with thorough tests and documented known gaps (streaming, heartbeat→control-plane). Gap: no GrpcTransportConfig validation, and the client tolerates an insecure+no-TLS config the server rejects.
- **Accuracy:** Mostly correct and carefully reasoned. The circuit-breaker single-probe state machine, atomic shutdown latch ordering, and error classification are sound. One real concern: the runtime() helper uses expect() on a production path (infallible-by-invariant but undocumented as non-test); and the client/server insecure-mode asymmetry can produce a silently-plaintext client channel.
- **Tech Debt:** Low. The MAX_PAYLOAD_BYTES mirror is a conscious, compile-time-guarded duplication with a documented migration plan. StreamSegments/Heartbeat are explicit unimplemented/minimal stubs, not silent ones.
- **Maintainability:** Excellent. Doc comments explain WHY (backpressure-not-failure, latch vs notify race, block_on bridge rationale) rather than WHAT. Functions are focused and well under the 50-line smell. Names are intention-revealing.
- **Extensibility:** Good. Transport sits behind the engine's trait; factory pattern mirrors InProcessTransportFactory; config is fully parameterized. Streaming RPC is wired in proto and stubbed cleanly for later. No over-engineering.
- **Dry:** Good. The two codec-limit raises (client/server) and the two payload-size guards (transport/server) are intentional both-ends-must-agree pairs, well-commented. No problematic duplication; the one cross-crate constant duplication is guarded.
- **Clean:** Clean. No needless allocation in the send path (tonic client clone is a cheap channel handle), no dead code, consistent abstraction levels. expect() in runtime() and the insecure-asymmetry are the only blemishes.

### tidal-server-cluster
- **Completeness:** Solid for the replicated topology and the experimental-cluster gating; the gap is that the entity-sharded write path (sharded_* routes) ships without the stable-shard-ordering precondition its own helper documents, and the in-module tests never exercise the real regions() ordering, so a routing-divergence bug is undetected.
- **Accuracy:** One CRITICAL routing-correctness bug: entity_shard relies on `shards` being in ascending order, but every caller feeds it SimulatedCluster::regions() which is unsorted/unstable HashMap iteration — the same write/read divergence the doc claims to have fixed. Concurrency/deadline/channel logic in dispatch_shards and the write pool is otherwise carefully correct.
- **Tech Debt:** Two large multi-concern files (cluster.rs, scatter_gather.rs) with explicit deferred-split notes; the f64-score sort comparator is duplicated verbatim across retrieve/search; a couple of magic constants (oneshot retry_after_ms=50) are inline.
- **Maintainability:** Above average — intention-revealing names, thorough WHY-focused doc comments, focused helpers. The deferred file splits are acknowledged and tracked, which is acceptable.
- **Extensibility:** Good trait-based MergeItem abstraction and a generic dispatch_shards make adding new fan-out query kinds cheap. Routing strategy is hard-wired to hash-mod-len rather than going through the engine ShardRouter for the index step, which couples the server to one strategy.
- **Dry:** MergeItem/dedup/reconcile abstractions are well-factored. The score-descending sort_by closure and the entire ScatterGatherMeta→ScatterGatherInfo + result-assembly block are duplicated between scatter_gather_retrieve and scatter_gather_search.
- **Clean:** Clear and efficient; no needless hot-path allocation (workers get owned Arc clones, buffers pre-sized with_capacity). The one stain is the misleading 'ascending order' contract that callers silently violate.

### tidal-server-core
- **Completeness:** Solid — both serve paths have graceful shutdown, SIGTERM+ctrl-c, readiness flip, and config-dir fail-loud resolution with good test coverage. Gap: standalone shutdown never OBSERVES the final-flush durability result (relies on Drop, which only logs), unlike the cluster path; and a lib.rs doc pointer references a non-existent 'Known Gaps' section.
- **Accuracy:** Correct — no unwrap/expect/panic in production paths, clean clippy, constant-time auth comparison, sweeper uses Weak (no leak), io→f64 widening is lossless, shutdown-flag atomics are sound (over-strong but correct). No races or durability bugs found in the slice.
- **Tech Debt:** Low — `extract_string` is dead pass-through indirection over `extract_string_field`; two io-error variants (`Io`/`Http`) overlap in role; config crate hand-rolls enum parse tables instead of an engine-provided FromStr.
- **Maintainability:** Good — intention-revealing names, focused functions, comments explain WHY (CONCURRENCY-1 sweeper rationale, the cluster-build-thread reactor note). Minor: SeqCst vs Acquire/Release inconsistency for the same shutdown-flag concept across ServerState and ClusterState.
- **Extensibility:** Mostly good — dto/health centralization (single source of truth across standalone+cluster routers) is the right abstraction. Weak spot: adding a Window/EntityKind/SignalAgg variant in the engine silently won't be accepted by the config parser until its hand-rolled match arm is updated.
- **Dry:** One real missing abstraction: canonical string<->enum mapping for Window/EntityKind/SignalAgg lives in the engine as label()/as_str() but is re-implemented as a divergent parse table in config.rs (proven drift: engine emits 'all', parser only accepts 'all_time'). Plus the `extract_string`/`extract_string_field` synonym pair.
- **Clean:** Clean and readable; no dead code beyond the `extract_string` delegate, no hot-path allocation concerns (this is the cold control-plane layer). Mapping helpers in dto.rs are tidy and well-scoped.

### tidalctl
- **Completeness:** Strong — every command has the success, empty, missing-dir, corrupt, and degraded paths handled and tested; the exit-code contract is documented at the crate root and honored. Gaps are minor: recover's only mode is verify-only with dead flag plumbing, and scope-stats has no human-readable pretty mode (it just indents JSON) unlike recover/diagnostics.
- **Accuracy:** Sound. No unwrap/expect/panic in production code, all fallible IO returns Result, exit codes match the documented contract, the uncheckpointed-lag arithmetic is saturating and correct. One latent: as_nanos() as u64 truncates for post-2554 timestamps (cosmetic, saturating_sub already guards the common skew case).
- **Tech Debt:** Low. No magic numbers, no leaked storage-engine types (all access is via tidaldb::wal/text/governance public fns), no commented-out code, no TODOs. The recover --verify-only gate is a forward-compat stub but reads as intentional, not rot.
- **Maintainability:** Excellent. Intention-revealing names, focused functions, and doc comments that consistently explain WHY (null-vs-zero rationale, lock-free constraint, single-source reasons). format_pretty in diagnostics is long (~120 lines) but it is flat, linear string-building — readable, not nested.
- **Extensibility:** Good. One run(...) -> (String, i32) shape per command, a single render_json path, untagged WalField enum for the ok/error shape. Adding a command is a module + one match arm. The Option<_>-means-null typing makes adding offline-derivable fields safe.
- **Dry:** The weakest dimension. The six-directory projection (base/wal/items/users/creators/cache) is duplicated between status.rs DirsOutput and paths.rs PathsOutput, and the offline-unavailable field set is maintained in two places (the OFFLINE_UNAVAILABLE table and the always-None fields of DiagnosticsOutput) with no compile-time link.
- **Clean:** Clear, logical, neat. Not a hot path, so per-string allocation is fine. serde owns all JSON escaping (the old hand-rolled json_escape control-char hole is gone and regression-tested). Mixed-abstraction is avoided — pretty formatting and serializable projection are cleanly separated.